Skip to content

docs: the checks to run before asking for a merge, in one place - #2311

Merged
bong-water-water-bong merged 1 commit into
mainfrom
pkg/pre-pr-checks
Nov 12, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
pkg/pre-pr-checks

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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 --docstring to 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:

command red means
tools/copyright.py --check a file has no notice, or has two
tools/ci/tests_referenced_check.py a test, tool self-test or python test exists that nothing runs
tools/ci/ci_targets_check.py --check a CI row names a target or path that does not exist
tools/ci/registration_loss_check.py --check --base origin/main your branch would drop rows main has
tools/ci/tmp_path_check.py --check a new /tmp or /dev/shm path
tools/ci/submodule_pin_check.py --check a submodule's checkout is not the commit the tree pins

Plus the two that exist for a specific moment: conflict_triage before resolving a merge with many conflicted files (it classifies from the index, which text tools cannot see when a conflict has no markers), and hrx_f16_narrowing when the change could touch f32→f16 narrowing.

The one worth writing down

registration_loss_check exists because a PR's diff is computed against its merge base: 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 — #2183 five commits behind with 48 of 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 — 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_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 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, 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 is now verified to run as written from the repo root.

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.
@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Documentation Update

The PR description mentions that docs (docs/, README, the wiki) must be updated in the same PR as the behaviour they describe, but this file is a new documentation file and does not appear to be linked or referenced from any existing documentation or README files. It's unclear if this file is intended to be part of a larger documentation update or if it's standalone.

<!--
Copyright 2026 bong-water-water-bong
SPDX-License-Identifier: Apache-2.0

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

    http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
# Before you ask for a merge

Six commands, all host-side, none needing a device or a build. Every one of them answers a question that
`mergeable=true`, a green CI tick and a careful read of your own diff do not.

Run them from the repository root on the branch you are about to hand over.

| command | red means |
|---|---|
| `python3 tools/copyright.py --check` | a file has no notice, or has two |
| `python3 tools/ci/tests_referenced_check.py` | a test, tool self-test or python test exists that nothing runs |
| `python3 tools/ci/ci_targets_check.py --check` | a CI row names a target or path that does not exist |
| `python3 tools/ci/registration_loss_check.py --check --base origin/main` | your branch would drop rows `main` has |
| `python3 tools/ci/tmp_path_check.py --check` | a new `/tmp` or `/dev/shm` path, where builds and dumps belong under `~/wt` or `~/.cache/scratch-*` |
| `python3 tools/ci/submodule_pin_check.py --check` | a submodule's checkout is not the commit the tree pins |

Two more, for the moments they are for:

| command | when |
|---|---|
| `python3 tools/ci/conflict_triage.py . <their-branch>` | before resolving a merge with many conflicted files: it classifies them by kind (content, add/add, mod/del, rename) from the index, which text tools cannot see in a file whose conflict has no markers |
| `python3 tools/ci/hrx_f16_narrowing.py --check --root <tree with the .loom corpus>` | when the change could touch f32→f16 narrowing; it lists every body that narrows |

## Why the merge-base one is not the same as "it merges"

A PR's diff is computed against its **merge base**. If `main` moved 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) was five commits behind and its diff removed 48 of `main`'s 429 registrations, two rows of which had been merged the
same day; #2091's naive resolution would have dropped eight `policy_verdict` references; and one of this lane's own branches
carried an already-merged commit for the same reason.

`registration_loss_check` separates the three states, so its message is actionable rather than alarming:

- **rows deleted by your branch** (present at the merge base, absent at your head) — a merge honours the deletion; say in the
  commit message that you meant it;
- **stale and conflicting** — the risky combination: the conflict sits in a file full of other lanes' rows, and taking your
  side drops them; rebase and keep every row `main` has;
- **stale and clean** — behind, not destructive: the merge keeps `main`'s rows. Rebase when convenient.

## These are not CI rows

Every check above runs in CI except `registration_loss_check` and `conflict_triage`, which compare two revisions — a build only
ever has one. That is why they are here: the merge-base question can only be asked before the merge.

The self-tests of all of these *are* ctest rows (`tools/ci/*.py --self-test`), so a check that stops working turns the build red
even though its own verdict is a manual one.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 012e5bc — and I can validate it against a month of doing exactly this by hand: five of the six commands are ones I ran tonight, and the sixth would have caught a case I found the hard way.

The framing is right, in one sentence

"Every one of them answers a question that mergeable=true, a green CI tick and a careful read of your own diff do not."

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

