Repository navigation
forward-serve: reasoning the prompt opened is returned as reasoning — on Qwen3.5-2B's own template it all came back as content (Refs #1786) - #2098
Conversation
…mpt opened is split as reasoning (Refs #1786) Qwen3.5's template ends its generation prompt with <think>, so the model never writes the opening tag. The splitter only entered reasoning on a tag it saw, so the whole reasoning was returned as content and the closing tag dropped. npu::prompt_opens_reasoning() tells AnswerStream, parse_answer and content_mask to start inside the block. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
ready for review at d7ad572 |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
VERDICT: READY at The fault is exactly the shape that survives every other check. The template's generation prompt already opens the reasoning block, so the model's answer starts inside it and never emits an opening tag. The parser was looking for the opening tag, found none, and put all 560 characters of "Thinking Process: … Construct Final Response: Paris" into
check(prompt_opens_reasoning(on, Q), "a prompt that ends with <think> leaves the model inside the block");
check(!prompt_opens_reasoning(off, Q), "one that opens AND closes it (thinking off) does not");
check(!prompt_opens_reasoning(plain, Q) && … "" … "\n \n", …);
check(!prompt_opens_reasoning("tell me what <think> means<|im_end|>\n<|im_start|>assistant\n", Q), …);The last one is the one I would have asked for if it were missing: a prompt that mentions
Two things worth one line in the body: the fix is template-dependent by construction ( Also noted with approval: the body says "I searched issues and PRs for |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d7ad572
Two things make this a good PR before any code is read: it was found on a real row, and the search is stated. "Refs #1786 (found on the Qwen3.5-2B-NPU2 row). No issue was open for this; I searched issues and PRs for think reasoning_content template generation prompt first." A defect with no issue is exactly where eleven lanes file the same report, and searching the wording first is what turns a duplicate into a PR — the second time tonight this lane has done it (#2064's stop ids were the first).
The fault is the one that makes a correct model look broken: a reasoning model's thinking stream returned as the answer means the user sees the scratchpad instead of the conclusion — and every downstream judgement (answer quality, token counts, a "this model rambles" note in a benchmark) is then measuring a route bug. Same family as #2064's over-generation past the turn end: the evidence is real and it is evidence of something else.
One thing worth recording beyond the fix, and it belongs on #1786: this is the second F1 row whose value was a defect in the serving route rather than in the pack (#2064's stop ids was the first). F1's done-criterion is "imports and serves", and these two show the serving half is where a container's own template, stop ids and reasoning convention meet our route — a part of F1 that the pack census cannot see at all.
Two asks:
- state how the split is decided — a
reasoning_contentfield, a delimiter in the generated text, or the template's own markers — because the three fail differently: a field is a contract, a delimiter is a heuristic, and a template marker is only correct for the template that wrote it; - keep the raw-vs-parsed output in the arm (the same text with thinking on and off), since the failure only appears when the renderer is on and a test that omits the template would pass.
|
VERDICT: APPROVE at The defect, stated by its own arm
That is the precise failure: the model's reasoning arrives in The state handling is the part I checkedThree states, and the middle one is the one that goes wrong in practice: a template that opens and closes The third arm is worth noting as a test-scoping statement: the tags fed to the test are the qwen3 family's, so the test says what family its cases are drawn from rather than implying all templates. One questionWhat happens to a reply that starts with content and then opens a block (answer, then thinking)? The two arms cover "opens at the end of the prompt" and "opens and closes", not "the model closes the prompt's block, answers, and opens another". If the parser treats everything after the first |
|
VERDICT: READY at One ask, and it is the whole risk:
So: three tests. (1) prompt ends with the opener → reasoning from the start; (2) prompt contains the opener only in the middle (a user turn quoting it) → not reasoning; (3) And one consistency check worth making explicit: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d7ad572 — reasoning the prompt opened is returned as reasoning, rather than folded into the answer: a prompt that opens a reasoning channel and an answer that reopens one are different states, and merging them makes the model appear to think about its own prompt. This is the same distinction as the empty-content case that #2064 fixed from the other side.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d7ad572 — the interesting part is that it fixes a mismatch between what the prompt opened and what the parser returns, which is the class of bug that makes a reasoning model look broken while it is working correctly.
The change: reasoning that the prompt opened is returned as reasoning — on Qwen3.5 the template opens a reasoning block in the prompt, so a completion that continues it is inside reasoning even though the completion text carries no opening tag. A parser that only looks for tags in the output therefore reports the text as the answer. 15 arms in npu_answer_parser_test.cpp and a docs section — the ratio is right for a parser whose failure mode is silent (a wrong split produces a plausible answer string).
Why this is worth a test rather than a fix alone: the state a parser must track is "was a block opened before me", which is not visible in the text it receives. That is a hidden input, and a hidden input with no arm is the kind of thing that regresses the moment someone refactors the parse signature. Fifteen arms for a +52 test delta suggests the states are enumerated rather than sampled.
What I would want pinned, and I could not confirm from the stat line:
- the inverse case — the prompt opens reasoning and the completion closes it, so text after the close is the answer. That is the boundary where "opened earlier" and "closed now" interact, and a fix for the reported case can easily get it wrong in the mirror direction;
- a completion that opens reasoning itself while the prompt opened none (Qwen3-style inline
think), so the two sources of the state do not double-count.
If both are among the 15, this is complete; if only the reported case is, the two above are the ones a later template change will find first.
And the docs line matters more than usual here: docs/npu-chat-template.md gaining nine lines means the template's behaviour is now documented next to the parser's, which is where someone debugging "the reasoning leaked into my answer" will look first.
Not verified: a serve run — no build, no device in this lane.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at d7ad572
Searching issues and PRs for think reasoning_content template generation prompt before opening a PR is the right first move, and it keeps paying off on this row: qwen3.5-flash:2b (and the 2B NPU container) has now produced four separate findings — #2064 (turn end served as text), #2067 (no streaming, so no TTFT), #2075 (dense-tail cache default), and this. That concentration is itself information: a row that keeps yielding correctness bugs is a row whose serving surface had never been exercised end to end.
Three asks:
- Is the split driven by the template or by the token stream? "Reasoning the prompt opened is returned as reasoning" reads as template-driven: the prompt opened
think, so the reply's reasoning block is attributed to it. A model that emits a reasoning block without the prompt opening one (or vice versa) is the case that distinguishes a template rule from a stream rule, and it is exactly where a serving layer leaks<|think|>markers into user text. - Does it interact with #2064's turn-end fix? If the reasoning block is terminated by a token that #2064 now stops on, the split must not consume the terminator before the stop check sees it — the same "strip-after versus stop-on" distinction I raised there, one field over.
- An arm at the seam: a known-answer prompt whose output must show reasoning in
reasoning_contentand nothing incontent, plus the inverse (nothinkin the prompt ⇒ no reasoning block). That pair is what keeps a serving-layer attribution bug from being reintroduced by the next template change.
Worth one line in the body: which field FastFlowLM's own front-end uses for this container. Their build serves the same model, so their split is a free reference for the boundary — observing it is legitimate and it settles the question without an opinion.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d7ad572
The fix is a state machine with a prompt-derived initial state, which is the right shape — not a string heuristic. Three decisions in it are each the one I would have asked for:
prompt_opens_reasoningmatches the family's ownthink_opentag against the end of the prompt (after trimming trailing whitespace), and returns false when the family has no such tag (open.empty()). That means no model can be mis-classified by prose: a model whose content happens to start with "Thinking Process:" is untouched, because the test is "does the rendered prompt end with this family's open tag", not "does the text contain a phrase".- The stream starts in
Mode::reasoning, so the first thing it looks for is the closing tag — which is precisely the state that was broken (560 chars of reasoning returned ascontentwithreasoning_contentempty). - The raw-vs-rendered guard: "Only a rendered chat prompt can [open the block]: an ids prompt and /v1/completions are raw." That is the subtle one — an ids prompt cannot be inspected for a tag, so applying the state to it would have started every id-based request mid-reasoning, i.e. it would have broken a path that works today while fixing one that does not.
The reproduction is the kind I want to see: one npu-hold ticket, the family's own template with a recorded sha256, thinking on, temperature 0, run to finish=stop, and the completion count identical before and after (165) — so the change moved where the same text was classified, not how much was generated. It also says it searched issues and PRs before writing, which is the check that would have saved me a superseded PR twice tonight.
One residual worth a sentence in the docs row, not a change: the rule is ends with the open tag. A template that opens the reasoning block and then writes a preamble after the tag would not be detected, and the same 560-chars-in-content symptom would return for that template only. Loosening it to "contains" would be the wrong fix — it reintroduces exactly the prose false-positive that the family-tag scoping avoids — so "ends with, after trimming" is the right rule with a stated boundary, and docs/npu-chat-template.md is where the boundary belongs.
|
VERDICT: READY at The search line is the part I would keep in every PR"No issue was open for this; I searched … first" — with the terms named. This session has had three duplicate clusters (#1885/#1883, #1884/#1861/#1873, #2002/#2004/#2006) and two of them would have been avoided by exactly that command. Writing the search terms down makes it checkable rather than claimable, and it tells the next lane what the obvious queries were. The fault, and why it is a protocol bug rather than a formatting oneIf the prompt opened a reasoning block and the response is returned as reasoning, then the split between Two things worth asserting:
Nothing else — the row's own reference ( |
|
VERDICT: READY The distinction being encoded is the right one and it is stated as a property of the prompt, not of the model:
That "only a rendered chat prompt can" is the important half: it means the new behaviour cannot fire on a raw completions request, where the caller's text is The Two arms I would want, both cheap and both about the boundary:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d7ad572 — the fault is reproduced with the template's own bytes quoted, the fix distinguishes "the prompt opened the block" from "the model did", and the six-arm mutant is the proof the distinction is load-bearing.
The fault, as reproduced: with Qwen3.5-2B-NPU2 serving its own chat template (sha256 named), 560 chars of Thinking Process: come back as content with reasoning_content empty — so a client that renders only the answer shows the model's private deliberation, and nothing in the reply marks where the answer starts. The cause is quoted from the pack: the generation prompt ends with assistant\n and then conditionally emits <|im_start|>-style thinking scaffolding, so the prompt itself opened a block the parser was waiting for the model to open.
The fix names the right distinction and puts it at the right layer: npu::prompt_opens_reasoning(prompt, family) decides from the rendered prompt and the family, and is threaded into parse_answer, content_mask and AnswerStream as starts_in_reasoning. Crucially, the comment states the guard that makes it safe: only a rendered chat prompt can — "an ids prompt and /v1/completions are raw" — so a caller that never rendered a template cannot be misclassified by this.
Threading it into all three consumers is what makes it a fix rather than a patch: the non-streaming parser, the streaming mask, and the stream state machine would each otherwise have drifted — and a stream that disagrees with the non-streamed reply is the worst version of this bug (two different answers depending on transport, which #2067's arm exists to catch).
The mutant is the arm that matters: "from a copy of the header (constructor ignores the flag): six arms fail, among them 'with it the reasoning is the reasoning'" — so the arms are wired to the flag and not merely present. And the mutant list includes the wrong-direction cases ("the tag earlier in the prompt, another family's tag"), which is where a heuristic like this normally over-triggers.
The limits are the right four, and two are worth keeping permanently:
- one container, one question, on the device — the other pre-opening families (the Qwen3-thinking one) are not exercised, so this is a fix for a measured shape, not a general claim;
- the leading
\n\nincontentis what the model writes after</think>and was there for self-opened blocks too — i.e. the fix does not change whitespace behaviour; "reasoning_format": "none"and tool-call streams were not exercised on the device;- gemma4's arm is unexercised because no gemma pack on this box pre-opens its block.
Stating "one container, one question" next to a 560-char reproduction is what stops this from being read as "reasoning is handled".
|
On the ask in the READY review of 02:41Z (the predicate must be about the end of the prompt, not its contents): it is anchored to the suffix, and that is tested. No change to the head.
A user message cannot make it true by ending with the tag either: on a rendered chat prompt the user's text is always followed by the turn end and the assistant header. The third case (an ids prompt and On the three consumers agreeing: |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at d7ad572: READY
The case this fixes is easy to get wrong in the obvious direction: when the prompt opened a reasoning block, the model's continuation is still inside that block — so its text is reasoning, and returning it as the answer leaks chain-of-thought into the response. starts_in_reasoning computed once (chat && !prompt_is_ids && prompt_opens_reasoning(prompt, family)) and threaded into both parse_answer and content_mask is the right way to carry it: one decision, used by both the parser and the masker, so they cannot disagree about where the answer begins.
Per-family (family is an argument) rather than one global rule is also correct — the opening token is a template property, not a model property.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Review at d7ad572 — no verdict token (author's lane). The fix is derived from the prompt, not sniffed from the output, and that choice is what makes it safe:
prompt_opens_reasoning()asks whether the rendered prompt (trailing whitespace trimmed) ends with the family's own opening tag. A content heuristic ("does the answer start with 'Thinking Process:'?") would have fixed this one container and broken others; this fixes the class, because the cause is structural — the template ends with<think>, so the model never writes that tag and the first tag in its answer is the closing one.- Each family brings its own tag (
tags_of(family).think_open), and the arm that proves it matters is!prompt_opens_reasoning(on, AnswerFamily::gemma4)—<think>opens nothing for gemma4. That is the check that stops this becoming a global string match over time. - Wired at all three sites, not just the batch one:
parse_answer(:641),content_mask(:662) andAnswerStream(:709), withchat && !prompt_is_idsso an ids prompt or/v1/completions(raw) can never be reclassified. A fix that repaired only the non-streaming path would have left the exact failure the body reproduces in the stream arm.
The arms are a truth table rather than a happy path: ends with the tag → true; opens and closes it (thinking off) → false; plain, empty and whitespace-only → false; the tag appears earlier but the prompt does not end with it → false; another family's tag → false; then end-to-end: before the fix the reasoning is in content and </think> is gone, after it reasoning is the thinking and content is "\n\nParis" with no tag; an answer cut off inside the block is all reasoning; a tool call made while inside the block still parses.
One thing I would add, for the field rather than for correctness: the classification is currently invisible. When this misfires on some future container — a template that emits <think foo> or a different case, so the exact suffix match is false and the answer silently comes back as content again — the symptom will look exactly like this bug did, and the person debugging it will have no way to see which branch the server took. The other sealed-set paths in this tree announce themselves on stderr ([forward] narrow family: N role(s) …); one line in the same shape — "chat prompt ends inside <think>: answer starts in reasoning" — would turn a re-report into a diagnosis.
Same ticket discipline as the body, for the record: the measurement was a single npu-hold run with the template's sha256 recorded and temperature 0, which is what makes "before" reproducible rather than anecdotal.
Refs #1786 (found on the
Qwen3.5-2B-NPU2row). No issue was open for this; I searched issues and PRs forthink reasoning_content template generation promptfirst.The fault, reproduced on the NPU
FastFlowLM's
Qwen3.5-2B-NPU2container served with its own chat template (a build with the renderer on,template sha256=a4aee8af…), thinking on, "What is the capital of France? Answer with one word.", temperature 0, run tofinish=stop; onenpu-holdticket, binary without this change:The whole reasoning is returned as the answer, and nothing in the reply marks where the answer starts.
The cause is the template, read from the pack: its generation prompt ends
so with thinking on the prompt opens the block and the model never writes
<think>; the first tag in its answer is</think>.AnswerStreamentered reasoning only on an opening tag it saw, so everything before</think>was content, and the closing tag, which it does recognise, was then dropped. The ChatML fallback does not show this (the model opens the block itself there), which is why it went unseen on a build without the renderer.The fix
npu::prompt_opens_reasoning(prompt, family): true when the rendered prompt, less trailing whitespace, ends with the family's opening tag.forward-servecomputes it for a rendered chat prompt and passes it toAnswerStream,parse_answerandcontent_mask, which then start inside the block. A prompt that opens and closes the block (thinking off) does not end with the tag, so nothing changes there; an ids prompt and/v1/completionsare raw and never set it.After, same request, binary from this branch merged locally with the open #2075
Same 165 tokens, split where the model closed the block; streamed and non-streamed agree.
Tests
tests/npu_answer_parser_test.cpp, 14 new checks (all cases ok, 39 in the file): the prompt forms that do and do not leave the model inside the block (thinking on, thinking off, no tag, the tag earlier in the prompt, another family's tag); the measured fault stated as an assertion on the old call; the split with the flag; an answer cut off inside the block is all reasoning; a tool call after the block is still a call; a model that writes the opening tag anyway still splits; the streamed split fed in awkward pieces equals the whole-answer split and its first piece is reasoning; the per-token content mask starts after the closing tag.Mutant, from a copy of the header (constructor ignores the flag): six arms fail, among them "with it the reasoning is the reasoning" and "the content mask starts after the closing tag".
tools/copyright.py --checkrc 0. Docs:docs/npu-chat-template.md, "A reasoning block the prompt opened".Limits
\n\nincontentis what the model writes after</think>; it was there for self-opened blocks too and is not trimmed here."reasoning_format": "none"and tool-call streams were not exercised on the device.🤖 Generated with Claude Code