Repository navigation
Conversation
Non-greedy </arg_value> matching truncated values that mention the close tag. Accept a candidate close only when what follows is end-of-args or a real <arg_key>/<arg_value> pair. Signed-off-by: santhreal <64453045+santhreal@users.noreply.github.com>
github-actions
Bot
added
tests
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesGLM argument parsing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 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 |
![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 replaces the regex-based argument extraction in Glm4MoeParser with a manual string scanning parser to handle edge cases where argument values contain literal <arg_key> or </arg_value> tags, and adds corresponding unit tests. The reviewer suggested a code improvement in find_arg_value_close to make the parser more robust against trailing garbage by treating the current tag as closing if no subsequent closing tags exist.
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.
| while let Some(rel) = value_and_rest[from..].find(needle) { | ||
| let abs = from + rel; | ||
| let after = &value_and_rest[abs + needle.len()..]; | ||
| if after.trim_start().is_empty() || Self::follows_with_real_arg_key(after) { |
There was a problem hiding this comment.
To make the parser more robust against trailing garbage or extra characters at the end of the tool call block, we can also accept the current </arg_value> if there are no subsequent </arg_value> tags in the remaining text. This ensures that the last </arg_value> is always treated as the closing tag even if there is trailing non-whitespace text.
| if after.trim_start().is_empty() || Self::follows_with_real_arg_key(after) { | |
| if after.trim_start().is_empty() || Self::follows_with_real_arg_key(after) || !after.contains(needle) { |
There was a problem hiding this comment.
Thanks. Accepted the final </arg_value> when no later close remains (!after.contains(needle)) in 37eac62.
![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: 273bd7c9ab
ℹ️ 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 Some(close_rel) = Self::find_arg_value_close(rest) else { | ||
| break; | ||
| }; |
There was a problem hiding this comment.
When a model closes a valid argument but then emits an incomplete next field before </tool_call> (for example, <arg_key>a</arg_key><arg_value>1</arg_value><arg_key>b</arg_key>), find_arg_value_close rejects a's real close because the follower is neither end-of-args nor a complete key/value pair, then returns None and this break drops a as well. The previous regex retained all complete preceding pairs, so this reduces recovery from malformed model output; accept the current close when the following key is incomplete, or otherwise preserve already-complete arguments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks. The same change covers this: an incomplete following <arg_key> no longer blocks the prior complete close (37eac62).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@crates/tool_parser/src/parsers/glm4_moe.rs`:
- Around line 122-134: Update find_arg_value_close to retain the index of the
first matched </arg_value> while scanning for a strict closing-tag match. Return
the strict match when after is empty or follows_with_real_arg_key(after)
succeeds; otherwise, after the loop, return the stored first-match index so
trailing garbage does not drop the final argument.
🪄 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: f30e91b5-528f-40a7-930b-115969e02ac8
📒 Files selected for processing (3)
crates/tool_parser/src/parsers/glm4_moe.rscrates/tool_parser/tests/tool_parser_glm47_moe.rscrates/tool_parser/tests/tool_parser_glm4_moe.rs
… remains Keeps literal close tags inside values, while recovering when trailing garbage or an incomplete next key follows the real close. Signed-off-by: santhreal <64453045+santhreal@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37eac62f6d
ℹ️ 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 after.trim_start().is_empty() | ||
| || Self::follows_with_real_arg_key(after) | ||
| || !after.contains(needle) |
There was a problem hiding this comment.
When a string value contains an unescaped complete argument example—for example, documentation text containing </arg_value><arg_key>sample</arg_key><arg_value>x</arg_value> before the value's actual closing tag—follows_with_real_arg_key accepts the first literal close as the boundary. This truncates the enclosing value and parses sample as a spurious tool argument, despite this change intending to preserve literal </arg_value> text in values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this as-is. A value that embeds a full </arg_value><arg_key>…</arg_key><arg_value>… example is indistinguishable from a real next field without a richer grammar, and fixing it would reopen the literal-close case this PR targets. Happy to follow a maintainer preference if you want a different tradeoff.
1 participant
Description
Problem
GLM-4 MoE argument parsing used a non-greedy
</arg_value>match. A value that mentions the close tag (for exampleuse </arg_value> in docs) was truncated at the first occurrence, so later keys were lost or values were wrong.Solution
Accept a candidate
</arg_value>only when what follows is end-of-args or a real<arg_key>…</arg_key><arg_value>pair. Applies to both GLM-4.5 and GLM-4.7 extractors.Changes
crates/tool_parser/src/parsers/glm4_moe.rs: scan-based close detection instead of non-greedy regex capturetool_parser_glm4_moe.rsandtool_parser_glm47_moe.rsTest Plan
Before:
<arg_value>use </arg_value> in docs</arg_value>stores onlyuse.After: stores
use </arg_value> in docs, and a following real key still parses.cargo +nightly fmt --all -- --check cargo clippy -p tool-parser --all-targets -- -D warnings cargo test -p tool-parser --test tool_parser_glm4_moe --test tool_parser_glm47_moeChecklist
cargo +nightly fmtpassescargo clippy --all-targetsontool-parserwith-D warningspassesSummary by CodeRabbit
Bug Fixes
Tests