diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index eccb2fb46..dd32be129 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -182,6 +182,47 @@ jobs: run: | set -euo pipefail + # NEVER WRITE AN EXPRESSION DELIMITER ANYWHERE IN THIS BODY, INCLUDING IN A COMMENT + # (ersatztv#751). A shell comment is not inert here. The runner scans this whole `run:` + # scalar for the expression opener before bash ever sees it, and one occurrence makes it + # rewrite the ENTIRE body into a single `format(...)` call so the result can be spliced + # back in. That rewrite is all-or-nothing: if the payload does not parse as an expression + # the interpolation of the whole scalar fails, and — the part that made this expensive — + # the runner DROPS THE STEP AND REPORTS THE JOB GREEN. + # + # That is exactly how this gate spent 2026-08-03 to 2026-08-06 dead: the #706 note above, + # explaining why a concurrency group does not work, quoted a `concurrency:` snippet + # containing a PR-number expression as ILLUSTRATION. `pr number` is not a valid + # expression, so this step never ran, `review-verdict/h10` was never posted by anything + # but a human hand, and both exemption classes silently stopped working while every run + # reported success. The prose documenting a fix disabled the fix. + # + # Two things now stop a recurrence, and they are deliberately different in kind: the + # start-marker below turns a dropped step into a RED job instead of a green one, and + # `test_the_verdict_workflow_has_NO_expression_delimiter_in_any_run_body` in + # scripts/tests/test_pr_changed_files.py rejects the delimiter at review time. Note that + # the older workflow-shape tests all scan comment-STRIPPED text (`_code_lines`) precisely + # so prose cannot redden them — which makes every one of them structurally blind to this + # class. The new test reads the raw scalar for that reason. To describe an expression in + # prose here, name it (`a github.event.pull_request.number expression`); do not quote the + # delimiters. + # + # THE START MARKER (ersatztv#751). Written before anything else can fail, and read by the + # `Assert the classifier actually executed` step below. Its absence means this step did + # not begin — the silent-green failure mode — and that is now a job failure. It records + # only that execution STARTED: every `exit 0` abstention path below is a legitimate + # outcome, so completion is deliberately not what is asserted. + # KEYED ON THE RUN, not a fixed name. Measured on this instance, `RUNNER_TEMP` is `/tmp` — + # not a private per-job directory — so a fixed name lives at a path other jobs can also + # write. The `small` lane starts a container per job today, which makes the file fresh in + # practice, but that is a property of the lane rather than a guarantee, and a STALE marker + # would satisfy the guard below on a run whose step was dropped: a silent PASS, the exact + # failure mode this guard exists to remove. Including the run id and attempt means a marker + # from any other run cannot answer for this one. If the runner does not export them the path + # degrades to a fixed name, which is no worse than having no key at all. + RAN_MARKER="${RUNNER_TEMP:-${GITHUB_WORKSPACE:-/tmp}}/h10-classifier-started-${GITHUB_RUN_ID:-norunid}-${GITHUB_RUN_ATTEMPT:-1}" + : > "$RAN_MARKER" + CONTEXT="review-verdict/h10" # The description this job writes when it repairs its own raced exemption (#706 race 2). # It is a SENTINEL, not just a message: `read_existing_verdict` recognises it, and the @@ -242,7 +283,19 @@ jobs: # containing executable hooks"). A derived rule has to be evaluated against the PR's own # file list, which is the very thing being classified — more moving parts inside a security # predicate, to remove a maintenance burden that is one line per new tooling directory. - PROTECTED='^(\.claude/|\.codex/|\.gitea/|\.husky/|scripts/|docker/ci/)' + # `CLAUDE.md` and `AGENTS.md` are listed even though they are `.md` files, because they are + # not prose about the project — they are the documents that DEFINE the completion protocol, + # the merge-consent convention and the H10 rule itself. `.claude/` being protected while the + # file that specifies what `.claude/` enforces was docs-only-exempt is the same + # self-exemption the header rules out, one directory over. Found by cold review + # (ersatztv#751): driving the real classify body with a lone `CLAUDE.md` change produced + # `review-verdict/h10=success`, "Exempt: docs-only change (no code, no protected path)". + # + # It is fixed HERE rather than deferred because this change is what makes it reachable + # again: no exemption `success` was writable at all while the classify step was dropped, so + # restoring the exemptions restores this hole with them. `README.md` is deliberately NOT + # listed — it is ordinary prose and carries no enforcement. + PROTECTED='^(\.claude/|\.codex/|\.gitea/|\.husky/|scripts/|docker/ci/|CLAUDE\.md$|AGENTS\.md$)' # Docs-only: prose and decision records. Deliberately narrower than the hook's pattern, # which also lets .claude/.gitea/.husky through — that carve-out is safe there only # because it falls through to a HUMAN PROMPT, whereas here it would post a green status @@ -328,11 +381,85 @@ jobs: read_existing_verdict() { local json row json=$(gh "$BASE_URL/repos/$REPO/commits/$SHA/status?limit=100") || json="" - if [ -z "${json//[[:space:]]/}" ] || ! printf '%s' "$json" | jq -e '.statuses | type == "array"' >/dev/null 2>&1; then - echo "::error::Could not read existing commit statuses for ${SHA:0:7}. Refusing to post anything rather than risk overwriting an existing verdict." + # `.statuses` IS `null`, NOT `[]`, ON A HEAD WITH NO STATUSES YET — the same nil-slice + # serialization as the timeline terminator, found by cold review of the fix for that one + # (ersatztv#751). Measured on this instance: PR #739's head 5fa672e2 returns + # `{"state":"pending","total_count":0,"statuses":null}`. An `array`-only gate read that as + # unreadable and took the `exit 1` below, so the job posted NOTHING — fail-closed, but the + # user-visible outcome is exactly the one this issue is about: an exempt PR left with no + # status and, since ersatztv#743, no bypass. Fixing only the timeline site would have left + # the identical bug one function away. + # + # `total_count` is required to agree, so `null` is accepted only as the empty case and not + # as a body that merely lost its array. Anything else is still "cannot tell" and still + # refuses to post, which is what protects an existing verdict from a transient API error. + # WHETHER PAGE 1 IS THE WHOLE LIST CANNOT BE DECIDED FROM PAGE 1 (ersatztv#751). Two + # plausible guards were tried and both are no-ops HERE, which is why this ended up as an + # extra round-trip instead of an arithmetic test: + # + # * `.statuses | length` vs `.total_count` — `total_count` is the count for the PAGE + # RETURNED, not for the commit. Measured at 1.27.1 on 3aed43c6 (6 contexts): + # `?limit=1` gives `len=1, total_count=1`, `?limit=3` gives `len=3, total_count=3`. + # Equal by construction, so the check proves nothing. + # * "refuse when the page came back FULL at the requested limit of 100" — this instance + # caps `limit` at the server-wide `MAX_RESPONSE_ITEMS`, measured at 50 + # (`/issues?limit=100` returns 50). A response can therefore never contain 100 rows and + # the comparison was DEAD CODE. The repo already knew this and said so in + # `scripts/pr-changed-files.sh`, two test files and `ci.script-tests-job`; the guard was + # written anyway. Hardcoding the cap instead would re-break the day it is reconfigured. + # + # So ask the server, and only when it matters. Completeness is needed ONLY to justify the + # conclusion "no verdict exists on this head" — if the row IS on page 1 there is nothing + # further to learn, since this endpoint returns the latest status per CONTEXT and the same + # context cannot recur on a later page. When the row is absent, page 2 is read: any rows + # there mean the list is longer than one page and the verdict could be sitting beyond it, + # so this refuses rather than concluding absence. Cap-independent by construction. + st_kind=$(printf '%s' "$json" | jq -r '.statuses | type' 2>/dev/null) || st_kind="" + # NUMBER, not `jq -r` text: `jq -r` renders the JSON number 0 and the JSON string "0" + # identically, so a schema-corrupted `"total_count": "0"` would satisfy a string compare + # (cold review reproduced this). Requiring the type as well pins the accept path to a real + # numeric zero. + st_total=$(printf '%s' "$json" | jq -r 'if (.total_count | type) == "number" then (.total_count | tostring) else "x" end' 2>/dev/null) || st_total="x" + st_ok=no + case "$st_kind" in + array) st_ok=yes ;; + null) if [ "$st_total" = "0" ]; then st_ok=yes; fi ;; + esac + if [ -z "${json//[[:space:]]/}" ] || [ "$st_ok" != yes ]; then + echo "::error::Could not read existing commit statuses for ${SHA:0:7} (.statuses was '${st_kind:-unparseable}', total_count '${st_total}'). Refusing to post anything rather than risk overwriting an existing verdict." exit 1 fi - row=$(printf '%s' "$json" | jq -r --arg c "$CONTEXT" '[.statuses[] | select(.context == $c)] | first // {}') + # `// []` so the null case cannot hard-error here under `set -e` once it is accepted above. + row=$(printf '%s' "$json" | jq -r --arg c "$CONTEXT" '[(.statuses // [])[] | select(.context == $c)] | first // {}') + # THE COMPLETENESS PROBE, run only when page 1 shows no verdict — see the note above. An + # unreadable or unexpected page 2 is treated as "cannot tell" and refuses, the same + # direction as every other unreadable case here: concluding "no verdict exists" is what + # licenses posting an exemption over one, so it is the conclusion that must be earned. + if [ "$(printf '%s' "$row" | jq -r '.context // ""')" = "" ]; then + more=$(gh "$BASE_URL/repos/$REPO/commits/$SHA/status?limit=100&page=2") || more="" + if [ -z "${more//[[:space:]]/}" ]; then + echo "::error::Could not read page 2 of the commit statuses for ${SHA:0:7}, so 'no verdict exists' cannot be established. Refusing to post anything." + exit 1 + fi + more_kind=$(printf '%s' "$more" | jq -r '.statuses | type' 2>/dev/null) || more_kind="" + more_len=$(printf '%s' "$more" | jq -r '(.statuses // []) | length' 2>/dev/null) || more_len="" + case "$more_kind" in + null) ;; + array) + case "$more_len" in + ''|*[!0-9]*) + echo "::error::Page 2 of the commit statuses for ${SHA:0:7} had a non-numeric length; refusing to conclude that no verdict exists." + exit 1 ;; + 0) ;; + *) + echo "::error::${CONTEXT} was not on page 1 of the statuses for ${SHA:0:7}, but page 2 carries ${more_len} more row(s) — the list is longer than one page and an existing verdict may be beyond it. Refusing to post anything rather than overwrite a verdict this job cannot see. A human verdict clears this: scripts/post-review-verdict.sh ${PR} MERGEABLE." + exit 1 ;; + esac ;; + *) + echo "::error::Page 2 of the commit statuses for ${SHA:0:7} was '${more_kind:-unparseable}'; refusing to conclude that no verdict exists." + exit 1 ;; + esac + fi ex_state=$(printf '%s' "$row" | jq -r '.status // ""') ex_creator=$(printf '%s' "$row" | jq -r '.creator.login // ""') ex_desc=$(printf '%s' "$row" | jq -r '.description // ""') @@ -410,8 +537,8 @@ jobs: # run 7521 (`synchronize`) had started. # # WHY NOT A CONCURRENCY GROUP, which is the obvious answer and what #706 proposed. It does - # not work here, also measured: with `concurrency: {group: …-${{ pr number }}, - # cancel-in-progress: false}` active on an identical probe, runs 7528 and 7529 still ran + # not work here, also measured: with a `concurrency:` key grouping by PR number and + # `cancel-in-progress: false` active on an identical probe, runs 7528 and 7529 still ran # CONCURRENTLY and 7528 ended 36s after 7529 began. Gitea 1.25.4 does auto-cancel superseded # `push` runs on a branch — a negative control with no `concurrency:` key at all showed that — # but that behaviour does NOT extend to `pull_request_target`. `cancel-in-progress: true` is @@ -438,19 +565,63 @@ jobs: # coming — a real stall. The retarget count moves only for the mutation that actually # invalidates a classification, and that mutation always brings its own re-run. # - # Completeness is a guard, not an assumption (`ci.paged-endpoint-completeness`): the count is + # Completeness is a guard, not an assumption (`ci.verdict-write-retarget-fence`; this + # cited `ci.paged-endpoint-completeness` until 2026-08-06, a key that has never existed as + # a record — resolve decisions through the catalog, never through a key or path quoted in a + # comment). The count is # 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() { rt_count=0 rt_ok=no - local page=1 raw n m total=0 + local page=1 raw n m kind total=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:]]/}" ] || ! printf '%s' "$raw" | jq -e 'type == "array"' >/dev/null 2>&1; then - return 0 - fi + if [ -z "${raw//[[:space:]]/}" ]; then return 0; fi + # A PAGE PAST THE END IS JSON `null` HERE, NOT `[]` (ersatztv#751). Measured on this + # instance at Gitea 1.27.1 (page 1 non-empty, page 2 exhausted): page 2 of PR #752's timeline is the + # four bytes `null`, while `/issues/{n}/comments` with no comments returns `[]`. The + # instance is NOT consistent between endpoints, so the terminator has to be read from + # what this endpoint actually does rather than from the shape a list endpoint "should" + # return. + # + # This mattered far more than it looks. An `array`-only type gate treated `null` as + # unreadable, so the walk NEVER reached a validated empty page, `rt_ok` was never `yes` + # for ANY pull request, and the fence below therefore withheld every exemption + # `success` — permanently. Renovate and docs-only PRs got no status at all rather than + # an auto-pass, which is the same user-visible outcome as ersatztv#751 arriving by a + # completely different route. It was invisible because it shipped in the SAME commit + # (8f6d4f443) that stopped this step from executing at all, so the fence had never once + # run in production, and because the test double asserted the wrong shape — it printed + # `[]` for a page past the end while claiming to mirror measured reality. + # + # Read the type as a VALUE rather than through `jq -e`. `jq -e` reports the truthiness + # of the last output, so distinguishing "the body is null" from "the predicate is + # false" through it means relying on the same exit-status subtlety that already bit this + # workflow once at jq 1.6 (ersatztv#647). A `case` over `jq -r 'type'` cannot be read + # two ways. Anything that is neither `null` nor `array` is still "cannot tell". + kind=$(printf '%s' "$raw" | jq -r 'type' 2>/dev/null) || kind="" + # `null` counts as exhaustion only from page 2 ON. THE INVARIANT, not a figure that + # rots: a real PR's timeline always carries at least one event on page 1 (it is created + # by a push, which is itself an event), so a `null` FIRST page is anomalous rather than + # empty. Spot-checked across #752/#753/#749/#739/#717, all non-empty; the counts + # themselves are deliberately not recorded here because timelines grow and an earlier + # version of this comment cited five numbers of which three were stale within days. + # Trusting a zero count from an anomalous first page would + # mean trusting that no retarget happened on the strength of a response we cannot + # explain. Requiring one real page keeps the property the walk is for: something was + # actually read. A PR that somehow has an empty first page falls through to `rt_ok=no`, + # 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. + case "$kind" in + null) + if [ "$page" -gt 1 ]; then rt_ok=yes; rt_count=$total; 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 @@ -775,9 +946,21 @@ jobs: # live head), `/statuses/{sha}` a BARE ARRAY (24 rows on the same head) — hence the different # `type == "array"` guard here. # - # No claim is made about the order rows come back in, because the check does not depend on - # it: it selects by id against the high-water mark rather than inspecting the top of the - # list. A verdict older than the mark is invisible to it no matter where it sits. + # ORDER IS NOT RELIED ON *WITHIN* A PAGE — the check selects by id against the high-water + # mark rather than inspecting the top of the list, so a verdict older than the mark is + # invisible to it no matter where it sits. But this read is ONE PAGE, and `limit=100` clamps + # to the server-wide `MAX_RESPONSE_ITEMS`, measured at 50 on this instance (ersatztv#751). + # An earlier version of this comment claimed order-independence flatly; that is false the + # moment a head carries more rows than the clamp, because a raced row can then sit on a page + # this never reads. Measured: a probe head reached 33 rows after ~5 runs, and the ordering is + # only coarsely newest-first (ids came back `33,32,31,30,28,29,27,…`), so >50 is ordinarily + # reachable on a PR with a few CI reruns. + # + # That matters more here than anywhere else in this job, because this is the ONE path whose + # failure direction is toward SUCCESS: missing a raced human `failure` leaves a forged green + # standing over a rejection. So the page-2 probe below treats "there are rows I did not read" + # as "assume raced" — the conservative direction — rather than as "no race found". + # Full paging of this endpoint, including for the high-water mark above, is ersatztv#763. # # The repair is `pending`, NEVER a copy of the human's state. Re-posting their `failure` # would attribute a human verdict to this job — the exact provenance confusion the @@ -822,6 +1005,39 @@ jobs: and (((.description // "") | startswith("Review-verdict:")))) or ((.creator == null) and ((.description // "") == $rd)) )] | length') + # "NO RACE FOUND ON PAGE 1" IS NOT "NO RACE" (ersatztv#751). If page 1 was not the whole + # list, a raced row can be beyond it, so an unread page is treated as a race rather than + # as absence. Only checked when page 1 looked clean — a race already found needs no + # further evidence. Unreadable page 2 also counts as raced: this is the fail-toward- + # SUCCESS path, so uncertainty must resolve to `pending`, never to leaving green. + # + # BE HONEST ABOUT THE COST: a repair here is STICKY, not a one-run stall. It writes + # `$REPAIR_DESC`, and the classification above refuses to grant an exemption over that + # sentinel and re-writes it as a fixed point on every later run — so a SPURIOUS repair + # (a head with more status rows than the page cap and no actual race) removes that head's + # exemption permanently, and only a human `post-review-verdict.sh` clears it. That is + # still the right direction, because the alternative is a forged green over a human + # rejection. But it is a per-sha loss of the exemption, not an inconvenience, which is + # the argument for replacing this with real paging (#763) rather than living with it. + if [ "$raced" = "0" ]; then + more_hist=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA?limit=100&page=2") || more_hist="UNREADABLE" + if [ "$more_hist" = "UNREADABLE" ]; then + echo "::warning::Could not read page 2 of the status history for ${SHA:0:7}; treating this as a possible raced verdict and repairing to pending rather than leaving an exemption green on an unverified head." + raced=1 + else + mh_kind=$(printf '%s' "$more_hist" | jq -r 'type' 2>/dev/null) || mh_kind="" + mh_len=$(printf '%s' "$more_hist" | jq -r 'if type == "array" then length else 0 end' 2>/dev/null) || mh_len="" + case "${mh_kind}:${mh_len}" in + null:*|array:0) ;; + array:*) + echo "::warning::The status history for ${SHA:0:7} runs past page 1 (${mh_len} more row(s)), so a raced human verdict could be on a page this job did not read. Repairing to pending rather than leaving the exemption green." + raced=1 ;; + *) + echo "::warning::Page 2 of the status history for ${SHA:0:7} was '${mh_kind:-unparseable}'; treating as a possible raced verdict and repairing to pending." + raced=1 ;; + esac + fi + fi case "$raced" in ''|*[!0-9]*) echo "::warning::Post-write verification for ${SHA:0:7} returned '${raced}' instead of a count; not acting on it." @@ -855,3 +1071,48 @@ jobs: if [ "$state" = "pending" ]; then echo "::notice::This PR needs an H10 review verdict for head ${SHA:0:7} before it can merge. After reviewing, run: scripts/post-review-verdict.sh ${PR} MERGEABLE" fi + + # THE SILENT-GREEN HALF OF ersatztv#751, WHICH IS THE ACTUAL DEFECT. The stray expression + # delimiter was one bug in one comment; this step exists because of how the runner REPORTED it. + # A step it declines to interpolate is dropped and the job concludes `success`, so the gate was + # dead for three days behind a green tick. Nothing surfaced it: the workflow's own status + # context said success, and `review-verdict/h10` was merely ABSENT — which reads as "not + # reviewed yet" on a normal PR and is indistinguishable from the correct pending state. + # + # This turns that class of failure — dropped for ANY reason, not only an interpolation failure + # — into a red job. It asserts execution STARTED, never that it completed: the classifier has + # several legitimate `exit 0` abstention paths (a human verdict landed mid-run, a mid-run + # retarget, a sentinel appearing) and treating those as failures would redden ordinary PRs. + # + # `if: always()` so it also runs when the classifier failed on purpose — its fail-closed paths + # exit non-zero, the job is already red, and this step then finds the marker and stays quiet + # rather than piling a misleading second error on top. + # + # ITS OWN BODY MUST STAY EXPRESSION-FREE. A guard that can be dropped by the very mechanism it + # guards against is worse than none, because its absence is also silent. No delimiters here, no + # illustrative snippets, and `always()` is written bare as `if:` requires — the static test in + # scripts/tests/test_pr_changed_files.py holds this to it. + # + # THAT A LATER STEP STILL RUNS AFTER AN EARLIER ONE IS DROPPED IS MEASURED, not assumed — it is + # the premise this guard stands on, and the #751 report could not settle it because the + # classifier was the job's last step, leaving nothing subsequent to observe. Established on this + # instance by a scratch-base probe with a negative control (Gitea 1.27.1, 2026-08-06): run 1863 + # dropped the classifier on a reintroduced bad payload, logged `evaluating expression 'always()' + # -> true`, ran THIS step, and the job concluded `failure`. Run 1866 is the positive control — + # the classifier ran, posted its exemption, and this step found the marker at + # `/tmp/h10-classifier-started-1866-1`, confirming the run-keyed path resolves. + # + # Had the runner dropped the remaining steps too, this guard could not work and the body would + # have had to move into `scripts/`, where a one-line `run:` makes the class unreachable. It does + # not, so it stays here. Re-measure if the runner is upgraded: this is the one assumption whose + # failure is silent again. + - name: Assert the classifier actually executed + if: always() + run: | + set -euo pipefail + marker="${RUNNER_TEMP:-${GITHUB_WORKSPACE:-/tmp}}/h10-classifier-started-${GITHUB_RUN_ID:-norunid}-${GITHUB_RUN_ATTEMPT:-1}" + if [ ! -f "$marker" ]; then + echo "::error::The classify step did not execute AT ALL — no start marker at ${marker}. The runner dropped it (an interpolation failure over the run: body does this and still reports the job green; see ersatztv#751) or it was skipped. review-verdict/h10 has NOT been posted for this head, so exempt PRs (Renovate, docs-only) are silently unmergeable. Failing the job so this is visible instead of green." + exit 1 + fi + echo "The classify step executed (start marker present at ${marker})." diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 4d9e18439..83cd4732e 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1002,7 +1002,149 @@ trust the editing PR's own checks. Verify the way ersatztv#672 did: branches. The same shape is what makes a `branches:`/`types:` change verifiable at all, since neither can be -observed from the editing PR. +observed from the editing PR. Note step 2 requires the scratch **base**'s own `branches:` filter to +name that base — the definition comes from the base, so a base the filter does not admit produces no +run at all. + +⚠️ **Never write an expression delimiter inside a `run:` body here — a comment is NOT inert** +(ersatztv#751, `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 so the result can be spliced back in. That +rewrite is all-or-nothing: 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`**. + +That is not hypothetical. From 8f6d4f443 (2026-08-03) to 2026-08-06 the classify step **never ran**. +The #706 note above, explaining why a concurrency group does not work here, quoted a `concurrency:` +snippet containing a PR-number expression *as an illustration*, in a shell comment. `pr number` is not +a valid expression. So `review-verdict/h10` was posted by nothing but a human hand for three days, +both exemption classes silently stopped working, and every run reported success. The prose documenting +a fix disabled the fix. + +**The silent green is the real defect.** An absent required status reads as "not reviewed yet", which +is indistinguishable from the correct pending state — so an ordinary PR looked ordinary while the gate +was dead, and the cost landed only where no human was in the loop. PR #739 (docs-only) merged +2026-08-05 with **zero** commit statuses on its head, and got in only because admin force-merge was +still enabled; ersatztv#743 removed that escape the next day, so a docs-only or Renovate-manifest PR +arriving after that would simply have been stuck with no bypass. The two Renovate PRs in the window +escaped by timing, merging minutes before the bad commit. + +Three things now hold the line, and they are deliberately different in kind: + +- **The prose names expressions instead of quoting them** — write "a + `github.event.pull_request.number` expression", not the delimiters. Pass values in through the + step's `env:` block, which is interpolated per value, so a bad payload there cannot take the body + with it. +- **A start-marker guard turns a dropped step RED.** The classifier writes a marker as its first act + and an `if: always()` step fails the job when it is missing. It asserts execution *started*, never + that it completed — the classifier has several legitimate `exit 0` abstention paths. The guard's own + body must stay expression-free, or the mechanism it guards against can delete the guard too, and + that absence would be silent as well. +- **Two static guards**, in `scripts/tests/test_pr_changed_files.py`: no delimiter in *any* `run:` + body of this file (absolute — a dropped step here is a dead merge gate, and its bodies are ~700 + lines of prose), and repo-wide, every expression payload's **head token** must name a context or + function the runner can resolve (permissive, because the other workflows interpolate into `run:` + legitimately — 7 occurrences today). Be precise about the second one's reach: it catches the + historical defect (`pr number`) and a nonexistent context, but **not** a syntactically invalid + payload whose tokens are all known (`${{ github.ref == }}` passes), nor a renamed output + (`steps.metadata.outputs.shortsha` passes — every token after the first is preceded by `.` and is + skipped), nor an unclosed opener. Catching those needs an expression parser. An earlier draft of + this section claimed it caught "a payload that cannot evaluate, wherever it sits"; that was false, + and the corrected claim is the one to rely on. + +Worth knowing why nothing caught this for three days: every *other* workflow-shape test in that file +reads `_code_lines()`, which strips comments. That is correct for what it was for, but it encodes the +assumption this bug falsifies. The strict test reads the raw scalar, and must never adopt +`_code_lines`. + +⚠️ **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*: +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 +the interpolation alone would not have restored the exemptions. + +Two things kept it invisible, and both are worth generalising: + +- It shipped in the **same commit** (8f6d4f443) that stopped the step executing, so the fence had + never once run in production. A guard's first real execution is not the same event as its merge. +- The **test double asserted the wrong shape while claiming measured fidelity.** Its comment read + "Real shapes, measured on this instance and deliberately mirrored" and it printed `[]` for a page + past the end. Every fence test was green against a response the server never produces, so the + `array`-only gate was never exercised by the suite either. With the double corrected and the old + gate restored, **most of the fence suite fails** — 18 tests when first measured at `c710db4a1`, 21 + once three more fence-dependent tests existed. The invariant is the point, not the count: they had + all been passing for the wrong reason. (Given as a range on purpose — an earlier draft cited a bare + "18", which was stale two commits later, inside a section about stale claims.) When a double claims + fidelity, that claim is a test assertion and needs re-measuring like any other. + +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 +(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. + +**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 +`{"state":"pending","total_count":0,"statuses":null}` (measured on PR #739's head). `read_existing_verdict` +gated on `.statuses | type == "array"`, so it hit its `exit 1` and posted nothing at all — fail-closed, +same user-visible outcome. `null` is now accepted there only when `total_count` is 0, so a body that +merely lost its array is still refused and an existing verdict is still protected from a transient +error. `scripts/pr-changed-files.sh` was swept and is unaffected (`pulls/{n}/files` returns `[]`). +**The generalisable rule: a nil Go slice serialises to `null`, so every list-shaped field on this API +is suspect and only a per-endpoint measurement settles it.** + +**Establishing that "no verdict exists" needs a second page, and both arithmetic guards for it are +no-ops here.** `read_existing_verdict` concluding absence is what licenses posting an exemption over a +verdict the job cannot see, so that conclusion has to be earned. Two obvious checks were tried and both +proved empty: + +- **`.statuses | length` vs `.total_count`** — `total_count` is the count for the **page returned**, not + for the commit. Measured at 1.27.1 on `3aed43c6` (6 contexts): `?limit=1` returns + `len=1, total_count=1`, `?limit=3` returns `len=3, total_count=3`. Equal by construction, so the check + reads as a completeness proof while proving nothing. +- **"refuse when the page comes back full at the requested `limit=100`"** — this instance caps `limit` + at the server-wide `MAX_RESPONSE_ITEMS`, **measured at 50** (`/issues?limit=100` returns 50). A + response can therefore never carry 100 rows, and the comparison was **dead code**. The repo already + documented that cap in `scripts/pr-changed-files.sh`, two test files and `ci.script-tests-job`; the + guard was written against 100 anyway, and a cold review caught it. Hardcoding 50 instead would + re-break the day the setting changes. + +So the job **asks the server, and only when it matters**: if the `review-verdict/h10` row is on page 1 +there is nothing further to learn (this endpoint returns the latest status per *context*, and a context +cannot recur on a later page). When the row is absent it reads **page 2** — any rows there mean the list +runs longer than one page and a verdict could be beyond it, so it refuses instead of concluding absence. +Cap-independent by construction. Paging is real here: measured `?limit=3&page=2` returning three further +rows, and `page=9` returning the same `statuses: null` terminator. + +The `total_count` zero-check also requires the JSON **type** to be a number: `jq -r` renders `0` and +`"0"` identically, so a text compare would accept a schema-corrupted `"total_count": "0"` as "no +statuses". + +**The repo-wide expression guard scans PARSED scalars, not raw file text.** A delimiter in an ordinary +top-level YAML comment is inert — the runner never evaluates it — so redding on it is a false positive, +and this file has now produced that false red twice. PyYAML drops those comments. A `run:` body is +itself a scalar and keeps its *shell* comments, which is the point: inside a `run:` scalar a comment is +not inert. Verified both directions by mutation — an inert top-level comment passes; the same payload +in a run-body comment still reds. + +**`CLAUDE.md` and `AGENTS.md` are now PROTECTED paths.** `DOCS_ONLY` matched them, so the documents +that *define* the completion protocol, the merge-consent convention and the H10 rule were themselves +docs-only-exemptible while `.claude/` was protected — the same self-exemption the gate rules out, one +directory over. Driving the real classify body with a lone `CLAUDE.md` change produced +`review-verdict/h10=success`. It is fixed here rather than deferred because restoring the exemptions is +what makes it reachable: no exemption `success` was writable at all while the classify step was +dropped. `README.md` is deliberately not listed — ordinary prose, no enforcement. For the same reason, +#706's known residual returns with the working fence: while `rt_ok` was never `yes`, route 1 was closed +by accident. + +**The guard is scoped to this file, and that is a gap rather than a decision** — `docker-build.yml`'s +`test` and `migrations` jobs are also required contexts, and there a dropped step is **fail-OPEN**: the +required check goes green having done no work, which is strictly worse than an absent status (compare +#684). Tracked as ersatztv#756. It lives in its **own workflow file** on purpose: `pr-checks.yml` sets `cancel-in-progress: true`, and a cancelled run there would leave an exempt PR with no status and no further push to diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 8ebe9f9d2..dfab343f5 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -44,7 +44,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.decisions-lifecycle-flake` | When `decisions lifecycle` is the **only** red job, do not investigate and do not create a new run to clear it — no rebase, no `--amend`, no no-op push; the operator reruns that single job from the Gitea UI. | 2026-07-21 | [link](records/ci/decisions-lifecycle-flake.md) | | `ci.docs-only-detect-shallow-safe` | The docs-only detect script must diff against `FETCH_HEAD` (always resolves after `git fetch`, even shallow) using a two-dot tree diff — not `origin/` with three-dot — because a `fetch-depth: 1` shallow clone has no remote-tracking ref and no merge-base, which silently fails the original detect into `docs_only=false` (full matrix, no functional error). A CI-behavior change must be verified by measuring the effect (job durations), not just a green check. | 2026-07-17 | [link](records/ci/docs-only-detect-shallow-safe.md) | | `ci.docs-only-skip-steps` | A docs-only change must still run every required job (`test`, `migrations`) so their commit-status contexts always report; each heavy job runs `scripts/ci-detect-docs-only.sh` first and gates its real STEPS on `if: steps.detect.outputs.docs_only != 'true'`, never `if:`-skips the whole job (an `if:`-skipped job reports `skipped`, not `success`, which branch protection may never unblock on). Detection biases toward running more on any doubt. | 2026-07-17 | [link](records/ci/docs-only-skip-steps.md) | -| `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict for THIS base, meaning a non-null `.creator.login` AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR's — tested by requiring the description to END with the exact literal `(base: )` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow's `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one. That residual is now FENCED rather than merely tracked — the job refuses to write at all if the PR's timeline retarget COUNT moved while it was classifying (`ci.verdict-write-retarget-fence`, #706) — leaving only the sub-round-trip window that no API without compare-and-set can close. The PROTECTED path list additionally covers `.codex/` (#711), which mirrors `.claude/hooks/` byte for byte including the merge-consent hook — latent while that directory is untracked, live the moment it is tracked; the list stays ENUMERATIVE rather than derived, because a derived rule would have to be evaluated against the very file list being classified. Path predicates are evaluated by COUNTING with `grep -c`, never `\| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`. | 2026-07-29 | [link](records/ci/exemption-provenance.md) | +| `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict for THIS base, meaning a non-null `.creator.login` AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR's — tested by requiring the description to END with the exact literal `(base: )` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow's `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one. That residual is now FENCED rather than merely tracked — the job refuses to write at all if the PR's timeline retarget COUNT moved while it was classifying (`ci.verdict-write-retarget-fence`, #706) — leaving only the sub-round-trip window that no API without compare-and-set can close. The PROTECTED path list additionally covers `CLAUDE.md` and `AGENTS.md` (#751) — they are not prose but the documents DEFINING the completion protocol, the merge-consent convention and the H10 rule, so protecting `.claude/` while the file specifying what it enforces stayed docs-only-exempt was the same self-exemption one directory over; driving the real classify body with a lone `CLAUDE.md` change produced an exemption `success`. `README.md` is deliberately not listed. It also covers `.codex/` (#711), which mirrors `.claude/hooks/` byte for byte including the merge-consent hook — latent while that directory is untracked, live the moment it is tracked; the list stays ENUMERATIVE rather than derived, because a derived rule would have to be evaluated against the very file list being classified. Reading the CURRENT status for input (3) must tolerate `statuses: null`: `GET /commits/{sha}/status` serialises a nil slice as `null`, not `[]`, on a head with no statuses yet, and an `array`-only gate made `read_existing_verdict` `exit 1` and post nothing at all (#751, `ci.workflow-run-body-no-expressions`) — `null` is accepted only when `total_count` is 0, so a body that merely lost its array is still refused. Path predicates are evaluated by COUNTING with `grep -c`, never `\| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`. | 2026-07-29 | [link](records/ci/exemption-provenance.md) | | `ci.format-gate-folder-mode` | The blocking `format` CI job (and matching pre-commit hook) runs `dotnet format whitespace . --folder --include ` instead of loading the full MSBuild/Roslyn solution, cutting the gate from ~480s to ~0.5s with unchanged whitespace/charset coverage. | 2026-07-19 | [link](records/ci/format-gate-folder-mode.md) | | `ci.functional-e2e-harness` | The `functional-e2e` CI job boots the PR's own code from source via `dotnet run` (`scripts/e2e-local.sh`) and runs deterministic assertions (`scripts/e2e-functional.sh`) as an advisory (non-blocking) job, not a `build` dependency or required check. Originally curl-only; since #445 the same job carries a second, headless-browser step for the contracts curl cannot express — see `ci.ui-e2e-harness`. | 2026-07-16 | [link](records/ci/functional-e2e-harness.md) | | `ci.gate-trigger-base-resolved` | The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch's gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree's scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class: Gitea injects a write-capable `GITEA_TOKEN` into EVERY job, so any ref-resolved workflow — and a collaborator's own API token, since branch protection binds the context and not its issuer — can still forge `review-verdict/h10`. The credential half is now RESOLVED in `ci.actions-credential-scoping` (#697): CI's registry secret was the ADMIN account's basic auth and is now a PAT that cannot post a status, which removes the ADMIN escalation and that credential's route (a user credential's forgery carries a real `creator` and is inherited as a human verdict; an Actions job's carries `creator: null` and is re-derived — but do NOT read that asymmetry as protection: re-derivation fires only on the trigger's `types`, and posting a status is not one of them, so a POST timed after the last PR event simply stands). It does not remove EVERY route: `RENOVATE_TOKEN` is a `write:repository` bot PAT in the same secret store, reachable by any PR-added workflow. The injected token stays write-capable until Gitea >=1.26 with a Restricted default (server-management#714), and a collaborator's own token remains unfixable; the exemption path has its own separate defects in #698. | 2026-07-28 | [link](records/ci/gate-trigger-base-resolved.md) | @@ -65,6 +65,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `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. Abstaining is a handoff, not a stall, and that is the property the design rests on: 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 and the last run writes the final answer. `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` cannot turn an unreviewed head green while withholding it would strand ordinary PRs for no safety gain. SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` 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. | 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 is NOT yet a repo-wide property: `docker-build.yml`'s `test` and `migrations` jobs are also required contexts and a dropped step there is fail-OPEN (the required check goes green having done no work), which is strictly worse than here; extending the guard to them is ersatztv#756, not something this record claims is done. | 2026-08-06 | [link](records/ci/workflow-run-body-no-expressions.md) | | `concurrency.diff-scalar-fanout` | The frozen Block optimistic-concurrency recipe (api-conventions §7a) fans out to Collection/Playout×2/MultiCollection/RerunCollection, keeping a guard-returned `PreconditionFailedError` out of any handler's generic `catch(Exception)`→422 mapping, and preserving each aggregate's existing `SaveChangesAsync() > 0` gate semantics under the new unconditional `Version++`. | 2026-07-11 | [link](records/concurrency/diff-scalar-fanout.md) | | `concurrency.etag-rotation-completion` | Every handler that mutates a versioned root's editor-visible config state must bump `Version` (rotating the ETag) with no per-aggregate carve-outs, short-circuiting on a genuine no-op before the bump so idempotent re-submits don't fire spurious rebuild fan-out; `SaveChangesForcingVersion` rebases the retry (stored + pending delta), never adopts the stored token verbatim. | 2026-07-12 | [link](records/concurrency/etag-rotation-completion.md) | | `concurrency.force-write-non-ifmatch` | Any handler that leaves a versioned root `Modified` or `Deleted` but takes no `If-Match` (deletes, item add/remove bumpers, scalar-config writers) must save through `ConcurrencyExtensions.SaveChangesForcingVersion` — force-write past a concurrent `Version` bump rather than throw an unhandled `DbUpdateConcurrencyException` (500). | 2026-07-12 | [link](records/concurrency/force-write-non-ifmatch.md) | diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md index 9e0702b6a..ba85d65bb 100644 --- a/docs/decisions/records/ci/exemption-provenance.md +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -5,7 +5,7 @@ status: active since: '2026-07-29' supersedes: none superseded-by: none -rule: 'The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR''s live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR''s immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict for THIS base, meaning a non-null `.creator.login` AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR''s — tested by requiring the description to END with the exact literal `(base: )` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow''s `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one. That residual is now FENCED rather than merely tracked — the job refuses to write at all if the PR''s timeline retarget COUNT moved while it was classifying (`ci.verdict-write-retarget-fence`, #706) — leaving only the sub-round-trip window that no API without compare-and-set can close. The PROTECTED path list additionally covers `.codex/` (#711), which mirrors `.claude/hooks/` byte for byte including the merge-consent hook — latent while that directory is untracked, live the moment it is tracked; the list stays ENUMERATIVE rather than derived, because a derived rule would have to be evaluated against the very file list being classified. Path predicates are evaluated by COUNTING with `grep -c`, never `| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`.' +rule: 'The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR''s live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR''s immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict for THIS base, meaning a non-null `.creator.login` AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR''s — tested by requiring the description to END with the exact literal `(base: )` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow''s `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one. That residual is now FENCED rather than merely tracked — the job refuses to write at all if the PR''s timeline retarget COUNT moved while it was classifying (`ci.verdict-write-retarget-fence`, #706) — leaving only the sub-round-trip window that no API without compare-and-set can close. The PROTECTED path list additionally covers `CLAUDE.md` and `AGENTS.md` (#751) — they are not prose but the documents DEFINING the completion protocol, the merge-consent convention and the H10 rule, so protecting `.claude/` while the file specifying what it enforces stayed docs-only-exempt was the same self-exemption one directory over; driving the real classify body with a lone `CLAUDE.md` change produced an exemption `success`. `README.md` is deliberately not listed. It also covers `.codex/` (#711), which mirrors `.claude/hooks/` byte for byte including the merge-consent hook — latent while that directory is untracked, live the moment it is tracked; the list stays ENUMERATIVE rather than derived, because a derived rule would have to be evaluated against the very file list being classified. Reading the CURRENT status for input (3) must tolerate `statuses: null`: `GET /commits/{sha}/status` serialises a nil slice as `null`, not `[]`, on a head with no statuses yet, and an `array`-only gate made `read_existing_verdict` `exit 1` and post nothing at all (#751, `ci.workflow-run-body-no-expressions`) — `null` is accepted only when `total_count` is 0, so a body that merely lost its array is still refused. Path predicates are evaluated by COUNTING with `grep -c`, never `| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`.' signals: 'forged review-verdict exemption, retarget race against the docs-only classifier, PR base changed mid-run, hijacked Renovate branch, bot exemption on a code change, machine-written success inherited as a verdict, status creator null vs user, never overwrite a human verdict, exemption chain skips docs-only for bots, why is my Renovate PR asking for a verdict, base ref binding on pr-changed-files.sh · paths: `.gitea/workflows/review-verdict.yml`, `scripts/pr-changed-files.sh`, `.claude/hooks/pretooluse-merge-consent.sh`, `scripts/tests/test_pr_changed_files.py` · issues: #698, #697, #672, #663, #649, #632' mechanics: '`scripts/pr-changed-files.sh ` (5 args; a 4-arg call exits 2); workflow env `BASE_REF: ${{ github.event.pull_request.base.ref }}`; `BOT_MANIFESTS` anchored allow-list; short-circuit requires `.creator.login` non-null AND description matching `^Review-verdict:`; `types: [opened, reopened, synchronize, ready_for_review, edited]`' --- diff --git a/docs/decisions/records/ci/verdict-write-retarget-fence.md b/docs/decisions/records/ci/verdict-write-retarget-fence.md index 2bf2f38fb..ef95fcbe1 100644 --- a/docs/decisions/records/ci/verdict-write-retarget-fence.md +++ b/docs/decisions/records/ci/verdict-write-retarget-fence.md @@ -7,7 +7,7 @@ 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. Abstaining is a handoff, not a stall, and that is the property the design rests on: 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 and the last run writes the final answer. `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` cannot turn an unreviewed head green while withholding it would strand ordinary PRs for no safety gain. SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` 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.' 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; `retargets_before`/`retargets_before_ok` captured before enumeration, re-counted immediately before the POST; `max_id_before` from `GET /repos/{repo}/statuses/{sha}` (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`' +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; `max_id_before` from `GET /repos/{repo}/statuses/{sha}` (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`' --- `ci.exemption-provenance` closed three routes into the exemption path and left one residual it named: @@ -70,3 +70,31 @@ marker, so the exemption returned two events later instead of one. Only a re-pos 4. **A timeline over the 20-page cap can never be exempted** — `rt_ok` stays `no` on every run, so only a human verdict clears it and comment-flooding becomes a fail-closed denial of exemption. Negligible at 1000 events; the log says so rather than promising a later run will fix it. + +**CORRECTION, 2026-08-06 (ersatztv#751).** Residual 4 above described as a narrow edge case what was +in fact the universal behaviour: `rt_ok` stayed `no` on **every** pull request, not only over-cap ones, +so the fence withheld **every** exemption `success` from the day it shipped. A page past the end of +this endpoint is the JSON value `null`, not `[]` (measured at Gitea 1.27.1 on PR #752; the same +instance returns `[]` for an empty `/issues/{n}/comments`, so it is not consistent between endpoints). +`count_retargets` gated on `type == "array"` and therefore read the real terminator as unreadable, +never reaching the validated empty page it required. Renovate and docs-only PRs got no status at all. + +Two reasons it read as deliberate rather than broken, both worth carrying forward: + +- **It never ran.** This fence shipped in 8f6d4f443 — the same commit whose prose comment stopped the + classify step from executing at all (`ci.workflow-run-body-no-expressions`). Merging a guard and + first executing it are different events, and only the second tells you anything. +- **The double asserted the wrong shape while claiming to be measured.** The stub's comment read "Real + shapes, measured on this instance and deliberately mirrored" and it printed `[]` past the end. So the + `array`-only gate was never exercised by the suite either. Correcting the double and restoring the + old gate reddens most of the fence suite — 18 tests when first measured, 21 once three more + fence-dependent tests existed. The invariant, not the number, is that every one of them had been + green for the wrong reason. A fidelity claim in a test double is an assertion, and it decays like any + other. + +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 at jq 1.6 (`ci.jq-version-contract`), and both `null` +and `[]` terminate the walk. `test_the_fence_TRUSTS_the_count_and_POSTS_when_the_timeline_terminates` +is parameterised over both shapes and asserts the POSTED STATUS rather than the log line — on the real +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. diff --git a/docs/decisions/records/ci/workflow-run-body-no-expressions.md b/docs/decisions/records/ci/workflow-run-body-no-expressions.md new file mode 100644 index 000000000..f7e65732f --- /dev/null +++ b/docs/decisions/records/ci/workflow-run-body-no-expressions.md @@ -0,0 +1,94 @@ +--- +key: ci.workflow-run-body-no-expressions +title: '2026-08-06 — an expression delimiter anywhere in a `run:` body, INCLUDING in a comment, silently drops the step and reports the job green (#751)' +status: active +since: '2026-08-06' +supersedes: none +superseded-by: none +rule: '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 is NOT yet a repo-wide property: `docker-build.yml`''s `test` and `migrations` jobs are also required contexts and a dropped step there is fail-OPEN (the required check goes green having done no work), which is strictly worse than here; extending the guard to them is ersatztv#756, not something this record claims is done.' +signals: 'Unable to interpolate expression format(, step never ran but job green, missing Run Main step marker, review-verdict/h10 absent after a green run, docs-only PR unmergeable, Renovate PR unmergeable, exemption stopped working, expression in a shell comment, workflow comment changed behaviour · paths: `.gitea/workflows/review-verdict.yml`, `scripts/tests/test_pr_changed_files.py` · issues: #751, #706, #748' +mechanics: '`RAN_MARKER` written at the top of the classify step and asserted by the `Assert the classifier actually executed` step (`if: always()`, expression-free body, `exit 1` on a missing marker); static guards `test_the_verdict_workflow_has_NO_expression_delimiter_in_any_run_body` (raw scalar, absolute, gate file only) and `test_every_workflow_expression_names_a_REAL_context_or_function` (repo-wide, allow-list of contexts/functions) plus `test_a_dropped_classify_step_FAILS_the_job_instead_of_going_green` (pins marker path agreement and guard ordering across ONE yaml parse)' +--- + +**How it happened, which is the part that generalises.** The #706 note explaining why a concurrency +group does not work in `review-verdict.yml` quoted a `concurrency:` snippet containing a PR-number +expression *as an illustration*, inside a shell comment. `pr number` is not a valid expression. From +8f6d4f443 (2026-08-03) to 2026-08-06 the classify step therefore never ran, `review-verdict/h10` was +posted by nothing but a human hand, and both exemption classes silently stopped working — while every +run reported success. The prose documenting a fix disabled the fix. + +**Why nothing caught it.** Every pre-existing workflow-shape test in +`scripts/tests/test_pr_changed_files.py` reads `_code_lines()`, which strips comment lines. That is +correct for what it was for — its own docstring notes that prose legitimately discusses +`pulls/N/files`, and a raw scan would redden the repo over a piece of writing — but it encodes the +assumption this bug falsifies: that a comment in a workflow cannot change behaviour. Inside a `run:` +scalar it can. The strict test added here reads the RAW scalar for exactly that reason and must never +adopt `_code_lines`. + +**The silent green is the defect; the delimiter was only the trigger.** An absent required status +reads as "not reviewed yet" on an ordinary PR, which is indistinguishable from the correct pending +state — so a normal PR looked normal while the gate was dead. The visible cost landed on the two +classes with no human in the loop: PR #739 (docs-only) merged 2026-08-05 with ZERO commit statuses on +its head, and got in only because admin force-merge was still enabled. #743 removed that escape the +next day, so by the time this was found the workaround that had been absorbing the bug was gone and +the next docs-only or Renovate-manifest PR would have been permanently stuck. The two Renovate PRs in +the window escaped by timing alone, merging minutes before the bad commit landed. + +**Scope of the strict rule, and why it is not repo-wide.** `docker-build.yml`, `ci-image.yml` and +`pr-checks.yml` interpolate into `run:` bodies legitimately (7 occurrences). A repo-wide ban would be +false and would be deleted the first time it got in someone's way. `review-verdict.yml` earns the +absolute rule on two counts: it writes the branch-protection-required status, and its `run:` bodies +are ~700 lines of dense prose — the only place the delimiter has ever appeared by accident. + +**The probe found a SECOND, independent reason the gate posted nothing**, and it is why fixing the +interpolation alone would not have restored the exemptions: a page past the end of +`/issues/{n}/timeline` is JSON `null`, not `[]`, so the retarget fence never trusted its count for ANY +PR and withheld every exemption `success`. Corrected in `ci.verdict-write-retarget-fence`, whose stated +residual had described that universal behaviour as a narrow over-cap edge case. Both defects shipped in +the same commit, which is the general lesson: a guard that has never executed has told you nothing, and +merging it is not executing it. + +**A THIRD instance of the same server behaviour was found by cold review of this fix**, and it is +the reason to distrust "I fixed the two I could see". `GET /commits/{sha}/status` also returns +`statuses: null` — not `[]` — for a head with no statuses yet (measured on PR #739's head 5fa672e2: +`{"state":"pending","total_count":0,"statuses":null}`). `read_existing_verdict` gated on +`.statuses | type == "array"` and took its `exit 1` path, posting nothing: fail-closed, but the same +user-visible outcome again. Its double printed `{"statuses": []}` at all three no-verdict sites, so +that branch was unreachable in the suite; correcting the double and restoring the old gate turns 40+ +tests red. `scripts/pr-changed-files.sh` was swept too and is unaffected — `pulls/{n}/files` returns +`[]`. The generalisable rule is that a nil Go slice serialises to `null`, so EVERY list-shaped field +on this API is suspect, and a per-endpoint measurement is the only way to know. + +**Restoring the exemptions restores a hole that had been dead**, and this is worth saying rather than +presenting the change as pure repair. `DOCS_ONLY` matched `CLAUDE.md` and `AGENTS.md`, the documents +that define the completion protocol and the H10 rule itself — so those were auto-exemptible while +`.claude/` was protected, which is the same self-exemption the workflow header rules out, one +directory over. Reachable only because exemptions work again, hence fixed here (both added to +`PROTECTED`; see `ci.exemption-provenance`). For the same reason, #706's known residual — the +sub-round-trip ABA window, "narrowed and observable, not closed" — comes back with the working fence: +while `rt_ok` was never `yes`, route 1 was closed by accident. + +**Three guards were proposed or written for the same hole and the first two were no-ops** — the hole +being that concluding "no verdict exists" is what licenses posting over one. `total_count` is per-PAGE +here (`?limit=1` on a 6-context head gives `len=1, total_count=1`), so length-vs-total is equal by +construction; and "refuse on a full page at `limit=100`" was DEAD CODE, because the instance caps +`limit` at `MAX_RESPONSE_ITEMS`, measured at 50 — a cap this repo already documented in three places +before the guard was written against 100. The working version asks the server: read page 2 when the row +is absent from page 1, and refuse if it carries anything. Cap-independent, so no reconfiguration +re-breaks it. Second, `jq -r` renders the number `0` and the string `"0"` identically, so the zero +check requires the JSON type as well. Neither was a live failure — both are the difference between a +guard that holds because the input happens to be well-formed and one that holds because it checks. + +**The tests written to close a review finding then needed closing themselves**, which is the honest +shape of work on this file. The behavioural guard test first extracted the two marker lines by text and +ran them alone — which passes even if the write is moved into a function nobody calls. It now executes +the classify body's real PREFIX down to the write, reproducing the production control flow instead of a +reconstruction of it. The anti-vacuity check first hand-counted `run:` keys with a regex, which +false-redded legal spellings (`- run: |`, a single-line `run: echo ok`) and could count a `run: |` +inside a heredoc; hand-parsing YAML to validate a YAML parse is the wrong shape, so it now asserts on +content — the walk reached at least three bodies and one over 5000 characters. + +**Verified by mutation, not by a green suite.** All six mutations produce a red and the restored tree +is green: reintroducing the exact defect (caught by both the strict and the general test), deleting +the guard step, deleting only the marker write, weakening `if: always()`, turning the guard's +`exit 1` into `exit 0`, and putting a delimiter in the guard's own body. diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index b97bced4a..eed2be30d 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -911,25 +911,38 @@ if "-X" in args and args[args.index("-X") + 1] == "POST": # edit to either pattern cannot silently reroute one endpoint into the other's handler. # # Real shapes, measured on this instance and deliberately mirrored, because the job's guards are -# type-sensitive: this endpoint returns a BARE ARRAY of events, each with a `type`; a retarget is +# type-sensitive: a NON-EMPTY page is a bare array of events, each with a `type`, and a retarget is # `change_target_branch` (confirmed on PR #703, the route-1 reproduction, which carries exactly two; -# and on PR #717, never retargeted, which carries none). +# and on PR #717, never retargeted, which carries none). A page PAST THE END is the JSON value `null` +# — NOT an empty array (measured again at 1.27.1 on PR #752, ersatztv#751). The earlier version of +# this comment claimed measured fidelity while the terminator below printed `[]`; that discrepancy is +# why the fence's type gate was never exercised and shipped rejecting every real timeline. if "/timeline" in url: mode = os.environ.get("STUB_TIMELINE_MODE", "none") page = 1 for part in url.split("?", 1)[-1].split("&"): if part.startswith("page="): page = int(part.split("=", 1)[1]) + if mode == "empty-first-page": + # A terminator on PAGE 1 — no real page ever read (ersatztv#751). The job must NOT certify a + # zero retarget count from this, so the exemption is withheld. + print(os.environ.get("STUB_TIMELINE_TERMINATOR", "null")) + sys.exit(0) if mode == "unreadable": print("502 Bad Gateway") sys.exit(0) if mode == "transport-error": sys.exit(22) - # Page 2+ is always the validated EMPTY page that terminates the walk. Without a real terminator - # the job would page to its cap and refuse to trust the count, which is a different branch from - # the one most of these tests mean to exercise. + # 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: - print("[]") + print(os.environ.get("STUB_TIMELINE_TERMINATOR", "null")) sys.exit(0) n_before, n_after = 0, 0 if mode.startswith("stable:"): @@ -956,6 +969,47 @@ if "/timeline" in url: # endpoint no longer shows. if "/statuses/" in url: mode = os.environ.get("STUB_HISTORY_MODE", "none") + + # PAGE, honoured. THE REASON IS THE TERMINATOR, NOT THE COUNTERS — and that distinction was itself + # a finding (#751 round 5). An earlier version of this comment said the guard sits ahead of the + # read-counting modes "so the page-2 probe cannot shift 'raced row appears on read N'", by analogy + # with the combined endpoint. Measured: moving this guard AFTER the counter modes reddens NOTHING, + # because no history mode that counts reads ever issues a page-2 request — `raced=1` on page 1 + # short-circuits the probe. So that half of the rationale read as a checked reason and was not, + # which is the precise class this branch exists to retire. + # + # What IS load-bearing: page 2 must terminate for every mode that describes page 1 only, or the job + # re-reads page 1's rows as a second page and concludes the list is longer than it is. Dropping + # just the `print("[]")` below reddens `test_a_PRE_EXISTING_human_row_does_NOT_trigger_a_repair`. + # (The ordering guard on the COMBINED endpoint is a different story and genuinely is counter- + # related — moving that one reddens three mid-run-race tests.) + # + # PAST THE END THIS ENDPOINT RETURNS `[]`, NOT `null` — measured at 1.27.1: + # `/statuses/{sha}?limit=100&page=99` is the two bytes `[]`, while `/commits/{sha}/status` past the + # end returns `{"statuses": null}`. A THIRD distinct empty shape on one server; the job tolerates + # both here precisely because guessing per endpoint has been wrong twice. + hist_page = 1 + for part in url.split("?", 1)[-1].split("&"): + if part.startswith("page="): + try: + hist_page = int(part.split("=", 1)[1]) + except ValueError: + hist_page = 1 + if hist_page > 1 and mode == "second-page-garbage": + print("502 Bad Gateway") + sys.exit(0) + if hist_page > 1 and mode == "second-page-error": + sys.exit(22) + if hist_page > 1 and mode not in ("second-page", "second-page-garbage", "second-page-error"): + print("[]") + sys.exit(0) + if mode == "second-page": + # The history runs past page 1, so "no raced row on page 1" does not establish "no race". The + # rows are deliberately ORDINARY (no verdict, no sentinel): the point is that unread rows EXIST, + # not that a verdict was found, so the repair must fire on uncertainty alone. + print(json.dumps([{"id": 7000 + i, "context": "ci/other", "status": "success", + "creator": None, "description": "unrelated"} for i in range(3)])) + sys.exit(0) rows = [] if mode.startswith("human-after-post"): # The raced verdict: absent when the high-water mark is taken, present afterwards. Its id is @@ -993,6 +1047,47 @@ if "/status" in url: # never-overwrite short-circuit unreachable: neither could be made to fire, so mutations # deleting them survived the whole suite. mode = os.environ.get("STUB_STATUS_MODE", "none") + + # PAGE, honoured — the job now reads page 2 to justify "no verdict exists" (ersatztv#751). The + # real endpoint pages properly: measured at 1.27.1 on a 6-context head, `?limit=3&page=2` returns + # 3 more rows and `page=9` returns the `statuses: null` terminator. Every mode below describes + # page 1 only, so page 2+ must terminate, or the job would read its own page-1 rows again and + # conclude the list is longer than it is. `twopage` is the one mode with a real second page. + status_page = 1 + for part in url.split("?", 1)[-1].split("&"): + if part.startswith("page="): + try: + status_page = int(part.split("=", 1)[1]) + except ValueError: + status_page = 1 + + # THE EMPTY SHAPE IS `statuses: null`, NOT `[]` (ersatztv#751). Measured on this instance: a head + # with no statuses returns `{"state":"pending","total_count":0,"statuses":null}` — PR #739's head + # 5fa672e2. This stub printed `{"statuses": []}` at all three no-verdict sites, which is a shape + # the server does not produce for that case, so the job's `.statuses | type == "array"` gate was + # never exercised against reality and its `exit 1` branch — posting nothing at all — was + # unreachable in the suite. Same class as the timeline terminator, one function over. + # Parameterised, not merely corrected: a head that HAS statuses really does return an array, so + # both shapes are live and the job must read either. + def empty_statuses(): + if os.environ.get("STUB_STATUS_EMPTY_SHAPE", "null") == "array": + return json.dumps({"state": "pending", "total_count": 0, "statuses": []}) + return json.dumps({"state": "pending", "total_count": 0, "statuses": None}) + + # BEFORE ANY READ-COUNTING MODE. The `appears-on-read:N` modes count how many times the job has + # LOOKED at the combined status, and the page-2 completeness probe is part of the same look, not a + # further one — letting it increment those counters shifted "the verdict appears on read N" by one + # and broke three mid-run-race tests. Page 2 also has to terminate here for every mode that + # describes page 1 only, or the job would re-read page 1's rows as a second page and conclude the + # list is longer than it is. + if status_page > 1 and mode == "page2-garbage": + print("502 Bad Gateway") + sys.exit(0) + if status_page > 1 and mode == "page2-error": + sys.exit(22) + if status_page > 1 and mode != "twopage": + print(empty_statuses()) + sys.exit(0) if mode.startswith("appears-on-read:"): # A human verdict that does NOT exist at the first read and DOES exist at the re-read made # immediately before the POST (ersatztv#706). Models a reviewer posting BLOCKED while the job @@ -1002,7 +1097,7 @@ if "/status" in url: n = int(ctr.read_text()) if ctr.exists() else 0 ctr.write_text(str(n + 1)) if n + 1 < nth: - print(json.dumps({"statuses": []})) + print(empty_statuses()) sys.exit(0) print(json.dumps({"statuses": [ {"context": "review-verdict/h10", "status": "failure", @@ -1017,11 +1112,36 @@ if "/status" in url: n = int(ctr.read_text()) if ctr.exists() else 0 ctr.write_text(str(n + 1)) if n + 1 < nth: - print(json.dumps({"statuses": []})); sys.exit(0) + print(empty_statuses()); sys.exit(0) print(json.dumps({"statuses": [ {"context": "review-verdict/h10", "status": "pending", "creator": None, "description": "Human verdict raced this exemption write — re-post the verdict"}]})) sys.exit(0) + if mode == "twopage": + # Page 1 AND page 2 both carry decoy contexts and NO `review-verdict/h10`: the list is longer + # than one page, so "no verdict exists" is not established and the job must refuse. Models a + # head with more contexts than the server-wide page cap (measured 50 here) — which is exactly + # why the guard cannot be written as a comparison against a hardcoded limit. + print(json.dumps({"state": "pending", "total_count": 3, + "statuses": [{"context": f"ci/p{status_page}x{i}", "status": "pending"} + for i in range(3)]})) + sys.exit(0) + if mode.startswith("rows:"): + # N decoy contexts and NO `review-verdict/h10`, to exercise the truncation guard. `total_count` + # deliberately MIRRORS the page length, because that is what the real endpoint does — it is the + # count for the page returned, not for the commit (measured at 1.27.1: `?limit=1` on a 6-context + # head returns len=1, total_count=1). A stub that reported the true total would make a + # length-vs-total guard look like it worked, which is the trap this models. + n = int(mode.split(":", 1)[1]) + print(json.dumps({"state": "pending", "total_count": n, + "statuses": [{"context": f"ci/decoy{i}", "status": "pending"} + for i in range(n)]})) + sys.exit(0) + if mode == "total-count-string": + # A schema-corrupted `total_count` as a STRING alongside a null `.statuses`. `jq -r` renders 0 + # and "0" identically, so a text compare would accept this. + print(json.dumps({"state": "pending", "total_count": "0", "statuses": None})) + sys.exit(0) if mode == "transport-error": # Real `gh()` is `curl -sf`: an HTTP error exits 22 with EMPTY stdout. sys.exit(22) @@ -1048,7 +1168,7 @@ if "/status" in url: "creator": ({"login": creator} if creator else None), "description": desc}, {"context": "Functional E2E", "status": "pending"}]})) sys.exit(0) - print(json.dumps({"statuses": []})) + print(empty_statuses()) sys.exit(0) print("{}") @@ -1059,7 +1179,9 @@ def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy", status_mode: str = "none", jq16: bool = False, status_creator: str | None = "timothy", status_desc: str = "Review-verdict: MERGEABLE @ a9e3e23 (base: main)", - timeline_mode: str = "none", history_mode: str = "none"): + timeline_mode: str = "none", history_mode: str = "none", + timeline_terminator: str = "null", + status_empty_shape: str = "null"): """Execute the workflow's classify `run:` block with a stubbed enumeration script. Returns the status payload the job POSTed, or None if it posted nothing. @@ -1085,6 +1207,8 @@ def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy", env["STUB_STATUS_CREATOR"] = status_creator or "" env["STUB_STATUS_DESC"] = status_desc env["STUB_TIMELINE_MODE"] = timeline_mode + env["STUB_TIMELINE_TERMINATOR"] = timeline_terminator + env["STUB_STATUS_EMPTY_SHAPE"] = status_empty_shape env["STUB_HISTORY_MODE"] = history_mode env.update({ "GITEA_TOKEN": "stub", @@ -2117,3 +2241,610 @@ def test_a_SENTINEL_landing_above_the_mark_also_triggers_the_repair(tmp_path): assert seq[1]["state"] == "pending" assert seq[1]["description"] == "Human verdict raced this exemption write — re-post the verdict", ( f"repaired, but not to the sentinel, so the fixed point is broken: {seq[1]}") + + +# --- The expression-delimiter class (ersatztv#751) ---------------------------------------------- +# +# WHAT HAPPENED, because the shape of it is what these tests are written against. A `run:` body is +# not shell yet when the runner reads it. The runner scans the whole scalar for the expression +# opener, and a single occurrence makes it rewrite the ENTIRE body into one `format(...)` call so +# the evaluated result can be spliced back in. That rewrite is all-or-nothing, so a payload that +# does not parse takes the whole step with it — and the runner then DROPS THE STEP AND CONCLUDES THE +# JOB `success`. +# +# The occurrence that did it was in a SHELL COMMENT: the #706 note explaining why a concurrency +# group does not work quoted a `concurrency:` snippet containing a PR-number expression as an +# illustration. `pr number` is not an expression. The step stopped running on 2026-08-03 and nothing +# went red until 2026-08-06. +# +# WHY EVERY EXISTING GUARD IN THIS FILE WAS BLIND TO IT, which is the part worth keeping: they all +# read `_code_lines()`, which strips comment lines. That choice is correct for what it was for — its +# own docstring explains that prose legitimately discusses `pulls/N/files`, and a raw scan would +# redden the repo over a piece of writing. But it encodes an assumption this bug falsifies: that a +# comment in a workflow cannot change behaviour. Inside a `run:` scalar it can. So the test below +# reads the RAW text, deliberately, and is the one test here that must never adopt `_code_lines`. + + +_EXPR = re.compile(r"\$\{\{(.*?)\}\}", re.S) + +# Roots of a dotted context path, and the callable functions. Both lists are what the runner +# actually accepts; anything outside them cannot evaluate, and an expression that cannot evaluate +# does not fail loudly — it silently removes the step it appears in. +_EXPR_CONTEXTS = frozenset({ + "github", "env", "vars", "secrets", "inputs", "runner", "steps", "needs", "matrix", "job", + "jobs", "strategy", +}) +_EXPR_FUNCTIONS = frozenset({ + "always", "success", "failure", "cancelled", "hashFiles", "format", "toJSON", "toJson", + "fromJSON", "fromJson", "contains", "startsWith", "endsWith", "join", +}) +_EXPR_LITERALS = frozenset({"true", "false", "null"}) + + +def _yaml_string_scalars(path: Path): + """Every string scalar in the parsed document — keys and values, recursively. + + Deliberately PARSED rather than raw (ersatztv#751, cold re-review). A `${{ … }}` in an ordinary + top-level YAML comment is inert: the runner never evaluates it, so redding on it would be a false + positive of exactly the kind this file has now produced twice. PyYAML drops those comments, which + is the behaviour wanted here. + + A `run:` body IS one of these scalars, and it keeps its SHELL comments — which is the whole point, + because inside a `run:` scalar a comment is not inert. So this covers the real defect class in + every workflow while ignoring the one place a delimiter is genuinely harmless. + """ + import yaml + out: list[str] = [] + + def walk(node): + if isinstance(node, str): + out.append(node) + elif isinstance(node, dict): + for k, v in node.items(): + if isinstance(k, str): + out.append(k) + walk(v) + elif isinstance(node, list): + for v in node: + walk(v) + + walk(yaml.safe_load(path.read_text())) + return out + + +def _workflow_files(): + # `*.y*ml` for the reason test_no_OTHER_workflow_writes_the_review_verdict_status gives: a + # workflow added as `.yaml` is just as executable and would otherwise go unscanned. + return sorted((REPO_ROOT / ".gitea" / "workflows").glob("*.y*ml")) + + +def test_the_verdict_workflow_has_NO_expression_delimiter_in_any_run_body(): + """The absolute rule, and it is scoped to this one file on purpose. + + Other workflows legitimately interpolate into a `run:` body (7 occurrences across + docker-build/ci-image/pr-checks today), so a repo-wide ban would be false and would be deleted + the first time it got in someone's way. This file is different in two ways that justify the + strict rule: it writes the branch-protection-required status, so a dropped step here is a dead + merge gate rather than a failed build; and its `run:` bodies are ~700 lines of dense prose, + which is the only place the delimiter has ever appeared by accident. + + The rule is also what keeps the dropped-step guard trustworthy. A guard that the guarded + mechanism can silently delete is worse than no guard, because its absence is silent too. + + The `run:` SCALAR AS PARSED, comments and all — never `_code_lines`, which strips them. The + distinction matters and is easy to garble: what must not be filtered is the SHELL comments inside + the body, because those are what the runner scans. Ordinary YAML comments outside a `run:` body are + genuinely inert and are handled by the repo-wide test below, which is why neither test reads the + file as flat text any more. + """ + offenders = [] + for job_name, step in _iter_workflow_steps(WORKFLOW): + for m in _EXPR.finditer(step.get("run") or ""): + offenders.append(f"{job_name}/{step.get('name', '?')}: {m.group(1).strip()!r}") + assert not offenders, ( + f"review-verdict.yml has an expression delimiter inside a run: body — {offenders}. Even in " + "a comment this is unsafe: the runner rewrites the WHOLE body into a format(...) call, and " + "if the payload does not parse it drops the step and reports the job GREEN, leaving the " + "required review-verdict/h10 unposted (ersatztv#751). To describe an expression in prose, " + "name it (`a github.event.pull_request.number expression`) instead of quoting the " + "delimiters. Pass values in through the step's `env:` block, which is interpolated per " + "value, so a bad payload there cannot take the body with it." + ) + # ANTI-VACUITY, and it has to be the right property. A `run:` body the YAML walk never reached + # would make the assertion above vacuously green — the failure mode to guard against. An earlier + # version of this guard compared the file's TOTAL delimiter count against the count inside + # `with:`/`env:` values, which is a different and wrong claim: it banned expressions everywhere + # else in the file too. Both reviewers reproduced the false red — writing `if: ${{ always() }}`, + # the standard and equivalent spelling of the `if:` two steps below, turned this test red, as did + # a delimiter in an inert top-level YAML comment. Neither is unsafe, and a red here blocks every + # merge through the combined status, so the guard was strictly more dangerous than the thing it + # was protecting against. + # + # ANTI-VACUITY WITHOUT A SECOND PARSER. A first attempt counted `run:` keys in the raw text and + # compared that to the walk. Cold re-review showed the regex only recognised an indented `run:` + # whose value starts with `|` or `>`, so legal spellings (`- run: |`, a single-line + # `run: echo ok`) counted as zero declarations and false-redded the file, while a `run: |` line + # sitting INSIDE a shell heredoc counted as a declaration. Hand-parsing YAML to check a YAML parse + # is the wrong shape: it adds a second, worse parser whose disagreements are all false alarms, and + # a red here blocks every merge through the combined status. + # + # The property that actually matters is that the walk reached the body that carries the risk. The + # classifier is ~700 lines; a walk that returned nothing, or only the short steps, is the failure + # to catch. Both are asserted on content, which no spelling change can spoof. + bodies = [s["run"] for _, s in _iter_workflow_steps(WORKFLOW) if s.get("run")] + # NOT `>= 3`, and not merely non-empty either. `>= 3` had zero slack — cold review deleted the + # optional jq-preflight step, a legitimate simplification, and this redded claiming the classifier + # had not been examined, which was untrue. But relaxing it to `assert bodies` threw away the only + # check that the walk reached ALL run-bearing steps: `max(len) > 5000` proves it reached the + # classifier and nothing about the short ones, so a helper that silently stopped yielding them + # would let an unscanned delimiter through. + # + # So count against the job's own step list, read here rather than through the helper under test. + # That catches the failure the count existed for (a helper looking at the wrong key, or dropping + # steps) without breaking when a step is legitimately added or removed. + import yaml as _yaml + _steps = (_yaml.safe_load(WORKFLOW.read_text())["jobs"]["set-verdict-status"]["steps"] or []) + declared = sum(1 for s in _steps if isinstance(s, dict) and s.get("run")) + assert len(bodies) == declared, ( + f"the YAML walk reached {len(bodies)} run: bodies but the job declares {declared} — the " + "assertion above did not examine every body, so a green here proves nothing") + assert max(len(b) for b in bodies) > 5000, ( + "the YAML walk did not reach a substantial run: body — the ~700-line classifier is the one " + "that must be scanned, so this test would be vacuous") + + +def _iter_workflow_steps(path: Path): + import yaml + doc = yaml.safe_load(path.read_text()) or {} + for job_name, job in (doc.get("jobs") or {}).items(): + for step in (job.get("steps") or []): + yield job_name, step + + +def test_every_workflow_expression_names_a_REAL_context_or_function(): + """The general form of #751, across every workflow and every field. + + The strict test above hardens the gate file. It cannot see the class, which is not "prose in a + run body" but "a payload that does not evaluate" — a typo in an `if:`, a renamed output, a + context that does not exist. All of them fail the same silent way, and in an `if:` the + consequence is the same shape as #751: the step does not run and nothing is red. + + BE PRECISE ABOUT WHAT THIS ENFORCES, because the first version of this docstring was not and + both reviewers caught it: it checks that THE HEAD TOKEN of each dotted path is a known context or + function. Nothing more. That catches the historical defect — `pr number` fails on `pr` — and a + payload naming a context that does not exist. It does NOT catch: + + * syntactically invalid expressions whose tokens are all known: `${{ github.ref == }}` and + `${{ github.event.pull_request.head.sha + }}` both pass, verified; + * a renamed or misspelled output or property, because every token after the first is preceded + by `.` and is deliberately skipped: `steps.metadata.outputs.shortsha` passes; + * an unclosed opener, since `_EXPR` requires the closing braces to match at all. + + A real fix for those is an expression parser, which is a different and much larger change. This is + a cheap net under the specific class that has bitten us, deliberately kept permissive so it cannot + false-red the repo (a red here blocks every merge through the combined status). Verified against + all 31 distinct payloads in this repo today, which pass. Do not restate this test as "catches any + payload that cannot evaluate" — that claim was in the docs and the decision record and was false. + """ + offenders = [] + for wf in _workflow_files(): + for scalar in _yaml_string_scalars(wf): + for m in _EXPR.finditer(scalar): + payload = m.group(1).strip() + # Strip string literals first: a path inside `hashFiles('web/package-lock.json')` is + # data, not an identifier, and would otherwise read as an unknown context. + bare = re.sub(r"'[^']*'", "''", payload) + for ident in re.finditer(r"(? unknown '{name}'") + assert not offenders, ( + "these workflow expressions name something the runner cannot resolve, so they will fail to " + f"interpolate — which DROPS THE STEP and still reports the job green (ersatztv#751): " + f"{offenders}. If this is prose describing an expression, do not write the delimiters; if " + "it is a real new context or function, add it to _EXPR_CONTEXTS / _EXPR_FUNCTIONS here." + ) + + +def test_a_dropped_classify_step_FAILS_the_job_instead_of_going_green(): + """The silent-green half of #751 — the actual defect, and the only part that generalises. + + The stray delimiter was one bug in one comment. What made it cost three days of a dead merge + gate was the REPORTING: a dropped step concludes `success`, the workflow's own status context + goes green, and the required `review-verdict/h10` is merely ABSENT — which on a normal PR is + indistinguishable from the correct "not reviewed yet" state. Nothing surfaced it. So the + classifier writes a start marker and a later `if: always()` step fails the job when it is + missing. + + Asserted structurally, and the marker PATH is compared across the two steps rather than + hardcoded here twice: the failure mode of a divergent path is a job that reddens on every run + (fail-closed, but it would look like this guard is broken rather than like a real drop), and a + test carrying its own third copy of the literal could not see the divergence at all. + + The premise — that the runner still executes a LATER step after dropping an earlier one — is + MEASURED, not assumed, and could not be settled from the #751 report because the classifier was + the job's last step. Gitea 1.27.1, 2026-08-06: probe run 1863 dropped the classifier on a + reintroduced bad payload, ran the guard anyway (`always()` evaluated true), and the job concluded + `failure`; run 1866 is the positive control. So the guard is load-bearing in production and the + body does not need to move into `scripts/`. This test pins the SHAPE; the probe pinned the + BEHAVIOUR, and neither substitutes for the other — which is why the behavioural test below + EXECUTES the guard body rather than only reading it. + """ + # ONE parse, and both steps located inside it. Using `_classify_step()` here instead was wrong + # in a way worth recording, because it went green-adjacent rather than loud: that helper does its + # own `yaml.safe_load`, so the dict it returns is never the same OBJECT as the equivalent step in + # this list. An `is not` filter against it therefore excluded nothing, the classify step matched + # as its own guard, and the assertions below ran against the wrong step. + steps = [s for _, s in _iter_workflow_steps(WORKFLOW)] + classifiers = [i for i, s in enumerate(steps) if "review-verdict/h10" in (s.get("run") or "")] + assert classifiers, "no step in review-verdict.yml posts review-verdict/h10" + classify = steps[classifiers[0]] + # The FULL path expansion is compared between the two steps, not a basename. The path is keyed on + # the run id, so a basename match would accept two steps that agree on the prefix and disagree on + # the key — which is precisely the divergence that would make the guard fail on every run. + marker_write = re.search(r'(\w+)="(\$\{RUNNER_TEMP[^"]*)"', classify["run"]) + assert marker_write, ( + "the classify step no longer assigns a start-marker path under RUNNER_TEMP, so a step the " + "runner drops goes green again (ersatztv#751)") + var, marker_name = marker_write.group(1), marker_write.group(2) + assert re.search(rf':\s*>\s*"\${var}"', classify["run"]), ( + f"the classify step defines {var} but never creates the marker, so the guard below will " + "fail on every run and read as broken rather than as a real dropped step") + + guard_idx = [i for i, s in enumerate(steps) + if i != classifiers[0] and marker_name in (s.get("run") or "")] + assert guard_idx, ( + f"no step checks for the {marker_name!r} start marker. Without it a dropped classify step " + "concludes success and the merge gate is silently dead (ersatztv#751)") + # AFTER the classifier, not merely present. A guard placed before it would read a marker that + # has not been written yet and fail on every run — fail-closed, but it would deadlock `main` and + # read as this guard being broken, which is how a correct-looking guard gets deleted. + assert guard_idx[0] > classifiers[0], ( + f"the dropped-step guard is step {guard_idx[0]} but the classifier is step " + f"{classifiers[0]} — a guard that runs first always fails") + guard = steps[guard_idx[0]] + # `always()` and `${{ always() }}` are the same condition; the runner accepts both and the second + # is the more common spelling. Pinning the bare form EXACTLY would red the repo over a + # semantically identical edit, so normalise instead. What must not change is that the guard runs + # when the classifier failed. + guard_if = re.sub(r"\s+", "", str(guard.get("if", ""))) + assert guard_if in ("always()", "${{always()}}"), ( + f"the dropped-step guard's `if:` is {guard.get('if')!r}; it must be `always()` (bare or " + "wrapped), or it will be skipped on exactly the runs where the classifier failed") + # INSIDE the missing-marker branch, not merely somewhere in the body. Cold review pointed out + # that a bare `exit 1` substring is satisfied by an unreachable `if false; then exit 1; fi` while + # the real branch says `exit 0` — the test passes and a dropped classifier goes green again. The + # behavioural test below is the real proof; this keeps the structural one from being satisfiable + # by dead code. + missing_branch = re.search(r'if \[ ! -f "\$marker" \]; then(.*?)\bfi\b', guard["run"], re.S) + assert missing_branch, ( + "the dropped-step guard no longer tests for a MISSING marker with `if [ ! -f \"$marker\" ]`, " + "so the assertion below cannot locate the branch that must fail the job") + assert re.search(r"exit\s+1", missing_branch.group(1)), ( + "the dropped-step guard detects the missing marker but does not `exit 1` inside that branch, " + "so it observes the failure and still lets the job go green — which is the whole defect") + assert not _EXPR.search(guard["run"]), ( + "the dropped-step guard's own run body contains an expression delimiter, so the mechanism " + "it guards against can drop the guard too — and that absence would be silent as well") + + +@pytest.mark.parametrize("terminator", ["null", "[]"], ids=["null-page", "empty-array-page"]) +def test_the_fence_TRUSTS_the_count_and_POSTS_when_the_timeline_terminates(tmp_path, terminator): + """The second defect found by the ersatztv#751 probe, and the one that actually kept the gate + 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 + 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. + + It hid for two reasons worth keeping written down. It shipped in the same commit (8f6d4f443) that + stopped the step from executing, so the fence had never once run in production; and the test + double printed `[]` while its comment claimed to mirror measured reality, so the type gate was + never exercised by the suite either. A green suite over an unfaithful double is what let a + fail-closed-by-accident branch look deliberate. + + Both shapes are asserted because both are live on this server: comments really do return `[]`. + Asserting the POSTED STATUS, not the log, because the log said `Decision: state=success` on the + real probe run and the job still posted nothing — the decision and the write are different events, + and only the write is what a merge reads. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode="stable:0", + timeline_terminator=terminator) + 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 " + "leaves the required review-verdict/h10 absent and the PR unmergeable with no bypass " + f"(ersatztv#751).\n{r.stdout[-1200:]}") + assert posted["state"] == "success", f"expected the docs-only exemption, got {posted}" + assert "trusted=yes" in r.stdout, ( + "the exemption was posted but the fence did not report a trusted count — the two must agree, " + f"or this test is passing for a different reason than it claims.\n{r.stdout[-800:]}") + + +def _marker_steps(): + """(classify body PREFIX through the marker write, guard body that CHECKS it) — from one parse. + + THE PREFIX, NOT THE MATCHING LINES. An earlier version picked out the lines matching `RAN_MARKER=` + and `: > "$RAN_MARKER"` and ran those two alone. Cold re-review showed that passes even if the + write is moved into a function nobody calls, or under `if false`: the extractor finds the text, + runs it at top level, the marker appears, and the test reports the guard proven while production + writes no marker at all. Executing the real prefix — everything from the top of the body down to + and including the write — reproduces the production control flow instead of a reconstruction of + it, so a write that is defined-but-not-reached simply does not happen and case B fails. + # + It also pins the property the marker needs anyway: that the write happens EARLY, before anything + in the classifier can fail. If it drifts down past code that needs credentials or network, this + prefix stops executing cleanly and the test says so. + """ + steps = [s for _, s in _iter_workflow_steps(WORKFLOW)] + idx = [i for i, s in enumerate(steps) if "review-verdict/h10" in (s.get("run") or "")] + classify = steps[idx[0]] + lines = classify["run"].splitlines() + write_at = [i for i, ln in enumerate(lines) if re.match(r'\s*:\s*>\s*"\$RAN_MARKER"', ln)] + assert len(write_at) == 1, ( + f"expected exactly one `: > \"$RAN_MARKER\"` in the classify body, found {len(write_at)}") + prefix = "\n".join(lines[:write_at[0] + 1]) + guard = next(s for i, s in enumerate(steps) + if i != idx[0] and "h10-classifier-started" in (s.get("run") or "")) + return prefix, guard["run"] + + +def test_the_dropped_step_guard_BEHAVIOURALLY_fails_without_the_marker_and_passes_with_it(tmp_path): + """Executes the guard, instead of reading it — the structural test above cannot prove the exit + code, and cold review was right that a bare `exit 1` substring is satisfiable by dead code. + + The marker is created by running THE CLASSIFY STEP'S OWN two prologue lines under the same + environment, never by rebuilding the path in Python. That is the point: it proves the two steps + AGREE on the path by construction. A test that computed the name itself would keep passing after + the two steps drifted apart, which is the one divergence that makes the guard fail on every run + and get deleted as broken. + + `GITHUB_RUN_ID`/`GITHUB_RUN_ATTEMPT` are set to fixed values, so this also covers the run-keying + added after the live probes: if either step stopped interpolating them, the paths would differ and + case B would fail. + """ + prologue, guard = _marker_steps() + assert prologue.count("RAN_MARKER=") == 1 and ': > "$RAN_MARKER"' in prologue, ( + f"could not extract the classify step's marker prologue; got {prologue!r}") + # The prefix must be the REAL top of the body, so a write hidden in an uncalled function is not + # executed by this test either. + assert prologue.lstrip().startswith("set -euo pipefail"), ( + "the extracted prefix does not start at the top of the classify body, so it is a " + f"reconstruction rather than the production path: {prologue[:120]!r}") + env = {"PATH": os.environ["PATH"], "RUNNER_TEMP": str(tmp_path), + "GITHUB_RUN_ID": "424242", "GITHUB_RUN_ATTEMPT": "7"} + + # A — the step was DROPPED: no marker exists. The job must fail. + a = subprocess.run(["bash", "-c", guard], env=env, capture_output=True, text=True) + assert a.returncode != 0, ( + "the guard exited 0 with NO start marker present — a dropped classify step would go green " + f"again, which is the whole defect (ersatztv#751).\nstdout: {a.stdout}\nstderr: {a.stderr}") + assert "did not execute" in (a.stdout + a.stderr), ( + f"the guard failed but without an actionable message: {a.stdout!r} {a.stderr!r}") + assert not list(tmp_path.glob("h10-classifier-started*")), ( + "the guard itself created the marker it is supposed to be checking for") + + # B — the classifier RAN: its own prologue created the marker. The guard must pass. + b = subprocess.run(["bash", "-c", prologue + "\n" + guard], env=env, + capture_output=True, text=True) + assert b.returncode == 0, ( + "the guard rejected a marker written by the classify step's OWN prologue — the two steps " + f"disagree on the path, so this guard would fail on every run.\nstdout: {b.stdout}\n" + f"stderr: {b.stderr}") + # C — the write must be reached by STRAIGHT-LINE code. Guarding the prefix trick itself: if the + # write were wrapped in a function or an `if`, the prefix would still contain it but production + # might not reach it. Executing the prefix with the function/conditional intact is the test; this + # assertion makes the intent explicit and fails loudly rather than subtly. + body_before = "\n".join(ln for ln in prologue.splitlines() + if not ln.lstrip().startswith("#")) + # Covers all four bash spellings: `mk() {`, `mk(){`, `function mk {`, `function mk() {`. The third + # was added when review found the first regex missed it, and the FOURTH still slipped that fix — + # the union form is the natural next spelling once `function mk {` is caught. Budget three rounds + # for any string-matching predicate. + assert not re.search(r"^\s*(function\s+)?\w+\s*(\(\s*\))?\s*\{", body_before, re.M), ( + "a function is defined before the marker write, so the write may be inside it and unreached " + f"in production while this test still passes:\n{body_before}") + + written = [q.name for q in tmp_path.glob("h10-classifier-started*")] + assert written == ["h10-classifier-started-424242-7"], ( + f"the marker is not keyed on the run id/attempt as intended; found {written}. A fixed name in " + "a shared RUNNER_TEMP lets a stale marker satisfy this guard on a run whose step was dropped") + + +@pytest.mark.parametrize("shape", ["null", "array"], + ids=["statuses-null", "statuses-empty-array"]) +def test_a_head_with_NO_statuses_YET_is_readable_and_still_gets_its_exemption(tmp_path, shape): + """The twin of the timeline terminator, found by cold review of the fix for that one (#751). + + `GET /commits/{sha}/status` returns `{"state":"pending","total_count":0,"statuses":null}` for a + head that has no statuses yet — measured on PR #739's head 5fa672e2. `read_existing_verdict` + gated on `.statuses | type == "array"`, so it read that as unreadable and took its `exit 1` + path: the job posted NOTHING. Fail-closed, but the user-visible result is exactly the outcome + this issue is about — an exempt PR with no status and, since #743, no bypass. + + Both shapes are asserted because a head that HAS statuses really does return an array, so the job + must read either. Restoring the `array`-only gate turns 40+ tests red with the corrected double, + which is the measure of how thoroughly the unfaithful stub was hiding this. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), status_empty_shape=shape) + assert r.returncode == 0, ( + f"the classifier died reading a `statuses: {shape}` body instead of treating it as " + f"'no verdict yet', so nothing was posted at all.\n{r.stdout[-1000:]}\n{r.stderr[-600:]}") + assert posted is not None, ( + f"a docs-only PR whose head has no statuses yet (shape: {shape}) got NO status at " + f"all.\n{r.stdout[-1000:]}") + assert posted["state"] == "success", f"expected the docs-only exemption, got {posted}" + + +@pytest.mark.parametrize("path", ["CLAUDE.md", "AGENTS.md"]) +def test_the_GOVERNANCE_docs_are_protected_and_get_no_docs_only_exemption(tmp_path, path): + """`CLAUDE.md`/`AGENTS.md` are not prose about the project — they define the completion protocol, + the merge-consent convention and the H10 rule itself. `DOCS_ONLY` (`^(docs/|[^/]*\\.md$)`) matched + them, so a PR editing the document that specifies what `.claude/` enforces was auto-exemptible + while `.claude/` itself was protected: the same self-exemption the workflow header rules out, one + directory over. Found by cold review of #751 by driving this exact case through the real body. + + Latent until #751, because no exemption `success` was writable at all while the classify step was + dropped — restoring the exemptions is what makes it reachable, which is why it is fixed there. + + Asserted through the PROTECTED branch specifically, not merely "not exempt": `pending` is reached + by several routes and a test that accepted any of them could not tell a working guard from a dead + one. + """ + posted, r = _run_classify(tmp_path, _emitting(path)) + assert posted is not None, f"the job posted nothing: {r.stderr[-1500:]}" + assert posted["state"] == "pending", ( + f"{path} was granted an exemption ({posted}) — the document that DEFINES the merge gate must " + f"not be able to exempt itself from it.\n{r.stdout[-800:]}") + assert "protected" in r.stdout.lower(), ( + f"{path} was not exempted, but not via the protected-path branch either, so that guard may be " + f"dead for it.\n{r.stdout[-800:]}") + + +def test_a_terminator_on_PAGE_ONE_does_not_certify_a_zero_retarget_count(tmp_path): + """`null` means "exhausted" only after a real page has been read. + + THE INVARIANT, not a figure that rots: a real PR's timeline always carries at least one event on + page 1, because the PR is created by a push and that is itself an event. Spot-checked non-empty + across #752/#753/#749/#739/#717; the counts are not recorded, because an earlier version of this + docstring cited five and three were stale within days. So a terminator on page 1 is anomalous, not + empty. Trusting a zero count from it would mean + certifying that no retarget happened on the strength of a response we cannot explain, which is the + one thing the fence exists to refuse. Withholding the exemption is the safe direction: the PR asks + for a human verdict instead. + + This narrows rather than closes the concern — a wrong `null` on page 3 is still read as exhaustion, + and no bounded number of round-trips can rule that out. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode="empty-first-page") + assert r.returncode == 0, r.stderr + assert posted is None, ( + "an exemption `success` was posted from a timeline whose FIRST page was already the " + f"terminator, so no page of events was ever actually read: {posted}\n{r.stdout[-800:]}") + assert "trusted=no" in r.stdout, ( + f"the exemption was withheld, but not because the count was untrusted:\n{r.stdout[-800:]}") + + +def test_a_SECOND_PAGE_of_statuses_refuses_to_conclude_that_no_verdict_exists(tmp_path): + """Completeness for the "no verdict exists" conclusion — the one that licenses posting over a + verdict this job cannot see. + + THE TWO OBVIOUS GUARDS ARE BOTH NO-OPS HERE, which is why this costs a round-trip: + + * `.statuses | length` vs `.total_count` — `total_count` is the count for the PAGE RETURNED, not + the commit. Measured at 1.27.1 on 3aed43c6 (6 contexts): `?limit=1` -> `len=1, total_count=1`. + Equal by construction. The stub mirrors that, so this test cannot pass for that wrong reason. + * "refuse when the page came back full at the requested limit of 100" — this instance caps `limit` + at `MAX_RESPONSE_ITEMS`, measured at 50 (`/issues?limit=100` returns 50), so a response can never + hold 100 rows and the comparison was DEAD CODE. The repo already documented that cap in three + places; the guard was written against 100 anyway and a cold review caught it. + + So the job asks the server, and only when the row is absent from page 1. Any rows on page 2 mean + the list is longer than one page and the verdict may be beyond it. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), status_mode="twopage") + assert posted is None, ( + "an exemption was posted while the status list ran to a second page, so an existing verdict " + f"beyond page 1 would have been silently overwritten: {posted}\n{r.stdout[-800:]}") + assert "page 2" in (r.stdout + r.stderr).lower(), ( + f"nothing was posted, but not because of the page-2 completeness probe:\n{r.stdout[-800:]}") + + +def test_a_SINGLE_page_of_statuses_reads_normally(tmp_path): + """Positive control for the probe above: without it, "refuse when page 2 has rows" could be + satisfied by refusing always, which deadlocks every PR while looking safe. 40 decoy contexts on + page 1, nothing on page 2. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), status_mode="rows:40") + assert r.returncode == 0, r.stderr + assert posted is not None and posted["state"] == "success", ( + f"a single page of statuses should read normally and still exempt; got {posted}\n" + f"{r.stdout[-800:]}") + + +def test_a_STRING_total_count_is_not_accepted_as_numeric_zero(tmp_path): + """`jq -r` renders the JSON number 0 and the JSON string "0" identically, so a text compare + accepts a schema-corrupted `"total_count": "0"` as "no statuses" (cold re-review reproduced it). + + The live schema uses an integer, so this is not a live failure — it is the difference between a + guard that holds because the input happens to be well-formed and one that holds because it checks. + The accept path requires the TYPE to be number. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), status_mode="total-count-string") + assert posted is None, ( + f"a string total_count was accepted as numeric zero, so a body that merely lost its statuses " + f"array reads as 'no verdict exists': {posted}\n{r.stdout[-800:]}") + + +@pytest.mark.parametrize("mode,why", [ + ("page2-garbage", "a non-JSON page 2"), + ("page2-error", "an HTTP error on page 2"), +], ids=["garbage", "transport-error"]) +def test_an_UNREADABLE_page_2_refuses_to_conclude_that_no_verdict_exists(tmp_path, mode, why): + """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 + full-page check written against a limit of 100 on a server that caps at 50. An unexercised branch + here has a track record. + + Both must fail CLOSED: the point of reading page 2 is to justify "no verdict exists", so a page 2 + that cannot be read justifies nothing. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), status_mode=mode) + assert posted is None, ( + f"an exemption was posted despite {why}, so 'no verdict exists' was concluded without " + f"evidence: {posted}\n{r.stdout[-800:]}") + assert "page 2" in (r.stdout + r.stderr).lower(), ( + f"nothing was posted, but not because of the page-2 probe:\n{r.stdout[-800:]}") + + +def test_a_status_history_RUNNING_PAST_PAGE_1_repairs_rather_than_leaving_green(tmp_path): + """The post-write race check reads ONE page of `/statuses/{sha}`, and `limit=100` clamps to the + server-wide cap (measured 50). So "no raced row on page 1" does not establish "no race" — a raced + human `failure` can sit on a page this job never reads. + + This is the ONE path in the design whose failure direction is toward SUCCESS: missing a raced + verdict leaves a forged green over a human rejection, permanently. So unread rows are treated as a + race and the exemption is repaired to `pending`, which is the conservative direction — a stall a + reviewer can clear, rather than a rejection silently turned green. + + The stub's second page carries ORDINARY rows, no verdict and no sentinel: what must trigger the + repair is the mere existence of rows this job did not read, not the discovery of a verdict. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="second-page") + assert r.returncode == 0, r.stderr + seq = _posted_sequence(tmp_path) + assert len(seq) == 2, ( + "the exemption was posted and left standing even though the status history ran past page 1, so " + f"a raced verdict beyond it would be buried. Posts: {seq}\n{r.stdout[-900:]}") + assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", ( + f"expected an exemption then a repair to pending; got {seq}") + assert seq[1]["description"] == "Human verdict raced this exemption write — re-post the verdict", ( + f"repaired, but not to the sentinel, so the fixed point is broken: {seq[1]}") + + +@pytest.mark.parametrize("mode,why", [ + ("second-page-garbage", "a non-JSON page 2"), + ("second-page-error", "an HTTP error on page 2"), +], ids=["garbage", "transport-error"]) +def test_an_UNREADABLE_history_page_2_also_repairs_rather_than_leaving_green(tmp_path, mode, why): + """Found by mutating the branch and watching nothing go red — my own coverage gap, in the same + class the review had just flagged twice. + + This is the fail-toward-SUCCESS path, so uncertainty must resolve to `pending`. "I could not read + the rest of the history" is not evidence that no verdict raced this write; treating it as such is + exactly how a forged green survives over a human rejection. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode=mode) + assert r.returncode == 0, r.stderr + seq = _posted_sequence(tmp_path) + assert len(seq) == 2, ( + f"the exemption was left standing despite {why} — a raced verdict beyond page 1 would be " + f"buried. Posts: {seq}\n{r.stdout[-900:]}") + assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", ( + f"expected an exemption then a repair to pending; got {seq}")