stack/agents/filbert/work/queue-d-review-r1-2026-09-27.md

Queue Piece D review, round 1 (#1508, row 12)

Filbert, 2026-09-27. Plan: queue-as-data-plan-2026-09-26.md §8.9. A2 round 1 review: queue-a2-review-r1-2026-09-27.md (sha256 f167b85e).

Verdict

Changes requested, one item (C1). The review covers build.patch sha256 cb7a6c6649f88496887eacab52db77ef1cc62aaf59a48c3cadcd5f8e33ae1243 on base 8ffbd73b (it applies cleanly at 8efc0ff3), with build-manifest.sha256 5646d938…. Everything else in the candidate is ready. C1 is a gap in my own plan's transition table, and D implements that table as written. Sage can rule it a plan question and approve without it, but I recommend fixing it now: no semantics-2 entry is logged yet, so the fix needs no semantics 3.

I re-hashed the inputs after the review. build.patch and build-manifest.sha256 match the hashes Darkwing sent. build.md changed from b079fbc1… to ced0946b… while I reviewed. The diff is two lines: the helper.patch hash now names helper round 2, and a helper test count went from 56 to 57. Both are in the helper, which is Rocko's scope, so the change doesn't touch this review.

C1. A Jason-gated row can close without its reviewers' verdicts

Where: packages/queue/src/queue.mjs, applyMove, the branch } else if (from === "in-review" && (to === "in-progress" || to === "waiting-on-jason")) { (lines 766–767 in the patched file). It calls only ownerOrPriv.

What happens. On a semantics-2 comment round, in-review→waiting-on-jason checks neither the reviewers' approvals nor the unresolved attempts. The in-review→done path checks both, but a Jason-gated row never takes that path; it goes through waiting-on-jason. After that, waiting-on-jason→done checks only for unresolved attempts. So on a Jason-gated row with reviewers, the listed reviewers are advisory.

Reproduced with a temporary probe test on the review.test.mjs harness (since deleted):

1. set 9 gate-owner jason on a row with reviewers.
2. The owner's move to in-review posts the request (comment 1000). The round's receipts is [].
3. darkwing: move 9 waiting-on-jason exits 0.
4. jason: move 9 done exits 0.

No reviewer recorded a verdict at any point.

Why it matters now. Row 5 (CHAT-03 source) is next through this path. Its gate owner is jason and its reviewers are filbert and rocko. D exists so that the queue, not a seat's memory, holds "reviewed before Jason sees it".

Cause. My plan's table, line 1470, gives in-review→waiting-on-jason as "owner, or privileged | —". I wrote the approval condition only on in-review→done (line 1472). D follows the table.

Fix. When the current round is a comment round, in-review→waiting-on-jason runs refuseUnresolved and requires an approving receipt from every listed reviewer, the same checks as in-review→done (lines 779–787). Share them in one helper. In-review→in-progress stays as it is: that's the changes path, and the next round already refuses unresolved attempts.

• Scope it to cur?.request === "comment". Only semantics-2 entries create such rounds, so v1 replay is unaffected.
• Whether by === "jason" is exempt is Sage's call. I lean against it: an exemption is one more path to test, and the owner can't use it.
• Add a test: the steps above refuse at step 3, then pass once every reviewer has recorded an approval. Add a mutant that drops the check.
• Update the plan table line 1470 and the README's review section to match. I'll make the plan edit if Sage wants it in my file.

What I checked

All of this ran in a scratch clone, /tmp/fqd, with push disabled, on frozen 0444 copies of the inputs in /tmp/fqd-in.

• Manifest and suites. The manifest checks 10/10 after git apply build.patch. node --test packages/queue/tests/ passes 141/141, and the ledger tests pass 51/51. scripts/test-queue.sh reports 27/0 with its "outside the canonical root" skip line, as Sage described.
• The source, read in full: review.mjs, and the diffs to queue.mjs, store.mjs, cli.mjs, scripts/test-queue.sh, the README, the tests and fixtures/fake-gitea.mjs. tools-md.patch matches the code; it's Sage's to apply.
• The deadline. timeout -s KILL kills the helper's grandchild. I checked it with a helper that forks a sleeper: the grandchild is gone after the deadline, and spawnSync reports signal: "SIGKILL", which classifyPost maps to uncertain.
• Mutations. I wrote 24 mutants of my own, separate from Darkwing's 61, and the suite kills all 24. They cover:
  • a same-op retry falling through to a second POST;
  • an abandoned attempt followed by a late post, which must be a conflict;
  • resolve accepting a comment with another id, another author, or no round marker;
  • credCheck skipping the mode check or the realpath check;
  • a 201 with no comment id treated as posted;
  • other HTTP codes treated as failed instead of uncertain;
  • a killed call treated as failed;
  • done without every approval;
  • a new round, or waiting-on-jason→done, ignoring unresolved attempts;
  • a new request while one is posted;
  • resolve without the GET;
  • the body limit removed;
  • sage posting as sage instead of jarvis;
  • verify-commit ignoring the mode or a deletion;
  • the outcome-write failure exiting 1 instead of 3;
  • a failed pre-send still posting;
  • next offering a round the reviewer already answered;
  • abandon leaving duplicateRisk false;
  • record accepting a different candidate digest.

Non-blocking

• n1. Done doesn't require a posted request. If every attempt failed or was abandoned, reviewers can still record approvals and the row can close. That's acceptable: the receipts are the evidence, and the comment is how reviewers are asked, not what they approve.
• n2. A review issue that is a pull request. checkComment needs issue_url to end in /issues/N. I haven't checked what Gitea returns for a comment on a pull request. If it's anything else, resolve refuses. That fails closed, and our review issues are issues, so no change now.
• n3. Abandoning a requesting attempt sets duplicateRisk even when the kill came before the POST. That's per plan: the queue can't tell which side of the POST the kill landed on.
• n4. The body travels on argv as a JSON argument. It's capped at 60,000 bytes, well under the per-argument limit. An E2BIG would classify as uncertain, which is the safe side.
• n5. For Rocko: scripts/gitea-api.sh writes the response to the predictable path /tmp/gitea-api-response.$$. That's the helper, not D.

What I'd keep as it is:

• resolve checking the comment's author against the requester's login. That goes beyond the plan, and it's right;
• semantics recorded per entry, with review verbs refused on v1 entries;
• the late-outcome rules in outcomeState;
• fixed detail strings, so nothing from a response reaches the log.

For Darkwing and Sage

Fix C1 with its test and mutant, re-send the patch and manifest, and I'll review round 2 against this list only. If Sage rules C1 out of D's scope, the candidate is approved as it stands, and C1 has to land before row 5 moves to in-review.