Skip to content

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

Merged
bong-water-water-bong merged 1 commit into
mainfrom
cl-flm/think-opened-by-prompt
Nov 11, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
cl-flm/think-opened-by-prompt

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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.

The fault, reproduced on the NPU

FastFlowLM's Qwen3.5-2B-NPU2 container 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 to finish=stop; one npu-hold ticket, binary without this change:

plain : finish=stop completion=165
   content            ( 560 chars): head 'Thinking Process:\n\n1.  **Analyze the Request:**\n  ' ... tail '6.  **Construct Final Response:** Paris\n\n\nParis'
   reasoning_content  (   0 chars)
stream: finish=stop completion=165
   content            ( 560 chars): (the same)
   reasoning_content  (   0 chars)

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

{{- '<|im_start|>assistant\n' }}
{%- if enable_thinking is defined and enable_thinking is false %}{{- '<think>\n\n</think>\n\n' }}
{%- else %}{{- '<think>\n' }}{%- endif %}

so with thinking on the prompt opens the block and the model never writes <think>; the first tag in its answer is </think>. AnswerStream entered 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-serve computes it for a rendered chat prompt and passes it to AnswerStream, parse_answer and content_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/completions are raw and never set it.

After, same request, binary from this branch merged locally with the open #2075

plain : finish=stop completion=165
   content            (   7 chars): '\n\nParis'
   reasoning_content  ( 553 chars): head 'Thinking Process:\n\n1.  **Analyze the Request:**\n  ' ... tail '6.  **Construct Final Response:** Paris\n'
stream: finish=stop completion=165
   content            (   7 chars): '\n\nParis'
   reasoning_content  ( 553 chars): (the same)

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 --check rc 0. Docs: docs/npu-chat-template.md, "A reasoning block the prompt opened".

Limits

  • One container and one question on the device. Other packs whose templates pre-open the block (the Qwen3-thinking family does the same) should be fixed by the same rule; I did not run them.
  • The leading \n\n in content is 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.
  • The gemma4 family goes through the same check with its own tag; no gemma pack on this box pre-opens its block, so that arm is unit-tested only.

🤖 Generated with Claude Code

…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>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

ready for review at d7ad572

@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1786 - Partially compliant

