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 <[email protected]>

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.

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}`) };

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$/);

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"));
+  const { queue } = s.m;
-  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/);
+  const d = s.doc();
-  ok(s.run(["review", "record", "9", "--verdict", "approve", "--comment", "1000", "--candidate", s.head(), "--op", "record-9-filbert"], "filbert"));
+  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) => {

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");