From 6fb50cc0a890c15ccd81e5f604a21597f26d89ca Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Sun, 4 Oct 2026 01:25:06 -0500 Subject: [PATCH] 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 --- agents/filbert/work/queue-33/build.md | 132 +++++++++++++++++++++ agents/filbert/work/queue-33/cold-runs.txt | 21 ++++ agents/filbert/work/queue-33/cold.sh | 31 +++++ docs/plans/DEFERRED.md | 48 +++----- packages/queue/README.md | 21 +++- packages/queue/src/queue.mjs | 15 ++- packages/queue/tests/commit.test.mjs | 59 +++++++-- packages/queue/tests/data.test.mjs | 43 ++++++- packages/queue/tests/store.test.mjs | 10 +- scripts/queue-commit.sh | 19 ++- 10 files changed, 347 insertions(+), 52 deletions(-) create mode 100644 agents/filbert/work/queue-33/build.md create mode 100644 agents/filbert/work/queue-33/cold-runs.txt create mode 100644 agents/filbert/work/queue-33/cold.sh diff --git a/agents/filbert/work/queue-33/build.md b/agents/filbert/work/queue-33/build.md new file mode 100644 index 00000000..e6dd8b87 --- /dev/null +++ b/agents/filbert/work/queue-33/build.md @@ -0,0 +1,132 @@ +# Queue row 33 build notes (#1508) + +Filbert, 2026-10-04. Brief: `docs/plans/2026-10-04_queue-follow-ups.md` +(blob 1dc08bf0…). Reviewer: Darkwing. Base: HEAD 3e39a26a, queue rev 45. +Candidate: `candidate-manifest.sha256` in this directory, uncommitted. +Sage commits it after approval. + +## What changed + +1. **Genesis and the owner-not-reviewer rule.** `parseMigrationMap` refuses + a map row whose owner is among its reviewers: "migration map row N owner + SEAT can't be a listed reviewer", exit 2. Genesis parses the map before + it writes anything, so no file, witness or view changes. +2. **Assign wording.** "can't assign row N to SEAT: SEAT is one of its + reviewers". The two tests that matched the old text now match this one. +3. **HEAD moved.** `queue-commit.sh` gains `head_moved`, called in + `guard_check` before the canary and again when the clean run fails. A + moved HEAD exits 2 with "refused (STEP): HEAD moved since H was + recorded (another commit landed); run queue-commit.sh again". A real + guard failure, with HEAD unmoved, keeps the old message. +4. **Calendar dates.** `checkTime` refuses any ISO time or date that + doesn't format back to itself through `Date.parse` and `toISOString`: + "WHAT is not a calendar time|date: VALUE". A shape error keeps its old + message. Every time the validator reads goes through `checkTime`: row + times, review times and log entry times. +5. **Cold start.** See "Cold runs" below. + +README: the commit steps, the owner rule and the time format. DEFERRED: +four items move to Done, and the cold-start item too, if no failure shows +up. + +## Choices + +- **C1. The genesis check sits in the map parser, not in replay.** The + review-record refusal is kept as defense in depth for a log written + before the rule (`review.test.mjs`). A check in `checkGenesis` would + make such a log unreadable. A new data test pins this: the parser + refuses the map, and a genesis document built past the parser still + loads. Mutant M10 (the rule added to replay) is killed by it. +- **C2. "another commit landed", not "another queue commit landed".** The + brief quotes the second wording. The check before the canary fires for + any commit, including one that touches no queue file, so "queue" would + be wrong there. The rest of the message is the brief's. +- **C3. HEAD is checked twice.** The brief asks for a check before the + canary. A commit can still land between that check and the hook run, and + then the old misdiagnosis comes back. A second check, only when the + clean run fails, closes that gap. It adds one `rev-parse` on a failing + path only. +- **C4. Two existing F1 tests changed.** + - "HEAD moving after commit-tree and before update-ref" moved its + trigger to `before update-ref`. A move after commit-tree is now caught + at the step-7 check (exit 2). The test still proves update-ref's + expected-old-value check, now at the only point where it's the last + line. + - "H is recorded before the canary…": a commit outside the queue files + during the step-1 canary used to reach update-ref (exit 1). It's now + refused at the step-7 check (exit 2), before that check's canary, and + update-ref never runs. The test asserts that. +- **C5. 24:00 is refused.** V8 parses `T24:00:00.000Z` as the next day's + midnight, so it doesn't round-trip. Minute 60 and second 60 already + parse as NaN. + +## Tests + +- `data.test.mjs`: + - a new genesis test (map refusal; replay tolerance); + - a new calendar test: month 13, month 0, day 32, 2026-02-30, + 2027-02-29 and 2026-04-31 as dates in `requiredSince` and `createdAt`; + the same days plus 24:00, minute 60 and second 60 as ISO times in + `updatedAt` and `requiredSince`; 2028-02-29 valid in both forms; the + shape message kept; a log entry `at` refused on replay; + - the assign wording. +- `store.test.mjs`: genesis from a map with row 9's owner among its + reviewers exits 2. For that repository and the two refusals before it, + there's no queue.json, no witness and an unchanged QUEUE.md. Also the + assign wording. +- `commit.test.mjs`, two new tests, both with a nested + `queue-commit.sh` run landing a real queue commit: + - after `symbolic-ref` (H recorded, before step 1's check): refused at + step 1 with the HEAD-moved message, no canary run, and a rerun commits + the next revision; + - before the first `git hook run`: the clean run fails on the moved + HEAD, and the recheck reports HEAD moved, not the guard. + The existing "the canary refuses a hook that git would not run" test + still covers the guard message. + +## Mutants + +Eleven, run on an export of the candidate. Ten are killed: + +| | Mutant | Killed by | +|---|---|---| +| M1 | no HEAD check before the canary | step-1 and step-7 HEAD tests | +| M2 | no recheck after a failed clean run | the between-check-and-canary test | +| M3 | `head_moved` exits 1 | all three HEAD tests | +| M4 | no round trip | calendar test | +| M5 | NaN check only | calendar test | +| M6 | round trip on ISO times only | calendar test | +| M7 | round trip on dates only | calendar test | +| M9 | genesis rule removed | data and store genesis tests | +| M10 | genesis rule also in replay | data genesis test | +| M11 | old assign wording | data and store assign tests | + +One survives: + +- **M8** compares only the date part of an ISO time. It's equivalent + under V8. The only out-of-range time V8 parses is 24:00, and that moves + the date, so the date part catches it. + +## Suites + +On the final candidate bytes, HEAD 3e39a26a: +- canonical checkout: `node --test packages/queue/tests/` passes 148/148 + and `bash scripts/test-queue.sh` 29/0; +- an export of HEAD with the candidate files: `bash scripts/test-queue.sh` + 27/0 (the live checks skip, as designed); +- `queue verify --current`: ok at rev 45. The live log replays under the + calendar check. + +## Cold runs + +Not reproduced in 20 cold runs. `cold.sh` (here) built each tree fresh +from HEAD 3e39a26a plus the candidate files: odd runs as a +`git clone --shared` with push set to DISABLED, even runs as a +`git archive` export. It ran `node --test packages/queue/tests/` once in +each, one run at a time with nothing else running, then deleted the tree. +Every run passed 148/148, 2960 of 2960 in total, taking 39 to 61 s each. +The counts are in `cold-runs.txt`. The 300 ms deadline test is unchanged. + +Run 1 started with a `queue-commit.sh` that differed from the candidate in +one comment line, the step-1 comment, which I reworded during run 1. Runs +2 to 20 used the candidate bytes. diff --git a/agents/filbert/work/queue-33/cold-runs.txt b/agents/filbert/work/queue-33/cold-runs.txt new file mode 100644 index 00000000..111c4292 --- /dev/null +++ b/agents/filbert/work/queue-33/cold-runs.txt @@ -0,0 +1,21 @@ +run 1 clone exit=0 pass=148 fail=0 secs=41 +run 2 export exit=0 pass=148 fail=0 secs=39 +run 3 clone exit=0 pass=148 fail=0 secs=39 +run 4 export exit=0 pass=148 fail=0 secs=48 +run 5 clone exit=0 pass=148 fail=0 secs=59 +run 6 export exit=0 pass=148 fail=0 secs=47 +run 7 clone exit=0 pass=148 fail=0 secs=42 +run 8 export exit=0 pass=148 fail=0 secs=46 +run 9 clone exit=0 pass=148 fail=0 secs=47 +run 10 export exit=0 pass=148 fail=0 secs=42 +run 11 clone exit=0 pass=148 fail=0 secs=45 +run 12 export exit=0 pass=148 fail=0 secs=47 +run 13 clone exit=0 pass=148 fail=0 secs=43 +run 14 export exit=0 pass=148 fail=0 secs=40 +run 15 clone exit=0 pass=148 fail=0 secs=40 +run 16 export exit=0 pass=148 fail=0 secs=42 +run 17 clone exit=0 pass=148 fail=0 secs=39 +run 18 export exit=0 pass=148 fail=0 secs=61 +run 19 clone exit=0 pass=148 fail=0 secs=57 +run 20 export exit=0 pass=148 fail=0 secs=52 +done diff --git a/agents/filbert/work/queue-33/cold.sh b/agents/filbert/work/queue-33/cold.sh new file mode 100644 index 00000000..0ffc53a8 --- /dev/null +++ b/agents/filbert/work/queue-33/cold.sh @@ -0,0 +1,31 @@ +#!/bin/bash +# 20 cold runs of node --test packages/queue/tests/ for row 33 item 5. +set -u +SRC=/mnt/storage/src/mosaic-stack +W=/tmp/fq33 +HEADC=$(cat $W/base.head) +FILES="packages/queue/README.md packages/queue/src/queue.mjs packages/queue/tests/commit.test.mjs packages/queue/tests/data.test.mjs packages/queue/tests/store.test.mjs scripts/queue-commit.sh" +: > $W/cold-results.txt +for i in $(seq 1 20); do + d=$W/cold-$i + rm -rf $d + if [ $((i % 2)) = 1 ]; then + kind=clone + git clone -q --shared --no-checkout $SRC $d && git -C $d remote set-url --push origin DISABLED && git -C $d checkout -q --detach $HEADC + else + kind=export + mkdir -p $d && git -C $SRC archive $HEADC | tar -x -C $d + fi + (cd $W/base && tar -c $FILES) | tar -x -C $d + start=$(date +%s) + (cd $d && env -u NODE_TEST_CONTEXT node --test --test-reporter=tap packages/queue/tests/ > $W/cold-$i.tap 2>&1) + rc=$? + end=$(date +%s) + pass=$(grep -E '^# pass' $W/cold-$i.tap | awk '{print $3}') + fail=$(grep -E '^# fail' $W/cold-$i.tap | awk '{print $3}') + failed=$(grep -E '^not ok' $W/cold-$i.tap | tr '\n' ';') + echo "run $i $kind exit=$rc pass=$pass fail=$fail secs=$((end-start)) $failed" >> $W/cold-results.txt + [ "$fail" = 0 ] && rm -f $W/cold-$i.tap + rm -rf $d +done +echo done >> $W/cold-results.txt diff --git a/docs/plans/DEFERRED.md b/docs/plans/DEFERRED.md index 80aee848..4d158f39 100644 --- a/docs/plans/DEFERRED.md +++ b/docs/plans/DEFERRED.md @@ -138,42 +138,12 @@ at every gate. Started 2026-09-12 during the control board MVP. Sage asked the SetSpark lead to use a one-word role. The rule stays as it is. (2026-09-26, #1506) -- **Queue: `genesis` from a map doesn't check the owner-not-reviewer - rule.** Row 31 added the refusal to `add`, `set reviewers` and `assign`, - but a genesis map can still create a row whose owner is a reviewer. - No current row has that shape. Filbert, row 31 r1 (#1508 comment 26629). - (2026-10-04, #1508) - **Queue: `add --issue N` also sets closes to N.** Follow-up rows under an umbrella issue, like rows 31 and 33 under #1508, would close the issue when they finish. Sage narrowed both by hand (revs 36 and 43). Either `add` takes `--closes` or the lead checks closes on every add. Not urgent while Sage adds the rows. (2026-10-04, #1508) -- **Queue: the `assign` refusal calls the seat "owner" too early.** The - message calls the seat the row's owner before the assign makes it one. - Wording only. Filbert, row 31 r1. (2026-10-04, #1508) -- **Queue tests: one unexplained failure on a cold start.** Darkwing's - first run of the D candidate in a fresh clone gave 141 passed, 1 failed. - The failing test's name wasn't captured. 15 reruns passed 142/0, - sequential and parallel. Darkwing suspects a timing bound, most likely - the 300 ms deadline case in the transport test, which allows 4000 ms. - That isn't confirmed. Next time: capture the name, then fix it as its - own patch. (2026-09-27, #1508) -- **queue-commit.sh blames the guard when HEAD moves.** Step 7's canary - reads H into its index (`scripts/queue-commit.sh:94`). The hook diffs - against the live HEAD. If another queue commit moves the branch after H - is recorded, the clean canary sees a difference and refuses with "git is - not running the queue guard as installed". The fix it prints does - nothing. The refusal is correct, the message is wrong. Fix: guard_check - compares HEAD with H before the canary and says "HEAD moved since H; - start again". Filbert found this during the row 12 live round. - (2026-09-27, #1508) - -- **Queue validator checks date shape, not the calendar.** `ISO_RE` and - `DATE_RE` accept month 13, and V8 turns 2026-02-30 into March 2 without - an error. The CLI can't write such a value today; a hand edit or a - future verb could. Fix: round-trip the parsed date. Filbert and - Darkwing, Piece E round 2. (2026-09-27, #1508) - **Ledger metric call can orphan curl.** The metric call in `packages/ledger/src/ledger.mjs` still relies on execFileSync's timeout, which kills the helper but can leave curl running. E's queue @@ -209,3 +179,21 @@ Moved to `docs/plans/QUEUE.md` on 2026-09-13. This file holds only gaps. now refuse the owner as a reviewer with exit 2 and write nothing. Rocko built it in the Gate G run, and Filbert approved round 1 (#1508 comment 26629, 8 of 8 mutants caught). +- Queue follow-ups from rows 12, 13 and 31 (2026-10-04, #1508): closed by + row 33. Filbert built it, and Darkwing reviews it. + - `genesis` refuses a map row whose owner is one of its reviewers (exit + 2, nothing written). The check is on the map, not in replay, so a log + from before the rule still loads. + - The `assign` refusal now reads "can't assign row N to SEAT: SEAT is + one of its reviewers". + - `queue-commit.sh` compares HEAD with H before each canary and again if + the clean run fails. A moved HEAD exits 2 with "HEAD moved since H was + recorded"; the guard message is kept for a real guard failure. + - Every time and date the validator accepts must format back to itself, + so month 13, day 32, 2026-02-30, 2027-02-29 and 24:00 are refused. + The live log at rev 45 still replays. + - The cold-start failure (2026-09-27): not reproduced in 20 cold runs. + Each ran `node --test packages/queue/tests/` in a fresh tree, 10 + shared clones and 10 exports, one after another. All 20 passed + 148/148 (2960 of 2960). The 300 ms deadline test is unchanged. Counts: + `agents/filbert/work/queue-33/cold-runs.txt`. diff --git a/packages/queue/README.md b/packages/queue/README.md index a209079f..500cabd1 100644 --- a/packages/queue/README.md +++ b/packages/queue/README.md @@ -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 diff --git a/packages/queue/src/queue.mjs b/packages/queue/src/queue.mjs index a2c69bc5..a29626ec 100644 --- a/packages/queue/src/queue.mjs +++ b/packages/queue/src/queue.mjs @@ -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")); diff --git a/packages/queue/tests/commit.test.mjs b/packages/queue/tests/commit.test.mjs index 9a1b6ec1..c254dce5 100644 --- a/packages/queue/tests/commit.test.mjs +++ b/packages/queue/tests/commit.test.mjs @@ -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) => { diff --git a/packages/queue/tests/data.test.mjs b/packages/queue/tests/data.test.mjs index 7ddba5e4..714da63c 100644 --- a/packages/queue/tests/data.test.mjs +++ b/packages/queue/tests/data.test.mjs @@ -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 { + 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$/); diff --git a/packages/queue/tests/store.test.mjs b/packages/queue/tests/store.test.mjs index b69a0e49..c55f9664 100644 --- a/packages/queue/tests/store.test.mjs +++ b/packages/queue/tests/store.test.mjs @@ -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" })); diff --git a/scripts/queue-commit.sh b/scripts/queue-commit.sh index 516e4a9d..6e2e2f0f 100755 --- a/scripts/queue-commit.sh +++ b/scripts/queue-commit.sh @@ -74,6 +74,15 @@ HOOK="$ROOT/.git/hooks/pre-commit" TMPD=$(mktemp -d "${TMPDIR:-/tmp}/queue-commit.XXXXXX") || die 1 "mktemp failed" +# The canary's index holds $1's tree, and the hook diffs it with the live +# HEAD. Once another commit lands, the clean run can fail with the guard +# working as installed, so a moved HEAD is reported as that, not as the guard. +head_moved() { + local now + now=$(g rev-parse --verify -q HEAD) || die 2 "refused ($2): HEAD has no commit" + [ "$now" = "$1" ] || die 2 "refused ($2): HEAD moved since $1 was recorded (another commit landed); run queue-commit.sh again" +} + # --- the queue guard, active and not just present (8.12, G2). $1 is the # commit whose hook blob and tree the checks use. --- guard_check() { @@ -88,11 +97,15 @@ guard_check() { || die 2 "refused ($when): .git/hooks/pre-commit differs from $HOOK_REL at $h" out=$(g config --show-scope --get-all core.hooksPath 2>/dev/null) [ -z "$out" ] || die 2 "refused ($when): core.hooksPath is set ($(printf '%s' "$out" | tr '\n\t' '; ')), so git would not run the queue guard" - # The canary: git itself runs the hook it would run for a commit. + # The canary: git itself runs the hook it would run for a commit. HEAD is + # checked before it and again if the clean run fails, since a commit can + # land between the two. + head_moved "$h" "$when" idx="$TMPD/canary-$when/index" mkdir -p "$(dirname "$idx")" GIT_INDEX_FILE=$idx g read-tree "$h" || die 1 "canary ($when): read-tree failed" if ! out=$(cd "$ROOT" && GIT_INDEX_FILE=$idx git hook run pre-commit 2>&1); then + head_moved "$h" "$when" die 2 "refused ($when): the canary's clean run failed, so git is not running the queue guard as installed: $out" fi GIT_INDEX_FILE=$idx g update-index --add --cacheinfo "100644,$(g rev-parse "$h:$HOOK_REL"),$QMD" \ @@ -133,8 +146,8 @@ if [ "$MODE" = install ]; then exit 0 fi -# --- 1. guard. H is recorded first, before the canary, so a branch that moves -# at any later point makes update-ref in step 7 fail. --- +# --- 1. guard. H is recorded first, before the canary. A branch that moves +# later is refused by a guard check or, after step 7's, by update-ref. --- H=$(g rev-parse --verify -q HEAD) || die 2 "refused: HEAD has no commit" BRANCH=$(g symbolic-ref -q --short HEAD) || die 2 "refused: HEAD is detached" guard_check "$H" step1