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

scripts/git-hooks/pre-push

@@ -1,5 +1,6 @@
 #!/usr/bin/env bash
-# Refuse a push to a branch whose pull request has already merged (C-340).
+# Refuse a push that ADDS COMMITS TO A BRANCH WHOSE PR HAS ALREADY MERGED
+# (C-340 mechanism 2).
 #
 # WHY THIS IS A HOOK AND NOT ADVICE
 # #416 merged the instant CI went green. A follow-up commit was then pushed
@@ -11,51 +12,105 @@
 # not anyone remembers. A checklist item would be the same class of defect
 # this epic exists to remove, so this is a hook: it cannot be forgotten.
 #
-# WHY IT ALLOWS THE PUSH WHEN IT CANNOT ANSWER
-# No `gh`, no auth, no network -> allow. A hook that blocks work when it
-# does not know is worse than the problem it solves, and it would be
-# uninstalled within a day. Same idiom as the deploy gates (C-320): skip
-# where the environment cannot answer, and say why.
+# TWO THINGS THIS GOT WRONG FIRST, both found by review (#437):
+#
+#   1. It read the branch from `git rev-parse HEAD` instead of from stdin.
+#      Git hands a pre-push hook the refs actually being pushed, one per
+#      line: <local ref> <local sha> <remote ref> <remote sha>. Using HEAD
+#      meant `git push origin feature-a:feature-a` while `main` was checked
+#      out hit the main fast-path and never checked feature-a at all —
+#      silently allowing the exact push this hook exists to refuse.
+#
+#   2. It matched on branch NAME alone. Names get reused: this repo has
+#      `docs/roadmap-plan-v11` across PRs #50-54 and `feat/acled-phase2`
+#      across #35-36. A fresh branch reusing a name would be permanently
+#      refused, with no recourse but --no-verify. A hook that blocks
+#      legitimate work is worse than the bug it prevents.
+#
+#   3. The first FIX for (2) was also wrong, and only the drill showed it.
+#      It asked "is the merged PR's head an ancestor of what I am pushing".
+#      For a squash merge that works; for a merge-commit merge the branch
+#      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, so every future branch would have
+#      been refused. Verified, not reasoned.
+#
+# The signal that actually distinguishes the two cases is already on stdin:
+# REMOTE_SHA — what the remote ref points at right now, or forty zeros if
+# it does not exist. If the remote branch is sitting exactly on the merged
+# PR's head, this push adds to merged work. If the remote ref is absent
+# (branch deleted on merge, name reused) or points somewhere else, it does
+# not.
+#
+# WHY IT ALLOWS THE PUSH WHENEVER IT CANNOT ANSWER
+# No `gh`, no auth, no network, object not present locally -> allow. A hook
+# that blocks work when it does not know gets uninstalled within a day, and
+# then it guards nothing. Same idiom as the deploy gates (C-320): skip where
+# the environment cannot answer, and say why.
 #
 # Install:  git config core.hooksPath scripts/git-hooks
 # Bypass:   git push --no-verify
 
 set -uo pipefail   # deliberately NOT -e: every failure path below must fall
-                   # through to "allow", never abort the push by accident.
-
-branch=$(git rev-parse --abbrev-ref HEAD 2>/dev/null) || exit 0
-[ -n "$branch" ] && [ "$branch" != "HEAD" ] || exit 0
+                   # through to "allow", never abort a push by accident.
 
-# Long-lived branches are pushed to constantly and never have their own PR.
-case "$branch" in
-    main|development) exit 0 ;;
-esac
+ZERO="0000000000000000000000000000000000000000"
+refused=0
 
 command -v gh >/dev/null 2>&1 || {
     echo "pre-push: gh not installed — cannot check for a merged PR, allowing." >&2
     exit 0
 }
 
