fix: make_json_serializable must not reinterpret JSON-looking strings#2524
Open
chuenchen309 wants to merge 1 commit into
Open
Conversation
make_json_serializable() opportunistically ran json.loads() on any string
starting with "{"/"[" and ending with "}"/"]", silently replacing the
original string with the parsed dict/list. This changes the Python type
of the value based on its content, which breaks the "make this object
JSON-serializable" contract - a string is already JSON-serializable and
should never need reinterpreting.
This is reachable through ToolCall.dict() (memory.py), which feeds both
the documented RunResult.steps API and, more importantly,
ActionStep.to_messages()'s "Calling tools:\n" + str([tc.dict() for tc in
self.tool_calls]) - the actual conversation history replayed back to the
model. Any tool whose argument is a plain string that happens to look
like JSON (e.g. a file-writing tool given literal JSON text to write) has
that argument silently mutated from str to dict in what the model sees
of its own prior tool calls.
Removed the JSON-sniffing block so strings pass through unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
make_json_serializable()opportunistically ranjson.loads()on any string starting with{/[and ending with}/], silently replacing the original string with the parsed dict/list. This changes the Python type of a value based on its content, which breaks the "make this object JSON-serializable" contract — a string is already JSON-serializable and should never need reinterpreting.This is reachable through
ToolCall.dict()(memory.py), which feeds both the documentedRunResult.stepsAPI and, more importantly,ActionStep.to_messages()'s"Calling tools:\n" + str([tc.dict() for tc in self.tool_calls])— the actual conversation history replayed back to the model. Any tool whose argument is a plain string that happens to look like JSON (e.g. a file-writing tool given literal JSON text to write) has that argument silently mutated fromstrtodictin what the model sees of its own prior tool calls.Fix
Removed the JSON-sniffing block so strings pass through unchanged.
Testing
Added
test_make_json_serializable_preserves_json_looking_stringstotests/test_utils.py. Confirmed viagit stashthat it fails against the pre-fix code (string values get silently turned into dict/list) and passes after.pytest tests/test_utils.py tests/test_memory.py— 45 + 12 passed, no regressions.ruff checkandruff format --checkboth clean on the changed files.AI disclosure
Developed with AI assistance (Claude Code), which located the bug via a targeted search for functions with no test coverage and traced how
ToolCall.dict()'s output reaches both the publicRunResult.stepsAPI and the model-facing conversation history. I reviewed the diff, independently reproduced the type-mutation bug and confirmed the fix by executing both against the actual function, and ran the tests/linters myself before opening this PR.