fix(763): page both /statuses/{sha} reads to a validated terminator #868

Merged

timothy merged 9 commits from fix/763-page-statuses-reads into main 2026-08-28 19:34:49 +02:00

Conversation 2 Commits 9 Files Changed 5

+1170 -154

timothy commented 2026-08-28 18:43:10 +02:00

Owner

Copy Link

Copy Source

Pages both /statuses/{sha} reads in review-verdict.yml to a validated terminator, and removes #751's blunt page-2 "assume raced" probe.

That probe fired in production: Renovate PR #761 accumulated status rows over ordinary CI re-runs until the probe repaired its exemption away, logging an ::error:: asserting a human verdict had been overwritten on a head that carried none — and, the sentinel being sticky, refused to re-exempt that head on every later run.

Measured live on Gitea 1.27.1, not taken from prose

• PR #761's head carries 114 rows (the issue said 63 — it grew): pages 1–2 return 50, page 3 returns 14, page 4 is []. A third distinct empty shape, after the timeline's bare null and /commits/{sha}/status's {"statuses": null}.
• A short page is not the end; an empty first page is legitimate for the pre-write caller.
• ids are strictly increasing with created_at, unique, one row per POST (no dedupe).

The issue's premise was overstated, and that is now recorded

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 missed 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 removed #761's stall is retiring the probe, not the walk. The walk earns its place for a different reason: the gate's one fail-toward-SUCCESS path no longer depends on an undocumented ordering the server honours only coarsely.

Two mechanisms added and then withdrawn

Each produced two defects, which is the signal to remove rather than patch twice:

• a currency witness — counted any row above the mark, so a stale-but-valid snapshot with an unrelated newer row passed while hiding the rejection; and one schema-valid stale read made a permanent sentinel.
• sort=highestindex (ASC) — closes a mid-walk-insert gap, but puts the oldest rows on page 1, inverting the partial-mark fallback into a spurious sticky repair. That is the #761 failure, re-introduced to close a smaller one. Both a structural and a behavioural test now guard the withdrawal.

Fail-opens closed along the way

The high-water mark gates the post-write check entirely, so refusing one skips it — requiring a complete walk would have made this change a wider fail-open than the bug it fixes. A partial list therefore still yields a mark. Also: both id comparisons are numeric-only (jq orders strings above every number — the .id > $since twin was live on main and caused a permanent per-sha stall); .creator is type-tested before indexing (.creator.login on a non-object exits jq 5 and set -e took the step down after the green was posted); and an empty post-write history is rejected, since this job has just written a row.

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.
• Six independent cold reviews (one cross-family Codex, five isolated Opus agents). They found a Blocker, four documentation overclaims, a live-on-main twin defect, and five "test passes for the wrong reason" instances — including two where a test was green because the stub crashed. All fixed; the last verdict was clean apart from a one-line assertion, now applied and mutation-proved.

Docs

docs/ci-cd.md and ci.verdict-write-retarget-fence updated, including both withdrawals and the accepted mid-walk-insert residual, so neither gets re-adopted.

fixes #763

