diff --git a/docs/plans/2026-10-13_retest-faults.md b/docs/plans/2026-10-13_retest-faults.md new file mode 100644 index 000000000..523e9e074 --- /dev/null +++ b/docs/plans/2026-10-13_retest-faults.md @@ -0,0 +1,139 @@ +# Faults from Jason's PM re-test on 664199f7 + +Jason's PM re-test finished on 2026-10-13 (Mos, PM run rcc0f72be8b17, +Vikunja #228). The #229 loop worked end to end: the coder pushed +010d30cb, the reviewer approved it after running its tests, and the PM +closed the task. Mos listed four faults. Sage checked two of them and +neither holds, so neither gets a row: + +- The reviewer did run the tests. Its own transcript shows + `node --test` in a clean worktree at 010d30cb (14 pass) eight seconds + before its verdict. Its first prompt was the coder's REVIEW-REQUEST, + so it didn't need the PM's later message. +- The PM's clone wasn't stale. Row 95's landing 17ad6f35 is an ancestor + of both commits the clone has held (4da68152 at clone, 664199f7 after + the launch fast-forward), and `app.js` and `board-held.test.mjs` are + tracked in it. The PM's search missed the code. + +The other two get rows here. + +## Seats commit under their own name + +### Problem + +A role session runs with HOME set to the run's empty `home/` and no git +identity. The coder's first commit failed with "Author identity +unknown", and the coder got past it with `git config user.name` and +`user.email` in its clone. + +That workaround breaks the next launch. `allowed()` in +`packages/seat/src/checkout.mjs` keeps a clone whose config holds any +key outside its list, so the coder's clone will be kept, not +fast-forwarded, from now on. Its config holds both keys today. + +A related confusion: the PM said its clone of `stack` on branch +`refactor` had no Board code when it did. Nothing tells a seat which +branch and commit its clone holds or when it refreshes. + +### Owner and reviewer + +Owner: Darkwing, after row 103 (both rows touch `packages/seat`). +Reviewer: Dewey (a sandbox row, decision 130). + +### Files owned + +- `packages/seat/src/sandbox.mjs` and whichever launch file passes it + the role and business, with their tests +- `packages/harness/src/bundle.mjs`, one prompt line, and its test +- `packages/seat/README.md` +- `agents/darkwing/work/queue-/` + +### What ships + +- Every role session's sandbox sets `GIT_AUTHOR_NAME`, + `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_NAME` and `GIT_COMMITTER_EMAIL`: + the name is the role, the email `@.mosaic.invalid`. A + test commits in a sandboxed clone with no config and reads the author + back. +- The clone's config needs no new key, so `allowed()` doesn't change. +- The bundle prompt gets one line: the clone's branch and the commit it + was at when the session started, that it refreshes only at launch, and + that setting git config in it stops the next launch from updating it. +- The README says where the identity comes from. + +Each new check is shown to fail on a mutant that removes it. + +Suites: the seat and harness node suites and every `scripts/test-*.sh`. + +### Out of scope + +Changing `allowed()`. Signing. Fixing the coder's existing clone: after +landing, with Mos's go, Sage unsets the two keys so the next launch +updates it again. + +### Gate + +Dewey approves on the row's issue, naming the candidate manifest, and +the suites above pass on Sage's gate rerun. Decision 109 notices go to +Mos before and after. The launch side is live only at a bus restart, +and that needs Mos's go. + +## A review request says whether a reviewer is running + +### Problem + +`review.request` in `packages/bus/src/broker.mjs` routes the +REVIEW-REQUEST to whichever run holds the reviewer role, or to nobody +when no run holds it. The message then waits until a run claims the +role. The reply is the same either way. + +In the re-test the coder asked before any reviewer held the role. The +PM happened to launch one, which got the request as its first prompt, +so the loop worked. But the coder couldn't tell that its request was +waiting on nobody, and nothing told anyone to launch a reviewer. Had +nobody launched one, the request would have sat there unseen. + +Refusing the request isn't the fix. Delivery on claim is what lets the +PM launch a reviewer after the coder asks, and that path worked. The +fix is to say so. + +### Owner and reviewer + +Owner: Researcher (a Haiku subagent may build it, decision 129). +Reviewer: Filbert (decision 130). + +### Files owned + +- `packages/bus/src/broker.mjs`, the `review.request` case only, and + its tests +- `packages/harness/src/tools.mjs`, the `request_review` description + only, and its test +- `packages/bus/README.md`, the review paragraph only +- `agents/researcher/work/queue-/` + +### What ships + +- The reply adds `held`: the run id holding the reviewer role when the + request was routed, or `null`. Tests cover both, and the existing + reply fields don't change. +- The `review.requested` event records the same value. +- The `request_review` description says what `null` means: no reviewer + is running, the request waits for one, and the requester should tell + whoever launches roles. +- The README says the same. + +Each new check is shown to fail on a mutant that removes it. + +Suites: the bus and harness node suites and every `scripts/test-*.sh`. + +### Out of scope + +Refusing a request to an unheld role. Launching a reviewer +automatically. The Console. Plain `message.send` to an unheld role. + +### Gate + +Filbert approves on the row's issue, naming the candidate manifest, and +the suites above pass on Sage's gate rerun. Decision 109 notices go to +Mos before and after. The bus side is live only at a bus restart, and +that needs Mos's go.