fix(grpc): preserve GLM output across stream boundaries - #1906
fix(grpc): preserve GLM output across stream boundaries#1906TongLing916 wants to merge 1 commit into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes improve model-to-parser resolution, strengthen GLM structured and streaming parsing, add parser finalization, verify SGLang request construction, and propagate reasoning, tool, tokenization, and decoding failures through gateway responses and streams. ChangesTool parser and model resolution
Gateway error propagation
SGLang request contract
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant Tokenizer
participant Parser
participant SseEncoder
Client->>Gateway: send generation request
Gateway->>Tokenizer: decode model output
Tokenizer-->>Gateway: text or decoding error
Gateway->>Parser: parse reasoning and tool output
Parser-->>Gateway: response chunks or parser error
Gateway->>SseEncoder: encode success or failure
SseEncoder-->>Client: stream response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
grpc
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the GLM4 Moe parser to support strict validation of argument tags, handle incremental streaming with structural markers, and reject malformed or incomplete tool calls. It also propagates parsing errors across the model gateway, updates SGLang request construction with new fields, and enhances schema type resolution. The review feedback highlights a critical bug in the incremental streaming logic where a complete start token is prematurely emitted as normal text when the end token is missing, and suggests a performance optimization to avoid redundant lookups during model resolution.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Downstream release coordination: SGLang's legacy gRPC contract branch now rejects the public |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Hi @TongLing916, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
6a12d19 to
1355f25
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a12d19c95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/tool_parser/src/parsers/glm4_moe.rs (1)
145-154: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject empty GLM-4.5 function names.
The GLM-4.5 regex permits an empty first line, so
<tool_call>\n</tool_call>succeeds throughparse_complete()with an empty function name when no schemas are supplied. Validatefunc_nameafter trimming and returnParsingFailedwhen empty.🤖 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 `@crates/tool_parser/src/parsers/glm4_moe.rs` around lines 145 - 154, Update parse_tool_call to validate the trimmed func_name before resolving parameter types or parsing arguments; when it is empty, return ParsingFailed so empty GLM-4.5 tool calls cannot succeed without schemas.model_gateway/src/routers/grpc/regular/processor.rs (1)
644-657: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake both Messages pipelines fail closed on parser errors.
The Messages-specific paths still bypass the new parser error contract:
model_gateway/src/routers/grpc/regular/processor.rs#L644-L657: return an HTTP 502 instead of logging and preserving malformed tool text.model_gateway/src/routers/grpc/regular/streaming.rs#L1893-L1893: propagate reasoning/tool errors and finalize the parser before emitting successful terminal events.🤖 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 `@model_gateway/src/routers/grpc/regular/processor.rs` around lines 644 - 657, The Messages-specific pipelines must fail closed on parser errors. In model_gateway/src/routers/grpc/regular/processor.rs lines 644-657, update the parse_tool_calls error branch to return an HTTP 502 rather than logging the error and preserving malformed tool text. In model_gateway/src/routers/grpc/regular/streaming.rs line 1893, propagate reasoning/tool parser errors, finalize the parser before emitting successful terminal events, and preserve normal success behavior.
🤖 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 `@crates/tool_parser/src/parsers/glm4_moe.rs`:
- Around line 205-218: Update partial structural-marker detection in
partial_structural_marker_suffix_len and its streaming/EOF callers so a one-byte
“<” suffix is not treated as incomplete; only retain meaningful prefixes such as
“<tool_” or “<arg_”. Ensure non-streaming parsing accepts text ending in “<” and
streaming finalization releases it as normal text while still rejecting longer
incomplete structural prefixes.
- Around line 311-325: Update the parser’s pre-tool-call path around
current_tool_id and contains_structural_fragment so exact unmatched structural
markers, including closing tool-call and argument markers, and meaningful
structural fragments are rejected before any early return. Apply the same
validation to the complete parsing path that currently returns when no opening
tool call exists, while preserving normal text handling for non-structural
content.
In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 1265-1299: The tool-enabled streaming branch around
process_specific_function_stream and process_tool_calls_stream currently emits
normal text without choice_logprobs. Pass the existing logprobs through both
tool-processing paths, including the corresponding branch around the alternate
location, and ensure tool_result_to_chat_chunks uses
add_choice_content_with_logprobs for ordinary text while preserving current
behavior when logprobs are absent.
---
Outside diff comments:
In `@crates/tool_parser/src/parsers/glm4_moe.rs`:
- Around line 145-154: Update parse_tool_call to validate the trimmed func_name
before resolving parameter types or parsing arguments; when it is empty, return
ParsingFailed so empty GLM-4.5 tool calls cannot succeed without schemas.
In `@model_gateway/src/routers/grpc/regular/processor.rs`:
- Around line 644-657: The Messages-specific pipelines must fail closed on
parser errors. In model_gateway/src/routers/grpc/regular/processor.rs lines
644-657, update the parse_tool_calls error branch to return an HTTP 502 rather
than logging the error and preserving malformed tool text. In
model_gateway/src/routers/grpc/regular/streaming.rs line 1893, propagate
reasoning/tool parser errors, finalize the parser before emitting successful
terminal events, and preserve normal success behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4abab061-09ee-40ab-8f9e-6cb5befe00e6
📒 Files selected for processing (9)
crates/tool_parser/src/factory.rscrates/tool_parser/src/parsers/glm4_moe.rscrates/tool_parser/src/parsers/helpers.rscrates/tool_parser/src/traits.rscrates/tool_parser/tests/tool_parser_glm47_moe.rsgrpc_servicer/smg_grpc_servicer/sglang/servicer.pygrpc_servicer/tests/test_sglang_generate_request_contract.pymodel_gateway/src/routers/grpc/regular/processor.rsmodel_gateway/src/routers/grpc/regular/streaming.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca158756b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
model_gateway/src/routers/grpc/regular/streaming.rs (1)
1321-1433: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider accepting
&ChatTextProcessingContextinstead of 9–11 individually-destructured parameters.This PR introduces
ChatTextProcessingContextspecifically to reduce parameter sprawl forprocess_chat_text_delta, but the two functions it delegates to still takerequest_id,model,created,system_fingerprint,history_tool_calls_count,tool_choice/use_json_parseretc. as separate positional args (each still wrapped in#[expect(clippy::too_many_arguments)]). Passingcontextdirectly would fully realize the refactor, remove the clippy suppressions, and reduce the risk of a positional-argument mix-up across this many parameters (which would also make it easier to threadchoice_logprobsthrough, per the sibling comment above).🤖 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 `@model_gateway/src/routers/grpc/regular/streaming.rs` around lines 1321 - 1433, Update process_specific_function_stream and process_tool_calls_stream to accept a shared &ChatTextProcessingContext instead of individually passing request_id, model, created, system_fingerprint, history_tool_calls_count, and tool_choice/use_json_parser; read the required values from the context at each call site. Remove the #[expect(clippy::too_many_arguments)] suppressions and preserve existing parsing and chunk-generation behavior.model_gateway/src/routers/grpc/regular/processor.rs (1)
644-657: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate Messages API tool-call parse failures
processor.rs:645-656still logsparse_tool_callserrors and continues, so Messages clients can receive 200 OK with raw tool-call markup inprocessed_text. The streaming Messages path does the same; match the Chat Completions path and fail these requests withupstream_output_parse_failedinstead.🤖 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 `@model_gateway/src/routers/grpc/regular/processor.rs` around lines 644 - 657, Update the Messages API tool-call handling around parse_tool_calls to propagate parsing failures instead of logging and continuing. In the Err branch, return the established upstream_output_parse_failed error, matching the Chat Completions path; apply the same behavior to the streaming Messages path so failed parsing cannot return raw tool-call markup or a successful response.
♻️ Duplicate comments (1)
model_gateway/src/routers/grpc/regular/streaming.rs (1)
1228-1319: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
choice_logprobsis dropped whenever the tool-call branch runs — requested logprobs are silently lost for tool-enabled streaming requests (previously flagged, still unresolved).Adds
finalize(...)calls on pooledtool_parser::ToolParserinstances after the token stream ends, emitting any buffered tool call outputs. More importantly,process_chat_text_deltareceiveschoice_logprobs: Option<ChatLogProbs>per chunk, but only forwards it toadd_choice_content_with_logprobsin the final non-tool branch (line ~1307). Whencontext.toolsisSomeand the tool branch is taken:let tool_chunks = if context.is_specific_function { Self::process_specific_function_stream(...) // no logprobs param } else { self.process_tool_calls_stream(...).await? // no logprobs param }; chunks.extend(tool_chunks); return Ok(chunks);
choice_logprobsis discarded entirely and never reachestool_result_to_chat_chunks, whose normal-text emission still uses the logprobs-less builder:if !result.normal_text.is_empty() { chunks.push( ChatCompletionStreamResponse::builder(request_id, model) .created(created) .add_choice_content(index, "assistant", result.normal_text) // drops logprobs ...This is exactly the "Preserve logprobs for normal text returned by the tool parser" issue flagged on a prior commit (originally at lines 1265-1299, "also applies to 1435-1454"): tool-enabled requests with
logprobs: truestill silently lose logprobs whenever the model emits ordinary text through the tool-parsing path. The refactor in this PR moved the code but didn't fix the gap.🐛 Proposed fix (thread `choice_logprobs` through the tool branch)
- let tool_chunks = if context.is_specific_function { - Self::process_specific_function_stream( - &normal_text, - index, - has_tool_calls, - context.tool_choice, + let tool_chunks = if context.is_specific_function { + Self::process_specific_function_stream( + &normal_text, + index, + has_tool_calls, + context.tool_choice, context.request_id, context.model, context.created, context.system_fingerprint, context.history_tool_calls_count, + choice_logprobs.clone(), ) } else { self.process_tool_calls_stream( &normal_text, index, tool_parsers, has_tool_calls, tools, context.request_id, context.model, context.created, context.system_fingerprint, context.history_tool_calls_count, context.used_json_schema, + choice_logprobs, ) .await? };And in
tool_result_to_chat_chunks/process_specific_function_stream, swapadd_choice_content(...)foradd_choice_content_with_logprobs(..., choice_logprobs)on the normal-text emission (confirmChatLogProbsderivesClonefor the.clone()above).Also applies to: 1436-1486
🤖 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 `@model_gateway/src/routers/grpc/regular/streaming.rs` around lines 1228 - 1319, Preserve per-chunk choice_logprobs through the tool-processing path in process_chat_text_delta. Thread the value into process_tool_calls_stream and process_specific_function_stream, then into tool_result_to_chat_chunks, using add_choice_content_with_logprobs for emitted normal text; clone it where needed because both reasoning/tool handling and output emission may consume it. Keep logprobs absent only when no value was provided.
🤖 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 `@crates/tool_parser/src/parsers/glm4_moe.rs`:
- Around line 273-306: Update tool-name validation in drain_incremental to match
parse_tool_calls_from_text: skip the tool_indices.contains_key check when tools
is empty, while retaining InvalidToolName validation when tools are declared.
Prefer extracting and reusing a shared validation helper from both paths, and
add a regression test covering parse_incremental/finalize with an empty tools
list.
In `@model_gateway/src/routers/grpc/regular/processor.rs`:
- Around line 912-1013: Deduplicate the test fakes by moving FailingTokenizer,
FailingReasoningParser, and FailingToolParser with their trait implementations
from model_gateway/src/routers/grpc/regular/processor.rs:912-1013 into a shared
#[cfg(test)] test-support module, then import them in processor.rs. In
model_gateway/src/routers/grpc/regular/streaming.rs:2890-2996, remove the
duplicate definitions and import the same shared fakes; preserve their existing
behavior and test-only visibility.
---
Outside diff comments:
In `@model_gateway/src/routers/grpc/regular/processor.rs`:
- Around line 644-657: Update the Messages API tool-call handling around
parse_tool_calls to propagate parsing failures instead of logging and
continuing. In the Err branch, return the established
upstream_output_parse_failed error, matching the Chat Completions path; apply
the same behavior to the streaming Messages path so failed parsing cannot return
raw tool-call markup or a successful response.
In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 1321-1433: Update process_specific_function_stream and
process_tool_calls_stream to accept a shared &ChatTextProcessingContext instead
of individually passing request_id, model, created, system_fingerprint,
history_tool_calls_count, and tool_choice/use_json_parser; read the required
values from the context at each call site. Remove the
#[expect(clippy::too_many_arguments)] suppressions and preserve existing parsing
and chunk-generation behavior.
---
Duplicate comments:
In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 1228-1319: Preserve per-chunk choice_logprobs through the
tool-processing path in process_chat_text_delta. Thread the value into
process_tool_calls_stream and process_specific_function_stream, then into
tool_result_to_chat_chunks, using add_choice_content_with_logprobs for emitted
normal text; clone it where needed because both reasoning/tool handling and
output emission may consume it. Keep logprobs absent only when no value was
provided.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e532e29-c6cb-493f-aafc-22c3ce8fada4
📒 Files selected for processing (9)
crates/tool_parser/src/factory.rscrates/tool_parser/src/parsers/glm4_moe.rscrates/tool_parser/src/parsers/helpers.rscrates/tool_parser/src/traits.rscrates/tool_parser/tests/tool_parser_glm47_moe.rsgrpc_servicer/smg_grpc_servicer/sglang/servicer.pygrpc_servicer/tests/test_sglang_generate_request_contract.pymodel_gateway/src/routers/grpc/regular/processor.rsmodel_gateway/src/routers/grpc/regular/streaming.rs
| struct FailingReasoningParser; | ||
|
|
||
| struct FailingToolParser; | ||
|
|
||
| #[derive(Default)] | ||
| struct FailingTokenizer { | ||
| special_tokens: SpecialTokens, | ||
| } | ||
|
|
||
| impl Encoder for FailingTokenizer { | ||
| fn encode(&self, _input: &str, _add_special_tokens: bool) -> anyhow::Result<Encoding> { | ||
| Ok(Encoding::Plain(Vec::new())) | ||
| } | ||
|
|
||
| fn encode_batch( | ||
| &self, | ||
| inputs: &[&str], | ||
| _add_special_tokens: bool, | ||
| ) -> anyhow::Result<Vec<Encoding>> { | ||
| Ok(inputs.iter().map(|_| Encoding::Plain(Vec::new())).collect()) | ||
| } | ||
| } | ||
|
|
||
| impl Decoder for FailingTokenizer { | ||
| fn decode(&self, _token_ids: &[u32], _skip_special_tokens: bool) -> anyhow::Result<String> { | ||
| Err(anyhow::anyhow!("fake decode failure")) | ||
| } | ||
| } | ||
|
|
||
| impl Tokenizer for FailingTokenizer { | ||
| fn vocab_size(&self) -> usize { | ||
| 0 | ||
| } | ||
|
|
||
| fn get_special_tokens(&self) -> &SpecialTokens { | ||
| &self.special_tokens | ||
| } | ||
|
|
||
| fn token_to_id(&self, _token: &str) -> Option<u32> { | ||
| None | ||
| } | ||
|
|
||
| fn id_to_token(&self, _id: u32) -> Option<String> { | ||
| None | ||
| } | ||
|
|
||
| fn as_any(&self) -> &dyn std::any::Any { | ||
| self | ||
| } | ||
| } | ||
|
|
||
| impl ReasoningParser for FailingReasoningParser { | ||
| fn detect_and_parse_reasoning( | ||
| &mut self, | ||
| _text: &str, | ||
| ) -> Result<ReasoningParserResult, ReasoningParseError> { | ||
| Err(ReasoningParseError::ConfigError("fake failure".to_string())) | ||
| } | ||
|
|
||
| fn parse_reasoning_streaming_incremental( | ||
| &mut self, | ||
| _text: &str, | ||
| ) -> Result<ReasoningParserResult, ReasoningParseError> { | ||
| Err(ReasoningParseError::ConfigError("fake failure".to_string())) | ||
| } | ||
|
|
||
| fn reset(&mut self) {} | ||
|
|
||
| fn model_type(&self) -> &str { | ||
| "failing" | ||
| } | ||
|
|
||
| fn is_in_reasoning(&self) -> bool { | ||
| false | ||
| } | ||
|
|
||
| fn mark_reasoning_started(&mut self) {} | ||
|
|
||
| fn mark_think_start_stripped(&mut self) {} | ||
| } | ||
|
|
||
| #[async_trait] | ||
| impl ToolParser for FailingToolParser { | ||
| async fn parse_complete( | ||
| &self, | ||
| _output: &str, | ||
| ) -> ToolParserResult<(String, Vec<ParsedToolCall>)> { | ||
| Err(ToolParserError::ParsingFailed("fake failure".to_string())) | ||
| } | ||
|
|
||
| async fn parse_incremental( | ||
| &mut self, | ||
| _chunk: &str, | ||
| _tools: &[Tool], | ||
| ) -> ToolParserResult<StreamingParseResult> { | ||
| Err(ToolParserError::ParsingFailed("fake failure".to_string())) | ||
| } | ||
|
|
||
| fn has_tool_markers(&self, _text: &str) -> bool { | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated test fakes (FailingTokenizer, FailingReasoningParser, FailingToolParser) across both files. Both files independently define byte-for-byte equivalent fake Tokenizer/ReasoningParser/ToolParser implementations solely to test the new error-propagation paths — one root cause (no shared test-support module), duplicated maintenance burden going forward as the underlying traits evolve.
model_gateway/src/routers/grpc/regular/processor.rs#L912-L1013: moveFailingReasoningParser,FailingToolParser, andFailingTokenizer(plus their trait impls) into a shared#[cfg(test)]test-support module and import from there.model_gateway/src/routers/grpc/regular/streaming.rs#L2890-L2996: replace the duplicateFailingToolParser/FailingReasoningParser/FailingTokenizerdefinitions with an import of the same shared fakes used inprocessor.rs.
📍 Affects 2 files
model_gateway/src/routers/grpc/regular/processor.rs#L912-L1013(this comment)model_gateway/src/routers/grpc/regular/streaming.rs#L2890-L2996
🤖 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 `@model_gateway/src/routers/grpc/regular/processor.rs` around lines 912 - 1013,
Deduplicate the test fakes by moving FailingTokenizer, FailingReasoningParser,
and FailingToolParser with their trait implementations from
model_gateway/src/routers/grpc/regular/processor.rs:912-1013 into a shared
#[cfg(test)] test-support module, then import them in processor.rs. In
model_gateway/src/routers/grpc/regular/streaming.rs:2890-2996, remove the
duplicate definitions and import the same shared fakes; preserve their existing
behavior and test-only visibility.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 757577c81f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| utils::send_error_sse( | ||
| tx, | ||
| &format!("Tool call parsing failed: {e}"), | ||
| "upstream_output_parse_failed", | ||
| ); | ||
| return Ok(()); |
There was a problem hiding this comment.
When the Messages API stream hits this branch (for example, a GLM tool parser rejects a malformed or unknown tool call), it sends the chat/OpenAI data: {"error":...} frame and then returns Ok(()), so the caller's existing Err path that emits a MessageStreamEvent::Error never runs. Messages streaming clients expect event:-framed Anthropic SSE from this function, so they get a malformed terminal event instead of a usable error; return Err(...) here or emit MessageStreamEvent::Error with send_messages_event.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de2c372aa4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let has_partial_marker = Self::partial_structural_marker_suffix_len(text) > 1; | ||
| if has_partial_marker { | ||
| return Err(ParserError::Incomplete); |
There was a problem hiding this comment.
When a GLM request has tools enabled and the model returns ordinary non-tool text ending with a short prefix such as </ or <t, this treats it as Incomplete even though there is no <tool_call> in the output. The non-streaming chat/messages path now propagates parser errors as upstream_output_parse_failed, so harmless text can become a 502 instead of a normal response; the streaming EOF path already exempts these ≤2-byte ambiguous prefixes, and this complete path should apply the same exemption before returning Incomplete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0035e9b1d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let held_len = if let Some(start) = self.buffer.find(self.bot_token) { | ||
| self.buffer.len() - start | ||
| } else if self.current_tool_id == -1 { | ||
| Self::partial_marker_suffix_len(&self.buffer, self.bot_token) | ||
| } else { |
There was a problem hiding this comment.
When GLM Messages streaming has tools enabled and the final parser buffer contains a held marker prefix (for example the model's last text is hello <t, or an incomplete <tool_), this new buffering keeps the suffix in self.buffer and only releases or errors it from finalize(). The chat stream calls ToolParser::finalize, but process_messages_streaming_chunks only checks get_unstreamed_tool_args(), so the Messages stream silently drops the suffix or sends message_stop instead of an error; please finalize the Messages streaming parser before closing blocks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/tool_parser/src/parsers/glm4_moe.rs (1)
314-335: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winFix premature stream failure for valid multiple tool calls.
Inspecting the entire
self.bufferfor unexpected structural fragments is unsafe here. If a valid second<tool_call>token is split across network chunks,self.buffermay legitimately hold a partial prefix like"<tool_". Becausecontains_structural_fragmentmatches this prefix andcurrent_tool_id != -1, it will prematurely fail the stream withParsingFailed("unexpected GLM structure after tool call").Move the check after
emit_lenis calculated, and only inspect the portion of the buffer that is about to be discarded (&self.buffer[..emit_len]), safely exempting the legally held partial marker.🐛 Proposed fix
- if self.current_tool_id != -1 && Self::contains_structural_fragment(&self.buffer) { - return Err(ParserError::ParsingFailed( - "unexpected GLM structure after tool call".to_string(), - )); - } - // If bot_token is already in the buffer (but eot_token hasn't arrived), // we must hold everything from that index so the tool-call marker isn't // drained and emitted as normal text. let held_len = if let Some(start) = self.buffer.find(self.bot_token) { self.buffer.len() - start } else if self.current_tool_id == -1 { Self::partial_marker_suffix_len(&self.buffer, self.bot_token) } else { Self::partial_structural_marker_suffix_len(&self.buffer) }; let emit_len = self.buffer.len() - held_len; + + if self.current_tool_id != -1 && Self::contains_structural_fragment(&self.buffer[..emit_len]) { + return Err(ParserError::ParsingFailed( + "unexpected GLM structure after tool call".to_string(), + )); + } if self.current_tool_id == -1 { normal_text.push_str(&self.buffer[..emit_len]); } self.buffer.drain(..emit_len); break;🤖 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 `@crates/tool_parser/src/parsers/glm4_moe.rs` around lines 314 - 335, Move the structural-fragment validation in the parsing flow around current_tool_id and emit_len until after held_len and emit_len are computed. When current_tool_id != -1, inspect only the discarded prefix self.buffer[..emit_len] with contains_structural_fragment, leaving the held suffix—including partial <tool_call> markers—unchecked; preserve the existing ParsingFailed error and buffer-drain behavior.model_gateway/src/routers/grpc/regular/processor.rs (1)
596-608: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate reasoning parsing errors in the Messages API.
While chat and tool-parsing failures are now correctly propagated as upstream errors, the reasoning parsing step in the Messages API still catches the error, logs a warning, and continues with unparsed text. To maintain consistent error propagation across endpoints as outlined in the PR objectives, propagate these failures.
model_gateway/src/routers/grpc/regular/processor.rs#L596-L608: replace thewarn!withreturn Err(error::bad_gateway("upstream_output_parse_failed", format!("Reasoning parsing failed: {e}")));.model_gateway/src/routers/grpc/regular/streaming.rs#L1599-L1609: change theprocess_messages_reasoningreturn type toResult<(String, String, bool), String>, replace thewarn!withreturn Err(format!("Reasoning parsing failed: {e}"));, and append?to the.awaitat the callsite (around line 1920).🤖 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 `@model_gateway/src/routers/grpc/regular/processor.rs` around lines 596 - 608, Propagate reasoning parsing failures instead of logging and continuing: in model_gateway/src/routers/grpc/regular/processor.rs lines 596-608, return error::bad_gateway("upstream_output_parse_failed", ...) from the detect_and_parse_reasoning error branch; in model_gateway/src/routers/grpc/regular/streaming.rs lines 1599-1609, change process_messages_reasoning to return Result<(String, String, bool), String> and return the parsing error string, then propagate it with ? at its await callsite around line 1920.
🤖 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 `@crates/tool_parser/src/parsers/glm4_moe.rs`:
- Around line 338-351: The EOF handling around the buffer flush must forgive
short ambiguous suffixes consistently after tool calls: in
crates/tool_parser/src/parsers/glm4_moe.rs lines 338-351, remove the
current_tool_id prerequisite so eligible buffers of two bytes or fewer are
cleared without returning Incomplete, while preserving structural-fragment
checks; in has_post_call_structural_residue at lines 238-241, change the
suffix-length condition from greater than zero to greater than two so complete
parsing accepts the same harmless trailing characters.
In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 2119-2130: In the tool-call parsing error branch within
process_messages_streaming_chunks, remove the manual MessageStreamEvent::Error
construction and send_messages_event call, and return the parsing error directly
as Err. Let process_messages_streaming_response perform the existing error-event
formatting and preserve failure propagation so prefill_stream.mark_completed()
is skipped.
---
Outside diff comments:
In `@crates/tool_parser/src/parsers/glm4_moe.rs`:
- Around line 314-335: Move the structural-fragment validation in the parsing
flow around current_tool_id and emit_len until after held_len and emit_len are
computed. When current_tool_id != -1, inspect only the discarded prefix
self.buffer[..emit_len] with contains_structural_fragment, leaving the held
suffix—including partial <tool_call> markers—unchecked; preserve the existing
ParsingFailed error and buffer-drain behavior.
In `@model_gateway/src/routers/grpc/regular/processor.rs`:
- Around line 596-608: Propagate reasoning parsing failures instead of logging
and continuing: in model_gateway/src/routers/grpc/regular/processor.rs lines
596-608, return error::bad_gateway("upstream_output_parse_failed", ...) from the
detect_and_parse_reasoning error branch; in
model_gateway/src/routers/grpc/regular/streaming.rs lines 1599-1609, change
process_messages_reasoning to return Result<(String, String, bool), String> and
return the parsing error string, then propagate it with ? at its await callsite
around line 1920.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d8736f25-9984-40b7-9ce6-1f5881e56478
📒 Files selected for processing (5)
crates/tool_parser/src/parsers/glm4_moe.rsmodel_gateway/src/routers/grpc/regular/mod.rsmodel_gateway/src/routers/grpc/regular/processor.rsmodel_gateway/src/routers/grpc/regular/streaming.rsmodel_gateway/src/routers/grpc/regular/test_fakes.rs
|
Hi @TongLing916, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edcd5cf403
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| saw_call | ||
| && (Self::contains_structural_fragment(&text[previous_end..]) | ||
| || Self::partial_structural_marker_suffix_len(&text[previous_end..]) > 2) |
There was a problem hiding this comment.
This leftover predicate is inside drain_incremental, but it references saw_call, text, and previous_end, which are not in scope, and it also leaves the Ok(StreamingParseResult { ... }) outside the function body. As committed, tool_parser cannot compile, so GLM streaming parsing is unavailable until this block is replaced by the intended final return from drain_incremental.
Useful? React with 👍 / 👎.
| Err(e) => { | ||
| error!("Tool call parsing error in messages streaming: {}", e); | ||
| Err(e) => { | ||
| return Err(format!("Tool call parsing failed: {e}")); | ||
| } |
There was a problem hiding this comment.
This nested Err(e) => arm is invalid Rust syntax in the Messages streaming tool-parser branch, so model_gateway fails to compile even after the parser file is fixed. The branch should contain a single Err(e) match arm that returns the Anthropic stream error path.
Useful? React with 👍 / 👎.
|
Hi @TongLing916, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
github-actions
Bot
added
dependencies
github-actions
Bot
added
docker
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9957209ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue; | ||
| } | ||
|
|
||
| if self.current_tool_id != -1 && Self::contains_structural_fragment(&self.buffer) { |
There was a problem hiding this comment.
When a GLM stream emits multiple sequential tool calls and the next <tool_call> marker is split so that a chunk leaves the buffer as <tool_, <tool_c, etc. after the first call, current_tool_id != -1 and this condition treats the buffered prefix as unexpected residue before the rest of the marker can arrive. That makes valid streamed multi-tool responses fail depending only on chunk boundaries; hold partial <tool_call> prefixes here the same way the first call is held.
Useful? React with 👍 / 👎.
mergify
Bot
removed
the
needs-rebase
|
Hi @TongLing916, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
e995720 to
55eddc1
Compare
github-actions
Bot
removed
dependencies
mergify
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55eddc13cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !has_complete_marker { | ||
| return Ok((text.to_string(), vec![])); |
There was a problem hiding this comment.
When a tools-enabled non-streaming response consists of (or otherwise contains without an opening marker) </tool_call>, has_complete_marker is false, so this returns it as ordinary assistant text instead of surfacing the malformed structured output. This bypasses the new parse-error/502 path and leaks raw GLM markup to clients; the new unmatched-end-marker checks only run after an opening <tool_call> has been found.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@grpc_servicer/tests/test_sglang_generate_request_contract.py`:
- Around line 12-24: Update the AST-based assertions in the request-construction
tests at both call sites to verify that the keyword values for input_embeds and
token_type_ids are ast.Constant(value=None), not merely that the keywords exist.
Preserve the existing required-field checks for all other fields.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1301759b-d51d-4e1f-90a5-3d8c70de899e
📒 Files selected for processing (11)
crates/tool_parser/src/factory.rscrates/tool_parser/src/parsers/glm4_moe.rscrates/tool_parser/src/parsers/helpers.rscrates/tool_parser/src/traits.rscrates/tool_parser/tests/tool_parser_glm47_moe.rsgrpc_servicer/smg_grpc_servicer/sglang/servicer.pygrpc_servicer/tests/test_sglang_generate_request_contract.pymodel_gateway/src/routers/grpc/regular/mod.rsmodel_gateway/src/routers/grpc/regular/processor.rsmodel_gateway/src/routers/grpc/regular/streaming.rsmodel_gateway/src/routers/grpc/regular/test_fakes.rs
| CURRENT_REQUIRED_FIELDS = { | ||
| "rid", | ||
| "input_text", | ||
| "input_ids", | ||
| "input_embeds", | ||
| "mm_inputs", | ||
| "token_type_ids", | ||
| "sampling_params", | ||
| "return_logprob", | ||
| "logprob_start_len", | ||
| "top_logprobs_num", | ||
| "token_ids_logprob", | ||
| "stream", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the new fields are actually None.
These tests verify only keyword presence. They would still pass if either constructor supplied a non-null value, so inspect the AST expressions and assert input_embeds and token_type_ids are ast.Constant(value=None) at both call sites.
Also applies to: 44-53
🤖 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 `@grpc_servicer/tests/test_sglang_generate_request_contract.py` around lines 12
- 24, Update the AST-based assertions in the request-construction tests at both
call sites to verify that the keyword values for input_embeds and token_type_ids
are ast.Constant(value=None), not merely that the keywords exist. Preserve the
existing required-field checks for all other fields.
|
Hi @TongLing916, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
…pagation Harden GLM tool-call parsing so malformed structured output surfaces as a parse error (502) instead of leaking raw GLM markup to clients, and propagate gRPC parse errors through the streaming/non-streaming paths. tool_parser (glm4_moe): - Reject a standalone unmatched GLM closing marker (no opening marker) in the non-streaming path. Previously `has_complete_marker` was false so the raw markup was returned as ordinary assistant text, bypassing the parse-error path. Now surfaced as ParsingFailed, mirroring the streaming path's post-call structural-residue handling. - Reject incomplete/malformed markers, post-call structural residue, and incomplete second calls after a valid first call. - Schema-aware argument coercion keeps string-typed params as strings. grpc_servicer: - Add `input_embeds`/`token_type_ids` (literal None) and `mm_inputs` to the SGLang TokenizedGenerateReqInput constructor at both call sites. - Add source-level contract tests asserting the required fields are supplied and that `input_embeds`/`token_type_ids` are literal None (ast.Constant), not merely present as keywords. model_gateway (regular gRPC router): - Propagate tool/reasoning parse errors through streaming chat/generate/ completion paths instead of falling back to raw text; finish_openai_streaming_task emits the error SSE without a trailing [DONE]. - Resolve parser names once per request (ParserResolver) and thread them through process_chat_text_delta / process_single_choice; add a configured_only test resolver. - Add test fakes (FailingToolParser/FailingReasoningParser/FailingTokenizer) and tests covering parse-error propagation and CJK tool-call flushing. Rebased onto main; resolves merge conflicts in servicer.py and streaming.rs.
55eddc1 to
ce0af70
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
model_gateway/src/routers/grpc/regular/streaming.rs (1)
2394-2408: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win🔴 Important The Messages tool-parser failure branch returns
Ok(())and skips stream termination.This branch sends an
errorevent and then returnsOk(()). Three consequences follow.
process_messages_streaming_chunksreturns before Phase 4 and Phase 5, so nomessage_deltaand nomessage_stopare emitted. The Anthropic stream ends mid-protocol.- The early return skips
grpc_stream.mark_completed(), the reservation settlement, andMetrics::record_streaming_metrics. A failed request is not accounted for.- In PD mode,
process_prefill_decode_messages_streaming_chunkstreatsOkas success and callsprefill_stream.mark_completed().Return
Errinstead.process_messages_streaming_responsealready formats an identicalMessageStreamEvent::Errorfrom anErrresult.This repeats a previous review comment on the same range that was marked as addressed.
🐛 Proposed fix
Err(e) => { - let error_event = MessageStreamEvent::Error { - error: messages::ErrorResponse { - error_type: "api_error".to_string(), - message: format!("Tool call parsing failed: {e}"), - }, - }; - let _ = Self::send_messages_event( - tx, - &mut sse_buffer, - &error_event, - ) - .await; - return Ok(()); + return Err(format!("Tool call parsing failed: {e}")); }🤖 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 `@model_gateway/src/routers/grpc/regular/streaming.rs` around lines 2394 - 2408, Update the tool-call parsing failure branch in process_messages_streaming_chunks to return Err after producing the error event, rather than Ok(()). Preserve the existing error event emission while allowing the caller’s failure path to perform stream termination, cleanup, reservation settlement, and metrics accounting correctly.
🧹 Nitpick comments (9)
crates/tool_parser/src/factory.rs (1)
239-265: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value🟡 Nit: Prefix-only matching makes interior-wildcard patterns inert.
The new matcher honors only a trailing
*. It comparescandidate.starts_with(&pattern[..pattern.len()-1]). Patterns with an interior*, such asmoonshot*/Kimi-K2*,moonshot*/Kimi-K3*, andmoonshot*/Kimi_K3*, can never match, because the literal*remains inside the compared prefix.These ids still resolve through the basename candidate (
kimi-k2*,kimi-k3*), so behavior is preserved. The three mappings are now dead entries. Remove them, or document that only trailing wildcards are supported.🤖 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 `@crates/tool_parser/src/factory.rs` around lines 239 - 265, Update the matcher around the model-resolution logic to explicitly support only trailing-wildcard patterns, and remove the inert interior-wildcard mappings such as moonshot*/Kimi-K2*, moonshot*/Kimi-K3*, and moonshot*/Kimi_K3*. Keep the existing full-id and basename resolution behavior unchanged.crates/tool_parser/src/parsers/glm4_moe.rs (1)
562-746: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win🟡 Nit: Add a regression test for a short suffix after a valid call.
The new tests cover short prefixes only when no tool call precedes them (line 608) and cover long residue after a call (line 705). No test covers
"<tool_call>f</tool_call>trailing <", which is the case the threshold mismatch on line 240 rejects. Add that case to both the complete path and the streamingfinalizepath.As per coding guidelines: "Run the pr-test-analyzer agent to verify that tests adequately cover new or changed functionality."
🤖 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 `@crates/tool_parser/src/parsers/glm4_moe.rs` around lines 562 - 746, The regression coverage is missing short ambiguous suffixes after a valid tool call. Extend the relevant non-streaming test around test_non_streaming_accepts_short_ambiguous_prefixes to assert that "<tool_call>f</tool_call>trailing <" is accepted with the trailing text preserved and no calls, and add the same scenario to streaming coverage using parse_incremental followed by finalize, verifying finalize succeeds with the suffix intact.Source: Coding guidelines
crates/tool_parser/tests/tool_parser_glm47_moe.rs (1)
242-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value🟡 Nit: Assert the alias dialect, not only its existence.
The loop checks
create_parser(alias).is_some(). A swapped registration, for exampleglm45bound toGlm4MoeParser::glm47(), would still pass. Parse a GLM-4.5 newline-format input throughglm45and the GLM-4.7 whitespace-format input throughglm47to pin each alias to its dialect.🤖 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 `@crates/tool_parser/tests/tool_parser_glm47_moe.rs` around lines 242 - 244, Update the alias assertions in the test loop to validate dialect behavior, not just parser existence: parse a GLM-4.5 newline-format input through `glm45` and a GLM-4.7 whitespace-format input through `glm47`, asserting each produces the expected result. Keep the checks tied to the corresponding alias so swapped registrations fail.model_gateway/src/routers/grpc/regular/streaming.rs (3)
556-561: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win🟡 Nit The per-chunk emit path clones the SSE buffer on every parsed chunk.
process_chat_text_deltacan return several chunks per token batch. Each iteration callsBytes::from(sse_buffer.clone()), which allocates and copies once per chunk in the per-token hot path. The flush path directly below (Line 583-587) already usessse_encoder.encode_data, which producesByteswithout the intermediate clone.Use
sse_encoderhere as well so both emit paths share one mechanism and avoid the per-chunk copy.Based on learnings: "Avoid unnecessary clone() calls in gRPC streaming hot paths, especially during per-token response processing."
🤖 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 `@model_gateway/src/routers/grpc/regular/streaming.rs` around lines 556 - 561, Replace the per-chunk Bytes::from(sse_buffer.clone()) emission in the chunk-processing loop with the existing sse_encoder mechanism used by the flush path. Pass each formatted chunk through sse_encoder.encode_data so both emit paths avoid cloning the SSE buffer while preserving the existing tx.send error handling.Sources: Coding guidelines, Learnings
140-154: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win🟡 Nit Streaming reports upstream parse failures as
internal_error, while non-streaming reportsupstream_output_parse_failed.
finish_openai_streaming_tasksends every terminal failure with the error typeinternal_error. The non-streaming path maps the same fault class — reasoning-parser, tool-parser, and token-decode failures — toBAD_GATEWAYwith the codeupstream_output_parse_failed(processor.rsLine 952-976). A client that distinguishes upstream faults from gateway faults now sees two different classifications for one cause, depending only onstream.Pass the error class through to this finalizer, or use
upstream_output_parse_failedfor parser and decode failures.As per coding guidelines: "Ensure HTTP and gRPC routers implement the same API contract across both code paths."
🤖 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 `@model_gateway/src/routers/grpc/regular/streaming.rs` around lines 140 - 154, Update finish_openai_streaming_task and its callers to preserve the upstream output parse failure classification for reasoning-parser, tool-parser, and token-decode errors, emitting upstream_output_parse_failed instead of internal_error. Pass the error class through the streaming finalization path while retaining existing handling for unrelated internal failures, so streaming and non-streaming routers expose the same API contract.Source: Coding guidelines
3419-3445: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win🟡 Nit Three new tests loop over endpoint or path names but run an identical body each iteration. The loop variable appears only in error strings and assertion labels, so the tests suggest per-endpoint coverage that they do not provide.
model_gateway/src/routers/grpc/regular/streaming.rs#L3419-L3445: drop the["generate", "completion"]loop, or driveprocess_generate_streamingand the completion coordinator so both call sites are covered.model_gateway/src/routers/grpc/regular/streaming.rs#L3457-L3469: drop the["regular", "prefill-decode"]loop, or driveprocess_generate_streamingandprocess_generate_streaming_with_input_logprobs.model_gateway/src/routers/grpc/regular/processor.rs#L1134-L1164: drop the["generate", "completion"]loop, or driveprocess_non_streaming_generate_responseandprocess_non_streaming_completion_responsewith a failing tokenizer.As per coding guidelines: "Run the pr-test-analyzer agent to verify that tests adequately cover new or changed functionality."
🤖 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 `@model_gateway/src/routers/grpc/regular/streaming.rs` around lines 3419 - 3445, The new tests loop over labels while exercising identical code, so they do not provide endpoint-specific coverage. In model_gateway/src/routers/grpc/regular/streaming.rs lines 3419-3445, remove the generate/completion loop or invoke process_generate_streaming and the completion coordinator separately; in streaming.rs lines 3457-3469, remove the regular/prefill-decode loop or cover process_generate_streaming and process_generate_streaming_with_input_logprobs; in processor.rs lines 1134-1164, remove the generate/completion loop or exercise process_non_streaming_generate_response and process_non_streaming_completion_response with a failing tokenizer. Run the pr-test-analyzer agent to verify coverage.Source: Coding guidelines
model_gateway/src/routers/grpc/regular/processor.rs (2)
952-966: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win🟡 Nit Messages API token-decode failures do not use this shared mapping.
process_non_streaming_output_tokensmaps decode failures toBAD_GATEWAYwith codeupstream_output_parse_failed, and generate and completion now use it.process_non_streaming_messages_responsestill maps the samestop_decoder.process_tokensfailure toerror::internal_error("process_tokens_failed", ...), which returns 500 with a different error code. Clients that key on status or error code see two different contracts for one upstream fault.Route the Messages decode failure through this helper so all non-streaming endpoints report the same status and code.
As per coding guidelines: "Ensure HTTP and gRPC routers implement the same API contract across both code paths."
🤖 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 `@model_gateway/src/routers/grpc/regular/processor.rs` around lines 952 - 966, Update process_non_streaming_messages_response to route stop_decoder.process_tokens failures through process_non_streaming_output_tokens instead of error::internal_error, ensuring Messages uses the same BAD_GATEWAY status and upstream_output_parse_failed code as the other non-streaming endpoints.Source: Coding guidelines
683-702: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win🟣 Pre-existing Messages API still swallows reasoning-parser failures while tool-parser failures now return 502.
This change makes tool-call parsing failures explicit for the Messages API. The reasoning-parser branch in the same function (
process_non_streaming_messages_response) still logswarn!("Reasoning parsing error, skipping parsing: {e}")and continues with unparsed text. The chat path inprocess_single_choice(Line 131-137) now propagates the same failure class. The result is that one endpoint returns partially parsed output for a reasoning failure and a 502 for a tool failure.Align the Messages reasoning branch with this branch, or document why the two failure classes differ.
As per coding guidelines: "Run the silent-failure-hunter agent on changed files to detect swallowed errors, inappropriate fallbacks, and missing error propagation."
🤖 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 `@model_gateway/src/routers/grpc/regular/processor.rs` around lines 683 - 702, Update process_non_streaming_messages_response so reasoning-parser failures follow the same propagation behavior as the tool-call parsing branch, returning an appropriate bad_gateway error instead of logging a warning and continuing with unparsed text. Keep successful parsing unchanged and align the behavior with process_single_choice.Source: Coding guidelines
model_gateway/src/routers/grpc/regular/test_fakes.rs (1)
101-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win🟡 Nit The tool-parser finalization failure path is untested because
FailingToolParserinherits the defaultfinalize.ToolParser::finalizedefaults toOk(StreamingParseResult::default()), so no fake can drive the new"Tool parser finalization failed"branch.
model_gateway/src/routers/grpc/regular/test_fakes.rs#L101-L121: add afinalizeoverride onFailingToolParserthat returnsToolParserError::ParsingFailed.model_gateway/src/routers/grpc/regular/streaming.rs#L619-L623: add a test that uses that fake and asserts the stream ends with an error event and without[DONE].As per coding guidelines: "Run the pr-test-analyzer agent to verify that tests adequately cover new or changed functionality."
🤖 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 `@model_gateway/src/routers/grpc/regular/test_fakes.rs` around lines 101 - 121, The finalization failure path is untested because FailingToolParser uses ToolParser’s successful default finalize implementation. In model_gateway/src/routers/grpc/regular/test_fakes.rs#L101-L121, override FailingToolParser::finalize to return ToolParserError::ParsingFailed; in model_gateway/src/routers/grpc/regular/streaming.rs#L619-L623, add a test using this fake that verifies an error event is emitted and [DONE] is not sent. Run the pr-test-analyzer agent to verify coverage.Source: Coding guidelines
🤖 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 `@crates/tool_parser/src/parsers/glm4_moe.rs`:
- Around line 238-241: In crates/tool_parser/src/parsers/glm4_moe.rs lines
238-241, update the complete-path residue check in the visible parsing function
to require a structural suffix length greater than 2, matching the rule used at
line 367. In lines 338-351, remove the current_tool_id == -1 prerequisite from
the EOF exemption while continuing to suppress released text after a tool call
has run.
In `@crates/tool_parser/src/traits.rs`:
- Around line 39-47: Update process_messages_streaming_chunks to call
ToolParser::finalize after all streaming chunks have been processed, passing the
available tools and handling its ParserResult consistently with
parse_incremental. Propagate the finalized StreamingParseResult so buffered
complete calls are emitted and incomplete markers are rejected.
---
Duplicate comments:
In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 2394-2408: Update the tool-call parsing failure branch in
process_messages_streaming_chunks to return Err after producing the error event,
rather than Ok(()). Preserve the existing error event emission while allowing
the caller’s failure path to perform stream termination, cleanup, reservation
settlement, and metrics accounting correctly.
---
Nitpick comments:
In `@crates/tool_parser/src/factory.rs`:
- Around line 239-265: Update the matcher around the model-resolution logic to
explicitly support only trailing-wildcard patterns, and remove the inert
interior-wildcard mappings such as moonshot*/Kimi-K2*, moonshot*/Kimi-K3*, and
moonshot*/Kimi_K3*. Keep the existing full-id and basename resolution behavior
unchanged.
In `@crates/tool_parser/src/parsers/glm4_moe.rs`:
- Around line 562-746: The regression coverage is missing short ambiguous
suffixes after a valid tool call. Extend the relevant non-streaming test around
test_non_streaming_accepts_short_ambiguous_prefixes to assert that
"<tool_call>f</tool_call>trailing <" is accepted with the trailing text
preserved and no calls, and add the same scenario to streaming coverage using
parse_incremental followed by finalize, verifying finalize succeeds with the
suffix intact.
In `@crates/tool_parser/tests/tool_parser_glm47_moe.rs`:
- Around line 242-244: Update the alias assertions in the test loop to validate
dialect behavior, not just parser existence: parse a GLM-4.5 newline-format
input through `glm45` and a GLM-4.7 whitespace-format input through `glm47`,
asserting each produces the expected result. Keep the checks tied to the
corresponding alias so swapped registrations fail.
In `@model_gateway/src/routers/grpc/regular/processor.rs`:
- Around line 952-966: Update process_non_streaming_messages_response to route
stop_decoder.process_tokens failures through process_non_streaming_output_tokens
instead of error::internal_error, ensuring Messages uses the same BAD_GATEWAY
status and upstream_output_parse_failed code as the other non-streaming
endpoints.
- Around line 683-702: Update process_non_streaming_messages_response so
reasoning-parser failures follow the same propagation behavior as the tool-call
parsing branch, returning an appropriate bad_gateway error instead of logging a
warning and continuing with unparsed text. Keep successful parsing unchanged and
align the behavior with process_single_choice.
In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 556-561: Replace the per-chunk Bytes::from(sse_buffer.clone())
emission in the chunk-processing loop with the existing sse_encoder mechanism
used by the flush path. Pass each formatted chunk through
sse_encoder.encode_data so both emit paths avoid cloning the SSE buffer while
preserving the existing tx.send error handling.
- Around line 140-154: Update finish_openai_streaming_task and its callers to
preserve the upstream output parse failure classification for reasoning-parser,
tool-parser, and token-decode errors, emitting upstream_output_parse_failed
instead of internal_error. Pass the error class through the streaming
finalization path while retaining existing handling for unrelated internal
failures, so streaming and non-streaming routers expose the same API contract.
- Around line 3419-3445: The new tests loop over labels while exercising
identical code, so they do not provide endpoint-specific coverage. In
model_gateway/src/routers/grpc/regular/streaming.rs lines 3419-3445, remove the
generate/completion loop or invoke process_generate_streaming and the completion
coordinator separately; in streaming.rs lines 3457-3469, remove the
regular/prefill-decode loop or cover process_generate_streaming and
process_generate_streaming_with_input_logprobs; in processor.rs lines 1134-1164,
remove the generate/completion loop or exercise
process_non_streaming_generate_response and
process_non_streaming_completion_response with a failing tokenizer. Run the
pr-test-analyzer agent to verify coverage.
In `@model_gateway/src/routers/grpc/regular/test_fakes.rs`:
- Around line 101-121: The finalization failure path is untested because
FailingToolParser uses ToolParser’s successful default finalize implementation.
In model_gateway/src/routers/grpc/regular/test_fakes.rs#L101-L121, override
FailingToolParser::finalize to return ToolParserError::ParsingFailed; in
model_gateway/src/routers/grpc/regular/streaming.rs#L619-L623, add a test using
this fake that verifies an error event is emitted and [DONE] is not sent. Run
the pr-test-analyzer agent to verify coverage.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a331bb6-e6d4-4bcb-9be5-eb7282c144f2
📒 Files selected for processing (11)
crates/tool_parser/src/factory.rscrates/tool_parser/src/parsers/glm4_moe.rscrates/tool_parser/src/parsers/helpers.rscrates/tool_parser/src/traits.rscrates/tool_parser/tests/tool_parser_glm47_moe.rsgrpc_servicer/tests/test_sglang_generate_request_contract.pymodel_gateway/src/routers/grpc/regular/mod.rsmodel_gateway/src/routers/grpc/regular/processor.rsmodel_gateway/src/routers/grpc/regular/streaming.rsmodel_gateway/src/routers/grpc/regular/test_fakes.rsmodel_gateway/src/routers/grpc/utils/parsers.rs
| saw_call | ||
| && (Self::contains_structural_fragment(&text[previous_end..]) | ||
| || Self::partial_structural_marker_suffix_len(&text[previous_end..]) > 0) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔴 Important: Short ambiguous structural suffixes are forgiven before a tool call but rejected after one. parse_complete_inner documents that suffixes of 2 bytes or fewer, such as < and </, are too common in ordinary text to treat as incomplete. Two sites do not apply that rule once current_tool_id advances, so a valid completion becomes ParsingFailed or Incomplete, and the gateway returns HTTP 502 or a terminal stream error.
crates/tool_parser/src/parsers/glm4_moe.rs#L238-L241: change the residue threshold frompartial_structural_marker_suffix_len(...) > 0to> 2so the complete path uses the same rule as line 367.crates/tool_parser/src/parsers/glm4_moe.rs#L338-L351: remove theself.current_tool_id == -1prerequisite from the EOF exemption, and keep suppressing the released text when a tool call already ran.
📍 Affects 1 file
crates/tool_parser/src/parsers/glm4_moe.rs#L238-L241(this comment)crates/tool_parser/src/parsers/glm4_moe.rs#L338-L351
🤖 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 `@crates/tool_parser/src/parsers/glm4_moe.rs` around lines 238 - 241, In
crates/tool_parser/src/parsers/glm4_moe.rs lines 238-241, update the
complete-path residue check in the visible parsing function to require a
structural suffix length greater than 2, matching the rule used at line 367. In
lines 338-351, remove the current_tool_id == -1 prerequisite from the EOF
exemption while continuing to suppress released text after a tool call has run.
| /// Finish a streaming parse at end-of-input. | ||
| /// | ||
| /// Stateful parsers can override this to drain complete buffered calls or | ||
| /// reject an incomplete structured marker. The default is intentionally a | ||
| /// no-op so existing parser implementations retain their current behavior. | ||
| async fn finalize(&mut self, _tools: &[Tool]) -> ParserResult<StreamingParseResult> { | ||
| Ok(StreamingParseResult::default()) | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate finalize call sites on the streaming tool-parser paths.
set -euo pipefail
rg -nP --type=rust -C6 '\.finalize\s*\(' model_gateway/src crates/tool_parser/src
rg -nP --type=rust -n 'fn (process_[a-z_]*streaming_chunks|process_tool_calls_stream)' model_gateway/srcRepository: smg-project/smg
Length of output: 19977
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- regular gRPC tool-call stream ---'
sed -n '280,700p' model_gateway/src/routers/grpc/regular/streaming.rs
sed -n '1580,1715p' model_gateway/src/routers/grpc/regular/streaming.rs
printf '%s\n' '--- all parser incremental and finalize call sites ---'
rg -nP --type=rust -C5 '(parse_incremental|\.finalize)\s*\(' model_gateway/src crates/tool_parser/src
printf '%s\n' '--- HTTP/router parser references ---'
rg -nP --type=rust -C4 '(ToolParser|tool_parser|parse_incremental|process_tool_calls_stream)' model_gateway/src/routers model_gateway/src | head -n 1200Repository: smg-project/smg
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- files that call parse_incremental ---'
rg -l --type=rust 'parse_incremental\s*\(' model_gateway/src crates/tool_parser/src | sort
printf '%s\n' '--- parser finalization calls, excluding unrelated finalize methods ---'
rg -n --type=rust -C4 'parser(_guard)?\s*\.finalize\s*\(|\.finalize\(tools|finalize\(tools\.as_deref' model_gateway/src crates/tool_parser/src
printf '%s\n' '--- streaming entry points and router files ---'
rg -n --type=rust 'process_(messages|completion|streaming)_streaming_chunks|process_tool_calls_stream|parse_incremental' model_gateway/src/routers --glob '!**/tests/**' | grep -E 'model_gateway/src/routers/.*:[0-9]+:' | head -n 500
printf '%s\n' '--- trait and GLM4-MoE finalize implementation ---'
sed -n '1,75p' crates/tool_parser/src/traits.rs
rg -n -C12 --type=rust 'async fn finalize' crates/tool_parser/src/parsers/glm4_moe.rs crates/tool_parser/src/parsersRepository: smg-project/smg
Length of output: 12731
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- messages streaming parser lifecycle ---'
sed -n '1953,2345p' model_gateway/src/routers/grpc/regular/streaming.rs
printf '%s\n' '--- completion streaming parser lifecycle ---'
sed -n '2938,3255p' model_gateway/src/routers/grpc/regular/streaming.rs
printf '%s\n' '--- all finalization-related identifiers in regular streaming ---'
rg -n -C8 --type=rust 'tool_parsers|parse_incremental|finalize|finish_reason' \
model_gateway/src/routers/grpc/regular/streaming.rs | tail -n 1000Repository: smg-project/smg
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- messages end-of-stream handling ---'
sed -n '2280,2665p' model_gateway/src/routers/grpc/regular/streaming.rs
printf '%s\n' '--- GLM4-MoE buffering and finalize behavior ---'
sed -n '90,260p' crates/tool_parser/src/parsers/glm4_moe.rs
sed -n '430,495p' crates/tool_parser/src/parsers/glm4_moe.rs
printf '%s\n' '--- static lifecycle check for each regular streaming function ---'
python3 - <<'PY'
from pathlib import Path
import re
path = Path("model_gateway/src/routers/grpc/regular/streaming.rs")
text = path.read_text()
functions = [
"process_streaming_chunks_inner",
"process_messages_streaming_chunks",
"process_completion_streaming_chunks",
]
for name in functions:
m = re.search(rf"(?m)^\s*(?:pub\s+)?async\s+fn\s+{name}\b", text)
if not m:
print(f"{name}: NOT FOUND")
continue
start = m.start()
next_fn = re.search(r"(?m)^\s*(?:pub\s+)?(?:async\s+)?fn\s+\w+\b", text[m.end():])
end = m.end() + next_fn.start() if next_fn else len(text)
body = text[start:end]
print(
f"{name}: parse_incremental={len(re.findall(r'parse_incremental\\s*\\(', body))}, "
f"tool_parser_finalize={len(re.findall(r'\\.finalize\\s*\\(.*tools', body, re.S))}"
)
PYRepository: smg-project/smg
Length of output: 26757
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- GLM4-MoE drain behavior ---'
rg -n -C20 'fn drain_incremental' crates/tool_parser/src/parsers/glm4_moe.rs
printf '%s\n' '--- corrected static lifecycle check ---'
python3 - <<'PY'
from pathlib import Path
import re
text = Path("model_gateway/src/routers/grpc/regular/streaming.rs").read_text()
parse_pat = re.compile(r"parse_incremental\s*\(")
finalize_pat = re.compile(r"\.finalize\s*\(")
for name in (
"process_streaming_chunks_inner",
"process_messages_streaming_chunks",
"process_completion_streaming_chunks",
):
start_match = re.search(
r"(?m)^\s*(?:pub\s+)?async\s+fn\s+" + re.escape(name) + r"\b",
text,
)
if not start_match:
print(name + ": NOT FOUND")
continue
next_match = re.search(
r"(?m)^\s*(?:pub\s+)?(?:async\s+)?fn\s+\w+\b",
text[start_match.end():],
)
end = start_match.end() + next_match.start() if next_match else len(text)
body = text[start_match.start():end]
print(
name
+ ": parse_incremental="
+ str(len(parse_pat.findall(body)))
+ ", finalize="
+ str(len(finalize_pat.findall(body)))
)
PYRepository: smg-project/smg
Length of output: 1999
Finalize the Messages streaming tool parser at end-of-stream.
process_messages_streaming_chunks calls parse_incremental but never calls ToolParser::finalize. Without it, final GLM tool calls can be dropped and incomplete markers can be accepted.
🤖 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 `@crates/tool_parser/src/traits.rs` around lines 39 - 47, Update
process_messages_streaming_chunks to call ToolParser::finalize after all
streaming chunks have been processed, passing the available tools and handling
its ParserResult consistently with parse_incremental. Propagate the finalized
StreamingParseResult so buffered complete calls are emitted and incomplete
markers are rejected.
Source: Coding guidelines
1 participant
Summary
This fixes silent output corruption in the SGLang gRPC path and makes response parsing ownership explicit at the Gateway boundary.
TokenizedGenerateReqInputschemaThe historical GLM CJK first-character detokenization bug is already addressed by the current
llm-tokenizerdependency. This PR targets the remaining parser/flush/error paths that could look like missing output or malformed tool calls.Changes
SGLang request contract
input_embeds=Noneandtoken_type_ids=Nonein normal request conversion and the health probeGLM tool parser
<tool_call>and closing markers across every chunk splitfinalize()so buffered EOF data cannot be silently dropped"00123"while coercing bool/number valuesglm45/glm47aliases and GLM-5.x organization/local-path model namesGateway output processing
decode -> reasoning -> tool -> contentpipelineupstream_output_parse_failed[DONE]after a failureVerification
cargo +1.93.0 test -p tool-parsercargo +1.93.0 test -p smg --lib routers::grpc::regular::processor::testscargo +1.93.0 test -p smg --lib routers::grpc::regular::streaming::testscargo +1.93.0 check -p smg --libcargo +1.93.0 clippy -p smg --lib -- -D warningscargo fmt --all -- --checkpython3 -m unittest grpc_servicer/tests/test_sglang_generate_request_contract.py -vFollow-ups / not tested