fix(870): an empty timeline page is not exhaustion — the walk reads to its cap #896

Merged

timothy merged 4 commits from fix/870-timeline-walk-filtered-page into main 2026-08-30 13:29:39 +02:00

Conversation 1 Commits 4 Files Changed 7

+604 -126

4 Commits

Author	SHA1	Message	Date
timothy	eb9cc6cb31	fix(870): round-2 review — cost arithmetic was wrong twice, and one fixture proved nothing ... Cold re-review of the fix commit (Opus, isolated worktree; the cross-family arm was unavailable, Codex having hit its usage quota mid-session). No Blocker, no High on the code — it executed the walk and every mutant of it. Every finding was in prose, cost accounting, or a test that did not discriminate. All are addressed here. COST ARITHMETIC, WRONG TWICE, BOTH NOW MEASURED RATHER THAN REASONED. - Worst-case sleeps: the comment said 19, reasoning that the last page could not sleep. It can. Executing the walk against scripted responses gives 40 requests and 20 sleeps. - Wall clock: the comment said ~335s, on the premise that only the last page can pay two timeouts. 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 continues. The pessimum is 20x(15+1+15) = 620s per walk, ~21min across the exempt path's three, and the job has no `timeout-minutes`. Stated rather than tuned, because the direction is the justification: the predecessor had no timeout at all. And it is bounded HERE only — `page_statuses` is still unbounded and runs in the same post-POST window, so the "a hang leaves the green standing" mode remains reachable there. TWO COSTS THE CHANGE DID NOT STATE, now measured on this machine: - The suite roughly doubled. `test_pr_changed_files.py`: 202s -> 474s (286 -> 294 tests, `-p no:randomly`, 528383cf3 vs this branch), because every fence fixture now drives 20 stub requests per walk instead of 2. `testing.mutation-claims-are-executed` carried a `~4min script-tests` figure measured before this; it is dated there rather than left to rot. - The exemption `success` is live from its POST until the post-write repair, and the walk in between went from ~2 requests to 20. The comment documenting that window was written when it was two round trips. A FIXTURE THAT PROVED NOTHING, caught by running the mutant it claimed to kill. The retry's coverage of a non-array BODY was pinned by a fixture serving a 502 HTML page — but jq fails on that, `|| kind=""` fires, and it takes the same path as a transport error. Narrowing the retry to `if [ -n "$kind" ]` passed it. The shape that discriminates is a well-formed JSON OBJECT, which is what this Gitea actually returns on an error (measured: `?since=NOTATIME` returns an object). Under the narrowing an error object ends the retry and is then refused by the `case`, costing the exemption. Fixture corrected, mutant now red on `[body]` and green on `[transport]` — which is the point of parameterising over both. PROSE, the same class the previous round removed six times: - The workflow stated the attacker's cost two ways 45 lines apart ("50 rows before, ~1000 now" against "50 comments either way"). Neither was right: the 50-row filtered block is unchanged, 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. Stated once now, in one unit, in all five places. - The record's residual intro said the second residual "was closed by #870" directly above a heading reading "NARROWED ... not closed". - "No terminator on this endpoint can do better" dropped both of the record's hedges. It is not a proof that none could exist: `?limit=49&page=21` reaches offsets 980..1028 with page edges that do not coincide with a 50-row walk's. Strictly better, still not a terminator. - Retired vocabulary swept: two sites still described trust as "reached a validated empty page", and one listed "a non-array body" as untrusted three lines from the paragraph explaining that a bare `null` IS the accepted empty page. - The retry's break set and the `case` arms are two enumerations of {array, null} whose agreement is what makes the stale-`kind` path harmless. Nothing said they must stay in sync; now it does. DELIBERATELY NOT DONE: no `test_MUTATION_*`-named fixture for the new clauses. The convention is for clauses disarmed in place via `_run_classify(mutate=...)`; these are page-LAYOUT properties, covered by behavioural fixtures that were each mutation-witnessed red (7 of 7, independently re-measured by the reviewer). Naming them for the convention would not add coverage. Full suite green. Fixes #870 Refs: #803, #706, #664, #751, #893 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-30 12:17:07 +02:00
timothyandClaude Opus 5	425f6b4a31	fix(870): state BOTH cost bounds, not just the healthy one ... Executing the shipped walk against synthetic page sequences showed the cost sentence was telling half the truth: "20 requests per walk, measured at 2.6s" is the healthy path, but the per-page retry added in the previous commit makes the worst case 40 requests and 19 one-second sleeps. A reader sizing the change against the job's budget would have sized it against the better half. Measured by execution (the walk extracted from the YAML and run against scripted responses): a healthy walk is exactly 20 requests; an empty page 1 short-circuits at 1; a persistent failure stops at 2; a single blip costs 21. The retry's kind/raw correspondence holds on every path — a non-array first attempt followed by a failed second leaves the walk untrusted rather than parsing a stale body. Refs: #870 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-30 12:17:07 +02:00
timothy	f34801b51f	fix(870): review round — the fix NARROWS the defeat, and the prose said "closed" ... Two independent cold reviews (Codex GPT-5.6 cross-family; a fresh Opus agent in an isolated worktree, which executed the walk against synthetic page shapes). Both landed on the same substantive defect, and it was in the prose, not the code. THE OVERCLAIM. The fix moves the unprovable inference from page 2 to the page cap; it does not remove it. An empty page 20 is still trusted, for the same reason an empty page 2 used to be — this endpoint cannot distinguish a filtered page from the end of the list at ANY offset. Six sentences across three docs and the workflow said otherwise, including "no longer accepted as exhaustion at all", "STOP INFERRING THE END AT ALL", and — in the paragraph that describes the case where it fails OPEN — "an over-cap timeline fails CLOSED ... it does not pass silently". All rewritten to NARROWED, not closed, with the residual stated where a reader meets the claim. THE PRICE, IN ONE UNIT. "50 rows instead of 1000" compared an authored-comment count against a total-row count and flattered the change. Before: ~100 timeline rows (a non-empty page 1, then a 50-row filtered block at a 50-aligned offset). After: over 1000, with the block pinned to offsets 950..999. About 10x, and the attacker authors 50 inline comments either way. THE PARITY CLAIM WAS FALSE. "The two walks now agree" — they do not. `page_statuses` still returns on its FIRST empty page; what the two share is only the cap rule. Worse, the diff asserted parity it had not established: whether `/statuses/{sha}` is subject to the same post-pagination filtering was never checked. Measured instead of asserted (Gitea 1.27.1, 2026-08-30): that endpoint returns a TRUE `X-Total-Count` (105 at both `?limit=1` and `?limit=50`) where `/issues/{n}/timeline` returns the post-filter page length. Consistent with counting before filtering — evidence, not proof, since no filtering predicate has been exhibited either way. Filed as #893 rather than resolved here. CODE CHANGES, both answering the amplification this fix introduced (~2 requests per walk to a fixed 20, three walks per exempt run): - Each page is now bounded `--connect-timeout 5 --max-time 15`. `gh` is bare `curl -sf` with no timeout, and the THIRD call site runs after the exemption `success` is posted, so a hang leaves the green standing with no repair attempted. Bounded at this call rather than inside `gh`, so no other call site changes behaviour. - Each page is retried once, mirroring `page_statuses` and its stated reason — "one transient blip would cost that head its exemption until a human clears it by hand". That walk retries; this one did not, and it just took 10x the round trips. - The four jq reads carry `2>/dev/null || x=""`, as `page_statuses`'s do. Unreachable today thanks to the ordering above them, which is exactly the "safe because of a property three lines up" reasoning the adjacent comment says it is retiring. STALE CONTRACT TEXT swept: the function's own header still said "trusted ONLY when paging reached a validated EMPTY page" and listed a short page as untrusted (false before and after — a short page is tallied and the walk continues); the record's `mechanics:` field still described the old terminator and omitted the new fixtures. TESTS (+3, and one reviewer claim refuted): - `test_a_FILTERED_page_before_a_FULL_cap_page_is_still_untrusted` — filtered page 19, full page 20. This kills the "trust if ANY page was empty" mutant, which the Opus review proved survived the previous round's suite; every earlier fixture ends on an empty page, so a latching flag and a correct one agree there. - `test_a_SINGLE_transient_blip_does_not_cost_the_exemption`, with a `flaky-first` stub mode. The existing `transport-error`/`unreadable` modes fail EVERY attempt, so a retrying walk and a non-retrying one refuse identically and neither could measure this. - `test_the_timeline_read_is_TIME_BOUNDED` — structural; a hang cannot be provoked through a stub that answers instantly. Each was mutated and witnessed red. Codex claimed the per-iteration `empty=no` reset was unguarded and that deleting it would leave all four new tests green; deleting it turns THREE of them red — a stale `yes` also suppresses the tally on every later non-empty page. The Opus review mutated the same line independently and got the same red. Recorded because a wrong mechanism claim in a review is as executable as a right one. Full suite: 1354 passed, 2 skipped. Fixes #870 Refs: #803, #706, #664, #751, #893 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-30 12:17:07 +02:00
timothy	0d8ef4c81c	fix(870): an empty timeline page is not exhaustion — the walk reads to its cap ... `count_pr_mutations` in `review-verdict.yml` treated an empty page past page 1 as proof it had reached the end of the PR timeline. Gitea does not mean that. `ListIssueCommentsAndTimeline` applies the LIMIT/OFFSET in `FindComments` at the DATABASE level and filters AFTERWARDS, dropping every `CommentTypeCode` row and every inaccessible cross-reference into a nil slice that serializes as bare `null`. A page whose 50 rows are all inline review comments is therefore byte-identical to a page past the end, while later pages still hold events — and rows are ordered ASCENDING, so the events a fence looks for are the furthest from page 1. Fifty inline comments, which a PR author can create on their own PR, truncated both walks at the same place: both counts agreed, the sha comparison agreed, and an ABA force-push (`H1 -> H2 -> H1`) yielded an exemption `success` over a diff no single head justified. The walk no longer infers the end at all. Past page 1 an empty page is SKIPPED; the loop reads every page up to its pre-existing 20-page cap and the trust decision moves to after the loop — `rt_ok=yes` only when the LAST page read came back empty. That is the contract the sibling status walk already stated for itself ("the 20th request has to be the EMPTY terminator"), so the two now agree instead of one silently claiming coverage it did not perform. An empty FIRST page and any unreadable shape still end the walk untrusted. "Require K consecutive empty pages" was rejected: 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. Three measurements at Gitea 1.27.1 rule out the cheaper fixes, and are recorded because each is the kind of thing a later reader would otherwise re-propose: - `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, and a page past the end returns 0. `/activities/feeds` DOES return a true total (5739), so this is a property of the handler, not the server. - `limit` clamps to 50 here (`?limit=200` returned 50 on #303/#237/#91/#202). - The endpoint's only query parameters are `since`, `before`, `page`, `limit` — 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 costs the "a real page was actually read" property that an anomalous empty page 1 provides. Residual, stated rather than left in the issue: the walk confirms offsets 0..949 and requires its last page to be empty, so hiding an event now needs a timeline over 1000 rows AND a fully-filtered block at offsets 950..999 — ~1000 rows instead of 50, and the same over-long -history residual the status walk already accepts. An over-cap timeline fails CLOSED. Cost: a fixed 20 requests per walk instead of ~2, measured at 2.6s against the live instance. Tests: a new stub knob `STUB_TIMELINE_FILTERED_PAGES` serves a filtered intermediate page — the shape the double could not previously produce. Three new tests reproduce the defeat (head axis over both empty shapes, and its base-axis twin) and go RED against the shipped predecessor, granting `state: success` on the hidden ABA. A positive control keeps the walk from simply refusing every filtered page, and `test_a_timeline_that_FILLS_the_page_cap_is_untrusted` guards the new post-loop clause — making that trust unconditional reddens it. Full suite: 1351 passed, 2 skipped. Fixes #870 Refs: #803, #706, #664, #751 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-30 12:17:07 +02:00