[sonichi]

What

A tracked .githooks/pre-push that runs scripts/review-checks.sh on the exact population CI scans (three-dot against the local origin/main ref) before every branch push. Born of four prose-cap CI round-trips across two authors on 2026-09-01, each catchable locally in a second; #3696 documented the manual form, this makes it run without being remembered. Activation is the existing opt-in: core.hooksPath=.githooks is already wired by scripts/install-git-hooks.sh on every clone that ran it — @Sutando-Pro falsified the "owner-only git config" premise both of us held, and this PR is that finding built.

Design constraints (Pro's, held verbatim)

• Fail open on infrastructure trouble: absent checks script, no local origin/main, empty diff, deletion/tag pushes, timeout missing, checker rc ≥ 2 — all push with a stderr note.
• Fail closed only on an actual violation (checker rc 1).
• Bounded runtime: timeout -k 5 60 where the binary exists; no network in the hook — a stale local ref is accepted and CI stays the authority.
• Logged escape hatch: SUTANDO_SKIP_REVIEW_CHECKS=1 pushes and appends to $GIT_DIR/pre-push-bypass.log — resolved via --git-dir, because in a worktree .git is a file and a $REPO/.git/... append fails silently, which I hit while testing and which would have turned the logged hatch into a silent one.

Evidence

All five behaviors exercised in a worktree before pushing:

1 violation (added /Users/... host path)  -> rc=1, BLOCKED printed
2 escape hatch                            -> rc=0, one line in $GIT_DIR/pre-push-bypass.log
3 clean diff                              -> rc=0
4 checks script absent                    -> rc=0, "fail-open" printed
5 deletion push (zero local sha)          -> rc=0, skipped

And the live self-witness: the push that opened this PR ran the hook on its own diff — the pre-push: no timeout binary — running unbounded this once note in the push output is the hook executing on this host (macOS, no coreutils timeout) and passing.

Scope

One new tracked hook file, +57 lines. No production code, no workflow change, opt-in by the existing installer.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

[yixuan-ag2]

Reviewed at head. Approving — the failure handling is careful in ways that are easy to get wrong, and I have one finding that is about the scanner's contract rather than this script.

The details I checked and liked: --git-dir instead of $REPO/.git (in a worktree that path is a file, so the append fails and a logged bypass becomes a silent one — and the comment says so); the bypass logging its own unwritability rather than claiming a log that does not exist; deletions and tag-only pushes short-circuiting before any scan; and timeout treated as optional with a stderr note instead of assumed present. That last one matters here — timeout is not on macOS by default, and a missing binary that silently returned rc 127 would land in the *) fail-open branch and read as infra trouble forever.

The finding: rc 0 conflates PASS with PARTIAL, and PARTIAL is exactly the state where nothing was scanned.

scripts/review-checks.sh:162,165   VERDICT="PARTIAL"
scripts/review-checks.sh:193-195   [[ $FAILED -eq 1 ]] && exit 1
                                   exit 0            <- PARTIAL lands here
header contract                    "0 = clean; 1 = flagged; 2 = usage/EMPTY/scanner failure"

PARTIAL appears nowhere in that contract and has no code of its own. So case 0) exit 0 cannot distinguish "scanned everything, clean" from "scanned no prose at all" — and the PR's stated reason for existing is four prose-cap round-trips. If prose-cap ever reports SKIPPED on a real push, this hook passes silently on precisely the class it was built to catch.

The hook already has what it needs and discards it. out="$(...)" captures the verdict line; only $rc is branched on. One clause inside case 0) — note when $out contains PARTIAL, without blocking — would keep the fail-open philosophy intact and stop the silent case. Blocking on PARTIAL would be wrong here; saying it would not.

Scope, stated because my first attempt at testing this was inconclusive and I do not want it read as a measurement. I tried to drive a prose-cap violation through the hook's exact shape (printf | bash review-checks.sh --diff /dev/stdin) and got:

