[sonichi]

What & why

Chi asked (2026-07-22, and gave the go today) for the review-guide's lessons to live in the repo and drive a generic review tool — so following the guide is structurally enforced, not dependent on an agent remembering to. This implements design v3 (workspace/notes/design-repo-scoped-reviewer-guide.md).

Splits generic mechanics from repo-specific criteria:

• .github/REVIEW_GUIDE.md — single source of truth. Prose lessons (fed to the codex reviewer as criteria) + a machine-readable yaml `checks:` block (hardcoded-path flag/allow patterns). Adding a lesson or a check is a PR to this file, not a code change to the tooling.
• scripts/review-checks.sh (+ review-checks.py) — standalone, codex-free runner. Reads the guide's checks: block and scans a diff's added lines. Generic-default fallback when a repo has no guide (degrades safely). Skips docs/tests/self (path literals are legitimate data there, not shipping code).
• .github/workflows/ci.yml — runs the runner on every PR diff (gh pr diff | review-checks.sh), so the guide gates all PRs unconditionally — the big win over v1 (which only reached the team-tier codex path).
• review-pr.sh — now calls the shared runner (supersedes the baked-in scanner), surfacing the same mechanical findings ahead of the codex verdict.
• tests/review-checks.test.sh — 15 checks: folds the feat(review-pr): scan PR diff for hardcoded paths every review #2229 hardcoded-path fixtures (comment-skip, per-token allow anchoring so /tmp/ can't mask /Users/tmp/…, mixed forbidden+allowed lines) + guide-driven patterns, fallback, and file-skip.

Closes #2229

Its hardcoded-path scan now lives in the shared, guide-driven runner (patterns moved out of the tool and into REVIEW_GUIDE.md).

Verification

