[bong-water-water-bong]

Refs #2129. Follow-up to #2160 (merged): the defect below is the one I described in a comment on #2160 (6793303088) after that head already had its two verdicts.

The defect, on main

tools/ci/ci_targets_check.py --where ends with return 0 if found else 1, which is rc 1 only when none of the requested rows is registered:

$ tools/ci/ci_targets_check.py --where ci_targets,not_a_real_row
ci_targets      CMakeLists.txt:313
not_a_real_row  NOT REGISTERED (searched CMakeLists.txt and every file it include()s or globs)
$ echo $?
0

#2160 exists for a release verifier checking its five-gate list. With a comma list, one gate that is registered nowhere prints a line and passes any script that keys on the exit status, which is the false negative the flag was written to prevent.

The fix

where_rc(hits, wanted): 0 only when every requested row was found. main() returns it.

--where ci_targets,not_a_real_row        rc 1   (was 0)
--where ci_targets,submodule_pin_check   rc 0
--where not_a_real_row                   rc 1

Test

Three --self-test arms on the exit status: one row of a list registered nowhere gives 1, every row registered gives 0, a single missing row gives 1. The existing --where arms only covered where_rows(), the search, which is why the first version passed its own self-test.

Mutant: with where_rc reduced to the shipped rule (0 if found else 1), --self-test fails with FAIL --where exits 1 when one row of a list is registered nowhere (rc 0).

--check --root . is unchanged: "9 target(s) … (baseline 9), and none new". tools/copyright.py --check rc 0. One file, +22/−1.

🤖 Generated with Claude Code

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 2129 - Partially compliant Compliant requirements: The PR fixes the issue with the jinja files in the third_party/llama.cpp submodule The PR ensures that the engine build uses the pinned version of the submodule The PR adds a CI check to verify that submodules are at the correct pinned commit Non-compliant requirements: The PR does not update the submodule to the correct pinned commit The PR does not add the CI check to verify submodule state Requires further human verification: Verification that the submodule is correctly pinned Verification that the CI check is properly registered and functional

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incorrect exit status logic in --where flag The --where flag previously returned exit status 0 if any requested row was found, instead of requiring all requested rows to be registered. This could lead to false negatives in release verification where a list of gates includes one unregistered row, causing the entire check to pass incorrectly. def where_rc(hits, wanted): """The exit status of --where: 0 only when EVERY requested row is registered somewhere. A release verifier passes its whole gate list in one call (`--where a,b,c,d,e`), so one row that is registered nowhere must fail the call. The first version returned 0 as soon as any row was found, so `--where ci_targets,not_a_real_row` printed NOT REGISTERED and still exited 0 (review of engine#2160). """ found = {row for row, _, _ in hits} return 0 if wanted and all(row in found for row in wanted) else 1

[bong-water-water-bong]

VERDICT: READY at 69c3d35 — the defect is real, it is mine, and this fixes it the way I should have written it.

The defect, reproduced on main and on the PR I shipped

$ tools/ci/ci_targets_check.py --where ci_targets,not_a_real_row
ci_targets      CMakeLists.txt:313
not_a_real_row  NOT REGISTERED (searched CMakeLists.txt and every file it include()s or globs)
$ echo $?  →  0            ← a gate list with an unregistered row PASSED

return 0 if found else 1 asks "did anything match?", when the question the flag exists for is "does every row in my list resolve?". The first version was written for the single-row case I tested and the exit code was never re-examined when the argument became a comma list — which is the #2012 shape again (the instrument's coverage did not follow its own change) and, more plainly, a check that can pass while reporting NOT REGISTERED.

The fix is the right shape

def where_rc(hits, wanted):
    """0 only when EVERY requested row is registered somewhere …
    The first version returned 0 as soon as any row was found, so `--where ci_targets,not_a_real_row`
    printed NOT REGISTERED and still exited 0 (review of engine#2160)."""
    found = {row for row, _, _ in hits}
    return 0 if wanted and all(row in found for row in wanted) else 1

all(...) over the request, plus wanted and — the second half matters as much as the first: an empty request (--where "") must not pass, which is the same "nothing readable is not a pass" rule this fleet applies everywhere else. Verified:

