review: M5c-2 review, lessons T59 and T60, and the gate-count rule

Co-Authored-By: Grok 4.6 <noreply@x.ai>
This commit is contained in:
2026-10-02 20:24:43 -07:00
co-authored by Grok 4.6
parent f80842640c
commit 1938b746ab
4 changed files with 71 additions and 7 deletions
+6 -3
View File
@@ -32,7 +32,8 @@ How it is used:
| I14 | Recovering from a crash is a case to write a test for, not only to reason about. A file the daemon itself can leave behind (created, not yet written) must load like an empty one. | M3a finding 1: one zero-length log file made `brokerd` panic at startup, where the same directory verified as `ok, 0 records`. |
| I15 | Never write the model's chat-template tags out whole (the think and tool-call tags, the angle-bar turn markers) in code, comments, commands or the log. Build them from pieces (`concat!("<", "think")`) and name them in words. Text holding them turns into the model's own control tokens when it reads it. | M5a task 04: the given test held the tags; Ornith's reasoning ended at the test file's closing think tag, and it lost track of the file and made no change. `docs/research/2026-09-24-chat-template-tokens.md`. | yes | M5a tasks 05 to 22: not seen again. |
| I16 | When a test outside your task fails, check once whether it fails without your change (stash, run, unstash), then stop and report both results. Do not keep experimenting, and never add a file the task does not list, not even a temporary one. | M5a task 21: the session did the check, then spent about 80 minutes on experiments and wrote two diagnostic test files into `crates/bxctl/tests/` until the owner stopped the run; the cause was outside the repository (review finding 4). M5a task 16 did it right: one check, its log row, a stop. | yes | M5c-1: the stash left the new untracked tests behind and a baseline printed no total (task 15; now `git stash -u` in AGENTS.md, T53); the two flaky tests were rerun and named, not stopped on (now an exception for a test the README lists, T52). |
| I17 | Write your log row from `git diff` after staging, naming only what the diff shows. A row written from memory describes what you meant to do. | M5a task 10's row describes checks that are not in `serve.rs`; task 13's says two runbook fragments were copied, and neither was (review findings 5 and 6 of 2026-09-24). | yes | M5c-1: task 10 logged one gate run and ran three, task 12 logged three and ran four and "Deviations: none" after anchoring an edit on its own line, and task 14's row blames the task for its own spacing error. |
| I17 | Write your log row from `git diff` after staging, naming only what the diff shows. A row written from memory describes what you meant to do. | M5a task 10's row describes checks that are not in `serve.rs`; task 13's says two runbook fragments were copied, and neither was (review findings 5 and 6 of 2026-09-24). | yes | M5c-1: task 10 logged one gate run and ran three, task 12 logged three and ran four and "Deviations: none" after anchoring an edit on its own line, and task 14's row blames the task for its own spacing error. M5c-2: every row's first gate is `pass`; task 01 still ran the gate twice, and every session ran `cargo test --workspace` after `gate: ok` only to count (T59). |
| I18 | After `gate: ok`, do not run `cargo test --workspace` (or the gate) only to count tests. The gate's total is the count. If HEAD already has this task's commit and a `done` row, stop; do not start the task again. | M5c-2: every session re-ran the workspace tests after a passing gate (T50 still driving extra work). Task 05's run log is a resume after HEAD was already at task 12; Laguna did not re-commit, and the original transcript was truncated. | yes | |
## Tips for writing tasks
@@ -91,11 +92,13 @@ How it is used:
| T51 | Never tell the implementer to run a command that the harness denies or that AGENTS.md forbids. Write the branch line as a check ("`git branch --show-current` prints `m5c1`"), not as `git switch`. | M5c-1: all fifteen transcripts start with a refused `git switch m5c1`; AGENTS.md forbids switching branch. |
| T52 | List the known flaky tests in the plan README, with what to do (a test outside the task that fails once, then passes on a rerun and in the gate, is named in the log row's Notes and the task goes on). Before hand-over, stress the whole workspace under load, not only the new tests (T32). | M5c-1 task 01: gatewayd's `brokerd_closing_first_ends_it_as_no_longer_pending` (2 of 10 runs fail on a loaded talos with no M5c-1 change); task 15: loopd's `pointers`. Both older tests; Laguna went on in both cases, and task 01's row calls a different test "the only flake". |
| T53 | Give the baseline in the task so the implementer never has to measure one. Where a check without the change is needed, the stash is `git stash -u`: a plain stash leaves the new untracked tests behind, and they no longer compile. | M5c-1 task 15: a baseline run printed an empty total; task 03 moved the new tests aside by hand before stashing. |
| T54 | Give an import change as a single added `use` line, never as a rewritten import block: the implementer's Edit retypes the lines around it, and the next task's "before" block stops matching. | M5c-1 task 12: its W1 edit rewrote the import block in another order, W2's "before" block then failed to match, and Laguna anchored on its own line instead of stopping (logged as "Deviations: none", I17). Task 06: an M2 replacement landed in the wrong `match`. |
| T54 | Give an import change as a single added `use` line, never as a rewritten import block: the implementer's Edit retypes the lines around it, and the next task's "before" block stops matching. | M5c-1 task 12: its W1 edit rewrote the import block in another order, W2's "before" block then failed to match, and Laguna anchored on its own line instead of stopping (logged as "Deviations: none", I17). Task 06: an M2 replacement landed in the wrong `match`. M5c-2: tasks 06, 07 and 10 rewrote import blocks again; 06 restored rustfmt's order before the commit, 07 anchored H2 after H1 reordered uses. |
| T55 | Put "its spaces are checked" on every edit to a literal whose whitespace matters, not only the first. | M5c-1: task 04's C1 had the note and was right first time; task 14's U1 did not, and got one extra leading space (caught by a test; the row blames the task). |
| T56 | When a task uses an external crate, say that the task's table of its API is complete and that the crate's source (the Cargo registry) is outside the repository and off limits; better, put the signatures needed into `files/`. | M5c-1 tasks 10, 11 and 13 each tried to read `roncho`'s source in `~/.cargo/registry` (`find / … roncho`); only the harness stopped them. |
| T57 | Split a task that combines skeleton fills with more than about fifteen before/after edits over four or more files. | M5c-1: tasks of one skeleton or only listed edits took 6 to 14 minutes; 09 (18), 15 (20, 17 test files and about 30 edits) and 13 (25, two skeletons and ten edits) were the slow ones. |
| T58 | For a class ceiling or trust rule in a spec, list every path that writes to the egress it guards (turns, mirrored memory writes, undo, review accept, notes) and give each path a test at the forbidden class. A rule written for one path is not checked on the others by default. | M5c-1 review finding 1: the `secret` ceiling was built and tested for turns only; memory writes from a `secret` session were mirrored to the trusted and fleet workspaces (the reference had it too). Finding 2 is the same shape for undo. |
| T58 | For a class ceiling or trust rule in a spec, list every path that writes to the egress it guards (turns, mirrored memory writes, undo, review accept, notes) and give each path a test at the forbidden class. A rule written for one path is not checked on the others by default. The same for a rule that a class *must* still take a path: give that path a test at that class. | M5c-1 review finding 1: the `secret` ceiling was built and tested for turns only; memory writes from a `secret` session were mirrored to the trusted and fleet workspaces (the reference had it too). Finding 2 is the same shape for undo. M5c-2 review finding 1: a `secret` session's Honcho query was implemented and unit-tested for `has_secret`, but not at the serve layer against the fake Honcho. |
| T59 | After `gate: ok`, the task is done counting. Do not tell the implementer to run `cargo test --workspace` (or the gate) again only to compare a number. AGENTS.md already forbids a second gate "only to count"; say the same of the workspace test run. | M5c-2: all twelve sessions ran `cargo test --workspace` after a passing gate to rediscover the total, even with T50's wording in the task. |
| T60 | The driver must not truncate a task's run log, and must not start a task whose commit and `done` row are already in HEAD. A power-loss resume continues the existing log file; it does not open it for write from offset zero, and it skips a task that is already committed. | M5c-2 task 05: `.state/runs/M5c2/05-memoryd-honcho-readers.log` is four `=== …` resume sections after HEAD was at task 12; the original transcript is gone. Commit `b370c60` was already complete. |
## What worked and should be kept