Repository navigation
Conversation
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. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/client/index.md">
<violation number="1" location="docs/client/index.md:144">
P2: This overstates the result path: a tool that raises `MCPError` is re-raised, so `Client.call_tool()` raises instead of returning `is_error=True`. Limit this to ordinary tool errors or call out the `MCPError` exception.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
| !!! warning "Unknown tools raise protocol errors" | ||
| Ask for a tool the server does not have and `Client.call_tool()` raises `MCPError` with | ||
| code `-32602` and `Unknown tool: <name>` as its message. Argument-validation failures and | ||
| tool execution failures return `is_error=True` instead. |
There was a problem hiding this comment.
P2: This overstates the result path: a tool that raises MCPError is re-raised, so Client.call_tool() raises instead of returning is_error=True. Limit this to ordinary tool errors or call out the MCPError exception.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/client/index.md, line 144:
<comment>This overstates the result path: a tool that raises `MCPError` is re-raised, so `Client.call_tool()` raises instead of returning `is_error=True`. Limit this to ordinary tool errors or call out the `MCPError` exception.</comment>
<file context>
@@ -138,12 +138,11 @@ A tool that raises does **not** raise in your client. It comes back as an ordina
+!!! warning "Unknown tools raise protocol errors"
+ Ask for a tool the server does not have and `Client.call_tool()` raises `MCPError` with
+ code `-32602` and `Unknown tool: <name>` as its message. Argument-validation failures and
+ tool execution failures return `is_error=True` instead.
+ **[Handling errors](../servers/handling-errors.md)** covers both paths.
</file context>
| tool execution failures return `is_error=True` instead. | |
| ordinary tool errors return `is_error=True`; a tool that raises `MCPError` still produces a protocol error. |
📚 Documentation preview
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked whether ToolNotFoundError needs an __all__ export — it follows the same non-exported pattern as ResourceNotFoundError in src/mcp/server/mcpserver/exceptions.py, so no — and whether the exc.name == params.name guard in src/mcp/server/mcpserver/server.py could misfire for a tool that calls a different missing name via ctx.mcp.call_tool (the names differ, so it stays an is_error result; covered by the new nested-call tests).
Extended reasoning...
The change swaps the unknown-tool tools/call response from an is_error=True result to a JSON-RPC INVALID_PARAMS error via a new ToolNotFoundError subclass and a name-equality guard in the top-level request handler, with matching docs and test updates; no auth, crypto or injection surface is touched. Inline findings already flag the pickle re-wrapping bug and the released-v2 behaviour change, so a human look is warranted and this note only records what else was examined.
| """SDK-defined: a call to a name that was never registered is the caller's mistake, logged | ||
| as one INFO record alongside the is_error result.""" | ||
| async def test_unknown_tool_raises_invalid_params_without_logging(caplog: pytest.LogCaptureFixture) -> None: | ||
| """An unknown tool is a protocol error, so MCPServer does not log it as an execution failure.""" |
There was a problem hiding this comment.
🟡 nit (optional): maintainers triaging a future failure of these tests lose the provenance tag the repo's test guide asks for. The rewritten docstring at tests/server/mcpserver/test_server.py:2649 drops the base branch's leading SDK-defined: marker, and the five new tests added below say neither spec-mandated nor SDK-defined. Fix: state the provenance in each docstring (these are SDK-defined choices; the INVALID_PARAMS code is the spec example), which covers the 6 sites listed. Same instruction at 6 sites (test_server.py:2649, test_server.py:3331, test_server.py:3345, test_server.py:3362, test_server.py:3382, test_server.py:3404). [also at: tests/server/mcpserver/test_server.py:3345 - nit: maintainers triaging a later failure of the new unknown-tool tests cannot tell from the docstring whether the pinned behaviour is spec-mandated or SDK-defined.; tests/server/mcpserver/test_server.py:3331 - nit: maintainers triaging a future failure of the new unknown-tool tests cannot tell from the docstring whether the pinned behaviour is spec-mandated or SDK-defined.]
Why this was flagged
.claude/skills/test-quality/SKILL.md (which AGENTS.md line 68 binds tests to) says docstrings must be "honest about provenance — spec-mandated, SDK-defined, or pinning a known gap? Say which". On the base branch the docstring of the unknown-tool logging test began SDK-defined: a call to a name that was never registered .... The diff replaces it at tests/server/mcpserver/test_server.py:2649 with An unknown tool is a protocol error, so MCPServer does not log it as an execution failure. and adds five tests at 3331, 3345, 3362, 3382 and 3404 whose docstrings likewise name no provenance. The guide states the reason: provenance is the triage key when the test later fails (a spec-mandated assertion breaking is a regression; an SDK-defined one may be an intended change). Nothing fails at runtime; this is a convention the repository's own instructions ask reviewers to check.
Verification: Any future failure of these six tests is triaged by a maintainer reading the docstring, which no longer says whether the pinned behaviour is spec-mandated or SDK-defined. The diff replaces the base SDK-defined: docstring at tests/server/mcpserver/test_server.py:2649 with one that names no provenance, and the five new tests at lines 3330, 3345, 3361, 3381 and 3403 likewise say neither. Nothing fails at runtime.
| if isinstance(exc, ToolNotFoundError) and exc.name == params.name: | ||
| raise MCPError(code=INVALID_PARAMS, message=str(exc)) from exc |
There was a problem hiding this comment.
🟡 nit (optional): AGENTS.md says any change to a released v2 API's observable behaviour is an explicit maintainer design decision that should generally be avoided: server.py now answers a tools/call for an unregistered name with a JSON-RPC INVALID_PARAMS error (HTTP 400) instead of the previously pinned CallToolResult(is_error=True, "Unknown tool: <name>"), so Client.call_tool() raises MCPError where it used to return. Fix: confirm this is the explicit maintainer decision the rule asks for (the PR cites #3659) and keep the change scoped to that decision; otherwise retain the is_error result and keep the divergence entry in tests/interaction/_requirements.py.
Why this was flagged
Nothing fails at runtime in the SDK itself. The rule guards 2.x compatibility: existing clients and hosts that handled unknown-tool misses by inspecting result.is_error (the behaviour the previous docs, troubleshooting page and the mcpserver:tool:unknown-name divergence note explicitly promised) will now get an unhandled MCPError from Client.call_tool(), and HTTP integrations expecting a 200 result will see a 400 error body. Mitigating facts for the maintainer to weigh: the PR is authored by Kludex and closes #3659, it aligns MCPServer with the spec example and with the low-level server path, and direct MCPServer.call_tool() callers keep catching ToolError because ToolNotFoundError subclasses it. The divergence between a top-level miss (protocol error) and a nested/override miss (execution error) rests on exc.name == params.name string equality.
Verification: AGENTS.md (base commit) "Branching Model": "v2 is released; its public API is a compatibility contract for the 2.x line. Removals, renames, or any change to an existing API's signature or observable behaviour ... is a design decision a maintainer makes explicitly, and should generally be avoided."
BrickerP
left a comment
BrickerP
left a comment
There was a problem hiding this comment.
Review (code-backed)
Aligns MCPServer with the tools error-handling spec (unknown name → JSON-RPC -32602 / INVALID_PARAMS) and with the TypeScript SDK, and correctly removes the mcpserver:tool:unknown-name divergence. The request-boundary gate is the right shape:
if isinstance(exc, ToolNotFoundError) and exc.name == params.name:
raise MCPError(code=INVALID_PARAMS, message=str(exc)) from excThat keeps nested call_tool("missing") and deliberate ToolError("Unknown tool: …") on the is_error=True path (covered by the new override/nested tests), mirrors ResourceNotFoundError → INVALID_PARAMS, and __reduce__ + the pickle/copy test address the reconstruction trap. CI is green, including interaction/conformance. Author is a listed Python SDK maintainer, so the outside-PR issue gate does not apply.
Looks ready to land from a code-review standpoint, with non-blocking notes:
1. Stale i18n still teaches the old contract (should fix before/with merge)
English docs/tests are updated; localized pages still say a missing tool returns is_error=True and that call_tool does not raise:
i18n/zh/pages/client/index.md(~line 140) andi18n/de/pages/client/index.md(~147–149):does_not_exist→is_error=True/ “nichts wird ausgelöst”i18n/zh/pages/troubleshooting.mdandi18n/de/pages/troubleshooting.md: still under the combined “Error executing tool…andUnknown tool…” heading, with “checkresult.is_error” for unknown namesi18n/zh/pages/handlers/context.md/i18n/de/pages/handlers/context.md: still result-shapedUnknown tool: recommend_bookinstead ofMCPError
Readers on zh/de will follow advice that is now wrong on this branch.
2. Client-visible break — fine under VERSIONING as spec bugfix, still worth a release note
VERSIONING.md allows minor “bug fixes … that make the SDK match its … specified behavior,” and this is exactly that (plus the old divergence was documented). Still, any host that did:
result = await client.call_tool(name, args)
if result.is_error: # handled unknown tools here
...will now see MCPError for missing names. A one-line release-note / migration blurb (“unknown tools/call name is now -32602; check is_error only for execution/validation failures”) would save people who relied on the divergent MCPServer behavior. Optional: breaking change label only if you want the Migration Guide path; not required by the “match the spec” carve-out.
3. Tiny docstring drift
ToolError’s class doc still leads with “The SDK raises it too, for an unknown tool name…” before the new ToolNotFoundError sentence. Prefer “unknown name → ToolNotFoundError (subclass); …” so the first sentence does not imply a bare ToolError.
4. Not blocking
- Tools that
raise ToolNotFoundError(<requested name>)intentionally get a protocol error; the override tests already show plainToolError("Unknown tool: …")staysis_error. Worth a half-sentence inToolNotFoundError’s doc that it is the lookup miss signal, not a general tool-failure type — optional.
Net: correct fix, good tests; please bring zh/de i18n along (or defer with an explicit follow-up) so localized docs do not contradict the new contract.
This branch has not been deployed
2 participants
Closes #3659.
Return JSON-RPC
INVALID_PARAMS(-32602) when the requested tool does not exist, instead ofCallToolResult(is_error=True). A lookup-specificToolNotFoundErrorcarries the missing name, and the request boundary translates it only when that name matches the request; direct calls remain catchable asToolError, dynamiccall_tool()overrides keep working, and nested lookup failures remain execution errors.Update the interaction requirements, HTTP assertions, client examples, and error documentation. Tests cover both protocol modes, removed tools, nested calls, and overrides.
Validation:
./scripts/testpasses with 6,218 tests and 100% coverage;strict-no-coverand all applicable pre-commit checks pass.AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.