stack/agents/filbert/work/queue-186/r1/body.md

Filbert review: row 186 round 1 (#1684), approve

Request 29418. Candidate manifest 449e91219333c88e4f3e99cf4fe3e4053a527aff4fa2068067214c0d7fd5cfff (3 paths) on base 1d7fd75e2. The brief is docs/plans/2026-10-20_plan-backup-line.md. This row is my N1 and N2 on row 185 (29404).

Verdict: approve. No findings block. N1 is a test note, and N2 rules on the reading BUILD.md raises.

What I ran

• I built ~/filbert-scratch/r186-r1/wt at b367f5730 (the refactor head), then ran git apply --index build.patch.
  • sha256sum -c on the manifest passed 3/3.
  • The patch-id is c97b9f6e25cd: 3 files, +44/-8.
• Base drift: git log 1d7fd75e2..refactor on the 3 paths is empty.
• Readers and pins: the only test that reads any of the 3 paths is stack-release.test.mjs. No 64-hex pin names them.
• I took the slot on sage-1262. The /proc busy check was CLEAR at 08:53:56Z and 08:58:52Z, and TMPDIR was ~/fs186.
  • stack-release.test.mjs: 132/132 pass, 0 fail, rc 0, from 08:53:57Z to 08:58:08Z. The counts are awk sums over the spec log.
• I ran two mutants of my own against the plan tests (^(T15|T23|T26|T27|T29|T50|T51|T52) , 18 tests), each restored by git checkout. After them the tree had only the 3 staged paths.
  • M7 sets early=1 in the S_CHANGED branch. Killed: rc 1, T29 failed. So the "keep the backup line on an S_CHANGED read" choice is pinned.
  • M6 moves the early test ahead of the no-store test. It survived: rc 0, 18/18. See N1.
• Darkwing's run (132/132, base control, m1-m3 killed) and Sage's gate are their receipts, not mine.

Checked against the brief

• Change 1. local early (:802) is set in the in-chain branch only without --migrate (:812), and in the store-newer branch (:815). These are the two places where run_steps (:846-847 at the base) rolls back before backup_store.
  • The backup block (:819-827) prints no line for a rollback, then the no-store line, then the new backup: none; the run rolls back before the backup line, then the backup line.
  • Same-schema and in-chain --migrate activates keep the backup line.
• Change 2. On an S_CHANGED read, the migration block prints nothing and early stays empty, so the backup line stays. I agree with BUILD.md's reason. A wrong backup line there means the run rolled back before it touched the store. A wrong "none" line would hide the directory a --restore needs. T29 pins it (my M7).
• Change 3. The script diff is inside plan only. run_steps, backup_store, the refusal codes, exit codes and flags are untouched.
• Change 4. TOOLS.md step 3 names the backup and both "none" reasons.
• Brief tests 1-6. T52's six subtests plan, check nothing was written (fp, the data tree, no starts or stops, no log line, no backups), then run the same activate. They check that backups/stack exists, and that the log line's backup is the activation, exactly when the plan printed the backup line.
  • T26's not-given case now expects the none line. T50 and T51 are unchanged.
  • That covers the brief's tests 1-6.
• Brief mutants. Darkwing's m1-m3 are the brief's three, so they're their receipts.

Notes

• N1 (test gap, doesn't block): no test pins that the no-store test comes before early, so my M6 survives.
  • With no store file, plan_store uses the pointer's schema. If that schema is outside the target's chain, or in it without --migrate, M6 prints "the run rolls back before the backup". But the run skips the whole store block (:840 at the base) and goes on to install.
  • BUILD.md argues this order on purpose, and the candidate's order is right.
  • The brief's test 5 is only T51's same-schema case, so this isn't a missing brief-required test.
  • The migration line in that case already says the run "would roll back with store-newer" (or migration-needs-flag), which is wrong for a missing store too. That's pre-existing, from before row 185.
  • A follow-up could add a no-store subtest with a pointer schema outside the chain to T52, and fix that migration line in the same row.
• N2 (my ruling on BUILD.md's reading outside the brief): I agree to leave it as a bounded follow-up.
  • When the bus is stopped and the store is unclean or unreadable, the plan falls back to the pointer's schema and can print the backup line, while the run rolls back first with store-unclean or schema-unreadable.
  • The plan's store schema line already says the store wasn't read and why. That makes it a narrower case than the old N1, where nothing hinted at it.
  • Only the stopped-and-unclean case is certain to roll back. A held store whose read failed may read fine once the run stops the bus. So a follow-up should use the store-unclean form only when the plan saw the stopped store unclean. It's Sage's call whether to file it with N1's.