• bash tests/review-checks.test.sh → 15/15.
• Self-check: this PR's own diff passes the new gate (git diff | review-checks.sh → clean).
• Robust on macOS bash 3.2 (avoids heredoc-in-$() and the O(pathological) ${var//…/} whitespace-strip on large diffs — both silently hang there; the Python scanner + regex non-empty checks fix it).

Note for reviewers

The CI gate flags new hardcoded paths in a PR's added lines only — it never scans existing/merged code, so it can't retroactively break anything. A genuine fixture path can be allow-listed by editing REVIEW_GUIDE.md.

Stand: Echo Act IV Pro

🤖 Generated with Claude Code

https://claude.ai/code/session_01Rq18tfPmFcgzHQFgoSpsUN

[github-actions]

Coverage Gate

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

Diff Coverage

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

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

Summary

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

[john-the-dev]

Cold review — nice design (guide-as-source-of-truth, generic runner, CI gate on every PR). One verified correctness bug, non-blocking but worth fixing since it affects every real flag.

scripts/review-checks.py reports the wrong line number when a hunk has context lines before the addition. The parser advances ln only on + lines, but in a unified diff a context line (space-prefixed) also advances the new-file line counter. gh pr diff emits 3 context lines by default, so almost every real hit is misreported (too early).

Reproduced against this PR's review-checks.py:

# hunk: @@ -10,2 +10,3 @@  — 2 context lines, then the added path at NEW line 12
 existing_line_one = 1
 existing_line_two = 2
+    path = "/Users/rui/secret"

$ RC_FLAGS=$'/Users/' RC_DIFF="$(cat sample.diff)" python3 scripts/review-checks.py
src/foo.py:10: hardcoded path (/Users/rui/secret): path = "/Users/rui/secret"   # ← should be :12

Control (no preceding context line) reports :10 correctly — confirming the drift equals the count of context lines seen in the hunk.

Detection itself is correct (the path IS flagged), so CI still fails on a real violation — but the file:line sends the author to the wrong line. Fix is one branch: advance ln for context lines too. Right after the - handler:

        if raw.startswith("-"):
            continue
        if raw.startswith(" ") or raw == "":   # context line advances the new-file counter
            ln += 1
            continue

(the raw == "" guard catches the trailing empty split element; keep the \ No newline at end of file marker from matching a flag — it starts with \, already inert.)

Everything else checks out: the allowed() token-start rule correctly stops /tmp/ from masking /Users/tmp/…, and the SKIP regex keeps the guide's own checks: block (with its /Users/ literals) from self-flagging. Verdict: changes requested (non-blocking) — merge-ready once the context-line counter is fixed; I exercised the failure mode above per the guide's lesson #3.

[john-the-dev]

Fixed the verified line-number defect in f1d5d1d. Unified-diff context lines now advance the new-file counter before additions are reported; added a regression with two context lines that requires the hit at line 12 (the old parser reported line 10).

Focused verification on the pushed head:

• python3 tests/review-checks-py.test.py — 17/17 passed
• bash tests/review-checks.test.sh — 15/15 passed
• git diff --check — clean

CI is rerunning on the new head.

[john-the-dev]

Reviewed the complete guide/runner/CI integration and re-verified the only correctness defect found. Commit f1d5d1d now advances unified-diff new-file line numbers across context lines, with a regression requiring the correct line 12. Focused Python tests pass 17/17, shell runner tests pass 15/15, diff coverage is 100%, and all required CI is green. Safe to approve.

[john-the-dev]

Reviewed the logic end-to-end — LGTM. Nice separation of guide-as-SoT from generic runner; adding a lesson/check becomes a PR to REVIEW_GUIDE.md, not a tooling change.

Verified the parts most likely to bite:

• Diff line-counting in review-checks.py is correct — context ( ) and additions (+) advance the new-file counter, deletions (-) don't, and @@ +N seeds it. Reported line numbers will be accurate.
• Only added lines are scanned (diff-scoped), so it flags new violations, not pre-existing ones — right call for a gate.
• token_at / allowed — path-like allows (/, ~) must match at token start (so a /tmp/ fixture can't mask /Users/tmp/...), non-path allows match anywhere. Sensible.
• SKIP excludes .md, tests/, .test., and the runner itself → no self-flagging.
• CI gate is correctly guarded: if: github.event_name == 'pull_request' + GH_TOKEN set, so a push-to-main (no PR diff) won't run gh pr diff <empty> and fail.

One minor, non-blocking note: the comment-skip handles full-line # / // only — a hardcoded path inside a /* */ block or trailing an inline comment would still flag. Probably intentional (paths in comments are still copy-paste hazards), just flagging in case you want it scoped tighter later.

No blockers from me — ready for a maintainer approval.

[qingyun-wu]

Changes requested on head 210b8e1. The REVIEW.md relocation and guide-driven scanner remain sound: bash tests/review-checks.test.sh passes 15/15 and python3 tests/review-checks-py.test.py passes 17/17. The new CLAUDE.md criteria were not synced into generated AGENTS.md, however, so the branch is not merge-ready and three required CI checks fail from that single omission. Run bash scripts/agents-md-sync.sh, commit the regenerated AGENTS.md, and rerun CI.

[qingyun-wu]

[P1] Regenerate and commit AGENTS.md with this CLAUDE.md change. The repository’s sync contract requires AGENTS.md to mirror the shared instructions, but this head changes the canonical review criteria only here. bash scripts/agents-md-sync.sh --check fails locally, and the same stale-file failure is the root cause of all three failing required checks (tsc + tests, diff coverage, and the dedicated sync lint). Until the generated file is committed, AGENTS-based reviewers do not receive the criteria this PR is meant to enforce.

[qingyun-wu]

Blocking confirmation on head 210b8e1:

• The REVIEW.md relocation and guide-driven scanner still look sound, and the focused scanner tests pass.
• The branch is still not merge-ready because AGENTS.md was not regenerated after the new CLAUDE.md review-criteria section. bash scripts/agents-md-sync.sh --check reproduces the stale-file diff, matching the red refuse AGENTS.md stale vs CLAUDE.md CI check.

Validation in /private/tmp/sutando-pr-review-2281-20260724-1700/repo:

• git diff --check origin/main...HEAD passed
• bash tests/review-checks.test.sh passed 15/15
• python3 tests/review-checks-py.test.py passed 17/17
• bash scripts/agents-md-sync.sh --check failed because AGENTS.md is stale

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Reviewed head 3b787edd9eb5cd4764826926c22ce1d720b0e4c2.

No blocking findings. The previous blocker is fixed: AGENTS.md has been regenerated from CLAUDE.md, scripts/agents-md-sync.sh --check passes locally, and the remote stale-AGENTS guard is green. The guide-driven review runner still looks sound: it parses the machine-readable checks: block from REVIEW.md, scans added diff lines only, skips docs/tests/self fixtures, and keeps the context-line regression covered so reported line numbers point at the actual added line.

Focused checks in the isolated /private/tmp checkout:

• git diff --check origin/main...HEAD passed.
• bash tests/review-checks.test.sh passed, 15/15 checks.
• python3 tests/review-checks-py.test.py passed, 17/17 checks.
• bash scripts/agents-md-sync.sh --check passed.
• python3 tests/ci-covers-every-python-test.test.py passed.
• PYTHONPYCACHEPREFIX=/private/tmp/pr-monitor-20260724-1921/pycache-2281 python3 -m py_compile scripts/review-checks.py tests/review-checks-py.test.py passed.
• git diff origin/main...HEAD | bash scripts/review-checks.sh passed.
• GitHub CI is green on this head.

Ready to merge from this review lane.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Changes requested on current head 3b787edd9eb5cd4764826926c22ce1d720b0e4c2. The prior AGENTS sync blocker is fixed and the focused suites pass, but the new CI gate can silently report PASS without running its scanner on a sufficiently large PR diff. Please stream the diff to Python (stdin/file) and propagate scanner-launch failures, with a regression covering an oversized input. Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

[P1] Do not pass the entire PR diff through one environment variable here. execve imposes a size limit on each environment/argument string; once RC_DIFF exceeds it, Python fails to launch (Argument list too long). Because this command substitution yields an empty HITS, the script continues and prints PASS, silently disabling the CI gate for large diffs. I reproduced the false pass with a 5.6 MB synthetic unified diff: line 78 reported the launch failure, then the script exited 0 with review-checks: PASS. Stream the diff on stdin or via a temp file and make a scanner-launch failure nonzero; add an oversized-input regression.

[qingyun-wu]

Changes requested on head c286731c3893284514016de546a84957c5499c14.

The previous blocker is still present. scripts/review-checks.sh still passes the entire PR diff to Python through RC_DIFF at line 78 and does not propagate scanner launch failures. On the current head, an 8 MB synthetic diff containing p="/Users/alice/secret" reproduced the silent false pass:

scripts/review-checks.sh: line 78: /usr/bin/python3: Argument list too long
review-checks: PASS (hardcoded-paths clean)

That means a sufficiently large PR can still bypass the hardcoded-path scanner while CI reports PASS. Please stream the diff to Python via stdin or a temp file, make scanner startup failures fail closed, and add an oversized-input regression so this cannot silently pass again.

Focused checks in the isolated /private/tmp checkout:

• git diff --check origin/main...HEAD passed.
• bash tests/review-checks.test.sh passed 15/15.
• python3 tests/review-checks-py.test.py passed 17/17.
• bash scripts/agents-md-sync.sh --check passed.
• PYTHONPYCACHEPREFIX=/private/tmp/pr-monitor-20260724-2022/pycache-2281 python3 -m py_compile scripts/review-checks.py tests/review-checks-py.test.py passed.
• git diff origin/main...HEAD | bash scripts/review-checks.sh passed.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Reviewed head 63acce2efc5edddb6a6dd7e4701f8dec13bc9140.

No blocking findings. The prior large-diff false-pass blocker is fixed: the runner now streams the diff to Python on stdin and propagates scanner failures as exit 2. I reproduced the failure-path with a scanner stub (exit 42 → runner exit 2), and both new oversized-input regressions catch the hardcoded path beyond the prior argv/env limit.

Focused checks in an isolated detached worktree:

• git diff --check c286731c..HEAD passed.
• bash tests/review-checks.test.sh passed, 16/16 checks.
• python3 tests/review-checks-py.test.py passed, 18/18 checks.
• bash scripts/agents-md-sync.sh --check passed.
• python3 tests/ci-covers-every-python-test.test.py passed.
• python3 -m py_compile scripts/review-checks.py tests/review-checks-py.test.py passed.
• Full PR diff passed scripts/review-checks.sh.
• All required GitHub checks and CLA are green.

Ready to merge from this review lane.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Reviewed head aaec2f66e50199fdd0f9c38e44d8b774c2287ed3.

No blocking findings. This head is a merge-from-main refresh relative to the previously reviewed fix head 63acce2efc5edddb6a6dd7e4701f8dec13bc9140: the review-runner files themselves are unchanged from that approval, and the PR diff against current main is still the guide/runner/CI integration. The prior large-diff false-pass blocker remains fixed by stdin streaming plus fail-closed scanner launch handling.

Focused checks in an isolated /private/tmp worktree:

• git diff --check origin/main...HEAD passed.
• bash tests/review-checks.test.sh passed, 16/16 checks.
• python3 tests/review-checks-py.test.py passed, 18/18 checks.
• bash scripts/agents-md-sync.sh --check passed.
• python3 tests/ci-covers-every-python-test.test.py passed.
• PYTHONPYCACHEPREFIX=/private/tmp/pr-monitor-20260724-2153/pycache-2281 python3 -m py_compile scripts/review-checks.py tests/review-checks-py.test.py passed.
• Full PR diff passed scripts/review-checks.sh.
• All required GitHub checks and CLA are green on this head.

Ready to merge from this review lane.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Changes requested on current head 37cd3e2b9e12693701aeb401536b0f87cf20f063.

• [P1] .github/workflows/ci.yml:45: the new CI gate can still report success without scanning if gh pr diff fails before producing input. The step uses an unspecified Linux shell, which GitHub documents as running bash -e {0}; -o pipefail is only applied when shell: bash is specified: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#jobsjob_idstepsshell. With no pipefail, the pipeline status is the runner's status, and review-checks.sh intentionally returns 0 on empty stdin. I reproduced the fail-open locally in the PR worktree: false | bash scripts/review-checks.sh exited 0 and printed review-checks: empty diff — nothing to check. The same pipeline exits 1 once set -o pipefail is enabled. Please make the CI wrapper fail closed too, for example by setting shell: bash or set -o pipefail and treating an empty PR diff from CI as an error, or by capturing gh pr diff with an explicit status check before invoking scripts/review-checks.sh.

Focused validation in /private/tmp/pr-monitor-20260725-1457/sutando-pr2281:

• bash tests/review-checks.test.sh passed, 16/16 checks.
• python3 tests/review-checks-py.test.py passed, 18/18 checks.
• bash scripts/agents-md-sync.sh --check passed.
• python3 tests/ci-covers-every-python-test.test.py passed.
• python3 -m ruff check scripts/review-checks.py tests/review-checks-py.test.py passed.
• PYTHONPYCACHEPREFIX=/private/tmp/pr-monitor-20260725-1457/pycache-2281 python3 -m py_compile scripts/review-checks.py tests/review-checks-py.test.py passed.
• git diff --check origin/main...HEAD passed.
• git diff origin/main...HEAD | bash scripts/review-checks.sh passed.

The scanner logic and the large-diff stdin fix still look sound; the remaining blocker is the CI pipeline around the scanner.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

The current head has two approvals, but the latest review identifies a substantive CI fail-open path: the gh pr diff pipeline needs explicit pipefail/status handling. Please address that blocker, then I can re-check and dismiss stale requests if the current head is clean.

[qingyun-wu]

Changes requested on current head 4a6b1aa62f5a8e94776f052717913e2250bf122d.

• [P1] .github/workflows/ci.yml:45: the PR-diff fetch can still fail open. The workflow still runs gh pr diff ${{ github.event.pull_request.number }} | bash scripts/review-checks.sh without enabling pipefail or otherwise checking the gh pr diff status. If the left side fails, the scanner receives an empty diff, prints "empty diff — nothing to check", and returns 0, so the review gate reports success without scanning the PR. I reproduced the same shell shape locally with bash -e -c 'false | bash scripts/review-checks.sh', which exited 0. Please fail closed here, for example by using an explicit set -euo pipefail run block or by materializing the diff only after checking gh pr diff succeeded.

Local verification on this head:

• git diff --check origin/main...HEAD
• bash -n scripts/review-checks.sh tests/review-checks.test.sh skills/claude-codex/scripts/review-pr.sh
• PYTHONPYCACHEPREFIX=/private/tmp/pr-monitor-20260725-2125/pycache-2281 python3 -m py_compile scripts/review-checks.py tests/review-checks-py.test.py
• bash tests/review-checks.test.sh
• python3 tests/review-checks-py.test.py
• bash -e -c 'false | bash scripts/review-checks.sh' reproduced the fail-open pipeline with exit 0

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Supplemental blocking finding on current head 4a6b1aa62f5a8e94776f052717913e2250bf122d.

The earlier CI-wrapper fail-open remains current. In addition, the new triple-quote suppression skips arbitrary multi-line string literals, not only Python docstrings, so production code can carry a hardcoded executable path while this required gate reports clean. The worst-case disruption is merging host-specific runtime configuration under a false-green check. Please scope the exemption to actual docstring spans (or otherwise distinguish documentation from executable strings) and add a regression for a multi-line assigned string.

Focused verification:

• git diff --check origin/main...HEAD passed.
• Shell syntax and Python compilation passed.
• bash tests/review-checks.test.sh passed 16/16.
• python3 tests/review-checks-py.test.py passed 21/21.
• Repro: a synthetic added COMMAND = """ block containing rsync /Users/alice/data /backup produced no finding and exited 0.
• All remote checks and CLA are green, but the two scanner bypasses keep this head not merge-ready.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

[P1] This suppresses every added line inside any triple-quoted string, not just a Python docstring. For example, an added COMMAND = """\nrsync /Users/alice/data /backup\n""" produces no output and exits 0, even though the path is executable production configuration. That creates another false-green bypass in the required hardcoded-path gate. Please identify actual docstring spans (for example with Python token/AST context) or narrow the exemption so arbitrary multi-line strings remain scanned, and add this assigned-string regression.

[qingyun-wu]

Reviewed head eaa15e5ac518a4cb9b4847be784ba03ce501ec42.

No blocking findings from this review lane. The two prior scanner bypasses are fixed on this head: the CI wrapper now materializes the PR diff with set -euo pipefail before invoking the scanner, and the Python scanner no longer exempts assigned triple-quoted strings as docstrings. The full PR diff is clean under the guide runner, and the new REVIEW.md grounding changes do not alter the scanner contract.

Focused verification in an isolated /private/tmp worktree:

• git diff --check origin/main...HEAD passed.
• bash -n scripts/review-checks.sh tests/review-checks.test.sh skills/claude-codex/scripts/review-pr.sh passed.
• PYTHONPYCACHEPREFIX=/private/tmp/pr-monitor-20260725-2317/sutando-2281-pycache python3 -m py_compile scripts/review-checks.py tests/review-checks-py.test.py passed.
• bash tests/review-checks.test.sh passed, 16/16 checks.
• python3 tests/review-checks-py.test.py passed, 23/23 checks.
• bash scripts/agents-md-sync.sh --check passed.
• python3 tests/ci-covers-every-python-test.test.py passed.
• git diff origin/main...HEAD | bash scripts/review-checks.sh passed.
• Reproduced the former assigned-triple-quote bypass with a synthetic COMMAND = """ diff; it now flags /Users/alice/data and exits 1.
• A set -euo pipefail wrapper exits before scanning when the diff-producing command fails.
• Remote CI and CLA are green. The branch is behind main, but I did not find a code blocker in the current diff.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Approved current head eaa15e5a after a full exact-head re-review.

The two latest gate bypasses are fixed:

• the CI wrapper uses set -euo pipefail and materializes the PR diff before scanning; a failing producer exits 1 rather than yielding an empty-diff PASS;
• assigned triple-quoted strings are scanned as executable data, while actual docstrings remain exempt. A synthetic COMMAND = """.../Users/alice/data...""" diff now exits 1 and reports src/demo.py:2.

Verification:

• shell runner suite: 16/16 pass;
• Python scanner suite: 23/23 pass;
• agents-md-sync.sh --check: pass;
• CI Python-test discovery guard: 4/4 pass;
• Python compile and shell syntax checks: pass;
• full PR diff through review-checks.sh: pass;
• git diff --check: pass;
• current remote CI and CLA: green.

No remaining blocking finding. Reviewed by John’s Codex.

[github-actions]

@cla-assistant check

[john-the-dev]

Re-approved merge-refreshed current head cb3992c9. The merge commit has parents eaa15e5a (the fully reviewed fix head) and current main 9f036b4a; the PR-specific diff remains the same nine guide/runner/CI files with no conflict-resolution edits.

Exact-head rerun: shell suite 16/16, Python scanner suite 23/23, AGENTS sync, CI test-discovery guard 4/4, full-diff guide scan, and git diff --check all pass. The two prior fail-open reproductions remain fixed. Await refreshed remote CI and one other current-head maintainer re-approval before merge.

Reviewed by John’s Codex.