diff --git a/crates/forge_app/src/utils.rs b/crates/forge_app/src/utils.rs index e7441af990..eedda7ad9b 100644 --- a/crates/forge_app/src/utils.rs +++ b/crates/forge_app/src/utils.rs @@ -230,26 +230,106 @@ fn normalize_schema_keywords( map: &mut serde_json::Map, strict_mode: bool, ) { - for key in ["properties", "$defs", "definitions", "patternProperties"] { - normalize_named_schema_keyword(map, key, strict_mode); + normalize_named_schema_keyword(map, "properties", strict_mode); + + for key in ["items", "additionalProperties", "allOf", "anyOf"] { + normalize_schema_keyword(map, key, strict_mode); + } + + if !strict_mode { + for key in ["$defs", "definitions", "patternProperties"] { + normalize_named_schema_keyword(map, key, strict_mode); + } + + for key in [ + "oneOf", + "prefixItems", + "contains", + "not", + "if", + "then", + "else", + ] { + normalize_schema_keyword(map, key, strict_mode); + } } +} +fn normalize_openai_schema_subset_keywords(map: &mut serde_json::Map) { for key in [ - "items", + "$schema", + "$id", + "$anchor", + "$comment", + "$defs", + "$ref", + "additionalItems", "contains", - "not", + "definitions", + "dependentRequired", + "dependentSchemas", + "deprecated", + "else", + "examples", + "exclusiveMaximum", + "exclusiveMinimum", "if", + "maxContains", + "maxItems", + "maxLength", + "maxProperties", + "maximum", + "minContains", + "minItems", + "minLength", + "minProperties", + "multipleOf", + "not", + "pattern", + "patternProperties", + "prefixItems", + "propertyNames", + "readOnly", "then", - "else", - "additionalProperties", - "additionalItems", + "title", + "unevaluatedItems", "unevaluatedProperties", + "uniqueItems", + "writeOnly", ] { - normalize_schema_keyword(map, key, strict_mode); + map.remove(key); } - for key in ["allOf", "anyOf", "oneOf", "prefixItems"] { - normalize_schema_keyword(map, key, strict_mode); + if let Some(const_value) = map.remove("const") + && !map.contains_key("enum") + { + map.insert( + "enum".to_string(), + serde_json::Value::Array(vec![const_value]), + ); + } +} + +fn normalize_one_of_keyword( + map: &mut serde_json::Map, + strict_mode: bool, +) { + if !strict_mode { + return; + } + + let Some(one_of) = map.remove("oneOf") else { + return; + }; + + match map.get_mut("anyOf") { + Some(serde_json::Value::Array(any_of)) => match one_of { + serde_json::Value::Array(mut one_of) => any_of.append(&mut one_of), + value => any_of.push(value), + }, + _ => { + map.insert("anyOf".to_string(), one_of); + } } } @@ -294,6 +374,19 @@ fn is_object_schema(map: &serde_json::Map) -> bool { || map.contains_key("additionalProperties") } +fn is_array_schema(map: &serde_json::Map) -> bool { + map.get("type") + .and_then(|value| value.as_str()) + .is_some_and(|ty| ty == "array") + || map.contains_key("items") +} + +fn normalize_array_items(map: &mut serde_json::Map, strict_mode: bool) { + if strict_mode && is_array_schema(map) && !map.contains_key("items") { + map.insert("items".to_string(), serde_json::json!({ "type": "string" })); + } +} + fn normalize_additional_properties( map: &mut serde_json::Map, strict_mode: bool, @@ -343,6 +436,10 @@ fn normalize_additional_properties( /// - All objects have a `required` array with all property keys /// - `allOf` branches are merged into a single schema object when strict mode /// is enabled +/// - unsupported JSON Schema keywords are removed, matching Codex's limited +/// Responses API schema subset, while preserving `default` and `minimum` +/// values +/// - `const` is converted to a single-value `enum` /// /// # Arguments /// * `schema` - The JSON schema to normalize (will be modified in place) @@ -353,8 +450,15 @@ pub fn enforce_strict_schema(schema: &mut serde_json::Value, strict_mode: bool) serde_json::Value::Object(map) => { if strict_mode { flatten_all_of_schema(map); - // Remove unsupported keywords that OpenAI/Codex doesn't allow - map.remove("propertyNames"); + // Match Codex's Responses API schema subset. Codex parses MCP + // schemas into a typed representation that only serializes the + // supported OpenAI fields; Forge keeps raw JSON schemas, so we + // explicitly remove unsupported validation/meta keywords here. + normalize_openai_schema_subset_keywords(map); + // Convert oneOf to anyOf because the Responses API rejects oneOf + // in tool parameter schemas while accepting equivalent anyOf + // unions. + normalize_one_of_keyword(map, strict_mode); } normalize_string_format_keyword(map, strict_mode); @@ -404,7 +508,6 @@ pub fn enforce_strict_schema(schema: &mut serde_json::Value, strict_mode: bool) } else if strict_mode && !map.contains_key("type") && !map.contains_key("anyOf") - && !map.contains_key("oneOf") && !map.contains_key("allOf") { // In strict mode, OpenAI/Codex requires all property schemas to have a @@ -417,6 +520,8 @@ pub fn enforce_strict_schema(schema: &mut serde_json::Value, strict_mode: bool) ); } + normalize_array_items(map, strict_mode); + if strict_mode && map .get("nullable") @@ -453,6 +558,39 @@ pub fn enforce_strict_schema(schema: &mut serde_json::Value, strict_mode: bool) } } +fn normalize_gemini_schema_subset_keywords(map: &mut serde_json::Map) { + if let Some(exclusive_minimum) = map.remove("exclusiveMinimum") { + map.entry("minimum".to_string()) + .or_insert(exclusive_minimum); + } + + if let Some(exclusive_maximum) = map.remove("exclusiveMaximum") { + map.entry("maximum".to_string()) + .or_insert(exclusive_maximum); + } + + for key in [ + "$schema", + "$id", + "$anchor", + "$comment", + "$defs", + "$ref", + "additionalItems", + "additionalProperties", + "definitions", + "deprecated", + "examples", + "title", + "unevaluatedItems", + "unevaluatedProperties", + "writeOnly", + "readOnly", + ] { + map.remove(key); + } +} + /// Sanitizes a JSON schema for Google/Gemini API compatibility. /// /// The Gemini API uses OpenAPI 3.0-style function declarations rather than raw @@ -471,9 +609,12 @@ pub fn enforce_strict_schema(schema: &mut serde_json::Value, strict_mode: bool) /// `required` entries that don't have corresponding entries in `properties`. /// The `required` array is filtered to only include fields present in /// `properties`. -/// - **`$schema` is rejected**: Removed if present. -/// - **`additionalProperties` is rejected**: OpenAPI 3.0 doesn't support this -/// keyword. It is removed from all object schemas. +/// - **Unsupported JSON Schema metadata and references are rejected**: +/// `$schema`, `$defs`, `$ref`, `title`, and `additionalProperties` are +/// removed. +/// - **Exclusive bounds are rejected**: `exclusiveMinimum` and +/// `exclusiveMaximum` are converted to `minimum` and `maximum` when the +/// inclusive bound is not already present. /// - **`const` is rejected**: Converted to single-value `enum` (OpenAPI 3.0 /// style). /// - **Nullable types**: `{ "type": ["string", "null"] }` is converted to `{ @@ -481,11 +622,7 @@ pub fn enforce_strict_schema(schema: &mut serde_json::Value, strict_mode: bool) pub fn sanitize_gemini_schema(schema: &mut serde_json::Value) { match schema { serde_json::Value::Object(map) => { - // Remove $schema field (Gemini API doesn't accept it) - map.remove("$schema"); - - // Remove additionalProperties (OpenAPI 3.0 doesn't support it) - map.remove("additionalProperties"); + normalize_gemini_schema_subset_keywords(map); // Convert const to enum if let Some(const_value) = map.remove("const") @@ -811,6 +948,49 @@ mod tests { assert_eq!(schema["required"], json!(["age", "name"])); } + #[test] + fn test_strict_schema_preserves_default_values() { + let mut fixture = json!({ + "type": "object", + "properties": { + "limit": { + "type": "integer", + "description": "Maximum number of records", + "default": 10, + "minimum": 0 + }, + "output_mode": { + "type": "string", + "enum": ["content", "files_with_matches"], + "default": "content" + } + } + }); + + enforce_strict_schema(&mut fixture, true); + + let actual = fixture; + let expected = json!({ + "type": "object", + "properties": { + "limit": { + "type": "integer", + "description": "Maximum number of records", + "default": 10, + "minimum": 0 + }, + "output_mode": { + "type": "string", + "enum": ["content", "files_with_matches"], + "default": "content" + } + }, + "additionalProperties": false, + "required": ["limit", "output_mode"] + }); + assert_eq!(actual, expected); + } + #[test] fn test_typeless_property_gets_string_type_in_strict_mode() { // MCP tool schemas from external servers (e.g. Affine) may define properties @@ -2045,6 +2225,143 @@ mod tests { ); } + #[test] + fn test_gemini_sanitizes_fetch_schema_exclusive_bounds_from_csv_failure() { + let mut fixture = json!({ + "description": "Parameters for fetching a URL.", + "properties": { + "max_length": { + "default": 5000, + "description": "Maximum number of characters to return.", + "exclusiveMaximum": 1000000, + "exclusiveMinimum": 0, + "title": "Max Length", + "type": "integer" + }, + "start_index": { + "default": 0, + "description": "On return output starting at this character index.", + "minimum": 0, + "title": "Start Index", + "type": "integer" + }, + "url": { + "description": "URL to fetch", + "format": "uri", + "minLength": 1, + "title": "Url", + "type": "string" + } + }, + "required": ["url"], + "title": "Fetch", + "type": "object" + }); + + sanitize_gemini_schema(&mut fixture); + + let actual = fixture; + let expected = json!({ + "description": "Parameters for fetching a URL.", + "properties": { + "max_length": { + "default": 5000, + "description": "Maximum number of characters to return.", + "maximum": 1000000, + "minimum": 0, + "type": "integer" + }, + "start_index": { + "default": 0, + "description": "On return output starting at this character index.", + "minimum": 0, + "type": "integer" + }, + "url": { + "description": "URL to fetch", + "format": "uri", + "minLength": 1, + "type": "string" + } + }, + "required": ["url"], + "type": "object" + }); + assert_eq!(actual, expected); + } + + #[test] + fn test_gemini_sanitizes_defs_refs_from_notion_schema_csv_failure() { + let mut fixture = json!({ + "$defs": { + "richTextRequest": { + "type": "object", + "additionalProperties": false, + "properties": { + "text": {"type": "string"} + } + } + }, + "type": "object", + "properties": { + "comment": { + "$ref": "#/$defs/richTextRequest" + }, + "children": { + "type": "array", + "items": { + "$ref": "#/$defs/richTextRequest" + } + } + } + }); + + sanitize_gemini_schema(&mut fixture); + + let actual = fixture; + let expected = json!({ + "type": "object", + "properties": { + "comment": {}, + "children": { + "type": "array", + "items": {} + } + } + }); + assert_eq!(actual, expected); + } + + #[test] + fn test_gemini_adds_array_items_for_fibery_where_csv_failure() { + let mut fixture = json!({ + "type": "object", + "properties": { + "q_where": { + "description": "Filter conditions", + "type": "array" + } + }, + "required": ["q_where"] + }); + + sanitize_gemini_schema(&mut fixture); + + let actual = fixture; + let expected = json!({ + "type": "object", + "properties": { + "q_where": { + "description": "Filter conditions", + "type": "array", + "items": {"type": "string"} + } + }, + "required": ["q_where"] + }); + assert_eq!(actual, expected); + } + #[test] fn test_gemini_nested_const_in_anyof_complex() { let mut schema = json!({ diff --git a/crates/forge_repo/src/provider/openai_responses/request.rs b/crates/forge_repo/src/provider/openai_responses/request.rs index 211e3fdb22..e6680b5c0c 100644 --- a/crates/forge_repo/src/provider/openai_responses/request.rs +++ b/crates/forge_repo/src/provider/openai_responses/request.rs @@ -734,6 +734,189 @@ mod tests { Ok(()) } + #[test] + fn test_codex_tool_parameters_removes_mcp_schema_draft_marker() -> anyhow::Result<()> { + let fixture = schemars::Schema::try_from(json!({ + "$schema": "http://json-schema.org/draft-07/schema#", + "additionalProperties": false, + "type": "object", + "properties": { + "output_mode": { + "description": "Output mode", + "nullable": true, + "type": "string", + "enum": ["content", "files_with_matches", "count", null] + } + }, + "required": ["output_mode"] + })) + .unwrap(); + + let (actual, actual_strict) = codex_tool_parameters(&fixture)?; + + let expected = json!({ + "additionalProperties": false, + "type": "object", + "properties": { + "output_mode": { + "description": "Output mode", + "anyOf": [ + {"type": "string", "enum": ["content", "files_with_matches", "count"]}, + {"type": "null"} + ] + } + }, + "required": ["output_mode"] + }); + let expected_strict = true; + assert_eq!(actual, expected); + assert_eq!(actual_strict, expected_strict); + + Ok(()) + } + + #[test] + fn test_codex_tool_parameters_converts_datadog_metric_query_one_of() -> anyhow::Result<()> { + let fixture = schemars::Schema::try_from(json!({ + "$schema": "http://json-schema.org/draft-07/schema#", + "additionalProperties": false, + "type": "object", + "properties": { + "queries": { + "description": "Array of metric queries.", + "type": "array", + "items": { + "oneOf": [ + {"type": "string"}, + { + "type": "object", + "properties": { + "metric_name": {"type": "string"}, + "space_aggregator": { + "type": "string", + "enum": ["avg", "sum", "min", "max"] + } + } + } + ] + } + } + }, + "required": ["queries"] + })) + .unwrap(); + + let (actual, actual_strict) = codex_tool_parameters(&fixture)?; + + let expected = json!({ + "additionalProperties": false, + "type": "object", + "properties": { + "queries": { + "description": "Array of metric queries.", + "type": "array", + "items": { + "anyOf": [ + {"type": "string"}, + { + "type": "object", + "properties": { + "metric_name": {"type": "string"}, + "space_aggregator": { + "type": "string", + "enum": ["avg", "sum", "min", "max"] + } + }, + "additionalProperties": false, + "required": ["metric_name", "space_aggregator"] + } + ] + } + } + }, + "required": ["queries"] + }); + let expected_strict = true; + assert_eq!(actual, expected); + assert_eq!(actual_strict, expected_strict); + + Ok(()) + } + + #[test] + fn test_codex_tool_parameters_sanitizes_unsupported_schema_keywords() -> anyhow::Result<()> { + let fixture = schemars::Schema::try_from(json!({ + "$schema": "http://json-schema.org/draft-07/schema#", + "$id": "https://example.com/schema.json", + "title": "Unsupported metadata", + "type": "object", + "properties": { + "status": { + "const": "ok", + "default": "ok", + "description": "Status value" + }, + "count": { + "type": "integer", + "minimum": 1, + "maximum": 10, + "multipleOf": 1 + }, + "tags": { + "type": "array", + "prefixItems": [{"type": "string"}], + "minItems": 1, + "uniqueItems": true + }, + "code": { + "type": "string", + "pattern": "^[A-Z]+$", + "minLength": 2, + "maxLength": 8 + } + }, + "propertyNames": {"pattern": "^[a-z_]+$"}, + "patternProperties": { + "^x-": {"type": "string"} + }, + "required": ["status"], + "additionalProperties": false + })) + .unwrap(); + + let (actual, actual_strict) = codex_tool_parameters(&fixture)?; + + let expected = json!({ + "type": "object", + "properties": { + "status": { + "type": "string", + "enum": ["ok"], + "default": "ok", + "description": "Status value" + }, + "count": { + "type": "integer", + "minimum": 1 + }, + "tags": { + "type": "array", + "items": {"type": "string"} + }, + "code": { + "type": "string" + } + }, + "required": ["code", "count", "status", "tags"], + "additionalProperties": false + }); + let expected_strict = true; + assert_eq!(actual, expected); + assert_eq!(actual_strict, expected_strict); + + Ok(()) + } + #[test] fn test_codex_request_tools_snapshot() -> anyhow::Result<()> { // Build a schema that exercises OpenAI strict-mode normalization: diff --git a/crates/forge_repo/src/provider/openai_responses/snapshots/forge_repo__provider__openai_responses__request__tests__openai_responses_all_catalog_tools.snap b/crates/forge_repo/src/provider/openai_responses/snapshots/forge_repo__provider__openai_responses__request__tests__openai_responses_all_catalog_tools.snap index 05b5451959..fe12598443 100644 --- a/crates/forge_repo/src/provider/openai_responses/snapshots/forge_repo__provider__openai_responses__request__tests__openai_responses_all_catalog_tools.snap +++ b/crates/forge_repo/src/provider/openai_responses/snapshots/forge_repo__provider__openai_responses__request__tests__openai_responses_all_catalog_tools.snap @@ -64,7 +64,6 @@ expression: actual.tools "range", "show_line_numbers" ], - "title": "FSRead", "type": "object" }, "strict": true, @@ -94,7 +93,6 @@ expression: actual.tools "file_path", "overwrite" ], - "title": "FSWrite", "type": "object" }, "strict": true, @@ -268,7 +266,6 @@ expression: actual.tools "pattern", "type" ], - "title": "FSSearch", "type": "object" }, "strict": true, @@ -307,7 +304,6 @@ expression: actual.tools "required": [ "queries" ], - "title": "SemanticSearch", "type": "object" }, "strict": true, @@ -327,7 +323,6 @@ expression: actual.tools "required": [ "path" ], - "title": "FSRemove", "type": "object" }, "strict": true, @@ -363,7 +358,6 @@ expression: actual.tools "old_string", "replace_all" ], - "title": "FSPatch", "type": "object" }, "strict": true, @@ -413,7 +407,6 @@ expression: actual.tools "edits", "file_path" ], - "title": "FSMultiPatch", "type": "object" }, "strict": true, @@ -433,7 +426,6 @@ expression: actual.tools "required": [ "path" ], - "title": "FSUndo", "type": "object" }, "strict": true, @@ -497,7 +489,6 @@ expression: actual.tools "env", "keep_ansi" ], - "title": "Shell", "type": "object" }, "strict": true, @@ -530,7 +521,6 @@ expression: actual.tools "raw", "url" ], - "title": "NetFetch", "type": "object" }, "strict": true, @@ -622,7 +612,6 @@ expression: actual.tools "option5", "question" ], - "title": "Followup", "type": "object" }, "strict": true, @@ -652,7 +641,6 @@ expression: actual.tools "plan_name", "version" ], - "title": "PlanCreate", "type": "object" }, "strict": true, @@ -672,7 +660,6 @@ expression: actual.tools "required": [ "name" ], - "title": "SkillFetch", "type": "object" }, "strict": true, @@ -717,7 +704,6 @@ expression: actual.tools "required": [ "todos" ], - "title": "TodoWrite", "type": "object" }, "strict": true, @@ -730,7 +716,6 @@ expression: actual.tools "additionalProperties": false, "properties": {}, "required": [], - "title": "TodoRead", "type": "object" }, "strict": true, @@ -771,7 +756,6 @@ expression: actual.tools "session_id", "tasks" ], - "title": "TaskInput", "type": "object" }, "strict": true,