Address T2 review asks from Karpathy (request_changes) + Jobs (approve with minor asks) without touching published commits: - scripts/codemod/README.md (new): records remove-appdatabase-passthrough.mjs usage, the completeness proof for METHOD_TO_MODULE (typecheck-clean + 524 tests green at118e3d8= no orphaned call sites), and the sequential revert recipe for C1 (3 -> 2 -> 1, not independent because118e3d8bundled vault simplification). - CONTRIBUTING.md (new): pass-through ban + codemod usage + sequential revert recipe + file-size ceiling + C6 wave cap. Whitelisted in .gitignore alongside the existing doc exceptions. - docs/proposals/C6.2-error-taxonomy-phase2.md: wave 2 scope boundary names renderer-adjacent / api-server / browser / automation / skills explicitly and excludes src/main/database/** + src/main/worker/runtime/** (B2 / M10 high-churn zones). Wave 2 and wave 3 each capped at 30 catch sites so C6 cannot expand without bound. - src/main/agent-runtime/artifact-projector.ts, runtime-bridge.ts: N-scan TODOs now carry a death date ("TODO(M-followup by 2026-05-20)"). If the date passes without decomposition, a follow-up session MUST revisit or justify the exception. Karpathy T2 asks #1 and #2 (split part 3 / "independent revert" language) are addressed in CONTRIBUTING + codemod README as documentation, not history rewrites, because118e3d8is already published. Typecheck clean. No structural code changes; pure docs + comment edits.
11 KiB
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 — renderer-adjacent + api-server + browser + automation + skills (≤30 catches)
Scope boundary (hard): Wave 2 covers src/main/{renderer-adjacent,api-server,browser,automation,skills}/*
plus top-level ipc-*.ts and *-service.ts files. Wave 2 explicitly
avoids src/main/database/** and src/main/worker/runtime/** —
those are the high-churn B2 (tasks state-machine) and M10 (agent-runtime
hooks) zones, and overlapping edits produce diff storms that lose both
sides' intent. Database and worker/runtime catches are deferred to
Wave 3 under the coordination gate below.
Scope cap (hard): Wave 2 migrates at most 30 catch sites. If the live count in the listed modules exceeds 30, stop at 30 and move the remainder to Wave 3. This prevents C6 from expanding into an unbounded sweep.
Buckets:
| Module | Approx sites (capped) | Dominant kind | Notes |
|---|---|---|---|
src/main/ipc-*.ts (excl. already-done credentials + support) |
~15 | user_input (zod fails), upstream (downstream handler fails) |
Uniform shape — candidate for a registerHandle-level classifier wrapper |
src/main/*-service.ts + src/main/api-server/** |
~8 | Mixed | Each service owns one subsystem; classify per service |
src/main/automation/** + src/main/skills/** + src/main/browser/** |
~7 | recoverable (rule eval), user_input (bad config), upstream (Electron session) |
Small; ride wave 2 PR as final commit |
Estimated effort: 2 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 + worker/runtime + credentials + leftovers (≤30 catches)
Scope cap (hard): Wave 3 migrates at most 30 catch sites. Any residue beyond 30 gets punted to a follow-up proposal (C6.3) rather than expanding Wave 3. C6 does not get to grow without bound.
⚠️ Coordination gate (hard): Wave 3 touches src/main/database/**
(Linus's domain — backup hook, migrations, state-machine work) and
src/main/connector-definitions/** (which was rehomed into a subdirectory
during C6.1). Before starting Wave 3 the executor MUST:
- Check
git log --oneline -20for any pending database / backup / migration commits from other streams. - Confirm B2 (tasks state-machine refactor) is either landed or pre-gated — migrating database catches while B2 is mid-flight produces diff storms that lose both sides' intent.
- Open a short coordination thread (issue or chat) with the database owner before the first file edit — this wave is the highest-risk of the three for scope conflicts.
Skipping this gate previously caused commit 093718d in the C6.1 wave to
accidentally carry an M4 scripts/ reorg because two agents touched
scripts/ in the same window. Do not repeat.
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<T, AppError> 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 typecheckcleanpnpm test:unitgreenpnpm test:evalgreen (no fixture drift)grep -rn "classifyUnknownError" src/main/ | wc -lmonotonically increasesgrep -rnE "^\s*\} catch \(?\w*\)? \{\s*$" src/main/ | wc -lmonotonically 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)
- Structured log transport: swap
log.error(\[${code}] …`)for a JSON shipper that keys onerr.code` directly. Needed for real telemetry buckets. - Renderer-side taxonomy: duplicate the module into
src/renderer/(or hoist tosrc/shared/) so IPC error payloads arrive as typed AppError envelopes. - 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 withthrow 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-logsplit — 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.