fix(706,707,711): fence the review-verdict write on the timeline retarget count
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 11s
PR Gates / Docs update reminder (pull_request) Successful in 15s
PR Gates / decisions lifecycle (pull_request) Successful in 23s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 23s
review-verdict/h10 Awaiting review verdict for 8f6d4f4
Review verdict / Set review-verdict status (pull_request_target) Successful in 20s
PR Gates / Script tests (pytest) (pull_request) Successful in 1m29s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 1m26s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 16m56s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 20m59s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 22m41s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 11s
PR Gates / Docs update reminder (pull_request) Successful in 15s
PR Gates / decisions lifecycle (pull_request) Successful in 23s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 23s
review-verdict/h10 Awaiting review verdict for 8f6d4f4
Review verdict / Set review-verdict status (pull_request_target) Successful in 20s
PR Gates / Script tests (pytest) (pull_request) Successful in 1m29s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 1m26s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 16m56s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 20m59s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 22m41s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Three related defects in the `review-verdict/h10` gate, all surfaced by the cross-family review of PR #705. #706 race 1 — a stale run could overwrite a fresher verdict, permanently. The race was reproduced live rather than reasoned about (Gitea 1.25.4): with every other workflow stripped, probe PR #722 showed run 7520 (`opened`) finishing 20s AFTER run 7521 (`synchronize`) started. `pull_request_target` runs for one PR genuinely overlap, older finishing last. The issue proposed serializing with a non-cancelling concurrency group. That is REFUTED by measurement: with the group active, runs 7528/7529 still overlapped and 7528 ended 36s after 7529 began. A first probe appeared to show the group working — a negative control with no `concurrency:` key at all showed the same cancellations, revealing Gitea auto-cancels superseded `push` runs on its own and the probe had measured that, not the group. The auto-cancel does not extend to `pull_request_target`. The fix leaves the runs unserialized and instead makes an overtaken run decline to write: count `change_target_branch` events on the PR timeline at start and again just before the POST, and post nothing if the count moved. The COUNT is the key because the branch NAME is ABA-vulnerable (`main -> S -> main` reads `main` at both ends — how #698 route 1 forged its exemption). Abstaining is a handoff, not a stall: every retarget fires `edited`, so the event that makes a run abstain has already queued its successor. `updated_at` was rejected as the key precisely because it moves for comments/labels, which queue nothing. #706 race 2 — a human BLOCKED landing in the unclosable window between the pre-POST re-read and the POST was silently turned green. After an exemption `success` the job now re-reads the per-POST history and repairs its own status to `pending` if a human verdict appeared above a high-water mark taken just before the write. The repair is `pending`, never a copy of the human's state. The id comparison is load-bearing: a presence test would fire forever on a base-mismatched verdict and deadlock that PR's exemption. #707 — `pr-changed-files.sh` bound `.base.ref` and `.head.sha` across the enumeration but never `.base.sha`, so an ordinary advance of `main` mid-paging could drop a code path from an offset-paged diff and leave a complete-looking docs-only list. Now bound from the JSON already fetched (no new round trips). #711 — `.codex/` added to PROTECTED. It mirrors `.claude/hooks/` byte for byte, including the merge-consent hook, so the "a PR that can weaken the gate cannot exempt itself" rule had an incomplete path list. Latent today (untracked), live the moment anyone tracks it. Residuals are stated, not implied: a retarget inside the final round-trip, and the repair being itself a read-then-write. Gitea's status API has no compare-and-set, so neither reaches zero; both now fail toward `pending`. Tests: 398 pass in scripts/tests. Each new guard was mutation-checked — the fence's motion comparison, the untrusted-count gate, the repair POST and the id high-water mark were each neutered in turn and the intended test went red while its positive control stayed green. fixes #706 fixes #707 fixes #711 Decisions-Edit: yes
This commit is contained in:
@@ -183,6 +183,14 @@ jobs:
|
||||
set -euo pipefail
|
||||
|
||||
CONTEXT="review-verdict/h10"
|
||||
# The description this job writes when it repairs its own raced exemption (#706 race 2).
|
||||
# It is a SENTINEL, not just a message: `read_existing_verdict` recognises it, and the
|
||||
# classification below refuses to post `success` over it. Without that, the repair lasted
|
||||
# exactly one event — the next run saw a machine-written `pending`, re-derived it, and
|
||||
# posted `success` again, with its own freshly-taken high-water mark now ABOVE the human
|
||||
# row, so the post-write check stayed silent and the rejection went green a second time.
|
||||
# Found by cold review. Refusing here can only ever withhold an exemption, never grant one.
|
||||
REPAIR_DESC="Human verdict raced this exemption write — re-post the verdict"
|
||||
# Accounts whose PRs may merge without a human verdict. Renovate only — keep this list
|
||||
# minimal and explicit; every entry is an account that can land code unreviewed.
|
||||
BOTS="renovate"
|
||||
@@ -218,7 +226,23 @@ jobs:
|
||||
BOT_MANIFESTS='^(Directory\.Packages\.props|\.config/dotnet-tools\.json)$'
|
||||
# Paths where NEITHER exemption applies, because a change here can alter the gate itself,
|
||||
# what CI runs, or what the hooks enforce.
|
||||
PROTECTED='^(\.claude/|\.gitea/|\.husky/|scripts/|docker/ci/)'
|
||||
#
|
||||
# `.codex/` is listed alongside `.claude/` (ersatztv#711). `.codex/hooks/` is a
|
||||
# byte-identical mirror of `.claude/hooks/` — including `pretooluse-merge-consent.sh` —
|
||||
# generated as the Codex-side port of the same enforcement hooks. Without it the rule "a PR
|
||||
# that can weaken the gate must not exempt itself from the gate" was expressed as a path
|
||||
# list that had gone incomplete: editing `.claude/hooks/pretooluse-merge-consent.sh`
|
||||
# correctly voided the exemption while editing its `.codex/` twin did not. Today that is
|
||||
# LATENT rather than live — `.codex/` is untracked and gitignored, and a PR cannot touch a
|
||||
# path that is not in the repo — but it becomes live the moment anyone tracks it, which is
|
||||
# the natural instinct given `.claude/` is tracked. Listed now because the cost is one
|
||||
# alternation and the failure mode is silent.
|
||||
#
|
||||
# The list stays ENUMERATIVE rather than derived (e.g. "any dotted top-level directory
|
||||
# containing executable hooks"). A derived rule has to be evaluated against the PR's own
|
||||
# file list, which is the very thing being classified — more moving parts inside a security
|
||||
# predicate, to remove a maintenance burden that is one line per new tooling directory.
|
||||
PROTECTED='^(\.claude/|\.codex/|\.gitea/|\.husky/|scripts/|docker/ci/)'
|
||||
# Docs-only: prose and decision records. Deliberately narrower than the hook's pattern,
|
||||
# which also lets .claude/.gitea/.husky through — that carve-out is safe there only
|
||||
# because it falls through to a HUMAN PROMPT, whereas here it would post a green status
|
||||
@@ -330,6 +354,10 @@ jobs:
|
||||
# no `(base: …)`, and re-deriving over one would un-approve a genuinely reviewed head. Only
|
||||
# a base that is PRESENT and DIFFERENT is rejected, which is exactly the escape above.
|
||||
ex_human=no
|
||||
ex_repair=no
|
||||
case "$ex_desc" in
|
||||
"$REPAIR_DESC"*) ex_repair=yes ;;
|
||||
esac
|
||||
case "$ex_desc" in
|
||||
"Review-verdict:"*)
|
||||
if [ -n "$ex_creator" ]; then ex_human=yes; fi
|
||||
@@ -374,6 +402,72 @@ jobs:
|
||||
fi
|
||||
}
|
||||
|
||||
# --- The retarget fence (ersatztv#706 race 1) ----------------------------------------
|
||||
# THE PROBLEM THIS SOLVES. Two `pull_request_target` runs for one PR overlap, and the OLDER
|
||||
# one can finish LAST — so a run that classified against a base the PR no longer targets can
|
||||
# post its stale answer over a fresher run's correct one, permanently. Measured on this
|
||||
# instance rather than assumed: probe PR #722 run 7520 (`opened`) ran to completion 20s AFTER
|
||||
# run 7521 (`synchronize`) had started.
|
||||
#
|
||||
# WHY NOT A CONCURRENCY GROUP, which is the obvious answer and what #706 proposed. It does
|
||||
# not work here, also measured: with `concurrency: {group: …-${{ pr number }},
|
||||
# cancel-in-progress: false}` active on an identical probe, runs 7528 and 7529 still ran
|
||||
# CONCURRENTLY and 7528 ended 36s after 7529 began. Gitea 1.25.4 does auto-cancel superseded
|
||||
# `push` runs on a branch — a negative control with no `concurrency:` key at all showed that —
|
||||
# but that behaviour does NOT extend to `pull_request_target`. `cancel-in-progress: true` is
|
||||
# deliberately untried: cancellation is the one thing this workflow's own header refuses,
|
||||
# because a cancelled run leaves an EXEMPT PR statusless with nothing left to re-trigger it.
|
||||
#
|
||||
# WHY A COUNTER AND NOT THE BRANCH NAME. The attack is an ABA: `main → S → main`. Every
|
||||
# name-based check reads `main` at both ends and passes, which is exactly how route 1 got a
|
||||
# forged exemption. Gitea's issue timeline records each retarget as a `change_target_branch`
|
||||
# event with `old_ref`/`new_ref`; the COUNT of those events is monotonic and cannot alias.
|
||||
# Verified on the real route-1 reproduction (PR #703: two events, `main → probe698/base-S` at
|
||||
# 18:17:29 and back at 18:18:31) with a negative control (PR #717, never retargeted: zero).
|
||||
#
|
||||
# WHY ABSTAINING IS NOT A STALL — the property the whole design rests on. A retarget always
|
||||
# fires `edited`, which is in this workflow's `types:` (see the header). So the very event
|
||||
# that makes this run abstain has already queued a successor whose window opens after it.
|
||||
# Abstention hands off; it does not drop the PR. The induction terminates when retargeting
|
||||
# stops, and the last run has a clean window and writes the final answer. This is why the
|
||||
# fence does not reintroduce the statusless-exempt-PR failure that rules out cancellation:
|
||||
# it never stops a run from RUNNING, only from WRITING state it knows is stale.
|
||||
#
|
||||
# `updated_at` was considered as the key and rejected: it moves for comments and labels,
|
||||
# which fire none of this workflow's `types:`, so a run could abstain with no successor
|
||||
# coming — a real stall. The retarget count moves only for the mutation that actually
|
||||
# invalidates a classification, and that mutation always brings its own re-run.
|
||||
#
|
||||
# Completeness is a guard, not an assumption (`ci.paged-endpoint-completeness`): the count is
|
||||
# trusted ONLY when paging reached a validated EMPTY page. A short page, a non-array body, a
|
||||
# non-numeric length or the page cap all leave `rt_ok=no`, and an untrusted count is treated
|
||||
# below as "cannot tell" rather than as zero.
|
||||
count_retargets() {
|
||||
rt_count=0
|
||||
rt_ok=no
|
||||
local page=1 raw n m total=0
|
||||
while [ "$page" -le 20 ]; do
|
||||
raw=$(gh "$BASE_URL/repos/$REPO/issues/$PR/timeline?limit=50&page=${page}") || return 0
|
||||
if [ -z "${raw//[[:space:]]/}" ] || ! printf '%s' "$raw" | jq -e 'type == "array"' >/dev/null 2>&1; then
|
||||
return 0
|
||||
fi
|
||||
n=$(printf '%s' "$raw" | jq -r 'length')
|
||||
case "$n" in ''|*[!0-9]*) return 0 ;; esac
|
||||
if [ "$n" -eq 0 ]; then rt_ok=yes; rt_count=$total; return 0; fi
|
||||
m=$(printf '%s' "$raw" | jq -r '[.[] | select(.type == "change_target_branch")] | length')
|
||||
case "$m" in ''|*[!0-9]*) return 0 ;; esac
|
||||
total=$(( total + m ))
|
||||
page=$(( page + 1 ))
|
||||
done
|
||||
return 0
|
||||
}
|
||||
|
||||
ex_repair=no
|
||||
count_retargets
|
||||
retargets_before=$rt_count
|
||||
retargets_before_ok=$rt_ok
|
||||
echo "Retarget fence: ${retargets_before} retarget event(s) observed before classifying (trusted=${retargets_before_ok})."
|
||||
|
||||
# --- Whose verdict is it? (ersatztv#698 route 3) -------------------------------------
|
||||
# This short-circuit used to exit on ANY existing `success`, which made an exemption this job
|
||||
# wrote indistinguishable from a verdict a human wrote. That is what let a forged exemption
|
||||
@@ -508,6 +602,13 @@ jobs:
|
||||
fi
|
||||
fi
|
||||
|
||||
if [ "$exempt" = yes ] && [ "$ex_repair" = yes ]; then
|
||||
# A previous run of this job already repaired a raced exemption on this sha, which means a
|
||||
# human verdict was written for it and then lost. Re-granting the exemption would bury that
|
||||
# rejection again, one event later. The PR needs a real verdict; only a human can clear this.
|
||||
exempt=no
|
||||
reason="a human verdict raced a previous exemption write on this head and was overwritten — this head needs a re-posted verdict, not another exemption"
|
||||
fi
|
||||
if [ "$exempt" = yes ]; then
|
||||
state=success
|
||||
desc="Exempt: $reason"
|
||||
@@ -517,24 +618,175 @@ jobs:
|
||||
fi
|
||||
echo "Decision: state=${state} — ${reason}"
|
||||
|
||||
# HIGH-WATER MARK for the post-write verification (ersatztv#706 race 2). Taken FIRST — before
|
||||
# the re-read below, before the fence, before the POST — and the ORDER IS THE POINT.
|
||||
#
|
||||
# An earlier version captured it just before the POST, "as late as possible". Cold review
|
||||
# caught that as a High: everything between the re-read and a late mark is a blind gap. A
|
||||
# human verdict landing there is invisible to the re-read (which already happened) AND
|
||||
# excluded from the post-write check (its id is BELOW a mark taken afterwards), so it is
|
||||
# silently overwritten with no repair. That gap spans the entire retarget re-count — up to 20
|
||||
# timeline round-trips — so it was far wider than the one-round-trip residual being claimed.
|
||||
#
|
||||
# Taking the mark first closes the read side completely: any row newer than the mark is caught
|
||||
# either by the re-read (abstain, post nothing) or by the post-write check (repair). There is
|
||||
# no false-fire cost to being early, because the test is `id > mark` — rows already present
|
||||
# when the mark is taken are below it and stay invisible either way.
|
||||
#
|
||||
# Presence alone would be the wrong test, and wrong in the direction that breaks the gate: the
|
||||
# short-circuit deliberately does NOT stop for a human verdict whose recorded base does not
|
||||
# match this PR's (`ex_human` is reset to `no` — see `read_existing_verdict`). Such a row stays
|
||||
# in the history forever, so a presence test would fire on EVERY later run of that PR,
|
||||
# downgrade every exemption to `pending`, and deadlock it permanently.
|
||||
#
|
||||
# `max` over an empty array is `null`, hence `// 0`. `.id? // 0` rather than `.id`: a bare
|
||||
# `.[].id` hard-errors under `set -e` if the array ever holds a non-object, which would kill
|
||||
# the job before it posts and strand an ordinary PR with no status at all.
|
||||
max_id_before=-1
|
||||
hist_before=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA?limit=100") || hist_before=""
|
||||
if [ -n "${hist_before//[[:space:]]/}" ] && printf '%s' "$hist_before" | jq -e 'type == "array"' >/dev/null 2>&1; then
|
||||
mark=$(printf '%s' "$hist_before" | jq -r '[.[] | .id? // 0] | max // 0' 2>/dev/null || true)
|
||||
case "$mark" in
|
||||
''|*[!0-9]*)
|
||||
# SKIP the check rather than treat everything as raced. A mark of 0 would make every
|
||||
# pre-existing human row look newer than the mark and repair every exemption away.
|
||||
echo "::warning::Status high-water mark for ${SHA:0:7} was not numeric ('${mark}'); the post-write race check will be skipped."
|
||||
max_id_before=-1 ;;
|
||||
*) max_id_before=$mark ;;
|
||||
esac
|
||||
else
|
||||
# Not fatal: the POST below is still correct, only the after-the-fact verification is
|
||||
# weakened. Recorded so a silent degradation is visible in the log.
|
||||
echo "::warning::Could not establish a status high-water mark for ${SHA:0:7}; the post-write race check will be skipped."
|
||||
max_id_before=-1
|
||||
fi
|
||||
|
||||
# LAST-MOMENT RE-READ (ersatztv#706). Classification takes several API round-trips, and a
|
||||
# reviewer can post a verdict during them — most dangerously a `failure`, which this job would
|
||||
# then overwrite with an exemption `success`, turning an explicit human rejection green. The
|
||||
# first read cannot see that; this one can. It NARROWS the window, it does not close it: there
|
||||
# is no compare-and-set on Gitea's status API, so a verdict landing between this read and the
|
||||
# POST below is still lost. Said plainly here rather than left as an implied guarantee.
|
||||
# POST below is still lost — which is what the post-write repair below is for.
|
||||
read_existing_verdict
|
||||
if [ "$ex_human" = yes ]; then
|
||||
echo "::notice::A human 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
|
||||
|
||||
# THE FENCE ITSELF (ersatztv#706 race 1). Re-count the retargets as late as possible and
|
||||
# refuse to write anything if the PR was retargeted since this run began. See the long note
|
||||
# at `count_retargets` for why this is a handoff rather than a stall, and why the count is
|
||||
# the only key that survives an ABA.
|
||||
#
|
||||
# The refusal covers `pending` as well as `success`, not just the dangerous write. A stale
|
||||
# `pending` over a fresh `success` is only a stall rather than a forged green, so gating it
|
||||
# is not strictly required — but the successor run is guaranteed either way, so there is
|
||||
# nothing to buy by writing a value this run already knows was computed against a base the
|
||||
# PR no longer targets. One rule, one direction, nothing to reason about per state.
|
||||
#
|
||||
# An UNTRUSTED count on either side (`rt_ok=no`: paging never reached a validated empty
|
||||
# page, a page was unreadable, the cap was hit) is NOT treated as "no retarget". It blocks
|
||||
# the exemption `success` only, and lets `pending` through: `pending` cannot turn a rejection
|
||||
# or an unreviewed head green, so withholding it would strand PRs for no safety gain, while
|
||||
# a `success` written on a count we could not verify is exactly the forged-green outcome
|
||||
# this fence exists to prevent.
|
||||
count_retargets
|
||||
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}."
|
||||
exit 0
|
||||
fi
|
||||
if { [ "$retargets_before_ok" != yes ] || [ "$rt_ok" != yes ]; } && [ "$state" = "success" ]; then
|
||||
echo "::error::Could not establish a trusted retarget 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. Posting nothing; ${CONTEXT} stays absent, which blocks the merge. 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
|
||||
fi
|
||||
|
||||
payload=$(jq -n --arg s "$state" --arg c "$CONTEXT" --arg d "$desc" --arg u "$PR_URL" \
|
||||
'{state:$s, context:$c, description:$d, target_url:$u}')
|
||||
gh -X POST -H 'Content-Type: application/json' -d "$payload" \
|
||||
"$BASE_URL/repos/$REPO/statuses/$SHA" >/dev/null
|
||||
echo "Posted ${CONTEXT}=${state} on ${SHA:0:7}."
|
||||
|
||||
# --- POST-POST VERIFICATION (ersatztv#706 race 2) ------------------------------------
|
||||
# The last-moment re-read above narrows the window between reading and writing; it cannot
|
||||
# close it, because Gitea's status API has no conditional write (no ETag, no If-Match, no
|
||||
# expected-previous-state), so there is no compare-and-set to make the read and the POST one
|
||||
# operation. A human `failure` landing in that remaining gap is overwritten by the POST above
|
||||
# — turning an explicit human REJECTION green, which is the worst outcome this gate can
|
||||
# produce and strictly worse than any stall.
|
||||
#
|
||||
# So verify AFTERWARDS and repair in the safe direction. This runs ONLY on the `success`
|
||||
# path, and that restriction is the point rather than an optimisation: `pending` cannot
|
||||
# turn a rejection green — it already blocks the merge — so the only write that can cause
|
||||
# the damage is the exemption `success`.
|
||||
#
|
||||
# WHY A DIFFERENT ENDPOINT. Everywhere else this job reads the COMBINED endpoint
|
||||
# (`/commits/{sha}/status`), which returns the LATEST status per context — and that is now
|
||||
# OUR success, with the human's row buried underneath it. The combined view is therefore
|
||||
# structurally incapable of showing the thing being looked for. `/statuses/{sha}` returns one
|
||||
# row per POST instead. Measured on this instance, the two really do differ in shape as well
|
||||
# as content: the combined endpoint returns an OBJECT with a `.statuses` array (12 rows on a
|
||||
# live head), `/statuses/{sha}` a BARE ARRAY (24 rows on the same head) — hence the different
|
||||
# `type == "array"` guard here.
|
||||
#
|
||||
# No claim is made about the order rows come back in, because the check does not depend on
|
||||
# it: it selects by id against the high-water mark rather than inspecting the top of the
|
||||
# list. A verdict older than the mark is invisible to it no matter where it sits.
|
||||
#
|
||||
# The repair is `pending`, NEVER a copy of the human's state. Re-posting their `failure`
|
||||
# would attribute a human verdict to this job — the exact provenance confusion the
|
||||
# `creator`-based short-circuit above exists to prevent, and it would be written with the
|
||||
# machine credential, so it would read as machine-derived to every later run. `pending`
|
||||
# asserts nothing about the review: it blocks the merge and asks for a real verdict, which
|
||||
# is true and safe regardless of which way the human ruled. The reviewer is told to re-post.
|
||||
#
|
||||
# A read failure here does NOT fail the job: the status is already posted, so `exit 1` would
|
||||
# change nothing about the gate's state while turning a routine API hiccup into a red run.
|
||||
# It is reported loudly and left alone — the residual is the read/POST gap either way.
|
||||
if [ "$state" = "success" ] && [ "$max_id_before" -ge 0 ]; then
|
||||
post_hist=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA?limit=100") || post_hist=""
|
||||
if [ -z "${post_hist//[[:space:]]/}" ] || ! printf '%s' "$post_hist" | jq -e 'type == "array"' >/dev/null 2>&1; then
|
||||
echo "::warning::Could not re-read the status history for ${SHA:0:7} after posting, so a human verdict landing during the write window would not be detected. The exemption ${CONTEXT}=success stands."
|
||||
else
|
||||
# `.id > $since` is what confines this to the write window. Our OWN row is excluded twice
|
||||
# over — it carries `creator: null` (an Actions-token POST, measured; see the provenance
|
||||
# note above) and its description is `Exempt: …`, not `Review-verdict:` — so the count is
|
||||
# of human verdicts that did not exist when the mark was taken.
|
||||
raced=$(printf '%s' "$post_hist" | jq -r --arg c "$CONTEXT" --argjson since "$max_id_before" \
|
||||
'[.[] | select(type == "object")
|
||||
| select(.context? == $c)
|
||||
| select((.id? // 0) > $since)
|
||||
| select(.creator != null and .creator.login != null and .creator.login != "")
|
||||
| select((.description // "") | startswith("Review-verdict:"))] | length')
|
||||
case "$raced" in
|
||||
''|*[!0-9]*)
|
||||
echo "::warning::Post-write verification for ${SHA:0:7} returned '${raced}' instead of a count; not acting on it."
|
||||
;;
|
||||
*)
|
||||
if [ "$raced" -gt 0 ]; then
|
||||
# The last-moment re-read found no human verdict, so any row present now was
|
||||
# written during the window and has just been masked by the exemption above.
|
||||
echo "::error::A human ${CONTEXT} verdict landed on ${SHA:0:7} while this job was writing its exemption, and was overwritten. Downgrading to 'pending' so an explicit human decision cannot be silently green. Re-post it with: scripts/post-review-verdict.sh ${PR} <VERDICT>"
|
||||
repair=$(jq -n --arg c "$CONTEXT" --arg u "$PR_URL" --arg d "$REPAIR_DESC" \
|
||||
'{state:"pending", context:$c, description:$d, target_url:$u}')
|
||||
# A failure HERE leaves the forged green standing, so it is retried once and then
|
||||
# screams. `set -e` would otherwise kill the job silently, after the success was
|
||||
# written and with nothing left to re-attempt.
|
||||
if ! gh -X POST -H 'Content-Type: application/json' -d "$repair" \
|
||||
"$BASE_URL/repos/$REPO/statuses/$SHA" >/dev/null 2>&1; then
|
||||
if ! gh -X POST -H 'Content-Type: application/json' -d "$repair" \
|
||||
"$BASE_URL/repos/$REPO/statuses/$SHA" >/dev/null 2>&1; then
|
||||
echo "::error::COULD NOT REPAIR ${CONTEXT} on ${SHA:0:7}. An exemption 'success' is standing on a head whose human verdict was overwritten. Post the verdict again immediately: scripts/post-review-verdict.sh ${PR} <VERDICT>"
|
||||
exit 1
|
||||
fi
|
||||
fi
|
||||
echo "Repaired ${CONTEXT} to pending on ${SHA:0:7}."
|
||||
state=pending
|
||||
fi
|
||||
;;
|
||||
esac
|
||||
fi
|
||||
fi
|
||||
|
||||
if [ "$state" = "pending" ]; then
|
||||
echo "::notice::This PR needs an H10 review verdict for head ${SHA:0:7} before it can merge. After reviewing, run: scripts/post-review-verdict.sh ${PR} MERGEABLE"
|
||||
fi
|
||||
|
||||
@@ -52,7 +52,7 @@ docker build -f docker/Dockerfile -t ersatztv:dev .
|
||||
- Test with **NUnit** + Shouldly + NSubstitute (the existing `*.Tests` projects); xUnit is **not** used here
|
||||
- **Dependencies use Central Package Management**: versions live in the repo-root `Directory.Packages.props`; csproj reference packages by name only. Add/upgrade by editing the central `<PackageVersion>` — never put `Version=` back on a `<PackageReference>` (trips `NU1008`). See `docs/ci-cd.md` → Dependency management.
|
||||
- **DB migrations target BOTH providers**: a `TvContext` model change needs a migration in `ErsatzTV.Infrastructure.Sqlite` **and** `ErsatzTV.Infrastructure.MySql` — run `scripts/add-migration.sh <Name>` (does both). CI's `migrations` job enforces model-drift + apply-to-fresh-DB per provider. See `docs/ci-cd.md` → Migration integrity.
|
||||
- **Renovate** is live (`.gitea/workflows/renovate.yml`, weekly + `workflow_dispatch`): opens dependency-update + OSV vuln-fix PRs and a Dependency Dashboard issue; patch bumps to test/dev-only packages auto-merge once `Build & test` passes, the rest are manual. Their `review-verdict/h10` required check is auto-passed **only when BOTH hold**: the PR touches none of `.claude/`/`.gitea/`/`.husky/`/`scripts/`/`docker/ci/`, **and** every changed path is a dependency manifest (`Directory.Packages.props`, `.config/dotnet-tools.json`) — ersatztv#698. A bot ACCOUNT does not attribute the CODE at a head, so identity alone is no longer sufficient; a Renovate PR touching a `.csproj` or a source file is not blocked, it just needs a real verdict. Cross-repo rollout: server-management#484. See `docs/ci-cd.md` → Dependency management.
|
||||
- **Renovate** is live (`.gitea/workflows/renovate.yml`, weekly + `workflow_dispatch`): opens dependency-update + OSV vuln-fix PRs and a Dependency Dashboard issue; patch bumps to test/dev-only packages auto-merge once `Build & test` passes, the rest are manual. Their `review-verdict/h10` required check is auto-passed **only when BOTH hold**: the PR touches none of `.claude/`/`.codex/`/`.gitea/`/`.husky/`/`scripts/`/`docker/ci/`, **and** every changed path is a dependency manifest (`Directory.Packages.props`, `.config/dotnet-tools.json`) — ersatztv#698. A bot ACCOUNT does not attribute the CODE at a head, so identity alone is no longer sufficient; a Renovate PR touching a `.csproj` or a source file is not blocked, it just needs a real verdict. Cross-repo rollout: server-management#484. See `docs/ci-cd.md` → Dependency management.
|
||||
- **Versioning**: release tags are `vYY.<release-seq>.<patch>` (year · sequential release-within-year · patch) — inherited from upstream, **not** year.month. `v26.3.1` = our infra rebuild of upstream 26.3.0 (no app changes); `v26.4.0` is reserved for the first release with app changes. Never `[skip ci]` a commit you'll tag (it suppresses the release build). Full policy: `docs/ci-cd.md` → Versioning & releases.
|
||||
- Backlog tracked via [Gitea Issues](http://192.168.1.95:3000/timothy/ersatztv/issues)
|
||||
|
||||
@@ -84,7 +84,7 @@ Every task that closes a Gitea issue MUST complete ALL of these before it is con
|
||||
|
||||
**Merge-consent is derived from state, not asserted (`## Done-when` convention — ersatztv#303 H6 + H10).** Any issue whose PR will merge to `main` should carry a `## Done-when` section in its **issue body** — a checklist of completion criteria (always include an "adversarial review passed" box; add per-issue criteria like tests-green, docs-updated, live-E2E). Two hooks derive merge-consent from it so a premature merge is blocked *by construction*, not by memory:
|
||||
- `pretooluse-merge-consent.sh` (Claude PreToolUse on the Gitea merge tool) — **auto-grants** a merge (emits `permissionDecision: allow`, so **no** redundant mechanical prompt fires) only when the PR's CI is green **and** every `## Done-when` box on the linked issue (`fixes #N`) is ticked **and** a `Review-verdict:` comment references the PR's *current head sha* (**H10**); **denies** on an unticked box, red CI, or a stale/negative review verdict; **asks** (falls back to a human prompt) when it can't derive state (no linked issue, no `## Done-when` section, no `Review-verdict:` comment yet, no creds, Gitea down). On the auto-grant (satisfied) path the derived state **is** the consent — do not also ask conversationally to merge; a separate human confirmation is warranted only when the gate **asks** (ersatztv#314). **The H10 review-verdict convention**: after an adversarial/Codex review of a PR (or its latest fix commit), run **`scripts/post-review-verdict.sh <pr> <MERGEABLE|APPROVED|BLOCKED|NOT-MERGEABLE> [note]`** — it posts both the `Review-verdict: … @ <head-sha>` comment and the sha-bound `review-verdict/h10` commit status, proving the *latest* commit was reviewed rather than a stale earlier diff (ersatztv#242). Do not hand-write the comment: the **status** is the required check branch protection enforces, and a comment alone leaves it absent.
|
||||
- **The gate is enforced server-side, per sha (ersatztv#622).** `review-verdict/h10` is a required status check on `main`. Because a commit status belongs to one sha, a commit pushed *after* an auto-merge is scheduled clears it and blocks the merge — closing the hole where `merge_when_checks_succeed` froze consent at scheduling time and Gitea later merged an unreviewed head. Renovate-authored and docs-only PRs are auto-passed by `.gitea/workflows/review-verdict.yml`, **except** when they touch `.claude/`, `.gitea/`, `.husky/`, `scripts/` or `docker/ci/`. See `docs/ci-cd.md` → Review-verdict gate.
|
||||
- **The gate is enforced server-side, per sha (ersatztv#622).** `review-verdict/h10` is a required status check on `main`. Because a commit status belongs to one sha, a commit pushed *after* an auto-merge is scheduled clears it and blocks the merge — closing the hole where `merge_when_checks_succeed` froze consent at scheduling time and Gitea later merged an unreviewed head. Renovate-authored and docs-only PRs are auto-passed by `.gitea/workflows/review-verdict.yml`, **except** when they touch `.claude/`, `.codex/`, `.gitea/`, `.husky/`, `scripts/` or `docker/ci/`. See `docs/ci-cd.md` → Review-verdict gate.
|
||||
- `.husky/pre-push` → `prepush-donewhen.sh` — a fail-open backstop that blocks a direct `git push origin main` whose commits `fix #N` an issue with unticked boxes.
|
||||
|
||||
Both need Gitea read creds in the env to enforce (**`ETV_GITEA_BASICAUTH=user:pass`** or `ETV_GITEA_TOKEN`; `ETV_GITEA_URL` overrides the base). Without them the merge hook asks and the push backstop is a no-op — the gate degrades to today's manual confirmation, never a silent pass. Docs-only PRs/pushes are exempt.
|
||||
|
||||
+29
-4
@@ -791,7 +791,7 @@ and comparing tips would invalidate every open verdict on every unrelated merge
|
||||
status as `success` for **Renovate-authored** PRs (it uses `platformAutomerge: true`, so a required
|
||||
verdict with no exemption would stall every dependency bump) and for **docs-only** PRs, and as
|
||||
`pending` for everything else so the block has a visible reason. Both exemptions are **void when the
|
||||
PR touches `.claude/`, `.gitea/`, `.husky/`, `scripts/` or `docker/ci/`** — a PR that can weaken the
|
||||
PR touches `.claude/`, `.codex/`, `.gitea/`, `.husky/`, `scripts/` or `docker/ci/`** — a PR that can weaken the
|
||||
gate must not be able to exempt itself from the gate. That includes Renovate's `docker/ci` base
|
||||
bumps, which already need the manual publish-then-pin two-step anyway.
|
||||
|
||||
@@ -837,9 +837,34 @@ the PR's live base does not match it, checked both before and after paging (ersa
|
||||
the head sha — a PR opened into `main` and retargeted mid-run was granted a docs-only exemption while
|
||||
its diff against `main` carried a C# file. The workflow passes the base from the `pull_request_target`
|
||||
payload, which a retarget cannot rewrite, and `edited` is in `types:` so a retarget reclassifies.
|
||||
`edited` gives **detection, not atomicity**: runs are not serialized, so a stale run can still post
|
||||
`success` after the reclassifying run posts `pending`. The `main → scratch → main` ABA transition is
|
||||
narrowed and observable, not closed — see the residual in `ci.exemption-provenance`.
|
||||
`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 (ersatztv#706).** 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
|
||||
both ends, which is how the forged exemption was obtained in the first place. Abstaining never strands
|
||||
a PR, because every retarget fires `edited` — the event that makes one run abstain has already queued
|
||||
its successor.
|
||||
|
||||
If the count can't be established (unreadable timeline, paging that never reached a validated empty
|
||||
page), only the exemption `success` is withheld; `pending` still posts, since `pending` cannot turn an
|
||||
unreviewed head green and withholding it would strand ordinary PRs for nothing. **If an exempt PR is
|
||||
unexpectedly missing its status after a retarget, this is why** — the job log names the counts.
|
||||
|
||||
Worth knowing before reaching for the obvious alternative: **a concurrency group does not work here**,
|
||||
measured rather than assumed. Gitea 1.25.4 auto-cancels superseded `push` runs on a branch, but *not*
|
||||
`pull_request_target` runs — two runs for one PR genuinely overlap, and adding
|
||||
`concurrency: {…, cancel-in-progress: false}` changed nothing (probe runs still overlapped by 36s).
|
||||
`cancel-in-progress: true` is deliberately untried, because a cancelled run leaves an exempt PR
|
||||
statusless with nothing left to re-trigger it. Full measurements and the two surviving residuals:
|
||||
`ci.verdict-write-retarget-fence`.
|
||||
|
||||
Separately, after posting an exemption `success` the job re-reads the per-POST status history and, if
|
||||
a human `Review-verdict:` row appeared during the write window, overwrites its own status with
|
||||
`pending` and logs an error — so a human `BLOCKED` can never be silently turned green. The repair is
|
||||
`pending`, never a copy of the human's verdict, which would attribute a human decision to the job.
|
||||
|
||||
Three properties of this workflow are security-relevant and are **structurally** asserted by tests in
|
||||
`scripts/tests/test_pr_changed_files.py` — those tests pin the workflow's shape, which is not the same
|
||||
|
||||
@@ -43,7 +43,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `ci.decisions-lifecycle-flake` | When `decisions lifecycle` is the **only** red job, do not investigate and do not create a new run to clear it — no rebase, no `--amend`, no no-op push; the operator reruns that single job from the Gitea UI. | 2026-07-21 | [link](records/ci/decisions-lifecycle-flake.md) |
|
||||
| `ci.docs-only-detect-shallow-safe` | The docs-only detect script must diff against `FETCH_HEAD` (always resolves after `git fetch`, even shallow) using a two-dot tree diff — not `origin/<base>` with three-dot — because a `fetch-depth: 1` shallow clone has no remote-tracking ref and no merge-base, which silently fails the original detect into `docs_only=false` (full matrix, no functional error). A CI-behavior change must be verified by measuring the effect (job durations), not just a green check. | 2026-07-17 | [link](records/ci/docs-only-detect-shallow-safe.md) |
|
||||
| `ci.docs-only-skip-steps` | A docs-only change must still run every required job (`test`, `migrations`) so their commit-status contexts always report; each heavy job runs `scripts/ci-detect-docs-only.sh` first and gates its real STEPS on `if: steps.detect.outputs.docs_only != 'true'`, never `if:`-skips the whole job (an `if:`-skipped job reports `skipped`, not `success`, which branch protection may never unblock on). Detection biases toward running more on any doubt. | 2026-07-17 | [link](records/ci/docs-only-skip-steps.md) |
|
||||
| `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict for THIS base, meaning a non-null `.creator.login` AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR's — tested by requiring the description to END with the exact literal `(base: <base>)` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow's `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one (residual, #706). Path predicates are evaluated by COUNTING with `grep -c`, never `\| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`. | 2026-07-29 | [link](records/ci/exemption-provenance.md) |
|
||||
| `ci.exemption-provenance` | The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR's live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict for THIS base, meaning a non-null `.creator.login` AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR's — tested by requiring the description to END with the exact literal `(base: <base>)` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow's `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one. That residual is now FENCED rather than merely tracked — the job refuses to write at all if the PR's timeline retarget COUNT moved while it was classifying (`ci.verdict-write-retarget-fence`, #706) — leaving only the sub-round-trip window that no API without compare-and-set can close. The PROTECTED path list additionally covers `.codex/` (#711), which mirrors `.claude/hooks/` byte for byte including the merge-consent hook — latent while that directory is untracked, live the moment it is tracked; the list stays ENUMERATIVE rather than derived, because a derived rule would have to be evaluated against the very file list being classified. Path predicates are evaluated by COUNTING with `grep -c`, never `\| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`. | 2026-07-29 | [link](records/ci/exemption-provenance.md) |
|
||||
| `ci.format-gate-folder-mode` | The blocking `format` CI job (and matching pre-commit hook) runs `dotnet format whitespace . --folder --include <files>` instead of loading the full MSBuild/Roslyn solution, cutting the gate from ~480s to ~0.5s with unchanged whitespace/charset coverage. | 2026-07-19 | [link](records/ci/format-gate-folder-mode.md) |
|
||||
| `ci.functional-e2e-harness` | The `functional-e2e` CI job boots the PR's own code from source via `dotnet run` (`scripts/e2e-local.sh`) and runs deterministic assertions (`scripts/e2e-functional.sh`) as an advisory (non-blocking) job, not a `build` dependency or required check. Originally curl-only; since #445 the same job carries a second, headless-browser step for the contracts curl cannot express — see `ci.ui-e2e-harness`. | 2026-07-16 | [link](records/ci/functional-e2e-harness.md) |
|
||||
| `ci.gate-trigger-base-resolved` | The workflow that writes the branch-protection-required `review-verdict/h10` status triggers on `pull_request_target` with `branches: [main]`, never on plain `pull_request`. Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit, so under that trigger a PR editing `.gitea/workflows/review-verdict.yml` ran its own rewritten copy and could post `h10=success` for itself; `pull_request_target` resolves the definition from the base instead. The `branches: [main]` filter is part of the rule, not a refinement of it: base resolution only relocates the rewrite from the head to the base, so without the filter a PR opened into an attacker-pushed base branch runs that branch's gate. `pull_request_target` is safe HERE only because this job never checks out or executes head-supplied code — it checks out `base.sha` and runs only that tree's scripts (`ci.shared-pr-file-enumeration`); reintroducing a head checkout under this trigger would be worse than the bug it fixed. This closes the rewrite route through THIS workflow and does NOT close the class: Gitea injects a write-capable `GITEA_TOKEN` into EVERY job, so any ref-resolved workflow — and a collaborator's own API token, since branch protection binds the context and not its issuer — can still forge `review-verdict/h10`. Tracked in #697; the exemption path has its own separate defects in #698. | 2026-07-28 | [link](records/ci/gate-trigger-base-resolved.md) |
|
||||
@@ -58,9 +58,10 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `ci.root-screenshot-guard` | The Husky `pre-commit` hook refuses a staged root-level `*.png` (belt-and-suspenders with the `.gitignore` rule); nested `*.png` real assets are unaffected. | 2026-07-12 | [link](records/ci/root-screenshot-guard.md) |
|
||||
| `ci.runner-placement` | No persistent Roslyn compiler server survives a CI build (`UseSharedCompilation=false` etc., runner env + Dockerfile `ENV`); every `services:` container gets its own explicit `--memory`/`--memory-swap`/`--cpus` cap (it does not inherit the job container's). | 2026-07-17 | [link](records/ci/runner-placement.md) |
|
||||
| `ci.script-tests-job` | The `scripts/tests/` pytest suite runs on every PR as a dedicated `script-tests` job in `pr-checks.yml` (`runs-on: small`, `setup-python` + `pip install pytest`, `PYTHONPATH=. python3 -m pytest scripts/tests -q`), unconditionally rather than behind a `scripts/**` path filter, and **never as a step inside `decisions-guard`** — a job whose reds a standing rule instructs sessions to ignore must never host a gate whose reds are real. Any new CI gate must be reachable by a failure that is unambiguously attributable to it. | 2026-07-26 | [link](records/ci/script-tests-job.md) |
|
||||
| `ci.shared-pr-file-enumeration` | A PR's complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding, and base-ref binding — see `ci.exemption-provenance`) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate. | 2026-07-26 | [link](records/ci/shared-pr-file-enumeration.md) |
|
||||
| `ci.shared-pr-file-enumeration` | A PR's complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding, base-ref binding — see `ci.exemption-provenance` — and base-TIP binding, #707: the ref answers "did this PR RETARGET", the tip answers "did the base ADVANCE mid-enumeration", and only the second can see `/pulls/{n}/files` recomputing each offset-paged page against a moved base and dropping a path out of an already-consumed range; both ends of the window are bound, and an advance BEFORE the window is deliberately not an error, or ordinary churn on `main` would fail every open PR) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate. | 2026-07-26 | [link](records/ci/shared-pr-file-enumeration.md) |
|
||||
| `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.ui-e2e-harness` | The UI-interactive E2E flows run as headless Playwright specs (`web/e2e/*.spec.ts`, driven by `scripts/e2e-ui.sh`) in a **second step of the existing advisory `functional-e2e` job**, never their own job; the browser is `chromium-headless-shell` **baked into the CI toolchain image** (`docker/ci/Dockerfile`, `PLAYWRIGHT_VERSION` kept equal to `web/package.json`'s EXACT `@playwright/test` pin), never installed per run; specs are `serial` with `retries: 0` and assert only contracts the curl harness structurally cannot reach. | 2026-07-25 | [link](records/ci/ui-e2e-harness.md) |
|
||||
| `ci.verdict-write-retarget-fence` | The `review-verdict/h10` job counts `change_target_branch` events on the PR's issue timeline at run start and again immediately before its POST, and writes NOTHING if the count moved. The COUNT is the key because the branch NAME is ABA-vulnerable — `main -> S -> main` reads `main` at both ends, which is how #698 route 1 obtained a forged exemption — while the event count is monotonic and cannot alias. Abstaining is a handoff, not a stall, and that is the property the design rests on: 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 and the last run writes the final answer. `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` cannot turn an unreviewed head green while withholding it would strand ordinary PRs for no safety gain. SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` and, if a human `Review-verdict:` row appeared with an id ABOVE a high-water mark taken just before the POST, overwrites its own status with `pending` and logs an error. The repair is `pending`, NEVER a copy of the human's state, since re-posting their `failure` under the machine credential would attribute a human verdict to the job; its description is a SENTINEL that the classification refuses to grant an exemption over, so the block survives later events instead of being re-derived away 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. | 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) |
|
||||
| `concurrency.diff-scalar-fanout` | The frozen Block optimistic-concurrency recipe (api-conventions §7a) fans out to Collection/Playout×2/MultiCollection/RerunCollection, keeping a guard-returned `PreconditionFailedError` out of any handler's generic `catch(Exception)`→422 mapping, and preserving each aggregate's existing `SaveChangesAsync() > 0` gate semantics under the new unconditional `Version++`. | 2026-07-11 | [link](records/concurrency/diff-scalar-fanout.md) |
|
||||
@@ -124,7 +125,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `release.prepush-clean-worktree-guard` | A fail-open pre-push hook blocks a push when any file in the branch's diff vs `origin/main` also has uncommitted working-tree or index changes, since a stale-index commit (e.g. `git reset --soft` + `git add` over an edited-but-unstaged fix) can silently push, CI-test, and get reviewed a different tree than the one on disk. Scope is precise to pushed-diff files; escape hatch `ETV_ALLOW_DIRTY_PUSH=1`. | 2026-07-17 | [link](records/release/prepush-clean-worktree-guard.md) |
|
||||
| `release.promotion-floating-prod` | Prod tracks the floating `:prod` image reference; a tag build's immutable `:<version>` image is scanned first, then promotion happens via a separate manual `DeployStack`, with daily auto-update only as a fallback — tag with enough runway before 03:00 to avoid an unscanned promotion. | 2026-07-13 | [link](records/release/promotion-floating-prod.md) |
|
||||
| `release.review-verdict-gate` | A PR may not merge until a `Review-verdict: <MERGEABLE\|APPROVED\|BLOCKED\|NOT-MERGEABLE> @ <head-sha>` comment references the PR's current head sha (short-sha prefix match against the verdict's OWN `@ <sha>` field, marker at COLUMN 0 (no indent, so indented code blocks cannot self-approve), whole-word verdict token, fenced code blocks stripped with markdown fence-length semantics, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c). The grammar lives in ONE tested place, `scripts/check-review-verdict.sh` — #629 found three false-opens that survived because it was implemented inline and untested while this record described stricter behaviour than the code had. | 2026-07-12 | [link](records/release/review-verdict-gate.md) |
|
||||
| `release.verdict-status-check` | The H10 review verdict is written as a `review-verdict/h10` Gitea **commit status** on the exact reviewed sha by `scripts/post-review-verdict.sh`, and that context is a REQUIRED status check on `main`. Because a status belongs to one sha, a later commit cannot inherit it, so Gitea's own `merge_when_checks_succeed` refuses to merge a head no one reviewed. The PreToolUse hook additionally refuses to SCHEDULE an auto-merge unless that status is already green on head. A `pull_request_target` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.gitea/`, `.husky/`, `scripts/`, `docker/ci/`). This extends — does not supersede — `release.review-verdict-gate` (#303 H10), whose comment convention remains the human-readable artifact and the hook's condition (c). | 2026-07-25 | [link](records/release/verdict-status-check.md) |
|
||||
| `release.verdict-status-check` | The H10 review verdict is written as a `review-verdict/h10` Gitea **commit status** on the exact reviewed sha by `scripts/post-review-verdict.sh`, and that context is a REQUIRED status check on `main`. Because a status belongs to one sha, a later commit cannot inherit it, so Gitea's own `merge_when_checks_succeed` refuses to merge a head no one reviewed. The PreToolUse hook additionally refuses to SCHEDULE an auto-merge unless that status is already green on head. A `pull_request_target` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.codex/`, `.gitea/`, `.husky/`, `scripts/`, `docker/ci/`). This extends — does not supersede — `release.review-verdict-gate` (#303 H10), whose comment convention remains the human-readable artifact and the hook's condition (c). | 2026-07-25 | [link](records/release/verdict-status-check.md) |
|
||||
| `rulebuilder.relative-date-macros` | The visual rule builder's `inLast`/`notInLast` date operators compile to/parse from the pre-existing `CustomMultiFieldQueryParser` macros `released_inthelast`/`released_notinthelast` and `added_inthelast`/`added_notinthelast`, value form `"<n> day\|week\|month\|year"`; there is no backend change. | 2026-07-23 | [link](records/rulebuilder/relative-date-macros.md) |
|
||||
| `scan.collections-scan-status` | `GET /api/v1/media-sources/collections-scan-status` reports a family-global (not per-source), boolean-only active-scan set read from `IEntityLocker`; the SPA reconciles authoritatively against it (with a grace-tick helper) instead of a fixed client-side timeout. | 2026-07-12 | [link](records/scan/collections-scan-status.md) |
|
||||
| `scan.getoraddfolder-db-lookup` | `ILibraryRepository.GetOrAddFolder` resolves the existing folder via a DB query on `(LibraryPathId, Path)`, not the caller's `LibraryPath.LibraryFolders` in-memory navigation, since that navigation is only eager-loaded on the local scan path and is null on remote (Jellyfin) callers. | 2026-07-20 | [link](records/scan/getoraddfolder-db-lookup.md) |
|
||||
|
||||
@@ -5,7 +5,7 @@ status: active
|
||||
since: '2026-07-29'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR''s live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR''s immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict for THIS base, meaning a non-null `.creator.login` AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR''s — tested by requiring the description to END with the exact literal `(base: <base>)` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow''s `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one (residual, #706). Path predicates are evaluated by COUNTING with `grep -c`, never `| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`.'
|
||||
rule: 'The three inputs the exemption decision rests on must each be bound to something the judged PR cannot mutate. (1) BASE — `scripts/pr-changed-files.sh` takes the expected base BRANCH as a REQUIRED 5th argument and re-reads it before and after paging, because `/pulls/{n}/files` diffs against the PR''s live base and retargeting moves the answer without moving the head sha; the workflow passes `github.event.pull_request.base.ref` from the `pull_request_target` payload, which a retarget cannot rewrite. (2) BOT EXEMPTION — an author match is necessary but never sufficient: `pull_request.user.login` is the PR''s immutable CREATOR while its head is not, so the exemption additionally requires EVERY changed path to be a dependency manifest (`Directory.Packages.props` or `.config/dotnet-tools.json`, and ONLY those — the npm manifests are excluded because `package.json` `scripts` are executed by CI). (3) INHERITED SUCCESS — the never-overwrite short-circuit fires only for a status POSITIVELY identified as a human verdict for THIS base, meaning a non-null `.creator.login` AND a `Review-verdict:` description AND, when that description records a base (`(base: …)`, `release.verdict-status-check`), a base matching the PR''s — tested by requiring the description to END with the exact literal `(base: <base>)` and to contain exactly ONE such marker, never by extracting a value (see below); a present-but-different base is rejected, an absent one is not, since verdicts predating that convention carry none; every other shape, including any unrecognised one, is re-derived rather than trusted. The bot and docs-only exemptions are evaluated as INDEPENDENT predicates and the decision made afterwards, never as an `elif` chain. `edited` is in the workflow''s `types:` so a retarget reclassifies — which gives DETECTION, not atomicity: status writes are not serialized, so a stale run can still post over a fresher one. That residual is now FENCED rather than merely tracked — the job refuses to write at all if the PR''s timeline retarget COUNT moved while it was classifying (`ci.verdict-write-retarget-fence`, #706) — leaving only the sub-round-trip window that no API without compare-and-set can close. The PROTECTED path list additionally covers `.codex/` (#711), which mirrors `.claude/hooks/` byte for byte including the merge-consent hook — latent while that directory is untracked, live the moment it is tracked; the list stays ENUMERATIVE rather than derived, because a derived rule would have to be evaluated against the very file list being classified. Path predicates are evaluated by COUNTING with `grep -c`, never `| grep -q` (SIGPIPE inversion) and never a here-string (temp-space failure) — see `ci.grep-q-pipefail-inversion`.'
|
||||
signals: 'forged review-verdict exemption, retarget race against the docs-only classifier, PR base changed mid-run, hijacked Renovate branch, bot exemption on a code change, machine-written success inherited as a verdict, status creator null vs user, never overwrite a human verdict, exemption chain skips docs-only for bots, why is my Renovate PR asking for a verdict, base ref binding on pr-changed-files.sh · paths: `.gitea/workflows/review-verdict.yml`, `scripts/pr-changed-files.sh`, `.claude/hooks/pretooluse-merge-consent.sh`, `scripts/tests/test_pr_changed_files.py` · issues: #698, #697, #672, #663, #649, #632'
|
||||
mechanics: '`scripts/pr-changed-files.sh <owner> <repo> <pr> <expected-head-sha> <expected-base-ref>` (5 args; a 4-arg call exits 2); workflow env `BASE_REF: ${{ github.event.pull_request.base.ref }}`; `BOT_MANIFESTS` anchored allow-list; short-circuit requires `.creator.login` non-null AND description matching `^Review-verdict:`; `types: [opened, reopened, synchronize, ready_for_review, edited]`'
|
||||
---
|
||||
@@ -36,6 +36,17 @@ 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.
|
||||
|
||||
**Resolved 2026-08-03 (#706), and worth recording that the guarantee 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
|
||||
look unchanged. Measurement is what redirected it: `pull_request_target` runs were confirmed to
|
||||
overlap live (probe PR #722, the older run finishing 20s after the newer one started), and a
|
||||
non-cancelling concurrency group — the fix this record's residual implied and #706 proposed — was
|
||||
measured doing nothing at all. The paragraph above stands as written; only its last sentence is
|
||||
overtaken, and the sub-round-trip window it describes survives, because Gitea's status API has no
|
||||
compare-and-set.
|
||||
|
||||
**Route 2 — a bot ACCOUNT does not attribute the CODE.** `pull_request.user.login` is the PR's
|
||||
immutable *creator*; its head is not. Push application code onto an open Renovate branch and the PR is
|
||||
still "authored by renovate", touches no protected path, and was exempted. Checking the *pusher* fixes
|
||||
|
||||
@@ -5,7 +5,7 @@ status: active
|
||||
since: '2026-07-26'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'A PR''s complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding, and base-ref binding — see `ci.exemption-provenance`) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate.'
|
||||
rule: 'A PR''s complete set of changed file paths is computed by exactly one implementation, `scripts/pr-changed-files.sh`, called by both `.claude/hooks/pretooluse-merge-consent.sh` (advisory — a failure falls through to a human prompt) and `.gitea/workflows/review-verdict.yml` (enforced — a failure must fail closed, because a match here posts the branch-protection-required `review-verdict/h10` status with nobody in the loop). The script owns exhaustiveness (pagination, rename/path validation, head-sha binding, base-ref binding — see `ci.exemption-provenance` — and base-TIP binding, #707: the ref answers "did this PR RETARGET", the tip answers "did the base ADVANCE mid-enumeration", and only the second can see `/pulls/{n}/files` recomputing each offset-paged page against a moved base and dropping a path out of an already-consumed range; both ends of the window are bound, and an advance BEFORE the window is deliberately not an error, or ordinary churn on `main` would fail every open PR) and returns exit 0 only for a verified-complete list; it does NOT classify paths — each caller keeps its own docs-only allow-list, and the two allow-lists differ on purpose and stay separate.'
|
||||
signals: 'duplicated PR file enumeration, enforced gate weaker than advisory hook, docs-only allow-list drift, shared mechanism not shared policy, pr-changed-files.sh, checkout base ref not PR head, gate judging its own PR, exhaustiveness bug in a security predicate · paths: `scripts/pr-changed-files.sh`, `.claude/hooks/pretooluse-merge-consent.sh`, `.gitea/workflows/review-verdict.yml` · issues: #643, #648, #649'
|
||||
mechanics: '`scripts/pr-changed-files.sh <owner> <repo> <pr> <expected-head-sha> <expected-base-ref>` -> stdout newline-delimited paths, exit 0 only if complete and bound to BOTH the given sha and the given base branch; the 5th argument is REQUIRED and a 4-arg call exits 2 (`ci.exemption-provenance`); callers: `.claude/hooks/pretooluse-merge-consent.sh`, `.gitea/workflows/review-verdict.yml`'
|
||||
---
|
||||
|
||||
@@ -0,0 +1,72 @@
|
||||
---
|
||||
key: ci.verdict-write-retarget-fence
|
||||
title: '2026-08-03 — the review-verdict job fences its write on the PR timeline''s retarget COUNT, and verifies the exemption write afterwards (#706)'
|
||||
status: active
|
||||
since: '2026-08-03'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'The `review-verdict/h10` job counts `change_target_branch` events on the PR''s issue timeline at run start and again immediately before its POST, and writes NOTHING if the count moved. The COUNT is the key because the branch NAME is ABA-vulnerable — `main -> S -> main` reads `main` at both ends, which is how #698 route 1 obtained a forged exemption — while the event count is monotonic and cannot alias. Abstaining is a handoff, not a stall, and that is the property the design rests on: 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 and the last run writes the final answer. `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` cannot turn an unreviewed head green while withholding it would strand ordinary PRs for no safety gain. SEPARATELY, and for the human-verdict race the fence does nothing about: after posting an exemption `success` the job re-reads `/statuses/{sha}` and, if a human `Review-verdict:` row appeared with an id ABOVE a high-water mark taken just before the POST, overwrites its own status with `pending` and logs an error. The repair is `pending`, NEVER a copy of the human''s state, since re-posting their `failure` under the machine credential would attribute a human verdict to the job; its description is a SENTINEL that the classification refuses to grant an exemption over, so the block survives later events instead of being re-derived away 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.'
|
||||
signals: 'stale review-verdict run overwrites a fresher one, retarget ABA against the docs-only classifier, concurrency group does not serialize pull_request_target, gitea auto-cancel push vs pull_request_target, forged exemption restored after reclassification, human BLOCKED silently turned green, post-write status verification, change_target_branch timeline count, why does my PR post no verdict status after a retarget · paths: `.gitea/workflows/review-verdict.yml`, `scripts/tests/test_pr_changed_files.py` · issues: #706, #698, #672, #663, #622'
|
||||
mechanics: '`count_retargets()` pages `GET /repos/{repo}/issues/{pr}/timeline?limit=50&page=N` (cap 20) setting `rt_count`/`rt_ok`, trusted only on a validated empty page; `retargets_before`/`retargets_before_ok` captured before enumeration, re-counted immediately before the POST; `max_id_before` from `GET /repos/{repo}/statuses/{sha}` (a BARE ARRAY, unlike the combined `/commits/{sha}/status` object); repair POST is `pending`; tests `test_a_RETARGET_DURING_the_run_posts_NOTHING`, `test_a_PR_retargeted_BEFORE_the_run_but_QUIET_during_it_is_STILL_exempt`, `test_an_UNTRUSTED_retarget_count_withholds_the_EXEMPTION`, `test_an_UNTRUSTED_retarget_count_STILL_LETS_PENDING_THROUGH`, `test_a_human_verdict_landing_AFTER_the_POST_is_repaired_to_pending`, `test_a_PRE_EXISTING_human_row_does_NOT_trigger_a_repair`'
|
||||
---
|
||||
|
||||
`ci.exemption-provenance` closed three routes into the exemption path and left one residual it named:
|
||||
status writes are not serialized, so a stale run can post over a fresher one. This record resolves it,
|
||||
**narrowing** that record rather than superseding it.
|
||||
|
||||
## Measured, not reasoned (Gitea 1.25.4, 2026-08-03)
|
||||
|
||||
- **`pull_request_target` runs for one PR overlap, older finishing last.** Probe PR #722: run 7520
|
||||
(`opened`) completed at 18:30:42, twenty seconds *after* run 7521 (`synchronize`) began. Race 1's
|
||||
mechanism, observed rather than argued.
|
||||
- **A non-cancelling concurrency group — #706's own proposal — does nothing.** With it active, runs
|
||||
7528/7529 still overlapped; 7528 ended 36s after 7529 started. Refuted, not declined.
|
||||
- **The control that saved it.** A first probe *with* a group showed cancellations, which looked like
|
||||
confirmation. The identical workflow with **no `concurrency:` key at all** cancelled the same way:
|
||||
Gitea auto-cancels superseded **`push`** runs by itself, and that does not extend to
|
||||
`pull_request_target`. Without the control, a no-op would have shipped as a solution.
|
||||
- **`cancel-in-progress: true` is deliberately untried** — cancellation is precisely what this
|
||||
workflow's header refuses, since a cancelled run leaves an exempt PR statusless with nothing to
|
||||
re-trigger it.
|
||||
|
||||
## Why the count, and why abstaining is safe
|
||||
|
||||
The timeline records each retarget as a `change_target_branch` event. Verified on the route-1
|
||||
reproduction PR #703 (exactly two: `main → probe698/base-S` and back) against PR #717 as a
|
||||
zero-control. The branch *name* aliases under `main → S → main`; the count cannot.
|
||||
|
||||
The standing objection to refuse-on-motion is that it strands the PR — fatal for `updated_at`,
|
||||
harmless here, and not by degree: a retarget **always** fires `edited`, so the abstaining run is
|
||||
guaranteed a successor. It defers rather than declines.
|
||||
|
||||
## What cold review caught (both easy to reintroduce)
|
||||
|
||||
**The mark must be taken BEFORE the last-moment re-read, not merely before the POST.** "As late as
|
||||
possible" is the safer-sounding instinct and is the opposite: a verdict landing between the re-read
|
||||
and a late mark is invisible to the re-read (already done) *and* excluded from the post-write check
|
||||
(id below a mark taken afterwards) — a gap spanning the whole retarget re-count, while the change
|
||||
claimed one round-trip. Early costs nothing, since `id > mark` hides pre-existing rows either way.
|
||||
Pinned structurally, as an order not an output: with the mark late the job still posts and still
|
||||
repairs in every scenario a stub can pose, and only the arithmetic silently changes.
|
||||
|
||||
**The repair must be durable or it lasts one event.** A repaired status is a machine `pending`,
|
||||
indistinguishable to the next run — which re-derived it, posted `success`, and took a fresh mark
|
||||
*above* the human row, leaving the check silent and the rejection green again. Its description is now
|
||||
a sentinel no exemption is granted over; refusing can only withhold one, and only a re-posted verdict
|
||||
clears it.
|
||||
|
||||
## What is NOT closed
|
||||
|
||||
1. A retarget between the final timeline read and the POST. Gitea's status API has no conditional
|
||||
write, so without compare-and-set this cannot reach zero. The magnitude changed: a *permanent*
|
||||
forged green became a *transient* one of about one round-trip, and that retarget still fires
|
||||
`edited`, so a later run re-derives it.
|
||||
2. The repair is itself a read-then-write and can be raced; it fails toward `pending`. A transport
|
||||
failure on its POST is retried once then fails the job loudly — the one path where giving up
|
||||
quietly would leave a forged green standing.
|
||||
3. **No vocabulary tripwire.** If an upgrade renames `change_target_branch` or drops it, both counts
|
||||
read `0`, compare equal, are "trusted", and the protection evaporates silently. Accepted (an
|
||||
analogue of the jq `--expect` pin needs a live fixture PR), recorded so the silence is chosen.
|
||||
4. **A timeline over the 20-page cap can never be exempted** — `rt_ok` stays `no` on every run, so only
|
||||
a human verdict clears it and comment-flooding becomes a fail-closed denial of exemption.
|
||||
Negligible at 1000 events; the log says so rather than promising a later run will fix it.
|
||||
@@ -5,7 +5,7 @@ status: active
|
||||
since: '2026-07-25'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'The H10 review verdict is written as a `review-verdict/h10` Gitea **commit status** on the exact reviewed sha by `scripts/post-review-verdict.sh`, and that context is a REQUIRED status check on `main`. Because a status belongs to one sha, a later commit cannot inherit it, so Gitea''s own `merge_when_checks_succeed` refuses to merge a head no one reviewed. The PreToolUse hook additionally refuses to SCHEDULE an auto-merge unless that status is already green on head. A `pull_request_target` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.gitea/`, `.husky/`, `scripts/`, `docker/ci/`). This extends — does not supersede — `release.review-verdict-gate` (#303 H10), whose comment convention remains the human-readable artifact and the hook''s condition (c).'
|
||||
rule: 'The H10 review verdict is written as a `review-verdict/h10` Gitea **commit status** on the exact reviewed sha by `scripts/post-review-verdict.sh`, and that context is a REQUIRED status check on `main`. Because a status belongs to one sha, a later commit cannot inherit it, so Gitea''s own `merge_when_checks_succeed` refuses to merge a head no one reviewed. The PreToolUse hook additionally refuses to SCHEDULE an auto-merge unless that status is already green on head. A `pull_request_target` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.codex/`, `.gitea/`, `.husky/`, `scripts/`, `docker/ci/`). This extends — does not supersede — `release.review-verdict-gate` (#303 H10), whose comment convention remains the human-readable artifact and the hook''s condition (c).'
|
||||
signals: 'merge_when_checks_succeed freezes consent, auto-merge merges an unreviewed head, verdict bound to sha, review-verdict/h10 required check, post-review-verdict.sh, Renovate platformAutomerge exemption · paths: `scripts/post-review-verdict.sh`, `.gitea/workflows/review-verdict.yml`, `.claude/hooks/pretooluse-merge-consent.sh` · issues: #622, #303 (H6/H10), #242, #619'
|
||||
mechanics: '`scripts/tests/test_post_review_verdict.py` (incl. a TOCTOU head-moved case and cross-checks against the hook''s own condition-(c) regexes); branch protection `status_check_contexts` on `main`'
|
||||
---
|
||||
|
||||
@@ -121,6 +121,27 @@ if [ -z "$base_before" ] || [ "$base_before" != "$expected_base" ]; then
|
||||
echo "pr-changed-files: PR #$pr targets '${base_before:-<unreadable>}', not the expected '$expected_base' — the diff would be computed against a different base, failing closed" >&2
|
||||
exit 1
|
||||
fi
|
||||
# Also capture the base's TIP at this same read (ersatztv#707). This costs no extra round trip —
|
||||
# `prjson_before` is already fetched above for the `.base.ref` check. It answers a DIFFERENT
|
||||
# question than that check does, and the two are not interchangeable:
|
||||
# * `.base.ref` (above) answers "did this PR RETARGET to a different branch" — comparing branch
|
||||
# NAMES is deliberate there (ersatztv#698 route 1 / ersatztv#632), because comparing tip shas
|
||||
# for that purpose would self-deadlock: `main` advancing on every unrelated merge would fail
|
||||
# every open enumeration even though the PR still targets the same branch it always did.
|
||||
# * `.base.sha` (here) answers "did `$expected_base` ADVANCE while THIS enumeration was running."
|
||||
# `/pulls/{n}/files` diffs against the base's LIVE tip and is offset-paged over several round
|
||||
# trips; if `main` gains a commit mid-enumeration, Gitea recomputes each subsequent page against
|
||||
# the new tip independently, so rows can drop out of the result entirely (a file `main` no longer
|
||||
# differs on) while later rows shift into offset ranges already consumed on the old tip. The
|
||||
# result reads as a complete, ordinary list — `.base.ref` never changed, `.head.sha` never
|
||||
# changed, page count and termination all look normal — while silently omitting a page's worth of
|
||||
# changed paths, including possibly the only code file in the diff. This is a narrower, additional
|
||||
# check layered on top of the ref check, not a replacement for it.
|
||||
base_sha_before=$(printf '%s' "$prjson_before" | jq -r '.base.sha // ""' 2>/dev/null || true)
|
||||
if [ -z "$base_sha_before" ]; then
|
||||
echo "pr-changed-files: could not read PR #$pr's base tip sha before enumerating — failing closed" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
PAGE_SIZE=50
|
||||
MAX_PAGES=40 # 2000 files; beyond this we refuse rather than guess
|
||||
@@ -240,5 +261,18 @@ if [ -z "$base_after" ] || [ "$base_after" != "$expected_base" ]; then
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Same window, the tip-advance question this time (ersatztv#707; see the comment at
|
||||
# `base_sha_before` above for why this is a DIFFERENT check from `.base.ref`, not a duplicate of
|
||||
# it). `prjson` is already fetched above to bind the head sha, so this is the same re-read, not a
|
||||
# new round trip. `$expected_base`'s branch name can be unchanged across the whole enumeration
|
||||
# while its TIP moved partway through — the exact #707 window: no retarget, no head movement,
|
||||
# nothing the ref check or the head-sha check can see, yet later pages were diffed against a base
|
||||
# earlier pages never saw.
|
||||
base_sha_after=$(printf '%s' "$prjson" | jq -r '.base.sha // ""' 2>/dev/null || true)
|
||||
if [ -z "$base_sha_after" ] || [ "$base_sha_after" != "$base_sha_before" ]; then
|
||||
echo "pr-changed-files: base '$expected_base' advanced during enumeration (${base_sha_before:0:7} -> ${base_sha_after:0:7}) — later pages may have been diffed against a base earlier pages were not, failing closed" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
printf '%s\n' "$files" | grep -v '^$' || true
|
||||
exit 0
|
||||
|
||||
@@ -70,8 +70,20 @@ if "/pulls/" in url:
|
||||
# how this stub failed when the argument was added: the eight failures were all positive cases.
|
||||
# Fail-closed, so not dangerous, but it would have made the advisory hook prompt on every
|
||||
# docs-only PR.
|
||||
#
|
||||
# `.base.sha` joined it for the same reason one release later (ersatztv#707), and the symptom
|
||||
# repeated almost exactly: NINE failures, every one a positive control, because the enumeration
|
||||
# now binds the base's TIP across the paging window and an absent tip fails closed. Worth stating
|
||||
# as a standing property of this stub rather than a second anecdote — it serves the fields the
|
||||
# SHARED enumeration reads, so every new binding the script learns must be modelled here too, and
|
||||
# the tell is always a wave of positive cases going red at once.
|
||||
base_sha = os.environ.get("STUB_BASE_SHA", "b" * 40)
|
||||
alt_base = state / "base_sha_after.txt"
|
||||
if alt_base.exists() and nread >= 1:
|
||||
base_sha = alt_base.read_text().strip()
|
||||
print(json.dumps({"head": {"sha": shas[0]},
|
||||
"base": {"ref": os.environ.get("STUB_BASE", "main")},
|
||||
"base": {"ref": os.environ.get("STUB_BASE", "main"),
|
||||
"sha": base_sha},
|
||||
"body": "no linked issue here"}))
|
||||
sys.exit(0)
|
||||
|
||||
|
||||
@@ -80,7 +80,21 @@ if "/pulls/" in url:
|
||||
if base_after.exists() and n >= 1: # retargeted mid-enumeration
|
||||
base = base_after.read_text().strip()
|
||||
|
||||
print(json.dumps({"head": {"sha": sha}, "base": {"ref": base}}))
|
||||
# The base's TIP sha (ersatztv#707) — independent of the branch NAME above. Defaults to a fixed
|
||||
# value so an unmoved base is the default for every existing test that never touches this.
|
||||
base_sha = os.environ.get("STUB_BASE_SHA", "aaaa000000000000000000000000000000000a")
|
||||
base_sha_before = state / "pr_base_sha_before.txt"
|
||||
base_sha_after = state / "pr_base_sha_after.txt"
|
||||
if base_sha_before.exists() and n == 0:
|
||||
base_sha = base_sha_before.read_text().strip()
|
||||
if base_sha_after.exists() and n >= 1:
|
||||
base_sha = base_sha_after.read_text().strip()
|
||||
|
||||
base_obj = {"ref": base}
|
||||
if base_sha != "MISSING":
|
||||
base_obj["sha"] = base_sha
|
||||
|
||||
print(json.dumps({"head": {"sha": sha}, "base": base_obj}))
|
||||
sys.exit(0)
|
||||
|
||||
print("{}")
|
||||
@@ -125,6 +139,21 @@ def enumerate_files(tmp_path):
|
||||
"""The PR is retargeted to `ref` between the files pages and the binding re-read."""
|
||||
(state / "pr_base_after.txt").write_text(ref)
|
||||
|
||||
def base_sha_is_already(self, sha):
|
||||
"""The base tip sits at `sha` for the WHOLE run — i.e. it advanced before this
|
||||
enumeration started, which is outside the window the #707 guard polices."""
|
||||
(state / "pr_base_sha_before.txt").write_text(sha)
|
||||
(state / "pr_base_sha_after.txt").write_text(sha)
|
||||
|
||||
def base_sha_moves_to(self, sha):
|
||||
"""The base branch's TIP advances to `sha` between the files pages and the binding
|
||||
re-read, with the branch NAME unchanged (ersatztv#707)."""
|
||||
(state / "pr_base_sha_after.txt").write_text(sha)
|
||||
|
||||
def base_sha_missing_after(self):
|
||||
"""The base tip sha is absent/unparseable on the post-paging re-read."""
|
||||
(state / "pr_base_sha_after.txt").write_text("MISSING")
|
||||
|
||||
def run(self, expected_sha=SHA, args=("timothy", "ersatztv", "42"),
|
||||
expected_base="main"):
|
||||
argv = ["bash", str(SCRIPT), *args, expected_sha]
|
||||
@@ -343,6 +372,78 @@ def test_positive_control_an_unmoved_base_still_enumerates(enumerate_files):
|
||||
assert enumerate_files.paths() == ["docs/a.md", "ErsatzTV/Program.cs"]
|
||||
|
||||
|
||||
def test_a_BASE_ADVANCE_DURING_enumeration_fails_closed(enumerate_files):
|
||||
"""ersatztv#707. `/pulls/{n}/files` diffs EACH PAGE against the base's LIVE tip; if `main`
|
||||
advances between the pre-paging read and the post-paging re-read, later pages can be diffed
|
||||
against a base earlier pages never saw. Rows can drop out of the result entirely (a file `main`
|
||||
no longer differs on) while later rows shift into offset ranges already consumed against the old
|
||||
tip. `.base.ref` never changes here — that is the entire point of the defect and of this being an
|
||||
ADDITIONAL guard layered on top of the existing retarget check, not a reclassification of it."""
|
||||
enumerate_files.set_pages(_rows(["docs/a.md", "ErsatzTV/Program.cs"]))
|
||||
enumerate_files.base_sha_moves_to("cccc000000000000000000000000000000000c")
|
||||
r = enumerate_files.run()
|
||||
assert r.returncode != 0, f"expected fail-closed, got {r.returncode}: stdout={r.stdout!r}"
|
||||
assert not r.stdout.strip(), f"stdout must be meaningless on a failed enumeration: {r.stdout!r}"
|
||||
assert "base" in r.stderr and "advanced" in r.stderr, (
|
||||
f"stderr should name the base-advance failure distinctly, got: {r.stderr!r}")
|
||||
assert re.search(r"[0-9a-f]{7}", r.stderr), (
|
||||
f"stderr should name both short shas involved, got: {r.stderr!r}")
|
||||
|
||||
|
||||
def test_positive_control_an_UNMOVED_base_sha_still_enumerates_across_pages(enumerate_files):
|
||||
"""Without this, the fail-closed tests around it could be passing only because the new guard
|
||||
broke the ordinary path outright rather than because it correctly distinguishes movement from
|
||||
no movement. Deliberately multi-page, to prove the guard survives several round trips."""
|
||||
enumerate_files.set_pages(_rows([f"docs/f{i}.md" for i in range(50)]),
|
||||
_rows(["ErsatzTV/Program.cs"]))
|
||||
paths = enumerate_files.paths()
|
||||
assert len(paths) == 51
|
||||
assert "ErsatzTV/Program.cs" in paths
|
||||
|
||||
|
||||
def test_the_707_WINDOW_base_ref_unchanged_but_TIP_advanced_still_fails(enumerate_files):
|
||||
"""The exact #707 window, and the centerpiece of this suite. The base BRANCH NAME is unchanged
|
||||
throughout (`main` -> `main`), so the pre-existing `.base.ref` retarget guard sees nothing to
|
||||
object to — by construction, since a mere advance is not a retarget. Only the branch's TIP moved.
|
||||
A test that passes here proves the NEW sha-based guard is what fired, not the old ref guard,
|
||||
which cannot see this case at all."""
|
||||
enumerate_files.set_pages(_rows(["docs/a.md"]), _rows(["ErsatzTV/Program.cs"]))
|
||||
enumerate_files.base_sha_moves_to("dddd000000000000000000000000000000000d")
|
||||
r = enumerate_files.run() # expected_base="main" throughout; ref never moves
|
||||
assert r.returncode != 0, (
|
||||
"base ref stayed 'main' but its tip advanced mid-enumeration; must still fail closed "
|
||||
f"(got {r.returncode}, stdout={r.stdout!r}, stderr={r.stderr!r})")
|
||||
|
||||
|
||||
def test_a_base_that_ADVANCED_BEFORE_the_enumeration_STILL_SUCCEEDS(enumerate_files):
|
||||
"""ersatztv#707, the deadlock this guard must NOT cause — and the reason the check is scoped to
|
||||
the window rather than to the base being "current".
|
||||
|
||||
`main` advances constantly. If the guard compared the base tip against anything other than what
|
||||
it observed at the START of THIS enumeration, every open PR would fail closed on every unrelated
|
||||
merge to `main` — losing its exemption for reasons that have nothing to do with it. Here the base
|
||||
already sits at a tip different from the suite's default before the first page is requested, and
|
||||
then holds still: that is an ordinary, healthy PR and it must enumerate normally.
|
||||
|
||||
Note this is deliberately NOT the same test as the unmoved-base control: that one pins "nothing
|
||||
happened", this one pins "something happened, but OUTSIDE the window, so it is none of our
|
||||
business."
|
||||
"""
|
||||
enumerate_files.base_sha_is_already("eeee000000000000000000000000000000000e")
|
||||
enumerate_files.set_pages(_rows(["docs/a.md", "docs/b.md"]), [])
|
||||
assert enumerate_files.paths() == ["docs/a.md", "docs/b.md"], (
|
||||
"a PR whose base advanced BEFORE this enumeration began was failed closed; the guard is "
|
||||
"comparing against a stale expectation instead of the tip it actually started from")
|
||||
|
||||
|
||||
def test_an_UNREADABLE_base_sha_on_the_AFTER_read_fails_closed(enumerate_files):
|
||||
"""Consistent with every other guard in this script: an absent/unparseable field is never read
|
||||
as 'no movement' — it is fail-closed, same as the head-sha and base-ref binding above."""
|
||||
enumerate_files.set_pages(_rows(["docs/a.md"]))
|
||||
enumerate_files.base_sha_missing_after()
|
||||
assert enumerate_files.fails_closed()
|
||||
|
||||
|
||||
def test_the_base_comparator_is_the_BRANCH_NAME_never_the_TIP_SHA():
|
||||
"""`.base.ref` is compared, never `.base.sha` — matching `post-review-verdict.sh` (ersatztv#632).
|
||||
|
||||
@@ -789,9 +890,90 @@ if "-X" in args and args[args.index("-X") + 1] == "POST":
|
||||
payload = args[args.index("-d") + 1]
|
||||
(out / "posted.json").write_text(payload)
|
||||
(out / "posted_url.txt").write_text(url)
|
||||
# EVERY post is also appended (ersatztv#706 race 2): the repair path POSTs a second time, and a
|
||||
# test that only ever saw the last write could not tell "posted success, then repaired it to
|
||||
# pending" from "posted pending once".
|
||||
#
|
||||
# Re-serialized COMPACT rather than appended verbatim: `jq -n` pretty-prints by default, so the
|
||||
# payloads arrive spanning several lines and a raw append would produce a file that is not JSONL
|
||||
# at all. (Found the honest way — the first draft of this recorder broke both race-2 tests.)
|
||||
with (out / "posted_all.jsonl").open("a") as fh:
|
||||
fh.write(json.dumps(json.loads(payload)) + "\n")
|
||||
print("{}")
|
||||
sys.exit(0)
|
||||
|
||||
# The PR timeline, which the retarget fence counts (ersatztv#706 race 1). Checked BEFORE the status
|
||||
# branch: the fence's URL does not contain "/status", but keeping the order explicit means a later
|
||||
# edit to either pattern cannot silently reroute one endpoint into the other's handler.
|
||||
#
|
||||
# Real shapes, measured on this instance and deliberately mirrored, because the job's guards are
|
||||
# type-sensitive: this endpoint returns a BARE ARRAY of events, each with a `type`; a retarget is
|
||||
# `change_target_branch` (confirmed on PR #703, the route-1 reproduction, which carries exactly two;
|
||||
# and on PR #717, never retargeted, which carries none).
|
||||
if "/timeline" in url:
|
||||
mode = os.environ.get("STUB_TIMELINE_MODE", "none")
|
||||
page = 1
|
||||
for part in url.split("?", 1)[-1].split("&"):
|
||||
if part.startswith("page="):
|
||||
page = int(part.split("=", 1)[1])
|
||||
if mode == "unreadable":
|
||||
print("<html>502 Bad Gateway</html>")
|
||||
sys.exit(0)
|
||||
if mode == "transport-error":
|
||||
sys.exit(22)
|
||||
# Page 2+ is always the validated EMPTY page that terminates the walk. Without a real terminator
|
||||
# the job would page to its cap and refuse to trust the count, which is a different branch from
|
||||
# the one most of these tests mean to exercise.
|
||||
if page > 1:
|
||||
print("[]")
|
||||
sys.exit(0)
|
||||
n_before, n_after = 0, 0
|
||||
if mode.startswith("stable:"):
|
||||
n_before = n_after = int(mode.split(":", 1)[1])
|
||||
elif mode.startswith("moves:"):
|
||||
# "moves:A,B" — A retarget events on the fence's FIRST count, B on the re-count taken just
|
||||
# before the POST. This is the race-1 window: the PR was retargeted while the job classified.
|
||||
a, b = mode.split(":", 1)[1].split(",")
|
||||
n_before, n_after = int(a), int(b)
|
||||
ctr = out / "timeline_reads.txt"
|
||||
seen = int(ctr.read_text()) if ctr.exists() else 0
|
||||
ctr.write_text(str(seen + 1))
|
||||
n = n_before if seen == 0 else n_after
|
||||
print(json.dumps([{"id": 1000 + i, "type": "change_target_branch",
|
||||
"old_ref": "main", "new_ref": "scratch"} for i in range(n)]
|
||||
+ [{"id": 900, "type": "comment"}]))
|
||||
sys.exit(0)
|
||||
|
||||
# The per-POST status HISTORY (`/statuses/{sha}`), read by the race-2 post-write verification. It is
|
||||
# a BARE ARRAY, one row per POST — a different shape AND a different meaning from the combined
|
||||
# `/commits/{sha}/status` below, which returns an object carrying the LATEST row per context. Both
|
||||
# shapes were measured on the same live head (24 rows vs 12). Modelling them identically would have
|
||||
# made the post-write check untestable, since the thing it looks for is precisely a row the combined
|
||||
# endpoint no longer shows.
|
||||
if "/statuses/" in url:
|
||||
mode = os.environ.get("STUB_HISTORY_MODE", "none")
|
||||
rows = []
|
||||
if mode.startswith("human-after-post"):
|
||||
# The raced verdict: absent when the high-water mark is taken, present afterwards. Its id is
|
||||
# ABOVE the mark, which is what makes it detectable.
|
||||
ctr = out / "history_reads.txt"
|
||||
seen = int(ctr.read_text()) if ctr.exists() else 0
|
||||
ctr.write_text(str(seen + 1))
|
||||
if seen > 0:
|
||||
rows = [{"id": 5000, "context": "review-verdict/h10", "status": "failure",
|
||||
"creator": {"login": "timothy"},
|
||||
"description": "Review-verdict: BLOCKED @ a9e3e23 (base: main)"}]
|
||||
elif mode == "stale-human-already-present":
|
||||
# A human verdict that was ALREADY in the history before this run — e.g. one whose recorded
|
||||
# base did not match, which `read_existing_verdict` deliberately declines to honour. It must
|
||||
# NOT be mistaken for a raced write, or every later run of that PR would repair its own
|
||||
# exemption to pending forever.
|
||||
rows = [{"id": 10, "context": "review-verdict/h10", "status": "success",
|
||||
"creator": {"login": "timothy"},
|
||||
"description": "Review-verdict: MERGEABLE @ a9e3e23 (base: other)"}]
|
||||
print(json.dumps(rows))
|
||||
sys.exit(0)
|
||||
|
||||
if "/status" in url:
|
||||
# Configurable. Hardcoding "no verdict yet" left the ersatztv#647 emptiness guard and the
|
||||
# never-overwrite short-circuit unreachable: neither could be made to fire, so mutations
|
||||
@@ -849,7 +1031,8 @@ print("{}")
|
||||
def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy",
|
||||
status_mode: str = "none", jq16: bool = False,
|
||||
status_creator: str | None = "timothy",
|
||||
status_desc: str = "Review-verdict: MERGEABLE @ a9e3e23 (base: main)"):
|
||||
status_desc: str = "Review-verdict: MERGEABLE @ a9e3e23 (base: main)",
|
||||
timeline_mode: str = "none", history_mode: str = "none"):
|
||||
"""Execute the workflow's classify `run:` block with a stubbed enumeration script.
|
||||
|
||||
Returns the status payload the job POSTed, or None if it posted nothing.
|
||||
@@ -873,6 +1056,8 @@ def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy",
|
||||
env["STUB_STATUS_MODE"] = status_mode
|
||||
env["STUB_STATUS_CREATOR"] = status_creator or ""
|
||||
env["STUB_STATUS_DESC"] = status_desc
|
||||
env["STUB_TIMELINE_MODE"] = timeline_mode
|
||||
env["STUB_HISTORY_MODE"] = history_mode
|
||||
env.update({
|
||||
"GITEA_TOKEN": "stub",
|
||||
"BASE_URL": "http://gitea.example/api/v1",
|
||||
@@ -1542,3 +1727,240 @@ def test_a_branch_name_containing_a_PAREN_cannot_truncate_into_the_current_base(
|
||||
"a verdict recorded against branch 'main)evil' was inherited by a PR targeting 'main' — the "
|
||||
f"base value is being truncated at the first ')'. Log:\n{r.stdout[-600:]}")
|
||||
assert posted["state"] == "pending"
|
||||
|
||||
|
||||
# --- ersatztv#706: the retarget fence (race 1) and the post-write repair (race 2) -----------------
|
||||
#
|
||||
# Race 1 was reproduced LIVE before any of this was written, because the fix turns on which Gitea
|
||||
# behaviours are real rather than on what the docs imply. Measured on Gitea 1.25.4:
|
||||
# * `pull_request_target` runs for one PR are NOT auto-cancelled — probe PR #722, run 7520
|
||||
# (`opened`) ran to completion 20s AFTER run 7521 (`synchronize`) started. The older run finished
|
||||
# LAST, which is exactly the mechanism by which a stale classification overwrites a fresh one.
|
||||
# * A NON-CANCELLING concurrency group does not serialize them either: with the group active, runs
|
||||
# 7528 and 7529 still overlapped and 7528 ended 36s after 7529 began. #706's headline proposal is
|
||||
# therefore refuted, not merely unattractive.
|
||||
# * Gitea DOES auto-cancel superseded `push` runs on a branch — a control workflow carrying no
|
||||
# `concurrency:` key at all showed that — which is why the concurrency-group probe was confounded
|
||||
# until the control separated the two.
|
||||
# The fence keys on the timeline's `change_target_branch` COUNT because the branch NAME is
|
||||
# ABA-vulnerable (`main → S → main` reads `main` at both ends), while the count is monotonic.
|
||||
|
||||
def _posted_sequence(tmp_path):
|
||||
"""Every status POST the job made, in order — not just the last one."""
|
||||
f = tmp_path / "posted_all.jsonl"
|
||||
return [json.loads(l) for l in f.read_text().splitlines() if l.strip()] if f.exists() else []
|
||||
|
||||
|
||||
def test_a_RETARGET_DURING_the_run_posts_NOTHING(tmp_path):
|
||||
"""THE race-1 test. A docs-only PR that would otherwise be exempted is retargeted while the job
|
||||
classifies, so this run's answer describes a base the PR may no longer target. It must write
|
||||
nothing at all and leave the field to the successor run the retarget's `edited` event queues."""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode="moves:0,1")
|
||||
assert r.returncode == 0, r.stderr
|
||||
assert posted is None, (
|
||||
"a stale run posted its classification after the PR was retargeted underneath it — the "
|
||||
f"retarget fence did not fire. Log:\n{r.stdout[-900:]}")
|
||||
# Assert the DISCRIMINATOR, not just the outcome: several unrelated failures also end in "posted
|
||||
# nothing", so the state alone cannot tell a working fence from a broken job.
|
||||
assert "retargeted while this job was classifying" in r.stdout, (
|
||||
f"nothing was posted, but NOT via the retarget fence. Log:\n{r.stdout[-900:]}")
|
||||
|
||||
|
||||
def test_positive_control_a_QUIET_run_still_posts_its_exemption(tmp_path):
|
||||
"""Without this, the test above passes against a job that simply never posts."""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode="none")
|
||||
assert posted is not None and posted["state"] == "success", (
|
||||
f"an undisturbed docs-only PR lost its exemption. Log:\n{r.stdout[-900:]}")
|
||||
|
||||
|
||||
def test_a_PR_retargeted_BEFORE_the_run_but_QUIET_during_it_is_STILL_exempt(tmp_path):
|
||||
"""The fence keys on MOTION during this run, never on "has ever been retargeted".
|
||||
|
||||
A PR legitimately retargeted once, long before this run, carries a permanently non-zero event
|
||||
count. Keying on the count being non-zero — rather than on it CHANGING — would deadlock that PR's
|
||||
exemption forever, which is a worse failure than the race being fixed.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), timeline_mode="stable:2")
|
||||
assert posted is not None and posted["state"] == "success", (
|
||||
"a PR with a settled, non-zero retarget history was refused its exemption — the fence is "
|
||||
f"testing the count's VALUE instead of its MOVEMENT. Log:\n{r.stdout[-900:]}")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("mode", ["unreadable", "transport-error"])
|
||||
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 posted is None, (
|
||||
"an exemption success was posted even though the retarget count could not be established "
|
||||
f"({mode}). Log:\n{r.stdout[-900:]}")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("mode", ["unreadable", "transport-error"])
|
||||
def test_an_UNTRUSTED_retarget_count_STILL_LETS_PENDING_THROUGH(tmp_path, mode):
|
||||
"""The other half, and the one that keeps a timeline outage from taking every PR down with it.
|
||||
|
||||
`pending` cannot turn an unreviewed head green — it is what blocks the merge — so withholding it
|
||||
buys no safety and would strand ordinary PRs with no status at all. Only the exemption is gated.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("ErsatzTV/Program.cs"), timeline_mode=mode)
|
||||
assert posted is not None and posted["state"] == "pending", (
|
||||
"an ordinary PR was left with NO status because the timeline was unreadable; only the "
|
||||
f"exemption success should be gated on the count. Log:\n{r.stdout[-900:]}")
|
||||
|
||||
|
||||
def test_a_human_verdict_landing_AFTER_the_POST_is_repaired_to_pending(tmp_path):
|
||||
"""Race 2's remainder. The pre-POST re-read cannot see a verdict that lands between it and the
|
||||
write, and there is no compare-and-set to make the two one operation. So the write is verified
|
||||
AFTERWARDS and repaired in the safe direction: silently greening an explicit human BLOCKED is the
|
||||
worst outcome this gate can produce, and strictly worse than a stall.
|
||||
|
||||
The repair is `pending`, never a copy of the human's `failure` — re-posting their state under the
|
||||
machine credential would attribute a human verdict to this job.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="human-after-post")
|
||||
assert r.returncode == 0, r.stderr
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 2, (
|
||||
"expected the exemption write followed by a repair; the post-write verification did not "
|
||||
f"fire. Posts: {seq}. Log:\n{r.stdout[-900:]}")
|
||||
assert seq[0]["state"] == "success"
|
||||
assert seq[1]["state"] == "pending", f"the raced exemption was not repaired: {seq}"
|
||||
assert posted is not None and posted["state"] == "pending"
|
||||
|
||||
|
||||
def test_a_PRE_EXISTING_human_row_does_NOT_trigger_a_repair(tmp_path):
|
||||
"""The false-positive guard, and the reason the check compares ids instead of asking "does a human
|
||||
verdict exist".
|
||||
|
||||
`read_existing_verdict` deliberately declines to honour a human verdict whose recorded base does
|
||||
not match this PR's — so such a row sits in the history forever. A presence test would fire on it
|
||||
on EVERY later run, repair each exemption to `pending`, and permanently deadlock a PR that had one
|
||||
mismatched verdict once. Only rows newer than the high-water mark count.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"),
|
||||
history_mode="stale-human-already-present")
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 1, (
|
||||
"an old, already-present human verdict row was mistaken for one that raced this run's write, "
|
||||
f"so the exemption was repaired away. Posts: {seq}. Log:\n{r.stdout[-900:]}")
|
||||
assert posted is not None and posted["state"] == "success"
|
||||
|
||||
|
||||
# --- ersatztv#711: the PROTECTED list must cover every mirror of the enforcement hooks -----------
|
||||
|
||||
def test_a_CODEX_hook_copy_is_a_PROTECTED_path(tmp_path):
|
||||
"""`.codex/hooks/` is a byte-identical mirror of `.claude/hooks/`, including
|
||||
`pretooluse-merge-consent.sh`. Editing the `.claude/` copy correctly voids both exemptions;
|
||||
before this, editing its `.codex/` twin did not — the rule "a PR that can weaken the gate must
|
||||
not exempt itself from the gate" was written as a path list and the list had gone incomplete.
|
||||
|
||||
Latent rather than live today (`.codex/` is untracked and gitignored, and a PR cannot touch a
|
||||
path that is not in the repo), which is exactly why it needs a test: it becomes live silently,
|
||||
the moment anyone tracks the directory.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md", ".codex/hooks/pretooluse-merge-consent.sh"))
|
||||
assert posted is not None, f"the job posted nothing: {r.stderr[-2000:]}"
|
||||
assert posted["state"] == "pending", (
|
||||
"a PR editing a .codex/ copy of the merge-consent hook was granted the docs-only exemption "
|
||||
f"(desc={posted['description']!r})")
|
||||
# Assert the DISCRIMINATOR: a non-exempt outcome is reachable by several routes, so the state
|
||||
# alone cannot show the PROTECTED branch is what rejected it.
|
||||
assert "protected" in r.stdout.lower(), (
|
||||
f"rejected, but NOT by the protected-path branch. Decision log:\n{r.stdout[-800:]}")
|
||||
|
||||
|
||||
def test_BOTH_hook_directories_are_in_the_PROTECTED_pattern():
|
||||
"""A structural pin over the list itself. The behavioural test above proves `.codex/` is covered
|
||||
today; this one states the INVARIANT — the two hook directories are mirrors, so any future edit
|
||||
that drops one while keeping the other is the bug, not a simplification."""
|
||||
src = _code_lines(WORKFLOW)
|
||||
protected = [l for l in src.splitlines() if l.strip().startswith("PROTECTED=")]
|
||||
assert len(protected) == 1, f"expected exactly one PROTECTED definition, found {protected}"
|
||||
for d in (r"\.claude/", r"\.codex/"):
|
||||
assert d in protected[0], (
|
||||
f"{d} is missing from PROTECTED ({protected[0].strip()!r}) — a directory carrying a copy "
|
||||
"of the enforcement hooks can exempt itself from the gate it enforces")
|
||||
|
||||
|
||||
# --- ersatztv#706, round 2: findings from the cold adversarial review ----------------------------
|
||||
|
||||
def test_the_high_water_MARK_is_captured_BEFORE_the_last_moment_re_read():
|
||||
"""The High finding of round 2, pinned as the ORDERING property it actually is.
|
||||
|
||||
The mark was originally taken "as late as possible", just before the POST. That leaves everything
|
||||
between the last-moment re-read and the mark as a blind gap: a human verdict landing there is
|
||||
invisible to the re-read (already done) and excluded from the post-write check (its id is BELOW a
|
||||
mark taken afterwards), so it is overwritten with no repair. The gap spanned the whole retarget
|
||||
re-count — up to 20 timeline round-trips — not the single round-trip that was being claimed.
|
||||
|
||||
Taking the mark FIRST closes the read side: any row newer than the mark is caught either by the
|
||||
re-read (abstain) or by the post-write check (repair). This is asserted structurally because the
|
||||
defect is an order, not an output — with the mark late the job still posts and still repairs in
|
||||
every scenario a stub can pose; only the id arithmetic silently changes.
|
||||
"""
|
||||
src = _classify_step()["run"]
|
||||
mark = src.index("max_id_before=-1")
|
||||
# The LAST-MOMENT re-read is the second bare `read_existing_verdict` call.
|
||||
calls = [i for i in range(len(src)) if src.startswith("read_existing_verdict\n", i)]
|
||||
assert len(calls) >= 2, f"expected two read_existing_verdict call sites, found {len(calls)}"
|
||||
assert mark < calls[-1], (
|
||||
"the high-water mark is captured AFTER the last-moment re-read, reopening the blind window "
|
||||
"in which a human verdict is neither seen by the re-read nor repaired by the post-write check")
|
||||
|
||||
|
||||
def test_a_previously_REPAIRED_head_is_never_re_exempted(tmp_path):
|
||||
"""The Medium finding of round 2: the repair used to last exactly one event.
|
||||
|
||||
After a repair, the status is a machine-written `pending` — indistinguishable, to the next run,
|
||||
from an ordinary one. That run re-derived it, posted `success`, and took a fresh high-water mark
|
||||
ABOVE the human row, so the post-write check stayed silent and the human's rejection went green
|
||||
again one event later. The repair description is now a sentinel the classification recognises.
|
||||
"""
|
||||
posted, r = _run_classify(
|
||||
tmp_path, _emitting("docs/a.md"),
|
||||
status_mode="existing:pending", status_creator=None,
|
||||
status_desc="Human verdict raced this exemption write — re-post the verdict")
|
||||
assert posted is not None, f"the job posted nothing: {r.stderr[-2000:]}"
|
||||
assert posted["state"] == "pending", (
|
||||
"a head whose human verdict was previously overwritten was granted a FRESH exemption, burying "
|
||||
f"the rejection again (desc={posted['description']!r})")
|
||||
assert "raced a previous exemption" in r.stdout, (
|
||||
f"rejected, but not via the repair-sentinel branch. Log:\n{r.stdout[-800:]}")
|
||||
|
||||
|
||||
def test_positive_control_an_ORDINARY_machine_pending_is_still_re_derived(tmp_path):
|
||||
"""Without this, the test above would pass against a job that had stopped exempting anything with
|
||||
a pre-existing `pending`. An ordinary machine `pending` — no sentinel — must still re-derive to
|
||||
`success` for a docs-only PR."""
|
||||
posted, r = _run_classify(
|
||||
tmp_path, _emitting("docs/a.md"),
|
||||
status_mode="existing:pending", status_creator=None,
|
||||
status_desc="Awaiting review verdict for a9e3e23")
|
||||
assert posted is not None and posted["state"] == "success", (
|
||||
f"an ordinary pending was not re-derived to an exemption. Log:\n{r.stdout[-800:]}")
|
||||
|
||||
|
||||
def test_the_fence_gates_PENDING_TOO_not_only_the_exemption(tmp_path):
|
||||
"""The fence deliberately refuses to write `pending` as well as `success` when the PR was
|
||||
retargeted mid-run. Untested, a success-only-fence mutant would have passed the whole suite.
|
||||
|
||||
A stale `pending` is only a stall rather than a forged green, so gating it is not strictly
|
||||
required — but the successor run is guaranteed either way, so there is nothing to buy by writing a
|
||||
value computed against a base the PR may no longer target.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("ErsatzTV/Program.cs"), timeline_mode="moves:0,1")
|
||||
assert posted is None, (
|
||||
"the fence let a stale `pending` through; it is gating only the exemption path "
|
||||
f"(desc={posted['description']!r})" if posted else "")
|
||||
assert "retargeted while this job was classifying" in r.stdout
|
||||
|
||||
|
||||
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 posted is None
|
||||
assert r.returncode == 0, f"the job died rather than declining cleanly: {r.stderr[-800:]}"
|
||||
assert "Could not establish a trusted retarget count" in r.stdout, (
|
||||
f"nothing posted, but not via the untrusted-count branch. Log:\n{r.stdout[-800:]}")
|
||||
|
||||
Reference in New Issue
Block a user