Pages both `/statuses/{sha}` reads in `review-verdict.yml` to a validated terminator, and removes #751's blunt page-2 "assume raced" probe. That probe **fired in production**: Renovate PR #761 accumulated status rows over ordinary CI re-runs until the probe repaired its exemption away, logging an `::error::` asserting a human verdict had been overwritten on a head that carried none — and, the sentinel being sticky, refused to re-exempt that head on every later run. ### Measured live on Gitea 1.27.1, not taken from prose - PR #761's head carries **114** rows (the issue said 63 — it grew): pages 1–2 return 50, page 3 returns 14, page 4 is `[]`. A third distinct empty shape, after the timeline's bare `null` and `/commits/{sha}/status`'s `{"statuses": null}`. - A **short page is not the end**; an empty **first** page is legitimate for the pre-write caller. - ids are strictly increasing with `created_at`, unique, one row per POST (no dedupe). ### The issue's premise was overstated, and that is now recorded 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 missed 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 removed #761's stall is retiring the probe, not the walk. The walk earns its place for a different reason: the gate's one fail-toward-SUCCESS path no longer depends on an undocumented ordering the server honours only coarsely. ### Two mechanisms added and then withdrawn Each produced two defects, which is the signal to remove rather than patch twice: - a **currency witness** — counted *any* row above the mark, so a stale-but-valid snapshot with an unrelated newer row passed while hiding the rejection; and one schema-valid stale read made a permanent sentinel. - **`sort=highestindex`** (ASC) — closes a mid-walk-insert gap, but puts the *oldest* rows on page 1, inverting the partial-mark fallback into a spurious sticky repair. That is the #761 failure, re-introduced to close a smaller one. Both a structural and a **behavioural** test now guard the withdrawal. ### Fail-opens closed along the way The high-water mark gates the post-write check entirely, so refusing one *skips* it — requiring a complete walk would have made this change a **wider** fail-open than the bug it fixes. A partial list therefore still yields a mark. Also: both id comparisons are numeric-only (jq orders strings above every number — the `.id > $since` twin was live on `main` and caused a permanent per-sha stall); `.creator` is type-tested before indexing (`.creator.login` on a non-object exits jq 5 and `set -e` took the step down *after* the green was posted); and an empty post-write history is rejected, since this job has just written a row. ### 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. - **Six independent cold reviews** (one cross-family Codex, five isolated Opus agents). They found a Blocker, four documentation overclaims, a live-on-`main` twin defect, and five "test passes for the wrong reason" instances — including two where a test was green because the *stub crashed*. All fixed; the last verdict was clean apart from a one-line assertion, now applied and mutation-proved. ### Docs `docs/ci-cd.md` and `ci.verdict-write-retarget-fence` updated, including both withdrawals and the accepted mid-walk-insert residual, so neither gets re-adopted. fixes #763

timothy added 8 commits 2026-08-28 18:43:11 +02:00

fix(763): page both /statuses/{sha} reads to a validated terminator ... 4368cc8cbe

`review-verdict.yml` read the per-POST status history twice with a single
`?limit=100` request. `limit` clamps to the server-wide `MAX_RESPONSE_ITEMS`
(measured 50), so on a head carrying more rows than the clamp both reads saw a
partial list. The high-water mark was only page 1's maximum, and — the direction
that matters — a raced human verdict beyond page 1 was invisible to the
post-write race check, leaving a forged green over a rejection.

Both reads now walk to a validated empty page (`[]` on this endpoint, measured
2026-08-28 against PR #761's 114-row head: pages 1-2 return 50, page 3 returns
14, page 4 is `[]`), never terminating on a short page, under a 20-page cap and
retrying each page once. Correctness does not depend on the cap value.

This retires #751's page-2 "assume raced" probe, which repaired every head that
outgrew one page. It fired on Renovate PR #761: an `::error::` claimed a human
verdict had been overwritten on a head carrying none, and the sticky sentinel
then refused re-exemption on every later run.

Two properties replace it. Uncertainty now fails closed at both ends — the
unreadable-history branch warned and left the exemption green while the page-2
probe repaired on the same uncertainty, one check disagreeing with itself; this
is affordable only because paging removed the common trigger. And the post-write
read must witness the job's own write: reaching a validated empty page proves the
walk finished, not that it saw a current list, so at least one row above the
pre-write mark must exist because the job just posted one.

The `::error::` now distinguishes a verdict actually found from an unverifiable
read. The sentinel description stays generic — the classification recognises it
as a fixed point, so its wording is load-bearing.

The stub gained faithful paging (50-row slices, `[]` past the end, one snapshot
per logical read so a counter mode cannot describe two different histories across
pages) and, separately, modelling of the job's own POST appearing in the history
— which it had never done, so in its world every ordinary run looked like a head
nothing had been posted to. `own-write-invisible` withholds exactly that detail
as the negative control for the currency witness.

Mutation-proved by execution, one clause at a time:
  - walk reads page 1 only -> RUNNING_PAST_PAGE_1_is_PAGED_and_the_exemption_
    STANDS, raced_verdict_on_PAGE_2_is_detected_and_repaired and both UNREADABLE
    history tests go red
  - currency-witness zero branch deleted -> CANNOT_SEE_OUR_OWN_WRITE red
  - fail-closed flipped to fail-open -> both UNREADABLE history tests red

