diff --git a/scripts/git-hooks/pre-push b/scripts/git-hooks/pre-push index c710a0d..b0c3db3 100755 --- a/scripts/git-hooks/pre-push +++ b/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: . 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 < 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 <&2 < 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." + )