Fix schema-guided tool parameter coercion#1143
Conversation
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 introduces a robust, centralized mechanism for coercing tool parameters based on their defined JSON schema. This enhancement addresses a common challenge where Large Language Models (LLMs) might provide parameters as stringified values (e.g., "5" instead of 5, or JSON strings for complex objects/arrays). By applying schema-guided normalization early in the tool execution lifecycle, it ensures that all tool inputs are correctly typed and formatted before they are approved or executed, improving reliability and reducing potential errors. The change refactors existing, more ad-hoc coercion logic into a unified system and includes extensive testing to cover various data types and structures. Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 introduces a centralized and robust schema-guided parameter normalization layer. The new coercion.rs module provides a powerful recursive implementation that correctly handles scalar types, nested objects, arrays, and even stringified JSON values, which is a common issue with LLM-generated tool calls.
The changes are consistently applied across all tool execution paths, including routines, the scheduler, the builder, and the main job worker, which greatly improves code maintainability and correctness by removing duplicated and limited coercion logic. The old WASM-specific coercion path has been cleanly removed, and a clever fallback mechanism for WASM discovery schemas has been preserved.
The addition of comprehensive unit tests for the new coercion logic and end-to-end regression tests for common real-world scenarios (like Google Sheets and Docs API formats) provides strong confidence in the correctness of this fix.
Overall, this is an excellent and well-executed refactoring that significantly improves the reliability of tool parameter handling. I have no further comments.
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared, schema-guided parameter normalization step for tool calls so that stringified scalars (e.g., "true", "5") and quoted JSON containers (arrays/objects) are coerced into the correct JSON types before approval checks, validation, and execution. It centralizes behavior that previously lived in the WASM wrapper and adds regressions for Google Sheets/Docs-style payloads that models commonly send.
Changes:
- Add
prepare_tool_params(schema-guided coercion) and apply it across multiple tool execution entry points (worker, scheduler, routines, builder, shared execute pipeline). - Remove the WASM-only coercion path while preserving WASM discovery-schema fallback behavior and improving tool error hints around container params.
- Add unit + E2E regressions covering stringified array/object payloads and approval behavior on normalized params.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e_tool_param_coercion.rs | New E2E regressions for Sheets/Docs-style quoted JSON container arguments. |
| src/worker/job.rs | Normalize params before approval/hooks/validation and again after hook modifications. |
| src/tools/wasm/wrapper.rs | Remove WASM-specific coercion; preserve typed discovery schema when override is untyped; improve usage hints. |
| src/tools/mod.rs | Wire in new coercion module and re-export prepare_tool_params internally. |
| src/tools/execute.rs | Normalize params in shared execution pipeline; add unit regression for stringified arrays. |
| src/tools/coercion.rs | New shared schema-guided coercion implementation + unit tests. |
| src/tools/builder/core.rs | Normalize params before executing build tools. |
| src/agent/scheduler.rs | Normalize params before approval in scheduler tool subtasks. |
| src/agent/routine_engine.rs | Normalize params before approval/validation/execution for lightweight routine tools. |
Comments suppressed due to low confidence (1)
src/agent/scheduler.rs:525
execute_tool_tasknormalizesparamsviaprepare_tool_paramsand then callsexecute_tool_with_safety, which normalizes again internally. This double-prepares/clones the params tree (potentially expensive for large payloads) and makes it harder to reason about where normalization happens. Consider either (a) passing the originalparamsintoexecute_tool_with_safetyand only using the prepared copy for the approval check, or (b) adding anexecute_tool_with_prepared_params/flag so the shared pipeline can skip the second normalization when the caller already prepared them.
let params = prepare_tool_params(tool.as_ref(), ¶ms);
// Scheduler-specific approval check
let requirement = tool.requires_approval(¶ms);
let blocked =
ApprovalContext::is_blocked_or_default(&approval_context, tool_name, requirement);
if blocked {
return Err(crate::error::ToolError::AuthRequired {
name: tool_name.to_string(),
}
.into());
}
// Delegate to shared tool execution pipeline
let output_str = crate::tools::execute::execute_tool_with_safety(
&tools, &safety, tool_name, ¶ms, &job_ctx,
)
.await?;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared, schema-guided tool-parameter normalization step that runs before approval checks, validation, and execution across the main tool execution entry points, and removes the prior WASM-only coercion path while preserving typed WASM discovery schema behavior.
Changes:
- Add
prepare_tool_params/ schema-guided coercion module and integrate it into worker, scheduler, routine, builder, and generic tool execution paths. - Adjust WASM wrapper schema override behavior and update error hinting to reference
tool_infoand avoid quoted JSON container inputs. - Add unit + E2E regressions covering Sheets-style
valuesand Docs-stylerequestspayload shapes with stringified JSON containers.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e_tool_param_coercion.rs | New E2E trace tests validating container coercion for Sheets/Docs-like payloads. |
| src/worker/job.rs | Normalize params prior to approval + hook handling; re-normalize hook-modified params. |
| src/tools/wasm/wrapper.rs | Remove WASM-only coercion; preserve extracted typed discovery schema when sidecar override is untyped; improve error hints. |
| src/tools/mod.rs | Wire in new internal coercion module and export prepare_tool_params within crate. |
| src/tools/execute.rs | Normalize params before validation/logging/execution; add regression test. |
| src/tools/coercion.rs | New schema-guided normalization implementation + unit tests. |
| src/tools/builder/core.rs | Normalize params before executing build tools. |
| src/agent/scheduler.rs | Normalize params before scheduler-specific approval gating; add regression test. |
| src/agent/routine_engine.rs | Normalize params before approval + validation + execution in routine tool runs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/tools/wasm/wrapper.rs
Outdated
| .map(|props| { | ||
| props | ||
| .values() | ||
| .filter(|prop| prop.get("type").and_then(|t| t.as_str()).is_some()) |
There was a problem hiding this comment.
Fixed in 7909289. typed_property_count now treats union/nullable type arrays as typed, and also recognizes other typed schema constructs ($ref, combinators, items, properties, typed additionalProperties) so we do not misclassify valid typed overrides as untyped.
| fn schema_contains_container_properties(schema: &serde_json::Value) -> bool { | ||
| schema | ||
| .get("properties") | ||
| .and_then(|p| p.as_object()) | ||
| .map(|props| { | ||
| props.values().any(|prop| { | ||
| matches!( | ||
| prop.get("type").and_then(|t| t.as_str()), | ||
| Some("array" | "object") | ||
| ) | ||
| }) | ||
| }) | ||
| .unwrap_or(false) | ||
| } |
There was a problem hiding this comment.
Fixed in 7909289. The container-hint check now uses the same schema type detection logic, so nullable container properties like {"type": ["array", "null"]} and object equivalents still trigger the native-JSON guidance.
src/tools/coercion.rs
Outdated
| if let Some(current) = coerced.get(key).cloned() { | ||
| coerced.insert(key.clone(), coerce_value(¤t, prop_schema)); |
There was a problem hiding this comment.
Also addressed in 7909289. I rewrote the object branch to coerce fields in place within the cloned map, which removes the extra get(...).cloned() / insert(...) churn while preserving the same behavior.
G7CNF
left a comment
G7CNF
left a comment
There was a problem hiding this comment.
Reviewed for tool-calling blocker scope: schema-guided coercion is applied across scheduler/routine/worker paths and has targeted regressions. LGTM.
|
Tool-calling blocker triage: this appears ready from a reliability perspective (shared schema-guided param normalization across scheduler/routine/worker + regressions). Recommend maintainer merge once branch policy requirements are satisfied. |
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review
Solid, well-motivated change. Lifting parameter coercion from the WASM layer to the shared execution layer is the right architectural move -- LLMs send stringified values regardless of tool type.
The new coerce_value() is recursive and handles nested objects, stringified JSON arrays/objects, nullable union types, and additionalProperties. Good test coverage (10 unit tests in coercion.rs, integration tests in execute.rs and scheduler.rs, E2E trace tests for Sheets/Docs payloads).
The with_schema guard using typed_property_count correctly prevents untyped sidecar overrides from clobbering typed WASM-extracted discovery schemas. build_tool_usage_hint conditionally adding the array/object hint is a nice touch.
Minor notes (non-blocking)
- Variable shadowing --
let params = prepare_tool_params(tool.as_ref(), ¶ms)shadows the function parameter in several call sites. Considernormalized_paramsfor readability. - Double normalization in
worker/job.rshook path -- hook-modified params get re-normalized while the fallback uses already-normalized params. Correct behavior but a comment explaining the asymmetry would help. oneOf/anyOf/$refschemas unsupported -- worth a comment incoerce_value()noting this limitation.- Cloning on every call --
prepare_tool_paramsalways clones even when no coercion needed.Cow<'_, Value>could avoid this for hot paths in a follow-up.
Approve with the above as optional improvements.
|
Addressed the non-blocking review notes in
I left the clone-avoidance idea as a follow-up; it changes the API shape more than I want in this PR. Verification on this branch: |
There was a problem hiding this comment.
Pull request overview
This PR fixes schema-guided tool parameter coercion by introducing a shared normalization step that runs before approval checks, validation, and execution, and by removing the prior WASM-only coercion logic while preserving typed discovery schema behavior for WASM tools.
Changes:
- Added a shared
prepare_tool_paramsnormalization layer (schema-guided coercion of stringified scalars/containers). - Integrated normalization into major tool execution entrypoints (worker, scheduler, routines, shared execute pipeline, builder tools).
- Added unit + E2E regression tests covering Sheets-style
valuesand Docs-stylerequestspayloads (quoted JSON arrays/objects).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e_tool_param_coercion.rs | New E2E tests exercising normalization through the real agent loop with Sheets/Docs-shaped payloads. |
| src/tools/coercion.rs | Implements schema-guided parameter preparation/coercion used across execution paths. |
| src/tools/mod.rs | Wires the new coercion module and re-exports prepare_tool_params for crate-internal use. |
| src/tools/execute.rs | Normalizes params before validation/logging/execution in the shared execution pipeline; adds a regression unit test. |
| src/worker/job.rs | Normalizes params before approval, hooks, validation, and execution in worker job execution. |
| src/agent/scheduler.rs | Normalizes params before scheduler approval checks and forwards into shared execution. |
| src/agent/routine_engine.rs | Normalizes params before approval/validation/execution for lightweight routine tool calls. |
| src/tools/builder/core.rs | Normalizes params before executing build tools with a dummy context. |
| src/tools/wasm/wrapper.rs | Removes WASM-only coercion path, improves schema override handling, and enhances tool-usage hinting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -518,7 +520,11 @@ impl Scheduler { | |||
|
|
|||
| // Delegate to shared tool execution pipeline | |||
| let output_str = crate::tools::execute::execute_tool_with_safety( | |||
| &tools, &safety, tool_name, ¶ms, &job_ctx, | |||
| &tools, | |||
| &safety, | |||
| tool_name, | |||
| &normalized_params, | |||
| &job_ctx, | |||
| ) | |||
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review: Extract schema-guided tool parameter coercion into shared module
Well-designed extraction. LLMs frequently send stringified values ("true", "42") that break tools expecting native types. This PR centralizes the coercion logic that was previously scattered/missing.
Positives:
- New
src/tools/coercion.rsprovidesprepare_tool_params()that coerces values guided by the tool's JSON Schema - Handles string->integer, string->boolean, string->number, string->array (JSON parse), string->object (JSON parse)
- Applied consistently at all execution points: routine engine, scheduler, builder, direct execute
- Extracted from WASM wrapper (which had ad-hoc coercion), reducing its complexity by ~70 lines
- Schema-awareness means coercion only happens when the schema says the target type isn't string
- Good test coverage in
tests/e2e_tool_param_coercion.rswith edge cases - Test in scheduler verifies coercion happens before approval check (stringified
"true"should resolve totruefor approval)
Minor notes:
discovery_schema()is used as the coercion guide, which is the right choice (it includes full property definitions)- G7CNF already approved
Title check: accurate.
LGTM.
* Fix schema-guided tool parameter coercion * Fix CI checks for coercion regression tests * Finish panic-scan annotations * Avoid redundant worker param preparation * Keep panic-scan annotations rustfmt-stable * Handle nullable WASM schema review feedback * Address param coercion review notes
Summary
Testing