ci(release): the milestone gate reads 200 PRs and vouches for the rest by marevol · Pull Request #362 · codelibs/recotem

marevol · 2026-09-06T12:32:38Z

What is broken

.github/scripts/check-milestone-landed.sh:162-166 asks for the milestone's
merged PRs with gh pr list --state merged --search "milestone:${MILESTONE}" --limit 200 --json number,title,mergeCommit.

--limit is a cap, not a request. gh returns at most that many rows and
says nothing about the ones it dropped, so a milestone larger than 200 has its
200 most recent PRs checked and the script then prints, at line 342:

OK: every merged PR in milestone 'X' is an ancestor of <sha>

about a set nobody read. This script's header states the doctrine it violates
here — "A gate that quietly does nothing … is worse than no gate: it reports
success for a question it never asked" — and the absent-milestone branch is
fatal for precisely that reason. The truncation path has no such guard.

Measured

gh truncates silently. Asked for 5 of a milestone that has 151, against the
real API — stdout 5, and stderr was captured to a file that came back zero
bytes. No marker in the JSON either.

The search API has its own ceiling, which gh also does not report. Asked for
1100 against a large public repository with more merged PRs than that, the
answer was exactly 1000.

How close this repository is to the old cap today — asking for the same
milestone with a limit of 1000 returns 151. That is 151 of 200: 49 PRs of
headroom, on a milestone that grew by 11 in a single day.

The change

The limit becomes 1000 — the point where gh pr list --search truncates
regardless, so the guard fires exactly when truncation starts rather than at a
number of our own choosing. A refusal is placed before the ancestry loop: a
truncated list cannot be made trustworthy by examining the part of it that
arrived. It counts the rows that came back and calls fail when that count
reaches the limit.

The message says that raising the number does not help, because the ceiling is
the API's, and gives the date-sliced manual query instead.

Behaviour on this repository is unchanged — the real gate still passes, with
Checked 151 merged PR(s), the recorded re-land of #245 by #256 cleared, and
OK: every merged PR in milestone '2.1.0' is an ancestor of 031ef69…, rc 0.

Positive control

The two new tests are each other's control. The limit is read out of the
script rather than restated in the test, so raising it there without moving
the guard fails these instead of leaving them measuring a boundary that has
moved.

rows fed to the script	expected	observed
a full page (limit)	refused, no success line	rc 1, "the maximum this query can return"
one short of the limit	checked normally	rc 0, "Checked 999 merged PR(s)"

Without the second row the first would pass for "many rows" rather than for the
boundary, and an off-by-one (-gt) would refuse every large but complete
milestone — blocking a release for a truncation that did not happen.

The rows carry none as the merge commit, which the loop skips without
spawning git, so both tests measure the guard rather than the loop.

Mutation matrix

Four directions on the single guard call site, each run against the whole
test file, with __pycache__ cleared between runs and bash -n checked after
each edit:

direction	mutation	result
baseline	—	17 passed
delete	the `if … fail` block removed	1 failed (full-page test)
move	guard kept verbatim, its count no longer derived from the fetched list	1 failed (full-page test)
negate	`-ge` → `-lt`, every token kept	11 failed
widen	`-ge` → `-gt` (the off-by-one)	1 failed (full-page test)

No surviving cells.

What this matrix cannot reach: the limit value itself. The tests drive the
script through a fake gh that ignores --limit, and the test reads whatever
limit the script declares — so setting it back to 200 would keep the suite green
with the guard correctly watching 200. The claim that 1000 is the API ceiling is
verified against the live API above and is not, and cannot be, pinned offline.

Verification

uv run pytest tests -q → 3064 passed, 4 deselected
uv run ruff check src tests → All checks passed!
uv run ruff format --check src tests → 177 files already formatted
bash -n .github/scripts/check-milestone-landed.sh → rc 0

Cost: the boundary control adds about 4s (999 iterations of the reporting loop,
which spawns three processes per row inside sanitize_title). The guard itself
spawns no new subprocess.

Why 2.2.0

Nothing a 2.1.0 user receives is wrong, and nothing about cutting 2.1.0
changes: the milestone holds 151 merged PRs, under the old cap, and the gate
returns the same verdict before and after this change. This is release
infrastructure hardening against a milestone that has not happened yet.

