premerge_check: run the static gates on the MERGE RESULT, before the merge - #2035
Conversation
… branch pr-validate.yml has exactly one trigger, `pull_request_target:`. There is no `push: branches: [main]`. Nothing builds a ROM after a merge, and every PR is judged against its own base -- so two PRs each green against base X can combine into a red main. That has happened twice in four days (#1987, #1990). This runs the toolchain-free gates on `git merge-tree --write-tree origin/main <head>` and diffs the verdicts against origin/main. It exits non-zero ONLY when a gate is green on the base and red on the merge result. A gate already red on the base is reported as `pre-existing` and can never fail the run: a tool that blames the wrong PR trains people to ignore it. Gates, all verified toolchain-free by reading them: converted-ratchet, dead-references, duplicate-sources, header-offsets, layout-check, src-tu-refs, and source-coverage as a byte-set delta. rombuild.py and eligible.py are excluded by construction -- build/ is shared between worktrees. Advisory. Nothing in .github/workflows/ runs this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WdCK1xgrJdiJzPCh3bAJfQ
tool-tests.yml enumerates its modules on purpose -- "an explicit list makes adding a module a reviewed decision" -- so a new suite has to be added by hand or it never runs. This adds the 47 tests for premerge_check.py, in the alphabetical position and with the annotation the header block asks for. The suite qualifies on the header's own terms: unittest.TestCase throughout, so `python -m unittest` really collects it (it is not pytest-style), and no bare-`return` toolchain guard -- the single test that needs a git binary calls self.skipTest and says so. The timing note now separates the measured 312 from the 47 this adds rather than restating 5.3s for a total that was never timed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WdCK1xgrJdiJzPCh3bAJfQ
✅ PR validation — Passednoverify: no source/build-data changes in this PR Each changed |
|
Dogfooded against itself, at base exit 0. |
Gatekeeper review — MERGE AFTER FIXES (three blocking)I put this through an adversarial pass rather than a read, because a tool whose whole job is to say "safe to merge" is one whose false negatives are invisible by construction. Summary: the tool is real, it is safe, and it works — I reproduced its headline claim against live #2001 on the current base today. It also violates its own stated anti-hollow principle for one of its seven gates, and I watched it print Let me lead with the good part, because it is the reason I want this landed rather than reworked. Live run against #2001 at base Correct, actionable, and it is a defect no existing check can see — #2001 is green on its own PR and green on main, red only on the tree that does not exist yet. Conflict goes to exit 3. A deleted gate tool goes ERROR to REGRESSION to exit 1. Those paths fail closed. Safety is clean: no The test module is not hollow — 10 BlockingF1 — Nothing was printed "above" — the output was thrown away. This is the one that has to change, because Realistic trigger, and it is your own motivating case: two PRs editing F2 — the coverage gate compares byte counts; CI compares the byte set. Observed: relocating one CI would report 71,984 B handed back with a blame line. And this is merge-shaped: two PRs editing the same module's delinks.txt merge cleanly at text level and can trade ranges. So the docstring's To be explicit: your TU-promotion false-positive fix was correct and well-measured. Dropping This is a family this project keeps hitting — the ROM-data metric that counted records, the coverage ratchet whose F3 — It matters because langmode is a ratchet against a banked baseline — precisely this tool's target class, and one with a documented root-baseline cascade hazard. Add it, or add it to the excluded list with a reason. One caveat to handle in the same edit: when the root override is absent CI falls back to Strongly recommendedF7 — F8 — the base ref is never refreshed. An export-size floor. There is no equivalent of Worth fixing, not blocking
Two notes, not change requestsI owe you a correction. I have been asserting that State the inherent limit in the docstring. The merge tree's own gate scripts are what run, so a PR that weakens a gate shows pass/pass. That is the right design after #1994 (a PR now tests its own tool change) — but it is a real blind spot and should be said out loud rather than discovered. One coverage gap worth closing while you are in here: Fix F1, F2 and F3 and I will merge. This is a genuinely valuable tool and I want it in front of the TU queue rather than behind it — it is the only thing in the repo that evaluates a merge result, and it has already earned its place by finding #2001. Addendum — a second live run, on #2023Ran it against the live TU-promotion PR that is next in my merge queue, twice, same merge tree That is the exact case the docstring's measurement narrative at So across two live targets the tool caught the one real regression and did not false-alarm on either TU promotion. That is the behaviour I need from it. F14 — a hard kill leaks a 12k-file export. Each ~12,700 files. With the default One thing to know before wiring this into automation: |
Three blocking defects from review, plus the cheap ones flagged alongside them. F1 source-coverage failed OPEN. coverage_metrics() returned None on OSError, timeout, non-zero exit and unparseable JSON alike, DISCARDED the child's output, and the delta of two Nones was []; the run printed "outcome": "clean", "exit": 0, "coverage": null with nothing saying the gate had not run. Coverage is now a row like any other, through classify(), with the child's stdout+stderr captured and printed where the row is, and coverageMeasured/coverageStatus in --json. F2 the gate compared byte COUNTS; CI compares the byte SET. Measured: relocating one .rodata range in config/arm9/delinks.txt by +0x400000 hands back 71,984 B with currentBytes and every bySection total IDENTICAL. It now runs the merge tree's own source_coverage.py --check --json --base <base> --ref <merge tree> -- the same comparison, the same waivers file, no second interval implementation -- with a base-vs-base self-check so a source_coverage.py broken on main reads as pre-existing rather than as this PR's fault. entries/modules stay INFO. F3 langmode_audit.py --check ran in CI and appeared in neither list. It is a gate now; a tree without the root langmode-baseline.json is an ERROR, because CI's fallback is _chaos_data/, which is untracked and cannot appear in an export. Also: worst_rc() replaces max() (a signal-killed child has a NEGATIVE returncode, so max(0, -9) == 0 reported PASS); export_tree removes its destination and verify_export refuses a short, index-mismatched or truncated export; a pre-existing row now prints BOTH summaries when they differ; resolve_pr runs gh with cwd=repo and its state/baseRefName are read; --fetch refreshes the base and its age is always printed; exports are named per run so concurrent runs under one --tmp cannot collide. Tests: 47 -> 87, every one a method on a unittest.TestCase. main(), run_gate's return-code handling, print_table and the export assertions had no coverage at all; each defect above has a regression test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WdCK1xgrJdiJzPCh3bAJfQ
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WdCK1xgrJdiJzPCh3bAJfQ
Gatekeeper note: three PRs contend on the
|
| PR | own module | header total |
|---|---|---|
#2035 premerge_check |
new, 87 | 312 → 399 |
#2038 test_symrecords |
new, 24 | 312 → 336 |
#2040 test_nearmiss_db |
12 → 21 | 312 → 321 |
All land ⇒ the true total is 432, and 306 of the 312 moves accordingly.
Two things worth stating plainly:
- This number is prose, not an assertion. The workflow comment calls it "the number
to compare against if this job's total ever moves" — nothing checks it. A wrong value
here is a silent documentation lie that survives indefinitely, which is the exact
failure class this repo keeps getting bitten by. - It will conflict, and that is the good outcome. All three PRs touch the same two
lines, so whichever lands second and third goes CONFLICTING rather than
auto-merging a stale integer.
When you resolve it: do not take --ours/--theirs on this file — that silently
reverts the other side's unrelated paths: and enumeration edits. Hand-edit the two
numbers, and measure the new total by running the enumerated list exactly as the
workflow does rather than doing the arithmetic. (#2040 already did it that way; that is
the standard.)
I am serializing the merges and will hold each one to a measured number.
The only conflict was .github/workflows/tool-tests.yml: #2035 wired tools.test_premerge_check into the same enumerated list and rewrote the same two prose passages this branch rewrites. Resolved by hand -- never with --ours/--theirs, which would have taken one whole side of the file and silently reverted the other's unrelated edits. Both sides survive: test_premerge_check (87) and test_symrecords (24) are both enumerated in the runner and both keep their legend block, and the "Costs nothing" paragraph names both. Every count in the header is now MEASURED on this merge result rather than carried from either side, because both sides' numbers were stale: 59 test_*.py modules (git ls-tree; main said 57, this branch said 58) 53 that ran only by hand (59 - the 6 that already had a workflow) 13 pytest-style modules (no unittest.TestCase; the header said eleven) 186 test functions in them (the header said 158) 423 assertions, 23 modules (python -m unittest -v <the enumerated list>: "Ran 423 tests ... OK". Main measures 399, so the delta is exactly test_symrecords' 24 and no module dropped out.) 417 of the 423 assert (423 - the 6 documented skips) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WdCK1xgrJdiJzPCh3bAJfQ
1 participant
The gap
.github/workflows/pr-validate.yml-- the job that submits to the external ROM validator, and the only job in this tree that builds anything -- has exactly one trigger:There is no
push: branches: [main]. No ROM is built after a merge, ever, and every PR is judged against its own base. Two PRs each honestly green against base X can combine into a red main, and nothing observes it until a person notices. That has happened twice in four days (#1987, #1990 -- bothtools/-only changes that landed against a base that had moved).source-coverage.ymlpartly closes the hole with a push-to-main job againstgithub.event.before. But that fires after the merge: it can tell you main is broken, it cannot tell you not to break it. And it is one metric out of seven.tools/premerge_check.pyfills exactly that gap: the static gates, run on the treegit mergewould produce, before the merge.Both trees are exported, every gate runs on both, and the verdicts print side by side.
It never blames a PR for a red base
okREGRESSIONpre-existingfixedA gate already red on
origin/mainis not this PR's to own and can never fail the run. A tool that blames the wrong PR trains people to ignore it. This came up for real mid-development:dead-referenceswent red on main (fixed since, in #2033) and the tool correctly reportedpre-existingon every open PR rather than reddening all of them.A
git merge-treeconflict is its own outcome --CONFLICT, no gates run, "rebase first" -- and is never reported as a gate failure.The gates
Seven, all verified toolchain-free by reading each one, not by observing that it passed:
converted-ratchettiers_ratchet.py --checkdead-referencescheck_dead_references.pyduplicate-sourcescheck_duplicate_sources.pyheader-offsetscheck_header_offsets.py(batched, 120 headers per call)layout-checklayout_check.py --quietsrc-tu-refscheck_src_tu.pysource-coveragesource_coverage.py --json, subtracted between the two treesDeliberately excluded, with reasons in the docstring:
rombuild.pyandeligible.py(hard requirement --build/is shared between worktrees and a concurrent run produces fake failures for everyone);check_references.py(needs an eligibility report, i.e. a compiler);check_src_tu_compiles.py,build_pin,linkcheck,romdata_check(compiler /extracted// ROM);prepush_attribution.py(needs history, not a tree)."Verified toolchain-free" here means the tool was read for the self-
return-reporting-pass shape this repo has shipped before. None of the seven has it.source-coverage is a byte delta, and only a byte delta
--checkwants a git ref and a bare merge tree has no commit, so both trees are measured with--jsonand subtracted here.currentBytesand the per-section map decide the verdict;entriesandmodulesare printed as context and can never fail a run.That distinction is a measurement, not fastidiousness. The first version failed on a decrease in any of the four numbers, and the first time it was pointed at real PRs it called two of them red -- #2023 (
entries11025 -> 11011) and #2024 (11025 -> 11017) -- withcurrentBytesand every per-section total unchanged. Both are TU promotions, which re-partition the address space by construction: thirty-eight per-function delinks entries become three merged ones covering the same bytes, and nothing is handed back to the cartridge. That is precisely the false alarmsource_coverage.py's own docstring exists to avoid. A gate that reddens on every TU promotion is a gate people learn to click past, which fails the same way as a gate that never reddens at all. Both PRs come back clean now, with the drop shown asinfo:.Real runs
Against
origin/main @ b309f5d3f069and four open PRs.origin/mainagainst itself is all-green with no diff, as it must be.Full output (exit 1, because of #2001)
Finding: PR #2001 is green everywhere and red on the tree that does not exist yet
#2001 (
tools/tu-control-policy-0830, headef16a307356c) is open, GitHub reports it MERGEABLE, it is green on its own PR andconverted-ratchetis green onorigin/main. Its merge result failsconverted-ratchet:src/actors/ActorDerived.cpplanded on main in one of the commits #2001 is behind. Neither side can see the collision; only the merge result can.Finding: the tool would have caught the failure that was live on main this morning
a8061b08d(tool-tests.yml) turneddead-referencesred on main and nothing noticed, becausedead-references.ymlfiltered on**.py/**.md/src/**/etc. and that commit touched exactly one.yml. The gate ran neither on the PR nor on the push. Replaying it:This tool runs every gate on the whole merge tree unconditionally, so a
paths:filter cannot hide a failure from it. (#2033 has since fixed both the reference and the filter, independently.)Finding: #2004 does not merge, and GitHub says it does
gh pr listreports #2004 MERGEABLE.git merge-treedisagrees -- real content conflicts inattribution.jsonandnotes/cpp-tu-current-state.md. GitHub's mergeability is computed against a base that has since moved; this is computed againstorigin/mainas it stands right now.Tests
47, in
tools/test_premerge_check.py, enrolled intool-tests.yml.Every test runs with no git, no
gh, no compiler and noextracted/. That is a requirement of the suite, not an accident: the general shape of a gate that reports a clean tree after its match count went to zero is a test file that self-returns when its inputs are missing. So the logic that decides the exit code -- the verdict diff, the coverage delta, the merge-tree parse, the PR resolution failures -- is written as pure functions over data and tested as such. The one test that genuinely needs a git binary callsself.skipTestand says why; a real skip is visible in the runner output, a barereturnis not.Includes
test_no_gate_can_ever_invoke_a_rom_build, which asserts that "rombuild" and "eligible" appear in no gate's argv.Advisory, on purpose
Nothing in
.github/workflows/runs this, and this PR adds no workflow that does. It is a command you run before you press merge. Wiring it into CI is a separate, deliberate decision -- it would need a call on where apull_requestjob gets a base to compare against, and on whether a green->red verdict should block a merge or only comment. Landing the tool first means that decision can be made against something people have actually used.Usage
Exit codes:
0clean,1a gate went green->red,2a merge conflict,3the tool could not answer. PR numbers resolve throughgh pr view N --json headRefOid, with a clear message rather than a traceback whenghis absent or unauthenticated. Exported trees are removed withshutil.rmtree(never git's own removal, which recurses through the worktree junctions).Runtime is about 2 minutes per target on Windows; the base tree is exported and gated once and reused across every target in a run.
🤖 Generated with Claude Code
https://claude.ai/code/session_01WdCK1xgrJdiJzPCh3bAJfQ