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.