From 6decaf8b3fba842c3874fd667d1bb6d13fc06bbb Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 3 Aug 2026 12:52:56 +0200 Subject: [PATCH 01/15] Skip the AI review job on Dependabot PRs Every Dependabot PR carried a red X from the review check, which failed twice over instead of reviewing: claude-code-action rejects non-human actors outright ("Workflow initiated by non-human actor: dependabot"), and a dependabot-triggered run draws from the Dependabot secret store, not the Actions one, so CLAUDE_CODE_OAUTH_TOKEN was empty regardless. Both open bumps (#420, #422) build and pass tests; only this check blocked them. A version bump has nothing an AI review would catch that build-and-test doesn't, so skip - neutral, not red - in the same spirit as the existing draft/fork/release-PR exclusions. Co-Authored-By: Claude Fable 5 --- .github/workflows/claude-code-review.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index c3916c3..d4d8a8f 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -37,9 +37,17 @@ jobs: # were each already reviewed on their own PR, so re-reviewing the whole # release adds nothing — and the diff is large enough that it reliably # exhausts the turn budget and fails, putting a red X on the release. + # + # Also skip Dependabot PRs. They fail twice over instead of getting a + # review: the action rejects non-human actors outright ("Workflow initiated + # by non-human actor: dependabot"), and a dependabot-triggered run draws + # from the DEPENDABOT secret store, not the Actions one, so the OAuth token + # is empty anyway. A version bump has nothing an AI review would catch that + # build-and-test doesn't; skipped is neutral, not a red X on every bump. if: | github.event.pull_request.draft == false && github.event.pull_request.head.repo.full_name == github.repository && + github.actor != 'dependabot[bot]' && !(github.event.pull_request.head.ref == 'dev' && github.event.pull_request.base.ref == 'main') runs-on: ubuntu-latest From 8001b90776bf924bc0fa611ad3db837fbec23031 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 3 Aug 2026 11:10:29 +0000 Subject: [PATCH 02/15] deps: Bump ModelContextProtocol and ModelContextProtocol.AspNetCore Bumps ModelContextProtocol from 1.4.1 to 2.0.0 Bumps ModelContextProtocol.AspNetCore from 1.4.1 to 2.0.0 --- updated-dependencies: - dependency-name: ModelContextProtocol dependency-version: 2.0.0 dependency-type: direct:production update-type: version-update:semver-major - dependency-name: ModelContextProtocol.AspNetCore dependency-version: 2.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- src/PlanViewer.App/PlanViewer.App.csproj | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/PlanViewer.App/PlanViewer.App.csproj b/src/PlanViewer.App/PlanViewer.App.csproj index 2d7bbe7..f6d3bb0 100644 --- a/src/PlanViewer.App/PlanViewer.App.csproj +++ b/src/PlanViewer.App/PlanViewer.App.csproj @@ -21,8 +21,8 @@ - - + + From f491a3e75d33e4ba9297c8b649785748e9bd1faf Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 3 Aug 2026 11:10:33 +0000 Subject: [PATCH 03/15] deps: Bump Microsoft.SqlServer.TransactSql.ScriptDom and SQLitePCLRaw.bundle_e_sqlite3 Bumps Microsoft.SqlServer.TransactSql.ScriptDom from 180.59.2 to 180.78.1 Bumps SQLitePCLRaw.bundle_e_sqlite3 from 3.0.4 to 3.0.5 --- updated-dependencies: - dependency-name: Microsoft.SqlServer.TransactSql.ScriptDom dependency-version: 180.78.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: Microsoft.SqlServer.TransactSql.ScriptDom dependency-version: 180.78.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: Microsoft.SqlServer.TransactSql.ScriptDom dependency-version: 180.78.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: Microsoft.SqlServer.TransactSql.ScriptDom dependency-version: 180.78.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: SQLitePCLRaw.bundle_e_sqlite3 dependency-version: 3.0.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor ... Signed-off-by: dependabot[bot] --- server/PlanShare/PlanShare.csproj | 2 +- src/PlanViewer.App/PlanViewer.App.csproj | 2 +- src/PlanViewer.Core/PlanViewer.Core.csproj | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/server/PlanShare/PlanShare.csproj b/server/PlanShare/PlanShare.csproj index 8d30dcd..a268903 100644 --- a/server/PlanShare/PlanShare.csproj +++ b/server/PlanShare/PlanShare.csproj @@ -14,7 +14,7 @@ drop it only once that floor moves past 2.1.11 on its own. 3.0.4 bundles SQLite 3.53.3, and Microsoft.Data.Sqlite 10.0.10 works against the 3.x native layer (full CRUD verified). --> - + diff --git a/src/PlanViewer.App/PlanViewer.App.csproj b/src/PlanViewer.App/PlanViewer.App.csproj index 2d7bbe7..f709b92 100644 --- a/src/PlanViewer.App/PlanViewer.App.csproj +++ b/src/PlanViewer.App/PlanViewer.App.csproj @@ -24,7 +24,7 @@ - + - - - + + + - - + + From 956b7786b91ec1b667b723d05696d89e428781a9 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:41:30 +0200 Subject: [PATCH 05/15] Give the WAM broker a parent window handle so Entra MFA works (#425) Interactive Microsoft Entra MFA could not work in Studio on Windows at all. Current Microsoft.Data.SqlClient (7.0.2 here) routes Authentication=ActiveDirectoryInteractive through the Windows WAM broker, and WAM requires the application to supply the HWND that will own its account picker. ServerConnection.BuildConnectionString set the authentication mode but nothing in the repo ever registered an auth provider, so instead of a prompt users got "0xwindow_handle_required / A window handle must be configured" and the connection failed. Not tenant-specific: broken for every Windows user on a current build. Studio 1.4.3 worked because it predates WAM being the default and fell back to a browser. Reported by joshdbe, who supplied the diagnostic that made it findable: 1.4.3 works, 1.19.1 does not. Same defect in PerformanceMonitor Lite (erikdarlingdata/PerformanceMonitor#2184); doing Studio first because that is the tool the reporter actually uses. EntraInteractiveAuth.Register takes a handle provider and installs an ActiveDirectoryAuthenticationProvider. Three deliberate choices: - Registered ONCE at app startup rather than per connection. SqlAuthenticationProvider installs against the authentication METHOD, not a connection, so one call covers every SqlConnection the process opens. Studio opens them from several unrelated places (connection dialog, query session control, schema service) and threading a handle through all of them is a change every future call site could forget. - WINDOWS ONLY. WAM does not exist on macOS or Linux, where interactive auth already works through the system browser and needs no handle, so registering a handle-supplying provider there would add a failure mode to the platforms Avalonia exists to serve. - The handle is resolved PER PROMPT, not captured at startup, and prefers the active window over the main window. A platform handle is not valid until its window is sourced, and a connection is usually triggered from the connection dialog - parenting the picker to the main window behind it would let the picker appear behind the dialog the user is looking at. The CLI gets a clear refusal instead of MSAL's error. planview accepts --auth entra and has no window to give, so it would have failed deep inside MSAL with a message that tells the operator nothing. It now says so up front and names the modes that work headless. Deliberately NOT silently substituting another auth mode: connecting as a different identity than the one asked for is worse than refusing. Whether the CLI should grow device-code flow is the open question on #425 rather than something to guess at here. Tests cover what is verifiable without a tenant: null handle provider rejected at wiring time rather than at prompt time, the non-Windows contract (Register declines, the provider is never consulted, and IsSupported stays TRUE so the CLI guard does not refuse entra on platforms where it works), and idempotent registration, since SetProvider is process-wide and a second call would otherwise silently replace the first once the SSMS extension wires in too. Whether the picker actually authenticates can only be established against a real Entra tenant on an Azure VM; the reporter has offered to test that and this is not claimed as verified until he does. Co-Authored-By: Claude Fable 5 --- src/PlanViewer.App/App.axaml.cs | 31 ++++++++ .../Commands/CliConnectionResolver.cs | 20 +++++ .../Services/EntraInteractiveAuth.cs | 79 +++++++++++++++++++ .../EntraInteractiveAuthTests.cs | 59 ++++++++++++++ 4 files changed, 189 insertions(+) create mode 100644 src/PlanViewer.Core/Services/EntraInteractiveAuth.cs create mode 100644 tests/PlanViewer.Core.Tests/EntraInteractiveAuthTests.cs diff --git a/src/PlanViewer.App/App.axaml.cs b/src/PlanViewer.App/App.axaml.cs index 0f328bf..5dd2a47 100644 --- a/src/PlanViewer.App/App.axaml.cs +++ b/src/PlanViewer.App/App.axaml.cs @@ -9,6 +9,7 @@ using System.Linq; using System.Threading.Tasks; using PlanViewer.App.Services; +using PlanViewer.Core.Services; namespace PlanViewer.App; @@ -30,6 +31,13 @@ public override void OnFrameworkInitializationCompleted() desktop.MainWindow = new MainWindow(); } + // Entra MFA needs a parent window handle for the WAM broker, or the prompt never + // appears and the connection fails with 0xwindow_handle_required (issue #425). + // Registered once here because SqlAuthenticationProvider is process-wide, so this + // covers every connection Studio opens without touching each call site. No-op off + // Windows, where interactive auth uses the browser and needs no handle. + EntraInteractiveAuth.Register(ActiveWindowHandle); + if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) { var iconPath = System.IO.Path.Combine(AppContext.BaseDirectory, "EDD.icns"); @@ -50,6 +58,29 @@ public override void OnFrameworkInitializationCompleted() base.OnFrameworkInitializationCompleted(); } + /// + /// The window that should own an Entra MFA prompt, resolved at the moment MSAL asks (issue #425). + /// + /// Prefers whichever window is currently active over the main window, because a connection is + /// usually triggered from the connection dialog — parenting the account picker to the main window behind + /// it would let the picker appear behind the dialog the user is looking at. Falls back to the main window, + /// then to , which MSAL treats the same as no handle: the prompt fails rather + /// than the app crashing, which is the right way round for an auth path. + /// + /// Resolved per call rather than captured once: a window's platform handle is not valid until the + /// window has been sourced, so a handle read at startup can be zero even though a real window exists a + /// moment later. + /// + private static IntPtr ActiveWindowHandle() + { + if (Current?.ApplicationLifetime is not IClassicDesktopStyleApplicationLifetime desktop) + return IntPtr.Zero; + + var window = desktop.Windows.FirstOrDefault(w => w.IsActive) ?? desktop.MainWindow; + + return window?.TryGetPlatformHandle()?.Handle ?? IntPtr.Zero; + } + /// /// Handles macOS file-open activations (). The /// opened plan paths arrive here via the activation event rather than argv, so we diff --git a/src/PlanViewer.Cli/Commands/CliConnectionResolver.cs b/src/PlanViewer.Cli/Commands/CliConnectionResolver.cs index 3893cd9..f8ce0fa 100644 --- a/src/PlanViewer.Cli/Commands/CliConnectionResolver.cs +++ b/src/PlanViewer.Cli/Commands/CliConnectionResolver.cs @@ -43,6 +43,26 @@ public static ServerConnection BuildServerConnection( throw new InvalidOperationException("No credentials configured"); } + /* Interactive Entra auth goes through the Windows WAM broker, which needs a parent window handle to + own its account picker (issue #425). A CLI has no window to give it, so the connection would fail + deep inside MSAL with "0xwindow_handle_required" — a message that tells the user nothing about + what to do. Say it here instead, up front, and name the modes that actually work headless. + + Deliberately not silently substituting another auth mode: picking a different identity than the + one the operator asked for is worse than refusing. Whether the CLI should grow device-code flow is + the open question on #425, not something to guess at from here. */ + if (authType == AuthenticationTypes.EntraMFA && !EntraInteractiveAuth.IsSupported) + { + Console.Error.WriteLine( + "Interactive Microsoft Entra MFA (--auth entra) needs a desktop window for the Windows " + + "account picker, so it cannot run from the CLI."); + Console.Error.WriteLine( + "Use the Studio app for interactive sign-in, or a non-interactive identity here: " + + "--auth sql with a stored credential, or a service principal / managed identity."); + Environment.ExitCode = 1; + throw new InvalidOperationException("Interactive Entra authentication is not available headless"); + } + return new ServerConnection { Id = server, diff --git a/src/PlanViewer.Core/Services/EntraInteractiveAuth.cs b/src/PlanViewer.Core/Services/EntraInteractiveAuth.cs new file mode 100644 index 0000000..317bec9 --- /dev/null +++ b/src/PlanViewer.Core/Services/EntraInteractiveAuth.cs @@ -0,0 +1,79 @@ +using System; +using Microsoft.Data.SqlClient; + +namespace PlanViewer.Core.Services; + +/// +/// Makes Authentication=ActiveDirectoryInteractive (Microsoft Entra MFA) work on Windows by giving +/// MSAL a parent window handle. +/// +/// Why this has to exist: current Microsoft.Data.SqlClient routes interactive Entra auth through +/// the Windows WAM broker, and WAM requires the calling application to supply the HWND that will own +/// the account picker. An application that never supplies one does not get a prompt — it gets +/// 0xwindow_handle_required / "A window handle must be configured" and the connection fails outright. +/// Studio set the authentication mode but never registered a provider, so Entra MFA was broken for every +/// Windows user rather than for some particular tenant (issue #425, reported via PerformanceMonitor#2184; +/// Studio 1.4.3 worked because it predates WAM being the default and fell back to a browser). +/// +/// Registration is process-wide. installs against +/// the authentication METHOD, not against a connection, so one call at startup covers every +/// SqlConnection the process opens — which is what we want here, because Studio opens connections from +/// several unrelated places (the connection dialog, the query session control, the schema service) and +/// threading a handle through all of them would be a change every future call site could forget to make. +/// +public static class EntraInteractiveAuth +{ + private static readonly object Gate = new(); + private static bool _registered; + + /// + /// Registers the interactive-auth provider, resolving the parent window through + /// at the moment MSAL asks for it. + /// + /// The handle is fetched per prompt rather than captured once, deliberately: a window's platform + /// handle is not valid until the window has been sourced, and the right parent is whichever window is + /// actually in front when the user triggers a connection — not necessarily the one that existed at + /// startup. + /// + /// Windows only. WAM does not exist on macOS or Linux, where interactive auth already works through + /// the system browser and needs no handle at all, so registering a handle-supplying provider there would + /// add a failure mode to platforms that currently work. Returns false when it did not register, so a + /// caller can tell "not applicable" from "done" without duplicating the OS check. + /// + /// + /// Returns the owning window handle, or when no window is available yet. + /// + /// True if the provider was registered by this call; false if not applicable or already done. + public static bool Register(Func parentWindowHandleProvider) + { + ArgumentNullException.ThrowIfNull(parentWindowHandleProvider); + + if (!OperatingSystem.IsWindows()) + return false; + + lock (Gate) + { + if (_registered) + return false; + + var provider = new ActiveDirectoryAuthenticationProvider(); + + /* Func rather than Func is MSAL's shape — it takes an Android Activity on + mobile and an HWND on Windows. Boxing the IntPtr is the intended usage. */ + provider.SetParentActivityOrWindowFunc(() => parentWindowHandleProvider()); + + SqlAuthenticationProvider.SetProvider(SqlAuthenticationMethod.ActiveDirectoryInteractive, provider); + + _registered = true; + return true; + } + } + + /// + /// Whether interactive Entra auth is expected to work in this process: either a provider has been + /// registered, or we are not on Windows and therefore never needed one. False means a prompt would fail + /// with 0xwindow_handle_required — which is the case a windowless host (the CLI) should surface as + /// a clear message instead of letting MSAL produce that error. + /// + public static bool IsSupported => _registered || !OperatingSystem.IsWindows(); +} diff --git a/tests/PlanViewer.Core.Tests/EntraInteractiveAuthTests.cs b/tests/PlanViewer.Core.Tests/EntraInteractiveAuthTests.cs new file mode 100644 index 0000000..4489d87 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/EntraInteractiveAuthTests.cs @@ -0,0 +1,59 @@ +using System; +using PlanViewer.Core.Services; +using Xunit; + +namespace PlanViewer.Core.Tests; + +/// +/// Issue #425: interactive Entra auth needs a parent window handle on Windows, because SqlClient routes it +/// through the WAM broker. These pin the parts that are verifiable without a tenant — the OS gating and the +/// contract the windowless CLI depends on. Whether the picker actually authenticates can only be established +/// against a real Entra tenant, which is what the reporter offered to do. +/// +public class EntraInteractiveAuthTests +{ + [Fact] + public void Register_RejectsANullHandleProvider() + { + /* The whole point of the type is supplying a handle; accepting null would register a provider that + fails at prompt time instead of at wiring time, which is the harder bug to find. */ + Assert.Throws(() => EntraInteractiveAuth.Register(null!)); + } + + [Fact] + public void OffWindows_RegistrationIsSkippedButInteractiveAuthIsStillConsideredSupported() + { + /* macOS and Linux have no WAM broker: interactive auth goes through the system browser and needs no + handle, so registering a handle-supplying provider there would add a failure mode to the platforms + that currently work. Register must decline, and IsSupported must still be TRUE — otherwise the CLI + guard would refuse `--auth entra` on exactly the platforms where it is fine. */ + Assert.SkipWhen(OperatingSystem.IsWindows(), "Windows has WAM; this pins the non-Windows contract."); + + var called = false; + var registered = EntraInteractiveAuth.Register(() => { called = true; return IntPtr.Zero; }); + + Assert.False(registered, "off Windows there is nothing to register"); + Assert.False(called, "the handle provider must not even be consulted off Windows"); + Assert.True(EntraInteractiveAuth.IsSupported, + "browser-based interactive auth works off Windows, so the CLI must not refuse it there"); + } + + [Fact] + public void OnWindows_RegistersOnceAndIsIdempotent() + { + /* SqlAuthenticationProvider.SetProvider is process-wide, so a second registration would silently + replace the first — and with several entry points able to call this (the app today, the SSMS + extension later) "first one wins, later ones are no-ops" is the contract worth pinning. */ + Assert.SkipUnless(OperatingSystem.IsWindows(), "WAM registration only happens on Windows."); + + var first = EntraInteractiveAuth.Register(() => IntPtr.Zero); + var second = EntraInteractiveAuth.Register(() => new IntPtr(1234)); + + Assert.False(second, "a second Register must be a no-op rather than replacing the provider"); + Assert.True(EntraInteractiveAuth.IsSupported); + + /* first is only true when this test observed the very first registration in the process; another test + or the host may legitimately have gotten there first, so it is not asserted either way. */ + _ = first; + } +} From 5f8411625f5aeb7e1520f9bcc2c506f86ac7a122 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 11 Aug 2026 20:02:36 +0200 Subject: [PATCH 06/15] Watchdog the test-host hang from outside the process PlanViewer.Core.Tests can wedge in a CoreCLR GC-suspension livelock on macOS ARM64 (.NET 10.0.8) and spin a full core until someone notices the heat. It happened twice unprompted today: three hosts at ~108% CPU each, one of them wedged 22 minutes and still going when sampled. Root-caused from a sample: SuspendEE present in all 2451 samples, the victim thread interrupted where CheckActivationSafePoint can never succeed, and the mach-exception thread churning thread_get_state / thread_set_state forever. The load-bearing fact is that nothing inside the test host can stop it. xUnit timeouts and in-test CancelAfter cannot fire because the execution engine itself is suspended - that IS the livelock - and dotnet-stack and EventPipe hang for the same reason. Only an out-of-process watchdog works, and both mechanisms here are enforced by vstest.console, a separate process from the host it watches. Two layers, deliberately: - The three CI invocations (ci, nightly, release) now pass the blame-hang options, which is what produces a sequence file naming the test that wedged. - RunSettingsFilePath in the test csproj points at hang-watchdog.runsettings, so a bare `dotnet test` with no flags gets a TestSessionTimeout backstop. This is the layer that matters most: BOTH incidents were plain local runs, so a fix living only in the workflow files would protect the machine that was never at risk and leave the laptop unprotected. 15 minutes is far above any real runtime (individual tests are milliseconds, the whole suite normally well under a minute), so it only fires on a genuine wedge. Verified rather than assumed, because a silently-ignored runsettings would look exactly like a working one: temporarily set TestSessionTimeout to 1ms and a bare `dotnet test` reported "Aborting test run: test run timeout of 1 milliseconds exceeded", then restored it. The blame collector confirms itself active on a normal run ("All tests finished running, Sequence file will not be generated"). Zero test hosts survived any run. Two XML-comment errors on the way in, both caught by the toolchain rather than by me: '--' is illegal inside an XML comment, and I had written the flag names and a dash-separated aside into comments in both the csproj and the runsettings. Evidence for a future dotnet/runtime report is preserved outside the session scratchpad at ~/Documents/dotnet-hang-evidence (sample, gzip, and a README explaining the signature); related to dotnet/runtime#66759. Co-Authored-By: Claude Fable 5 --- .github/workflows/ci.yml | 2 +- .github/workflows/nightly.yml | 2 +- .github/workflows/release.yml | 2 +- .../PlanViewer.Core.Tests.csproj | 7 +++++ .../hang-watchdog.runsettings | 30 +++++++++++++++++++ 5 files changed, 40 insertions(+), 3 deletions(-) create mode 100644 tests/PlanViewer.Core.Tests/hang-watchdog.runsettings diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8dfc5a1..5c48edb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,4 +46,4 @@ jobs: run: dotnet build PlanViewer.sln -c Release --no-restore - name: Run tests - run: dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --no-build --verbosity normal + run: dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --no-build --verbosity normal --blame-hang --blame-hang-timeout 5m --blame-hang-dump-type none diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 3581793..c8d4137 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -65,7 +65,7 @@ jobs: dotnet restore tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj - name: Run tests - run: dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --verbosity normal + run: dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --verbosity normal --blame-hang --blame-hang-timeout 5m --blame-hang-dump-type none - name: Publish App (all platforms) run: | diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 48d7596..8b7e3ad 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -52,7 +52,7 @@ jobs: run: | dotnet restore dotnet build -c Release - dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --no-build --verbosity normal + dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --no-build --verbosity normal --blame-hang --blame-hang-timeout 5m --blame-hang-dump-type none - name: Publish App (all platforms) run: | diff --git a/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj b/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj index ff8e2e3..38c85e9 100644 --- a/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj +++ b/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj @@ -7,6 +7,13 @@ false true + + + $(MSBuildProjectDirectory)/hang-watchdog.runsettings diff --git a/tests/PlanViewer.Core.Tests/hang-watchdog.runsettings b/tests/PlanViewer.Core.Tests/hang-watchdog.runsettings new file mode 100644 index 0000000..141a25e --- /dev/null +++ b/tests/PlanViewer.Core.Tests/hang-watchdog.runsettings @@ -0,0 +1,30 @@ + + + + + 900000 + + From 28be5edb605a831ed5ab6e46d783ba9cafe5094c Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 11 Aug 2026 18:12:11 -0400 Subject: [PATCH 07/15] Address review: marshal handle lookup to the UI thread; pin the CLI refusal Two findings from the PR review: - ActiveWindowHandle is called by MSAL from whatever thread SqlClient's token acquisition runs on, while desktop.Windows is a UI-thread-owned collection that can be mutated mid-enumeration by a dialog opening or closing. Marshal the lookup through Dispatcher.UIThread (safe: no connection path blocks the UI thread on auth - every open in the app is async), and degrade to IntPtr.Zero if the dispatcher can't deliver, which is MSAL's normal no-handle failure rather than a new one. - The CLI's --auth entra refusal had no test. Pin it with a real call through BuildServerConnection, made deterministic by an internal test-only reset of the process-wide one-way registration flag (the registration tests do run first in this process, so a skip-if- contaminated guard would have skipped on exactly the platform where the branch is reachable). The two test classes share a collection so the global state can't flip mid-test. Also drop using directives already provided as global usings in both projects, which the IDE flags on every touch of these files. Co-Authored-By: Claude Fable 5 --- src/PlanViewer.App/App.axaml.cs | 22 +++++++++ .../Services/EntraInteractiveAuth.cs | 14 +++++- .../CliConnectionResolverTests.cs | 48 +++++++++++++++++++ .../EntraInteractiveAuthTests.cs | 6 ++- 4 files changed, 87 insertions(+), 3 deletions(-) create mode 100644 tests/PlanViewer.Core.Tests/CliConnectionResolverTests.cs diff --git a/src/PlanViewer.App/App.axaml.cs b/src/PlanViewer.App/App.axaml.cs index 5dd2a47..8b23a24 100644 --- a/src/PlanViewer.App/App.axaml.cs +++ b/src/PlanViewer.App/App.axaml.cs @@ -6,6 +6,7 @@ using Avalonia.Markup.Xaml; using Avalonia.Platform; using Avalonia.Platform.Storage; +using Avalonia.Threading; using System.Linq; using System.Threading.Tasks; using PlanViewer.App.Services; @@ -70,8 +71,29 @@ public override void OnFrameworkInitializationCompleted() /// Resolved per call rather than captured once: a window's platform handle is not valid until the /// window has been sourced, so a handle read at startup can be zero even though a real window exists a /// moment later. + /// + /// Marshaled to the UI thread: MSAL invokes this from whatever thread SqlClient's token acquisition + /// happens to run on, and desktop.Windows is a UI-thread-owned collection that the UI thread can + /// mutate (a dialog opening or closing) mid-enumeration. Blocking on is + /// safe here because no connection path blocks the UI thread on auth — every open in the app is async. If + /// the dispatcher can't deliver anyway (shutdown timing), Zero degrades to MSAL's normal no-handle + /// failure instead of throwing from inside the auth callback. /// private static IntPtr ActiveWindowHandle() + { + try + { + return Dispatcher.UIThread.CheckAccess() + ? ActiveWindowHandleOnUIThread() + : Dispatcher.UIThread.Invoke(ActiveWindowHandleOnUIThread); + } + catch + { + return IntPtr.Zero; + } + } + + private static IntPtr ActiveWindowHandleOnUIThread() { if (Current?.ApplicationLifetime is not IClassicDesktopStyleApplicationLifetime desktop) return IntPtr.Zero; diff --git a/src/PlanViewer.Core/Services/EntraInteractiveAuth.cs b/src/PlanViewer.Core/Services/EntraInteractiveAuth.cs index 317bec9..e800eb2 100644 --- a/src/PlanViewer.Core/Services/EntraInteractiveAuth.cs +++ b/src/PlanViewer.Core/Services/EntraInteractiveAuth.cs @@ -1,4 +1,3 @@ -using System; using Microsoft.Data.SqlClient; namespace PlanViewer.Core.Services; @@ -76,4 +75,17 @@ mobile and an HWND on Windows. Boxing the IntPtr is the intended usage. */ /// a clear message instead of letting MSAL produce that error. /// public static bool IsSupported => _registered || !OperatingSystem.IsWindows(); + + /* Test-only escape hatch for the one-way registration flag. Registration is deliberately irreversible + in production — SqlAuthenticationProvider offers no unregister — so a test that needs to observe the + never-registered state (the CLI refusal path) resets the flag rather than the provider. The provider + itself may stay registered with SqlClient; nothing in the test process opens interactive connections, + so that is inert. */ + internal static void ResetRegistrationForTests() + { + lock (Gate) + { + _registered = false; + } + } } diff --git a/tests/PlanViewer.Core.Tests/CliConnectionResolverTests.cs b/tests/PlanViewer.Core.Tests/CliConnectionResolverTests.cs new file mode 100644 index 0000000..9f37881 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/CliConnectionResolverTests.cs @@ -0,0 +1,48 @@ +using PlanViewer.Cli.Commands; +using PlanViewer.Core.Interfaces; +using PlanViewer.Core.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// Issue #425: the CLI accepts --auth entra but has no window to hand the WAM broker, so +/// must refuse up front with actionable guidance +/// instead of letting MSAL fail with 0xwindow_handle_required. This pins that refusal. +/// +/// Shares a collection with because both read or flip the +/// process-wide registration state behind ; running them in +/// parallel would let that state change between this test's skip check and its assertion. +/// +[Collection("EntraInteractiveAuth process-wide state")] +public class CliConnectionResolverTests +{ + [Fact] + public void BuildServerConnection_RefusesInteractiveEntraWhereItCannotWork() + { + /* Only reachable on Windows: off Windows IsSupported is permanently true (browser auth works + headless there) and the CLI correctly does not refuse. */ + Assert.SkipUnless(OperatingSystem.IsWindows(), "the refusal only exists where WAM does"); + + /* Registration state is process-wide and one-way, and the registration tests may have run earlier + in this process; reset the flag so this test observes the state the real CLI process is always + in — nothing ever registered. The shared collection keeps the registration tests from running + concurrently and re-flipping it mid-test. */ + EntraInteractiveAuth.ResetRegistrationForTests(); + + var ex = Assert.Throws(() => + CliConnectionResolver.BuildServerConnection("srv", "entra", trustCert: false, new NoCredentials())); + + Assert.Contains("headless", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + /* Minimal stand-in: the resolver only asks whether a credential exists, and the entra refusal must fire + before credentials ever matter. */ + private sealed class NoCredentials : ICredentialService + { + public bool SaveCredential(string serverId, string username, string password) => false; + public (string Username, string Password)? GetCredential(string serverId) => null; + public bool DeleteCredential(string serverId) => false; + public bool CredentialExists(string serverId) => false; + public bool UpdateCredential(string serverId, string username, string password) => false; + } +} diff --git a/tests/PlanViewer.Core.Tests/EntraInteractiveAuthTests.cs b/tests/PlanViewer.Core.Tests/EntraInteractiveAuthTests.cs index 4489d87..62ead1f 100644 --- a/tests/PlanViewer.Core.Tests/EntraInteractiveAuthTests.cs +++ b/tests/PlanViewer.Core.Tests/EntraInteractiveAuthTests.cs @@ -1,6 +1,4 @@ -using System; using PlanViewer.Core.Services; -using Xunit; namespace PlanViewer.Core.Tests; @@ -9,7 +7,11 @@ namespace PlanViewer.Core.Tests; /// through the WAM broker. These pin the parts that are verifiable without a tenant — the OS gating and the /// contract the windowless CLI depends on. Whether the picker actually authenticates can only be established /// against a real Entra tenant, which is what the reporter offered to do. +/// +/// Shares a collection with : registration here flips the +/// process-wide state that the CLI refusal test keys off, so the two classes must not run in parallel. /// +[Collection("EntraInteractiveAuth process-wide state")] public class EntraInteractiveAuthTests { [Fact] From f6f53c71d39543ce65eb6484c8f7973d9eee7474 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sun, 16 Aug 2026 02:30:00 +0200 Subject: [PATCH 08/15] Stop Robot Advice from crashing the app on a deep plan (#430) (#431) System.Text.Json defaults MaxDepth to 64 and throws past it. An operator tree nests once per operator and each level costs two JSON levels (the object, then the Children array), so a plan roughly 30 operators deep exhausts the default. The throw came out of an Avalonia click handler, which Avalonia does not guard, so the process died with no dialog and nothing the user could act on. The reported stack named the cause precisely: the path was $.Statements.OperatorTree.Children.Children...(30 deep)...NodeId - thirty CONSECUTIVE Children. That distinction decides the fix, because the exception message offers two causes ("either be due to a cycle or if the object depth is larger than the maximum allowed depth of 64") and they want opposite remedies. The serialized type is OperatorResult, which has Children and no parent link, so it is a tree and depth is the right read. Worth recording for whoever hits this next: the INTERNAL model it is mapped from, PlanNode, DOES carry a Parent back-reference that the parser populates (ShowPlanParser.RelOp.cs). Serializing that type directly would be a genuine cycle needing [JsonIgnore], not a bigger number. The ceiling is now one shared constant, because four places serialize this object and none of them could tell when they got it wrong: both Robot Advice entry points (QuerySessionControl and MainWindow build the payload independently), the MCP tool options, and the CLI's two option sets. Only the reported one crashes the process; the others returned an error where the caller expected a plan. 1024 is about 500 nested operators against the ~30 that used to fail, and it is deliberately paired with a caller-side catch at both UI sites: a serialization limit should never be able to take the app down, so the headroom is not the only thing standing between a deep plan and a crash. Tested: 137/137 in PlanViewer.Core.Tests, including a reproduction that fails on the old inline options and passes on the shared ones. App and CLI build with zero warnings. Co-authored-by: Claude Opus 5 (1M context) --- .../Controls/QuerySessionControl.Advice.cs | 16 +++- src/PlanViewer.App/MainWindow.PlanViewer.cs | 15 +++- src/PlanViewer.App/Mcp/McpHelpers.cs | 10 ++- src/PlanViewer.Cli/Commands/AnalyzeCommand.cs | 5 ++ src/PlanViewer.Core/Output/AnalysisJson.cs | 47 +++++++++++ .../AnalysisJsonDepthTests.cs | 84 +++++++++++++++++++ 6 files changed, 174 insertions(+), 3 deletions(-) create mode 100644 src/PlanViewer.Core/Output/AnalysisJson.cs create mode 100644 tests/PlanViewer.Core.Tests/AnalysisJsonDepthTests.cs diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.Advice.cs b/src/PlanViewer.App/Controls/QuerySessionControl.Advice.cs index f5c96f1..85f2e7d 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.Advice.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.Advice.cs @@ -49,7 +49,21 @@ private void RobotAdvice_Click(object? sender, RoutedEventArgs e) var analysis = GetCurrentAnalysis(); if (analysis == null) { SetStatus("No plan to analyze", autoClear: false); return; } - var json = JsonSerializer.Serialize(analysis, new JsonSerializerOptions { WriteIndented = true }); + string json; + try + { + json = JsonSerializer.Serialize(analysis, AnalysisJson.Indented); + } + catch (Exception ex) when (ex is JsonException or NotSupportedException) + { + /* #430: Avalonia does not guard click handlers, so anything thrown on this path takes the + process down with no dialog and nothing logged. AnalysisJson's depth ceiling makes this + unreachable for any plan seen in the field — this catch is here so that "unreachable" is + not the only thing standing between a deep plan and a silent crash. */ + SetStatus($"Could not build robot advice for this plan: {ex.Message}", autoClear: false); + return; + } + ShowAdviceWindow("Advice for Robots", json); } diff --git a/src/PlanViewer.App/MainWindow.PlanViewer.cs b/src/PlanViewer.App/MainWindow.PlanViewer.cs index b1ab639..502543e 100644 --- a/src/PlanViewer.App/MainWindow.PlanViewer.cs +++ b/src/PlanViewer.App/MainWindow.PlanViewer.cs @@ -63,7 +63,20 @@ private DockPanel CreatePlanTabContent(PlanViewerControl viewer) { if (viewer.CurrentPlan == null) return; var analysis = ResultMapper.Map(viewer.CurrentPlan, "file", viewer.Metadata); - var json = JsonSerializer.Serialize(analysis, new JsonSerializerOptions { WriteIndented = true }); + string json; + try + { + json = JsonSerializer.Serialize(analysis, AnalysisJson.Indented); + } + catch (Exception ex) when (ex is JsonException or NotSupportedException) + { + /* #430: the same unguarded-click-handler crash as QuerySessionControl's Robot Advice + button — this entry point builds the payload independently, so it needed the same + depth ceiling and the same guard. */ + ShowError($"Could not build robot advice for this plan: {ex.Message}"); + return; + } + ShowAdviceWindow("Advice for Robots", json); }; diff --git a/src/PlanViewer.App/Mcp/McpHelpers.cs b/src/PlanViewer.App/Mcp/McpHelpers.cs index c7f9906..fa871c5 100644 --- a/src/PlanViewer.App/Mcp/McpHelpers.cs +++ b/src/PlanViewer.App/Mcp/McpHelpers.cs @@ -1,5 +1,6 @@ using System; using System.Text.Json; +using PlanViewer.Core.Output; namespace PlanViewer.App.Mcp; @@ -7,7 +8,14 @@ internal static class McpHelpers { public const int MaxTop = 100; - public static readonly JsonSerializerOptions JsonOptions = new() { WriteIndented = true }; + /* #430: MaxDepth, because several of these tools return an analysis carrying an OperatorTree and the + default ceiling of 64 is roughly 30 nested operators. Unlike the UI buttons an MCP tool failing here + does not crash the process, but it does return an error where the caller expected a plan. */ + public static readonly JsonSerializerOptions JsonOptions = new() + { + WriteIndented = true, + MaxDepth = AnalysisJson.MaxDepth, + }; public static string? Truncate(string? value, int maxLength) { diff --git a/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs b/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs index e517dae..ac28cff 100644 --- a/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs +++ b/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs @@ -11,14 +11,19 @@ namespace PlanViewer.Cli.Commands; public static class AnalyzeCommand { + /* #430: MaxDepth on both, because the default ceiling of 64 is roughly 30 nested operators and + `analyze --json` on a large plan hit it. The CLI surfaces this as a non-zero exit rather than a + crash, but a plan too deep to print is the same defect wearing a different coat. */ private static readonly JsonSerializerOptions JsonOptions = new() { WriteIndented = true, + MaxDepth = AnalysisJson.MaxDepth, DefaultIgnoreCondition = System.Text.Json.Serialization.JsonIgnoreCondition.WhenWritingNull }; private static readonly JsonSerializerOptions CompactJsonOptions = new() { + MaxDepth = AnalysisJson.MaxDepth, DefaultIgnoreCondition = System.Text.Json.Serialization.JsonIgnoreCondition.WhenWritingNull }; diff --git a/src/PlanViewer.Core/Output/AnalysisJson.cs b/src/PlanViewer.Core/Output/AnalysisJson.cs new file mode 100644 index 0000000..c55bb48 --- /dev/null +++ b/src/PlanViewer.Core/Output/AnalysisJson.cs @@ -0,0 +1,47 @@ +using System.Text.Json; + +namespace PlanViewer.Core.Output; + +/// +/// How deep a serializer must be willing to go to write an , in ONE place +/// because every writer of this object needs the same answer and none of them can tell when they got it +/// wrong. +/// +/// Why this exists (#430). defaults MaxDepth to 64 and +/// throws when a graph exceeds it. nests once per operator, and each +/// level costs two JSON levels (the object, then the array), so a plan roughly 30 operators deep exhausts +/// the default. Clicking "Advice for Robots" on a large plan therefore threw a JsonException out of +/// an Avalonia click handler, which Avalonia does not guard, and the process died with no dialog and no +/// log entry the user could act on. +/// +/// Why raising it is safe here, which is the part worth checking before copying this. The +/// exception message reads "A possible object cycle was detected. This can either be due to a cycle or if +/// the object depth is larger than the maximum allowed depth of 64", and those two causes want opposite +/// fixes: raising the limit on a genuine cycle just moves the crash. The serialized graph is +/// , which has Children and no parent link, so it is a tree and the +/// depth reading is the right one. Note that the INTERNAL model this is mapped from, +/// PlanViewer.Core.Models.PlanNode, DOES carry a Parent back-reference that the parser +/// populates — so if anything ever serializes PlanNode directly, that is a real cycle and needs +/// [JsonIgnore] on Parent rather than a bigger number. +/// +/// The number is headroom, not a guarantee, and it is deliberately paired with a caller-side +/// guard. 1024 covers about 500 nested operators against the ~30 that used to fail, which is far past any +/// plan seen in the field; a plan deeper than that now surfaces as a message instead of terminating the +/// process, because a serialization limit should never be able to take the app down. +/// +public static class AnalysisJson +{ + /// Depth ceiling for every serializer that writes an . + public const int MaxDepth = 1024; + + /// + /// The indented options the UI writes advice with. Matches what the call sites built inline before + /// #430 — WriteIndented only — so the emitted JSON is unchanged apart from no longer failing + /// partway down a deep tree. + /// + public static readonly JsonSerializerOptions Indented = new() + { + WriteIndented = true, + MaxDepth = MaxDepth, + }; +} diff --git a/tests/PlanViewer.Core.Tests/AnalysisJsonDepthTests.cs b/tests/PlanViewer.Core.Tests/AnalysisJsonDepthTests.cs new file mode 100644 index 0000000..6336f8c --- /dev/null +++ b/tests/PlanViewer.Core.Tests/AnalysisJsonDepthTests.cs @@ -0,0 +1,84 @@ +using System.Text.Json; +using PlanViewer.Core.Output; + +namespace PlanViewer.Core.Tests; + +/// +/// #430: "Robot Advice" crashed the app on a large plan. +/// +/// System.Text.Json defaults MaxDepth to 64 and throws past it. An operator tree nests once per +/// operator and each level costs two JSON levels (the object, then the Children array), so a plan +/// roughly 30 operators deep exhausts the default. The throw came out of an Avalonia click handler, +/// which Avalonia does not guard, so the process died with no dialog and nothing actionable logged. +/// +/// The reported stack named the cause precisely: the path was +/// $.Statements.OperatorTree.Children.Children...(30 deep)...NodeId — thirty CONSECUTIVE Children. +/// That matters, because the exception message offers two causes ("either be due to a cycle or if the +/// object depth is larger than the maximum allowed depth of 64") and they want opposite fixes. A +/// consecutive descent is depth. A cycle would have alternated, and raising the ceiling on a cycle +/// only moves the crash. +/// +public class AnalysisJsonDepthTests +{ + /// An analysis whose statement carries a single chain of nested operators. + private static AnalysisResult WithOperatorChain(int operators) + { + var node = new OperatorResult { PhysicalOp = "Leaf" }; + for (var i = 1; i < operators; i++) + node = new OperatorResult { PhysicalOp = "Nested", Children = { node } }; + + return new AnalysisResult { Statements = { new StatementResult { OperatorTree = node } } }; + } + + /// + /// The defect, reproduced against the options the call sites used to build inline. Without this the + /// fix below is unfalsifiable — a passing serialize proves nothing if nothing ever failed. + /// + [Fact] + public void TheOldInlineOptionsFailOnADeepPlan() + { + var exception = Assert.Throws(() => + JsonSerializer.Serialize(WithOperatorChain(60), new JsonSerializerOptions { WriteIndented = true })); + + Assert.Contains("depth", exception.Message, System.StringComparison.OrdinalIgnoreCase); + } + + /// The fix: the shared options carry the plan that used to crash the app. + [Theory] + [InlineData(60)] + [InlineData(200)] + [InlineData(400)] + public void TheSharedOptionsCarryADeepPlan(int operators) + { + var json = JsonSerializer.Serialize(WithOperatorChain(operators), AnalysisJson.Indented); + + Assert.Contains("\"Leaf\"", json, System.StringComparison.Ordinal); + /* Every level actually made it out, rather than the serializer stopping quietly partway. */ + Assert.Equal(operators - 1, System.Text.RegularExpressions.Regex.Matches(json, "\"Nested\"").Count); + } + + /// + /// The ceiling is headroom, not a guarantee — which is exactly why the two UI call sites also catch. + /// A plan past it must still fail as an exception the caller can turn into a message, not as + /// silently truncated JSON that reads like a complete plan. + /// + [Fact] + public void PastTheCeilingItStillThrowsRatherThanTruncating() + { + Assert.Throws(() => + JsonSerializer.Serialize(WithOperatorChain(AnalysisJson.MaxDepth + 10), AnalysisJson.Indented)); + } + + /// + /// Pins the headroom itself. 1024 is about 500 nested operators against the ~30 that used to fail; + /// a future edit dropping it back toward the default would re-open #430 for large plans only, which + /// is the shape of bug that reaches users rather than tests. + /// + [Fact] + public void TheCeilingIsFarAboveAnyRealPlan() + { + Assert.Equal(1024, AnalysisJson.MaxDepth); + Assert.Equal(AnalysisJson.MaxDepth, AnalysisJson.Indented.MaxDepth); + Assert.True(AnalysisJson.Indented.WriteIndented, "advice output is read by people as well as models"); + } +} From 344e3feab6995bb71f029314922a3d702cea0d40 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 17 Aug 2026 10:19:06 +0000 Subject: [PATCH 09/15] deps: Bump the patch-and-minor group with 11 updates Bumps Avalonia from 11.3.19 to 11.3.20 Bumps Avalonia.Desktop from 11.3.19 to 11.3.20 Bumps Avalonia.Fonts.Inter from 11.3.19 to 11.3.20 Bumps Avalonia.Themes.Fluent from 11.3.19 to 11.3.20 Bumps Microsoft.AspNetCore.Components.WebAssembly from 10.0.10 to 10.0.11 Bumps Microsoft.AspNetCore.Components.WebAssembly.DevServer from 10.0.10 to 10.0.11 Bumps Microsoft.Data.Sqlite from 10.0.10 to 10.0.11 Bumps Microsoft.NET.Test.Sdk from 18.8.1 to 18.9.0 Bumps ModelContextProtocol from 2.1.0 to 2.2.0 Bumps ModelContextProtocol.AspNetCore from 2.1.0 to 2.2.0 Bumps System.CommandLine from 2.0.10 to 2.0.11 --- updated-dependencies: - dependency-name: Microsoft.Data.Sqlite dependency-version: 10.0.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Desktop dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Fonts.Inter dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Themes.Fluent dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: ModelContextProtocol dependency-version: 2.2.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: ModelContextProtocol.AspNetCore dependency-version: 2.2.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: System.CommandLine dependency-version: 2.0.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Microsoft.AspNetCore.Components.WebAssembly dependency-version: 10.0.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Microsoft.AspNetCore.Components.WebAssembly.DevServer dependency-version: 10.0.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Desktop dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Fonts.Inter dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Themes.Fluent dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Microsoft.NET.Test.Sdk dependency-version: 18.9.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: ModelContextProtocol dependency-version: 2.2.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: ModelContextProtocol.AspNetCore dependency-version: 2.2.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: System.CommandLine dependency-version: 2.0.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor ... Signed-off-by: dependabot[bot] --- server/PlanShare/PlanShare.csproj | 2 +- src/PlanViewer.App/PlanViewer.App.csproj | 12 ++++++------ src/PlanViewer.Cli/PlanViewer.Cli.csproj | 2 +- src/PlanViewer.Web/PlanViewer.Web.csproj | 4 ++-- .../PlanViewer.Core.Tests.csproj | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/server/PlanShare/PlanShare.csproj b/server/PlanShare/PlanShare.csproj index a268903..77afe95 100644 --- a/server/PlanShare/PlanShare.csproj +++ b/server/PlanShare/PlanShare.csproj @@ -7,7 +7,7 @@ - + - - - + + + - - + + diff --git a/src/PlanViewer.Cli/PlanViewer.Cli.csproj b/src/PlanViewer.Cli/PlanViewer.Cli.csproj index 54e3a34..de7552b 100644 --- a/src/PlanViewer.Cli/PlanViewer.Cli.csproj +++ b/src/PlanViewer.Cli/PlanViewer.Cli.csproj @@ -20,7 +20,7 @@ - + diff --git a/src/PlanViewer.Web/PlanViewer.Web.csproj b/src/PlanViewer.Web/PlanViewer.Web.csproj index dc3df01..dfc14a6 100644 --- a/src/PlanViewer.Web/PlanViewer.Web.csproj +++ b/src/PlanViewer.Web/PlanViewer.Web.csproj @@ -9,8 +9,8 @@ - - + + diff --git a/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj b/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj index 38c85e9..81a19f4 100644 --- a/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj +++ b/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj @@ -18,7 +18,7 @@ - + From 1c17b06923bca9f307d430fa31a60284be3516fa Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 21 Aug 2026 00:55:33 +0200 Subject: [PATCH 10/15] Only call a conversion non-SARGable when it converts the column (#436) (#437) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rule 12 flagged any predicate whose text contained CONVERT_IMPLICIT, and told the user it "prevents an index seek". Which side the conversion is on decides whether that is true, and the rule never looked. Data type precedence picks the side, and SQL Server converts the LOWER-precedence one. A numeric(18,0) column compared to an integer parameter converts the PARAMETER up: [db].[dbo].[t].[UNIV_SEZIONI]=CONVERT_IMPLICIT(numeric(18,0),[@0],0) The column is untouched, so it is still seekable, and SQL Server agrees: it emits no PlanAffectingConvert warning on that plan, which is why SSMS shows no warning icon while we showed one. The damaging shape is the mirror image, CONVERT_IMPLICIT(nvarchar(40),[db].[dbo].[t].[DisplayName],0)=[@d], where every row has to be converted before it can be compared. Reproduced rather than reasoned about, because the seekability claim is the whole question. Captured two actual plans of the reporter's repro from SQL Server 2025 under PARAMETERIZATION FORCED. Without an index on the search column it is a scan; ADD an index on that same column and SQL Server produces an Index Seek whose seek predicate is CONVERT_IMPLICIT(numeric(18,0),[@0],0) — it seeks straight through the conversion. That settles it: the conversion was never what prevented the seek. The scan fixture is committed as the test plan. So the check reads what is INSIDE the conversion rather than splitting on the comparison operator: the first argument is the target type and carries no brackets, so a bracket-qualified column reference in the remainder is the conversion input. Balanced-paren extraction is needed because the target type has parentheses of its own — numeric(18,0), varchar(50) — so the first ')' is not the end of the argument list. Second change, and it is load-bearing rather than tidying: the function-on-column check now walks every regex match instead of only the first. A parameter-side conversion now falls through to that check and is skipped there as benign, so with only the first match a conversion sitting to the LEFT of a genuine function-on-column would hide it — trading the fixed false positive for a new false negative. Verified by reverting just that hunk: Rule12d then fails with "Collection was empty". Behavior change, stated plainly: plans carrying a parameter-side conversion on a scan lose their "Non-SARGable Predicate" warning. The reporter's plan now reports Rule 11 instead — "scan with residual predicate, 50 of 50,000 rows survived filtering, check that you have appropriate indexes" — which is the true and actionable read of that plan. Nothing silently disappears. The existing corpus is untouched: regenerating WarningBaseline.txt across all 38 committed plans changes nothing except the two lines each for the two new fixtures. No committed plan was relying on the old verdict. Tested: 71/71 in the analyzer classes (PlanAnalyzerTests, WarningCharacterizationTests, PlanAnalysisPipelineTests, OperatorSelfTimeTests), plus 130 more across the other classes. Both new tests fail on the unfixed rule — Rule12c with "Collection was not empty", Rule12d with "Filter not matched". dotnet build is clean, 0 warnings. Not fixed here and not caused here: a full `dotnet test` does not finish on macOS ARM64 — it wedges inside McpPlanPathPolicyTests with every thread blocked. It is NOT the GC-suspension livelock from 5f84116 (no SuspendEE, 0% CPU, not spinning), --blame-hang-timeout does not fire on it, and it reproduces on unmodified dev. CI is ubuntu-latest, which is why it is invisible there. Co-authored-by: Claude Opus 5 (1M context) --- .../Services/PlanAnalyzer.Detection.cs | 77 +++++++++++++++++-- src/PlanViewer.Core/Services/PlanAnalyzer.cs | 11 +++ .../PlanAnalyzerTests.cs | 41 ++++++++++ ...ert_implicit_masking_function_plan.sqlplan | 31 ++++++++ ...nvert_implicit_parameter_side_plan.sqlplan | 2 + .../PlanViewer.Core.Tests/WarningBaseline.txt | 6 ++ 6 files changed, 160 insertions(+), 8 deletions(-) create mode 100644 tests/PlanViewer.Core.Tests/Plans/convert_implicit_masking_function_plan.sqlplan create mode 100644 tests/PlanViewer.Core.Tests/Plans/convert_implicit_parameter_side_plan.sqlplan diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs index 6b79bf4..29086f6 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs @@ -157,8 +157,9 @@ private static bool IsScanOperator(PlanNode node) if (CaseInPredicateRegex.IsMatch(predicate)) return "CASE expression in predicate"; - // CONVERT_IMPLICIT — most common non-SARGable pattern - if (predicate.Contains("CONVERT_IMPLICIT", StringComparison.OrdinalIgnoreCase)) + // CONVERT_IMPLICIT — most common non-SARGable pattern, but only when it converts the + // COLUMN. Converting the parameter up to the column's type costs nothing (#436). + if (ConvertImplicitWrapsColumn(predicate)) return "Implicit conversion (CONVERT_IMPLICIT)"; // ISNULL / COALESCE wrapping column @@ -168,8 +169,10 @@ private static bool IsScanOperator(PlanNode node) // Common function calls on columns — but only if the function wraps a column, // not a parameter/variable. Split on comparison operators to check which side // the function is on. Predicate format: [db].[schema].[table].[col]>func(...) - var funcMatch = FunctionInPredicateRegex.Match(predicate); - if (funcMatch.Success) + // Every match, not just the first: a parameter-side CONVERT_IMPLICIT now falls through to + // here, and it is skipped below. Taking only the first match would let a benign conversion + // sitting to the left of a real function-on-column hide it (#436). + foreach (Match funcMatch in FunctionInPredicateRegex.Matches(predicate)) { var funcName = funcMatch.Groups[1].Value.ToUpperInvariant(); if (funcName != "CONVERT_IMPLICIT" && IsFunctionOnColumnSide(predicate, funcMatch)) @@ -183,6 +186,65 @@ private static bool IsScanOperator(PlanNode node) return null; } + /// + /// Checks whether any CONVERT_IMPLICIT in a predicate converts a COLUMN, which is the only + /// version of it that costs a seek. + /// + /// Why this is not just "contains CONVERT_IMPLICIT" (#436). Data type precedence + /// decides which side SQL Server converts, and it converts the LOWER-precedence side. Comparing a + /// numeric(18,0) column to an int parameter converts the parameter UP: + /// [db].[dbo].[t].[col]=CONVERT_IMPLICIT(numeric(18,0),[@0],0). The column is untouched and + /// still seekable — SQL Server will seek straight through that predicate given an index, and it + /// raises no PlanAffectingConvert warning of its own. The damaging shape is the mirror image, + /// CONVERT_IMPLICIT(nvarchar(40),[db].[dbo].[t].[col],0)=[@d], where the conversion wraps + /// the column and every row has to be converted before it can be compared. + /// + /// So the question is not whether a conversion is present but what is inside it, which is + /// why this reads the CONVERT_IMPLICIT argument list rather than splitting on the comparison + /// operator the way does. The first argument is the target + /// type and carries no brackets; a column reference in the remainder is the conversion input. + /// + private static bool ConvertImplicitWrapsColumn(string predicate) + { + foreach (Match match in ConvertImplicitRegex.Matches(predicate)) + { + // The regex ends at the opening paren, so its last character is where the args start. + var arguments = ExtractBalancedArguments(predicate, match.Index + match.Length - 1); + + // Unparseable means we cannot tell what is being converted. Assume the worst, matching + // IsFunctionOnColumnSide, rather than silently dropping a real conversion. + if (arguments == null || ColumnReferenceRegex.IsMatch(arguments)) + return true; + } + + return false; + } + + /// + /// Returns the text between the parenthesis at and its match, + /// or null if the parentheses do not balance. Needed because the target type of a conversion can + /// carry its own parentheses — numeric(18,0), varchar(50) — so the first ')' is not the end. + /// + private static string? ExtractBalancedArguments(string text, int openParenIndex) + { + var depth = 0; + for (var i = openParenIndex; i < text.Length; i++) + { + if (text[i] == '(') + { + depth++; + } + else if (text[i] == ')') + { + depth--; + if (depth == 0) + return text[(openParenIndex + 1)..i]; + } + } + + return null; + } + /// /// Checks whether a function call in a predicate is on the column side of the comparison. /// Predicate ScalarStrings look like: [db].[schema].[table].[col]>dateadd(day,(0),[@var]) @@ -207,10 +269,9 @@ private static bool IsFunctionOnColumnSide(string predicate, Match funcMatch) ? predicate[..compPos] : predicate[(compPos + compMatch.Length)..]; - // Column references are multi-part bracket-qualified: [schema].[table].[column] - // Variables are [@var] or [@var] — single bracket pair with @ prefix. - // Match [identifier].[identifier] (at least two dotted parts) to distinguish columns. - return Regex.IsMatch(side, @"\[[^\]@]+\]\.\["); + // Same column-vs-variable distinction ConvertImplicitWrapsColumn needs, so it shares the + // one regex rather than keeping a second copy of the pattern in sync by hand. + return ColumnReferenceRegex.IsMatch(side); } /// diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.cs index 7fea227..eb6a3f6 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.cs @@ -24,6 +24,17 @@ public static partial class PlanAnalyzer @"\bCASE\s+(WHEN\b|$)", RegexOptions.IgnoreCase | RegexOptions.Compiled); + private static readonly Regex ConvertImplicitRegex = new( + @"\bCONVERT_IMPLICIT\s*\(", + RegexOptions.IgnoreCase | RegexOptions.Compiled); + + // A column reference in a ScalarString is multi-part bracket-qualified ([schema].[table]). + // A variable is a single bracket pair with an @ prefix ([@0]), so excluding @ from the first + // part is what separates the two. + private static readonly Regex ColumnReferenceRegex = new( + @"\[[^\]@]+\]\.\[", + RegexOptions.Compiled); + public static void Analyze(ParsedPlan plan, AnalyzerConfig? config = null, ServerMetadata? serverMetadata = null) => AnalyzeCancellable(plan, config, serverMetadata, CancellationToken.None); diff --git a/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs b/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs index ff60299..393842c 100644 --- a/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs +++ b/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs @@ -264,6 +264,47 @@ public void Rule12a_NonSargable_ConvertImplicit() Assert.Contains(warnings, w => w.Message.Contains("CONVERT_IMPLICIT")); } + // --------------------------------------------------------------- + // Rule 12: Non-SARGable Predicate — CONVERT_IMPLICIT on the parameter + // --------------------------------------------------------------- + + /// + /// #436: a conversion is only non-SARGable when it converts the COLUMN. Data type precedence + /// decides which side SQL Server converts, and it converts the lower-precedence one — so a + /// numeric(18,0) column compared to an integer parameter converts the PARAMETER up, leaving the + /// column seekable. SQL Server agrees: it raises no PlanAffectingConvert warning on this plan, + /// which is why SSMS shows no warning icon on it either. + /// + /// The fixture is a real SQL Server 2025 actual plan of the reporter's repro, captured under + /// PARAMETERIZATION FORCED (hence [@0]) with no index on the search column. Rule 11 still + /// reports the scan and its residual predicate — that part is true, and it is the actionable + /// half. What must not appear is the claim that a conversion prevented a seek. + /// + [Fact] + public void Rule12c_NonSargable_ConvertImplicitOnParameter_NotFlagged() + { + var plan = PlanTestHelper.LoadAndAnalyze("convert_implicit_parameter_side_plan.sqlplan"); + + Assert.Empty(PlanTestHelper.WarningsOfType(plan, "Non-SARGable Predicate")); + Assert.NotEmpty(PlanTestHelper.WarningsOfType(plan, "Scan With Predicate")); + } + + /// + /// #436: the parameter-side conversion above now falls through to the function-on-column check, + /// and it sorts before the function in the predicate text. Reading only the FIRST function match + /// would find CONVERT_IMPLICIT, skip it as benign, and never look at the datepart() sitting on + /// the column — turning a fixed false positive into a new false negative. + /// + [Fact] + public void Rule12d_NonSargable_BenignConvertDoesNotMaskFunctionOnColumn() + { + var plan = PlanTestHelper.LoadAndAnalyze("convert_implicit_masking_function_plan.sqlplan"); + var warnings = PlanTestHelper.WarningsOfType(plan, "Non-SARGable Predicate"); + + Assert.NotEmpty(warnings); + Assert.Contains(warnings, w => w.Message.Contains("DATEPART")); + } + // --------------------------------------------------------------- // Rule 12: Non-SARGable Predicate — Function Call // --------------------------------------------------------------- diff --git a/tests/PlanViewer.Core.Tests/Plans/convert_implicit_masking_function_plan.sqlplan b/tests/PlanViewer.Core.Tests/Plans/convert_implicit_masking_function_plan.sqlplan new file mode 100644 index 0000000..40db85f --- /dev/null +++ b/tests/PlanViewer.Core.Tests/Plans/convert_implicit_masking_function_plan.sqlplan @@ -0,0 +1,31 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/PlanViewer.Core.Tests/Plans/convert_implicit_parameter_side_plan.sqlplan b/tests/PlanViewer.Core.Tests/Plans/convert_implicit_parameter_side_plan.sqlplan new file mode 100644 index 0000000..7ad9789 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/Plans/convert_implicit_parameter_side_plan.sqlplan @@ -0,0 +1,2 @@ + + diff --git a/tests/PlanViewer.Core.Tests/WarningBaseline.txt b/tests/PlanViewer.Core.Tests/WarningBaseline.txt index 322cf24..05cef6f 100644 --- a/tests/PlanViewer.Core.Tests/WarningBaseline.txt +++ b/tests/PlanViewer.Core.Tests/WarningBaseline.txt @@ -7,6 +7,12 @@ Non-SARGable Predicate | Warning | CASE expression in a predicate prevents an in ### compile_memory_exceeded_plan.sqlplan Compile Memory Exceeded | Critical | Optimization was aborted early because the compile memory limit was exceeded. The plan is likely suboptimal. Simplify the query by breaking it into smaller steps using #temp tables. +### convert_implicit_masking_function_plan.sqlplan +Non-SARGable Predicate | Warning | Function call (DATEPART) on column prevents an index seek. Remove the function from the column side — apply it to the parameter instead, or create a computed column with the expression and index that.\nPredicate: CONVERT_IMPLICIT(int,[@0],0)=datepart(year,[TestDB].[dbo].[Orders].[OrderDate]) + +### convert_implicit_parameter_side_plan.sqlplan +Scan With Predicate | Critical | Scan with residual predicate — SQL Server is reading every row and filtering after the fact. This scan is 100% of the plan cost. This scan took 100% of elapsed time. Only 0.100% of rows survived filtering (50 of 50,000). Check that you have appropriate indexes.\nPredicate: [TestDB].[dbo].[IB03_GARANZ].[UNIV_SEZIONI] as [IB03].[UNIV_SEZIONI]=CONVERT_IMPLICIT(numeric(18,0),[@0],0) + ### convert_implicit_plan.sqlplan Implicit Conversion | Critical | Seek Plan: CONVERT_IMPLICIT(nvarchar(40),[ub].[DisplayName],0)=[@d] High Compile CPU | Warning | Query took 1,000ms of CPU just to compile a plan (before any data was read). Simplify the query by breaking it into smaller steps using #temp tables. From 718b2caedac93f0218add19d95ce79913ecdf4a9 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 21 Aug 2026 00:55:35 +0200 Subject: [PATCH 11/15] Give querystore the depth ceiling that analyze got (#430) (#438) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #430 was fixed by making the depth ceiling a shared constant and referencing it from every serializer that writes an AnalysisResult. That enumeration missed two: QueryStoreCommand builds its own JsonOptions and CompactJsonOptions, identical in shape to AnalyzeCommand's, and neither ever got MaxDepth. So `planview querystore` has kept failing on any plan deeper than roughly 30 operators for as long as `analyze` has been fine. It fails quietly, which is why nobody has reported it. The per-plan try/catch in the sweep loop turns the JsonException into one "ERROR: ..." row in summary.txt and moves to the next plan, so a Query Store sweep over a few hundred plans silently analyzes all but the deep ones. Query Store batch analysis is exactly where the deep plans are. The interesting part is WHY it was missed, because that decides the fix. A shared constant only helps the call sites that remember to reference it, and the options themselves were still duplicated per command — so editing AnalyzeCommand's two sets to add MaxDepth did nothing for the identical pair one file over, and nothing anywhere said they were supposed to match. The options now live on AnalysisJson next to the constant, as IndentedWithoutNulls and CompactWithoutNulls, and both commands point at them. There is no longer a copy to forget. Values are unchanged for analyze: same WriteIndented, same JsonIgnoreCondition.WhenWritingNull, same ceiling. Only querystore's behavior changes, and only from "fails on deep plans" to "does not". The test walks the options instead of trusting the call sites, since trusting them is what went wrong: every static JsonSerializerOptions on a *Command type in PlanViewer.Cli must carry the ceiling. Verified by reverting just the QueryStoreCommand hunk, at which point it fails naming both offenders — "QueryStoreCommand.JsonOptions (MaxDepth 0), QueryStoreCommand.CompactJsonOptions (MaxDepth 0)". A new command that rolls its own now fails here rather than in someone's Query Store sweep. Tested: 82 tests across AnalysisJsonDepthTests and the CLI/analysis classes, 0 failures. dotnet build clean, 0 warnings. Co-authored-by: Claude Opus 5 (1M context) --- src/PlanViewer.Cli/Commands/AnalyzeCommand.cs | 19 +++-------- .../Commands/QueryStoreCommand.cs | 14 +++----- src/PlanViewer.Core/Output/AnalysisJson.cs | 26 ++++++++++++++ .../AnalysisJsonDepthTests.cs | 34 +++++++++++++++++++ 4 files changed, 70 insertions(+), 23 deletions(-) diff --git a/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs b/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs index ac28cff..d670e52 100644 --- a/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs +++ b/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs @@ -11,21 +11,12 @@ namespace PlanViewer.Cli.Commands; public static class AnalyzeCommand { - /* #430: MaxDepth on both, because the default ceiling of 64 is roughly 30 nested operators and - `analyze --json` on a large plan hit it. The CLI surfaces this as a non-zero exit rather than a - crash, but a plan too deep to print is the same defect wearing a different coat. */ - private static readonly JsonSerializerOptions JsonOptions = new() - { - WriteIndented = true, - MaxDepth = AnalysisJson.MaxDepth, - DefaultIgnoreCondition = System.Text.Json.Serialization.JsonIgnoreCondition.WhenWritingNull - }; + /* #430: the depth ceiling and the null handling both live on AnalysisJson now. They used to be + built here, which is how the identical pair in QueryStoreCommand got missed when #430 was + fixed — see AnalysisJson.IndentedWithoutNulls. */ + private static readonly JsonSerializerOptions JsonOptions = AnalysisJson.IndentedWithoutNulls; - private static readonly JsonSerializerOptions CompactJsonOptions = new() - { - MaxDepth = AnalysisJson.MaxDepth, - DefaultIgnoreCondition = System.Text.Json.Serialization.JsonIgnoreCondition.WhenWritingNull - }; + private static readonly JsonSerializerOptions CompactJsonOptions = AnalysisJson.CompactWithoutNulls; public static Command Create(ICredentialService? credentialService = null) { diff --git a/src/PlanViewer.Cli/Commands/QueryStoreCommand.cs b/src/PlanViewer.Cli/Commands/QueryStoreCommand.cs index 7e8adfb..010ad9c 100644 --- a/src/PlanViewer.Cli/Commands/QueryStoreCommand.cs +++ b/src/PlanViewer.Cli/Commands/QueryStoreCommand.cs @@ -10,16 +10,12 @@ namespace PlanViewer.Cli.Commands; public static class QueryStoreCommand { - private static readonly JsonSerializerOptions JsonOptions = new() - { - WriteIndented = true, - DefaultIgnoreCondition = System.Text.Json.Serialization.JsonIgnoreCondition.WhenWritingNull - }; + /* #430: these two were built inline and never got the depth ceiling, so `querystore` still + failed on a plan deeper than ~30 operators long after the crash was "fixed" — one ERROR row + in summary.txt per deep plan, which is quieter than the crash and no more correct. */ + private static readonly JsonSerializerOptions JsonOptions = AnalysisJson.IndentedWithoutNulls; - private static readonly JsonSerializerOptions CompactJsonOptions = new() - { - DefaultIgnoreCondition = System.Text.Json.Serialization.JsonIgnoreCondition.WhenWritingNull - }; + private static readonly JsonSerializerOptions CompactJsonOptions = AnalysisJson.CompactWithoutNulls; public static Command Create(ICredentialService? credentialService = null) { diff --git a/src/PlanViewer.Core/Output/AnalysisJson.cs b/src/PlanViewer.Core/Output/AnalysisJson.cs index c55bb48..5361d21 100644 --- a/src/PlanViewer.Core/Output/AnalysisJson.cs +++ b/src/PlanViewer.Core/Output/AnalysisJson.cs @@ -1,4 +1,5 @@ using System.Text.Json; +using System.Text.Json.Serialization; namespace PlanViewer.Core.Output; @@ -44,4 +45,29 @@ public static class AnalysisJson WriteIndented = true, MaxDepth = MaxDepth, }; + + /// + /// What the CLI writes analysis files with — same as plus dropping nulls, + /// which is what keeps `analyze --json` output readable. + /// + /// These live here rather than on the commands because the commands each built their own + /// copy, and that is precisely how querystore was left behind when #430 was fixed: the + /// depth ceiling was made a shared constant, but the OPTIONS were still duplicated, so adding + /// MaxDepth to the two sets in AnalyzeCommand silently did nothing for the identical pair + /// in QueryStoreCommand. A shared constant only helps the call sites that remember to reference + /// it. Sharing the options instead is what makes the next command unable to get it wrong. + /// + public static readonly JsonSerializerOptions IndentedWithoutNulls = new() + { + WriteIndented = true, + MaxDepth = MaxDepth, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, + }; + + /// The unindented counterpart to , for --compact. + public static readonly JsonSerializerOptions CompactWithoutNulls = new() + { + MaxDepth = MaxDepth, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, + }; } diff --git a/tests/PlanViewer.Core.Tests/AnalysisJsonDepthTests.cs b/tests/PlanViewer.Core.Tests/AnalysisJsonDepthTests.cs index 6336f8c..b786d49 100644 --- a/tests/PlanViewer.Core.Tests/AnalysisJsonDepthTests.cs +++ b/tests/PlanViewer.Core.Tests/AnalysisJsonDepthTests.cs @@ -1,3 +1,4 @@ +using System.Linq; using System.Text.Json; using PlanViewer.Core.Output; @@ -69,6 +70,39 @@ public void PastTheCeilingItStillThrowsRatherThanTruncating() JsonSerializer.Serialize(WithOperatorChain(AnalysisJson.MaxDepth + 10), AnalysisJson.Indented)); } + /// + /// The miss that #430's original fix left behind, and the reason the OPTIONS are shared and not + /// just the constant. + /// + /// Every CLI command that writes an analysis built its own JsonSerializerOptions. Making + /// MaxDepth a shared constant only fixed the sets that were edited to reference it — AnalyzeCommand's + /// two — while the identical pair in QueryStoreCommand kept the default ceiling of 64 and kept + /// failing on any plan deeper than ~30 operators. It failed quietly there: the per-plan catch turns + /// it into one "ERROR" row in summary.txt instead of an analysis, which is exactly the kind of + /// wrong-but-not-loud result nobody files a bug about. + /// + /// So this walks the options rather than trusting the call sites, and a new command that + /// rolls its own will fail here rather than in someone's Query Store sweep. + /// + [Fact] + public void EveryCliCommandWritesAnalysesWithTheCeiling() + { + var offenders = + (from type in typeof(PlanViewer.Cli.Commands.AnalyzeCommand).Assembly.GetTypes() + where type.Name.EndsWith("Command", System.StringComparison.Ordinal) + from field in type.GetFields(System.Reflection.BindingFlags.NonPublic + | System.Reflection.BindingFlags.Public + | System.Reflection.BindingFlags.Static) + where field.FieldType == typeof(JsonSerializerOptions) + let options = (JsonSerializerOptions?)field.GetValue(null) + where options is not null && options.MaxDepth != AnalysisJson.MaxDepth + select $"{type.Name}.{field.Name} (MaxDepth {options.MaxDepth})").ToList(); + + Assert.True(offenders.Count == 0, + "These write an analysis with the default depth ceiling and will fail on a deep plan: " + + string.Join(", ", offenders)); + } + /// /// Pins the headroom itself. 1024 is about 500 nested operators against the ~30 that used to fail; /// a future edit dropping it back toward the default would re-open #430 for large plans only, which From 60eb0bf43a7e94c8d9b268c904f61da53ce60390 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 21 Aug 2026 00:55:38 +0200 Subject: [PATCH 12/15] Say which warnings are SQL Server's and which are ours (#436) (#439) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reporter could not tell our advice apart from the engine's, and there was no way to: both arrive as a PlanWarning carrying nothing but type, severity and message, and both render identically. 14 warning constructions in ShowPlanParser.Warnings.cs are lifted straight out of the plan's own element; 47 across PlanAnalyzer*.cs are our rules reading plan shape. Nothing in the model or the output said which was which. The distinction is not cosmetic. A warning SQL Server wrote into the plan is a record of what the engine did — it spilled, it converted, it had no statistics. One of our rules is an inference, and an inference can be wrong about a particular plan in a way the engine's own record cannot be. The same issue supplies the example: we told this reporter a conversion prevented an index seek on a plan SQL Server had raised no conversion warning about at all, and being able to see "that line is ours, this line is the engine's" is what would have let him weigh the two himself. PlanWarning.Source defaults to PerformanceStudio, and everything the parser produces is stamped SqlServer in ONE place — the single return of ParseWarningsFromElement, which every parser warning already funnels through. Stamping the 14 construction sites individually would have been the same bug as #430's missed QueryStoreCommand: a rule you have to remember at each site is a rule that eventually gets forgotten. A new engine warning added to that method is attributed correctly without anyone thinking about it. UX, since it was left to me: - Only the ENGINE's warnings are tagged, as " [SQL Server]", in both the GUI warnings panel and the CLI text output. Tagging both kinds would put a badge on every line and carry no information — our own advice is what a reader already expects from a plan analyzer, so the marked case should be the exception. - The tag sits next to the existing " [legacy]" tag and is built the same way, because that badge already established the idiom here. - JSON and MCP consumers get it as a "source" field rather than having to parse a tag out of a string. CLI output contract, flagged rather than slipped in: adding "source" to every warning changes the bytes of `analyze --compact`, so HistoricalCliContractTests.ExpectedCompactOutputSha256 is rolled. The change is additive — nothing removed or renamed, so a consumer reading fields by name is unaffected — but anything hashing or diffing whole output will see a difference. That constant exists to make this a decision instead of a discovery, so it is called out here rather than quietly updated. The characterization baseline is untouched: it digests type, severity and message, none of which changed. Tested: 65 across WarningSourceTests, HistoricalCliContractTests, WarningCharacterizationTests and PlanAnalyzerTests, 0 failures. The new tests pin both kinds on one plan that carries both, that no warning type is ever produced as both kinds across every committed plan, that only the engine's are tagged in text output, and that the JSON field is populated. dotnet build clean. Co-authored-by: Claude Opus 5 (1M context) --- .../Controls/PlanViewerControl.Properties.cs | 10 +- .../Controls/PlanViewerControl.Rendering.cs | 8 ++ src/PlanViewer.Core/Models/PlanModels.cs | 26 +++++ src/PlanViewer.Core/Output/AnalysisResult.cs | 9 ++ src/PlanViewer.Core/Output/ResultMapper.cs | 6 +- src/PlanViewer.Core/Output/TextFormatter.cs | 18 ++- .../Services/ShowPlanParser.Warnings.cs | 6 + .../HistoricalCliContractTests.cs | 7 +- .../WarningSourceTests.cs | 106 ++++++++++++++++++ 9 files changed, 184 insertions(+), 12 deletions(-) create mode 100644 tests/PlanViewer.Core.Tests/WarningSourceTests.cs diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs index 4978979..dbd8369 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs @@ -818,9 +818,10 @@ private void ShowPropertiesPanel(PlanNode node) : w.Severity == PlanWarningSeverity.Warning ? "#FFB347" : "#6BB5FF"; var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) }; var legacyTag = w.IsLegacy ? " [legacy]" : ""; + var sourceTag = WarningSourceTag(w); var planWarnHeader = w.MaxBenefitPercent.HasValue - ? $"\u26A0 {w.WarningType}{legacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" - : $"\u26A0 {w.WarningType}{legacyTag}"; + ? $"\u26A0 {w.WarningType}{sourceTag}{legacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" + : $"\u26A0 {w.WarningType}{sourceTag}{legacyTag}"; warnPanel.Children.Add(new TextBlock { Text = planWarnHeader, @@ -901,9 +902,10 @@ private void ShowPropertiesPanel(PlanNode node) : w.Severity == PlanWarningSeverity.Warning ? "#FFB347" : "#6BB5FF"; var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) }; var nodeLegacyTag = w.IsLegacy ? " [legacy]" : ""; + var nodeSourceTag = WarningSourceTag(w); var nodeWarnHeader = w.MaxBenefitPercent.HasValue - ? $"\u26A0 {w.WarningType}{nodeLegacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" - : $"\u26A0 {w.WarningType}{nodeLegacyTag}"; + ? $"\u26A0 {w.WarningType}{nodeSourceTag}{nodeLegacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" + : $"\u26A0 {w.WarningType}{nodeSourceTag}{nodeLegacyTag}"; warnPanel.Children.Add(new TextBlock { Text = nodeWarnHeader, diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs index 6955563..ae4ef22 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs @@ -516,6 +516,14 @@ private static string FormatBytes(double bytes) private static string FormatBenefitPercent(double pct) => pct >= 100 ? $"{pct:N0}" : $"{pct:N1}"; + /// + /// #436: marks the warnings SQL Server itself wrote into the plan, so they are not read as one of + /// our inferences. Only the engine's are tagged — they are the minority, and a badge on every + /// warning would carry no information. + /// + private static string WarningSourceTag(PlanWarning warning) => + warning.Source == PlanWarningSource.SqlServer ? " [SQL Server]" : ""; + private static bool HasSpillInPlanTree(PlanNode node) { foreach (var w in node.Warnings) diff --git a/src/PlanViewer.Core/Models/PlanModels.cs b/src/PlanViewer.Core/Models/PlanModels.cs index 6f8ab49..20ecd09 100644 --- a/src/PlanViewer.Core/Models/PlanModels.cs +++ b/src/PlanViewer.Core/Models/PlanModels.cs @@ -375,6 +375,22 @@ public class PlanWarning public PlanWarningSeverity Severity { get; set; } public SpillDetail? SpillDetails { get; set; } + /// + /// Who says so — SQL Server itself, or us (#436). + /// + /// These two carry very different weight and a reader cannot tell them apart from the + /// message text. A warning SQL Server wrote into the plan's own <Warnings> element is a + /// statement of fact from the engine that executed the query: it spilled, it converted, it had no + /// statistics. One of our rules is an inference from plan shape, and inferences can be wrong about + /// a particular plan in a way the engine's own record cannot be. + /// + /// Defaults to because the analyzer builds + /// the large majority of them. Everything the parser lifts out of the plan XML is stamped + /// in one place, at the single return of + /// ShowPlanParser.ParseWarningsFromElement, so a new engine warning cannot be added and forgotten. + /// + public PlanWarningSource Source { get; set; } = PlanWarningSource.PerformanceStudio; + /// /// Maximum percentage of elapsed time that could be saved by addressing this finding. /// null = not quantifiable, 0 = calculated as negligible. @@ -397,6 +413,16 @@ public class PlanWarning public enum PlanWarningSeverity { Info, Warning, Critical } +/// Where a came from. See . +public enum PlanWarningSource +{ + /// An inference of ours, from the shape of the plan. + PerformanceStudio, + + /// Read out of the plan's own <Warnings> element — the engine's record, not ours. + SqlServer +} + public class MemoryGrantInfo { public long SerialRequiredMemoryKB { get; set; } diff --git a/src/PlanViewer.Core/Output/AnalysisResult.cs b/src/PlanViewer.Core/Output/AnalysisResult.cs index dbea260..267f4fc 100644 --- a/src/PlanViewer.Core/Output/AnalysisResult.cs +++ b/src/PlanViewer.Core/Output/AnalysisResult.cs @@ -247,6 +247,15 @@ public class WarningResult /// [JsonPropertyName("is_legacy")] public bool IsLegacy { get; set; } + + /// + /// "SqlServer" when the engine wrote this into the plan's own <Warnings> element, or + /// "PerformanceStudio" when it is one of our rules inferring it from plan shape (#436). Values + /// match , spelled the same way + /// spells its enum. + /// + [JsonPropertyName("source")] + public string Source { get; set; } = ""; } public class MissingIndexResult diff --git a/src/PlanViewer.Core/Output/ResultMapper.cs b/src/PlanViewer.Core/Output/ResultMapper.cs index 4a485a2..97996af 100644 --- a/src/PlanViewer.Core/Output/ResultMapper.cs +++ b/src/PlanViewer.Core/Output/ResultMapper.cs @@ -195,7 +195,8 @@ private static StatementResult MapStatement( Message = w.Message, MaxBenefitPercent = w.MaxBenefitPercent, ActionableFix = w.ActionableFix, - IsLegacy = w.IsLegacy + IsLegacy = w.IsLegacy, + Source = w.Source.ToString() }); } @@ -304,7 +305,8 @@ private static OperatorResult MapNode(PlanNode node, CancellationToken cancellat NodeId = node.NodeId, MaxBenefitPercent = w.MaxBenefitPercent, ActionableFix = w.ActionableFix, - IsLegacy = w.IsLegacy + IsLegacy = w.IsLegacy, + Source = w.Source.ToString() }); } diff --git a/src/PlanViewer.Core/Output/TextFormatter.cs b/src/PlanViewer.Core/Output/TextFormatter.cs index bc6f303..5e6ad25 100644 --- a/src/PlanViewer.Core/Output/TextFormatter.cs +++ b/src/PlanViewer.Core/Output/TextFormatter.cs @@ -193,7 +193,7 @@ internal static void WriteTextCancellable( ? $" (up to {(w.MaxBenefitPercent.Value >= 100 ? w.MaxBenefitPercent.Value.ToString("N0") : w.MaxBenefitPercent.Value.ToString("N1"))}% benefit)" : ""; var legacyTag = w.IsLegacy ? " [legacy]" : ""; - writer.WriteLine($" [{w.Severity}] {w.Type}{legacyTag}{benefitTag}: {EscapeNewlines(w.Message)}"); + writer.WriteLine($" [{w.Severity}] {w.Type}{SourceTag(w.Source)}{legacyTag}{benefitTag}: {EscapeNewlines(w.Message)}"); if (!string.IsNullOrEmpty(w.ActionableFix)) writer.WriteLine($" Fix: {EscapeNewlines(w.ActionableFix)}"); } @@ -339,7 +339,7 @@ private static void WriteGroupedOperatorWarnings( // Split each message into "data | explanation" at the last sentence boundary // that starts with "The " (the harm assessment). Group by shared explanation. - var entries = new List<(string Severity, string Operator, string Data, string? Explanation, double? Benefit, bool IsLegacy)>(); + var entries = new List<(string Severity, string Operator, string Data, string? Explanation, double? Benefit, bool IsLegacy, string Source)>(); foreach (var w in sorted) { var msg = w.Message; @@ -358,7 +358,7 @@ private static void WriteGroupedOperatorWarnings( data = msg; } - entries.Add((w.Severity, w.Operator ?? "?", data, explanation, w.MaxBenefitPercent, w.IsLegacy)); + entries.Add((w.Severity, w.Operator ?? "?", data, explanation, w.MaxBenefitPercent, w.IsLegacy, w.Source)); } // Group entries that share the same severity, type, and explanation @@ -380,7 +380,7 @@ private static void WriteGroupedOperatorWarnings( var benefitTag = item.Benefit.HasValue ? $" (up to {(item.Benefit.Value >= 100 ? item.Benefit.Value.ToString("N0") : item.Benefit.Value.ToString("N1"))}% benefit)" : ""; - writer.WriteLine($" [{item.Severity}] {item.Operator}{legacyTag}{benefitTag}: {EscapeNewlines(item.Data)}"); + writer.WriteLine($" [{item.Severity}] {item.Operator}{SourceTag(item.Source)}{legacyTag}{benefitTag}: {EscapeNewlines(item.Data)}"); } writer.WriteLine($" -> {group.Key.Item2}"); } @@ -394,7 +394,7 @@ private static void WriteGroupedOperatorWarnings( var benefitTag = item.Benefit.HasValue ? $" (up to {(item.Benefit.Value >= 100 ? item.Benefit.Value.ToString("N0") : item.Benefit.Value.ToString("N1"))}% benefit)" : ""; - writer.WriteLine($" [{item.Severity}] {item.Operator}{legacyTag}{benefitTag}: {EscapeNewlines(full)}"); + writer.WriteLine($" [{item.Severity}] {item.Operator}{SourceTag(item.Source)}{legacyTag}{benefitTag}: {EscapeNewlines(full)}"); } } } @@ -420,6 +420,14 @@ public static string FormatMemoryGrantKB(long kb) /// survive the top-level line split in AdviceContentBuilder.Build(). /// CreateWarningBlock splits on U+001F to restore the internal structure. /// + /// + /// #436: marks the warnings SQL Server itself put in the plan, so they read differently from our + /// inferences. Only the engine's are tagged — they are the minority and the ones carrying extra + /// authority, and tagging all of them would just be noise on every line. + /// + private static string SourceTag(string? source) => + source == nameof(PlanViewer.Core.Models.PlanWarningSource.SqlServer) ? " [SQL Server]" : ""; + private static string EscapeNewlines(string text) => text.Replace('\n', '\x1F'); private static void CollectNodeTimings( diff --git a/src/PlanViewer.Core/Services/ShowPlanParser.Warnings.cs b/src/PlanViewer.Core/Services/ShowPlanParser.Warnings.cs index 5fe1ca1..3a2fb8b 100644 --- a/src/PlanViewer.Core/Services/ShowPlanParser.Warnings.cs +++ b/src/PlanViewer.Core/Services/ShowPlanParser.Warnings.cs @@ -332,6 +332,12 @@ private static List ParseWarningsFromElement(XElement warningsEl) }); } + /* #436: stamped here rather than on each of the constructions above, so that everything read + out of the plan's own element is marked as the engine's, including whatever gets + added to this method next. This is the only place parser warnings are built. */ + foreach (var warning in result) + warning.Source = PlanWarningSource.SqlServer; + return result; } } diff --git a/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs b/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs index c2248a3..5be64e4 100644 --- a/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs +++ b/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs @@ -6,8 +6,13 @@ namespace PlanViewer.Core.Tests; public sealed class HistoricalCliContractTests { + /* Rolled for #436, which adds "source" to every warning in the JSON output so a consumer can tell + SQL Server's own warnings from Performance Studio's inferences. The change is additive — nothing + was removed or renamed, so a consumer reading fields by name is unaffected — but anything + diffing or hashing whole output sees different bytes, which is exactly what this constant is + here to make somebody decide on rather than discover. */ private const string ExpectedCompactOutputSha256 = - "0c609fed8e250d9366eb9a6cd5eaf40b661ee30d7ba2546bd7726960592e9d87"; + "06975e4e513eef23669c86bc2cfeec916f442461d605060bbd996bd44df13405"; [Fact] public async Task AnalyzeCompact_PreservesHistoricalOutputBytes() diff --git a/tests/PlanViewer.Core.Tests/WarningSourceTests.cs b/tests/PlanViewer.Core.Tests/WarningSourceTests.cs new file mode 100644 index 0000000..d2098b8 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/WarningSourceTests.cs @@ -0,0 +1,106 @@ +using System.IO; +using System.Linq; +using PlanViewer.Core.Models; +using PlanViewer.Core.Output; + +namespace PlanViewer.Core.Tests; + +/// +/// #436: a reader could not tell which warnings SQL Server itself raised and which ones we inferred, +/// because both arrive as a with nothing but type, severity and message. +/// +/// The distinction matters more than presentation. A warning the engine wrote into the plan's own +/// <Warnings> element is a record of what happened when the query ran — it spilled, it converted, +/// it had no statistics. One of our rules is an inference from plan shape, and an inference can be +/// wrong about a particular plan in a way the engine's own record cannot be. #436 was itself an +/// example: we claimed a conversion prevented a seek on a plan SQL Server had raised no conversion +/// warning about at all. +/// +public class WarningSourceTests +{ + /// + /// One plan carrying both kinds. "Implicit Conversion" is lifted from the PlanAffectingConvert + /// element SQL Server wrote; "Non-SARGable Predicate" is Rule 12 reading the predicate text. + /// + [Fact] + public void TheTwoKindsAreToldApartOnAPlanCarryingBoth() + { + var plan = PlanTestHelper.LoadAndAnalyze("convert_implicit_plan.sqlplan"); + + Assert.All( + PlanTestHelper.WarningsOfType(plan, "Implicit Conversion"), + w => Assert.Equal(PlanWarningSource.SqlServer, w.Source)); + + Assert.All( + PlanTestHelper.WarningsOfType(plan, "Non-SARGable Predicate"), + w => Assert.Equal(PlanWarningSource.PerformanceStudio, w.Source)); + } + + /// + /// The stamp is applied once, at the single return of ParseWarningsFromElement, rather than at + /// each construction — so this asserts the property that arrangement buys: across every committed + /// plan, no warning type is ever produced as both kinds. A type appearing as both would mean a + /// construction site got missed or an analyzer rule started claiming the engine's authority. + /// + [Fact] + public void NoWarningTypeIsEverProducedAsBothKinds() + { + var plansDir = Path.Combine(AppContext.BaseDirectory, "Plans"); + var confusions = + (from file in Directory.GetFiles(plansDir, "*.sqlplan") + let plan = PlanTestHelper.LoadAndAnalyze(Path.GetFileName(file)) + from warning in PlanTestHelper.AllWarnings(plan) + group warning.Source by warning.WarningType into byType + where byType.Distinct().Count() > 1 + select byType.Key).ToList(); + + Assert.True(confusions.Count == 0, + "These types are attributed to both SQL Server and us: " + string.Join(", ", confusions)); + } + + /// + /// Only the engine's warnings are tagged in rendered output. Tagging both would put a badge on + /// every line, which carries no information — our own advice is what a reader already expects + /// from a plan analyzer. + /// + [Fact] + public void OnlyTheEnginesWarningsAreTaggedInTextOutput() + { + var plan = PlanTestHelper.LoadAndAnalyze("convert_implicit_plan.sqlplan"); + var result = ResultMapper.Map(plan, "convert_implicit_plan.sqlplan"); + + var writer = new StringWriter(); + TextFormatter.WriteText(result, writer); + var text = writer.ToString(); + + var tagged = text.Split('\n').Where(l => l.Contains("[SQL Server]")).ToList(); + + Assert.NotEmpty(tagged); + Assert.All(tagged, line => Assert.Contains("Implicit Conversion", line)); + Assert.DoesNotContain("Non-SARGable Predicate [SQL Server]", text); + } + + /// The JSON and MCP consumers get it as a field rather than having to parse the tag out. + [Fact] + public void TheJsonOutputCarriesTheSource() + { + var plan = PlanTestHelper.LoadAndAnalyze("convert_implicit_plan.sqlplan"); + var result = ResultMapper.Map(plan, "convert_implicit_plan.sqlplan"); + + var all = result.Statements + .SelectMany(s => s.Warnings.Concat(Flatten(s.OperatorTree))) + .ToList(); + + Assert.Contains(all, w => w.Type == "Implicit Conversion" && w.Source == nameof(PlanWarningSource.SqlServer)); + Assert.Contains(all, w => w.Type == "Non-SARGable Predicate" && w.Source == nameof(PlanWarningSource.PerformanceStudio)); + Assert.All(all, w => Assert.NotEqual("", w.Source)); + } + + private static System.Collections.Generic.IEnumerable Flatten(OperatorResult? node) + { + if (node == null) yield break; + foreach (var w in node.Warnings) yield return w; + foreach (var child in node.Children) + foreach (var w in Flatten(child)) yield return w; + } +} From 557f910774207fa8f604337de9bad268e66621e8 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:16:12 +0200 Subject: [PATCH 13/15] Stop corrupting memory resolving an open file handle on macOS (#441) (#443) * Stop corrupting memory resolving an open file handle on macOS (#441) fcntl(2) is variadic - int fcntl(int, int, ...) - and this called it through a plain DllImport with a fixed third parameter: [DllImport("libc", EntryPoint = "fcntl", SetLastError = true)] private static extern int Fcntl(int fileDescriptor, int command, byte[] buffer); On Apple arm64 the variadic ABI differs from AAPCS64: named arguments go in registers, variadic arguments go on the STACK. A fixed-signature P/Invoke puts the buffer in x2, and the callee never looks there. It does not fail, which is why nothing caught it. Reproduced standalone, outside the test suite, one open file: Arch : Arm64 FIXED rc=0 errno=0 len=0 -> '' rc=0 is fcntl reporting SUCCESS. F_GETPATH on success writes the path into the buffer it was handed; ours came back empty, so it wrote up to MAXPATHLEN bytes through whatever pointer happened to be in that stack slot. An arbitrary ~1KB write, on every call, into this process. That is the whole of #441. `dotnet test` has been unrunnable on macOS - it wedged, sometimes as a GC-suspension livelock spinning a core, sometimes as an all-threads-blocked deadlock, which is exactly what an arbitrary write into runtime memory looks like from the outside. Both shapes are gone. Diagnosed as a prediction rather than described afterwards: the three tests in McpPlanPathPolicyTests that pass are precisely the three that never reach GetFinalPath, and both that reach it hung, deterministically, 5/5 and 2/2. It is also why CI never saw it - Linux takes the /proc/self/fd branch and Windows takes GetFinalPathNameByHandle, so only macOS goes anywhere near fcntl, and only arm64 has the mismatch. ubuntu-latest structurally cannot reproduce this. The fix is libproc's proc_pidfdinfo, which answers the same question with a FIXED signature, so ordinary marshalling is correct. .NET has no varargs P/Invoke on this target at all - __arglist throws "Vararg calling convention not supported" - so repairing the fcntl declaration in place is not available. Worth recording, because I recommended the wrong fix on the issue first and it sounds right: comparing fstat(fd) against stat(path) by device+inode does NOT work here. stat() re-resolves the path, so if a symlink was swapped before the open it follows the swap too and the inodes match. It detects nothing. Resolving the descriptor back to its real path is the only thing that answers "where does this handle actually live", which is the question the TOCTOU check exists to ask. The struct offsets are derived in a comment from rather than being magic numbers, and the returned size is checked against the expected 1200 instead of trusted - so a layout change in a future macOS fails loudly rather than quietly handing back the wrong bytes, which is the failure mode this file just came out of. Also note macOS answers with the CANONICAL path: /private/var/... where Path.GetTempPath() reports /var/... The roots are canonicalized through ResolveLinkTarget already, so containment still matches; the new test compares by identity rather than by string for the same reason. The old test asserted on the returned handle's Label and passed happily on Linux and Windows while this call had never once worked on Apple silicon. The new test asserts the thing that was actually broken - that the resolver returns the real path of the file that is genuinely open - on whatever platform it runs. Security note: GetFinalPath is the TOCTOU re-validation, confirming the path the kernel really opened is still inside the advertised roots. On macOS arm64 it has never performed that check. It failed CLOSED - the empty string reached Path.GetFullPath, which throws ArgumentException, which is not in OpenAsync's catch filter - so this was not an exploitable bypass, but the guarantee OpenAsync_ValidatesAndReturnsTheSameOpenedHandle claims to prove was not being provided, and the FileStream leaked on the way out. Tested: full `dotnet test` on macOS ARM64 now finishes in 16 seconds, 305 passed, 0 failed, 2 skipped, twice. It did not finish at all before. McpPlanPathPolicyTests went from 5/5 hangs to 3/3 passes. General rule this earns: no variadic libc function through a plain DllImport in this repo - fcntl, open, ioctl, the printf family. They look fine on x64 and silently corrupt memory on Apple silicon. Co-Authored-By: Claude Opus 5 (1M context) * Write down the variadic P/Invoke rule that #441 earned The defect cost months of unrunnable tests on macOS and was invisible to CI, and nothing in the repo said not to do it. The specific trap is that it fails silently in both directions: fcntl reports success, and the corruption lands somewhere else entirely, so the crash never points at the call that caused it. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- CONTRIBUTING.md | 20 ++++++ .../ReplSurface/OpenedFilePathResolver.cs | 60 +++++++++++++--- .../OpenedFilePathResolverTests.cs | 71 +++++++++++++++++++ 3 files changed, 143 insertions(+), 8 deletions(-) create mode 100644 tests/PlanViewer.Core.Tests/OpenedFilePathResolverTests.cs diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 83652dd..1e87677 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -62,6 +62,26 @@ PerformanceStudio/ - No unnecessary abstractions — keep it simple and direct - Tests use real `.sqlplan` XML fixtures, not mocks +## Calling native code + +**Never call a variadic C function through a plain `DllImport`.** `fcntl`, `open`, `ioctl` and the +`printf` family all take `...`, and on Apple arm64 variadic arguments are passed on the *stack* while +a fixed-signature P/Invoke passes them in *registers*. The callee reads a stack slot you never wrote. + +This does not throw, and it does not return an error. In #441 `fcntl(F_GETPATH)` returned 0 for +success and wrote up to 1KB through whatever pointer happened to be in that slot — an arbitrary write +into the process on every call — while the buffer we passed came back empty. It made `dotnet test` +unrunnable on macOS for months, sometimes as a GC livelock and sometimes as a deadlock, and CI never +saw any of it because Linux and Windows take different branches. + +Use a non-variadic equivalent instead (`proc_pidfdinfo` in place of `fcntl(F_GETPATH)`, for example). +`__arglist` is not an escape hatch — it throws `Vararg calling convention not supported` on this +target. + +When you do P/Invoke a struct-returning native call, derive the offsets in a comment from the system +header rather than leaving magic numbers, and check the returned size against what you expected. A +layout change should fail loudly, not hand back plausible-looking wrong bytes. + ## Adding Analysis Rules Rules live in `PlanAnalyzer.cs`. Each rule: diff --git a/src/PlanViewer.Cli/ReplSurface/OpenedFilePathResolver.cs b/src/PlanViewer.Cli/ReplSurface/OpenedFilePathResolver.cs index ccb3bcc..675b48f 100644 --- a/src/PlanViewer.Cli/ReplSurface/OpenedFilePathResolver.cs +++ b/src/PlanViewer.Cli/ReplSurface/OpenedFilePathResolver.cs @@ -7,7 +7,31 @@ namespace PlanViewer.Cli.ReplSurface; internal static class OpenedFilePathResolver { - private const int MacOsGetPath = 50; + /* macOS resolves an open handle back to its path through libproc rather than fcntl(F_GETPATH), + because fcntl(2) is VARIADIC - int fcntl(int, int, ...) - and a variadic function cannot be + called correctly through a plain DllImport on Apple arm64. There the variadic arguments are + passed on the STACK while a fixed-signature P/Invoke puts them in registers, so the callee + reads a stack slot we never wrote. It does not fail: fcntl returns 0 for success and writes up + to MAXPATHLEN bytes through whatever pointer that slot happened to hold, which is an arbitrary + ~1KB write on every call. The buffer we passed comes back empty. See #441. + + proc_pidfdinfo has a fixed signature, so the ordinary marshalling is correct. */ + private const int ProcPidFdVNodePathInfo = 2; + + /* Offset of vip_path within struct vnode_fdinfowithpath, and the struct's total size, both from + : + + vnode_fdinfowithpath { proc_fileinfo pfi; vnode_info_path vip; } + proc_fileinfo = 4 + 4 + 8 + 4 + 4 = 24 + vnode_info = vinfo_stat(136) + 4 + 4 + fsid_t(8) = 152 -> vip_path begins at 176 + vnode_info_path = 152 + MAXPATHLEN(1024) = 1176 + total = 24 + 1176 = 1200 + + The size is checked against the call's return value rather than trusted, so a layout change in + a future macOS fails loudly here instead of quietly handing back the wrong bytes - which is the + failure mode this whole file just came out of. */ + private const int VNodePathInfoSize = 1200; + private const int VipPathOffset = 176; public static string GetFinalPath(FileStream stream) { @@ -53,13 +77,28 @@ private static string GetWindowsPath(SafeFileHandle handle) private static string GetMacOsPath(SafeFileHandle handle) { - var buffer = new byte[4096]; - if (Fcntl(handle.DangerousGetHandle().ToInt32(), MacOsGetPath, buffer) != 0) + var buffer = new byte[VNodePathInfoSize]; + var written = ProcPidFdInfo( + Environment.ProcessId, + handle.DangerousGetHandle().ToInt32(), + ProcPidFdVNodePathInfo, + buffer, + buffer.Length); + + if (written <= 0) throw CreateNativeIOException("Could not resolve the opened macOS file handle."); - var terminator = Array.IndexOf(buffer, (byte)0); + if (written != VNodePathInfoSize) + throw new IOException( + $"Unexpected vnode_fdinfowithpath size {written}; expected {VNodePathInfoSize}."); + + var terminator = Array.IndexOf(buffer, (byte)0, VipPathOffset); if (terminator < 0) - terminator = buffer.Length; - return Path.GetFullPath(Encoding.UTF8.GetString(buffer, 0, terminator)); + throw new IOException("Opened macOS file handle resolved to an unterminated path."); + if (terminator == VipPathOffset) + throw new IOException("Opened macOS file handle resolved to an empty path."); + + return Path.GetFullPath( + Encoding.UTF8.GetString(buffer, VipPathOffset, terminator - VipPathOffset)); } private static IOException CreateNativeIOException(string message) => @@ -72,6 +111,11 @@ private static extern uint GetFinalPathNameByHandle( uint filePathLength, uint flags); - [DllImport("libc", EntryPoint = "fcntl", SetLastError = true)] - private static extern int Fcntl(int fileDescriptor, int command, byte[] buffer); + [DllImport("libproc", EntryPoint = "proc_pidfdinfo", SetLastError = true)] + private static extern int ProcPidFdInfo( + int processId, + int fileDescriptor, + int flavor, + byte[] buffer, + int bufferSize); } diff --git a/tests/PlanViewer.Core.Tests/OpenedFilePathResolverTests.cs b/tests/PlanViewer.Core.Tests/OpenedFilePathResolverTests.cs new file mode 100644 index 0000000..3a7579d --- /dev/null +++ b/tests/PlanViewer.Core.Tests/OpenedFilePathResolverTests.cs @@ -0,0 +1,71 @@ +using PlanViewer.Cli.ReplSurface; + +namespace PlanViewer.Core.Tests; + +/// +/// #441: the macOS resolver called fcntl(F_GETPATH) through a plain DllImport. fcntl(2) is variadic, +/// and on Apple arm64 variadic arguments are passed on the stack while a fixed-signature P/Invoke +/// puts them in registers — so the callee read a stack slot we never wrote. +/// +/// It did not fail. fcntl returned 0 for success and wrote up to MAXPATHLEN bytes through whatever +/// pointer that slot happened to hold, an arbitrary ~1KB write on every call, while the buffer we +/// passed came back empty. +/// +/// Nothing caught it, and that is the interesting part: the old test asserted on the returned +/// handle's Label and passed on Linux and Windows, which take entirely different branches. So this +/// asserts the one thing that was actually broken — that the resolver returns the real path of the +/// file that is genuinely open — and it asserts it on whatever platform the suite is running on. +/// +public class OpenedFilePathResolverTests +{ + [Fact] + public void GetFinalPath_ReturnsTheRealPathOfTheOpenHandle() + { + var path = Path.Combine(Path.GetTempPath(), $"resolver-{Guid.NewGuid():N}.sqlplan"); + File.WriteAllText(path, ""); + try + { + using var stream = new FileStream(path, FileMode.Open, FileAccess.Read); + + var resolved = OpenedFilePathResolver.GetFinalPath(stream); + + /* Compared by identity rather than by string, because macOS answers with the canonical + path — Path.GetTempPath() reports /var/folders/... while the kernel reports + /private/var/folders/..., and /var is a symlink to /private/var. A string comparison + here would fail for a reason that has nothing to do with the defect. */ + Assert.True(File.Exists(resolved), $"Resolver returned a path that does not exist: '{resolved}'"); + Assert.Equal( + new FileInfo(path).Length, + new FileInfo(resolved).Length); + Assert.Equal( + Path.GetFileName(path), + Path.GetFileName(resolved)); + } + finally + { + File.Delete(path); + } + } + + /// + /// The empty string is precisely what the broken call produced, and it is worth pinning that it + /// can never be mistaken for a valid answer: an empty path would sail through the extension and + /// containment checks in McpPlanPathPolicy as a Path.GetFullPath argument exception rather than + /// as a denial. + /// + [Fact] + public void GetFinalPath_NeverReturnsAnEmptyPath() + { + var path = Path.Combine(Path.GetTempPath(), $"resolver-{Guid.NewGuid():N}.sqlplan"); + File.WriteAllText(path, ""); + try + { + using var stream = new FileStream(path, FileMode.Open, FileAccess.Read); + Assert.False(string.IsNullOrWhiteSpace(OpenedFilePathResolver.GetFinalPath(stream))); + } + finally + { + File.Delete(path); + } + } +} From 42045a18b1dbc842b91f3c13089a93c52983aa7b Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:16:14 +0200 Subject: [PATCH 14/15] Move the test suite to Microsoft.Testing.Platform for xunit.v3 4.0.0 (#434) (#442) Dependabot #434 fails to build, and not because of anything it did wrong: Microsoft.Testing.Platform.MSBuild.targets(320,5): error : Testing with VSTest target is no longer supported by Microsoft.Testing.Platform on .NET 10 SDK and later. xunit.v3 4.0.0 drops VSTest on the .NET 10 SDK. There is no version of this bump that keeps the old runner, so it is migrate or stay on 3.2.2. #433 is the same family and passes only because bumping the VSTest adapter alone changes nothing. The opt-in is repo-level, not per-project, which the target names outright (_SupportsGlobalJsonTestRunner): global.json now selects the runner. Setting TestingPlatformDotnetTestSupport in the csproj alone does not do it, and I tried that first. xunit.runner.visualstudio is dropped because it IS the VSTest adapter and has no role here any more. That moots #433. The watchdog from 5f84116 needed porting, and this is the part worth reading. Both of its layers were VSTest-only: RunSettingsFilePath/TestSessionTimeout for bare local runs, and the blame-hang options in the three workflows. The local layer is now TestingPlatformCommandLineArguments carrying a 15m session timeout, same value and same no-flags-to-remember property; the workflows pass the hangdump options, which produce the same name-the-wedged-test artifact. I verified the local layer the way the original was verified, because a silently ignored property looks exactly like a working one: set it to 1ms temporarily and a bare `dotnet test` cancelled with 0 tests succeeded, then restored it. What the port does NOT recover, stated plainly because it would be easy to leave implied: neither MTP mechanism stops the GC-suspension livelock the original watchdog was written for. Measured against a genuinely wedged host on macOS ARM64, not assumed - it ran 2m55s at 107% CPU against a 90s hangdump timeout, and 8 minutes against a 60s session timeout, ignoring both. The reason is the same one 5f84116 gives for xUnit timeouts: the diagnostics the platform needs are served by the execution engine that is suspended. MTP does put the timeout in a separate controller process, which was the property that mattered, and it still cannot win. An ordinary hang, where the runtime is responsive, is killed fine. So this is not a regression against VSTest so much as both runners being equally powerless there, and I want to be careful not to claim more: I did not prove the OLD TestSessionTimeout could kill a livelocked host either. It was verified at 1ms on a healthy run, never against a real wedge. Filed separately. Tested: 279 passed, 0 failed, 2 skipped under the new runner (the whole suite except the Mcp/Repl classes, which wedge on macOS for reasons that predate this and are the subject of that separate issue). The CI flag form was run verbatim to confirm the options parse. dotnet build clean; the 9 warnings are the pre-existing MCP9005 obsolete-API uses in McpSmokeTests.cs. Two XML-comment '--' errors on the way in, exactly as 5f84116 warned. Noted in the comment so the next person does not rediscover it. Co-authored-by: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 2 +- .github/workflows/nightly.yml | 2 +- .github/workflows/release.yml | 2 +- global.json | 5 +++ .../PlanViewer.Core.Tests.csproj | 33 ++++++++++++++----- .../hang-watchdog.runsettings | 30 ----------------- 6 files changed, 32 insertions(+), 42 deletions(-) create mode 100644 global.json delete mode 100644 tests/PlanViewer.Core.Tests/hang-watchdog.runsettings diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5c48edb..5c3a52b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,4 +46,4 @@ jobs: run: dotnet build PlanViewer.sln -c Release --no-restore - name: Run tests - run: dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --no-build --verbosity normal --blame-hang --blame-hang-timeout 5m --blame-hang-dump-type none + run: dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --no-build --verbosity normal -- --hangdump --hangdump-timeout 5m --hangdump-type none diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index c8d4137..f86ef58 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -65,7 +65,7 @@ jobs: dotnet restore tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj - name: Run tests - run: dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --verbosity normal --blame-hang --blame-hang-timeout 5m --blame-hang-dump-type none + run: dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --verbosity normal -- --hangdump --hangdump-timeout 5m --hangdump-type none - name: Publish App (all platforms) run: | diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 8b7e3ad..92ff73f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -52,7 +52,7 @@ jobs: run: | dotnet restore dotnet build -c Release - dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --no-build --verbosity normal --blame-hang --blame-hang-timeout 5m --blame-hang-dump-type none + dotnet test tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj -c Release --no-build --verbosity normal -- --hangdump --hangdump-timeout 5m --hangdump-type none - name: Publish App (all platforms) run: | diff --git a/global.json b/global.json new file mode 100644 index 0000000..3140116 --- /dev/null +++ b/global.json @@ -0,0 +1,5 @@ +{ + "test": { + "runner": "Microsoft.Testing.Platform" + } +} diff --git a/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj b/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj index 81a19f4..a06f49e 100644 --- a/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj +++ b/tests/PlanViewer.Core.Tests/PlanViewer.Core.Tests.csproj @@ -7,21 +7,36 @@ false true - - - $(MSBuildProjectDirectory)/hang-watchdog.runsettings + true + + + --timeout 15m + - - + diff --git a/tests/PlanViewer.Core.Tests/hang-watchdog.runsettings b/tests/PlanViewer.Core.Tests/hang-watchdog.runsettings deleted file mode 100644 index 141a25e..0000000 --- a/tests/PlanViewer.Core.Tests/hang-watchdog.runsettings +++ /dev/null @@ -1,30 +0,0 @@ - - - - - 900000 - - From 3e8ff039832e7da83b0d5caeea14413ba9fc3b48 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:21:12 +0200 Subject: [PATCH 15/15] Bump version to 1.20.0 (#444) Minor rather than patch. 1.19.x would understate it: #439 adds a "source" field to every warning in the JSON and MCP output and a new badge in the app and CLI, and #437 changes what an existing analysis rule concludes about a plan. Both are things a consumer can notice, and one of them is output-shape. What ships: - #437 Rule 12 no longer calls a conversion non-SARGable when it converts the parameter rather than the column. Plans carrying a parameter-side conversion on a scan lose that warning and report the scan's residual predicate instead. Verified against all 38 committed plans: no other plan's verdict moves. - #439 SQL Server's own warnings are now told apart from ours, tagged [SQL Server] in the app and CLI and carried as "source" in JSON/MCP. Additive, but it changes the bytes of analyze --compact. - #431 Robot Advice no longer takes the app down on a deep plan. - #438 querystore gets the same depth ceiling analyze got; it had been failing quietly on deep plans, one ERROR row per plan. - #443 the macOS handle resolver no longer corrupts memory on Apple silicon. - #425 Entra MFA works again (WAM parent window handle). Not user-facing but worth knowing for anyone building from this tag: the suite runs on Microsoft.Testing.Platform now (#442), and `dotnet test` finishes on macOS for the first time (#443) - 307 tests, 305 passing, 13 seconds. Co-authored-by: Claude Opus 5 (1M context) --- CITATION.cff | 4 ++-- src/Directory.Build.props | 2 +- src/PlanViewer.Ssms/Properties/AssemblyInfo.cs | 4 ++-- src/PlanViewer.Ssms/source.extension.vsixmanifest | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/CITATION.cff b/CITATION.cff index 137f805..c3281a5 100644 --- a/CITATION.cff +++ b/CITATION.cff @@ -9,8 +9,8 @@ authors: website: "https://erikdarling.com" repository-code: "https://github.com/erikdarlingdata/PerformanceStudio" license: MIT -version: "1.19.1" -date-released: "2026-07-29" +version: "1.20.0" +date-released: "2026-08-21" keywords: - sql-server - execution-plan diff --git a/src/Directory.Build.props b/src/Directory.Build.props index 4637066..52a638c 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -15,7 +15,7 @@ Tests and server/ projects are outside src/ and are unaffected. --> - 1.19.1 + 1.20.0 Erik Darling Darling Data LLC Performance Studio diff --git a/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs b/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs index a0682f2..6ff4ddb 100644 --- a/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs +++ b/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs @@ -7,5 +7,5 @@ [assembly: AssemblyProduct("Performance Studio for SSMS")] [assembly: AssemblyCopyright("Copyright Darling Data 2026")] [assembly: ComVisible(false)] -[assembly: AssemblyVersion("1.19.1.0")] -[assembly: AssemblyFileVersion("1.19.1.0")] +[assembly: AssemblyVersion("1.20.0.0")] +[assembly: AssemblyFileVersion("1.20.0.0")] diff --git a/src/PlanViewer.Ssms/source.extension.vsixmanifest b/src/PlanViewer.Ssms/source.extension.vsixmanifest index b1eae0a..191276b 100644 --- a/src/PlanViewer.Ssms/source.extension.vsixmanifest +++ b/src/PlanViewer.Ssms/source.extension.vsixmanifest @@ -3,7 +3,7 @@ xmlns:d="http://schemas.microsoft.com/developer/vsx-schema-design/2011"> Performance Studio for SSMS