fix: parameter coercion and validation for oneOf/anyOf/allOf schemas - #1397
Conversation
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) <noreply@anthropic.com>
github-actions
Bot
added
scope: agent
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system's ability to handle complex JSON schemas, particularly for tool parameters, by introducing advanced coercion and validation logic for Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves support for complex JSON schemas using oneOf, anyOf, and allOf across coercion, validation, and tool information components. The changes are well-structured and include new tests for the added functionality. I've identified a couple of areas for improvement: one for code deduplication to enhance maintainability, aligning with best practices for refactoring duplicated code, and a more critical issue where schema validation for required properties is incomplete when properties are defined within combinators like allOf. Addressing these will make the schema validation more robust.
| 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; | ||
| } | ||
| }; |
There was a problem hiding this comment.
This validation logic for properties is incomplete. If a schema uses allOf to define properties in its variants instead of a top-level properties block, this function returns early and skips the validation of required fields. This can lead to invalid schemas being accepted. You should collect properties from allOf variants and validate required keys against this merged set of properties.
There was a problem hiding this comment.
Resolved in commits 0f2f8c1, bfeb0e4, e9f640d, 987bea3. Validators now: check .as_array() (not .is_some()), report non-array combinator values as errors, require object-typed variants via has_object_combinator_variants(), validate required keys against merged combinator properties, and recurse into variants with properties even without explicit type: "object".
| 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; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Similar to schema_validator.rs, this validation logic is incomplete. If properties are defined within allOf variants instead of at the top level, this function returns early, and the validation for required properties is skipped. This could allow invalid tool schemas. The required keys should be checked against the effective properties, including those from allOf variants.
There was a problem hiding this comment.
Resolved — same fixes as schema_validator.rs. See commits 0f2f8c1, bfeb0e4, e9f640d, 987bea3.
| // oneOf: find discriminated match | ||
| if let Some(one_of) = schema.get("oneOf").and_then(|o| o.as_array()) | ||
| && let Some(variant) = find_discriminated_variant(one_of, obj) | ||
| && let Some(props) = variant.get("properties").and_then(|p| p.as_object()) | ||
| { | ||
| merged.extend(props.iter().map(|(k, v)| (k.clone(), v.clone()))); | ||
| } | ||
|
|
||
| // anyOf: find discriminated match | ||
| if let Some(any_of) = schema.get("anyOf").and_then(|a| a.as_array()) | ||
| && let Some(variant) = find_discriminated_variant(any_of, obj) | ||
| && let Some(props) = variant.get("properties").and_then(|p| p.as_object()) | ||
| { | ||
| merged.extend(props.iter().map(|(k, v)| (k.clone(), v.clone()))); | ||
| } |
There was a problem hiding this comment.
The logic for handling oneOf and anyOf is duplicated. You can refactor this into a loop to improve maintainability and reduce code duplication. This aligns with the guideline to prefer refactoring duplicated code.
| // oneOf: find discriminated match | |
| if let Some(one_of) = schema.get("oneOf").and_then(|o| o.as_array()) | |
| && let Some(variant) = find_discriminated_variant(one_of, obj) | |
| && let Some(props) = variant.get("properties").and_then(|p| p.as_object()) | |
| { | |
| merged.extend(props.iter().map(|(k, v)| (k.clone(), v.clone()))); | |
| } | |
| // anyOf: find discriminated match | |
| if let Some(any_of) = schema.get("anyOf").and_then(|a| a.as_array()) | |
| && let Some(variant) = find_discriminated_variant(any_of, obj) | |
| && let Some(props) = variant.get("properties").and_then(|p| p.as_object()) | |
| { | |
| merged.extend(props.iter().map(|(k, v)| (k.clone(), v.clone()))); | |
| } | |
| // oneOf & anyOf: find discriminated match | |
| 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(props) = variant.get("properties").and_then(|p| p.as_object()) | |
| { | |
| merged.extend(props.iter().map(|(k, v)| (k.clone(), v.clone()))); | |
| } | |
| } |
References
- When an issue is found in duplicated code, prefer refactoring into a shared function over applying localized fixes.
There was a problem hiding this comment.
Resolved. Empty-string coercion tightened in bfeb0e4: only coerces to null when schema allows null or doesn't allow string. Comment fixed to say 'return unchanged'. oneOf/anyOf deduplicated into loop in 0f2f8c1.
There was a problem hiding this comment.
Pull request overview
This PR broadens JSON-schema handling for tool parameters (coercion + validation) to support oneOf/anyOf/allOf-style schemas, while also introducing a significant Web UI “Settings” rework and adding crash-recovery logic/tests for dispatched routine runs.
Changes:
- Add combinator-aware parameter coercion and schema validation so tools can define structure via
oneOf/anyOf/allOf. - Rework the web frontend to consolidate Extensions/Skills into a Settings tab with subtabs, settings import/export, and a custom confirm modal.
- Add DB API + routine-engine reconciliation for orphaned dispatched routine runs, plus integration tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ws_gateway_integration.rs | Updates GatewayState construction for new active_config field. |
| tests/support/gateway_workflow_harness.rs | Updates GatewayState construction for new active_config field. |
| tests/openai_compat_integration.rs | Updates GatewayState construction for new active_config field. |
| tests/e2e/scenarios/test_wasm_lifecycle.py | Adjusts navigation to Settings > Extensions subtab. |
| tests/e2e/scenarios/test_skills.py | Adds Settings > Skills navigation helper and updates removal flow for custom modal. |
| tests/e2e/scenarios/test_extensions.py | Updates navigation + mocks to new Settings subtabs; adapts removal/auth flows to custom modal and new layout. |
| tests/e2e/helpers.py | Adds selectors for settings subtabs/panels and confirm modal; updates top-level tabs list. |
| tests/dispatched_routine_run_tests.rs | New libsql-gated integration tests for dispatched routine run tracking / reconciliation. |
| src/tools/wasm/wrapper.rs | Makes permissiveness/container-property checks and typed-property counting combinator-aware. |
| src/tools/tool.rs | Updates tool schema runtime validation to allow combinator-based object schemas and recurse into object variants. |
| src/tools/schema_validator.rs | Updates strict schema validation to allow combinator-based object schemas and recurse into object variants. |
| src/tools/execute.rs | Removes debug_assert on empty tool name; strengthens test to assert graceful NotFound. |
| src/tools/coercion.rs | Adds combinator-aware coercion (discriminated oneOf/anyOf, allOf merges) + new unit tests. |
| src/tools/builtin/tool_info.rs | Collects parameter names from combinator variants for discovery output completeness. |
| src/tools/builtin/routine.rs | Caches discovery schemas with OnceLock to avoid repeated construction. |
| src/main.rs | Ensures WASM channels directory exists; injects ActiveConfigSnapshot into gateway. |
| src/history/store.rs | Adds list_dispatched_routine_runs query for postgres-backed Store. |
| src/db/postgres.rs | Exposes list_dispatched_routine_runs via RoutineStore for Postgres backend. |
| src/db/mod.rs | Extends RoutineStore trait with list_dispatched_routine_runs. |
| src/db/libsql/routines.rs | Implements list_dispatched_routine_runs for libsql backend. |
| src/context/state.rs | Removes debug_assert around state transitions (keeps runtime error). |
| src/channels/web/ws.rs | Updates test GatewayState construction for new active_config field. |
| src/channels/web/test_helpers.rs | Updates test GatewayState construction for new active_config field. |
| src/channels/web/static/style.css | Introduces Settings layout styling, new modal styling, and visual refinements. |
| src/channels/web/static/index.html | Replaces Extensions/Skills top-level tabs with Settings tab + subtabs; adds confirm modal markup. |
| src/channels/web/static/i18n/zh-CN.js | Adds Settings strings; updates extensions wording; removes tools table strings. |
| src/channels/web/static/i18n/en.js | Adds Settings strings; updates extensions wording; removes tools table strings. |
| src/channels/web/static/app.js | Implements Settings tab logic, structured settings rendering, import/export, confirm modal, and reworked extensions/channels/MCP views. |
| src/channels/web/server.rs | Adds ActiveConfigSnapshot to GatewayState and exposes it via /api/gateway/status. |
| src/channels/web/mod.rs | Adds with_active_config to GatewayChannel; ensures state rebuild carries active_config. |
| src/agent/routine_engine.rs | Adds dispatched-run reconciliation (sync_dispatched_runs), improves watcher state mapping, and refactors concurrent-count loading. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let has_combinators = schema.get("oneOf").is_some() | ||
| || schema.get("anyOf").is_some() | ||
| || schema.get("allOf").is_some(); | ||
|
|
||
| // Rule 1: must have "type": "object" at this level (unless combinators define the structure) |
There was a problem hiding this comment.
Resolved — same fixes as schema_validator.rs. See commits 0f2f8c1, bfeb0e4, e9f640d, 987bea3.
| let has_combinators = schema.get("oneOf").is_some() | ||
| || schema.get("anyOf").is_some() | ||
| || schema.get("allOf").is_some(); | ||
|
|
||
| // Rule 1: must have "type": "object" (unless combinators define the structure) |
There was a problem hiding this comment.
Resolved in commits 0f2f8c1, bfeb0e4, e9f640d, 987bea3. Validators now: check .as_array() (not .is_some()), report non-array combinator values as errors, require object-typed variants via has_object_combinator_variants(), validate required keys against merged combinator properties, and recurse into variants with properties even without explicit type: "object".
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds robust parameter coercion/validation support for JSON Schema combinators (oneOf/anyOf/allOf) so tools (notably the GitHub WASM tool) can correctly handle string-typed scalars and discriminated-union schemas during execution and discovery.
Changes:
- Extend coercion to resolve effective properties across combinator schemas (discriminated
oneOf/anyOf, mergedallOf) and update type checks accordingly. - Relax and extend schema validation to accept combinator-structured object schemas and recursively validate explicit object-typed variants.
- Improve WASM tool schema inspection + tool discovery parameter-name extraction to account for combinator variants; add new E2E + unit tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e_wasm_github_coercion.rs | New E2E test harness for real GitHub WASM tool coercion using an HTTP interceptor. |
| tests/e2e_tool_param_coercion.rs | Adds E2E trace tests for oneOf discriminated-union coercion via a GitHub-like fixture tool. |
| src/tools/wasm/wrapper.rs | Adds optional HTTP interception for WASM tool HTTP requests; updates schema heuristics to inspect combinators. |
| src/tools/tool.rs | Updates lenient tool schema validator to allow combinator-defined object schemas and recurse into object-typed variants. |
| src/tools/schema_validator.rs | Updates strict schema validator similarly for combinator-defined schemas. |
| src/tools/mod.rs | Exposes coercion module and re-exports prepare_tool_params. |
| src/tools/coercion.rs | Makes prepare_tool_params public; adds combinator-aware property resolution + tests. |
| src/tools/builtin/tool_info.rs | Collects parameter names from combinator variants for more complete discovery output. |
Comments suppressed due to low confidence (1)
src/tools/coercion.rs:6
prepare_tool_paramswas widened frompub(crate)topub, making it part of the crate’s public API. If this is primarily to support integration tests, consider exposing it behind#[cfg(feature = "integration")]+#[doc(hidden)](as used for other test-only helpers) to avoid committing to this API for downstream users.
pub fn prepare_tool_params(
tool: &dyn crate::tools::tool::Tool,
params: &serde_json::Value,
) -> serde_json::Value {
prepare_params_for_schema(params, &tool.discovery_schema())
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub mod builder; | ||
| pub mod builtin; | ||
| mod coercion; | ||
| pub mod coercion; |
There was a problem hiding this comment.
Resolved. Module is private (mod coercion;), only prepare_tool_params is re-exported as pub(crate). See 982c725.
| let wasm_path = std::path::Path::new(GITHUB_WASM_PATH); | ||
| if !wasm_path.exists() { | ||
| eprintln!( | ||
| "Skipping WASM github test: binary not found at {GITHUB_WASM_PATH}. \ | ||
| Build with: CARGO_TARGET_DIR=tools-src/github/target \ | ||
| cargo build --manifest-path tools-src/github/Cargo.toml \ | ||
| --target wasm32-wasip2 --release" | ||
| ); | ||
| return None; | ||
| } |
There was a problem hiding this comment.
Resolved. Tests use #[ignore] instead of silent skip (982c725). with_wasm_tool takes Option<PathBuf> with corrected docs (982c725). tokio::fs::read for async loading (bfeb0e4). Soft URL check documented at module level.
| // 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 intercept_headers: Vec<(String, String)> = | ||
| headers.iter().map(|(k, v)| (k.clone(), v.clone())).collect(); | ||
| 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<String, String> = 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(), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| let result = rt.block_on(async { | ||
| let client = reqwest::Client::builder() |
There was a problem hiding this comment.
Resolved. Headers sorted for stability (982c725), deserialization via HashMap (982c725), credential redaction before after_response (bfeb0e4), dead-code comment updated (0f2f8c1), after_response called for recording mode (0f2f8c1). with_http_interceptor kept public intentionally — useful for production trace recording too.
| /// 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<dyn HttpInterceptor>) -> Self { | ||
| self.http_interceptor = Some(interceptor); | ||
| self | ||
| } |
There was a problem hiding this comment.
Resolved. Headers sorted for stability (982c725), deserialization via HashMap (982c725), credential redaction before after_response (bfeb0e4), dead-code comment updated (0f2f8c1), after_response called for recording mode (0f2f8c1). with_http_interceptor kept public intentionally — useful for production trace recording too.
| let has_combinators = schema.get("oneOf").is_some() | ||
| || schema.get("anyOf").is_some() | ||
| || schema.get("allOf").is_some(); | ||
|
|
||
| // 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) => { | ||
| errors.push(format!("{path}: expected type \"object\", got \"{other}\"")); | ||
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
Resolved — same fixes as schema_validator.rs. See commits 0f2f8c1, bfeb0e4, e9f640d, 987bea3.
| let has_combinators = schema.get("oneOf").is_some() | ||
| || schema.get("anyOf").is_some() | ||
| || schema.get("allOf").is_some(); | ||
|
|
||
| // 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) => { | ||
| errors.push(format!("{path}: expected type \"object\", got \"{other}\"")); | ||
| return errors; | ||
| } | ||
| None => { | ||
| errors.push(format!("{path}: missing \"type\": \"object\"")); | ||
| return errors; | ||
| if !has_combinators { | ||
| errors.push(format!("{path}: missing \"type\": \"object\"")); | ||
| return errors; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Resolved in commits 0f2f8c1, bfeb0e4, e9f640d, 987bea3. Validators now: check .as_array() (not .is_some()), report non-array combinator values as errors, require object-typed variants via has_object_combinator_variants(), validate required keys against merged combinator properties, and recurse into variants with properties even without explicit type: "object".
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) <noreply@anthropic.com>
- 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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR improves tool parameter coercion and schema validation to correctly handle JSON Schema combinators (oneOf/anyOf discriminated unions and allOf merges), and extends WASM tooling/tests to better exercise real-world schemas and HTTP behavior.
Changes:
- Enhance coercion and schema introspection to resolve effective object properties across combinator schemas, and update discovery output accordingly.
- Relax/extend schema validators to accept combinator-defined object shapes and validate combinator variants when possible.
- Add TestRig support for loading real WASM tools and introduce E2E tests to validate coercion against a real GitHub WASM tool (with HTTP replay).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/support/test_rig.rs | Adds WASM tool loading to the test rig and shares a replay HTTP interceptor between agent + WASM tools. |
| tests/e2e_wasm_github_coercion.rs | New E2E tests that load a real GitHub WASM tool and verify string→typed coercion via replayed HTTP exchanges. |
| tests/e2e_tool_param_coercion.rs | Adds an in-process fixture tool mirroring a GitHub-style oneOf schema to reproduce/guard coercion behavior. |
| src/tools/wasm/wrapper.rs | Adds optional HTTP interception for WASM tool HTTP requests; updates schema heuristics to inspect combinator variants. |
| src/tools/tool.rs | Updates the lenient runtime schema validator to accept combinator-structured schemas and validate object-typed variants recursively. |
| src/tools/schema_validator.rs | Updates strict CI-time schema validation similarly for combinator-structured schemas. |
| src/tools/mod.rs | Re-exports prepare_tool_params publicly from the tools module. |
| src/tools/coercion.rs | Implements combinator-aware coercion (discriminated union matching + allOf merge) and adds unit tests. |
| src/tools/builtin/tool_info.rs | Collects parameter names from combinator variants for more complete discovery output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Notify the interceptor about the completed response (recording mode). | ||
| // In practice this is dead code because our only interceptor | ||
| // (ReplayingHttpInterceptor) always returns Some from before_request, | ||
| // hitting the early return above. Implemented for correctness so a | ||
| // RecordingHttpInterceptor would capture WASM HTTP exchanges. |
There was a problem hiding this comment.
Resolved. Headers sorted for stability (982c725), deserialization via HashMap (982c725), credential redaction before after_response (bfeb0e4), dead-code comment updated (0f2f8c1), after_response called for recording mode (0f2f8c1). with_http_interceptor kept public intentionally — useful for production trace recording too.
| let resp_headers: Vec<(String, String)> = | ||
| serde_json::from_str(&resp.headers_json).unwrap_or_default(); | ||
| let resp_body = String::from_utf8_lossy(&resp.body).to_string(); | ||
| let exchange_resp = HttpExchangeResponse { | ||
| status: resp.status, | ||
| headers: resp_headers, | ||
| body: resp_body, |
There was a problem hiding this comment.
Resolved. Headers sorted for stability (982c725), deserialization via HashMap (982c725), credential redaction before after_response (bfeb0e4), dead-code comment updated (0f2f8c1), after_response called for recording mode (0f2f8c1). with_http_interceptor kept public intentionally — useful for production trace recording too.
| let intercept_headers: Vec<(String, String)> = headers | ||
| .iter() | ||
| .map(|(k, v)| (k.clone(), v.clone())) | ||
| .collect(); |
There was a problem hiding this comment.
Resolved. Headers sorted for stability (982c725), deserialization via HashMap (982c725), credential redaction before after_response (bfeb0e4), dead-code comment updated (0f2f8c1), after_response called for recording mode (0f2f8c1). with_http_interceptor kept public intentionally — useful for production trace recording too.
| fn skip_if_no_wasm() -> bool { | ||
| if !std::path::Path::new(GITHUB_WASM).exists() { | ||
| eprintln!( | ||
| "Skipping: github WASM binary not found at {GITHUB_WASM}. \ | ||
| Build with: cargo build -p github-tool --target wasm32-wasip2 --release" | ||
| ); | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
|
|
||
| /// 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] | ||
| async fn wasm_github_list_issues_coerces_string_limit() { | ||
| if skip_if_no_wasm() { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Resolved. Tests use #[ignore] instead of silent skip (982c725). with_wasm_tool takes Option<PathBuf> with corrected docs (982c725). tokio::fs::read for async loading (bfeb0e4). Soft URL check documented at module level.
| BuildPhase, BuildRequirement, BuildResult, BuildSoftwareTool, BuilderConfig, Language, | ||
| LlmSoftwareBuilder, SoftwareBuilder, SoftwareType, Template, TemplateEngine, TemplateType, | ||
| TestCase, TestHarness, TestResult, TestSuite, ValidationError, ValidationResult, WasmValidator, | ||
| }; |
There was a problem hiding this comment.
Resolved. Module is private (mod coercion;), only prepare_tool_params is re-exported as pub(crate). See 982c725.
| capabilities_path: Option<impl Into<std::path::PathBuf>>, | ||
| ) -> Self { | ||
| self.wasm_tools.push(WasmToolSpec { | ||
| name: name.into(), | ||
| wasm_path: wasm_path.into(), | ||
| capabilities_path: capabilities_path.map(Into::into), |
There was a problem hiding this comment.
Resolved. Tests use #[ignore] instead of silent skip (982c725). with_wasm_tool takes Option<PathBuf> with corrected docs (982c725). tokio::fs::read for async loading (bfeb0e4). Soft URL check documented at module level.
| /// 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. | ||
| /// | ||
| /// Gracefully skips if the WASM binary does not exist (returns the builder | ||
| /// unchanged so tests can use `if !rig.has_tool("name") { return; }`). |
There was a problem hiding this comment.
Resolved. Tests use #[ignore] instead of silent skip (982c725). with_wasm_tool takes Option<PathBuf> with corrected docs (982c725). tokio::fs::read for async loading (bfeb0e4). Soft URL check documented at module level.
- 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) <noreply@anthropic.com>
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Code Review — oneOf/anyOf/allOf parameter coercion and validation
+1363 / -59 across 8 files. Fixes coercion and validation for combinator-based schemas (discriminated unions, allOf merges). Well-structured with good test coverage.
Issues
1. Duplicated "merge combinator properties + validate required" block (high)
The exact same ~30-line block for merging combinator variant properties and validating required keys is copy-pasted between schema_validator.rs:415-445 and tool.rs:506-535. This is a maintenance hazard — a bug fix in one won't propagate to the other.
Extract a shared helper, e.g.:
fn collect_combinator_property_keys(schema: &serde_json::Value) -> HashSet<String>and reuse it in both validators.
2. Validators accept non-object combinator schemas (medium)
Both check_object_schema and validate_tool_schema_inner set has_combinators = true when oneOf/anyOf/allOf are present as arrays, which bypasses the type: "object" and properties requirements. But they only recursively validate variants that have type: "object". This means a schema like:
{ "oneOf": [{"type": "string"}, {"type": "integer"}] }passes validation with zero errors — even though it's not a valid tool parameter schema (which must be object-shaped).
Add a guard: when has_combinators is true and type is not "object", at least one variant must have object-shaped properties (or type: "object").
3. typed_property_count merges all variant properties with last-wins (medium)
WasmToolSchemas::typed_property_count() (wrapper.rs:700-731) merges properties from ALL combinator variants into a single map. For oneOf schemas, this is semantically wrong — only ONE variant is active at a time. If two variants define the same key with different types (e.g., limit: integer in one, limit: string in another), the count will reflect whichever variant comes last, not the actual effective schema.
For allOf this is correct. For oneOf/anyOf, consider taking the max count across variants rather than merging.
4. find_discriminated_variant only matches const and single-element enum (low-medium)
The discriminator matching in find_discriminated_variant (coercion.rs:135-171) only handles const and single-element enum. It won't match:
- Multi-element
enumdiscriminators (common in OpenAPI:"enum": ["fetch", "pull"]) discriminatorkeyword from OpenAPI 3.x ({ "discriminator": { "propertyName": "action" } })
The current behavior is safe (no match = no coercion = noop), but it means schemas using these patterns silently skip coercion. Consider at least a tracing::debug! when a oneOf/anyOf has no discriminator match so users can diagnose why coercion isn't happening.
5. schema_allows_type treats any combinator as "object" (low-medium)
schema_allows_type (coercion.rs:184-192) now returns true for "object" when any of oneOf/anyOf/allOf is present — regardless of whether the variants are actually object-typed. A schema like { "oneOf": [{"type": "string"}] } would incorrectly be treated as allowing type "object".
This should check that at least one variant has type: "object" or properties.
6. prepare_tool_params visibility widened to pub (low)
prepare_tool_params changed from pub(crate) to pub, and coercion module changed from mod to pub mod. The re-export in mod.rs also changed to pub use. This expands the public API surface. If this is only for integration tests, consider #[cfg(test)] or pub(crate) instead.
7. schema_param_names doesn't recurse into nested combinators (low)
schema_param_names (tool_info.rs:47-67) collects property names from one level of combinator variants, but doesn't recurse. If a variant itself uses allOf to compose its properties, those nested properties are missed. Matches the depth of the coercion logic, so this is consistent — but worth noting as a known limitation.
What's good
- Discriminator matching is sound: requires ALL discriminator properties to match, returns first match — correct for
oneOfsemantics - Safe fallback: no discriminator match → no coercion → params pass through unchanged. No data loss path.
BTreeSetinschema_param_names: gives stable, deduplicated output across variants- Comprehensive test coverage: 5 new coercion unit tests + fixture tool mimicking real GitHub WASM schema + e2e tests
- Consistent pattern: all 4 wrapper.rs functions (
is_permissive_schema,typed_property_count,schema_contains_container_properties, and the validators) updated with the same combinator iteration pattern - HTTP interceptor infrastructure: clean integration for WASM tool testing with proper
before_request/after_responselifecycle
Verdict
Approve with suggestions. Items 1 and 2 should be addressed before merge — the duplicated validation block is a clear maintenance risk, and the non-object combinator gap could accept invalid schemas. The rest are lower priority.
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 <17498900+spiritj@users.noreply.github.com> Co-Authored-By: Xing Ji <41811005+micsama@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…earai#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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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 `nearai/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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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 nearai#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 nearai#755 Co-Authored-By: spiritj <17498900+spiritj@users.noreply.github.com> Co-Authored-By: Xing Ji <41811005+micsama@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: spiritj <17498900+spiritj@users.noreply.github.com> Co-authored-by: Xing Ji <41811005+micsama@users.noreply.github.com>
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Updated Code Review — post commit 987bea3
Following up on my earlier review. The new commits (6904abf..7c04ea9) address several of my original findings and introduce additional changes. Here's what's new and what remains.
Issues addressed since last review
Non-array combinator validation (was item 2) — Fixed. Both schema_validator.rs and tool.rs now report non-array combinator values ("oneOf": {}) as explicit errors before has_object_combinator_variants runs. This closes the gap where malformed schemas slipped through.
Variants without explicit type: "object" now validated (new) — Recursive validation now triggers on variant.get("properties").is_some() in addition to type: "object". This means variants that define properties without an explicit type annotation are properly validated — good catch.
Issues still present
1. Duplicated validation block (high — unchanged)
The ~30-line "merge combinator properties + validate required keys" block is still copy-pasted identically between schema_validator.rs:81-110 and tool.rs:506-535. Extract to a shared helper like:
fn collect_combinator_property_keys(schema: &serde_json::Value) -> HashSet<String>This duplication is now worse than before — both files also duplicate the new has_object_combinator_variants() function (identical implementations in both files).
2. ApprovalContext change is unrelated and significant (medium)
tool.rs includes a semantic change to ApprovalContext::is_blocked() — autonomous jobs now block ALL tools not in allowed_tools, regardless of ApprovalRequirement. Previously, Never and UnlessAutoApproved tools were always allowed in autonomous contexts.
This is a behavioral change that affects routine/job execution, not just schema validation. It should be in a separate PR with its own review, or at minimum called out in the PR description. The test updates confirm this is intentional, but reviewers focused on "oneOf/anyOf/allOf coercion" could easily miss it.
3. typed_property_count still merges all oneOf variants (low — unchanged)
WasmToolSchemas::typed_property_count() still merges properties from ALL variants. For oneOf, only one variant is active — the count may be inflated.
New observations
4. $ref resolution is pre-pass only (low)
resolve_refs() runs as a pre-pass in prepare_params_for_schema but NOT in the validators (check_object_schema, validate_tool_schema_inner). If a tool schema uses $ref, the validators won't see the resolved properties and may reject a valid schema. Consider either running resolve_refs in the validators too, or documenting this as a known limitation.
5. Empty string → null coercion is sound but has edge cases (low)
The empty-string-to-null coercion (coercion.rs:164-171) correctly checks !required.contains(key) and schema_allows_type(prop_schema, "null"). But the required set is built from the top-level required array only — it doesn't include required arrays from matched combinator variants. A field that's required in a oneOf variant but not at the top level could get nulled.
Summary
Good progress — the validator gaps from my first review are closed. The main remaining items are:
- Extract shared
has_object_combinator_variants+ required-key validation (duplicated across 2 files) - Split out
ApprovalContextchange or document it prominently $refresolution missing from validators (minor)
…earai#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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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 `nearai/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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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 nearai#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 nearai#755 Co-Authored-By: spiritj <17498900+spiritj@users.noreply.github.com> Co-Authored-By: Xing Ji <41811005+micsama@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: spiritj <17498900+spiritj@users.noreply.github.com> Co-authored-by: Xing Ji <41811005+micsama@users.noreply.github.com>
…earai#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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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 `nearai/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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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 nearai#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 nearai#755 Co-Authored-By: spiritj <17498900+spiritj@users.noreply.github.com> Co-Authored-By: Xing Ji <41811005+micsama@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: spiritj <17498900+spiritj@users.noreply.github.com> Co-authored-by: Xing Ji <41811005+micsama@users.noreply.github.com>
Summary
Comprehensive parameter coercion and schema validation overhaul so WASM extension tools with complex JSON schemas (discriminated unions,
$ref, nested combinators) work correctly when LLMs pass mistyped parameters.Coercion layer (
src/tools/coercion.rs)oneOf/anyOfdiscriminated unions: match variants byconstor single-elementenumdiscriminator, then coerce properties per the matched variant's schemaallOfmerging: merge all variants' properties for coercion$refresolution: inline#/definitions/<name>and#/$defs/<name>references in a pre-pass (depth-limited for circular ref safety)allOfcontainingoneOf)additionalPropertiesinheritance: check combinator variants for typedadditionalProperties""→nullfor non-required fields when schema allows null or doesn't allow string (closes fix: built-in time tool call failure #755, builds on fix(time): treat empty timezone string as absent #1127)Schema validators (
schema_validator.rs,tool.rs)oneOf/anyOf/allOfdefine the structure instead of requiring top-leveltype: "object"+properties{ "oneOf": [{"type":"integer"}] })requiredkeys against merged combinator variant properties{ "oneOf": {} })WASM wrapper (
src/tools/wasm/wrapper.rs)is_permissive_schema(),typed_property_count(),schema_contains_container_properties()inspect combinator variantsReplayingHttpInterceptorvia TestRigafter_responsecallsTool discovery (
tool_info.rs)schema_param_names()collects parameter names from all combinator variantsTestRig (
tests/support/test_rig.rs)with_wasm_tool(name, wasm_path, capabilities_path)loads real WASM binaries with shared HTTP interceptorCloses #755
Test plan
#[ignore], require pre-compiled binary)cargo clippy --all --all-features— zero warningscargo test— full suite passes🤖 Generated with Claude Code