Page both /statuses/{sha} reads in review-verdict.yml — limit clamps to 50, so the post-write race check can miss a raced verdict #763

Closed
opened 2026-08-10 19:28:49 +02:00 by timothy · 3 comments
Owner

Split out of #751, raised by its fourth cold review round.

The residual

review-verdict.yml reads the per-POST status history GET /statuses/{sha}?limit=100 twice — once for the high-water mark (hist_before) and once for the post-write race check (post_hist). Neither pages.

limit=100 clamps to the server-wide MAX_RESPONSE_ITEMS, measured at 50 on this instance. So on a head with more than 50 status rows, both reads see a partial list:

  • post_hist — a raced human Review-verdict: BLOCKED can sit on a page the job never reads, so raced=0, no repair fires, and the exemption success stands over a human rejection. This is the one path in the design whose failure direction is toward SUCCESS.
  • hist_before — the high-water mark may not be the true maximum id, which makes pre-existing rows look newer than the mark. That direction is safe (a false repair to pending), but it is a stall nobody would understand.

#751 added a conservative mitigation for the first: if page 1 shows no raced row, the job reads page 2 and treats any rows there — or an unreadable page 2 — as "assume raced", repairing to pending. That closes the fail-open direction without paging, but it is a blunt instrument: on a head that genuinely runs past one page, every exemption gets repaired to pending and needs a human verdict.

Why it is not urgent

Reachable but not currently reached: a live probe head carried 33 rows after ~5 workflow runs (measured 2026-08-10), against a cap of 50. A PR with a few more CI reruns gets there. Ordering is only coarsely newest-first (ids came back 33,32,31,30,28,29,27,…), so the raced row being on page 1 is not something to rely on — and review-verdict.yml explicitly disclaims relying on order.

Proposed

Page both reads to a validated terminator, the way count_retargets does, and drop the blunt page-2 mitigation once real paging exists.

