fix(698): bind the base, constrain the bot exemption by content, re-derive unattributable successes #705

Merged

timothy merged 8 commits from fix/698-exemption-provenance into main 2026-07-30 21:22:02 +02:00

Conversation 2 Commits 8 Files Changed 11

+1231 -76

8 Commits

Author	SHA1	Message	Date
timothyandClaude Opus 5	57e33f9937	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) <noreply@anthropic.com>	2026-07-30 21:02:41 +02:00
timothyandClaude Opus 5	fe00e0d71f	fix(698): compare the recorded base exactly, never parse it out ... 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: <this PR's 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) <noreply@anthropic.com>	2026-07-30 20:51:57 +02:00
timothyandClaude Opus 5	ef92b46dd2	fix(698): parse the recorded base at its FIRST occurrence, not its last ... 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) <noreply@anthropic.com>	2026-07-29 22:11:08 +02:00
timothyandClaude Opus 5	e7bae06385	fix(698): review round 5 — a human verdict formed against ANOTHER base is no longer inherited ... 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) <noreply@anthropic.com>	2026-07-29 21:44:44 +02:00
timothyandClaude Opus 5	d4c600149d	fix(698): review round 4 — the PROTECTED guard was DEAD; define before use, fail closed, fix prescriptive docs ... 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>	2026-07-29 21:13:56 +02:00
timothyandClaude Opus 5	d8bd1dcba9	fix(698): review round 3 — count instead of matching, re-read before the POST, fix stale docs ... 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) <noreply@anthropic.com>	2026-07-29 20:53:36 +02:00
timothyandClaude Opus 5	bafb487eaa	fix(698): review round 2 — close a PRE-EXISTING SIGPIPE inversion, narrow the bot allow-list, correct two overclaims ... 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) <noreply@anthropic.com>	2026-07-29 20:35:29 +02:00
timothyandClaude Opus 5	f523fc535d	fix(698): bind the base, constrain the bot exemption by content, re-derive unattributable successes ... 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) <noreply@anthropic.com>	2026-07-29 19:52:00 +02:00