fix(frontend): strip unconsumed control markers from parsed content by yifjiang · Pull Request #12510 · ai-dynamo/dynamo

yifjiang · 2026-07-31T21:38:23Z

Context: one of six fixes from bringing up thinkingmachines/inkling on Dynamo + vLLM (4×GB200, TP=4). inkling's reasoning parser is vLLM's InklingParser (vllm/parser/inkling.py, via InklingParserReasoningAdapter) — a text-grammar parser, which is what surfaced this. The bug itself is general: it hits any model whose parser forces skip_special_tokens=False. Sibling PRs: #12541, #12511, #12507, #12543.

Problem

Models whose reasoning parser is a text-grammar parser separate thinking from answer using content-kind control markers in the token stream. Such a parser can only split the stream if those markers survive detokenisation — which is exactly why ParserEngine.adjust_request forces skip_special_tokens = False.

Nothing then removes the markers the parser did not consume, so terminal ones leak into user-visible content:

content = "<|end_message|>408<|end_message|>"

As shipped the two settings are mutually exclusive, and neither is correct:

client sends	result
`skip_special_tokens: true` (default)	markers stripped before the parser can split → reasoning separation silently doesn't happen, `reasoning_content` comes back empty
`skip_special_tokens: false`	parser works, but unconsumed markers leak into visible output

Neither path reports an error — you get HTTP 200 and wrong content either way.

Fix

Keep the markers for the parser, then strip the residue afterwards. Applied at three points: _compose_delta_message, the _build_choice funnel, and the one site that seeds a delta directly from delta_text.

The funnel is the part worth reviewing. _build_choice has eight call sites on current main and some seed delta["content"] directly without going through the composer — so a strip applied only at the composer still let a leading marker through. Enforcing the invariant once at the last point every choice passes through is what actually closes it. (The _fast_plain_text early return also exits through _build_choice, so the invariant genuinely holds for every choice.)

Scope is deliberately narrow

Only when a reasoning or tool parser is active — the only case where skip_special_tokens was forced off. Models that never had markers stripped are unaffected.
Only the <|...|> shape, anchored on both delimiters, so ordinary prose containing a bare <| or |> is untouched (a <| b |> c and <|end-message|> both pass through unchanged — the latter because - is not in the character class).
reasoning is left alone — it is already clean, and rewriting it would risk mangling legitimate text the model quoted.
A parser's own markers are preserved — see below.

Why a shape, and not `self._control_markers`?

Fair question, since StreamingPostProcessor already collects tokenizer.all_special_tokens into self._control_markers for _is_control_only_content. Two reasons:

One of the three strip sites seeds delta_message from the raw delta_text before tool parsing, and its result feeds the _tool_start_markers() scan that decides whether to buffer for tool extraction. A blanket special-token strip there would delete a tool-call marker that is also a special token and silently break tool detection.
In practice the two are near-identical — for Qwen/Qwen3-0.6B, every entry of all_special_tokens matches this pattern — so the shape costs nothing while staying unable to reach a marker a parser owns.

For the same reason, markers reported by _tool_start_markers() / _tool_end_markers() are preserved even when they match the shape (<|tool_call|> and friends belong to the parser's wire format). That case is covered by test_a_parsers_own_markers_survive.

Relationship to `_is_control_only_content`

main already has _is_control_only_content, but it is a suppression predicate — it drops a delta that is entirely markers, and only on the tool-call path (if self.in_progress_tool_calls). It does not strip, so mixed content like <|end_message|>408<|end_message|> still reaches the client intact. The two are complementary.

Test

Now covered — the earlier revision of this PR had no unit tests because prepost.py imports vllm and I had no local install. Resolved by running the suite inside a dynamo+vLLM container.

TestControlMarkerStrip, 15 cases:

the reported leak, <|end_message|>408<|end_message|> → 408
leading-only, mid-string, no-marker passthrough, empty, None
the two near-miss shapes above
the parser-inactive gate (test_no_parser_active_leaves_text_untouched)
a parser's own <|...|>-shaped markers survive, while markers it does not own are still stripped
both funnel behaviours: strip in place, and drop the key when the delta was only markers (rather than emitting an empty string)
reasoning untouched while content is stripped
end to end through process_output, and the _fast_plain_text path left alone

14 of the 15 fail without the strip. (The one that passes is the no-parser gate case, which the patch correctly does not change.)

Verified against current main in a dynamo+vLLM container: no regressions in test_vllm_processor_unit.py or tests/frontend/test_prepost.py versus a pristine-main baseline, and no new ruff or mypy findings.

Also validated end to end on a real deployment before this rewrite: the observed marker-leak count went 11 → 6 (composer only) → 0 (with the funnel).

Notes

Ordering with #12541. #12541 makes skip_special_tokens=False the effective default for requests with reasoning active — which is exactly the condition that produces this leak. That PR does not contain this fix, so merging it alone would put the leak on main. This one should land first, or alongside.

Summary by CodeRabbit

Bug Fixes
- Removed unconsumed control markers from user-visible streamed and completed responses when reasoning or tool parsing is active.
- Preserved markers that are managed by the tool parser.
- Removed responses containing only control markers while preserving normal plain-text output.
Tests
- Added coverage for streaming, completed responses, parser-owned markers, empty content, and unconfigured parser behavior.

github-actions · 2026-07-31T21:38:32Z

👋 Hi yifjiang! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

datadog-official · 2026-07-31T21:47:19Z

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 45.86% (-3.81%)

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 1b7687a | Docs | Datadog PR Page | Give us feedback!}

