Skip to content

Take a MiniMax M3 thought only where it starts the reply, as llama.cpp does (#1887) - #1894

Closed
leehack wants to merge 4 commits into
fix/1531-qwen3-coder-required-grammarfrom
fix/1887-minimax-m3-thinking-off
Closed

leehack wants to merge 4 commits into
fix/1531-qwen3-coder-required-grammarfrom
fix/1887-minimax-m3-thinking-off

Conversation

@leehack

@leehack leehack commented Nov 10, 2026 •

Copy link
Copy Markdown
Owner

Summary

Fixes #1887. Refs #1874 (0.19.4 plan, lane 7).

Stacked on #1890 (base fix/1531-qwen3-coder-required-grammar at b75aa2e91, on #1885 and #1882). Review commits 933e48bee (fix) and 302757be6 (audit follow-ups); 22fe46da5 and bf8735ab8 merge the moved base (95d4986c9 -> 55c69df04 -> b75aa2e91). Both merges were clean except that the union-merged changelogs kept #1531's old required-call bullet beside its reworded one; the stale copy was removed each time, so each changelog gains only the #1887 bullet. Retarget once #1890 merges.

After a prompt that opens no thought (MiniMax M3 with thinking off, or thinking_mode: adaptive), llama.cpp cd959bae9 (common/parsers/minimax-m3.cpp) takes a thought only where <mm:think> starts the reply, with no whitespace before it, drops a </mm:think> there, and keeps every other thinking tag as content. llamadart took a <mm:think> thought from anywhere in the reply, and a </mm:think> after text ended a thought.

  • minimaxM3 gets the takesThoughtOnlyAtReplyStart trait from Take a thought only at the start of a reply after a prompt that opens no thought, as llama.cpp does (#1865) #1882.
  • New trait thoughtStartsReplyExactly (MiniMax M3 only): the reply-start thought skips no whitespace, and an end tag that starts the reply is dropped. withThoughtOnlyAtReplyStart(parse, exact:) carries it to extractThinking; the stream waits at the reply start for either tag.
  • The stream's tool-envelope check (_isToolCallEnvelopeBuffer) applies the same rule, so a <mm:think> after leading whitespace or after a leading </mm:think>, followed by a call before any end tag, streams as content instead of being taken as a thought and suppressed (audit note 1).

Why the extra trait. The issue's proposed fix (the #1882 trait alone) fixes the mid-reply thought, but in a wider corpus it breaks 300 rows that matched: a reply starting with a bare </mm:think> would keep the tag as content, where llama.cpp drops it. The #1882 audit corpus had no such reply.

The 16 leading-thought rows from the issue. With thinking off, llama.cpp consumes a leading <mm:think>…</mm:think> and drops it. llamadart's parseRendered still returns it as reasoningContent (documented: with thinking off the engine drops reasoning chunks, while parseRendered still returns it), so the engine's output matches llama.cpp. The leading-whitespace variants ( \n<mm:think>…) were real differences and are fixed.

Production-readiness scope

  • User-facing scope: MiniMax M3 replies to a prompt that opens no thought parse and stream as in llama-server: a thought after text or after leading whitespace, and a </mm:think> after text, stay content.
  • Supported platforms/paths: All backends that use the Dart chat parser (native llama.cpp, WebGPU bridge). VM and Chrome suites pass.
  • Unsupported or intentionally unavailable paths: Unchanged differences, listed in closed_thought_upstream.md: with thinking off, parseRendered returns a leading thought as reasoning that llama.cpp drops (the engine drops it too); a partial parse drops a trailing partial thinking tag after content, which llama.cpp keeps (final parse and stream unaffected). With thinking on and no thinking_mode kwarg, llamadart renders enabled (forced-open <mm:think>) where llama-server renders adaptive, by design since DeepSeek V3.1 and Qwen thinking parity gaps against llama.cpp (thinking flag, escapes, repeated <think>) #750 and documented in the guide.
  • Out of scope / follow-ups: None.

Completeness checklist

  • Declared scope is fully implemented, or explicitly reduced above.
  • Unsupported platform/option combinations fail loudly with actionable diagnostics or are clearly documented as unavailable. (N/A: parse behavior only.)
  • Public API docs, README/website docs, examples, support matrices, and changelog entries are updated where relevant. (Both changelogs, chat-template-and-parsing.md, template-engine-internals.md. skills/llamadart-chat-streaming/SKILL.md already states the rule MiniMax M3 now follows; unchanged.)
  • Regression coverage covers the original issue plus key negative/version-skew paths where relevant.
  • Public capability changes update the validation coverage registry with a pack or a reasoned, issue-linked exemption; inventory checks pass (or N/A with scope). N/A: no capability change.
  • Security/privacy review completed: no secrets, bearer tokens, signed URLs, or raw secret-bearing paths leak through logs, cache keys, metadata, errors, or snapshots.
  • Follow-up work that is useful but not required for this PR is tracked in GitHub Issues, or explicitly marked "None" above.

PR type guidance

  • Bugfix PR: Root cause: minimaxM3 lacked the reply-start rule, so extractThinking took a <mm:think> thought anywhere and ended a thought at a </mm:think> after text; the generic rule also skips leading whitespace and keeps a leading bare end tag, which MiniMax M3's upstream parser does not; and the stream's tool-envelope check stripped any leading thought. Regression tests:
    • Two MiniMax M3 entries in closed_thought_upstream.json (thinking off with default kwargs; thinking on with thinking_mode: adaptive), 21 + 26 cases plus 5 dropped_thought_cases, recorded from the cd959bae9 probe (common_chat_session, whole = byte-by-byte = finish-only for every case; partial cases from common_chat_peg_parse). Each case checks parseRendered with and without tools and the stream at whole, per-char, 3-char and every 2-split chunking; dropped-thought cases check the engine stream with thinking off. On base 95d4986c9 47 of the first set failed. The audit follow-up cases unfinished-thought-after-space-then-call and unfinished-thought-after-end-tag-then-call fail (both entries) without the envelope fix; end-tag-only pins a reply of just </mm:think>.
    • thinking_utils_test.dart: withThoughtOnlyAtReplyStart(exact: true).
    • chat_format_registry_test.dart: the reviewed-dispatch text for the final envelope check now includes exactReplyStart.

Test Plan

All at 302757be6 unless noted.

  • dart run tool/prepare_workspace.dart (no tracked changes)
  • dart format --output=none --set-exit-if-changed .; dart analyze; dart run tool/testing/check_platform_boundaries.dart
  • dart test -p vm -j 1 --exclude-tags local-only (isolated TMPDIR): 12,728 passed, 129 skipped, 6 failed, all windows_registry_qualification_test.dart (needs pwsh, absent on this host)
  • dart test -p chrome --exclude-tags local-only test/unit/core test/integration/core: 3,819 passed, 1 skipped (full Chrome run at 3cdc8948f: 4,667 passed, 1 skipped)
  • coverage-lib: Test Linux VM (with Coverage) and Test Linux & Web (with Coverage) pass in run 42802975044
  • ./tool/testing/run_template_parity_suites.sh: detection 74, diagnostic 103, unit template 1,876, upstream parser 10, GBNF acceptance 81, all passed

Matrix Evidence

Matrix row Scope covered Platform / model / backend Result Evidence / notes
root-vm native-safe unit/integration macOS arm64 VM PASS 12,728 passed, 129 skipped, 6 pwsh-only failures
root-chrome browser-compatible tests Chrome PASS core suites at head; CI
template-parity detection, render and parse parity; GBNF acceptance macOS arm64 PASS see Test Plan
structured-output-adversarial partial/final parse and every-chunking stream vs llama.cpp; thinking on/off; tool choice none/auto/required/parallel; with and without tools llama.cpp cd959bae9 common_chat_session + common_chat_peg_parse oracle PASS corpora below; 0 regressions
coverage-lib lib/ line coverage Linux CI PASS run 42802975044
high-risk-exact-head-independent-qa independent blocking review of exact head — PASS at 22fe46da5; re-audit needed at 302757be6 audit notes 1–3 addressed here
Other rows — — N/A no release, LiteRT-LM or runtime routing change

Upstream comparison (llama.cpp cd959bae9), base b75aa2e91 vs head 302757be6

MiniMax M3 corpus: the upstream template x thinking on/off x kwargs (none, thinking, thinking_mode enabled/disabled, thinking_mode: adaptive) x 5 tool modes x 235 replies (#1858/#1865/#1525 shapes, the #1882 audit shapes, bare </mm:think>, namespaced calls, and unfinished thoughts after whitespace or an end tag before a call). "Engine" is the stream with the request's own enableThinking.

Rows (finished unless noted) base head
parse = llama.cpp, all 9,400 4,405 6,117
partial parse = llama.cpp, all 9,400 4,305 5,967
engine stream = llama.cpp, all 9,400 4,739 6,859
every chunking = parse, all 9,400 7,385 8,695
prompt opens no thought (thinking off, or adaptive): engine = llama.cpp, of 5,800 rows llama.cpp parses 3,680 5,800
same rows: every chunking = parse 4,490 5,800
regressions (row matched on base, not on head) — 0

The remaining mismatches are the forced-open render with thinking on and no thinking_mode (see above), the documented thinking-off parseRendered reasoning, and partial trailing tag prefixes.

All-template sweeps (76 upstream models/templates + vendored test/fixtures/templates; thinking on/off x kwargs x 5 tool modes):

Corpus parse = llama.cpp partial = llama.cpp every chunking = parse regressions
#1865 bare/mid-reply, upstream 60,852 -> 61,242 / 104,160 61,569 -> 61,939 96,922 -> 97,302 0
#1525 thinking, upstream 67,921 -> 68,241 / 103,212 — 99,892 -> 100,192 0
native tool calls, upstream 3,572 -> 3,572 / 7,040 3,218 -> 3,218 6,912 -> 6,912 0
#1865 bare/mid-reply, vendored 18,116 -> 18,116 / 24,000 18,480 -> 18,480 23,592 -> 23,592 0
#1525 thinking, vendored 20,256 -> 20,256 / 24,120 — 23,696 -> 23,696 0

Only MiniMax M3 rows change, apart from 7 Gemma 4 rows whose byte, 2-char or 3-char stream already differs from their parse on base and leaks a different length of call JSON as content from run to run (the stream's time-based partial parse interval); parse results are identical and both runs mismatch, so they are unrelated to this change.

Mutation check

16 mutants over core/unit, core/engine and core/template tests. 12 killed: trait removed, exact trait removed, parse keeps the leading end tag, parse skips leading space, engine passes exact: false, stream exact rule off, stream does not wait on an end-tag prefix, stream keeps the end tag, stream skips leading space, zone rule ignores exact, B5 (rest.length <= endTag.length, killed by end-tag-only), and envelope check without the exact rule (killed by the two new tool-call cases). 4 give identical results on all 18,800 MiniMax M3 corpus rows: the tool-mode decision path without the exact rule (unreachable for MiniMax M3's stream, so that argument was dropped), the exact strip not dropping a leading end tag or not waiting on a partial tag (the envelope check only suppresses content the parse leaves empty in those shapes). The leadingThoughtOnly guard on the exact flag also survives but is not equivalent: _isToolCallEnvelopeBuffer does reach the exact strip on forced-open prompts, and removing the guard makes 88 forced-open corpus rows chunking-dependent; no test pins it yet (independent audit at 302757be, follow-up on #1861).

Review Notes

  • Independent review status: PASS at 22fe46da5 (audit notes 1–3 addressed in 302757be6); re-audit of 302757be6 pending
  • CI status / head SHA: 302757be6; dispatched ci.yml run 42802975044: 26 passed, 6 skipped, 0 failed

High-risk regression review

  • Classification: high-risk (backendRuntime, structuredOutput)
  • Implementation task: 0.19.4 lane 7 (#1874)
  • Independent blocking QA task: PASS at 22fe46da5; pending at 302757be6
  • Exact head / current base: 302757be6 / b75aa2e91 (fix/1531-qwen3-coder-required-grammar)
  • Production-branch deletion, bypass, or miswire proof: mutation check above (12/16 killed, 4 equivalent on the corpus with reasons)
  • Affected-family real-model/artifact evidence: MiniMax M3 weights unavailable (several hundred GB MoE). Evidence is the upstream template and the pinned cd959bae9 parser via the oracle, with durable fixtures in closed_thought_upstream.json.
  • Explicit unavailable-family or other N/A evidence: MiniMax M3 (above)
  • Known PR-caused P1 regressions: 0
  • Unresolved review threads: 0

leehack added a commit that referenced this pull request Nov 10, 2026
@leehack

leehack commented Nov 10, 2026

Copy link
Copy Markdown
Owner Author

Landed on main via batch integration PR #1895 (rebase merge, main ba4e0e1b; tree identical to the audited integration head 5ffd1224).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant