feat(queue): Piece D, reviews as issue comments, raw per-seat token helper (row 12, #1508)
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]>
This commit is contained in:
Claude Opus 5.5
@@ -1467,7 +1467,7 @@ cooperative trust model (J2).
|
||||
| in-progress→briefed (`release`) | claimant, or privileged | clears `claim` |
|
||||
| in-progress→in-review | claimant | once D is built, for a row with `reviewers`, this move is the review request, and it takes `--candidate` (8.9). Before D, or with no reviewers, it opens the round with `request: none` |
|
||||
| in-review→in-progress | owner, or privileged | changes requested (Sage) |
|
||||
| in-review→waiting-on-jason | owner, or privileged | — |
|
||||
| in-review→waiting-on-jason | owner, or privileged | with D built, on a comment round: refused while any attempt is unresolved, and it needs the approving receipts of every listed reviewer for that round, the same checks as in-review→done (8.9). Before D, or on a round with `request: none`: no condition |
|
||||
| waiting-on-jason→done | `jason`; or `sage` with `--evidence` citing Jason's approval | the evidence reference is logged |
|
||||
| in-review→done | the row's `gateOwner`, or privileged | refused when `gateOwner` is `jason` (that path runs through waiting-on-jason). `--evidence` must name the current round and its candidate digest: with D built, the approving receipts of every listed reviewer for that round (8.9); before D, a comment id together with the round's candidate digest, which must match. Logged (J5) |
|
||||
| any non-terminal→blocked | owner, or privileged | reason required; records `previousState`; blocked→blocked refused (update the reason with `note`) |
|
||||
@@ -1688,6 +1688,10 @@ The CLI may print the issue URL and the marker as a hint. It never decides.
|
||||
**Receipts.** `review record` is accepted from a listed reviewer, for the
|
||||
current round, citing the comment id and the candidate digest that was
|
||||
approved. A mismatch refuses. Every round is kept in `review.rounds[]`.
|
||||
On a comment round, the receipts gate both moves out of in-review toward
|
||||
done: in-review→done, and in-review→waiting-on-jason, which is the route
|
||||
for a Jason-gated row. Each needs an approval from every listed reviewer
|
||||
and no unresolved attempt.
|
||||
|
||||
**Tests** use a fake transport:
|
||||
- a kill:
|
||||
@@ -2402,3 +2406,8 @@ The round-2 modifications:
|
||||
- the guard checked as active on every invocation, including a
|
||||
`git hook run` canary (G2), checked in a scratch repository;
|
||||
- the lead's expected login is `jarvis`. 8.19 maps the findings.
|
||||
- 2026-09-27: correction from Filbert's Piece D review round 1 (C1),
|
||||
ruled by Sage. The transition table let in-review→waiting-on-jason pass
|
||||
with no reviewer approvals, so a Jason-gated row could close without its
|
||||
reviewers. That move now carries the in-review→done checks on a comment
|
||||
round; the table and 8.9's receipts paragraph say so.
|
||||
|
||||
@@ -0,0 +1,142 @@
|
||||
# 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.
|
||||
@@ -0,0 +1,90 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user