Models whose reasoning parser is a text-grammar parser separate thinking from answer with content-kind control markers in the token stream. Such a parser can only split the stream if those markers survive detokenisation, which is why ParserEngine.adjust_request forces `skip_special_tokens = False`. Nothing then removes the markers the parser did not consume, so terminal ones leak into user-visible content: content = "<|end_message|>408<|end_message|>" As shipped the two settings are mutually exclusive. With the default `skip_special_tokens=true` the markers are stripped before the parser can split, so reasoning separation silently does not happen and `reasoning_content` comes back empty. With `false` the parser works but leaks markers into the answer. Neither gives a correct response, and neither reports an error. Keep the markers for the parser, then strip the residue afterwards. The strip is applied at `_compose_delta_message`, at the `_build_choice` funnel, and at the one site that seeds a delta directly from `delta_text`. The funnel matters: it has several call sites and some seed `delta["content"]` without going through the composer, so a strip applied only at the composer still let a leading marker through. Scope is deliberately narrow: * only when a reasoning or tool parser is active -- the only case where skip_special_tokens was forced off, so models that never had markers stripped are unaffected * only the `<|...|>` control-token shape, anchored on both delimiters, so ordinary text containing a bare `<|` or `|>` is untouched * `reasoning` is left alone -- it is already clean, and rewriting it would risk mangling legitimate text the model quoted Signed-off-by: Yifan Jiang <19356972+yifjiang@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…s own marker Adds the unit coverage this PR was missing, and fixes a case the coverage found. TestControlMarkerStrip, 15 cases: the reported leak ("<|end_message|>408<|end_message|>" -> "408"), mid-string markers, no-marker passthrough, empty/None, the two near-miss shapes ("a <| b |> c" and "<|end-message|>", the latter because `-` is not in the character class), the parser-inactive gate, both funnel behaviours (strip in place, and drop the key when the delta was only markers), reasoning left untouched, and end-to-end through process_output. 14 of the 15 fail without the strip. The fix the tests prompted: a tool parser may own a marker of the `<|...|>` shape. One of the three strip sites seeds delta_message from the raw delta_text *before* tool parsing, and that result feeds the _tool_start_markers() scan deciding whether to buffer for tool extraction -- so stripping a parser's own marker there would silently break tool detection. Markers reported by _tool_start_markers()/_tool_end_markers() are now preserved. Also documents why this matches on shape rather than reusing self._control_markers (tokenizer.all_special_tokens, already collected for _is_control_only_content): a blanket special-token strip at that pre-parse site has exactly the failure above. The two are near-identical in practice -- for Qwen/Qwen3-0.6B every all_special_tokens entry matches this pattern. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqtTc1QsXejXiEdpmLZ4Wz

dynamo-ops · 2026-08-26T00:59:42Z

/ok to test 3e5ecff

yifjiang · 2026-08-26T01:00:09Z

/ok to test 3e5ecff

coderabbitai · 2026-08-26T01:02:33Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 170979dc-e7c0-4f0c-a062-2fe181844e3c

📥 Commits

Reviewing files that changed from the base of the PR and between 2f90000 and 3e5ecff.

📒 Files selected for processing (2)

