Skip to content

Update to MCP 2026-07-28 without backwards compat. - #256

Open
tnull wants to merge 4 commits into
lightningdevkit:mainfrom
tnull:2026-08-update-mcp
Open

Update to MCP 2026-07-28 without backwards compat.#256
tnull wants to merge 4 commits into
lightningdevkit:mainfrom
tnull:2026-08-update-mcp

Conversation

@tnull

@tnull tnull commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Alternative to #255 (which I only saw after opening this).

Here we don't keep the old version around, just go for the new thing.

(cc @benthecarman let me know what you prefer).

Require self-describing 2026-07-28 requests and expose server
discovery so callers no longer depend on initialization state.

Return cache and result metadata required by the new wire contract,
and reject legacy or malformed requests with actionable errors.

Co-Authored-By: HAL 9000
@ldk-reviews-bot

ldk-reviews-bot commented Aug 10, 2026

Copy link
Copy Markdown

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@tnull
tnull marked this pull request as draft August 10, 2026 14:37
@benthecarman

Copy link
Copy Markdown
Collaborator

Yeah based on the goose team's comments this seems like the better way to go

@tnull
tnull marked this pull request as ready for review August 11, 2026 08:22
Comment thread ldk-server-mcp/src/main.rs Outdated
Comment thread ldk-server-mcp/src/main.rs Outdated
Comment thread ldk-server-mcp/tests/integration.rs Outdated
Comment thread ldk-server-mcp/src/main.rs Outdated
tnull added 3 commits August 13, 2026 10:34
Dispatch unsupported methods before validating request metadata so
legacy and unknown methods receive method-not-found errors. Compare
protocol versions before validating other metadata so unsupported
clients receive actionable negotiation details.

Reuse the production protocol version in integration tests and keep
request handling helpers below the entry point.

Co-Authored-By: HAL 9000
Send modern request metadata from the live test harness and
replace the legacy initialization check with server discovery
coverage.

Verify cache and server metadata on discovery, listing, and
live tool responses so the stateless wire contract is exercised
against a node.

Co-Authored-By: HAL 9000
Describe the required per-request metadata and discovery flow so
MCP clients use the server without the removed initialization
handshake.

Call out the latest-only compatibility boundary and supported
stdio method surface.

Co-Authored-By: HAL 9000
@tnull
tnull force-pushed the 2026-08-update-mcp branch from 09cdb76 to c5b1b2b Compare August 13, 2026 08:41
@tnull
tnull requested a review from benthecarman August 13, 2026 08:42

@benthecarman benthecarman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks good to me, just some final review comments on correctness from claude

Ok(ToolCallResult::success(text))
},
Err(e) => ToolCallResult::error(format!("{}: {}", e.category(), e.message)),
Err(e) if e.is_tool_execution() => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argument-validation errors now violate the spec (tools/mod.rs:66). I earlier relayed that converting invalid-argument errors to JSON-RPC -32602 was spec-prescribed — that was wrong. The spec’s tools Error Handling section explicitly lists input validation errors (“value out of range”, wrong format) under Tool Execution Errors reported with isError: true, reserving protocol errors for unknown tools and schema-shape failures. The PR moves all 37 tools’ argument parsing to hard -32602 envelope errors, which breaks the LLM self-correction loop the spec designed isError for. This is the most consequential finding after the notification bug.

return error_response(id, INVALID_PARAMS, "Missing required parameter: name");
};
let tool_args = match params.get("arguments") {
Some(arguments) if !arguments.is_object() => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"arguments": null is rejected while an omitted key succeeds (main.rs:175). Confirmed by running the binary — many client serializers emit null for “no arguments,” and the old code tolerated it.

};

let id = message.get("id").cloned();
let response_id = id

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ldk-server-mcp/src/protocol.rsJsonRpcRequest::from_value: the !valid early return runs before the let Some(id) = id else { return Ok(None) } notification bail-out, so a malformed notification (e.g. {"jsonrpc":"2.0","method":"notifications/cancelled","params":null}) gets an id: null error response on stdout. JSON-RPC 2.0 and MCP both say notifications MUST NOT receive a response, and this crate’s CLAUDE.md says they’re ignored. Also, an explicit "id": null now maps to Some(Value::Null) and gets rejected, where serde previously folded it to None (silent).

Fix: move the notification bail-out above the validity check and treat a null id as absent, e.g. let Some(id) = id.filter(|id| !id.is_null()) else { return Ok(None); }; — then simplify response_id and the id clause of valid, which can assume a present, non-null id. Please also extend test_notification_no_response with a params: null (or array-params) notification asserting no output line.

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.

3 participants