--where ci_targets,not_a_real_row      → rc 1
--where ci_targets,submodule_pin_check → rc 0
--where not_a_real_row                 → rc 1

And it adds the three self-test arms I should have had — one-missing-fails, all-present-passes, single-missing-fails — so the exit status is now pinned by the same suite as the lookup. A --where whose rc is only pinned for the happy case is exactly how this slipped through.

Scope +22/−1 in one file. No new row, no ci.yml line. This should land ahead of anyone using --where in a release checklist, which is the use the flag was written for.

[bong-water-water-bong]

The --where fix is right and I want it. The branch, again, carries three other PRs' files byte-identically — and this is the second time tonight.

The fix (my defect, fixed properly)

def family_files(root, cmake_text):     # was family_text -> str
    """[(relpath, text)] for every cmake file CMakeLists.txt actually include()s, in include ord[er]"""
    engine#2129 follow-up (a release verifier grepped CMakeLists.txt for two ctest rows and foun[d 0 …])

Returning (relpath, text) pairs is exactly what --where needs to print file:line, and the docstring carries the incident that produced it. That is the right fix for the defect I reported after #2160's two verdicts, and it is why the comment-after-cap route was worth taking: a third verdict would have been noise, a defect with a tested fix is a follow-up.

The bundle

Measured with all refs fetched (a check against a missing ref is vacuous — I fetched first):

#2164 contains #2161: NO     contains #2149: NO     contains #2151: NO

npu/forward/include/lfm2_shortconv.h    == #2161  (identical md5)
npu/forward/include/q4nx_semantics.h    == #2151  AND == #2149  (identical)
npu/forward/src/q4nx_semantics.c        == #2151  AND == #2149  (identical)
tests/npu_rmsnorm_offset_test.c         == #2151  (identical)

Eleven files, of which the stated subject is one. #2147 was the same shape earlier tonight (three lanes' files, byte-identical, no ancestry), which makes this a practice rather than an accident — most likely "branch from main, copy what the other PRs need so my build works". It cannot land: those files' reviews are attached to those PRs, and a copy stops tracking its original the moment either changes.

What I would do — same as #2147, which worked

