Skip to content

Conversation

@soaringk
Copy link
Contributor

@soaringk soaringk commented Oct 31, 2025

Motivation

This PR fixes a bug where the tool call parser fails when the model's output contains literal escaped characters, such as \n, \".

The Problem:
When GLM-4 outputs tool calls, the XML may contain '\n' literal characters and the JSON content within <arg_value> tags may contain escaped characters like \", causing regex matching and json parsing both failed. For example:

<tool_call>todo_write\n<arg_key>todos</arg_key>\n<arg_value>[{\"id\": \"1\", \"task\": \"检查后端代码中的硬编码问题\", \"status\": \"in_progress\"}, {\"id\": \"2\", \"task\": \"检查前端代码中的硬编码问题\", \"status\": \"pending\"}, {\"id\": \"3\", \"task\": \"检查违反单一职责的代码\", \"status\": \"pending\"}, {\"id\": \"4\", \"task\": \"创建整改建议报告\", \"status\": \"pending\"}]</arg_value>
</tool_call>

The current regex fails to match the literal \n. Besides, even if the regex extracts the content within <arg_value>, it is not valid JSON. Attempting json.loads() on this string fails with JSONDecodeError.

When parsing failed, the argument value was treated as a plain string. This led to incorrect behavior in BaseFormatDetector to incorrectly perform another layer of JSON serialization on the arguments. The result is a double-escaped string (e.g., \\" instead of \"), breaking the tool call functionality.

Example of problematic tool output:

[
  {
    "function": {
      "name": "todo_write",
      "arguments": "{\"todos\": \"[{\\\"id\\\": \\\"1\\\", \\\"task\\\": \\\"检查后端代码的硬编码问题\\\", \\\"status\\\": \\\"in_progress\\\"}, {\\\"id\\\": \\\"2\\\", \\\"task\\\": \\\"检查前端代码的硬编码问题\\\", \\\"status\\\": \\\"pending\\\"}, {\\\"id\\\": \\\"3\\\", \\\"task\\\": \\\"检查违反单一职责原则的代码\\\", \\\"status\\\": \\\"pending\\\"}, {\\\"id\\\": \\\"4\\\", \\\"4\\\", \\\"task\\\": \\\"制定整改方案\\\", \\\"status\\\": \\\"pending\\\"}, {\\\"id\\\": \\\"5\\\", \\\"5\\\", \\\"task\\\": \\\"实施整改\\\", \\\"status\\\": \\\"pending\\\"}]\"}"
    },
    "index": 0,
    "id": "call_eb99d2a2c7fd44328776fb9c",
    "type": "function"
  }
]

more test cases can be found at test_array_argument_with_escaped_json()

Modifications

  1. Robust Regex for Newlines: Updated regular expression used to extract the function name and arguments (func_name, func_args). It now correctly recognizes both actual newlines (\n) and their literal form (\\n). This ensures the initial parsing succeeds even with escaped newlines.

  2. Robust JSON Deserialization: Updated parse_arguments() in glm4_moe_detector.py to handle escaped JSON strings using a two-step JSON parsing approach:

    1. First attempt: Try parsing directly with json.loads() (handles normal case where no extra escaping is present)
    2. Second attempt (if first fails): Use two-step deserialization:
      • Wrap the input string as a JSON string value: '{"tmp": "' + json_value + '"}'
      • Parse this wrapper with json.loads() - this causes the JSON parser to unescape only JSON-specific escape sequences (", \,
        \n, \t, etc.)
      • Extract the unescaped value from wrapped["tmp"]
      • Parse the unescaped value again with json.loads()
  3. Final fallback: Use ast.literal_eval() for Python literals

Why twp-step deserialization:

  • Safer: Compared to decode('unicode_escape'), only unescapes JSON-standard escape sequences, not Python-specific ones (like \x, \0).
  1. Compiled the regexes during initialization to improve performance at runtime.

These changes ensure that tool calls with escaped characters are parsed correctly on the first attempt, avoiding the double-serialization bug.

Accuracy Tests

Benchmarking and Profiling

Checklist

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @soaringk, 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 resolves a critical bug in the tool call parser that previously caused failures when processing model outputs containing literal escaped characters. The fix prevents double JSON serialization of arguments by improving the robustness of both regular expression-based parsing for newlines and the subsequent JSON deserialization process, ensuring accurate and reliable tool call execution.

Highlights

  • Tool Call Parser Fix: Addresses a bug where the tool call parser failed to correctly handle literal escaped characters (e.g., "\n", """) in model outputs.
  • Double Serialization Prevention: Resolves an issue where incorrect parsing led to arguments being double-serialized, breaking tool call functionality.
  • Enhanced Regex for Newlines: Updates the regular expression for func_detail_regex and func_arg_regex to robustly recognize both actual and escaped newlines.
  • Improved JSON Deserialization: Introduces an additional step in parse_arguments to replace escaped quotes (""") with standard quotes (""") before JSON loading, making deserialization more resilient.
  • New Unit Test: Adds test_array_argument_with_escaped_json to verify the correct handling of complex JSON arguments with escaped characters.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@soaringk soaringk changed the title [fix] Handle escaped characters in tool call parser to prevent double serialization [fix] Handle escaped characters in GLM tool call parser to prevent double serialization Oct 31, 2025
@soaringk soaringk marked this pull request as draft October 31, 2025 09:58
@soaringk soaringk closed this Oct 31, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to fix a bug in the tool call parser for GLM-4 models, where escaped characters in the model's output caused parsing failures and double serialization. The changes include updating regexes to handle both literal and escaped newlines, and making the JSON deserialization more robust. The approach is sound, and new tests are added to cover the fix. However, I've found a critical issue in the implementation of the JSON parsing logic that needs to be addressed.

@soaringk soaringk reopened this Oct 31, 2025
@soaringk soaringk marked this pull request as ready for review October 31, 2025 10:04
@b8zhong b8zhong added the run-ci label Nov 1, 2025
@JustinTong0323 JustinTong0323 added the express-lane A PR may be merged without a full CI check label Nov 4, 2025
@Kangyan-Zhou Kangyan-Zhou merged commit 44da737 into sgl-project:main Nov 5, 2025
213 of 268 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

express-lane A PR may be merged without a full CI check run-ci

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants