Skip to content

fix(mcp): return -32601 for unknown methods, and implement the spec-required ping - #13

Open
LifeInTheWeeds wants to merge 1 commit into
mainfrom
fix/mcp-jsonrpc-error-codes
Open

LifeInTheWeeds wants to merge 1 commit into
mainfrom
fix/mcp-jsonrpc-error-codes

Conversation

@LifeInTheWeeds

Copy link
Copy Markdown
Collaborator

Independent of #12 — different file, no overlap, merges in either order.

Problem

The MCP stdio server has a single error path, so every failure — including "this method is not implemented" — is reported as JSON-RPC -32603:

_ => Err(anyhow!("Unknown method: {}", request.method)),
// ...later, unconditionally:
code: -32603, // Internal error

-32603 means the server itself failed. -32601 means the method is not implemented. Answering a capability probe with "I have failed internally" is a materially different signal, and a strict client may reasonably treat the server as unhealthy.

Three concrete effects, all reproduced against the stdio server on main:

Method Before Why it matters
ping -32603 ping is REQUIRED by the MCP spec and is used for liveness checks. A client that pings during a long session gets an internal error, and the correct reaction to that is to drop the connection.
resources/list -32603 Clients probe this after initialize even when the capability is not advertised.
prompts/list -32603 Same.

After

ping            -> {"result":{}}
resources/list  -> {"result":{"resources":[]}}
prompts/list    -> {"result":{"prompts":[]}}
totally/bogus   -> {"error":{"code":-32601,"message":"Unknown method: totally/bogus"}}

Scope and honesty notes

This does not fix any connection failure. I found it while investigating one whose cause turned out to be entirely unrelated (a Windows DLL-path collision in my own environment). The spec violations are real on their own merits, but I want to be clear this is not a fix for a reported bug.

Build note: main does not currently compile against duckdb >= 1.10505.0, so this branch cannot be built or tested standalone until the first commit of #12 lands. The behaviour above was verified with that fix applied locally.

🤖 Generated with Claude Code

https://claude.ai/code/session_016pWGjWtzGf4UvtgiWqYQyR

The MCP stdio server had a single error path, so every failure -- including
"this method is not implemented" -- was reported as JSON-RPC -32603:

    _ => Err(anyhow!("Unknown method: {}", request.method)),
    ...
    code: -32603, // Internal error

-32603 means the server itself failed. -32601 ("Method not found") means the
method is simply not implemented. Answering a capability probe with "I have
failed internally" is a materially different signal, and a strict client may
reasonably treat the server as unhealthy.

Three concrete effects, all reproduced against the stdio server:

  ping            -> -32603   `ping` is REQUIRED by the MCP specification and
                              is used for liveness checks. A client that pings
                              during a long session gets an internal error and
                              the correct response to that is to drop the
                              connection.
  resources/list  -> -32603   Clients probe these after initialize even when
  prompts/list    -> -32603   the capability is not advertised.

After this change:

    ping            -> {"result":{}}
    resources/list  -> {"result":{"resources":[]}}
    prompts/list    -> {"result":{"prompts":[]}}
    totally/bogus   -> {"error":{"code":-32601,...}}

Scope note: this is independent of the DuckDB CLI work on
`fix/duckdb-cli-engine-gate` and is branched from main so it can be reviewed
and merged on its own. It does NOT fix any connection failure -- it was found
while investigating one whose cause turned out to be unrelated.

Build note: main does not currently compile against duckdb >= 1.10505.0, so
this branch needs the `non_exhaustive` wildcard fix (first commit of
fix/duckdb-cli-engine-gate) to build. The behaviour above was verified with
that fix applied locally.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016pWGjWtzGf4UvtgiWqYQyR
@LifeInTheWeeds

Copy link
Copy Markdown
Collaborator Author

Both red checks here are expected, and neither is a defect in this change.

Rustfmt was genuinely my error and is now fixed (force-pushed ca04708) — it passes.

Suggested order: #12 → #14 → #13. Happy to rebase this once the first two land.

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.

1 participant