Verification: `scripts/tests` 1085 passed, 2 skipped; decisions_validate and
build_decisions_catalog --check both exit 0.

fixes #763

Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(763): round 2+3 — close the fail-opens the paging change introduced ... 007d2fd3df

Two independent cold reviews (Codex GPT-5.6 cross-family, and an isolated
Opus agent) converged on the same blocker, which is fixed here along with
everything else they found.

BLOCKER — the mark walk turned a fail-closed case into a fail-open. The
high-water mark gates the post-write race check entirely: `max_id_before=-1`
skips it. Before paging, only a failure of the single page-1 request could
reach that. Requiring a COMPLETE walk newly routed a page-2 hiccup, an
over-cap history, or one malformed id on a later page into the same hole, so
a human rejection racing the write was left green where `main` repaired.
A partial list now still yields a mark: it can only be LOWER than the true
maximum, which makes the check more eager, never blinder. Only a read
returning no rows at all abandons it — the pre-existing #849 gap, unchanged
and now asserted by a test so it stays visible.

WITHDRAWN — the "currency witness". It produced two defects from one
mechanism, which is the signal to remove rather than patch twice: counting
ANY row above the mark does not witness this job's write, so a stale-but-valid
snapshot carrying an unrelated newer row passed while hiding a rejection; and
a schema-valid stale read is not retried, so one such response turned a
transient anomaly into a permanent sentinel. The hazard has no mechanism here
either — Gitea is a single instance with no read replicas. Removing it
restores the pre-change exposure on that path, a non-regression.

Also fixed, each a fail-open with a fixture and an executed mutation:
  - `.creator` is type-tested before indexing. `.creator.login` on a non-object
    exits jq 5 and `set -e` took the step down after the green was posted and
    before the repair. Reproduced by both reviewers.
  - the mark is the max over NUMERIC ids only. jq orders strings above every
    number, so one `"id": "99999"` passed the numeric gate and inflated the
    mark until nothing looked newer.
  - an unusable `raced` count now repairs instead of "not acting on it".
  - `sort=highestindex` (ASC, measured) so a row inserted mid-walk appends at
    the end rather than at position 0 on a page already read. An unknown sort
    value silently falls back to DESC, so this is insurance, not load-bearing,
    and the comment says so.
  - `ph_ok`/`ph_rows` renamed off `read_existing_verdict`'s `st_ok`. No live
    bug, but a name collision in a 1400-line step.

Tests the reviews showed were missing, each proved by an executed mutation:
  - verdict beyond a SHORT page (a deliberately unfaithful truncated response
    — against a faithful double a short page is always the last, so the rule
    "terminate only on an EMPTY page" was unobservable)
  - pre-write paging failure still yields a usable mark
  - pre-write read returning nothing abandons the mark and says so
  - a TRANSIENT page failure is retried (the retry was unproven code: every
    other error mode fails on every attempt, so disarming it reddened nothing)
  - a string id cannot inflate the mark
  - a malformed `creator` row does not kill the job

Stub corrections, both the same class as the earlier `[]`-vs-`null` gap: it
served one flat list (so paging was unobservable) and computed its own-post id
with `max()` over mixed str/int, which raised TypeError and made the string-id
test pass because the DOUBLE crashed rather than because the mark was right.

Mutation matrix, all executed, each reddening exactly its named test: retry
disarmed; numeric-max reverted; partial-mark fallback removed; short-page
terminates; page-1-only walk; post-write fail-closed flipped open; jq
type-guard reverted. The unusable-count arm is unreachable by any fixture and
is annotated as such rather than claimed as proved.

Verification: `scripts/tests` 1090 passed, 2 skipped; decisions_validate and
build_decisions_catalog --check both exit 0; terminator, clamp, sort order and
id monotonicity all re-measured live on Gitea 1.27.1.

refs #763

Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(763): round 4 — withdraw the ASC sort, which inverted the partial-mark fallback ... 1eb7b41fc1

Round 3 added `sort=highestindex` to close a mid-walk-insert gap: under the
server default (`created_unix DESC`) a row inserted while the walk is running
lands at position 0, on a page already read, so the walk never sees it.

