diff --git a/docs/future-tasks/code-quality-tasks/invalid-state-representations-report.md b/docs/future-tasks/code-quality-tasks/invalid-state-representations-report.md new file mode 100644 index 000000000..ae0ffd75a --- /dev/null +++ b/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 { + success: boolean; + error?: string; + message?: string; + details?: Record; +} +``` + +**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 } + | { success: false; error: string; details?: Record }; +``` + +## 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. diff --git a/docs/future-tasks/code-quality-tasks/module-graph-import-issues-report.md b/docs/future-tasks/code-quality-tasks/module-graph-import-issues-report.md new file mode 100644 index 000000000..400876ec3 --- /dev/null +++ b/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