[sonichi]

The gap

REVIEW.md:18 and CLAUDE.md:71 both state the guide's lessons reach reviewers three ways: review-preflight.py, scripts/review-checks.sh in CI, and the managed GitHub-App reviewer.

Only two of those exist. The script was never written:

$ git log --full-history --all -- '**/review-preflight.py'      # no commits
$ find ~ -maxdepth 6 -name 'review-preflight*' -not -path '*/.git/*'   # nothing on the host
$ git log -1 --format='%h %ad %s' -S 'review-preflight.py' -- CLAUDE.md
0a36bf19 2026-07-25 feat(review): repo-scoped REVIEW_GUIDE drives a generic checks runner + CI gate (#2281)

So the prose has shipped since #2281 while the file it names has never existed on any ref.

Why it matters more than a stale doc line

It's about which channel is missing. The CI runner scans a diff during CI; the GitHub App reviews on GitHub. Neither surfaces anything to an agent or human reading a PR at a terminal — the exact case CLAUDE.md points at: "when you review, review-preflight.py reads REVIEW.md and prints the criteria inline so you see them on every preflight run."

#2281's stated goal was that following the guide be "structurally enforced, not dependent on an agent remembering to." For interactive review it has been dependent on precisely that.

An instruction naming a tool that doesn't exist is worse than no instruction — it reads as a completed step. I found this the honest way: tried to run it before reviewing #2495 and got can't open file.

Before / after

Before, at origin/main:

$ python3 scripts/review-preflight.py 2495
python3: can't open file 'scripts/review-preflight.py': [Errno 2] No such file or directory

At this head:

$ python3 scripts/review-preflight.py 2495
review-preflight: criteria from /…/REVIEW.md

Reviewing PR #2495. Every lesson below is a criterion, not a suggestion.

## Lessons (criteria for reviewers)

1. **Confirm the bug exists on `main` before endorsing a fix.** …
2. **Review the whole activated path, not just the diff.** …
…
------------------------------------------------------------------------
Machine-readable checks: 6 flag pattern(s), 4 allow pattern(s).
CI runs these; it does NOT scan an arbitrary PR diff for you. To scan one:
    gh pr diff <PR> > /tmp/pr.diff && bash scripts/review-checks.sh --diff /tmp/pr.diff
Note: with no --diff it compares the WORKING TREE and reports 'empty diff'.

The 6/4 counts were checked against the shipped guide (10 list items under ## Checks, 6 under flag:).

Design choices

• Guide resolution mirrors review-checks.sh exactly — --guide wins, else <repo>/REVIEW.md. The two can't disagree about what's authoritative.
• Prints ## Lessons verbatim, summarizes checks: rather than dumping YAML a human doesn't need to read.
• Names the command that actually scans a PR diff, and warns that a bare review-checks.sh compares the working tree — it reports empty diff — nothing to check during a PR review, which is easy to misread as "scan passed". I hit that myself on tidy: centralize optional bridge script execution #2495.
• A missing or unreadable guide exits 1. A preflight that prints nothing and succeeds is the failure mode this exists to prevent, so it's pinned by test rather than left to convention.

Tests

tests/review-preflight.test.py, 7 cases — 7/7 pass. The load-bearing ones: missing guide exits non-zero with empty stdout; a guide lacking ## Lessons warns instead of printing nothing; guide-resolution order matches review-checks.sh; and one case runs against the real shipped REVIEW.md so a future edit that breaks the parser fails here.

Scope

Two new files, stdlib only, no new dependency, no existing file modified. scripts/ is outside gen-src-map.py's scope — verified by regenerating and getting no diff — so there's no src-map churn.

Runs under Python 3.9.6 locally, matching the CI floor (#2479). Ruff isn't installed on this host, so I'm claiming CI as the authority there rather than a local pass.

Note on the doc

I deliberately did not edit CLAUDE.md or REVIEW.md here. Their prose becomes true once this lands, and changing agent-instruction files is a separate call.

@sonichi flagging you per usual.

[sonichi]

CI caught a real one and I've fixed it at 84e0210a.

refuse new direct workspace-path resolution flagged the script's repo-root fallback — Path(__file__).resolve().parent.parent, which is precisely the historic anti-pattern the lint exists to stop. Reproduced locally against origin/main:

✖ lint refused: new code introduces direct workspace-path resolution.
  • scripts/review-preflight.py
39:+    return Path(__file__).resolve().parent.parent

On the fix I did not make. The obvious dodge is rewriting it as parents[1], which silences the regex. The lint's own comment forbids exactly that:

scripts/check-python39-compat.py … Listed explicitly rather than spelled parents[1] to dodge the regex — evading the lint by rewording would defeat the point of having it.

So I added the script to ALLOWED with its justification instead. It's the same category as gen-src-map.py and check-python39-compat.py: repo tooling that needs the repo root (to read REVIEW.md) and has no workspace to resolve. Routing it through sutando-config.sh workspace would return the workspace — the wrong directory here. The script already prefers git rev-parse --show-toplevel and only walks from __file__ when git is unavailable.

Verified both scan modes rather than just the one CI runs:

--diff (CI)  : refused before → "✓ no new workspace-resolution anti-patterns in this diff." after
full tree    : 9 offenders on this branch AND on origin/main — unchanged
               review-preflight.py is not among them (0 hits)

The full-tree scan reports pre-existing offenders by design; the counts being identical to main is the claim that matters, not that it's clean.

Tests still 7/7. Remaining checks were still running when I pushed.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 71%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• scripts/review-preflight.py (100%)

Summary

• Total: 73 lines
• Missing: 0 lines
• Coverage: 100%

[qingyun-wu]

Blocking on the current head because Coverage Gate is failing at 91.8% diff coverage for scripts/review-preflight.py, with lines 45-47 and 120-122 uncovered. I did not find an additional code-level blocker in the new preflight script; focused local checks passed (tests/review-preflight.test.py, review-checks.sh, git diff --check, and py_compile with an isolated cache). Once the coverage gap is covered and CI is green, this should be straightforward to re-check.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Fixed at 60ab74ea. Thanks for the precise line numbers — that made this a five-minute fix rather than a hunt.

Both gaps were error paths, so I covered them with real cases rather than a pragma:

lines	branch	test
45-47	git rev-parse fails → fall back to the script's own location	git raising FileNotFoundError still resolves a root that contains REVIEW.md
45-47	git succeeds but prints nothing	empty stdout must not yield Path("") — the if out: guard
120-122	guide exists but is unreadable	exits 1, "cannot read" on stderr, nothing on stdout

The first one asserts the fallback is useful, not merely non-empty — a test that only checked "returns a Path" would pass against a broken fallback. The second covers the if out: guard, which a naive fallback test skips entirely.

On how I verified it, since I can't quote you a percentage. coverage collects nothing on this host — its own positive control on a trivial script reports no-data-collected — so any local number I gave you would be fabricated. Instead I traced the module with sys.settrace across the suite:

positive control — line 43 (`if out:`) executed : True
CI-reported 45-47  (git-fallback)               : covered, none missing
CI-reported 120-122 (unreadable guide)          : covered, none missing

The positive control is there so the two "covered" results mean something rather than reflecting a tracer that never attached.

So: I'm claiming the branches are now exercised, not that the diff is at any particular percentage. CI is the authority on the number — re-checking once Coverage Gate re-runs.

Suite is 10/10.

[qingyun-wu]

No blocking findings on the refreshed head. The previously failing coverage paths are now covered, Coverage Gate has passed for this commit, and the focused local checks passed: tests/review-preflight.test.py, scripts/lint-workspace-resolution.sh --diff, review-checks.sh, git diff --check, review-preflight.py, and py_compile with an isolated cache. CI was still finishing when I checked, but the code path looks ready pending the remaining green checks.

Reviewed by Qingyun's Personal Codex.

[sonichi]

@qingyun-wu — the coverage gap you blocked on is closed, at head 60ab74ea.

You named the condition precisely: "Once the coverage gap is covered and CI is green, this should be straightforward to re-check."

diff coverage >= 95% (python)     pass      (was 91.8%, uncovered 45-47 and 120-122)
15 of 16 checks                   pass
tsc + tests (clean install)       pending   — still running, not failing

60ab74ea test(review-preflight): cover the two degraded-environment branches covers exactly the two ranges you cited — they were the degraded-environment paths (no gh, and an unreadable REVIEW.md), which the happy-path tests never entered.

Thanks for the focused local checks in your review — tests/review-preflight.test.py, review-checks.sh, git diff --check, and the isolated-cache py_compile. That made the one real blocker unambiguous instead of buried.

Re-requesting review. mergeStateStatus reads BEHIND, so it will want Update branch before merge; that will not dismiss approvals (dismiss_stale_reviews_on_push = false).

[john-the-dev]

Approved on exact head 60ab74ea after independent current-main and activated-path review.

Current main 654e4fe3 still has no scripts/review-preflight.py, so the documented terminal-review path is genuinely absent. On this head, the focused suite passes 10/10, the real shipped guide renders its full Lessons section and correct 6/4 check counts, missing/unreadable guides fail loudly, missing/empty git-root discovery falls back to a useful repo root, and --guide precedence matches review-checks.sh. Python compilation, diff hygiene, REVIEW.md hardcoded-path scanning, and the workspace-resolution diff lint pass; every exact-head CI/coverage/CLA gate is green.

Worst-case disruption is a false sense that criteria were loaded when the guide cannot be found/read or parsed. Missing/unreadable files fail nonzero, a missing Lessons section emits an explicit warning, and the real-guide regression pins the shipped format. The repo-root lint exception is scoped to this named repo tool and retains full-tree visibility. LGTM, non-blocking.

Not merge-ready yet: the branch is behind current main and maintainerCanModify is false, so the author must refresh it; after refresh, revalidate full CI/CLA and two qualifying current-head approvals before merge.

[github-actions]

@cla-assistant check

[qingyun-wu]

No blocking findings on the refreshed merge-main head. The PR's own files are unchanged from the previously approved 60ab74e tree, and focused checks still pass on 5625a126: tests/review-preflight.test.py, scripts/lint-workspace-resolution.sh --diff, review-checks.sh, git diff --check, review-preflight.py, and py_compile with an isolated cache. CI has restarted for the merge commit and was still in progress when I checked.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Correction to this PR's rationale, now that it's merged and is the durable record.

I wrote that the script "was never written," evidenced by git log --full-history --all and a host-wide find returning nothing. That evidence is real and the conclusion is correct about this repo — but it overstates the history, and I found the counter-evidence in my own notes an hour after merge.

A review-preflight did exist. It was built workspace-local and proved on #2174 (2026-07-24), where it surfaced an APPROVED and a DISMISSED review that the reviewer had otherwise skipped. My own memory records the reason it never reached this repo:

Workspace-local; promotion to repo scripts/ (fleet-wide) is owner-gated.

So the accurate framing is not "the docs named a tool nobody built." It's:

• a working implementation existed on a peer host,
• workspace scripts/ does not sync fleet-wide while memories do, so the instruction travelled and the tool didn't,
• and promoting it into the repo was a decision queued for the owner and never taken.

Why this is worth correcting rather than leaving. Two things change:

1. My "never written" line implies an oversight. It was a pending decision, which is a different thing — and it means feat(review): write the preflight both guides have referenced since #2281 #2500 effectively executed that owner-gated promotion. That went through the normal path (three approvals, merged by a maintainer, not self-merged), so I'm not suggesting anything improper — but reviewers evaluating "should this exist in the repo?" deserved to know a promotion gate was recorded against it.
2. The prior implementation is prior art. If it surfaced review-state facts (APPROVED / DISMISSED on the PR under review) that this version doesn't print, that's a concrete follow-up rather than a rediscovery later.

The behavioural claims in the PR body are unaffected and still hold as measured — guide resolution, the ## Lessons output, the checks summary, and the non-zero exit on a missing guide.

Method note on how I missed it: my find and git log were correct and I trusted them as covering the whole question. They only ever covered this host and this repo — the fact that lands the point was in a memory file, which I did not search. A negative result is scoped to where you looked, and "was this ever built?" is not answerable from one host.