Skip to content

adds more robust arg parsing - #31

Open
MrDiggles2 wants to merge 1 commit into
digitalocean:mainfrom
MrDiggles2:chan/adds-more-robust-args-parsing
Open

adds more robust arg parsing#31
MrDiggles2 wants to merge 1 commit into
digitalocean:mainfrom
MrDiggles2:chan/adds-more-robust-args-parsing

Conversation

@MrDiggles2

Copy link
Copy Markdown
Contributor

Noticed that with some "weaker" models like openai-gpt-oss-20b tend to over-escape JSON for tool call arguments, e.g. {"query": \\"foo\\"} instead of {"query": "foo"}

This PR adds a new function to repair the JSON if the initial parse fails.

Also ran make format_diff

@bnarasimha21 bnarasimha21 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for this — the intent is right and the repair does fix the openai-gpt-oss-20b over-escape case. But the blanket replace('\\"', '"') introduces a regression worse than the bug it fixes:

1. Truncated payloads now execute with silently corrupted arguments. A payload cut off right after a legitimate in-string escape — e.g. {"note": "5\"} (realistic max_tokens truncation) — fails the first parse, then "repairs" to valid JSON {"note": "5"} with the quote deleted. Before this PR that payload was skipped with a warning; now the tool runs with wrong args. Worse example: {"a": "\"} repairs to {"a": ""} — the entire value erased. Suggest guarding the repair so it only triggers when the payload looks wholly over-escaped (e.g. starts with {\"), and adding {"note": "5\"} as a "must skip, not mis-parse" regression test.

2. Streaming has the same bug with a quieter failure. _stream puts raw argument strings into tool_call_chunks, and langchain-core's chunk assembly (parse_partial_json) silently degrades over-escaped args to args: {} — the tool call lands in tool_calls, not invalid_tool_calls, and executes with empty arguments (verified on langchain-core 1.4.8, the locked version). If the repair is worth doing in _generate, the streaming path needs it more.

Also a heads-up: most of this diff is make format_diff churn, which will conflict with #32.

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.

2 participants