From 007d2fd3df9b62dc4d02ded155f25ee750ecdf9d Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 28 Aug 2026 15:56:39 +0200 Subject: [PATCH] =?UTF-8?q?fix(763):=20round=202+3=20=E2=80=94=20close=20t?= =?UTF-8?q?he=20fail-opens=20the=20paging=20change=20introduced?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .gitea/workflows/review-verdict.yml | 221 ++++++---- docs/ci-cd.md | 44 +- docs/decisions/README.md | 2 +- .../ci/verdict-write-retarget-fence.md | 34 +- scripts/tests/test_pr_changed_files.py | 400 +++++++++++++----- 5 files changed, 497 insertions(+), 204 deletions(-) diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index c75ce44f2..21e3d05b6 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -752,25 +752,29 @@ jobs: # AN EMPTY PAGE 1 IS LEGITIMATE HERE, unlike `count_retargets`. A head nothing has posted to # yet genuinely has no statuses (and a bogus sha returns `[]` too — measured), so an empty # first page is an ordinary answer rather than the anomaly it is on a PR timeline. It yields - # `st_ok=yes` over zero rows. Copying the timeline walk's "one real page required" rule here + # `ph_ok=yes` over zero rows. Copying the timeline walk's "one real page required" rule here # would withhold the mark on every first run. # - # RETRY ONCE, THEN REFUSE. `st_ok=no` is consumed below as "could not establish", which on + # RETRY ONCE, THEN REFUSE. `ph_ok=no` is consumed below as "could not establish", which on # the post-write path repairs to the STICKY sentinel — so one transient blip would cost that # head its exemption until a human clears it by hand. A single retry absorbs the blip; # anything persistent still refuses, because on that path uncertainty must never resolve to # green. The retry covers a non-array body too (a 502 HTML page reads exactly like one). page_statuses() { - st_rows='[]' - st_ok=no + ph_rows='[]' + ph_ok=no local page=1 raw kind n acc='[]' try + # EVERY exit publishes what WAS read, not just the complete ones. `ph_ok` alone says + # whether the list is whole; a caller that can still use a partial list must be able to + # reach it, and the high-water mark below is exactly such a caller — see the fail-open it + # otherwise creates (ersatztv#763, round 2). while [ "$page" -le 20 ]; do raw="" kind="" # `if`/`then`, never `cmd && var=yes`: under `set -e` a bare `A && B` whose test fails is # itself a failing command, and as the last command of a loop body it would kill the job. for try in 1 2; do - raw=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA?limit=50&page=${page}") || raw="" + raw=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA?limit=50&page=${page}&sort=highestindex") || raw="" if [ -n "${raw//[[:space:]]/}" ]; then # Read the type as a VALUE, not through `jq -e`, for the reason recorded at # `count_retargets`: `jq -e` reports the truthiness of its last output, so it cannot @@ -778,18 +782,25 @@ jobs: kind=$(printf '%s' "$raw" | jq -r 'type' 2>/dev/null) || kind="" fi if [ "$kind" = array ]; then break; fi + # A SECOND IMMEDIATE REQUEST BARELY COUNTS AS A RETRY. One second does not survive a + # Gitea restart either, and saying so is the point: this absorbs a momentary blip, not + # an outage. An outage still lands on `ph_ok=no`, which is the safe direction. + sleep 1 done - if [ "$kind" != array ]; then return 0; fi + if [ "$kind" != array ]; then ph_rows=$acc; return 0; fi n=$(printf '%s' "$raw" | jq -r 'length' 2>/dev/null) || n="" - case "$n" in ''|*[!0-9]*) return 0 ;; esac - if [ "$n" -eq 0 ]; then st_ok=yes; st_rows=$acc; return 0; fi + case "$n" in ''|*[!0-9]*) ph_rows=$acc; return 0 ;; esac + if [ "$n" -eq 0 ]; then ph_ok=yes; ph_rows=$acc; return 0; fi acc=$(printf '%s\n%s' "$acc" "$raw" | jq -s -c 'add' 2>/dev/null) || acc="" if [ -z "$acc" ]; then return 0; fi + ph_rows=$acc page=$(( page + 1 )) done - # THE PAGE CAP IS NOT EXHAUSTION. Falling out of the loop leaves `st_ok=no`, so a history - # longer than 20 pages (1000 rows) is "could not establish" rather than a silent partial - # list — the same treatment as an unreadable page, and for the same reason. + # THE PAGE CAP IS NOT EXHAUSTION. Falling out of the loop leaves `ph_ok=no`, so an + # over-long history is "could not establish" rather than a silent partial list — the same + # treatment as an unreadable page, and for the same reason. The bound is 950 rows, not + # 1000: the 20th request has to be the EMPTY terminator for the walk to be validated, so + # only 19 full pages can be confirmed. return 0 } @@ -1080,8 +1091,29 @@ jobs: # raced — but it is a stall with no diagnosis attached, and it is the same read the # post-write check depends on being complete. page_statuses - if [ "$st_ok" = yes ]; then - mark=$(printf '%s' "$st_rows" | jq -r '[.[] | .id? // 0] | max // 0' 2>/dev/null || true) + # A PARTIAL LIST STILL YIELDS A USABLE MARK, and refusing one is a FAIL-OPEN (round 2). + # Skipping the mark skips the post-write race check entirely, so a human rejection landing + # in the write window is neither detected nor repaired and the exemption stands green over + # it. Before paging existed only a failure of the single page-1 request could reach that; + # requiring a COMPLETE walk here would newly route a page-2 hiccup, an over-cap history or + # one malformed id on a later page into the same hole — turning a paging fix into a wider + # fail-open than the bug it closes. + # + # A mark taken over fewer rows can only be LOWER than the true maximum, and low is the safe + # direction: it makes the post-write check MORE eager (it may repair over a pre-existing + # row), never blinder. That is the same trade the pre-paging code made every time, since a + # single page was all it ever read. + ph_len=$(printf '%s' "$ph_rows" | jq -r 'if type == "array" then length else 0 end' 2>/dev/null) || ph_len="" + case "$ph_len" in ''|*[!0-9]*) ph_len=0 ;; esac + if [ "$ph_ok" != yes ] && [ "$ph_len" -gt 0 ]; then + echo "::warning::Could not page the whole status history for ${SHA:0:7}; taking the high-water mark over the ${ph_len} row(s) that were read. The mark may be below the true maximum, which makes the post-write race check more eager, never blinder." + fi + if [ "$ph_ok" = yes ] || [ "$ph_len" -gt 0 ]; then + # NUMBERS ONLY. jq orders strings above every number, so a single `"id": "12"` from a + # schema-corrupt row becomes the max, passes the `*[!0-9]*` gate as `12`, and — with a + # larger string — silently inflates the mark until nothing looks newer than it. That is a + # fail-open, so ids that are not numbers are excluded rather than coerced. + mark=$(printf '%s' "$ph_rows" | jq -r '[.[] | select(type == "object") | .id | numbers] | max // 0' 2>/dev/null || true) case "$mark" in ''|*[!0-9]*) # SKIP the check rather than treat everything as raced. A mark of 0 would make every @@ -1216,13 +1248,25 @@ jobs: # live head), `/statuses/{sha}` a BARE ARRAY (24 rows on the same head) — hence the different # `type == "array"` guard here. # - # ORDER IS NOT RELIED ON, and since ersatztv#763 that is finally true without a caveat. - # The check selects by id against the high-water mark rather than inspecting the top of the - # list, so a verdict older than the mark is invisible to it no matter where it sits — and - # the read is now the WHOLE list, paged by `page_statuses`, not one clamped page. The - # ordering is only coarsely newest-first (ids came back `33,32,31,30,28,29,27,…`), so - # position was never safe to reason from; what changed is that position no longer decides - # whether a row is read at all. + # ORDER IS NOT RELIED ON *WITHIN* THE RESULT — the check selects by id against the + # high-water mark rather than inspecting the top of the list, so a verdict older than the + # mark is invisible to it no matter where it sits. The read is now the whole list rather + # than one clamped page. + # + # "THE WHOLE LIST" MEANS WHAT WAS PRESENT WHEN EACH PAGE WAS FETCHED, not a snapshot. These + # are independent offset-paginated GETs with no snapshot token, so a row INSERTED MID-WALK + # can be missed: under the server default (`created_unix DESC`, measured) a new row lands at + # position 0, on a page already read, while everything else shifts down — so the walk + # re-reads a duplicate and never sees the newcomer. That is why the request asks for + # `sort=highestindex`, which returns `index` ASCENDING (measured 2026-08-28: page 1 ids + # `1,2,3,4,5`, page 2 `51,52,…`, 114 unique ids across the walk), so an insert appends at the + # END and lands on a later page the walk has yet to fetch. + # + # The residual, stated rather than implied: an UNKNOWN sort value is silently ignored and + # falls back to DESC (measured), so if that parameter is ever renamed this degrades quietly + # to the mid-walk-insert gap above. The consequence is bounded — a verdict arriving after + # this job's POST is not one this job overwrote, and it wins on the combined endpoint that + # branch protection reads — so this is insurance, not the mechanism the repair depends on. # # That matters more here than anywhere else in this job, because this is the ONE path whose # failure direction is toward SUCCESS: missing a raced human `failure` leaves a forged green @@ -1245,7 +1289,7 @@ jobs: # sentinel DESCRIPTION stays generic on purpose — the classification recognises it as a # fixed point, so its wording is load-bearing — but the `::error::` beside it need not be. raced_why=human - if [ "$st_ok" != yes ]; then + if [ "$ph_ok" != yes ]; then # FAIL CLOSED, AND THAT IS A BEHAVIOUR CHANGE (ersatztv#763). This used to warn and leave # the exemption green, while #751's page-2 probe a few lines further down repaired on # exactly the same uncertainty — the two halves of one check disagreed about which way @@ -1303,81 +1347,92 @@ jobs: # It cannot false-fire. A sentinel that already existed would have been seen at the FIRST # read, forcing the pending path, and this block only runs after a `success` — so a # sentinel ABOVE the mark can only have been written by another run mid-flight. - raced=$(printf '%s' "$st_rows" | jq -r --arg c "$CONTEXT" --argjson since "$max_id_before" --arg rd "$REPAIR_DESC" \ + # `.creator` IS TYPE-TESTED BEFORE IT IS INDEXED, and the substitution is guarded. + # `.creator != null and .creator.login` hard-errors ("Cannot index number with string") + # on a row whose `creator` is any non-object, jq exits 5, and under `set -e` the + # assignment takes the step down — AFTER the exemption `success` is already posted and + # with the repair never attempted, leaving a forged green on a head that may carry a + # rejection. Reproduced (round 2). `(.creator | type) == "object"` short-circuits jq's + # `and` before the index, so a malformed row is dropped from the count instead of + # killing the job, and a well-formed verdict beside it is still counted. + raced=$(printf '%s' "$ph_rows" | jq -r --arg c "$CONTEXT" --argjson since "$max_id_before" --arg rd "$REPAIR_DESC" \ '[.[] | select(type == "object") | select(.context? == $c) | select((.id? // 0) > $since) | select( - ((.creator != null and .creator.login != null and .creator.login != "") + (((.creator | type) == "object") and ((.creator.login // "") != "") and (((.description // "") | startswith("Review-verdict:")))) or ((.creator == null) and ((.description // "") == $rd)) - )] | length') - # THE READ MUST WITNESS OUR OWN WRITE (ersatztv#763). This replaces #751's page-2 - # probe, which treated "there are rows I did not read" as "assume raced" — the only - # conservative move available without paging, and one that fired for real on Renovate - # PR #761: a head with more rows than the clamp got an `::error::` asserting a human - # verdict had been overwritten when none existed, and the STICKY sentinel then refused - # to re-exempt that head on every later run. Paging removes the false positive AND the - # need for the probe, because there is no longer an unread page to reason about. + )] | length') || raced="" + # NO "CURRENCY WITNESS" HERE, AND THAT IS A DELIBERATE WITHDRAWAL (round 2). + # A draft of this change also asserted that the post-write read must show at least one + # row above the mark, on the grounds that reaching a validated empty page proves the + # walk finished but not that it saw the POST just made. Two independent defects came out + # of that one mechanism, which is the signal to remove it rather than patch it twice: # - # What paging does NOT establish is that the list is CURRENT. `st_ok=yes` proves the - # walk reached a validated empty page; it does not prove the walk saw the POST that just - # happened. So assert a positive witness instead of an absence: our own row was written - # after the mark was taken, so at least one row with `id > $max_id_before` must exist. - # Zero means the read cannot be showing post-POST reality — replica lag, a cache, a - # wrong sha — and a `raced=0` derived from it would be a green nobody verified. + # * it did not witness what it claimed. Counting ANY row above the mark accepts an + # unrelated newer row, so a stale-but-valid snapshot carrying that row and not the + # human rejection passed the check and left the exemption green — the exact + # fail-toward-SUCCESS it was added to prevent. + # * it converted a single transient anomaly into a PERMANENT sentinel. A schema-valid + # stale read is not retried (only transport failures and non-array bodies are), so + # one such response cost that head its exemption until a human cleared it. # - # DELIBERATELY NOT "our exact row is present": concurrent runs post indistinguishable - # exemption rows, so pinning identity would fail on a race this check is not there to - # detect. Any row above the mark proves currency, which is all that is being claimed. - above=$(printf '%s' "$st_rows" | jq -r --argjson since "$max_id_before" \ - '[.[] | select(type == "object") | select((.id? // 0) > $since)] | length' 2>/dev/null) || above="" - case "$above" in - ''|*[!0-9]*) - echo "::warning::Could not count the status rows newer than the pre-write high-water mark for ${SHA:0:7}; treating the post-write history as unverified and repairing ${CONTEXT} to pending." - raced_why="the post-write history could not be counted" - raced=1 ;; - 0) - echo "::warning::The status history for ${SHA:0:7} shows no row newer than the pre-write high-water mark, yet this job just posted one — so the read cannot reflect the write it is meant to verify. Repairing ${CONTEXT} to pending rather than leaving an exemption green on an unverified head." - raced_why="the post-write read did not reflect this job's own write" - raced=1 ;; - esac + # The hazard it addressed is also not one this deployment plausibly has: Gitea here is a + # single instance with no read replicas, so "the read does not reflect the write" has no + # mechanism behind it. Removing it restores exactly the pre-change exposure on this + # path — a non-regression — while the paging above closes the hole #763 is actually + # about. If a real instance of a stale read ever appears, it needs a witness that + # identifies THIS job's own row, plus a re-read before repairing. fi case "$raced" in ''|*[!0-9]*) - echo "::warning::Post-write verification for ${SHA:0:7} returned '${raced}' instead of a count; not acting on it." - ;; - *) - if [ "$raced" -gt 0 ]; then - # The last-moment re-read found no human verdict, so any row present now was - # written during the window and has just been masked by the exemption above. - # DO NOT ASSERT AN OVERWRITE THAT MAY NOT HAVE HAPPENED (ersatztv#763). This - # message used to be unconditional, so when the old page-2 probe repaired on mere - # uncertainty it reported a human verdict as overwritten when none existed — which - # is what made the live PR #761 stall undiagnosable from the status alone. - if [ "$raced_why" = human ]; then - echo "::error::A human ${CONTEXT} verdict landed on ${SHA:0:7} while this job was writing its exemption, and was overwritten. Downgrading to 'pending' so an explicit human decision cannot be silently green. Re-post it with: scripts/post-review-verdict.sh ${PR} " - else - echo "::error::Could not verify that no human ${CONTEXT} verdict raced this exemption write on ${SHA:0:7} — ${raced_why}. Downgrading to 'pending' rather than leaving an unverified exemption green; no verdict was necessarily overwritten. Clear it with: scripts/post-review-verdict.sh ${PR} " - fi - repair=$(jq -n --arg c "$CONTEXT" --arg u "$PR_URL" --arg d "$REPAIR_DESC" \ - '{state:"pending", context:$c, description:$d, target_url:$u}') - # A failure HERE leaves the forged green standing, so it is retried once and then - # screams. `set -e` would otherwise kill the job silently, after the success was - # written and with nothing left to re-attempt. - if ! gh -X POST -H 'Content-Type: application/json' -d "$repair" \ - "$BASE_URL/repos/$REPO/statuses/$SHA" >/dev/null 2>&1; then - if ! gh -X POST -H 'Content-Type: application/json' -d "$repair" \ - "$BASE_URL/repos/$REPO/statuses/$SHA" >/dev/null 2>&1; then - echo "::error::COULD NOT REPAIR ${CONTEXT} on ${SHA:0:7}. An exemption 'success' is standing on a head whose human verdict was overwritten. Post the verdict again immediately: scripts/post-review-verdict.sh ${PR} " - exit 1 - fi - fi - echo "Repaired ${CONTEXT} to pending on ${SHA:0:7}." - state=pending - fi + # FAILS CLOSED (round 2). "Not acting on it" left the exemption green on a head whose + # write window could not be inspected at all — the same uncertainty every other branch + # here resolves to `pending`, resolved the opposite way purely because it arrived as a + # malformed count rather than a failed read. + # + # NO FIXTURE REACHES THIS BRANCH, and that is stated rather than implied. With the + # type-safe filter above, `raced` is a number for every input the stub can pose, so + # this arm is defense-in-depth against a future jq or schema change and is proved only + # in combination (disarming it alone reddens nothing — measured, not assumed). + echo "::warning::Post-write verification for ${SHA:0:7} returned '${raced}' instead of a count, so a raced verdict could not be ruled out; repairing ${CONTEXT} to pending." + raced_why="the post-write verification did not return a count" + raced=1 ;; esac + # NORMALISE FIRST, THEN ACT ONCE. Setting `raced=1` inside a `case` arm and expecting the + # repair to happen in a SIBLING arm does not work — the case has already dispatched. That + # is why the unusable-count branch above sets the flag and the decision lives out here, + # where every path that concluded "raced" reaches the same single writer. + if [ "$raced" -gt 0 ]; then + # The last-moment re-read found no human verdict, so any row present now was + # written during the window and has just been masked by the exemption above. + # DO NOT ASSERT AN OVERWRITE THAT MAY NOT HAVE HAPPENED (ersatztv#763). This + # message used to be unconditional, so when the old page-2 probe repaired on mere + # uncertainty it reported a human verdict as overwritten when none existed — which + # is what made the live PR #761 stall undiagnosable from the status alone. + if [ "$raced_why" = human ]; then + echo "::error::A human ${CONTEXT} verdict landed on ${SHA:0:7} while this job was writing its exemption, and was overwritten. Downgrading to 'pending' so an explicit human decision cannot be silently green. Re-post it with: scripts/post-review-verdict.sh ${PR} " + else + echo "::error::Could not verify that no human ${CONTEXT} verdict raced this exemption write on ${SHA:0:7} — ${raced_why}. Downgrading to 'pending' rather than leaving an unverified exemption green; no verdict was necessarily overwritten. Clear it with: scripts/post-review-verdict.sh ${PR} " + fi + repair=$(jq -n --arg c "$CONTEXT" --arg u "$PR_URL" --arg d "$REPAIR_DESC" \ + '{state:"pending", context:$c, description:$d, target_url:$u}') + # A failure HERE leaves the forged green standing, so it is retried once and then + # screams. `set -e` would otherwise kill the job silently, after the success was + # written and with nothing left to re-attempt. + if ! gh -X POST -H 'Content-Type: application/json' -d "$repair" \ + "$BASE_URL/repos/$REPO/statuses/$SHA" >/dev/null 2>&1; then + if ! gh -X POST -H 'Content-Type: application/json' -d "$repair" \ + "$BASE_URL/repos/$REPO/statuses/$SHA" >/dev/null 2>&1; then + echo "::error::COULD NOT REPAIR ${CONTEXT} on ${SHA:0:7}. An exemption 'success' is standing on a head whose write window could not be cleared (${raced_why}). Post a verdict immediately: scripts/post-review-verdict.sh ${PR} " + exit 1 + fi + fi + echo "Repaired ${CONTEXT} to pending on ${SHA:0:7}." + state=pending + fi fi if [ "$state" = "pending" ]; then diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 5957112c6..2e5faeab0 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1578,29 +1578,49 @@ verified, and a rejection it masks is re-derived green by a later run (ersatztv# for the high-water mark, after it for the race check — and `limit` clamps to `MAX_RESPONSE_ITEMS` (measured 50), so a single read of a busy head saw a partial list. The post-write direction is the one that mattered: a raced human verdict on an unread page left the exemption green over a rejection. Both -reads now walk to a **validated empty page**, never terminating on a short one, under a 20-page cap. -The terminator is measured per endpoint and they differ: `/statuses/{sha}` returns `[]`, +reads now walk to a **validated empty page**, never terminating on a short one, retrying each page +once, under a 20-page cap — which validates at most 950 rows, since the 20th request must be the empty +terminator. The terminator is measured per endpoint and they differ: `/statuses/{sha}` returns `[]`, `/issues/{n}/timeline` a bare `null`, `/commits/{sha}/status` an object with `statuses: null`. -Correctness does not depend on the page size — any cap pages correctly, which is the property the -dead `limit=100` guard below lacked. +Correctness does not depend on the page size — any cap pages correctly, which is the property the dead +`limit=100` guard below lacked. This **replaced** ersatztv#751's conservative page-2 probe, which treated "there are rows I did not read" as "assume raced". That probe fired on Renovate PR #761: a head that grew past one page over ordinary CI re-runs had its exemption repaired away, with an `::error::` asserting a human verdict was overwritten when the head carried none, and the sticky sentinel then refused re-exemption on every -later run. Two consequences of the replacement: +later run. -- **Uncertainty fails closed at both ends.** An unreadable history used to warn and leave the exemption - green while the page-2 probe repaired on the same uncertainty; both now repair to `pending`. That is - affordable only because paging removed the common trigger (an ordinary head over 50 rows). -- **The 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 the check asserts a positive fact: at least one row above - the pre-write high-water mark must exist, because the job just posted one. Zero means the read cannot - reflect the write it is verifying. +The two directions are **not** symmetric, and the asymmetry is deliberate: + +- **Post-write, uncertainty fails closed.** An unreadable history, an over-cap history, or a count that + does not come back as a number all repair to `pending`. Previously an unreadable history warned and + left the exemption green while the page-2 probe repaired on the same uncertainty — one check + disagreeing with itself. Failing closed is affordable only because paging removed the common trigger + (an ordinary head over 50 rows). +- **Pre-write, a partial list still yields a mark.** The mark gates the post-write check entirely, so + refusing one *skips* the check and is itself a fail-open. A mark over fewer rows 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 the mark — the pre-existing gap tracked as ersatztv#849, unchanged here. + +Three smaller properties, each closing a fail-open rather than a stall: the walk requests +`sort=highestindex` so a row inserted mid-walk appends at the end and lands on a page not yet fetched +(the server default `created_unix DESC` puts it at position 0, on a page already read) — an unknown +sort value silently falls back to DESC, so this is insurance, not load-bearing; the high-water mark is +the max over **numeric** ids only, since jq orders strings above every number and one `"id": "99999"` +would sail through the numeric gate and inflate the mark until nothing looked newer; and `.creator` is +type-tested before it is indexed, because `.creator.login` on a non-object exits jq 5 and, under +`set -e`, took the step down after the green was posted and before the repair. The `::error::` now distinguishes a verdict actually found from an unverifiable read; the sentinel *description* stays generic, because the classification recognises it as a fixed point. +A draft of this change also asserted that the post-write read must show a row above the mark (proving +it reflects the POST just made). That was **withdrawn**: counting any row above the mark does not +witness this job's own write, so a stale-but-valid snapshot carrying an unrelated newer row still +passed, and a single schema-valid stale read created a permanent sentinel. Gitea here is a single +instance with no read replicas, so the hazard has no mechanism behind it on this deployment. + Three properties of this workflow are security-relevant and are **structurally** asserted by tests in `scripts/tests/test_pr_changed_files.py` — those tests pin the workflow's shape, which is not the same as establishing that the gate cannot be forged (see the residual below, and ersatztv#697/#698): diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 09ee2b4da..6a503a68c 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -67,7 +67,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.small-lane-git-only` | `runs-on: small` is defined by what a job does (git-only), not its usual runtime; the two `docker build` jobs (docker-build.yml, ci-image.yml) move to `ubuntu-latest` because their worst-case memory, not median runtime, was pinning the small lane's per-slot cap. | 2026-07-20 | [link](records/ci/small-lane-git-only.md) | | `ci.toolchain-image-publish-is-a-dispatch` | A `push` trigger reachable from any ref other than `main` — that is `branches:` AND `tags:`, judged by the ref class it admits and never by which keyword is present — executes ref-supplied YAML, because Gitea resolves a `push` workflow's definition from the pushed ref — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. Be precise about what a `branches:` filter buys: it is loaded from the pushed ref like the rest of the file, so a branch that deletes it re-enables the route — this removes the DRIVE-BY case, and is not a boundary against a writer who intends to run their own YAML. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`'s `expected` in the same change — a DECIDED tradeoff, not a necessity: keeping it is workable via the branch dispatch, but prices every edit to that file, comments included, at a ~2GB publish plus a five-pin bump, redone after every rebase. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, and the ONLY remedy is to make it alongside a `docker/ci/**` edit: publishing after the merge and then pinning cannot work, because `expected` is the last `docker/ci` commit and would reject that pin. This closes the push route INTO THIS FILE, not the class: `docker-build.yml` remains reachable from an arbitrary ref by a `v*` tag push and by `pull_request`, and four workflows carry an unrestricted `workflow_dispatch` (#853). | 2026-08-27 | [link](records/ci/toolchain-image-publish-is-a-dispatch.md) | | `ci.ui-e2e-harness` | The UI-interactive E2E flows run as headless Playwright specs (`web/e2e/*.spec.ts`, driven by `scripts/e2e-ui.sh`) in a **second step of the existing advisory `functional-e2e` job**, never their own job; the browser is `chromium-headless-shell` **baked into the CI toolchain image** (`docker/ci/Dockerfile`, `PLAYWRIGHT_VERSION` kept equal to `web/package.json`'s EXACT `@playwright/test` pin), never installed per run; specs are `serial` with `retries: 0` and assert only contracts the curl harness structurally cannot reach. | 2026-07-25 | [link](records/ci/ui-e2e-harness.md) | -| `ci.verdict-write-retarget-fence` | 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. This NARROWS the residual, it does not resolve it — a retarget between the FINAL pre-write count and the POST still yields a PERMANENT forged green, because the successor run can consume the `edited` event and exit before the stale run posts last (corrected 2026-08-27, #849). Abstaining is a handoff, not a stall, and that is the property the design rests on — for a run that ABSTAINS; it says nothing about one that already passed its final count and then posts (see the residual, corrected #849): 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 — but ONLY over runs that ABSTAIN. A run that already passed its final count is outside it: it writes whenever it gets there, so the run that writes LAST is not necessarily the one that classified last, and that is the permanent residual (#849). `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` blocks the merge immediately while withholding it would strand ordinary PRs whenever the timeline is unreadable — the right trade, but NOT a free one ("for no safety gain" retracted 2026-08-27): a GENERIC `pending` masks a rejection landing in its own write window, post-write verification does not run for it, and a later run re-derives it into an exemption `success` (#849). SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` IN FULL (PAGED since #763) 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. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run's `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run's mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. BOTH `/statuses/{sha}` reads PAGE to a validated EMPTY page since #763 — `[]` on this endpoint, a THIRD terminator shape distinct from the timeline's bare `null` and the combined endpoint's `{"statuses": null}`, so each terminator is MEASURED per endpoint — never terminating on a SHORT page, under a 20-page cap; correctness does not depend on the measured cap of 50, because any page size pages correctly. This RETIRED #751's conservative page-2 probe, which treated "there are rows I did not read" as "assume raced" and so repaired every head that outgrew one page: it fired on Renovate PR #761, reporting a human verdict as overwritten when none existed and then, the sentinel being sticky, refusing to re-exempt that head on every later run. Two properties replace it. Uncertainty now fails CLOSED at both ends — an unreadable history used to warn and leave the exemption green while the page-2 probe repaired on the very same uncertainty — which is affordable ONLY because paging removed the common trigger, an ordinary head carrying more than 50 rows. 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, and zero means the read cannot reflect the write it is verifying. The `::error::` now distinguishes a verdict actually FOUND from an unverifiable read, while the sentinel DESCRIPTION stays generic because the classification recognises it as a fixed point. | 2026-08-03 | [link](records/ci/verdict-write-retarget-fence.md) | +| `ci.verdict-write-retarget-fence` | 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. This NARROWS the residual, it does not resolve it — a retarget between the FINAL pre-write count and the POST still yields a PERMANENT forged green, because the successor run can consume the `edited` event and exit before the stale run posts last (corrected 2026-08-27, #849). Abstaining is a handoff, not a stall, and that is the property the design rests on — for a run that ABSTAINS; it says nothing about one that already passed its final count and then posts (see the residual, corrected #849): 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 — but ONLY over runs that ABSTAIN. A run that already passed its final count is outside it: it writes whenever it gets there, so the run that writes LAST is not necessarily the one that classified last, and that is the permanent residual (#849). `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` blocks the merge immediately while withholding it would strand ordinary PRs whenever the timeline is unreadable — the right trade, but NOT a free one ("for no safety gain" retracted 2026-08-27): a GENERIC `pending` masks a rejection landing in its own write window, post-write verification does not run for it, and a later run re-derives it into an exemption `success` (#849). SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` IN FULL (PAGED since #763) 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. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run's `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run's mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. BOTH `/statuses/{sha}` reads PAGE to a validated EMPTY page since #763 — `[]` on this endpoint, a THIRD terminator shape distinct from the timeline's bare `null` and the combined endpoint's `{"statuses": null}`, so each terminator is MEASURED per endpoint — never terminating on a SHORT page, retrying each page once, under a 20-page cap that validates at most 950 rows (the 20th request must be the empty terminator); correctness does not depend on the measured cap of 50, because any page size pages correctly. This RETIRED #751's conservative page-2 probe, which treated "there are rows I did not read" as "assume raced" and so repaired every head that outgrew one page: it fired on Renovate PR #761, reporting a human verdict as overwritten when none existed and then, the sentinel being sticky, refusing to re-exempt that head on every later run. The two directions are NOT symmetric. POST-WRITE, uncertainty fails CLOSED — an unreadable history, an over-cap history, or a count that is not a number all repair to `pending`; previously an unreadable history warned and left the exemption green while the page-2 probe repaired on the same uncertainty, one check disagreeing with itself. PRE-WRITE, a PARTIAL list still yields a mark, because the mark gates the post-write check entirely and refusing one SKIPS that check, which is itself the fail-open; a mark over fewer rows can only be LOWER than the true maximum, which makes the check more eager, never blinder, and only a read returning no rows at all abandons it (the pre-existing #849 gap, unchanged). The `::error::` now distinguishes a verdict actually FOUND from an unverifiable read, while the sentinel DESCRIPTION stays generic because the classification recognises it as a fixed point. `.creator` is TYPE-TESTED before it is indexed: `.creator != null and .creator.login` hard-errors on a non-object creator, jq exits 5, and under `set -e` that took the step down AFTER the exemption was posted and BEFORE the repair — a forged green reported as an infrastructure error. | 2026-08-03 | [link](records/ci/verdict-write-retarget-fence.md) | | `ci.verify-locally-ci-confirms` | Treat the local build/verify/review pass as the decision point and CI as confirmation — don't idle waiting on a run you have no reason to doubt. | 2026-07-21 | [link](records/ci/verify-locally-ci-confirms.md) | | `ci.web-test-per-test-timeouts` | Give heavy-render web tests an explicit per-test vitest timeout (e.g. 15s); never raise the global default to fix one slow test. | 2026-07-21 | [link](records/ci/web-test-per-test-timeouts.md) | | `ci.workflow-run-body-no-expressions` | A `run:` body is not shell when the runner reads it: the runner scans the whole scalar for the expression opener and, on finding one, rewrites the ENTIRE body into a single `format(...)` call. That rewrite is all-or-nothing, so a payload that does not evaluate fails the interpolation of the whole scalar — and the runner then DROPS THE STEP AND CONCLUDES THE JOB `success`. A shell comment is therefore NOT inert. In `.gitea/workflows/review-verdict.yml` no expression delimiter may appear in ANY `run:` body, in code or in prose, because a dropped step there is a dead merge gate rather than a failed build; pass values in through the step's `env:` block, which is interpolated per value so a bad payload cannot take the body with it, and describe an expression in prose by NAMING it (`a github.event.pull_request.number expression`) rather than quoting the delimiters. Repo-wide the rule is weaker and its reach must be stated precisely rather than generously: every expression payload in every workflow field must have a HEAD TOKEN naming a context or function the runner can resolve. That catches the defect above and a nonexistent context; it does NOT catch a syntactically invalid payload whose tokens are all known (`${{ github.ref == }}`), a renamed output (every token after the first is skipped), or an unclosed opener — those need an expression parser, and the guard is kept permissive on purpose because a red here blocks every merge through the combined status. In `review-verdict.yml` specifically, any step whose non-execution is consequential is paired with a start-marker guard that FAILS the job when the marker is absent, and that guard's own body must be expression-free — a guard the guarded mechanism can silently delete is worse than none. That pairing now also covers `docker-build.yml`'s `test` and `migrations` jobs, where a dropped step is fail-OPEN (the required check goes green having done no work) rather than fail-closed as it is here — see `ci.required-job-step-execution-markers`, which adds per-STEP markers there and extends this file's delimiter ban to those two jobs. It is still not a repo-wide property, but the remaining exceptions are narrower than this record originally said: `build` was brought into the ban too (its `Smoke + IPTV E2E` runs AFTER the image is pushed, so a drop there ships an unsmoked release candidate — its two payloads moved to `env:`, so the ban was free), leaving only `api-docs` and `format`, whose one `github.base_ref` each sits in a detect step that gates nothing that ships. | 2026-08-06 | [link](records/ci/workflow-run-body-no-expressions.md) | diff --git a/docs/decisions/records/ci/verdict-write-retarget-fence.md b/docs/decisions/records/ci/verdict-write-retarget-fence.md index 88c233bbe..f98fea030 100644 --- a/docs/decisions/records/ci/verdict-write-retarget-fence.md +++ b/docs/decisions/records/ci/verdict-write-retarget-fence.md @@ -5,9 +5,9 @@ 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. This NARROWS the residual, it does not resolve it — a retarget between the FINAL pre-write count and the POST still yields a PERMANENT forged green, because the successor run can consume the `edited` event and exit before the stale run posts last (corrected 2026-08-27, #849). Abstaining is a handoff, not a stall, and that is the property the design rests on — for a run that ABSTAINS; it says nothing about one that already passed its final count and then posts (see the residual, corrected #849): 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 — but ONLY over runs that ABSTAIN. A run that already passed its final count is outside it: it writes whenever it gets there, so the run that writes LAST is not necessarily the one that classified last, and that is the permanent residual (#849). `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` blocks the merge immediately while withholding it would strand ordinary PRs whenever the timeline is unreadable — the right trade, but NOT a free one ("for no safety gain" retracted 2026-08-27): a GENERIC `pending` masks a rejection landing in its own write window, post-write verification does not run for it, and a later run re-derives it into an exemption `success` (#849). SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` IN FULL (PAGED since #763) 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. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run''s `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run''s mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. BOTH `/statuses/{sha}` reads PAGE to a validated EMPTY page since #763 — `[]` on this endpoint, a THIRD terminator shape distinct from the timeline''s bare `null` and the combined endpoint''s `{"statuses": null}`, so each terminator is MEASURED per endpoint — never terminating on a SHORT page, under a 20-page cap; correctness does not depend on the measured cap of 50, because any page size pages correctly. This RETIRED #751''s conservative page-2 probe, which treated "there are rows I did not read" as "assume raced" and so repaired every head that outgrew one page: it fired on Renovate PR #761, reporting a human verdict as overwritten when none existed and then, the sentinel being sticky, refusing to re-exempt that head on every later run. Two properties replace it. Uncertainty now fails CLOSED at both ends — an unreadable history used to warn and leave the exemption green while the page-2 probe repaired on the very same uncertainty — which is affordable ONLY because paging removed the common trigger, an ordinary head carrying more than 50 rows. 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, and zero means the read cannot reflect the write it is verifying. The `::error::` now distinguishes a verdict actually FOUND from an unverifiable read, while the sentinel DESCRIPTION stays generic because the classification recognises it as a fixed point.' +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. This NARROWS the residual, it does not resolve it — a retarget between the FINAL pre-write count and the POST still yields a PERMANENT forged green, because the successor run can consume the `edited` event and exit before the stale run posts last (corrected 2026-08-27, #849). Abstaining is a handoff, not a stall, and that is the property the design rests on — for a run that ABSTAINS; it says nothing about one that already passed its final count and then posts (see the residual, corrected #849): 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 — but ONLY over runs that ABSTAIN. A run that already passed its final count is outside it: it writes whenever it gets there, so the run that writes LAST is not necessarily the one that classified last, and that is the permanent residual (#849). `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` blocks the merge immediately while withholding it would strand ordinary PRs whenever the timeline is unreadable — the right trade, but NOT a free one ("for no safety gain" retracted 2026-08-27): a GENERIC `pending` masks a rejection landing in its own write window, post-write verification does not run for it, and a later run re-derives it into an exemption `success` (#849). SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` IN FULL (PAGED since #763) 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. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run''s `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run''s mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. BOTH `/statuses/{sha}` reads PAGE to a validated EMPTY page since #763 — `[]` on this endpoint, a THIRD terminator shape distinct from the timeline''s bare `null` and the combined endpoint''s `{"statuses": null}`, so each terminator is MEASURED per endpoint — never terminating on a SHORT page, retrying each page once, under a 20-page cap that validates at most 950 rows (the 20th request must be the empty terminator); correctness does not depend on the measured cap of 50, because any page size pages correctly. This RETIRED #751''s conservative page-2 probe, which treated "there are rows I did not read" as "assume raced" and so repaired every head that outgrew one page: it fired on Renovate PR #761, reporting a human verdict as overwritten when none existed and then, the sentinel being sticky, refusing to re-exempt that head on every later run. The two directions are NOT symmetric. POST-WRITE, uncertainty fails CLOSED — an unreadable history, an over-cap history, or a count that is not a number all repair to `pending`; previously an unreadable history warned and left the exemption green while the page-2 probe repaired on the same uncertainty, one check disagreeing with itself. PRE-WRITE, a PARTIAL list still yields a mark, because the mark gates the post-write check entirely and refusing one SKIPS that check, which is itself the fail-open; a mark over fewer rows can only be LOWER than the true maximum, which makes the check more eager, never blinder, and only a read returning no rows at all abandons it (the pre-existing #849 gap, unchanged). The `::error::` now distinguishes a verdict actually FOUND from an unverifiable read, while the sentinel DESCRIPTION stays generic because the classification recognises it as a fixed point. `.creator` is TYPE-TESTED before it is indexed: `.creator != null and .creator.login` hard-errors on a non-object creator, jq exits 5, and under `set -e` that took the step down AFTER the exemption was posted and BEFORE the repair — a forged green reported as an infrastructure error.' 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, which is a page of EITHER `null` (what this endpoint really returns past the end) or `[]` — an `array`-only type gate read the real terminator as unreadable and withheld every exemption (#751); `retargets_before`/`retargets_before_ok` captured before enumeration, re-counted immediately before the POST; `page_statuses()` pages `GET /repos/{repo}/statuses/{sha}?limit=50&page=N` (cap 20) setting `st_rows`/`st_ok`, trusted only on a validated EMPTY ARRAY page (this endpoint''s terminator, measured 2026-08-28 on PR #761''s 114-row head: pages 1-2 return 50, page 3 returns 14, page 4 is `[]`), retrying each page once; `max_id_before` is the max id over the FULL paged list (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`, `test_a_status_history_RUNNING_PAST_PAGE_1_is_PAGED_and_the_exemption_STANDS`, `test_a_raced_verdict_on_PAGE_2_is_detected_and_repaired`, `test_a_post_write_read_that_CANNOT_SEE_OUR_OWN_WRITE_is_not_trusted`' +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, which is a page of EITHER `null` (what this endpoint really returns past the end) or `[]` — an `array`-only type gate read the real terminator as unreadable and withheld every exemption (#751); `retargets_before`/`retargets_before_ok` captured before enumeration, re-counted immediately before the POST; `page_statuses()` pages `GET /repos/{repo}/statuses/{sha}?limit=50&page=N&sort=highestindex` (cap 20) setting `ph_rows`/`ph_ok`, trusted only on a validated EMPTY ARRAY page (this endpoint''s terminator, measured 2026-08-28 on PR #761''s 114-row head: pages 1-2 return 50, page 3 returns 14, page 4 is `[]`), retrying each page once with a 1s backoff; `sort=highestindex` returns `index` ASCENDING (measured) so a row inserted MID-WALK appends at the end and lands on a page not yet fetched, where the server default `created_unix DESC` puts it at position 0 on a page already read — an unknown sort value is silently ignored and falls back to DESC, so this is insurance, not a mechanism the repair depends on; `max_id_before` is the max over NUMERIC ids only in the paged list (jq orders strings above every number, so one `"id": "99999"` would otherwise pass the numeric gate and inflate the mark until nothing looked newer than it — a fail-open) (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`, `test_a_status_history_RUNNING_PAST_PAGE_1_is_PAGED_and_the_exemption_STANDS`, `test_a_raced_verdict_on_PAGE_2_is_detected_and_repaired`, `test_a_verdict_BEYOND_A_SHORT_PAGE_is_still_found`, `test_a_PRE_WRITE_paging_failure_still_yields_a_usable_high_water_mark`, `test_a_TRANSIENT_page_failure_is_retried_and_the_walk_still_completes`, `test_a_STRING_id_cannot_inflate_the_high_water_mark`, `test_a_MALFORMED_creator_row_does_not_kill_the_job_after_the_green_is_posted`' --- `ci.exemption-provenance` closed three routes into the exemption path and left one residual it named: @@ -138,13 +138,25 @@ Two things paging does not buy on its own, both now explicit: exemption green while the page-2 probe repaired on the same uncertainty — one check disagreeing with itself. Both repair now. That is affordable only because paging removed the common trigger; failing closed on top of the old single-page read would have cost the exemption on every busy PR. -- **A completed walk is not a current one.** `st_ok=yes` proves the walk reached a validated empty - page, not that it saw the POST just made. So the check asserts a positive fact — at least one row - above the pre-write mark, because the job itself wrote one — rather than the absence it is actually - interested in. Zero means the read cannot reflect the write it is verifying. +- **Refusing a partial list pre-write is itself a fail-open.** The mark gates the post-write check + entirely, so demanding a COMPLETE walk for it would route a page-2 hiccup, an over-cap history or one + malformed id on a later page into "skip the check" — a wider fail-open than the bug being fixed. A + mark over fewer rows can only be lower than the true maximum, which makes the check more eager, never + blinder. That is the trade the pre-paging code made on every run, a single page being all it read. -The test double did not model the job's own POST appearing in the history at all, so in its world every -ordinary run looked like a head nothing had ever been posted to. Same class as the `[]`-vs-`null` -fidelity gap above: a double's claim to mirror reality is an assertion, and it decays. The -`own-write-invisible` stub mode withholds exactly that one detail and is the negative control that -keeps the currency witness from being unproven code. +**A "currency witness" was drafted and WITHDRAWN.** It asserted that the post-write read must show at +least one row above the mark, on the grounds that a completed walk is not necessarily a current one. +Two independent defects came out of that single mechanism, which is the signal to remove it rather than +patch it 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 the rejection — the exact +fail-toward-SUCCESS it was added to prevent; and a schema-valid stale read is not retried, so one such +response converted a transient anomaly into a permanent sentinel. The hazard also has no mechanism on +this deployment: Gitea here is a single instance with no read replicas. Removing it restores the +pre-change exposure on that path, a non-regression, while the paging closes what #763 is about. + +Two test-double corrections were needed, and both are the same class as the `[]`-vs-`null` gap above — +a double's claim to mirror reality is an assertion, and it decays. The stub did not model the job's own +POST appearing in the history, so every ordinary run looked like a head nothing had been posted to; and +it served one flat list, so "terminate only on an EMPTY page" was unobservable, since against a faithful +double a short page is always the last. `verdict-after-short-page` serves a deliberately UNFAITHFUL +short-then-more sequence — the truncated response the rule actually guards against. diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index d7e84d439..dfe443821 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -1013,10 +1013,10 @@ if "/timeline" in url: if "/statuses/" in url: mode = os.environ.get("STUB_HISTORY_MODE", "none") - # PAGES FAITHFULLY (ersatztv#763). The job now walks this endpoint to a validated empty page - # instead of reading one clamped page, so a stub that describes page 1 only is no longer a - # description of anything the job does. Two properties have to hold together, and the second is - # the one that is easy to lose: + # PAGES FAITHFULLY (ersatztv#763). The job walks this endpoint to a validated empty page instead + # of reading one clamped page, so a stub describing page 1 only no longer describes anything the + # job does. The snapshot is a LIST OF PAGES, not a flat list, because two modes below need a page + # boundary the uniform 50-row slicing cannot express. # # 1. PAGE SIZE AND TERMINATOR match the real endpoint. Measured at 1.27.1 on 2026-08-28 against # PR #761's 114-row head: pages 1 and 2 return 50, page 3 returns 14, page 4 is `[]`. So the @@ -1024,18 +1024,20 @@ if "/statuses/" in url: # `/issues/{n}/timeline` returns and what `/commits/{sha}/status` spells `{"statuses": null}`. # Three distinct empty shapes on one server; each stub owes its own measurement. # - # 2. ONE LOGICAL READ IS ONE SNAPSHOT. The counter modes below resolve their rows from how many - # times the job has LOOKED, and the job now looks two or three times per logical read. If - # page 2 recomputed from an advanced counter it would describe a DIFFERENT history than page - # 1 — a state the real server can never be in — and `human-after-post` would surface its - # verdict halfway through the pre-write read, inverting the very race being modelled. So the - # full list is resolved once, on page 1, and cached; later pages slice that cache. + # 2. ONE LOGICAL READ IS ONE SNAPSHOT. The counter modes resolve their rows from how many times + # the job has LOOKED, and the job now looks two or three times per logical read. Recomputing + # on page 2 from an advanced counter would describe a different history than page 1 and + # surface `human-after-post`'s verdict halfway through the PRE-write read, inverting the race + # being modelled. So the pages are resolved once, on page 1, and cached. # - # THE PAGE GUARD SITS AHEAD OF THE COUNTERS, and unlike before this is now load-bearing. The - # previous comment here recorded, honestly, that moving the guard after the counters reddened - # NOTHING, because no counting mode ever issued a page-2 request. That is no longer true: every - # mode issues at least one terminator request per read, so the guard is what keeps the counter - # counting logical reads rather than HTTP round-trips. + # This is a MODELLING CHOICE, not a fidelity claim, and the difference matters. These are + # independent offset-paginated GETs with no snapshot token, so the REAL history can change + # between page requests — overlapping rows and shifting offsets are all reachable live. The + # cache deliberately suppresses that, because the tests here are about the job's paging + # logic, not about mid-walk mutation. Mid-walk mutation is untested, and saying so is the + # honest form of the claim. + # + # Counter branches are confined to the page-1 arm below, so a later page can never advance them. HIST_PAGE_SIZE = 50 hist_page = 1 for part in url.split("?", 1)[-1].split("&"): @@ -1044,17 +1046,20 @@ if "/statuses/" in url: hist_page = int(part.split("=", 1)[1]) except ValueError: hist_page = 1 - if hist_page > 1 and mode == "second-page-garbage": - print("502 Bad Gateway") - sys.exit(0) - if hist_page > 1 and mode == "second-page-error": - sys.exit(22) + + ordinary = [{"id": 7000 + i, "context": "ci/other", "status": "success", + "creator": None, "description": "unrelated"} for i in range(60)] + raced_row = {"id": 9000, "context": "review-verdict/h10", "status": "failure", + "creator": {"login": "timothy"}, + "description": "Review-verdict: BLOCKED @ a9e3e23 (base: main)"} snapshot = out / "history_snapshot.json" - if hist_page > 1: - rows = json.loads(snapshot.read_text()) if snapshot.exists() else [] - else: + logical = out / "history_logical_reads.txt" + if hist_page == 1: + n_logical = (int(logical.read_text()) if logical.exists() else 0) + 1 + logical.write_text(str(n_logical)) rows = [] + pages = None if mode.startswith("human-after-post"): # The raced verdict: absent when the high-water mark is taken, present afterwards. Its id # is ABOVE the mark, which is what makes it detectable. @@ -1066,9 +1071,8 @@ if "/statuses/" in url: # INHERITANCE test: a verdict from an account off `$H10_REVIEWERS` still counts as # "something human landed while we were writing", because narrowing it here would # leave the exemption green over that row instead of repairing to pending. - rows = [{"id": 5000, "context": "review-verdict/h10", "status": "failure", - "creator": {"login": os.environ.get("STUB_HISTORY_CREATOR", "timothy")}, - "description": "Review-verdict: BLOCKED @ a9e3e23 (base: main)"}] + rows = [dict(raced_row, id=5000, + creator={"login": os.environ.get("STUB_HISTORY_CREATOR", "timothy")})] elif mode == "sentinel-after-post": # Another overlapping run repaired mid-flight: its SENTINEL lands above this run's mark, # while the human row it records sits BELOW the mark and is therefore invisible here. @@ -1087,59 +1091,122 @@ if "/statuses/" in url: rows = [{"id": 10, "context": "review-verdict/h10", "status": "success", "creator": {"login": "timothy"}, "description": "Review-verdict: MERGEABLE @ a9e3e23 (base: other)"}] - elif mode in ("second-page", "verdict-on-page-2"): + elif mode in ("second-page", "verdict-on-page-2", "premark-page2-error", + "flaky-page2", "premark-page1-error"): # A history that genuinely runs past one page: 60 ORDINARY rows, no verdict and no # sentinel. Under the pre-#763 page-2 probe the mere existence of these rows forced a # repair; now they are simply read, and `second-page` asserts the exemption STANDS. - rows = [{"id": 7000 + i, "context": "ci/other", "status": "success", - "creator": None, "description": "unrelated"} for i in range(60)] - if mode == "verdict-on-page-2": + rows = list(ordinary) + if mode in ("verdict-on-page-2", "premark-page2-error"): ctr = out / "history_reads.txt" seen = int(ctr.read_text()) if ctr.exists() else 0 ctr.write_text(str(seen + 1)) if seen > 0: - # THE RACED VERDICT, PLACED BEYOND THE FIRST PAGE. Index 55 puts it on page 2, and - # its id is above every ordinary row so it is above the high-water mark too. A job - # that reads only page 1 cannot see it — which is exactly the fail-toward-SUCCESS - # hole #763 closes. - rows.insert(55, {"id": 9000, "context": "review-verdict/h10", "status": "failure", - "creator": {"login": "timothy"}, - "description": "Review-verdict: BLOCKED @ a9e3e23 (base: main)"}) - # THE JOB'S OWN WRITES APPEAR IN THE HISTORY (ersatztv#763). `/statuses/{sha}` returns one row - # per POST, so once this job has posted its exemption the very next read MUST show it. The - # stub did not model that at all: the history was whatever the mode described, before and - # after the write alike, so every ordinary run looked like a head to which nothing had ever - # been posted. - # - # That gap was invisible while the post-write check only ever asked "is there a HUMAN row - # above the mark". It stopped being invisible the moment the check also asserted that the - # read reflects the write it is verifying — the currency witness went to zero on every clean - # run, because in the stub's world the POST really had left no trace. The double was wrong, - # not the check. - # - # `creator: null` is measured: an Actions-token POST records no creator, which is also what - # keeps our own row out of the raced count. The id is one above everything present, mirroring - # the real endpoint's monotonic per-head ids and guaranteeing our row sits above the - # high-water mark taken before the write. + # THE RACED VERDICT, PLACED BEYOND THE FIRST PAGE. Its id is above every ordinary + # row, so it is above the high-water mark too. A job that reads only page 1 cannot + # see it — the fail-toward-SUCCESS hole #763 closes. + rows.insert(55, dict(raced_row)) + elif mode == "string-id-inflates-mark": + # A STRING ID. jq orders strings above every number, so `max` over raw ids returns + # `"99999"` — which then passes the numeric gate as a plain `99999` and sets a high-water + # mark far above anything real. Every later row looks OLDER than the mark, so the raced + # verdict below is invisible and the exemption stands over it. One corrupt row is enough. + ctr = out / "history_reads.txt" + seen = int(ctr.read_text()) if ctr.exists() else 0 + ctr.write_text(str(seen + 1)) + rows = [{"id": "99999", "context": "ci/other", "status": "success", + "creator": None, "description": "unrelated"}, + {"id": 10, "context": "ci/other", "status": "success", + "creator": None, "description": "unrelated"}] + if seen > 0: + rows.append(dict(raced_row)) + elif mode == "malformed-creator-beside-verdict": + # A SCHEMA-CORRUPT ROW NEXT TO A REAL ONE. `creator` is a number, so `.creator.login` + # hard-errors in jq ("Cannot index number with string"), jq exits 5, and an unguarded + # `raced=$(...)` takes the whole step down under `set -e` — after the exemption `success` + # is posted and with the repair never attempted. The genuine verdict beside it is what + # makes the consequence visible: the correct behaviour is to drop the malformed row and + # still repair. + ctr = out / "history_reads.txt" + seen = int(ctr.read_text()) if ctr.exists() else 0 + ctr.write_text(str(seen + 1)) + if seen > 0: + rows = [{"id": 8500, "context": "review-verdict/h10", "status": "failure", + "creator": 7, "description": "Review-verdict: BLOCKED @ a9e3e23 (base: main)"}, + dict(raced_row)] + elif mode == "verdict-after-short-page": + # DELIBERATELY UNFAITHFUL, and that is the point. Page 2 is SHORT (10 rows) and yet page 3 + # still carries rows — a shape the measured server does not produce, but exactly what a + # truncated or partially-served response looks like. It is the only way to observe the + # rule "terminate ONLY on a validated EMPTY page, never on a short one": against a + # faithful double a short page is always the last one, so an implementation that stops + # there is indistinguishable from a correct one. + ctr = out / "history_reads.txt" + seen = int(ctr.read_text()) if ctr.exists() else 0 + ctr.write_text(str(seen + 1)) + pages = [ordinary[:50], ordinary[50:], [dict(raced_row)] if seen > 0 else []] + if pages is None: + pages = [rows[i:i + HIST_PAGE_SIZE] for i in range(0, len(rows), HIST_PAGE_SIZE)] or [[]] + + # THE JOB'S OWN WRITES APPEAR IN THE HISTORY. `/statuses/{sha}` returns one row per POST, so + # once this job has posted its exemption the next read MUST show it. The stub did not model + # that at all — the history was whatever the mode described, before and after the write alike, + # so every ordinary run looked like a head nothing had ever been posted to. `creator: null` is + # measured (an Actions-token POST records no creator), which is also what keeps our own row + # out of the raced count; the id is one above everything present, mirroring the real + # endpoint's per-head monotonic ids (verified live: 114 rows, ids strictly increasing with + # `created_at`, no duplicates). posted_log = out / "posted_all.jsonl" - # `own-write-invisible` withholds exactly this modelling — a history that never shows the - # job's own POST, which is what the stub did for every mode before ersatztv#763. It is the - # negative control for the currency witness: without it that witness is unproven code, since - # every other mode now satisfies it as a side effect of being faithful. - if posted_log.exists() and mode != "own-write-invisible": + if posted_log.exists(): for line in posted_log.read_text().splitlines(): if not line.strip(): continue body = json.loads(line) - rows.append({"id": max([r.get("id") or 0 for r in rows] or [0]) + 1, - "context": body.get("context"), - "status": body.get("state"), - "creator": None, - "description": body.get("description", "")}) - snapshot.write_text(json.dumps(rows)) + # NUMERIC IDS ONLY when computing the next one. A fixture may deliberately carry a + # schema-corrupt id (see `string-id-inflates-mark`), and `max()` over a str beside an + # int raises TypeError — which fails the whole stub request, makes the walk look + # unreadable, and produces a repair for a reason the test was not asking about. That + # is a test passing for the wrong reason: the string-id mutation stayed green because + # the double crashed rather than because the mark was computed correctly. + nxt = max([r.get("id") for pg in pages for r in pg + if isinstance(r.get("id"), int)] or [0]) + 1 + pages[-1].append({"id": nxt, "context": body.get("context"), + "status": body.get("state"), "creator": None, + "description": body.get("description", "")}) + snapshot.write_text(json.dumps(pages)) + else: + n_logical = int(logical.read_text()) if logical.exists() else 1 + pages = json.loads(snapshot.read_text()) if snapshot.exists() else [[]] - start = (hist_page - 1) * HIST_PAGE_SIZE - print(json.dumps(rows[start:start + HIST_PAGE_SIZE])) + if hist_page == 1 and mode == "premark-page1-error": + # PAGE 1 ITSELF FAILS, for the whole first logical read (both attempts), so the walk returns + # nothing at all and the mark must be abandoned. Counted in its own file because the logical + # read counter below is only reached on a SUCCESSFUL page-1 serve. + ctr = out / "p1_attempts.txt" + seen = int(ctr.read_text()) if ctr.exists() else 0 + ctr.write_text(str(seen + 1)) + if seen < 2: + sys.exit(22) + if hist_page > 1 and mode == "flaky-page2": + # TRANSIENT: fails the first attempt of each logical read and succeeds on the retry. This is + # the only fixture that exercises the retry at all — the other error modes fail every attempt, + # so against them a one-shot walk and a retrying walk are indistinguishable. + ctr = out / "page2_attempts.txt" + seen = int(ctr.read_text()) if ctr.exists() else 0 + ctr.write_text(str(seen + 1)) + if seen % 2 == 0: + sys.exit(22) + if hist_page > 1 and mode == "second-page-garbage": + print("502 Bad Gateway") + sys.exit(0) + if hist_page > 1 and mode == "second-page-error": + sys.exit(22) + if hist_page > 1 and mode == "premark-page2-error" and n_logical == 1: + # Fails ONLY on the PRE-write read, so the high-water mark must be salvaged from the partial + # list; the post-write read then pages cleanly and must still catch the raced verdict. + sys.exit(22) + + print(json.dumps(pages[hist_page - 1] if hist_page - 1 < len(pages) else [])) sys.exit(0) if "/status" in url: @@ -2728,6 +2795,10 @@ def test_the_high_water_MARK_is_captured_BEFORE_the_last_moment_re_read(): # definition matters: the definition sits with the other helpers near the top of the step, so # keying on it would place the "fetch" far earlier than the round-trip actually happens and this # assertion would hold vacuously. + assert "\npage_statuses\n" in src, ( + "no bare `page_statuses` call site found; the mark is no longer fetched where this test " + "believes it is, and the ordering assertion below would be vacuous" + ) mark = max(src.index("max_id_before=-1"), src.index("\npage_statuses\n")) # The LAST-MOMENT re-read is the second bare `read_existing_verdict` call. calls = [i for i in range(len(src)) if src.startswith("read_existing_verdict\n", i)] @@ -3607,37 +3678,6 @@ def test_a_status_history_RUNNING_PAST_PAGE_1_is_PAGED_and_the_exemption_STANDS( ) -def test_a_post_write_read_that_CANNOT_SEE_OUR_OWN_WRITE_is_not_trusted(tmp_path): - """The currency witness (ersatztv#763), and the reason paging alone is not enough. - - `st_ok=yes` proves the walk reached a validated empty page. It does NOT prove the walk saw a - list that includes the POST this job just made — a replica lag, a cache, or a read aimed at the - wrong sha all terminate cleanly while showing a pre-write world. A `raced=0` derived from such a - read is a green nobody verified, on the one path whose failure direction is toward SUCCESS. - - So the check asserts a POSITIVE fact rather than an absence: our own row was written after the - high-water mark was taken, so at least one row above that mark must exist. The stub mode here - withholds exactly that one modelling detail and changes nothing else. - """ - posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="own-write-invisible") - assert r.returncode == 0, r.stderr - seq = _posted_sequence(tmp_path) - assert len(seq) == 2, ( - "the post-write history could not even see this job's own write, yet the exemption was left " - f"standing on the strength of it. Posts: {seq}\n{r.stdout[-900:]}" - ) - assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", ( - f"expected an exemption then a repair to pending; got {seq}" - ) - # Reported as uncertainty, NOT as an overwritten verdict — nothing here says a human ruled. - assert "did not reflect this job" in (r.stdout + r.stderr), ( - f"repaired, but not for the stated reason:\n{r.stdout[-900:]}" - ) - assert "was overwritten" not in (r.stdout + r.stderr), ( - f"an uncertainty repair was reported as an overwritten human verdict:\n{r.stdout[-900:]}" - ) - - def test_a_raced_verdict_on_PAGE_2_is_detected_and_repaired(tmp_path): """The hole #763 exists to close, and the half a paging change can still get wrong. @@ -3670,6 +3710,172 @@ def test_a_raced_verdict_on_PAGE_2_is_detected_and_repaired(tmp_path): ) +def test_a_STRING_id_cannot_inflate_the_high_water_mark(tmp_path): + """jq sorts strings above every number, so one schema-corrupt id silently blinds the race check. + + `max` over raw ids returns `"99999"` rather than the largest real id. `jq -r` then renders it as + `99999`, which sails through the `*[!0-9]*` numeric gate, and the mark is set far above anything + that exists. Every subsequent row — including a genuine human rejection racing the write — tests + as OLDER than the mark and is invisible, so the exemption stands over it. + + It fails toward SUCCESS and needs no attacker: one corrupt row is enough. So the mark is taken + over numeric ids only, and a non-numeric id is excluded rather than coerced. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="string-id-inflates-mark") + assert r.returncode == 0, r.stderr + seq = _posted_sequence(tmp_path) + assert len(seq) == 2, ( + "a string id inflated the high-water mark, so the raced verdict tested as older than it and " + f"the exemption stands over a human rejection. Posts: {seq}\n{r.stdout[-900:]}" + ) + assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", ( + f"expected an exemption then a repair to pending; got {seq}" + ) + + +def test_a_TRANSIENT_page_failure_is_retried_and_the_walk_still_completes(tmp_path): + """The retry clause, which was unproven code until this fixture existed. + + Every other error mode here fails on EVERY attempt, so a one-shot walk and a retrying walk behave + identically against them — mutating `for try in 1 2` to `for try in 1` reddened nothing. That + matters because the retry is the stated reason failing closed is affordable: without it, one + momentary blip costs the head its exemption permanently, since the repair sentinel is sticky. + + Here page 2 fails the first attempt of each logical read and succeeds on the retry. The history + carries no verdict, so the correct outcome is a completed walk and a STANDING exemption. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="flaky-page2") + assert r.returncode == 0, r.stderr + seq = _posted_sequence(tmp_path) + assert len(seq) == 1, ( + "a transient page failure was not retried, so the walk gave up and the exemption was repaired " + f"away on a head that nothing raced. Posts: {seq}\n{r.stdout[-900:]}" + ) + assert seq[0]["state"] == "success", f"expected the exemption to stand; got {seq}" + + +def test_a_PRE_WRITE_read_that_returns_NOTHING_abandons_the_mark_and_says_so(tmp_path): + """The boundary of the partial-list fallback, pinned so the remaining fail-open is explicit. + + A partial list still yields a usable mark. A read that returns NO rows at all cannot: there is + nothing to take a maximum over, so `max_id_before` stays -1 and the post-write race check is + skipped entirely — the exemption is posted with nothing verifying it afterwards. + + That is the pre-existing gap tracked as ersatztv#849, deliberately unchanged here, and it is + asserted rather than left implicit so the degradation is visible in the log and a future change + that widens or closes it has to come past this test. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="premark-page1-error") + assert r.returncode == 0, r.stderr + log = r.stdout + r.stderr + assert "Could not establish a status high-water mark" in log, ( + f"the mark was abandoned silently, with no record of the degradation:\n{r.stdout[-900:]}" + ) + assert "taking the high-water mark over the" not in log, ( + f"a mark was salvaged from an empty read, which there is nothing to compute:\n{r.stdout[-900:]}" + ) + seq = _posted_sequence(tmp_path) + assert len(seq) == 1 and seq[0]["state"] == "success", ( + f"expected the exemption to be posted with the race check skipped; got {seq}" + ) + + +def test_a_MALFORMED_creator_row_does_not_kill_the_job_after_the_green_is_posted(tmp_path): + """A row whose `creator` is not an object used to be fatal, at the worst possible moment. + + `.creator != null and .creator.login` hard-errors in jq on any non-object creator; jq exits 5 and + under `set -euo pipefail` the assignment takes the step down. That happens AFTER the exemption + `success` has been posted and BEFORE the repair is attempted, so one schema-corrupt row leaves a + green standing on a head that carries a genuine human rejection — and the job reports failure in a + way that looks like an unrelated infrastructure error. + + The fix type-tests `creator` before indexing it, so the malformed row is dropped from the count + while the real verdict beside it is still counted and still repaired. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), + history_mode="malformed-creator-beside-verdict") + assert r.returncode == 0, ( + f"the step died instead of skipping a malformed row: {r.stderr[-1200:]}" + ) + seq = _posted_sequence(tmp_path) + assert len(seq) == 2, ( + "a malformed `creator` row suppressed the repair, leaving the exemption green over the real " + f"verdict beside it. Posts: {seq}\n{r.stdout[-900:]}" + ) + assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", ( + f"expected an exemption then a repair to pending; got {seq}" + ) + # THE REASON, not just the outcome — this is what makes the type guard individually provable. + # Three clauses can each rescue this fixture (the type test, the `|| raced=""` guard, and the + # fail-closed unusable-count branch), so the repair alone cannot tell them apart. Only the type + # test lets the REAL verdict beside the malformed row be counted as a genuine human race; without + # it the count comes back empty and the repair is reported as an unusable count instead. + assert "was overwritten" in (r.stdout + r.stderr), ( + "the malformed row suppressed the real verdict beside it — repaired, but as an unverifiable " + f"read rather than as the human rejection it is:\n{r.stdout[-900:]}" + ) + + +def test_a_verdict_BEYOND_A_SHORT_PAGE_is_still_found(tmp_path): + """"Terminate only on a validated EMPTY page, never on a short one" — the rule that is invisible + against a faithful double. + + On the real endpoint a short page IS the last page (measured: 50, 50, 14, `[]`), so an + implementation that stops at the first short page is indistinguishable from a correct one, and + every other test here would pass against it. The only way to observe the property is to model what + it actually guards: a TRUNCATED response. This stub serves 50 rows, then a short page of 10, then + a third page carrying the raced verdict. + + A walk that treats the short page as exhaustion never reads page 3, misses the verdict, and leaves + the exemption green over a human rejection. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="verdict-after-short-page") + assert r.returncode == 0, r.stderr + seq = _posted_sequence(tmp_path) + assert len(seq) == 2, ( + "a raced verdict beyond a SHORT page was not found, so the walk stopped on a short page " + f"instead of on a validated empty one. Posts: {seq}\n{r.stdout[-900:]}" + ) + assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", ( + f"expected an exemption then a repair to pending; got {seq}" + ) + assert "was overwritten" in (r.stdout + r.stderr), ( + f"repaired, but not reported as a found verdict:\n{r.stdout[-900:]}" + ) + + +def test_a_PRE_WRITE_paging_failure_still_yields_a_usable_high_water_mark(tmp_path): + """The fail-open a paging change can introduce while fixing one (round-2 Blocker). + + The high-water mark gates the post-write race check entirely: `max_id_before=-1` skips it, so a + human rejection landing in the write window is neither detected nor repaired. Before paging, only + a failure of the single page-1 request could reach that. Requiring a COMPLETE walk for the mark + would newly route a page-2 hiccup, an over-cap history, or one malformed id on a later page into + the same hole — a WIDER fail-open than the bug being fixed. + + So a partial list still yields a mark. It can only be LOWER than the true maximum, which makes the + check more eager, never blinder. + + Here page 2 fails on the PRE-write read only; the post-write read pages cleanly and carries a raced + verdict. With the mark salvaged from page 1 the verdict is above it and the exemption is repaired. + With `max_id_before=-1` the check never runs and the rejection stays green. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="premark-page2-error") + assert r.returncode == 0, r.stderr + log = r.stdout + r.stderr + assert "taking the high-water mark over the" in log, ( + f"the partial-list fallback did not run, so the mark was not salvaged:\n{r.stdout[-900:]}" + ) + seq = _posted_sequence(tmp_path) + assert len(seq) == 2, ( + "the pre-write read could not be paged completely, the mark was abandoned, and the post-write " + f"race check was skipped — leaving a raced rejection green. Posts: {seq}\n{r.stdout[-900:]}" + ) + assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", ( + f"expected an exemption then a repair to pending; got {seq}" + ) + + @pytest.mark.parametrize( "mode,why", [