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 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.
This commit is contained in:
ZacharyZhang-NY committed
1 parent
69d03312e6
commit
ded707d960
6 files changed
+256
-14
No files matched your search
@@ -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
|
||||
|
||||
+111
@@ -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.<module>.<method>(...)` (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 <C1 part 2/3 sha>
|
||||
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 -- <vault-files>
|
||||
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.
|
||||
@@ -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
|
||||
|
||||
@@ -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.<method>(...)` → `db.<module>.<method>(...)` 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\.(<method>)\(` → `$1$2db.<module>.<method>(`.
|
||||
|
||||
### 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.<method>(...)` against a deleted method, and the
|
||||
TypeScript compiler would surface it as `Property '<method>' 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 -- <vault-files>`.
|
||||
@@ -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";
|
||||
|
||||
@@ -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";
|
||||
|
||||
Reference in new issue
Block a user