fix(queue): genesis owner-not-reviewer check, assign wording, queue-commit HEAD-moved message, calendar dates (row 33, #1508)
Built by filbert, approved by darkwing in round 1 (#1508 comment 26651). Manifest fe7da3ef, 10 paths. Suites green on an index export. Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
Claude Opus 5.5
@@ -53,9 +53,11 @@ 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.
|
||||
A row's owner can't also be one of its reviewers: `add`, `set ID reviewers`,
|
||||
`assign ID SEAT` and `genesis` (for a map row) each refuse rather than
|
||||
create that shape, since a row can't collect its own owner's required
|
||||
approval. Replay doesn't check it, so a log written before the rule still
|
||||
loads.
|
||||
|
||||
Exit codes: 0 ok; 1 the operation failed; 2 invalid data or refused;
|
||||
3 uncertain (visible or durable, not acknowledged; for a review request,
|
||||
@@ -145,7 +147,10 @@ semantics 2.
|
||||
- `docs/plans/queue.json`: `{version, canonicalRoot, revision, rows, log}`,
|
||||
serialized deterministically. A file that doesn't re-serialize byte for
|
||||
byte, or doesn't replay from genesis to its `rows`, is refused by every
|
||||
verb.
|
||||
verb. Times are ISO times in UTC to the millisecond; `requiredSince` and
|
||||
`createdAt` may also be a bare date. Each must be a real calendar day and
|
||||
time: a value that doesn't format back to itself, such as month 13 or
|
||||
2026-02-30, is refused.
|
||||
- `docs/plans/QUEUE.md`: hand-written header, generated body between the
|
||||
markers. Each log entry records the SHA-256 of the body it rendered, so a
|
||||
body is `current`, `stale` (an earlier render) or `unknown` (no render).
|
||||
@@ -212,8 +217,12 @@ is swept in:
|
||||
tests and `verify --snapshot` against the base.
|
||||
5. to 7. Blobs, a tree from a temporary index, `commit-tree`, the guard
|
||||
checks again, then `update-ref` with H as the expected old value. If the
|
||||
branch moved at any point after step 1, this fails and nothing is
|
||||
published.
|
||||
branch moved at any point after H was recorded, nothing is published.
|
||||
Each guard check compares HEAD with H before its canary and again if
|
||||
the clean run fails. A move found there exits 2 with `HEAD moved since
|
||||
H was recorded`, since the canary's index holds H's tree and the hook
|
||||
diffs it with the live HEAD. A move after the step-7 check fails
|
||||
`update-ref` (exit 1).
|
||||
8. Reconcile the shared index's two queue entries with `git reset -q --
|
||||
docs/plans/queue.json docs/plans/QUEUE.md`, only if HEAD is the new
|
||||
commit and those entries are still H's. Otherwise exit 3 and print the
|
||||
|
||||
@@ -141,7 +141,15 @@ function checkAfter(v, what = "after") {
|
||||
|
||||
function checkTime(v, what, { unknown = false, date = false } = {}) {
|
||||
if (unknown && v === "unknown") return v;
|
||||
if (typeof v === "string" && (ISO_RE.test(v) || (date && DATE_RE.test(v)))) return v;
|
||||
const iso = typeof v === "string" && ISO_RE.test(v);
|
||||
if (iso || (date && typeof v === "string" && DATE_RE.test(v))) {
|
||||
// The patterns check the shape only. A value passes if it formats back to
|
||||
// itself, which refuses month 13, day 32 and 2026-02-30 (V8 reads that
|
||||
// as March 2).
|
||||
const t = Date.parse(iso ? v : `${v}T00:00:00.000Z`);
|
||||
if (Number.isFinite(t) && new Date(t).toISOString().slice(0, v.length) === v) return v;
|
||||
throw refuse(`${what} is not a calendar ${iso ? "time" : "date"}: ${JSON.stringify(v)}`);
|
||||
}
|
||||
throw refuse(`${what} must be an ISO time${date ? " or date" : ""}${unknown ? " or \"unknown\"" : ""}: ${JSON.stringify(v)}`);
|
||||
}
|
||||
|
||||
@@ -949,7 +957,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`);
|
||||
if (row.reviewers.includes(a.seat)) throw refuse(`can't assign row ${row.id} to ${a.seat}: ${a.seat} is one of its reviewers`);
|
||||
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}`) };
|
||||
@@ -1023,6 +1031,9 @@ export function parseMigrationMap(text) {
|
||||
map.rows.forEach((r) => {
|
||||
keysExactly(r, MAP_ROW_KEYS, `migration map row ${r?.id}`);
|
||||
if (r.brief !== null) keysExactly(r.brief, ["path", "anchor"], `migration map row ${r.id} brief`);
|
||||
// The rule add, set and assign apply. Here, not in checkGenesis, so a
|
||||
// log written before the rule still replays.
|
||||
if (Array.isArray(r.reviewers) && r.reviewers.includes(r.owner)) throw refuse(`migration map row ${r.id} owner ${r.owner} can't be a listed reviewer`);
|
||||
});
|
||||
if (!Array.isArray(map.retired)) throw refuse("migration map retired must be a list");
|
||||
map.retired.forEach((id) => checkId(id, "retired id"));
|
||||
|
||||
@@ -223,11 +223,11 @@ test("F1: step 8 with index.lock held exits 3, and ordinary commits stay refused
|
||||
assert.equal(r.g("rev-parse", "HEAD^").trim(), c);
|
||||
});
|
||||
|
||||
test("F1: HEAD moving after commit-tree and before update-ref: refused, nothing published", (t) => {
|
||||
test("F1: HEAD moving after the step-7 guard check and before update-ref: refused, nothing published", (t) => {
|
||||
const r = ready(t);
|
||||
note(r);
|
||||
const h = r.head();
|
||||
r.on("git", `if [ "$phase $sub" = "after commit-tree" ] && once move; then
|
||||
r.on("git", `if [ "$phase $sub" = "before update-ref" ] && once move; then
|
||||
echo other > "$R/other.txt"; git -C "$R" add other.txt; git -C "$R" commit -q -m other
|
||||
fi`);
|
||||
const res = r.qc(["-m", "queue rev 1"]);
|
||||
@@ -238,7 +238,7 @@ fi`);
|
||||
assert.equal(r.g("log", "-1", "--format=%s").trim(), "other");
|
||||
});
|
||||
|
||||
test("F1: H is recorded before the canary, so HEAD moving during the canary makes update-ref fail", (t) => {
|
||||
test("F1: H is recorded before the canary, so HEAD moving during the step-1 canary is refused at step 7", (t) => {
|
||||
const r = ready(t);
|
||||
note(r);
|
||||
const h = r.head();
|
||||
@@ -246,13 +246,58 @@ test("F1: H is recorded before the canary, so HEAD moving during the canary make
|
||||
echo other > "$R/other.txt"; git -C "$R" add other.txt; git -C "$R" commit -q -m other
|
||||
fi`);
|
||||
const res = r.qc(["-m", "queue rev 1"]);
|
||||
assert.equal(res.code, 1, res.err);
|
||||
assert.match(res.err, new RegExp(`moved since ${h}`));
|
||||
assert.equal(res.code, 2, res.err);
|
||||
assert.match(res.err, new RegExp(`refused \\(step7\\): HEAD moved since ${h} was recorded \\(another commit landed\\); run queue-commit.sh again`));
|
||||
assert.equal(r.g("log", "-1", "--format=%s").trim(), "other");
|
||||
assert.equal(r.g("rev-parse", "HEAD^").trim(), h);
|
||||
assert.equal(r.revAt("HEAD"), 0);
|
||||
// Both guard checks ran in full (two canary runs each), so only update-ref caught the move.
|
||||
assert.equal(r.calls("git").filter((c) => c === "before hook").length, 4);
|
||||
// A commit outside the queue files passes the step-1 canary. The step-7
|
||||
// check refuses before its canary, so update-ref never runs.
|
||||
assert.equal(r.calls("git").filter((c) => c === "before hook").length, 2);
|
||||
assert.ok(!r.calls("git").includes("before update-ref"));
|
||||
});
|
||||
|
||||
// Another queue commit landing after H is recorded. The canary's index
|
||||
// holds H's tree and the hook diffs it with the live HEAD, so before
|
||||
// 2026-10-04 the clean run failed and the script blamed the guard.
|
||||
const NESTED = 'bash "$R/scripts/queue-commit.sh" -m nested >> "$CTL/nested.log" 2>&1; echo $? > "$CTL/nested-exit"';
|
||||
|
||||
test("F1: a queue commit landing after H is recorded: step 1 says HEAD moved, not the guard", (t) => {
|
||||
const r = ready(t);
|
||||
note(r);
|
||||
const h = r.head();
|
||||
r.on("git", `if [ "$phase $sub" = "after symbolic-ref" ] && once move; then ${NESTED}; fi`);
|
||||
const res = r.qc(["-m", "queue rev 1"]);
|
||||
assert.equal(r.ctlFile("nested-exit"), "0", r.ctlFile("nested.log"));
|
||||
assert.equal(res.code, 2, res.err);
|
||||
assert.match(res.err, new RegExp(`refused \\(step1\\): HEAD moved since ${h} was recorded \\(another commit landed\\); run queue-commit.sh again`));
|
||||
assert.doesNotMatch(res.err, /not running the queue guard/);
|
||||
assert.equal(r.calls("git").filter((c) => c === "before hook").length, 0);
|
||||
assert.equal(r.g("log", "-1", "--format=%s").trim(), "nested");
|
||||
assert.equal(r.revAt("HEAD"), 1);
|
||||
// Running it again, as the message says, finds the queue already committed.
|
||||
r.off("git");
|
||||
note(r);
|
||||
const again = r.qc(["-m", "queue rev 2"]);
|
||||
assert.equal(again.code, 0, again.err);
|
||||
assert.equal(r.revAt("HEAD"), 2);
|
||||
});
|
||||
|
||||
test("F1: a queue commit landing between the HEAD check and the canary: the failed clean run is reported as HEAD moved", (t) => {
|
||||
const r = ready(t);
|
||||
note(r);
|
||||
const h = r.head();
|
||||
r.on("git", `if [ "$phase $sub" = "before hook" ] && once move; then ${NESTED}; fi`);
|
||||
const res = r.qc(["-m", "queue rev 1"]);
|
||||
assert.equal(r.ctlFile("nested-exit"), "0", r.ctlFile("nested.log"));
|
||||
assert.equal(res.code, 2, res.err);
|
||||
assert.match(res.err, new RegExp(`refused \\(step1\\): HEAD moved since ${h} was recorded \\(another commit landed\\); run queue-commit.sh again`));
|
||||
assert.doesNotMatch(res.err, /not running the queue guard/);
|
||||
// The clean run did run, and failed on the moved HEAD.
|
||||
assert.equal(r.calls("git").filter((c) => c === "before hook").length, 1);
|
||||
assert.ok(r.calls("git").includes("after hook"));
|
||||
assert.equal(r.g("log", "-1", "--format=%s").trim(), "nested");
|
||||
assert.equal(r.revAt("HEAD"), 1);
|
||||
});
|
||||
|
||||
test("F1: a shared-index change during the procedure is not committed", (t) => {
|
||||
|
||||
@@ -20,8 +20,7 @@ function brief(path = "docs/plans/brief-b.md", anchor = "Queue", blob = BLOB) {
|
||||
}
|
||||
|
||||
// A genesis document built the way store.mjs builds one.
|
||||
function genesisDoc(rows = MAP_ROWS, op = "genesis-op-0001") {
|
||||
const map = parseMigrationMap(mapText(rows));
|
||||
function genesisDoc(rows = MAP_ROWS, op = "genesis-op-0001", map = parseMigrationMap(mapText(rows))) {
|
||||
const blobs = new Map(map.rows.filter((r) => r.brief).map((r) => [r.id, BLOB]));
|
||||
const t = at();
|
||||
const grows = genesisRows(map, blobs, op, t, "sage");
|
||||
@@ -507,6 +506,44 @@ test("every accepted text renders to nine cells on every row (N8)", () => {
|
||||
for (const t of ["a\\| done | x", "x<y", "x\\y"]) assert.ok(!accepted.has(t), t);
|
||||
});
|
||||
|
||||
test("genesis: the map refuses an owner among its row's reviewers; replay doesn't (2026-10-04)", () => {
|
||||
const self = MAP_ROWS.map((m) => (m.id === 9 ? { ...m, reviewers: ["darkwing", "filbert"] } : m));
|
||||
refused(() => parseMigrationMap(mapText(self)), /migration map row 9 owner darkwing can't be a listed reviewer/);
|
||||
// A log from before the rule still loads: the check is on the map, not in replay.
|
||||
const old = genesisDoc(self, "genesis-op-0001", { rows: self, retired: [7], highWater: 11 });
|
||||
assert.deepEqual(row(loadDoc(Buffer.from(serialize(old))).doc, 9).reviewers, ["darkwing", "filbert"]);
|
||||
});
|
||||
|
||||
test("times and dates must be calendar values, not just the shape (2026-10-04)", () => {
|
||||
// Row 9 is required, so requiredSince is checked; it allows a date, as createdAt does.
|
||||
const r = row(genesisDoc(), 9);
|
||||
for (const v of ["2028-02-29", "2026-12-31", "2026-04-30"]) {
|
||||
validateRow({ ...r, requiredSince: v });
|
||||
validateRow({ ...r, createdAt: v });
|
||||
}
|
||||
for (const v of ["2026-13-01", "2026-00-10", "2026-01-32", "2026-02-30", "2027-02-29", "2026-04-31"]) {
|
||||
refused(() => validateRow({ ...r, requiredSince: v }), new RegExp(`row 9 requiredSince is not a calendar date: "${v}"`));
|
||||
refused(() => validateRow({ ...r, createdAt: v }), new RegExp(`row 9 createdAt is not a calendar date: "${v}"`));
|
||||
}
|
||||
// ISO times: the same days, and times past the clock. V8 reads 2026-02-30
|
||||
// as March 2 and 24:00 as the next day; only the round trip catches those.
|
||||
for (const v of ["2028-02-29T00:00:00.000Z", "2026-12-31T23:59:59.999Z"]) {
|
||||
validateRow({ ...r, updatedAt: v });
|
||||
validateRow({ ...r, requiredSince: v });
|
||||
}
|
||||
const T = "T12:00:00.000Z";
|
||||
for (const v of [`2026-13-01${T}`, `2026-01-32${T}`, `2026-02-30${T}`, `2027-02-29${T}`, "2026-01-01T24:00:00.000Z", "2026-01-01T23:60:00.000Z", "2026-01-01T23:59:60.000Z"]) {
|
||||
refused(() => validateRow({ ...r, updatedAt: v }), new RegExp(`row 9 updatedAt is not a calendar time: "${v}"`));
|
||||
refused(() => validateRow({ ...r, requiredSince: v }), new RegExp(`row 9 requiredSince is not a calendar time: "${v}"`));
|
||||
}
|
||||
// A shape error keeps its own message.
|
||||
refused(() => validateRow({ ...r, updatedAt: "2026-01-01" }), /row 9 updatedAt must be an ISO time: "2026-01-01"/);
|
||||
// A log entry's time goes through the same check on replay.
|
||||
const d = step(genesisDoc(), "note", { id: 9, text: "x" }, "darkwing");
|
||||
d.log[1].at = `2026-02-30${T}`;
|
||||
refused(() => loadDoc(Buffer.from(serialize(d))), /log entry 1 at is not a calendar time: "2026-02-30T12:00:00.000Z"/);
|
||||
});
|
||||
|
||||
test("replay holds every op id to the caller's rule (N11)", () => {
|
||||
const load = (d) => loadDoc(Buffer.from(serialize(d)));
|
||||
const d = genesisDoc();
|
||||
@@ -549,7 +586,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/);
|
||||
refused(() => step(d, "assign", { id: 9, seat: "filbert" }, "sage"), /can't assign row 9 to filbert: filbert is one of its reviewers/);
|
||||
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$/);
|
||||
|
||||
@@ -63,6 +63,14 @@ test("genesis: the map must be committed, well formed, with committed briefs and
|
||||
no(cli(ghost, genesisArgs(ghost), { by: "sage" }), 2, /row 11 owner ghost is not jason, coordinator, unassigned or a seat/);
|
||||
const noBrief = scratchRepo(t, { map: mapText(MAP_ROWS.map((r) => (r.id === 11 ? { ...r, brief: null } : r))) });
|
||||
no(cli(noBrief, genesisArgs(noBrief), { by: "sage" }), 2, /only a done row may lack one/);
|
||||
// The owner-not-reviewer rule of add, set and assign (2026-10-04).
|
||||
const selfReview = scratchRepo(t, { map: mapText(MAP_ROWS.map((r) => (r.id === 9 ? { ...r, reviewers: ["darkwing", "filbert"] } : r))) });
|
||||
no(cli(selfReview, genesisArgs(selfReview), { by: "sage" }), 2, /migration map row 9 owner darkwing can't be a listed reviewer/);
|
||||
for (const repo of [ghost, noBrief, selfReview]) {
|
||||
assert.throws(() => readFileSync(repo.queuePath));
|
||||
assert.throws(() => readFileSync(join(repo.gitDir, "mosaic-queue.head")));
|
||||
assert.equal(readFileSync(repo.viewPath, "utf8"), queueMd());
|
||||
}
|
||||
});
|
||||
|
||||
test("genesis: markers, a stray witness, once only; a retry returns the receipt", (t) => {
|
||||
@@ -241,7 +249,7 @@ test("add, set reviewers and assign refuse the row's owner as a reviewer", (t) =
|
||||
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/);
|
||||
no(cli(repo, ["assign", "9", "filbert", "--op", "assign-9-filbert"], { by: "sage" }), 2, /can't assign row 9 to filbert: filbert is one of its reviewers/);
|
||||
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" }));
|
||||
|
||||
Reference in New Issue
Block a user