Skip to content

Accept numeric tool arguments sent as strings - #532

Closed
alistair3149 wants to merge 1 commit into
masterfrom
fix-quoted-numeric-tool-arguments
Closed

Accept numeric tool arguments sent as strings#532
alistair3149 wants to merge 1 commit into
masterfrom
fix-quoted-numeric-tool-arguments

Conversation

@alistair3149

Copy link
Copy Markdown
Member

Fixes #526

MCP clients quote numeric arguments often enough that a bare z.number() refuses calls that are otherwise well formed. update-page was the visible case: a quoted section number was rejected with Invalid input, leaving a full-page rewrite as the only way to edit one section. The same refusal applied to all 24 numeric parameters, among them update-page's own latestId, which its description tells callers to pair with section=N.

unquoteNumber() reads a bare run of digits as the number it spells and hands everything else to the wrapped schema to refuse. So "", null, false and [] are still rejected rather than read as zero the way z.coerce.number() would, zero being a page's lead section.

The published JSON Schema is unchanged, byte for byte across all 46 tools, so callers are still asked for a number. Optionality belongs on the wrapper's result rather than its argument, or the published schema would demand a value the tool treats as optional; the compiler refuses the wrong order.

Tool tests call handle() or dispatch() with already-parsed arguments and so never reach the schema, which is why this went unnoticed. The regression tests go over a real MCP session, and a sweep over every numeric argument on the tool surface fails if one is ever left unwrapped.

To decide

Scope. The issue asks only for update-page's section. That fix alone leaves latestId refusing quoted numbers in the very workflow the issue reproduces, so this covers all numeric parameters. Say the word if you want it narrowed.

z.boolean() has the identical bug, left out of this PR: six parameters (get-recent-changes's five hide* flags, move-page.moveTalk) still refuse "true". A client that quotes 2 will quote true. Worth a follow-up issue.

Read #526's diagnosis with care. Its title says update-page "rejects all numeric section values" and its suspected cause says get-page coerces. Both are wrong: section: 2 passes on master, and nothing coerces anywhere — get-page section:"1", search-page limit:"3" and get-revision revisionId:"123" all fail identically. Only callers that quote the number were affected.

Considered, omitted

  • README tool table — no name, description or grant changes.
  • neowiki-cypher-query.parametersz.record(z.string(), z.unknown()) forwards {"minYear": "2000"} to Neo4j as a string. No declared numeric type to wrap, so out of reach here.
  • Folding tests/runtime/cancellation.test.ts's hand-rolled sessions onto the new helper — unrelated churn.

Verified

lint, typecheck, fmt:check, build, check:mcp and 1766 tests green.

  • Published schema dumped for all 46 descriptors through the SDK's own conversion path and diffed against master: identical, key order and error text included.
  • End-to-end over raw JSON-RPC against en.wikipedia: quoted section, latestId, limit, revisionId and namespaces reach the handler and return real results; "lead", "", null and an omitted revisionId are still refused.
  • New tests mutation-tested rather than assumed: reverting the helper fails 6, dropping .nonoptional() fails only the test that names it, swapping in Number() coercion fails the zero test, and unwrapping one parameter fails the sweep naming that parameter.

AI-authored — Claude Code, Opus 5 1M (ultracode); one-line ask from @alistair3149, who set the wide scope and ruled #530 out of this PR; diff not yet human-reviewed; verification as above, and three independent AI review passes whose findings are folded in.

MCP clients quote numeric arguments often enough that a bare z.number()
refuses calls that are otherwise well formed. update-page was the visible
case: a quoted section number was rejected with "Invalid input", leaving a
full-page rewrite as the only way to edit one section. The same refusal
applied to all 24 numeric parameters, among them update-page's own
latestId, which its description tells callers to pair with section=N.

unquoteNumber() reads a bare run of digits as the number it spells and
hands everything else to the wrapped schema to refuse. So "", null, false
and [] are still rejected rather than read as zero the way
z.coerce.number() would, zero being a page's lead section.

The published JSON Schema is unchanged, byte for byte across all 46 tools,
so callers are still asked for a number. Optionality belongs on the
wrapper's result rather than its argument, or the published schema would
demand a value the tool treats as optional; the compiler refuses the wrong
order.

Tool tests call handle() or dispatch() with already-parsed arguments and so
never reach the schema, which is why this went unnoticed. The regression
tests go over a real MCP session, and a sweep over every numeric argument
on the tool surface fails if one is ever left unwrapped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@alistair3149

Copy link
Copy Markdown
Member Author

Closing this: quoted numeric arguments are a client-side defect, and absorbing them here is the wrong layer.

The change worked and was verified — 24 numeric parameters, published schema byte-identical across all 46 tools, quoted values reaching the handler end to end. The objection is not to the implementation but to the premise. The server would be silently accepting values its own published schema says are invalid, which hides the defect from the clients that need to fix it and commits us to carrying that tolerance indefinitely.

The section parameter that prompted #526 is also no longer union-typed: #533 made it a plain integer, the same shape as get-page's section, which the original reporter observed working in the same session where update-page failed.

Some of the work here is worth keeping and will be split out separately:

  • tests/helpers/callTool.ts already landed in Stop update-page creating sections #533. Tool tests call handle() or dispatch() with arguments the SDK has already validated, so none of them could observe a schema rejection — that blind spot is why update-page rejects all numeric section values; only section="new" is accepted #526 shipped.
  • The .nonoptional() trap found while building this: z.preprocess sets optin: 'optional', so wrapping a required parameter silently drops it from the published schema's required array while the runtime still refuses the omission. A runtime safeParse({}) test cannot see that; the assertion has to be on the emitted JSON Schema.
  • z.coerce.number() is not a safe shortcut anywhere in this codebase: it reads "", " ", null, false and [] as 0, and 0 is a page's lead section on a tool marked destructiveHint: true.

Branch left in place for reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

update-page rejects all numeric section values; only section="new" is accepted

1 participant