Claude's suggestion for code improvements

docs/future-tasks/code-quality-tasks/invalid-state-representations-report.md

@@ -0,0 +1,241 @@
+# Invalid State Representations Report
+
+Based on the analysis of the codebase against AGENTS.md guidelines, the
+following interfaces allow invalid state representations that should be
+refactored:
+
+## 1. LLMRequest Interface
+
+**File**: `packages/llm/src/types.ts` **Lines**: 28-38
+
+```typescript
+export interface LLMRequest {
+  cache?: boolean;
+  messages: LLMMessage[];
+  model: ModelName;
+  system?: string;
+  maxTokens?: number;
+  stream?: boolean;
+  stop?: string;
+  mode?: "json";
+  metadata?: LLMRequestMetadata;
+}
+```
+
+**Issue**: The `cache` property being optional creates ambiguity. Code
+throughout the codebase defaults it to `true` if undefined, but this implicit
+behavior isn't clear from the interface.
+
+**Recommendation**: Rename `cache` to `noCache` with `false` as default:
+
+```typescript
+export interface LLMRequest {
+  noCache?: boolean;
+  // ... other properties
+}
+```
+
+## 2. OAuth2Tokens Interface
+
+**File**:
+`packages/toolshed/routes/integrations/google-oauth/google-oauth.utils.ts`
+**Lines**: 9-16
+
+```typescript
+export interface OAuth2Tokens {
+  accessToken: string;
+  refreshToken?: string;
+  expiresIn?: number;
+  tokenType: string;
+  scope?: string[];
+  expiresAt?: number;
+}
+```
+
+**Issue**: `refreshToken` is optional but critical for token renewal. Code has
+to handle cases where it might be missing, leading to potential authentication
+failures.
+
+**Recommendation**: Create separate types for initial and renewable tokens:
+
+```typescript
+export interface InitialOAuth2Tokens {
+  accessToken: string;
+  tokenType: string;
+  expiresIn: number;
+  scope: string[];
+}
+
+export interface RenewableOAuth2Tokens extends InitialOAuth2Tokens {
+  refreshToken: string;
+  expiresAt: number;
+}
+```
+
+## 3. UserInfo Interface
+
+**File**:
+`packages/toolshed/routes/integrations/google-oauth/google-oauth.utils.ts`
+**Lines**: 18-28
+
+```typescript
+export interface UserInfo {
+  id?: string;
+  email?: string;
+  verified_email?: boolean;
+  name?: string;
+  given_name?: string;
+  family_name?: string;
+  picture?: string;
+  locale?: string;
+  error?: string;
+}
+```
+
+**Issue**: Mixes success and error states in one interface. Either you have user
+data OR an error, never both.
+
+**Recommendation**: Use discriminated union:
+
+```typescript
+type UserInfoResult =
+  | {
+    success: true;
+    data: {
+      id: string;
+      email: string;
+      verified_email: boolean;
+      name: string;
+      // ... other fields
+    };
+  }
+  | { success: false; error: string };
+```
+
+## 4. CallbackResult Interface
+
+**File**:
+`packages/toolshed/routes/integrations/google-oauth/google-oauth.utils.ts`
+**Lines**: 30-35
+
+```typescript
+export interface CallbackResult extends Record<string, unknown> {
+  success: boolean;
+  error?: string;
+  message?: string;
+  details?: Record<string, unknown>;
+}
+```
+
+**Issue**: When `success` is false, `error` should be required. When `success`
+is true, `error` shouldn't exist.
+
+**Recommendation**: Use discriminated union:
+
+```typescript
+type CallbackResult =
+  | { success: true; message?: string; details?: Record<string, unknown> }
+  | { success: false; error: string; details?: Record<string, unknown> };
+```
+
+## 5. BackgroundCharmServiceOptions Interface
+
+**File**: `packages/background-charm-service/src/service.ts` **Lines**: 12-19
+
+```typescript
+export interface BackgroundCharmServiceOptions {
+  identity: Identity;
+  toolshedUrl: string;
+  runtime: Runtime;
+  bgSpace?: string;
+  bgCause?: string;
+  workerTimeoutMs?: number;
+}
+```
+
+**Issue**: Optional properties have defaults (`BG_SYSTEM_SPACE_ID` and
+`BG_CELL_CAUSE`) applied in constructor, but this isn't clear from the
+interface.
+
+**Recommendation**: Make defaults explicit in the type:
+
+```typescript
+export interface BackgroundCharmServiceOptions {
+  identity: Identity;
+  toolshedUrl: string;
+  runtime: Runtime;
+  bgSpace: string;
+  bgCause: string;
+  workerTimeoutMs: number;
+}
+
+export const DEFAULT_BG_OPTIONS = {
+  bgSpace: BG_SYSTEM_SPACE_ID,
+  bgCause: BG_CELL_CAUSE,
+  workerTimeoutMs: 30000,
+} as const;
+```
+
+## 6. Module Interface
+
+**File**: `packages/builder/src/types.ts` **Lines**: 182-188
+
+```typescript
+export interface Module {
+  type: "ref" | "javascript" | "recipe" | "raw" | "isolated" | "passthrough";
+  implementation?: ((...args: any[]) => any) | Recipe | string;
+  wrapper?: "handler";
+  argumentSchema?: JSONSchema;
+  resultSchema?: JSONSchema;
+}
+```
+
+**Issue**: `implementation` is optional but certain module types require it. The
+relationship between `type` and required properties isn't enforced.
+
+**Recommendation**: Use discriminated unions:
+
+```typescript
+type Module =
+  | {
+    type: "ref";
+    implementation: string;
+    wrapper?: "handler";
+    argumentSchema?: JSONSchema;
+    resultSchema?: JSONSchema;
+  }
+  | {
+    type: "javascript";
+    implementation: (...args: any[]) => any;
+    wrapper?: "handler";
+    argumentSchema?: JSONSchema;
+    resultSchema?: JSONSchema;
+  }
+  | {
+    type: "recipe";
+    implementation: Recipe;
+    wrapper?: "handler";
+    argumentSchema?: JSONSchema;
+    resultSchema?: JSONSchema;
+  }
+  | { type: "raw"; implementation: string }
+  | { type: "isolated"; implementation: string }
+  | { type: "passthrough" };
+```
+
+## Impact of These Issues
+
+1. **Runtime Errors**: Optional properties that are actually required lead to
+   runtime failures
+2. **Defensive Programming**: Developers must add null checks everywhere,
+   cluttering the code
+3. **Hidden Dependencies**: Implicit defaults make it hard to understand the
+   true requirements
+4. **Type Safety Loss**: TypeScript can't catch invalid combinations of
+   properties
+
+## General Principles from AGENTS.md
+
+"Making invalid states unrepresentable is good" - These interfaces violate this
+principle by allowing combinations of properties that shouldn't exist together
+or by making required properties optional.