components/src/dynamo/frontend/prepost.py
components/src/dynamo/frontend/tests/test_vllm_processor_unit.py

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Walkthrough

Changes

Control marker filtering

Layer / File(s)	Summary
Implement marker filtering `components/src/dynamo/frontend/prepost.py`	The postprocessor strips unconsumed `<\|...\|>` markers during delta composition, choice construction, and streaming initialization. Parser-owned tool markers remain unchanged.
Validate parser-specific behavior `components/src/dynamo/frontend/tests/test_vllm_processor_unit.py`	Tests cover parser-active filtering, empty content, parser-owned markers, streaming output, and unchanged no-parser behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 3e5ec

This change removes leftover control markers from parsed user-visible content while preserving parser-owned markers; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description provides detailed problem context, implementation scope, test coverage, validation results, and merge-order notes. However, it omits the repository-required Related Issues section and …	Add the required Related Issues section. Either list the applicable issue references, such as Closes `#XXXX` or Relates to `#XXXX`, or confirm no related issue with the provided checkbox. Preserve the existing technical details; the other templ…
Docstring Coverage	⚠️ Warning	Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary change: removing unconsumed control markers from parsed frontend content.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The description provides detailed problem context, implementation scope, test coverage, validation results, and merge-order notes. However, it omits the repository-required Related Issues section and does not select either the linked-issue or no-related-issue path.

Resolution

Add the required Related Issues section. Either list the applicable issue references, such as Closes #XXXX or Relates to #XXXX, or confirm no related issue with the provided checkbox. Preserve the existing technical details; the other template sections are sufficiently covered by the current content.

Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

Create stacked PR
Commit on current branch

_{Comment @coderabbitai help to get the list of available commands.}

devin-ai-integration

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqtTc1QsXejXiEdpmLZ4Wz

yifjiang · 2026-08-26T01:06:57Z

/ok to test 46173b0

dynamo-ops · 2026-08-26T01:07:09Z

/ok to test 46173b0

CI caught a gap the local container did not: on the arm64 runner test_process_output_strips_the_directly_seeded_delta reached _extract_tool_calls_streaming, and the stub had no extract_tool_calls_streaming, so it died with AttributeError rather than asserting anything. _PassthroughStreamingToolParser implements the hook and returns the raw delta_text, which is what a real parser is handed. That makes the end-to-end test assert the funnel guarantee -- whatever a producer puts in delta["content"], no control marker reaches the client -- rather than the seed strip, which the parser overwrites anyway. Renamed to match. Every test in the class now builds its postprocessor with that parser, so no path through process_output can trip over a missing hook again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqtTc1QsXejXiEdpmLZ4Wz

yifjiang · 2026-08-26T01:42:31Z

/ok to test 3deb8fb

…base Chasing missing attributes one at a time was the wrong fix. The first CI round died on extract_tool_calls_streaming; adding just that method moved the failure to engine_based_streaming, because a bare stub only carries whatever the paths exercised locally happen to read, and the runner takes a different branch. _PassthroughStreamingToolParser now subclasses vllm.tool_parsers.ToolParser and overrides only extract_tool_calls_streaming (pass the raw delta through) and get_structural_tag. Every attribute the postprocessor touches -- vocab, engine_based_streaming, supports_required_and_named, the rest -- comes from the base, so no path through process_output can find one missing. _MarkerOwningToolParser inherits from it for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqtTc1QsXejXiEdpmLZ4Wz

yifjiang · 2026-08-26T02:10:17Z

/ok to test 39608f4

dynamo-ops · 2026-08-26T02:10:29Z

/ok to test 39608f4

