From 323598e764c104272fb0b2d254cb0f27beb9ee37 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Thu, 7 May 2026 04:12:22 -0500 Subject: [PATCH] fix(api): harden strict tool schemas (#1005) --- crates/tui/src/client.rs | 54 +++++ crates/tui/src/client/chat.rs | 36 ++- crates/tui/src/config.rs | 5 +- crates/tui/src/core/engine.rs | 4 +- crates/tui/src/core/engine/turn_loop.rs | 7 +- crates/tui/src/tools/schema_sanitize.rs | 281 ++++++++++++++++++++++++ crates/tui/src/tui/app.rs | 1 + 7 files changed, 381 insertions(+), 7 deletions(-) diff --git a/crates/tui/src/client.rs b/crates/tui/src/client.rs index e71cc8ae..b5a0eb56 100644 --- a/crates/tui/src/client.rs +++ b/crates/tui/src/client.rs @@ -967,6 +967,7 @@ mod tests { use crate::client::chat::{ build_chat_messages, build_chat_messages_for_request, count_reasoning_replay_chars, parse_chat_message, parse_sse_chunk, sanitize_thinking_mode_messages, tool_to_chat, + tool_to_chat_for_base_url, }; use crate::models::{ ContentBlock, ContentBlockStart, Delta, Message, MessageRequest, StreamEvent, Tool, @@ -1365,6 +1366,59 @@ mod tests { assert!(encoded.get("strict").is_none()); } + #[test] + fn deepseek_non_beta_base_url_strips_strict_tool_flag() { + let tool = Tool { + tool_type: Some("function".to_string()), + name: "emit_json".to_string(), + description: "Emit JSON".to_string(), + input_schema: json!({"type": "object", "properties": {}}), + allowed_callers: None, + defer_loading: None, + input_examples: None, + strict: Some(true), + cache_control: None, + }; + + let encoded = tool_to_chat_for_base_url(&tool, "https://api.deepseek.com/v1"); + + assert!( + encoded + .get("function") + .and_then(|function| function.get("strict")) + .is_none() + ); + } + + #[test] + fn deepseek_beta_and_custom_base_urls_keep_strict_tool_flag() { + let tool = Tool { + tool_type: Some("function".to_string()), + name: "emit_json".to_string(), + description: "Emit JSON".to_string(), + input_schema: json!({"type": "object", "properties": {}}), + allowed_callers: None, + defer_loading: None, + input_examples: None, + strict: Some(true), + cache_control: None, + }; + + for base_url in [ + "https://api.deepseek.com/beta", + "https://example.com/openai/v1", + ] { + let encoded = tool_to_chat_for_base_url(&tool, base_url); + assert_eq!( + encoded + .get("function") + .and_then(|function| function.get("strict")) + .and_then(Value::as_bool), + Some(true) + ); + } + } + #[test] fn chat_messages_drop_thinking_only_assistant_for_non_reasoning_model() { let message = Message { diff --git a/crates/tui/src/client/chat.rs b/crates/tui/src/client/chat.rs index af1c8d53..cd854ec2 100644 --- a/crates/tui/src/client/chat.rs +++ b/crates/tui/src/client/chat.rs @@ -88,7 +88,12 @@ impl DeepSeekClient { body["top_p"] = json!(top_p); } if let Some(tools) = request.tools.as_ref() { - body["tools"] = json!(tools.iter().map(tool_to_chat).collect::>()); + body["tools"] = json!( + tools + .iter() + .map(|tool| tool_to_chat_for_base_url(tool, &self.base_url)) + .collect::>() + ); } if let Some(choice) = request.tool_choice.as_ref() && let Some(mapped) = map_tool_choice_for_chat(choice) @@ -157,7 +162,12 @@ impl DeepSeekClient { body["top_p"] = json!(top_p); } if let Some(tools) = request.tools.as_ref() { - body["tools"] = json!(tools.iter().map(tool_to_chat).collect::>()); + body["tools"] = json!( + tools + .iter() + .map(|tool| tool_to_chat_for_base_url(tool, &self.base_url)) + .collect::>() + ); } if let Some(choice) = request.tool_choice.as_ref() && let Some(mapped) = map_tool_choice_for_chat(choice) @@ -712,6 +722,28 @@ pub(super) fn tool_to_chat(tool: &Tool) -> Value { value } +pub(super) fn tool_to_chat_for_base_url(tool: &Tool, base_url: &str) -> Value { + let mut value = tool_to_chat(tool); + if !deepseek_base_url_supports_strict_tools(base_url) + && let Some(function) = value.get_mut("function") + && let Some(obj) = function.as_object_mut() + { + obj.remove("strict"); + } + value +} + +fn deepseek_base_url_supports_strict_tools(base_url: &str) -> bool { + let trimmed = base_url.trim_end_matches('/').to_ascii_lowercase(); + let is_deepseek = trimmed == "https://api.deepseek.com" + || trimmed == "https://api.deepseek.com/v1" + || trimmed == "https://api.deepseek.com/beta" + || trimmed == "https://api.deepseeki.com" + || trimmed == "https://api.deepseeki.com/v1" + || trimmed == "https://api.deepseeki.com/beta"; + !is_deepseek || trimmed.ends_with("/beta") +} + fn map_tool_choice_for_chat(choice: &Value) -> Option { if let Some(choice_str) = choice.as_str() { return Some(json!(choice_str)); diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index 512f08a1..0f838fa9 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -741,8 +741,9 @@ pub struct Config { pub mcp_config_path: Option, pub notes_path: Option, pub memory_path: Option, - /// When true, set `tool_choice: "required"` in all API requests so the - /// model MUST call a tool on every step (V4 strict tool-following mode). + /// When true, set `tool_choice: "required"` and opt compatible function + /// schemas into DeepSeek beta strict mode. Schemas with root alternatives + /// stay non-strict to avoid changing optional/one-of tool semantics. pub strict_tool_mode: Option, /// Additional system-prompt sources concatenated in declared order /// (#454). Paths are expanded via `expand_path` so `~` and env diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index 6c08cfc9..24127123 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -148,8 +148,8 @@ pub struct EngineConfig { /// caller resolves this from `Settings` once at engine /// construction; the engine never touches disk for it. pub locale_tag: String, - /// When true, force `tool_choice: "required"` so the model always calls - /// a tool on every turn step (V4 strict tool-following mode). + /// When true, force `tool_choice: "required"` and opt compatible function + /// schemas into DeepSeek beta strict mode. pub strict_tool_mode: bool, /// Workshop / large-tool-output routing (#548). `None` disables routing. pub workshop: Option, diff --git a/crates/tui/src/core/engine/turn_loop.rs b/crates/tui/src/core/engine/turn_loop.rs index 4b374f34..b43fb8a4 100644 --- a/crates/tui/src/core/engine/turn_loop.rs +++ b/crates/tui/src/core/engine/turn_loop.rs @@ -214,7 +214,7 @@ impl Engine { // Build the request let force_update_plan_this_step = force_update_plan_first && turn.tool_calls.is_empty(); - let active_tools = if tool_catalog.is_empty() { + let mut active_tools = if tool_catalog.is_empty() { None } else { Some(active_tools_for_step( @@ -223,6 +223,11 @@ impl Engine { force_update_plan_this_step, )) }; + if self.config.strict_tool_mode + && let Some(tools) = active_tools.as_mut() + { + crate::tools::schema_sanitize::prepare_tools_for_strict_mode(tools); + } // Resolve `auto` reasoning_effort to a concrete tier (#663). let effective_reasoning_effort = resolve_auto_effort( diff --git a/crates/tui/src/tools/schema_sanitize.rs b/crates/tui/src/tools/schema_sanitize.rs index 7b92523d..52d286b3 100644 --- a/crates/tui/src/tools/schema_sanitize.rs +++ b/crates/tui/src/tools/schema_sanitize.rs @@ -12,6 +12,8 @@ use serde_json::{Map, Value}; +use crate::models::Tool; + /// Sanitize a JSON Schema in-place for DeepSeek strict-tool compatibility. /// /// Applies a sequence of normalisations chosen to be semantics-preserving: @@ -37,6 +39,66 @@ 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"`. +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; + } + + for tool in tools { + sanitize_for_strict(&mut tool.input_schema); + tool.strict = Some(true); + } + true +} + +/// Sanitize a schema for DeepSeek strict function-calling. +/// +/// This extends the general sanitizer with the official strict-mode object +/// rules: every object must set `additionalProperties: false`, and every +/// property must be listed in `required`. +pub fn sanitize_for_strict(schema: &mut Value) { + sanitize(schema); + enforce_strict_subset(schema); +} + +fn strict_schema_supported(schema: &Value) -> bool { + let mut normalized = schema.clone(); + sanitize(&mut normalized); + !has_strict_incompatible_composition(&normalized, true) +} + +fn has_strict_incompatible_composition(schema: &Value, is_root: bool) -> bool { + if let Some(obj) = schema.as_object() { + if obj.contains_key("oneOf") || obj.contains_key("allOf") { + return true; + } + if is_root && obj.contains_key("anyOf") { + return true; + } + return obj + .values() + .any(|value| has_strict_incompatible_composition(value, false)); + } + schema.as_array().is_some_and(|arr| { + arr.iter() + .any(|value| has_strict_incompatible_composition(value, false)) + }) +} + /// Collapse `{"anyOf":[X, {"type":"null"}]}` → `X ∪ {"nullable": true}`. /// /// Same treatment for `oneOf`. Only collapses when exactly one non-null @@ -135,6 +197,59 @@ fn collapse_single_element_unions(schema: &mut Value) { } } +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)); + obj.insert("additionalProperties".into(), Value::Bool(false)); + } + + for value in obj.values_mut() { + enforce_strict_subset(value); + } + } else if let Some(arr) = schema.as_array_mut() { + for value in arr { + enforce_strict_subset(value); + } + } +} + +fn strip_unsupported_strict_keywords(obj: &mut Map) { + obj.remove("patternProperties"); + match obj.get("type").and_then(Value::as_str) { + Some("string") => { + obj.remove("minLength"); + obj.remove("maxLength"); + } + Some("array") => { + obj.remove("minItems"); + obj.remove("maxItems"); + } + _ => {} + } +} + +fn is_object_schema(obj: &Map) -> bool { + obj.get("type").and_then(Value::as_str) == Some("object") || obj.contains_key("properties") +} + +fn ensure_properties_object(obj: &mut Map) -> &mut Map { + let needs_replacement = !matches!(obj.get("properties"), Some(Value::Object(_))); + if needs_replacement { + obj.insert("properties".into(), Value::Object(Map::new())); + } + obj.get_mut("properties") + .and_then(Value::as_object_mut) + .expect("properties was just ensured as object") +} + #[cfg(test)] mod tests { use super::*; @@ -322,4 +437,170 @@ mod tests { sanitize(&mut schema); assert_eq!(schema, after_first, "sanitize must be idempotent"); } + + #[test] + fn strict_sanitize_requires_all_object_properties_and_closes_extra_keys() { + let mut schema = json!({ + "type": "object", + "properties": { + "name": {"type": "string"}, + "count": {"type": "integer"} + }, + "required": ["name"], + "additionalProperties": {"type": "string"} + }); + + sanitize_for_strict(&mut schema); + + assert_eq!(schema["additionalProperties"], false); + assert_eq!(schema["required"], json!(["count", "name"])); + } + + #[test] + fn strict_sanitize_applies_object_rules_recursively() { + let mut schema = json!({ + "type": "object", + "properties": { + "outer": { + "type": "object", + "properties": { + "inner": {"type": "string"} + }, + "required": [] + } + }, + "required": [] + }); + + sanitize_for_strict(&mut schema); + + assert_eq!(schema["required"], json!(["outer"])); + assert_eq!(schema["additionalProperties"], false); + assert_eq!(schema["properties"]["outer"]["required"], json!(["inner"])); + assert_eq!(schema["properties"]["outer"]["additionalProperties"], false); + } + + #[test] + fn strict_sanitize_removes_unsupported_string_and_array_bounds() { + let mut schema = json!({ + "type": "object", + "properties": { + "name": { + "type": "string", + "minLength": 1, + "maxLength": 64, + "pattern": "^[a-z]+$" + }, + "items": { + "type": "array", + "minItems": 1, + "maxItems": 5, + "items": {"type": "string"} + }, + "score": { + "type": "integer", + "minimum": 1, + "maximum": 5 + } + } + }); + + sanitize_for_strict(&mut schema); + + let name = &schema["properties"]["name"]; + assert!(name.get("minLength").is_none()); + assert!(name.get("maxLength").is_none()); + assert_eq!(name["pattern"], "^[a-z]+$"); + + let items = &schema["properties"]["items"]; + assert!(items.get("minItems").is_none()); + assert!(items.get("maxItems").is_none()); + + let score = &schema["properties"]["score"]; + assert_eq!(score["minimum"], 1); + assert_eq!(score["maximum"], 5); + } + + #[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, + }]; + + assert!(!prepare_tools_for_strict_mode(&mut tools)); + assert_eq!(tools[0].strict, None); + assert!(tools[0].input_schema.get("anyOf").is_some()); + } + + #[test] + fn strict_mode_rejects_nested_unsupported_composition() { + let mut tools = vec![Tool { + tool_type: None, + name: "nested".to_string(), + description: "Nested oneOf".to_string(), + input_schema: json!({ + "type": "object", + "properties": { + "value": { + "oneOf": [ + {"type": "string"}, + {"type": "integer"} + ] + } + } + }), + allowed_callers: None, + defer_loading: None, + input_examples: None, + strict: None, + cache_control: None, + }]; + + assert!(!prepare_tools_for_strict_mode(&mut tools)); + assert_eq!(tools[0].strict, None); + } + + #[test] + fn strict_mode_marks_compatible_tools_strict() { + let mut tools = vec![Tool { + tool_type: None, + name: "lookup".to_string(), + description: "Lookup".to_string(), + input_schema: json!({ + "type": "object", + "properties": { + "query": {"type": "string"} + }, + "required": [] + }), + allowed_callers: None, + defer_loading: None, + input_examples: None, + strict: None, + cache_control: None, + }]; + + assert!(prepare_tools_for_strict_mode(&mut tools)); + assert_eq!(tools[0].strict, Some(true)); + assert_eq!(tools[0].input_schema["required"], json!(["query"])); + assert_eq!(tools[0].input_schema["additionalProperties"], false); + } } diff --git a/crates/tui/src/tui/app.rs b/crates/tui/src/tui/app.rs index 44cc12e7..f66b44de 100644 --- a/crates/tui/src/tui/app.rs +++ b/crates/tui/src/tui/app.rs @@ -4297,6 +4297,7 @@ mod tests { #[test] fn recoverable_clear_stashes_nonempty_draft() { let mut app = App::new(test_options(false), &Config::default()); + app.input_history.clear(); app.input = "recover this".to_string(); app.cursor_position = app.input.chars().count();