That fix and the round-2 partial-mark fallback are incompatible. ASC puts the
OLDEST rows on page 1, so an incomplete walk takes its high-water mark over
the oldest rows — leaving every pre-existing row above the mark and read as
"raced". That is a spurious STICKY repair on a head nothing raced, which is
precisely the #761 failure this whole issue exists to remove. Under the
default DESC the newest row is on page 1 by construction and ids are monotonic
with `created_at` (measured), so a partial mark is at or very near the true
maximum and "lower is safe" actually holds.

Two defects from one mechanism again, so the mechanism goes rather than
getting patched: the sort is withdrawn and the mid-walk-insert residual is
ACCEPTED and documented. It is bounded — a row arriving after this job's POST
is not one this job overwrote, and being newest it wins on the combined
endpoint branch protection reads.

Both the code comment and the docs record the withdrawal and the reason, so
the next reader does not re-adopt it.

Verification: `scripts/tests` 1090 passed, 2 skipped; the partial-mark mutation
still reddens `test_a_PRE_WRITE_paging_failure_still_yields_a_usable_high_water_mark`;
decisions_validate and build_decisions_catalog --check both exit 0.

refs #763

Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(763): round 5 — correct the overstated motivation, and fix the string-id twin ... 53ee44cbc1

A third cold review (Opus, isolated worktree) returned NOT MERGEABLE with one
High and three Medium. All are addressed.

HIGH — the stated motivation was wrong, and self-contradictory once round 4
landed. Under the server default (`created_unix DESC`) page 1 holds the NEWEST
rows and ids are monotonic with `created_at`, so page 1 already carried the
true maximum id AND every row newer than the mark — the only rows the
post-write check selects on. A single-page read therefore missed 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", which the issue, the comments and
the docs all asserted. Reviewer executed an order-faithful DESC stub: a
page-1-only reader repairs identically to the full walk.

What actually removed #761's stall is retiring #751's page-2 probe, not the
paging. The walk still earns its place, for a reason now stated instead of the
false one: it stops the gate's one fail-toward-SUCCESS path depending on an
undocumented ordering the server honours only coarsely (page 1 came back
`114,112,113,111,110`). That measurement was deleted in commit 1 and is
restored, since round 4's safety argument rests on exactly it.

MEDIUM/real defect — the string-id TWIN, live on `main` and one expression
away from the fix already made: `select((.id? // 0) > $since)`. jq orders
strings above every number, so a PRE-EXISTING row with `"id": "3"` reads as
newer than any mark, is counted as having raced the write, and gets the sticky
sentinel plus a false "was overwritten" on EVERY later run — a permanent
per-sha stall no re-trigger clears. Now numeric-only, with a test.

Also fixed: a non-empty history carrying no numeric id was collapsed to a mark
of 0 (making every pre-existing row look newer); it is now reported unusable
and the check is skipped. `sleep` no longer fires after the final attempt.

Three unpinned clauses now have tests, each proved by an executed mutation:
  - the page cap is a refusal, not a terminator (1050-row fixture)
  - the `::error::` found-vs-unverifiable distinction (forcing `raced_why=human`
    reddened nothing before)
  - the walk requests no sort order — a structural guard on round 4's
    withdrawal, which nothing mechanical protected. It reads request LINES, not
    comments, since the withdrawal note names the parameter to explain it.

Honest scoping, not new code: the test double is ordering-blind, so the paging
tests prove WALK COMPLETENESS, not that a real raced verdict would otherwise be
missed — under DESC it would not be. The stub comment and the docstrings now
say so rather than implying the stronger claim.

Docs: `ci-cd.md` and the record's `rule:` carry the corrected reachability, the
DESC dependency of the partial-mark fallback, and both rejected alternatives
stated as rejected alternatives rather than as draft chronology
(`docs.no-session-narrative`).

Verification: `scripts/tests` 1094 passed, 2 skipped; eleven executed
mutations across rounds 2-5, each reddening exactly its named test;
decisions_validate and build_decisions_catalog --check exit 0.

refs #763

Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(763): round 6 — measure the partial-mark fallback under the REAL ordering ... 89db6022d9

A fourth cold review (Opus, isolated worktree, tests/double/docs focus)
reported no correctness bugs in shipped behaviour but two coverage defects on
exactly the two things this change advertises. Both are closed.