copy-pr-bot · 2026-08-26T16:48:02Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Addresses review: P1 correctness, P2 double work, P3 test surface. P1. The shape regex `<\|[A-Za-z0-9_]+\|>` treated textual shape as proof of protocol residue, so a literal the model generated was deleted from content. Confirmed against Qwen/Qwen3-0.6B: `<|not_a_real_special_token|>` is not in all_special_tokens, and the old code turned "The literal is <|not_a_real_special_token|>." into "The literal is .". Removal is now driven by membership in tokenizer.all_special_tokens minus the markers the active parser owns, matched as escaped literals. P2. The strip ran at three sites; content reaching a client passed two of them. It now runs once, in _build_choice, which every emitted choice passes through. _compose_delta_message goes back to being the staticmethod it was, and the delta seed is untouched, so both drop out of the diff entirely. The marker set is computed once per postprocessor instead of per delta, and the compiled pattern is memoised per (tokenizer, parser) because re.compile over ~15 escaped literals is ~12 us and a postprocessor is built per request. P3. Test block cut from 183 lines / 15 cases to 101 / 4, keeping only the distinct contracts: the reported end-to-end leak, parser-owned marker preservation, inactive-parser pass-through, and the new non-special-literal case. Dropped the empty/None/helper-level and shape-anchoring cases, which were asserting on the regex that no longer exists. Tests use `<|im_end|>` rather than `<|end_message|>`: the reported marker is special on the affected model's tokenizer but not on Qwen3-0.6B, so under the corrected rule the old assertions were checking a string the code must now leave alone. Signed-off-by: Yifan Jiang <19356972+yifjiang@users.noreply.github.com>

dynamo-ops · 2026-08-26T17:57:23Z

/ok to test 0596394

dynamo-ops · 2026-08-26T18:15:55Z

/ok to test a71c53f

Addresses the [P1] review thread: the strip missed configured-but-inactive reasoning parsers. What forces skip_special_tokens=false is the deployment's configured parser NAME. preprocessor.rs::parser_requires_special_tokens (PRE.1) matches on static names only, and vLLM's ParserEngine.adjust_request likewise -- neither sees this request's enable_thinking or response_reasoning_ended. But those two are exactly what leave self.reasoning_parser None in this class, so keying the gate on the instance made it a no-op precisely on the requests that still receive raw markers. Reproduced on this head with a real Qwen3 reasoning adapter and tokenizer: both inactivity paths returned '<|im_end|>408<|im_end|>' in content, the same shape as the MiniMax M3 ']~b]408]~b]' payload in the review. Gate now reads reasoning_parser_class. The only behaviour cell that changes is class-set / instance-None / no-tool-parser, which is the reported bug. Chose the class over the reviewer's alternative of the resolved decode policy: in the Python-only path nothing sets sampling_params.skip_special_tokens False for a reasoning-only, no-tools request, so that gate would leave the bug unfixed there. Regression test covers both inactivity paths, parametrized. Both fail without the one-line change, with exactly the reported symptom. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqtTc1QsXejXiEdpmLZ4Wz

dynamo-ops · 2026-08-26T22:04:16Z

/ok to test a95cc72

yifjiang · 2026-08-26T22:05:05Z

/ok to test a95cc72

dynamo-ops · 2026-08-26T23:16:32Z

/ok to test c57ad0a

… marker Refreshes the vendored patch from vllm-project/vllm#54120, which now begins the tool/text tags at `<|content_invoke_tool_json|>` / `<|content_text|>` instead of at `<|message_model|>`. Inkling's chat template prefills the role marker into the generation prompt (it ends `...<|end_message|><|message_model|>`), so generation resumes inside the model message and never re-emits it. The tags are `tags_with_separator` with `at_least_one=true`, so they anchor rather than trigger -- the old grammar therefore required a SECOND role marker. Compiling the tag with xgrammar shows the inversion, for forced/required/auto alike: suffix-only with duplicate marker before rejected accepted after accepted rejected It appeared to work only because the parser absorbed the duplicate, while manufacturing exactly the unconsumed-control-marker residue ai-dynamo#12510 strips. Validated on Inkling-NVFP4 / GB300, vLLM 0.28.0, MTP 8, 2-replica AGG, applied over the shipped image via ConfigMap: forced tool choice honoured 5/5 against a prompt matching the other tool; required = 1 call; parallel_tool_calls true/false = 2/1; no control-marker residue in content -- all in both streaming and non-streaming. Full p0 blocking count unchanged at 2 (the unrelated reasoning_budget_zero vLLM limitation). Note the probe suite could NOT have caught this: control_marker_residue passed before the fix too, because the parser was absorbing the duplicate. The grammar-level acceptance test is what exposed it. Reported by dynamo-review-agent on ai-dynamo#13954. Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>

pull-request-size Bot added the size/M label Jul 31, 2026

yifjiang temporarily deployed to external_collaborator July 31, 2026 21:38 — with GitHub Actions Inactive

