Finish #456's descent: score, override, display, and share proc-body statements

server/PlanShare/Program.cs

@@ -105,6 +105,16 @@ created_at TEXT NOT NULL
 
 const int MaxTtlDays = 365;
 
+// Depth ceiling for parsing an uploaded share, mirroring PlanViewer.Core's
+// AnalysisJson.MaxDepth — that class is the source of truth for how deep a serialized
+// AnalysisResult can go (#431: an operator costs two JSON levels, so the JsonDocument
+// default of 64 rejects a plan ~30 operators deep as "Invalid JSON" after the client
+// happily serialized it at 1024). Mirrored rather than referenced because this project
+// deliberately takes no dependency on PlanViewer.Core, and a shared constant only helps
+// call sites that reference it; this one cannot. If AnalysisJson.MaxDepth ever changes,
+// change this with it — the client-side depth tests pin 1024, so start there.
+var shareDocumentOptions = new JsonDocumentOptions { MaxDepth = 1024 };
+
 // --- Endpoints ---
 
 app.MapGet("/health", () => Results.Content("OK", "text/plain"));
@@ -127,11 +137,13 @@ created_at TEXT NOT NULL
         return Results.BadRequest("Empty body");
     }
 
-    // Parse and extract ttl_days from the JSON
+    // Parse and extract ttl_days from the JSON. shareDocumentOptions, not defaults: this body
+    // wraps a full serialized analysis, and the default 64-level ceiling turned a deep plan's
+    // legitimate upload into a 400 before ttl_days was ever read.
     int ttlDays = 7;
     try
     {
-        using var doc = JsonDocument.Parse(body);
+        using var doc = JsonDocument.Parse(body, shareDocumentOptions);
         if (doc.RootElement.TryGetProperty("ttl_days", out var ttlProp) && ttlProp.TryGetInt32(out var t))
             ttlDays = Math.Clamp(t, 1, MaxTtlDays);
     }

src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs

@@ -16,13 +16,16 @@ namespace PlanViewer.App.Controls;
 
 public partial class PlanViewerControl : UserControl
 {
-    private void PopulateStatementsGrid(List<PlanStatement> statements)
+    /* Takes the container-aware entries rather than bare statements (#456 follow-up): the grid
+       now lists stored procedure and UDF body statements alongside the outer batch, and a row
+       needs to say WHICH module its statement came from or five bare SELECTs are indistinguishable. */
+    private void PopulateStatementsGrid(List<StatementWithContainer> statements)
     {
         StatementsHeader.Text = $"Statements ({statements.Count})";
 
-        var hasActualTimes = statements.Any(s => s.QueryTimeStats != null &&
-            (s.QueryTimeStats.CpuTimeMs > 0 || s.QueryTimeStats.ElapsedTimeMs > 0));
-        var hasUdf = statements.Any(s => s.QueryUdfElapsedTimeMs > 0);
+        var hasActualTimes = statements.Any(e => e.Statement.QueryTimeStats != null &&
+            (e.Statement.QueryTimeStats.CpuTimeMs > 0 || e.Statement.QueryTimeStats.ElapsedTimeMs > 0));
+        var hasUdf = statements.Any(e => e.Statement.QueryUdfElapsedTimeMs > 0);
 
         // Build columns
         StatementsGrid.Columns.Clear();
@@ -129,14 +132,22 @@ private void PopulateStatementsGrid(List<PlanStatement> statements)
         var rows = new List<StatementRow>();
         for (int i = 0; i < statements.Count; i++)
         {
-            var stmt = statements[i];
+            var stmt = statements[i].Statement;
             var allWarnings = stmt.PlanWarnings.ToList();
             if (stmt.RootNode != null)
                 CollectNodeWarnings(stmt.RootNode, allWarnings);
 
             var fullText = stmt.StatementText;
             if (string.IsNullOrWhiteSpace(fullText))
                 fullText = $"Statement {i + 1}";
+
+            /* A body statement gets its module name in front ("dbo.Proc > SELECT ...") in both
+               the cell and its tooltip — display only. Copy/open-in-editor read
+               row.Statement.StatementText and hand out the statement exactly as the plan
+               recorded it, prefix-free. */
+            if (!string.IsNullOrEmpty(statements[i].ContainerPath))
+                fullText = $"{statements[i].ContainerPath} > {fullText}";
+
             var displayText = fullText.Length > 120 ? fullText[..120] + "..." : fullText;
 
             rows.Add(new StatementRow

src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs

@@ -345,9 +345,20 @@ public bool LoadPlan(string planXml, string label, string? queryText = null)
 
         PlanAnalysisPipeline.AnalyzeParsed(_currentPlan, ConfigLoader.Load(), _serverMetadata);
 
-        var allStatements = _currentPlan.Batches
-            .SelectMany(b => b.Statements)
-            .Where(s => s.RootNode != null)
+        /* #456 gave the analysis pipeline and Human/Robot Advice (ResultMapper) the shared
+           PlanStatements.EnumerateAll traversal, which descends into stored procedure and UDF
+           bodies. The grid and the MCP registration below kept walking batch.Statements, so an
+           EXEC <procedure> plan showed one grid row — the EXEC itself — and registered near-zero
+           counts, while the advice discussed dozens of warnings the UI could neither display nor
+           navigate to. Same traversal here so what the grid shows, what the session reports, and
+           what the advice says are the same plan. */
+        var everyStatement = PlanStatements.EnumerateAllWithContainer(_currentPlan).ToList();
+
+        /* Only statements with a root node can render on the canvas. The same filter has always
+           applied to the outer batch (where the parser's synthetic statement roots mean it
+           rarely excludes anything); it now applies across the whole traversal. */
+        var allStatements = everyStatement
+            .Where(e => e.Statement.RootNode != null)
             .ToList();
 
         if (allStatements.Count == 0)
@@ -361,16 +372,20 @@ public bool LoadPlan(string planXml, string label, string? queryText = null)
         PlanScrollViewer.IsVisible = true;
 
         // Always show statement grid — useful summary even for single-statement plans
-        _allStatements = allStatements;
+        _allStatements = allStatements.Select(e => e.Statement).ToList();
         PopulateStatementsGrid(allStatements);
         ShowStatementsPanel();
         StatementsGrid.SelectedIndex = 0;
 
-        // Register with MCP session manager for AI tool access
-        // Count warnings from both statement-level PlanWarnings and all node Warnings
+        /* Register with MCP session manager for AI tool access. Counts run over EVERY statement,
+           renderable or not, because that is what the advice an MCP client reads was built from:
+           statement-level PlanWarnings plus all node warnings, proc/UDF bodies included.
+           StatementCount likewise matches the analysis output's total_statements rather than the
+           grid's renderable subset. */
         int warningCount = 0, criticalCount = 0;
-        foreach (var s in allStatements)
+        foreach (var entry in everyStatement)
         {
+            var s = entry.Statement;
             warningCount += s.PlanWarnings.Count;
             criticalCount += s.PlanWarnings.Count(w => w.Severity == PlanWarningSeverity.Critical);
             if (s.RootNode != null)
@@ -385,8 +400,8 @@ public bool LoadPlan(string planXml, string label, string? queryText = null)
             Source = sessionSource,
             Plan = _currentPlan,
             QueryText = queryText,
-            StatementCount = allStatements.Count,
-            HasActualStats = allStatements.Any(s => s.QueryTimeStats != null),
+            StatementCount = everyStatement.Count,
+            HasActualStats = everyStatement.Any(e => e.Statement.QueryTimeStats != null),
             WarningCount = warningCount,
             CriticalWarningCount = criticalCount,
             MissingIndexCount = _currentPlan.AllMissingIndexes.Count

src/PlanViewer.App/Mcp/McpQueryStoreTools.cs

@@ -311,8 +311,16 @@ internal static PlanSession CaptureSession(
         string connectionInfo)
     {
         var analysis = ResultMapper.Map(parsed, "query-store");
-        var allStatements = parsed.Batches.SelectMany(batch => batch.Statements).ToList();
-        var executableStatement = allStatements.FirstOrDefault(statement => statement.RootNode is not null);
+
+        /* #456 follow-up: the counts used to come from a second walk over batch.Statements while
+           the Analysis stored on this very session is mapped from PlanStatements.EnumerateAll —
+           stored procedure and UDF bodies included, node-level warnings counted — so a Query
+           Store EXEC plan registered statement_count 1 and warning_count 0 alongside an analysis
+           full of findings. The counts now come from that analysis's own summary: one source, and
+           nothing left to disagree with what an MCP client reads. */
+        var executableStatement = Core.Services.PlanStatements.EnumerateAll(parsed)
+            .FirstOrDefault(statement => statement.RootNode is not null);
+
         var session = new PlanSession
         {
             SessionId = sessionId,
@@ -324,12 +332,11 @@ internal static PlanSession CaptureSession(
             DatabaseName = executableStatement?.RootNode?.DatabaseName,
             QueryText = queryText,
             ConnectionInfo = connectionInfo,
-            StatementCount = allStatements.Count,
-            HasActualStats = false,
-            WarningCount = allStatements.Sum(statement => statement.PlanWarnings.Count),
-            CriticalWarningCount = allStatements.Sum(statement =>
-                statement.PlanWarnings.Count(warning => warning.Severity == Core.Models.PlanWarningSeverity.Critical)),
-            MissingIndexCount = parsed.AllMissingIndexes.Count
+            StatementCount = analysis.Summary.TotalStatements,
+            HasActualStats = analysis.Summary.HasActualStats,
+            WarningCount = analysis.Summary.TotalWarnings,
+            CriticalWarningCount = analysis.Summary.CriticalWarnings,
+            MissingIndexCount = analysis.Summary.MissingIndexes
         };
 
         parsed.RawXml = string.Empty;

src/PlanViewer.Core/Models/PlanModels.cs

@@ -12,8 +12,14 @@ public class ParsedPlan
     public bool ClusteredMode { get; set; }
     public List<PlanBatch> Batches { get; set; } = new();
 
-    public List<MissingIndex> AllMissingIndexes => Batches
-        .SelectMany(b => b.Statements)
+    /* #456 follow-up: analysis output lists a missing index per statement, procedure and UDF
+       bodies included, so this rollup must descend the same way — it feeds the MissingIndexCount
+       the MCP session registrations report, and an EXEC <procedure> plan whose only missing-index
+       suggestions live in the body used to register as having none while the advice discussed
+       them. Uses the shared traversal rather than its own descent so it cannot drift from what
+       the analysis actually saw. */
+    public List<MissingIndex> AllMissingIndexes =>
+        Services.PlanStatements.EnumerateAll(this)
         .SelectMany(s => s.MissingIndexes)
         .ToList();
 }

src/PlanViewer.Core/Output/AnalysisJson.cs

@@ -70,4 +70,34 @@ public static class AnalysisJson
         MaxDepth = MaxDepth,
         DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
     };
+
+    /// <summary>
+    /// The share wire format: default JSON in every respect except the depth ceiling.
+    ///
+    /// <para>#431 raised the ceiling for "every writer of this object", and the web share
+    /// endpoints turned out to be three more writers nobody listed: the upload serialized the
+    /// analysis with inline default options, and loading a share back parsed and deserialized it
+    /// at the default 64 again — so a deep-but-real plan (~30 nested operators) analyzed fine on
+    /// screen and then failed to Share, or shared and failed to open, with an "object cycle"
+    /// message pointing at the wrong cause. This is deliberately NOT one of the WithoutNulls
+    /// variants: shares already in the database were written with default formatting, and the fix
+    /// is the ceiling, not a wire-format change riding along with it. Used for both directions —
+    /// deserializers enforce MaxDepth too, and a share that was legal to write must be legal to
+    /// read back.</para>
+    /// </summary>
+    public static readonly JsonSerializerOptions Wire = new()
+    {
+        MaxDepth = MaxDepth,
+    };
+
+    /// <summary>
+    /// The same ceiling for <see cref="System.Text.Json.JsonDocument.Parse(string, JsonDocumentOptions)"/>
+    /// call sites: JsonDocumentOptions is a separate type with its own default MaxDepth of 64, so a
+    /// reader that picks a share apart with JsonDocument before deserializing — which is exactly
+    /// what loading a share does — hits the same wall the serializer options alone cannot fix.
+    /// </summary>
+    public static readonly JsonDocumentOptions Document = new()
+    {
+        MaxDepth = MaxDepth,
+    };
 }

src/PlanViewer.Core/Services/BenefitScorer.cs

@@ -30,23 +30,26 @@ public static void Score(ParsedPlan plan) =>
 
     internal static void ScoreCancellable(ParsedPlan plan, CancellationToken cancellationToken)
     {
-        foreach (var batch in plan.Batches)
+        /* #456 made the analyzer descend into stored procedure and UDF bodies via
+           PlanStatements.EnumerateAll, but this walk was left on batch.Statements. The analyzer
+           then CREATED warnings on the body statements and the scorer never visited them, so their
+           MaxBenefitPercent stayed null and their wait stats were never scored or surfaced — the
+           UI sorts unquantified warnings below quantified ones, which quietly buried every finding
+           inside an EXEC <procedure> plan. Same shared traversal as the analyzer so the two passes
+           cannot see different statements again. */
+        foreach (var stmt in PlanStatements.EnumerateAll(plan))
         {
             cancellationToken.ThrowIfCancellationRequested();
-            foreach (var stmt in batch.Statements)
-            {
-                cancellationToken.ThrowIfCancellationRequested();
-                ScoreStatementWarnings(stmt);
+            ScoreStatementWarnings(stmt);
 
-                if (stmt.RootNode != null)
-                    ScoreNodeTree(stmt.RootNode, stmt, cancellationToken);
+            if (stmt.RootNode != null)
+                ScoreNodeTree(stmt.RootNode, stmt, cancellationToken);
 
-                if (stmt.WaitStats.Count > 0 && stmt.QueryTimeStats != null)
-                    ScoreWaitStats(stmt);
+            if (stmt.WaitStats.Count > 0 && stmt.QueryTimeStats != null)
+                ScoreWaitStats(stmt);
 
-                if (stmt.WaitStats.Count > 0)
-                    EmitWaitStatWarnings(stmt);
-            }
+            if (stmt.WaitStats.Count > 0)
+                EmitWaitStatWarnings(stmt);
         }
     }
 

src/PlanViewer.Core/Services/PlanAnalyzer.Helpers.cs

@@ -30,18 +30,21 @@ private static void MarkLegacyWarningsOnTree(PlanNode node)
             MarkLegacyWarningsOnTree(child);
     }
 
+    /* #456 follow-up: the analyzer walks every statement — stored procedure and UDF bodies
+       included — through PlanStatements.EnumerateAll, but this pass still walked batch.Statements,
+       so a user's severity override applied to a warning on the outer batch and silently did not
+       apply to the identical warning inside an EXEC <procedure> body. (MarkLegacyWarnings does not
+       have this problem: it is called per-statement from inside the analyzer's EnumerateAll loop,
+       so it was carried along when that loop learned to descend.) */
     private static void ApplySeverityOverrides(ParsedPlan plan, AnalyzerConfig cfg)
     {
-        foreach (var batch in plan.Batches)
+        foreach (var stmt in PlanStatements.EnumerateAll(plan))
         {
-            foreach (var stmt in batch.Statements)
-            {
-                foreach (var w in stmt.PlanWarnings)
-                    TryOverrideSeverity(w, cfg);
+            foreach (var w in stmt.PlanWarnings)
+                TryOverrideSeverity(w, cfg);
 
-                if (stmt.RootNode != null)
-                    ApplyOverridesToTree(stmt.RootNode, cfg);
-            }
+            if (stmt.RootNode != null)
+                ApplyOverridesToTree(stmt.RootNode, cfg);
         }
     }