From ad14ae6f9e809fe194326359c0cbb987e0fc46c3 Mon Sep 17 00:00:00 2001 From: XVVH Date: Wed, 15 Jul 2026 09:21:55 -0400 Subject: [PATCH] Codify the review battery: scripts/ci hygiene lane + doctrine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Founding case: the PR #50 SI-32 candidate, where a self-administered battery collapsed into one subagent let an external review find 5 gaps it should have killed (a code-fact claim contradicting source, three filing-named deliverables dropped, a phantom appendix, a wrong-base branch). Fix, per the repo's own G9 remedy: make the battery a required artifact plus a mechanical lane, not a thing the author remembers. - scripts/check-doc-hygiene: dependency-free POSIX shell, three repo-state checks (doc/ADR ref resolution; ordered-list numbering; named-"..."-appendix section existence). Two-sided via --self-test (checks fire on broken fixtures, pass on good + a false-positive guard). roadmap.md exempt from file-existence refs (forward-looking). - scripts/ci: new `hygiene` lane, wired into run_test so it runs in contracts/test/required/full/all and in GitHub Actions' `test` step. - docs/review-battery.md: the doctrine — the three tiers, the structural rule (enumeration -> tables, adversary -> findings, never bundled), the PR-context checks, and the running order. - CLAUDE.md / AGENTS.md: binding paragraph next to G9. Mutation testing N/A (shell enforcement, not Rust — the two-sided contract discipline's stated escape hatch); the --self-test is the two-sided evidence. No Rust touched. Co-Authored-By: Claude Fable 5 --- AGENTS.md | 17 ++++ CLAUDE.md | 17 ++++ docs/review-battery.md | 111 +++++++++++++++++++++++ scripts/check-doc-hygiene | 185 ++++++++++++++++++++++++++++++++++++++ scripts/ci | 15 +++- 5 files changed, 344 insertions(+), 1 deletion(-) create mode 100644 docs/review-battery.md create mode 100755 scripts/check-doc-hygiene diff --git a/AGENTS.md b/AGENTS.md index 0f21001..5211d90 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -106,6 +106,23 @@ posture row); a finding with no citable clause is a filing (SI/RF/G/P), not a blocker. Un-parking a posture row or a parked roadmap item is an operator gate decision, never a review outcome. +Review battery (founded by the PR #50 SI-32 candidate, whose external +review found 5 gaps a self-administered battery should have killed): +authority-surface and spec-ratification changes run the codified battery +in `docs/review-battery.md` BEFORE independent review. Non-negotiable +shape: **enumeration tasks produce row-per-item tables (completeness +against the filing's named deliverables; every `as-built`/`file:line` +claim verified at source), adversarial tasks produce findings, and the +two never bundle into one pass** — a missing table row is a visible +miss; prose hides omissions. Tier 1 (`scripts/ci hygiene`: doc/ADR ref +resolution, ordered-list numbering, named-section existence; plus the +PR-context declared-scope and branch-off-`main` checks) is mechanical +and runs in `required`. Tier 2 (the two tables) ships in the PR body +like the G9 sweep. Tier 3 is the internal adversarial pre-review for +candidates, scoped to soundness/shape, never asked to also be the +exhaustive checker. A review round that finds completeness or code-fact +debris means a Tier-1/2 pass was skipped or run as prose. + ## Session-end git contract Work exists once it is on `origin`, not before. Push your branch before diff --git a/CLAUDE.md b/CLAUDE.md index ffc46fc..201d9e7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -106,6 +106,23 @@ posture row); a finding with no citable clause is a filing (SI/RF/G/P), not a blocker. Un-parking a posture row or a parked roadmap item is an operator gate decision, never a review outcome. +Review battery (founded by the PR #50 SI-32 candidate, whose external +review found 5 gaps a self-administered battery should have killed): +authority-surface and spec-ratification changes run the codified battery +in `docs/review-battery.md` BEFORE independent review. Non-negotiable +shape: **enumeration tasks produce row-per-item tables (completeness +against the filing's named deliverables; every `as-built`/`file:line` +claim verified at source), adversarial tasks produce findings, and the +two never bundle into one pass** — a missing table row is a visible +miss; prose hides omissions. Tier 1 (`scripts/ci hygiene`: doc/ADR ref +resolution, ordered-list numbering, named-section existence; plus the +PR-context declared-scope and branch-off-`main` checks) is mechanical +and runs in `required`. Tier 2 (the two tables) ships in the PR body +like the G9 sweep. Tier 3 is the internal adversarial pre-review for +candidates, scoped to soundness/shape, never asked to also be the +exhaustive checker. A review round that finds completeness or code-fact +debris means a Tier-1/2 pass was skipped or run as prose. + ## Session-end git contract Work exists once it is on `origin`, not before. Push your branch before diff --git a/docs/review-battery.md b/docs/review-battery.md new file mode 100644 index 0000000..25b76ee --- /dev/null +++ b/docs/review-battery.md @@ -0,0 +1,111 @@ +# The review battery — codified pre-review discipline + +Authority-surface and spec-ratification changes in this repo go through +independent adversarial review (the CLAUDE.md rule). This document codifies the +**author-side battery** that runs *before* a change reaches that review, so the +review spends its cost on genuine soundness questions rather than on gaps a +checklist would have caught. + +**Why this exists (founding case).** PR #50 (the SI-32 candidate) was drafted +after a self-administered "audit battery" plus one internal adversarial +subagent. An external review then found 8 issues, 5 of them the battery should +have killed: a code-fact claim that contradicted source (`capture_sqlite` +follows symlinks), three deliverables the originating filing *named* but the +draft dropped or hand-waved (directory rules, G-PUBLISH, active-window edits), a +referenced appendix that did not exist, and a branch based on the wrong parent. +Root cause: the battery lived in the author's head, was **collapsed into one +subagent** carrying five lenses at once, had **no artifact-hygiene tier**, and +handed **checklist tasks off as adversarial prose** — so enumeration got sampled +instead of enumerated. This is the same failure G9 fixed for test coverage +("green lanes still missed three PR #33 findings"), and the fix is the same: +make the battery a **required artifact plus a mechanical lane**, not a thing the +author remembers. + +## The one structural rule + +**Enumeration tasks produce tables; adversarial tasks produce findings; never +bundle them.** A completeness or code-fact pass must emit a row-per-item table, +because a missing or `OPEN` row is a *visible* miss a reviewer or a script can +catch — prose hides omissions, which is exactly how named deliverables get +sampled out. The adversarial pass stays free-form and is reserved for what it is +good at: soundness, tier-completeness, over/under-deciding. Enumeration and +adversary run as **separate passes**, not five bullets in one prompt. + +## When the battery applies + +Any PR that touches the spec, an ADR, the SI/RF/P/G ledgers as load-bearing +records, or any authority surface. Trivial docs edits and code-only changes +already covered by the two-sided contract discipline do not need Tier 2/3, but +Tier 1 runs on every push regardless (it is in `scripts/ci required`). + +## Tier 1 — mechanical (enforced, cannot be skipped) + +**Repo-state checks — `scripts/ci hygiene`** (`scripts/check-doc-hygiene`), wired +into `run_test` so it runs in `contracts`/`test`/`required`/`full`/`all` and in +GitHub Actions. Dependency-free POSIX shell; two-sided by construction +(`--self-test` proves each check fires on a broken fixture and passes on a good +one — the negative side; the real docs tree is the positive side). Mutation +testing does not apply (shell enforcement, not Rust — the two-sided contract +discipline's stated escape hatch). It checks: + +1. **doc-refs** — every referenced `docs/.md` and `ADR 0NNN` resolves to a + file that exists. `roadmap.md` is exempt from *file-existence* refs only + (by its own boundary rule it names planned and unmerged-branch artifacts); + ADR-number refs are enforced everywhere. +2. **list-number** — within a section, a top-level ordered list runs 1,2,3,…; a + reset to `1.` starts a new list; a duplicate or a skip fails. +3. **self-appendix** — a doc that names an "appendix" must contain an appendix + heading (a referenced-but-absent inline section). + +**PR-context checks — author + reviewer step** (need the PR as source of truth, +so they are required steps, not part of the local lane): + +- **Declared scope matches the diff.** The PR body lists the files it means to + touch; `git diff main...HEAD --stat` must match. A stray file (e.g. an + unrelated ADR carried in because the branch was cut from the wrong parent) is + a finding. The reviewer checks this first. +- **Branch based on current `main`.** `git merge-base --is-ancestor origin/main + HEAD` after `git fetch`; no commits from another open PR ride along. Cut the + branch from `main`, not from a sibling feature branch. +- **PR numbers, not SHAs, in docs** (the session-end git contract, applied to + citations). + +## Tier 2 — required PR artifacts (reviewer checks they are complete) + +Every applicable PR carries these two tables in its body, the same way the G9 +conformance sweep is already required. Their value is the **blank/OPEN row**: an +unaddressed item is visible instead of silently absent. + +- **Completeness table** — one row per deliverable the originating filing + (SI-n / RF-n / the roadmap item) named → the ADR/spec line that closes it, or + an explicit `OPEN` / deferral with an owner. Extract the deliverables from the + filing *first*, before drafting, so the draft is written against the list. +- **Code-fact table** — one row per "as-built" / `file:line` claim the artifact + makes → confirmed-at-source or refuted, with the anchor. Every claim gets a + row; a claim with no verified anchor does not ship as fact. + +## Tier 3 — adversarial passes + +- **Internal adversarial pre-review** (ratification candidates, before any paid + external round): a fresh-context subagent briefed on the accumulated + failure-mode profile, scoped to soundness / tier-completeness / shape + (over/under-deciding) — *not* asked to also be the exhaustive checker; Tiers + 1–2 own that. Its findings are applied and recorded before the artifact is + pushed. +- **Independent-context review** (the existing CLAUDE.md authority-surface rule): + the external round. The battery's job is to make this round return only + genuine soundness seams, not completeness or code-fact debris. + +## Running order + +1. Extract the filing's deliverables into the completeness table (drives + drafting). +2. Draft. +3. Fill the code-fact table by verifying each claim at source. +4. `scripts/ci hygiene` (and the PR-context checks against the intended diff). +5. Internal adversarial pre-review; apply findings. +6. Open the PR with both tables; independent-context review. + +A round that still finds completeness or code-fact gaps means a Tier-1/2 pass +was skipped or run as prose — that is a process miss to name, not just a finding +to fix. diff --git a/scripts/check-doc-hygiene b/scripts/check-doc-hygiene new file mode 100755 index 0000000..bdc4cd4 --- /dev/null +++ b/scripts/check-doc-hygiene @@ -0,0 +1,185 @@ +#!/bin/sh +# Repo-state hygiene for docs and ADRs — Tier 1 of the review battery +# (docs/review-battery.md). Dependency-free POSIX shell, in the spirit of +# check-test-contracts: it runs in the ordinary local gate, needs no network +# and no cargo, and the repository — not a hosting provider — defines what +# "hygienic" means. +# +# Deterministic, low-false-positive checks over docs/*.md and top-level *.md: +# 1. doc-refs — every referenced `docs/.md` and `ADR 0NNN` resolves +# to a file that exists (catches a phantom appendix/ADR ref). +# 2. list-number — within a section, a top-level ordered list runs 1,2,3,…; a +# reset to `1.` starts a new list; a duplicate or a skip is an +# error (catches the double-"2." class). +# 3. self-appendix — a doc that names an "appendix" must contain an appendix +# heading (catches a referenced-but-absent inline section). +# +# Two-sided evidence: `--self-test` builds a passing fixture and a set of +# deliberately-broken fixtures in a tempdir and asserts each check fires on the +# break and passes on the good copy — the negative side of the lane; the +# real-tree run is the positive side. Mutation testing does not apply: this is +# shell enforcement logic, not Rust (docs/review-battery.md states this). + +set -eu + +SELF=$(CDPATH= cd -- "$(dirname -- "$0")/.." && pwd) + +fail() { + printf 'doc-hygiene error: %s\n' "$1" >&2 + exit 1 +} + +# List markdown under a root: the docs/ tree plus top-level *.md. +list_md() { + root=$1 + [ -d "$root/docs" ] && find "$root/docs" -name '*.md' -type f + for f in "$root"/*.md; do + [ -f "$f" ] && printf '%s\n' "$f" + done +} + +# Append every hygiene violation under $root to the file $out. Returns nothing; +# emptiness of $out is the verdict, so results survive the pipeline subshells. +run_checks() { + root=$1 + out=$2 + + list_md "$root" | while IFS= read -r f; do + rel=${f#"$root"/} + + # --- Check 1: referenced doc files and ADRs resolve --------------- + # roadmap.md is exempt from file-existence refs only: by its own + # boundary rule it orders *future* work and names planned or + # unmerged-branch artifacts (e.g. a doc recovered on an unmerged + # branch). Every other doc describes current state and is enforced. + # ADR-number refs are enforced everywhere, roadmap included. + case "$rel" in + docs/roadmap.md) ;; + *) + grep -noE 'docs/[A-Za-z0-9._/-]+\.md' "$f" 2>/dev/null \ + | while IFS=: read -r ln ref; do + [ -f "$root/$ref" ] || printf '%s:%s references missing file %s\n' \ + "$rel" "$ln" "$ref" >> "$out" + done + ;; + esac + grep -noE 'ADR 0[0-9][0-9][0-9]' "$f" 2>/dev/null \ + | while IFS=: read -r ln ref; do + num=${ref#ADR } + hit=$(find "$root/docs/adr" -name "$num-*.md" -type f 2>/dev/null | head -n1) + [ -n "$hit" ] || printf '%s:%s references missing %s\n' \ + "$rel" "$ln" "$ref" >> "$out" + done + + # --- Check 2: ordered-list numbering per section ----------------- + awk -v rel="$rel" ' + /^```/ { fence = !fence; next } + fence { next } + /^#/ { prev = 0; next } + /^[0-9]+\. / { + n = $1 + 0 + if (n == 1) { prev = 1; next } + if (prev == 0) { prev = n; next } + if (n == prev + 1) { prev = n; next } + printf "%s:%d: ordered-list number %d follows %d (expected %d or a reset to 1)\n", \ + rel, NR, n, prev, prev + 1 + } + ' "$f" >> "$out" + + # --- Check 3: a named "..." appendix has an appendix heading ----- + # High-precision: fires only on a *named* appendix reference — a + # double-quoted name immediately followed by "appendix" (the finding-7 + # shape) — not on prose that merely uses the word (e.g. a doc + # describing this very check). Requires a matching appendix heading. + if grep -vE '^#' "$f" | grep -qE '"[^"]{2,}"[[:space:]]+[Aa]ppendix'; then + grep -qiE '^#+ .*appendix' "$f" \ + || printf '%s: references a named "..." appendix but has no appendix heading\n' "$rel" >> "$out" + fi + done +} + +report() { + root=$1 + out=$(mktemp "${TMPDIR:-/tmp}/asf-dochyg.XXXXXX") + trap 'rm -f "$out"' EXIT HUP INT TERM + run_checks "$root" "$out" + if [ -s "$out" ]; then + printf 'doc-hygiene: violations found\n\n' >&2 + sort "$out" >&2 + printf '\n' >&2 + return 1 + fi + printf 'doc-hygiene OK: docs and ADRs under %s pass repo-state checks\n' "${root#"$SELF"/}" + return 0 +} + +self_test() { + dir=$(mktemp -d "${TMPDIR:-/tmp}/asf-dochyg-fx.XXXXXX") + trap 'rm -rf "$dir"' EXIT HUP INT TERM + mkdir -p "$dir/docs/adr" + + # Passing fixture: valid ref, contiguous list, appendix heading present. + cat > "$dir/docs/adr/0001-good.md" <<'EOF' +# Good +See docs/other.md and ADR 0001. +1. one +2. two +3. three +## Rejected +1. a +2. b +## The inventory appendix +rows here +EOF + cat > "$dir/docs/other.md" <<'EOF' +# Other +No issues here. +EOF + out=$(mktemp "${TMPDIR:-/tmp}/asf-dochyg-st.XXXXXX") + run_checks "$dir" "$out" + [ -s "$out" ] && { printf 'self-test FAIL: good fixture flagged:\n'; cat "$out"; rm -f "$out"; return 1; } + + # Break 1: dangling doc + ADR reference. + printf 'ref docs/missing.md and ADR 0099\n' >> "$dir/docs/adr/0001-good.md" + : > "$out"; run_checks "$dir" "$out" + grep -q 'missing file docs/missing.md' "$out" || { printf 'self-test FAIL: dangling doc ref not caught\n'; rm -f "$out"; return 1; } + grep -q 'missing ADR 0099' "$out" || { printf 'self-test FAIL: dangling ADR ref not caught\n'; rm -f "$out"; return 1; } + + # Break 2: duplicate ordered-list number. + cat > "$dir/docs/dup.md" <<'EOF' +# Dup +1. one +2. two +2. also two +4. four +EOF + : > "$out"; run_checks "$dir" "$out" + grep -q 'dup.md:4: ordered-list number 2 follows 2' "$out" || { printf 'self-test FAIL: duplicate list number not caught\n'; rm -f "$out"; return 1; } + + # Break 3: appendix named but no appendix heading. + cat > "$dir/docs/noapp.md" <<'EOF' +# Inventory reference with no section +The "Publication site inventory" appendix lists every site. +## Body +text +EOF + : > "$out"; run_checks "$dir" "$out" + grep -q 'noapp.md: references a named' "$out" || { printf 'self-test FAIL: missing appendix heading not caught\n'; rm -f "$out"; return 1; } + # And a doc that merely discusses the word "appendix" must NOT fire. + cat > "$dir/docs/meta.md" <<'EOF' +# Meta +This check flags a doc that names an "appendix" with no heading. +EOF + : > "$out"; run_checks "$dir" "$out" + grep -q 'meta.md:' "$out" && { printf 'self-test FAIL: meta-discussion false-positive\n'; rm -f "$out"; return 1; } + + rm -f "$out" + printf 'doc-hygiene self-test OK: all three checks fire on breaks and pass on the good fixture\n' + return 0 +} + +case "${1:-}" in + --self-test) self_test ;; + "") report "$SELF" ;; + *) report "$1" ;; +esac diff --git a/scripts/ci b/scripts/ci index 36bd0da..29890d8 100755 --- a/scripts/ci +++ b/scripts/ci @@ -38,8 +38,17 @@ run_contracts() { sh scripts/check-test-contracts } +run_hygiene() { + heading "doc & ADR repo-state hygiene (review battery, Tier 1)" + # Self-test first (the negative side: the checks fire on broken fixtures), + # then the real docs tree (the positive side). See docs/review-battery.md. + sh scripts/check-doc-hygiene --self-test + sh scripts/check-doc-hygiene +} + run_test() { run_contracts + run_hygiene heading "workspace tests (all current and future targets)" cargo test --workspace --all-targets --locked @@ -211,7 +220,8 @@ usage: scripts/ci lanes: static strict Clippy over the workspace and all targets contracts validate two-sided evidence and reject unclassified new tests - test contracts + all workspace targets + both acceptance demos + hygiene doc & ADR repo-state checks (review battery, Tier 1) + test contracts + hygiene + all workspace targets + both acceptance demos required static + test; deterministic/offline and suitable for pre-push audit RustSec advisory audit (requires cargo-audit and network/cache) full required + audit @@ -228,6 +238,9 @@ case "${1:-required}" in contracts) run_contracts ;; + hygiene) + run_hygiene + ;; test) run_test ;;