feat(778): name the check-and-use race, and verify the protection the merge gate rests on (#802)
Build ErsatzTV Image / Build & test (.NET) (push) Successful in 16s
Build ErsatzTV Image / Delimiter ban (release path) (push) Successful in 19s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (push) Successful in 13s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (push) Successful in 12s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (push) Skipped
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (push) Skipped
Build ErsatzTV Image / Build & push image (amd64) (push) Successful in 5m50s
Build ErsatzTV Image / Build & test (.NET) (push) Successful in 16s
Build ErsatzTV Image / Delimiter ban (release path) (push) Successful in 19s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (push) Successful in 13s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (push) Successful in 12s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (push) Skipped
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (push) Skipped
Build ErsatzTV Image / Build & push image (amd64) (push) Successful in 5m50s
This commit was merged in pull request #802.
This commit is contained in:
@@ -8,8 +8,12 @@
|
||||
# LATEST commit was reviewed, not a stale earlier diff (the ersatztv#242 failure mode:
|
||||
# "re-review the fix commit, not just the initial PR diff").
|
||||
#
|
||||
# EVERY ONE OF THOSE IS A SNAPSHOT, taken when the merge tool is called. That is sound for an
|
||||
# immediate merge and UNSOUND for a scheduled one: with merge_when_checks_succeed, Gitea merges
|
||||
# EVERY ONE OF THOSE IS A SNAPSHOT, taken when the merge tool is called. The window is SMALL for an
|
||||
# immediate merge and UNBOUNDED for a scheduled one. Small is not zero, and this comment used to say
|
||||
# "sound", which is the overclaim ersatztv#778 removed: this hook returns `allow` and a SEPARATE call
|
||||
# performs the merge, so a push can still land in between. The merge API accepts an optional
|
||||
# `head_commit_id` that would make that call a true compare-and-set; a PreToolUse hook cannot add an
|
||||
# argument, only refuse without one. With merge_when_checks_succeed, Gitea merges
|
||||
# later, against whatever head is green then (ersatztv#622). So the sha-bound half of H10 is
|
||||
# enforced by the SERVER, not here — `review-verdict/h10` is a required status check on `main`,
|
||||
# written per-sha by scripts/post-review-verdict.sh, and a new commit cannot inherit it. This hook
|
||||
@@ -175,10 +179,39 @@ fi
|
||||
# after which a later, successful status read could still auto-grant. A transient failure would then
|
||||
# have produced a "merge gate: satisfied" message for a comparison that never happened. Every
|
||||
# unreadable input here therefore falls through to a human (`ask`), never to silence.
|
||||
live_base=$(printf '%s' "$prjson" | jq -r '.base.ref // ""' 2>/dev/null || true)
|
||||
if [ -z "$live_base" ]; then
|
||||
# RE-READ THE BASE HERE, ONCE, FOR EVERY PATH BELOW (ersatztv#778).
|
||||
#
|
||||
# "Below" is literal, and the one consumer ABOVE is disclosed rather than implied: the docs-only
|
||||
# enumeration still runs against the snapshot `$base_ref` and can `decide allow` before reaching
|
||||
# this point. That is bounded and deliberate — a docs-only match is a PASSTHROUGH to the ordinary
|
||||
# human prompt, never an auto-grant, so a stale base there costs a prompt someone was going to see
|
||||
# anyway. Every path that can GRANT passes through the check below.
|
||||
#
|
||||
# `$base_ref` above comes from the PR snapshot taken at the top of this hook, and the docs-only
|
||||
# enumeration between there and here is up to forty round trips. A PERSISTENT retarget in that gap
|
||||
# needs no ABA and no force-push: every base-dependent decision below would be formed against a
|
||||
# branch the PR no longer targets. Checking a stale identifier is not checking — which is the whole
|
||||
# of `process.check-and-use-pins-a-version`, so the guard enforcing that rule must not break it.
|
||||
#
|
||||
# This re-read first landed inside the scheduled-auto-merge branch only, which fixed the branch-
|
||||
# protection lookup and left the #632 retarget DETECTION below still reading the stale snapshot. Cold
|
||||
# review demonstrated the consequence with this repo's own fixture: scheduled+retarget denied, while
|
||||
# immediate+retarget auto-GRANTED. That is the twin-missed shape — a fix applied to the path where it
|
||||
# was noticed — so the re-read is hoisted above every consumer rather than duplicated into each.
|
||||
prjson_now=$(gq "repos/$owner/$repo/pulls/$pr")
|
||||
if [ -z "${prjson_now//[[:space:]]/}" ] || ! printf '%s' "$prjson_now" | jq -e 'type == "object"' >/dev/null 2>&1; then
|
||||
decide ask "H10 merge gate: could not re-read PR #$pr to confirm it still targets '$base_ref' before checking the verdict against it. Confirm the target branch, then merge."
|
||||
fi
|
||||
base_now=$(printf '%s' "$prjson_now" | jq -r '.base.ref // ""' 2>/dev/null || true)
|
||||
if [ -z "$base_now" ]; then
|
||||
decide ask "H10 merge gate: PR #$pr reports no base branch (.base.ref), so the verdict cannot be checked against the branch it was formed for (ersatztv#632). Confirm the PR still targets the branch it was reviewed against before merging."
|
||||
fi
|
||||
if [ -n "$base_ref" ] && [ "$base_now" != "$base_ref" ]; then
|
||||
decide deny "H6/H10 merge gate: BLOCKED — PR #$pr was retargeted from '$base_ref' to '$base_now' while this gate was evaluating. Every check formed against '$base_ref', including the changed-file enumeration and the review verdict, describes a merge that is no longer the one being requested (ersatztv#632). Re-review against '$base_now' and run: scripts/post-review-verdict.sh $pr MERGEABLE"
|
||||
fi
|
||||
# From here on both names are the freshly-confirmed base; they are equal by the check above.
|
||||
base_ref=$base_now
|
||||
live_base=$base_now
|
||||
if [ -n "$sha" ]; then
|
||||
# This is the THIRD read of this endpoint in a worst-case hook run (the ordinary-CI branch and the
|
||||
# scheduled-auto-merge branch each do their own). Sharing one snapshot would close a narrow
|
||||
@@ -308,8 +341,19 @@ else
|
||||
# status decide this. Here the fallthrough happens to land on `vstate=""` -> deny (fail-CLOSED,
|
||||
# so this was never a hole), but it would have surfaced the wrong message — a "BLOCKED, no
|
||||
# verdict" deny instead of the "could not read the status" ask this branch exists to give.
|
||||
if [ -z "${vjson//[[:space:]]/}" ] || ! printf '%s' "$vjson" | jq -e '.statuses | type == "array"' >/dev/null 2>&1; then
|
||||
decide ask "H6/H10 merge gate: could not read the 'review-verdict/h10' status for PR #$pr head ${sha:0:7} (Gitea unreachable or an unexpected response). Confirm the current head is reviewed before scheduling an auto-merge."
|
||||
# Validate the MEMBERS, not just the array. `.statuses | type == "array"` passes for
|
||||
# `{"statuses":[1]}`, and the extraction below then errors with "Cannot index number with string"
|
||||
# and exits 5 — which, under `set -e`, aborts this hook with NO JSON on stdout at all. A consent
|
||||
# hook that emits nothing has violated its own contract: it neither grants, denies nor asks. Same
|
||||
# one-level-down swallow as the #632 base-change guard and the branch-protection shape check
|
||||
# below; the validation domain must match the CONSUMPTION domain (ersatztv#778).
|
||||
if [ -z "${vjson//[[:space:]]/}" ] \
|
||||
|| ! printf '%s' "$vjson" \
|
||||
| jq -e '(.statuses | type == "array")
|
||||
and all(.statuses[]; type == "object"
|
||||
and ((.context | type) == "string")
|
||||
and ((.status | type) == "string"))' >/dev/null 2>&1; then
|
||||
decide ask "H6/H10 merge gate: could not read the 'review-verdict/h10' status for PR #$pr head ${sha:0:7} (Gitea unreachable, or a response whose status rows are not the expected shape). Confirm the current head is reviewed before scheduling an auto-merge."
|
||||
fi
|
||||
vstate=$(printf '%s' "$vjson" | jq -r '[.statuses[] | select(.context == "review-verdict/h10")] | first | .status // ""')
|
||||
case "$vstate" in
|
||||
@@ -318,6 +362,210 @@ else
|
||||
pending) decide deny "H6/H10 merge gate: BLOCKED — 'review-verdict/h10' is still pending on PR #$pr head ${sha:0:7} (no verdict posted for this commit yet). Review the current head and run: scripts/post-review-verdict.sh $pr MERGEABLE" ;;
|
||||
*) decide deny "H6/H10 merge gate: BLOCKED — 'review-verdict/h10' is '$vstate' on PR #$pr head ${sha:0:7}. Resolve the findings, then run: scripts/post-review-verdict.sh $pr MERGEABLE" ;;
|
||||
esac
|
||||
|
||||
# --- The mitigation this path RESTS on, verified instead of asserted (ersatztv#778). -----------
|
||||
# Everything above proves a property of the head that exists NOW. What makes that safe under
|
||||
# merge_when_checks_succeed is stated in the paragraph opening this branch: `review-verdict/h10`
|
||||
# is a REQUIRED status check on the base, a commit status belongs to exactly ONE sha, so a commit
|
||||
# pushed after scheduling cannot inherit it and Gitea's own gate refuses the merge.
|
||||
#
|
||||
# That guarantee is branch-protection CONFIG. It lives outside this repo, no code here owned it,
|
||||
# and until #778 nothing compared the two — so the grant reason handed to a human cited a
|
||||
# protection that could have been switched off with no signal anywhere. The comment above and the
|
||||
# grant string below are claims about the past; a dated claim is not a check.
|
||||
#
|
||||
# This is the hook's OWN defect class (#778 / `process.check-and-use-pins-a-version`): a check
|
||||
# ("a later push clears the status") authorizes an action ("arm an auto-merge that Gitea completes
|
||||
# later") over state that can change in between, with nothing pinning it. The read here does not
|
||||
# pin anything either — branch protection can still be edited after this call — but it converts an
|
||||
# ASSUMPTION that was never observed into a precondition that is, which is the honest ceiling for
|
||||
# a config whose API offers no version, ETag or conditional read.
|
||||
#
|
||||
# Tri-state, matching this file's idiom throughout: unreadable -> ask (a human adjudicates),
|
||||
# present -> proceed, ABSENT -> deny. Absence is not a degraded read; it is #622's hole reopened,
|
||||
# and the whole point of that issue is that the failure is silent from the merge caller's side.
|
||||
# Belt-and-braces: `$base_ref` was proven non-empty and re-confirmed at the hoisted check above,
|
||||
# so this cannot fire today. Kept because it is the precondition this block's URL depends on, and
|
||||
# a future edit that moves either piece should fail loudly here rather than request a URL with an
|
||||
# empty path segment.
|
||||
[ -n "$base_ref" ] || decide ask "H6/H10 merge gate: could not resolve PR #$pr's base branch, so the 'review-verdict/h10' required-check protection that makes a scheduled auto-merge safe (ersatztv#622) can't be confirmed. Verify branch protection on the base, or merge immediately instead of scheduling."
|
||||
# The base was re-read and confirmed unchanged above, for every path — see the hoist comment
|
||||
# there. It is deliberately NOT re-read a second time here: two reads would create a window
|
||||
# between them for no gain, and the hoisted check already covers the enumeration gap that made
|
||||
# this necessary.
|
||||
# A read failure here is NOT evidence about the branch. The deleted by-name endpoint answered 404
|
||||
# for "no rule with this name", which was a finding; the LIST endpoint's 404 means the repo was not
|
||||
# found or is invisible to this credential, which is a read failure. Absence is now established by
|
||||
# the classifier returning `nomatch` over a list that WAS read, never by an HTTP status.
|
||||
# ALWAYS enumerate the rule LIST; never look a rule up by name. The by-name endpoint
|
||||
# (`branch_protections/{name}`) is an exact DB lookup — `GetProtectedBranchRuleByName` — which
|
||||
# performs no matching and knows nothing about precedence, so a 200 from it means only "a rule
|
||||
# with this NAME exists and lists this context", never "this context is required on this branch".
|
||||
#
|
||||
# It was used first, with the list consulted only on a 404, and cold review found what that left
|
||||
# behind: the precedence argument below guarded the 404 path while the 200 path — the one this
|
||||
# repo actually takes — granted without it. Given a rule `main` requiring `review-verdict/h10` and
|
||||
# a rule `m*` with better Priority that does not, Gitea applies `m*`, and the by-name hit on
|
||||
# `main` granted anyway. The hardened path was dead code and the unhardened one was live. Deleting
|
||||
# the twin rather than documenting it is the point: one fetch, one classifier, one argument, and
|
||||
# no second path to keep in step. The ref no longer reaches a URL segment, so it needs no
|
||||
# encoding either.
|
||||
bp_file=$(mktemp) || decide ask "H6/H10 merge gate: could not allocate a temp file to read branch protection for '$base_ref'. Confirm the 'review-verdict/h10' required check manually before scheduling an auto-merge."
|
||||
if [ -n "${ETV_GITEA_TOKEN:-}" ]; then
|
||||
bp_code=$(curl -s -o "$bp_file" -w '%{http_code}' -H "Authorization: token $ETV_GITEA_TOKEN" "$base_url/repos/$owner/$repo/branch_protections" 2>/dev/null || true)
|
||||
else
|
||||
bp_code=$(curl -s -o "$bp_file" -w '%{http_code}' -u "$ETV_GITEA_BASICAUTH" "$base_url/repos/$owner/$repo/branch_protections" 2>/dev/null || true)
|
||||
fi
|
||||
bp_list=$(cat "$bp_file" 2>/dev/null || true)
|
||||
bp=""
|
||||
if [ "$bp_code" = "200" ] && printf '%s' "$bp_list" | jq -e 'type == "array"' >/dev/null 2>&1; then
|
||||
# DO NOT claim parity with Gitea's matcher — this code cannot have it, and asserting it would
|
||||
# be the exact defect this PR records (a mitigation outside the code, asserted rather than
|
||||
# verified). Gitea compiles a rule name with gobwas/glob and a `/` separator, so its `*` does
|
||||
# NOT cross a slash, `?`/`[…]`/`{a,b}` are wildcards, and a plain name is folded case-
|
||||
# insensitively. Reimplementing that here would be a second copy of somebody else's parser.
|
||||
#
|
||||
# So the classification is deliberately THREE-way, and each arm is safe without knowing the
|
||||
# dialect:
|
||||
# exact — no glob rule could apply, AND some rule name has no glob metacharacter and
|
||||
# equals the base case-insensitively. Only then is a single rule decidable.
|
||||
#
|
||||
# UNDECIDABLE IS EVALUATED FIRST, and the order is the point. Gitea picks the
|
||||
# governing rule with `GetFirstMatched` over a list sorted by Priority, THEN
|
||||
# by plain-name-ness — so a glob rule with a better Priority outranks an
|
||||
# exactly-named one. Preferring `exact` would therefore inspect a rule Gitea
|
||||
# might not be applying: if the exact rule requires `review-verdict/h10` and a
|
||||
# higher-priority glob rule does not, the gate auto-grants on a base where the
|
||||
# check is not enforced. Asking whenever ANY glob rule could apply is sound
|
||||
# without knowing the precedence rules at all, which is the only claim this
|
||||
# code is entitled to make about somebody else's resolver.
|
||||
#
|
||||
# Case folding is ASCII-only here, while Gitea's `EqualFold` is
|
||||
# Unicode-aware — so a rule `ünstable` and a base `Ünstable` fold equal there
|
||||
# and not here. ASCII-fold equality implies EqualFold equality, so the gap can
|
||||
# only MISS a match, never invent one; but a miss lands on `none`, which
|
||||
# DENIES with the stated cause that no rule can govern the base. The backslash
|
||||
# paragraph below rejects "nearly unreachable" as a standard for that arm, and
|
||||
# the same standard has to apply here, so a rule name carrying any non-ASCII
|
||||
# byte is `undecidable` rather than fold-compared. Two fold-equal plain names
|
||||
# are undecidable too: this code picks by list order while Gitea picks by
|
||||
# Priority, and guessing which one is enforced is the defect the arm order
|
||||
# above exists to avoid.
|
||||
# undecidable — some glob rule COULD govern this base. Tested with a provable SUPERSET of any
|
||||
# glob dialect: literal prefix before the first metacharacter, `.*`, literal
|
||||
# suffix after the last. If even that does not match, no dialect can, because
|
||||
# every dialect requires the literal head and tail to match literally.
|
||||
#
|
||||
# BACKSLASH counts as a metacharacter for that purpose, and it is the one case that breaks the
|
||||
# superset proof if it does not. gobwas/glob reads `\{` as a LITERAL brace, so a rule `a\{b`
|
||||
# governs the base `a{b` — while a superset that treated `\` as literal would build `a\.*b`,
|
||||
# fail to match, and answer `none`, i.e. deny a base that IS protected. Git ref rules make this
|
||||
# nearly unreachable (a branch name may not contain `*`, `?`, `[` or `\`, though it MAY contain
|
||||
# `{`), but `none` is the arm that authorises a DENY on the stated grounds "nothing can govern
|
||||
# this base", so its premise has to hold unconditionally rather than usually.
|
||||
# none — nothing can possibly govern the base, so it is genuinely unprotected.
|
||||
#
|
||||
# `undecidable` asks rather than granting or denying. Over-matching would auto-grant on a base
|
||||
# whose protection we never established (#622's hole, reached through the block written to
|
||||
# close it); under-matching would deny with a stated cause that is false, which this block's
|
||||
# own comment calls the worse outcome. Asking is the only answer that is honest in both
|
||||
# directions, and it is rare in practice: as of 2026-08-19 this repo's only rule is the plain
|
||||
# name `main`, which the classifier resolves to `exact` on every run. That is a dated
|
||||
# observation about mutable remote config, not a property to rely on.
|
||||
bp_verdict=$(printf '%s' "$bp_list" | jq --arg b "$base_ref" -c '
|
||||
def esc: gsub("(?<c>[.+?^${}()|\\[\\]\\\\])"; "\\" + .c);
|
||||
def offs: [match("[*?\\[\\]{}\\\\]"; "g").offset];
|
||||
def superset: . as $n | (offs) as $o
|
||||
| ($n[0:$o[0]] | esc) + ".*" + ($n[($o[-1]+1):] | esc);
|
||||
def nonascii: explode | any(. > 127);
|
||||
. as $rules | $b as $base |
|
||||
($rules | map(select((.branch_name // .rule_name // "") as $n
|
||||
| (($n|offs|length) == 0)
|
||||
and (($n|ascii_downcase) == ($base|ascii_downcase))))) as $exacts |
|
||||
(($base|nonascii) or ($rules | any((.branch_name // .rule_name // "") as $n
|
||||
| ($n|offs|length) == 0 and ($n|nonascii)))) as $unfoldable |
|
||||
if ($rules | any((.branch_name // .rule_name // "") as $n
|
||||
| (($n|offs|length) > 0)
|
||||
and ($base | test("^" + ($n|superset) + "$")))) then {verdict:"undecidable"}
|
||||
elif $unfoldable then {verdict:"undecidable"}
|
||||
elif ($exacts | length) > 1 then {verdict:"undecidable"}
|
||||
elif ($exacts | length) == 1 then {verdict:"exact", rule:($exacts | first)}
|
||||
else {verdict:"none"} end' 2>/dev/null || true)
|
||||
case $(printf '%s' "$bp_verdict" | jq -r '.verdict // ""' 2>/dev/null || true) in
|
||||
exact) bp=$(printf '%s' "$bp_verdict" | jq -c '.rule' 2>/dev/null || true); bp_code=200 ;;
|
||||
undecidable) rm -f "$bp_file"
|
||||
decide ask "H6/H10 merge gate: no branch-protection rule on this repo governs '$base_ref' decidably — a GLOB rule could govern it, or two rule names fold-equal, or a name is non-ASCII. This hook deliberately does not reimplement Gitea's glob matcher, so whether 'review-verdict/h10' is required on this base cannot be derived here (ersatztv#778). Confirm it in the repo's branch-protection settings, or merge immediately instead of scheduling." ;;
|
||||
none) bp_code=nomatch; bp="" ;;
|
||||
*) bp_code=unreadable-rules; bp="" ;;
|
||||
esac
|
||||
else
|
||||
# A 200 whose body is NOT an array never reaches the classifier — it is diverted by the array
|
||||
# gate above — so it needs the same sentinel, or the generic ask below reports
|
||||
# "HTTP '200' — Gitea unreachable" about a read that plainly succeeded. Same defect as the
|
||||
# throw-inside-the-classifier arm, one branch earlier; fixing only the arm where it was noticed
|
||||
# is the twin-missed shape this PR is largely about.
|
||||
if [ "$bp_code" = "200" ]; then
|
||||
bp_code=unreadable-rules
|
||||
else
|
||||
bp_code=${bp_code:-000} # a real transport/HTTP failure -> the ask arm below
|
||||
fi
|
||||
bp=""
|
||||
fi
|
||||
rm -f "$bp_file"
|
||||
# `nomatch` is the CLASSIFIER's verdict, deliberately not an HTTP code. Reusing 404 for it made
|
||||
# this deny reachable from an HTTP 404 on the list read too — repo not found, or invisible to the
|
||||
# credential, which Gitea also answers 404 — and then the reason claimed "the full rule list was
|
||||
# read and none matches" about a read that never happened. A transport failure must reach the ask
|
||||
# below, not a deny stating a finding.
|
||||
if [ "$bp_code" = "nomatch" ]; then
|
||||
decide deny "H6/H10 merge gate: BLOCKED — no branch-protection rule on this repo can govern '$base_ref' (the full rule list was read and none matches), so 'review-verdict/h10' is not a required check on it. A scheduled auto-merge is safe ONLY because that per-sha required check stops a commit pushed after scheduling from merging unreviewed (ersatztv#622). Restore branch protection on '$base_ref', or merge immediately (without merge_when_checks_succeed) once CI is green."
|
||||
fi
|
||||
# `unreadable-rules` is the CLASSIFIER failing on a 200 it could not parse — a numeric
|
||||
# `branch_name` makes jq throw, and `//` does not catch it because it fires only on null/false.
|
||||
# It gets its own sentinel for the same reason `nomatch` does: reporting "HTTP '000' — Gitea
|
||||
# unreachable" about a successful 200 read states a cause that did not happen, which is the defect
|
||||
# fixed one arm over for the deny.
|
||||
if [ "$bp_code" = "unreadable-rules" ]; then
|
||||
decide ask "H6/H10 merge gate: this repo's branch-protection rules came back in a shape this hook could not parse, so whether 'review-verdict/h10' is required on '$base_ref' is unknown. Check the rules manually, or merge immediately instead of scheduling."
|
||||
fi
|
||||
if [ "$bp_code" != "200" ] || [ -z "${bp//[[:space:]]/}" ] || ! printf '%s' "$bp" | jq -e 'type == "object"' >/dev/null 2>&1; then
|
||||
decide ask "H6/H10 merge gate: could not read this repo's branch-protection rules (HTTP '${bp_code:-none}' — Gitea unreachable, or these credentials lack the repo-admin scope that endpoint needs), so whether 'review-verdict/h10' is required on '$base_ref' is unknown. Scheduling an auto-merge is only safe while 'review-verdict/h10' is a REQUIRED check there (ersatztv#622) — confirm that manually, or merge immediately instead of scheduling."
|
||||
fi
|
||||
# The membership test is `any(.[]; . == …)` over a value FIRST PROVEN to be an array of strings —
|
||||
# never `index()`. `index` on a STRING is substring search, so a `status_check_contexts` that
|
||||
# arrived as the string "prefix-review-verdict/h10-suffix" would answer "yes" and auto-grant a
|
||||
# merge on a base where no such context is required. That is a FALSE-OPEN in the gate, reachable
|
||||
# from any payload shape drift, and it is the direction that matters: a false-closed costs a
|
||||
# prompt, a false-open costs an unreviewed merge.
|
||||
#
|
||||
# Validating `$bp` as an object does not make its MEMBERS well-formed, which is the same
|
||||
# one-level-down swallow that survived the first fix in the #632 base-change guard — the
|
||||
# validation domain has to match the CONSUMPTION domain, not stop at the top-level type. So the
|
||||
# shape is checked explicitly and anything else becomes "unknown" rather than a decision.
|
||||
#
|
||||
# `null` and `[]` are legitimate (an unprotected-in-practice branch) and answer "no", not
|
||||
# "unknown": absent IS the finding here, not a read failure. The word is then matched
|
||||
# exhaustively, because "" is not a third synonym for "no".
|
||||
# `// []` defaults on FALSE as well as on null, because jq's alternative operator fires for both.
|
||||
# So `"status_check_contexts": false` — a malformed shape — became `[]` and answered "no", i.e. a
|
||||
# confident DENY derived from a payload that was never understood. Absent and null are defaulted
|
||||
# explicitly; every other non-array is "unknown".
|
||||
# `enable_status_check` is validated as a BOOLEAN before it is trusted, for the same reason the
|
||||
# contexts list is: `"true"` (the string) is not `true`, and comparing it to `true` yields a
|
||||
# confident "no" -> deny derived from a payload never understood. Every malformed shape on this
|
||||
# endpoint has to reach the same "unknown" -> ask arm, or the tri-state is only two states.
|
||||
guarded=$(printf '%s' "$bp" \
|
||||
| jq -r 'def ctxs: if (has("status_check_contexts") | not) or .status_check_contexts == null
|
||||
then [] else .status_check_contexts end;
|
||||
if (.enable_status_check | type) != "boolean" then "unknown"
|
||||
elif (ctxs | type) != "array" or any(ctxs[]; type != "string") then "unknown"
|
||||
elif (.enable_status_check == true) and any(ctxs[]; . == "review-verdict/h10") then "yes"
|
||||
else "no" end' 2>/dev/null || true)
|
||||
case "$guarded" in
|
||||
yes) : ;;
|
||||
no) decide deny "H6/H10 merge gate: BLOCKED — 'review-verdict/h10' is NOT a required status check on '$base_ref' (branch protection reports enable_status_check/status_check_contexts without it). A scheduled auto-merge is safe ONLY because that per-sha required check stops a commit pushed after scheduling from merging unreviewed (ersatztv#622); without it, arming merge_when_checks_succeed freezes consent at a head Gitea may not be the one to merge. Restore it in branch protection, or merge immediately (without merge_when_checks_succeed) once CI is green." ;;
|
||||
*) decide ask "H6/H10 merge gate: branch protection for '$base_ref' came back in an unexpected shape, so the 'review-verdict/h10' required check that makes a scheduled auto-merge safe (ersatztv#622) could not be confirmed either way. Check it manually, or merge immediately instead of scheduling." ;;
|
||||
esac
|
||||
fi
|
||||
|
||||
# --- (c) Review-verdict freshness (ersatztv#303 H10): a review-verdict comment must reference the
|
||||
@@ -372,7 +620,7 @@ if [ "$class" = "positive" ]; then
|
||||
# path this hook never read the CI status at all (it is delegated to Gitea), so saying "CI green"
|
||||
# there was a plain falsehood in the one message a human reads to decide whether to trust the gate.
|
||||
if [ "$mwcs" = "true" ]; then
|
||||
decide grant "H6/H10 merge gate: satisfied — all Done-when boxes ticked, and both a positive Review-verdict comment and the 'review-verdict/h10' status cover the current head ($short). CI is gated by Gitea (merge_when_checks_succeed), and because the verdict status is bound to this sha, a commit pushed before Gitea merges will clear it and block the merge (ersatztv#622). Auto-granted."
|
||||
decide grant "H6/H10 merge gate: satisfied — all Done-when boxes ticked, and both a positive Review-verdict comment and the 'review-verdict/h10' status cover the current head ($short). CI is gated by Gitea (merge_when_checks_succeed). A commit pushed before Gitea merges clears the sha-bound verdict status and is blocked by the 'review-verdict/h10' required check (ersatztv#622) — which this hook has just CONFIRMED is still required on '$base_ref' — read from the repo's full rule list and matched with Gitea's own plain-vs-glob split, refusing rather than guessing wherever precedence or folding is not derivable. That guarantee holds while that branch protection stands; if it is weakened after this check, nothing here would see it (ersatztv#778). Auto-granted."
|
||||
fi
|
||||
decide grant "H6/H10 merge gate: satisfied — CI green, all Done-when boxes ticked, and a positive Review-verdict references the current head ($short). Auto-granted (no separate confirmation needed)."
|
||||
fi
|
||||
|
||||
@@ -26,6 +26,7 @@ doc below, or that changes which sections a task signal points to.**
|
||||
| CI / release pipeline work | `docs/ci-cd.md` + `docs/decisions/release-ci-governance.md` |
|
||||
| Proposing a new guard / CI check / regression test convention | `docs/defect-shapes-773.md` §4 (detector menu + the classes where no detector is plausible), then the two rules every guard must satisfy: `docs/decisions/records/testing/guard-derives-population-from-source.md` and `…/guard-ships-with-mutation-proof.md` |
|
||||
| Adding / changing / deleting a guard file | `docs/guard-inventory.md` — every guard's row is machine-checked by `scripts/tests/test_guard_inventory.py`, so a new guard must acquire a row before the suite goes green |
|
||||
| Writing code that reads live Gitea/remote state and then acts on it | `docs/decisions/records/process/check-and-use-pins-a-version.md`, then `docs/remote-state-inventory.md` — a new executable under `scripts/` (**excluding `scripts/tests/`**), `.claude/hooks/`, `.husky/` or `.gitea/workflows/` must acquire a row there before `scripts/tests/test_remote_state_inventory.py` goes green |
|
||||
| Finding every site that references a symbol (multi-site fix/sweep) | `docs/local-lsp-tooling.md` — which surface answers, and why a delegated agent must be pointed at the `csharp-lsp` MCP tools rather than the `LSP` tool |
|
||||
| Live local run / Playwright-MCP verification | `docs/e2e-local.md` + `scripts/e2e-local.sh` |
|
||||
| Adding/changing a UI-E2E browser flow | `docs/e2e-local.md` → "UI-E2E harness" + `scripts/e2e-ui.sh` |
|
||||
@@ -112,6 +113,15 @@ bounds, what's mined per issue): `docs/handoffs/chicorytv-issue-queue.md` → "K
|
||||
and an audit of which configured hooks/MCP servers/LSPs are actually invoked. Read it before
|
||||
proposing a new guard or CI check — §4 is the detector menu, and it argues against enumerating
|
||||
cases one incident at a time.
|
||||
- **`docs/remote-state-inventory.md`** — every executable in `scripts/` (**excluding
|
||||
`scripts/tests/`**), `.claude/hooks/`, `.husky/` and `.gitea/workflows/` that reads live remote
|
||||
state and acts on that read, classified `PINNED` / `CAS` / `UNSAFE-KNOWN` / `N/A` with the window
|
||||
and what bounds it. Code outside those directories — C#/TypeScript guards, `web/`, and the test
|
||||
suites themselves — is out of scope, and the doc states that rather than implying coverage.
|
||||
The population is derived from `git ls-files` and compared for set equality by
|
||||
`scripts/tests/test_remote_state_inventory.py`, so a new script that talks to a remote service
|
||||
cannot ship unclassified. Read it with `process.check-and-use-pins-a-version`; it is that record's
|
||||
detector, since the class has no plausible linter (`docs/defect-shapes-773.md` §4 detector D).
|
||||
- **`docs/guard-inventory.md`** — every executable guard file, what it blocks, whether it is a
|
||||
`GUARD` or `TOOLING`, and whether it ships a mutation proof (`MUTATION` / `BEHAVIOUR-ONLY` /
|
||||
`NONE`) with a `file::function` ref. The population is derived from the filesystem and the
|
||||
|
||||
@@ -106,6 +106,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `process.bom-format-detection-recipe` | Before any push touching `.cs`, detect BOMs with the `od -A n -t x1 -N 3` byte check and verify the format gate with `dotnet format --include` run under `bash -c`, never bare zsh. NOT `xxd`: it ships with vim and is absent on plain Linux hosts including this repo's CI runner, where the substitution yields empty, never matches, and the check reports all-clean — the same all-clean-detector failure this record was written about, in the detector it prescribed. | 2026-07-21 | [link](records/process/bom-format-detection-recipe.md) |
|
||||
| `process.branch-off-feature-branch` | To fix work on an unmerged feature branch, branch off that branch and land by fast-forward push — and after creating a worktree, drive the first Edit/Read from ITS absolute paths and `git status` it before building. | 2026-07-21 | [link](records/process/branch-off-feature-branch.md) |
|
||||
| `process.build-concurrency-limits` | Run at most 3–4 concurrent dotnet/npm builds on this Mac, gate launches on FREE RAM rather than CPU load, and never set `ETV_UPDATE_GOLDENS` / `ETV_UPDATE_PLAYOUT_GOLDENS`. | 2026-07-21 | [link](records/process/build-concurrency-limits.md) |
|
||||
| `process.check-and-use-pins-a-version` | 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. | 2026-08-16 | [link](records/process/check-and-use-pins-a-version.md) |
|
||||
| `process.codex-cheap-worker-launch` | For bounded tool-bearing selector/recon work, launch a Codex worker with `codex exec -m gpt-5.4-mini -c model_reasoning_effort=low -s read-only`; `spawn_agent` buys parallelism but no cost savings. | 2026-07-21 | [link](records/process/codex-cheap-worker-launch.md) |
|
||||
| `process.consistency-fix-new-code-scrutiny` | Review a "make X consistent with Y" change as new code, not as a mechanical copy — and for any timer or effect involved, ask explicitly "when does this fire?", including on mount. | 2026-07-21 | [link](records/process/consistency-fix-new-code-scrutiny.md) |
|
||||
| `process.enumerate-workaround-behaviors-before-deleting` | When an issue says "delete X", enumerate every behavior X provided before removing it — a workaround often serves a second purpose that outlives the first. | 2026-07-21 | [link](records/process/enumerate-workaround-behaviors-before-deleting.md) |
|
||||
|
||||
@@ -0,0 +1,138 @@
|
||||
---
|
||||
key: process.check-and-use-pins-a-version
|
||||
title: '2026-08-16 — a check and the action it authorizes are bound to one version, or the gap is stated and fenced (#778)'
|
||||
status: active
|
||||
since: '2026-08-16'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: '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.'
|
||||
signals: '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'
|
||||
mechanics: '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 `die`s 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.
|
||||
@@ -120,15 +120,17 @@ all.
|
||||
| `scripts/tests/test_jq_preflight.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_merge_consent_base_change.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_merge_consent_exemption.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_merge_consent_required_check.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_migration_equivalence.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_post_review_verdict.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_pr_changed_files.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_prepush_rebase_check_tag_exemption.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_prove_fix.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_remote_state_inventory.py` | the `script-tests` job, on an executable that talks to a remote service with no row in `docs/remote-state-inventory.md` | GUARD | MUTATION | `test_remote_state_inventory.py::test_MUTATION_PROOF_a_dropped_row_and_a_phantom_row_are_both_detected` |
|
||||
|
||||
## What the numbers say
|
||||
|
||||
33 guards, 5 tooling scripts, 14 proof files. **7 guards carry a mutation proof; 6 are
|
||||
34 guards, 5 tooling scripts, 15 proof files. **8 guards carry a mutation proof; 6 are
|
||||
behaviour-only; 20 have none.** These figures are asserted against the table by
|
||||
`test_the_summary_counts_match_the_table` — they were wrong in the first draft (28/4/6/3/19 against
|
||||
a table holding 27/5/6/3/18), because a hand-maintained summary of a table is a second copy of it,
|
||||
|
||||
@@ -0,0 +1,187 @@
|
||||
# Remote-state inventory (ersatztv#778)
|
||||
|
||||
Every **git-tracked** file matching one of these, that reads live remote state and later acts on
|
||||
that read, and whether the read is bound to something that cannot change underneath it:
|
||||
|
||||
| Directory | Files |
|
||||
|---|---|
|
||||
| `scripts/` (recursive, **excluding `scripts/tests/`**) | `*.sh`, `*.py` |
|
||||
| `.claude/hooks/` | `*.sh` |
|
||||
| `.husky/` | all tracked files |
|
||||
| `.gitea/workflows/` | `*.yml`, `*.yaml` |
|
||||
|
||||
The per-directory extensions are stated because the prose once attached them to `scripts/` alone
|
||||
while the guard applied them everywhere, so a `.py` hook would have joined the scope the doc
|
||||
described and acquired no row.
|
||||
|
||||
The scope qualifier is load-bearing, not throat-clearing: this file twice claimed to cover "every
|
||||
executable in this repo" while its own derivation missed real remote readers, so the heading now
|
||||
states exactly what the guard enforces — including the `scripts/tests/` exclusion, so nobody adds a
|
||||
remote-reading test executable expecting a red guard that will stay green. C#/TypeScript guards,
|
||||
`web/`, and anything outside those directories are **not** covered. Scope is limit 3; the
|
||||
files-not-call-sites limit is limit 2.
|
||||
|
||||
`scripts/tests/test_remote_state_inventory.py` derives the population from `git ls-files` and
|
||||
asserts **set equality** against the `Site` column, so a new script that talks to a remote service
|
||||
cannot ship without acquiring a row.
|
||||
|
||||
**Read `docs/decisions/records/process/check-and-use-pins-a-version.md` before adding a row or
|
||||
changing a classification.**
|
||||
|
||||
## Why this file exists rather than a linter
|
||||
|
||||
`docs/defect-shapes-773.md` §4 detector D is a **fix pattern, not a mechanical detector**: there is
|
||||
no general lint for "this code should have pinned a sha." What makes the class actionable is that
|
||||
the population is small and enumerable, so the detector is detector A — derive the population from
|
||||
an authoritative source and assert set equality — applied to this inventory. The inventory is the
|
||||
artifact; the test keeps it from rotting.
|
||||
|
||||
## Columns
|
||||
|
||||
- **Class** —
|
||||
- `PINNED` — the read is bound to an immutable version identifier (a **full** commit sha, an
|
||||
image digest, a monotonic event count), and that binding still holds when the action runs. Two
|
||||
shapes qualify, and the second was missing from the first wording: either the action
|
||||
**re-validates** against the identifier immediately before committing (a snapshot nobody
|
||||
re-checks is not pinned — binding alone is never enough), **or** the check and the use are a
|
||||
single step over a value that cannot move, such as a workflow reading a full sha straight out of
|
||||
its own fixed event payload. What never qualifies is a value captured early and trusted later.
|
||||
The second shape is distinguished from an `N/A` row that says "resolution and use are one step"
|
||||
by the IDENTIFIER, not by the step count: `PINNED` requires the value itself to be immutable (a
|
||||
full sha, a digest), while a one-step read of a MUTABLE identifier — a registry tag, a branch
|
||||
name — is not pinned and is graded on what it authorizes.
|
||||
- `CAS` — the write itself carries a compare-and-set condition the server enforces.
|
||||
- `UNSAFE-KNOWN` — read-then-act with nothing pinning it, **accepted** with the reason stated in
|
||||
the Note. Every row here must say why the residual is tolerable, not merely that it exists.
|
||||
- `N/A` — reads no live remote state, or draws no authorization from what it reads.
|
||||
- **Note** — the window, and what closes or bounds it.
|
||||
|
||||
A row is about a *site*, not a file: a file with two independent reads gets two rows only where the
|
||||
classifications differ; otherwise the strictest applies and the Note names the exception.
|
||||
|
||||
## The inventory
|
||||
|
||||
### Hooks
|
||||
|
||||
| Site | Class | Note |
|
||||
|---|---|---|
|
||||
| `.claude/hooks/pretooluse-merge-consent.sh` — head-sha reads (CI status, H10 status, verdict comments) | `UNSAFE-KNOWN` | Every comparison uses the **full** 40-char `.head.sha` (`${sha:0:7}` appears only in human-readable strings), which is the right identifier — but it is captured ONCE from the PR snapshot at the top of the hook, and is stale in two distinct ways. FIRST, within the run: every later check (CI status, H10 status, verdict comments) is evaluated against that captured sha, so a push landing mid-run is checked against the commit it replaced. This is the same defect that WAS live for `$base_ref` until it was re-read before the branch-protection lookup; the sha is not re-read, and closing it symmetrically is tracked in **#803**. SECOND, after the decision: the hook returns `allow` and a separate call performs the merge. "No async window" was the second overclaim cold review removed from this file. Both are bounded the same way — a head the verdict does not cover cannot inherit the sha-bound required status, so the server refuses it — and neither is bounded by anything in this hook. The merge API accepts an optional `head_commit_id`, which would make the call a true CAS; a PreToolUse hook cannot ADD that argument, only refuse without it, and requiring it changes every merge call's shape — tracked as follow-up rather than smuggled in here. Accepted meanwhile because the window is one tool call wide with no scheduler in it, and the server-side per-sha required check still refuses an unreviewed head. |
|
||||
| `.claude/hooks/pretooluse-merge-consent.sh` — scheduled auto-merge (`merge_when_checks_succeed`) | `UNSAFE-KNOWN` | **Preflight, not a pin** — graded down by cold review, which was right: the hook's own comment concedes the branch-protection read pins nothing, so calling it `PINNED` contradicted this file's definition. What the hook proves is a snapshot; Gitea merges later; and since #778 it also verifies that `review-verdict/h10` is a required check, reading the repo's **full rule list** (never the by-name endpoint, which does no matching and knows nothing about precedence) — nothing can govern the base → deny, unreadable → ask, and a **glob rule that could govern it → ask**, because the hook deliberately does not reimplement Gitea's glob dialect. It also asks when two rule names fold equal, or when either name is non-ASCII, since Gitea's `EqualFold` is Unicode-aware and its rule precedence is not derivable here. That converts an unobserved assumption into an observed precondition and detects drift, but an admin can still weaken the protection *after* the read. Accepted, and the earlier wording here was **circular** — it said the residual was "closed one layer down" by the very branch protection an admin may have removed. It is not closed; it is BOUNDED, and the bound is a trust assumption that should be stated rather than dressed as a mechanism: everything on this path assumes repo-admin branch-protection config is not hostile. If protection is present at preflight and removed afterwards, an unreviewed head can merge, and nothing in this repo would detect it. What the check does buy is that the far commoner case — protection already weakened when the merge is attempted — stops being silent. A PreToolUse hook cannot add `head_commit_id` to the merge call, so it can never convert its own grant into a CAS — it can only refuse, which is what it now does. |
|
||||
| `.claude/hooks/pretooluse-merge-consent.sh` — base-retarget detection | `UNSAFE-KNOWN` | `recorded_base` parsed from the H10 status description is compared against the PR's live `.base.ref` (#632), and it deliberately compares the base **ref** rather than `base.sha` — the tip moves on every unrelated merge, so comparing it would deadlock every open PR. The residual is the same ABA the enumerator has: a name can be retargeted away and back, and the comparison cannot see it. Accepted because the alternative that CAN see it is the monotonic event count, which lives in the workflow that writes the enforced status rather than in this advisory hook. |
|
||||
| `.claude/hooks/pretooluse-merge-consent.sh` — `## Done-when` issue-body read | `UNSAFE-KNOWN` | The issue body carries an `updated_at` that is not used, so a box unticked between the read and the merge is invisible. Accepted: the only actor who can edit the issue is the one requesting the merge, so this is a self-inflicted race with no adversary and no silent-failure mode. |
|
||||
| `.claude/hooks/prepush-donewhen.sh` — `## Done-when` issue-body read | `UNSAFE-KNOWN` | No pin, but the hook's exit code gates the push synchronously — git blocks on this process. Blast radius is near zero regardless: `main` carries `enable_push: false` and `block_admin_merge_override: true`, so the direct push this hook exists to block is refused server-side for every account (#743). This is belt-and-braces over a path the server already refuses. |
|
||||
| `.claude/hooks/pretooluse-nav-guard.sh` | `N/A` | Reads only the proposed tool call's own parameters and decides synchronously; the `curl` mentions in the file are prose, not executed lines. |
|
||||
|
||||
| `.claude/hooks/prepush-rebase-check.sh` — `git fetch origin main`, then `git merge-base --is-ancestor origin/main HEAD` to decide whether to block the push as behind | `UNSAFE-KNOWN` | Fetch-then-decide with no re-validation before the verdict; `origin/main` can advance inside that window. Accepted because the decision is self-correcting and cannot reach `main`: a push allowed on a now-stale read still lands on a feature branch, since direct pushes to `main` are refused server-side (#743), so the worst case is a rebase nag arriving one push later rather than a bad merge. This row exists because the previous token filter did not list `git fetch` and so could not see it at all. |
|
||||
| `.claude/hooks/prepush-clean-worktree-check.sh` — `git fetch origin main`, then diffs `origin/main...HEAD` to scope which dirty files are in the pushed diff | `UNSAFE-KNOWN` | Same fetch-then-decide shape and the same bound: the verdict advises a feature-branch push only, `main` refuses direct pushes server-side (#743), and a stale `origin/main` read at worst lets a dirty-file push through, which the downstream review gate still catches before any merge. |
|
||||
| `.claude/hooks/decisions-guard.sh` | `N/A` | Reads no live remote state — runs `scripts/decisions_validate.py` over the local working tree; no fetch, no HTTP call. |
|
||||
| `.claude/hooks/design-sync-reminder.sh` | `N/A` | Reads no live remote state — compares local `git diff`/`ls-files` output against the session's own edits, never contacts origin. |
|
||||
| `.claude/hooks/posttooluse-worktree-marker.sh` | `N/A` | Reads no live remote state — parses the tool call's own JSON payload and writes a local ownership marker. |
|
||||
| `.claude/hooks/pretooluse-agent-model.sh` | `N/A` | Reads no live remote state — inspects only the proposed Agent call's own `model`/`subagent_type` fields. |
|
||||
| `.claude/hooks/pretooluse-agent-ram.sh` | `N/A` | Reads no live remote state — samples local `memory_pressure -Q` output. |
|
||||
| `.claude/hooks/pretooluse-bash-guard.sh` | `N/A` | Reads no live remote state — pattern-matches the proposed Bash command string for `ETV_UPDATE_GOLDENS=`. |
|
||||
| `.claude/hooks/pretooluse-bom-guard.sh` | `N/A` | Reads no live remote state — inspects local `git diff` output and reads local `.cs` bytes for a BOM. |
|
||||
| `.claude/hooks/pretooluse-worktree-guard.sh` | `N/A` | Reads no live remote state — reads a local `.claude-worktree-owner` marker in the target worktree. |
|
||||
|
||||
### Husky git hooks
|
||||
|
||||
| Site | Class | Note |
|
||||
|---|---|---|
|
||||
| `.husky/pre-push` | `N/A` | Delegates every remote read to `prepush-donewhen.sh`, `prepush-rebase-check.sh` and `prepush-clean-worktree-check.sh`, each of which carries its own row. This file forwards stdin ref lines and runs local `npm run check:api/lint/typecheck/build` against the checked-out tree. |
|
||||
| `.husky/pre-commit` | `N/A` | Reads no live remote state — local lint-staged, `decisions-guard.sh`, a `git diff --cached` scan, and local `dotnet format --verify-no-changes`. |
|
||||
| `.husky/commit-msg` | `N/A` | Reads no live remote state — greps the local commit-message file for a trailer. |
|
||||
|
||||
### Scripts
|
||||
|
||||
| Site | Class | Note |
|
||||
|---|---|---|
|
||||
| `scripts/post-review-verdict.sh` — commit-status write | `PINNED` | Re-reads the PR and compares **both** `.head.sha` and `.base.ref` immediately before the POST, and `die`s (exit 1, no status written) on a mismatch **or on a field it cannot read**. That last clause is new: both comparisons were guarded by `[ -n "$x" ] &&`, so a well-formed 2xx body that merely omitted the field made the check a no-op and the status was posted having confirmed nothing — found by cold review on #778 and regression-tested against the real predecessor, since the redundant `-z` arm alone mutates green. Closes #706 and #632 for this path by read-compare-refuse, not by CAS: Gitea's status API offers no conditional write. Residual: the **comment** is posted *before* the re-read, so a head that moves in between leaves a verdict comment with no status — the comment is not the gate, but the mismatch is confusing and is tracked in **#792**. |
|
||||
| `scripts/pr-changed-files.sh` — paged file enumeration | `UNSAFE-KNOWN` | #707's fix, graded honestly after cold review: `.base.ref`, `.base.sha` and `.head.sha` are captured before paging and re-checked after, and any *observed* movement fails the whole enumeration closed rather than emitting a short list. But before-and-after equality is **ABA-vulnerable** — a `main → scratch → main` retarget during paging can return the same ref and, if nothing merged meanwhile, the same base sha, while the pages in between were diffed against the scratch base. The script's own comment says it narrows rather than erases; this row previously said "any movement fails", which was stronger than the code. Accepted here because the enumerator cannot close it alone, but be exact about what the caller-side fence does and does not cover: `ci.verdict-write-retarget-fence` counts `change_target_branch` events, so it catches the BASE alias and **nothing else**. A HEAD alias is not covered by anything — a force-push `H1 -> H2 -> H1` during pagination leaves the final `.head.sha` comparison equal while the middle pages were enumerated against `H2`, and no counter moves. That residual is real, unfenced, and stated here rather than papered over; closing it needs a monotonic head-mutation fence or enumeration bound to an immutable tree, neither of which exists today — tracked in **#803**, which also carries the three older contracts that still assert more than this row does. |
|
||||
| `scripts/select-queue.sh` — issue list, then per-issue `/dependencies` | `UNSAFE-KNOWN` | The open-issue list (labels, milestone, priority) is snapshotted once; per-candidate dependency reads happen seconds later and never re-read the issue's own labels, so an issue claimed `in-progress` in that gap still appears on the shortlist. Accepted: the script authorizes **no write**. The real gate is the four-way claim check in `process.parallel-session-claim`, which runs after selection and re-reads live state by construction. Tightening this would move a check that must be adversarial into a tool that is advisory. |
|
||||
| `scripts/ci-detect-already-validated.sh` — prior-head combined status | `UNSAFE-KNOWN` | Reads the PR head's status and emits `skip=true`, with nothing re-checking before the consuming job runs. Accepted and narrow: the skip elides only **re-running** test/migrations on a tree already validated; the `build` job still builds and pushes unconditionally, so no image ever ships from unvalidated source. |
|
||||
| `scripts/issue-qualification-audit.sh` — paged issue list, then a report | `UNSAFE-KNOWN` | Pages the open-issue list and reports which issues lack a `priority:` label, so like every paged read here its pages can straddle a change and the report can name a state no single instant held. Graded to match `select-queue.sh` rather than `N/A`: the two run the same shape, and the reason offered for accepting `select-queue.sh` — it authorizes no write — cannot simultaneously be the reason this one is out of the class. Accepted on the same terms: it is advisory, session-end, human-read, and the labels it prompts for are applied by hand afterwards. |
|
||||
| `scripts/security-scan.sh` — `docker pull`, then `docker run` the same tag | `UNSAFE-KNOWN` | Pull and run are two steps over a MUTABLE tag, which is the same shape the registry rows below were graded down for; "resolution and use are one step" overstated it. In practice the second step resolves against the local daemon, which holds the image the pull just placed, so a mid-window retag does not change what runs. Accepted on that, plus the scope: this boots a throwaway container and scans it, authorizing nothing. |
|
||||
| `scripts/migration-smoke.sh` — `docker pull`, then `docker run` the same tag | `UNSAFE-KNOWN` | The same pull-then-run over a mutable tag as `security-scan.sh` above, and graded with it rather than left behind: its row previously said "Same shape" as a note that has since been rewritten to the opposite conclusion, so the backreference had quietly inverted. This one deserves the grade MORE, not less — `security-scan.sh` boots a throwaway container and authorizes nothing, while this is the pre-deploy migration smoke that gates a production stack recreation. Accepted on the same bound (the run resolves against the local daemon holding the image the pull just placed) plus its own stated operator-trust gap: the resolved image id is **reported** for a human rather than compared against a prior read. |
|
||||
| `scripts/hook-fire-log.sh` | `N/A` | Entirely local: reads stdin and writes JSONL under the cache dir; the only `curl` in the file is in a comment. |
|
||||
| `scripts/e2e-local.sh` | `N/A` | No outbound call at all; readiness is a local log grep and a local port probe against a subprocess it started. |
|
||||
| `scripts/e2e-ui.sh` | `N/A` | Launches a local Chromium and runs specs against `http://localhost:$PORT`. |
|
||||
| `scripts/e2e-functional.sh` | `N/A` | Every call targets `$BASE_URL`, defaulting to `http://localhost:8409`. The one non-local-looking address, `192.0.2.1`, is TEST-NET-1 (RFC 5737) — written into the DB as a connection row precisely so it is unroutable, never dialed by the script. |
|
||||
|
||||
| `scripts/ci-detect-docs-only.sh` — `git fetch origin "$base"`, then diffs the fetched tip against HEAD to emit `docs_only`, which gates whether the required test/migrations jobs run their real steps | `UNSAFE-KNOWN` | Read-then-act with no re-check between the fetch and the emitted value, and the decision genuinely gates required CI work. Accepted because the script is deliberately asymmetric: every ambiguous, undeterminable or shallow-checkout case resolves to `docs_only=false` (run everything), and only an exact unanimous all-docs diff yields `true` — so a stale or racing base read can at worst cause an unnecessary full run, never a skipped one (#416). |
|
||||
| `scripts/refresh-shared-checkout.sh` — `git fetch origin main`, then `git merge --ff-only origin/main` and a conditional `npm ci` | `UNSAFE-KNOWN` | Fetch-then-act with no re-check between the fetch and the merge. Accepted because every action is self-refusing or reversible: `--ff-only` fails harmlessly rather than diverging if the ref moved on, the script refuses outright when the tree is not clean `main` or is ahead or mid-rebase, and this is a developer-convenience checkout rather than a release or merge-authorization path — a stale read costs one extra fetch next run, never lost work. |
|
||||
| `scripts/check-review-verdict.sh` | `N/A` | Reads no live remote state itself — classifies a comments JSON payload supplied on stdin; the fetch belongs to the caller's row. |
|
||||
| `scripts/decisions_validate.py` | `N/A` | Reads no live remote state — its `git log`/`show`/`ls-tree`/`merge-base` calls operate on refs the caller already checked out or passed via `--base`/`--head`, never a fetch. |
|
||||
| `scripts/prove-fix.sh` | `N/A` | Reads no live remote state — `git worktree add`/`rev-parse`/`diff-tree` operate on the local repository's own objects. |
|
||||
| `scripts/add-migration.sh` | `N/A` | Reads no live remote state — runs `dotnet ef migrations add` against local project files; implicit NuGet resolution is dependency supply-chain, out of this class per limit 1. |
|
||||
| `scripts/update-openapi.sh` | `N/A` | Reads no live remote state — a local `dotnet build`/`GenerateOpenApiDocuments` then a local python script. |
|
||||
| `scripts/cleanup-code.sh` | `N/A` | `dotnet tool restore` resolves and uses tooling in one step (limit 1, not a check-and-use split); the rest is a local `git status --porcelain` scan. |
|
||||
| `scripts/cleanup-all-code.sh` | `N/A` | Same shape as `cleanup-code.sh` — restore-and-use in one step, no check-then-act over remote state. |
|
||||
| `scripts/build_decisions_catalog.py` | `N/A` | Reads no live remote state — parses local decision records and writes the local catalog. |
|
||||
| `scripts/decisions_lib.py` | `N/A` | Reads no live remote state — pure parser over local decision-record files. |
|
||||
| `scripts/migrate_decisions_split.py` | `N/A` | Reads no live remote state — one-shot local file migration over `docs/decisions/`. |
|
||||
| `scripts/generate-endpoint-index.py` | `N/A` | Reads no live remote state — reads local `v1.json` and writes a local markdown index. |
|
||||
| `scripts/check-kickoff-guard.sh` | `N/A` | Reads no live remote state — scans a fixed local file list for forbidden phrasing. |
|
||||
| `scripts/check-local-lsp.sh` | `N/A` | Reads no live remote state — probes local PATH binaries and spawns a local MCP server over stdio. |
|
||||
| `scripts/mcp_smoke.py` | `N/A` | Reads no live remote state — spawns a local subprocess and speaks JSON-RPC over stdio pipes, no network socket. |
|
||||
| `scripts/jq-preflight.sh` | `N/A` | Reads no live remote state — runs local `jq --version`. |
|
||||
| `scripts/ci-peak-anon.sh` | `N/A` | Reads no live remote state — samples the runner's local cgroup `memory.stat`/`memory.peak`. |
|
||||
| `scripts/ci-prove-ban-detects.sh` | `N/A` | Reads no live remote state — mutates a local workflow copy and runs pytest against the local checkout. |
|
||||
| `scripts/ci-step-ran.sh` | `N/A` | Reads no live remote state — reads runner-supplied env vars and local marker files it wrote itself. |
|
||||
| `scripts/set-provider.sh` | `N/A` | Reads no live remote state — sets local `dotnet user-secrets` values. |
|
||||
| `scripts/__init__.py` | `N/A` | Empty package marker — executes nothing. |
|
||||
| `scripts/scripted-schedules/entrypoint.py` — `ScriptedScheduleApi.get_context(build_id)`, then `define_content` / `reset_playout` / `build_playout` against the same live server | `UNSAFE-KNOWN` | A genuine read-then-act over live ErsatzTV state, and the row cold review found missing when the population was still non-recursive. The context is fetched, handed to user-supplied script functions that mutate the playout, and re-fetched after a reset with nothing pinning either read — a concurrent build or edit between them is invisible. Accepted because it runs inside a single scripted-schedule build the server itself serialises per playout, and because the API exposes no version or ETag on the context to compare against; the honest bound is that the blast radius is one playout's content, reversible by rebuilding. |
|
||||
| `scripts/macOS/bundle.sh` | `N/A` | Reads no live remote state — moves files and creates symlinks in a local app bundle. |
|
||||
| `scripts/macOS/sign.sh` | `N/A` | `codesign --timestamp` contacts Apple's timestamp server, but resolution and use are one step with no earlier check whose answer is later trusted — the same boundary as limit 1. |
|
||||
| `scripts/macOS/sign-dmg.sh` | `N/A` | Same shape as `sign.sh` — a timestamped `codesign` over a local DMG, no check-then-act over remote state. |
|
||||
|
||||
### Workflows
|
||||
|
||||
| Site | Class | Note |
|
||||
|---|---|---|
|
||||
| `.gitea/workflows/review-verdict.yml` — status read → status POST | `UNSAFE-KNOWN` | The residual this whole class reduces to. Gitea's status API has no ETag, no If-Match and no expected-previous-state, so read and write cannot be made one operation. Narrowed twice rather than claimed closed: a monotonic `change_target_branch` **event-count** fence refuses to write if the count moved (`ci.verdict-write-retarget-fence`; the count is used because the branch *name* is ABA-vulnerable), and a high-water-mark re-read repairs a `success` posted over a human verdict back to `pending`. The file states the residual window explicitly rather than asserting safety. |
|
||||
| `.gitea/workflows/review-verdict.yml` — base-ref checkout | `PINNED` | `ref: ${{ github.event.pull_request.base.sha }}` — a full sha from the fixed event payload, so the PR head cannot supply the workflow definition that judges it. |
|
||||
| `.gitea/workflows/review-verdict.yml` — changed-file enumeration | `UNSAFE-KNOWN` | Delegates to `scripts/pr-changed-files.sh` with the head sha and expected base, and therefore **inherits that row's residual, not a pin** — this row said "inherits that row's pins" while the row it points at was being graded down, which is exactly the stale-cross-reference a multi-round edit produces. Accepted on better terms than the enumerator alone — this is the one caller that also runs the monotonic `change_target_branch` event-count fence (`ci.verdict-write-retarget-fence`) — but only for the BASE axis. The HEAD alias described in the enumerator's row (`H1 -> H2 -> H1` during pagination) is unfenced here too, and this row previously implied the fence covered it. |
|
||||
| `.gitea/workflows/docker-build.yml` — CI toolchain image | `UNSAFE-KNOWN` | Graded down by cold review, correctly: this file's own definition of `PINNED` names an image **digest**, and `ersatztv-ci:<short-sha>` is a mutable **tag**. A registry tag can be repointed after `ci-image-pin` verifies it and before a job pulls it, and a 7-hex short sha is additionally collision-prone. Accepted rather than fixed here because the exposure needs write access to our own LAN registry — i.e. an attacker already inside the trust boundary — and two controls bound it: jobs never consume `:latest`, and `pr-checks.yml`'s `ci-image-pin` fails the build if the tag drifts from the last commit touching `docker/ci/`. Consuming `image@sha256:…` is the real fix and is the natural companion to **#772**, which already covers the availability half of this tag's weakness. |
|
||||
| `.gitea/workflows/docker-build.yml` — release smoke pull | `UNSAFE-KNOWN` | Pulls `${IMAGE}:${SMOKE_SHORT_SHA}`, the tag this same job pushed moments earlier. The first draft called that `PINNED` on the strength of the job's `concurrency` group; cold review showed the group is **per-ref**, so a branch build and a tag build of the same commit sit in DIFFERENT groups and can publish the same `:<short-sha>` — the smoke step can therefore pull the other run's image. Accepted rather than fixed here because the fix is the same one-line change as the row above (pull `image@sha256:…`, propagated from the push step) and belongs with it; until then no registry row in this file claims to be pinned. |
|
||||
| `.gitea/workflows/docker-build.yml` — `api-docs` / `format` base fetch | `UNSAFE-KNOWN` | Fetches the live base tip to diff generated artifacts, with nothing pinning it. Graded up from `N/A` by cold review, which was right that "advisory" was too quick: these are not branch-protection-required contexts, but the merge-consent hook reads Gitea's **combined** status, and a combined state that is not `success` blocks the auto-grant — so a wrong answer here does participate in merge consent. Accepted because the failure direction is benign: a base that advanced mid-job makes a generated artifact look stale and FAILS the job, costing a re-run, rather than passing something it should not. |
|
||||
| `.gitea/workflows/ci-image.yml` — verify the pushed image | `UNSAFE-KNOWN` | Pulls back the `:<sha>` tag it pushed in the immediately preceding step. Same demotion and same reason as the two rows above: a registry tag is not a digest, and anything able to write to the registry can repoint it between the push and the verify, which would make the verification confirm an image other than the one built. Accepted on the same terms — the exposure requires registry write access, i.e. an actor already inside the trust boundary — and the same fix applies. |
|
||||
| `.gitea/workflows/pr-checks.yml` — `prove-fix` | `PINNED` | `base`/`head` are full shas from the event payload, immune to later PR mutation. |
|
||||
| `.gitea/workflows/pr-checks.yml` — `ci-image-pin` | `N/A` | A purely local comparison — `git log` over the checked-out tree against a literal in `docker-build.yml`. It never queries the registry, which is exactly why it cannot bound the retagging the image rows above describe. |
|
||||
| `.gitea/workflows/pr-checks.yml` — `docs-reminder` base fetch | `N/A` | Fetches the live base tip, but the result only selects the text of a non-blocking warning. The rationale has to be narrower than the first draft's "the job cannot fail and never reaches the combined status", which is false — any job's status joins the combined state, and runner or checkout failure can redden it. What is true, and is what earns the `N/A`: the fetch and diff are failure-swallowed, so the remote read cannot change this job's outcome, only the warning's wording. It draws no authorization from what it reads, this file's second `N/A` clause. Grouping it with `decisions-guard` over-demoted it, and the two have different classifications, so per the rule above they get separate rows. |
|
||||
| `.gitea/workflows/pr-checks.yml` — `decisions-guard` base fetch | `UNSAFE-KNOWN` | Fetches the live base tip and diffs against it, unpinned, and unlike `docs-reminder` this job CAN fail — so its answer reaches the combined status the merge hook reads and therefore participates in consent. Accepted on the same grounds as the `api-docs`/`format` row: a base that moved mid-job produces a spurious FAILURE and a re-run, never a spurious pass, and it re-fetches fresh on every trigger with no state carried between runs. |
|
||||
| `.gitea/workflows/renovate.yml` — `renovate/renovate:43` | `N/A` | Out of this class, in scope for a different one — see the limits below. |
|
||||
| `.gitea/workflows/dependency-scan.yml` — NuGet advisory query | `N/A` | `dotnet list package --vulnerable` queries a live advisory database and fails the job on the report marker in the same step, so there is no check-then-act split. Worth one line anyway: a green here means "no advisories **as of this run**", which is a dated claim about mutable remote data rather than a property of the tree — which is why the scan runs on a weekly schedule instead of only on PRs. |
|
||||
|
||||
## Limits, stated rather than implied
|
||||
|
||||
1. **Unpinned dependencies are a different class and are not graded here.** `actions/checkout@v4`,
|
||||
`docker/build-push-action@v6`, `mysql:8.4` and `renovate/renovate:43` are floating tags, and
|
||||
`renovate.yml` runs its one with a repo-writing token. But resolution and execution are the same
|
||||
step — there is no earlier check whose answer a later action trusts — so they are a
|
||||
supply-chain-pinning concern, not a check-and-use race. Listing them as `N/A` here records that
|
||||
they were examined and classified, not that they are safe.
|
||||
|
||||
2. **The population is files, not call sites.** The test derives which *files* are in scope; it
|
||||
cannot tell that an existing file grew a second, unpinned read. That residue is the
|
||||
sites-in-code limit named in `testing.guard-derives-population-from-source` — it needs
|
||||
find-all-references tooling, tracked in #777 — and it is why the Note column is prose a reviewer
|
||||
reads rather than a field a script checks.
|
||||
|
||||
3. **Scope is hand-written; the population inside it is derived, with no content filter at all.**
|
||||
Scope is every file under `scripts/` (`*.sh`, `*.py`), `.claude/hooks/`, `.husky/` and
|
||||
`.gitea/workflows/` — a reviewed policy choice. Inside it, *every* file gets a row, and a file
|
||||
that reads no remote state earns an explicit `N/A` rather than silently staying out.
|
||||
|
||||
The first version filtered that scope by an outbound-network token list and argued the filter was
|
||||
a scope choice rather than a population filter. Cold review rejected the distinction, and the
|
||||
evidence settled it: the list omitted `git fetch`, which is this repo's most common remote read,
|
||||
so `prepush-rebase-check.sh` — which fetches `origin/main` and derives a **push decision** from
|
||||
it — was structurally invisible to a guard claiming to cover "every executable that reads live
|
||||
remote state", along with three others. The defence offered was that over-inclusion is the safe
|
||||
direction; the filter also *under*-included, which is the direction that costs a blind spot.
|
||||
Enumerating the directories costs more rows and has none.
|
||||
|
||||
4. **Nothing here checks that a `PINNED` claim is true.** The test asserts every in-scope file has a
|
||||
row and that the classifications come from a closed vocabulary. Whether a row is honest stays a
|
||||
review responsibility, and this table is what review reads — the same split, and the same
|
||||
admitted residue, as `docs/guard-inventory.md`.
|
||||
@@ -142,15 +142,36 @@ printf 'posted comment: Review-verdict: %s @ %s\n' "$verdict" "$short"
|
||||
# introduced by the refactor, so the refusal is now explicit rather than a side effect of `set -e`.
|
||||
prjson_now=$(api_get "repos/$owner/$repo/pulls/$pr") \
|
||||
|| die "could not re-read PR #$pr to confirm the head and base had not moved while posting — no status was written. Re-run once Gitea is reachable."
|
||||
# `[ -n "$x" ] && [ "$x" != "$want" ]` was a fail-OPEN on BOTH of the checks below (ersatztv#778).
|
||||
# A 2xx body that merely LOST the field — `{"head":{},"base":{}}` — yields an empty value, so the
|
||||
# `-n` conjunct is false, the comparison never runs, and the status is posted having confirmed
|
||||
# NOTHING about either the head or the base. The re-read exists precisely to refuse when it cannot
|
||||
# confirm, so a field it cannot read must die exactly like a field that moved. The transport failure
|
||||
# one line up is already fatal; this closes the same hole one level down, which is where it keeps
|
||||
# reappearing in this repo.
|
||||
#
|
||||
# WHICH LINE CARRIES THE SAFETY, stated because it is not the one it looks like: dropping the `-n`
|
||||
# conjunct is the fix. The unconditional `!=` below already rejects an empty value, so the explicit
|
||||
# `-z` arms are REDUNDANT for the safety property and exist only to give the operator an accurate
|
||||
# message ("carried no head sha" rather than "moved to ''"). Disarming a `-z` arm alone therefore
|
||||
# leaves the suite green — the two overlap, and a mutation proof aimed at it would be vacuous. The
|
||||
# proof in `test_a_reread_that_LOSES_a_field_refuses_instead_of_posting` is taken against the real
|
||||
# predecessor (the `-n` conjunct restored), which is what actually goes red.
|
||||
sha_now=$(printf '%s' "$prjson_now" | jq -r '.head.sha // ""')
|
||||
if [ -n "$sha_now" ] && [ "$sha_now" != "$sha" ]; then
|
||||
if [ -z "$sha_now" ]; then
|
||||
die "re-read PR #$pr but its response carried no head sha, so it is UNPROVEN that the head is still $short — no status was written. Re-run once Gitea returns a well-formed PR body."
|
||||
fi
|
||||
if [ "$sha_now" != "$sha" ]; then
|
||||
die "head moved from $short to ${sha_now:0:7} while posting — that commit is UNREVIEWED, so no status was written. Re-review the new head and run this again."
|
||||
fi
|
||||
# The same TOCTOU window applies to the base (ersatztv#632): a retarget between the read above and
|
||||
# the status write below would bind the verdict to a base that is no longer the PR's, and the head
|
||||
# sha check would not notice because retargeting does not move the head.
|
||||
base_now=$(printf '%s' "$prjson_now" | jq -r '.base.ref // ""')
|
||||
if [ -n "$base_now" ] && [ "$base_now" != "$base_ref" ]; then
|
||||
if [ -z "$base_now" ]; then
|
||||
die "re-read PR #$pr but its response carried no base ref, so it is UNPROVEN that the base is still '$base_ref' — no status was written. Re-run once Gitea returns a well-formed PR body."
|
||||
fi
|
||||
if [ "$base_now" != "$base_ref" ]; then
|
||||
die "base branch changed from '$base_ref' to '$base_now' while posting — the diff you reviewed is not the diff this PR now merges, so no status was written. Re-review against the new base and run this again."
|
||||
fi
|
||||
|
||||
|
||||
@@ -0,0 +1,830 @@
|
||||
"""The scheduled-auto-merge path verifies the protection it rests on (ersatztv#778).
|
||||
|
||||
`merge_when_checks_succeed` hands the actual merge to Gitea, to be performed later against whatever
|
||||
head is green at that moment. Everything the hook proves is therefore a SNAPSHOT. What makes that
|
||||
safe is stated in the hook and in #622: `review-verdict/h10` is a REQUIRED status check on the base
|
||||
branch, a commit status belongs to exactly one sha, so a commit pushed after scheduling cannot
|
||||
inherit the verdict and Gitea's own gate refuses the merge.
|
||||
|
||||
That guarantee is branch-protection CONFIG. It lives outside this repo, and before #778 nothing
|
||||
compared the two — the hook asserted it in a comment and in the reason string a human reads, which
|
||||
is a claim about the past, not a check. These tests pin the conversion of that assumption into a
|
||||
precondition.
|
||||
|
||||
The outcome set is the contract, and each arm is asserted separately because collapsing any two of
|
||||
them is how this class of guard has failed here before:
|
||||
|
||||
* required check PRESENT -> proceed (no opinion drawn from this check)
|
||||
* branch protection UNREADABLE -> ask (a transient failure is not evidence of safety)
|
||||
* required check ABSENT -> deny (this is #622's hole reopened, not a degraded read)
|
||||
* a GLOB rule COULD govern the base -> ask, and distinctly from the unreadable case. This hook
|
||||
does not reimplement Gitea's glob dialect, so "some rule might apply and we cannot tell" is a
|
||||
fourth answer, not a flavour of the third. Tests that assert only `"ask" in reason` cannot tell
|
||||
the two apart — and a crashed classifier also produces an ask — so each arm is pinned on the
|
||||
text unique to it.
|
||||
|
||||
Observable contract: the hook exits 0 with EMPTY stdout when it has no opinion, and emits a JSON
|
||||
`permissionDecision` otherwise.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import os
|
||||
import subprocess
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
HOOK = REPO_ROOT / ".claude" / "hooks" / "pretooluse-merge-consent.sh"
|
||||
|
||||
SHA = "a9e3e23abf337980ca4c05854f5b1e210099d08b"
|
||||
SHORT = SHA[:7]
|
||||
|
||||
# The PR is deliberately NOT docs-only: the docs-only exemption short-circuits the whole gate before
|
||||
# the scheduled-merge branch is reached, so a docs PR would pass these tests without ever running
|
||||
# the code under test.
|
||||
CURL_SHIM = r'''#!/usr/bin/env python3
|
||||
import json, os, sys, pathlib, urllib.parse
|
||||
|
||||
state = pathlib.Path(os.environ["STUB_DIR"])
|
||||
args = sys.argv[1:]
|
||||
url = [a for a in args if a.startswith("http")][-1]
|
||||
|
||||
# The branch-protection call uses `-o <file> -w '%{http_code}'` rather than `curl -sf`, precisely so
|
||||
# it can tell a 200 from every other outcome (`curl -sf` collapses every HTTP error into exit 22
|
||||
# with empty output, hiding the difference between an empty list and a failed request). It does
|
||||
# NOT treat 404 as a finding. The shim must therefore behave like real curl for those
|
||||
# flags: body to the -o file, status code to stdout. A shim that ignored them would make the hook
|
||||
# read an empty body and a blank code on EVERY path, and the tests would pass by accident against a
|
||||
# guard that never ran — the "test double's fidelity claim" failure this repo has on record.
|
||||
def respond(body, code="200"):
|
||||
if "-o" in args:
|
||||
pathlib.Path(args[args.index("-o") + 1]).write_text(body)
|
||||
else:
|
||||
sys.stdout.write(body)
|
||||
if "-w" in args:
|
||||
sys.stdout.write(code)
|
||||
sys.exit(0)
|
||||
|
||||
# RECORD BEFORE FILTERING. This recorder used to live inside the `endswith` branch below, which
|
||||
# made the "no ref reaches the URL" assertion unfalsifiable: the only URLs it could record were ones
|
||||
# that already satisfied it, so a by-name request was invisible to the very test written to forbid
|
||||
# it. Cold review reintroduced a by-name lookup in the hook and the suite stayed 33/33 green. That is
|
||||
# the filter-on-the-asserted-property defect this PR's sibling record is about, committed inside the
|
||||
# guard against it — so the recorder now sees EVERY branch-protection URL, whatever its shape.
|
||||
if "/branch_protections" in url:
|
||||
with (state / "bp_urls").open("a") as fh:
|
||||
fh.write(url + "\n")
|
||||
|
||||
if url.rstrip("/").endswith("/branch_protections"):
|
||||
# The hook reads ONLY this endpoint now — the by-name lookup was deleted because it performs no
|
||||
# matching and knows nothing about rule precedence, so a 200 from it proved less than it looked.
|
||||
mode = (state / "bp").read_text().strip()
|
||||
|
||||
if mode == "TRANSPORT-ERROR":
|
||||
respond("", "000")
|
||||
if mode == "FORBIDDEN":
|
||||
respond('{"message":"token does not have at least one of required scope(s)"}', "403")
|
||||
if mode == "GARBAGE":
|
||||
respond('{"message":"not an array"}')
|
||||
if mode == "EMPTY":
|
||||
respond('')
|
||||
if mode == "UNPARSEABLE-RULES":
|
||||
# A 200 whose rule NAME is a number: `//` fires only on null/false, so the classifier's
|
||||
# `explode`/`match` throws and the program dies on a read that plainly succeeded.
|
||||
respond(json.dumps([{"branch_name": 7, "enable_status_check": True,
|
||||
"status_check_contexts": ["review-verdict/h10"]}]))
|
||||
if mode == "LIST-404":
|
||||
# An HTTP 404 from the LIST endpoint: the repo is absent, or invisible to this credential.
|
||||
# Gitea answers 404 for both, and it says NOTHING about whether the base is protected.
|
||||
respond('{"message":"Not Found"}', "404")
|
||||
if mode == "EMPTY-LIST":
|
||||
# The list WAS read and holds no rule — the only shape that establishes absence.
|
||||
respond("[]")
|
||||
if mode == "LIST-UNREADABLE":
|
||||
respond('{"message":"internal error"}', "500")
|
||||
if mode == "GLOB-RULE":
|
||||
respond(json.dumps([{"branch_name": "m*", "enable_status_check": True,
|
||||
"status_check_contexts": ["review-verdict/h10"]}]))
|
||||
if mode == "REGEX-META-RULE":
|
||||
respond(json.dumps([{"branch_name": "mai.", "enable_status_check": True,
|
||||
"status_check_contexts": ["review-verdict/h10"]}]))
|
||||
if mode == "GLOB-WITH-DOT-RULE":
|
||||
respond(json.dumps([{"branch_name": "release/26.*", "enable_status_check": True,
|
||||
"status_check_contexts": ["review-verdict/h10"]}]))
|
||||
if mode == "CHARCLASS-RULE":
|
||||
respond(json.dumps([{"branch_name": "a[b", "enable_status_check": True,
|
||||
"status_check_contexts": ["review-verdict/h10"]},
|
||||
{"branch_name": "main", "enable_status_check": True,
|
||||
"status_check_contexts": ["review-verdict/h10"]}]))
|
||||
if mode == "ESCAPED-META-RULE":
|
||||
respond(json.dumps([{"branch_name": "a\\{b", "enable_status_check": True,
|
||||
"status_check_contexts": ["review-verdict/h10"]}]))
|
||||
if mode == "CASEFOLD-RULE":
|
||||
respond(json.dumps([{"branch_name": "MAIN", "enable_status_check": True,
|
||||
"status_check_contexts": ["review-verdict/h10"]}]))
|
||||
if mode == "TWO-CASE-VARIANT-RULES":
|
||||
respond(json.dumps([{"branch_name": "MAIN", "enable_status_check": True,
|
||||
"status_check_contexts": ["review-verdict/h10"]},
|
||||
{"branch_name": "main", "enable_status_check": True,
|
||||
"status_check_contexts": ["Build ErsatzTV Image / Build & test (.NET)"]}]))
|
||||
if mode == "NONASCII-RULE":
|
||||
respond(json.dumps([{"branch_name": "\u00fcnstable", "enable_status_check": True,
|
||||
"status_check_contexts": ["review-verdict/h10"]}]))
|
||||
if mode == "EXACT-PLUS-GLOB-RULE":
|
||||
respond(json.dumps([{"branch_name": "main", "enable_status_check": True,
|
||||
"status_check_contexts": ["review-verdict/h10"]},
|
||||
{"branch_name": "m*", "enable_status_check": True,
|
||||
"status_check_contexts": ["Build ErsatzTV Image / Build & test (.NET)"]}]))
|
||||
if mode == "MALFORMED-MEMBER":
|
||||
respond(json.dumps([{"branch_name": "main", "enable_status_check": True,
|
||||
"status_check_contexts": 7}]))
|
||||
if mode == "SUBSTRING-STRING":
|
||||
respond(json.dumps([{"branch_name": "main", "enable_status_check": True,
|
||||
"status_check_contexts": "prefix-review-verdict/h10-suffix"}]))
|
||||
if mode == "FALSE-CONTEXTS":
|
||||
respond(json.dumps([{"branch_name": "main", "enable_status_check": True,
|
||||
"status_check_contexts": False}]))
|
||||
if mode == "STRING-ENABLE":
|
||||
respond(json.dumps([{"branch_name": "main", "enable_status_check": "true",
|
||||
"status_check_contexts": ["review-verdict/h10"]}]))
|
||||
if mode == "NON-STRING-MEMBER":
|
||||
respond(json.dumps([{"branch_name": "main", "enable_status_check": True,
|
||||
"status_check_contexts": [7, "review-verdict/h10"]}]))
|
||||
if mode == "EMPTY-CONTEXTS":
|
||||
respond(json.dumps([{"branch_name": "main", "enable_status_check": True,
|
||||
"status_check_contexts": []}]))
|
||||
if mode == "NULL-CONTEXTS":
|
||||
respond(json.dumps([{"branch_name": "main", "enable_status_check": True,
|
||||
"status_check_contexts": None}]))
|
||||
if mode == "LONGER-STRING-MEMBER":
|
||||
respond(json.dumps([{"branch_name": "main", "enable_status_check": True,
|
||||
"status_check_contexts": ["xx-review-verdict/h10-yy"]}]))
|
||||
if mode == "STATUS-CHECK-OFF":
|
||||
respond(json.dumps([{"branch_name": "main", "enable_status_check": False,
|
||||
"status_check_contexts": ["review-verdict/h10"]}]))
|
||||
if mode == "MISSING-CONTEXT":
|
||||
respond(json.dumps([{"branch_name": "main", "enable_status_check": True,
|
||||
"status_check_contexts":
|
||||
["Build ErsatzTV Image / Build & test (.NET) (pull_request)"]}]))
|
||||
# GUARDED
|
||||
respond(json.dumps([{"branch_name": "main", "enable_status_check": True,
|
||||
"status_check_contexts":
|
||||
["Build ErsatzTV Image / Build & test (.NET) (pull_request)",
|
||||
"review-verdict/h10"]}]))
|
||||
|
||||
if "/pulls/" in url and "/files" in url:
|
||||
q = urllib.parse.parse_qs(urllib.parse.urlparse(url).query)
|
||||
page = int(q.get("page", ["1"])[0])
|
||||
if page == 1:
|
||||
print(json.dumps([{"filename": "ErsatzTV/Program.cs", "status": "modified"}]))
|
||||
else:
|
||||
print("[]")
|
||||
sys.exit(0)
|
||||
|
||||
if "/status" in url and (state / "no_recorded_base").exists():
|
||||
# A verdict posted before ersatztv#632 carries no `(base: …)` marker, so the #632 detection
|
||||
# takes its graceful-adoption path and forms no opinion. That isolates the hoisted retarget
|
||||
# check as the ONLY guard that can deny.
|
||||
print(json.dumps({"state": "success", "statuses": [
|
||||
{"context": "review-verdict/h10", "status": "success",
|
||||
"description": "Review-verdict: MERGEABLE @ %s" % os.environ["STUB_SHORT"]}]}))
|
||||
sys.exit(0)
|
||||
|
||||
if "/status" in url:
|
||||
# A 2xx body whose `.statuses` IS an array but whose members are scalars. `.statuses | type ==
|
||||
# "array"` passes; indexing a number then makes jq exit 5 and, under `set -e`, kills the hook
|
||||
# with no JSON at all.
|
||||
_m = (state / "bp").read_text().strip()
|
||||
if _m == "SCALAR-STATUS-ROW":
|
||||
print('{"state":"success","statuses":[1]}')
|
||||
sys.exit(0)
|
||||
if _m == "NONSTRING-STATUS-ROW":
|
||||
# Object shape, string context, valid description — passes the #632 block, which does NOT
|
||||
# validate `.status` — but a NUMERIC status. This is the shape that actually reaches the
|
||||
# scheduled branch's validator, i.e. the new clause's reachable contribution.
|
||||
print(json.dumps({"state": "success", "statuses": [
|
||||
{"context": "review-verdict/h10", "status": 7,
|
||||
"description": "Review-verdict: MERGEABLE @ %s (base: main)"
|
||||
% os.environ["STUB_SHORT"]}]}))
|
||||
sys.exit(0)
|
||||
print(json.dumps({"state": "success", "statuses": [
|
||||
{"context": "review-verdict/h10", "status": "success",
|
||||
"description": "Review-verdict: MERGEABLE @ %s (base: main)" % os.environ["STUB_SHORT"]}]}))
|
||||
sys.exit(0)
|
||||
|
||||
if "/issues/" in url and "/comments" in url:
|
||||
print(json.dumps([{"body": "Review-verdict: MERGEABLE @ %s" % os.environ["STUB_SHORT"]}]))
|
||||
sys.exit(0)
|
||||
|
||||
if "/issues/" in url:
|
||||
print(json.dumps({"body": "## Done-when\n- [x] everything\n"}))
|
||||
sys.exit(0)
|
||||
|
||||
if "/pulls/" in url:
|
||||
# The base is served per-GET so a PERSISTENT retarget mid-run can be modelled: the first read
|
||||
# (top of the hook) sees `main`, a later one sees whatever `retarget` names.
|
||||
counter = state / "pr_get_count"
|
||||
n = int(counter.read_text()) if counter.exists() else 0
|
||||
counter.write_text(str(n + 1))
|
||||
base = "main"
|
||||
bo = state / "base_override"
|
||||
if bo.exists():
|
||||
base = bo.read_text().strip()
|
||||
rt = state / "retarget"
|
||||
if rt.exists() and n >= 1:
|
||||
base = rt.read_text().strip()
|
||||
print(json.dumps({"head": {"sha": os.environ["STUB_SHA"]},
|
||||
"base": {"ref": base, "sha": "b" * 40},
|
||||
"body": "fixes #1"}))
|
||||
sys.exit(0)
|
||||
|
||||
print("{}")
|
||||
'''
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def hook(tmp_path):
|
||||
bindir = tmp_path / "bin"; bindir.mkdir()
|
||||
curl = bindir / "curl"; curl.write_text(CURL_SHIM); curl.chmod(0o755)
|
||||
state = tmp_path / "state"; state.mkdir()
|
||||
(state / "bp").write_text("GUARDED")
|
||||
|
||||
env = dict(os.environ)
|
||||
env["PATH"] = f"{bindir}{os.pathsep}{env['PATH']}"
|
||||
env["STUB_DIR"] = str(state)
|
||||
env["STUB_SHA"] = SHA
|
||||
env["STUB_SHORT"] = SHORT
|
||||
env["ETV_GITEA_TOKEN"] = "stub"
|
||||
env["ETV_GITEA_URL"] = "http://gitea.example"
|
||||
env["CLAUDE_PROJECT_DIR"] = str(REPO_ROOT)
|
||||
env.pop("ETV_GITEA_BASICAUTH", None)
|
||||
|
||||
class Handle:
|
||||
def set_branch_protection(self, mode):
|
||||
(state / "bp").write_text(mode)
|
||||
|
||||
def set_base(self, ref):
|
||||
"""The base reported by EVERY PR read — a stable target, not a retarget."""
|
||||
(state / "base_override").write_text(ref)
|
||||
|
||||
def drop_recorded_base(self):
|
||||
"""Serve a pre-#632 verdict (no `(base: …)`), so only the hoisted check can deny."""
|
||||
(state / "no_recorded_base").write_text("1")
|
||||
|
||||
def set_retarget(self, ref):
|
||||
"""Persistent retarget: every PR read after the first reports `ref`."""
|
||||
(state / "retarget").write_text(ref)
|
||||
|
||||
def branch_protection_urls(self):
|
||||
f = state / "bp_urls"
|
||||
return f.read_text().splitlines() if f.exists() else []
|
||||
|
||||
def decision(self, scheduled=True):
|
||||
payload = {"tool_input": {"method": "merge", "owner": "timothy",
|
||||
"repo": "ersatztv", "pull_number": 42,
|
||||
"merge_when_checks_succeed": scheduled}}
|
||||
r = subprocess.run(["bash", str(HOOK)], input=json.dumps(payload),
|
||||
env=env, capture_output=True, text=True)
|
||||
assert r.returncode == 0, r.stderr
|
||||
if not r.stdout.strip():
|
||||
return None
|
||||
return json.loads(r.stdout)
|
||||
|
||||
def reason(self, scheduled=True):
|
||||
d = self.decision(scheduled=scheduled)
|
||||
return "" if d is None else json.dumps(d)
|
||||
|
||||
return Handle()
|
||||
|
||||
|
||||
def test_a_base_without_the_required_check_denies_a_SCHEDULED_merge(hook):
|
||||
"""The defect #778 closes: arming an auto-merge while the per-sha gate that makes it safe is
|
||||
absent. Nothing else in the flow notices, which is what makes it worth a guard."""
|
||||
hook.set_branch_protection("MISSING-CONTEXT")
|
||||
reason = hook.reason()
|
||||
assert "deny" in reason, (
|
||||
"a scheduled auto-merge was armed with no 'review-verdict/h10' required check on the base — "
|
||||
"that is ersatztv#622's hole reopened")
|
||||
assert "review-verdict/h10" in reason, (
|
||||
"the deny must name the missing context; a reader cannot act on 'branch protection is wrong'")
|
||||
|
||||
|
||||
def test_status_checks_disabled_wholesale_also_denies(hook):
|
||||
"""The context can be listed while `enable_status_check` is false, in which case Gitea enforces
|
||||
none of them. Reading only the list would report the protection as present — the same
|
||||
check-the-label-not-the-capability shape (#697/#698) this repo has paid for twice."""
|
||||
hook.set_branch_protection("STATUS-CHECK-OFF")
|
||||
reason = hook.reason()
|
||||
assert "deny" in reason, (
|
||||
"status checks were disabled entirely and the listed context was read as protection anyway")
|
||||
|
||||
|
||||
def test_positive_control_a_guarded_base_REACHES_the_check_and_still_auto_grants(hook):
|
||||
"""Without this, every test above passes if the hook denies on all paths — which it very nearly
|
||||
does, since this PR is non-docs and several later conditions are stubbed only loosely.
|
||||
|
||||
Asserting the absence of one phrase was not enough (cold review): an unrelated early `ask`, or a
|
||||
differently-worded deny, would satisfy it while proving nothing. So this pins all three of the
|
||||
things that must be true — the branch-protection endpoint was actually CALLED, the decision is
|
||||
`allow`, and the reason is the satisfied-gate message rather than any refusal.
|
||||
"""
|
||||
decision = hook.decision()
|
||||
assert hook.branch_protection_urls(), (
|
||||
"the guarded case never reached the branch-protection endpoint, so the other tests are not "
|
||||
"exercising the code they claim to")
|
||||
assert decision is not None, "the hook passed through instead of auto-granting"
|
||||
verdict = decision["hookSpecificOutput"]["permissionDecision"]
|
||||
assert verdict == "allow", f"a fully-satisfied gate did not auto-grant (got {verdict!r})"
|
||||
assert "satisfied" in decision["hookSpecificOutput"]["permissionDecisionReason"]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("shape", ["SUBSTRING-STRING", "LONGER-STRING-MEMBER"])
|
||||
def test_a_context_name_that_merely_CONTAINS_the_required_one_does_not_satisfy_it(hook, shape):
|
||||
"""The false-OPEN this guard must not have.
|
||||
|
||||
jq's `index()` on a STRING is substring search, so `"prefix-review-verdict/h10-suffix"` answers
|
||||
yes to a naive membership test — auto-granting a scheduled merge on a base where the context is
|
||||
not required at all. `LONGER-STRING-MEMBER` covers the same confusion inside a real array.
|
||||
A false-closed here costs one prompt; a false-open costs an unreviewed merge, so the membership
|
||||
test is exact equality over a value first proven to be an array of strings.
|
||||
"""
|
||||
hook.set_branch_protection(shape)
|
||||
reason = hook.reason()
|
||||
assert "allow" not in reason or "deny" in reason or "ask" in reason, (
|
||||
f"payload shape {shape} auto-granted a scheduled merge")
|
||||
assert "satisfied" not in reason, (
|
||||
f"a context name that merely contains 'review-verdict/h10' ({shape}) was accepted as it")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("shape", ["EMPTY-CONTEXTS", "NULL-CONTEXTS"])
|
||||
def test_an_empty_or_null_contexts_list_DENIES_rather_than_asking(hook, shape):
|
||||
"""Absent is the finding, not a read failure. An empty or null list is a well-formed answer
|
||||
meaning "nothing is required here", so it must take the deny arm and not be swept into the
|
||||
unknown-shape ask alongside genuinely unreadable payloads."""
|
||||
hook.set_branch_protection(shape)
|
||||
reason = hook.reason()
|
||||
assert "deny" in reason, (
|
||||
f"{shape} was treated as unreadable rather than as a confirmed absent required check")
|
||||
|
||||
|
||||
def test_a_FALSE_contexts_value_asks_rather_than_denying(hook):
|
||||
"""jq's `//` alternative fires on `false`, not only on null, so `// []` mapped this malformed
|
||||
payload to an empty list and answered "no" — a confident deny derived from a shape that was
|
||||
never understood. Absent and null are defaulted explicitly; everything else is unknown."""
|
||||
hook.set_branch_protection("FALSE-CONTEXTS")
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason, "a false contexts value was defaulted to [] and produced a deny"
|
||||
|
||||
|
||||
def test_a_non_string_MEMBER_inside_the_array_asks(hook):
|
||||
"""`[7, "review-verdict/h10"]` contains the context, but the payload is not the shape this
|
||||
guard knows how to reason about. Answering "yes" would mean trusting a structure we cannot
|
||||
validate; the honest answer is that we could not tell."""
|
||||
hook.set_branch_protection("NON-STRING-MEMBER")
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason, "an array with a non-string member produced a decision anyway"
|
||||
|
||||
|
||||
def test_a_malformed_contexts_MEMBER_asks_rather_than_denying_with_the_wrong_reason(hook):
|
||||
"""One level below the response-shape check, and it survives it.
|
||||
|
||||
`{"status_check_contexts": 7}` is a perfectly good object, so the top-level type guard passes;
|
||||
jq then errors on the member, `|| true` turns that into an empty string, and a two-way test
|
||||
would report "NOT a required status check" — a confident, specific, wrong diagnosis of a payload
|
||||
that was never read. The same swallow one level down as the #632 base-change guard's second fix.
|
||||
"""
|
||||
hook.set_branch_protection("MALFORMED-MEMBER")
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason, "a malformed contexts member produced a decision instead of a question"
|
||||
assert "NOT a required status check" not in reason, (
|
||||
"an unreadable payload was reported as a confirmed missing required check")
|
||||
|
||||
|
||||
def test_a_base_with_NO_branch_protection_at_all_denies_rather_than_asking(hook):
|
||||
"""The strongest form of the thing being checked, and the likeliest real trigger.
|
||||
|
||||
A rule list that is READABLE and EMPTY has nothing that can govern any base, so
|
||||
`review-verdict/h10` is definitively not required and scheduling an auto-merge is #622's hole.
|
||||
That must DENY, not ask: routing the most likely real-world trigger — branch protection removed
|
||||
— to a human prompt would make it read like a transient hiccup.
|
||||
|
||||
Absence is established by the LIST, never by a status code; this fixture returns 200 with `[]`.
|
||||
An HTTP 404 means the repo was absent or invisible to the credential and is a read failure,
|
||||
covered by `test_an_HTTP_404_on_the_LIST_read_asks_and_does_not_claim_the_list_was_read`.
|
||||
"""
|
||||
hook.set_branch_protection("EMPTY-LIST")
|
||||
reason = hook.reason()
|
||||
assert "deny" in reason, (
|
||||
"a base with no branch protection at all did not deny a scheduled auto-merge")
|
||||
assert "none matches" in reason, (
|
||||
"the deny must distinguish 'the list was read and nothing governs this base' from "
|
||||
"'could not read'")
|
||||
|
||||
|
||||
def test_a_403_asks_because_it_says_only_that_we_could_not_look(hook):
|
||||
"""A credential without the repo-admin scope this endpoint needs proves nothing about the
|
||||
protection, so it must NOT deny — otherwise the guard strands every scheduled merge run made
|
||||
with a narrower token."""
|
||||
hook.set_branch_protection("FORBIDDEN")
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason, "a 403 was treated as evidence about the protection"
|
||||
assert "none matches" not in reason, (
|
||||
"a 403 was reported as a confirmed absence of branch protection")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("failure", ["TRANSPORT-ERROR", "GARBAGE", "EMPTY", "LIST-404"])
|
||||
def test_an_UNREADABLE_branch_protection_asks_rather_than_denying_or_passing(hook, failure):
|
||||
""""Could not check" is a third outcome, not a synonym for either neighbour.
|
||||
|
||||
Denying would strand every scheduled merge on a Gitea hiccup or on credentials without the
|
||||
repo-admin scope this endpoint needs. Passing would be worse: it would restore the exact
|
||||
unverified assumption #778 exists to remove, while now printing a reason string claiming the
|
||||
protection was confirmed.
|
||||
"""
|
||||
hook.set_branch_protection(failure)
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason, (
|
||||
f"an unreadable branch-protection response ({failure}) did not fall through to a human")
|
||||
assert "branch-protection rules" in reason, (
|
||||
"the ask must name what could not be checked")
|
||||
|
||||
|
||||
def test_an_IMMEDIATE_merge_is_not_subjected_to_this_check(hook):
|
||||
"""Scope, deliberately narrow — and stated without the overclaim cold review removed.
|
||||
|
||||
An immediate merge is not window-FREE: the hook returns `allow` and a separate call performs the
|
||||
merge, so a push can still land in between. What it lacks is a SCHEDULER — nothing waits on
|
||||
pending checks, so the gap is one tool call rather than however long CI takes. The required
|
||||
branch-protection check is what protects the scheduled path specifically, so extending this deny
|
||||
to immediate merges would block a materially safer operation and invite the whole guard being
|
||||
switched off. The residual on this path is carried in docs/remote-state-inventory.md.
|
||||
"""
|
||||
hook.set_branch_protection("MISSING-CONTEXT")
|
||||
reason = hook.reason(scheduled=False)
|
||||
assert "NOT a required status check" not in reason, (
|
||||
"the required-check deny fired on an immediate merge, which has no post-scheduling window")
|
||||
|
||||
|
||||
def test_a_NON_BOOLEAN_enable_status_check_asks(hook):
|
||||
"""`"true"` is not `true`. Comparing the string to `true` yields a confident "no" -> deny from a
|
||||
payload never understood, which collapses the documented tri-state into two states. Every
|
||||
malformed shape on this endpoint has to reach the same ask arm."""
|
||||
hook.set_branch_protection("STRING-ENABLE")
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason, "a string enable_status_check produced a decision instead of a question"
|
||||
assert "NOT a required status check" not in reason
|
||||
|
||||
|
||||
def test_a_SCALAR_status_row_asks_instead_of_killing_the_hook(hook):
|
||||
"""The consent hook's contract is that it always emits exactly one of grant/deny/ask, and
|
||||
`{"statuses":[1]}` is the payload that can break it: it passes an `.statuses | type == "array"`
|
||||
check, after which indexing a number errors and exits 5, which under `set -e` aborts the hook
|
||||
with NO JSON at all. A gate that emits nothing has not failed closed; it has failed to decide.
|
||||
|
||||
WHAT THIS TEST DOES *NOT* PROVE, stated because the mutation showed it. Restoring the
|
||||
scheduled-branch validation to its predecessor leaves this test GREEN, because the #632
|
||||
base-retarget block runs FIRST and validates the members it consumes — so it catches THIS
|
||||
payload and asks before the scheduled branch is reached. The two guards overlap, which is the
|
||||
masking shape `duplicate guards mask each other` describes.
|
||||
|
||||
The caveat is scoped to this payload, NOT to the clause. The earlier block validates `.context`
|
||||
and `.description` but not `.status`, so an object row with a numeric status passes it and does
|
||||
reach the new validator — `test_a_NON_STRING_status_reaches_the_scheduled_validator` covers that
|
||||
and goes red when the clause is removed. So the clause is masked for scalar rows and load-bearing
|
||||
for that one. An earlier draft called the whole clause defence-in-depth, understating it in the
|
||||
opposite direction from this repo's usual error.
|
||||
|
||||
So this asserts the OBSERVABLE contract — a decision is always emitted for this payload — which
|
||||
is true and worth pinning whichever guard supplies it. It is deliberately not offered as a
|
||||
mutation proof of the newer clause, because it is not one.
|
||||
"""
|
||||
hook.set_branch_protection("SCALAR-STATUS-ROW")
|
||||
decision = hook.decision()
|
||||
assert decision is not None, (
|
||||
"the hook emitted no decision at all for a malformed statuses payload — it neither granted, "
|
||||
"denied nor asked")
|
||||
assert decision["hookSpecificOutput"]["permissionDecision"] == "ask"
|
||||
|
||||
|
||||
def test_a_PERSISTENT_retarget_denies_on_the_IMMEDIATE_path_too(hook):
|
||||
"""The twin. The re-read first landed inside the scheduled branch only, so this exact case —
|
||||
same fixture, `merge_when_checks_succeed` absent — AUTO-GRANTED while its sibling denied.
|
||||
|
||||
Cold review demonstrated it side by side, and it is the shape this repo has on record as
|
||||
"fix one path, then check its TWIN": the fix was applied where the defect was noticed, and the
|
||||
other consumer of the same stale value kept it. The re-read is now hoisted above every
|
||||
base-dependent decision rather than duplicated into the branch that happened to be under review.
|
||||
"""
|
||||
hook.drop_recorded_base()
|
||||
hook.set_retarget("scratch")
|
||||
reason = hook.reason(scheduled=False)
|
||||
assert "deny" in reason, (
|
||||
"an immediate merge was auto-granted after the PR was retargeted mid-evaluation")
|
||||
assert "scratch" in reason and "main" in reason
|
||||
|
||||
|
||||
def test_a_PERSISTENT_retarget_after_the_first_read_denies(hook):
|
||||
"""The defect this guard had itself, found in the fifth cold-review round.
|
||||
|
||||
`$base_ref` is captured from the PR snapshot at the top of the hook, and everything between
|
||||
then and the branch-protection lookup is round trips — the file enumeration alone can be forty
|
||||
pages. A retarget in that gap needs no ABA and no force-push: the lookup would name the OLD
|
||||
base, confirm `review-verdict/h10` on a branch the PR no longer targets, and grant a scheduled
|
||||
merge onto one that may require nothing. Checking a stale identifier is not checking, which is
|
||||
the whole of `process.check-and-use-pins-a-version` — so the guard enforcing that rule had to
|
||||
stop breaking it.
|
||||
"""
|
||||
hook.drop_recorded_base()
|
||||
hook.set_retarget("scratch")
|
||||
reason = hook.reason()
|
||||
assert "deny" in reason, (
|
||||
"the PR was retargeted mid-evaluation and the gate still granted on the original base")
|
||||
assert "scratch" in reason and "main" in reason, (
|
||||
"the deny must name both branches; a reader cannot act on 'the base changed'")
|
||||
|
||||
|
||||
def test_a_NON_STRING_status_reaches_the_scheduled_validator(hook):
|
||||
"""The reachable contribution of the scheduled-branch member validation, which the previous
|
||||
caveat understated.
|
||||
|
||||
The #632 block validates `.context` and `.description` but NOT `.status`, so an object row with
|
||||
a numeric status passes it and arrives here. Without this clause it becomes `vstate=7` and falls
|
||||
to the catch-all deny arm — fail-closed, but reported as "the verdict is '7'" rather than as a
|
||||
payload that could not be read. So the clause is masked for scalar rows and load-bearing for
|
||||
this one; the caveat on the scalar test is scoped accordingly.
|
||||
"""
|
||||
hook.set_branch_protection("NONSTRING-STATUS-ROW")
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason, "a non-string .status produced a verdict-shaped decision"
|
||||
|
||||
|
||||
def test_a_base_that_a_GLOB_rule_could_govern_ASKS(hook):
|
||||
"""A base covered only by a glob rule has no rule bearing its own name, and the deleted by-name
|
||||
endpoint answered 404 for exactly that — read as "unprotected", producing a hard DENY with a
|
||||
specific, false cause.
|
||||
|
||||
But the opposite error is worse: deciding the glob DOES match would auto-grant on a base whose
|
||||
protection was never established. This hook does not reimplement Gitea's glob dialect (its `*`
|
||||
does not cross `/`, and `?`/`[…]`/`{a,b}` are wildcards), so a glob rule that COULD govern the
|
||||
base is undecidable and asks — the only answer honest in both directions.
|
||||
"""
|
||||
hook.set_branch_protection("GLOB-RULE")
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason, (
|
||||
"a base a glob rule could govern was decided rather than referred to a human")
|
||||
assert "could govern it" in reason, (
|
||||
"the ask came from the generic could-not-read arm, not the undecidable-glob arm — those "
|
||||
"are different outcomes and a crashed classifier must not pass as a correct classification")
|
||||
assert "none matches" not in reason, (
|
||||
"a base a glob rule could govern was reported as having no protection at all")
|
||||
|
||||
|
||||
def test_an_unreadable_rule_LIST_asks_rather_than_denying(hook):
|
||||
"""A read failure must not convert 'could not confirm' into 'confirmed absent'. Absence is
|
||||
established only by the classifier returning `nomatch` over a list that WAS read."""
|
||||
hook.set_branch_protection("LIST-UNREADABLE")
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason, "an unreadable rule list was treated as proof of absence"
|
||||
assert "none matches" not in reason
|
||||
|
||||
|
||||
def test_the_protection_lookup_puts_NO_ref_in_the_url(hook):
|
||||
"""The successor to a URL-encoding test, and the reason it could be retired.
|
||||
|
||||
The ref used to be interpolated into `branch_protections/{name}`, where a base like
|
||||
`release/26.4` injected a path separator and 404'd — read as "unprotected". That endpoint is
|
||||
gone: it performed no matching and knew nothing about rule precedence, so a 200 from it proved
|
||||
less than it looked. Only the LIST endpoint is read now, which takes no ref at all, so the whole
|
||||
encoding hazard is removed by construction rather than escaped.
|
||||
"""
|
||||
hook.set_base("release/26.4")
|
||||
hook.drop_recorded_base()
|
||||
hook.reason()
|
||||
urls = hook.branch_protection_urls()
|
||||
assert urls, "the branch-protection endpoint was never requested"
|
||||
for u in urls:
|
||||
assert u.rstrip("/").endswith("/branch_protections"), (
|
||||
f"a ref reached the branch-protection URL: {u}")
|
||||
|
||||
|
||||
def test_a_rule_name_with_REGEX_METACHARACTERS_does_not_match_a_different_base(hook):
|
||||
"""The false-open in the glob fallback: `*` must be the only wildcard.
|
||||
|
||||
Substituting `*` into a raw regex left every other metacharacter live, so a rule named `mai.`
|
||||
matched the base `main` (and `a+b` matched `aab`). A spurious match to some OTHER rule that
|
||||
happens to require `review-verdict/h10` reports this base as protected when nothing governs it —
|
||||
a consent gate answering yes on evidence about a different branch. Verified directly before the
|
||||
fix: `main.x` matched `mainax`.
|
||||
|
||||
Here the only rule is `mai.`, which governs a branch that is not `main`, so nothing protects the
|
||||
base and the gate must deny rather than grant.
|
||||
"""
|
||||
hook.set_branch_protection("REGEX-META-RULE")
|
||||
reason = hook.reason()
|
||||
assert "deny" in reason, (
|
||||
"a rule named 'mai.' was regex-matched against base 'main' and read as protection")
|
||||
assert "none matches" in reason, (
|
||||
"'mai.' contains no GLOB metacharacter, so it is decidable: it simply does not govern "
|
||||
"'main', and the base is genuinely unprotected")
|
||||
|
||||
|
||||
def test_a_GLOB_rule_whose_literal_part_has_a_metacharacter_still_MATCHES(hook):
|
||||
"""The positive control the first escaping attempt lacked, and the reason it looked green.
|
||||
|
||||
Escaping is only half the property: `*` must still span. The first version emitted TWO
|
||||
backslashes (`\\.` = "a literal backslash, then any character"), which made every rule
|
||||
containing a metacharacter UNMATCHABLE — so the fallback found nothing and hard-denied with the
|
||||
stated cause that no rule can govern the base — a false-open converted into a false DENY.
|
||||
|
||||
A negative-only assertion cannot see that: a rule matched literally and a rule made unmatchable
|
||||
both fail to match the wrong base. Only a rule that SHOULD match distinguishes them. Here the rule
|
||||
`release/26.*` could govern the base `release/26.4`, so the gate must reach a decision about it
|
||||
rather than reporting the base as unprotected.
|
||||
"""
|
||||
hook.set_base("release/26.4")
|
||||
hook.drop_recorded_base() # keep the #632 comparison out of this test's way
|
||||
hook.set_branch_protection("GLOB-WITH-DOT-RULE")
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason, (
|
||||
"the glob rule 'release/26.*' could govern base 'release/26.4', which is undecidable here "
|
||||
"and must ask")
|
||||
assert "could govern it" in reason, (
|
||||
"the ask must come from the undecidable-glob arm, not from the classifier failing")
|
||||
assert "none matches" not in reason, (
|
||||
"a base a glob rule could govern was reported as entirely unprotected — the over-escaping "
|
||||
"failure this test exists to catch")
|
||||
|
||||
|
||||
def test_a_rule_name_containing_a_CHAR_CLASS_bracket_does_not_crash_the_matcher(hook):
|
||||
"""`a[b` built the pattern `a\\[b`, which is a premature end of char-class: jq exits 5, the
|
||||
`|| true` swallows it, and the whole list is discarded — so an unrelated rule's NAME could
|
||||
poison the lookup and deny a base that a later rule in the same list protects."""
|
||||
hook.set_branch_protection("CHARCLASS-RULE")
|
||||
reason = hook.reason()
|
||||
assert reason, "the hook emitted no decision at all"
|
||||
assert "ask" not in reason, (
|
||||
"the exact-name rule was decidable and must have been honoured; an ask here means the "
|
||||
"classifier failed rather than classified")
|
||||
assert "none matches" not in reason, (
|
||||
"one rule with a bracket in its name discarded the whole list, including the exact-name "
|
||||
"rule that actually protects this base")
|
||||
assert "deny" not in reason, (
|
||||
"an exact-name rule requiring review-verdict/h10 was present and was not honoured")
|
||||
|
||||
|
||||
def test_a_plain_rule_name_is_matched_CASE_INSENSITIVELY(hook):
|
||||
"""Gitea compares a rule name with no glob metacharacter using `EqualFold`, so a rule named
|
||||
`MAIN` governs the base `main`. Comparing case-sensitively here would find no rule, conclude the
|
||||
base is unprotected, and deny with a false stated cause."""
|
||||
hook.set_branch_protection("CASEFOLD-RULE")
|
||||
reason = hook.reason()
|
||||
assert "ask" not in reason, (
|
||||
"the case-folded exact rule was decidable and must have been honoured; an ask means the "
|
||||
"classifier failed rather than classified")
|
||||
assert "none matches" not in reason, (
|
||||
"a rule named 'MAIN' governs base 'main' in Gitea but was missed by a case-sensitive compare")
|
||||
assert "deny" not in reason
|
||||
|
||||
|
||||
def test_a_BACKSLASH_ESCAPED_metacharacter_in_a_rule_name_is_undecidable_not_absent(hook):
|
||||
"""The one case that breaks the superset proof the `none` arm rests on.
|
||||
|
||||
`none` authorises a DENY on the stated grounds that nothing can possibly govern this base, so
|
||||
its premise must hold unconditionally. gobwas/glob reads `\\{` as a LITERAL brace, so the rule
|
||||
`a\\{b` governs the base `a{b`; a superset that treated `\\` as an ordinary character would build
|
||||
`a\\.*b`, fail to match, and deny a base that is in fact protected. Treating backslash as a
|
||||
metacharacter restores the property.
|
||||
|
||||
Git ref rules make this nearly unreachable — a branch name may not contain `*`, `?`, `[` or `\\`
|
||||
— but `{` IS legal in a branch name, and "nearly unreachable" is not the standard for the arm
|
||||
that issues a deny.
|
||||
"""
|
||||
hook.set_base("a{b")
|
||||
hook.drop_recorded_base()
|
||||
hook.set_branch_protection("ESCAPED-META-RULE")
|
||||
reason = hook.reason()
|
||||
assert "none matches" not in reason, (
|
||||
"a rule whose escaped brace governs this base was reported as unable to govern it")
|
||||
assert "ask" in reason, (
|
||||
"an escaped-metacharacter rule is undecidable here and must ask")
|
||||
assert "could govern it" in reason, (
|
||||
"the ask must come from the undecidable-glob arm, not from the classifier failing — this "
|
||||
"test hits the same arm as its two siblings and needs the same pin")
|
||||
|
||||
|
||||
def test_the_precedence_check_runs_even_when_an_exactly_named_rule_EXISTS(hook):
|
||||
"""The twin the restructure deletes, pinned so it cannot come back.
|
||||
|
||||
The hook used to look the rule up by NAME first and only enumerate the list on a 404. That
|
||||
by-name endpoint is an exact DB lookup that performs no matching and knows nothing about
|
||||
precedence, so on a 200 — the path this repo actually takes, since its rule IS named `main` —
|
||||
the gate granted having consulted one rule and never asked which rule Gitea would apply. The
|
||||
precedence argument guarded the 404 path only: hardened code that was dead, next to live code
|
||||
that was not.
|
||||
|
||||
`EXACT-PLUS-GLOB-RULE` is exactly that configuration: a rule NAMED `main` that requires
|
||||
`review-verdict/h10`, plus `m*` that does not. Under the old flow the by-name hit returned the
|
||||
`main` rule, saw h10 and granted. Now there is one path, so the classifier sees both rules and
|
||||
refuses to guess which one Gitea applies.
|
||||
"""
|
||||
hook.set_branch_protection("EXACT-PLUS-GLOB-RULE")
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason, (
|
||||
"an exactly-named rule was trusted without asking which rule Gitea would actually apply")
|
||||
assert "could govern it" in reason
|
||||
# And the by-name endpoint must not be consulted at all — its existence is what split the paths.
|
||||
for u in hook.branch_protection_urls():
|
||||
assert u.rstrip("/").endswith("/branch_protections"), (
|
||||
f"the by-name lookup is back, and with it the unguarded path: {u}")
|
||||
|
||||
|
||||
def test_a_GLOB_rule_that_could_outrank_an_exact_one_wins_and_ASKS(hook):
|
||||
"""The arm ORDER, pinned. Without this the reorder is invisible to the suite — swapping the arms
|
||||
back left all 29 tests green, which is how an unproven change ships.
|
||||
|
||||
Gitea picks the governing rule with `GetFirstMatched` over a list sorted by Priority and THEN by
|
||||
plain-name-ness, so a glob rule can outrank an exactly-named one. Here `main` requires
|
||||
`review-verdict/h10` and `m*` does not. Evaluating `exact` first inspects the rule that requires
|
||||
h10, concludes the base is protected, and AUTO-GRANTS a scheduled merge onto a base where the
|
||||
check may not be enforced at all — #622's hole, reached through the block written to close it.
|
||||
|
||||
Evaluating `undecidable` first is sound without knowing Gitea's precedence rules, which is the
|
||||
only claim this code is entitled to make about somebody else's resolver.
|
||||
"""
|
||||
hook.set_branch_protection("EXACT-PLUS-GLOB-RULE")
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason, (
|
||||
"an exact rule was trusted while a glob rule could outrank it — the gate granted on a base "
|
||||
"whose enforced rule it never identified")
|
||||
assert "could govern it" in reason, (
|
||||
"the ask must come from the undecidable-glob arm, not from the classifier failing")
|
||||
|
||||
|
||||
def test_two_rules_differing_only_in_CASE_are_undecidable(hook):
|
||||
"""`first` picks list order; Gitea picks by Priority. With `MAIN` requiring `review-verdict/h10`
|
||||
and `main` not, inspecting whichever the API happened to list first would auto-grant on a base
|
||||
whose enforced rule was never identified — the same defect as the arm order, one level down."""
|
||||
hook.set_branch_protection("TWO-CASE-VARIANT-RULES")
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason, (
|
||||
"two fold-equal rules disagree about review-verdict/h10 and one was picked by list order")
|
||||
assert "could govern it" in reason
|
||||
|
||||
|
||||
def test_a_NON_ASCII_rule_or_base_is_undecidable_rather_than_fold_compared(hook):
|
||||
"""`ascii_downcase` is not Gitea's Unicode-aware `EqualFold`, so a rule `ünstable` and a base
|
||||
`Ünstable` fold equal there and not here. The miss lands on `none`, which DENIES with the stated
|
||||
cause "none matches" — and the backslash arm already rejects "nearly unreachable"
|
||||
as a standard for the arm that issues a deny, so the same standard applies here."""
|
||||
hook.set_base("\u00dcnstable")
|
||||
hook.drop_recorded_base()
|
||||
hook.set_branch_protection("NONASCII-RULE")
|
||||
reason = hook.reason()
|
||||
assert "none matches" not in reason, (
|
||||
"a rule that folds equal to this base under EqualFold was reported as unable to govern it")
|
||||
assert "ask" in reason
|
||||
|
||||
|
||||
def test_an_HTTP_404_on_the_LIST_read_asks_and_does_not_claim_the_list_was_read(hook):
|
||||
"""The `nomatch` sentinel, pinned — it shipped UNPINNED, and a full revert left the suite green.
|
||||
|
||||
Absence must be established by the CLASSIFIER over a list that was actually read, never by an
|
||||
HTTP status. Gitea answers 404 on this endpoint when the repo is absent or invisible to the
|
||||
credential, which says nothing about the base. Reusing 404 for the classifier's own
|
||||
nothing-can-govern verdict let that read reach the deny whose reason states "the full rule list
|
||||
was read and none matches" — a claim about a read that never happened.
|
||||
|
||||
No fixture emitted an HTTP 404 on the list before this test, which is exactly why reverting the
|
||||
sentinel to `bp_code=404` changed nothing observable.
|
||||
"""
|
||||
hook.set_branch_protection("LIST-404")
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason, "an unreadable repo was treated as evidence about the base"
|
||||
assert "none matches" not in reason, (
|
||||
"a 404 read claimed the full rule list had been read and matched nothing")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("shape", ["UNPARSEABLE-RULES", "GARBAGE", "EMPTY"])
|
||||
def test_a_200_the_classifier_cannot_PARSE_asks_without_blaming_the_transport(hook, shape):
|
||||
"""The twin of the `nomatch` fix, on the other arm — and pinned this time rather than assumed.
|
||||
|
||||
A rule whose `branch_name` is a number makes the classifier throw on a read that plainly
|
||||
succeeded. Mapping that to `bp_code=000` produced "could not read … (HTTP '000' — Gitea
|
||||
unreachable…)", stating a transport cause for a 200. The decision (ask) was always safe; only
|
||||
the reason lied, which is precisely the defect corrected one arm over for the deny.
|
||||
|
||||
Parametrised over all three shapes that reach a 200 the hook cannot use, because the first fix
|
||||
covered only `UNPARSEABLE-RULES` — the arm where it was noticed. `GARBAGE` (an object, not an
|
||||
array) and `EMPTY` are diverted one branch EARLIER, by the array gate, and kept `bp_code=200`,
|
||||
so they reported "HTTP '200' — Gitea unreachable" about a successful read. Fixing one arm and
|
||||
leaving its twin is the shape this PR is largely about.
|
||||
"""
|
||||
hook.set_branch_protection(shape)
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason, "an unparseable rule list produced a decision instead of a question"
|
||||
assert "could not parse" in reason, (
|
||||
"the ask blamed the transport for a 200 the classifier simply could not read")
|
||||
assert "unreachable" not in reason
|
||||
@@ -73,6 +73,10 @@ if "/pulls/" in url and not url.endswith("/files"):
|
||||
"state": (state / "pr_state").read_text().strip(),
|
||||
"html_url": "http://gitea.example/timothy/ersatztv/pulls/42",
|
||||
}
|
||||
# A 2xx body that merely LOST the field, as distinct from an unreachable PR ('GONE' above).
|
||||
# This is the shape the `[ -n "$x" ] &&` conjunct used to wave through (ersatztv#778).
|
||||
if sha == "NOHEAD":
|
||||
body["head"] = {}
|
||||
if base != "MISSING":
|
||||
body["base"] = {"ref": base}
|
||||
print(json.dumps(body))
|
||||
@@ -374,3 +378,34 @@ def test_each_verdict_word_posts_its_established_polarity(word, expected, gitea)
|
||||
"matching arm, so a token that has appeared in the other arm resolves there silently — and "
|
||||
"a BLOCKED verdict posting `success` writes a green required context."
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("head_seq", "base_seq", "field"),
|
||||
[
|
||||
(("NOHEAD",), ("main", "main"), "head sha"),
|
||||
((SHA_A,), ("main", "MISSING"), "base ref"),
|
||||
],
|
||||
)
|
||||
def test_a_reread_that_LOSES_a_field_refuses_instead_of_posting(head_seq, base_seq, field, gitea):
|
||||
"""The fail-OPEN one level below the TOCTOU guard, found by cold review (ersatztv#778).
|
||||
|
||||
Both re-read checks were written as `[ -n "$x" ] && [ "$x" != "$want" ]`. That conjunct makes an
|
||||
EMPTY value a no-op: a well-formed 2xx response that merely omits `.head.sha` or `.base.ref`
|
||||
yields an empty variable, neither comparison runs, and the status is posted having confirmed
|
||||
NOTHING about the head or the base — while the script's whole purpose at that point is to refuse
|
||||
unless it can confirm. The transport failure one line above was already fatal, which is exactly
|
||||
what made this shape easy to miss: the loud case was handled and the quiet one was not.
|
||||
|
||||
Asserted on the OBSERVABLE outcome — no status written — rather than on message text, so it
|
||||
still holds if the wording changes.
|
||||
"""
|
||||
gitea.set_head_sequence(SHA_A, *head_seq)
|
||||
gitea.set_base_sequence(*base_seq)
|
||||
result = gitea.run("42", "MERGEABLE")
|
||||
assert result.returncode != 0, (
|
||||
f"a re-read missing its {field} was accepted; the script posted a verdict having confirmed "
|
||||
"nothing about it")
|
||||
assert not gitea.statuses(), (
|
||||
f"a status was written despite the re-read carrying no {field} — this is the fail-open "
|
||||
"the -n conjunct created")
|
||||
|
||||
@@ -0,0 +1,314 @@
|
||||
"""`docs/remote-state-inventory.md` covers exactly the in-scope files, each classified for whether
|
||||
it reads live remote state and acts on that read (ersatztv#778).
|
||||
|
||||
WHAT THIS CANNOT DO, said first because #778's own issue body says it: there is no lint for "this
|
||||
code should have pinned a sha." `docs/defect-shapes-773.md` §4 grades detector D as a *fix pattern*
|
||||
whose detector is detector A applied to an enumerated inventory. So this file does not try to grade
|
||||
pinning. It guarantees that every in-scope file has been CLASSIFIED BY SOMEONE, and that no file
|
||||
joined the scope without acquiring a row — which converts "remember to think about this" into "the
|
||||
suite is red until you have".
|
||||
|
||||
The split is deliberate and mirrors `test_guard_inventory.py`:
|
||||
|
||||
* the POPULATION is derived from `git ls-files` and compared for SET EQUALITY, both directions;
|
||||
* the CLASSIFICATION vocabulary is closed, so a typo cannot invent a state;
|
||||
* whether a `PINNED` row is TELLING THE TRUTH is not checked here and cannot be. That stays with
|
||||
review, and the inventory's prose is what review reads.
|
||||
|
||||
SCOPE vs POPULATION, per `testing.guard-derives-population-from-source`: the SCOPE — four
|
||||
directories, their per-directory file patterns, and one excluded subdirectory — is a hand-written
|
||||
policy choice and is reviewable as one. The POPULATION inside that scope is derived on every run
|
||||
from the git index, with no content predicate at all: a file that reads no remote state earns an
|
||||
explicit `N/A` row rather than staying out, which is why most rows are `N/A`.
|
||||
|
||||
No count is written here on purpose. An exact "N of M" has now gone stale FOUR times in this change,
|
||||
most recently in the same commit that demoted two rows — a hand-maintained number is a second copy of
|
||||
the table, and `docs/guard-inventory.md` earns its counts by having a test assert them. Nothing
|
||||
asserts one here, so nothing states one.
|
||||
|
||||
That is the fourth version, and the history is the point — see the `SCOPE` comment below for the
|
||||
three that failed and what each one hid.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import fnmatch
|
||||
import re
|
||||
import subprocess
|
||||
from pathlib import Path
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
INVENTORY = REPO_ROOT / "docs" / "remote-state-inventory.md"
|
||||
|
||||
CLASSES = {"PINNED", "CAS", "UNSAFE-KNOWN", "N/A"}
|
||||
|
||||
# NO CONTENT FILTER. The population is every file in the scoped directories, and a file that reads
|
||||
# no remote state earns an `N/A` row rather than silently staying out.
|
||||
#
|
||||
# The first version filtered on a token list (`curl`, `wget`, `urllib`, ...) and called that a
|
||||
# SCOPE choice rather than a population filter. Cold review rejected the distinction and was right:
|
||||
# the list omitted `git fetch`, which is this repo's most common remote read, so
|
||||
# `.claude/hooks/prepush-rebase-check.sh` — which fetches `origin/main` and derives a PUSH DECISION
|
||||
# from it — was structurally invisible to a guard whose stated claim is "every executable that
|
||||
# reads live remote state". Three more (`prepush-clean-worktree-check.sh`, `ci-detect-docs-only.sh`,
|
||||
# `refresh-shared-checkout.sh`) were missing for the same reason.
|
||||
#
|
||||
# That is precisely the defect `testing.guard-derives-population-from-source` describes: a filter
|
||||
# cannot see the member that is missing, because the absent member is not a row the predicate
|
||||
# rejected, it is a row that was never produced. The defence offered for it — "over-inclusion is the
|
||||
# safe direction" — was answered by the filter ALSO under-including. Enumerating the directories
|
||||
# costs more rows and has no blind spot; that is the trade the rule already made.
|
||||
SCOPE = (
|
||||
("scripts", ("*.sh", "*.py")),
|
||||
(".claude/hooks", ("*.sh",)),
|
||||
(".husky", ("*",)),
|
||||
(".gitea/workflows", ("*.yml", "*.yaml")),
|
||||
)
|
||||
|
||||
# THE POPULATION IS DERIVED FROM `git ls-files`, NOT FROM THE FILESYSTEM.
|
||||
#
|
||||
# This is the third time this population has been found incomplete or wrong, each time by a
|
||||
# different mechanism, and the third fix is the one that stops patching the traversal:
|
||||
#
|
||||
# 1. a content filter 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
|
||||
# structurally invisible;
|
||||
# 2. a non-recursive `Path.glob`, which missed four nested files including one that calls a live
|
||||
# ErsatzTV API and acts on the reply;
|
||||
# 3. `Path.rglob`, which is recursive and therefore ALSO enumerated `.husky/_/` — 17 husky shims
|
||||
# generated by `npm ci` via web/package.json's `prepare` script, gitignored (`.husky/_/.gitignore`
|
||||
# is `*`) and untracked. That made this guard RED on every developer checkout while staying green
|
||||
# in CI, whose `script-tests` job checks out and pip-installs but never runs `npm ci`. A guard
|
||||
# that fails everywhere except where it runs is worse than no guard: it trains its readers to
|
||||
# ignore it, and it would have done so on the artifact whose entire thesis is population
|
||||
# correctness.
|
||||
#
|
||||
# The lesson each time was the same one this repo already wrote down — derive the population from an
|
||||
# AUTHORITATIVE source — and the filesystem is not one. It reports build output, editor droppings and
|
||||
# anything else that happens to be on disk, and it varies per machine. The repo's index is
|
||||
# authoritative, versioned, identical for every checkout and for CI, and it excludes untracked
|
||||
# generated files by construction rather than by an exclusion list that must be maintained.
|
||||
#
|
||||
# `scripts/tests/` is still excluded explicitly, because those files ARE tracked. That exclusion is a
|
||||
# scope decision, reviewable in one line: they run only under pytest and authorize nothing. Their
|
||||
# network activity is CONFINED rather than absent — `test_hook_fire_log.py` starts a real
|
||||
# `http.server` on 127.0.0.1 and drives it with real `curl`, and several suites create real local git
|
||||
# remotes — but all of it is fixture state the test creates and tears down, so there is no live remote
|
||||
# to race.
|
||||
EXCLUDED_DIRS = ("scripts/tests",)
|
||||
|
||||
# `| ` + backticked path, optionally followed by ` — <site description>`, then the class cell.
|
||||
_ROW = re.compile(r"^\|\s*`([^`]+?)`[^|]*\|\s*`([^`]+)`\s*\|", re.M)
|
||||
|
||||
|
||||
def _tracked_files() -> list[str]:
|
||||
"""Every file git tracks, as repo-relative posix paths.
|
||||
|
||||
Fails LOUDLY rather than returning nothing: an empty population would make every completeness
|
||||
assertion below pass vacuously, which is the exact failure this guard exists to prevent.
|
||||
"""
|
||||
proc = subprocess.run(
|
||||
["git", "-C", str(REPO_ROOT), "ls-files", "-z"],
|
||||
capture_output=True, check=True,
|
||||
)
|
||||
return [p for p in proc.stdout.decode().split("\0") if p]
|
||||
|
||||
|
||||
def derived_population() -> set[str]:
|
||||
"""Every in-scope tracked file, as a repo-relative posix path.
|
||||
|
||||
No content predicate of any kind: a file that reads no remote state earns an explicit `N/A` row
|
||||
rather than silently staying out.
|
||||
"""
|
||||
found: set[str] = set()
|
||||
for path in _tracked_files():
|
||||
if any(path == d or path.startswith(d + "/") for d in EXCLUDED_DIRS):
|
||||
continue
|
||||
for directory, globs in SCOPE:
|
||||
if not (path == directory or path.startswith(directory + "/")):
|
||||
continue
|
||||
name = path.rsplit("/", 1)[-1]
|
||||
if any(fnmatch.fnmatch(name, pattern) for pattern in globs):
|
||||
found.add(path)
|
||||
# First matching scope entry wins. Safe only while no scope directory nests inside
|
||||
# another; if one ever does, the inner entry's patterns would be silently skipped.
|
||||
break
|
||||
return found
|
||||
|
||||
|
||||
def _inventory_section(text: str) -> str:
|
||||
"""Only the classification tables, never the surrounding prose.
|
||||
|
||||
This is the second time this parser has read the document's own explanation as data: the
|
||||
UNSAFE-KNOWN justification check once parsed the "Columns" paragraph that DEFINES
|
||||
`UNSAFE-KNOWN`, and adding a scope TABLE to the heading made three more prose rows parse as
|
||||
sites. A guard that treats its own documentation as input is the failure this whole change is
|
||||
about, so the boundary is explicit rather than left to a cleverer regex.
|
||||
"""
|
||||
for heading in ("## The inventory", "## Limits"):
|
||||
if heading not in text:
|
||||
raise AssertionError(
|
||||
f"{INVENTORY.name} has no {heading!r} heading. Row parsing is bounded by "
|
||||
"'## The inventory' and '## Limits'; renaming or reordering either one would "
|
||||
"silently change which rows are checked, so it fails here instead.")
|
||||
start = text.index("## The inventory")
|
||||
try:
|
||||
end = text.index("## Limits", start)
|
||||
except ValueError:
|
||||
# Reachable when '## Limits' exists but PRECEDES '## The inventory' — the presence check
|
||||
# above passes and the bounded search does not. An earlier version put an `end <= start`
|
||||
# guard here instead, which `str.index(…, start)` makes unreachable by construction: it
|
||||
# either returns an index >= start or raises. A guard that cannot execute proves nothing.
|
||||
raise AssertionError(
|
||||
f"{INVENTORY.name}: '## Limits' precedes '## The inventory', so the parsed window "
|
||||
"would be empty and every completeness assertion would pass vacuously.") from None
|
||||
return text[start:end]
|
||||
|
||||
|
||||
def inventory_rows(text: str | None = None) -> list[tuple[str, str]]:
|
||||
if text is None:
|
||||
text = INVENTORY.read_text(encoding="utf-8")
|
||||
return _ROW.findall(_inventory_section(text))
|
||||
|
||||
|
||||
def inventory_sites(text: str | None = None) -> set[str]:
|
||||
return {site for site, _ in inventory_rows(text)}
|
||||
|
||||
|
||||
def test_the_inventory_file_exists_and_is_not_empty():
|
||||
assert INVENTORY.is_file(), f"{INVENTORY} is missing"
|
||||
assert INVENTORY.stat().st_size > 0
|
||||
|
||||
|
||||
def test_anti_vacuity_the_derivation_and_the_table_both_found_something():
|
||||
"""The characteristic failure of a completeness check is reporting success over an empty
|
||||
population. Both sides get a floor, because either one collapsing to zero would make the set
|
||||
comparison below pass trivially."""
|
||||
population = derived_population()
|
||||
sites = inventory_sites()
|
||||
assert len(population) >= 40, (
|
||||
f"derived only {len(population)} in-scope files — the globs are broken, not the repo "
|
||||
"(the scope held 59 files on 2026-08-16, and it only grows)")
|
||||
assert len(sites) >= 40, (
|
||||
f"parsed only {len(sites)} rows out of the inventory — the row regex has drifted from the "
|
||||
"table format")
|
||||
|
||||
|
||||
def test_every_in_scope_file_has_a_row_and_every_row_names_a_real_file():
|
||||
"""Set equality in BOTH directions, because the two failures are different defects and a single
|
||||
'sets differ' message invites fixing one and re-running.
|
||||
|
||||
MISSING: a script that talks to a remote service and was never classified — the defect #778
|
||||
exists to prevent. PHANTOM: a row for a file that was renamed or deleted, which leaves the table
|
||||
claiming coverage it has lost.
|
||||
"""
|
||||
population = derived_population()
|
||||
sites = inventory_sites()
|
||||
missing = sorted(population - sites)
|
||||
phantom = sorted(sites - population)
|
||||
assert not missing, (
|
||||
"in scope but absent from docs/remote-state-inventory.md (classify each as "
|
||||
f"PINNED / CAS / UNSAFE-KNOWN / N/A): {missing}")
|
||||
assert not phantom, (
|
||||
f"listed in docs/remote-state-inventory.md but no such in-scope file exists: {phantom}")
|
||||
|
||||
|
||||
def test_MUTATION_PROOF_a_dropped_row_and_a_phantom_row_are_both_detected():
|
||||
"""The proof that the set comparison above is load-bearing (`testing.guard-ships-with-mutation-
|
||||
proof`). This guard IS a test, so disarming it makes it absent rather than red; the admissible
|
||||
proof is therefore the contrapositive — introduce the defect into an isolated copy of the
|
||||
GUARDED ARTIFACT and show the comparison reports it.
|
||||
|
||||
Both directions are mutated, because they are different defects: a dropped row is an
|
||||
unclassified script, a phantom row is a table claiming coverage it has lost. This ran for real
|
||||
on the day it was written — `dependency-scan.yml` was genuinely absent from the first draft of
|
||||
the inventory and this comparison is what found it.
|
||||
"""
|
||||
text = INVENTORY.read_text(encoding="utf-8")
|
||||
population = derived_population()
|
||||
|
||||
victim = sorted(population)[0]
|
||||
dropped = "\n".join(
|
||||
line for line in text.splitlines() if not line.startswith(f"| `{victim}`"))
|
||||
assert victim not in inventory_sites(dropped), (
|
||||
f"the mutation did not actually remove {victim}; the proof below would be vacuous")
|
||||
assert population - inventory_sites(dropped), (
|
||||
"a row was removed from the inventory and the comparison still reported complete coverage")
|
||||
|
||||
# Inserted INSIDE the inventory section, not appended to the file: rows are parsed only between
|
||||
# "## The inventory" and "## Limits", so appending at the end would test nothing.
|
||||
phantom = text.replace(
|
||||
"## Limits",
|
||||
"| `scripts/does-not-exist.sh` — invented | `PINNED` | n/a |\n\n## Limits", 1)
|
||||
assert inventory_sites(phantom) - population == {"scripts/does-not-exist.sh"}, (
|
||||
"a row naming a file that does not exist was not reported as phantom")
|
||||
|
||||
|
||||
def test_every_class_cell_comes_from_the_closed_vocabulary():
|
||||
bad = sorted({cls for _, cls in inventory_rows() if cls not in CLASSES})
|
||||
assert not bad, (
|
||||
f"unknown classification(s) {bad}; allowed: {sorted(CLASSES)}. A typo here would silently "
|
||||
"create a state nobody reviews.")
|
||||
|
||||
|
||||
def test_every_unsafe_row_states_why_the_residual_is_accepted():
|
||||
"""`UNSAFE-KNOWN` means 'accepted with a reason', not 'noticed'. A row that records the window
|
||||
without the argument for tolerating it is how a deferral becomes permanent by default."""
|
||||
text = _inventory_section(INVENTORY.read_text(encoding="utf-8"))
|
||||
thin = []
|
||||
for line in text.splitlines():
|
||||
# TABLE ROWS ONLY. The first version matched any line containing the token, so the prose in
|
||||
# "Columns" that DEFINES `UNSAFE-KNOWN` was parsed as a row and the split blew up. A guard
|
||||
# that reads its own documentation as data is the failure this whole change is about.
|
||||
if not line.startswith("|") or "`UNSAFE-KNOWN`" not in line:
|
||||
continue
|
||||
cells = [c.strip() for c in line.strip().strip("|").split("|")]
|
||||
if len(cells) < 3:
|
||||
continue
|
||||
site, note = cells[0], cells[-1]
|
||||
if len(note) < 120:
|
||||
thin.append(site[:60])
|
||||
assert not thin, (
|
||||
f"UNSAFE-KNOWN row(s) with no stated justification: {thin}")
|
||||
|
||||
|
||||
def test_the_population_never_includes_a_file_git_does_not_track(monkeypatch):
|
||||
"""The regression for the third population defect, and the reason the source is the index.
|
||||
|
||||
`Path.rglob` enumerated `.husky/_/` — 17 husky shims generated by `npm ci`, gitignored and
|
||||
untracked — so this guard was RED on every developer checkout and GREEN in CI, which never runs
|
||||
`npm ci`. A guard that fails everywhere except where it runs trains its readers to ignore it.
|
||||
|
||||
Asserted through the mechanism rather than against the current disk, so it holds on a machine
|
||||
that has never installed husky: the tracked list is narrowed, and anything outside it must
|
||||
disappear from the population even though it is still sitting on disk and still matches the
|
||||
scope globs.
|
||||
"""
|
||||
real = derived_population()
|
||||
assert real, "empty population — the derivation is broken, not the repo"
|
||||
|
||||
victim = sorted(real)[0]
|
||||
tracked = [p for p in _tracked_files() if p != victim]
|
||||
monkeypatch.setattr(
|
||||
"scripts.tests.test_remote_state_inventory._tracked_files",
|
||||
lambda: tracked,
|
||||
raising=False,
|
||||
)
|
||||
# Patch the module object this test is running inside, whatever name it was imported under.
|
||||
import sys
|
||||
mod = sys.modules[__name__]
|
||||
monkeypatch.setattr(mod, "_tracked_files", lambda: tracked)
|
||||
|
||||
assert (REPO_ROOT / victim).is_file(), (
|
||||
f"{victim} must still exist on disk for this proof to mean anything")
|
||||
assert victim not in derived_population(), (
|
||||
f"{victim} is on disk and matches the scope, but git no longer tracks it — it must not enter "
|
||||
"the population, or untracked build output can redden this guard again")
|
||||
|
||||
|
||||
def test_every_derived_member_is_tracked():
|
||||
"""The same property stated as an invariant over the real tree, so a future refactor that goes
|
||||
back to walking the filesystem fails here rather than on someone's laptop."""
|
||||
tracked = set(_tracked_files())
|
||||
stray = sorted(p for p in derived_population() if p not in tracked)
|
||||
assert not stray, f"population contains untracked path(s): {stray}"
|
||||
Reference in New Issue
Block a user