docs/future-tasks/code-quality-tasks/module-graph-import-issues-report.md

@@ -0,0 +1,144 @@
+# Module Graph and Import Issues Report
+
+Based on the analysis of the codebase against AGENTS.md guidelines, the
+following module graph and import issues were found:
+
+## 1. Default Exports Instead of Named Exports
+
+The following files violate the "prefer named exports over default exports"
+guideline:
+
+### Component Files
+
+- `packages/charm/src/iframe/recipe.ts`
+- `packages/charm/src/commands.ts`
+- `packages/html/src/render.ts`
+- `packages/html/src/path.ts`
+- `packages/memory/clock.ts` - `export default new Clock();`
+
+### Plugin Files
+
+- `packages/deno-vite-plugin/src/index.ts`
+- `packages/deno-vite-plugin/src/resolvePlugin.ts`
+- `packages/deno-vite-plugin/src/prefixPlugin.ts`
+
+### View Components
+
+- `packages/jumble/src/assets/ShapeLogo.tsx`
+- `packages/jumble/src/views/CharmView.tsx`
+- `packages/jumble/src/views/ErrorBoundaryView.tsx`
+- `packages/jumble/src/views/DebugView.tsx`
+- Multiple other view files in `packages/jumble/src/views/`
+
+**Example Fix**:
+
+```typescript
+// Bad
+export default class Clock { ... }
+
+// Good
+export class Clock { ... }
+
+// Bad
+export default new Clock();
+
+// Good
+export const clock = new Clock();
+// Or better: export a factory function
+export function createClock(): Clock { ... }
+```
+
+## 2. Missing or Incorrect Import Grouping
+
+The following files don't follow the import grouping convention (standard
+library → external → internal):
+
+### `packages/jumble/src/components/CharmRunner.tsx`
+
+```typescript
+// Current (mixed imports)
+import { html } from "lit";
+import { Recipe } from "@commontools/common-runner";
+import { SpellRunner } from "./SpellRunner.js";
+
+// Should be:
+// Standard library
+import { html } from "lit";
+
+// External
+// (none in this case)
+
+// Internal
+import { Recipe } from "@commontools/common-runner";
+import { SpellRunner } from "./SpellRunner.js";
+```
+
+### `packages/builder/src/utils.ts`
+
+```typescript
+// Current (internal packages mixed without grouping)
+import { JSONSchema7 } from "json-schema";
+import { isOpaqueRef, OpaqueRef } from "./spell.js";
+import { diffAndUpdate, maybeGetCellLink } from "@commontools/runner";
+
+// Should be:
+// Standard library
+// (none)
+
+// External
+import { JSONSchema7 } from "json-schema";
+
+// Internal
+import { diffAndUpdate, maybeGetCellLink } from "@commontools/runner";
+import { isOpaqueRef, OpaqueRef } from "./spell.js";
+```
+
+### `packages/identity/src/ed25519/index.ts`
+
+- External imports (`@scure/bip39`) mixed with internal imports without proper
+  grouping
+
+## 3. Module-Specific Dependencies (Good Practices Found)
+
+The codebase correctly follows the guideline for module-specific dependencies:
+
+### Good Examples:
+
+- `packages/llm/deno.json` - Correctly declares `json5` dependency
+- `packages/ui/deno.json` - Correctly declares `@shoelace-style/shoelace`
+  dependency
+- Most packages don't have unnecessary dependencies in their `deno.json`
+
+**This is a positive finding** - the codebase correctly avoids adding
+package-specific dependencies to the workspace `deno.json`.
+
+## Recommendations
+
+### 1. Convert Default to Named Exports
+
+```typescript
+// For all identified files:
+// Change: export default Clock
+// To:     export { Clock }
+
+// Change: export default new Clock()
+// To:     export const clock = new Clock()
+//         or export function createClock(): Clock
+```
+
+### 2. Enforce Import Grouping
+
+Add ESLint rule or formatter configuration to enforce:
+
+1. Standard library imports
+2. External package imports
+3. Internal package imports
+4. Relative imports
+
+## Impact Summary
+
+- **Circular dependencies**: Critical issue affecting maintainability and
+  testability
+- **Default exports**: Medium impact - affects consistency and tree-shaking
+- **Import grouping**: Low impact - affects readability
+- **Heavy utilities**: Medium impact - affects bundle size and modularity