From 4f28eb5522c4292df4eebf276fa4f5e6d2bfd275 Mon Sep 17 00:00:00 2001 From: = Date: Thu, 6 Aug 2026 23:46:10 +0200 Subject: [PATCH 1/2] fix(mcp): bind the request-scoped server so roots survive the MCP v2 upgrade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bumps ModelContextProtocol 1.4.1 -> 2.0.0. The upgrade broke the build on four MCP9005 errors: the Roots feature is deprecated by specification version 2026-07-28 (SEP-2577). Roots stays wire-supported for at least twelve months and its replacement — taking the workspace root as a tool parameter or as server configuration — is a behavioural change, so it is kept and the diagnostic is suppressed narrowly at each call site rather than project-wide. Silencing the diagnostic then surfaced a real regression. From 2026-07-28 there is no initialize handshake; the client restates its capabilities per request in _meta, so ClientCapabilities is null on the root McpServer and is populated only on the request-scoped instance the SDK binds to a tool-method parameter. ProjGraphTools is a singleton and took McpServer as a constructor parameter, so it always held the root instance: every relative path would have failed with "client does not support workspace roots" for any client on the current revision. McpServer moves to the tool methods, where the SDK binds it per request and excludes it from the tool's JSON schema. That revision also drops the session that roots/list_changed invalidates, so caching the roots across requests would serve them stale for the life of the process. The roots are now re-read per request there, and cached only on the initialize-handshake revisions that can invalidate them. "Unsupported" is never cached either, since a request without roots says nothing about the next one. A client that advertises roots but refuses the request degrades to the actionable "provide an absolute path" guidance instead of a generic error. McpToolRootsTests drives real tools/call requests, which is the only way to exercise the request-scoped server; the pre-existing roots tests drive the root McpServer directly and are pinned to 2025-11-25 accordingly. The six tests that reached the resolver through a reflection-seeded cache and a null server now use a live session instead. Co-Authored-By: Claude Opus 5 (1M context) --- Directory.Packages.props | 2 +- src/ProjGraph.Mcp/ProjGraphTools.cs | 37 +++++- src/ProjGraph.Mcp/README.md | 8 +- src/ProjGraph.Mcp/WorkspaceRootService.cs | 76 ++++++++++-- .../McpErdContractTests.cs | 7 +- .../McpProjectGraphContractTests.cs | 7 +- .../McpProjectStatsContractTests.cs | 8 +- .../Helpers/InProcessMcpSession.cs | 16 ++- .../Helpers/McpTestHelper.cs | 1 - .../McpRootsTests.cs | 69 ++++++----- .../McpToolRootsTests.cs | 108 ++++++++++++++++++ .../WorkspaceRootServiceTests.cs | 2 + 12 files changed, 280 insertions(+), 61 deletions(-) create mode 100644 tests/ProjGraph.Tests.Integration.Mcp/McpToolRootsTests.cs diff --git a/Directory.Packages.props b/Directory.Packages.props index 67721b5..fc2f2bc 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -17,7 +17,7 @@ - + diff --git a/src/ProjGraph.Mcp/ProjGraphTools.cs b/src/ProjGraph.Mcp/ProjGraphTools.cs index 7f0249f..5a412ef 100644 --- a/src/ProjGraph.Mcp/ProjGraphTools.cs +++ b/src/ProjGraph.Mcp/ProjGraphTools.cs @@ -28,7 +28,6 @@ internal sealed class ProjGraphTools( DiagramRenderers renderers, IFileSystem fileSystem, DiagramResourceCache cache, - McpServer server, WorkspaceRootService rootService, CollectingOutputConsole outputConsole) { @@ -48,6 +47,9 @@ public async Task GetClassDiagramAsync( [Description("Whether to include the title in the diagram (default: true).")] bool showTitle = true, IProgress? progress = null, + // Request-scoped, never the singleton's root instance — see PreparePathAsync. Defaulted so + // tests can call the method directly with an absolute path, which never consults the server. + McpServer server = null!, CancellationToken cancellationToken = default) { if (options is { MaxDepth: < 0 }) @@ -56,7 +58,7 @@ public async Task GetClassDiagramAsync( throw new McpException($"maxDepth must not be negative; got {options.MaxDepth}."); } - path = await PreparePathAsync(path, cancellationToken); + path = await PreparePathAsync(path, server, cancellationToken); if (!fileSystem.FileExists(path) && !fileSystem.DirectoryExists(path)) throw new McpException($"Path not found: {path}"); @@ -133,9 +135,12 @@ public async Task GetProjectGraphAsync( [Description("Whether to include NuGet package dependencies in the graph (default: false).")] bool includePackages = false, IProgress? progress = null, + // Request-scoped, never the singleton's root instance — see PreparePathAsync. Defaulted so + // tests can call the method directly with an absolute path, which never consults the server. + McpServer server = null!, CancellationToken cancellationToken = default) { - path = await PreparePathAsync(path, cancellationToken); + path = await PreparePathAsync(path, server, cancellationToken); progress?.Report(new ProgressNotificationValue { @@ -187,6 +192,9 @@ public async Task GetProjectStatsAsync( [Description("Number of top most-referenced projects to include. Defaults to 5.")] int topN = 5, IProgress? progress = null, + // Request-scoped, never the singleton's root instance — see PreparePathAsync. Defaulted so + // tests can call the method directly with an absolute path, which never consults the server. + McpServer server = null!, CancellationToken cancellationToken = default) { if (topN < 1) @@ -194,7 +202,7 @@ public async Task GetProjectStatsAsync( throw new McpException($"topN must be at least 1; got {topN}."); } - path = await PreparePathAsync(path, cancellationToken); + path = await PreparePathAsync(path, server, cancellationToken); progress?.Report(new ProgressNotificationValue { @@ -248,6 +256,9 @@ public async Task GetErdAsync( [Description("How EF Core owned types are shown: 'mirror' (default) inlines table-split owned types onto the owner as EF names them; 'classic' gives every owned type its own entity")] string ownedMode = "mirror", IProgress? progress = null, + // Request-scoped, never the singleton's root instance — see PreparePathAsync. Defaulted so + // tests can call the method directly with an absolute path, which never consults the server. + McpServer server = null!, CancellationToken cancellationToken = default) { if (!ErdOwnedModeParser.TryParse(ownedMode, out var mode)) @@ -260,7 +271,7 @@ public async Task GetErdAsync( throw new McpException($"Invalid ownedMode '{ownedMode}'. Expected 'mirror' or 'classic'."); } - path = await PreparePathAsync(path, cancellationToken); + path = await PreparePathAsync(path, server, cancellationToken); RequireFileExists(path); RequireCsFile(path); @@ -437,7 +448,21 @@ private static async Task RunAnalysisAsync(Func> analysis) } } - private async Task PreparePathAsync(string path, CancellationToken cancellationToken) + /// + /// Validates the requested path and resolves it to an absolute one, using the client's + /// workspace roots when it is relative. + /// + /// The path supplied by the client. + /// + /// The request-scoped bound to the tool-method parameter, never one + /// captured at construction time: from protocol revision 2026-07-28 the client declares its + /// capabilities per request in _meta rather than in an initialize handshake, so + /// is populated only on the request-scoped instance. + /// + /// A token to cancel the operation. + /// The resolved absolute path. + /// Thrown when the path is empty or cannot be resolved. + private async Task PreparePathAsync(string path, McpServer server, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); if (string.IsNullOrWhiteSpace(path)) diff --git a/src/ProjGraph.Mcp/README.md b/src/ProjGraph.Mcp/README.md index 5453fd4..04c1a76 100644 --- a/src/ProjGraph.Mcp/README.md +++ b/src/ProjGraph.Mcp/README.md @@ -75,7 +75,13 @@ Diagram resources are automatically created when tools generate output. Clients ### Roots The server resolves relative file paths against workspace roots declared by the client. When a client declares roots via -`roots/list`, passing `"MyApp.slnx"` instead of `"D:/Projects/MyApp/MyApp.slnx"` just works. +`roots/list`, passing `"MyApp.slnx"` instead of `"D:/Projects/MyApp/MyApp.slnx"` just works. A client that declares no +roots is asked to pass an absolute path instead. + +Roots is deprecated by specification version 2026-07-28 ([SEP-2577](https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/seps/2577-deprecate-roots-sampling-and-logging.md)) +and stays supported for at least twelve months. Until it is retired the server keeps serving it on both that revision and +down-level ones; on 2026-07-28 the roots are re-read per request, since that revision drops the session that +`roots/list_changed` would otherwise invalidate. ### Progress Notifications diff --git a/src/ProjGraph.Mcp/WorkspaceRootService.cs b/src/ProjGraph.Mcp/WorkspaceRootService.cs index 696c1ad..4d9687d 100644 --- a/src/ProjGraph.Mcp/WorkspaceRootService.cs +++ b/src/ProjGraph.Mcp/WorkspaceRootService.cs @@ -6,6 +6,25 @@ namespace ProjGraph.Mcp; +/// +/// Resolves relative paths supplied to the MCP tools against the client's workspace roots. +/// +/// +/// The Roots feature is deprecated by specification version 2026-07-28 (SEP-2577), which is why the +/// SDK calls below are wrapped in MCP9005 suppressions. It stays wire-supported for at least +/// twelve months — removing it needs a separate SEP — so it is still served here, on both the new +/// revision and down-level ones. A client that does not advertise the capability degrades to +/// and is told to pass an absolute path. Retiring it means +/// taking the workspace root as a tool parameter or as server configuration, which is a behavioural +/// change tracked separately from this SDK upgrade. +/// +/// +/// Every method takes the request-scoped . From 2026-07-28 there is no +/// initialize handshake: the client restates its capabilities per request in _meta, so +/// is null on the root server and populated only on the +/// instance the SDK binds to a tool-method parameter. +/// +/// The file system used to probe candidate paths under each root. internal sealed class WorkspaceRootService(IFileSystem fileSystem) : IAsyncDisposable { private readonly SemaphoreSlim _initLock = new(1, 1); @@ -143,6 +162,7 @@ private void EnsureRootsChangedHandler(McpServer server) // Only mark as registered after a successful call, so a failed registration can be retried // on the next initialization instead of permanently disabling roots invalidation. +#pragma warning disable MCP9005 // Roots is deprecated (SEP-2577); still served for down-level clients. See the file header. _rootsChangedRegistration = server.RegisterNotificationHandler( NotificationMethods.RootsListChangedNotification, (_, _) => @@ -150,12 +170,15 @@ private void EnsureRootsChangedHandler(McpServer server) InvalidateRoots(); return default; }); +#pragma warning restore MCP9005 _notificationHandlerRegistered = true; } internal async Task RefreshRootsAsync(McpServer server, CancellationToken ct) { +#pragma warning disable MCP9005 // Roots is deprecated (SEP-2577); still served for down-level clients. See the file header. var result = await server.RequestRootsAsync(new ListRootsRequestParams(), ct); +#pragma warning restore MCP9005 var paths = new List(); foreach (var root in result.Roots) { @@ -166,10 +189,41 @@ internal async Task RefreshRootsAsync(McpServer server, CancellationToken ct) _status = RootsStatusKind.Ready; } + /// + /// Indicates whether the connection established client state once, via the initialize + /// handshake (protocol revision 2025-11-25 and earlier). Only such a connection has a + /// session for roots to be cached against and a durable channel for the client's + /// roots/list_changed notification to invalidate that cache; from 2026-07-28 the + /// client restates its capabilities on every request instead, so the roots are re-fetched each + /// time rather than served stale for the rest of the process's life. + /// + /// The request-scoped server handling the current request. + /// when client state is session-scoped. + private static bool UsesSessionScopedCapabilities(McpServer server) + { + // Date-based revisions order correctly under an ordinal comparison. + var version = server.NegotiatedProtocolVersion; + return version is not null && string.CompareOrdinal(version, "2026-07-28") < 0; + } + private async Task EnsureInitializedAsync(McpServer server, CancellationToken ct) { - if (_status != RootsStatusKind.Unknown) + var sessionScoped = UsesSessionScopedCapabilities(server); + + if (sessionScoped && _status == RootsStatusKind.Ready) + { + return; + } + + // "Unsupported" is deliberately never served from the cache: from protocol revision + // 2026-07-28 the client declares its capabilities per request in _meta, so a request that + // arrives without roots says nothing about the next one. Re-reading costs nothing — it is a + // property, not a round trip. +#pragma warning disable MCP9005 // Roots is deprecated (SEP-2577); still served for down-level clients. See the file header. + if (server.ClientCapabilities?.Roots is null) +#pragma warning restore MCP9005 { + _status = RootsStatusKind.Unsupported; return; } @@ -178,20 +232,28 @@ private async Task EnsureInitializedAsync(McpServer server, CancellationToken ct { // Double-checked locking: re-check after acquiring lock #pragma warning disable CA1508 // Avoid dead conditional code — volatile field may change between outer check and lock acquisition - if (_status != RootsStatusKind.Unknown) + if (sessionScoped && _status == RootsStatusKind.Ready) { return; } #pragma warning restore CA1508 - if (server.ClientCapabilities?.Roots is null) + if (sessionScoped) { - _status = RootsStatusKind.Unsupported; - return; + EnsureRootsChangedHandler(server); } - EnsureRootsChangedHandler(server); - await RefreshRootsAsync(server, ct); + try + { + await RefreshRootsAsync(server, ct); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + // A client can advertise the capability and still refuse the request — most likely + // once it drops the deprecated feature. Degrade to Unsupported so the caller gets + // the actionable "provide an absolute path" guidance instead of a generic SDK error. + _status = RootsStatusKind.Unsupported; + } } finally { diff --git a/tests/ProjGraph.Tests.Contract/McpErdContractTests.cs b/tests/ProjGraph.Tests.Contract/McpErdContractTests.cs index 1422826..203e2ca 100644 --- a/tests/ProjGraph.Tests.Contract/McpErdContractTests.cs +++ b/tests/ProjGraph.Tests.Contract/McpErdContractTests.cs @@ -83,9 +83,10 @@ public void GetErd_ShouldHave_Parameters() var method = type.GetMethod("GetErdAsync"); var parameters = method!.GetParameters(); - // Assert parameters exist (path, contextName, showTitle, ownedMode, progress, cancellationToken) - parameters.Should().HaveCount(6, - "GetErd should have 6 parameters: path, contextName, showTitle, ownedMode, progress, and cancellationToken"); + // Assert parameters exist. 'server' is bound by the SDK to the request-scoped McpServer and + // is excluded from the tool's JSON schema, so it does not widen the client-facing contract. + parameters.Should().HaveCount(7, + "GetErd should have 7 parameters: path, contextName, showTitle, ownedMode, progress, server, and cancellationToken"); var pathParam = parameters.Should().ContainSingle(p => p.Name == "path").Which; pathParam.ParameterType.Should().Be(); diff --git a/tests/ProjGraph.Tests.Contract/McpProjectGraphContractTests.cs b/tests/ProjGraph.Tests.Contract/McpProjectGraphContractTests.cs index 692c471..e4d6c90 100644 --- a/tests/ProjGraph.Tests.Contract/McpProjectGraphContractTests.cs +++ b/tests/ProjGraph.Tests.Contract/McpProjectGraphContractTests.cs @@ -59,9 +59,10 @@ public void GetProjectGraph_ShouldHave_Parameters() var method = type.GetMethod("GetProjectGraphAsync"); var parameters = method!.GetParameters(); - // Assert parameters exist (path, showTitle, includePackages, progress, cancellationToken) - parameters.Should().HaveCount(5, - "GetProjectGraph should have 'path', 'showTitle', 'includePackages', 'progress', and 'cancellationToken' parameters"); + // Assert parameters exist. 'server' is bound by the SDK to the request-scoped McpServer and + // is excluded from the tool's JSON schema, so it does not widen the client-facing contract. + parameters.Should().HaveCount(6, + "GetProjectGraph should have 'path', 'showTitle', 'includePackages', 'progress', 'server', and 'cancellationToken' parameters"); var pathParam = parameters.Should().ContainSingle(p => p.Name == "path").Which; pathParam.ParameterType.Should().Be(); diff --git a/tests/ProjGraph.Tests.Contract/McpProjectStatsContractTests.cs b/tests/ProjGraph.Tests.Contract/McpProjectStatsContractTests.cs index 9f9ff8f..6efac2d 100644 --- a/tests/ProjGraph.Tests.Contract/McpProjectStatsContractTests.cs +++ b/tests/ProjGraph.Tests.Contract/McpProjectStatsContractTests.cs @@ -86,8 +86,10 @@ public void GetProjectStats_ShouldHave_CorrectParameterCount() var method = typeof(ProjGraphTools).GetMethod("GetProjectStatsAsync"); var parameters = method!.GetParameters(); - // path, topN, progress, cancellationToken = 4 parameters - parameters.Should().HaveCount(4, - "GetProjectStats should have 'path', 'topN', 'progress', and 'cancellationToken' parameters"); + // path, topN, progress, server, cancellationToken = 5 parameters. 'server' is bound by the + // SDK to the request-scoped McpServer and is excluded from the tool's JSON schema, so it + // does not widen the client-facing contract. + parameters.Should().HaveCount(5, + "GetProjectStats should have 'path', 'topN', 'progress', 'server', and 'cancellationToken' parameters"); } } diff --git a/tests/ProjGraph.Tests.Integration.Mcp/Helpers/InProcessMcpSession.cs b/tests/ProjGraph.Tests.Integration.Mcp/Helpers/InProcessMcpSession.cs index 82f6472..039d611 100644 --- a/tests/ProjGraph.Tests.Integration.Mcp/Helpers/InProcessMcpSession.cs +++ b/tests/ProjGraph.Tests.Integration.Mcp/Helpers/InProcessMcpSession.cs @@ -82,10 +82,23 @@ public static McpServerOptions CreateServerOptions() /// /// Supplies the workspace root directories for each request. /// Client options that answer roots/list from . - public static McpClientOptions CreateClientOptionsWithRoots(Func> rootProvider) + /// + /// When (the default), pins the client to the 2025-11-25 + /// handshake so client capabilities are established session-scoped on the root + /// — required by tests that drive that root instance directly. When + /// , the client negotiates the latest revision, where capabilities + /// arrive per request in _meta and are visible only on the request-scoped server. + /// + public static McpClientOptions CreateClientOptionsWithRoots( + Func> rootProvider, + bool pinDownLevel = true) { + // Roots is deprecated (SEP-2577) but still served by WorkspaceRootService for down-level + // clients, so this harness keeps exercising it. +#pragma warning disable MCP9005 return new McpClientOptions { + ProtocolVersion = pinDownLevel ? "2025-11-25" : null, ClientInfo = new Implementation { Name = "ProjGraph.Tests", @@ -109,6 +122,7 @@ public static McpClientOptions CreateClientOptionsWithRoots(Func diff --git a/tests/ProjGraph.Tests.Integration.Mcp/Helpers/McpTestHelper.cs b/tests/ProjGraph.Tests.Integration.Mcp/Helpers/McpTestHelper.cs index 0739695..f143f0b 100644 --- a/tests/ProjGraph.Tests.Integration.Mcp/Helpers/McpTestHelper.cs +++ b/tests/ProjGraph.Tests.Integration.Mcp/Helpers/McpTestHelper.cs @@ -59,7 +59,6 @@ public static ProjGraphTools CreateTools(CollectingOutputConsole console, Diagra new MermaidErdRenderer()), fs, cache ?? new DiagramResourceCache(), - null!, new WorkspaceRootService(fs), console); } diff --git a/tests/ProjGraph.Tests.Integration.Mcp/McpRootsTests.cs b/tests/ProjGraph.Tests.Integration.Mcp/McpRootsTests.cs index 1110d26..6ad98dd 100644 --- a/tests/ProjGraph.Tests.Integration.Mcp/McpRootsTests.cs +++ b/tests/ProjGraph.Tests.Integration.Mcp/McpRootsTests.cs @@ -1,6 +1,7 @@ using ModelContextProtocol; using ProjGraph.Lib.Core.Infrastructure; using ProjGraph.Mcp; +using ProjGraph.Tests.Integration.Mcp.Helpers; using ProjGraph.Tests.Shared.Helpers; using System.Reflection; @@ -24,15 +25,16 @@ public async Task TryResolve_AbsolutePath_ShouldPassThrough() [Fact] public async Task TryResolve_RelativePath_NoRootsCapability_ShouldThrow() { - var service = new WorkspaceRootService(new PhysicalFileSystem()); + await using var session = await InProcessMcpSession.StartAsync( + clientOptions: InProcessMcpSession.CreateClientOptionsWithoutRoots()); + await using var service = new WorkspaceRootService(new PhysicalFileSystem()); - // Using a McpServer with null ClientCapabilities fails, so pass null - // which exercises the Unsupported path when server capabilities are unavailable - var act = async () => await service.TryResolveAsync("MySolution.slnx", null!, CancellationToken.None); + var act = async () => await service.TryResolveAsync("MySolution.slnx", session.Server, CancellationToken.None); - // Without a server, we expect a NullReferenceException trying to access ClientCapabilities - // In production, this is handled by the MCP server providing capabilities - await act.Should().ThrowAsync(); + // McpException so the guidance reaches the client; the SDK strips the message from any + // other exception type. + await act.Should().ThrowAsync() + .WithMessage("*does not support workspace roots*absolute path*"); } [Fact] @@ -150,10 +152,11 @@ public async Task TryResolve_RelativePath_FileFoundInRoot_ShouldReturnFullPath() const string fileName = "MySolution.slnx"; var filePath = _temp.CreateFile(fileName, ""); - var service = new WorkspaceRootService(new PhysicalFileSystem()); - SetRoots(service, [_temp.DirectoryPath]); + await using var session = await InProcessMcpSession.StartAsync( + clientOptions: InProcessMcpSession.CreateClientOptionsWithRoots(() => [_temp.DirectoryPath])); + await using var service = new WorkspaceRootService(new PhysicalFileSystem()); - var result = await service.TryResolveAsync(fileName, null!, CancellationToken.None); + var result = await service.TryResolveAsync(fileName, session.Server, CancellationToken.None); result.Should().Be(filePath); } @@ -164,10 +167,11 @@ public async Task TryResolve_RelativePath_FileFoundInSubdirectory_ShouldReturnFu const string fileName = "Deep.slnx"; var filePath = _temp.CreateFile(Path.Combine("src", "nested", fileName), ""); - var service = new WorkspaceRootService(new PhysicalFileSystem()); - SetRoots(service, [_temp.DirectoryPath]); + await using var session = await InProcessMcpSession.StartAsync( + clientOptions: InProcessMcpSession.CreateClientOptionsWithRoots(() => [_temp.DirectoryPath])); + await using var service = new WorkspaceRootService(new PhysicalFileSystem()); - var result = await service.TryResolveAsync(fileName, null!, CancellationToken.None); + var result = await service.TryResolveAsync(fileName, session.Server, CancellationToken.None); result.Should().Be(filePath); } @@ -175,10 +179,11 @@ public async Task TryResolve_RelativePath_FileFoundInSubdirectory_ShouldReturnFu [Fact] public async Task TryResolve_RelativePath_FileNotFound_ShouldThrowMcpException() { - var service = new WorkspaceRootService(new PhysicalFileSystem()); - SetRoots(service, [_temp.DirectoryPath]); + await using var session = await InProcessMcpSession.StartAsync( + clientOptions: InProcessMcpSession.CreateClientOptionsWithRoots(() => [_temp.DirectoryPath])); + await using var service = new WorkspaceRootService(new PhysicalFileSystem()); - var act = async () => await service.TryResolveAsync("missing.slnx", null!, CancellationToken.None); + var act = async () => await service.TryResolveAsync("missing.slnx", session.Server, CancellationToken.None); // McpException so the not-found guidance reaches the client instead of a stripped generic error. await act.Should().ThrowAsync() @@ -194,10 +199,12 @@ public async Task TryResolve_RelativePath_AmbiguousMatch_ShouldThrowMcpException using var temp2 = new TestDirectory(); temp2.CreateFile(fileName, ""); - var service = new WorkspaceRootService(new PhysicalFileSystem()); - SetRoots(service, [_temp.DirectoryPath, temp2.DirectoryPath]); + await using var session = await InProcessMcpSession.StartAsync( + clientOptions: InProcessMcpSession.CreateClientOptionsWithRoots( + () => [_temp.DirectoryPath, temp2.DirectoryPath])); + await using var service = new WorkspaceRootService(new PhysicalFileSystem()); - var act = async () => await service.TryResolveAsync(fileName, null!, CancellationToken.None); + var act = async () => await service.TryResolveAsync(fileName, session.Server, CancellationToken.None); // McpException so the ambiguity guidance reaches the client instead of a stripped generic error. await act.Should().ThrowAsync() @@ -210,10 +217,11 @@ public async Task TryResolve_RelativePath_FileInsideBinDirectory_ShouldNotBeFoun const string fileName = "Hidden.slnx"; _temp.CreateFile(Path.Combine("bin", fileName), ""); - var service = new WorkspaceRootService(new PhysicalFileSystem()); - SetRoots(service, [_temp.DirectoryPath]); + await using var session = await InProcessMcpSession.StartAsync( + clientOptions: InProcessMcpSession.CreateClientOptionsWithRoots(() => [_temp.DirectoryPath])); + await using var service = new WorkspaceRootService(new PhysicalFileSystem()); - var act = async () => await service.TryResolveAsync(fileName, null!, CancellationToken.None); + var act = async () => await service.TryResolveAsync(fileName, session.Server, CancellationToken.None); await act.Should().ThrowAsync(); } @@ -224,10 +232,11 @@ public async Task TryResolve_RelativePath_FileInsideObjDirectory_ShouldNotBeFoun const string fileName = "Artifact.slnx"; _temp.CreateFile(Path.Combine("obj", fileName), ""); - var service = new WorkspaceRootService(new PhysicalFileSystem()); - SetRoots(service, [_temp.DirectoryPath]); + await using var session = await InProcessMcpSession.StartAsync( + clientOptions: InProcessMcpSession.CreateClientOptionsWithRoots(() => [_temp.DirectoryPath])); + await using var service = new WorkspaceRootService(new PhysicalFileSystem()); - var act = async () => await service.TryResolveAsync(fileName, null!, CancellationToken.None); + var act = async () => await service.TryResolveAsync(fileName, session.Server, CancellationToken.None); await act.Should().ThrowAsync(); } @@ -240,16 +249,6 @@ public async Task DisposeAsync_ShouldReleaseSemaphore_WithoutThrowing() await act.Should().NotThrowAsync(); } - private static void SetRoots(WorkspaceRootService service, IEnumerable roots) - { - var type = typeof(WorkspaceRootService); - var rootPathsField = type.GetField("_rootPaths", BindingFlags.NonPublic | BindingFlags.Instance)!; - var statusField = type.GetField("_status", BindingFlags.NonPublic | BindingFlags.Instance)!; - rootPathsField.SetValue(service, roots.ToList()); - // RootsStatusKind.Ready = 2 (private enum inside WorkspaceRootService) - statusField.SetValue(service, Enum.ToObject(statusField.FieldType, 2)); - } - public void Dispose() { _temp.Dispose(); diff --git a/tests/ProjGraph.Tests.Integration.Mcp/McpToolRootsTests.cs b/tests/ProjGraph.Tests.Integration.Mcp/McpToolRootsTests.cs new file mode 100644 index 0000000..fd20f3d --- /dev/null +++ b/tests/ProjGraph.Tests.Integration.Mcp/McpToolRootsTests.cs @@ -0,0 +1,108 @@ +using ModelContextProtocol.Protocol; +using ModelContextProtocol.Server; +using ProjGraph.Tests.Integration.Mcp.Helpers; +using ProjGraph.Tests.Shared.Helpers; + +namespace ProjGraph.Tests.Integration.Mcp; + +/// +/// Drives the tools the way a real client does — an actual tools/call over the wire — so the +/// request-scoped is the one the SDK binds, not a root instance a test +/// handed in. This is what distinguishes protocol revision 2026-07-28 (client capabilities declared +/// per request in _meta, visible only on the request-scoped server) from the earlier +/// initialize handshake that WorkspaceRootServiceTests pins itself to. +/// +public sealed class McpToolRootsTests : IDisposable +{ + private readonly TestDirectory _temp = new(); + + /// + /// Builds server options exposing the real ProjGraph tools, so a tools/call reaches the + /// production code path including workspace-root resolution. + /// + private static McpServerOptions CreateServerOptionsWithTools() + { + var tools = McpTestHelper.CreateTools(); + var options = InProcessMcpSession.CreateServerOptions(); + + options.ToolCollection = + [ + McpServerTool.Create(tools.GetProjectGraphAsync), + McpServerTool.Create(tools.GetProjectStatsAsync) + ]; + + return options; + } + + [Fact] + public async Task CallTool_RelativePath_OnCurrentProtocol_ShouldResolveAgainstTheClientRoots() + { + // A minimal but real solution file, so resolution is the only thing under test. + _temp.CreateFile("App.slnx", ""); + + await using var session = await InProcessMcpSession.StartAsync( + CreateServerOptionsWithTools(), + // Not pinned: the client negotiates the latest revision (2026-07-28), which drops the + // initialize handshake. This is the case that silently regressed on the v2 upgrade. + InProcessMcpSession.CreateClientOptionsWithRoots(() => [_temp.DirectoryPath], pinDownLevel: false)); + + var result = await session.Client.CallToolAsync( + "get_project_graph", + new Dictionary { ["path"] = "App.slnx" }); + + result.IsError.Should().NotBeTrue( + "a relative path must resolve against the client's workspace roots on the current protocol revision"); + } + + [Fact] + public async Task CallTool_RelativePath_WithoutRootsCapability_ShouldAskForAnAbsolutePath() + { + await using var session = await InProcessMcpSession.StartAsync( + CreateServerOptionsWithTools(), + InProcessMcpSession.CreateClientOptionsWithoutRoots()); + + var result = await session.Client.CallToolAsync( + "get_project_graph", + new Dictionary { ["path"] = "App.slnx" }); + + result.IsError.Should().BeTrue(); + var text = string.Concat(result.Content.OfType().Select(block => block.Text)); + text.Should().Contain("absolute path", + "the guidance must survive the SDK's tool boundary instead of being replaced by a generic error"); + } + + [Fact] + public async Task CallTool_OnCurrentProtocol_ShouldNotServeStaleRootsAcrossRequests() + { + using var secondRoot = new TestDirectory(); + secondRoot.CreateFile("Moved.slnx", ""); + _temp.CreateFile("App.slnx", ""); + + var currentRoots = new List { _temp.DirectoryPath }; + await using var session = await InProcessMcpSession.StartAsync( + CreateServerOptionsWithTools(), + InProcessMcpSession.CreateClientOptionsWithRoots( + () => [.. Volatile.Read(ref currentRoots)], pinDownLevel: false)); + + var first = await session.Client.CallToolAsync( + "get_project_graph", + new Dictionary { ["path"] = "App.slnx" }); + first.IsError.Should().NotBeTrue(); + + // The workspace moves. On 2026-07-28 there is no session for roots/list_changed to + // invalidate, so the roots must be re-fetched per request rather than cached. + Volatile.Write(ref currentRoots, [secondRoot.DirectoryPath]); + + var second = await session.Client.CallToolAsync( + "get_project_graph", + new Dictionary { ["path"] = "Moved.slnx" }); + + second.IsError.Should().NotBeTrue( + "the roots of the current request must be used, not those cached from an earlier one"); + } + + public void Dispose() + { + _temp.Dispose(); + } +} diff --git a/tests/ProjGraph.Tests.Integration.Mcp/WorkspaceRootServiceTests.cs b/tests/ProjGraph.Tests.Integration.Mcp/WorkspaceRootServiceTests.cs index f90d8ea..321ade6 100644 --- a/tests/ProjGraph.Tests.Integration.Mcp/WorkspaceRootServiceTests.cs +++ b/tests/ProjGraph.Tests.Integration.Mcp/WorkspaceRootServiceTests.cs @@ -129,7 +129,9 @@ public async Task RootsListChangedNotification_ShouldInvalidateTheCachedRoots() // The workspace switches to a different folder and the client announces it. Volatile.Write(ref currentRoots, [secondRoot.DirectoryPath]); +#pragma warning disable MCP9005 // Roots is deprecated (SEP-2577); still served for down-level clients. await session.Client.SendNotificationAsync(NotificationMethods.RootsListChangedNotification); +#pragma warning restore MCP9005 await WaitForRootsInvalidationAsync(service); var resolved = await service.TryResolveAsync("Moved.slnx", session.Server, CancellationToken.None); From d784fad11e45af02597f81bab4bc7eab70bbd9a6 Mon Sep 17 00:00:00 2001 From: = Date: Fri, 7 Aug 2026 00:03:11 +0200 Subject: [PATCH 2/2] fix(mcp): keep per-request roots out of the shared cache Addresses Copilot's review on #180. On the per-request revision the roots belong to the request that fetched them, but they were still written to the singleton's _rootPaths under the init lock and read back after the lock was released. Two overlapping tool calls could therefore interleave so that one resolved its path against the other's workspace roots. Roots resolution now returns the list for the current request instead of publishing it: the per-request revision fetches into a local and touches no shared field, while the initialize-handshake revisions keep the cache and its roots/list_changed invalidation. RefreshRootsAsync splits into a pure TryFetchRootsAsync, so the caller decides whether the result is cached. Co-Authored-By: Claude Opus 5 (1M context) --- src/ProjGraph.Mcp/WorkspaceRootService.cs | 102 +++++++++++------- .../Helpers/McpTestHelper.cs | 7 +- .../McpToolRootsTests.cs | 41 ++++++- 3 files changed, 110 insertions(+), 40 deletions(-) diff --git a/src/ProjGraph.Mcp/WorkspaceRootService.cs b/src/ProjGraph.Mcp/WorkspaceRootService.cs index 4d9687d..47c835c 100644 --- a/src/ProjGraph.Mcp/WorkspaceRootService.cs +++ b/src/ProjGraph.Mcp/WorkspaceRootService.cs @@ -47,18 +47,20 @@ public async Task TryResolveAsync(string path, McpServer server, Cancell return path; } - await EnsureInitializedAsync(server, ct); + // Resolved into a local: on the per-request revision the roots belong to this request only, + // so an overlapping request must not be able to swap them out from under this one. + var roots = await ResolveRootsAsync(server, ct); // Every failure below throws McpException: the SDK replaces the message of any other // exception type with a generic "An error occurred invoking '…'", so the guidance // (most importantly "provide an absolute path") would never reach the client. - if (_status == RootsStatusKind.Unsupported) + if (roots is null) { throw new McpException( "Client does not support workspace roots. Please provide an absolute path."); } - var matches = ResolveMatches(_rootPaths, path); + var matches = ResolveMatches(roots, path); return matches.Count switch { @@ -174,19 +176,43 @@ private void EnsureRootsChangedHandler(McpServer server) _notificationHandlerRegistered = true; } - internal async Task RefreshRootsAsync(McpServer server, CancellationToken ct) + /// + /// Fetches the client's workspace roots over roots/list. + /// + /// The request-scoped server handling the current request. + /// A token to cancel the request. + /// + /// The root directories, or when the client refuses the request. + /// + private static async Task?> TryFetchRootsAsync(McpServer server, CancellationToken ct) { + try + { #pragma warning disable MCP9005 // Roots is deprecated (SEP-2577); still served for down-level clients. See the file header. - var result = await server.RequestRootsAsync(new ListRootsRequestParams(), ct); + var result = await server.RequestRootsAsync(new ListRootsRequestParams(), ct); #pragma warning restore MCP9005 - var paths = new List(); - foreach (var root in result.Roots) + var paths = new List(); + foreach (var root in result.Roots) + { + paths.Add(new Uri(root.Uri).LocalPath); + } + + return paths; + } + catch (Exception ex) when (ex is not OperationCanceledException) { - paths.Add(new Uri(root.Uri).LocalPath); + // A client can advertise the capability and still refuse the request — most likely once + // it drops the deprecated feature. Reporting it as unsupported gets the caller the + // actionable "provide an absolute path" guidance instead of a generic SDK error. + return null; } + } - _rootPaths = paths; - _status = RootsStatusKind.Ready; + private static bool HasRootsCapability(McpServer server) + { +#pragma warning disable MCP9005 // Roots is deprecated (SEP-2577); still served for down-level clients. See the file header. + return server.ClientCapabilities?.Roots is not null; +#pragma warning restore MCP9005 } /// @@ -206,25 +232,33 @@ private static bool UsesSessionScopedCapabilities(McpServer server) return version is not null && string.CompareOrdinal(version, "2026-07-28") < 0; } - private async Task EnsureInitializedAsync(McpServer server, CancellationToken ct) + /// + /// Produces the workspace roots the current request must resolve against. + /// + /// The request-scoped server handling the current request. + /// A token to cancel the request. + /// + /// The root directories, or when the client offers none. + /// + private async Task?> ResolveRootsAsync(McpServer server, CancellationToken ct) { - var sessionScoped = UsesSessionScopedCapabilities(server); + if (!UsesSessionScopedCapabilities(server)) + { + // The per-request revision keeps nothing: the roots are scoped to this request, so + // publishing them to the shared cache would let an overlapping request resolve against + // the wrong workspace, and there is no session for roots/list_changed to invalidate. + return HasRootsCapability(server) ? await TryFetchRootsAsync(server, ct) : null; + } - if (sessionScoped && _status == RootsStatusKind.Ready) + if (_status == RootsStatusKind.Ready) { - return; + return _rootPaths; } - // "Unsupported" is deliberately never served from the cache: from protocol revision - // 2026-07-28 the client declares its capabilities per request in _meta, so a request that - // arrives without roots says nothing about the next one. Re-reading costs nothing — it is a - // property, not a round trip. -#pragma warning disable MCP9005 // Roots is deprecated (SEP-2577); still served for down-level clients. See the file header. - if (server.ClientCapabilities?.Roots is null) -#pragma warning restore MCP9005 + if (!HasRootsCapability(server)) { _status = RootsStatusKind.Unsupported; - return; + return null; } await _initLock.WaitAsync(ct); @@ -232,28 +266,24 @@ private async Task EnsureInitializedAsync(McpServer server, CancellationToken ct { // Double-checked locking: re-check after acquiring lock #pragma warning disable CA1508 // Avoid dead conditional code — volatile field may change between outer check and lock acquisition - if (sessionScoped && _status == RootsStatusKind.Ready) + if (_status == RootsStatusKind.Ready) { - return; + return _rootPaths; } #pragma warning restore CA1508 - if (sessionScoped) - { - EnsureRootsChangedHandler(server); - } + EnsureRootsChangedHandler(server); - try + var paths = await TryFetchRootsAsync(server, ct); + if (paths is null) { - await RefreshRootsAsync(server, ct); - } - catch (Exception ex) when (ex is not OperationCanceledException) - { - // A client can advertise the capability and still refuse the request — most likely - // once it drops the deprecated feature. Degrade to Unsupported so the caller gets - // the actionable "provide an absolute path" guidance instead of a generic SDK error. _status = RootsStatusKind.Unsupported; + return null; } + + _rootPaths = paths; + _status = RootsStatusKind.Ready; + return paths; } finally { diff --git a/tests/ProjGraph.Tests.Integration.Mcp/Helpers/McpTestHelper.cs b/tests/ProjGraph.Tests.Integration.Mcp/Helpers/McpTestHelper.cs index f143f0b..4d313e0 100644 --- a/tests/ProjGraph.Tests.Integration.Mcp/Helpers/McpTestHelper.cs +++ b/tests/ProjGraph.Tests.Integration.Mcp/Helpers/McpTestHelper.cs @@ -24,7 +24,10 @@ public static ProjGraphTools CreateTools() return CreateTools(new CollectingOutputConsole()); } - public static ProjGraphTools CreateTools(CollectingOutputConsole console, DiagramResourceCache? cache = null) + public static ProjGraphTools CreateTools( + CollectingOutputConsole console, + DiagramResourceCache? cache = null, + WorkspaceRootService? rootService = null) { var fs = new PhysicalFileSystem(); var slnParser = new SlnParser(fs); @@ -59,7 +62,7 @@ public static ProjGraphTools CreateTools(CollectingOutputConsole console, Diagra new MermaidErdRenderer()), fs, cache ?? new DiagramResourceCache(), - new WorkspaceRootService(fs), + rootService ?? new WorkspaceRootService(fs), console); } } diff --git a/tests/ProjGraph.Tests.Integration.Mcp/McpToolRootsTests.cs b/tests/ProjGraph.Tests.Integration.Mcp/McpToolRootsTests.cs index fd20f3d..6a9cd1d 100644 --- a/tests/ProjGraph.Tests.Integration.Mcp/McpToolRootsTests.cs +++ b/tests/ProjGraph.Tests.Integration.Mcp/McpToolRootsTests.cs @@ -1,7 +1,10 @@ using ModelContextProtocol.Protocol; using ModelContextProtocol.Server; +using ProjGraph.Lib.Core.Infrastructure; +using ProjGraph.Mcp; using ProjGraph.Tests.Integration.Mcp.Helpers; using ProjGraph.Tests.Shared.Helpers; +using System.Reflection; namespace ProjGraph.Tests.Integration.Mcp; @@ -20,9 +23,13 @@ public sealed class McpToolRootsTests : IDisposable /// Builds server options exposing the real ProjGraph tools, so a tools/call reaches the /// production code path including workspace-root resolution. /// - private static McpServerOptions CreateServerOptionsWithTools() + /// + /// The roots service to wire in, when a test needs to inspect it afterwards. + /// + /// Server options whose tool collection is backed by the real tools. + private static McpServerOptions CreateServerOptionsWithTools(WorkspaceRootService? rootService = null) { - var tools = McpTestHelper.CreateTools(); + var tools = McpTestHelper.CreateTools(new CollectingOutputConsole(), rootService: rootService); var options = InProcessMcpSession.CreateServerOptions(); options.ToolCollection = @@ -101,6 +108,36 @@ public async Task CallTool_OnCurrentProtocol_ShouldNotServeStaleRootsAcrossReque "the roots of the current request must be used, not those cached from an earlier one"); } + [Fact] + public async Task CallTool_OnCurrentProtocol_ShouldNotPublishTheRootsToTheSharedCache() + { + _temp.CreateFile("App.slnx", ""); + + await using var rootService = new WorkspaceRootService(new PhysicalFileSystem()); + await using var session = await InProcessMcpSession.StartAsync( + CreateServerOptionsWithTools(rootService), + InProcessMcpSession.CreateClientOptionsWithRoots(() => [_temp.DirectoryPath], pinDownLevel: false)); + + var result = await session.Client.CallToolAsync( + "get_project_graph", + new Dictionary { ["path"] = "App.slnx" }); + result.IsError.Should().NotBeTrue(); + + // The roots of a per-request revision belong to the request that fetched them. Leaving them + // in the singleton's fields is what would let an overlapping request resolve its own path + // against them, so the shared cache must still be untouched. + var type = typeof(WorkspaceRootService); + var rootPaths = type.GetField("_rootPaths", BindingFlags.NonPublic | BindingFlags.Instance)! + .GetValue(rootService); + var status = type.GetField("_status", BindingFlags.NonPublic | BindingFlags.Instance)! + .GetValue(rootService); + + rootPaths.Should().BeAssignableTo>() + .Which.Should().BeEmpty("the request's roots must not be published to the shared cache"); + // RootsStatusKind.Unknown = 0 (private enum inside WorkspaceRootService) + ((int)status!).Should().Be(0, "the shared status must stay untouched on the per-request revision"); + } + public void Dispose() { _temp.Dispose();