fix(706): make the repair sentinel a fixed point, not a two-event delay
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 13s
PR Gates / Docs update reminder (pull_request) Successful in 17s
PR Gates / decisions lifecycle (pull_request) Successful in 19s
review-verdict/h10 Awaiting review verdict for 6304029
Review verdict / Set review-verdict status (pull_request_target) Successful in 35s
PR Gates / Script tests (pytest) (pull_request) Successful in 1m44s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 18s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 15s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 9m18s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 16m0s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 16m6s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 13s
PR Gates / Docs update reminder (pull_request) Successful in 17s
PR Gates / decisions lifecycle (pull_request) Successful in 19s
review-verdict/h10 Awaiting review verdict for 6304029
Review verdict / Set review-verdict status (pull_request_target) Successful in 35s
PR Gates / Script tests (pytest) (pull_request) Successful in 1m44s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 18s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 15s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 9m18s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 16m0s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 16m6s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Round-2 cold review found the round-1 sentinel self-clobbering: the branch refused the exemption but fell through to the shared else, which posts the GENERIC "Awaiting review verdict" description — erasing the very marker the refusal depends on. The next run saw an ordinary machine `pending`, re-derived it, and posted `success`, burying the human rejection two events after the repair instead of one. The single-hop test passed throughout, and the positive control asserting that an ordinary machine `pending` DOES re-derive was itself the proof of the second hop. Durability is a fixed point, and only a chain can assert a fixed point, so the new test runs the job twice and feeds run N's own posted description in as run N+1's existing status. Keyed on `ex_repair` alone rather than on the exempt path: the fact recorded is "a human verdict was lost on this sha", a property of the sha rather than of this run's classification. Verified by mutation — restoring the defect turns the chained test RED while the single-hop test stays GREEN, which is exactly why the chain was needed. Refs #706
This commit is contained in:
@@ -5,7 +5,7 @@ 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.'
|
||||
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 AND re-writes verbatim on every later run, so the block is a FIXED POINT rather than decaying — writing the generic `pending` description there instead erases the marker and the exemption simply returns 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`'
|
||||
---
|
||||
@@ -49,11 +49,11 @@ claimed one round-trip. Early costs nothing, since `id > mark` hides pre-existin
|
||||
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.
|
||||
**The repair must be a FIXED POINT or it merely decays more slowly.** A repaired status is a machine
|
||||
`pending`, indistinguishable to the next run — which re-derived it and posted `success` again. The
|
||||
description is now a sentinel no exemption is granted over *and* is re-written verbatim by every later
|
||||
run: the first attempt refused the exemption but wrote the GENERIC pending text, erasing its own
|
||||
marker, so the exemption returned two events later instead of one. Only a re-posted verdict clears it.
|
||||
|
||||
## What is NOT closed
|
||||
|
||||
|
||||
Reference in New Issue
Block a user