fix: deepseek DSML leak

[bsbds]

No description provided.

[gemini-code-assist]

Code Review

This pull request introduces support for parsing DeepSeek DSML (DeepSeek Markup Language) tool calls, specifically addressing cases where DeepSeek leaks DSML tool grammar into the assistant message content during forced-final turns in the ReAct pattern. It adds a new DSML parser module (deepseek_dsml.py), integrates it into the DeepSeek LLM adapter (deepseek.py), updates the ReAct pattern execution loop to support recovering these tool calls, and includes comprehensive unit and integration tests. There are no review comments, so I have no feedback to provide.

[rogercloud]

Summary

DeepSeek V4 intermittently emits its internal DSML tool-call grammar (sentinel tokens like |DSML|, including full-width Unicode pipe variants, wrapping <tool_calls> / <invoke> / <parameter> tags) as plain assistant message.content instead of structured OpenAI-style tool_calls. When that happens the raw markup leaks to the user and the intended tool calls never execute. This PR adds deepseek_dsml.py, which parses and strips that markup back into structured tool calls, wires it into deepseek.py's chat() / stream_chat(), and adds a duck-typed recovery hook in react.py / runtime.py so DeepSeek can specifically recover DSML tool calls during forced-final-answer turns (where tool_schemas is intentionally emptied).

Design verdict: sound

The central design choice — routing any DSML-recovery-eligible call through non-streaming chat() because DSML can only be parsed from a complete response — is the right call. It sidesteps a whole class of streaming-boundary bugs (partial sentinels, multibyte splits) rather than reinventing an incremental parser. Provider-scoping is clean, and the fail-closed validation posture (raise on leftover markup / unknown tool / duplicate parameter) is reasonable.

Design note: the recovery requirement legitimately trades away token-level streaming, but the current implementation applies that tradeoff more broadly than the design justifies — every tool-bearing DeepSeek turn loses live streaming, not just the narrow forced-final DSML-recovery case. See Finding 1.

Findings

MAJOR

1. src/xagent/core/model/chat/basic/deepseek.py:322 — stream_chat routes any call with tools truthy through non-streaming chat(), then replays the full answer as one or two chunks via stream_chunks_from_chat_result. Confirmed against the base commit: previously stream_chat streamed unconditionally regardless of tools. This is a new regression — all ReAct tool-enabled DeepSeek turns lose token-by-token streaming, not just the forced-final DSML-recovery case the design justifies. Suggest narrowing the condition to bypass streaming only when DSML recovery is actually possible/needed (e.g. gate on DEEPSEEK_DSML_PARSE_TOOLS_KWARG specifically), or document explicitly why plain tools must also trigger buffering.

2. src/xagent/core/agent/runtime.py:115 (new start_immediately parameter interacting with the pre-existing unconditional await stream.finish(content) a few lines below, plus src/xagent/core/model/chat/basic/deepseek_dsml.py:148-161) — confirmed double-answer UI bug. On a forced-final turn, start_immediately=False defers opening the final-answer stream, but if a recovered DSML tool call carries leading prose (e.g. "Let me calculate..."), stream_chunks_from_chat_result yields a TOKEN chunk before the TOOL_CALL chunk. emit_text_delta reacts to the TOKEN chunk and lazily opens+starts the final-answer stream via emit_delta, then the (unchanged) stream.finish(content) call unconditionally closes that stream as a completed answer bubble containing only the leaked prose, without checking whether response actually carries tool_calls. Because this path runs with tool_schemas=[] (no answer_streamer), _finish_streamed_answer_if_final in react.py never reconciles it. The real tool call then executes, and a second, separate final-answer stream opens later for the actual answer. Net effect: the user sees a bogus "finished" answer bubble followed by a second real answer. Suggest not calling stream.finish() unconditionally when the response contains a recovered tool call — check for tool calls in response before finishing the stream as a completed answer, or defer opening/finishing the stream until it's confirmed there's no trailing tool call. No test currently exercises this path.

MINOR

3. src/xagent/core/model/chat/basic/deepseek_dsml.py:67-68 — normalize_deepseek_dsml_response returns early (content untouched) whenever response.get("tool_calls") is truthy. If DeepSeek ever returns official tool_calls AND leftover DSML markup in content in the same response, the markup leaks through unstripped — the exact leak class this PR targets. Narrow/uncommon case but currently untested and not documented as out of scope. Consider still stripping DSML markup from content even when official tool_calls are present.

4. Test coverage gaps in tests/core/model/chat/basic/test_deepseek_dsml.py / test_deepseek.py / tests/core/agent/test_react.py:

   • (a) No test where a DSML block has non-empty surrounding prose asserting clean_content equals that prose — all current cases yield content == "".
   • (b) No test combining official tool_calls with DSML markup in content (covers Finding 3).
   • (c) No test for the prose+DSML forced-final streaming scenario from Finding 2 — the existing test_react_pattern_converts_deepseek_forced_final_dsml_tool_call recovers via a native TOOL_CALL chunk, not via mid-stream TOKEN+TOOL_CALL splitting. Since Finding 2 is a confirmed live bug, (c) in particular should have caught it — recommend adding it as a regression test alongside that fix.

Simplification opportunities

• deepseek_dsml.py L148-171: dedup. StreamChunk(type=ChunkType.TOKEN, content=content, delta=content, raw=response) is emitted identically in both the tool_calls and no-tool_calls branches of stream_chunks_from_chat_result. Hoist if content: yield StreamChunk(...) above the branch, then branch only on TOOL_CALL vs END. Verified behavior-identical across all 4 branch combinations.
• deepseek_dsml.py L109-127: simplify. The clean_parts/last_index splice loop in parse_deepseek_dsml_tool_calls only ever deletes matched spans — identical to _DSML_TOOL_CALLS_BLOCK_RE.sub("", text).strip(). Replace the splice loop with a single .sub() call; keep the existing finditer list separately for extracting tool-call groups. Verified behavior-identical for multi-match, boundary-whitespace, and no-match cases.

net: -6 lines possible

Tests

All three changed test files pass (verified locally, exit 0). Coverage gaps are enumerated in Finding 4 — most notably the missing regression test for the Finding 2 double-answer path.

Scope note (FYI, not a defect)

Leakage of non-tool-call DSML sentinels (DSML tags other than tool_calls / invoke / parameter) is intentionally out of scope and is a consistent boundary across all commits in this PR — this PR neither introduces nor narrows it. Noted only so readers don't mistake it for a gap.

[rogercloud]

Re-review — PR #790 "fix: deepseek DSML leak"

What changed since the last review

The last review was posted against aca8327d. The PR has advanced to 778c46b5 with three new commits that directly target the findings from that round:

• f93551c1 "fix(deepseek): keep regular tool streams native" — narrows the stream_chat non-streaming bypass so only DSML-recovery calls buffer, restoring native per-chunk streaming for ordinary tool-enabled ReAct turns.
• f103a294 "fix(agent): avoid finishing tool-call preambles" — reworks stream_final_answer to defer text deltas and to fail() rather than finish() a started stream when the forced-final response actually carries tool calls, eliminating the double-answer bubble.
• 778c46b5 "fix(deepseek): strip dsml around official tool calls" — makes normalize_deepseek_dsml_response strip DSML markup from content even when official tool_calls are present, instead of early-returning untouched.

The two simplification suggestions from the prior review were also applied: the splice loop is now a single .sub() call inside a new _extract_dsml_tool_call_blocks helper, and the duplicated TOKEN-yield was hoisted above the tool_calls branch in stream_chunks_from_chat_result.

Status of prior findings

1. MAJOR — all tool turns lost streaming → FIXED. The bypass condition now checks only for the DSML sentinel kwarg, rather than triggering on any tools. New test test_stream_chat_with_regular_tools_keeps_native_streaming patches llm.chat to raise if invoked, proving the buffering path is not taken for plain tool calls. test_deepseek.py: 36/36 pass.
2. MAJOR — forced-final double-answer bubble → FIXED. stream_final_answer now defers deltas and, after the call, fail()s a started stream when tool calls are present instead of finishing a bogus answer. New regression test test_react_pattern_does_not_finish_forced_final_preamble_before_tool_call reproduces the exact leaked-preamble scenario and asserts a single correct final-answer triple. test_react.py: 63/63 pass.
3. MINOR — DSML markup leaking alongside official tool_calls → PARTIAL. The original cosmetic leak is genuinely fixed and covered by a test, but the fix introduces a new MAJOR regression — see below.
4. MINOR (coverage) — three test gaps → FIXED. All three added: a prose-preservation test, a tool_calls+DSML-coexistence test, and the double-answer regression test.

New finding

MAJOR — Stripping DSML markup around official tool calls raises on partial/truncated markup, turning a usable tool-call response into a hard turn failure

src/xagent/core/model/chat/basic/deepseek_dsml.py, in normalize_deepseek_dsml_response's tool_calls branch (the call to _strip_dsml_tool_call_blocks).

