Files
ersatztv/docs/decisions/records/process/check-and-use-pins-a-version.md
T
timothyandClaude Opus 5 7d81f85b38 fix(778): make the by-name pin falsifiable — it was filtering on the property it asserted
Confirmation pass returned BLOCKED, and its lead finding is the one worth having.

The test forbidding the by-name lookup recorded URLs from INSIDE the
`endswith("/branch_protections")` branch, so the only URLs it could ever record were
ones that already satisfied the assertion. A by-name request was invisible to the very
test written to forbid it. Cold review proved it by reintroducing the lookup in the
hook: the suite stayed 33/33 green. That is the filter-on-the-asserted-property defect
this PR's sibling record exists to describe, committed inside the guard against it —
and the commit message had called the twin "pinned so it cannot come back".

The recorder now sees every branch-protection URL whatever its shape. Re-verified by
the same mutation: reintroducing a by-name call reddens exactly the two tests that
forbid it.

Also from that pass:
- an HTTP 404 on the LIST read reached the "the full rule list was read and none
  matches" deny — a claim about a read that never happened. Gitea answers 404 for a
  repo that is absent or invisible to the credential, so the classifier's own verdict
  is now the sentinel `nomatch` and HTTP failures reach the ask;
- two comment blocks still described the deleted by-name endpoint as live, one of them
  asserting the classifier "is never reached at all";
- the decision record still documented `branch_protections/{base}` and its 404
  semantics as the mechanism, in the record this PR authored — now rewritten to the
  list endpoint, with why reading the LIST is the load-bearing choice;
- seven assertions on a string the hook no longer emits, and three test
  names/docstrings describing the removed 404 flow;
- an unused fixture helper, and 79 lines left over-indented by the removed nesting.

724 tests green.

refs #778

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-19 21:29:09 +02:00

14 KiB

