Repository navigation
Conversation
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 enhances the WASM tool loading mechanism by intelligently advertising typed schemas to the Large Language Model (LLM) by default, rather than always presenting a permissive empty object schema. This change improves the accuracy of tool parameter descriptions for the LLM, while falling back to a permissive schema only when the typed schema is too large or untyped. 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
|
There was a problem hiding this comment.
Code Review
This pull request is a great improvement that allows WASM tools to advertise their typed schemas by default, as long as they are not too large. This will help the LLM make better-informed tool calls. The logic for deciding when to use the typed schema is sound, and the changes are well-tested. I have one suggestion to improve the memory efficiency of the schema size check, aligning with our WASM performance guidelines.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3a4a987fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let advertised = if Self::typed_property_count(&discovery) > 0 | ||
| && Self::is_reasonably_small_schema(&discovery) | ||
| { | ||
| discovery.clone() |
There was a problem hiding this comment.
Validate exported WASM schemas before advertising them
If a WASM extension's schema() export is only partially valid (for example, it has one typed property but also an array property without items or an orphaned required key), this branch now publishes that raw schema to the LLM/provider because WasmToolSchemas::new() only checks typed_property_count() and size. Unlike sidecar schemas in src/tools/wasm/loader.rs, exported schemas never go through validate_tool_schema(), and the provider-side normalizer in src/llm/rig_adapter.rs only adds strict-mode fields; it does not repair those structural errors. Before this change we fell back to the permissive {} schema, so malformed exports did not break function-calling for the tool.
Useful? React with 👍 / 👎.
henrypark133
left a comment
There was a problem hiding this comment.
Review: Advertise typed WASM schemas to LLMs by default
Good improvement for LLM tool-use quality. Previously all WASM tools sent permissive schemas and relied on tool_info hints; now typed schemas are promoted directly when they exist and fit within budget.
Positives:
MAX_ADVERTISED_SCHEMA_BYTES = 64KBguard prevents context bloat from huge schemas- Dual-gate:
typed_property_count > 0AND size check — untyped schemas correctly stay permissive withtool_infohint fallback - Loader log messages updated to reflect the new behavior
- Both paths tested:
test_typed_wasm_schema_is_advertised_to_llmandtest_untyped_wasm_schema_stays_permissive
LGTM.
|
Follow-up from review: A competing PR (#1348) independently implemented the same fix with a tighter size limit of 8KB ( Could you tighten the limit from |
There was a problem hiding this comment.
Pull request overview
Updates WASM tool schema advertising so that the LLM sees a typed exported schema by default (when it’s typed and below a size threshold), fixing cases where tools were invoked with {} due to overly-permissive advertised schemas.
Changes:
- Advertise the extracted WASM schema directly when it has typed properties and is reasonably small; otherwise keep the permissive
{}fallback. - Adjust WASM wrapper tests to validate typed vs. untyped advertised schema behavior and the presence/absence of the
tool_infohint. - Update loader warning messages related to missing/invalid
parametersin capabilities sidecars.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/tools/wasm/wrapper.rs | Introduces size-gated typed schema advertising and updates/adds tests for typed vs. untyped behavior. |
| src/tools/wasm/loader.rs | Refines warning messages around capabilities-sidecar parameter schema handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "Invalid parameters schema in capabilities.json, \ | ||
| using permissive fallback" | ||
| "Invalid parameters schema in capabilities.json; \ | ||
| falling back to permissive parameters" |
| fn new(discovery: serde_json::Value) -> Self { | ||
| let advertised = if Self::typed_property_count(&discovery) > 0 | ||
| && Self::is_reasonably_small_schema(&discovery) | ||
| { | ||
| discovery.clone() |
ilblackdragon
left a comment
There was a problem hiding this comment.
Review: fix(wasm): advertise typed schemas by default
This PR is superseded — recommend closing.
PR #1699 ("fix(wasm): use typed WASM schema as advertised schema when available") was already merged to staging on 2026-03-28 (commit 27e8d6f8). It implements a strictly more sophisticated solution to the same issue #1303:
- This PR's approach: Clone the full
discoveryschema asadvertisedwhen typed properties exist and schema is < 64 KB. - #1699's approach (merged):
compact_schema()extractsrequiredandenum/constproperties fromoneOf/anyOf/allOfvariants, producing a smaller, more LLM-friendly schema.
Both wrapper.rs and loader.rs have merge conflicts with current staging. Merging this PR would regress the schema compaction logic.
|
Closing as superseded by #1699 |
Use the extracted WASM schema as the advertised schema when it is typed and small enough, instead of always showing permissive {} parameters.
Fixes #1303.