Notes from #751 that apply directly:

  • Terminators differ per endpoint on this server. /statuses/{sha} past the end returns []; /commits/{sha}/status returns {"statuses": null}; /issues/{n}/timeline returns bare null; /issues/{n}/comments returns []. Measure the one you are paging — guessing has been wrong twice.
  • Do not write a guard against a hardcoded cap. #751 shipped a check against limit=100 that was dead code because the cap is 50, and the repo had already documented that cap in three places.
  • total_count on the combined endpoint is per page, not per commit, so it cannot detect truncation.
  • Terminate only on a validated empty page, never on a short one (ci.paged-endpoint-completeness is cited in the workflow but has never existed as a record#751 repointed that citation).
  • The test doubles must page too, and the page guard has to sit ahead of the read-counting stub modes: appears-on-read:N/human-after-post count how many times the job LOOKED, and a completeness probe is part of the same look. Getting this wrong presents as three unrelated mid-run-race tests going red, and the tempting fix (bumping the expected counts) destroys what they measure.

Done-when

  • Both /statuses/{sha} reads page to a validated terminator, with a page cap and an explicit "could not establish" path
  • The high-water mark is computed over the full list, not page 1
  • The blunt page-2 "assume raced" mitigation from #751 is removed once real paging replaces it
  • A test with a head whose history exceeds one page, asserting a raced verdict on page 2 IS detected and repaired
  • The stubs page faithfully, with the page guard ahead of the read counters
  • docs/ci-cd.md + ci.verdict-write-retarget-fence updated
  • Adversarial review passed
Split out of #751, raised by its fourth cold review round. ## The residual `review-verdict.yml` reads the per-POST status history `GET /statuses/{sha}?limit=100` twice — once for the high-water mark (`hist_before`) and once for the post-write race check (`post_hist`). Neither pages. `limit=100` **clamps to the server-wide `MAX_RESPONSE_ITEMS`, measured at 50** on this instance. So on a head with more than 50 status rows, both reads see a partial list: - **`post_hist`** — a raced human `Review-verdict: BLOCKED` can sit on a page the job never reads, so `raced=0`, no repair fires, and the exemption `success` stands over a human rejection. This is the one path in the design whose failure direction is toward SUCCESS. - **`hist_before`** — the high-water mark may not be the true maximum id, which makes pre-existing rows look newer than the mark. That direction is safe (a false repair to `pending`), but it is a stall nobody would understand. #751 added a **conservative** mitigation for the first: if page 1 shows no raced row, the job reads page 2 and treats *any* rows there — or an unreadable page 2 — as "assume raced", repairing to `pending`. That closes the fail-open direction without paging, but it is a blunt instrument: on a head that genuinely runs past one page, every exemption gets repaired to `pending` and needs a human verdict. ## Why it is not urgent Reachable but not currently reached: a live probe head carried **33 rows** after ~5 workflow runs (measured 2026-08-10), against a cap of 50. A PR with a few more CI reruns gets there. Ordering is only coarsely newest-first (ids came back `33,32,31,30,28,29,27,…`), so the raced row being on page 1 is not something to rely on — and `review-verdict.yml` explicitly disclaims relying on order. ## Proposed Page both reads to a **validated** terminator, the way `count_retargets` does, and drop the blunt page-2 mitigation once real paging exists. Notes from #751 that apply directly: - **Terminators differ per endpoint on this server.** `/statuses/{sha}` past the end returns `[]`; `/commits/{sha}/status` returns `{"statuses": null}`; `/issues/{n}/timeline` returns bare `null`; `/issues/{n}/comments` returns `[]`. Measure the one you are paging — guessing has been wrong twice. - **Do not write a guard against a hardcoded cap.** #751 shipped a check against `limit=100` that was dead code because the cap is 50, and the repo had already documented that cap in three places. - `total_count` on the combined endpoint is **per page**, not per commit, so it cannot detect truncation. - Terminate only on a validated empty page, never on a short one (`ci.paged-endpoint-completeness` is cited in the workflow but **has never existed as a record** — #751 repointed that citation). - The **test doubles must page too**, and the page guard has to sit ahead of the read-counting stub modes: `appears-on-read:N`/`human-after-post` count how many times the job LOOKED, and a completeness probe is part of the same look. Getting this wrong presents as three unrelated mid-run-race tests going red, and the tempting fix (bumping the expected counts) destroys what they measure. ## Done-when - [x] Both `/statuses/{sha}` reads page to a validated terminator, with a page cap and an explicit "could not establish" path - [x] The high-water mark is computed over the full list, not page 1 - [x] The blunt page-2 "assume raced" mitigation from #751 is removed once real paging replaces it - [x] A test with a head whose history exceeds one page, asserting a raced verdict on page 2 IS detected and repaired - [x] The stubs page faithfully, with the page guard ahead of the read counters - [x] `docs/ci-cd.md` + `ci.verdict-write-retarget-fence` updated - [x] Adversarial review passed
timothy added the ci-cdpriority: mediumsecurity labels 2026-08-10 19:28:50 +02:00
Author
Owner

Live instance observed 2026-08-26 (while working #708)

This issue's predicted cost — "on a head that genuinely runs past one page, every exemption gets
repaired to pending and needs a human verdict"
fired on a real Renovate PR, not a probe head.

Renovate PR #761 (renovate/meziantou.analyzer-3.x, head 8798a1d2, changed set exactly
Directory.Packages.props). The gate classified it correctly and posted its exemption, then repaired
it away in the same job:

Decision: state=success — authored by the 'renovate' bot account, touches no protected path, and changes only dependency manifests
Posted review-verdict/h10=success on 8798a1d.
::warning::The status history for 8798a1d runs past page 1 (13 more row(s)), so a raced human verdict could be on a page this job did not read. Repairing to pending rather than leaving the exemption green.
::error::A human review-verdict/h10 verdict landed on 8798a1d while this job was writing its exemption, and was overwritten. Downgrading to 'pending' so an explicit human decision cannot be silently green.
Repaired review-verdict/h10 to pending on 8798a1d.

No human verdict existed on that head. The ::error:: is a false positive: page 2 merely had rows,
which the conservative rule treats as "assume raced".

Three things this pins down that the issue body left open:

  1. The threshold is reachable in ordinary use. 63 status rows on one head is enough — that is a
    long-lived dependency PR accumulating re-runs, not an artificial case. MAX_RESPONSE_ITEMS is
    confirmed at 50 on this instance.
  2. The repair is STICKY, so the cost is not one stalled run. Once REPAIR_DESC is on the sha, the
    ex_repair branch refuses to re-exempt it on every later run — by design (it must be a fixed
    point), but it means a head that crosses the page boundary is permanently un-exempt until a human
    posts a verdict. Re-triggering does not clear it; a later run posted the same sentinel again.
  3. Any re-trigger is enough — a title edit re-fired the workflow via types: [… edited] and that
    was sufficient. No push, no retarget, no actual concurrency required.

So the failure direction is toward a stall that nobody can diagnose from the status alone (the
description asserts a human verdict was overwritten, which is false), and it lands on exactly the PRs
the exemption exists to keep moving. That argues for the paging fix this issue proposes rather than
leaving the blunt mitigation in place.

Evidence: run 2288 / job 9690, and the review-verdict/h10 history on 8798a1d2 (4 rows, ids 63/64
posted in the same second, then 98 and 102).

Side effect to be aware of: PR #761's h10 is currently pending for this reason and needs a
re-posted verdict (or will re-exempt itself on Renovate's next rebase to a fresh sha). It carries no
human rejection — the sentinel is the false positive described above.

## Live instance observed 2026-08-26 (while working #708) This issue's predicted cost — *"on a head that genuinely runs past one page, every exemption gets repaired to `pending` and needs a human verdict"* — **fired on a real Renovate PR**, not a probe head. Renovate PR **#761** (`renovate/meziantou.analyzer-3.x`, head `8798a1d2`, changed set exactly `Directory.Packages.props`). The gate classified it correctly and posted its exemption, then repaired it away in the same job: ``` Decision: state=success — authored by the 'renovate' bot account, touches no protected path, and changes only dependency manifests Posted review-verdict/h10=success on 8798a1d. ::warning::The status history for 8798a1d runs past page 1 (13 more row(s)), so a raced human verdict could be on a page this job did not read. Repairing to pending rather than leaving the exemption green. ::error::A human review-verdict/h10 verdict landed on 8798a1d while this job was writing its exemption, and was overwritten. Downgrading to 'pending' so an explicit human decision cannot be silently green. Repaired review-verdict/h10 to pending on 8798a1d. ``` **No human verdict existed on that head.** The `::error::` is a false positive: page 2 merely had rows, which the conservative rule treats as "assume raced". Three things this pins down that the issue body left open: 1. **The threshold is reachable in ordinary use.** 63 status rows on one head is enough — that is a long-lived dependency PR accumulating re-runs, not an artificial case. `MAX_RESPONSE_ITEMS` is confirmed at 50 on this instance. 2. **The repair is STICKY, so the cost is not one stalled run.** Once `REPAIR_DESC` is on the sha, the `ex_repair` branch refuses to re-exempt it on every later run — by design (it must be a fixed point), but it means a head that crosses the page boundary is permanently un-exempt until a human posts a verdict. Re-triggering does not clear it; a later run posted the same sentinel again. 3. **Any re-trigger is enough** — a title edit re-fired the workflow via `types: [… edited]` and that was sufficient. No push, no retarget, no actual concurrency required. So the failure direction is toward a **stall that nobody can diagnose from the status alone** (the description asserts a human verdict was overwritten, which is false), and it lands on exactly the PRs the exemption exists to keep moving. That argues for the paging fix this issue proposes rather than leaving the blunt mitigation in place. Evidence: run 2288 / job 9690, and the `review-verdict/h10` history on `8798a1d2` (4 rows, ids 63/64 posted in the same second, then 98 and 102). **Side effect to be aware of:** PR #761's `h10` is currently `pending` for this reason and needs a re-posted verdict (or will re-exempt itself on Renovate's next rebase to a fresh sha). It carries no human rejection — the sentinel is the false positive described above.
timothy added the in-progress label 2026-08-28 14:36:13 +02:00
Author
Owner

Claiming (Claude Code session, 2026-08-28, worktree main-2).

Pre-claim checks per process.parallel-session-claim, all four run: no open PR referencing #763 (the five open PRs are all Renovate dependency bumps), no remote branch naming it, no prior claiming comment, and a fresh git fetch origin main.

Parallel-session note (sessions are indistinguishable in Gitea, so stating intent): a concurrent session claimed #781 + #799 at 14:34 today — plugin/MCP config hygiene under .mcp.json / .claude/settings*.json. That is disjoint from this issue's file set (.gitea/workflows/review-verdict.yml, scripts/tests/, docs/). I also skipped the higher-ranked #747 deliberately rather than on size: its remaining scope mutates shared live infra — the owner-level Actions token mode and main's branch protection — which the concurrent session depends on to merge.

Not bundling #803, though it is the nearest sibling (same label, same subsystem, both about paged reads of remote state in the gate). #803 was split out of #778 precisely because stacking pre-existing-contract corrections onto an already multi-round gate PR "is how a scoped change stops being reviewable", and the fix here rewrites this workflow's paging semantics. Folding them together would reproduce the exact failure #803 documents as its own reason for existing.

Starting from the 2026-08-26 live-fire comment above (Renovate PR #761, head 8798a1d2) as the reproduction target: the conservative page-2 mitigation posted a false ::error:: asserting a human verdict was overwritten when none existed, and the repair is sticky. Note that PR #761 is still open with h10 stuck pending for this reason — I will not clear that by hand, since it is the live evidence.

Claiming (Claude Code session, 2026-08-28, worktree `main-2`). Pre-claim checks per `process.parallel-session-claim`, all four run: no open PR referencing #763 (the five open PRs are all Renovate dependency bumps), no remote branch naming it, no prior claiming comment, and a fresh `git fetch origin main`. **Parallel-session note (sessions are indistinguishable in Gitea, so stating intent):** a concurrent session claimed #781 + #799 at 14:34 today — plugin/MCP config hygiene under `.mcp.json` / `.claude/settings*.json`. That is disjoint from this issue's file set (`.gitea/workflows/review-verdict.yml`, `scripts/tests/`, `docs/`). I also skipped the higher-ranked #747 deliberately rather than on size: its remaining scope mutates *shared live infra* — the owner-level Actions token mode and `main`'s branch protection — which the concurrent session depends on to merge. **Not bundling #803**, though it is the nearest sibling (same label, same subsystem, both about paged reads of remote state in the gate). #803 was split out of #778 precisely because stacking pre-existing-contract corrections onto an already multi-round gate PR "is how a scoped change stops being reviewable", and the fix here rewrites this workflow's paging semantics. Folding them together would reproduce the exact failure #803 documents as its own reason for existing. Starting from the 2026-08-26 live-fire comment above (Renovate PR #761, head `8798a1d2`) as the reproduction target: the conservative page-2 mitigation posted a false `::error::` asserting a human verdict was overwritten when none existed, and the repair is sticky. Note that PR #761 is still open with `h10` stuck `pending` for this reason — I will not clear that by hand, since it is the live evidence.
Author
Owner

Closing record

Outcome: Shipped — PR #868, merged as 609fd852c. Both /statuses/{sha} reads page to a validated empty page (page cap 20, one retry per page, explicit "could not establish" path), the high-water mark is computed over the paged list, and #751's page-2 "assume raced" probe is deleted. All seven ## Done-when boxes satisfied.

Root cause: GET /statuses/{sha}?limit=100 clamps to the server-wide MAX_RESPONSE_ITEMS (50), so a single read returned a partial list. But this issue's own framing of the consequence was wrong, and that correction is the most reusable thing here. Under the server default (created_unix DESC) page 1 already held the true maximum id and every row newer than the mark — the only rows the post-write check selects on. A single-page read could miss a raced verdict only if more than 50 rows were created inside the write window, not merely on "a head with more than 50 rows". What actually caused PR #761's stall was the page-2 probe treating "there are rows I did not read" as "assume raced"; retiring it is the fix. The walk earns its place for a different reason: the gate's one fail-toward-SUCCESS path no longer rests on an undocumented ordering the server honours only coarsely.

Decisions/conventions changed: none added. ci.verdict-write-retarget-fence updated (rule, mechanics, body) — it already owned this workflow's paging discipline, so a second record would have split one rule across two.

Reusable knowledge:

  1. A defect one expression away from the one you are fixing is probably the same defect. .id > $since had the identical string-vs-number flaw as the high-water max — jq orders strings above every number, so a pre-existing row with "id": "3" read as newer than any mark, was counted as raced, and earned the sticky sentinel plus a false "was overwritten" on every later run. Live on main, found only because a reviewer swept the twin.
  2. Two defects from one mechanism means remove the mechanism. Twice: a "currency witness" (counted any row above the mark, so a stale-but-valid snapshot passed; and one schema-valid stale read made a permanent sentinel) and sort=highestindex (closes a mid-walk-insert gap, but ASC puts the oldest rows on page 1, inverting the partial-mark fallback into the very #761 stall). Both withdrawn, both documented as rejected alternatives so they are not re-adopted — the second with a behavioural test, not just a structural one.
  3. Fixing a fail-open can widen one. Requiring a complete walk for the high-water mark meant a page-2 hiccup, an over-cap history or one malformed id newly routed into "skip the post-write check entirely" — a wider fail-open than the bug being fixed. A partial list still yields a mark; low is the safe direction, and only because the newest rows are on page 1.
  4. A test double's infidelity produces green mutations. Two here: the stub served one flat list (so "terminate only on an empty page" was unobservable), and computed its own-post id with max() over mixed str/int — so a mutation stayed green because the double crashed, not because the code was right.
  5. git checkout -- <file> restores from the INDEX. Used it to revert a mutation and it silently discarded unstaged edits. Restore from an explicit backup copy instead.

Verification: scripts/tests 1097 passed / 2 skipped. Eighteen executed mutations, each reddening its named test; one arm is unreachable by any fixture and is annotated as such rather than claimed as proved. page_statuses executed in isolation under set -euo pipefail across 13–15 hostile inputs. Terminator shape, limit clamp, sort order and id monotonicity all measured live on Gitea 1.27.1 (2026-08-28) rather than taken from prose. CI green on 11287a54b.

Review: six independent cold passes (one cross-family Codex, five isolated Opus agents). They found a Blocker, a live-on-main twin defect, four documentation overclaims, and five "test passes for the wrong reason" instances. A seventh cross-family pass was attempted and could not complete — the Codex account hit its usage limit — so the re-reviews of the fix rounds were same-family; recorded rather than glossed.

Deferred:

  • A row inserted mid-walk can still be missed (offset paging, no snapshot token; under DESC a new row lands at position 0, on a page already read). Accepted and documented: a row arriving after this job's POST is not one the job overwrote, and being newest it wins on the combined endpoint branch protection reads.
  • The mark is abandoned only when a read both failed and returned nothing — the pre-existing #849 gap, unchanged here and now pinned by a test so it stays visible.
  • The numeric-only id guards are a type guard, not a value bound: a corrupt but genuinely numeric id would still inflate the mark. Not attacker-controllable (ids are server-assigned).
  • #803 deliberately not bundled — it was split out of #778 precisely because stacking pre-existing-contract corrections onto a multi-round gate PR stops it being reviewable, and this PR ran nine rounds.

Docs updated: docs/ci-cd.md, docs/decisions/records/ci/verdict-write-retarget-fence.md (+ regenerated docs/decisions/README.md). Both carry the corrected reachability, the DESC dependency of the partial-mark fallback, both withdrawals, and the accepted residual.

## Closing record **Outcome:** Shipped — PR #868, merged as `609fd852c`. Both `/statuses/{sha}` reads page to a validated empty page (page cap 20, one retry per page, explicit "could not establish" path), the high-water mark is computed over the paged list, and #751's page-2 "assume raced" probe is deleted. All seven `## Done-when` boxes satisfied. **Root cause:** `GET /statuses/{sha}?limit=100` clamps to the server-wide `MAX_RESPONSE_ITEMS` (50), so a single read returned a partial list. But **this issue's own framing of the consequence was wrong, and that correction is the most reusable thing here.** Under the server default (`created_unix DESC`) page 1 already held the true maximum id *and* every row newer than the mark — the only rows the post-write check selects on. A single-page read could miss a raced verdict only if **more than 50 rows were created inside the write window**, not merely on "a head with more than 50 rows". What actually caused PR #761's stall was the page-2 probe treating "there are rows I did not read" as "assume raced"; retiring it is the fix. The walk earns its place for a different reason: the gate's one fail-toward-SUCCESS path no longer rests on an undocumented ordering the server honours only coarsely. **Decisions/conventions changed:** none added. `ci.verdict-write-retarget-fence` updated (rule, mechanics, body) — it already owned this workflow's paging discipline, so a second record would have split one rule across two. **Reusable knowledge:** 1. **A defect one expression away from the one you are fixing is probably the same defect.** `.id > $since` had the identical string-vs-number flaw as the high-water `max` — jq orders strings above every number, so a pre-existing row with `"id": "3"` read as newer than any mark, was counted as raced, and earned the sticky sentinel plus a false "was overwritten" on *every* later run. Live on `main`, found only because a reviewer swept the twin. 2. **Two defects from one mechanism means remove the mechanism.** Twice: a "currency witness" (counted *any* row above the mark, so a stale-but-valid snapshot passed; and one schema-valid stale read made a permanent sentinel) and `sort=highestindex` (closes a mid-walk-insert gap, but ASC puts the *oldest* rows on page 1, inverting the partial-mark fallback into the very #761 stall). Both withdrawn, both documented as rejected alternatives so they are not re-adopted — the second with a behavioural test, not just a structural one. 3. **Fixing a fail-open can widen one.** Requiring a *complete* walk for the high-water mark meant a page-2 hiccup, an over-cap history or one malformed id newly routed into "skip the post-write check entirely" — a wider fail-open than the bug being fixed. A partial list still yields a mark; low is the safe direction, and only *because* the newest rows are on page 1. 4. **A test double's infidelity produces green mutations.** Two here: the stub served one flat list (so "terminate only on an empty page" was unobservable), and computed its own-post id with `max()` over mixed `str`/`int` — so a mutation stayed green because the **double crashed**, not because the code was right. 5. **`git checkout -- <file>` restores from the INDEX.** Used it to revert a mutation and it silently discarded unstaged edits. Restore from an explicit backup copy instead. **Verification:** `scripts/tests` 1097 passed / 2 skipped. **Eighteen executed mutations**, each reddening its named test; one arm is unreachable by any fixture and is annotated as such rather than claimed as proved. `page_statuses` executed in isolation under `set -euo pipefail` across 13–15 hostile inputs. Terminator shape, `limit` clamp, sort order and id monotonicity all measured live on Gitea 1.27.1 (2026-08-28) rather than taken from prose. CI green on `11287a54b`. **Review:** six independent cold passes (one cross-family Codex, five isolated Opus agents). They found a Blocker, a live-on-`main` twin defect, four documentation overclaims, and five "test passes for the wrong reason" instances. A seventh cross-family pass was attempted and **could not complete** — the Codex account hit its usage limit — so the re-reviews of the fix rounds were same-family; recorded rather than glossed. **Deferred:** - A row inserted **mid-walk** can still be missed (offset paging, no snapshot token; under DESC a new row lands at position 0, on a page already read). Accepted and documented: a row arriving after this job's POST is not one the job overwrote, and being newest it wins on the combined endpoint branch protection reads. - The mark is abandoned only when a read both **failed** and returned nothing — the pre-existing #849 gap, unchanged here and now pinned by a test so it stays visible. - The numeric-only id guards are a **type** guard, not a value bound: a corrupt but genuinely numeric id would still inflate the mark. Not attacker-controllable (ids are server-assigned). - #803 deliberately not bundled — it was split out of #778 precisely because stacking pre-existing-contract corrections onto a multi-round gate PR stops it being reviewable, and this PR ran nine rounds. **Docs updated:** `docs/ci-cd.md`, `docs/decisions/records/ci/verdict-write-retarget-fence.md` (+ regenerated `docs/decisions/README.md`). Both carry the corrected reachability, the DESC dependency of the partial-mark fallback, both withdrawals, and the accepted residual.
timothy removed the in-progress label 2026-08-28 19:35:46 +02:00
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#763