fix(706,707,711): fence the review-verdict write on the timeline retarget count
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 11s
PR Gates / Docs update reminder (pull_request) Successful in 15s
PR Gates / decisions lifecycle (pull_request) Successful in 23s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 23s
review-verdict/h10 Awaiting review verdict for 8f6d4f4
Review verdict / Set review-verdict status (pull_request_target) Successful in 20s
PR Gates / Script tests (pytest) (pull_request) Successful in 1m29s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 1m26s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 16m56s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 20m59s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 22m41s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 11s
PR Gates / Docs update reminder (pull_request) Successful in 15s
PR Gates / decisions lifecycle (pull_request) Successful in 23s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 23s
review-verdict/h10 Awaiting review verdict for 8f6d4f4
Review verdict / Set review-verdict status (pull_request_target) Successful in 20s
PR Gates / Script tests (pytest) (pull_request) Successful in 1m29s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 1m26s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 16m56s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 20m59s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 22m41s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
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
This commit is contained in:
@@ -0,0 +1,72 @@
|
||||
---
|
||||
key: ci.verdict-write-retarget-fence
|
||||
title: '2026-08-03 — the review-verdict job fences its write on the PR timeline''s retarget COUNT, and verifies the exemption write afterwards (#706)'
|
||||
status: active
|
||||
since: '2026-08-03'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'The `review-verdict/h10` job counts `change_target_branch` events on the PR''s issue timeline at run start and again immediately before its POST, and writes 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, which is how #698 route 1 obtained a forged exemption — while the event count is monotonic and cannot alias. Abstaining is a handoff, not a stall, and that is the property the design rests on: every retarget fires `edited`, which is in this workflow''s `types:`, so the event that makes a run abstain has already queued a successor whose window opens after it; the induction terminates when retargeting stops and the last run writes the final answer. `updated_at` was REJECTED as the key because it also moves for comments and labels, which fire none of this workflow''s `types:` — a run could abstain with no successor coming, which is a real stall. The count is trusted only when paging reached a validated EMPTY page; an untrusted count (unreadable page, non-array body, non-numeric length, page cap hit) blocks the exemption `success` ONLY and still lets `pending` through, because `pending` cannot turn an unreviewed head green while withholding it would strand ordinary PRs for no safety gain. SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` and, if a human `Review-verdict:` row appeared with an id ABOVE a high-water mark taken just before the POST, overwrites its own status with `pending` and logs an error. The repair is `pending`, NEVER a copy of the human''s state, since re-posting their `failure` under the machine credential would attribute a human verdict to the job; its description is a SENTINEL that the classification refuses to grant an exemption over, so the block survives later events instead of being re-derived away one event later. The mark is captured BEFORE the last-moment re-read, not merely before the POST — a later mark leaves a multi-round-trip blind gap in which a verdict is neither seen by the re-read nor repaired afterwards. The id comparison is load-bearing: a mere presence test would fire forever on a base-mismatched verdict that `read_existing_verdict` deliberately declines to honour, deadlocking that PR''s exemption permanently.'
|
||||
signals: 'stale review-verdict run overwrites a fresher one, retarget ABA against the docs-only classifier, concurrency group does not serialize pull_request_target, gitea auto-cancel push vs pull_request_target, forged exemption restored after reclassification, human BLOCKED silently turned green, post-write status verification, change_target_branch timeline count, why does my PR post no verdict status after a retarget · paths: `.gitea/workflows/review-verdict.yml`, `scripts/tests/test_pr_changed_files.py` · issues: #706, #698, #672, #663, #622'
|
||||
mechanics: '`count_retargets()` pages `GET /repos/{repo}/issues/{pr}/timeline?limit=50&page=N` (cap 20) setting `rt_count`/`rt_ok`, trusted only on a validated empty page; `retargets_before`/`retargets_before_ok` captured before enumeration, re-counted immediately before the POST; `max_id_before` from `GET /repos/{repo}/statuses/{sha}` (a BARE ARRAY, unlike the combined `/commits/{sha}/status` object); repair POST is `pending`; tests `test_a_RETARGET_DURING_the_run_posts_NOTHING`, `test_a_PR_retargeted_BEFORE_the_run_but_QUIET_during_it_is_STILL_exempt`, `test_an_UNTRUSTED_retarget_count_withholds_the_EXEMPTION`, `test_an_UNTRUSTED_retarget_count_STILL_LETS_PENDING_THROUGH`, `test_a_human_verdict_landing_AFTER_the_POST_is_repaired_to_pending`, `test_a_PRE_EXISTING_human_row_does_NOT_trigger_a_repair`'
|
||||
---
|
||||
|
||||
`ci.exemption-provenance` closed three routes into the exemption path and left one residual it named:
|
||||
status writes are not serialized, so a stale run can post over a fresher one. This record resolves it,
|
||||
**narrowing** that record rather than superseding it.
|
||||
|
||||
## Measured, not reasoned (Gitea 1.25.4, 2026-08-03)
|
||||
|
||||
- **`pull_request_target` runs for one PR overlap, older finishing last.** Probe PR #722: run 7520
|
||||
(`opened`) completed at 18:30:42, twenty seconds *after* run 7521 (`synchronize`) began. Race 1's
|
||||
mechanism, observed rather than argued.
|
||||
- **A non-cancelling concurrency group — #706's own proposal — does nothing.** With it active, runs
|
||||
7528/7529 still overlapped; 7528 ended 36s after 7529 started. Refuted, not declined.
|
||||
- **The control that saved it.** A first probe *with* a group showed cancellations, which looked like
|
||||
confirmation. The identical workflow with **no `concurrency:` key at all** cancelled the same way:
|
||||
Gitea auto-cancels superseded **`push`** runs by itself, and that does not extend to
|
||||
`pull_request_target`. Without the control, a no-op would have shipped as a solution.
|
||||
- **`cancel-in-progress: true` is deliberately untried** — cancellation is precisely what this
|
||||
workflow's header refuses, since a cancelled run leaves an exempt PR statusless with nothing to
|
||||
re-trigger it.
|
||||
|
||||
## Why the count, and why abstaining is safe
|
||||
|
||||
The timeline records each retarget as a `change_target_branch` event. Verified on the route-1
|
||||
reproduction PR #703 (exactly two: `main → probe698/base-S` and back) against PR #717 as a
|
||||
zero-control. The branch *name* aliases under `main → S → main`; the count cannot.
|
||||
|
||||
The standing objection to refuse-on-motion is that it strands the PR — fatal for `updated_at`,
|
||||
harmless here, and not by degree: a retarget **always** fires `edited`, so the abstaining run is
|
||||
guaranteed a successor. It defers rather than declines.
|
||||
|
||||
## What cold review caught (both easy to reintroduce)
|
||||
|
||||
**The mark must be taken BEFORE the last-moment re-read, not merely before the POST.** "As late as
|
||||
possible" is the safer-sounding instinct and is the opposite: a verdict landing between the re-read
|
||||
and a late mark is invisible to the re-read (already done) *and* excluded from the post-write check
|
||||
(id below a mark taken afterwards) — a gap spanning the whole retarget re-count, while the change
|
||||
claimed one round-trip. Early costs nothing, since `id > mark` hides pre-existing rows either way.
|
||||
Pinned structurally, as an order not an output: with the mark late the job still posts and still
|
||||
repairs in every scenario a stub can pose, and only the arithmetic silently changes.
|
||||
|
||||
**The repair must be durable or it lasts one event.** A repaired status is a machine `pending`,
|
||||
indistinguishable to the next run — which re-derived it, posted `success`, and took a fresh mark
|
||||
*above* the human row, leaving the check silent and the rejection green again. Its description is now
|
||||
a sentinel no exemption is granted over; refusing can only withhold one, and only a re-posted verdict
|
||||
clears it.
|
||||
|
||||
## What is NOT closed
|
||||
|
||||
1. A retarget between the final timeline read and the POST. Gitea's status API has no conditional
|
||||
write, so without compare-and-set this cannot reach zero. The magnitude changed: a *permanent*
|
||||
forged green became a *transient* one of about one round-trip, and that retarget still fires
|
||||
`edited`, so a later run re-derives it.
|
||||
2. The repair is itself a read-then-write and can be raced; it fails toward `pending`. A transport
|
||||
failure on its POST is retried once then fails the job loudly — the one path where giving up
|
||||
quietly would leave a forged green standing.
|
||||
3. **No vocabulary tripwire.** If an upgrade renames `change_target_branch` or drops it, both counts
|
||||
read `0`, compare equal, are "trusted", and the protection evaporates silently. Accepted (an
|
||||
analogue of the jq `--expect` pin needs a live fixture PR), recorded so the silence is chosen.
|
||||
4. **A timeline over the 20-page cap can never be exempted** — `rt_ok` stays `no` on every run, so only
|
||||
a human verdict clears it and comment-flooding becomes a fail-closed denial of exemption.
|
||||
Negligible at 1000 events; the log says so rather than promising a later run will fix it.
|
||||
Reference in New Issue
Block a user