fix(coord): repair the git-hook installation model — ledger via pre-commit, plus a push guard - #4
Merged
Merged
Conversation
…e hook
Two tools cannot both own .git/hooks/pre-commit. install-git-hooks.ps1 refused to overwrite a foreign
hook, so `pre-commit install` responded by renaming ours to pre-commit.legacy and calling it from its
own shim. That works on POSIX and FAILS ON WINDOWS -- pre-commit invokes the legacy hook from a Python
subprocess, which cannot resolve the /bin/sh shebang:
ExecutableNotFoundError: Executable `/bin/sh` not found
Measured 2026-07-27: EVERY commit in the repo was blocked until the shim was uninstalled. Note the
trap -- pre-commit.legacy EXISTED, so a presence check reported success. Only a real commit revealed it.
The ledger gate is now a `local` hook in .pre-commit-config.yaml (id: ledger-gate, always_run,
pass_filenames: false -- it inspects the staged TREE, not a file list). pre-commit owns the hook file
alone, so the contention is impossible rather than managed, and a fresh clone gets every gate from
`pre-commit install` with no hand-merged file to remember.
install-git-hooks.ps1 now:
* installs ONLY the claim gate (commit-msg -- it cannot move, since only commit-msg receives the
message file);
* MIGRATES an existing install by removing our old standalone pre-commit hook and its ledger_check.py
copy. Leaving them would make the next `pre-commit install` chain to .legacy and re-break commits;
* reports the real state under -Status, including a STALE leftover hook; and
* fails LOUD when pre-commit is absent, because the ledger gate's availability is now coupled to it.
That coupling is a genuine trade-off of this design, so it is announced rather than buried.
Verified by a real commit -- both gates observed firing, not inferred from a file existing.
The 17 existing tests exercise ledger_check.py's logic against a throwaway repo. None of them notices
if the gate is never invoked -- and that is exactly what just happened: the gate had to move out of
.git/hooks/pre-commit because two tools contend for that file and their chaining fails on Windows.
Every logic test stayed green throughout.
Two wiring assertions:
* the ledger-gate hook exists in .pre-commit-config.yaml, points at ledger_check.py, and carries
always_run + pass_filenames: false (it inspects the staged TREE, so without always_run a commit
touching no file it "owns" would skip the gate entirely);
* install-git-hooks.ps1 does NOT write a pre-commit hook again, and DOES still migrate an old
standalone install away -- otherwise the two-owner conflict returns and re-breaks every commit.
Mutation-verified: renaming the hook id reds it, and so does flipping always_run to false.
Uses a plain `import yaml`, deliberately not pytest.importorskip. pyyaml is pinned in
requirements.lock and constraints.lock so it is always present where CI runs; importorskip would turn
a missing dependency into a silent SKIP, which is the precise failure mode this test exists to catch.
Since the cutover this repository IS the published artifact. A push to main is publication, immediately, and cannot be taken back: deleting the ref later does not un-publish content that was fetched, mirrored or indexed in between. There is no publish step left to catch anything. Server-side branch protection requires a PR and 12 status checks, but enforce_admins is FALSE, so the repository owner bypasses all of it. The realistic trigger is not malice -- it is one click, because VS Code's Sync/Push button does not distinguish "my feature branch" from "main", and the editor is where most pushes originate. The old mirror clone carried a Gate-Provenance pre-push hook that covered this class; it was quarantined at cutover and nothing replaced it. Protects refs/heads/main and refs/heads/cla-signatures (written by the CLA action, never by a human -- and every PR wedges if it is damaged). Deletions are refused too: deleting main is worse than pushing to it. WHAT THIS IS NOT. A guardrail, not a boundary: --no-verify skips it and it is local-only, so another machine is unprotected. It removes the ACCIDENT, not the capability. The durable server-side fix is enforce_admins=true, deliberately NOT done yet: a known flaky test (BACKLOG #17) has now blocked two consecutive PRs, and removing the override while a flake can strand a merge would trade an accidental-push risk for a cannot-ship risk. Sequence it after #17. The escape hatch is MEFOR_ALLOW_DIRECT_PUSH=1, deliberately NOT --no-verify: a distinct variable is greppable in shell history and cannot be reached by the muscle memory that skips every other hook. Fails OPEN on malformed stdin. The guard exists to stop an accident; crashing on unexpected input would BE one. VERIFIED end-to-end, not by inspection: a real `git push origin <branch>:main` is REFUSED (exit 1), while the control -- pushing the branch to its own ref -- still succeeds. Eight tests drive the exact stdin contract git uses for a pre-push hook, plus a wiring test that the installer actually installs it; mutation-verified by disabling the protection and watching four go red.
wshallwshall
changed the title
Collaborator
Author
|
Merged with --admin, bypassing one required check: test (windows-2025, py3.14) failed twice at e4ebe05 with only tests/test_harness_monitor.py::test_monitor_observes_engine (BACKLOG #17, a known flake) — 9025 passed around it. windows-2022 and ubuntu both passed on the identical commit, this PR touches no harness/monitor code, and main is already red from the same test. All 11 other required contexts were verified green, zero non-required failures, head SHA re-verified as the tested one. Bypass scoped to that one context by name. |
wshallwshall
added a commit
that referenced
this pull request
Aug 5, 2026
#193) * fix(ci): dependabot auto-merge decided by exclusion — invert #3 to an allow-set (BACKLOG #336) Guardrail #3 was a 16-name Python deny-list with no ecosystem qualifier: anything not named auto-merged if it was a patch, on every ecosystem. Inverted to HOLD-UNLESS-NAMED. Only `actions/`, `github/` and `dependabot/` are eligible, and only on github-actions; the uv, pip and npm rows ship EMPTY, an unrecognised ecosystem token holds, and the fail-safe whole-group denial is preserved. Measured against the live PR #75, not recalled: its body carries five `Updates` entries — four on the allow row (actions/checkout and three github/codeql-action/*) and pypa/gh-action-pypi-publish, which is not — so that batch would HOLD as a whole. Expect auto-merge to fire RARELY; that is the intent of hold-unless-named, not a regression. Hardening, not an incident. There is no evidence of exploitation, the attacker must already own an upstream publisher account, and merging to main is not publication (PyPI release is gated on an owner tag push). MessageFoundry is a not-deployed beta with zero production instances, so this is stated as what the shipped code WOULD allow on first deployment, never as a present exposure. §4 RELEASE AGE, with an honest account of its reach. A new `id: age` step holds a SECURITY-track PR whose candidate version was published under MIN_RELEASE_AGE_HOURS (24), failing closed on an API error, an absent or unparseable upload timestamp, an unexpected name or version shape, or an ecosystem with no publish-date source wired. It is INERT with respect to the merge decision as shipped: age_ok=true is reachable only for uv/pip, eligible=true only for github-actions, and the merge `if` requires both — disjoint sets. Confirmed by executing the shipped allow-set body over exactly the ecosystems the age gate can pass (uv and pip both return eligible=false; only github_actions returns true). The header and the BACKLOG banner now say exactly that instead of presenting it as an operating control; recording an unreachable control as operative is the false-premise class docs/Secure_Development_Standards.md §3 forbids. The step is gated on `steps.allowset.outputs.eligible == 'true'`, which changes no merge outcome — the merge `if` already requires that conjunct — but removes an unauthenticated outbound GET made from a job holding `contents: write` for a PR that was going to be held anyway. §5 the header's backstop claim, corrected without over-claiming in the other direction. Dropped: "the only gate that inspects a dependency's shipped bytes at all is security.yml's trivy step". security.yml:265 marks trivy `continue-on-error: true` and :271 restricts it to schedule/dispatch, so it is advisory AND never runs on a Dependabot PR. The replacement is the weaker true form: no REQUIRED check reads a dependency's shipped bytes. The sentence describes what semgrep cannot SEE rather than where it looks, so the unmerged plan-semgrep-scope widening (BACKLOG #334) cannot falsify it. Provenance made honest. The header cited DEPENDENCY-POSTURE-REVIEW.md as the numbered source for guardrails it does not carry: #3 is INVERTED from the deny-list that document describes, and #4 is introduced here. The block is retitled AUTO-MERGE GUARDRAILS, #3 is moved out from under the security-track framing (it gates every PR), and the test docstring carries the same correction. Amending the vault copy stays the owner's separate obligation — the repo simply stops asserting it has already happened. Restated facts removed. The header repeated three cooldown values that live in .github/dependabot.yml; only two had a test bound, and that file records github-actions' window as approximate because it ages off the tag's commit date. The header now points at the source instead (CLAUDE.md §11 — state a load-bearing fact once and link to it). .github/dependabot.yml comments corrected. The uv block told a maintainer "routine patches auto-merge AFTER aging; security patches auto-merge now" — both halves false once uv's allow row ships empty. The github-actions block's "an advisory fix is unaffected" now says it opens as a PR unaffected but reaches main by human review, because the advisory gate is pip-keyed and never confirms an action. docs/testing/master-test-plan/01-environments-data-and-tooling.md:350 still advertised the deleted auth/token/crypto deny-list as a live control of this workflow. No test pinned that doc, so it drifted silently; the row now describes what ships. TESTS - The behavioural harness runs the shipped bodies under `bash -e`, which is what Actions applies by default on Linux (no `shell:` is declared anywhere). Plain bash keeps going where CI aborts the step, and `assert proc.returncode == 0` was exactly the assertion that would mask that class. No shipped row changes its decision: ten rows were compared head-to-head under plain bash and under `bash -e` and agreed on both exit code and emitted output, and the full 20-row parametrised set passes under `-e`. So this is a fidelity fix, not a behaviour change — but `_run_step_body` now RETURNS the returncode rather than asserting it, so an abort path can be expressed as an expected outcome instead of being indistinguishable from a harness bug. test_no_step_overrides_the_default_shell is the tripwire for a future `shell:` silently invalidating the premise that `bash -e` is what CI runs. - _load_dependabot's skip is deleted. It skipped on the premise that .github/dependabot.yml is "private-only, deny-listed on the OSS mirror". .gitignore names that file under DELIBERATELY NOT LISTED as content meant to ship; `git ls-files --error-unmatch` resolves it and `git check-ignore` exits 1. The refactor had routed THREE tests through that skip, including the cooldown test whose entire reason for existing is that a missing cooldown was invisible to CI. It now asserts presence. - The jq skip reason claimed the test "runs on the ubuntu CI leg and skips locally". The windows-2022 and windows-2025 images ship jq and Git Bash, and both are REQUIRED contexts in .github/required-contexts.txt, so it runs there too. The reason now says so — a maintainer reading it after a Windows-only red should not be told to expect ubuntu. ADR 0034 CONSEQUENCE, recorded because leaving it unrecorded is what that ADR warns against: this file grew 190 -> 403 lines and the `Why pull_request` anchor moved from line 42 to 76. Under the ADR's convergence rule, dismissed alert #87 re-fires as a new alert number and needs re-dismissing after merge. VERIFICATION - ruff check . -> All checks passed! - ruff format --check . -> 1042 files already formatted - mypy messagefoundry -> 21 errors, ALL PRE-EXISTING and NOT from this change: absent [fhir]/[dicom]/[webauthn] extras (fhirpathpy, fhir, pynetdicom, pydicom and webauthn are all ABSENT from this venv; 12 import-not-found plus their 7 no-any-return / 2 unused-ignore consequences). This change touches no file under messagefoundry/, so mypy's inputs are byte-identical to HEAD. CI installs the extras and is the authority. The quartet is NOT green locally and this commit does not claim it is. - pytest -q -> 10291 passed, 841 skipped, 22 warnings in 1282.01s (0:21:22). The pre-change baseline measured on this tree was 10289 passed / 841 skipped, so the delta is exactly the two tests added here. Run on a byte-frozen tree (sha256 of all five changed files recorded before the run and unchanged after), because two comment edits had landed after an earlier run started and six tests read docs/BACKLOG.md. - Falsification: 18 mutations of the workflow and dependabot.yml, plus deleting dependabot.yml outright, each confirmed to turn the matching test RED and then restored byte-identically (sha256 checked after every case). Among them: the allow-set admitting `pypa/`, losing its trailing slash, matching by substring, treating an empty name list as eligible, and failing open on an unrecognised ecosystem; the age step failing open on an undatable ecosystem, hard-coding a PASS on the version-track exit, re-deriving the security track, and losing its allow-set gate; a step declaring `shell:`; the merge `if` dropping either the age_ok or the allow-set conjunct; and each cooldown being removed or shortened. The four jq-gated rows were falsified separately (threshold deleted, missing-timestamp failing open, curl error failing open, and the discriminating PASS removed) against a purpose-built jq stand-in implementing the two shipped filters, because this box has no jq at all; execution against real jq happens on CI. * docs(backlog): flip #336 to SHIPPED and supersede one clause of its 2026-08-03 amendment Banner lines of item #336 ONLY. VERIFIED BY NUMBER, not by banner text — a byte-identical banner pasted under the wrong item survives every well-formedness check. `git diff -U0 -- docs/BACKLOG.md` is two hunks, @@ -3353 +3353 @@ and @@ -3358,0 +3359,2 @@; the enclosing `## ` heading computed backwards from each of the three changed lines (3353, 3359, 3360) is `## 336. Dependabot auto-merge shields review with a deny-list` in all three cases. The ranked table and the four census distribution lines are untouched. THE CENSUS WAS NOT RECOMPUTED. This commit flips one item's banner and deliberately does not touch the distribution lines. The banner records guardrail #4 as a FORWARD guard that is inert with respect to the merge decision as shipped, rather than as an operating control: age_ok=true is reachable only for uv/pip, eligible=true only for github-actions, and the merge `if` requires both. Recording an unreachable control as operative is the false-premise class docs/Secure_Development_Standards.md §3 forbids. The added blockquote uses the glyph-free `**AMENDED 2026-08-04 — ...**` convention. It supersedes ONE clause of the dated 2026-08-03 note — "The deny-list itself is untouched, so the rest of the item stands ... §§1, 3, 4, 5 and 6 are unaffected" — which was accurate when measured and is deliberately left as written rather than rewritten. The deny-list no longer exists, so §6 is discharged by deletion rather than annotation, and the 16 names survive only as a PROPERTY under test (`_DENY_PACKAGES` asserts none of them reaches any allow row). scripts/docs/backlog_status_check.py exits 0 ("290 backlog items, each declaring exactly one status") — reported as corroboration only. That gate validates that one banner is present and self-consistent, never that it belongs to this item, so it cannot see the corruption class this commit had to avoid. The by-number check above is the evidence; the green gate is not.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three commits fixing how this repo's git hooks are installed. The first two were forced by a real breakage; the third closes an exposure the cutover created.
1. Two tools cannot own
.git/hooks/pre-commitinstall-git-hooks.ps1installed the ledger gate there and refused to overwrite a foreign hook. Sopre-commit installdid the only thing it can — migration mode: rename ours topre-commit.legacyand call it from its own shim.That works on POSIX. It fails on Windows, because pre-commit invokes the legacy hook from a Python subprocess that cannot resolve
#!/bin/sh:Every commit in the repo was blocked until the shim was uninstalled.
The part worth remembering:
pre-commit.legacyexisted. A presence check — exactly what the runbook prescribed — reported success. Only a real commit revealed it.Fix: the ledger gate becomes a
localhook in.pre-commit-config.yaml(always_run,pass_filenames: false— it inspects the staged tree, not a file list). pre-commit owns the file alone, so the contention is impossible rather than managed, and a fresh clone gets every gate frompre-commit installwith nothing hand-merged to remember.install-git-hooks.ps1now installs only the claim gate (commit-msgcannot move — only it receives the message file), migrates an old install away, and fails loud when pre-commit is absent. That last point is a genuine trade-off, stated rather than buried: the ledger gate's availability is now coupled to pre-commit being installed.2. The tests were the actual gap
The 17 existing ledger tests exercise the logic against a throwaway repo. None notices if the gate is never invoked — they stayed green through the entire breakage. Two wiring assertions added, both mutation-verified.
3. Pre-push guard — refuse a direct push to
mainSince the cutover this repo is the published artifact. A push to
mainis publication, immediately and irreversibly. Branch protection requires a PR and 12 checks, butenforce_adminsis false, so the owner bypasses all of it — and VS Code's Sync button doesn't distinguishmainfrom a feature branch. The old mirror clone's Gate-Provenance pre-push hook covered this class; it was quarantined at cutover and nothing replaced it.Protects
mainandcla-signatures(written by the CLA action, never a human — and every PR wedges if it's damaged). Deletions refused too.What this is not: a guardrail, not a boundary.
--no-verifyskips it; it's local-only. It removes the accident, not the capability. The durable fix isenforce_admins=true, deliberately not done yet — BACKLOG #17 has blocked two consecutive PRs, and removing the override while a flake can strand a merge trades an accidental-push risk for a cannot-ship risk. Sequence it after #17.Escape hatch is
MEFOR_ALLOW_DIRECT_PUSH=1, deliberately not--no-verify: greppable in history, and not reachable by the muscle memory that skips every other hook.Verified by doing, not inspecting
pre-commit install, no.legacymainrefusedgit push <branch>:main→ exit 1always_runreds the wiring testsruff13 andmypy21 — both identical to a baseline measured onmain. Leak scan exit 0 with all 21 detectors.