Release v1.24.0

.claude/skills/maintenance/SKILL.md

@@ -0,0 +1,74 @@
+---
+name: maintenance
+description: Quarterly maintenance pass (every 1-3 months) — dependency/security audit, build health, and repo hygiene for PerformanceMonitor and PerformanceStudio
+argument-hint: [optional: deps | build | all]
+disable-model-invocation: false
+---
+
+# Routine Maintenance Pass
+
+A repeatable every-1-3-months health check for the .NET desktop + SQL-monitoring repos
+(PerformanceMonitor = WPF, PerformanceStudio = Avalonia; same dependency/build/release shape).
+`$ARGUMENTS` optionally scopes it (`deps`, `build`, or `all` — default `all`).
+
+Work top to bottom. Do read-only scans first and report findings; make fixes on a feature
+branch/worktree (branch protection: never commit to `dev`/`main`), build + test, then PR to `dev`.
+Low-risk patch/minor bumps can go in one PR; majors and anything release-critical get their own.
+
+## A. Dependencies & Security
+
+> **Scope — scan every project, not just the solution.** A repo's `.sln` may not list every project. PerformanceStudio's `PlanViewer.sln` omits `server/PlanShare.csproj` and the SSMS VSIX (`PlanViewer.Ssms`, `PlanViewer.Ssms.Installer`), so the `dotnet list <Solution>.sln …` scans and the solution build all silently skip them. Run the checks against those projects too. The net472 VSIX is old-style, so `dotnet list` is unreliable on it — read its `<PackageReference>`s by hand (its `Microsoft.VSSDK.BuildTools` is intentionally held on the 17.x line; 18.x is un-restorable from nuget.org and targets VS 18, not VS 2022). Where the `.sln` covers all projects, the solution is enough.
+
+1. **Outdated packages.** `dotnet list <Solution>.sln package --outdated`
+   - Take low-risk **patch/minor** bumps (Microsoft.Extensions.*, Test.Sdk, etc.).
+   - **Majors get their own effort** — especially the update framework (Velopack: bump the library AND the `vpk` CLI pin in the release workflow — `.github/workflows/release.yml` for PerformanceStudio — together; validate the in-app + Setup.exe update path) and anything release-critical.
+   - **Engine-wrapped bindings** (e.g. DuckDB.NET) trail the native engine — bump when the binding catches up, and validate behavior (for DuckDB run `tools/CompactionRepro` re: the parquet-COPY memory-limit floor before changing the version).
+   - After version edits, **if the repo uses lock files** (`packages.lock.json` — PerformanceMonitor does; PerformanceStudio does not), regenerate them: `dotnet restore <Solution>.sln --force-evaluate` (CI restores `--locked-mode`).
+
+2. **Vulnerable packages (security).** `dotnet list <Solution>.sln package --vulnerable --include-transitive`
+   - Must be **zero**. Any hit (incl. transitive) is urgent — bump or pin to a fixed version. This catches CVEs the `--outdated` check does not.
+   - **Code-level pass, not just packages:** run the `security-review` skill/agent on the diff since the last maintenance pass. These apps have real attack surface beyond their dependencies — PerformanceStudio's MCP server opens a local network listener, both store DB credentials (Windows Credential Manager), and both parse untrusted input (e.g. execution-plan XML). Triage anything it flags.
+     - **Calibrate severity to the deployment.** PerformanceStudio runs on a single-user personal laptop, and its MCP tools are strictly read-only (no arbitrary SQL, no writes/config changes). So loopback-bound / opt-in / local-IPC findings — the MCP listener, the named-pipe single-instance server — are **Low/informational here, not High**: there's no other local user or attacker to exploit them, and read-only tools can at most leak data they already return. Reserve High for *remotely reachable* vectors (e.g. a missing `Host`/`Origin` check that allows DNS rebinding) or credential disclosure. This calibration would change only if Studio shipped the MCP server enabled-by-default or ran on a shared/multi-user host. Don't re-raise the same local-IPC findings at High each pass.
+
+3. **Deprecated packages.** `dotnet list <Solution>.sln package --deprecated` — replace anything abandoned.
+
+4. **Framework / runtime currency.** Confirm the TFM is on a **supported, released** .NET (do NOT move to a preview). Take the latest servicing patch of the current major. WPF/Avalonia track the runtime/their own NuGet — check both.
+
+5. **CI tool & action pins.** In `.github/workflows/*.yml`: confirm `uses:` actions are on current majors, and that any `dotnet tool install` is **version-pinned to match its library** (e.g. `vpk --version` must equal the Velopack PackageReference). Bump GitHub Actions that are behind / deprecated.
+
+6. **Dependabot (optional — not a finding).** Two separate features: *security alerts* (passive CVE notifications that close the gap between manual `--vulnerable` passes — mild value) and *version-update PRs* (automated bump PRs — redundant and noisy once you do periodic manual sweeps). For PerformanceStudio, Erik relies on the manual passes; treat Dependabot as **optional and do not report it as a finding**. If alerts are ever wanted they're a one-toggle enable (repo Settings → Code security, no config file); the version-update PRs aren't wanted.
+
+## B. Code & Build Health
+
+7. **Zero-warning build.** Build the whole solution and capture warnings — the standard is **0**:
+   ```
+   dotnet build <Solution>.sln -c Debug --nologo 2>&1 | Select-String ": warning "
+   ```
+   - Kill the running app(s) first OR build in a worktree — a running Dashboard/Lite/Studio locks its `bin` DLLs (MSB3021). Note: an incremental no-op build emits no warnings; force a clean compile of any project you're checking.
+   - Fix each warning honestly (don't add to `NoWarn` to silence). Remove dead code (e.g. an unused test seam → CS0649).
+
+8. **NoWarn review.** Scan each csproj `<NoWarn>`: every suppressed rule should have a reason (these repos keep inline comments). Remove suppressions that no longer fire; don't let the list grow silently. Most existing CA suppressions are intentional high-count ones — leave those.
+
+9. **Stale markers.** `grep -rE "\b(TODO|FIXME|HACK|XXX)\b" --include=*.cs` — resolve or file an issue; a `// TODO: restore to X` next to a non-X value may mean the comment is the leftover (ask before flipping).
+
+10. **Git repo hygiene.**
+    - **Line endings / `.gitattributes`.** If the repo has no `.gitattributes`, line endings drift (CRLF/LF mixed, `core.autocrlf=false`) and bulk edits balloon diffs. Add one and run a **dedicated** `git add --renormalize .` commit (its own PR, when no other work is in flight — it touches nearly every file).
+    - **Stale branches & worktrees.** `git worktree list` — remove leftover worktrees (anything under `.claude/worktrees/` or other agent/isolation worktrees) with `git worktree remove`. Then `git fetch --prune` to drop stale remote-tracking refs, and audit: `git branch --merged origin/dev` (local branches already in dev — safe to delete) and `git branch -r` (remote branches from closed/merged PRs). Delete merged/dead branches; **keep intentionally-parked ones** (note which and why — e.g. a blocked-upgrade branch like `upgrade/avalonia-12`). For branches *you didn't create*, surface and confirm before deleting rather than assuming abandoned. Confirm open PRs are still wanted.
+
+## C. App data & runtime hygiene (lighter)
+
+11. **Retention / archive end-to-end.** Confirm the app's data retention/purge and (Lite) parquet archiving actually prune old data, and logs rotate. A new time-series table must be registered for retention/archive or it grows forever.
+
+12. **Perf regression spot-check.** Re-run the UI-latency-under-load harness if available; watch known hot spots (e.g. the Lite Blocking tab render hitch). Quick collector-health pass.
+
+## D. Release & platform currency (lighter)
+
+13. **Release infra freshness.** Test servers online/patched; signing cert (SignPath) not near expiry; cloud creds (`az`/`aws`) valid; the `release-checklist` skill still accurate.
+    - **Cross-platform publish smoke.** `dotnet publish` the desktop app for the non-Windows runtimes it ships (PerformanceStudio: `linux-x64`, `osx-arm64`/`osx-x64`) and confirm each still produces a runnable app. The Avalonia/SkiaSharp native pins are fragile — the Linux `SkiaSharp.NativeAssets.Linux` pin exists to guard GitHub issue #139 — and a Windows-only build won't catch a broken Linux/macOS runtime.
+
+14. **SQL Server / cloud drift + bundled tools.** New SQL Server CU/version, new DMVs/columns, Azure SQL DB / RDS changes (cloud collector paths have a bug history); refresh bundled community procs (sp_WhoIsActive, sp_BlitzLock, sp_HealthParser, sp_HumanEventsBlockViewer).
+
+## Output
+
+Report per section: ✅ clean / ⚠️ findings (with the fix made or recommended). Group merged PRs and
+"parked" items (e.g. a major bump deferred) so the next pass knows where things stand.

.gitignore

@@ -424,5 +424,6 @@ FodyWeavers.xsd
 
 # Internal development files (not for public release)
 .internal/
-.claude/
+.claude/*
+!.claude/skills/
 CLAUDE.md

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/Directory.Build.props

@@ -15,7 +15,7 @@
     Tests and server/ projects are outside src/ and are unaffected.
   -->
   <PropertyGroup>
-    <Version>1.23.0</Version>
+    <Version>1.24.0</Version>
     <Authors>Erik Darling</Authors>
     <Company>Darling Data LLC</Company>
     <Product>Performance Studio</Product>

src/PlanViewer.App/AboutWindow.axaml.cs

@@ -6,6 +6,7 @@
 
 using System;
 using System.Diagnostics;
+using System.Threading.Tasks;
 using System.Reflection;
 using System.Runtime.InteropServices;
 using Avalonia.Controls;
@@ -188,7 +189,28 @@ private async void CheckUpdate_Click(object? sender, RoutedEventArgs e)
 
     private bool _updateDownloaded;
 
+    /* Every branch of UpdateLink_Click awaits — a dialog, the unsaved-work walk, a download —
+       and the link stays clickable the whole time, so a second click would start a second
+       concurrent copy of whichever step is in flight (two restart dialogs, two walks prompting
+       about the same tabs, two downloads). One latch at the top covers all of them. */
+    private bool _updateActionInFlight;
+
     private async void UpdateLink_Click(object? sender, PointerPressedEventArgs e)
+    {
+        if (_updateActionInFlight)
+            return;
+        _updateActionInFlight = true;
+        try
+        {
+            await HandleUpdateLinkClickAsync();
+        }
+        finally
+        {
+            _updateActionInFlight = false;
+        }
+    }
+
+    private async Task HandleUpdateLinkClickAsync()
     {
         // Step 3: User clicks "Restart now" after download — confirm first
         if (_updateDownloaded && _velopackMgr != null && _velopackUpdate != null)
@@ -231,6 +253,33 @@ private async void UpdateLink_Click(object? sender, PointerPressedEventArgs e)
 
             if (result)
             {
+                /* ApplyUpdatesAndRestart kills the process outright: MainWindow.OnClosing
+                   never fires, so the unsaved-changes walk (#462/#473) never ran on this
+                   route and dirty edits were discarded without a question — and OnClosed's
+                   session save never ran either, so the relaunched app restored nothing
+                   (RestoreOpenPlans had already cleared the saved list at startup). Ask the
+                   same questions the close path asks, and if anyone answers Cancel, abort
+                   the restart and leave this window usable — the update stays downloaded. */
+                /* Resolved through the application lifetime, not Owner: an Owner-typed check
+                   would fail OPEN — shown with any other owner, the walk silently vanishes and
+                   this route is right back to discarding dirty edits, the exact bug being
+                   fixed. The main window owns every session, so if none exists there is no
+                   unsaved work to lose and restarting without a walk is genuinely safe. */
+                var main = Owner as MainWindow
+                    ?? (Avalonia.Application.Current?.ApplicationLifetime
+                        as Avalonia.Controls.ApplicationLifetimes.IClassicDesktopStyleApplicationLifetime)
+                        ?.MainWindow as MainWindow;
+
+                if (main != null)
+                {
+                    if (!await main.ConfirmAllUnsavedWorkAsync())
+                        return;
+
+                    /* After the walk, not before: a Save answer in the walk can give a
+                       scratch tab a file, which this then writes down for the restore. */
+                    main.PersistSessionForRestart();
+                }
+
                 _velopackMgr.ApplyUpdatesAndRestart(_velopackUpdate.TargetFullRelease);
             }
             return;

src/PlanViewer.App/App.axaml.cs

@@ -54,7 +54,11 @@ public override void OnFrameworkInitializationCompleted()
         // Register the .sqlplan association (Windows/Linux) off the UI thread so it
         // never delays first paint. Best-effort; the OS then routes double-clicks to
         // the existing argv/pipe open path. No-op on macOS (handled by Info.plist).
-        Task.Run(FileAssociationService.RegisterForCurrentExecutable);
+        // Never in the test host: this writes HKCU\Software\Classes, and the harness
+        // booting the real App (#451) rewrote the machine's .sqlplan association and
+        // DefaultIcon to point at the test runner executable — confirmed live.
+        if (!AppRuntimeMode.IsTestHost)
+            Task.Run(FileAssociationService.RegisterForCurrentExecutable);
 
         base.OnFrameworkInitializationCompleted();
     }

src/PlanViewer.App/AppRuntimeMode.cs

@@ -0,0 +1,25 @@
+namespace PlanViewer.App;
+
+/// <summary>
+/// Process-wide answer to "is this the real app or the test host?"
+///
+/// <para>The headless test harness (#451) boots the REAL <see cref="App"/> — deliberately,
+/// because MainWindow resolves styles from the application XAML — which means the real
+/// startup side effects run inside the test runner unless something says otherwise. That
+/// was not hypothetical: a local <c>dotnet test</c> rewrote HKCU's .sqlplan association to
+/// point at the test host executable, polluted the developer's Recent Plans with fixture
+/// paths, and destroyed the saved open-tab list, all confirmed live.</para>
+///
+/// <para>This is the one seam those side effects consult, rather than a scattering of
+/// environment sniffs. The harness sets it before the first App boots; nothing in the
+/// product ever sets it, so in the real app every check reads a constant false and
+/// behavior is unchanged.</para>
+/// </summary>
+internal static class AppRuntimeMode
+{
+    /// <summary>
+    /// True only inside the test host. Set once by the test harness's module initializer,
+    /// never by the app itself.
+    /// </summary>
+    internal static bool IsTestHost;
+}

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

@@ -360,7 +360,11 @@ private async void SavePlan_Click(object? sender, RoutedEventArgs e)
             catch (Exception ex)
             {
                 System.Diagnostics.Debug.WriteLine($"SavePlan failed: {ex.Message}");
-                CostText.Text = $"Save failed: {(ex.Message.Length > 60 ? ex.Message[..60] + "..." : ex.Message)}";
+                /* #452's mirror, the sixth site (spotted during the review sweep): pre-cutting to
+                   60 characters threw away exactly the part of an I/O error that says what to fix.
+                   Full message out; the tooltip carries whatever the label clips. */
+                CostText.Text = $"Save failed: {ex.Message}";
+                ToolTip.SetTip(CostText, ex.Message);
             }
         }
     }

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