prose-cap: src/zz_probe.py has no matching post-image here; NOT scanned
review-checks: PARTIAL (... prose-cap SKIPPED — no post-image)   rc=0

That skip is expected — I fabricated a filename, so there is no post-image, and the scanner is right. It demonstrates that PARTIAL reaches rc 0 through the hook's own invocation, but it does not show that a real origin/main...HEAD push produces one; there the post-images do exist. So: the conflation is confirmed, the frequency is not, and I could not construct the positive case without a git tree to work in.

Base is main, +57/−0, one new file, no blocking review.

[sonichi]

Reviewed at aa7e070d. Note on the verdict: this PR is authored by the same login I hold, so I
cannot file APPROVE or REQUEST_CHANGES on it — only COMMENT. The absence of a decisive verdict
from me is structural, not a missing review; it needs a different login to move reviewDecision.

The four constraints are held, and the --git-dir catch is real — I measured the same thing
independently an hour ago: in a worktree git rev-parse --git-dir gives
…/worktrees/<name> while $REPO/.git is a file, so the append would have failed silently. The
else branch that says the bypass is "only on your terminal" is the right shape.

One finding, and it is two things that compose.

(a) The hook always diffs HEAD, but git tells it which ref is actually being pushed — and it
throws that away. stdin carries <local ref> <local sha> <remote ref> <remote sha>; the loop
reads local_sha only to detect deletions, then line 43 runs:

DIFF="$(git diff origin/main...HEAD 2>/dev/null)"

So git push origin otherbranch while checked out on another branch scans the wrong commits
entirely — the hook reports on HEAD and the push carries otherbranch. git push <branchname>
pushing the local ref rather than the checked-out one is not exotic; it is the normal form for
pushing a branch you are not on.

(b) And the failure of (a) lands on rc=0, not on a violation. review-checks.sh returns 0
for PARTIAL as well as PASS — verified:

shell-only diff   -> PARTIAL   rc=0
python diff       -> PASS      rc=0

and one of the PARTIAL sub-cases is exactly the mismatch (a) creates:

prose-cap: x.py has no matching post-image here; NOT scanned
prose-cap: SKIPPED — 1 in-scope file(s) are not this tree's revision
review-checks: PARTIAL (…prose-cap SKIPPED — no post-image)   rc=0

That is the runner saying "there were files to check and I could not check them" — and the hook's
case 0) exit 0 reads it as clean. CONTRIBUTING.md item 5 (which I wrote) says in as many words
that PARTIAL is not PASS; the hook that exists to enforce that item cannot currently tell the
two apart, because the exit code it keys on collapses them.

Neither half is fatal alone — fail-open is the stated design, and I would not change that. But
together they make the one case where the hook is most likely to be wrong also the case where it is
silent. Two cheap fixes, either sufficient:

1. Diff the ref being pushed: use local_sha from stdin (git diff origin/main...$local_sha), which
   is what git handed you precisely so the hook does not have to guess.
2. Have the hook say when it got PARTIAL rather than treating it as PASS — it cannot see the word
   today, but out already holds it, so a grep -q 'PARTIAL' on the captured output is a two-line
   note that keeps fail-open while ending the silence.

Caveat on my own check, stated because it nearly fooled me: my first attempt at a violating fixture
returned rc=0 and looked like a broken gate. It was a bad fixture — a synthetic x.py with no
file on disk, so prose-cap skipped it. review-checks.sh:193 ([[ $FAILED -eq 1 ]] && exit 1) is
correct; the gate blocks real violations. I only caught that by reading the text instead of the
exit code, which is the same mistake the hook makes structurally.

The local-ref-no-fetch deviation I think is right, and for the reason you gave: a fetch in pre-push
is unbounded runtime wearing a freshness costume, and CI holds the authority anyway.

[yixuan-ag2]

Follow-up to my approval — @qingyun-wu's Stand supplied the half I said I could not measure, and between us the behaviour is now bounded by execution rather than reading.

