Skip to content

fix(stream): scrub MiniMax M3 Chinese reasoning markers - #43839

Closed
kiranmagic7 wants to merge 2 commits into
NousResearch:mainfrom
kiranmagic7:kiran/minimax-m3-chinese-reasoning-scrub
Closed

fix(stream): scrub MiniMax M3 Chinese reasoning markers#43839
kiranmagic7 wants to merge 2 commits into
NousResearch:mainfrom
kiranmagic7:kiran/minimax-m3-chinese-reasoning-scrub

Conversation

@kiranmagic7

Copy link
Copy Markdown

Summary

Validation

  • git diff --check
  • uv run ruff check agent/think_scrubber.py agent/agent_runtime_helpers.py gateway/stream_consumer.py cli.py tests/agent/test_think_scrubber.py tests/gateway/test_stream_consumer.py tests/run_agent/test_run_agent.py
  • uv run --extra dev python -m pytest tests/agent/test_think_scrubber.py tests/gateway/test_stream_consumer.py::TestFilterAndAccumulate tests/run_agent/test_run_agent.py::TestStripThinkBlocks tests/run_agent/test_run_agent.py::TestStreamingApiCall::test_content_assembly -q — 94 passed
  • scripts/run_tests.sh tests/agent/test_think_scrubber.py tests/gateway/test_stream_consumer.py tests/run_agent/test_run_agent.py -- -q — 517 passed

@liuhao1024

Copy link
Copy Markdown
Contributor

Reviewed the diff — the approach is well-structured and consistent across all four touch points:

  • Boundary gating in agent_runtime_helpers.py: regex anchors (^|[\r\n]) ensure markers are only stripped at line boundaries, not mid-prose. re.escape(_marker) prevents regex injection. re.DOTALL with .*? (non-greedy) correctly handles multi-line blocks without spanning unrelated content.

  • Streaming scrubber (think_scrubber.py): Chinese markers are added to _OPEN_TAGS/_CLOSE_TAGS for state-machine detection but excluded from _find_earliest_closed_pair (only XML pairs used there) and _strip_orphan_close_tags. This correctly treats them as boundary-gated rather than arbitrary closed pairs.

  • Partial-holdback fix: if tag.startswith(" ") and suffix.isspace(): continue in _max_partial_suffix prevents trailing whitespace from being held back as a potential 思考 prefix. Correct edge-case handling.

  • Consistent propagation: _CHINESE_TAGS added identically in cli.py (_stream_delta) and gateway/stream_consumer.py (_OPEN_THINK_TAGS/_CLOSE_THINK_TAGS), all importing from the canonical CHINESE_REASONING_MARKERS constant.

  • Prose preservation test: test_chinese_reasoning_word_in_prose_is_preserved confirms 我在思考这个问题。 passes through untouched. The leading-space + boundary-anchoring design ensures natural-language mentions are safe.

Test coverage spans the scrubber, CLI stream delta, gateway stream consumer, and agent runtime helpers — thorough.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery provider/minimax MiniMax (Anthropic transport) labels Jun 11, 2026
@kiranmagic7

Copy link
Copy Markdown
Author

Quick status check after the review comment: latest head is still d49108f03c1160cb4cc81395d47e70cbec6423c6, REST reports it mergeable with no conflicts, and GitHub still reports no checks for the branch.

No code changes from my side since the validation in the PR body still matches this head:

  • ruff on the touched runtime/gateway/CLI/test files
  • focused pytest path: 94 passed
  • scripts/run_tests.sh over the touched suites: 517 passed

Happy to refresh the branch or adjust the patch shape if maintainers want anything narrower.

…inese-reasoning-scrub

# Conflicts:
#	tests/gateway/test_stream_consumer.py
@kiranmagic7

Copy link
Copy Markdown
Author

Refreshed this branch against current main to clear the merge conflict.

The only manual conflict was in tests/gateway/test_stream_consumer.py; the resolution keeps the newer upstream fresh-final tests and leaves the PR-visible diff against current main limited to the existing scrubber/runtime/streaming files.

Verification on f5fe71bdf1846ed29043358f9648fb773f383d6d:

  • ruff check agent/agent_runtime_helpers.py agent/think_scrubber.py cli.py gateway/stream_consumer.py tests/agent/test_think_scrubber.py tests/gateway/test_stream_consumer.py tests/run_agent/test_run_agent.py
  • python -m pytest tests/run_agent/test_run_agent.py::TestStripThinkBlocks -q — 30 passed
  • python scripts/run_tests_parallel.py tests/agent/test_think_scrubber.py tests/gateway/test_stream_consumer.py -- -q — 139 passed
  • git diff --check

GitHub now reports the PR mergeable again. No behavior change beyond resolving main-branch drift.

@kiranmagic7

Copy link
Copy Markdown
Author

Status check on current head f5fe71bdf1846ed29043358f9648fb773f383d6d: REST reports the branch mergeable with no conflicts, and GitHub still shows no required checks for this PR. No code changes from my side unless maintainers want a different shape.

Review focus remains the line-boundary MiniMax M3 reasoning-marker scrub across runtime, CLI, and gateway streaming paths.

@teknium1

teknium1 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Thanks for this, and for the detailed analysis in the linked issue. We're going to close it though — it runs against a standing design line: Hermes does not add parsing/scrubbing layers to strip a given model's bespoke raw reasoning delimiters.

The existing English <think> family scrubbers are the boundary we hold; we don't extend that surface to chase each model's custom reasoning markers (思考/反思/推理/推敲, mm:think, and whatever the next model emits). That set grows without end, and it masks the real issue: reasoning that leaks inline is a reasoning-routing / endpoint problem, not a missing-filter problem.

Concretely for MiniMax-M3: our provider plugin already sends reasoning_split: True to the api.minimax.io/v1 OpenAI-compatible route, so the model returns reasoning in a separate reasoning_content field rather than inline. If reasoning is still reaching delta.content, that points at the reasoning-split routing for that deployment, not at a tag the scrubber should learn.

There's also a correctness hazard specific to this fix: 思考/推理/反思 are ordinary Chinese words, not XML-shaped sentinels. Stripping everything from a bare 思考 (or between two occurrences of it) would corrupt legitimate Chinese-language answers — a fix that damages the feature it's meant to protect.

Not a knock on the work — the leak you saw is real to you. But the right fix lives in reasoning routing, not in a new raw-reasoning parser. Closing.

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

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists provider/minimax MiniMax (Anthropic transport) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Chinese reasoning tags ( 思考 / 反思 / 推理 / 推敲) from MiniMax-M3 leak to user output in streaming + final response

4 participants