fix: the pre-push hook checked the wrong branch, and my first fix was wrong too (#437 review)

[Polichinel]

Recovers work orphaned on ops/pre-push-hook-and-automerge after #437 auto-merged. Part of #421, Story 5 follow-up.

This is C-340 mechanism 2, in the PR that fixes C-340 mechanism 2

#437 auto-merged at 15:25:52 carrying the broken v1 hook. The review fixes were pushed to that branch afterwards, landed on a branch with no open PR, and git push reported success. Exactly #416's shape, in the change built to prevent it.

The hook that would have refused that push was not installed in this clone — core.hooksPath is per-clone config git does not version, which is precisely the gap the story's own documentation warns about. Now installed here.

development currently carries a hook that is worse than none, which is why this PR matters: it checks the wrong branch, and it refuses legitimate work.

Three defects, in order

1. It checked the wrong branch entirely. Git hands a pre-push hook the refs being pushed, one per line on stdin. v1 read git rev-parse HEAD instead. So git push origin feature-a:feature-a with main checked out took the main fast-path and never consulted feature-a — silently allowing the exact push the hook exists to refuse.

2. It matched on branch NAME. Names get reused here: docs/roadmap-plan-v11 spans PRs #50–54, feat/acled-phase2 spans #35–36. A fresh branch reusing a name was permanently refused, with no recourse but --no-verify. Reproduced against the live repo. A hook that blocks legitimate work is worse than the bug it prevents — it gets uninstalled.

3. My fix for (2) was also wrong, and only the drill showed it. It asked whether the merged PR's head was an ancestor of what was being pushed. That holds for a squash merge. For a merge-commit merge the head enters the base branch's history permanently, so it is an ancestor of every branch cut from it afterwards — PR #54's head is an ancestor of development today. Every future branch would still have been refused. The drill returned rc=1 where rc=0 was required.

The fix

The distinguishing signal was on stdin all along: remote_sha. If the remote ref sits exactly on the merged PR's head, this push adds to merged work. Forty zeros (the branch is being created) or any other value means it does not.

reused name, creating branch                     rc=0
reused name, remote at another sha               rc=0
remote sitting on merged PR #54's head           rc=1
remote sitting on merged PR #436's head          rc=1
branch delete                                    rc=0
tag push                                         rc=0
development                                      rc=0

The guard now executes the hook

The review's third finding was that tests/test_git_hooks.py only grepped its own source and would have caught none of this. Correct — every text assertion passed straight over all three defects.

11 tests now run the hook: deletes, tags, long-lived branches, empty stdin, the gh-absent fail-open path, that stdin is read at all, and the reused-name case — which short-circuits before gh is consulted, so it asserts the real decision rather than an offline fallback.

That harness was wrong first too: PATH=/nonexistent-bin hid bash from the test runner itself. Replaced with a sandbox holding git, sed and bash but not gh, with gh's absence asserted before the run.

Verification

• bash -n scripts/git-hooks/pre-push → OK
• ruff check . → All checks passed
• pytest tests/test_git_hooks.py → 11 passed
• Full suite was green on this content before ops: a hook that cannot be forgotten, and a readback that cannot be skipped (C-340, #426) #437 merged; CI will confirm here

🤖 Generated with Claude Code

[Polichinel]

Closing unmerged. The hook is being abandoned, not fixed — see the multi-expert review below.

Four versions, four different environment properties, all defeating it:

	approach	defeated by
v1	HEAD + branch name	checked the wrong branch; permanently refused reused names
v2	merged head is an ancestor	merge-commit merges put it in development forever
v3	remote_sha equals merged head	delete_branch_on_merge makes it 0000… — proved with a two-clone experiment
v4	ancestor AND not in base	never shipped

Plus the test suite was vacuous: reconstructing the v1 hook and running all seven behavioural tests — every one passed.

The framing was the error. A client-side check is racing GitHub's asynchronous branch deletion; that is a property of the system, not a bug to iterate out. Each failure came from a different environment property, which is the signature of inferring state you cannot see.

The orphaned state is trivially observable after the fact, which is what this should have been from the start:

remote branch                              in development?      open PR?
fix/pre-push-hook-review-findings          NOT in development   438
ops/pre-push-hook-and-automerge            NOT in development   none   <- the orphan

Replaced by a detector in release-topology.yml, which already has fetch-depth: 0, issues: write, a daily cron, and one-reusable-issue machinery. It cannot block work, cannot be uninstalled, needs no core.hooksPath, and would have caught tonight's incident.

scripts/arm_automerge.sh (mechanism 1) is kept — it works and is drilled.