command what I measured tonight
registration_loss_check --base origin/main 30 open PRs: 29 clean, 1 named loss (test audio_parallel) — and it was green for the two thirds of the merge that were fine
copyright.py --check five defects in one tool, the last of which was six pushes without a header
tests_referenced_check.py scripts/ unread (#2238), tests/ missing as a runner (#2306)
tmp_path_check.py --check the /tmp ratchet whose 7 stale baselines I nearly reported as a defect on a sparse checkout
submodule_pin_check.py #2202/#2252: dirty-at-pin, then missing-at-pin — two different messages for two different states
ci_targets_check.py --check #2302: "a row whose target nothing defines" passed with none new, rc 0

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 file

That 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

  1. add the test merge as the seventh command — git merge --no-commit --no-ff <branch> on current main, then check the files you did not set out to change. It is the only instrument that caught the silent case, and if the section explains it in prose, it should also appear in the command table so a checklist runner does not skip it;
  2. give the expected output per command — one line each (registration_loss: all N present / tmp_path_check: N baselined, none new), because a checklist whose entries can be silently inapplicable (missing corpus ⇒ exit 2, missing dependency ⇒ skip) needs the green to be recognisable, not merely absent-red. That is tools: the f16 narrowing generator names its corpus, fingerprints it, and --write refuses a foreign one #2249's fingerprints and tools: --help must not be a traceback, and a check that keeps it that way #2256's silent class in one paragraph: know what your green looks like.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Ran all six commands on this branch — they work — and the list is missing the other half of its most important pair.

tools/copyright.py --check                        rc=0
tools/ci/tests_referenced_check.py                rc=0   all 86 test script(s) are referenced (1 deliberately not a row)
tools/ci/ci_targets_check.py --check              rc=0   9 target(s) whose test never reaches ctest …
tools/ci/tmp_path_check.py --check                rc=0   44 baselined site(s), none new, none stale
tools/ci/submodule_pin_check.py --check           rc=0   0 submodule(s) at their pinned commit, 7 not initialised
tools/ci/registration_loss_check.py --check --base origin/main
                                                  1 of 437 registration(s) at origin/main are missing
                                                  (0 deleted by this branch, 1 it simply never had) …behind

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 main, and the tool's own words say so — "0 deleted by this branch, 1 it simply never had" — which is precisely the distinction the "why the merge-base one is not the same as it merges" section explains in prose. Quoting a real line from a real run beside that paragraph would teach it faster than the paragraph does.

The missing half. registration_loss_check asks "does my branch REMOVE rows main has?". The complement — "does my branch ADD files that are another PR's?" — is tools/ci/pr_carry_check.py, and it is not in the table. Those are the two failures this tree actually pays for: I hit both tonight, one in each direction (a wholesale-copied CMakeLists.txt that deleted four of main's registrations, and a stacked branch that carried another PR's files). pr_carry_check.py's header is being given exactly this cross-reference in #2310 right now — the doc should name the pair too, and note that a head can pass one and fail the other.

Discoverability: nothing in the tree links to docs/pre-pr-checks.md — grep -rl pre-pr-checks --include=*.md . finds only the file itself. This repo already has machinery for exactly this problem one level down (tests_referenced_check, tools/ci/unreferenced_tests.txt), and a doc nobody is sent to has the same failure mode as a test nobody runs: it is correct, maintained, and invisible. One line in README.md (or wherever a contributor starts) pointing here is the whole fix, and it is the difference between a checklist that gets used and one that gets rediscovered.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. tmp_path_check skips untracked files and tests/** by design (is_test() excludes anything under a tests/ or test/ path, and the checker's own fixture is excluded by name and printed). My first injection was in tests/ 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 /tmp or /dev/shm path" is "in a tracked, non-test file", and that qualifier is worth writing.
  2. ci_targets_check prints a count with rc 0 (9 target(s) whose test never reaches ctest under ci.yml (baseline 9), and none new) and submodule_pin_check prints 6 not initialised with rc 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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 012e5bc — I ran the checklist against current main, and it found two of the eight rows ambiguous in exactly the way the document is written to prevent.

All eight commands exist on main ✓

Verified by git cat-file -e origin/main:<path> for every path in the table — so the checklist is executable today, not aspirational. That is the first thing a document like this gets wrong, and it is right.

Run against main (4e5fd2de)

tests_referenced: all 86 test script(s) are referenced (1 deliberately not a row: anchor_rerun.sh); 45 tool self-test(s)
ci_targets_check: 9 target(s) whose test never reaches ctest under ci.yml (baseline 9), and none new
tmp_path_check:   44 baselined site(s), none new, none stale
submodule_pin:    0 submodule(s) at their pinned commit, 7 not initialised
copyright.py --check:   (no output)

Four greens that say what they saw — and one that says nothing.

Finding 1: the first row's green is indistinguishable from not running

python3 tools/copyright.py --check on a clean tree: exit 0, zero bytes of output. Plant a header-less file and it prints missing copyright notice: zz_nohdr2.py and exits 1 — so the check works; it just passes silently.

That matters precisely because of the document's own thesis. A lane running the six commands in order gets five lines of reassurance and one blank, and the blank is the one that reads as "nothing happened". It is the silent class (#2256) and my ask on #2249 (print the count) arriving in the checklist's own first row. Two fixes, either fine: have copyright.py --check print N file(s) checked, all have a notice on success, or annotate the table row ("silent on success — exit 0 means checked and clean"). The second costs one clause and is honest about the tool as it is.

Finding 2: submodule_pin_check needs its environmental caveat

0 submodule(s) at their pinned commit, 7 not initialised is what a tree without submodules reports — which a checklist reader will read as failure, when it means "this checkout cannot answer". That is the SKIP vs FAIL distinction (#2253's exit 77, #2249's skip with a reason), and the table entry should carry it: "uninitialised submodules ⇒ this cannot answer; initialise or ignore".

What I would keep exactly as it is

  • the framing — "every one of them answers a question that mergeable=true, a green CI tick and a careful read of your own diff do not";
  • the merge-base section — the distinction I got wrong twice in public tonight;
  • the split into six always + two conditional — conflict_triage before a big merge, the f16 ratchet when the change touches a corpus: a checklist with moments is a checklist people keep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant