timothy
8f6d4f4432
Three related defects in the `review-verdict/h10` gate, all surfaced by the cross-family review of PR #705. #706 race 1 — a stale run could overwrite a fresher verdict, permanently. The race was reproduced live rather than reasoned about (Gitea 1.25.4): with every other workflow stripped, probe PR #722 showed run 7520 (`opened`) finishing 20s AFTER run 7521 (`synchronize`) started. `pull_request_target` runs for one PR genuinely overlap, older finishing last. The issue proposed serializing with a non-cancelling concurrency group. That is REFUTED by measurement: with the group active, runs 7528/7529 still overlapped and 7528 ended 36s after 7529 began. A first probe appeared to show the group working — a negative control with no `concurrency:` key at all showed the same cancellations, revealing Gitea auto-cancels superseded `push` runs on its own and the probe had measured that, not the group. The auto-cancel does not extend to `pull_request_target`. The fix leaves the runs unserialized and instead makes an overtaken run decline to write: count `change_target_branch` events on the PR timeline at start and again just before the POST, and post nothing if the count moved. The COUNT is the key because the branch NAME is ABA-vulnerable (`main -> S -> main` reads `main` at both ends — how #698 route 1 forged its exemption). Abstaining is a handoff, not a stall: every retarget fires `edited`, so the event that makes a run abstain has already queued its successor. `updated_at` was rejected as the key precisely because it moves for comments/labels, which queue nothing. #706 race 2 — a human BLOCKED landing in the unclosable window between the pre-POST re-read and the POST was silently turned green. After an exemption `success` the job now re-reads the per-POST history and repairs its own status to `pending` if a human verdict appeared above a high-water mark taken just before the write. The repair is `pending`, never a copy of the human's state. The id comparison is load-bearing: a presence test would fire forever on a base-mismatched verdict and deadlock that PR's exemption. #707 — `pr-changed-files.sh` bound `.base.ref` and `.head.sha` across the enumeration but never `.base.sha`, so an ordinary advance of `main` mid-paging could drop a code path from an offset-paged diff and leave a complete-looking docs-only list. Now bound from the JSON already fetched (no new round trips). #711 — `.codex/` added to PROTECTED. It mirrors `.claude/hooks/` byte for byte, including the merge-consent hook, so the "a PR that can weaken the gate cannot exempt itself" rule had an incomplete path list. Latent today (untracked), live the moment anyone tracks it. Residuals are stated, not implied: a retarget inside the final round-trip, and the repair being itself a read-then-write. Gitea's status API has no compare-and-set, so neither reaches zero; both now fail toward `pending`. Tests: 398 pass in scripts/tests. Each new guard was mutation-checked — the fence's motion comparison, the untrusted-count gate, the repair POST and the id high-water mark were each neutered in turn and the intended test went red while its positive control stayed green. fixes #706 fixes #707 fixes #711 Decisions-Edit: yes
11 KiB
key, title, status, since, supersedes, superseded-by, rule, signals, mechanics
| key | title | status | since | supersedes | superseded-by | rule | signals | mechanics |
|---|---|---|---|---|---|---|---|---|
| ci.shared-pr-file-enumeration | 2026-07-26 — `scripts/pr-changed-files.sh` is the ONE enumeration of a PR's changed files; the advisory hook and the enforced gate share mechanism, never policy (#649) | active | 2026-07-26 | none | none | 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, base-ref binding — see `ci.exemption-provenance` — and base-TIP binding, #707: the ref answers "did this PR RETARGET", the tip answers "did the base ADVANCE mid-enumeration", and only the second can see `/pulls/{n}/files` recomputing each offset-paged page against a moved base and dropping a path out of an already-consumed range; both ends of the window are bound, and an advance BEFORE the window is deliberately not an error, or ordinary churn on `main` would fail every open PR) 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. | 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 | `scripts/pr-changed-files.sh <owner> <repo> <pr> <expected-head-sha> <expected-base-ref>` -> 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
be an ordinary duplication smell anywhere else; here it was actively dangerous, because the two
copies had unequal consequence. The advisory hook's failure mode is a human permission prompt — a
missed guard there just means a person gets asked instead of an automatic decision. The enforced
workflow's failure mode is a success write to review-verdict/h10, the one status branch
protection actually requires — a missed guard there merges an unreviewed PR with nobody asked at all.
The drift that motivated this. Four rounds of #643 hardening landed entirely on the copy with the
lower stakes. The hook accumulated CR/LF rejection, .. rejection, a closed .status allow-list,
previous_filename validation on every row (not just renamed), termination only on a validated
empty page, and head-sha binding — while the enforced workflow kept the original, weaker logic. Its
fail-closed behavior on a garbage API response was incidental (an empty n erroring a bash
conditional to false), not a designed property. The gate with real authority was strictly weaker than
the gate with none, which is the wrong way around by construction, not by anyone's mistake in a
single review — nothing in the original layout forced the two to move together.
Why the fix is "one script, two callers" rather than "copy the hardening across." Copying keeps
the two-implementation shape; the next hardening round would only need to happen twice again, and
there is no mechanism that would surface a second drift before it mattered. Extracting
scripts/pr-changed-files.sh makes the enumeration a single artifact with a single test suite
(scripts/tests/test_pr_changed_files.py), so a future guard is added once and both callers get it
atomically.
Mechanism, not policy — the two allow-lists stay separate on purpose. The extracted script answers exactly one question: "what is the complete set of paths this PR touches, at one head, or can we not tell?" It does not decide whether that set makes the PR docs-only. Each caller keeps its own classification:
- The hook's docs-only pattern also lets
.claude/,.gitea/,.husky/through, which is safe there only because a non-match falls through to a human prompt rather than an auto-grant. - The workflow's is narrower, because there a match posts a green status with nobody in the loop, and
both docs-only and Renovate exemptions are void when the PR touches
.claude/,.gitea/,.husky/,scripts/ordocker/ci/— the gate must not be able to exempt itself from review by editing itself.
Merging the two allow-lists would have quietly widened the enforced exemption to match the advisory one, turning a difference that exists for a reason into an accident of refactoring. Sharing the enumeration closes the drift that actually caused harm without touching the part that was correctly different.
What the shared script owns. Six guards, all now exercised by one test suite instead of a subset in each caller:
- CR/LF rejection and
..rejection on every path. - A closed
.statusallow-list —added/deleted/changed/modified/renamed/copied, not an open denylist. Notechangedanddeletedare the values live Gitea 1.25.4 actually emits;modifiedis accepted alongsidechangedbecause a closed list built from the wrong vocabulary would gate every genuine docs-only PR. GitHub'sremovedis deliberately not in the list — an earlier draft of this record said it was, which would have sent a maintainer looking for a value the code rejects. previous_filenamevalidated on every row the extraction consumes, not only rows whose.statusisrenamed— amodified/copiedrow can still carry it, and an earlier fix that validated only therenamedcase was found incomplete for exactly this reason (seeci.script-tests-jobfor the review trail).- Termination only on a validated empty page — Gitea's paging can return fewer rows than requested well before the real end of the list, so "short page" is not a valid termination signal.
- Head-sha binding: the head is re-read after enumeration, and the caller must refuse to trust the list if it moved mid-enumeration, since paging is several round-trips and a force-push between them would otherwise yield a list belonging to no single commit. This detects ONE-WAY movement only. An A→B→A force-push round trip restores the expected sha, so the binding holds while the pages came from two different states — see #664. Closing that needs a commit-pinned files endpoint (Gitea has none) or a local diff, not a tighter check here; the guarantee is stated narrowly rather than left to read as complete.
Base-ref checkout — binds the SCRIPTS to the base, not the workflow itself. review-verdict.yml
checks out the PR's BASE ref (ref: ${{ github.event.pull_request.base.sha }},
persist-credentials: false), never the head, so the scripts the job executes — above all
scripts/pr-changed-files.sh — come from the already-reviewed base rather than from the PR under
judgment. The checkout and its ref are the security-relevant parts: a bare run: calling the
script would not have been sufficient, since the script would then have come from wherever the
runner happened to be.
It does NOT mean a PR cannot rewrite the gate that judges it (#672). Gitea resolves a
pull_request workflow definition from the PR's own head, so a PR editing review-verdict.yml
runs its own rewritten copy — which can delete this checkout, or simply post
review-verdict/h10=success for its head sha and stop. Branch protection does not close that: it
requires the context, not an author, and carries required_approvals: 0. An earlier revision of
this paragraph said the workflow "cannot be rewritten by that same PR to weaken its own judgment",
which is true of the scripts and false of the workflow — and stated in the one sentence a reader
resolving this record from the catalog is most likely to stop at.
That half is now closed, elsewhere — see ci.gate-trigger-base-resolved (#672). The workflow
triggers on pull_request_target scoped to branches: [main], so Gitea resolves its definition from
the base rather than the head. The paragraph above is kept in the past tense rather than deleted
because it names the distinction this record turns on: the base-ref checkout binds the scripts, and
only the trigger binds the definition. Note the dependency runs the other way too — that checkout is
what makes pull_request_target safe to use at all here, since this job never executes head-supplied
code.
An earlier revision of this record stated the requirement in the future tense, because the wiring
was staged over two PRs: the workflow runs the BASE version of the gate, and until the shared script
existed on main a wired workflow would have exited 127 on its own PR and blocked the merge gate
through the combined status. That staging is complete. Both halves are asserted by
scripts/tests/test_pr_changed_files.py, which parses the workflow YAML rather than substring-
matching it — head.sha for base.sha is a nine-character diff, and a text-level check would still
pass if a second checkout step took the head afterwards and won.
What is deliberately NOT claimed. The PROTECTED
path list remains the guard that stops a bot-authored PR from editing the gate and exempting itself,
and mutation testing was what established that PROTECTED is load-bearing only on the BOT path —
it and DOCS_ONLY are disjoint patterns, so on the docs-only path that clause can never fire. A
test written against a docs-only-plus-protected file list passed with the clause deleted.
A commit status is repo-global, which this record does not fix either. review-verdict/h10 is
attached to a sha in the repository, not to a pull request, so a success earned on one PR is
inherited by any other PR with the same head — including one opened against a different base after
the first is closed (#663). That is the same property that makes #622's per-sha binding work, read
from the other end. Out of scope here; noted so the enumeration's guarantees are not mistaken for a
guarantee about which PR a verdict belongs to.
Severity, stated honestly. Every enumeration defect found in this area (#643) downgraded a mechanical deny/ask to a human prompt on the hook side; none produced a silent self-merge on their own. It is still a real weakening worth fixing — the whole point of #649 is that the same class of bug on the enforced copy would not have been merely a downgrade.