-merged=$(gh pr list --head "$branch" --state merged \
-             --json number,mergedAt --limit 1 2>/dev/null) || {
-    echo "pre-push: gh could not answer (auth or network) — allowing." >&2
-    exit 0
-}
+# One line per ref being pushed. Reading from stdin without a pipe keeps
+# this in the current shell, so `refused` survives the loop.
+while read -r _local_ref local_sha remote_ref remote_sha; do
+    [ -n "${remote_ref:-}" ] || continue
+
+    # Deleting a remote ref: nothing is being added, nothing to refuse.
+    [ "$local_sha" = "$ZERO" ] && continue
+
+    # Branches only. Tags are immutable by ruleset and have no PR.
+    case "$remote_ref" in
+        refs/heads/*) branch=${remote_ref#refs/heads/} ;;
+        *) continue ;;
+    esac
+
+    # Long-lived branches are pushed to constantly and have no PR of their own.
+    case "$branch" in
+        main|development) continue ;;
+    esac
 
-# Empty list, or anything unparseable, means "no merged PR" — allow.
-case "$merged" in
-    ""|"[]") exit 0 ;;
-esac
+    # Creating the branch on the remote: there is nothing there to add to.
+    # This is the reused-name case, and it must be allowed.
+    [ "${remote_sha:-$ZERO}" = "$ZERO" ] && continue
 
-number=$(printf '%s' "$merged" | sed -n 's/.*"number":\([0-9]*\).*/\1/p')
-when=$(printf '%s' "$merged" | sed -n 's/.*"mergedAt":"\([^"]*\)".*/\1/p')
-[ -n "$number" ] || exit 0   # could not parse -> do not block
+    pr=$(gh pr list --head "$branch" --state merged \
+             --json number,mergedAt,headRefOid --limit 1 2>/dev/null) || {
+        echo "pre-push: gh could not answer for $branch (auth or network) — allowing." >&2
+        continue
+    }
+    case "$pr" in ""|"[]") continue ;; esac
 
-cat >&2 <<EOF
+    number=$(printf '%s' "$pr" | sed -n 's/.*"number":\([0-9]*\).*/\1/p')
+    when=$(printf '%s' "$pr" | sed -n 's/.*"mergedAt":"\([^"]*\)".*/\1/p')
+    head=$(printf '%s' "$pr" | sed -n 's/.*"headRefOid":"\([^"]*\)".*/\1/p')
+    [ -n "$number" ] && [ -n "$head" ] || continue   # unparseable -> allow
 
-  REFUSED: $branch already had its pull request merged.
+    # THE DISTINGUISHING TEST. The remote branch must be sitting exactly
+    # on the merged PR's head — that is the #416 shape: branch merged,
+    # remote ref left where the merge found it, and a new commit pushed on
+    # top. Anything else (remote moved on, name reused after a delete) is
+    # not this bug and must not be blocked.
+    [ "$remote_sha" = "$head" ] || continue
+
+    refused=1
+    cat >&2 <<EOF
+
+  REFUSED: $branch still carries the work of a pull request that has merged.
 
     PR #${number} merged at ${when:-unknown}
+    the remote branch is still sitting on its head ${head}
 
   Pushing here does not reach the base branch. The commit lands on a branch
   with no open PR, git reports success, and the work is silently not merged.
@@ -69,4 +124,6 @@ cat >&2 <<EOF
   If you are certain you want this push:  git push --no-verify
 
 EOF
-exit 1
+done
+
+exit "$refused"

tests/test_git_hooks.py

@@ -23,6 +23,8 @@
 from __future__ import annotations
 
 import os
+import shutil
+import subprocess
 from pathlib import Path
 
 REPO = Path(__file__).resolve().parents[1]
@@ -67,3 +69,162 @@ def test_install_step_is_documented(self) -> None:
             "per-clone and git does not version it, so an undocumented hook "
             "is a file nobody installs."
         )
