codex: address PR review feedback (#18159)

codex-rs/tools/src/json_schema.rs

@@ -283,6 +283,9 @@ fn resolve_json_schema_reference(
     let reference = map.get("$ref")?.as_str()?;
     let pointer = reference.strip_prefix('#')?;
     let mut replacement = root_schema.pointer(pointer)?.clone();
+    if matches!(replacement, JsonValue::Bool(true)) {
+        replacement = JsonValue::Object(serde_json::Map::new());
+    }
     if let JsonValue::Object(replacement_map) = &mut replacement {
         merge_json_schema_objects(replacement_map, map);
     }
@@ -425,10 +428,13 @@ fn merge_schema_types(base_value: &mut JsonValue, overlay_value: &JsonValue) {
         return;
     }
 
-    let merged_types: Vec<String> = base_types
-        .into_iter()
-        .filter(|base_type| overlay_types.contains(base_type))
-        .collect();
+    let merged_types: Vec<String> = [
+        "string", "number", "boolean", "integer", "object", "array", "null",
+    ]
+    .into_iter()
+    .filter(|candidate| type_sets_overlap(&base_types, &overlay_types, candidate))
+    .map(str::to_string)
+    .collect();
     if merged_types.is_empty() {
         return;
     }
@@ -444,6 +450,26 @@ fn merge_schema_types(base_value: &mut JsonValue, overlay_value: &JsonValue) {
     };
 }
 
+fn type_sets_overlap(base_types: &[String], overlay_types: &[String], candidate: &str) -> bool {
+    if base_types.iter().any(|base_type| base_type == candidate)
+        && overlay_types
+            .iter()
+            .any(|overlay_type| overlay_type == candidate)
+    {
+        return true;
+    }
+
+    candidate == "integer"
+        && ((base_types.iter().any(|base_type| base_type == "integer")
+            && overlay_types
+                .iter()
+                .any(|overlay_type| overlay_type == "number"))
+            || (base_types.iter().any(|base_type| base_type == "number")
+                && overlay_types
+                    .iter()
+                    .any(|overlay_type| overlay_type == "integer")))
+}
+
 fn json_schema_type_names(value: &JsonValue) -> Vec<String> {
     match value {
         JsonValue::String(schema_type) => vec![schema_type.clone()],
@@ -499,13 +525,8 @@ fn unwrap_single_variant_combiner(map: &serde_json::Map<String, JsonValue>) -> O
 
         let mut replacement = variants[0].clone();
         if let JsonValue::Object(replacement_map) = &mut replacement {
-            for (key, value) in map {
-                if key != combiner {
-                    replacement_map
-                        .entry(key.clone())
-                        .or_insert_with(|| value.clone());
-                }
-            }
+            merge_json_schema_objects(replacement_map, map);
+            replacement_map.remove(combiner);
         }
         return Some(replacement);
     }

codex-rs/tools/src/json_schema_tests.rs

@@ -609,6 +609,119 @@ fn parse_tool_input_schema_handles_cyclic_local_refs_without_recursing_forever()
     );
 }
 
+#[test]
+fn parse_tool_input_schema_merges_outer_constraints_when_unwrapping_single_variant_combiners() {
+    let schema = parse_tool_input_schema(&serde_json::json!({
+        "type": "object",
+        "properties": {
+            "wrapped": {
+                "allOf": [{
+                    "type": "object",
+                    "properties": {
+                        "a": { "type": "string" }
+                    },
+                    "required": ["a"]
+                }],
+                "properties": {
+                    "b": { "type": "string" }
+                },
+                "required": ["b"]
+            }
+        }
+    }))
+    .expect("parse schema");
+
+    assert_eq!(
+        schema,
+        JsonSchema::object(
+            BTreeMap::from([(
+                "wrapped".to_string(),
+                JsonSchema::object(
+                    BTreeMap::from([
+                        ("a".to_string(), JsonSchema::string(/*description*/ None)),
+                        ("b".to_string(), JsonSchema::string(/*description*/ None)),
+                    ]),
+                    Some(vec!["a".to_string(), "b".to_string()]),
+                    /*additional_properties*/ None,
+                ),
+            )]),
+            /*required*/ None,
+            /*additional_properties*/ None
+        )
+    );
+}
+
+#[test]
+fn parse_tool_input_schema_preserves_sibling_constraints_for_true_ref_targets() {
+    let schema = parse_tool_input_schema(&serde_json::json!({
+        "type": "object",
+        "properties": {
+            "config": {
+                "$ref": "#/$defs/anything",
+                "type": "object",
+                "properties": {
+                    "dateTime": { "type": "string" }
+                },
+                "required": ["dateTime"]
+            }
+        },
+        "$defs": {
+            "anything": true
+        }
+    }))
+    .expect("parse schema");
+
+    assert_eq!(
+        schema,
+        JsonSchema::object(
+            BTreeMap::from([(
+                "config".to_string(),
+                JsonSchema::object(
+                    BTreeMap::from([(
+                        "dateTime".to_string(),
+                        JsonSchema::string(/*description*/ None),
+                    )]),
+                    Some(vec!["dateTime".to_string()]),
+                    /*additional_properties*/ None,
+                ),
+            )]),
+            /*required*/ None,
+            /*additional_properties*/ None
+        )
+    );
+}
+
+#[test]
+fn parse_tool_input_schema_narrows_number_and_integer_type_intersections() {
+    let schema = parse_tool_input_schema(&serde_json::json!({
+        "type": "object",
+        "properties": {
+            "count": {
+                "$ref": "#/$defs/number_like",
+                "type": "integer"
+            }
+        },
+        "$defs": {
+            "number_like": {
+                "type": "number"
+            }
+        }
+    }))
+    .expect("parse schema");
+
+    assert_eq!(
+        schema,
+        JsonSchema::object(
+            BTreeMap::from([(
+                "count".to_string(),
+                JsonSchema::integer(/*description*/ None),
+            )]),
+            /*required*/ None,
+            /*additional_properties*/ None
+        )
+    );
+}
+
 #[test]
 fn parse_tool_input_schema_unwraps_single_variant_all_of_objects() {
     let schema = parse_tool_input_schema(&serde_json::json!({