When a response has valid official tool_calls and DSML markup in content, this branch calls _strip_dsml_tool_call_blocks -> _extract_dsml_tool_call_blocks, which raises DeepSeekDSMLParseError if the strict block regex finds zero complete matches. Entry into this branch is gated by a separate, looser marker check that matches on a bare/partial sentinel tag prefix and does not require a closing tag or a matching end tag. So a response carrying valid, actionable tool_calls plus a truncated DSML fragment in content (a realistic partial-leak artifact — the entire point of this PR is that DeepSeek's DSML emissions can be partial/malformed) now makes the whole call raise. Nothing catches this exception up through deepseek.py's chat(). Before this commit (early-return-on-tool_calls), the same input succeeded with the tool calls intact and only cosmetic leaked text.

Reproduced directly against the current worktree:

response = {"content": "Some leading text <DSML-token>tool_calls", "tool_calls": [<valid tool call>]}
normalize_deepseek_dsml_response(response, tools=None, model_name="deepseek-v4-flash")
# -> raises DeepSeekDSMLParseError: "... returned incomplete DeepSeek DSML tool markup."

The only new test for this branch uses a complete, well-formed DSML block, so the partial-markup case is uncovered.

Suggested fix: in this branch the tool calls are already valid and actionable, so correctness should defer to them — do not raise when no complete block is found here. Either return content unchanged (matching the pre-fix safety), or best-effort-strip only complete matches and leave partial fragments in place. Please also add a regression test combining valid tool_calls with a partial/truncated DSML fragment in content.

Non-blocking notes

• ReActPattern now duck-types a provider-specific method (deepseek_dsml_parse_kwargs) via getattr/callable — a small DeepSeek-specific abstraction leak into the generic ReAct layer, degrading gracefully for other providers. Acceptable.
• Forced-final DeepSeek turns buffer the whole response as one non-streaming call and emit the final answer as a single delta (no token-by-token streaming). This is an inherent cost of DSML recovery, not newly introduced or worsened by these three commits — FYI only.
• Benign corner: a forced-final turn producing literally empty content and no tool calls fires zero outbound events for that turn. Not a regression, not worth changing.
• A simplification lens flagged the new PatternRuntime._response_tool_calls helper as looking like a duplicate of similar helpers elsewhere in the codebase. Verified: it is not safely interchangeable (different behavior for non-dict responses) and reusing the other helper would introduce a circular import. No action needed.

Test results

• test_deepseek.py: 36/36 pass.
• test_react.py: 63/63 pass.
• test_deepseek_dsml.py: 15/15 pass.
• Combined total: 87/87 pass per-file. Running all three files in a single pytest invocation hits an unrelated circular-import error from test collection order — pre-existing, not caused by this PR; per-file runs are clean.

Verdict

This PR is very close — three of the four prior findings are cleanly resolved with regression tests, and the overall design remains sound. However, the latest commit's DSML-stripping change introduces a new MAJOR correctness regression that can turn a successful tool-call turn into a hard failure on realistic partial-leak input. Recommending this stay at COMMENT (not approve) pending that one fix plus a partial-markup regression test.

[rogercloud]

Re-review of PR #790 — "fix: deepseek DSML leak"

What changed since the last review

Exactly one commit landed since the last review (778c46b): 95dcf576 "fix(deepseek): preserve tool calls with partial dsml", touching only src/xagent/core/model/chat/basic/deepseek_dsml.py (17 source lines) and its test file (one new test). It targets the prior MAJOR finding.

Status of prior finding

MAJOR (prior) — DSML strip raising DeepSeekDSMLParseError around official tool_calls when only partial/truncated markup is present → FIXED.
The tool_calls branch of normalize_deepseek_dsml_response (deepseek_dsml.py:71-77) now calls a new helper _strip_complete_dsml_tool_call_blocks (deepseek_dsml.py:185-188) instead of the old _strip_dsml_tool_call_blocks. The new helper returns (text, False) unchanged when no complete DSML block is found, rather than raising. Verified by live repro (no exception, tool_calls preserved, content untouched) and the new test test_normalize_deepseek_dsml_response_keeps_official_tool_calls_with_partial_markup (test_deepseek_dsml.py:156-182), which passes and would fail if the bug were reintroduced. The turn-crashing path is closed.

Design question (not a line-level bug — please clarify)

Why doesn't DSML recovery cover the primary streaming ReAct path with tools?

As far as I can trace, DSML recovery only runs on two paths: (a) the non-streaming chat() call, and (b) the forced-final buffered-stream path (where tool_schemas is intentionally emptied and a hidden sentinel kwarg triggers recovery). The main, ordinary streaming ReAct turn — a turn that has real tool_schemas and streams natively via run_streaming_llm_call — never sets that sentinel and therefore never invokes normalize_deepseek_dsml_response / parse_deepseek_dsml_tool_calls on its output.

If that's correct, the implicit assumption is that DeepSeek only emits leaked DSML markup on forced-final turns (tools=None/emptied), never on ordinary tool-enabled streaming turns. Is that assumption verified against real DeepSeek behavior, or just the scenario this PR happened to reproduce? If DeepSeek can also leak DSML markup during a normal tool-enabled streaming turn, this PR would leave that path completely uncovered — the raw sentinel markup would stream straight to the user and the intended tool call would be lost, with no recovery attempted at all.

Could you confirm: (1) whether this scoping is deliberate and based on confirmed DeepSeek behavior, and if so a one-line comment/docstring noting it would help future readers, or (2) whether ordinary tool-enabled streaming turns should also be covered, in which case this should be tracked as a follow-up rather than left implicit.

New findings

MEDIUM #1 — Mixed complete+partial DSML in the same content string leaks the partial fragment verbatim to the user.
src/xagent/core/model/chat/basic/deepseek_dsml.py:185-188, _strip_complete_dsml_tool_call_blocks.
Unlike _extract_dsml_tool_call_blocks (deepseek_dsml.py:172-176), which re-checks for residual markup after stripping and raises if any remains, _strip_complete_dsml_tool_call_blocks only strips complete blocks via .sub("", text) with no post-strip check. Repro: content containing one complete <...tool_calls>...</...tool_calls> block plus a separate trailing partial <...tool_calls> fragment (no closing tag) — the complete block is stripped, but the partial fragment's raw sentinel markup (a DSML sentinel token) survives unstripped in the returned content. This is not inert: the output content flows through stream_chunks_from_chat_result (deepseek_dsml.py:140-147) as a ChunkType.TOKEN chunk emitted before the TOOL_CALL chunk, which runtime.py's _chunk_text_delta treats as live, user-visible streamed text (the tool-call preamble). So raw DSML sentinel markup can leak into what the user sees during a tool-calling turn — the exact class of bug this PR exists to fix. Not covered by any existing test (the new test only covers a solely-partial case, not mixed complete+partial).
Suggested fix: after stripping complete blocks, re-check with contains_deepseek_dsml_tool_markup and either strip/mask the residual partial marker or leave it explicitly untouched by design — but either way add a test covering the mixed case so the behavior is intentional rather than accidental.

MEDIUM #2 — Non-tool_calls path still crashes the whole turn on partial/truncated DSML markup (pre-existing; not addressed by 95dcf57).
src/xagent/core/model/chat/basic/deepseek_dsml.py:167-176, _extract_dsml_tool_call_blocks, reached via parse_deepseek_dsml_tool_calls when there are NO official tool_calls. It raises DeepSeekDSMLParseError on zero complete matches, and the exception is never caught up the stack — confirmed uncaught through deepseek.py's chat(), runtime.py's stream_final_answer (only stream.fail() then re-raise), and react.py (only fail() then re-raise). This path is byte-identical since before 778c46b; 95dcf57 only touched the tool_calls-present branch, so the gap is unchanged by the latest commit and was not the subject of the last review round either. A real turn hitting truncated/malformed DSML with no valid tool_calls (e.g. a cut-off stream) will hard-crash instead of degrading to plain text, cutting against the PR's "fix DSML leak" robustness goal. Flagged as a known gap rather than a blocker for this incremental commit (it is outside the diff under re-review), but worth a follow-up before the DSML-recovery feature is considered fully hardened.

Simplification opportunities

• L180-182: dead-code _strip_dsml_tool_call_blocks has zero callers repo-wide after 95dcf57 re-pointed the call site to _strip_complete_dsml_tool_call_blocks; deleting it loses no behavior or coverage (the _extract_dsml_tool_call_blocks it wrapped is still used independently by parse_deepseek_dsml_tool_calls). Delete the function.
• net: -3 lines possible

Test results

• test_deepseek_dsml.py: 15/15 pass (including the new ..._keeps_official_tool_calls_with_partial_markup).
• Broader previously-relevant suite (all 3 files): 112/112 pass.

Verdict: COMMENT (not approve)

The targeted fix in 95dcf57 is correct and closes the crash-causing MAJOR bug from the last round — the turn no longer aborts when official tool_calls arrive alongside wholly-partial DSML markup. However, several related gaps remain open: the design-scoping question above (does DSML recovery need to cover ordinary streaming tool turns?), MEDIUM #1 (introduced by this very fix — mixed complete+partial content still leaks a raw sentinel fragment to the user), and MEDIUM #2 (pre-existing — the no-tool_calls path still hard-crashes on truncated markup). None of these block merge on their own, but all three should be answered/tracked for the DSML-recovery feature to be considered robust. Recommend merging the incremental fix while opening follow-ups for the open items.

[rogercloud]

Summary

This PR fixes a DeepSeek DSML (tool-markup) leak: when the provider returns
authoritative tool_calls, residual DSML markup could survive in the visible
content and stream to the user. The fix is scoped to the provider module
deepseek_dsml.py; forced-final turns opt into buffered local DSML recovery
via a private sentinel, while ordinary tool-enabled streams stay
provider-native. This round adds two commits: 8cd01cf reworks the strip
helper into _strip_dsml_markup_for_official_tool_calls, which now also
truncates any residual/partial DSML marker left after complete blocks are
removed, and 1654735 expands an explanatory comment in deepseek.py
clarifying the stream-recovery scope.

Round 0 design verdict

Acceptable-with-reservations. The fix is well-factored — an isolated,
provider-scoped module with clean sentinel plumbing. One design-level gap
persists: DSML-recovery coverage is asymmetric between the
tool_calls-present branch (now degrades gracefully on partial markup) and
the tool_calls-absent branch (still hard-raises DeepSeekDSMLParseError on
partial/incomplete markup, uncaught up the stack) — see prior finding #4.

Status of prior findings

1. MAJOR — DeepSeekDSMLParseError raised around official tool_calls on
   partial markup — FIXED (still fixed). Confirmed the two new commits do not
   regress the round-3 fix.

2. Design question — "why doesn't DSML recovery cover the primary streaming
   ReAct path with tools?" — WAIVED-WITH-EXPLANATION (not fully resolved).
   The code path is byte-identical; only the comment text changed
   (deepseek.py:318-321). The new comment adds a genuine architectural
   rationale: forced-final turns intentionally send tools=None to the
   provider (so local DSML recovery is the only fallback), whereas ordinary
   tool-enabled streams send real tool schemas and rely on provider-native
   tool-calling. That is a reasonable why, but it does not answer the
   original question: is it actually verified that DeepSeek never leaks DSML
   markup on an ordinary tool-enabled streaming turn? No test, comment, or code
   change establishes that. If the provider ever does leak markup on that path,
   it streams straight to the user with zero recovery — the same gap as before.
   Recommend keeping this open pending an explicit answer, not treating it as
   closed by the comment alone.

3. MEDIUM — mixed complete+partial DSML leak — FIXED.
   _strip_dsml_markup_for_official_tool_calls
   (src/xagent/core/model/chat/basic/deepseek_dsml.py:180-188) now re-checks
   for a residual marker after stripping complete blocks and truncates at it.
   The new regression test
   test_normalize_deepseek_dsml_response_strips_mixed_complete_and_partial_markup
   reproduces the exact mixed-block case and passes. No gap remains within the
   finding's original scope.

4. MEDIUM — non-tool_calls path still crashes on partial/truncated DSML
   markup — NOT FIXED (still open; pre-existing, out of scope of this diff).
   _extract_dsml_tool_call_blocks / parse_deepseek_dsml_tool_calls and the
   whole call stack (deepseek.py chat(), runtime.py stream_final_answer,
   react.py) are byte-for-byte unchanged since the last review;
   DeepSeekDSMLParseError is still uncaught anywhere and still hard-fails the
   turn. Notably, on this path even a complete DSML block followed by a
   residual partial fragment raises rather than degrading — so the asymmetry
   with the now-fixed tool_calls-present branch (finding #3) is sharper than
   before: that branch degrades gracefully in the exact same mixed scenario,
   this one still hard-crashes. Nothing in this diff touches it; flagging for a
   follow-up.

5. Simplification — dead code _strip_dsml_tool_call_blocks — FIXED.
   Repo-wide grep confirms zero remaining references to the old function name
   or the round-3 intermediate name _strip_complete_dsml_tool_call_blocks.
   Cleanly consolidated into _strip_dsml_markup_for_official_tool_calls.

New findings (this round)

• LOW — dead/unreachable branch —
  src/xagent/core/model/chat/basic/deepseek_dsml.py:73-74. Inside
  normalize_deepseek_dsml_response, the if not stripped: return response, False guard after calling _strip_dsml_markup_for_official_tool_calls can
  never be true. Reaching this branch already requires
  contains_deepseek_dsml_tool_markup(content) to have returned True (line
  68), and the helper's own residual-marker fallback (lines 185-188) guarantees
  stripped=True whenever that same marker-presence condition holds on the
  input. This is a clarity nit, not a bug — the dead branch is harmless. If you
  want, the branch (and the helper's bool return) can be simplified away; not
  required.

A candidate finding — that residual-marker truncation might discard legitimate
trailing prose — was investigated and dropped: _DSML_MARKER_RE requires the
literal multi-byte DSML sentinel glyph sequence plus tag syntax, which cannot
arise from ordinary or coincidental model prose. The risk is theoretical, not
practical.

Simplification opportunities

Lean already — no simplification findings on the two new commits. The rename to
_strip_dsml_markup_for_official_tool_calls is clean with zero dangling
references, and the residual-marker check reuses existing regex machinery with
no new dependencies or abstractions.

Test results

113/113 tests pass across test_deepseek_dsml.py, test_deepseek.py, and
test_react.py (exit code 0), including the new mixed-markup regression test.

Verdict

COMMENT (not approve).

Nothing in this diff regresses — the two new commits are correct, well-scoped,
and fully tested, and the previously-flagged MAJOR and both in-scope MEDIUM
issues that this PR set out to address are resolved. However, I am withholding
approval because two items remain genuinely open and are worth an explicit
answer before merge: the design question (#2) is only partially addressed —
the comment explains the intended scope but does not establish that the
ordinary tool-enabled streaming path is leak-free — and MEDIUM finding #4 (the
non-tool_calls path hard-crashes on partial DSML markup) is still unresolved,
now with a sharper asymmetry against the branch this PR just fixed. Both touch
the core purpose of this PR (eliminating DSML leaks) rather than being
unrelated pre-existing debt, so this is not a blocking REQUEST_CHANGES;
COMMENT records the open questions while acknowledging the diff itself is
safe. Recommend tracking both as explicit follow-ups (or an explicit
author confirmation that the risk is acceptable) before final approval. The
LOW dead-branch item is a non-blocking nit.

[rogercloud]

Nit (LOW, non-blocking): this if not stripped: return response, False guard is now unreachable. Reaching this branch requires contains_deepseek_dsml_tool_markup(content) to have already returned True above, and _strip_dsml_markup_for_official_tool_calls's residual-marker fallback guarantees stripped=True whenever that same marker-presence condition holds on the input. The branch is harmless, but if you want you could drop it and simplify the helper's return to just the cleaned content.

[rogercloud]

Summary

This PR fixes a DeepSeek DSML (tool-markup) leak: when the provider returns
authoritative tool_calls, residual DSML markup could survive in the visible
content and stream to the user. The fix is scoped to the provider module
deepseek_dsml.py; forced-final turns opt into buffered local DSML recovery
via a private sentinel, while ordinary tool-enabled streams stay
provider-native. This round adds two commits: 8cd01cf reworks the strip
helper into _strip_dsml_markup_for_official_tool_calls, which now also
truncates any residual/partial DSML marker left after complete blocks are
removed, and 1654735 expands an explanatory comment in deepseek.py
clarifying the stream-recovery scope.

Round 0 design verdict

Acceptable-with-reservations. The fix is well-factored — an isolated,
provider-scoped module with clean sentinel plumbing. One design-level gap
persists: DSML-recovery coverage is asymmetric between the
tool_calls-present branch (now degrades gracefully on partial markup) and
the tool_calls-absent branch (still hard-raises DeepSeekDSMLParseError on
partial/incomplete markup, uncaught up the stack) — see prior finding #4.

Status of prior findings

1. MAJOR — DeepSeekDSMLParseError raised around official tool_calls on
   partial markup — FIXED (still fixed). Confirmed the two new commits do not
   regress the round-3 fix.

2. Design question — "why doesn't DSML recovery cover the primary streaming
   ReAct path with tools?" — WAIVED-WITH-EXPLANATION (not fully resolved).
   The code path is byte-identical; only the comment text changed
   (deepseek.py:318-321). The new comment adds a genuine architectural
   rationale: forced-final turns intentionally send tools=None to the
   provider (so local DSML recovery is the only fallback), whereas ordinary
   tool-enabled streams send real tool schemas and rely on provider-native
   tool-calling. That is a reasonable why, but it does not answer the
   original question: is it actually verified that DeepSeek never leaks DSML
   markup on an ordinary tool-enabled streaming turn? No test, comment, or code
   change establishes that. If the provider ever does leak markup on that path,
   it streams straight to the user with zero recovery — the same gap as before.
   Recommend keeping this open pending an explicit answer, not treating it as
   closed by the comment alone.

3. MEDIUM — mixed complete+partial DSML leak — FIXED.
   _strip_dsml_markup_for_official_tool_calls
   (src/xagent/core/model/chat/basic/deepseek_dsml.py:180-188) now re-checks
   for a residual marker after stripping complete blocks and truncates at it.
   The new regression test
   test_normalize_deepseek_dsml_response_strips_mixed_complete_and_partial_markup
   reproduces the exact mixed-block case and passes. No gap remains within the
   finding's original scope.

4. MEDIUM — non-tool_calls path still crashes on partial/truncated DSML
   markup — NOT FIXED (still open; pre-existing, out of scope of this diff).
   _extract_dsml_tool_call_blocks / parse_deepseek_dsml_tool_calls and the
   whole call stack (deepseek.py chat(), runtime.py stream_final_answer,
   react.py) are byte-for-byte unchanged since the last review;
   DeepSeekDSMLParseError is still uncaught anywhere and still hard-fails the
   turn. Notably, on this path even a complete DSML block followed by a
   residual partial fragment raises rather than degrading — so the asymmetry
   with the now-fixed tool_calls-present branch (finding #3) is sharper than
   before: that branch degrades gracefully in the exact same mixed scenario,
   this one still hard-crashes. Nothing in this diff touches it; flagging for a
   follow-up.

5. Simplification — dead code _strip_dsml_tool_call_blocks — FIXED.
   Repo-wide grep confirms zero remaining references to the old function name
   or the round-3 intermediate name _strip_complete_dsml_tool_call_blocks.
   Cleanly consolidated into _strip_dsml_markup_for_official_tool_calls.

New findings (this round)

• LOW — dead/unreachable branch —
  src/xagent/core/model/chat/basic/deepseek_dsml.py:73-74. Inside
  normalize_deepseek_dsml_response, the if not stripped: return response, False guard after calling _strip_dsml_markup_for_official_tool_calls can
  never be true. Reaching this branch already requires
  contains_deepseek_dsml_tool_markup(content) to have returned True (line
  68), and the helper's own residual-marker fallback (lines 185-188) guarantees
  stripped=True whenever that same marker-presence condition holds on the
  input. This is a clarity nit, not a bug — the dead branch is harmless. If you
  want, the branch (and the helper's bool return) can be simplified away; not
  required.

A candidate finding — that residual-marker truncation might discard legitimate
trailing prose — was investigated and dropped: _DSML_MARKER_RE requires the
literal multi-byte DSML sentinel glyph sequence plus tag syntax, which cannot
arise from ordinary or coincidental model prose. The risk is theoretical, not
practical.

Simplification opportunities

Lean already — no simplification findings on the two new commits. The rename to
_strip_dsml_markup_for_official_tool_calls is clean with zero dangling
references, and the residual-marker check reuses existing regex machinery with
no new dependencies or abstractions.

Test results

113/113 tests pass across test_deepseek_dsml.py, test_deepseek.py, and
test_react.py (exit code 0), including the new mixed-markup regression test.

Verdict

COMMENT (not approve).

Nothing in this diff regresses — the two new commits are correct, well-scoped,
and fully tested, and the previously-flagged MAJOR and both in-scope MEDIUM
issues that this PR set out to address are resolved. However, I am withholding
approval because two items remain genuinely open and are worth an explicit
answer before merge: the design question (#2) is only partially addressed —
the comment explains the intended scope but does not establish that the
ordinary tool-enabled streaming path is leak-free — and MEDIUM finding #4 (the
non-tool_calls path hard-crashes on partial DSML markup) is still unresolved,
now with a sharper asymmetry against the branch this PR just fixed. Both touch
the core purpose of this PR (eliminating DSML leaks) rather than being
unrelated pre-existing debt, so this is not a blocking REQUEST_CHANGES;
COMMENT records the open questions while acknowledging the diff itself is
safe. Recommend tracking both as explicit follow-ups (or an explicit
author confirmation that the risk is acceptable) before final approval. The
LOW dead-branch item is a non-blocking nit.

[rogercloud]

Summary

This round re-reviews a single new commit, 84b0a1a9 "fix(deepseek): degrade incomplete dsml content", which restructures normalize_deepseek_dsml_response so the non-tool_calls path no longer raises an uncaught DeepSeekDSMLParseError on partial/truncated DSML markup. A new helper _trim_unmatched_dsml_marker_tail scans markers left-to-right and truncates the content at the first marker not enclosed in a complete <...tool_calls>...</...tool_calls> block; if no complete block survives, it degrades gracefully to stripped plain text instead of crashing. The commit touches only deepseek_dsml.py (30 lines) and its unit test file (44 new lines), and incidentally simplifies _strip_dsml_markup_for_official_tool_calls into a plain-string-returning _strip_dsml_markup_from_visible_content, removing a previously-flagged dead guard branch.

Round 0 — Design verdict

SOUND (acceptable with reservations). This is the right general fix for the crash bug, and the trim-then-check approach is well-reasoned for the common case (truncation at the tail of a stream). However, it introduces one new correctness regression (see MEDIUM below) by converting a loud failure into a silent one for a less-common but plausible input ordering.

Status of prior open findings

1. MEDIUM — non-tool_calls path crashed on partial/truncated DSML markup — FIXED. Verified live: normalize_deepseek_dsml_response with no tool_calls and a bare/truncated marker now returns (normalized, True) with degraded plain-text content instead of raising. New tests test_normalize_deepseek_dsml_response_strips_partial_markup_without_tool_calls and test_normalize_deepseek_dsml_response_recovers_complete_block_before_partial_markup cover this and pass. parse_deepseek_dsml_tool_calls is now only reached for genuinely complete-but-malformed blocks (e.g. an invoke missing its name), which is legitimate, distinct error behavior.

2. LOW — dead/unreachable guard branch (if not stripped: return response, False) — FIXED. The pattern no longer exists in the file. This commit's simplification of the tool_calls-present branch (dropping the tuple return for a plain string) is what eliminated it — a natural byproduct of the same refactor.

3. Design question — does DSML recovery need to cover the ordinary tool-enabled streaming path (not just forced-final turns)? — STILL WAIVED, no movement. This commit touches only deepseek_dsml.py and its test file; deepseek.py's stream_chat dispatch logic is unchanged. Ordinary tool-enabled streaming turns still bypass DSML recovery entirely, and the underlying empirical question (can DeepSeek leak DSML markup on that path?) remains unanswered by any code, comment, or test. Not introduced or worsened by this commit — carried forward as-is.

The two remaining prior findings (the original MAJOR crash bug and the mixed complete+partial leak MEDIUM) were fixed in earlier rounds and are untouched by this diff.

New findings this round

MEDIUM (regression) — silent drop of a valid tool call when a stray marker precedes a complete block

src/xagent/core/model/chat/basic/deepseek_dsml.py:191-198 (_trim_unmatched_dsml_marker_tail)

When a stray/unmatched closing tag or invoke/parameter marker appears before a later, fully well-formed <...tool_calls>...</...tool_calls> block, the function truncates at that leading stray marker and silently discards the valid block that follows. normalize_deepseek_dsml_response then returns changed=True with content degraded to plain text and no tool call recovered — no exception, no log signal, nothing to indicate a tool call was dropped.

Reproduced directly: input answer </||DSML||tool_calls>\n<complete valid block with invoke> yields {'content': 'answer', 'tool_calls': None} — the tool call is silently lost.

This is a regression in kind, not merely an unfixed edge case. Before this commit, the same input reached a different path (_extract_dsml_tool_call_blocks's residual-markup guard) and raised DeepSeekDSMLParseError — a loud, visible failure. This commit converts that loud failure into a silent one that drops a valid, executable tool call while returning high-confidence-looking changed=True output. The trigger (a stray marker preceding a complete block) is less common than the tail-truncation case this commit targets, but is plausible — e.g. an LLM emitting a garbled/duplicated tag from a self-correction before a clean retry in the same turn — and is not covered by any test.

Suggested direction: before truncating at an unmatched marker, check whether a complete block exists after it in the original text and prefer parsing that block; or, at minimum, only trim trailing (suffix) unmatched markers rather than the first one encountered anywhere in the string.

LOW — test coverage gaps

No test exercises the ordering in the finding above (stray marker before a valid block — the exact scenario that regresses). Separately, no adapter/integration-level test (DeepSeekLLM.chat()) exists for the new degrade-to-plain-text behavior; all coverage is at the normalize_deepseek_dsml_response / parse_deepseek_dsml_tool_calls unit level (confirmed: zero references to DeepSeekLLM/.chat( in the DSML test file, zero dsml references in test_deepseek.py).

NIT (non-blocking) — redundant strip in the degrade branch

src/xagent/core/model/chat/basic/deepseek_dsml.py:76-80. In the degrade branch, calling _strip_dsml_markup_from_visible_content(content) after _trim_unmatched_dsml_marker_tail has already removed everything from the first unmatched marker onward is a no-op for every input reaching that branch. Harmless but redundant; could simplify to content.strip() or None. Not required.

Simplification opportunities

One candidate was investigated (extracting a shared _strip_complete_dsml_blocks helper for the single regex-substitution line duplicated across _strip_dsml_markup_from_visible_content and _extract_dsml_tool_call_blocks) and dropped: it's one clear line in exactly two places with no divergence risk, so extracting a helper would be net-neutral-to-negative. The code is already lean here.

Test results

All tests pass: the three relevant test files (test_deepseek_dsml.py, test_deepseek.py, test_react.py) report 115/115 passing (exit code 0), including the two new regression tests for the degrade-to-plain-text and complete-block-recovery scenarios.

Verdict

REQUEST_CHANGES. This commit correctly fixes two prior issues (the MEDIUM crash on truncated markup and the LOW dead-guard branch), but it introduces a new MEDIUM regression squarely within this PR's own stated purpose — DSML handling correctness. Converting a loud DeepSeekDSMLParseError into a silent drop of a valid, executable tool call (with high-confidence changed=True output and no log signal) is a strictly worse failure mode than what existed before this commit, since silent data loss is far harder to detect and diagnose than an exception. The fix is small and well-scoped: prefer a complete block that appears after an unmatched marker, or restrict trimming to suffix markers only, plus a regression test for the stray-marker-before-valid-block ordering. The still-open streaming-path design question from earlier rounds remains waived and is not a blocker on its own.

[rogercloud]

MEDIUM (regression): this truncates at the first unmatched marker anywhere in the string, which silently discards a valid, complete <...tool_calls>...</...tool_calls> block that appears after a stray/unmatched marker. Reproduced: input answer </||DSML||tool_calls>\n<complete valid block with invoke> returns {'content': 'answer', 'tool_calls': None} — the executable tool call is dropped with no exception and no log signal, and normalize_deepseek_dsml_response still reports changed=True.

This is a regression in kind: before this commit the same input raised DeepSeekDSMLParseError via _extract_dsml_tool_call_blocks's residual-markup guard — a loud, visible failure. This commit turns that into silent data loss. The trigger (a stray/duplicated marker before a clean block, e.g. from an LLM self-correction retry within one turn) is less common than tail truncation but plausible, and is untested.

Suggested fix: before truncating at an unmatched marker, check whether a complete block exists after it and prefer parsing that block; or restrict trimming to trailing (suffix) unmatched markers only rather than the first one found anywhere. Please add a regression test for the stray-marker-before-valid-block ordering.