From f523fc535d6d99ce13c231276672519e084442ba Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 29 Jul 2026 19:52:00 +0200 Subject: [PATCH 1/8] fix(698): bind the base, constrain the bot exemption by content, re-derive unattributable successes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `review-verdict/h10` exemption path decided from mutable or unattributed PR state, and a machine-written `success` was never revalidated. Three routes, one root cause, so one change. Route 1 (reproduced live as probe PR #703, closed unmerged): `/pulls/{n}/files` diffs against the PR's LIVE base, so retargeting moves the answer without moving the head sha. A PR opened into `main` and retargeted mid-run enumerated docs-only and was granted `h10=success` while its diff against `main` carried a C# file; retargeting back reclassified nothing. `scripts/pr-changed-files.sh` now takes the expected base branch as a REQUIRED 5th argument (optional would be a silent opt-out) and checks it before and after paging; the workflow passes it from the `pull_request_target` payload, which a retarget cannot rewrite, and `edited` is in `types:` so a retarget reclassifies. A pinned two-sha diff would close route 1 outright but Gitea 1.25.4 cannot serve one: `compare/{base}...{head}` returns no `files`, and a `--depth=1` fetch of the two shas has no merge base. Measured, not assumed. The residual window is stated in the code and the record rather than papered over. Route 2: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so pushing code onto an open Renovate branch kept the exemption. The bot exemption now also requires EVERY path to be a dependency manifest — a set measured across all 11 Renovate PRs this repo has had, not guessed. Route 3: the never-overwrite short-circuit exited on ANY `success`, so a forgery obtained once was inherited forever. It now fires only for a status positively identified as a human verdict (non-null `.creator.login` AND a `Review-verdict:` description — measured: user-posted statuses carry a creator, Actions-posted ones carry null). Written in the positive direction so an unrecognised shape is re-derived rather than trusted. The two exemptions are composed, not chained: as an `elif` chain a Renovate docs-only PR lost the docs-only exemption. Caught before commit and pinned by a test. Tests: 17 new cases in scripts/tests/test_pr_changed_files.py, each verified by mutating the clause it covers (8 mutations, 8 kills). Both records trimmed under the 60-line prose ceiling so the corpus tail-boundary check stays calibrated. Does NOT close the class: anyone who can POST a status directly can still impersonate a verdict — that is #697, deliberately left open. Refs: #698 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) --- .claude/hooks/pretooluse-merge-consent.sh | 13 +- .gitea/workflows/review-verdict.yml | 141 ++++++- docs/ci-cd.md | 26 ++ docs/decisions/README.md | 3 +- .../records/ci/exemption-provenance.md | 72 ++++ .../records/ci/gate-trigger-base-resolved.md | 14 +- .../records/ci/shared-pr-file-enumeration.md | 4 +- scripts/pr-changed-files.sh | 60 ++- scripts/tests/test_merge_consent_exemption.py | 10 +- scripts/tests/test_pr_changed_files.py | 350 +++++++++++++++++- 10 files changed, 645 insertions(+), 48 deletions(-) create mode 100644 docs/decisions/records/ci/exemption-provenance.md diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index 5271dc7ea..35efd0d5f 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -104,9 +104,20 @@ 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 diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index 237b699d2..e73f3cb24 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -88,10 +88,21 @@ 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`. 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 +164,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 +180,29 @@ 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). `web/package.json` / `web/package-lock.json` are included because Renovate manages the + # SPA's dependencies too and a first such PR must not deadlock. + # + # 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. + BOT_MANIFESTS='^(Directory\.Packages\.props|\.config/dotnet-tools\.json|web/package\.json|web/package-lock\.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/)' @@ -203,12 +244,57 @@ jobs: 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." + # --- WHOSE success 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 subsequent 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 good reason: re-posting `pending` over a real + # human verdict would un-approve a reviewed head and stall the PR. So the fix is not to + # delete it but to make it discriminate by PROVENANCE. + # + # MEASURED on this instance (Gitea 1.25.4), not assumed: a status POSTed with a USER + # credential — which is 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`. Confirmed on the COMBINED endpoint this job reads, not merely + # on `/statuses/{sha}`: 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 — a non-null creator AND the + # `Review-verdict:` description contract that `post-review-verdict.sh` is the sole writer of. + # Anything else, including anything we simply do not recognise, is RE-DERIVED. Written the + # other way round ("skip if it looks machine-written") an unrecognised shape would be + # trusted, which is the fail-open this issue is about. `null` never gets the benefit of the + # doubt: if a future Gitea starts populating `creator` for Actions, a machine exemption + # gains a creator but still fails the description test, so it is re-derived — the guard + # degrades toward re-deriving, never toward trusting. + # + # Note what this does NOT claim. A collaborator who can POST statuses directly can write + # both fields and impersonate a verdict; branch protection binds the CONTEXT, not its + # issuer, and no in-repo logic can fix that. That is #697, and it is why this is a + # provenance check rather than an authentication one. + existing_row=$(printf '%s' "$statusjson" \ + | jq -r --arg c "$CONTEXT" '[.statuses[] | select(.context == $c)] | first // {}') + existing=$(printf '%s' "$existing_row" | jq -r '.status // ""') + existing_creator=$(printf '%s' "$existing_row" | jq -r '.creator.login // ""') + existing_desc=$(printf '%s' "$existing_row" | jq -r '.description // ""') + + human_verdict=no + if [ -n "$existing_creator" ] && printf '%s' "$existing_desc" | grep -q '^Review-verdict:'; then + human_verdict=yes + fi + + if [ "$human_verdict" = yes ] && { [ "$existing" = "success" ] || [ "$existing" = "failure" ]; }; then + echo "${CONTEXT} is already '${existing}' on ${SHA:0:7}, written by '${existing_creator}' as a human verdict — leaving it alone." exit 0 fi + if [ -n "$existing" ]; then + echo "${CONTEXT} is '${existing}' on ${SHA:0:7} but is NOT an attributable human verdict (creator='${existing_creator:-null}', description='${existing_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 +317,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="" @@ -251,14 +337,43 @@ jobs: reason="no changed files could be read from the API — no exemption" elif printf '%s\n' "$files" | grep -qE "$PROTECTED"; 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. + is_bot=no + if printf '%s\n' "$BOTS" | tr ' ' '\n' | grep -qxF "$AUTHOR"; then is_bot=yes; fi + manifests_only=no + if ! printf '%s\n' "$files" | grep -qvE "$BOT_MANIFESTS"; then manifests_only=yes; fi + docs_only=no + if ! printf '%s\n' "$files" | grep -qvE "$DOCS_ONLY"; 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 diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 454ebfb2b..e85b3673e 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -794,12 +794,38 @@ 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`, `.config/dotnet-tools.json`, `web/package.json`, `web/package-lock.json` +(ersatztv#698). 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. +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. + 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): diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 972090d81..a23a87807 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -43,6 +43,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.decisions-lifecycle-flake` | When `decisions lifecycle` is the **only** red job, do not investigate and do not create a new run to clear it — no rebase, no `--amend`, no no-op push; the operator reruns that single job from the Gitea UI. | 2026-07-21 | [link](records/ci/decisions-lifecycle-flake.md) | | `ci.docs-only-detect-shallow-safe` | The docs-only detect script must diff against `FETCH_HEAD` (always resolves after `git fetch`, even shallow) using a two-dot tree diff — not `origin/` with three-dot — because a `fetch-depth: 1` shallow clone has no remote-tracking ref and no merge-base, which silently fails the original detect into `docs_only=false` (full matrix, no functional error). A CI-behavior change must be verified by measuring the effect (job durations), not just a green check. | 2026-07-17 | [link](records/ci/docs-only-detect-shallow-safe.md) | | `ci.docs-only-skip-steps` | A docs-only change must still run every required job (`test`, `migrations`) so their commit-status contexts always report; each heavy job runs `scripts/ci-detect-docs-only.sh` first and gates its real STEPS on `if: steps.detect.outputs.docs_only != 'true'`, never `if:`-skips the whole job (an `if:`-skipped job reports `skipped`, not `success`, which branch protection may never unblock on). Detection biases toward running more on any doubt. | 2026-07-17 | [link](records/ci/docs-only-skip-steps.md) | +| `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props`, `.config/dotnet-tools.json`, `web/package.json`, `web/package-lock.json`). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict, meaning a non-null `.creator.login` AND a `Review-verdict:` description; 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. | 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) | @@ -56,7 +57,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..764af59a2 --- /dev/null +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -0,0 +1,72 @@ +--- +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`, `.config/dotnet-tools.json`, `web/package.json`, `web/package-lock.json`). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict, meaning a non-null `.creator.login` AND a `Review-verdict:` description; 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.' +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, and +Gitea 1.25.4 cannot serve that — measured, not assumed: `compare/{base}...{head}` returns +`total_commits`/`commits` and **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. A full-history fetch per PR event was judged too expensive. 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. What +makes that acceptable is that it is no longer *durable* — the retarget back to `main` fires `edited`, +and the re-run re-derives. The two are one fix: `edited` without re-derivation re-runs and then exits +on the existing `success`; re-derivation without `edited` never gets its second run. + +**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); the `web/` manifests are included +so the first SPA bump does not deadlock. `*.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. + +**The shape occurs without an attacker, as a counterfactual.** Renovate PR #20 touched a `.csproj` and +two C# files, but has no `h10` status and was never exempted: it merged 2026-06-27, the gate landed +2026-07-25. The point is what identity-only *would* have done with it. An earlier draft claimed it had +actually 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 the job reads: 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. diff --git a/docs/decisions/records/ci/gate-trigger-base-resolved.md b/docs/decisions/records/ci/gate-trigger-base-resolved.md index e6298b13c..d47094a68 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,7 @@ 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/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..60e7d0ae4 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("{}") diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index e48af2dcb..14a553a6d 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,72 @@ 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_a_base_that_merely_ADVANCES_is_not_a_retarget(enumerate_files): + """`.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. The stub always serves branch NAMES, so this passes only + because the script never looks at a tip sha. + """ + enumerate_files.set_pages(_rows(["docs/a.md"])) + enumerate_files.head_moves_to(SHA) # head unchanged; base branch unchanged + assert enumerate_files.paths() == ["docs/a.md"] + + 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 +549,38 @@ 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}") + + @CALLERS def test_the_caller_does_not_reimplement_the_enumeration(caller): """Structural rather than behavioural on purpose. Behavioural equivalence tests would still pass @@ -671,9 +794,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 +816,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 +840,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 +850,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 +1084,179 @@ 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", + "web/package.json", "web/package-lock.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" -- 2.47.3 From bafb487eaa5f0b97956e67dec5fdb50fef7d7552 Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 29 Jul 2026 20:35:29 +0200 Subject: [PATCH 2/8] =?UTF-8?q?fix(698):=20review=20round=202=20=E2=80=94?= =?UTF-8?q?=20close=20a=20PRE-EXISTING=20SIGPIPE=20inversion,=20narrow=20t?= =?UTF-8?q?he=20bot=20allow-list,=20correct=20two=20overclaims?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-family adversarial review returned BLOCKED with 3 Blockers + 2 Highs. Two were verified independently before acting on them. BLOCKER 1 (pre-existing on main, not introduced here). `producer | grep -q…` inverts under `set -o pipefail`: grep exits at its first match, the producer takes SIGPIPE and exits 141 once the list exceeds the pipe buffer, so a MATCH is reported as a FAILED pipeline. Reproduced at 1901 paths / ~171KB, inside the enumerator's 2000-file cap: * `A.cs` + 1900 docs paths -> docs_only=yes, status 141 (a C# PR exempted) * `.gitea/…` + 1900 docs -> the PROTECTED guard MISSES entirely The second reaches a green required status on a PR rewriting the gate, with no retarget, no bot account and no forged status. This construct shipped with the original docs-only check (review-verdict.yml:257 on main), so it was live. Fixed in both the enforced gate and the advisory hook by using here-strings, which have no writer to signal. New record `ci.grep-q-pipefail-inversion`. BLOCKER 2 (mine). `web/package.json` / `web/package-lock.json` are removed from BOT_MANIFESTS. 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). It widened an exemption onto a code-execution path for no benefit. BLOCKER 3 + HIGH (documentation was wrong, code unchanged). The claim that `edited` made the retarget residual "non-durable" is retracted: runs are not serialized, so a stale run can post `success` after the reclassifying run posts `pending`. The ABA transition is narrowed and observable, NOT closed. Likewise the provenance check asks "posted by a user credential", not "posted by a reviewer" — ETV_STATUS_AUTH is basic auth, so a #697 forgery gets a non-null creator AND an attacker-chosen description and is preserved as human. Both now stated at full strength. TESTS. 4 large-input cases crossing the pipe buffer, each paired with a large-input POSITIVE control so "large lists now fail closed" (a deadlock) cannot pass as a fix. Verified by mutation: reverting the here-strings turns all three negatives red while the control stays green. Two of my own weak tests fixed — the "base advances" case called head_moves_to(SHA) with the already-current sha (a duplicate positive control, now a structural assertion that the comparator is .base.ref and never .base.sha), and the arity test counted five arguments without checking the fifth was the base. The whole class was invisible because every previous test used a handful of short paths: a guard whose behaviour depends on a buffer threshold needs a test that crosses it. Refs: #698 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) --- .claude/hooks/pretooluse-merge-consent.sh | 7 +- .gitea/workflows/review-verdict.yml | 35 +++++-- docs/ci-cd.md | 15 ++- docs/decisions/README.md | 1 + .../records/ci/exemption-provenance.md | 46 +++++---- .../records/ci/grep-q-pipefail-inversion.md | 43 ++++++++ scripts/tests/test_pr_changed_files.py | 98 +++++++++++++++++-- 7 files changed, 210 insertions(+), 35 deletions(-) create mode 100644 docs/decisions/records/ci/grep-q-pipefail-inversion.md diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index 35efd0d5f..fef4370ee 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -121,7 +121,12 @@ if files=$("$repo_root/scripts/pr-changed-files.sh" "$owner" "$repo" "$pr" "$sha files_complete=yes fi -if [ "$files_complete" = yes ] && [ -n "$files" ] && ! printf '%s\n' "$files" | grep -qvE '^(docs/|\.claude/|\.husky/|\.gitea/|.*\.md$)'; then +# The here-string is load-bearing, not style: `printf … | grep -q` inverts under `set -o pipefail` +# once the list exceeds the pipe buffer, because grep exits early and printf dies of SIGPIPE (141) — +# turning a MATCH into a failed pipeline and this negated test into a spurious docs-only exemption. +# Same defect as the enforced gate's (ersatztv#698); advisory here, so the blast radius is a missing +# prompt rather than a green required check, but the fix is identical. +if [ "$files_complete" = yes ] && [ -n "$files" ] && ! grep -qvE '^(docs/|\.claude/|\.husky/|\.gitea/|.*\.md$)' <<< "$files"; 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 e73f3cb24..8bf0f24e9 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -202,7 +202,15 @@ jobs: # 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. - BOT_MANIFESTS='^(Directory\.Packages\.props|\.config/dotnet-tools\.json|web/package\.json|web/package-lock\.json)$' + # 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/)' @@ -284,7 +292,7 @@ jobs: existing_desc=$(printf '%s' "$existing_row" | jq -r '.description // ""') human_verdict=no - if [ -n "$existing_creator" ] && printf '%s' "$existing_desc" | grep -q '^Review-verdict:'; then + if [ -n "$existing_creator" ] && grep -q '^Review-verdict:' <<< "$existing_desc"; then human_verdict=yes fi @@ -335,7 +343,7 @@ jobs: 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 grep -qE "$PROTECTED" <<< "$files"; then reason="touches a protected path (gate/CI/hooks/scripts/ci-image) — exemptions do not apply" else # The two exemptions are evaluated as INDEPENDENT predicates rather than as a chain. @@ -356,12 +364,27 @@ jobs: # 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. + # WHY HERE-STRINGS (`<<<`) AND NOT `printf … | grep -q` — a fail-open found by cross-family + # review and reproduced here. `grep -q` exits at its FIRST match; the upstream `printf` then + # takes SIGPIPE and exits 141 once the path list exceeds the pipe buffer (~64K). Under + # `set -o pipefail` the PIPELINE is therefore a failure even though grep MATCHED, which + # inverts every guard built on it: + # * `if ! … grep -qvE "$DOCS_ONLY"` sets docs_only=yes for a list containing a C# file; + # * `if … grep -qE "$PROTECTED"` MISSES, so a PR editing `.gitea/` skips the protected-path + # guard entirely and falls through to the exemptions. + # Reproduced with `A.cs` + 1900 docs paths (171KB, well inside the enumerator's 2000-file cap): + # `docs_only=yes`, pipeline status 141. This construct PREDATES #698 — it shipped with the + # original docs-only check — so it was a live hole on `main`, reachable with no retarget, no + # bot account and no forged status, by nothing more sinister than a large PR. + # A here-string is written to a temp file rather than a pipe, so the writer cannot be + # signalled and the exit status is grep's alone. `grep -c`/`grep -v` without `-q` are safe + # (they drain stdin) and are left as they are. is_bot=no - if printf '%s\n' "$BOTS" | tr ' ' '\n' | grep -qxF "$AUTHOR"; then is_bot=yes; fi + if grep -qxF "$AUTHOR" <<< "$(tr ' ' '\n' <<< "$BOTS")"; then is_bot=yes; fi manifests_only=no - if ! printf '%s\n' "$files" | grep -qvE "$BOT_MANIFESTS"; then manifests_only=yes; fi + if ! grep -qvE "$BOT_MANIFESTS" <<< "$files"; then manifests_only=yes; fi docs_only=no - if ! printf '%s\n' "$files" | grep -qvE "$DOCS_ONLY"; then docs_only=yes; fi + if ! grep -qvE "$DOCS_ONLY" <<< "$files"; then docs_only=yes; fi if [ "$is_bot" = yes ] && [ "$manifests_only" = yes ]; then exempt=yes diff --git a/docs/ci-cd.md b/docs/ci-cd.md index e85b3673e..38a76f636 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -795,8 +795,10 @@ gate must not be able to exempt itself from the gate. That includes Renovate's ` 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`, `.config/dotnet-tools.json`, `web/package.json`, `web/package-lock.json` -(ersatztv#698). An author match alone is not enough, because `pull_request.user.login` is the PR's +`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 @@ -819,12 +821,21 @@ used by the advisory hook `.claude/hooks/pretooluse-merge-consent.sh` (ersatztv# 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. Use a +here-string (`grep -q… <<< "$files"`). 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 diff --git a/docs/decisions/README.md b/docs/decisions/README.md index a23a87807..608963565 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -48,6 +48,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `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. Use a here-string (`grep -q… <<< "$data"`), which is written to a temp file and cannot signal its writer, so the exit status is grep's alone. `grep -c` and `grep -v` WITHOUT `-q` are safe because they drain stdin. 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) | diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md index 764af59a2..ef567c34e 100644 --- a/docs/decisions/records/ci/exemption-provenance.md +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -20,39 +20,46 @@ mid-flight. The job enumerated against the moved base, read docs-only, and poste `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, and -Gitea 1.25.4 cannot serve that — measured, not assumed: `compare/{base}...{head}` returns -`total_commits`/`commits` and **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. A full-history fetch per PR event was judged too expensive. 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. What -makes that acceptable is that it is no longer *durable* — the retarget back to `main` fires `edited`, -and the re-run re-derives. The two are one fix: `edited` without re-derivation re-runs and then exits -on the existing `success`; re-derivation without `edited` never gets its second run. +**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); the `web/` manifests are included -so the first SPA bump does not deadlock. `*.csproj` is excluded — under Central Package Management +`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. -**The shape occurs without an attacker, as a counterfactual.** Renovate PR #20 touched a `.csproj` and -two C# files, but has no `h10` status and was never exempted: it merged 2026-06-27, the gate landed -2026-07-25. The point is what identity-only *would* have done with it. An earlier draft claimed it had -actually been exempted — wrong, and the correction is kept because "was silently exempted" and "would -have been" are different claims. +**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 the job reads: a status POSTed with a user +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 @@ -70,3 +77,6 @@ a PR, because `pull_request_target` resolves this definition from `main` — the `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/grep-q-pipefail-inversion.md b/docs/decisions/records/ci/grep-q-pipefail-inversion.md new file mode 100644 index 000000000..7ce51f440 --- /dev/null +++ b/docs/decisions/records/ci/grep-q-pipefail-inversion.md @@ -0,0 +1,43 @@ +--- +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. Use a here-string (`grep -q… <<< "$data"`), which is written to a temp file and cannot signal its writer, so the exit status is grep''s alone. `grep -c` and `grep -v` WITHOUT `-q` are safe because they drain stdin. 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: '`if ! grep -qvE "$RE" <<< "$files"` / `if grep -qE "$RE" <<< "$files"`; regression tests `test_a_LARGE_pr_*` in `scripts/tests/test_pr_changed_files.py` build 1900+ paths (~171KB) to cross the pipe buffer' +--- + +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. + +**Why a here-string rather than dropping `-q`.** Dropping `-q` also works (grep then drains stdin), but +it relies on a subtle side effect and reads as a redundant `>/dev/null`. `<<<` removes the pipe +entirely: bash materialises the string, so there is no writer to signal and the status is unambiguous. +The cost is a temp file per evaluation, which is irrelevant at these sizes. + +**The testing lesson, which generalises past this bug.** 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/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 14a553a6d..4d05cbf8c 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -343,16 +343,26 @@ def test_positive_control_an_unmoved_base_still_enumerates(enumerate_files): assert enumerate_files.paths() == ["docs/a.md", "ErsatzTV/Program.cs"] -def test_a_base_that_merely_ADVANCES_is_not_a_retarget(enumerate_files): +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. The stub always serves branch NAMES, so this passes only - because the script never looks at a tip sha. + 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. """ - enumerate_files.set_pages(_rows(["docs/a.md"])) - enumerate_files.head_moves_to(SHA) # head unchanged; base branch unchanged - assert enumerate_files.paths() == ["docs/a.md"] + 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): @@ -579,6 +589,11 @@ def test_the_caller_passes_the_BASE_REF_argument(caller): 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 @@ -1130,8 +1145,7 @@ def test_bot_positive_control_a_REAL_dependency_bump_IS_still_exempt(tmp_path): "would now stall waiting on a human verdict") -@pytest.mark.parametrize("path", ["Directory.Packages.props", ".config/dotnet-tools.json", - "web/package.json", "web/package-lock.json"]) +@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.""" @@ -1260,3 +1274,71 @@ def test_a_PENDING_status_from_a_previous_run_is_replaced_normally(tmp_path): 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})") -- 2.47.3 From d8bd1dcba9a4029b85846e4c9b942d604299c44a Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 29 Jul 2026 20:53:36 +0200 Subject: [PATCH 3/8] =?UTF-8?q?fix(698):=20review=20round=203=20=E2=80=94?= =?UTF-8?q?=20count=20instead=20of=20matching,=20re-read=20before=20the=20?= =?UTF-8?q?POST,=20fix=20stale=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 cross-family review returned BLOCKED: 2 High + 3 Medium. HIGH — here-strings traded one fail-open for another. `grep -q… <<< "$data"` fixes the SIGPIPE inversion, but bash materialises a large here-string via temporary storage, so it fails when temp space is full or unwritable — and since these sit inside `if`/`!`, that failure flips the predicate exactly as SIGPIPE did. It did NOT reproduce on my bash 3.2, DID on the reviewer's Linux bash 5.x, and CI is Linux; the disagreement is itself the argument for a construct that cannot fail either way. Path predicates now COUNT with `grep -c`, which drains stdin (no early exit, no SIGPIPE) over an ordinary pipe (no temp file), and grep's status is read honestly: exit 1 means "zero matches", a legitimate answer, while >1 is a real error that FAILS THE JOB rather than silently reading as "no match". `set -e` does not catch these on its own — they sit in command substitution inside a conditional. Verified correct under 171KB input AND an unwritable TMPDIR. The description test became a `case` prefix match, removing another pipeline from a security predicate. New record `ci.grep-q-pipefail-inversion` covers the whole class. HIGH — a human verdict landing mid-run was still overwritten, and the code claimed otherwise. The job read statuses once, classified over several round-trips, then posted: a reviewer posting BLOCKED in between had it replaced by an exemption `success`, turning an explicit rejection into a merge. Added a re-read immediately before the POST which refuses to write over a human verdict found then. The heading no longer says "never overwrite" — it cannot promise that, since there is no compare-and-set on Gitea's status API. Remainder tracked as #706. MEDIUM — documentation was stale in three places, all mine. The record's frontmatter `rule:` still listed the npm manifests (I fixed the body and forgot the frontmatter, so the canonical rule AND the generated catalog were wrong); docs/ci-cd.md still said `edited` was absent from `types:`, contradicting a section I had just updated; and the workflow header still implied the `edited` re-run settles the ABA race. All corrected to say detection, not atomicity. TESTS. 373 pass. New: a mid-run human verdict via a status stub that returns nothing on the first read and BLOCKED on the re-read, and large-input regression tests for the ADVISORY hook, which had none — the copy with less authority is the one that quietly keeps a bug (#649's whole point). Mutation-verified: reverting the hook predicate, the workflow predicates, or the pre-POST re-read turns exactly the intended tests red while every positive control stays green. Refs: #698 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) --- .claude/hooks/pretooluse-merge-consent.sh | 22 +- .gitea/workflows/review-verdict.yml | 221 +++++++++++------- docs/ci-cd.md | 7 +- docs/decisions/README.md | 2 +- .../records/ci/exemption-provenance.md | 2 +- scripts/tests/test_merge_consent_exemption.py | 33 +++ scripts/tests/test_pr_changed_files.py | 42 ++++ 7 files changed, 229 insertions(+), 100 deletions(-) diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh index fef4370ee..ef81a9ea5 100755 --- a/.claude/hooks/pretooluse-merge-consent.sh +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -121,12 +121,22 @@ if files=$("$repo_root/scripts/pr-changed-files.sh" "$owner" "$repo" "$pr" "$sha files_complete=yes fi -# The here-string is load-bearing, not style: `printf … | grep -q` inverts under `set -o pipefail` -# once the list exceeds the pipe buffer, because grep exits early and printf dies of SIGPIPE (141) — -# turning a MATCH into a failed pipeline and this negated test into a spurious docs-only exemption. -# Same defect as the enforced gate's (ersatztv#698); advisory here, so the blast radius is a missing -# prompt rather than a green required check, but the fix is identical. -if [ "$files_complete" = yes ] && [ -n "$files" ] && ! grep -qvE '^(docs/|\.claude/|\.husky/|\.gitea/|.*\.md$)' <<< "$files"; 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 8bf0f24e9..0ba8d9fed 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -99,6 +99,12 @@ name: Review verdict # 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] @@ -227,81 +233,80 @@ 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. + # --- 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. # - # 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. + # 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. # - # 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 - # --- WHOSE success 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 subsequent 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 good reason: re-posting `pending` over a real - # human verdict would un-approve a reviewed head and stall the PR. So the fix is not to - # delete it but to make it discriminate by PROVENANCE. - # - # MEASURED on this instance (Gitea 1.25.4), not assumed: a status POSTed with a USER - # credential — which is 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`. Confirmed on the COMBINED endpoint this job reads, not merely - # on `/statuses/{sha}`: 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 — a non-null creator AND the - # `Review-verdict:` description contract that `post-review-verdict.sh` is the sole writer of. - # Anything else, including anything we simply do not recognise, is RE-DERIVED. Written the - # other way round ("skip if it looks machine-written") an unrecognised shape would be - # trusted, which is the fail-open this issue is about. `null` never gets the benefit of the - # doubt: if a future Gitea starts populating `creator` for Actions, a machine exemption - # gains a creator but still fails the description test, so it is re-derived — the guard - # degrades toward re-deriving, never toward trusting. - # - # Note what this does NOT claim. A collaborator who can POST statuses directly can write - # both fields and impersonate a verdict; branch protection binds the CONTEXT, not its - # issuer, and no in-repo logic can fix that. That is #697, and it is why this is a - # provenance check rather than an authentication one. - existing_row=$(printf '%s' "$statusjson" \ - | jq -r --arg c "$CONTEXT" '[.statuses[] | select(.context == $c)] | first // {}') - existing=$(printf '%s' "$existing_row" | jq -r '.status // ""') - existing_creator=$(printf '%s' "$existing_row" | jq -r '.creator.login // ""') - existing_desc=$(printf '%s' "$existing_row" | jq -r '.description // ""') + # 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. + ex_human=no + case "$ex_desc" in + "Review-verdict:"*) + if [ -n "$ex_creator" ]; then ex_human=yes; fi + ;; + esac + } - human_verdict=no - if [ -n "$existing_creator" ] && grep -q '^Review-verdict:' <<< "$existing_desc"; then - human_verdict=yes - fi - - if [ "$human_verdict" = yes ] && { [ "$existing" = "success" ] || [ "$existing" = "failure" ]; }; then - echo "${CONTEXT} is already '${existing}' on ${SHA:0:7}, written by '${existing_creator}' as a human verdict — leaving it alone." + # --- 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 "$existing" ]; then - echo "${CONTEXT} is '${existing}' on ${SHA:0:7} but is NOT an attributable human verdict (creator='${existing_creator:-null}', description='${existing_desc}') — re-deriving it from the PR's current state rather than inheriting it." + 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. ------------------------- @@ -343,7 +348,7 @@ jobs: reason="${enum_error} — no exemption" elif [ "${count:-0}" -eq 0 ]; then reason="no changed files could be read from the API — no exemption" - elif grep -qE "$PROTECTED" <<< "$files"; then + elif [ "$(count_matching "$PROTECTED" "$files")" -gt 0 ]; then reason="touches a protected path (gate/CI/hooks/scripts/ci-image) — exemptions do not apply" else # The two exemptions are evaluated as INDEPENDENT predicates rather than as a chain. @@ -364,27 +369,53 @@ jobs: # 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. - # WHY HERE-STRINGS (`<<<`) AND NOT `printf … | grep -q` — a fail-open found by cross-family - # review and reproduced here. `grep -q` exits at its FIRST match; the upstream `printf` then - # takes SIGPIPE and exits 141 once the path list exceeds the pipe buffer (~64K). Under - # `set -o pipefail` the PIPELINE is therefore a failure even though grep MATCHED, which - # inverts every guard built on it: - # * `if ! … grep -qvE "$DOCS_ONLY"` sets docs_only=yes for a list containing a C# file; - # * `if … grep -qE "$PROTECTED"` MISSES, so a PR editing `.gitea/` skips the protected-path - # guard entirely and falls through to the exemptions. - # Reproduced with `A.cs` + 1900 docs paths (171KB, well inside the enumerator's 2000-file cap): - # `docs_only=yes`, pipeline status 141. This construct PREDATES #698 — it shipped with the - # original docs-only check — so it was a live hole on `main`, reachable with no retarget, no - # bot account and no forged status, by nothing more sinister than a large PR. - # A here-string is written to a temp file rather than a pipe, so the writer cannot be - # signalled and the exit status is grep's alone. `grep -c`/`grep -v` without `-q` are safe - # (they drain stdin) and are left as they are. + # HOW THE PATH PREDICATES ARE EVALUATED, and why neither obvious spelling is used. + # + # `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=$? + if [ "$st" -gt 1 ]; then + echo "::error::grep failed (status ${st}) evaluating a path predicate; refusing to classify." >&2 + exit 1 + 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; refusing to classify." >&2 + exit 1 + fi + printf '%s' "${out:-0}" + } + + # 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 - if grep -qxF "$AUTHOR" <<< "$(tr ' ' '\n' <<< "$BOTS")"; then is_bot=yes; fi + 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 ! grep -qvE "$BOT_MANIFESTS" <<< "$files"; then manifests_only=yes; fi + if [ "$(count_not_matching "$BOT_MANIFESTS" "$files")" -eq 0 ]; then manifests_only=yes; fi docs_only=no - if ! grep -qvE "$DOCS_ONLY" <<< "$files"; then docs_only=yes; fi + if [ "$(count_not_matching "$DOCS_ONLY" "$files")" -eq 0 ]; then docs_only=yes; fi if [ "$is_bot" = yes ] && [ "$manifests_only" = yes ]; then exempt=yes @@ -408,6 +439,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 38a76f636..fb4a5c4e9 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -859,9 +859,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 608963565..cdd20a3f6 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -43,7 +43,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.decisions-lifecycle-flake` | When `decisions lifecycle` is the **only** red job, do not investigate and do not create a new run to clear it — no rebase, no `--amend`, no no-op push; the operator reruns that single job from the Gitea UI. | 2026-07-21 | [link](records/ci/decisions-lifecycle-flake.md) | | `ci.docs-only-detect-shallow-safe` | The docs-only detect script must diff against `FETCH_HEAD` (always resolves after `git fetch`, even shallow) using a two-dot tree diff — not `origin/` with three-dot — because a `fetch-depth: 1` shallow clone has no remote-tracking ref and no merge-base, which silently fails the original detect into `docs_only=false` (full matrix, no functional error). A CI-behavior change must be verified by measuring the effect (job durations), not just a green check. | 2026-07-17 | [link](records/ci/docs-only-detect-shallow-safe.md) | | `ci.docs-only-skip-steps` | A docs-only change must still run every required job (`test`, `migrations`) so their commit-status contexts always report; each heavy job runs `scripts/ci-detect-docs-only.sh` first and gates its real STEPS on `if: steps.detect.outputs.docs_only != 'true'`, never `if:`-skips the whole job (an `if:`-skipped job reports `skipped`, not `success`, which branch protection may never unblock on). Detection biases toward running more on any doubt. | 2026-07-17 | [link](records/ci/docs-only-skip-steps.md) | -| `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props`, `.config/dotnet-tools.json`, `web/package.json`, `web/package-lock.json`). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict, meaning a non-null `.creator.login` AND a `Review-verdict:` description; 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. | 2026-07-29 | [link](records/ci/exemption-provenance.md) | +| `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict, meaning a non-null `.creator.login` AND a `Review-verdict:` description; 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) | diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md index ef567c34e..25a2905e5 100644 --- a/docs/decisions/records/ci/exemption-provenance.md +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -5,7 +5,7 @@ status: active since: '2026-07-29' supersedes: none superseded-by: none -rule: 'The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR''s live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR''s immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props`, `.config/dotnet-tools.json`, `web/package.json`, `web/package-lock.json`). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict, meaning a non-null `.creator.login` AND a `Review-verdict:` description; 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.' +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, meaning a non-null `.creator.login` AND a `Review-verdict:` description; 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]`' --- diff --git a/scripts/tests/test_merge_consent_exemption.py b/scripts/tests/test_merge_consent_exemption.py index 60e7d0ae4..07a1221b8 100644 --- a/scripts/tests/test_merge_consent_exemption.py +++ b/scripts/tests/test_merge_consent_exemption.py @@ -503,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 4d05cbf8c..7cf5027ef 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -797,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) @@ -1342,3 +1358,29 @@ def test_positive_control_a_LARGE_genuinely_docs_only_pr_IS_still_exempt(tmp_pat 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" -- 2.47.3 From d4c600149dbecea08575caa5125558bfe920aba9 Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 29 Jul 2026 21:13:56 +0200 Subject: [PATCH 4/8] =?UTF-8?q?fix(698):=20review=20round=204=20=E2=80=94?= =?UTF-8?q?=20the=20PROTECTED=20guard=20was=20DEAD;=20define=20before=20us?= =?UTF-8?q?e,=20fail=20closed,=20fix=20prescriptive=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 cross-family review returned BLOCKED with 3 Mediums. The first was serious and self-inflicted. THE PROTECTED GUARD WAS A NO-OP. Round 3's `count_matching` / `count_not_matching` helpers were defined AFTER the classification chain that calls them, so `count_matching` was `command not found` on every run, `$( )` yielded an empty string, `[ "" -gt 0 ]` errored, and the `elif` was simply skipped — the protected-path check never executed at all. Confirmed by direct execution before fixing. Three "protected path" tests stayed GREEN throughout, because a protected path is also not a manifest and not docs-only, so the job still reached `pending` down a different route. Asserting the STATE could not distinguish a working guard from a dead one. The mutation battery missed it too: I had mutated the predicates, not their reachability. Fixed three ways: * helpers are defined immediately after `gh()`, before any use; * the three counts are evaluated ONCE at TOP LEVEL and validated numeric, because `exit 1` inside `$( )` leaves only the subshell and, with the substitution sitting in a conditional, `set -e` never fires either — so a grep error had been silently reading as "no match". A non-numeric result now aborts with nothing posted, and an absent required check blocks the merge; * the helpers return a non-numeric sentinel instead of trying to `exit`. Verified: an invalid regex now exits 2 and posts NOTHING (previously it classified and posted). Renaming the helper at its definition turns six tests red. TESTS, aimed at the failure mode rather than the symptom: * assert the DISCRIMINATOR (the job's `Decision:` reason line), not the outcome — when several branches yield the same verdict, the verdict cannot tell you which ran. A first draft of this test asserted the status description and failed against a WORKING guard, because for `pending` the description is constant; * a cheap stderr sweep for `command not found` / `integer expression expected` / `unbound variable` across four representative PR shapes. Each of those makes an `if` condition merely false while the job exits 0 and posts a plausible status, so this catches a whole family of silently-skipped guards. DOCS. The record and ci-cd.md still PRESCRIBED the here-string that round 3 removed — following them would have reintroduced the temp-storage failure. Both now prescribe counting, define-before-use, top-level evaluation and numeric validation. The workflow's measurement paragraph still said the npm manifests "are included" three lines above the note saying they are excluded; corrected. 379 tests pass. Refs: #698 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/review-verdict.yml | 113 +++++++++++------- docs/ci-cd.md | 7 +- docs/decisions/README.md | 2 +- .../records/ci/grep-q-pipefail-inversion.md | 32 +++-- scripts/tests/test_pr_changed_files.py | 60 ++++++++++ 5 files changed, 160 insertions(+), 54 deletions(-) diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index 0ba8d9fed..7665613da 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -199,8 +199,7 @@ jobs: # # 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). `web/package.json` / `web/package-lock.json` are included because Renovate manages the - # SPA's dependencies too and a first such PR must not deadlock. + # (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# @@ -233,6 +232,56 @@ jobs: gh() { curl -sf -H "Authorization: token $GITEA_TOKEN" "$@"; } + # 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. + # + # HOW THE PATH PREDICATES ARE EVALUATED, and why neither obvious spelling is used. + # + # `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 @@ -342,13 +391,29 @@ 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 [ "$(count_matching "$PROTECTED" "$files")" -gt 0 ]; then + elif [ "$n_protected" -gt 0 ]; then reason="touches a protected path (gate/CI/hooks/scripts/ci-image) — exemptions do not apply" else # The two exemptions are evaluated as INDEPENDENT predicates rather than as a chain. @@ -369,53 +434,15 @@ jobs: # 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. - # HOW THE PATH PREDICATES ARE EVALUATED, and why neither obvious spelling is used. - # - # `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=$? - if [ "$st" -gt 1 ]; then - echo "::error::grep failed (status ${st}) evaluating a path predicate; refusing to classify." >&2 - exit 1 - 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; refusing to classify." >&2 - exit 1 - fi - printf '%s' "${out:-0}" - } - # 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 [ "$(count_not_matching "$BOT_MANIFESTS" "$files")" -eq 0 ]; then manifests_only=yes; fi + if [ "$n_not_manifest" -eq 0 ]; then manifests_only=yes; fi docs_only=no - if [ "$(count_not_matching "$DOCS_ONLY" "$files")" -eq 0 ]; then docs_only=yes; fi + if [ "$n_not_docs" -eq 0 ]; then docs_only=yes; fi if [ "$is_bot" = yes ] && [ "$manifests_only" = yes ]; then exempt=yes diff --git a/docs/ci-cd.md b/docs/ci-cd.md index fb4a5c4e9..fe7d76236 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -824,8 +824,11 @@ 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. Use a -here-string (`grep -q… <<< "$files"`). Full detail: `ci.grep-q-pipefail-inversion`. +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). diff --git a/docs/decisions/README.md b/docs/decisions/README.md index cdd20a3f6..c446dc8ca 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -48,7 +48,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `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. Use a here-string (`grep -q… <<< "$data"`), which is written to a temp file and cannot signal its writer, so the exit status is grep's alone. `grep -c` and `grep -v` WITHOUT `-q` are safe because they drain stdin. 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.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) | diff --git a/docs/decisions/records/ci/grep-q-pipefail-inversion.md b/docs/decisions/records/ci/grep-q-pipefail-inversion.md index 7ce51f440..6c7701c13 100644 --- a/docs/decisions/records/ci/grep-q-pipefail-inversion.md +++ b/docs/decisions/records/ci/grep-q-pipefail-inversion.md @@ -5,9 +5,9 @@ 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. Use a here-string (`grep -q… <<< "$data"`), which is written to a temp file and cannot signal its writer, so the exit status is grep''s alone. `grep -c` and `grep -v` WITHOUT `-q` are safe because they drain stdin. This applies to both the enforced gate `.gitea/workflows/review-verdict.yml` and the advisory hook `.claude/hooks/pretooluse-merge-consent.sh`.' +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: '`if ! grep -qvE "$RE" <<< "$files"` / `if grep -qE "$RE" <<< "$files"`; regression tests `test_a_LARGE_pr_*` in `scripts/tests/test_pr_changed_files.py` build 1900+ paths (~171KB) to cross the pipe buffer' +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 @@ -31,13 +31,29 @@ The second is the serious one: it reaches a green required status on a PR that r **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. -**Why a here-string rather than dropping `-q`.** Dropping `-q` also works (grep then drains stdin), but -it relies on a subtle side effect and reads as a redundant `>/dev/null`. `<<<` removes the pipe -entirely: bash materialises the string, so there is no writer to signal and the status is unambiguous. -The cost is a temp file per evaluation, which is irrelevant at these sizes. +**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. -**The testing lesson, which generalises past this bug.** 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 +**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/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 7cf5027ef..9393445b1 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -1384,3 +1384,63 @@ 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. + Any of them appearing on stderr means some guard is not running, whatever the verdict looks like. + """ + _, 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}" -- 2.47.3 From e7bae063858855dc3e9e956ea5f9a9b8806433e1 Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 29 Jul 2026 21:44:44 +0200 Subject: [PATCH 5/8] =?UTF-8?q?fix(698):=20review=20round=205=20=E2=80=94?= =?UTF-8?q?=20a=20human=20verdict=20formed=20against=20ANOTHER=20base=20is?= =?UTF-8?q?=20no=20longer=20inherited?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 cross-family review returned BLOCKED with a single Medium; the three round-3 items were confirmed actually fixed. THE SHA-BINDING WAS ESCAPABLE THROUGH THE HUMAN PATH, not the exemption path. The short-circuit identified a human verdict by creator + `Review-verdict:` prefix and then exited before looking at the base. So: earn a GENUINE `success` on head H while it targets a scratch base with a benign diff, then retarget H onto `main`, where its diff carries unreviewed code. Creator real, prefix real, status inherited — a green required check over code nobody reviewed. `post-review-verdict.sh` has recorded the reviewed base in the description since #632; this gate simply never read it. The merge-consent hook did compare it, but that is advisory and covers only its own path: a merge through the Gitea UI or API sees nothing but the status. The gate now rejects a verdict whose recorded base differs from the PR's. An ABSENT base is deliberately NOT a mismatch — verdicts predating #632 carry none, and re-deriving over one would un-approve a genuinely reviewed head. Only present-and-different is rejected, which is exactly the escape. Tests: the mismatch case, plus two positive controls (matching base still short-circuits; a legacy no-base verdict still short-circuits) so the check cannot pass by blanket rejection. Mutation-verified: removing the check reddens only the mismatch test. Also from round 4: sharpened the docstring of test_the_classify_step_runs_without_SHELL_ERRORS. It catches guards that die NOISILY; it is not a general liveness check, since a clean mutation like hardcoding n_protected=0 emits nothing. The branch-discriminator test is the actual liveness guard. Claiming otherwise would have made a cheap net look like a strong one. And fixed a dangling decision key I had just introduced: the base-in-description convention belongs to `release.verdict-status-check`, not the `ci.verdict-records-base` I invented — the breadcrumb hazard our own retrieval rules warn about. 382 tests pass. Refs: #698 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/review-verdict.yml | 26 ++++++++++ docs/decisions/README.md | 2 +- .../records/ci/exemption-provenance.md | 2 +- scripts/tests/test_pr_changed_files.py | 49 ++++++++++++++++++- 4 files changed, 76 insertions(+), 3 deletions(-) diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index 7665613da..e78229aad 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -315,12 +315,38 @@ jobs: # 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 + case "$ex_desc" in + *"(base: "*) + ex_base=${ex_desc##*"(base: "} + ex_base=${ex_base%%)*} + if [ "$ex_base" != "$BASE_REF" ]; then + ex_human=no + echo "${CONTEXT} on ${SHA:0:7} is a human verdict, but it was formed against base '${ex_base}' while this PR targets '${BASE_REF}' — the reviewed diff is not this PR's diff, so it is NOT treated as a verdict for this base." + fi + ;; + esac + fi } # --- Whose verdict is it? (ersatztv#698 route 3) ------------------------------------- diff --git a/docs/decisions/README.md b/docs/decisions/README.md index c446dc8ca..56de13093 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -43,7 +43,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.decisions-lifecycle-flake` | When `decisions lifecycle` is the **only** red job, do not investigate and do not create a new run to clear it — no rebase, no `--amend`, no no-op push; the operator reruns that single job from the Gitea UI. | 2026-07-21 | [link](records/ci/decisions-lifecycle-flake.md) | | `ci.docs-only-detect-shallow-safe` | The docs-only detect script must diff against `FETCH_HEAD` (always resolves after `git fetch`, even shallow) using a two-dot tree diff — not `origin/` with three-dot — because a `fetch-depth: 1` shallow clone has no remote-tracking ref and no merge-base, which silently fails the original detect into `docs_only=false` (full matrix, no functional error). A CI-behavior change must be verified by measuring the effect (job durations), not just a green check. | 2026-07-17 | [link](records/ci/docs-only-detect-shallow-safe.md) | | `ci.docs-only-skip-steps` | A docs-only change must still run every required job (`test`, `migrations`) so their commit-status contexts always report; each heavy job runs `scripts/ci-detect-docs-only.sh` first and gates its real STEPS on `if: steps.detect.outputs.docs_only != 'true'`, never `if:`-skips the whole job (an `if:`-skipped job reports `skipped`, not `success`, which branch protection may never unblock on). Detection biases toward running more on any doubt. | 2026-07-17 | [link](records/ci/docs-only-skip-steps.md) | -| `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict, meaning a non-null `.creator.login` AND a `Review-verdict:` description; 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.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 equal to the PR's — 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) | diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md index 25a2905e5..e8d0fb0ab 100644 --- a/docs/decisions/records/ci/exemption-provenance.md +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -5,7 +5,7 @@ status: active since: '2026-07-29' supersedes: none superseded-by: none -rule: 'The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR''s live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR''s immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict, meaning a non-null `.creator.login` AND a `Review-verdict:` description; 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`.' +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 equal to the PR''s — 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]`' --- diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 9393445b1..18c26c2d6 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -1435,7 +1435,11 @@ def test_the_classify_step_runs_without_SHELL_ERRORS(tmp_path, paths, author): `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. - Any of them appearing on stderr means some guard is not running, whatever the verdict looks like. + + 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() @@ -1444,3 +1448,46 @@ def test_the_classify_step_runs_without_SHELL_ERRORS(tmp_path, paths, author): 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 -- 2.47.3 From ef92b46dd2e9311aa8a56c10960670bcad36b782 Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 29 Jul 2026 22:11:08 +0200 Subject: [PATCH 6/8] fix(698): parse the recorded base at its FIRST occurrence, not its last MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-found while writing the round-5 review brief, by asking what an attacker who can influence the status description (#697) could do to the parse I had just added. `${ex_desc##*"(base: "}` is greedy, so it reads the LAST occurrence. A description of `Review-verdict: MERGEABLE @ abc1234 (base: probe/scratch) (base: main)` therefore parsed as `main`, matched the PR's base, and the verdict was inherited — reopening the exact hole the base check was added to close, one commit earlier. Measured both forms before choosing: first-match yields `probe/scratch`, mismatches, and fails closed. Two adjacent cases confirmed to fail in the safe direction: a `)` inside a branch name truncates the value (mismatch), and an empty `(base: )` is present-but-different (mismatch), so neither is waved through by the legacy-absent-base allowance. Tests for both, and the appended-base test is mutation-verified: restoring `##` reddens it. 384 tests pass. Refs: #698 Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/review-verdict.yml | 9 ++++++++- scripts/tests/test_pr_changed_files.py | 28 ++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index e78229aad..ba9a2ef9e 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -338,7 +338,14 @@ jobs: if [ "$ex_human" = yes ]; then case "$ex_desc" in *"(base: "*) - ex_base=${ex_desc##*"(base: "} + # FIRST occurrence (`#`), never the last (`##`). The description is influencable by + # anyone who can POST a status (#697), and with greedy matching a real + # `(base: probe/scratch)` followed by an appended `(base: main)` parses as `main` and + # is inherited. Taking the first occurrence yields the real base, which then + # mismatches and fails closed. Measured both ways before choosing. + # A `)` inside a branch name truncates the value, which also mismatches — safe + # direction. An empty `(base: )` mismatches too. + ex_base=${ex_desc#*"(base: "} ex_base=${ex_base%%)*} if [ "$ex_base" != "$BASE_REF" ]; then ex_human=no diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 18c26c2d6..767c352f0 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -1491,3 +1491,31 @@ def test_a_LEGACY_verdict_with_no_recorded_base_is_still_honoured(tmp_path): 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. + + With greedy `${ex_desc##*"(base: "}` this description parses as `main` and the verdict is inherited; + with first-match `${ex_desc#*"(base: "}` it parses as `probe/scratch`, mismatches, and fails closed. + Both forms were measured before choosing. + """ + 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" -- 2.47.3 From fe00e0d71f05e734b8a5c3366b902d6676fe12ec Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 30 Jul 2026 20:51:57 +0200 Subject: [PATCH 7/8] fix(698): compare the recorded base exactly, never parse it out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 5 returned BLOCKED with one High, and it needed no forgery and no #697 — just a branch name. `main)evil` IS A VALID GIT BRANCH NAME (`git check-ref-format --branch 'main)evil'` succeeds). A genuine human verdict earned while head H targeted it is written `(base: main)evil)`. Truncating at the first `)` yields exactly `main`, which matches a PR that has since been retargeted onto `main`, so the verdict is inherited over a completely different diff. I had asserted the opposite in a code comment one commit earlier — 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. Two attempts at extracting this value have now been defeated (`##` last-marker by an appended marker, `#` first-marker by this), so the lesson is the shape, not the off-by-one: do not parse a value out of user- or attacker-influenced text when you can compare against the exact expected literal instead. The description must now END with the literal `(base: )` AND contain exactly ONE marker — the marker count kills the append trick without having to decide which occurrence is authoritative. Pure shell (`${#}` arithmetic), no truncation to abuse. Verified across all six shapes, including a PR that legitimately targets `main)evil` (accepted) and `(base: )` (rejected). Absent markers remain accepted, since verdicts predating #632 carry none. Mutation-verified: restoring the truncating parse reddens only the new paren test, while the appended-marker, matching-base and legacy tests stay green. 385 tests pass. Note for the record: pytest has never executed inside the review sandbox in any of the five rounds, so the suite has only ever been run here. Refs: #698 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/review-verdict.yml | 52 +++++++++++++------ docs/decisions/README.md | 2 +- .../records/ci/exemption-provenance.md | 2 +- scripts/tests/test_pr_changed_files.py | 29 +++++++++-- 4 files changed, 63 insertions(+), 22 deletions(-) diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index ba9a2ef9e..4c5746315 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -336,23 +336,41 @@ jobs: ;; esac if [ "$ex_human" = yes ]; then - case "$ex_desc" in - *"(base: "*) - # FIRST occurrence (`#`), never the last (`##`). The description is influencable by - # anyone who can POST a status (#697), and with greedy matching a real - # `(base: probe/scratch)` followed by an appended `(base: main)` parses as `main` and - # is inherited. Taking the first occurrence yields the real base, which then - # mismatches and fails closed. Measured both ways before choosing. - # A `)` inside a branch name truncates the value, which also mismatches — safe - # direction. An empty `(base: )` mismatches too. - ex_base=${ex_desc#*"(base: "} - ex_base=${ex_base%%)*} - if [ "$ex_base" != "$BASE_REF" ]; then - ex_human=no - echo "${CONTEXT} on ${SHA:0:7} is a human verdict, but it was formed against base '${ex_base}' while this PR targets '${BASE_REF}' — the reviewed diff is not this PR's diff, so it is NOT treated as a verdict for this base." - fi - ;; - esac + # 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 } diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 56de13093..d04651101 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -43,7 +43,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.decisions-lifecycle-flake` | When `decisions lifecycle` is the **only** red job, do not investigate and do not create a new run to clear it — no rebase, no `--amend`, no no-op push; the operator reruns that single job from the Gitea UI. | 2026-07-21 | [link](records/ci/decisions-lifecycle-flake.md) | | `ci.docs-only-detect-shallow-safe` | The docs-only detect script must diff against `FETCH_HEAD` (always resolves after `git fetch`, even shallow) using a two-dot tree diff — not `origin/` with three-dot — because a `fetch-depth: 1` shallow clone has no remote-tracking ref and no merge-base, which silently fails the original detect into `docs_only=false` (full matrix, no functional error). A CI-behavior change must be verified by measuring the effect (job durations), not just a green check. | 2026-07-17 | [link](records/ci/docs-only-detect-shallow-safe.md) | | `ci.docs-only-skip-steps` | A docs-only change must still run every required job (`test`, `migrations`) so their commit-status contexts always report; each heavy job runs `scripts/ci-detect-docs-only.sh` first and gates its real STEPS on `if: steps.detect.outputs.docs_only != 'true'`, never `if:`-skips the whole job (an `if:`-skipped job reports `skipped`, not `success`, which branch protection may never unblock on). Detection biases toward running more on any doubt. | 2026-07-17 | [link](records/ci/docs-only-skip-steps.md) | -| `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict for THIS base, meaning a non-null `.creator.login` AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base equal to the PR's — 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.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) | diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md index e8d0fb0ab..73ca1f80d 100644 --- a/docs/decisions/records/ci/exemption-provenance.md +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -5,7 +5,7 @@ status: active since: '2026-07-29' supersedes: none superseded-by: none -rule: 'The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR''s live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR''s immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict for THIS base, meaning a non-null `.creator.login` AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base equal to the PR''s — 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`.' +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]`' --- diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index 767c352f0..1175c0313 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -1497,9 +1497,9 @@ 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. - With greedy `${ex_desc##*"(base: "}` this description parses as `main` and the verdict is inherited; - with first-match `${ex_desc#*"(base: "}` it parses as `probe/scratch`, mismatches, and fails closed. - Both forms were measured before choosing. + 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"), @@ -1519,3 +1519,26 @@ def test_an_EMPTY_recorded_base_is_treated_as_a_mismatch(tmp_path): 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" -- 2.47.3 From 57e33f99371e55bc5b7630d6d0ab24575bc31645 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 30 Jul 2026 21:02:41 +0200 Subject: [PATCH 8/8] chore(698): drop a trailing blank line at EOF Nit from review round 6 (git diff --check). No behaviour change. Refs: #698 Co-Authored-By: Claude Opus 5 (1M context) --- docs/decisions/records/ci/gate-trigger-base-resolved.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/decisions/records/ci/gate-trigger-base-resolved.md b/docs/decisions/records/ci/gate-trigger-base-resolved.md index d47094a68..975f6f3e9 100644 --- a/docs/decisions/records/ci/gate-trigger-base-resolved.md +++ b/docs/decisions/records/ci/gate-trigger-base-resolved.md @@ -69,4 +69,3 @@ treatment renames them and deadlocks merges unless branch protection is edited i 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). - -- 2.47.3