key, title, status, since, supersedes, superseded-by, rule, signals, mechanics
key title status since supersedes superseded-by rule signals mechanics
process.check-and-use-pins-a-version 2026-08-16 — a check and the action it authorizes are bound to one version, or the gap is stated and fenced (#778) active 2026-08-16 none none Where a CHECK authorizes an ACTION over state that can change in between, the two are bound to ONE version of that state. Binding alone is not enough and is the half that keeps being skipped: a snapshot nothing re-validates is not pinned, it is a stale read wearing a version number. Three substrates, three mechanisms, and they are the SAME rule — in-process, a compare-exchange claim taken by the caller, never a `Volatile.Read` in one place and an `Interlocked` in another (`ffmpeg.work-ahead-slot-atomic`); over our own HTTP API, RFC 7232 `If-Match`/ETag, with the force-write path named explicitly rather than left implicit (`concurrency.ifmatch-rfc7232`, `concurrency.force-write-non-ifmatch`); against a remote service, a full commit sha, an image digest or a monotonic event count re-read immediately before the write. Prefer true compare-and-set where the server offers it. Where it does not — Gitea's commit-status API has no ETag, no If-Match and no expected-previous-state — the ceiling is READ-COMPARE-REFUSE: re-read the identifier immediately before the write and FAIL CLOSED on any movement, which narrows the window to one round trip and makes the loss observable instead of silent. A residual that cannot be closed is STATED in the code and carried in `docs/remote-state-inventory.md` as `UNSAFE-KNOWN` with the reason it is tolerable; "noticed" is not "accepted". Two identifier traps are load-bearing here: compare the FULL sha, never a 7-char prefix, and compare a base BRANCH REF rather than its tip sha, because the tip moves on every unrelated merge and comparing it deadlocks every open PR. Finally, and this is the failure #778 actually found: a mitigation that lives OUTSIDE the code relying on it — branch protection, a required status context, a server-side refusal — must be VERIFIED at the point of use, not asserted in a comment or in the reason string a human reads. A dated claim about configuration is not a check, and it is worse than no claim, because it talks the next reader out of looking. check-and-use race · TOCTOU over remote state · pin a version or compare-and-set · read-compare-refuse · fail closed on movement · snapshot that stops being true · full sha never a 7-char prefix · base ref not base tip sha · monotonic event count not a branch name · ABA · required status check verified not asserted · the mitigation lives outside the code that relies on it · `UNSAFE-KNOWN` with a stated reason · paths: `docs/remote-state-inventory.md`, `scripts/tests/test_remote_state_inventory.py`, `.claude/hooks/pretooluse-merge-consent.sh`, `scripts/post-review-verdict.sh`, `scripts/pr-changed-files.sh` · issues: #778, #773, #707, #706, #632, #622, #536 The detector is not a linter — there is no way to spot "this code should have pinned a sha". It is `testing.guard-derives-population-from-source` applied to an enumerated inventory: `docs/remote-state-inventory.md` classifies every in-scope executable `PINNED`/`CAS`/`UNSAFE-KNOWN`/`N/A`, and `scripts/tests/test_remote_state_inventory.py` derives the population from `git ls-files` — the index, never the filesystem, which reports untracked build output and differs per machine — and asserts set equality both ways, so a new script that talks to a remote service cannot ship unclassified. Nothing checks that a `PINNED` claim is true; that stays with review.

docs/defect-shapes-773.md §3 names this as Family D, the one class #773's taxonomy had no bucket for at all. Three raters proposed it unprompted; a fourth, working blind, proposed it again under its own name. Five records: #536, #622, #632, #706, #707.

The unifying property, and why the name matters. A check and the action it authorizes are separated in time over state that can change in between, with nothing pinning a version. #622's own record states it exactly: "The gate was never bypassed — it was satisfied against a snapshot that stops being true." That sentence is the whole class. Nothing is mis-scoped and no predicate is wrong; the answer was simply computed about a different world than the one the action lands in.

The repo had already solved this twice without noticing it was one problem. #536 was fixed with a compare-exchange claim. The whole /api/v1 write surface was given RFC 7232 If-Match a year earlier. Both are this rule; neither pointed at the other, and the tooling — the third substrate, where the state is somebody else's server — got the fix a third time from scratch at #706 and #707. That is the same one-record-per-instance growth defect-shapes-773.md §4 criticises in this repo's own knowledge base, and it is the reason this record is written at the class level.

Binding is the easy half; re-validation is the half that gets skipped. A sha captured into a variable and then used in a URL feels pinned and is not. scripts/post-review-verdict.sh is the worked example of doing it properly: it re-reads the PR immediately before the status POST, compares both .head.sha and .base.ref, and dies without writing anything on either mismatch. The comparison, not the capture, is what makes it safe.

Where no compare-and-set exists, say so instead of implying one. Gitea's status API offers no conditional write, so review-verdict.yml cannot make its read and its POST one operation. It narrows the window twice — a monotonic change_target_branch event-count fence, and a high-water-mark re-read that repairs a success posted over a human verdict back to pending — and then states the remaining gap in the file. The count is used rather than the branch NAME because a name is ABA-vulnerable: main -> S -> main reads main at both ends, which is how #698 route 1 obtained a forged exemption. An honest residual is a design output. A file claiming atomicity it does not have is the thing that stops getting re-examined.

The failure this issue actually found, which none of the five records predicted. The merge-consent hook's scheduled-auto-merge path is safe only because review-verdict/h10 is a REQUIRED status check on main — a commit status belongs to one sha, so a commit pushed after scheduling cannot inherit the verdict and Gitea refuses the merge. That is #622's fix and it works. But it is branch-protection configuration. It lives outside this repo, nothing in the repo compared the two, and the hook asserted it in a comment and in the grant reason a human reads: "because the verdict status is bound to this sha, a commit pushed before Gitea merges will clear it and block the merge." Switch that context off and every word of that sentence becomes false while the hook keeps printing it and keeps auto-granting.

So the class has a second face: not only "the state moved between the check and the action", but "the thing that made the action safe was never observed at all". The hook now reads the repo's FULL rule list, GET /repos/{owner}/{repo}/branch_protections, and classifies it — present proceeds, unreadable asks (a transient failure, or a credential without the repo-admin scope that endpoint needs, is not evidence of safety), nothing-can-govern denies, and anything it cannot decide asks. Denying on absence is the point: that is #622's hole reopened, and its defining property is that it is silent from the merge caller's side.

Reading the LIST rather than the rule named after the base is the load-bearing choice, and the first version got it wrong in the way this record is about. GET …/branch_protections/{name} is an exact database lookup that performs no matching and knows nothing about precedence, so a 200 from it establishes only that a rule with that NAME lists the context — never that the context is required ON that branch. Gitea resolves the governing rule by Priority first and plain-name-ness second, so a glob rule can outrank an exactly-named one. Using the by-name endpoint first and the list only on a 404 therefore guarded the 404 path while the 200 path — the one that actually fires, since the rule IS named main — granted without the check at all: hardened code that was dead, beside live code that was not. It was fixed by DELETING the by-name path, not by documenting the gap, so there is one fetch, one classifier and one argument to keep true. Absence is now established by the classifier over a list that WAS read, never by an HTTP status, because a 404 from the list endpoint means the repo was not found rather than that the branch is unprotected.

And that check is a preflight, not a pin — say so, because the temptation is to bank it. The first draft of the inventory graded that path PINNED; cold review pointed out that the hook's own comment concedes the read pins nothing, and it was right. Branch protection has no version, ETag or conditional read, so an admin can still weaken it after the hook looks. What the check buys is drift DETECTION and the removal of an unobserved assumption, which is the honest ceiling for that API. The residual is BOUNDED, not closed, and the bound is a trust assumption worth naming: everything on that path assumes repo-admin branch-protection config is not hostile. Saying it was "closed one layer down" by the very protection an admin may have removed was circular — the same sentence appeared in the inventory and was rewritten there first, which is how a stale twin survives a fix round. A row claiming otherwise would be exactly the overclaim this record warns about, and it is recorded here because the record's own deliverable made it on the first pass.

This is the same gap testing.guard-derives-population-from-source already flagged one directory over — MARKED_JOBS in test_ci_dropped_step_guard.py is a hand-written mirror of those same required contexts, annotated with a date. A dated comment is a claim about the past. Two independent guards now depend on that configuration; one of them checks it.

The deliverable's own population was wrong three times, and that is the most transferable part of this record. The inventory is the detector, so its population is the guard. Round one filtered the scope on an outbound-network token list, which omitted git fetch — this repo's commonest remote read — so a hook that fetches origin/main and derives a push decision was invisible. Round two dropped the filter but used non-recursive glob, so four nested files stayed out, one of them calling a live ErsatzTV API and acting on the reply. Round three used rglob, which is recursive and therefore also enumerated .husky/_/ — untracked, gitignored shims that npm ci generates — leaving the guard red on every developer checkout and green in CI, which never runs npm ci. A guard that fails everywhere except where it runs teaches its readers to ignore it, which is worse than no guard at all.

Every round shipped with an argument for why the traversal was sufficient, and every argument was wrong the same way. The fix that finally held was not a better traversal: it was deriving the population from git ls-files. The filesystem is not an authoritative source — it reports build output, editor droppings and whatever else is on disk, and it differs per machine. The index is authoritative, versioned, identical for CI and every checkout, and excludes untracked generated files by construction rather than by an exclusion list somebody has to maintain. So the generalisation is the one testing.guard-derives-population-from-source already states, applied one level up: when a guard enumerates a population, ask what is the authoritative list of these things — and if the answer is "whatever the walk finds", the guard is not finished, however carefully the walk is written.

Grade down before you argue. Three cold-review rounds demoted row after row — the scheduled-merge path, both merge-consent head/base reads, the file enumerator, every registry-tag row — and in each case the row asserted a property the code did not have while the code beneath it was fine. Only three rows survive as PINNED, out of roughly seventy. That is the honest finding about this class in a tooling codebase: almost nothing that talks to a remote service is genuinely pinned, most of it is bounded by an argument, and the argument is what has to be written down.

The denominator is deliberately approximate, and that is a finding rather than laziness. Written as an exact "N of M" it went stale three times in three rounds — twice because a demotion landed after the count, once because splitting one row into two moved M inside the very commit that cited it. A figure that changes whenever the artifact it describes is edited is a second copy of that artifact, and this repo already knows what to do about a hand-maintained mirror: give it an equality check or stop maintaining it. docs/guard-inventory.md took the first route because its counts are the point; a rationale record takes the second, because the load-bearing claim here is "almost nothing is pinned", not any particular integer. A row that overstates is worse than a row that admits a gap, because this repo's own record is that a guard described as sound stops being re-examined.

What this record does not claim. The inventory grades files, so it cannot see an existing file growing a second unpinned read — the sites-in-code limit tracked in #777. And nothing verifies that a row marked PINNED is telling the truth. Both residues are stated in docs/remote-state-inventory.md rather than papered over, because a guard described as sound stops being re-examined, which is the failure mode this whole family is made of.