fix: escape control characters in LLM tool call arguments JSON

[bjulian5]

• Add json_escape_control_chars_in_string() utility function
• Apply fix to OpenAI and Databricks response parsers
• Enhance error messages with raw and processed arguments
• Add comprehensive test coverage for control character escaping

Testing

• ✅ Added unit tests for the new utility function covering various control character scenarios
• ✅ Updated existing tests to verify empty argument handling
• ✅ Manual testing with both OpenAI providers
• ✅ Verified that valid JSON arguments are unchanged
• ✅ Confirmed that structural JSON elements (quotes, backslashes) are preserved

Backwards Compatibility

This change is fully backwards compatible. It only affects the processing of malformed JSON arguments that would have previously failed to parse. Valid JSON arguments continue to work exactly as before.

Related Issues

Fixes #2892

[michaelneale]

looks nice - @salman1993 is active in there with goose-llm crate so probably good to get a blessing there.

[salman1993]

goose-llm crate is currently not used by goose. can you please describe where / how you ran into this error? #2892

i have a feeling the bug is in goose crate and not goose-llm

[bjulian5]

@salman1993 I ran into this error using the goose cli. I fixed in the goose crate and found that the exact same code was in goose-llm so added a fix there as well.

[bjulian5]

@salman1993 bump 🙏

[taniacryptid]

Let me bump this to the goose dev team for you!

[angiejones]

I removed the changes to crates/goose-llm. @salman1993 can you take another look? other users are indicating this PR will fix their issues as well

[DOsinga]

I think we should move the escaping level one higher - see my remark and then it should be good to go

[DOsinga]

so what's happening here is that we receive a json document from the provider that almost certainly is correct json, but that the arguments element in it is doubly encoded json and that second tag is not always entirely correct?

if so I think we should slightly change the approach here - move the entire json parsing into utils and call it safely_parse_json and first try if it parses without your replacements and only if it doesn't do the escape replacements.

arguments could be something like {"key1": "value1",\n"key2": "value"} which contains a \n but is perfectly fine, while you are trying to fix {"key1": "value1\n","key2": "value"} which is not

[bjulian5]

@DOsinga updated the PR with your suggestions. PTAL @DOsinga @salman1993 🙏

[DOsinga]

can you do the DCO thing and we can get this in 🙏 https://github.com/block/goose/pull/2893/checks?check_run_id=46249519944

[michaelneale]

oh this is a nice one, kudos

[bjulian5]

@DOsinga done!

[DOsinga]

thanks!

[veriditin]

Hi! I've been tracking this issue and it's nice that most control characters are now properly escaped, but I think a few things were left on the table.

The quotation mark " can in MOST cases also be unambiguously escaped without breaking the JSON document. I also find my LLM of choice (qwen-3) makes the most mistakes in this category, so that would be nice to get fixed as well.

JSON is overly strictly defined in this sense. Escaping the quotation mark is only strictly necessary if the first non-white space character is a "meaningful" one, i.e. one of ,, :, ], }. If it is not, we know for sure we are still inside a String type and can replace the " with \". If it is followed by a meaning character (ignoring whitespace), we cannot know for sure so we should leave it alone.

{"contents": "yeah, this is an awesome string one might say "super" cool"} can be unambiguously escaped to: {"contents": "yeah, this is an awesome string one might say \"super\" cool"} as the first quotation mark within the contents value is followed by an s and the second one is followed by a c.

I additionally find that Qwen-3 will sometimes fail to properly close the JSON, i.e. to only use one } instead of }} if the final object is nested inside another object. I find it useful to keep track of the amount of opened objects and lists (in a stack / in order) and if the string ends with objects/list still open, to simply close them. So:

{"name": "John", "age": 30, "city": "New York" would be fixed to {"name": "John", "age": 30, "city": "New York"}

and combining the two:

{"items": [{"name": "item1", "desc": "A "great" item"}, {"name": "item2"} would be fixed to {"items": [{"name": "item1", "desc": "A \"great\" item"}, {"name": "item2"}]}

[DOsinga]

good points, @veriditin - but at that point I think that search & replace with regexps is not going to do it and we're going to need a tolerant json parser - https://crates.io/crates/json_partial looks like an option? not sure it does your " in the middle of a string though

[veriditin]

Looking at their examples as to what kind of errors can be fixed:

I think the comma fixer in this crate might clash with the quotation mark fix I'm suggesting (which in my experience is a more common problem than missing commas). So.. it seems this would need some careful consideration as to which types of problems happen significantly often and can be fixed independently of each other.. It also doesn't feel like this crate is making the best trade-offs for the goose/agentic use-case.