Repository navigation
fix(tools): normalize optional fillers before validation - #1523
Conversation
Strict-schema clients send every optional property, using "", [], 0 or null for the ones they do not intend to set. These were validated as real values, so valid calls were rejected or got unsafe behavior (e.g. a zero query timeout disables the deadline). - add `fillerSafeParameters`: top-level optional `[]`, `""`, `0`, `null` are treated as omitted before validation and defaults - apply it in the shared tool wrapper, the legacy zod adapter and plugin tools - SQL tools: filler timeouts keep the configured deadline, zero row limit uses the default, oversized row limits and byte budgets clamp - `sql_check`: coverage still requires an explicit `pattern` - add regression tests for registry-wide fillers and the SQL tools Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 09f3ae64-0f73-4563-b680-918cc2da40db) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (11 files)
Previous Review Summaries (3 snapshots, latest commit 335b9eb)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 335b9eb)Status: No Issues Found | Recommendation: Merge Files Reviewed (11 files)
Previous review (commit aa8bc0e)Status: No Issues Found | Recommendation: Merge Files Reviewed (11 files)
Previous review (commit af1174b)Status: No Issues Found | Recommendation: Merge Files Reviewed (11 files)
Reviewed by gemini-4-argon · Input: 59.6K · Output: 1.4K · Cached: 193.9K Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af1174b6c3
ℹ️ 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".
Validate arguments as sent first so schema-valid intentional values (`max_entries: 0`, `description: ""`, `integration_ids: []`) are preserved. Only a call the schema rejects is retried with top-level optional fillers removed. - `fillerSafeParameters` is now a union of the original schema and the stripped-retry schema - SQL tool `query_timeout_ms` now requires a positive value, so a filler `0` falls back to the configured deadline - tests: valid empty/zero values preserved, rejected fillers still accepted Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: d50420d0-896c-4a97-8918-915803f5c7b6) |
|
Pushed aa8bc0e: arguments are now validated as sent first; optional fillers are stripped only when validation fails. Schema-valid intentional values (zero, empty string, empty array) are preserved. SQL tool timeouts require a positive value so filler zeros fall back to the configured deadline. Deferred P1 removed from the description. |
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa8bc0e94f
ℹ️ 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".
When validation fails, retry keeping each optional filler that is valid on its own and dropping only the rejected ones, instead of stripping all fillers. Intentional zero/empty values next to an unrelated filler survive. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 197e66d3-ff12-4317-9321-b5cee6501d5f) |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Plugin handlers historically received raw arguments. Use the normalized arguments only when they validate, keep arguments the schema does not list, and otherwise hand the handler the arguments as sent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 92c8a122-11a8-4d91-b9cf-dee7c920705f) |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 0d340663-0540-4d89-9b6d-5e83303daa7c) |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
dad62eb
into
feat/analyst-harness-dab-2026-10
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 participant
Issue for this PR
Closes #1495
Type of change
What does this PR do?
Stacked on
feat/analyst-harness-dab-2026-10(#1441).Why. Strict-schema model clients send every optional tool property, filling unused ones with
"",[],0ornull. These were validated as real values, so valid calls were rejected (zero row limit, empty arrays against minimum-length schemas, null) or behaved unsafely (a zero query timeout disables the deadline in the drivers).Root cause. The shared tool wrapper, the legacy zod adapter and plugin tools all validated raw arguments with no optional-filler contract; the SQL tool schemas also accepted zero timeouts and rejected oversized caps.
Before -> after.
[],"",0,nullthat make validation fail are treated as omitted and validation is retried (newfillerSafeParameters), for native, legacy and plugin tools. Required fields, nested payloads, explicitfalseand nonempty/positive values are unchanged.sql_execute,sql_materialize,sql_check: filler timeouts keep the configured deadline instead of disabling it; positive overrides still work.sql_execute.limit:0uses the default; oversized values clamp to 10,000 instead of failing.sql_materializebyte budget: oversized values reach the existing hard cap instead of failing schema validation.sql_check: coverage still requires an explicitpattern(empty pattern stays meaningful); published schema stays flat for strict providers.Fallback, not unconditional stripping. Arguments are validated as sent first, so schema-valid intentional values (
max_entries: 0,description: "",integration_ids: []) are preserved. Only a call the schema rejects is retried, dropping just the top-level optional"",[],0,nullvalues that are not valid on their own and keeping valid siblings. SQL toolquery_timeout_msnow requires a positive value, so a filler0falls back to the configured deadline (an explicit0no longer disables the deadline through these tools).Known limits / follow-ups (P2).
batch, plugin arguments are still decoded twice, which breaks input-changing transforms.tool.definitionplugin hook output may not reach the published JSON schema because it is captured before the hook runs.How did you verify your code works?
test/tool/filler-parameters.test.ts(registry-wide fillers, native/legacy/plugin execution, union discriminants), sql-execute, sql-materialize, sql-check. Each touched file passes in isolation (5, 47, 58, 63 tests; 0 fail); new test covers valid empty/zero values preserved and rejected fillers still accepted;test/tool/registry.test.ts14 pass.bun test test/tool test/altimate/toolsplus the sql-materialize suites: 1399 pass, 4 fail, 2 errors in one combined run. Failures are 5s hook timeouts under load (all pass when rerun alone) and two pre-existing failures that also fail at the base commit (datamate-list-integrations,project-scan).bun run typecheckpasses;git diff --checkclean.src/tool/tool.tsandsrc/tool/registry.ts(present at the v1 baseline tag) have every added line insidealtimate_changeblocks, none nested.Screenshots / recordings
N/A
Checklist
🤖 Generated with Claude Code
Note
Medium Risk
Changes shared tool argument decoding for all tools and alters SQL timeout/limit/cap semantics, which can affect agent tool calls and query execution boundaries.
Overview
Adds
fillerSafeParametersso strict-schema clients can send every optional field as"",[],0, ornullwithout failing validation: arguments are decoded as sent first, and only on failure are top-level optional fillers stripped and decode retried. This is wired into native tools (Tool.wrap), legacy zod tools (tool-zod-compat), and plugin tools (fillerSafePluginin the registry).SQL tool behavior is tightened alongside that: filler
query_timeout_msno longer disables deadlines (positive values only at schema level; omit/0→ connection default).sql_executetreatslimit: 0like omitted/default, and clamps oversized limits to 10k instead of rejecting.sql_checkvalidates with a discriminated union (coverage still requirespattern) but publishes a flat wire schema for strict providers.sql_materializedrops the JSON-schema max onmax_bytesso huge values clamp at execution (tests updated).New
test/tool/filler-parameters.test.tsplus updates to sql-execute, sql-check, and sql-materialize tests cover registry-wide fillers and the new normalization paths.Reviewed by Cursor Bugbot for commit 01c89ca. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes valid tool calls being rejected by strict-schema model clients, which send every optional property filled with
"",[],0, ornull. Arguments are validated as sent first; only when that fails are top-level optional fillers treated as omitted in a retry via the newfillerSafeParameters, so defaults and constraints behave as if the field were not sent. Intentional0/""/[]values that validate are preserved, and valid fillers stay when an unrelated one is rejected. Closes #1495.Behavior changes
sql_executelimit0uses the default; values over 10,000 clamp instead of failing.sql_materializebyte budgets over the 2 GiB cap clamp instead of failing validation.sql_checkcoverage still requires an explicitpattern; the published JSON schema stays flat for strict providers.Written for commit 01c89ca. Summary will update on new commits.