[https://nvbugs/6240584][fix] Qwen3ToolParser: bare-JSON fallback for reasoning-preceded tool calls#16866
Open
JunyiXu-nv wants to merge 3 commits into
Open
[https://nvbugs/6240584][fix] Qwen3ToolParser: bare-JSON fallback for reasoning-preceded tool calls#16866JunyiXu-nv wants to merge 3 commits into
JunyiXu-nv wants to merge 3 commits into
Conversation
… in detect_and_parse
Some Qwen3 chat templates (notably Qwen3.6 FP8 served with
--reasoning_parser qwen3_5 --tool_parser qwen3) emit tool calls as bare
JSON objects without the <tool_call>...</tool_call> wrapper once the
reasoning parser strips the </think> block. detect_and_parse() previously
returned calls=[] unconditionally whenever the bot_token ("<tool_call>\n")
was missing, so those tool calls leaked out as message.content with
finish_reason="stop".
Try json.loads + parse_base_json before falling through to the original
normal_text return. Streaming (parse_streaming_increment) and
has_tool_call() are intentionally unchanged.
Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
…SON fallback
This stacks on the initial fix in this PR and closes four gaps:
1) detect_and_parse: tighten exception handling. Replace
`except (JSONDecodeError, AttributeError)` with an explicit
`isinstance(parsed, (dict, list))` guard so bare JSON scalars
(e.g. "42", null) fall through cleanly without relying on
AttributeError.
2) parse_streaming_increment: extend the streaming path with the same
bare-JSON fallback. When the stream never contains `<tool_call>`
but the accumulated buffer parses as a JSON object matching
`{"name": ..., "arguments"/"parameters": ...}`, emit the tool
call using the base-class streaming pattern (name-first with empty
params, then arguments). Unblocks streaming clients on Qwen3.6 FP8
which otherwise saw the tool call leak into `delta.content` with
`finish_reason="stop"`. Wrapped happy path is preserved by
routing to the existing base implementation once `<tool_call>` is
detected.
3) has_tool_call() semantics unchanged. It still strictly gates on
`<tool_call>` for callers using it as a wrapper detector; the
bare-JSON fallback lives inside detect_and_parse /
parse_streaming_increment.
4) Tests. Unit tests exercise the bare-JSON fallback on
detect_and_parse (dict, list, `parameters` key, non-JSON text,
JSON scalar, malformed JSON) and on parse_streaming_increment
(single chunk, split chunks, no content leak, non-JSON flush,
wrapped-form regression). An integration test wires the qwen3_5
reasoning parser plus qwen3 tool parser through the postprocess
pipeline using the exact bug-report input
`<think>...</think>\\n{...}` and asserts `has_tool_call[0]` is
True (so downstream flips `finish_reason` to `tool_calls`).
Non-goals honored: no changes to other tool parsers, no changes to
openai_protocol.py or the API stability YAML.
Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…otocol
qwen3_5 is registered with `reasoning_at_start=True` because the Qwen3.6
chat template pre-injects `<think>\n` into the assistant prompt prefix,
so model output starts inside the reasoning block with no opening
`<think>` tag. `DeepSeekR1Parser.parse` in that mode only partitions on
`</think>`; it never strips a leading `<think>`.
The integration test was feeding `<think>Reasoning here.</think>\n{...}`
which does not match that protocol: the leading `<think>` was preserved
in `reasoning_content`, so the `reasoning_content == "Reasoning here."`
assertion failed on CI.
Update the input to match the real production shape (Reasoning content
+ `</think>` sentinel + bare JSON tool call) so the test exercises what
NVBug 6240584 actually describes.
Runtime fix in qwen3_tool_parser.py is unchanged and continues to cover
the bug scenario.
Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
JunyiXu-nv
force-pushed
the
user/junyix/fix-nvbug-6240584-v2
branch
from
July 25, 2026 04:44
2d47bcb to
1ef4ab3
Compare
Collaborator
Author
|
/bot run --disable-fail-fast |
Contributor
WalkthroughChangesQwen3 bare JSON parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ModelOutput
participant Qwen35ReasoningParser
participant Qwen3ToolParser
ModelOutput->>Qwen35ReasoningParser: output with reasoning wrapper and bare JSON
Qwen35ReasoningParser->>Qwen3ToolParser: stripped content and bare JSON
Qwen3ToolParser->>Qwen3ToolParser: parse JSON and emit tool call
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Collaborator
|
PR_Github #61680 [ run ] triggered by Bot. Commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tensorrt_llm/serve/tool_parser/qwen3_tool_parser.py (1)
189-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
current_tool_id/current_tool_name_sentbookkeeping is best-effort only.Setting
current_tool_id = max(0, len(calls) - 1)doesn't mirror the base class's convention of leavingcurrent_tool_idpointing past the last completed tool (base class incrementscurrent_tool_idafter each completed tool). Since the comment already flags this as "best-effort... for callers that inspect these" and the mode never falls back to wrapped streaming afterward, this is unlikely to cause a functional issue today, but could be worth a code comment noting the divergence from base-class semantics if any future caller inspectscurrent_tool_idfor multi-turn bookkeeping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/serve/tool_parser/qwen3_tool_parser.py` around lines 189 - 193, Document the intentional divergence from base-class semantics beside the current_tool_id/current_tool_name_sent assignments in the bare JSON initialization path. Note that current_tool_id is best-effort bookkeeping and does not represent the base class’s post-completion position for multi-tool calls; leave the existing behavior unchanged.tests/unittest/llmapi/apps/test_tool_parsers.py (1)
2846-2907: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winIntegration test only covers the non-streaming path.
The test explicitly notes
# Non-streaming path.and never exercisesapply_reasoning_parser/apply_tool_parserwithstreaming=True, even though the reported production scenario (NVBug 6240584) is most likely to occur during streamed chat completions. Consider adding a streaming variant that feeds the same reasoning+bare-JSON text in chunks and assertsargs.has_tool_call.get(0) is Trueat the end.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/llmapi/apps/test_tool_parsers.py` around lines 2846 - 2907, Extend test_qwen3_5_reasoning_plus_qwen3_tool_parser_bare_json_pipeline to cover streaming=True by feeding the same reasoning-plus-bare-JSON input in chunks through apply_reasoning_parser and apply_tool_parser. Assert the streamed result produces one get_weather tool call, sets args.has_tool_call[0] to True, and does not expose the bare JSON as visible content.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/serve/tool_parser/qwen3_tool_parser.py`:
- Around line 163-196: Update the bare-JSON parsing branch around json.loads in
the streaming parser to decode the first complete JSON value and track its end
index, rather than requiring the entire stripped buffer to be valid JSON. Emit
the parsed tool calls immediately when a valid dict or list is found, while
preserving any trailing content for subsequent normal-text processing and
avoiding indefinite buffering.
- Around line 77-90: Update the bare-JSON fallback in the tool parser to decode
only the leading JSON value using raw_decode() or the existing
partial_json_loads() helper, rather than requiring json.loads() to consume the
entire stripped text. Preserve dict/list validation and tool-call parsing, and
append any trailing text after the decoded value to normal_text when no tool
call is returned.
In `@tests/unittest/llmapi/apps/test_tool_parsers.py`:
- Around line 800-951: Add coverage for bare-JSON tool calls followed by
trailing non-whitespace content in both detect_and_parse and
parse_streaming_increment. Use input such as a complete get_weather JSON object
followed by newline-delimited prose, and assert the tool call is recovered while
the trailing text is handled as normal content without raising an Extra data
parsing error.
---
Nitpick comments:
In `@tensorrt_llm/serve/tool_parser/qwen3_tool_parser.py`:
- Around line 189-193: Document the intentional divergence from base-class
semantics beside the current_tool_id/current_tool_name_sent assignments in the
bare JSON initialization path. Note that current_tool_id is best-effort
bookkeeping and does not represent the base class’s post-completion position for
multi-tool calls; leave the existing behavior unchanged.
In `@tests/unittest/llmapi/apps/test_tool_parsers.py`:
- Around line 2846-2907: Extend
test_qwen3_5_reasoning_plus_qwen3_tool_parser_bare_json_pipeline to cover
streaming=True by feeding the same reasoning-plus-bare-JSON input in chunks
through apply_reasoning_parser and apply_tool_parser. Assert the streamed result
produces one get_weather tool call, sets args.has_tool_call[0] to True, and does
not expose the bare JSON as visible content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d68cbc50-2624-492a-9666-81769e51449a
📒 Files selected for processing (2)
tensorrt_llm/serve/tool_parser/qwen3_tool_parser.pytests/unittest/llmapi/apps/test_tool_parsers.py
Comment on lines
+77
to
90
| # Some Qwen3 chat templates (e.g. Qwen3.6 FP8 with thinking enabled) | ||
| # emit tool calls as bare JSON without a <tool_call> wrapper. Try | ||
| # to recover those before dropping the text into normal_text. Use | ||
| # an explicit type guard to avoid relying on AttributeError to | ||
| # catch scalar JSON like "42" or null. | ||
| try: | ||
| parsed = json.loads(text.strip()) | ||
| except json.JSONDecodeError: | ||
| return StreamingParseResult(normal_text=normal_text, calls=[]) | ||
| if isinstance(parsed, (dict, list)): | ||
| calls = self.parse_base_json(parsed, tools) | ||
| if calls: | ||
| return StreamingParseResult(normal_text="", calls=calls) | ||
| return StreamingParseResult(normal_text=normal_text, calls=[]) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the cited lines.
sed -n '1,220p' tensorrt_llm/serve/tool_parser/qwen3_tool_parser.py
# Find related parser implementations / helpers for comparison.
rg -n "json\.loads\(|raw_decode|parse_base_json|StreamingParseResult|normal_text" tensorrt_llm/serve/tool_parser -SRepository: NVIDIA/TensorRT-LLM
Length of output: 36110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' tensorrt_llm/serve/tool_parser/qwen3_tool_parser.py
rg -n "json\.loads\(|raw_decode|parse_base_json|StreamingParseResult|normal_text" tensorrt_llm/serve/tool_parser -SRepository: NVIDIA/TensorRT-LLM
Length of output: 36110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the target file with line numbers.
cat -n tensorrt_llm/serve/tool_parser/qwen3_tool_parser.py
# Inspect the base parser helper and sibling implementations for comparison.
for f in tensorrt_llm/serve/tool_parser/*.py; do
echo "===== $f ====="
rg -n "parse_base_json|raw_decode|json\.loads\(|StreamingParseResult|normal_text" "$f" -n -S || true
doneRepository: NVIDIA/TensorRT-LLM
Length of output: 28883
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n tensorrt_llm/serve/tool_parser/utils.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 5965
Parse the leading JSON value instead of the full string. json.loads(text.strip()) fails on "{...} trailing text" with Extra data, so a valid bare-JSON tool call gets sent back as normal_text. Use raw_decode() or the existing partial_json_loads() helper and keep any suffix as text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tensorrt_llm/serve/tool_parser/qwen3_tool_parser.py` around lines 77 - 90,
Update the bare-JSON fallback in the tool parser to decode only the leading JSON
value using raw_decode() or the existing partial_json_loads() helper, rather
than requiring json.loads() to consume the entire stripped text. Preserve
dict/list validation and tool-call parsing, and append any trailing text after
the decoded value to normal_text when no tool call is returned.
Comment on lines
+163
to
+196
| # Leading `{` or `[` — could be a bare JSON tool call, or partial. | ||
| try: | ||
| parsed = json.loads(stripped) | ||
| except json.JSONDecodeError: | ||
| # Not yet complete — buffer more. | ||
| return StreamingParseResult() | ||
|
|
||
| if isinstance(parsed, (dict, list)): | ||
| calls = self.parse_base_json(parsed, tools) | ||
| if calls: | ||
| streaming_calls: List[ToolCallItem] = [] | ||
| for i, call_item in enumerate(calls): | ||
| # Emit name first with empty parameters, then the full | ||
| # arguments — mirrors the base class streaming pattern. | ||
| streaming_calls.append( | ||
| ToolCallItem( | ||
| tool_index=i, | ||
| name=call_item.name, | ||
| parameters="", | ||
| )) | ||
| if call_item.parameters: | ||
| streaming_calls.append( | ||
| ToolCallItem( | ||
| tool_index=i, | ||
| parameters=call_item.parameters, | ||
| )) | ||
| self._stream_mode = self._STREAM_MODE_BARE_JSON | ||
| self._bare_json_buffer = "" | ||
| # Best-effort bookkeeping for callers that inspect these. | ||
| self.current_tool_id = max(0, len(calls) - 1) | ||
| self.current_tool_name_sent = True | ||
| return StreamingParseResult(normal_text="", | ||
| calls=streaming_calls) | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files 'tensorrt_llm/serve/tool_parser/*' | sed -n '1,120p'
printf '\n== Outline: qwen3_tool_parser.py ==\n'
ast-grep outline tensorrt_llm/serve/tool_parser/qwen3_tool_parser.py --view expanded || true
printf '\n== Outline: base_tool_parser.py ==\n'
ast-grep outline tensorrt_llm/serve/tool_parser/base_tool_parser.py --view expanded || true
printf '\n== Relevant slices: qwen3_tool_parser.py ==\n'
sed -n '130,230p' tensorrt_llm/serve/tool_parser/qwen3_tool_parser.py
printf '\n== Relevant slices: base_tool_parser.py ==\n'
sed -n '1,260p' tensorrt_llm/serve/tool_parser/base_tool_parser.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 18923
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Search for flush/finish/end-of-stream hooks ==\n'
rg -n "def (finish|flush|end|close)|finish\(" tensorrt_llm/serve/tool_parser tensorrt_llm/serve -g '*.py' || true
printf '\n== Search for qwen3 tool parser state usage ==\n'
rg -n "_stream_mode|_bare_json_buffer|parse_streaming_increment|current_tool_name_sent|current_tool_id" tensorrt_llm/serve/tool_parser/qwen3_tool_parser.py tensorrt_llm/serve -g '*.py' || true
printf '\n== Search for streaming parser invocation sites ==\n'
rg -n "parse_streaming_increment\(" tensorrt_llm/serve -g '*.py' || true
printf '\n== Relevant slice: qwen3_tool_parser.py around streaming branch ==\n'
sed -n '107,220p' tensorrt_llm/serve/tool_parser/qwen3_tool_parser.py
printf '\n== Relevant slice: any end-of-stream handling in serving layer ==\n'
rg -n "StreamingParseResult|current_tool_name_sent|streamed_args_for_tool|prev_tool_call_arr" tensorrt_llm/serve -g '*.py' || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '960,1035p' tensorrt_llm/serve/responses_utils.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 3003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Streaming loop around tool parser calls ==\n'
sed -n '1460,1535p' tensorrt_llm/serve/responses_utils.py
printf '\n== Search for any parser finalization after streaming ends ==\n'
rg -n "tool_parser_dict|parse_streaming_increment\(text, tools\)|finished|finish_result|flush" tensorrt_llm/serve/responses_utils.py tensorrt_llm/serve/postprocess_handlers.py -g '*.py' || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 7894
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '150,340p' tensorrt_llm/serve/postprocess_handlers.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 8169
Handle trailing content in the bare-JSON streaming branch (tensorrt_llm/serve/tool_parser/qwen3_tool_parser.py:163-196) — json.loads(stripped) still treats a complete JSON prefix plus trailing text as “incomplete,” so the parser can stay stuck buffering forever and never emit the tool call. Parse the JSON boundary separately (for example with raw_decode()/end-index tracking) instead of waiting on the whole buffer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tensorrt_llm/serve/tool_parser/qwen3_tool_parser.py` around lines 163 - 196,
Update the bare-JSON parsing branch around json.loads in the streaming parser to
decode the first complete JSON value and track its end index, rather than
requiring the entire stripped buffer to be valid JSON. Emit the parsed tool
calls immediately when a valid dict or list is found, while preserving any
trailing content for subsequent normal-text processing and avoiding indefinite
buffering.
Comment on lines
+800
to
+951
| # ------------------------------------------------------------------ | ||
| # NVBug 6240584: bare-JSON fallback in detect_and_parse | ||
| # | ||
| # Some Qwen3 chat templates (notably Qwen3.6 FP8 with | ||
| # `--reasoning_parser qwen3_5 --tool_parser qwen3`) emit tool calls | ||
| # as bare JSON, without a `<tool_call>...</tool_call>` wrapper, once | ||
| # the reasoning parser strips the `</think>` block. The parser must | ||
| # recover those before dropping the text into `normal_text`. | ||
| # ------------------------------------------------------------------ | ||
|
|
||
| def test_detect_and_parse_bare_json_dict(self, sample_tools, parser): | ||
| """Bare JSON dict without <tool_call> wrapper is parsed as a tool call.""" | ||
| text = '{"name":"get_weather","arguments":{"location":"Paris"}}' | ||
|
|
||
| result = parser.detect_and_parse(text, sample_tools) | ||
|
|
||
| assert result.normal_text == "" | ||
| assert len(result.calls) == 1 | ||
| assert result.calls[0].name == "get_weather" | ||
| assert json.loads(result.calls[0].parameters) == {"location": "Paris"} | ||
|
|
||
| def test_detect_and_parse_bare_json_list(self, sample_tools, parser): | ||
| """Bare JSON list of tool calls without wrapper is parsed.""" | ||
| text = '[{"name":"get_weather","arguments":{"location":"Paris"}}]' | ||
|
|
||
| result = parser.detect_and_parse(text, sample_tools) | ||
|
|
||
| assert result.normal_text == "" | ||
| assert len(result.calls) == 1 | ||
| assert result.calls[0].name == "get_weather" | ||
| assert json.loads(result.calls[0].parameters) == {"location": "Paris"} | ||
|
|
||
| def test_detect_and_parse_bare_json_parameters_key(self, sample_tools, | ||
| parser): | ||
| """Bare JSON with `parameters` (instead of `arguments`) is still parsed.""" | ||
| text = '{"name":"get_weather","parameters":{"location":"Paris"}}' | ||
|
|
||
| result = parser.detect_and_parse(text, sample_tools) | ||
|
|
||
| assert len(result.calls) == 1 | ||
| assert result.calls[0].name == "get_weather" | ||
| assert json.loads(result.calls[0].parameters) == {"location": "Paris"} | ||
|
|
||
| def test_detect_and_parse_non_json_text_falls_through( | ||
| self, sample_tools, parser): | ||
| """Plain non-JSON text passes through as normal_text with no calls.""" | ||
| text = "Hello world" | ||
|
|
||
| result = parser.detect_and_parse(text, sample_tools) | ||
|
|
||
| assert result.normal_text == "Hello world" | ||
| assert result.calls == [] | ||
|
|
||
| def test_detect_and_parse_bare_json_scalar_falls_through( | ||
| self, sample_tools, parser): | ||
| """A JSON scalar (e.g. `"42"`) must fall through cleanly, not crash. | ||
|
|
||
| This exercises the explicit `isinstance(parsed, (dict, list))` guard — | ||
| `parse_base_json` would raise `AttributeError` on a bare int, so the | ||
| guard prevents relying on exception catching for scalar JSON. | ||
| """ | ||
| text = "42" | ||
|
|
||
| result = parser.detect_and_parse(text, sample_tools) | ||
|
|
||
| assert result.calls == [] | ||
| # No crash is the important part. | ||
|
|
||
| def test_detect_and_parse_malformed_bare_json_falls_through( | ||
| self, sample_tools, parser): | ||
| """Malformed JSON without <tool_call> wrapper falls through cleanly.""" | ||
| text = '{"name": "get_weather", "arguments": MALFORMED}' | ||
|
|
||
| result = parser.detect_and_parse(text, sample_tools) | ||
|
|
||
| assert result.calls == [] | ||
| assert result.normal_text == text | ||
|
|
||
| # ------------------------------------------------------------------ | ||
| # NVBug 6240584: bare-JSON fallback in parse_streaming_increment | ||
| # | ||
| # The streaming path must also recover bare-JSON tool calls when the | ||
| # `<tool_call>` wrapper never appears. Without this, streaming clients | ||
| # receive the JSON as `delta.content` with `finish_reason="stop"`. | ||
| # ------------------------------------------------------------------ | ||
|
|
||
| def test_streaming_bare_json_one_chunk(self, sample_tools, parser): | ||
| """A complete bare-JSON tool call arriving in a single chunk emits calls.""" | ||
| result = parser.parse_streaming_increment( | ||
| '{"name":"get_weather","arguments":{"city":"Paris"}}', sample_tools) | ||
|
|
||
| names = [c.name for c in result.calls if c.name] | ||
| assert "get_weather" in names | ||
| params = "".join(c.parameters for c in result.calls if c.parameters) | ||
| assert "Paris" in params | ||
| assert result.normal_text == "" | ||
|
|
||
| def test_streaming_bare_json_split_across_chunks(self, sample_tools, | ||
| parser): | ||
| """Bare-JSON tool call split across multiple chunks parses on completion.""" | ||
| r1 = parser.parse_streaming_increment('{"name":"get_', sample_tools) | ||
| r2 = parser.parse_streaming_increment('weather","arguments":', | ||
| sample_tools) | ||
| r3 = parser.parse_streaming_increment('{"city":"Paris"}}', sample_tools) | ||
|
|
||
| all_calls = list(r1.calls) + list(r2.calls) + list(r3.calls) | ||
| names = [c.name for c in all_calls if c.name] | ||
| assert "get_weather" in names | ||
| params = "".join(c.parameters for c in all_calls if c.parameters) | ||
| assert "Paris" in params | ||
|
|
||
| def test_streaming_bare_json_does_not_leak_content(self, sample_tools, | ||
| parser): | ||
| """After a bare-JSON tool call is emitted, trailing text is not leaked. | ||
|
|
||
| This must be the case even for subsequent empty/whitespace chunks: | ||
| leaking any normal_text would flip `finish_reason` back to `stop`. | ||
| """ | ||
| r1 = parser.parse_streaming_increment( | ||
| '{"name":"get_weather","arguments":{"city":"Paris"}}', sample_tools) | ||
| # Any subsequent chunks must not emit normal_text either. | ||
| r2 = parser.parse_streaming_increment("", sample_tools) | ||
|
|
||
| assert r1.normal_text == "" | ||
| assert r2.normal_text == "" | ||
|
|
||
| def test_streaming_non_json_text_flushed_as_normal(self, sample_tools, | ||
| parser): | ||
| """Non-JSON text without a wrapper is flushed to normal_text.""" | ||
| result = parser.parse_streaming_increment("Hello world", sample_tools) | ||
|
|
||
| assert result.normal_text == "Hello world" | ||
| assert result.calls == [] | ||
|
|
||
| def test_streaming_wrapped_form_unregressed(self, sample_tools, parser): | ||
| """The pre-existing wrapped-form streaming path continues to work.""" | ||
| # Send bot token. | ||
| parser.parse_streaming_increment("<tool_call>\n", sample_tools) | ||
|
|
||
| # Partial JSON with name -> emits name with empty params. | ||
| r_name = parser.parse_streaming_increment('{"name":"get_weather"', | ||
| sample_tools) | ||
| assert len(r_name.calls) == 1 | ||
| assert r_name.calls[0].name == "get_weather" | ||
| assert r_name.calls[0].parameters == "" | ||
|
|
||
| # Complete the JSON and the wrapper. | ||
| r_args = parser.parse_streaming_increment( | ||
| ',"arguments":{"location":"SF"}}\n</tool_call>', sample_tools) | ||
| assert len(r_args.calls) == 1 | ||
| assert json.loads(r_args.calls[0].parameters) == {"location": "SF"} | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Good coverage of the new bare-JSON fallback paths, but missing a "trailing content" case.
None of the new tests exercise a bare-JSON tool call followed by trailing non-whitespace content (e.g. two back-to-back bare-JSON objects, or prose after the JSON), which is exactly the scenario that trips json.loads's "Extra data" error in both detect_and_parse and parse_streaming_increment (see the corresponding comments on qwen3_tool_parser.py). Adding a test like text = '{"name":"get_weather","arguments":{}}\nExtra text' for both detect_and_parse and streaming would have caught this.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unittest/llmapi/apps/test_tool_parsers.py` around lines 800 - 951, Add
coverage for bare-JSON tool calls followed by trailing non-whitespace content in
both detect_and_parse and parse_streaming_increment. Use input such as a
complete get_weather JSON object followed by newline-delimited prose, and assert
the tool call is recovered while the trailing text is handled as normal content
without raising an Extra data parsing error.
Collaborator
|
PR_Github #61680 [ run ] completed with state
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dev Engineer Review
<tool_call>parsing andhas_tool_call()behavior.QA Engineer Review
Test-code changes were made in
tests/unittest/llmapi/apps/test_tool_parsers.py.Coverage includes:
reasoning_at_start=True.No
tests/integration/test_lists/,test-db/, orqa/entries were modified, so coverage is not registered in the CI/manual QA test lists.Verdict: needs follow-up.
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.