+
+
+BASH = shutil.which("bash") or "/bin/bash"
+
+
+def _sandbox_without_gh(tmp_path: Path) -> str:
+    """A PATH holding everything the hook needs EXCEPT ``gh``.
+
+    Not simply an empty PATH: the hook also runs ``git`` and ``sed``, so
+    blanking PATH would exercise a different failure than the one under
+    test. The first version of this helper set ``PATH=/nonexistent-bin``
+    and hid ``bash`` from the test runner itself — the harness broke, not
+    the hook.
+    """
+    binder = tmp_path / "bin"
+    binder.mkdir(exist_ok=True)
+    for tool in ("bash", "git", "sed", "cat", "tr", "cut", "printf"):
+        found = shutil.which(tool)
+        if found:
+            link = binder / tool
+            if not link.exists():
+                link.symlink_to(found)
+    assert shutil.which("gh", path=str(binder)) is None, (
+        "sandbox still exposes gh — the drill would test nothing"
+    )
+    return str(binder)
+
+
+def _run_hook(stdin: str, path: str | None = None) -> subprocess.CompletedProcess[str]:
+    """Invoke the hook the way git does: refs on stdin, remote in argv."""
+    env = dict(os.environ)
+    if path is not None:
+        env["PATH"] = path
+    return subprocess.run(
+        [BASH, str(HOOK), "origin", "https://example.invalid/repo.git"],
+        input=stdin,
+        capture_output=True,
+        text=True,
+        env=env,
+        cwd=REPO,
+    )
+
+
+ZERO = "0" * 40
+
+
+class TestHookReadsWhatGitActuallySends:
+    """Behavioural, because the substring checks above caught neither bug.
+
+    Review of #437 found two defects that every text assertion in this
+    file passed straight over:
+
+    1. the hook read the branch from ``git rev-parse HEAD`` instead of
+       from stdin, so ``git push origin feature-a:feature-a`` while
+       ``main`` was checked out was never checked at all — silently
+       allowing the push the hook exists to refuse;
+    2. it matched on branch *name*, so a fresh branch reusing a name
+       (this repo has several) was permanently refused.
+
+    A guard that only greps its own source proves the source contains
+    words. These run it.
+    """
+
+    def test_a_delete_push_is_allowed(self) -> None:
+        """local sha of all zeros means "delete" — nothing is being added."""
+        r = _run_hook(f"refs/heads/x {ZERO} refs/heads/x abc123\n")
+        assert r.returncode == 0, (
+            f"deleting a remote branch must not be refused; got "
+            f"{r.returncode}\n{r.stderr}"
+        )
+
+    def test_a_tag_push_is_allowed(self) -> None:
+        r = _run_hook(f"refs/tags/v9.9.9 abc123 refs/tags/v9.9.9 {ZERO}\n")
+        assert r.returncode == 0, (
+            f"tag pushes have no PR and must not be refused; got "
+            f"{r.returncode}\n{r.stderr}"
+        )
+
+    def test_long_lived_branches_are_allowed(self) -> None:
+        for branch in ("main", "development"):
+            r = _run_hook(
+                f"refs/heads/{branch} abc123 refs/heads/{branch} {ZERO}\n"
+            )
+            assert r.returncode == 0, (
+                f"{branch} is pushed to constantly and has no PR of its "
+                f"own; got {r.returncode}\n{r.stderr}"
+            )
+
+    def test_empty_stdin_is_allowed(self) -> None:
+        assert _run_hook("").returncode == 0
+
+    def test_creating_a_branch_is_allowed_even_if_the_name_was_used_before(
+        self,
+    ) -> None:
+        """The reused-name case, which two earlier versions got wrong.
+
+        ``remote_sha`` of all zeros means the remote ref does not exist —
+        the branch is being created. There is nothing on the remote to add
+        to, so this cannot be C-340 mechanism 2 whatever the name.
+
+        This repo really does reuse names: ``docs/roadmap-plan-v11``
+        spans PRs #50-54 and ``feat/acled-phase2`` spans #35-36. v1 of
+        this hook refused all of them permanently. v2 refused them too,
+        for a subtler reason — it asked whether the merged head was an
+        *ancestor*, and a merge-commit merge puts that head into the base
+        branch's history forever, so it is an ancestor of every branch cut
+        from it afterwards.
+
+        Short-circuits before ``gh`` is consulted, so this asserts the
+        real decision rather than an offline fallback.
+        """
+        r = _run_hook(
+            f"refs/heads/docs/roadmap-plan-v11 abc123 "
+            f"refs/heads/docs/roadmap-plan-v11 {ZERO}\n"
+        )
+        assert r.returncode == 0, (
+            f"creating a branch must never be refused, however many merged "
+            f"PRs once used that name; got {r.returncode}\n{r.stderr}"
+        )
+        assert not r.stderr.strip(), (
+            f"should short-circuit silently before consulting gh; "
+            f"stderr was {r.stderr!r}"
+        )
+
+    def test_it_allows_when_gh_is_absent(self, tmp_path: Path) -> None:
+        """The fail-open path, executed rather than grepped."""
+        r = _run_hook(
+            f"refs/heads/anything abc123 refs/heads/anything {ZERO}\n",
+            path=_sandbox_without_gh(tmp_path),
+        )
+        assert r.returncode == 0, (
+            f"must allow when gh cannot be found; got {r.returncode}"
+        )
+        assert "gh not installed" in r.stderr, (
+            f"must say WHY it allowed; stderr was {r.stderr!r}"
+        )
+
+    def test_it_reads_the_ref_from_stdin_not_from_head(
+        self, tmp_path: Path
+    ) -> None:
+        """The defect that made the hook check the wrong branch entirely.
+
+        With ``gh`` unavailable every branch is allowed, so this cannot
+        assert a refusal. What it can assert is that the hook *consulted
+        the ref it was handed* rather than the checked-out branch: given
+        a non-long-lived ref on stdin it reaches the gh lookup and says
+        so, whereas the old HEAD-based version would have taken the
+        ``main``/``development`` fast path and stayed silent.
+        """
+        r = _run_hook(
+            f"refs/heads/some-feature abc123 refs/heads/some-feature {ZERO}\n",
+            path=_sandbox_without_gh(tmp_path),
+        )
+        assert "gh not installed" in r.stderr, (
+            "the hook did not reach the gh lookup for a ref given on "
+            "stdin — it is not reading stdin. That is the defect where "
+            "`git push origin feature-a:feature-a` from `main` went "
+            "entirely unchecked."
+        )