fix(870): round-2 review — cost arithmetic was wrong twice, and one fixture proved nothing
Build ErsatzTV Image / CI toolchain image resolves (pull_request) Successful in 9s
Build ErsatzTV Image / Delimiter ban (release path) (pull_request) Successful in 25s
Review verdict / Set review-verdict status (pull_request_target) Successful in 18s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 11m27s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 8m7s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Skipped
review-verdict/h10 Review-verdict: MERGEABLE @ eb9cc6c (base: main)
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 8m59s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 9s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 5s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 10s
PR Gates / Docs update reminder (pull_request) Successful in 16s
PR Gates / decisions lifecycle (pull_request) Successful in 21s
PR Gates / Fix proofs (Proves trailers) (pull_request) Successful in 15s
PR Gates / Script lint and tests (ruff + pytest) (pull_request) Successful in 18m47s
Build ErsatzTV Image / CI toolchain image resolves (pull_request) Successful in 9s
Build ErsatzTV Image / Delimiter ban (release path) (pull_request) Successful in 25s
Review verdict / Set review-verdict status (pull_request_target) Successful in 18s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 11m27s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 8m7s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Skipped
review-verdict/h10 Review-verdict: MERGEABLE @ eb9cc6c (base: main)
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 8m59s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 9s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 5s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 10s
PR Gates / Docs update reminder (pull_request) Successful in 16s
PR Gates / decisions lifecycle (pull_request) Successful in 21s
PR Gates / Fix proofs (Proves trailers) (pull_request) Successful in 15s
PR Gates / Script lint and tests (ruff + pytest) (pull_request) Successful in 18m47s
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>
This commit is contained in:
@@ -919,8 +919,9 @@ jobs:
|
||||
# a record — resolve decisions through the catalog, never through a key or path quoted in a
|
||||
# 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 non-array
|
||||
# body, a non-numeric length, an empty FIRST page, or a cap reached on a NON-empty page all
|
||||
# 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.
|
||||
@@ -983,9 +984,9 @@ 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
|
||||
@@ -1020,6 +1021,10 @@ jobs:
|
||||
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
|
||||
@@ -1070,8 +1075,8 @@ jobs:
|
||||
# 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: 50 attacker-authored
|
||||
# rows bought silence before, and ~1000 buy it now. A "require K consecutive empty
|
||||
# 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.
|
||||
@@ -1093,9 +1098,26 @@ jobs:
|
||||
# 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 19
|
||||
# one-second sleeps, a page sleeping only when its first attempt yielded neither
|
||||
# an array nor null.
|
||||
# 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
|
||||
@@ -1104,11 +1126,14 @@ jobs:
|
||||
# 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 in ONE unit or it flatters the
|
||||
# change: BEFORE, the timeline needed ~100 rows (a non-empty page 1, then a 50-row
|
||||
# filtered block at a 50-aligned offset); AFTER it needs more than 1000, with the
|
||||
# block pinned to offsets 950..999. The number of COMMENTS the attacker authors is 50
|
||||
# either way — "50 vs 1000" compares an authored-comment count with a total-row count.
|
||||
# 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
|
||||
@@ -1117,8 +1142,12 @@ jobs:
|
||||
# READABLE pages does fail closed; one that fills it and hides the 20th behind a
|
||||
# filtered block does not.
|
||||
#
|
||||
# No terminator on this endpoint can do better — that is measured, not assumed. See
|
||||
# the measurements above: there is no total to page against, no row-type filter to
|
||||
# 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).
|
||||
@@ -2439,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.
|
||||
#
|
||||
|
||||
+6
-4
@@ -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
|
||||
@@ -2031,8 +2032,9 @@ page it read came back empty — the same rule the status walk above states for
|
||||
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 — a timeline over 1000 rows rather
|
||||
than ~100, the attacker authoring 50 inline comments either way — not a closed hole. It is NOT the status walk's over-cap residual: `/statuses/{sha}` does not
|
||||
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
|
||||
|
||||
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 — **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 filtered block pinned to offsets 950..999 — about 10x the price, the attacker authoring 50 inline comments either way. 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/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,16 +1012,34 @@ if "/timeline" in url:
|
||||
for part in url.split("?", 1)[-1].split("&"):
|
||||
if part.startswith("page="):
|
||||
page = int(part.split("=", 1)[1])
|
||||
if os.environ.get("STUB_TIMELINE_FLAKY_FIRST") == "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
|
||||
@@ -1801,7 +1819,7 @@ def _run_classify(
|
||||
push_mode: str = "none",
|
||||
timeline_pages: int = 1,
|
||||
timeline_filtered_pages: str = "",
|
||||
timeline_flaky_first: bool = False,
|
||||
timeline_flaky_first: str = "",
|
||||
history_mode: str = "none",
|
||||
history_creator: str = "timothy",
|
||||
history_extra: list | None = None,
|
||||
@@ -1848,7 +1866,7 @@ def _run_classify(
|
||||
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"] = "1" if timeline_flaky_first else ""
|
||||
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
|
||||
@@ -3710,7 +3728,8 @@ def test_a_FILTERED_page_before_a_FULL_cap_page_is_still_untrusted(tmp_path):
|
||||
)
|
||||
|
||||
|
||||
def test_a_SINGLE_transient_blip_does_not_cost_the_exemption(tmp_path):
|
||||
@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,
|
||||
@@ -3721,17 +3740,27 @@ def test_a_SINGLE_transient_blip_does_not_cost_the_exemption(tmp_path):
|
||||
`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=True,
|
||||
timeline_flaky_first=blip,
|
||||
)
|
||||
assert posted is not None and posted["state"] == "success", (
|
||||
"a docs-only PR lost its exemption to ONE transient timeline blip — the walk does not "
|
||||
f"retry. Log:\n{r.stdout[-900:]}"
|
||||
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:]}"
|
||||
)
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user