diff --git a/crates/tui/src/client/responses.rs b/crates/tui/src/client/responses.rs index 93150086..c3e3e2c7 100644 --- a/crates/tui/src/client/responses.rs +++ b/crates/tui/src/client/responses.rs @@ -597,11 +597,16 @@ fn convert_messages_to_responses_input(request: &MessageRequest) -> Vec { /// Convert a CodeWhale tool definition to a Responses API function tool. fn tool_to_responses_function(tool: &Tool) -> Value { let mut parameters = tool.input_schema.clone(); - schema_sanitize::sanitize_for_responses(&mut parameters); + let constraint_note = schema_sanitize::sanitize_for_responses(&mut parameters); + let description = match constraint_note { + Some(note) if tool.description.trim().is_empty() => note, + Some(note) => format!("{}\n\n{}", tool.description.trim(), note), + None => tool.description.clone(), + }; json!({ "type": "function", "name": tool.name, - "description": tool.description, + "description": description, "parameters": parameters, "strict": false, }) @@ -750,6 +755,66 @@ mod tests { assert!(parameters.get("not").is_none()); assert!(parameters["properties"].get("patch").is_some()); assert!(parameters["properties"].get("changes").is_some()); + assert_eq!( + payload["description"], + "Apply patch\n\nExactly one of these parameter groups must be provided: `changes` | `patch`." + ); assert!(tool.input_schema.get("oneOf").is_some()); } + + #[test] + fn responses_function_tool_trims_description_before_constraint_note() { + let tool = Tool { + tool_type: None, + name: "apply_patch".to_string(), + description: "Apply patch\n".to_string(), + input_schema: json!({ + "type": "object", + "properties": { + "patch": {"type": "string"}, + "changes": {"type": "array"} + }, + "oneOf": [ + {"required": ["patch"]}, + {"required": ["changes"]} + ] + }), + allowed_callers: None, + defer_loading: None, + input_examples: None, + strict: None, + cache_control: None, + }; + + let payload = tool_to_responses_function(&tool); + + assert_eq!( + payload["description"], + "Apply patch\n\nExactly one of these parameter groups must be provided: `changes` | `patch`." + ); + } + + #[test] + fn responses_function_tool_leaves_description_unchanged_without_constraint_note() { + let tool = Tool { + tool_type: None, + name: "lookup".to_string(), + description: "Lookup".to_string(), + input_schema: json!({ + "type": "object", + "properties": { + "query": {"type": "string"} + } + }), + allowed_callers: None, + defer_loading: None, + input_examples: None, + strict: None, + cache_control: None, + }; + + let payload = tool_to_responses_function(&tool); + + assert_eq!(payload["description"], "Lookup"); + } } diff --git a/crates/tui/src/tools/schema_sanitize.rs b/crates/tui/src/tools/schema_sanitize.rs index a55bb969..fc39a02c 100644 --- a/crates/tui/src/tools/schema_sanitize.rs +++ b/crates/tui/src/tools/schema_sanitize.rs @@ -41,28 +41,21 @@ pub fn sanitize(schema: &mut Value) { /// Prepare a complete active tool set for DeepSeek strict function-calling. /// -/// Returns `false` and leaves the tools in non-strict mode when any root schema -/// uses conditional alternatives (`anyOf`, `oneOf`, or `allOf`). DeepSeek's -/// strict object rules make every property required, so forcing strict mode on -/// root-alternative tools such as `apply_patch` or `finance` would either 400 or -/// change their semantics. In that case callers should keep the normal -/// best-effort schema and may still use `tool_choice = "required"`. +/// Each tool is evaluated independently: compatible schemas are sanitized and +/// marked strict, while incompatible schemas remain unchanged and non-strict. +/// Returns `true` only when every tool in the set can use strict mode. pub fn prepare_tools_for_strict_mode(tools: &mut [Tool]) -> bool { - if tools - .iter() - .any(|tool| !strict_schema_supported(&tool.input_schema)) - { - for tool in tools { - tool.strict = None; - } - return false; - } - + let mut all_strict = true; for tool in tools { - sanitize_for_strict(&mut tool.input_schema); - tool.strict = Some(true); + if strict_schema_supported(&tool.input_schema) { + sanitize_for_strict(&mut tool.input_schema); + tool.strict = Some(true); + } else { + tool.strict = None; + all_strict = false; + } } - true + all_strict } /// Sanitize a schema for DeepSeek strict function-calling. @@ -82,7 +75,14 @@ pub fn sanitize_for_strict(schema: &mut Value) { /// schema permissive rather than changing tool semantics: merge any root /// alternative properties we can see, then remove the root-only composition /// keywords while preserving nested schemas. -pub fn sanitize_for_responses(schema: &mut Value) { +/// +/// Returns a short description note when root composition constraints with +/// meaningful `required` groups are dropped. +pub fn sanitize_for_responses(schema: &mut Value) -> Option { + let constraint_note = schema + .as_object() + .and_then(root_composition_constraint_note); + sanitize(schema); if !schema.is_object() { @@ -90,7 +90,7 @@ pub fn sanitize_for_responses(schema: &mut Value) { } let Some(obj) = schema.as_object_mut() else { - return; + return constraint_note; }; merge_root_composition_properties(obj); @@ -102,6 +102,7 @@ pub fn sanitize_for_responses(schema: &mut Value) { obj.remove("not"); ensure_properties_object(obj); prune_dangling_required(schema); + constraint_note } fn strict_schema_supported(schema: &Value) -> bool { @@ -230,13 +231,23 @@ fn enforce_strict_subset(schema: &mut Value) { if let Some(obj) = schema.as_object_mut() { strip_unsupported_strict_keywords(obj); if is_object_schema(obj) { - let mut property_names: Vec = ensure_properties_object(obj) - .keys() - .cloned() - .map(Value::String) - .collect(); - property_names.sort_by(|a, b| a.as_str().cmp(&b.as_str())); - obj.insert("required".into(), Value::Array(property_names)); + let originally_required = required_names(obj); + let properties = ensure_properties_object(obj); + let mut property_names: Vec = properties.keys().cloned().collect(); + property_names.sort(); + for property_name in &property_names { + if !originally_required + .iter() + .any(|required| required == property_name) + && let Some(property_schema) = properties.get_mut(property_name) + { + mark_nullable(property_schema); + } + } + obj.insert( + "required".into(), + Value::Array(property_names.into_iter().map(Value::String).collect()), + ); obj.insert("additionalProperties".into(), Value::Bool(false)); } @@ -279,6 +290,25 @@ fn ensure_properties_object(obj: &mut Map) -> &mut Map) -> Vec { + obj.get("required") + .and_then(Value::as_array) + .map(|required| { + required + .iter() + .filter_map(Value::as_str) + .map(ToOwned::to_owned) + .collect() + }) + .unwrap_or_default() +} + +fn mark_nullable(schema: &mut Value) { + if let Some(obj) = schema.as_object_mut() { + obj.insert("nullable".into(), Value::Bool(true)); + } +} + fn merge_root_composition_properties(obj: &mut Map) { let mut merged = Map::new(); for key in ["oneOf", "anyOf", "allOf"] { @@ -305,11 +335,64 @@ fn merge_root_composition_properties(obj: &mut Map) { } } +fn root_composition_constraint_note(obj: &Map) -> Option { + for (key, prefix) in [ + ("oneOf", "Exactly one"), + ("anyOf", "At least one"), + ("allOf", "All"), + ] { + let Some(items) = obj.get(key).and_then(Value::as_array) else { + continue; + }; + let mut groups: Vec = items.iter().filter_map(required_group_label).collect(); + groups.sort(); + groups.dedup(); + if groups.len() >= 2 { + return Some(format!( + "{prefix} of these parameter groups must be provided: {}.", + groups.join(" | ") + )); + } + } + None +} + +fn required_group_label(item: &Value) -> Option { + let mut names: Vec = item + .get("required")? + .as_array()? + .iter() + .filter_map(Value::as_str) + .map(|name| format!("`{name}`")) + .collect(); + if names.is_empty() { + None + } else { + names.sort(); + names.dedup(); + Some(names.join(" + ")) + } +} + #[cfg(test)] mod tests { use super::*; use serde_json::json; + fn test_tool(name: &str, input_schema: Value) -> Tool { + Tool { + tool_type: None, + name: name.to_string(), + description: name.to_string(), + input_schema, + allowed_callers: None, + defer_loading: None, + input_examples: None, + strict: None, + cache_control: None, + } + } + #[test] fn collapses_nullable_anyof() { let mut schema = json!({ @@ -509,6 +592,53 @@ mod tests { assert_eq!(schema["additionalProperties"], false); assert_eq!(schema["required"], json!(["count", "name"])); + assert_eq!(schema["properties"]["count"]["nullable"], true); + assert!(schema["properties"]["name"].get("nullable").is_none()); + } + + #[test] + fn strict_sanitize_preserves_optional_properties_as_nullable() { + let mut schema = json!({ + "type": "object", + "properties": { + "path": {"type": "string"}, + "start_line": {"type": "integer"}, + "max_lines": {"type": "integer"}, + "options": { + "type": "object", + "properties": { + "encoding": {"type": "string"}, + "trim": {"type": "boolean"} + }, + "required": ["encoding"] + } + }, + "required": ["path", "options"] + }); + + sanitize_for_strict(&mut schema); + + assert_eq!( + schema["required"], + json!(["max_lines", "options", "path", "start_line"]) + ); + assert!(schema["properties"]["path"].get("nullable").is_none()); + assert!(schema["properties"]["options"].get("nullable").is_none()); + assert_eq!(schema["properties"]["start_line"]["nullable"], true); + assert_eq!(schema["properties"]["max_lines"]["nullable"], true); + assert_eq!( + schema["properties"]["options"]["required"], + json!(["encoding", "trim"]) + ); + assert!( + schema["properties"]["options"]["properties"]["encoding"] + .get("nullable") + .is_none() + ); + assert_eq!( + schema["properties"]["options"]["properties"]["trim"]["nullable"], + true + ); } #[test] @@ -577,32 +707,60 @@ mod tests { } #[test] - fn strict_mode_rejects_root_composition_for_whole_tool_set() { - let mut tools = vec![Tool { - tool_type: None, - name: "either".to_string(), - description: "Either input shape".to_string(), - input_schema: json!({ - "type": "object", - "properties": { - "a": {"type": "string"}, - "b": {"type": "string"} - }, - "anyOf": [ - {"required": ["a"]}, - {"required": ["b"]} - ] - }), - allowed_callers: None, - defer_loading: None, - input_examples: None, - strict: Some(true), - cache_control: None, - }]; + fn strict_mode_applies_per_tool_in_mixed_catalog() { + let mut tools = vec![ + test_tool( + "lookup", + json!({ + "type": "object", + "properties": { + "query": {"type": "string"} + }, + "required": [] + }), + ), + test_tool( + "either", + json!({ + "type": "object", + "properties": { + "a": {"type": "string"}, + "b": {"type": "string"} + }, + "anyOf": [ + {"required": ["a"]}, + {"required": ["b"]} + ] + }), + ), + test_tool( + "nested", + json!({ + "type": "object", + "properties": { + "value": { + "oneOf": [ + {"type": "string"}, + {"type": "integer"} + ] + } + } + }), + ), + ]; assert!(!prepare_tools_for_strict_mode(&mut tools)); - assert_eq!(tools[0].strict, None); - assert!(tools[0].input_schema.get("anyOf").is_some()); + assert_eq!(tools[0].strict, Some(true)); + assert_eq!(tools[0].input_schema["required"], json!(["query"])); + assert_eq!(tools[0].input_schema["additionalProperties"], false); + assert_eq!(tools[1].strict, None); + assert!(tools[1].input_schema.get("anyOf").is_some()); + assert_eq!(tools[2].strict, None); + assert!( + tools[2].input_schema["properties"]["value"] + .get("oneOf") + .is_some() + ); } #[test] @@ -684,7 +842,7 @@ mod tests { ] }); - sanitize_for_responses(&mut schema); + let note = sanitize_for_responses(&mut schema); assert_eq!(schema["type"], "object"); assert!(schema.get("oneOf").is_none()); @@ -694,6 +852,10 @@ mod tests { assert!(schema.get("not").is_none()); assert!(schema["properties"].get("patch").is_some()); assert!(schema["properties"].get("changes").is_some()); + assert_eq!( + note.as_deref(), + Some("Exactly one of these parameter groups must be provided: `changes` | `patch`.") + ); } #[test] @@ -717,13 +879,17 @@ mod tests { ] }); - sanitize_for_responses(&mut schema); + let note = sanitize_for_responses(&mut schema); assert_eq!(schema["type"], "object"); assert!(schema.get("anyOf").is_none()); assert!(schema["properties"].get("path").is_some()); assert!(schema["properties"].get("url").is_some()); assert!(schema.get("required").is_none()); + assert_eq!( + note.as_deref(), + Some("At least one of these parameter groups must be provided: `path` | `url`.") + ); } #[test] @@ -740,11 +906,51 @@ mod tests { } }); - sanitize_for_responses(&mut schema); + let note = sanitize_for_responses(&mut schema); assert_eq!(schema["type"], "object"); assert!(schema.get("anyOf").is_none()); assert!(schema["properties"]["value"].get("anyOf").is_some()); + assert_eq!(note, None); + } + + #[test] + fn responses_sanitize_plain_object_has_no_constraint_note() { + let mut schema = json!({ + "type": "object", + "properties": { + "query": {"type": "string"} + } + }); + + let note = sanitize_for_responses(&mut schema); + + assert_eq!(schema["type"], "object"); + assert_eq!(note, None); + } + + #[test] + fn responses_constraint_note_is_sorted_and_deduped() { + let mut schema = json!({ + "type": "object", + "properties": { + "a": {"type": "string"}, + "b": {"type": "string"}, + "c": {"type": "string"} + }, + "oneOf": [ + {"required": ["b", "a", "a"]}, + {"required": ["c"]}, + {"required": ["a", "b"]} + ] + }); + + let note = sanitize_for_responses(&mut schema); + + assert_eq!( + note.as_deref(), + Some("Exactly one of these parameter groups must be provided: `a` + `b` | `c`.") + ); } }