diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index 8412c1218..62e578078 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -96,8 +96,9 @@ sha=$(printf '%s' "$prjson" | jq -r '.head.sha // ""' 2>/dev/null || true) body=$(printf '%s' "$prjson" | jq -r '.body // ""' 2>/dev/null || true) # --- Docs-only exemption: if every changed file is docs/process, skip the gate. --- -# The file list must be enumerated EXHAUSTIVELY, validated row by row, and bound to ONE head, or the -# exemption is unsafe. ALL of that now lives in scripts/pr-changed-files.sh — the single shared +# The file list must be enumerated EXHAUSTIVELY, validated row by row, and checked for head/base +# movement across the paging round trips, or the exemption is unsafe. (That check detects ONE-WAY +# movement only — this said "bound to ONE head" until 2026-08-28, ersatztv#803.) ALL of that now lives in scripts/pr-changed-files.sh — the single shared # implementation, also called by .gitea/workflows/review-verdict.yml (ersatztv#649). # # Why it moved: this logic was written twice. This copy is ADVISORY (a failure produces a human @@ -212,6 +213,48 @@ 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 + +# THE HEAD IS RE-READ AT THE SAME HOIST, FROM THE SAME RESPONSE (ersatztv#803). +# +# `$sha` comes from the PR snapshot at the top of this hook, and until 2026-08-28 every later check +# consumed that captured value: the CI combined status, the `review-verdict/h10` status, and the +# verdict-comment classification were all evaluated against `/commits/$sha/status` and `--head $sha`. +# A push landing in the gap — which includes the docs-only enumeration's up-to-forty round trips — +# was therefore checked against the commit it had just replaced, and the hook would report "a +# positive Review-verdict references the current head" about a head that was no longer current. +# +# This is the SAME defect the base had until #778 hoisted the re-read above, and it is fixed the same +# way rather than a different way. Reading `.head.sha` off `$prjson_now` — the response the base +# check already fetched — costs NO extra round trip, and it keeps the two axes on ONE snapshot, so +# they cannot disagree about which moment they describe. Two separate reads would answer about two +# different instants while reading as one check. +# +# DENY, not ask, and for the same reason the `stale` verdict class denies: a head that moved means +# the verdict this hook is about to accept covers an OLDER commit, which is a state we have +# positively established rather than failed to establish. An UNREADABLE `.head.sha` is the different +# case and asks. +# +# WHAT THIS DOES NOT CLOSE, said here rather than left to be inferred. A push landing after this +# check still passes, exactly as a retarget does — the file's rule against a second re-read applies +# unchanged (see the branch-protection block below), because two reads only move the window rather +# than closing it. That residual is bounded server-side and this hook is not what bounds it: the new +# head has no `review-verdict/h10` status, and that context is REQUIRED on `main`, so Gitea refuses +# the merge (#622). The hook's job here is to stop CLAIMING a head is reviewed when it can see that +# it is not — an advisory gate that states something false is worse than one that asks. +if [ -n "$sha" ]; then + sha_now=$(printf '%s' "$prjson_now" | jq -r '.head.sha // ""' 2>/dev/null || true) + if [ -z "$sha_now" ]; then + decide ask "H10 merge gate: PR #$pr reports no head commit (.head.sha) on re-read, so whether the review verdict still covers the current head could not be confirmed. Check the PR, then merge." + fi + if [ "$sha_now" != "$sha" ]; then + decide deny "H6/H10 merge gate: BLOCKED — PR #$pr's head moved from ${sha:0:7} to ${sha_now:0:7} while this gate was evaluating. Every check formed against ${sha:0:7} — the changed-file enumeration, the CI status and the review verdict — describes a commit that is no longer the one being merged (ersatztv#803). Re-review the current head and run: scripts/post-review-verdict.sh $pr MERGEABLE" + fi + # From here on `$sha` is the freshly-confirmed head; the two are equal by the check above. Mirrors + # `base_ref=$base_now` a few lines up, and is written for the same reason that one is: it makes the + # value every later check consumes the one that was just re-read, so a future edit moving a + # consumer above this point fails visibly rather than silently reading the stale capture. + sha=$sha_now +fi 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 diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index 4d1a521ff..1e2cf37cd 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -135,7 +135,7 @@ jobs: # `contents: write` would also work but additionally grants `releases: write`, which is not used. permissions: code: write # POST /statuses/{sha} (the gate) + the /commits/{sha}/status reads - issues: read # GET /issues/{n}/timeline — the retarget-count fence (#706) + issues: read # GET /issues/{n}/timeline — the retarget+push count fence (#706, head axis #803) pull-requests: read # scripts/pr-changed-files.sh -> GET /pulls/{n}/files (#698 exemption path) env: CI_JOB_ROLE: guard @@ -621,7 +621,7 @@ jobs: fi } - # --- The retarget fence (ersatztv#706 race 1) ---------------------------------------- + # --- The PR-mutation fence (ersatztv#706 race 1; head axis ersatztv#803/#664) -------- # THE PROBLEM THIS NARROWS — not solves, and the difference is #849's. Two # `pull_request_target` runs for one PR overlap, and the OLDER # one can finish LAST — so a run that classified against a base the PR no longer targets can @@ -665,10 +665,73 @@ jobs: # trusted ONLY when paging reached a validated EMPTY page. A short page, a non-array body, a # non-numeric length or the page cap all leave `rt_ok=no`, and an untrusted count is treated # below as "cannot tell" rather than as zero. - count_retargets() { + # THE SAME WALK CARRIES A SECOND AXIS: HEAD MUTATION (ersatztv#803/#664). + # + # The base fence above closes `main -> S -> main`. The HEAD aliases the same way and was + # fenced by nothing: a force-push `H1 -> H2 -> H1` during `pr-changed-files.sh`'s paging + # leaves its final `.head.sha` comparison equal while the middle pages were enumerated + # against `H2`, so a mixed file list can produce a docs-only exemption no single head ever + # justified. That script cannot close it alone — the binding it performs is a post-hoc + # comparison of one value against itself, which is the definition of ABA-vulnerable — and + # pinning the diff to two shas is still not available: re-measured on THIS instance at + # Gitea 1.27.1 on 2026-08-28, `GET /compare/{base}...{head}` returns `total_commits` and + # `commits` and NO `files` key (probed over a 12-commit range). So the fix has to be a + # caller-side monotonic counter, exactly as it was for the base. + # + # `pull_push` IS THAT COUNTER, and it is the same shape of key as `change_target_branch`: + # the timeline records EVERY push to the PR branch as one `pull_push` event whose body is + # `{"is_force_push": bool, "commit_ids": [...]}`. Measured on this instance at 1.27.1 + # (2026-08-28): PR #802 carries eighteen, all `is_force_push: false`; PR #761 carries five, + # four of them `true`. An `H1 -> H2 -> H1` round trip is necessarily TWO pushes, so the + # count moves by two where the sha moves by zero. The shape is not hypothetical — PR #761 + # really did go `8798a1d -> 830a407 -> 8798a1d` (26 minutes apart, so no enumeration + # straddled it, but the ALIAS is what the sha check cannot see whatever the interval). + # + # WHY THE COUNT AND NOT `is_force_push`. Filtering to force-pushes only would be the + # tempting narrowing and it is wrong twice over: an ordinary non-force push also invalidates + # a mid-flight enumeration (it moves the head, and `pr-changed-files.sh`'s own one-way check + # exists precisely for that), and a `H1 -> H2 -> H1` restoration can be performed with the + # SECOND push non-forced when `H1` is an ancestor. Every push is counted; the boolean is + # deliberately not read, and the body is never parsed — a counter that had to parse + # `commit_ids` would fail closed on any body shape change, which is a worse trade than + # counting rows by `.type` alone. + # + # WHY THIS DOES NOT ABSTAIN ON ITS OWN TRIGGER — the objection that would make this a + # permanent stall rather than a fence. A `synchronize` run is caused by a push, and if that + # push's timeline event were created AFTER the run started, `count_before` would miss it, + # `count_after` would see it, and every such run would abstain forever. + # + # It is created first, and that is an ORDERING PROPERTY OF THE SOURCE rather than a lucky + # margin: in Gitea v1.27.1 the push comment is written BEFORE the synchronize notification + # is emitted, so a run always sees its own causative event, and retries and re-runs add no + # new event. The measurement corroborates rather than carries the claim — across 20 + # triggered (push -> `pull_request_target` run) pairs on PRs #802, #834 and #761 the + # `pull_push` event predates its own run's `started_at` by 26-102s, itself a lower bound + # since the job runs a checkout and several steps before counting (69s end to end on run + # 2385). An earlier version of this comment rested the claim on that margin alone and + # called it a deployment property; see `ci.verdict-write-retarget-fence`, which also names + # the one window this does NOT cover — a PR's FIRST push is recorded about a second after + # creation and fires no `synchronize`, so an `opened` run landing inside it would abstain + # with no successor queued. + # + # The termination argument transfers from the base axis with ONE exception, which is the + # window named just above and must not be dropped here: a push to an ALREADY-OPEN PR fires + # `synchronize`, which is in this workflow's `types:` (see the header), so the very event + # that makes this run abstain has already queued a successor and abstention hands off. A + # PR's FIRST push does not — it is recorded about a second after creation and fires no + # `synchronize` — so an `opened` run abstaining on it has no successor coming. "Transfers + # unchanged" was the wording here until 2026-08-28 and was the overclaim, regenerated one + # paragraph below the fix that removed it. + # + # ONE WALK, TWO COUNTERS, ONE TRUST FLAG. The two axes share `rt_ok` because they share the + # pages it certifies: a walk that never reached a validated empty page has not established + # EITHER count, and giving them separate flags would invite a future edit to trust one over + # pages that did not certify the other. + count_pr_mutations() { rt_count=0 + hp_count=0 rt_ok=no - local page=1 raw n m kind total=0 + local page=1 raw n m p bad kind total=0 ptotal=0 while [ "$page" -le 20 ]; do raw=$(gh "$BASE_URL/repos/$REPO/issues/$PR/timeline?limit=50&page=${page}") || return 0 if [ -z "${raw//[[:space:]]/}" ]; then return 0; fi @@ -708,19 +771,78 @@ jobs: # which withholds the exemption and asks for a human verdict — the safe direction. # This narrows rather than closes the general concern: a wrong `null` on page 3 is still # read as exhaustion, and no bounded number of round-trips can rule that out. + # + # AND THAT WRONG `null` IS CONSTRUCTIBLE, NOT HYPOTHETICAL (established 2026-08-28 by + # cross-family review of ersatztv#803, from the v1.27.1 source; tracked + # as ersatztv#870, because it predates #803 and defeats BOTH axes identically). The handler pages at the + # DATABASE level and filters AFTERWARDS: `FindComments` applies the LIMIT/OFFSET, then + # `ListIssueCommentsAndTimeline` drops every `CommentTypeCode` row and every + # cross-reference the reader cannot access, appending survivors to a `var apiComments + # []*api.TimelineComment` — a NIL slice. So a page whose rows are ALL inline-code + # comments serializes as bare `null` while later pages still hold events, and rows are + # ordered ASCENDING, so the events a fence cares about are the ones furthest from page + # 1. Fifty inline review comments, which a PR author can create on their own PR, are + # enough to truncate both walks at the same place; both counts then agree, the sha + # comparison agrees, and the ABA goes unseen. + # + # So DO NOT read the fence below as closing the head ABA outright. What it closes is + # the ABA on a PR whose timeline has no such truncating block — which is every PR this + # repo has, but is not a property anyone enforces. Stated here rather than left in the + # issue because the whole of #803 is that a contract asserting more than its code does + # is worse than no contract. case "$kind" in null) - if [ "$page" -gt 1 ]; then rt_ok=yes; rt_count=$total; fi + if [ "$page" -gt 1 ]; then rt_ok=yes; rt_count=$total; hp_count=$ptotal; fi return 0 ;; array) ;; *) return 0 ;; esac n=$(printf '%s' "$raw" | jq -r 'length') case "$n" in ''|*[!0-9]*) return 0 ;; esac - if [ "$n" -eq 0 ]; then rt_ok=yes; rt_count=$total; return 0; fi + # AN EMPTY **ARRAY** FIRST PAGE IS AS ANOMALOUS AS A `null` ONE (fixed 2026-08-28, + # ersatztv#803 review). The `null` arm above requires `page > 1` and the comment + # above it says "a PR that somehow has an empty first page falls through to + # `rt_ok=no`" — but this arm trusted `[]` on ANY page, so the code contradicted its + # own stated rule and the sentence describing the safe behaviour was false for one of + # the two empty shapes. A `[]` page 1 yielded `rt_ok=yes` over a ZERO count, which is + # a fail-OPEN: it certifies "nothing was retargeted and nothing was pushed" from a + # response that cannot be explained, and a docs-only PR is then exempted on it. The + # two shapes now take the SAME rule, which is also what stops a serializer change + # from silently switching the walk between a trusted and an untrusted terminator. + if [ "$n" -eq 0 ]; then + if [ "$page" -gt 1 ]; then rt_ok=yes; rt_count=$total; hp_count=$ptotal; fi + return 0 + fi + # VALIDATE THE FIELD THE SECURITY DECISION READS, on every row (ersatztv#803 review). + # Both tallies below select on `.type`; a row without a string `.type` matches neither + # and is silently treated as an irrelevant event. That is the same + # validate-what-you-consume rule `pr-changed-files.sh` already applies to every row it + # extracts, and the reason is the same: a shape we cannot read is not evidence that + # nothing happened. Every timeline row measured on this instance carries a string + # `.type`, so this rejects only shapes the server has not been observed to produce. + # + # COUNTED, NOT `jq -e`, and that is this function's own standing rule rather than a + # style preference. The `kind` read above says it in full: `jq -e` reports the + # truthiness of its LAST OUTPUT, which is the exit-status subtlety that already bit + # this workflow at jq 1.6 (ersatztv#647), and the runner still ships 1.6. A count read + # as a VALUE and then range-checked in shell cannot be read two ways, and it matches + # every other tally here. (The first draft of this guard used `jq -e`, safely by + # accident — the array is known non-empty by the check above, so the empty-input + # divergence could not fire — but "safe because of a property three lines up" is + # exactly the reasoning the rule exists to retire.) + bad=$(printf '%s' "$raw" | jq -r '[.[] | select(type != "object" or (.type | type) != "string")] | length') + case "$bad" in ''|*[!0-9]*) return 0 ;; esac + if [ "$bad" -ne 0 ]; then return 0; fi m=$(printf '%s' "$raw" | jq -r '[.[] | select(.type == "change_target_branch")] | length') case "$m" in ''|*[!0-9]*) return 0 ;; esac + # Counted in the SAME pass over the SAME page, so the head axis costs no extra round + # trip and cannot be certified by a different set of pages than the base axis. A + # non-numeric length here abandons BOTH counts, exactly as it does above — a page whose + # push tally is unreadable has not established the retarget tally either. + p=$(printf '%s' "$raw" | jq -r '[.[] | select(.type == "pull_push")] | length') + case "$p" in ''|*[!0-9]*) return 0 ;; esac total=$(( total + m )) + ptotal=$(( ptotal + p )) page=$(( page + 1 )) done return 0 @@ -757,13 +879,13 @@ jobs: # (114 rows): pages 1 and 2 return 50, page 3 returns 14, page 4 is the two bytes `[]`. # `/issues/{n}/timeline` returns bare `null` past the end and `/commits/{sha}/status` # returns `{"statuses": null}` — three distinct empty shapes on one instance, which is why - # `count_retargets` above and this walk deliberately do NOT share a terminator. + # `count_pr_mutations` above and this walk deliberately do NOT share a terminator. # # A SHORT PAGE IS NOT THE END. Page 3 above carried 14 rows and was still followed by a real # page 4, and more fundamentally a short page is indistinguishable from a truncated # response. Only an empty page is evidence, which costs exactly one extra round-trip. # - # AN EMPTY PAGE 1 IS LEGITIMATE HERE, unlike `count_retargets`. A head nothing has posted to + # AN EMPTY PAGE 1 IS LEGITIMATE HERE, unlike `count_pr_mutations`. A head nothing has posted to # yet genuinely has no statuses (and a bogus sha returns `[]` too — measured), so an empty # first page is an ordinary answer rather than the anomaly it is on a PR timeline. It yields # `ph_ok=yes` over zero rows. Copying the timeline walk's "one real page required" rule here @@ -791,7 +913,7 @@ jobs: raw=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA?limit=50&page=${page}") || raw="" if [ -n "${raw//[[:space:]]/}" ]; then # Read the type as a VALUE, not through `jq -e`, for the reason recorded at - # `count_retargets`: `jq -e` reports the truthiness of its last output, so it cannot + # `count_pr_mutations`: `jq -e` reports the truthiness of its last output, so it cannot # separate "the body is null" from "the predicate is false". kind=$(printf '%s' "$raw" | jq -r 'type' 2>/dev/null) || kind="" fi @@ -804,7 +926,7 @@ jobs: if [ "$try" -eq 1 ]; then sleep 1; fi done # `null` COUNTS AS AN EMPTY PAGE, though this endpoint returns `[]` today. An - # array-only gate is the exact shape of #751: `count_retargets` had one, the timeline + # array-only gate is the exact shape of #751: `count_pr_mutations` had one, the timeline # really did return `null` past the end, so the walk never reached a validated empty # page and the fence withheld EVERY exemption. Re-adopting that narrowing on a second # endpoint would be worse, because this walk's failure is the STICKY sentinel — every @@ -836,10 +958,11 @@ jobs: } ex_repair=no - count_retargets + count_pr_mutations retargets_before=$rt_count + pushes_before=$hp_count retargets_before_ok=$rt_ok - echo "Retarget fence: ${retargets_before} retarget event(s) observed before classifying (trusted=${retargets_before_ok})." + echo "Mutation fence: ${retargets_before} retarget event(s) and ${pushes_before} push event(s) observed before classifying (trusted=${retargets_before_ok})." # --- Whose verdict is it? (ersatztv#698 route 3) ------------------------------------- # This short-circuit used to exit on ANY existing `success`, which made an exemption this job @@ -968,12 +1091,27 @@ jobs: # termination only on a validated EMPTY page rather than a merely short one, and head-sha # binding across the paging round-trips. ersatztv#649. # - # READ THE EXIT STATUS, NEVER THE STDOUT OF A FAILED RUN. exit 0 means "complete and bound - # to $SHA"; anything else means "could not tell" and stdout is meaningless. That the - # script happens to print nothing on its failure paths is redundancy, not contract — - # `files` is therefore cleared explicitly rather than trusted to be empty. stderr is left - # attached to the job log on purpose: its diagnostic is the only thing that distinguishes - # a force-push mid-enumeration from a dead API. + # READ THE EXIT STATUS, NEVER THE STDOUT OF A FAILED RUN. Anything but exit 0 means "could + # not tell" and stdout is meaningless. That the script happens to print nothing on its + # failure paths is redundancy, not contract — `files` is therefore cleared explicitly + # rather than trusted to be empty. stderr is left attached to the job log on purpose: its + # diagnostic is the only thing that distinguishes a force-push mid-enumeration from a dead + # API. + # + # WHAT exit 0 ACTUALLY ASSERTS (corrected 2026-08-28, ersatztv#803/#664). It said "complete + # and bound to $SHA". The first half is right; the second claims more than the script can + # do, and this job is the caller for which the difference has consequences — a match here + # posts a green required status with nobody in the loop. The script's head check compares + # `.head.sha` against `$SHA` after paging, which catches every one-way move and NO alias: + # `H1 -> H2 -> H1` restores the expected value while the middle pages came from `H2`. So + # exit 0 means "complete, with no head or base movement OBSERVABLE from inside the + # enumeration". + # + # The alias is fenced HERE instead, by the monotonic `pull_push` count in + # `count_pr_mutations` — the write is withheld if the head branch was pushed at all while + # this job was classifying. That is why the fence and this call site must be read together: + # neither is sufficient alone, and the enumerator is the half that CANNOT be made + # sufficient (`ci.verdict-write-retarget-fence`). ENUM=./scripts/pr-changed-files.sh files="" complete=no @@ -1223,10 +1361,11 @@ jobs: exit 0 fi - # THE FENCE ITSELF (ersatztv#706 race 1). Re-count the retargets as late as possible and - # refuse to write anything if the PR was retargeted since this run began. See the long note - # at `count_retargets` for why this is a handoff rather than a stall, and why the count is - # the only key that survives an ABA. + # THE FENCE ITSELF (ersatztv#706 race 1; head axis ersatztv#803/#664). Re-count BOTH + # mutation axes as late as possible and refuse to write anything if either moved since this + # run began — the PR was retargeted, or its head branch was pushed. See the long note at + # `count_pr_mutations` for why this is a handoff rather than a stall, and why a COUNT is the + # only key that survives an ABA on either axis. # # The refusal covers `pending` as well as `success`, not just the dangerous write. A stale # `pending` over a fresh `success` is only a stall rather than a forged green, so gating it @@ -1246,13 +1385,27 @@ jobs: # `success` with that rejection below the new mark. Letting it through is still right — the # alternative strands every PR whenever the timeline is unreadable — but the trade is # "immediate block, later re-derivation risk", not "free". - count_retargets + count_pr_mutations if [ "$retargets_before_ok" = yes ] && [ "$rt_ok" = yes ] && [ "$rt_count" -ne "$retargets_before" ]; then echo "::notice::PR #${PR} was retargeted while this job was classifying (${retargets_before} -> ${rt_count} retarget events). This run's classification was computed against a base the PR may no longer target, so it posts NOTHING. The retarget fired an 'edited' event, so a successor run is already queued and will write the authoritative status for ${SHA:0:7}." exit 0 fi + # THE HEAD AXIS (ersatztv#803/#664). Separate arm, separate message, same rule — see the + # long note at `count_pr_mutations`. A push during this window means the file enumeration + # this run classified may have been paged across two different heads, and `.head.sha` + # equality at both ends cannot see that when the head was restored. The count can. + # + # THIS ARM IS NOT REDUNDANT WITH `pr-changed-files.sh`'s OWN CHECK, which is the reading to + # guard against: that check compares the head to `$SHA` and so catches every one-way move. + # What it cannot catch is the move that comes BACK. This arm is for the ABA specifically, + # and it is why the enumerator's contract now says "detects one-way movement" rather than + # "bound to one head". + if [ "$retargets_before_ok" = yes ] && [ "$rt_ok" = yes ] && [ "$hp_count" -ne "$pushes_before" ]; then + echo "::notice::PR #${PR}'s head branch was pushed while this job was classifying (${pushes_before} -> ${hp_count} push events). The file enumeration behind this run's classification may have been paged across more than one head, and a force-push that RESTORED ${SHA:0:7} would leave every sha comparison equal, so this run posts NOTHING. A push to an OPEN PR fires 'synchronize', so a successor run is normally already queued and will write the authoritative status for whatever head is current — with one exception worth knowing before you conclude this PR is stuck: a PR's FIRST push is recorded about a second after creation and fires no 'synchronize', so an 'opened' run that abstained on it has no successor coming and the PR needs a re-trigger: a push, or a title/body/base edit. A label or a comment fires none of this workflow's triggers and will NOT re-trigger it." + exit 0 + fi if { [ "$retargets_before_ok" != yes ] || [ "$rt_ok" != yes ]; } && [ "$state" = "success" ]; then - echo "::error::Could not establish a trusted retarget count for PR #${PR} (before=${retargets_before_ok}, after=${rt_ok}), so an exemption 'success' cannot be shown to have been computed against the PR's current base. Posting nothing; ${CONTEXT} stays absent, which blocks the merge. NOTE a later run only helps if the cause was transient — a PR whose timeline exceeds the page cap will fail this way on every run, and needs a human verdict." + echo "::error::Could not establish a trusted retarget/push count for PR #${PR} (before=${retargets_before_ok}, after=${rt_ok}), so an exemption 'success' cannot be shown to have been computed against the PR's current base, nor at a single head. Posting nothing; ${CONTEXT} stays absent, which blocks the merge. NOTE a later run only helps if the cause was transient — a PR whose timeline exceeds the page cap will fail this way on every run, and needs a human verdict." exit 0 fi diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 8f75717e7..bf18b0b3c 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -190,7 +190,11 @@ dropped-step shape. What shows each read unit was exercised is a different obser they are not interchangeable: - **`issues: read`** — both runs logged `Retarget fence: 0 retarget event(s) … (trusted=yes)`, which - requires `count_retargets` to have walked `/issues/{n}/timeline`. + requires the timeline walk to have read `/issues/{n}/timeline`. That log line and the function + behind it were renamed by ersatztv#803, which added the head axis to the same walk: the line now + reads `Mutation fence: N retarget event(s) and M push event(s) … (trusted=…)` and the function is + `count_pr_mutations`. The observation above is left as it was recorded — it is what those runs + printed — but grep for the new strings when reproducing it. - **`pull-requests: read`** — the positive control posted `=success`, and a `success` is reachable only through `complete=yes`, which requires `scripts/pr-changed-files.sh` to have paged `/pulls/{n}` and `/pulls/{n}/files` to a validated end. The fence and the start marker do **not** @@ -1645,6 +1649,12 @@ measured rather than assumed. Gitea 1.25.4 auto-cancels superseded `push` runs o statusless with nothing left to re-trigger it. Full measurements and the two surviving residuals: `ci.verdict-write-retarget-fence`. +Since ersatztv#803 that fence counts **two** event types on the one timeline walk: +`change_target_branch` for the base alias (`main -> S -> main`) and `pull_push` for the HEAD alias +(a force-push `H1 -> H2 -> H1` spanning `pr-changed-files.sh`'s paging, which leaves every sha +comparison equal). Either count moving withholds the write. The run log line is +`Mutation fence: N retarget event(s) and M push event(s) … (trusted=…)`. + The `push`-supersession half of that claim stays **1.25.4-dated on purpose** (ersatztv#747, 2026-08-28). It is not unobservable, but it is no longer reproducible ON DEMAND. No workflow triggers on a push to a *non-`main`* branch — `docker-build.yml` filters its `push` trigger to `main` plus `v*` @@ -1867,7 +1877,8 @@ assumption this bug falsifies. The strict test reads the raw scalar, and must ne ⚠️ **A page past the end of `/issues/{n}/timeline` is JSON `null`, not `[]`** — and this instance is not consistent between endpoints (`/issues/{n}/comments` returns `[]` when empty). The retarget -fence's `count_retargets` gated on `type == "array"`, so it read the real terminator as *unreadable*: +fence's timeline walk (`count_retargets`, renamed `count_pr_mutations` by #803) gated on +`type == "array"`, so it read the real terminator as *unreadable*: the walk never reached a validated empty page, `rt_ok` was never `yes` for **any** PR, and the fence therefore withheld **every** exemption `success`. Renovate and docs-only PRs got no status at all — the same user-visible outcome as the dropped step above, by a completely unrelated route. So fixing @@ -1890,11 +1901,12 @@ Two things kept it invisible, and both are worth generalising: The type is now read as a value (`case` over `jq -r 'type'`) rather than through `jq -e`, whose exit-status semantics already bit this workflow once at jq 1.6, and both `null` and `[]` terminate the walk. The regression test is parameterised over both shapes because both are live on this server. -`null` is accepted as exhaustion only from **page 2 on** — every real PR's first page carries events +An empty page — `null` OR `[]`, which take the SAME rule since ersatztv#803 — is accepted as +exhaustion only from **page 2 on** — every real PR's first page carries events (spot-checked non-empty across #752/#753/#749/#739/#717; the counts are deliberately not recorded here -because timelines grow and an earlier draft's five figures were stale within days), so a `null` first -page is anomalous rather -than empty, and the walk should not certify "no retarget happened" from a response it cannot explain. +because timelines grow and an earlier draft's five figures were stale within days), so an empty first +page is ANOMALOUS rather than EXHAUSTED, and the walk should not certify "no retarget happened" from a +response it cannot explain. **The same nil-slice shape bites `/commits/{sha}/status`** — a third instance, found by cold review of the fix for the second. A head with no statuses yet returns diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 654fad462..7161f7f62 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -63,11 +63,11 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.root-screenshot-guard` | The Husky `pre-commit` hook refuses a staged root-level `*.png` (belt-and-suspenders with the `.gitignore` rule); nested `*.png` real assets are unaffected. | 2026-07-12 | [link](records/ci/root-screenshot-guard.md) | | `ci.runner-placement` | No persistent Roslyn compiler server survives a CI build (`UseSharedCompilation=false` etc., runner env + Dockerfile `ENV`); every `services:` container gets its own explicit `--memory`/`--memory-swap`/`--cpus` cap (it does not inherit the job container's). | 2026-07-17 | [link](records/ci/runner-placement.md) | | `ci.script-tests-job` | The `scripts/tests/` pytest suite runs on every PR as a dedicated `script-tests` job in `pr-checks.yml` (`runs-on: small`, `setup-python` + `pip install pytest pyyaml`, `PYTHONPATH=. python3 -m pytest scripts/tests -q`; since #780 it also runs a pinned ruff over a `git ls-files` population first), unconditionally rather than behind a `scripts/**` path filter, and **never as a step inside `decisions-guard`** — a job whose reds a standing rule instructs sessions to ignore must never host a gate whose reds are real. Any new CI gate must be reachable by a failure that is unambiguously attributable to it. | 2026-07-26 | [link](records/ci/script-tests-job.md) | -| `ci.shared-pr-file-enumeration` | A PR's complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding, base-ref binding — see `ci.exemption-provenance` — and base-TIP binding, #707: the ref answers "did this PR RETARGET", the tip answers "did the base ADVANCE mid-enumeration", and only the second can see `/pulls/{n}/files` recomputing each offset-paged page against a moved base and dropping a path out of an already-consumed range; both ends of the window are bound, and an advance BEFORE the window is deliberately not an error, or ordinary churn on `main` would fail every open PR) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate. | 2026-07-26 | [link](records/ci/shared-pr-file-enumeration.md) | +| `ci.shared-pr-file-enumeration` | A PR's complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha and base-ref MOVEMENT DETECTION — one-way, never a binding: an A->B->A alias on either axis passes, #664/#803 — see `ci.exemption-provenance` — and base-TIP binding, #707: the ref answers "did this PR RETARGET", the tip answers "did the base ADVANCE mid-enumeration", and only the second can see `/pulls/{n}/files` recomputing each offset-paged page against a moved base and dropping a path out of an already-consumed range; both ends of the window are read and compared, and an advance BEFORE the window is deliberately not an error, or ordinary churn on `main` would fail every open PR) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate. | 2026-07-26 | [link](records/ci/shared-pr-file-enumeration.md) | | `ci.small-lane-git-only` | `runs-on: small` is defined by what a job does (git-only), not its usual runtime; the two `docker build` jobs (docker-build.yml, ci-image.yml) move to `ubuntu-latest` because their worst-case memory, not median runtime, was pinning the small lane's per-slot cap. | 2026-07-20 | [link](records/ci/small-lane-git-only.md) | | `ci.toolchain-image-publish-is-a-dispatch` | A `push` trigger reachable from any ref other than `main` — that is `branches:` AND `tags:`, judged by the ref class it admits and never by which keyword is present — executes ref-supplied YAML, because Gitea resolves a `push` workflow's definition from the pushed ref — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. Be precise about what a `branches:` filter buys: it is loaded from the pushed ref like the rest of the file, so a branch that deletes it re-enables the route — this removes the DRIVE-BY case, and is not a boundary against a writer who intends to run their own YAML. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`'s `expected` in the same change — a DECIDED tradeoff, not a necessity: keeping it is workable via the branch dispatch, but prices every edit to that file, comments included, at a ~2GB publish plus a five-pin bump, redone after every rebase. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, and the ONLY remedy is to make it alongside a `docker/ci/**` edit: publishing after the merge and then pinning cannot work, because `expected` is the last `docker/ci` commit and would reject that pin. This closes the push route INTO THIS FILE, not the class: `docker-build.yml` remains reachable from an arbitrary ref by a `v*` tag push and by `pull_request`, and four workflows carry an unrestricted `workflow_dispatch` (#853). | 2026-08-27 | [link](records/ci/toolchain-image-publish-is-a-dispatch.md) | | `ci.ui-e2e-harness` | The UI-interactive E2E flows run as headless Playwright specs (`web/e2e/*.spec.ts`, driven by `scripts/e2e-ui.sh`) in a **second step of the existing advisory `functional-e2e` job**, never their own job; the browser is `chromium-headless-shell` **baked into the CI toolchain image** (`docker/ci/Dockerfile`, `PLAYWRIGHT_VERSION` kept equal to `web/package.json`'s EXACT `@playwright/test` pin), never installed per run; specs are `serial` with `retries: 0` and assert only contracts the curl harness structurally cannot reach. | 2026-07-25 | [link](records/ci/ui-e2e-harness.md) | -| `ci.verdict-write-retarget-fence` | The `review-verdict/h10` job counts `change_target_branch` events on the PR's issue timeline at run start and again immediately before its POST, and writes NOTHING if the count moved. The COUNT is the key because the branch NAME is ABA-vulnerable — `main -> S -> main` reads `main` at both ends, which is how #698 route 1 obtained a forged exemption — while the event count is monotonic and cannot alias. This NARROWS the residual, it does not resolve it — a retarget between the FINAL pre-write count and the POST still yields a PERMANENT forged green, because the successor run can consume the `edited` event and exit before the stale run posts last (corrected 2026-08-27, #849). Abstaining is a handoff, not a stall, and that is the property the design rests on — for a run that ABSTAINS; it says nothing about one that already passed its final count and then posts (see the residual, corrected #849): every retarget fires `edited`, which is in this workflow's `types:`, so the event that makes a run abstain has already queued a successor whose window opens after it; the induction terminates when retargeting stops — but ONLY over runs that ABSTAIN. A run that already passed its final count is outside it: it writes whenever it gets there, so the run that writes LAST is not necessarily the one that classified last, and that is the permanent residual (#849). `updated_at` was REJECTED as the key because it also moves for comments and labels, which fire none of this workflow's `types:` — a run could abstain with no successor coming, which is a real stall. The count is trusted only when paging reached a validated EMPTY page; an untrusted count (unreadable page, non-array body, non-numeric length, page cap hit) blocks the exemption `success` ONLY and still lets `pending` through, because `pending` blocks the merge immediately while withholding it would strand ordinary PRs whenever the timeline is unreadable — the right trade, but NOT a free one ("for no safety gain" retracted 2026-08-27): a GENERIC `pending` masks a rejection landing in its own write window, post-write verification does not run for it, and a later run re-derives it into an exemption `success` (#849). SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` IN FULL (PAGED since #763) and, if a human `Review-verdict:` row appeared with an id ABOVE a high-water mark taken just before the POST, overwrites its own status with `pending` and logs an error. The repair is `pending`, NEVER a copy of the human's state, since re-posting their `failure` under the machine credential would attribute a human verdict to the job; its description is a SENTINEL that the classification refuses to grant an exemption over AND re-writes verbatim on every later run, so the block is a FIXED POINT rather than decaying — writing the generic `pending` description there instead erases the marker and the exemption simply returns one event later. The mark is captured BEFORE the last-moment re-read, not merely before the POST — a later mark leaves a multi-round-trip blind gap in which a verdict is neither seen by the re-read nor repaired afterwards. The id comparison is load-bearing: a mere presence test would fire forever on a base-mismatched verdict that `read_existing_verdict` deliberately declines to honour, deadlocking that PR's exemption permanently. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run's `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run's mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. BOTH `/statuses/{sha}` reads PAGE to a validated EMPTY page since #763 — `[]` on this endpoint, a THIRD terminator shape distinct from the timeline's bare `null` and the combined endpoint's `{"statuses": null}`, so each terminator is MEASURED per endpoint — never terminating on a SHORT page, retrying each page once, under a 20-page cap that validates at most 950 rows (the 20th request must be the empty terminator); correctness does not depend on the measured cap of 50, because any page size pages correctly. WHAT THE PAGING BUYS IS NOT WHAT #763 CLAIMED: under the DESC default page 1 already held the true maximum id AND every row newer than the mark, so a single-page read missed a raced verdict only if more than 50 rows were created INSIDE the write window — not merely on a head over 50 rows. What removed #761's stall is retiring the probe, not the walk; the walk's value is that the gate's one fail-toward-SUCCESS path no longer depends on an UNDOCUMENTED ordering the server honours only coarsely (page 1 came back `114,112,113,111,110`). This RETIRED #751's conservative page-2 probe, which treated "there are rows I did not read" as "assume raced" and so repaired every head that outgrew one page: it fired on Renovate PR #761, reporting a human verdict as overwritten when none existed and then, the sentinel being sticky, refusing to re-exempt that head on every later run. The two directions are NOT symmetric. POST-WRITE, uncertainty fails CLOSED — an unreadable history, an over-cap history, or a count that is not a number all repair to `pending`; previously an unreadable history warned and left the exemption green while the page-2 probe repaired on the same uncertainty, one check disagreeing with itself. PRE-WRITE, a PARTIAL list still yields a mark, because the mark gates the post-write check entirely and refusing one SKIPS that check, which is itself the fail-open; this rests on the DESC DEFAULT — the newest row, carrying the maximum id, is on page 1, so a walk that fails later still saw it — while a VALIDATED empty history is NOT abandoned (it yields a mark of 0, correct for a first run, since every later row is newer) — what abandons the mark is a read that both FAILED and returned nothing (the pre-existing #849 gap, unchanged) — and a NON-EMPTY history carrying no numeric id is reported unusable rather than collapsed to 0. BOTH id comparisons are NUMERIC-ONLY: jq orders strings above every number, so one `"id": "99999"` inflates the mark until nothing looks newer, and `.id > $since` reads any string id as newer than any mark — making a PRE-EXISTING base-mismatched verdict look raced on every run, a permanent per-sha stall (the twin was live on `main`). An EMPTY post-write history is REJECTED: the walk terminates on an empty page, which is correct before the write and impossible after it (one row per POST), and a well-formed "no statuses exist" is not retried — so accepting it would conclude `raced=0` from a list that cannot be real, silently. That is NOT the withdrawn currency witness, which asked whether ANY row sat above the mark and was satisfied by an unrelated newer row; this asks only whether the list is EMPTY, a state no unrelated row can produce. The `::error::` now names its own cause, of which there are THREE — a verdict actually FOUND, a read that could not be COMPLETED, and a read that completed but returned an IMPOSSIBLE answer (the third is not a variety of the second) — while the sentinel DESCRIPTION stays generic because the classification recognises it as a fixed point. `.creator` is TYPE-TESTED before it is indexed: `.creator != null and .creator.login` hard-errors on a non-object creator, jq exits 5, and under `set -e` that took the step down AFTER the exemption was posted and BEFORE the repair — a forged green reported as an infrastructure error. | 2026-08-03 | [link](records/ci/verdict-write-retarget-fence.md) | +| `ci.verdict-write-retarget-fence` | The `review-verdict/h10` job counts BOTH `change_target_branch` AND `pull_push` events on the PR's issue timeline at run start and again immediately before its POST, and writes NOTHING if EITHER count moved. The COUNT is the key on both axes because the underlying VALUE is ABA-vulnerable — `main -> S -> main` reads `main` at both ends, which is how #698 route 1 obtained a forged exemption, and a force-push `H1 -> H2 -> H1` leaves `.head.sha` equal at both ends while the middle pages of `scripts/pr-changed-files.sh`'s enumeration came from `H2` (#803/#664) — while an event count is monotonic and cannot alias. ONE walk certifies BOTH counts, so an unreadable page abandons both; the two tallies are separate so the diagnostic names the axis that actually moved. Every push is counted and `is_force_push` is deliberately NOT read: an ordinary push also invalidates a mid-flight enumeration, and an `H1 -> H2 -> H1` restoration can have its second push non-forced when `H1` is an ancestor. The head fence does NOT abstain on its own triggering push — established FROM THE v1.27.1 SOURCE, since that would be a permanent stall rather than a fence: the push comment is created BEFORE the synchronize notification is emitted, so a run always sees its own causative event, and retries add no new event. The 26-102s margin measured across 20 triggered pairs on PRs #802/#834/#761 corroborates it and is a lower bound (the job runs a checkout first; 69s end to end on run 2385) — it is NOT the basis of the claim, which an earlier draft said it was. This NARROWS the residual, it does not resolve it — a retarget between the FINAL pre-write count and the POST still yields a PERMANENT forged green, because the successor run can consume the `edited` event and exit before the stale run posts last (corrected 2026-08-27, #849). Abstaining is a handoff, not a stall, and that is the property the design rests on — for a run that ABSTAINS; it says nothing about one that already passed its final count and then posts (see the residual, corrected #849): every retarget fires `edited`, which is in this workflow's `types:`, so the event that makes a run abstain has already queued a successor whose window opens after it; the induction terminates when retargeting stops — but ONLY over runs that ABSTAIN. A run that already passed its final count is outside it: it writes whenever it gets there, so the run that writes LAST is not necessarily the one that classified last, and that is the permanent residual (#849). `updated_at` was REJECTED as the key because it also moves for comments and labels, which fire none of this workflow's `types:` — a run could abstain with no successor coming, which is a real stall. The count is trusted only when paging reached a validated EMPTY page; an untrusted count (unreadable page, non-array body, non-numeric length, page cap hit) blocks the exemption `success` ONLY and still lets `pending` through, because `pending` blocks the merge immediately while withholding it would strand ordinary PRs whenever the timeline is unreadable — the right trade, but NOT a free one ("for no safety gain" retracted 2026-08-27): a GENERIC `pending` masks a rejection landing in its own write window, post-write verification does not run for it, and a later run re-derives it into an exemption `success` (#849). SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` IN FULL (PAGED since #763) and, if a human `Review-verdict:` row appeared with an id ABOVE a high-water mark taken just before the POST, overwrites its own status with `pending` and logs an error. The repair is `pending`, NEVER a copy of the human's state, since re-posting their `failure` under the machine credential would attribute a human verdict to the job; its description is a SENTINEL that the classification refuses to grant an exemption over AND re-writes verbatim on every later run, so the block is a FIXED POINT rather than decaying — writing the generic `pending` description there instead erases the marker and the exemption simply returns one event later. The mark is captured BEFORE the last-moment re-read, not merely before the POST — a later mark leaves a multi-round-trip blind gap in which a verdict is neither seen by the re-read nor repaired afterwards. The id comparison is load-bearing: a mere presence test would fire forever on a base-mismatched verdict that `read_existing_verdict` deliberately declines to honour, deadlocking that PR's exemption permanently. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run's `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run's mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. BOTH `/statuses/{sha}` reads PAGE to a validated EMPTY page since #763 — `[]` on this endpoint, a THIRD terminator shape distinct from the timeline's bare `null` and the combined endpoint's `{"statuses": null}`, so each terminator is MEASURED per endpoint — never terminating on a SHORT page, retrying each page once, under a 20-page cap that validates at most 950 rows (the 20th request must be the empty terminator); correctness does not depend on the measured cap of 50, because any page size pages correctly. WHAT THE PAGING BUYS IS NOT WHAT #763 CLAIMED: under the DESC default page 1 already held the true maximum id AND every row newer than the mark, so a single-page read missed a raced verdict only if more than 50 rows were created INSIDE the write window — not merely on a head over 50 rows. What removed #761's stall is retiring the probe, not the walk; the walk's value is that the gate's one fail-toward-SUCCESS path no longer depends on an UNDOCUMENTED ordering the server honours only coarsely (page 1 came back `114,112,113,111,110`). This RETIRED #751's conservative page-2 probe, which treated "there are rows I did not read" as "assume raced" and so repaired every head that outgrew one page: it fired on Renovate PR #761, reporting a human verdict as overwritten when none existed and then, the sentinel being sticky, refusing to re-exempt that head on every later run. The two directions are NOT symmetric. POST-WRITE, uncertainty fails CLOSED — an unreadable history, an over-cap history, or a count that is not a number all repair to `pending`; previously an unreadable history warned and left the exemption green while the page-2 probe repaired on the same uncertainty, one check disagreeing with itself. PRE-WRITE, a PARTIAL list still yields a mark, because the mark gates the post-write check entirely and refusing one SKIPS that check, which is itself the fail-open; this rests on the DESC DEFAULT — the newest row, carrying the maximum id, is on page 1, so a walk that fails later still saw it — while a VALIDATED empty history is NOT abandoned (it yields a mark of 0, correct for a first run, since every later row is newer) — what abandons the mark is a read that both FAILED and returned nothing (the pre-existing #849 gap, unchanged) — and a NON-EMPTY history carrying no numeric id is reported unusable rather than collapsed to 0. BOTH id comparisons are NUMERIC-ONLY: jq orders strings above every number, so one `"id": "99999"` inflates the mark until nothing looks newer, and `.id > $since` reads any string id as newer than any mark — making a PRE-EXISTING base-mismatched verdict look raced on every run, a permanent per-sha stall (the twin was live on `main`). An EMPTY post-write history is REJECTED: the walk terminates on an empty page, which is correct before the write and impossible after it (one row per POST), and a well-formed "no statuses exist" is not retried — so accepting it would conclude `raced=0` from a list that cannot be real, silently. That is NOT the withdrawn currency witness, which asked whether ANY row sat above the mark and was satisfied by an unrelated newer row; this asks only whether the list is EMPTY, a state no unrelated row can produce. The `::error::` now names its own cause, of which there are THREE — a verdict actually FOUND, a read that could not be COMPLETED, and a read that completed but returned an IMPOSSIBLE answer (the third is not a variety of the second) — while the sentinel DESCRIPTION stays generic because the classification recognises it as a fixed point. `.creator` is TYPE-TESTED before it is indexed: `.creator != null and .creator.login` hard-errors on a non-object creator, jq exits 5, and under `set -e` that took the step down AFTER the exemption was posted and BEFORE the repair — a forged green reported as an infrastructure error. | 2026-08-03 | [link](records/ci/verdict-write-retarget-fence.md) | | `ci.verify-locally-ci-confirms` | Treat the local build/verify/review pass as the decision point and CI as confirmation — don't idle waiting on a run you have no reason to doubt. | 2026-07-21 | [link](records/ci/verify-locally-ci-confirms.md) | | `ci.web-test-per-test-timeouts` | Give heavy-render web tests an explicit per-test vitest timeout (e.g. 15s); never raise the global default to fix one slow test. | 2026-07-21 | [link](records/ci/web-test-per-test-timeouts.md) | | `ci.workflow-run-body-no-expressions` | A `run:` body is not shell when the runner reads it: the runner scans the whole scalar for the expression opener and, on finding one, rewrites the ENTIRE body into a single `format(...)` call. That rewrite is all-or-nothing, so a payload that does not evaluate fails the interpolation of the whole scalar — and the runner then DROPS THE STEP AND CONCLUDES THE JOB `success`. A shell comment is therefore NOT inert. In `.gitea/workflows/review-verdict.yml` no expression delimiter may appear in ANY `run:` body, in code or in prose, because a dropped step there is a dead merge gate rather than a failed build; pass values in through the step's `env:` block, which is interpolated per value so a bad payload cannot take the body with it, and describe an expression in prose by NAMING it (`a github.event.pull_request.number expression`) rather than quoting the delimiters. Repo-wide the rule is weaker and its reach must be stated precisely rather than generously: every expression payload in every workflow field must have a HEAD TOKEN naming a context or function the runner can resolve. That catches the defect above and a nonexistent context; it does NOT catch a syntactically invalid payload whose tokens are all known (`${{ github.ref == }}`), a renamed output (every token after the first is skipped), or an unclosed opener — those need an expression parser, and the guard is kept permissive on purpose because a red here blocks every merge through the combined status. In `review-verdict.yml` specifically, any step whose non-execution is consequential is paired with a start-marker guard that FAILS the job when the marker is absent, and that guard's own body must be expression-free — a guard the guarded mechanism can silently delete is worse than none. That pairing now also covers `docker-build.yml`'s `test` and `migrations` jobs, where a dropped step is fail-OPEN (the required check goes green having done no work) rather than fail-closed as it is here — see `ci.required-job-step-execution-markers`, which adds per-STEP markers there and extends this file's delimiter ban to those two jobs. It is still not a repo-wide property, but the remaining exceptions are narrower than this record originally said: `build` was brought into the ban too (its `Smoke + IPTV E2E` runs AFTER the image is pushed, so a drop there ships an unsmoked release candidate — its two payloads moved to `env:`, so the ban was free), leaving only `api-docs` and `format`, whose one `github.base_ref` each sits in a detect step that gates nothing that ships. | 2026-08-06 | [link](records/ci/workflow-run-body-no-expressions.md) | diff --git a/docs/decisions/records/ci/actions-credential-scoping.md b/docs/decisions/records/ci/actions-credential-scoping.md index 74802e2ff..a14b8cd22 100644 --- a/docs/decisions/records/ci/actions-credential-scoping.md +++ b/docs/decisions/records/ci/actions-credential-scoping.md @@ -119,8 +119,11 @@ shipped `review-verdict.yml`, so the gate's own shape is what was measured. - `code: read` -> job failure at the POST, `curl` exit 22 (HTTP >=400 under `-f`), NO status written. What each observation licenses, kept separate. `issues: read` was exercised: both runs logged -`Retarget fence: 0 retarget event(s) ... (trusted=yes)`, which requires `count_retargets` to have -walked `/issues/{n}/timeline`. `pull-requests: read` was exercised: the positive control posted +`Retarget fence: 0 retarget event(s) ... (trusted=yes)`, which requires the timeline walk to have +read `/issues/{n}/timeline`. (That line and `count_retargets` were renamed by #803, which put the +head axis on the same walk: `Mutation fence: N retarget event(s) and M push event(s) ...` and +`count_pr_mutations`. The quoted string is what those runs actually printed and is kept verbatim; +reproduce against the current names.) `pull-requests: read` was exercised: the positive control posted `=success`, reachable only through `complete=yes`, which requires `scripts/pr-changed-files.sh` to have paged `/pulls/{n}` and `/pulls/{n}/files` to a validated end — the fence and the start marker do NOT show this, the `success` does. Both reached the classify step's start marker, so neither was the diff --git a/docs/decisions/records/ci/shared-pr-file-enumeration.md b/docs/decisions/records/ci/shared-pr-file-enumeration.md index b94297b4a..065a78f91 100644 --- a/docs/decisions/records/ci/shared-pr-file-enumeration.md +++ b/docs/decisions/records/ci/shared-pr-file-enumeration.md @@ -5,9 +5,9 @@ status: active since: '2026-07-26' supersedes: none superseded-by: none -rule: 'A PR''s complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding, base-ref binding — see `ci.exemption-provenance` — and base-TIP binding, #707: the ref answers "did this PR RETARGET", the tip answers "did the base ADVANCE mid-enumeration", and only the second can see `/pulls/{n}/files` recomputing each offset-paged page against a moved base and dropping a path out of an already-consumed range; both ends of the window are bound, and an advance BEFORE the window is deliberately not an error, or ordinary churn on `main` would fail every open PR) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate.' +rule: 'A PR''s complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha and base-ref MOVEMENT DETECTION — one-way, never a binding: an A->B->A alias on either axis passes, #664/#803 — see `ci.exemption-provenance` — and base-TIP binding, #707: the ref answers "did this PR RETARGET", the tip answers "did the base ADVANCE mid-enumeration", and only the second can see `/pulls/{n}/files` recomputing each offset-paged page against a moved base and dropping a path out of an already-consumed range; both ends of the window are read and compared, and an advance BEFORE the window is deliberately not an error, or ordinary churn on `main` would fail every open PR) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate.' signals: 'duplicated PR file enumeration, enforced gate weaker than advisory hook, docs-only allow-list drift, shared mechanism not shared policy, pr-changed-files.sh, checkout base ref not PR head, gate judging its own PR, exhaustiveness bug in a security predicate · paths: `scripts/pr-changed-files.sh`, `.claude/hooks/pretooluse-merge-consent.sh`, `.gitea/workflows/review-verdict.yml` · issues: #643, #648, #649' -mechanics: '`scripts/pr-changed-files.sh ` -> stdout newline-delimited paths, exit 0 only if complete and bound to BOTH the given sha and the given base branch; the 5th argument is REQUIRED and a 4-arg call exits 2 (`ci.exemption-provenance`); callers: `.claude/hooks/pretooluse-merge-consent.sh`, `.gitea/workflows/review-verdict.yml`' +mechanics: '`scripts/pr-changed-files.sh ` -> stdout newline-delimited paths, exit 0 only if complete AND neither the given sha nor the given base branch was observed to move across the paging round trips — ONE-WAY detection, not a binding: an A->B->A alias on either axis passes (#664/#803), which is why the enforced caller adds a monotonic event-count fence; the 5th argument is REQUIRED and a 4-arg call exits 2 (`ci.exemption-provenance`); callers: `.claude/hooks/pretooluse-merge-consent.sh`, `.gitea/workflows/review-verdict.yml`' --- Before #649, the PR changed-file enumeration existed as two independent implementations. That would @@ -34,7 +34,8 @@ there is no mechanism that would surface a second drift before it mattered. Extr atomically. **Mechanism, not policy — the two allow-lists stay separate on purpose.** The extracted script -answers exactly one question: "what is the complete set of paths this PR touches, at one head, or can +answers exactly one question: "what is the complete set of paths this PR touches, with no head or base +movement observable from inside the enumeration, or can we not tell?" It does not decide whether that set makes the PR docs-only. Each caller keeps its own classification: @@ -78,9 +79,20 @@ in each caller: would otherwise yield a list belonging to no single commit. **This detects ONE-WAY movement only.** An A→B→A force-push round trip restores the expected sha, so the binding holds while the pages came from two different states — see #664. Closing that needs a commit-pinned files endpoint (Gitea has - none) or a local diff, not a tighter check here; the guarantee is stated narrowly rather than left + none — re-probed at 1.27.1 on 2026-08-28: `compare/{base}...{head}` still returns no `files` key) + or a local diff, not a tighter check here; the guarantee is stated narrowly rather than left to read as complete. + **Fenced at the ENFORCED caller since 2026-08-28 (#803), and only there.** The script's contract is + unchanged and still one-way, because nothing checkable inside it can do better. What changed is that + `review-verdict.yml` — the caller whose match posts a green required status with nobody in the loop — + now refuses to write if the PR timeline's `pull_push` count moved while it classified, a monotonic + key an alias cannot defeat (`ci.verdict-write-retarget-fence`). The advisory hook is deliberately not + given that fence: its failure mode is a human prompt, and it pays for the gap differently, by + re-reading `.head.sha` off the same response as its base re-read and DENYING if the head moved. So + the mechanism stays shared and single while the two callers keep buying different amounts of + protection with it — the same mechanism-not-policy split this record is about. + **Base-ref checkout — binds the SCRIPTS to the base, not the workflow itself.** `review-verdict.yml` checks out the PR's BASE ref (`ref: ${{ github.event.pull_request.base.sha }}`, `persist-credentials: false`), never the head, so the *scripts the job executes* — above all diff --git a/docs/decisions/records/ci/verdict-write-retarget-fence.md b/docs/decisions/records/ci/verdict-write-retarget-fence.md index ab16d7530..b73f2316e 100644 --- a/docs/decisions/records/ci/verdict-write-retarget-fence.md +++ b/docs/decisions/records/ci/verdict-write-retarget-fence.md @@ -1,13 +1,13 @@ --- key: ci.verdict-write-retarget-fence -title: '2026-08-03 — the review-verdict job fences its write on the PR timeline''s retarget COUNT, and verifies the exemption write afterwards (#706)' +title: '2026-08-03 — the review-verdict job fences its write on the PR timeline''s retarget AND push COUNTS, and verifies the exemption write afterwards (#706, head axis #803)' status: active since: '2026-08-03' supersedes: none superseded-by: none -rule: 'The `review-verdict/h10` job counts `change_target_branch` events on the PR''s issue timeline at run start and again immediately before its POST, and writes NOTHING if the count moved. The COUNT is the key because the branch NAME is ABA-vulnerable — `main -> S -> main` reads `main` at both ends, which is how #698 route 1 obtained a forged exemption — while the event count is monotonic and cannot alias. This NARROWS the residual, it does not resolve it — a retarget between the FINAL pre-write count and the POST still yields a PERMANENT forged green, because the successor run can consume the `edited` event and exit before the stale run posts last (corrected 2026-08-27, #849). Abstaining is a handoff, not a stall, and that is the property the design rests on — for a run that ABSTAINS; it says nothing about one that already passed its final count and then posts (see the residual, corrected #849): every retarget fires `edited`, which is in this workflow''s `types:`, so the event that makes a run abstain has already queued a successor whose window opens after it; the induction terminates when retargeting stops — but ONLY over runs that ABSTAIN. A run that already passed its final count is outside it: it writes whenever it gets there, so the run that writes LAST is not necessarily the one that classified last, and that is the permanent residual (#849). `updated_at` was REJECTED as the key because it also moves for comments and labels, which fire none of this workflow''s `types:` — a run could abstain with no successor coming, which is a real stall. The count is trusted only when paging reached a validated EMPTY page; an untrusted count (unreadable page, non-array body, non-numeric length, page cap hit) blocks the exemption `success` ONLY and still lets `pending` through, because `pending` blocks the merge immediately while withholding it would strand ordinary PRs whenever the timeline is unreadable — the right trade, but NOT a free one ("for no safety gain" retracted 2026-08-27): a GENERIC `pending` masks a rejection landing in its own write window, post-write verification does not run for it, and a later run re-derives it into an exemption `success` (#849). SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` IN FULL (PAGED since #763) and, if a human `Review-verdict:` row appeared with an id ABOVE a high-water mark taken just before the POST, overwrites its own status with `pending` and logs an error. The repair is `pending`, NEVER a copy of the human''s state, since re-posting their `failure` under the machine credential would attribute a human verdict to the job; its description is a SENTINEL that the classification refuses to grant an exemption over AND re-writes verbatim on every later run, so the block is a FIXED POINT rather than decaying — writing the generic `pending` description there instead erases the marker and the exemption simply returns one event later. The mark is captured BEFORE the last-moment re-read, not merely before the POST — a later mark leaves a multi-round-trip blind gap in which a verdict is neither seen by the re-read nor repaired afterwards. The id comparison is load-bearing: a mere presence test would fire forever on a base-mismatched verdict that `read_existing_verdict` deliberately declines to honour, deadlocking that PR''s exemption permanently. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run''s `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run''s mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. BOTH `/statuses/{sha}` reads PAGE to a validated EMPTY page since #763 — `[]` on this endpoint, a THIRD terminator shape distinct from the timeline''s bare `null` and the combined endpoint''s `{"statuses": null}`, so each terminator is MEASURED per endpoint — never terminating on a SHORT page, retrying each page once, under a 20-page cap that validates at most 950 rows (the 20th request must be the empty terminator); correctness does not depend on the measured cap of 50, because any page size pages correctly. WHAT THE PAGING BUYS IS NOT WHAT #763 CLAIMED: under the DESC default page 1 already held the true maximum id AND every row newer than the mark, so a single-page read missed a raced verdict only if more than 50 rows were created INSIDE the write window — not merely on a head over 50 rows. What removed #761''s stall is retiring the probe, not the walk; the walk''s value is that the gate''s one fail-toward-SUCCESS path no longer depends on an UNDOCUMENTED ordering the server honours only coarsely (page 1 came back `114,112,113,111,110`). This RETIRED #751''s conservative page-2 probe, which treated "there are rows I did not read" as "assume raced" and so repaired every head that outgrew one page: it fired on Renovate PR #761, reporting a human verdict as overwritten when none existed and then, the sentinel being sticky, refusing to re-exempt that head on every later run. The two directions are NOT symmetric. POST-WRITE, uncertainty fails CLOSED — an unreadable history, an over-cap history, or a count that is not a number all repair to `pending`; previously an unreadable history warned and left the exemption green while the page-2 probe repaired on the same uncertainty, one check disagreeing with itself. PRE-WRITE, a PARTIAL list still yields a mark, because the mark gates the post-write check entirely and refusing one SKIPS that check, which is itself the fail-open; this rests on the DESC DEFAULT — the newest row, carrying the maximum id, is on page 1, so a walk that fails later still saw it — while a VALIDATED empty history is NOT abandoned (it yields a mark of 0, correct for a first run, since every later row is newer) — what abandons the mark is a read that both FAILED and returned nothing (the pre-existing #849 gap, unchanged) — and a NON-EMPTY history carrying no numeric id is reported unusable rather than collapsed to 0. BOTH id comparisons are NUMERIC-ONLY: jq orders strings above every number, so one `"id": "99999"` inflates the mark until nothing looks newer, and `.id > $since` reads any string id as newer than any mark — making a PRE-EXISTING base-mismatched verdict look raced on every run, a permanent per-sha stall (the twin was live on `main`). An EMPTY post-write history is REJECTED: the walk terminates on an empty page, which is correct before the write and impossible after it (one row per POST), and a well-formed "no statuses exist" is not retried — so accepting it would conclude `raced=0` from a list that cannot be real, silently. That is NOT the withdrawn currency witness, which asked whether ANY row sat above the mark and was satisfied by an unrelated newer row; this asks only whether the list is EMPTY, a state no unrelated row can produce. The `::error::` now names its own cause, of which there are THREE — a verdict actually FOUND, a read that could not be COMPLETED, and a read that completed but returned an IMPOSSIBLE answer (the third is not a variety of the second) — while the sentinel DESCRIPTION stays generic because the classification recognises it as a fixed point. `.creator` is TYPE-TESTED before it is indexed: `.creator != null and .creator.login` hard-errors on a non-object creator, jq exits 5, and under `set -e` that took the step down AFTER the exemption was posted and BEFORE the repair — a forged green reported as an infrastructure error.' -signals: 'stale review-verdict run overwrites a fresher one, retarget ABA against the docs-only classifier, concurrency group does not serialize pull_request_target, gitea auto-cancel push vs pull_request_target, forged exemption restored after reclassification, human BLOCKED silently turned green, post-write status verification, change_target_branch timeline count, why does my PR post no verdict status after a retarget · paths: `.gitea/workflows/review-verdict.yml`, `scripts/tests/test_pr_changed_files.py` · issues: #706, #698, #672, #663, #622' -mechanics: '`count_retargets()` pages `GET /repos/{repo}/issues/{pr}/timeline?limit=50&page=N` (cap 20) setting `rt_count`/`rt_ok`, trusted only on a validated empty page, which is a page of EITHER `null` (what this endpoint really returns past the end) or `[]` — an `array`-only type gate read the real terminator as unreadable and withheld every exemption (#751); `retargets_before`/`retargets_before_ok` captured before enumeration, re-counted immediately before the POST; `page_statuses()` pages `GET /repos/{repo}/statuses/{sha}?limit=50&page=N` (cap 20) setting `ph_rows`/`ph_ok`, trusted only on a validated EMPTY ARRAY page (this endpoint''s terminator, measured 2026-08-28 on PR #761''s 114-row head: pages 1-2 return 50, page 3 returns 14, page 4 is `[]`), retrying each page once with a 1s backoff; the server default `created_unix DESC` is KEPT, so a row inserted MID-WALK can be missed (it lands at position 0, on a page already read) — accepted and bounded, since a row arriving after this job''s POST is not one the job overwrote and wins on the combined endpoint anyway. `sort=highestindex` (index ASC, measured) closes that gap and was WITHDRAWN because ASC puts the OLDEST rows on page 1, which inverts the partial-mark fallback into a spurious-STICKY-repair engine — the #761 failure, re-introduced to close a smaller gap; `max_id_before` is the max over NUMERIC ids only in the paged list (jq orders strings above every number, so one `"id": "99999"` would otherwise pass the numeric gate and inflate the mark until nothing looked newer than it — a fail-open); this endpoint is a BARE ARRAY, unlike the combined `/commits/{sha}/status` object; repair POST is `pending`; tests `test_a_RETARGET_DURING_the_run_posts_NOTHING`, `test_a_PR_retargeted_BEFORE_the_run_but_QUIET_during_it_is_STILL_exempt`, `test_an_UNTRUSTED_retarget_count_withholds_the_EXEMPTION`, `test_an_UNTRUSTED_retarget_count_STILL_LETS_PENDING_THROUGH`, `test_a_human_verdict_landing_AFTER_the_POST_is_repaired_to_pending`, `test_a_PRE_EXISTING_human_row_does_NOT_trigger_a_repair`, `test_a_status_history_RUNNING_PAST_PAGE_1_is_PAGED_and_the_exemption_STANDS`, `test_a_raced_verdict_on_PAGE_2_is_detected_and_repaired`, `test_a_verdict_BEYOND_A_SHORT_PAGE_is_still_found`, `test_a_PRE_WRITE_paging_failure_still_yields_a_usable_high_water_mark`, `test_a_TRANSIENT_page_failure_is_retried_and_the_walk_still_completes`, `test_a_STRING_id_cannot_inflate_the_high_water_mark`, `test_a_MALFORMED_creator_row_does_not_kill_the_job_after_the_green_is_posted`, `test_a_STRING_id_on_a_PRE_EXISTING_row_is_not_read_as_raced`, `test_a_partial_mark_is_SAFE_because_the_newest_rows_are_on_page_1`, `test_an_EMPTY_post_write_history_is_not_evidence_that_nothing_raced`, `test_a_NULL_page_terminates_the_walk_as_an_empty_one`, `test_the_walk_does_not_request_a_SORT_order`' +rule: 'The `review-verdict/h10` job counts BOTH `change_target_branch` AND `pull_push` events on the PR''s issue timeline at run start and again immediately before its POST, and writes NOTHING if EITHER count moved. The COUNT is the key on both axes because the underlying VALUE is ABA-vulnerable — `main -> S -> main` reads `main` at both ends, which is how #698 route 1 obtained a forged exemption, and a force-push `H1 -> H2 -> H1` leaves `.head.sha` equal at both ends while the middle pages of `scripts/pr-changed-files.sh`''s enumeration came from `H2` (#803/#664) — while an event count is monotonic and cannot alias. ONE walk certifies BOTH counts, so an unreadable page abandons both; the two tallies are separate so the diagnostic names the axis that actually moved. Every push is counted and `is_force_push` is deliberately NOT read: an ordinary push also invalidates a mid-flight enumeration, and an `H1 -> H2 -> H1` restoration can have its second push non-forced when `H1` is an ancestor. The head fence does NOT abstain on its own triggering push — established FROM THE v1.27.1 SOURCE, since that would be a permanent stall rather than a fence: the push comment is created BEFORE the synchronize notification is emitted, so a run always sees its own causative event, and retries add no new event. The 26-102s margin measured across 20 triggered pairs on PRs #802/#834/#761 corroborates it and is a lower bound (the job runs a checkout first; 69s end to end on run 2385) — it is NOT the basis of the claim, which an earlier draft said it was. This NARROWS the residual, it does not resolve it — a retarget between the FINAL pre-write count and the POST still yields a PERMANENT forged green, because the successor run can consume the `edited` event and exit before the stale run posts last (corrected 2026-08-27, #849). Abstaining is a handoff, not a stall, and that is the property the design rests on — for a run that ABSTAINS; it says nothing about one that already passed its final count and then posts (see the residual, corrected #849): every retarget fires `edited`, which is in this workflow''s `types:`, so the event that makes a run abstain has already queued a successor whose window opens after it; the induction terminates when retargeting stops — but ONLY over runs that ABSTAIN. A run that already passed its final count is outside it: it writes whenever it gets there, so the run that writes LAST is not necessarily the one that classified last, and that is the permanent residual (#849). `updated_at` was REJECTED as the key because it also moves for comments and labels, which fire none of this workflow''s `types:` — a run could abstain with no successor coming, which is a real stall. The count is trusted only when paging reached a validated EMPTY page; an untrusted count (unreadable page, non-array body, non-numeric length, page cap hit) blocks the exemption `success` ONLY and still lets `pending` through, because `pending` blocks the merge immediately while withholding it would strand ordinary PRs whenever the timeline is unreadable — the right trade, but NOT a free one ("for no safety gain" retracted 2026-08-27): a GENERIC `pending` masks a rejection landing in its own write window, post-write verification does not run for it, and a later run re-derives it into an exemption `success` (#849). SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` IN FULL (PAGED since #763) and, if a human `Review-verdict:` row appeared with an id ABOVE a high-water mark taken just before the POST, overwrites its own status with `pending` and logs an error. The repair is `pending`, NEVER a copy of the human''s state, since re-posting their `failure` under the machine credential would attribute a human verdict to the job; its description is a SENTINEL that the classification refuses to grant an exemption over AND re-writes verbatim on every later run, so the block is a FIXED POINT rather than decaying — writing the generic `pending` description there instead erases the marker and the exemption simply returns one event later. The mark is captured BEFORE the last-moment re-read, not merely before the POST — a later mark leaves a multi-round-trip blind gap in which a verdict is neither seen by the re-read nor repaired afterwards. The id comparison is load-bearing: a mere presence test would fire forever on a base-mismatched verdict that `read_existing_verdict` deliberately declines to honour, deadlocking that PR''s exemption permanently. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run''s `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run''s mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. BOTH `/statuses/{sha}` reads PAGE to a validated EMPTY page since #763 — `[]` on this endpoint, a THIRD terminator shape distinct from the timeline''s bare `null` and the combined endpoint''s `{"statuses": null}`, so each terminator is MEASURED per endpoint — never terminating on a SHORT page, retrying each page once, under a 20-page cap that validates at most 950 rows (the 20th request must be the empty terminator); correctness does not depend on the measured cap of 50, because any page size pages correctly. WHAT THE PAGING BUYS IS NOT WHAT #763 CLAIMED: under the DESC default page 1 already held the true maximum id AND every row newer than the mark, so a single-page read missed a raced verdict only if more than 50 rows were created INSIDE the write window — not merely on a head over 50 rows. What removed #761''s stall is retiring the probe, not the walk; the walk''s value is that the gate''s one fail-toward-SUCCESS path no longer depends on an UNDOCUMENTED ordering the server honours only coarsely (page 1 came back `114,112,113,111,110`). This RETIRED #751''s conservative page-2 probe, which treated "there are rows I did not read" as "assume raced" and so repaired every head that outgrew one page: it fired on Renovate PR #761, reporting a human verdict as overwritten when none existed and then, the sentinel being sticky, refusing to re-exempt that head on every later run. The two directions are NOT symmetric. POST-WRITE, uncertainty fails CLOSED — an unreadable history, an over-cap history, or a count that is not a number all repair to `pending`; previously an unreadable history warned and left the exemption green while the page-2 probe repaired on the same uncertainty, one check disagreeing with itself. PRE-WRITE, a PARTIAL list still yields a mark, because the mark gates the post-write check entirely and refusing one SKIPS that check, which is itself the fail-open; this rests on the DESC DEFAULT — the newest row, carrying the maximum id, is on page 1, so a walk that fails later still saw it — while a VALIDATED empty history is NOT abandoned (it yields a mark of 0, correct for a first run, since every later row is newer) — what abandons the mark is a read that both FAILED and returned nothing (the pre-existing #849 gap, unchanged) — and a NON-EMPTY history carrying no numeric id is reported unusable rather than collapsed to 0. BOTH id comparisons are NUMERIC-ONLY: jq orders strings above every number, so one `"id": "99999"` inflates the mark until nothing looks newer, and `.id > $since` reads any string id as newer than any mark — making a PRE-EXISTING base-mismatched verdict look raced on every run, a permanent per-sha stall (the twin was live on `main`). An EMPTY post-write history is REJECTED: the walk terminates on an empty page, which is correct before the write and impossible after it (one row per POST), and a well-formed "no statuses exist" is not retried — so accepting it would conclude `raced=0` from a list that cannot be real, silently. That is NOT the withdrawn currency witness, which asked whether ANY row sat above the mark and was satisfied by an unrelated newer row; this asks only whether the list is EMPTY, a state no unrelated row can produce. The `::error::` now names its own cause, of which there are THREE — a verdict actually FOUND, a read that could not be COMPLETED, and a read that completed but returned an IMPOSSIBLE answer (the third is not a variety of the second) — while the sentinel DESCRIPTION stays generic because the classification recognises it as a fixed point. `.creator` is TYPE-TESTED before it is indexed: `.creator != null and .creator.login` hard-errors on a non-object creator, jq exits 5, and under `set -e` that took the step down AFTER the exemption was posted and BEFORE the repair — a forged green reported as an infrastructure error.' +signals: 'stale review-verdict run overwrites a fresher one, retarget ABA against the docs-only classifier, head ABA force-push H1 H2 H1 during paging, pull_push timeline event count, force-push during changed-file enumeration, docs-only exemption from a file list spanning two heads, concurrency group does not serialize pull_request_target, gitea auto-cancel push vs pull_request_target, forged exemption restored after reclassification, human BLOCKED silently turned green, post-write status verification, change_target_branch timeline count, why does my PR post no verdict status after a retarget · paths: `.gitea/workflows/review-verdict.yml`, `scripts/tests/test_pr_changed_files.py` · issues: #706, #698, #672, #663, #622, #803, #664' +mechanics: '`count_pr_mutations()` (named `count_retargets()` until #803 put the head axis on the same walk) pages `GET /repos/{repo}/issues/{pr}/timeline?limit=50&page=N` (cap 20) setting `rt_count` (`change_target_branch`), `hp_count` (`pull_push`) and the SHARED trust flag `rt_ok`, trusted only on a validated empty page, which is a page of EITHER `null` (what this endpoint really returns past the end) or `[]` — an `array`-only type gate read the real terminator as unreadable and withheld every exemption (#751); `retargets_before`/`pushes_before`/`retargets_before_ok` captured before enumeration, re-counted immediately before the POST, with a SEPARATE fence arm and diagnostic per axis (the base arm is evaluated first); the run log line is `Mutation fence: N retarget event(s) and M push event(s) ... (trusted=...)`, renamed from `Retarget fence: ...` by #803; `page_statuses()` pages `GET /repos/{repo}/statuses/{sha}?limit=50&page=N` (cap 20) setting `ph_rows`/`ph_ok`, trusted only on a validated EMPTY ARRAY page (this endpoint''s terminator, measured 2026-08-28 on PR #761''s 114-row head: pages 1-2 return 50, page 3 returns 14, page 4 is `[]`), retrying each page once with a 1s backoff; the server default `created_unix DESC` is KEPT, so a row inserted MID-WALK can be missed (it lands at position 0, on a page already read) — accepted and bounded, since a row arriving after this job''s POST is not one the job overwrote and wins on the combined endpoint anyway. `sort=highestindex` (index ASC, measured) closes that gap and was WITHDRAWN because ASC puts the OLDEST rows on page 1, which inverts the partial-mark fallback into a spurious-STICKY-repair engine — the #761 failure, re-introduced to close a smaller gap; `max_id_before` is the max over NUMERIC ids only in the paged list (jq orders strings above every number, so one `"id": "99999"` would otherwise pass the numeric gate and inflate the mark until nothing looked newer than it — a fail-open); this endpoint is a BARE ARRAY, unlike the combined `/commits/{sha}/status` object; repair POST is `pending`; tests `test_a_HEAD_ABA_DURING_the_run_posts_NOTHING`, `test_the_head_fence_holds_under_EITHER_terminator_shape`, `test_the_head_arm_DEFERS_to_the_trust_guard_when_the_second_count_fails`, `test_a_push_landing_BEYOND_page_1_is_still_counted`, `test_positive_control_a_MULTI_PAGE_quiet_timeline_still_exempts`, `test_an_EMPTY_FIRST_page_is_untrusted_WHICHEVER_empty_shape_it_is`, `test_a_timeline_row_with_NO_READABLE_TYPE_is_not_counted_as_nothing`, `test_a_SINGLE_head_push_during_the_run_also_posts_NOTHING`, `test_a_PR_PUSHED_BEFORE_the_run_but_QUIET_during_it_is_STILL_exempt`, `test_the_head_and_base_axes_are_counted_SEPARATELY`, `test_the_head_fence_reports_the_OBSERVED_counts_in_its_notice`, `test_a_PROTECTED_pr_pushed_mid_run_ALSO_posts_nothing`, `test_a_RETARGET_DURING_the_run_posts_NOTHING`, `test_a_PR_retargeted_BEFORE_the_run_but_QUIET_during_it_is_STILL_exempt`, `test_an_UNTRUSTED_retarget_count_withholds_the_EXEMPTION`, `test_an_UNTRUSTED_retarget_count_STILL_LETS_PENDING_THROUGH`, `test_a_human_verdict_landing_AFTER_the_POST_is_repaired_to_pending`, `test_a_PRE_EXISTING_human_row_does_NOT_trigger_a_repair`, `test_a_status_history_RUNNING_PAST_PAGE_1_is_PAGED_and_the_exemption_STANDS`, `test_a_raced_verdict_on_PAGE_2_is_detected_and_repaired`, `test_a_verdict_BEYOND_A_SHORT_PAGE_is_still_found`, `test_a_PRE_WRITE_paging_failure_still_yields_a_usable_high_water_mark`, `test_a_TRANSIENT_page_failure_is_retried_and_the_walk_still_completes`, `test_a_STRING_id_cannot_inflate_the_high_water_mark`, `test_a_MALFORMED_creator_row_does_not_kill_the_job_after_the_green_is_posted`, `test_a_STRING_id_on_a_PRE_EXISTING_row_is_not_read_as_raced`, `test_a_partial_mark_is_SAFE_because_the_newest_rows_are_on_page_1`, `test_an_EMPTY_post_write_history_is_not_evidence_that_nothing_raced`, `test_a_NULL_page_terminates_the_walk_as_an_empty_one`, `test_the_walk_does_not_request_a_SORT_order`' --- `ci.exemption-provenance` closed three routes into the exemption path and left one residual it named: @@ -120,6 +120,137 @@ is parameterised over both shapes and asserts the POSTED STATUS rather than the probe run the log said `Decision: state=success` and the job still posted nothing, so the decision and the write are separate events and only the write is what a merge reads. +## The head axis (2026-08-28, ersatztv#803 / ersatztv#664) + +The fence above closes `main -> S -> main`. The HEAD aliases identically and was fenced by nothing: +a force-push `H1 -> H2 -> H1` spanning `scripts/pr-changed-files.sh`'s paging leaves that script's +final `.head.sha` comparison equal while the middle pages were enumerated against `H2`, so a mixed +file list can yield a docs-only exemption no single head ever justified. Two issues found it +independently — #664 from the #649 cold review, #803 from #778's read-then-write inventory — which +is itself the argument for fixing it rather than documenting it a third time. + +**The enumerator cannot close it, and that is structural rather than an omission.** Every binding it +performs compares a value against itself, which is the definition of ABA-vulnerable. Pinning the diff +to two shas would close both axes at once and the API will not serve it: re-probed at Gitea **1.27.1 +on 2026-08-28**, `GET /compare/{base}...{head}` returns `total_commits` and `commits` and no `files` +key, over a 12-commit range. (That confirms #747's re-dating of the same claim from 1.25.4 — worth +recording as an independent probe, because a "cannot be done" carrying a stale version stamp is +exactly the claim a reader stops checking.) So the fix is a caller-side monotonic counter, the same +answer as the base. + +**`pull_push` is that counter.** The timeline records every push to the PR branch as one `pull_push` +event whose body is `{"is_force_push": bool, "commit_ids": [...]}`. Measured at 1.27.1: PR #802 +carries eighteen, all `false`; PR #761 carries five, four `true`. An `H1 -> H2 -> H1` round trip is +necessarily TWO pushes, so the count moves by two where the sha moves by zero. + +**The shape is not hypothetical.** PR #761 really went `8798a1d -> 830a407 -> 8798a1d`. The two +pushes were 26 minutes apart, so no enumeration straddled them and nothing was forged — but the head +returned to its starting value, which is precisely what a sha comparison cannot see, whatever the +interval. Ordinary force-push-and-revert produces the alias; an attacker is not required. + +**`is_force_push` is deliberately not read.** Filtering to force-pushes is the tempting narrowing and +is wrong twice: an ordinary push also invalidates a mid-flight enumeration, and the restoring push of +an ABA can itself be non-forced when `H1` is an ancestor. The body is never parsed at all, so a +change to its shape cannot break the count. + +**Why this does not abstain on its own trigger — established from the SOURCE, corroborated by +measurement.** A `synchronize` run is caused by a push. If that push's timeline event were created +*after* the run's before-count, every such run would abstain forever — the #751 shape, where a fence +shipped and silently withheld every exemption. It is not: in Gitea v1.27.1 the push comment is +created BEFORE the synchronize notification is emitted, so a run always sees its own causative event, +and retries and re-runs add no new event. That is an ordering property of the code, which is what the +claim needed — an earlier draft of this record rested it on the measurement alone and called it a +deployment property, which was weaker than the truth. + +**One window the `synchronize` argument does not cover, named because it is a DIFFERENT route than +the one above tells a reader to watch for.** A PR's FIRST push is recorded by the async push task +about a second after the PR is created (PR #866: created 16:55:32, row 16:55:33; PR #868: 18:43:10 / +18:43:11) and fires no `synchronize` — PR #868 got exactly one `pull_request_target` run. So an +`opened` run whose before-count landed inside that ~1s window would abstain with no successor queued, +which IS the #751 shape. Unreachable against a queue latency of 26s and up, and not measured by the +20 push->run pairs below, which are all `synchronize`. Recorded rather than fixed. + +The measurement corroborates and is kept for the margin it gives: across 20 triggered +(push -> `pull_request_target` run) pairs on PRs #802, #834 and #761 the `pull_push` event predates +its own run's `started_at` by 26-102s. `started_at` is only a LOWER bound, because the job then runs +a checkout and several steps before counting — timed once end to end on run 2385 (PR #868): push at +16:51:39Z, fence count at 16:52:47.7Z, **69s**, of which 16.7s was inside the job. + +**Two tallies, one walk, one trust flag.** The counts come off the same pages, so the head axis costs +no extra round trip and cannot be certified by a different set of pages than the base axis; a page +whose push tally is unreadable abandons the retarget tally too. They are separate *tallies* so the +notice names the axis that moved — a fence that reports only "something moved" cannot be triaged from +a run log, which is how #751 stayed invisible. + +**Two invariants were untestable BY CONSTRUCTION until the doubles were changed**, which is a +different failure from a missing test: the assertion existed, the mutation survived it anyway. Every +synthetic `pull_push` row carried `is_force_push: true`, so narrowing the tally to force-pushes only +— the exact narrowing three paragraphs of comment call wrong — passed the whole fence suite; the flag +now alternates. And every fence test terminated on the default `null`, so deleting the head count +from the empty-ARRAY arm left the head fence silently dead for `[]`-terminated timelines with the +suite green; the ABA test is now parameterised over both shapes. + +**What the tests had to be built to catch**, because the plausible wrong implementations all pass a +naive suite. Keying on the count's VALUE rather than its MOVEMENT would withhold the exemption from +every PR in existence — *every* PR has a non-zero `pull_push` count, since it is created by a push — +so `test_a_PR_PUSHED_BEFORE_the_run_but_QUIET_during_it_is_STILL_exempt` holds a settled count of 18. +Folding both event types into one tally passes every single-axis test, because with one axis moving a +shared counter still moves; only holding one axis stable AND non-zero while the other moves separates +them. Both directions are posed, and only one is reachable: the arms are evaluated base-first, so +pushes leaking into the retarget tally are caught (the base arm pre-empts the head message) while +retargets leaking into the push tally are not observable from here, and are not claimed to be. An +earlier draft asserted the base-moves row alone while claiming to catch the folding mutation, and +stayed GREEN under exactly that mutation — the fixture must pose the scenario it narrates. + +**The contracts that asserted more than the code did are corrected in the same change** (#803's +floor, which was to land regardless of whether the fence did). The issue named three: +`scripts/pr-changed-files.sh`'s `exit 0` line said "bound to ``" and now says what +it can support — complete, with no movement OBSERVABLE from inside the enumeration; its head re-read +said "Bind the enumeration to ONE head" and now says ONE-WAY ONLY; and `review-verdict.yml`'s call +site said `exit 0` means "complete and bound to $SHA", the reading that matters most because that +caller posts a green required status with nobody in the loop. + +**A count is the wrong shape for this and the sweep found four more.** Grepping the retracted WORDING +missed every paraphrase of it: "at one head" in the script's SCOPE paragraph and again in this +record's sibling `ci.shared-pr-file-enumeration`, "bound to ONE head" in the advisory hook — the +other caller of the same script, in a file this change already edits — and "bound to BOTH the given +sha" in that record's machine-readable `mechanics:` line, which left the frontmatter contradicting +its own body two screens below. Sweep such a retraction by CONCEPT, enumerating the claims, not by +the sentence that happened to be quoted in the issue. + +**Two fail-opens in the shared walk were found by the same review and fixed here**, both pre-existing +and both affecting the base axis equally. An empty **ARRAY** first page was trusted on any page while +the `null` arm required `page > 1` — so `[]` on page 1 certified "nothing retargeted, nothing pushed" +over a response the walk cannot explain, and the comment three lines above already asserted the safe +behaviour, making the code contradict its own stated rule for one of the two empty shapes this server +produces. And no row was validated before `.type` was selected on, so a page of rows carrying no +readable `.type` certified a zero count; `pr-changed-files.sh` has applied validate-what-you-consume +to every row it extracts since #643, and the walk whose counts gate the write did not. + +**Not closed by this.** Two residuals, and the second is the one to read carefully. + +1. The residual named in "What is NOT closed" item 1 applies unchanged to the head axis: a push + landing between the final pre-write count and the POST is not seen, and by the #849 argument that + leaves a permanent rather than transient forged green. + +2. **The walk's terminator is defeatable, so neither axis is closed outright** — pre-existing, found + by cross-family review of #803 from the v1.27.1 source, and tracked as ersatztv#870 because fixing + it is a redesign of a walk both fences share. `ListIssueCommentsAndTimeline` pages at the DATABASE + level (`FindComments` applies the LIMIT/OFFSET) and filters AFTERWARDS, dropping every + `CommentTypeCode` row and every inaccessible cross-reference into a `var apiComments + []*api.TimelineComment` — a NIL slice, which serializes as bare `null`. So a page whose rows are + all inline-code comments returns the exact value this walk trusts as exhaustion, while later pages + still hold events; and rows are ordered ASCENDING, so the events a fence cares about are the ones + furthest from page 1. Fifty inline review comments — which a PR author can create on their own + PR — truncate both walks at the same place, both counts then agree, the sha comparison agrees, and + the ABA is unseen. + + What the fence closes is therefore the ABA on a PR whose timeline carries no such truncating + block, which is every PR this repo has but is not a property anyone enforces. Said here rather + than left in the issue because the whole of #803 is that a contract asserting more than its code + does is worse than no contract — and a fence advertised as closing the head ABA would have been + the fourth such contract, added by the change that removed three. + ## The status history is paged too (2026-08-28, ersatztv#763) The timeline walk above was one half of a single rule; `/statuses/{sha}` was still read as one @@ -139,7 +270,7 @@ page 3 returns 14, page 4 is the two bytes `[]`. The terminator here is an empty distinct empty shape on one instance, after the timeline's bare `null` and the combined endpoint's `{"statuses": null}`. A short page is **not** the end. An empty **first** page is legitimate rather than anomalous **for the pre-write caller** (a head nothing has posted to yet returns `[]`, and so does a -bogus sha), which is why this walk cannot simply copy `count_retargets`, whose "one real page required" +bogus sha), which is why this walk cannot simply copy `count_retargets` (renamed `count_pr_mutations` by #803), whose "one real page required" rule would withhold the mark on every first run. It is *impossible* for the post-write caller, which has just written a row — so that caller rejects an empty result itself rather than the walk refusing it for everyone. diff --git a/docs/decisions/records/release/verdict-status-check.md b/docs/decisions/records/release/verdict-status-check.md index 5b59d55bf..7881ea2d1 100644 --- a/docs/decisions/records/release/verdict-status-check.md +++ b/docs/decisions/records/release/verdict-status-check.md @@ -63,6 +63,25 @@ follow that no hook-side fix could deliver: - it cannot be satisfied by a workflow merely running successfully, since the required context is the status the workflow *posts*, never the workflow's own job status. +**Read those three as scoped to the head that has NO status, which is what "by construction" buys and +all it buys** (qualified 2026-08-28, ersatztv#803, under `process.check-and-use-pins-a-version`). The +sha binding makes it impossible for a *new* commit to inherit an *old* verdict. It says nothing about +how a head ACQUIRES a status, and one of the two ways is machine-written: the exempt-class auto-pass +in `review-verdict.yml`. That path derives "docs-only" from a paged file enumeration, and a paged +enumeration is a read-then-write over remote state with a real window — up to forty round trips, over +which the PR's base or head can move and move back. So the honest form of the invariant is: + +> a head cannot inherit a verdict written for another commit; a head CAN be given an exemption +> `success` computed over a window in which it was not the only head. + +That window is fenced, not eliminated (`ci.verdict-write-retarget-fence`): the job refuses to write if +the timeline's `change_target_branch` or `pull_push` count moved while it classified, which is what +catches the `main -> S -> main` and `H1 -> H2 -> H1` aliases that every value comparison reads as +"unchanged". Two residuals survive it, both recorded there rather than restated here — a mutation +landing between the final pre-write count and the POST (#849), and the walk's own terminator, which +Gitea's page-then-filter ordering makes forgeable (#870). Neither is reachable through the sha binding this section is about; both are reachable +through the exemption write, which is why the scope sentence matters more than the residuals do. + The comment convention of `release.review-verdict-gate` is unchanged and still load-bearing: it is what a human reads, and it remains the hook's condition (c). `post-review-verdict.sh` writes both from one command so they cannot drift, and re-reads the head after posting the comment — if a commit landed in diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index a8654f167..8bfcafa56 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -234,6 +234,7 @@ recorded as unexamined rather than as cleared. | `scripts/tests/test_mcp_smoke.py` | the `script-tests` job, when the MCP smoke checker accepts a server it should refuse — an impostor identity, a reply carrying only an id, a missing expected tool, or an `initialize` pre-answered before the request (the `tools/list` twin is held by two mechanisms jointly and is covered behaviourally, not by the declared clause) | GUARD | MUTATION | `test_mcp_smoke.py::test_MUTATION_a_PRE_ANSWERED_id_is_refused_because_the_request_ids_are_UNGUESSABLE` | | `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_head_change.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_mutation_harness.py` | the `script-tests` job, when a `MUTATION` row's declared clause no longer reddens the test the row names | GUARD | MUTATION | `test_mutation_harness.py::test_MUTATION_disarming_the_DIAGNOSTIC_gate_accepts_a_red_for_the_wrong_reason` | @@ -355,7 +356,7 @@ clause-provable and the entry is regraded. ## What the numbers say -44 guards, 6 tooling scripts, 21 proof files. **22 guards carry a mutation proof; 6 are +44 guards, 6 tooling scripts, 22 proof files. **22 guards carry a mutation proof; 6 are behaviour-only; 16 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, diff --git a/docs/remote-state-inventory.md b/docs/remote-state-inventory.md index 61b23db11..b37d29220 100644 --- a/docs/remote-state-inventory.md +++ b/docs/remote-state-inventory.md @@ -65,7 +65,7 @@ classifications differ; otherwise the strictest applies and the Note names the e | 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` — 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. It WAS stale in two distinct ways; the first is closed. FIRST, within the run: every later check (CI status, H10 status, verdict comments) was evaluated against the captured sha, so a push landing mid-run — including across the docs-only enumeration's up-to-forty round trips — was checked against the commit it replaced. **Closed 2026-08-28 (#803)**, symmetrically with the base and at the same hoist: `.head.sha` is re-read from `prjson_now`, the response the base re-read already fetches, so the two axes cost one round trip between them and cannot describe two different instants; a moved head DENIES, matching the `stale` verdict class, while an unreadable one asks. 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. The one that remains is bounded the same way it always was — a head the verdict does not cover cannot inherit the sha-bound required status, so the server refuses it — and is not bounded by anything in this hook. Note the re-read narrows the window rather than erasing it: a push landing after the hoist still passes, and the hook deliberately does not re-read a second time (two reads move the window rather than closing it, the same rule the branch-protection block states). 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. | @@ -97,7 +97,7 @@ classifications differ; otherwise the strictest applies and the Note names the e | 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 closed on the write ORDER since #792: the status is written first and the comment second, so a refusal can no longer leave a verdict comment with no status behind it — the only reachable half-state is a status with no comment, which the merge hook resolves as `ask` (`release.verdict-writes-status-before-comment`). | -| `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/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 — a force-push `H1 -> H2 -> H1` during pagination — leaves the final `.head.sha` comparison equal while the middle pages were enumerated against `H2`, and NOTHING inside this script can see it. **Fenced at the enforced caller since #803** (2026-08-28): the same timeline walk now also counts `pull_push`, so a head that moved at all during classification withholds the write — subject to that walk's own terminator, which is defeatable by a page of filtered rows (**#870**, pre-existing and equally affecting the base axis). The grade stays `UNSAFE-KNOWN` for the SCRIPT, because the script's own guarantee is unchanged and one-way — a caller that does not run the fence inherits the alias. Enumeration bound to an immutable tree remains unavailable: re-probed at 1.27.1 on 2026-08-28, `compare/{base}...{head}` still returns no `files` key. | | `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. | @@ -144,9 +144,9 @@ classifications differ; otherwise the strictest applies and the Note names the e | 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` — 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 rather than claimed closed: a monotonic **event-count** fence refuses to write if the PR timeline's `change_target_branch` OR `pull_push` count moved (`ci.verdict-write-retarget-fence`; counts are used because the branch *name* and the head *sha* are both ABA-vulnerable — the head axis added by #803), 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/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 event-count fence (`ci.verdict-write-retarget-fence`), and since #803 that fence covers BOTH axes: `change_target_branch` for the base alias and `pull_push` for the HEAD alias described in the enumerator's row (`H1 -> H2 -> H1` during pagination). This row said the head alias was unfenced here, which was true until 2026-08-28. What remains is the residual the fence shares with the base axis — a mutation landing between the final pre-write count and the POST (#849) — not an unwatched axis. | | `.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:` 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 `:` — 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. | diff --git a/scripts/pr-changed-files.sh b/scripts/pr-changed-files.sh index 4a89a4ef0..e65d4a926 100755 --- a/scripts/pr-changed-files.sh +++ b/scripts/pr-changed-files.sh @@ -10,7 +10,9 @@ # drift; one cannot. # # SCOPE — mechanism, not policy. This script answers exactly one question: "what is the complete set of -# paths this PR touches, at one head, or can we not tell?" It deliberately does NOT classify the PR. +# paths this PR touches, with no head or base movement observable from in here, or can we not tell?" +# ("at one head" until 2026-08-28 — the same overclaim as the CONTRACT below, in the paraphrase that +# survived the first sweep for it.) It deliberately does NOT classify the PR. # The two callers' allow-lists differ ON PURPOSE and must stay separate: # * the hook's docs-only pattern also lets .claude/ .gitea/ .husky/ through, which is safe there only # because it falls through to a HUMAN PROMPT; @@ -20,8 +22,23 @@ # CONTRACT # Usage: pr-changed-files.sh # stdout: newline-delimited paths, BOTH sides of every rename, no blank lines. May be empty. -# exit 0 the enumeration is COMPLETE and bound to AND . -# stdout is authoritative. +# exit 0 the enumeration is COMPLETE, and both and were +# observed unchanged at BOTH ends of it. stdout is authoritative. +# +# READ THAT AS THE ONE-WAY GUARANTEE IT IS (ersatztv#803/#664). This line said "bound to +# " until 2026-08-28, which claims more than the code can do. Every +# binding below compares a value against ITSELF, so it detects movement that is still in +# effect at the end and is blind to an ALIAS: `H1 -> H2 -> H1` across the paging round +# trips, or `main -> S -> main`, restores the expected value while the middle pages were +# enumerated against the other one. Exit 0 means "no movement was OBSERVABLE from here", +# not "this list belongs to one head". +# +# A caller needing the stronger property must fence it with a MONOTONIC key, because a +# count cannot alias where a value can. The ENFORCED caller +# (`.gitea/workflows/review-verdict.yml`) does exactly that on both axes — it counts +# `change_target_branch` and `pull_push` timeline events before and after, and writes +# nothing if either moved (`ci.verdict-write-retarget-fence`). The advisory hook does not, +# and does not need to: its failure mode is a human prompt, not a green status. # exit 1 the enumeration could NOT be completed or verified. stdout is meaningless — the caller # MUST fail closed (withhold any exemption). A diagnostic goes to stderr. # exit 2 usage error. @@ -45,8 +62,14 @@ # {head}` returns `total_commits`/`commits` and NO `files` (measured on 1.25.4, re-confirmed on # 1.27.1 2026-08-28 ersatztv#747). Separately, and NOT re-probed since 1.25.4: a `--depth=1` fetch of # the two shas has no merge base, so a three-dot diff is impossible while a two-dot one over-reports -# every commit `main` gained since the branch point (measured, #698). The remainder is covered one level up -# instead, by the workflow reclassifying on `edited` rather than trusting a machine-written success. +# every commit `main` gained since the branch point (measured, #698). Independently re-probed again on +# 1.27.1, 2026-08-28 (ersatztv#803): still `total_commits`/`commits` only, over a 12-commit range. +# +# The remainder is covered one level up instead, and since ersatztv#803 that cover is explicit on BOTH +# axes rather than the base alone: the enforced caller fences its WRITE on the monotonic count of +# `change_target_branch` AND `pull_push` timeline events (`ci.verdict-write-retarget-fence`) — which +# is the thing an ALIAS cannot defeat — in addition to reclassifying on `edited` rather than trusting +# a machine-written success. # # AUTH/TRANSPORT is caller-supplied via env, because the two callers authenticate differently: # ETV_GITEA_TOKEN | GITEA_TOKEN -> `Authorization: token` @@ -240,10 +263,16 @@ if [ "$complete" != yes ]; then exit 1 fi -# Bind the enumeration to ONE head. Paging is several round-trips; a force-push between them means -# page 1 came from head A and page 2 from head B, so the assembled list belongs to no single commit — -# B's code page can be skipped entirely while B's docs page reads as a clean short tail. Re-read the -# head and refuse if it moved. +# Re-read the head and refuse if it MOVED. Paging is several round-trips; a force-push between them +# means page 1 came from head A and page 2 from head B, so the assembled list belongs to no single +# commit — B's code page can be skipped entirely while B's docs page reads as a clean short tail. +# +# ONE-WAY ONLY. This said "Bind the enumeration to ONE head" until 2026-08-28, which is the overclaim +# ersatztv#664 was filed against and ersatztv#803 carried: `H1 -> H2 -> H1` across the paging window +# passes the comparison below, because the value re-read is the value expected, while pages 1 and 2 +# came from different trees. Nothing checkable HERE closes that — the check would have to compare +# against something the API does not offer (see the `compare/` probe above) — so it lives at the +# caller as a monotonic event count instead (`ci.verdict-write-retarget-fence`). prjson=$(gq "repos/$owner/$repo/pulls/$pr") if [ -z "${prjson//[[:space:]]/}" ]; then echo "pr-changed-files: could not re-read PR head to bind the enumeration — failing closed" >&2 @@ -268,7 +297,7 @@ fi # Same window, the tip-advance question this time (ersatztv#707; see the comment at # `base_sha_before` above for why this is a DIFFERENT check from `.base.ref`, not a duplicate of -# it). `prjson` is already fetched above to bind the head sha, so this is the same re-read, not a +# it). `prjson` is already fetched above to re-read the head sha, so this is the same read, not a # new round trip. `$expected_base`'s branch name can be unchanged across the whole enumeration # while its TIP moved partway through — the exact #707 window: no retarget, no head movement, # nothing the ref check or the head-sha check can see, yet later pages were diffed against a base diff --git a/scripts/tests/test_merge_consent_head_change.py b/scripts/tests/test_merge_consent_head_change.py new file mode 100644 index 000000000..59e124375 --- /dev/null +++ b/scripts/tests/test_merge_consent_head_change.py @@ -0,0 +1,194 @@ +"""Tests for the head re-read in `.claude/hooks/pretooluse-merge-consent.sh` (ersatztv#803). + +`$sha` is captured once from the PR snapshot at the top of the hook, and every later check — the CI +combined status, the `review-verdict/h10` status, and the verdict-comment classification — is +addressed by it. Between the capture and those checks sits the docs-only enumeration, up to forty +round trips. A push landing in that gap was therefore checked against the commit it had just +replaced, and the hook would report "a positive Review-verdict references the current head" about a +head that was no longer current. + +The fix is deliberately the SAME one the base got in #778, at the SAME hoist and off the SAME +response, so the two axes cannot describe two different instants. These tests pin that it fires, that +it names both shas, that it does not fire on a quiet run, and that an unreadable head asks rather than +denying. + +WHAT IS NOT CLAIMED HERE. The hook cannot detect an ABA — a push away and back leaves `.head.sha` +equal at both of its reads, exactly as it does inside `scripts/pr-changed-files.sh`. That case belongs +to the monotonic `pull_push` count in `.gitea/workflows/review-verdict.yml` +(`ci.verdict-write-retarget-fence`), and is covered in `test_pr_changed_files.py`. No test here +implies the hook closes it. + +Observable contract: the hook exits 0 with EMPTY stdout when it has no opinion (passthrough to normal +permissioning), 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" +MOVED = "b7c4d19ffe2210873bb9a0417c6e3f2280551ac4" + +# NOT docs-only, on purpose: the docs-only exemption short-circuits the gate long before the hoist, +# so a docs PR would never reach the head check and every test here would pass without exercising it. +# +# THE MOVE IS SERVED FROM THE SECOND `/pulls/{n}` READ ONWARD, not at a hardcoded read index. The +# hook's own two reads are not adjacent — `scripts/pr-changed-files.sh` makes its own in between, in a +# separate process sharing this shim — so counting to a specific read would pin an implementation +# detail that any refactor of the enumerator would silently break. "Moved once, early, and stayed +# moved" is both the realistic shape and the one that needs no such knowledge. +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] + +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: + print(json.dumps({"state": "success", "statuses": [ + {"context": "review-verdict/h10", "status": "success", + "description": "Review-verdict: MERGEABLE @ a9e3e23 (base: main)"}]})) + sys.exit(0) + +if "/pulls/" in url: + ctr = state / "pr_reads" + seen = int(ctr.read_text()) if ctr.exists() else 0 + ctr.write_text(str(seen + 1)) + mode = (state / "head_mode").read_text().strip() + sha = os.environ["STUB_SHA"] + if seen >= 1: + if mode == "moved": + sha = os.environ["STUB_MOVED"] + elif mode == "unreadable": + sha = "" + body = {"base": {"ref": "main"}, "body": "fixes #1"} + if sha: + body["head"] = {"sha": sha} + print(json.dumps(body)) + 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 / "head_mode").write_text("stable") + + env = dict(os.environ) + env["PATH"] = f"{bindir}{os.pathsep}{env['PATH']}" + env["STUB_DIR"] = str(state) + env["STUB_SHA"] = SHA + env["STUB_MOVED"] = MOVED + env["ETV_GITEA_TOKEN"] = "stub" # noqa: S105 - deliberately fake; the real credential comes from the environment + env["ETV_GITEA_URL"] = "http://gitea.example" + env.pop("ETV_GITEA_BASICAUTH", None) + + class Handle: + def set_head_mode(self, mode): + """'stable' | 'moved' | 'unreadable' — applied from the SECOND /pulls read onward.""" + (state / "head_mode").write_text(mode) + + def decision(self, mwcs=False): + payload = {"tool_input": {"method": "merge", "owner": "timothy", "repo": "ersatztv", "pull_number": 42}} + if mwcs: + payload["tool_input"]["merge_when_checks_succeed"] = True + r = subprocess.run(["bash", str(HOOK)], input=json.dumps(payload), env=env, capture_output=True, text=True) + assert r.returncode == 0, r.stderr + return json.loads(r.stdout) if r.stdout.strip() else None + + def reason(self, mwcs=False): + d = self.decision(mwcs=mwcs) + return "" if d is None else json.dumps(d) + + return Handle() + + +def test_a_head_that_MOVED_mid_run_denies(hook): + hook.set_head_mode("moved") + reason = hook.reason() + assert "deny" in reason, "the hook kept its opinion about a head that was replaced while it was evaluating" + assert SHA[:7] in reason and MOVED[:7] in reason, ( + "the deny must name BOTH shas; 'the head moved' is not something a reader can act on" + ) + assert "803" in reason, "the deny should cite the issue that explains the window" + + +def test_positive_control_a_QUIET_head_does_not_trigger_the_head_deny(hook): + """Without this, the test above passes against a hook that denies on every path. + + It very nearly would: this PR is deliberately non-docs, so the gate runs to completion, and the + assertion is not "no decision" but "not a decision caused by the HEAD arm". + """ + hook.set_head_mode("stable") + reason = hook.reason() + assert "head moved from" not in reason, f"the head arm fired on a run whose head never moved: {reason}" + # Positively witness that the run got PAST the head arm rather than never reaching it — an + # absence assertion alone is satisfied by a hook that exited before the check ever ran. This + # fixture stops at the issue-body read, which is downstream of the hoist. + assert "could not fetch linked issue" in reason, ( + "the run did not reach the checks downstream of the head re-read, so this control does not " + f"show the head arm was exercised and stayed silent: {reason}" + ) + + +def test_an_UNREADABLE_head_on_re_read_ASKS_rather_than_denying(hook): + """The two cases are not the same and must not collapse into one decision. + + A head that MOVED is a state we positively established — the verdict covers an older commit, + which is what the `stale` verdict class denies for. A head we could not READ is the absence of + evidence, and this gate's standing rule is that uncertainty asks. Collapsing them would block + merges on a transport blip; collapsing them the other way would grant on one. + """ + hook.set_head_mode("unreadable") + reason = hook.reason() + assert "deny" not in reason, f"an unreadable head was treated as a moved one: {reason}" + # ASSERT WHICH ask, not merely that SOME ask happened. This fixture's issue endpoint returns + # `{}`, so the hook asks later anyway for a completely unrelated reason — meaning a mutation + # deleting this arm outright (empty `sha_now` -> fall through and keep using `$sha`) left the + # test green. Naming the arm's own message is what makes the test measure the arm. + assert "reports no head commit (.head.sha) on re-read" in reason, ( + f"the hook asked, but NOT via the unreadable-head arm: {reason}" + ) + + +@pytest.mark.parametrize("mwcs", [False, True], ids=["immediate", "scheduled"]) +def test_the_head_deny_covers_BOTH_merge_paths(hook, mwcs): + """The twin-miss shape, tested because this exact hook has already been bitten by it. + + The base re-read first landed INSIDE the scheduled-auto-merge branch only, and cold review + found the consequence with this repo's own fixture: scheduled+retarget denied while + immediate+retarget auto-GRANTED. The head check is placed at the same hoist precisely so it + sits ABOVE the point where the two paths diverge — but "it is above the split" is a claim about + the source, and the thing worth pinning is the OUTCOME on both paths. + """ + hook.set_head_mode("moved") + reason = hook.reason(mwcs=mwcs) + assert "deny" in reason, ( + f"a head that moved mid-run was not denied on the {'scheduled' if mwcs else 'immediate'} merge path: {reason}" + ) + assert MOVED[:7] in reason, f"the deny does not name the new head: {reason}" diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index d3e42e1d9..30272b476 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -15,7 +15,8 @@ loop could have silently flipped. Every failure-path test below therefore assert explicitly, so the behaviour is a contract rather than a coincidence. Observable contract of the script: - exit 0 -> enumeration complete and bound to the expected head; stdout is the authoritative path set + exit 0 -> enumeration complete, with no head/base movement observable from inside it (ONE-WAY; + an A->B->A alias passes — #664/#803); stdout is the authoritative path set exit 1 -> could not enumerate/verify; stdout meaningless, caller MUST withhold any exemption exit 2 -> usage error """ @@ -974,19 +975,64 @@ if "/timeline" in url: if mode == "unreadable": print("502 Bad Gateway") sys.exit(0) + if mode == "trusted-then-unreadable": + # The FIRST count succeeds, the SECOND cannot be established. `timeline_reads.txt` is + # incremented only on the LAST-REAL-PAGE branch below, after the `page > pages` and + # `page < pages` early exits — so it counts REAL PAGES SERVED, not requests: it holds 1 + # after the before-count, and `>= 2` first becomes true partway through the after-count. + # (An earlier version of this comment said "requests 0-1 are the before-count", which is a + # mechanism nobody measured; the arrangement is produced either way, but the description + # was of a different program.) + # + # This is the only arrangement that separates the head arm from its trust guard: with both + # counts trusted the guard is a no-op, and with both untrusted `pushes_before` is 0 too, so + # the arm cannot fire either way. Only trusted-then-untrusted makes the two disagree. + ctr0 = out / "timeline_reads.txt" + seen0 = int(ctr0.read_text()) if ctr0.exists() else 0 + if seen0 >= 2: + print("502 Bad Gateway") + sys.exit(0) + if mode == "untyped-rows": + # Well-formed JSON, well-formed array, rows the walk cannot classify: no `.type` at all on + # one row and a non-string one on another. Both are invisible to a `select(.type == ...)`. + # + # THIS MODE MUST STILL TERMINATE, and getting that wrong made the test measure nothing. + # Serving these rows on EVERY page ran the walk into its 20-page cap, which withholds the + # exemption on its own — so the test passed identically with the row validation deleted. + # Page 2 therefore terminates normally: without the guard the walk completes and certifies a + # zero count (exemption GRANTED), with it the page is refused (exemption withheld). Only + # then does the assertion discriminate. + if page > 1: + print(os.environ.get("STUB_TIMELINE_TERMINATOR", "null")) + else: + print(json.dumps([{"id": 1}, {"id": 2, "type": 7}, {"id": 3, "type": "comment"}])) + sys.exit(0) if mode == "transport-error": sys.exit(22) - # Page 2+ terminates the walk. THE DEFAULT IS `null`, NOT `[]`, because that is what this - # endpoint really returns past the end — measured at Gitea 1.27.1 (ersatztv#751). This stub - # printed `[]` for two rounds of #706 work while the comment above claimed it mirrored measured - # reality, so every fence test was green against a shape the server never produces, and the - # fence's `array`-only type gate — which reads `null` as unreadable — was never exercised. On the - # real instance that made `rt_ok` false for EVERY pr, so the fence withheld every exemption. - # Parameterised rather than simply corrected: `/issues/{n}/comments` really does return `[]` when - # empty, so both shapes are live on this server and the job must accept either. - if page > 1: + # MULTI-PAGE TIMELINES (ersatztv#803 review). Until this existed the double always terminated + # after page 1 and put every event on it, so nothing in the suite exercised the walk ACCUMULATING + # across pages — a walk that read page 1 and stopped passed every fence test. `STUB_TIMELINE_PAGES` + # says how many REAL pages there are; page P+1 is the terminator. Events go on the LAST real page, + # which is where the server would put them: timeline rows are ordered ASCENDING, so the newest + # events — the ones a fence is looking for — are the furthest from page 1. Earlier pages carry + # filler `comment` rows so they are full-length and cannot be mistaken for the end. + # + # PAST THE LAST REAL PAGE THE DEFAULT IS `null`, NOT `[]`, because that is what this endpoint + # really returns past the end — measured at Gitea 1.27.1 (ersatztv#751). This stub printed `[]` + # for two rounds of #706 work while its comment claimed it mirrored measured reality, so every + # fence test was green against a shape the server never produces, and the fence's `array`-only + # type gate — which reads `null` as unreadable — was never exercised. On the real instance that + # made `rt_ok` false for EVERY pr, so the fence withheld every exemption. Parameterised rather + # than simply corrected: `/issues/{n}/comments` really does return `[]` when empty, so both + # shapes are live on this server and the job must accept either. + pages = int(os.environ.get("STUB_TIMELINE_PAGES", "1")) + if page > pages: print(os.environ.get("STUB_TIMELINE_TERMINATOR", "null")) sys.exit(0) + if page < pages: + print(json.dumps([{"id": 100 + i, "type": "comment"} for i in range(50)])) + sys.exit(0) + n_before, n_after = 0, 0 if mode.startswith("stable:"): n_before = n_after = int(mode.split(":", 1)[1]) @@ -995,12 +1041,39 @@ if "/timeline" in url: # before the POST. This is the race-1 window: the PR was retargeted while the job classified. a, b = mode.split(":", 1)[1].split(",") n_before, n_after = int(a), int(b) + # THE PUSH AXIS IS INDEPENDENT OF THE RETARGET AXIS (ersatztv#803/#664), and it has to be, or the + # head-fence tests could not distinguish which fence fired. Both counts come off the SAME page — + # the job makes one walk and tallies two `.type` values — so the stub serves them from one + # response, but the two `moves:`/`stable:` knobs are separate. + pmode = os.environ.get("STUB_PUSH_MODE", "none") + p_before, p_after = 0, 0 + if pmode.startswith("stable:"): + p_before = p_after = int(pmode.split(":", 1)[1]) + elif pmode.startswith("moves:"): + a, b = pmode.split(":", 1)[1].split(",") + p_before, p_after = int(a), int(b) ctr = out / "timeline_reads.txt" seen = int(ctr.read_text()) if ctr.exists() else 0 ctr.write_text(str(seen + 1)) n = n_before if seen == 0 else n_after + np = p_before if seen == 0 else p_after + # `is_force_push` is emitted because the real endpoint emits it (measured on PR #761), NOT + # because the job reads it — the fence counts rows by `.type` and never parses the body. A stub + # that omitted it would leave a reader thinking the field is unused by contract rather than by + # choice; one that made the count DEPEND on it would test a job we did not write. + # + # IT ALTERNATES, STARTING FALSE. Every row carried `true` until 2026-08-28, which made the + # documented invariant untestable: narrowing the tally to + # `select(.type == "pull_push" and (.body | fromjson | .is_force_push))` passed the ENTIRE fence + # suite. That narrowing is wrong precisely where it matters — an ordinary push also invalidates a + # mid-flight enumeration, and an ABA's restoring push can be non-forced when H1 is an ancestor — + # so the double has to contain at least one row the narrowed predicate would drop. print(json.dumps([{"id": 1000 + i, "type": "change_target_branch", "old_ref": "main", "new_ref": "scratch"} for i in range(n)] + + [{"id": 1500 + i, "type": "pull_push", + "body": json.dumps({"is_force_push": bool(i % 2), + "commit_ids": ["a" * 40, "b" * 40]})} + for i in range(np)] + [{"id": 900, "type": "comment"}])) sys.exit(0) @@ -1490,6 +1563,8 @@ def _run_classify( status_creator: str | None = "timothy", status_desc: str = "Review-verdict: MERGEABLE @ a9e3e23 (base: main)", timeline_mode: str = "none", + push_mode: str = "none", + timeline_pages: int = 1, history_mode: str = "none", history_creator: str = "timothy", midrun_creator: str = "timothy", @@ -1530,6 +1605,8 @@ def _run_classify( env["STUB_STATUS_CREATOR"] = status_creator or "" env["STUB_STATUS_DESC"] = status_desc env["STUB_TIMELINE_MODE"] = timeline_mode + env["STUB_PUSH_MODE"] = push_mode + env["STUB_TIMELINE_PAGES"] = str(timeline_pages) env["STUB_TIMELINE_TERMINATOR"] = timeline_terminator env["STUB_STATUS_EMPTY_SHAPE"] = status_empty_shape env["STUB_HISTORY_MODE"] = history_mode @@ -2751,6 +2828,472 @@ def test_positive_control_a_QUIET_run_still_posts_its_exemption(tmp_path): ) +def _fold_continuations(lines): + r"""Join backslash-continued shell lines the way bash does: delete the backslash AND the newline, + joining with NOTHING. + + The exit-status detector is per-line, so a `jq \` whose flag sits on the next line would slip + past it. Folding first closes that — but only if the fold matches the shell, and the separator is + where it goes wrong. An earlier version joined with a SPACE, which is not cosmetic: it turns + `jq -\` + `e '.'` into `jq - e '.'` (two tokens, no flag) where bash produces `jq -e '.'`. + + WHICH SPLITS ARE ACTUALLY REACHABLE, measured rather than assumed, because the obvious + construction is not one. Bash does NOT strip the continuation line's leading whitespace, so + `echo -\` followed by an INDENTED `e X` prints `- e X` — three tokens, no flag re-formed, and + `jq -\` + indented `e` exits 3 rather than acting as `-e`. The reachable case is a continuation + whose next line begins at COLUMN 0, and in this file that is not exotic: YAML strips the `run:` + block's common indentation (10 spaces here), so any line written at the block's base indent runs + at column 0. Verified end to end: a mutant of that exact shape parses as YAML, passes `bash -n`, + forms a live `jq -e` — and reddens the guard under this fold while passing under the space-joining + one. + + NO `rstrip()`, for the same reason. A backslash followed by trailing whitespace is not a + continuation to bash — the backslash escapes the SPACE — and tolerating it was the one route by + which this fold could DESTROY a `jq` token rather than fabricate one: `ok=y\` plus invisible + trailing spaces, then a dedented `jq -e ...`, folds to `okyjq -e`, and `\bjq\b` finds no word + boundary after the `y`. Every other divergence here over-joins, which can only ever cost a false + positive. Matching the shell exactly is both simpler and the only direction that cannot lose a + `jq`. + """ + joined, buf = [], "" + for ln in lines: + if ln.endswith("\\"): + buf += ln[:-1] + else: + joined.append(buf + ln) + buf = "" + if buf: + joined.append(buf) + return joined + + +def _uses_jq_exit_status(line: str) -> bool: + """True if `line` invokes jq with the exit-status flag, in any spelling of the FLAG. + + Scope, stated because the obvious reading is wider than the truth: this is per-line and knows + nothing about shell continuations. `jq \\` on one line and `-e` on the next is a real evasion, + and it is closed by the CALLER, which folds continuations before scanning — not here. + + THIS IS THE THIRD VERSION AND IT IS DELIBERATELY THE DUMBEST. The first was + `"jq -e" in line`, which missed `jq -re`, `jq -e` and `jq --exit-status` (it caught `jq -er` + only because `jq -e` is a substring of it). The second parsed + the LEADING option tokens and stopped at the first non-option, on the reasoning that everything + after the jq program is source — and that reasoning bought two more false negatives: + `jq --argjson e 1 -e '.'` (the walk stops at the bare `e` argument) and `jq '.a' -e` (jq accepts + options after the filter). Three defects in one string-matching predicate is this repo's + documented signal to stop adding cases and remove the cleverness instead. + + So: scan EVERY whitespace token after each `jq`, with no attempt to find where the program + begins. That over-approximates — a jq program containing a token that looks like a short flag + with an `e` in it would trip this — and the over-approximation is the SAFE direction. It guards + one short function we own: a false positive costs a rewrite, a false negative restores the + jq-1.6 exit-status divergence inside the code that writes a required status. + + BE EXACT ABOUT WHY THERE IS NO FALSE POSITIVE TODAY, because the loose version of that sentence + is false. The guarded function DOES contain `-`-prefixed tokens carrying an `e`: `-eq`, `-ne` + and `-le`, the shell test operators. They are invisible to this only because none of them shares + a LINE with a `jq` invocation, and the scan starts at `jq`. Measured: of its five jq-bearing + lines, none carries a `-`-prefixed token after `jq` other than `-r`. A future + `if [ "$(... | jq -r .)" -eq 0 ]` would trip this guard for no real reason — and the fix then is + to split the line, not to make the detector clever again. + """ + for m in re.finditer(r"\bjq\b", line): + for tok in line[m.end() :].split(): + if tok == "--exit-status": + return True + if tok.startswith("-") and not tok.startswith("--") and "e" in tok[1:]: + return True + return False + + +@pytest.mark.parametrize( + "raw,folded", + [ + # The reachable shape: the continuation's next line is at column 0, which is what a line + # written at the `run:` block's base indent becomes once YAML strips it. Bash re-forms the + # token; so must the fold, or the detector sees `jq - e` and reports nothing. + ([" x=$(... | jq -\\", "e 'length')"], [" x=$(... | jq -e 'length')"]), + # An INDENTED continuation does NOT re-form the token — bash keeps the leading whitespace — + # so the fold must not invent one either. Measured: `echo -\` + indented `e X` prints `- e X`. + ([" x=$(... | jq -\\", " e 'length')"], [" x=$(... | jq - e 'length')"]), + (["a\\", "b\\", "c"], ["abc"]), # two consecutive continuations + (["plain", "lines"], ["plain", "lines"]), # nothing to fold + (["trailing\\"], ["trailing"]), # continuation with nothing after it + # A backslash followed by whitespace is NOT a continuation — bash escapes the space. + # Nothing bound this before, so dropping the shell-faithfulness survived every other + # case while opening the one route by which the fold can HIDE a `jq` token instead of + # inventing one: `ok=y\` + trailing spaces then a dedented `jq -e` folds to `okyjq -e`, + # and `\\bjq\\b` finds no boundary after the `y`. + (["a\\ ", "b"], ["a\\ ", "b"]), + ], +) +def test_the_continuation_fold_matches_the_SHELL(raw, folded): + r"""It joins with NOTHING, because that is what `\` removal does. + + Joining with a space is the intuitive choice and it silently defeats the guard downstream: it + turns `jq -\` + `e` into `jq - e`, two tokens carrying no flag, where bash produces `jq -e`. + """ + assert _fold_continuations(raw) == folded + + +@pytest.mark.parametrize( + "line,expected", + [ + (""" if ! printf '%s' "$raw" | jq -e 'all(.[]; .a)' ; then""", True), + (""" x=$(jq --exit-status '.a' <<< "$raw")""", True), + (""" x=$(jq -re '.a')""", True), + (""" x=$(jq -er '.a')""", True), + (""" x=$(jq -e '.a')""", True), + # The two the leading-token parser missed, named by cold review of round 4. + (""" x=$(jq --argjson e 1 -e '.')""", True), + (""" x=$(jq '.a' -e)""", True), + (""" n=$(printf '%s' "$raw" | jq -r 'length')""", False), + (''' kind=$(printf '%s' "$raw" | jq -r 'type' 2>/dev/null) || kind=""''', False), + # The jq PROGRAM may legitimately contain an `e`, or a `-e` inside a string. These pass + # because neither carries a `-`-prefixed token at all — NOT because the scan stops at the + # program. It does not: that was the v2 reasoning this file withdrew, and restating it here + # would invite someone to "restore" the leading-token walk and its two false negatives. + (""" bad=$(jq -r '[.[] | select(.type != "e")] | length')""", False), + (""" m=$(jq -r --arg e x '[.[] | select(.type == $e)] | length')""", False), + ], +) +def test_the_jq_exit_status_detector_reads_FLAGS_not_a_substring(line, expected): + """The detector is itself a string-matching predicate, so it gets its own cases. + + `"jq -e" in line` was the first version. It misses `jq -re`, `jq -er`, `jq -e` and + `jq --exit-status` — three spellings of the same request (it catches `jq -er`, since `jq -e` + is a substring of it) — and a guard a rename walks straight + through is not a guard. The false-positive rows matter as much: an `e` inside the jq PROGRAM is + not a flag, and a detector that reddens on one would make the rule unusable and get waived. + """ + assert _uses_jq_exit_status(line) is expected + + +def test_count_pr_mutations_uses_NO_jq_e(tmp_path): + """The timeline walk reads jq output as VALUES; `jq -e`'s exit status is banned inside it. + + The function's own comment states the rule and the reason: `jq -e` reports the truthiness of its + LAST OUTPUT, and over empty input jq 1.6 exits 0 where >= 1.7 exits 4 — the divergence that + already took this workflow's enforced gate down once (ersatztv#647), on a runner that still ships + 1.6. Every tally here is therefore read with `jq -r` and range-checked in shell. + + This exists because the rule was BROKEN one commit after it was quoted (ersatztv#803 round 2 added + a row-validation guard using `jq -e`, three lines below the comment forbidding it) and nothing + caught it — it was found by hand while re-reading the diff. A convention stated only in prose is + one refactor from being false, and this one is function-scoped: `jq -e` is legitimate elsewhere in + this same workflow and in the hook, so a file-wide grep would be wrong. + + Comments are stripped, so the paragraphs explaining the rule do not redden it. + """ + body = _classify_step()["run"] + lines = [ln for ln in body.splitlines() if not ln.lstrip().startswith("#")] + fn, depth, inside = [], 0, False + for ln in lines: + if "count_pr_mutations() {" in ln: + inside = True + if inside: + fn.append(ln) + depth += ln.count("{") - ln.count("}") + if depth == 0 and len(fn) > 1: + break + assert inside and depth == 0, ( + "could not locate the `count_pr_mutations` body in the classify step — if it was renamed, " + "rename it here too rather than deleting this guard" + ) + # A BALANCED PREFIX IS NOT THE WHOLE FUNCTION. `depth == 0` is reached by the real closing brace + # today, but it would also be reached early by any line with a net-negative brace count, and the + # assertion above cannot tell the two apart — so a `jq -e` past that point would be invisible + # while the guard reported success. + # + # The check is STRUCTURAL, not a pinned statement. An earlier version asserted that the body + # contained `page=$(( page + 1 ))`, which is a real line near the end and would have worked — + # and would also have reddened on any refactor of the increment, for no reason connected to + # this guard. A guard that cries wolf gets waived, which is how the repo already has to + # document one CI job as "do nothing". Requiring the capture to END on the closing brace says + # the same thing about extent while surviving every edit to the body. + assert fn[-1].strip() == "}", ( + "the extracted body does not end on `count_pr_mutations`'s closing brace, so it is a PREFIX " + f"of the function rather than the whole of it — this guard would be scanning only part of " + f"what it claims to cover. Last captured line: {fn[-1]!r}" + ) + joined = _fold_continuations(fn) + offenders = [ln.strip() for ln in joined if _uses_jq_exit_status(ln)] + assert not offenders, ( + "`jq -e` appeared inside `count_pr_mutations`, whose own comment bans it because the runner " + "ships jq 1.6 and `-e`'s exit status over empty input diverges there (ersatztv#647/#803). " + "Read the value with `jq -r` and range-check it in shell instead:\n " + "\n ".join(offenders) + ) + + +def test_a_HEAD_ABA_DURING_the_run_posts_NOTHING(tmp_path): + """THE ersatztv#803/#664 test: `H1 -> H2 -> H1` across the enumeration. + + This is the case no sha comparison can see, and the reason the fence keys on a COUNT. The head + is force-pushed away and back while the job classifies, so `$SHA` still equals `.head.sha` at + both ends — `pr-changed-files.sh` exits 0, having observed no movement — while the + middle pages were served from `H2`. A mixed file list can then produce a docs-only exemption + that no single head ever justified. + + TWO push events, not one, because that is what the ABA costs: away, and back. The retarget axis + is held at `none` deliberately, so a pass here cannot be the BASE fence firing. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode="none", push_mode="moves:0,2") + assert r.returncode == 0, r.stderr + assert posted is None, ( + "the job posted an exemption for a head that was force-pushed away and back underneath it " + f"— the head-mutation fence did not fire. Log:\n{r.stdout[-900:]}" + ) + # The DISCRIMINATOR, and specifically the head one: the retarget arm also ends in "posted + # nothing", so without this the test would pass against a job whose base fence fired for the + # wrong reason. + assert "head branch was pushed while this job was classifying" in r.stdout, ( + f"nothing was posted, but NOT via the head fence. Log:\n{r.stdout[-900:]}" + ) + assert "retargeted while this job was classifying" not in r.stdout, ( + "the BASE fence fired on a run whose base never moved, so this test is not measuring the " + f"head axis. Log:\n{r.stdout[-900:]}" + ) + + +@pytest.mark.parametrize("terminator", ["null", "[]"]) +def test_the_head_fence_holds_under_EITHER_terminator_shape(tmp_path, terminator): + """The `[]` and `null` walk-exits are separate arms, and the head axis must publish from both. + + Deleting `hp_count=$ptotal` from the empty-ARRAY arm alone left every fence test green, because + every one of them terminated on the default `null`. The head fence would then be silently dead + for any timeline ending in `[]` — a shape this code explicitly supports and this server really + produces on a sibling endpoint. + """ + posted, r = _run_classify( + tmp_path, + _emitting("docs/a.md"), + timeline_mode="none", + push_mode="moves:0,2", + timeline_terminator=terminator, + ) + assert posted is None, ( + f"an exemption was granted over an ABA on a timeline terminating with `{terminator}` — the " + f"head count is not published from that arm. Log:\n{r.stdout[-900:]}" + ) + assert "head branch was pushed while this job was classifying" in r.stdout, ( + f"nothing was posted, but NOT via the head fence. Log:\n{r.stdout[-900:]}" + ) + + +def test_the_head_arm_DEFERS_to_the_trust_guard_when_the_second_count_fails(tmp_path): + """An untrusted re-count is "cannot tell", never "the head moved". + + Both outcomes post nothing, so only the DIAGNOSTIC separates them — and the difference is not + cosmetic: the untrusted branch is an `::error::` saying an exemption cannot be shown to have been + computed at a single head and that a later run only helps if the cause was transient, while the + head arm is a `::notice::` promising a successor run that the `synchronize` event has queued. + Reporting a push that was never observed would send a reader looking for a force-push that did + not happen. + + Dropping the `rt_ok` conditions from the head arm leaves the rest of the suite green, because + every other test has both counts trusted (guard is a no-op) or both untrusted (`pushes_before` + is 0, so the arm cannot fire). Only a trusted-then-untrusted pair separates them. + """ + posted, r = _run_classify( + tmp_path, + _emitting("docs/a.md"), + timeline_mode="trusted-then-unreadable", + push_mode="stable:3", + ) + assert posted is None, f"an exemption was granted on an untrusted count. Log:\n{r.stdout[-900:]}" + assert "Could not establish a trusted retarget/push count" in r.stdout, ( + f"the untrusted re-count was not reported as such. Log:\n{r.stdout[-900:]}" + ) + assert "head branch was pushed while this job was classifying" not in r.stdout, ( + "an untrusted re-count was reported as observed head movement — the head arm is firing " + f"without its trust guard. Log:\n{r.stdout[-900:]}" + ) + + +def test_a_SINGLE_head_push_during_the_run_also_posts_NOTHING(tmp_path): + """One push is the ordinary one-way move, and the fence covers it too. + + `pr-changed-files.sh` already fails closed on this via its own sha comparison, so the exemption + was withheld before #803 — but through a DIFFERENT mechanism, one that reports an enumeration + error rather than a handoff. Asserting the fence arm here pins that the head axis is not + narrowed to "two or more pushes" by some future edit reasoning that one-way is already covered. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode="none", push_mode="moves:0,1") + assert r.returncode == 0, f"the job died rather than abstaining cleanly: {r.stderr[-600:]}" + assert posted is None, f"an exemption was posted for a PR pushed mid-classification. Log:\n{r.stdout[-900:]}" + assert "head branch was pushed while this job was classifying" in r.stdout, ( + f"nothing was posted, but NOT via the head fence. Log:\n{r.stdout[-900:]}" + ) + + +def test_a_PR_PUSHED_BEFORE_the_run_but_QUIET_during_it_is_STILL_exempt(tmp_path): + """The head fence keys on MOTION during this run, never on "has ever been pushed". + + This is the test that would go red on the most plausible wrong implementation. EVERY pull + request has a non-zero `pull_push` count — it is created by a push, and PR #802 carries + eighteen — so a fence keying on the count being non-zero, rather than on it CHANGING, would + withhold the exemption from every PR that has ever existed. That is not a subtle regression: it + is the #751 shape, where a fence shipped and silently refused every exemption on the instance. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode="none", push_mode="stable:18") + assert posted is not None and posted["state"] == "success", ( + "a PR with a settled, non-zero push history was refused its exemption — the head fence is " + f"testing the count's VALUE instead of its MOVEMENT. Log:\n{r.stdout[-900:]}" + ) + + +# Each row: (timeline_mode, push_mode, arm expected to fire, arm that must stay silent). +# BOTH DIRECTIONS ARE POSED, because they fail to different mutations and only one of them is +# reachable at all. The arms are evaluated base-first, so a tally that folds PUSHES into the retarget +# total is caught only by the push-moves row (the base arm fires and pre-empts the head message); +# folding retargets into the push total is NOT observable from here for the same ordering reason, +# and is deliberately not claimed to be. Stating that is the point — an earlier version of this test +# asserted the base-moves row alone while its docstring claimed it caught the folding mutation, and +# it stayed GREEN under exactly that mutation. +_AXIS_ROWS = [ + ( + "moves:0,1", + "stable:18", + "retargeted while this job was classifying", + "head branch was pushed while this job was classifying", + ), + ( + "stable:2", + "moves:0,2", + "head branch was pushed while this job was classifying", + "retargeted while this job was classifying", + ), +] + + +@pytest.mark.parametrize("tl_mode,push_mode,expected,silent", _AXIS_ROWS) +def test_the_head_and_base_axes_are_counted_SEPARATELY(tmp_path, tl_mode, push_mode, expected, silent): + """One walk, two tallies, and neither may be read for the other. + + The cheapest wrong implementation reuses one tally for both axes; it passes every single-axis + test above, because with only one axis in motion a shared counter still moves. What distinguishes + it is holding one axis STABLE AND NON-ZERO while the other moves, then demanding that the arm + which reports is the one that actually moved. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode=tl_mode, push_mode=push_mode) + assert posted is None, f"neither fence fired. Log:\n{r.stdout[-900:]}" + assert expected in r.stdout, f"the moving axis was not the one reported. Log:\n{r.stdout[-900:]}" + assert silent not in r.stdout, ( + f"a STABLE axis was reported as having moved — the two counts are sharing a tally. Log:\n{r.stdout[-900:]}" + ) + + +def test_a_push_landing_BEYOND_page_1_is_still_counted(tmp_path): + """The walk must ACCUMULATE across pages, not answer from the first one. + + Timeline rows are ordered ASCENDING, so the newest events — exactly the ones a fence looks for — + are furthest from page 1. A walk that read page 1 and stopped would see a stable zero on both + counts and let the ABA through, while passing every other fence test in this file: until the + double grew `timeline_pages`, every event it served was on page 1. + """ + posted, r = _run_classify( + tmp_path, + _emitting("docs/a.md"), + timeline_mode="none", + push_mode="moves:0,2", + timeline_pages=3, + ) + assert posted is None, ( + "an exemption was granted over an ABA whose push events lived past page 1 — the walk is " + f"answering from the first page. Log:\n{r.stdout[-900:]}" + ) + assert "head branch was pushed while this job was classifying" in r.stdout, ( + f"nothing was posted, but NOT via the head fence. Log:\n{r.stdout[-900:]}" + ) + + +def test_positive_control_a_MULTI_PAGE_quiet_timeline_still_exempts(tmp_path): + """Without this, the test above passes against a walk that refuses every multi-page timeline.""" + posted, r = _run_classify( + tmp_path, + _emitting("docs/a.md"), + timeline_mode="none", + push_mode="stable:2", + timeline_pages=3, + ) + assert posted is not None and posted["state"] == "success", ( + f"a quiet PR with a multi-page timeline lost its exemption. Log:\n{r.stdout[-900:]}" + ) + + +@pytest.mark.parametrize("terminator", ["null", "[]"]) +def test_an_EMPTY_FIRST_page_is_untrusted_WHICHEVER_empty_shape_it_is(tmp_path, terminator): + """Both empty shapes must fall through to `rt_ok=no`, and only one of them did. + + The `null` arm required `page > 1`; the empty-ARRAY arm trusted any page, so a `[]` first page + certified "nothing was retargeted and nothing was pushed" over a response the walk could not + explain, and a docs-only PR was exempted on it. The comment three lines above already claimed the + safe behaviour, so the code contradicted its own stated rule for one of the two shapes — and this + server produces both, on different endpoints. Parameterised rather than corrected in place, + because the point is that the two shapes now take the SAME rule. + """ + posted, r = _run_classify( + tmp_path, + _emitting("docs/a.md"), + timeline_mode="empty-first-page", + timeline_terminator=terminator, + ) + assert posted is None, ( + f"an exemption was granted from an empty FIRST page ({terminator}) — a zero count was " + f"trusted from a response the walk cannot explain. Log:\n{r.stdout[-900:]}" + ) + + +def test_a_timeline_row_with_NO_READABLE_TYPE_is_not_counted_as_nothing(tmp_path): + """A shape we cannot read is not evidence that nothing happened. + + Both tallies select on `.type`. A row lacking one, or carrying a non-string one, matches neither + and was silently treated as some other event — so a page of unclassifiable rows certified a zero + count. This is the same validate-what-you-consume rule `pr-changed-files.sh` applies to every row + IT extracts, which the walk gating the write did not have. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode="untyped-rows") + assert posted is None, ( + "an exemption was granted over a timeline page whose rows carry no readable `.type` — the " + f"count was trusted from rows the walk cannot classify. Log:\n{r.stdout[-900:]}" + ) + + +def test_the_head_fence_reports_the_OBSERVED_counts_in_its_notice(tmp_path): + """The notice must name the numbers it fenced on. + + A fence whose diagnostic says only "something moved" cannot be triaged from a run log, and this + workflow's history is full of runs that posted nothing for reasons nobody could reconstruct + afterwards (#751 went unnoticed for exactly that reason). Pin the before/after pair. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode="none", push_mode="moves:3,5") + assert posted is None, f"the head fence did not fire. Log:\n{r.stdout[-900:]}" + assert "(3 -> 5 push events)" in r.stdout, ( + f"the head fence notice does not report the counts it fenced on. Log:\n{r.stdout[-900:]}" + ) + + +def test_a_PROTECTED_pr_pushed_mid_run_ALSO_posts_nothing(tmp_path): + """The fence covers `pending` as well as the exemption `success`. + + Same rule as the base axis, and the same reason it is one rule rather than a + reason-about-it-per-state condition: the successor run is guaranteed either way, so there is + nothing to buy by writing a value this run already knows was computed across two heads. + """ + posted, r = _run_classify(tmp_path, _emitting("ErsatzTV/Program.cs"), timeline_mode="none", push_mode="moves:0,2") + assert r.returncode == 0, f"the job died rather than abstaining cleanly: {r.stderr[-600:]}" + assert posted is None, ( + "a run that saw its head move still wrote a status for a non-exempt PR — the fence is " + f"scoped to `success` only. Log:\n{r.stdout[-900:]}" + ) + # Assert the DISCRIMINATOR. "Posted nothing" is also produced by a crash, an unreadable timeline + # and several unrelated abstentions, so without this the test proves only that something went + # wrong somewhere. + assert "head branch was pushed while this job was classifying" in r.stdout, ( + f"nothing was posted, but NOT via the head fence. Log:\n{r.stdout[-900:]}" + ) + + def test_a_PR_retargeted_BEFORE_the_run_but_QUIET_during_it_is_STILL_exempt(tmp_path): """The fence keys on MOTION during this run, never on "has ever been retargeted". @@ -2977,7 +3520,9 @@ def test_an_UNTRUSTED_count_withholds_the_exemption_BY_THAT_BRANCH(tmp_path): posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode="unreadable") assert posted is None assert r.returncode == 0, f"the job died rather than declining cleanly: {r.stderr[-800:]}" - assert "Could not establish a trusted retarget count" in r.stdout, ( + # The wording covers BOTH axes since ersatztv#803 — one walk certifies one trust flag, so an + # unreadable page abandons the push count and the retarget count together. + assert "Could not establish a trusted retarget/push count" in r.stdout, ( f"nothing posted, but not via the untrusted-count branch. Log:\n{r.stdout[-800:]}" ) @@ -3453,7 +3998,8 @@ def test_the_fence_TRUSTS_the_count_and_POSTS_when_the_timeline_terminates(tmp_p from posting anything. A page past the end of `/issues/{n}/timeline` is the JSON value `null` on this instance, not `[]`. - `count_retargets` gated on `type == "array"` and so treated the real terminator as unreadable: the + `count_pr_mutations` (named `count_retargets` then) gated on `type == "array"` and so treated + the real terminator as unreadable: the walk never reached a validated empty page, `rt_ok` was never `yes` for ANY pull request, and the fence therefore withheld every exemption `success`. Renovate and docs-only PRs got NO status — the same user-visible outcome as #751, by an unrelated route. @@ -3475,7 +4021,7 @@ def test_the_fence_TRUSTS_the_count_and_POSTS_when_the_timeline_terminates(tmp_p assert r.returncode == 0, r.stderr assert posted is not None, ( f"a docs-only PR whose timeline terminates with `{terminator}` got NO status at all. The " - "fence could not establish a trusted retarget count, so it withheld the exemption — which " + "fence could not establish a trusted retarget/push count, so it withheld the exemption — which " "leaves the required review-verdict/h10 absent and the PR unmergeable with no bypass " f"(ersatztv#751).\n{r.stdout[-1200:]}" ) @@ -3728,7 +4274,7 @@ def test_an_UNREADABLE_page_2_refuses_to_conclude_that_no_verdict_exists(tmp_pat """The two refuse branches of the completeness probe, which cold review found untested. Worth a test rather than trusting the shape: this file's history is two consecutive guards that - were UNREACHABLE — the `count_retargets` type gate that never saw a real terminator, and a + were UNREACHABLE — the timeline walk's type gate that never saw a real terminator, and a full-page check written against a limit of 100 on a server that caps at 50. An unexercised branch here has a track record.