feat: warn the model about unsupported tool parameters#519
Conversation
Adds an integration test that drives the real MCP server over stdio (like an LLM client) and asserts on the actual CallToolResult for three cases: - Unknown/unaccepted params (e.g. view_range, foo_bar) are silently stripped: the read succeeds, isError is falsy, and nothing tells the model its param was ignored. This documents the current gap. The intended future behavior is to keep the normal response but append a warning that params were stripped; when that lands, the Case 1 "no warning" assertion flips and acts as the regression anchor. - Wrong type on a known param returns a dispatcher-shaped isError: true. - Missing required param (path) returns a dispatcher-shaped isError: true. Test only; no production code changes.
Zod's default object schemas silently strip unknown keys, so a model that invents a parameter (e.g. read_file with view_range) gets a normal-looking response with no signal its input was ignored - it just sees defaults applied. Add a central, dispatcher-level hook that detects parameters the caller sent which the tool's schema does not accept, and prepends a corrective warning to the response naming the ignored params and listing the supported ones. The call still succeeds; the warning is advisory. - src/utils/unsupportedParams.ts: pure helpers (getSupportedParams, detectUnsupportedParams, buildUnsupportedParamsWarning). Top-level detection; unwraps ZodEffects/optional/default defensively; never warns when the supported set can't be determined. - src/tools/schemas.ts: toolArgSchemas map (name -> schema) for all 27 tools. - src/server.ts: hook before the dispatcher's return, wrapped so it can never break a successful call. Covers every tool, not just read_file. - test/integration/read-file-unknown-params.js: validates clean call (no warning), unsupported params (warning prepended, names + supported list, read still served), wrong type and missing-required still return isError.
📝 WalkthroughWalkthroughAdds unsupported-parameter advisory warning support to the MCP tool call dispatcher. A new ChangesUnsupported Params Advisory Warnings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.ts (1)
1476-1478:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
track_ui_eventskips the new advisory hookLine 1477 returns before the unsupported-params warning block runs, so this tool never gets the new warning behavior even though it is registered in
toolArgSchemas.Suggested fix
- if (name === 'track_ui_event') { - return result; - } - - if (result.isError) { + if (name !== 'track_ui_event' && result.isError) { await usageTracker.trackFailure(name); console.log(`[FEEDBACK DEBUG] Tool ${name} failed, not checking feedback`); - } else { + } else if (name !== 'track_ui_event') { await usageTracker.trackSuccess(name); console.log(`[FEEDBACK DEBUG] Tool ${name} succeeded, checking feedback...`); ... result = await processDockerPrompt(result, name); } // advisory warning block remains here so it runs for all tools, including track_ui_eventAlso applies to: 1568-1588
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.ts` around lines 1476 - 1478, The early return for track_ui_event in the condition checking if name === 'track_ui_event' causes the tool to skip the advisory hook warning block that should execute for all registered tools including those in toolArgSchemas. Remove this early return statement so that track_ui_event flows through the same advisory hook/warning block as other tools. Apply the same fix to any other similar early return conditions mentioned in the related code sections (also applies to lines 1568-1588).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/server.ts`:
- Around line 1476-1478: The early return for track_ui_event in the condition
checking if name === 'track_ui_event' causes the tool to skip the advisory hook
warning block that should execute for all registered tools including those in
toolArgSchemas. Remove this early return statement so that track_ui_event flows
through the same advisory hook/warning block as other tools. Apply the same fix
to any other similar early return conditions mentioned in the related code
sections (also applies to lines 1568-1588).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0ea80e6-2ba6-49f2-9aec-a807b829574d
📒 Files selected for processing (4)
src/server.tssrc/tools/schemas.tssrc/utils/unsupportedParams.tstest/integration/read-file-unknown-params.js
What
Makes the server tell the model when it sends a parameter a tool doesn't support, instead of silently dropping it.
Zod's default object schemas strip unknown keys, so e.g.
read_filecalled withview_rangereturned a normal-looking response with the defaults applied and no signal the param was ignored. This adds a central, dispatcher-level hook that detects unsupported parameters and prepends a corrective warning to the response, naming the ignored params and listing the supported ones. The call still succeeds — the warning is advisory.Example warning the model now receives as the first content block:
Covers all tools
The hook runs at the single dispatcher choke point against a
toolArgSchemasmap (name → schema) wired for all 27 tools, so it's not specific toread_file.How it works / safety
src/utils/unsupportedParams.ts— pure helpers. Detection is top-level only, which is safe because no tool schema uses a top-level.passthrough(); free-form fields (options,params,pdfOptions) are named keys, so their contents aren't flagged. UnwrapsZodEffects/optional/default defensively, and never warns when it can't determine the supported set.src/server.ts— the hook is wrapped in try/catch so an advisory warning can never break an otherwise-successful tool call. It only runs on the success path (type errors / missing-required still throw and returnisError: trueas before).Tests
test/integration/read-file-unknown-params.jsdrives the real server over stdio (like an LLM) and asserts on the actualCallToolResult:isErrorfalsy, warning prepended namingview_range/foo_bar+ supported list, file content still servedisError: truepath→isError: trueUnit-checked the helpers separately (supported-param extraction, detection, named free-form field not flagged, all 27 tools mapped).
npm run buildpasses.Note on #518
This supersedes #518 — that PR characterized the silent-strip gap; this branch descends from it and flips the test to assert the new warning behavior. Recommend closing #518 in favor of this, or merging #518 first.
Possible follow-ups (not in this PR)
captureignored-key names only, never values) to measure real-world frequency in BigQuery.Summary by CodeRabbit
New Features
Tests