The partial-mark fallback's safety is a claim ABOUT THE ORDERING — page 1 holds
the newest rows, so a walk that fails later still saw the true maximum. The
fixture pinning it served ASCENDING ids, i.e. the arrangement the design calls
unsafe, and passed anyway because the raced row's id sat above even the partial
mark. It could not distinguish safe from unsafe.

The stub now HONOURS the sort parameter: order-faithful modes serve DESC by
default and ASC when the request asks. The new fixture holds a PRE-EXISTING
base-mismatched verdict at id 7055 among 60 rows. Under DESC the salvaged mark
is 7059 and that row is below it — the exemption correctly stands. Under ASC
the mark would be 7049 and that untouched row tests as NEWER, a sticky repair
on a head nothing raced. So re-adding `sort=highestindex` now reddens by
BEHAVIOUR, not only by the structural assertion added in round 5. Measured:
re-adding it reds both tests.

Most modes stay ordering-blind on purpose and now say so: they test walk
COMPLETENESS, which is order-independent, and insertion order is what lets a
fixture place a row beyond page 1.

Also fixed:
  - `null` is accepted as an empty page. An array-only gate is the exact shape
    of #751 — `count_retargets` had one, the timeline really did return `null`
    past the end, and the fence withheld EVERY exemption from the day it
    shipped. The same narrowing here is worse, because this walk's failure is
    the STICKY sentinel: every exempt PR would need a hand-posted verdict, per
    head. Tolerating `null` cannot misread `[]`. Proved by fixture.
  - the fail-closed comment said "past the 1000-row page cap"; the bound is 950,
    as the walk's own comment and both docs already said.
  - the docs claimed "only a read returning no rows at all abandons the mark".
    False: a VALIDATED empty history yields a mark of 0 and is not abandoned —
    that is the normal first run. What abandons it is a read that both FAILED
    and returned nothing. Corrected in ci-cd.md and the record `rule:`.
  - a comment pointed at the page-2 probe "a few lines further down"; it was
    deleted, so the deixis pointed at nothing.
  - the stub claimed its logical-read counter "is only reached on a SUCCESSFUL
    page-1 serve" — measured false; it counts page-1 requests, retries included.
  - five `(round N)` markers removed. A round number is session chronology and
    does not parse for a reader who never saw it (`docs.no-session-narrative`);
    an issue number does. The four that remain predate this change.

Verification: `scripts/tests` 1096 passed, 2 skipped. Thirteen executed
mutations across rounds 2-6. The reviewer independently re-ran the earlier
matrix and confirmed it, with one correction carried here: two of those
mutations redden MORE than their named test, so "each reddening exactly its
named test" was wrong — they redden at least it.

refs #763

Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(763): round 7 — reject an empty post-write history, and the fourth overclaim ... e6f04cc739

A fourth cold review returned NOT-MERGEABLE on two Mediums. Both fixed, plus
its three Lows.

MEDIUM, and a defect this branch introduced. Tolerating a `null`/`[]` page 1 as
"complete, zero rows" is correct for the PRE-write caller — a head nothing has
posted to genuinely has no statuses — and impossible for the POST-write one,
which has just written a row to that sha. The body is well-formed, so nothing
retries it, and the walk reports success: `raced=0` concluded from a list that
cannot be real, on the one path whose failure direction is toward SUCCESS.
Worse than the code it replaced, which at least emitted a `::warning::` — a
logged fail-open had become an unlogged one. Reviewer measured both directions.

The post-write caller now rejects an empty result itself; the walk stays
caller-agnostic because the pre-write caller genuinely needs the empty answer.
This is NOT the withdrawn currency witness: that asked whether ANY row sat above
the mark, which an unrelated newer row satisfied while the rejection stayed
hidden, and it fired on schema-valid staleness. This asks only whether the list
is EMPTY — a state no unrelated row can produce and no ordering can disguise.
It carries neither defect. Proved by fixture; disarming it reddens the named
test, and the previously-uncovered `null`-at-page-1 clause is now covered too.