github-actions Bot added external-contribution Pull request is from an external contributor fix frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` labels Jul 31, 2026

yifjiang and others added 2 commits August 25, 2026 17:54

yifjiang force-pushed the yifjiang/strip-control-markers branch from 1b7687a to 3e5ecff Compare August 26, 2026 00:59

pull-request-size Bot removed the size/M label Aug 26, 2026

yifjiang temporarily deployed to external_collaborator August 26, 2026 00:59 — with GitHub Actions Inactive

pull-request-size Bot added the size/L label Aug 26, 2026

yifjiang marked this pull request as ready for review August 26, 2026 01:00

yifjiang requested a review from a team as a code owner August 26, 2026 01:00

devin-ai-integration Bot reviewed Aug 26, 2026

View reviewed changes

style: apply black 23.1.0 to the new test class

46173b0

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqtTc1QsXejXiEdpmLZ4Wz

yifjiang temporarily deployed to external_collaborator August 26, 2026 01:06 — with GitHub Actions Inactive

yifjiang temporarily deployed to external_collaborator August 26, 2026 02:10 — with GitHub Actions Inactive

rmccorm4 reviewed Aug 26, 2026

View reviewed changes

Comment thread components/src/dynamo/frontend/prepost.py Outdated

rmccorm4 reviewed Aug 26, 2026

View reviewed changes

Comment thread components/src/dynamo/frontend/prepost.py Outdated

rmccorm4 reviewed Aug 26, 2026

View reviewed changes

Comment thread components/src/dynamo/frontend/tests/test_vllm_processor_unit.py

yifjiang temporarily deployed to external_collaborator August 26, 2026 16:55 — with GitHub Actions Inactive

yifjiang force-pushed the yifjiang/strip-control-markers branch from 44cae6f to 0596394 Compare August 26, 2026 17:57

yifjiang temporarily deployed to external_collaborator August 26, 2026 17:57 — with GitHub Actions Inactive

Merge branch 'main' into yifjiang/strip-control-markers

a71c53f

rmccorm4 temporarily deployed to external_collaborator August 26, 2026 18:15 — with GitHub Actions Inactive

rmccorm4 reviewed Aug 26, 2026

View reviewed changes

Comment thread components/src/dynamo/frontend/prepost.py Outdated

yifjiang temporarily deployed to external_collaborator August 26, 2026 22:04 — with GitHub Actions Inactive

Merge branch 'main' into yifjiang/strip-control-markers

c57ad0a

sara4dev temporarily deployed to external_collaborator August 26, 2026 23:16 — with GitHub Actions Inactive

rmccorm4 approved these changes Aug 28, 2026

View reviewed changes

rmccorm4 enabled auto-merge (squash) August 28, 2026 03:31

rmccorm4 merged commit fc956aa into ai-dynamo:main Aug 28, 2026
106 checks passed

sungsooha mentioned this pull request Aug 28, 2026

build(container): add Inkling vLLM 0.28.0 runtime support #13954

Merged

Conversation

yifjiang commented Jul 31, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Scope is deliberately narrow

Why a shape, and not self._control_markers?

Relationship to _is_control_only_content

Test

Notes

Summary by CodeRabbit

Uh oh!

github-actions Bot commented Jul 31, 2026

Uh oh!

datadog-official Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dynamo-ops commented Aug 26, 2026

Uh oh!

yifjiang commented Aug 26, 2026

Uh oh!

coderabbitai Bot commented Aug 26, 2026

Walkthrough

Changes

❌ Failed checks (2 warnings)

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

✅ Devin Review: No Issues Found

Uh oh!

yifjiang commented Aug 26, 2026

Uh oh!

dynamo-ops commented Aug 26, 2026

Uh oh!

yifjiang commented Aug 26, 2026

Uh oh!

yifjiang commented Aug 26, 2026

Uh oh!

dynamo-ops commented Aug 26, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

copy-pr-bot Bot commented Aug 26, 2026

Uh oh!

dynamo-ops commented Aug 26, 2026

Uh oh!

dynamo-ops commented Aug 26, 2026

Uh oh!

Uh oh!

dynamo-ops commented Aug 26, 2026

Uh oh!

yifjiang commented Aug 26, 2026

Uh oh!

dynamo-ops commented Aug 26, 2026

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yifjiang commented Jul 31, 2026 •

edited by coderabbitai Bot

Loading

Why a shape, and not `self._control_markers`?

Relationship to `_is_control_only_content`

datadog-official Bot commented Jul 31, 2026 •

edited

Loading