From c9f927f90e8d2d2d6aefc5d16dcd2fadce108fe1 Mon Sep 17 00:00:00 2001 From: Dr Alexander Mikhalev Date: Sun, 30 Aug 2026 23:04:48 +0100 Subject: [PATCH] fix(openai): omit absent tool description, never serialise a null schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The OpenAI adapter built each tool with `json!`, so both optional fields serialised as explicit nulls when unset: "description": tool.description // None -> null "parameters": tool.schema // None -> null Neither is valid. `description` is optional in the OpenAI function schema, and omitting a key is not the same as sending `null` — providers that validate the payload reject a null where a string is expected. `parameters` is worse: it is *required* to be a schema object, so `null` is not a legal value at all. Builds the function object explicitly instead: `description` is inserted only when present, and `parameters` falls back to `{"type": "object"}` — the empty schema, which is what a tool taking no arguments actually means. `strict: false` is deliberately unchanged. It is the documented default, and altering it would change structured-output behaviour for every adapter that shares this serialiser; the existing TODO about `strict: true` needing `additionalProperties: false` is left in place for whoever takes that on. Reachable in practice: terraphim-llm-proxy accepts tools in both the OpenAI shape (`{type, function:{...}}`) and the Anthropic shape (`{name, input_schema}`), and the Anthropic form frequently carries no description. ## Tests Five in `adapter_shared::tests`: - absent description is omitted rather than nulled - present description still serialises - absent schema falls back to the empty object schema, never null - present schema passes through verbatim - `strict` and the `type: function` envelope are preserved The first and third fail against the previous code. Note: `cargo clippy -- -D warnings` fails on `src/webc/web_stream.rs` (collapsible_match). That is pre-existing on main and untouched here. --- src/adapter/adapters/openai/adapter_shared.rs | 95 ++++++++++++++++--- 1 file changed, 84 insertions(+), 11 deletions(-) diff --git a/src/adapter/adapters/openai/adapter_shared.rs b/src/adapter/adapters/openai/adapter_shared.rs index b82a1ee8..4867e150 100644 --- a/src/adapter/adapters/openai/adapter_shared.rs +++ b/src/adapter/adapters/openai/adapter_shared.rs @@ -381,19 +381,30 @@ impl OpenAIAdapter { tools .into_iter() .map(|tool| { - // TODO: Need to handle the error correctly - // TODO: Needs to have a custom serializer (tool should not have to match to a provider) - // NOTE: Right now, low probability, so, we just return null if cannot convert to value. + // `description` is optional in the OpenAI function schema, but + // `json!` turns `None` into an explicit `null`, and providers that + // validate the payload reject a null where a string is expected. + // Omit the key instead of emitting null. + // + // `parameters` is *required* to be a schema object, so a tool + // declared without one falls back to the empty object schema + // rather than serialising `null`. + let mut function = serde_json::Map::new(); + function.insert("name".to_string(), json!(tool.name)); + if let Some(description) = tool.description { + function.insert("description".to_string(), json!(description)); + } + function.insert( + "parameters".to_string(), + tool.schema.unwrap_or_else(|| json!({"type": "object"})), + ); + // TODO: If we need to support `strict: true` we need to add additionalProperties: false into the schema + // above (like structured output) + function.insert("strict".to_string(), json!(false)); + json!({ "type": "function", - "function": { - "name": tool.name, - "description": tool.description, - "parameters": tool.schema, - // TODO: If we need to support `strict: true` we need to add additionalProperties: false into the schema - // above (like structured output) - "strict": false, - } + "function": Value::Object(function), }) }) .collect::>() @@ -468,6 +479,68 @@ mod tests { ModelIden::new(AdapterKind::OpenAI, "test-model") } + fn tool_named(name: &str) -> crate::chat::Tool { + crate::chat::Tool::new(name.to_string()) + } + + fn serialised_tools(tools: Vec) -> Vec { + let chat_req = ChatRequest::new(vec![ChatMessage::user("hi")]).with_tools(tools); + let parts = OpenAIAdapter::into_openai_request_parts(&test_model(), chat_req).expect("should serialize"); + parts.tools.expect("tools must be present") + } + + /// `description` is optional in the OpenAI function schema. Serialising it + /// as an explicit `null` is not the same as omitting it: providers that + /// validate the payload reject a null where a string is expected. + #[test] + fn test_tool_without_description_omits_the_key_rather_than_emitting_null() { + let tools = serialised_tools(vec![tool_named("get_weather")]); + let function = &tools[0]["function"]; + + assert!( + function.get("description").is_none(), + "absent description must be omitted, got: {function}" + ); + assert_eq!(function["name"], "get_weather"); + } + + #[test] + fn test_tool_with_description_still_serialises_it() { + let tools = serialised_tools(vec![tool_named("get_weather").with_description("Get weather")]); + assert_eq!(tools[0]["function"]["description"], "Get weather"); + } + + /// `parameters` is required to be a schema object. A tool declared without + /// one previously serialised `"parameters": null`, which is not a schema. + #[test] + fn test_tool_without_schema_falls_back_to_the_empty_object_schema() { + let tools = serialised_tools(vec![tool_named("no_args")]); + let parameters = &tools[0]["function"]["parameters"]; + + assert!(!parameters.is_null(), "parameters must never serialise as null"); + assert_eq!(parameters["type"], "object"); + } + + #[test] + fn test_tool_with_schema_passes_it_through_verbatim() { + let schema = json!({ + "type": "object", + "properties": {"city": {"type": "string"}}, + "required": ["city"], + }); + let tools = serialised_tools(vec![tool_named("get_weather").with_schema(schema.clone())]); + assert_eq!(tools[0]["function"]["parameters"], schema); + } + + /// `strict` is deliberately unchanged: `false` is the documented default and + /// altering it would change structured-output behaviour for every adapter. + #[test] + fn test_strict_flag_is_preserved() { + let tools = serialised_tools(vec![tool_named("get_weather")]); + assert_eq!(tools[0]["function"]["strict"], false); + assert_eq!(tools[0]["type"], "function"); + } + /// When an assistant message carries reasoning_content, it must appear /// in the serialized JSON so providers that require it (Kimi, DeepSeek) /// don't reject the request.