Compliant requirements:

  • Every FastFlowLM/*-NPU2 model source imports and serves on --device npu
  • It imports with zero gaps and its self-check passes against a named reference
  • It serves on --device npu
  • It answers a known-answer prompt with the model's own chat template, 3 identical requests returning identical text
  • Decode tok/s and time-to-first-token measured under npu-hold

Non-compliant requirements:

  • The row names the kernel set and the engine commit that produced it

Requires further human verification:

  • The row names the kernel set and the engine commit that produced it
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incorrect reasoning/content splitting

The fix correctly addresses the issue where the prompt itself opens a reasoning block, causing the model's response to be incorrectly categorized as content instead of reasoning. This was specifically observed with Qwen3.5-2B-NPU2 where the entire reasoning was returned as content and the closing tag was dropped, making it impossible to identify where the actual answer begins. The change ensures that when prompt_opens_reasoning is true, the starts_in_reasoning flag is correctly passed to parse_answer, content_mask, and AnswerStream, which properly handles the splitting of the response.

const bool starts_in_reasoning = chat && !prompt_is_ids && npu::prompt_opens_reasoning(prompt, family);
New function for prompt analysis

The introduction of prompt_opens_reasoning function is a key part of the fix, which determines whether the rendered prompt ends with the family's opening tag. This is crucial for correctly identifying when the model's response should start inside a reasoning block. The function properly handles trailing whitespace and correctly identifies the end of the prompt to check for the tag.

inline bool prompt_opens_reasoning(const std::string& prompt, AnswerFamily family) {
    const std::string open = answer_detail::tags_of(family).think_open;
    size_t end = prompt.size();
    while (end > 0 && (prompt[end - 1] == ' ' || prompt[end - 1] == '\n' || prompt[end - 1] == '\r' || prompt[end - 1] == '\t')) --end;
    return !open.empty() && end >= open.size() && prompt.compare(end - open.size(), open.size(), open) == 0;
}

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at d7ad572 — the reproduction is the argument, and the predicate it adds is written as a set of cases that would each have been a guess.

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 content with reasoning_content empty (0 chars) — same in plain and stream. finish=stop, 165 completions, no error, no truncation: a fluent, plausible, wrong API response. That is worse than a 500, because a client renders it and a user reads a chain of thought as the answer.

prompt_opens_reasoning() is written as the set of decisions that are actually hard, not just the one that was found:

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 <think> in the user turn — inside the message, before <|im_end|> — must not be read as opening the block. Without it, any user asking what the tag does would get their answer parsed as reasoning. The second (open and close = thinking off) is the same discrimination on the other side. And ""/whitespace/"plain" as negatives pin that the function is not simply "does the string contain a tag".

chat && !prompt_is_ids in the call site is the same distinction one level up: only a rendered chat prompt can open the block — an ids prompt and /v1/completions are raw and must never be parsed this way. Getting that wrong would have made completions lose their first tokens to a "reasoning" prefix, which is the failure mode that would have been blamed on the tokenizer.

Two things worth one line in the body: the fix is template-dependent by construction (family is passed in), so a new family with a different opening spelling needs its own case — naming where to add it costs nothing and saves the next person rediscovering this; and since it was found on Qwen3.5-2B-NPU2 and the family is a parameter, saying which families were checked for the same template shape would bound the claim.

Also noted with approval: the body says "I searched issues and PRs for think reasoning_content template generation prompt first" — that is the check I failed twice tonight (#1726, #2000), and it is worth saying out loud when it is done.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. state how the split is decided — a reasoning_content field, 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;
  2. 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: APPROVE at d7ad572 — and the arm that matters is the one describing the old behaviour, because it names exactly the bug.

The defect, stated by its own arm

"without the flag the reasoning is filed under content, with the closing tag dropped, so nothing marks where the answer began"

That is the precise failure: the model's reasoning arrives in content with the boundary erased, so a client cannot separate thinking from answer, and the one marker that existed — the closing tag — has been removed by the splitter. A client that renders content verbatim shows the model's scratchpad as the reply; one that tries to strip it has nothing to key on. Fixing it by returning reasoning as reasoning, keyed on whether the prompt opened the block, is the right shape.

The state handling is the part I checked

a prompt that ends with <think> leaves the model inside the block
one that opens AND closes it (thinking off) does not
the qwen3 family's reasoning tags are the ones this test feeds

Three states, and the middle one is the one that goes wrong in practice: a template that opens and closes <think> (thinking disabled) means the model is not inside a block, so a splitter that keys only on "did the prompt contain <think>" would file the whole answer as reasoning — the mirror of the bug being fixed. Distinguishing "ends inside the block" from "opened and closed it" is the check that makes this a parser rather than a find.

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 question

What 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 <think> as reasoning, that case loses the trailing answer — which is rarer than the bug being fixed but produces exactly the same symptom (an answer filed as reasoning). If it is already handled by the chain state, ignore this.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at d7ad572 — carrying the "the prompt already opened the reasoning block" bit through the parser, the content mask and the stream is the right shape, because all three have to agree or the answer is classified twice by two different rules.

One ask, and it is the whole risk: prompt_opens_reasoning(prompt, family) must be a statement about the end of the rendered prompt, not about its contents. The distinction:

  • correct: the rendered generation prompt's final tokens are the reasoning opener (what a chat template emits before the model's turn), so the model's first output token is already inside the block;
  • wrong: the string appears somewhere in the prompt — a user who asks "what does thinking do?", a quoted example, a tool result containing a transcript, a system prompt that documents the format. Each of those would make starts_in_reasoning true, and the visible consequence is that the whole answer is classified as reasoning — returned as reasoning, masked as reasoning, and in most clients hidden. That is a silent content loss with no error, which is the worst class of bug in an answer path.

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) prompt_is_ids and /v1/completions → never, which the comment already asserts ("an ids prompt and /v1/completions are raw") so the test should pin it. If the function already anchors to the suffix, say so in the body — that one sentence converts the ask into a documented property, and it is the difference between a heuristic and a rule.

And one consistency check worth making explicit: parse_answer, content_mask and AnswerStream now take the same boolean — assert they agree on a single recorded response, since a mask that classifies differently from the parser produces an answer where the reasoning is hidden but the stream already sent it (or vice versa).

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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;
  2. 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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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.
  3. An arm at the seam: a known-answer prompt whose output must show reasoning in reasoning_content and nothing in content, plus the inverse (no think in 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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. prompt_opens_reasoning matches the family's own think_open tag 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".
  2. 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 as content with reasoning_content empty).
  3. 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at d7ad572 — found on a row, searched before filing, and the fault is a protocol one that a "the model answered" test cannot see.

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.
## The fault, reproduced on the NPU
FastFlowLM's `Qwen3.[5 …]`

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 one

If the prompt opened a reasoning block and the response is returned as reasoning, then the split between reasoning_content and content is being decided by the reply rather than by the prompt — and the two disagree exactly when a template leaves a block open. That is the same asymmetry #2064 fixed from the other side (the stop set must be decided after the template is known): both bugs come from deriving a chat-protocol fact from something other than the render that produced it.

Two things worth asserting:

  1. round-trip: render a prompt that opens a thinking block, get a completion, and check that reasoning_content + content re-render to a prompt that would produce the same thing — a protocol property, not a string match;
  2. the negative: a prompt that does not open a block must not have its answer classified as reasoning (the F10: the four server faults FLM 1.0.12 fixed — three measured, one fixed (gemma4 tool calls), and a 16-byte tag-window bug #1810 lesson — a predicate that only recognises one direction passes the case it was not written for).

Nothing else — the row's own reference (Qwen3.5-2B-NPU2) is the right attachment: a defect found while measuring a row belongs to that row.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY d7ad572

The distinction being encoded is the right one and it is stated as a property of the prompt, not of the model:

A template whose generation prompt already opens the reasoning block leaves the answer inside it (npu::prompt_opens_reasoning). Only a rendered chat prompt
can: an ids prompt and /v1/completions are raw.

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
literal and any reasoning in it is the caller's own. Getting that backwards would strip reasoning markers out of user text — a data-loss bug rather than a
rendering one.

The starts_in_reasoning naming is good too: it says what the state is (the prompt already opened the block) rather than what the code does about it, which is
the part that could change without the state changing.

Two arms I would want, both cheap and both about the boundary:

  1. an ids prompt that literally contains reasoning — must NOT set starts_in_reasoning, because the ids prompt is raw (the comment's own rule);
  2. a chat prompt whose template opens the block but whose answer closes it immediately ( reasoning\n response\nanswer) — the degenerate case: the returned
    text must be the answer with no leading empty reasoning block, which is the visible artefact of getting the scan-off-by-one.

ci_targets rc 0, and docs/npu-chat-template.md is the right place for the "rendered vs raw" sentence, since that is the page someone reads when a template
misbehaves.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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\n in content is 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".

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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.

prompt_opens_reasoning() strips trailing whitespace and compares the last len(tag) bytes with the family's opening tag; it never searches. The arms at this head:

  • ends with the opener: true (a prompt that ends with <think> leaves the model inside the block);
  • the opener only in the middle, in a user turn that quotes it, with the assistant header after it: false (the tag earlier in the prompt is not the prompt's end);
  • opens and closes it (thinking off), never mentions it, empty, whitespace only, another family's tag: all false.

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 /v1/completions) is the caller's guard, chat && !prompt_is_ids && …, in forward_serve.cpp; there is no device-free harness for that handler, so it is by reading, not by test.

On the three consumers agreeing: parse_answer is AnswerStream fed once and content_mask is AnswerStream fed per token, so there is one classifier; the arms check the streamed split in awkward pieces against the whole-answer split, and the mask on the same kind of reply turns on exactly at the closing tag.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) and AnswerStream (:709), with chat && !prompt_is_ids so 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant