Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
 renovate
|
6b41340abe |
@@ -3,7 +3,7 @@
|
||||
"isRoot": true,
|
||||
"tools": {
|
||||
"jetbrains.resharper.globaltools": {
|
||||
"version": "2025.3.5",
|
||||
"version": "2025.3.4.1",
|
||||
"commands": [
|
||||
"jb"
|
||||
],
|
||||
|
||||
@@ -183,14 +183,6 @@ 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"
|
||||
@@ -226,23 +218,7 @@ 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.
|
||||
#
|
||||
# `.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/)'
|
||||
PROTECTED='^(\.claude/|\.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
|
||||
@@ -354,10 +330,6 @@ 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
|
||||
@@ -402,72 +374,6 @@ 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
|
||||
@@ -602,252 +508,33 @@ jobs:
|
||||
fi
|
||||
fi
|
||||
|
||||
if [ "$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. 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"
|
||||
elif [ "$ex_repair" = yes ]; then
|
||||
# CARRY THE SENTINEL FORWARD. This branch exists because the first version of it did not,
|
||||
# and cold review reproduced the consequence: refusing the exemption but posting the
|
||||
# GENERIC pending description overwrote the very sentinel the refusal depends on, so the
|
||||
# next run saw an ordinary machine `pending`, re-derived it, and posted `success` — burying
|
||||
# the human rejection two events after the repair instead of one. The block has to be a
|
||||
# FIXED POINT: what this branch writes must be what re-triggers this branch.
|
||||
#
|
||||
# It is keyed on `ex_repair` alone rather than on the exempt path, so the marker also
|
||||
# survives runs where the PR was not exemptible anyway — the fact being recorded is "a
|
||||
# human verdict was lost on this sha", which is a property of the sha, not of this run's
|
||||
# classification.
|
||||
state=pending
|
||||
desc="$REPAIR_DESC"
|
||||
else
|
||||
state=pending
|
||||
desc="Awaiting review verdict for ${SHA:0:7}"
|
||||
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 — which is what the post-write repair below is for.
|
||||
# POST below is still lost. Said plainly here rather than left as an implied guarantee.
|
||||
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
|
||||
|
||||
# A SENTINEL THAT APPEARED MID-RUN (ersatztv#706, round-3 review). The re-read above recomputes
|
||||
# `ex_repair`, and until this guard existed nothing downstream read it: the POST writes the
|
||||
# `$state` frozen at classification time, so a STALE OVERLAPPING RUN would post its `success`
|
||||
# straight over a sentinel another run had just written — burying a human rejection, with no
|
||||
# repair (the human row is below this run's mark) and no log. That fails toward SUCCESS, so it
|
||||
# was not covered by the "repair fails toward pending" residual; it is the exact outcome this
|
||||
# whole change exists to prevent, reached through the run overlap this branch itself measured.
|
||||
#
|
||||
# THE RULE IS "NEVER REPLACE A SENTINEL WITH A NON-SENTINEL", not "never overwrite it with a
|
||||
# success". A first draft of this guard tested `state = success`, which is one branch too
|
||||
# narrow: a run can reach the POST on `state=pending` carrying the GENERIC description — most
|
||||
# realistically after a transient enumeration failure (`complete != yes`) — and that run
|
||||
# passes a success-only guard, passes the fence, and overwrites the sentinel with ordinary
|
||||
# text. The next run then sees no sentinel, re-derives, and posts `success`: the same buried
|
||||
# human rejection as before, reached in two steps instead of one.
|
||||
#
|
||||
# Comparing the DESCRIPTION rather than the state is exactly as precise and strictly more
|
||||
# general. A sentinel present at the FIRST read forces `desc="$REPAIR_DESC"` (the carry-forward
|
||||
# branch in the decision above), so this guard cannot fire on the ordinary repaired-head path
|
||||
# and the fixed point is intact. Any other description alongside `ex_repair=yes` means the
|
||||
# sentinel arrived DURING this run, whatever this run concluded.
|
||||
#
|
||||
# Abstaining is strictly correct here and, unlike the retarget fence, needs no successor run:
|
||||
# the sentinel row is already `pending` and already carries the re-post instruction.
|
||||
if [ "$ex_repair" = yes ] && [ "$desc" != "$REPAIR_DESC" ]; then
|
||||
echo "::notice::A repair sentinel was written on ${SHA:0:7} while this job was classifying, meaning a human verdict was raced and repaired by another run. This run's exemption is stale — posting NOTHING and leaving the sentinel standing."
|
||||
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.
|
||||
# TWO row shapes count as "something raced this write", not one (round-5 review).
|
||||
#
|
||||
# (a) a HUMAN verdict — non-null creator, `Review-verdict:` description;
|
||||
# (b) a machine SENTINEL — null creator, description exactly `$REPAIR_DESC`.
|
||||
#
|
||||
# (b) is not decoration. With two overlapping runs A and B, the human row can land BELOW
|
||||
# A's mark (so (a) cannot see it) while B masks it with an exemption success and only
|
||||
# afterwards writes the sentinel. A then finds nothing human above its mark, does not
|
||||
# repair, and posts its own success ON TOP of the sentinel — a permanent forged green over
|
||||
# a human rejection, which is precisely the outcome this whole change exists to prevent.
|
||||
# Counting the sentinel closes it: A repairs, and both runs converge on the fixed point.
|
||||
#
|
||||
# It cannot false-fire. A sentinel that already existed would have been seen at the FIRST
|
||||
# read, forcing the pending path, and this block only runs after a `success` — so a
|
||||
# sentinel ABOVE the mark can only have been written by another run mid-flight.
|
||||
raced=$(printf '%s' "$post_hist" | jq -r --arg c "$CONTEXT" --argjson since "$max_id_before" --arg rd "$REPAIR_DESC" \
|
||||
'[.[] | select(type == "object")
|
||||
| select(.context? == $c)
|
||||
| select((.id? // 0) > $since)
|
||||
| select(
|
||||
((.creator != null and .creator.login != null and .creator.login != "")
|
||||
and (((.description // "") | startswith("Review-verdict:"))))
|
||||
or ((.creator == null) and ((.description // "") == $rd))
|
||||
)] | 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/`/`.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.
|
||||
- **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.
|
||||
- **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/`, `.codex/`, `.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/`, `.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.
|
||||
|
||||
@@ -6,7 +6,7 @@
|
||||
<ItemGroup>
|
||||
<PackageVersion Include="AsyncFixer" Version="2.1.0" />
|
||||
<PackageVersion Include="Blurhash.SkiaSharp" Version="2.0.0" />
|
||||
<PackageVersion Include="CliWrap" Version="3.10.4" />
|
||||
<PackageVersion Include="CliWrap" Version="3.10.3" />
|
||||
<PackageVersion Include="coverlet.collector" Version="6.0.4" />
|
||||
<PackageVersion Include="Dapper" Version="2.1.79" />
|
||||
<PackageVersion Include="Destructurama.Attributed" Version="5.2.0" />
|
||||
@@ -94,7 +94,7 @@
|
||||
<!-- Direct pin to override EF Core 9's transitive SQLitePCLRaw 2.1.10 (vulnerable
|
||||
bundled SQLite, GHSA-2m69-gcr7-jv3q). The 3.x line ships the patched native
|
||||
(lib.e_sqlite3 3.50.3); core 3.0.4 satisfies Microsoft.Data.Sqlite's `>= 2.1.10`. (#8) -->
|
||||
<PackageVersion Include="SQLitePCLRaw.bundle_e_sqlite3" Version="3.0.4" />
|
||||
<PackageVersion Include="SQLitePCLRaw.bundle_e_sqlite3" Version="3.0.5" />
|
||||
<PackageVersion Include="System.CommandLine" Version="2.0.2" />
|
||||
<PackageVersion Include="TagLibSharp" Version="2.3.0" />
|
||||
<PackageVersion Include="Testably.Abstractions" Version="10.0.0" />
|
||||
|
||||
@@ -593,97 +593,7 @@ public class PipelineBuilderBaseTests
|
||||
command.ShouldNotContain("-readrate_initial_burst");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Realtime_Input_Should_Catch_Up_When_Option_Is_Supported()
|
||||
{
|
||||
string command = BuildRealtimeCommand(new CatchupCapableFFmpegCapabilities());
|
||||
|
||||
// -readrate paces an input off its furthest-behind stream, so a sparse stream sharing the
|
||||
// input pins throughput below realtime; catchup lets it recover (ersatztv#726). anchor on
|
||||
// the input path so this can't be satisfied by some other input carrying the option
|
||||
// this overlaps Bitmap_Subtitle_Burn_In_... by design: that one pins the #726 MECHANISM on a
|
||||
// bitmap pipeline, this one pins the plain no-subtitle shape plus the uniqueness guard below
|
||||
command.ShouldContain("-readrate 1.05 -readrate_initial_burst 8 -readrate_catchup 6.0 -i /tmp/whatever.mkv");
|
||||
Regex.Matches(command, Regex.Escape("-readrate_catchup 6.0")).Count.ShouldBe(1);
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Realtime_Input_Should_Not_Catch_Up_A_Still_Image()
|
||||
{
|
||||
// mirrors the burst's still-image exclusion (ersatztv#350): the video input takes no readrate
|
||||
// at all, so catchup would only reach the separate audio input and run it ahead of a graph
|
||||
// that the realtime filter is already pacing. pinned so the divergence can't reappear silently
|
||||
string command = BuildRealtimeCommand(new CatchupCapableFFmpegCapabilities(), stillImage: true);
|
||||
|
||||
// the positive anchor keeps this from passing vacuously if the helper ever stops
|
||||
// producing a realtime audio input at all
|
||||
command.ShouldContain("-readrate 1.05");
|
||||
command.ShouldNotContain("-readrate_catchup");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Realtime_Input_Should_Not_Catch_Up_When_Option_Is_Unsupported()
|
||||
{
|
||||
// an older binary silently keeps today's behavior rather than failing to start
|
||||
string command = BuildRealtimeCommand(new BurstCapableFFmpegCapabilities());
|
||||
|
||||
// the positive anchor keeps this from passing vacuously if the helper ever stops
|
||||
// producing a realtime input at all
|
||||
command.ShouldContain("-readrate 1.05");
|
||||
command.ShouldNotContain("-readrate_catchup");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Concat_Should_Never_Catch_Up()
|
||||
{
|
||||
// concat reads already-written segments from the running segmenter at a flat 1.0; it has no
|
||||
// sparse stream to lag on, and letting it catch up would gallop through the segments
|
||||
var concatInputFile = new ConcatInputFile("http://localhost:8080/ffmpeg/concat/1", new FrameSize(1920, 1080));
|
||||
|
||||
var builder = new SoftwarePipelineBuilder(
|
||||
new CatchupCapableFFmpegCapabilities(),
|
||||
HardwareAccelerationMode.None,
|
||||
None,
|
||||
None,
|
||||
None,
|
||||
None,
|
||||
concatInputFile,
|
||||
Option<GraphicsEngineInput>.None,
|
||||
"",
|
||||
"",
|
||||
_logger);
|
||||
|
||||
FFmpegPipeline result = builder.Concat(concatInputFile, FFmpegState.Concat(false, "Some Channel"));
|
||||
|
||||
string command = PrintCommand(None, None, None, concatInputFile, None, result);
|
||||
|
||||
command.ShouldContain("-readrate 1.0");
|
||||
command.ShouldNotContain("-readrate_catchup");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Bitmap_Subtitle_Burn_In_Should_Catch_Up_On_The_Shared_Video_Input()
|
||||
{
|
||||
// THE #726 regression test. an embedded bitmap subtitle is read through the SAME -i as the
|
||||
// video (SubtitleInputFile carries the video's path and resolves to a stream specifier on
|
||||
// that input), and being sparse it drags that input's pacing down to ~0.53x realtime.
|
||||
// this must be built on a BITMAP subtitle: a text subtitle is fetched by the libass filter
|
||||
// outside the demuxer, so the same assertions would pass vacuously while the bug is present.
|
||||
string command = BuildRealtimeCommand(new CatchupCapableFFmpegCapabilities(), imageSubtitle: true);
|
||||
|
||||
// the mechanism itself: subtitle stream 2 resolves onto input 0 -- the VIDEO's input -- so it
|
||||
// is read through the throttled demuxer that catchup is being applied to. if the subtitle
|
||||
// ever moves to an input of its own this label changes and the test fails, which is the point
|
||||
command.ShouldContain("[0:0][0:2]overlay");
|
||||
|
||||
// ...so the catchup has to be on that input
|
||||
command.ShouldContain("-readrate 1.05 -readrate_initial_burst 8 -readrate_catchup 6.0 -i /tmp/whatever.mkv");
|
||||
}
|
||||
|
||||
private string BuildRealtimeCommand(
|
||||
IFFmpegCapabilities capabilities,
|
||||
bool stillImage = false,
|
||||
bool imageSubtitle = false)
|
||||
private string BuildRealtimeCommand(IFFmpegCapabilities capabilities, bool stillImage = false)
|
||||
{
|
||||
var videoInputFile = new VideoInputFile(
|
||||
"/tmp/whatever.mkv",
|
||||
@@ -766,22 +676,13 @@ public class PipelineBuilderBaseTests
|
||||
AudioFilter.None,
|
||||
Option<double>.None));
|
||||
|
||||
// an embedded bitmap subtitle carries the VIDEO's path, which is how it ends up sharing the
|
||||
// video's single throttled -i rather than getting one of its own (ersatztv#726)
|
||||
Option<SubtitleInputFile> subtitleInputFile = imageSubtitle
|
||||
? new SubtitleInputFile(
|
||||
"/tmp/whatever.mkv",
|
||||
new List<MediaStream> { new(2, "dvdsub", StreamKind.Subtitle) },
|
||||
SubtitleMethod.Burn)
|
||||
: Option<SubtitleInputFile>.None;
|
||||
|
||||
var builder = new SoftwarePipelineBuilder(
|
||||
capabilities,
|
||||
HardwareAccelerationMode.None,
|
||||
videoInputFile,
|
||||
audioInputFile,
|
||||
None,
|
||||
subtitleInputFile,
|
||||
None,
|
||||
None,
|
||||
Option<GraphicsEngineInput>.None,
|
||||
"",
|
||||
@@ -834,19 +735,4 @@ public class PipelineBuilderBaseTests
|
||||
new System.Collections.Generic.HashSet<string>(),
|
||||
new System.Collections.Generic.HashSet<string> { FFmpegKnownOption.ReadrateInitialBurst.Name },
|
||||
new System.Collections.Generic.HashSet<string>());
|
||||
|
||||
// a binary new enough for -readrate_catchup also has -readrate_initial_burst, so this models a
|
||||
// real ffmpeg rather than an impossible catchup-without-burst one
|
||||
public class CatchupCapableFFmpegCapabilities() : FFmpegCapabilities(
|
||||
string.Empty,
|
||||
new System.Collections.Generic.HashSet<string>(),
|
||||
new System.Collections.Generic.HashSet<string>(),
|
||||
new System.Collections.Generic.HashSet<string>(),
|
||||
new System.Collections.Generic.HashSet<string>(),
|
||||
new System.Collections.Generic.HashSet<string>
|
||||
{
|
||||
FFmpegKnownOption.ReadrateInitialBurst.Name,
|
||||
FFmpegKnownOption.ReadrateCatchup.Name
|
||||
},
|
||||
new System.Collections.Generic.HashSet<string>());
|
||||
}
|
||||
|
||||
@@ -13,15 +13,8 @@ public record FFmpegKnownOption
|
||||
// ffmpeg 6.1+; lets a readrate-throttled input read flat out for an initial window
|
||||
public static FFmpegKnownOption ReadrateInitialBurst => new("readrate_initial_burst");
|
||||
|
||||
// ffmpeg 8.0+ (added 2025-02-15 in 6232f416b, first released in 8.0); lets a readrate-throttled
|
||||
// input read faster than its readrate *while it is behind*, so a sparse stream sharing that
|
||||
// input cannot pin throughput below realtime (ersatztv#726). verified present in 8.1.2, the
|
||||
// pinned base image — note this is NEWER than 7.1, so it is detected at runtime, never assumed
|
||||
public static FFmpegKnownOption ReadrateCatchup => new("readrate_catchup");
|
||||
|
||||
public static IList<string> AllOptions =>
|
||||
[
|
||||
ReadrateInitialBurst.Name,
|
||||
ReadrateCatchup.Name
|
||||
ReadrateInitialBurst.Name
|
||||
];
|
||||
}
|
||||
|
||||
@@ -3,11 +3,10 @@ using ErsatzTV.FFmpeg.Environment;
|
||||
|
||||
namespace ErsatzTV.FFmpeg.InputOption;
|
||||
|
||||
public class ReadrateInputOption(double readRate, Option<int> initialBurstSeconds, Option<double> catchupReadRate)
|
||||
: IInputOption
|
||||
public class ReadrateInputOption(double readRate, Option<int> initialBurstSeconds) : IInputOption
|
||||
{
|
||||
public ReadrateInputOption(double readRate)
|
||||
: this(readRate, Option<int>.None, Option<double>.None)
|
||||
: this(readRate, Option<int>.None)
|
||||
{
|
||||
}
|
||||
|
||||
@@ -31,17 +30,6 @@ public class ReadrateInputOption(double readRate, Option<int> initialBurstSecond
|
||||
result.Add(burst.ToString(CultureInfo.InvariantCulture));
|
||||
}
|
||||
|
||||
// -readrate paces the WHOLE input off its furthest-behind stream, so one sparse stream
|
||||
// (an embedded PGS/DVD bitmap subtitle feeding the overlay) drags the video down with it
|
||||
// and output collapses to ~0.53x realtime. catchup lets a lagging input read faster until
|
||||
// it is level again; it is a ceiling that only applies WHILE behind, never a target, so
|
||||
// caught-up input still paces at readRate and cannot race ahead (ersatztv#726)
|
||||
foreach (double catchup in catchupReadRate)
|
||||
{
|
||||
result.Add("-readrate_catchup");
|
||||
result.Add(catchup.ToString("0.0####", CultureInfo.InvariantCulture));
|
||||
}
|
||||
|
||||
return result.ToArray();
|
||||
}
|
||||
|
||||
|
||||
@@ -22,14 +22,6 @@ public abstract class PipelineBuilderBase : IPipelineBuilder
|
||||
// an operator who raises that setting above 2 gets less of the benefit (ersatztv#350)
|
||||
private const int InitialBurstSeconds = OutputFormatHls.SegmentSeconds * 2;
|
||||
|
||||
// how fast a LAGGING realtime input may read until it is level again. measured on the #726
|
||||
// repro (embedded dvd_subtitle -> overlay, QSV encode): 1.05 alone sustains 0.53x, catchup 2.0
|
||||
// reaches 0.711x, and 6.0 restores the full 1.067x that the same pipeline achieves with no
|
||||
// subtitle at all. 20.0 also measures 1.067x — i.e. the value is not a throughput dial above
|
||||
// the point where the input catches up, so 6.0 is chosen as the smallest measured-sufficient
|
||||
// ceiling rather than the largest that works (ersatztv#726)
|
||||
private const double CatchupReadRate = 6.0;
|
||||
|
||||
private readonly Option<AudioInputFile> _audioInputFile;
|
||||
private readonly Option<ConcatInputFile> _concatInputFile;
|
||||
private readonly IFFmpegCapabilities _ffmpegCapabilities;
|
||||
@@ -879,26 +871,8 @@ public abstract class PipelineBuilderBase : IPipelineBuilder
|
||||
? InitialBurstSeconds
|
||||
: Option<int>.None;
|
||||
|
||||
// -readrate paces an input off its furthest-behind stream. an embedded bitmap subtitle is
|
||||
// read through the SAME -i as the video (its SubtitleInputFile carries the video's path and
|
||||
// resolves to a stream specifier on that input), and being sparse it falls further behind
|
||||
// every second, dragging video throughput to ~0.53x — well under the 1.0x a live client
|
||||
// consumes at. catchup lets the lagging input recover instead of pinning the whole process.
|
||||
// applied to every realtime input, not just subtitle pipelines: it is inert unless an input
|
||||
// is actually behind, and any sparse stream can cause this (ersatztv#726).
|
||||
//
|
||||
// a still image is excluded for the SAME reason the burst above excludes it: its video input
|
||||
// takes no readrate at all, so this would reach only the separate audio input and let it run
|
||||
// ahead of the video, which is exactly what #350 declined. for a non-still-image item both
|
||||
// inputs carry identical options, so the symmetry is preserved there. and an image-based
|
||||
// subtitle always rides the video path, so this shape cannot suffer the starvation anyway
|
||||
Option<double> catchupReadRate =
|
||||
!isStillImage && _ffmpegCapabilities.HasOption(FFmpegKnownOption.ReadrateCatchup)
|
||||
? CatchupReadRate
|
||||
: Option<double>.None;
|
||||
|
||||
_audioInputFile.Iter(a => a.AddOption(new ReadrateInputOption(readRate, initialBurstSeconds, catchupReadRate)));
|
||||
videoInputFile.AddOption(new ReadrateInputOption(readRate, initialBurstSeconds, catchupReadRate));
|
||||
_audioInputFile.Iter(a => a.AddOption(new ReadrateInputOption(readRate, initialBurstSeconds)));
|
||||
videoInputFile.AddOption(new ReadrateInputOption(readRate, initialBurstSeconds));
|
||||
}
|
||||
|
||||
protected static void SetStillImageLoop(
|
||||
|
||||
+9
-55
@@ -39,8 +39,6 @@ Upstream's final release was **`v26.3.0`** (archived). Our line continues from t
|
||||
| `v26.10.0` | Auto-Tune channel workflow (#69) + weighted content distribution (#70); scheduling refactors, health-check remediation UX (#164), HLS cold-start instrumentation (#350), security hardening (#293/#376/#308). |
|
||||
| `v26.11.0` | **QSV profiles decode via VA-API** — `QsvPreferNativeDecoder`, default **on**, fixes ~50% channel cold-start failures on Intel (#498); unified logo/on-screen bug via a shared watermark preset (#67). Media-scanner resilience: Jellyfin mixed-content libraries (#489), music-video scan correctness (#488/#494/#497), remote-stream probing before ffmpeg (#473/#480); weighted-distribution SPA (#404). **First release deployed to `jazz`** (server-management#633). |
|
||||
| `v26.12.0` | **`ErsatzTV.Mcp` MCP server** — read + cautious-write over `/api/v1`, `ERSATZTV_ALLOW_WRITES`-gated (#58). **External channel-logo URLs download + cache at save time** (#525), with the on-screen bug now rendered for external-URL logos (#502). HLS cold-start hardening: burst-read the first segments so start isn't `-readrate`-bound (#350) and floor QSV extra hardware frames so an unthrottled read can't exhaust the pool (#529); remote graphics-engine image fetches bounded — timeout, size cap, decode cap, redirects, pooling (#511). Decision-lifecycle tooling + parallel-orientation startup rewrite (#520/#521); CI `docker build` lane rebalance (#508). |
|
||||
| `v26.13.0` | **RuleBuilder maturation** — arbitrary-depth group nesting (#436), inline smart-query authoring in Channel Builder (#437), DB-sourced facet typeahead + relative-date operators + validation (#434/#435/#438), and an artist typeahead covering music-video/song credits with `album_artist` no longer 404ing (#578). **Per-channel On Now/Next transient overlay** (#74/#570) and **per-schedule clock-boundary padding** (#392); in-browser channel preview (#60); Auto-Tune per-source weight steppers + exclude/add-untagged (#440). Library-browse pickers now resolve by search instead of a 100-row window, closing several silent at-cap truncations (#644/#650/#651/#634). Correctness: one watermark resolver for all four attachment points, incl. `MiddleCenter` (#503/#510); QSV HDR tonemaps through OpenCL because `vpp_qsv=tonemap` is a silent no-op (#505); `LibraryFolder` unique index + concurrent-insert tolerance (#491); per-library music-video identity with soft trash (#496); Jellyfin Album/Track music-video projection (#177); metadata-collection dedup (#500); accented facet values via a registered Unicode fold on SQLite (#668); `WorkAheadSlots` atomic slot claim, never a negative count (#536/#539); on-demand guide rebuild on thaw (#68). Process/CI: the H10 review-verdict gate became a sha-bound **required** commit status and was hardened through its false-open chain (#622/#629/#632/#648/#649/#672/#698), the decision corpus split to one YAML-frontmatter record per file (#610/#620), and headless Playwright UI-E2E flows landed (#445/#533). Five dual-provider migrations. |
|
||||
| `v26.14.0` | **Live TV no longer starves on embedded bitmap subtitles** — `-readrate` paces an input off its *furthest-behind* stream, and a PGS/DVD subtitle read through the video's own `-i` is sparse enough to drag the whole process to **0.53x realtime** against the 1.0x a client consumes, draining the buffer until the channel stalls. Fixed with a capability-gated `-readrate_catchup` (ffmpeg 8.0+) on realtime inputs, keeping `-readrate` on the frame-producing path so the `ffmpeg.qsv-extra-hw-frames-floor` bound is untouched; measured 0.533x → 1.067x on QSV and software, with a 240s QSV soak clean of allocation errors (#726). Affects items carrying an embedded bitmap subtitle matching the channel's subtitle mode — 3,182 of 24,646 media versions on prod, and a property of the *item*, not the channel, which is why the stall presented as random. Process/CI: the H10 review-verdict gate's repair sentinel became a fixed point and its write is now fenced on the timeline retarget count, closing a raced-sentinel false-open (#706/#707/#711). **The decisions validator now cross-checks its dependency-free frontmatter parse against PyYAML** and reports both the truncating unquoted `` #`` and the scalar-closing bare apostrophe as errors, so a record whose `rule:` silently halves under PyYAML fails the local gate instead of CI (#674/#688) — the ceiling-calibration claim was also split so the suite pins what the derivation MEANS rather than live-corpus order statistics. Dependencies: CliWrap 3.10.4, JetBrains.ReSharper.GlobalTools 2025.3.5. |
|
||||
|
||||
**Before cutting a release — sweep `docs/decisions.md` + `docs/decisions/`** (ersatztv#521, supersedes
|
||||
the ersatztv#303 H9 append-only ritual). Supersession/retirement is now a same-PR act (add the new
|
||||
@@ -48,12 +46,7 @@ active record, relocate the predecessor to `docs/decisions/archive/` with recipr
|
||||
`supersedes`/`superseded-by` links), not a release-boundary batch job — most of the old "consolidate"
|
||||
step is now continuous. The release boundary is instead where you:
|
||||
1. Run `PYTHONPATH=. python3 scripts/decisions_validate.py` — confirms lifecycle metadata is
|
||||
well-formed and every `supersedes`/`superseded-by` link resolves both ways. Since **ersatztv#674**
|
||||
it also cross-checks its dependency-free frontmatter parse against **PyYAML when PyYAML is
|
||||
importable**, failing on any file PyYAML rejects (a bare apostrophe in a single-quoted value) or
|
||||
reads differently (an unquoted ` #`, which YAML truncates as a comment). Where PyYAML is absent —
|
||||
the `decisions-guard` job, the Husky hooks — the cross-check is **skipped with a `::notice::`**
|
||||
and every other check still runs; the read path stays dependency-free.
|
||||
well-formed and every `supersedes`/`superseded-by` link resolves both ways.
|
||||
2. Confirm every record already classified `superseded`/`retired` actually lives under
|
||||
`docs/decisions/archive/` (the validator fails this, but eyeball it at the boundary too).
|
||||
3. Regenerate the active catalog: `PYTHONPATH=. python3 scripts/build_decisions_catalog.py` and
|
||||
@@ -62,24 +55,10 @@ step is now continuous. The release boundary is instead where you:
|
||||
- a **per-record prose ceiling** (`decisions_validate.py --record-ceiling <n>`, default **60**)
|
||||
— a **non-blocking `::warning::`** naming every record over it. This is the actionable signal:
|
||||
it points at a file. The 60 is derived from the distribution, not picked as a round number.
|
||||
Its **calibration is guarded in two pieces of different robustness** (ersatztv#688), because
|
||||
four earlier single-assertion versions all failed — the first two by being vacuous or
|
||||
accepting an absurd ceiling, the last two by ratcheting:
|
||||
- **blocking** (`script-tests`) — only the coarse property that the ceiling flags a
|
||||
**meaningful minority** of records (`0.02 <= fraction_over <= 0.25`). One record moves a
|
||||
fraction by at most 1/N, so no SINGLE ordinary addition can cross it. This is measured
|
||||
headroom, not immunity: from today's 18/183 it takes 38 consecutive over-ceiling additions to
|
||||
breach the cap, 718 short ones to dilute below the floor, or — the tightest arm —
|
||||
consolidating 15 of the 18 offenders away. The floor is
|
||||
a fraction rather than "at least one record", which would accept any ceiling up to 229 on the
|
||||
live corpus; as a fraction the accepted range is 43..180.
|
||||
- **reported, never asserted against the LIVE corpus** — the fine claim that the ceiling sits
|
||||
between the **90th and 95th percentile**, i.e. at the tail boundary. `main()` prints a
|
||||
`::notice::` when it drifts; the tests assert it only on distributions they own.
|
||||
It is an order statistic over a sparse distribution, so a single new record could move p90 by
|
||||
21 lines and red the blocking job for whoever wrote it; a ceiling going out of date is
|
||||
the passage of corpus growth, not a defect in the commit under test, so it is treated like
|
||||
`stale-after`. Re-derive the constant when the notice says so.
|
||||
A test pins what that derivation MEANS rather than any particular numbers: the ceiling must sit
|
||||
between the **90th and 95th percentile** of record lengths, i.e. at the tail boundary. Stated
|
||||
as percentiles it is scale-free, so ordinary corpus growth cannot ratchet it — it fires only
|
||||
when the ceiling genuinely stops marking the tail and should be re-derived.
|
||||
- the **aggregate prose total**, printed every run as an unthresholded `::notice::` **trend**.
|
||||
It has no pass/fail. A total over a monotonically growing corpus can only ratchet: the old
|
||||
4800→5600 budget went quiet at 5228 after #610 changed the metric and was back over at 5658
|
||||
@@ -811,7 +790,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/`, `.codex/`, `.gitea/`, `.husky/`, `scripts/` or `docker/ci/`** — a PR that can weaken the
|
||||
PR touches `.claude/`, `.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.
|
||||
|
||||
@@ -857,34 +836,9 @@ 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 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.
|
||||
`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`.
|
||||
|
||||
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. 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.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.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,10 +58,9 @@ 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, 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.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.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 AND re-writes verbatim on every later run, so the block is a FIXED POINT rather than decaying — writing the generic `pending` description there instead erases the marker and the exemption simply returns one event later. The mark is captured BEFORE the last-moment re-read, not merely before the POST — a later mark leaves a multi-round-trip blind gap in which a verdict is neither seen by the re-read nor repaired afterwards. The id comparison is load-bearing: a mere presence test would fire forever on a base-mismatched verdict that `read_existing_verdict` deliberately declines to honour, deadlocking that PR's exemption permanently. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run's `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run's mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead. | 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) |
|
||||
@@ -72,11 +71,10 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `concurrency.replace-all-contract` | Replace-all aggregate PUTs carry a uniform plain `int Version` concurrency token (EF `.IsConcurrencyToken()`), checked pre-save and enforced by the EF UPDATE guard, returning 412 (not 409) on a stale `If-Match`. | 2026-07-11 | [link](records/concurrency/replace-all-contract.md) |
|
||||
| `concurrency.schedule-item-child-identity` | `PUT /api/schedules/{id}/items` reconciles by an optional round-tripped child `Id` (null/absent/0 ⇒ new item), never by array position, so fill-group/shuffle state follows the logical item across reorders; an unknown or duplicate id is rejected 422 (checked after the §7a `CheckVersion`, so 412 precedes 422). | 2026-07-11 | [link](records/concurrency/schedule-item-child-identity.md) |
|
||||
| `docs.convention-docs-session-start` | Docs-first, not source-first: conventions (api-conventions, spa-conventions, e2e-local, blazor-route-parity, domain-model, decisions, README) are read from docs, not reverse-engineered from code, via `docs/README.md`'s task-signal map — only the sections it points to for the task at hand, not the whole set. Each doc is updated in the same PR that changes what it documents, replacing deferred/follow-up doc updates. | 2026-07-07 | [link](records/docs/convention-docs-session-start.md) |
|
||||
| `docs.corpus-size-signal` | The corpus's size signal is a per-record prose ceiling (`decisions_validate.py --record-ceiling`, default 60, chosen at a natural gap in the distribution), reported as a NON-BLOCKING `::warning::` naming each record over it. The aggregate prose total is still printed every run but carries NO threshold — it is a `::notice::` trend only — because a total over a monotonically growing corpus can only ratchet, and the generated catalog (`docs/decisions/README.md`) is no longer counted at all since it gains one row per record and cannot be consolidated away. Being listed by the ceiling is an invitation to check for REDUNDANCY, never an instruction to cut: a long record that is all distinct findings is a legitimate decline, and should be recorded as one. The ceiling's CALIBRATION is guarded in two pieces of different robustness (#688): the blocking test asserts only the coarse, non-ratcheting property that the ceiling flags a MEANINGFUL MINORITY of records (`0.02 <= fraction_over <= 0.25`), while the fine claim — that it sits between p90 and p95 — is REPORTED by `main()` as a `::notice::` and never asserted against the live corpus. A ceiling drifting out of date is the passage of corpus growth, not a defect in the commit under test, so it gets `stale_records`' treatment rather than a red in the blocking `script-tests` job. | 2026-07-26 | [link](records/docs/corpus-size-signal.md) |
|
||||
| `docs.corpus-size-signal` | The corpus's size signal is a per-record prose ceiling (`decisions_validate.py --record-ceiling`, default 60, chosen at a natural gap in the distribution), reported as a NON-BLOCKING `::warning::` naming each record over it. The aggregate prose total is still printed every run but carries NO threshold — it is a `::notice::` trend only — because a total over a monotonically growing corpus can only ratchet, and the generated catalog (`docs/decisions/README.md`) is no longer counted at all since it gains one row per record and cannot be consolidated away. Being listed by the ceiling is an invitation to check for REDUNDANCY, never an instruction to cut: a long record that is all distinct findings is a legitimate decline, and should be recorded as one. | 2026-07-26 | [link](records/docs/corpus-size-signal.md) |
|
||||
| `docs.decision-lifecycle` | every decision `##` record (active or archived) carries a 5-field metadata block (`key`, `status`, `since`, `supersedes`, `superseded-by`) checked by `scripts/decisions_validate.py`; a record is never deleted or line-edited to reverse a call — it is moved to `docs/decisions/archive/` with `status: superseded`/`retired` and a reciprocal `superseded-by`/`supersedes` key pair to its replacement. | 2026-07-21 | [link](records/docs/decision-lifecycle.md) |
|
||||
| `docs.decision-one-file-per-record` | Each decision record is its own file at `docs/decisions/records/<area>/<topic>.md` (archived ones at `docs/decisions/archive/<area>/<topic>.md`) with YAML frontmatter; the filename IS the key, so one-active-record-per-key is a filesystem property rather than a validator check, and supersession is a `git mv`. | 2026-07-25 | [link](records/docs/decision-one-file-per-record.md) |
|
||||
| `docs.decision-optional-provenance` | Decision records gain two OPTIONAL fields — `stale-after: YYYY-MM-DD` on the metadata line and a `**Sources:**` line in the metadata block; the Open Knowledge Format (OKF) itself is NOT adopted as the record format. | 2026-07-25 | [link](records/docs/decision-optional-provenance.md) |
|
||||
| `docs.frontmatter-pyyaml-crosscheck` | `decisions_validate.py` runs `pyyaml_frontmatter_faults()` over every record-wing file: it loads the frontmatter with PyYAML and reports an ERROR when PyYAML rejects the document OR when any key's value differs from what the dependency-free `dl._read_frontmatter` read. PyYAML is the WRITER of these files (`migrate_decisions_split.render_record` emits them with `yaml.safe_dump`), so on any disagreement PyYAML is authoritative and the defect is in the FILE, not in either parser. The check is strictly additive: when PyYAML is not importable it is SKIPPED and `main()` says so with a `::notice::`, never silently — the read path stays dependency-free because `decisions-guard`, the Husky hooks and contributor machines install nothing. The comparison has exactly ONE implementation, called by both the validator and `test_frontmatter_reader_matches_pyyaml_on_every_real_record`, so the suite and the tool cannot drift on what "matches PyYAML" means. | 2026-08-04 | [link](records/docs/frontmatter-pyyaml-crosscheck.md) |
|
||||
| `docs.record-wing-parse-guard` | `decisions_validate.py` asserts, per PATH, that every `*.md` under `docs/decisions/records/**` and `docs/decisions/archive/**` parses to exactly one record carrying a `key` — an ERROR, not a warning, since a file in the record wings that is not a record is a mistake by definition. A file sitting DIRECTLY in `archive/` is exempt only when it actually looks like a #610 stripped index — exactly one keyless record with a known generated heading — never merely by living there. The one other exemption, `archive/README.md`, is by exact RELATIVE PATH; nothing is ever exempt by BASENAME, since that would exempt the same filename in the active wing too. `_read_frontmatter` is deliberately NOT extended to accept YAML block scalars: every record value goes on ONE line, and the structural check is what makes that limitation loud instead of silent. | 2026-07-26 | [link](records/docs/record-wing-parse-guard.md) |
|
||||
| `docs.tracker-comment-retrofit` | When the knowledge exporter flags an over-cap tracker issue and excludes it from ingestion, triage its comments instead of assuming a retrofit is owed — and for each decision-shaped item check the **worked issue first**, because a tracker session comment is by construction a précis of the fuller closing record posted on the issue it narrates. Applied to #237 (111 comments) this yielded **zero** records, so server-management#642's "a fact found only in a #237 comment" retrieval row has no valid subject and its interim target (an already-migrated record) is permanent. | 2026-07-21 | [link](records/docs/tracker-comment-retrofit.md) |
|
||||
| `ffmpeg.external-logo-graphics-engine` | External-URL channel logos pass through to the graphics engine like any other watermark source; `WatermarkSelector` must never gate them on `File.Exists` (always false for a URL) and never route them through the ffmpeg-native overlay shortcut. | 2026-07-20 | [link](records/ffmpeg/external-logo-graphics-engine.md) |
|
||||
@@ -84,7 +82,6 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `ffmpeg.qsv-decode-encode-split` | QSV decode is decoupled from QSV encode via a single `FFmpegProfile.QsvPreferNativeDecoder` bool (default ON, Linux-only), so a QSV encode profile can decode with the more tolerant native VA-API decoder instead of the QSV decoder, mirroring Jellyfin's hybrid decode/encode toggle instead of a general decode-family enum. | 2026-07-20 | [link](records/ffmpeg/qsv-decode-encode-split.md) |
|
||||
| `ffmpeg.qsv-extra-hw-frames-floor` | a QSV upload never emits `extra_hw_frames` below `FFmpegState.MinimumQsvExtraHardwareFrames` (64); a stored `0` or negative value is treated as "no pool configured" rather than honored literally, because with no headroom any unthrottled read exhausts the pool and the transcode writes nothing at all. | 2026-07-21 | [link](records/ffmpeg/qsv-extra-hw-frames-floor.md) |
|
||||
| `ffmpeg.qsv-hdr-tonemap-opencl` | the QSV pipeline never emits `vpp_qsv=tonemap=1`, which is a SILENT no-op on pre-Gen11 Intel graphics; HDR is tonemapped on the GPU via `hwupload=derive_device=vaapi` → `scale_vaapi` → `hwmap=derive_device=opencl` → `tonemap_opencl` when a VA-API device exists, the frames are still in software, and `tonemap_opencl` is available, and by the software `TonemapFilter` otherwise. The scale runs BEFORE the tonemap, and any hardware filter on the path forces the output to be re-tagged bt709. | 2026-07-26 | [link](records/ffmpeg/qsv-hdr-tonemap-opencl.md) |
|
||||
| `ffmpeg.readrate-catchup-sparse-streams` | a realtime video/audio input also gets `-readrate_catchup` (6.0) when the binary supports it — but NOT a still-image input (mirroring the #350 exclusion) and NOT a concat input, which keep at most bare `-readrate` (a still image's video input takes none at all). Reason: `-readrate` paces the whole input off its furthest-behind stream, so a sparse stream sharing that input (an embedded PGS/DVD bitmap subtitle feeding the overlay) otherwise pins output at ~0.53x realtime. Catchup is a ceiling that applies only WHILE an input is behind, never a target, so it does not let a caught-up input race ahead. | 2026-08-04 | [link](records/ffmpeg/readrate-catchup-sparse-streams.md) |
|
||||
| `ffmpeg.remote-image-fetcher-bounded` | remote graphics-engine images are fetched through `IRemoteImageFetcher` with a pooled `HttpClientFactory` client, a body-covering deadline, a wire-transfer size cap, and a decoder-enforced `DecoderOptions.MaxFrames` bound re-verified post-decode — never cached, re-fetched per element init. | 2026-07-20 | [link](records/ffmpeg/remote-image-fetcher-bounded.md) |
|
||||
| `ffmpeg.watermark-resolution-unified` | Every watermark `WatermarkSelector` resolves goes through one shared `ResolveWatermark` — the playout-item, channel and global precedence levels AND the deco path, for all three `ChannelWatermarkImageSource` values. An unresolvable watermark (missing file, un-migrated external URL, or no logo artwork) resolves to no on-screen bug plus a warning, never a dead path or a URL handed downstream; the one deliberate exception is a playout-item `Custom` with a blank image, which still falls THROUGH to channel/global. The generated-initials fallback is therefore off everywhere, including the deco path where it demonstrably rendered. Watermarks built OUTSIDE the selector (the song-progress overlay, #653) are not covered and remain unchecked. | 2026-07-26 | [link](records/ffmpeg/watermark-resolution-unified.md) |
|
||||
| `ffmpeg.work-ahead-slot-atomic` | `workAheadSegmenterLimit` is enforced by a single compare-exchange claim on a shared `WorkAheadSlots` pool taken by the *caller* of `Transcode`, which then passes ownership in and gets the release in `Transcode`'s `finally` — never a `Volatile.Read` compare in one place and an `Interlocked.Increment` in another. | 2026-07-21 | [link](records/ffmpeg/work-ahead-slot-atomic.md) |
|
||||
@@ -127,7 +124,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/`, `.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) |
|
||||
| `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) |
|
||||
| `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. 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`.'
|
||||
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`.'
|
||||
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,17 +36,6 @@ 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, 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.'
|
||||
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.'
|
||||
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`'
|
||||
---
|
||||
|
||||
@@ -1,72 +0,0 @@
|
||||
---
|
||||
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 AND re-writes verbatim on every later run, so the block is a FIXED POINT rather than decaying — writing the generic `pending` description there instead erases the marker and the exemption simply returns one event later. The mark is captured BEFORE the last-moment re-read, not merely before the POST — a later mark leaves a multi-round-trip blind gap in which a verdict is neither seen by the re-read nor repaired afterwards. The id comparison is load-bearing: a mere presence test would fire forever on a base-mismatched verdict that `read_existing_verdict` deliberately declines to honour, deadlocking that PR''s exemption permanently. Finally, a run whose last-moment re-read finds a sentinel it did not see at its FIRST read ABSTAINS instead of posting: that can only mean an overlapping run repaired a raced verdict mid-flight, and this run''s `success` — frozen at classification time, with the human row below its own mark, so neither the fence nor the post-write check would catch it — would otherwise bury the rejection. That is the one path in this design that failed toward SUCCESS rather than `pending`. The post-write check counts TWO row shapes above the mark, not one — a human `Review-verdict:` row AND a machine sentinel — because with two overlapping runs the human row can sit BELOW the second run''s mark while the first masks it and only then writes the sentinel, leaving the second to post its own `success` on top; counting the sentinel converges both runs on the fixed point instead.'
|
||||
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 a FIXED POINT or it merely decays more slowly.** A repaired status is a machine
|
||||
`pending`, indistinguishable to the next run — which re-derived it and posted `success` again. The
|
||||
description is now a sentinel no exemption is granted over *and* is re-written verbatim by every later
|
||||
run: the first attempt refused the exemption but wrote the GENERIC pending text, erasing its own
|
||||
marker, so the exemption returned two events later instead of one. 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. A human re-posting a BASE-MISMATCHED
|
||||
verdict after a repair does bury the sentinel — that needs a user credential, so it is #697's.
|
||||
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,8 +5,8 @@ status: active
|
||||
since: '2026-07-26'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'The corpus''s size signal is a per-record prose ceiling (`decisions_validate.py --record-ceiling`, default 60, chosen at a natural gap in the distribution), reported as a NON-BLOCKING `::warning::` naming each record over it. The aggregate prose total is still printed every run but carries NO threshold — it is a `::notice::` trend only — because a total over a monotonically growing corpus can only ratchet, and the generated catalog (`docs/decisions/README.md`) is no longer counted at all since it gains one row per record and cannot be consolidated away. Being listed by the ceiling is an invitation to check for REDUNDANCY, never an instruction to cut: a long record that is all distinct findings is a legitimate decline, and should be recorded as one. The ceiling''s CALIBRATION is guarded in two pieces of different robustness (#688): the blocking test asserts only the coarse, non-ratcheting property that the ceiling flags a MEANINGFUL MINORITY of records (`0.02 <= fraction_over <= 0.25`), while the fine claim — that it sits between p90 and p95 — is REPORTED by `main()` as a `::notice::` and never asserted against the live corpus. A ceiling drifting out of date is the passage of corpus growth, not a defect in the commit under test, so it gets `stale_records`'' treatment rather than a red in the blocking `script-tests` job.'
|
||||
signals: 'aggregate active-corpus budget, schedule a consolidation warning, permanently red ratchet, per-record ceiling, 60-line prose ceiling, generated catalog counted in budget, corpus consolidation has no owner, size is not redundancy, ceiling calibration reddens script-tests, adding a record fails CI on length, p90 sits on the ceiling, tail-boundary drift notice · paths: `scripts/decisions_validate.py`, `scripts/tests/test_decisions_validate.py`, `docs/ci-cd.md` · issues: #688, #620, #610, #603, #542, #520'
|
||||
rule: 'The corpus''s size signal is a per-record prose ceiling (`decisions_validate.py --record-ceiling`, default 60, chosen at a natural gap in the distribution), reported as a NON-BLOCKING `::warning::` naming each record over it. The aggregate prose total is still printed every run but carries NO threshold — it is a `::notice::` trend only — because a total over a monotonically growing corpus can only ratchet, and the generated catalog (`docs/decisions/README.md`) is no longer counted at all since it gains one row per record and cannot be consolidated away. Being listed by the ceiling is an invitation to check for REDUNDANCY, never an instruction to cut: a long record that is all distinct findings is a legitimate decline, and should be recorded as one.'
|
||||
signals: 'aggregate active-corpus budget, schedule a consolidation warning, permanently red ratchet, per-record ceiling, 60-line prose ceiling, generated catalog counted in budget, corpus consolidation has no owner, size is not redundancy · paths: `scripts/decisions_validate.py`, `scripts/tests/test_decisions_validate.py`, `docs/ci-cd.md` · issues: #620, #610, #603, #542, #520'
|
||||
mechanics: '`scripts/decisions_validate.py` -> `oversized_records` / `_budget_total`; `docs/ci-cd.md` -> "`decisions-guard` job"'
|
||||
---
|
||||
|
||||
@@ -46,49 +46,19 @@ meant to prevent exactly that could not see it: `max(under) <= 60 < min(over)` i
|
||||
construction** of the two lists it builds, and passes on a distribution with no gap at all. A
|
||||
rationale-guarding test that cannot fail is worse than none, because it launders the claim.
|
||||
|
||||
It is replaced by `test_real_corpus_ceiling_flags_a_nonempty_proper_minority`, which asserts that the
|
||||
ceiling flags a meaningful minority of records (`0.02 <= fraction_over <= 0.25`) and reads the value
|
||||
from `RECORD_CEILING_DEFAULT`, so test and CLI cannot drift.
|
||||
It is replaced by `test_real_corpus_ceiling_sits_at_the_TAIL_BOUNDARY_of_the_distribution`, which
|
||||
states the property directly and scale-free: **the ceiling sits between the 90th and 95th percentile
|
||||
of record lengths** — that is what "marks the start of the tail" means — and reads the value from
|
||||
`RECORD_CEILING_DEFAULT` so test and CLI cannot drift.
|
||||
|
||||
Getting there took five versions, and the failures are the useful part:
|
||||
Getting there took four versions, and the failures are the useful part:
|
||||
|
||||
| | assertion | why it failed |
|
||||
|---|---|---|
|
||||
| v1 | `max(under) <= 60 < min(over)` | true **by construction** of those two lists |
|
||||
| v2 | a minimum gap WIDTH | a ceiling of 200 also sits in a wide gap — it passed |
|
||||
| v3 | 2-12% fraction band + "clear air" above | **hostage to an unrelated record**: one ordinary 62-line addition reddened it with the ceiling correctly placed, and the only remedy was to RAISE the ceiling — this very treadmill, as a hard failure in what #631 makes a blocking job. The fraction band had the same coupling more slowly (12 more long records breached it), and `0 <= headroom` was vacuous. |
|
||||
| v4 | `p90 <= ceiling <= p95` | percentiles move with the corpus, but an order statistic over a SPARSE distribution is a STEP function. The lengths climb to the ceiling and then jump straight to 81 with NOTHING in between (measured; the multiplicities move with every record added, the gap is the point), so ONE record can move p90 by twenty-one lines (that is today's gap; the #672 event moved it less and still reddened CI). It reddened the blocking job twice live (#672, #706), and both times the only in-scope remedy was to trim the new record to fit the constant — the v3 ratchet, pointed at record authors |
|
||||
| v5 | coarse `0.02 <= fraction_over <= 0.25` asserted; fine `p90 <= ceiling <= p95` REPORTED | splits the claim by robustness instead of hunting for a better single assertion (#688) |
|
||||
|
||||
**v5 is not a fifth attempt at the same shape — it stops trying.** Four versions failed because they
|
||||
all asserted, in the blocking job, a property of a corpus the commit under test does not control.
|
||||
The fine claim is genuinely useful and genuinely fragile, so it is now measured on every run and
|
||||
printed as a `::notice::` — the same treatment `stale_records` gets, and for the same stated reason:
|
||||
a constant going out of date is the passage of time, not a defect in this change. What stays
|
||||
blocking is only what no SINGLE ordinary addition can break — each record moves a fraction by at
|
||||
most 1/N, so from **18/183** over the ceiling it takes **38** consecutive over-ceiling additions to
|
||||
BREACH the 25% cap (37 lands exactly on 0.25, which still passes), against **one** record to break v4.
|
||||
|
||||
**The floor is a fraction, not `> 0`, and review is why.** The first draft of v5 asserted only
|
||||
`0 < fraction_over < 1/3`, which measured against the real corpus accepted **every ceiling from 39
|
||||
to 229** — including the ceiling of 200 the draft itself offered as the case it catches, because a
|
||||
single 230-line record keeps the count nonzero. A bound that a deliberately absurd value satisfies
|
||||
is not a guard. At a 2% floor and a 25% cap the accepted range is **43..180** (measured, contiguous):
|
||||
a ceiling of 200 flags 0.5% of records and is rejected, a ceiling of 20 flags 60% and is rejected,
|
||||
and today's 9.8% sits about 5x ABOVE the floor and 38 over-ceiling additions below the cap.
|
||||
|
||||
**Three arms, and the tightest is CONSOLIDATION** — stated because it is the easy one to forget.
|
||||
Breaching the cap takes 38 over-ceiling additions; diluting below the floor takes 718 short ones;
|
||||
but taking **15** of today's 18 over-ceiling records out of the over-set also drops below it — trimming them to <=60 leaves 3/183 = 1.64%, archiving them leaves 3/168 = 1.79%, since archiving moves the denominator too. That
|
||||
is a real tension with `test_oversized_records_can_go_green`, and it is accepted rather than papered
|
||||
over: at 3/183 the constant genuinely IS mis-calibrated, so the red is the signal working. A
|
||||
consolidation PR big enough to reach it should re-derive the ceiling in the same change.
|
||||
|
||||
The honest cost, stated rather than buried: **nothing now forces a re-derivation.** The ceiling can
|
||||
drift while only a notice complains. That is accepted on the same reasoning this record already
|
||||
applies to its two "keep listed" consolidation candidates — the warning names it on every run, which
|
||||
tracks it better than a red that gets trimmed around, and a red an author can only clear by editing
|
||||
an unrelated constant is not enforcement, it is a toll.
|
||||
| v4 | `p90 <= ceiling <= p95` | percentiles move WITH the corpus, so routine growth cannot ratchet it; it fires only when the ceiling genuinely stops marking the tail |
|
||||
|
||||
Two rules came out of that sequence, and they outlive this metric:
|
||||
**a guard test must depend only on the thing it guards**, and
|
||||
@@ -98,7 +68,7 @@ is restated as a self-referential fact** for the same reason: the warning report
|
||||
numbers on every run, and a number frozen in prose is one edit away from being a lie.
|
||||
|
||||
**Size is a proxy for the thing we actually care about, and the proxy is demonstrably wrong.** Of
|
||||
the records over the ceiling, the longest —
|
||||
the records over the ceiling, the largest by ~1.6x —
|
||||
`scan.libraryfolder-unique-identity`, 230 lines — is a dozen-odd **distinct** hard-won traps (MySQL
|
||||
`utf8mb4_bin` PAD SPACE, create-the-composite-index-before-dropping-its-predecessor, clearing the
|
||||
connection pool per MySQL fixture, lazy hash healing that must never abort a scan…). Shortening it
|
||||
|
||||
@@ -1,72 +0,0 @@
|
||||
---
|
||||
key: docs.frontmatter-pyyaml-crosscheck
|
||||
title: '2026-08-04 — `decisions_validate.py` cross-checks its dependency-free frontmatter parse against PyYAML whenever PyYAML is importable (#674)'
|
||||
status: active
|
||||
since: '2026-08-04'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: '`decisions_validate.py` runs `pyyaml_frontmatter_faults()` over every record-wing file: it loads the frontmatter with PyYAML and reports an ERROR when PyYAML rejects the document OR when any key''s value differs from what the dependency-free `dl._read_frontmatter` read. PyYAML is the WRITER of these files (`migrate_decisions_split.render_record` emits them with `yaml.safe_dump`), so on any disagreement PyYAML is authoritative and the defect is in the FILE, not in either parser. The check is strictly additive: when PyYAML is not importable it is SKIPPED and `main()` says so with a `::notice::`, never silently — the read path stays dependency-free because `decisions-guard`, the Husky hooks and contributor machines install nothing. The comparison has exactly ONE implementation, called by both the validator and `test_frontmatter_reader_matches_pyyaml_on_every_real_record`, so the suite and the tool cannot drift on what "matches PyYAML" means.'
|
||||
signals: 'validator reports OK on a broken record, bare apostrophe in single-quoted frontmatter, unquoted hash truncates a value, hand-rolled frontmatter parser, PyYAML rejects the file but decisions-validate passes, dependency-free read path, frontmatter cross-check skipped · paths: `scripts/decisions_validate.py`, `scripts/decisions_lib.py`, `scripts/tests/test_decisions_validate.py`, `scripts/tests/test_decisions_lib.py` · issues: #674, #578, #651, #621'
|
||||
mechanics: '`scripts/decisions_validate.py` -> `pyyaml_frontmatter_faults` / `_frontmatter_block`, wired into `main()` alongside `record_wing_faults`'
|
||||
---
|
||||
|
||||
The validator read ordinary English prose in a `rule:` field and reported **OK** on a file PyYAML
|
||||
refuses to parse. It was hit **twice in one session by two independent agents** on unrelated
|
||||
branches (#578, #651), which is what makes it a guard rather than a note: it is not an exotic edge
|
||||
case, it is what happens when anyone writes `SQLite's LOWER()` into a single-quoted scalar.
|
||||
|
||||
**Why the hand parser exists, and why it stays.** `dl._read_frontmatter` is deliberately
|
||||
dependency-free — it runs in `decisions-guard`, in the Husky hooks, and on every contributor
|
||||
machine, none of which install anything. Requiring PyYAML there once made the validator crash with
|
||||
`ModuleNotFoundError` on the very records the split had just written. So the fix could not be
|
||||
"import yaml in the reader". It is a second, optional opinion layered on top.
|
||||
|
||||
**The two known hazards fail DIFFERENTLY, and that shaped the fix.**
|
||||
|
||||
| input | dependency-free reader | PyYAML |
|
||||
|---|---|---|
|
||||
| `rule: 'SQLite's LOWER()'` | `SQLite's LOWER()` | **`ParserError`** — the bare apostrophe closes the scalar early |
|
||||
| `rule: use --flag #2` | `use --flag #2` | `use --flag` — ` #` starts a comment, **silently truncating** |
|
||||
|
||||
A `try/except` would have caught only the first row. The second produces no exception at all: a
|
||||
valid record whose `rule` has quietly lost its tail — the `parse-to-WRONG` case
|
||||
`docs.record-wing-parse-guard` explicitly names as the gap its structural check cannot see. So the
|
||||
cross-check compares the parsed **result** key by key, and reports a rejection and a mismatch as two
|
||||
distinct faults with different remedies. The `except` around the load is deliberately broad, not
|
||||
`yaml.YAMLError`: PyYAML's timestamp constructor raises a bare `ValueError` on an impossible date
|
||||
(`stale-after: 2026-06-31`), and an additive check must never be the reason the validator can't run.
|
||||
|
||||
**That is also what makes it general.** #674 asked for a fix that catches the *next* character class
|
||||
rather than enumerating hazards one at a time. Comparing against the writer's own library is that:
|
||||
any construct where the two parsers disagree surfaces as a diff, with nobody having to predict it.
|
||||
|
||||
**Direction is the load-bearing part.** PyYAML is not a second opinion of equal standing — it WROTE
|
||||
these files, so when the two disagree the on-disk bytes mean what PyYAML says, the record is corrupt
|
||||
and the permissive reader is the one hiding it. That is what turns an ambiguous "parsers differ"
|
||||
report into an actionable "this record is silently wrong".
|
||||
|
||||
**A skip is announced, not silent.** When PyYAML is absent the check does not run, which is correct
|
||||
on the dependency-free path — but `main()` prints a `::notice::` saying so. A check that reports
|
||||
success while doing nothing is the defect this corpus keeps re-learning (#603's `stale-after` that
|
||||
never fired, #609's marker that exempted everything while printing OK), and adding a quiet skip
|
||||
while fixing a quiet pass would have reintroduced it one level up.
|
||||
|
||||
**One implementation, two callers.** The comparison already existed — in the test suite only, which
|
||||
is exactly why the validator could disagree with `scripts/tests` about the same file. Rather than
|
||||
leaving a second copy, `test_frontmatter_reader_matches_pyyaml_on_every_real_record` now delegates
|
||||
to `pyyaml_frontmatter_faults`, so the tool and the suite cannot drift on the definition.
|
||||
|
||||
**What this buys, stated precisely, because it is less than it looks.** In CI `decisions-guard`
|
||||
installs nothing, so the validator there always takes the skip path, by design; and `script-tests`
|
||||
already went red on both hazards before this change, and an advisory red still blocks the merge gate
|
||||
(#598). So **no broken record has reached `main` and the CI delta here is close to zero** — though
|
||||
procedurally, not structurally: branch protection on `main` requires exactly three contexts (`Build
|
||||
& test (.NET)`, `EF migration integrity`, `review-verdict/h10`), and NEITHER `script-tests` nor
|
||||
`decisions-guard` is among them. What this fixes is the case #674 described: the LOCAL loop, where
|
||||
the validator is the tool an agent reaches for directly and it printed OK on a corrupt file — plus
|
||||
the tool/suite disagreement, now impossible.
|
||||
|
||||
**Limits, and the positive control.** It does not catch a mis-parse both parsers agree on — strong,
|
||||
not total, the same qualification `record_wing_faults` carries. The suite pins that
|
||||
`record_wing_faults` ALONE still reports both hazard files as clean; without that, the cross-check
|
||||
could be deleted and the tests would stay green while the guard vanished.
|
||||
@@ -58,8 +58,7 @@ mechanics: '`SetRealtimeInput` readrate-burst option; `FFmpegKnownOption.HasOpti
|
||||
session" flag through `FFmpegState`; that complexity was not judged worth a bounded peak.
|
||||
- **Still images are excluded.** Their video input is paced by the realtime *filter* and takes no
|
||||
readrate at all, so a burst would only run the audio input ahead of the video for songs and offline
|
||||
filler, with no cold-start gain to show for it. (`-readrate_catchup` mirrors this exclusion for the
|
||||
same reason — `ffmpeg.readrate-catchup-sparse-streams`, #726.)
|
||||
filler, with no cold-start gain to show for it.
|
||||
- **Non-HLS realtime outputs (`TransportStream`, HLS-Direct) burst too**, since
|
||||
`FFmpegPlaybackSettingsCalculator` makes them unconditionally realtime. That is untested by the
|
||||
benchmark, which was segmenter-only; it is kept because the same first-read throttle delays those
|
||||
|
||||
@@ -1,71 +0,0 @@
|
||||
---
|
||||
key: ffmpeg.readrate-catchup-sparse-streams
|
||||
title: 2026-08-04 — a realtime input gets `-readrate_catchup`, because `-readrate` paces off its furthest-behind stream (#726)
|
||||
status: active
|
||||
since: '2026-08-04'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'a realtime video/audio input also gets `-readrate_catchup` (6.0) when the binary supports it — but NOT a still-image input (mirroring the #350 exclusion) and NOT a concat input, which keep at most bare `-readrate` (a still image''s video input takes none at all). Reason: `-readrate` paces the whole input off its furthest-behind stream, so a sparse stream sharing that input (an embedded PGS/DVD bitmap subtitle feeding the overlay) otherwise pins output at ~0.53x realtime. Catchup is a ceiling that applies only WHILE an input is behind, never a target, so it does not let a caught-up input race ahead.'
|
||||
signals: 'readrate, readrate_catchup, sparse stream, bitmap subtitle, PGS, DVD subtitle, dvdsub, pgssub, overlay burn-in, Live TV buffering/stalling, "Resumed reading at pts N with rate R after a lag of Ns" · paths: `PipelineBuilderBase.SetRealtimeInput`, `ReadrateInputOption`, `FFmpegKnownOption` · issues: #726, #350, #529'
|
||||
mechanics: '`PipelineBuilderBase.CatchupReadRate` (6.0); `ReadrateInputOption` catchup arg; `FFmpegKnownOption.ReadrateCatchup` capability gate'
|
||||
---
|
||||
|
||||
- **`-readrate` throttles an input, not a stream, and it paces off whichever stream is furthest
|
||||
behind.** An embedded bitmap subtitle is read through the *same* `-i` as the video —
|
||||
`SubtitleInputFile` carries the video's path and `ComplexFilter` resolves it to a stream specifier
|
||||
on that input, and `CommandGenerator` never emits a second `-i` for it. Being sparse, the subtitle
|
||||
stream falls further behind every second and drags the video down with it. FFmpeg says so itself at
|
||||
`-loglevel warning`: `[sist#0:3/dvd_subtitle] Resumed reading at pts 10.400 with rate 6.000 after a
|
||||
lag of 0.922s`, repeating with the lag growing 0.9→3.8 s while `pts` stays pinned (no new packet).
|
||||
- **Measured on prod (QSV, `-threads 1`, `dvd_subtitle`→overlay), 45 s steady-state window after a
|
||||
6 s settle:**
|
||||
|
||||
| variant | throughput |
|
||||
|---|---|
|
||||
| `-readrate 1.05` (baseline) | **0.533x** (×3 runs) |
|
||||
| `+ -readrate_catchup 2.0` | 0.711x |
|
||||
| `+ -readrate_catchup 6.0` | **1.067x** (×2 runs) |
|
||||
| `+ -readrate_catchup 20.0` | 1.067x |
|
||||
| no subtitle overlay (control) | 1.067x |
|
||||
|
||||
A live client consumes at 1.0x, so 0.53x drains its buffer until it stalls — the reported symptom.
|
||||
- **`20.0` measuring the same as `6.0` is why 6.0 was chosen** — above the catch-up point the value
|
||||
is not a throughput dial, so there is nothing to buy by going higher. It is **not** evidence about
|
||||
allocation: that is a steady-state throughput number, not a count of frames in flight.
|
||||
- **Why this does not reopen `ffmpeg.qsv-extra-hw-frames-floor` (#529).** Not because catchup is
|
||||
brief (a permanently GPU-bound channel lags forever, so 6x is a standing licence), and **not**
|
||||
because read rate is allocation-irrelevant — #529 measured that it is not (at `extra_hw_frames=0`,
|
||||
`1.05` without a burst exits 0 while `1.05`+burst hits ENOMEM). Read rate changes how fast frames
|
||||
enter the graph, not how deep its queues are, and #529 showed that only bites when the pool has
|
||||
**no headroom**. The 64-frame floor now guarantees headroom, so the load-bearing measurement is
|
||||
row 5 of that truth table — **no `-readrate` at all with 64 frames → 14 segments, exit 0** — and a
|
||||
6x ceiling is strictly less aggressive than no throttle. Reinforcing it, `-readrate_initial_burst 8`
|
||||
has read *flat out* at the start of every playout item since #350, so an unbounded read here is not
|
||||
new. A 240 s QSV soak (64 frames, 60 segment boundaries) adds 1.043x sustained with **zero**
|
||||
`Cannot allocate memory` — but it stayed largely caught-up, so it corroborates rather than proves;
|
||||
the argument above is what carries the decision.
|
||||
- **Not QSV-specific:** reproduces on libx264 too (0.533x → 1.067x), as expected for an input-pacing
|
||||
option upstream of any encoder or filter choice.
|
||||
- **Raising the base `-readrate` is not an alternative, and was measured:** 2.0→0.62x, 3.0→0.80x,
|
||||
4.0→0.80x, 6.0→0.89x. It asymptotes *below* realtime, because the rate ceiling was never the
|
||||
binding constraint. Recorded so it is not re-proposed.
|
||||
- **Catchup does NOT subsume the #350 burst; they fix orthogonal metrics.** Measured
|
||||
time-to-first-segment: `-readrate` alone 3.71 s, `+burst` **0.72 s**, `+catchup` alone **3.65 s**,
|
||||
both 0.67 s. Catchup buys nothing at cold start (no accumulated lag at t=0 to recover) and the
|
||||
burst buys nothing for throughput (the 0.533x baseline already had it), so removing the burst on
|
||||
the theory that catchup replaces it would regress tune-in ~5x.
|
||||
- **Applied to realtime video/audio inputs generally, not only subtitle pipelines** — it is inert
|
||||
unless an input is behind, and any sparse stream can trigger this, so gating it on "has a bitmap
|
||||
subtitle" would fix the site instead of the boundary. Two deliberate exclusions, both test-pinned:
|
||||
`ConcatInputFile` (reads already-written segments at a flat 1.0, nothing sparse to lag on) and
|
||||
**still images**, mirroring #350 — their video input takes no readrate at all, so catchup would
|
||||
reach only the separate audio input and break the pacing symmetry #350 declined to break. An
|
||||
image-based subtitle always rides the *video* path, so that shape cannot starve this way anyway.
|
||||
- **Capability-gated via `FFmpegKnownOption.HasOption`**, the same fail-safe posture as
|
||||
`-readrate_initial_burst`: detection parses `ffmpeg -h long`, so a binary without the option
|
||||
silently keeps today's behavior instead of failing to start.
|
||||
|
||||
**Accepted residual:** the affected population is items carrying an embedded bitmap subtitle matching
|
||||
the channel's subtitle mode — 3,182 of 24,646 media versions (12.9%) on prod. It is a property of the
|
||||
*item*, not the channel, which is why the stall presented as random: a channel plays one episode fine
|
||||
and stalls on the next.
|
||||
@@ -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/`, `.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).'
|
||||
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).'
|
||||
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`'
|
||||
---
|
||||
|
||||
@@ -17,7 +17,6 @@ import subprocess
|
||||
import sys
|
||||
from datetime import date
|
||||
from pathlib import Path
|
||||
from typing import NamedTuple
|
||||
|
||||
import scripts.decisions_lib as dl # noqa: E402 (run with PYTHONPATH=. or as module)
|
||||
|
||||
@@ -53,18 +52,6 @@ _STALE_AFTER_RE = re.compile(r"^\d{4}-\d{2}-\d{2}$")
|
||||
# guards the calibration claim asserts against the SAME value the CLI uses and the two cannot drift.
|
||||
RECORD_CEILING_DEFAULT = 60
|
||||
|
||||
# The coarse, non-ratcheting calibration bound (#688): the ceiling must flag a MEANINGFUL MINORITY
|
||||
# of records. Below the floor it is parked among the outliers and names almost nobody; above the cap
|
||||
# it is cutting into the bulk rather than marking a tail. See `ceiling_calibration` for why the fine
|
||||
# percentile claim is reported instead of asserted.
|
||||
#
|
||||
# The floor is NOT "at least one record" — that was the first draft and it was nearly unfalsifiable:
|
||||
# measured on the live corpus it accepted every ceiling from 39 to 229, including the ceiling of 200
|
||||
# this module's own docstring offered as the case it catches (one 230-line record keeps the count
|
||||
# nonzero). A 2% floor rejects 200/229/230 and still leaves ~5x headroom below today's 9.8%.
|
||||
CEILING_MINORITY_MIN = 0.02
|
||||
CEILING_MINORITY_MAX = 0.25
|
||||
|
||||
|
||||
def _parse_stale_after(value: str | None) -> date | None:
|
||||
"""`stale-after` as a date, or None if absent, empty, or malformed.
|
||||
@@ -554,102 +541,6 @@ def record_wing_faults(records_dir: Path | None = None, archive_dir: Path | None
|
||||
return faults
|
||||
|
||||
|
||||
def _frontmatter_block(text: str) -> str | None:
|
||||
"""The raw YAML between the opening `---` and the next `---`, or None if there isn't one."""
|
||||
if not dl.has_frontmatter(text):
|
||||
return None
|
||||
lines = text.splitlines()
|
||||
end = next((i for i, ln in enumerate(lines[1:], start=1) if ln.rstrip() == "---"), None)
|
||||
if end is None:
|
||||
return None
|
||||
return "\n".join(lines[1:end])
|
||||
|
||||
|
||||
def pyyaml_frontmatter_faults(files) -> tuple[list[str], bool]:
|
||||
"""Faults where PyYAML disagrees with the dependency-free reader. Returns (faults, ran).
|
||||
|
||||
#674: the two known hazards are a bare apostrophe inside a single-quoted scalar
|
||||
(`rule: 'SQLite's LOWER()'`) and an unquoted ` #` (`rule: use --flag #2`). Before this check the
|
||||
validator reported OK on both, because `dl._read_frontmatter` is a hand parser that cannot see
|
||||
either. It was hit TWICE in one session by two independent agents, which is what makes it worth
|
||||
a guard rather than a note.
|
||||
|
||||
The two hazards fail DIFFERENTLY, and catching only the first would have missed half of it:
|
||||
|
||||
* the apostrophe makes PyYAML **reject** the document outright (`ParserError`);
|
||||
* the unquoted ` #` parses fine and **silently truncates** the value — PyYAML reads
|
||||
`use --flag`, the hand parser reads `use --flag #2`. No exception, a wrong value.
|
||||
|
||||
So this compares the parsed RESULT and does not merely try/except the load. That is also why it
|
||||
generalizes past the two known characters, which is the property #674 asked for: any future
|
||||
construct where the writer's library and our reader disagree shows up as a diff, without anyone
|
||||
enumerating it first.
|
||||
|
||||
DIRECTION MATTERS: PyYAML is the WRITER (`migrate_decisions_split.render_record` emits these
|
||||
files with `yaml.safe_dump`), so it is the authority on what the on-disk bytes mean. The hand
|
||||
reader is the permissive one, and a disagreement is a defect in the FILE, not in either parser.
|
||||
|
||||
`ran` is False when PyYAML is not importable. The read path is deliberately dependency-free —
|
||||
`decisions-guard`, the Husky hooks and every contributor machine install nothing — so this check
|
||||
is strictly additive: it must never be the reason the validator cannot run. main() announces the
|
||||
skip rather than passing quietly, because a check that reports success while doing nothing is
|
||||
the exact defect class this corpus keeps re-learning (#603's `stale-after`, #609's marker).
|
||||
"""
|
||||
try:
|
||||
import yaml # pyright: ignore[reportMissingImports]
|
||||
except Exception:
|
||||
return [], False
|
||||
|
||||
faults: list[str] = []
|
||||
for p in files:
|
||||
try:
|
||||
text = p.read_text(encoding="utf-8")
|
||||
except Exception: # noqa: S112 (record_wing_faults already reports the unreadable file by name)
|
||||
continue
|
||||
block = _frontmatter_block(text)
|
||||
if block is None:
|
||||
continue # no/unterminated frontmatter: reported by record_wing_faults
|
||||
try:
|
||||
theirs_raw = yaml.safe_load(block)
|
||||
except Exception as exc:
|
||||
# DELIBERATELY broad. `yaml.YAMLError` alone is too narrow: PyYAML's timestamp
|
||||
# constructor raises a BARE ValueError for a well-shaped but impossible date
|
||||
# (`stale-after: 2026-06-31` -> "day is out of range for month"), which would escape as
|
||||
# a traceback. This check is meant to be strictly additive — it must never be the
|
||||
# reason the validator cannot run, so every failure to load becomes a reported fault.
|
||||
first = str(exc).splitlines()[0] if str(exc).strip() else exc.__class__.__name__
|
||||
faults.append(
|
||||
f"{p}: PyYAML REJECTS this frontmatter, though the dependency-free reader accepted "
|
||||
f"it ({exc.__class__.__name__}: {first}). PyYAML is what WROTE these files, so its "
|
||||
f"verdict is authoritative. Quote the offending value and, inside single quotes, "
|
||||
f"double any literal apostrophe — as `yaml.safe_dump` does. Usual causes: a bare "
|
||||
f"apostrophe inside a single-quoted value, an unquoted `: ` or leading backtick, or "
|
||||
f"an impossible date."
|
||||
)
|
||||
continue
|
||||
if theirs_raw is None:
|
||||
theirs_raw = {}
|
||||
if not isinstance(theirs_raw, dict):
|
||||
faults.append(f"{p}: frontmatter parses as {type(theirs_raw).__name__}, not a mapping.")
|
||||
continue
|
||||
mine = dl._read_frontmatter(block)
|
||||
if mine is None:
|
||||
continue # the reader bailed: reported by record_wing_faults as parse-to-0
|
||||
theirs = {k: ("" if v is None else str(v)) for k, v in theirs_raw.items()}
|
||||
# `key=str`: PyYAML returns TYPED mapping keys, so a stray `1: x` yields an int key while the
|
||||
# hand reader yields "1", and sorting that mixed set raises TypeError — an uncaught traceback
|
||||
# replacing what `_unknown_frontmatter_keys` used to report as an actionable error.
|
||||
for k in sorted(set(mine) | set(theirs), key=str):
|
||||
if mine.get(k) != theirs.get(k):
|
||||
faults.append(
|
||||
f"{p}: frontmatter key {k!r} means different things to the two parsers — "
|
||||
f"reader={mine.get(k)!r} but PyYAML={theirs.get(k)!r}. PyYAML wrote this file, "
|
||||
f"so its reading is the real value and the record is silently corrupt. Common "
|
||||
f"cause: an unquoted ` #`, which YAML treats as a comment and truncates there."
|
||||
)
|
||||
return faults, True
|
||||
|
||||
|
||||
def _unknown_frontmatter_keys(path: Path) -> set[str]:
|
||||
"""Frontmatter keys outside the known schema. Empty on any read/parse failure (reported elsewhere)."""
|
||||
try:
|
||||
@@ -738,7 +629,7 @@ def oversized_records(records, ceiling: int) -> list[tuple[str, int]]:
|
||||
act on instead of asserting that "the corpus" is too big.
|
||||
|
||||
The ceiling sits at a natural gap in the real distribution rather than a round number: at #620
|
||||
the records ran 2..59 prose lines (median 26, p90 52) and then jumped straight to 83, with
|
||||
the records run 0..59 prose lines (median 26, p90 52) and then jump straight to 83, with
|
||||
nothing in between. 60 separates the bulk from the tail without splitting a cluster.
|
||||
|
||||
IMPORTANT — a prompt for judgement, not a target. Length is a PROXY for "grown past what a
|
||||
@@ -751,79 +642,6 @@ def oversized_records(records, ceiling: int) -> list[tuple[str, int]]:
|
||||
return sorted([kv for kv in out if kv[1] > ceiling], key=lambda kv: -kv[1])
|
||||
|
||||
|
||||
class CeilingCalibration(NamedTuple):
|
||||
n: int
|
||||
p90: int
|
||||
p95: int
|
||||
n_over: int
|
||||
fraction_over: float
|
||||
marks_tail: bool # the FINE claim: p90 <= ceiling <= p95
|
||||
flags_minority: bool # the COARSE claim: MINORITY_MIN <= fraction_over <= MINORITY_MAX
|
||||
|
||||
|
||||
def ceiling_calibration(records, ceiling: int) -> CeilingCalibration:
|
||||
"""How well `ceiling` still marks the start of the corpus's tail (#688).
|
||||
|
||||
Two claims of DIFFERENT robustness, deliberately separated, because conflating them is what
|
||||
made the previous guard a ratchet:
|
||||
|
||||
`marks_tail` — `p90 <= ceiling <= p95`. Correct as a definition of "start of the tail", but an
|
||||
order statistic over a SPARSE distribution is a STEP function: the lengths climb to the ceiling
|
||||
and then jump straight to 81 with nothing in between AS MEASURED TODAY (the gap's width moves
|
||||
with the corpus — this is the shape, not a constant), so ONE new record could move p90 by 21
|
||||
lines, and the only remedy the assertion admitted was to raise the ceiling. It is real signal,
|
||||
but it is signal about the CONSTANT drifting, not a defect in the commit under test — the same
|
||||
shape as `stale_records`, and it is reported the same way: a notice, never a failure.
|
||||
|
||||
`flags_minority` — `CEILING_MINORITY_MIN <= fraction_over <= CEILING_MINORITY_MAX`. Deliberately
|
||||
coarse, and what the blocking test asserts. Each added record moves a fraction by at most 1/N, so
|
||||
NO SINGLE ordinary addition can cross it — this is measured headroom, not immunity. From a live
|
||||
18/183 (9.8%), BREACHING the 25% cap takes 38
|
||||
consecutive over-ceiling additions (37 lands exactly on 0.25, which still passes under `<=`),
|
||||
or 718 short ones to dilute below the floor — against ONE record to break `marks_tail`.
|
||||
|
||||
The THIRD arm is the tightest and is stated here because it is the easy one to forget:
|
||||
CONSOLIDATION. Taking 15 of today's 18 over-ceiling records out of the over-set drops below the
|
||||
2% floor — trimming them to <=60 leaves 3/183 = 1.64%, archiving them outright leaves 3/168 =
|
||||
1.79% (the denominator moves too); either way, under the floor. That is a real tension with
|
||||
`test_oversized_records_can_go_green` — the ceiling is allowed to go green — and it is accepted
|
||||
rather than papered over: at 3/183 the
|
||||
constant genuinely IS mis-calibrated and saying so is the signal working. A consolidation PR
|
||||
large enough to hit it should re-derive the ceiling in the same change.
|
||||
|
||||
It still catches genuine mis-calibration in both directions, measured on the real corpus: a
|
||||
ceiling of 20 flags 60% of records (cutting into the bulk, so every author learns to ignore it),
|
||||
and a ceiling of 200 flags 0.5% — one record — which is below the floor and rejected. Note that
|
||||
"flags NOBODY" is the wrong way to state the upper failure: at a ceiling of 200 the count is
|
||||
still nonzero because one 230-line record exists, which is exactly why the floor is a fraction
|
||||
and not `> 0`.
|
||||
|
||||
Why not simply re-derive the constant instead: re-deriving fixes the instance and keeps the
|
||||
mechanism. The v3 fraction band and the v4 percentile containment both failed the same way, one
|
||||
faster than the other, and picking a new number would queue up the fifth version.
|
||||
"""
|
||||
lengths = sorted(record_prose_lines(r) for r in records if r.key)
|
||||
n = len(lengths)
|
||||
if n == 0:
|
||||
return CeilingCalibration(0, 0, 0, 0, 0.0, False, False)
|
||||
|
||||
def pct(q: float) -> int:
|
||||
return lengths[min(int(n * q), n - 1)]
|
||||
|
||||
n_over = sum(1 for v in lengths if v > ceiling)
|
||||
frac = n_over / n
|
||||
p90, p95 = pct(0.90), pct(0.95)
|
||||
return CeilingCalibration(
|
||||
n=n,
|
||||
p90=p90,
|
||||
p95=p95,
|
||||
n_over=n_over,
|
||||
fraction_over=frac,
|
||||
marks_tail=p90 <= ceiling <= p95,
|
||||
flags_minority=CEILING_MINORITY_MIN <= frac <= CEILING_MINORITY_MAX,
|
||||
)
|
||||
|
||||
|
||||
def _catalog_ok() -> bool:
|
||||
try:
|
||||
import scripts.build_decisions_catalog as bc # pyright: ignore[reportMissingImports]
|
||||
@@ -866,19 +684,6 @@ def main(argv=None) -> int:
|
||||
archive_records += dl.parse_file(f)
|
||||
removed, rewritten, demoted = _diff_findings(args.base, args.head) if args.base and args.head else ([], [], [])
|
||||
oversized = oversized_records(records, args.record_ceiling)
|
||||
|
||||
# #674: cross-check the dependency-free reader against the library that WROTE these files.
|
||||
# Strictly additive — absent PyYAML skips the check (and SAYS so) rather than failing the run.
|
||||
yaml_faults, yaml_ran = pyyaml_frontmatter_faults(record_wing_files())
|
||||
if not yaml_ran:
|
||||
print(
|
||||
"::notice::decisions-validate: PyYAML is not importable, so the frontmatter cross-check "
|
||||
"was SKIPPED — every other check ran. This is the expected state on the dependency-free "
|
||||
"read path (decisions-guard, the Husky hooks); CI's script-tests job runs it with PyYAML "
|
||||
"present.",
|
||||
file=sys.stderr,
|
||||
)
|
||||
|
||||
errs = validate(
|
||||
records,
|
||||
archive_keys=_archive_keys(),
|
||||
@@ -887,7 +692,7 @@ def main(argv=None) -> int:
|
||||
rewritten=rewritten,
|
||||
archive_records=archive_records,
|
||||
demoted=demoted,
|
||||
wing_faults=record_wing_faults() + yaml_faults,
|
||||
wing_faults=record_wing_faults(),
|
||||
)
|
||||
|
||||
# Aggregate: an unthresholded TREND, not a gate (#620). Printed every run so the number stays
|
||||
@@ -918,20 +723,6 @@ def main(argv=None) -> int:
|
||||
file=sys.stderr,
|
||||
)
|
||||
|
||||
# Ceiling calibration drift (#688): a NOTICE, never a failure. The ceiling drifting away from
|
||||
# the tail boundary is the passage of corpus growth, not a defect in the commit under test — the
|
||||
# same reasoning `stale_records` is built on. Asserting it in the blocking `script-tests` job
|
||||
# made the next author of a substantial record pay for an unrelated constant going out of date.
|
||||
cal = ceiling_calibration(records, args.record_ceiling)
|
||||
if cal.n and not cal.marks_tail:
|
||||
print(
|
||||
f"::notice::decisions-validate: the {args.record_ceiling}-line ceiling has drifted from "
|
||||
f"the tail boundary of the distribution (p90={cal.p90}, p95={cal.p95}, "
|
||||
f"{cal.n_over}/{cal.n} records over it). Re-derive it when convenient — this is a "
|
||||
f"maintenance signal about the constant, not a problem with this change.",
|
||||
file=sys.stderr,
|
||||
)
|
||||
|
||||
stale = stale_records(records, date.today())
|
||||
if stale:
|
||||
listed = "; ".join(f"{h} (stale-after {d})" for h, d in stale)
|
||||
|
||||
@@ -121,27 +121,6 @@ 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
|
||||
@@ -261,18 +240,5 @@ 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
|
||||
|
||||
@@ -71,20 +71,23 @@ def test_frontmatter_reader_matches_pyyaml_on_every_real_record():
|
||||
made the validator crash with ModuleNotFoundError once the corpus was migrated.) A hand parser
|
||||
is only safe if it provably matches the library that WROTE the files, so this compares the two
|
||||
across every record rather than on a sample.
|
||||
|
||||
Since #674 the comparison itself lives in `decisions_validate.pyyaml_frontmatter_faults`, which
|
||||
the VALIDATOR now runs too — before that it existed only here, so `decisions_validate.py`
|
||||
happily reported OK on a record PyYAML rejects. This test delegates to that one implementation
|
||||
rather than keeping a second copy of the comparison, so the suite and the validator cannot
|
||||
drift apart and agree on what "matches PyYAML" means.
|
||||
"""
|
||||
pytest.importorskip("yaml")
|
||||
import scripts.decisions_validate as dv
|
||||
yaml = pytest.importorskip("yaml")
|
||||
|
||||
files = [p for p in dl.RECORDS_DIR.rglob("*.md")] + [p for p in dl.ARCHIVE_DIR.rglob("*.md")]
|
||||
files = [f for f in files if dl.has_frontmatter(f.read_text(encoding="utf-8"))]
|
||||
assert len(files) > 100, f"only {len(files)} frontmatter files found — test would be near-vacuous"
|
||||
|
||||
faults, ran = dv.pyyaml_frontmatter_faults(files)
|
||||
assert ran, "PyYAML is importable here, so the comparison must have actually run"
|
||||
assert not faults, f"{len(faults)} frontmatter fault(s) vs PyYAML:\n" + "\n".join(faults[:5])
|
||||
diffs = []
|
||||
for f in files:
|
||||
lines = f.read_text(encoding="utf-8").splitlines()
|
||||
end = next(i for i, ln in enumerate(lines[1:], start=1) if ln.rstrip() == "---")
|
||||
block = "\n".join(lines[1:end])
|
||||
mine = dl._read_frontmatter(block)
|
||||
theirs = yaml.safe_load(block) or {}
|
||||
theirs = {k: ("" if v is None else str(v)) for k, v in theirs.items()}
|
||||
if mine != theirs:
|
||||
for k in set(mine or {}) | set(theirs):
|
||||
if (mine or {}).get(k) != theirs.get(k):
|
||||
diffs.append(f"{f.name}:{k}\n mine ={(mine or {}).get(k)!r}\n pyyaml={theirs.get(k)!r}")
|
||||
assert not diffs, f"{len(diffs)} field(s) differ from PyYAML:\n" + "\n".join(diffs[:5])
|
||||
|
||||
@@ -1002,423 +1002,39 @@ def test_budget_total_excludes_the_generated_catalog(tmp_path, monkeypatch):
|
||||
assert total < 100, f"the 500-line generated catalog leaked into the total ({total})"
|
||||
|
||||
|
||||
def test_real_corpus_ceiling_flags_a_nonempty_proper_minority():
|
||||
"""Guards the calibration claim. This is the FIFTH version; the failures are the lesson.
|
||||
def test_real_corpus_ceiling_sits_at_the_TAIL_BOUNDARY_of_the_distribution():
|
||||
"""Guards the calibration claim. This is the FOURTH version; the failures are the lesson.
|
||||
|
||||
v1 `max(under) <= 60 < min(over)` — true by construction of those two lists.
|
||||
v2 a minimum gap WIDTH — but a ceiling of 200 also sits in a wide gap, so it passed.
|
||||
v3 a 2-12% fraction band plus "clear air" measured against `min(over)` — the nearest
|
||||
record ABOVE the ceiling. That made the test a hostage to an unrelated record: one
|
||||
ordinary 62-line addition reddened it with the ceiling correctly placed, and the only
|
||||
remedy the assertion admitted was to RAISE the ceiling.
|
||||
v4 `p90 <= ceiling <= p95`. Scale-free and correct AS A DEFINITION, but an order statistic
|
||||
over a SPARSE distribution is a STEP function. The lengths climb to the ceiling and then
|
||||
jump STRAIGHT to 81 with nothing between, so ONE new record can move p90 by 21 lines and
|
||||
reddened the BLOCKING `script-tests` job for whoever happened to write it. It reproduced
|
||||
twice live (#672, #706) and both times the only in-scope remedy was to trim the new
|
||||
record to fit the constant — the ratchet pointed at record authors, which is precisely
|
||||
what the v3 note says this whole design abolishes.
|
||||
remedy the assertion admitted was to RAISE the ceiling. That is the ratchet this whole
|
||||
change abolishes, reinstated as a hard failure in what #631 makes a blocking CI job.
|
||||
The fraction band had the same coupling more slowly (12 more long records breached it),
|
||||
and `0 <= headroom` was vacuous — `max(under)` is by construction <= ceiling.
|
||||
|
||||
v5 SPLITS the claim by robustness instead of hunting for a better single assertion:
|
||||
|
||||
* the COARSE property — the ceiling flags a meaningful minority — is asserted HERE,
|
||||
blocking. One record moves a fraction by at most 1/N, so no SINGLE ordinary addition can
|
||||
cross it — measured headroom, not immunity (38 over-ceiling additions, 718 short ones, or
|
||||
consolidating 15 of the 18 offenders would each reach a bound).
|
||||
* the FINE property — `p90 <= ceiling <= p95` — is now REPORTED by `main()` as a notice.
|
||||
It is real signal about the CONSTANT drifting out of date, which is the passage of corpus
|
||||
growth rather than a defect in the commit under test. That is the same reasoning
|
||||
`stale_records` is built on, and it gets the same treatment.
|
||||
|
||||
Note what did NOT change: the ceiling is still 60, and the fine claim is still measured on
|
||||
every run. v5 moves where each claim is enforced, it does not stop making them.
|
||||
v4 states the property directly and scale-free: **the ceiling marks the start of the tail**,
|
||||
i.e. it sits between the 90th and 95th percentile of record lengths. Percentiles move WITH the
|
||||
corpus, so routine growth cannot ratchet this; it fires only when the ceiling genuinely stops
|
||||
marking the tail boundary, which is exactly when it should be re-derived.
|
||||
"""
|
||||
recs = [r for r in dl.all_active_records() if r.key]
|
||||
# A low floor on purpose: this guards against a VACUOUS scan, not against corpus shrinkage.
|
||||
# At >100 it would red after ~83 legitimate retirements even with the ceiling still calibrated.
|
||||
assert len(recs) > 20, f"corpus looks empty ({len(recs)}) — this check would be vacuous"
|
||||
assert len(recs) > 100, f"corpus looks empty ({len(recs)}) — this check would be vacuous"
|
||||
|
||||
ceiling = dv.RECORD_CEILING_DEFAULT # the value the CLI actually uses; cannot drift from here
|
||||
cal = dv.ceiling_calibration(recs, ceiling)
|
||||
ceiling = dv.RECORD_CEILING_DEFAULT # the value the CLI actually uses; cannot drift from here
|
||||
lengths = sorted(dv.record_prose_lines(r) for r in recs)
|
||||
p90 = lengths[int(len(lengths) * 0.90)]
|
||||
p95 = lengths[int(len(lengths) * 0.95)]
|
||||
|
||||
assert cal.flags_minority, (
|
||||
f"the ceiling ({ceiling}) no longer flags a nonempty proper minority of records: "
|
||||
f"{cal.n_over}/{cal.n} = {cal.fraction_over:.1%} are over it. At 0% it names nobody and "
|
||||
f"signals nothing; above {dv.CEILING_MINORITY_MAX:.0%} it is cutting into the bulk of the "
|
||||
f"corpus rather than marking its tail. Re-derive it from the distribution."
|
||||
assert p90 <= ceiling <= p95, (
|
||||
f"the ceiling ({ceiling}) no longer marks the tail boundary: p90={p90}, p95={p95}. "
|
||||
f"Below p90 it cuts into the bulk and every author will learn to ignore it; above p95 it is "
|
||||
f"parked among the outliers and signals nothing. Re-derive it from the distribution."
|
||||
)
|
||||
|
||||
|
||||
def test_ceiling_calibration_detects_drift_in_BOTH_directions():
|
||||
"""The fine claim is asserted here, on a distribution the test OWNS.
|
||||
|
||||
This is the point of the v5 split: the property is still pinned, but against synthetic data
|
||||
instead of the live corpus, so it cannot be reddened by someone else's record landing.
|
||||
"""
|
||||
# 100 records: 95 of 20 lines, 5 of 200. Index 90 lands in the short block and index 95 in the
|
||||
# long one, so p90 == 20 and p95 == 200 — a wide, unambiguous tail boundary to aim at.
|
||||
recs = [_rec_body(f"a.s{i}", 20) for i in range(95)] + [_rec_body(f"a.l{i}", 200) for i in range(5)]
|
||||
assert [dv.ceiling_calibration(recs, 60).p90, dv.ceiling_calibration(recs, 60).p95] == [20, 200]
|
||||
|
||||
assert dv.ceiling_calibration(recs, 60).marks_tail, "60 sits between p90=20 and p95=200"
|
||||
assert not dv.ceiling_calibration(recs, 10).marks_tail, "below p90 it cuts into the bulk"
|
||||
assert not dv.ceiling_calibration(recs, 999).marks_tail, "above p95 it is parked among outliers"
|
||||
|
||||
# BOTH ends of `marks_tail` are inclusive. Review found the upper one unpinned — `ceiling <= p95`
|
||||
# mutated to `<` survived the whole suite. It is notice-only rather than blocking, but an
|
||||
# unpinned boundary is how a documented claim quietly stops being true.
|
||||
assert dv.ceiling_calibration(recs, 20).marks_tail, "p90 itself must satisfy the lower bound"
|
||||
assert dv.ceiling_calibration(recs, 200).marks_tail, "p95 itself must satisfy the upper bound"
|
||||
assert not dv.ceiling_calibration(recs, 201).marks_tail, "one line above p95 must not"
|
||||
|
||||
# and the coarse property separates the same two failure modes
|
||||
assert not dv.ceiling_calibration(recs, 999).flags_minority, "a ceiling nobody is over signals nothing"
|
||||
assert not dv.ceiling_calibration(recs, 10).flags_minority, "100% over the ceiling is not a tail"
|
||||
assert dv.ceiling_calibration(recs, 60).flags_minority
|
||||
|
||||
|
||||
def test_the_coarse_bound_REJECTS_a_badly_placed_ceiling():
|
||||
"""The blocking property must have teeth.
|
||||
|
||||
Review's strongest finding on the first draft: a floor of `fraction_over > 0` was nearly
|
||||
unfalsifiable — measured on the live corpus it accepted every ceiling from 39 to 229, including
|
||||
the ceiling of 200 the docstring itself offered as the case it catches, because one 230-line
|
||||
record keeps the count nonzero. A FRACTION floor is what restores the teeth.
|
||||
|
||||
The rejections are pinned on a SYNTHETIC distribution: asserting that a specific absurd ceiling
|
||||
stays rejected by the live corpus is itself growth-coupled (three new 200+ line records flip the
|
||||
200 arm). Only the acceptance of today's ceiling is checked against live data.
|
||||
"""
|
||||
# The TEETH are demonstrated on an owned distribution, for the reason in
|
||||
# `test_v4_would_have_reddened_where_v5_holds`: an assertion that a specific absurd ceiling is
|
||||
# rejected by the LIVE corpus is itself growth-coupled (review found that three new 200+ line
|
||||
# records would flip the 200 arm). 100 records of 30 lines and one of 230 — an outlier-only
|
||||
# tail, which is precisely the shape a badly-placed ceiling fails to distinguish.
|
||||
synthetic = [_rec_body(f"a.s{i}", 30) for i in range(100)] + [_rec_body("a.outlier", 230)]
|
||||
|
||||
for bad in (200, 229, 230):
|
||||
cal = dv.ceiling_calibration(synthetic, bad)
|
||||
assert not cal.flags_minority, (
|
||||
f"a ceiling of {bad} flags only {cal.n_over}/{cal.n} records and must be rejected, got {cal}"
|
||||
)
|
||||
assert not dv.ceiling_calibration(synthetic, 10).flags_minority, "a ceiling of 10 cuts into the bulk"
|
||||
|
||||
# The only claim made against the LIVE corpus is the robust one: today's ceiling is accepted.
|
||||
# Reaching a bound takes 38 consecutive over-ceiling additions, 718 short ones by dilution, or
|
||||
# consolidating 15 of the 18 offenders — the tightest arm, and the one worth remembering.
|
||||
recs = [r for r in dl.all_active_records() if r.key]
|
||||
assert len(recs) > 20, "corpus looks empty — this check would be vacuous"
|
||||
assert dv.ceiling_calibration(recs, dv.RECORD_CEILING_DEFAULT).flags_minority
|
||||
|
||||
|
||||
def test_ceiling_calibration_is_empty_safe():
|
||||
"""A vacuous corpus must report both claims FALSE, never a passing default."""
|
||||
cal = dv.ceiling_calibration([], 60)
|
||||
assert cal.n == 0 and not cal.marks_tail and not cal.flags_minority
|
||||
|
||||
|
||||
def test_the_minority_band_BOUNDARIES_are_exactly_where_documented():
|
||||
"""Pins both constants AND both inclusivities, which review found entirely unmutated.
|
||||
|
||||
Mutating `0.02 -> 0.03`, `0.25 -> 0.30`, or either `<=` to `<` passed all eight calibration
|
||||
tests. These are not free parameters — they ARE the documented CI-red thresholds, so a silent
|
||||
shift changes them (a strict cap reds after 37 long additions instead of 38; a strict floor
|
||||
after 717 short ones instead of 718), quietly falsifying the numbers in `docs.corpus-size-signal`
|
||||
and `docs/ci-cd.md`.
|
||||
|
||||
100-record fixtures make the fraction exact and readable: k over the ceiling IS k%. Both
|
||||
`2/100` and `25/100` are exactly representable and compare equal to the module constants, so
|
||||
these are true boundary cases rather than near-misses.
|
||||
"""
|
||||
|
||||
def corpus(n_over: int, total: int = 100):
|
||||
return [_rec_body(f"a.o{i}", 61) for i in range(n_over)] + [
|
||||
_rec_body(f"b.u{i}", 10) for i in range(total - n_over)
|
||||
]
|
||||
|
||||
# The bounds are INCLUSIVE — exactly on either edge still passes.
|
||||
assert dv.ceiling_calibration(corpus(2), 60).flags_minority, "the 2% floor must be inclusive"
|
||||
assert dv.ceiling_calibration(corpus(25), 60).flags_minority, "the 25% cap must be inclusive"
|
||||
|
||||
# ...and one record beyond either edge does not.
|
||||
assert not dv.ceiling_calibration(corpus(1), 60).flags_minority, "1% is below the floor"
|
||||
assert not dv.ceiling_calibration(corpus(26), 60).flags_minority, "26% is above the cap"
|
||||
|
||||
# The constants themselves, so a change has to be deliberate and visible in the diff.
|
||||
assert (dv.CEILING_MINORITY_MIN, dv.CEILING_MINORITY_MAX) == (0.02, 0.25)
|
||||
|
||||
|
||||
# `test_adding_ordinary_records_cannot_RED_the_blocking_property` used to live here. It appended two
|
||||
# long synthetic records to the LIVE corpus and asserted `flags_minority` on the result — which
|
||||
# crosses the 25% cap TWO records before the production bound does (56/221 vs 54/219), making the
|
||||
# test named "cannot RED the blocking property" a tighter tripwire than the property it guarded.
|
||||
# That is the #688 defect in miniature, and the fourth instance found in this change.
|
||||
#
|
||||
# Deleted rather than tuned, because both of its jobs are covered without touching live data:
|
||||
# `test_v4_would_have_reddened_where_v5_holds` demonstrates the v4/v5 contrast on an owned
|
||||
# distribution, and `test_real_corpus_ceiling_flags_a_nonempty_proper_minority` is the deliberate
|
||||
# live guard — at the production threshold rather than two records inside it.
|
||||
|
||||
|
||||
def test_ceiling_calibration_IGNORES_keyless_records_and_counts_the_rest():
|
||||
"""`n` and the `if r.key` filter, both of which review found unpinned.
|
||||
|
||||
`main()` passes the UNFILTERED record list, so the filter is load-bearing in production while
|
||||
every live-corpus test hands this function a pre-filtered list — the oracle and production's
|
||||
input agreed only by accident. The corpus really does carry keyless entries (the generated
|
||||
"Records formerly in this file" scaffolding, one of them 106 lines), and counting them would
|
||||
drag p90/p95 around with content that is not a record.
|
||||
|
||||
`n` itself lost its only pin when the over-tight live test was deleted: a mutation returning
|
||||
`n=1` passed everything, which would print a wrong denominator in the drift notice.
|
||||
|
||||
The oracle is DYNAMIC and runs at two distinct cardinalities on purpose. The first attempt
|
||||
asserted `n == 10` against a ten-record fixture, and review killed it: a mutation returning a
|
||||
constant 10 for every input satisfied it while changing the live denominator from 183 to 10 —
|
||||
preserving the exact production defect the test claims to close. A single hardcoded count
|
||||
cannot distinguish "counts the input" from "returns this number".
|
||||
"""
|
||||
for size in (7, 13):
|
||||
recs = [_rec_body(f"a.s{i}", 10) for i in range(size)]
|
||||
assert dv.ceiling_calibration(recs, 60).n == size, f"n must count the {size} keyed records given"
|
||||
|
||||
recs = [_rec_body(f"a.s{i}", 10) for i in range(9)] + [_rec_body("b.long", 500)]
|
||||
keyless = _rec(key=None, heading="Records formerly in this file", body="\n".join("x" for _ in range(500)))
|
||||
assert dv.ceiling_calibration(recs + [keyless], 60) == dv.ceiling_calibration(recs, 60)
|
||||
|
||||
|
||||
def test_ceiling_calibration_counts_over_the_ceiling_EXCLUSIVELY():
|
||||
"""`n_over` is recomputed inside `ceiling_calibration`, so its boundary needs its own pin.
|
||||
|
||||
`oversized_records` has an exclusivity test; this counter does not share its code. Flipping
|
||||
`>` to `>=` here would silently shift the fraction by the number of records sitting exactly ON
|
||||
the ceiling (3 in the live corpus), and the mutation survived the whole suite.
|
||||
"""
|
||||
recs = [_rec_body("a.under", 59), _rec_body("b.exact", 60), _rec_body("c.over", 61)]
|
||||
assert dv.ceiling_calibration(recs, 60).n_over == 1
|
||||
|
||||
|
||||
def test_ceiling_calibration_uses_the_95th_percentile_not_a_higher_one():
|
||||
"""Pins p95's quantile. The synthetic 95/5 fixture cannot tell 0.95 from 0.99, so a mutation
|
||||
widening the upper quantile survived the whole suite."""
|
||||
# 100 records: indices 0..89 = 10, 90..94 = 50, 95..98 = 90, 99 = 900.
|
||||
recs = (
|
||||
[_rec_body(f"a.s{i}", 10) for i in range(90)]
|
||||
+ [_rec_body(f"b.m{i}", 50) for i in range(5)]
|
||||
+ [_rec_body(f"c.h{i}", 90) for i in range(4)]
|
||||
+ [_rec_body("d.max", 900)]
|
||||
)
|
||||
cal = dv.ceiling_calibration(recs, 60)
|
||||
assert (cal.p90, cal.p95) == (50, 90), f"p95 must read index 95, not a higher quantile: {cal}"
|
||||
|
||||
|
||||
def test_v4_would_have_reddened_where_v5_holds():
|
||||
"""The v4-vs-v5 contrast, on a distribution the test OWNS rather than the live corpus.
|
||||
|
||||
THIRD TIME for this defect class in one change, which is why the fix is to remove the coupling
|
||||
rather than patch the instance. Round 1 of review caught it in the drift test; round 2 caught it
|
||||
here, in what looked like a safe `if before.marks_tail:` guard — the GUARD was conditional but
|
||||
the CONCLUSION was still an assertion about live order statistics, and appending 16 ordinary
|
||||
30-line records (nothing long, nothing unusual) makes `after.marks_tail` true again and fires it:
|
||||
|
||||
extra= 0 before(marks=True) after(marks=False) -> reds: False
|
||||
extra=16 before(marks=True) after(marks=True) -> reds: True
|
||||
|
||||
Nothing about this demonstration needs the real corpus. The synthetic base reproduces the shape
|
||||
that matters — a sparse gap immediately above the ceiling, which is what #688 measured on
|
||||
`main` (nothing at all between 60 and 81) — so two over-ceiling additions advance p90 off the
|
||||
ceiling and break v4, while v5 is untouched.
|
||||
"""
|
||||
base = (
|
||||
[_rec_body(f"a.s{i}", 30) for i in range(90)] # the bulk
|
||||
+ [_rec_body("a.edge", 60)] # sits exactly ON the ceiling, as main does today
|
||||
+ [_rec_body(f"a.l{i}", 112) for i in range(9)] # the tail, across a sparse gap
|
||||
)
|
||||
before = dv.ceiling_calibration(base, 60)
|
||||
assert (before.p90, before.p95) == (60, 112), before
|
||||
assert before.marks_tail and before.flags_minority, before
|
||||
|
||||
after = dv.ceiling_calibration(base + [_rec_body("new.a", 107), _rec_body("new.b", 107)], 60)
|
||||
|
||||
assert not after.marks_tail, f"v4 must break on these additions, or the contrast is empty: {after}"
|
||||
assert after.flags_minority, f"v5 must survive what broke v4: {after}"
|
||||
|
||||
|
||||
# --- #674: the validator cross-checks its own parse against PyYAML ------------------------------
|
||||
|
||||
|
||||
_HAZARDS = {
|
||||
# PyYAML REJECTS: the bare apostrophe closes the single-quoted scalar early.
|
||||
"apostrophe": "rule: 'SQLite's LOWER() folds ASCII only'",
|
||||
# PyYAML ACCEPTS but reads a DIFFERENT value: ` #` starts a comment, truncating the rule.
|
||||
"unquoted-hash": "rule: use --flag #2 for this",
|
||||
}
|
||||
|
||||
|
||||
def _wing_with(tmp_path: Path, frontmatter_line: str) -> tuple[Path, Path]:
|
||||
"""A record wing containing one file whose frontmatter carries `frontmatter_line`."""
|
||||
records = tmp_path / "records" / "ci"
|
||||
records.mkdir(parents=True)
|
||||
(records / "a.md").write_text(
|
||||
"---\n"
|
||||
"key: ci.a\n"
|
||||
"title: 'T'\n"
|
||||
"status: active\n"
|
||||
"since: '2026-01-01'\n"
|
||||
"supersedes: none\n"
|
||||
"superseded-by: none\n"
|
||||
f"{frontmatter_line}\n"
|
||||
"signals: 's'\n"
|
||||
"---\n\nprose.\n"
|
||||
)
|
||||
archive = tmp_path / "archive"
|
||||
archive.mkdir(parents=True)
|
||||
return records, archive
|
||||
|
||||
|
||||
@pytest.mark.parametrize("hazard", sorted(_HAZARDS))
|
||||
def test_pyyaml_crosscheck_catches_frontmatter_the_hand_reader_accepts(tmp_path, hazard):
|
||||
"""#674, both shapes. Hit twice in one session by two independent agents (#578, #651)."""
|
||||
pytest.importorskip("yaml")
|
||||
records, _ = _wing_with(tmp_path, _HAZARDS[hazard])
|
||||
|
||||
faults, ran = dv.pyyaml_frontmatter_faults(sorted(records.rglob("*.md")))
|
||||
assert ran, "PyYAML is installed here, so the cross-check must have run"
|
||||
assert faults, f"the {hazard} hazard slipped through the cross-check"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("hazard", sorted(_HAZARDS))
|
||||
def test_the_hand_reader_really_IS_blind_to_these(tmp_path, hazard):
|
||||
"""The positive control: pin the MECHANISM, so this suite cannot pass for the wrong reason.
|
||||
|
||||
If `record_wing_faults` ever started catching these on its own, the cross-check above could be
|
||||
deleted and the tests would stay green while the guard vanished. Asserting that the pre-#674
|
||||
machinery reports these files as CLEAN is what makes the cross-check's red meaningful — and it
|
||||
is the exact state #674 was filed about: `decisions_validate.py` printed OK on input the
|
||||
writer's own library rejects.
|
||||
"""
|
||||
records, archive = _wing_with(tmp_path, _HAZARDS[hazard])
|
||||
assert dv.record_wing_faults(records, archive) == [], (
|
||||
"the structural guard now catches this by itself — re-derive whether the PyYAML "
|
||||
"cross-check is still the thing closing this gap"
|
||||
)
|
||||
|
||||
|
||||
def test_crosscheck_REPORTS_an_impossible_date_instead_of_crashing(tmp_path):
|
||||
"""PyYAML raises a bare `ValueError`, not a `YAMLError`, for a well-shaped impossible date.
|
||||
|
||||
`stale-after: 2026-06-31` (June has 30 days) escaped an `except yaml.YAMLError` as a traceback,
|
||||
killing the validator on any machine with PyYAML — including the Husky pre-commit hook. A check
|
||||
documented as "strictly additive, must never be the reason the validator cannot run" must
|
||||
REPORT this, so the except is deliberately broad.
|
||||
"""
|
||||
pytest.importorskip("yaml")
|
||||
records, _ = _wing_with(tmp_path, "stale-after: 2026-06-31")
|
||||
|
||||
faults, ran = dv.pyyaml_frontmatter_faults(sorted(records.rglob("*.md")))
|
||||
assert ran
|
||||
assert faults and "REJECTS" in faults[0], faults
|
||||
assert "ValueError" in faults[0], f"the fault must name the real exception: {faults[0]}"
|
||||
|
||||
|
||||
def test_crosscheck_survives_a_TYPED_mapping_key(tmp_path):
|
||||
"""PyYAML returns typed keys, so a stray `1: x` yields int 1 where the reader yields "1".
|
||||
|
||||
Sorting that mixed set raised `TypeError` — an uncaught traceback replacing what
|
||||
`_unknown_frontmatter_keys` used to report as an actionable error. Removing the `key=str` sort
|
||||
key restores the crash, and without this test every other test here stays green.
|
||||
"""
|
||||
pytest.importorskip("yaml")
|
||||
records, _ = _wing_with(tmp_path, "1: stray")
|
||||
|
||||
faults, ran = dv.pyyaml_frontmatter_faults(sorted(records.rglob("*.md")))
|
||||
assert ran
|
||||
assert faults, "a typed mapping key must be reported, not swallowed"
|
||||
assert any("1" in f for f in faults), faults
|
||||
|
||||
|
||||
def test_pyyaml_crosscheck_is_clean_on_the_REAL_corpus():
|
||||
"""No false positives. A cross-check that flags correct records would be reverted within a day."""
|
||||
pytest.importorskip("yaml")
|
||||
files = dv.record_wing_files()
|
||||
assert len(files) > 100, f"only {len(files)} wing files found — this check would be near-vacuous"
|
||||
|
||||
faults, ran = dv.pyyaml_frontmatter_faults(files)
|
||||
assert ran
|
||||
assert faults == [], "the cross-check disagrees with the live corpus:\n" + "\n".join(faults[:5])
|
||||
|
||||
|
||||
def test_crosscheck_skips_cleanly_when_pyyaml_is_absent(tmp_path, monkeypatch):
|
||||
"""The read path stays dependency-free (#674's second Done-when box).
|
||||
|
||||
`decisions-guard`, the Husky hooks and every contributor machine install nothing, so an absent
|
||||
PyYAML must SKIP the cross-check rather than fault or crash — while every other check runs.
|
||||
"""
|
||||
records, _ = _wing_with(tmp_path, _HAZARDS["apostrophe"])
|
||||
|
||||
import builtins
|
||||
|
||||
real_import = builtins.__import__
|
||||
|
||||
def no_yaml(name, *a, **kw):
|
||||
if name == "yaml":
|
||||
raise ImportError("no yaml here")
|
||||
return real_import(name, *a, **kw)
|
||||
|
||||
monkeypatch.setattr(builtins, "__import__", no_yaml)
|
||||
|
||||
faults, ran = dv.pyyaml_frontmatter_faults(sorted(records.rglob("*.md")))
|
||||
assert ran is False, "an absent PyYAML must report that the check did not run"
|
||||
assert faults == [], "a skipped check must not manufacture faults"
|
||||
|
||||
|
||||
def test_main_ANNOUNCES_a_skipped_crosscheck(capsys, monkeypatch):
|
||||
"""A skipped check that says nothing is the '#603 stale-after' defect: reports success, does
|
||||
nothing. The skip is legitimate; staying quiet about it is not."""
|
||||
monkeypatch.setattr(dv, "pyyaml_frontmatter_faults", lambda files: ([], False))
|
||||
assert dv.main([]) == 0
|
||||
err = capsys.readouterr().err
|
||||
assert "cross-check" in err and "SKIPPED" in err, err
|
||||
|
||||
|
||||
def test_main_FEEDS_the_crosscheck_the_REAL_wing_files(monkeypatch, capsys):
|
||||
"""Pins the cross-check's INPUT, not just that its output is consumed.
|
||||
|
||||
Mutation testing found this hole: replacing `pyyaml_frontmatter_faults(record_wing_files())`
|
||||
with `pyyaml_frontmatter_faults([])` in main() left the ENTIRE suite green — exit 0, no skip
|
||||
notice, every other test passing. The two wiring tests monkeypatch the function itself, so they
|
||||
prove the return value reaches `errs`; nothing proved the argument was the corpus. That is the
|
||||
'#609 marker that printed OK while doing nothing' defect one level up, which is the exact thing
|
||||
this record indicts — and the sibling of `test_main_actually_CALLS_the_wing_scan`.
|
||||
"""
|
||||
seen: list[list] = []
|
||||
|
||||
def spy(files):
|
||||
seen.append(list(files))
|
||||
return [], True
|
||||
|
||||
monkeypatch.setattr(dv, "pyyaml_frontmatter_faults", spy)
|
||||
dv.main([])
|
||||
|
||||
assert seen, "main() never called the cross-check at all"
|
||||
assert len(seen[0]) > 100, f"main() passed only {len(seen[0])} file(s) — not the real wings"
|
||||
assert set(seen[0]) == set(dv.record_wing_files()), (
|
||||
"main() passed a file list that is not record_wing_files() — the cross-check is not seeing "
|
||||
"the corpus it is supposed to check"
|
||||
)
|
||||
|
||||
|
||||
def test_main_FAILS_when_the_crosscheck_reports_a_fault(capsys, monkeypatch):
|
||||
"""Wiring test: the faults must reach the exit code, not just be computed.
|
||||
|
||||
Without this, `wing_faults=record_wing_faults() + yaml_faults` could drop the second term and
|
||||
every other test here would still pass.
|
||||
"""
|
||||
monkeypatch.setattr(dv, "pyyaml_frontmatter_faults", lambda files: (["x.md: bogus fault"], True))
|
||||
assert dv.main([]) == 1
|
||||
assert "bogus fault" in capsys.readouterr().err
|
||||
|
||||
|
||||
def test_retired_budget_flag_says_it_is_ignored(capsys):
|
||||
"""A retired flag must announce itself, not no-op silently.
|
||||
|
||||
@@ -1430,104 +1046,24 @@ def test_retired_budget_flag_says_it_is_ignored(capsys):
|
||||
|
||||
|
||||
def test_no_budget_flag_means_no_retirement_warning(capsys):
|
||||
# Match the retirement notice specifically, not a bare "RETIRED": a legitimate record whose
|
||||
# TITLE contains that word and whose `stale-after` has passed gets printed by the stale notice,
|
||||
# which would red this on an unrelated corpus change.
|
||||
dv.main([])
|
||||
assert "is RETIRED and was IGNORED" not in capsys.readouterr().err
|
||||
assert "RETIRED" not in capsys.readouterr().err
|
||||
|
||||
|
||||
|
||||
def test_main_reports_ceiling_drift_as_a_NOTICE_and_still_exits_0(capsys):
|
||||
"""The fine claim's live wiring (#688): the drift notice must fire, and must NOT turn the run
|
||||
red — the entire point of the v5 split.
|
||||
|
||||
The ceiling is DERIVED as one line above the longest record, so it is off the tail boundary by
|
||||
definition. A hardcoded 999 looked safe and was not: review showed ten valid 1000-line records
|
||||
would put p95 at 1000, making 999 calibrated — so the notice would stop firing and this test
|
||||
would go RED, for a corpus change that is nobody's defect.
|
||||
"""
|
||||
longest = max(dv.record_prose_lines(r) for r in dl.all_active_records() if r.key)
|
||||
assert dv.main(["--record-ceiling", str(longest + 1)]) == 0
|
||||
err = capsys.readouterr().err
|
||||
drift = [ln for ln in err.splitlines() if "drifted from the tail boundary" in ln]
|
||||
assert len(drift) == 1, err
|
||||
assert drift[0].startswith("::notice::"), f"drift must be a notice, not a warning: {drift[0]}"
|
||||
|
||||
|
||||
def test_main_reports_drift_IFF_the_ceiling_is_off_the_tail_boundary(capsys):
|
||||
"""The complement of the test above — asserting the WIRING, not the corpus's current state.
|
||||
|
||||
The obvious way to write this is `dv.main([]); assert "drifted" not in err`, and that is a trap
|
||||
review caught: `main()` emits the notice exactly when `p90 <= 60 <= p95` is false over the LIVE
|
||||
corpus, so such a test fails under precisely the condition #688 exists to stop failing — it
|
||||
would move v4's assertion three functions down and leave it in the same blocking job. Today p90
|
||||
sits exactly ON the ceiling, so ONE new over-ceiling record would have reddened it.
|
||||
|
||||
So the oracle is `ceiling_calibration` itself: whatever the corpus currently looks like, the
|
||||
notice must be present iff the fine claim is false. The 999 case pins that at least one branch
|
||||
is genuinely exercised, so this cannot pass by never firing.
|
||||
"""
|
||||
recs = [r for r in dl.all_active_records() if r.key]
|
||||
# Non-empty is all the derivations below need; a higher floor would itself be a growth tripwire.
|
||||
assert recs, "corpus is empty — the derived ceilings need at least one record"
|
||||
lengths = sorted(dv.record_prose_lines(r) for r in recs)
|
||||
|
||||
# Both ceilings are DERIVED so each branch is guaranteed by construction, not by luck. Review
|
||||
# caught the earlier version relying on the live 60/999 pair: once one 61-line record lands,
|
||||
# BOTH of those drift, and an UNCONDITIONAL notice would have passed the test.
|
||||
# * p90 itself is always calibrated — `p90 <= p90 <= p95` holds for any distribution.
|
||||
# * one line above the longest record is always off the tail — it exceeds p95 by definition.
|
||||
quiet_ceiling = lengths[min(int(len(lengths) * 0.90), len(lengths) - 1)]
|
||||
drift_ceiling = lengths[-1] + 1
|
||||
|
||||
expectations = []
|
||||
for ceiling in (quiet_ceiling, drift_ceiling):
|
||||
expected = not dv.ceiling_calibration(recs, ceiling).marks_tail
|
||||
dv.main(["--record-ceiling", str(ceiling)])
|
||||
err = capsys.readouterr().err
|
||||
assert ("drifted from the tail boundary" in err) is expected, (
|
||||
f"ceiling {ceiling}: expected drift notice={expected}, got the opposite"
|
||||
)
|
||||
expectations.append(expected)
|
||||
|
||||
assert expectations == [False, True], (
|
||||
f"the two derived ceilings must exercise BOTH branches, got {expectations} — otherwise an "
|
||||
f"unconditional notice (or none at all) would pass this test"
|
||||
)
|
||||
|
||||
|
||||
def test_main_actually_REPORTS_the_ceiling_and_the_trend(capsys):
|
||||
"""The new signal's live wiring was untested: `if oversized:` -> `if False:`, or bumping the
|
||||
default ceiling to 999999, left every test green while main() reported nothing. Only the pure
|
||||
function `oversized_records()` was covered — so the replacement signal could silently do
|
||||
nothing, which is the exact defect this change exists to retire.
|
||||
|
||||
Stated as an IFF against the live offender list rather than `assert over` (#688): the ceiling
|
||||
is ALLOWED to go green — `test_oversized_records_can_go_green` says so explicitly — so a bare
|
||||
precondition that the corpus still has an offender would red the blocking job the day someone
|
||||
consolidates the last one, punishing exactly the work the warning asks for."""
|
||||
nothing, which is the exact defect this change exists to retire."""
|
||||
dv.main([])
|
||||
err = capsys.readouterr().err
|
||||
assert "prose lines across" in err, "the aggregate trend notice must always print"
|
||||
|
||||
assert "exceed the" in err and "prose ceiling" in err, "the per-record ceiling warning must print"
|
||||
over = [r.key for r in dl.all_active_records()
|
||||
if r.key and dv.record_prose_lines(r) > dv.RECORD_CEILING_DEFAULT]
|
||||
warned = "exceed the" in err and "prose ceiling" in err
|
||||
assert warned is bool(over), f"ceiling warning printed={warned} but {len(over)} record(s) are over it"
|
||||
if over:
|
||||
assert any(k in err for k in over), "the warning must NAME the offending records"
|
||||
|
||||
# The IFF above is only non-vacuous while an offender exists: once the corpus is legitimately
|
||||
# consolidated to zero, `False is False` passes even if main()'s whole `if oversized:` branch
|
||||
# were deleted. So force the branch with a ceiling nothing can sit under. It is -1, not 0:
|
||||
# an empty record body is validator-valid and `record_prose_lines` returns 0, so a corpus of
|
||||
# empty-bodied records has no offender at 0. Below zero the arm cannot go vacuous at all.
|
||||
dv.main(["--record-ceiling", "-1"])
|
||||
forced = capsys.readouterr().err
|
||||
assert "exceed the" in forced and "prose ceiling" in forced, (
|
||||
"at a ceiling of -1 every record is over it — the warning branch must fire"
|
||||
)
|
||||
assert over, "precondition: the live corpus has at least one over-ceiling record"
|
||||
assert any(k in err for k in over), "the warning must NAME the offending records"
|
||||
|
||||
|
||||
def test_trend_notice_reports_record_prose_and_scaffolding_separately(capsys):
|
||||
|
||||
@@ -70,20 +70,8 @@ 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"),
|
||||
"sha": base_sha},
|
||||
"base": {"ref": os.environ.get("STUB_BASE", "main")},
|
||||
"body": "no linked issue here"}))
|
||||
sys.exit(0)
|
||||
|
||||
|
||||
@@ -80,21 +80,7 @@ if "/pulls/" in url:
|
||||
if base_after.exists() and n >= 1: # retargeted mid-enumeration
|
||||
base = base_after.read_text().strip()
|
||||
|
||||
# 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}))
|
||||
print(json.dumps({"head": {"sha": sha}, "base": {"ref": base}}))
|
||||
sys.exit(0)
|
||||
|
||||
print("{}")
|
||||
@@ -139,21 +125,6 @@ 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]
|
||||
@@ -372,78 +343,6 @@ 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).
|
||||
|
||||
@@ -890,100 +789,9 @@ 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 == "sentinel-after-post":
|
||||
# Another overlapping run repaired mid-flight: its SENTINEL lands above this run's mark, while
|
||||
# the human row it records sits BELOW the mark and is therefore invisible here (round 5).
|
||||
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": 6000, "context": "review-verdict/h10", "status": "pending",
|
||||
"creator": None,
|
||||
"description": "Human verdict raced this exemption write — re-post the verdict"}]
|
||||
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
|
||||
@@ -1005,19 +813,6 @@ if "/status" in url:
|
||||
"creator": {"login": "timothy"},
|
||||
"description": "Review-verdict: BLOCKED @ a9e3e23 (base: main)"}]}))
|
||||
sys.exit(0)
|
||||
if mode.startswith("sentinel-appears-on-read:"):
|
||||
# A repair sentinel written by ANOTHER, overlapping run between this job's first read and its
|
||||
# last-moment re-read (ersatztv#706 round 3). Creator is null: the sentinel is machine-written.
|
||||
nth = int(mode.split(":", 1)[1])
|
||||
ctr = out / "status_reads.txt"
|
||||
n = int(ctr.read_text()) if ctr.exists() else 0
|
||||
ctr.write_text(str(n + 1))
|
||||
if n + 1 < nth:
|
||||
print(json.dumps({"statuses": []})); sys.exit(0)
|
||||
print(json.dumps({"statuses": [
|
||||
{"context": "review-verdict/h10", "status": "pending", "creator": None,
|
||||
"description": "Human verdict raced this exemption write — re-post the verdict"}]}))
|
||||
sys.exit(0)
|
||||
if mode == "transport-error":
|
||||
# Real `gh()` is `curl -sf`: an HTTP error exits 22 with EMPTY stdout.
|
||||
sys.exit(22)
|
||||
@@ -1054,13 +849,11 @@ 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)",
|
||||
timeline_mode: str = "none", history_mode: str = "none"):
|
||||
status_desc: str = "Review-verdict: MERGEABLE @ a9e3e23 (base: main)"):
|
||||
"""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.
|
||||
"""
|
||||
tmp_path.mkdir(parents=True, exist_ok=True) # the chained sentinel test passes sub-paths
|
||||
bindir = tmp_path / "bin"; bindir.mkdir()
|
||||
curl = bindir / "curl"; curl.write_text(WORKFLOW_STUB_CURL); curl.chmod(0o755)
|
||||
if jq16:
|
||||
@@ -1080,8 +873,6 @@ 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",
|
||||
@@ -1751,363 +1542,3 @@ 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"]
|
||||
# Assert on BOTH the init and the FETCH: keying only on the init line would let a refactor that
|
||||
# splits initialisation from the read slide the actual round-trip back past the re-read while this
|
||||
# test stayed green (round-3 review nit).
|
||||
mark = max(src.index("max_id_before=-1"), src.index('hist_before=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA'))
|
||||
# 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:]}")
|
||||
|
||||
|
||||
def test_the_repair_sentinel_is_a_FIXED_POINT_across_consecutive_runs(tmp_path):
|
||||
"""Round-3 finding: durability is a fixed point, and only a CHAIN can assert a fixed point.
|
||||
|
||||
The first version of the sentinel refused the exemption but posted the GENERIC pending
|
||||
description, erasing the marker it depends on. The next run then saw an ordinary machine
|
||||
`pending`, re-derived it, and posted `success` — burying the human rejection two events after the
|
||||
repair instead of one. The single-hop test passed throughout, and the positive control asserting
|
||||
that an ordinary machine `pending` DOES re-derive was itself the proof of the second hop.
|
||||
|
||||
So chain two runs: feed run N's posted description in as run N+1's existing status. The property
|
||||
is that the sentinel branch's own output re-triggers the sentinel branch, forever.
|
||||
"""
|
||||
first, r1 = _run_classify(
|
||||
tmp_path / "run1", _emitting("docs/a.md"),
|
||||
status_mode="existing:pending", status_creator=None,
|
||||
status_desc="Human verdict raced this exemption write — re-post the verdict")
|
||||
assert first is not None and first["state"] == "pending", f"run 1: {r1.stdout[-600:]}"
|
||||
|
||||
second, r2 = _run_classify(
|
||||
tmp_path / "run2", _emitting("docs/a.md"),
|
||||
status_mode="existing:pending", status_creator=None,
|
||||
status_desc=first["description"]) # <-- the chain
|
||||
assert second is not None, f"run 2 posted nothing: {r2.stderr[-800:]}"
|
||||
assert second["state"] == "pending", (
|
||||
"the repair decayed: run 1's own posted description did not re-trigger the sentinel, so run 2 "
|
||||
f"re-derived the exemption and buried the human verdict again (run1 desc={first['description']!r}, "
|
||||
f"run2 state={second['state']}, run2 desc={second['description']!r})")
|
||||
assert second["description"] == first["description"], (
|
||||
"the sentinel is not a fixed point — run 2 wrote a different description than run 1, so run 3 "
|
||||
f"would not recognise it ({first['description']!r} -> {second['description']!r})")
|
||||
|
||||
|
||||
def test_a_sentinel_APPEARING_MID_RUN_stops_a_stale_run_overwriting_it(tmp_path):
|
||||
"""Round-3 finding, and the only one in this series that failed toward SUCCESS.
|
||||
|
||||
Two runs overlap for the same sha — the regime this branch measured live (probe PR #722: the older
|
||||
run finished 20s after the newer one started). Run B catches a raced human BLOCKED and repairs to
|
||||
the sentinel. Run A is still in flight: its FIRST read predates all of it, so it classified
|
||||
`success`; its high-water mark was taken after the human row, so the post-write check stays silent;
|
||||
and the retarget fence sees nothing. Until this guard, A posted its stale `success` straight over
|
||||
the sentinel — burying the human rejection with no repair and no log.
|
||||
|
||||
The re-read recomputes `ex_repair`; the bug was that nothing downstream consulted it.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"),
|
||||
status_mode="sentinel-appears-on-read:2")
|
||||
assert r.returncode == 0, r.stderr
|
||||
assert posted is None, (
|
||||
"a stale run posted its exemption over a repair sentinel written mid-run, burying the human "
|
||||
f"verdict it records (posted={posted})")
|
||||
assert "repair sentinel was written" in r.stdout, (
|
||||
f"nothing posted, but not via the mid-run sentinel guard. Log:\n{r.stdout[-800:]}")
|
||||
|
||||
|
||||
def test_positive_control_a_sentinel_present_from_the_START_still_posts_pending(tmp_path):
|
||||
"""Without this, the guard above could be an unconditional abstain whenever a sentinel exists.
|
||||
|
||||
A sentinel present at the FIRST read is the ordinary repaired-head case: it must still POST
|
||||
`pending` carrying the sentinel forward (the fixed point), not abstain. That difference is exactly
|
||||
what makes the mid-run guard's condition exact rather than conservative.
|
||||
"""
|
||||
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 repaired head stopped posting entirely: {r.stdout[-800:]}"
|
||||
assert posted["state"] == "pending"
|
||||
assert posted["description"] == "Human verdict raced this exemption write — re-post the verdict"
|
||||
|
||||
|
||||
def test_a_PENDING_path_run_also_refuses_to_clobber_a_mid_run_sentinel(tmp_path):
|
||||
"""Round-4 finding: the guard's first form tested `state = success`, one branch too narrow.
|
||||
|
||||
A run can reach the POST on `pending` carrying the GENERIC description — most realistically after a
|
||||
transient enumeration failure — and a success-only guard waves it through. It then overwrites the
|
||||
sentinel with ordinary text, the next run sees no sentinel, re-derives, and posts `success`: the
|
||||
same buried human rejection, two steps instead of one.
|
||||
|
||||
Hence the rule is "never replace a sentinel with a non-sentinel", compared on the DESCRIPTION. That
|
||||
is strictly more general and exactly as precise, because the carry-forward branch guarantees a
|
||||
first-read sentinel already sets `desc` to the sentinel.
|
||||
"""
|
||||
posted, r = _run_classify(
|
||||
tmp_path,
|
||||
"#!/usr/bin/env bash\n" + DOCS_ONLY + "exit 1\n", # enumeration fails -> pending, generic desc
|
||||
status_mode="sentinel-appears-on-read:2")
|
||||
assert r.returncode == 0, r.stderr
|
||||
assert posted is None, (
|
||||
"a pending-path run overwrote a repair sentinel written mid-run with the generic description; "
|
||||
f"the next run would re-derive the exemption and bury the human verdict (posted={posted})")
|
||||
assert "repair sentinel was written" in r.stdout, (
|
||||
f"nothing posted, but not via the mid-run sentinel guard. Log:\n{r.stdout[-800:]}")
|
||||
|
||||
|
||||
def test_a_SENTINEL_landing_above_the_mark_also_triggers_the_repair(tmp_path):
|
||||
"""Round-5 finding: the post-write filter counted only HUMAN rows, and that is not sufficient.
|
||||
|
||||
With two overlapping runs A and B, the human BLOCKED can land BELOW A's high-water mark — so A
|
||||
cannot see it — while B masks it with an exemption `success` and only afterwards writes the
|
||||
sentinel. A then finds nothing human above its mark, does not repair, and posts its own `success`
|
||||
on top of the sentinel: a PERMANENT forged green over a human rejection, and the repair race
|
||||
failing toward success rather than pending, which the decision record explicitly promises it does
|
||||
not do.
|
||||
|
||||
A sentinel above the mark can only have been written by another run mid-flight — a pre-existing one
|
||||
would have been seen at the first read and forced the pending path — so counting it cannot
|
||||
false-fire, and it converges both runs on the fixed point.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="sentinel-after-post")
|
||||
assert r.returncode == 0, r.stderr
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 2, (
|
||||
"the run posted its exemption on top of a sentinel written by an overlapping run and did not "
|
||||
f"repair, leaving a human rejection permanently green. Posts: {seq}\n{r.stdout[-800:]}")
|
||||
assert seq[0]["state"] == "success"
|
||||
assert seq[1]["state"] == "pending"
|
||||
assert seq[1]["description"] == "Human verdict raced this exemption write — re-post the verdict", (
|
||||
f"repaired, but not to the sentinel, so the fixed point is broken: {seq[1]}")
|
||||
|
||||
Reference in New Issue
Block a user