fix(870): an empty timeline page is not exhaustion — the walk reads to its cap #896
@@ -917,10 +917,14 @@ jobs:
|
||||
# Completeness is a guard, not an assumption (`ci.verdict-write-retarget-fence`; this
|
||||
# cited `ci.paged-endpoint-completeness` until 2026-08-06, a key that has never existed as
|
||||
# a record — resolve decisions through the catalog, never through a key or path quoted in a
|
||||
# comment). The count is
|
||||
# trusted ONLY when paging reached a validated EMPTY page. A short page, a non-array body, a
|
||||
# non-numeric length or the page cap all leave `rt_ok=no`, and an untrusted count is treated
|
||||
# below as "cannot tell" rather than as zero.
|
||||
# comment). The count is trusted ONLY when the walk read EVERY page up to its 20-page cap
|
||||
# AND the LAST page came back empty (ersatztv#870 — reaching an empty page earlier is no
|
||||
# longer sufficient, and no longer ends the walk). A page that is unreadable, a body that
|
||||
# is neither an array NOR `null` (a bare `null` is this endpoint's empty page and is
|
||||
# accepted), a non-numeric length, an empty FIRST page, or a cap reached on a NON-empty page all
|
||||
# leave `rt_ok=no`, and an untrusted count is treated below as "cannot tell" rather than as
|
||||
# zero. A SHORT page is not in that list and never was: it is tallied and the walk carries
|
||||
# on — the sentence here said otherwise until 2026-08-30.
|
||||
# THE SAME WALK CARRIES A SECOND AXIS: HEAD MUTATION (ersatztv#803/#664).
|
||||
#
|
||||
# The base fence above closes `main -> S -> main`. The HEAD aliases the same way and was
|
||||
@@ -980,17 +984,50 @@ jobs:
|
||||
# paragraph below the fix that removed it.
|
||||
#
|
||||
# ONE WALK, TWO COUNTERS, ONE TRUST FLAG. The two axes share `rt_ok` because they share the
|
||||
# pages it certifies: a walk that never reached a validated empty page has not established
|
||||
# EITHER count, and giving them separate flags would invite a future edit to trust one over
|
||||
# pages that did not certify the other.
|
||||
# pages it certifies: a walk that did not read to its cap and finish on an empty page has
|
||||
# not established EITHER count, and giving them separate flags would invite a future edit to
|
||||
# trust one over pages that did not certify the other.
|
||||
count_pr_mutations() {
|
||||
rt_count=0
|
||||
hp_count=0
|
||||
rt_ok=no
|
||||
local page=1 raw n m p bad kind total=0 ptotal=0
|
||||
local page=1 raw n m p bad kind try total=0 ptotal=0 empty=no
|
||||
while [ "$page" -le 20 ]; do
|
||||
raw=$(gh "$BASE_URL/repos/$REPO/issues/$PR/timeline?limit=50&page=${page}") || return 0
|
||||
if [ -z "${raw//[[:space:]]/}" ]; then return 0; fi
|
||||
# BOUNDED AND RETRIED ONCE, because this walk got 10x longer (ersatztv#870 review).
|
||||
# Both halves answer the same amplification and neither is decoration.
|
||||
#
|
||||
# BOUNDED: `gh` is bare `curl -sf` with no timeout of its own, and the THIRD call site
|
||||
# runs AFTER the exemption `success` is already posted — a hang there leaves the green
|
||||
# standing with no repair attempted. That exposure predates this change, but going
|
||||
# from ~2 requests per walk to a fixed 20 multiplies the chances of hitting it. The
|
||||
# bound is applied HERE rather than inside `gh` so every other call site keeps the
|
||||
# behaviour it was reviewed with: widening a shared helper changes callers nobody
|
||||
# re-examined. 15s is ~100x the measured per-request latency, so it bounds a hang
|
||||
# without reddening a slow-but-working instance.
|
||||
#
|
||||
# RETRIED: `page_statuses` retries each page once and states why — "one transient blip
|
||||
# would cost that head its exemption until a human clears it by hand". That argument
|
||||
# applies here with 10x the force after this change, because `rt_ok=no` on the
|
||||
# exemption path writes the sticky sentinel too. This walk had no retry when it made
|
||||
# ~2 requests; at 20 it needs the sibling's. Same shape deliberately: same `for try`,
|
||||
# same one-second sleep BETWEEN attempts only, same "an outage still refuses".
|
||||
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. (Carried from `page_statuses`, where the same trap was found.)
|
||||
for try in 1 2; do
|
||||
raw=$(gh --connect-timeout 5 --max-time 15 "$BASE_URL/repos/$REPO/issues/$PR/timeline?limit=50&page=${page}") || raw=""
|
||||
if [ -n "${raw//[[:space:]]/}" ]; then
|
||||
kind=$(printf '%s' "$raw" | jq -r 'type' 2>/dev/null) || kind=""
|
||||
fi
|
||||
# THIS SET MUST MATCH THE `case` ARMS BELOW. Their agreement is load-bearing rather
|
||||
# than tidy: breaking only on shapes the `case` accepts is what stops a stale `kind`
|
||||
# from attempt 1 being judged against a `raw` that attempt 2 cleared. Widen one
|
||||
# without the other and that correspondence is gone.
|
||||
if [ "$kind" = array ] || [ "$kind" = null ]; then break; fi
|
||||
if [ "$try" -eq 1 ]; then sleep 1; fi
|
||||
done
|
||||
# A PAGE PAST THE END IS JSON `null` HERE, NOT `[]` (ersatztv#751). Measured on this
|
||||
# instance at Gitea 1.27.1 (page 1 non-empty, page 2 exhausted): page 2 of PR #752's timeline is the
|
||||
# four bytes `null`, while `/issues/{n}/comments` with no comments returns `[]`. The
|
||||
@@ -1013,94 +1050,187 @@ jobs:
|
||||
# false" through it means relying on the same exit-status subtlety that already bit this
|
||||
# workflow once at jq 1.6 (ersatztv#647). A `case` over `jq -r 'type'` cannot be read
|
||||
# two ways. Anything that is neither `null` nor `array` is still "cannot tell".
|
||||
kind=$(printf '%s' "$raw" | jq -r 'type' 2>/dev/null) || kind=""
|
||||
# `null` counts as exhaustion only from page 2 ON. THE INVARIANT, not a figure that
|
||||
# rots: a real PR's timeline always carries at least one event on page 1 (it is created
|
||||
# by a push, which is itself an event), so a `null` FIRST page is anomalous rather than
|
||||
# empty. Spot-checked across #752/#753/#749/#739/#717, all non-empty; the counts
|
||||
# themselves are deliberately not recorded here because timelines grow and an earlier
|
||||
# version of this comment cited five numbers of which three were stale within days.
|
||||
# Trusting a zero count from an anomalous first page would
|
||||
# mean trusting that no retarget happened on the strength of a response we cannot
|
||||
# explain. Requiring one real page keeps the property the walk is for: something was
|
||||
# actually read. A PR that somehow has an empty first page falls through to `rt_ok=no`,
|
||||
# which withholds the exemption and asks for a human verdict — the safe direction.
|
||||
# This narrows rather than closes the general concern: a wrong `null` on page 3 is still
|
||||
# read as exhaustion, and no bounded number of round-trips can rule that out.
|
||||
#
|
||||
# AND THAT WRONG `null` IS CONSTRUCTIBLE, NOT HYPOTHETICAL (established 2026-08-28 by
|
||||
# cross-family review of ersatztv#803, from the v1.27.1 source; tracked
|
||||
# as ersatztv#870, because it predates #803 and defeats BOTH axes identically). The handler pages at the
|
||||
# DATABASE level and filters AFTERWARDS: `FindComments` applies the LIMIT/OFFSET, then
|
||||
# AN EMPTY PAGE IS NOT EXHAUSTION, AND THIS WALK NO LONGER TREATS IT AS ONE
|
||||
# (ersatztv#870). Gitea pages this endpoint at the DATABASE level and filters
|
||||
# AFTERWARDS: `FindComments` applies the LIMIT/OFFSET, then
|
||||
# `ListIssueCommentsAndTimeline` drops every `CommentTypeCode` row and every
|
||||
# cross-reference the reader cannot access, appending survivors to a `var apiComments
|
||||
# []*api.TimelineComment` — a NIL slice. So a page whose rows are ALL inline-code
|
||||
# comments serializes as bare `null` while later pages still hold events, and rows are
|
||||
# ordered ASCENDING, so the events a fence cares about are the ones furthest from page
|
||||
# 1. Fifty inline review comments, which a PR author can create on their own PR, are
|
||||
# enough to truncate both walks at the same place; both counts then agree, the sha
|
||||
# comparison agrees, and the ABA goes unseen.
|
||||
# cross-reference the reader cannot access, appending the survivors to a
|
||||
# `var apiComments []*api.TimelineComment` — a NIL slice, which serializes as bare
|
||||
# `null`. So a page whose rows are ALL inline-code comments comes back empty WHILE
|
||||
# LATER PAGES STILL HOLD EVENTS, and rows are ordered ASCENDING, so the events a fence
|
||||
# looks for are the ones furthest from page 1. Fifty inline review comments, which a
|
||||
# PR author can create on their own PR, were enough to truncate both walks at the same
|
||||
# place: both counts then agreed, the sha comparison agreed, and the ABA went unseen.
|
||||
#
|
||||
# So DO NOT read the fence below as closing the head ABA outright. What it closes is
|
||||
# the ABA on a PR whose timeline has no such truncating block — which is every PR this
|
||||
# repo has, but is not a property anyone enforces. Stated here rather than left in the
|
||||
# issue because the whole of #803 is that a contract asserting more than its code does
|
||||
# is worse than no contract.
|
||||
# THE FIX MOVES THE INFERENCE TO THE CAP; IT DOES NOT REMOVE IT, and saying so
|
||||
# precisely matters more here than a tidy claim. This loop already carries a 20-page
|
||||
# cap, and the sibling status walk below already states the CAP half of the contract
|
||||
# this one was breaking — "the 20th request has to be the EMPTY terminator for the
|
||||
# walk to be validated". Only the cap half: `page_statuses` still returns on its FIRST
|
||||
# empty page, so the two walks do NOT now share a termination rule and saying they do
|
||||
# would be a parity claim nobody established (ersatztv#893 tracks whether that walk
|
||||
# needs the same change; `/statuses/{sha}` returns a TRUE `X-Total-Count`, which this
|
||||
# endpoint does not, so its terminator may well mean what it says). So the walk now
|
||||
# reads EVERY page up to that cap and decides afterwards:
|
||||
# an empty page BEFORE the cap is SKIPPED, never believed. The ONE empty page still
|
||||
# trusted is the page-20 terminator, and it is trusted for the same unprovable reason
|
||||
# the old page-2 terminator was — the endpoint cannot distinguish a filtered page from
|
||||
# the end of the list, at ANY offset. What changes is the price — see the note on
|
||||
# the residual below for the one statement of it this file makes. A "require K consecutive empty
|
||||
# pages" rule was rejected because K is arbitrary and only moves the bar to 50K
|
||||
# filtered rows; the cap is not arbitrary — it is the coverage this walk already
|
||||
# claimed to perform.
|
||||
#
|
||||
# MEASURED AT GITEA 1.27.1 ON THIS INSTANCE (2026-08-30), because the cheaper
|
||||
# alternatives are ruled out by measurement rather than by reasoning:
|
||||
# * `X-Total-Count` on this endpoint is the POST-FILTER LENGTH OF THE PAGE, not a
|
||||
# total. `?limit=1` returns `X-Total-Count: 1` on a PR whose timeline holds 14
|
||||
# rows, `?limit=5` returns 5, and a page past the end returns 0. It therefore
|
||||
# carries exactly what the body already carries and cannot derive a page count.
|
||||
# (`/activities/feeds` DOES return a true total — 5739 — so this is a property of
|
||||
# this handler, not of the server, and a future Gitea could change it.)
|
||||
# * `limit` clamps to 50 here: `?limit=200` returned 50 on #303, #237, #91 and #202.
|
||||
# * The endpoint's ONLY query parameters are `since`, `before`, `page` and `limit`
|
||||
# (read from the live swagger, `issueGetCommentsAndTimeline`). There is no
|
||||
# row-type filter, so the paged set and the serialized set cannot be made to agree.
|
||||
# A `since`-bounded walk was considered and NOT taken on its own: it narrows the attack
|
||||
# to the run's own window but leaves the identical ambiguity inside it, and it would
|
||||
# cost the "a real page was actually read" property below, which depends on an empty
|
||||
# page 1 being anomalous. COST, both bounds, because only one of them is the common
|
||||
# case: a healthy walk is exactly 20 requests, measured at 2.6s against this
|
||||
# instance; the worst case with the per-page retry below is 40 requests and 20
|
||||
# one-second sleeps — every page blipping once and recovering — which is 20s of
|
||||
# sleep on top. MEASURED by executing this function against scripted responses,
|
||||
# not counted by eye: the first draft of this sentence said 19, reasoning that
|
||||
# the last page could not sleep. It can.
|
||||
#
|
||||
# WALL-CLOCK, which is what the timeout is actually for. An earlier version of this
|
||||
# comment reasoned that only the last page can pay two timeouts, since a page whose
|
||||
# BOTH attempts fail ends the walk, and put the bound at ~335s. That bounds TIMEOUTS,
|
||||
# not elapsed time: attempt 1 can burn its full 15s and attempt 2 still SUCCEED
|
||||
# slowly, so the page costs ~31s and the walk carries on. The pessimum is therefore
|
||||
# 20 x (15 + 1 + 15) = 620s per walk — ~21min on the exempt path's three walks — and
|
||||
# the job carries no `timeout-minutes` to catch it.
|
||||
#
|
||||
# Stated anyway rather than tuned down, because the DIRECTION is what justifies it:
|
||||
# before this, `gh` carried no timeout at all and one hung connection had no bound
|
||||
# whatsoever. Bounded-and-slow replaces unbounded — for THIS call site only.
|
||||
# `page_statuses` still has no `--max-time`, walks up to 20 pages x 2 attempts, and
|
||||
# runs in the same post-POST window ahead of the third walk here, so the "a hang
|
||||
# leaves the green standing" mode is still reachable through the sibling.
|
||||
#
|
||||
# WHAT REMAINS UNCOVERED — NOT CLOSED, NARROWED — stated because #803's whole point
|
||||
# is that a contract must not assert more than its code does. Pages 1-19 cover offsets
|
||||
# 0..949 and page 20 covers 950..999, so an event is still hidden by a timeline of
|
||||
# more than 1000 rows that ALSO presents a fully-filtered block at offsets 950..999:
|
||||
# the walk reads an empty page 20, never requests page 21, and publishes a too-low
|
||||
# count with `rt_ok=yes`. Both re-reads share the blind spot, so the counts agree and
|
||||
# the exemption is granted — the SAME construction as before, at the cap instead of at
|
||||
# page 2, at roughly 10x the price. State that price ONCE and in one unit, because
|
||||
# two earlier drafts of this comment stated it two ways 45 lines apart. The FILTERED
|
||||
# BLOCK is 50 rows in both cases — that is the part which must be inline review
|
||||
# comments. What grew is the timeline the attacker has to build around it: ~100 rows
|
||||
# before (a non-empty page 1, then a 50-row block at a 50-aligned offset) against more
|
||||
# than 1000 now, with the block pinned to offsets 950..999. On their own PR the padding
|
||||
# is theirs to supply too, so "50 comments either way" was wrong: it counted only the
|
||||
# block and ignored the ~950 rows that now have to exist beneath it.
|
||||
#
|
||||
# This is NOT the same residual as the status walk's, and equating them would
|
||||
# understate it: there a non-empty page 20 is the only over-cap shape, because
|
||||
# `/statuses/{sha}` does not filter after paging. Only this endpoint lets a page-20
|
||||
# response be empty while later pages hold rows. A timeline that fills the cap with
|
||||
# READABLE pages does fail closed; one that fills it and hides the 20th behind a
|
||||
# filtered block does not.
|
||||
#
|
||||
# No terminator AVAILABLE on this endpoint does better — measured, not assumed, and
|
||||
# NOT a proof that none could exist. A different page SIZE, for instance, shifts the
|
||||
# boundaries without closing anything: `?limit=49&page=21` reaches offsets 980..1028,
|
||||
# past this walk's horizon of 999, and its page edges do not coincide with a 50-row
|
||||
# walk's, so a block that exactly fills one walk's terminator page does not fill the
|
||||
# other's. Strictly better, still not a terminator. See the measurements above: there is no total to page against, no row-type filter to
|
||||
# make the paged and serialized sets agree, and a `since` bound leaves the identical
|
||||
# ambiguity inside its window. Closing this needs a monotonic key that is not served
|
||||
# by a filtered, paged endpoint, and none is known (ersatztv#870 option 3).
|
||||
#
|
||||
# Read the type as a VALUE rather than through `jq -e` — see the `kind` note above.
|
||||
empty=no
|
||||
case "$kind" in
|
||||
null)
|
||||
if [ "$page" -gt 1 ]; then rt_ok=yes; rt_count=$total; hp_count=$ptotal; fi
|
||||
return 0 ;;
|
||||
null) empty=yes ;;
|
||||
array) ;;
|
||||
*) return 0 ;;
|
||||
esac
|
||||
n=$(printf '%s' "$raw" | jq -r 'length')
|
||||
case "$n" in ''|*[!0-9]*) return 0 ;; esac
|
||||
# AN EMPTY **ARRAY** FIRST PAGE IS AS ANOMALOUS AS A `null` ONE (fixed 2026-08-28,
|
||||
# ersatztv#803 review). The `null` arm above requires `page > 1` and the comment
|
||||
# above it says "a PR that somehow has an empty first page falls through to
|
||||
# `rt_ok=no`" — but this arm trusted `[]` on ANY page, so the code contradicted its
|
||||
# own stated rule and the sentence describing the safe behaviour was false for one of
|
||||
# the two empty shapes. A `[]` page 1 yielded `rt_ok=yes` over a ZERO count, which is
|
||||
# a fail-OPEN: it certifies "nothing was retargeted and nothing was pushed" from a
|
||||
# response that cannot be explained, and a docs-only PR is then exempted on it. The
|
||||
# two shapes now take the SAME rule, which is also what stops a serializer change
|
||||
# from silently switching the walk between a trusted and an untrusted terminator.
|
||||
if [ "$n" -eq 0 ]; then
|
||||
if [ "$page" -gt 1 ]; then rt_ok=yes; rt_count=$total; hp_count=$ptotal; fi
|
||||
return 0
|
||||
if [ "$empty" = no ]; then
|
||||
n=$(printf '%s' "$raw" | jq -r 'length' 2>/dev/null) || n=""
|
||||
case "$n" in ''|*[!0-9]*) return 0 ;; esac
|
||||
# `[]` AND `null` TAKE THE SAME RULE (ersatztv#803 review). `/issues/{n}/comments`
|
||||
# really does return `[]` when empty while this endpoint returns `null`, so both
|
||||
# shapes are live on this server; treating them differently once let a `[]` page 1
|
||||
# certify a zero count. Keeping them identical is also what stops a serializer
|
||||
# change from silently switching the walk between a trusted and an untrusted
|
||||
# terminator.
|
||||
if [ "$n" -eq 0 ]; then empty=yes; fi
|
||||
fi
|
||||
if [ "$empty" = yes ]; then
|
||||
# AN EMPTY PAGE 1 IS ANOMALOUS AND ENDS THE WALK UNTRUSTED. THE INVARIANT, not a
|
||||
# figure that rots: a real PR's timeline always carries at least one event on page 1
|
||||
# (the PR is created by a push, which is itself an event). Spot-checked across
|
||||
# #752/#753/#749/#739/#717, all non-empty; the counts themselves are deliberately
|
||||
# not recorded here because timelines grow and an earlier version of this comment
|
||||
# cited five numbers of which three were stale within days. Trusting a zero count
|
||||
# from an anomalous first page would mean trusting that no retarget happened on the
|
||||
# strength of a response we cannot explain, so it falls through to `rt_ok=no`, which
|
||||
# withholds the exemption and asks for a human verdict — the safe direction.
|
||||
if [ "$page" -eq 1 ]; then return 0; fi
|
||||
else
|
||||
# EVERY jq READ BELOW IS GUARDED `2>/dev/null || x=""`, matching `page_statuses`
|
||||
# (added ersatztv#870 review). Today none of them can error — the `array` gate above
|
||||
# and the `bad -eq 0` ordering see to that — but "safe because of a property three
|
||||
# lines up" is exactly the reasoning the `jq -e` note below says it is retiring, and
|
||||
# an unguarded jq error dies under `set -e`. At the POST-WRITE call site that death
|
||||
# lands after the exemption `success` is on the head and before any repair, which is
|
||||
# the shape this file already fixed once for `.creator`. The `case` validation on
|
||||
# each value turns an empty string into the existing untrusted path.
|
||||
#
|
||||
# VALIDATE THE FIELD THE SECURITY DECISION READS, on every row (ersatztv#803 review).
|
||||
# Both tallies below select on `.type`; a row without a string `.type` matches neither
|
||||
# and is silently treated as an irrelevant event. That is the same
|
||||
# validate-what-you-consume rule `pr-changed-files.sh` already applies to every row it
|
||||
# extracts, and the reason is the same: a shape we cannot read is not evidence that
|
||||
# nothing happened. Every timeline row measured on this instance carries a string
|
||||
# `.type`, so this rejects only shapes the server has not been observed to produce.
|
||||
#
|
||||
# COUNTED, NOT `jq -e`, and that is this function's own standing rule rather than a
|
||||
# style preference. The `kind` read above says it in full: `jq -e` reports the
|
||||
# truthiness of its LAST OUTPUT, which is the exit-status subtlety that already bit
|
||||
# this workflow at jq 1.6 (ersatztv#647), and the runner still ships 1.6. A count read
|
||||
# as a VALUE and then range-checked in shell cannot be read two ways, and it matches
|
||||
# every other tally here. (The first draft of this guard used `jq -e`, safely by
|
||||
# accident — the array is known non-empty by the check above, so the empty-input
|
||||
# divergence could not fire — but "safe because of a property three lines up" is
|
||||
# exactly the reasoning the rule exists to retire.)
|
||||
bad=$(printf '%s' "$raw" | jq -r '[.[] | select(type != "object" or (.type | type) != "string")] | length' 2>/dev/null) || bad=""
|
||||
case "$bad" in ''|*[!0-9]*) return 0 ;; esac
|
||||
if [ "$bad" -ne 0 ]; then return 0; fi
|
||||
m=$(printf '%s' "$raw" | jq -r '[.[] | select(.type == "change_target_branch")] | length' 2>/dev/null) || m=""
|
||||
case "$m" in ''|*[!0-9]*) return 0 ;; esac
|
||||
# Counted in the SAME pass over the SAME page, so the head axis costs no extra round
|
||||
# trip and cannot be certified by a different set of pages than the base axis. A
|
||||
# non-numeric length here abandons BOTH counts, exactly as it does above — a page whose
|
||||
# push tally is unreadable has not established the retarget tally either.
|
||||
p=$(printf '%s' "$raw" | jq -r '[.[] | select(.type == "pull_push")] | length' 2>/dev/null) || p=""
|
||||
case "$p" in ''|*[!0-9]*) return 0 ;; esac
|
||||
total=$(( total + m ))
|
||||
ptotal=$(( ptotal + p ))
|
||||
fi
|
||||
# VALIDATE THE FIELD THE SECURITY DECISION READS, on every row (ersatztv#803 review).
|
||||
# Both tallies below select on `.type`; a row without a string `.type` matches neither
|
||||
# and is silently treated as an irrelevant event. That is the same
|
||||
# validate-what-you-consume rule `pr-changed-files.sh` already applies to every row it
|
||||
# extracts, and the reason is the same: a shape we cannot read is not evidence that
|
||||
# nothing happened. Every timeline row measured on this instance carries a string
|
||||
# `.type`, so this rejects only shapes the server has not been observed to produce.
|
||||
#
|
||||
# COUNTED, NOT `jq -e`, and that is this function's own standing rule rather than a
|
||||
# style preference. The `kind` read above says it in full: `jq -e` reports the
|
||||
# truthiness of its LAST OUTPUT, which is the exit-status subtlety that already bit
|
||||
# this workflow at jq 1.6 (ersatztv#647), and the runner still ships 1.6. A count read
|
||||
# as a VALUE and then range-checked in shell cannot be read two ways, and it matches
|
||||
# every other tally here. (The first draft of this guard used `jq -e`, safely by
|
||||
# accident — the array is known non-empty by the check above, so the empty-input
|
||||
# divergence could not fire — but "safe because of a property three lines up" is
|
||||
# exactly the reasoning the rule exists to retire.)
|
||||
bad=$(printf '%s' "$raw" | jq -r '[.[] | select(type != "object" or (.type | type) != "string")] | length')
|
||||
case "$bad" in ''|*[!0-9]*) return 0 ;; esac
|
||||
if [ "$bad" -ne 0 ]; then return 0; fi
|
||||
m=$(printf '%s' "$raw" | jq -r '[.[] | select(.type == "change_target_branch")] | length')
|
||||
case "$m" in ''|*[!0-9]*) return 0 ;; esac
|
||||
# Counted in the SAME pass over the SAME page, so the head axis costs no extra round
|
||||
# trip and cannot be certified by a different set of pages than the base axis. A
|
||||
# non-numeric length here abandons BOTH counts, exactly as it does above — a page whose
|
||||
# push tally is unreadable has not established the retarget tally either.
|
||||
p=$(printf '%s' "$raw" | jq -r '[.[] | select(.type == "pull_push")] | length')
|
||||
case "$p" in ''|*[!0-9]*) return 0 ;; esac
|
||||
total=$(( total + m ))
|
||||
ptotal=$(( ptotal + p ))
|
||||
page=$(( page + 1 ))
|
||||
done
|
||||
# THE CAP IS THE TERMINATOR, AND THE LAST PAGE READ MUST BE EMPTY (ersatztv#870).
|
||||
# Falling out of the loop on a NON-empty page 20 means the timeline may extend past the
|
||||
# offsets this walk covered, so it is "could not establish" and leaves `rt_ok=no` — the
|
||||
# same treatment an unreadable page gets, and the same rule the status walk states for
|
||||
# itself. That is also why the confirmable bound is 950 rows and not 1000: the 20th
|
||||
# request has to come back empty for the walk to be validated.
|
||||
if [ "$empty" = yes ]; then rt_ok=yes; rt_count=$total; hp_count=$ptotal; fi
|
||||
return 0
|
||||
}
|
||||
# PAGE THE PER-POST STATUS HISTORY TO A VALIDATED TERMINATOR (ersatztv#763).
|
||||
@@ -1823,7 +1953,7 @@ jobs:
|
||||
# nothing to buy by writing a value this run already knows was computed against a base the
|
||||
# PR no longer targets. One rule, one direction, nothing to reason about per state.
|
||||
#
|
||||
# An UNTRUSTED count on either side (`rt_ok=no`: paging never reached a validated empty
|
||||
# An UNTRUSTED count on either side (`rt_ok=no`: the walk did not reach its cap on an empty
|
||||
# page, a page was unreadable, the cap was hit) is NOT treated as "no retarget". It blocks
|
||||
# the exemption `success` only, and lets `pending` through — because a `success` written on a
|
||||
# count we could not verify is exactly the forged-green outcome this fence exists to
|
||||
@@ -2338,7 +2468,10 @@ jobs:
|
||||
# WHAT THAT BUYS IS TRANSIENT INSTEAD OF PERMANENT, not "the green never stands" — which is
|
||||
# what this comment claimed until round-2 review. The `success` is live between its POST
|
||||
# and the repair below, including the timeline round trips in between, so branch protection
|
||||
# or an already-scheduled auto-merge can observe it. Closing THAT window needs a
|
||||
# or an already-scheduled auto-merge can observe it. THAT WINDOW GREW WITH #870 and the
|
||||
# sentence is dated accordingly: the walk in between is now 20 requests rather than ~2,
|
||||
# about +2.3s on a healthy run at the measured 0.13s per request, and bounded by the
|
||||
# pessimum above rather than by anything tighter. Closing THAT window needs a
|
||||
# compare-and-set or serialization the API does not offer; what is removed is the case
|
||||
# where no event remained to correct it.
|
||||
#
|
||||
|
||||
+25
-10
@@ -1698,8 +1698,9 @@ both ends, which is how the forged exemption was obtained in the first place. Ab
|
||||
a PR, because every retarget fires `edited` — the event that makes one run abstain has already queued
|
||||
its successor.
|
||||
|
||||
If the count can't be established (unreadable timeline, paging that never reached a validated empty
|
||||
page), only the exemption `success` is withheld; `pending` still posts, since it blocks the merge
|
||||
If the count can't be established (unreadable timeline, an empty FIRST page, or paging that reached
|
||||
the 20-page cap on a NON-empty one — since #870 an empty page before the cap no longer ends the walk),
|
||||
only the exemption `success` is withheld; `pending` still posts, since it blocks the merge
|
||||
immediately and withholding it would strand ordinary PRs whenever the timeline is unreadable. That is
|
||||
the right trade but not a free one — a generic `pending` still masks a rejection landing in its own
|
||||
write window. What made that durable is gone: since ersatztv#849 the post-write check runs after
|
||||
@@ -2019,14 +2020,28 @@ Two things kept it invisible, and both are worth generalising:
|
||||
fidelity, that claim is a test assertion and needs re-measuring like any other.
|
||||
|
||||
The type is now read as a value (`case` over `jq -r 'type'`) rather than through `jq -e`, whose
|
||||
exit-status semantics already bit this workflow once at jq 1.6, and both `null` and `[]` terminate the
|
||||
walk. The regression test is parameterised over both shapes because both are live on this server.
|
||||
An empty page — `null` OR `[]`, which take the SAME rule since ersatztv#803 — is accepted as
|
||||
exhaustion only from **page 2 on** — every real PR's first page carries events
|
||||
(spot-checked non-empty across #752/#753/#749/#739/#717; the counts are deliberately not recorded here
|
||||
because timelines grow and such figures go stale within days), so an empty first
|
||||
page is ANOMALOUS rather than EXHAUSTED, and the walk should not certify "no retarget happened" from a
|
||||
response it cannot explain.
|
||||
exit-status semantics already bit this workflow once at jq 1.6, and both `null` and `[]` are read as
|
||||
the same EMPTY shape. The regression test is parameterised over both because both are live on this
|
||||
server. An empty page — `null` OR `[]`, which take the SAME rule since ersatztv#803 — is **no longer
|
||||
accepted as exhaustion before the page cap** (ersatztv#870, 2026-08-30). Gitea pages this endpoint at the database level and
|
||||
filters afterwards, so a page whose 50 rows are all inline-code comments serializes as exactly the
|
||||
value the walk used to trust as the end of the list, while later pages still hold events — 50 comments,
|
||||
which a PR author can create on their own PR, bought silence on both axes. The walk now SKIPS an empty
|
||||
page before the cap, reads every page up to its 20-page cap, and trusts the counts only when the LAST
|
||||
page it read came back empty — the same rule the status walk above states for the CAP, though not for termination: that walk still returns on its first empty page, and whether it needs the same change is tracked at ersatztv#893 rather than assumed (measured 2026-08-30, `/statuses/{sha}` returns a TRUE `X-Total-Count`, which this endpoint does not). **That
|
||||
moves the inference to the cap rather than removing it, and the residual is real**: pages 1-19 cover
|
||||
offsets 0..949, so a timeline of more than 1000 rows whose offsets 950..999 are all filtered still
|
||||
returns an empty page 20, and the walk publishes a too-low count as trusted. Both re-reads share the
|
||||
blind spot, so this is the same defeat at about 10x the price — the 50-row filtered block is the
|
||||
same, but the timeline it must sit in grows from ~100 rows to over 1000, and on their own PR the
|
||||
padding is the attacker's to supply — not a closed hole. It is NOT the status walk's over-cap residual: `/statuses/{sha}` does not
|
||||
filter after paging, so only here can a page-20 response be empty while later pages hold rows. An empty
|
||||
FIRST page still ends the walk untrusted: every real PR's first page carries events (spot-checked
|
||||
non-empty across #752/#753/#749/#739/#717; the counts are deliberately not recorded here because
|
||||
timelines grow and such figures go stale within days), so an empty first page is ANOMALOUS rather than
|
||||
EXHAUSTED, and the walk should not certify "no retarget happened" from a response it cannot explain.
|
||||
`X-Total-Count` cannot shortcut this: on this endpoint it is the post-filter length of the PAGE, not a
|
||||
total (`?limit=1` returns 1 on a 14-row timeline), unlike `/activities/feeds`, which does return one.
|
||||
|
||||
**The same nil-slice shape bites `/commits/{sha}/status`** — a third instance. A head with no statuses yet returns
|
||||
`{"state":"pending","total_count":0,"statuses":null}` (measured on PR #739's head). `read_existing_verdict`
|
||||
|
||||
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
@@ -7,7 +7,7 @@ supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'A `MUTATION` row in `docs/guard-inventory.md` is not a statement that someone once witnessed a red. It carries a DECLARED clause mutation in `scripts/tests/mutation_manifest.py`, and `scripts/tests/test_mutation_harness.py` applies that mutation to an isolated copy of the repository on every run and requires the row''s OWN named test to go red. The manifest and the MUTATION rows are compared for SET EQUALITY in both directions, so a row cannot claim the grade without a mutation and a mutation cannot outlive the grade it justifies. EXIT STATUS IS NOT THE VERDICT: each entry also declares the DIAGNOSTIC its red must carry, matched against pytest''s exception output alone, because pytest reports a crashing test exactly as it reports a detecting one and a red for an unrelated reason is evidence about nothing. WHERE THE GUARD IS ITSELF A TEST, `target` may differ from `guard` and the exact-once check applies to the declared TARGET. Two shapes are admissible and the choice is not free. Where the guard''s assertion IS the check — a completeness comparison against a Markdown inventory — the mutation goes into the guarded ARTIFACT, per `testing.guard-ships-with-mutation-proof`''s checker-guard exception, because mutating such a checker''s own POPULATION demonstrates a false POSITIVE while proving nothing about the detection the row claims. Where the guard is a test module wrapping a separately mutable DETECTOR or helper, the clause may be in that detector, since disarming it is a real clause disarm and the module''s own assertion is what notices. THE MUTATION IS DECLARED, NEVER INFERRED: a harness that guessed which clause of a 90-line hook is the guard would manufacture the confident-but-empty coverage this exists to prevent, which is why `testing.guard-ships-with-mutation-proof` rejected a generic runner. Where a proof test already names its clause in source, the manifest reuses THAT string, so a retarget in either place is caught by the other. COARSENESS IS RECORDED, NOT HIDDEN: each entry is graded `CLAUSE` or `DETECTOR`, and a `DETECTOR` entry — one whose detector accumulates faults from independent arms, so disarming any single arm leaves its proof test green — must CARRY the finer mutation that survived, which is re-run every time and required to keep surviving. Guards that are not graded `MUTATION` each carry a STATED reason in that same manifest, keyed on the guard and compared for SET EQUALITY against the inventory''s `GUARD` rows in both directions — so a new guard cannot arrive without someone writing what a proof would need, and a reason cannot outlive the row it is about. Keying the reason on the row''s GRADE instead is tautological (a new guard inherits one and nobody looks at it) and a pinned COUNT moves only on net change; both were tried and are rejected. The sandbox is a real git repository built from `git ls-files` with working-tree content, never a filesystem walk.'
|
||||
signals: 'mutation harness · declared clause not inferred · MUTATION row is executed · set equality manifest vs inventory · CLAUSE vs DETECTOR granularity · surviving finer mutation is re-run · pytest exit code 1 is the only red that counts · every other exit status is rejected · positive control before any mutation · sandbox from git ls-files · paths: `scripts/tests/test_mutation_harness.py`, `scripts/tests/mutation_manifest.py`, `scripts/tests/mutation_harness_lib.py`, `docs/guard-inventory.md` · issues: #790, #775, #774, #778, #806'
|
||||
mechanics: 'The harness builds ONE sandbox per session — tracked files only, from `git ls-files -s`, in an environment with every `GIT_*` variable stripped so an exported `GIT_DIR`/`GIT_COMMON_DIR`/`GIT_CONFIG_*` cannot point its `init`/`add`/`commit` at the real repository — and resets it between mutations with `git reset --hard` TO A BASELINE COMMIT RECORDED OUTSIDE THE REPOSITORY, plus `git clean -qffdx`. Both halves matter: bare `git reset --hard` resets to whatever HEAD is, so a proof test that COMMITS inside the sandbox moves HEAD onto a commit carrying the mutant and every later reset restores it faithfully; and a baseline held as a REF inside the sandbox is one more thing a proof can move. `core.hooksPath`, `commit.gpgsign` and `core.worktree` are pinned on every git invocation for the same reason. The `-ff` removes a nested repository a proof may have left. It runs the full set of named proof tests UNMUTATED first, inside the fixture rather than as a separate test so ordering is a dependency and not a convention. Only pytest exit code 1 counts as red; every other status is rejected. That is what closes the two ways a proof ref goes stale, and they are measured rather than assumed: with an explicit `file.py::function` node id a missing file and a missing function both exit 4, while 5 needs a collection that succeeded and selected nothing. The sandbox skips the `ErsatzTV-macOS` gitlink and recreates the `.claude/skills/jellyfin` symlink as a symlink. Cost: ~14s, against a ~4min `script-tests` suite.'
|
||||
mechanics: 'The harness builds ONE sandbox per session — tracked files only, from `git ls-files -s`, in an environment with every `GIT_*` variable stripped so an exported `GIT_DIR`/`GIT_COMMON_DIR`/`GIT_CONFIG_*` cannot point its `init`/`add`/`commit` at the real repository — and resets it between mutations with `git reset --hard` TO A BASELINE COMMIT RECORDED OUTSIDE THE REPOSITORY, plus `git clean -qffdx`. Both halves matter: bare `git reset --hard` resets to whatever HEAD is, so a proof test that COMMITS inside the sandbox moves HEAD onto a commit carrying the mutant and every later reset restores it faithfully; and a baseline held as a REF inside the sandbox is one more thing a proof can move. `core.hooksPath`, `commit.gpgsign` and `core.worktree` are pinned on every git invocation for the same reason. The `-ff` removes a nested repository a proof may have left. It runs the full set of named proof tests UNMUTATED first, inside the fixture rather than as a separate test so ordering is a dependency and not a convention. Only pytest exit code 1 counts as red; every other status is rejected. That is what closes the two ways a proof ref goes stale, and they are measured rather than assumed: with an explicit `file.py::function` node id a missing file and a missing function both exit 4, while 5 needs a collection that succeeded and selected nothing. The sandbox skips the `ErsatzTV-macOS` gitlink and recreates the `.claude/skills/jellyfin` symlink as a symlink. Cost: ~14s. The ~4min `script-tests` suite it was measured against is PRE-#870 and no longer holds: that change made `count_pr_mutations` read to its 20-page cap, and `test_pr_changed_files.py` went from 202s to 474s locally (286 -> 294 tests, same machine, `-p no:randomly`, 528383cf3 vs the #870 branch), so the job is roughly double. Re-measure on a runner before quoting a job figure.'
|
||||
---
|
||||
|
||||
**What #775 left open, in its own words: "nothing checks that a row claiming `MUTATION` is telling
|
||||
|
||||
@@ -97,7 +97,7 @@ classifications differ; otherwise the strictest applies and the Note names the e
|
||||
| Site | Class | Note |
|
||||
|---|---|---|
|
||||
| `scripts/post-review-verdict.sh` — commit-status write | `PINNED` | Re-reads the PR and compares **both** `.head.sha` and `.base.ref` immediately before the POST, and `die`s (exit 1, no status written) on a mismatch **or on a field it cannot read**. That last clause is load-bearing: guarding both comparisons with `[ -n "$x" ] &&` alone makes a well-formed 2xx body that merely omits the field a no-op, and the status is posted having confirmed nothing. Regression-tested against the real predecessor (#778), since the redundant-looking `-z` arm alone mutates green. Closes #706 and #632 for this path by read-compare-refuse, not by CAS: Gitea's status API offers no conditional write. Residual closed on the write ORDER since #792: the status is written first and the comment second, so a refusal can no longer leave a verdict comment with no status behind it — the only reachable half-state is a status with no comment, which the merge hook resolves as `ask` (`release.verdict-writes-status-before-comment`). Since #845 the status is also READ BACK after the POST and refused unless it is the write just made, and — for a `success` only, mirroring the gate's own asymmetry — unless its `.creator.login` is on the allow-list derived from the gate (a `failure` is inherited from any attributable account, so requiring membership there would refuse a rejection the gate honours) — the same read-compare-refuse shape, applied to WHOSE verdict it is rather than to which commit it covers. That read is check-then-act too and is graded with this row rather than separately: it can only withhold this tool's success report, never authorize anything, and the residual it leaves — a non-inheritable status standing with no comment beside it — is the `ask` half-state above. Two properties of that read are recorded because neither is visible in the response. `GET /commits/{sha}/status` PAGES and its `total_count` reports the PAGE rather than the total (measured Gitea 1.27.1, 2026-08-29 — `?limit=2` on a 15-context head returned 2 rows and `total_count: 2`), so a truncated body is indistinguishable from a complete one. That looked like a risk of the verdict row falling off a page and is NOT, because of the second property: the endpoint selects each context's MAX row id and orders those DESCENDING before paginating (same measurement — ids `[17,19,...,41,43]`, and `?limit=2` returns 41 and 43, the two highest). The row this tool has just POSTed is the newest on the head, so it sorts first and page 1 holds it at any page size. `?limit=100` (the instance clamps `limit` to `MAX_RESPONSE_ITEMS`, measured at 50) is kept as insurance against a burst of concurrent status writes and against that ordering changing, and has no observable effect otherwise — stated so it is not mistaken for an untested guard. A row it cannot find is therefore refused with the row count it read and no stated cause; an earlier version of this row named truncation as the candidate cause, before the ordering was measured. |
|
||||
| `scripts/pr-changed-files.sh` — paged file enumeration | `UNSAFE-KNOWN` | #707's fix: `.base.ref`, `.base.sha` and `.head.sha` are captured before paging and re-checked after, and any *observed* movement fails the whole enumeration closed rather than emitting a short list. But before-and-after equality is **ABA-vulnerable** — a `main → scratch → main` retarget during paging can return the same ref and, if nothing merged meanwhile, the same base sha, while the pages in between were diffed against the scratch base. The script's own comment says it narrows rather than erases — "any movement fails" would be stronger than the code. Accepted here because the enumerator cannot close it alone, but be exact about what the caller-side fence does and does not cover: `ci.verdict-write-retarget-fence` counts `change_target_branch` events, so it catches the BASE alias and **nothing else**. A HEAD alias — a force-push `H1 -> H2 -> H1` during pagination — leaves the final `.head.sha` comparison equal while the middle pages were enumerated against `H2`, and NOTHING inside this script can see it. **Fenced at the enforced caller since #803** (2026-08-28): the same timeline walk now also counts `pull_push`, so a head that moved at all during classification withholds the write — subject to that walk's own terminator, which is defeatable by a page of filtered rows (**#870**, pre-existing and equally affecting the base axis). The grade stays `UNSAFE-KNOWN` for the SCRIPT, because the script's own guarantee is unchanged and one-way — a caller that does not run the fence inherits the alias. Enumeration bound to an immutable tree remains unavailable: re-probed at 1.27.1 on 2026-08-28, `compare/{base}...{head}` still returns no `files` key. |
|
||||
| `scripts/pr-changed-files.sh` — paged file enumeration | `UNSAFE-KNOWN` | #707's fix: `.base.ref`, `.base.sha` and `.head.sha` are captured before paging and re-checked after, and any *observed* movement fails the whole enumeration closed rather than emitting a short list. But before-and-after equality is **ABA-vulnerable** — a `main → scratch → main` retarget during paging can return the same ref and, if nothing merged meanwhile, the same base sha, while the pages in between were diffed against the scratch base. The script's own comment says it narrows rather than erases — "any movement fails" would be stronger than the code. Accepted here because the enumerator cannot close it alone, but be exact about what the caller-side fence does and does not cover: `ci.verdict-write-retarget-fence` counts `change_target_branch` events, so it catches the BASE alias and **nothing else**. A HEAD alias — a force-push `H1 -> H2 -> H1` during pagination — leaves the final `.head.sha` comparison equal while the middle pages were enumerated against `H2`, and NOTHING inside this script can see it. **Fenced at the enforced caller since #803** (2026-08-28): the same timeline walk now also counts `pull_push`, so a head that moved at all during classification withholds the write — subject to that walk's own terminator, which is defeatable by a page of filtered rows — **NARROWED, not closed, by #870** (2026-08-30): the walk no longer reads an empty page BEFORE the cap as the end of the list, but reads to its 20-page cap and requires the last page to be empty, so the same defeat now needs a timeline of more than 1000 rows rather than ~100, with the same 50-row filtered block pinned to offsets 950..999 — about 10x the price, the padding beneath the block being the attacker's to supply on their own PR. A cap filled with READABLE pages fails closed; a cap hidden behind a filtered block still certifies a too-low count. The grade stays `UNSAFE-KNOWN` for the SCRIPT, because the script's own guarantee is unchanged and one-way — a caller that does not run the fence inherits the alias. Enumeration bound to an immutable tree remains unavailable: re-probed at 1.27.1 on 2026-08-28, `compare/{base}...{head}` still returns no `files` key. |
|
||||
| `scripts/select-queue.sh` — issue list, then per-issue `/dependencies` | `UNSAFE-KNOWN` | The open-issue list (labels, milestone, priority) is snapshotted once; per-candidate dependency reads happen seconds later and never re-read the issue's own labels, so an issue claimed `in-progress` in that gap still appears on the shortlist. Accepted: the script authorizes **no write**. The real gate is the four-way claim check in `process.parallel-session-claim`, which runs after selection and re-reads live state by construction. Tightening this would move a check that must be adversarial into a tool that is advisory. |
|
||||
| `scripts/ci-detect-already-validated.sh` — prior-head combined status | `UNSAFE-KNOWN` | Reads the PR head's status and emits `skip=true`, with nothing re-checking before the consuming job runs. Accepted and narrow: the skip elides only **re-running** test/migrations on a tree already validated; the `build` job still builds and pushes unconditionally, so no image ever ships from unvalidated source. |
|
||||
| `scripts/issue-qualification-audit.sh` — paged issue list, then a report | `UNSAFE-KNOWN` | Pages the open-issue list and reports which issues lack a `priority:` label, so like every paged read here its pages can straddle a change and the report can name a state no single instant held. Graded to match `select-queue.sh` rather than `N/A`: the two run the same shape, and the reason offered for accepting `select-queue.sh` — it authorizes no write — cannot simultaneously be the reason this one is out of the class. Accepted on the same terms: it is advisory, session-end, human-read, and the labels it prompts for are applied by hand afterwards. |
|
||||
|
||||
@@ -1012,6 +1012,35 @@ if "/timeline" in url:
|
||||
for part in url.split("?", 1)[-1].split("&"):
|
||||
if part.startswith("page="):
|
||||
page = int(part.split("=", 1)[1])
|
||||
flaky = os.environ.get("STUB_TIMELINE_FLAKY_FIRST", "")
|
||||
if flaky:
|
||||
# ONE TRANSIENT BLIP, on the very first timeline request of the run (ersatztv#870 review).
|
||||
# This is the only arrangement that separates a walk that retries from one that does not:
|
||||
# `transport-error` and `unreadable` fail EVERY attempt, so a retry changes nothing and both
|
||||
# implementations refuse identically. Counting REQUESTS, not pages, because the retry is
|
||||
# per-request — `timeline_reads.txt` bumps once per walk and cannot see a second attempt.
|
||||
#
|
||||
# TWO BLIP SHAPES, because they reach DIFFERENT arms of the retry and only one was pinned.
|
||||
# `transport` exits non-zero with no body, so `raw` is empty and `kind` is never computed.
|
||||
# `body` returns 200 with a well-formed JSON OBJECT — which is what this Gitea really sends
|
||||
# on an error, measured: `?since=NOTATIME` returns an object, not an array — so `jq -r type`
|
||||
# SUCCEEDS and sets `kind=object`.
|
||||
#
|
||||
# That difference is the whole point of the second shape. An unreadable body (a 502 HTML
|
||||
# page) would NOT do: jq fails on it, `|| kind=""` fires, and the walk takes the same path
|
||||
# as `transport`. Only a parseable non-array body distinguishes "break out of the retry on
|
||||
# anything readable" from "break only on a shape the `case` below accepts", and that
|
||||
# narrowing is a real fail-open — an error object would be read as the walk's terminator
|
||||
# verdict instead of being retried. The first draft of this fixture used the 502 page and
|
||||
# the mutant passed it.
|
||||
rctr = out / "timeline_requests.txt"
|
||||
rseen = int(rctr.read_text()) if rctr.exists() else 0
|
||||
rctr.write_text(str(rseen + 1))
|
||||
if rseen == 0:
|
||||
if flaky == "body":
|
||||
print(json.dumps({"message": "internal server error", "url": "http://gitea"}))
|
||||
sys.exit(0)
|
||||
sys.exit(22)
|
||||
if mode == "empty-first-page":
|
||||
# A terminator on PAGE 1 — no real page ever read (ersatztv#751). The job must NOT certify a
|
||||
# zero retarget count from this, so the exemption is withheld.
|
||||
@@ -1087,6 +1116,25 @@ if "/timeline" in url:
|
||||
# than simply corrected: `/issues/{n}/comments` really does return `[]` when empty, so both
|
||||
# shapes are live on this server and the job must accept either.
|
||||
pages = int(os.environ.get("STUB_TIMELINE_PAGES", "1"))
|
||||
# A FILTERED INTERMEDIATE PAGE (ersatztv#870). This is the shape the double could not produce
|
||||
# before, and it is not "a page that happens to be empty": the endpoint pages at the DATABASE
|
||||
# level and filters AFTERWARDS, so a page whose 50 rows are all `CommentTypeCode` serializes as
|
||||
# the SAME `null` a page past the end does, while LATER pages still hold events. Serving it from
|
||||
# the terminator env var rather than a literal is deliberate — the two shapes are
|
||||
# indistinguishable to the walk on the wire, which is the entire defect, so a double that made
|
||||
# them distinguishable would test a server we do not have.
|
||||
#
|
||||
# It exits BEFORE the last-real-page branch, so it does not touch `timeline_reads.txt`. Be exact
|
||||
# about what that counter is, because the comment above it is not: it increments ONLY on the
|
||||
# last-real-page branch, so it counts WALKS (one bump each, at the walk's last real page), not
|
||||
# pages served. A filtered page must not bump it either way, or the `moves:`/`stable:` sequences
|
||||
# would advance a walk early and address the wrong element.
|
||||
filtered = {
|
||||
int(x) for x in os.environ.get("STUB_TIMELINE_FILTERED_PAGES", "").split(",") if x.strip()
|
||||
}
|
||||
if page in filtered:
|
||||
print(os.environ.get("STUB_TIMELINE_TERMINATOR", "null"))
|
||||
sys.exit(0)
|
||||
if page > pages:
|
||||
print(os.environ.get("STUB_TIMELINE_TERMINATOR", "null"))
|
||||
sys.exit(0)
|
||||
@@ -1770,6 +1818,8 @@ def _run_classify(
|
||||
timeline_mode: str = "none",
|
||||
push_mode: str = "none",
|
||||
timeline_pages: int = 1,
|
||||
timeline_filtered_pages: str = "",
|
||||
timeline_flaky_first: str = "",
|
||||
history_mode: str = "none",
|
||||
history_creator: str = "timothy",
|
||||
history_extra: list | None = None,
|
||||
@@ -1815,6 +1865,8 @@ def _run_classify(
|
||||
env["STUB_TIMELINE_MODE"] = timeline_mode
|
||||
env["STUB_PUSH_MODE"] = push_mode
|
||||
env["STUB_TIMELINE_PAGES"] = str(timeline_pages)
|
||||
env["STUB_TIMELINE_FILTERED_PAGES"] = timeline_filtered_pages
|
||||
env["STUB_TIMELINE_FLAKY_FIRST"] = timeline_flaky_first
|
||||
env["STUB_TIMELINE_TERMINATOR"] = timeline_terminator
|
||||
env["STUB_STATUS_EMPTY_SHAPE"] = status_empty_shape
|
||||
env["STUB_HISTORY_MODE"] = history_mode
|
||||
@@ -3530,6 +3582,206 @@ def test_positive_control_a_MULTI_PAGE_quiet_timeline_still_exempts(tmp_path):
|
||||
)
|
||||
|
||||
|
||||
# THE FILTERED-INTERMEDIATE-PAGE DEFEAT (ersatztv#870), on both axes.
|
||||
#
|
||||
# The walk used to read an empty page past page 1 as proof of exhaustion. Gitea does not mean that:
|
||||
# `ListIssueCommentsAndTimeline` applies the LIMIT/OFFSET in `FindComments` and drops
|
||||
# `CommentTypeCode` rows and inaccessible cross-references AFTERWARDS, into a nil slice that
|
||||
# serializes as bare `null`. So a page of 50 inline review comments — which a PR author can create on
|
||||
# their own PR — comes back looking exactly like the end of the list while later pages still hold
|
||||
# events, and rows are ASCENDING, so the events a fence looks for are the furthest from page 1.
|
||||
#
|
||||
# Both of these fixtures are the same construction with a different event on the far page, because
|
||||
# the defeat is not axis-specific: one walk publishes both counts. Covering only the head axis would
|
||||
# have repeated `process.fix-one-path-then-check-its-twin`.
|
||||
|
||||
|
||||
@pytest.mark.parametrize("terminator", ["null", "[]"])
|
||||
def test_a_FILTERED_intermediate_page_does_not_END_the_walk_head_axis(tmp_path, terminator):
|
||||
"""A page that is empty because it was FILTERED must not certify the count. Head axis.
|
||||
|
||||
Page 1 is full filler, page 2 comes back empty, page 3 carries the pushes. A walk that stops at
|
||||
page 2 certifies a zero push count on both reads, so the two agree, the sha comparison agrees,
|
||||
and the ABA is exempted — which is precisely the attack: 50 inline comments buy silence.
|
||||
|
||||
Parameterised over both empty shapes because the walk has to treat them identically here; the
|
||||
`null`-only fixture would leave the `[]` path free to keep the old early return.
|
||||
"""
|
||||
posted, r = _run_classify(
|
||||
tmp_path,
|
||||
_emitting("docs/a.md"),
|
||||
timeline_mode="none",
|
||||
push_mode="moves:0,2",
|
||||
timeline_pages=3,
|
||||
timeline_filtered_pages="2",
|
||||
timeline_terminator=terminator,
|
||||
)
|
||||
assert posted is None, (
|
||||
f"an exemption was granted over an ABA hidden behind a filtered page 2 (`{terminator}`) — "
|
||||
f"the walk is reading a filtered page as the end of the timeline. Log:\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert "head branch was pushed while this job was classifying" in r.stdout, (
|
||||
f"nothing was posted, but NOT via the head fence. Log:\n{r.stdout[-900:]}"
|
||||
)
|
||||
|
||||
|
||||
def test_a_FILTERED_intermediate_page_does_not_END_the_walk_base_axis(tmp_path):
|
||||
"""The twin: the same construction hides a RETARGET rather than a push.
|
||||
|
||||
Same page layout, retargets on the far page. The base axis is the older of the two fences and
|
||||
reads the same `rt_ok`/`rt_count` pair, so a fix that only reached the head arm would leave the
|
||||
original #706 fence defeated by a construction its own test suite never built.
|
||||
"""
|
||||
posted, r = _run_classify(
|
||||
tmp_path,
|
||||
_emitting("docs/a.md"),
|
||||
timeline_mode="moves:0,1",
|
||||
timeline_pages=3,
|
||||
timeline_filtered_pages="2",
|
||||
)
|
||||
assert posted is None, (
|
||||
f"an exemption was granted over a retarget hidden behind a filtered page 2. Log:\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert "retargeted while this job was classifying" in r.stdout, (
|
||||
f"nothing was posted, but NOT via the base fence. Log:\n{r.stdout[-900:]}"
|
||||
)
|
||||
|
||||
|
||||
def test_positive_control_a_FILTERED_intermediate_page_on_a_QUIET_pr_still_exempts(tmp_path):
|
||||
"""Without this, the two tests above pass against a walk that refuses every filtered page.
|
||||
|
||||
Refusing outright would be safe and useless: every PR carrying inline review comments would lose
|
||||
its exemption. The walk must SKIP the empty page and keep counting, which is only visible when a
|
||||
quiet PR with the same page layout still gets its `success`.
|
||||
"""
|
||||
posted, r = _run_classify(
|
||||
tmp_path,
|
||||
_emitting("docs/a.md"),
|
||||
timeline_mode="none",
|
||||
push_mode="stable:2",
|
||||
timeline_pages=3,
|
||||
timeline_filtered_pages="2",
|
||||
)
|
||||
assert posted is not None and posted["state"] == "success", (
|
||||
f"a quiet PR lost its exemption because a page of its timeline was filtered. Log:\n{r.stdout[-900:]}"
|
||||
)
|
||||
|
||||
|
||||
def test_a_timeline_that_FILLS_the_page_cap_is_untrusted(tmp_path):
|
||||
"""The cap is not exhaustion, and the LAST page read has to be the empty one.
|
||||
|
||||
Now that an empty page no longer ends the walk, the trust decision moved to after the loop — and
|
||||
the whole of it rests on that decision still being conditional. Making it unconditional (a bare
|
||||
`rt_ok=yes` once the loop finishes) would certify a partial count over an over-long timeline,
|
||||
which is the failure the sibling status walk already refuses by the same rule. 25 real pages
|
||||
means the walk never sees an empty one, so it must report "could not establish" and withhold.
|
||||
"""
|
||||
posted, r = _run_classify(
|
||||
tmp_path,
|
||||
_emitting("docs/a.md"),
|
||||
timeline_mode="none",
|
||||
push_mode="stable:0",
|
||||
timeline_pages=25,
|
||||
)
|
||||
# NOT `posted is None`. An untrusted count on the exemption path does not merely stay silent —
|
||||
# since ersatztv#849 it replaces the unknown state with the sticky unverified-write sentinel, so
|
||||
# something IS posted and the assertion has to discriminate on the STATE. Asserting silence here
|
||||
# passed a `pending` sentinel off as a failure and would have gone red on correct behaviour.
|
||||
assert posted is None or posted["state"] != "success", (
|
||||
"an exemption was granted over a timeline that never terminated inside the page cap — the "
|
||||
f"post-loop trust decision is unconditional. Log:\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert "Could not establish a trusted retarget/push count" in r.stdout, (
|
||||
"no exemption was granted, but NOT because the count was untrusted — this test is not "
|
||||
f"measuring the page cap. Log:\n{r.stdout[-900:]}"
|
||||
)
|
||||
|
||||
|
||||
def test_a_FILTERED_page_before_a_FULL_cap_page_is_still_untrusted(tmp_path):
|
||||
"""A filtered page must not survive as trust when the walk then runs into the cap.
|
||||
|
||||
Page 19 comes back empty (filtered), page 20 is full. The walk must finish `empty=no` and refuse.
|
||||
This is what makes the per-iteration `empty=no` reset load-bearing in the direction the other
|
||||
fixtures cannot see: they all END on an empty page, so a stale flag and a correct one agree
|
||||
there. Here they disagree — without the reset the flag is still `yes` from page 19 when the loop
|
||||
falls out, and line `if [ "$empty" = yes ]` would certify a walk that never reached a terminator.
|
||||
|
||||
(The reset is ALSO covered from the other side: deleting it makes the three filtered-page tests
|
||||
above go red, because a stale `yes` suppresses the tally on every later non-empty page and the
|
||||
hidden events stop being counted. Measured, not assumed — an early review round predicted those
|
||||
three would stay green.)
|
||||
"""
|
||||
posted, r = _run_classify(
|
||||
tmp_path,
|
||||
_emitting("docs/a.md"),
|
||||
timeline_mode="none",
|
||||
push_mode="stable:0",
|
||||
timeline_pages=25,
|
||||
timeline_filtered_pages="19",
|
||||
)
|
||||
assert posted is None or posted["state"] != "success", (
|
||||
"an exemption was granted over a walk that hit the page cap on a NON-empty page after a "
|
||||
f"filtered one — the empty flag is stale. Log:\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert "Could not establish a trusted retarget/push count" in r.stdout, (
|
||||
f"nothing was exempted, but NOT because the count was untrusted. Log:\n{r.stdout[-900:]}"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("blip", ["transport", "body"])
|
||||
def test_a_SINGLE_transient_blip_does_not_cost_the_exemption(tmp_path, blip):
|
||||
"""The walk retries each page once, as `page_statuses` does and for its stated reason.
|
||||
|
||||
This walk had no retry while it made ~2 requests per call. At a fixed 20 it makes 10x as many,
|
||||
so a blip is 10x likelier — and `rt_ok=no` on the exemption path writes the STICKY sentinel,
|
||||
which costs that head its exemption until a human clears it by hand. That is a regression this
|
||||
change would otherwise have shipped.
|
||||
|
||||
`transport-error` and `unreadable` cannot measure this: they fail every attempt, so a retrying
|
||||
walk and a non-retrying one refuse identically. Only a blip that clears on the second attempt
|
||||
separates them.
|
||||
|
||||
BOTH SHAPES, because they reach different arms and only one was pinned. `transport` leaves `raw`
|
||||
empty. `body` returns a well-formed JSON OBJECT — what this Gitea actually sends on an error
|
||||
(measured: `?since=NOTATIME` returns an object) — so `jq -r type` succeeds and `kind=object`.
|
||||
|
||||
That is what kills the narrowing `if [ -n "$kind" ]; then break; fi`, under which an error object
|
||||
ends the retry and is then refused by the `case`, costing the exemption. An UNREADABLE body does
|
||||
not kill it: jq fails, `kind=""`, and the walk takes `transport`'s path. The first version of
|
||||
this fixture used a 502 HTML page for exactly that reason and the mutant passed it — measured,
|
||||
not assumed.
|
||||
"""
|
||||
posted, r = _run_classify(
|
||||
tmp_path,
|
||||
_emitting("docs/a.md"),
|
||||
timeline_mode="none",
|
||||
push_mode="stable:0",
|
||||
timeline_flaky_first=blip,
|
||||
)
|
||||
assert posted is not None and posted["state"] == "success", (
|
||||
f"a docs-only PR lost its exemption to ONE transient timeline blip ({blip}) — the walk does "
|
||||
f"not retry this shape. Log:\n{r.stdout[-900:]}"
|
||||
)
|
||||
|
||||
|
||||
def test_the_timeline_read_is_TIME_BOUNDED(tmp_path):
|
||||
"""`gh` is bare `curl -sf` with no timeout, and this walk now issues 20 requests instead of ~2.
|
||||
|
||||
The third call site runs AFTER the exemption `success` is posted, so a hang there leaves the
|
||||
green standing with no repair attempted. The bound is applied at THIS call rather than inside
|
||||
`gh`, so the other call sites keep the behaviour they were reviewed with. Asserted structurally
|
||||
because a hang cannot be provoked through the stub — the stub answers instantly by construction.
|
||||
"""
|
||||
body = _classify_step()["run"]
|
||||
line = [ln for ln in body.splitlines() if "/timeline?limit=50&page=" in ln]
|
||||
assert len(line) == 1, f"expected exactly one timeline read, found {len(line)}: {line}"
|
||||
assert "--max-time" in line[0], (
|
||||
"the timeline read carries no --max-time; a hung request on the POST-WRITE walk leaves an "
|
||||
f"exemption success standing with no repair attempted. Line: {line[0]}"
|
||||
)
|
||||
assert "--connect-timeout" in line[0], f"the timeline read carries no --connect-timeout. Line: {line[0]}"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("terminator", ["null", "[]"])
|
||||
def test_an_EMPTY_FIRST_page_is_untrusted_WHICHEVER_empty_shape_it_is(tmp_path, terminator):
|
||||
"""Both empty shapes must fall through to `rt_ok=no`, and only one of them did.
|
||||
|
||||
Reference in New Issue
Block a user