-
Notifications
You must be signed in to change notification settings - Fork 705
Deprecate McpErrorCode.ResourceNotFound (-32002) and use McpErrorCode.InvalidParams (-32602) per SEP-2164
#1558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
2ce5669
af62cc2
9946a4f
4e00bb7
a794abc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,30 @@ internal static bool SupportsPrimingEvent(string? protocolVersion) | |
| return string.Compare(protocolVersion, MinResumabilityProtocolVersion, StringComparison.Ordinal) >= 0; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Checks whether the negotiated protocol version takes the SEP-2164 error code for | ||
| /// unresolvable resource URIs (<see cref="McpErrorCode.InvalidParams"/>, -32602) rather | ||
| /// than the legacy <see cref="McpErrorCode.ResourceNotFound"/> (-32002). | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// SEP-2164 (MCP spec 2026-06-30) replaces the legacy code with the standard JSON-RPC | ||
| /// <see cref="McpErrorCode.InvalidParams"/>. Returns <see langword="true"/> for protocol | ||
| /// versions that sort lexically >= <c>"2026-06-30"</c>; this also covers the in-flight | ||
| /// draft string <c>"DRAFT-2026-06-v1"</c> because <c>'D'</c> sorts greater than <c>'2'</c> | ||
| /// in ASCII. | ||
| /// </remarks> | ||
| internal static bool UseSep2164ResourceErrors(string? protocolVersion) | ||
| { | ||
| const string MinSep2164ProtocolVersion = "2026-06-30"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for updating this. Now that #1553 has been merged, you can add this logic to |
||
|
|
||
| if (protocolVersion is null) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return string.Compare(protocolVersion, MinSep2164ProtocolVersion, StringComparison.Ordinal) >= 0; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For now, only use the InvalidParams error codes if there's an exact ordinal match with "DRAFT-2026-v1". We shouldn't be doing a |
||
| } | ||
|
|
||
| private readonly bool _isServer; | ||
| private readonly string _transportKind; | ||
| private readonly ITransport _transport; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.Options; | ||
| using ModelContextProtocol.Client; | ||
| using ModelContextProtocol.Protocol; | ||
| using ModelContextProtocol.Server; | ||
|
|
@@ -22,6 +23,27 @@ private async Task<McpClient> CreateClientWithResourcesAsync(params McpServerRes | |
| return await CreateMcpClientForServer(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Starts the server with the specified resources, pins both the server's and the | ||
| /// client's protocol version to <paramref name="protocolVersion"/>, and returns a | ||
| /// connected client. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Both ends must be pinned because <see cref="McpClient"/> strictly compares the | ||
| /// server's negotiated version against the client's requested version and refuses to | ||
| /// connect on mismatch. Used to drive the SEP-2164 error-code gate from tests without | ||
| /// bumping the SDK's <c>SupportedProtocolVersions</c> array. | ||
| /// </remarks> | ||
| private async Task<McpClient> CreateClientWithResourcesAndServerVersionAsync( | ||
| string protocolVersion, | ||
| params McpServerResource[] resources) | ||
| { | ||
| McpServerBuilder.WithResources(resources); | ||
| McpServerBuilder.Services.Configure<McpServerOptions>(o => o.ProtocolVersion = protocolVersion); | ||
| StartServer(); | ||
| return await CreateMcpClientForServer(new McpClientOptions { ProtocolVersion = protocolVersion }); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Asserts that the given URI matches the template and produces the expected text result. | ||
| /// </summary> | ||
|
|
@@ -40,20 +62,54 @@ private async Task AssertMatchAsync( | |
| } | ||
|
|
||
| /// <summary> | ||
| /// Asserts that the given URI does NOT match the template. | ||
| /// Asserts that the given URI does NOT match the template. Server is pinned to the | ||
| /// 2026-06-30 protocol version so the SEP-2164 gate produces <see cref="McpErrorCode.InvalidParams"/> | ||
| /// rather than the legacy <c>ResourceNotFound</c>. | ||
| /// </summary> | ||
| private async Task AssertNoMatchAsync( | ||
| string uriTemplate, | ||
| Delegate method, | ||
| string uri) | ||
| { | ||
| var resource = McpServerResource.Create(options: new() { UriTemplate = uriTemplate }, method: method); | ||
| var client = await CreateClientWithResourcesAsync(resource); | ||
| var client = await CreateClientWithResourcesAndServerVersionAsync("2026-06-30", resource); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should update the generic "AssertNoMatchAsync" test helper method to use a draft protocol version. That should be explicitly opted into with theory data or at least a literal in the top-level test. |
||
|
|
||
| var ex = await Assert.ThrowsAsync<McpProtocolException>(async () => | ||
| await client.ReadResourceAsync(uri, null, TestContext.Current.CancellationToken)); | ||
|
|
||
| Assert.Equal(McpErrorCode.ResourceNotFound, ex.ErrorCode); | ||
| Assert.Equal(McpErrorCode.InvalidParams, ex.ErrorCode); | ||
| } | ||
|
|
||
| // SEP-2164 standardizes the error code for "unknown resource URI" from the legacy | ||
| // -32002 (McpErrorCode.ResourceNotFound) to the standard JSON-RPC -32602 | ||
| // (McpErrorCode.InvalidParams). Per Mike Kistler's review on PR #1558, this is | ||
| // version-gated to keep pre-2026-06-30 clients working: when the negotiated protocol | ||
| // version is < "2026-06-30", the SDK still returns the legacy code. | ||
| // | ||
| // The three inline cases below exercise: | ||
| // - "2025-11-25" — the latest pre-SEP-2164 protocol version. | ||
| // - "DRAFT-2026-06-v1" — the in-flight draft string used during spec review. | ||
| // - "2026-06-30" — the anticipated final spec version. | ||
| // | ||
| // The draft string is included because lexically `'D' > '2'`, so the SDK's | ||
| // string-comparison-based gate (matching the precedent set by `SupportsPrimingEvent`) | ||
| // routes both the draft and the final version through the new code path. | ||
| [Theory] | ||
| [InlineData("2025-11-25", McpErrorCode.ResourceNotFound)] | ||
| [InlineData("DRAFT-2026-06-v1", McpErrorCode.InvalidParams)] | ||
| [InlineData("2026-06-30", McpErrorCode.InvalidParams)] | ||
| public async Task ResourceNotFound_ErrorCode_IsVersionGated(string serverProtocolVersion, McpErrorCode expectedCode) | ||
| { | ||
| var resource = McpServerResource.Create( | ||
| options: new() { UriTemplate = "test://known/{id}" }, | ||
| method: (string id) => $"ok: {id}"); | ||
|
|
||
| var client = await CreateClientWithResourcesAndServerVersionAsync(serverProtocolVersion, resource); | ||
|
|
||
| var ex = await Assert.ThrowsAsync<McpProtocolException>(async () => | ||
| await client.ReadResourceAsync("test://unknown", null, TestContext.Current.CancellationToken)); | ||
|
|
||
| Assert.Equal(expectedCode, ex.ErrorCode); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -63,8 +119,10 @@ private async Task AssertNoMatchAsync( | |
| [Fact] | ||
| public async Task MultipleTemplatedResources_MatchesCorrectResource() | ||
| { | ||
| // Register templates from most specific to least specific | ||
| var client = await CreateClientWithResourcesAsync( | ||
| // Server pinned to the SEP-2164-aware protocol version so the no-match assertion at | ||
| // the bottom of this test sees InvalidParams rather than the legacy ResourceNotFound. | ||
| var client = await CreateClientWithResourcesAndServerVersionAsync( | ||
| "2026-06-30", | ||
| McpServerResource.Create(options: new() { UriTemplate = "test://resource/non-templated" }, method: () => "static"), | ||
| McpServerResource.Create(options: new() { UriTemplate = "test://resource/{id}" }, method: (string id) => $"template: {id}"), | ||
| McpServerResource.Create(options: new() { UriTemplate = "test://params{?a1,a2,a3}" }, method: (string a1, string a2, string a3) => $"params: {a1}, {a2}, {a3}"), | ||
|
|
@@ -92,7 +150,7 @@ public async Task MultipleTemplatedResources_MatchesCorrectResource() | |
|
|
||
| // Literal template braces in URI should not match (template literal is not a valid URI) | ||
| var mcpEx = await Assert.ThrowsAsync<McpProtocolException>(async () => await client.ReadResourceAsync("test://params{?a1,a2,a3}", null, TestContext.Current.CancellationToken)); | ||
| Assert.Equal(McpErrorCode.ResourceNotFound, mcpEx.ErrorCode); | ||
| Assert.Equal(McpErrorCode.InvalidParams, mcpEx.ErrorCode); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's not update the generic tests to use the draft protocol version just yet. We can do this later. |
||
| Assert.Equal("Request failed (remote): Unknown resource URI: 'test://params{?a1,a2,a3}'", mcpEx.Message); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove any mention of "SEP-2164" or SEPs in general from method names and comments. I know it's internal, but the individual SEPs won't matter much after this PR is merged.