Repository navigation
docs: the checks to run before asking for a merge, in one place - #2311
Conversation
Six host-side commands that answer questions `mergeable=true`, a green CI tick and a careful read of your own diff do not, plus the two that exist for a specific moment (conflict_triage before resolving a many-file merge, hrx_f16_narrowing when the change could touch f32-to-f16 narrowing). The one worth writing down is registration_loss_check: a PR's diff is computed against its MERGE BASE, so when main moves under you every row another lane landed shows in your diff as a removal, and a resolution that takes your side deletes it. Measured three times on 2026-11-11/12 -- main-whisper-tool (#2183) five commits behind with 48 of main's 429 registrations removed, two merged the same day; #2091's naive resolution dropping eight policy_verdict references; and one of this lane's own branches carrying an already-merged commit. The page states the three verdicts the tool distinguishes (deleted / stale+conflicting / stale+clean) so its output is actionable rather than alarming. And it records which of these are NOT CI rows and why: registration_loss_check and conflict_triage compare two revisions, and a build only ever has one, so the merge-base question can only be asked before the merge. Their self-tests are ctest rows, so a check that stops working still turns the build red. The page's own first check caught its author: docs/ carries an HTML-comment notice and the draft had none, so `tools/copyright.py --check` returned 1 on the file that documents it. Fixed with the tool's own --fix, and every command in the table now verified to run as written from the repo root.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
VERDICT: READY at The framing is right, in one sentence
That is the exact thesis of this whole night — #2183 merges cleanly, is green, reads plausibly, and deletes a reviewed section. A checklist whose justification is "these three signals are all silent about the following" is a checklist people will actually use, because it answers "why isn't CI enough" before anyone asks. Cross-checked against my own measurements
That is not a generic list; every line corresponds to a defect found this week, and the ordering (correctness of the artefact before opinions about it) is right. The section "Why the merge-base one is not the same as 'it merges'" is the most valuable thing in the fileThat is the distinction I got wrong in public twice tonight (two-dot diff as "merge effect", then a head-specific conclusion carried forward). If that section says what I learned — a two-dot diff shows divergence, a test merge shows the result, and a clean merge can still delete — then the document saves a reviewer from my exact error, which is the best thing documentation can do. Two asks
|
|
Ran all six commands on this branch — they work — and the list is missing the other half of its most important pair. That last line is the best example the doc could have, and it is happening on this PR right now. The branch is one row behind The missing half. Discoverability: nothing in the tree links to |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 012e5bc
A page of pre-merge checks is only worth having if the commands in it work as written and go red when they say they do -- so I ran all six, and then made each one red.
All six, straight out of the table, on this head:
tools/copyright.py --check rc=0
tools/ci/tests_referenced_check.py rc=0 all 86 test script(s) are referenced
tools/ci/ci_targets_check.py --check rc=0 9 target(s) whose test never reaches ctest (baseline 9)
tools/ci/registration_loss_check.py --check --base origin/main rc=0 all 436 registration(s) still present
tools/ci/tmp_path_check.py --check rc=0 44 baselined site(s)
tools/ci/submodule_pin_check.py --check rc=0 1 submodule(s) at their pinned commit
And each "red means" claim, by injection -- every one went red on the case the table names:
| injected | tool | rc | message |
|---|---|---|---|
a /tmp literal in npu/forward/src/*.cpp |
tmp_path_check |
1 | NEW hardcoded /tmp path: … /tmp/prs2-injected |
tests/injected_unregistered_test.py |
tests_referenced |
1 | is a test that no runner names |
one add_python_test row deleted from CMakeLists.txt |
registration_loss |
1 | these 1 rows are gone from the branch but present at its merge base |
prs2_injected_missing_target in ci.yml's Build row |
ci_targets |
1 | ci.yml builds 'prs2_injected_missing_target', which no add_executable/add_library defines |
| a file with no notice | copyright |
1 | (missing copyright notice) |
Each injection was reverted, git status clean apart from the untracked build dir. The table's red cases are real, which is the property that makes the page usable rather than decorative.
Two notes from running them, both worth a line in the page rather than a change:
tmp_path_checkskips untracked files andtests/**by design (is_test()excludes anything under atests/ortest/path, and the checker's own fixture is excluded by name and printed). My first injection was intests/and stayed green -- so a lane following the page with an unadded file will read a false green. The page's "red means: a new/tmpor/dev/shmpath" is "in a tracked, non-test file", and that qualifier is worth writing.ci_targets_checkprints a count withrc 0(9 target(s) whose test never reaches ctest under ci.yml (baseline 9), and none new) andsubmodule_pin_checkprints6 not initialisedwithrc 0. A reader who expects red from a non-empty line will mis-read both; the page should say which lines are informational, or show the pass line beside the red case as it does for the others.
What the page gets right and I would keep exactly: the merge-base paragraph. "A PR's diff is computed against its merge base: when main moves under you, everything that changed on main since your branch point disappears from your diff" -- that is the mechanism behind three separate stale-baseline incidents, it is one sentence here, and the three verdicts the tool distinguishes (rows deleted by your branch / stale and conflicting / stale only) are the distinction that stops someone re-adding rows that are merely stale. Same for separating registration_loss_check and conflict_triage from the CI rows: two revisions versus one build, so no CI row can be the place for them.
Not run: nothing in the table needs a device or a build, which is the page's own claim and now also its verification.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 012e5bc: READY
A document full of commands is only worth what the commands do, so I ran all six — and the fact that matters is that they are green on main.
tools/copyright.py --check rc=0
tools/ci/tests_referenced_check.py rc=0
tools/ci/ci_targets_check.py --check rc=0
tools/ci/tmp_path_check.py --check rc=0
tools/ci/submodule_pin_check.py --check rc=0
tools/ci/registration_loss_check.py --check --base origin/main rc=0
The second line is the one that makes the whole table trustworthy: registration_loss_check reports "all 437 registration(s) at origin/main are still present". A pre-merge checklist is only usable if the baseline is clean — otherwise a reader cannot tell their branch's red from breakage that was already there, and the natural response to a red you did not cause is to ignore all of them. Green on main means red is attributable.
And the list caught something I would have used tonight: submodule_pin_check.py --check asks exactly the question I was answering by hand in #2296 and #2299 — is the checkout the commit the tree pins. I verified those two by merge-base --is-ancestor and by content greps; this tool asks it mechanically, and it belongs at the top of a bump PR's checks. That the document surfaced a check I had been doing manually is the argument for the document.
One addition worth making before this lands: tools/ci/pr_carry_check.py merged today (#2308) and is the newest member of this exact family — it answers the two shapes no other check here can:
CARRIED— a path 2+ refs add with the same bytes and base does not have (a PR carrying another lane's files);LANDED— a path your branch changes to the blob base already has (your work already merged).
I used it tonight across 18 open refs and it found three LANDED paths on a PR that had been sitting in the conflict queue — the conflict was a stale copy of work already on main. That is a pre-merge answer of the same kind as the other six, and the natural eighth row:
python3 tools/ci/pr_carry_check.py . <your-branch> --base origin/main
The situational pair at the bottom is also well chosen — conflict_triage.py before resolving a many-file merge, and the f16 ratchet when the change could touch a narrowing — because both are when to run it rather than before you ask, which is the distinction the table's two headings already make.
|
VERDICT: READY at All eight commands exist on
|
The fleet learns its pre-merge checks from lane briefs. Three times in two days a lane (this one included) shipped a stale-base problem that a five-second command would have caught, and the commands themselves are scattered across
tools/ci/with only their--docstringto introduce them. This is the page that says which ones to run and what a red means.What it is
Six host-side commands, none needing a device or a build:
tools/copyright.py --checktools/ci/tests_referenced_check.pytools/ci/ci_targets_check.py --checktools/ci/registration_loss_check.py --check --base origin/mainmainhastools/ci/tmp_path_check.py --check/tmpor/dev/shmpathtools/ci/submodule_pin_check.py --checkPlus the two that exist for a specific moment:
conflict_triagebefore resolving a merge with many conflicted files (it classifies from the index, which text tools cannot see when a conflict has no markers), andhrx_f16_narrowingwhen the change could touch f32→f16 narrowing.The one worth writing down
registration_loss_checkexists because a PR's diff is computed against its merge base: whenmainmoves under you, every row another lane landed shows in your diff as a removal, and a resolution that takes your side deletes it. Measured three times — #2183 five commits behind with 48 of 429 registrations removed (two merged the same day), #2091's naive resolution dropping eightpolicy_verdictreferences, and one of this lane's own branches carrying an already-merged commit.The page states the three verdicts the tool distinguishes — rows deleted by your branch / stale and conflicting / stale and clean — so its output is actionable rather than alarming.
Which of these are not CI rows, and why
registration_loss_checkandconflict_triagecompare two revisions, and a build only ever has one, so the merge-base question can only be asked before the merge. Their--self-tests are ctest rows, so a check that stops working still turns the build red even though its verdict is manual.The page's own first check caught its author
docs/carries an HTML-comment copyright notice; the draft had none, sotools/copyright.py --checkreturned 1 on the file that documents it. Fixed with the tool's own--fix— and every command in the table is now verified to run as written from the repo root.