guard: the reviewer checklist's rule numbers are unique, contiguous and stable

[lilyshen0722]

Why

docs/development/review-checklist.md numbers its 34 rules, and those numbers are names that other documents cite. The file's own header names three external citations (ADR-028 rule 23, ADR-019 rule 9, REVIEW.md §7), and the rules cite each other nine more times (rule 5, rule 7, rule 9, rule 12 twice, rule 14, rule 16, rule 23, rules 27–28). Nothing mechanical checked any of it.

That cost a real collision today. #1877 landed a rule 34 on main while an open PR added its own rule 34. Git happened to catch that one as a text conflict, because both appended at the end of the file. An insert mid-file conflicts less reliably, and a "keep both" merge of two rule 34s would have shipped with every check green — the ADR-018 shape, where a duplicate number survived long enough that a PR author followed the wrong member of the pair and shipped a wake-policy regression (#963). adr-numbering-guard.yml exists for that; this is the same guard one file over.

What it checks

scripts/verify-numbered-rules.js (also npm run verify:numbered-rules):

1. Unique — every literal rule number appears once.
2. Contiguous — 1..N ascending, no gap. A gap is what a deleted or renumbered rule leaves behind, and citations still point into it.
3. Citations resolve — every rule N / rules N–M inside the file points at a rule that exists, and a range ascends. Lists (rules 5, 7 and 9) and en/em dashes are handled; fenced code is skipped.
4. Stable — against a reference version, no rule has changed its number or its lead sentence. This is the check that protects the outside citations, since every one of them is a bare number.

The one design decision worth reviewing

The PR arm's reference is main as it is right now, not the merge base. adr-numbering-guard.yml goes to some trouble to build the post-merge tree from current main, and for the same reason: a merge base predates whatever landed while the PR was open, and that window is exactly where this collision lives. Case m7 below is that state, and it only reds with main as the reference.

The push arm re-checks main afterwards, because two PRs that each insert at a different place can both be green and still collide once both have merged. Like the ADR guard, that arm cannot prevent it — it makes main say so within a minute.

What it deliberately does not check, stated in the script header because a guard that overstates itself is worse than none:

• a rule present in the reference and absent here. On a PR that is the normal state of predating a rule someone else merged, and it is not distinguishable from a tail deletion without more history. A mid-file deletion is caught anyway by the contiguity check.
• an edit to a rule's body. Only the bold lead is compared, because that is the rule's name.
• citations from other files. Resolving those needs to know which rule N an ADR means, which is the ambiguity this guard exists to keep from growing; they are protected by keeping the numbers and the names stable instead.

Where a retraction goes, since there is no bypass and the first person who needs one will try the two routes that fail: append the withdrawal to the rule's body and leave its number and lead alone (m9, green; citations of that number still resolve, to a rule that says it is withdrawn). Editing the lead to say so reads as a replacement (m10), and deleting the rule renumbers everything after it (m11). That paragraph is in the script's header, not just here.

Evidence

12-case mutation campaign (/tmp/rulecheck_campaign.py), each mutation asserted to apply exactly once — a mutation that never applied would read as green — with a byte-identical restore and a green baseline after:

case	mutation	result
baseline	none	✓ green, 34 rules, 10 citations
m1	rule 30 renumbered to 29 in place	red, 2 errors (duplicate + gap "missing 30")
m2	rule 20 deleted	red, 1 error (gap "missing 20")
m3	new rule inserted at 20, tail renumbered 20→21…34	green without --previous — this is the blind spot
m3b	same tree, --previous = main	red, 15 errors (every moved rule named with its new number: 20..34) — the count is corpus-dependent, 15 because this branch's file has 34 rules and 16 once #1884's rule 35 is in main's file
m4	a citation retargeted to rule 99	red, 1 error (cites a rule that does not exist)
m5	rules 27–28 reversed to 28–27	red, 1 error (not an ascending range)
m6	a valid new citation added (rule 34)	✓ green — positive control that citations are not blanket-flagged
m7	a foreign rule claims main's number 34	red, 1 error (one number, two rules) — today's collision
m8	a PR that merely predates main's newest rules	✓ green — no false positive on the ordinary stale-branch state
m9	a withdrawal appended to a rule's body, lead intact	✓ green — the route a retraction should take
m10	the same withdrawal marked by editing the rule's lead	red, 1 error — reported as a number claimed twice
m11	the rule deleted and the gap closed	red, 14 errors — every rule after it moved

