diff --git a/.github/workflows/review-checklist-numbering-guard.yml b/.github/workflows/review-checklist-numbering-guard.yml new file mode 100644 index 000000000..d8ff2a94f --- /dev/null +++ b/.github/workflows/review-checklist-numbering-guard.yml @@ -0,0 +1,132 @@ +name: Review Checklist Numbering Guard + +# The rules in docs/development/review-checklist.md are numbered, and the +# numbers are cited BY NUMBER from outside the file — the file's own header +# names three (ADR-028 rule 23, ADR-019 rule 9, REVIEW.md §7) and the rules +# cite each other nine more times. A number is a name, and nothing mechanical +# noticed when two PRs claimed one: on 2026-09-25 main gained a `rule 34` +# (#1877) while an open PR added its own `rule 34`. Git caught that one as a +# text conflict only because both appended at the file's end; an insert +# mid-file conflicts less reliably, and a "keep both" merge of two rule 34s +# would have shipped silently — the same shape as the ADR-018 duplicate, where +# an author followed the wrong member of the pair and shipped a wake-policy +# regression (#963). scripts/verify-numbered-rules.js is the check; this +# workflow is when it runs. +# +# Two arms on purpose, the same split as adr-numbering-guard.yml: +# * the PR arm compares the PR against MAIN AS IT IS NOW, not against the +# merge base. A merge base predates whatever landed while the PR was open, +# which is exactly the state the collision occurs in. +# * 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. That arm cannot prevent it; it makes main say so within a minute. + +on: + push: + branches: [ main ] + pull_request: + branches: [ main ] + # `edited` catches base-branch retargeting, same reasoning as + # package-version-guard.yml: a stacked PR retargeted to main after its + # parent merges otherwise enters this population with no event firing. + types: [opened, synchronize, reopened, ready_for_review, edited] + +permissions: + contents: read + +concurrency: + # github.ref, not the PR number: on a push event there is no PR to key on and + # every main build would share one group and cancel its predecessor. + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + review-checklist-main: + name: main's rule numbers are coherent + if: github.event_name == 'push' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + # HEAD^ is the reference for "did a rule change number or name in + # what just merged"; a shallow checkout has no parent to read. + fetch-depth: 2 + + - name: Numbers unique, contiguous, cited, and unchanged + shell: bash + run: | + set -euo pipefail + + args=() + if git rev-parse --verify -q HEAD^ >/dev/null; then + if git cat-file -e "HEAD^:docs/development/review-checklist.md" 2>/dev/null; then + git show "HEAD^:docs/development/review-checklist.md" > /tmp/prev.md + args+=(--previous /tmp/prev.md) + else + echo "· the file did not exist in the previous commit; nothing to compare" + fi + else + echo "· no parent commit on this ref; nothing to compare" + fi + + node scripts/verify-numbered-rules.js --file docs/development/review-checklist.md "${args[@]}" + + review-checklist: + name: Rule numbers are unique, cited, and stable + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + # The PR HEAD, not the merge ref: refs/pull/N/merge is recomputed + # lazily and can be badly stale. A guard reading a stale tree fails a + # PR for a collision somebody already fixed, which teaches authors + # that this check is noise. We read main separately below. + ref: ${{ github.event.pull_request.head.sha }} + # --no-tags and no --depth: a shallow base makes the fork point + # unreachable and the comparison meaningless. Same trap as + # package-version-guard.yml (#1113) and no `|| true` for the same + # reason — a main we cannot read must stop the guard, not pass it. + fetch-depth: 0 + + - name: No rule number is duplicated, skipped, moved, or claimed twice + shell: bash + run: | + set -euo pipefail + + git fetch --no-tags origin main + if ! base=$(git merge-base FETCH_HEAD HEAD); then + echo "::error::No merge base between origin/main and this PR head. Not passing on a baseline this guard cannot see." + exit 1 + fi + echo "· merge base $base; main is $(git rev-parse --short FETCH_HEAD)" + + if ! git cat-file -e "FETCH_HEAD:docs/development/review-checklist.md" 2>/dev/null; then + echo "· main does not carry the file yet; only the in-file checks apply" + node scripts/verify-numbered-rules.js --file docs/development/review-checklist.md + exit 0 + fi + git show "FETCH_HEAD:docs/development/review-checklist.md" > /tmp/main.md + + # The checker itself has to be resolved the same way as the file, and + # for the same reason adr-numbering-guard.yml does it (#1504): on a + # `pull_request` event GitHub takes the WORKFLOW from the merge ref + # but this job checks out the PR HEAD, so every PR branched before + # this guard landed has the workflow file and not the script, and + # `node scripts/...` would die with MODULE_NOT_FOUND — a red PR with + # no rule output at all, which reads as a broken guard. + bin=$(mktemp -d) + if git cat-file -e "HEAD:scripts/verify-numbered-rules.js" 2>/dev/null; then + git show "HEAD:scripts/verify-numbered-rules.js" > "$bin/verify-numbered-rules.js" + else + git show "FETCH_HEAD:scripts/verify-numbered-rules.js" > "$bin/verify-numbered-rules.js" + fi + + # --file is passed explicitly rather than defaulted: the checker is + # loaded from a temp dir, and a default resolved from ITS location + # rather than the checkout is a bug this guard shipped with for one + # run (ENOENT /tmp/docs/development/review-checklist.md, run + # 36137344359). + node "$bin/verify-numbered-rules.js" \ + --file docs/development/review-checklist.md \ + --previous /tmp/main.md diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index 3aa914c9a..3441815be 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -3,7 +3,7 @@ **Status:** Draft for adoption (assembled from milestone #11 *Sharpen*, 2026-07-28/29; assertion-inversion norm pending Sam's adopt-or-strike call) **Audience:** reviewers, mid-PR. Distinct from `REVIEW.md`'s *Self-review checklist* (authors, pre-PR) — two checklists, two moments; retitled here to break the name collision. **Scope:** applies to code review and to spec/document review; each rule names the incident that earned it. Companion to `/REVIEW.md` (the rubric); this file is the incident-derived checklist. -**Numbering:** rules must ascend in document order. Rule numbers are cited by number elsewhere (`ADR-028` rule 23, `ADR-019` rule 9, `REVIEW.md` §7), and markdown numbers ordered-list items by *position*, not by the literal number in the source — so a rule added to the tail of a thematic section renders under that section's next number instead of its own. Add new rules at the end of the file. +**Numbering:** rules must ascend in document order. Rule numbers are cited by number elsewhere (`ADR-028` rule 23, `ADR-019` rule 9, `REVIEW.md` §7), and markdown numbers ordered-list items by *position*, not by the literal number in the source — so a rule added to the tail of a thematic section renders under that section's next number instead of its own. Add new rules at the end of the file. Guarded by `scripts/verify-numbered-rules.js` (`.github/workflows/review-checklist-numbering-guard.yml`), which fails a pull request that duplicates, skips, moves or double-claims a number. **Lands with (pointer edits, same PR — all four now in the diff):** index row in `docs/development/README.md` · companion pointer in `REVIEW.md` §"Before you review" step 6 (cites §7) + the two-checklists note above REVIEW.md's author checklist · mention beside REVIEW.md's required-reading line in `CLAUDE.md` · the CLAUDE.md sentinel-line correction (main read "sent verbatim", wrong since PR #785). ## Tests and assertions diff --git a/package.json b/package.json index 4a826d813..e6b2a7b8c 100644 --- a/package.json +++ b/package.json @@ -17,6 +17,7 @@ "lint:fix:cli": "cd cli && npm run lint:fix", "verify:moltbot-tools": "node scripts/verify-moltbot-tool-contract.js", "verify:litellm-patch-runner": "node scripts/verify-litellm-patch-runner.js", + "verify:numbered-rules": "node scripts/verify-numbered-rules.js", "prepare": "husky" }, "devDependencies": { diff --git a/scripts/verify-numbered-rules.js b/scripts/verify-numbered-rules.js new file mode 100644 index 000000000..ebde182a5 --- /dev/null +++ b/scripts/verify-numbered-rules.js @@ -0,0 +1,256 @@ +#!/usr/bin/env node +/** + * Numbered-rule guard for `docs/development/review-checklist.md`. + * + * The rules in that file are numbered, and the numbers are cited BY NUMBER from + * outside it — the file's own header names three (`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`). A number is therefore a name, and nothing in the + * toolchain noticed when two PRs claimed one: on 2026-09-25 main gained a + * `rule 34` (#1877) while an open PR added its own `rule 34`, which git caught + * as a text conflict only because both appended at the file's end. An insert + * mid-file conflicts less reliably, and a keep-both merge of two rule 34s + * would have shipped silently — the same shape as the ADR-018 duplicate that + * mis-routed a citation into a wake-policy regression. + * + * Checks: + * 1. every rule's literal number is unique + * 2. the numbers are 1..N, ascending, with no gap (the file's header states + * this; a gap is what a dropped rule leaves behind) + * 3. every `rule N` / `rules N–M` citation inside the file resolves to a + * rule that exists, and a range ascends + * 4. with `--previous `, no rule that exists in + * BOTH versions has changed its NUMBER OR ITS LEAD SENTENCE. Two numbers + * changed hands and nothing else says so: + * - MOVED: the rule at N is now at M. Every citation of N — the ones in + * ADR-028, ADR-019 and REVIEW.md, and the nine inside this file — + * silently points at different text. New rules go at the END of the + * file (the header's own instruction), so a move is always a defect. + * - CLAIMED TWICE: main defines rule N and this version defines a + * different rule at N. That is two PRs adding a rule against one + * number, each green on its own — the collision of 2026-09-25 — and + * it is why the reference file is main, not the merge base: a PR's + * merge base predates whatever landed while it was open, which is the + * only state this check exists to see. + * + * Deliberately NOT checked: + * - a rule that exists in `--previous` and NOT in this file. That is the + * ordinary state of a PR opened before another rule landed — on a PR it + * means "predates", not "deleted" — and it is not distinguishable from a + * tail deletion without more history. A rule deleted from the middle is + * caught anyway, by the gap in check 2. + * - an in-place edit of a rule's BODY. Only the bold lead sentence is + * compared, since that is the rule's name. Editing the lead itself is + * reported as a number claimed twice, because a rewritten lead and a + * foreign rule at the same number are the same bytes; keep leads stable. + * - citations from other files (`ADR-028` rule 23 and friends). Resolving + * those needs to know which `rule N` a document means, which is the + * ambiguity this guard exists to keep from growing; they are protected by + * the two checks above, which keep the numbers and the names stable. + * + * Withdrawing a rule, if the day comes: append the withdrawal to the rule's + * BODY and leave its number, its lead sentence and its neighbours alone. That + * is green, the number stays claimed, and a citation of it still resolves — + * to a rule that says it is withdrawn. The two obvious routes both fail, and + * the failures are measured rather than reasoned (cases m9-m11 in the + * campaign): deleting the rule and closing the gap renumbers every rule after + * it (14 errors on a 34-rule file, each one a citation now pointing at + * different text), and marking the lead `Withdrawn` reads as a second rule + * claiming that number (1 error). There is no bypass — deliberately, the same + * as adr-numbering-guard.yml — so the body is where a retraction goes. + */ +const fs = require('fs'); +const path = require('path'); + +const args = process.argv.slice(2); +const argValue = (name, fallback) => { + const i = args.indexOf(name); + return i !== -1 && args[i + 1] ? args[i + 1] : fallback; +}; + +// Resolved from the working directory, not from __dirname: the CI job loads +// this script from a temp dir (so that a PR branched before the guard landed +// still runs the checker from main — see the workflow), and __dirname there +// would point the file lookup at /tmp. The first run of this guard redded on +// exactly that: ENOENT /tmp/docs/development/review-checklist.md. +const ROOT = process.cwd(); +const DEFAULT = path.join('docs', 'development', 'review-checklist.md'); +const FILE = path.resolve(argValue('--file', DEFAULT)); +const PREVIOUS = argValue('--previous', null); + +// A rule begins with its literal number followed by a bold lead. Nothing else +// in the file currently looks like this (35 rules, 35 matches); a bold-lead +// list item nested inside a rule's body would be read as a rule, which is why +// the file keeps its numbered lists un-bolded. +const DEF = /^(\d+)\.\s+\*\*/; +// `rule 5`, `rules 27–28`, `rules 5, 7 and 9` — the separators are what a list +// of citations uses. Ranges accept hyphen, en dash and em dash. +const RANGE = '[\\u2010-\\u2015-]'; +const CITE = new RegExp( + `\\brules?\\s+(\\d+(?:\\s*${RANGE}\\s*\\d+)?(?:\\s*(?:,|and|&)\\s*\\d+(?:\\s*${RANGE}\\s*\\d+)?)*)`, + 'gi' +); + +const fingerprint = (text) => text + .replace(/\*\*/g, '') + .replace(/[`*_]/g, '') + .replace(/\s+/g, ' ') + .trim() + .slice(0, 60); + +/** Parse a rule file into [{number, line, lead, fingerprint}]. */ +function parseRules(body) { + const lines = body.split('\n'); + const rules = []; + let current = null; + lines.forEach((line, i) => { + const m = DEF.exec(line); + if (m) { + current = { number: Number(m[1]), line: i + 1, lines: [line] }; + rules.push(current); + } else if (current) { + current.lines.push(line); + } + }); + for (const r of rules) { + // The bold lead, up to its closing `**`, is the rule's name. + const joined = r.lines.join('\n'); + const lead = /\*\*(.+?)\*\*/s.exec(joined); + r.lead = fingerprint(lead ? lead[1] : joined); + delete r.lines; + } + return rules; +} + +/** Every citation in the file's prose, as {number, text, line} per number. */ +function parseCitations(body) { + const found = []; + const lines = body.split('\n'); + let inFence = false; + lines.forEach((line, i) => { + if (/^\s*(```|~~~)/.test(line)) { inFence = !inFence; return; } + if (inFence) return; + CITE.lastIndex = 0; + let m; + while ((m = CITE.exec(line)) !== null) { + const parts = m[1].split(/\s*(?:,|and|&)\s*/i).filter(Boolean); + for (const part of parts) { + const range = part.split(new RegExp(`\\s*${RANGE}\\s*`)); + if (range.length === 2) { + found.push({ number: Number(range[0]), end: Number(range[1]), text: m[0], line: i + 1 }); + } else { + found.push({ number: Number(range[0]), end: null, text: m[0], line: i + 1 }); + } + } + } + }); + return found; +} + +const errors = []; +// A path outside the repo (CI materialises the base version in a temp dir) is +// printed as given; a relative one is what a reader of the log can click. +const rel = path.relative(ROOT, FILE); +const file = rel.startsWith('..') ? FILE : rel; +const body = fs.readFileSync(FILE, 'utf8'); +const rules = parseRules(body); + +// 1. uniqueness +const byNumber = new Map(); +for (const r of rules) { + if (!byNumber.has(r.number)) byNumber.set(r.number, []); + byNumber.get(r.number).push(r); +} +for (const [n, dupes] of [...byNumber].sort((a, b) => a[0] - b[0])) { + if (dupes.length > 1) { + errors.push( + `rule ${n} is defined ${dupes.length} times (lines ${dupes.map((d) => d.line).join(', ')}). ` + + `A duplicated number makes every citation of "rule ${n}" ambiguous — the two rules merge green ` + + `when they arrive from different PRs. Keep one at ${n}, send the other to the end of the file.` + ); + } +} + +// 2. contiguity and order +rules.forEach((r, i) => { + if (i === 0 && r.number !== 1) { + errors.push(`the first rule is numbered ${r.number}; the file starts at 1.`); + } + if (i > 0) { + const prev = rules[i - 1]; + if (r.number === prev.number) return; // already reported as a duplicate + if (r.number === prev.number + 1) return; + const gap = r.number - prev.number - 1; + const missing = r.number > prev.number + 1 + ? `missing ${gap === 1 ? `${prev.number + 1}` : `${prev.number + 1}..${r.number - 1}`}` + : 'none — this is a descent'; + errors.push( + `line ${r.line}: rule ${r.number} follows rule ${prev.number} — the numbers must ascend 1..N with ` + + `no gap (${missing}). ` + + `A gap is what a deleted or renumbered rule leaves behind, and citations still point into it.` + ); + } +}); + +// 3. citations resolve +const max = rules.length ? Math.max(...rules.map((r) => r.number)) : 0; +for (const c of parseCitations(body)) { + const targets = c.end === null ? [c.number] : [c.number, c.end]; + if (c.end !== null && c.number >= c.end) { + errors.push(`line ${c.line}: "${c.text}" is not an ascending range (${c.number} → ${c.end}).`); + } + for (const t of targets) { + if (!byNumber.has(t)) { + errors.push( + `line ${c.line}: "${c.text}" cites rule ${t}, which does not exist (rules are 1..${max}). ` + + `A citation into a gap resolves to the wrong rule or to nothing.` + ); + } + } +} + +// 4. no rule moved, and no number is claimed by two different rules, against +// the version on main right now +if (PREVIOUS) { + if (!fs.existsSync(PREVIOUS)) { + errors.push(`--previous ${PREVIOUS} does not exist; the stability check cannot run, so this guard will not pass.`); + } else { + const prevRules = parseRules(fs.readFileSync(PREVIOUS, 'utf8')); + for (const p of prevRules) { + const here = byNumber.get(p.number); + // Absent here = this version predates that rule (or dropped a tail rule). + // Not an error: on a PR against a moving main it is the normal state. + if (!here || here.length !== 1) continue; + if (here[0].lead === p.lead) continue; // same number, same name + + const moved = rules.find((r) => r.lead === p.lead); + if (moved) { + errors.push( + `rule ${p.number} ("${p.lead}…") is now rule ${moved.number} (line ${moved.line}). ` + + `Every citation of rule ${p.number} — including the ones in ADR-028, ADR-019 and REVIEW.md — ` + + `now points at different text. New rules go at the END of the file; do not insert mid-file.` + ); + } else { + errors.push( + `rule ${p.number} on main is "${p.lead}…" and this version's rule ${p.number} is ` + + `"${here[0].lead}…" (line ${here[0].line}) — one number, two rules. This is what two PRs each ` + + `adding a rule look like when they pick the same number: each is green alone, and the second ` + + `one to merge silently re-points every citation of rule ${p.number}. Main is at ${prevRules.length} ` + + `rules; renumber this one above that, or append it at the end.` + ); + } + } + } +} + +const cited = parseCitations(body).length; +if (errors.length) { + for (const e of errors) console.error(`::error file=${file}::${e}`); + console.error(`\n${errors.length} numbering problem(s) in ${file} (${rules.length} rules).`); + process.exit(1); +} +console.log( + `✓ ${file}: ${rules.length} rules, numbers 1..${max} ascending with no gap, ` + + `${cited} citation(s) all resolve${PREVIOUS ? ', and no rule changed its number or its name' : ''}.` +);