fix(mcp): surface actionable errors through the MCP boundary

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 `<PackageReference Include="ModelContextProtocol"/>` 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<T>` 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`.

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<T> RunAnalysisAsync<T>(Func<Task<T>> 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).

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<string> GetClassDiagramAsync(
         });
 
         ClassModel model;
-        var warningMarkup = string.Empty;
+        var warnings = new List<string>();
 
         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<string> 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<string> 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<string> 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<string> 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<string> 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<string> 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,18 +286,24 @@ public async Task<string> 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,
                 Total = 3,
                 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);
     }
 
+    /// <summary>
+    /// Resolves which discovered DbContext/ModelSnapshot class to analyze, failing with an
+    /// actionable <see cref="McpException"/> 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.
+    /// </summary>
+    /// <param name="candidates">The class names discovered in the file.</param>
+    /// <param name="requestedName">The caller-supplied class name, if any.</param>
+    /// <param name="kind">The kind of class being resolved ("DbContext" or "ModelSnapshot"), for messages.</param>
+    /// <param name="path">The analyzed file path, for messages.</param>
+    /// <returns>The single resolved class name.</returns>
+    /// <exception cref="McpException">Thrown when the requested name is unknown, none exist, or the choice is ambiguous.</exception>
+    private static string ResolveCandidateName(
+        List<string> 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.");
+    }
+
+    /// <summary>
+    /// Runs a library analysis call, converting any <see cref="ProjGraphException"/> (the base of
+    /// <c>AnalysisException</c>/<c>ParsingException</c>) into an <see cref="McpException"/> 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.
+    /// </summary>
+    /// <typeparam name="T">The analysis result type.</typeparam>
+    /// <param name="analysis">The analysis call to run.</param>
+    /// <returns>The analysis result.</returns>
+    /// <exception cref="McpException">Thrown when the analysis fails with a <see cref="ProjGraphException"/>.</exception>
+    private static async Task<T> RunAnalysisAsync<T>(Func<Task<T>> 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<string> 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);
     }