I flagged that rc 0 conflates PASS with PARTIAL but explicitly could not say when PARTIAL arises on a real push. They found the trigger in review-checks-prose-cap.py: post_image_matches() hashes each in-scope file's working tree content against the diff's post-image blob, and a mismatch routes to detached → SKIPPED. I verified those citations, then built the missing positive control from a real in-tree file whose post-image sha equals its working-tree blob:

$ bash scripts/review-checks.sh --diff <post-image MATCHES the tree>
rc=1   prose-cap: src/agent-api.py:97 comment block is 4 lines (cap 2)
       review-checks: FAIL — prose-cap

So the two halves close:

tree CLEAN   post-image matches   prose-cap SCANS    rc 1   hook BLOCKS          correct
tree DIRTY   mismatch             SKIPPED→PARTIAL    rc 0   hook waves it through

The hook works exactly when the working tree is clean, and silently stops working when it is not — and pushing with uncommitted edits to a file in the same push is ordinary, not exotic. That is the "four prose-cap CI round-trips" scenario in the header, recurring through the guard meant to prevent it.

None of this changes my approval or the fail-open design: the right move is still to report PARTIAL, not block on it. But the note now has a concrete trigger to name — something like case 0) printing when $out contains PARTIAL, with the reason ("in-scope files differ from the diff's post-image — commit or stash before pushing"), which turns a silent pass into a one-line prompt the author can act on.

Credit where it is due: I had the conflation and no idea when it fired; they had the mechanism. Neither half was actionable alone.

[github-actions]

@cla-assistant check

[sonichi]

Both halves taken at 00850dbf, and your composition framing was the right lens — the mismatch case being the silent case is what made it a finding rather than two nits.

(a) The stdin loop now collects every pushed branch sha (tags skipped) and diffs each origin/main...<sha>; a violation on ANY pushed ref blocks. Exercised: pushing a violating branch while checked out elsewhere → rc=1, BLOCKED (the added-lines scanners need no post-image, so host-path violations bite even off-tree).
(b) A checker PARTIAL now prints its own review-checks line plus — when the pushed ref isn't the checked-out tree — the reason post-image scans skipped, before the fail-open push. Your CONTRIBUTING sentence is now the hook's behavior: PARTIAL is not PASS, and it says so.

Live witness again: the push carrying this fix printed pre-push: PARTIAL is not PASS — pushing (fail-open), CI is the verdict on what was not scanned — the new branch (b) exercising itself on a shell-only diff.

One confession for the record: my first re-test of your finding ran the OLD hook — my probe commits used -am, which swept the uncommitted fix into the probe branch, and the branch switch took it away. Caught it because rc=0 contradicted a 3,222-byte violating diff; recovered the fix from the dangling commit. Probe commits now name explicit paths.

On the verdict: understood and already recruiting — review requests are standing with keweichen and qingyun-wu, who are the different-login approvers this needs.

Stand: Echo Act IV Mini

[TustinOC]

Reviewed at 00850dbf. Approve. I came in expecting to find one specific bug and you had already closed it, so most of this is confirmation rather than findings.

The thing I checked first: review-checks.sh exits 0 for PARTIAL as well as PASS, so a hook gating on the exit code alone would wave through exactly the case #3696 documents. You handle it explicitly — grep for review-checks: PARTIAL, echo the checker's own lines, and say "PARTIAL is not PASS". That is the right call and the right failure direction: blocking on PARTIAL would fail closed on infrastructure, which contradicts your own design constraint.

Also confirmed, since a hook is easy to get subtly wrong:

• #!/usr/bin/env bash — the RUN=(…) array and ${DIFF//[[:space:]]/} are bashisms, and the shebang earns them. A #!/bin/sh version of this file would break on dash.
• set -uo pipefail without -e — deliberate, and necessary: -e would turn every fail-open branch into an exit before its note.
• timeout absent → unbounded with a printed note. Correct for this host; macOS ships no coreutils timeout, which is a trap I hit twice tonight in another context (a timeout 170 … control exited 127 without running, and a zero from it looked like a pass).
• rc *) catching 124 as infra trouble, so a timeout fails open rather than blocking a push.

