diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index 00fa3f2d8..8ba1c3675 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -723,6 +723,115 @@ jobs: done return 0 } + # PAGE THE PER-POST STATUS HISTORY TO A VALIDATED TERMINATOR (ersatztv#763). + # + # Both callers below want the WHOLE history, and a single `?limit=100` read cannot give it: + # `limit` clamps to the server-wide `MAX_RESPONSE_ITEMS`, measured at 50. + # + # BE PRECISE ABOUT WHAT THAT COST, because the issue and an earlier version of this comment + # both overstated it. Under the server default (`created_unix DESC`) page 1 holds the NEWEST + # rows, and ids are monotonic with `created_at` (measured). So page 1 already carried the + # true maximum id, AND every row newer than the mark — which is the only thing the + # post-write check selects on. A single-page read therefore missed a raced verdict only if + # more than 50 rows were created INSIDE the write window, not merely "on a head with more + # than 50 rows". What actually removed #761's stall is deleting #751's page-2 "assume raced" + # probe, further down; this walk is not what fixed it. + # + # THE WALK IS STILL WORTH HAVING, for a different reason than the issue gives: it removes + # the gate's one fail-toward-SUCCESS path from depending on an UNDOCUMENTED ordering that + # the server honours only coarsely. Page 1 came back `114,112,113,111,110` — newest-first, + # but not strictly, and nothing in the API contract promises even that. Selecting by id over + # a list known to be complete does not care how the server ordered it. + # + # CORRECTNESS DOES NOT DEPEND ON THE NUMBER 50. The walk stops at a validated EMPTY page, so + # whatever page size the server actually applies is paged correctly; `limit=50` only asks + # for what this instance will really give. The distinction matters because #751 shipped a + # completeness check written against a hardcoded `limit=100` that was DEAD CODE precisely + # because the cap is 50. A guard against a hardcoded cap is the thing to avoid; a request + # that names one, whose correctness does not turn on it, is not the same mistake. + # + # THE TERMINATOR IS MEASURED PER ENDPOINT — this server is not consistent between them and + # guessing has been wrong twice. Measured at 1.27.1 on 2026-08-28 against PR #761's head + # (114 rows): pages 1 and 2 return 50, page 3 returns 14, page 4 is the two bytes `[]`. + # `/issues/{n}/timeline` returns bare `null` past the end and `/commits/{sha}/status` + # returns `{"statuses": null}` — three distinct empty shapes on one instance, which is why + # `count_retargets` above and this walk deliberately do NOT share a terminator. + # + # A SHORT PAGE IS NOT THE END. Page 3 above carried 14 rows and was still followed by a real + # page 4, and more fundamentally a short page is indistinguishable from a truncated + # response. Only an empty page is evidence, which costs exactly one extra round-trip. + # + # 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 + # `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. `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() { + 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="" + 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 + # separate "the body is null" from "the predicate is false". + kind=$(printf '%s' "$raw" | jq -r 'type' 2>/dev/null) || kind="" + fi + if [ "$kind" = array ] || [ "$kind" = null ]; 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. + # Only BETWEEN attempts — sleeping after the last one delays a failure nobody is + # waiting on, once per walk and so twice per job run. + if [ "$try" -eq 1 ]; then sleep 1; fi + done + # `null` COUNTS AS AN EMPTY PAGE, though this endpoint returns `[]` today. An + # array-only gate is the exact shape of #751: `count_retargets` had one, the timeline + # really did return `null` past the end, so the walk never reached a validated empty + # page and the fence withheld EVERY exemption. Re-adopting that narrowing on a second + # endpoint would be worse, because this walk's failure is the STICKY sentinel — every + # exempt PR would need a human verdict, per head. It cannot misread `[]` either. + # + # It is NOT free, though, and the cost lands on one caller: an empty page 1 accepted as + # "complete, zero rows" is correct BEFORE the write and impossible AFTER it. The + # post-write caller therefore rejects an empty result itself, below — this walk stays + # caller-agnostic because the pre-write caller genuinely needs the empty answer. + case "$kind" in + null) ph_ok=yes; ph_rows=$acc; return 0 ;; + array) ;; + *) ph_rows=$acc; return 0 ;; + esac + n=$(printf '%s' "$raw" | jq -r 'length' 2>/dev/null) || n="" + 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 `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 + } ex_repair=no count_retargets @@ -1005,9 +1114,45 @@ jobs: # `.[].id` hard-errors under `set -e` if the array ever holds a non-object, which would kill # the job before it posts and strand an ordinary PR with no status at all. max_id_before=-1 - hist_before=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA?limit=100") || hist_before="" - if [ -n "${hist_before//[[:space:]]/}" ] && printf '%s' "$hist_before" | jq -e 'type == "array"' >/dev/null 2>&1; then - mark=$(printf '%s' "$hist_before" | jq -r '[.[] | .id? // 0] | max // 0' 2>/dev/null || true) + # PAGED (ersatztv#763). This was one `?limit=100` read, so on a head carrying more rows than + # the clamp the "maximum" was only the maximum of page 1. That direction is safe — a mark + # that is too low makes pre-existing rows look newer and repairs an exemption that nobody + # 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 + # A PARTIAL LIST STILL YIELDS A USABLE MARK, and refusing one is a FAIL-OPEN (ersatztv#763). + # This depends on the DESC default above: the newest row — the one carrying the maximum id, + # ids being monotonic with `created_at` (measured) — is on page 1, so a walk that fails + # later still saw it. Under an ASCENDING sort this reasoning inverts and the fallback + # becomes a spurious-repair engine; see the withdrawal note at `page_statuses`. + # 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. + # AN EMPTY HISTORY AND AN UNUSABLE ONE ARE DIFFERENT ANSWERS. A head nothing has posted + # to legitimately has no rows, and its mark is 0 — every later row is newer, which is + # correct. But a NON-empty history in which no row carries a numeric id is a schema we + # cannot read, and collapsing that to 0 makes every pre-existing row look newer than the + # mark and repairs the exemption away. So the two are reported separately and only the + # first yields a number; `none` falls through the numeric gate below and skips the check. + mark=$(printf '%s' "$ph_rows" | jq -r 'if length == 0 then 0 else ([.[] | select(type == "object") | .id | numbers] | max // "none") end' 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 @@ -1142,21 +1287,31 @@ 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 *WITHIN* A PAGE — 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. But this read is ONE PAGE, and `limit=100` clamps - # to the server-wide `MAX_RESPONSE_ITEMS`, measured at 50 on this instance (ersatztv#751). - # An earlier version of this comment claimed order-independence flatly; that is false the - # moment a head carries more rows than the clamp, because a raced row can then sit on a page - # this never reads. Measured: a probe head reached 33 rows after ~5 runs, and the ordering is - # only coarsely newest-first (ids came back `33,32,31,30,28,29,27,…`), so >50 is ordinarily - # reachable on a PR with a few CI reruns. + # 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 residual is ACCEPTED, and it is + # bounded: a row arriving after this job's POST is not one this job overwrote, and being + # newest it wins on the combined endpoint that branch protection actually reads. + # + # `sort=highestindex` WAS TRIED AND WITHDRAWN. It returns `index` ASCENDING (measured), which + # does close the mid-walk-insert gap — an insert appends at the end, onto a page not yet + # fetched. But ASC puts the OLDEST rows on page 1, and that inverts the safety of the + # partial-mark fallback below: an incomplete walk would then take its maximum over the + # oldest rows, leaving every pre-existing row above the mark and read as "raced" — the + # spurious STICKY repair this change exists to stop. Under the server default the newest row + # is on page 1 by construction, so a partial mark is at or very near the true maximum. + # Closing a bounded gap by re-opening the one that caused #761 is the wrong trade. # # 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 - # standing over a rejection. So the page-2 probe below treats "there are rows I did not read" - # as "assume raced" — the conservative direction — rather than as "no race found". - # Full paging of this endpoint, including for the high-water mark above, is ersatztv#763. + # standing over a rejection. Everything below therefore resolves uncertainty to `pending`. # # The repair is `pending`, NEVER a copy of the human's state. Re-posting their `failure` # would attribute a human verdict to this job — the exact provenance confusion the @@ -1167,11 +1322,57 @@ jobs: # # A read failure here does NOT fail the job: the status is already posted, so `exit 1` would # change nothing about the gate's state while turning a routine API hiccup into a red run. - # It is reported loudly and left alone — the residual is the read/POST gap either way. + # It is reported loudly and REPAIRED (ersatztv#763) — the old treatment left it alone, which + # is the wrong direction on this path; see below. if [ "$state" = "success" ] && [ "$max_id_before" -ge 0 ]; then - post_hist=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA?limit=100") || post_hist="" - if [ -z "${post_hist//[[:space:]]/}" ] || ! printf '%s' "$post_hist" | jq -e 'type == "array"' >/dev/null 2>&1; then - echo "::warning::Could not re-read the status history for ${SHA:0:7} after posting, so a human verdict landing during the write window would not be detected. The exemption ${CONTEXT}=success stands." + page_statuses + # AN EMPTY HISTORY IS IMPOSSIBLE HERE, so it is not evidence. This job has just POSTed to + # this sha, and `/statuses/{sha}` returns one row per POST, so a well-formed response + # saying "no statuses exist" contradicts a write that succeeded. Accepting it yields + # `raced=0` — a green concluded from a list that cannot be real — on the one path whose + # failure direction is toward SUCCESS, and silently, since the walk itself reports + # success. The predecessor was not better in the way it first appears: its + # `jq -e 'type == "array"'` gate ACCEPTED `[]` and, of the two empty shapes, warned only + # on `null` (an empty body and a non-array object warned too), so the `[]` half + # of this fail-open was already unlogged on `main`. What is new is that the paged walk + # additionally reports such a read as a SUCCESS, which is why the rejection lives here. + # + # THIS IS NOT THE WITHDRAWN CURRENCY WITNESS, and the difference is what makes it safe. + # That mechanism asked "is there ANY row above the mark", which an unrelated newer row + # satisfied while the rejection stayed hidden, and which fired on schema-valid staleness. + # This asks only whether the list is EMPTY — a state no unrelated row can produce and no + # ordering can disguise. It carries neither defect. + ph_len_after=$(printf '%s' "$ph_rows" | jq -r 'if type == "array" then length else 0 end' 2>/dev/null) || ph_len_after=0 + case "$ph_len_after" in ''|*[!0-9]*) ph_len_after=0 ;; esac + # WHY the repair fired, so the log can stop asserting something it does not know. The + # 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. + # + # THE EMPTY CASE GETS ITS OWN REASON rather than borrowing the unreadable one. Routing it + # through "could not be read completely" would print that beside a walk that DID complete, + # on a validated terminator — the answer was impossible, not unreadable, and an operator + # reading a sticky sentinel needs to know which. + raced_why=human + if [ "$ph_ok" = yes ] && [ "$ph_len_after" -eq 0 ]; then + echo "::warning::The status history for ${SHA:0:7} came back empty after this job posted to it, which cannot be true, so a raced verdict could not be ruled out. Repairing ${CONTEXT} to pending." + raced_why="the status history came back empty after this job posted to it" + raced=1 + elif [ "$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 (since deleted) repaired on + # exactly the same uncertainty — the two halves of one check disagreed about which way + # "I could not read it" resolves. On the one path whose failure direction is toward + # SUCCESS it has to be `pending`: an unreadable history is not evidence that nothing + # raced the write. + # + # This is affordable ONLY because the walk above is real paging. Under the old probe the + # common trigger was an ordinary head carrying more than 50 rows, so failing closed then + # would have cost the exemption on every busy PR. Now "could not establish" means a + # genuine API failure that survived a retry, or a history past the page cap (950 rows, + # since the twentieth request must be the empty terminator). + echo "::warning::Could not establish a complete status history for ${SHA:0:7} after posting, so a human verdict landing during the write window could not be ruled out. Repairing ${CONTEXT} to pending rather than leaving an exemption green on an unverified head." + raced_why="the status history could not be read completely" + raced=1 else # `.id > $since` is what confines this to the write window. Our OWN row is excluded twice # over — it carries `creator: null` (an Actions-token POST, measured; see the provenance @@ -1215,75 +1416,91 @@ 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' "$post_hist" | 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 (ersatztv#763). `(.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(((.id | numbers) // 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') - # "NO RACE FOUND ON PAGE 1" IS NOT "NO RACE" (ersatztv#751). If page 1 was not the whole - # list, a raced row can be beyond it, so an unread page is treated as a race rather than - # as absence. Only checked when page 1 looked clean — a race already found needs no - # further evidence. Unreadable page 2 also counts as raced: this is the fail-toward- - # SUCCESS path, so uncertainty must resolve to `pending`, never to leaving green. + )] | length') || raced="" + # NO "CURRENCY WITNESS" HERE, AND THAT IS A DELIBERATE WITHDRAWAL (ersatztv#763). + # 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: # - # BE HONEST ABOUT THE COST: a repair here is STICKY, not a one-run stall. It writes - # `$REPAIR_DESC`, and the classification above refuses to grant an exemption over that - # sentinel and re-writes it as a fixed point on every later run — so a SPURIOUS repair - # (a head with more status rows than the page cap and no actual race) removes that head's - # exemption permanently, and only a human `post-review-verdict.sh` clears it. That is - # still the right direction, because the alternative is a forged green over a human - # rejection. But it is a per-sha loss of the exemption, not an inconvenience, which is - # the argument for replacing this with real paging (#763) rather than living with it. - if [ "$raced" = "0" ]; then - more_hist=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA?limit=100&page=2") || more_hist="UNREADABLE" - if [ "$more_hist" = "UNREADABLE" ]; then - echo "::warning::Could not read page 2 of the status history for ${SHA:0:7}; treating this as a possible raced verdict and repairing to pending rather than leaving an exemption green on an unverified head." - raced=1 - else - mh_kind=$(printf '%s' "$more_hist" | jq -r 'type' 2>/dev/null) || mh_kind="" - mh_len=$(printf '%s' "$more_hist" | jq -r 'if type == "array" then length else 0 end' 2>/dev/null) || mh_len="" - case "${mh_kind}:${mh_len}" in - null:*|array:0) ;; - array:*) - echo "::warning::The status history for ${SHA:0:7} runs past page 1 (${mh_len} more row(s)), so a raced human verdict could be on a page this job did not read. Repairing to pending rather than leaving the exemption green." - raced=1 ;; - *) - echo "::warning::Page 2 of the status history for ${SHA:0:7} was '${mh_kind:-unparseable}'; treating as a possible raced verdict and repairing to pending." - raced=1 ;; - esac + # * 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. + # + # 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]*) + # FAILS CLOSED (ersatztv#763). "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 - 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. - 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} " - 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 - ;; - esac + echo "Repaired ${CONTEXT} to pending on ${SHA:0:7}." + state=pending fi fi diff --git a/docs/ci-cd.md b/docs/ci-cd.md index f2f0fb235..54c2198da 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1574,6 +1574,86 @@ never be silently turned green" is too strong and is retracted: a generic `pendi verified, and a rejection it masks is re-derived green by a later run (ersatztv#849). The repair is `pending`, never a copy of the human's verdict, which would attribute a human decision to the job. +**Both `/statuses/{sha}` reads are PAGED** (ersatztv#763). The history is read twice — before the write +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 returned a partial list. + +**Be precise about what that cost**, because ersatztv#763's own framing overstated it. Under the server +default (`created_unix DESC`) page 1 holds the *newest* rows and ids are monotonic with `created_at`, +so page 1 already carried both the true maximum id and every row newer than the mark — the only rows +the post-write check selects on. A single-page read therefore missed a raced verdict only if **more +than 50 rows were created inside the write window**, not merely on "a head with more than 50 rows". +What actually removed PR #761's stall is the deletion of the page-2 probe below, not the paging. + +The walk earns its place for a different reason: it removes the gate's one fail-toward-SUCCESS path +from depending on an **undocumented ordering** the server honours only coarsely (page 1 came back +`114,112,113,111,110` — newest-first, but not strictly, and nothing in the API contract promises even +that). Both reads walk to a **validated empty page**, never terminating on a short one, retrying each +page once. The termination rule is page-size independent; the *capacity* is not — 20 requests of 50 +validate at most 950 rows, since the twentieth 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`. + +**The page-2 probe is gone.** ersatztv#751 treated "there are rows I did not read" as "assume raced". +That 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. + +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. +- **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. This rests on the DESC default above: the + newest row, carrying the maximum id, is on page 1, so a walk that fails later still saw it. A + *validated* empty history is a different case and is not abandoned: it yields a mark of 0, which is + correct, since every later row is newer — that is the normal first run. What abandons the mark is a + read that both **failed** and returned nothing, the pre-existing gap tracked as ersatztv#849 and + unchanged here. A non-empty history carrying no numeric id is reported as unusable rather than + collapsed to 0. + +Two id comparisons are **numeric-only**, closing the *string* half of a fail-open (a type guard, not +a value bound — a corrupt but genuinely numeric id would still inflate the mark, which is not +attacker-controllable since ids are server-assigned): jq orders strings above every number, so one +`"id": "99999"` would inflate the high-water mark until nothing looked newer, and +`.id > $since` would read any string id as newer than any mark — making a pre-existing base-mismatched +verdict look raced on every run, a permanent stall. `.creator` is likewise 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. + +**Rejected alternatives**, recorded so they are not re-adopted: + +- **A currency witness** (require a row above the mark, proving the read reflects the POST just made). + Counting *any* row above the mark does not witness this job's write, so a stale-but-valid snapshot + carrying an unrelated newer row still passes; and a schema-valid stale read is not retried, so one + such response turns a transient anomaly into a permanent sentinel. Gitea here is a single instance + with no read replicas, so the hazard has no mechanism behind it. +- **`sort=highestindex`** (index ASC, measured), which would close the mid-walk-insert gap below. ASC + puts the *oldest* rows on page 1, which inverts the partial-mark fallback: the incomplete walk's + maximum becomes the max of the oldest rows, leaving every pre-existing row above the mark and read as + raced. That is the #761 failure, re-introduced to close a smaller one. A test asserts the request + carries no sort parameter. + +**Accepted residual: a row inserted mid-walk can be missed.** These are independent offset-paginated +GETs with no snapshot token, so under DESC a new row lands at position 0 — on a page already read — +while everything else shifts down. It is bounded: a row arriving after this job's POST is not one the +job overwrote, and being newest it wins on the combined endpoint branch protection reads. + +**An empty post-write history is rejected.** The walk terminates on an empty page — correct before the +write, impossible after it, since this job has just POSTed and the endpoint returns one row per POST. A +well-formed "no statuses exist" is not retried (the body is valid), so accepting it would conclude +`raced=0` from a list that cannot be real, silently. This is *not* the withdrawn currency witness: that +asked whether ANY row sat above the mark, which an unrelated newer row satisfied; this asks only whether +the list is empty, which no unrelated row can produce. + +The `::error::` names its own cause, and there are three: a verdict actually **found**; a read that +could not be **completed**; and a read that completed but returned an **impossible** answer. The third +is not a variety of the second — an operator holding a sticky sentinel needs to know whether to go +looking for an API failure that never happened. The sentinel *description* stays generic, because the +classification recognises it as a fixed point. + 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 f0b42ce53..13e3f327e 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}` 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. | 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. WHAT THE PAGING BUYS IS NOT WHAT #763 CLAIMED: under the DESC default page 1 already held the true maximum id AND every row newer than the mark, so a single-page read missed a raced verdict only if more than 50 rows were created INSIDE the write window — not merely on a head over 50 rows. What removed #761's stall is retiring the probe, not the walk; the walk's value is that the gate's one fail-toward-SUCCESS path no longer depends on an UNDOCUMENTED ordering the server honours only coarsely (page 1 came back `114,112,113,111,110`). 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; this rests on the DESC DEFAULT — the newest row, carrying the maximum id, is on page 1, so a walk that fails later still saw it — while a VALIDATED empty history is NOT abandoned (it yields a mark of 0, correct for a first run, since every later row is newer) — what abandons the mark is a read that both FAILED and returned nothing (the pre-existing #849 gap, unchanged) — and a NON-EMPTY history carrying no numeric id is reported unusable rather than collapsed to 0. BOTH id comparisons are NUMERIC-ONLY: jq orders strings above every number, so one `"id": "99999"` inflates the mark until nothing looks newer, and `.id > $since` reads any string id as newer than any mark — making a PRE-EXISTING base-mismatched verdict look raced on every run, a permanent per-sha stall (the twin was live on `main`). An EMPTY post-write history is REJECTED: the walk terminates on an empty page, which is correct before the write and impossible after it (one row per POST), and a well-formed "no statuses exist" is not retried — so accepting it would conclude `raced=0` from a list that cannot be real, silently. That is NOT the withdrawn currency witness, which asked whether ANY row sat above the mark and was satisfied by an unrelated newer row; this asks only whether the list is EMPTY, a state no unrelated row can produce. The `::error::` now names its own cause, of which there are THREE — a verdict actually FOUND, a read that could not be COMPLETED, and a read that completed but returned an IMPOSSIBLE answer (the third is not a variety of the second) — 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 71f67e12b..5475c543a 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}` 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.' +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. WHAT THE PAGING BUYS IS NOT WHAT #763 CLAIMED: under the DESC default page 1 already held the true maximum id AND every row newer than the mark, so a single-page read missed a raced verdict only if more than 50 rows were created INSIDE the write window — not merely on a head over 50 rows. What removed #761''s stall is retiring the probe, not the walk; the walk''s value is that the gate''s one fail-toward-SUCCESS path no longer depends on an UNDOCUMENTED ordering the server honours only coarsely (page 1 came back `114,112,113,111,110`). 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; this rests on the DESC DEFAULT — the newest row, carrying the maximum id, is on page 1, so a walk that fails later still saw it — while a VALIDATED empty history is NOT abandoned (it yields a mark of 0, correct for a first run, since every later row is newer) — what abandons the mark is a read that both FAILED and returned nothing (the pre-existing #849 gap, unchanged) — and a NON-EMPTY history carrying no numeric id is reported unusable rather than collapsed to 0. BOTH id comparisons are NUMERIC-ONLY: jq orders strings above every number, so one `"id": "99999"` inflates the mark until nothing looks newer, and `.id > $since` reads any string id as newer than any mark — making a PRE-EXISTING base-mismatched verdict look raced on every run, a permanent per-sha stall (the twin was live on `main`). An EMPTY post-write history is REJECTED: the walk terminates on an empty page, which is correct before the write and impossible after it (one row per POST), and a well-formed "no statuses exist" is not retried — so accepting it would conclude `raced=0` from a list that cannot be real, silently. That is NOT the withdrawn currency witness, which asked whether ANY row sat above the mark and was satisfied by an unrelated newer row; this asks only whether the list is EMPTY, a state no unrelated row can produce. The `::error::` now names its own cause, of which there are THREE — a verdict actually FOUND, a read that could not be COMPLETED, and a read that completed but returned an IMPOSSIBLE answer (the third is not a variety of the second) — 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; `max_id_before` from `GET /repos/{repo}/statuses/{sha}` (a BARE ARRAY, unlike the combined `/commits/{sha}/status` object); repair POST is `pending`; tests `test_a_RETARGET_DURING_the_run_posts_NOTHING`, `test_a_PR_retargeted_BEFORE_the_run_but_QUIET_during_it_is_STILL_exempt`, `test_an_UNTRUSTED_retarget_count_withholds_the_EXEMPTION`, `test_an_UNTRUSTED_retarget_count_STILL_LETS_PENDING_THROUGH`, `test_a_human_verdict_landing_AFTER_the_POST_is_repaired_to_pending`, `test_a_PRE_EXISTING_human_row_does_NOT_trigger_a_repair`' +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 `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; the server default `created_unix DESC` is KEPT, so a row inserted MID-WALK can be missed (it lands at position 0, on a page already read) — accepted and bounded, since a row arriving after this job''s POST is not one the job overwrote and wins on the combined endpoint anyway. `sort=highestindex` (index ASC, measured) closes that gap and was WITHDRAWN because ASC puts the OLDEST rows on page 1, which inverts the partial-mark fallback into a spurious-STICKY-repair engine — the #761 failure, re-introduced to close a smaller gap; `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); this endpoint is 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`, `test_a_STRING_id_on_a_PRE_EXISTING_row_is_not_read_as_raced`, `test_a_partial_mark_is_SAFE_because_the_newest_rows_are_on_page_1`, `test_an_EMPTY_post_write_history_is_not_evidence_that_nothing_raced`, `test_a_NULL_page_terminates_the_walk_as_an_empty_one`, `test_the_walk_does_not_request_a_SORT_order`' --- `ci.exemption-provenance` closed three routes into the exemption path and left one residual it named: @@ -109,3 +109,60 @@ and `[]` terminate the walk. `test_the_fence_TRUSTS_the_count_and_POSTS_when_the is parameterised over both shapes and asserts the POSTED STATUS rather than the log line — on the real probe run the log said `Decision: state=success` and the job still posted nothing, so the decision and the write are separate events and only the write is what a merge reads. + +## The status history is paged too (2026-08-28, ersatztv#763) + +The timeline walk above was one half of a single rule; `/statuses/{sha}` was still read as one +`?limit=100` page at both sites. `limit` clamps to `MAX_RESPONSE_ITEMS` (measured 50), so on a head +carrying more rows than the clamp the high-water mark was only page 1's maximum, and — the direction +that matters — a raced human verdict beyond page 1 was invisible to the post-write check. + +#751 mitigated the fail-open half without paging: when page 1 showed no raced row it read page 2 and +treated any rows there, or an unreadable page 2, as "assume raced". Correct in direction, blunt in +effect, and reached in production. Renovate PR #761 accumulated status rows over ordinary CI re-runs +until the probe repaired its exemption away, logging an `::error::` asserting a human verdict had been +overwritten on a head that carried none; the repair sentinel being a fixed point, that head then +refused re-exemption on every later run, and a title edit sufficed to re-trigger it. + +**Measured before rewriting** (1.27.1, 2026-08-28, PR #761's head, 114 rows): pages 1 and 2 return 50, +page 3 returns 14, page 4 is the two bytes `[]`. The terminator here is an empty ARRAY — a third +distinct empty shape on one instance, after the timeline's bare `null` and the combined endpoint's +`{"statuses": null}`. A short page is **not** the end. An empty **first** page is legitimate rather than +anomalous **for the pre-write caller** (a head nothing has posted to yet returns `[]`, and so does a +bogus sha), which is why this walk cannot simply copy `count_retargets`, whose "one real page required" +rule would withhold the mark on every first run. It is *impossible* for the post-write caller, which +has just written a row — so that caller rejects an empty result itself rather than the walk refusing it +for everyone. + +Two things paging does not buy on its own, both now explicit: + +- **Post-write uncertainty fails closed; pre-write does not, and the asymmetry is deliberate.** The + page-2 probe was RETIRED, not converted — it does not repair anything now, because it is gone. What + changed is that the unreadable-history branch, which used to warn and leave the exemption green, + repairs instead; the probe had been repairing on that same uncertainty, so one check had been + disagreeing with itself. Failing closed is affordable only because paging removed the common trigger + (an ordinary head over 50 rows); on top of the old single-page read it would have cost the exemption + on every busy PR. Pre-write is the opposite: refusing a mark SKIPS the post-write check, so a partial + list is used rather than discarded. +- **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. + +**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 1f69999cf..d3e42e1d9 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -1013,24 +1013,53 @@ if "/timeline" in url: if "/statuses/" in url: mode = os.environ.get("STUB_HISTORY_MODE", "none") - # PAGE, honoured. THE REASON IS THE TERMINATOR, NOT THE COUNTERS — and that distinction was itself - # a finding (#751 round 5). An earlier version of this comment said the guard sits ahead of the - # read-counting modes "so the page-2 probe cannot shift 'raced row appears on read N'", by analogy - # with the combined endpoint. Measured: moving this guard AFTER the counter modes reddens NOTHING, - # because no history mode that counts reads ever issues a page-2 request — `raced=1` on page 1 - # short-circuits the probe. So that half of the rationale read as a checked reason and was not, - # which is the precise class this branch exists to retire. + # 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. # - # What IS load-bearing: page 2 must terminate for every mode that describes page 1 only, or the job - # re-reads page 1's rows as a second page and concludes the list is longer than it is. Dropping - # just the `print("[]")` below reddens `test_a_PRE_EXISTING_human_row_does_NOT_trigger_a_repair`. - # (The ordering guard on the COMBINED endpoint is a different story and genuinely is counter- - # related — moving that one reddens three mid-run-race tests.) + # 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 + # slice width is 50 and past the end is `[]` — NOT `null`, which is what + # `/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. # - # PAST THE END THIS ENDPOINT RETURNS `[]`, NOT `null` — measured at 1.27.1: - # `/statuses/{sha}?limit=100&page=99` is the two bytes `[]`, while `/commits/{sha}/status` past the - # end returns `{"statuses": null}`. A THIRD distinct empty shape on one server; the job tolerates - # both here precisely because guessing per endpoint has been wrong twice. + # 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. + # + # 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. + # + # DELIBERATELY ORDERING-BLIND, and this bounds what the paging tests prove. The real endpoint + # serves `created_unix DESC`, so a row created DURING the write window is among the NEWEST and + # lands on page 1 — a genuinely raced verdict would not sit on page 2 at all. These pages are + # served in insertion order regardless, so a fixture can place a row beyond page 1 and observe + # whether the walk reaches it. + # + # So `verdict-on-page-2` and `verdict-after-short-page` prove WALK COMPLETENESS — that the job + # reads past page 1, and past a short page, to a validated empty one. They do NOT prove that a + # real raced verdict would otherwise be missed; under the server's ordering it would not be. That + # is the honest scope, and the workflow comment says the same thing: the walk exists so the one + # fail-toward-SUCCESS path stops depending on an undocumented ordering, not because the ordering + # is currently wrong. + # + # Counter branches are confined to the page-1 arm below, so a later page can never advance them. + HIST_PAGE_SIZE = 50 + # ORDER-FAITHFUL MODES HONOUR `sort`. Most modes here are ordering-blind on purpose (see above): + # they test walk COMPLETENESS, which is an order-independent property, and serving insertion order + # lets a fixture place a row beyond page 1. But the partial-mark fallback's safety is a claim ABOUT + # the ordering, so the mode that pins it serves the real arrangement — DESC by default, ASC when + # the request asks for `sort=highestindex`. That is what lets re-adding the withdrawn parameter be + # caught by BEHAVIOUR rather than only by a structural assertion on the request line. + want_asc = "sort=highestindex" in url + order_faithful = False hist_page = 1 for part in url.split("?", 1)[-1].split("&"): if part.startswith("page="): @@ -1038,55 +1067,249 @@ if "/statuses/" in url: hist_page = int(part.split("=", 1)[1]) except ValueError: hist_page = 1 + + 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" + 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. + ctr = out / "history_reads.txt" + seen = int(ctr.read_text()) if ctr.exists() else 0 + ctr.write_text(str(seen + 1)) + if seen > 0: + # Configurable creator (ersatztv#742). The RACED test must stay broader than the + # 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 = [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. + 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": 6000, "context": "review-verdict/h10", "status": "pending", + "creator": None, + "description": "Human verdict raced this exemption write — re-post the verdict"}] + elif mode == "stale-human-already-present": + # A human verdict that was ALREADY in the history before this run — e.g. one whose + # recorded base did not match, which `read_existing_verdict` deliberately declines to + # honour. It must NOT be mistaken for a raced write, or every later run of that PR would + # repair its own exemption to pending forever. + 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", "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 = 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. 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 == "premark-page2-error-desc": + # THE PARTIAL-MARK FALLBACK, MEASURED UNDER THE REAL ORDERING. 60 rows, one of them a + # PRE-EXISTING base-mismatched human verdict at id 7055 — genuinely older than the newest + # row and therefore not something that raced this write. + # + # Under the server default (DESC) page 1 carries 7059..7010, so a walk that fails on page 2 + # still saw the true maximum: the mark is 7059, the verdict at 7055 is below it, and the + # exemption correctly STANDS. Under ASC page 1 carries 7000..7049, the salvaged mark is + # 7049, and that same pre-existing verdict tests as NEWER than the mark — a spurious sticky + # repair on a head nothing raced, which is the #761 failure. This fixture is what makes + # that difference observable. + order_faithful = True + rows = [{"id": 7000 + i, "context": "ci/other", "status": "success", + "creator": None, "description": "unrelated"} + for i in range(60) if 7000 + i != 7055] + rows.append({"id": 7055, "context": "review-verdict/h10", "status": "success", + "creator": {"login": "timothy"}, + "description": "Review-verdict: MERGEABLE @ a9e3e23 (base: other)"}) + rows.sort(key=lambda r: r["id"]) + elif mode == "null-page1-after-post": + # The post-write read answers "no statuses exist" for a sha this job has just POSTed to. + # That is impossible — the endpoint returns one row per POST — but it is well-formed, so + # nothing retries it and the walk reports SUCCESS over it. + # + # NO RACED VERDICT IS PLANTED, deliberately. An earlier shape appended one to dramatise + # the stakes, and it was inert twice over: unconditionally it also joined the PRE-write + # read, which set the mark to its OWN id — the strict `> $since` then excluded it — and + # gated to the post-write read it is never served at all, because page 1 answers `null` + # before any row reaches the wire. A row the test + # cannot observe is decoration that reads as coverage. What is actually under test is + # narrower and sufficient: a response asserting an empty history for a sha this job wrote + # to must not be accepted as proof that nothing raced. + rows = [{"id": 7000 + i, "context": "ci/other", "status": "success", + "creator": None, "description": "unrelated"} for i in range(3)] + elif mode == "null-terminator": + # This endpoint returns `[]` past the end today. `null` is what `/issues/{n}/timeline` + # returns, and an array-only gate on THAT endpoint was ersatztv#751 — the walk never + # reached a validated empty page and every exemption was withheld. Same shape, second + # endpoint, worse blast radius (the sticky sentinel), so `null` is tolerated and this + # fixture proves it rather than asserting it. + rows = [{"id": 7000 + i, "context": "ci/other", "status": "success", + "creator": None, "description": "unrelated"} for i in range(60)] + elif mode == "string-id-preexisting-row": + # THE TWIN of `string-id-inflates-mark`, on the COMPARISON rather than the maximum. A + # PRE-EXISTING row (a base-mismatched verdict `read_existing_verdict` declines to honour) + # carries a string id. jq orders strings above every number, so `.id > $since` reads it as + # newer than any mark and counts it as having raced this write — a sticky sentinel and a + # false "was overwritten" on every later run of that PR, forever. + rows = [{"id": 9999, "context": "ci/other", "status": "success", + "creator": None, "description": "unrelated"}, + {"id": "3", "context": "review-verdict/h10", "status": "success", + "creator": {"login": "timothy"}, + "description": "Review-verdict: MERGEABLE @ a9e3e23 (base: other)"}] + elif mode == "no-numeric-ids": + # A NON-EMPTY history in which no row carries a numeric id. Distinct from an EMPTY history, + # whose mark of 0 is correct; collapsing this case to 0 too would be a schema we cannot + # read reported as one we can. + rows = [{"id": "abc", "context": "ci/other", "status": "success", + "creator": None, "description": "unrelated"}] + elif mode == "over-cap": + # Longer than the 20-page cap (1050 rows), so the walk can never reach a validated empty + # page. That is "could not establish", not exhaustion. + rows = [{"id": 20000 + i, "context": "ci/other", "status": "success", + "creator": None, "description": "unrelated"} for i in range(1050)] + 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 order_faithful and not want_asc: + rows = list(reversed(rows)) + 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" + if posted_log.exists(): + for line in posted_log.read_text().splitlines(): + if not line.strip(): + continue + body = json.loads(line) + # 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 + own = {"id": nxt, "context": body.get("context"), + "status": body.get("state"), "creator": None, + "description": body.get("description", "")} + # WHERE the newest row goes depends on the served ordering. Under DESC it is FIRST; + # appending it last would put the job's own POST on the OLDEST page, the arrangement + # the order-faithful fixture exists to rule out. Ordering-blind modes keep insertion + # order, which is what lets them place a row beyond page 1. + if order_faithful and not want_asc: + pages[0].insert(0, own) + else: + pages[-1].append(own) + 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 [[]] + + 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 counts page-1 REQUESTS, retries included — not logical reads. + 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 not in ("second-page", "second-page-garbage", "second-page-error"): - print("[]") + if hist_page == 1 and mode == "null-page1-after-post" and n_logical >= 2: + print("null") sys.exit(0) - if mode == "second-page": - # The history runs past page 1, so "no raced row on page 1" does not establish "no race". The - # rows are deliberately ORDINARY (no verdict, no sentinel): the point is that unread rows EXIST, - # not that a verdict was found, so the repair must fire on uncertainty alone. - print(json.dumps([{"id": 7000 + i, "context": "ci/other", "status": "success", - "creator": None, "description": "unrelated"} for i in range(3)])) + if hist_page > 1 and mode == "null-terminator" and hist_page > len(pages): + print("null") sys.exit(0) - rows = [] - 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. - ctr = out / "history_reads.txt" - seen = int(ctr.read_text()) if ctr.exists() else 0 - ctr.write_text(str(seen + 1)) - if seen > 0: - # Configurable creator (ersatztv#742). The RACED test must stay broader than the - # 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)"}] - 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 (round 5). - 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": 6000, "context": "review-verdict/h10", "status": "pending", - "creator": None, - "description": "Human verdict raced this exemption write — re-post the verdict"}] - elif mode == "stale-human-already-present": - # A human verdict that was ALREADY in the history before this run — e.g. one whose recorded - # base did not match, which `read_existing_verdict` deliberately declines to honour. It must - # NOT be mistaken for a raced write, or every later run of that PR would repair its own - # exemption to pending forever. - rows = [{"id": 10, "context": "review-verdict/h10", "status": "success", - "creator": {"login": "timothy"}, - "description": "Review-verdict: MERGEABLE @ a9e3e23 (base: other)"}] - print(json.dumps(rows)) + if hist_page > 1 and mode in ("premark-page2-error", "premark-page2-error-desc") 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. + # `n_logical` is incremented on every page-1 REQUEST, including retried ones — which is fine + # here because page 1 never fails in this mode, but it counts requests, not logical reads. + sys.exit(22) + + print(json.dumps(pages[hist_page - 1] if hist_page - 1 < len(pages) else [])) sys.exit(0) if "/status" in url: @@ -2670,7 +2893,16 @@ def test_the_high_water_MARK_is_captured_BEFORE_the_last_moment_re_read(): # Assert on BOTH the init and the FETCH: keying only on the init line would let a refactor that # splits initialisation from the read slide the actual round-trip back past the re-read while this # test stayed green (round-3 review nit). - mark = max(src.index("max_id_before=-1"), src.index('hist_before=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA')) + # The FETCH is now the first bare `page_statuses` CALL (ersatztv#763 replaced the single + # `hist_before=$(gh …)` read with the paged walk). Anchoring on the call and not on the function + # 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)] assert len(calls) >= 2, f"expected two read_existing_verdict call sites, found {len(calls)}" @@ -3513,25 +3745,62 @@ def test_an_UNREADABLE_page_2_refuses_to_conclude_that_no_verdict_exists(tmp_pat ) -def test_a_status_history_RUNNING_PAST_PAGE_1_repairs_rather_than_leaving_green(tmp_path): - """The post-write race check reads ONE page of `/statuses/{sha}`, and `limit=100` clamps to the - server-wide cap (measured 50). So "no raced row on page 1" does not establish "no race" — a raced - human `failure` can sit on a page this job never reads. +def test_a_status_history_RUNNING_PAST_PAGE_1_is_PAGED_and_the_exemption_STANDS(tmp_path): + """THE INVERSION (ersatztv#763). This test asserted the opposite until real paging existed, and + the change of expectation IS the fix. - This is the ONE path in the design whose failure direction is toward SUCCESS: missing a raced - verdict leaves a forged green over a human rejection, permanently. So unread rows are treated as a - race and the exemption is repaired to `pending`, which is the conservative direction — a stall a - reviewer can clear, rather than a rejection silently turned green. + Before: the post-write check read one clamped page, so "no raced row on page 1" did not establish + "no race", and #751's conservative probe treated any rows on page 2 as a race. A head with more + status rows than the cap therefore lost its exemption to a STICKY sentinel with no race anywhere + near it. That is not hypothetical — it fired on Renovate PR #761, whose head accumulated rows over + ordinary CI re-runs until the gate started reporting a human verdict as overwritten when none + existed. - The stub's second page carries ORDINARY rows, no verdict and no sentinel: what must trigger the - repair is the mere existence of rows this job did not read, not the discovery of a verdict. + After: the walk pages to a validated empty page, so rows beyond the first page are simply READ. + The stub's history is 60 ORDINARY rows — no verdict, no sentinel — and the correct answer is that + nothing raced this write, so the exemption must STAND. + + This is the discriminating half of the pair: it is the assertion that goes red against the old + single-page read (which would repair here), while + `test_a_raced_verdict_on_PAGE_2_is_detected_and_repaired` goes red against a walk that pages but + stops early. Neither test alone can tell "pages correctly" from "repairs on everything". """ posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="second-page") assert r.returncode == 0, r.stderr seq = _posted_sequence(tmp_path) + assert len(seq) == 1, ( + "the exemption was repaired away even though the whole history was readable and carried no " + f"verdict — the page-2 'assume raced' probe is still in force. Posts: {seq}\n{r.stdout[-900:]}" + ) + assert seq[0]["state"] == "success", f"expected the exemption to stand; got {seq}" + # The false `::error::` from the old probe is the thing PR #761 could not be diagnosed from, so + # pin its absence too — a run that stands green but still shouts about an overwritten verdict + # would satisfy the count assertion above while leaving the log as misleading as before. + assert "was overwritten" not in (r.stdout + r.stderr), ( + f"the exemption stood, but the log still claims a verdict was overwritten:\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. + + A verdict sits BEYOND the first page — 60 ordinary rows with it inserted at index 55 — so only a + walk that continues past page 1 can find it. Under the pre-#763 code the exemption would still + have been repaired here, but for the wrong reason (rows merely EXISTED on page 2), which is why + this test is paired with the one above rather than standing alone: together they separate + "actually read page 2" from "repaired on any uncertainty". + + SCOPE, stated because the double is ordering-blind: this proves WALK COMPLETENESS, not that a real + raced verdict would otherwise be missed. The live endpoint serves `created_unix DESC`, so a row + created inside the write window is among the newest and lands on page 1. The walk exists so that + the one fail-toward-SUCCESS path does not rest on that undocumented ordering. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="verdict-on-page-2") + assert r.returncode == 0, r.stderr + seq = _posted_sequence(tmp_path) assert len(seq) == 2, ( - "the exemption was posted and left standing even though the status history ran past page 1, so " - f"a raced verdict beyond it would be buried. Posts: {seq}\n{r.stdout[-900:]}" + "a human verdict sitting on page 2 of the status history was not detected, so the exemption " + f"stands over a rejection nobody read. 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}" @@ -3539,6 +3808,388 @@ def test_a_status_history_RUNNING_PAST_PAGE_1_repairs_rather_than_leaving_green( assert seq[1]["description"] == "Human verdict raced this exemption write — re-post the verdict", ( f"repaired, but not to the sentinel, so the fixed point is broken: {seq[1]}" ) + # It must be reported as a REAL overwrite, not as uncertainty — the two now say different things + # in the log, and a verdict genuinely found on page 2 is the case that earns the strong wording. + assert "was overwritten" in (r.stdout + r.stderr), ( + f"repaired, but the log did not report an overwritten verdict:\n{r.stdout[-900:]}" + ) + + +def test_a_partial_mark_is_SAFE_because_the_newest_rows_are_on_page_1(tmp_path): + """The partial-mark fallback's safety is a claim ABOUT THE ORDERING, so it is measured under the + real one — this is the only order-faithful fixture here. + + The fallback exists because refusing a mark skips the post-write check entirely, which is a + fail-open. It is safe only because the server serves `created_unix DESC` and ids are monotonic + with `created_at`: page 1 therefore carries the true maximum, so a walk that fails on page 2 has + still seen it. + + The history holds a PRE-EXISTING base-mismatched verdict at id 7055 — older than the newest row, + so nothing it does raced this write. Under DESC the salvaged mark is 7059 and that row is below + it: the exemption correctly STANDS. Under ASC (`sort=highestindex`, withdrawn in round 4) page 1 + would be 7000..7049, the mark 7049, and that same untouched row would test as NEWER than the mark + — a sticky repair on a head nothing raced, which is exactly #761. + + So re-adding the sort parameter reddens this test by BEHAVIOUR, not merely by the structural + assertion in `test_the_walk_does_not_request_a_SORT_order`. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="premark-page2-error-desc") + assert r.returncode == 0, r.stderr + log = r.stdout + r.stderr + # This pins that the walk really was INCOMPLETE — it does not prove the fallback ran, because the + # warning is emitted by a different condition than the one that salvages the mark. Deleting the + # fallback leaves this line green; the sibling test + # `test_a_PRE_WRITE_paging_failure_still_yields_a_usable_high_water_mark` is what reddens then. + assert "taking the high-water mark over the" in log, ( + f"the pre-write walk completed, so this test is not exercising a partial mark at all:\n{r.stdout[-900:]}" + ) + seq = _posted_sequence(tmp_path) + assert len(seq) == 1, ( + "a PRE-EXISTING verdict was read as newer than the salvaged mark, so the exemption was " + f"repaired away on a head nothing raced. Posts: {seq}\n{r.stdout[-900:]}" + ) + assert seq[0]["state"] == "success", f"expected the exemption to stand; got {seq}" + assert "was overwritten" not in log, ( + f"nothing raced this write, but the log claims a verdict was overwritten:\n{r.stdout[-900:]}" + ) + + +def test_an_EMPTY_post_write_history_is_not_evidence_that_nothing_raced(tmp_path): + """A well-formed response that cannot be true must not be trusted, and this one is free to reach. + + The walk terminates on an empty page — correct BEFORE the write, where a head nothing has posted + to genuinely has no statuses. AFTER the write it is impossible: this job has just POSTed, and + `/statuses/{sha}` returns one row per POST, so "no statuses exist" contradicts a write that + succeeded. Nothing retries it either, because the body is valid JSON of the right type. + + Accepting it yields `raced=0` from a list that cannot be real, on the one path whose failure + direction is toward SUCCESS — and silently, since the walk itself reports success. Whether a + verdict actually raced this particular write is not the point and is not modelled here: the + response is not evidence either way, so concluding "nothing raced" from it is unsound regardless. + + This is NOT the withdrawn currency witness. That asked "is there ANY row above the mark", which an + unrelated newer row satisfied while the rejection stayed hidden. This asks only whether the list is + EMPTY — a state no unrelated row can produce and no ordering can disguise. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="null-page1-after-post") + assert r.returncode == 0, r.stderr + log = r.stdout + r.stderr + seq = _posted_sequence(tmp_path) + assert len(seq) == 2, ( + "an empty post-write history was accepted as proof that nothing raced this write, so an " + f"exemption stands on a head whose write window was never inspected. 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 "came back empty" in log, ( + f"repaired, but the impossible empty read was not reported as the reason:\n{r.stdout[-900:]}" + ) + assert "was overwritten" not in log, ( + f"an unverifiable read was reported as an overwritten human verdict:\n{r.stdout[-900:]}" + ) + # THE REASON IS PART OF THE FIX, so it is asserted POSITIVELY. An absence assertion alone leaves + # `raced_why` unpinned — any string at all, including a placeholder, satisfies it while the + # operator gets a sticky sentinel explained by nothing. The em-dash and full stop discriminate the + # `::error::` reason from the `::warning::` text, which continues "…, which cannot be true". + assert "— the status history came back empty after this job posted to it." in log, ( + "the repair did not report its own reason; the ::error:: carries whatever `raced_why` held " + f"and nothing pins it:\n{r.stdout[-900:]}" + ) + # The walk COMPLETED here, on a validated terminator — the answer was impossible, not unreadable. + # Borrowing the unreadable-history wording would send an operator staring at a sticky sentinel + # looking for an API failure that never happened. + assert "could not be read completely" not in log, ( + "an empty-but-complete read was reported as an incomplete one, which points at the wrong " + f"cause:\n{r.stdout[-900:]}" + ) + + +def test_a_NULL_page_terminates_the_walk_as_an_empty_one(tmp_path): + """`/statuses/{sha}` returns `[]` past the end today; `/issues/{n}/timeline` returns bare `null`. + + An array-only gate on the timeline was ersatztv#751: the walk never reached a validated empty + page, `rt_ok` was never `yes` for any PR, and the fence withheld EVERY exemption `success` from + the day it shipped. Re-adopting that narrowing on this endpoint would be worse, because this + walk's failure mode is the STICKY sentinel — every exempt PR would need a human verdict, per head, + clearable only by hand. + + Tolerating `null` cannot misread `[]`, so it costs nothing. This asserts it rather than trusting + that the endpoint will never change its empty shape. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="null-terminator") + assert r.returncode == 0, r.stderr + seq = _posted_sequence(tmp_path) + assert len(seq) == 1, ( + "a `null` page past the end was read as unreadable rather than as exhaustion, so the walk " + f"never validated and the exemption was repaired away. Posts: {seq}\n{r.stdout[-900:]}" + ) + assert seq[0]["state"] == "success", f"expected the exemption to stand; got {seq}" + + +def test_a_STRING_id_on_a_PRE_EXISTING_row_is_not_read_as_raced(tmp_path): + """The twin of `test_a_STRING_id_cannot_inflate_the_high_water_mark`, on the COMPARISON rather + than the maximum — and the twin was live on `main`, one expression away from the fix. + + jq orders strings above every number, so `.id > $since` reads a string id as newer than ANY mark. + A pre-existing row carrying one — such as a base-mismatched verdict `read_existing_verdict` + deliberately declines to honour — is therefore counted as having raced this write on EVERY run of + that PR. Each run posts the sticky sentinel and a false "was overwritten", so the exemption is + lost permanently and no re-trigger clears it. + + The row here is genuinely older than the mark (`"3"` against a mark of 9999) and must be invisible. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="string-id-preexisting-row") + assert r.returncode == 0, r.stderr + seq = _posted_sequence(tmp_path) + assert len(seq) == 1, ( + "a PRE-EXISTING row with a string id was read as newer than the high-water mark and treated " + f"as a raced verdict, so this PR loses its exemption permanently. Posts: {seq}\n{r.stdout[-900:]}" + ) + assert seq[0]["state"] == "success", f"expected the exemption to stand; got {seq}" + assert "was overwritten" not in (r.stdout + r.stderr), ( + f"nothing raced this write, but the log claims a verdict was overwritten:\n{r.stdout[-900:]}" + ) + + +def test_a_history_with_NO_numeric_ids_SKIPS_the_check_instead_of_marking_zero(tmp_path): + """An empty history and an unreadable one are different answers, and only one of them is 0. + + A head nothing has posted to has no rows and its mark is 0 — correct, every later row is newer. + A NON-empty history in which no row carries a numeric id is a schema this job cannot read. + Reporting that as 0 claims a mark was established when none was, and every pre-existing row then + tests as newer than it. + + The observable difference is the log: the unusable case says so and skips the post-write check. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="no-numeric-ids") + assert r.returncode == 0, r.stderr + log = r.stdout + r.stderr + assert "was not numeric" in log, ( + "a history carrying no numeric id at all was silently treated as a mark of 0, so the " + f"degradation is invisible in the log:\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_history_LONGER_THAN_THE_PAGE_CAP_is_not_treated_as_exhausted(tmp_path): + """The page cap is a refusal, not a terminator. + + Twenty requests of fifty rows cannot validate a history of 1050: the walk never reaches an empty + page, so it has NOT established that it saw everything. Treating the cap as exhaustion would hand + the post-write check a list it knows is partial and let it conclude "nothing raced" from it — on + the one path whose failure direction is toward SUCCESS. + + Only 950 rows can ever be validated, incidentally, because the twentieth request has to be the + empty terminator. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="over-cap") + assert r.returncode == 0, r.stderr + seq = _posted_sequence(tmp_path) + assert len(seq) == 2, ( + "a history longer than the page cap was treated as fully read, so a raced verdict beyond it " + f"would have been concluded absent. 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 "could not be read completely" in (r.stdout + r.stderr), ( + f"repaired, but not reported as an incomplete read:\n{r.stdout[-900:]}" + ) + + +def test_the_walk_does_not_request_a_SORT_order(tmp_path): + """A regression guard on a WITHDRAWAL, which otherwise nothing mechanical protects. + + `sort=highestindex` returns index ASCENDING, which would put the OLDEST rows on page 1. The + partial-mark fallback then takes its maximum over the oldest rows, leaving every pre-existing row + above the mark and read as raced — a spurious STICKY repair, which is the #761 failure this change + exists to remove. It was added, measured, and withdrawn for exactly that reason. + + Nothing about the code's behaviour reveals a re-added sort parameter (the tests' double is + ordering-blind by design), so the guard is structural: the request must not carry one. + """ + src = _classify_step()["run"] + # The REQUEST LINES ONLY — comments are excluded deliberately, because the withdrawal note inside + # `page_statuses` names the parameter in order to explain why it is not used. A guard that reads + # prose would fire on its own documentation. + requests = [line for line in src.splitlines() if "statuses/$SHA?" in line and not line.lstrip().startswith("#")] + assert requests, ( + "no `/statuses/{sha}` request line found; the walk has been reshaped, so re-point this guard " + "rather than letting the sort-order assertion below pass vacuously" + ) + offenders = [line.strip() for line in requests if "sort=" in line] + assert not offenders, ( + "the status-history request carries a sort order again. ASC ordering puts the OLDEST rows on " + "page 1, which inverts the partial-mark fallback into a spurious sticky repair — the #761 " + f"failure this change removes. See the withdrawal note in `page_statuses`. Offending: {offenders}" + ) + + +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 (ersatztv#763). + + 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( @@ -3567,6 +4218,17 @@ def test_an_UNREADABLE_history_page_2_also_repairs_rather_than_leaving_green(tmp assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", ( f"expected an exemption then a repair to pending; got {seq}" ) + # THE REASON IS PART OF THE FIX, so it is asserted. #761's stall was undiagnosable precisely + # because an uncertainty repair reported itself as an overwritten human verdict. Without these + # two the `raced_why` branch is unpinned — forcing it back to `human` reddens nothing. + log = r.stdout + r.stderr + assert "could not be read completely" in log, ( + f"repaired, but not reported as an incomplete read:\n{r.stdout[-900:]}" + ) + assert "was overwritten" not in log, ( + "an uncertainty repair is reported as an overwritten human verdict — the false ::error:: that " + f"made PR #761's stall undiagnosable:\n{r.stdout[-900:]}" + ) # --- Workflow token scope (ersatztv#748) -------------------------------------------------------