mosaicstack/stack
25
0
Fork 1
Code Issues 348 Pull Requests 13 Actions Packages Projects Releases 15 Wiki Activity

feat(queue): refuse a row's owner as its reviewer at add, set reviewers and assign (row 31, #1508)

Browse Source 
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]>
This commit is contained in:
jason.woltje
2026-10-04 00:16:41 -05:00
co-authored by Claude Opus 5.5
parent 1649f5e2a3
commit bb2fa9704c
5 changed files with 62 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/queue/README.md
+4
View File
@@ -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
+7 -2
View File
@@ -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
+6
View File
@@ -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
+29 -3
View File
@@ -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) => {
packages/queue/tests/store.test.mjs
+16
View File
@@ -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");