Overlap

#291 also modifies check-milestone-landed.sh (reverts, no-op merges, and the
HEAD disclaimer). It does not touch the query or its limit, so the two are
independent in intent but will touch the same file — whichever lands second
will want a rebase.

`check-milestone-landed.sh` asks `gh pr list --search "milestone:X"` for at most 200 rows. `--limit` is a cap, not a request: `gh` returns that many and says nothing about the ones it dropped. A milestone larger than the cap has its most recent 200 PRs checked, and the script then prints OK: every merged PR in milestone 'X' is an ancestor of <sha> about a set nobody read. That is the vacuous-check shape this script's own header refuses everywhere else -- the absent-milestone case is fatal for exactly this reason. Raise the limit to 1000, where `gh pr list --search` truncates anyway, and refuse a full page instead of checking a prefix of it.

marevol · 2026-09-09T13:41:31Z

Re-classifying this to milestone 2.1.0. It was correctly 2.2.0 when filed; the tree has since crossed the threshold it describes.

Measured on 641485ff today:

gh pr list --state merged --search "milestone:2.1.0" --limit 200   -> 199
gh pr list --state merged --search "milestone:2.1.0" --limit 1000  -> 199

199 merged PRs against a hard cap of 200. This PR's own explanatory comment says "Milestone 2.1.0 carries 151 merged PRs today, so the old cap was 49 away from silently under-reporting" — that was true when written and is now stale by 48. The margin is one PR. Worth re-measuring that sentence before merge.

The failure direction is open, confirmed with a single-call-site mutation on a copy of the script (--limit 200 changed to --limit 5, nothing else):

ARM A  unmodified   "Checked 199 merged PR(s)"  "OK: every merged PR ... is an ancestor"  rc=0
ARM B  --limit 5    "Checked 5 merged PR(s)"    "OK: every merged PR ... is an ancestor"  rc=0

Arm B read 5 of 199 and still vouched for the whole milestone. Past the cap the gate reports success for a question it never asked — the vacuous-check shape the script's own header refuses everywhere else.

Why this becomes a 2.1.0 blocker rather than a 2.2.0 cleanup: every PR added to milestone 2.1.0 before the tag raises the count. At 201 the gate silently drops one. And publish.yml has run exactly twice ever — both before check-milestone-landed.sh existed — so the first real execution of this gate will be the v2.1.0 tag push, on a milestone it under-reads. A gate whose first run is also its first truncated run cannot be distinguished from one that works.

Landing this resolves it at any round size: it raises the cap to 1000 and, more importantly, refuses a full page rather than checking a prefix. Merging it into milestone 2.1.0 makes it the 200th merged PR — exactly at the old cap, so still complete — and removes the ceiling for everything after.

Recommend landing this before the tag.

