diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index c45447a85..2c0fc2c20 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -276,7 +276,16 @@ jobs: # stall bounded instead of permanent (see RECONCILE below), and it is sound because # `/statuses/{sha}` returns one row per POST: a verdict this job masked on the COMBINED # endpoint is still in the per-POST history for a later run to find. - UNVERIFIED_DESC="Exemption write could not be verified — re-post the verdict" + # "STATUS", NOT "EXEMPTION". This is written on paths that grant no exemption at all — an + # ordinary non-exempt `pending` with no mark, a page-2 read refusal on a head that carries + # nothing, an unreadable combined read — and the description is the operator-facing text of + # a branch-protection-required check. Telling someone whose plain unreviewed PR is waiting + # that an "exemption write" could not be verified names a thing that never happened. + # + # RENAMING IS A ONE-RUN TRANSITION, not a breaking change: a run that reads a head carrying + # the old text does not recognise it as a sentinel, re-derives it, and writes the new one. + # Nothing in production carries either string yet, so the transition is free here. + UNVERIFIED_DESC="Status write could not be verified — re-post the verdict" # WHOSE verdict may be INHERITED (ersatztv#742). Space-separated logins, compared exactly. # # The test this replaces was "the status has a non-null `.creator.login`", which only proves @@ -432,13 +441,18 @@ jobs: # every time. Measured 2026-08-29, this repo puts 8 contexts on a `main` head. That case # already stalled before this change — with an ABSENT required check, which reads as "not # reviewed yet" — so what changes is that the stall now says why. + # THE RETURN VALUE IS THE WHOLE POINT, and the first version did not have one: the `else` + # branch ended with a successful `echo`, so the function returned 0 after BOTH POSTs failed + # and its fence caller took the `exit 0` beside it as though the head had been marked. An + # explicit `return` per arm, and every caller acts on it. replace_unknown_state() { # $1 = the ::error:: naming what could not be established echo "::error::$1" if repair_status_to "$UNVERIFIED_DESC"; then echo "::error::Replaced ${CONTEXT} on ${SHA:0:7} with the unverified-write sentinel rather than leaving a state this job could not read standing. A later run will either clear it, or — if it finds a verdict underneath — ask you to re-post that verdict. To settle it now: scripts/post-review-verdict.sh ${PR} " - else - echo "::error::COULD NOT WRITE THE UNVERIFIED SENTINEL to ${SHA:0:7}. Whatever ${CONTEXT} this head carries is standing unread. Post a verdict immediately: scripts/post-review-verdict.sh ${PR} " + return 0 fi + echo "::error::COULD NOT WRITE THE UNVERIFIED SENTINEL to ${SHA:0:7}. Whatever ${CONTEXT} this head carries is standing unread. Post a verdict immediately: scripts/post-review-verdict.sh ${PR} " + return 1 } # WRITING THE SENTINEL AND FAILING THE JOB ARE SEPARATE DECISIONS, and only the read paths # take both. `read_existing_verdict`'s refusals already exited non-zero on `main`, so @@ -447,7 +461,10 @@ jobs: # every one of them into a red run is the noise this file elsewhere refuses to add. It # calls `replace_unknown_state` and returns cleanly. replace_unknown_and_die() { - replace_unknown_state "$1" + # The write may itself fail; either way this path exits non-zero, so the return value is + # deliberately not branched on here — it is `replace_unknown_state` that reports which + # happened, and the two messages differ. + replace_unknown_state "$1" || true exit 1 } @@ -510,8 +527,9 @@ jobs: # two provenance flags ex_attributable / ex_human (see the flag split below, ersatztv#742). # Factored into a function because it is now called TWICE — once here, and once immediately # before the POST (see below). An unreadable/unparseable response must NOT be read as "no - # verdict exists": the job dies WITHOUT posting, so a transient API error can never overwrite - # a verdict. + # verdict exists". Until ersatztv#849 the job simply died WITHOUT posting; it now REPLACES the + # unknown state with the sticky sentinel first, because declining to write protects a real + # verdict and leaves a FORGED one — and this job's own red status is not a required check. # # The empty case is checked EXPLICITLY, not left to jq's exit status: `jq -e` over empty input # exits 4 on jq >= 1.7 but 0 on jq 1.6, and THE RUNNER SHIPS 1.6 (ersatztv#647) — so on a @@ -613,7 +631,17 @@ jobs: replace_unknown_and_die "Could not read existing commit statuses for ${SHA:0:7} (.statuses was '${st_kind:-unparseable}', total_count '${st_total}') after a retry, so any ${CONTEXT} already on this head — including one posted by an account this gate does not accept verdicts from — can neither be read nor re-derived." fi # `// []` so the null case cannot hard-error here under `set -e` once it is accepted above. - row=$(printf '%s' "$json" | jq -r --arg c "$CONTEXT" '[(.statuses // [])[] | select(.context == $c)] | first // {}') + # TYPE-SAFE, AND A SCHEMA FAILURE REPLACES RATHER THAN DYING (ersatztv#849 round 3). + # `.statuses` being an array was checked; its ELEMENTS were not. A single scalar in that + # array makes `select(.context == ...)` hard-error, jq exits 5, and under `set -e` this + # unguarded assignment took the step down — BEFORE any of the replacement paths below, + # so a `success` already on the head stayed authoritative while the job merely went red + # on a status branch protection does not read. Same rule the post-write filter already + # applies to its own rows: drop what cannot be read, then judge what is left. + row=$(printf '%s' "$json" | jq -r --arg c "$CONTEXT" '[(.statuses // [])[] | select(type == "object") | select(.context? == $c)] | first // {}') || row="" + if [ -z "${row//[[:space:]]/}" ]; then + replace_unknown_and_die "The commit statuses for ${SHA:0:7} parsed as an array but could not be read row by row, so any ${CONTEXT} on this head can neither be read nor re-derived." + fi # THE COMPLETENESS PROBE, run only when page 1 shows no verdict — see the note above. An # unreadable or unexpected page 2 is treated as "cannot tell" and refuses, the same # direction as every other unreadable case here: concluding "no verdict exists" is what @@ -652,9 +680,13 @@ jobs: # updated_at/url, ids 14..30 ascending. `// ""` so a server that ever stopped sending it # degrades to the pre-existing text comparison rather than to a false "changed". ex_id=$(printf '%s' "$row" | jq -r 'if (.id | type) == "number" then (.id | tostring) else "" end') - ex_state=$(printf '%s' "$row" | jq -r '.status // ""') - ex_creator=$(printf '%s' "$row" | jq -r '.creator.login // ""') - ex_desc=$(printf '%s' "$row" | jq -r '.description // ""') + ex_state=$(printf '%s' "$row" | jq -r 'if (.status | type) == "string" then .status else "" end') + # `.creator.login` HARD-ERRORS on any non-object creator — the same defect #763 fixed in + # the post-write filter, still live on this read. `(.creator | type)` short-circuits it, + # so a malformed row reads as "no creator" (unattributable, hence re-derived) instead of + # killing the step after a `success` is already standing. + ex_creator=$(printf '%s' "$row" | jq -r 'if (.creator | type) == "object" then (.creator.login // "") else "" end') + ex_desc=$(printf '%s' "$row" | jq -r 'if (.description | type) == "string" then .description else "" end') # A `case` prefix test rather than grep: the description is a single short string, and this # removes one more pipeline from a security predicate entirely. The PATTERN is a literal, so # there is no glob-injection concern from $ex_desc. @@ -1273,10 +1305,29 @@ jobs: # mark", which an unrelated newer row satisfied, and it converted one anomaly into a # PERMANENT sentinel. This asks for a SPECIFIC row already known to exist, and failing it # carries the sentinel forward for THIS run only — the next run retries. - witness=$(printf '%s' "$ph_rows" | jq -r --arg c "$CONTEXT" --arg ud "$UNVERIFIED_DESC" \ - '[.[] | select(type == "object") - | select(.context? == $c) - | select(((.description | type) == "string") and (.description == $ud))] | length') || witness="" + # THE WITNESS IS THE CURRENT ROW'S ID, not merely a row with the right text + # (ersatztv#849 round 3). Matching on description alone is satisfied by an OLDER + # identical sentinel, which is precisely what a fixed point produces: with S1 and a + # buried human verdict below the CURRENT S2, a read carrying only S1 satisfies the + # witness, `buried` sees nothing, the sentinel clears, and the verdict ends up below the + # fresh mark and invisible. The record already described this as identifying a SPECIFIC + # row; the code did not. + # + # `$ex_id` COMES FROM THE COMBINED READ, so it names the row that is current right now. + # When the server omits it — not observed on this instance, where every row carries one — + # there is nothing to match on and the check degrades to the description, which is the + # pre-round-3 behaviour rather than a new hole. + if [ -n "$ex_id" ]; then + witness=$(printf '%s' "$ph_rows" | jq -r --arg c "$CONTEXT" --argjson wid "$ex_id" \ + '[.[] | select(type == "object") + | select(.context? == $c) + | select(((.id | numbers) // -1) == $wid)] | length') || witness="" + else + witness=$(printf '%s' "$ph_rows" | jq -r --arg c "$CONTEXT" --arg ud "$UNVERIFIED_DESC" \ + '[.[] | select(type == "object") + | select(.context? == $c) + | select(((.description | type) == "string") and (.description == $ud))] | length') || witness="" + fi case "$witness" in ''|*[!0-9]*) witness=0 ;; esac if [ "$ph_ok" != yes ] || [ "$witness" -eq 0 ]; then echo "::warning::Could not read a status history for ${SHA:0:7} that is both complete and contains the sentinel this head carries (complete=${ph_ok}, sentinel rows found=${witness}), so the unverified write from an earlier run still cannot be reconciled. Carrying the sentinel forward; ${CONTEXT} stays pending and no exemption is granted." @@ -1373,8 +1424,11 @@ jobs: for v in "$n_protected" "$n_not_manifest" "$n_not_docs"; do case "$v" in ''|*[!0-9]*) - echo "::error::A path predicate returned '${v}' instead of a count — the classifier is not operating, so no ${CONTEXT} status will be written for ${SHA:0:7}." - exit 1 ;; + # REPLACES, for the reason every other refusal here does (ersatztv#849 round 3): this + # run has already DECLINED to inherit whatever the head carries, so exiting without + # writing leaves that row authoritative. The old message said no status "will be + # written", which was true and beside the point — the question is what is standing. + replace_unknown_and_die "A path predicate returned '${v}' instead of a count for ${SHA:0:7}, so the classifier is not operating and this run cannot say what belongs on this head." ;; esac done @@ -1564,16 +1618,9 @@ jobs: max_id_before=-1 fi - # WITHHOLD THE EXEMPTION WHEN NOTHING CAN VERIFY IT (ersatztv#849 route 1). - # - # POSITIONED HERE, AFTER THE MARK, NOT WITH THE CLASSIFICATION. `$max_id_before` does not - # exist up there and referencing it early is an unbound variable under `set -u` — a job - # that dies before posting anything, on every PR. - # - # NOT POST-THEN-REPAIR, which is what the `success` path does when the mark DOES exist and - # the check then fires. Here the defect is known BEFORE the write, so there is no reason to - # publish a green and take it back: a repair leaves a window in which branch protection can - # see the exemption, and an already-scheduled auto-merge can fire inside it. + # MARK ANY WRITE NOTHING CAN VERIFY (ersatztv#849 route 1). The heading said "withhold the + # EXEMPTION" while the block downgrades every re-derivable state, which is narrower than + # what it does. # # EVERY RE-DERIVABLE WRITE IS DOWNGRADED, not only `success` (corrected in round 2). An # earlier version restricted this to the exemption, reasoning that a sticky generic @@ -1616,9 +1663,20 @@ jobs: # over the row — and the post-write repair does not cover that, because it is skipped # whenever the high-water mark could not be established. read_existing_verdict - if [ "$ex_attributable" = yes ] \ - && { [ "$ex_state" != "$pre_state" ] || [ "$ex_creator" != "$pre_creator" ] \ - || [ "$ex_desc" != "$pre_desc" ] || [ "$ex_id" != "$pre_id" ]; }; then + # `$row_replaced` — "the row changed", computed ONCE so both mid-run guards ask the same + # question. The id is evidence of a REPLACEMENT only when BOTH reads supplied one: if one + # response omits `id` and the other includes it, a bare inequality reports a replacement + # that did not happen, and this guard's reaction to that is to abstain — leaving a row the + # classification had already declined to inherit. One-sided absence therefore falls back to + # the triple, which is exactly the pre-existing behaviour. + row_replaced=no + if [ "$ex_state" != "$pre_state" ] || [ "$ex_creator" != "$pre_creator" ] || [ "$ex_desc" != "$pre_desc" ]; then + row_replaced=yes + fi + if [ -n "$ex_id" ] && [ -n "$pre_id" ] && [ "$ex_id" != "$pre_id" ]; then + row_replaced=yes + fi + if [ "$ex_attributable" = yes ] && [ "$row_replaced" = yes ]; then echo "::notice::An attributable verdict ('${ex_state}' by '${ex_creator}') landed on ${SHA:0:7} while this job was classifying — leaving it alone and posting nothing." exit 0 fi @@ -1666,8 +1724,7 @@ jobs: # # A run whose own write IS a sentinel is exempt from the guard: replacing a sentinel with a # sentinel loses nothing, and the repair sentinel outranks this one. - if [ "$ex_unverified" = yes ] \ - && { [ "$ex_desc" != "$pre_desc" ] || [ "$ex_id" != "$pre_id" ]; } \ + if [ "$ex_unverified" = yes ] && [ "$row_replaced" = yes ] \ && [ "$desc" != "$REPAIR_DESC" ] && [ "$desc" != "$UNVERIFIED_DESC" ]; then echo "::notice::An unverified-write sentinel was written on ${SHA:0:7} while this job was classifying, so another run posted something it could not check. This run would overwrite that record with an unmarked status a later run could re-derive into an exemption — posting NOTHING and leaving the sentinel standing." exit 0 @@ -1692,14 +1749,35 @@ jobs: # prevent, while `pending` blocks the merge immediately. # # "FOR NO SAFETY GAIN" WAS TOO STRONG and is retracted (ersatztv#742 review, #849). A - # GENERIC `pending` is not inert: it masks a rejection landing in its own write window, and - # post-write verification never runs for it, so a LATER run re-derives it into an exemption - # `success` with that rejection below the new mark. Letting it through is still right — the - # alternative strands every PR whenever the timeline is unreadable — but the trade is - # "immediate block, later re-derivation risk", not "free". + # GENERIC `pending` is not inert: it masks a rejection landing in its own write window. What + # made that DURABLE — post-write verification skipping it, so a later run re-derived it into + # an exemption `success` with the rejection below the new mark — is closed: the check now + # runs after every write, and a write no high-water mark can cover becomes the sentinel + # instead of a re-derivable description. Letting `pending` through here is still right — the + # alternative strands every PR whenever the timeline is unreadable — but the masking itself + # remains a cost, so the trade is "immediate block, one repaired write", not "free". + # ABSTAINING IS A HANDOFF ONLY WHEN THERE IS NOTHING TO HAND OFF (ersatztv#849 round 3). + # The two arms below are right not to write their CLASSIFICATION — it was computed against + # a base or a head the PR may no longer have, and the mutation that invalidated it has + # already queued a successor. But when this run DECLINED to inherit a row the head carries, + # posting nothing leaves that row authoritative for the whole window until the successor + # finishes, and the successor is not guaranteed in the one case the head-arm's own message + # names: a PR's FIRST push fires no `synchronize`. + # + # So the arms mark the head instead of merely leaving it. The sentinel is not a + # classification — it asserts nothing about the diff, only that this head carries something + # unverified — so writing it does not reintroduce what the fence exists to prevent. + # Scoped to `pre_state` being non-empty: on a head that carried nothing, abstaining leaves + # nothing, and a write there would be noise on the commonest path in this job. + mark_declined_row_if_any() { # $1 = the ::notice:: this arm already emitted + if [ -z "$pre_state" ]; then return 0; fi + replace_unknown_state "$1 This head already carried a ${CONTEXT} that this run declined to inherit, so leaving it alone would keep it authoritative until a successor finishes — and for a PR's FIRST push no successor is queued at all." || return 1 + return 0 + } count_pr_mutations if [ "$retargets_before_ok" = yes ] && [ "$rt_ok" = yes ] && [ "$rt_count" -ne "$retargets_before" ]; then - echo "::notice::PR #${PR} was retargeted while this job was classifying (${retargets_before} -> ${rt_count} retarget events). This run's classification was computed against a base the PR may no longer target, so it posts NOTHING. The retarget fired an 'edited' event, so a successor run is already queued and will write the authoritative status for ${SHA:0:7}." + echo "::notice::PR #${PR} was retargeted while this job was classifying (${retargets_before} -> ${rt_count} retarget events). This run's classification was computed against a base the PR may no longer target, so it does NOT post that classification. The retarget fired an 'edited' event, so a successor run is already queued and will write the authoritative status for ${SHA:0:7}." + mark_declined_row_if_any "PR #${PR} was retargeted while this job was classifying." || exit 1 exit 0 fi # THE HEAD AXIS (ersatztv#803/#664). Separate arm, separate message, same rule — see the @@ -1713,7 +1791,8 @@ jobs: # and it is why the enumerator's contract now says "detects one-way movement" rather than # "bound to one head". if [ "$retargets_before_ok" = yes ] && [ "$rt_ok" = yes ] && [ "$hp_count" -ne "$pushes_before" ]; then - echo "::notice::PR #${PR}'s head branch was pushed while this job was classifying (${pushes_before} -> ${hp_count} push events). The file enumeration behind this run's classification may have been paged across more than one head, and a force-push that RESTORED ${SHA:0:7} would leave every sha comparison equal, so this run posts NOTHING. A push to an OPEN PR fires 'synchronize', so a successor run is normally already queued and will write the authoritative status for whatever head is current — with one exception worth knowing before you conclude this PR is stuck: a PR's FIRST push is recorded about a second after creation and fires no 'synchronize', so an 'opened' run that abstained on it has no successor coming and the PR needs a re-trigger: a push, or a title/body/base edit. A label or a comment fires none of this workflow's triggers and will NOT re-trigger it." + echo "::notice::PR #${PR}'s head branch was pushed while this job was classifying (${pushes_before} -> ${hp_count} push events). The file enumeration behind this run's classification may have been paged across more than one head, and a force-push that RESTORED ${SHA:0:7} would leave every sha comparison equal, so this run does NOT post that classification. A push to an OPEN PR fires 'synchronize', so a successor run is normally already queued and will write the authoritative status for whatever head is current — with one exception worth knowing before you conclude this PR is stuck: a PR's FIRST push is recorded about a second after creation and fires no 'synchronize', so an 'opened' run that abstained on it has no successor coming and the PR needs a re-trigger: a push, or a title/body/base edit. A label or a comment fires none of this workflow's triggers and will NOT re-trigger it." + mark_declined_row_if_any "PR #${PR}'s head branch was pushed while this job was classifying." || exit 1 exit 0 fi if { [ "$retargets_before_ok" != yes ] || [ "$rt_ok" != yes ]; } && [ "$state" = "success" ]; then @@ -1729,8 +1808,12 @@ jobs: # established (this run could not verify its own classification), blocks the merge, and # is cleared by the next run that can read the timeline — so a transient outage costs one # event rather than a human verdict. - replace_unknown_state "Could not establish a trusted retarget/push count for PR #${PR} (before=${retargets_before_ok}, after=${rt_ok}), so an exemption 'success' cannot be shown to have been computed against the PR's current base, nor at a single head. NOTE a later run only helps if the cause was transient — a PR whose timeline exceeds the page cap will fail this way on every run, and needs a human verdict." - exit 0 + if replace_unknown_state "Could not establish a trusted retarget/push count for PR #${PR} (before=${retargets_before_ok}, after=${rt_ok}), so an exemption 'success' cannot be shown to have been computed against the PR's current base, nor at a single head. NOTE a later run only helps if the cause was transient — a PR whose timeline exceeds the page cap will fail this way on every run, and needs a human verdict."; then + exit 0 + fi + # THE SENTINEL WRITE FAILED, so nothing marked this head and whatever it carries is still + # authoritative. Exiting 0 here would report an abstention that did not happen. + exit 1 fi payload=$(jq -n --arg s "$state" --arg c "$CONTEXT" --arg d "$desc" --arg u "$PR_URL" \ @@ -1843,7 +1926,10 @@ jobs: # the next run instead of requiring a human. Before this the uncertainty arms wrote # `REPAIR_DESC` and then had to soften their own message to "no verdict was necessarily # overwritten", i.e. the description and the log contradicted each other. - raced_why=human + # `raced_why` IS OPERATOR-FACING, so its initial value is a sentence rather than the bare + # token `human`. The skip below prints it, and a log line reading "(human)" says nothing + # to whoever has to act on it. + raced_why="a human verdict landed in the write window" repair_desc="$REPAIR_DESC" if [ "$ph_ok" = yes ] && [ "$ph_len_after" -eq 0 ]; then echo "::warning::The status history for ${SHA:0:7} came back empty after this job posted to it, which cannot be true, so a raced verdict could not be ruled out. Repairing ${CONTEXT} to pending." @@ -1908,8 +1994,10 @@ jobs: # Counting the sentinel closes it: A repairs, and both runs converge on the fixed point. # # It cannot false-fire. A sentinel that already existed would have been seen at the FIRST - # read, forcing the pending path, and this block only runs after a `success` — so a - # sentinel ABOVE the mark can only have been written by another run mid-flight. + # read, forcing the pending path — so a sentinel ABOVE the mark can only have been + # written by another run mid-flight. (This block runs after EVERY write since #849; the + # argument never depended on the state, only on what a sentinel at the FIRST read + # forces, and `$own` excludes this run's own row.) # `.creator` IS TYPE-TESTED BEFORE IT IS INDEXED, and the substitution is guarded. # `.creator != null and .creator.login` hard-errors ("Cannot index number with string") # on a row whose `creator` is any non-object, jq exits 5, and under `set -e` the @@ -2033,7 +2121,15 @@ jobs: # floor above pins `repair_desc` to this run's own description: without this the job # POSTs a duplicate row and logs "Repaired …" for a change that did not happen. if [ "$raced" -gt 0 ] && [ "$repair_desc" = "$desc" ]; then - echo "::notice::Post-write verification for ${SHA:0:7} could not clear the write window (${raced_why}), but ${CONTEXT} already carries the description the repair would write, so nothing is re-posted." + # THE HUMAN CASE STILL SCREAMS. Skipping the POST is right — the head already carries + # the strongest marker this job writes — but the `::error::` below is the only place a + # reviewer is told their verdict was buried and given the command to re-post it, and an + # early `raced=0` used to swallow it. What is skipped is the WRITE, not the report. + if [ "$raced_why" = "a human verdict landed in the write window" ]; then + echo "::error::A human ${CONTEXT} verdict landed on ${SHA:0:7} while this job was writing its '${state}' status, and was overwritten. ${CONTEXT} already carries the strongest marker this job writes, so nothing is re-posted — but the verdict itself still needs re-posting: scripts/post-review-verdict.sh ${PR} " + else + echo "::notice::Post-write verification for ${SHA:0:7} could not clear the write window (${raced_why}), and the repair would write the description this run already POSTed, so nothing is re-posted. If another run has written since, its row stands — a convergence on its answer, not a silent loss: both sentinels block the merge." + fi raced=0 fi if [ "$raced" -gt 0 ]; then @@ -2047,7 +2143,7 @@ jobs: # STATE-NEUTRAL WORDING since ersatztv#849: this block now also runs after a `pending` # write, so a message naming "this exemption write" would be wrong on the very path # that was added, and wrong in the direction of understating what happened. - if [ "$raced_why" = human ]; then + if [ "$raced_why" = "a human verdict landed in the write window" ]; then echo "::error::A human ${CONTEXT} verdict landed on ${SHA:0:7} while this job was writing its '${state}' status, and was overwritten. Downgrading to 'pending' so an explicit human decision cannot be silently green, and marking the head so a later run cannot re-derive it. Re-post it with: scripts/post-review-verdict.sh ${PR} " else echo "::error::Could not verify that no human ${CONTEXT} verdict raced this job's '${state}' write on ${SHA:0:7} — ${raced_why}. Downgrading to 'pending' rather than leaving an unverified write standing; no verdict was necessarily overwritten. Clear it with: scripts/post-review-verdict.sh ${PR} " diff --git a/docs/ci-cd.md b/docs/ci-cd.md index df5233d8a..0be8aaf80 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -1690,7 +1690,7 @@ payload, which a retarget cannot rewrite, and `edited` is in `types:` so a retar `edited` gives **detection, not atomicity**: runs are not serialized, so a stale run could still post `success` after the reclassifying run posted `pending`. -**That residual is now fenced — NARROWED, not resolved (ersatztv#706; correction ersatztv#849).** The fence never re-counts *after* the POST, so a retarget landing between its final pre-write count and the write still yields a **permanent** forged green: the successor run consumes the `edited` event and exits on the existing status, and the stale run then posts last with nothing left to correct it. Runs are still not serialized — instead a run that was +**That residual is now fenced — NARROWED, not resolved (ersatztv#706; correction ersatztv#849).** The pre-write fence does not re-count *after* the POST, so a retarget landing between its final pre-write count and the write used to yield a **permanent** forged green: the successor run consumes the `edited` event and exits on the existing status, and the stale run then posts last with nothing left to correct it. Since ersatztv#849 a SEPARATE post-POST re-count (below) makes that green transient rather than permanent. Runs are still not serialized — instead a run that was overtaken *declines to write*. The job counts `change_target_branch` events on the PR's issue timeline at start and again immediately before its POST, and posts **nothing** if the count moved. The count is the key precisely because the branch *name* is ABA-vulnerable: `main → scratch → main` reads `main` at @@ -1792,9 +1792,10 @@ made the shape reachable. The cost lands where the replacement actually cost som APPROVED PR, reconciliation finds the masked verdict and upgrades to the repair sentinel, so the recovery is "you are asked to re-post", not "it clears itself". -**Both `/statuses/{sha}` reads are PAGED** (ersatztv#763). The history is read twice — before the write -for the high-water mark, after it for the race check — and `limit` clamps to `MAX_RESPONSE_ITEMS` -(measured 50), so a single read of a busy head returned a partial list. +**Every `/statuses/{sha}` read is PAGED** (ersatztv#763). The history is read twice on an ordinary +run — before the write for the high-water mark, after it for the race check — and a THIRD time on a +head carrying the unverified sentinel, for the reconciliation (ersatztv#849). `limit` clamps to +`MAX_RESPONSE_ITEMS` (measured 50), so a single read of a busy head returned a partial list. **Be precise about what that cost** — ersatztv#763's framing of it is too strong. Under the server default (`created_unix DESC`) page 1 holds the *newest* rows and ids are monotonic with `created_at`, diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 1c6b7490c..104146e99 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -68,7 +68,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ci.small-lane-git-only` | `runs-on: small` is defined by what a job does (git-only), not its usual runtime; the two `docker build` jobs (docker-build.yml, ci-image.yml) move to `ubuntu-latest` because their worst-case memory, not median runtime, was pinning the small lane's per-slot cap. | 2026-07-20 | [link](records/ci/small-lane-git-only.md) | | `ci.toolchain-image-publish-is-a-dispatch` | A `push` trigger reachable from any ref other than `main` — that is `branches:` AND `tags:`, judged by the ref class it admits and never by which keyword is present — executes ref-supplied YAML, because Gitea resolves a `push` workflow's definition from the pushed ref — so `ci-image.yml` is `branches: [main]`, and publishing a toolchain image from a feature branch is a deliberate `workflow_dispatch` on that branch rather than a side effect of pushing. Be precise about what a `branches:` filter buys: it is loaded from the pushed ref like the rest of the file, so a branch that deletes it re-enables the route — this removes the DRIVE-BY case, and is not a boundary against a writer who intends to run their own YAML. The self-referencing trigger path `.gitea/workflows/ci-image.yml` came out of BOTH its own `paths:` and `ci-image-pin`'s `expected` in the same change — a DECIDED tradeoff, not a necessity: keeping it is workable via the branch dispatch, but prices every edit to that file, comments included, at a ~2GB publish plus a five-pin bump, redone after every rebase. The cost is stated, not assumed away — a change to HOW the image is built that lives only in `ci-image.yml` no longer republishes, and the ONLY remedy is to make it alongside a `docker/ci/**` edit: publishing after the merge and then pinning cannot work, because `expected` is the last `docker/ci` commit and would reject that pin. This closes the push route INTO THIS FILE, not the class: `docker-build.yml` remains reachable from an arbitrary ref by a `v*` tag push and by `pull_request`, and four workflows carry an unrestricted `workflow_dispatch`. The DISPATCH THIRD of that is settled — #853 probed it and ACCEPTED it (`ci.workflow-dispatch-ref-unrestricted`): no ref restriction exists at Gitea 1.27.1, and restricting it would close nothing anyway, because the head-resolved `pull_request:` route runs attacker-authored YAML that reaches every secret in the store. The `v*` tag push and `pull_request:` rows are NOT settled and remain open in #885. Do not re-derive any of this. | 2026-08-27 | [link](records/ci/toolchain-image-publish-is-a-dispatch.md) | | `ci.ui-e2e-harness` | The UI-interactive E2E flows run as headless Playwright specs (`web/e2e/*.spec.ts`, driven by `scripts/e2e-ui.sh`) in a **second step of the existing advisory `functional-e2e` job**, never their own job; the browser is `chromium-headless-shell` **baked into the CI toolchain image** (`docker/ci/Dockerfile`, `PLAYWRIGHT_VERSION` kept equal to `web/package.json`'s EXACT `@playwright/test` pin), never installed per run; specs are `serial` with `retries: 0` and assert only contracts the curl harness structurally cannot reach. | 2026-07-25 | [link](records/ci/ui-e2e-harness.md) | -| `ci.verdict-unverified-write-sentinel` | The `review-verdict/h10` job runs its post-write race check after EVERY status write, not only an exemption `success`, and where no mark exists to run it against, the write ITSELF becomes the sentinel — so every status this job writes is either verified against the history or marked as unverifiable: a GENERIC `pending` masks a rejection landing in its own write window exactly as a `success` does, and because it carries no marker a later run re-derives it into an exemption with the human row now below THAT run's high-water mark — the damage arrives one event later, not never. Where a write cannot be checked at all the job writes a SECOND sentinel, `UNVERIFIED_DESC`, distinct from the repair sentinel and never interchangeable with it: the repair sentinel ASSERTS that a verdict existed and was buried, which only the arm that actually COUNTED such a row may claim, while every "could not check" arm — an unreadable or over-cap history, an impossible empty history, an unusable count — states only what it established. Both are STICKY (the classification refuses to grant an exemption over either, and re-writes its own description verbatim, so each is a FIXED POINT a later run cannot re-derive into `success`); only the unverified one is RECONCILABLE. Reconciliation is what bounds the stall that got the #742 attempt withdrawn: a later run pages `/statuses/{sha}` IN FULL and, ONLY IF that history contains the sentinel's own row, either finds a `Review-verdict:` row underneath the sentinel — an established fact, so it UPGRADES to the repair sentinel, clearable only by a human — or finds none, resolving the uncertainty and clearing it so the PR is classified normally. The reconciliation is sound because the two endpoints differ: a verdict this job masked is invisible on the COMBINED endpoint (latest row per context, which is the sentinel) and still present in the per-POST history. It is deliberately BROADER than the inheritance test — no `H10_REVIEWERS` membership, no `(base: …)` match — because over-counting upgrades to a stall a human clears in one command while MISSING one re-exempts a head carrying a buried rejection. ONLY a COMPLETE walk CONTAINING THE SENTINEL ROW may clear it. The witness is not optional decoration: `ex_unverified` means the combined endpoint just returned that row, and `/statuses/{sha}` keeps one row per POST, so a complete history WITHOUT it — an empty one included — contradicts a write that demonstrably happened. `page_statuses` accepts an empty page 1 as complete (correct for a first-run mark), which is what made that shape reachable, and clearing on it exempted a head whose sentinel may have been sitting on a rejection. This is NOT the withdrawn currency witness: that asked whether ANY row sat above the mark, which an unrelated row satisfied, and it made one anomaly permanent — this asks for a SPECIFIC row already known to exist, and failing it costs one run rather than requiring a human. When NO high-water mark can be established the write is withheld BEFORE the POST rather than posted and repaired, since the defect is known in advance and publishing a green to take it back opens a window branch protection — and an already-scheduled auto-merge — can see. EVERY re-derivable state is downgraded there, not only `success`: an earlier draft restricted it to the exemption on the grounds that a sticky generic `pending` "withholds nothing, since an unreviewed PR is blocked already", which analysed the wrong PR — the damaging case is one that IS exemptible and got the generic `pending` only from a transient enumeration failure, whose unmarked description the next run re-derives into the exemption with the human row below its own mark. `$REPAIR_DESC` is the one exemption, being the stronger fact and not re-derivable. SEPARATELY, the retarget count is re-taken AFTER the POST on the exemption path, which closes the PERMANENT forged green `ci.verdict-write-retarget-fence` recorded as its residual: a retarget landing after the final pre-write count leaves a stale `success` with the `edited` event already consumed by a successor that short-circuited, so nothing remains to reclassify. Re-counting afterwards means a retarget later than the re-check necessarily queues a successor that STARTS after the stale `success` exists, and a machine-written `success` is re-derived rather than inherited. The RETARGET axis only: a push after the POST moves the head, so the status no longer gates that PR, while a retarget changes the effective diff with the sha unchanged. An untrusted post-POST count repairs too — reaching that point with `success` means both earlier counts were trusted, so a third unreadable read is a fresh failure and "I cannot tell whether the base moved" must not resolve to leaving a green. FINALLY, every path that cannot establish what the head carries REPLACES the unknown state instead of merely declining to write — the combined read (retried once first), all four page-2 completeness refusals, and the fence branch that cannot trust its retarget count while holding a derived `success`. That last one is reached only AFTER the classification declined to inherit the row the head carries, so posting nothing left the declined row current; its message used to say the context "stays absent", which is true only of a head that had none. Declining protects a real verdict and leaves a FORGED one — an off-list `success` is the row #742 exists to revoke, revocation happens by re-deriving it, and an unreadable read is the one thing that stops it while the job goes red on a status branch protection does not read. Nothing is destroyed: the per-POST history keeps the masked verdict and the next run's reconciliation upgrades to the repair sentinel, telling the reviewer to re-post rather than silently un-approving them. The page-2 refusals were briefly excluded on the reasoning that the probe fires when NO row for this context was on page 1, so there is no green of any provenance to leave standing — self-contradictory, since the ONLY reason page 2 is read is that the row MAY be beyond page 1. WRITING THE SENTINEL AND FAILING THE JOB ARE SEPARATE DECISIONS: the read refusals were already non-zero exits on `main` and stay red, while the fence branch exited 0 there and still does, because an unreadable timeline is an ordinary hiccup and reddening every one of them is noise this file elsewhere refuses to add. The repair also has a FLOOR — it may never write a description weaker than the one this run decided, or a transient post-write read rewrites a correct `$REPAIR_DESC` carry-forward with the machine-clearable sentinel — and it is skipped entirely when it would rewrite what is already there. | 2026-08-29 | [link](records/ci/verdict-unverified-write-sentinel.md) | +| `ci.verdict-unverified-write-sentinel` | The `review-verdict/h10` job runs its post-write race check after EVERY status write, not only an exemption `success`, and where no mark exists to run it against, the write ITSELF becomes the sentinel — so every status this job writes is either verified against the history or marked as unverifiable: a GENERIC `pending` masks a rejection landing in its own write window exactly as a `success` does, and because it carries no marker a later run re-derives it into an exemption with the human row now below THAT run's high-water mark — the damage arrives one event later, not never. Where a write cannot be checked at all the job writes a SECOND sentinel, `UNVERIFIED_DESC`, distinct from the repair sentinel and never interchangeable with it: the repair sentinel ASSERTS that a verdict existed and was buried, which only the arm that actually COUNTED such a row may claim, while every "could not check" arm — an unreadable or over-cap history, an impossible empty history, an unusable count — states only what it established. Both are STICKY (the classification refuses to grant an exemption over either, and re-writes its own description verbatim, so each is a FIXED POINT a later run cannot re-derive into `success`); only the unverified one is RECONCILABLE. Reconciliation is what bounds the stall that got the #742 attempt withdrawn: a later run pages `/statuses/{sha}` IN FULL and, ONLY IF that history contains the sentinel's own row, either finds a `Review-verdict:` row underneath the sentinel — an established fact, so it UPGRADES to the repair sentinel, clearable only by a human — or finds none, resolving the uncertainty and clearing it so the PR is classified normally. The reconciliation is sound because the two endpoints differ: a verdict this job masked is invisible on the COMBINED endpoint (latest row per context, which is the sentinel) and still present in the per-POST history. It is deliberately BROADER than the inheritance test — no `H10_REVIEWERS` membership, no `(base: …)` match — because over-counting upgrades to a stall a human clears in one command while MISSING one re-exempts a head carrying a buried rejection. ONLY a COMPLETE walk CONTAINING THE CURRENT SENTINEL ROW — matched by the `id` the combined read reported, not merely by a row with the same description — may clear it. Description alone is satisfied by an OLDER identical sentinel, which is exactly what a fixed point produces: with an earlier sentinel and a buried verdict below the current one, a read carrying only the earlier row satisfies the test, the sentinel clears, and the verdict ends up below the fresh mark. Where the server omits `id` there is nothing to match on and the check degrades to the description, which is the pre-existing behaviour rather than a new hole. The witness is not optional decoration: `ex_unverified` means the combined endpoint just returned that row, and `/statuses/{sha}` keeps one row per POST, so a complete history WITHOUT it — an empty one included — contradicts a write that demonstrably happened. `page_statuses` accepts an empty page 1 as complete (correct for a first-run mark), which is what made that shape reachable, and clearing on it exempted a head whose sentinel may have been sitting on a rejection. This is NOT the withdrawn currency witness: that asked whether ANY row sat above the mark, which an unrelated row satisfied, and it made one anomaly permanent — this asks for a SPECIFIC row already known to exist. Failing it costs one run in the transient case; it is NOT bounded in general, because a history past the 20-page cap can never be walked completely, so such a head needs a human verdict and no later run will clear it. When NO high-water mark can be established the write is withheld BEFORE the POST rather than posted and repaired, since the defect is known in advance and publishing a green to take it back opens a window branch protection — and an already-scheduled auto-merge — can see. EVERY re-derivable state is downgraded there, not only `success`: an earlier draft restricted it to the exemption on the grounds that a sticky generic `pending` "withholds nothing, since an unreviewed PR is blocked already", which analysed the wrong PR — the damaging case is one that IS exemptible and got the generic `pending` only from a transient enumeration failure, whose unmarked description the next run re-derives into the exemption with the human row below its own mark. `$REPAIR_DESC` is the one exemption, being the stronger fact and not re-derivable. SEPARATELY, the retarget count is re-taken AFTER the POST on the exemption path, which closes the PERMANENT forged green `ci.verdict-write-retarget-fence` recorded as its residual: a retarget landing after the final pre-write count leaves a stale `success` with the `edited` event already consumed by a successor that short-circuited, so nothing remains to reclassify. Re-counting afterwards means a retarget later than the re-check necessarily queues a successor that STARTS after the stale `success` exists, and a machine-written `success` is re-derived rather than inherited. The RETARGET axis only: a push after the POST moves the head, so the status no longer gates that PR, while a retarget changes the effective diff with the sha unchanged. An untrusted post-POST count repairs too — reaching that point with `success` means both earlier counts were trusted, so a third unreadable read is a fresh failure and "I cannot tell whether the base moved" must not resolve to leaving a green. FINALLY, every path that cannot establish what the head carries REPLACES the unknown state instead of merely declining to write — the combined read (retried once first), all four page-2 completeness refusals, and the fence branch that cannot trust its retarget count while holding a derived `success`. That last one is reached only AFTER the classification declined to inherit the row the head carries, so posting nothing left the declined row current; its message used to say the context "stays absent", which is true only of a head that had none. The two OBSERVED-mutation arms take the same decision for the same reason, scoped to a head that actually carries a declined row: they still refuse to post their CLASSIFICATION, which was computed against a base or head the PR may no longer have, but a row this run declined must not stay authoritative for the whole window until a successor finishes — and for a PR's FIRST push no successor is queued at all. Every element and every consumed field of the combined response is TYPE-CHECKED before extraction, and a schema failure routes to the same replacement rather than dying: `.statuses` being an array was checked and its ELEMENTS were not, so one scalar made `select(.context == $c)` hard-error and `set -e` took the step down before any path could mark the head. The path-predicate failure replaces too. And the write helper RETURNS a status: its first version ended the failure arm with a successful `echo`, so it reported 0 after both POSTs failed and the fence caller exited 0 as though the head had been marked. Declining protects a real verdict and leaves a FORGED one — an off-list `success` is the row #742 exists to revoke, revocation happens by re-deriving it, and an unreadable read is the one thing that stops it while the job goes red on a status branch protection does not read. Nothing is destroyed: the per-POST history keeps the masked verdict and the next run's reconciliation upgrades to the repair sentinel, telling the reviewer to re-post rather than silently un-approving them. The page-2 refusals were briefly excluded on the reasoning that the probe fires when NO row for this context was on page 1, so there is no green of any provenance to leave standing — self-contradictory, since the ONLY reason page 2 is read is that the row MAY be beyond page 1. WRITING THE SENTINEL AND FAILING THE JOB ARE SEPARATE DECISIONS: the read refusals were already non-zero exits on `main` and stay red, while the fence branch exited 0 there and still does, because an unreadable timeline is an ordinary hiccup and reddening every one of them is noise this file elsewhere refuses to add. The repair also has a FLOOR — it may never write a description weaker than the one this run decided, or a transient post-write read rewrites a correct `$REPAIR_DESC` carry-forward with the machine-clearable sentinel — and it is skipped entirely when it would rewrite what is already there. | 2026-08-29 | [link](records/ci/verdict-unverified-write-sentinel.md) | | `ci.verdict-write-retarget-fence` | The `review-verdict/h10` job counts BOTH `change_target_branch` AND `pull_push` events on the PR's issue timeline at run start and again immediately before its POST, and writes NOTHING if EITHER count moved. The COUNT is the key on both axes because the underlying VALUE is ABA-vulnerable — `main -> S -> main` reads `main` at both ends, which is how #698 route 1 obtained a forged exemption, and a force-push `H1 -> H2 -> H1` leaves `.head.sha` equal at both ends while the middle pages of `scripts/pr-changed-files.sh`'s enumeration came from `H2` (#803/#664) — while an event count is monotonic and cannot alias. ONE walk certifies BOTH counts, so an unreadable page abandons both; the two tallies are separate so the diagnostic names the axis that actually moved. Every push is counted and `is_force_push` is deliberately NOT read: an ordinary push also invalidates a mid-flight enumeration, and an `H1 -> H2 -> H1` restoration can have its second push non-forced when `H1` is an ancestor. The head fence does NOT abstain on its own triggering push — established FROM THE v1.27.1 SOURCE, since that would be a permanent stall rather than a fence: the push comment is created BEFORE the synchronize notification is emitted, so a run always sees its own causative event, and retries add no new event. The 26-102s margin measured across 20 triggered pairs on PRs #802/#834/#761 corroborates it and is a lower bound (the job runs a checkout first; 69s end to end on run 2385) — it is NOT the basis of the claim, which an earlier draft said it was. This NARROWS the residual rather than resolving it, and what CLOSED the permanent case is a separate mechanism recorded at `ci.verdict-unverified-write-sentinel`: the retarget count is re-taken AFTER the POST, so a retarget between the final pre-write count and the POST — which used to leave a PERMANENT forged green, the successor having consumed the `edited` event and exited before the stale run posted last — is now caught by the writing run itself, and one landing after the re-check necessarily queues a successor that starts with the stale `success` already visible and re-derivable (corrected 2026-08-27, closed 2026-08-29, #849). Abstaining is a handoff, not a stall, and that is the property the design rests on — for a run that ABSTAINS; it says nothing about one that already passed its final count and then posts (see the residual, corrected #849): every retarget fires `edited`, which is in this workflow's `types:`, so the event that makes a run abstain has already queued a successor whose window opens after it; the induction terminates when retargeting stops — but ONLY over runs that ABSTAIN. A run that already passed its final count is outside it: it writes whenever it gets there, so the run that writes LAST is not necessarily the one that classified last. That was the permanent residual until #849 gave the writing run a post-POST re-count of its own (`ci.verdict-unverified-write-sentinel`), which puts such a run back inside the induction — it either withdraws its own stale write or leaves a green a guaranteed successor re-derives. `updated_at` was REJECTED as the key because it also moves for comments and labels, which fire none of this workflow's `types:` — a run could abstain with no successor coming, which is a real stall. The count is trusted only when paging reached a validated EMPTY page; an untrusted count (unreadable page, non-array body, non-numeric length, page cap hit) blocks the exemption `success` ONLY and still lets `pending` through, because `pending` blocks the merge immediately while withholding it would strand ordinary PRs whenever the timeline is unreadable — the right trade, but NOT a free one ("for no safety gain" retracted 2026-08-27): a GENERIC `pending` masks a rejection landing in its own write window just as a `success` does. What made that DURABLE — post-write verification skipping it, so a later run re-derived it into an exemption `success` — is closed since 2026-08-29: the check now runs after EVERY write (`ci.verdict-unverified-write-sentinel`), so such a write is repaired to the sticky repair sentinel and cannot be re-derived. The masking itself is still a cost, and a write the mark cannot cover is still unverified; that is what the second sentinel is for. SEPARATELY, and for the human-verdict race the fence does nothing about: after posting ANY status — every write since #849, not only an exemption `success` — the job re-reads `/statuses/{sha}` IN FULL (PAGED since #763) and, if a human `Review-verdict:` row appeared with an id ABOVE a high-water mark taken just before the POST, overwrites its own status with `pending` and logs an error. The repair is `pending`, NEVER a copy of the human's state, since re-posting their `failure` under the machine credential would attribute a human verdict to the job; its description is a SENTINEL that the classification refuses to grant an exemption over AND re-writes verbatim on every later run, so the block is a FIXED POINT rather than decaying — writing the generic `pending` description there instead erases the marker and the exemption simply returns one event later. The mark is captured BEFORE the last-moment re-read, not merely before the POST — a later mark leaves a multi-round-trip blind gap in which a verdict is neither seen by the re-read nor repaired afterwards. The id comparison is load-bearing: a mere presence test would fire forever on a base-mismatched verdict that `read_existing_verdict` deliberately declines to honour, deadlocking that PR's exemption permanently. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run's `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run's mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. BOTH `/statuses/{sha}` reads PAGE to a validated EMPTY page since #763 — `[]` on this endpoint, a THIRD terminator shape distinct from the timeline's bare `null` and the combined endpoint's `{"statuses": null}`, so each terminator is MEASURED per endpoint — never terminating on a SHORT page, retrying each page once, under a 20-page cap that validates at most 950 rows (the 20th request must be the empty terminator); correctness does not depend on the measured cap of 50, because any page size pages correctly. WHAT THE PAGING BUYS IS NOT WHAT #763 CLAIMED: under the DESC default page 1 already held the true maximum id AND every row newer than the mark, so a single-page read missed a raced verdict only if more than 50 rows were created INSIDE the write window — not merely on a head over 50 rows. What removed #761's stall is retiring the probe, not the walk; the walk's value is that the gate's one fail-toward-SUCCESS path no longer depends on an UNDOCUMENTED ordering the server honours only coarsely (page 1 came back `114,112,113,111,110`). This RETIRED #751's conservative page-2 probe, which treated "there are rows I did not read" as "assume raced" and so repaired every head that outgrew one page: it fired on Renovate PR #761, reporting a human verdict as overwritten when none existed and then, the sentinel being sticky, refusing to re-exempt that head on every later run. The two directions are NOT symmetric. POST-WRITE, uncertainty fails CLOSED — an unreadable history, an over-cap history, or a count that is not a number all repair to `pending`; previously an unreadable history warned and left the exemption green while the page-2 probe repaired on the same uncertainty, one check disagreeing with itself. PRE-WRITE, a PARTIAL list still yields a mark, because the mark gates the post-write check entirely and refusing one SKIPS that check, which is itself the fail-open; this rests on the DESC DEFAULT — the newest row, carrying the maximum id, is on page 1, so a walk that fails later still saw it — while a VALIDATED empty history is NOT abandoned (it yields a mark of 0, correct for a first run, since every later row is newer) — what abandons the mark is a read that both FAILED and returned nothing — which since 2026-08-29 WITHHOLDS the exemption before the POST and marks the head with the reconcilable sentinel, rather than posting a green nothing can check (#849) — and a NON-EMPTY history carrying no numeric id is reported unusable rather than collapsed to 0. BOTH id comparisons are NUMERIC-ONLY: jq orders strings above every number, so one `"id": "99999"` inflates the mark until nothing looks newer, and `.id > $since` reads any string id as newer than any mark — making a PRE-EXISTING base-mismatched verdict look raced on every run, a permanent per-sha stall (the twin was live on `main`). An EMPTY post-write history is REJECTED: the walk terminates on an empty page, which is correct before the write and impossible after it (one row per POST), and a well-formed "no statuses exist" is not retried — so accepting it would conclude `raced=0` from a list that cannot be real, silently. That is NOT the withdrawn currency witness, which asked whether ANY row sat above the mark and was satisfied by an unrelated newer row; this asks only whether the list is EMPTY, a state no unrelated row can produce. The `::error::` now names its own cause, of which there are THREE — a verdict actually FOUND, a read that could not be COMPLETED, and a read that completed but returned an IMPOSSIBLE answer (the third is not a variety of the second) — while WHICH sentinel description is written is itself part of the answer since #849: only an arm that actually COUNTED a verdict row may write the repair sentinel, and every "could not check" arm writes the reconcilable one (`ci.verdict-unverified-write-sentinel`). Both are fixed points the classification recognises. `.creator` is TYPE-TESTED before it is indexed: `.creator != null and .creator.login` hard-errors on a non-object creator, jq exits 5, and under `set -e` that took the step down AFTER the exemption was posted and BEFORE the repair — a forged green reported as an infrastructure error. | 2026-08-03 | [link](records/ci/verdict-write-retarget-fence.md) | | `ci.verify-locally-ci-confirms` | Treat the local build/verify/review pass as the decision point and CI as confirmation — don't idle waiting on a run you have no reason to doubt. | 2026-07-21 | [link](records/ci/verify-locally-ci-confirms.md) | | `ci.web-test-per-test-timeouts` | Give heavy-render web tests an explicit per-test vitest timeout (e.g. 15s); never raise the global default to fix one slow test. | 2026-07-21 | [link](records/ci/web-test-per-test-timeouts.md) | diff --git a/docs/decisions/records/ci/exemption-provenance.md b/docs/decisions/records/ci/exemption-provenance.md index 286ae286b..87c0e219c 100644 --- a/docs/decisions/records/ci/exemption-provenance.md +++ b/docs/decisions/records/ci/exemption-provenance.md @@ -38,7 +38,7 @@ than described as fixed. `edited` and re-derivation remain one fix — `edited` on the existing `success`; re-derivation alone never gets a second run — but together they are mitigation, not a guarantee. -**Narrowed 2026-08-03 (#706) — an earlier heading here said RESOLVED, corrected 2026-08-27 (#849): the fence never re-counts after its final pre-write read, so the post-final-count/pre-POST window remains, and is PERMANENT rather than transient. Worth recording that the narrowing finally came from somewhere else +**Narrowed 2026-08-03 (#706) — an earlier heading here said RESOLVED, corrected 2026-08-27 (#849): the pre-write fence does not re-count after its final read, so the post-final-count/pre-POST window remains. It was PERMANENT rather than transient until #849 added a post-POST re-count (`ci.verdict-unverified-write-sentinel`), which makes it transient — the window between the POST and the repair — rather than closing it. Worth recording that the narrowing finally came from somewhere else entirely.** The missing piece was never ordering: `ci.verdict-write-retarget-fence` leaves the runs as unserialized as they ever were and instead makes a run that was overtaken decline to write, keyed on the timeline's monotonic retarget COUNT — the one signal the `main → scratch → main` ABA cannot make diff --git a/docs/decisions/records/ci/verdict-unverified-write-sentinel.md b/docs/decisions/records/ci/verdict-unverified-write-sentinel.md index 15d2387cd..292de0316 100644 --- a/docs/decisions/records/ci/verdict-unverified-write-sentinel.md +++ b/docs/decisions/records/ci/verdict-unverified-write-sentinel.md @@ -5,9 +5,9 @@ status: active since: '2026-08-29' supersedes: none superseded-by: none -rule: 'The `review-verdict/h10` job runs its post-write race check after EVERY status write, not only an exemption `success`, and where no mark exists to run it against, the write ITSELF becomes the sentinel — so every status this job writes is either verified against the history or marked as unverifiable: a GENERIC `pending` masks a rejection landing in its own write window exactly as a `success` does, and because it carries no marker a later run re-derives it into an exemption with the human row now below THAT run''s high-water mark — the damage arrives one event later, not never. Where a write cannot be checked at all the job writes a SECOND sentinel, `UNVERIFIED_DESC`, distinct from the repair sentinel and never interchangeable with it: the repair sentinel ASSERTS that a verdict existed and was buried, which only the arm that actually COUNTED such a row may claim, while every "could not check" arm — an unreadable or over-cap history, an impossible empty history, an unusable count — states only what it established. Both are STICKY (the classification refuses to grant an exemption over either, and re-writes its own description verbatim, so each is a FIXED POINT a later run cannot re-derive into `success`); only the unverified one is RECONCILABLE. Reconciliation is what bounds the stall that got the #742 attempt withdrawn: a later run pages `/statuses/{sha}` IN FULL and, ONLY IF that history contains the sentinel''s own row, either finds a `Review-verdict:` row underneath the sentinel — an established fact, so it UPGRADES to the repair sentinel, clearable only by a human — or finds none, resolving the uncertainty and clearing it so the PR is classified normally. The reconciliation is sound because the two endpoints differ: a verdict this job masked is invisible on the COMBINED endpoint (latest row per context, which is the sentinel) and still present in the per-POST history. It is deliberately BROADER than the inheritance test — no `H10_REVIEWERS` membership, no `(base: …)` match — because over-counting upgrades to a stall a human clears in one command while MISSING one re-exempts a head carrying a buried rejection. ONLY a COMPLETE walk CONTAINING THE SENTINEL ROW may clear it. The witness is not optional decoration: `ex_unverified` means the combined endpoint just returned that row, and `/statuses/{sha}` keeps one row per POST, so a complete history WITHOUT it — an empty one included — contradicts a write that demonstrably happened. `page_statuses` accepts an empty page 1 as complete (correct for a first-run mark), which is what made that shape reachable, and clearing on it exempted a head whose sentinel may have been sitting on a rejection. This is NOT the withdrawn currency witness: that asked whether ANY row sat above the mark, which an unrelated row satisfied, and it made one anomaly permanent — this asks for a SPECIFIC row already known to exist, and failing it costs one run rather than requiring a human. When NO high-water mark can be established the write is withheld BEFORE the POST rather than posted and repaired, since the defect is known in advance and publishing a green to take it back opens a window branch protection — and an already-scheduled auto-merge — can see. EVERY re-derivable state is downgraded there, not only `success`: an earlier draft restricted it to the exemption on the grounds that a sticky generic `pending` "withholds nothing, since an unreviewed PR is blocked already", which analysed the wrong PR — the damaging case is one that IS exemptible and got the generic `pending` only from a transient enumeration failure, whose unmarked description the next run re-derives into the exemption with the human row below its own mark. `$REPAIR_DESC` is the one exemption, being the stronger fact and not re-derivable. SEPARATELY, the retarget count is re-taken AFTER the POST on the exemption path, which closes the PERMANENT forged green `ci.verdict-write-retarget-fence` recorded as its residual: a retarget landing after the final pre-write count leaves a stale `success` with the `edited` event already consumed by a successor that short-circuited, so nothing remains to reclassify. Re-counting afterwards means a retarget later than the re-check necessarily queues a successor that STARTS after the stale `success` exists, and a machine-written `success` is re-derived rather than inherited. The RETARGET axis only: a push after the POST moves the head, so the status no longer gates that PR, while a retarget changes the effective diff with the sha unchanged. An untrusted post-POST count repairs too — reaching that point with `success` means both earlier counts were trusted, so a third unreadable read is a fresh failure and "I cannot tell whether the base moved" must not resolve to leaving a green. FINALLY, every path that cannot establish what the head carries REPLACES the unknown state instead of merely declining to write — the combined read (retried once first), all four page-2 completeness refusals, and the fence branch that cannot trust its retarget count while holding a derived `success`. That last one is reached only AFTER the classification declined to inherit the row the head carries, so posting nothing left the declined row current; its message used to say the context "stays absent", which is true only of a head that had none. Declining protects a real verdict and leaves a FORGED one — an off-list `success` is the row #742 exists to revoke, revocation happens by re-deriving it, and an unreadable read is the one thing that stops it while the job goes red on a status branch protection does not read. Nothing is destroyed: the per-POST history keeps the masked verdict and the next run''s reconciliation upgrades to the repair sentinel, telling the reviewer to re-post rather than silently un-approving them. The page-2 refusals were briefly excluded on the reasoning that the probe fires when NO row for this context was on page 1, so there is no green of any provenance to leave standing — self-contradictory, since the ONLY reason page 2 is read is that the row MAY be beyond page 1. WRITING THE SENTINEL AND FAILING THE JOB ARE SEPARATE DECISIONS: the read refusals were already non-zero exits on `main` and stay red, while the fence branch exited 0 there and still does, because an unreadable timeline is an ordinary hiccup and reddening every one of them is noise this file elsewhere refuses to add. The repair also has a FLOOR — it may never write a description weaker than the one this run decided, or a transient post-write read rewrites a correct `$REPAIR_DESC` carry-forward with the machine-clearable sentinel — and it is skipped entirely when it would rewrite what is already there.' +rule: 'The `review-verdict/h10` job runs its post-write race check after EVERY status write, not only an exemption `success`, and where no mark exists to run it against, the write ITSELF becomes the sentinel — so every status this job writes is either verified against the history or marked as unverifiable: a GENERIC `pending` masks a rejection landing in its own write window exactly as a `success` does, and because it carries no marker a later run re-derives it into an exemption with the human row now below THAT run''s high-water mark — the damage arrives one event later, not never. Where a write cannot be checked at all the job writes a SECOND sentinel, `UNVERIFIED_DESC`, distinct from the repair sentinel and never interchangeable with it: the repair sentinel ASSERTS that a verdict existed and was buried, which only the arm that actually COUNTED such a row may claim, while every "could not check" arm — an unreadable or over-cap history, an impossible empty history, an unusable count — states only what it established. Both are STICKY (the classification refuses to grant an exemption over either, and re-writes its own description verbatim, so each is a FIXED POINT a later run cannot re-derive into `success`); only the unverified one is RECONCILABLE. Reconciliation is what bounds the stall that got the #742 attempt withdrawn: a later run pages `/statuses/{sha}` IN FULL and, ONLY IF that history contains the sentinel''s own row, either finds a `Review-verdict:` row underneath the sentinel — an established fact, so it UPGRADES to the repair sentinel, clearable only by a human — or finds none, resolving the uncertainty and clearing it so the PR is classified normally. The reconciliation is sound because the two endpoints differ: a verdict this job masked is invisible on the COMBINED endpoint (latest row per context, which is the sentinel) and still present in the per-POST history. It is deliberately BROADER than the inheritance test — no `H10_REVIEWERS` membership, no `(base: …)` match — because over-counting upgrades to a stall a human clears in one command while MISSING one re-exempts a head carrying a buried rejection. ONLY a COMPLETE walk CONTAINING THE CURRENT SENTINEL ROW — matched by the `id` the combined read reported, not merely by a row with the same description — may clear it. Description alone is satisfied by an OLDER identical sentinel, which is exactly what a fixed point produces: with an earlier sentinel and a buried verdict below the current one, a read carrying only the earlier row satisfies the test, the sentinel clears, and the verdict ends up below the fresh mark. Where the server omits `id` there is nothing to match on and the check degrades to the description, which is the pre-existing behaviour rather than a new hole. The witness is not optional decoration: `ex_unverified` means the combined endpoint just returned that row, and `/statuses/{sha}` keeps one row per POST, so a complete history WITHOUT it — an empty one included — contradicts a write that demonstrably happened. `page_statuses` accepts an empty page 1 as complete (correct for a first-run mark), which is what made that shape reachable, and clearing on it exempted a head whose sentinel may have been sitting on a rejection. This is NOT the withdrawn currency witness: that asked whether ANY row sat above the mark, which an unrelated row satisfied, and it made one anomaly permanent — this asks for a SPECIFIC row already known to exist. Failing it costs one run in the transient case; it is NOT bounded in general, because a history past the 20-page cap can never be walked completely, so such a head needs a human verdict and no later run will clear it. When NO high-water mark can be established the write is withheld BEFORE the POST rather than posted and repaired, since the defect is known in advance and publishing a green to take it back opens a window branch protection — and an already-scheduled auto-merge — can see. EVERY re-derivable state is downgraded there, not only `success`: an earlier draft restricted it to the exemption on the grounds that a sticky generic `pending` "withholds nothing, since an unreviewed PR is blocked already", which analysed the wrong PR — the damaging case is one that IS exemptible and got the generic `pending` only from a transient enumeration failure, whose unmarked description the next run re-derives into the exemption with the human row below its own mark. `$REPAIR_DESC` is the one exemption, being the stronger fact and not re-derivable. SEPARATELY, the retarget count is re-taken AFTER the POST on the exemption path, which closes the PERMANENT forged green `ci.verdict-write-retarget-fence` recorded as its residual: a retarget landing after the final pre-write count leaves a stale `success` with the `edited` event already consumed by a successor that short-circuited, so nothing remains to reclassify. Re-counting afterwards means a retarget later than the re-check necessarily queues a successor that STARTS after the stale `success` exists, and a machine-written `success` is re-derived rather than inherited. The RETARGET axis only: a push after the POST moves the head, so the status no longer gates that PR, while a retarget changes the effective diff with the sha unchanged. An untrusted post-POST count repairs too — reaching that point with `success` means both earlier counts were trusted, so a third unreadable read is a fresh failure and "I cannot tell whether the base moved" must not resolve to leaving a green. FINALLY, every path that cannot establish what the head carries REPLACES the unknown state instead of merely declining to write — the combined read (retried once first), all four page-2 completeness refusals, and the fence branch that cannot trust its retarget count while holding a derived `success`. That last one is reached only AFTER the classification declined to inherit the row the head carries, so posting nothing left the declined row current; its message used to say the context "stays absent", which is true only of a head that had none. The two OBSERVED-mutation arms take the same decision for the same reason, scoped to a head that actually carries a declined row: they still refuse to post their CLASSIFICATION, which was computed against a base or head the PR may no longer have, but a row this run declined must not stay authoritative for the whole window until a successor finishes — and for a PR''s FIRST push no successor is queued at all. Every element and every consumed field of the combined response is TYPE-CHECKED before extraction, and a schema failure routes to the same replacement rather than dying: `.statuses` being an array was checked and its ELEMENTS were not, so one scalar made `select(.context == $c)` hard-error and `set -e` took the step down before any path could mark the head. The path-predicate failure replaces too. And the write helper RETURNS a status: its first version ended the failure arm with a successful `echo`, so it reported 0 after both POSTs failed and the fence caller exited 0 as though the head had been marked. Declining protects a real verdict and leaves a FORGED one — an off-list `success` is the row #742 exists to revoke, revocation happens by re-deriving it, and an unreadable read is the one thing that stops it while the job goes red on a status branch protection does not read. Nothing is destroyed: the per-POST history keeps the masked verdict and the next run''s reconciliation upgrades to the repair sentinel, telling the reviewer to re-post rather than silently un-approving them. The page-2 refusals were briefly excluded on the reasoning that the probe fires when NO row for this context was on page 1, so there is no green of any provenance to leave standing — self-contradictory, since the ONLY reason page 2 is read is that the row MAY be beyond page 1. WRITING THE SENTINEL AND FAILING THE JOB ARE SEPARATE DECISIONS: the read refusals were already non-zero exits on `main` and stay red, while the fence branch exited 0 there and still does, because an unreadable timeline is an ordinary hiccup and reddening every one of them is noise this file elsewhere refuses to add. The repair also has a FLOOR — it may never write a description weaker than the one this run decided, or a transient post-write read rewrites a correct `$REPAIR_DESC` carry-forward with the machine-clearable sentinel — and it is skipped entirely when it would rewrite what is already there.' signals: 'exemption success standing over a human failure, generic pending re-derived into an exemption, unverified write sentinel, reconcilable sentinel vs repair sentinel, no high-water mark could be established, post-write verification runs for every write, retarget after the POST leaves a permanent forged green, unreadable combined status read leaves an off-list green, why did my docs-only PR lose its exemption, why does the gate say the write could not be verified · paths: `.gitea/workflows/review-verdict.yml`, `scripts/tests/test_pr_changed_files.py` · issues: #849, #742, #763, #706, #803' -mechanics: '`UNVERIFIED_DESC` ("Exemption write could not be verified — re-post the verdict") beside `REPAIR_DESC`; `read_existing_verdict()` sets `ex_unverified` from a prefix match, retries the combined read once and on persistent failure calls `repair_status_to "$UNVERIFIED_DESC"` before `exit 1`; the RECONCILE block runs `page_statuses` and counts `Review-verdict:` rows with a non-null creator, clearing `ex_unverified`, upgrading to `ex_repair`, or carrying the sentinel forward on `ph_ok != yes`; the classification chain is `exempt` → `ex_repair` → `ex_unverified` → generic, so the repair sentinel outranks the unverified one; the no-mark downgrade sits AFTER the mark (referencing `$max_id_before` earlier is an unbound variable under `set -u`); the mid-run guard adds `[ "$ex_desc" != "$pre_desc" ]`, which the repair guard does not need because a pre-existing repair sentinel forces its own description while a pre-existing unverified one is deliberately replaced after reconciliation; the post-write gate is `if [ "$max_id_before" -ge 0 ]`, with the no-mark case handled before the POST instead; `replace_unknown_state()` is the shared writer for every path that cannot establish the head''s state and `replace_unknown_and_die()` wraps it for the two read refusals that were already non-zero exits; the reconciliation witness is a count of rows whose description equals `$UNVERIFIED_DESC`; `pre_id`/`ex_id` extend the mid-run changed-row comparison to row IDENTITY, since two sentinel POSTs are byte-identical by design (the combined endpoint carries `id` — measured 2026-08-29 at 1.27.1, head 736649b3, 8 rows, ids 14..30); `--arg own "$desc"` excludes the job''s OWN row from both machine-sentinel selectors, which became necessary the moment the block started running for every write; `.description` is TYPE-TESTED before `startswith`, since `(.description // "")` does not replace a NUMBER and `startswith` then hard-errors, killing the whole count and losing a genuine verdict beside the malformed row; `repair_status_to()` is the shared retry-once writer for all three repair sites; tests `test_a_verdict_racing_a_PENDING_write_is_repaired_to_the_repair_sentinel`, `test_a_raced_PENDING_write_repairs_and_the_repair_SURVIVES_the_next_run`, `test_the_unverified_sentinel_is_a_FIXED_POINT_while_it_cannot_be_reconciled`, `test_the_unverified_sentinel_is_RECONCILED_AWAY_once_the_history_can_be_read`, `test_the_reconciliation_UPGRADES_to_the_repair_sentinel_when_a_verdict_is_BURIED`, `test_a_RETARGET_AFTER_the_POST_replaces_the_exemption`, `test_an_UNREADABLE_retarget_count_AFTER_the_POST_also_replaces_the_exemption`, `test_a_MALFORMED_description_row_does_not_kill_the_post_write_count`, `test_a_transport_failure_on_the_STATUS_READ_REPLACES_the_unknown_state`, each paired with a `test_MUTATION_…` proof that disarms the shipped clause it names through `_run_classify(mutate=…)`, whose count assertion is the binding. Two of those mutants restore `origin/main` verbatim, one restores the shape #742 withdrew, and the rest disarm clauses that have no predecessor because the blocks they gate are new — the section header in that file enumerates which is which, because "restores the exact predecessor" is true of only some of them.' +mechanics: '`UNVERIFIED_DESC` ("Exemption write could not be verified — re-post the verdict") beside `REPAIR_DESC`; `read_existing_verdict()` sets `ex_unverified` from a prefix match, retries the combined read once and on persistent failure calls `repair_status_to "$UNVERIFIED_DESC"` before `exit 1`; the RECONCILE block runs `page_statuses` and counts `Review-verdict:` rows with a non-null creator, clearing `ex_unverified`, upgrading to `ex_repair`, or carrying the sentinel forward on `ph_ok != yes`; the classification chain is `exempt` → `ex_repair` → `ex_unverified` → generic, so the repair sentinel outranks the unverified one; the no-mark downgrade sits AFTER the mark (referencing `$max_id_before` earlier is an unbound variable under `set -u`); the mid-run guard adds `[ "$ex_desc" != "$pre_desc" ]`, which the repair guard does not need because a pre-existing repair sentinel forces its own description while a pre-existing unverified one is deliberately replaced after reconciliation; the post-write gate is `if [ "$max_id_before" -ge 0 ]`, with the no-mark case handled before the POST instead; `replace_unknown_state()` is the shared writer for every path that cannot establish the head''s state and `replace_unknown_and_die()` wraps it for the two read refusals that were already non-zero exits; the reconciliation witness is a count of rows whose description equals `$UNVERIFIED_DESC`; `pre_id`/`ex_id` extend the mid-run changed-row comparison to row IDENTITY through a shared `row_replaced` flag, since two sentinel POSTs are byte-identical by design — and an id difference counts only when BOTH reads supplied one, because a response that omits `id` beside one that includes it would otherwise report a replacement that did not happen and make the run abstain over a row it had already declined (the combined endpoint carries `id` — measured 2026-08-29 at 1.27.1, head 736649b3, 8 rows, ids 14..30); `--arg own "$desc"` excludes the job''s OWN row from both machine-sentinel selectors, which became necessary the moment the block started running for every write; `.description` is TYPE-TESTED before `startswith`, since `(.description // "")` does not replace a NUMBER and `startswith` then hard-errors, killing the whole count and losing a genuine verdict beside the malformed row; `repair_status_to()` is the shared retry-once writer for all three repair sites; tests `test_a_verdict_racing_a_PENDING_write_is_repaired_to_the_repair_sentinel`, `test_a_raced_PENDING_write_repairs_and_the_repair_SURVIVES_the_next_run`, `test_the_unverified_sentinel_is_a_FIXED_POINT_while_it_cannot_be_reconciled`, `test_the_unverified_sentinel_is_RECONCILED_AWAY_once_the_history_can_be_read`, `test_the_reconciliation_UPGRADES_to_the_repair_sentinel_when_a_verdict_is_BURIED`, `test_a_RETARGET_AFTER_the_POST_replaces_the_exemption`, `test_an_UNREADABLE_retarget_count_AFTER_the_POST_also_replaces_the_exemption`, `test_a_MALFORMED_description_row_does_not_kill_the_post_write_count`, `test_a_transport_failure_on_the_STATUS_READ_REPLACES_the_unknown_state`, each paired with a `test_MUTATION_…` proof that disarms the shipped clause it names through `_run_classify(mutate=…)`, whose count assertion is the binding. Three of those mutants restore text this branch or `origin/main` actually shipped, and the rest disarm clauses that have no predecessor because the blocks they gate are new — the section header in that file enumerates which is which, because "restores the exact predecessor" is true of only some of them. ONE CLAUSE IS DELIBERATELY UNPROVEN and says so rather than being counted: the path-predicate failure branch, which fires only when `grep -c` exits above 1, has no fixture that can reach it — the same standing exception the post-write unusable-count arm already carries, and it is defence in depth behind a filter that makes the count numeric for every input a stub can pose.' --- `ci.verdict-write-retarget-fence` fenced the write and verified it afterwards. Five routes survived diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 10940f412..8172f790c 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -331,7 +331,7 @@ job it covers, because a proof reference that covers a fraction must not read as | `pr-checks.yml::decisions-guard` | a PR whose decision records fail lifecycle validation, whose catalog is stale, or whose kickoff file drifted | GUARD | `scripts/decisions_validate.py`, `scripts/build_decisions_catalog.py`, `scripts/check-kickoff-guard.sh` | those scripts' rows above | | `pr-checks.yml::prove-fix` | a PR whose `Proves:` trailer names a test that passes without the fix | GUARD | inline + `scripts/prove-fix.sh` | that script's row above | | `pr-checks.yml::script-tests` | a PR failing ruff or the `scripts/tests` suite — this job is the RUNNER for every `scripts/tests/` row above | GUARD | inline (the ruff population guards) + pytest | the suite it runs | -| `review-verdict.yml::set-verdict-status` | a MERGE, by withholding the branch-protection-required `review-verdict/h10` status | GUARD | inline | `scripts/tests/test_pr_changed_files.py`, which EXECUTES the shipped `run:` body: it guards the changed-file derivation, the exemption classification, the timeline fence and — since #849 — the post-write verification state machine, each `test_MUTATION_…` there restoring the exact predecessor clause through `_run_classify(mutate=…)`. It does NOT cover the runner's step wiring, which no local test can reach | +| `review-verdict.yml::set-verdict-status` | a MERGE, by withholding the branch-protection-required `review-verdict/h10` status | GUARD | inline | `scripts/tests/test_pr_changed_files.py`, which EXECUTES the shipped `run:` body: it guards the changed-file derivation, the exemption classification, the timeline fence and — since #849 — the post-write verification state machine, each `test_MUTATION_…` there disarming through `_run_classify(mutate=…)` the shipped clause it names, bound by a count assertion — three restore text that actually shipped and the rest are counterfactual, which that file's own section header enumerates, and the one clause with no reachable fixture is named there rather than counted. It does NOT cover the runner's step wiring, which no local test can reach | ### The four jobs with no dropped-step guard, decided per job (ersatztv#786) diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index adab949fb..00c7cdc93 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -956,6 +956,12 @@ url = [a for a in args if a.startswith("http")][-1] out = pathlib.Path(os.environ["STUB_DIR"]) if "-X" in args and args[args.index("-X") + 1] == "POST": + if os.environ.get("STUB_POST_FAILS") == "1": + # EVERY POST FAILS, retries included. `gh` is `curl -sf`, so an HTTP error is exit 22 with + # empty stdout. Without this the write helpers' failure arms are unreachable — and one of + # them reported SUCCESS after both attempts failed, which is what let a caller exit 0 + # believing the head had been marked. + sys.exit(22) payload = args[args.index("-d") + 1] (out / "posted.json").write_text(payload) (out / "posted_url.txt").write_text(url) @@ -1406,6 +1412,28 @@ if "/statuses/" in url: n_logical = int(logical.read_text()) if logical.exists() else 1 pages = json.loads(snapshot.read_text()) if snapshot.exists() else [[]] + if hist_page > 1 and mode == "reconcile-page2-error" and n_logical == 1: + # PAGE 2 FAILS ON THE FIRST LOGICAL READ — the reconciliation walk — while page 1, which + # carries the seeded sentinel, is served. That separates the two halves of the trust + # condition: `ph_ok` is `no` and `witness` is 1, so a mutant that drops only the completeness + # operand still clears, and a mutant that drops only the witness operand does not. Against a + # fixture where BOTH are false, `if false` disarms two guards at once and isolates neither. + sys.exit(22) + if hist_page == 1 and mode == "postwrite-page1-error": + # FAILS THE POST-WRITE WALK ONLY (ersatztv#849 round 2). `premark-page1-error` fails the + # FIRST logical read, which is the mark; the repair floor needs a run that got its mark, made + # its write, and THEN could not read the history back. Both attempts of the second logical + # read fail, so the retry cannot rescue it. + # THE THRESHOLD IS DERIVED FROM WHERE THE REQUESTS FALL, not picked: `page_statuses` asks + # for page 1 exactly once per successful walk, so the mark's walk is request 0 and the + # post-write walk is requests 1 and 2 (the second being its retry). `>= 1` therefore lets the + # mark be established and fails the post-write read outright, which is the arrangement the + # repair floor needs and the one `premark-page1-error` cannot produce. + ctr = out / "p1_attempts_pw.txt" + seen = int(ctr.read_text()) if ctr.exists() else 0 + ctr.write_text(str(seen + 1)) + if seen >= 1: + sys.exit(22) if hist_page == 1 and mode == "premark-page1-error": # PAGE 1 ITSELF FAILS, for the whole first logical read (both attempts), so the walk returns # nothing at all and the mark must be abandoned. Counted in its own file because the logical @@ -1543,6 +1571,52 @@ if "/status" in url: ] print(json.dumps({"state": "pending", "total_count": len(mid_rows), "statuses": mid_rows})) sys.exit(0) + if mode == "malformed-row-beside-verdict": + # A SCALAR IN `.statuses` BESIDE A REAL ROW. `.statuses` is an array and `total_count` agrees, + # so the response passes the shape gate; it is the ELEMENTS that cannot be read. An untyped + # `select(.context == $c)` hard-errors on the scalar, jq exits 5, and under `set -e` the + # assignment takes the whole step down — before any path that could mark the head, while the + # `success` below stays authoritative. + rows = [7, {"context": "review-verdict/h10", "status": "success", + "creator": {"login": "mallory"}, + "description": "Review-verdict: MERGEABLE @ a9e3e23 (base: main)"}] + print(json.dumps({"state": "success", "total_count": len(rows), "statuses": rows})) + sys.exit(0) + if mode == "id-appears-on-second-read": + # THE SAME ROW, reported once WITHOUT `id` and once WITH it. Nothing else about it moves. An + # id comparison that does not require both sides to be present reads this as a replacement + # and makes the run abstain — over a row it had already declined to inherit. + ctr = out / "status_reads.txt" + n = int(ctr.read_text()) if ctr.exists() else 0 + ctr.write_text(str(n + 1)) + row = {"context": "review-verdict/h10", "status": "success", + "creator": {"login": "mallory"}, + "description": "Review-verdict: MERGEABLE @ a9e3e23 (base: main)"} + if n > 0: + row = dict(row, id=77) + rows = [{"context": "ci/decoy", "status": "pending", "creator": None, + "description": "unrelated"}, row] + print(json.dumps({"state": "pending", "total_count": len(rows), "statuses": rows})) + sys.exit(0) + if mode == "sentinel-replaced-mid-run": + # THE SAME SENTINEL TEXT AT TWO DIFFERENT IDS (ersatztv#849 round 2). Both reads return an + # unverified sentinel whose description is byte-identical — which is what a fixed point IS — + # so only the row id distinguishes "the row I snapshotted" from "a row another run wrote + # while I classified". Ids are carried here and nowhere else in this stub because this is the + # only fixture whose outcome turns on them. + ctr = out / "status_reads.txt" + n = int(ctr.read_text()) if ctr.exists() else 0 + ctr.write_text(str(n + 1)) + rows = [ + {"context": "ci/decoy", "status": "pending", "creator": None, "description": "unrelated"}, + {"id": 100 if n == 0 else 200, "context": "review-verdict/h10", "status": "pending", + "creator": None, + # FROM THE SHIPPED BODY, never a copy: the sentinel is a fixed point, so a stub carrying + # its own spelling would keep passing after the workflow reworded its own. + "description": os.environ["STUB_UNVERIFIED_DESC"]}, + ] + print(json.dumps({"state": "pending", "total_count": len(rows), "statuses": rows})) + sys.exit(0) if mode.startswith("sentinel-appears-on-read:"): # A repair sentinel written by ANOTHER, overlapping run between this job's first read and its # last-moment re-read (ersatztv#706 round 3). Creator is null: the sentinel is machine-written. @@ -1635,6 +1709,7 @@ def _run_classify( timeline_terminator: str = "null", status_empty_shape: str = "null", mutate: tuple[str, str] | None = None, + post_fails: bool = False, ): """Execute the workflow's classify `run:` block with a stubbed enumeration script. @@ -1674,6 +1749,8 @@ def _run_classify( env["STUB_HISTORY_MODE"] = history_mode env["STUB_HISTORY_CREATOR"] = history_creator env["STUB_HISTORY_EXTRA"] = json.dumps(history_extra) if history_extra else "" + env["STUB_UNVERIFIED_DESC"] = UNVERIFIED_DESC + env["STUB_POST_FAILS"] = "1" if post_fails else "" env["STUB_MIDRUN_CREATOR"] = midrun_creator env["STUB_PRE_ROW"] = pre_row env["STUB_MIDRUN_ROW"] = midrun_row @@ -1755,25 +1832,35 @@ def _run_classify( DOCS_ONLY = 'printf "docs/a.md\\ndocs/b.md\\n"\n' -def _assert_withheld(posted, r, why): +def _assert_withheld(tmp_path, r, why, expect_rc): """The gate refused to grant the exemption — which since ersatztv#849 means the head is MARKED, not left alone. - Every one of these tests asserted `posted is None`. That was right while "withhold" meant - "write nothing", and it became a fail-open assertion when it stopped meaning that: declining to - write protects a REAL verdict on the head and leaves a FORGED one, and this job's own red status - is not a required check, so branch protection still sees whatever was already there. + Every one of these tests asserted `posted is None`. That was right while "withhold" meant "write + nothing", and it became a fail-open assertion when it stopped meaning that: declining to write + protects a REAL verdict on the head and leaves a FORGED one, and this job's own red status is not + a required check, so branch protection still sees whatever was already there. - Asserted positively — state AND description — because `pending` alone does not distinguish the - sticky sentinel from the generic awaiting-verdict text a later run happily re-derives. + THE WHOLE POST SEQUENCE, not the last write. Inspecting only the final status would accept a job + that posted `success` and then repaired it — and the green interval IS part of the threat model + here, since branch protection and an already-scheduled auto-merge can both observe it. Exactly + one POST, and it is the sentinel. + + THE EXIT CODE IS PART OF THE CONTRACT and differs by path, so each caller states its own rather + than inheriting a default: the two read refusals were already non-zero exits before this change + and stay red, while the fence branch abstains cleanly. A single default would let one path's + regression hide behind the other's expectation. """ - assert posted is not None, ( - f"{why}: nothing was posted, so whatever {'{CONTEXT}'} this head already carries is standing " - f"unread and unre-derived.\n{r.stdout[-900:]}" + seq = _posted_sequence(tmp_path) + assert len(seq) == 1, ( + f"{why}: expected exactly one status write — the sentinel — got {seq}. Nothing posted at all " + f"means whatever this head already carries is standing unread; more than one means a green " + f"was published and taken back.\n{r.stdout[-900:]}" ) - assert posted["state"] == "pending" and posted["description"] == UNVERIFIED_DESC, ( - f"{why}: expected the unverified-write sentinel, got {posted}\n{r.stdout[-900:]}" + assert seq[0]["state"] == "pending" and seq[0]["description"] == UNVERIFIED_DESC, ( + f"{why}: expected the unverified-write sentinel, got {seq[0]}\n{r.stdout[-900:]}" ) + assert r.returncode == expect_rc, f"{why}: expected exit {expect_rc}, got {r.returncode}\n{r.stdout[-900:]}" def test_a_FAILING_enumeration_withholds_the_exemption_even_when_stdout_looks_docs_only(tmp_path): @@ -3249,7 +3336,9 @@ def test_the_head_arm_DEFERS_to_the_trust_guard_when_the_second_count_fails(tmp_ timeline_mode="trusted-then-unreadable", push_mode="stable:3", ) - _assert_withheld(posted, r, "test_the_head_arm_DEFERS_to_the_trust_guard_when_the_second_count_fails") + _assert_withheld( + tmp_path, r, "test_the_head_arm_DEFERS_to_the_trust_guard_when_the_second_count_fails", expect_rc=0 + ) assert "Could not establish a trusted retarget/push count" in r.stdout, ( f"the untrusted re-count was not reported as such. Log:\n{r.stdout[-900:]}" ) @@ -3387,7 +3476,7 @@ def test_an_EMPTY_FIRST_page_is_untrusted_WHICHEVER_empty_shape_it_is(tmp_path, timeline_mode="empty-first-page", timeline_terminator=terminator, ) - _assert_withheld(posted, r, "test_an_EMPTY_FIRST_page_is_untrusted_WHICHEVER_empty_shape_it_is") + _assert_withheld(tmp_path, r, "test_an_EMPTY_FIRST_page_is_untrusted_WHICHEVER_empty_shape_it_is", expect_rc=0) def test_a_timeline_row_with_NO_READABLE_TYPE_is_not_counted_as_nothing(tmp_path): @@ -3399,7 +3488,7 @@ def test_a_timeline_row_with_NO_READABLE_TYPE_is_not_counted_as_nothing(tmp_path IT extracts, which the walk gating the write did not have. """ posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode="untyped-rows") - _assert_withheld(posted, r, "test_a_timeline_row_with_NO_READABLE_TYPE_is_not_counted_as_nothing") + _assert_withheld(tmp_path, r, "test_a_timeline_row_with_NO_READABLE_TYPE_is_not_counted_as_nothing", expect_rc=0) def test_the_head_fence_reports_the_OBSERVED_counts_in_its_notice(tmp_path): @@ -3456,7 +3545,7 @@ def test_an_UNTRUSTED_retarget_count_withholds_the_EXEMPTION(tmp_path, mode): """A `success` that cannot be shown to describe the PR's current base must not be written. An absent required check blocks the merge, which is the safe direction.""" posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode=mode) - _assert_withheld(posted, r, "test_an_UNTRUSTED_retarget_count_withholds_the_EXEMPTION") + _assert_withheld(tmp_path, r, "test_an_UNTRUSTED_retarget_count_withholds_the_EXEMPTION", expect_rc=0) @pytest.mark.parametrize("mode", ["unreadable", "transport-error"]) @@ -3665,7 +3754,7 @@ def test_an_UNTRUSTED_count_withholds_the_exemption_BY_THAT_BRANCH(tmp_path): """Round 2 test-gap: the existing untrusted-count test asserted only "posted nothing", which a crash also produces. Assert the discriminator and a clean exit.""" posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode="unreadable") - _assert_withheld(posted, r, "test_an_UNTRUSTED_count_withholds_the_exemption_BY_THAT_BRANCH") + _assert_withheld(tmp_path, r, "test_an_UNTRUSTED_count_withholds_the_exemption_BY_THAT_BRANCH", expect_rc=0) assert r.returncode == 0, f"the job died rather than declining cleanly: {r.stderr[-800:]}" # The wording covers BOTH axes since ersatztv#803 — one walk certifies one trust flag, so an # unreadable page abandons the push count and the retarget count together. @@ -4349,7 +4438,7 @@ def test_a_terminator_on_PAGE_ONE_does_not_certify_a_zero_retarget_count(tmp_pat """ posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode="empty-first-page") assert r.returncode == 0, r.stderr - _assert_withheld(posted, r, "test_a_terminator_on_PAGE_ONE_does_not_certify_a_zero_retarget_count") + _assert_withheld(tmp_path, r, "test_a_terminator_on_PAGE_ONE_does_not_certify_a_zero_retarget_count", expect_rc=0) assert "trusted=no" in r.stdout, ( f"the exemption was withheld, but not because the count was untrusted:\n{r.stdout[-800:]}" ) @@ -4373,7 +4462,9 @@ def test_a_SECOND_PAGE_of_statuses_refuses_to_conclude_that_no_verdict_exists(tm the list is longer than one page and the verdict may be beyond it. """ posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), status_mode="twopage") - _assert_withheld(posted, r, "test_a_SECOND_PAGE_of_statuses_refuses_to_conclude_that_no_verdict_exists") + _assert_withheld( + tmp_path, r, "test_a_SECOND_PAGE_of_statuses_refuses_to_conclude_that_no_verdict_exists", expect_rc=1 + ) assert "page 2" in (r.stdout + r.stderr).lower(), ( f"nothing was posted, but not because of the page-2 completeness probe:\n{r.stdout[-800:]}" ) @@ -4426,7 +4517,7 @@ def test_an_UNREADABLE_page_2_refuses_to_conclude_that_no_verdict_exists(tmp_pat that cannot be read justifies nothing. """ posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), status_mode=mode) - _assert_withheld(posted, r, "test_an_UNREADABLE_page_2_refuses_to_conclude_that_no_verdict_exists") + _assert_withheld(tmp_path, r, "test_an_UNREADABLE_page_2_refuses_to_conclude_that_no_verdict_exists", expect_rc=1) assert "page 2" in (r.stdout + r.stderr).lower(), ( f"nothing was posted, but not because of the page-2 probe:\n{r.stdout[-800:]}" ) @@ -4951,6 +5042,17 @@ def test_an_UNREADABLE_history_page_2_also_repairs_rather_than_leaving_green(tmp # text verbatim. # * `test_MUTATION_a_GENERIC_pending_...` restores the shape #742 attempted and WITHDREW, not # `main` — which had no downgrade at all. +# * `test_MUTATION_restoring_the_SUCCESS_only_no_mark_downgrade_...` restores the predicate this +# branch itself shipped one commit earlier, which is where a cold review found it surviving the +# whole suite: the nearest existing proof mutated the DESCRIPTION the downgrade writes, not its +# SCOPE, and its fixture ran a succeeding enumeration, so `state=success` there and the +# `success`-only predecessor fired identically. +# +# TWO GUARDS HERE ARE OUTCOME-REDUNDANT AND WOULD OTHERWISE HIDE EACH OTHER: the `$own` exclusions +# and the no-op-repair skip both suppress the same duplicate POST, so mutating either alone leaves +# the post sequence unchanged. What the exclusions alone decide is the REPORT — without them a run +# counts its own row and tells a reviewer their verdict was overwritten when nothing raced it — so +# their proofs assert the LOG. That is the honest discriminator, not a weaker one. # * the `if false` mutants disarm clauses that have NO predecessor, because the blocks they gate # are new. Disarming the condition isolates the DECISION from the round-trip beside it, which # deleting the block would not; they are counterfactual mutants and are sound as such. @@ -5238,25 +5340,30 @@ def test_MUTATION_reconciling_on_an_UNREADABLE_history_re_exempts_a_head(tmp_pat pending. Treating it as "nothing buried" is the fail-open, and it is a tempting simplification because the happy path looks identical. - The mutant disarms the trust test. BE EXACT ABOUT WHAT THE FIXTURE SHOWS, because an earlier - version of this docstring claimed it "makes the reconciliation walk fail while a verdict IS - present in the history" — `premark-page1-error` serves ordinary rows and no verdict, so that was - a description of a different fixture. What is shown here is narrower and sufficient: on a walk - that could not be read, the shipped code carries the sentinel forward and the mutant clears it - and grants the exemption. That the cleared sentinel can be sitting on a real verdict is shown by + THE MUTATION ISOLATES THE COMPLETENESS OPERAND, which needs a fixture where the OTHER operand + is satisfied. `reconcile-page2-error` serves page 1 — carrying the seeded sentinel, so the + witness is 1 — and fails page 2 of the reconciliation walk, so `ph_ok` is `no`. Dropping the + completeness operand therefore clears the sentinel over a list the job knows it did not finish + reading, which is where a buried verdict would be. An earlier version used a fixture with BOTH + operands false and mutated the whole condition to `if false`, which disarms two guards at once + and isolates neither. + + That the cleared sentinel can be sitting on a real verdict is shown by `test_the_reconciliation_UPGRADES_to_the_repair_sentinel_when_a_verdict_is_BURIED`, which is the test that supplies one. """ + seed = [_sentinel_row()] posted, r = _run_classify( tmp_path / "fixed", _emitting("docs/a.md"), status_mode="existing:pending", status_creator=None, status_desc=UNVERIFIED_DESC, - history_mode="premark-page1-error", + history_mode="reconcile-page2-error", + history_extra=seed, ) assert posted is not None and posted["description"] == UNVERIFIED_DESC, ( - f"an unreadable reconciliation did not carry the sentinel forward: {posted}\n{r.stdout[-900:]}" + f"an incomplete reconciliation did not carry the sentinel forward: {posted}\n{r.stdout[-900:]}" ) mutant, rm = _run_classify( @@ -5265,10 +5372,11 @@ def test_MUTATION_reconciling_on_an_UNREADABLE_history_re_exempts_a_head(tmp_pat status_mode="existing:pending", status_creator=None, status_desc=UNVERIFIED_DESC, - history_mode="premark-page1-error", + history_mode="reconcile-page2-error", + history_extra=seed, mutate=( - '\n if [ "$ph_ok" != yes ] || [ "$witness" -eq 0 ]; then\n', - "\n if false; then\n", + '[ "$ph_ok" != yes ] || [ "$witness" -eq 0 ]; then', + '[ "$witness" -eq 0 ]; then', ), ) assert mutant is not None and mutant["state"] == "success", ( @@ -5498,6 +5606,477 @@ def test_MUTATION_declining_to_replace_an_unreadable_combined_read_leaves_the_fo ) +def test_MUTATION_a_page_2_refusal_that_only_EXITS_leaves_the_head_unmarked(tmp_path): + """The page-2 completeness probe refuses AND replaces (ersatztv#849 round 2). + + It was excluded from the replacement on the reasoning that the probe fires when NO row for this + context was on page 1, so there is no green of any provenance to leave standing. That is + self-contradictory: the only reason page 2 is read is that the row MAY be beyond page 1, which + the probe's own message says. + + `twopage` is a head whose status list runs past one page with no `h10` on either — the shape that + makes "no verdict exists" unestablishable. + """ + posted, r = _run_classify(tmp_path / "fixed", _emitting("docs/a.md"), status_mode="twopage") + assert posted is not None and posted["description"] == UNVERIFIED_DESC, ( + f"a page-2 refusal left the head unmarked: {posted}\n{r.stdout[-900:]}" + ) + + mutant, rm = _run_classify( + tmp_path / "mutant", + _emitting("docs/a.md"), + status_mode="twopage", + mutate=( + 'replace_unknown_and_die "${CONTEXT} was not on page 1 of the statuses for ${SHA:0:7},' + " but page 2 carries ${more_len} more row(s) — the list is longer than one page, so a" + ' verdict of ANY provenance may be sitting beyond it where this job cannot read it." ;;', + "exit 1 ;;", + ), + ) + assert mutant is None, ( + "the mutant still posted, so this fixture does not reach the replacement through the page-2 " + f"refusal and the assertion above proves nothing about it: {mutant}\n{rm.stdout[-900:]}" + ) + + +def test_MUTATION_an_untrusted_fence_that_only_ABSTAINS_leaves_the_declined_row_current(tmp_path): + """The untrusted-fence branch writes rather than abstains (ersatztv#849 round 2). + + It is reached only AFTER the classification declined to inherit whatever `h10` the head carries — + that is why it is re-deriving — so posting nothing leaves the declined row current, and no + retarget or push need have occurred, so no successor run is guaranteed. Its message used to say + the context "stays absent", which is true only of a head that had none. + """ + posted, r = _run_classify(tmp_path / "fixed", _emitting("docs/a.md"), timeline_mode="unreadable") + assert posted is not None and posted["description"] == UNVERIFIED_DESC, ( + f"an untrusted fence left the head unmarked: {posted}\n{r.stdout[-900:]}" + ) + + mutant, rm = _run_classify( + tmp_path / "mutant", + _emitting("docs/a.md"), + timeline_mode="unreadable", + mutate=( + 'replace_unknown_state "Could not establish a trusted retarget/push count for PR #${PR}' + " (before=${retargets_before_ok}, after=${rt_ok}), so an exemption 'success' cannot be" + " shown to have been computed against the PR's current base, nor at a single head. NOTE a" + " later run only helps if the cause was transient — a PR whose timeline exceeds the page" + ' cap will fail this way on every run, and needs a human verdict."', + ":", + ), + ) + assert mutant is None, ( + f"the mutant still posted, so the replacement does not come from that branch: {mutant}\n{rm.stdout[-900:]}" + ) + + +def test_MUTATION_dropping_the_ROW_ID_from_the_mid_run_comparison_overwrites_another_runs_sentinel( + tmp_path, +): + """Two sentinels are byte-identical by design, so only the row id can tell them apart. + + This run reconciles a pre-existing sentinel away — which is the case the guard must NOT fire on, + and the reason it cannot simply abstain whenever a sentinel is present — and then classifies + docs-only. Between the two reads another run replaces that sentinel with its own. The + description is unchanged, so the state/creator/description triple sees nothing; the id moved. + + Without the id clause the run posts its exemption over a sentinel another run had just written, + which is a marker that something on this head is unchecked being replaced by a status a later run + re-derives. + """ + # THE SEEDED SENTINEL CARRIES THE ID THE COMBINED READ REPORTS AT THE FIRST READ (100), because + # the reconciliation witness now matches that id rather than the description. A seed with an + # unrelated id would make this run carry the sentinel forward instead of reconciling it, and the + # guard under test would never be reached. + seed = [ + {"id": 3900, "context": "ci/other", "status": "success", "creator": None, "description": "unrelated"}, + _sentinel_row(100), + ] + posted, r = _run_classify( + tmp_path / "fixed", + _emitting("docs/a.md"), + status_mode="sentinel-replaced-mid-run", + history_extra=seed, + ) + assert posted is None, ( + f"a sentinel written by another run mid-classification was overwritten: {posted}\n{r.stdout[-900:]}" + ) + assert "written on" in (r.stdout + r.stderr), f"abstained, but silently:\n{r.stdout[-900:]}" + + mutant, rm = _run_classify( + tmp_path / "mutant", + _emitting("docs/a.md"), + status_mode="sentinel-replaced-mid-run", + history_extra=seed, + mutate=( + 'if [ -n "$ex_id" ] && [ -n "$pre_id" ] && [ "$ex_id" != "$pre_id" ]; then', + "if false; then", + ), + ) + assert mutant is not None and mutant["state"] == "success", ( + "the mutant did not overwrite the sentinel, so the id clause is not what stops it and the " + f"assertion above proves nothing about it: {mutant}\n{rm.stdout[-900:]}" + ) + + +def test_MUTATION_removing_the_repair_FLOOR_downgrades_the_repair_sentinel(tmp_path): + """The repair may never write a description weaker than the one this run decided. + + Widening the post-write gate to every write means the block now also runs after a carry-forward + write of `$REPAIR_DESC`. One transient post-write read then rewrote that head with the strictly + weaker, machine-clearable sentinel — reversing the ordering the classification chain states, and + depending on a later reconciliation to put it back. + """ + posted, r = _run_classify( + tmp_path / "fixed", + _emitting("docs/a.md"), + status_mode="existing:pending", + status_creator=None, + status_desc=REPAIR_DESC, + history_mode="postwrite-page1-error", + ) + seq = _posted_sequence(tmp_path / "fixed") + assert seq and seq[-1]["description"] == REPAIR_DESC, ( + f"the repair sentinel was downgraded on an unreadable post-write read: {seq}\n{r.stdout[-900:]}" + ) + + _run_classify( + tmp_path / "mutant", + _emitting("docs/a.md"), + status_mode="existing:pending", + status_creator=None, + status_desc=REPAIR_DESC, + history_mode="postwrite-page1-error", + mutate=(' if [ "$desc" = "$REPAIR_DESC" ]; then repair_desc="$REPAIR_DESC"; fi', " :"), + ) + mseq = _posted_sequence(tmp_path / "mutant") + assert mseq and mseq[-1]["description"] == UNVERIFIED_DESC, ( + "the mutant did not downgrade, so the floor is not what preserves the repair sentinel and " + f"the assertion above proves nothing about it: {mseq}" + ) + + +def test_an_OBSERVED_retarget_MARKS_a_row_this_run_declined(tmp_path): + """Abstaining is a handoff only when there is nothing to hand off (ersatztv#849 round 3). + + The arm is right not to post its CLASSIFICATION — computed against a base the PR may no longer + target — but when the head already carries a row this run DECLINED to inherit, posting nothing + leaves that row authoritative for the whole window until the successor finishes. And in the case + the head-arm's own message names, a PR's FIRST push, no successor is queued at all. + + Here `mallory` is off `$H10_REVIEWERS`, so the existing `success` is declined rather than + inherited, and `moves:0,1` retargets the PR mid-classification. + """ + posted, r = _run_classify( + tmp_path, + _emitting("docs/a.md"), + status_mode="existing:success", + status_creator="mallory", + timeline_mode="moves:0,1", + ) + assert posted is not None and posted["description"] == UNVERIFIED_DESC, ( + "a declined `success` was left authoritative while this run abstained on an observed " + f"retarget: {posted}\n{r.stdout[-900:]}" + ) + assert "was retargeted while this job was classifying" in (r.stdout + r.stderr), ( + f"marked, but not by the retarget arm:\n{r.stdout[-900:]}" + ) + + +def test_positive_control_an_OBSERVED_retarget_on_an_UNMARKED_head_still_posts_nothing(tmp_path): + """The scoping, asserted rather than assumed. + + On a head that carries nothing there is nothing to leave standing, so the arm must stay silent — + a write there would be noise on the commonest path in this job, and it is also what + `test_a_RETARGET_DURING_the_run_posts_NOTHING` above depends on. + """ + posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode="moves:0,1") + assert posted is None, f"an empty head was marked on an observed retarget: {posted}" + + +def test_MUTATION_not_marking_the_declined_row_leaves_it_authoritative(tmp_path): + """Disarming the scope test, which is the whole decision this helper makes.""" + _run_classify( + tmp_path / "mutant", + _emitting("docs/a.md"), + status_mode="existing:success", + status_creator="mallory", + timeline_mode="moves:0,1", + mutate=('if [ -z "$pre_state" ]; then return 0; fi', "return 0"), + ) + seq = _posted_sequence(tmp_path / "mutant") + assert seq == [], ( + "the mutant still marked the head, so the scope test is not what produces the write and the " + f"test above proves nothing about it: {seq}" + ) + + +def test_a_MALFORMED_combined_ROW_does_not_kill_the_read_before_anything_can_mark_the_head(tmp_path): + """`.statuses` being an array was checked; its ELEMENTS were not. + + A scalar beside a real row makes an untyped `select(.context == $c)` hard-error, jq exits 5, and + under `set -euo pipefail` the unguarded assignment takes the step down — before any of the paths + that replace an unknown state, and with an off-list `success` still authoritative on the head. + The job goes red, but its own status is not a required check. + + The type-safe filter drops the unreadable element and judges what is left, so the real row is + still found, still declined (`mallory` is off the allow-list), and still re-derived. + """ + posted, r = _run_classify(tmp_path / "fixed", _emitting("docs/a.md"), status_mode="malformed-row-beside-verdict") + assert posted is not None, f"a malformed neighbour row stopped the job writing anything: {r.stdout[-900:]}" + assert posted["state"] == "success" and posted["description"].startswith("Exempt:"), ( + f"expected the off-list row to be re-derived into the docs-only exemption; got {posted}" + ) + + mutant, rm = _run_classify( + tmp_path / "mutant", + _emitting("docs/a.md"), + status_mode="malformed-row-beside-verdict", + mutate=( + '\'[(.statuses // [])[] | select(type == "object") | select(.context? == $c)] | first // {}\') || row=""', + "'[(.statuses // [])[] | select(.context == $c)] | first // {}')", + ), + ) + assert mutant is None and rm.returncode != 0, ( + "the mutant did not die on the malformed row, so the type test is not what keeps this read " + f"alive: {mutant} rc={rm.returncode}" + ) + + +def test_a_generic_PENDING_with_no_mark_also_becomes_the_sentinel(tmp_path): + """The no-mark downgrade covers every re-derivable write, not only the exemption. + + The damaging PR is one that IS exemptible and got the generic `pending` only from a transient + enumeration failure. Its description carries no marker, the post-write check does not run without + a mark, so a verdict landing in the write window is buried and the NEXT run re-derives that + `pending` into the exemption with the human row below its own mark. + + The fixture is that PR: the enumerator exits 1 (generic `pending`) AND the status history cannot + be read (no mark). + """ + posted, r = _run_classify( + tmp_path, + "#!/usr/bin/env bash\n" + DOCS_ONLY + "exit 1\n", + history_mode="premark-page1-error", + ) + assert posted is not None and posted["description"] == UNVERIFIED_DESC, ( + "a generic `pending` nothing could verify was posted with its re-derivable description " + f"intact: {posted}\n{r.stdout[-900:]}" + ) + + +def test_MUTATION_restoring_the_SUCCESS_only_no_mark_downgrade_leaves_a_re_derivable_pending(tmp_path): + """The exact predecessor from this branch's own previous commit, restored. + + This is the mutation the round-2 suite did not have: the nearest proof mutated the DESCRIPTION + the downgrade writes, not its SCOPE, and its fixture ran a succeeding enumeration — so + `state=success` there and the `success`-only predicate fired identically. Nothing reached the + downgrade with `state=pending`, and the predecessor survived the whole suite. + """ + posted, r = _run_classify( + tmp_path / "mutant", + "#!/usr/bin/env bash\n" + DOCS_ONLY + "exit 1\n", + history_mode="premark-page1-error", + mutate=( + '[ "$max_id_before" -lt 0 ] && [ "$desc" != "$REPAIR_DESC" ]; then', + '[ "$state" = "success" ] && [ "$max_id_before" -lt 0 ]; then', + ), + ) + assert posted is not None and posted["description"] != UNVERIFIED_DESC, ( + "the `success`-only predecessor still wrote the sentinel, so this fixture does not reach the " + f"downgrade with state=pending and the test above proves nothing about its scope: {posted}" + f"\n{r.stdout[-900:]}" + ) + + +def test_MUTATION_dropping_the_no_op_repair_SKIP_re_posts_what_is_already_there(tmp_path): + """The skip, isolated: a repair that would write what this run already wrote is not a repair. + + The fixture reaches it the ordinary way — a carry-forward `$REPAIR_DESC` write whose post-write + walk then fails, so the floor pins `repair_desc` to the description just POSTed. + """ + _run_classify( + tmp_path / "fixed", + _emitting("docs/a.md"), + status_mode="existing:pending", + status_creator=None, + status_desc=REPAIR_DESC, + history_mode="postwrite-page1-error", + ) + assert len(_posted_sequence(tmp_path / "fixed")) == 1, ( + f"the shipped code re-posted: {_posted_sequence(tmp_path / 'fixed')}" + ) + + _run_classify( + tmp_path / "mutant", + _emitting("docs/a.md"), + status_mode="existing:pending", + status_creator=None, + status_desc=REPAIR_DESC, + history_mode="postwrite-page1-error", + mutate=('if [ "$raced" -gt 0 ] && [ "$repair_desc" = "$desc" ]; then', "if false; then"), + ) + mseq = _posted_sequence(tmp_path / "mutant") + assert len(mseq) == 2 and mseq[0] == mseq[1], ( + "the mutant did not duplicate the row, so the skip is not what suppresses it and the " + f"assertion above proves nothing about it: {mseq}" + ) + + +def test_MUTATION_dropping_the_OWN_row_exclusion_reports_a_race_that_did_not_happen(tmp_path): + """`--arg own "$desc"` keeps this job from counting the row it has just written. + + The two exclusions are OUTCOME-redundant with the no-op skip above — drop one and the other + still suppresses the duplicate POST — which is exactly the shape where two guards hide each + other. What the exclusion alone decides is the REPORT, and after the skip learned to keep the + human `::error::` that report is a false alarm: a run whose own carry-forward row is counted + tells a reviewer their verdict was overwritten when nothing raced it. + + So this asserts the log, not the post sequence, and that is the honest discriminator rather than + a weaker one. The head carries `$REPAIR_DESC`, the history is otherwise empty (mark 0), and this + run's own POST lands above the mark. + """ + _, r = _run_classify( + tmp_path / "fixed", + _emitting("docs/a.md"), + status_mode="existing:pending", + status_creator=None, + status_desc=REPAIR_DESC, + ) + # `::error::A human` and not the bare phrase "was overwritten": the classification's own REASON + # string for a carry-forward run also contains that phrase, so matching it would report the + # shipped code as failing on text it is supposed to print. + assert "::error::A human" not in (r.stdout + r.stderr), ( + f"the shipped code reported a race against its own row:\n{r.stdout[-900:]}" + ) + + _, rm = _run_classify( + tmp_path / "mutant", + _emitting("docs/a.md"), + status_mode="existing:pending", + status_creator=None, + status_desc=REPAIR_DESC, + mutate=( + 'or ((.creator == null) and ((.description // "") == $rd)\n' + ' and ((.description // "") != $own))', + 'or ((.creator == null) and ((.description // "") == $rd))', + ), + ) + assert "::error::A human" in (rm.stdout + rm.stderr), ( + "the mutant did not report a false race, so the `$own` exclusion on the repair-sentinel arm " + f"is not what prevents it:\n{rm.stdout[-900:]}" + ) + + +def test_MUTATION_dropping_the_OWN_exclusion_on_the_UNVERIFIED_arm_reports_a_phantom_other_run(tmp_path): + """The twin, on the arm that counts the reconcilable sentinel. + + A run carrying the unverified sentinel forward POSTs it, then its own row sits above the mark. If + the arm does not exclude it, the job reports that ANOTHER run recorded an unverified write on this + head — a second run that does not exist. + """ + _, r = _run_classify( + tmp_path / "fixed", + _emitting("docs/a.md"), + status_mode="existing:pending", + status_creator=None, + status_desc=UNVERIFIED_DESC, + ) + assert "another run recorded an unverified write" not in (r.stdout + r.stderr), ( + f"the shipped code reported a phantom second run:\n{r.stdout[-900:]}" + ) + + _, rm = _run_classify( + tmp_path / "mutant", + _emitting("docs/a.md"), + status_mode="existing:pending", + status_creator=None, + status_desc=UNVERIFIED_DESC, + mutate=( + 'select((.creator == null) and ((.description // "") == $ud)\n' + ' and ((.description // "") != $own))] | length\')', + 'select((.creator == null) and ((.description // "") == $ud))] | length\')', + ), + ) + assert "another run recorded an unverified write" in (rm.stdout + rm.stderr), ( + "the mutant did not report a phantom run, so the `$own` exclusion on the unverified arm is " + f"not what prevents it:\n{rm.stdout[-900:]}" + ) + + +def test_a_FAILED_sentinel_write_on_the_fence_path_FAILS_the_job(tmp_path): + """The write helper reports whether it wrote, and the fence caller acts on it. + + Its first version ended the failure arm with a successful `echo`, so it returned 0 after BOTH + POST attempts failed and the caller's `exit 0` beside it reported an abstention that had not + happened — while whatever the head carried stayed authoritative. + """ + posted, r = _run_classify(tmp_path / "fixed", _emitting("docs/a.md"), timeline_mode="unreadable", post_fails=True) + assert posted is None, "the stub was supposed to reject every POST" + assert r.returncode != 0, ( + "the job reported a clean abstention while the head was left unmarked and its POSTs had all " + f"failed:\n{r.stdout[-900:]}" + ) + assert "COULD NOT WRITE THE UNVERIFIED SENTINEL" in (r.stdout + r.stderr), ( + f"the job went red, but not because the sentinel write failed:\n{r.stdout[-900:]}" + ) + + +def test_MUTATION_ignoring_the_write_result_reports_a_clean_abstention(tmp_path): + """The predecessor: `replace_unknown_state` followed by an unconditional `exit 0`.""" + _, rm = _run_classify( + tmp_path / "mutant", + _emitting("docs/a.md"), + timeline_mode="unreadable", + post_fails=True, + # MUTATES THE CALLER'S REACTION, not the `if` itself: dropping the `if` keyword leaves a + # dangling `then`/`fi` and the step dies on a syntax error, which is a red for the wrong + # reason. Turning the failure exit into a clean one is exactly the predecessor's OUTCOME and + # isolates the decision. + mutate=( + " # THE SENTINEL WRITE FAILED, so nothing marked this head and whatever it carries is still\n" + " # authoritative. Exiting 0 here would report an abstention that did not happen.\n" + " exit 1", + " exit 0", + ), + ) + assert rm.returncode == 0, ( + "the mutant still failed the job, so the caller is not what turns a failed write into a red " + f"run and the test above proves nothing about it: rc={rm.returncode}\n{rm.stdout[-900:]}" + ) + + +def test_an_ID_that_appears_on_only_ONE_read_is_not_a_replacement(tmp_path): + """One response omitting `id` beside one that includes it is not evidence of a mid-run write. + + The row, its state, its creator and its description are all unchanged; only the SERVER's + reporting differs. Treating that as a replacement makes the run abstain — leaving current a row + the classification had already declined to inherit, which is the direction that costs something. + """ + posted, r = _run_classify(tmp_path / "fixed", _emitting("docs/a.md"), status_mode="id-appears-on-second-read") + assert posted is not None, f"an asymmetric id report was read as a mid-run replacement:\n{r.stdout[-900:]}" + assert posted["state"] == "success" and posted["description"].startswith("Exempt:"), ( + f"expected the off-list row to be re-derived into the docs-only exemption; got {posted}" + ) + + +def test_MUTATION_comparing_ids_WITHOUT_requiring_both_makes_the_run_abstain(tmp_path): + """Disarming the presence guards alone, leaving the inequality.""" + mutant, rm = _run_classify( + tmp_path / "mutant", + _emitting("docs/a.md"), + status_mode="id-appears-on-second-read", + mutate=( + 'if [ -n "$ex_id" ] && [ -n "$pre_id" ] && [ "$ex_id" != "$pre_id" ]; then', + 'if [ "$ex_id" != "$pre_id" ]; then', + ), + ) + assert mutant is None, ( + "the mutant did not abstain, so the both-present requirement is not what keeps an asymmetric " + f"id report from reading as a replacement: {mutant}\n{rm.stdout[-900:]}" + ) + + # --- Workflow token scope (ersatztv#748) ------------------------------------------------------- # # Both assertions guard a property whose violation is SILENT and, for the first, unrecoverable.