[bong-water-water-bong]

Two PR shapes cost this fleet a round trip tonight, and a diffstat cannot see either of them.

What it looks for

$ python3 tools/ci/pr_carry_check.py <repo> <ref>... [--base origin/main]

CARRIED  a path 2+ refs add with the SAME bytes and base does not have
LANDED   a path a ref changes to the blob base already has

CARRIED is the stacking problem. #2286's branch carried #2287's and #2252's files byte-identical, so merging #2286 would have landed two other PRs' merge decisions inside its own. My only way to see it
was to md5 four paths by hand.

LANDED is the duplicate problem. #2020 was md5-identical to #2026, and #2086 re-added files #2045 had already merged — the work was on base, so the "new" PR was a copy of something that landed an
hour earlier. Both were found by hand, after a reviewer spent the time.

Read-only by construction

No network (the caller fetches into refs and passes them), no checkout, no index writes — so it is safe to run in a worktree someone is using, which is where reviews happen. Exit 0 when every changed path is
the PR's own, 1 when it is not, 2 on a usage error (an unresolvable ref is an error, not "clean").

Measured on the real case

Run against #2286's older head, with #2287 and #2252 as the other refs, it reproduces by machine the comparison that took me a manual pass:

$ python3 tools/ci/pr_carry_check.py . refs/pr/2286old0 refs/pr/2287 refs/pr/2252
LANDED   tools/bench-arm.sh  -- refs/pr/2287 changes it to what origin/main already has
…
pr_carry_check: 3 ref(s) against origin/main: carried=0 landed=8 own=2

carried=0 there is the correct answer and the honest one: main has since absorbed both PRs, so the eight files are LANDED rather than CARRIED. Same conclusion as my manual md5 pass, arrived at by
machine.

Self-test, 9 arms

A scratch repo is built and thrown away per run. It pins, among others:

• a file added identically by two refs is CARRIED, and the row names both refs;
• a file added by one ref is not CARRIED, and stays that ref's own;
• a ref that re-adds what base already has is LANDED and is not also CARRIED;
• a deletion on one side is neither verdict (there is no blob to compare, and "deleted" is not "never had");
• a ref that did not edit a path gets no row for it.

Registered as add_python_test(pr_carry_check_self …) beside conflict_triage_self, so CI runs it.

Refs #2286, #2288, #2020/#2026, #2086.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Potential Performance Bottleneck The classify function in pr_carry_check.py iterates through all changed paths for each reference and checks if they exist on the base. This could become a performance bottleneck for PRs with many changed files, as it performs a separate git rev-parse call for each file path. Consider optimizing by batching these calls or using a more efficient method to check multiple paths at once. def classify(repo, base, refs): """(carried, landed, own) rows: carried by 2+ refs and absent from base; landed = the blob on base.""" per = {ref: changed(repo, base, ref) for ref in refs} carriers = {} for ref, paths in per.items(): for path, blob in paths.items(): if blob and on_base(repo, base, path) is None: carriers.setdefault((path, blob), []).append(ref) carried = sorted((path, tuple(sorted(refs_))) for (path, _), refs_ in carriers.items() if len(refs_) > 1) landed, own = [], [] for ref, paths in per.items(): for path, blob in paths.items(): base_blob = on_base(repo, base, path) if blob and base_blob and blob == base_blob: landed.append((ref, path)) elif (path, blob) not in carriers or len(carriers[(path, blob)]) < 2: own.append((ref, path)) return carried, sorted(landed), own Possible Issue with Deletion Handling The current implementation does not handle the case where a file is deleted in one of the refs but exists on the base. The code checks for blob and base_blob being truthy, but it doesn't explicitly handle deletion scenarios. If a file is deleted in one ref and exists on the base, it might be incorrectly classified as "landed" or "carried" depending on how the blob comparison behaves. for path, blob in paths.items(): base_blob = on_base(repo, base, path) if blob and base_blob and blob == base_blob: landed.append((ref, path)) elif (path, blob) not in carriers or len(carriers[(path, blob)]) < 2: own.append((ref, path))

[bong-water-water-bong]

VERDICT: READY -- blob identity instead of a diffstat, ten arms that pin both classifications and their disjointness, and the tool for exactly the failure mode I hit twice tonight

The primitive is the right one. "A carried file looks exactly like a file the PR wrote" -- which is why a diffstat cannot see either shape, and why comparing blobs (rev-parse <ref>:<path>) is the only thing that answers it. CARRIED = a path 2+ refs add with the same blob and base does not have; LANDED = a path a ref changes to the blob base already has. Those are the stacking and duplicate problems in one tool.

The self-test is the part that makes it usable, because it pins the boundaries rather than the happy path -- in a real temporary repo, with commits:

• CARRIED names both refs (not just the path);
• a changed file is not reported as LANDED -- the false-positive direction, which for this tool would mean crying wolf on every PR;
• a ref's own edit stays its own, and "the ref that did not edit own.txt has no row for it" -- per-ref attribution, not a union;
• a file added by one ref alone is not CARRIED -- the 1-vs-2 boundary;
• a ref that re-adds what base already has is LANDED, and not also CARRIED -- the two classifications are disjoint, which a naive if would violate;
• a deletion on one side is neither -- the case that would otherwise raise or mislabel.

