jason.woltjeandClaude Opus 5.5
f539466fcb
queue move ID in-review posts the review request as a Gitea comment and review record reads verdicts back, so reviews stop being files in docs/plans/reviews/. On a comment round, in-review to waiting-on-jason now needs every listed reviewer's approval for the current round, the same as in-review to done (Filbert r1 C1). scripts/gitea-api.sh reads the raw per-seat token files (lead decisions 37 to 39): config built and checked before curl starts, export attribute cleared, fixed base URL. test-queue.sh skips its live checks outside the canonical root. Darkwing authored. Filbert approved D r2 (cf1d3fd0) after r1 (a2dc2302) and corrected the plan (293747cd). Rocko reviewed the helper (e896192f, 2096b0a3), and Sage's lead check passed under decision 38. Manifest b402fb38, 19 files. Co-Authored-By: Claude Opus 5.5 <[email protected]>
91 lines
4.7 KiB
Markdown
91 lines
4.7 KiB
Markdown
# Queue Piece D review, round 2 (#1508, row 12)
|
|
|
|
Filbert, 2026-09-27. Round 1: `queue-d-review-r1-2026-09-27.md`
|
|
(sha256 a2dc2302…). This round checks C1 only, per Sage's ruling to fix it
|
|
in D.
|
|
|
|
## Verdict
|
|
|
|
**Approved.** The review covers `build.patch` sha256
|
|
8a19f7fa03e2ee6c3bd9ce7b2e296acb21655471c4cc56ba19f25464c8129209 (base
|
|
8ffbd73b, applied at cdcedb27), with `build-manifest.sha256` 647d0170…
|
|
and `build.md` e77c8c5e…. The C1 fix is correct. One mutant of mine
|
|
survives: an approval from an earlier round counted in the current one.
|
|
The code handles that case correctly; the tests just don't check it. n1
|
|
gives a four-line test that kills the mutant. I'd add it before the
|
|
commit, and it doesn't need a round 3. I checked it myself (below).
|
|
|
|
## What I checked
|
|
|
|
All of this ran in a scratch clone, `/tmp/fqd2`, at cdcedb27 with push
|
|
disabled, on frozen 0444 copies of the three inputs.
|
|
|
|
- **Manifest and suites.** The manifest checks 10/10. `node --test
|
|
packages/queue/tests/` passes 142/142, and the ledger tests pass 51/51
|
|
(Darkwing's 58 includes the helper patch, which I didn't apply).
|
|
`scripts/test-queue.sh` reports 27/0 with the "outside the canonical
|
|
root" skip line.
|
|
- **What changed.** I compared all ten manifest files with round 1's tree.
|
|
Only `queue.mjs`, `README.md` and `review.test.mjs` differ, as Darkwing
|
|
said.
|
|
- **The fix in `queue.mjs`.**
|
|
- `requireApprovals` is the round-1 inline check moved into a helper.
|
|
in-review→done's messages are unchanged.
|
|
- in-review→in-progress still checks only `ownerOrPriv`, which is right
|
|
for the changes path.
|
|
- in-review→waiting-on-jason runs `ownerOrPriv`, then `refuseUnresolved`,
|
|
then `requireApprovals` when the last round is a comment round.
|
|
`unresolvedAttempts` skips rounds that aren't comment rounds, and v1
|
|
entries can't open one. So v1 replay is unaffected.
|
|
- There's no exemption for jason or sage, as agreed.
|
|
- **The tests.** The new test runs my round-1 reproduction and now
|
|
refuses at the step that used to pass. It covers a reviewer who asked
|
|
for changes, a second round, and reviewers cleared after the round
|
|
opened. The rewritten unresolved test keeps waiting-on-jason→done
|
|
covered through a late post that turns an abandoned attempt into a
|
|
conflict. That's a better route than the old one.
|
|
- **The README** matches the code.
|
|
- **Mutations.** I wrote eight mutants of my own for this round. The
|
|
suite kills seven:
|
|
- G1: sage bypasses the approval check on waiting-on-jason;
|
|
- G2: a `changes` verdict counts as an approval;
|
|
- G3: the check reads the first round, not the last;
|
|
- G4: one approval is enough;
|
|
- G5: the owner-or-privileged check dropped from waiting-on-jason;
|
|
- G6: the unresolved check dropped from waiting-on-jason→done;
|
|
- G7: the approval check dropped from in-review→done.
|
|
|
|
G8 survives: approvals are collected from every round's receipts
|
|
instead of the current round's. See n1.
|
|
- **My plan.** Table line 1470 and 8.9's Receipts paragraph (plan sha256
|
|
293747cd…) describe the code as built.
|
|
|
|
## Non-blocking
|
|
|
|
- **n1. Add a test that an earlier round's approval doesn't count (kills
|
|
G8).** In the new test, filbert approves round 1 and rocko asks for
|
|
changes. In round 2 both approve before anything checks, so an old
|
|
approval counting in round 2 goes unnoticed. In
|
|
`packages/queue/tests/review.test.mjs`, replace the round-2 `for` loop
|
|
(three lines) with:
|
|
|
|
```js
|
|
ok(s.run(["review", "record", "9", "--verdict", "approve", "--comment", "91", "--candidate", head, "--op", "record-9-rock02"], "rocko"));
|
|
// Filbert's round 1 approval doesn't carry into round 2.
|
|
no(s.run(["move", "9", "waiting-on-jason", "--op", "wait-9-000001"], "darkwing"), 2, /row 9 round 2 has no approval recorded by filbert$/m);
|
|
ok(s.run(["review", "record", "9", "--verdict", "approve", "--comment", "90", "--candidate", head, "--op", "record-9-filb02"], "filbert"));
|
|
```
|
|
|
|
I checked it in a copy of the scratch clone: `review.test.mjs` passes
|
|
20/20 on the candidate and fails 1 under G8.
|
|
- **n2. An owner who is also a listed reviewer blocks the row.** `set
|
|
reviewers` and `assign` don't stop the owner from being on the list.
|
|
`record` refuses the owner, but `requireApprovals` still waits for the
|
|
owner's approval. The row then can't reach done or waiting-on-jason
|
|
until a privileged actor runs `set reviewers`. That fails closed, and it
|
|
was already true of done in round 1. The refusal names the owner as
|
|
missing, which could confuse. Don't fix it by dropping the owner from
|
|
the required list: with reviewers `[owner]`, that list would be empty
|
|
and the row would pass with no review. A later fix could refuse the
|
|
owner at `set reviewers` and `assign`. That's a follow-up, not D.
|