fix(#1086): enforce protected branches from git's pre-push ref list - #1090

Code Review: PR #1090

Commit: 541e6b4660435c6970f00929887c7794b5068d5c

Summary

Step 2 of #1086. .githooks/pre-push now decides "block this push?" from git's own stdin ref lines rather than from shell command text, adds _lib-protected-branches.sh as the shared config resolver, and ships an 11-case / 30-assertion suite driven by a real git binary against a real bare remote. block-main-push.sh is genuinely untouched.

I verified the central property independently rather than reading the author's evidence: the branch is derived only from $REMOTE_REF, and the single grep in the file matches that branch against the static config regex. There is no $COMMAND, no tool_input, no command-string scan of any kind.

Checklist Results

• Architecture & Design: Pass
• Code Quality: Pass
• Testing: Pass
• Security: Pass
• Performance: Pass (n/a)
• PR Description & Glossary: Pass
• Summary Bullet Narrative: Pass
• Technical Decisions (AgDR): N/A — implements AgDR-0104, cites AgDR-0113; the AgDR-0104 status update is scoped to step 3 by the issue
• Adopter Handbooks: Pass — general/commit-message-quality (advisory) satisfied; architecture/migration-safety is blocking but out of scope (no DB migration in this diff); architecture/clean-architecture-layers n/a
• Fallow Static Analysis: N/A (not JS/TS)

Independent verification

Loop exit semantics — confirmed correct, and it is load-bearing. Isolated the two shapes:

procsub:  exit 1 inside loop -> script rc=1, "REACHED-END" never printed
pipe:     exit 1 inside loop -> script rc=0, "REACHED-END" printed

The shipped file uses done < <(printf '%s\n' "$STDIN_REFS"), so the loop body runs in the current shell and exit 1 terminates the hook. Confirmed live against the shipped file by feeding it synthetic ref lines: a single refs/heads/main line gives rc=1 and the runner never starts — if the loop were a subshell the script would have fallen through to exec bash "$RUNNER" and returned the runner's 0. A two-line push where the protected ref is the second line also blocks, proving the loop scans all lines and still exits the script.

exit 1 vs exit 2 — correct. Git blocks on any non-zero from pre-push; there is no exit-2 convention at the git layer. The repo's exit 2 idiom belongs to Claude Code PreToolUse hooks, a different contract. The existing fail-open paths in this same file already use exit 0, so exit 1 for "blocked" is internally consistent.

Discriminating before/after — verified myself, not taken on report. Reconstructed the pre-change 35-line hook from the merge base (33b4915) into a parallel root and ran the same test file against it:

Every #1066 / #1083 / #1084 / #1085 shape, plus bare push on main, explicit push origin main, and push origin --delete main, is ALLOWED before and BLOCKED after. That is genuine discrimination, not green-in-both-directions.

Over-narrowing — clean, including cases the suite doesn't cover. The four controls (tag push, feature-branch push, two-remote tag push, delete non-protected) pass in both directions. I added four of my own by feeding ref lines directly:

• maintenance, devops, main-backup → allowed (regex is anchored ^(...)$; no substring over-match)
• HEAD:refs/heads/main and HEAD:main refspec renames → blocked. Worth calling out: this shape is not in the author's suite, and it is exactly the kind of thing a text parser has to special-case. Here it falls out for free because git resolves the refspec before the hook sees it. That is the design paying off.
• empty stdin → allowed, delegates normally

Stdin hand-off — re-confirmed independently. Built a sandbox with an instrumented runner that reports what it sees on fd 0 and exits 7:

• allowed push → RUNNER-STARTED, RUNNER-STDIN-EOF-OR-EMPTY, hook rc = 7. So the runner is reached, sees EOF (not ref lines, and not a hang), and exec still makes the runner's status the hook's status.
• blocked push → runner never starts.

The author's audit is correct: nothing in bin/run-pre-push-checks.sh or its three checks reads fd 0 (find | xargs reads the pipe, not stdin; run_check uses bash -c with no reads). And critically, the failure direction is benign — a downstream reader would get EOF, not block.

The delete decision — right, and the test does not pass for the wrong reason. Blocking a protected-branch delete is correct: it is strictly more destructive than a push, and it closes a real gap (block-main-push.sh's extract_push_ref bails on --delete and falls back to validating the checked-out branch). On receive.denyDeleteCurrent=ignore: the before-run settles it empirically — delete protected (main): remote main intact FAILED against the old hook, i.e. main was actually deleted on the remote. If the remote's own safety net were still masking the hook, that assertion would have passed against the old hook and the case would have proved nothing. It didn't. The neutralisation works and the case is honest.

Test suite: 130/130, chunked at 20 per batch.

PASS=130  FAIL=0   (7 chunks: 20/20/20/20/20/20/10)

test_block_main_push.sh and test_block_main_push_heredoc.sh both PASS (chunk 1) — block-main-push.sh is genuinely untouched. New suite standalone: PASS=30 FAIL=0.

shellcheck: clean on all three files at default severity (not just --severity=warning) — zero output, rc=0.

On the test approach (real git vs a stubbed git)

The deviation is right, and for a sharper reason than "it's more realistic". A stub would have to reimplement refspec resolution, upstream-tracking resolution, tag-vs-branch classification, and the all-zeroes sentinel in order to hand the hook correct stdin. The test would then be asserting that the stub's model of git agrees with the hook — while the actual claim under test is that git's model does. That is a second implementation that has to be trusted to agree, which is the precise failure mode this ticket exists to escape. Using real git removes the thing that could disagree.

The instruction it deviated from was written for block-main-push.sh, where a stub is genuinely the only way to prove a simulated command would have reached git. That premise doesn't hold here — this hook runs inside the real push lifecycle, so there is no simulated step to fool. The deviation is correctly scoped and correctly argued in the test header.

Can any test pass for the wrong reason? I looked for four mechanisms and found none:

1. Remote-side rejection masking the hook — ruled out empirically above (the before-run really does delete main).
2. The stubbed runner producing the non-zero — the stub is exit 0, and the block assertions match BLOCKED.*protected branch 'main', which the runner's own failure text (BLOCKED: pre-push check failed) does not match.
3. A no-op push git wouldn't even invoke the hook for — new_commit forces a genuine ref advance before every "must block" case.
4. Setup pushes silently exercising the hook — the remote is seeded via git clone --bare, so no push occurs during setup; only the case's own push goes through the hook.

The one residual: because the sandbox stubs the runner to exit 0, the stdin hand-off itself is not pinned by any automated test. I verified it by hand and it is correct today, but a future change that made the runner read fd 0 would not be caught in CI. See suggestion 2.

Issues Found

None blocking.

The duplicate — my call: defer to step 3, but pin it with a test

Third option, and I'd hold it as a strong recommendation rather than a merge blocker.

Why not fold the refactor in now. block-main-push.sh is the currently-deployed control. Editing it inside a PR that exists to build a new layer beneath it enlarges the blast radius of the riskiest kind of change for the sake of hygiene, and it produces a state nobody designed: a file that sources the shared lib but is still the control. Step 3 is specifically about resolving what that file is; doing half of it early makes step 3's review harder, not easier. The framework's own rail — security and trust-chain changes go Heavy, ambiguity rounds up — argues for one trust-chain edit per PR, not two.

Why not plain defer either. "Two copies, agreement asserted in a comment" is the pattern that generated this backlog. The real-world divergence risk here is genuinely low — both copies are the same three lines reading the same .git.protected_branches[] key through the same config_get, with the same literal fallback, and I confirmed they resolve identically both in a clean tree and in a fork that has a real project-config.json. But "low risk" asserted in prose is exactly what the last several rounds of this backlog were also asserting.

Why the pin is the right third answer. A test that asserts protected_branch_regex equals block-main-push.sh's inline resolution — under both config-present and config-absent conditions — converts the comment into an enforced invariant, without touching the deployed control at all. It costs a few lines, it makes divergence during the step-2→step-3 window impossible rather than merely unlikely, and it has a clean end of life: once step 3 migrates block-main-push.sh onto the lib, the test collapses to a tautology and gets deleted with the duplicate it was guarding. That is the cheapest available way to keep the deployed control untouched and not rely on trust.

I'm not blocking on it because the duplicate does not make anything worse than the pre-PR state — block-main-push.sh's resolution is byte-identical to what shipped before — and because the enforcement improvement this PR delivers is real and shouldn't wait on a hygiene item.

Does the PR document the duplicate honestly enough that step 3 can't miss it? In the code, yes. In the tracker, no.

The PR body's third bullet names it plainly, and _lib-protected-branches.sh's header (lines 31–39) goes further — it explicitly frames itself as "the thing step 3 migrates block-main-push.sh onto, not a competing implementation". That is the honest disclosure this repo's recent history has been short of, and it's a genuine improvement in reviewability.

The gap is that #1086's step-3 scope doesn't mention it. Step 3 reads: become advisory, keep a --no-verify check, close the parser bugs, update AgDR-0104. Nothing about migrating the protected-list resolution onto the shared lib. A step-3 implementer working from the issue could complete every bullet, demote the file to advisory, and leave the inline copy sitting there — because demoting to advisory doesn't require touching the resolution at all. The disclosure lives only in files that implementer has no particular reason to open.

Ask: add one bullet to #1086 step 3 — "migrate block-main-push.sh's inline protected-branch resolution onto .claude/hooks/_lib-protected-branches.sh and delete the duplicate" — and, if you take the pin, note that the pinning test is deleted at the same time. That's a comment on the issue, not a change to this PR.

Suggestions (non-blocking)

1. Manual invocation now hangs on an interactive stdin. STDIN_REFS=$(cat) blocks forever when fd 0 is an open TTY. Verified: sleep 30 | bash .githooks/pre-push times out; bash .githooks/pre-push </dev/null runs normally. Git always closes the hook's stdin, so the enforcement path is unaffected, and direct invocation isn't a documented path (docs/getting-started.md points at core.hooksPath, and the documented way to run the checks by hand is bin/run-pre-push-checks.sh). Still, it's a silent behaviour change for anyone who did run the hook directly. One line fixes it:

   if [ -t 0 ]; then STDIN_REFS=""; else STDIN_REFS=$(cat); fi

2. Pin the stdin hand-off with a test. The sandbox's exit 0 runner stub means CI would not notice if the runner started reading fd 0. A case with a runner that reports whether it saw stdin and exits a distinctive status would pin both halves of the contract (runner reached with EOF; exec preserves its status) for a few lines.

3. echo "$BRANCH" | grep → printf '%s\n' "$BRANCH" | grep is marginally safer for a pathological ref name beginning -e/-n (creatable via update-ref, not via git branch). Cosmetic.

4. PROTECTED_REGEX is interpolated unescaped into the regex, so an adopter setting .git.protected_branches: ["release/*"] gets regex semantics rather than the glob they probably meant. Identical in block-main-push.sh, so not a regression — worth a line in the config docs at some point, possibly alongside step 3.

5. Observation, not a finding: in a managed-project clone with no .claude/hooks/, the lib is absent and the hook WARNs and fails open. Step 1's installer already exits 3 for a repo with no tracked hooks dir, so this is consistent and acknowledged — just worth not assuming #1086's "human terminal push in a managed project" row is closed when step 3 lands.

Verdict

APPROVED

The design change is the right one and the evidence behind it is unusually strong: the discriminating before/after is real (20/20 must-block assertions flip), the over-narrowing controls hold including three shapes I added myself, and the one subtle way this could have silently failed — a pipe-fed loop swallowing exit 1 — is both correctly implemented and verified live against the shipped file. The deliberate duplicate is disclosed honestly in the code; my one ask is that it also be disclosed in the tracker, plus the optional pinning test, neither of which needs to happen in this PR.

Reviewed by Rex (Code Reviewer Agent)
Reviewed commit: 541e6b4660435c6970f00929887c7794b5068d5c

Security Review: PR #1090

Commit: 541e6b4660435c6970f00929887c7794b5068d5c

Summary

Step 2 of #1086. I verified by execution in a sandbox (real git binary, real bare remote), not by reading. Rex's core findings hold and I did not re-derive them; this review covers the ground he did not: degradation/fail-direction, config-driven regex injection assessed as a security property, --no-verify disclosure, and the control-comparison question step 3 depends on.

The design premise is sound and I confirmed it independently. Every refspec shape I threw at real git resolves to refs/heads/main in the hook's stdin — push origin, bare push, --all origin, main:refs/heads/main, +main:main. main:HEAD resolves to refs/heads/HEAD (a branch literally named HEAD — it does not touch main, so it is not a bypass). There is genuinely no text to misparse at this layer.

Nothing here is a regression. block-main-push.sh is untouched and remains the active control, so this PR strictly increases the union of protections. My findings are all either pre-existing parity or scoped to step 3.

Checklist Results

• Secrets & Credentials: Pass (no secrets, no credential handling)
• Injection Prevention: Pass with finding — see MEDIUM-1 (config-to-regex interpolation)
• XSS Prevention: N/A
• Auth & Authorisation: Pass — this is an authorisation control; see the headline section
• Data Protection: Pass
• API Security: N/A
• Gate & Trust-Chain Integrity: Pass — this adds an enforcement point and weakens none. block-main-push.sh byte-identical, .claude/settings.json untouched, no gate made to exit 0 on a path it previously blocked.

Headline: is this genuinely stronger than block-main-push.sh?

Answer: it is stronger on one axis, strictly weaker on another. It is complementary, not a superset — and #1086's step 3 as currently written would be a net reduction in protection.

What the PreToolUse hook sees that a pre-push hook structurally cannot:

What the new hook sees that the old one cannot: every refspec shape (verified above), deletions, force pushes, and human terminal pushes.

The sharpest item is the commit gate. .githooks/ contains only pre-push — there is no pre-commit hook. block-main-push.sh blocks commits on a protected branch today with exit 2. #1086's step 3 says that file "becomes an advisory … [and] --no-verify becomes the one text-matching job that remains" — which would leave protected-branch commits with no blocking control anywhere, and the git layer has nothing to replace it with.

I rate this HIGH, scoped to step 3 — it does not block this PR, because block-main-push.sh is untouched here and the commit gate is fully intact at this commit. But step 3 must not land until it explicitly retains (a) the commit gate as a control, not an advisory, (b) the --no-verify check, and (c) some answer for clones where core.hooksPath was never installed. Please record this on #1086 alongside the migration bullet already added.

Security Issues Found

MEDIUM-1 — a malformed protected-branch list fails OPEN, silently disabling protection for every branch in it. Pre-existing parity, not a regression.

PROTECTED_REGEX is interpolated unescaped into grep -qE "^(${PROTECTED_REGEX})$". Rex called this cosmetic; the fail direction is the part that matters. Measured, with config present, against both layers:

A single malformed entry makes grep exit 2 (grep: parentheses not balanced), which the if reads as "not protected" — so protection for main disappears because of a typo in an unrelated entry. The error text does go to stderr, but the push proceeds. The [ -n "$PROTECTED_REGEX" ] guard does not catch the ["",""] case either, since the regex is the literal |, and ^(|)$ matches only the empty string.

Related, and the likelier real-world hit: ["release/*"] is regex, not glob — it protects release and release/ but not release/1.0. An adopter writing the obvious thing gets silently less protection than they think.

Both layers behave identically, so this is not introduced here. It matters because step 3 makes this the sole blocking layer. Cheapest fix, no regex engine involved: match with a bash case over the config values, or validate the assembled regex once and fail closed with a loud message if grep exits ≥2.

MEDIUM-2 — the enforcement block fails open silently when the lib is present but unusable.

Degradation matrix, measured by feeding the shipped hook synthetic ref lines:

The command -v protected_branch_regex guard converts "the lib exists but is unusable" into a silent no-op. The missing-file case warns; the unusable-file case does not. That asymmetry is worth closing precisely because step 3 will migrate block-main-push.sh onto this lib — a rename of protected_branch_regex during that migration would remove enforcement with zero signal, and every test that supplies a well-formed lib would still pass. One else branch emitting the same WARN fixes it.

The empty-stdin fail-open is fine and I'd leave it: I confirmed git does not invoke the hook at all when there is nothing to push ("Everything up-to-date" produces no hook invocation), so it is unreachable on a real push.

MEDIUM-3 — --no-verify is not named anywhere, while the PR claims to close the whole bug class.

Zero mentions of no-verify in .githooks/pre-push; no gate for it anywhere in .claude/hooks/ or bin/ (it appears only as an argument to skip over in _lib-extract-push-ref.sh's option table). The PR body's "What this PR does NOT do" is honest about the core.hooksPath dependency — genuinely good disclosure — but omits the flag that bypasses this layer entirely, while the first bullet says this "closes the whole bug class AgDR-0113 documents". AgDR-0104's standing requirement is to name what is a control and what is a backstop. #1086 states the residual correctly in its Risks section; this PR should carry the same sentence, in the hook header and in the PR body. Documentation-only, but on a trust-chain enforcement point that a later step demotes another control on the strength of.

LOW-1 — I agree with Rex that the $(cat) TTY hang is non-blocking. I looked for a reachable path and found none: no CI workflow invokes the hook, bin/install-git-hooks.sh and check-git-hooks-installed.sh never execute it, and git always closes the hook's stdin. I did confirm the hang is real and specific to an open fd 0 (sleep 8 | pre-push times out; < /dev/null exits cleanly). The one-line [ -t 0 ] guard Rex proposed is still worth taking — it costs nothing and removes a foot-gun for anyone running the hook by hand.

LOW-2 — the config source is not necessarily the repo being pushed. protected_branch_regex resolves its root through resolve_ops_root (session-pin first, otherwise a walk for .apexyard-fork), so the protected list can come from the ops fork's config rather than the config of the repo whose push is being judged. This is the intended #310 behaviour and block-main-push.sh does the same, so it is parity — but the hook's own help text points the reader at .claude/project-config.json without noting which one wins. Worth a clarifying line.

The duplication — Rex's "byte-identical" claim verified, and the exposure is bounded in the safe direction

I tested the two resolutions side by side across six config conditions including the malformed and empty ones, and they agree in every case. More importantly for the security question: while both layers are active, protection is the union — either can block. So a divergence during the step-2→step-3 window makes protection stricter, not looser. The exposure is bounded and fails safe. It only becomes a weakening at step 3, when block-main-push.sh stops being a control — which is exactly when the migration bullet already added to #1086 removes the duplicate. I have nothing to add to Rex's call; deferring is right.

Independent confirmation of the delete finding — it is real

Confirmed directly, not taken from the author or from Rex. With a feature branch checked out, block-main-push.sh returns rc=0 with no output for a delete-push of main — allowed. The same shape at the git layer (delete sentinel on refs/heads/main) returns rc=1 — blocked. The gap is specifically "delete a protected branch from a non-protected checkout"; from a main checkout the fallback happens to catch it for the wrong reason. This is a live gap in the currently-deployed control and should be recorded on #1086 regardless of this PR's fate.

Verification performed

• Full hook suite re-run in 6 chunks of ≤20: PASS=115 FAIL=0 (115 test files, 0 failures — my count is per-file; the runner's own tally of 130 counts differently, but there are no failures either way)
• New suite standalone: PASS=30 FAIL=0
• shellcheck at default severity on all three files: clean, rc=0
• Sandbox probes: refspec resolution, config injection, degradation matrix, delete gap, hang reachability

One live corroboration of the PR's own premise, from this review session: while writing up these findings, validate-branch-name.sh false-blocked on prose inside a heredoc, extracting a branch named git. That is the same text-matching unsoundness class this PR exists to escape, firing unprompted during its review.

Recommendations

1. Record the commit-gate requirement on #1086 step 3 before it is scoped (HIGH, gates step 3, not this PR).
2. Add the --no-verify residual to the hook header and PR body (MEDIUM-3).
3. Emit the WARN on the unusable-lib paths, not only the missing-file path (MEDIUM-2).
4. Validate the assembled protected-branch regex and fail closed on a grep error (MEDIUM-1) — ideally before step 3 makes this the only layer.
5. Take Rex's [ -t 0 ] one-liner (LOW-1).

None of these blocks this PR.

Verdict

APPROVED

The design is right, the premise is verified rather than asserted, and the enforcement improvement is real. Every finding above is either pre-existing parity that this PR inherits without worsening, or a scoping requirement for step 3 — and step 3 is where the genuine risk sits: this layer is not a superset of block-main-push.sh, and demoting that hook without preserving the commit gate, the --no-verify check, and a story for uninstalled clones would be a net reduction in protection.

Reviewed by Hakim (Security Auditor)
Reviewed commit: 541e6b4660435c6970f00929887c7794b5068d5c