diff --git a/.github/workflows/release-topology.yml b/.github/workflows/release-topology.yml index 59b981a..2d43ebc 100644 --- a/.github/workflows/release-topology.yml +++ b/.github/workflows/release-topology.yml @@ -48,6 +48,16 @@ on: permissions: contents: read issues: write + # `pull-requests: read` is load-bearing, not tidiness. Once a `permissions:` + # block exists, every scope NOT listed is set to `none` — so omitting this + # revokes it. The orphan detector below is built entirely on `gh pr list`, + # and its calls are written `... 2>/dev/null) || continue`, which makes a + # 403 indistinguishable from "no PR found": every branch would be skipped + # and the step would report "No orphaned branches" forever, green, on a + # daily cron. It was nearly shipped that way — the drill that verified the + # detector ran under the operator's own `gh` credentials, which carry full + # scope, so it could not have caught this (C-347). + pull-requests: read concurrency: group: release-topology @@ -129,6 +139,119 @@ jobs: # TestF7ProductPlanCurrency reads only files and already runs in the # normal PR suite; it needs no scheduled runner. + - name: Look for a branch that outlived its pull request + id: orphans + # C-340 mechanism 2. A PR auto-merges the instant CI goes green; a + # follow-up commit is then pushed to a branch that no longer has an + # open PR, and the work is simply not on the base branch. `git push` + # reports success. It happened on #416 and again on #437. + # + # THIS ASKS ONE QUESTION, AND THAT IS THE WHOLE DESIGN. + # `delete_branch_on_merge` is on, so GitHub removes a branch when its + # PR merges. A remote branch that still exists and has NO OPEN PR is + # therefore already anomalous, whatever the reason. Report it; let a + # human look. + # + # An earlier version tried to establish that work was *definitely* + # orphaned — matching the merged PR, comparing its head SHA, counting + # commits beyond it. Two review rounds found fifteen defects in that + # version, each from a different property of git, `gh`, or GitHub: + # branch names are reused so the name match was wrong; `--limit 1` + # orders by createdAt not mergedAt; `git fetch ` resolves + # `refs/tags/` before `refs/heads/`; `merge-base` exits 128 for a + # missing object and that was booked as a definite answer; and the + # recovery it printed could never clear its own alert. Four versions + # of a pre-push hook died the same way before it. The lesson is not + # any one defect: **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 is reported. That is a + # false positive by the strict definition and not one in practice — + # it is a branch with work on it and no route to `development`, which + # is the thing worth knowing. + # + # Needs `pull-requests: read` — see the permissions block. + env: + GH_TOKEN: ${{ github.token }} + run: | + set -uo pipefail + + # Capture and check, rather than iterating the substitution directly. + # `for x in $(cmd)` swallows a failure of `cmd` completely: GitHub + # runs `bash -e {0}` and `set -uo pipefail` does NOT remove `-e`, but + # a failing command substitution in a `for` word list is exempt from + # it. Verified: `bash -e -c 'for x in $(false | sed s/a/b/); do echo + # BODY; done; echo END'` prints only END. One transient `ls-remote` + # failure would give an empty loop and a confident all-clear. + branches=$(git ls-remote --heads origin | sed 's|.*refs/heads/||') || { + echo "::error::git ls-remote failed — cannot enumerate branches." + exit 1 + } + # A repository always has at least `main` and `development`. Empty + # here means the command succeeded and told us nothing, which is not + # the same as "no branches" and must not read as a clean result. + [ -n "$branches" ] || { + echo "::error::git ls-remote returned no branches at all." + exit 1 + } + + # "Could not ask" is NOT "asked and it is fine". A rate limit or a + # 502 removes a branch from the check silently, and if enough fire + # the step reports a confident all-clear having observed nothing. + found="" + answered=0 + unanswered=0 + for branch in $branches; do + case "$branch" in main|development) continue ;; esac + + open=$(gh pr list --head "$branch" --state open --json number \ + --jq '.[0].number // empty' 2>/dev/null) || { + unanswered=$((unanswered + 1)); continue; } + answered=$((answered + 1)) + [ -n "$open" ] && continue + + found="${found} + - \`${branch}\`" + done + + echo "Checked ${answered} branch(es); ${unanswered} could not be answered for." + + # Answering for NOTHING while reporting clean is the defect itself, + # not a degraded mode of it. + if [ "$answered" -eq 0 ] && [ "$unanswered" -gt 0 ]; then + echo "::error::Could not answer for any of ${unanswered} branch(es) — reporting clean here would assert nothing." + exit 1 + fi + + # A partial scan must not write an unqualified all-clear: the close + # step keys on this output, and would close a live orphan issue on + # the strength of a scan that skipped the branch it was about. + [ "$unanswered" -gt 0 ] && echo "::warning::${unanswered} branch(es) could not be checked; this result is partial." + if [ -n "$found" ]; then + # A finding is a finding even from a degraded scan. + echo "orphans=true" >> "$GITHUB_OUTPUT" + elif [ "$unanswered" -gt 0 ]; then + # Nothing found, but we did not look everywhere. NOT 'false': + # the close step keys on 'false' and would close a live orphan + # issue on the strength of a scan that skipped its branch. + echo "orphans=partial" >> "$GITHUB_OUTPUT" + else + echo "orphans=false" >> "$GITHUB_OUTPUT" + echo "No branches without an open pull request." + fi + + if [ -n "$found" ]; then + DELIM="ORPHANS_$(openssl rand -hex 8)" + { + echo "list<<${DELIM}" + printf '%s\n' "$found" + echo "${DELIM}" + } >> "$GITHUB_OUTPUT" + echo "::warning::A remote branch has no open pull request." + fi + - name: Summarise the gate result id: gateresult run: | @@ -153,9 +276,11 @@ jobs: fi - name: Open or update the tracking issue - if: steps.check.outputs.diverged == 'true' || steps.gateresult.outputs.failed == 'true' + if: steps.check.outputs.diverged == 'true' || steps.gateresult.outputs.failed == 'true' || steps.orphans.outputs.orphans == 'true' env: GH_TOKEN: ${{ github.token }} + ORPHANS: ${{ steps.orphans.outputs.orphans }} + ORPHAN_LIST: ${{ steps.orphans.outputs.list }} DIVERGED: ${{ steps.check.outputs.diverged }} COMMITS: ${{ steps.check.outputs.commits }} SHAS: ${{ steps.check.outputs.shas }} @@ -211,10 +336,39 @@ jobs: ) fi set -euo pipefail - TITLE="Release hygiene: one or more deploy gates are failing" + TITLE="Release hygiene: one or more checks are failing" + ORPHAN_SECTION="" + if [ "${ORPHANS:-false}" = "true" ]; then + ORPHAN_SECTION=$(cat < + \`\`\` + \`\`\`bash + git push origin --delete + \`\`\` + + If the branch is already gone but its work never reached \`development\`, recover it the + way #417 did — cherry-pick onto a new branch and open a pull request. + ORPH + ) + fi BODY=$(cat </protectio --- -## One-time setup — the pre-push hook +## Work orphaned on a merged branch — detected, not prevented + +A pull request auto-merges the instant CI goes green. Push a follow-up commit and it lands on a +branch with no open PR: `git push` reports success and the work is simply not on `development`. +That is C-340 mechanism 2, and it has happened twice — #416, and again on #437 while #437 was +fixing it. + +**There is no client-side guard, deliberately.** A pre-push hook was written four times and +abandoned; each version was defeated by a different property of the environment. A client check +races GitHub's asynchronous branch deletion, which is a property of the system rather than a bug to +iterate out. The register entry for C-340 records all four so nobody rebuilds them. + +**It is not yet running.** GitHub executes a `schedule` trigger from the **default branch only**, and +this repository's default branch is `main`. The check lands on `development`, so the 06:00 cron goes +on running `main`'s copy until the next release promotion carries it across (C-350). Until then, +trigger it by hand: ```bash -git config core.hooksPath scripts/git-hooks +gh workflow run release-topology.yml --ref development ``` -Run this once per clone. `core.hooksPath` is per-clone config that git does **not** version, so a -fresh clone has no hooks until you set it. +**`--ref` is not optional here.** Without it `gh` 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. + +**What it checks, once live.** `delete_branch_on_merge` is on, so a branch normally disappears when +its pull request merges. `release-topology.yml` therefore asks one question of every remote branch: +**is there an open pull request for it?** If not, it says so, and folds the branch into the same +tracking issue as the other release-hygiene checks. + +That is deliberately blunter than "is this work orphaned". A first version tried to establish +orphan-ness properly — matching the merged pull request, comparing head SHAs, counting commits +beyond it — and two review rounds found fifteen defects in it, each from a different property of +git, `gh`, or GitHub. Precision there requires inferring state we cannot see. This version reports a +branch you pushed before opening its PR, which is not really a false positive: it is a branch with +work on it and no route to `development`. + +**Clearing it takes one command, either way:** -`scripts/git-hooks/pre-push` **refuses a push to a branch whose pull request has already merged.** -That is C-340 mechanism 2, and it happened: #416 merged the instant CI went green, a follow-up -commit was pushed to that branch, and two pieces of work were simply not on `development`. -`git push` reported success. The only signal was a PR showing one commit when two had been pushed. +```bash +gh pr create --base development --head +``` + +```bash +git push origin --delete +``` -It **allows** the push whenever it cannot answer — `gh` absent, unauthenticated, or offline — and -says why. A hook that blocks work when it does not know gets uninstalled within a day, and then it -guards nothing. Bypass with `git push --no-verify`. +If a branch is already gone but its work never reached `development`, recover it the way #417 did — +cherry-pick onto a new branch and open a pull request. twice in ~440 PRs — and the recovery figure this entry first cited was wrong. **Measured:** #416 merged 2026-08-03T11:24:10Z and its recovery #417 was not opened until 2026-08-04T01:08:49Z, **13h 44m later**; #437's recovery #438 was closed unmerged and that work never landed at all (it became moot when the hook was deleted). The claim *"both recovered by cherry-pick inside the hour"* was repeated in this register, the changelog and the guide, and it was the only quantitative basis offered for abandoning the guard. Corrected 2026-08-12 by `/code-review max`. It cuts both ways and both are worth stating: 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. ## Arming auto-merge — use the script, not `gh pr merge` diff --git a/reports/register_changelog.md b/reports/register_changelog.md index 30e295f..65a1300 100644 --- a/reports/register_changelog.md +++ b/reports/register_changelog.md @@ -17,6 +17,220 @@ entry could. --- +## `/code-review max` — the detector was never going to run, and three of my own corrections were wrong (2026-08-12) + +Second review round on #439. Fifteen findings, verified empirically rather than argued: the shell +step was extracted from the YAML and executed under `bash -e` against synthetic repositories 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**. This repository's default branch is `main`; all work goes through `development`. So the +detector merges to `development` and the 06:00 cron keeps executing `main`'s copy, which contains 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, this changelog and C-340's narrowing all stated *"checks daily"* as present +fact, and the four new guards assert properties of the **branch's** file, so they go green on every +pull request 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 ever 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` step, so a git command aborts with `fatal: not +a git repository`. Freshness alerting for the served artefacts has been dead for five days. Recorded +immediately rather than as a review aside, because *"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; and a bare +`"::error::" in run` searching a 130-line body was satisfied by an unrelated guard a hundred lines +below, so deleting *both* branch-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:` rather than by a substring of the +body under test — then drilled against the exact three defeats the review demonstrated. + +Rewriting them produced a fourth instance of the same thing: 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 now stripped before matching — which is what +`test_heartbeat_secret.py` already learned, in this same epic. + +**The C-345 addendum written this morning was wrong, and is retracted in place.** It claimed pytest +colourises its summary so `grep -cE '^FAILED'` cannot match. pytest does **not** colourise into a +file; the ANSI codes came from `FORCE_COLOR=3` exported in this operator's shell. Measured both +ways: with it, `grep -cE '^FAILED'` returns 0 on a failing run; with `env -u FORCE_COLOR`, it +returns 1 and there are zero ANSI lines. **C-345's original prescription was fine in a plain shell +and in CI.** An anomaly produced by an uncontrolled environment variable was diagnosed as a defect +in the tool and escalated into a general claim about this register — *"the second time an entry has +prescribed a defective remedy"* — which is withdrawn. That is C-347, committed inside the commit +registering C-347. `--color=no` stays, because removing a dependency on the caller's environment is +right for the reason the wrong diagnosis inverted. + +**C-349 undercounted its own locations.** It named three `JSONDecodeError` sites; `grep -rn +JSONDecodeError src/` returns seven, and `digests_and_ledgers.py` — the provenance package's own +ledger reader — **already logs the skip**, falsifying the entry's central *"nothing anywhere records +that it happened"*. An entry that undercounts gets closed after a partial fix. Corrected, with the +grep written into the Location field so the count is re-derivable rather than asserted. + +**What was not fixed at that point** — nine verified behavioural findings in the *precise* detector: +the printed recovery never cleared the alert; a partial scan wrote an unqualified `orphans=false` +that auto-closed a genuine orphan issue; the reuse path never rendered the orphan section; a +`merge-base` exit 128 was booked as a definite answer; two `sed` parsers depended on `gh`'s compact +JSON; `--limit 1` ordered by `createdAt`; and a bare branch name in `git fetch` resolved `refs/tags/` +first. + +**Superseded by `3e1fa37`, which deleted the code they lived in.** Seven of the nine have no +referent in the shipped workflow — grep the executable lines for `merge-base`, `--limit 1`, +`git fetch`, `rev-list` or `sed -n` and you get nothing; those tokens survive only in comments +describing the abandoned version. The remaining two were fixed: the detector now writes a third +value, `partial`, rather than an unqualified `false`, and the reuse path renders the orphan section. +**This paragraph is left standing with its correction rather than rewritten**, because #428 is +chartered to close the epic from this record and would otherwise schedule work against nine defects +nobody can reproduce — which is how a changelog stops being checkable (C-336). + +**The pattern is the finding.** Round one: five defects. Round two: fifteen. Round three, against the +*simplified* version: fifteen again, **seven of them in the new test guards themselves** — five +assertions defeated by one-line mutations that keep the suite green. That is the epic's own subject, +committed inside it, twice. + +--- + +## C-347, C-348 registered and C-345 corrected — the detector arrived with three fails-green defects (2026-08-12) + +Epic #421 Story 5 (#439), the `/code-review` round on the fix for C-340. **The cluster grew out of an +attempt to shrink it.** That is the honest result and Story 7 (#428) has to report it. + +**Three defects in the replacement, none of which would have reddened anything.** The orphan +detector was drilled end-to-end the night before and reported working. Review found: (1) the +workflow's `permissions:` block never granted `pull-requests`, and an explicit block sets every +unlisted scope to `none` — so both `gh pr list` calls would 403, swallowed by `2>/dev/null || continue`, +and the step would report "No orphaned branches" on a daily cron forever; (2) merged PRs were matched +by **branch name alone**, the same property that defeated hook v1 — and reuse is real here +(`chore/version-bump-1.2.13`, `docs/roadmap-plan-v11`, `feat/acled-phase2` have each headed more than +one PR), with the old head absent from the new branch's ancestry so `git rev-list` errored and was +reported as `? commit(s)`; (3) `for branch in $(git ls-remote ...)` hides a failure of the +substitution completely — GitHub runs `bash -e {0}` and `set -uo pipefail` does not remove `-e`, but 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`. + +**C-347 — why the drill could not have caught any of this.** It ran under the operator's personal `gh` +credentials, which carry full scope; production runs under `GITHUB_TOKEN` with `pull-requests` +revoked. The drill obtained and reported its result *correctly*; it simply ran in the wrong world. +That is not C-345 (an instrument misreading a result it did obtain) and it is not C-336 (a guard whose +claim is narrower than its property) — it is 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` in the wrong tree; and this. **A drill is +an experiment, and it silently inherits every variable you did not control.** + +**C-348 — the deletion took a guard with it.** `tests/test_git_hooks.py` was deleted with the hook, +correctly for the hook's own assertions. It also held the only guard on `scripts/arm_automerge.sh`'s +executable bit — a script the same pull request deliberately *keeps*. A test module is an +organisational unit that quietly doubles as a coverage unit, and only the first is visible when you +delete it. Tier 4: the loss surfaces loudly as `Permission denied`. Registered because two more +deletions are scheduled in this epic. + +**C-345's own prescribed mitigation is defective, and the drill is what proved it.** The entry +recommends `grep -cE '^FAILED' out.txt` as a second independent reader. pytest **colourises** its +summary, so the line is `\033[31mFAILED\033[0m tests/...` and the caret never matches. The harness +built for these drills required both `rc != 0` and `FAILED >= 1`, and reported *"DID NOT CATCH"* four +times while all four drills had in fact caught. Had it trusted only the grep — the reading C-345 +recommends — a red suite would have read green. Corrected to `--color=no` at the source rather than an +ANSI-aware regex, which would only be one more narrow claim. **This is the second time an entry in +this register 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 — worth saying out loud, because the corrections are the +valuable part. + +**All four new guards were drilled against the broken state, with a control.** Revoke the permission, +restore the blind loop, move the fetch step after the detector, strip the executable bit: each +reddened exactly the guard aimed at it, and the fixed tree stayed green. + +**Then `/review-diff` found the same defect one level down, and C-349 came out of generalising it.** +The permission fix removed the *systematic* 403, but every per-branch `|| continue` still converted +"could not check this branch" into "this branch is fine" — a rate limit or a 502 would skip branches +and the step would still print a confident all-clear. The detector now counts what it answered for, +qualifies a partial result, and **exits non-zero if it answered for nothing**: a clean report resting +on zero observations is the defect itself, not a degraded mode of it. + +Grepping for that shape in production code found it three times — `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` twenty lines of a different module away already does it +correctly with `n_skipped_spatial += 1`. Since the consolidators use the ledger to decide what to +consolidate, a dropped line means a successful harvest is silently not consolidated. Registered as +**C-349** and deliberately **not fixed here** — #439 is a CI story, and an untested change to +consolidation does not belong in a pull request about a workflow. + +**The reused-name fix was drilled in a scratch repository**, since git ancestry semantics do not +differ between the operator's machine and the runner (the C-347 caveat is about credentials and +environment, not about git). Squash-merge a branch, push a follow-up to it, then delete the branch +and cut a fresh one reusing the name: the new logic reports the genuine orphan as 1 stranded commit +and **skips** the namesake, where the old logic reported the namesake as 3 commits stranded. + +**The permission fix was then verified the way C-347 says to verify things** — by dispatching the +workflow (run 31590304501) rather than running its body locally. It reported *"Checked 1 branch(es); +0 could not be answered for."* Had the scope still been revoked, `gh pr list` would have 403'd into +`|| continue` and the new zero-answers rule would have turned the run red. Writing C-347 and then +*not* dispatching would have been the entry's own failure mode, committed in the commit that +registers it. + +One guard was also loosened rather than tightened: the ls-remote assertion had matched the exact +wording of a log message, which would redden on a reword for no behavioural reason. It now checks +that a failure path exists at all. Asserting the *how* instead of the *what* is C-336's second +addendum, and it is easy to commit while writing a guard against something else. + +--- + +## C-340 NARROWED — four versions of a guard, abandoned for a detector (2026-08-12) + +Epic #421 Story 5 (#426, #437, #438 closed unmerged, #439). The most instructive entry of the epic, +because the deliverable is a **removal**. + +**Mechanism 1 resolved.** `scripts/arm_automerge.sh` arms via GraphQL disable/enable and reads the +method back. Reproduced live on #437 before shipping: `gh pr merge --auto --squash` against an +already-`MERGE`-armed PR exited **0** and changed nothing. + +**Mechanism 2: four client-side attempts, four different defeats.** v1 read `git rev-parse HEAD` +instead of the refs git supplies on stdin *and* matched on branch name, permanently refusing names +reused from old PRs. v2 used ancestry — defeated because a merge-commit merge puts the head into the +base branch forever. v3 used `remote_sha` — defeated because `delete_branch_on_merge` removes the +branch first, so git reports `0000…`. v4 was never shipped. 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 pull request addressing it. The +hook was not installed in that clone — `core.hooksPath` is per-clone config git does not version, +which is itself the argument against an install-it-yourself guard. + +**Why it was abandoned rather than fixed a fifth time.** Four failures from four *different* +environment properties is the signature of inferring state you cannot see, not of carelessness. A +multi-expert panel converged independently: Kleppmann (a client check races an asynchronous +deletion — a property, not a bug), Ousterhout (a shallow module whose complexity is entirely special +cases; four versions are four attempts to enumerate them), Beck (twice in ~440 PRs, both recovered +by cherry-pick — the guard had already cost more than the failures; **note the "inside the hour" +figure originally cited to Beck here was wrong, see the 2026-08-12 correction: the real gap was +13h 44m and the second incident was never recovered**). Their verdict +on the *warning* variant was the sharpest: a notice printed on every push goes invisible in a week, +which is the exact fails-green shape this epic exists to remove, and shipping it would have created +a precedent to cite later. + +**The framing was the error, and that was the finding.** 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. `release-topology.yml` already had `fetch-depth: 0`, `issues: write`, a daily +cron and one-reusable-issue machinery. + +**Drilled end-to-end against live state**, with the body extracted from the workflow rather than +retyped: it found the genuine orphan (*"1 commit(s) pushed AFTER PR #437 merged, no open PR"*), +correctly ignored a branch whose PR was closed-unmerged, and reported clean once that branch was +deleted. Measured against the merged PR's 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. + +**Left open on the residue**, and #428 must say so: this is detection, not prevention, and nothing +forces `arm_automerge.sh` — `gh pr merge --auto` is one keystroke away. + +--- + ## C-341 RESOLVED by deleting the gate, and C-346 — four more that cannot fail (2026-08-11) Epic #421 Story 4 (#425), which also closes #363. The instruction was "delete F1". It turned out diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index af5b0a2..a47c056 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -1,9 +1,9 @@ # Technical Risk Register **Date:** 2026-03-17 (updated 2026-07-27) -**Last update:** 2026-08-11 — C-341 RESOLVED and C-346 registered (#425): the version gate that could never fail was deleted rather than given a runner, replaced by a tag-vs-version test that does fail and an unskippable publish-time guard; four circular sibling copies are recorded rather than swept up. Full narrative history, including corrections and retractions, is in [`register_changelog.md`](register_changelog.md). Keep this line to one sentence: the header is an index, and the search-window guard (`test_falsification_merge_readiness.py`, 8000 chars) is what it protects. New narrative goes in the changelog, never here (#404). +**Last update:** 2026-08-12 — C-347, C-348 and C-349 registered, C-340 narrowed and C-345 corrected (#439): the orphan detector that replaced the abandoned pre-push hook was found, before merge, to be unauthorised, name-matched and silently empty-loopable, and the drill that had "verified" it ran under the operator's own credentials rather than the runner's. Full narrative history, including corrections and retractions, is in [`register_changelog.md`](register_changelog.md). Keep this line to one sentence: the header is an index, and the search-window guard (`test_falsification_merge_readiness.py`, 8000 chars) is what it protects. New narrative goes in the changelog, never here (#404). **Source:** 71 audits, reviews, and incidents — multi-expert engineering review, repo assimilation, falsification audits, test reviews, security sweeps, and production incidents. Full list in [`register_changelog.md`](register_changelog.md#where-the-findings-came-from). Add new sources there, not here (#404). -**Status:** 346 concern IDs assigned (C-28 merged into C-31, C-107 merged into C-60, C-183 merged into C-44, C-44 merged into C-164, C-03 merged into C-176): 304 resolved-or-demoted, 39 open concerns (0 Tier 1, 4 Tier 2, 10 Tier 3, 19 Tier 4, 6 deferred by design; 4 with fired trigger); 5 demoted to tech-debt backlog 2026-08-04, 8 open disagreements. 167 resolved concerns as full entries + 19 early-archive reference rows + 120 struck-through in active register (299 unique after dedup — 5 appear in both archive and active) + 32 resolved disagreements in archive. 42 disagreement IDs total: 34 resolved, 8 open. +**Status:** 351 concern IDs assigned (C-28 merged into C-31, C-107 merged into C-60, C-183 merged into C-44, C-44 merged into C-164, C-03 merged into C-176): 304 resolved-or-demoted, 44 open concerns (0 Tier 1, 8 Tier 2, 10 Tier 3, 20 Tier 4, 6 deferred by design; 4 with fired trigger); 5 demoted to tech-debt backlog 2026-08-04, 8 open disagreements. 167 resolved concerns as full entries + 19 early-archive reference rows + 120 struck-through in active register (299 unique after dedup — 5 appear in both archive and active) + 32 resolved disagreements in archive. 42 disagreement IDs total: 34 resolved, 8 open. **Archive:** Resolved concerns and disagreements are in `archive/technical_risk_register_resolved.md`. **Ranking criteria:** Impact if wrong x likelihood x detectability. Items marked **[DEFER]** are accepted risks or wait for a specific trigger condition. See ADR-020 for governance rationale. @@ -162,6 +162,11 @@ | C-329 | 3 | The PyPI-publishing job runs unpinned third-party actions while holding OIDC publish rights — a poisoned wheel needs no secret to leak | Before the next release — pin `publish_package.yml` actions to full commit SHAs | Supply chain | | ~~C-330~~ | ~~4~~ | ~~Rotation undocumented; file mode inferred, not observed~~ | Resolved 2026-08-03 on the server: the config pointed at `/root/...`, a path the pipeline left months ago, and `missingok` made it exit successfully every night. Path fixed, `monthly`, `create 0640`, `su views-deploy`; verified by dry run. Mode observed: was 644, now 640 | Server hardening | | C-346 | 4 | Four copies of `test_version_not_already_tagged` use a conditional `xfail` that reads as rigorous and is circular — the test runs only when the version is untagged, then asserts it is untagged. Measured green in every reachable state | **Before trusting any `xfail`-marked test as a gate**: name the state that makes it fail. If none does, it is decoration | Test infra | +| C-349 | 2 | A malformed ledger line is silently dropped by both consolidators and by the health reader — `except json.JSONDecodeError: continue`, uncounted. A harvest recorded as successful would simply not be consolidated, and the run would report success | **Before the next consolidation run after any unclean shutdown, disk-full event, or manual ledger edit** — and when adding any new ledger reader, count what you skip rather than skipping silently | Consolidation correctness | +| C-350 | 2 | A workflow merged to `development` never runs on its own `schedule` — GitHub fires cron from the **default branch only**, which here is `main`. The orphan detector was documented as "checks daily" in three places while `origin/main`'s copy contained no detector at all | **Before documenting any scheduled workflow as live**, confirm the file is on the default branch: `git show origin/main:`. And at every release promotion, list which workflows the promotion is what activates | Operational monitoring | +| C-351 | 2 | **Live incident:** `serving-freshness.yml` has **never once succeeded**. All 10 runs since it was added on 2026-08-03 have failed with `fatal: not a git repository`, because the job has no `actions/checkout` step. Freshness alerting for served data has never worked and nothing surfaced it | **Immediately** — this is firing now, not a future risk. Then: no scheduled workflow should be able to fail silently for days; the run-status of the monitoring workflows is itself unmonitored | Operational monitoring | +| C-347 | 2 | A drill run in the operator's environment cannot verify a mechanism that runs in the runner's — the orphan detector was drilled under personal `gh` auth and passed; under `GITHUB_TOKEN`, whose workflow grants no `pull-requests` scope, every `gh pr list` 403s into `\|\| continue` and it reports clean forever | **Before calling any CI or server mechanism verified because a local drill passed**: name what the runner has that you do not, and what you have that it does not — credentials, PATH, refs, working directory | Test infra | +| C-348 | 4 | Deleting a test module drops the assertions that were only co-located with it — removing `tests/test_git_hooks.py` with the hook also removed the sole guard on `scripts/arm_automerge.sh`'s executable bit, a script the same PR keeps and the guide tells operators to run | **Before deleting any test module**, list its test functions and check which ones cover code that survives the deletion | Test coverage | | C-345 | 2 | Verification tooling reported a green suite that was red, twice in one session — a piped `pytest \| tail; echo $?` yields the pipe's status, and a task notification reported "exit code 0" for a run that exited 1 | **When capturing a long-running check's result** — piping it, backgrounding it, or reading a notification instead of an unpiped `$?`. Redirect to a file, capture `$?` unpiped, and grep `^FAILED` as a second reader | Test infra | | ~~C-344~~ | ~~2~~ | ~~`views-deploy`'s `~/.profile` was mode 644 inside a 751 home — every harvest credential (`UCDP_API_TOKEN`, `ACLED_*`, `GDL_API_TOKEN`, `HEARTBEAT_URL`) readable by all four accounts, continuously~~ | Registered and resolved 2026-08-10 (#432): `chmod 600`, verified unreadable from a second account and still readable by the owner. Rotation considered and **declined** by the operator 2026-08-10 — a judgement about who holds the three accounts, not evidence of non-access; revisit if a new shell account appears (C-88) | Credential hygiene | | ~~C-331~~ | ~~4~~ | ~~`HEARTBEAT_URL` capability URL passed on the curl command line — readable via `/proc`~~ | Resolved 2026-08-10 (#423): all three pings take the URL on stdin via `-K -`; drilled with a canary and a negative control. The entry's own suggested unquoted form was superseded — it truncates at whitespace and sends anyway | Operational monitoring | @@ -201,7 +206,7 @@ silence, not error** — a thing reports success while not doing what it claims. *C-342 was added to the cluster 2026-08-08, found while building the guard for C-337 — which is the cluster's own rule working: the drill found a defect adjacent to the one it was aimed at. C-343 was added 2026-08-08 from the production host, and C-317 was **closed by drill** 2026-08-10 — struck in -the table below rather than removed, so the table has ten rows. C-331 closed 2026-08-10; C-345 and C-346 added and C-342 and C-341 closed 2026-08-11, leaving eight open members of twelve rows.* +the table below rather than removed, so the table has ten rows. C-331 closed 2026-08-10; C-345 and C-346 added and C-342 and C-341 closed 2026-08-11. C-347 and C-348 were added 2026-08-12 from a `/code-review` of the fix for another member (C-340) — the cluster growing out of an attempt to shrink it, which is the honest shape of this work and is what Story 7 (#428) has to report.* | ID | What reported success while being wrong | |---|---| @@ -217,6 +222,11 @@ the table below rather than removed, so the table has ten rows. C-331 closed 202 | ~~**C-341**~~ | ~~A skipped test is not a red test — gates that assure only whoever ran them~~ — **closed 2026-08-11** | | ~~**C-342**~~ | ~~`uv sync` repairs a stale lockfile in CI's checkout, so CI is green and the stale lock stays committed~~ — **closed 2026-08-11** | | **C-343** | The deploy tag file read correctly, the pipeline was green, and the server ran the previous release | +| **C-347** | A drill that passed under the operator's credentials, for a mechanism that runs under the runner's | +| **C-348** | A test module deleted with its subject, taking an unrelated guard with it | +| **C-349** | A ledger line that fails to parse is skipped uncounted, so a missing harvest reads as a complete run | +| **C-350** | A scheduled workflow that exists only on a non-default branch — documented as running daily, never invoked | +| **C-351** | A monitoring workflow red every morning for five days, with nobody reading the mornings | **Why this belongs in the register rather than a post-mortem.** The individual fixes are already made or tracked. What the cluster adds is a *design rule*: in this system, **absence of an error is @@ -1230,7 +1240,40 @@ Cross-ref: ~~C-330~~ (the work being done when this happened), C-323/C-324 (the **The shared property, which is the reason this is registered at all:** both fail *green*. `git push` succeeds; `gh pr merge` exits 0. Neither has a failure mode that announces itself, so neither can be caught by anything except deliberately reading state back. That is the same class as C-330 (a nightly no-op reporting success) and C-337 (a lockfile frozen with no error). -Cross-ref: ~~C-320~~ (auto-merge silently degrading to a plain merge when branch protection was absent — same family, different mechanism, resolved), C-339 (the other silent-failure incident of this session). Part of work package: **Operational safety**. +**NARROWED 2026-08-12 (#426, #437, #439). Mechanism 1 has a working guard. Mechanism 2 has a detector, not a preventer, and the reason is worth more than the fix.** + +**Mechanism 1 — resolved.** `scripts/arm_automerge.sh` arms via the GraphQL disable/enable pair (which does honour the method) and then **reads `auto_merge.mergeMethod` back**, exiting non-zero on mismatch. Reproduced live on #437 before shipping: `gh pr merge 437 --auto --squash` against an already-`MERGE`-armed PR exited **0** and left the method `MERGE`; the script changed it and verified. Nothing forces the script's use, which is the residue below. + +**Mechanism 2 — four client-side attempts, four different defeats.** A pre-push hook was built and abandoned: + +| | approach | defeated by | +|---|---|---| +| v1 | `git rev-parse HEAD` + branch name | checked the wrong branch entirely (git supplies pushed refs on **stdin**); and permanently refused branch names reused from old PRs — `docs/roadmap-plan-v11` spans #50-54 | +| v2 | merged head is an **ancestor** of the push | a merge-commit merge puts that head in the base branch **permanently**, so it is an ancestor of every later branch. PR #54's head is an ancestor of `development` today | +| v3 | `remote_sha` equals the merged head | `delete_branch_on_merge` removes the branch first, so git reports `0000…` — indistinguishable from creating a branch. Proved with a two-clone experiment | +| v4 | ancestor AND not in base | never shipped | + +Its test suite was **vacuous**: reconstructing v1 and running all seven behavioural tests passed every one. + +**Why it was abandoned rather than fixed a fifth time.** A client-side check is racing GitHub's asynchronous branch deletion. That is a property of the system, not a defect to iterate out — and four failures from four *different* environment properties is the signature of inferring state you cannot see. A multi-expert panel reached the same conclusion independently: Kleppmann (two views of one system read at different times, with an async deletion between), Ousterhout (a shallow module whose complexity is entirely special cases), Beck (twice in ~440 PRs — and the recovery figure this entry first cited was wrong. **Measured:** #416 merged 2026-08-03T11:24:10Z and its recovery #417 was not opened until 2026-08-04T01:08:49Z, **13h 44m later**; #437's recovery #438 was closed unmerged and that work never landed at all (it became moot when the hook was deleted). The claim *"both recovered by cherry-pick inside the hour"* was repeated in this register, the changelog and the guide, and it was the only quantitative basis offered for abandoning the guard. Corrected 2026-08-12 by `/code-review max`. It cuts both ways and both are worth stating: 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). + +**What replaced it, after two review rounds cut it down.** The first replacement tried to establish orphan-ness *properly* — match the merged PR, compare head SHAs, count commits beyond it, exclude what is already on `development`. `/code-review max` found **fifteen** defects in it, each from a different property of the environment: branch names are reused so the name match was wrong; `gh pr list --limit 1` orders by `createdAt`, not `mergedAt`; `git fetch ` resolves `refs/tags/` before `refs/heads/`; `git merge-base` exits 128 for a missing object and that was booked as a *definite answer*; and the recovery the issue body printed could never clear its own alert, so the cron would have gone permanently red. That is four hook versions and two detector versions defeated the same way. **Precision here requires inferring state we cannot see, and every attempt to infer it acquires a new special case.** + +The shipped version asks **one question**: `delete_branch_on_merge` is on, so a branch that still exists and has **no open pull request** is already anomalous, whatever the reason. No SHAs, no ancestry, no fetch, no merged-PR lookup. It reports a branch pushed before its PR was opened, which is not really a false positive — that is a branch with work on it and no route to `development`. Clearing it is one command either way: open the PR, or delete the branch. + +**The residue, and it is real.** This is detection, not prevention. Work can still be orphaned; you learn within a day rather than at push time — and only once the workflow reaches `main` (C-350). And nothing forces `arm_automerge.sh` — `gh pr merge --auto` is one keystroke away, so *"read the value back rather than trusting the command"* remains a habit no mechanism enforces. **This entry stays open on that residue**, and Story 7 (#428) should say so rather than claim a clean resolution. + +**It recurred while being fixed.** #437 auto-merged carrying the broken v1 hook; the review fixes were pushed to that branch afterwards and orphaned — the same defect, inside the pull request addressing it. The hook was not installed in that clone, because `core.hooksPath` is per-clone config git does not version. That is the strongest available argument that a client-side install-it-yourself guard was the wrong instrument. + +**Addendum 2026-08-12 — the replacement inherited the property that defeated the original, and `/code-review` caught it before merge (#439).** Three defects, all of the cluster's own shape: + +1. **Reused branch names, again.** The detector matched a merged PR by **name alone**, with no ancestry check — the exact property that made hook v1 permanently refuse fresh branches. Verified reuse in this repo: `chore/version-bump-1.2.13`, `docs/roadmap-plan-v11` and `feat/acled-phase2` have each headed more than one PR. A new branch on a reused name, pushed before its PR is opened, would be reported as orphaned; and because the old head is not in the new branch's ancestry, `git rev-list --count` fails, `stranded` becomes `?`, and the `= "0"` guard does not catch it. **The redesign changed the moment of the check but not the question it asked.** +2. **A transient `git ls-remote` failure reported a silent all-clear.** GitHub Actions runs `bash -e {0}`, and `set -uo pipefail` does not remove `-e` — but a failing command substitution in 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`. One network hiccup and the loop never runs, `found` is empty, and the step reports clean. +3. **A new ordering dependency with no assertion.** The ancestor check needs `origin/development` from a fetch step a hundred lines earlier; `tests/test_ci_gates.py` exists precisely to assert such orderings, because getting them wrong yields a *green* run — and it was not extended. + +All three are fixed in #439. They are recorded because the lesson is not any one of them: **a mechanism built to close a fails-green defect arrived with three fails-green defects of its own**, and the drill that preceded it found none — see C-347 for why. + +Cross-ref: ~~C-320~~ (auto-merge silently degrading to a plain merge when branch protection was absent — same family, different mechanism, resolved), C-339 (the other silent-failure incident of that session), C-345 (a check reporting success while establishing nothing — the vacuous test suite here is the same shape). Part of work package: **Operational safety**. GitHub: #426, #437, #438 (closed unmerged), #439. --- @@ -3201,6 +3244,26 @@ Two readers, because one reader that can be wrong is what this entry is about. **Open, because a habit is not a control.** Nothing prevents the next pipeline from masking a status the same way. The instrument would be a wrapper that refuses to report a result it did not obtain unpiped — proposed for **#424**, which is the story about giving checks somewhere to run other than one operator's discretion. +**Addendum 2026-08-12 (#439), CORRECTED THE SAME DAY — the first version of this addendum was wrong, and being wrong is the point of it.** + +**What was claimed:** that the mitigation prescribed above (`grep -cE '^FAILED' out.txt`) is defective because *"pytest colourises its summary"*, so the line is `\033[31mFAILED\033[0m` and the caret never matches. It was written after a drill harness reported *"DID NOT CATCH"* four times while all four drills had in fact caught, because the grep returned 0 on genuinely failing runs. + +**What is actually true.** pytest does **not** colourise when stdout is redirected to a file. The ANSI codes came from `FORCE_COLOR=3`, which is exported in the operator/agent shell. Measured both ways on a genuinely failing run in this repository: + +``` +uv run pytest -q > out.txt 2>&1 -> grep -cE '^FAILED' = 0, 19 ANSI-bearing lines +env -u FORCE_COLOR uv run pytest -q > out.txt 2>&1 -> grep -cE '^FAILED' = 1, 0 ANSI-bearing lines +``` + +**So C-345's original prescription works** — in a plain shell and in CI, which is where it matters. It fails only in a shell that forces colour, which this operator's does. + +**The correction is worth more than the original claim, because the error is C-347 and it was committed inside the commit that registers C-347.** A drill ran in an environment carrying a variable nobody controlled for, produced an anomalous result, and the anomaly was diagnosed as a defect in the *tool* rather than in the *environment*. It was then escalated into a general claim about this register — *"the second time an entry has prescribed a defective remedy"*, *"a mitigation written into an entry is untested code"* — resting entirely on that uncontrolled variable. Both sentences are withdrawn. C-331's defective remedy was real and measured; this one was not, and pairing them manufactured a pattern from one instance. + +**What survives.** `--color=no` is still the right thing to write, because it removes a dependency on the caller's environment rather than assuming it — the same reasoning that makes it correct is what the wrong diagnosis got backwards. The mitigation block above is left standing unedited, with this addendum after it, per the convention that corrections are recorded rather than applied silently. + +**And the second reader really was not independent** — not because of pytest, but because the harness required `rc != 0` **and** `FAILED >= 1`, so a reader that always returned 0 in that shell could only ever suppress a catch, never report one. Requiring both readers to agree means the weaker one governs. That part of the original addendum stands. + + Cross-ref: C-339 (the other assistant-workflow hazard, and the precedent for registering one), C-330/C-337/C-343 (same shape, different mechanisms), C-341 (gates that assure only whoever ran them — this is the failure mode *of* running them). Part of the **mechanisms that fail green** cluster. GitHub: #432. --- @@ -3244,6 +3307,148 @@ Cross-ref: ~~C-341~~ (this was its last residue; resolved by the same PR), C-336 --- +### C-347: A drill run in the operator's environment cannot verify a mechanism that runs in the runner's + +**Source:** `/code-review medium` on #439 (2026-08-12), reviewing the orphan detector that replaced the abandoned pre-push hook. The detector had been drilled end-to-end the previous night and reported working. + +**Trigger:** **Before calling any CI or server mechanism verified because a local drill passed.** Name what the runner has that you do not, and what you have that it does not — credentials and their *scopes*, `PATH`, which refs exist locally, the working directory, the shell's flags. If the drill cannot be run under the production principal, say the claim is unverified rather than verified. + +**Location:** not a repo file — a verification-methodology defect, registered on the precedent of C-339 and C-345. The instance: `.github/workflows/release-topology.yml` (the `orphans` step and the workflow's `permissions:` block). + +**What happened.** The detector calls `gh pr list` twice. Drilled locally it worked, found the genuine orphan, and correctly ignored a branch whose PR was closed-unmerged — a good drill, with a positive and a negative case. It ran under the operator's personal `gh` credentials, which carry full scope. + +In production it runs under `GITHUB_TOKEN`, and the workflow declares: + +```yaml +permissions: + contents: read + issues: write +``` + +An explicit `permissions:` block sets every unlisted scope to `none`. `pull-requests` is unlisted, so both calls 403 — and both are written `... 2>/dev/null) || continue`, which makes a 403 **indistinguishable from "no PR found"**. Every branch would be skipped, `found` would stay empty, and the step would print "No orphaned branches" and set `orphans=false`, on a daily cron, permanently, green. This is the first `gh pr` call in any workflow in this repository, so no precedent established that the grant was sufficient. + +**Why this is not C-345.** C-345 is *the instrument misreported a result it did obtain* — a piped exit status, a task notification. Here the drill obtained and reported its result correctly. The defect is that the drill ran **in the wrong world**, so no amount of care in reading it would have helped. The assertion was sound; its subject was not the thing that ships. + +**Tier 2, and the justification is required.** Not Tier 3: the deliverable of epic #421 is CI mechanisms, and if drills cannot verify them then every mechanism the epic installs carries unquantified false confidence — including the ones already merged. Not Tier 1: nothing is corrupted and no model output is wrong; the failure is in *knowing whether* a guard guards. + +**It has fired four times in this epic, all in one week, and all four were caught by accident or by review rather than by any control:** + +| | the drill | what differed | +|---|---|---| +| 1 | `PATH=/usr/bin:/bin` to prove the hook fails open with `gh` absent | `gh` lives in `~/.local/bin`; it was never hidden, so the drill tested nothing. The rebuilt version asserted absence first — and then hid `bash` from the test runner | +| 2 | `git checkout v1.11.0` to run a gate off-tag | the checkout was silently refused (uncommitted changes), so the "want FAIL" case ran on-tag and returned 0 | +| 3 | `pytest` for a test that shells out to `git describe` | run from the main repo, not the checkout under test — the subprocess answered about the wrong tree | +| 4 | `gh pr list` under personal auth | production runs under `GITHUB_TOKEN` with `pull-requests` revoked | + +Four instances, four *different* environment properties — the same signature that ended the pre-push hook (C-340). The pattern is not carelessness; it is that a drill is an experiment, and an experiment silently inherits every variable you did not control. + +**Verified live, by the remedy this entry prescribes (run 31590304501, 2026-08-12).** The workflow was **dispatched** on the branch rather than having its body run locally, and the step reported: + +``` +Checked 1 branch(es); 0 could not be answered for. +``` + +That is the decisive line. The pull request's own branch is the only branch that is neither `main` nor `development`; it is not in `development`, so the step reached `gh pr list` and **got an answer**. Had `pull-requests` still been revoked, that call would have 403'd into `|| continue`, giving `answered=0, unanswered=1`, and the new zero-answers rule would have exited non-zero. The run was green, the tracking-issue step was correctly skipped, and no issue was opened. + +**What this does not verify, stated because the entry is about exactly this distinction:** only the *permission* and the *counting* are confirmed in the production environment. The detector's positive path — actually finding an orphan — has no live instance to exercise and was drilled against a scratch repository, where git ancestry semantics are identical. Two different classes of evidence, and conflating them is the mistake. + +**What would actually close this.** A drill is only evidence if it is run under the production principal, or if the difference is enumerated and argued to be irrelevant. For workflows specifically, that means dispatching the workflow rather than running its body locally. Nothing enforces this today, which is why the entry is open. #424 is the story about giving checks somewhere to run other than one operator's discretion, and this belongs to it. + +Cross-ref: C-345 (the instrument misreading a result it did obtain — this is the complement: reading correctly in the wrong environment), C-339 (the precedent for registering an operator-workflow hazard), C-340 (the mechanism whose replacement this nearly shipped broken), C-341 (gates that assure only whoever ran them — the same question asked of *where* rather than *whether*), C-336 (a guard's claim narrower than the property). Part of the **mechanisms that fail green** cluster. GitHub: #426, #439. + +--- + +### C-348: Deleting a test module drops the assertions that were only co-located with it + +**Source:** `/code-review medium` on #439 (2026-08-12). + +**Trigger:** **Before deleting any test module** — list its test functions and check which ones cover code that survives the deletion. The trigger is concrete and imminent, not perpetual: epic #421 is a deletion-heavy sprint (Story 4 deleted `TestF1VersionBumped`, Story 5 deletes the hook and its tests) and Story 7 (#428) has more removal in it. + +**Location:** `tests/test_git_hooks.py` (deleted in #439), `scripts/arm_automerge.sh` (kept). + +**What happened.** `tests/test_git_hooks.py` was written for the pre-push hook. When the hook was abandoned, the module went with it — correctly, for the hook-specific assertions. But it also held `test_arm_helper_exists_and_is_executable`, the **only** guard on `scripts/arm_automerge.sh`'s existence and executable bit. That script is deliberately kept by the same pull request, is the working half of C-340, and `docs/guides/publishing_to_pypi.md` instructs operators to run it directly. After the deletion, `grep -rl arm_automerge tests/` returned nothing. + +The deleted test's own docstring is the argument for restoring it: *"A mode lost to a rebase, a patch, or a filesystem copy is invisible."* + +**Why it fails green.** Nothing detects a deleted assertion. The suite goes from N tests to N−4 and stays green; coverage tools do not measure a file's mode; and the reviewer's attention is on the thing being removed, not on what was sitting next to it. A test module is an *organisational* unit that quietly doubles as a *coverage* unit, and only the first is visible when you delete it. + +**Tier 4.** No correctness or reliability impact and single-operator scope: the concrete loss is that `arm_automerge.sh` losing its executable bit would surface as `Permission denied` at the moment an operator runs it — loud, immediate, and one `chmod` from fixed. Registered rather than merely fixed because the *mechanism* is general and the trigger is live: two more deletions are scheduled in this epic, and the same review that found this one will not necessarily run on them. + +Cross-ref: C-346 (the sibling copies that survived a deletion — the same sprint, the opposite error: too little removed rather than too much), C-341 (a gate deleted deliberately, with its coverage argued through first — the pattern this entry asks for), C-340 (the deletion in question). Part of the **mechanisms that fail green** cluster. GitHub: #439. + +--- + +### C-349: A ledger line that fails to parse is skipped without being counted + +**Source:** `/review-diff` on #439 (2026-08-12). Found by generalising a critical finding about the orphan detector's `|| continue` paths, then grepping for the same shape in production code. + +**Trigger:** **Before the next consolidation run following any unclean shutdown, disk-full event, or manual ledger edit** — those are the ways a partial line gets written. And **when adding any new ledger reader**: count what you skip. + +**Location:** seven `json.JSONDecodeError` sites under `src/` — `grep -rn JSONDecodeError src/` is the enumeration, and **counting them is part of the fix**. The ones that swallow silently: `consolidators/acled.py`, `consolidators/ucdp.py`, `datafactory_provenance/health.py` (`read_last_entries`, and a second `except (json.JSONDecodeError, OSError): pass` in the freshness check that leaves `content_fresh` unset). **The precedent to copy is `datafactory_provenance/digests_and_ledgers.py` (`_read_ledger_entries`), which already logs `"Skipping malformed ledger line %d in %s"`** — the provenance package's own ledger reader, a closer model than `grid_compilation.py`'s out-of-bounds cell counter. + +**The defect.** All three read the provenance ledger line by line and do: + +```python +try: + entry = json.loads(line) +except json.JSONDecodeError: + continue +``` + +A line that fails to parse is dropped, and in these readers **nothing records that it happened**. (An earlier version of this entry said *nothing anywhere* records it, and named three sites. Both were wrong: the grep returns seven, and `digests_and_ledgers.py` already logs the skip. Corrected by `/code-review max` on #439 before merge — an entry that undercounts its own locations gets closed after a partial fix, which is C-336's *claim narrower than the property* inside a new concern.) The consolidators use the ledger to decide which harvested files to consolidate, so a dropped line means a harvest that succeeded is simply not consolidated — and the run reports success, because from its point of view there was nothing to do. The output is short by one source file, with no error, no warning, and no count. + +**Why it is registered rather than merely fixed.** The repository already contains the correct pattern **in the same package**: `digests_and_ledgers.py` warns on a malformed ledger line, and `grid_compilation.py` counts its skipped cells. The inconsistency between readers of the *same file format* is the finding. A reader of either consolidator has no way to tell whether "skip silently" was a considered decision or an omission, and the next ledger reader will be copied from whichever one is nearest. + +**Tier 2, argued rather than assumed.** Not Tier 3: the consequence is missing data in a consolidated artefact that feeds model inputs, not a maintainability cost. Not Tier 1: no instance has been observed, and the ledger is written by our own code with `json.dumps` under a lock (C-294/C-295), so a malformed line requires a partial write or a manual edit. Likelihood is low; **detectability is zero**, and that is the axis this cluster is about. + +**The fix is small and already specified by the codebase's own precedent:** count the skips, include the count in the outcome, and fail loudly if it is non-zero — a ledger that cannot be parsed is not a ledger with fewer entries. Deliberately **not** done in #439, which is a CI story; doing it there would have put an untested change to consolidation inside a pull request about a workflow. + +Cross-ref: C-347 (the same "could not check" read as "checked and fine", in the orphan detector — this entry is that finding generalised into production code), C-330 (a no-op reporting success), C-136 (`read_last_entries` crashing on non-UTF8 — the *loud* failure mode of the same function, demoted; this is the silent one), C-294/C-295 (the locking that makes a partial write unlikely). Part of the **mechanisms that fail green** cluster. GitHub: #439. + +--- + +### C-350: A workflow merged to `development` does not run on its own schedule + +**Source:** `/code-review max` on #439 (2026-08-12). The headline finding of that review, and the reason the story it reviewed is not shippable as written. + +**Trigger:** **Before describing any scheduled workflow as live** — confirm the file is on the default branch with `git show origin/main:.github/workflows/.yml`. And **at every release promotion**, note which workflows the promotion is what activates; that is a side effect of promotion nobody currently records. + +**Location:** `.github/workflows/release-topology.yml` (the `orphans` step, added on `development`), `docs/guides/publishing_to_pypi.md`, and this register's own C-340 narrowing. All three have been corrected. + +**The mechanism.** GitHub fires a `schedule` trigger from the **default branch only** — not from the branch the workflow file was added on. This repository's default branch is `main`, and all work goes through `development` (never PR to `main` is a standing rule here). So **every scheduled workflow authored on `development` is dark until a release promotion carries it to `main`**, and promotions are irregular — the last was v1.11.0 on 2026-08-03. + +**Measured, not inferred:** `gh repo view --json defaultBranchRef` returns `main`; `git show origin/main:.github/workflows/release-topology.yml | grep -ci orphan` returns **0**; and every run of that workflow with `event: schedule` has `headBranch: main`. + +**Why this is the cluster's shape and not a footnote.** The detector was written, drilled, reviewed, verified live by `workflow_dispatch`, and documented in three places as *"checks daily"* — while the thing that would actually invoke it had no copy of it. Every one of those verifications was true and none of them was the question. **A `workflow_dispatch` run proves the code works; it says nothing about whether anything will ever call it.** The four guards in `tests/test_ci_gates.py` assert properties of the branch's file, so they go green on every pull request while the branch that runs the cron has none of it — a test suite that is structurally incapable of noticing. + +**Tier 2, argued.** Not Tier 3: the consequence is a control believed to be operating that is not, which is worse than a control known to be absent — the belief displaces the manual habit. Not Tier 1: nothing is corrupted; the detector's absence only means orphaned work is found later, by hand, as it was before. The generality is what earns the tier — this applies to *every* scheduled workflow this project will ever add, and the project adds them regularly (`release-topology.yml`, `serving-freshness.yml`). + +**What would close it.** A guard that reads the default branch's copy of each scheduled workflow and compares it to the branch's, so "this workflow is not live yet" is a visible state rather than an assumption. Note the ordinary CI job cannot do this naively — it would redden every PR that touches a workflow, which is correct information delivered as noise. Belongs with #424. + +Cross-ref: C-347 (verified in the wrong environment — this is verified in the right environment and never invoked, the complementary error), C-341 (a gate that skipped itself), C-343 (a deploy that deployed nothing — the same "shipped ≠ running" gap, one layer down), C-351 (a scheduled workflow that *is* live and has been failing unnoticed). Part of the **mechanisms that fail green** cluster. GitHub: #439. + +--- + +### C-351: `serving-freshness.yml` has failed every scheduled run since 2026-08-08 — [FIRING] + +**Source:** `/code-review max` on #439 (2026-08-12), flagged as an out-of-diff observation while checking the workflow family. + +**Trigger:** **Fired.** This is a live failure, not a future risk. Fix the workflow, then answer the second-order question: nothing watches whether the watchers ran. + +**Location:** `.github/workflows/serving-freshness.yml`. + +**Observed.** `gh run list --workflow=serving-freshness.yml` returns **10 runs, 10 failures, zero successes**, the earliest 2026-08-03 — the day the workflow was added. An earlier version of this entry said "since at least 2026-08-08… dead for five days", which understated it and, worse, implied a regression. It is not a regression: **it shipped broken and was never verified**, which makes it C-350's sibling rather than C-338's. The job has **no `actions/checkout` step**, so a git command in the close step aborts with `fatal: not a git repository`. `grep -c 'actions/checkout' .github/workflows/serving-freshness.yml` returns 0. + +**What it means operationally.** This workflow is the freshness check for served data — the mechanism that says the zarr/parquet endpoints are serving something current. It has never worked. Nobody noticed, which is the part that matters: a red cron in the Actions tab is *loud* in principle and *silent* in practice, because nobody opens the Actions tab on a morning when nothing seems wrong. + +**Tier 2.** Freshness alerting for the served artefacts is inoperative, and per C-338 this workflow exists precisely because the monitoring vendor's free tier cannot do content checks — so there is no second path. Not Tier 1: no data is corrupted and the pipeline itself is unaffected; what is lost is notice. + +**Deliberately not fixed in #439**, which is a story about the orphan detector. Recorded here immediately rather than left as a review aside, because *"pre-existing"* is not a disposition this project accepts — we are the only developers, and a failing check is a failing check on the day it is seen. + +Cross-ref: C-338 (why this workflow exists rather than a vendor keyword monitor), C-335 (the serving-path freshness question), C-131 (the dead-man switch — the same "silence reads as health" problem), C-350 (the other scheduled-workflow defect found in the same review). Part of the **mechanisms that fail green** cluster. GitHub: #439. + +--- + ## Deferred by Design ### C-10: Ontology vocabulary overhead diff --git a/scripts/git-hooks/pre-push b/scripts/git-hooks/pre-push deleted file mode 100755 index c710a0d..0000000 --- a/scripts/git-hooks/pre-push +++ /dev/null @@ -1,72 +0,0 @@ -#!/usr/bin/env bash -# Refuse a push to a branch whose pull request has already merged (C-340). -# -# WHY THIS IS A HOOK AND NOT ADVICE -# #416 merged the instant CI went green. A follow-up commit was then pushed -# to that branch, which by then had no open PR. Two pieces of work were -# simply not on `development`. `git push` reported success; the only signal -# was a PR showing one commit when two had been pushed. -# -# Everything else in epic #421 is a test or a workflow — it runs whether or -# 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. -# -# 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 - -# Long-lived branches are pushed to constantly and never have their own PR. -case "$branch" in - main|development) exit 0 ;; -esac - -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 -} - -# Empty list, or anything unparseable, means "no merged PR" — allow. -case "$merged" in - ""|"[]") exit 0 ;; -esac - -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 - -cat >&2 < - git push -u origin - - If you are certain you want this push: git push --no-verify - -EOF -exit 1 diff --git a/tests/test_ci_gates.py b/tests/test_ci_gates.py index 2df3aad..1f0a01b 100644 --- a/tests/test_ci_gates.py +++ b/tests/test_ci_gates.py @@ -28,6 +28,7 @@ from __future__ import annotations +import re from pathlib import Path from typing import Any @@ -52,6 +53,19 @@ def _index_of_run(steps: list[dict[str, Any]], needle: str) -> int: raise AssertionError(f"no step runs {needle!r}") +def _step_by_id(workflow: Path, job: str, step_id: str) -> dict[str, Any]: + """Locate a step by its stable ``id:``. + + NOT by a substring of the body being asserted on: that makes the + guard retarget itself if any other step ever contains the same + command, and it couples step identity to the very text under test. + """ + for step in _steps(workflow, job): + if step.get("id") == step_id: + return step + raise AssertionError(f"{workflow.name}:{job} has no step with id {step_id!r}") + + class TestEveryStepIsWellFormed: """A step with two `run:` keys parses fine and silently loses one.""" @@ -126,3 +140,139 @@ def test_the_gates_get_a_token_and_the_ci_marker(self) -> None: "local branch, so it would otherwise claim coverage it does not " "have." ) + + +class TestOrphanDetectorCanActuallyAnswer: + """The orphan detector's silent preconditions. + + Every assertion here was rewritten after `/code-review max` found + that three of the four originals could not fail for the property + they claimed: ``"answered="`` is a substring of ``"unanswered="``, so + stripping every ``answered`` counter still matched; and a bare + ``"::error::" in run`` was satisfied by an unrelated guard elsewhere + in the same 130-line step, so deleting both branch-enumeration + guards kept the suite green. A guard written against fails-green + that itself fails green is the epic's subject, committed inside it. + """ + + def _run(self) -> str: + """The step body with whole-line comments removed. + + Load-bearing. This step's comments quote the anti-patterns they + warn against — ``# `for x in $(cmd)` swallows a failure`` — so a + shape assertion run against the raw body matches the explanation + and reddens on the *fixed* file. That is exactly how v1 of + ``test_heartbeat_secret.py`` failed (C-336), caught here by the + control run rather than by review. + """ + run = _step_by_id(HYGIENE, "topology", "orphans")["run"] + return "\n".join( + line for line in run.splitlines() if not line.lstrip().startswith("#") + ) + + def test_the_workflow_grants_pull_requests_read(self) -> None: + """The detector is built entirely on ``gh pr list``. + + An explicit ``permissions:`` block sets every unlisted scope to + ``none``, so omitting this revokes it. The calls are written + ``... 2>/dev/null) || ...``, making a 403 indistinguishable from + "no PR found". Verified live under ``GITHUB_TOKEN`` by + dispatching the workflow (run 31590304501), which is the only + way to check a grant — a local drill runs as the operator. + """ + data = yaml.safe_load(HYGIENE.read_text()) + perms = data.get("permissions") or {} + assert perms.get("pull-requests") == "read", ( + f"release-topology.yml must grant `pull-requests: read`; it " + f"grants {perms!r}. Without it every `gh pr list` 403s, is " + f"swallowed, and the step reports clean forever (C-347)." + ) + + def test_the_branch_list_is_not_iterated_blind(self) -> None: + """``for x in $(cmd)`` hides a failure of ``cmd`` completely. + + GitHub runs ``bash -e {0}``; ``set -uo pipefail`` does not remove + ``-e``; and a failing command substitution in a ``for`` word list + is exempt from it. So a transient ``ls-remote`` failure gives an + empty loop and a confident all-clear. + + Matched on the *shape* rather than on the loop variable's name, + which the first version keyed on and which a rename defeated. + """ + assert not re.search(r"for\s+\w+\s+in\s+\$\(", self._run()), ( + "The orphan detector iterates a command substitution " + "directly. A failure of that command is invisible there, so " + "one network hiccup yields an empty loop and a green 'No " + "orphaned branches'. Capture it, check the status, reject " + "empty (C-347)." + ) + + def test_failing_to_enumerate_branches_is_loud(self) -> None: + """Both enumeration guards must survive, individually. + + The first version asserted ``"::error::" in run and "exit 1" in + run`` against the whole step — satisfied on their own by the + zero-observation guard a hundred lines below, so deleting BOTH + enumeration guards left it green. Each is now anchored to the + condition it guards. + """ + run = self._run() + pattern = r"git ls-remote[^\n]*\n(?:[^\n]*\n)?\s*echo \"::error::" + assert re.search(pattern, run), ( + "The `git ls-remote` failure path no longer raises an error. " + "Reporting clean because the branch listing failed is exactly " + "the defect the detector exists to catch." + ) + assert re.search(r'\[ -n "\$branches" \]', run), ( + "The empty-branch-list guard is gone. `ls-remote` succeeding " + "and returning nothing is not the same as 'no branches' — a " + "repository always has at least main and development." + ) + + def test_answered_and_unanswered_are_counted_separately(self) -> None: + """The counting the zero-observation rule rests on. + + ``"answered="`` is a SUBSTRING of ``"unanswered="``, so the first + version of this test passed with every ``answered`` counter + deleted. The lookbehind is the whole point. + """ + run = self._run() + assert re.search(r"(? None: + """`orphans=false` is what closes the tracking issue. + + If a scan that could not reach some branches wrote `false`, the + close step would close a live orphan issue on the strength of a + scan that skipped the very branch it was about. A third value is + the point: `partial` neither opens nor closes. + """ + run = self._run() + assert "orphans=partial" in run, ( + "The detector no longer distinguishes a partial scan from a " + "clean one. `orphans=false` drives the issue-close step, so a " + "degraded scan writing `false` closes a live orphan issue " + "(C-347)." + ) + # And a finding must survive a degraded scan rather than being + # rounded down to "partial". + assert run.index('orphans=true') < run.index('orphans=partial'), ( + "The `found` branch must be tested BEFORE the partial branch, " + "or a real orphan discovered during a degraded scan is " + "reported as merely 'partial' and never opens an issue." + ) diff --git a/tests/test_git_hooks.py b/tests/test_git_hooks.py deleted file mode 100644 index f0665e1..0000000 --- a/tests/test_git_hooks.py +++ /dev/null @@ -1,69 +0,0 @@ -"""Guard: the pre-push hook stays installable and stays fail-open. - -The hook itself is the machinery for C-340 mechanism 2 — it runs on every -push and cannot be forgotten. What a test can add is narrow but real: - -**It must remain executable.** Git silently ignores a hook without the -executable bit. No error, no warning — the protection just stops, and the -next push to a merged branch orphans a commit exactly as #416's did. A -mode lost to a rebase, a patch, or a filesystem copy is invisible. - -**It must remain fail-open.** Every path where the hook cannot answer — -no ``gh``, no auth, no network — has to allow the push. A hook that -blocks work when it does not know gets uninstalled within a day, and -then it protects nothing at all. This asserts the escape hatches are -still present rather than that they still work; the behaviour is drilled -separately, in the PR. - -**The install step must stay documented.** ``core.hooksPath`` is per-clone -config that git does not version, so a hook nobody installs is a file -nobody runs. -""" - -from __future__ import annotations - -import os -from pathlib import Path - -REPO = Path(__file__).resolve().parents[1] -HOOK = REPO / "scripts" / "git-hooks" / "pre-push" -ARM = REPO / "scripts" / "arm_automerge.sh" -GUIDE = REPO / "docs" / "guides" / "publishing_to_pypi.md" - - -class TestHookStaysUsable: - def test_hook_exists_and_is_executable(self) -> None: - assert HOOK.is_file(), f"{HOOK} is missing — C-340 mechanism 2 is unguarded" - assert os.access(HOOK, os.X_OK), ( - f"{HOOK} has lost its executable bit. Git ignores a non-executable " - f"hook SILENTLY — no error, no warning, the protection simply " - f"stops. Restore with `chmod +x`." - ) - - def test_arm_helper_exists_and_is_executable(self) -> None: - assert ARM.is_file(), f"{ARM} is missing" - assert os.access(ARM, os.X_OK), f"{ARM} has lost its executable bit" - - def test_hook_still_fails_open(self) -> None: - """Every 'cannot answer' path must allow the push.""" - text = HOOK.read_text() - for needle, why in [ - ("command -v gh", "must check gh exists before using it"), - ("gh not installed", "must say why it is allowing when gh is absent"), - ("could not answer", "must say why it is allowing when gh cannot auth"), - ("--no-verify", "must tell the operator the escape hatch"), - ]: - assert needle in text, ( - f"pre-push hook no longer contains {needle!r} — it {why}. " - f"A hook that blocks work when it cannot answer is worse than " - f"the problem it solves; it gets uninstalled, and then it " - f"guards nothing (C-320's lesson applied to a hook)." - ) - - def test_install_step_is_documented(self) -> None: - assert "core.hooksPath" in GUIDE.read_text(), ( - "publishing_to_pypi.md no longer documents " - "`git config core.hooksPath scripts/git-hooks`. That setting is " - "per-clone and git does not version it, so an undocumented hook " - "is a file nobody installs." - ) diff --git a/tests/test_ops_scripts.py b/tests/test_ops_scripts.py new file mode 100644 index 0000000..ef45781 --- /dev/null +++ b/tests/test_ops_scripts.py @@ -0,0 +1,68 @@ +"""Guard: operator scripts stay runnable (C-348). + +These are the scripts a human is told to invoke by name from a guide. +Their failure mode is not subtle — a lost executable bit surfaces as +``Permission denied`` the moment someone runs one — but nothing else in +the suite notices that they exist at all, and that is how the coverage +was lost in the first place. + +**Why this file exists separately.** The assertion below used to live in +``tests/test_git_hooks.py``, alongside tests for the pre-push hook. When +the hook was abandoned (C-340) that module was deleted with it, and this +assertion went too — silently, because deleting a test module removes +assertions nobody is thinking about. ``scripts/arm_automerge.sh`` is the +*working* half of C-340 and is deliberately kept, so its guard is kept +too, in a module named for what it actually covers rather than for what +it happened to sit next to. +""" + +from __future__ import annotations + +import os +from pathlib import Path + +REPO = Path(__file__).resolve().parents[1] +GUIDE = REPO / "docs" / "guides" / "publishing_to_pypi.md" + +# Scripts a guide tells an operator to run directly, by path. +OPERATOR_SCRIPTS = [ + ( + REPO / "scripts" / "arm_automerge.sh", + "C-340 mechanism 1 — arms auto-merge and reads the method back", + ), +] + + +class TestOperatorScriptsStayRunnable: + def test_each_script_exists_and_is_executable(self) -> None: + broken = [ + (str(path.relative_to(REPO)), why, path.is_file()) + for path, why in OPERATOR_SCRIPTS + if not (path.is_file() and os.access(path, os.X_OK)) + ] + assert not broken, ( + f"Operator scripts missing or not executable: {broken} " + f"(path, purpose, exists). A mode lost to a rebase, a patch, " + f"or a filesystem copy is invisible until someone runs the " + f"script and gets `Permission denied`. Restore with `chmod +x`." + ) + + def test_the_guide_still_points_at_them(self) -> None: + """A script no guide names is a script nobody runs. + + The counterweight to the test above: it must not be satisfiable + by deleting the script, and the coverage must not drift away + from what operators are actually told to do. + """ + text = GUIDE.read_text() + missing = [ + str(path.relative_to(REPO)) + for path, _why in OPERATOR_SCRIPTS + if str(path.relative_to(REPO)) not in text + ] + assert not missing, ( + f"{missing} are guarded here but no longer named in " + f"publishing_to_pypi.md. Either the guide dropped them — in " + f"which case operators have no route to them — or they were " + f"renamed and this list is stale." + )