ci: abandon the pre-push hook, detect the orphan instead (C-340, #426) - #439
ci: abandon the pre-push hook, detect the orphan instead (C-340, #426)#439Polichinel wants to merge 5 commits into
Conversation
The deliverable is a removal. Four versions of a client-side guard, four
different defeats:
v1 HEAD + branch name checked the wrong branch (git supplies pushed
refs on stdin); permanently refused reused
names — docs/roadmap-plan-v11 spans #50-54
v2 merged head is an a merge-commit merge puts that head in the base
ancestor branch forever, so it is an ancestor of every
later branch. PR #54's is in development today
v3 remote_sha equals delete_branch_on_merge removes the branch first,
the merged head so git reports 0000... — proved with a two-clone
experiment
v4 ancestor AND not never shipped
in base
The test suite was vacuous: reconstructing v1 and running all seven
behavioural tests passed every one.
It recurred while being fixed. #437 auto-merged carrying broken v1; the
review fixes were pushed to that branch afterwards and orphaned — the
same defect inside the PR addressing it. The hook was not installed in
that clone, because core.hooksPath is per-clone config git does not
version. That is the argument against an install-it-yourself guard.
Abandoned rather than fixed a fifth time. Four failures from four
DIFFERENT environment properties is the signature of inferring state you
cannot see. A multi-expert panel converged independently: a client check
races an asynchronous deletion (a property, not a bug); the module is
shallow and its complexity is entirely special cases; twice in ~440 PRs,
both recovered by cherry-pick inside the hour. Their verdict on the
warning variant was sharpest — a notice printed on every push goes
invisible in a week, which is the exact fails-green shape this epic
exists to remove.
The framing was the error. The orphaned STATE is unambiguous once things
settle: a remote branch with commits beyond its merged PR's head and no
open PR. release-topology.yml already had fetch-depth: 0, issues: write,
a daily cron and one-reusable-issue machinery.
Drilled end-to-end with the body extracted from the workflow, not
retyped: found the genuine orphan ("1 commit(s) pushed AFTER PR #437
merged, no open PR"), ignored a branch whose PR was closed-unmerged, and
reported clean once it was deleted. Measured against the merged head
rather than development — merges here are squashes, so a branch's own
commits are never ancestors of development and the naive comparison would
flag every merged branch.
scripts/arm_automerge.sh is kept: mechanism 1 works and was drilled live.
C-340 stays OPEN on the residue — detection is not prevention, and
nothing forces the script's use.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code reviewFound 5 issues. The first three are all the same shape the PR exists to remove: the detector reports
views-datafactory/.github/workflows/release-topology.yml Lines 47 to 51 in 0ddec67
views-datafactory/.github/workflows/release-topology.yml Lines 178 to 183 in 0ddec67
views-datafactory/.github/workflows/release-topology.yml Lines 159 to 161 in 0ddec67
views-datafactory/.github/workflows/release-topology.yml Lines 166 to 168 in 0ddec67
views-datafactory/docs/guides/publishing_to_pypi.md Lines 145 to 149 in 0ddec67 Two lower-confidence observations, not blocking: the 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…349) Addresses every finding from /code-review and /review-diff on #439. The detector shipped in the previous commit would have reported "No orphaned branches" forever, green, on a daily cron. WHY THE DRILL MISSED ALL OF IT — C-347, Tier 2 It ran under the operator's personal `gh` credentials, which carry full scope. Production runs under GITHUB_TOKEN, and an explicit permissions: block sets every UNLISTED scope to `none` — so `pull-requests` was revoked and both `gh pr list` calls would 403 into `2>/dev/null || continue`, indistinguishable from "no PR found". The drill obtained and reported its result correctly; it simply ran in the wrong world. That is neither C-345 (an instrument misreading a result it did obtain) nor C-336 (a guard narrower than its property) but a third thing, and it has now fired four times in one week, each from a different environment property: `gh` never hidden because it lives in ~/.local/bin; a `git checkout` silently refused so a "want FAIL" case ran on-tag; pytest run from the main repo while the test shelled out to `git describe`; this. FIXES permissions `pull-requests: read`, with the reasoning inline reuse require the merged head to be a genuine ANCESTOR of the tip. Names are reused here — chore/version-bump-1.2.13, docs/roadmap-plan-v11, feat/acled-phase2 have each headed more than one PR — and matching by name alone was the property that defeated hook v1, surviving into its replacement. Plus `--not origin/development` for the pre-#54 merge-commit era blind loop `for x in $(cmd)` swallows a failure of cmd completely: GHA runs `bash -e {0}`, `set -uo pipefail` does not remove -e, and a `for` word list is exempt. Verified: `bash -e -c 'for x in $(false|sed s/a/b/); do echo BODY; done; echo END'` prints only END per-branch every `|| continue` turned "could not check" into "this branch is fine". Now counted; a partial result is qualified; answering for NOTHING exits non-zero, because a clean report resting on zero observations is the defect itself, not a degraded mode of it GUARDS, ALL DRILLED AGAINST THE BROKEN STATE WITH A CONTROL test_ci_gates.py gains four assertions (the permission grant, the fetch ordering, the blind-iteration idiom, the zero-answers rule). Revoke, restore, reorder, remove: each reddened exactly the guard aimed at it. The reused-name fix was drilled in a scratch repo — git ancestry does not differ between machines — where the new logic reports the genuine orphan as 1 stranded commit and SKIPS the namesake, while the old logic reported the namesake as 3 commits stranded. test_ops_scripts.py (new) restores the guard on arm_automerge.sh's executable bit, deleted as collateral when test_git_hooks.py went with the hook — C-348. A test module is an organisational unit that quietly doubles as a coverage unit, and only the first is visible when you delete it. C-345 CORRECTED — ITS OWN PRESCRIBED MITIGATION IS DEFECTIVE It recommends `grep -cE '^FAILED'` as a second independent reader. pytest colourises, so the line is \033[31mFAILED\033[0m and the caret never matches. The drill harness here reported "DID NOT CATCH" four times while all four drills had caught. Corrected to --color=no at the source. Second time an entry has prescribed a defective remedy (C-331's unquoted printf 'url=%s' turned a failure ping into a success ping): a mitigation written into an entry is untested code that inherits the entry's credibility without earning it. C-349 REGISTERED, DELIBERATELY NOT FIXED HERE — Tier 2 Generalising the per-branch finding found the same shape in production: acled.py, ucdp.py and health.py all drop an unparseable ledger line with a bare `except json.JSONDecodeError: continue`, uncounted, while grid_compilation.py already does it right with n_skipped_spatial += 1. The consolidators use the ledger to decide what to consolidate, so a dropped line means a successful harvest is silently not consolidated. An untested change to consolidation does not belong in a workflow PR. validate_docs PASSED | ruff clean | PYTEST_EXIT=0 | FAILED=0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… fail Second review round on #439 (/code-review max). Fifteen findings, verified by extracting the shell step from the YAML and running it under `bash -e` against synthetic repos with a stubbed gh. The round is worth more than the story. C-350 — THE DELIVERABLE WOULD NOT HAVE RUN GitHub fires `schedule` from the DEFAULT BRANCH ONLY. Default here is main; all work goes through development. So the detector merges to development and the 06:00 cron keeps running main's copy, which has no detector, until an irregular release promotion carries it over. Measured: origin/main's workflow has ZERO occurrences of "orphan", and every event:schedule run has headBranch:main. Meanwhile the guide, the changelog and C-340's narrowing all said "checks daily" as present fact, and the four new guards assert properties of the BRANCH's file — green on every PR while the branch that runs the cron has none of it. Every verification performed was true and none of them was the question. A workflow_dispatch run proves the code works; it says nothing about whether anything will call it. All three claims corrected. C-351 — A LIVE RED GATE NOBODY WAS READING serving-freshness.yml has failed every scheduled run since at least 2026-08-08: no actions/checkout, so a git command aborts with `fatal: not a git repository`. Freshness alerting for served data has been dead five days. Registered immediately — "pre-existing" is not a disposition this project accepts. THREE OF MY OWN GUARDS COULD NOT FAIL FOR WHAT THEY CLAIMED "answered=" is a SUBSTRING of "unanswered=", so the counting assertion passed with every `answered` counter deleted. A bare `"::error::" in run` over a 130-line body was satisfied by an unrelated guard a hundred lines below, so deleting BOTH enumeration guards kept the suite green. The class comment three lines above warns against asserting "the how, not the what" and cites C-336. Rewritten with a lookbehind, anchored patterns, and step lookup by stable `id:` instead of a substring of the body under test — then drilled against the exact three defeats. Rewriting them produced a fourth instance: the first replacement matched the step's own COMMENTS, which quote the anti-patterns they warn against, so it reddened the fixed file. Caught by the control run. Comments are stripped before matching now — what test_heartbeat_secret already learned, in this same epic. C-345's ADDENDUM FROM THIS MORNING WAS WRONG — RETRACTED IN PLACE It claimed pytest colourises so `grep -cE '^FAILED'` cannot match. pytest does NOT colourise into a file; the ANSI came from FORCE_COLOR=3 in this operator's shell. Measured: with it, grep returns 0 on a failing run; with `env -u FORCE_COLOR`, 1 and zero ANSI lines. C-345's original prescription was fine in a plain shell and in CI. An anomaly from an uncontrolled variable was diagnosed as a tool defect and escalated into a general claim about the register — withdrawn. That is C-347, committed inside the commit registering C-347. C-349 UNDERCOUNTED ITS OWN LOCATIONS Named 3 JSONDecodeError sites; the grep returns 7, and digests_and_ledgers.py already logs the skip, falsifying the entry's "nothing anywhere records that it happened". The grep is now in the Location field so the count is re-derivable rather than asserted. NOT FIXED, AND WHY THE STORY STOPS HERE Nine verified behavioural findings remain: the prescribed recovery never clears the alert (permanently red cron, permanently open issue); a partial scan writes an unqualified orphans=false that auto-closes a genuine orphan issue; the issue-reuse path never renders the orphan section, so the daily comment names no branch; merge-base exit 128 is booked as a definite answer INSIDE the accounting added to fix C-347; two seds depend on gh's compact JSON; --limit 1 orders by createdAt not mergedAt; a bare branch name in git fetch resolves refs/tags/ first. Round one: five defects. Round two: fifteen, each from a different property of git, gh, or GitHub. That is the signature that ended the pre-push hook after four versions, reappearing in its replacement — and the panel's argument applies unchanged: twice in ~440 PRs, both recovered by cherry-pick inside the hour. The design question goes back to the operator rather than being resolved by a third round. validate_docs PASSED | ruff clean | PYTEST_EXIT=0 | FAILED=0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Converted to draft — the ritual stopped at iteration 2 of 3
GitHub fires The guide, the changelog and C-340's narrowing all said "checks daily" as present fact. The four guards assert properties of the branch's file, so they go green on every PR while the branch that runs the cron has none of it. Every verification I performed was true and none of them was the question — a Fixed in
|
Second review round found 15 defects in the precise detector. This
replaces it with the blunt one.
WHAT IT DOES NOW
delete_branch_on_merge is on, so a branch normally disappears when its
PR merges. So: for each remote branch, is there an open PR? If not, say
so. No SHAs, no ancestry, no fetch, no merged-PR lookup, no rev-list.
WHAT THAT DELETES, AND WHY IT IS THE POINT
Nine of the fifteen findings had nowhere left to live: the name-matched
merged-PR lookup (names are reused here); `--limit 1` ordering by
createdAt rather than mergedAt; `git fetch <name>` resolving refs/tags/
before refs/heads/; `merge-base` exit 128 for a missing object booked as
a DEFINITE ANSWER — C-347 inside the accounting added to fix C-347; two
sed parsers depending on gh's compact JSON; and the latch, where the
recovery the issue body printed could never clear its own alert, so the
cron would have gone permanently red and the shared issue permanently
open.
Four hook versions and two detector versions, each defeated by a
different property of git, gh, or GitHub. Precision here requires
inferring state we cannot see, and every attempt to infer it acquires a
new special case. This version cannot be wrong about what it observed
because it observes almost nothing.
A branch pushed before its PR is opened gets reported. That is not
really a false positive — it is a branch with work on it and no route to
development.
FIXED RATHER THAN DROPPED
three-valued a PARTIAL scan writes `partial`, not `false`. `false` is
what closes the tracking issue, so a degraded scan
writing it would close a live orphan issue on the
strength of a scan that skipped that very branch
finding wins `found` is tested before `partial`, or a real orphan
discovered during a degraded scan is never reported
reuse path now renders the orphan section. It previously commented
only "diverged=false, gates_failed=false" — on a red
run, naming no branch, so the one actionable fact never
reached the reader
title no longer asserts "deploy gates are failing" when only
the orphan check fired; old titles kept in the close
loop so no issue is stranded
recovery `gh pr create` or `git push --delete`, either of which
actually clears the check
C-350 — IT WILL NOT RUN UNTIL PROMOTED
GitHub fires `schedule` from the default branch only; that is main, and
all work goes through development. Documented in the guide and the
register with the workflow_dispatch fallback, rather than left as the
claim that it "checks daily".
Guards updated and drilled against the exact defeats: collapse `partial`
into `false`, revoke the permission.
validate_docs PASSED | ruff clean | PYTEST_EXIT=0 | FAILED=0
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Simplified to one question —
|
Third review round. Corrections only — no behaviour change. At the ritual's 3-iteration cap; remaining findings are reported, not fixed. "BOTH RECOVERED BY CHERRY-PICK INSIDE THE HOUR" IS FALSE The sole quantitative basis for abandoning the pre-push hook, cited to Beck, repeated in the register, the changelog twice, and the guide. Measured: #416 merged 2026-08-03T11:24:10Z #417 opened 2026-08-04T01:08:49Z -> 13h 44m, not "inside the hour" #438 CLOSED, never merged -> that recovery never landed It cuts both ways and both are now stated: 14 hours to notice makes a daily detector comparable rather than clearly worse, AND it removes the "cheap to recover" premise the abandonment argument leaned on. C-351's DATES WERE WRONG, AND THE ROOT CAUSE WITH THEM Said "failed since at least 2026-08-08 — dead for five days", implying a regression. `gh run list --workflow=serving-freshness.yml` returns 10 runs, 10 failures, ZERO successes, earliest 2026-08-03 — the day it was added. It did not regress: it shipped broken and was never verified, which makes it C-350's sibling rather than C-338's. THE DOCUMENTED WORKAROUND FOR C-350 DID NOT WORK `gh workflow run release-topology.yml` with no --ref dispatches the DEFAULT branch's copy — main, which has no detector — and the run goes green having executed none of it, which reads exactly like a clean result. Both of my own dispatches used --ref, so I never exercised the form I wrote down. Now `--ref development`, with the trap spelled out. THE CHANGELOG'S "WHAT IS NOT FIXED" LISTED NINE DEFECTS THAT NO LONGER EXIST 3e1fa37 deleted the code seven of them lived in; two were fixed. Left standing with its correction rather than rewritten, because #428 closes the epic from this record and would otherwise schedule work against defects nobody can reproduce (C-336). validate_docs PASSED | ruff clean | PYTEST_EXIT=0 | FAILED=0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Closing unmerged, deliberately. Cost/benefit, stated plainly. C-340 mechanism 2 is: a commit lands on a branch whose PR already merged. It has happened twice in ~440 pull requests, and both times the work was recoverable. Against that: 3 pull requests, 7 guard versions (4 hook + 3 detector), 3 review rounds finding 5 → 15 → 15 defects, and 5 new register concerns — none of which touched the product. This PR's own register entry contains the sentence "the guard had already cost more than the failures", written before three more attempts at the guard. The detector is not worth what it costs to make correct. Round three found seven defects in the test guards written to prevent exactly the defect class the guards were for. That is the signal to stop, not to iterate. What survives, in a separate minimal PR: the broken v1 pre-push hook is still on C-340 mechanism 1 remains solved — Superseded work, for the record: #437 (merged, shipped the broken hook), #438 (closed), this. |
1 participant
Epic #421 Story 5 (#426). Supersedes #438 (closed unmerged). The deliverable is a removal.
What this does
scripts/git-hooks/pre-pushandtests/test_git_hooks.pyscripts/arm_automerge.sh— mechanism 1 works and was drilled liverelease-topology.yml(daily cron, folded into the existing single reusable issue)Why the hook was abandoned
Four versions, four different defeats:
git rev-parse HEAD+ branch namedocs/roadmap-plan-v11spans #50–54)developmenttodayremote_shaequals the merged headdelete_branch_on_mergeremoves the branch first, so git reports0000…. Proved with a two-clone experimentThe test suite was vacuous: reconstructing v1 and running all seven behavioural tests passed every one.
It recurred while being fixed. #437 auto-merged carrying broken v1; the review fixes were pushed to that branch afterwards and orphaned — the same defect inside the PR addressing it. The hook was not installed in that clone, because
core.hooksPathis per-clone config git does not version. That is itself the argument against an install-it-yourself guard.Four failures from four different environment properties is the signature of inferring state you cannot see. A multi-expert panel converged independently: a client check races an asynchronous deletion (a property, not a bug); the module is shallow and its complexity is entirely special cases; the failure occurred twice in ~440 PRs, both recovered by cherry-pick inside the hour. Their verdict on the warning variant was sharpest — a notice printed on every push goes invisible in a week, which is the exact fails-green shape this epic exists to remove.
Why detection works where prevention did not
The framing was the error. The orphaned state is unambiguous once things settle: a remote branch with commits beyond its merged PR's head and no open PR. Two questions, no ancestry subtleties, no merge-method dependence, nothing to install, and it cannot block anyone's work.
Measured against the merged PR's head, not
development— merges here are squashes, so a branch's own commits are never ancestors ofdevelopmentand the naive comparison would flag every merged branch.Drill
End-to-end against live state, with the body extracted verbatim from the workflow rather than retyped:
ops/pre-push-hook-and-automerge — 1 commit(s) pushed AFTER PR #437 merged, no open PRfix/pre-push-hook-review-findings(PR closed-unmerged)orphans=falseafter both stale branches were deleted from the remoteResidue — #428 must say this
Detection is not prevention, and nothing forces
arm_automerge.sh:gh pr merge --autois one keystroke away. C-340 stays open.Gates
validate_docs.shPASSED ·ruff check .All checks passed ·PYTEST_EXIT=0·FAILED: 0🤖 Generated with Claude Code