MEDIUM — the fourth overclaim of the same class, in the decision record body:
"Uncertainty must fail closed at both ends … Both repair now." The page-2 probe
was DELETED, not converted; it repairs nothing. It also contradicted the
record's own `rule:` ("the two directions are NOT symmetric") and the bullet
directly beneath it. Round 5 retracted this wording in `docs/ci-cd.md` only —
the sweep was by subject, not by the retracted words.

Also fixed:
  - the record presented "an empty FIRST page is legitimate" as a property of
    the walk; it is a property of the pre-write caller.
  - `docs/ci-cd.md` called the numeric-only id comparisons a fix for mark
    inflation; they are a TYPE guard, closing the string half. A corrupt but
    genuinely numeric id still inflates the mark — not attacker-controllable,
    since ids are server-assigned, and now stated rather than implied.
  - `test_a_partial_mark_is_SAFE...`'s self-guard promised to detect that the
    fallback ran; it keys on a warning emitted by a different condition, so
    deleting the fallback left it green. Its sibling is what reddens; the
    message now says what it actually pins.
  - the order-faithful fixture appended the job's own POST after the reversal,
    serving the NEWEST row on the OLDEST page — the opposite of DESC, in the one
    fixture that exists to be ordering-faithful.
  - "twice per walk" for the wasted sleep; it is once per walk, twice per run.
  - a dead counter read in the DESC mode.

Rebased onto b16ec15d6 (the other session's #781/#799 docs work; no file
overlap, no conflicts).

Verification: `scripts/tests` 1097 passed, 2 skipped; fifteen executed mutations
across rounds 2-7; decisions_validate and build_decisions_catalog --check exit 0.

refs #763

Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(763): round 8 — a fixture must pose the scenario it narrates ... 4164efbc3e

A fifth cold review confirmed the gate's behaviour is correct and proof-backed,
and blocked on three non-behavioural items. All three fixed; none touches the
shipped logic.

MEDIUM — the round-7 fixture narrated a raced human verdict it did not
construct. `null-page1-after-post` appended the row unconditionally, so it also
joined the PRE-write read and lifted the high-water mark above itself; removing
it changed nothing. The reviewer's suggested fix was to gate the append on the
post-write read. Measured after gating: still inert, because page 1 answers
`null` before any row reaches the wire.

So the row is gone rather than gated, and the prose now describes what the
fixture actually poses: a response asserting an empty history for a sha this job
wrote to must not be accepted as proof that nothing raced. Whether a verdict
really raced is not modelled and does not need to be — the response is not
evidence either way. A row the test cannot observe is decoration that reads as
coverage, which is the same class this branch has now been blocked on five
times.

LOW — the comment claimed the predecessor "at least produced a `::warning::`".
Half false, measured against `origin/main`: its `jq -e 'type == "array"'` gate
ACCEPTED `[]` silently and warned only on `null`. What is actually new is that
the paged walk reports such a read as a SUCCESS.

LOW — when the empty clause fired it set `ph_ok=no`, so the log said "could not
be read completely" beside a walk that completed on a validated terminator. The
answer was impossible, not unreadable, and an operator holding a sticky sentinel
needs to know which. It now carries its own `raced_why`, asserted by the test.

Both clauses mutation-proved: disarming the empty check, and reverting to the
borrowed wording, each redden the named test.

Verification: `scripts/tests` 1097 passed, 2 skipped; decisions_validate and
build_decisions_catalog --check exit 0.

refs #763

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(763): round 9 — assert the repair's reason positively, not by absence ... 462118d98d

A sixth cold review found everything in round 8 clean except one line, and it
is the rule this branch keeps rediscovering: the test pinned the new
`raced_why` only by asserting the ABSENCE of the borrowed wording. Measured —
replacing the string with `zzz` left the suite green while an operator would
get `::error::… — zzz.` beside a sticky sentinel. The sibling test 330 lines
away states the rule and follows it; this one did not.

Now asserted positively, with the em-dash and full stop discriminating the
`::error::` reason from the `::warning::` text that continues ", which cannot
be true". The `zzz` mutation reddens it.

