mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-09-02 17:49:20 +00:00
fix: parameter coercion and validation for oneOf/anyOf/allOf schemas (#1397)
* fix: parameter coercion and validation for oneOf/anyOf/allOf schemas WASM extension tools with multi-action schemas (e.g. github extension) fail when the LLM passes numeric parameters as strings because the coercion layer skips JSON Schema combinators. This causes serde deserialization errors like `invalid type: string "100", expected u32`. Add discriminated-union resolution to the coercion layer: for oneOf/anyOf, match the active variant by const or single-element enum discriminators; for allOf, merge all variants' properties. Also propagate combinator awareness to schema validators, WASM wrapper helpers, and tool discovery so they no longer reject or ignore valid combinator-based schemas. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * test: add e2e tests for oneOf discriminated union parameter coercion Add three end-to-end tests using a fixture tool that mirrors the github WASM tool's oneOf schema with #[serde(tag = "action")] deserialization. Each test sends string-typed numeric/boolean params through the full agent loop, verifying that coercion resolves them before serde runs: - list_issues: limit "100" → 100 (integer in oneOf variant) - get_issue: issue_number "42" → 42 (integer in different variant) - create_pull_request: draft "true" → true (boolean in variant) Without the coercion fix these fail with: invalid type: string "100", expected u32 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * test: add real WASM github tool e2e tests with HTTP interception Load the actual compiled github WASM binary, send params with string-typed numbers through the coercion layer, and verify the WASM tool constructs correct HTTP API calls via a new HTTP interceptor in the WASM wrapper. Changes: - Add `http_interceptor` field to `StoreData` and `WasmToolWrapper` so WASM tool HTTP requests can be captured/mocked in tests - Make `prepare_tool_params` and `coercion` module public for integration tests - Add 3 e2e tests loading the real github WASM binary: - list_issues: `limit: "50"` → URL contains `per_page=50` - get_issue: `issue_number: "42"` → URL contains `/issues/42` - list_pull_requests: `limit: "25"` → URL contains `per_page=25` Tests gracefully skip if the WASM binary isn't compiled. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * refactor: simplify WASM e2e tests to use TestRig with with_wasm_tool() Replace the manual WasmToolWrapper construction with TestRig integration: - Add `with_wasm_tool(name, wasm_path, capabilities_path)` to TestRigBuilder that loads real WASM binaries and wires the shared HTTP interceptor - Build the HTTP interceptor before tool registration so it can be shared between AgentDeps and WASM tool wrappers - Rewrite github WASM e2e tests to use the standard trace pattern: TraceLlm sends tool calls with string params, http_exchanges specify expected outgoing requests and canned responses The test code is now identical to other trace-based e2e tests — no custom interceptors or manual WASM construction needed. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: address review comments on combinator schema support - Validate `has_combinators` checks array type (`.as_array().is_some()`) instead of bare `.is_some()` to reject malformed `{ "oneOf": {} }` - Validate top-level `required` keys against merged combinator variant properties when no top-level `properties` exists (both validators) - Deduplicate oneOf/anyOf handling into single loop in coercion.rs - Revert `pub mod coercion` to private; only re-export `prepare_tool_params` - Call `after_response` on interceptor after real HTTP when `before_request` returns None (recording mode correctness) - Fix formatting (CI failure) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: address second round of review comments - Fix headers deserialization bug: deserialize resp.headers_json as HashMap<String, String> then convert to Vec, not directly as Vec - Sort interceptor headers for deterministic trace fixtures - Update after_response comment: RecordingHttpInterceptor does exercise this path (returns None from before_request) - Mark WASM tests #[ignore] instead of silent skip — avoids false-green CI while keeping them runnable with --ignored - Fix with_wasm_tool signature: Option<PathBuf> instead of Option<impl Into<PathBuf>> which doesn't compile in nested position - Fix with_wasm_tool doc comment to match actual behavior - Revert prepare_tool_params to pub(crate) — no longer needed publicly Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: coerce empty strings to null for optional tool parameters LLMs often send "" instead of null/omitting optional parameters, causing parse errors in tools that expect typed values (e.g., timezone, schedule). PR #1127 fixed this per-field in the time tool. This commit adds dispatcher-level coercion so all tools benefit: - Non-required properties with value "" are coerced to null at the object level (based on the schema's `required` array) - Explicitly nullable schemas (`type: ["string", "null"]`) coerce "" to null in the per-value coercion path - Required string-only fields keep "" unchanged Closes #755 Co-Authored-By: spiritj <[email protected]> Co-Authored-By: Xing Ji <[email protected]> Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * feat: complete coercion coverage for $ref, nested combinators, and additionalProperties Close remaining coercion gaps so 3rd-party tools (MCP servers, complex WASM tools) work correctly: - $ref resolution: inline all #/definitions/<name> and #/$defs/<name> references in a pre-pass before coercion, with depth limit (16) for circular ref safety - Nested combinators: resolve_effective_properties now recurses into variants that themselves contain allOf/oneOf/anyOf (depth limit 4) - additionalProperties inheritance: check allOf variants and matched oneOf/anyOf variant for additionalProperties schemas New tests: - resolves_ref_and_coerces_referenced_properties - resolves_nested_refs_in_oneof_variants - coerces_nested_combinators_allof_containing_oneof - coerces_array_items_with_oneof_discriminator - circular_ref_does_not_infinite_loop Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: address third round of review comments - Validators: tighten has_combinators to require at least one object-typed variant (has type:"object" or properties), rejecting non-object combinator schemas like { "oneOf": [{"type":"integer"}] } - Empty-string coercion: only coerce "" → null when schema allows null or doesn't allow string; pure type:"string" fields keep "" as meaningful - Fix comment: "coerce to null" → "return unchanged" for empty strings with no type match (code returns None, not null) - Redact credentials before passing to after_response interceptor to prevent secret leakage into recorded trace files - Switch to tokio::fs::read for async WASM binary loading in test rig - Add doc comment explaining soft URL check in WASM e2e tests Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * ci: retrigger after staging merge [skip-regression-check] * fix: merge staging, report non-array combinator values as errors Merge latest staging to fix CI (missing fallback_deliverable field). Add explicit error reporting when oneOf/anyOf/allOf values are not arrays in both strict and lenient validators. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: recurse into combinator variants that have properties but no explicit type Both validators only recursed into variants with `type: "object"`, missing variants that define `properties` without an explicit type (common in allOf patterns). Now recurse when variant has either. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]> Co-authored-by: spiritj <[email protected]> Co-authored-by: Xing Ji <[email protected]>
This commit is contained in:
Illia Polosukhin
co-authored by
Claude Opus 4.6
spiritj
Xing Ji
Claude Opus 4.6
spiritj
Xing Ji
parent
6232609080
commit
8ad7d78a70
+112
-15
@@ -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<std::path::PathBuf>,
|
||||
}
|
||||
|
||||
/// Builder for constructing a `TestRig`.
|
||||
pub struct TestRigBuilder {
|
||||
trace: Option<LlmTrace>,
|
||||
@@ -354,6 +361,7 @@ pub struct TestRigBuilder {
|
||||
enable_routines: bool,
|
||||
http_exchanges: Vec<HttpExchange>,
|
||||
extra_tools: Vec<Arc<dyn Tool>>,
|
||||
wasm_tools: Vec<WasmToolSpec>,
|
||||
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<String>,
|
||||
wasm_path: impl Into<std::path::PathBuf>,
|
||||
capabilities_path: Option<std::path::PathBuf>,
|
||||
) -> 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<Arc<dyn HttpInterceptor>> = {
|
||||
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<dyn HttpInterceptor>)
|
||||
}
|
||||
};
|
||||
|
||||
// 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<dyn ironclaw::llm::recording::HttpInterceptor>)
|
||||
}
|
||||
},
|
||||
http_interceptor,
|
||||
transcription: None,
|
||||
document_extraction: None,
|
||||
sandbox_readiness: ironclaw::agent::SandboxReadiness::Available, // tests don't use real Docker
|
||||
|
||||
Reference in New Issue
Block a user