Skip to content

Conversation

@phawrylak
Copy link
Contributor

Fix tool call argument serialization issue

Problem Description

Tool call arguments were being incorrectly serialized when passed as JSON strings, causing them to be wrapped in an unnecessary query field instead of being parsed and used directly.

Before (Broken):

{
  "query": "{\"operation\":\"write\",\"todoList\":[{\"id\":1,\"title\":\"test\"}]}"
}

After (Fixed):

{
  "operation": "write",
  "todoList": [
    {
      "id": 1,
      "title": "test"
    }
  ]
}

Root Cause

The issue was in the sse_translate_chat function in utils.py at two locations:

  1. Lines ~324-328: Web search call processing
  2. Lines ~405-409: Function call output processing

The original logic treated all string arguments the same way:

elif isinstance(eff_args, str):
    args = json.dumps({"query": eff_args})  # Always wrapped in query

This caused valid JSON strings to be double-wrapped instead of being parsed and used directly.

Solution

Enhanced the argument serialization logic to:

  1. Parse JSON strings first - Check if string arguments are valid JSON
  2. Use parsed objects directly - If parsing succeeds and results in dict/list, serialize directly
  3. Preserve query wrapping for plain strings - Only wrap non-JSON strings in query field

New Logic:

elif isinstance(eff_args, str):
    # Try to parse as JSON first
    try:
        parsed = json.loads(eff_args)
        if isinstance(parsed, (dict, list)):
            args = json.dumps(parsed)  # Use parsed object directly
        else:
            args = json.dumps({"query": eff_args})  # Primitive value, wrap in query
    except (json.JSONDecodeError, ValueError):
        # Not valid JSON, treat as plain string
        args = json.dumps({"query": eff_args})

Changes Made

  • Fixed web_search_call argument processing (lines ~324-328)
  • Fixed function_call argument processing (lines ~405-409)
  • Maintained backwards compatibility for plain string arguments
  • Added proper JSON parsing with error handling

Testing

The fix handles multiple scenarios correctly:

  • Direct objects (dict/list) - Serialized directly as before
  • JSON strings - Now parsed and used directly ✅ FIXED
  • Plain strings - Still wrapped in query field for compatibility
  • Invalid data - Safely defaults to empty object

Impact

This fix resolves tool call argument serialization issues for VS Code extensions and other clients that pass structured data as JSON strings, ensuring proper parameter passing without breaking existing functionality for simple string queries.

Copilot AI review requested due to automatic review settings September 20, 2025 09:28
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a tool call argument serialization issue where JSON strings were being incorrectly wrapped in a query field instead of being parsed and used directly. The fix enhances argument processing to first attempt JSON parsing of string arguments, using the parsed object directly if it's a dict/list, while preserving backwards compatibility for plain strings.

Key Changes:

  • Enhanced string argument processing to parse JSON before wrapping in query field
  • Applied the fix to both web search call processing and function call output processing
  • Maintained backwards compatibility for plain string arguments

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@alexx-ftw
Copy link
Contributor

alexx-ftw commented Sep 20, 2025

Amazing good job. Thanks for the PR!
@RayBytes I would like to take this opportunity to propose introducing a suite of tests that cover the different functional surfaces. This project is becoming increasingly bigger and depended upon, and it could help ship clean, efficient code.

@alexx-ftw
Copy link
Contributor

Just tested this locally, fix works as advertised.

  • On main a JSON string like "{"city":"Paris"}" gets double‑encoded under query.
  • On this branch it becomes a real object. Plain strings are wrapped as {"query":"..."}. Dicts/lists pass through unchanged.
  • Fallback to {} on errors stays the same.

I simulated a few SSE events to reproduce. PR branch passes, main fails the JSON‑string case. LGTM.

@alexx-ftw
Copy link
Contributor

alexx-ftw commented Sep 20, 2025

Quick update: I opened a small stacked PR with follow ups so the diff stays focused:

  • reuse one serializer across routes
  • preserve string args for web_search in streaming
  • keep list args in transform
  • no tests added, no flags changed

Stacked PR: phawrylak#1
Happy to tweak or split if you prefer.

@RayBytes
Copy link
Owner

Amazing good job. Thanks for the PR! @RayBytes I would like to take this opportunity to propose introducing a suite of tests that cover the different functional surfaces. This project is becoming increasingly bigger and depended upon, and it could help ship clean, efficient code.

Thats fair, wouldn't be accepting tests in this PR, but later on please do propose a PR for tests. Could definitely be beneficial so we don't have these issues.

@RayBytes
Copy link
Owner

Quick update: I opened a small stacked PR with follow ups so the diff stays focused:

  • reuse one serializer across routes
  • preserve string args for web_search in streaming
  • keep list args in transform
  • no tests added, no flags changed

Stacked PR: phawrylak#1 Happy to tweak or split if you prefer.

I honestly do not feel these changes are needed for this simple bug fix, and personally find the code slightly less legible (for _serialize_tool_args). PR LGTM

@RayBytes RayBytes merged commit fe5796a into RayBytes:main Sep 20, 2025
@alexx-ftw
Copy link
Contributor

alexx-ftw commented Sep 20, 2025

Quick update: I opened a small stacked PR with follow ups so the diff stays focused:

  • reuse one serializer across routes
  • preserve string args for web_search in streaming
  • keep list args in transform
  • no tests added, no flags changed

Stacked PR: phawrylak#1 Happy to tweak or split if you prefer.

I honestly do not feel these changes are needed for this simple bug fix, and personally find the code slightly less legible (for _serialize_tool_args). PR LGTM

Thanks for the quick review - keeping #39 focused makes sense.

One thing I re-checked on the current branch:

  • Streaming web_search: when the model sends arguments as a JSON string, the emitted arguments become {} instead of the parsed
    object. I can share a 10‑line repro if helpful.

Happy to proceed however you prefer:

  • I can open a tiny follow‑up that fixes only this web_search string case, no helper, minimal diff.
  • Or I can file an Issue first and park it until you want it addressed.
  • I’ll close my stacked PR to avoid noise unless you want to pull anything from it.

Wen-Tsui pushed a commit to Wen-Tsui/ChatMock that referenced this pull request Nov 16, 2025
* Tool calling arguments JSON fix

* Extracted duplicated code to helper function

* Update utils.py

---------

Co-authored-by: Game_Time <108236317+RayBytes@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants