diff --git a/crates/forge_app/src/dto/anthropic/transforms/enforce_schema.rs b/crates/forge_app/src/dto/anthropic/transforms/enforce_schema.rs index 43c7987e57..18b26ab5e4 100644 --- a/crates/forge_app/src/dto/anthropic/transforms/enforce_schema.rs +++ b/crates/forge_app/src/dto/anthropic/transforms/enforce_schema.rs @@ -3,12 +3,20 @@ use forge_domain::Transformer; use crate::dto::anthropic::{OutputFormat, Request}; use crate::utils::enforce_strict_schema; -/// Transformer that normalizes output_format schema to meet Anthropic API -/// requirements. +/// Transformer that normalizes JSON schemas in the Anthropic request to meet +/// API requirements. /// -/// Anthropic requires that all object types in JSON schemas must explicitly set -/// `additionalProperties: false`. This transformer recursively processes the -/// schema to add this requirement. +/// Two categories of schemas are processed: +/// +/// 1. **`output_format` schema** — Anthropic requires all object types to +/// explicitly set `additionalProperties: false`. +/// +/// 2. **Tool `input_schema`** — each tool's parameter schema is sanitized to +/// remove constructs that produce invalid JSON Schema, such as `null` +/// values in `enum` arrays that contradict the declared `type`. This is +/// essential when routing to Anthropic-compatible endpoints backed by +/// third-party models (e.g. Moonshot/Kimi) whose validators reject such +/// schemas. /// /// # Example /// @@ -54,6 +62,13 @@ impl Transformer for EnforceStrictObjectSchema { } } + // Normalize each tool's input schema. Non-strict mode keeps the + // `nullable` keyword for providers that support it while stripping + // invalid constructs (e.g. null in enum) that strict validators reject. + for tool in &mut request.tools { + enforce_strict_schema(&mut tool.input_schema, false); + } + request } } @@ -64,6 +79,8 @@ mod tests { use schemars::JsonSchema; use serde::Deserialize; + use crate::dto::anthropic::ToolDefinition; + use super::*; #[derive(Deserialize, JsonSchema)] @@ -135,4 +152,40 @@ mod tests { assert_eq!(actual.output_format, None); } + + #[test] + fn test_normalize_tool_schema_strips_null_from_enum() { + // Simulate a tool input schema with a nullable enum that contains + // null in the enum array — exactly what Moonshot/Kimi rejects. + let fixture = Request::default().max_tokens(1u64).tools(vec![ToolDefinition { + name: "fs_search".to_string(), + description: Some("Search files".to_string()), + cache_control: None, + input_schema: serde_json::json!({ + "type": "object", + "properties": { + "output_mode": { + "type": "string", + "enum": ["content", "files_with_matches", "count", null], + "nullable": true + } + } + }), + }]); + + let actual = EnforceStrictObjectSchema.transform(fixture); + + let tool = &actual.tools[0]; + let enum_values = tool + .input_schema + .pointer("/properties/output_mode/enum") + .and_then(|v| v.as_array()) + .unwrap(); + + assert!( + !enum_values.contains(&serde_json::Value::Null), + "enum must not contain null after normalization" + ); + assert_eq!(enum_values.len(), 3); + } } diff --git a/crates/forge_app/src/utils.rs b/crates/forge_app/src/utils.rs index 349157d793..36f2b27dde 100644 --- a/crates/forge_app/src/utils.rs +++ b/crates/forge_app/src/utils.rs @@ -522,6 +522,25 @@ pub fn enforce_strict_schema(schema: &mut serde_json::Value, strict_mode: bool) normalize_array_items(map, strict_mode); + // Always strip null from enum arrays when nullable is set, + // regardless of strict mode. A null enum value contradicts any + // non-null type declaration, producing invalid JSON Schema that + // strict validators (e.g. Moonshot/Kimi) reject. This also guards + // against external MCP schemas that may carry the same + // inconsistency, which are not processed by NormalizeNullable. + if map + .get("nullable") + .and_then(|v| v.as_bool()) + .unwrap_or(false) + { + if let Some(serde_json::Value::Array(enum_values)) = map.get_mut("enum") { + enum_values.retain(|v| !v.is_null()); + } + } + + // In strict mode, convert the nullable keyword into an explicit + // anyOf union because OpenAI's Responses API does not support + // nullable. The enum array has already been cleaned above. if strict_mode && map .get("nullable") @@ -530,10 +549,6 @@ pub fn enforce_strict_schema(schema: &mut serde_json::Value, strict_mode: bool) { map.remove("nullable"); - if let Some(serde_json::Value::Array(enum_values)) = map.get_mut("enum") { - enum_values.retain(|v| !v.is_null()); - } - let description = map.remove("description"); let non_null_branch = serde_json::Value::Object(std::mem::take(map)); let null_branch = serde_json::json!({"type": "null"}); @@ -1327,6 +1342,40 @@ mod tests { assert!(schema["properties"]["output_mode"].get("anyOf").is_none()); } + #[test] + fn test_nullable_enum_null_stripped_in_non_strict_mode() { + // Schemas produced by external MCP tools may carry null in enum + // alongside nullable: true. Even in non-strict mode, null must be + // stripped so the schema stays valid for strict validators like + // Moonshot/Kimi. + let mut schema = json!({ + "type": "object", + "properties": { + "output_mode": { + "nullable": true, + "type": "string", + "enum": ["content", "files_with_matches", "count", null] + } + } + }); + + enforce_strict_schema(&mut schema, false); + + let enum_values = schema["properties"]["output_mode"]["enum"] + .as_array() + .unwrap(); + assert!( + !enum_values.contains(&json!(null)), + "enum must not contain null in non-strict mode" + ); + assert_eq!(enum_values.len(), 3); + // nullable marker should be preserved + assert_eq!( + schema["properties"]["output_mode"]["nullable"], + json!(true) + ); + } + #[test] fn test_schema_valued_additional_properties_is_normalized() { let mut schema = json!({ diff --git a/crates/forge_domain/src/tools/catalog.rs b/crates/forge_domain/src/tools/catalog.rs index ef911357c7..23a06460b2 100644 --- a/crates/forge_domain/src/tools/catalog.rs +++ b/crates/forge_domain/src/tools/catalog.rs @@ -860,7 +860,7 @@ fn normalize_tool_name(name: &ToolName) -> ToolName { impl ToolCatalog { pub fn schema(&self) -> Schema { use schemars::generate::SchemaSettings; - use schemars::transform::{AddNullable, Transform}; + use schemars::transform::Transform; let r#gen = SchemaSettings::default() .with(|s| { @@ -889,8 +889,10 @@ impl ToolCatalog { ToolCatalog::TodoRead(_) => r#gen.into_root_schema_for::(), }; - // Apply transform to add nullable property and remove null from type - AddNullable::default().transform(&mut schema); + // Normalize nullable schemas: add the "nullable": true marker and + // ensure enum arrays stay internally consistent (no null values that + // contradict the declared type). + crate::NormalizeNullable.transform(&mut schema); schema } diff --git a/crates/forge_domain/src/tools/definition/tool_definition.rs b/crates/forge_domain/src/tools/definition/tool_definition.rs index e33a3c005f..37f478792f 100644 --- a/crates/forge_domain/src/tools/definition/tool_definition.rs +++ b/crates/forge_domain/src/tools/definition/tool_definition.rs @@ -3,6 +3,7 @@ use schemars::Schema; use schemars::generate::SchemaGenerator; use schemars::transform::{Transform, transform_subschemas}; use serde::{Deserialize, Serialize}; +use serde_json::Value; use crate::ToolName; @@ -25,6 +26,60 @@ impl Transform for RemoveSchemaTitles { } } +/// A [`Transform`] that marks nullable schemas with `"nullable": true` while +/// keeping `enum` arrays internally consistent. +/// +/// Schemars' built-in [`schemars::transform::AddNullable`] adds the +/// `"nullable": true` keyword and removes `"null"` from the `type` keyword, +/// but it does **not** remove `null` values from `enum` arrays. This leaves +/// schemas where the `enum` array contains `null` while `type` is e.g. +/// `"string"` — an invalid combination that strict JSON Schema validators +/// (such as Moonshot/Kimi) reject. +/// +/// This transform delegates to [`AddNullable`](schemars::transform::AddNullable) +/// for the `type` keyword, then recursively strips `null` from every `enum` +/// array in a schema that carries `"nullable": true`. The `nullable` marker +/// alone fully conveys nullability in OpenAPI 3.0-style dialects, so `null` +/// in `enum` is redundant once the marker is present. +#[derive(Debug, Clone, Default)] +pub struct NormalizeNullable; + +impl Transform for NormalizeNullable { + fn transform(&mut self, schema: &mut Schema) { + // Delegate type-keyword normalization (including subschema recursion) + // to schemars' AddNullable. + schemars::transform::AddNullable::default().transform(schema); + + // Recursively clean up enum arrays left inconsistent by AddNullable. + remove_null_enum_values(schema); + transform_subschemas(&mut remove_null_enum_values, schema); + } +} + +/// Removes `null` entries from the `enum` array of a schema that is marked +/// `nullable: true`. The `nullable` marker already conveys nullability, so +/// keeping `null` in `enum` is redundant and can make the schema invalid when +/// the `type` no longer includes `"null"`. +fn remove_null_enum_values(schema: &mut Schema) { + if let Some(map) = schema.as_object_mut() { + let is_nullable = map + .get("nullable") + .and_then(Value::as_bool) + .unwrap_or(false); + + if is_nullable { + if let Some(Value::Array(enum_values)) = map.get_mut("enum") { + enum_values.retain(|v| !v.is_null()); + + // Drop the enum key if every value was null. + if enum_values.is_empty() { + map.remove("enum"); + } + } + } + } +} + /// Returns a [`SchemaGenerator`] whose settings include [`RemoveSchemaTitles`] /// as a registered transform. /// @@ -155,4 +210,82 @@ mod tests { None ); } + + #[test] + fn test_normalize_nullable_strips_null_from_enum() { + use schemars::json_schema; + + // Simulate what schemars produces for Option: + // type includes "null", and enum array contains null. + let mut schema = json_schema!({ + "type": ["string", "null"], + "enum": ["content", "files_with_matches", "count", null] + }); + + NormalizeNullable.transform(&mut schema); + + let actual = serde_json::to_value(&schema).unwrap(); + + // nullable marker should be present + assert_eq!(actual["nullable"], serde_json::Value::Bool(true)); + + // type should no longer contain "null" + assert_eq!(actual["type"], serde_json::json!("string")); + + // enum must NOT contain null — this is the bug fix + let enum_values = actual["enum"].as_array().unwrap(); + assert!( + !enum_values.contains(&serde_json::Value::Null), + "enum must not contain null after NormalizeNullable" + ); + assert_eq!(enum_values.len(), 3); + } + + #[test] + fn test_normalize_nullable_preserves_non_nullable_enum() { + use schemars::json_schema; + + // A non-nullable enum should be left untouched. + let mut schema = json_schema!({ + "type": "string", + "enum": ["a", "b", "c"] + }); + + let expected = serde_json::to_value(&schema).unwrap(); + + NormalizeNullable.transform(&mut schema); + + let actual = serde_json::to_value(&schema).unwrap(); + assert_eq!(actual, expected); + } + + #[test] + fn test_normalize_nullable_recurses_into_properties() { + use schemars::json_schema; + + // Nullable enum nested inside a properties object. + let mut schema = json_schema!({ + "type": "object", + "properties": { + "mode": { + "type": ["string", "null"], + "enum": ["fast", "slow", null] + } + } + }); + + NormalizeNullable.transform(&mut schema); + + let actual = serde_json::to_value(&schema).unwrap(); + let mode_enum = actual + .pointer("/properties/mode/enum") + .and_then(|v| v.as_array()) + .unwrap(); + + assert!( + !mode_enum.contains(&serde_json::Value::Null), + "nested enum must not contain null" + ); + assert_eq!(mode_enum.len(), 2); + } } diff --git a/crates/forge_domain/src/tools/snapshots/forge_domain__tools__catalog__tests__tool_definition_json.snap b/crates/forge_domain/src/tools/snapshots/forge_domain__tools__catalog__tests__tool_definition_json.snap index c12f16e7f9..768b32d86a 100644 --- a/crates/forge_domain/src/tools/snapshots/forge_domain__tools__catalog__tests__tool_definition_json.snap +++ b/crates/forge_domain/src/tools/snapshots/forge_domain__tools__catalog__tests__tool_definition_json.snap @@ -125,8 +125,7 @@ expression: tools "enum": [ "content", "files_with_matches", - "count", - null + "count" ], "nullable": true },