From 908e1b414a88a710eea5ed4a0b917d93f3aef200 Mon Sep 17 00:00:00 2001 From: asto Date: Thu, 5 Nov 2026 13:40:07 +0800 Subject: [PATCH 1/7] fix(tui): keep the DeepSeek strict pipeline on its documented contract and routes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three conformance fixes, one pipeline. Optional properties no longer manufacture nullable. The strict documents support neither null nor a nullable keyword, so mark_nullable injected an undocumented keyword to keep an optional property accepting omission. The property is removed from the schema instead — preserving the strict contract's all-required plus additionalProperties:false shape — and its name is reported through sanitize_for_strict, which prepare_tools_for_strict_mode surfaces as a description note listing the removed optional parameters. Removed subtrees are not reshaped or reported: a removed property's internals go with it. Non-numeric const lands on the documented enum keyword. The strict documents support const for number/integer schemas only; a string/bool const now becomes enum:[v] instead of shipping an undocumented keyword (numeric const stays put; a non-numeric const beside an existing enum yields to it). default keeps passing through verbatim — the documents show it and list no ban. The whole pipeline now acts only on official DeepSeek-family routes. route_is_official_deepseek_family (config.rs) requires provider typing (Deepseek/DeepseekCN/DeepseekAnthropic) and an api.deepseek.com / api.deepseeki.com host with a strict host-boundary check; the engine's turn surface build and the child sub-agent surface apply it, so a misconfigured provider no longer receives DeepSeek-only reshaping that strips keywords its own documents support (minItems/maxItems on OpenAI). Because the flag flows out of the same surface policy, tool_choice required and the preview surface follow the same gate. The doctor's custom-endpoint verdict flips accordingly (nothing strict is sent to a custom endpoint), and the config field doc states the scope. Signed-off-by: asto --- crates/tui/src/config.rs | 42 +++++- crates/tui/src/config/tests.rs | 62 ++++++++ crates/tui/src/core/engine.rs | 13 +- crates/tui/src/lib.rs | 15 +- crates/tui/src/tools/schema_sanitize.rs | 193 ++++++++++++++++++------ crates/tui/src/tools/subagent/mod.rs | 16 +- crates/tui/src/tools/subagent/tests.rs | 16 +- 7 files changed, 295 insertions(+), 62 deletions(-) diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index d78aea3fc6..fa2623e9f1 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -976,6 +976,40 @@ pub fn requested_model_for_provider(provider: ApiProvider, model: &str) -> Optio } } +/// Whether the route is an official DeepSeek-family endpoint — the only place +/// the DeepSeek strict tool pipeline may act on the wire. +/// +/// The pipeline (`prepare_tools_for_strict_mode` plus +/// `tool_choice: "required"`) encodes DeepSeek's beta strict contract: every +/// property required, `additionalProperties: false`, optional properties +/// removed, `minItems`/`maxItems`/`minLength`/`maxLength` stripped. Applied +/// anywhere else it would strip keywords the other provider's documents +/// explicitly support (OpenAI, for one, documents `minItems`/`maxItems`) and +/// remove optional properties no other provider's contract asks to remove. +/// Provider typing plus the official host keeps a misconfigured +/// `provider`/`base_url` entry from receiving DeepSeek-only reshaping. +/// +/// Source: the DeepSeek strict function-calling/structured-output contract +/// (, fetched +/// 2026-11-05); the official hosts also cover the family's +/// `api.deepseeki.com` alias the rest of the codebase treats as official. +pub fn route_is_official_deepseek_family(provider: ApiProvider, base_url: &str) -> bool { + if !matches!( + provider, + ApiProvider::Deepseek | ApiProvider::DeepseekCN | ApiProvider::DeepseekAnthropic + ) { + return false; + } + let normalized = base_url.trim().trim_end_matches('/').to_ascii_lowercase(); + let official_host = |prefix: &str| { + normalized == prefix + || normalized + .strip_prefix(prefix) + .is_some_and(|rest| rest.starts_with('/')) + }; + official_host("https://api.deepseek.com") || official_host("https://api.deepseeki.com") +} + /// Reject a provider/model tuple that we can be confident is invalid *before* /// it reaches the network (#3227). /// @@ -3058,8 +3092,12 @@ pub struct Config { pub notes_path: Option, pub memory_path: Option, /// 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. + /// schemas into DeepSeek beta strict mode — effective only on official + /// DeepSeek-family routes (api.deepseek.com hosts), because the schema + /// reshaping follows DeepSeek's strict documents and would otherwise + /// strip keywords other providers document (e.g. OpenAI's + /// `minItems`/`maxItems`). Schemas with root alternatives stay non-strict + /// to avoid changing optional/one-of tool semantics. pub strict_tool_mode: Option, /// Additional user-owned system-prompt sources concatenated in declared /// order (#454). Paths are expanded via `expand_path` so `~` and env vars diff --git a/crates/tui/src/config/tests.rs b/crates/tui/src/config/tests.rs index 6f83f477a8..dd5555aa6d 100644 --- a/crates/tui/src/config/tests.rs +++ b/crates/tui/src/config/tests.rs @@ -13701,3 +13701,65 @@ fn picker_and_request_path_disagree_when_the_secret_slot_marker_is_missing() -> println!("CAPTURED first-fix: {:?}", resolution.first_fix()); Ok(()) } + +#[test] +fn strict_pipeline_scope_is_official_deepseek_family_routes() { + // The DeepSeek strict pipeline reshapes schemas per DeepSeek's strict + // documents, so only provider-typed DeepSeek family routes on the + // official hosts may act on it — a misconfigured provider or a lookalike + // host must not receive the reshaping. + for provider in [ + ApiProvider::Deepseek, + ApiProvider::DeepseekCN, + ApiProvider::DeepseekAnthropic, + ] { + for base_url in [ + "https://api.deepseek.com", + "https://api.deepseek.com/", + "https://api.deepseek.com/v1", + "https://api.deepseek.com/beta", + "https://api.deepseek.com/anthropic", + "https://api.deepseeki.com", + "https://api.deepseeki.com/beta", + " HTTPS://API.DEEPSEEK.COM/BETA ", + ] { + assert!( + super::route_is_official_deepseek_family(provider, base_url), + "{provider:?} on {base_url} is an official family route" + ); + } + } + + for (provider, base_url, why) in [ + ( + ApiProvider::Openai, + "https://api.deepseek.com/beta", + "a DeepSeek URL under a non-DeepSeek provider stays out of scope", + ), + ( + ApiProvider::Deepseek, + "https://llm.example.com/v1", + "a custom endpoint is not an official family route", + ), + ( + ApiProvider::Deepseek, + "https://api.deepseek.com.evil.example/beta", + "a lookalike host must not pass the host-boundary check", + ), + ( + ApiProvider::Deepseek, + "http://api.deepseek.com/beta", + "plaintext http is not the official endpoint", + ), + ( + ApiProvider::Moonshot, + "https://api.moonshot.ai/v1", + "unrelated providers stay out of scope", + ), + ] { + assert!( + !super::route_is_official_deepseek_family(provider, base_url), + "{why}: {provider:?} on {base_url}" + ); + } +} diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index f9d0c55661..15a0130d43 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -5718,13 +5718,24 @@ impl Engine { if self.config.features.enabled(Feature::Mcp) { apply_registry_first_shell_guidance(&mut catalog); } + // The strict pipeline encodes DeepSeek's strict-mode contract, so it + // acts only on official DeepSeek-family routes; elsewhere it would + // strip keywords the other provider documents (minItems/maxItems on + // OpenAI) and remove optional properties no other contract asks to + // remove. This gate also drives the turn loop's `tool_choice` and the + // preview surface, which read the flag off this policy. + let strict_tool_mode = self.config.strict_tool_mode + && crate::config::route_is_official_deepseek_family( + route.provider, + &route.api_config.deepseek_base_url(), + ); let surface = ToolSurfacePolicy::new( tool_registry, Some(catalog), input_policy.mode, &always_load, &input_policy.dynamic_active_tools, - self.config.strict_tool_mode, + strict_tool_mode, allowed_tools, self.config.disallowed_tools.clone(), self.config.max_tool_calls, diff --git a/crates/tui/src/lib.rs b/crates/tui/src/lib.rs index 7741a034ea..1faaa5d1d4 100644 --- a/crates/tui/src/lib.rs +++ b/crates/tui/src/lib.rs @@ -7535,8 +7535,10 @@ fn doctor_strict_tool_mode_status(config: &Config) -> DoctorStrictToolModeStatus None => DoctorStrictToolModeStatus { enabled: true, status: "custom_endpoint", - function_strict_sent: true, - message: "enabled; function.strict will be sent to this custom endpoint".to_string(), + function_strict_sent: false, + message: "enabled, but the strict tool pipeline only acts on official DeepSeek \ + routes; this custom endpoint keeps its tools unreshaped" + .to_string(), recommended_base_url: None, }, } @@ -14018,7 +14020,10 @@ mod doctor_endpoint_tests { } #[test] - fn strict_tool_mode_doctor_marks_custom_endpoint_as_forwarded() { + fn strict_tool_mode_doctor_marks_custom_endpoint_as_out_of_scope() { + // The strict pipeline reshapes schemas per DeepSeek's strict-mode + // documents; a custom (non-DeepSeek-family) endpoint keeps its tools + // unreshaped, and nothing is advertised as sent strict. let config = Config { provider: Some("vllm".to_string()), strict_tool_mode: Some(true), @@ -14028,8 +14033,8 @@ mod doctor_endpoint_tests { let status = doctor_strict_tool_mode_status(&config); assert_eq!(status.status, "custom_endpoint"); - assert!(status.function_strict_sent); - assert!(status.message.contains("custom endpoint")); + assert!(!status.function_strict_sent); + assert!(status.message.contains("official DeepSeek")); } #[test] diff --git a/crates/tui/src/tools/schema_sanitize.rs b/crates/tui/src/tools/schema_sanitize.rs index bdc0bda33b..42605b85e2 100644 --- a/crates/tui/src/tools/schema_sanitize.rs +++ b/crates/tui/src/tools/schema_sanitize.rs @@ -46,12 +46,32 @@ pub fn sanitize(schema: &mut Value) { /// /// 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. +/// Optional properties removed to satisfy the strict contract are reported in +/// the tool description so the model knows not to send them. Returns `true` +/// only when every tool in the set can use strict mode. +/// +/// Callers gate this pipeline on official DeepSeek-family routes +/// (`crate::config::route_is_official_deepseek_family`): the reshaping +/// follows DeepSeek's strict-mode documents and would strip keywords other +/// providers document (e.g. `minItems`/`maxItems` on OpenAI). pub fn prepare_tools_for_strict_mode(tools: &mut [Tool]) -> bool { let mut all_strict = true; for tool in tools { if strict_schema_supported(&tool.input_schema) { - sanitize_for_strict(&mut tool.input_schema); + let removed_optionals = sanitize_for_strict(&mut tool.input_schema); + if !removed_optionals.is_empty() { + let note = format!( + "Strict mode: these optional parameters were removed from the schema and \ + must not be sent: {}.", + removed_optionals.join(", ") + ); + let description = tool.description.trim(); + tool.description = if description.is_empty() { + note + } else { + format!("{description}\n\n{note}") + }; + } tool.strict = Some(true); } else { tool.strict = None; @@ -65,10 +85,20 @@ pub fn prepare_tools_for_strict_mode(tools: &mut [Tool]) -> bool { /// /// 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) { +/// property must be listed in `required`. A property the schema does not +/// require cannot satisfy "every property listed in `required`" honestly — +/// marking it `nullable:true` would inject a keyword outside the documented +/// strict subset — so it is removed from `properties` instead, and its name +/// is returned here so the caller can surface the removal. +/// +/// Returns the removed optional property names, sorted and deduplicated. +pub fn sanitize_for_strict(schema: &mut Value) -> Vec { sanitize(schema); - enforce_strict_subset(schema); + let mut removed_optionals = Vec::new(); + enforce_strict_subset(schema, &mut removed_optionals); + removed_optionals.sort(); + removed_optionals.dedup(); + removed_optionals } /// Sanitize a tool `parameters` schema for xAI chat completions. @@ -260,7 +290,7 @@ fn collapse_single_element_unions(schema: &mut Value) { } } -fn enforce_strict_subset(schema: &mut Value) { +fn enforce_strict_subset(schema: &mut Value, removed_optionals: &mut Vec) { if let Some(obj) = schema.as_object_mut() { strip_unsupported_strict_keywords(obj); if is_object_schema(obj) { @@ -268,34 +298,53 @@ fn enforce_strict_subset(schema: &mut Value) { 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); - } + // The strict contract is "every property is required". A property + // the tool left optional would have to carry `nullable: true` to + // keep accepting omission — a keyword outside the documented + // strict subset — so it is removed instead and reported through + // `removed_optionals` (see `sanitize_for_strict`). + let removed_here: Vec = property_names + .iter() + .filter(|name| !originally_required.iter().any(|required| required == *name)) + .cloned() + .collect(); + for name in &removed_here { + properties.shift_remove(name); } + removed_optionals.extend(removed_here); + let mut remaining: Vec = properties.keys().cloned().collect(); + remaining.sort(); obj.insert( "required".into(), - Value::Array(property_names.into_iter().map(Value::String).collect()), + Value::Array(remaining.into_iter().map(Value::String).collect()), ); obj.insert("additionalProperties".into(), Value::Bool(false)); } for value in obj.values_mut() { - enforce_strict_subset(value); + enforce_strict_subset(value, removed_optionals); } } else if let Some(arr) = schema.as_array_mut() { for value in arr { - enforce_strict_subset(value); + enforce_strict_subset(value, removed_optionals); } } } fn strip_unsupported_strict_keywords(obj: &mut Map) { obj.remove("patternProperties"); + // The strict documents support `const` for number/integer schemas only. + // A non-numeric const moves onto the documented `enum` keyword with its + // value as the single member; a numeric const stays put, and a non-numeric + // const beside an existing enum yields to it. `default` stays verbatim + // (the documents show it, and list no ban). + if let Some(const_value) = obj.remove("const") { + if const_value.is_number() { + obj.insert("const".into(), const_value); + } else if !obj.contains_key("enum") { + obj.insert("enum".into(), Value::Array(vec![const_value])); + } + } match obj.get("type").and_then(Value::as_str) { Some("string") => { obj.remove("minLength"); @@ -336,12 +385,6 @@ fn required_names(obj: &Map) -> Vec { .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"] { @@ -670,16 +713,26 @@ mod tests { "additionalProperties": {"type": "string"} }); - sanitize_for_strict(&mut schema); + let removed = sanitize_for_strict(&mut schema); assert_eq!(schema["additionalProperties"], false); - assert_eq!(schema["required"], json!(["count", "name"])); - assert_eq!(schema["properties"]["count"]["nullable"], true); + assert_eq!(schema["required"], json!(["name"])); + assert!( + schema["properties"].get("count").is_none(), + "an optional property is removed, not nullable-marked: {schema}" + ); + assert_eq!(removed, vec!["count".to_string()]); assert!(schema["properties"]["name"].get("nullable").is_none()); } #[test] - fn strict_sanitize_preserves_optional_properties_as_nullable() { + fn strict_sanitize_removes_optional_properties_and_reports_them() { + // The strict contract is "every property is required". The strict + // documents' supported keyword set has no `nullable`, so keeping an + // optional property would need an undocumented keyword to keep + // accepting omission; the property is removed instead and its name + // reported so the caller can tell the model. Nested objects are + // treated the same way, recursively. let mut schema = json!({ "type": "object", "properties": { @@ -698,29 +751,28 @@ mod tests { "required": ["path", "options"] }); - sanitize_for_strict(&mut schema); + let removed = sanitize_for_strict(&mut schema); - assert_eq!( - schema["required"], - json!(["max_lines", "options", "path", "start_line"]) - ); + assert_eq!(removed, vec!["max_lines", "start_line", "trim"]); + assert_eq!(schema["required"], json!(["options", "path"])); + assert!(schema["properties"].get("start_line").is_none()); + assert!(schema["properties"].get("max_lines").is_none()); 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"]) + json!(["encoding"]) + ); + assert!( + schema["properties"]["options"]["properties"] + .get("trim") + .is_none() ); assert!( schema["properties"]["options"]["properties"]["encoding"] .get("nullable") .is_none() ); - assert_eq!( - schema["properties"]["options"]["properties"]["trim"]["nullable"], - true - ); } #[test] @@ -739,16 +791,45 @@ mod tests { "required": [] }); - sanitize_for_strict(&mut schema); + let removed = sanitize_for_strict(&mut schema); - assert_eq!(schema["required"], json!(["outer"])); + // Nothing was required at the root, so `outer` is removed with its + // whole subtree — a removed property's internals go with it, so only + // the name at the level where removal happened is reported. + assert_eq!(removed, vec!["outer"]); + assert_eq!(schema["required"], json!([])); + assert!(schema["properties"].get("outer").is_none()); assert_eq!(schema["additionalProperties"], false); - assert_eq!(schema["properties"]["outer"]["required"], json!(["inner"])); - assert_eq!(schema["properties"]["outer"]["additionalProperties"], false); + } + + #[test] + fn strict_sanitize_lands_non_numeric_const_on_the_documented_enum() { + // The strict documents support `const` for number/integer schemas + // only. A string/bool const moves onto the documented `enum` keyword + // with its value as the single member; a numeric const stays `const`. + let mut schema = json!({ + "type": "object", + "properties": { + "kind": {"type": "string", "const": "fast"}, + "level": {"type": "integer", "const": 3}, + "flag": {"type": "boolean", "const": true} + }, + "required": ["kind", "level", "flag"] + }); + + sanitize_for_strict(&mut schema); + + assert!(schema["properties"]["kind"].get("const").is_none()); + assert_eq!(schema["properties"]["kind"]["enum"], json!(["fast"])); + assert_eq!(schema["properties"]["level"]["const"], 3); + assert!(schema["properties"]["flag"].get("const").is_none()); + assert_eq!(schema["properties"]["flag"]["enum"], json!([true])); } #[test] fn strict_sanitize_removes_unsupported_string_and_array_bounds() { + // Every property is required so the nodes survive the optional-removal + // pass; this test pins the bound keyword strip specifically. let mut schema = json!({ "type": "object", "properties": { @@ -769,7 +850,8 @@ mod tests { "minimum": 1, "maximum": 5 } - } + }, + "required": ["items", "name", "score"] }); sanitize_for_strict(&mut schema); @@ -833,8 +915,19 @@ mod tests { 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); + assert_eq!( + tools[0].input_schema["required"], + json!([]), + "the optional query is removed, not nullable-marked" + ); + assert!(tools[0].input_schema["properties"].get("query").is_none()); + assert!( + tools[0] + .description + .contains("optional parameters were removed"), + "the removal must be surfaced in the tool description: {}", + tools[0].description + ); assert_eq!(tools[1].strict, None); assert!(tools[1].input_schema.get("anyOf").is_some()); assert_eq!(tools[2].strict, None); @@ -917,8 +1010,14 @@ mod tests { 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["required"], json!([])); + assert!(tools[0].input_schema["properties"].get("query").is_none()); assert_eq!(tools[0].input_schema["additionalProperties"], false); + assert!( + tools[0].description.contains("must not be sent: query"), + "the note names the removed optional: {}", + tools[0].description + ); } #[test] diff --git a/crates/tui/src/tools/subagent/mod.rs b/crates/tui/src/tools/subagent/mod.rs index 45e96577cf..d2f2cfb1f6 100644 --- a/crates/tui/src/tools/subagent/mod.rs +++ b/crates/tui/src/tools/subagent/mod.rs @@ -12941,13 +12941,19 @@ async fn run_subagent( messages.push(child_completion_runtime_message(&child_completions)); } + // Same gate as the parent path: the strict pipeline encodes DeepSeek's + // strict-mode contract, so it acts only on official DeepSeek-family + // routes (crate::config::route_is_official_deepseek_family). + let strict_tool_mode = runtime.api_config.as_deref().is_some_and(|config| { + config.strict_tool_mode.unwrap_or(false) + && crate::config::route_is_official_deepseek_family( + config.api_provider(), + &config.deepseek_base_url(), + ) + }); let tools = tool_surface.request_tools( tool_registry.deferred_catalog_for_model(&agent_type), - runtime - .api_config - .as_ref() - .and_then(|config| config.strict_tool_mode) - .unwrap_or(false), + strict_tool_mode, ); let request_active_tool_names = tool_surface.active_names.clone(); let has_tools = !tools.is_empty(); diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index 6c89e4085c..99e09b1834 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -6042,11 +6042,23 @@ fn provider_schema_sanitizers_preserve_the_closed_fleet_role_enum() { "chat completions sanitize must not erase or widen the role enum" ); - // Strict-mode structured outputs pass. + // Strict-mode structured outputs pass. The strict contract removes + // optional properties outright (the Fleet role is optional), so the + // property — and with it the enum — leaves the schema, and + // `prepare_tools_for_strict_mode` reports the removal in the tool + // description. When the role is required, the property survives and its + // enum must be neither erased nor widened. let mut strict = agent_schema.clone(); schema_sanitize::sanitize_for_strict(&mut strict); + assert!( + strict["properties"].get("type").is_none(), + "the strict contract removes the optional role property outright: {strict}" + ); + let mut strict_required = agent_schema.clone(); + strict_required["required"] = json!(["action", "type"]); + schema_sanitize::sanitize_for_strict(&mut strict_required); assert_eq!( - strict["properties"]["type"]["enum"], expected, + strict_required["properties"]["type"]["enum"], expected, "strict-mode sanitize must not erase or widen the role enum" ); From 410a5ca681be674492b94a4db79c38c889b3777e Mon Sep 17 00:00:00 2001 From: asto Date: Thu, 5 Nov 2026 22:54:53 +0800 Subject: [PATCH 2/7] fix(tui): answer the strict-mode doctor from the route gate and pin claimed strict behaviors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes for the DeepSeek strict pipeline PR: - doctor_strict_tool_mode_status now answers from route_is_official_deepseek_family (the same predicate the engine gate uses) instead of classifying the URL alone: an official DeepSeek URL under a non-family provider is reported as the new `provider_mismatch` status (strict_sent=false) instead of a false `ready`, and official-host custom paths — including the DeepseekAnthropic default api.deepseek.com/anthropic endpoint, which the gate reshapes — are reported as the new `official_route` status (strict_sent=true) instead of a false "keeps its tools unreshaped". The doctor icon match covers both statuses. - Correct the config.rs citations: the strict contract lives in the "strict Mode (Beta)" section of api-docs.deepseek.com/guides/tool_calls (guides/structured_output does not exist), and api.deepseeki.com is a legacy typo host kept for config compatibility per docs/REBRAND.md, not an official host. The strict_tool_mode field doc now also states the on-route wire changes (optional-parameter removal + description note, const -> enum). - Pin the two claimed-but-untested behaviors: the removal note replaces an empty tool description, and a non-numeric const yields to an existing enum (agreeing or contradicting). Signed-off-by: asto --- crates/tui/src/config.rs | 25 ++++--- crates/tui/src/lib.rs | 92 +++++++++++++++++++++++-- crates/tui/src/tools/schema_sanitize.rs | 64 +++++++++++++++++ 3 files changed, 166 insertions(+), 15 deletions(-) diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index fa2623e9f1..141b995ce4 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -989,10 +989,13 @@ pub fn requested_model_for_provider(provider: ApiProvider, model: &str) -> Optio /// Provider typing plus the official host keeps a misconfigured /// `provider`/`base_url` entry from receiving DeepSeek-only reshaping. /// -/// Source: the DeepSeek strict function-calling/structured-output contract -/// (, fetched -/// 2026-11-05); the official hosts also cover the family's -/// `api.deepseeki.com` alias the rest of the codebase treats as official. +/// Source: the DeepSeek strict function-calling contract, "`strict` Mode +/// (Beta)" (, fetched +/// 2026-11-05). `api.deepseeki.com` is not an official host — the docs never +/// mention it, and `docs/REBRAND.md` records it as a legacy typo kept only in +/// URL heuristics for existing configs (#1079); it is accepted here to mirror +/// the compat set the rest of the codebase already recognizes +/// (`client.rs`, `client/chat.rs`, the doctor). pub fn route_is_official_deepseek_family(provider: ApiProvider, base_url: &str) -> bool { if !matches!( provider, @@ -3093,11 +3096,15 @@ pub struct Config { pub memory_path: Option, /// When true, set `tool_choice: "required"` and opt compatible function /// schemas into DeepSeek beta strict mode — effective only on official - /// DeepSeek-family routes (api.deepseek.com hosts), because the schema - /// reshaping follows DeepSeek's strict documents and would otherwise - /// strip keywords other providers document (e.g. OpenAI's - /// `minItems`/`maxItems`). Schemas with root alternatives stay non-strict - /// to avoid changing optional/one-of tool semantics. + /// DeepSeek-family routes (provider-typed DeepSeek on the + /// `api.deepseek.com` / legacy `api.deepseeki.com` hosts), because the + /// schema reshaping follows DeepSeek's strict documents and would + /// otherwise strip keywords other providers document (e.g. OpenAI's + /// `minItems`/`maxItems`). On those routes optional parameters are removed + /// from the schema (reported in the tool description) and non-numeric + /// `const` values land on `enum`, per the strict contract. Schemas with + /// root alternatives stay non-strict to avoid changing optional/one-of + /// tool semantics. pub strict_tool_mode: Option, /// Additional user-owned system-prompt sources concatenated in declared /// order (#454). Paths are expanded via `expand_path` so `~` and env vars diff --git a/crates/tui/src/lib.rs b/crates/tui/src/lib.rs index 1faaa5d1d4..ed130f4fe4 100644 --- a/crates/tui/src/lib.rs +++ b/crates/tui/src/lib.rs @@ -4718,8 +4718,10 @@ async fn run_doctor( } let strict_tool_mode = doctor_strict_tool_mode_status(config); let strict_icon = match strict_tool_mode.status { - "ready" => "✓".truecolor(aqua_r, aqua_g, aqua_b), - "fallback_non_beta" | "custom_endpoint" => "!".truecolor(sky_r, sky_g, sky_b), + "ready" | "official_route" => "✓".truecolor(aqua_r, aqua_g, aqua_b), + "fallback_non_beta" | "custom_endpoint" | "provider_mismatch" => { + "!".truecolor(sky_r, sky_g, sky_b) + } _ => "·".dimmed(), }; println!( @@ -7512,6 +7514,35 @@ fn doctor_strict_tool_mode_status(config: &Config) -> DoctorStrictToolModeStatus } let target = doctor_api_target(config); + // Answer from the same predicate the engine gate uses + // (`route_is_official_deepseek_family`): provider typing plus the official + // host. Classifying the URL alone made the Beta arm claim strict was sent + // to a DeepSeek URL under a non-family provider, and made this arm claim + // "unreshaped" for official-host paths (e.g. the Anthropic-family + // endpoint) that the gate actually reshapes. + if !crate::config::route_is_official_deepseek_family(config.api_provider(), &target.base_url) { + if known_deepseek_base_url_kind(&target.base_url).is_some() { + return DoctorStrictToolModeStatus { + enabled: true, + status: "provider_mismatch", + function_strict_sent: false, + message: "enabled, but this route's provider is not the DeepSeek family, so \ + the strict tool pipeline stays off even though the base URL is an \ + official DeepSeek endpoint" + .to_string(), + recommended_base_url: None, + }; + } + return DoctorStrictToolModeStatus { + enabled: true, + status: "custom_endpoint", + function_strict_sent: false, + message: "enabled, but the strict tool pipeline only acts on official DeepSeek \ + routes; this custom endpoint keeps its tools unreshaped" + .to_string(), + recommended_base_url: None, + }; + } match known_deepseek_base_url_kind(&target.base_url) { Some(DeepSeekBaseUrlKind::Beta) => DoctorStrictToolModeStatus { enabled: true, @@ -7532,12 +7563,15 @@ fn doctor_strict_tool_mode_status(config: &Config) -> DoctorStrictToolModeStatus recommended_base_url: Some(recommended.to_string()), } } + // Official host, non-canonical path (the Anthropic-family endpoint, + // /v2, ...): the gate acts here — schemas are reshaped and + // `tool_choice:"required"` is sent — and the client keeps the strict + // flag on official paths it does not canonicalize. None => DoctorStrictToolModeStatus { enabled: true, - status: "custom_endpoint", - function_strict_sent: false, - message: "enabled, but the strict tool pipeline only acts on official DeepSeek \ - routes; this custom endpoint keeps its tools unreshaped" + status: "official_route", + function_strict_sent: true, + message: "enabled; the strict tool pipeline acts on this official DeepSeek route" .to_string(), recommended_base_url: None, }, @@ -14037,6 +14071,52 @@ mod doctor_endpoint_tests { assert!(status.message.contains("official DeepSeek")); } + #[test] + fn strict_tool_mode_doctor_keeps_official_deepseek_urls_out_of_scope_for_other_providers() { + // The engine gate requires DeepSeek-family provider typing, so an + // official DeepSeek URL under another provider stays out of scope and + // must not be reported as "ready". + let config = Config { + provider: Some("openai".to_string()), + strict_tool_mode: Some(true), + providers: Some(crate::config::ProvidersConfig { + openai: crate::config::ProviderConfig { + base_url: Some("https://api.deepseek.com/beta".to_string()), + ..Default::default() + }, + ..Default::default() + }), + ..Default::default() + }; + + let status = doctor_strict_tool_mode_status(&config); + + assert_eq!(status.status, "provider_mismatch"); + assert!(!status.function_strict_sent); + assert!(status.message.contains("not the DeepSeek family")); + assert!(status.recommended_base_url.is_none()); + } + + #[test] + fn strict_tool_mode_doctor_reports_official_custom_paths_as_in_scope() { + // Any path on the official host passes the engine gate, so the + // Anthropic-family default endpoint is in scope: schemas are reshaped + // and `tool_choice:"required"` is sent — the doctor must not claim the + // tools stay unreshaped there. + let config = Config { + provider: Some("deepseek-anthropic".to_string()), + strict_tool_mode: Some(true), + ..Default::default() + }; + + let status = doctor_strict_tool_mode_status(&config); + + assert_eq!(status.status, "official_route"); + assert!(status.function_strict_sent); + assert!(status.message.contains("official DeepSeek route")); + assert!(status.recommended_base_url.is_none()); + } + #[test] fn doctor_tls_status_reports_verification_enabled_by_default() { let status = doctor_tls_status(&Config::default()); diff --git a/crates/tui/src/tools/schema_sanitize.rs b/crates/tui/src/tools/schema_sanitize.rs index 42605b85e2..76c29c68f9 100644 --- a/crates/tui/src/tools/schema_sanitize.rs +++ b/crates/tui/src/tools/schema_sanitize.rs @@ -826,6 +826,34 @@ mod tests { assert_eq!(schema["properties"]["flag"]["enum"], json!([true])); } + #[test] + fn strict_sanitize_non_numeric_const_yields_to_an_existing_enum() { + // A non-numeric const beside an existing enum yields to the enum: the + // const is dropped and the enum passes through verbatim, whether the + // const value agrees with the enum or contradicts it. + let mut schema = json!({ + "type": "object", + "properties": { + "matching": {"type": "string", "const": "fast", "enum": ["fast", "slow"]}, + "contradicting": {"type": "string", "const": "turbo", "enum": ["fast", "slow"]} + }, + "required": ["matching", "contradicting"] + }); + + sanitize_for_strict(&mut schema); + + assert!(schema["properties"]["matching"].get("const").is_none()); + assert_eq!( + schema["properties"]["matching"]["enum"], + json!(["fast", "slow"]) + ); + assert!(schema["properties"]["contradicting"].get("const").is_none()); + assert_eq!( + schema["properties"]["contradicting"]["enum"], + json!(["fast", "slow"]) + ); + } + #[test] fn strict_sanitize_removes_unsupported_string_and_array_bounds() { // Every property is required so the nodes survive the optional-removal @@ -1020,6 +1048,42 @@ mod tests { ); } + #[test] + fn strict_mode_note_replaces_an_empty_tool_description() { + // A tool with no description gets the removal note as its whole + // description instead of an empty prefix. + let mut tools = vec![Tool { + tool_type: None, + name: "lookup".to_string(), + description: String::new(), + 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!( + tools[0].description.starts_with("Strict mode:"), + "the note must replace an empty description: {:?}", + tools[0].description + ); + assert!( + tools[0].description.contains("query"), + "the note names the removed optional: {}", + tools[0].description + ); + } + #[test] fn responses_sanitize_removes_root_composition_from_apply_patch_shape() { let mut schema = json!({ From 10a09d5bb76913bf7e0f82189724f0f534b28e59 Mon Sep 17 00:00:00 2001 From: asto Date: Fri, 6 Nov 2026 01:22:39 +0800 Subject: [PATCH 3/7] fix(tui): close the remaining strict-pipeline conformance gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review found the strict pipeline still shipping undocumented shapes and the doctor still disagreeing with one wire: - Fail closed on required nullables: `has_strict_incompatible_composition` now rejects `nullable` and null-type `anyOf` branches at any depth. `sanitize` collapses the common `X | null` union into `nullable: true`, so the keyword is how a required nullable survives to the gate; such tools ship unchanged and non-strict instead of carrying an undocumented keyword under a strict flag. - Never walk name maps or literals as schemas: `enforce_strict_subset` used to descend into the `properties` map itself, where the keyword strips and object rules rewrote property NAMES — a required property named `const` was renamed to `enum` with a garbage schema while `required` still demanded `const` (every call invalid under `additionalProperties: false`), a `properties` property was erased with spurious keys injected into the name map, and the removal note could name parameters that never existed. Property and definition schemas are now visited through their maps (`properties`, `$defs`, `definitions`), and literal keywords (`enum`, `const`, `default`, `examples`) ship verbatim. - Strip `dependentRequired` like the other undocumented bounds keywords. - Declare the `remember` tool's `note` parameter required — `execute` already rejects its absence for every writing action, and the all-optional schema left strict mode nothing to keep (the tool was callable only with an empty object). - One route composition: new `strict_tool_mode_for_route` answers for the engine turn surface, the child sub-agent surface, and the doctor, so the surfaces cannot disagree. The doctor no longer claims `function.strict` is stripped for the Anthropic-family provider on canonical non-beta endpoints — the Anthropic Messages wire forwards it untranslated — and the out-of-scope messages now state that `tool_choice` falls back to `auto`. - Tests: nullable fail-closed (collapsed and three-branch unions), keyword-named property survival (required + optional), `$defs`/literal walking, `dependentRequired` strip, cross-level dedupe, the route composition, the engine turn-surface wiring across three route quadrants, and the Anthropic forwarding verdict. Local: `schema_sanitize` 73, `strict*` 70, `strict_tool_mode_doctor` 8, `tools::remember` 9 — all green; fmt and clippy clean under the CI allowlist. Signed-off-by: asto --- crates/tui/src/config.rs | 23 +- crates/tui/src/config/tests.rs | 27 +++ crates/tui/src/core/engine.rs | 13 +- crates/tui/src/core/engine/tests.rs | 136 ++++++++++++ crates/tui/src/lib.rs | 70 +++++- crates/tui/src/tools/remember.rs | 7 +- crates/tui/src/tools/schema_sanitize.rs | 270 +++++++++++++++++++++++- crates/tui/src/tools/subagent/mod.rs | 12 +- 8 files changed, 529 insertions(+), 29 deletions(-) diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index 141b995ce4..7eeaf1de60 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -982,7 +982,9 @@ pub fn requested_model_for_provider(provider: ApiProvider, model: &str) -> Optio /// The pipeline (`prepare_tools_for_strict_mode` plus /// `tool_choice: "required"`) encodes DeepSeek's beta strict contract: every /// property required, `additionalProperties: false`, optional properties -/// removed, `minItems`/`maxItems`/`minLength`/`maxLength` stripped. Applied +/// removed, `minItems`/`maxItems`/`minLength`/`maxLength`/`dependentRequired` +/// stripped, and schemas carrying undocumented `nullable`/null-type union +/// shapes left non-strict. Applied /// anywhere else it would strip keywords the other provider's documents /// explicitly support (OpenAI, for one, documents `minItems`/`maxItems`) and /// remove optional properties no other provider's contract asks to remove. @@ -1013,6 +1015,15 @@ pub fn route_is_official_deepseek_family(provider: ApiProvider, base_url: &str) official_host("https://api.deepseek.com") || official_host("https://api.deepseeki.com") } +/// Whether the DeepSeek strict tool pipeline (schema reshaping plus +/// `tool_choice: "required"`) acts on a route: the user flag AND the +/// official-route gate. The engine turn-surface build, the child sub-agent +/// surface, and the doctor all answer from this one composition so the +/// surfaces can never disagree about where strict mode is in scope. +pub fn strict_tool_mode_for_route(flag: bool, provider: ApiProvider, base_url: &str) -> bool { + flag && route_is_official_deepseek_family(provider, base_url) +} + /// Reject a provider/model tuple that we can be confident is invalid *before* /// it reaches the network (#3227). /// @@ -3101,10 +3112,12 @@ pub struct Config { /// schema reshaping follows DeepSeek's strict documents and would /// otherwise strip keywords other providers document (e.g. OpenAI's /// `minItems`/`maxItems`). On those routes optional parameters are removed - /// from the schema (reported in the tool description) and non-numeric - /// `const` values land on `enum`, per the strict contract. Schemas with - /// root alternatives stay non-strict to avoid changing optional/one-of - /// tool semantics. + /// from the schema (reported in the tool description), non-numeric + /// `const` values land on `enum`, and `dependentRequired` tables are + /// stripped, per the strict contract. Schemas the strict subset cannot + /// represent faithfully — root alternatives, or required nullables + /// (`nullable` / null-type union branches) — stay non-strict: they keep + /// their original schema and `tool_choice` falls back to `auto`. pub strict_tool_mode: Option, /// Additional user-owned system-prompt sources concatenated in declared /// order (#454). Paths are expanded via `expand_path` so `~` and env vars diff --git a/crates/tui/src/config/tests.rs b/crates/tui/src/config/tests.rs index dd5555aa6d..20048a43d8 100644 --- a/crates/tui/src/config/tests.rs +++ b/crates/tui/src/config/tests.rs @@ -13763,3 +13763,30 @@ fn strict_pipeline_scope_is_official_deepseek_family_routes() { ); } } + +#[test] +fn strict_tool_mode_for_route_combines_the_flag_with_the_official_route_gate() { + // One composition answers for the engine turn surface, the child + // sub-agent surface, and the doctor — the user flag alone must never + // enable the pipeline, and an official URL alone must not either. + assert!(super::strict_tool_mode_for_route( + true, + ApiProvider::Deepseek, + "https://api.deepseek.com/beta" + )); + assert!(!super::strict_tool_mode_for_route( + false, + ApiProvider::Deepseek, + "https://api.deepseek.com/beta" + )); + assert!(!super::strict_tool_mode_for_route( + true, + ApiProvider::Openai, + "https://api.deepseek.com/beta" + )); + assert!(!super::strict_tool_mode_for_route( + true, + ApiProvider::Deepseek, + "https://llm.example.com/v1" + )); +} diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index 15a0130d43..94aa879af7 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -491,7 +491,8 @@ pub struct EngineConfig { /// construction; the engine never touches disk for it. pub locale_tag: String, /// When true, force `tool_choice: "required"` and opt compatible function - /// schemas into DeepSeek beta strict mode. + /// schemas into DeepSeek beta strict mode — on official DeepSeek-family + /// routes only (`crate::config::strict_tool_mode_for_route`). pub strict_tool_mode: bool, /// Workshop / large-tool-output routing (#548). `None` disables routing. pub workshop: Option, @@ -5724,11 +5725,11 @@ impl Engine { // OpenAI) and remove optional properties no other contract asks to // remove. This gate also drives the turn loop's `tool_choice` and the // preview surface, which read the flag off this policy. - let strict_tool_mode = self.config.strict_tool_mode - && crate::config::route_is_official_deepseek_family( - route.provider, - &route.api_config.deepseek_base_url(), - ); + let strict_tool_mode = crate::config::strict_tool_mode_for_route( + self.config.strict_tool_mode, + route.provider, + &route.api_config.deepseek_base_url(), + ); let surface = ToolSurfacePolicy::new( tool_registry, Some(catalog), diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index 35ff93980d..2deb9cfc05 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -36355,3 +36355,139 @@ fn run_completion_real_factories_observe_shutdown_and_direct_handles_do_not() { ); }); } + +#[tokio::test] +#[allow(clippy::await_holding_lock)] +async fn turn_surface_strict_tool_mode_follows_the_official_route_gate() { + // The turn surface must receive the strict flag only through the + // official-route gate. Dropping that gate would re-expose the pre-gate + // behavior where any provider with `strict_tool_mode` on received the + // DeepSeek reshaping and `tool_choice:"required"` on every route. + let _env_lock = lock_test_env(); + let tmp = tempdir().expect("tempdir"); + let home = tmp.path().join("home"); + fs::create_dir_all(&home).expect("create isolated home"); + let _home = EnvVarGuard::set("HOME", &home); + let _userprofile = EnvVarGuard::set("USERPROFILE", &home); + let _codewhale_home = EnvVarGuard::set("CODEWHALE_HOME", home.join(".codewhale")); + let workspace = tmp.path().join("workspace"); + fs::create_dir_all(&workspace).expect("create workspace"); + + let strict_api_config = Config { + api_key: Some("strict-gate-fixture".to_string()), + default_text_model: Some(DEFAULT_TEXT_MODEL.to_string()), + strict_tool_mode: Some(true), + ..Config::default() + }; + let (mut engine, _engine_handle) = Engine::new( + EngineConfig { + workspace, + allow_shell: true, + strict_tool_mode: true, + ..EngineConfig::default() + }, + &strict_api_config, + ); + engine.config.features.disable(Feature::Mcp); + let client = engine.deepseek_client.clone(); + let locale_tag = engine.config.locale_tag.clone(); + let role_models = engine.subagent_role_models(); + let authority = crate::core::authority::TurnAuthority::from_effective_fields( + AppMode::Agent, + true, + false, + false, + crate::tui::approval::ApprovalMode::Suggest, + ); + let route_with = |provider: ApiProvider, api_config: Config| TurnRouteContext { + provider, + model: DEFAULT_TEXT_MODEL.to_string(), + capabilities: codewhale_config::route::RouteCapabilities::default(), + limits: None, + client: client.clone(), + api_config: Box::new(api_config), + locale_tag: locale_tag.clone(), + role_models: role_models.clone(), + auto_model: false, + reasoning_effort: None, + reasoning_effort_auto: false, + }; + + // Family provider on the official endpoint: in scope. + let build = engine + .build_turn_tool_registry_and_catalog( + &authority, + &[], + None, + SubAgentWiring::Inert, + McpAccess::PassiveSnapshot, + route_with(ApiProvider::Deepseek, strict_api_config.clone()), + "", + ) + .await; + assert!( + build.surface.strict_tool_mode, + "the official family route is in scope" + ); + + // The same family provider on a custom endpoint: the gate keeps the + // pipeline off — no reshaping, no `tool_choice:"required"`. + let custom = Config { + api_key: Some("strict-gate-fixture".to_string()), + default_text_model: Some(DEFAULT_TEXT_MODEL.to_string()), + strict_tool_mode: Some(true), + providers: Some(crate::config::ProvidersConfig { + deepseek: crate::config::ProviderConfig { + base_url: Some("https://llm.example.com/v1".to_string()), + ..Default::default() + }, + ..Default::default() + }), + ..Config::default() + }; + let build = engine + .build_turn_tool_registry_and_catalog( + &authority, + &[], + None, + SubAgentWiring::Inert, + McpAccess::PassiveSnapshot, + route_with(ApiProvider::Deepseek, custom), + "", + ) + .await; + assert!( + !build.surface.strict_tool_mode, + "a family provider on a custom endpoint stays out of scope" + ); + + // An official DeepSeek URL under a non-family provider: out of scope. + let openai_on_official = Config { + api_key: Some("strict-gate-fixture".to_string()), + default_text_model: Some(DEFAULT_TEXT_MODEL.to_string()), + strict_tool_mode: Some(true), + providers: Some(crate::config::ProvidersConfig { + openai: crate::config::ProviderConfig { + base_url: Some("https://api.deepseek.com/beta".to_string()), + ..Default::default() + }, + ..Default::default() + }), + ..Config::default() + }; + let build = engine + .build_turn_tool_registry_and_catalog( + &authority, + &[], + None, + SubAgentWiring::Inert, + McpAccess::PassiveSnapshot, + route_with(ApiProvider::Openai, openai_on_official), + "", + ) + .await; + assert!( + !build.surface.strict_tool_mode, + "a non-family provider on an official URL stays out of scope" + ); +} diff --git a/crates/tui/src/lib.rs b/crates/tui/src/lib.rs index ed130f4fe4..29415b4a73 100644 --- a/crates/tui/src/lib.rs +++ b/crates/tui/src/lib.rs @@ -7514,13 +7514,17 @@ fn doctor_strict_tool_mode_status(config: &Config) -> DoctorStrictToolModeStatus } let target = doctor_api_target(config); - // Answer from the same predicate the engine gate uses - // (`route_is_official_deepseek_family`): provider typing plus the official - // host. Classifying the URL alone made the Beta arm claim strict was sent - // to a DeepSeek URL under a non-family provider, and made this arm claim - // "unreshaped" for official-host paths (e.g. the Anthropic-family - // endpoint) that the gate actually reshapes. - if !crate::config::route_is_official_deepseek_family(config.api_provider(), &target.base_url) { + // Answer from the same composition the engine and sub-agent gates use + // (`strict_tool_mode_for_route`): the flag plus provider typing plus the + // official host. Classifying the URL alone made the Beta arm claim strict + // was sent to a DeepSeek URL under a non-family provider, and made this + // arm claim "unreshaped" for official-host paths (e.g. the + // Anthropic-family endpoint) that the gate actually reshapes. + if !crate::config::strict_tool_mode_for_route( + config.strict_tool_mode.unwrap_or(false), + config.api_provider(), + &target.base_url, + ) { if known_deepseek_base_url_kind(&target.base_url).is_some() { return DoctorStrictToolModeStatus { enabled: true, @@ -7528,7 +7532,8 @@ fn doctor_strict_tool_mode_status(config: &Config) -> DoctorStrictToolModeStatus function_strict_sent: false, message: "enabled, but this route's provider is not the DeepSeek family, so \ the strict tool pipeline stays off even though the base URL is an \ - official DeepSeek endpoint" + official DeepSeek endpoint; tools keep their schemas and \ + `tool_choice` falls back to `auto`" .to_string(), recommended_base_url: None, }; @@ -7538,7 +7543,8 @@ fn doctor_strict_tool_mode_status(config: &Config) -> DoctorStrictToolModeStatus status: "custom_endpoint", function_strict_sent: false, message: "enabled, but the strict tool pipeline only acts on official DeepSeek \ - routes; this custom endpoint keeps its tools unreshaped" + routes; this custom endpoint keeps its tools unreshaped and \ + `tool_choice` falls back to `auto`" .to_string(), recommended_base_url: None, }; @@ -7552,6 +7558,24 @@ fn doctor_strict_tool_mode_status(config: &Config) -> DoctorStrictToolModeStatus recommended_base_url: None, }, Some(DeepSeekBaseUrlKind::NonBeta) => { + // The chat-completions wire strips the strict flag on the + // canonical non-beta endpoints, but the Anthropic Messages wire + // has no such strip — `client/anthropic.rs` forwards + // `function.strict` untranslated — so for the Anthropic-family + // provider the flag IS sent, and the beta-endpoint + // recommendation would point an Anthropic-dialect route at a + // chat-completions surface it must not use. + if config.api_provider() == crate::config::ApiProvider::DeepseekAnthropic { + return DoctorStrictToolModeStatus { + enabled: true, + status: "official_route", + function_strict_sent: true, + message: "enabled; the Anthropic wire forwards function.strict \ + untranslated on this official DeepSeek endpoint" + .to_string(), + recommended_base_url: None, + }; + } let recommended = recommended_strict_base_url(config, &target.base_url); DoctorStrictToolModeStatus { enabled: true, @@ -14117,6 +14141,34 @@ mod doctor_endpoint_tests { assert!(status.recommended_base_url.is_none()); } + #[test] + fn strict_tool_mode_doctor_reports_anthropic_forwarding_on_non_beta_canonical_endpoints() { + // The chat wire strips `function.strict` on the canonical non-beta + // endpoints, but the Anthropic Messages wire forwards it + // untranslated — the doctor must not claim the flag is stripped for + // the Anthropic-family provider, and must not recommend pointing an + // Anthropic-dialect route at the chat-completions beta surface. + let config = Config { + provider: Some("deepseek-anthropic".to_string()), + strict_tool_mode: Some(true), + providers: Some(crate::config::ProvidersConfig { + deepseek_anthropic: crate::config::ProviderConfig { + base_url: Some("https://api.deepseek.com".to_string()), + ..Default::default() + }, + ..Default::default() + }), + ..Default::default() + }; + + let status = doctor_strict_tool_mode_status(&config); + + assert_eq!(status.status, "official_route"); + assert!(status.function_strict_sent); + assert!(status.message.contains("Anthropic wire forwards")); + assert!(status.recommended_base_url.is_none()); + } + #[test] fn doctor_tls_status_reports_verification_enabled_by_default() { let status = doctor_tls_status(&Config::default()); diff --git a/crates/tui/src/tools/remember.rs b/crates/tui/src/tools/remember.rs index d53e6ea728..cc19865c2f 100644 --- a/crates/tui/src/tools/remember.rs +++ b/crates/tui/src/tools/remember.rs @@ -75,7 +75,12 @@ impl ToolSpec for RememberTool { "description": "Native backend scope; defaults to global. workspace requires a git repository with an origin." } }, - "required": [] + // `note` is required at runtime for every writing action + // (`execute` rejects its absence), so the schema says so. An + // all-optional schema would leave strict mode nothing to keep: + // DeepSeek's strict contract removes optional properties, which + // would leave this tool callable only with an empty object. + "required": ["note"] }) } diff --git a/crates/tui/src/tools/schema_sanitize.rs b/crates/tui/src/tools/schema_sanitize.rs index 76c29c68f9..92a8ec88b7 100644 --- a/crates/tui/src/tools/schema_sanitize.rs +++ b/crates/tui/src/tools/schema_sanitize.rs @@ -50,6 +50,14 @@ pub fn sanitize(schema: &mut Value) { /// the tool description so the model knows not to send them. Returns `true` /// only when every tool in the set can use strict mode. /// +/// Incompatible includes schemas the documented strict subset cannot +/// represent faithfully: root alternatives, and required nullables (`nullable` +/// or a null-type union branch — `sanitize` collapses the common `X | null` +/// union into `nullable: true`, so the keyword is how a required nullable +/// survives to the gate). Such a tool keeps its original schema and ships +/// non-strict rather than carrying an undocumented keyword under a strict +/// flag. +/// /// Callers gate this pipeline on official DeepSeek-family routes /// (`crate::config::route_is_official_deepseek_family`): the reshaping /// follows DeepSeek's strict-mode documents and would strip keywords other @@ -164,12 +172,31 @@ fn has_strict_incompatible_composition(schema: &Value, is_root: bool) -> bool { if obj.contains_key("oneOf") || obj.contains_key("allOf") || obj.contains_key("dependentSchemas") + // `nullable` is outside the documented strict subset — the + // supported-type list has no null and the documents never + // mention the keyword. `sanitize` collapses the common + // `X | null` union into `nullable: true`, so by the time a + // schema reaches here the keyword is the surviving shape of a + // required nullable; such a tool falls back to non-strict + // instead of shipping an undocumented keyword under a strict + // flag. + || obj.contains_key("nullable") { return true; } if is_root && obj.contains_key("anyOf") { return true; } + // A union that still carries a null branch after the collapse pass + // (three or more alternatives) has the same undocumented-null + // problem. + if obj + .get("anyOf") + .and_then(Value::as_array) + .is_some_and(|branches| branches.iter().any(is_null_type)) + { + return true; + } return obj .values() .any(|value| has_strict_incompatible_composition(value, false)); @@ -321,8 +348,27 @@ fn enforce_strict_subset(schema: &mut Value, removed_optionals: &mut Vec obj.insert("additionalProperties".into(), Value::Bool(false)); } - for value in obj.values_mut() { - enforce_strict_subset(value, removed_optionals); + // Recurse into sub-schemas, but never into the map-shaped values + // that hold named schemas or literal values. Walking those as if + // they were schemas applied the keyword strips and the object rules + // to property/definition NAMES: a tool with a property named + // `const`, `properties`, or `patternProperties` came out corrupted + // (renamed, erased, or dangling in `required`) while marked strict. + // Property and definition schemas are visited through their name + // maps, and the literal keywords are left alone — their members are + // values, not schemas. + for (key, value) in obj.iter_mut() { + match key.as_str() { + "properties" | "$defs" | "definitions" => { + if let Some(named_schemas) = value.as_object_mut() { + for named_schema in named_schemas.values_mut() { + enforce_strict_subset(named_schema, removed_optionals); + } + } + } + "enum" | "const" | "default" | "examples" => {} + _ => enforce_strict_subset(value, removed_optionals), + } } } else if let Some(arr) = schema.as_array_mut() { for value in arr { @@ -333,6 +379,9 @@ fn enforce_strict_subset(schema: &mut Value, removed_optionals: &mut Vec fn strip_unsupported_strict_keywords(obj: &mut Map) { obj.remove("patternProperties"); + // Conditional dependency tables are likewise outside the documented + // strict subset; dropping one only widens what the endpoint accepts. + obj.remove("dependentRequired"); // The strict documents support `const` for number/integer schemas only. // A non-numeric const moves onto the documented `enum` keyword with its // value as the single member; a numeric const stays put, and a non-numeric @@ -1084,6 +1133,223 @@ mod tests { ); } + #[test] + fn strict_mode_fails_closed_on_required_nullable_unions() { + // `nullable` is outside the documented strict subset (the supported + // type list has no null and the documents never mention the keyword), + // and `sanitize` collapses the common `X | null` union into + // `nullable: true`. A schema that still carries the keyword after + // that collapse — i.e. a required nullable — cannot be represented + // faithfully under a strict flag, so the tool ships unchanged and + // non-strict instead of inviting the server 400 the pipeline exists + // to prevent. + let mut tools = vec![Tool { + tool_type: None, + name: "search".to_string(), + description: "Search".to_string(), + input_schema: json!({ + "type": "object", + "properties": { + "query": {"type": "string"}, + "mode": {"anyOf": [{"type": "string"}, {"type": "null"}]}, + "limit": { + "anyOf": [ + {"type": "integer"}, + {"type": "string"}, + {"type": "null"} + ] + } + }, + "required": ["query", "mode", "limit"] + }), + 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); + assert_eq!(tools[0].description, "Search"); + assert_eq!( + tools[0].input_schema["properties"]["mode"]["anyOf"], + json!([{"type": "string"}, {"type": "null"}]) + ); + assert_eq!( + tools[0].input_schema["properties"]["limit"]["anyOf"], + json!([ + {"type": "integer"}, + {"type": "string"}, + {"type": "null"} + ]) + ); + assert_eq!( + tools[0].input_schema["required"], + json!(["query", "mode", "limit"]) + ); + } + + #[test] + fn strict_sanitize_keeps_properties_named_after_schema_keywords_intact() { + // The recursion must treat the `properties` value as a name map, not + // as a schema: walking it as a schema applied the keyword strips and + // the object rules to property NAMES, renaming a required `const` + // property to `enum`, erasing a `properties` property's schema, and + // dangling `patternProperties` in `required` — all while marking the + // tool strict. Required keyword-named properties survive intact; the + // strict object rules land on the tool schema and on each property's + // own sub-schema, never on the name map. + let mut schema = json!({ + "type": "object", + "properties": { + "const": {"type": "string"}, + "properties": {"type": "object"}, + "patternProperties": {"type": "object"}, + "type": {"type": "string"} + }, + "required": ["const", "patternProperties", "properties", "type"] + }); + + let removed = sanitize_for_strict(&mut schema); + + assert!(removed.is_empty()); + assert_eq!(schema["properties"]["const"], json!({"type": "string"})); + assert_eq!(schema["properties"]["type"], json!({"type": "string"})); + assert_eq!( + schema["properties"]["properties"]["type"], + json!("object"), + "the property's own schema keeps its type keyword" + ); + assert_eq!( + schema["properties"]["properties"]["additionalProperties"], false, + "the strict object rules apply to the property schema itself" + ); + assert_eq!( + schema["properties"]["patternProperties"]["type"], + json!("object") + ); + assert_eq!( + schema["required"], + json!(["const", "patternProperties", "properties", "type"]) + ); + assert_eq!(schema["additionalProperties"], false); + assert!( + schema["properties"].get("required").is_none() + && schema["properties"].get("additionalProperties").is_none(), + "the name map gains no spurious schema keys: {schema}" + ); + } + + #[test] + fn strict_sanitize_reports_an_optional_property_named_properties() { + // An optional keyword-named property is removed and reported like any + // other optional, and the name map stays clean. + let mut schema = json!({ + "type": "object", + "properties": { + "query": {"type": "string"}, + "properties": {"type": "object"} + }, + "required": ["query"] + }); + + let removed = sanitize_for_strict(&mut schema); + + assert_eq!(removed, vec!["properties".to_string()]); + assert!(schema["properties"].get("properties").is_none()); + assert_eq!(schema["properties"]["query"], json!({"type": "string"})); + assert_eq!(schema["required"], json!(["query"])); + assert!(schema["properties"].get("required").is_none()); + } + + #[test] + fn strict_sanitize_walks_schemas_but_never_name_maps_or_literals() { + // `$defs` entries are schemas and keep getting the strict rules, but + // a definition named after a keyword must not corrupt the map, and + // `enum`/`default` members are values, not schemas — they ship + // verbatim. + let mut schema = json!({ + "type": "object", + "properties": { + "mode": { + "type": "string", + "enum": [ + {"type": "object", "properties": {"a": {"type": "string"}}} + ], + "default": {"const": 1} + } + }, + "required": ["mode"], + "$defs": { + "properties": {"type": "string"}, + "span": { + "type": "object", + "properties": {"b": {"type": "integer"}}, + "required": ["b"] + } + } + }); + + let removed = sanitize_for_strict(&mut schema); + + assert!(removed.is_empty()); + assert_eq!( + schema["properties"]["mode"]["enum"], + json!([{"type": "object", "properties": {"a": {"type": "string"}}}]) + ); + assert_eq!(schema["properties"]["mode"]["default"], json!({"const": 1})); + assert_eq!(schema["$defs"]["properties"], json!({"type": "string"})); + assert_eq!(schema["$defs"]["span"]["additionalProperties"], false); + assert_eq!(schema["$defs"]["span"]["required"], json!(["b"])); + } + + #[test] + fn strict_sanitize_strips_dependent_required_tables() { + // Conditional dependency tables are outside the documented strict + // subset; stripping one only widens what the endpoint accepts, the + // same treatment as `patternProperties` and the length bounds. + let mut schema = json!({ + "type": "object", + "properties": { + "a": {"type": "string"}, + "b": {"type": "string"} + }, + "required": ["a"], + "dependentRequired": {"a": ["b"]} + }); + + let removed = sanitize_for_strict(&mut schema); + + assert_eq!(removed, vec!["b".to_string()]); + assert!(schema.get("dependentRequired").is_none()); + assert_eq!(schema["required"], json!(["a"])); + } + + #[test] + fn strict_sanitize_dedupes_optionals_shared_across_levels() { + // The same optional name at two levels is reported once. + let mut schema = json!({ + "type": "object", + "properties": { + "trim": {"type": "boolean"}, + "outer": { + "type": "object", + "properties": { + "trim": {"type": "boolean"}, + "alpha": {"type": "string"} + }, + "required": ["alpha"] + } + }, + "required": ["outer"] + }); + + let removed = sanitize_for_strict(&mut schema); + + assert_eq!(removed, vec!["trim".to_string()]); + } + #[test] fn responses_sanitize_removes_root_composition_from_apply_patch_shape() { let mut schema = json!({ diff --git a/crates/tui/src/tools/subagent/mod.rs b/crates/tui/src/tools/subagent/mod.rs index d2f2cfb1f6..019da06626 100644 --- a/crates/tui/src/tools/subagent/mod.rs +++ b/crates/tui/src/tools/subagent/mod.rs @@ -12943,13 +12943,13 @@ async fn run_subagent( // Same gate as the parent path: the strict pipeline encodes DeepSeek's // strict-mode contract, so it acts only on official DeepSeek-family - // routes (crate::config::route_is_official_deepseek_family). + // routes (crate::config::strict_tool_mode_for_route). let strict_tool_mode = runtime.api_config.as_deref().is_some_and(|config| { - config.strict_tool_mode.unwrap_or(false) - && crate::config::route_is_official_deepseek_family( - config.api_provider(), - &config.deepseek_base_url(), - ) + crate::config::strict_tool_mode_for_route( + config.strict_tool_mode.unwrap_or(false), + config.api_provider(), + &config.deepseek_base_url(), + ) }); let tools = tool_surface.request_tools( tool_registry.deferred_catalog_for_model(&agent_type), From 76fefa87ab4ef1e7d7c70e3425e683c6a9bb987b Mon Sep 17 00:00:00 2001 From: asto Date: Fri, 6 Nov 2026 17:06:46 +0800 Subject: [PATCH 4/7] fix(tui): fail strict mode closed on unusable and unrepresentable schemas The strict pipeline now fails a tool closed instead of shipping it strict when the documented subset cannot serve it faithfully: - optional removal that would strip every declared parameter (an all-optional root left action-discriminated tools callable only with an empty object while their runtime still demands the values) - type arrays and bare null types, null-carrying const/enum members - the structural/conditional keyword family the Moonshot MFJS path already rejects (not, if/then/else, contains, prefixItems, propertyNames, unevaluated*, tuple-form items) Draft-7 dependencies, uniqueItems, and the property/containment bounds strip like the other widening-only bounds; the documents' singular $def spelling is walked as a name map; a null const drops outright instead of landing on enum. The route gate no longer trims whitespace, matching the doctor classifier and the wire strip sets, so a padded URL degrades to out-of-scope instead of leaking the flag past the non-beta strip. The doctor answers the Anthropic family from the wire on every official path (beta included) instead of the chat-dialect ready verdict, names https in the custom-endpoint message, and calls the deepseeki host a recognized URL rather than an official endpoint. The remember schema comment states the retire exception instead of claiming note is required for every writing action. Signed-off-by: asto --- crates/tui/src/config.rs | 7 +- crates/tui/src/config/tests.rs | 8 +- crates/tui/src/lib.rs | 86 +++-- crates/tui/src/tools/remember.rs | 11 +- crates/tui/src/tools/schema_sanitize.rs | 415 ++++++++++++++++++++++-- 5 files changed, 474 insertions(+), 53 deletions(-) diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index 7eeaf1de60..bc28f060a1 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -1005,7 +1005,12 @@ pub fn route_is_official_deepseek_family(provider: ApiProvider, base_url: &str) ) { return false; } - let normalized = base_url.trim().trim_end_matches('/').to_ascii_lowercase(); + // No whitespace trimming: the doctor's URL classifier and the client's + // strict-host sets string-match the same URL without trimming, so + // admitting a padded URL here alone would put the pipeline in scope on + // a route the wire does not canonicalize — the flag would leak past the + // non-beta strip. A padded URL degrades to out-of-scope instead. + let normalized = base_url.trim_end_matches('/').to_ascii_lowercase(); let official_host = |prefix: &str| { normalized == prefix || normalized diff --git a/crates/tui/src/config/tests.rs b/crates/tui/src/config/tests.rs index 20048a43d8..68085f7a9e 100644 --- a/crates/tui/src/config/tests.rs +++ b/crates/tui/src/config/tests.rs @@ -13721,7 +13721,6 @@ fn strict_pipeline_scope_is_official_deepseek_family_routes() { "https://api.deepseek.com/anthropic", "https://api.deepseeki.com", "https://api.deepseeki.com/beta", - " HTTPS://API.DEEPSEEK.COM/BETA ", ] { assert!( super::route_is_official_deepseek_family(provider, base_url), @@ -13756,6 +13755,13 @@ fn strict_pipeline_scope_is_official_deepseek_family_routes() { "https://api.moonshot.ai/v1", "unrelated providers stay out of scope", ), + ( + ApiProvider::Deepseek, + " HTTPS://API.DEEPSEEK.COM/BETA ", + "a whitespace-padded URL matches no surface that string-compares the base URL \ + (this gate, the doctor classifier, the wire strip set), so admitting it here \ + alone would leak the strict flag past the non-beta strip; it stays out of scope", + ), ] { assert!( !super::route_is_official_deepseek_family(provider, base_url), diff --git a/crates/tui/src/lib.rs b/crates/tui/src/lib.rs index 29415b4a73..a6fbfc5906 100644 --- a/crates/tui/src/lib.rs +++ b/crates/tui/src/lib.rs @@ -7531,8 +7531,8 @@ fn doctor_strict_tool_mode_status(config: &Config) -> DoctorStrictToolModeStatus status: "provider_mismatch", function_strict_sent: false, message: "enabled, but this route's provider is not the DeepSeek family, so \ - the strict tool pipeline stays off even though the base URL is an \ - official DeepSeek endpoint; tools keep their schemas and \ + the strict tool pipeline stays off even though the base URL is a \ + recognized DeepSeek endpoint URL; tools keep their schemas and \ `tool_choice` falls back to `auto`" .to_string(), recommended_base_url: None, @@ -7543,12 +7543,29 @@ fn doctor_strict_tool_mode_status(config: &Config) -> DoctorStrictToolModeStatus status: "custom_endpoint", function_strict_sent: false, message: "enabled, but the strict tool pipeline only acts on official DeepSeek \ - routes; this custom endpoint keeps its tools unreshaped and \ + https endpoints; this custom endpoint keeps its tools unreshaped and \ `tool_choice` falls back to `auto`" .to_string(), recommended_base_url: None, }; } + // The Anthropic Messages wire forwards `function.strict` untranslated on + // every official path — the chat-completions strip does not apply there — + // while the beta message below describes a chat-completions surface an + // Anthropic-dialect route must not be pointed at. Answer that provider + // from the wire, not from the URL kind: the flag IS sent, and the + // beta/non-beta recommendation is withheld for the dialect. + if config.api_provider() == crate::config::ApiProvider::DeepseekAnthropic { + return DoctorStrictToolModeStatus { + enabled: true, + status: "official_route", + function_strict_sent: true, + message: "enabled; the Anthropic wire forwards function.strict \ + untranslated on this official DeepSeek endpoint" + .to_string(), + recommended_base_url: None, + }; + } match known_deepseek_base_url_kind(&target.base_url) { Some(DeepSeekBaseUrlKind::Beta) => DoctorStrictToolModeStatus { enabled: true, @@ -7558,24 +7575,6 @@ fn doctor_strict_tool_mode_status(config: &Config) -> DoctorStrictToolModeStatus recommended_base_url: None, }, Some(DeepSeekBaseUrlKind::NonBeta) => { - // The chat-completions wire strips the strict flag on the - // canonical non-beta endpoints, but the Anthropic Messages wire - // has no such strip — `client/anthropic.rs` forwards - // `function.strict` untranslated — so for the Anthropic-family - // provider the flag IS sent, and the beta-endpoint - // recommendation would point an Anthropic-dialect route at a - // chat-completions surface it must not use. - if config.api_provider() == crate::config::ApiProvider::DeepseekAnthropic { - return DoctorStrictToolModeStatus { - enabled: true, - status: "official_route", - function_strict_sent: true, - message: "enabled; the Anthropic wire forwards function.strict \ - untranslated on this official DeepSeek endpoint" - .to_string(), - recommended_base_url: None, - }; - } let recommended = recommended_strict_base_url(config, &target.base_url); DoctorStrictToolModeStatus { enabled: true, @@ -14123,13 +14122,20 @@ mod doctor_endpoint_tests { #[test] fn strict_tool_mode_doctor_reports_official_custom_paths_as_in_scope() { - // Any path on the official host passes the engine gate, so the - // Anthropic-family default endpoint is in scope: schemas are reshaped - // and `tool_choice:"required"` is sent — the doctor must not claim the - // tools stay unreshaped there. + // Any path on the official host passes the engine gate, so a chat + // dialect on a non-canonical official path is in scope: schemas are + // reshaped and `tool_choice:"required"` is sent — the doctor must + // not claim the tools stay unreshaped there. let config = Config { - provider: Some("deepseek-anthropic".to_string()), + provider: Some("deepseek".to_string()), strict_tool_mode: Some(true), + providers: Some(crate::config::ProvidersConfig { + deepseek: crate::config::ProviderConfig { + base_url: Some("https://api.deepseek.com/v2".to_string()), + ..Default::default() + }, + ..Default::default() + }), ..Default::default() }; @@ -14141,6 +14147,34 @@ mod doctor_endpoint_tests { assert!(status.recommended_base_url.is_none()); } + #[test] + fn strict_tool_mode_doctor_answers_the_anthropic_provider_from_the_wire_on_beta() { + // The gate passes for the Anthropic-family provider on the official + // beta host, but the beta verdict describes a chat-completions + // surface; the Anthropic Messages wire forwards the flag on its own + // dialect, so the doctor answers from the wire (`official_route`) + // and never shows the chat-dialect `ready` message for it. + let config = Config { + provider: Some("deepseek-anthropic".to_string()), + strict_tool_mode: Some(true), + providers: Some(crate::config::ProvidersConfig { + deepseek_anthropic: crate::config::ProviderConfig { + base_url: Some("https://api.deepseek.com/beta".to_string()), + ..Default::default() + }, + ..Default::default() + }), + ..Default::default() + }; + + let status = doctor_strict_tool_mode_status(&config); + + assert_eq!(status.status, "official_route"); + assert!(status.function_strict_sent); + assert!(status.message.contains("Anthropic wire forwards")); + assert!(status.recommended_base_url.is_none()); + } + #[test] fn strict_tool_mode_doctor_reports_anthropic_forwarding_on_non_beta_canonical_endpoints() { // The chat wire strips `function.strict` on the canonical non-beta diff --git a/crates/tui/src/tools/remember.rs b/crates/tui/src/tools/remember.rs index cc19865c2f..f36562a8aa 100644 --- a/crates/tui/src/tools/remember.rs +++ b/crates/tui/src/tools/remember.rs @@ -75,11 +75,12 @@ impl ToolSpec for RememberTool { "description": "Native backend scope; defaults to global. workspace requires a git repository with an origin." } }, - // `note` is required at runtime for every writing action - // (`execute` rejects its absence), so the schema says so. An - // all-optional schema would leave strict mode nothing to keep: - // DeepSeek's strict contract removes optional properties, which - // would leave this tool callable only with an empty object. + // `note` is required at runtime for append and revise (`execute` + // rejects its absence there; `retire` ignores the value), so the + // schema says so — and the field stays required for retire too, + // or an all-optional schema would leave strict mode nothing to + // keep: DeepSeek's strict contract removes optional properties, + // which would leave this tool callable only with an empty object. "required": ["note"] }) } diff --git a/crates/tui/src/tools/schema_sanitize.rs b/crates/tui/src/tools/schema_sanitize.rs index 92a8ec88b7..437bd26ceb 100644 --- a/crates/tui/src/tools/schema_sanitize.rs +++ b/crates/tui/src/tools/schema_sanitize.rs @@ -51,12 +51,17 @@ pub fn sanitize(schema: &mut Value) { /// only when every tool in the set can use strict mode. /// /// Incompatible includes schemas the documented strict subset cannot -/// represent faithfully: root alternatives, and required nullables (`nullable` +/// represent faithfully: root alternatives, required nullables (`nullable` /// or a null-type union branch — `sanitize` collapses the common `X | null` /// union into `nullable: true`, so the keyword is how a required nullable -/// survives to the gate). Such a tool keeps its original schema and ships -/// non-strict rather than carrying an undocumented keyword under a strict -/// flag. +/// survives to the gate), null-carrying `type` arrays / `const` / `enum` +/// members, structural or conditional keywords outside the documented subset +/// (the same family the Moonshot MFJS path fails closed on), and a removal +/// that would strip every parameter the tool declares (an all-optional root +/// leaves the tool callable only with an empty object while its runtime +/// still demands the values). Such a tool keeps its original schema and +/// ships non-strict rather than carrying an undocumented keyword under a +/// strict flag or advertising parameters it can no longer accept. /// /// Callers gate this pipeline on official DeepSeek-family routes /// (`crate::config::route_is_official_deepseek_family`): the reshaping @@ -66,7 +71,32 @@ pub fn prepare_tools_for_strict_mode(tools: &mut [Tool]) -> bool { let mut all_strict = true; for tool in tools { if strict_schema_supported(&tool.input_schema) { + let original_schema = tool.input_schema.clone(); let removed_optionals = sanitize_for_strict(&mut tool.input_schema); + // The strict contract removes every property the schema left + // optional. A tool whose whole root property set was optional is + // left with no way to pass any parameter while its runtime still + // demands them (action-discriminated tools mark everything + // optional and dispatch on the values `execute` re-reads) — that + // shape cannot serve the tool's contract under a strict flag, so + // it ships unchanged and non-strict, the same fail-closed rule + // as required nullables. No removal note either: nothing was + // removed from the schema that ships. + let emptied = tool + .input_schema + .get("properties") + .and_then(Value::as_object) + .is_some_and(Map::is_empty) + && original_schema + .get("properties") + .and_then(Value::as_object) + .is_some_and(|properties| !properties.is_empty()); + if emptied { + tool.input_schema = original_schema; + tool.strict = None; + all_strict = false; + continue; + } if !removed_optionals.is_empty() { let note = format!( "Strict mode: these optional parameters were removed from the schema and \ @@ -197,6 +227,56 @@ fn has_strict_incompatible_composition(schema: &Value, is_root: bool) -> bool { { return true; } + // The documented subset's union mechanism is `anyOf`; a `type` array + // is a draft-4-style union the documents never describe, and `null` + // is not a supported type anywhere — neither bare nor as an array + // member. Either shape cannot be represented faithfully under a + // strict flag. + match obj.get("type") { + Some(Value::String(schema_type)) if schema_type == "null" => return true, + Some(Value::Array(_)) => return true, + _ => {} + } + // An enum or const that accepts null is a required nullable wearing + // another keyword: the property must be sent and can be null. + if obj + .get("enum") + .and_then(Value::as_array) + .is_some_and(|members| members.iter().any(Value::is_null)) + { + return true; + } + if obj.get("const").is_some_and(Value::is_null) { + return true; + } + // Structural and conditional keywords outside the documented subset + // carry constraints the reshaping cannot express — the same family + // the Moonshot MFJS path fails closed on (`contains`, `prefixItems`, + // `not`, `if`/`then`/`else`, `propertyNames`, `unevaluatedProperties`, + // `unevaluatedItems`, tuple-form `items`). A schema relying on one + // ships unchanged and non-strict rather than reshaped-and-strict. + if obj.keys().any(|key| { + matches!( + key.as_str(), + "not" + | "if" + | "then" + | "else" + | "contains" + | "prefixItems" + | "propertyNames" + | "unevaluatedProperties" + | "unevaluatedItems" + ) + }) { + return true; + } + if obj + .get("items") + .is_some_and(|items| matches!(items, Value::Array(_))) + { + return true; + } return obj .values() .any(|value| has_strict_incompatible_composition(value, false)); @@ -359,7 +439,12 @@ fn enforce_strict_subset(schema: &mut Value, removed_optionals: &mut Vec // values, not schemas. for (key, value) in obj.iter_mut() { match key.as_str() { - "properties" | "$defs" | "definitions" => { + // `$def` is the singular spelling the DeepSeek documents + // themselves use for the definitions map; walk it like the + // standard `$defs`/`definitions` so a definition named after + // a stripped keyword is not deleted or renamed when its map + // is visited as if it were a schema. + "properties" | "$defs" | "definitions" | "$def" => { if let Some(named_schemas) = value.as_object_mut() { for named_schema in named_schemas.values_mut() { enforce_strict_subset(named_schema, removed_optionals); @@ -380,18 +465,39 @@ fn enforce_strict_subset(schema: &mut Value, removed_optionals: &mut Vec fn strip_unsupported_strict_keywords(obj: &mut Map) { obj.remove("patternProperties"); // Conditional dependency tables are likewise outside the documented - // strict subset; dropping one only widens what the endpoint accepts. + // strict subset — draft-7 `dependencies` covers both forms (a + // name table like `dependentRequired`, a schema map like + // `dependentSchemas`); dropping one only widens what the endpoint + // accepts. obj.remove("dependentRequired"); + obj.remove("dependencies"); + // Uniqueness and the property/containment count bounds are not in the + // documented subset either. With every property required and + // `additionalProperties: false` the property-count bounds are redundant + // or contradictory anyway, `minContains`/`maxContains` are meaningless + // without `contains` (which the compatibility gate rejects outright), + // and dropping a bound only widens what the endpoint accepts. + obj.remove("uniqueItems"); + obj.remove("minProperties"); + obj.remove("maxProperties"); + obj.remove("minContains"); + obj.remove("maxContains"); // The strict documents support `const` for number/integer schemas only. // A non-numeric const moves onto the documented `enum` keyword with its // value as the single member; a numeric const stays put, and a non-numeric - // const beside an existing enum yields to it. `default` stays verbatim - // (the documents show it, and list no ban). + // const beside an existing enum yields to it. A `const: null` is dropped + // outright — null is not a supported type, and landing it on `enum` would + // ship a null member (the compatibility gate already fails a tool closed + // for a null const; this is the defense for direct `sanitize_for_strict` + // calls). `default` stays verbatim (the documents show it, and list no + // ban). if let Some(const_value) = obj.remove("const") { - if const_value.is_number() { - obj.insert("const".into(), const_value); - } else if !obj.contains_key("enum") { - obj.insert("enum".into(), Value::Array(vec![const_value])); + if !const_value.is_null() { + if const_value.is_number() { + obj.insert("const".into(), const_value); + } else if !obj.contains_key("enum") { + obj.insert("enum".into(), Value::Array(vec![const_value])); + } } } match obj.get("type").and_then(Value::as_str) { @@ -955,9 +1061,10 @@ mod tests { json!({ "type": "object", "properties": { - "query": {"type": "string"} + "query": {"type": "string"}, + "index": {"type": "string", "enum": ["global", "workspace"]} }, - "required": [] + "required": ["index"] }), ), test_tool( @@ -994,7 +1101,7 @@ mod tests { assert_eq!(tools[0].strict, Some(true)); assert_eq!( tools[0].input_schema["required"], - json!([]), + json!(["index"]), "the optional query is removed, not nullable-marked" ); assert!(tools[0].input_schema["properties"].get("query").is_none()); @@ -1067,6 +1174,10 @@ mod tests { #[test] fn strict_mode_marks_compatible_tools_strict() { + // A required property stays so the removal does not empty the tool + // (an all-optional root fails closed instead — see + // `strict_mode_fails_closed_when_removal_empties_the_tool`); the + // optional one is removed with a note. let mut tools = vec![Tool { tool_type: None, name: "lookup".to_string(), @@ -1074,9 +1185,10 @@ mod tests { input_schema: json!({ "type": "object", "properties": { - "query": {"type": "string"} + "query": {"type": "string"}, + "index": {"type": "string", "enum": ["global", "workspace"]} }, - "required": [] + "required": ["index"] }), allowed_callers: None, defer_loading: None, @@ -1087,7 +1199,7 @@ mod tests { assert!(prepare_tools_for_strict_mode(&mut tools)); assert_eq!(tools[0].strict, Some(true)); - assert_eq!(tools[0].input_schema["required"], json!([])); + assert_eq!(tools[0].input_schema["required"], json!(["index"])); assert!(tools[0].input_schema["properties"].get("query").is_none()); assert_eq!(tools[0].input_schema["additionalProperties"], false); assert!( @@ -1108,9 +1220,10 @@ mod tests { input_schema: json!({ "type": "object", "properties": { - "query": {"type": "string"} + "query": {"type": "string"}, + "index": {"type": "string", "enum": ["global", "workspace"]} }, - "required": [] + "required": ["index"] }), allowed_callers: None, defer_loading: None, @@ -1133,6 +1246,169 @@ mod tests { ); } + #[test] + fn strict_mode_fails_closed_when_removal_empties_the_tool() { + // A tool whose whole root property set is optional loses every + // parameter to the strict contract while its runtime still demands + // them (action-discriminated tools mark everything optional and + // dispatch on the values `execute` re-reads). That shape cannot + // serve the tool under a strict flag, so it ships unchanged and + // non-strict — with no removal note, because nothing was removed + // from the schema that ships. + let mut tools = vec![Tool { + tool_type: None, + name: "tasks".to_string(), + description: "Durable tasks".to_string(), + input_schema: json!({ + "type": "object", + "properties": { + "action": {"type": "string", "enum": ["create", "read", "cancel"]}, + "prompt": {"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, None); + assert_eq!( + tools[0].input_schema["properties"]["action"]["enum"], + json!(["create", "read", "cancel"]) + ); + assert_eq!(tools[0].input_schema["required"], json!([])); + assert_eq!( + tools[0].input_schema["properties"].get("prompt"), + Some(&json!({"type": "string"})) + ); + assert!(tools[0].input_schema.get("additionalProperties").is_none()); + assert_eq!(tools[0].description, "Durable tasks"); + } + + #[test] + fn strict_mode_fails_closed_on_type_array_unions() { + // The documented subset's union mechanism is `anyOf`; a `type` + // array is a draft-4-style union the documents never describe, and + // `null` is not a supported type anywhere — neither bare nor as an + // array member. Either shape ships unchanged and non-strict instead + // of carrying an undocumented construct under a strict flag. + let mut tools = vec![Tool { + tool_type: None, + name: "search".to_string(), + description: "Search".to_string(), + input_schema: json!({ + "type": "object", + "properties": { + "mode": {"type": ["string", "null"]}, + "level": {"type": ["integer", "string"]} + }, + "required": ["mode", "level"] + }), + 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); + assert_eq!( + tools[0].input_schema["properties"]["mode"], + json!({"type": ["string", "null"]}) + ); + assert_eq!( + tools[0].input_schema["properties"]["level"], + json!({"type": ["integer", "string"]}) + ); + assert_eq!(tools[0].description, "Search"); + } + + #[test] + fn strict_mode_fails_closed_on_null_valued_schema_nodes() { + // A bare `{"type":"null"}`, a `const: null`, and an enum with a + // null member are all required nullables wearing another keyword: + // the property must be sent and can be null. Such tools stay + // non-strict with their original schema. + let mut tools = vec![Tool { + tool_type: None, + name: "probe".to_string(), + description: "Probe".to_string(), + input_schema: json!({ + "type": "object", + "properties": { + "empty": {"type": "null"}, + "fixed": {"const": null}, + "choice": {"enum": [null, "a"]} + }, + "required": ["empty", "fixed", "choice"] + }), + 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); + assert_eq!( + tools[0].input_schema["properties"]["empty"], + json!({"type": "null"}) + ); + assert_eq!( + tools[0].input_schema["properties"]["fixed"], + json!({"const": null}) + ); + assert_eq!( + tools[0].input_schema["properties"]["choice"], + json!({"enum": [null, "a"]}) + ); + } + + #[test] + fn strict_mode_fails_closed_on_structural_and_conditional_keywords() { + // Structural and conditional keywords outside the documented subset + // carry constraints the reshaping cannot express — the same family + // the Moonshot MFJS path fails closed on. Tuple-form `items` is + // likewise a draft construct the documents never describe. + let mut tools = vec![Tool { + tool_type: None, + name: "check".to_string(), + description: "Check".to_string(), + input_schema: json!({ + "type": "object", + "properties": { + "pattern": {"type": "string"}, + "tags": { + "type": "array", + "prefixItems": [{"type": "string"}, {"type": "string"}] + } + }, + "required": ["pattern", "tags"] + }), + 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); + assert!( + tools[0].input_schema["properties"]["tags"] + .get("prefixItems") + .is_some(), + "the schema ships unchanged: {}", + tools[0].input_schema + ); + } + #[test] fn strict_mode_fails_closed_on_required_nullable_unions() { // `nullable` is outside the documented strict subset (the supported @@ -1326,6 +1602,105 @@ mod tests { assert_eq!(schema["required"], json!(["a"])); } + #[test] + fn strict_sanitize_strips_undocumented_dependency_and_bound_keywords() { + // Draft-7 `dependencies` (both the name-table and the schema form), + // `uniqueItems`, and the property/containment count bounds are not + // in the documented subset; dropping one only widens what the + // endpoint accepts, the same treatment as `dependentRequired`. + let mut schema = json!({ + "type": "object", + "properties": { + "a": {"type": "string"}, + "b": {"type": "string"} + }, + "required": ["a"], + "dependencies": {"a": ["b"]}, + "uniqueItems": true, + "minProperties": 1, + "maxProperties": 5, + "minContains": 1, + "maxContains": 3 + }); + + let removed = sanitize_for_strict(&mut schema); + + assert!(schema.get("dependencies").is_none()); + assert!(schema.get("uniqueItems").is_none()); + assert!(schema.get("minProperties").is_none()); + assert!(schema.get("maxProperties").is_none()); + assert!(schema.get("minContains").is_none()); + assert!(schema.get("maxContains").is_none()); + assert_eq!(schema["required"], json!(["a"])); + assert_eq!(removed, vec!["b".to_string()]); + } + + #[test] + fn strict_sanitize_drops_a_null_const_instead_of_landing_it_on_enum() { + // `const: null` cannot be represented (null is not a supported + // type), and landing it on `enum` would ship a null member. The + // compatibility gate fails such a tool closed; for direct + // `sanitize_for_strict` calls the strip drops the const outright + // instead of manufacturing `enum: [null]`. + let mut schema = json!({ + "type": "object", + "properties": { + "fixed": {"type": "string", "const": null}, + "kept": {"type": "integer", "const": 3} + }, + "required": ["fixed", "kept"] + }); + + let removed = sanitize_for_strict(&mut schema); + + assert_eq!( + schema["properties"]["fixed"], + json!({"type": "string"}), + "a null const is dropped, not turned into a null enum member" + ); + assert_eq!(schema["properties"]["kept"]["const"], 3); + assert_eq!(removed, Vec::::new()); + } + + #[test] + fn strict_sanitize_walks_the_documented_def_spelling_as_a_name_map() { + // The DeepSeek documents spell the definitions map `$def` + // (singular). It must be walked as a name map like + // `$defs`/`definitions`, so a definition named after a stripped + // keyword survives intact instead of being deleted or renamed when + // its map is visited as if it were a schema. + let mut schema = json!({ + "type": "object", + "properties": {"item": {"$ref": "#/$def/const"}}, + "required": ["item"], + "$def": { + "const": {"type": "string"}, + "patternProperties": {"type": "object"}, + "span": { + "type": "object", + "properties": {"b": {"type": "integer"}}, + "required": ["b"] + } + } + }); + + let removed = sanitize_for_strict(&mut schema); + + assert!(removed.is_empty()); + assert_eq!( + schema["$def"]["const"], + json!({"type": "string"}), + "a definition named `const` keeps its schema instead of becoming an `enum` key" + ); + assert_eq!( + schema["$def"]["patternProperties"]["type"], + json!("object"), + "a definition named `patternProperties` is not deleted by the keyword strip" + ); + assert_eq!(schema["$def"]["span"]["additionalProperties"], false); + assert_eq!(schema["$def"]["span"]["required"], json!(["b"])); + } + #[test] fn strict_sanitize_dedupes_optionals_shared_across_levels() { // The same optional name at two levels is reported once. From 705546c94615cdd0237dc9e4945508d76a7d9d4f Mon Sep 17 00:00:00 2001 From: asto Date: Fri, 6 Nov 2026 22:09:57 +0800 Subject: [PATCH 5/7] fix(tui): gate the strict pipeline on the enforced shape and unusable tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The compatibility gate now judges the shape that would actually ship: the enforce pass removes optional properties before the gate walks the schema, so a null-carrying shape under an OPTIONAL property is exempt (the removal already represents it faithfully) while the same shape under a required property still fails the tool closed. Constraints the enforce pass strips before the gate can see them — a null `const` and a schema-form draft-7 `dependencies` table (semantically `dependentSchemas`, which the gate fails closed on) — are detected on the sanitized shape with the optional-removal in view. A removal that empties a nested object now fails closed like the root-emptied rule: a required object whose every property is optional ships callable only with `{}` while the runtime still reads the removed members, and a required bare `{"type":"object"}` parameter collapses to the same shape once `sanitize` injects the empty `properties`. `update_goal` and `send_later` fail closed by name: their schemas keep one discriminating parameter required while `execute` demands sibling parameters the schema marks optional, so after the removal pass they shipped `strict: true` callable only with values the runtime rejects (`update_goal` for every status value, `send_later` for three of four actions). The documented strict subset cannot express those per-value requirements, so the tools ship unchanged and non-strict; census tests feed their real schemas through the pipeline so the list cannot drift. The strip drops `format` values outside the documented enumeration (email, hostname, ipv4, ipv6, uuid) and draft-4 boolean `exclusiveMinimum`/`exclusiveMaximum` — both widening-only — and the null-family and structural-keyword fail-closed tests are pinned per arm so a single-keyword regression cannot pass. Signed-off-by: asto --- crates/tui/src/tools/goal.rs | 34 ++ crates/tui/src/tools/remember.rs | 48 +++ crates/tui/src/tools/schema_sanitize.rs | 492 ++++++++++++++++++++++-- crates/tui/src/tools/send_later.rs | 54 +++ 4 files changed, 602 insertions(+), 26 deletions(-) diff --git a/crates/tui/src/tools/goal.rs b/crates/tui/src/tools/goal.rs index 5641e0671f..cda54d1de6 100644 --- a/crates/tui/src/tools/goal.rs +++ b/crates/tui/src/tools/goal.rs @@ -1992,4 +1992,38 @@ mod tests { assert_eq!(state.time_used_seconds, 0); assert_eq!(state.continuation_count, 0); } + + #[test] + fn strict_mode_fails_the_update_goal_tool_closed() { + // `update_goal` keeps only `status` in strict mode while every + // status value runtime-requires a sibling parameter the schema + // marks optional (`execute` demands `evidence`/`verification` for + // `complete`, `blocker` for `blocked`, `verification` for + // `not_achieved`, `advisory` for `advisory`). The strict pipeline + // fails the tool closed by name (`STRICT_UNUSABLE_TOOLS`) so it + // ships unchanged, non-strict, and usable. This census test feeds + // the real schema through the pipeline so the two cannot drift. + let tool = UpdateGoalTool::new(new_shared_goal_state()); + let schema = tool.input_schema(); + assert_eq!(schema["required"], json!(["status"])); + let mut tools = vec![crate::models::Tool { + tool_type: None, + name: tool.name().to_string(), + description: tool.description().to_string(), + input_schema: schema.clone(), + allowed_callers: None, + defer_loading: None, + input_examples: None, + strict: None, + cache_control: None, + }]; + + assert!( + !crate::tools::schema_sanitize::prepare_tools_for_strict_mode(&mut tools), + "update_goal must fail closed, not ship strict with only `status`" + ); + assert_eq!(tools[0].strict, None); + assert_eq!(tools[0].input_schema, schema); + assert_eq!(tools[0].description, tool.description()); + } } diff --git a/crates/tui/src/tools/remember.rs b/crates/tui/src/tools/remember.rs index f36562a8aa..43c55b92a0 100644 --- a/crates/tui/src/tools/remember.rs +++ b/crates/tui/src/tools/remember.rs @@ -451,4 +451,52 @@ mod tests { let err = tool.execute(json!({}), &ctx).await.unwrap_err(); assert!(err.to_string().to_lowercase().contains("note"), "{err}"); } + + #[test] + fn strict_mode_keeps_remember_usable_with_its_note() { + // The schema declares `note` required so strict mode has something + // to keep: the pipeline marks the tool strict, `note` survives + // (append is the default action, so `{"note": ...}` stays callable), + // and the other parameters are removed with the description note. + // Reverting the schema to an all-optional shape must fail here — + // without this pin the schema and the strict pipeline could drift + // apart silently. + let tool = RememberTool; + let schema = tool.input_schema(); + let mut tools = vec![crate::models::Tool { + tool_type: None, + name: tool.name().to_string(), + description: tool.description().to_string(), + input_schema: schema.clone(), + allowed_callers: None, + defer_loading: None, + input_examples: None, + strict: None, + cache_control: None, + }]; + + assert!( + crate::tools::schema_sanitize::prepare_tools_for_strict_mode(&mut tools), + "remember's schema is strict-representable: {}", + schema + ); + assert_eq!(tools[0].strict, Some(true)); + assert_eq!(tools[0].input_schema["required"], json!(["note"])); + let properties = tools[0].input_schema["properties"].as_object().unwrap(); + assert_eq!( + properties.len(), + 1, + "only `note` ships in strict mode: {properties:?}" + ); + assert!(properties.contains_key("note")); + assert_eq!( + tools[0].input_schema["properties"]["note"]["type"], + schema["properties"]["note"]["type"] + ); + assert!( + tools[0].description.contains("Strict mode:"), + "the removal note replaces nothing: {}", + tools[0].description + ); + } } diff --git a/crates/tui/src/tools/schema_sanitize.rs b/crates/tui/src/tools/schema_sanitize.rs index 437bd26ceb..3d4064e567 100644 --- a/crates/tui/src/tools/schema_sanitize.rs +++ b/crates/tui/src/tools/schema_sanitize.rs @@ -56,20 +56,50 @@ pub fn sanitize(schema: &mut Value) { /// union into `nullable: true`, so the keyword is how a required nullable /// survives to the gate), null-carrying `type` arrays / `const` / `enum` /// members, structural or conditional keywords outside the documented subset -/// (the same family the Moonshot MFJS path fails closed on), and a removal -/// that would strip every parameter the tool declares (an all-optional root -/// leaves the tool callable only with an empty object while its runtime -/// still demands the values). Such a tool keeps its original schema and -/// ships non-strict rather than carrying an undocumented keyword under a -/// strict flag or advertising parameters it can no longer accept. +/// (the same family the Moonshot MFJS path fails closed on), a null `const` +/// or schema-form `dependencies` table (stripped before the gate could see +/// them), and a removal that would leave the tool callable only with an +/// empty object — an all-optional root, or a nested object whose every +/// property was optional. Such a tool keeps its original schema and ships +/// non-strict rather than carrying an undocumented keyword under a strict +/// flag or advertising parameters it can no longer accept. The gate runs on +/// the enforced shape, so a null-carrying shape under an *optional* property +/// is exempt: the removal already represents it faithfully. /// /// Callers gate this pipeline on official DeepSeek-family routes /// (`crate::config::route_is_official_deepseek_family`): the reshaping /// follows DeepSeek's strict-mode documents and would strip keywords other /// providers document (e.g. `minItems`/`maxItems` on OpenAI). +/// Tools the strict pipeline fails closed by name because no schema-level +/// analysis available to the reshaping can see the breakage: the schema +/// advertises one discriminating parameter as the only required property +/// while `execute` demands sibling parameters the schema leaves optional, +/// so after the optional-removal pass the tool ships `strict: true` +/// callable only with values its runtime rejects. +/// +/// `update_goal` runtime-requires `evidence` and `verification` for +/// `status: "complete"`, `blocker` for `"blocked"`, `verification` for +/// `"not_achieved"`, and `advisory` for `"advisory"` (`tools/goal.rs`); +/// `send_later` runtime-requires `message` for `action: "schedule"` and +/// `trigger_id` for `"read"`/`"cancel"` (`tools/send_later.rs`). The +/// documented strict subset cannot express those per-value requirements — +/// `if`/`then` and root alternatives are outside it — and widening the +/// schema requirements would make non-strict callers send junk for every +/// other value, so the tools ship unchanged and non-strict instead. +/// Per-action schema honesty would retire this list. +const STRICT_UNUSABLE_TOOLS: &[&str] = &["update_goal", "send_later"]; + pub fn prepare_tools_for_strict_mode(tools: &mut [Tool]) -> bool { let mut all_strict = true; for tool in tools { + if STRICT_UNUSABLE_TOOLS.contains(&tool.name.as_str()) { + // The runtime demands parameters the schema marks optional (see + // the list above); the removal pass would leave the tool strict + // with only its discriminator. Ship unchanged and non-strict. + tool.strict = None; + all_strict = false; + continue; + } if strict_schema_supported(&tool.input_schema) { let original_schema = tool.input_schema.clone(); let removed_optionals = sanitize_for_strict(&mut tool.input_schema); @@ -194,9 +224,82 @@ pub fn sanitize_for_responses(schema: &mut Value) -> Option { fn strict_schema_supported(schema: &Value) -> bool { let mut normalized = schema.clone(); sanitize(&mut normalized); + // The enforce pass strips some constraints before the compatibility + // gate below can judge them: a null `const` (dropped outright) and a + // schema-form draft-7 `dependencies` table (dropped with the name-table + // form) carry requirements the shipped schema would no longer express. + // They are detected here, on the sanitized shape, with the + // optional-removal in view — an occurrence under an optional property + // is moot because the removal takes the whole property. + if has_unrepresentable_stripped_constraint(&normalized, true) { + return false; + } + // Gate the shape that would actually ship: the enforce pass removes + // optional properties, so a null-carrying shape under an optional + // property is faithfully represented by that removal and no longer + // forces the tool non-strict, while the same shape under a required + // property still does. + enforce_strict_subset(&mut normalized, &mut Vec::new()); !has_strict_incompatible_composition(&normalized, true) } +/// Detect the constraints the enforce pass strips before the compatibility +/// gate can judge them. `ships` reports whether the node survives the +/// optional-removal pass — only then does its constraint still reach the +/// wire. A null `const` is a required nullable wearing another keyword, and +/// a schema-form `dependencies` value is semantically `dependentSchemas` +/// (which the gate fails closed), so its draft-7 spelling fails closed too +/// rather than shipping strict with the cross-schema requirement silently +/// gone; the name-table form is `dependentRequired`, whose strip only +/// widens and stays a strip. +fn has_unrepresentable_stripped_constraint(schema: &Value, ships: bool) -> bool { + let Some(obj) = schema.as_object() else { + return schema.as_array().is_some_and(|values| { + values + .iter() + .any(|value| has_unrepresentable_stripped_constraint(value, ships)) + }); + }; + if ships && obj.get("const").is_some_and(Value::is_null) { + return true; + } + if ships + && obj + .get("dependencies") + .and_then(Value::as_object) + .is_some_and(|table| table.values().any(Value::is_object)) + { + return true; + } + let required = required_names(obj); + for (key, value) in obj { + match key.as_str() { + "properties" | "$defs" | "definitions" | "$def" => { + if let Some(named_schemas) = value.as_object() { + for (name, named_schema) in named_schemas { + // Definitions ship whenever their `$ref` resolves, + // so they are judged without the optional-property + // exemption; only a property the schema leaves + // optional is removed wholesale. + let ships_child = + key != "properties" || required.iter().any(|required| required == name); + if has_unrepresentable_stripped_constraint(named_schema, ships_child) { + return true; + } + } + } + } + "enum" | "const" | "default" | "examples" => {} + _ => { + if has_unrepresentable_stripped_constraint(value, ships) { + return true; + } + } + } + } + false +} + fn has_strict_incompatible_composition(schema: &Value, is_root: bool) -> bool { if let Some(obj) = schema.as_object() { if obj.contains_key("oneOf") @@ -277,6 +380,26 @@ fn has_strict_incompatible_composition(schema: &Value, is_root: bool) -> bool { { return true; } + // A nested object left with no properties by the optional-removal + // pass ships callable only with `{}` while the tool's runtime may + // still read the removed members — the same degenerate shape the + // root-emptied check in `prepare_tools_for_strict_mode` fails + // closed, one level down (the root keeps its own check there so + // authored no-argument roots stay strict). A bare + // `{"type":"object"}` node gets an empty `properties` injected by + // `sanitize`, and under `additionalProperties: false` that shape + // cannot carry a value either. `$defs` entries are judged here too: + // the gate does not track `$ref` reachability, so a definition that + // could not serve a resolved reference also fails the tool closed. + if !is_root + && is_object_schema(obj) + && obj + .get("properties") + .and_then(Value::as_object) + .is_some_and(Map::is_empty) + { + return true; + } return obj .values() .any(|value| has_strict_incompatible_composition(value, false)); @@ -500,6 +623,23 @@ fn strip_unsupported_strict_keywords(obj: &mut Map) { } } } + // `format` is documented with an enumerated set (email, hostname, + // ipv4, ipv6, uuid); any other value ships a constraint outside that + // list, so it is dropped — dropping a format only widens what the + // endpoint accepts. A draft-4 boolean `exclusiveMinimum`/`Maximum` + // is likewise outside the documented numeric form. + if obj + .get("format") + .and_then(Value::as_str) + .is_some_and(|format| !matches!(format, "email" | "hostname" | "ipv4" | "ipv6" | "uuid")) + { + obj.remove("format"); + } + for key in ["exclusiveMinimum", "exclusiveMaximum"] { + if obj.get(key).is_some_and(Value::is_boolean) { + obj.remove(key); + } + } match obj.get("type").and_then(Value::as_str) { Some("string") => { obj.remove("minLength"); @@ -1329,30 +1469,22 @@ mod tests { } #[test] - fn strict_mode_fails_closed_on_null_valued_schema_nodes() { - // A bare `{"type":"null"}`, a `const: null`, and an enum with a - // null member are all required nullables wearing another keyword: - // the property must be sent and can be null. Such tools stay - // non-strict with their original schema. - let mut tools = vec![Tool { - tool_type: None, - name: "probe".to_string(), - description: "Probe".to_string(), - input_schema: json!({ + fn strict_mode_fails_closed_on_a_bare_null_type() { + // A bare `{"type":"null"}` is a required nullable wearing another + // keyword: the property must be sent and can be null, and null is + // not a supported type anywhere in the documented subset. The tool + // stays non-strict with its original schema. (Pinned per arm: a + // shared fixture let a single-arm regression pass.) + let mut tools = vec![test_tool( + "probe", + json!({ "type": "object", "properties": { - "empty": {"type": "null"}, - "fixed": {"const": null}, - "choice": {"enum": [null, "a"]} + "empty": {"type": "null"} }, - "required": ["empty", "fixed", "choice"] + "required": ["empty"] }), - 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); @@ -1360,14 +1492,100 @@ mod tests { tools[0].input_schema["properties"]["empty"], json!({"type": "null"}) ); + assert_eq!(tools[0].description, "probe"); + } + + #[test] + fn strict_mode_fails_closed_on_a_null_const() { + // A `const: null` is the same required nullable in another keyword: + // the strip drops it outright for direct callers, so the gate must + // catch it before the strip or the tool would ship strict without + // the constraint it advertises. + let mut tools = vec![test_tool( + "probe", + json!({ + "type": "object", + "properties": { + "fixed": {"const": null} + }, + "required": ["fixed"] + }), + )]; + + assert!(!prepare_tools_for_strict_mode(&mut tools)); + assert_eq!(tools[0].strict, None); assert_eq!( tools[0].input_schema["properties"]["fixed"], json!({"const": null}) ); + assert_eq!(tools[0].description, "probe"); + } + + #[test] + fn strict_mode_fails_closed_on_a_null_enum_member() { + // An enum with a null member is a required nullable wearing the + // documented `enum` keyword: landing on it would ship a null member + // for a type the documents never support. + let mut tools = vec![test_tool( + "probe", + json!({ + "type": "object", + "properties": { + "choice": {"enum": [null, "a"]} + }, + "required": ["choice"] + }), + )]; + + assert!(!prepare_tools_for_strict_mode(&mut tools)); + assert_eq!(tools[0].strict, None); assert_eq!( tools[0].input_schema["properties"]["choice"], json!({"enum": [null, "a"]}) ); + assert_eq!(tools[0].description, "probe"); + } + + #[test] + fn strict_mode_keeps_strict_when_null_shapes_sit_under_optional_properties() { + // The gate judges the enforced shape: a null-carrying shape under + // an OPTIONAL property is faithfully represented by that removal, + // so the tool stays strict and the property is removed with the + // standard note instead of failing the whole tool closed. The same + // shapes under a required property keep failing the tool closed + // (see the per-arm tests above). + let mut tools = vec![test_tool( + "probe", + json!({ + "type": "object", + "properties": { + "name": {"type": "string"}, + "opt_union": {"anyOf": [{"type": "string"}, {"type": "null"}]}, + "opt_nullable": {"type": "string", "nullable": true}, + "opt_null_type": {"type": "null"}, + "opt_null_const": {"const": null}, + "opt_null_enum": {"enum": [null, "a"]}, + "opt_type_array": {"type": ["string", "null"]} + }, + "required": ["name"] + }), + )]; + + assert!(prepare_tools_for_strict_mode(&mut tools)); + assert_eq!(tools[0].strict, Some(true)); + assert_eq!(tools[0].input_schema["required"], json!(["name"])); + let properties = tools[0].input_schema["properties"].as_object().unwrap(); + assert_eq!( + properties.len(), + 1, + "only the required property ships: {properties:?}" + ); + assert!(properties.contains_key("name")); + assert!( + tools[0].description.contains("opt_null_const"), + "the note names the removed optional: {}", + tools[0].description + ); } #[test] @@ -1409,6 +1627,178 @@ mod tests { ); } + #[test] + fn strict_mode_fails_closed_on_each_structural_keyword() { + // Each member of the structural/conditional family is pinned + // individually — a shared fixture let a single-arm regression (a + // typo in one keyword arm) pass silently. + for keyword in [ + "not", + "if", + "then", + "else", + "contains", + "prefixItems", + "propertyNames", + "unevaluatedProperties", + "unevaluatedItems", + ] { + let mut target = serde_json::Map::new(); + target.insert("type".into(), json!("string")); + target.insert(keyword.into(), json!(true)); + let mut tools = vec![test_tool( + "check", + json!({ + "type": "object", + "properties": {"target": Value::Object(target)}, + "required": ["target"] + }), + )]; + + assert!( + !prepare_tools_for_strict_mode(&mut tools), + "the {keyword} keyword must fail the tool closed" + ); + assert_eq!(tools[0].strict, None, "keyword: {keyword}"); + assert!( + tools[0].input_schema["properties"]["target"] + .get(keyword) + .is_some(), + "the {keyword} keyword ships unchanged" + ); + } + } + + #[test] + fn strict_mode_fails_closed_when_removal_empties_a_nested_object() { + // The root-emptied rule has a nested analog: a REQUIRED object whose + // every property is optional loses them all and ships callable only + // with `{}` while the runtime still reads the removed members. The + // tool ships unchanged and non-strict, like the root case. + let mut tools = vec![test_tool( + "deploy", + json!({ + "type": "object", + "properties": { + "id": {"type": "string"}, + "opts": { + "type": "object", + "properties": { + "region": {"type": "string"}, + "replicas": {"type": "integer"} + }, + "required": [] + } + }, + "required": ["id", "opts"] + }), + )]; + + assert!(!prepare_tools_for_strict_mode(&mut tools)); + assert_eq!(tools[0].strict, None); + assert_eq!( + tools[0].input_schema["properties"]["opts"]["required"], + json!([]) + ); + assert!( + tools[0].input_schema["properties"]["opts"]["properties"] + .get("region") + .is_some(), + "the nested schema ships unchanged: {}", + tools[0].input_schema + ); + assert_eq!(tools[0].description, "deploy"); + } + + #[test] + fn strict_mode_fails_closed_on_a_required_bare_object_parameter() { + // `sanitize` injects an empty `properties` on a bare + // `{"type":"object"}` node; under `additionalProperties: false` that + // shape cannot carry a value, so a required bare-object parameter + // fails the tool closed instead of shipping strict accepting only + // `{}`. + let mut tools = vec![test_tool( + "configure", + json!({ + "type": "object", + "properties": { + "name": {"type": "string"}, + "config": {"type": "object"} + }, + "required": ["name", "config"] + }), + )]; + + assert!(!prepare_tools_for_strict_mode(&mut tools)); + assert_eq!(tools[0].strict, None); + assert_eq!( + tools[0].input_schema["properties"]["config"], + json!({"type": "object"}), + "the schema ships unchanged" + ); + assert_eq!(tools[0].description, "configure"); + } + + #[test] + fn strict_mode_fails_closed_on_a_schema_form_dependencies_table() { + // A draft-7 `dependencies` entry whose value is a schema is + // semantically `dependentSchemas`, which the gate fails closed on; + // the strip drops both spellings, so the schema form must be + // detected before the strip or the tool would ship strict with the + // cross-schema requirement silently gone. The name-table form + // (`dependentRequired`) keeps its widening-only strip. + let mut tools = vec![test_tool( + "agent", + json!({ + "type": "object", + "properties": { + "a": {"type": "string"}, + "b": {"type": "string"} + }, + "required": ["a"], + "dependencies": {"a": {"required": ["b"]}} + }), + )]; + + assert!(!prepare_tools_for_strict_mode(&mut tools)); + assert_eq!(tools[0].strict, None); + assert_eq!( + tools[0].input_schema["dependencies"]["a"], + json!({"required": ["b"]}), + "the table ships unchanged" + ); + assert_eq!(tools[0].description, "agent"); + } + + #[test] + fn strict_mode_keeps_strict_when_a_schema_form_dependencies_table_is_optional() { + // The same table under an OPTIONAL property is removed wholesale — + // no fail-closed, no widening concern: the constraint ships nowhere. + let mut tools = vec![test_tool( + "agent", + json!({ + "type": "object", + "properties": { + "name": {"type": "string"}, + "opts": { + "type": "object", + "properties": { + "a": {"type": "string"} + }, + "required": ["a"], + "dependencies": {"a": {"required": ["b"]}, "b": {"type": "string"}} + } + }, + "required": ["name"] + }), + )]; + + assert!(prepare_tools_for_strict_mode(&mut tools)); + assert_eq!(tools[0].strict, Some(true)); + assert_eq!(tools[0].input_schema["required"], json!(["name"])); + assert!(tools[0].input_schema["properties"].get("opts").is_none()); + } + #[test] fn strict_mode_fails_closed_on_required_nullable_unions() { // `nullable` is outside the documented strict subset (the supported @@ -1635,6 +2025,56 @@ mod tests { assert_eq!(removed, vec!["b".to_string()]); } + #[test] + fn strict_sanitize_strips_undocumented_formats_and_draft4_exclusive_bounds() { + // `format` is documented with an enumerated set (email, hostname, + // ipv4, ipv6, uuid); any other value ships a constraint outside + // that list, so it is dropped — dropping a format only widens what + // the endpoint accepts. A draft-4 boolean + // `exclusiveMinimum`/`Maximum` is likewise outside the documented + // numeric form, while the numeric form stays. + let mut schema = json!({ + "type": "object", + "properties": { + "when": {"type": "string", "format": "date-time"}, + "mail": {"type": "string", "format": "email"}, + "host": {"type": "string", "format": "uuid"}, + "score": { + "type": "number", + "minimum": 0, + "exclusiveMinimum": true, + "maximum": 10 + }, + "weight": { + "type": "number", + "exclusiveMinimum": 0, + "exclusiveMaximum": 100 + } + }, + "required": ["when", "mail", "host", "score", "weight"] + }); + + let removed = sanitize_for_strict(&mut schema); + + assert_eq!(removed, Vec::::new()); + assert!( + schema["properties"]["when"].get("format").is_none(), + "an undocumented format is dropped: {}", + schema["properties"]["when"] + ); + assert_eq!(schema["properties"]["mail"]["format"], "email"); + assert_eq!(schema["properties"]["host"]["format"], "uuid"); + assert!( + schema["properties"]["score"] + .get("exclusiveMinimum") + .is_none(), + "a draft-4 boolean exclusiveMinimum is dropped" + ); + assert_eq!(schema["properties"]["weight"]["exclusiveMinimum"], 0); + assert_eq!(schema["properties"]["weight"]["exclusiveMaximum"], 100); + assert_eq!(schema["properties"]["weight"]["type"], "number"); + } + #[test] fn strict_sanitize_drops_a_null_const_instead_of_landing_it_on_enum() { // `const: null` cannot be represented (null is not a supported diff --git a/crates/tui/src/tools/send_later.rs b/crates/tui/src/tools/send_later.rs index 8d97792636..91450cb310 100644 --- a/crates/tui/src/tools/send_later.rs +++ b/crates/tui/src/tools/send_later.rs @@ -989,4 +989,58 @@ mod tests { let due = manager.collect_due_triggers(chrono::Utc::now()).unwrap(); assert_eq!(due.len(), 1, "should fire a past-due trigger"); } + + #[test] + fn strict_mode_fails_the_send_later_tool_closed() { + // `send_later` keeps only `action` in strict mode while + // `schedule` runtime-requires `message` and `read`/`cancel` + // runtime-require `trigger_id` — only `list` would stay reachable. + // The strict pipeline fails the tool closed by name + // (`STRICT_UNUSABLE_TOOLS`) so it ships unchanged, non-strict, and + // usable. This census test feeds the real schema through the + // pipeline so the two cannot drift. + let tool = SendLaterTool::new("send_later"); + let schema = tool.input_schema(); + assert_eq!(schema["required"], json!(["action"])); + let mut tools = vec![crate::models::Tool { + tool_type: None, + name: tool.name().to_string(), + description: tool.description().to_string(), + input_schema: schema.clone(), + allowed_callers: None, + defer_loading: None, + input_examples: None, + strict: None, + cache_control: None, + }]; + + assert!( + !crate::tools::schema_sanitize::prepare_tools_for_strict_mode(&mut tools), + "send_later must fail closed, not ship strict with only `action`" + ); + assert_eq!(tools[0].strict, None); + assert_eq!(tools[0].input_schema, schema); + assert_eq!(tools[0].description, tool.description()); + + // The read-only variant advertises the same discriminator-only + // shape, so it fails closed identically. + let read_only = SendLaterTool::read_only("send_later"); + let read_only_schema = read_only.input_schema(); + let mut read_only_tools = vec![crate::models::Tool { + tool_type: None, + name: read_only.name().to_string(), + description: read_only.description().to_string(), + input_schema: read_only_schema.clone(), + allowed_callers: None, + defer_loading: None, + input_examples: None, + strict: None, + cache_control: None, + }]; + assert!( + !crate::tools::schema_sanitize::prepare_tools_for_strict_mode(&mut read_only_tools) + ); + assert_eq!(read_only_tools[0].strict, None); + assert_eq!(read_only_tools[0].input_schema, read_only_schema); + } } From 3b2dec71ad37548dc8dbc551381b40316102e888 Mon Sep 17 00:00:00 2001 From: asto Date: Fri, 6 Nov 2026 22:10:05 +0800 Subject: [PATCH 6/7] fix(tui): keep the strict doctor's recommendation actionable and its wording honest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fallback_non_beta recommendation ships with its `/beta` path intact. Both renderings passed it through the authority-only URL redaction used for configured endpoints, so the doctor advised `https://api.deepseek.com` — a URL where the chat wire still strips `function.strict` — sending the user straight back into the same verdict. The recommendation is a built-in constant, never a configured URL, so no redaction is needed; the JSON report and the human output now emit it verbatim and the test pins the full value. The official_route messages say "DeepSeek-family" instead of "official": the gate also accepts the legacy `api.deepseeki.com` compat host, which docs/REBRAND.md records as recognized but not official — the same distinction the provider_mismatch arm already drew. The `strict_tool_mode` field doc no longer claims `tool_choice` falls back to `auto` when individual tools fail closed: `tool_choice` follows the route gate, not per-tool outcomes — it stays `required` on an in-scope route and falls back only when the route itself is out of scope. Signed-off-by: asto --- crates/tui/src/config.rs | 10 +++++++--- crates/tui/src/lib.rs | 42 ++++++++++++++++++++++++++++------------ 2 files changed, 37 insertions(+), 15 deletions(-) diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index bc28f060a1..6a0ccd33b5 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -3120,9 +3120,13 @@ pub struct Config { /// from the schema (reported in the tool description), non-numeric /// `const` values land on `enum`, and `dependentRequired` tables are /// stripped, per the strict contract. Schemas the strict subset cannot - /// represent faithfully — root alternatives, or required nullables - /// (`nullable` / null-type union branches) — stay non-strict: they keep - /// their original schema and `tool_choice` falls back to `auto`. + /// represent faithfully — root alternatives, required nullables + /// (`nullable` / null-type union branches), or tools whose runtime + /// demands schema-optional parameters (`update_goal`, `send_later`) — + /// stay non-strict: they keep their original schema. `tool_choice` + /// follows the route gate, not per-tool outcomes: it stays `required` + /// on an in-scope route even when some tools fail closed, and falls + /// back to `auto` only when the route itself is out of scope. pub strict_tool_mode: Option, /// Additional user-owned system-prompt sources concatenated in declared /// order (#454). Paths are expanded via `expand_path` so `~` and env vars diff --git a/crates/tui/src/lib.rs b/crates/tui/src/lib.rs index a6fbfc5906..a4f08dcb04 100644 --- a/crates/tui/src/lib.rs +++ b/crates/tui/src/lib.rs @@ -4729,10 +4729,13 @@ async fn run_doctor( strict_icon, strict_tool_mode.message ); if let Some(recommended) = strict_tool_mode.recommended_base_url.as_deref() { - println!( - " Use the {} endpoint for DeepSeek strict schemas.", - crate::doctor::structural_url_authority(recommended) - ); + // The recommendation is a built-in constant, never a configured + // URL, so its path must survive verbatim: the authority-only + // redaction used for configured endpoints would render + // `https://api.deepseek.com/beta` as `https://api.deepseek.com`, + // and the bare host is exactly where the chat wire still strips + // `function.strict`. + println!(" Use the {recommended} endpoint for DeepSeek strict schemas."); } let capability = crate::config::provider_capability(config.api_provider(), &api_target.model); if let Some(alias) = capability.alias_deprecation.as_ref() { @@ -7560,8 +7563,12 @@ fn doctor_strict_tool_mode_status(config: &Config) -> DoctorStrictToolModeStatus enabled: true, status: "official_route", function_strict_sent: true, + // "DeepSeek-family", not "official": the gate also accepts the + // legacy `api.deepseeki.com` compat host, which is not an + // official endpoint (docs/REBRAND.md) — the same wording the + // provider_mismatch arm uses ("recognized"). message: "enabled; the Anthropic wire forwards function.strict \ - untranslated on this official DeepSeek endpoint" + untranslated on this DeepSeek-family endpoint" .to_string(), recommended_base_url: None, }; @@ -7594,7 +7601,10 @@ fn doctor_strict_tool_mode_status(config: &Config) -> DoctorStrictToolModeStatus enabled: true, status: "official_route", function_strict_sent: true, - message: "enabled; the strict tool pipeline acts on this official DeepSeek route" + // "DeepSeek-family": the arm also covers the legacy + // `api.deepseeki.com` compat host, which is recognized but not + // official (docs/REBRAND.md). + message: "enabled; the strict tool pipeline acts on this DeepSeek-family route" .to_string(), recommended_base_url: None, }, @@ -7607,10 +7617,11 @@ fn doctor_strict_tool_mode_report_json(status: &DoctorStrictToolModeStatus) -> s "status": status.status, "function_strict_sent": status.function_strict_sent, "message": status.message, - "recommended_base_url": status - .recommended_base_url - .as_deref() - .map(crate::doctor::structural_url_authority), + // Verbatim, not authority-redacted: the recommendation is a + // built-in constant, and its `/beta` path is the whole point — + // the redacted `https://api.deepseek.com` is a URL where the + // chat wire still strips `function.strict`. + "recommended_base_url": status.recommended_base_url, }) } @@ -14054,9 +14065,13 @@ mod doctor_endpoint_tests { status.recommended_base_url.as_deref(), Some(crate::config::DEFAULT_DEEPSEEK_BASE_URL) ); + // The recommendation ships with its `/beta` path intact — the + // bare `https://api.deepseek.com` authority is exactly where the + // chat wire still strips `function.strict`, so a path-stripped + // recommendation would send the user back into this same verdict. assert_eq!( doctor_strict_tool_mode_report_json(&status)["recommended_base_url"], - "https://api.deepseek.com" + "https://api.deepseek.com/beta" ); } @@ -14143,7 +14158,10 @@ mod doctor_endpoint_tests { assert_eq!(status.status, "official_route"); assert!(status.function_strict_sent); - assert!(status.message.contains("official DeepSeek route")); + // "DeepSeek-family", not "official": the arm also covers the legacy + // api.deepseeki.com compat host, which is recognized but not + // official (docs/REBRAND.md). + assert!(status.message.contains("DeepSeek-family route")); assert!(status.recommended_base_url.is_none()); } From 1165fb30a0ffae613f79896f6f151d2ccdd26fa3 Mon Sep 17 00:00:00 2001 From: asto Date: Mon, 9 Nov 2026 12:00:00 +0800 Subject: [PATCH 7/7] fix(tui): walk the strict gate like its siblings and answer the doctor from every wire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-5 fixes from the 2026-11-09 fresh review: - The composition gate now walks `properties`/`$defs`/`definitions`/`$def` as name maps and skips `enum`/`const`/`default`/`examples` as values, like its sibling walkers: a required property named after a gate keyword (or a definition named `not`, an object-shaped `enum` member, a `default` shaped like a keyword) no longer fails the tool closed on its name or shape alone. - The nine structural/conditional keywords share one constant between the gate and the test loop that pins each of them. - A root map shape (`additionalProperties` schema form or `true` beside no properties) fails closed pre-enforce instead of shipping `strict: true` callable only with `{}`; a typeless node with a non-`false` `additionalProperties` fails closed instead of shipping the authored keyword verbatim (the enforce pass normalizes only object schemas). Authored no-argument roots keep the exemption. - The doctor reports a new `responses_dialect` verdict when the route's model resolves to the Responses endpoint (the wire sends `strict: false` and `tool_choice: "auto"`): reshaping applies, no enforcement is sent, and no base-URL recommendation is offered — the dialect is model-driven, not URL-driven. Exact catalog rows stay authoritative. - The discarded `all_strict` return carries the mixed-set disposition at the only production call site: the documents describe strict mode with every function flagged and leave the server's mixed-set behavior undocumented, so the return stays the lever for an all-or-nothing policy if a probe ever shows one is needed. Signed-off-by: asto --- crates/tui/src/core/engine/tool_catalog.rs | 11 + crates/tui/src/lib.rs | 90 +++++- crates/tui/src/tools/schema_sanitize.rs | 317 ++++++++++++++++++--- 3 files changed, 379 insertions(+), 39 deletions(-) diff --git a/crates/tui/src/core/engine/tool_catalog.rs b/crates/tui/src/core/engine/tool_catalog.rs index 10dd8298eb..4f09804339 100644 --- a/crates/tui/src/core/engine/tool_catalog.rs +++ b/crates/tui/src/core/engine/tool_catalog.rs @@ -883,6 +883,17 @@ pub(crate) fn active_tools_for_request( } let mut tools = active_tools_for_step(catalog, active); if strict_tool_mode { + // `prepare_tools_for_strict_mode` returns whether EVERY tool shipped + // `strict: true`. The return is intentionally not consulted: a + // per-tool fail-closed outcome (a schema that needs an undocumented + // keyword, or a runtime demanding parameters the strict removal + // takes away) ships that tool unchanged without the flag beside + // `strict: true` siblings, while `tool_choice` follows the route + // gate rather than per-tool results. The documents describe strict + // mode with every function flagged and leave the server's mixed-set + // behavior undocumented; if a probe ever shows the endpoint + // rejecting or silently downgrading mixed sets, this discarded + // boolean is the lever for an all-or-nothing policy. crate::tools::schema_sanitize::prepare_tools_for_strict_mode(&mut tools); } Some(tools) diff --git a/crates/tui/src/lib.rs b/crates/tui/src/lib.rs index a4f08dcb04..bc5697b5b9 100644 --- a/crates/tui/src/lib.rs +++ b/crates/tui/src/lib.rs @@ -4719,7 +4719,7 @@ async fn run_doctor( let strict_tool_mode = doctor_strict_tool_mode_status(config); let strict_icon = match strict_tool_mode.status { "ready" | "official_route" => "✓".truecolor(aqua_r, aqua_g, aqua_b), - "fallback_non_beta" | "custom_endpoint" | "provider_mismatch" => { + "fallback_non_beta" | "custom_endpoint" | "provider_mismatch" | "responses_dialect" => { "!".truecolor(sky_r, sky_g, sky_b) } _ => "·".dimmed(), @@ -7470,6 +7470,11 @@ struct DoctorApiTarget { base_url: String, model: String, resolution: DoctorModelResolution, + /// The resolver's endpoint key for the resolved route (`Some` only when + /// `resolution` is `Resolved`). The dialect answers — Anthropic + /// forwarding, Responses `strict: false` — are wire facts keyed on this, + /// not on the URL. + endpoint_key: Option, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -7489,12 +7494,17 @@ fn doctor_api_target(config: &Config) -> DoctorApiTarget { // saved provider models, alias normalization, and roster preference // included — instead of re-deriving a config default that can diverge // from the engine (DGF-01, dogfood 2026-08-02). - let (model, resolution) = + let (model, resolution, endpoint_key) = match crate::route_runtime::resolve_runtime_route(config, provider, None) { - Ok(route) => (route.model.clone(), DoctorModelResolution::Resolved), + Ok(route) => ( + route.model.clone(), + DoctorModelResolution::Resolved, + Some(route.candidate.endpoint().endpoint_key.clone()), + ), Err(_) => ( config.default_model(), DoctorModelResolution::ConfiguredOnly, + None, ), }; DoctorApiTarget { @@ -7502,6 +7512,7 @@ fn doctor_api_target(config: &Config) -> DoctorApiTarget { base_url: config.deepseek_base_url(), model, resolution, + endpoint_key, } } @@ -7573,6 +7584,25 @@ fn doctor_strict_tool_mode_status(config: &Config) -> DoctorStrictToolModeStatus recommended_base_url: None, }; } + // The Responses dialect answers from its wire too: a route whose model + // resolves to the Responses endpoint ships `"strict": false` per tool + // and `tool_choice: "auto"` regardless of which official URL serves it, + // so the reshaping applies with no enforcement. The dialect is + // model-driven, not URL-driven — no base-URL recommendation changes it. + if target.endpoint_key.as_deref() == Some("responses") { + return DoctorStrictToolModeStatus { + enabled: true, + status: "responses_dialect", + function_strict_sent: false, + message: "enabled, but this route's model resolves to the Responses dialect, \ + whose wire sends `strict: false` and `tool_choice: \"auto\"`; schemas \ + are still reshaped for the strict contract, but no strict enforcement \ + is sent — pick a model that resolves to the chat-completions surface \ + to send the flag" + .to_string(), + recommended_base_url: None, + }; + } match known_deepseek_base_url_kind(&target.base_url) { Some(DeepSeekBaseUrlKind::Beta) => DoctorStrictToolModeStatus { enabled: true, @@ -14221,6 +14251,60 @@ mod doctor_endpoint_tests { assert!(status.recommended_base_url.is_none()); } + #[test] + fn strict_tool_mode_doctor_reports_the_responses_dialect_for_versioned_models() { + // A route whose model resolves to the Responses endpoint ships + // `"strict": false` per tool and `tool_choice: "auto"` on the wire, + // so the doctor must not claim the flag was sent — the dialect is + // model-driven, so no base-URL recommendation is offered either. + let config = Config { + strict_tool_mode: Some(true), + providers: Some(crate::config::ProvidersConfig { + deepseek: crate::config::ProviderConfig { + base_url: Some("https://api.deepseek.com/beta".to_string()), + model: Some("deepseek-v3".to_string()), + ..Default::default() + }, + ..Default::default() + }), + ..Default::default() + }; + + let status = doctor_strict_tool_mode_status(&config); + + assert_eq!(status.status, "responses_dialect"); + assert!(!status.function_strict_sent); + assert!(status.message.contains("Responses dialect")); + assert!(status.recommended_base_url.is_none()); + let report = doctor_strict_tool_mode_report_json(&status); + assert_eq!(report["status"], "responses_dialect"); + assert_eq!(report["function_strict_sent"], false); + } + + #[test] + fn strict_tool_mode_doctor_keeps_the_url_verdict_for_a_chat_surface_model() { + // Exact catalog rows stay authoritative: a model whose row resolves + // to the chat surface keeps the URL-kind verdict (`ready` on the + // beta endpoint) even though it is versioned. + let config = Config { + strict_tool_mode: Some(true), + providers: Some(crate::config::ProvidersConfig { + deepseek: crate::config::ProviderConfig { + base_url: Some("https://api.deepseek.com/beta".to_string()), + model: Some("deepseek-v4-pro".to_string()), + ..Default::default() + }, + ..Default::default() + }), + ..Default::default() + }; + + let status = doctor_strict_tool_mode_status(&config); + + assert_eq!(status.status, "ready"); + assert!(status.function_strict_sent); + } + #[test] fn doctor_tls_status_reports_verification_enabled_by_default() { let status = doctor_tls_status(&Config::default()); diff --git a/crates/tui/src/tools/schema_sanitize.rs b/crates/tui/src/tools/schema_sanitize.rs index 3d4064e567..ac18f23585 100644 --- a/crates/tui/src/tools/schema_sanitize.rs +++ b/crates/tui/src/tools/schema_sanitize.rs @@ -89,6 +89,24 @@ pub fn sanitize(schema: &mut Value) { /// Per-action schema honesty would retire this list. const STRICT_UNUSABLE_TOOLS: &[&str] = &["update_goal", "send_later"]; +/// Structural and conditional keywords outside the documented strict +/// subset. Their constraints cannot be expressed by the reshaping — the +/// same family the Moonshot MFJS path fails closed on — so a schema +/// relying on one ships unchanged and non-strict. Shared between the +/// composition gate and the test loop that pins each keyword so the two +/// lists cannot drift. +const STRICT_UNREPRESENTABLE_KEYWORDS: &[&str] = &[ + "not", + "if", + "then", + "else", + "contains", + "prefixItems", + "propertyNames", + "unevaluatedProperties", + "unevaluatedItems", +]; + pub fn prepare_tools_for_strict_mode(tools: &mut [Tool]) -> bool { let mut all_strict = true; for tool in tools { @@ -224,6 +242,29 @@ pub fn sanitize_for_responses(schema: &mut Value) -> Option { fn strict_schema_supported(schema: &Value) -> bool { let mut normalized = schema.clone(); sanitize(&mut normalized); + // A root map shape — no properties of its own beside a `true` or + // schema-valued `additionalProperties` — collapses under the enforce + // pass into `additionalProperties: false` plus an empty `properties` + // map: `strict: true` callable only with `{}` while the tool's + // runtime reads arbitrary keys. The authored no-argument roots that + // stay strict are the ones whose `additionalProperties` is absent or + // already `false` (tools that take no values at all); the same shape + // fails closed one level down through the gate's nested rule, so the + // root fails closed here, while the authored keyword shape is still + // visible. A node the enforce pass would not normalize at all keeps + // the keyword and is caught by the detector below. + if normalized + .get("properties") + .and_then(Value::as_object) + .is_none_or(Map::is_empty) + && normalized + .get("additionalProperties") + .is_some_and(|additional| { + !additional.is_boolean() || additional.as_bool() == Some(true) + }) + { + return false; + } // The enforce pass strips some constraints before the compatibility // gate below can judge them: a null `const` (dropped outright) and a // schema-form draft-7 `dependencies` table (dropped with the name-table @@ -271,6 +312,20 @@ fn has_unrepresentable_stripped_constraint(schema: &Value, ships: bool) -> bool { return true; } + // The enforce pass normalizes `additionalProperties` to `false` only + // on schemas it treats as objects (`type: "object"` or a `properties` + // map). On any other node the authored keyword ships verbatim under a + // strict flag — and a `true` or schema-valued `additionalProperties` + // is map semantics the documented subset does not describe, so the + // node fails closed instead. + if ships + && !is_object_schema(obj) + && obj.get("additionalProperties").is_some_and(|additional| { + !additional.is_boolean() || additional.as_bool() == Some(true) + }) + { + return true; + } let required = required_names(obj); for (key, value) in obj { match key.as_str() { @@ -352,26 +407,14 @@ fn has_strict_incompatible_composition(schema: &Value, is_root: bool) -> bool { if obj.get("const").is_some_and(Value::is_null) { return true; } - // Structural and conditional keywords outside the documented subset - // carry constraints the reshaping cannot express — the same family - // the Moonshot MFJS path fails closed on (`contains`, `prefixItems`, - // `not`, `if`/`then`/`else`, `propertyNames`, `unevaluatedProperties`, - // `unevaluatedItems`, tuple-form `items`). A schema relying on one - // ships unchanged and non-strict rather than reshaped-and-strict. - if obj.keys().any(|key| { - matches!( - key.as_str(), - "not" - | "if" - | "then" - | "else" - | "contains" - | "prefixItems" - | "propertyNames" - | "unevaluatedProperties" - | "unevaluatedItems" - ) - }) { + // Keywords in `STRICT_UNREPRESENTABLE_KEYWORDS` carry constraints + // the reshaping cannot express — the same family the Moonshot MFJS + // path fails closed on. A schema relying on one ships unchanged and + // non-strict rather than reshaped-and-strict. + if obj + .keys() + .any(|key| STRICT_UNREPRESENTABLE_KEYWORDS.contains(&key.as_str())) + { return true; } if obj @@ -400,9 +443,34 @@ fn has_strict_incompatible_composition(schema: &Value, is_root: bool) -> bool { { return true; } - return obj - .values() - .any(|value| has_strict_incompatible_composition(value, false)); + // Walk children like the sibling walkers do: `properties`, + // `$defs`, `definitions`, and `$def` map names to schemas, and + // `enum`/`const`/`default`/`examples` hold literal values — only + // their values are schemas. Judging the name maps themselves as + // schemas made the gate fire on names alone: a required property + // named after a keyword (or a definition named `not`) failed the + // tool closed on its name, and object-shaped `enum` members or + // `default` values failed it on their shape. + for (key, value) in obj { + match key.as_str() { + "properties" | "$defs" | "definitions" | "$def" => { + if let Some(named_schemas) = value.as_object() { + if named_schemas.values().any(|named_schema| { + has_strict_incompatible_composition(named_schema, false) + }) { + return true; + } + } + } + "enum" | "const" | "default" | "examples" => {} + _ => { + if has_strict_incompatible_composition(value, false) { + return true; + } + } + } + } + return false; } schema.as_array().is_some_and(|arr| { arr.iter() @@ -1629,20 +1697,11 @@ mod tests { #[test] fn strict_mode_fails_closed_on_each_structural_keyword() { - // Each member of the structural/conditional family is pinned + // Each member of `STRICT_UNREPRESENTABLE_KEYWORDS` is pinned // individually — a shared fixture let a single-arm regression (a - // typo in one keyword arm) pass silently. - for keyword in [ - "not", - "if", - "then", - "else", - "contains", - "prefixItems", - "propertyNames", - "unevaluatedProperties", - "unevaluatedItems", - ] { + // typo in one keyword arm) pass silently. Iterating the same + // constant the gate checks keeps the two lists from drifting. + for &keyword in STRICT_UNREPRESENTABLE_KEYWORDS { let mut target = serde_json::Map::new(); target.insert("type".into(), json!("string")); target.insert(keyword.into(), json!(true)); @@ -1669,6 +1728,192 @@ mod tests { } } + #[test] + fn strict_mode_keeps_strict_when_a_required_property_is_named_after_a_keyword() { + // The gate walks name maps as name maps: a required property named + // after a structural keyword (or `nullable`) is a name, not a + // keyword occurrence, and must not fail the tool closed on it. + let mut tools = vec![test_tool( + "search", + json!({ + "type": "object", + "properties": { + "needle": {"type": "string"}, + "contains": { + "type": "string", + "description": "substring the result must contain" + } + }, + "required": ["needle", "contains"] + }), + )]; + + assert!(prepare_tools_for_strict_mode(&mut tools)); + assert_eq!(tools[0].strict, Some(true)); + assert_eq!( + tools[0].input_schema["required"], + json!(["contains", "needle"]) + ); + } + + #[test] + fn strict_mode_keeps_strict_when_a_definition_is_named_after_a_keyword() { + // `$defs` is a name map too: a definition named `not` ships when its + // `$ref` resolves, and its name is not a keyword occurrence. A + // definition whose SCHEMA relies on a structural keyword still fails + // the tool closed. + let mut named = vec![test_tool( + "cite", + json!({ + "type": "object", + "properties": {"span": {"$ref": "#/$defs/not"}}, + "required": ["span"], + "$defs": {"not": {"type": "string"}} + }), + )]; + assert!(prepare_tools_for_strict_mode(&mut named)); + assert_eq!(named[0].strict, Some(true)); + assert_eq!( + named[0].input_schema["$defs"]["not"], + json!({"type": "string"}) + ); + + let mut structural = vec![test_tool( + "cite", + json!({ + "type": "object", + "properties": {"span": {"$ref": "#/$defs/span"}}, + "required": ["span"], + "$defs": {"span": {"type": "string", "not": {"type": "string"}}} + }), + )]; + assert!(!prepare_tools_for_strict_mode(&mut structural)); + assert_eq!(structural[0].strict, None); + } + + #[test] + fn strict_mode_keeps_strict_when_literals_hold_schema_shaped_values() { + // `enum` members and `default` values are values, not schemas: an + // object member that happens to carry a keyword-shaped key, or a + // `default` shaped like `{"nullable": true}`, is not a keyword + // occurrence. + let mut tools = vec![test_tool( + "filter", + json!({ + "type": "object", + "properties": { + "mode": {"enum": [{"contains": "abc"}, {"exact": "abc"}]}, + "fallback": { + "type": "object", + "properties": {"limit": {"type": "integer"}}, + "required": ["limit"], + "default": {"nullable": true} + } + }, + "required": ["mode", "fallback"] + }), + )]; + + assert!(prepare_tools_for_strict_mode(&mut tools)); + assert_eq!(tools[0].strict, Some(true)); + assert_eq!( + tools[0].input_schema["properties"]["mode"]["enum"], + json!([{"contains": "abc"}, {"exact": "abc"}]) + ); + } + + #[test] + fn strict_mode_fails_closed_on_a_root_map_shape() { + // A root that declares no properties but takes arbitrary keys + // (`additionalProperties` in schema form or `true`) collapses under + // the enforce pass into `properties: {}` + + // `additionalProperties: false` — `strict: true` callable only with + // `{}` while the runtime reads the keys. It ships unchanged and + // non-strict instead. The authored no-argument roots that stay + // strict are the ones whose `additionalProperties` is absent or + // already `false`. + let mut map_schema = vec![test_tool( + "annotate", + json!({ + "type": "object", + "additionalProperties": {"type": "string"} + }), + )]; + assert!(!prepare_tools_for_strict_mode(&mut map_schema)); + assert_eq!(map_schema[0].strict, None); + assert_eq!( + map_schema[0].input_schema, + json!({ + "type": "object", + "additionalProperties": {"type": "string"} + }), + "the schema ships unchanged" + ); + + let mut open_map = vec![test_tool( + "annotate", + json!({ + "type": "object", + "additionalProperties": true + }), + )]; + assert!(!prepare_tools_for_strict_mode(&mut open_map)); + assert_eq!(open_map[0].strict, None); + + let mut no_arg = vec![test_tool( + "ping", + json!({ + "type": "object", + "additionalProperties": false + }), + )]; + assert!(prepare_tools_for_strict_mode(&mut no_arg)); + assert_eq!(no_arg[0].strict, Some(true)); + } + + #[test] + fn strict_mode_fails_closed_when_a_typeless_map_keyword_would_ship_verbatim() { + // The enforce pass normalizes `additionalProperties` only on object + // schemas; a node without `type`/`properties` keeps the authored + // keyword, and a non-`false` value is map semantics the documented + // subset does not describe — it fails closed instead of shipping the + // keyword under a strict flag. + let mut tools = vec![test_tool( + "tag", + json!({ + "type": "object", + "properties": { + "labels": {"additionalProperties": {"type": "string"}} + }, + "required": ["labels"] + }), + )]; + assert!(!prepare_tools_for_strict_mode(&mut tools)); + assert_eq!(tools[0].strict, None); + } + + #[test] + fn strict_mode_fails_closed_on_a_required_nested_map() { + // One level down the same shape already fails closed through the + // gate's nested rule once the enforce pass injects the empty + // `properties` map — pinned here beside the root variants. + let mut tools = vec![test_tool( + "annotate", + json!({ + "type": "object", + "properties": { + "meta": { + "type": "object", + "additionalProperties": {"type": "string"} + } + }, + "required": ["meta"] + }), + )]; + assert!(!prepare_tools_for_strict_mode(&mut tools)); + assert_eq!(tools[0].strict, None); + } + #[test] fn strict_mode_fails_closed_when_removal_empties_a_nested_object() { // The root-emptied rule has a nested analog: a REQUIRED object whose