Repository navigation
feat(mcpx): validate CallTool arguments against inputSchema (-32602), warn on output mismatch (#1483) - #1596
Conversation
…output mismatch (#1483) Phase 2 of #1481, consuming the Phase-1 schemaval core (#1482): - ListTools compiles each tool input/output schema via internal/schemaval (cached per tool, keyed by schema hash); a tool whose schema does not compile (unsupported dialect, unresolvable $ref, invalid/oversized) is excluded with a Warning, the x-mcp-header precedent. Applies on every connection, not just modern HTTP. - CallTool validates marshalled arguments against the compiled input validator before the transport is touched; failure is a typed InvalidParamsError carrying -32602 (RPCCode), naming the tool and the path-qualified reason; the upstream receives zero calls. - The gateway invoke path in internal/mcpserver relays an RPCCode -32602 error as a protocol error instead of an isError tool result (interface check, no import cycle). - structuredContent is validated against the output validator warn-only; the result is delivered unchanged. - Validation applies once a connection has listed (the pool always lists before calling); a never-listed connection behaves exactly as before. - Conformance matrix rows for the four json-schema ids updated from gap #254 to implemented. Tests: defining tests in internal/mcpclient/schemacheck_test.go (wrong type / missing required / additional property rejected with upstream call count 0; valid args pass; unsupported dialect and unresolvable $ref excluded with warnings; output violation warn-only). Full mcpclient + mcpserver + pool suites pass on the laptop. Benchmark (laptop, Ryzen 7840U): BenchmarkCallToolValidateArgs 41.6us/op for a full scripted CallTool round trip including validation of a ~5-level/~20-property nested schema — inside the triage budget of <=100us/op median.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 26 minutes. View limit details
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 |
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. |
![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: 7f7f87f5a8
ℹ️ 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".
| ts, ok := c.validatorsFor(name) | ||
| if !ok || ts.input == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
For session- and call-scoped servers, this silently skips validation on production calls: Pool.RefreshSchemas lists tools on its catalogue connection and then releases that instance (internal/pool/pool.go:827-880), whereas Pool.Call acquires another connection and immediately calls CallTool without listing tools. The new connection therefore has no toolValidators, so invalid arguments reach the upstream and output validation is also skipped. The pool's cached schemas need to initialize the executing connection's validators, or that connection must load them before validation; listing on the separate catalogue connection does not cover this path.
Useful? React with 👍 / 👎.
| var coded interface{ RPCCode() int } | ||
| if errors.As(err, &coded) && coded.RPCCode() == codeInvalidParams { | ||
| return fail(codeInvalidParams, err.Error()) |
There was a problem hiding this comment.
When argument validation fails through the actual MCP gateway, this assertion never sees InvalidParamsError: mcpBackend.Call communicates with the daemon over HTTP (internal/cli/serve.go:122-156), and daemon.callErrorBody preserves RPC codes only for errors with reflected Code and Message fields (internal/daemon/callerr.go:161-203). The new error has neither field, so it becomes an HTTPError without RPCCode() or an upstream error envelope, and the gateway returns an isError tool result instead of the intended -32602 protocol error. Serialize and reconstruct the validation code across that boundary, including the asynchronous ask path, rather than relying only on an in-process interface assertion.
Useful? React with 👍 / 👎.
…cs (#1483) Phase 2 validation refused schema-moved calls in mcpclient before the transport, so the daemon explainCall path (which only recognised upstream rpcErrors) attached no diagnostics: five e2e tests failed in CI (callerr x4, ceiling x1) and refused calls lost the schema-change explanation every surface is built around. - daemon: clientFault() recognises an error carrying RPCCode() -32602 (interface assertion, the gateway precedent) and explainCall runs diagnose.CallError for it, so /v1/call, tasks, scripts and the CLI carry the same diagnostics as an upstream refusal. - mcpclient: InvalidParamsError headline uses the canonical "Invalid params" spelling, matching upstream -32602 messages. - e2e: two assertions that hardcoded the upstream headline ("mcp error -32602...") now expect the validator headline; the diagnostics assertions are unchanged.
…binator failures (#1483) schemaval.Validate read DetailedOutput as a flat list: interior units (anyOf/oneOf/allOf/not and other grouping keywords) carry a nil Error, so an argument failure through a combinator nil-pointer panicked the daemon — the mcpx conformance server-all leg crashed mid-suite on PR #1596 (196/51 vs main 272/0) and every later scenario failed to connect. Walk the output tree to its leaves instead. Regression tests: combinator cases in schemaval and a CallTool anyOf case in mcpclient (returns -32602, upstream untouched).
|
Do not merge b4a06b8 — conformance found a real crash; fixed in 4ff47f7. The Root cause: Fix (4ff47f7): walk the output tree to its leaves when flattening problems. Regression tests: combinator cases in Note: PR #1606 (Phase 3 elicitation, #1484) validates through the same Awaiting CI on 4ff47f7 — merge only when green incl. conformance. |
…ool-validation # Conflicts: # pkgs/mcpx/docs/spec/conformance-matrix.md
… a violation (#1483) CallTool validation now drops null-valued non-required top-level properties from the copy it validates (SEP-2243: a null parameter is not provided); the wire arguments are unchanged and a null for a required property still fails -32602. This restores the client-2026-07-28 conformance checks http-custom-headers/omit-null, whose suite call sends verbose:null against a boolean schema. officialclient: the no-arguments fallback now synthesizes minimal schema-valid arguments from each advertised tool's inputSchema instead of {}, restoring http-invalid-tool-headers/valid_tool (required region) under CallTool enforcement.
|
Pushed
Laptop verification: — queue worker (repo-worker-code), lane A, task 1616 resume |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 550585c2cc
ℹ️ 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".
| for key, value := range obj { | ||
| if string(bytes.TrimSpace(value)) == "null" && !ts.required[key] { | ||
| delete(obj, key) |
There was a problem hiding this comment.
When an input schema declares a required nullable property through allOf or $ref, requiredProps only reads the root-level required, so this removes a valid explicit null before validation. For example, {"type":"object","allOf":[{"required":["x"],"properties":{"x":{"type":["null","string"]}}}]} accepts {"x":null}, but the client instead validates {} and rejects the call with -32602 without contacting upstream. Preserve schema-valid nulls rather than deciding optionality solely from the root-level keyword.
Useful? React with 👍 / 👎.
…ool-validation # Conflicts: # pkgs/mcpx/docs/spec/conformance-matrix.md # pkgs/mcpx/internal/mcpclient/client.go
Fixes #1483 (Phase 2 of #1481; queue task 1616). Gate: Phase 1 #1482 merged (PR #1570, main 1da6d84) — verified fresh this shift.
What
internal/mcpclient/schemacheck.go, wired intoListToolson every connection): each tool'sinputSchema/outputSchemais compiled withinternal/schemaval, cached per tool keyed by schema hash. A schema that does not compile (unsupported dialect, unresolvable/external$ref, invalid or oversized) excludes the tool with aWarningcarrying the cause — the x-mcp-header precedent, surfaced by the pool asserver.warning.CallToolvalidates the marshalled arguments against the compiled input validator before the transport is touched. Failure returns a typedInvalidParamsError(-32602 viaRPCCode()), naming the tool and the path-qualified reason; the upstream receives zero calls.tools/callinvoke path ininternal/mcpserverrelays anRPCCode() == -32602error as a protocol error instead of converting it to anisErrorresult (interface check — no import cycle).structuredContentis validated against the output validator; a violation records aWarningand the result is delivered unchanged.tools/listis issued on a call's behalf. Tools with no schema behave exactly as today.Tests
New defining tests in
schemacheck_test.go: wrong-type / missing-required / additional-property arguments all fail -32602 with upstream call count 0; valid arguments reach the upstream; unsupported-dialect and unresolvable-$reftools are absent fromListToolswith warnings naming the cause; output violation is warn-only with the result unchanged.Local (laptop):
go test ./internal/mcpclient/... ./internal/mcpserver/... ./internal/pool/...— all pass.Benchmark (triage budget: ≤100µs/op median, ≤1ms worst)
BenchmarkCallToolValidateArgs(nested schema, ~5 levels / ~20 properties): 41.6 µs/op on the laptop (Ryzen 7840U) for a full scriptedCallToolround trip including validation — inside budget. Also recorded on #1483.Conformance
Matrix rows
json-schema-support-2020-12,json-schema-validate-per-dialect-and-error-on-unsupported,json-schema-document-dialects,json-schema-reject-unresolved-refupdated from gap #254 to implemented (both occurrences each).Need help on this PR? Tag
@codesmith-botwith what you need.