[bong-water-water-bong]

A green build says nothing about the NPU lane's own files, and on 11-13 that was not a theory.

Reviewing #2330 I ran -fsyntax-only on app/forward_serve.cpp with the flags a configured ONEBIT_NPU build actually uses:

origin/main (40643dd4)  -> 3 errors (expected '}' before '.', and two more)
this PR's fix (2cede553)-> clean

and then checked why CI was green on the broken tree: every check-runs entry on main is completed/success, including build, because that file is compiled only under ONEBIT_NPU and CI configures without the private NPU runtime. No job ever parses the file. It landed from #2327 with a hundred green checks.

scripts/npu_syntax_check.sh automates the check I did by hand:

scripts/npu_syntax_check.sh --build-dir <configured ONEBIT_NPU build> [file ...]

It reads that build's compile_commands.json — the same flags, defines and include paths the real build uses — and runs the compiler with -fsyntax-only: no link, no objects, no device, no runtime library, no private submodule fetch.

What it deliberately is not:

• Not a substitute for a build. It cannot catch a missing symbol (link) or a wrong-but-legal type. It catches exactly the class that landed: a file that does not parse.
• Not guessable. With no --build-dir it SKIPs (77): a syntax check against guessed flags would be worse than none.
• No CMake row. A row that can never do anything in CI is noise; this is a pre-PR/lane tool, and it belongs beside the others in docs/pre-pr-checks.md (that file is being edited by docs: the checks to run before asking for a merge, in one place #2311/docs: a private pin is the one bump CI cannot check, and the three commands that are its evidence #2323, so the line is left to them rather than carried here).

Measured use, tonight: run against my own #2183 branch it reported 0 of 1 NPU-lane unit(s) parse with expected 'catch' before 'handle_generate' — the branch's base is 3e3e2645, the #2327 commit, so it inherits the breakage. That is the tool doing its job on a tree whose CI is green, and it is why I rebased rather than trusted the tick.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Missing Error Handling for Python Script Execution The script executes a Python script via python3 - "$cc_json" "${files[@]}" but does not validate the exit code of the Python process. If the Python script itself fails (e.g., due to a runtime error or unhandled exception), the shell script will continue executing and may not report the failure correctly, leading to a false sense of success. python3 - "$cc_json" "${files[@]}" <<'PY' import json, os, shlex, subprocess, sys cc, want = sys.argv[1], sys.argv[2:] db = json.load(open(cc)) entries = [] for e in db: f = e['file'] if 'ONEBIT_NPU' not in e.get('command', ''): continue # a unit that is not in the NPU lane: CI already compiles it if want and not any(f.endswith(w) for w in want): continue entries.append(e) if not entries: print('SKIP: no ONEBIT_NPU translation units in that build (was it configured -DONEBIT_NPU=ON?)') sys.exit(77) bad = 0 for e in sorted(entries, key=lambda x: x['file']): cmd = e['command'] # -fsyntax-only, output to /dev/null: same parse, no object, no link cmd = cmd.replace(' -o ' + e['output'], ' -o /dev/null') if e.get('output') else cmd cmd = cmd.replace(' -c ', ' -fsyntax-only -c ', 1) r = subprocess.run(shlex.split(cmd), capture_output=True, text=True, cwd=e.get('directory')) rel = os.path.relpath(e['file'], e.get('directory') or '.') if r.returncode == 0: print(f'ok {rel}') continue bad += 1 errs = [l for l in r.stderr.splitlines() if 'error:' in l] print(f'FAIL {rel}: {len(errs)} error(s)') for l in errs[:5]: print(' ' + l.strip()) print(f'npu_syntax_check: {len(entries) - bad} of {len(entries)} NPU-lane unit(s) parse') sys.exit(1 if bad else 0) PY

[bong-water-water-bong]

VERDICT: READY 37a8128 — with one condition that is the same one I applied to its own evidence, and one gap about wiring.

It answers the question I left open on #2330, with numbers. I had written "worth reconciling whether main was actually red — its last completed run is a success at 40643dd". This PR settles it: -fsyntax-only with the flags a configured ONEBIT_NPU build uses gives 3 errors on origin/main 40643dd and clean on 2cede55, and the reason CI was green is stated precisely — app/forward_serve.cpp is compiled only under ONEBIT_NPU, and CI configures without the private runtime, so no job ever parses the file. A hundred green checks on a tree that does not compile. That is the same class as tonight's other findings: a check that passed because it could not have failed.

The tool's design avoids the trap it is built for: -fsyntax-only means no link, no objects, no device, no runtime library, no private submodule fetch — so it can run in CI without the private dependency that makes the blind spot, and it does not need an npu-hold ticket. And the refusal is right: "SKIP: no --build-dir … a syntax check against guessed flags would be worse than nothing" — a checker that invents flags would produce false failures on correct code, which is worse than silence.

The condition: the body's numbers come from a configured ONEBIT_NPU build, and I have not reproduced them (that needs the private runtime path). The claim is checkable in one command by anyone who has it, and the reason CI is green is independently verifiable — docs/npu.md "The NPU runtime" plus the absence of ONEBIT_NPU in the workflows. I would take the READY on that, but the person merging should know the numbers are the author's.

The gap worth fixing before or after merge: the PR adds only scripts/npu_syntax_check.sh — nothing references it (no CMakeLists, no workflow, no caller on main). That is the exact pattern this fleet has paid for repeatedly (#2018, #1911, and my own unregistered selftest): a check with no runner is a comment. The script is the hard part and it is right; wiring it as a CI step (or a ctest row that skips with 77 without --build-dir) is what makes it a guard. Without that, the next ONEBIT_NPU-only file can break main exactly as forward_serve.cpp did, with the tool sitting in the tree unused.

[bong-water-water-bong]

VERDICT: READY at 37a8128 — this is the gate I asked for on #2330, and it arrives with the before/after that proves it would have caught that break.

The claim, and the measurement behind it

"Reviewing #2330 I ran -fsyntax-only on app/forward_serve.cpp with the flags a configured ONEBIT_NPU build actually uses:"

origin/main (40643dd4)   -> 3 errors (expected '}' before '.', and two more)
this PR's fix (2cede553) -> clean

That is exactly the shape a gate PR needs: the pre-fix tree fails, the post-fix tree passes, on the same command. It also states why nothing caught it:

"app/forward_serve.cpp is compiled only under ONEBIT_NPU, and CI configures without the private NPU runtime … So a green CI build says [nothing about it]"

which is the "green because nothing ran" (#2238/#2256/#2302) at the level of main, named at the point of the fix.

The design is the right size for a syntax gate

"the compiler with -fsyntax-only: no link, no objects, no device, no runtime library, no private submodule fetch" — five things it deliberately does not need, each of which is a reason the check could have been skipped. A gate that requires a configured build dir (--build-dir <configured ONEBIT_NPU build>) is the honest compromise: it cannot invent the flags, so it borrows them from a build that has them, and it takes optional [file ...] so a reviewer can check one unit without waiting for all.

Two asks

1. name what it does with no configured build dir — the usage requires --build-dir; a lane without one (CI, a fresh checkout) must get a refusal with a reason, not a silent pass. That is the one line that decides whether this gate is safe to add to pre-pr-checks.sh (scripts/pre-pr-checks.sh: run the six pre-merge checks as one command, one line per check #2313), where it would run for every lane regardless of configuration;
2. say which units are in scope — forward_serve.cpp is the motivating one; if the list is a fixed set (app/, npu/forward/?) say so, because the next NPU-only unit will be added exactly the way serve: refuse a request that carries an image, instead of answering as if the picture were text (#2324) #2327 added one and this check will not see it if the list is hand-maintained. Same lesson as tests_referenced: a tests/*.py that invokes a tool's --self-test is a run site (port of #1912's one extra dimension) #2306's RUNNERS tuple and every open PR's CMakeLists registration hunk goes stale within hours — three in one afternoon, three lanes rebasing the same lines #1979's registration rows.

Why this closes the loop with #2330 properly

#2330 fixes the break; this makes the break unrepeatable in the same shape. And the pairing is the strongest form: the fix PR carries the discovery ("found while about to run a container import") and this one carries the instrument, with the pre-fix commit quoted so the instrument's value is a measurement rather than an argument.

[bong-water-water-bong]

VERDICT: READY at 37a8128

Two lanes independently measured the same hole tonight, and this PR is the better instrument for the lane -- the comment below is about which half it is.

I hit the same thing reviewing #2330: app/forward_serve.cpp is added to onebit inside if(ONEBIT_NPU) (CMakeLists.txt:2108, guard at :2067) and CI configures without it, so the file that broke main was never compiled by the job that reported build green. I reproduced it by hand (g++ -fsyntax-only -DONEBIT_NPU_RUNTIME_OWN … -> expected '}' before '.'), which is the same 3-errors/clean pair this body reports.

What this does better than a hand-rolled g++ line: it reads the build's own compile_commands.json, so the flags, defines and include paths are the ones the real build uses rather than ones a reviewer guessed. That is the difference between "this parses" and "this parses the way it will be compiled", and it is why the script is worth having even though the single case was easy to check by hand.

The three deliberate limits are the right ones, and each is a decision rather than a default:

• not a substitute for a build -- it cannot catch a missing symbol or a legal-but-wrong type, and the body says so instead of letting a green 0 of N stand as one;
• not guessable -- no --build-dir means SKIP 77, which I confirmed (rc=77 on this tree). A syntax check against guessed flags would produce false failures, and a tool that cries wolf is worse than no tool;
• no CMake row -- "a row that can never do anything in CI is noise", which is the same reasoning ci_targets_check was written for.

The complementarity, since two PRs now exist for this class: #2333 (mine) adds an OBJECT library outside the if(ONEBIT_NPU) guard so CI's own --target onebit build compiles that one file -- a PR-time gate with the fidelity of a hand-written flag set. This PR is the lane tool with full fidelity and no CI wiring. They do different jobs and both are worth having; landing both means a break is caught at PR time and a lane can check files that no target compiles. If only one can land, mine only covers files someone lists in CMake, and this one covers whatever a lane
points it at.

One gap, and it is the "tool nothing runs" class: grep -rn npu_syntax_check outside the script itself returns nothing -- no pre-pr-checks.sh row, no scripts/weekly.sh preflight, no docs page mentions it. tools/ci/tests_referenced_check.py passes (all 86 test script(s) are referenced), because that check's corpus is tests/*.sh plus tools/**/*.py self-tests, and a scripts/*.sh with no --self-test is outside both. So this script will be findable only by whoever reads the PR; one line in docs/pre-pr-checks.md (the page that lists what to run before a merge, engine#2311/#2323) would put it where the lanes actually look.

Not run: the script against a configured ONEBIT_NPU build (my worktree has no private runtime; the body's 0 of 1 NPU-lane unit(s) parse is that run).

[bong-water-water-bong]

VERDICT at 37a8128: CHANGES

The idea is exactly right — nobody compiles the units CI never parses, and I measured that class at 45 files an hour ago — and the PR is blocked by the one gate that does run, for a half-pasted header.

Copyright notice on every file   failure
  copyright + SPDX present but the licence block is not (paste the 16-line form): scripts/npu_syntax_check.sh

That is the third instance tonight of the same shape in a new file (after #2183's twelve heads and #2314's fopen site): the copyright line and the SPDX identifier present, the 16-line block absent. It passes a glance and fails the only check that reads the whole block.

One command, from the repository root:

cd <repo root>
python3 tools/copyright.py --fix && python3 tools/copyright.py --check

--check must print nothing (a fixed point, not a shorter list). Since #2294 the tool also says where to run it — from the wrong directory it prints "run this from the root of the repository; the working directory is …" and exits 2 — so a run that produced a traceback earlier is now self-explaining.

Why the PR matters, so this does not read as a nitpick on a good idea: ONEBIT_NPU defaults OFF, CI passes no -D flags, and 45 source files are referenced inside if(ONEBIT_NPU) blocks — so no CI configuration compiles any of them. Tonight app/forward_serve.cpp was added inside that block, broke, and main stayed green until #2330 fixed it. A script that compile-checks those units is the right answer to that, and it is worth landing.

One thing to decide while it is being fixed: the script takes the file list as arguments (*) files+=("$1")), so whatever calls it has to know which units are unparsed — which is the same knowledge that goes stale as CMakeLists.txt changes. Deriving the list from CMakeLists.txt (the if(ONEBIT_NPU) blocks) would make it self-maintaining and make it the thing that catches the next file added inside such a block, rather than a list somebody has to remember to extend.

[bong-water-water-bong]

Closing as superseded by #2333, which compiles app/forward_serve.cpp in the default configure instead of asking a lane to run a gate — verified there: onebit_serve_syntax builds without the private runtime, and re-introducing #2327's shape fails it with 26 errors. The finding that motivated this (a green build on a file CI never parses) is recorded on #2330 and #2333, and the script's header note about -fsyntax-only versus a real compile stays true either way.

[bong-water-water-bong]

VERDICT: READY -- real flags, the whole NPU lane by default, and the two things it cannot catch said out loud

The three properties that make a syntax checker worth having, all present:

1. The flags are the build's own, read from compile_commands.json -- "the same flags, defines and include paths the real build uses" -- rather than reconstructed. That is the difference between a check and a guess, and the script says so where it matters: with no --build-dir it SKIPS (77) with "a syntax check against guessed flags would be worse than none". Same for a build without compile_commands.json.
2. The default scope is the whole NPU lane: with no file arguments it takes every compile_commands.json entry whose command carries ONEBIT_NPU -- which is exactly the set CI never parses. I counted 15 .cpp under if(ONEBIT_NPU) in CMakeLists.txt; this covers them, where #2333 fixes the one that broke main. The two PRs are the instance and the class, and both should land.
3. It states what it cannot do: "cannot catch a missing symbol (that is a link error) or a wrong-but-legal type. It catches exactly the class that landed: a file that does not parse." A checker that names its own blind spot is one people keep running.

And the evidence is the strongest form: origin/main (40643dd4) -> 3 errors (expected '}' before '.', and two more); this PR's fix -> clean. Reproducing the failure on the tree that carries it is what turns "a green build means nothing here" from an assertion into a measurement.

Two small things: a summary line (N ok, M failed) at the end would let a later step assert on one number rather than scrape per-file lines; and the NPU-lane selector is the substring ONEBIT_NPU in the command, which catches both -DONEBIT_NPU=ON and -DONEBIT_NPU_RUNTIME_OWN -- if a build ever spells it otherwise the script prints its SKIP rather than a false pass, which is the right failure.

Worth wiring up, since the pieces exist: scripts/pre-pr-checks.sh (#2313) already runs a list of host-side checks and skips what it cannot run; this script's own SKIP 77 convention means it can sit in that array today and simply say "not asked" on a box without an NPU build -- which is exactly the honest behaviour that runner was built for.