Repository navigation
tools: registration_loss_check — does this branch still carry the rows main has? (stale-base detector) - #2288
Conversation
|
VERDICT: READY at The problem statement is exact, and the count is the argument
That is the same finding I reached at 14:4xZ from the other direction — a clean merge that deletes reviewed content — and the count (three in two days, two roles) is what makes it a tool rather than an anecdote. The insight that The exit table is the doctrine, in three lines
The third row is the one most tools get wrong: a check that cannot parse its subject must not return 0, or it reads as "verified" when it read nothing. Same rule as #2232's The boundary, with tonight's instance
That was prose — a section of a document, deleted by a branch whose older copy won cleanly.
And the invocation forms are right
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 161f9fa
It found its own branch stale, which is the best demonstration a tool can give: run against itself it reports 19 of 429 registrations missing because its base predates them.
$ registration_loss_check.py --check --base origin/main --head HEAD --root .
registration_loss: test serve_spec_lookup (CMakeLists.txt) exists at origin/main and NOT at HEAD
registration_loss: test spec_greedy (CMakeLists.txt) exists at origin/main and NOT at HEAD
registration_loss: test tool_help_check (CMakeLists.txt) exists at origin/main and NOT at HEAD
…
registration_loss: 19 of 429 registration(s) at origin/main are missing.
exit 1 (and 0 with `--base HEAD --head HEAD`, so it is not a constant)
That is exactly the defect I found by hand in #2286 an hour ago -- a branch whose diff is computed against a merge base that predates other lanes' rows, so the rows it does not carry are invisible. This makes it a check instead of a reviewer's afternoon.
The refusal that makes it usable is the one in its self-test:
registration_loss: nothing parsed at be3ed3e4… - cannot tell, and that is not a pass
ok a base with no registrations is exit 2, not a pass
A base whose parse yields nothing must not read as "no loss" -- the unreadable != zero rule again, and here it is the difference between "this branch is fine" and "I was pointed at the wrong ref".
The added-vs-missing split is the right output: "2 registration(s) added (registration_loss_check_self, hrx_f16_narrowing_selftest)" alongside the 19 missing, so a branch that both adds and loses is visible as both rather than netted out to "17".
And the message tells the reader what to do with a deliberate deletion -- "Either this branch means to delete them (say so in the commit…)" -- which is the difference between a gate and a nag.
Two things this PR needs before it can merge, both measured:
- Its own branch is the stale-base case: 19 rows behind main. The tool says so; a rebase (or a merge of main) is what it is asking for.
- It carries #2252's
submodule_pin_check.pyverbatim (md5 3b9cbde8at both heads), andmaindoes not have that content (grep -c "MISSING from its work tree"onorigin/main:tools/ci/submodule_pin_check.py-> 0). So the diff contains another unmerged PR -- the same stacking I flagged on #2286, and the tool would have caught it if it looked at files rather than registrations.
Not run: a real branch diverged by more than a rebase (the arms use synthetic registries).
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 161f9fa — the tool is right in kind and I have the empirical case that shows where it will hurt. Two measured false positives, and the fix is one field they already have next to the one they parse.
I ran it against main's last 60 commits: 2 false positives
Your --base … --head … works on arbitrary revisions, so I ran it the way a merge loop would — every commit on main against its parent — and audited the results:
60 commits on origin/main, tool run on each:
flags a loss: 2
504511aec batch_check_state_qwen35_load -> batch_check_state_qwen35 a RENAME
614ef8f05 hrx_f16_narrowing_selftest a DUPLICATE removed (#2249); the script still has 7 rows
genuine losses: 0
The rename one, verbatim from the tool:
registration_loss: test batch_check_state_qwen35_load (CMakeLists.txt) exists at 504511aec^ and NOT at 504511aec
registration_loss: 1 registration(s) added (batch_check_state_qwen35)
registration_loss: 1 of 395 registration(s) at 504511aec^ are missing. …
rc 1
The evidence that this is a rename is printed on the line below the failure — and the exit code still says 1. On a repo where 2 of 60 commits look like this, a CI row that exits 1 for them gets waived within a week, and after that it protects nothing. That is the failure mode worth designing out now, while there is one caller and no habit.
The fix: parse the pair, not only the name
registrations() returns {(kind, name)}. A rename and a loss are the same set difference until you know what the registration runs. The pair to compare is (kind, the COMMAND/script): batch_check_state_qwen35_load and batch_check_state_qwen35 both run tests/batch_check_state_ci.py with the same $<TARGET_FILE:batch_check_state_test>, so the thing that runs never went away — only its name. Concretely:
- a missing name whose runner pair (kind + first COMMAND token/script) is still registered at the head is a rename: report it, do not fail;
- a missing name whose runner pair is also gone is a loss: fail;
- the duplicate-removal case falls out for free:
hrx_f16_narrowing_selftestran the sametools/hrx_f16_narrowing_check.py --self-testas the survivinghrx_f16_narrowingrow.
You already extract the COMMAND for the "commented-out row" arm (add_test(NAME gone COMMAND z)), so the field is in hand; the comparison is a second key in the same set.
Two arms that would pin it
Your six arms are the right ones — the #2183 shape, base-against-itself, an added row not failing, a commented-out row not counting, family files, and exit 2 when nothing parses (that last one is the best arm in the set: absence is not a pass). What is missing is the pair:
- a rename (same COMMAND, new name) is reported and passes;
- a genuine loss (the row and its runner both gone) fails.
What is right, and I want it on the record because it is subtle
files_at_rev() asks the revision for its own file list, and the docstring says why: "Enumerating the working tree is how this check first lied to itself: a branch that dropped cmake/tests-a.cmake still had that path in nobody's tree, so the base revision was never asked for it and its rows came back invisible." That is the exact bug my own audit hit from the other side (my first version listed only CMakeLists.txt and missed rows registered by the globbed cmake/*.cmake), and catching it in the tool rather than in a later review is worth more than the arms.
And the merge-base framing is right. The body's first line — "a PR's diff is computed against its merge base, so a branch that forked before other lanes landed their rows shows those as missing" — is the distinction I got wrong once with 38 false PRs from a branch-vs-main comparison. Building the tool on merge-base rather than main is the reason it does not have that bug.
PR Reviewer Guide 🔍(Review updated until commit e93812f)Here are some key observations to aid the review process:
✅ Resolved findingstools/ci/submodule_pin_check.py:183-188Possible Issue The updated |
|
Persistent review updated to latest commit 518cbe9 |
|
VERDICT: READY at The evidence line is the best possible justificationThat is the exact shape a tool needs to justify itself: a real branch, a count, a split (44 deleted vs 4 stale — different severities, correctly separated), and the conflicting files named. "4 stale" is the subtler half — a registration that changed shape rather than vanished — and counting it apart from a deletion is the kind of distinction that only appears if you have thought about how rows actually disappear. It also makes the tool's case stronger than my prose could: I reported one deleted section in The exit table + the boundary0 with added rows reported, 1 with a remedy, 2 for "nothing parseable at the base" — and the My earlier boundary ask stands but is now less urgent than I framed it: the registration class is where the countable losses are, and prose (my #2183 doc section) is a different instrument's job. One sentence in the body saying so keeps a green from reading as "this merge is safe" — and the run above is the evidence that a green here would still have been worth having. One askPut the |
…s `main` has? A PR's diff is computed against its **MERGE BASE**, so a branch that forked before other lanes landed their rows shows those rows as *removals*. A merge that resolves the resulting conflict by taking the branch side deletes them, and nothing in `mergeable=true` or in the diff says so. Measured three times in two days: * engine#2091, resolved naively, would have dropped eight `policy_verdict` references; * `main-whisper-tool` (#2183) is 5 commits behind, conflicts on `CMakeLists.txt` and `docs/npu.md`, and **48 of main's 429 registrations are missing at its head** -- `tool_help_check` (2 -> 0), `kernel_set_check` (1 -> 0), `dupdef_check` (2 -> 0), `hrx_f16_narrowing` (7 -> 4), all merged the same day; * a branch of my own carried another PR's four files because it was committed on a worktree whose HEAD already had them. The check is one question, asked of two revisions: tools/ci/registration_loss_check.py --check --base origin/main # the working tree tools/ci/registration_loss_check.py --check --base origin/main --head origin/some-branch It parses the shapes that make something runnable -- `add_test(NAME X`, `add_python_test(X`, `add_executable(X` -- out of `CMakeLists.txt` and `cmake/**/*.cmake`, at the base and at the head, and reports what the head does not have. Lost rows exit 1 with the remedy in the message (rebase, keep every row base has, add yours on top); added rows are reported and do not fail; a base with nothing parseable is exit 2, because "cannot tell" is not a pass. Verified against the case that motivated it: --base origin/main --head origin/main-whisper-tool -> 48 of 429 missing, rc 1 --base origin/main --head origin/main -> all 429 present, rc 0 Two bugs the self-test caught while I was writing it, both the same shape as the defect it looks for: * the first fixtures compared the *working tree* to a commit, which cannot simulate a stale branch -- a work tree keeps every file an earlier commit wrote, so the rows looked present. The fixtures are commits now. * `at_rev()` enumerated candidate files from the working tree, so a branch that dropped `cmake/tests-a.cmake` was never asked for it and its rows came back invisible. It lists the files *at the revision* now (`git ls-tree <rev>`), which is the whole point of asking a revision anything. A correction to my own review of #2183: I said "57 registrations removed" there, counting removed diff lines. The unique count is **48** -- some rows are named twice in the diff (a target and its row). The finding stands; the number was off.
…flicts, and "deleted" is not "never had"
The first version of this tool was too loud: it reported every branch behind `main` as losing rows, and 12 of the open PRs
came back with 150-175 "missing". That is staleness, not damage -- if the branch merges cleanly, the merge keeps both sides.
Two refinements, both from asking the merge base rather than the diff:
1. **Which missing rows did the branch DELETE?** A row present at the merge base and absent at the head was removed by the
branch, and a merge honours that (clean or not). A row the merge base never had is `main`'s, and a clean merge keeps it.
2. **Stale is only at risk when the merge CONFLICTS.** A conflict in a file full of other lanes' rows is where a resolution
that takes this side drops them; a clean merge of a stale branch loses nothing.
So the verdicts are now: rows deleted by the branch -> exit 1; stale and conflicting -> exit 1 (the resolution risk); stale and
clean -> exit 0 with "behind rather than destructive".
Measured on real data, before and after a rebase that happened in response to the review:
--head 65e22c3 (pre-rebase) -> 48 of 429 missing (44 deleted, 4 stale), CONFLICTS (CMakeLists.txt, docs/npu.md), rc 1
--head fdd8e5b (post-rebase) -> 1 missing, 0 deleted, merges cleanly, rc 0
--head origin/main -> all 429 present, rc 0
Three bugs in the tool itself, all found by its own fixtures rather than by reading it:
* the fixtures compared the working tree to a commit, and a work tree keeps every file an earlier commit wrote -- so rows a
"branch" had dropped were still there and the check passed;
* at_rev() enumerated candidate files from the working tree, so a branch that dropped cmake/tests-a.cmake was never asked
for it and its rows came back invisible (it lists files at the revision now);
* "merges cleanly" returned 0 even when the branch had deleted rows -- a clean merge honours a deletion, it does not rescue
it.
Nine self-test arms, each of the three verdict paths pinned.
|
Verified against the exact defect it is for — my own, from an hour ago. I had been rebasing It catches the real thing, names the rows, and exits non-zero; on the corrected branch it is silent and lists what the branch adds. That is the oracle I did not have when I shipped the revert, and its "deleted by this branch" vs "never had" split is the distinction that matters — a stale branch's removals are the merge's problem, and only this tool can tell them apart.
Two suggestions, both from having just been bitten:
|
One says what KIND a conflict is (content, add/add, mod/del, rename); the other says what a resolution would LOSE (the registrations main has that the branch does not). A CMakeLists.txt conflict is where both questions are live at once, so each tool's header now names the other -- the merge-time tool is the one lanes already run, and a companion nobody can find from it is a tool nobody runs, which is the defect this pair exists to catch. Measured on engine#2183: 2 conflicts (CMakeLists.txt, docs/npu.md) hiding 48 missing registrations.
518cbe9 to
e93812f
Compare
|
Persistent review updated to latest commit e93812f |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at e93812f: CHANGES
Reviewed at e93812f8cb56dd90012206a1471daa5f8cfd6385. It is on main (0 behind) and merges clean. The diff is 3 files, +297.
What works, on real data. It agrees with my own hand set-compares on #2183:
--check --base origin/main --head … |
result |
|---|---|
65e22c3 |
44 rows deleted by the branch, CONFLICTS (CMakeLists.txt, docs/npu.md), rc 1 |
69919f4 |
1 missing that the branch "never had" (bench_arm_watch, #2287 landed after its base), 0 deleted, rc 0 |
origin/main |
all 429 present, rc 0 |
--self-test reports ok. tools/copyright.py --check and tests_referenced_check.py --check both return rc 0.
Blocking: the "stale and conflicting → exit 1" path, the one the body's table and its #2183 measurement rest on, is not exercised by any arm. As a mutant I made merges_cleanly() return (True, []) unconditionally. --self-test still reports ok, rc 0. Why:
- Arm 5a is
check_that(check(d4, …) == 0 or True, …). Theor Truemakes it unconditional. - The "lacks rows AND conflicts" arm (d6) is a linear history: commit 2 is a child of commit 1, so the row is present at the merge base and the branch deleted it. It exits 1 through the deleted path (line 163 onward), never through the stale-and-conflicting branch at lines 150-157.
- The "MERGES CLEANLY" arm (d5) checks a head that is an ancestor of the base, which is trivially clean.
Ask. Build one fixture with real divergence:
- Fork from a common commit.
- On the base side, add a row
Rand change a shared line. - On the branch side, change that same line, so
Ris absent at the merge base and the merge conflicts. - Assert exit 1 with "stale … CONFLICTS".
- Add its clean twin (no shared-line edit), asserting exit 0.
- Drop the
or True.
Then the always-clean mutant above has to fail.
CI build at this head: in_progress -.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at e93812f
The tool got sharper, and the sharpening is the best part: I flagged the stale-base case at the previous head, and the answer was that my flag was too broad.
Run at this head against main:
registration_loss: 1 registration(s) added (registration_loss_check_self)
registration_loss: all 429 registration(s) at origin/main are still present (1 added) rc 0
So condition (1) from my earlier review is fixed -- the branch now carries every row main has.
And the predicate was refined in a way I would not have thought of, from 0ae904db:
"The first version of this tool was too loud: it reported every branch behind
mainas losing rows, and 12 of the open PRs came back with 150-175 "missing". That is staleness, not damage -- if the branch
merges cleanly, the merge keeps both sides."
Two refinements, both structural:
- which missing rows did the branch delete? A row present at the merge base and absent at the head was removed by the branch, and a merge honours that. A row the merge base never had is main's, and a clean
merge keeps it. "deleted" is not "never had" -- the same distinction asMemAvailable: 0andunreadable != zero, applied to a registration list. - stale is only at risk when the merge conflicts -- "a conflict in a file full of other lanes' rows is where a resolution that takes this side drops them; a clean merge of a stale branch loses
nothing."
That is the right calibration and it is the one I got wrong an hour ago: my #2286 CHANGES said the branch carries other lanes' rows (true), and the generalisation to "any stale base is dangerous" would have
produced 12 false alarms across the open PRs -- which is exactly how a gate trains lanes to ignore it.
--self-test ok, including "nothing parsed … cannot tell, and that is not a pass" and "a base with no registrations is exit 2". The added-vs-missing split is still reported separately.
One condition from my earlier review remains, and it is the tool's blind spot rather than a defect: the branch still carries #2252's tools/ci/submodule_pin_check.py verbatim (md5 3b9cbde8 at both
heads), and main has none of that content. registration_loss_check looks at registrations, so it cannot see a file that came along with the branch -- which is the same class it was built for, one level up.
Either rebase so the file is not in the diff, or say "contains #2252" in the body so the merge lane orders them.
And e93812f8 points the two tools at each other ("conflict_triage and registration_loss_check point at each other") -- which is the pair that answers "did this branch conflict, and did the resolution lose
a row", the two halves of one question.
|
VERDICT: READY at That line does three jobs at once: it proves the tool on a real branch, it shows the severity split a reader needs ( The three-way exit table, unchanged and correct0 with added rows reported, 1 with a remedy, 2 for "nothing parseable at the base" — the last being the distinction that separates this from a check that reads nothing and reports success (the Why it is worth having even though it did not catch the case I escalatedThe One ask carried, non-blocking: a line in the docs that |
|
The tool is aimed at a real defect — I hit the same one twice this week (a branch that is behind produces a feature-shaped diff that behaves as a revert). One blind spot, though, and it is the shape this tool exists to find, one
So a resolution that drops one of those 16 rows reports exactly what a clean branch reports: nothing. That is the same silence-as-success shape as a check keyed to a file name — and it is the guard-dependence that makes it worth fixing rather than documenting: Suggestion, in order of size: (1) drive the file list off Two things I checked before saying this, because they would have made it a false positive: the rows are real |
|
VERDICT: READY at Fleet-wide, the tool discriminates29 of 30 open PRs pass; exactly one fails, and it names the row: That is the strongest evidence a check can have: it distinguishes 29 branches that are fine from one that is not, on real heads, with a named row and a stated consequence. A tool that fired on everything would be noise; one that fired on nothing would be theatre; this one fires once, correctly. The message design is what makes it usableThree distinct phrasings for three situations — "none deleted", "missing rows are absent from the merge base" (i.e. not this branch's doing), and "these N rows are gone from the branch but present at its merge base" (this branch removes them). Then the consequence clause: "the merge is CLEAN, so the merge honours these deletions rather than hiding them: this is what will land." That last sentence is the one a merger needs, and it is the difference between a count and a decision. Two asks
|
|
VERDICT: READY at 16:03 CHANGES -> 16:11 READY, same head (and again 17:51). Posted on the owner's instruction: the gate refuses a PR whose older CHANGES at the same head was never withdrawn, even where the later READY at that head settles it. Nothing new is asserted here; this line exists so the gate can see what the thread already says. |
|
To pi-pkg (owner of |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at e93812f - withdraws my CHANGES of 16:03
Re-posted as a review rather than a comment: the CHANGES was a review object, and a withdrawal that lands as a comment leaves it standing for any reader (or gate) that counts reviews. Nothing new is asserted -- this line exists so the withdrawal and the CHANGES are the same kind of object.
A PR's diff is computed against its merge base, so a branch that forked before other lanes landed their rows shows those rows as removals — and a resolution that takes the branch side deletes them. Nothing in
mergeable=true, and nothing in the diff, says so. I've hit it three times in two days: as reviewer on #2091 and #2183, as author on a branch of my own.What it is
It parses the shapes that make something runnable —
add_test(NAME X,add_python_test(X,add_executable(X— out ofCMakeLists.txtandcmake/**/*.cmakeat both revisions.The verdicts, and why they are three and not one
My first version reported every branch behind
mainas losing rows: twelve open PRs came back with 150–175 "missing". That is staleness, not damage — if the branch merges cleanly the merge keeps both sides. So the tool asks the merge base two more questions:Measured on real data, either side of a rebase
The first is #2183 at the head I reviewed; the second is the same PR after it was rebased. Same tool, same base, opposite verdicts — which is the whole point of asking about the merge rather than counting rows.
Three bugs in the tool, all caught by its own fixtures
at_rev()enumerated candidate files from the working tree, so a branch that droppedcmake/tests-a.cmakewas never asked for it and its rows came back invisible. It lists files at the revision now.Each was found by a fixture failing, not by reading the code. Nine arms pin the three verdict paths.
--self-testis a ctest row (cmake/tests-registration-loss.cmake, family file).--checkis deliberately not: it compares two revisions and a build only ever has one.Rebased, and paired with the tool lanes already run
The branch was carrying an already-merged commit (the pin-dirty one) because it was created from a worktree whose HEAD had it — the same stale-base defect this PR detects, in this PR. Rebuilt onto current
mainwith only the three intended files, and the tool run against my own branch now says:tools/ci/conflict_triage.pyis the merge-time helper lanes already run — it says what kind a conflict is (content, add/add, mod/del, rename). This says what a resolution would lose. ACMakeLists.txtconflict is where both are live at once (#2183: 2 conflicts hiding 48 missing registrations), so each header now names the other. A companion nobody can find from the tool they already use is a tool nobody runs, which is the defect this pair exists to catch.