fix(mcp): route object params to JSON editor, keep invalid drafts out of tool args#5570
Conversation
…ts out of tool args
Two follow-up gaps from the earlier MCP schema fix:
- getInputType only routed array-typed and non-primitive-enum params to
the long-input JSON editor; a plain object-typed param (no enum) fell
through to the default short-input, which stores raw text via
toString() and never round-trips a real object.
- The long-input onChange fell back to storing the raw typed text
whenever JSON.parse failed (needed to keep the controlled textarea
responsive mid-edit), but that meant an incomplete/invalid edit
(e.g. `{"a":1` before the closing brace) could persist into the
actual tool arguments. If executed in that state, the MCP execute
route's array-coercion step would silently wrap the malformed string
into a corrupted array instead of failing validation.
Invalid-edit text now lives in local `invalidJsonDrafts` state, keyed
by param name, instead of the real argument store — the textarea still
reflects every keystroke, but the persisted tool argument is always
either the last successfully parsed value or untouched. Drafts reset
when the selected tool changes.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview For params that must be real JSON values (object, array, or non-primitive enum), invalid or in-progress text is held in local draft state keyed to the last valid value. The stored argument stays the last successfully parsed value (or unchanged), which avoids malformed strings reaching execution paths that could coerce them into bad array shapes. Array params still accept comma-separated plain text when the input does not look like a Drafts are cleared when the selected tool or cached schema changes, or when the live discovered schema signature changes after a genuine refresh (with logic to ignore initial load and transient empty refetches). Reviewed by Cursor Bugbot for commit 9cd9120. Configure here. |
Greptile SummaryThis PR improves MCP dynamic argument editing for structured tool params. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (8): Last reviewed commit: "fix(mcp): re-baseline live schema tracke..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…hange The draft reset only fired when the selected tool id changed, so a same-tool schema refresh (e.g. re-discovering tools from the live MCP server) could leave a stale invalid draft displayed under a param name whose shape had since changed. Key the reset off both the tool id and a signature of the effective schema's properties, so any change to what's actually being edited clears stale drafts.
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0f7b3a5. Configure here.
…oolSchema toolSchema resolves to cachedSchema || selectedToolConfig?.inputSchema, so a live-only schema refresh (the discovered tool's inputSchema changes but the cached _toolSchema snapshot doesn't) left the reset key unchanged and could keep a stale invalid draft on screen. Track a signature of each schema source independently so a change in either one clears drafts, regardless of which source toolSchema resolves to.
|
@cursor review |
…ernal value changes Two more follow-up gaps: - schemaSignature only serialized schema.properties, so a same-tool refresh that changed only top-level fields like `required` (properties byte-identical) left the reset key unchanged. Sign the entire schema instead of cherry-picking fields, so nothing schema-level can be missed. - A draft only reset on tool/schema change, so if the persisted argument changed for any other reason (undo/redo, a diff baseline switch, a collaborator's concurrent edit), the draft could keep shadowing the now-current value in the editor while execution used the real one. Drafts now carry a baseline signature of the value they were typed against; a draft only displays while that baseline still matches the live persisted value, so any external change makes it fall back to showing the real value instead of stale text.
|
@cursor review |
…set on tool load
Two more follow-up gaps:
- Holding every JSON.parse failure in a local draft blocked the
documented comma-separated array shorthand (see the placeholder text)
from ever reaching toolArgs, since plain comma-separated text is
never valid JSON. Only an in-progress JSON array/object literal
(starting with `[` or `{`) needs to stay in the draft until valid;
plain array-typed text that isn't attempting JSON persists
immediately as before, letting the execute route's existing
comma-split/wrap coercion handle it as designed.
- draftResetKey always included the live selectedToolConfig schema
signature, even when cachedSchema wins the `toolSchema` resolution.
That segment flips from empty to populated the moment mcpTools
finishes an unrelated async load, wiping in-progress drafts though
neither the rendered schema nor the stored args changed. The live
signature now only factors into the key when there's no cached
snapshot for toolSchema to prefer.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 261f296. Configure here.
…irst load Excluding the live schema signature whenever a cached snapshot exists (the prior fix for a Cursor finding about mcpTools' initial load spuriously wiping drafts) went too far the other way: a genuine same-tool live schema refresh while a cached snapshot is still present would no longer reset drafts either. Track the live schema signature unconditionally, but only treat a change as a real reset trigger when it goes from one non-empty signature to a *different* non-empty one. The bare empty → non-empty transition (mcpTools completing its initial fetch) is excluded, since that's not a schema change; a populated → differently-populated transition (an actual re-discovery) still resets drafts regardless of whether a cached snapshot is present.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 72bdb1f. Configure here.
… render
Comparing the live schema signature only against the immediately
preceding render's value meant a schema that dropped to empty and then
reappeared with different content was invisible to the reset check —
both the drop (X → '') and the reappearance ('' → Y) look like a bare
empty/non-empty transition, which was deliberately excluded to avoid
resetting on mcpTools' initial load. Track the last non-empty value
actually observed instead, so a transient empty gap no longer erases
the baseline: the schema reset now fires correctly when the tool
reappears with a genuinely different schema, while a real first-ever
load (no prior non-empty value at all) still doesn't spuriously reset.
|
@cursor review |
lastNonEmptyLiveSchemaSignature carried over across a tool switch whenever the newly-selected tool's live schema hadn't loaded yet in that same render (still empty). When it loaded a moment later, the comparison was against the *previous* tool's signature, so the new tool's first schema load could be misread as a "genuine refresh" and wipe drafts the user had already started typing against the new tool. A tool/cached-schema change now always re-baselines the live-schema tracker to the new tool's current signature (even if still empty), so the "same tool" refresh comparison never bleeds across tool switches.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9cd9120. Configure here.
1 participant
Summary
getInputTypepreviously only handled array-typed and non-primitive-enum params, leaving object params on the short-input's rawtoString()path{"a":1before the closing brace) used to fall back to storing the raw text, which the MCP execute route's array-coercion step could then silently wrap into a corrupted array instead of failing validation. Invalid text now lives in local per-param draft state so the textarea stays responsive but the persisted argument is always either the last valid parsed value or untouchedType of Change
Testing
Tested manually
Checklist