From db282ab0f4c0b174d78c656bf0ae28077cb30da2 Mon Sep 17 00:00:00 2001 From: Bernhard Seefeld Date: Mon, 2 Jun 2025 16:37:44 -0700 Subject: [PATCH 1/2] Claude's suggestion for code improvements --- .../ambiguous-types-report.md | 115 ++++++ .../inappropriate-error-handling-report.md | 282 +++++++++++++++ .../invalid-state-representations-report.md | 336 ++++++++++++++++++ .../module-graph-import-issues-report.md | 191 ++++++++++ .../singleton-patterns-report.md | 151 ++++++++ 5 files changed, 1075 insertions(+) create mode 100644 docs/code-quality-tasks/ambiguous-types-report.md create mode 100644 docs/code-quality-tasks/inappropriate-error-handling-report.md create mode 100644 docs/code-quality-tasks/invalid-state-representations-report.md create mode 100644 docs/code-quality-tasks/module-graph-import-issues-report.md create mode 100644 docs/code-quality-tasks/singleton-patterns-report.md diff --git a/docs/code-quality-tasks/ambiguous-types-report.md b/docs/code-quality-tasks/ambiguous-types-report.md new file mode 100644 index 000000000..f72fe3b6a --- /dev/null +++ b/docs/code-quality-tasks/ambiguous-types-report.md @@ -0,0 +1,115 @@ +# Ambiguous Types and Inappropriate 'any' Usage Report + +Based on the search through the TypeScript files in the packages/ directory, I've found several patterns of inappropriate type usage according to AGENTS.md guidelines: + +## 1. Direct Usage of 'any' Type + +### packages/runner/src/utils.ts +- **Line 31**: `extractDefaultValues(schema: any): any` - Function accepts and returns 'any' without proper type validation +- **Line 35**: `const obj: any = {}` - Local variable using 'any' +- **Line 58**: `mergeObjects(...objects: any[]): any` - Function accepts array of 'any' and returns 'any' +- **Line 64**: `const result: any = {}` - Local variable using 'any' +- **Line 107-110**: `sendValueToBinding(doc: DocImpl, binding: any, value: any, log?: ReactivityLog)` - Multiple 'any' parameters +- **Line 137-139**: `setNestedValue(doc: DocImpl, path: PropertyKey[], value: any, log?: ReactivityLog)` - 'any' in generic and parameter +- **Line 167-169**: Multiple functions in traverse.ts using 'any' parameters (lines ~221-224) +- **Line 256**: `unsafe_noteParentOnRecipes(recipe: Recipe, binding: any)` - Accepts 'any' parameter +- **Line 283**: `findAllAliasedCells(binding: any, doc: DocImpl)` - Multiple 'any' usage +- **Line 540-546**: `maybeGetCellLink(value: any, parent?: DocImpl)` - 'any' parameters +- **Line 555**: `followAliases(alias: any, doc: DocImpl, log?: ReactivityLog)` - 'any' parameter +- **Line 583**: `diffAndUpdate(current: CellLink, newValue: any, log?: ReactivityLog, context?: any)` - Multiple 'any' +- **Line 616-620**: `normalizeAndDiff(current: CellLink, newValue: any, log?: ReactivityLog, context?: any)` - Multiple 'any' +- **Line 896**: `addCommonIDfromObjectID(obj: any, fieldName: string = "id")` - 'any' parameter +- **Line 915**: `maybeUnwrapProxy(value: any): any` - Accepts and returns 'any' +- **Line 932**: `containsOpaqueRef(value: any): boolean` - 'any' parameter +- **Line 941**: `deepCopy(value: any): any` - Accepts and returns 'any' + +### packages/js-runtime/interface.ts +- **Line 12**: `invoke(...args: any[]): JsValue` - Array of 'any' as parameters +- **Line 13**: `inner(): any` - Returns 'any' + +### packages/builder/src/built-in.ts +- **Line 24**: `error: any` - Property typed as 'any' +- **Line 44**: `error: any` - Property typed as 'any' +- **Line 55**: `error: any` - Property typed as 'any' +- **Line 57**: `ifElse` - Generic parameters defaulting to 'any' +- **Line 69**: `let ifElseFactory: NodeFactory<[any, any, any], any> | undefined` - Multiple 'any' in generic +- **Line 74**: `(cell: OpaqueRef) => OpaqueRef` - 'any' in generic +- **Line 82**: `...values: any[]` - Array of 'any' +- **Line 89**: `values: any[]` - Array of 'any' + +### packages/memory/traverse.ts +- **Line 167-169**: `[k, this.traverseDAG(doc, docRoot, v, tracker)]),` - Using 'any' in type assertion +- **Line 355**: `isPointer(value: any): boolean` - 'any' parameter +- **Line 365**: `isJSONCellLink(value: any): value is JSONCellLink` - 'any' parameter +- **Line 371-379**: Multiple functions accepting 'any' parameters + +### packages/html/src/render.ts +- **Line 60-70**: Comments indicate potential issues with type handling but implementation uses proper types + +## 2. Type Assertions Using 'as any' + +### packages/runner/src/runner.ts +- **Line ~224**: Type assertion `as any` in runtime context + +### packages/runner/src/cell.ts +- **Line ~118**: `(self.key as any)(key).set(value)` - Type assertion to bypass type checking + +### packages/runner/src/query-result-proxy.ts +- **Line ~42**: `const target = valueCell.getAtPath(valuePath) as any` - Type assertion +- **Line ~120**: `undefined as unknown as any[]` - Multiple type assertions + +### packages/builder/src/spell.ts +- **Line ~25**: `if (typeof prop === "symbol") return (target as any)[prop]` - Type assertion +- **Line ~48**: `[key, (self as any)[key]]` - Type assertion in mapping + +### packages/builder/src/utils.ts +- **Line ~117**: `for (const key in value as any)` - Type assertion in loop + +### packages/builder/src/recipe.ts +- **Lines ~182-186**: Multiple `as any` assertions when deleting properties + +### packages/memory/traverse.ts +- **Line ~169**: Type assertion in mapping function + +## 3. Overly Broad Union Types + +While I didn't find the specific pattern `string | number | object | null`, there are several places with overly permissive types: + +### packages/builder/src/types.ts +- **Line ~244**: `cell?: any` in Alias type - Should be more specific +- **Line ~263**: `implementation?: ((...args: any[]) => any) | Recipe | string` - Function signature with 'any' + +## 4. Functions Accepting Overly Broad Types Without Validation + +### packages/runner/src/utils.ts +- `extractDefaultValues`, `mergeObjects`, `sendValueToBinding`, `setNestedValue` - All accept 'any' without proper type guards or validation +- These functions perform operations on the values but don't validate the types first + +### packages/memory/traverse.ts +- Multiple traversal functions accept 'any' or 'unknown' without proper validation + +## 5. Unknown Types That Should Be More Specific + +### packages/js-runtime/interface.ts +- **Lines 70-79**: `isJsModule` function uses 'unknown' but could use a more specific base type +- **Lines 97-104**: `isSourceMap` function uses 'unknown' but could use a more specific base type + +### packages/memory/traverse.ts +- **Lines 371-389**: Multiple functions use 'unknown' parameters that could be more specific + +## Recommendations + +1. **Replace 'any' with specific types or generics** where possible +2. **Add proper type guards** for functions that need to accept broad types +3. **Use discriminated unions** instead of 'any' for values that can be multiple types +4. **Avoid type assertions** (`as any`) - instead, use type guards or proper typing +5. **Document why 'any' is necessary** in the few cases where it truly is needed +6. **Consider using 'unknown'** instead of 'any' and add type guards for safety + +## Priority Files to Fix + +1. **packages/runner/src/utils.ts** - Has the most 'any' usage +2. **packages/builder/src/built-in.ts** - Core functionality with multiple 'any' +3. **packages/memory/traverse.ts** - Complex traversal logic with 'any' +4. **packages/builder/src/types.ts** - Type definitions themselves use 'any' +5. **packages/js-runtime/interface.ts** - Interface definitions with 'any' \ No newline at end of file diff --git a/docs/code-quality-tasks/inappropriate-error-handling-report.md b/docs/code-quality-tasks/inappropriate-error-handling-report.md new file mode 100644 index 000000000..a5e3b570e --- /dev/null +++ b/docs/code-quality-tasks/inappropriate-error-handling-report.md @@ -0,0 +1,282 @@ +# Inappropriate Error Handling Report + +Based on the analysis of the codebase against AGENTS.md guidelines, the +following inappropriate error handling patterns were found: + +## 1. Try/catch Blocks That Silently Swallow Errors + +### `packages/jumble/src/components/NetworkInspector.tsx` + +**Lines**: 236-238 + +```typescript +} catch (e) { + return null; +} +``` + +**Issue**: Silently catches errors and returns null without any logging or +re-throwing. This makes debugging impossible and hides potential issues. + +### `packages/toolshed/lib/otel.ts` + +**Lines**: 60-62 + +```typescript +} catch (_) { + // ignored – not running on Deno with telemetry support +} +``` + +**Issue**: Empty catch block that completely swallows errors. Even if the error +is expected, it should at least be logged for debugging purposes. + +## 2. Different Error Handling Approaches in the Same Code Path + +### `packages/runner/src/runner.ts` + +**Lines**: 358-363 + +```typescript +try { + cancel(); +} catch (error) { + console.warn("Error canceling operation:", error); +} +``` + +**Issue**: Uses `console.warn` for errors in a method that should likely +propagate errors up to the caller for proper handling. + +### `packages/js-runtime/typescript/compiler.ts` + +**Lines**: 111-114 + +```typescript +} catch (e) { + console.warn(`There was an error parsing "${key}" source map: ${e}`); +} +``` + +**Issue**: Uses `console.warn` instead of throwing or properly handling the +parsing error. Source map parsing errors could indicate serious issues. + +### `packages/llm/src/client.ts` + +**Lines**: 101-103, 115-117 + +```typescript +} catch (error) { + console.error("Failed to parse JSON line:", line, error); +} +``` + +**Issue**: Logs errors to console instead of propagating them, while other parts +of the same method throw errors. This creates inconsistent error handling. + +## 3. Functions Returning undefined/null on Error Instead of Throwing + +### `packages/jumble/src/components/NetworkInspector.tsx` + +**Lines**: 213-239 + +```typescript +const extractTransactionDetails = (value: any): any => { + try { + // ... processing logic ... + } catch (e) { + return null; + } +}; +``` + +**Issue**: Returns null on any error instead of throwing or properly handling +specific error cases. Callers can't distinguish between "no data" and "error +occurred". + +### `packages/ui/src/components/common-form.ts` + +**Lines**: 64-66 + +```typescript +} catch { + return "Invalid reference format"; +} +``` + +**Issue**: Returns an error string instead of throwing, inconsistent with other +validation patterns that throw errors. + +### `packages/charm/src/spellbook.ts` + +```typescript +} catch (error) { + console.error("Failed to save spell:", error); + return false; +} +``` + +**Issue**: Returns false on error instead of throwing, preventing callers from +understanding why the operation failed. + +## 4. Overly Broad try/catch Blocks + +### `packages/charm/src/manager.ts` + +**Lines**: 500-700+ + +```typescript +try { + // Hundreds of lines of code including: + // - Multiple async operations + // - Complex data transformations + // - Nested function calls +} catch (error) { + console.debug("Error in findReferencedCharms:", error); +} +``` + +**Issue**: Try block covers too much code, making it difficult to understand +which operation actually failed. Errors from very different operations are +handled identically. + +## 5. Low-level try/catch Where Errors Should Propagate + +### `packages/charm/src/workflow.ts` + +Multiple instances of: + +```typescript +try { + // Some operation +} catch (error) { + console.warn("Operation failed:", error); + // Continues execution +} +``` + +**Issue**: Error handling at the workflow processing level prevents higher-level +error handlers from taking appropriate action (retry, fallback, user +notification). + +### `packages/background-charm-service/src/worker.ts` + +**Lines**: 186-188 + +```typescript +} catch (error) { + const errorMessage = String(error); + // Loses stack trace and error type information +} +``` + +**Issue**: Converts errors to strings, losing valuable debugging information +like stack traces and error types. + +## 6. Missing Error Context or Unhelpful Error Messages + +### `packages/toolshed/routes/integrations/google-oauth/google-oauth.utils.ts` + +**Lines**: 162, 195 + +```typescript +throw new Error(`Failed to get auth cell: ${error}`); +throw new Error(`Error persisting tokens: ${error}`); +``` + +**Issue**: Simply wraps errors without adding meaningful context about what was +being attempted, what the inputs were, or suggestions for resolution. + +## 7. Console Logging in Production Code + +Multiple files use console methods for error handling instead of proper error +propagation: + +- `packages/charm/src/format.ts` - Uses `console.error` +- `packages/charm/src/search.ts` - Uses `console.warn` +- `packages/charm/src/workflow.ts` - Mixed `console.warn` and `console.error` +- `packages/charm/src/iframe/recipe.ts` - Uses `console.log` for errors + +**Issue**: Console logging is not appropriate for production error handling. +Errors should be propagated or logged through proper logging mechanisms. + +## Recommendations Based on AGENTS.md + +### 1. Let Errors Propagate + +```typescript +// Good - let caller handle the error +async function getData(): Promise { + const res = await fetch(URL); + if (res.ok) { + return res.text(); + } + throw new Error("Unsuccessful HTTP response"); +} + +// Bad - swallowing errors +async function getData(): Promise { + try { + const res = await fetch(URL); + if (res.ok) { + return res.text(); + } + } catch (e) { + console.error(e); + } +} +``` + +### 2. Only Catch When You Can Handle + +```typescript +// Good - specific error handling with retry +async function fetchWithRetry(url: string, retries = 3): Promise { + for (let i = 0; i < retries; i++) { + try { + return await fetch(url); + } catch (error) { + if (i === retries - 1) throw error; + await delay(1000 * Math.pow(2, i)); // exponential backoff + } + } + throw new Error("Should not reach here"); +} +``` + +### 3. Add Meaningful Context + +```typescript +// Good - adds context to errors +try { + await saveUserData(userId, data); +} catch (error) { + throw new Error( + `Failed to save data for user ${userId}: ${error.message}`, + { cause: error }, + ); +} +``` + +### 4. Use Type Guards for Expected Errors + +```typescript +// Good - handle specific, expected scenarios +function isFeatureSupported(): boolean { + try { + // Try to use feature + crypto.subtle.importKey(...); + return true; + } catch { + // Expected error - feature not supported + return false; + } +} +``` + +## Priority Areas to Fix + +1. **High Priority**: Error swallowing in core libraries (`runner`, `builder`, + `charm`) +2. **Medium Priority**: Console logging instead of proper error handling +3. **Low Priority**: Missing error context in utility functions diff --git a/docs/code-quality-tasks/invalid-state-representations-report.md b/docs/code-quality-tasks/invalid-state-representations-report.md new file mode 100644 index 000000000..2a8c16885 --- /dev/null +++ b/docs/code-quality-tasks/invalid-state-representations-report.md @@ -0,0 +1,336 @@ +# 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**: Make `cache` required with explicit default: + +```typescript +export interface LLMRequest { + cache: boolean; // Always explicit, no ambiguity + // ... other properties +} +``` + +## 2. WorkflowForm Interface + +**File**: `packages/charm/src/workflow.ts` **Lines**: 299-348 + +```typescript +export interface WorkflowForm { + classification: { + workflowType: WorkflowType; + confidence: number; + reasoning: string; + } | null; + + plan: { + features?: string[]; + description?: string; + charms?: CharmSearchResult[]; + } | null; + + generation: { + charm: Cell; + } | null; + + searchResults: { + castable: Record<...> + } | null; + + spellToCast: { + charmId: string; + spellId: string; + } | null; +} +``` + +**Issue**: Using `null` to represent "not yet processed" states requires +constant null checking. This doesn't make invalid states unrepresentable. + +**Recommendation**: Use discriminated unions: + +```typescript +type WorkflowForm = + | { state: 'unclassified' } + | { state: 'classified'; classification: {...}; plan: null } + | { state: 'planned'; classification: {...}; plan: {...}; generation: null } + | { state: 'generated'; classification: {...}; plan: {...}; generation: {...} } + // etc. +``` + +## 3. 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; +} +``` + +## 4. 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 }; +``` + +## 5. 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 }; +``` + +## 6. RuntimeOptions Interface + +**File**: `packages/runner/src/runtime.ts` **Lines**: 43-51 + +```typescript +export interface RuntimeOptions { + storageUrl: string; + signer?: Signer; + consoleHandler?: ConsoleHandler; + errorHandlers?: ErrorHandler[]; + blobbyServerUrl?: string; + recipeEnvironment?: RecipeEnvironment; + debug?: boolean; +} +``` + +**Issue**: Optional properties have implicit defaults that aren't clear from the +interface. Code later throws if `storageUrl` doesn't exist (line 275). + +**Recommendation**: Make defaults explicit: + +```typescript +export interface RuntimeOptions { + storageUrl: string; + signer: Signer | null; + consoleHandler: ConsoleHandler | null; + errorHandlers: ErrorHandler[]; + blobbyServerUrl: string | null; + recipeEnvironment: RecipeEnvironment | null; + debug: boolean; +} + +// With a factory function for defaults: +export function createRuntimeOptions( + partial: PartialRuntimeOptions, +): RuntimeOptions { + return { + storageUrl: partial.storageUrl, // required + signer: partial.signer ?? null, + consoleHandler: partial.consoleHandler ?? defaultConsoleHandler, + errorHandlers: partial.errorHandlers ?? [], + blobbyServerUrl: partial.blobbyServerUrl ?? null, + recipeEnvironment: partial.recipeEnvironment ?? null, + debug: partial.debug ?? false, + }; +} +``` + +## 7. 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; +``` + +## 8. 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/code-quality-tasks/module-graph-import-issues-report.md b/docs/code-quality-tasks/module-graph-import-issues-report.md new file mode 100644 index 000000000..c8a8d20cb --- /dev/null +++ b/docs/code-quality-tasks/module-graph-import-issues-report.md @@ -0,0 +1,191 @@ +# 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. Circular Dependencies Between Modules + +### Critical Circular Dependency: `@commontools/builder` ↔ `@commontools/runner` + +**Builder → Runner imports:** +- `packages/builder/src/built-in.ts` imports from `@commontools/runner` +- `packages/builder/src/schema-to-ts.ts` imports from `@commontools/runner` +- `packages/builder/src/utils.ts` imports from `@commontools/runner` + +**Runner → Builder imports:** +- `packages/runner/src/cfc.ts` imports from `@commontools/builder` +- `packages/runner/src/recipe-manager.ts` imports from `@commontools/builder` +- `packages/runner/src/runner.ts` imports from `@commontools/builder` +- `packages/runner/src/env.ts` imports from `@commontools/builder` +- `packages/runner/src/doc-map.ts` imports from `@commontools/builder` +- `packages/runner/src/cell.ts` imports from `@commontools/builder` + +**Impact**: This circular dependency makes it impossible to: +- Test these modules in isolation +- Understand the module hierarchy +- Reuse either module independently +- Avoid loading unnecessary code + +**Recommendation**: Extract shared types and interfaces to a separate package like `@commontools/core-types` or `@commontools/shared`. + +## 2. 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 { ... } +``` + +## 3. 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 + +## 4. Heavy Dependencies in Utility Modules + +### `packages/builder/src/utils.ts` +**Issue**: This utility module imports from `@commontools/runner`, making it a heavyweight utility rather than a lightweight one. + +**Impact**: +- Any code that needs simple utilities from `builder` must also load the entire `runner` package +- Creates the circular dependency issue mentioned above +- Violates the principle that utility modules should have minimal dependencies + +**Recommendation**: +- Move utilities that depend on `runner` to a separate module like `builder-runner-utils.ts` +- Keep pure utilities without external dependencies in `utils.ts` + +## 5. 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`. + +## 6. Non-standard JS Usage + +**Positive Finding**: The codebase is clean of non-portable JavaScript patterns: +- No direct use of environment variables in module scope +- No Vite-specific APIs (`import.meta.hot`, `import.meta.glob`) outside of Vite plugin code +- Clean separation between build-time and runtime code + +## Recommendations + +### 1. Break Circular Dependencies +Create a new package structure: +``` +@commontools/core-types // Shared interfaces and types +@commontools/builder // Depends on core-types +@commontools/runner // Depends on core-types and builder +``` + +### 2. 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 +``` + +### 3. 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 + +### 4. Refactor Heavy Utilities +Split `packages/builder/src/utils.ts`: +- `utils.ts` - Pure utilities with no external dependencies +- `runner-utils.ts` - Utilities that need runner functionality + +### 5. Document Module Dependencies +Create a module dependency graph documentation to help developers understand: +- Which packages can be used independently +- Which packages have peer dependencies +- The intended hierarchy of packages + +## 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 \ No newline at end of file diff --git a/docs/code-quality-tasks/singleton-patterns-report.md b/docs/code-quality-tasks/singleton-patterns-report.md new file mode 100644 index 000000000..cf439f0d4 --- /dev/null +++ b/docs/code-quality-tasks/singleton-patterns-report.md @@ -0,0 +1,151 @@ +# Singleton Patterns Report + +Based on the analysis of the codebase against AGENTS.md guidelines, the +following singleton patterns were found that should be refactored: + +## 1. Module-level Maps and Caches + +### `packages/deno-vite-plugin/src/resolver.ts` + +- **Line 75**: `const resolverCache = new Map();` +- **Issue**: Global cache that prevents multiple instances and complicates + testing + +### `packages/static/index.ts` + +- **Line 5**: `const cache = new StaticCache();` +- **Issue**: Exported singleton cache instance + +## 2. Exported Singleton Instances + +### `packages/toolshed/lib/otel.ts` + +- **Line 20**: `export const otlpExporter = new OTLPTraceExporter(...)` +- **Line 26**: `export const provider = new BasicTracerProvider(...)` +- **Issue**: Global telemetry instances that can't be mocked or isolated for + testing + +### `packages/toolshed/routes/storage/blobby/utils.ts` + +- **Line 7**: `export const storage = new DiskStorage(DATA_DIR);` +- **Line 10**: `let redisClient: RedisClientType | null = null;` +- **Issue**: Global storage instance and module-level Redis client state + +### `packages/memory/clock.ts` + +- **Line 20**: `export default new Clock();` +- **Issue**: Default export of singleton instance prevents multiple clock + instances + +## 3. Module-level TextEncoder/TextDecoder Instances + +### `packages/identity/src/identity.ts` + +- **Line 11**: `const textEncoder = new TextEncoder();` + +### `packages/utils/src/encoding.ts` + +- **Line 1**: `const encoder = new TextEncoder();` +- **Line 2**: `const decoder = new TextDecoder();` +- **Issue**: While less problematic, these could be passed as dependencies for + better testability + +## 4. Module-level Configuration Objects + +### `packages/background-charm-service/src/env.ts` + +- **Line 42**: `export const env = loadEnv();` +- **Issue**: Global environment configuration that can't be easily mocked + +### `packages/toolshed/lib/constants.ts` + +- **Lines 4-8**: `export const ZOD_ERROR_MESSAGES = { ... }` +- **Lines 10-12**: `export const ZOD_ERROR_CODES = { ... }` +- **Lines 14-16**: + `export const notFoundSchema = createMessageObjectSchema(...)` +- **Issue**: Global constants that might need different values in different + contexts + +### `packages/runner/src/query-result-proxy.ts` + +- **Lines 14-46**: + `const arrayMethods: { [key: string]: ArrayMethodType } = { ... }` +- **Issue**: Module-level method mapping + +## 5. Module-level State and Initialization + +### `packages/memory/deno.ts` + +- **Lines 8-18**: `const serviceDid: DID = (() => { ... })();` +- **Line 20**: `const storePath = ...` +- **Line 21**: `const STORE = new URL(...)` +- **Lines 22-29**: Module-level provider initialization +- **Issue**: Complex module-level initialization that runs on import + +### `packages/toolshed/lib/otel.ts` + +- **Line 17**: `let _providerRegistered = false;` +- **Line 18**: `let _provider: BasicTracerProvider | undefined;` +- **Issue**: Module-level state tracking for provider registration + +## Refactoring Recommendations + +According to AGENTS.md, these patterns should be refactored using: + +### 1. Class-based Patterns + +```typescript +// Instead of: const cache = new Map(); +export class Cache { + private map: Map = new Map(); + get(key: string): string | undefined { + return this.map.get(key); + } + set(key: string, value: string) { + this.map.set(key, value); + } +} +``` + +### 2. Functional Patterns + +```typescript +// Instead of module-level state +export type Cache = Map; +export const get = (cache: Cache, key: string): string | undefined => + cache.get(key); +export const set = (cache: Cache, key: string, value: string) => + cache.set(key, value); +``` + +### 3. Dependency Injection + +```typescript +// Instead of: export const storage = new DiskStorage(DATA_DIR); +export interface StorageProvider { + storage: DiskStorage; +} + +export function createStorageProvider(dataDir: string): StorageProvider { + return { + storage: new DiskStorage(dataDir), + }; +} +``` + +## Impact + +These singleton patterns: + +- Make unit testing difficult or impossible +- Prevent running multiple instances of the application +- Create tight coupling between modules +- Make it hard to mock dependencies +- Can cause state leakage between tests + +## Priority + +1. **High Priority**: Fix singletons in core modules like `memory`, `toolshed`, + and `runner` +2. **Medium Priority**: Fix utility singletons like encoders and caches +3. **Low Priority**: Fix configuration objects that rarely change From a03ac33a0976c24fdfe3c91a878f1acc495d835d Mon Sep 17 00:00:00 2001 From: Bernhard Seefeld Date: Tue, 10 Jun 2025 14:20:05 -0700 Subject: [PATCH 2/2] moved directory & removed done/outdated/soon-to-be-redundant suggestions --- .../ambiguous-types-report.md | 115 ------- .../inappropriate-error-handling-report.md | 282 ------------------ .../singleton-patterns-report.md | 151 ---------- .../invalid-state-representations-report.md | 109 +------ .../module-graph-import-issues-report.md | 105 ++----- 5 files changed, 36 insertions(+), 726 deletions(-) delete mode 100644 docs/code-quality-tasks/ambiguous-types-report.md delete mode 100644 docs/code-quality-tasks/inappropriate-error-handling-report.md delete mode 100644 docs/code-quality-tasks/singleton-patterns-report.md rename docs/{ => future-tasks}/code-quality-tasks/invalid-state-representations-report.md (67%) rename docs/{ => future-tasks}/code-quality-tasks/module-graph-import-issues-report.md (50%) diff --git a/docs/code-quality-tasks/ambiguous-types-report.md b/docs/code-quality-tasks/ambiguous-types-report.md deleted file mode 100644 index f72fe3b6a..000000000 --- a/docs/code-quality-tasks/ambiguous-types-report.md +++ /dev/null @@ -1,115 +0,0 @@ -# Ambiguous Types and Inappropriate 'any' Usage Report - -Based on the search through the TypeScript files in the packages/ directory, I've found several patterns of inappropriate type usage according to AGENTS.md guidelines: - -## 1. Direct Usage of 'any' Type - -### packages/runner/src/utils.ts -- **Line 31**: `extractDefaultValues(schema: any): any` - Function accepts and returns 'any' without proper type validation -- **Line 35**: `const obj: any = {}` - Local variable using 'any' -- **Line 58**: `mergeObjects(...objects: any[]): any` - Function accepts array of 'any' and returns 'any' -- **Line 64**: `const result: any = {}` - Local variable using 'any' -- **Line 107-110**: `sendValueToBinding(doc: DocImpl, binding: any, value: any, log?: ReactivityLog)` - Multiple 'any' parameters -- **Line 137-139**: `setNestedValue(doc: DocImpl, path: PropertyKey[], value: any, log?: ReactivityLog)` - 'any' in generic and parameter -- **Line 167-169**: Multiple functions in traverse.ts using 'any' parameters (lines ~221-224) -- **Line 256**: `unsafe_noteParentOnRecipes(recipe: Recipe, binding: any)` - Accepts 'any' parameter -- **Line 283**: `findAllAliasedCells(binding: any, doc: DocImpl)` - Multiple 'any' usage -- **Line 540-546**: `maybeGetCellLink(value: any, parent?: DocImpl)` - 'any' parameters -- **Line 555**: `followAliases(alias: any, doc: DocImpl, log?: ReactivityLog)` - 'any' parameter -- **Line 583**: `diffAndUpdate(current: CellLink, newValue: any, log?: ReactivityLog, context?: any)` - Multiple 'any' -- **Line 616-620**: `normalizeAndDiff(current: CellLink, newValue: any, log?: ReactivityLog, context?: any)` - Multiple 'any' -- **Line 896**: `addCommonIDfromObjectID(obj: any, fieldName: string = "id")` - 'any' parameter -- **Line 915**: `maybeUnwrapProxy(value: any): any` - Accepts and returns 'any' -- **Line 932**: `containsOpaqueRef(value: any): boolean` - 'any' parameter -- **Line 941**: `deepCopy(value: any): any` - Accepts and returns 'any' - -### packages/js-runtime/interface.ts -- **Line 12**: `invoke(...args: any[]): JsValue` - Array of 'any' as parameters -- **Line 13**: `inner(): any` - Returns 'any' - -### packages/builder/src/built-in.ts -- **Line 24**: `error: any` - Property typed as 'any' -- **Line 44**: `error: any` - Property typed as 'any' -- **Line 55**: `error: any` - Property typed as 'any' -- **Line 57**: `ifElse` - Generic parameters defaulting to 'any' -- **Line 69**: `let ifElseFactory: NodeFactory<[any, any, any], any> | undefined` - Multiple 'any' in generic -- **Line 74**: `(cell: OpaqueRef) => OpaqueRef` - 'any' in generic -- **Line 82**: `...values: any[]` - Array of 'any' -- **Line 89**: `values: any[]` - Array of 'any' - -### packages/memory/traverse.ts -- **Line 167-169**: `[k, this.traverseDAG(doc, docRoot, v, tracker)]),` - Using 'any' in type assertion -- **Line 355**: `isPointer(value: any): boolean` - 'any' parameter -- **Line 365**: `isJSONCellLink(value: any): value is JSONCellLink` - 'any' parameter -- **Line 371-379**: Multiple functions accepting 'any' parameters - -### packages/html/src/render.ts -- **Line 60-70**: Comments indicate potential issues with type handling but implementation uses proper types - -## 2. Type Assertions Using 'as any' - -### packages/runner/src/runner.ts -- **Line ~224**: Type assertion `as any` in runtime context - -### packages/runner/src/cell.ts -- **Line ~118**: `(self.key as any)(key).set(value)` - Type assertion to bypass type checking - -### packages/runner/src/query-result-proxy.ts -- **Line ~42**: `const target = valueCell.getAtPath(valuePath) as any` - Type assertion -- **Line ~120**: `undefined as unknown as any[]` - Multiple type assertions - -### packages/builder/src/spell.ts -- **Line ~25**: `if (typeof prop === "symbol") return (target as any)[prop]` - Type assertion -- **Line ~48**: `[key, (self as any)[key]]` - Type assertion in mapping - -### packages/builder/src/utils.ts -- **Line ~117**: `for (const key in value as any)` - Type assertion in loop - -### packages/builder/src/recipe.ts -- **Lines ~182-186**: Multiple `as any` assertions when deleting properties - -### packages/memory/traverse.ts -- **Line ~169**: Type assertion in mapping function - -## 3. Overly Broad Union Types - -While I didn't find the specific pattern `string | number | object | null`, there are several places with overly permissive types: - -### packages/builder/src/types.ts -- **Line ~244**: `cell?: any` in Alias type - Should be more specific -- **Line ~263**: `implementation?: ((...args: any[]) => any) | Recipe | string` - Function signature with 'any' - -## 4. Functions Accepting Overly Broad Types Without Validation - -### packages/runner/src/utils.ts -- `extractDefaultValues`, `mergeObjects`, `sendValueToBinding`, `setNestedValue` - All accept 'any' without proper type guards or validation -- These functions perform operations on the values but don't validate the types first - -### packages/memory/traverse.ts -- Multiple traversal functions accept 'any' or 'unknown' without proper validation - -## 5. Unknown Types That Should Be More Specific - -### packages/js-runtime/interface.ts -- **Lines 70-79**: `isJsModule` function uses 'unknown' but could use a more specific base type -- **Lines 97-104**: `isSourceMap` function uses 'unknown' but could use a more specific base type - -### packages/memory/traverse.ts -- **Lines 371-389**: Multiple functions use 'unknown' parameters that could be more specific - -## Recommendations - -1. **Replace 'any' with specific types or generics** where possible -2. **Add proper type guards** for functions that need to accept broad types -3. **Use discriminated unions** instead of 'any' for values that can be multiple types -4. **Avoid type assertions** (`as any`) - instead, use type guards or proper typing -5. **Document why 'any' is necessary** in the few cases where it truly is needed -6. **Consider using 'unknown'** instead of 'any' and add type guards for safety - -## Priority Files to Fix - -1. **packages/runner/src/utils.ts** - Has the most 'any' usage -2. **packages/builder/src/built-in.ts** - Core functionality with multiple 'any' -3. **packages/memory/traverse.ts** - Complex traversal logic with 'any' -4. **packages/builder/src/types.ts** - Type definitions themselves use 'any' -5. **packages/js-runtime/interface.ts** - Interface definitions with 'any' \ No newline at end of file diff --git a/docs/code-quality-tasks/inappropriate-error-handling-report.md b/docs/code-quality-tasks/inappropriate-error-handling-report.md deleted file mode 100644 index a5e3b570e..000000000 --- a/docs/code-quality-tasks/inappropriate-error-handling-report.md +++ /dev/null @@ -1,282 +0,0 @@ -# Inappropriate Error Handling Report - -Based on the analysis of the codebase against AGENTS.md guidelines, the -following inappropriate error handling patterns were found: - -## 1. Try/catch Blocks That Silently Swallow Errors - -### `packages/jumble/src/components/NetworkInspector.tsx` - -**Lines**: 236-238 - -```typescript -} catch (e) { - return null; -} -``` - -**Issue**: Silently catches errors and returns null without any logging or -re-throwing. This makes debugging impossible and hides potential issues. - -### `packages/toolshed/lib/otel.ts` - -**Lines**: 60-62 - -```typescript -} catch (_) { - // ignored – not running on Deno with telemetry support -} -``` - -**Issue**: Empty catch block that completely swallows errors. Even if the error -is expected, it should at least be logged for debugging purposes. - -## 2. Different Error Handling Approaches in the Same Code Path - -### `packages/runner/src/runner.ts` - -**Lines**: 358-363 - -```typescript -try { - cancel(); -} catch (error) { - console.warn("Error canceling operation:", error); -} -``` - -**Issue**: Uses `console.warn` for errors in a method that should likely -propagate errors up to the caller for proper handling. - -### `packages/js-runtime/typescript/compiler.ts` - -**Lines**: 111-114 - -```typescript -} catch (e) { - console.warn(`There was an error parsing "${key}" source map: ${e}`); -} -``` - -**Issue**: Uses `console.warn` instead of throwing or properly handling the -parsing error. Source map parsing errors could indicate serious issues. - -### `packages/llm/src/client.ts` - -**Lines**: 101-103, 115-117 - -```typescript -} catch (error) { - console.error("Failed to parse JSON line:", line, error); -} -``` - -**Issue**: Logs errors to console instead of propagating them, while other parts -of the same method throw errors. This creates inconsistent error handling. - -## 3. Functions Returning undefined/null on Error Instead of Throwing - -### `packages/jumble/src/components/NetworkInspector.tsx` - -**Lines**: 213-239 - -```typescript -const extractTransactionDetails = (value: any): any => { - try { - // ... processing logic ... - } catch (e) { - return null; - } -}; -``` - -**Issue**: Returns null on any error instead of throwing or properly handling -specific error cases. Callers can't distinguish between "no data" and "error -occurred". - -### `packages/ui/src/components/common-form.ts` - -**Lines**: 64-66 - -```typescript -} catch { - return "Invalid reference format"; -} -``` - -**Issue**: Returns an error string instead of throwing, inconsistent with other -validation patterns that throw errors. - -### `packages/charm/src/spellbook.ts` - -```typescript -} catch (error) { - console.error("Failed to save spell:", error); - return false; -} -``` - -**Issue**: Returns false on error instead of throwing, preventing callers from -understanding why the operation failed. - -## 4. Overly Broad try/catch Blocks - -### `packages/charm/src/manager.ts` - -**Lines**: 500-700+ - -```typescript -try { - // Hundreds of lines of code including: - // - Multiple async operations - // - Complex data transformations - // - Nested function calls -} catch (error) { - console.debug("Error in findReferencedCharms:", error); -} -``` - -**Issue**: Try block covers too much code, making it difficult to understand -which operation actually failed. Errors from very different operations are -handled identically. - -## 5. Low-level try/catch Where Errors Should Propagate - -### `packages/charm/src/workflow.ts` - -Multiple instances of: - -```typescript -try { - // Some operation -} catch (error) { - console.warn("Operation failed:", error); - // Continues execution -} -``` - -**Issue**: Error handling at the workflow processing level prevents higher-level -error handlers from taking appropriate action (retry, fallback, user -notification). - -### `packages/background-charm-service/src/worker.ts` - -**Lines**: 186-188 - -```typescript -} catch (error) { - const errorMessage = String(error); - // Loses stack trace and error type information -} -``` - -**Issue**: Converts errors to strings, losing valuable debugging information -like stack traces and error types. - -## 6. Missing Error Context or Unhelpful Error Messages - -### `packages/toolshed/routes/integrations/google-oauth/google-oauth.utils.ts` - -**Lines**: 162, 195 - -```typescript -throw new Error(`Failed to get auth cell: ${error}`); -throw new Error(`Error persisting tokens: ${error}`); -``` - -**Issue**: Simply wraps errors without adding meaningful context about what was -being attempted, what the inputs were, or suggestions for resolution. - -## 7. Console Logging in Production Code - -Multiple files use console methods for error handling instead of proper error -propagation: - -- `packages/charm/src/format.ts` - Uses `console.error` -- `packages/charm/src/search.ts` - Uses `console.warn` -- `packages/charm/src/workflow.ts` - Mixed `console.warn` and `console.error` -- `packages/charm/src/iframe/recipe.ts` - Uses `console.log` for errors - -**Issue**: Console logging is not appropriate for production error handling. -Errors should be propagated or logged through proper logging mechanisms. - -## Recommendations Based on AGENTS.md - -### 1. Let Errors Propagate - -```typescript -// Good - let caller handle the error -async function getData(): Promise { - const res = await fetch(URL); - if (res.ok) { - return res.text(); - } - throw new Error("Unsuccessful HTTP response"); -} - -// Bad - swallowing errors -async function getData(): Promise { - try { - const res = await fetch(URL); - if (res.ok) { - return res.text(); - } - } catch (e) { - console.error(e); - } -} -``` - -### 2. Only Catch When You Can Handle - -```typescript -// Good - specific error handling with retry -async function fetchWithRetry(url: string, retries = 3): Promise { - for (let i = 0; i < retries; i++) { - try { - return await fetch(url); - } catch (error) { - if (i === retries - 1) throw error; - await delay(1000 * Math.pow(2, i)); // exponential backoff - } - } - throw new Error("Should not reach here"); -} -``` - -### 3. Add Meaningful Context - -```typescript -// Good - adds context to errors -try { - await saveUserData(userId, data); -} catch (error) { - throw new Error( - `Failed to save data for user ${userId}: ${error.message}`, - { cause: error }, - ); -} -``` - -### 4. Use Type Guards for Expected Errors - -```typescript -// Good - handle specific, expected scenarios -function isFeatureSupported(): boolean { - try { - // Try to use feature - crypto.subtle.importKey(...); - return true; - } catch { - // Expected error - feature not supported - return false; - } -} -``` - -## Priority Areas to Fix - -1. **High Priority**: Error swallowing in core libraries (`runner`, `builder`, - `charm`) -2. **Medium Priority**: Console logging instead of proper error handling -3. **Low Priority**: Missing error context in utility functions diff --git a/docs/code-quality-tasks/singleton-patterns-report.md b/docs/code-quality-tasks/singleton-patterns-report.md deleted file mode 100644 index cf439f0d4..000000000 --- a/docs/code-quality-tasks/singleton-patterns-report.md +++ /dev/null @@ -1,151 +0,0 @@ -# Singleton Patterns Report - -Based on the analysis of the codebase against AGENTS.md guidelines, the -following singleton patterns were found that should be refactored: - -## 1. Module-level Maps and Caches - -### `packages/deno-vite-plugin/src/resolver.ts` - -- **Line 75**: `const resolverCache = new Map();` -- **Issue**: Global cache that prevents multiple instances and complicates - testing - -### `packages/static/index.ts` - -- **Line 5**: `const cache = new StaticCache();` -- **Issue**: Exported singleton cache instance - -## 2. Exported Singleton Instances - -### `packages/toolshed/lib/otel.ts` - -- **Line 20**: `export const otlpExporter = new OTLPTraceExporter(...)` -- **Line 26**: `export const provider = new BasicTracerProvider(...)` -- **Issue**: Global telemetry instances that can't be mocked or isolated for - testing - -### `packages/toolshed/routes/storage/blobby/utils.ts` - -- **Line 7**: `export const storage = new DiskStorage(DATA_DIR);` -- **Line 10**: `let redisClient: RedisClientType | null = null;` -- **Issue**: Global storage instance and module-level Redis client state - -### `packages/memory/clock.ts` - -- **Line 20**: `export default new Clock();` -- **Issue**: Default export of singleton instance prevents multiple clock - instances - -## 3. Module-level TextEncoder/TextDecoder Instances - -### `packages/identity/src/identity.ts` - -- **Line 11**: `const textEncoder = new TextEncoder();` - -### `packages/utils/src/encoding.ts` - -- **Line 1**: `const encoder = new TextEncoder();` -- **Line 2**: `const decoder = new TextDecoder();` -- **Issue**: While less problematic, these could be passed as dependencies for - better testability - -## 4. Module-level Configuration Objects - -### `packages/background-charm-service/src/env.ts` - -- **Line 42**: `export const env = loadEnv();` -- **Issue**: Global environment configuration that can't be easily mocked - -### `packages/toolshed/lib/constants.ts` - -- **Lines 4-8**: `export const ZOD_ERROR_MESSAGES = { ... }` -- **Lines 10-12**: `export const ZOD_ERROR_CODES = { ... }` -- **Lines 14-16**: - `export const notFoundSchema = createMessageObjectSchema(...)` -- **Issue**: Global constants that might need different values in different - contexts - -### `packages/runner/src/query-result-proxy.ts` - -- **Lines 14-46**: - `const arrayMethods: { [key: string]: ArrayMethodType } = { ... }` -- **Issue**: Module-level method mapping - -## 5. Module-level State and Initialization - -### `packages/memory/deno.ts` - -- **Lines 8-18**: `const serviceDid: DID = (() => { ... })();` -- **Line 20**: `const storePath = ...` -- **Line 21**: `const STORE = new URL(...)` -- **Lines 22-29**: Module-level provider initialization -- **Issue**: Complex module-level initialization that runs on import - -### `packages/toolshed/lib/otel.ts` - -- **Line 17**: `let _providerRegistered = false;` -- **Line 18**: `let _provider: BasicTracerProvider | undefined;` -- **Issue**: Module-level state tracking for provider registration - -## Refactoring Recommendations - -According to AGENTS.md, these patterns should be refactored using: - -### 1. Class-based Patterns - -```typescript -// Instead of: const cache = new Map(); -export class Cache { - private map: Map = new Map(); - get(key: string): string | undefined { - return this.map.get(key); - } - set(key: string, value: string) { - this.map.set(key, value); - } -} -``` - -### 2. Functional Patterns - -```typescript -// Instead of module-level state -export type Cache = Map; -export const get = (cache: Cache, key: string): string | undefined => - cache.get(key); -export const set = (cache: Cache, key: string, value: string) => - cache.set(key, value); -``` - -### 3. Dependency Injection - -```typescript -// Instead of: export const storage = new DiskStorage(DATA_DIR); -export interface StorageProvider { - storage: DiskStorage; -} - -export function createStorageProvider(dataDir: string): StorageProvider { - return { - storage: new DiskStorage(dataDir), - }; -} -``` - -## Impact - -These singleton patterns: - -- Make unit testing difficult or impossible -- Prevent running multiple instances of the application -- Create tight coupling between modules -- Make it hard to mock dependencies -- Can cause state leakage between tests - -## Priority - -1. **High Priority**: Fix singletons in core modules like `memory`, `toolshed`, - and `runner` -2. **Medium Priority**: Fix utility singletons like encoders and caches -3. **Low Priority**: Fix configuration objects that rarely change diff --git a/docs/code-quality-tasks/invalid-state-representations-report.md b/docs/future-tasks/code-quality-tasks/invalid-state-representations-report.md similarity index 67% rename from docs/code-quality-tasks/invalid-state-representations-report.md rename to docs/future-tasks/code-quality-tasks/invalid-state-representations-report.md index 2a8c16885..ae0ffd75a 100644 --- a/docs/code-quality-tasks/invalid-state-representations-report.md +++ b/docs/future-tasks/code-quality-tasks/invalid-state-representations-report.md @@ -26,63 +26,16 @@ export interface LLMRequest { throughout the codebase defaults it to `true` if undefined, but this implicit behavior isn't clear from the interface. -**Recommendation**: Make `cache` required with explicit default: +**Recommendation**: Rename `cache` to `noCache` with `false` as default: ```typescript export interface LLMRequest { - cache: boolean; // Always explicit, no ambiguity + noCache?: boolean; // ... other properties } ``` -## 2. WorkflowForm Interface - -**File**: `packages/charm/src/workflow.ts` **Lines**: 299-348 - -```typescript -export interface WorkflowForm { - classification: { - workflowType: WorkflowType; - confidence: number; - reasoning: string; - } | null; - - plan: { - features?: string[]; - description?: string; - charms?: CharmSearchResult[]; - } | null; - - generation: { - charm: Cell; - } | null; - - searchResults: { - castable: Record<...> - } | null; - - spellToCast: { - charmId: string; - spellId: string; - } | null; -} -``` - -**Issue**: Using `null` to represent "not yet processed" states requires -constant null checking. This doesn't make invalid states unrepresentable. - -**Recommendation**: Use discriminated unions: - -```typescript -type WorkflowForm = - | { state: 'unclassified' } - | { state: 'classified'; classification: {...}; plan: null } - | { state: 'planned'; classification: {...}; plan: {...}; generation: null } - | { state: 'generated'; classification: {...}; plan: {...}; generation: {...} } - // etc. -``` - -## 3. OAuth2Tokens Interface +## 2. OAuth2Tokens Interface **File**: `packages/toolshed/routes/integrations/google-oauth/google-oauth.utils.ts` @@ -119,7 +72,7 @@ export interface RenewableOAuth2Tokens extends InitialOAuth2Tokens { } ``` -## 4. UserInfo Interface +## 3. UserInfo Interface **File**: `packages/toolshed/routes/integrations/google-oauth/google-oauth.utils.ts` @@ -159,7 +112,7 @@ type UserInfoResult = | { success: false; error: string }; ``` -## 5. CallbackResult Interface +## 4. CallbackResult Interface **File**: `packages/toolshed/routes/integrations/google-oauth/google-oauth.utils.ts` @@ -185,55 +138,7 @@ type CallbackResult = | { success: false; error: string; details?: Record }; ``` -## 6. RuntimeOptions Interface - -**File**: `packages/runner/src/runtime.ts` **Lines**: 43-51 - -```typescript -export interface RuntimeOptions { - storageUrl: string; - signer?: Signer; - consoleHandler?: ConsoleHandler; - errorHandlers?: ErrorHandler[]; - blobbyServerUrl?: string; - recipeEnvironment?: RecipeEnvironment; - debug?: boolean; -} -``` - -**Issue**: Optional properties have implicit defaults that aren't clear from the -interface. Code later throws if `storageUrl` doesn't exist (line 275). - -**Recommendation**: Make defaults explicit: - -```typescript -export interface RuntimeOptions { - storageUrl: string; - signer: Signer | null; - consoleHandler: ConsoleHandler | null; - errorHandlers: ErrorHandler[]; - blobbyServerUrl: string | null; - recipeEnvironment: RecipeEnvironment | null; - debug: boolean; -} - -// With a factory function for defaults: -export function createRuntimeOptions( - partial: PartialRuntimeOptions, -): RuntimeOptions { - return { - storageUrl: partial.storageUrl, // required - signer: partial.signer ?? null, - consoleHandler: partial.consoleHandler ?? defaultConsoleHandler, - errorHandlers: partial.errorHandlers ?? [], - blobbyServerUrl: partial.blobbyServerUrl ?? null, - recipeEnvironment: partial.recipeEnvironment ?? null, - debug: partial.debug ?? false, - }; -} -``` - -## 7. BackgroundCharmServiceOptions Interface +## 5. BackgroundCharmServiceOptions Interface **File**: `packages/background-charm-service/src/service.ts` **Lines**: 12-19 @@ -271,7 +176,7 @@ export const DEFAULT_BG_OPTIONS = { } as const; ``` -## 8. Module Interface +## 6. Module Interface **File**: `packages/builder/src/types.ts` **Lines**: 182-188 diff --git a/docs/code-quality-tasks/module-graph-import-issues-report.md b/docs/future-tasks/code-quality-tasks/module-graph-import-issues-report.md similarity index 50% rename from docs/code-quality-tasks/module-graph-import-issues-report.md rename to docs/future-tasks/code-quality-tasks/module-graph-import-issues-report.md index c8a8d20cb..400876ec3 100644 --- a/docs/code-quality-tasks/module-graph-import-issues-report.md +++ b/docs/future-tasks/code-quality-tasks/module-graph-import-issues-report.md @@ -1,37 +1,15 @@ # 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: +Based on the analysis of the codebase against AGENTS.md guidelines, the +following module graph and import issues were found: -## 1. Circular Dependencies Between Modules +## 1. Default Exports Instead of Named Exports -### Critical Circular Dependency: `@commontools/builder` ↔ `@commontools/runner` - -**Builder → Runner imports:** -- `packages/builder/src/built-in.ts` imports from `@commontools/runner` -- `packages/builder/src/schema-to-ts.ts` imports from `@commontools/runner` -- `packages/builder/src/utils.ts` imports from `@commontools/runner` - -**Runner → Builder imports:** -- `packages/runner/src/cfc.ts` imports from `@commontools/builder` -- `packages/runner/src/recipe-manager.ts` imports from `@commontools/builder` -- `packages/runner/src/runner.ts` imports from `@commontools/builder` -- `packages/runner/src/env.ts` imports from `@commontools/builder` -- `packages/runner/src/doc-map.ts` imports from `@commontools/builder` -- `packages/runner/src/cell.ts` imports from `@commontools/builder` - -**Impact**: This circular dependency makes it impossible to: -- Test these modules in isolation -- Understand the module hierarchy -- Reuse either module independently -- Avoid loading unnecessary code - -**Recommendation**: Extract shared types and interfaces to a separate package like `@commontools/core-types` or `@commontools/shared`. - -## 2. Default Exports Instead of Named Exports - -The following files violate the "prefer named exports over default exports" guideline: +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` @@ -39,11 +17,13 @@ The following files violate the "prefer named exports over default exports" guid - `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` @@ -51,6 +31,7 @@ The following files violate the "prefer named exports over default exports" guid - Multiple other view files in `packages/jumble/src/views/` **Example Fix**: + ```typescript // Bad export default class Clock { ... } @@ -67,11 +48,13 @@ export const clock = new Clock(); export function createClock(): Clock { ... } ``` -## 3. Missing or Incorrect Import Grouping +## 2. Missing or Incorrect Import Grouping -The following files don't follow the import grouping convention (standard library → external → internal): +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"; @@ -91,6 +74,7 @@ import { SpellRunner } from "./SpellRunner.js"; ``` ### `packages/builder/src/utils.ts` + ```typescript // Current (internal packages mixed without grouping) import { JSONSchema7 } from "json-schema"; @@ -110,51 +94,28 @@ import { isOpaqueRef, OpaqueRef } from "./spell.js"; ``` ### `packages/identity/src/ed25519/index.ts` -- External imports (`@scure/bip39`) mixed with internal imports without proper grouping -## 4. Heavy Dependencies in Utility Modules +- External imports (`@scure/bip39`) mixed with internal imports without proper + grouping -### `packages/builder/src/utils.ts` -**Issue**: This utility module imports from `@commontools/runner`, making it a heavyweight utility rather than a lightweight one. - -**Impact**: -- Any code that needs simple utilities from `builder` must also load the entire `runner` package -- Creates the circular dependency issue mentioned above -- Violates the principle that utility modules should have minimal dependencies - -**Recommendation**: -- Move utilities that depend on `runner` to a separate module like `builder-runner-utils.ts` -- Keep pure utilities without external dependencies in `utils.ts` - -## 5. Module-Specific Dependencies (Good Practices Found) +## 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 +- `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`. - -## 6. Non-standard JS Usage - -**Positive Finding**: The codebase is clean of non-portable JavaScript patterns: -- No direct use of environment variables in module scope -- No Vite-specific APIs (`import.meta.hot`, `import.meta.glob`) outside of Vite plugin code -- Clean separation between build-time and runtime code +**This is a positive finding** - the codebase correctly avoids adding +package-specific dependencies to the workspace `deno.json`. ## Recommendations -### 1. Break Circular Dependencies -Create a new package structure: -``` -@commontools/core-types // Shared interfaces and types -@commontools/builder // Depends on core-types -@commontools/runner // Depends on core-types and builder -``` +### 1. Convert Default to Named Exports -### 2. Convert Default to Named Exports ```typescript // For all identified files: // Change: export default Clock @@ -165,27 +126,19 @@ Create a new package structure: // or export function createClock(): Clock ``` -### 3. Enforce Import Grouping +### 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 -### 4. Refactor Heavy Utilities -Split `packages/builder/src/utils.ts`: -- `utils.ts` - Pure utilities with no external dependencies -- `runner-utils.ts` - Utilities that need runner functionality - -### 5. Document Module Dependencies -Create a module dependency graph documentation to help developers understand: -- Which packages can be used independently -- Which packages have peer dependencies -- The intended hierarchy of packages - ## Impact Summary -- **Circular dependencies**: Critical issue affecting maintainability and testability +- **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 \ No newline at end of file +- **Heavy utilities**: Medium impact - affects bundle size and modularity