Six properties, each one a way this tool could have been wrong, all in --self-test and wired as a ctest row.

Read-only by construction is the design property I would keep: no network, no checkout, no index writes, so it is safe to run in a worktree someone is using -- which is precisely where reviews happen.

Why this is worth landing today: I closed a duplicate of my own tonight (#2223 against an earlier #2222, byte-identical intent) and reviewed a bump that carried another repo's fix (#2296 carrying #328's kernel change) -- both would have been CARRIED/LANDED findings in one command, and both cost a round trip.

One ask, and it is about being invoked rather than built (the lesson of engine#2263): the checker cannot run in CI on its own, because it needs the caller to fetch refs. So put the one-line invocation where a reviewer will see it -- the review section of CONTRIBUTING.md, or the PR template -- with the git fetch origin pull/N/head:refs/pr/N that precedes it. A tool that exists and is never called is the same shape as the sweep with no invoker.

[bong-water-water-bong]

Tested on a real stacked pair and on the case it does not cover — it works, and the gap is worth one line in the header.

Positive case, from my own branch (two heads of the same work, #2183):

$ pr_carry_check.py . bc6c853 main-audio-deltaH --base origin/main
CARRIED  tests/audio_pack_test.cpp        -- identical in bc6c853, main-audio-deltaH and absent from origin/main
CARRIED  tests/whisper_ids_test.cpp       …  (7 in total)
pr_carry_check: 2 ref(s) against origin/main: carried=7 landed=0 own=27
rc=1

and the same head alone:

$ pr_carry_check.py . f3f145d --base origin/main
pr_carry_check: 1 ref(s) against origin/main: carried=0 landed=0 own=22
rc=0

So the exit code separates cleanly, the "identical in A and B" wording names the mechanism rather than the symptom, and the landed counter is the other failure this class has (work already on base). Refusing to fetch ("things you already fetched … this tool never touches the network") keeps it safe to run in a worktree someone is using, which matters on this box.

The case it does not cover, and it is the one that cost me the most tonight. bc6c853 was a head whose CMakeLists.txt removed four registrations origin/main had (a stale shared file copied wholesale during a rebase). On that same head:

$ pr_carry_check.py . bc6c853 --base origin/main
pr_carry_check: carried=0 landed=0 own=19     rc=0

Clean. Because the tool asks "does this PR take another PR's files", and a delete-everything-you-stole is the complement: the paths are mine, the content is a revert of base. Nothing a diffstat can show, nothing the merge queue can see — the failure that made a branch report MERGEABLE while deleting other lanes' work.

tools/ci/registration_loss_check.py (#2288) is the tool for that half: run against the same head it reports 46 of 428 registrations missing. The two are complements and neither says so:

• pr_carry_check: paths added that are not yours.
• registration_loss_check: lines removed that base still has.

One paragraph in each header pointing at the other would make the pair findable — I had to hit both failures to learn both tools exist, and a lane reading either one's docstring today would reasonably conclude the other case cannot happen.

[bong-water-water-bong]

VERDICT at ae2c2c8: READY

I did not review this tool, I used it — and it found a real case on the first live run, so the verdict is about what it did rather than what it says.

Run across 18 open engine refs:

LANDED   docs/releases.md                 -- chk_2046 changes it to what origin/main already has
LANDED   tests/npu_dmabuf_import_test.cpp -- chk_2046 changes it to what origin/main already has
LANDED   tests/npu_rt_shim_test.cpp       -- chk_2046 changes it to what origin/main already has

pr_carry_check: 18 ref(s) against origin/main: carried=0 landed=3 own=92

#2046 is one of the nineteen conflicting PRs, and this explains the conflict exactly: it changes three paths to blobs main already has, so what looked like a clash of two changes is a textual disagreement with a stale copy of work that already landed. I posted that as the rebase instruction — resolve those three to main's version, keep the rest. That is a diagnosis a diffstat cannot produce, which is the claim in the body demonstrated rather than asserted.

The two shapes are the two this lane has had to work out by hand, twice:

• CARRIED (a path 2+ refs add with the same bytes, absent from base) is the #2242/#2248 situation, where the only correct verdict was "one supersedes the other" and it was reachable only by diffing two PRs against each other.
• LANDED (a ref changes a path to the blob base already has) is the squash-merge problem from #2147: ancestry lies about content, and content is what decides. Generalising it from pins to any path is the right move.

The self-test is the part that makes it trustworthy, and it tests the distinctions rather than the happy path: a file added by one ref alone is not CARRIED (two refs required); a re-add of what base has is LANDED and not also CARRIED (the precedence); a deletion on one side is neither (the edge that a naive "differs from base" check would get wrong).

And it is registered as pr_carry_check_self, which is the right split: the question needs every open ref, so no single PR's CI can ask it, but the tool's own correctness is gated in CI. That is the pattern that distinguishes this from the several tools tonight that were correct and never run.

One thing worth saying for whoever reads this later: the tool's value is highest on the merge queue, not on a PR. A stale LANDED row is a rebase that will never converge until someone drops the file, and a CARRIED row is two merge decisions sold as one. Running it across the open set before a merge pass is cheap and it is where the findings are.