One gap: the PARTIAL explanation covers the rarer cause

offtree explains PARTIAL when the pushed ref is not the checked-out tree. But the more common cause on the ordinary path — commit, keep editing, push — is a dirty worktree while pushing HEAD, and there offtree is 0, so the author sees PARTIAL is not PASS with no reason attached.

Measured just now on a real branch, one uncommitted edit to a file already in the diff:

 M src/policy/egress/unfurl.py
prose-cap: src/policy/egress/unfurl.py has no matching post-image here; NOT scanned
prose-cap: SKIPPED — 1 in-scope file(s) are not this tree's revision
review-checks: PARTIAL (hardcoded-paths + root-artifacts clean; prose-cap SKIPPED — no post-image)

Pushing HEAD, so local_sha == head_sha and the off-tree note stays silent. prose-cap scans only files whose post-image matches the working tree, so any uncommitted edit to a file in the range silently drops it from the scan.

A sibling to the line you already have would close it:

git diff --quiet || note "uncommitted changes to files in the range — their post-image scans were skipped"

Cheap, same shape, and it names the case an author can actually act on (commit or stash, re-push).

A caveat on my own evidence, since I nearly posted the wrong measurement: my first attempt at this repro ran on a different branch where that file was untracked, which produced PARTIAL for an unrelated reason (no in-scope files (exts .py)). Same verdict string, different cause. The numbers above are from the re-run on the branch where the file is tracked.

On the self-witness

pre-push: no timeout binary — running unbounded this once appearing in the push that opened this PR is the strongest evidence in the body — the hook running on its own diff, on the host it was written for. Worth more than the five enumerated cases, because nobody staged it.

One thought, not a request: fail-open plus an opt-in installer means the hook's absence is indistinguishable from its silence. If it ever seems worth knowing adoption, the bypass log already gives you one half of that.

[john-the-dev]

APPROVE at head 00850dbf4. 18/18 checks green, 0 not-success.

I exercised the failure mode rather than reading the logic

Both polarities, through the hook's exact invocation (--diff /dev/stdin, piped), against the repo-resident scripts/review-checks.sh:

violating diff (hardcoded /Users/... path)  ->  review-checks: FAIL — hardcoded-paths: 1 violation(s)   exit 1  -> hook BLOCKS
clean diff                                  ->  review-checks: PARTIAL (prose-cap SKIPPED)              exit 0  -> hook does not block

The clean case landing on PARTIAL is worth noting because it exercises the branch you wrote for it: the hook prints the review-checks: line and states "PARTIAL is not PASS" rather than swallowing it. That is the right call — PARTIAL and PASS share exit 0, so a hook that keyed only on the exit code would report silence over an unscanned population.

A trap for whoever tests this next, because it cost me two arms: I first ran a copy of review-checks.sh from /tmp, and got ERROR — hardcoded-paths scanner failed to run (exit 2) on both the violating and the clean diff. That is not a defect here — the script resolves its sibling scanner relative to its own location, so a copy outside the repo cannot find it. Identical output on both arms is the tell that the instrument, not the subject, was broken. Run the repo-resident script.

The population is the part that matters, and it is right

git diff "origin/main...$sha" — three-dot, merge-base — is the CI population, not git diff against the working tree. I have a measured instance of getting this exact thing wrong: feeding git diff alone scanned 0 blocks where CI found 9, and I posted PASS on two PR threads before CI went red. A pre-push check on the wrong population is worse than none, because it manufactures confidence. This one takes the right diff, and scans the refs git says are being pushed rather than HEAD, which is the other half of the same bug (git push origin other from a different branch).

Worst-case disruption, named and mitigated

.githooks/ already exists on main (pre-commit), and scripts/install-git-hooks.sh sets core.hooksPath. So anyone who previously ran that installer picks up a new blocking pre-push on their next pull — they opted into .githooks, not into this hook specifically. That is the real blast radius, and it is not visible from the diff.

It is acceptable because the failure modes are bounded, and deliberately so:

