diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index 5271dc7ea..ef81a9ea5 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -104,13 +104,39 @@ body=$(printf '%s' "$prjson" | jq -r '.body // ""' 2>/dev/null || true) # A non-zero exit means "could not tell" and MUST withhold the exemption — never read stdout without # checking the status. An empty `$sha` (unparseable PR JSON) reaches the script as an empty argument # and is rejected there, so that path also fails closed. +# +# The 5th argument binds the enumeration to a base branch (ersatztv#698 route 1), because +# `/pulls/{n}/files` diffs against the PR's LIVE base and retargeting moves that without moving the +# head. Be precise about what it buys HERE, which is less than what it buys in the workflow: the +# workflow passes the base from a `pull_request_target` event payload, fixed at event time and beyond +# a retarget's reach, so it detects a retarget outright. This hook has no such trusted snapshot — it +# passes the base it just read from the live PR, so what it asserts is that the base did not move +# between that read and the enumeration. Narrower, and still worth having: without it the hook cannot +# tell a mid-flight retarget from an honest read at all. An empty/unparseable `.base.ref` reaches the +# script as an empty argument and is rejected there, so that path fails closed too. +base_ref=$(printf '%s' "$prjson" | jq -r '.base.ref // ""' 2>/dev/null || true) repo_root=$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd) files=""; files_complete=no -if files=$("$repo_root/scripts/pr-changed-files.sh" "$owner" "$repo" "$pr" "$sha" 2>/dev/null); then +if files=$("$repo_root/scripts/pr-changed-files.sh" "$owner" "$repo" "$pr" "$sha" "$base_ref" 2>/dev/null); then files_complete=yes fi -if [ "$files_complete" = yes ] && [ -n "$files" ] && ! printf '%s\n' "$files" | grep -qvE '^(docs/|\.claude/|\.husky/|\.gitea/|.*\.md$)'; then +# HOW THIS PREDICATE IS EVALUATED, matching the enforced gate (ersatztv#698, +# `ci.grep-q-pipefail-inversion`). `printf … | grep -q` INVERTS under `set -o pipefail`: grep -q exits +# at its first match, printf then takes SIGPIPE (141), and a MATCH is reported as a failed pipeline — +# so this negated test would grant a spurious docs-only exemption for any PR whose path list exceeds +# the pipe buffer. A here-string fixes that but is materialised via temporary storage for large inputs, +# so it can fail when temp space is full or unwritable and flip the predicate the same way. Counting +# with `grep -c` drains stdin (no SIGPIPE) over an ordinary pipe (no temp file); `grep -c` exits 1 for +# a zero count, which is a legitimate answer, so only a status >1 is a real error and is treated as +# "cannot tell" -> no exemption. +# Advisory here, so the blast radius is a missing prompt rather than a green required check; the +# construct is identical on purpose, because the two copies drifting is what ersatztv#649 was about. +docs_nonmatching=$(printf '%s\n' "$files" | grep -cvE '^(docs/|\.claude/|\.husky/|\.gitea/|.*\.md$)') || docs_grep_status=$? +if [ "${docs_grep_status:-0}" -gt 1 ]; then + docs_nonmatching=1 # grep itself failed: cannot tell, so withhold the exemption +fi +if [ "$files_complete" = yes ] && [ -n "$files" ] && [ "${docs_nonmatching:-1}" -eq 0 ]; then # Docs/process-only PR: the Done-when + review-verdict gate doesn't apply — but this exemption is a # file-TYPE bypass, NOT the a+b+c "provably reviewed & ready" proof, so it does NOT auto-grant. It # passes through to normal permissioning (one prompt). This deliberately keeps a human in the loop for diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index 237b699d2..4c5746315 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -88,10 +88,27 @@ name: Review verdict # into `main` carrying the same head. With the filter, a PR whose base is not `main` produces no run # and no status at all (verified the same way). +# `edited` IS LOAD-BEARING (ersatztv#698 route 1), not completeness for its own sake. Gitea fires it +# when a PR's base is retargeted, and a retarget changes the effective diff WITHOUT moving the head +# sha — so none of the other four types fire and the per-sha status stays exactly as it was. That is +# what made route 1 persist rather than merely exist: a PR was opened into `main`, retargeted to a +# scratch base while this job was in flight so the enumeration read docs-only and posted an exemption +# `success`, then retargeted BACK to `main`, where the forged success sat unchallenged on a head whose +# diff against `main` carried a C# file (reproduced as probe PR #703; `created_at == updated_at` +# afterwards proves nothing reclassified). With `edited`, the retarget back re-runs this job — and the +# short-circuit below now re-derives machine-written successes instead of inheriting them, which is +# the half that makes the re-run actually change the answer. The two are one fix; `edited` alone would +# re-run and then bail out on the existing `success`. +# +# BE PRECISE ABOUT WHAT THIS BUYS: detection, not atomicity or ordering. Runs are NOT serialized, so +# the stale run can post `success` AFTER the reclassifying run posts `pending` — restoring the forged +# state with no further event left to correct it — and an already-scheduled auto-merge can fire in the +# green window between them. The `main -> scratch -> main` ABA transition is therefore NARROWED and +# observable, not closed. Tracked as ersatztv#706; do not read this block as claiming otherwise. on: pull_request_target: branches: [main] - types: [opened, reopened, synchronize, ready_for_review] + types: [opened, reopened, synchronize, ready_for_review, edited] defaults: run: @@ -153,6 +170,13 @@ jobs: PR: ${{ github.event.pull_request.number }} SHA: ${{ github.event.pull_request.head.sha }} BASE_SHA: ${{ github.event.pull_request.base.sha }} + # The base BRANCH the event was raised for, passed down to the enumeration so the diff it + # reads cannot silently be one against a different base (ersatztv#698 route 1). This comes + # from the `pull_request_target` event payload, which is fixed at event time and is exactly + # what a mid-run retarget cannot rewrite — the live PR object can, which is the whole bug. + # `branches: [main]` means this is always `main` today; it is threaded through as a value + # rather than hardcoded so the two stay consistent if the filter ever widens. + BASE_REF: ${{ github.event.pull_request.base.ref }} AUTHOR: ${{ github.event.pull_request.user.login }} PR_URL: ${{ github.event.pull_request.html_url }} run: | @@ -162,6 +186,36 @@ jobs: # Accounts whose PRs may merge without a human verdict. Renovate only — keep this list # minimal and explicit; every entry is an account that can land code unreviewed. BOTS="renovate" + # The bot exemption is additionally constrained by CONTENT (ersatztv#698 route 2), because + # identity alone is not attributable to whoever wrote the code. `AUTHOR` is + # `pull_request.user.login` — the PR's CREATOR, which is immutable — while the head a PR + # points at is not: force-push application code onto an open Renovate branch and the PR is + # still authored by `renovate`, still touches no protected path, and was exempted. Nothing + # in the identity check attributes the CODE to the bot. + # + # Checking the pusher instead would not fix it — a git author/committer is self-asserted + # text and forgeable. So the exemption is gated on what a dependency bump can legitimately + # BE: an unattended merge is justified only for the manifests Renovate actually edits. + # + # The set is measured, not guessed: across all 11 Renovate PRs this repo has ever had, the + # paths touched were `Directory.Packages.props` (10 of them) and `.config/dotnet-tools.json` + # (1). The npm manifests are deliberately NOT included — see the BOT_MANIFESTS note below. + # + # Deliberately EXCLUDED, with the cost stated: `*.csproj` and any source file. The one + # historical Renovate PR outside the set above is #20, which touched a `.csproj` AND two C# + # files — and received an unattended bot exemption for a source change. Under Central + # Package Management versions live in `Directory.Packages.props`, so a `.csproj` edit + # attributed to Renovate is anomalous by construction. Such a PR is not blocked, it simply + # needs a real verdict, which is the correct handling for a PR carrying source changes. + # NOTE the npm manifests are deliberately ABSENT. An earlier draft included + # `web/package.json` / `web/package-lock.json` "so a first SPA bump cannot deadlock". That was + # a self-inflicted code-execution vector for zero benefit: `renovate.json` sets + # `enabledManagers: ["nuget", "github-actions", "dockerfile"]`, so Renovate does not manage npm + # in this repo at all, while `package.json` carries `scripts` that CI EXECUTES (`npm ci`, + # `npm run build` in docker-build.yml). Exempting it would let a hijacked bot branch run + # arbitrary shell in CI while every path still "looked like a manifest". If npm is ever added + # to enabledManagers, the lockfile may be exemptible but `package.json` is not. + BOT_MANIFESTS='^(Directory\.Packages\.props|\.config/dotnet-tools\.json)$' # Paths where NEITHER exemption applies, because a change here can alter the gate itself, # what CI runs, or what the hooks enforce. PROTECTED='^(\.claude/|\.gitea/|\.husky/|scripts/|docker/ci/)' @@ -178,37 +232,182 @@ jobs: gh() { curl -sf -H "Authorization: token $GITEA_TOKEN" "$@"; } - # --- Already decided for THIS sha? Never overwrite a real verdict. ------------------- - # A human/agent verdict for this exact head may already exist (the reviewer ran the - # script before this workflow finished, or a rerun). Re-posting `pending` over it would - # un-approve a reviewed head and stall the PR. + # DEFINED HERE, BEFORE ANY USE. An earlier round defined these AFTER the classification + # chain that calls them, so `count_matching` was `command not found` on every run, the + # PROTECTED branch silently never fired, and three "protected path" tests still passed — + # they reached `pending` by another route, so the guard being dead was invisible. # - # Read the COMBINED endpoint, not `/statuses/{sha}`: the latter returns one row per - # status POST (not per context) and pages at 50, so a head with a few CI reruns can push - # an earlier verdict off the first page. Missing it here is NOT harmless — we would post - # `pending` (or worse, an exemption `success`) over a real human verdict. The combined - # endpoint returns latest-per-context, which is both what we mean and ~11 rows. + # HOW THE PATH PREDICATES ARE EVALUATED, and why neither obvious spelling is used. # - # An unreadable/unparseable response must NOT be read as "no verdict exists": fail the - # job WITHOUT posting anything, so a transient API error can never overwrite a verdict. - statusjson=$(gh "$BASE_URL/repos/$REPO/commits/$SHA/status?limit=100") || statusjson="" - # The empty case is checked EXPLICITLY, not left to jq's exit status: `jq -e` over empty - # input exits 4 on jq >= 1.7 but 0 on jq 1.6 (ersatztv#647), and the RUNNER SHIPS 1.6 — - # so on a transient API error this guard passed, `existing` came back "", and the job - # went on to post `pending` (or an exemption `success`) over a possibly-existing human - # verdict. Exactly what the paragraph above says must never happen. The sibling fix in - # .claude/hooks/pretooluse-merge-consent.sh (ersatztv#643) missed this copy: that hook - # runs on a dev Mac with jq 1.8, this workflow runs where the bug is live. - if [ -z "${statusjson//[[:space:]]/}" ] || ! printf '%s' "$statusjson" | 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." - exit 1 - fi - existing=$(printf '%s' "$statusjson" \ - | jq -r --arg c "$CONTEXT" '[.statuses[] | select(.context == $c)] | first | .status // ""') - if [ "$existing" = "success" ] || [ "$existing" = "failure" ]; then - echo "${CONTEXT} is already '${existing}' on ${SHA:0:7} — leaving the existing verdict alone." + # `producer | grep -q…` is FORBIDDEN here: `grep -q` exits at its first match, the producer + # then takes SIGPIPE and exits 141 once the list exceeds the pipe buffer, and under + # `set -o pipefail` the pipeline is a FAILURE even though grep MATCHED — inverting the guard + # for exactly the large PRs that matter. Reproduced with `A.cs` + 1900 docs paths (171KB, + # inside the enumerator's 2000-file cap): `docs_only=yes`, status 141; and a `.gitea/` path + # made `PROTECTED` MISS. That construct predates #698 and was live on `main`. + # + # A here-string (`grep -q… <<< "$files"`) fixes the SIGPIPE but bash materialises a large + # here-string via TEMPORARY STORAGE, so it can fail when the runner's temp space is full or + # unwritable — and because these run inside `if`/`!`, that failure would flip the predicate + # the same way. Trading a buffer bug for an environmental one is not a fix. + # + # So: count with `grep -c`, which DRAINS stdin (no early exit, no SIGPIPE) over an ordinary + # pipe (no temp file), and treat grep's own exit status honestly — `grep -c` exits 1 when the + # count is zero, which is a legitimate answer, while anything >1 is a real error and must FAIL + # THE JOB rather than silently read as "no match". `set -e` would not catch these on its own + # because they sit inside command substitution in a conditional. + count_matching() { # how many lines of $2 match $1 + local out st=0 + out=$(printf '%s\n' "$2" | grep -cE "$1") || st=$? + # NOT `exit 1`: these run inside `$( )`, so an exit leaves only the SUBSHELL and, because + # the substitution sits in a conditional, `set -e` does not fire either — the job would sail + # on with the predicate silently reading as "no match". Emit a NON-NUMERIC sentinel instead + # and let the caller, at top level, refuse to classify. + if [ "$st" -gt 1 ]; then + echo "::error::grep failed (status ${st}) evaluating a path predicate." >&2 + printf 'ERR' + return 0 + fi + printf '%s' "${out:-0}" + } + count_not_matching() { # how many lines of $2 do NOT match $1 + local out st=0 + out=$(printf '%s\n' "$2" | grep -cvE "$1") || st=$? + if [ "$st" -gt 1 ]; then + echo "::error::grep failed (status ${st}) evaluating a path predicate." >&2 + printf 'ERR' + return 0 + fi + printf '%s' "${out:-0}" + } + + + # --- Is there already a verdict for THIS sha? ---------------------------------------- + # NOTE the heading no longer says "never overwrite". It cannot promise that: the read below + # and the POST at the end of this job are not atomic, so a human verdict posted in between is + # still overwritten. The re-read immediately before the POST narrows that window; it does not + # close it. Tracked as ersatztv#706 rather than claimed as solved. + # Reads the CONTEXT row for $SHA and sets ex_state / ex_creator / ex_desc / ex_human. + # Factored into a function because it is now called TWICE — once here, and once immediately + # before the POST (see below). An unreadable/unparseable response must NOT be read as "no + # verdict exists": the job dies WITHOUT posting, so a transient API error can never overwrite + # a verdict. + # + # The empty case is checked EXPLICITLY, not left to jq's exit status: `jq -e` over empty input + # exits 4 on jq >= 1.7 but 0 on jq 1.6, and THE RUNNER SHIPS 1.6 (ersatztv#647) — so on a + # transient error this guard passed, the row came back "", and the job posted over a + # possibly-existing human verdict. + # + # The COMBINED endpoint is read, not `/statuses/{sha}`: the latter returns one row per POST + # (not per context) and pages at 50, so a head with a few CI reruns can push an earlier verdict + # off the first page. + 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." + exit 1 + fi + row=$(printf '%s' "$json" | jq -r --arg c "$CONTEXT" '[.statuses[] | select(.context == $c)] | first // {}') + 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 // ""') + # A `case` prefix test rather than grep: the description is a single short string, and this + # removes one more pipeline from a security predicate entirely. The PATTERN is a literal, so + # there is no glob-injection concern from $ex_desc. + # A human verdict also has to have been formed against THIS base (ersatztv#698, found in + # round-4 review). `post-review-verdict.sh` records the base it reviewed in the status + # description — `Review-verdict: MERGEABLE @ abc1234 (base: main)` — precisely because + # retargeting changes the effective diff without moving the head sha (ersatztv#632). + # Without this check the sha-binding is escapable through the HUMAN path rather than the + # exemption path: get a genuine `success` on head H while it targets a scratch base S with + # a benign diff, then retarget H onto `main`, where its diff contains unreviewed code. The + # status is real, its creator is real, and it was silently inherited. The merge-consent + # hook compares the base and would object, but that is advisory and covers only its own + # path — a merge through the Gitea UI or API just sees a green required check. + # + # An ABSENT base is deliberately NOT treated as a mismatch: verdicts predating #632 carry + # no `(base: …)`, and re-deriving over one would un-approve a genuinely reviewed head. Only + # a base that is PRESENT and DIFFERENT is rejected, which is exactly the escape above. + ex_human=no + case "$ex_desc" in + "Review-verdict:"*) + if [ -n "$ex_creator" ]; then ex_human=yes; fi + ;; + esac + if [ "$ex_human" = yes ]; then + # COMPARE, NEVER PARSE. Two earlier attempts both extracted the base out of the + # description and both were defeated, the second in a way that looked like a fix for the + # first: + # * `${ex_desc##*"(base: "}` (LAST occurrence) let an APPENDED `(base: main)` override a + # genuine `(base: probe/scratch)`; + # * `${ex_desc#*"(base: "}` (FIRST occurrence) fixed that, but `${...%%)*}` still + # truncates at the first `)`. `main)evil` IS A VALID GIT BRANCH NAME + # (`git check-ref-format --branch 'main)evil'` succeeds), so a verdict earned while + # targeting it reads `(base: main)evil)`, truncates to exactly `main`, and is + # INHERITED after retargeting onto `main`. No forged description, no #697 needed. + # The comment here previously asserted a `)` in a branch name "mismatches — safe + # direction"; that was generalised from `feat/foo)bar` and is FALSE for any branch + # whose name starts with the target base. + # + # So extract nothing. `post-review-verdict.sh` writes the marker LAST, so require the + # description to END with the exact literal `(base: )` and to contain + # exactly ONE marker — which kills the append trick without having to decide which + # occurrence is authoritative. Pure shell; no truncation exists to abuse. + # + # `${#}` arithmetic rather than a `grep -o | wc -l` pipeline; 7 is the length of + # "(base: ". An ABSENT marker is still not a mismatch (verdicts predate #632). + ex_stripped=${ex_desc//"(base: "/} + ex_markers=$(( (${#ex_desc} - ${#ex_stripped}) / 7 )) + if [ "$ex_markers" -ne 0 ]; then + ex_base_ok=no + if [ "$ex_markers" -eq 1 ]; then + case "$ex_desc" in + *"(base: $BASE_REF)") ex_base_ok=yes ;; + esac + fi + if [ "$ex_base_ok" != yes ]; then + ex_human=no + echo "${CONTEXT} on ${SHA:0:7} is a human verdict, but its recorded base does not match this PR's base '${BASE_REF}' (description: ${ex_desc}) — the reviewed diff is not this PR's diff, so it is NOT treated as a verdict for this base." + fi + fi + fi + } + + # --- Whose verdict is it? (ersatztv#698 route 3) ------------------------------------- + # This short-circuit used to exit on ANY existing `success`, which made an exemption this job + # wrote indistinguishable from a verdict a human wrote. That is what let a forged exemption + # survive: obtained once — via the route-1 retarget race, a sibling workflow holding + # status-write credentials (#697), a direct API call, or inheritance across PRs by sha (#663) + # — it was thereafter accepted unchanged on every run, because the guard exited before it + # looked at the PR, the base, the author or the files. + # + # The guard still exists for its original reason: re-posting `pending` over a real human + # verdict would un-approve a reviewed head and stall the PR. So it discriminates by PROVENANCE. + # + # MEASURED on this instance (Gitea 1.25.4), on the COMBINED endpoint this job reads: a status + # POSTed with a USER credential — how `scripts/post-review-verdict.sh` writes a verdict — + # carries `.creator.login`, while one POSTed by an Actions job with the built-in `GITEA_TOKEN` + # carries `"creator": null`. A real verdict read back `creator=timothy`; this job's own + # exemption read back `creator=null`. + # + # BOTH conditions are required, and the DIRECTION of the test is the point: we short-circuit + # only on something POSITIVELY identified as a human verdict. Anything else, including anything + # we do not recognise, is RE-DERIVED. Written the other way round ("skip if it looks + # machine-written") an unrecognised shape would be trusted — the fail-open this issue is about. + # + # What this does NOT claim: the test asks "was this POSTed by a user credential", NOT "by a + # reviewer". `ETV_STATUS_AUTH` is basic auth, so head-controlled code can POST a success with a + # non-null creator AND an attacker-chosen `Review-verdict:` description, which this guard then + # preserves. That is #697 — provenance, not authentication. + read_existing_verdict + if [ "$ex_human" = yes ] && { [ "$ex_state" = "success" ] || [ "$ex_state" = "failure" ]; }; then + echo "${CONTEXT} is already '${ex_state}' on ${SHA:0:7}, written by '${ex_creator}' as a human verdict — leaving it alone." exit 0 fi + if [ -n "$ex_state" ]; then + echo "${CONTEXT} is '${ex_state}' on ${SHA:0:7} but is NOT an attributable human verdict (creator='${ex_creator:-null}', description='${ex_desc}') — re-deriving it from the PR's current state rather than inheriting it." + fi # --- Changed files: the SHARED enumeration, or no exemption. ------------------------- # `scripts/pr-changed-files.sh` (from the BASE checkout) owns every guard this job used to @@ -231,7 +430,7 @@ jobs: # Only reachable for a PR whose BASE predates ersatztv#658. Fail closed with a readable # status rather than an absent one, so the PR shows why instead of stalling silently. enum_error="the PR's base ref (${BASE_SHA:0:7}) has no executable ${ENUM}" - elif files=$("$ENUM" "${REPO%%/*}" "${REPO#*/}" "$PR" "$SHA"); then + elif files=$("$ENUM" "${REPO%%/*}" "${REPO#*/}" "$PR" "$SHA" "$BASE_REF"); then complete=yes else files="" @@ -243,22 +442,70 @@ jobs: echo "Changed files (${count}, complete=${complete}):" printf '%s\n' "$files" | sed 's/^/ /' + # Evaluated ONCE, at TOP LEVEL, so a failure can actually stop the job. Evaluating them + # inline inside the `if`/`elif` chain is what hid the two defects above: a bad status or a + # missing function turned into an empty string, `[ "" -gt 0 ]` errored, and the branch was + # simply skipped. A non-numeric result here is fatal and posts nothing — an absent required + # check blocks the merge, which is the correct direction. + n_protected=$(count_matching "$PROTECTED" "$files") + n_not_manifest=$(count_not_matching "$BOT_MANIFESTS" "$files") + n_not_docs=$(count_not_matching "$DOCS_ONLY" "$files") + for v in "$n_protected" "$n_not_manifest" "$n_not_docs"; do + case "$v" in + ''|*[!0-9]*) + echo "::error::A path predicate returned '${v}' instead of a count — the classifier is not operating, so no ${CONTEXT} status will be written for ${SHA:0:7}." + exit 1 ;; + esac + done + exempt=no reason="" if [ "$complete" != yes ]; then reason="${enum_error} — no exemption" elif [ "${count:-0}" -eq 0 ]; then reason="no changed files could be read from the API — no exemption" - elif printf '%s\n' "$files" | grep -qE "$PROTECTED"; then + elif [ "$n_protected" -gt 0 ]; then reason="touches a protected path (gate/CI/hooks/scripts/ci-image) — exemptions do not apply" - elif printf '%s\n' "$BOTS" | tr ' ' '\n' | grep -qxF "$AUTHOR"; then - exempt=yes - reason="authored by the '$AUTHOR' bot account and touches no protected path" - elif ! printf '%s\n' "$files" | grep -qvE "$DOCS_ONLY"; then - exempt=yes - reason="docs-only change (no code, no protected path)" else - reason="awaiting an H10 review verdict for head ${SHA:0:7}" + # The two exemptions are evaluated as INDEPENDENT predicates rather than as a chain. + # An `elif` chain was wrong once the bot exemption gained a second condition + # (ersatztv#698 route 2): a Renovate PR that changes only `docs/` would enter the bot + # branch, fail the manifest test, and never reach the docs-only branch at all — silently + # withdrawing an exemption that the docs-only rule grants on its own merits, for any + # author. Composing the predicates and deciding afterwards keeps each rule's meaning + # independent of the order they happen to be written in. + # + # `grep -qv` asks "is there any line NOT in this allow-list", so an unrecognised path + # withholds the exemption instead of being ignored — the same closed-set direction the + # enumeration itself uses. Both are safe against an empty `$files` because `count -eq 0` + # is handled above. + # Written as `if`/`then`, never as `cmd && var=yes`: under `set -e` a bare `A && B` + # statement whose `A` fails takes the failure as the statement's own exit status and + # kills the job. That would fail closed here (no status posted, absent required check + # blocks the merge) but it would do so on the ORDINARY path — every non-bot PR — so the + # gate would look broken rather than strict. `cmd || var=yes` is safe for the same + # reason it is confusing; both are spelled out instead. + # BOTS is a short fixed literal, so it cannot reach the pipe buffer; it is still written + # with an explicit status capture so a grep error cannot read as "not a bot" by accident. + is_bot=no + bot_hits=$(printf '%s\n' "$BOTS" | tr ' ' '\n' | grep -cxF "$AUTHOR") || bot_hits=0 + if [ "${bot_hits:-0}" -gt 0 ]; then is_bot=yes; fi + manifests_only=no + if [ "$n_not_manifest" -eq 0 ]; then manifests_only=yes; fi + docs_only=no + if [ "$n_not_docs" -eq 0 ]; then docs_only=yes; fi + + if [ "$is_bot" = yes ] && [ "$manifests_only" = yes ]; then + exempt=yes + reason="authored by the '$AUTHOR' bot account, touches no protected path, and changes only dependency manifests" + elif [ "$docs_only" = yes ]; then + exempt=yes + reason="docs-only change (no code, no protected path)" + elif [ "$is_bot" = yes ]; then + reason="authored by the '$AUTHOR' bot account, but changes files outside the dependency-manifest set — a bot ACCOUNT does not attribute the CODE at this head (the account is the PR's immutable creator; the head is not), so this needs a real verdict" + else + reason="awaiting an H10 review verdict for head ${SHA:0:7}" + fi fi if [ "$exempt" = yes ]; then @@ -270,6 +517,18 @@ jobs: fi echo "Decision: state=${state} — ${reason}" + # LAST-MOMENT RE-READ (ersatztv#706). Classification takes several API round-trips, and a + # reviewer can post a verdict during them — most dangerously a `failure`, which this job would + # then overwrite with an exemption `success`, turning an explicit human rejection green. The + # first read cannot see that; this one can. It NARROWS the window, it does not close it: there + # is no compare-and-set on Gitea's status API, so a verdict landing between this read and the + # POST below is still lost. Said plainly here rather than left as an implied guarantee. + read_existing_verdict + if [ "$ex_human" = yes ]; then + echo "::notice::A human verdict ('${ex_state}' by '${ex_creator}') landed on ${SHA:0:7} while this job was classifying — leaving it alone and posting nothing." + exit 0 + fi + payload=$(jq -n --arg s "$state" --arg c "$CONTEXT" --arg d "$desc" --arg u "$PR_URL" \ '{state:$s, context:$c, description:$d, target_url:$u}') gh -X POST -H 'Content-Type: application/json' -d "$payload" \ diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 454ebfb2b..fe7d76236 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -794,12 +794,52 @@ PR touches `.claude/`, `.gitea/`, `.husky/`, `scripts/` or `docker/ci/`** — a gate must not be able to exempt itself from the gate. That includes Renovate's `docker/ci` base bumps, which already need the manual publish-then-pin two-step anyway. +The Renovate exemption additionally requires **every** changed path to be a dependency manifest — +`Directory.Packages.props` or `.config/dotnet-tools.json`, and only those (ersatztv#698). The npm +manifests are deliberately excluded: `renovate.json` enables only `nuget`/`github-actions`/`dockerfile`, +so npm is unmanaged here, while `package.json` `scripts` are executed by CI (`npm ci`, `npm run build`) +— exempting it would put a code-execution path inside the allow-list for no benefit. An author match alone is not enough, because `pull_request.user.login` is the PR's +*immutable creator* while its head is not: pushing application code onto an open Renovate branch +leaves the PR still "authored by renovate" and, previously, still exempt. A Renovate PR touching +anything else — a `.csproj`, a source file — is not blocked, it just needs a real verdict. **If a +dependency PR is unexpectedly asking for a verdict, this is why**; the status description says so. + +The two exemptions are evaluated as **independent predicates**, never as an `elif` chain: a Renovate +PR touching only `docs/` still gets the docs-only exemption on its own merits. + +An existing `review-verdict/h10` on the head is **only** left alone when it is positively identifiable +as a human verdict — a non-null `.creator.login` **and** a `Review-verdict:` description, which is what +`post-review-verdict.sh` writes. Anything else, including any shape the workflow does not recognise, is +**re-derived** rather than inherited. (Measured: a status POSTed with a user credential carries a +creator; one POSTed by an Actions job carries `"creator": null`.) Without this, an exemption obtained +once was accepted unchanged on every later run. This is a *provenance* check, not an authentication +one — someone who can POST statuses directly can still impersonate a verdict, which is ersatztv#697. + Deciding either exemption requires the PR's **complete** changed-file list, which the workflow does not compute itself: it calls `scripts/pr-changed-files.sh`, the single shared implementation also used by the advisory hook `.claude/hooks/pretooluse-merge-consent.sh` (ersatztv#649). The workflow reads that script's **exit status** — a non-zero exit means "could not tell" and withholds the exemption; its stdout is meaningless on any failure path and is never consumed. +**Never write a classification guard as `producer | grep -q…` here.** Under `set -o pipefail`, `grep -q` +exits at its first match, the producer takes SIGPIPE (141), and a MATCH is reported as a failed +pipeline — inverting the guard for any PR whose path list exceeds the pipe buffer. That let a large PR +be classified docs-only, and let one editing `.gitea/` skip the protected-path check entirely. A +here-string is **also** wrong (bash spills a large one to temp storage, which fails the same way when +temp is full). **Count** instead — `grep -c` drains stdin over an ordinary pipe — evaluate the counts +once at top level rather than inline in an `if`, and fail closed on a non-numeric result. Full detail: +`ci.grep-q-pipefail-inversion`. + +That script takes the expected base branch as a **required 5th argument** and refuses to enumerate when +the PR's live base does not match it, checked both before and after paging (ersatztv#698). +`/pulls/{n}/files` diffs against the PR's *live* base, so retargeting changes the answer without moving +the head sha — a PR opened into `main` and retargeted mid-run was granted a docs-only exemption while +its diff against `main` carried a C# file. The workflow passes the base from the `pull_request_target` +payload, which a retarget cannot rewrite, and `edited` is in `types:` so a retarget reclassifies. +`edited` gives **detection, not atomicity**: runs are not serialized, so a stale run can still post +`success` after the reclassifying run posts `pending`. The `main → scratch → main` ABA transition is +narrowed and observable, not closed — see the residual in `ci.exemption-provenance`. + Three properties of this workflow are security-relevant and are **structurally** asserted by tests in `scripts/tests/test_pr_changed_files.py` — those tests pin the workflow's shape, which is not the same as establishing that the gate cannot be forged (see the residual below, and ersatztv#697/#698): @@ -822,9 +862,10 @@ as establishing that the gate cannot be forged (see the residual below, and ersa write-capable `GITEA_TOKEN` into every job and branch protection binds the *context*, not its issuer. The exemption path has separate defects of its own (ersatztv#698). One operational consequence of the trigger change: a PR whose base is not `main` now gets **no** - `review-verdict/h10` at all. That is fail-closed, but `edited` is not among the trigger's `types`, - so a PR opened against another base and later **retargeted** onto `main` stays statusless until its - next push — if a retargeted PR looks stalled with the check simply missing, that is why. + `review-verdict/h10` at all. That is fail-closed. `edited` **is** now among the trigger's `types` + (ersatztv#698), so a PR retargeted onto `main` reclassifies instead of staying statusless until its + next push — but note that only gives *detection*: runs are not serialized, so a stale run can still + post `success` after the reclassifying run posts `pending` (ersatztv#706). - **The checkout takes the PR's BASE ref**, `ref: ${{ github.event.pull_request.base.sha }}` with `persist-credentials: false` — never the head. This job judges the PR, so the PR must not supply the code that judges it; a head checkout would let a PR edit the enumeration to return an empty diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 972090d81..d04651101 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -43,10 +43,12 @@ 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 (residual, #706). 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`. Tracked in #697; the exemption path has its own separate defects in #698. | 2026-07-28 | [link](records/ci/gate-trigger-base-resolved.md) | | `ci.gitea-milestone-filter-noop` | Never filter issues with the server-side `?milestones=` parameter — fetch all open issues once and filter LOCALLY on each issue's `.milestone.title`. | 2026-07-21 | [link](records/ci/gitea-milestone-filter-noop.md) | +| `ci.grep-q-pipefail-inversion` | In any script running under `set -o pipefail`, a security or classification predicate of the form `producer \| grep -q…` is FORBIDDEN: `grep -q` exits at its first match, the producer then takes SIGPIPE and exits 141 once the data exceeds the pipe buffer (~64K), so `pipefail` reports the pipeline as FAILED even though grep MATCHED — inverting the predicate exactly when the input is large. A here-string (`grep -q… <<< "$data"`) is ALSO forbidden: bash materialises a large here-string via temporary storage, so it fails when temp space is full or unwritable, and inside an `if`/`!` that failure flips the predicate the same way. COUNT instead — `n=$(printf '%s\n' "$data" \| grep -cE "$re")` — because `grep -c` drains stdin (no early exit, no SIGPIPE) over an ordinary pipe (no temp file). Read grep's status honestly: exit 1 means a zero count and is a legitimate answer, anything >1 is a real error. Evaluate the counts ONCE at TOP LEVEL, never inline inside an `if`/`elif` condition: inside `$( )` an `exit` leaves only the subshell and `set -e` does not fire, so an error silently reads as "no match". Validate that each result is numeric and fail closed if not. This applies to both the enforced gate `.gitea/workflows/review-verdict.yml` and the advisory hook `.claude/hooks/pretooluse-merge-consent.sh`. | 2026-07-29 | [link](records/ci/grep-q-pipefail-inversion.md) | | `ci.infra-shaped-red-under-load` | When a job dies inside a setup/cache step before your code compiles, check the runner host's load before diagnosing the diff, and never file a CI bug off one sample under pressure. | 2026-07-21 | [link](records/ci/infra-shaped-red-under-load.md) | | `ci.jq-version-contract` | Every shell gate that shells out to `jq` is authored to the jq 1.6-compatible subset, because the CI runner ships jq 1.6 while every developer Mac ships 1.8.x. `scripts/jq-preflight.sh` (no args) prints the parsed version and asserts a floor of 1.6 in every gate job's log; `scripts/jq-preflight.sh --expect 1.6` additionally pins and fails loudly, but ONLY in the `script-tests` job. `review-verdict.yml` never pins — it writes the branch-protection-required `review-verdict/h10` status, so a hard pin there would turn any jq bump into a repo-wide merge deadlock. | 2026-07-26 | [link](records/ci/jq-version-contract.md) | | `ci.killed-job-triage` | Never trust a job's `conclusion` field alone — read the log tail and require an `❌ Failure - Main …` marker before treating a red as a real failure. | 2026-07-21 | [link](records/ci/killed-job-triage.md) | @@ -56,7 +58,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.root-screenshot-guard` | The Husky `pre-commit` hook refuses a staged root-level `*.png` (belt-and-suspenders with the `.gitignore` rule); nested `*.png` real assets are unaffected. | 2026-07-12 | [link](records/ci/root-screenshot-guard.md) | | `ci.runner-placement` | No persistent Roslyn compiler server survives a CI build (`UseSharedCompilation=false` etc., runner env + Dockerfile `ENV`); every `services:` container gets its own explicit `--memory`/`--memory-swap`/`--cpus` cap (it does not inherit the job container's). | 2026-07-17 | [link](records/ci/runner-placement.md) | | `ci.script-tests-job` | The `scripts/tests/` pytest suite runs on every PR as a dedicated `script-tests` job in `pr-checks.yml` (`runs-on: small`, `setup-python` + `pip install pytest`, `PYTHONPATH=. python3 -m pytest scripts/tests -q`), unconditionally rather than behind a `scripts/**` path filter, and **never as a step inside `decisions-guard`** — a job whose reds a standing rule instructs sessions to ignore must never host a gate whose reds are real. Any new CI gate must be reachable by a failure that is unambiguously attributable to it. | 2026-07-26 | [link](records/ci/script-tests-job.md) | -| `ci.shared-pr-file-enumeration` | A PR's complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate. | 2026-07-26 | [link](records/ci/shared-pr-file-enumeration.md) | +| `ci.shared-pr-file-enumeration` | A PR's complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding, and base-ref binding — see `ci.exemption-provenance`) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate. | 2026-07-26 | [link](records/ci/shared-pr-file-enumeration.md) | | `ci.small-lane-git-only` | `runs-on: small` is defined by what a job does (git-only), not its usual runtime; the two `docker build` jobs (docker-build.yml, ci-image.yml) move to `ubuntu-latest` because their worst-case memory, not median runtime, was pinning the small lane's per-slot cap. | 2026-07-20 | [link](records/ci/small-lane-git-only.md) | | `ci.ui-e2e-harness` | The UI-interactive E2E flows run as headless Playwright specs (`web/e2e/*.spec.ts`, driven by `scripts/e2e-ui.sh`) in a **second step of the existing advisory `functional-e2e` job**, never their own job; the browser is `chromium-headless-shell` **baked into the CI toolchain image** (`docker/ci/Dockerfile`, `PLAYWRIGHT_VERSION` kept equal to `web/package.json`'s EXACT `@playwright/test` pin), never installed per run; specs are `serial` with `retries: 0` and assert only contracts the curl harness structurally cannot reach. | 2026-07-25 | [link](records/ci/ui-e2e-harness.md) | | `ci.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) | diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md new file mode 100644 index 000000000..73ca1f80d --- /dev/null +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -0,0 +1,82 @@ +--- +key: ci.exemption-provenance +title: '2026-07-29 — the `review-verdict/h10` exemption path binds the base ref, constrains the bot exemption by CONTENT, and re-derives any success it cannot attribute to a human (#698)' +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 (residual, #706). 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]`' +--- + +`ci.gate-trigger-base-resolved` stopped a PR supplying the gate's own *definition*. This closes the +layer below: the exemption path still **decided from mutable or unattributed PR state**, and a +machine-written `success` was never revalidated. Three routes, one root cause, one fix. + +**Route 1 was reproduced, not theorised** (probe PR #703). A head `H` and scratch base `S` chosen so +`H` vs `S` is docs-only; opened `H → main` so the trusted base definition ran; retargeted to `S` +mid-flight. The job enumerated against the moved base, read docs-only, and posted +`review-verdict/h10=success — "Exempt: docs-only change"`. Retargeted back to `main`: **nothing +reclassified** (`created_at == updated_at`), leaving a PR into `main` whose diff carried a C# file +behind a green required check. Closed unmerged, branches deleted, no forged `h10` left anywhere. +**Why a base BINDING and not a pinned diff.** Diffing two immutable shas would close it outright; +Gitea 1.25.4 cannot serve that — measured: `compare/{base}...{head}` returns no `files`, and a +`--depth=1` fetch of the two shas has no merge base, so three-dot is impossible and two-dot +over-reports everything `main` gained since the branch point. So the base is read before the first page +and after the last, and **the gap is stated plainly**: a retarget opening *and* closing strictly +between the files call and the re-read stays invisible from inside the enumeration. + +**An earlier draft claimed `edited` made that residual non-durable. It does not, and cross-family +review was right to call it a Blocker.** `edited` gives DETECTION, not atomicity or ordering: runs are +not serialized, so the stale run can post `success` AFTER the reclassifying run posts `pending`, and an +already-scheduled merge can fire in the green window between them. The `main → scratch → main` ABA +transition is therefore NARROWED and observable, not closed. Tracked as an explicit residual rather +than described as fixed. `edited` and re-derivation remain one fix — `edited` alone re-runs and exits +on the existing `success`; re-derivation alone never gets a second run — but together they are +mitigation, not a guarantee. + +**Route 2 — a bot ACCOUNT does not attribute the CODE.** `pull_request.user.login` is the PR's +immutable *creator*; its head is not. Push application code onto an open Renovate branch and the PR is +still "authored by renovate", touches no protected path, and was exempted. Checking the *pusher* fixes +nothing — a git author is self-asserted text. So the exemption is constrained by what a bump can +legitimately *be*: across all 11 Renovate PRs this repo has had, the paths touched were +`Directory.Packages.props` (10) and `.config/dotnet-tools.json` (1) — and ONLY those. An earlier draft +also exempted `web/package.json`/`web/package-lock.json` "so a first SPA bump cannot deadlock"; review +called that a Blocker and was right. `renovate.json` enables only nuget/github-actions/dockerfile, so +npm is unmanaged here and the entry bought nothing, while `package.json` `scripts` are EXECUTED by CI +(`npm ci`, `npm run build`) — widening an exemption onto a code-execution path for no benefit. `*.csproj` is excluded — under Central Package Management +versions live in `Directory.Packages.props`, so a Renovate `.csproj` edit is anomalous by +construction. Cost stated: such a PR is not blocked, it needs a real verdict. The two exemptions are +evaluated as INDEPENDENT predicates: written as an `elif` chain, a Renovate PR touching only `docs/` +entered the bot branch, failed the manifest test, and never reached the docs-only branch. + +**A counterfactual, not an incident.** Renovate PR #20 touched a `.csproj` and two C# files but has no +`h10` status: it merged 2026-06-27, the gate landed 2026-07-25. The point is what identity-only *would* +have done. An earlier draft claimed it HAD been exempted — wrong, and the correction is kept because +"was silently exempted" and "would have been" are different claims. + +**Route 3 — provenance, and the direction of the test.** The short-circuit exited on any `success`, so +an exemption this job wrote was indistinguishable from a human verdict; obtained once, a forgery was +accepted on every later run. It could not simply be deleted — it exists so `pending` cannot un-approve +a reviewed head. Measured on the **combined** endpoint: a status POSTed with a user +credential carries `.creator.login`, one POSTed by an Actions job carries `"creator": null`. The test is +written in the **positive** direction — short-circuit only on something identified as human — because +spelled the other way ("skip if it looks machine-written") any unrecognised shape falls through to +*trusted*. Both halves are required, so if Gitea later populates `creator` for Actions the description +test still fails: the guard degrades toward re-deriving, never toward trusting. + +**What this does NOT close.** Anyone who can POST statuses directly can write both a creator and a +`Review-verdict:` description and impersonate a verdict; branch protection binds the *context*, not its +issuer. A provenance check, not an authentication one — that is `#697`, left open because its durable +fix is credential scoping, partly server-management territory. Severity as `#672`: requires push +access, so the threat model is a compromised contributor. + +**Verification honesty.** Route 1 was reproduced live; the "and now it fails" half cannot be shown from +a PR, because `pull_request_target` resolves this definition from `main` — the self-test gap +`ci.gate-trigger-base-resolved` records. Pre-merge evidence is that reproduction plus the +executed-behaviour tests in `scripts/tests/test_pr_changed_files.py`, each verified by mutation; the +live re-check happens on `main` right after merge. + +**Separate defect found reviewing this change:** `ci.grep-q-pipefail-inversion` — a pre-existing +SIGPIPE inversion that let a large PR skip the `PROTECTED` guard entirely. Fixed in the same PR. diff --git a/docs/decisions/records/ci/gate-trigger-base-resolved.md b/docs/decisions/records/ci/gate-trigger-base-resolved.md index e6298b13c..975f6f3e9 100644 --- a/docs/decisions/records/ci/gate-trigger-base-resolved.md +++ b/docs/decisions/records/ci/gate-trigger-base-resolved.md @@ -7,7 +7,7 @@ supersedes: none superseded-by: none rule: '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`. Tracked in #697; the exemption path has its own separate defects in #698.' signals: 'workflow definition resolved from head, PR rewrites the gate that judges it, self-approve a required status check, pull_request_target vs pull_request, gate trigger branches filter, attacker-supplied base branch, how to test a change to review-verdict.yml, workflow not exercised by its own PR, gate edit goes live only on merge, required_approvals 0 does not bind an author, forged commit status inherited by sha · paths: `.gitea/workflows/review-verdict.yml`, `scripts/tests/test_pr_changed_files.py` · issues: #672, #663, #649, #622' -mechanics: '`on: pull_request_target: {branches: [main], types: [opened, reopened, synchronize, ready_for_review]}`; asserted by `test_the_workflow_trigger_is_pull_request_TARGET_scoped_to_main` in `scripts/tests/test_pr_changed_files.py`; the job''s own context is renamed to `... (pull_request_target)` and must stay OUT of branch protection''s required list' +mechanics: '`on: pull_request_target: {branches: [main], types: [opened, reopened, synchronize, ready_for_review, edited]}` (`edited` added by `ci.exemption-provenance` so a retarget reclassifies); asserted by `test_the_workflow_trigger_is_pull_request_TARGET_scoped_to_main` in `scripts/tests/test_pr_changed_files.py`; the job''s own context is renamed to `... (pull_request_target)` and must stay OUT of branch protection''s required list' --- `ci.shared-pr-file-enumeration` had this job check out the PR's **base** ref so the PR cannot supply @@ -25,9 +25,8 @@ on this instance (Gitea 1.25.4) with **four** scratch PRs, not by analogy to Git the head's rewrite and never wrote the real `h10`; `pull_request_target` ignored the identical rewrite and the base definition posted `h10=pending` on `opened` and `synchronize` alike, secrets available; `branches: [main]` produced no run at all from a non-`main` base; and the fourth — the negative one -that established the residual below — is counted because omitting it turns an honest partial into an -overclaim. Probes posted only probe-named contexts, never a forged `h10`. Full results in #699. Not -measured: whether `reopened`/`ready_for_review` fire here. +establishing the residual below — is counted because omitting it turns an honest partial into an +overclaim. Probes posted only probe-named contexts, never a forged `h10`. Full results in #699. **Why `branches: [main]` is load-bearing rather than tidy.** The *base branch* supplies the definition, and anyone who can push a branch can make it a base — so dropping the filter trades a @@ -67,6 +66,6 @@ was never one of branch protection's required contexts (the two `docker-build.ym `review-verdict/h10`); adding it would let the workflow satisfy the gate by merely running. **A trap for #697:** those two carry the literal `(pull_request)` suffix, so giving `docker-build.yml` the same treatment renames them and deadlocks merges unless branch protection is edited in the same operation. -A non-`main` base now yields no status where it previously got one — fail-closed, and it removes a -`#663` hazard — but `edited` is absent from `types:`, so a PR retargeted onto `main` stays statusless -until its next `synchronize`. +A non-`main` base now yields no status where it previously got one — fail-closed, removing a `#663` +hazard. The `edited` gap this section once recorded as a mere inconvenience ("statusless until its next +`synchronize`") was the persistence half of a live forgery; RESOLVED in `ci.exemption-provenance` (#698). diff --git a/docs/decisions/records/ci/grep-q-pipefail-inversion.md b/docs/decisions/records/ci/grep-q-pipefail-inversion.md new file mode 100644 index 000000000..6c7701c13 --- /dev/null +++ b/docs/decisions/records/ci/grep-q-pipefail-inversion.md @@ -0,0 +1,59 @@ +--- +key: ci.grep-q-pipefail-inversion +title: '2026-07-29 — never feed `grep -q` from a pipe under `set -o pipefail`: SIGPIPE turns a MATCH into a failed pipeline and inverts the guard (#698)' +status: active +since: '2026-07-29' +supersedes: none +superseded-by: none +rule: 'In any script running under `set -o pipefail`, a security or classification predicate of the form `producer | grep -q…` is FORBIDDEN: `grep -q` exits at its first match, the producer then takes SIGPIPE and exits 141 once the data exceeds the pipe buffer (~64K), so `pipefail` reports the pipeline as FAILED even though grep MATCHED — inverting the predicate exactly when the input is large. A here-string (`grep -q… <<< "$data"`) is ALSO forbidden: bash materialises a large here-string via temporary storage, so it fails when temp space is full or unwritable, and inside an `if`/`!` that failure flips the predicate the same way. COUNT instead — `n=$(printf ''%s\n'' "$data" | grep -cE "$re")` — because `grep -c` drains stdin (no early exit, no SIGPIPE) over an ordinary pipe (no temp file). Read grep''s status honestly: exit 1 means a zero count and is a legitimate answer, anything >1 is a real error. Evaluate the counts ONCE at TOP LEVEL, never inline inside an `if`/`elif` condition: inside `$( )` an `exit` leaves only the subshell and `set -e` does not fire, so an error silently reads as "no match". Validate that each result is numeric and fail closed if not. This applies to both the enforced gate `.gitea/workflows/review-verdict.yml` and the advisory hook `.claude/hooks/pretooluse-merge-consent.sh`.' +signals: 'grep -q pipefail, exit 141, SIGPIPE in a shell guard, large PR classified docs-only, protected path guard skipped, printf pipe grep -q, classification inverts on big input, pipe buffer 64K shell predicate · paths: `.gitea/workflows/review-verdict.yml`, `.claude/hooks/pretooluse-merge-consent.sh`, `scripts/tests/test_pr_changed_files.py` · issues: #698, #649' +mechanics: '`count_matching` / `count_not_matching` helpers in `review-verdict.yml`, DEFINED BEFORE FIRST USE, results precomputed into `n_protected`/`n_not_manifest`/`n_not_docs` at top level and validated numeric; regression tests `test_a_LARGE_pr_*` build 1900+ paths (~171KB) to cross the pipe buffer, `test_the_classify_step_runs_without_SHELL_ERRORS` catches a dead guard, `test_a_protected_path_is_rejected_BY_THE_PROTECTED_BRANCH` asserts the branch rather than the outcome' +--- + +Found by cross-family adversarial review of the #698 PR, then reproduced directly. It is not a defect +of that change — the construct shipped with the ORIGINAL docs-only check and was live on `main`. + +**The mechanism.** `grep -q` stops reading at its first match. If the writer still has data queued, it +gets SIGPIPE and exits 141. `pipefail` makes the pipeline's status that of the failing member, so a +successful MATCH is reported as a FAILED pipeline. Every guard written as `if ! producer | grep -q…` +or `if producer | grep -q…` therefore flips, and only for large inputs — which is why every existing +test missed it: they all used a handful of short paths, far below the buffer. + +**Both directions were reproduced**, at 1901 paths / ~171KB, inside the enumerator's 2000-file cap: + +- `A.cs` followed by 1900 docs paths → `docs_only=yes`, pipeline status 141. A C# PR classified + docs-only and auto-granted `review-verdict/h10=success`. +- `.gitea/workflows/review-verdict.yml` followed by 1900 docs paths → the `PROTECTED` guard MISSES, so + a PR editing the gate's own definition skips the protected-path check and falls through to the + exemptions. + +The second is the serious one: it reaches a green required status on a PR that rewrites the gate, with +**no retarget, no bot account and no forged status** — nothing but a large PR. It is a more direct hole +than any of the three routes #698 was filed for. + +**The here-string was the FIRST fix, and it was wrong.** `grep -q… <<< "$data"` does remove the +SIGPIPE, and it shipped for one round. But bash materialises a large here-string through temporary +storage, so it fails when temp space is full or unwritable — and because these predicates sit inside +`if`/`!`, that failure flips them exactly as SIGPIPE did. It did not reproduce on macOS bash 3.2 and +did on Linux bash 5.x, which is the environment CI runs; the disagreement is itself the argument for a +construct that cannot fail either way. Counting with `grep -c` uses an ordinary pipe and drains stdin, +so neither failure mode exists. + +**Two follow-on traps, both found only by re-review.** First, the helpers were defined AFTER the +classification chain that called them, so `count_matching` was `command not found` on every run and the +`PROTECTED` branch never fired — while three "protected path" tests stayed green, because a protected +path is also not a manifest and not docs-only, so the job reached `pending` down another route. Second, +`exit 1` inside those helpers only left the command-substitution SUBSHELL, and since the substitution +sat in a conditional, `set -e` never fired either. Hence the rule: define before use, evaluate once at +top level, validate the result is numeric, and fail closed when it is not. + +**Two testing lessons.** When several branches produce the SAME outcome, asserting the outcome cannot +tell you which branch ran — assert the discriminator (here the `Decision:` reason line). And a cheap +stderr sweep for `command not found` / `integer expression expected` / `unbound variable` catches a +whole family of silently-skipped guards, because each of those makes an `if` condition merely false +while the job exits 0 and posts a plausible status. + +**The input-size lesson.** The whole class was invisible because every test used small inputs. A guard whose behaviour depends on a BUFFER THRESHOLD needs a test that crosses +the threshold; otherwise the suite is measuring the wrong regime entirely and full coverage of the +small regime proves nothing. The regression tests pair each large-input negative with a large-input +POSITIVE control, so "large lists now fail closed" (a merge deadlock) cannot masquerade as a fix. diff --git a/docs/decisions/records/ci/shared-pr-file-enumeration.md b/docs/decisions/records/ci/shared-pr-file-enumeration.md index db4dfc91e..e9621b97c 100644 --- a/docs/decisions/records/ci/shared-pr-file-enumeration.md +++ b/docs/decisions/records/ci/shared-pr-file-enumeration.md @@ -5,9 +5,9 @@ status: active since: '2026-07-26' supersedes: none superseded-by: none -rule: 'A PR''s complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate.' +rule: 'A PR''s complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding, and base-ref binding — see `ci.exemption-provenance`) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate.' signals: 'duplicated PR file enumeration, enforced gate weaker than advisory hook, docs-only allow-list drift, shared mechanism not shared policy, pr-changed-files.sh, checkout base ref not PR head, gate judging its own PR, exhaustiveness bug in a security predicate · paths: `scripts/pr-changed-files.sh`, `.claude/hooks/pretooluse-merge-consent.sh`, `.gitea/workflows/review-verdict.yml` · issues: #643, #648, #649' -mechanics: '`scripts/pr-changed-files.sh ` -> stdout newline-delimited paths, exit 0 only if complete and bound to the given sha; callers: `.claude/hooks/pretooluse-merge-consent.sh`, `.gitea/workflows/review-verdict.yml`' +mechanics: '`scripts/pr-changed-files.sh ` -> stdout newline-delimited paths, exit 0 only if complete and bound to BOTH the given sha and the given base branch; the 5th argument is REQUIRED and a 4-arg call exits 2 (`ci.exemption-provenance`); callers: `.claude/hooks/pretooluse-merge-consent.sh`, `.gitea/workflows/review-verdict.yml`' --- Before #649, the PR changed-file enumeration existed as two independent implementations. That would diff --git a/scripts/pr-changed-files.sh b/scripts/pr-changed-files.sh index e6be5001b..4eb03d8a7 100755 --- a/scripts/pr-changed-files.sh +++ b/scripts/pr-changed-files.sh @@ -18,14 +18,35 @@ # Sharing the enumeration fixes the drift; sharing the classification would erase an intended difference. # # CONTRACT -# Usage: pr-changed-files.sh +# Usage: pr-changed-files.sh # stdout: newline-delimited paths, BOTH sides of every rename, no blank lines. May be empty. -# exit 0 the enumeration is COMPLETE and bound to . stdout is authoritative. +# exit 0 the enumeration is COMPLETE and bound to AND . +# stdout is authoritative. # exit 1 the enumeration could NOT be completed or verified. stdout is meaningless — the caller # MUST fail closed (withhold any exemption). A diagnostic goes to stderr. # exit 2 usage error. # Callers must treat any non-zero exit as "no exemption". Never read stdout without checking the status. # +# WHY THE BASE REF IS AN ARGUMENT, AND WHY IT IS NOT OPTIONAL (ersatztv#698 route 1). +# `/pulls/{n}/files` computes the diff against the PR's **live** base, which is mutable. Retargeting a +# PR changes the enumerated file set without moving the head sha, so head-binding alone does not bind +# the ANSWER — only the commit it is nominally about. Reproduced live on this instance: a PR opened +# into `main` and retargeted mid-run to a scratch base enumerated as docs-only and was granted +# `review-verdict/h10=success`, while its diff against `main` carried a C# file (probe PR #703). +# +# REQUIRED rather than optional on purpose. An optional binding on a shared security primitive is an +# opt-out, and the caller that forgets it is precisely the caller that needed it — silently. Five +# arguments or exit 2. +# +# This NARROWS the window, it does not erase it. The base is re-read after the paging round trips +# alongside the head, so a retarget that is still in effect at that point fails closed; a retarget +# that opens and closes strictly between the files call and the re-read is not observable from here. +# Pinning the diff to two shas would close it, and Gitea 1.25.4 cannot serve that: `compare/{base}... +# {head}` returns `total_commits`/`commits` and NO `files`, and a `--depth=1` fetch of the two shas +# has no merge base, so a three-dot diff is impossible while a two-dot one over-reports every commit +# `main` gained since the branch point (both measured, #698). The remainder is covered one level up +# instead, by the workflow reclassifying on `edited` rather than trusting a machine-written success. +# # AUTH/TRANSPORT is caller-supplied via env, because the two callers authenticate differently: # ETV_GITEA_TOKEN | GITEA_TOKEN -> `Authorization: token` # ETV_GITEA_BASICAUTH -> curl -u user:pass @@ -43,8 +64,8 @@ set -euo pipefail -if [ "$#" -ne 4 ]; then - echo "usage: pr-changed-files.sh " >&2 +if [ "$#" -ne 5 ]; then + echo "usage: pr-changed-files.sh " >&2 exit 2 fi @@ -52,9 +73,10 @@ owner=$1 repo=$2 pr=$3 expected_sha=$4 +expected_base=$5 -if [ -z "$owner" ] || [ -z "$repo" ] || [ -z "$pr" ] || [ -z "$expected_sha" ]; then - echo "pr-changed-files: empty owner/repo/pr/sha argument" >&2 +if [ -z "$owner" ] || [ -z "$repo" ] || [ -z "$pr" ] || [ -z "$expected_sha" ] || [ -z "$expected_base" ]; then + echo "pr-changed-files: empty owner/repo/pr/sha/base argument" >&2 exit 2 fi @@ -85,6 +107,21 @@ if [ -z "${ETV_GITEA_TOKEN:-}" ] && [ -z "${GITEA_TOKEN:-}" ] && [ -z "${ETV_GIT exit 1 fi +# Bind the BASE before the first page is requested (ersatztv#698 route 1). Checking only afterwards +# would leave the common case — a PR retargeted before the enumeration even starts — indistinguishable +# from an honest one, because every page would agree with every other page while all of them described +# a diff against the wrong base. Both ends are checked; neither alone is sufficient. +prjson_before=$(gq "repos/$owner/$repo/pulls/$pr") +if [ -z "${prjson_before//[[:space:]]/}" ]; then + echo "pr-changed-files: could not read PR #$pr to bind the base ref before enumerating — failing closed" >&2 + exit 1 +fi +base_before=$(printf '%s' "$prjson_before" | jq -r '.base.ref // ""' 2>/dev/null || true) +if [ -z "$base_before" ] || [ "$base_before" != "$expected_base" ]; then + echo "pr-changed-files: PR #$pr targets '${base_before:-}', not the expected '$expected_base' — the diff would be computed against a different base, failing closed" >&2 + exit 1 +fi + PAGE_SIZE=50 MAX_PAGES=40 # 2000 files; beyond this we refuse rather than guess @@ -192,5 +229,16 @@ if [ -z "$sha_after" ] || [ "$sha_after" != "$expected_sha" ]; then exit 1 fi +# The same round-trip window applies to the BASE, and the head check cannot see it: retargeting moves +# the diff without moving the head sha (ersatztv#698 route 1). Comparing `.base.ref` — the branch NAME, +# never its tip — is deliberate and matches `post-review-verdict.sh` (ersatztv#632): a base that merely +# ADVANCES is ordinary churn, while comparing tips would fail every enumeration on every unrelated +# merge to `main`. +base_after=$(printf '%s' "$prjson" | jq -r '.base.ref // ""' 2>/dev/null || true) +if [ -z "$base_after" ] || [ "$base_after" != "$expected_base" ]; then + echo "pr-changed-files: base moved during enumeration ('$expected_base' -> '${base_after:-}') — the enumerated diff is against a base this PR no longer targets, failing closed" >&2 + exit 1 +fi + printf '%s\n' "$files" | grep -v '^$' || true exit 0 diff --git a/scripts/tests/test_merge_consent_exemption.py b/scripts/tests/test_merge_consent_exemption.py index 517ab2abe..07a1221b8 100644 --- a/scripts/tests/test_merge_consent_exemption.py +++ b/scripts/tests/test_merge_consent_exemption.py @@ -64,7 +64,15 @@ if "/pulls/" in url: ctr.write_text(str(nread + 1)) if alt.exists() and nread >= 1: shas = [alt.read_text().strip()] - print(json.dumps({"head": {"sha": shas[0]}, "body": "no linked issue here"})) + # `.base.ref` is served because the hook now reads it and threads it to the enumeration as the + # required 5th argument (ersatztv#698 route 1). Without it the hook passes an empty base, the + # script exits 2, and EVERY docs-only exemption silently stops being granted — which is exactly + # how this stub failed when the argument was added: the eight failures were all positive cases. + # Fail-closed, so not dangerous, but it would have made the advisory hook prompt on every + # docs-only PR. + print(json.dumps({"head": {"sha": shas[0]}, + "base": {"ref": os.environ.get("STUB_BASE", "main")}, + "body": "no linked issue here"})) sys.exit(0) print("{}") @@ -495,3 +503,36 @@ def test_array_valued_status_does_not_dodge_the_allow_list(hook): def test_object_valued_status_is_also_rejected(hook): hook.set_pages([{"filename": "docs/a.md", "status": {"x": "renamed"}}], []) assert hook.exempted() is False + + + +# --- The `grep -q` / pipefail inversion, on the ADVISORY side (ersatztv#698) -------------------- +# +# Round-2 cross-family review noted the enforced gate gained large-input regression tests while the +# hook — which carries the SAME predicate — did not. The hook's blast radius is smaller (a missing +# prompt, not a green required check), but `ci.shared-pr-file-enumeration` exists precisely because +# the copy with LESS authority is the one that quietly keeps a bug. So test both. +# +# `grep -q` exits at its first match; the producer then takes SIGPIPE (141) once the path list exceeds +# the pipe buffer, and under `set -o pipefail` a MATCH is reported as a FAILED pipeline — inverting the +# negated docs-only test. ~171KB is needed to cross the threshold; every other test in this file uses a +# handful of short paths, which is exactly why the class was invisible here. + +def _many_docs(n=1900): + return [f"docs/{'d' * 40}-{i:040d}.md" for i in range(n)] + + +def test_a_LARGE_pr_containing_a_code_file_is_NOT_exempt(hook): + """The code file goes FIRST so the guard matches immediately and the producer is left with the + bulk of ~171KB still to write.""" + hook.set_pages(_rows(["A.cs", *_many_docs()])) + assert hook.exempted() is False, ( + "a large PR containing A.cs was granted the docs-only exemption — the predicate inverted") + + +def test_positive_control_a_LARGE_genuinely_docs_only_pr_IS_still_exempt(hook): + """Guards the opposite failure: if large lists merely errored, the test above would pass while the + hook prompted on every big docs PR. Without this, 'fixed' and 'broken' are indistinguishable.""" + hook.set_pages(_rows(_many_docs())) + assert hook.exempted() is True, ( + "a large but genuinely docs-only PR lost its exemption") diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index e48af2dcb..1175c0313 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -60,13 +60,27 @@ if "/pulls/" in url and "/files" in url: print(json.dumps(entry)); sys.exit(0) if "/pulls/" in url: - # A second head sha served from the Nth PR-object read onward models a force-push landing - # between pagination round-trips. + # The PR object is read TWICE now: once before paging to bind the base, once after to bind both + # the head and the base (ersatztv#698). A counter distinguishes them, so a test can move either + # field at either end and the two guards can be isolated from each other. + counter = state / "pr_reads.txt" + n = int(counter.read_text()) if counter.exists() else 0 + counter.write_text(str(n + 1)) + sha = os.environ["STUB_SHA"] alt = state / "pr_sha_after.txt" - if alt.exists(): + if alt.exists() and n >= 1: # force-push landing between pagination round-trips sha = alt.read_text().strip() - print(json.dumps({"head": {"sha": sha}})) + + base = os.environ.get("STUB_BASE", "main") + base_before = state / "pr_base_before.txt" + base_after = state / "pr_base_after.txt" + if base_before.exists() and n == 0: # already retargeted when the job started + base = base_before.read_text().strip() + if base_after.exists() and n >= 1: # retargeted mid-enumeration + base = base_after.read_text().strip() + + print(json.dumps({"head": {"sha": sha}, "base": {"ref": base}})) sys.exit(0) print("{}") @@ -103,10 +117,20 @@ def enumerate_files(tmp_path): def head_moves_to(self, sha): (state / "pr_sha_after.txt").write_text(sha) - def run(self, expected_sha=SHA, args=("timothy", "ersatztv", "42")): - return subprocess.run( - ["bash", str(SCRIPT), *args, expected_sha], - env=self.env, capture_output=True, text=True) + def base_is_already(self, ref): + """The PR targets `ref` before the first page is requested (retargeted pre-run).""" + (state / "pr_base_before.txt").write_text(ref) + + def base_moves_to(self, ref): + """The PR is retargeted to `ref` between the files pages and the binding re-read.""" + (state / "pr_base_after.txt").write_text(ref) + + def run(self, expected_sha=SHA, args=("timothy", "ersatztv", "42"), + expected_base="main"): + argv = ["bash", str(SCRIPT), *args, expected_sha] + if expected_base is not None: + argv.append(expected_base) + return subprocess.run(argv, env=self.env, capture_output=True, text=True) def paths(self): """Assert success and return the enumerated path set.""" @@ -237,13 +261,14 @@ def test_renamed_row_without_previous_filename_fails_closed(enumerate_files): def test_head_differing_from_the_expected_sha_fails_closed(enumerate_files): """Narrowed to what this actually proves, per cold review. - The stub serves the alternate sha from the FIRST PR-object read, and the script reads that - object only once, after paging — so this exercises "the final head does not equal the expected - sha", not movement *during* enumeration. The distinction matters: an A->B->A force-push round - trip would restore the expected sha and pass this check while the pages came from two different - states. That race is inherent to enumerating a mutable list over several round-trips against an - API with no commit-pinned files endpoint, and is tracked separately rather than papered over - with a test name that implies it is covered. + The stub serves the alternate sha from the BINDING read (the one after paging; since + ersatztv#698 the script also reads the PR object BEFORE paging, to bind the base). So this + exercises "the final head does not equal the expected sha", not movement *during* enumeration. + The distinction matters: an A->B->A force-push round trip would restore the expected sha and + pass this check while the pages came from two different states. That race is inherent to + enumerating a mutable list over several round-trips against an API with no commit-pinned files + endpoint, and is tracked separately rather than papered over with a test name that implies it is + covered. """ enumerate_files.set_pages(_rows(["docs/a.md"])) enumerate_files.head_moves_to(OTHER_SHA) @@ -264,6 +289,82 @@ def test_usage_errors_exit_2(enumerate_files, args): assert r.returncode == 2, r.stderr +# --- The base-ref binding (ersatztv#698 route 1) ------------------------------------------------ +# +# `/pulls/{n}/files` diffs against the PR's LIVE base, so retargeting changes the answer without +# moving the head. Reproduced on the real instance as probe PR #703: opened into `main`, retargeted +# mid-run, enumerated docs-only, granted `review-verdict/h10=success` while its diff against `main` +# carried a C# file. + +def test_a_FOUR_argument_call_is_a_usage_error_not_an_unbound_enumeration(enumerate_files): + """The binding is REQUIRED, not optional. + + This is the test that matters most for the shape of the fix. Had the base ref been added as an + OPTIONAL 5th argument, every existing caller would have kept compiling and kept running + unbound — an opt-out that is invisible at the call site, and the caller most likely to omit it + is the one that most needed it. Dropping the argument must be LOUD. + """ + enumerate_files.set_pages(_rows(["docs/a.md"])) + r = enumerate_files.run(expected_base=None) + assert r.returncode == 2, ( + "a 4-argument call was accepted, so the base binding is effectively optional and any caller " + f"that forgets it silently enumerates against a mutable base (stdout={r.stdout!r})") + + +def test_an_EMPTY_base_ref_argument_fails_closed(enumerate_files): + """The hook passes `.base.ref` straight from PR JSON; unparseable JSON yields an empty string.""" + enumerate_files.set_pages(_rows(["docs/a.md"])) + r = enumerate_files.run(expected_base="") + assert r.returncode == 2, r.stderr + + +def test_a_PR_already_retargeted_before_the_enumeration_fails_closed(enumerate_files): + """The pre-paging read. Without it, a PR retargeted before the job started would enumerate + against the scratch base with every page agreeing with every other page — internally consistent + and entirely wrong.""" + enumerate_files.set_pages(_rows(["docs/a.md"])) + enumerate_files.base_is_already("probe/scratch-base") + assert enumerate_files.fails_closed() + + +def test_a_RETARGET_DURING_the_enumeration_fails_closed(enumerate_files): + """The post-paging read. The head never moves here, which is the entire point: the existing + head-sha binding cannot see a retarget, so removing the base half of the binding leaves this + case exempted.""" + enumerate_files.set_pages(_rows(["docs/a.md"])) + enumerate_files.base_moves_to("probe/scratch-base") + assert enumerate_files.fails_closed() + + +def test_positive_control_an_unmoved_base_still_enumerates(enumerate_files): + """Without this, the three tests above could pass because the 5-argument call is broken outright + rather than because the binding works.""" + enumerate_files.set_pages(_rows(["docs/a.md", "ErsatzTV/Program.cs"])) + assert enumerate_files.paths() == ["docs/a.md", "ErsatzTV/Program.cs"] + + +def test_the_base_comparator_is_the_BRANCH_NAME_never_the_TIP_SHA(): + """`.base.ref` is compared, never `.base.sha` — matching `post-review-verdict.sh` (ersatztv#632). + + Comparing tips would fail every enumeration on every unrelated merge to `main`: a self-inflicted + deadlock dressed as a security control. + + This is a STRUCTURAL assertion on purpose, and the previous version of this test is why. It was + written behaviourally as `head_moves_to(SHA)` — the sha that was ALREADY current — so it modelled + no movement at all and was simply a duplicate positive control. It would have passed just as + happily against a script comparing tip shas. The stub serves only branch names, so no behavioural + test in this harness can distinguish the two comparators; say so and assert the source instead. + """ + src = SCRIPT.read_text() + assert ".base.ref" in src, "the enumeration no longer reads .base.ref" + binding = [ln for ln in src.splitlines() + if "base_before=" in ln or "base_after=" in ln] + assert binding, "no base binding assignments found" + for ln in binding: + assert ".base.ref" in ln and ".base.sha" not in ln, ( + f"the base binding compares a tip sha, which deadlocks on ordinary churn: {ln.strip()!r}") + + def test_a_SHORT_page_does_not_end_the_enumeration(enumerate_files): """Gitea caps `limit` at the server-wide MAX_RESPONSE_ITEMS and may return fewer than asked. 'Fewer than 50 rows means last page' would complete over a partial list without any transport @@ -458,6 +559,43 @@ def test_the_caller_uses_the_shared_script(caller): f"{caller.relative_to(REPO_ROOT)} no longer calls the shared enumeration") +@CALLERS +def test_the_caller_passes_the_BASE_REF_argument(caller): + """Both callers must invoke the 5-argument form (ersatztv#698 route 1). + + The runtime tests prove the SCRIPT rejects a 4-argument call. They cannot prove a caller still + makes a 5-argument one, and the failure is quiet in opposite directions at the two sites: the + workflow would die on `set -u` (fail-closed, but it takes every PR with it), while the hook would + pass an empty base and lose every docs-only exemption. Pin the call site itself. + + Matching is deliberately narrow. A first draft keyed on "any line mentioning the script name" and + matched the enum_error MESSAGE string, failing for a reason that had nothing to do with the call. + The workflow also invokes through `"$ENUM"` rather than the literal path, so the alias is resolved + here and asserted to point at the shared script — otherwise this test could be satisfied while + `ENUM` pointed somewhere else entirely. + """ + code = _code_lines(caller) + if '"$ENUM"' in code: + assert re.search(r'^\s*ENUM=\S*scripts/pr-changed-files\.sh\s*$', code, re.M), ( + f"{caller.relative_to(REPO_ROOT)} invokes \"$ENUM\" but ENUM is not assigned the shared " + "enumeration script") + invocations = [ln for ln in code.splitlines() + if re.search(r'\$\(\s*"(\$ENUM|[^"]*pr-changed-files\.sh)"', ln)] + assert invocations, ( + f"{caller.relative_to(REPO_ROOT)} has no executable call to the shared enumeration") + for ln in invocations: + after = ln.split('"', 2)[2] if '"$ENUM"' in ln else ln.split("pr-changed-files.sh", 1)[1] + args = re.findall(r'"[^"]*\$[^"]*"', after) + assert len(args) >= 5, ( + f"{caller.relative_to(REPO_ROOT)} calls the enumeration with {len(args)} quoted " + f"arguments, expected 5 including the expected base ref: {ln.strip()!r}") + # Counting five arguments is not enough — cold review caught that passing `"$SHA"` twice + # satisfied the count while stalling every real exemption. Name the fifth. + assert re.search(r'(?i)base', args[4]), ( + f"{caller.relative_to(REPO_ROOT)} passes {args[4]} as the 5th argument; it must be the " + f"expected BASE ref: {ln.strip()!r}") + + @CALLERS def test_the_caller_does_not_reimplement_the_enumeration(caller): """Structural rather than behavioural on purpose. Behavioural equivalence tests would still pass @@ -659,6 +797,22 @@ 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") + 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 + # is still enumerating — the window the first read structurally cannot see. + nth = int(mode.split(":", 1)[1]) + ctr = out / "status_reads.txt" + 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(json.dumps({"statuses": [ + {"context": "review-verdict/h10", "status": "failure", + "creator": {"login": "timothy"}, + "description": "Review-verdict: BLOCKED @ a9e3e23 (base: main)"}]})) + sys.exit(0) if mode == "transport-error": # Real `gh()` is `curl -sf`: an HTTP error exits 22 with EMPTY stdout. sys.exit(22) @@ -671,9 +825,18 @@ if "/status" in url: # a `success` decoy triggers the very same short-circuit as a real verdict, so dropping # `select(.context == $c)` produced an identical outcome and the mutation survived. With # `pending` decoys, mis-selecting means no short-circuit — the job posts, and the test sees it. + # + # The row's PROVENANCE is configurable (ersatztv#698 route 3). A status POSTed by a user + # credential carries `.creator.login` and, for a real verdict, a `Review-verdict:` + # description; one POSTed by an Actions job carries `"creator": null`. Both were measured on + # the live combined endpoint. Defaults model a HUMAN verdict, so a test that does not opt out + # exercises the never-overwrite property. + creator = os.environ.get("STUB_STATUS_CREATOR", "timothy") + desc = os.environ.get("STUB_STATUS_DESC", "Review-verdict: MERGEABLE @ a9e3e23 (base: main)") print(json.dumps({"statuses": [ {"context": "Build & test (.NET)", "status": "pending"}, - {"context": "review-verdict/h10", "status": mode.split(":", 1)[1]}, + {"context": "review-verdict/h10", "status": mode.split(":", 1)[1], + "creator": ({"login": creator} if creator else None), "description": desc}, {"context": "Functional E2E", "status": "pending"}]})) sys.exit(0) print(json.dumps({"statuses": []})) @@ -684,7 +847,9 @@ print("{}") def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy", - status_mode: str = "none", jq16: bool = False): + status_mode: str = "none", jq16: bool = False, + status_creator: str | None = "timothy", + status_desc: str = "Review-verdict: MERGEABLE @ a9e3e23 (base: main)"): """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. @@ -706,6 +871,8 @@ def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy", env["PATH"] = f"{bindir}{os.pathsep}{env['PATH']}" env["STUB_DIR"] = str(tmp_path) env["STUB_STATUS_MODE"] = status_mode + env["STUB_STATUS_CREATOR"] = status_creator or "" + env["STUB_STATUS_DESC"] = status_desc env.update({ "GITEA_TOKEN": "stub", "BASE_URL": "http://gitea.example/api/v1", @@ -714,6 +881,10 @@ def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy", "PR": "42", "SHA": SHA, "BASE_SHA": OTHER_SHA, + # The base branch the event was raised for, threaded to the enumeration (ersatztv#698). + # Omitting it is not a soft failure: `set -u` kills the step, no status is posted, and the + # absent required check blocks the merge — fail-closed, but it would take every PR with it. + "BASE_REF": "main", "AUTHOR": author, "PR_URL": "http://gitea.example/timothy/ersatztv/pulls/42", }) @@ -944,3 +1115,430 @@ def test_DOCS_ONLY_anchors_the_START_of_the_path_too(tmp_path): posted, _ = _run_classify(tmp_path, _emitting("ErsatzTV/docs/Evil.cs")) assert posted is not None assert posted["state"] == "pending", "a path merely CONTAINING docs/ was accepted as docs-only" + + +# --- Route 2: a bot ACCOUNT does not attribute the CODE (ersatztv#698) -------------------------- +# +# `AUTHOR` is `pull_request.user.login` — the PR's CREATOR, which is immutable. The head a PR points +# at is not. Force-push application code onto an open Renovate branch and the PR is still authored by +# `renovate`, still touches no protected path, and was exempted. Checking the PUSHER instead does not +# help: a git author/committer is self-asserted text. So the exemption is constrained by what a +# dependency bump can legitimately BE. +# +# The allow-list is measured, not guessed: across all 11 Renovate PRs this repo has ever had, the +# paths touched were `Directory.Packages.props` (10) and `.config/dotnet-tools.json` (1). The one +# historical outlier, PR #20, touched a `.csproj` AND two C# files — and received an unattended bot +# exemption for a source change. + +def test_a_BOT_pr_carrying_a_CODE_file_is_NOT_exempt(tmp_path): + """Route 2, stated as a behaviour: the hijacked-branch case.""" + posted, _ = _run_classify(tmp_path, _emitting("ErsatzTV/Program.cs"), author="renovate") + assert posted is not None + assert posted["state"] == "pending", ( + "a PR authored by the renovate account was exempted while changing a C# file — the bot " + "identity is being read as attribution for code it did not write") + + +def test_a_BOT_pr_mixing_a_manifest_WITH_code_is_NOT_exempt(tmp_path): + """The realistic shape of the attack: keep the manifest edit so the PR still looks like a bump, + and smuggle the code alongside it. A rule that asked 'does it touch a manifest' rather than 'is + EVERY path a manifest' would exempt this.""" + posted, _ = _run_classify( + tmp_path, _emitting("Directory.Packages.props", "ErsatzTV/Program.cs"), author="renovate") + assert posted is not None + assert posted["state"] == "pending", ( + "a manifest edit was enough to carry a C# file through the bot exemption — the allow-list " + "is being applied as 'any' rather than 'all'") + + +def test_bot_positive_control_a_REAL_dependency_bump_IS_still_exempt(tmp_path): + """Renovate uses platformAutomerge, so breaking this deadlocks every dependency PR. The negative + tests above prove nothing if the exemption no longer works at all.""" + posted, _ = _run_classify(tmp_path, _emitting("Directory.Packages.props"), author="renovate") + assert posted is not None + assert posted["state"] == "success", ( + f"a plain dependency bump lost its exemption ({posted['description']!r}) — every Renovate PR " + "would now stall waiting on a human verdict") + + +@pytest.mark.parametrize("path", ["Directory.Packages.props", ".config/dotnet-tools.json"]) +def test_every_manifest_in_the_allow_list_is_actually_exempt(tmp_path, path): + """Pin the SET, not one example of it. A test that only ever exercises Directory.Packages.props + cannot see a typo in any of the other three alternations.""" + posted, _ = _run_classify(tmp_path, _emitting(path), author="renovate") + assert posted is not None, path + assert posted["state"] == "success", f"{path} is in the allow-list but was not exempted" + + +def test_the_manifest_allow_list_is_ANCHORED(tmp_path): + """Unanchored, `Directory.Packages.props` would match `evil/Directory.Packages.props.cs`. Same + class as the two DOCS_ONLY anchoring tests above, which is why it is tested the same way.""" + posted, _ = _run_classify(tmp_path, _emitting("evil/Directory.Packages.props.cs"), + author="renovate") + assert posted is not None + assert posted["state"] == "pending", "the manifest allow-list matched mid-path" + + +def test_a_BOT_docs_only_pr_is_STILL_exempt_via_the_docs_rule(tmp_path): + """The regression the restructure exists to prevent. + + Written as an `elif` chain, a Renovate PR touching only `docs/` enters the bot branch, fails the + manifest test, and never reaches the docs-only branch — silently withdrawing an exemption the + docs-only rule grants on its own merits for ANY author. The predicates are therefore evaluated + independently and the decision made afterwards. + """ + posted, _ = _run_classify(tmp_path, _emitting("docs/note.md"), author="renovate") + assert posted is not None + assert posted["state"] == "success", ( + "a docs-only PR lost its docs-only exemption merely because its author is a bot — the " + "exemptions are chained rather than composed") + + +def test_a_BOT_pr_touching_a_protected_path_is_still_NOT_exempt(tmp_path): + """PROTECTED must keep outranking both exemptions, including the manifest one.""" + posted, _ = _run_classify(tmp_path, _emitting("Directory.Packages.props", "scripts/evil.sh"), + author="renovate") + assert posted is not None + assert posted["state"] == "pending" + + +# --- Route 3: an inherited `success` is re-derived unless it is an attributable human verdict ---- +# +# The short-circuit used to exit on ANY existing `success`, so an exemption this job wrote was +# indistinguishable from a verdict a human wrote. Obtained once, a forged success was thereafter +# accepted unchanged on every later run, because the guard exited before looking at the PR, the base, +# the author or the files. +# +# MEASURED on Gitea 1.25.4, on the COMBINED endpoint this job reads: a status POSTed with a user +# credential carries `.creator.login` (`timothy`), one POSTed by an Actions job carries +# `"creator": null`. Both halves are required, and the test is written in the POSITIVE direction — +# short-circuit only on something identified as a human verdict — so an unrecognised shape is +# re-derived rather than trusted. + +def test_a_MACHINE_written_exemption_success_is_RE_DERIVED_not_inherited(tmp_path): + """Route 3. The status looks exactly like one this job writes: creator null, `Exempt:` wording. + + The PR now carries a C# file, so re-deriving must downgrade it to `pending`. Inheriting it would + leave a forged exemption standing forever. + """ + posted, _ = _run_classify(tmp_path, _emitting("ErsatzTV/Program.cs"), + status_mode="existing:success", + status_creator=None, + status_desc="Exempt: docs-only change (no code, no protected path)") + assert posted is not None, ( + "an existing machine-written success was inherited unchanged — this is route 3, and it is " + "how a forgery obtained once survives every subsequent run") + assert posted["state"] == "pending" + + +def test_a_success_with_NO_creator_but_a_VERDICT_LOOKING_description_is_re_derived(tmp_path): + """Isolates the creator half. Both conditions are required; either alone is forgeable by the + other party. If a future Gitea populates `creator` for Actions, the description half still + fails — the guard degrades toward re-deriving, never toward trusting.""" + posted, _ = _run_classify(tmp_path, _emitting("ErsatzTV/Program.cs"), + status_mode="existing:success", + status_creator=None, + status_desc="Review-verdict: MERGEABLE @ a9e3e23 (base: main)") + assert posted is not None, "a creatorless status was trusted on the strength of its description" + assert posted["state"] == "pending" + + +def test_a_success_with_a_creator_but_an_EXEMPT_description_is_re_derived(tmp_path): + """Isolates the description half, the mirror of the test above.""" + posted, _ = _run_classify(tmp_path, _emitting("ErsatzTV/Program.cs"), + status_mode="existing:success", + status_creator="timothy", + status_desc="Exempt: docs-only change (no code, no protected path)") + assert posted is not None, "a status was trusted on the strength of its creator alone" + assert posted["state"] == "pending" + + +def test_an_UNRECOGNISED_success_shape_is_re_derived_rather_than_trusted(tmp_path): + """The direction-of-test check. Written as 'skip if it looks machine-written', anything novel + would fall through to TRUSTED. Written as 'skip only if positively identified as human', novel + shapes are re-derived. This test fails under the first spelling and passes under the second.""" + posted, _ = _run_classify(tmp_path, _emitting("ErsatzTV/Program.cs"), + status_mode="existing:success", + status_creator="", status_desc="") + assert posted is not None + assert posted["state"] == "pending" + + +@pytest.mark.parametrize("existing", ["success", "failure"]) +def test_a_REAL_human_verdict_is_still_NEVER_overwritten(tmp_path, existing): + """The property the short-circuit exists for, which the fix must not break. + + `failure` is the sharp case: that is a human saying NO, and an exemption posted over it would + turn a rejection into a merge. Deliberately paired with a docs-only file list, so the job WOULD + have posted an exemption `success` had it not short-circuited — without that, a passing test + would prove only that nothing was posted for some unrelated reason. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), + status_mode=f"existing:{existing}", + status_creator="timothy", + status_desc=f"Review-verdict: MERGEABLE @ a9e3e23 (base: main)") + assert r.returncode == 0, r.stderr + assert posted is None, ( + f"overwrote a human '{existing}' verdict written by timothy — the never-overwrite property " + "has been lost while fixing route 3") + + +def test_a_PENDING_status_from_a_previous_run_is_replaced_normally(tmp_path): + """`pending` was never short-circuited and must stay that way, or a PR that becomes exempt after + an earlier pending run could never reach `success`.""" + posted, _ = _run_classify(tmp_path, _emitting("docs/a.md"), status_mode="existing:pending", + status_creator=None, status_desc="Awaiting review verdict for a9e3e23") + assert posted is not None + assert posted["state"] == "success" + + +@pytest.mark.parametrize("path", ["web/package.json", "web/package-lock.json"]) +def test_the_npm_manifests_are_NOT_exempt(tmp_path, path): + """Cross-family review called an earlier draft's inclusion of these a Blocker, correctly. + + `renovate.json` sets `enabledManagers: ["nuget", "github-actions", "dockerfile"]`, so Renovate does + not manage npm here at all — the entry bought nothing. Meanwhile `package.json` carries `scripts` + that CI EXECUTES (`npm ci`, `npm run build`), so exempting it lets a hijacked bot branch run + arbitrary shell in CI while every path still matches a "manifest" allow-list. Widening an exemption + to a code-execution vector for no operational benefit is strictly worse than the hole being closed. + """ + posted, _ = _run_classify(tmp_path, _emitting(path), author="renovate") + assert posted is not None + assert posted["state"] == "pending", ( + f"{path} was exempted; package.json scripts execute in CI, and npm is not a managed ecosystem " + "in this repo") + + +# --- The `grep -q` + `pipefail` inversion (found by cross-family review of this PR) --------------- +# +# `grep -q` exits at its FIRST match, so the upstream writer takes SIGPIPE (141) once the path list +# exceeds the pipe buffer. Under `set -o pipefail` the pipeline is then a FAILURE even though grep +# MATCHED, inverting every guard built on `printf … | grep -q`. Reproduced at 171KB / 1901 paths, +# comfortably inside the enumerator's 2000-file cap. +# +# These are the regression guards, and they are the tests the original round did not have: every +# earlier case used a handful of short paths, far below the buffer, so the whole class was invisible. +# The construct PREDATES #698, so `main` carried this hole with no retarget or bot account required. + +def _many_docs(n=1900): + return [f"docs/{'d' * 40}-{i:040d}.md" for i in range(n)] + + +def test_a_LARGE_pr_with_a_code_file_is_not_classified_docs_only(tmp_path): + """The code file goes FIRST so `grep -qv` matches immediately and the writer is left with ~171KB + still to push — the exact shape that produced exit 141 and `docs_only=yes`.""" + posted, r = _run_classify(tmp_path, _emitting("A.cs", *_many_docs())) + assert posted is not None, f"the job posted nothing: {r.stderr[-2000:]}" + assert posted["state"] == "pending", ( + "a PR containing A.cs was exempted as docs-only because the classification pipeline was " + f"inverted by SIGPIPE on a large file list (desc={posted['description']!r})") + + +def test_a_LARGE_pr_touching_a_PROTECTED_path_still_voids_the_exemptions(tmp_path): + """The worse direction: here the inversion makes the PROTECTED guard MISS, so a PR editing the + gate's own workflow falls through to the docs-only exemption and self-exempts.""" + posted, r = _run_classify(tmp_path, _emitting(".gitea/workflows/review-verdict.yml", *_many_docs())) + assert posted is not None, f"the job posted nothing: {r.stderr[-2000:]}" + assert posted["state"] == "pending", ( + "a PR editing .gitea/workflows/ was exempted — the protected-path guard was skipped on a " + f"large file list (desc={posted['description']!r})") + + +def test_a_LARGE_bot_pr_with_a_code_file_is_not_manifest_exempt(tmp_path): + """The same inversion reached through the third guard added by this change.""" + posted, r = _run_classify(tmp_path, _emitting("A.cs", *_many_docs()), author="renovate") + assert posted is not None, f"the job posted nothing: {r.stderr[-2000:]}" + assert posted["state"] == "pending" + + +def test_positive_control_a_LARGE_genuinely_docs_only_pr_IS_still_exempt(tmp_path): + """Without this the three tests above could pass because large lists now fail outright, which + would be a merge deadlock rather than a fix.""" + posted, r = _run_classify(tmp_path, _emitting(*_many_docs())) + assert posted is not None, f"the job posted nothing: {r.stderr[-2000:]}" + assert posted["state"] == "success", ( + f"a large but genuinely docs-only PR lost its exemption (desc={posted['description']!r})") + + +def test_a_human_verdict_landing_MID_RUN_is_not_overwritten(tmp_path): + """ersatztv#706, the half that IS mitigated here. + + The first status read finds nothing, so the job proceeds to classify. A reviewer then posts a human + `failure` (BLOCKED). Without the re-read immediately before the POST, this docs-only PR would post + an exemption `success` OVER an explicit human rejection — turning a "no" into a merge, which is the + worst outcome in this class. + + Note precisely what this does and does not prove: it pins the re-read, not atomicity. A verdict + landing between the re-read and the POST is still lost — there is no compare-and-set on Gitea's + status API. That remainder is #706, not a claim made here. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), + status_mode="appears-on-read:2") + assert r.returncode == 0, r.stderr + assert posted is None, ( + "an exemption success was posted over a human BLOCKED verdict that landed while the job was " + "classifying — the pre-POST re-read is missing or ineffective") + + +def test_positive_control_no_late_verdict_still_posts_normally(tmp_path): + """Without this, the test above would pass against a job that had simply stopped posting.""" + posted, _ = _run_classify(tmp_path, _emitting("docs/a.md"), status_mode="none") + assert posted is not None and posted["state"] == "success" + + +# --- The PROTECTED branch must actually EXECUTE, not merely coincide with the right answer -------- +# +# Round-3 review found `count_matching` being called before its definition, so it was +# `command not found` on every run and the PROTECTED branch never fired. Three "protected path" tests +# passed anyway, because a protected path is also not a manifest and not docs-only, so the job reached +# `pending` down a different route. Asserting the STATE could not see it; the guard was dead and the +# suite was green. +# +# The lesson generalises: when several branches produce the same outcome, asserting the outcome cannot +# tell you which branch ran. Assert the DISCRIMINATOR — here the reason string the branch writes. + +def test_a_protected_path_is_rejected_BY_THE_PROTECTED_BRANCH(tmp_path): + posted, r = _run_classify(tmp_path, _emitting("scripts/evil.sh", "docs/a.md")) + assert posted is not None, f"the job posted nothing: {r.stderr[-2000:]}" + assert posted["state"] == "pending" + # The DISCRIMINATOR is the job's `Decision:` line, not the status description: for `pending` the + # description is always "Awaiting review verdict for ", identical no matter which branch + # produced it. A first draft of this test asserted on the description and failed against a WORKING + # guard — the assertion has to be aimed at something that actually differs per branch. + assert "protected" in r.stdout.lower(), ( + "the PR was not exempted, but NOT via the protected-path branch — it reached the same verdict " + f"by another route, so that guard may be dead. Decision log:\n{r.stdout[-800:]}") + + +def test_a_protected_path_defeats_the_BOT_exemption_by_the_protected_branch(tmp_path): + """A bot PR whose every path IS a manifest, plus one protected path. Without a live PROTECTED + branch this still lands on `pending` (the protected file is not a manifest), so again only the + reason string distinguishes a working guard from a dead one.""" + posted, r = _run_classify( + tmp_path, _emitting("Directory.Packages.props", ".gitea/workflows/renovate.yml"), + author="renovate") + assert posted is not None + assert posted["state"] == "pending" + assert "protected" in r.stdout.lower(), ( + f"the bot exemption was refused, but not by the PROTECTED branch:\n{r.stdout[-800:]}") + + +@pytest.mark.parametrize("paths,author", [ + (("docs/a.md",), "timothy"), + (("Directory.Packages.props",), "renovate"), + (("ErsatzTV/Program.cs",), "timothy"), + (("scripts/evil.sh",), "timothy"), +]) +def test_the_classify_step_runs_without_SHELL_ERRORS(tmp_path, paths, author): + """A cheap, general trap-catcher for the whole step. + + `command not found`, `integer expression expected`, `unbound variable` — each of these silently + skips a branch inside an `if`/`elif` (the condition just evaluates false) while the job exits 0 and + posts a plausible status. That is precisely how the dead PROTECTED guard survived a green suite. + + Scope, stated so this is not mistaken for more than it is: it catches guards that die NOISILY on + stderr. It is NOT a general liveness check — a clean mutation such as hardcoding `n_protected=0` + emits none of these and passes here. That case is covered by the branch-discriminator test above, + which is the actual liveness guard; this one is the cheap net for the whole error-emitting family. + """ + _, r = _run_classify(tmp_path, _emitting(*paths), author=author) + bad = [ln for ln in r.stderr.splitlines() + if "command not found" in ln + or "integer expression expected" in ln + or "unbound variable" in ln + or "syntax error" in ln] + assert not bad, f"the classification step emitted shell errors, so a guard is not running: {bad}" + + +def test_a_human_verdict_formed_against_ANOTHER_BASE_is_not_inherited(tmp_path): + """Round-4 review: the sha-binding is escapable through the HUMAN verdict path. + + Get a genuine `success` on head H while it targets scratch base S (benign diff there), then + retarget H onto `main`, where its diff contains unreviewed code. Creator is real, prefix is real, + so the short-circuit preserved it — a green required check over code nobody reviewed. The + merge-consent hook compares the base, but that is advisory; a merge through the Gitea UI or API + only sees the status. + + `post-review-verdict.sh` already records the base it reviewed (`(base: …)`, ersatztv#632); this + asserts the gate actually READS it. + """ + posted, r = _run_classify(tmp_path, _emitting("ErsatzTV/Program.cs"), + status_mode="existing:success", + status_creator="timothy", + status_desc="Review-verdict: MERGEABLE @ a9e3e23 (base: probe/scratch-base)") + assert posted is not None, ( + "a human verdict formed against a DIFFERENT base was inherited unchanged — the reviewed diff " + f"is not this PR's diff. Log:\n{r.stdout[-600:]}") + assert posted["state"] == "pending" + + +def test_a_human_verdict_for_THIS_base_is_still_honoured(tmp_path): + """The positive control the test above needs: matching bases must still short-circuit, or the + check has simply broken every verdict.""" + posted, _ = _run_classify(tmp_path, _emitting("ErsatzTV/Program.cs"), + status_mode="existing:success", + status_creator="timothy", + status_desc="Review-verdict: MERGEABLE @ a9e3e23 (base: main)") + assert posted is None, "a verdict formed against THIS base was re-derived; the base check is too strict" + + +def test_a_LEGACY_verdict_with_no_recorded_base_is_still_honoured(tmp_path): + """Verdicts predating ersatztv#632 carry no `(base: …)`. Absent is deliberately not treated as a + mismatch: re-deriving over one would un-approve a genuinely reviewed head. Only a base that is + PRESENT and DIFFERENT is rejected.""" + posted, _ = _run_classify(tmp_path, _emitting("ErsatzTV/Program.cs"), + status_mode="existing:success", + status_creator="timothy", + status_desc="Review-verdict: MERGEABLE @ a9e3e23") + assert posted is None + + +def test_an_APPENDED_base_cannot_override_the_real_one(tmp_path): + """The description is attacker-influencable by anyone who can POST a status (#697), so the parse + must not be trickable into reading a second, appended base. + + This defeated the greedy `##` parse. The implementation no longer parses at all — it requires the + description to END with the exact literal marker AND to contain exactly one marker — so a second + appended marker makes the count 2 and is rejected outright. + """ + posted, r = _run_classify( + tmp_path, _emitting("ErsatzTV/Program.cs"), + status_mode="existing:success", status_creator="timothy", + status_desc="Review-verdict: MERGEABLE @ a9e3e23 (base: probe/scratch) (base: main)") + assert posted is not None, ( + "an appended '(base: main)' overrode the real recorded base, so a verdict formed elsewhere was " + f"inherited. Log:\n{r.stdout[-600:]}") + assert posted["state"] == "pending" + + +def test_an_EMPTY_recorded_base_is_treated_as_a_mismatch(tmp_path): + """`(base: )` is not 'absent' — it is present and not equal to the PR's base, so it fails closed + rather than being waved through by the legacy-verdict allowance.""" + posted, _ = _run_classify( + tmp_path, _emitting("ErsatzTV/Program.cs"), + status_mode="existing:success", status_creator="timothy", + status_desc="Review-verdict: MERGEABLE @ a9e3e23 (base: )") + assert posted is not None and posted["state"] == "pending" + + +def test_a_branch_name_containing_a_PAREN_cannot_truncate_into_the_current_base(tmp_path): + """Round-5 review, and the sharpest finding of the five: it needs no forgery and no #697. + + `main)evil` is a VALID git branch name (`git check-ref-format --branch 'main)evil'` succeeds). A + genuine verdict earned while head H targeted it is written `(base: main)evil)`. Any implementation + that extracts the value by truncating at the first `)` gets exactly `main`, matches a PR that now + targets `main`, and inherits a verdict covering a completely different diff. + + An earlier comment in the workflow asserted that a `)` in a branch name "mismatches — safe + direction". That was generalised from `feat/foo)bar` (which does mismatch) and is false for every + branch whose name STARTS with the target base. Hence the rule the code now follows: compare against + the exact expected literal, never parse a value out of attacker- or user-influenced text. + """ + posted, r = _run_classify( + tmp_path, _emitting("ErsatzTV/Program.cs"), + status_mode="existing:success", status_creator="timothy", + status_desc="Review-verdict: MERGEABLE @ a9e3e23 (base: main)evil)") + assert posted is not None, ( + "a verdict recorded against branch 'main)evil' was inherited by a PR targeting 'main' — the " + f"base value is being truncated at the first ')'. Log:\n{r.stdout[-600:]}") + assert posted["state"] == "pending"