From bb2fa9704c9634e12a219b0cdd4cf9bb0be1b664 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Sun, 4 Oct 2026 00:16:41 -0500 Subject: [PATCH] feat(queue): refuse a row's owner as its reviewer at add, set reviewers and assign (row 31, #1508) Built by rocko (Gate G run), approved by filbert in round 1 (#1508 comment 26629). Manifest 35199daf. Suites green on an index export. Co-Authored-By: Claude Opus 5.5 --- packages/queue/README.md | 4 ++++ packages/queue/src/queue.mjs | 9 ++++++-- packages/queue/tests/data.test.mjs | 6 ++++++ packages/queue/tests/review.test.mjs | 32 +++++++++++++++++++++++++--- packages/queue/tests/store.test.mjs | 16 ++++++++++++++ 5 files changed, 62 insertions(+), 5 deletions(-) diff --git a/packages/queue/README.md b/packages/queue/README.md index 7a68fa83..a209079f 100644 --- a/packages/queue/README.md +++ b/packages/queue/README.md @@ -53,6 +53,10 @@ becomes the new list. If it was narrowed (`set ID closes` with `--reason`, or genesis from the map), it keeps the part still among the new issues, and the receipt ends in `(kept narrowed)`. +A row's owner can't also be one of its reviewers: `add`, `set ID reviewers` +and `assign ID SEAT` each refuse rather than create that shape, since a row +can't collect its own owner's required approval. + Exit codes: 0 ok; 1 the operation failed; 2 invalid data or refused; 3 uncertain (visible or durable, not acknowledged; for a review request, not known to be posted); 4 usage. diff --git a/packages/queue/src/queue.mjs b/packages/queue/src/queue.mjs index a5926237..a2c69bc5 100644 --- a/packages/queue/src/queue.mjs +++ b/packages/queue/src/queue.mjs @@ -826,6 +826,7 @@ function applySet(rows, row, entry, resolved) { switch (field) { case "piece": case "gate": case "reviewers": case "issues": requirePriv(by, `change ${field}`); + if (field === "reviewers" && value.includes(row.owner)) throw refuse(`row ${row.id} owner ${row.owner} can't be a listed reviewer`); next[field] = value; // N10: closes follows the issues only if nobody narrowed it. A logged // narrowing keeps its intersection with the new issues. @@ -897,12 +898,15 @@ export function applyEntry(state, entry, resolved) { const brief = checkBrief(resolved.brief); if (brief.path !== parseBriefSpec(a.brief).path || brief.anchor !== parseBriefSpec(a.brief).anchor) throw refuse("resolved brief does not match the brief argument"); const id = highWater + 1; + const owner = a.owner ?? by; + const reviewers = a.reviewers ?? []; + if (reviewers.includes(owner)) throw refuse(`row ${id} owner ${owner} can't be a listed reviewer`); highWater = id; const required = a.required ?? false; const row = { - id, piece: a.piece, owner: a.owner ?? by, issues: a.issues, closes: a.issues, state: "queued", previousState: null, + id, piece: a.piece, owner, issues: a.issues, closes: a.issues, state: "queued", previousState: null, required, requiredSince: required ? entry.at : null, gate: a.gate, gateOwner: a.gateOwner ?? "jason", brief, - after: a.after ?? [], reviewers: a.reviewers ?? [], review: null, claim: null, note: a.note, blockedReason: null, + after: a.after ?? [], reviewers, review: null, claim: null, note: a.note, blockedReason: null, createdAt: entry.at, updatedAt: entry.at, updatedBy: by, }; rows.set(id, row); @@ -945,6 +949,7 @@ export function applyEntry(state, entry, resolved) { requirePriv(by, "assign a row"); if (row.state === "done") throw refuse(`row ${row.id} is done; done rows never change`); if (row.owner === a.seat) throw refuse(`row ${row.id} is already owned by ${a.seat}`); + if (row.reviewers.includes(a.seat)) throw refuse(`row ${row.id} owner ${a.seat} can't be a listed reviewer`); const claim = row.claim ? { seat: a.seat, op: entry.op } : null; rows.set(row.id, touch({ ...row, owner: a.seat, claim }, entry)); result = { row: row.id, field: "owner", from: row.owner, to: a.seat, receipt: receipt(entry, rev, `row ${row.id} owner: ${row.owner}→${a.seat}`) }; diff --git a/packages/queue/tests/data.test.mjs b/packages/queue/tests/data.test.mjs index 85cb3d7e..7ddba5e4 100644 --- a/packages/queue/tests/data.test.mjs +++ b/packages/queue/tests/data.test.mjs @@ -120,6 +120,8 @@ test("add: defaults for an ordinary seat, privileged extras, refusals", () => { refused(() => step(d, "add", { ...add, owner: "rocko" }, "dewey", { brief: brief() }), /only a privileged actor may set owner/); refused(() => step(d, "add", { ...add, reviewers: ["rocko"] }, "dewey", { brief: brief() }), /reviewers/); refused(() => step(d, "add", { ...add, required: true }, "dewey", { brief: brief() }), /required/); + refused(() => step(d, "add", { ...add, owner: "rocko", reviewers: ["rocko"] }, "sage", { brief: brief() }), /row 12 owner rocko can't be a listed reviewer/); + refused(() => step(d, "add", { ...add, reviewers: ["sage"] }, "sage", { brief: brief() }), /row 12 owner sage can't be a listed reviewer/); const d2 = step(d, "add", { ...add, owner: "rocko", gateOwner: "filbert", after: [{ id: 9, when: "done" }], reviewers: ["dewey"], required: true }, "sage", { brief: brief() }); const r2 = row(d2, 12); assert.deepEqual([r2.owner, r2.gateOwner, r2.required, r2.after], ["rocko", "filbert", true, [{ id: 9, when: "done" }]]); @@ -390,6 +392,9 @@ test("field edits: who may change what", () => { const set = (id, field, value, reason = null) => ({ id, field, value, reason }); refused(() => step(d, "set", set(11, "piece", "x"), "dewey"), /privileged/); assert.match(step(d, "set", set(11, "piece", "Renamed"), "sage").log.at(-1).result.receipt, /row 11 piece: Brief template→Renamed$/); + refused(() => step(d, "set", set(9, "reviewers", ["darkwing"]), "sage"), /row 9 owner darkwing can't be a listed reviewer/); + refused(() => step(d, "set", set(9, "reviewers", ["filbert", "darkwing"]), "sage"), /row 9 owner darkwing can't be a listed reviewer/); + assert.deepEqual(row(step(d, "set", set(9, "reviewers", ["dewey"]), "sage"), 9).reviewers, ["dewey"]); refused(() => step(d, "set", set(9, "after", []), "sage"), /only jason may change after on a required row/); step(d, "set", set(9, "after", []), "jason"); step(d, "set", set(11, "after", [{ id: 9, when: "done" }]), "sage"); @@ -544,6 +549,7 @@ test("note: owner, listed reviewer or privileged; empty clears", () => { test("assign moves the claim with the owner; done clears it", () => { let d = row9Started(); refused(() => step(d, "assign", { id: 9, seat: "dewey" }, "darkwing"), /privileged/); + refused(() => step(d, "assign", { id: 9, seat: "filbert" }, "sage"), /row 9 owner filbert can't be a listed reviewer/); d = step(d, "assign", { id: 9, seat: "dewey" }, "sage", {}, "assign-row-9-dewey"); assert.deepEqual(row(d, 9).claim, { seat: "dewey", op: "assign-row-9-dewey" }); assert.match(d.log.at(-1).result.receipt, /owner: darkwing→dewey$/); diff --git a/packages/queue/tests/review.test.mjs b/packages/queue/tests/review.test.mjs index 18bfa242..7a3c46f7 100644 --- a/packages/queue/tests/review.test.mjs +++ b/packages/queue/tests/review.test.mjs @@ -591,12 +591,38 @@ test("semantics: v1 entries replay as before; review entries need v2", async (t) assert.throws(() => queue.canonArgs("review-outcome", { id: 9, attempt: "review-9-00001", outcome: "posted", status: 200, comment: 4, detail: "x" }), /posted exactly when HTTP 201/); }); +test("set reviewers refuses the row's owner (2026-09-28)", async (t) => { + const s = await ready(t); + no(s.run(["set", "9", "reviewers", "filbert,darkwing", "--op", "reviewers-9-self"], "sage"), 2, /row 9 owner darkwing can't be a listed reviewer/); + assert.deepEqual(s.row(9).reviewers, ["filbert"]); +}); + +// `set reviewers` now refuses a list that carries the owner (2026-09-28), so +// this can no longer happen through the CLI. review-record's own refusal +// (queue.mjs, applyReview) is defense in depth for a row that reached this +// shape another way (a pre-fix log, a hand edit); this drives applyEntry +// directly against a state built with the owner spliced into reviewers. test("the owner records no verdict, even as a listed reviewer", async (t) => { const s = await ready(t); ok(s.run(move9(), "darkwing")); - ok(s.run(["set", "9", "reviewers", "filbert,darkwing", "--op", "reviewers-9-self"], "sage")); - no(s.run(["review", "record", "9", "--verdict", "approve", "--comment", "1000", "--candidate", s.head(), "--op", "record-9-self-01"], "darkwing"), 2, /only a listed reviewer other than the owner may record a verdict on row 9/); - ok(s.run(["review", "record", "9", "--verdict", "approve", "--comment", "1000", "--candidate", s.head(), "--op", "record-9-filbert"], "filbert")); + const { queue } = s.m; + const d = s.doc(); + const state = queue.replay(d.log); + const rows = new Map(state.rows); + const r9 = rows.get(9); + rows.set(9, { ...r9, reviewers: [...r9.reviewers, r9.owner].sort() }); + const legacy = { ...state, rows }; + const record = (by, op, comment) => ({ + rev: d.revision + 1, op, verb: "review-record", + args: queue.canonArgs("review-record", { id: 9, verdict: "approve", comment, candidate: s.head() }), + by, at: "2026-09-28T00:00:00.000Z", semantics: 2, result: null, viewSha: null, + }); + assert.throws( + () => queue.applyEntry(legacy, record("darkwing", "record-9-self-01", 1000), {}), + /only a listed reviewer other than the owner may record a verdict on row 9/, + ); + const recorded = queue.applyEntry(legacy, record("filbert", "record-9-filbert", 1000), {}); + assert.equal(recorded.result.verdict, "approve"); }); test("a request comment over the length limit is not sent", async (t) => { diff --git a/packages/queue/tests/store.test.mjs b/packages/queue/tests/store.test.mjs index e2414ca9..b69a0e49 100644 --- a/packages/queue/tests/store.test.mjs +++ b/packages/queue/tests/store.test.mjs @@ -234,6 +234,22 @@ test("claims and add defaults through the CLI; candidates are manifests or reach assert.equal(row(repo, 6).claim, null); }); +test("add, set reviewers and assign refuse the row's owner as a reviewer", (t) => { + const repo = ready(t); + const before = doc(repo); + no(cli(repo, ["add", "--op", "add-owner-reviewer", "--piece", "x", "--gate", "g", "--brief", "docs/plans/brief-b.md#Template", "--owner", "rocko", "--reviewer", "rocko"], { by: "sage" }), 2, /row 12 owner rocko can't be a listed reviewer/); + assert.deepEqual(doc(repo), before); + no(cli(repo, ["set", "9", "reviewers", "filbert,darkwing", "--op", "set-9-owner-reviewer"], { by: "sage" }), 2, /row 9 owner darkwing can't be a listed reviewer/); + assert.deepEqual(doc(repo), before); + no(cli(repo, ["assign", "9", "filbert", "--op", "assign-9-filbert"], { by: "sage" }), 2, /row 9 owner filbert can't be a listed reviewer/); + assert.deepEqual(doc(repo), before); + // The unchanged accept paths: a reviewer list or a new owner that does not collide with the other. + ok(cli(repo, ["set", "9", "reviewers", "filbert,dewey", "--op", "set-9-ok"], { by: "sage" })); + assert.deepEqual(row(repo, 9).reviewers, ["dewey", "filbert"]); + ok(cli(repo, ["assign", "11", "rocko", "--op", "assign-11-rocko"], { by: "sage" })); + assert.equal(row(repo, 11).owner, "rocko"); +}); + test("the working-brief check: a changed working copy refuses the start and flags next", (t) => { const repo = ready(t); writeFileSync(join(repo.root, "docs/plans/brief-b.md"), "# Briefs B\n\n## Queue\n\nEdited.\n\n## Template\n\nEdited.\n");