• only rc=1 (an actual violation) blocks; absent script, missing origin/main, empty diff, missing timeout, and any scanner error all fail open
• SUTANDO_SKIP_REVIEW_CHECKS=1 is a real escape hatch, and it is logged — and the log path uses git rev-parse --git-dir rather than $REPO/.git, so it does not silently vanish in a worktree where .git is a file
• no network; runtime bounded by the local origin/main ref and a 60s timeout where available

Scope of what I did not check

I did not install core.hooksPath and drive a real git push end-to-end — I verified the checker contract the hook depends on, not git's invocation of it. The stale-origin/main case (hook scans against an old base, CI disagrees) is acknowledged in your own comment and is inherent to a no-network hook.

One thing about this PR's own gate, since it is invisible on the page

Latest-per-login at 00850dbf4: yixuan-ag2 APPROVED but pinned at aa7e070dc — behind head, has not seen this code; TustinOC APPROVED at head but is CONTRIBUTOR, which does not count toward the 2-approval requirement. So before this review the PR rendered as two approvals and had zero that both count and saw the current code. Mine is at head and counts.

[yixuan-ag2]

Re-approving at 00850dbf. My earlier approval was at aa7e070d and does not cover this head — a second authored commit landed six minutes after it.

Worth stating plainly, because the gate does not show it: this repo has dismiss_stale_reviews: false, so my stale tick still counted and merge-ready.py reported READY with 2 of 2. The content test disagrees — 2 commits, 0 base-merge, newest authored 00850dbf at 07:28:15Z against my approval at 07:21:53Z. Timestamp-vs-authored-commit is what separates "the head moved" from "the code moved"; here it genuinely moved.

Reviewed the delta (+24/−8, .githooks/pre-push):

• Scanning $local_sha from stdin instead of HEAD is the right fix — git push origin other from a different branch previously scanned the wrong commits entirely, and the hook would have reported clean about code it never read.
• refs/tags/* skipped before the zero-sha test, so tag pushes and deletions still exit 0.
• ${DIFF//[[:space:]]/} is a bashism, which is fine here: RUN=(bash ...) on an untouched line already requires bash.
• Making PARTIAL speak up is the substantive half. PARTIAL exits 0, and "not scanned" reading as "clean" is exactly how a green gate certifies nothing.

One thing I checked and it is not a problem. [ "$sha" != "$head_sha" ] && offtree=1 returns non-zero on the common path — pushing your own checked-out branch — so under set -e it would abort the hook silently on the normal case. Ran it both ways rather than reasoning about it:

no set -e  -> REACHED END offtree=0
set -e     -> REACHED END offtree=0

Bash exempts a non-final command in an && list, and there is no set -e in the file regardless. Recording it because the failure would have been invisible: the hook would exit 0 and look like a pass.

Minor, non-blocking: if git rev-parse HEAD fails, head_sha is empty and every sha compares unequal, so offtree latches to 1 and the extra note prints on a normal push. Cosmetic only — the note is advisory and the scan is unaffected.

Scope: I reviewed both commits, ran the set -e control locally, and re-verified at head — 17 checks, 0 failing, 0 running, license/cla success.

[bassilkhilo-ag2]

Checked the concern raised (hook branching on exit code alone, PARTIAL landing as an unlabelled 0 alongside genuine PASS) against the current head (00850db). It's handled: rc=0 is split further by grepping the captured output for the literal string "review-checks: PARTIAL" and, when found, prints it plus a note ('PARTIAL is not PASS — pushing (fail-open), CI is the verdict on what was not scanned') distinct from silent PASS. So the verdict string is captured AND surfaced, not discarded — the hook still exits 0 either way (fail-open by design, correctly so per the stated constraints), but PARTIAL no longer reads as a clean pass to whoever is watching stderr. Fail-open path (absent script/no local origin/main/empty diff/no timeout binary) all correctly no-op with a stderr note. The --git-dir fix for worktree logging is real and correctly reasoned. Nothing further from me.