diff --git a/docs/superpowers/plans/2026-07-16-mcp-error-surfacing.md b/docs/superpowers/plans/2026-07-16-mcp-error-surfacing.md new file mode 100644 index 0000000..8223209 --- /dev/null +++ b/docs/superpowers/plans/2026-07-16-mcp-error-surfacing.md @@ -0,0 +1,62 @@ +# Plan — MCP error surfacing + +Spec: docs/superpowers/specs/2026-07-16-mcp-error-surfacing-design.md +Branch: revamp/mcp-error-surfacing (off develop @ 7f4edc6) + +## Global constraints + +- TDD per task: failing test first, then the fix, then full suite. +- Prefer `dtk` over raw `dotnet`. Full suite: `dtk test ProjGraph.slnx`. Format gate: + `dotnet format ProjGraph.slnx --verify-no-changes` (CI enforces it). +- No EF golden may move — nothing here touches rendering of entities. +- MCP stdout invariant: no code path may write to stdout outside JSON-RPC. + +## Task A — e2e transport harness + the High's red/green pin + +1. Add `` to + `tests/ProjGraph.Tests.Integration.Mcp/ProjGraph.Tests.Integration.Mcp.csproj` (version is + centrally pinned at 1.4.1 in Directory.Packages.props). +2. New `Helpers/McpServerFixture.cs` (or per-test helper): create `McpClient` over + `StdioClientTransport` running `dotnet ProjGraph.Mcp.dll` from `AppContext.BaseDirectory` + (ProjectReference copies the exe + runtimeconfig there; verify — if runtimeconfig is + missing, fall back to the source bin path via `TestPathHelper`). +3. New `McpTransportTests.cs`: + - `ListTools_OverStdio_ReturnsAllFourTools`. + - `GetErd_RelativePath_NoRootsCapability_ErrorMentionsAbsolutePath`: call `get_erd` with a + relative path; assert error result text contains "provide an absolute path". +4. RED: second test fails with the generic stripped message. Commit test-first only if the + suite stays runnable; otherwise commit together with Task B's fix noting the observed RED. + +## Task B — `McpException` conversions (makes A green) + +1. `WorkspaceRootService`: four throw sites → `McpException`, messages unchanged. +2. `ProjGraphTools`: add `RunAnalysisAsync` wrapping `ProjGraphException`; route every + `analysisServices.*` / `EfService.Discover*` call through it. +3. Update `McpRootsTests` BCL-type assertions → `McpException` (message assertions unchanged). +4. Unit-level pin: hand-wired `GetErdAsync` on a .cs file with no DbContext now surfaces + `McpException` "DbContext not found in file" (was `AnalysisException`). +5. Full suite + confirm Task A's pin is green over the real transport. + +## Task C — multi-DbContext candidates + snapshot name validation + +1. Tests (hand-wired, McpErdTests): + - two DbContexts, no contextName → `McpException` listing both names. + - two DbContexts, contextName = second → analyzes it (diagram contains its entity). + - contextName typo (context file) → `McpException` with candidates. + - snapshot file, contextName typo → `McpException` with candidates (was stripped generic). + - zero DbContexts covered by Task B's pin. +2. Implement per spec §3 in `GetErdAsync` only — `DbContextIdentifier` stays untouched (the + CLI's interactive prompt path also uses it). + +## Task D — class-diagram warning placement + +1. Test (hand-wired, directory with >50 .cs files via `TestDirectory`): output starts with + front-matter/diagram, warning appears as a trailing `%% WARNING:` line; cached resource + matches the returned string. +2. Reuse `AppendWarningComments`; delete the prepend. + +## Wrap-up + +- `dotnet format` clean, full suite green, no golden drift. +- Update CLAUDE.md only if behavior documented there changed (it isn't). +- PR to develop titled `fix(mcp): surface actionable errors through the MCP boundary`. diff --git a/docs/superpowers/specs/2026-07-16-mcp-error-surfacing-design.md b/docs/superpowers/specs/2026-07-16-mcp-error-surfacing-design.md new file mode 100644 index 0000000..080aea6 --- /dev/null +++ b/docs/superpowers/specs/2026-07-16-mcp-error-surfacing-design.md @@ -0,0 +1,105 @@ +# MCP error surfacing — McpException conversions, warning placement, multi-DbContext + +Date: 2026-07-16 +Status: Approved (design) +Program: EF rewrite / Phase 2 queue item 3 (audit §6) + +## Problem + +ModelContextProtocol 1.4.1 (verified against the shipped DLL during the 2026-07-14 audit) +replaces the message of every non-`McpException` thrown by a tool with the generic +"An error occurred invoking '…'". Four defects hide behind that behavior: + +1. **[High residual, audit §3 #4]** `WorkspaceRootService` throws BCL exceptions + (`InvalidOperationException` roots-unsupported, `FileNotFoundException` not-found, + `AmbiguousMatchException` multi-root, `ArgumentException` wildcard). The exact scenario + that made the original finding High — relative path + roots-unsupported client — produces + guidance ("Client does not support workspace roots. Please provide an absolute path.") + that **never reaches the client**. Library exceptions (`AnalysisException`, + `ParsingException`) thrown during analysis are likewise stripped: "DbContext not found in + file" becomes the generic message. +2. **[Medium, audit §4.4 #5 residual]** `get_class_diagram` *prepends* its >50-files warning + ahead of the YAML front-matter (`ProjGraphTools.GetClassDiagramAsync`), which strict Mermaid + parsers reject; `get_project_graph` already appends. The cached resource stores the same + prepended form. +3. **[Medium, audit §4.4 #6 residual]** `get_erd` on a file with multiple DbContexts silently + analyzes the first (`DbContextIdentifier.FindContextClass` → `FirstOrDefault`). The snapshot + branch already errors with the candidate list; the DbContext branch does not. +4. **[Low, audit §5.4]** The snapshot branch does not validate a caller-supplied `contextName` + against the discovered snapshot list — a typo falls through to `AnalyzeSnapshotAsync` and + surfaces as a stripped generic error even though the code already holds the candidate list. + +A fifth, structural gap keeps all of these invisible: the MCP "integration" suite hand-wires +`ProjGraphTools` with a `null!` server (audit §4.7 #18), so no test ever crosses the real SDK +boundary where the stripping happens. + +## Design + +### 1. `WorkspaceRootService` → `McpException` + +All four throw sites in `TryResolveAsync`/`ResolveMatches` become `McpException` with the same +messages (the not-found message additionally gains "Provide an absolute path.", matching its +ambiguous-match sibling). The service lives in `ProjGraph.Mcp`, so the dependency already exists. The existing +`McpRootsTests` assertions on BCL exception types are updated to `McpException` — that is the +point of the change, not collateral damage. + +### 2. Tool-boundary wrap for library exceptions + +`ProjGraphTools` gains one private helper: + +```csharp +private static async Task RunAnalysisAsync(Func> analysis) +{ + try { return await analysis(); } + catch (ProjGraphException ex) { throw new McpException(ex.Message); } +} +``` + +Every call into `AnalysisServices` (graph, stats, class file/directory, EF context/snapshot, +snapshot/context discovery) goes through it. `ProjGraphException` is the library's base type, +so `AnalysisException`/`ParsingException` are both covered; unexpected BCL exceptions still +propagate unwrapped (they carry no user guidance worth preserving, and masking genuine bugs +as protocol errors would hide stack traces from the server log). + +### 3. `get_erd` DbContext branch mirrors the snapshot branch + +Before analyzing, the DbContext branch calls `EfService.DiscoverContextsAsync(path)`: + +- 0 contexts → `McpException` "No DbContext found in '{path}'." +- `contextName` supplied but not in the list → `McpException` naming it and listing candidates. +- no `contextName` and ≥2 → `McpException` listing candidates: "Specify one using the + contextName parameter." +- otherwise analyze the single/named context. + +The snapshot branch gains the same supplied-name validation against the discovered list. +Behavior change: multiple contexts without `contextName` was silent-first, now an error — +deliberate; MCP callers are typically LLMs that need the signal (same rationale recorded for +`ownedMode` validation in PR #161). + +### 4. `get_class_diagram` warning appended + +The >50-files warning is collected as a plain string and run through the existing +`AppendWarningComments` (which trims and appends `%% WARNING:` lines after the diagram body), +identical to `get_project_graph`. The cached resource stores the appended form. Message text +unchanged ("Scanning {N} files. Large diagrams may be hard to read."). + +### 5. One true end-to-end transport test + +`Tests.Integration.Mcp` gains the `ModelContextProtocol` package and a test fixture that +launches the real server (`ProjGraph.Mcp.dll` from the test output, where the ProjectReference +copies it) over `StdioClientTransport` via `McpClient`. Minimum assertions: + +- `ListToolsAsync` returns the four tools (transport + DI wiring smoke). +- `get_erd` with a **relative path** from a client **without roots capability** returns an + error result whose text contains "provide an absolute path" — the exact High scenario. + This test FAILS before fix 1 (message stripped to "An error occurred invoking…") and passes + after: it is the red/green pin proving the conversion matters at the protocol level, which + no hand-wired test can do. + +## Out of scope + +- Absolute-path root confinement (audit §4.4 #8, accepted risk). +- `WorkspaceRootService` negative caching for bare-name scans (audit §5.4 L, perf not errors). +- `CollectingOutputConsole.PromptSelectionAsync` booby trap (audit §5.4 L; unreachable today). +- CLI parity for multi-context (CLI already prompts interactively). +- CancellationToken plumbing (Phase 2 themed PR). diff --git a/src/ProjGraph.Mcp/ProjGraphTools.cs b/src/ProjGraph.Mcp/ProjGraphTools.cs index c2da2f1..7f0249f 100644 --- a/src/ProjGraph.Mcp/ProjGraphTools.cs +++ b/src/ProjGraph.Mcp/ProjGraphTools.cs @@ -1,5 +1,6 @@ using ModelContextProtocol; using ModelContextProtocol.Server; +using ProjGraph.Core.Exceptions; using ProjGraph.Core.Models; using ProjGraph.Lib.ClassDiagram.Application; using ProjGraph.Lib.ClassDiagram.Application.UseCases; @@ -68,14 +69,14 @@ public async Task GetClassDiagramAsync( }); ClassModel model; - var warningMarkup = string.Empty; + var warnings = new List(); if (fileSystem.DirectoryExists(path)) { var files = discoverCsFilesUseCase.Execute(path); if (files.Count > 50) { - warningMarkup = $"%% WARNING: Scanning {files.Count} files. Large diagrams may be hard to read.\n"; + warnings.Add($"Scanning {files.Count} files. Large diagrams may be hard to read."); } progress?.Report(new ProgressNotificationValue @@ -85,7 +86,7 @@ public async Task GetClassDiagramAsync( Message = "Analyzing types and members" }); - model = await analysisServices.ClassService.AnalyzeDirectoryAsync(path, options); + model = await RunAnalysisAsync(() => analysisServices.ClassService.AnalyzeDirectoryAsync(path, options)); } else { @@ -98,7 +99,7 @@ public async Task GetClassDiagramAsync( Message = "Analyzing types and members" }); - model = await analysisServices.ClassService.AnalyzeFileAsync(path, options); + model = await RunAnalysisAsync(() => analysisServices.ClassService.AnalyzeFileAsync(path, options)); } progress?.Report(new ProgressNotificationValue @@ -108,8 +109,11 @@ public async Task GetClassDiagramAsync( Message = "Rendering class diagram" }); - var diagram = renderers.ClassRenderer.Render(model, new DiagramOptions(showTitle, false)); - var result = warningMarkup + diagram; + // Appended, not prepended: a comment ahead of the YAML front-matter breaks strict + // Mermaid parsers (same placement rule as get_project_graph's warnings). + var result = AppendWarningComments( + renderers.ClassRenderer.Render(model, new DiagramOptions(showTitle, false)), + warnings); var filename = Path.GetFileName(path); await cache.StoreAsync("class", path, "text/plain", result, @@ -151,7 +155,8 @@ public async Task GetProjectGraphAsync( }); outputConsole.ClearWarnings(); - var graph = await analysisServices.GraphService.BuildGraphAsync(path, includePackages, cancellationToken); + var graph = await RunAnalysisAsync(() => + analysisServices.GraphService.BuildGraphAsync(path, includePackages, cancellationToken)); var warnings = outputConsole.DrainWarnings(); progress?.Report(new ProgressNotificationValue @@ -209,7 +214,8 @@ public async Task GetProjectStatsAsync( }); outputConsole.ClearWarnings(); - var stats = await analysisServices.StatsService.ComputeStatsAsync(path, topN, cancellationToken); + var stats = await RunAnalysisAsync(() => + analysisServices.StatsService.ComputeStatsAsync(path, topN, cancellationToken)); var warnings = outputConsole.DrainWarnings(); progress?.Report(new ProgressNotificationValue @@ -270,17 +276,8 @@ public async Task GetErdAsync( if (path.EndsWith($"ModelSnapshot{FilePathGuard.CSharpExtension}", StringComparison.OrdinalIgnoreCase)) { - var snapshots = await analysisServices.EfService.DiscoverSnapshotsAsync(path); - - var snapshotName = !string.IsNullOrEmpty(contextName) - ? contextName - : snapshots.Count switch - { - 0 => throw new McpException($"No ModelSnapshot found in '{path}'."), - 1 => snapshots[0], - _ => throw new McpException( - $"Multiple ModelSnapshots found in '{path}': {string.Join(", ", snapshots)}. Specify one using the contextName parameter.") - }; + var snapshots = await RunAnalysisAsync(() => analysisServices.EfService.DiscoverSnapshotsAsync(path)); + var snapshotName = ResolveCandidateName(snapshots, contextName, "ModelSnapshot", path); progress?.Report(new ProgressNotificationValue { @@ -289,10 +286,16 @@ public async Task GetErdAsync( Message = "Analyzing entities and relationships" }); - model = await analysisServices.EfService.AnalyzeSnapshotAsync(path, snapshotName); + model = await RunAnalysisAsync(() => analysisServices.EfService.AnalyzeSnapshotAsync(path, snapshotName)); } else { + // Mirror the snapshot branch: with several DbContexts in the file, silently analyzing + // the first (the old FindContextClass FirstOrDefault behavior) hands an MCP caller + // plausible-but-wrong output with no signal that the others were never considered. + var contexts = await RunAnalysisAsync(() => analysisServices.EfService.DiscoverContextsAsync(path)); + var resolvedName = ResolveCandidateName(contexts, contextName, "DbContext", path); + progress?.Report(new ProgressNotificationValue { Progress = 2, @@ -300,7 +303,7 @@ public async Task GetErdAsync( Message = "Analyzing entities and relationships" }); - model = await analysisServices.EfService.AnalyzeContextAsync(path, contextName); + model = await RunAnalysisAsync(() => analysisServices.EfService.AnalyzeContextAsync(path, resolvedName)); } progress?.Report(new ProgressNotificationValue @@ -373,10 +376,77 @@ private static string SerializeStatsWithWarnings(SolutionStats stats, IReadOnlyL return node.ToJsonString(JsonSerializerOptions); } + /// + /// Resolves which discovered DbContext/ModelSnapshot class to analyze, failing with an + /// actionable instead of silently picking a wrong or missing one: + /// a requested name is validated against the discovered candidates (a typo previously fell + /// through to analysis and surfaced as a stripped generic error), and with several candidates + /// and no requested name the caller is asked to choose rather than being handed the first. + /// + /// The class names discovered in the file. + /// The caller-supplied class name, if any. + /// The kind of class being resolved ("DbContext" or "ModelSnapshot"), for messages. + /// The analyzed file path, for messages. + /// The single resolved class name. + /// Thrown when the requested name is unknown, none exist, or the choice is ambiguous. + private static string ResolveCandidateName( + List candidates, string? requestedName, string kind, string path) + { + if (candidates.Count == 0) + { + throw new McpException($"No {kind} found in '{path}'."); + } + + if (!string.IsNullOrEmpty(requestedName)) + { + return candidates.Contains(requestedName) + ? requestedName + : throw new McpException( + $"{kind} '{requestedName}' not found in '{path}'. Available: {string.Join(", ", candidates)}."); + } + + return candidates.Count == 1 + ? candidates[0] + : throw new McpException( + $"Multiple {kind}s found in '{path}': {string.Join(", ", candidates)}. Specify one using the contextName parameter."); + } + + /// + /// Runs a library analysis call, converting any (the base of + /// AnalysisException/ParsingException) into an carrying + /// the same message. The MCP SDK replaces the message of every other exception type with a + /// generic "An error occurred invoking '…'", so without this wrap the library's actionable + /// guidance (e.g. "DbContext not found in file") never reaches the client. Unexpected BCL + /// exceptions still propagate unwrapped: they carry no user guidance worth preserving, and + /// wrapping them would dress genuine bugs up as clean protocol errors. + /// + /// The analysis result type. + /// The analysis call to run. + /// The analysis result. + /// Thrown when the analysis fails with a . + private static async Task RunAnalysisAsync(Func> analysis) + { + try + { + return await analysis(); + } + catch (ProjGraphException ex) + { + // Inner exception preserved so the original type and stack trace stay in server logs. + throw new McpException(ex.Message, ex); + } + } + private async Task PreparePathAsync(string path, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); - ArgumentException.ThrowIfNullOrWhiteSpace(path); + if (string.IsNullOrWhiteSpace(path)) + { + // McpException so the guidance reaches the client; the SDK strips the message from + // any other exception type. + throw new McpException("path must not be empty."); + } + return await rootService.TryResolveAsync(path, server, cancellationToken); } diff --git a/src/ProjGraph.Mcp/WorkspaceRootService.cs b/src/ProjGraph.Mcp/WorkspaceRootService.cs index 53cdc99..696c1ad 100644 --- a/src/ProjGraph.Mcp/WorkspaceRootService.cs +++ b/src/ProjGraph.Mcp/WorkspaceRootService.cs @@ -1,8 +1,8 @@ +using ModelContextProtocol; using ModelContextProtocol.Protocol; using ModelContextProtocol.Server; using ProjGraph.Lib.Core.Abstractions; using ProjGraph.Lib.Core.Infrastructure; -using System.Reflection; namespace ProjGraph.Mcp; @@ -30,9 +30,12 @@ public async Task TryResolveAsync(string path, McpServer server, Cancell await EnsureInitializedAsync(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) { - throw new InvalidOperationException( + throw new McpException( "Client does not support workspace roots. Please provide an absolute path."); } @@ -40,10 +43,10 @@ public async Task TryResolveAsync(string path, McpServer server, Cancell return matches.Count switch { - 0 => throw new FileNotFoundException( - $"'{path}' not found under any workspace root"), + 0 => throw new McpException( + $"'{path}' not found under any workspace root. Provide an absolute path."), 1 => matches[0], - _ => throw new AmbiguousMatchException( + _ => throw new McpException( $"'{path}' matches multiple roots: {string.Join(", ", matches)}. Provide an absolute path.") }; } @@ -57,16 +60,15 @@ public async Task TryResolveAsync(string path, McpServer server, Cancell /// The workspace root directories. /// The relative path to resolve. Must not contain wildcard characters. /// The distinct set of matching absolute paths across all roots. - /// Thrown when contains a wildcard. + /// Thrown when contains a wildcard. internal List ResolveMatches(IEnumerable rootPaths, string path) { // The input is a path, not a glob: reject wildcards so it cannot match an arbitrary file - // (e.g. "*.slnx") under a root. + // (e.g. "*.slnx") under a root. McpException so the guidance reaches the client. if (path.IndexOfAny(['*', '?']) >= 0) { - throw new ArgumentException( - $"Path '{path}' must not contain wildcard characters. Provide a specific relative path.", - nameof(path)); + throw new McpException( + $"Path '{path}' must not contain wildcard characters. Provide a specific relative path."); } return diff --git a/tests/ProjGraph.Tests.Integration.Mcp/Helpers/McpTestHelper.cs b/tests/ProjGraph.Tests.Integration.Mcp/Helpers/McpTestHelper.cs index f87b096..0739695 100644 --- a/tests/ProjGraph.Tests.Integration.Mcp/Helpers/McpTestHelper.cs +++ b/tests/ProjGraph.Tests.Integration.Mcp/Helpers/McpTestHelper.cs @@ -24,7 +24,7 @@ public static ProjGraphTools CreateTools() return CreateTools(new CollectingOutputConsole()); } - public static ProjGraphTools CreateTools(CollectingOutputConsole console) + public static ProjGraphTools CreateTools(CollectingOutputConsole console, DiagramResourceCache? cache = null) { var fs = new PhysicalFileSystem(); var slnParser = new SlnParser(fs); @@ -58,7 +58,7 @@ public static ProjGraphTools CreateTools(CollectingOutputConsole console) new DiagramRenderers(new MermaidGraphRenderer(), new MermaidClassDiagramRenderer(), new MermaidErdRenderer()), fs, - new DiagramResourceCache(), + cache ?? new DiagramResourceCache(), null!, new WorkspaceRootService(fs), console); diff --git a/tests/ProjGraph.Tests.Integration.Mcp/McpErdTests.cs b/tests/ProjGraph.Tests.Integration.Mcp/McpErdTests.cs index 9d91f6c..9141a44 100644 --- a/tests/ProjGraph.Tests.Integration.Mcp/McpErdTests.cs +++ b/tests/ProjGraph.Tests.Integration.Mcp/McpErdTests.cs @@ -203,7 +203,7 @@ public async Task GetErd_NonExistentFile_ShouldThrow() } [Fact] - public async Task GetErd_InvalidCsFile_ShouldThrow() + public async Task GetErd_InvalidCsFile_ShouldThrowMcpExceptionNamingDbContext() { // Arrange var tools = CreateTools(); @@ -213,8 +213,146 @@ public async Task GetErd_InvalidCsFile_ShouldThrow() // Act var act = async () => await tools.GetErdAsync(invalidFile); + // Assert - the library's "DbContext not found" guidance must cross the tool boundary as + // McpException; the SDK strips the message from any other exception type. + (await act.Should().ThrowAsync()).Which.Message.Should().Contain("DbContext"); + } + + private string WriteTwoContextFile() + { + var path = Path.Combine(_temp.DirectoryPath, "TwoContexts.cs"); + const string content = """ + using Microsoft.EntityFrameworkCore; + + namespace TestNamespace; + + public class Order + { + public int Id { get; set; } + public string Reference { get; set; } + } + + public class Customer + { + public int Id { get; set; } + public string FullName { get; set; } + } + + public class OrderContext : DbContext + { + public DbSet Orders { get; set; } + } + + public class CustomerContext : DbContext + { + public DbSet Customers { get; set; } + } + """; + File.WriteAllText(path, content); + return path; + } + + [Fact] + public async Task GetErd_MultipleDbContexts_NoContextName_ShouldThrowMcpExceptionListingCandidates() + { + // Arrange — silently analyzing the first context is the worst outcome for an LLM caller: + // plausible-but-wrong output with no signal that CustomerContext was never considered. + var tools = CreateTools(); + var path = WriteTwoContextFile(); + + // Act + var act = async () => await tools.GetErdAsync(path); + // Assert - await act.Should().ThrowAsync(); + var message = (await act.Should().ThrowAsync()).Which.Message; + message.Should().Contain("OrderContext"); + message.Should().Contain("CustomerContext"); + message.Should().Contain("contextName"); + } + + [Fact] + public async Task GetErd_MultipleDbContexts_WithContextName_AnalyzesTheNamedContext() + { + // Arrange + var tools = CreateTools(); + var path = WriteTwoContextFile(); + + // Act + var result = await tools.GetErdAsync(path, contextName: "CustomerContext"); + + // Assert + result.Should().Contain("Customer {"); + result.Should().NotContain("Order {"); + } + + [Fact] + public async Task GetErd_ContextNameTypo_ShouldThrowMcpExceptionListingCandidates() + { + // Arrange + var tools = CreateTools(); + var path = WriteTwoContextFile(); + + // Act + var act = async () => await tools.GetErdAsync(path, contextName: "OrderConetxt"); + + // Assert — the error must name the typo and offer the real candidates + var message = (await act.Should().ThrowAsync()).Which.Message; + message.Should().Contain("OrderConetxt"); + message.Should().Contain("OrderContext"); + message.Should().Contain("CustomerContext"); + } + + [Fact] + public async Task GetErd_SnapshotContextNameTypo_ShouldThrowMcpExceptionListingCandidates() + { + // Arrange — the snapshot branch already held the discovered candidate list but never + // validated a caller-supplied name against it: a typo fell through to analysis and + // surfaced as a stripped generic error. + var tools = CreateTools(); + var path = Path.Combine(_temp.DirectoryPath, "MyDbContextModelSnapshot.cs"); + const string content = """ + using Microsoft.EntityFrameworkCore; + using Microsoft.EntityFrameworkCore.Infrastructure; + + namespace TestNamespace; + + public class MyDbContextModelSnapshot : ModelSnapshot + { + protected override void BuildModel(ModelBuilder modelBuilder) + { + modelBuilder.Entity("TestNamespace.Blog", b => + { + b.Property("Id"); + b.HasKey("Id"); + b.ToTable("Blogs"); + }); + } + } + """; + await File.WriteAllTextAsync(path, content); + + // Act + var act = async () => await tools.GetErdAsync(path, contextName: "WrongSnapshot"); + + // Assert + var message = (await act.Should().ThrowAsync()).Which.Message; + message.Should().Contain("WrongSnapshot"); + message.Should().Contain("MyDbContextModelSnapshot"); + } + + [Fact] + public async Task GetErd_WhitespacePath_ShouldThrowMcpException() + { + // Arrange — a blank path previously hit ArgumentException.ThrowIfNullOrWhiteSpace, whose + // message the SDK strips to a generic error; the hottest parameter of every tool deserves + // an actionable failure. + var tools = CreateTools(); + + // Act + var act = async () => await tools.GetErdAsync(" "); + + // Assert + (await act.Should().ThrowAsync()).Which.Message.Should().Contain("path"); } [Fact] diff --git a/tests/ProjGraph.Tests.Integration.Mcp/McpIntegrationTests.cs b/tests/ProjGraph.Tests.Integration.Mcp/McpIntegrationTests.cs index 6e139f0..da0e71f 100644 --- a/tests/ProjGraph.Tests.Integration.Mcp/McpIntegrationTests.cs +++ b/tests/ProjGraph.Tests.Integration.Mcp/McpIntegrationTests.cs @@ -1,3 +1,4 @@ +using ModelContextProtocol; using ProjGraph.Mcp; using ProjGraph.Tests.Integration.Mcp.Helpers; @@ -9,33 +10,39 @@ public class McpIntegrationTests [InlineData(null)] [InlineData("")] [InlineData(" ")] - public async Task GetProjectGraph_NullOrEmptyPath_ShouldThrowArgumentException(string? path) + public async Task GetProjectGraph_NullOrEmptyPath_ShouldThrowMcpException(string? path) { var tools = CreateTools(); var act = async () => await tools.GetProjectGraphAsync(path!); - 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(); } [Theory] [InlineData(null)] [InlineData("")] [InlineData(" ")] - public async Task GetClassDiagram_NullOrEmptyPath_ShouldThrowArgumentException(string? path) + public async Task GetClassDiagram_NullOrEmptyPath_ShouldThrowMcpException(string? path) { var tools = CreateTools(); var act = async () => await tools.GetClassDiagramAsync(path!); - 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(); } [Theory] [InlineData(null)] [InlineData("")] [InlineData(" ")] - public async Task GetErd_NullOrEmptyPath_ShouldThrowArgumentException(string? path) + public async Task GetErd_NullOrEmptyPath_ShouldThrowMcpException(string? path) { var tools = CreateTools(); var act = async () => await tools.GetErdAsync(path!); - 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(); } private static ProjGraphTools CreateTools() diff --git a/tests/ProjGraph.Tests.Integration.Mcp/McpRootsTests.cs b/tests/ProjGraph.Tests.Integration.Mcp/McpRootsTests.cs index 452d1d3..1110d26 100644 --- a/tests/ProjGraph.Tests.Integration.Mcp/McpRootsTests.cs +++ b/tests/ProjGraph.Tests.Integration.Mcp/McpRootsTests.cs @@ -1,3 +1,4 @@ +using ModelContextProtocol; using ProjGraph.Lib.Core.Infrastructure; using ProjGraph.Mcp; using ProjGraph.Tests.Shared.Helpers; @@ -97,7 +98,8 @@ public void ResolveMatches_WildcardPattern_ShouldThrow() var act = () => service.ResolveMatches([_temp.DirectoryPath], "*.cs"); - act.Should().Throw(); + // McpException so the wildcard guidance reaches the client instead of a stripped generic error. + act.Should().Throw().WithMessage("*wildcard*"); } [Fact] @@ -171,19 +173,20 @@ public async Task TryResolve_RelativePath_FileFoundInSubdirectory_ShouldReturnFu } [Fact] - public async Task TryResolve_RelativePath_FileNotFound_ShouldThrowFileNotFoundException() + public async Task TryResolve_RelativePath_FileNotFound_ShouldThrowMcpException() { var service = new WorkspaceRootService(new PhysicalFileSystem()); SetRoots(service, [_temp.DirectoryPath]); var act = async () => await service.TryResolveAsync("missing.slnx", null!, CancellationToken.None); - await act.Should().ThrowAsync() + // McpException so the not-found guidance reaches the client instead of a stripped generic error. + await act.Should().ThrowAsync() .WithMessage("*missing.slnx*"); } [Fact] - public async Task TryResolve_RelativePath_AmbiguousMatch_ShouldThrowAmbiguousMatchException() + public async Task TryResolve_RelativePath_AmbiguousMatch_ShouldThrowMcpException() { const string fileName = "Shared.slnx"; _temp.CreateFile(fileName, ""); @@ -196,7 +199,8 @@ public async Task TryResolve_RelativePath_AmbiguousMatch_ShouldThrowAmbiguousMat var act = async () => await service.TryResolveAsync(fileName, null!, CancellationToken.None); - await act.Should().ThrowAsync() + // McpException so the ambiguity guidance reaches the client instead of a stripped generic error. + await act.Should().ThrowAsync() .WithMessage("*Shared.slnx*"); } @@ -211,7 +215,7 @@ public async Task TryResolve_RelativePath_FileInsideBinDirectory_ShouldNotBeFoun var act = async () => await service.TryResolveAsync(fileName, null!, CancellationToken.None); - await act.Should().ThrowAsync(); + await act.Should().ThrowAsync(); } [Fact] @@ -225,7 +229,7 @@ public async Task TryResolve_RelativePath_FileInsideObjDirectory_ShouldNotBeFoun var act = async () => await service.TryResolveAsync(fileName, null!, CancellationToken.None); - await act.Should().ThrowAsync(); + await act.Should().ThrowAsync(); } [Fact] diff --git a/tests/ProjGraph.Tests.Integration.Mcp/McpTransportTests.cs b/tests/ProjGraph.Tests.Integration.Mcp/McpTransportTests.cs new file mode 100644 index 0000000..f8b5b3b --- /dev/null +++ b/tests/ProjGraph.Tests.Integration.Mcp/McpTransportTests.cs @@ -0,0 +1,88 @@ +using ModelContextProtocol.Client; +using ModelContextProtocol.Protocol; +using ProjGraph.Tests.Shared.Helpers; + +namespace ProjGraph.Tests.Integration.Mcp; + +/// +/// True end-to-end tests that launch the real MCP server executable and talk to it over the +/// stdio transport with a real . Unlike the hand-wired tests (which +/// construct directly), these cross the SDK's +/// tool-invocation boundary — the layer that replaces the message of any non-McpException +/// with a generic "An error occurred invoking …", which no in-process test can observe. +/// +public sealed class McpTransportTests +{ + /// + /// Connects a client to the server apphost in ProjGraph.Mcp's own build output (guaranteed + /// up to date by the ProjectReference). ProjGraph.Mcp is a self-contained exe, so its build + /// lands in a RID subdirectory and must be launched via its apphost — the DLL that the + /// ProjectReference copies into the test output has no runtime next to it and cannot start. + /// The client deliberately advertises no capabilities — in particular no workspace roots. + /// + private static async Task ConnectAsync() + { + var transport = new StdioClientTransport(new StdioClientTransportOptions + { + Name = "ProjGraph e2e", + Command = LocateServerExecutable() + }); + + return await McpClient.CreateAsync(transport); + } + + private static string LocateServerExecutable() + { + // .../tests/ProjGraph.Tests.Integration.Mcp/bin/{Configuration}/{tfm}/ + var testOutput = new DirectoryInfo(AppContext.BaseDirectory.TrimEnd(Path.DirectorySeparatorChar)); + var binRoot = TestPathHelper.GetRootPath(Path.Combine( + "src", "ProjGraph.Mcp", "bin", testOutput.Parent!.Name, testOutput.Name)); + Directory.Exists(binRoot).Should().BeTrue( + $"the MCP server build output must exist at {binRoot}"); + var exeName = OperatingSystem.IsWindows() ? "ProjGraph.Mcp.exe" : "ProjGraph.Mcp"; + + // The build RID matches the machine that built it, so probing the RID subdirectories + // is exact enough without reconstructing the RID by hand. Preferring the most recently + // written apphost keeps a dev machine with stale cross-RID leftovers deterministic. + var serverExe = Directory.GetDirectories(binRoot) + .Select(ridDir => new FileInfo(Path.Combine(ridDir, exeName))) + .Where(apphost => apphost.Exists) + .OrderByDescending(apphost => apphost.LastWriteTimeUtc) + .FirstOrDefault(); + + serverExe.Should().NotBeNull($"the MCP server apphost must be present under {binRoot}"); + return serverExe!.FullName; + } + + private static string JoinText(CallToolResult result) + { + return string.Join("\n", result.Content.OfType().Select(block => block.Text)); + } + + [Fact] + public async Task ListTools_OverRealStdioTransport_ExposesAllFourTools() + { + await using var client = await ConnectAsync(); + + var tools = await client.ListToolsAsync(); + + tools.Select(tool => tool.Name).Should().BeEquivalentTo( + "get_class_diagram", "get_project_graph", "get_project_stats", "get_erd"); + } + + [Fact] + public async Task GetErd_RelativePathWithoutRootsCapability_SurfacesAbsolutePathGuidance() + { + await using var client = await ConnectAsync(); + + var result = await client.CallToolAsync( + "get_erd", + new Dictionary { ["path"] = "SomeDbContext.cs" }); + + result.IsError.Should().BeTrue("a relative path cannot be resolved without workspace roots"); + + // The audit's High scenario: WorkspaceRootService's guidance must survive the SDK + // boundary instead of being stripped to "An error occurred invoking 'get_erd'". + JoinText(result).Should().Contain("absolute path"); + } +} diff --git a/tests/ProjGraph.Tests.Integration.Mcp/McpWarningsTests.cs b/tests/ProjGraph.Tests.Integration.Mcp/McpWarningsTests.cs index 7354f06..535aef8 100644 --- a/tests/ProjGraph.Tests.Integration.Mcp/McpWarningsTests.cs +++ b/tests/ProjGraph.Tests.Integration.Mcp/McpWarningsTests.cs @@ -52,6 +52,34 @@ public async Task GetProjectGraph_HealthySolution_HasNoWarningComment() result.Should().NotContain("%% WARNING"); } + [Fact] + public async Task GetClassDiagram_ManyFiles_AppendsFileCountWarningAfterDiagram() + { + // Arrange — 51 files crosses the >50 threshold for the large-scan warning. + for (var i = 0; i < 51; i++) + { + _temp.CreateFile(Path.Combine("many", $"C{i}.cs"), $"namespace Many; public class C{i} {{ }}"); + } + + var cache = new DiagramResourceCache(); + var tools = McpTestHelper.CreateTools(new CollectingOutputConsole(), cache); + var dirPath = Path.Combine(_temp.DirectoryPath, "many"); + + // Act + var result = await tools.GetClassDiagramAsync(dirPath); + + // Assert — the warning must TRAIL the diagram like get_project_graph's warnings do: + // prepended ahead of the YAML front-matter, strict Mermaid parsers reject the diagram. + result.Should().Contain("%% WARNING: Scanning 51 files"); + result.TrimStart().Should().NotStartWith("%% WARNING"); + result.IndexOf("classDiagram", StringComparison.Ordinal).Should().BeLessThan( + result.IndexOf("%% WARNING", StringComparison.Ordinal)); + + // The cached resource must carry the same (appended) form as the returned diagram. + var cachedUri = $"projgraph://diagrams/class/{Uri.EscapeDataString(dirPath)}"; + cache.TryRead(cachedUri).Should().Be(result); + } + [Fact] public async Task CollectingOutputConsole_ScopesWarningsToAsyncFlow() { diff --git a/tests/ProjGraph.Tests.Integration.Mcp/ProjGraph.Tests.Integration.Mcp.csproj b/tests/ProjGraph.Tests.Integration.Mcp/ProjGraph.Tests.Integration.Mcp.csproj index e60b99d..b8d2ef7 100644 --- a/tests/ProjGraph.Tests.Integration.Mcp/ProjGraph.Tests.Integration.Mcp.csproj +++ b/tests/ProjGraph.Tests.Integration.Mcp/ProjGraph.Tests.Integration.Mcp.csproj @@ -7,6 +7,7 @@ +