fix(706,707,711): fence the review-verdict write on the timeline retarget count #723

Merged

timothy merged 5 commits from fix/706-verdict-status-serialization into main 2026-08-03 23:01:13 +02:00

Conversation 1 Commits 5 Files Changed 11

+1054 -17

timothy commented 2026-08-03 21:43:33 +02:00

Owner

Copy Link

Copy Source

Closes 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 permanently overwrite a fresher verdict

Reproduced live before anything was written (Gitea 1.25.4, probe PR #722, every other workflow stripped so nothing else could fire):

run	event	started	ended
7520	opened	18:29:54	18:30:42
7521	synchronize	18:30:22	18:31:14

Run 7520 finished 20s after 7521 started. pull_request_target runs for one PR genuinely overlap, older finishing last.

The issue's own headline proposal is refuted, not declined. With concurrency: {group: …-${{ pr number }}, cancel-in-progress: false} active on the same probe, runs 7528/7529 still ran concurrently — 7528 ended 36s after 7529 began.

A first probe with a group appeared to confirm the fix worked. A negative control — the identical workflow with no concurrency: key at all — cancelled identically, 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. Without the control this would have shipped a no-op as a solution.

The fix leaves runs unserialized and makes an overtaken run decline to write: count change_target_branch timeline events at start and again before the POST, 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 — exactly how #698 route 1 forged its exemption). Verified the primitive on PR #703 (2 events) against PR #717 (0).

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 silently turned green

The issue lists "re-read before the POST" as an unevaluated candidate; that landed in main 17 minutes after the issue was filed. The remaining window is now covered by a post-write verification that repairs the job's own status to pending — never a copy of the human's verdict, which would attribute a human decision to the machine.

#707 — pr-changed-files.sh never bound .base.sha

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 JSON already fetched (no new round trips), with the .base.ref comparator left intact — the two answer different questions.

#711 — .codex/ added to PROTECTED

It mirrors .claude/hooks/ byte for byte including the merge-consent hook. Latent today (untracked), live the moment anyone tracks it.

Review

Cold adversarial review returned BLOCKED on a real HIGH: the high-water mark was captured after the pre-POST re-read, leaving a multi-round-trip blind gap in which a human verdict is neither seen by the re-read nor repaired afterwards. Fixed by reordering (mark first) and pinned structurally, since it is an order rather than an output. Also fixed from that review: the repair lasting only one event (now a sentinel the classifier refuses to exempt over), a repair-POST transport failure leaving a forged green, jq hard-errors on non-object rows, and an untruthful page-cap log message.

Verification

• 406 script tests pass.
• Every new guard mutation-checked individually — fence motion comparison, untrusted-count gate, repair POST, id high-water mark, mark ordering, repair sentinel, fence-gates-pending, .codex/ protection, and the anti-deadlock control — each neutered in turn, each producing the intended red while its positive control stayed green.
• The full-suite run caught a regression a scoped run missed: #707's guard broke 9 hook tests whose shim served no base.sha (all positive controls, failing closed); the shim now models the field.

Residuals are stated in ci.verdict-write-retarget-fence, not implied: the sub-round-trip window (no compare-and-set exists), the repair being itself a read-then-write, the absent vocabulary tripwire, and the 20-page cap.

fixes #706
fixes #707
fixes #711

Closes 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 permanently overwrite a fresher verdict **Reproduced live before anything was written** (Gitea 1.25.4, probe PR #722, every other workflow stripped so nothing else could fire): | run | event | started | ended | |---|---|---|---| | 7520 | `opened` | 18:29:54 | **18:30:42** | | 7521 | `synchronize` | 18:30:22 | 18:31:14 | Run 7520 finished **20s after** 7521 started. `pull_request_target` runs for one PR genuinely overlap, older finishing last. **The issue's own headline proposal is refuted, not declined.** With `concurrency: {group: …-${{ pr number }}, cancel-in-progress: false}` active on the same probe, runs 7528/7529 still ran concurrently — 7528 ended **36s after** 7529 began. A first probe *with* a group appeared to confirm the fix worked. A negative control — the identical workflow with **no `concurrency:` key at all** — cancelled identically, 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`. **Without the control this would have shipped a no-op as a solution.** **The fix** leaves runs unserialized and makes an overtaken run *decline to write*: count `change_target_branch` timeline events at start and again before the POST, 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 — exactly how #698 route 1 forged its exemption). Verified the primitive on PR #703 (2 events) against PR #717 (0). 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 silently turned green The issue lists "re-read before the POST" as an unevaluated candidate; that landed in `main` 17 minutes *after* the issue was filed. The remaining window is now covered by a post-write verification that repairs the job's own status to `pending` — never a copy of the human's verdict, which would attribute a human decision to the machine. ## #707 — `pr-changed-files.sh` never bound `.base.sha` 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 JSON already fetched (no new round trips), with the `.base.ref` comparator left intact — the two answer different questions. ## #711 — `.codex/` added to `PROTECTED` It mirrors `.claude/hooks/` byte for byte including the merge-consent hook. Latent today (untracked), live the moment anyone tracks it. ## Review Cold adversarial review returned **BLOCKED** on a real HIGH: the high-water mark was captured *after* the pre-POST re-read, leaving a multi-round-trip blind gap in which a human verdict is neither seen by the re-read nor repaired afterwards. Fixed by reordering (mark first) and pinned structurally, since it is an order rather than an output. Also fixed from that review: the repair lasting only one event (now a sentinel the classifier refuses to exempt over), a repair-POST transport failure leaving a forged green, `jq` hard-errors on non-object rows, and an untruthful page-cap log message. ## Verification - **406 script tests pass.** - Every new guard **mutation-checked individually** — fence motion comparison, untrusted-count gate, repair POST, id high-water mark, mark ordering, repair sentinel, fence-gates-pending, `.codex/` protection, and the anti-deadlock control — each neutered in turn, each producing the intended red while its positive control stayed green. - The full-suite run caught a regression a scoped run missed: #707's guard broke 9 hook tests whose shim served no `base.sha` (all positive controls, failing closed); the shim now models the field. Residuals are stated in `ci.verdict-write-retarget-fence`, not implied: the sub-round-trip window (no compare-and-set exists), the repair being itself a read-then-write, the absent vocabulary tripwire, and the 20-page cap. fixes #706 fixes #707 fixes #711

timothy added 1 commit 2026-08-03 21:43:33 +02:00

fix(706,707,711): fence the review-verdict write on the timeline retarget count ... 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

timothy added 1 commit 2026-08-03 21:58:09 +02:00

fix(706): make the repair sentinel a fixed point, not a two-event delay ... 63040296f4

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

timothy referenced this pull request 2026-08-03 21:59:31 +02:00

`decisions_validate` ceiling calibration test is one line from red — any record ≥61 prose lines fails `script-tests` #688

timothy added 1 commit 2026-08-03 22:06:46 +02:00

fix(706): abstain when a repair sentinel appears mid-run ... 5077408528

Round-3 cold review: `ex_repair` was recomputed by the last-moment re-read but
never consulted after it, so the POST wrote the `$state` frozen at
classification time. A stale overlapping run therefore posted its `success`
straight over a sentinel another run had just written — burying a human
rejection with no repair (the human row sits below the stale run's own
high-water mark) and no log entry.

This is the one path in the design that failed toward SUCCESS rather than
`pending`, so it was not covered by the recorded residual, and it is reachable
through exactly the run overlap this branch measured live (probe PR #722: the
older run finished 20s after the newer one started).

The guard is exact rather than conservative: a sentinel present at the FIRST
read forces `state=pending`, so `success` together with `ex_repair=yes` at
re-read time can only mean the sentinel arrived mid-run. Abstaining is then
strictly correct and, unlike the retarget fence, needs no successor run — the
sentinel row is already `pending` and already carries the re-post instruction.

Mutation-verified three ways: removing the guard reddens the new mid-run test
while its positive control stays green; making it unconditional on `ex_repair`
reddens the fixed-point test and the positive control, proving the condition is
precisely scoped and not merely present.

Also strengthens the mark-ordering test to pin the status-history FETCH as well
as its initialisation, closing the refactor evasion review flagged; sliding the
fetch past the re-read now reddens it.

Refs #706

timothy added 1 commit 2026-08-03 22:13:40 +02:00

fix(706): never replace a sentinel with a non-sentinel ... a8bbd74a64

Round-4 cold review: the mid-run sentinel guard tested `state = success`, which
is one branch too narrow. A run can reach the POST on `state=pending` carrying
the GENERIC description — most realistically after a transient enumeration
failure (`complete != yes`) — and such a run passed the success-only guard,
passed the fence, and overwrote the sentinel with ordinary text. The next run
then saw no sentinel, re-derived, and posted `success`: the same buried human
rejection as the round-2 defect, reached in two steps instead of one.

The guard now compares the DESCRIPTION rather than the state:

    if [ "$ex_repair" = yes ] && [ "$desc" != "$REPAIR_DESC" ]

"Never replace a sentinel with a non-sentinel." This is strictly more general
and exactly as precise, because the carry-forward branch guarantees that a
sentinel seen at the FIRST read already sets `desc` to the sentinel — so the
guard cannot fire on the ordinary repaired-head path and the fixed point stays
intact.

It also makes the code match the decision record, which already stated the
general property ("a run whose last-moment re-read finds a sentinel it did not
see at its FIRST read ABSTAINS instead of posting") while the code implemented
only the success case. Of the two, the code was the one that had to move.

Mutation-verified on both clauses independently: reverting to the success-only
condition reddens the new pending-path test; dropping the description check
reddens the fixed-point test and the sentinel-from-the-start control.

Refs #706

timothy added 1 commit 2026-08-03 22:22:12 +02:00

fix(706): count a raced SENTINEL, not only a raced human verdict ... fe3d29276a

Round-5 cold review: the post-write check counted only human `Review-verdict:`
rows above the high-water mark, which is not sufficient under the run overlap
this branch measured.

Sequence, all inside that regime, runs A and B on the same exempt-classified
sha: the human BLOCKED lands BELOW A's mark (so A cannot see it), B masks it
with an exemption `success`, and only afterwards writes the sentinel. A then
finds nothing human above its mark, does not repair, and posts its own `success`
on top of the sentinel. The human rejection is permanently green and every later
run re-derives it — the repair race failing toward SUCCESS, while the record
states it fails toward `pending`.

The filter now counts two row shapes above the mark: a human verdict (non-null
creator, `Review-verdict:` description) OR a machine sentinel (null creator,
description exactly $REPAIR_DESC). A then repairs and both runs converge on the
fixed point.

It cannot false-fire: a pre-existing sentinel would have been seen at the FIRST
read and forced the pending path, and this block only runs after a `success`, so
a sentinel above the mark can only have been written mid-flight by another run.

Mutation-verified on both halves independently — dropping the sentinel
alternation reddens the new test; dropping the human half reddens the original
race-2 test — so neither can be removed without a test noticing.

Refs #706

timothy commented 2026-08-03 22:25:38 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ fe3d292

Independent cold adversarial review (worktree-isolated, review-only brief), six rounds. Five real defects found and fixed, each mutation-pinned: (1) high-water mark ordered after the pre-POST re-read, leaving a multi-round-trip window in which a human verdict was neither seen nor repaired; (2) the repair undone by the next event; (3) the sentinel self-clobbering via the generic pending description; (4) a stale overlapping run posting success over a mid-run sentinel (ex_repair dead after the re-read) - the one path failing toward SUCCESS; (5) the same clobber via the pending path; (6) the post-write filter blind to sentinel rows in a two-run interleaving. Reviewer re-ran its own probes on this head: fixed-point chain, classification flip, and pending-path abstain all hold. 411 script tests green. Both races reproduced live on scratch PRs with probe-only contexts (#722, #724). Residuals recorded honestly in ci.verdict-write-retarget-fence: sub-round-trip window (no compare-and-set exists), repair POST double-failure, absent vocabulary tripwire, 20-page cap denial - none failing toward silent success.

Review-verdict: MERGEABLE @ fe3d292 Independent cold adversarial review (worktree-isolated, review-only brief), six rounds. Five real defects found and fixed, each mutation-pinned: (1) high-water mark ordered after the pre-POST re-read, leaving a multi-round-trip window in which a human verdict was neither seen nor repaired; (2) the repair undone by the next event; (3) the sentinel self-clobbering via the generic pending description; (4) a stale overlapping run posting success over a mid-run sentinel (ex_repair dead after the re-read) - the one path failing toward SUCCESS; (5) the same clobber via the pending path; (6) the post-write filter blind to sentinel rows in a two-run interleaving. Reviewer re-ran its own probes on this head: fixed-point chain, classification flip, and pending-path abstain all hold. 411 script tests green. Both races reproduced live on scratch PRs with probe-only contexts (#722, #724). Residuals recorded honestly in ci.verdict-write-retarget-fence: sub-round-trip window (no compare-and-set exists), repair POST double-failure, absent vocabulary tripwire, 20-page cap denial - none failing toward silent success.

timothy merged commit aa79ec59c9 into main 2026-08-03 23:01:13 +02:00

timothy referenced this issue from a commit 2026-08-03 23:01:14 +02:00

Merge pull request 'fix(706,707,711): fence the review-verdict write on the timeline retarget count' (#723) from fix/706-verdict-status-serialization into main

timothy deleted branch fix/706-verdict-status-serialization 2026-08-03 23:01:15 +02:00

timothy referenced this pull request 2026-08-03 23:02:23 +02:00

`review-verdict/h10` writes are not serialized — a stale run can overwrite a fresher verdict (ABA retarget + human-rejection race) #706

timothy referenced this pull request 2026-08-03 23:02:23 +02:00

`pr-changed-files.sh` paging can drop a path when the base branch advances mid-enumeration #707

timothy referenced this pull request 2026-08-03 23:02:24 +02:00

`.codex/` mirrors the gate-enforcing hooks but is absent from the merge gate's PROTECTED list #711

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#723