Three nits from the same review, all verified by execution rather than reading:
  - the earlier fixture's row was excluded by the strict `> $since` because the
    mark became its OWN id, not because it sat below the mark.
  - the predecessor comment said `main` "warned only on `null`". True of the two
    EMPTY shapes being contrasted; an empty body and a non-array object warned
    as well. Scoped.
  - `docs/ci-cd.md` and the record described the `::error::` as a two-way split
    (found vs unverifiable). Round 8's whole argument is that a complete read
    returning an IMPOSSIBLE answer is a third case, not a variety of the second
    — which is the operator-facing point, since it decides whether to go looking
    for an API failure that never happened. Both now say three.

The review re-verified, by comment-stripped diff, that round 8 changed no
executable line beyond the `raced_why` string and the if/elif restructure, and
independently reproduced both inertness measurements and the `origin/main`
predecessor behaviour.

Verification: `scripts/tests` 1097 passed, 2 skipped; decisions_validate and
build_decisions_catalog --check exit 0.

refs #763

Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

timothy commented 2026-08-28 18:43:49 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 462118d

Six independent cold reviews (1 cross-family Codex + 5 isolated Opus). Findings: a Blocker (mark walk widened a fail-open), a live-on-main twin defect (string id read as newer than any mark), four doc overclaims, five tests-passing-for-the-wrong-reason (two where the stub crashed). Two mechanisms added then withdrawn after each produced two defects. 18 executed mutations; 1097 tests pass.

Review-verdict: MERGEABLE @ 462118d Six independent cold reviews (1 cross-family Codex + 5 isolated Opus). Findings: a Blocker (mark walk widened a fail-open), a live-on-main twin defect (string id read as newer than any mark), four doc overclaims, five tests-passing-for-the-wrong-reason (two where the stub crashed). Two mechanisms added then withdrawn after each produced two defects. 18 executed mutations; 1097 tests pass.

timothy added 1 commit 2026-08-28 18:51:39 +02:00

fix(763): satisfy ruff — E741 and formatting on the touched test file ... 11287a54ba

CI's `Script lint and tests` job went red. Cause: I never ran ruff locally,
which this repo's Python convention requires after any .py change.

  - E741 twice: `l` as a comprehension variable in the sort-order guard.
  - `ruff format --check`: the file was correctly formatted on `main`; my edits
    broke it. One of them left a docstring line at column 0, which `ruff format`
    then "corrected" by over-indenting the rest of the paragraph — repaired at
    the source rather than accepting that rewrite.

Verified the way CI does: local ruff is the pinned 0.12.11, and both
`ruff check` and `ruff format --check` run under bash over the full tracked
population (`git ls-files -z '*.py' '*.pyi' '*.ipynb'`, 46 files) are clean.
The population is counted, not assumed — an empty glob would pass vacuously,
which is the failure `scripts/tests` guards against elsewhere.

`scripts/tests` 1097 passed, 2 skipped after the reformat.

refs #763

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

timothy commented 2026-08-28 18:52:06 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 11287a5

Re-verdict for the lint head. Delta vs the reviewed sha 462118d is provably logic-free: only scripts/tests/test_pr_changed_files.py changed, and its AST is identical apart from an l->line rename and whitespace (checked by normalised ast.dump). ruff check + ruff format --check clean over the full 46-file tracked population at the pinned 0.12.11; 1097 tests pass.

Review-verdict: MERGEABLE @ 11287a5 Re-verdict for the lint head. Delta vs the reviewed sha 462118d is provably logic-free: only scripts/tests/test_pr_changed_files.py changed, and its AST is identical apart from an l->line rename and whitespace (checked by normalised ast.dump). ruff check + ruff format --check clean over the full 46-file tracked population at the pinned 0.12.11; 1097 tests pass.

timothy merged commit 609fd852c2 into main 2026-08-28 19:34:49 +02:00

timothy deleted branch fix/763-page-statuses-reads 2026-08-28 19:34:50 +02:00

timothy referenced this pull request 2026-08-28 19:35:41 +02:00

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

timothy referenced this pull request 2026-08-28 21:44:12 +02:00

docs(796): verification code is code under test — and the proof it was claiming an exemption from #871

timothy referenced this pull request 2026-08-28 21:46:46 +02:00

A verification harness is itself unproven code — five false greens, each created by the fix for the last #796

timothy referenced this issue from a commit 2026-08-28 23:29:47 +02:00

fix(803,664): round 2 — the guard-inventory row, two shared-walk fail-opens, and a false measurement

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#868