fix(llm): filter XML tool-call recovery by context - #1641
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and security of the LLM tool-call recovery mechanism. It introduces logic to prevent the system from inadvertently parsing XML-formatted tool calls that appear within markdown code blocks, inline code, or blockquotes. This change ensures that only legitimate, standalone tool-call payloads are processed, mitigating potential security risks associated with misinterpreting user-provided content as executable commands. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request enhances the recover_tool_calls_from_content function by introducing new helper functions (overlaps_code_region, line_bounds, is_recoverable_tool_call_segment) to prevent the parsing of XML-style tool calls that appear within markdown code blocks, inline code, or blockquotes. This ensures that code examples or quoted snippets are not mistakenly interpreted as executable tool calls. New test cases have been added to validate these scenarios. Feedback indicates that the logic within is_recoverable_tool_call_segment for checking blockquotes and surrounding text is confusing and conflates concerns, suggesting a refactoring for improved clarity and robustness.
zmanian
left a comment
There was a problem hiding this comment.
Review: fix(llm): filter XML tool-call recovery by context
Well-scoped security hardening. The core idea -- preventing XML tool-call recovery inside code blocks, inline code, and blockquotes -- is correct and the implementation is clean.
Low
-
Blockquote check only examines first line: Multi-line tool calls could have the opening tag outside a blockquote. Acceptable in practice since the "isolated on own line" check already constrains the format tightly.
-
Bracket-format recovery not filtered: The
[Called tool ...]recovery at lines 1359-1402 doesn't go throughis_recoverable_tool_call_segment. This seems intentional since bracket format comes fromflatten_tool_messages(internal), not model output. Confirm this is deliberate.
Suggestions (non-blocking)
- Add a test for inline-with-prose rejection (e.g.,
"Here is <tool_call>tool_list</tool_call>") - Consider unit tests for
line_boundsdirectly -- it does byte-level string indexing where off-by-one errors hide
Good refactoring of the search loop from remaining slice to search_from offset. Test coverage hits the key cases. Approve.
* fix(llm): filter XML tool-call recovery by context * fix: address review comments on PR nearai#1641
* fix(llm): filter XML tool-call recovery by context * fix: address review comments on PR nearai#1641
Summary
recover_tool_calls_from_contentfrom converting XML tool-call examples inside fenced code blocks, inline code, or markdown blockquotes into real tool callsChange Type
Linked Issue
Validation
cargo fmtcargo clippy --all --benches --tests --examples --all-featurescargo test test_recover_ --lib,cargo test llm::reasoning --libSecurity Impact
src/llm/reasoning.rs.Database Impact
Blast Radius
src/llm/reasoning.rsXML tool-call recovery and its unit tests.Rollback Plan
8724178to restore the previous recovery behavior.Review track: C (security/runtime/DB/CI)