Repository navigation
serve: refuse a request that carries an image, instead of answering as if the picture were text (#2324) - #2327
Conversation
…s if the picture were text (engine#2324)
The NPU route parses no pixels: `app/` has zero `image_url`, `data:image` or base64 content parts, and nothing calls the
vision chain. But `messages_for_template()` passes a content-part ARRAY straight to the chat template, so a request with an
image reached the template and was answered -- a wrong answer with a 200, about a picture the route never read.
It is now refused by name, before rendering:
{"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}}
Two spellings count as a picture (`image_url`, the OpenAI form, and the bare `image` several local servers accept), and the
predicate deliberately does NOT match a `"type":"image_url"` string that appears inside a tool call's JSON arguments -- that
is text the template is entitled to see, and matching it would break the tool-call requests the same test file already covers.
Eight arms in `tests/npu_template_test.cpp`, both directions: a plain string content, a text-only array, a body whose messages
are not an array, parts without a string type, and the tool-arguments string are NOT pictures; an `image_url` part, a bare
`image` part, and one in a later message ARE. Built and run here: all eight pass.
This is (6)'s precondition, not (6). The vision chain is on main -- `npu/vision_tower.{h,cpp}` (`tower_forward(pack, pixels,
gh, gw, tower*)`), `vision_plan.h`, `vision_prompt.h` (`ImageMarkers`, `image_tokens`), `vision_image.{h,cpp}`,
`vision_pack.{h,cpp}` -- and `npu/forward/minicpm_image_probe.cpp` drives all of it on the host. What is missing is the
serve-side parse and the join, and (8)'s cache key: `npu_prefix_cache.h` matches a slot on token ids alone, so two different
pictures at the same placeholder positions would reuse the same KV state.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY -- a wrong answer with a 200 turned into a named refusal, with the false-positive direction armed
The class is the worst one in a serve path: messages_for_template() passed a content-part array straight to the chat template, so a request carrying an image was rendered and answered -- "a wrong answer with a 200, about a picture the route never read." An error is recoverable; a confident answer to a question nobody asked is not, because the client has no reason to retry.
The refusal is by name, before rendering, and says the useful thing -- that the chain exists (npu/forward/minicpm_image_probe.cpp) and only the serve wiring is missing, that text-only requests are unaffected, and it cites the tracking issue. "Refuses rather than guesses" is exactly the right behaviour for a route that parses no pixels.
Both spellings are covered (image_url, OpenAI's, and image, which several local servers accept) -- and, more importantly, the false-positive direction is armed:
- a plain string content -- not refused;
- a text-only content-part array -- not refused. That is a legal OpenAI shape and it is what most non-vision clients send; refusing it would have broken every normal request on the route, which is the cost of a predicate that only tests the positive case;
image_urlandimage-- refused;- a non-array
messagesvalue, parts with notype, and parts with a non-stringtype-- not refused.
Six arms, both directions, in a predicate with one job.
One gap, and it is the same gap one step over: the predicate is about images, and the route also has an audio chain it does not wire (whisper_transcribe, #1783). An OpenAI input_audio part would today render as text and be answered in exactly the same way -- a wrong answer with a 200. Either the predicate should be "this request carries a part this route cannot consume" (with the list being image and audio), or the body should say that audio is not covered yet and where it is tracked, so the next reader does not assume the guard is general.
Nothing else to raise: no pixels are parsed anywhere in app/ (verified in the body by the absence of image_url/data:image/base64 handling), the guard runs before rendering rather than after, and the message names the issue a reader would need.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 6643451
The predicate is correct on all eight claims -- I drove it directly, without the renderer, so the check does not depend on the fixture path:
g++ -std=c++26 -Iapp -Ibuild/generated -I<json> harness.cpp # calls onebit::npu::message_carries_an_image
harness: all 8 predicate arms hold
| body | verdict |
|---|---|
content: "hello" (plain string) |
not a picture |
content: [{"type":"text",...}] |
not a picture |
{"type":"image_url","image_url":{"url":...}} |
picture |
{"type":"image","data":"AAA"} |
picture |
| an image part in a later message | picture |
a body whose messages is not an array |
not a picture |
parts without a string type |
not a picture |
"type":"image_url" inside a tool call's arguments string |
not a picture |
That last row is the one that makes this safe to land: tool-call requests carry {"type":"image_url"} as text inside a JSON string argument, and a substring search for image_url would refuse them. Matching on the part's own
type field cannot, and the test file already covers those requests for other reasons -- so the guard is structural rather than a special case.
And the refusal is the right shape, which matters more than the predicate: it is refused by name before rendering, with "type": "unsupported_image_part", code: 400, and a message that says the route parses no pixels,
and where the vision chain already lives (npu/vision_tower.h, vision_prompt.h, driven by npu/forward/minicpm_image_probe.cpp) and what is missing (the serve-side parse, the image positions). A 400 that names the missing piece is the difference between "this server is broken" and "this feature is item (6)".
One note, and it is a coverage gap rather than a defect: the eight arms live in npu_template_test, and in an ONEBIT_JINJA=OFF build that binary returns 77 (skip) for the whole file -- I hit it (--no-renderer -> skip this build has no chat-template renderer), and my worktree is such a build. CI inits third_party/llama.cpp precisely so JINJA is on there ("without it … the renderer would reach a green build without ever being compiled"), so the arms do run in CI. But they are pure-JSON predicates that need no renderer, and a build without one loses this coverage entirely. The cheap fix if it ever matters: put the predicate arms in
their own ctest row (or above the renderer guard) so --no-renderer still exercises them.
Scope is stated honestly and I agree with it: this is (6)'s precondition, not (6) -- and the note about npu_prefix_cache.h matching a slot on token ids alone, so two different pictures at the same placeholder positions would reuse one KV state, is exactly the sort of thing that turns a vision feature into a wrong answer later. Naming it now is why the
follow-up will not ship without a cache key.
|
VERDICT: READY at The fault, with the measurement that locates it
Three greps establishing a zero, and then the one line that explains why the zero did not protect anyone: the array is forwarded verbatim, so the image arrives as a struct the template renders — and the model, which never saw a pixel, answers about it. That is worse than an error: a client that sends an image to this route today gets a plausible answer to a different question, and nothing in the response says so. Refusal is the right response, and the wording matters"refuse a request that carries an image, instead of answering as if the picture were text" — the ordering of the sentence is the whole design: the wrong behaviour is named first (answering as if the picture were text), because that is the thing to remember when someone proposes "just ignore the image parts". Ignoring is not neutral: it makes the endpoint lie quietly, which is precisely the Two asks
Why this belongs beside the "refused by name" family#2254 ( |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
READY at this head. CI build passes (10m46s); six pre-pr checks pass; the eight arms were built and run locally (npu_template_test with the repo's template fixtures): all eight pass.
The hazard is live today, not hypothetical: messages_for_template() passes a content-part array straight to the chat template and app/ parses no pixels, so an image request was answered as if the picture were text — a wrong answer with a 200. This refuses it before rendering, with a 400 that names engine#2324 and says what is missing.
The arm I would look at first is the negative one: a "type":"image_url" string inside a tool call's JSON arguments must NOT count as a picture — it is text the template is entitled to see, and matching it would break the tool-call requests the same test file already covers. The others: a plain string content, a text-only array, a non-array body and parts without a string type are not pictures; an image_url part, a bare image part, and one in a later message are.
Scope, stated so it is not over-read: this is (6)'s precondition, not (6). The vision chain is on main (npu/vision_tower.{h,cpp} tower_forward(...), vision_plan.h, vision_prompt.h, vision_image.{h,cpp}, vision_pack.{h,cpp}) and npu/forward/minicpm_image_probe.cpp drives all of it on the host; what remains is the serve-side parse and the join, plus (8)'s cache key (npu_prefix_cache.h matches on token ids alone, so two pictures at the same placeholder positions would reuse the same KV state).
And one thing I checked and rejected: app/serve.cpp also reads only type == "text" parts, but that is request_state(), the Laya/prefix helper — the HRX route's prompt goes to a child llama-server which handles --mmproj itself. The NPU route is the only one that silently dropped an image.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 6643451: READY
This is the worst kind of bug the fleet can ship — "a wrong answer with a 200, about a picture the route never read" — and the fix turns a confident wrong answer into a named refusal. The two things I checked were that the refusal is narrow and that it is pinned in both directions.
Why the bug was possible at all is the part worth keeping: the route parses no pixels (app/ has zero image_url, data:image or base64 parts, and nothing calls a vision chain) — but messages_for_template() passes a content-part array straight to the chat template. So the absence of vision was not the defect; the absence of a refusal in front of a code path that never needed to understand the content was. A route that cannot read a picture must say so, because the alternative is an answer that looks authoritative and describes nothing.
The detection is narrow in exactly the right place. message_carries_an_image requires content to be an array and each part to be an object with a string type — and the six arms pin the boundary from both sides:
content: "hello" -> not a picture (the plain form)
content: [{type:"text"}] -> not a picture ← the one that matters
content: [{…image_url…}] -> IS a picture
content: [{type:"image", data:"AAA"}] -> IS a picture
messages: {"not":"an array"} -> not a picture (malformed, no crash)
The third line is the arm that makes the fix safe. The bug was "an array reached the template", and the tempting over-correction is to refuse arrays — which would break every text request that uses the content-array form, the modern shape most clients send. Asserting that a text-only array is not a picture is what stops the fix from trading a wrong answer for a spurious 400, and it is precisely the negative arm a positive-only test would have missed.
And the error is worth reading as a template for refusals: it names the issue (engine#2324), says what the route does not do (parses no pixels), says what would have happened (answering it would ignore the picture), and scopes the change (text-only requests are unaffected). Four facts in one message — the reader knows whether they are affected and what to do about it.
No concerns. Refusing before rendering is also right: the template is where the image was silently absorbed, so the check has to precede it.
…short (fix-forward) (#2330) #2327 added the "this route has no vision wiring" refusal and left the nlohmann initialiser one `}` short, so `1bit` does not build on main at all: app/forward_serve.cpp:282:76: error: expected '}' before '.' token 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. Rather than add the missing brace to a 6-line nested initialiser, the body is written the way the other error responses in this file are written (a named ordered_json), so the next reader does not have to count braces to see what it says. The message text is unchanged. Found while about to import a container: the acceptance-case import needs a working `1bit`, and main had none. No open PR covered it (searched before touching it). Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
…GB, everything else refused by name (#2324) (#2329) * npu: the data: URL decode in front of the vision chain -- P6 to raw RGB, everything else refused by name (engine#2324) The vision chain is decoder-agnostic: npu/vision_image.h's preprocess() and vision_tower.h's tower_forward() both take raw RGB plus its geometry. What the serve route had no way to produce was those bytes, because an image part arrives as a data: URL and nothing in this tree decodes one. This is that one function. `app/npu_image_part.h`: DecodedImage img; if (!decode_image_data_url(url, img)) -> 400 naming img.error preprocess(img.rgb.data(), img.height, img.width, plan) **It decodes exactly one media type -- image/x-portable-pixmap, P6, maxval 255 -- and refuses the rest by name.** That is the same discipline as the route's refusal two commits ago (#2327): a PNG handed to a route with no PNG decoder must not become "an image that produced nothing". P6 is the format npu/forward/minicpm_image_probe.cpp already reads, so a picture that works in the probe works here, and adding a real decoder later is a change to this one function. Sixteen checks, built and run here (`npu_image_part: 16 checks ok`), both directions: decodes a 2x1 P6 to exactly its six samples; an omitted media type (`data:;base64,`) as P6 refuses, by name a PNG ("no decoder for media type 'image/png'"); a remote URL ("does not fetch remote images"); a P3 (ASCII) PPM; maxval 16; a 0-wide image; a header with no maxval; a payload that is short ("N of M sample bytes"); invalid base64; a non-base64 data URL The short-payload case is the one I would look at first in review: the chain downstream will happily read `w * h * 3` bytes out of whatever it is given, so the length check has to be here and it has to be exact -- a decoder that returns a buffer one row short is the same class of defect as a tap that reads one layer off. **This does not make images work on the serve route, and the route still refuses them.** #2327's refusal stands until the tower call and the image-position join land; this is the parse in front of them. When they land, the refusal is what those commits replace, and `npu_prefix_cache.h`'s key over pixels is the precondition for allowing the request near the cache at all. * npu_image_part: bound the header fields in int64_t, and say "at least" where the code means it Both review points, and the first is the one that mattered. ## 1. `read_uint` overflowed `int` before anything bounded it v = v * 10 + (raw[i] - '0'); // signed overflow is UB; a 14-digit width produced 829341693 The `w <= 0 || h <= 0` check caught the *result* -- which is why the review could say "nothing bad happened here" -- but the multiply was undefined behaviour, and this parses a **remote** `data: URL`, so a header claiming a width of 10^14 is a hostile value rather than a typo. Accumulated in `int64_t` now, refused as soon as it passes 2^31-1, with its own message: a PPM header field is out of range (a dimension is larger than 2^31-1) rather than being reported as "the header is truncated", which is what a wrapped int looked like. ## 2. "exact" was a claim the code did not make The check is `raw.size() < i + need`, i.e. **at least** `w*h*3` sample bytes, and a PPM stream may legitimately carry trailing data. Accepting it is right; saying "exact" in the body and the comment was not. The header now says why the test is "at least" and names the thing that *is* exact: the buffer handed downstream, `assign(begin+i, begin+i+need)`, never "whatever was decoded". Three arms hold both directions of that: a width past 2^31-1 -> refused, "out of range" (not "truncated") a 14-digit width -> refused a PPM with trailing bytes after samples -> accepted, and the buffer is exactly w*h*3 **21 checks, built and run here** (`npu_image_part: 21 checks ok`). What the review said is right about the part that matters, and I am repeating it rather than only fixing the two nits: the size comes from the *header's* `w*h*3` and is compared against the **decoded** count, so a short payload is refused with both numbers, and the downstream buffer is structurally exactly `w*h*3` -- not checked afterwards. --------- Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
serve: refuse a request that carries an image, instead of answering as if the picture were text (engine#2324)
The NPU route parses no pixels:
app/has zeroimage_url,data:imageor base64 content parts, and nothing calls thevision chain. But
messages_for_template()passes a content-part ARRAY straight to the chat template, so a request with animage reached the template and was answered -- a wrong answer with a 200, about a picture the route never read.
It is now refused by name, before rendering:
Two spellings count as a picture (
image_url, the OpenAI form, and the bareimageseveral local servers accept), and thepredicate deliberately does NOT match a
"type":"image_url"string that appears inside a tool call's JSON arguments -- thatis text the template is entitled to see, and matching it would break the tool-call requests the same test file already covers.
Eight arms in
tests/npu_template_test.cpp, both directions: a plain string content, a text-only array, a body whose messagesare not an array, parts without a string type, and the tool-arguments string are NOT pictures; an
image_urlpart, a bareimagepart, and one in a later message ARE. Built and run here: all eight pass.This is (6)'s precondition, not (6). The vision chain is on main --
npu/vision_tower.{h,cpp}(tower_forward(pack, pixels, gh, gw, tower*)),vision_plan.h,vision_prompt.h(ImageMarkers,image_tokens),vision_image.{h,cpp},vision_pack.{h,cpp}-- andnpu/forward/minicpm_image_probe.cppdrives all of it on the host. What is missing is theserve-side parse and the join, and (8)'s cache key:
npu_prefix_cache.hmatches a slot on token ids alone, so two differentpictures at the same placeholder positions would reuse the same KV state.