… at startup (#425) ## What this fixes `_RecipeWatchState.recipe` was the body parsed when the process started. Nothing refreshed it — the only two assignments to it sat inside YAML-error-recovery branches — so for a recipe that always parses cleanly it stayed the startup body for the life of the process. #417 fixed one reader of that field (the recipe-drift warning) by adding a second field, `latest_recipe`, refreshed on every rescan. **Three more readers were left on the startup body.** This PR fixes all three by making the one field mean what its name says: *the recipe as it is on disk now*. ### 1. `item_metadata` — a retrain silently produced a mixed response `_build_entry` joins the response against the table the recipe names. Repoint `item_metadata.path` **and retrain**, and the new artifact hot-swaps in while the join still reads the table the *startup* body named. Nothing warns, because the artifact and the recipe on disk now legitimately agree — the drift warning is silent by design in exactly this case. **Which body it reads now, and why that one.** The watcher cannot reconstruct the body an artifact was trained from; the header carries only that body's `recipe_hash`, not the body. So "the trained-from body" is not an option that exists — the startup body was only ever a proxy for it, and it stops being one at the first hot-swap: after an edit and a retrain, the startup body is neither the body on disk nor the body the new artifact was trained from. The body on disk is the only one the watcher can name, and it is already the one `app.py` uses on the startup load path, so a hot-swap and a restart now build the same entry from the same bytes. That parity is the same reason the recipe-hash gate was mirrored onto both load paths in #270. When the two bodies do differ — an edit with no retrain — the drift warning is already firing, and this PR corrects that warning's docstring, which claimed the response is "of a piece" while it fires. It is not: what comes out of the artifact is the older recipe, while `item_metadata` is read from the recipe on disk at load time. The docstring now says which half is which. ### 2. `output.path` — an edit was ignored until restart `state.artifact_path` was taken from `recipe.output.path` once, at discovery. An edited output path was ignored for the life of the process, and silently: the drift warning is emitted from the load path, and no load ever happens at the new location, so nothing anywhere says the watcher is polling a file the recipe no longer names. The rescan now follows it. A re-point drops everything the watcher remembers about the previous file — change marker, payload digest, `.sha256` sidecar contents, load backoff, repeat-suppression signatures — because none of it says anything about the new path. Dropping `last_sha256` matters in particular: a new path holding byte-identical content would otherwise take the "unchanged bytes" short-circuit and never rebuild the entry, leaving the served entry naming the old path. One INFO line, `recipe_output_path_changed`, carries `previous_path` and `path`. **A path that does not exist yet** — the ordinary case, an operator who edited the recipe and has not retrained — degrades rather than evicting. The model in the registry keeps serving, the next poll records "artifact missing or unreadable" against the entry and `/v1/health/details` goes degraded. That is the same M-2 contract every other missing artifact already gets, and there is a test for it. ### 3. The `.sha256` sidecar suppression latch could never be released Three consecutive unreadable sidecar reads set `sidecar_unsupported` so a broken sidecar cannot drive a full reload on every tick (m7). The documented escape — clear it when the recipe changes (C4) — was reached through: ```python getattr(state.recipe, "_yaml_path", None) or getattr(state.recipe, "yaml_path", None) ``` `Recipe` is a pydantic model with `extra="forbid"` and no private attributes, so it has never carried either name. That chain has answered `None` for every recipe that has ever existed, the guard it feeds (`yaml_mtime is None or ...`) always declined, and the recovery has never run once in a live server. The latch was permanent, and the sidecar is a real backstop: it is what catches a change the `(mtime, size)` marker cannot discriminate. **Chosen fix: make the recovery work, rather than delete it.** Deleting the dead chain would have been a pure no-op — the latch is already permanent in practice — and would have made "no recovery at all" the design. Instead the clearing moves to `_scan_recipes_dir`, where the watcher *already* decides that the YAML changed (`_yaml_mtime_cache` miss). That is a signal the watcher genuinely has, it removes a second, independently-drifting mtime comparison, and it is now load-bearing for a reason the old placement did not have: with fix 2 above, a recipe edit can move `output.path`, and the sidecar path is `artifact_path + ".sha256"`, so an edit really can move the sidecar out from under a latch set on the old one. The three dead `getattr` chains and the `sidecar_unsupported_at_mtime` field they compared against are removed. **The test for this recovery passed, and proved nothing.** `test_sidecar_unsupported_clears_on_yaml_mtime_change` built the state around a `MagicMock` with `recipe._yaml_path = yaml_path` — the only kind of object in existence for which the code worked. It is replaced by `test_sidecar_unsupported_is_never_cleared_by_the_sidecar_check_itself`, which uses a real `Recipe` and pins the other half of the split, so a reintroduced mtime probe in `_check_sidecar_changed` fails there. ## Pre-existing since 2.0.0 — evidence None of these is a regression from #415, #417, #418 or #362. | defect | evidence | |---|---| | `item_metadata` from the startup body | `_build_entry(name, state.recipe, ...)` and `_RecipeWatchState` come from `8395a811` (2026-05-14, the 2.0 rewrite); `git merge-base --is-ancestor 8395a81 v2.0.0` succeeds | | frozen `output.path` | same commit introduced `_RecipeWatchState.artifact_path` | | dead `sidecar_unsupported` recovery | `git show v2.0.0:src/recotem/serving/watcher.py \| grep -c '_yaml_path", None'` → **3**, the same count as on `main`; the chains are at lines 1245 / 1277 / 1338 of the 2.0.0 file, and `git show v2.0.0:src/recotem/recipe/models.py` has no `yaml_path` | A note on the dating, because the obvious command misleads: `git log -S '_yaml_path' -- src/recotem/serving/watcher.py` returns `f15ebbd6` (2026-09-03, #167), which is not in `v2.0.0` and reads like "new in 2.1.0". `-S` reports commits that changed the *count* of the string, and #167's only `_yaml_path` line is a docstring mention of the unrelated `_yaml_path_to_name` map. The `getattr` chains themselves are untouched since 2.0.0. ## Tests Each behaviour has a test that fails on unmodified `main` **on its assertion**, not on an import or a signature error. New file `tests/unit/test_watcher_live_recipe_body.py`. On `main` (696d55c), before the fix — 4 failed, 1 passed: ``` E AssertionError: the artifact that just swapped in was trained FROM the edited recipe, yet the metadata join still uses the table the STARTUP body named. The response is the new model on the old table. Titles: ['OLD_TITLE'] E assert ['OLD_TITLE'] == ['NEW_TITLE'] E AssertionError: output.path was repointed under a running watcher and the artifact at the new path never loaded: the watcher is still polling '.../old.recotem', the path captured at discovery. E AssertionError: output.path now names a file that does not exist and nothing was recorded against the entry: the watcher is still polling the old path ('.../old.recotem') and reporting success for it. E AssertionError: the recipe YAML was re-parsed and the suppression was not cleared: the documented recovery reads Recipe._yaml_path, which does not exist, so the latch is permanent and the sidecar backstop is off until restart. E assert True is False ``` The one that passes on `main` is the control, `test_item_metadata_is_not_reloaded_without_a_hot_swap`: an edit with *no* retrain must leave the old model and the old table in place. Without it, "the join followed the edit" could as easily mean "the join re-reads every tick", which would be a different behaviour. After the fix: `5 passed`. ### Each fix is load-bearing — single-call-site mutations Each mutation rewrites one expression and leaves the statement in place (never a deletion, so a presence check cannot score as a behaviour check): | mutation | change | result | |---|---|---| | M1 | `existing_state.recipe = recipe` → assign the field back to itself | `test_item_metadata_follows_the_edited_recipe_after_a_retrain` **and both #417 drift tests** fail — one site now carries both fixes | | M2 | `_repoint_artifact(..., recipe.output.path)` → pass `existing_state.artifact_path`, so the call still runs and is a no-op | both `output.path` tests fail | | M3 | `existing_state.sidecar_unsupported = False` → assign the field back to itself | the sidecar test fails | M1 failing both #417 tests is the check that removing `latest_recipe` did not quietly drop #417's fix. ### Suite `main` baseline: 3157 passed, 1 skipped, 4 deselected. This branch: **3162 passed, 1 skipped, 4 deselected** (+5 = the new file). `ruff check src tests` and `ruff format --check src tests` clean. `uv run bash tests/e2e/run.sh` passes. ### Concurrent watcher work `fix/watcher-pointer-content-marker` (the pointer-content change marker) is in flight against the same file. The two touch disjoint hunks, so this opens against `main` rather than stacking. Merged together locally the tree is clean and green: **3165 passed, 1 skipped, 4 deselected**, ruff clean — no conflict and no cross-failure. That branch narrows, but does not remove, the reason the sidecar backstop matters: it folds a digest into the marker only for pointer-sized files, so `versioning: always_overwrite` (a real artifact at the watched path) still relies on `(mtime, size)`, and the sidecar is still its only backstop. ## Deliberately not changed - **An `item_metadata` edit alone does not trigger a reload.** Metadata is read where the model is built, so an edit with no retrain leaves the old model *and* the old table — which is coherent, and is what the drift warning describes. Making a metadata edit its own hot-swap trigger is a new trigger, not this fix. - **`ModelEntry.artifact_path` is not updated eagerly on a re-point.** That field documents "the path last successfully loaded", which stays true; the `recipe_output_path_changed` log line ties the old and new paths together. - **`_stat_marker` is untouched** — that is the other branch's lane. ## Docs follow-up (separate repo) `https://recotem.org/2.1/docs/operations.html` needs two edits that are not in this repo: - the "Watcher and registry semantics" bullet list should say that an edited `output.path` is now followed at the next rescan, and that `item_metadata` is re-read from the recipe on disk at the next hot-swap; - the watcher event table should gain `recipe_output_path_changed` (INFO). Also pre-existing and unrelated to this PR, but worth correcting while that page is open: it states that during a drift warning "`/v1/recipes/{name}` reports the *current* recipe's `algorithms`, `metric` and `cutoff`". It reports the *artifact's* — `routes.py` never reads a `Recipe` object; every field on that endpoint comes from `entry.header`.

…o-op, and calls HEAD the release (#291) This PR closes two of the four ways a milestone's "MERGED" claim goes wrong, and states the other two in the gate's own output rather than leaving a reader to infer completeness. The gate asks an **ancestry** question; every failure is about **content**, and the two coincide only in the first row. | class | instance | ancestry alone | this PR | |---|---|---|---| | merged, never reached main | #245 | **catches** | unchanged | | reached main, then reverted | #259, #261 | blind — reports LANDED | **refused** | | merges cleanly, moves no bytes | #277 | blind — and **vouches** for it | **refused** | | removed later with no revert trailer | — | blind | **declared unchecked** | | HEAD is main plus a smuggled commit | R9-P3 | blind — **names the smuggled SHA as verified** | **disclaimed, owner named** | The last row is not a content question at all: it is the gate answering a true question about the wrong tree. It is not enforced here because `check-release-tag.sh` owns it and this script is a documented local pre-flight run from a branch — but **that owner's implementation is not on main**: #259 added it, #276 reverted it, #277's re-land is one of the no-ops this PR now refuses. The gate says so out loud. --- # Commit 1 — reverts `check-milestone-landed.sh` asks one question: is this PR's merge commit an ancestor of the tree being released? That detects a PR that NEVER REACHED main -- #245, stranded on a branch whose base was squash-merged first. It is structurally blind to the other way the same claim goes wrong: a PR that REACHED MAIN AND WAS TAKEN BACK OUT. A reverted PR's merge commit stays an ancestor forever, so the ancestry test reports it landed while the tree has none of its lines. Both end in the identical false statement -- the milestone says the release contains a change the tree does not have -- and only one of them was checked. **This repository contains a live instance right now.** PR #276 reverted #259 (`90c96f0`) and #261 (`d0118fc`): git merge-base --is-ancestor 90c96f0 HEAD -> exit 0 "LANDED" git merge-base --is-ancestor d0118fc HEAD -> exit 0 "LANDED" grep -c 'is-ancestor|not on main' check-release-tag.sh at 90c96f0: 3 in the tree: 0 grep -c mariadb src/recotem/training/search.py at d0118fc: 5 in the tree: 0 Not a 2.1.0 problem: both carry milestone 2.2.0, so the 2.1.0 check never looks at them. But **on the day 2.2.0 is cut, this gate would green-light a release missing exactly the content it was written to catch.** That is #245's failure one turn of the crank later, and the reason to fix it before the milestone that contains the instance comes up rather than after. ## The check Each PR that passes the ancestry test is now also checked for a revert that still stands, keyed on the canonical `This reverts commit <40-hex>` trailer -- a record `git revert` and GitHub's own Revert button both write, so it is produced by the tooling rather than by a convention anyone has to remember. `--fixed-strings` keeps the SHA out of the regex engine. Two things it deliberately does NOT do: * **A revert of a revert is not reported.** "We put it back" leaves the content in the tree, and a gate that fails a healthy release is a gate that gets switched off. Depth is one un-revert, documented at the function: anything deeper is rare enough that the existing waiver is a better answer than more recursion, and a loud wrong answer an operator clears by hand beats a silent one. * **A revert is not a stranding and does not get the stranding remedy.** Being told to cherry-pick a change someone deliberately backed out is the wrong instruction. The message offers both real options and names the usual one first: re-land it and record the replacement, or move the PR off the milestone -- which for a deliberate revert is the honest record and clears the check on its own. The `relanded-prs.tsv` waiver now serves both failure modes, and gained the matching condition: a replacement clears the original only if its own merge commit is an ancestor **and has not itself been reverted**. Without that second half a reverted re-land would keep clearing the original, so the waiver would outlive the change it vouches for. The file's header says all of this, and says not to add a row for a deliberate revert. ## Guard `tests/unit/test_check_milestone_landed.py`, 15 -> 24 tests, driving the real script against synthetic repositories with a fake `gh`, as the existing ones do. `test_ancestry_alone_cannot_see_a_revert` is the positive control on the *defect*: it asserts a reverted commit is still an ancestor and that the content really is gone, so nothing below can pass for the wrong reason. Two run against this repository's own history rather than a fixture, using #259/#261/#276 -- the shape as someone really made it, not one written to be caught. They skip rather than fail where the history is absent (a shallow clone), because "cannot see" and "not present" are different states of knowledge. Mutation-tested, each mutation syntax-checked before the run: revert detection disabled (the shipped, blind behaviour) -> 5 failed waiver stops checking whether the replacement was reverted-> 1 failed depth-1 un-revert dropped (revert-of-a-revert reports) -> 1 failed trailer match dropped (any commit in range counts) -> 8 failed restored -> 24 passed The first row is the one that matters: it is the behaviour on main today, and it takes the live-history test with it. 2891 passed (+9), 4 deselected; ruff check and ruff format --check clean; `bash -n` clean. --- # Commit 2 — no-op merges, and the declared boundary P3 found a third class, and it is sharper than the revert one: a PR can merge cleanly and change nothing, and after merging the gate does not merely fail to notice -- it ASSERTS the milestone is complete on the strength of it. class instance ancestry alone --------------------------------- --------- ------------------------- merged, never reached main #245 catches reached main, then reverted #259 #261 blind: reports LANDED merges cleanly, moves no bytes #277 blind: VOUCHES for it PR #277 is titled "re-land of #259" and is stacked on #276, which reverted #259. Its branch therefore reverts #259, reverts #261, then restores #259 -- and the first and third cancel. Measured by merging it: git merge --no-ff pr/277 -> "Merge made by the 'ort' strategy" git diff --stat origin/main HEAD-> (empty) check-release-tag.sh -> 305 lines, not #259's 555 'is-ancestor|not on main' hits -> 0, not 3 A PR named for re-landing #259 restores none of it, merges cleanly, and becomes an ancestor. It also opens a hole in the revert check in the first commit of this PR, which is why the two ship together: record that no-op in relanded-prs.tsv as the re-land of #259 and the waiver clears #259 on the strength of a PR that restored nothing. The gate would not just miss the gap, it would certify it closed. ## What this adds `contributes_nothing` -- the commit's diff against its first parent is empty -- applied in two places: the main loop (a milestone PR that carried nothing is reported, with its own remedy) and the waiver (a no-op can never clear the PR it is recorded against). First parent is the right comparison for both shapes here: for a squash merge it is "what did this add to main", and for a true merge it is the same question asked of the mainline. **False-positive rate, counted rather than predicted.** Over all 329 first-parent commits on main -- 4 of them true merges, 1 a root -- the number with an empty first-parent diff is **ZERO**. A gate that cries wolf on ordinary refactoring is a gate someone switches off, so this is pinned by a test that re-counts it against the live history and fails if the number ever moves. ## What this deliberately does NOT add, and why the output now says so A fourth shape exists: a later commit removes the change with no revert trailer -- a rewrite, a refactor, a hand-edit. That is a CONTENT question and content is materially harder than reachability. The cheap forms do not survive contact with a real repository: re-applying each PR's diff to see whether it is a no-op flags every file another PR has legitimately touched since, and hunk-grepping flags every reformat. It is not attempted. The two classes taken here were taken precisely because they are exact -- "is there a revert trailer naming this commit" and "is this commit's diff empty" are both decidable with no heuristic and no threshold. So the success message stops implying a completeness the gate does not have. It was a bare "is an ancestor of <sha>", which reads as "the milestone is complete". It now enumerates the three questions asked and names the one that is not, in the gate's own output where a reader will actually meet it: OK: every merged PR in milestone '2.1.0' is in the tree at <sha>. Checked, per PR: the merge commit is an ancestor; no revert of it still stands; the merge is not an empty no-op. NOT checked: whether a later commit removed the change WITHOUT a 'This reverts commit' trailer -- a rewrite or a refactor that drops a change silently is invisible here. [...] A gate that announces its own boundary is worth more than one a reader infers completeness from. ## Guard `tests/unit/test_check_milestone_landed.py`, 24 -> 30 tests. One builds #277's actual shape (revert then restore, merged) rather than asserting it, with fixture self-checks that the merge really is both a no-op and an ancestor. One is the negative control -- an ordinary change must stay green. One pins the false-positive count against the live history. One asserts the success message declares what it does not check. Mutation-tested, each mutant `bash -n`-checked first: M1 revert detection disabled (behaviour on main today) -> 6 failed M2 waiver stops checking the replacement for a revert -> 1 failed M3 depth-1 un-revert dropped -> 1 failed M4 revert trailer match dropped -> 9 failed M5 no-op detection disabled -> 2 failed M6 waiver stops checking the replacement for a no-op -> 1 failed M7 emptiness test inverted to always-true (over-broad) -> 9 failed M8 success message stops declaring what is NOT checked -> 1 failed restored -> 30 passed M7 matters as much as M5: it shows the check is bounded in both directions, so a future edit cannot quietly turn it into something that flags real work. 2897 passed (+6), 4 deselected; ruff check and ruff format --check clean; `bash -n` clean. --- # Commit 3 — HEAD is not the release tree R9-P3 found a fourth class, and it is not about a PR's content being missing -- it is this gate answering a true question about the wrong tree. Every question here is asked of `git rev-parse HEAD`. "Every milestone PR is an ancestor of HEAD" stays true when HEAD is main plus something smuggled on top: the extra commit is not any PR's merge commit, and nothing here looks for commits that no PR explains. P3 demonstrated it on a real off-main commit carrying a marker -- the script exited 0 and printed the smuggled SHA in its own success line as though it were the release. Reproduced independently here before writing this, on a commit built on top of 7871f9f and not on main: smuggled HEAD 9ffec607d8b5 is it on main? NO milestone PRs checked 13 milestone PRs NOT ancestors of it 0 All thirteen pass. The claim the success line makes is true, and much narrower than it reads. ## Why the rule is not enforced here 1. **It belongs to the tag guard.** `check-release-tag.sh` runs on the tag, where HEAD is always the tagged commit, and #259 implemented it there: shallow-clone refusal first (a shallow clone answers ancestry *wrongly* rather than failing), then `origin/main` else `refs/heads/main`, then `merge-base --is-ancestor HEAD <main>`. Two owners for one rule is how a rule ends up with none. 2. **This script is documented as a local pre-flight**, run from a branch before tagging -- see Usage in the header. A hard "HEAD must be on main" check would make its own documented workflow fail. So this does not add the check. It stops the output implying one. ## What it does add The success line was already enumerating what it checks and one thing it does not; it now names the second, and names the owner: NOT checked, 2 of 2: that <sha> is on main. Everything above is asked of HEAD, and 'every milestone PR is an ancestor of HEAD' stays true when HEAD is main plus a commit no PR explains. This line is not a statement that the release is main. check-release-tag.sh owns that rule -- and it is NOT on main today: #259 added it, #276 reverted it, and #277's re-land is an empty no-op. It returns with #277's rebuild. The last sentence is the part that matters and is why this is not merely a docstring. **The protection for P3's class existed and was reverted.** #259 added it, #276 backed it out, and #277's re-land is one of the no-ops this same PR now refuses -- so today NEITHER script checks that the released commit is on main. Verified: the current `check-release-tag.sh` makes no ancestry call at all, and #259's version has the check at line 455. Naming an open hole in the gate's own output beats leaving a reader to infer completeness from a success message, which is the whole argument of the commit before this one. ## Guard One test, `test_success_message_does_not_present_head_as_main`, asserting both the disclaimer and the owner's name -- the second because a disclaimer that does not say who *does* check reads as "nobody does" rather than "someone else does". **And a correction to the test added in the previous commit.** Numbering the disclaimers "1 of 2" and "2 of 2" silently weakened `test_success_message_states_what_is_not_checked`, which asserted the shared substring `"NOT checked"`: with two disclaimers present, deleting the first left the second satisfying the assertion. Caught by the mutation matrix -- M8 went from 1 failed to 31 passed -- not by reading. The test now asserts each disclaimer by its distinguishing marker, and M8 was widened to delete the whole block rather than one line of it. This is the second species of green-but-empty guard, introduced and caught inside one PR. Mutation matrix, all nine dead, each mutant `bash -n`-checked first: M1 revert detection disabled (behaviour on main today) -> 6 failed M2 waiver stops checking the replacement for a revert -> 1 failed M3 depth-1 un-revert dropped -> 1 failed M4 revert trailer match dropped -> 10 failed M5 no-op detection disabled -> 2 failed M6 waiver stops checking the replacement for a no-op -> 1 failed M7 emptiness test inverted to always-true (over-broad) -> 10 failed M8 the "silently removed" disclaimer deleted -> 1 failed M9 the "HEAD is not main" disclaimer deleted -> 2 failed restored -> 31 passed 2898 passed (+1), 4 deselected; ruff check and ruff format --check clean; `bash -n` clean. --- # Update — brought onto current main This branch was written when main was 100 commits younger. Rebasing it changed three things about what it says, and one thing about what the repository has to record. ### `.github/relanded-prs.tsv` gains two rows, because the new check fires Run against milestone 2.2.0 on the rebased branch, the gate refuses the release and names the live pair: ::error::2 PR(s) in milestone '2.2.0' reached main and were REVERTED, so v2.2.0 would publish without them: #261 d0118fc reverted by 504905c #259 90c96f0 reverted by 504905c That is the finding working, not a regression. Both changes did come back: #259 as #297 and #261 as #280, each merged onto current main. The rows record that, and the waiver re-checks each replacement itself — ancestor, not reverted, not an empty diff — rather than taking the row's word for it: 2 PR(s) cleared by a recorded re-land: #261 -> re-landed by #280 (b231522) #259 -> re-landed by #297 (774745e) OK: every merged PR in milestone '2.2.0' is in the tree at <sha>. Without the rows, 2.2.0 would have been green-lit as containing two changes whose every line had been removed and restored under other numbers. ### #277 is closed, not merged The header and the `contributes_nothing` docstring described it in the present tense as a no-op that merges cleanly and carries nothing — true when this was written, when #277 was open. It was closed unmerged, so the no-op class now has no merged instance in this history. The check stays, because nothing but somebody noticing stopped that merge, and both comments now say which of the two it is instead of pointing at a PR that is not live. ### The "329 first-parent commits" figure is dropped, both places Same reason #424 gave for dropping the milestone count it replaced: main only grows, so a number written into a comment is exact the day it is typed and misleading after the next merge. The claim it supported — zero commits in this history have an empty first-parent diff — is re-measured on every run of `test_no_commit_in_this_repositorys_history_is_a_false_positive`, which is the copy that cannot go stale. ### Conflict resolution `tests/unit/test_check_milestone_landed.py` conflicted at the end of the file: this branch appended its 16 new tests there and #362 appended two of its own (`test_a_full_page_of_results_is_refused_not_checked_partially`, `test_one_short_of_the_cap_is_still_checked`). Both blocks kept; no test from either side dropped or edited. `check-milestone-landed.sh` auto-merged — #424 touched only the `PR_LIMIT` comment, which this branch does not. ### Verified on the merged tree pytest tests 3218 passed, 1 skipped, 4 deselected ruff check / format clean bash -n clean milestone gate, 2.2.0 exit 0, both re-lands reported by name

marevol modified the milestones: 2.2.0, 2.1.0 Sep 6, 2026

marevol self-assigned this Sep 9, 2026

marevol merged commit 696d55c into main Sep 9, 2026
8 checks passed

marevol mentioned this pull request Sep 9, 2026

fix(serve): the watcher read three things out of the recipe it parsed at startup #425

Merged

marevol mentioned this pull request Sep 10, 2026

ci(release): the milestone gate is blind to a revert, vouches for a no-op, and calls HEAD the release #291

Merged

Navigation Menu

ci(release): the milestone gate reads 200 PRs and vouches for the rest - #362

ci(release): the milestone gate reads 200 PRs and vouches for the rest#362
marevol merged 1 commit into
mainfrom
r12p1-milestone-gate-silent-truncation

marevol commented Sep 6, 2026

Uh oh!

marevol commented Sep 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

marevol commented Sep 6, 2026

What is broken

Measured

The change

Positive control

Mutation matrix

Verification

Why 2.2.0

Overlap

Uh oh!

marevol commented Sep 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant