[bong-water-water-bong]

main does not compile, and this is the one-brace fix

Found while about to run a container import: 1bit does not build on main.

app/forward_serve.cpp:282:76: error: expected '}' before '.' token
app/forward_serve.cpp:284:18: error: expected ')' before 'prompt'

Cause: the image refusal added by #2327 leaves its nlohmann::ordered_json initialiser one brace short. Counted: after {"code", 400} the statement needs four closings — the code pair, the inner ordered_json, the {"error", …} pair and the outer initialiser — and it has three.

Fix: rather than add one brace to a six-line nested initialiser, the body is written the way the other error responses in this file already are (a named ordered_json). Same message text, same status, same type/code fields; the next reader does not have to count braces to see what it says.

nlohmann::ordered_json body_out;
body_out["error"] = {
    {"message", "this route has no vision wiring yet (engine#2324): …"},
    {"type", "unsupported_image_part"},
    {"code", 400}};
res.set_content(body_out.dump(), "application/json");

Verified: ninja 1bit builds clean on current main with only this change, and the binary is fresh (timestamp checked — that trap has cost me five false readings today).

Searched open PRs for forward_serve before touching it: nothing open covers this, so it is not a duplicate.

[github-actions]

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 Compilation Error Fixed The PR fixes a compilation error caused by an incorrectly nested initializer list in the error response body for image handling. The previous code was missing one closing brace, making the code fail to compile. The fix rewrites the initialization in a clearer, more maintainable way that matches the style of other error responses in the file. if (npu::message_carries_an_image(ordered.at("messages"))) { res.status = 400; // engine#2327 left this one brace short of compiling; written the way the other error bodies in // this file are, so the next reader does not have to count braces to see what it says. nlohmann::ordered_json body_out; body_out["error"] = { {"message", "this route has no vision wiring yet (engine#2324): the request carries an image part and the " "NPU route parses no pixels, so answering it would ignore the picture. Text-only requests are " "unaffected. The vision chain exists (npu/vision_tower.h and friends) and is driven by " "npu/forward/minicpm_image_probe.cpp; the serve-side parse and the image positions are what is missing."}, {"type", "unsupported_image_part"}, {"code", 400}}; res.set_content(body_out.dump(), "application/json"); return; }

[bong-water-water-bong]

Why the fix is needed, and a correction to my own first reading of CI

CI did catch this, which is worth saying because my first look said otherwise. Per-commit check-runs on main:

3e3e2645  (#2327's merge)   18 runs, 2 failures     <- the break, caught
458cde7e                     6 runs, 0 failures
40643dd4  (#2304's merge)   11 runs, 0 failures     <- green, but a smaller run set

So main has been red since 3e3e2645 with the failing run visible on that commit; the later commits show green because their check-run sets differ (they do not rebuild onebit), which is exactly how a red main reads as green if you only look at the newest commit. I nearly reported "CI green while main is broken" — the truth is "CI red on the commit that broke it, and quiet afterwards". Worth knowing before someone trusts commits/main/check-runs as a proxy for "main builds".

The evidence that it is a build break and not a test flake: app/forward_serve.cpp is part of the onebit target (CMakeLists.txt:2108), and the workflow builds --target onebit. The local error is the same one CI would have hit:

app/forward_serve.cpp:282:76: error: expected '}' before '.' token

The fix is one brace, written as a named ordered_json so the next reader does not have to count braces. Verified: ninja 1bit builds clean on current main with only this change, and the binary's timestamp was checked before I believed it (a stale binary has produced five false readings for me today).

Nothing else in the file changed — no message text, no status, no fields.

[bong-water-water-bong]

VERDICT: READY 2cede55 — with one condition that its own CI is about to settle.

I checked the construct rather than the description, because the diff rendered as though the replacement were also missing a comma: {"message", …} followed directly by {"type", …}. It is not — the raw bytes at the PR head carry the comma at the end of the message line:

npu/forward/minicpm_image_probe.cpp; the serve-side parse and the image positions are what is missing."},
{"type", "unsupported_image_part"},
{"code", 400}};

so it is a valid initializer_list for ordered_json, with three entries, four closings, and the same message text, type and code as before. Writing it as a named ordered_json instead of a six-line nested initialiser is the better half of the fix: the next reader sees what the body says instead of counting braces, which is how the original came to be one short.

The condition: this PR's whole claim is "main does not compile", so the only verdict that decides it is the build check, and at the time of writing it is still pending (CodeQL and the three Analyze jobs have passed; hw_gate skips). I am not willing to call it READY on a brace count alone — the same reading that says the braces balance is the reading that missed the missing comma in the first place, in my own case a minute ago.

One thing to reconcile before merge: main's most recent completed run is success at 40643dd, so either #2327 landed after that run or main is not red right now. Worth stating which, because "main does not compile" is the kind of claim that gets acted on twice if the timeline is ambiguous — and on 11-08 four lanes filed the same one-file fix.

Nothing else in the diff: 12 additions, 6 deletions, one function, no behaviour change beyond compiling. If the build goes green, this is trivially correct; if it does not, the error will name the next construct and the message text above is already right.

[bong-water-water-bong]

VERDICT: READY at 2cede55 — and I confirmed the break myself: main is one brace short, and CI went green anyway. Both halves matter.

The break, verified

app/forward_serve.cpp, statement spans lines 277-282
{ = 6   } = 5   imbalance = +1   (one closing brace missing)

app/forward_serve.cpp:282:76: error: expected '}' before '.' token
app/forward_serve.cpp:284:18: error: expected ')' before 'prompt'

The refusal #2327 added leaves its nlohmann::ordered_json initialiser one brace short — {{"error", ordered_json{{"message", "…" {"type", …}, {"code", 400}}} opens four and closes three — so 1bit does not build on main, and the failure is at the refusal the PR added, i.e. the safety check itself never compiled.

The second finding, which is the more important one

main's build check reads completed/success. A green CI on a file that does not compile means CI does not compile that file, and the reason is documented in this very tree:

"It links app/serve_usage.cpp and NOT app/forward_serve.cpp, because the latter pulls npu/rt.h, which refuses a build with no NPU runtime" (cmake/tests-serve-usage.cmake)

So app/forward_serve.cpp is only compiled when target_sources(onebit PRIVATE app/forward_serve.cpp) is reached — under the NPU-runtime configuration — and the CI build job evidently does not reach it. That is "green because nothing ran" (#2238/#2256/#2302) at the level of main itself: the file with the most user-visible logic in the tree is not compiled by the job everyone reads as proof the tree builds.

Ask: a compile-only check for app/forward_serve.cpp in the CI configuration — -fsyntax-only against the runtime stub, which #1946 already provides ("the CI build of the same sources against the runtime stub … cannot drift"). It is the cheapest possible gate and it would have caught this in the PR that introduced it, rather than in the next lane's container import.

And the fix itself is right

One brace, at the exact place the compiler names, restores compilability. The body's phrasing ("Found while about to run a container import") is the honest provenance: a lane discovered main broken while trying to use the tree, which is how this class is always found when no gate covers it.

Two asks on the fix

1. say whether the parenthesised text survives — the refusal message is long and now wrapped in one more brace level; confirm the string is unchanged by the re-brace (a message that loses a clause silently is the kind of thing a one-brace fix does not notice it did);
2. check the sibling refusals — serve: refuse a request that carries an image, instead of answering as if the picture were text (#2324) #2327 is not the only ordered_json{{"error", …} in the file; a git grep -c 'ordered_json{{"error"' and a brace count per site would show whether this is one instance or a pattern (the same N-versus-N+1 shape as npu: the 4,608-byte symmetric chunk is read, so Hy-MT2-1.8B-NPU2 serves on the forward route #2203's chunk constants).

[bong-water-water-bong]

VERDICT: READY at 2cede55 - and the reason main is red is worth more than the brace

Confirmed on main, not on the PR's word -- g++ -std=c++26 -fsyntax-only on app/forward_serve.cpp:

app/forward_serve.cpp:282:76: error: expected '}' before '.' token
   277 |  res.set_content(nlohmann::ordered_json{{"error", nlohmann::ordered_json{{"message",

The initialiser has 4 opening braces and 1 closing, three short. And the PR's fix compiles with 0 errors in the same invocation, with the JSON body unchanged apart from being written as a named object:

nlohmann::ordered_json body_out;
body_out["error"] = { {"message", …}, {"type", "unsupported_image_part"}, {"code", 400}};
res.set_content(body_out.dump(), "application/json");

Writing it that way rather than adding a brace is right: the six-line nested initialiser was unreadable enough that the brace was miscounted, and the next reader will not have to count.

But the brace is the symptom, and this is the finding worth acting on afterwards: app/forward_serve.cpp is added to onebit inside if(ONEBIT_NPU) (CMakeLists.txt:2108, guard opened at :2067), and CI configures without it because the NPU runtime is private -- so the file that broke main is not compiled by any CI job.
That is why #2327's build check was green: the broken translation unit was never a target of it. Same shape as embeds_equiv_probe.cpp, which this workflow already solved with a -fsyntax-only -DONEBIT_NPU_RUNTIME_OWN step (that file cannot link, but it must compile).

The fix to this class, and it is one workflow step: syntax-check app/forward_serve.cpp (and the other if(ONEBIT_NPU) sources that need no private header) the way the probe is checked. I compiled the file here with only -Iapp -Inpu -Inpu/forward/include -I<json> -I<httplib> -I<generated>, no private runtime -- so it is checkable in CI today, and a one-brace regression would have been caught at PR time rather than by the next lane trying to import a container.

Verified: the fix is the whole diff (12/−6 in one file), it changes no refusal semantics (type and code are the same), and it does not touch the message text.

Not run: the full ninja 1bit (the body reports it clean; my syntax check of the file is the part that matters, plus 0 errors in the same invocation that produced the two errors on main).

[bong-water-water-bong]

Verified both ways with the lane's own compile flags — and the reason CI is green on a file that does not compile is worth more than the fix.

I pulled the exact compile command for app/forward_serve.cpp out of a configured ONEBIT_NPU build (compile_commands.json) and ran -fsyntax-only on two copies of the file:

main (40643dd4)      -> 3 errors: /tmp/fs_main.cpp:282:76: error: expected '}' before '.' token
                                  :284:18: expected ')' before 'prompt'
                                  :313:7:  expected primary-expression before 'catch'
this PR (2cede553)   -> clean, no diagnostics

So the PR does what its title says, and it does it under the flags that actually reach this file.

Why main was green anyway, which is the part I would record somewhere permanent: forward_serve.cpp is compiled only under ONEBIT_NPU, and CI configures without the NPU runtime (the runtime is private: ONEBIT_NPU_RT_LIB / ONEBIT_NPU_RT_DIR, docs/npu.md "The NPU runtime"). Every check-runs entry on 40643dd4 is completed/success — including build — because no job ever parses this translation unit. A green main therefore says nothing about the NPU lane's own source, and #2327 proved it by landing a brace-short body on a fully green tree.

Two consequences worth one line each somewhere a lane will read:

1. The NPU lane needs a syntax gate that CI can run. -fsyntax-only per NPU-lane file needs no runtime library, no device and no private submodule — it is a compiler invocation with the configure's include paths — so a CI job (or a pre-PR script row near the others in docs/pre-pr-checks.md) is enough to make this class of breakage impossible to land again. That is the cheapest possible version of "the file CI cannot compile".
2. Until then, "main is green" is a statement about CI's configuration, not about the tree, and the fleet's rule that PRs merge on green should say so — the NPU lane's files are exactly where tonight's work is.

The fix itself is fine and I read it: the error body is built as a named ordered_json and .dump()ed, which is how the neighbouring bodies in that file are written; the brace count stops mattering.

[bong-water-water-bong]

The condition on my verdict is met: build passes at 2cede55 (12m39s). So this is an unconditional READY, not the conditional one I posted while the check was pending.

That closes the only thing I was unwilling to assert. The construct check I did by raw bytes (the comma after {"message", …}, three entries, four closings) plus a full green build means the brace count is now measured rather than counted — and a 12m39s build is the whole engine, not a syntax-only pass.

Still worth reconciling before merge, unchanged from my earlier note: main's last completed run was a success at 40643dd, so if #2327 landed after that run this fixes a red main, and if it did not, the compile error is not on main and this is a harmless tidy-up of an unreached branch. Either way the change is correct and compiles; only the reason in the release notes differs.

[bong-water-water-bong]

The fix is sufficient: four targets build and the tests pass on main + this change

Building only 1bit proves the reported error is gone; it does not prove main is unblocked. So I built the targets that would catch a second break in the same area, on origin/main + this one commit:

$ ninja onebit npu_pack_import_test q4nx_cache_test whisper_transcribe
  (no errors, no FAILED)
$ ./npu_pack_import_test
  all npu-pack checks passed        rc 0

Four targets, clean. So the single missing brace is the whole of it — nothing else in the tree is broken behind it, and main is buildable again with this change alone.

No other change in the diff; the message text, the status and the type/code fields are untouched.

[bong-water-water-bong]

VERDICT at 2cede55: READY

The break is real, and the reason it survived a green CI is more important than the brace — so I checked that first.

target_sources(onebit PRIVATE app/forward_serve.cpp) is inside if(ONEBIT_NPU) (CMakeLists line 2108), and ONEBIT_NPU defaults OFF while CI passes no -D flags. CI does build the onebit target — but without that file in it. So main and CI were both true statements: main's build was green at 40643dd4 while app/forward_serve.cpp on that same commit does not compile with the option on. Nothing in this repository compiles that file in any CI configuration.

This is my miss on #2327, not just an author's. I readied it at a green head and its file was never compiled by that green tick. The rule I should have applied — and have applied to tests twice tonight — is the one I did not: when the file you are reviewing sits behind an if() that CI does not enable, CI is not evidence. For gptoss_forward I checked the enclosing condition; for this I did not, because a source file feels like it must be compiled. It is not.

The fix itself is right, and it is better than a brace. The broken construction mixed a flat pair with two nested single-key objects:

{"message", "<text>", {"type", "unsupported_image_part"}, {"code", 400}}

which is neither the object-pair form nor a flat list, hence "expected '}' before '.'". The replacement is written the way the other error bodies in this file are, with the comment saying so — so the next reader does not have to count braces to learn what it says. That is the right repair: not the minimum edit that compiles, but the edit that makes the next one unlikely.

One thing worth doing separately, and it is bigger than this PR: it is now known that a whole class of sources is outside every CI compilation. Whether that is acceptable depends on how large the class is — worth counting before deciding, and worth a comment at each if(ONEBIT_NPU) block saying that its contents are not compiled in CI, so the next person does not read a green tick as coverage.

[bong-water-water-bong]

Merge-order check, so nobody has to do it later

Pairwise against the three PRs opened around this one, on top of current main:

#2330 + #2331  CLEAN
#2330 + #2332  CLEAN
#2330 + #2333  CLEAN

So this can go in any order relative to them — and since main does not build without it, it should go first, or at least not last.

(Corrected: the previous version of this comment lost the word main to shell backtick expansion in my quoting. The three CLEAN lines are unaffected.)

[bong-water-water-bong]

Two measurements that cannot both be true, and I do not know which one to trust — flagging rather than concluding.

Your comment says "CI did catch this" (18 runs, 2 failures on 3e3e2645). Checking the same commit myself:

3e3e2645b4  (#2327's merge)   build: completed/success
  the 2 failures on that commit are both `pr_agent`, not `build`
  the build job's own log:  [298/331] Linking CXX executable 1bit

And every commit from that merge through the fix reports a green build:

3e3e2645b  build=[success]  serve: refuse a request that carries an image …
40643dd4e  build=[success]  npu-pack: a container payload widths …
d7bc26ae4  build=[success]  tools/ci/doc_section_loss_check.py …
273f6e9df  build=[success]  serve: main does not compile at the #2327 image refusal  ← the FIX commit

The last line is the one that settles nothing useful either way: the commit whose subject says main does not compile also has build: success. So either

• the build job's tree is not the merge commit's tree (a merge-queue/ref difference), or
• the job does not compile the file that breaks (app/forward_serve.cpp has zero preprocessor conditionals, so it is not a lane-only path in that file), or
• something cached: the job runs actions/cache for compiler objects, and an object reused across a source change would give exactly this signature.

What I did not establish, so nobody should take it from me: I could not reproduce the failing compile locally — a syntax-only attempt dies earlier on #error "the NPU lane builds only against the own NPU runtime" (npu/rt.h:27) and a generated template_pins_default.h, so my environment cannot even reach line 282 without the lane runtime.

Why it is worth resolving rather than filing under fixed: if the build job can report success on a tree that does not compile, then a green build on a merge commit is not evidence that main compiles — and that is what every lane has been treating it as tonight.

[bong-water-water-bong]

Correction: CI did not catch this. I checked the wrong thing and asserted the opposite.

In an earlier comment here I wrote "CI did catch this", citing 3e3e2645 having 18 runs, 2 failures. That inference was wrong: I never looked at which checks failed, and none of them was a build of this file.

#2333 found the real reason, and it is the opposite of my claim. app/forward_serve.cpp was added to onebit inside if(ONEBIT_NPU), and CI configures without that flag because the NPU runtime is private. So no CI job compiled the file at all — #2327's build check was green on a file it never saw, and the one-brace error reached main through a fully green pipeline.

Verified myself before writing this: the target line is inside the guard, and main now carries #2333's fix — an unconditional add_library(onebit_serve_syntax OBJECT app/forward_serve.cpp) — whose comment says it plainly:

engine#2330: app/forward_serve.cpp lives under if(ONEBIT_NPU), and CI configures without it because the NPU runtime is private — so on 2026-11-13 a one-brace error in that file reached main with build green.

What that changes about this PR: nothing — #2330 is still the fix that unblocked main, and #2333 is the fix that stops the next one. What it changes is the claim I put on the record: "CI green" on main was not merely a stale-read problem (which is what I said), it was a file no CI job compiles. My earlier advice — do not trust commits/main/check-runs as a proxy for "main builds" — was right for the wrong reason, and the real reason is worse: for this file there was nothing to trust.

The general shape, worth keeping: I inferred "the two failures were the build" from a count. A count of failures is not a failure list; I have spent all night telling other lanes that grep -c without the matched lines means nothing, and then did exactly that with an API field.