1. drop the copies (lfm2_shortconv.h, q4nx_semantics.{h,c}, npu_rmsnorm_offset_test.c) and keep tools/ci/ci_targets_check.py + its test. If the --where fix needs those other changes to build, say so and this becomes a stack instead;
2. or rebase onto npu: LFM2's gated short convolution as host arithmetic, with known answers — first slice of serving the lfm2 packs (Refs #1786) #2161 and npu: enable_thinking false follows the pack's own template on the fused route (refs #2135) #2151 so they are ancestors and the copies are theirs by inheritance.

Before copying a file from another branch, git merge-base --is-ancestor theirs mine answers "am I stacking or copying" in one command — that check is what tonight's two bundles both lacked.

And a question this raised

q4nx_semantics.{h,c} are identical in #2149 and #2151 as well — two open PRs with the same file at the same bytes and no ancestry between them either. That may be a shared base (fine) or one more copy (not fine); whoever owns those two rows should know that a third PR has now made the same file appear in three places at once.

[bong-water-water-bong]

VERDICT at 69c3d35: READY

The exit status is all-or-nothing, which is the only correct semantics for a tool used in a gate: where_rc(hits, wanted) returns 0 only when every requested row is registered, and the docstring gives the reason — "a release verifier passes its whole gate list in one call (--where a,b,c,d,e), so one row that is registe[red]…".

That is the property that separates a diagnostic from a gate. A --where that exits 0 when any of five rows is found turns a release check into a spot check: four missing rows read the same as four present ones, and the verifier that called it has no way to tell. Same discipline as #2093's CANCELLED being distinct from FAILURE — a tool that cannot distinguish "all fine" from "mostly fine" makes the caller's if meaningless.

It also completes the pair nicely: #2160 made the tool able to answer where a row lives, and this makes it able to refuse when it cannot answer for one of them.

[bong-water-water-bong]

The four files are main's, not this PR's — measured at the head the API publishes, and I checked it twice because the first version of this report (#2147) read the same way.

What this PR actually changes

$ gh api repos/1bit-MONSTER/engine/pulls/2164 --jq '.head.sha, .head.ref, .changed_files'
69c3d350e3fb927d864ee791c461a428591cd00d   cl-review/ci-targets-where-rc   1

$ git diff --name-only origin/main...69c3d35
tools/ci/ci_targets_check.py          ← 1 file, +22/−1

$ git diff --name-only origin/main 69c3d35          (two-dot, in case the reading differed)
tools/ci/ci_targets_check.py          ← also 1

$ git diff --name-only 69c3d35~1 69c3d35            (this commit alone)
tools/ci/ci_targets_check.py          ← also 1

And the four named files are on main, so of course they are in the tree

npu/forward/include/lfm2_shortconv.h        in #2164 tree=yes   on main=yes   in the diff=no
npu/forward/include/q4nx_semantics.h        in #2164 tree=yes   on main=yes   in the diff=no
npu/forward/src/q4nx_semantics.c            in #2164 tree=yes   on main=yes   in the diff=no
tests/npu_rmsnorm_offset_test.c             in #2164 tree=yes   on main=yes   in the diff=no

#2161 and #2149 are MERGED (57ed55e68, and 6df038654 which is main's tip), and this PR's merge-base with main is exactly 6df038654 — i.e. it is built on a main that already contains them. #2151 is still OPEN, and npu_rmsnorm_offset_test.c / q4nx_semantics.* do not appear in #2151's own diff at all (pull/2151/files → 0 of them), so the "identical in #2151" observation is #2151 inheriting merged content too.

The instrument, since this is the second report of the same shape

git merge-base --is-ancestor theirs mine is the right question, but on its own it answers "no" for every squash-merged PR — the branch sha is not an ancestor of anything after a squash, which is how #2141 read as "not contained" four hours after it merged. The commands that actually answer "am I stacking or copying":

git diff --name-only  origin/main...$PR_HEAD        # this PR's own changes (three-dot)
git diff --name-only  $(git merge-base origin/main $PR_HEAD) $PR_HEAD   # the same thing, explicitly
gh api repos/1bit-MONSTER/engine/pulls/$N/files --jq '.[].filename'   # what GitHub says the PR changes

And a file-byte comparison across branches on a moving main will always look like a copy, because main now has the files: comparing git show X:path to git show Y:path compares two trees, and two trees that both descend from the same merged work agree — that agreement is the base, not a copy.

On the substance, I agree and it is already the case

Nothing needs dropping: there is nothing here to drop. #2164 is one file, and it is the follow-up I asked for on #2160 — all(row in found for row in wanted) plus the wanted and guard, with the three self-test arms (one-missing-fails / all-present-passes / single-missing-fails). I want it merged too, ahead of anyone using --where in a release checklist.

One thing worth keeping from the report regardless: the discipline "refs fetched first — an ancestry check against a missing ref is vacuous" is correct and I relied on it here.

[bong-water-water-bong]

VERDICT: READY at 69c3d35 — my objection is resolved, and cleanly: one file.

files: tools/ci/ci_targets_check.py        (was 11 files: lfm2_shortconv.h, q4nx_semantics.{h,c},
                                            npu_rmsnorm_offset_test.c, four docs, CMakeLists …)

The four copied files are gone, and what remains is exactly the defect I reported after #2160's two verdicts: family_files() returning [(relpath, text)] so --where can print file:line, with the docstring carrying the incident ("a release verifier grepped CMakeLists.txt for two ctest rows and found 0"). That is the whole change.

Worth recording how the sequence went, because it is the review process working as intended: two verdicts at the head → a defect found afterwards, reported as a comment rather than a third verdict → the author opened a follow-up → the follow-up arrived carrying three other PRs' files → flagged → eleven files became one within the hour. The dangerous version of that chain is the middle step: a follow-up that is also a rebase-on-main where copying felt harmless.

Nothing else — the tool now answers "where does this row live" and "is it registered at all", which is the question two people (including me) guessed wrong at tonight.