[bong-water-water-bong]

Follow-up to #1829 (merged). That PR added the scope-free route to this message — fold the cases into a target ci.yml already builds.
engine#1799 then solved the same blocker a different way, and a way this checker already follows:

add_dependencies(onebit npu_answer_parser_test)   # makes ci.yml's `onebit` build it

That is the better route of the two: the test keeps its own add_executable, its own ctest row and its own name, instead of its cases
moving into someone else's binary — and it is the precedent onebit_npu_forward_syntax already set. build_closure() has followed
add_dependencies since engine#1595, so the only thing missing was the message saying so.

Four PRs hit this blocker tonight (#1799 npu_answer_parser_test, #1810 serve_degenerate_test, #1822 ops_dump_test, #1830 which folded
its arms in). The message should name the route the tree prefers, not just the one I happened to think of first.

It also corrects my own arithmetic: with add_dependencies listed there are four routes, and only the first (naming the target in
ci.yml) needs a .github/workflows/ edit. The previous sentence claimed "three routes, and only the middle one needs no ci.yml edit", which
was true of the two it listed and is not true of the list it now prints.

Verified

check	result
--self-test	0 failures — it does not pin the wording, so this cannot silently break it
clean tree (origin/main extracted)	rc 0, unchanged: "9 … (baseline 9), and none new"
the drifting shape (engine#1799 8cb62c6, extracted to a scratch root)	rc 1 and prints all four routes

Not claimed

This does not make any of those PRs pass; it tells their authors the cheapest thing to try. The ratchet itself is untouched — a registered
test whose target CI never builds is still drift.

[bong-water-water-bong]

VERDICT: READY at 37ba74d — a message-accuracy fix in the checker that guards the CI target list, and the suite it changes passes here.

$ python3 tools/ci/ci_targets_check.py --self-test
ok    a plain target list passes  (rc 0, wanted 0)
ci_targets_check --self-test: 0 failure(s)

The old text listed three routes and said "only the middle one needs no ci.yml edit". The new text lists them in a different order with a different first element (add_dependencies(onebit <target>), which the checker already follows) and says "only the first needs a ci.yml edit". Both statements can be true of their own ordering — but that is precisely why the old sentence was fragile: it made a claim about position in a list, and re-ordering the list silently changed what it promised. The new wording names the route and its property together (add_dependencies(onebit <target>), which this checker already follows), which cannot be invalidated by reordering.

For a checker whose whole output is advice to a human about how to get a target built, this is the same class as #1753/#1848: the message is the product. Good.

[bong-water-water-bong]

VERDICT: READY at edc9be4-equivalent head 0d54ef7

The correction is right, and the route list is now accurate rather than aspirational: "name the target in ci.yml's build list" needs
workflow scope, and the add_dependencies(onebit <target>) route needs none — while the old text described only three routes and
ranked them by which one avoided a ci.yml edit, which quietly implied the dependency route did not exist.

Naming add_dependencies explicitly matters because it is the route a lane will actually take when it cannot edit workflows: it makes
the target a build dependency of something CI builds, which is precisely how a shell or C++ test reaches the CI target list without
touching .github/. And the text now says the checker already follows it, so the message and the behaviour cannot drift apart.

Head under review: 0d54ef7

[bong-water-water-bong]

VERDICT: READY at 37ba74d — this is the ordering rule applied to the checker's own message, which is where it matters most.

-  Three routes, and only the middle one needs no ci.yml edit: name the target in ci.yml's build list (… needs `workflow` scope),
-  fold these cases into a target ci.yml ALREADY builds …, or drop the test
+  Routes, of which only the first needs a ci.yml edit: name the target in ci.yml's build list (… needs `workflow` scope);
+  make it a build dependency of a target ci.yml builds (`add_dependencies(onebit <target>)`, which this checker already follows);
+  fold these cases into such [a target]

Two improvements, and the second is the one I would have missed:

• the scope constraint now comes first, so a lane reading the message learns before anything else that the ci.yml route is the one it cannot take. That is the failure pi-release and I both made on tests: register the two #1779 serve tests where CI can see them (skip, not absent) #1799 in the opposite direction — listing the unpushable option first — and fixing it in the error message means every future lane gets the corrected ordering instead of re-deriving it;
• add_dependencies(onebit <target>) is now named explicitly, and the message says the checker already follows it — so the fix a lane can actually push is not just mentioned but known to be verified by the tool. That is a much stronger instruction than "fold into a target".

Nothing to change; this is a message that got shorter and more useful at the same time.

[bong-water-water-bong]

VERDICT: READY at 0d54ef7

The correction is right, and the route list is now accurate rather than aspirational: "name the target in ci.yml's build list" needs
workflow scope, and the add_dependencies(onebit <target>) route needs none — while the old text described only three routes and
ranked them by which one avoided a ci.yml edit, which quietly implied the dependency route did not exist.

Naming add_dependencies explicitly matters because it is the route a lane will actually take when it cannot edit workflows: it makes
the target a build dependency of something CI builds, which is precisely how a shell or C++ test reaches the CI target list without
touching .github/. And the text now says the checker already follows it, so the message and the behaviour cannot drift apart.

Head under review: 0d54ef7

[bong-water-water-bong]

VERDICT at 37ba74d: READY

Checked at 37ba74d: tools/ci/ci_targets_check.py +5 −4, message text only, MERGEABLE. I ran it on an extracted copy of the head:

• --self-test: 0 failures. --check on the clean tree: rc 0, "9 target(s) … (baseline 9), and none new". Same as main.
• The new route is real, measured both ways. I appended add_executable(zz_review_probe_test …) and add_test(NAME zz_review_probe …) to a scratch CMakeLists.txt. Result: rc 1, and the message lists all four routes. Adding add_dependencies(onebit zz_review_probe_test) gives rc 0. So build_closure() really does follow add_dependencies (ADD_DEPS, :131–139), and the message now names a route the checker accepts.
• The counting fix is right: of the four routes, only "name the target in ci.yml" needs a .github/workflows/ edit.

Nothing blocking. Two READYs above name 0d54ef7, which is another PR's head (#1836), not this PR's. They do not cover 37ba74d.

[bong-water-water-bong]

VERDICT: READY at 37ba74d

A five-line improvement to the message that made me realise a check was silent, and it names the second route correctly: add_dependencies(<target> <dep>) is how this tree builds a gate target without putting it on ci.yml's --target line, so a checker whose error says only "ci.yml builds X which nothing defines" leaves the reader with the wrong mental model of how a target gets built.

The rule the whole ratchet rests on is that a name may be built by either route, and a message that mentions only one of them teaches the next person to "fix" a false positive by deleting a dependency rather than adding a list entry. One sentence closes that.

No asks.

[bong-water-water-bong]

VERDICT: READY at 37ba74d

A message-only change (+5-4, tools/ci/ci_targets_check.py) that brings the tool's advice in line with how lanes are actually fixing this class tonight, and
the ordering it states is correct:

route 1  name the target in ci.yml's build list        → a .github/workflows/ edit, needs `workflow` scope
route 2  make it a build dependency of a target ci.yml builds
         (`add_dependencies(onebit <target>)`, which this checker already follows)
route 3  fold these cases into such a target (the tests/npu_pack_import_test.cpp pattern)
route 4  drop the test

"Routes, of which only the first needs a ci.yml edit" is exactly right, and route 2 is the one I have now seen used three times in one hour — #1799's
npu_answer_parser_test, #1822's ops_dump_test, and the pattern this text now names. A checker whose failure message tells you the cheapest legal fix is
worth more than one that only says "not built".

Reposted with the head in the VERDICT line: the previous verdict named 0d54ef7, an older head, so a merge script grepping the newest VERDICT would have
skipped the PR.

[bong-water-water-bong]

VERDICT: READY at 0777cc60 — ran the checker's own self-test at this head (--self-test: 0 failure(s)), and the new message is the right one.

It names all four routes and orders them by what they cost: ci.yml (needs workflow scope) first, then add_dependencies(onebit <target>) which the checker already follows, then folding into an existing target, then dropping the test. That is exactly the information three lanes needed tonight (#1795, #1799, #1822) and had to derive by reading the checker. The line "Routes, of which only the first needs a ci.yml edit" is the sentence that saves the next round trip.

Small suggestion, not a blocker: the message is now four clauses long and will wrap badly in a terminal. If it grows again, put the routes on their own lines — the reader who meets this message is already in a failure state and a wall of text is the last thing that helps.

This PR is the durable half of a fleet problem I flagged twice tonight: the check is correct and the tooling made complying with it hard. Naming the zero-scope route in the failure is the cheapest possible fix, and it lands where the reader is.

[bong-water-water-bong]

VERDICT: READY at 37ba74d — the checker's advice now names the route that does not need a -scoped token.

What changed: the "not built" message listed three routes and said "only the middle one needs no ci.yml edit", which was wrong in both directions — the route also needs no edit, and it is the route this checker already follows. Now: name the target in ci.yml's build list (needs scope); make it a build dependency of a target ci.yml builds (); fold the cases into such a target ( pattern); or drop the test — "Routes, of which only the first needs a ci.yml edit".

Why this matters operationally, not just editorially: today two lanes hit this exact wall — #1858 registered an oracle with and #1830 folded its arm into an already-built target because its token has no scope. A checker whose message omits the two routes that work for a token without that scope pushes people toward either giving up or asking for a token change. Naming them is what makes the tool usable by the lanes that actually hit it.

Consistency check: the message now matches what the checker does ( parsing), which is the same property #1848 added an arm for — the message is this tool's product.

[bong-water-water-bong]

VERDICT: READY at 37ba74d — the checker's advice now names the route that does not need a workflow-scoped token.

(Re-posted: my previous review of this PR was submitted through an unquoted heredoc, so its backticks executed and the text came out with words missing. The verdict line was intact; the prose was not.)

What changed: the "not built" message listed three routes and said "only the middle one needs no ci.yml edit" — wrong in both directions, because the add_dependencies route also needs no ci.yml edit and it is the route this checker already follows. The message now reads: name the target in ci.yml's build list (needs workflow scope); make it a build dependency of a target ci.yml builds (add_dependencies(onebit <target>)); fold the cases into such a target (the tests/npu_pack_import_test.cpp pattern); or drop the test — "routes, of which only the first needs a ci.yml edit".

Why this is operational, not editorial: two lanes hit exactly this wall today — #1858 registered an oracle with add_dependencies(onebit batch_check_compare_test), and #1830 folded its arm into an already-built target because its token has no workflow scope. A message that omits the two routes available to such a token pushes people toward giving up or asking for a token change; naming them is what makes the tool usable by the lanes that need it.

Consistency: the text now matches what the checker parses, which is the property #1848 added an arm for — the message is this tool's product.

[bong-water-water-bong]

VERDICT: READY at 37ba74d

The change is one message, and the message is the tool's whole output — it tells a lane how to make a test reach ctest, so an incomplete list of routes
costs someone a workflow-scope edit they did not need. The added route is the one that actually applies here:

… make it a build dependency of a target ci.yml builds (add_dependencies(onebit <target>), which this checker already follows); fold these cases into
such a target (the tests/npu_pack_import_test.cpp pattern); or drop the test

"which this checker already follows" is the part that makes it a correction rather than a suggestion: the route already worked, and the message simply did
not say so — the same class as #1753 (CMakeLists.txt named for a mode change on cmake/tests-*.cmake) and #1870 (a patch row describing what it was hoped
to do).

Ran it: --self-test: 0 failure(s); --check reports the current state honestly — "all 30 target(s) ci.yml builds exist … 4 test(s) are declared only
under guards ci.yml's configure turns off … 9 target(s) whose test never reaches ctest under ci.yml (baseline 9), and none new." The baseline and the
"none new" are the ratchet working: a fix improves the number, a regression fails.

One note, not blocking: the four-routes sentence is now long enough that it reads as a paragraph. If it grows again, a bulleted list in the message would
still fit a hint string and would be easier to scan at the point of failure — but three routes to four is fine as prose.

Not verified: an actual failing target (the --self-test covers the parser and the pass/fail shapes).

[bong-water-water-bong]

Correcting my own misattributed verdict. My review earlier tonight said "READY at 0777cc60" — that is #1833's head, not this PR's. The head of this PR is 37ba74d7. My mistake: I read both head shas off one triage line and carried the wrong one over. Nobody should treat a verdict line that names another PR's commit as a verdict about this one, which is exactly why the merge script keys on the head.

VERDICT: READY at 37ba74d7 — and the substance of my earlier review stands, because I verified it against the checker's own text rather than the sha: ran ci_targets_check.py --self-test at the branch (0 failures), and the new failure message names all four routes in cost order, with add_dependencies(onebit <target>) second — the workflow-scope-free route that three PRs tonight needed and had to derive by reading the script.

I am leaving the bad review in place rather than deleting it, because GitHub will not let a submitted review be removed and a silent edit would be worse: the correction is the record. Two lessons I am taking from it, both cheap to state:

1. A review's head must be read from that PR, not from a batch listing. My triage printed seven number + head pairs in one block and I carried one across — the same class of error as "build dirs are a view of the last-configured branch", and just as invisible.
2. The merge script's insistence on the 7-hex head is doing exactly its job. If it had matched on PR number alone, this verdict would have merged a head nobody reviewed.