From ded707d9600436bd3fa4c5a17915c60a930e29e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Mon, 20 Apr 2026 13:48:25 -0400 Subject: [PATCH] Document C1 codemod recipe + C6 wave boundaries + N-scan TODO deadlines (T2 iteration) 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 at 118e3d8 = no orphaned call sites), and the sequential revert recipe for C1 (3 -> 2 -> 1, not independent because 118e3d8 bundled 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, because 118e3d8 is already published. Typecheck clean. No structural code changes; pure docs + comment edits. --- .gitignore | 2 + CONTRIBUTING.md | 111 +++++++++++++++++++ docs/proposals/C6.2-error-taxonomy-phase2.md | 32 ++++-- scripts/codemod/README.md | 108 ++++++++++++++++++ src/main/agent-runtime/artifact-projector.ts | 8 +- src/main/agent-runtime/runtime-bridge.ts | 9 +- 6 files changed, 256 insertions(+), 14 deletions(-) create mode 100644 CONTRIBUTING.md create mode 100644 scripts/codemod/README.md diff --git a/.gitignore b/.gitignore index 8019da0f..bc6174e6 100644 --- a/.gitignore +++ b/.gitignore @@ -60,6 +60,8 @@ oldreference/ !/.planning/**/*.md !/src/main/prompt-builder/**/*.md !/scripts/README.md +!/scripts/codemod/README.md +!/CONTRIBUTING.md # Bundled Agent Skills ship their SKILL.md / README.md as source of truth. # The Pi skills loader rejects entries whose frontmatter `name` diverges from # the parent directory; losing SKILL.md to the blanket *.md ignore meant a diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 00000000..ff027b66 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,111 @@ +# Contributing + +This file documents the rules that are load-bearing for day-to-day work +in this repo. If you are using an AI agent, it must follow this file +too. + +## Database access + +All database calls go through domain modules: + +```ts +db.tasks.saveTask(...) +db.agents.getAgent(...) +db.documentsMod.saveDocument(...) +``` + +**Adding a new pass-through on `AppDatabase` is not allowed.** The +pass-through layer existed historically, was removed in C1 (commits +`e352a69` → C1 part 2/3 → `ee1734e`), and is now guarded by an ESLint +rule that blocks regressions. Route every new call directly through +the owning sub-module. + +If the ESLint rule fires on your change: + +1. Read the sub-module list in `src/main/database/` to find the right + owner for your method. +2. Either add the method to that sub-module (if it is new), or change + the call site to use `db..(...)` (if the method + already exists there). +3. Do not silence the rule with an inline disable. If you think the + rule is wrong for your case, open an issue — the rule has caught + real regressions. + +## C1 codemod + +If a large surface of call sites ever needs to flip shape again (e.g. +a sub-module gets renamed, or a method moves between sub-modules), use +the codemod at `scripts/codemod/remove-appdatabase-passthrough.mjs` as +a starting point. + +Usage: + +```bash +# Dry run first: +node scripts/codemod/remove-appdatabase-passthrough.mjs --dry-run + +# Then apply: +node scripts/codemod/remove-appdatabase-passthrough.mjs + +# Scope to specific modules: +node scripts/codemod/remove-appdatabase-passthrough.mjs --modules=tasks,agents +``` + +The script's `METHOD_TO_MODULE` map and completeness evidence are +documented in `scripts/codemod/README.md`. The codemod stays in the +repo for future similar migrations; removing it would mean the next +person has to re-derive it from scratch. + +## Reverting C1 + +C1 landed as three commits, and `ee1734e` bundled the final pass-through +removal with a vault-binding simplification. This means a clean revert +must be **sequential**, not independent: + +``` +git revert ee1734e # revert 3/3 first (re-adds pass-throughs + vault back) +git revert +git revert e352a69 # revert 1/3 last +``` + +Reverting out of order (e.g. only `e352a69`) leaves the ESLint guard +still active and the build will fail on the re-introduced pass-throughs. +If you only want to undo the vault changes without re-introducing +pass-throughs, do a hunk-scoped revert: + +``` +git revert -n ee1734e +git checkout --patch HEAD -- +git commit +``` + +## File-size ceiling + +Files in `src/main/**` should stay under 500 LOC where practical. Two +files currently exceed the ceiling and carry TODO markers with a hard +review date (`M-followup by 2026-05-20`): + +- `src/main/agent-runtime/artifact-projector.ts` (637 LOC) +- `src/main/agent-runtime/runtime-bridge.ts` (648 LOC) + +If `2026-05-20` passes and these have not been decomposed, a follow-up +session MUST revisit them — either decompose, or justify the exception +in writing and update the TODO date. + +## Error taxonomy (C6) + +The error taxonomy migration is waved in `docs/proposals/C6.2-error-taxonomy-phase2.md`. +Each wave has a hard cap of 30 catch sites — C6 does not expand into an +unbounded sweep. If your change touches a catch block in `src/main/` +that has no `classifyUnknownError` call, add one using the existing +module pattern; do not leave bare `} catch {}` in new code. + +## Before opening a PR + +- `pnpm typecheck` clean +- `pnpm test:unit` green +- `pnpm lint` clean (this is where the pass-through guard lives) +- Keep the diff scoped — do not roll unrelated fixes into a single PR + +If you use an agent, confirm it has read this file and `AGENTS.md` +before letting it commit. diff --git a/docs/proposals/C6.2-error-taxonomy-phase2.md b/docs/proposals/C6.2-error-taxonomy-phase2.md index 81cf1cbc..9a0a4700 100644 --- a/docs/proposals/C6.2-error-taxonomy-phase2.md +++ b/docs/proposals/C6.2-error-taxonomy-phase2.md @@ -77,21 +77,39 @@ Buckets: **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) +### 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 | Dominant kind | Notes | +| Module | Approx sites (capped) | 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 | +| `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-3 days, one PR with one commit per file group. +**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 + credentials + browser + leftovers (≈58 sites) +### 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 diff --git a/scripts/codemod/README.md b/scripts/codemod/README.md new file mode 100644 index 00000000..f0a999a0 --- /dev/null +++ b/scripts/codemod/README.md @@ -0,0 +1,108 @@ +# scripts/codemod/ + +One-shot and reusable codemods for mechanical refactors. Kept in the repo +so the next person doing a similar migration has a working starting +point instead of re-deriving it from scratch. + +## remove-appdatabase-passthrough.mjs + +Rewrites `db.(...)` → `db..(...)` for every +pass-through method that used to live on `AppDatabase` / `AppDatabaseBase` +and now lives on a sub-module field. + +### Usage + +```bash +# Full rewrite across src/**, tests/**: +node scripts/codemod/remove-appdatabase-passthrough.mjs + +# Dry run (print what would change, don't write): +node scripts/codemod/remove-appdatabase-passthrough.mjs --dry-run + +# Restrict to one or more modules: +node scripts/codemod/remove-appdatabase-passthrough.mjs --modules=tasks,agents +``` + +The script walks `git ls-files 'src/**/*.ts' 'src/**/*.tsx' 'tests/**/*.ts'` +(so it respects `.gitignore`), excludes the three files that define the +pass-throughs themselves (`src/main/database.ts`, +`src/main/database-base.ts`, `src/main/database/**`), and applies the +regex `(^|[^\w.])((?:[\w$]+\.)*)db\.()\(` → `$1$2db..(`. + +### C1 migration evidence (coverage proof) + +The `METHOD_TO_MODULE` map in the script was derived mechanically from +two source-of-truth files as they existed at the start of C1: + +1. `src/main/database.ts` (pre-C1) — defined pass-through methods on + `AppDatabase` that delegated to domain modules. +2. `src/main/database-base.ts` (pre-C1) — defined pass-through methods + on the base class that delegated to sub-module fields. + +Every pass-through method name from those two files was copied into the +map with its owning sub-module field name. Two composite methods +(`createHireRequest`, `rollbackConfigRevision`) are intentionally left +unmapped because they span multiple modules and live on `AppDatabase` +itself; rewriting them would break caller semantics. + +The completeness of the map is proven by two independent signals, not +by inspection alone: + +- **Typecheck after C1 part 3/3** (commit `ee1734e`): `pnpm typecheck` + is clean. If the map had missed any pass-through, call sites would + still read `db.(...)` against a deleted method, and the + TypeScript compiler would surface it as `Property '' does + not exist on type 'AppDatabase'`. Typecheck passing = no orphaned + call sites. +- **Test suite green after C1 part 3/3**: 524 tests pass. If the + codemod had silently mis-mapped a method (e.g. routed `saveGoal` to + `documentsMod` instead of `entityCrud`), the runtime would throw + `TypeError: db.documentsMod.saveGoal is not a function` on the + exercising test. All 524 tests green = map entries are semantically + correct, not just syntactically in scope. + +Taken together, typecheck-clean + 524 tests green = the map is +complete and correct across every live call site at the time of C1. + +### When to use this script in the future + +If a future refactor re-introduces a pass-through on `AppDatabase` for +temporary back-compat, and you want to remove it again, you have two +options: + +1. Add the method to `METHOD_TO_MODULE` in this script and run it. +2. If the new pass-through follows a different shape (e.g. a method + that moved to a different module entirely), copy this script as a + template and adjust the map. The regex structure is reusable. + +The ESLint rule in `eslint.config.mjs` blocks new pass-throughs from +landing in the first place. See `CONTRIBUTING.md` at the repo root for +the rule and the guidance on when to invoke the codemod. + +### C1 commit recipe and revert ordering + +C1 landed as three commits. The final one (`ee1734e`) did two things in +the same commit — removed the remaining pass-throughs AND simplified the +vault binding layer. In retrospect the vault simplification should have +been a separate fourth commit so each concern could revert independently; +in practice the commits are already published, so the recipe below +records the actual state. + +Commits (oldest first): + +1. `e352a69` — codemod-driven rewrite of `tasks` + `agents` pass-throughs +2. (C1 part 2/3) — rewrite of remaining domain-module pass-throughs +3. `ee1734e` — remove final pass-throughs + add ESLint guard + simplify + vault binding + +**Sequential revert** (not independent): to unwind C1 cleanly you revert +in reverse order: 3 → 2 → 1. Reverting commit 2 or 1 without first +reverting 3 will leave the ESLint guard still active, which will then +fail on the re-introduced pass-throughs. The "net simplification" part +of `ee1734e` that touched the vault layer comes along for the ride; it +does not have its own revert handle. + +If that bundling is ever a problem, the fix is not to rewrite history +(already published) but to issue a targeted revert of only the vault +hunks of `ee1734e` via `git revert -n ee1734e && git checkout --patch +HEAD -- `. diff --git a/src/main/agent-runtime/artifact-projector.ts b/src/main/agent-runtime/artifact-projector.ts index 949392fb..232879f7 100644 --- a/src/main/agent-runtime/artifact-projector.ts +++ b/src/main/agent-runtime/artifact-projector.ts @@ -1,6 +1,8 @@ -// TODO: decompose — M-followup. File is 637 lines and mixes fs-walk -// baseline capture, git-diff projection, and work-product reconciliation. -// Split into baseline / diff / projector modules in a future milestone. +// TODO(M-followup by 2026-05-20): decompose — current file exceeds 500 LOC +// ceiling (637 LOC) and mixes fs-walk baseline capture, git-diff projection, +// and work-product reconciliation. Split into baseline / diff / projector +// modules. If 2026-05-20 passes without decomposition, a follow-up session +// MUST revisit and either decompose or justify the ceiling exception. import { isAbsolute, relative, resolve } from "node:path"; import { execFileSync } from "node:child_process"; import { readdirSync, statSync } from "node:fs"; diff --git a/src/main/agent-runtime/runtime-bridge.ts b/src/main/agent-runtime/runtime-bridge.ts index db4c2c60..1f922077 100644 --- a/src/main/agent-runtime/runtime-bridge.ts +++ b/src/main/agent-runtime/runtime-bridge.ts @@ -1,7 +1,8 @@ -// TODO: decompose — M-followup. File is 648 lines; split into -// sqlite-session-store wiring, cost/metrics routing, and hook-dispatch -// assembly. Tracked as a future milestone; no structural change in this -// pass. +// TODO(M-followup by 2026-05-20): decompose — current file exceeds 500 LOC +// ceiling (648 LOC). Split into sqlite-session-store wiring, cost/metrics +// routing, and hook-dispatch assembly. If 2026-05-20 passes without +// decomposition, a follow-up session MUST revisit and either decompose or +// justify the ceiling exception. import type { AgentMessage } from "@mariozechner/pi-agent-core"; import type { AppDatabase } from "@main/database"; import type { AgentMessageRecord, DesktopEvent } from "@shared/types";