diff --git a/docs/proposals/C6.2-error-taxonomy-phase2.md b/docs/proposals/C6.2-error-taxonomy-phase2.md new file mode 100644 index 00000000..19d25003 --- /dev/null +++ b/docs/proposals/C6.2-error-taxonomy-phase2.md @@ -0,0 +1,176 @@ +# Proposal C6.2: Error taxonomy phase 2 — migrate remaining 228 catch sites + +> Source: Roundtable 2026-04-20 (Linus + Karpathy + Steve Jobs); C6.1 executed tonight +> Status: proposed +> Prerequisite: C6.1 landed (`src/main/errors/` module + 20 pilot migrations) +> Owner: TBD +> Estimated effort: 4-6 working days, split across three waves +> Blocking dependencies: none — but coordinate with any in-flight module refactors to avoid diff storms + +## Problem statement + +C6.1 established the `AppError` taxonomy and migrated the 20 highest-signal +catch sites. At the time of this proposal src/main/ still contains **228 +catch blocks** with zero classification. The breakdown by category: + +| Category | Count | Typical shape | Risk if left unmigrated | +|---|---|---|---| +| `} catch {}` empty | ~35 | Silent fallthrough | Fatal conditions (DB corruption, OOM) hidden indistinguishable from expected misses | +| `} catch (e) { log.warn; return default }` fail-open | ~90 | Already logs, but no `code`/`kind` | Telemetry cannot bucket; ops cannot answer "how many upstream 5xx in the last hour" | +| `} catch (e) { log.error; throw e }` pass-through | ~40 | Correct but loses classification on re-throw | Upstream handler re-swallows without context | +| `} catch (e) { return null / false }` silent fail-open | ~30 | No log at all | Behaviour correct-ish, observability non-existent | +| Narrow `instanceof` guarded catches | ~20 | e.g. `if (err.message.startsWith("X")) return null; else throw` | These already encode intent — migrate carefully to `UserInputError` subclass vs re-throw | +| Mixed / platform-specific | ~13 | Electron render-disposed, ENOENT-on-read, EPIPE | Each needs a bespoke classifier | + +Total: **228 non-trivial catches** (count reconfirmed via +`grep -rn "} catch " src/main/ | wc -l` = 248, minus 20 already migrated in C6.1). + +## Why this matters (Linus) + +The backup / migration work already landing in B2 wants to know whether a +DB open failure is "file corrupt" (fatal) or "file missing, first boot" +(recoverable). Today the catch that opens the DB returns the same +log-and-rethrow shape for both. Until every DB-path catch carries a +`code`, the migration precondition checker has to re-classify by string +matching — exactly the kind of fragile guesswork we wanted to avoid. + +## Why this matters (Karpathy) + +Tonight's harness (B5) is structural. Next-tier harnesses (B5.1 judge +tier, future chaos-injection tier) will want to assert things like "when +the Anthropic provider returns 529, the agent must log an +`upstream.anthropic.overloaded` code and retry; a +`fatal.db.corrupt` must abort the run". That kind of assertion is +impossible until every catch site publishes a stable code. + +## Scope + +Migrate all 228 remaining catch blocks in `src/main/**` (excluding tests, +scripts, and the already-migrated 20). Pure-library files with no +accessible logger (e.g. `skill-keys.ts`, `skill-format.ts`) are kept out +of scope unless a logger is plumbed in first — a separate concern. + +## Non-goals + +- Renderer-side catches (`src/renderer/`) — renderer errors surface as + red-screens today; a separate proposal should cover them. +- Changing observable behaviour of any catch. This migration is + **pure classification**: same return values, same throws, same logs + (only the log prefix gains a `[code]`). +- Refactoring the catches themselves (merging, splitting, removing). + +## Migration waves + +Partition the 228 sites into three waves so each wave is one coherent +module group and one reviewable PR. + +### Wave 1 — worker + orchestration (≈80 sites) + +Buckets: + +| Module | Approx sites | Dominant kind | Why first | +|---|---|---|---| +| `src/main/worker/**` | ~35 | `upstream` (pi-ai), `recoverable` (lease/db) | Highest churn in production; biggest payoff for observability | +| `src/main/orchestration/**` | ~20 | `recoverable` (lookups), `user_input` (payload parse) | Feeds into every run; code-path coverage bump is large | +| `src/main/agent-runtime/**` | ~25 | Mixed | artifact-projector's fs-walk catches are intentional silent; document in-line rather than classify | + +**Estimated effort**: 2 days, one PR. Run `pnpm test:eval` and +`pnpm test:unit` after each file. Commit per module. + +### Wave 2 — IPC surfaces + service layer (≈90 sites) + +Buckets: + +| Module | Approx sites | Dominant kind | Notes | +|---|---|---|---| +| `src/main/ipc-*.ts` (excl. already-done credentials + support) | ~50 | `user_input` (zod fails), `upstream` (downstream handler fails) | Uniform shape — candidate for a `registerHandle`-level classifier wrapper | +| `src/main/*-service.ts` | ~25 | Mixed | Each service owns one subsystem; classify per service | +| `src/main/automation/**` | ~15 | `recoverable` (rule eval), `user_input` (bad config) | Small; can ride wave 2 PR as final commit | + +**Estimated effort**: 2-3 days, one PR with one commit per file group. +Depends on Wave 1 only insofar as `registerHandle` wrapper lives in +`ipc-support.ts` and was partially migrated in C6.1. + +### Wave 3 — database + credentials + browser + leftovers (≈58 sites) + +Buckets: + +| Module | Approx sites | Dominant kind | Notes | +|---|---|---|---| +| `src/main/database/**` | ~20 | `fatal` (corruption), `recoverable` (busy/locked) | **Coordinate with M1 / B2** — these catches are the raw material for the backup/migration preconditions | +| `src/main/browser/**` + `browser-manager.ts` | ~12 | `upstream` (Electron session), `user_input` (blocked URL) | Small surface | +| `src/main/credentials/**` | ~8 | `upstream` (vault decrypt), `user_input` (schema) | Should flow through `UpstreamError` / `UserInputError` subclasses | +| `src/main/skills/**` | ~8 | `recoverable` (fs), `upstream` (GitHub) | | +| Residue (top-level files not already done) | ~10 | Mixed | Clean-up commit | + +**Estimated effort**: 2 days, one PR. + +## Decisions to settle before execution + +| Question | Default | Notes | +|---|---|---| +| Should empty `} catch {}` get a minimum `classifyUnknownError` call? | Yes, when a logger is in scope — even a silent fallback becomes grep-able by `code` | Skip only in pure-library files with no logger | +| Should we introduce a `Result` envelope for IPC? | No — out of scope for C6.2 | Separate proposal if desired | +| Should `AppError.toJSON()` be wired into existing log transports? | Yes, but in a follow-up — taxonomy migration first, transport swap second | | +| Code naming convention | `module.subdomain.condition` (lowercase, dots) | Examples: `db.lock.busy`, `worker.run_log.lease_renewal_lookup_failed`, `ipc.payload.invalid` | + +## Verification checklist per wave + +- `pnpm typecheck` clean +- `pnpm test:unit` green +- `pnpm test:eval` green (no fixture drift) +- `grep -rn "classifyUnknownError" src/main/ | wc -l` monotonically increases +- `grep -rnE "^\s*\} catch \(?\w*\)? \{\s*$" src/main/ | wc -l` monotonically decreases (the empty-catch count) + +## Rollback plan + +Each wave is one PR with per-file commits. Revert at file granularity if +a regression surfaces in the eval harness — no cross-file entanglement +by construction (`classifyUnknownError` is a pure call with no shared +state). + +## Out-of-band follow-ups (not blocking this proposal) + +1. Structured log transport: swap `log.error(\`[${code}] …\`)` for a + JSON shipper that keys on `err.code` directly. Needed for real + telemetry buckets. +2. Renderer-side taxonomy: duplicate the module into `src/renderer/` + (or hoist to `src/shared/`) so IPC error payloads arrive as typed + AppError envelopes. +3. Lint rule: "no bare `} catch (e) {}` in src/main/". Enforceable via + an eslint custom rule once coverage is > 95%. + +## Out of scope explicitly + +- Replacing `throw new Error(...)` call sites with `throw new FatalError(...)` + — that is a separate, heavier surgical pass that would change type + signatures of many functions. File a follow-up if wanted. +- Removing the `console.error` / `electron-log` split — orthogonal. + +## Appendix A — file-level checklist (seed) + +The following is a starter list of the 228 sites, extracted from the +current grep. A reviewer should refresh this list before starting each +wave since the codebase is actively evolving. + +``` +src/main/worker/entry/*.ts (re-grep) +src/main/worker/runtime/*.ts (re-grep) +src/main/worker/handlers/*.ts (14 remain after C6.1) +src/main/worker/observability/*.ts (re-grep) +src/main/orchestration/*.ts (18 remain after C6.1) +src/main/agent-runtime/*.ts (~25) +src/main/api-server/*.ts (~15) +src/main/ipc-*.ts (non-credentials) (~50) +src/main/automation/*.ts (~15) +src/main/database/*.ts (~20) +src/main/browser/*.ts (~12) +src/main/credentials/*.ts (~8) +src/main/skills/*.ts (~8) +src/main/connector-*.ts (~10) +src/main/message-*.ts (re-grep) +leftover top-level files (~10) +``` + +Refresh with `grep -rn "} catch " src/main/ | grep -v "errors/"` before +starting each wave. diff --git a/src/main/connector-definitions-data-apikey.ts b/src/main/connector-definitions/apikey.ts similarity index 100% rename from src/main/connector-definitions-data-apikey.ts rename to src/main/connector-definitions/apikey.ts diff --git a/src/main/connector-definitions-data-cloud.ts b/src/main/connector-definitions/cloud.ts similarity index 100% rename from src/main/connector-definitions-data-cloud.ts rename to src/main/connector-definitions/cloud.ts diff --git a/src/main/connector-definitions-data-oauth.ts b/src/main/connector-definitions/oauth.ts similarity index 100% rename from src/main/connector-definitions-data-oauth.ts rename to src/main/connector-definitions/oauth.ts