fix(openai): tolerate object-form tool-call arguments in streaming#1822
Conversation
Shaurya-Sethi
left a comment
There was a problem hiding this comment.
thanks for the clear write-up and focused fix.
I've verified the deserialization failure on main using the minimal json from your pr (object-form "arguments": {} fails with invalid type: map, expected a string), but haven't reproduced it against a live openai-compatible gateway.
Before merge
- Issue + real repro : Please open an issue for this bug on the actual OpenAI-compatible gateway that emits object-form
argumentsin streaming (provider name, endpoint if relevant, and steps to reproduce). Link it from this PR (Fixes #<n>).
The new unit tests are a good regression guard, but we will still need a reproducible report on the actual gateway.
- Deduplication : As noted inline on
de_tool_call_arguments, please consider reusing the existingjson_utils::value_to_json_stringhelper.
Once you document and link the issue, a maintainer will likely follow-up with additional feedback before approving :)
| /// | ||
| /// We now accept either: a string is taken verbatim; any other JSON value is | ||
| /// re-serialized to its compact JSON-string form. | ||
| fn de_tool_call_arguments<'de, D>(deserializer: D) -> Result<Option<String>, D::Error> |
There was a problem hiding this comment.
This logic duplicates json_utils::value_to_json_string in crates/rig-core/src/json_utils.rs, which already does string vs non-string normalization:
pub fn value_to_json_string(value: &serde_json::Value) -> String {
match value {
serde_json::Value::String(s) => s.clone(),
other => other.to_string(),
}
}We could reuse that (or add a small shared deserializer in json_utils) instead of maintaining a second copy here
There was a problem hiding this comment.
Thanks for catching this.
Moved it into json_utils::deserialize_json_string_or_value (on top of the existing value_to_json_string) and applied via #[serde(deserialize_with = …)]. Added unit tests in json_utils; the streaming tests stay as integration coverage.
463e173 to
f371efd
Compare
eb45cc1 to
ac502ea
Compare
|
Thanks for the PR! And if you want, you can mention your company in the "who's using rig" section of our readme! |
|
Thanks! Happy to contribute, and thanks for the offer about the README! |
Fixes #1829
Problem
Some OpenAI-compatible gateways stream
tool_calls[].function.argumentsas a JSON object (e.g."arguments": {}) instead of the spec-mandated JSON-encoded string ("arguments": "{}").StreamingFunction.argumentsis typedOption<String>, so serde fails withinvalid type: map, expected a string.In
normalize_chunkthe parse error is only logged, and the whole SSE chunk is dropped (Ok(None)), so the tool call never surfaces, and the turn ends with empty assistant text, and then the caller sees a blank response, with no error unlessrigdebug logs are enabled.Minimal reproducing chunk:
{"choices":[{"index":0,"delta":{"role":"assistant","content":"","tool_calls":[{"index":0,"id":"call_x","type":"function","function":{"name":"list_dir","arguments":{}}}]},"finish_reason":null}],"usage":null}Fix
Deserialize
argumentswith a tolerant helper: a string is taken verbatim; any other JSON value is re-serialized to its compact JSON-string form.null/missing staysNone.Fully backward compatible with the spec-compliant string form.
Moves the tolerant deserializer into a shared
json_utils::deserialize_json_string_or_value(built on the existingvalue_to_json_string) and applies it toStreamingFunction.argumentsvia#[serde(deserialize_with = …)], so other providers can reuse it.Adds unit tests in
json_utilscovering string / object / nested / null / missing.Tests
Adds
test_streaming_function_object_arguments(empty + nested object) andtest_streaming_function_null_arguments(null / missing).All existing streaming tests still pass.