Repository navigation
fix(grpc): report max_tokens and skip MCP dispatch for tool calls cut short - #2718
Conversation
The JSON parsers (json, llama, mistral, qwen and cohere, through the
shared streaming helper) and minimax_m2 streamed only the name of a
complete call that has no arguments, such as `{"name": "ping"}` or an
`<invoke>` without parameters, while their non-streaming parse gives
the call `{}`. A stream then ended with the call's arguments missing.
The JSON helper also stayed on such a call, so a call after it in the
same stream was lost.
Stream `{}` when such a call completes, as qwen_xml already does. The
JSON helper then moves on to the next call, as after a call with
arguments. A call cut short before its arguments still has none.
Signed-off-by: yechank <161688079+yechank-nvidia@users.noreply.github.com>
…l call A Messages response stopped with `tool_use` whenever a tool call was parsed, even when the engine stopped at the token limit. Tool parsers that report a call's name before its arguments (qwen_xml and others) start a `tool_use` block as soon as the name is complete, so a stream cut off inside a call ended as if the model had asked for the tool. A whole call followed by output cut off at the limit ended the same way, streamed or not. Check a `length` finish first and report `max_tokens`, in the streaming and the non-streaming path. Chat already keeps `length` when calls were parsed, and Responses reports such a response as incomplete. The content blocks are unchanged. Signed-off-by: yechank <161688079+yechank-nvidia@users.noreply.github.com>
A streaming tool parser reports a call's name before its arguments, so
a server call whose output ended before its arguments, or whose
arguments did not parse, reached the Responses tool loop with arguments
that are not JSON. The loop ran the MCP tool with `{}` for it, as it
does for any arguments that are not a JSON object.
Treat a server call whose arguments are not JSON like the calls of a
truncated or failed generation, in the streaming and the non-streaming
loop: nothing is dispatched, the loop ends, and the unfinished server
call is not handed to the client as a function call. Results of earlier
iterations are kept. Arguments that are JSON but not an object still
run with `{}`, and so does a complete call without arguments, which
streams `{}`. The non-streaming parse returns JSON arguments with every
parser today; the check there keeps the two loops alike.
Signed-off-by: yechank <161688079+yechank-nvidia@users.noreply.github.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (12)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughComplete tool calls without arguments now emit ChangesEmpty tool-call arguments
Gateway tool-call response handling
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The change preserves empty arguments for complete tool calls, blocks incomplete MCP dispatch, and reports max_tokens for length-limited Messages responses. No merge-blocking issue remains identified; merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change reduces accidental execution of incomplete tool calls and reports truncation more accurately. No introduced security issue was established, but downstream client behavior and production security boundaries are not fully evidenced. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Description
Problem
A streaming tool parser reports a call's name as soon as it is complete and the arguments after it. When the output stops inside a call, or a call's arguments never become JSON, two gRPC paths still treat the call as a finished one.
Messages reports
tool_usefor a truncated response. The stop reason istool_usewhenever a call was parsed, even when the engine stopped at the token limit. Withqwen_xml, an output cut atmax_tokensinside a call (<tool_call>\n<function=lookup>\n<parameter=city>\nPar) starts atool_useblock and ends withstop_reason: "tool_use", so a client that acts ontool_useruns the tool with a partial input. A whole call followed by text that is cut at the limit ends the same way, streamed or not. For the same output, Chat reportsfinish_reason: "length"and Responses reportsincomplete.The Responses tool loop runs an MCP call whose arguments are not JSON. It runs the server tool with
{}, as it does for any arguments that are not a JSON object. When streaming, a server call reaches the loop with arguments that are not JSON when the output stops (finish_reason: "stop"):json,llama,mistral,qwen(<tool_call>\n{"name": "brave_web_search") andinkling;deepseek,deepseek31,kimik2,hy_v4andinkling({"city": "Par);deepseek,deepseek31andkimik2({"city": Paris}).The non-streaming parse of these outputs returns no call. In the contract-test harness, where the mock model repeats its output,
mainruns the tool with{}on every turn of the streamed response until the internal limit of 10 calls, and the response fails withmax_tool_calls_exceeded. The same request without streaming completes and runs nothing.A guard on the MCP loop needs a parser fix first.
json,llama,mistral,qwenandcohere(through the shared JSON streaming helper) andminimax_m2stream only the name of a complete call that takes no arguments, such as{"name": "ping"}or an<invoke>without parameters. Their non-streaming parse returns{}for it. A Chat stream then ends with the call'sargumentsmissing, and with the MCP guard alone a streamed MCP call without arguments would no longer run. The JSON helper also stays on such a call, so a later call in the same stream is lost:jsonandqwenstream one call for two calls without arguments.Solution
Three commits, the parser fix first:
fix(tool_parser): stream{}when a call without arguments completes, asqwen_xmlalready does. The JSON helper then moves on to the next call, as after a call with arguments. A call cut short before its arguments still has none.fix(grpc): in Messages, check alengthfinish before the parsed calls and reportmax_tokens, in the streaming and the non-streaming path. The content blocks are unchanged.fix(responses): treat a server call whose arguments are not JSON like the calls of a truncated or failed generation, in the streaming and the non-streaming loop. Nothing in that batch is dispatched, the loop ends, and the unfinished server call is not handed to the client as a function call. Results of earlier iterations are kept. Arguments that are JSON but not an object still run with{}, and so does a complete call without arguments, which now streams{}. The non-streaming parse of every registered parser returns JSON arguments, so there the check only keeps the two loops alike.Changes
crates/tool_parser/src/parsers/helpers.rs: inhandle_json_tool_streaming, a complete call withoutarguments(orparameters) takes{}.crates/tool_parser/src/parsers/minimax_m2.rs: at</invoke>, stream{}when nothing was streamed for the call.crates/tool_parser/tests/tool_parser_streaming_flush.rs:a_call_without_arguments_streams_an_empty_objectforjson,llama,mistralandqwen, fed in one chunk and in 2–3 character chunks.tool_parser_minimax_m2.rs:test_minimax_streaming_empty_parameters.tool_parser_cohere.rs:test_cohere_streaming_empty_parameters.model_gateway/src/routers/grpc/regular/streaming.rsandprocessor.rs: the Messagesstop_reasoncheckslengthfirst.model_gateway/src/routers/grpc/regular/streaming/eof_tests.rs:messages_truncated_after_a_tool_call_starts_stop_at_max_tokensrunsqwen_xmlthrough the streaming and the non-streaming Messages path, withlengthandstop.model_gateway/src/routers/grpc/regular/responses/common.rs:has_unfinished_mcp_call. The streaming and the non-streaming loop call it next to the existing check for a truncated or failed generation.model_gateway/tests/grpc_responses_stream_contract_test.rsmcp_call_without_json_arguments_never_dispatches: the output ends after the server call's name.mcp_call_without_arguments_runs_with_an_empty_object: a complete server call without arguments still runs, with{}.The harmony Responses loop is not changed.
Behavior changes
max_tokensinstead oftool_usewhen the finish islengthand a call was parsed, including a namedtool_choice. Astopfinish with calls still givestool_use. Chat and Responses are unchanged.stopfinish givescompleted. Function tools and arguments that are JSON but not an object are unchanged.{}withjson,llama,mistral,qwen,cohereandminimax_m2, as their non-streaming parse returns.Test Plan
Negative control. I ran every new test by name on
main(61b250f7) with only the test files of this PR added.maina_call_without_arguments_streams_an_empty_object[json] arguments""instead of{}test_minimax_streaming_empty_parameters""instead of{}test_cohere_streaming_empty_parameters""instead of{}messages_truncated_after_a_tool_call_starts_stop_at_max_tokenstool_useinstead ofmax_tokens(streamed, cut inside the call)mcp_call_without_json_arguments_never_dispatchesfailedafter 10 MCP callsmcp_call_without_arguments_runs_with_an_empty_objectmainruns every call)The last test guards the parser fix together with the MCP guard. With the MCP guard and without the parser fix, its streamed case fails (see the mutation check).
Mutation check. On a copy of this branch, I reverted one change at a time and ran the new tests.
mcp_call_without_arguments_runs_with_an_empty_object: the streamed call runs 0 times instead of oncemcp_call_without_json_arguments_never_dispatches: the streamed response failsstop_reasonorder in Messages streamingmessages_truncated_after_a_tool_call_starts_stop_at_max_tokens:tool_usefor the streamed call cut insidestop_reasonorder in Messages non-streamingmessages_truncated_after_a_tool_call_starts_stop_at_max_tokens:tool_usefor the whole call followed by textAll registered tool parsers. A scratch test fed a complete call without arguments to each of the 24 registered parsers other than
passthrough, token by token with special tokens kept whole, and compared the streamed arguments with the non-streaming parse. Onmain,json,llama,mistral,qwen,cohereandminimax_m2stream""where the non-streaming parse returns{}. With this PR, all 24 stream the same arguments as their non-streaming parse. The same scratch test fed the whole output in one chunk. The differences left there are existing ones that do not depend on arguments:json,mistralandqwenstream only the first of two calls given in one chunk (mistraltoken by token too), andcohereandstep3stream no call when the whole block comes in one final chunk.Commands. All ran offline on CPU.
cargo +nightly fmt --all -- --checkcargo test -p tool-parsercargo test -p smg --libeof_tests)cargo test -p smg --testgrpc_responses_stream_contract_test/messages_streaming_test/messages_test/api_tests/spec_test/grpc_context_length_test/grpc_pd_fanout_testcargo clippy --workspace --all-targets -- -D warnings;-p smg --all-targetswith default features and withgrpc-server,jemalloc-profiling,test-util; CI's two--no-default-featuresvariants;-p tool-parser --all-targets --all-featurestool-parsertests,eof_tests,grpc_responses_stream_contract_testpre-commit run --all-fileswith CI'sSKIPlist, and thecommit-msghooks on each commitOpen PRs. A 3-way merge with #2715 and with #2716 is clean in both orders. On the merge of this PR with each of them,
eof_tests,grpc_responses_stream_contract_test,messages_streaming_test,messages_testandcargo clippy -p smg --all-targets -- -D warningspass. The new Messages test builds itsMessagesResponseSpecfrom a request, so it does not need the field that #2716 adds to that struct.The offline crate cache did not have the three dependency bumps on
main:lru0.18.5,cc1.5.1 and theopentelemetry-proto0.33 dev-dependency. So the build copies used theCargo.lockfrom before those bumps and the dev-dependency at 0.32. No source file differed.Checklist
cargo +nightly fmtpassescargo clippy --all-targets -- -D warningspasses for the workspace,-p smgwith default features and withgrpc-server,jemalloc-profiling,test-util, CI's two--no-default-featuresvariants, and-p tool-parser --all-features.--all-featuresfor the workspace needs OpenCV and was not run offline.