diff --git a/src/tools/builtin/tool_info.rs b/src/tools/builtin/tool_info.rs index 264547aa..77ee5abe 100644 --- a/src/tools/builtin/tool_info.rs +++ b/src/tools/builtin/tool_info.rs @@ -45,11 +45,23 @@ impl ToolInfoDetail { } fn schema_param_names(schema: &serde_json::Value) -> Vec { - schema - .get("properties") - .and_then(|p| p.as_object()) - .map(|props| props.keys().cloned().collect()) - .unwrap_or_default() + let mut names = std::collections::BTreeSet::new(); + + if let Some(props) = schema.get("properties").and_then(|p| p.as_object()) { + names.extend(props.keys().cloned()); + } + + for key in ["allOf", "oneOf", "anyOf"] { + if let Some(variants) = schema.get(key).and_then(|v| v.as_array()) { + for variant in variants { + if let Some(props) = variant.get("properties").and_then(|p| p.as_object()) { + names.extend(props.keys().cloned()); + } + } + } + } + + names.into_iter().collect() } fn fallback_summary(schema: &serde_json::Value) -> ToolDiscoverySummary { diff --git a/src/tools/coercion.rs b/src/tools/coercion.rs index 34ef0057..518bbe3a 100644 --- a/src/tools/coercion.rs +++ b/src/tools/coercion.rs @@ -1,4 +1,4 @@ -pub(crate) fn prepare_tool_params( +pub fn prepare_tool_params( tool: &dyn crate::tools::tool::Tool, params: &serde_json::Value, ) -> serde_json::Value { @@ -9,14 +9,87 @@ pub(crate) fn prepare_params_for_schema( params: &serde_json::Value, schema: &serde_json::Value, ) -> serde_json::Value { - coerce_value(params, schema) + let resolved = resolve_refs(schema); + coerce_value(params, &resolved) } +// ── $ref resolution ────────────────────────────────────────────────── + +/// Inline all `$ref` pointers in a JSON Schema so downstream coercion +/// operates on a flat, self-contained schema tree. +/// +/// Supports `#/definitions/` and `#/$defs/` (JSON Schema +/// draft-07 and 2020-12 respectively). Unknown `$ref` formats are left +/// unchanged. A depth limit prevents infinite recursion from circular refs. +fn resolve_refs(schema: &serde_json::Value) -> serde_json::Value { + let definitions = schema + .get("definitions") + .or_else(|| schema.get("$defs")) + .cloned() + .unwrap_or(serde_json::Value::Null); + resolve_refs_inner(schema, &definitions, 0) +} + +const MAX_REF_DEPTH: usize = 16; + +fn resolve_refs_inner( + schema: &serde_json::Value, + definitions: &serde_json::Value, + depth: usize, +) -> serde_json::Value { + if depth > MAX_REF_DEPTH { + return schema.clone(); + } + match schema { + serde_json::Value::Object(obj) => { + // If this node is a $ref, resolve it and recurse into the target. + if let Some(ref_str) = obj.get("$ref").and_then(|v| v.as_str()) { + if let Some(target) = resolve_ref_pointer(ref_str, definitions) { + return resolve_refs_inner(&target, definitions, depth + 1); + } + return schema.clone(); + } + + // Recursively resolve refs in all values (skip definitions maps). + let resolved: serde_json::Map = obj + .iter() + .map(|(k, v)| { + if k == "definitions" || k == "$defs" { + (k.clone(), v.clone()) + } else { + (k.clone(), resolve_refs_inner(v, definitions, depth + 1)) + } + }) + .collect(); + serde_json::Value::Object(resolved) + } + serde_json::Value::Array(arr) => serde_json::Value::Array( + arr.iter() + .map(|v| resolve_refs_inner(v, definitions, depth + 1)) + .collect(), + ), + _ => schema.clone(), + } +} + +fn resolve_ref_pointer( + ref_str: &str, + definitions: &serde_json::Value, +) -> Option { + let path = ref_str.strip_prefix("#/")?; + let parts: Vec<&str> = path.split('/').collect(); + if parts.len() == 2 && (parts[0] == "definitions" || parts[0] == "$defs") { + return definitions.get(parts[1]).cloned(); + } + None +} + +// ── Core coercion ──────────────────────────────────────────────────── + fn coerce_value(value: &serde_json::Value, schema: &serde_json::Value) -> serde_json::Value { - // This coercer intentionally handles the concrete schema shapes we expose in - // discovery today. It does not resolve combinators like anyOf/oneOf/allOf or - // references via $ref; those schemas pass through unchanged unless they also - // advertise a directly coercible type/property shape. + // This coercer handles concrete schema shapes including discriminated unions + // (oneOf/anyOf with const or single-element enum discriminators), allOf + // merges, and $ref references (resolved in a pre-pass). if value.is_null() { return value.clone(); } @@ -47,12 +120,35 @@ fn coerce_value(value: &serde_json::Value, schema: &serde_json::Value) -> serde_ return value.clone(); } - let properties = schema.get("properties").and_then(|p| p.as_object()); - let additional_schema = schema.get("additionalProperties").filter(|v| v.is_object()); + let resolved = resolve_effective_properties(schema, obj); + let properties = resolved + .as_ref() + .or_else(|| schema.get("properties").and_then(|p| p.as_object())); + let additional_schema = schema + .get("additionalProperties") + .filter(|v| v.is_object()) + .or_else(|| resolve_additional_properties(schema, obj)); + let required: std::collections::HashSet<&str> = schema + .get("required") + .and_then(|r| r.as_array()) + .map(|arr| arr.iter().filter_map(|v| v.as_str()).collect()) + .unwrap_or_default(); let mut coerced = obj.clone(); for (key, current) in &mut coerced { if let Some(prop_schema) = properties.and_then(|props| props.get(key)) { + // LLMs send "" for optional fields instead of omitting them. + // Coerce to null only when the field is not required AND the schema + // allows null or doesn't allow string — a `type: "string"` field + // may legitimately accept "" as a meaningful value. + if current.as_str() == Some("") + && !required.contains(key.as_str()) + && (schema_allows_type(prop_schema, "null") + || !schema_allows_type(prop_schema, "string")) + { + *current = serde_json::Value::Null; + continue; + } *current = coerce_value(current, prop_schema); continue; } @@ -68,11 +164,179 @@ fn coerce_value(value: &serde_json::Value, schema: &serde_json::Value) -> serde_ value.clone() } +/// When the schema uses `oneOf`, `anyOf`, or `allOf` combinators, build a +/// merged property map that can be used for coercion. +/// +/// - Top-level `properties` are included first (base properties). +/// - `allOf`: merge ALL variants' properties (last-wins on conflicts). +/// - `oneOf`/`anyOf`: find the discriminated match and merge its properties. +/// +/// Returns `None` if no combinators are present or no match is found, so the +/// caller falls back to the existing top-level `properties` lookup. +fn resolve_effective_properties( + schema: &serde_json::Value, + obj: &serde_json::Map, +) -> Option> { + collect_properties(schema, obj, 0) +} + +const MAX_COMBINATOR_DEPTH: usize = 4; + +/// Recursively collect properties from a schema and its combinator variants. +fn collect_properties( + schema: &serde_json::Value, + obj: &serde_json::Map, + depth: usize, +) -> Option> { + if depth > MAX_COMBINATOR_DEPTH { + return None; + } + + let has_combinators = schema.get("allOf").is_some() + || schema.get("oneOf").is_some() + || schema.get("anyOf").is_some(); + + if !has_combinators { + return None; + } + + let mut merged = serde_json::Map::new(); + + // Start with top-level properties + if let Some(props) = schema.get("properties").and_then(|p| p.as_object()) { + merged.extend(props.iter().map(|(k, v)| (k.clone(), v.clone()))); + } + + // allOf: merge ALL variants' properties, recursing into nested combinators + if let Some(all_of) = schema.get("allOf").and_then(|a| a.as_array()) { + for variant in all_of { + if let Some(props) = variant.get("properties").and_then(|p| p.as_object()) { + merged.extend(props.iter().map(|(k, v)| (k.clone(), v.clone()))); + } + // Recurse into variant if it has its own combinators + if let Some(nested) = collect_properties(variant, obj, depth + 1) { + merged.extend(nested); + } + } + } + + // oneOf/anyOf: find discriminated match and merge its properties + for key in ["oneOf", "anyOf"] { + if let Some(variants) = schema.get(key).and_then(|v| v.as_array()) + && let Some(variant) = find_discriminated_variant(variants, obj) + { + if let Some(props) = variant.get("properties").and_then(|p| p.as_object()) { + merged.extend(props.iter().map(|(k, v)| (k.clone(), v.clone()))); + } + // Recurse into matched variant if it has its own combinators + if let Some(nested) = collect_properties(variant, obj, depth + 1) { + merged.extend(nested); + } + } + } + + if merged.is_empty() { + None + } else { + Some(merged) + } +} + +/// Find `additionalProperties` from a matched combinator variant. +/// +/// Checks `allOf` variants first (last-wins), then the matched `oneOf`/`anyOf` +/// variant. Returns `None` if no variant defines `additionalProperties`. +fn resolve_additional_properties<'a>( + schema: &'a serde_json::Value, + obj: &serde_json::Map, +) -> Option<&'a serde_json::Value> { + // allOf: last variant with additionalProperties wins + if let Some(all_of) = schema.get("allOf").and_then(|a| a.as_array()) { + for variant in all_of.iter().rev() { + if let Some(ap) = variant.get("additionalProperties") + && ap.is_object() + { + return Some(ap); + } + } + } + + // oneOf/anyOf: check matched variant + for key in ["oneOf", "anyOf"] { + if let Some(variants) = schema.get(key).and_then(|v| v.as_array()) + && let Some(variant) = find_discriminated_variant(variants, obj) + && let Some(ap) = variant.get("additionalProperties") + && ap.is_object() + { + return Some(ap); + } + } + + None +} + +/// Find a `oneOf`/`anyOf` variant that matches the given object by checking +/// `const`-valued and single-element `enum`-valued properties (discriminators). +/// +/// A variant matches when ALL its discriminator properties match the object's +/// values and at least one such discriminator exists. Returns `None` if no +/// variant matches (safe fallback — no coercion). +fn find_discriminated_variant<'a>( + variants: &'a [serde_json::Value], + obj: &serde_json::Map, +) -> Option<&'a serde_json::Value> { + variants.iter().find(|variant| { + let Some(props) = variant.get("properties").and_then(|p| p.as_object()) else { + return false; + }; + + let mut discriminator_count = 0; + + for (key, prop_schema) in props { + // Check for const discriminator + if let Some(const_val) = prop_schema.get("const") { + discriminator_count += 1; + match obj.get(key) { + Some(v) if v == const_val => {} + _ => return false, + } + continue; + } + + // Check for single-element enum discriminator + if let Some(enum_vals) = prop_schema.get("enum").and_then(|e| e.as_array()) + && enum_vals.len() == 1 + { + discriminator_count += 1; + match obj.get(key) { + Some(v) if v == &enum_vals[0] => {} + _ => return false, + } + } + } + + discriminator_count > 0 + }) +} + fn coerce_string_value(s: &str, schema: &serde_json::Value) -> Option { + // LLMs often send "" instead of null for optional fields. Coerce empty + // strings to null when the schema allows null but not string, or allows + // both but the value is empty (a string field with content "" is kept). + if s.is_empty() && schema_allows_type(schema, "null") && !schema_allows_type(schema, "string") { + return Some(serde_json::Value::Null); + } + if schema_allows_type(schema, "string") { return None; } + // Empty string with no type match — return unchanged since we can't + // determine the intended type. + if s.is_empty() { + return None; + } + if schema_allows_type(schema, "integer") && let Ok(v) = s.parse::() { @@ -114,10 +378,15 @@ fn schema_allows_type(schema: &serde_json::Value, expected: &str) -> bool { Some(serde_json::Value::String(t)) => t == expected, Some(serde_json::Value::Array(types)) => types.iter().any(|t| t.as_str() == Some(expected)), _ => match expected { - "object" => schema - .get("properties") - .and_then(|p| p.as_object()) - .is_some(), + "object" => { + schema + .get("properties") + .and_then(|p| p.as_object()) + .is_some() + || schema.get("oneOf").is_some() + || schema.get("anyOf").is_some() + || schema.get("allOf").is_some() + } "array" => schema.get("items").is_some(), _ => false, }, @@ -325,6 +594,91 @@ mod tests { assert_eq!(result["value"], serde_json::json!("{\"mode\":\"raw\"}")); // safety: test-only assertion } + #[test] + fn coerces_empty_string_to_null_for_nullable_non_required_field() { + let schema = serde_json::json!({ + "type": "object", + "properties": { + "timezone": { "type": ["string", "null"] }, + "schedule": { "type": "string" } + }, + "required": ["schedule"] + }); + let params = serde_json::json!({ + "timezone": "", + "schedule": "0 9 * * *" + }); + + let result = prepare_params_for_schema(¶ms, &schema); + + // Non-required nullable "timezone" with empty string → null + assert_eq!(result["timezone"], serde_json::Value::Null); + // Required "schedule" keeps its value even if empty would be weird + assert_eq!(result["schedule"], serde_json::json!("0 9 * * *")); + } + + #[test] + fn keeps_empty_string_for_non_required_string_only_field() { + let schema = serde_json::json!({ + "type": "object", + "properties": { + "timezone": { "type": "string" }, + "schedule": { "type": "string" } + }, + "required": ["schedule"] + }); + let params = serde_json::json!({ + "timezone": "", + "schedule": "0 9 * * *" + }); + + let result = prepare_params_for_schema(¶ms, &schema); + + // Non-required string-only "timezone" keeps empty string (meaningful value) + assert_eq!(result["timezone"], serde_json::json!("")); + assert_eq!(result["schedule"], serde_json::json!("0 9 * * *")); + } + + #[test] + fn coerces_empty_string_to_null_for_explicit_nullable_type() { + let schema = serde_json::json!({ + "type": "object", + "properties": { + "from_timezone": { "type": ["string", "null"] }, + "operation": { "type": "string" } + }, + "required": ["operation"] + }); + let params = serde_json::json!({ + "from_timezone": "", + "operation": "now" + }); + + let result = prepare_params_for_schema(¶ms, &schema); + + // Nullable type with empty string → null (even if it were required, + // the per-value coercion in coerce_string_value handles this) + assert_eq!(result["from_timezone"], serde_json::Value::Null); + assert_eq!(result["operation"], serde_json::json!("now")); + } + + #[test] + fn keeps_empty_string_for_required_string_only_field() { + let schema = serde_json::json!({ + "type": "object", + "properties": { + "name": { "type": "string" } + }, + "required": ["name"] + }); + let params = serde_json::json!({ "name": "" }); + + let result = prepare_params_for_schema(¶ms, &schema); + + // Required string-only field keeps empty string + assert_eq!(result["name"], serde_json::json!("")); + } + #[test] fn permissive_schema_is_noop() { let schema = serde_json::json!({ @@ -339,6 +693,341 @@ mod tests { assert_eq!(result["count"], serde_json::json!("10")); // safety: test-only assertion } + #[test] + fn coerces_oneof_discriminated_variant() { + let schema = serde_json::json!({ + "oneOf": [ + { + "type": "object", + "properties": { + "action": { "const": "list_repos" }, + "limit": { "type": "integer" }, + "sort": { "type": "string" } + } + }, + { + "type": "object", + "properties": { + "action": { "const": "get_repo" }, + "repo": { "type": "string" } + } + } + ] + }); + let params = serde_json::json!({ + "action": "list_repos", + "limit": "100", + "sort": "stars" + }); + + let result = prepare_params_for_schema(¶ms, &schema); + + assert_eq!(result["action"], serde_json::json!("list_repos")); + assert_eq!(result["limit"], serde_json::json!(100)); + assert_eq!(result["sort"], serde_json::json!("stars")); + } + + #[test] + fn coerces_oneof_with_enum_discriminator() { + let schema = serde_json::json!({ + "oneOf": [ + { + "type": "object", + "properties": { + "mode": { "enum": ["fetch"] }, + "count": { "type": "integer" } + } + }, + { + "type": "object", + "properties": { + "mode": { "enum": ["push"] }, + "force": { "type": "boolean" } + } + } + ] + }); + let params = serde_json::json!({ + "mode": "push", + "force": "true" + }); + + let result = prepare_params_for_schema(¶ms, &schema); + + assert_eq!(result["mode"], serde_json::json!("push")); + assert_eq!(result["force"], serde_json::json!(true)); + } + + #[test] + fn coerces_allof_merged_properties() { + let schema = serde_json::json!({ + "allOf": [ + { + "type": "object", + "properties": { + "page": { "type": "integer" } + } + }, + { + "type": "object", + "properties": { + "per_page": { "type": "integer" }, + "verbose": { "type": "boolean" } + } + } + ] + }); + let params = serde_json::json!({ + "page": "2", + "per_page": "50", + "verbose": "false" + }); + + let result = prepare_params_for_schema(¶ms, &schema); + + assert_eq!(result["page"], serde_json::json!(2)); + assert_eq!(result["per_page"], serde_json::json!(50)); + assert_eq!(result["verbose"], serde_json::json!(false)); + } + + #[test] + fn oneof_no_discriminator_match_is_noop() { + let schema = serde_json::json!({ + "oneOf": [ + { + "type": "object", + "properties": { + "action": { "const": "list_repos" }, + "limit": { "type": "integer" } + } + }, + { + "type": "object", + "properties": { + "action": { "const": "get_repo" }, + "repo": { "type": "string" } + } + } + ] + }); + let params = serde_json::json!({ + "action": "unknown_action", + "limit": "100" + }); + + let result = prepare_params_for_schema(¶ms, &schema); + + // No variant matched, so no coercion happens + assert_eq!(result["limit"], serde_json::json!("100")); + } + + #[test] + fn anyof_without_discriminator_is_noop() { + let schema = serde_json::json!({ + "anyOf": [ + { + "type": "object", + "properties": { + "name": { "type": "string" } + }, + "required": ["name"] + }, + { + "type": "object", + "properties": { + "id": { "type": "integer" } + }, + "required": ["id"] + } + ] + }); + let params = serde_json::json!({ + "id": "42" + }); + + let result = prepare_params_for_schema(¶ms, &schema); + + // No const/enum discriminators, so no variant matches, no coercion + assert_eq!(result["id"], serde_json::json!("42")); + } + + #[test] + fn resolves_ref_and_coerces_referenced_properties() { + let schema = serde_json::json!({ + "type": "object", + "definitions": { + "Pagination": { + "type": "object", + "properties": { + "page": { "type": "integer" }, + "per_page": { "type": "integer" } + } + } + }, + "allOf": [ + { "$ref": "#/definitions/Pagination" }, + { + "type": "object", + "properties": { + "query": { "type": "string" } + } + } + ] + }); + let params = serde_json::json!({ + "page": "2", + "per_page": "50", + "query": "test" + }); + + let result = prepare_params_for_schema(¶ms, &schema); + + assert_eq!(result["page"], serde_json::json!(2)); + assert_eq!(result["per_page"], serde_json::json!(50)); + assert_eq!(result["query"], serde_json::json!("test")); + } + + #[test] + fn resolves_nested_refs_in_oneof_variants() { + let schema = serde_json::json!({ + "type": "object", + "$defs": { + "ListParams": { + "properties": { + "action": { "const": "list" }, + "limit": { "type": "integer" } + } + } + }, + "oneOf": [ + { "$ref": "#/$defs/ListParams" }, + { + "properties": { + "action": { "const": "get" }, + "id": { "type": "integer" } + } + } + ] + }); + let params = serde_json::json!({ + "action": "list", + "limit": "25" + }); + + let result = prepare_params_for_schema(¶ms, &schema); + + assert_eq!(result["limit"], serde_json::json!(25)); + } + + #[test] + fn coerces_nested_combinators_allof_containing_oneof() { + // allOf where one variant is itself a oneOf (nested combinator) + let schema = serde_json::json!({ + "type": "object", + "allOf": [ + { + "properties": { + "version": { "type": "integer" } + } + }, + { + "oneOf": [ + { + "properties": { + "mode": { "const": "fast" }, + "threads": { "type": "integer" } + } + }, + { + "properties": { + "mode": { "const": "safe" }, + "retries": { "type": "integer" } + } + } + ] + } + ] + }); + let params = serde_json::json!({ + "version": "3", + "mode": "fast", + "threads": "8" + }); + + let result = prepare_params_for_schema(¶ms, &schema); + + assert_eq!(result["version"], serde_json::json!(3)); + assert_eq!(result["threads"], serde_json::json!(8)); + } + + #[test] + fn coerces_array_items_with_oneof_discriminator() { + let schema = serde_json::json!({ + "type": "object", + "properties": { + "actions": { + "type": "array", + "items": { + "oneOf": [ + { + "type": "object", + "properties": { + "type": { "const": "move" }, + "distance": { "type": "integer" } + } + }, + { + "type": "object", + "properties": { + "type": { "const": "wait" }, + "seconds": { "type": "number" } + } + } + ] + } + } + } + }); + let params = serde_json::json!({ + "actions": [ + { "type": "move", "distance": "10" }, + { "type": "wait", "seconds": "2.5" } + ] + }); + + let result = prepare_params_for_schema(¶ms, &schema); + + assert_eq!(result["actions"][0]["distance"], serde_json::json!(10)); + assert_eq!(result["actions"][1]["seconds"], serde_json::json!(2.5)); + } + + #[test] + fn circular_ref_does_not_infinite_loop() { + let schema = serde_json::json!({ + "type": "object", + "definitions": { + "Node": { + "type": "object", + "properties": { + "value": { "type": "integer" }, + "child": { "$ref": "#/definitions/Node" } + } + } + }, + "properties": { + "root": { "$ref": "#/definitions/Node" } + } + }); + let params = serde_json::json!({ + "root": { "value": "42" } + }); + + // Should not hang — depth limit stops the recursion + let result = prepare_params_for_schema(¶ms, &schema); + + assert_eq!(result["root"]["value"], serde_json::json!(42)); + } + #[test] fn prepare_tool_params_uses_discovery_schema() { let tool = StubTool { diff --git a/src/tools/schema_validator.rs b/src/tools/schema_validator.rs index df87afa4..3212bbb3 100644 --- a/src/tools/schema_validator.rs +++ b/src/tools/schema_validator.rs @@ -42,11 +42,38 @@ pub fn validate_strict_schema( } } +/// Returns true if the schema uses `oneOf`, `anyOf`, or `allOf` combinators +/// where at least one variant is an object type (has `type: "object"` or `properties`). +fn has_object_combinator_variants(schema: &serde_json::Value) -> bool { + for key in ["oneOf", "anyOf", "allOf"] { + if let Some(variants) = schema.get(key).and_then(|v| v.as_array()) + && variants.iter().any(|v| { + v.get("type").and_then(|t| t.as_str()) == Some("object") + || v.get("properties").is_some() + }) + { + return true; + } + } + false +} + /// Recursively validate an object-typed schema node. fn check_object_schema(schema: &serde_json::Value, path: &str) -> Vec { let mut errors = Vec::new(); - // Rule 1: must have "type": "object" + // Report non-array combinator values as errors. + for key in ["oneOf", "anyOf", "allOf"] { + if let Some(val) = schema.get(key) + && !val.is_array() + { + errors.push(format!("{path}: \"{key}\" must be an array")); + } + } + + let has_combinators = has_object_combinator_variants(schema); + + // Rule 1: must have "type": "object" (unless combinators define the structure) match schema.get("type").and_then(|t| t.as_str()) { Some("object") => {} Some(other) => { @@ -54,16 +81,67 @@ fn check_object_schema(schema: &serde_json::Value, path: &str) -> Vec { return errors; } None => { - errors.push(format!("{path}: missing \"type\": \"object\"")); - return errors; + if !has_combinators { + errors.push(format!("{path}: missing \"type\": \"object\"")); + return errors; + } } } - // Rule 2: must have "properties" as an object + // Validate combinator variants recursively + for key in ["allOf", "oneOf", "anyOf"] { + if let Some(variants) = schema.get(key).and_then(|v| v.as_array()) { + for (i, variant) in variants.iter().enumerate() { + if variant.get("type").and_then(|t| t.as_str()) == Some("object") + || variant.get("properties").is_some() + { + let variant_path = format!("{path}.{key}[{i}]"); + errors.extend(check_object_schema(variant, &variant_path)); + } + } + } + } + + // Rule 2: must have "properties" as an object (unless combinators define them) let properties = match schema.get("properties").and_then(|p| p.as_object()) { Some(p) => p, None => { - errors.push(format!("{path}: missing or non-object \"properties\"")); + if !has_combinators { + errors.push(format!("{path}: missing or non-object \"properties\"")); + return errors; + } + // Combinators define the structure — validate top-level `required` keys + // against merged properties from all combinator variants. + if let Some(required) = schema.get("required").and_then(|r| r.as_array()) { + let mut merged_keys = std::collections::HashSet::new(); + if let Some(all_of) = schema.get("allOf").and_then(|a| a.as_array()) { + for variant in all_of { + if let Some(props) = variant.get("properties").and_then(|p| p.as_object()) { + merged_keys.extend(props.keys().cloned()); + } + } + } + for key in ["oneOf", "anyOf"] { + if let Some(variants) = schema.get(key).and_then(|v| v.as_array()) { + for variant in variants { + if let Some(props) = + variant.get("properties").and_then(|p| p.as_object()) + { + merged_keys.extend(props.keys().cloned()); + } + } + } + } + for req in required { + if let Some(key) = req.as_str() + && !merged_keys.contains(key) + { + errors.push(format!( + "{path}: required key \"{key}\" not found in any combinator variant properties" + )); + } + } + } return errors; } }; diff --git a/src/tools/tool.rs b/src/tools/tool.rs index c361e50c..2e2ee060 100644 --- a/src/tools/tool.rs +++ b/src/tools/tool.rs @@ -462,6 +462,22 @@ pub fn redact_params(params: &serde_json::Value, sensitive: &[&str]) -> serde_js /// on maliciously crafted schemas. const MAX_SCHEMA_DEPTH: usize = 16; +/// Returns true if the schema uses `oneOf`, `anyOf`, or `allOf` combinators +/// where at least one variant is an object type (has `type: "object"` or `properties`). +fn has_object_combinator_variants(schema: &serde_json::Value) -> bool { + for key in ["oneOf", "anyOf", "allOf"] { + if let Some(variants) = schema.get(key).and_then(|v| v.as_array()) + && variants.iter().any(|v| { + v.get("type").and_then(|t| t.as_str()) == Some("object") + || v.get("properties").is_some() + }) + { + return true; + } + } + false +} + pub fn validate_tool_schema(schema: &serde_json::Value, path: &str) -> Vec { validate_tool_schema_inner(schema, path, 0) } @@ -476,7 +492,18 @@ fn validate_tool_schema_inner(schema: &serde_json::Value, path: &str, depth: usi return errors; } - // Rule 1: must have "type": "object" at this level + // Report non-array combinator values as errors. + for key in ["oneOf", "anyOf", "allOf"] { + if let Some(val) = schema.get(key) + && !val.is_array() + { + errors.push(format!("{path}: \"{key}\" must be an array")); + } + } + + let has_combinators = has_object_combinator_variants(schema); + + // Rule 1: must have "type": "object" at this level (unless combinators define the structure) match schema.get("type").and_then(|t| t.as_str()) { Some("object") => {} Some(other) => { @@ -484,16 +511,71 @@ fn validate_tool_schema_inner(schema: &serde_json::Value, path: &str, depth: usi return errors; // Can't check further } None => { - errors.push(format!("{path}: missing \"type\": \"object\"")); - return errors; + if !has_combinators { + errors.push(format!("{path}: missing \"type\": \"object\"")); + return errors; + } } } - // Rule 2: must have "properties" as an object + // Validate combinator variants recursively + for key in ["allOf", "oneOf", "anyOf"] { + if let Some(variants) = schema.get(key).and_then(|v| v.as_array()) { + for (i, variant) in variants.iter().enumerate() { + if variant.get("type").and_then(|t| t.as_str()) == Some("object") + || variant.get("properties").is_some() + { + let variant_path = format!("{path}.{key}[{i}]"); + errors.extend(validate_tool_schema_inner( + variant, + &variant_path, + depth + 1, + )); + } + } + } + } + + // Rule 2: must have "properties" as an object (unless combinators define them) let properties = match schema.get("properties").and_then(|p| p.as_object()) { Some(p) => p, None => { - errors.push(format!("{path}: missing or non-object \"properties\"")); + if !has_combinators { + errors.push(format!("{path}: missing or non-object \"properties\"")); + return errors; + } + // Combinators define the structure — validate top-level `required` keys + // against merged properties from all combinator variants. + if let Some(required) = schema.get("required").and_then(|r| r.as_array()) { + let mut merged_keys = std::collections::HashSet::new(); + if let Some(all_of) = schema.get("allOf").and_then(|a| a.as_array()) { + for variant in all_of { + if let Some(props) = variant.get("properties").and_then(|p| p.as_object()) { + merged_keys.extend(props.keys().cloned()); + } + } + } + for key in ["oneOf", "anyOf"] { + if let Some(variants) = schema.get(key).and_then(|v| v.as_array()) { + for variant in variants { + if let Some(props) = + variant.get("properties").and_then(|p| p.as_object()) + { + merged_keys.extend(props.keys().cloned()); + } + } + } + } + for req in required { + if let Some(key) = req.as_str() + && !merged_keys.contains(key) + { + errors.push(format!( + "{path}: required key \"{key}\" not found in any combinator variant properties" + )); + } + } + } return errors; } }; diff --git a/src/tools/wasm/wrapper.rs b/src/tools/wasm/wrapper.rs index be089dd8..679f33ab 100644 --- a/src/tools/wasm/wrapper.rs +++ b/src/tools/wasm/wrapper.rs @@ -17,6 +17,7 @@ use wasmtime::component::Linker; use wasmtime_wasi::{ResourceTable, WasiCtx, WasiCtxBuilder, WasiView}; use crate::context::JobContext; +use crate::llm::recording::{HttpExchangeRequest, HttpExchangeResponse, HttpInterceptor}; use crate::safety::LeakDetector; use crate::secrets::SecretsStore; use crate::tools::tool::{Tool, ToolError, ToolOutput}; @@ -99,6 +100,9 @@ struct StoreData { /// Dedicated tokio runtime for HTTP requests, lazily initialized. /// Reused across multiple `http_request` calls within one execution. http_runtime: Option, + /// Optional HTTP interceptor for testing — returns canned responses + /// instead of making real requests when set. + http_interceptor: Option>, } impl StoreData { @@ -119,6 +123,7 @@ impl StoreData { credentials, host_credentials, http_runtime: None, + http_interceptor: None, } } @@ -344,6 +349,59 @@ impl near::agent::host::Host for StoreData { ); } let rt = self.http_runtime.as_ref().expect("just initialized"); // safety: is_none branch above guarantees Some + + // If an HTTP interceptor is set (testing), short-circuit with a canned response. + if let Some(interceptor) = &self.http_interceptor { + let interceptor = Arc::clone(interceptor); + let intercept_url = url.clone(); + let intercept_method = method.clone(); + let mut intercept_headers: Vec<(String, String)> = headers + .iter() + .map(|(k, v)| (k.clone(), v.clone())) + .collect(); + intercept_headers.sort_by(|a, b| a.0.cmp(&b.0)); + let intercept_body = body + .as_ref() + .map(|b| String::from_utf8_lossy(b).to_string()); + let intercepted = rt.block_on(async { + let req = HttpExchangeRequest { + method: intercept_method, + url: intercept_url, + headers: intercept_headers, + body: intercept_body, + }; + interceptor.before_request(&req).await + }); + if let Some(resp) = intercepted { + let resp_headers: HashMap = resp + .headers + .iter() + .map(|(k, v)| (k.clone(), v.clone())) + .collect(); + let resp_headers_json = + serde_json::to_string(&resp_headers).unwrap_or_else(|_| "{}".to_string()); + return Ok(near::agent::host::HttpResponse { + status: resp.status, + headers_json: resp_headers_json, + body: resp.body.into_bytes(), + }); + } + } + + // Capture request metadata before headers/body are consumed by the reqwest + // builder. Used for after_response callback when a recording interceptor is set. + let interceptor_req = self.http_interceptor.as_ref().map(|_| HttpExchangeRequest { + method: method.clone(), + url: url.clone(), + headers: headers + .iter() + .map(|(k, v)| (k.clone(), v.clone())) + .collect(), + body: body + .as_ref() + .map(|b| String::from_utf8_lossy(b).to_string()), + }); + let result = rt.block_on(async { let client = reqwest::Client::builder() .connect_timeout(Duration::from_secs(10)) @@ -434,6 +492,51 @@ impl near::agent::host::Host for StoreData { }) }); + // Notify the interceptor about the completed response (recording mode). + // RecordingHttpInterceptor returns None from before_request and captures + // exchanges via after_response, so this path is exercised during trace recording. + if let (Some(interceptor), Some(req), Ok(resp)) = + (&self.http_interceptor, &interceptor_req, &result) + { + let interceptor = Arc::clone(interceptor); + + // Redact credentials from request before passing to the interceptor + // to prevent credential leakage into recorded traces. + let mut redacted_req = req.clone(); + redacted_req.url = self.redact_credentials(&redacted_req.url); + redacted_req.headers = redacted_req + .headers + .into_iter() + .map(|(k, v)| (k, self.redact_credentials(&v))) + .collect(); + redacted_req.body = redacted_req.body.map(|b| self.redact_credentials(&b)); + + let resp_headers: Vec<(String, String)> = + serde_json::from_str::>(&resp.headers_json) + .unwrap_or_default() + .into_iter() + .collect(); + let resp_body = String::from_utf8_lossy(&resp.body).to_string(); + + // Redact credentials from response as well + let redacted_headers: Vec<(String, String)> = resp_headers + .into_iter() + .map(|(k, v)| (k, self.redact_credentials(&v))) + .collect(); + let redacted_body = self.redact_credentials(&resp_body); + + let exchange_resp = HttpExchangeResponse { + status: resp.status, + headers: redacted_headers, + body: redacted_body, + }; + rt.block_on(async { + interceptor + .after_response(&redacted_req, &exchange_resp) + .await; + }); + } + // Redact credentials from error messages before returning to WASM result.map_err(|e| self.redact_credentials(&e)) } @@ -476,6 +579,9 @@ pub struct WasmToolWrapper { secrets_store: Option>, /// OAuth refresh configuration for auto-refreshing expired tokens. oauth_refresh: Option, + /// Optional HTTP interceptor for testing — returns canned responses + /// instead of making real requests when set. + http_interceptor: Option>, } #[derive(Debug, Clone)] @@ -502,23 +608,51 @@ impl WasmToolSchemas { } fn is_permissive_schema(schema: &serde_json::Value) -> bool { - schema + if schema .get("properties") .and_then(|p| p.as_object()) - .is_none_or(|p| p.is_empty()) + .is_some_and(|p| !p.is_empty()) + { + return false; + } + + // Schemas with combinator variants containing properties are not permissive + for key in ["oneOf", "anyOf", "allOf"] { + if let Some(variants) = schema.get(key).and_then(|v| v.as_array()) + && variants.iter().any(|v| { + v.get("properties") + .and_then(|p| p.as_object()) + .is_some_and(|p| !p.is_empty()) + }) + { + return false; + } + } + + true } fn typed_property_count(schema: &serde_json::Value) -> usize { - schema - .get("properties") - .and_then(|p| p.as_object()) - .map(|props| { - props - .values() - .filter(|prop| schema_is_typed_property(prop)) - .count() - }) - .unwrap_or(0) + let mut all_props = serde_json::Map::new(); + + if let Some(props) = schema.get("properties").and_then(|p| p.as_object()) { + all_props.extend(props.iter().map(|(k, v)| (k.clone(), v.clone()))); + } + + for key in ["allOf", "oneOf", "anyOf"] { + if let Some(variants) = schema.get(key).and_then(|v| v.as_array()) { + for variant in variants { + if let Some(props) = variant.get("properties").and_then(|p| p.as_object()) { + all_props.extend(props.iter().map(|(k, v)| (k.clone(), v.clone()))); + } + } + } + } + + all_props + .values() + .filter(|prop| schema_is_typed_property(prop)) + .count() } fn new(discovery: serde_json::Value) -> Self { @@ -564,9 +698,20 @@ impl WasmToolWrapper { credentials: HashMap::new(), secrets_store: None, oauth_refresh: None, + http_interceptor: None, } } + /// Set an HTTP interceptor for testing. + /// + /// When set, WASM tool HTTP requests are routed through the interceptor + /// instead of making real network calls. This allows tests to verify the + /// exact HTTP requests a WASM tool constructs. + pub fn with_http_interceptor(mut self, interceptor: Arc) -> Self { + self.http_interceptor = Some(interceptor); + self + } + /// Override the tool description. pub fn with_description(mut self, description: impl Into) -> Self { self.description = description.into(); @@ -651,12 +796,13 @@ impl WasmToolWrapper { let limits = &self.prepared.limits; // Create store with fresh state (NEAR pattern: fresh instance per call) - let store_data = StoreData::new( + let mut store_data = StoreData::new( limits.memory_bytes, self.capabilities.clone(), self.credentials.clone(), host_credentials, ); + store_data.http_interceptor = self.http_interceptor.clone(); let mut store = Store::new(engine, store_data); // Configure fuel if enabled @@ -872,6 +1018,7 @@ impl Tool for WasmToolWrapper { credentials, secrets_store: None, // Not needed in blocking task oauth_refresh: None, // Already used above for pre-refresh + http_interceptor: self.http_interceptor.clone(), }; tokio::task::spawn_blocking(move || { @@ -1320,15 +1467,33 @@ fn is_private_ip(ip: std::net::IpAddr) -> bool { } fn schema_contains_container_properties(schema: &serde_json::Value) -> bool { - schema + let has_container = |props: &serde_json::Map| { + props + .values() + .any(|prop| schema_declares_type(prop, "array") || schema_declares_type(prop, "object")) + }; + + if schema .get("properties") .and_then(|p| p.as_object()) - .map(|props| { - props.values().any(|prop| { - schema_declares_type(prop, "array") || schema_declares_type(prop, "object") + .is_some_and(has_container) + { + return true; + } + + for key in ["allOf", "oneOf", "anyOf"] { + if let Some(variants) = schema.get(key).and_then(|v| v.as_array()) + && variants.iter().any(|v| { + v.get("properties") + .and_then(|p| p.as_object()) + .is_some_and(has_container) }) - }) - .unwrap_or(false) + { + return true; + } + } + + false } fn schema_declares_type(schema: &serde_json::Value, expected: &str) -> bool { diff --git a/tests/e2e_tool_param_coercion.rs b/tests/e2e_tool_param_coercion.rs index e5258762..cf0672ac 100644 --- a/tests/e2e_tool_param_coercion.rs +++ b/tests/e2e_tool_param_coercion.rs @@ -343,4 +343,412 @@ mod tests { rig.shutdown(); } + + /// Fixture tool that mirrors the github WASM tool's `oneOf` discriminated + /// union schema. Uses `#[serde(tag = "action")]` deserialization — exactly + /// what the real tool does — so if coercion fails the test reproduces: + /// `invalid type: string "100", expected u32` + struct GitHubFixtureTool; + + #[derive(Debug, Deserialize)] + #[serde(tag = "action")] + enum GitHubFixtureAction { + #[serde(rename = "list_issues")] + ListIssues { + owner: String, + repo: String, + #[serde(default)] + state: Option, + #[serde(default)] + limit: Option, + }, + #[serde(rename = "get_issue")] + GetIssue { + owner: String, + repo: String, + issue_number: u32, + }, + #[serde(rename = "list_pull_requests")] + ListPullRequests { + owner: String, + repo: String, + #[serde(default)] + limit: Option, + #[serde(default)] + page: Option, + }, + #[serde(rename = "create_pull_request")] + CreatePullRequest { + owner: String, + repo: String, + title: String, + head: String, + base: String, + #[serde(default)] + draft: Option, + }, + } + + use serde::Deserialize; + + #[async_trait] + impl Tool for GitHubFixtureTool { + fn name(&self) -> &str { + "github_fixture" + } + + fn description(&self) -> &str { + "Fixture mirroring the github WASM tool's oneOf schema" + } + + fn parameters_schema(&self) -> serde_json::Value { + json!({ + "type": "object", + "required": ["action"], + "oneOf": [ + { + "properties": { + "action": { "const": "list_issues" }, + "owner": { "type": "string" }, + "repo": { "type": "string" }, + "state": { "type": "string", "enum": ["open", "closed", "all"] }, + "limit": { "type": "integer", "default": 30 } + }, + "required": ["action", "owner", "repo"] + }, + { + "properties": { + "action": { "const": "get_issue" }, + "owner": { "type": "string" }, + "repo": { "type": "string" }, + "issue_number": { "type": "integer" } + }, + "required": ["action", "owner", "repo", "issue_number"] + }, + { + "properties": { + "action": { "const": "list_pull_requests" }, + "owner": { "type": "string" }, + "repo": { "type": "string" }, + "limit": { "type": "integer", "default": 30 }, + "page": { "type": "integer" } + }, + "required": ["action", "owner", "repo"] + }, + { + "properties": { + "action": { "const": "create_pull_request" }, + "owner": { "type": "string" }, + "repo": { "type": "string" }, + "title": { "type": "string" }, + "head": { "type": "string" }, + "base": { "type": "string" }, + "draft": { "type": "boolean", "default": false } + }, + "required": ["action", "owner", "repo", "title", "head", "base"] + } + ] + }) + } + + async fn execute( + &self, + params: serde_json::Value, + _ctx: &JobContext, + ) -> Result { + // Deserialize exactly like the real github WASM tool does. + // Without coercion, this fails: `invalid type: string "100", expected u32` + let action: GitHubFixtureAction = serde_json::from_value(params).map_err(|e| { + ToolError::InvalidParameters(format!("serde deserialization failed: {e}")) + })?; + + let result = match action { + GitHubFixtureAction::ListIssues { + owner, + repo, + state, + limit, + } => json!({ + "action": "list_issues", + "owner": owner, + "repo": repo, + "state": state.unwrap_or_else(|| "open".to_string()), + "limit": limit.unwrap_or(30), + }), + GitHubFixtureAction::GetIssue { + owner, + repo, + issue_number, + } => json!({ + "action": "get_issue", + "owner": owner, + "repo": repo, + "issue_number": issue_number, + }), + GitHubFixtureAction::ListPullRequests { + owner, + repo, + limit, + page, + } => json!({ + "action": "list_pull_requests", + "owner": owner, + "repo": repo, + "limit": limit.unwrap_or(30), + "page": page.unwrap_or(1), + }), + GitHubFixtureAction::CreatePullRequest { + owner, + repo, + title, + head, + base, + draft, + } => json!({ + "action": "create_pull_request", + "owner": owner, + "repo": repo, + "title": title, + "head": head, + "base": base, + "draft": draft.unwrap_or(false), + }), + }; + + Ok(ToolOutput::success(result, Duration::from_millis(1))) + } + + fn requires_sanitization(&self) -> bool { + false + } + } + + /// Reproduces the exact bug: LLM sends `limit: "100"` and `issue_number: "42"` + /// as strings to a `oneOf` discriminated union schema. Without coercion support + /// for combinators, serde fails with `invalid type: string "100", expected u32`. + #[tokio::test] + async fn e2e_coerces_oneof_discriminated_union_params() { + let trace = LlmTrace { + model_name: "test-coercion-oneof".to_string(), + turns: vec![crate::support::trace_llm::TraceTurn { + user_input: "List issues in nearai/ironclaw with limit 100".to_string(), + steps: vec![ + TraceStep { + request_hint: None, + response: TraceResponse::ToolCalls { + tool_calls: vec![TraceToolCall { + id: "call_gh_list".to_string(), + name: "github_fixture".to_string(), + // LLM sends numeric params as strings — the exact bug + arguments: json!({ + "action": "list_issues", + "owner": "nearai", + "repo": "ironclaw", + "state": "open", + "limit": "100" + }), + }], + input_tokens: 100, + output_tokens: 30, + }, + expected_tool_results: Vec::new(), + }, + TraceStep { + request_hint: None, + response: TraceResponse::Text { + content: "Found issues in nearai/ironclaw with limit 100.".to_string(), + input_tokens: 150, + output_tokens: 20, + }, + expected_tool_results: Vec::new(), + }, + ], + expects: TraceExpects::default(), + }], + memory_snapshot: Vec::new(), + http_exchanges: Vec::new(), + expects: TraceExpects { + tools_used: vec!["github_fixture".to_string()], + all_tools_succeeded: Some(true), + max_tool_calls: Some(1), + min_responses: Some(1), + ..Default::default() + }, + steps: Vec::new(), + }; + + let rig = TestRigBuilder::new() + .with_trace(trace.clone()) + .with_extra_tools(vec![Arc::new(GitHubFixtureTool)]) + .build() + .await; + + rig.send_message("List issues in nearai/ironclaw with limit 100") + .await; + let responses = rig.wait_for_responses(1, Duration::from_secs(15)).await; + + rig.verify_trace_expects(&trace, &responses); + let tool_results = rig.tool_results(); + assert!( + tool_results + .iter() + .any(|(name, preview)| name == "github_fixture" + && preview.contains("\"limit\"") + && preview.contains("100")), + "expected coerced list_issues result, got {tool_results:?}" + ); + + rig.shutdown(); + } + + /// Tests a second oneOf variant with different string-to-integer coercions: + /// `issue_number: "42"` must be coerced to match the `get_issue` variant. + #[tokio::test] + async fn e2e_coerces_oneof_get_issue_variant() { + let trace = LlmTrace { + model_name: "test-coercion-oneof-issue".to_string(), + turns: vec![crate::support::trace_llm::TraceTurn { + user_input: "Get issue 42 from nearai/ironclaw".to_string(), + steps: vec![ + TraceStep { + request_hint: None, + response: TraceResponse::ToolCalls { + tool_calls: vec![TraceToolCall { + id: "call_gh_issue".to_string(), + name: "github_fixture".to_string(), + arguments: json!({ + "action": "get_issue", + "owner": "nearai", + "repo": "ironclaw", + "issue_number": "42" + }), + }], + input_tokens: 80, + output_tokens: 20, + }, + expected_tool_results: Vec::new(), + }, + TraceStep { + request_hint: None, + response: TraceResponse::Text { + content: "Issue 42 retrieved.".to_string(), + input_tokens: 100, + output_tokens: 10, + }, + expected_tool_results: Vec::new(), + }, + ], + expects: TraceExpects::default(), + }], + memory_snapshot: Vec::new(), + http_exchanges: Vec::new(), + expects: TraceExpects { + tools_used: vec!["github_fixture".to_string()], + all_tools_succeeded: Some(true), + max_tool_calls: Some(1), + min_responses: Some(1), + ..Default::default() + }, + steps: Vec::new(), + }; + + let rig = TestRigBuilder::new() + .with_trace(trace.clone()) + .with_extra_tools(vec![Arc::new(GitHubFixtureTool)]) + .build() + .await; + + rig.send_message("Get issue 42 from nearai/ironclaw").await; + let responses = rig.wait_for_responses(1, Duration::from_secs(15)).await; + + rig.verify_trace_expects(&trace, &responses); + let tool_results = rig.tool_results(); + assert!( + tool_results + .iter() + .any(|(name, preview)| name == "github_fixture" + && preview.contains("\"issue_number\"") + && preview.contains("42")), + "expected coerced get_issue result, got {tool_results:?}" + ); + + rig.shutdown(); + } + + /// Tests boolean coercion in a oneOf variant: `draft: "true"` must become + /// a boolean for the `create_pull_request` variant. + #[tokio::test] + async fn e2e_coerces_oneof_boolean_in_variant() { + let trace = LlmTrace { + model_name: "test-coercion-oneof-bool".to_string(), + turns: vec![crate::support::trace_llm::TraceTurn { + user_input: "Create a draft PR".to_string(), + steps: vec![ + TraceStep { + request_hint: None, + response: TraceResponse::ToolCalls { + tool_calls: vec![TraceToolCall { + id: "call_gh_pr".to_string(), + name: "github_fixture".to_string(), + arguments: json!({ + "action": "create_pull_request", + "owner": "nearai", + "repo": "ironclaw", + "title": "Fix coercion", + "head": "fix/coercion", + "base": "main", + "draft": "true" + }), + }], + input_tokens: 90, + output_tokens: 25, + }, + expected_tool_results: Vec::new(), + }, + TraceStep { + request_hint: None, + response: TraceResponse::Text { + content: "Draft PR created.".to_string(), + input_tokens: 110, + output_tokens: 10, + }, + expected_tool_results: Vec::new(), + }, + ], + expects: TraceExpects::default(), + }], + memory_snapshot: Vec::new(), + http_exchanges: Vec::new(), + expects: TraceExpects { + tools_used: vec!["github_fixture".to_string()], + all_tools_succeeded: Some(true), + max_tool_calls: Some(1), + min_responses: Some(1), + ..Default::default() + }, + steps: Vec::new(), + }; + + let rig = TestRigBuilder::new() + .with_trace(trace.clone()) + .with_extra_tools(vec![Arc::new(GitHubFixtureTool)]) + .build() + .await; + + rig.send_message("Create a draft PR").await; + let responses = rig.wait_for_responses(1, Duration::from_secs(15)).await; + + rig.verify_trace_expects(&trace, &responses); + let tool_results = rig.tool_results(); + assert!( + tool_results + .iter() + .any(|(name, preview)| name == "github_fixture" + && preview.contains("\"draft\"") + && preview.contains("true")), + "expected coerced create_pull_request result with draft=true, got {tool_results:?}" + ); + + rig.shutdown(); + } } diff --git a/tests/e2e_wasm_github_coercion.rs b/tests/e2e_wasm_github_coercion.rs new file mode 100644 index 00000000..5277ea91 --- /dev/null +++ b/tests/e2e_wasm_github_coercion.rs @@ -0,0 +1,277 @@ +//! E2E test: real github WASM tool with parameter coercion via TestRig. +//! +//! Loads the compiled github WASM binary into the test rig, replays an LLM +//! trace that sends string-typed numeric params, and verifies the WASM tool +//! constructs the correct HTTP API call via `http_exchanges` in the trace. +//! +//! These tests are `#[ignore]` by default because they require a pre-compiled +//! WASM binary. Build it with: +//! cargo build -p github-tool --target wasm32-wasip2 --release +//! Then run with: +//! cargo test --features libsql --test e2e_wasm_github_coercion -- --ignored + +#[cfg(feature = "libsql")] +mod support; + +/// Note on URL verification: the `ReplayingHttpInterceptor` logs warnings on +/// URL mismatch but still returns the canned response. The real verification is +/// that the tool succeeds end-to-end: coercion produced the correct typed +/// parameters, serde deserialization succeeded, and the WASM tool constructed a +/// valid HTTP request. A URL mismatch warning in logs does not indicate test +/// failure — it is a soft check only. +#[cfg(feature = "libsql")] +mod tests { + use std::time::Duration; + + use serde_json::json; + + use ironclaw::llm::recording::{HttpExchange, HttpExchangeRequest, HttpExchangeResponse}; + + use crate::support::test_rig::TestRigBuilder; + use crate::support::trace_llm::{ + LlmTrace, TraceExpects, TraceResponse, TraceStep, TraceToolCall, + }; + + const GITHUB_WASM: &str = "tools-src/github/target/wasm32-wasip2/release/github_tool.wasm"; + const GITHUB_CAPS: &str = "tools-src/github/github-tool.capabilities.json"; + + fn github_ok(body: &str) -> HttpExchangeResponse { + HttpExchangeResponse { + status: 200, + headers: vec![ + ("content-type".to_string(), "application/json".to_string()), + ("x-ratelimit-remaining".to_string(), "100".to_string()), + ], + body: body.to_string(), + } + } + + /// LLM sends `limit: "50"` (string) to `list_issues`. Coercion converts it + /// to integer, and the WASM tool must call `GET /repos/.../issues?...&per_page=50`. + #[tokio::test] + #[ignore] // requires pre-compiled WASM binary + async fn wasm_github_list_issues_coerces_string_limit() { + let expected_url = + "https://api.github.com/repos/nearai/ironclaw/issues?state=open&per_page=50"; + + let trace = LlmTrace { + model_name: "test-wasm-coercion-list-issues".to_string(), + turns: vec![crate::support::trace_llm::TraceTurn { + user_input: "List issues in nearai/ironclaw with limit 50".to_string(), + steps: vec![ + TraceStep { + request_hint: None, + response: TraceResponse::ToolCalls { + tool_calls: vec![TraceToolCall { + id: "call_gh_1".to_string(), + name: "github".to_string(), + arguments: json!({ + "action": "list_issues", + "owner": "nearai", + "repo": "ironclaw", + "state": "open", + "limit": "50" + }), + }], + input_tokens: 100, + output_tokens: 30, + }, + expected_tool_results: Vec::new(), + }, + TraceStep { + request_hint: None, + response: TraceResponse::Text { + content: "Found 1 issue.".to_string(), + input_tokens: 150, + output_tokens: 10, + }, + expected_tool_results: Vec::new(), + }, + ], + expects: TraceExpects::default(), + }], + memory_snapshot: Vec::new(), + http_exchanges: vec![HttpExchange { + request: HttpExchangeRequest { + method: "GET".to_string(), + url: expected_url.to_string(), + headers: vec![], + body: None, + }, + response: github_ok(r#"[{"number":1,"title":"Test issue","state":"open"}]"#), + }], + expects: TraceExpects { + tools_used: vec!["github".to_string()], + all_tools_succeeded: Some(true), + max_tool_calls: Some(1), + min_responses: Some(1), + ..Default::default() + }, + steps: Vec::new(), + }; + + let rig = TestRigBuilder::new() + .with_trace(trace.clone()) + .with_wasm_tool("github", GITHUB_WASM, Some(GITHUB_CAPS.into())) + .build() + .await; + + rig.send_message("List issues in nearai/ironclaw with limit 50") + .await; + let responses = rig.wait_for_responses(1, Duration::from_secs(15)).await; + rig.verify_trace_expects(&trace, &responses); + + rig.shutdown(); + } + + /// LLM sends `issue_number: "42"` (string) to `get_issue`. Coercion converts + /// it to integer, and the URL must contain `/issues/42`. + #[tokio::test] + #[ignore] // requires pre-compiled WASM binary + async fn wasm_github_get_issue_coerces_string_issue_number() { + let expected_url = "https://api.github.com/repos/nearai/ironclaw/issues/42"; + + let trace = LlmTrace { + model_name: "test-wasm-coercion-get-issue".to_string(), + turns: vec![crate::support::trace_llm::TraceTurn { + user_input: "Get issue 42 from nearai/ironclaw".to_string(), + steps: vec![ + TraceStep { + request_hint: None, + response: TraceResponse::ToolCalls { + tool_calls: vec![TraceToolCall { + id: "call_gh_2".to_string(), + name: "github".to_string(), + arguments: json!({ + "action": "get_issue", + "owner": "nearai", + "repo": "ironclaw", + "issue_number": "42" + }), + }], + input_tokens: 80, + output_tokens: 20, + }, + expected_tool_results: Vec::new(), + }, + TraceStep { + request_hint: None, + response: TraceResponse::Text { + content: "Issue 42 retrieved.".to_string(), + input_tokens: 100, + output_tokens: 10, + }, + expected_tool_results: Vec::new(), + }, + ], + expects: TraceExpects::default(), + }], + memory_snapshot: Vec::new(), + http_exchanges: vec![HttpExchange { + request: HttpExchangeRequest { + method: "GET".to_string(), + url: expected_url.to_string(), + headers: vec![], + body: None, + }, + response: github_ok(r#"{"number":42,"title":"Test","state":"open","body":"desc"}"#), + }], + expects: TraceExpects { + tools_used: vec!["github".to_string()], + all_tools_succeeded: Some(true), + max_tool_calls: Some(1), + min_responses: Some(1), + ..Default::default() + }, + steps: Vec::new(), + }; + + let rig = TestRigBuilder::new() + .with_trace(trace.clone()) + .with_wasm_tool("github", GITHUB_WASM, Some(GITHUB_CAPS.into())) + .build() + .await; + + rig.send_message("Get issue 42 from nearai/ironclaw").await; + let responses = rig.wait_for_responses(1, Duration::from_secs(15)).await; + rig.verify_trace_expects(&trace, &responses); + + rig.shutdown(); + } + + /// LLM sends `limit: "25"` (string) to `list_pull_requests`. URL must + /// contain `per_page=25`. + #[tokio::test] + #[ignore] // requires pre-compiled WASM binary + async fn wasm_github_list_prs_coerces_string_limit() { + let expected_url = + "https://api.github.com/repos/nearai/ironclaw/pulls?state=open&per_page=25"; + + let trace = LlmTrace { + model_name: "test-wasm-coercion-list-prs".to_string(), + turns: vec![crate::support::trace_llm::TraceTurn { + user_input: "List PRs in nearai/ironclaw".to_string(), + steps: vec![ + TraceStep { + request_hint: None, + response: TraceResponse::ToolCalls { + tool_calls: vec![TraceToolCall { + id: "call_gh_3".to_string(), + name: "github".to_string(), + arguments: json!({ + "action": "list_pull_requests", + "owner": "nearai", + "repo": "ironclaw", + "limit": "25" + }), + }], + input_tokens: 80, + output_tokens: 20, + }, + expected_tool_results: Vec::new(), + }, + TraceStep { + request_hint: None, + response: TraceResponse::Text { + content: "Found PRs.".to_string(), + input_tokens: 100, + output_tokens: 10, + }, + expected_tool_results: Vec::new(), + }, + ], + expects: TraceExpects::default(), + }], + memory_snapshot: Vec::new(), + http_exchanges: vec![HttpExchange { + request: HttpExchangeRequest { + method: "GET".to_string(), + url: expected_url.to_string(), + headers: vec![], + body: None, + }, + response: github_ok(r#"[{"number":1,"title":"Test PR","state":"open"}]"#), + }], + expects: TraceExpects { + tools_used: vec!["github".to_string()], + all_tools_succeeded: Some(true), + max_tool_calls: Some(1), + min_responses: Some(1), + ..Default::default() + }, + steps: Vec::new(), + }; + + let rig = TestRigBuilder::new() + .with_trace(trace.clone()) + .with_wasm_tool("github", GITHUB_WASM, Some(GITHUB_CAPS.into())) + .build() + .await; + + rig.send_message("List PRs in nearai/ironclaw").await; + let responses = rig.wait_for_responses(1, Duration::from_secs(15)).await; + rig.verify_trace_expects(&trace, &responses); + + rig.shutdown(); + } +} diff --git a/tests/support/test_rig.rs b/tests/support/test_rig.rs index 55cba5d0..737fd819 100644 --- a/tests/support/test_rig.rs +++ b/tests/support/test_rig.rs @@ -23,7 +23,7 @@ use crate::support::metrics::{ToolInvocation, TraceMetrics}; use crate::support::test_channel::{TestChannel, TestChannelHandle}; use crate::support::trace_llm::{LlmTrace, TraceLlm}; -use ironclaw::llm::recording::{HttpExchange, ReplayingHttpInterceptor}; +use ironclaw::llm::recording::{HttpExchange, HttpInterceptor, ReplayingHttpInterceptor}; // --------------------------------------------------------------------------- // TestRig @@ -343,6 +343,13 @@ impl Drop for TestRig { // TestRigBuilder // --------------------------------------------------------------------------- +/// Specification for loading a real WASM tool in the test rig. +pub struct WasmToolSpec { + pub name: String, + pub wasm_path: std::path::PathBuf, + pub capabilities_path: Option, +} + /// Builder for constructing a `TestRig`. pub struct TestRigBuilder { trace: Option, @@ -354,6 +361,7 @@ pub struct TestRigBuilder { enable_routines: bool, http_exchanges: Vec, extra_tools: Vec>, + wasm_tools: Vec, keep_bootstrap: bool, } @@ -370,10 +378,34 @@ impl TestRigBuilder { enable_routines: false, http_exchanges: Vec::new(), extra_tools: Vec::new(), + wasm_tools: Vec::new(), keep_bootstrap: false, } } + /// Load a real WASM tool binary into the test rig. + /// + /// The tool will be compiled, registered, and wired with the same HTTP + /// interceptor used for `with_http_exchanges()`, so `http_exchanges` in + /// the trace can specify expected requests/responses for WASM tool HTTP calls. + /// + /// If the WASM binary does not exist at build time, the tool is silently + /// skipped (logged as a warning). Tests should use `#[ignore]` or check + /// for the binary in a preamble if the tool is required. + pub fn with_wasm_tool( + mut self, + name: impl Into, + wasm_path: impl Into, + capabilities_path: Option, + ) -> Self { + self.wasm_tools.push(WasmToolSpec { + name: name.into(), + wasm_path: wasm_path.into(), + capabilities_path, + }); + self + } + /// Set the LLM trace to replay. pub fn with_trace(mut self, trace: LlmTrace) -> Self { self.trace = Some(trace); @@ -465,6 +497,7 @@ impl TestRigBuilder { enable_routines, http_exchanges: explicit_http_exchanges, extra_tools, + wasm_tools, keep_bootstrap, } = self; @@ -560,6 +593,20 @@ impl TestRigBuilder { let scheduler_slot: ironclaw::tools::builtin::SchedulerSlot = Arc::new(tokio::sync::RwLock::new(None)); + // Build HTTP interceptor once — shared by both AgentDeps and WASM tools. + let http_interceptor: Option> = { + let exchanges = if explicit_http_exchanges.is_empty() { + trace_http_exchanges + } else { + explicit_http_exchanges + }; + if exchanges.is_empty() { + None + } else { + Some(Arc::new(ReplayingHttpInterceptor::new(exchanges)) as Arc) + } + }; + // 6. Register job tools, routine tools, and extra tools. { // Ensure filesystem/shell dev tools are always available in the @@ -620,6 +667,69 @@ impl TestRigBuilder { for tool in extra_tools { components.tools.register(tool).await; } + + // Register WASM tools with the shared HTTP interceptor. + if !wasm_tools.is_empty() { + use ironclaw::tools::wasm::{ + Capabilities, CapabilitiesFile, WasmRuntimeConfig, WasmToolRuntime, + WasmToolWrapper, + }; + + let runtime = Arc::new( + WasmToolRuntime::new(WasmRuntimeConfig::default()) + .expect("create WASM runtime for test rig"), + ); + + for spec in wasm_tools { + if !spec.wasm_path.exists() { + tracing::warn!( + name = %spec.name, + path = %spec.wasm_path.display(), + "WASM tool binary not found, skipping" + ); + continue; + } + let wasm_bytes = tokio::fs::read(&spec.wasm_path) + .await + .unwrap_or_else(|e| panic!("read {}: {e}", spec.wasm_path.display())); + let (capabilities, description, schema) = + if let Some(cap_path) = &spec.capabilities_path { + if cap_path.exists() { + let cap_bytes = tokio::fs::read(cap_path) + .await + .unwrap_or_else(|e| panic!("read {}: {e}", cap_path.display())); + let cap_file = CapabilitiesFile::from_bytes(&cap_bytes) + .expect("parse capabilities.json"); + ( + cap_file.to_capabilities(), + cap_file.description.clone(), + cap_file.parameters.clone(), + ) + } else { + (Capabilities::default(), None, None) + } + } else { + (Capabilities::default(), None, None) + }; + + let prepared = runtime + .prepare(&spec.name, &wasm_bytes, None) + .await + .unwrap_or_else(|e| panic!("prepare WASM tool '{}': {e}", spec.name)); + let mut wrapper = + WasmToolWrapper::new(Arc::clone(&runtime), prepared, capabilities); + if let Some(desc) = description { + wrapper = wrapper.with_description(desc); + } + if let Some(s) = schema { + wrapper = wrapper.with_schema(s); + } + if let Some(interceptor) = &http_interceptor { + wrapper = wrapper.with_http_interceptor(Arc::clone(interceptor)); + } + components.tools.register(Arc::new(wrapper)).await; + } + } } // Save references for test accessors. @@ -643,20 +753,7 @@ impl TestRigBuilder { hooks: components.hooks, cost_guard: components.cost_guard, sse_tx: None, - http_interceptor: { - // Prefer explicit exchanges from with_http_exchanges(), fall back to trace. - let exchanges = if explicit_http_exchanges.is_empty() { - trace_http_exchanges - } else { - explicit_http_exchanges - }; - if exchanges.is_empty() { - None - } else { - Some(Arc::new(ReplayingHttpInterceptor::new(exchanges)) - as Arc) - } - }, + http_interceptor, transcription: None, document_extraction: None, sandbox_readiness: ironclaw::agent::SandboxReadiness::Available, // tests don't use real Docker