The workflow YAML was parsed rather than eyeballed; there is no actionlint in this repo. The PR arm's script-resolution fallback (read the checker from main if the PR head predates it) is copied from adr-numbering-guard.yml, where #1504 earned it: without it, every PR branched before the guard landed reds with MODULE_NOT_FOUND and no rule output, which reads as a broken guard.

The arrival check — a new gate has to not red on day one, and this one was measured against the two trees it will actually see (sprint-review, 74018, against main after #1884 merged):

tree	result
the PR branch	✓ green, 34 rules
main itself	✓ green, 35 rules, 1..35, 10 citations resolve
the simulated merge result (git merge-tree)	✓ green, 35 rules, clean

The branch-vs-main staleness difference is m8 above, now the live state rather than a synthetic one. The PR's only edit to review-checklist.md is the one header line pointing at the guard; it touches no rule.

The guard's own first run was red, which is why --file is explicit

Run 36137344359: ENOENT: no such file or directory, open '/tmp/docs/development/review-checklist.md'. The PR arm loads the checker from a temp dir (so a PR branched before the guard landed still runs main's copy), and the script resolved its default path from __dirname — /tmp/... So the job died before it looked at a single rule number.

Fixed in 6c19fb77: the default now resolves from process.cwd(), and the workflow passes --file explicitly so the path cannot depend on where the script itself was loaded from. adr-numbering-guard.yml's --dir exists for the same reason; this keeps the trick but stops paying for it twice.

The current run is green: · merge base a7dea33a; main is a7dea33a then ✓ 34 rules, numbers 1..34 ascending with no gap, 10 citations all resolve, and no rule changed its number or its name.

Not verified

• Not run against a fork where the file is absent from main (the cat-file branch is exercised only by reading); the fallback prints that it is skipping the reference comparison rather than passing silently.
• The paths: filter is intentionally absent, so this runs on every PR. It is a ~2 s job and a path filter would skip exactly the case of the script itself changing.

[lilyshen0722]

Arrival check against a moved base, since main advanced twice under this PR and one of those commits edits the file it guards.

#1916 (main 055e1cff) appended rules 38–40 to docs/development/review-checklist.md — the same file this PR touches. Two things measured on the merged content, not assumed:

• No text conflict. git merge-tree --write-tree --name-only origin/main origin/guard/review-checklist-numbering → tree efcd7890, no conflicted paths. The branch edits the header's Numbering: line; docs(checklist): three rules on what a reading actually measures #1916 appended at EOF.
• The guard passes on the merged file, and is not vacuous there. Reconstructed the merge (main's file + this PR's one header line) and ran the checker: 40 rules, numbers 1..40 ascending with no gap, 17 citation(s) all resolve, and no rule changed its number or its name — exit 0. The same run without --previous also exits 0.
• Controls on that same content. Renumbering rule 40 → 41 reds by name (rule 41 follows rule 39 … missing 40); a citation to a non-existent rule 99 reds (rules are 1..40). So the green is a green check, not a checker that cannot see.

PR-arm check as CI will run it (branch file, --previous = main as of now): 34 rules, numbers 1..34 … no rule changed its number or its name, exit 0. Rules 35–40 exist only on main, which the script's documented exclusion covers ("a rule present in the reference and absent here … on a PR that is the normal state of predating a rule someone else merged").

So the push arm will be green on merge, and nothing here needs a re-gate from #1916.

[lilyshen0722]

Head moved ecf25687 → adc70a4e (merge commit — no rebase, no force-push). No review was bound to the old head (gh pr view 1885 --json reviews = 0), so nothing was voided.

Why. This PR crossed the stale-base ceiling while displaying a green guard. At 13:23Z it was 38 behind; at 14:47Z it was exactly 40 (the test is -gt, so 40 passes and 41 fails); main's cb20121e (#1923) took it to 41. The check on the PR still read success, because pr-base-freshness.yml is a pull_request workflow — its status attaches to the head sha and it does not re-run as main advances.

The part worth recording, because it is the reason this was fixed rather than left pressable. Merging does not require an up-to-date branch, and a merge does not trigger the guard, so a PR can cross the ceiling and merge on a green that was measured at a smaller distance — the gate never evaluates the distance it exists to bound. That is the same shape as #1876, which was 45 behind on a frozen green before the press. A frozen green is not a passing gate; here the instrument is the distance, not the check:

git fetch origin main && git rev-list --count <head>..origin/main

Fix, and it is verified rather than hoped. Merged origin/main in; behind is now 0. The guard re-ran on the new head at 2026-09-26T15:24:50Z and passed. The numbering guard itself (Rule numbers are unique, cited, and stable) also reports SUCCESS alongside it. Locally on the merged tree:

node scripts/verify-numbered-rules.js --file docs/development/review-checklist.md
✓ 40 rules, numbers 1..40 ascending with no gap, 17 citation(s) all resolve.

Not verified: the rest of CI on adc70a4e (Test & Coverage, kind cluster smoke test, one Analyze leg) was still in flight when this was written. No red at the time of writing.

[lilyshen0722]

Re-stamp requested at adc70a4e — this PASS does not carry, and I measured why rather than taking it as a formality.

The branch's own commits, oldest → newest: 03765b87 → 6c19fb77 → ecf25687 → adc70a4e (merge main). The PASS is on the first of those; two commits landed after it, both touching the surfaces it gated:

6c19fb77  .github/workflows/review-checklist-numbering-guard.yml | 13 ++++++---
          scripts/verify-numbered-rules.js                       | 10 ++++++--
ecf25687  scripts/verify-numbered-rules.js                       | 11 +++++++++++

PR's OWN diff vs its merge base:
  at 03765b87 (base a7dea33a)  workflow +125   script +239   4 files, 366 insertions
  at adc70a4e (base cb20121e)  workflow +132   script +256   4 files, 390 insertions

The per-file counts differ, so the reviewed artifact is not the artifact now on the head — a re-read rather than a carry. Nothing is wrong with the earlier verdict; it is simply bound to a sha.

adc70a4e is 31 behind main and the guard re-ran green on this head at 15:24:50Z. No GitHub review is bound to any head here (gh pr view 1885 --json reviews = 0), so the previous PASS was a pod-level one and moving the head voided nothing on GitHub.

[lilyshen0722]

PASS at adc70a4e — recorded here because it arrived by private consult and gh pr view 1885 --json reviews is empty, so the presser cannot see it on the PR. Quoting the gate; the head is unchanged by this comment.

PASS at adc70a4e. … My latest PASS was not 03765b87 — it was ecf25687. I re-tested at 6c19fb77 when the __dirname fix landed (BASE plus three cases, since the script itself had moved) and then confirmed the carry to ecf25687 after reading all 11 added lines to check they were comment-only. … From my actual last PASS, the carry is cleaner than you describe: verify-numbered-rules.js, the workflow and package.json are all byte-identical to ecf25687. The merge brought only main in.

But one input did move, and it's the one that matters for a guard: review-checklist.md changed — main gained rules while this sat open. The script is unchanged; what it reads is not. A guard's verdict is a function of both, so I re-established it rather than carrying it: the head's own file (40 rules, 1..40, 17 citations resolve); the PR arm against main-as-now (green); main itself against the guard about to land (green — so it still doesn't red on arrival); discriminating cases re-run on the new corpus (duplicate reds, gap reds, stale-branch reference stays green), 0 dirty. Worth noting the rule count went 34 → 40 since the original gate, so the "no day-one breakage" result genuinely needed redoing — six rules landed that the earlier check never saw.

It's CLEAN but 31 behind; at the 40 ceiling that's nine more main advances before the head has to move and this PASS is void again.

Correction to my comment above. I wrote that the PASS was at 03765b87 and that a re-read was owed because two commits landed after it. That sha came from the presser's record, not from the gate — it was two heads stale, and both of the re-reads I inferred as owed had already happened (6c19fb77 for the __dirname fix, then the carry to ecf25687).

What I measured was true and about the wrong object. I verified the branch's history — which commits exist, and that the PR's own diff moved between the two shas I was handed — but never asked the gate which sha it was bound to. Ask the gate for its sha; do not take the sha from the person requesting the re-gate, because the requester is the one surface that cannot see the gate's own record. Same class as the head citation on #1876 earlier today, where b0477697 was an ancestor rather than the head.

The input that actually moved, which neither I nor the presser named. A guard's verdict is a function of the script and the corpus it reads. The script never changed after ecf25687; docs/development/review-checklist.md did — main appended rules 35–40 while this sat open. So a merge-in that touches none of your files can still change what a guard reads, and for a checker whose whole job is "does main survive this on arrival", that is the only re-measurement that mattered.