fix(698): bind the base, constrain the bot exemption by content, re-derive unattributable successes #705
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#705
Reference in New Issue
Block a user
fixes #698
The exemption path decided from mutable or unattributed PR state, and a machine-written
successwas never revalidated. Three routes, one root cause, so one change.Route 1 — retarget race (reproduced live, probe PR #703)
/pulls/{n}/filesdiffs against the PR's live base, so retargeting moves the answer without moving the head sha. OpenedH → mainso the trusted base definition ran, retargeted to a scratch base mid-flight: the job enumerated docs-only and postedh10=success — "Exempt: docs-only change". Retargeted back tomainand nothing reclassified (created_at == updated_at), leaving a PR intomainwhose diff carried a C# file behind a green required check. Closed unmerged, branches deleted, no forgedh10left anywhere.Fix:
scripts/pr-changed-files.shtakes the expected base branch as a required 5th argument, checked before and after paging. Required rather than optional deliberately — an optional binding on a shared security primitive is a silent opt-out, and the caller that forgets it is the one that needed it. The workflow passes it from thepull_request_targetpayload, which a retarget cannot rewrite.Why a binding and not a pinned diff, measured not assumed:
compare/{base}...{head}returnstotal_commits/commitsand nofileson Gitea 1.25.4, and a--depth=1fetch of the two shas has no merge base — three-dot is impossible, two-dot over-reports everythingmaingained since the branch point. A full-history fetch per PR event was judged too expensive. The residual window (a retarget opening and closing strictly between the files call and the re-read) is stated in the code and the record rather than papered over; it is covered byedited+ re-derivation making it non-durable.Route 2 — a bot ACCOUNT does not attribute the CODE
pull_request.user.loginis the PR's immutable creator; its head is not. 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 (Directory.Packages.props×10,.config/dotnet-tools.json×1, plus theweb/manifests so a first SPA bump can't deadlock), not guessed.Route 3 — inherited success
The 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.loginand aReview-verdict:description. Measured on the combined endpoint the job actually reads — user-posted statuses carry a creator, Actions-posted ones carrynull. Written in the positive direction so an unrecognised shape is re-derived rather than trusted; if Gitea later populatescreatorfor Actions, the description half still fails and the guard degrades toward re-deriving.Verification
scripts/tests/test_pr_changed_files.py.editedis measured, not assumed (the issue flagged it unverified). The self-test gap blocks testing onmain, so anedited-only probe workflow was put on a scratch base branch and a PR retargeted onto it — the run was created, which only happens if the event fired and thebranches:filter matched the new base. Honest limit: the job never got a runner slot, so I have the run's creation, not its log line.elifchain the new bot condition silently withdrew the docs-only exemption from Renovate docs PRs. Predicates are now composed, and a test pins it.Corrections made during the work
An earlier draft of the decision record claimed historical Renovate PR #20 had been exempted for a source change. Verified and false — #20 merged 2026-06-27, the gate landed 2026-07-25, and it carries no
h10status. It is now stated as the counterfactual it is, with the correction kept in the record.Does NOT close the class
Anyone who can POST a status directly can write both a creator and a
Review-verdict:description and impersonate a verdict; branch protection binds the context, not its issuer. That is #697, deliberately left open — its durable fix is credential scoping, partly server-management territory.Outstanding
The route 2 live reproduction (pushing code onto an open Renovate PR branch under a WIP guard) is not done: the permission classifier blocked the force-push needed to restore the branch afterwards, so I stopped rather than mutate a live dependency PR with no clean way back. #679 was left byte-identical to how it started. That Done-when box is therefore unticked on purpose.
Docs updated in this PR:
docs/ci-cd.md, new recordci.exemption-provenance,ci.gate-trigger-base-resolved(residual resolved),ci.shared-pr-file-enumeration(contract now 5 args), regenerated catalog.Review-verdict: BLOCKED @
f523fc5Cross-family adversarial review returned 3 Blockers + 2 Highs. Two verified independently by me. Blocker: grep -q under set -o pipefail inverts BOTH the docs-only and the PROTECTED guards once the path list exceeds the pipe buffer (grep exits early, printf takes SIGPIPE 141, pipefail turns a MATCH into a failed pipeline). Reproduced: a list of A.cs + 1900 docs paths classifies docs_only=yes, and a list led by .gitea/workflows/review-verdict.yml MISSES the PROTECTED guard entirely. This construct is PRE-EXISTING on main (review-verdict.yml:257), so main is currently exposed. Blocker: web/package.json + web/package-lock.json must come out of BOT_MANIFESTS -- renovate.json enables only nuget/github-actions/dockerfile (no npm), and package.json scripts execute in CI via npm ci / npm run build. Not merging.
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) <noreply@anthropic.com>Review-verdict: MERGEABLE @
57e33f9Six cross-family adversarial review rounds: BLOCKED (3 Blockers + 2 Highs) -> BLOCKED (2H + 3M) -> BLOCKED (3M) -> BLOCKED (1M) -> BLOCKED (1 High) -> MERGEABLE, no new findings. Round 6 verified the base comparison across branch-name, marker-count, quoting and whitespace cases and found no bypass. CI green on this head: all 11 substantive checks success, image push skipped (normal for PRs). 385 local tests pass; every new guard mutation-verified red-then-green with positive controls. Scope note: this head is one whitespace-only commit past the reviewed one, removing a trailing blank line that round 6 itself flagged. Known limitation: pytest never executed inside the review sandbox in any round (no writable temp), so the suite has only ever been run locally. Residuals tracked, not dropped: #706 (status writes unserialized), #707 (paging vs live base), #708 (route-2 live reproduction), #697 (credentials that can write the status).