From 3010287bbaf786b144ae1f6ca8cfb138fb15f4dc Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 13 Oct 2026 05:23:48 +0000 Subject: [PATCH 01/21] ci: bump the actions group with 2 updates Bumps the actions group with 2 updates: [anthropics/claude-code-action](https://github.com/anthropics/claude-code-action) and [actions/upload-artifact](https://github.com/actions/upload-artifact). Updates `anthropics/claude-code-action` from 1.0.240 to 1.0.248 - [Release notes](https://github.com/anthropics/claude-code-action/releases) - [Commits](https://github.com/anthropics/claude-code-action/compare/ed670b4cf9de2a5a570d130d2f6197b9e543cd64...1d6de8cb0c237e7c15e9e1bdf973826ebae490cc) Updates `actions/upload-artifact` from 7.0.1 to 7.0.2 - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](https://github.com/actions/upload-artifact/compare/043fb46d1a93c77aae656e7c1c64a875d1fc6a0a...cf430e030ddbb5b0abf93d22962f4752f3646cd9) --- updated-dependencies: - dependency-name: anthropics/claude-code-action dependency-version: 1.0.248 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions - dependency-name: actions/upload-artifact dependency-version: 7.0.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions ... Signed-off-by: dependabot[bot] --- .github/workflows/claude-code-review.yml | 2 +- .github/workflows/claude.yml | 2 +- .github/workflows/release.yml | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index c86d925..56cbf42 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -56,7 +56,7 @@ jobs: with: fetch-depth: 1 - - uses: anthropics/claude-code-action@ed670b4cf9de2a5a570d130d2f6197b9e543cd64 # v1.0.240 + - uses: anthropics/claude-code-action@1d6de8cb0c237e7c15e9e1bdf973826ebae490cc # v1.0.248 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} prompt: | diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml index 6f13b2a..ccd3be2 100644 --- a/.github/workflows/claude.yml +++ b/.github/workflows/claude.yml @@ -36,7 +36,7 @@ jobs: with: fetch-depth: 1 - - uses: anthropics/claude-code-action@ed670b4cf9de2a5a570d130d2f6197b9e543cd64 # v1.0.240 + - uses: anthropics/claude-code-action@1d6de8cb0c237e7c15e9e1bdf973826ebae490cc # v1.0.248 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} claude_args: | diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 85a0bf1..fbf6db1 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -124,7 +124,7 @@ jobs: - name: Upload Windows build for signing id: upload-unsigned - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 with: name: App-unsigned path: publish/win-x64/ @@ -201,7 +201,7 @@ jobs: - name: Upload the Velopack-generated executables for signing id: upload-postpack - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 with: name: Velopack-generated-unsigned path: postpack/unsigned/ @@ -263,7 +263,7 @@ jobs: id: upload-ssms-unsigned if: steps.ssms.outputs.BUILT == 'true' continue-on-error: true - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 with: name: Ssms-unsigned path: ssms-unsigned/ From 718f28238d3046208cba7cf36406ea56ff501b57 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 13 Oct 2026 05:28:27 +0000 Subject: [PATCH 02/21] deps: Bump the patch-and-minor group with 9 updates Bumps Avalonia from 12.1.3 to 12.1.4 Bumps Avalonia.Desktop from 12.1.3 to 12.1.4 Bumps Avalonia.Fonts.Inter from 12.1.3 to 12.1.4 Bumps Avalonia.Headless from 12.1.3 to 12.1.4 Bumps Avalonia.Themes.Fluent from 12.1.3 to 12.1.4 Bumps Meziantou.Framework.Win32.CredentialManager from 3.0.4 to 3.0.5 Bumps Microsoft.SqlServer.TransactSql.ScriptDom from 180.117.0 to 180.140.0 Bumps Microsoft.Testing.Extensions.HangDump from 2.4.1 to 2.5.1 Bumps xunit.v3 from 4.0.1 to 4.0.2 --- updated-dependencies: - dependency-name: Avalonia dependency-version: 12.1.4 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Desktop dependency-version: 12.1.4 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Fonts.Inter dependency-version: 12.1.4 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Themes.Fluent dependency-version: 12.1.4 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Meziantou.Framework.Win32.CredentialManager dependency-version: 3.0.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Microsoft.SqlServer.TransactSql.ScriptDom dependency-version: 180.140.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: Meziantou.Framework.Win32.CredentialManager dependency-version: 3.0.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Microsoft.SqlServer.TransactSql.ScriptDom dependency-version: 180.140.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: Meziantou.Framework.Win32.CredentialManager dependency-version: 3.0.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Microsoft.SqlServer.TransactSql.ScriptDom dependency-version: 180.140.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: Avalonia dependency-version: 12.1.4 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Desktop dependency-version: 12.1.4 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Fonts.Inter dependency-version: 12.1.4 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Headless dependency-version: 12.1.4 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Themes.Fluent dependency-version: 12.1.4 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Meziantou.Framework.Win32.CredentialManager dependency-version: 3.0.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Microsoft.SqlServer.TransactSql.ScriptDom dependency-version: 180.140.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: Microsoft.Testing.Extensions.HangDump dependency-version: 2.5.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: xunit.v3 dependency-version: 4.0.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Microsoft.Testing.Extensions.HangDump dependency-version: 2.5.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: xunit.v3 dependency-version: 4.0.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor ... Signed-off-by: dependabot[bot] --- Directory.Packages.props | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index 94a5a51..bfd2e21 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -27,7 +27,7 @@ SSMS project has an explicit reference that needs a central version. --> - + @@ -41,6 +41,6 @@ - + From 9d5cc34010479076aff5b04f75c019bacb0bc5e9 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 13 Oct 2026 23:34:33 -0400 Subject: [PATCH 03/21] Use sql2025 in the package README example Suggested by Neal Mummau on #646: the latest SQL Server version makes the better example server name. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01HJ4mqVCaTTw3cnQqTEbRbC --- src/PlanViewer.Cli/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PlanViewer.Cli/README.md b/src/PlanViewer.Cli/README.md index bc2c01f..7c7dac8 100644 --- a/src/PlanViewer.Cli/README.md +++ b/src/PlanViewer.Cli/README.md @@ -21,7 +21,7 @@ dotnet tool install --tool-path ./tools PerformanceStudio.Cli --version Date: Wed, 14 Oct 2026 18:49:09 -0400 Subject: [PATCH 04/21] Suppress individual findings by query hash in .planview.json (#651) Add a "suppressions" list beside "rules". Each entry names a statement by query hash plus a rule number or warning type, an optional operator label substring, and a required reason. Matching findings are flagged, never removed, and drop out of total_warnings, critical_warnings and warning_types. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018MrD9SGvLFx4kJaeGZyXG9 --- README.md | 57 ++- .../Controls/PlanViewerControl.Properties.cs | 6 +- .../Controls/PlanViewerControl.Rendering.cs | 10 +- .../Controls/PlanViewerControl.axaml.cs | 5 +- .../AdviceContentBuilder.CardSections.cs | 4 + src/PlanViewer.Cli/Commands/AnalyzeCommand.cs | 12 +- .../Commands/QueryStoreCommand.cs | 12 +- src/PlanViewer.Core/Models/AnalyzerConfig.cs | 37 ++ src/PlanViewer.Core/Models/PlanModels.cs | 9 + src/PlanViewer.Core/Output/AnalysisResult.cs | 21 + src/PlanViewer.Core/Output/HtmlExporter.cs | 10 +- .../Output/PlanOperationResults.cs | 12 + src/PlanViewer.Core/Output/ResultMapper.cs | 35 +- src/PlanViewer.Core/Output/TextFormatter.cs | 20 +- src/PlanViewer.Core/Services/ConfigLoader.cs | 51 ++- src/PlanViewer.Core/Services/OperatorLabel.cs | 28 ++ .../Services/PlanAnalysisPipeline.cs | 2 + .../Services/PlanOperations.cs | 8 +- .../Services/SuppressionApplier.cs | 93 ++++ .../ConfigLoaderTests.cs | 121 +++++ .../HistoricalCliContractTests.cs | 9 +- .../PlanViewer.Core.Tests/SuppressionTests.cs | 431 ++++++++++++++++++ 22 files changed, 949 insertions(+), 44 deletions(-) create mode 100644 src/PlanViewer.Core/Services/OperatorLabel.cs create mode 100644 src/PlanViewer.Core/Services/SuppressionApplier.cs create mode 100644 tests/PlanViewer.Core.Tests/ConfigLoaderTests.cs create mode 100644 tests/PlanViewer.Core.Tests/SuppressionTests.cs diff --git a/README.md b/README.md index f271cfe..f5c3c47 100644 --- a/README.md +++ b/README.md @@ -512,6 +512,7 @@ Options: --timeout Query timeout (default: 60) --login SQL Server login (bypasses credential store) --password SQL Server password (bypasses credential store) + --config Path to a .planview.json config file (overrides auto-discovery) ``` ### `planview credential` @@ -524,6 +525,60 @@ planview credential remove On Linux, `planview credential` exits with an error. See [Platform Support](#platform-support). +### Configuration file (.planview.json) + +A `.planview.json` file changes what the analyzer reports. The CLI, the desktop app and the MCP server all read it. + +The file is looked for in this order, and the first one found wins: + +1. The path given to `--config`. If that file does not exist, the command stops with an error. +2. `.planview.json` in the current directory. +3. `.planview.json` in your home directory. + +If none exists, the analyzer uses its defaults. The file is JSON. Comments and trailing commas are allowed. + +```jsonc +{ + "rules": { + "disabled": [3], + "severity_overrides": { "10": "Info" } + }, + "suppressions": [ + { + "query_hash": "0x00E1987CD1C28B4C", + "rule": 10, + "operator": "eventuous.Messages", + "reason": "Library procedure. The library owns the clustered index." + }, + { + "query_hash": "00e1987cd1c28b4c", + "type": "Implicit Conversion", + "reason": "Accepted. A vendor fixes the column type." + } + ] +} +``` + +**`rules.disabled`** is a list of rule numbers. A disabled rule never runs. + +**`rules.severity_overrides`** maps a rule number to a new severity. The allowed values are `Info`, `Warning` and `Critical`. + +**`suppressions`** marks single findings as accepted. Use it when a finding is correct but you cannot or will not act on it. A suppressed finding is not removed. It stays in the output with `"suppressed": true` and its reason, and the text and HTML output tag it as `[suppressed: ]`. It does not count toward `total_warnings`, `critical_warnings` or `warning_types`. The summary reports it in `suppressed_warnings` instead. A suppressed finding keeps its severity, including one set by `severity_overrides`. + +Each suppression has these fields: + +| Field | Required | Meaning | +|-------|----------|---------| +| `query_hash` | Yes | The query hash of the statement. Leading `0x` is optional and case does not matter, so `0x00E1987CD1C28B4C` and `00e1987cd1c28b4c` are the same hash. The suppression applies to that statement only, including statements inside a stored procedure body. | +| `rule` | One of `rule` or `type` | A rule number. It matches findings from that rule. It never matches a warning that SQL Server wrote into the plan, because no rule produced it. | +| `type` | One of `rule` or `type` | A warning type, such as `Implicit Conversion`. The whole type must match and case does not matter. Use it for warnings that have no rule number. If you give both `rule` and `type`, both must match. | +| `operator` | No | Text that must appear in the operator label. Case does not matter. Use the label from the `operator` field of the JSON output, such as `Key Lookup on eventuous.Messages (Node 1)`. A partial match works, so `eventuous.Messages` is enough. A finding on the statement as a whole has no operator, so a suppression that sets `operator` never matches it. | +| `reason` | Yes | Why the finding is accepted. It must not be empty. | + +A suppression that is missing a field is an error. The command prints one line, for example `Suppression 2 in .planview.json has no "reason".`, and exits with code 1. The number is the zero-based position in the list. + +To find the `query_hash` of a statement, run `planview analyze` with JSON output and read `statements[].query_hash`. Each warning in the JSON also shows its `rule` number when a rule produced it, and its `operator` label when it belongs to an operator. + ## Authentication There are three ways to authenticate, in order of precedence: @@ -582,7 +637,7 @@ The analyzer runs 30 rules against each plan, covering: | Objects | Table variables, table-valued functions, CTE multiple references, eager index spools, lazy spools, row count spools | | Operators | Operator self-time calculation with per-thread awareness for parallel plans | -Rules can be disabled or have their severity overridden via a `.planview.json` config file. See the `--config` option. +Rules can be disabled, have their severity overridden, or have single findings suppressed per statement via a `.planview.json` config file. See [Configuration file](#configuration-file-planviewjson). ## Sponsors diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs index d990431..4def70d 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs @@ -859,7 +859,7 @@ private void ShowPropertiesPanel(PlanNode node) foreach (var w in sortedPlanWarnings) { var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) }; - var legacyTag = w.IsLegacy ? " [legacy]" : ""; + var legacyTag = (w.IsLegacy ? " [legacy]" : "") + SuppressedWarningTag(w); var sourceTag = WarningSourceTag(w); var planWarnHeader = w.MaxBenefitPercent.HasValue ? $"\u26A0 {w.WarningType}{sourceTag}{legacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" @@ -945,7 +945,7 @@ murky origin - but until now the only way to see one was to already have clicked ? $" \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" : ""; var opHeaderText = - $"\u26A0 {w.WarningType}{WarningSourceTag(w)}{(w.IsLegacy ? " [legacy]" : "")}{opBenefit}"; + $"\u26A0 {w.WarningType}{WarningSourceTag(w)}{(w.IsLegacy ? " [legacy]" : "")}{SuppressedWarningTag(w)}{opBenefit}"; var opHeader = new TextBlock { Text = opHeaderText, @@ -1021,7 +1021,7 @@ murky origin - but until now the only way to see one was to already have clicked foreach (var w in sortedNodeWarnings) { var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) }; - var nodeLegacyTag = w.IsLegacy ? " [legacy]" : ""; + var nodeLegacyTag = (w.IsLegacy ? " [legacy]" : "") + SuppressedWarningTag(w); var nodeSourceTag = WarningSourceTag(w); var nodeWarnHeader = w.MaxBenefitPercent.HasValue ? $"\u26A0 {w.WarningType}{nodeSourceTag}{nodeLegacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs index 2c52595..377a77b 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs @@ -19,8 +19,9 @@ public partial class PlanViewerControl : UserControl { private static void CountNodeWarnings(PlanNode node, ref int total, ref int critical) { - total += node.Warnings.Count; - critical += node.Warnings.Count(w => w.Severity == PlanWarningSeverity.Critical); + // Suppressed findings (#651) stay visible but do not count, matching the analysis summary. + total += node.Warnings.Count(w => !w.IsSuppressed); + critical += node.Warnings.Count(w => !w.IsSuppressed && w.Severity == PlanWarningSeverity.Critical); foreach (var child in node.Children) CountNodeWarnings(child, ref total, ref critical); } @@ -569,6 +570,11 @@ cursor and the click below would die in the gaps between words. This is a naviga private static string WarningSourceTag(PlanWarning warning) => warning.Source == PlanWarningSource.SqlServer ? " [SQL Server]" : ""; + private static string SuppressedWarningTag(PlanWarning warning) => + !warning.IsSuppressed ? "" + : string.IsNullOrWhiteSpace(warning.SuppressionReason) ? " [suppressed]" + : $" [suppressed: {warning.SuppressionReason}]"; + private static bool HasSpillInPlanTree(PlanNode node) { foreach (var w in node.Warnings) diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs b/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs index c74f113..1553af2 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs @@ -515,8 +515,9 @@ StatementCount likewise matches the analysis output's total_statements rather th foreach (var entry in everyStatement) { var s = entry.Statement; - warningCount += s.PlanWarnings.Count; - criticalCount += s.PlanWarnings.Count(w => w.Severity == PlanWarningSeverity.Critical); + // Suppressed findings (#651) do not count, matching the analysis summary. + warningCount += s.PlanWarnings.Count(w => !w.IsSuppressed); + criticalCount += s.PlanWarnings.Count(w => !w.IsSuppressed && w.Severity == PlanWarningSeverity.Critical); if (s.RootNode != null) CountNodeWarnings(s.RootNode, ref warningCount, ref criticalCount); } diff --git a/src/PlanViewer.App/Services/AdviceContentBuilder.CardSections.cs b/src/PlanViewer.App/Services/AdviceContentBuilder.CardSections.cs index 3a6f66e..5fbd281 100644 --- a/src/PlanViewer.App/Services/AdviceContentBuilder.CardSections.cs +++ b/src/PlanViewer.App/Services/AdviceContentBuilder.CardSections.cs @@ -205,6 +205,10 @@ private static Border FindingRow(Finding finding) tags.Add("SQL Server"); if (w.IsLegacy) tags.Add("legacy"); + if (w.IsSuppressed) + tags.Add(string.IsNullOrWhiteSpace(w.SuppressionReason) + ? "suppressed" + : $"suppressed: {w.SuppressionReason}"); if (w.MaxBenefitPercent.HasValue) tags.Add($"up to {FormatBenefit(w.MaxBenefitPercent.Value)}% benefit"); if (tags.Count > 0) diff --git a/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs b/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs index d95b719..287d914 100644 --- a/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs +++ b/src/PlanViewer.Cli/Commands/AnalyzeCommand.cs @@ -149,7 +149,17 @@ public static Command Create(ICredentialService? credentialService = null) var configPath = parseResult.GetValue(configOption); // Load analyzer config - var analyzerConfig = ConfigLoader.Load(configPath); + AnalyzerConfig analyzerConfig; + try + { + analyzerConfig = ConfigLoader.Load(configPath); + } + catch (Exception ex) when (ex is System.IO.FileNotFoundException or System.IO.InvalidDataException) + { + Console.Error.WriteLine(ex.Message); + Environment.ExitCode = 1; + return; + } // Load .env file if present (CLI args take precedence) var env = ConnectionHelper.LoadEnvFile(); diff --git a/src/PlanViewer.Cli/Commands/QueryStoreCommand.cs b/src/PlanViewer.Cli/Commands/QueryStoreCommand.cs index 0d49fe5..037cf09 100644 --- a/src/PlanViewer.Cli/Commands/QueryStoreCommand.cs +++ b/src/PlanViewer.Cli/Commands/QueryStoreCommand.cs @@ -334,7 +334,17 @@ public static Command Create(ICredentialService? credentialService = null) }; } - var analyzerConfig = ConfigLoader.Load(configPath); + AnalyzerConfig analyzerConfig; + try + { + analyzerConfig = ConfigLoader.Load(configPath); + } + catch (Exception ex) when (ex is System.IO.FileNotFoundException or System.IO.InvalidDataException) + { + Console.Error.WriteLine(ex.Message); + Environment.ExitCode = 1; + return; + } // Build connection string string connectionString; diff --git a/src/PlanViewer.Core/Models/AnalyzerConfig.cs b/src/PlanViewer.Core/Models/AnalyzerConfig.cs index 95d5eec..4849e7a 100644 --- a/src/PlanViewer.Core/Models/AnalyzerConfig.cs +++ b/src/PlanViewer.Core/Models/AnalyzerConfig.cs @@ -8,6 +8,13 @@ public class AnalyzerConfig [JsonPropertyName("rules")] public RulesConfig? Rules { get; set; } + /// + /// Findings to mark as accepted (#651). Matched on the statement's query hash plus a rule + /// number or warning type. A suppressed finding stays in the output, flagged, with its reason. + /// + [JsonPropertyName("suppressions")] + public List? Suppressions { get; set; } + public static AnalyzerConfig Default => new(); public bool IsRuleDisabled(int ruleNumber) @@ -24,6 +31,36 @@ public bool IsRuleDisabled(int ruleNumber) } } +public class SuppressionEntry +{ + [JsonPropertyName("query_hash")] + public string? QueryHash { get; set; } + + /// Matches , so it never matches an engine warning. + [JsonPropertyName("rule")] + public int? Rule { get; set; } + + /// Matches exactly, ignoring case. + [JsonPropertyName("type")] + public string? Type { get; set; } + + /// Optional. Substring of the operator label the JSON output prints, ignoring case. + [JsonPropertyName("operator")] + public string? Operator { get; set; } + + [JsonPropertyName("reason")] + public string? Reason { get; set; } + + /// Trims and drops a leading 0x or 0X so both sides of a hash comparison look alike. + public static string NormalizeHash(string? hash) + { + var h = (hash ?? "").Trim(); + if (h.StartsWith("0x", System.StringComparison.OrdinalIgnoreCase)) + h = h.Substring(2); + return h; + } +} + public class RulesConfig { [JsonPropertyName("disabled")] diff --git a/src/PlanViewer.Core/Models/PlanModels.cs b/src/PlanViewer.Core/Models/PlanModels.cs index df5b4bd..9966728 100644 --- a/src/PlanViewer.Core/Models/PlanModels.cs +++ b/src/PlanViewer.Core/Models/PlanModels.cs @@ -457,6 +457,15 @@ public class PlanWarning /// Renderers show a "legacy" badge when true. /// public bool IsLegacy { get; set; } + + /// + /// True when a suppression in .planview.json matched this finding (#651). The warning is kept, + /// never removed, so a reader can still see it and why it was set aside. + /// + public bool IsSuppressed { get; set; } + + /// The "reason" from the matching suppression. Null unless . + public string? SuppressionReason { get; set; } } public enum PlanWarningSeverity { Info, Warning, Critical } diff --git a/src/PlanViewer.Core/Output/AnalysisResult.cs b/src/PlanViewer.Core/Output/AnalysisResult.cs index e8fa75a..2943249 100644 --- a/src/PlanViewer.Core/Output/AnalysisResult.cs +++ b/src/PlanViewer.Core/Output/AnalysisResult.cs @@ -39,6 +39,13 @@ public class AnalysisSummary [JsonPropertyName("critical_warnings")] public int CriticalWarnings { get; set; } + /// + /// Findings a suppression in .planview.json matched (#651). They are left out of + /// total_warnings, critical_warnings and warning_types. + /// + [JsonPropertyName("suppressed_warnings")] + public int SuppressedWarnings { get; set; } + [JsonPropertyName("missing_indexes")] public int MissingIndexes { get; set; } @@ -270,6 +277,20 @@ public class WarningResult [JsonPropertyName("is_legacy")] public bool IsLegacy { get; set; } + /// + /// The analyzer rule number that produced this finding. Omitted for the engine's own warnings + /// and for the wait-stats findings, which no numbered rule produces. + /// + [JsonPropertyName("rule")] + public int? Rule { get; set; } + + /// True when a suppression in .planview.json matched this finding (#651). + [JsonPropertyName("suppressed")] + public bool IsSuppressed { get; set; } + + [JsonPropertyName("suppression_reason")] + public string? SuppressionReason { 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 diff --git a/src/PlanViewer.Core/Output/HtmlExporter.cs b/src/PlanViewer.Core/Output/HtmlExporter.cs index c164897..718e60f 100644 --- a/src/PlanViewer.Core/Output/HtmlExporter.cs +++ b/src/PlanViewer.Core/Output/HtmlExporter.cs @@ -194,6 +194,7 @@ .statement h2 { .warn-type { font-size: 0.75rem; font-weight: 600; } .warn-benefit { font-size: 0.7rem; font-weight: 600; color: var(--text-muted); padding: 0.05rem 0.3rem; border-radius: 3px; background: rgba(0,0,0,0.04); } .warn-msg { font-size: 0.8rem; color: var(--text); flex-basis: 100%; } +.warn-suppressed { font-size: 0.65rem; font-weight: 600; color: var(--text-muted); padding: 0.05rem 0.3rem; border-radius: 3px; background: rgba(0,0,0,0.08); letter-spacing: 0.02em; } .warn-legacy { font-size: 0.65rem; font-weight: 600; color: var(--text-muted); padding: 0.05rem 0.3rem; border-radius: 3px; background: rgba(0,0,0,0.08); text-transform: uppercase; letter-spacing: 0.05em; } .warn-fix { font-size: 0.75rem; color: var(--text-secondary); font-style: italic; flex-basis: 100%; border-left: 2px solid var(--border); padding-left: 0.5rem; margin-top: 0.15rem; } .spill-tag { font-size: 0.75rem; font-weight: 600; color: var(--orange); margin-left: 0.4rem; } @@ -493,9 +494,10 @@ private static void WriteWarnings(StringBuilder sb, StatementResult stmt) if (allWarnings.Count == 0) return; - var critCount = allWarnings.Count(w => w.Severity == "Critical"); - var warnCount = allWarnings.Count(w => w.Severity == "Warning"); - var infoCount = allWarnings.Count(w => w.Severity == "Info"); + // Suppressed findings (#651) are listed below but left out of the badge counts. + var critCount = allWarnings.Count(w => !w.IsSuppressed && w.Severity == "Critical"); + var warnCount = allWarnings.Count(w => !w.IsSuppressed && w.Severity == "Warning"); + var infoCount = allWarnings.Count(w => !w.IsSuppressed && w.Severity == "Info"); sb.AppendLine("
"); sb.Append("

Warnings"); @@ -522,6 +524,8 @@ private static void WriteWarnings(StringBuilder sb, StatementResult stmt) sb.AppendLine($"{Encode(w.Type)}"); if (w.IsLegacy) sb.AppendLine("legacy"); + if (w.IsSuppressed) + sb.AppendLine($"suppressed{(string.IsNullOrWhiteSpace(w.SuppressionReason) ? "" : ": " + Encode(w.SuppressionReason))}"); if (w.MaxBenefitPercent.HasValue) sb.AppendLine($"up to {(w.MaxBenefitPercent.Value >= 100 ? w.MaxBenefitPercent.Value.ToString("N0") : w.MaxBenefitPercent.Value.ToString("N1"))}% benefit"); sb.AppendLine($"{Encode(w.Message)}"); diff --git a/src/PlanViewer.Core/Output/PlanOperationResults.cs b/src/PlanViewer.Core/Output/PlanOperationResults.cs index a9e219d..d5cd55d 100644 --- a/src/PlanViewer.Core/Output/PlanOperationResults.cs +++ b/src/PlanViewer.Core/Output/PlanOperationResults.cs @@ -32,6 +32,9 @@ public sealed class PlanSummaryResult [JsonPropertyName("critical_warnings")] public int CriticalWarnings { get; init; } + [JsonPropertyName("suppressed_warnings")] + public int SuppressedWarnings { get; init; } + [JsonPropertyName("missing_indexes")] public int MissingIndexes { get; init; } @@ -83,6 +86,15 @@ public sealed class PlanWarningItem [JsonPropertyName("statement")] public string Statement { get; init; } = ""; + + [JsonPropertyName("rule")] + public int? Rule { get; init; } + + [JsonPropertyName("suppressed")] + public bool IsSuppressed { get; init; } + + [JsonPropertyName("suppression_reason")] + public string? SuppressionReason { get; init; } } diff --git a/src/PlanViewer.Core/Output/ResultMapper.cs b/src/PlanViewer.Core/Output/ResultMapper.cs index 24983dd..3a1c441 100644 --- a/src/PlanViewer.Core/Output/ResultMapper.cs +++ b/src/PlanViewer.Core/Output/ResultMapper.cs @@ -252,7 +252,10 @@ describe the one thing this result no longer shows — a shortened statement — Message = message, MaxBenefitPercent = w.MaxBenefitPercent, ActionableFix = w.ActionableFix, + Rule = w.RuleNumber, IsLegacy = w.IsLegacy, + IsSuppressed = w.IsSuppressed, + SuppressionReason = w.SuppressionReason, Source = w.Source.ToString(), OriginNodeIds = w.OriginNodeIds }); @@ -381,11 +384,14 @@ private static OperatorResult MapSingleNode(PlanNode node, CancellationToken can Type = w.WarningType, Severity = w.Severity.ToString(), Message = w.Message, - Operator = FormatOperatorLabel(node), + Operator = OperatorLabel.Format(node), NodeId = node.NodeId, MaxBenefitPercent = w.MaxBenefitPercent, ActionableFix = w.ActionableFix, + Rule = w.RuleNumber, IsLegacy = w.IsLegacy, + IsSuppressed = w.IsSuppressed, + SuppressionReason = w.SuppressionReason, Source = w.Source.ToString(), OriginNodeIds = w.OriginNodeIds }); @@ -407,17 +413,21 @@ private static AnalysisSummary BuildSummary( CollectNodeWarnings(stmt.OperatorTree, allWarnings, cancellationToken); } + // Suppressed findings (#651) stay in the output but do not count toward the totals. + var activeWarnings = allWarnings.Where(w => !w.IsSuppressed).ToList(); + return new AnalysisSummary { TotalStatements = result.Statements.Count, - TotalWarnings = allWarnings.Count, - CriticalWarnings = allWarnings.Count(w => w.Severity == "Critical"), + TotalWarnings = activeWarnings.Count, + CriticalWarnings = activeWarnings.Count(w => w.Severity == "Critical"), + SuppressedWarnings = allWarnings.Count - activeWarnings.Count, MissingIndexes = result.Statements.Sum(s => s.MissingIndexes.Count), HasActualStats = result.Statements.Any(s => s.QueryTime != null), MaxEstimatedCost = result.Statements.Count > 0 ? result.Statements.Max(s => s.EstimatedCost) : 0, - WarningTypes = allWarnings.Select(w => w.Type).Distinct().OrderBy(t => t).ToList() + WarningTypes = activeWarnings.Select(w => w.Type).Distinct().OrderBy(t => t).ToList() }; } @@ -440,21 +450,4 @@ private static void CollectNodeWarnings( } } - /// - /// Formats an operator label for the Operator field on warnings. - /// Includes object name for data access operators (scans, seeks, lookups) - /// where it helps identify which table/index is involved. - /// - private static string FormatOperatorLabel(PlanNode node) - { - if (!string.IsNullOrEmpty(node.ObjectName)) - { - var objRef = !string.IsNullOrEmpty(node.DatabaseName) - ? $"{node.DatabaseName}.{node.ObjectName}" - : node.ObjectName; - return $"{node.PhysicalOp} on {objRef} (Node {node.NodeId})"; - } - - return $"{node.PhysicalOp} (Node {node.NodeId})"; - } } diff --git a/src/PlanViewer.Core/Output/TextFormatter.cs b/src/PlanViewer.Core/Output/TextFormatter.cs index 5e6ad25..bf623a9 100644 --- a/src/PlanViewer.Core/Output/TextFormatter.cs +++ b/src/PlanViewer.Core/Output/TextFormatter.cs @@ -36,6 +36,8 @@ internal static void WriteTextCancellable( writer.WriteLine("=== Summary ==="); writer.WriteLine($"Statements: {result.Summary.TotalStatements}"); writer.WriteLine($"Warnings: {result.Summary.TotalWarnings} ({result.Summary.CriticalWarnings} critical)"); + if (result.Summary.SuppressedWarnings > 0) + writer.WriteLine($"Suppressed warnings: {result.Summary.SuppressedWarnings}"); writer.WriteLine($"Missing indexes: {result.Summary.MissingIndexes}"); writer.WriteLine($"Actual stats: {(result.Summary.HasActualStats ? "yes" : "no (estimated plan)")}"); if (result.Summary.WarningTypes.Count > 0) @@ -193,7 +195,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}{SourceTag(w.Source)}{legacyTag}{benefitTag}: {EscapeNewlines(w.Message)}"); + writer.WriteLine($" [{w.Severity}] {w.Type}{SourceTag(w.Source)}{legacyTag}{SuppressedTag(w)}{benefitTag}: {EscapeNewlines(w.Message)}"); if (!string.IsNullOrEmpty(w.ActionableFix)) writer.WriteLine($" Fix: {EscapeNewlines(w.ActionableFix)}"); } @@ -339,7 +341,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, string Source)>(); + var entries = new List<(string Severity, string Operator, string Data, string? Explanation, double? Benefit, bool IsLegacy, string Source, string SuppressedTag)>(); foreach (var w in sorted) { var msg = w.Message; @@ -358,7 +360,7 @@ private static void WriteGroupedOperatorWarnings( data = msg; } - entries.Add((w.Severity, w.Operator ?? "?", data, explanation, w.MaxBenefitPercent, w.IsLegacy, w.Source)); + entries.Add((w.Severity, w.Operator ?? "?", data, explanation, w.MaxBenefitPercent, w.IsLegacy, w.Source, SuppressedTag(w))); } // Group entries that share the same severity, type, and explanation @@ -380,7 +382,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}{SourceTag(item.Source)}{legacyTag}{benefitTag}: {EscapeNewlines(item.Data)}"); + writer.WriteLine($" [{item.Severity}] {item.Operator}{SourceTag(item.Source)}{legacyTag}{item.SuppressedTag}{benefitTag}: {EscapeNewlines(item.Data)}"); } writer.WriteLine($" -> {group.Key.Item2}"); } @@ -394,7 +396,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}{SourceTag(item.Source)}{legacyTag}{benefitTag}: {EscapeNewlines(full)}"); + writer.WriteLine($" [{item.Severity}] {item.Operator}{SourceTag(item.Source)}{legacyTag}{item.SuppressedTag}{benefitTag}: {EscapeNewlines(full)}"); } } } @@ -428,6 +430,14 @@ public static string FormatMemoryGrantKB(long kb) private static string SourceTag(string? source) => source == nameof(PlanViewer.Core.Models.PlanWarningSource.SqlServer) ? " [SQL Server]" : ""; + /// The " [suppressed: reason]" tag for a finding a .planview.json suppression matched (#651). + private static string SuppressedTag(WarningResult w) => + w.IsSuppressed + ? string.IsNullOrWhiteSpace(w.SuppressionReason) + ? " [suppressed]" + : $" [suppressed: {EscapeNewlines(w.SuppressionReason)}]" + : ""; + private static string EscapeNewlines(string text) => text.Replace('\n', '\x1F'); private static void CollectNodeTimings( diff --git a/src/PlanViewer.Core/Services/ConfigLoader.cs b/src/PlanViewer.Core/Services/ConfigLoader.cs index 8eff3b7..09b041d 100644 --- a/src/PlanViewer.Core/Services/ConfigLoader.cs +++ b/src/PlanViewer.Core/Services/ConfigLoader.cs @@ -54,7 +54,54 @@ public static AnalyzerConfig Load(string? explicitPath = null) return AnalyzerConfig.Default; var json = File.ReadAllText(configPath); - var config = JsonSerializer.Deserialize(json, JsonOptions); - return config ?? AnalyzerConfig.Default; + AnalyzerConfig? config; + try + { + config = JsonSerializer.Deserialize(json, JsonOptions); + } + catch (JsonException ex) + { + throw new InvalidDataException($"Could not read {configPath}: {ex.Message}", ex); + } + + if (config == null) + return AnalyzerConfig.Default; + + ValidateSuppressions(config); + return config; + } + + /// + /// A suppression that cannot match anything, or that hides a finding without saying why, + /// is a mistake worth stopping for. Throws naming the + /// entry by its zero-based index (#651). + /// + private static void ValidateSuppressions(AnalyzerConfig config) + { + var entries = config.Suppressions; + if (entries == null) + return; + + for (var i = 0; i < entries.Count; i++) + { + var entry = entries[i]; + var prefix = $"Suppression {i} in {ConfigFileName}"; + + if (entry == null) + throw new InvalidDataException($"{prefix} is empty."); + + if (SuppressionEntry.NormalizeHash(entry.QueryHash).Length == 0) + throw new InvalidDataException($"{prefix} has an empty \"query_hash\"."); + + var hasType = !string.IsNullOrWhiteSpace(entry.Type); + if (entry.Rule == null && !hasType) + throw new InvalidDataException($"{prefix} needs \"rule\" or \"type\"."); + + if (entry.Operator != null && string.IsNullOrWhiteSpace(entry.Operator)) + throw new InvalidDataException($"{prefix} has an empty \"operator\"."); + + if (string.IsNullOrWhiteSpace(entry.Reason)) + throw new InvalidDataException($"{prefix} has no \"reason\"."); + } } } diff --git a/src/PlanViewer.Core/Services/OperatorLabel.cs b/src/PlanViewer.Core/Services/OperatorLabel.cs new file mode 100644 index 0000000..ae57c5f --- /dev/null +++ b/src/PlanViewer.Core/Services/OperatorLabel.cs @@ -0,0 +1,28 @@ +using PlanViewer.Core.Models; + +namespace PlanViewer.Core.Services; + +/// +/// The one place that spells an operator label. The JSON output prints it in the Operator field +/// of a warning, and a suppression's "operator" text is matched against it (#651), so the two +/// share this method and can never drift. +/// +internal static class OperatorLabel +{ + /// + /// Includes the object name for data access operators (scans, seeks, lookups) where it helps + /// identify which table or index is involved. + /// + internal static string Format(PlanNode node) + { + if (!string.IsNullOrEmpty(node.ObjectName)) + { + var objRef = !string.IsNullOrEmpty(node.DatabaseName) + ? $"{node.DatabaseName}.{node.ObjectName}" + : node.ObjectName; + return $"{node.PhysicalOp} on {objRef} (Node {node.NodeId})"; + } + + return $"{node.PhysicalOp} (Node {node.NodeId})"; + } +} diff --git a/src/PlanViewer.Core/Services/PlanAnalysisPipeline.cs b/src/PlanViewer.Core/Services/PlanAnalysisPipeline.cs index f13b80d..bfc2a5f 100644 --- a/src/PlanViewer.Core/Services/PlanAnalysisPipeline.cs +++ b/src/PlanViewer.Core/Services/PlanAnalysisPipeline.cs @@ -71,6 +71,8 @@ internal static ParsedPlan AnalyzeParsed( cancellationToken.ThrowIfCancellationRequested(); BenefitScorer.ScoreCancellable(plan, cancellationToken); cancellationToken.ThrowIfCancellationRequested(); + // Last, so the scorer's wait-stats findings can be suppressed too (#651). + SuppressionApplier.Apply(plan, config); return plan; } } diff --git a/src/PlanViewer.Core/Services/PlanOperations.cs b/src/PlanViewer.Core/Services/PlanOperations.cs index f740664..2a0967c 100644 --- a/src/PlanViewer.Core/Services/PlanOperations.cs +++ b/src/PlanViewer.Core/Services/PlanOperations.cs @@ -312,6 +312,7 @@ public PlanSummaryResult GetSummary(PlanSession session) TotalStatements = analysis.Summary.TotalStatements, TotalWarnings = analysis.Summary.TotalWarnings, CriticalWarnings = analysis.Summary.CriticalWarnings, + SuppressedWarnings = analysis.Summary.SuppressedWarnings, MissingIndexes = analysis.Summary.MissingIndexes, HasActualStats = analysis.Summary.HasActualStats, MaxEstimatedCost = analysis.Summary.MaxEstimatedCost, @@ -685,7 +686,12 @@ private static void AddRanked( Message = Truncate(warning.Message, DefaultMaxResponseTextLength), NodeId = warning.NodeId, Operator = warning.Operator is null ? null : Truncate(warning.Operator, 512), - Statement = statement + Statement = statement, + Rule = warning.Rule, + IsSuppressed = warning.IsSuppressed, + SuppressionReason = warning.SuppressionReason is null + ? null + : Truncate(warning.SuppressionReason, DefaultMaxResponseTextLength) }; private static BoundedColumnList BoundColumns(IEnumerable columns) diff --git a/src/PlanViewer.Core/Services/SuppressionApplier.cs b/src/PlanViewer.Core/Services/SuppressionApplier.cs new file mode 100644 index 0000000..294e38a --- /dev/null +++ b/src/PlanViewer.Core/Services/SuppressionApplier.cs @@ -0,0 +1,93 @@ +using System; +using System.Collections.Generic; +using PlanViewer.Core.Models; + +namespace PlanViewer.Core.Services; + +/// +/// Flags findings that a suppression in .planview.json matches (#651). It runs after the +/// scorer so the scorer's wait-stats findings can be suppressed too. It only sets +/// and : +/// no warning is removed and no severity changes. +/// +internal static class SuppressionApplier +{ + internal static void Apply(ParsedPlan plan, AnalyzerConfig config) + { + var entries = config.Suppressions; + if (entries == null || entries.Count == 0) + return; + + foreach (var stmt in PlanStatements.EnumerateAll(plan)) + { + var hash = SuppressionEntry.NormalizeHash(stmt.QueryHash); + if (hash.Length == 0) + continue; + + var forStatement = new List(); + foreach (var entry in entries) + { + if (entry != null && + string.Equals(SuppressionEntry.NormalizeHash(entry.QueryHash), hash, StringComparison.OrdinalIgnoreCase)) + forStatement.Add(entry); + } + if (forStatement.Count == 0) + continue; + + foreach (var w in stmt.PlanWarnings) + TryApply(w, null, forStatement); + + if (stmt.RootNode != null) + ApplyToTree(stmt.RootNode, forStatement); + } + } + + private static void ApplyToTree(PlanNode root, List entries) + { + var pending = new Stack(); + pending.Push(root); + while (pending.TryPop(out var node)) + { + if (node.Warnings.Count > 0) + { + var label = OperatorLabel.Format(node); + foreach (var w in node.Warnings) + TryApply(w, label, entries); + } + foreach (var child in node.Children) + pending.Push(child); + } + } + + private static void TryApply(PlanWarning warning, string? operatorLabel, List entries) + { + foreach (var entry in entries) + { + if (!Matches(warning, operatorLabel, entry)) + continue; + warning.IsSuppressed = true; + warning.SuppressionReason = entry.Reason; + return; + } + } + + private static bool Matches(PlanWarning warning, string? operatorLabel, SuppressionEntry entry) + { + if (entry.Rule is int rule && warning.RuleNumber != rule) + return false; + + if (!string.IsNullOrWhiteSpace(entry.Type) && + !string.Equals(warning.WarningType, entry.Type.Trim(), StringComparison.OrdinalIgnoreCase)) + return false; + + if (!string.IsNullOrWhiteSpace(entry.Operator)) + { + if (operatorLabel == null) + return false; + if (operatorLabel.IndexOf(entry.Operator.Trim(), StringComparison.OrdinalIgnoreCase) < 0) + return false; + } + + return true; + } +} diff --git a/tests/PlanViewer.Core.Tests/ConfigLoaderTests.cs b/tests/PlanViewer.Core.Tests/ConfigLoaderTests.cs new file mode 100644 index 0000000..2c83bd4 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/ConfigLoaderTests.cs @@ -0,0 +1,121 @@ +using System; +using System.IO; +using PlanViewer.Core.Models; +using PlanViewer.Core.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// #651: the .planview.json loader. A suppression that cannot do its job, or that hides a finding +/// without saying why, stops the load with one line naming the entry by its zero-based index. +/// +public class ConfigLoaderTests : IDisposable +{ + private readonly string _dir = Path.Combine(Path.GetTempPath(), "planview-config-" + Guid.NewGuid().ToString("N")); + + public ConfigLoaderTests() => Directory.CreateDirectory(_dir); + + public void Dispose() + { + try { Directory.Delete(_dir, recursive: true); } catch (IOException) { } + } + + private string Write(string json) + { + var path = Path.Combine(_dir, ".planview.json"); + File.WriteAllText(path, json); + return path; + } + + [Fact] + public void AValidSuppressionsListRoundTrips() + { + var config = ConfigLoader.Load(Write(""" + { + // comments and trailing commas are allowed + "rules": { "disabled": [3], "severity_overrides": { "10": "Info" } }, + "suppressions": [ + { "query_hash": "0x00E1987CD1C28B4C", "rule": 10, "operator": "eventuous.Messages", "reason": "Library owns the index." }, + { "query_hash": "00e1987cd1c28b4c", "type": "Implicit Conversion", "reason": "Vendor column type." }, + ] + } + """)); + + Assert.True(config.IsRuleDisabled(3)); + Assert.Equal("Info", config.GetSeverityOverride(10)); + + var suppressions = Assert.IsType>(config.Suppressions); + Assert.Equal(2, suppressions.Count); + Assert.Equal("0x00E1987CD1C28B4C", suppressions[0].QueryHash); + Assert.Equal(10, suppressions[0].Rule); + Assert.Equal("eventuous.Messages", suppressions[0].Operator); + Assert.Equal("Library owns the index.", suppressions[0].Reason); + Assert.Null(suppressions[0].Type); + Assert.Equal("Implicit Conversion", suppressions[1].Type); + Assert.Null(suppressions[1].Rule); + Assert.Null(suppressions[1].Operator); + } + + [Fact] + public void AFileWithNoSuppressionsKeyStillLoads() + { + var config = ConfigLoader.Load(Write("""{ "rules": { "disabled": [5] } }""")); + + Assert.True(config.IsRuleDisabled(5)); + Assert.Null(config.Suppressions); + } + + [Fact] + public void AnEmptySuppressionsListLoads() + { + var config = ConfigLoader.Load(Write("""{ "suppressions": [] }""")); + Assert.Empty(config.Suppressions!); + } + + [Theory] + [InlineData("""{ "suppressions": [ { "query_hash": "0xAB", "rule": 3, "reason": "ok" }, { "query_hash": "0xAB", "rule": 4, "reason": "ok" }, { "query_hash": "0xAB", "rule": 5 } ] }""", + "Suppression 2 in .planview.json has no \"reason\".")] + [InlineData("""{ "suppressions": [ { "query_hash": "0xAB", "rule": 3, "reason": " " } ] }""", + "Suppression 0 in .planview.json has no \"reason\".")] + [InlineData("""{ "suppressions": [ { "query_hash": "0xAB", "rule": 3, "reason": "ok" }, { "query_hash": "", "rule": 3, "reason": "ok" } ] }""", + "Suppression 1 in .planview.json has an empty \"query_hash\".")] + [InlineData("""{ "suppressions": [ { "rule": 3, "reason": "ok" } ] }""", + "Suppression 0 in .planview.json has an empty \"query_hash\".")] + [InlineData("""{ "suppressions": [ { "query_hash": "0x", "rule": 3, "reason": "ok" } ] }""", + "Suppression 0 in .planview.json has an empty \"query_hash\".")] + [InlineData("""{ "suppressions": [ { "query_hash": "0xAB", "reason": "ok" } ] }""", + "Suppression 0 in .planview.json needs \"rule\" or \"type\".")] + [InlineData("""{ "suppressions": [ { "query_hash": "0xAB", "type": " ", "reason": "ok" } ] }""", + "Suppression 0 in .planview.json needs \"rule\" or \"type\".")] + [InlineData("""{ "suppressions": [ { "query_hash": "0xAB", "rule": 3, "operator": "", "reason": "ok" } ] }""", + "Suppression 0 in .planview.json has an empty \"operator\".")] + [InlineData("""{ "suppressions": [ null ] }""", + "Suppression 0 in .planview.json is empty.")] + public void AnInvalidSuppressionFailsWithOneLineNamingTheEntry(string json, string expected) + { + var ex = Assert.Throws(() => ConfigLoader.Load(Write(json))); + Assert.Equal(expected, ex.Message); + } + + [Fact] + public void MalformedJsonFailsWithAMessageNotAStackTrace() + { + var path = Write("""{ "suppressions": [ { "query_hash": """); + var ex = Assert.Throws(() => ConfigLoader.Load(path)); + Assert.Contains(path, ex.Message); + Assert.DoesNotContain(Environment.NewLine, ex.Message); + } + + [Fact] + public void ARuleThatIsNotANumberFailsAsAnInvalidFile() + { + Assert.Throws(() => ConfigLoader.Load(Write( + """{ "suppressions": [ { "query_hash": "0xAB", "rule": "ten", "reason": "ok" } ] }"""))); + } + + [Fact] + public void AMissingExplicitPathStillThrowsFileNotFound() + { + Assert.Throws(() => ConfigLoader.Load(Path.Combine(_dir, "nope.json"))); + } +} diff --git a/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs b/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs index d0d809a..911ec52 100644 --- a/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs +++ b/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs @@ -12,9 +12,14 @@ every warning so a consumer can tell SQL Server's own warnings from Performance operator that produced it. Nothing has been removed or renamed either time, 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 exists to make somebody decide on rather - than discover. Verified for #440 that the only new key on a warning is origin_node_ids. */ + than discover. Verified for #440 that the only new key on a warning is origin_node_ids. + + Rolled a third time for #651, again additively: every warning gains "suppressed" (always + written) and "rule" (written when a numbered rule produced it), and the summary gains + "suppressed_warnings". Verified by removing exactly those keys from the new output, which + gives back the previous hash, 779a85036b74dc7cc7d26d4bad3d7e2b327e4ff89a5ed79b00b78cfae3717cab. */ private const string ExpectedCompactOutputSha256 = - "779a85036b74dc7cc7d26d4bad3d7e2b327e4ff89a5ed79b00b78cfae3717cab"; + "0eeb5da0bc69ceb162100f4326eee446e07234b4cc1b6660dda19221f951f01a"; [Fact] public async Task AnalyzeCompact_PreservesHistoricalOutputBytes() diff --git a/tests/PlanViewer.Core.Tests/SuppressionTests.cs b/tests/PlanViewer.Core.Tests/SuppressionTests.cs new file mode 100644 index 0000000..8052e14 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/SuppressionTests.cs @@ -0,0 +1,431 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text.Json; +using PlanViewer.App.Mcp; +using AnalyzerConfig = PlanViewer.Core.Models.AnalyzerConfig; +using CorePlanSession = PlanViewer.Core.Models.PlanSession; +using PlanViewer.Core.Models; +using PlanViewer.Core.Output; +using PlanViewer.Core.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// #651: a suppression in .planview.json marks one finding on one statement as accepted. The +/// finding stays in the output, flagged with its reason, and drops out of the summary counts. +/// +public class SuppressionTests +{ + private const string Hash = "0x00E1987CD1C28B4C"; + + private static PlanWarning Warn( + string type, + int? rule, + PlanWarningSeverity severity = PlanWarningSeverity.Warning, + PlanWarningSource source = PlanWarningSource.PerformanceStudio) => + new() + { + WarningType = type, + Message = type + " message", + Severity = severity, + RuleNumber = rule, + Source = source + }; + + private static PlanNode Node(int id, string physicalOp, string? objectName, params PlanWarning[] warnings) => + new() + { + NodeId = id, + PhysicalOp = physicalOp, + ObjectName = objectName, + Warnings = warnings.ToList() + }; + + /// + /// One statement. The statement carries a rule finding and an engine warning. The tree is a + /// Nested Loops (node 0) over two Key Lookups of the same type, on different tables (nodes 1 + /// and 2). Node 2 also carries the engine warning. + /// + private static ParsedPlan SamplePlan(string hash = Hash) + { + var root = Node(0, "Nested Loops", null); + root.Children.Add(Node(1, "Key Lookup", "eventuous.Messages", Warn("Key Lookup", 10))); + root.Children.Add(Node(2, "Key Lookup", "dbo.Orders", + Warn("Key Lookup", 10, PlanWarningSeverity.Critical), + Warn("Implicit Conversion", null, source: PlanWarningSource.SqlServer))); + + var stmt = new PlanStatement + { + QueryHash = hash, + RootNode = root, + PlanWarnings = + [ + Warn("Dynamic Cursor", 36), + Warn("Implicit Conversion", null, source: PlanWarningSource.SqlServer) + ] + }; + return new ParsedPlan { Batches = [new PlanBatch { Statements = [stmt] }] }; + } + + private static AnalyzerConfig Cfg(params SuppressionEntry[] entries) => + new() { Suppressions = entries.ToList() }; + + private static SuppressionEntry Entry( + string hash = Hash, int? rule = null, string? type = null, string? op = null, string reason = "accepted") => + new() { QueryHash = hash, Rule = rule, Type = type, Operator = op, Reason = reason }; + + private static ParsedPlan Suppress(ParsedPlan plan, params SuppressionEntry[] entries) + { + SuppressionApplier.Apply(plan, Cfg(entries)); + return plan; + } + + private static List Suppressed(ParsedPlan plan) => + PlanTestHelper.AllWarnings(plan).Where(w => w.IsSuppressed).ToList(); + + [Fact] + public void RuleMatchesEveryFindingOfThatRuleOnTheStatement() + { + var plan = Suppress(SamplePlan(), Entry(rule: 10)); + + var suppressed = Suppressed(plan); + Assert.Equal(2, suppressed.Count); + Assert.All(suppressed, w => Assert.Equal(10, w.RuleNumber)); + Assert.All(suppressed, w => Assert.Equal("accepted", w.SuppressionReason)); + } + + [Fact] + public void RuleNeverMatchesAnEngineWarning() + { + var plan = Suppress(SamplePlan(), Entry(rule: 10)); + + var engine = PlanTestHelper.AllWarnings(plan).Where(w => w.Source == PlanWarningSource.SqlServer); + Assert.All(engine, w => Assert.False(w.IsSuppressed)); + } + + [Fact] + public void TypeMatchesAWarningThatHasNoRuleNumber() + { + var plan = Suppress(SamplePlan(), Entry(type: "implicit conversion")); + + var suppressed = Suppressed(plan); + Assert.Equal(2, suppressed.Count); + Assert.All(suppressed, w => Assert.Null(w.RuleNumber)); + Assert.All(suppressed, w => Assert.Equal("Implicit Conversion", w.WarningType)); + } + + [Fact] + public void RuleAndTypeBothHaveToMatch() + { + var wrongType = Suppress(SamplePlan(), Entry(rule: 10, type: "Dynamic Cursor")); + Assert.Empty(Suppressed(wrongType)); + + var rightType = Suppress(SamplePlan(), Entry(rule: 10, type: "Key Lookup")); + Assert.Equal(2, Suppressed(rightType).Count); + } + + [Theory] + [InlineData("0x00E1987CD1C28B4C")] + [InlineData("0X00E1987CD1C28B4C")] + [InlineData("00E1987CD1C28B4C")] + [InlineData("0x00e1987cd1c28b4c")] + [InlineData("00e1987cd1c28b4c")] + [InlineData(" 0x00e1987cd1c28b4c ")] + public void HashMatchesWithOrWithoutThePrefixInEitherCase(string configured) + { + var plan = Suppress(SamplePlan("0x00E1987CD1C28B4C"), Entry(hash: configured, rule: 36)); + Assert.Single(Suppressed(plan)); + } + + [Theory] + [InlineData("00e1987cd1c28b4c")] + [InlineData("0X00E1987CD1C28B4C")] + public void HashMatchesWhenThePlanSideDiffersInShape(string planHash) + { + var plan = Suppress(SamplePlan(planHash), Entry(hash: "0x00E1987CD1C28B4C", rule: 36)); + Assert.Single(Suppressed(plan)); + } + + [Fact] + public void AnotherStatementsHashIsLeftAlone() + { + var plan = Suppress(SamplePlan("0x1111111111111111"), Entry(rule: 10)); + Assert.Empty(Suppressed(plan)); + } + + [Fact] + public void AStatementWithNoHashIsNeverMatched() + { + var plan = Suppress(SamplePlan(hash: null!), Entry(rule: 10)); + Assert.Empty(Suppressed(plan)); + } + + [Fact] + public void OperatorMatchesOnlyTheNodeWhoseLabelContainsIt() + { + var plan = Suppress(SamplePlan(), Entry(rule: 10, op: "eventuous.Messages")); + + var suppressed = Assert.Single(Suppressed(plan)); + var root = plan.Batches[0].Statements[0].RootNode!; + Assert.Same(suppressed, root.Children[0].Warnings.Single()); + // Node 2 holds the same type of finding and is untouched. + Assert.False(root.Children[1].Warnings.First(w => w.RuleNumber == 10).IsSuppressed); + } + + [Fact] + public void OperatorIsMatchedAgainstTheLabelTheJsonOutputPrints() + { + var plan = SamplePlan(); + var result = ResultMapper.Map(plan, "x"); + var printed = result.Statements[0].OperatorTree!.Children[0].Warnings.Single().Operator!; + + // Pasting the printed label into a suppression has to hit that node and no other. + Suppress(plan, Entry(rule: 10, op: printed)); + + Assert.Single(Suppressed(plan)); + Assert.Equal("Key Lookup on eventuous.Messages (Node 1)", printed); + } + + [Fact] + public void OperatorIsCaseInsensitive() + { + var plan = Suppress(SamplePlan(), Entry(rule: 10, op: "DBO.ORDERS")); + Assert.Single(Suppressed(plan)); + } + + [Fact] + public void OperatorNeverMatchesAStatementLevelWarning() + { + var plan = Suppress(SamplePlan(), Entry(rule: 36, op: "Nested Loops")); + Assert.Empty(Suppressed(plan)); + + var withoutOperator = Suppress(SamplePlan(), Entry(rule: 36)); + Assert.Single(Suppressed(withoutOperator)); + } + + [Fact] + public void ASuppressionKeepsTheWarningAndItsSeverity() + { + var plan = SamplePlan(); + Suppress(plan, Entry(rule: 10, op: "dbo.Orders")); + + var warning = plan.Batches[0].Statements[0].RootNode!.Children[1].Warnings.First(w => w.RuleNumber == 10); + Assert.True(warning.IsSuppressed); + Assert.Equal(PlanWarningSeverity.Critical, warning.Severity); + Assert.Equal(5, PlanTestHelper.AllWarnings(plan).Count); + } + + [Fact] + public void ASeverityOverrideAndASuppressionBothApplyToOneWarning() + { + var stmt = new PlanStatement + { + QueryHash = Hash, + CursorActualType = "Dynamic" + }; + var plan = new ParsedPlan { Batches = [new PlanBatch { Statements = [stmt] }] }; + var config = new AnalyzerConfig + { + Rules = new RulesConfig { SeverityOverrides = { [36] = "Info" } }, + Suppressions = [Entry(rule: 36, reason: "vendor cursor")] + }; + + PlanAnalysisPipeline.AnalyzeParsed(plan, config); + + var warning = Assert.Single(stmt.PlanWarnings, w => w.RuleNumber == 36); + Assert.Equal(PlanWarningSeverity.Info, warning.Severity); + Assert.True(warning.IsSuppressed); + Assert.Equal("vendor cursor", warning.SuppressionReason); + } + + [Fact] + public void ASuppressionReachesAStatementInsideAProcedureBody() + { + var baseline = PlanTestHelper.LoadAndAnalyze("exec_stored_procedure_plan.sqlplan"); + var topLevel = baseline.Batches.SelectMany(b => b.Statements).ToHashSet(); + var nested = PlanStatements.EnumerateAll(baseline) + .Where(s => !topLevel.Contains(s) && !string.IsNullOrEmpty(s.QueryHash)) + .Select(s => (Statement: s, Warning: AllOf(s).FirstOrDefault(w => w.RuleNumber != null))) + .First(x => x.Warning != null); + + var hash = nested.Statement.QueryHash!; + var rule = nested.Warning!.RuleNumber!.Value; + + var xml = File.ReadAllText(Path.Combine("Plans", "exec_stored_procedure_plan.sqlplan")) + .Replace("encoding=\"utf-16\"", "encoding=\"utf-8\""); + var plan = ShowPlanParser.Parse(xml); + PlanAnalysisPipeline.AnalyzeParsed(plan, Cfg(Entry(hash: hash, rule: rule, reason: "library owns it"))); + + var suppressedStatements = PlanStatements.EnumerateAll(plan) + .Where(s => AllOf(s).Any(w => w.IsSuppressed)) + .ToList(); + Assert.NotEmpty(suppressedStatements); + Assert.All(suppressedStatements, s => Assert.Equal(hash, s.QueryHash)); + Assert.DoesNotContain(plan.Batches.SelectMany(b => b.Statements), + s => AllOf(s).Any(w => w.IsSuppressed) && s.QueryHash != hash); + Assert.All(PlanTestHelper.AllWarnings(plan).Where(w => w.IsSuppressed), + w => Assert.Equal(rule, w.RuleNumber)); + } + + private static List AllOf(PlanStatement stmt) + { + var all = new List(stmt.PlanWarnings); + all.AddRange(PlanTestHelper.AllNodeWarnings(stmt)); + return all; + } + + [Fact] + public void AScorerWaitStatsFindingCanBeSuppressedByType() + { + // The scorer adds its wait-stats findings after the analyzer, so the pass has to run last. + static ParsedPlan Run(AnalyzerConfig config) + { + var xml = File.ReadAllText(Path.Combine("Plans", "case_predicate_plan.sqlplan")) + .Replace("encoding=\"utf-16\"", "encoding=\"utf-8\""); + var parsed = ShowPlanParser.Parse(xml); + PlanAnalysisPipeline.AnalyzeParsed(parsed, config); + return parsed; + } + + var baseline = Run(new AnalyzerConfig()); + var wait = PlanStatements.EnumerateAll(baseline) + .SelectMany(s => s.PlanWarnings.Select(w => (Statement: s, Warning: w))) + .First(x => x.Warning.WarningType.StartsWith("Wait: ", StringComparison.Ordinal) + && !string.IsNullOrEmpty(x.Statement.QueryHash)); + Assert.Null(wait.Warning.RuleNumber); + + var plan = Run(Cfg(Entry(hash: wait.Statement.QueryHash!, type: wait.Warning.WarningType))); + + Assert.Contains(PlanTestHelper.AllWarnings(plan), + w => w.IsSuppressed && w.WarningType == wait.Warning.WarningType); + } + + // ---- Output ------------------------------------------------------------------------------- + + [Fact] + public void SuppressedWarningsLeaveTheSummaryCountsButStayInTheOutput() + { + var plan = SamplePlan(); + var before = ResultMapper.Map(plan, "x").Summary; + Assert.Equal(5, before.TotalWarnings); + Assert.Equal(1, before.CriticalWarnings); + Assert.Equal(0, before.SuppressedWarnings); + + // The Critical lookup on dbo.Orders, and the statement-level Dynamic Cursor. + Suppress(plan, Entry(rule: 10, op: "dbo.Orders", reason: "r1"), Entry(rule: 36, reason: "r2")); + var result = ResultMapper.Map(plan, "x"); + + Assert.Equal(3, result.Summary.TotalWarnings); + Assert.Equal(0, result.Summary.CriticalWarnings); + Assert.Equal(2, result.Summary.SuppressedWarnings); + Assert.DoesNotContain("Dynamic Cursor", result.Summary.WarningTypes); + Assert.Contains("Key Lookup", result.Summary.WarningTypes); // node 1 is still active + + var statement = result.Statements[0]; + var cursor = Assert.Single(statement.Warnings, w => w.Type == "Dynamic Cursor"); + Assert.True(cursor.IsSuppressed); + Assert.Equal("r2", cursor.SuppressionReason); + + var lookup = statement.OperatorTree!.Children[1].Warnings.First(w => w.Type == "Key Lookup"); + Assert.True(lookup.IsSuppressed); + Assert.Equal("r1", lookup.SuppressionReason); + Assert.Equal("Critical", lookup.Severity); + } + + [Fact] + public void WarningTypesDropsATypeOnlyWhenEveryFindingOfItIsSuppressed() + { + var plan = SamplePlan(); + Suppress(plan, Entry(type: "Key Lookup")); + + var summary = ResultMapper.Map(plan, "x").Summary; + + Assert.DoesNotContain("Key Lookup", summary.WarningTypes); + Assert.Equal(3, summary.TotalWarnings); + Assert.Equal(2, summary.SuppressedWarnings); + } + + [Fact] + public void TheSerializedJsonCarriesRuleSuppressedAndReasonAndOmitsRuleForEngineWarnings() + { + var plan = SamplePlan(); + Suppress(plan, Entry(rule: 10, op: "eventuous.Messages", reason: "library owns it")); + + var json = JsonSerializer.Serialize(ResultMapper.Map(plan, "x"), AnalysisJson.IndentedWithoutNulls); + using var doc = JsonDocument.Parse(json); + var statement = doc.RootElement.GetProperty("statements")[0]; + + Assert.Equal(1, doc.RootElement.GetProperty("summary").GetProperty("suppressed_warnings").GetInt32()); + + var cursor = statement.GetProperty("warnings").EnumerateArray().Single(w => w.GetProperty("type").GetString() == "Dynamic Cursor"); + Assert.Equal(36, cursor.GetProperty("rule").GetInt32()); + Assert.False(cursor.GetProperty("suppressed").GetBoolean()); + Assert.False(cursor.TryGetProperty("suppression_reason", out _)); + + var engine = statement.GetProperty("warnings").EnumerateArray().Single(w => w.GetProperty("type").GetString() == "Implicit Conversion"); + Assert.False(engine.TryGetProperty("rule", out _)); + Assert.False(engine.GetProperty("suppressed").GetBoolean()); + + var lookup = statement.GetProperty("operator_tree").GetProperty("children")[0].GetProperty("warnings")[0]; + Assert.Equal(10, lookup.GetProperty("rule").GetInt32()); + Assert.True(lookup.GetProperty("suppressed").GetBoolean()); + Assert.Equal("library owns it", lookup.GetProperty("suppression_reason").GetString()); + } + + [Fact] + public void TheTextOutputTagsASuppressedWarningWithItsReasonAndReportsTheCount() + { + var plan = SamplePlan(); + Suppress(plan, Entry(rule: 36, reason: "vendor cursor")); + + var text = TextFormatter.Format(ResultMapper.Map(plan, "x")); + + Assert.Contains("[suppressed: vendor cursor]", text); + Assert.Contains("Suppressed warnings: 1", text); + Assert.Contains("Warnings: 4 (1 critical)", text); + } + + [Fact] + public void TheHtmlOutputTagsASuppressedWarningWithItsReason() + { + var plan = SamplePlan(); + Suppress(plan, Entry(rule: 36, reason: "vendor ")); + + var html = HtmlExporter.Export(ResultMapper.Map(plan, "x"), ""); + + Assert.Contains("suppressed: vendor <cursor>", html); + } + + [Fact] + public void TheMcpSummaryAndWarningListCarryTheSuppression() + { + var plan = SamplePlan(); + Suppress(plan, Entry(rule: 36, reason: "vendor cursor")); + + var manager = new PlanSessionManager(); + var sessionId = $"mcp-{Guid.NewGuid():N}"; + manager.Register(new CorePlanSession + { + SessionId = sessionId, + Label = "sample", + Source = "file", + Plan = plan + }); + var operations = new PlanOperations(manager, AnalyzerConfig.Default, enforceQueryAdmission: false); + + var summary = operations.GetSummary(sessionId); + Assert.Equal(1, summary.SuppressedWarnings); + Assert.Equal(4, summary.TotalWarnings); + + var warnings = operations.GetWarnings(sessionId); + var cursor = Assert.Single(warnings.Warnings, w => w.Type == "Dynamic Cursor"); + Assert.True(cursor.IsSuppressed); + Assert.Equal("vendor cursor", cursor.SuppressionReason); + Assert.Equal(36, cursor.Rule); + var engine = warnings.Warnings.First(w => w.Type == "Implicit Conversion"); + Assert.Null(engine.Rule); + Assert.False(engine.IsSuppressed); + } +} From e0a65f54a18572804e291f95419564bfd2f6f77c Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 14 Oct 2026 19:03:57 -0400 Subject: [PATCH 05/21] Fix CI build, config error handling, suppressed counts and docs (#651) Link OperatorLabel.cs into the Web project. Catch loader failures in the REPL and the desktop app. Exclude suppressed warnings from the grid, root badge, advice chip and comparison counts. Name the real config file in validation errors. Add CLI, grouped text, advice and comparison tests. Docs follow-ups. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018MrD9SGvLFx4kJaeGZyXG9 --- README.md | 12 +-- .../Controls/PlanViewerControl.Rendering.cs | 2 +- .../Controls/PlanViewerControl.Statements.cs | 4 +- .../Controls/PlanViewerControl.axaml | 6 ++ .../Controls/PlanViewerControl.axaml.cs | 22 ++++- .../AdviceContentBuilder.WaitStats.cs | 6 +- src/PlanViewer.Cli/Program.cs | 13 ++- .../Output/ComparisonFormatter.cs | 2 +- src/PlanViewer.Core/Services/ConfigLoader.cs | 6 +- src/PlanViewer.Web/PlanViewer.Web.csproj | 1 + .../PlanViewer.Core.Tests/AdviceCardsTests.cs | 29 +++++++ .../ComparisonModelTests.cs | 13 +++ .../ConfigLoaderTests.cs | 17 +++- .../HistoricalCliContractTests.cs | 83 +++++++++++++++---- .../PlanViewer.Core.Tests/SuppressionTests.cs | 46 +++++++++- 15 files changed, 225 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index f5c3c47..d822a35 100644 --- a/README.md +++ b/README.md @@ -527,7 +527,7 @@ On Linux, `planview credential` exits with an error. See [Platform Support](#pla ### Configuration file (.planview.json) -A `.planview.json` file changes what the analyzer reports. The CLI, the desktop app and the MCP server all read it. +A `.planview.json` file changes what the analyzer reports. The CLI commands `analyze`, `query-store` and `repl` read it. The desktop app reads it each time it opens a plan. The MCP server does not read it and always uses the defaults. The file is looked for in this order, and the first one found wins: @@ -559,11 +559,13 @@ If none exists, the analyzer uses its defaults. The file is JSON. Comments and t } ``` -**`rules.disabled`** is a list of rule numbers. A disabled rule never runs. +The `rules.disabled` list holds rule numbers. A disabled rule never runs. -**`rules.severity_overrides`** maps a rule number to a new severity. The allowed values are `Info`, `Warning` and `Critical`. +The `rules.severity_overrides` map gives a rule number a new severity. The allowed values are `Info`, `Warning` and `Critical`. -**`suppressions`** marks single findings as accepted. Use it when a finding is correct but you cannot or will not act on it. A suppressed finding is not removed. It stays in the output with `"suppressed": true` and its reason, and the text and HTML output tag it as `[suppressed: ]`. It does not count toward `total_warnings`, `critical_warnings` or `warning_types`. The summary reports it in `suppressed_warnings` instead. A suppressed finding keeps its severity, including one set by `severity_overrides`. +The `suppressions` list marks single findings as accepted. Use it when a finding is correct but you cannot or will not act on it. A suppressed finding is not removed. It stays in the output with `"suppressed": true` and its reason. The text output tags it as `[suppressed: ]` and the HTML output shows a `suppressed: ` badge. A suppressed finding keeps its severity, including one set by `severity_overrides`. + +A suppressed finding does not count toward `total_warnings`, `critical_warnings` or `warning_types`. The summary reports it in `suppressed_warnings` instead. The same rule applies to the counts in the desktop app. Each suppression has these fields: @@ -575,7 +577,7 @@ Each suppression has these fields: | `operator` | No | Text that must appear in the operator label. Case does not matter. Use the label from the `operator` field of the JSON output, such as `Key Lookup on eventuous.Messages (Node 1)`. A partial match works, so `eventuous.Messages` is enough. A finding on the statement as a whole has no operator, so a suppression that sets `operator` never matches it. | | `reason` | Yes | Why the finding is accepted. It must not be empty. | -A suppression that is missing a field is an error. The command prints one line, for example `Suppression 2 in .planview.json has no "reason".`, and exits with code 1. The number is the zero-based position in the list. +A suppression that is missing a field is an error. The command prints one line that names the file, for example `Suppression 2 in C:\work\team.json has no "reason".`, and exits with code 1. The number is the zero-based position in the list. The desktop app does not stop. It opens the plan with the defaults and shows the message in the toolbar. To find the `query_hash` of a statement, run `planview analyze` with JSON output and read `statements[].query_hash`. Each warning in the JSON also shows its `rule` number when a rule produced it, and its `operator` label when it belongs to an operator. diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs index 377a77b..bae6c6a 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs @@ -49,7 +49,7 @@ private void RenderStatement(PlanStatement statement) // Render nodes — pass total warning count to root node for badge var allWarnings = new List(); CollectWarnings(statement.RootNode, allWarnings); - RenderNodes(statement.RootNode, divergenceLimit, allWarnings.Count); + RenderNodes(statement.RootNode, divergenceLimit, allWarnings.Count(w => !w.IsSuppressed)); // Update banners ShowMissingIndexes(statement.MissingIndexes); diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs index 6e6c61e..03221d3 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs @@ -171,8 +171,8 @@ row.Statement.StatementText and hand out the statement exactly as the plan ElapsedMs = stmt.QueryTimeStats?.ElapsedTimeMs ?? 0, UdfMs = stmt.QueryUdfElapsedTimeMs, EstCost = stmt.StatementSubTreeCost, - Critical = allWarnings.Count(w => w.Severity == PlanWarningSeverity.Critical), - Warnings = allWarnings.Count(w => w.Severity == PlanWarningSeverity.Warning), + Critical = allWarnings.Count(w => !w.IsSuppressed && w.Severity == PlanWarningSeverity.Critical), + Warnings = allWarnings.Count(w => !w.IsSuppressed && w.Severity == PlanWarningSeverity.Warning), Statement = stmt }); } diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.axaml b/src/PlanViewer.App/Controls/PlanViewerControl.axaml index f7b1b85..5fceff8 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.axaml +++ b/src/PlanViewer.App/Controls/PlanViewerControl.axaml @@ -95,6 +95,12 @@ + + + + + + all + $(WarningsAsErrors);NU1903 + + diff --git a/Directory.Packages.props b/Directory.Packages.props index bfd2e21..9086d92 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -30,6 +30,11 @@ + + @@ -39,6 +44,11 @@ + + diff --git a/src/Directory.Build.props b/src/Directory.Build.props index d2f380d..9062488 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -1,4 +1,7 @@ + + + + diff --git a/src/PlanViewer.Ssms/PlanViewer.Ssms.csproj b/src/PlanViewer.Ssms/PlanViewer.Ssms.csproj index 5d65d33..8efca03 100644 --- a/src/PlanViewer.Ssms/PlanViewer.Ssms.csproj +++ b/src/PlanViewer.Ssms/PlanViewer.Ssms.csproj @@ -83,6 +83,8 @@ + + From 5dfe9b78cbe1da3955c4ad5aa692495e8e7a963f Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 14 Oct 2026 19:27:38 -0400 Subject: [PATCH 08/21] Exclude MessagePack runtime assets in SSMS, fix workflow steps and a comment (#650) Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018MrD9SGvLFx4kJaeGZyXG9 --- .github/workflows/dependency-submission.yml | 7 +++++++ src/Directory.Build.props | 9 ++++++--- src/PlanViewer.Ssms/PlanViewer.Ssms.csproj | 6 ++++-- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/.github/workflows/dependency-submission.yml b/.github/workflows/dependency-submission.yml index 00bcb5a..eb84914 100644 --- a/.github/workflows/dependency-submission.yml +++ b/.github/workflows/dependency-submission.yml @@ -30,10 +30,17 @@ jobs: with: dotnet-version: 10.0.x + # ci.yml does the same before restoring the solution (Blazor WASM project). + - name: Install WASM workload + run: dotnet workload install wasm-tools + # The detector reads project.assets.json, which only exists after a restore. - name: Restore solution run: dotnet restore PlanViewer.sln + - name: Add MSBuild to PATH + uses: microsoft/setup-msbuild@30375c66a4eea26614e0d39710365f22f8b0af57 # v3.0.0 + - name: Restore SSMS projects run: | dotnet restore src/PlanViewer.Ssms.Installer/PlanViewer.Ssms.Installer.csproj diff --git a/src/Directory.Build.props b/src/Directory.Build.props index 9062488..51f9e3f 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -9,13 +9,16 @@ PlanViewer.App, PlanViewer.Core, PlanViewer.Cli, PlanViewer.Web, PlanViewer.Ssms.Installer. - NOT picked up by PlanViewer.Ssms — it's a legacy non-SDK project. - Its version lives in: + PlanViewer.Ssms is a legacy non-SDK project. It imports this file too, through + Microsoft.Common.props, which is how it gets the NuGet audit settings from the + repo-root file. But a legacy project does not generate assembly attributes from + , so its version lives in: Properties/AssemblyInfo.cs (AssemblyVersion + AssemblyFileVersion) source.extension.vsixmanifest (Identity Version="...") Bump those manually when bumping the version here. - Tests and server/ projects are outside src/ and are unaffected. + Tests and server/ projects are outside src/. They pick up the repo-root + Directory.Build.props directly and never see the version block below. --> 1.30.0 diff --git a/src/PlanViewer.Ssms/PlanViewer.Ssms.csproj b/src/PlanViewer.Ssms/PlanViewer.Ssms.csproj index 8efca03..3da6035 100644 --- a/src/PlanViewer.Ssms/PlanViewer.Ssms.csproj +++ b/src/PlanViewer.Ssms/PlanViewer.Ssms.csproj @@ -83,8 +83,10 @@ - - + + From e49b942666e8fccbe0cfba5eeb9cbb7c19f36cb4 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 14 Oct 2026 19:51:42 -0400 Subject: [PATCH 09/21] Apply round-1 review: gate NU1904, harden dependency submission, fix Pkcs drop condition Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018MrD9SGvLFx4kJaeGZyXG9 --- .github/workflows/dependency-submission.yml | 21 ++++++++++++++++----- .github/workflows/deploy-planshare.yml | 1 + .github/workflows/deploy-web.yml | 1 + .github/workflows/release.yml | 7 ++++++- .github/workflows/ssms-installer-tests.yml | 2 ++ Directory.Build.props | 7 ++++--- Directory.Packages.props | 9 ++++++--- 7 files changed, 36 insertions(+), 12 deletions(-) diff --git a/.github/workflows/dependency-submission.yml b/.github/workflows/dependency-submission.yml index eb84914..90e70ed 100644 --- a/.github/workflows/dependency-submission.yml +++ b/.github/workflows/dependency-submission.yml @@ -8,6 +8,9 @@ name: Dependency submission # # Dependabot alerts read the default branch (main), so transitive alerts start after the # next dev-to-main release merge. The dev trigger lets the workflow be exercised earlier. +# Dev snapshots feed no alerts, so drop dev from the push trigger once the first run on dev +# is green: each run holds a contents: write token while running a binary the action +# downloads at run time (the latest microsoft/component-detection release). on: push: @@ -24,6 +27,8 @@ jobs: steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false - name: Setup .NET 10.0 uses: actions/setup-dotnet@a98b56852c35b8e3190ac28c8c2271da59106c68 # v6.0.0 @@ -34,18 +39,24 @@ jobs: - name: Install WASM workload run: dotnet workload install wasm-tools - # The detector reads project.assets.json, which only exists after a restore. + # The detector reads project.assets.json, which only exists after a restore. Every restore + # here passes -p:NuGetAudit=false: with the audit gate on, a new high advisory fails the + # restore, the submit step never runs, and the graph goes stale exactly when an alert is + # needed. Resolution is unchanged, and GitHub matches advisories on the submitted graph. - name: Restore solution - run: dotnet restore PlanViewer.sln + run: dotnet restore PlanViewer.sln -p:NuGetAudit=false - name: Add MSBuild to PATH uses: microsoft/setup-msbuild@30375c66a4eea26614e0d39710365f22f8b0af57 # v3.0.0 - name: Restore SSMS projects run: | - dotnet restore src/PlanViewer.Ssms.Installer/PlanViewer.Ssms.Installer.csproj - dotnet restore tests/PlanViewer.Ssms.Installer.Tests/PlanViewer.Ssms.Installer.Tests.csproj - msbuild src/PlanViewer.Ssms/PlanViewer.Ssms.csproj -t:Restore -p:Configuration=Release + dotnet restore src/PlanViewer.Ssms.Installer/PlanViewer.Ssms.Installer.csproj -p:NuGetAudit=false + dotnet restore tests/PlanViewer.Ssms.Installer.Tests/PlanViewer.Ssms.Installer.Tests.csproj -p:NuGetAudit=false + msbuild src/PlanViewer.Ssms/PlanViewer.Ssms.csproj -t:Restore -p:Configuration=Release -p:NuGetAudit=false - name: Submit dependency snapshot uses: advanced-security/component-detection-dependency-submission-action@a02aeda7a175ad4dd38a13c4d3ff2df9da8f2cf0 # v0.2.0 + with: + # The default submits an empty snapshot when nothing is found, replacing the graph. + fail-on-empty: 'true' diff --git a/.github/workflows/deploy-planshare.yml b/.github/workflows/deploy-planshare.yml index 2d3876d..2dc3b70 100644 --- a/.github/workflows/deploy-planshare.yml +++ b/.github/workflows/deploy-planshare.yml @@ -8,6 +8,7 @@ on: branches: [main] paths: - 'server/PlanShare/**' + - 'Directory.Build.props' - 'Directory.Packages.props' - '.github/workflows/deploy-planshare.yml' workflow_dispatch: diff --git a/.github/workflows/deploy-web.yml b/.github/workflows/deploy-web.yml index 682d932..271b859 100644 --- a/.github/workflows/deploy-web.yml +++ b/.github/workflows/deploy-web.yml @@ -6,6 +6,7 @@ on: paths: - 'src/PlanViewer.Core/**' - 'src/PlanViewer.Web/**' + - 'Directory.Build.props' - 'src/Directory.Build.props' - 'Directory.Packages.props' - '.github/workflows/deploy-web.yml' diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b95c5a1..0d9da06 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -118,7 +118,12 @@ jobs: # Those props redirect VSToolsPath into the Microsoft.VSSDK.BuildTools # package, which is how the VSSDK targets resolve on a runner that # lacks the Visual Studio extension-development workload. - msbuild src/PlanViewer.Ssms/PlanViewer.Ssms.csproj -restore -t:Build -p:Configuration=Release -p:DeployExtension=false + $msbuildLog = Join-Path $env:RUNNER_TEMP 'ssms-msbuild.log' + msbuild src/PlanViewer.Ssms/PlanViewer.Ssms.csproj -restore -t:Build -p:Configuration=Release -p:DeployExtension=false | Tee-Object -FilePath $msbuildLog + $auditHit = Select-String -Path $msbuildLog -Pattern 'NU1903|NU1904' | Select-Object -First 1 + if ($auditHit) { + Write-Host "::warning::SSMS restore failed the NuGet audit: $($auditHit.Line.Trim())" + } dotnet build src/PlanViewer.Ssms.Installer/PlanViewer.Ssms.Installer.csproj -c Release $vsix = 'src/PlanViewer.Ssms/bin/Release/PlanViewer.Ssms.vsix' diff --git a/.github/workflows/ssms-installer-tests.yml b/.github/workflows/ssms-installer-tests.yml index d086001..87004f6 100644 --- a/.github/workflows/ssms-installer-tests.yml +++ b/.github/workflows/ssms-installer-tests.yml @@ -13,6 +13,8 @@ on: paths: - 'src/PlanViewer.Ssms.Installer/**' - 'tests/PlanViewer.Ssms.Installer.Tests/**' + - 'Directory.Build.props' + - 'src/Directory.Build.props' - 'Directory.Packages.props' - '.github/workflows/ssms-installer-tests.yml' workflow_dispatch: diff --git a/Directory.Build.props b/Directory.Build.props index 9cad409..359ebc3 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -6,13 +6,14 @@ --> all - $(WarningsAsErrors);NU1903 + $(WarningsAsErrors);NU1903;NU1904 diff --git a/Directory.Packages.props b/Directory.Packages.props index 9086d92..0c341a5 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -45,9 +45,12 @@ + (GHSA-846f-f764-76mx high, GHSA-2fmv-rv2c-59x7 moderate; both cover 9.0.0 through + 9.0.20). PlanViewer.Core references this directly so 10.0.13 wins in + project.assets.json and in the planview CLI package. The desktop app's self-contained + publish takes Pkcs from the ASP.NET Core runtime pack instead, which the audit never + sees. Drop this pin and the Core reference once SqlClient depends on Pkcs 9.0.21 or + later (10.0.13 or later on 10.x). --> From a98c6c75171a930a5be13529fe870b9f7e5cb91a Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 14 Oct 2026 20:10:45 -0400 Subject: [PATCH 10/21] Submit the dependency snapshot without a third-party binary holding the write token (#655) Split dependency-submission.yml into a read-only detect job that builds the snapshot from obj/project.assets.json with a repo script, and a write-token submit job that only downloads the artifact and posts it with gh api. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018MrD9SGvLFx4kJaeGZyXG9 --- .github/scripts/New-DependencySnapshot.ps1 | 198 ++++++++++++++++++++ .github/workflows/dependency-submission.yml | 84 +++++++-- 2 files changed, 264 insertions(+), 18 deletions(-) create mode 100644 .github/scripts/New-DependencySnapshot.ps1 diff --git a/.github/scripts/New-DependencySnapshot.ps1 b/.github/scripts/New-DependencySnapshot.ps1 new file mode 100644 index 0000000..184cc48 --- /dev/null +++ b/.github/scripts/New-DependencySnapshot.ps1 @@ -0,0 +1,198 @@ +<# +.SYNOPSIS + Builds a GitHub dependency snapshot from the obj/project.assets.json files NuGet wrote. + +.DESCRIPTION + Reads every obj/project.assets.json under the repository and writes the JSON body for + POST /repos/{owner}/{repo}/dependency-graph/snapshots. No third-party tool is involved: the + assets file already holds the exact package versions NuGet resolved, transitive packages + included, and which of them the project references directly. + + The resolved map is keyed by package URL and dependencies list package URLs, as in the + snapshots GitHub accepted from the component-detection action. Packages in tests/ projects + get scope 'development', all others 'runtime'. + + One manifest per project per target framework. The manifest is named after the project file + (relative to the repository root, forward slashes), with the target framework appended in + parentheses only when a project has more than one target. + + Fails when it finds no assets file or resolves no package at all, so a broken restore cannot + replace the dependency graph with an empty snapshot. + +.PARAMETER Sha + Commit SHA the snapshot describes (github.sha). + +.PARAMETER Ref + Fully qualified ref, e.g. refs/heads/main (github.ref). + +.PARAMETER RunId + Workflow run id (github.run_id). + +.PARAMETER Correlator + Stable string that makes a new snapshot replace the previous one from the same source. + +.PARAMETER OutputPath + Where to write the snapshot JSON. + +.PARAMETER RepoRoot + Repository root. Defaults to two levels above this script. +#> +[CmdletBinding()] +param( + [Parameter(Mandatory)][string]$Sha, + [Parameter(Mandatory)][string]$Ref, + [Parameter(Mandatory)][string]$RunId, + [Parameter(Mandatory)][string]$Correlator, + [string]$OutputPath = 'snapshot.json', + [string]$RepoRoot = (Join-Path $PSScriptRoot '..\..') +) + +$ErrorActionPreference = 'Stop' + +$root = (Resolve-Path -LiteralPath $RepoRoot).Path.TrimEnd('\', '/') +$rootPrefix = $root.Replace('\', '/') + '/' + +function Get-RelativePath([string]$fullPath) { + $normalized = $fullPath.Replace('\', '/') + if ($normalized.StartsWith($rootPrefix, [System.StringComparison]::OrdinalIgnoreCase)) { + return $normalized.Substring($rootPrefix.Length) + } + return $normalized +} + +# Assets files under .claude/, tmp-*/ and bin/ are scratch copies, not part of the product. +$assetsFiles = Get-ChildItem -LiteralPath $root -Recurse -Filter 'project.assets.json' -File | + Where-Object { $_.Directory.Name -eq 'obj' } | + Where-Object { + $rel = Get-RelativePath $_.FullName + $rel -notmatch '(^|/)(\.claude|bin|tmp-[^/]*)/' + } | + Sort-Object FullName + +if (-not $assetsFiles) { + throw "No obj/project.assets.json found under $root. Did the restore steps run?" +} + +# package_url values: percent-encode the name and version (a version can carry '+' build metadata). +function Get-PackageUrl([string]$name, [string]$version) { + 'pkg:nuget/{0}@{1}' -f [uri]::EscapeDataString($name), [uri]::EscapeDataString($version) +} + +# Split a "Name/Version" key at the last slash. +function Split-LibraryKey([string]$key) { + $i = $key.LastIndexOf('/') + if ($i -lt 1) { return $null } + [pscustomobject]@{ Name = $key.Substring(0, $i); Version = $key.Substring($i + 1) } +} + +$manifests = [ordered]@{} +$totalResolved = 0 + +foreach ($file in $assetsFiles) { + $assets = Get-Content -LiteralPath $file.FullName -Raw | ConvertFrom-Json + + $projectPath = $assets.project.restore.projectPath + if ([string]::IsNullOrWhiteSpace($projectPath)) { + Write-Warning "Skipping $($file.FullName): no project.restore.projectPath." + continue + } + $projectFile = Get-RelativePath $projectPath + # Test-only packages are development dependencies; everything else ships. + $scope = $(if ($projectFile -like 'tests/*') { 'development' } else { 'runtime' }) + + # The direct references: names listed for the target framework, plus the framework-less group. + $directByFramework = @{} + foreach ($group in $assets.projectFileDependencyGroups.PSObject.Properties) { + $names = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::OrdinalIgnoreCase) + foreach ($line in @($group.Value)) { + if ([string]::IsNullOrWhiteSpace($line)) { continue } + [void]$names.Add(($line.Trim() -split '\s+', 2)[0]) + } + $directByFramework[$group.Name] = $names + } + + # A restore with runtime identifiers adds "tfm/rid" targets that repeat the plain "tfm" target's + # packages; keep one manifest per framework and drop the runtime-specific copies. + $allTargets = @($assets.targets.PSObject.Properties) + $plainNames = @($allTargets | Where-Object { $_.Name -notlike '*/*' } | ForEach-Object { $_.Name }) + $targets = @($allTargets | Where-Object { $_.Name -notlike '*/*' -or ($plainNames -notcontains ($_.Name -split '/', 2)[0]) }) + foreach ($target in $targets) { + # A runtime-specific target is keyed "tfm/rid"; its direct references sit under "tfm". + $framework = ($target.Name -split '/', 2)[0] + $direct = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::OrdinalIgnoreCase) + foreach ($key in @($framework, $target.Name, '')) { + if ($directByFramework.ContainsKey($key)) { $direct.UnionWith($directByFramework[$key]) } + } + + # Pass 1: every package NuGet resolved for this target. + $packages = [ordered]@{} + foreach ($entry in $target.Value.PSObject.Properties) { + if ($entry.Value.type -ne 'package') { continue } + $id = Split-LibraryKey $entry.Name + if ($null -eq $id) { continue } + $packages[$id.Name] = [pscustomobject]@{ Version = $id.Version; Entry = $entry.Value } + } + + # Pass 2: build the resolved map, keyed by package URL (the shape GitHub accepted from the + # previous action). Dependencies are package URLs of packages in this same map. + $purlByName = @{} + foreach ($name in $packages.Keys) { $purlByName[$name] = Get-PackageUrl $name $packages[$name].Version } + + $resolved = [ordered]@{} + foreach ($name in $packages.Keys) { + $pkg = $packages[$name] + $purl = $purlByName[$name] + $dependsOn = [System.Collections.Generic.List[string]]::new() + if ($pkg.Entry.PSObject.Properties['dependencies']) { + foreach ($dep in $pkg.Entry.dependencies.PSObject.Properties) { + # $purlByName is case-insensitive, like NuGet package ids. + if ($purlByName.ContainsKey($dep.Name) -and $purlByName[$dep.Name] -ne $purl) { + if (-not $dependsOn.Contains($purlByName[$dep.Name])) { $dependsOn.Add($purlByName[$dep.Name]) } + } + } + } + $resolved[$purl] = [ordered]@{ + package_url = $purl + relationship = $(if ($direct.Contains($name)) { 'direct' } else { 'indirect' }) + scope = $scope + dependencies = [string[]]$dependsOn.ToArray() + } + } + + if ($resolved.Count -eq 0) { continue } + + $manifestName = $projectFile + if ($targets.Count -gt 1) { $manifestName = "$projectFile ($($target.Name))" } + + $manifests[$manifestName] = [ordered]@{ + name = $manifestName + file = [ordered]@{ source_location = $projectFile } + resolved = $resolved + } + $totalResolved += $resolved.Count + Write-Host ("{0}: {1} packages" -f $manifestName, $resolved.Count) + } +} + +if ($manifests.Count -eq 0 -or $totalResolved -eq 0) { + throw "Found $($assetsFiles.Count) assets file(s) but no resolved packages; refusing to write an empty snapshot." +} + +$snapshot = [ordered]@{ + version = 0 + sha = $Sha + ref = $Ref + job = [ordered]@{ correlator = $Correlator; id = $RunId } + detector = [ordered]@{ + name = 'performancestudio-assets-snapshot' + version = '1.0.0' + url = 'https://github.com/erikdarlingdata/PerformanceStudio' + } + scanned = (Get-Date).ToUniversalTime().ToString('yyyy-MM-ddTHH:mm:ssZ') + manifests = $manifests +} + +$json = $snapshot | ConvertTo-Json -Depth 10 +[System.IO.File]::WriteAllText([System.IO.Path]::GetFullPath($OutputPath, (Get-Location).Path), $json, [System.Text.UTF8Encoding]::new($false)) + +Write-Host ("Wrote {0}: {1} manifests, {2} resolved packages from {3} assets files." -f $OutputPath, $manifests.Count, $totalResolved, $assetsFiles.Count) diff --git a/.github/workflows/dependency-submission.yml b/.github/workflows/dependency-submission.yml index 90e70ed..b600878 100644 --- a/.github/workflows/dependency-submission.yml +++ b/.github/workflows/dependency-submission.yml @@ -3,27 +3,33 @@ name: Dependency submission # Dependabot's dependency graph cannot read versions from Directory.Packages.props (central # package management) and no project has a packages.lock.json, so the graph lists direct # packages with no version and no transitive packages at all. That means no Dependabot -# alerts for a vulnerable transitive package (issue #650). This job restores the projects and -# submits what NuGet actually resolved, transitive packages included. +# alerts for a vulnerable transitive package (issue #650). This workflow restores the projects +# and submits what NuGet actually resolved, transitive packages included. # -# Dependabot alerts read the default branch (main), so transitive alerts start after the -# next dev-to-main release merge. The dev trigger lets the workflow be exercised earlier. -# Dev snapshots feed no alerts, so drop dev from the push trigger once the first run on dev -# is green: each run holds a contents: write token while running a binary the action -# downloads at run time (the latest microsoft/component-detection release). +# Two jobs, so the token that can write to the repository never sits next to build-time code: +# detect contents: read. Restores the projects (package and MSBuild code runs here) and turns +# the obj/project.assets.json files into a snapshot with +# .github/scripts/New-DependencySnapshot.ps1. +# submit contents: write. Checks out nothing and runs no repository code. It only downloads the +# snapshot artifact and posts it with the gh CLI that ships on the runner (issue #655). +# +# Dependabot alerts read the default branch (main), so transitive alerts start after the next +# dev-to-main release merge. Snapshots from dev feed no alerts; the dev trigger exists to +# exercise this workflow before a release merge. on: push: branches: [main, dev] workflow_dispatch: -permissions: - contents: write +permissions: {} jobs: - submit: + detect: # Windows, so the old-format SSMS add-in project can be restored with MSBuild too. runs-on: windows-latest + permissions: + contents: read steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -39,10 +45,10 @@ jobs: - name: Install WASM workload run: dotnet workload install wasm-tools - # The detector reads project.assets.json, which only exists after a restore. Every restore - # here passes -p:NuGetAudit=false: with the audit gate on, a new high advisory fails the - # restore, the submit step never runs, and the graph goes stale exactly when an alert is - # needed. Resolution is unchanged, and GitHub matches advisories on the submitted graph. + # The snapshot script reads project.assets.json, which only exists after a restore. Every + # restore here passes -p:NuGetAudit=false: with the audit gate on, a new high advisory fails + # the restore, the snapshot is never submitted, and the graph goes stale exactly when an + # alert is needed. Resolution is unchanged, and GitHub matches advisories on the submitted graph. - name: Restore solution run: dotnet restore PlanViewer.sln -p:NuGetAudit=false @@ -55,8 +61,50 @@ jobs: dotnet restore tests/PlanViewer.Ssms.Installer.Tests/PlanViewer.Ssms.Installer.Tests.csproj -p:NuGetAudit=false msbuild src/PlanViewer.Ssms/PlanViewer.Ssms.csproj -t:Restore -p:Configuration=Release -p:NuGetAudit=false - - name: Submit dependency snapshot - uses: advanced-security/component-detection-dependency-submission-action@a02aeda7a175ad4dd38a13c4d3ff2df9da8f2cf0 # v0.2.0 + # Fails when there is no assets file or no resolved package, so a broken restore cannot + # replace the graph with an empty snapshot. The correlator is the one the previous action + # used by default (the job id, "submit"), so this snapshot replaces its snapshot instead of + # adding a second view. Values reach the script through env vars, never spliced into the script text. + - name: Build dependency snapshot + shell: pwsh + env: + SNAPSHOT_SHA: ${{ github.sha }} + SNAPSHOT_REF: ${{ github.ref }} + SNAPSHOT_RUN_ID: ${{ github.run_id }} + run: ./.github/scripts/New-DependencySnapshot.ps1 -Sha $env:SNAPSHOT_SHA -Ref $env:SNAPSHOT_REF -RunId $env:SNAPSHOT_RUN_ID -Correlator 'submit' -OutputPath snapshot.json + + - name: Upload snapshot + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: dependency-snapshot + path: snapshot.json + if-no-files-found: error + retention-days: 7 + + submit: + needs: detect + runs-on: ubuntu-latest + permissions: + contents: write + + steps: + - name: Download snapshot + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: - # The default submits an empty snapshot when nothing is found, replacing the graph. - fail-on-empty: 'true' + name: dependency-snapshot + + - name: Submit dependency snapshot + env: + GH_TOKEN: ${{ github.token }} + run: | + gh api --method POST -H "Accept: application/vnd.github+json" \ + "repos/${{ github.repository }}/dependency-graph/snapshots" \ + --input snapshot.json > response.json + cat response.json + echo + result=$(jq -r '.result' response.json) + echo "result: $result" + echo "message: $(jq -r '.message' response.json)" + if [ "$result" = "INVALID" ]; then + exit 1 + fi From 74d552c577a8d23a1ef85787523ec24772a3f3d5 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 14 Oct 2026 20:30:03 -0400 Subject: [PATCH 11/21] Apply round-1 security review to the dependency snapshot workflow (#655) - Restore PlanShare in its own step. - Restore SSMS projects under bash so the first failing restore fails the step. - Script fails when an assets file logs a restore Error; add #Requires -Version 7.0. - submit rewrites sha, ref and job id from the run, fails unless result is SUCCESS or ACCEPTED, and prints the response body on an HTTP error. - Concurrency group per ref so the latest commit's graph is submitted last. - Correct the correlator comment (the old action's snapshot is not replaced) and the scope comment. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018MrD9SGvLFx4kJaeGZyXG9 --- .github/scripts/New-DependencySnapshot.ps1 | 15 ++++++-- .github/workflows/dependency-submission.yml | 39 ++++++++++++++++----- 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/.github/scripts/New-DependencySnapshot.ps1 b/.github/scripts/New-DependencySnapshot.ps1 index 184cc48..3e6ec41 100644 --- a/.github/scripts/New-DependencySnapshot.ps1 +++ b/.github/scripts/New-DependencySnapshot.ps1 @@ -1,3 +1,4 @@ +#Requires -Version 7.0 <# .SYNOPSIS Builds a GitHub dependency snapshot from the obj/project.assets.json files NuGet wrote. @@ -10,7 +11,9 @@ The resolved map is keyed by package URL and dependencies list package URLs, as in the snapshots GitHub accepted from the component-detection action. Packages in tests/ projects - get scope 'development', all others 'runtime'. + get scope 'development'; every other project's packages get 'runtime'. The assets file does + not mark build-time packages in product projects (ILLink, the WASM DevServer, + VSSDK.BuildTools), so they are listed as 'runtime' too. That only affects alert triage. One manifest per project per target framework. The manifest is named after the project file (relative to the repository root, forward slashes), with the target framework appended in @@ -91,13 +94,21 @@ $totalResolved = 0 foreach ($file in $assetsFiles) { $assets = Get-Content -LiteralPath $file.FullName -Raw | ConvertFrom-Json + # NuGet writes the assets file even when the restore failed, with the packages it could + # resolve and an Error entry in logs. A partial graph must not replace the full one. + $restoreErrors = @($assets.logs | Where-Object { $_.level -eq 'Error' }) + if ($restoreErrors.Count -gt 0) { + throw "$($file.FullName): restore logged $($restoreErrors.Count) error(s), first $($restoreErrors[0].code). The snapshot would be incomplete." + } + $projectPath = $assets.project.restore.projectPath if ([string]::IsNullOrWhiteSpace($projectPath)) { Write-Warning "Skipping $($file.FullName): no project.restore.projectPath." continue } $projectFile = Get-RelativePath $projectPath - # Test-only packages are development dependencies; everything else ships. + # Packages in tests/ projects are development; everything else is reported as runtime, + # build-only tools in product projects included (the assets file does not mark them). $scope = $(if ($projectFile -like 'tests/*') { 'development' } else { 'runtime' }) # The direct references: names listed for the target framework, plus the framework-less group. diff --git a/.github/workflows/dependency-submission.yml b/.github/workflows/dependency-submission.yml index b600878..7b5d310 100644 --- a/.github/workflows/dependency-submission.yml +++ b/.github/workflows/dependency-submission.yml @@ -24,6 +24,12 @@ on: permissions: {} +# A newer push cancels an older run still in progress, so the latest commit's graph is the last +# one submitted and an older sha cannot overwrite it. +concurrency: + group: dependency-submission-${{ github.ref }} + cancel-in-progress: true + jobs: detect: # Windows, so the old-format SSMS add-in project can be restored with MSBuild too. @@ -52,19 +58,29 @@ jobs: - name: Restore solution run: dotnet restore PlanViewer.sln -p:NuGetAudit=false + # server/PlanShare is not in PlanViewer.sln and is the internet-facing component. + - name: Restore PlanShare + run: dotnet restore server/PlanShare/PlanShare.csproj -p:NuGetAudit=false + - name: Add MSBuild to PATH uses: microsoft/setup-msbuild@30375c66a4eea26614e0d39710365f22f8b0af57 # v3.0.0 + # bash runs with -eo pipefail, so the first failing restore fails the step (pwsh would only + # report the last command's exit code). msbuild is on PATH from the step above. - name: Restore SSMS projects + shell: bash run: | dotnet restore src/PlanViewer.Ssms.Installer/PlanViewer.Ssms.Installer.csproj -p:NuGetAudit=false dotnet restore tests/PlanViewer.Ssms.Installer.Tests/PlanViewer.Ssms.Installer.Tests.csproj -p:NuGetAudit=false msbuild src/PlanViewer.Ssms/PlanViewer.Ssms.csproj -t:Restore -p:Configuration=Release -p:NuGetAudit=false # Fails when there is no assets file or no resolved package, so a broken restore cannot - # replace the graph with an empty snapshot. The correlator is the one the previous action - # used by default (the job id, "submit"), so this snapshot replaces its snapshot instead of - # adding a second view. Values reach the script through env vars, never spliced into the script text. + # replace the graph with an empty snapshot, and when an assets file records a restore error. + # GitHub keeps the latest snapshot per correlator plus detector name. The correlator stays + # "submit" so it is stable for this workflow, but the detector name differs from the old + # action's, so this snapshot does not replace that one: the old action's snapshot exists only + # on dev and stays beside this one (main never received one). + # Values reach the script through env vars, never spliced into the script text. - name: Build dependency snapshot shell: pwsh env: @@ -97,14 +113,19 @@ jobs: env: GH_TOKEN: ${{ github.token }} run: | - gh api --method POST -H "Accept: application/vnd.github+json" \ - "repos/${{ github.repository }}/dependency-graph/snapshots" \ - --input snapshot.json > response.json + # sha, ref and run id come from this run, not from the artifact. + jq --arg sha "$GITHUB_SHA" --arg ref "$GITHUB_REF" --arg id "$GITHUB_RUN_ID" '.sha = $sha | .ref = $ref | .job.id = $id' snapshot.json > body.json + # On an HTTP error gh writes the error body to stdout, so print it. + if ! gh api --method POST -H "Accept: application/vnd.github+json" "repos/$GITHUB_REPOSITORY/dependency-graph/snapshots" --input body.json > response.json; then + cat response.json + exit 1 + fi cat response.json echo - result=$(jq -r '.result' response.json) + result=$(jq -r '.result // empty' response.json) echo "result: $result" - echo "message: $(jq -r '.message' response.json)" - if [ "$result" = "INVALID" ]; then + echo "message: $(jq -r '.message // empty' response.json)" + if [ "$result" != "SUCCESS" ] && [ "$result" != "ACCEPTED" ]; then + echo "unexpected result: '$result'" exit 1 fi From 7621e83747e8984fbf50806f0c4fc67166bad144 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 14 Oct 2026 20:32:01 -0400 Subject: [PATCH 12/21] Restore line continuations in the submit step (#655) Cosmetic: the two multi-line commands had been flattened into single lines with runs of spaces. Same commands, same behavior. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018MrD9SGvLFx4kJaeGZyXG9 --- .github/workflows/dependency-submission.yml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/workflows/dependency-submission.yml b/.github/workflows/dependency-submission.yml index 7b5d310..e2efeef 100644 --- a/.github/workflows/dependency-submission.yml +++ b/.github/workflows/dependency-submission.yml @@ -114,9 +114,12 @@ jobs: GH_TOKEN: ${{ github.token }} run: | # sha, ref and run id come from this run, not from the artifact. - jq --arg sha "$GITHUB_SHA" --arg ref "$GITHUB_REF" --arg id "$GITHUB_RUN_ID" '.sha = $sha | .ref = $ref | .job.id = $id' snapshot.json > body.json + jq --arg sha "$GITHUB_SHA" --arg ref "$GITHUB_REF" --arg id "$GITHUB_RUN_ID" \ + '.sha = $sha | .ref = $ref | .job.id = $id' snapshot.json > body.json # On an HTTP error gh writes the error body to stdout, so print it. - if ! gh api --method POST -H "Accept: application/vnd.github+json" "repos/$GITHUB_REPOSITORY/dependency-graph/snapshots" --input body.json > response.json; then + if ! gh api --method POST -H "Accept: application/vnd.github+json" \ + "repos/$GITHUB_REPOSITORY/dependency-graph/snapshots" \ + --input body.json > response.json; then cat response.json exit 1 fi From 0d058da675680065d8aef1ccfebc8e61108d804f Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 14 Oct 2026 20:44:26 -0400 Subject: [PATCH 13/21] Run the SSMS restore step under pwsh with per-command exit checks (#655) The first dev run of the two-job workflow (38660830104) failed at "Restore SSMS projects" with "msbuild: command not found". The round-1 review had switched the step to bash so the first failing restore would fail the step, but the msbuild folder that setup-msbuild adds to PATH does not reach a bash step on the Windows runner. It did reach the pwsh step in the green run before that change. The step runs under pwsh again, and each restore checks $LASTEXITCODE so a failure in the first or second command still fails the step. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018MrD9SGvLFx4kJaeGZyXG9 --- .github/workflows/dependency-submission.yml | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/.github/workflows/dependency-submission.yml b/.github/workflows/dependency-submission.yml index e2efeef..b678c64 100644 --- a/.github/workflows/dependency-submission.yml +++ b/.github/workflows/dependency-submission.yml @@ -65,14 +65,18 @@ jobs: - name: Add MSBuild to PATH uses: microsoft/setup-msbuild@30375c66a4eea26614e0d39710365f22f8b0af57 # v3.0.0 - # bash runs with -eo pipefail, so the first failing restore fails the step (pwsh would only - # report the last command's exit code). msbuild is on PATH from the step above. + # pwsh, because the msbuild folder that setup-msbuild adds to PATH reaches pwsh steps but not + # bash steps on the Windows runner (run 38660830104 failed with "msbuild: command not found"). + # pwsh only reports the last command's exit code, so each command checks its own. - name: Restore SSMS projects - shell: bash + shell: pwsh run: | dotnet restore src/PlanViewer.Ssms.Installer/PlanViewer.Ssms.Installer.csproj -p:NuGetAudit=false + if ($LASTEXITCODE) { exit $LASTEXITCODE } dotnet restore tests/PlanViewer.Ssms.Installer.Tests/PlanViewer.Ssms.Installer.Tests.csproj -p:NuGetAudit=false + if ($LASTEXITCODE) { exit $LASTEXITCODE } msbuild src/PlanViewer.Ssms/PlanViewer.Ssms.csproj -t:Restore -p:Configuration=Release -p:NuGetAudit=false + if ($LASTEXITCODE) { exit $LASTEXITCODE } # Fails when there is no assets file or no resolved package, so a broken restore cannot # replace the graph with an empty snapshot, and when an assets file records a restore error. From 9b1315be59ddb03c3a70c82e71f04bcb2d55acd1 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 19 Oct 2026 10:08:45 +0000 Subject: [PATCH 14/21] ci: bump the actions group with 2 updates Bumps the actions group with 2 updates: [anthropics/claude-code-action](https://github.com/anthropics/claude-code-action) and [actions/download-artifact](https://github.com/actions/download-artifact). Updates `anthropics/claude-code-action` from 1.0.248 to 1.0.252 - [Release notes](https://github.com/anthropics/claude-code-action/releases) - [Commits](https://github.com/anthropics/claude-code-action/compare/1d6de8cb0c237e7c15e9e1bdf973826ebae490cc...f27be6b65a4652f08a2ba3cff79114829b588967) Updates `actions/download-artifact` from 8.0.1 to 8.0.2 - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](https://github.com/actions/download-artifact/compare/3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c...9000827ccba6bdab643e8b6fd33ac0654aef8333) --- updated-dependencies: - dependency-name: anthropics/claude-code-action dependency-version: 1.0.252 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions - dependency-name: actions/download-artifact dependency-version: 8.0.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions ... Signed-off-by: dependabot[bot] --- .github/workflows/claude-code-review.yml | 2 +- .github/workflows/claude.yml | 2 +- .github/workflows/dependency-submission.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 56cbf42..23558ad 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -56,7 +56,7 @@ jobs: with: fetch-depth: 1 - - uses: anthropics/claude-code-action@1d6de8cb0c237e7c15e9e1bdf973826ebae490cc # v1.0.248 + - uses: anthropics/claude-code-action@f27be6b65a4652f08a2ba3cff79114829b588967 # v1.0.252 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} prompt: | diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml index ccd3be2..9b18387 100644 --- a/.github/workflows/claude.yml +++ b/.github/workflows/claude.yml @@ -36,7 +36,7 @@ jobs: with: fetch-depth: 1 - - uses: anthropics/claude-code-action@1d6de8cb0c237e7c15e9e1bdf973826ebae490cc # v1.0.248 + - uses: anthropics/claude-code-action@f27be6b65a4652f08a2ba3cff79114829b588967 # v1.0.252 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} claude_args: | diff --git a/.github/workflows/dependency-submission.yml b/.github/workflows/dependency-submission.yml index b678c64..59858e1 100644 --- a/.github/workflows/dependency-submission.yml +++ b/.github/workflows/dependency-submission.yml @@ -109,7 +109,7 @@ jobs: steps: - name: Download snapshot - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 with: name: dependency-snapshot From 563da59865b5f4b808f10f7ed86c286556f9f4c9 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 19 Oct 2026 11:54:44 -0400 Subject: [PATCH 15/21] Self CPU: subtract whole batch zones, and sum threads for an operator's own CPU The CLI's "Expensive operators" list and the GUI's advice cards each did their own row-mode subtraction. It took only a batch child's own time, so a row-mode parent was charged the rest of the batch zone, for elapsed and CPU both. On param-sniffing-posttypeid2.sqlplan the Stream Aggregate (node 4) showed 14,771 ms CPU and 14,720 ms elapsed. PlanAnalyzer already gave it 3,059 ms of elapsed. The listed CPU shares added up to 159%. - PlanAnalyzer.GetOperatorOwnCpuMs (new): own CPU summed across threads, per-thread subtraction, batch zones summed. OperatorResult gets OwnCpuMs and OwnElapsedMs (own_cpu_ms / own_elapsed_ms in JSON) from ResultMapper, and the text formatter and advice cards read those instead of subtracting again. - GetEffectiveChildCpuMs sums a batch child's zone, like the elapsed path does. This also fixes the serial path of GetOperatorMaxThreadOwnCpuMs. - GetOperatorMaxThreadOwnCpuMs returns a batch operator's busiest thread without subtracting children, like GetOperatorOwnElapsedMs. - NodeTimeAttribution.GetOwnCpuMs sums batch zones too. After: node 4 shows 3,112 ms CPU and 3,059 ms elapsed, and the shares add up to 98%. Across the 60 test plans, the Expensive operators text changes on 10. Two plans listed CPU shares over 105% before, and none do now. Four new tests in OperatorSelfTimeTests. Core tests: 1,503 total, 0 failed, 40 skipped (1,499 and 40 before). Co-Authored-By: Claude Opus 5.5 --- .../AdviceContentBuilder.CardSections.cs | 45 +-------- src/PlanViewer.Core/Output/AnalysisResult.cs | 14 +++ src/PlanViewer.Core/Output/ResultMapper.cs | 2 + src/PlanViewer.Core/Output/TextFormatter.cs | 92 ++----------------- .../Services/NodeTimeAttribution.cs | 25 ++++- .../Services/PlanAnalyzer.Timing.cs | 83 +++++++++++++++-- .../OperatorSelfTimeTests.cs | 58 ++++++++++++ 7 files changed, 182 insertions(+), 137 deletions(-) diff --git a/src/PlanViewer.App/Services/AdviceContentBuilder.CardSections.cs b/src/PlanViewer.App/Services/AdviceContentBuilder.CardSections.cs index 5fbd281..ee6b033 100644 --- a/src/PlanViewer.App/Services/AdviceContentBuilder.CardSections.cs +++ b/src/PlanViewer.App/Services/AdviceContentBuilder.CardSections.cs @@ -447,19 +447,11 @@ private static void CollectNodeTimings( // Exchanges do negligible own work and carry misleading elapsed times. if (node.PhysicalOp != "Parallelism") { - var mode = node.ActualExecutionMode ?? node.ExecutionMode; - - long ownCpu = 0; - if (node.ActualCpuMs.HasValue) - ownCpu = mode == "Batch" - ? node.ActualCpuMs.Value - : Math.Max(0, node.ActualCpuMs.Value - ChildCpuSum(node)); - - long ownElapsed = 0; - if (node.ActualElapsedMs.HasValue) - ownElapsed = mode == "Batch" - ? node.ActualElapsedMs.Value - : Math.Max(0, node.ActualElapsedMs.Value - ChildElapsedSum(node)); + /* Own times come from the analyzer, which subtracts whole batch zones and works per + thread. Subtracting only a batch child's own time charged a row-mode parent with the + rest of the zone. */ + var ownCpu = node.OwnCpuMs ?? 0; + var ownElapsed = node.OwnElapsedMs ?? 0; /* With CPU data present, elapsed-without-CPU is a cumulative timing artifact rather than work this operator did. Without it (an estimated plan) elapsed is all there @@ -478,31 +470,4 @@ than work this operator did. Without it (an estimated plan) elapsed is all there foreach (var child in node.Children) CollectNodeTimings(child, timings); } - - private static long ChildCpuSum(OperatorResult node) - { - long sum = 0; - foreach (var child in node.Children) - sum += child.ActualCpuMs ?? ChildCpuSum(child); - return sum; - } - - private static long ChildElapsedSum(OperatorResult node) - { - long sum = 0; - foreach (var child in node.Children) - { - if (child.PhysicalOp == "Parallelism" && child.Children.Count > 0) - sum += child.Children - .Where(c => c.ActualElapsedMs.HasValue) - .Select(c => c.ActualElapsedMs!.Value) - .DefaultIfEmpty(0) - .Max(); - else if (child.ActualElapsedMs.HasValue) - sum += child.ActualElapsedMs.Value; - else - sum += ChildElapsedSum(child); - } - return sum; - } } diff --git a/src/PlanViewer.Core/Output/AnalysisResult.cs b/src/PlanViewer.Core/Output/AnalysisResult.cs index 2943249..3c29ecd 100644 --- a/src/PlanViewer.Core/Output/AnalysisResult.cs +++ b/src/PlanViewer.Core/Output/AnalysisResult.cs @@ -439,6 +439,20 @@ public class OperatorResult [JsonPropertyName("actual_cpu_ms")] public long? ActualCpuMs { get; set; } + /// + /// This operator's own elapsed time, children subtracted (slowest worker thread). + /// is cumulative in row mode and is not this. + /// + [JsonPropertyName("own_elapsed_ms")] + public long? OwnElapsedMs { get; set; } + + /// + /// This operator's own CPU, children subtracted, summed across threads. + /// is cumulative in row mode and is not this. + /// + [JsonPropertyName("own_cpu_ms")] + public long? OwnCpuMs { get; set; } + [JsonPropertyName("actual_logical_reads")] public long? ActualLogicalReads { get; set; } diff --git a/src/PlanViewer.Core/Output/ResultMapper.cs b/src/PlanViewer.Core/Output/ResultMapper.cs index 3a1c441..a1b80c5 100644 --- a/src/PlanViewer.Core/Output/ResultMapper.cs +++ b/src/PlanViewer.Core/Output/ResultMapper.cs @@ -372,6 +372,8 @@ private static OperatorResult MapSingleNode(PlanNode node, CancellationToken can result.ExpectedRows = RowEstimateHelper.GetExpectedRows(node); result.ActualElapsedMs = node.ActualElapsedMs; result.ActualCpuMs = node.ActualCPUMs; + result.OwnElapsedMs = PlanAnalyzer.GetOperatorOwnElapsedMs(node); + result.OwnCpuMs = PlanAnalyzer.GetOperatorOwnCpuMs(node); result.ActualLogicalReads = node.ActualLogicalReads; result.ActualPhysicalReads = node.ActualPhysicalReads; } diff --git a/src/PlanViewer.Core/Output/TextFormatter.cs b/src/PlanViewer.Core/Output/TextFormatter.cs index bf623a9..8b2f544 100644 --- a/src/PlanViewer.Core/Output/TextFormatter.cs +++ b/src/PlanViewer.Core/Output/TextFormatter.cs @@ -449,33 +449,12 @@ private static void CollectNodeTimings( // Skip exchanges — negligible own work, misleading elapsed times if (node.PhysicalOp != "Parallelism") { - var mode = node.ActualExecutionMode ?? node.ExecutionMode; - - // Compute own CPU - long ownCpu = 0; - if (node.ActualCpuMs.HasValue) - { - if (mode == "Batch") - ownCpu = node.ActualCpuMs.Value; - else - { - var childCpuSum = GetChildCpuSum(node, cancellationToken); - ownCpu = Math.Max(0, node.ActualCpuMs.Value - childCpuSum); - } - } - - // Compute own elapsed - long ownElapsed = 0; - if (node.ActualElapsedMs.HasValue) - { - if (mode == "Batch") - ownElapsed = node.ActualElapsedMs.Value; - else - { - var childSum = GetChildElapsedSum(node, cancellationToken); - ownElapsed = Math.Max(0, node.ActualElapsedMs.Value - childSum); - } - } + // Own times come from the analyzer (PlanAnalyzer.Timing), which works per thread, + // looks through pass-through operators and subtracts whole batch zones. The row-mode + // subtraction that used to live here took only a batch child's own time, so a + // Stream Aggregate over a batch Sort was charged the Sort's whole subtree. + var ownCpu = node.OwnCpuMs ?? 0; + var ownElapsed = node.OwnElapsedMs ?? 0; // When CPU data is available, only include operators that did real CPU work. // Row-mode elapsed with 0 CPU is a cumulative timing artifact, not real work. @@ -495,63 +474,4 @@ private static void CollectNodeTimings( CollectNodeTimings(child, timings, cancellationToken); } - /// - /// Sums CPU time from all direct children, skipping through transparent - /// operators (Compute Scalar, etc.) that have no runtime stats. - /// - private static long GetChildCpuSum( - OperatorResult node, - CancellationToken cancellationToken) - { - cancellationToken.ThrowIfCancellationRequested(); - long sum = 0; - foreach (var child in node.Children) - { - if (child.ActualCpuMs.HasValue) - { - sum += child.ActualCpuMs.Value; - } - else - { - // Transparent operator (e.g. Compute Scalar) — skip through - sum += GetChildCpuSum(child, cancellationToken); - } - } - return sum; - } - - /// - /// Sums elapsed time from all direct children, skipping through exchange - /// and transparent operators. - /// - private static long GetChildElapsedSum( - OperatorResult node, - CancellationToken cancellationToken) - { - cancellationToken.ThrowIfCancellationRequested(); - long sum = 0; - foreach (var child in node.Children) - { - long childTime; - if (child.PhysicalOp == "Parallelism" && child.Children.Count > 0) - { - childTime = child.Children - .Where(c => c.ActualElapsedMs.HasValue) - .Select(c => c.ActualElapsedMs!.Value) - .DefaultIfEmpty(0) - .Max(); - } - else if (child.ActualElapsedMs.HasValue) - { - childTime = child.ActualElapsedMs.Value; - } - else - { - childTime = GetChildElapsedSum(child, cancellationToken); - } - sum += childTime; - } - return sum; - } - } diff --git a/src/PlanViewer.Core/Services/NodeTimeAttribution.cs b/src/PlanViewer.Core/Services/NodeTimeAttribution.cs index 67f2821..42d0540 100644 --- a/src/PlanViewer.Core/Services/NodeTimeAttribution.cs +++ b/src/PlanViewer.Core/Services/NodeTimeAttribution.cs @@ -78,7 +78,9 @@ private static long GetChildCpuMsSum(PlanNode node) long sum = 0; foreach (var child in node.Children) { - if (child.ActualCPUMs > 0) + if ((child.ActualExecutionMode ?? child.ExecutionMode) == "Batch") + sum += SumBatchSubtreeCpuMs(child); // standalone CPU: the zone adds up + else if (child.ActualCPUMs > 0) sum += child.ActualCPUMs; else sum += GetChildCpuMsSum(child); // skip through transparent operators @@ -86,6 +88,27 @@ private static long GetChildCpuMsSum(PlanNode node) return sum; } + /// + /// Sums CPU across a contiguous batch-mode zone, stopping at exchange boundaries. + /// The CPU twin of . + /// + private static long SumBatchSubtreeCpuMs(PlanNode node) + { + long sum = node.ActualCPUMs; + foreach (var child in node.Children) + { + if (child.PhysicalOp == "Parallelism") continue; // zone boundary + + if ((child.ActualExecutionMode ?? child.ExecutionMode) == "Batch") + sum += SumBatchSubtreeCpuMs(child); + else if (child.ActualCPUMs > 0) + sum += child.ActualCPUMs; // row mode: already cumulative + else + sum += GetChildCpuMsSum(child); // pass-through, no stats + } + return sum; + } + private static long GetChildElapsedMsSum(PlanNode node) { long sum = 0; diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs index 4ce169f..2d5c42f 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs @@ -87,27 +87,32 @@ private static long GetPerThreadOwnElapsed(PlanNode node) => GetPerThreadOwn(node, t => t.ActualElapsedMs, n => n.ActualElapsedMs); private static long GetPerThreadOwn( + PlanNode node, + Func pick, + Func nodeTotal) => + GetPerThreadOwnByThread(node, pick, nodeTotal).Values.DefaultIfEmpty(0).Max(); + + /// + /// Self time on each worker thread: the thread's time minus the same thread's time + /// in the children. Elapsed takes the max of these; total CPU takes the sum. + /// + private static Dictionary GetPerThreadOwnByThread( PlanNode node, Func pick, Func nodeTotal) { - var parentByThread = new Dictionary(); - foreach (var ts in WorkThreads(node)) - parentByThread[ts.ThreadId] = pick(ts); - var childSumByThread = new Dictionary(); foreach (var child in node.Children) AddEffectiveChildByThread(child, pick, nodeTotal, childSumByThread); - var maxSelf = 0L; - foreach (var (threadId, parentMs) in parentByThread) + var selfByThread = new Dictionary(); + foreach (var ts in WorkThreads(node)) { - childSumByThread.TryGetValue(threadId, out var childMs); - var self = Math.Max(0, parentMs - childMs); - if (self > maxSelf) maxSelf = self; + childSumByThread.TryGetValue(ts.ThreadId, out var childMs); + selfByThread[ts.ThreadId] = Math.Max(0, pick(ts) - childMs); } - return maxSelf; + return selfByThread; } /// @@ -197,6 +202,11 @@ internal static long GetOperatorMaxThreadOwnCpuMs(PlanNode node) { if (!node.HasActualStats || node.ActualCPUMs <= 0) return 0; + // Batch mode reports each thread's own CPU directly. Subtracting the + // children would take their standalone CPU off a number that never held it. + if (node.ActualExecutionMode == "Batch") + return WorkThreads(node).Select(t => t.ActualCPUMs).DefaultIfEmpty(0).Max(); + if (node.PerThreadStats.Count > 1) return GetPerThreadOwn(node, t => t.ActualCPUMs, n => n.ActualCPUMs); @@ -207,10 +217,43 @@ internal static long GetOperatorMaxThreadOwnCpuMs(PlanNode node) return Math.Max(0, node.ActualCPUMs - totalChildCpu); } + /// + /// The operator's own CPU, children subtracted, summed across threads. CPU sums + /// because every thread burned its own, so this is the figure to set beside + /// QueryTimeStats CpuTime, and self CPU above self elapsed means parallelism. + /// Serial and batch operators get the same value as + /// ; a parallel row-mode operator gets + /// all of its threads instead of the busiest one. + /// + internal static long GetOperatorOwnCpuMs(PlanNode node) + { + if (!node.HasActualStats || node.ActualCPUMs <= 0) return 0; + + if (node.ActualExecutionMode == "Batch") + return node.ActualCPUMs; + + if (node.PerThreadStats.Count > 1) + return GetPerThreadOwnByThread(node, t => t.ActualCPUMs, n => n.ActualCPUMs).Values.Sum(); + + var totalChildCpu = 0L; + foreach (var child in node.Children) + totalChildCpu += GetEffectiveChildCpuMs(child); + return Math.Max(0, node.ActualCPUMs - totalChildCpu); + } + private static long GetEffectiveChildCpuMs(PlanNode child) { if (child.PhysicalOp == "Parallelism" && child.Children.Count > 0) return child.Children.Max(GetEffectiveChildCpuMs); + + // Batch CPU is standalone, exactly like batch elapsed, so subtract the whole + // zone. Taking only the child's own value left the rest of the zone in a + // row-mode parent: a serial Stream Aggregate over a batch Sort reported + // 14,771 ms of self CPU next to 3,059 ms of self elapsed. The zone gives 3,112. + var mode = child.ActualExecutionMode ?? child.ExecutionMode; + if (mode == "Batch" && child.HasActualStats) + return SumBatchSubtreeCpuMs(child); + if (child.ActualCPUMs > 0) return child.ActualCPUMs; if (child.Children.Count == 0) @@ -293,6 +336,26 @@ private static long SumBatchSubtreeElapsedMs(PlanNode node) return sum; } + /// + /// The CPU twin of : batch operators report + /// standalone CPU, so a zone's CPU adds up. Stops at exchange zone boundaries. + /// + private static long SumBatchSubtreeCpuMs(PlanNode node) + { + long sum = node.ActualCPUMs; + foreach (var child in node.Children) + { + if (child.PhysicalOp == "Parallelism") continue; + + var childMode = child.ActualExecutionMode ?? child.ExecutionMode; + if (childMode == "Batch" && child.HasActualStats) + sum += SumBatchSubtreeCpuMs(child); + else + sum += GetEffectiveChildCpuMs(child); + } + return sum; + } + /// /// Calculates a Parallelism (exchange) operator's own elapsed time. /// Exchange times are unreliable — they accumulate wait time caused by diff --git a/tests/PlanViewer.Core.Tests/OperatorSelfTimeTests.cs b/tests/PlanViewer.Core.Tests/OperatorSelfTimeTests.cs index 2fd184e..ce7f355 100644 --- a/tests/PlanViewer.Core.Tests/OperatorSelfTimeTests.cs +++ b/tests/PlanViewer.Core.Tests/OperatorSelfTimeTests.cs @@ -1,5 +1,6 @@ using System.Linq; using PlanViewer.Core.Models; +using PlanViewer.Core.Output; using PlanViewer.Core.Services; namespace PlanViewer.Core.Tests; @@ -131,4 +132,61 @@ public void SerialPlanStillUsesItsOnlyThread() Assert.True(withStats.ActualElapsedMs >= 0); } } + + [Fact] + public void SerialRowModeParentAboveBatchZoneDoesNotAbsorbItsCpu() + { + // A serial row-mode Stream Aggregate (node 4) sits above a batch Sort (node 5) + // over a row-mode Nested Loops (node 6). Batch CPU is standalone, so the zone + // under the aggregate used 14,142 + 11,659 ms. Subtracting only the Sort's + // 14,142 left 14,771 ms of "self" CPU beside 3,059 ms of self elapsed. + var root = RootOf("param-sniffing-posttypeid2.sqlplan"); + var aggregate = TryFindNode(root, 4)!; + + Assert.InRange(PlanAnalyzer.GetOperatorOwnElapsedMs(aggregate), 3_000, 3_100); + Assert.InRange(PlanAnalyzer.GetOperatorOwnCpuMs(aggregate), 3_050, 3_200); + Assert.InRange(PlanAnalyzer.GetOperatorMaxThreadOwnCpuMs(aggregate), 3_050, 3_200); + Assert.InRange(NodeTimeAttribution.GetOwnCpuMs(aggregate), 3_050, 3_200); + } + + [Fact] + public void ParallelOwnCpuSumsThreadsAndMaxThreadKeepsTheBusiestOne() + { + // A DOP 8 Clustered Index Scan (node 1) burned 624 ms across its workers in a + // 629 ms query. Its own CPU is the sum. The busiest thread alone had 79 ms, + // which is what the external-wait formula wants from the max-thread figure. + var root = RootOf("case_predicate_plan.sqlplan"); + var scan = TryFindNode(root, 1)!; + + Assert.InRange(PlanAnalyzer.GetOperatorOwnCpuMs(scan), 600, 640); + Assert.InRange(PlanAnalyzer.GetOperatorMaxThreadOwnCpuMs(scan), 70, 90); + } + + [Fact] + public void BatchOperatorMaxThreadCpuIsNotReducedByItsChildren() + { + // Batch mode reports each thread's own CPU. The max-thread figure used to + // subtract the children's CPU from it, though the batch number never held it. + var root = RootOf("parallel_row_over_batch_plan.sqlplan"); + var hashMatch = TryFindNode(root, 2)!; + var busiest = hashMatch.PerThreadStats.Where(t => t.ThreadId > 0).Max(t => t.ActualCPUMs); + + Assert.Equal(busiest, PlanAnalyzer.GetOperatorMaxThreadOwnCpuMs(hashMatch)); + } + + [Fact] + public void ExpensiveOperatorsTextUsesTheAnalyzersOwnTimes() + { + // The CLI's "Expensive operators" list did its own row-mode subtraction and + // charged the Stream Aggregate with 14,771 ms CPU and 14,720 ms elapsed. Its + // CPU shares added up to 159%. + var plan = PlanTestHelper.LoadAndAnalyze("param-sniffing-posttypeid2.sqlplan"); + var text = TextFormatter.Format(ResultMapper.Map(plan, "param-sniffing-posttypeid2.sqlplan")); + var lines = text.Split('\n'); + var at = System.Array.FindIndex(lines, l => l.Contains("Stream Aggregate (Node 4)")); + + Assert.True(at >= 0, "node 4 is no longer listed as an expensive operator"); + Assert.Contains("3,112ms CPU", lines[at + 1]); + Assert.Contains("3,059ms elapsed", lines[at + 1]); + } } From 060c304616a286f0cd6c134eec44ec82a4265c75 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 19 Oct 2026 13:12:31 -0400 Subject: [PATCH 16/21] Update two comments for the analyzer-owned own times The advice cards no longer mirror TextFormatter's attribution, only its filtering, ranking and formatting; both read OwnCpuMs and OwnElapsedMs from OperatorResult. NodeTimeAttribution's remark now names GetOperatorOwnCpuMs. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01UoaQ2xKMpZEXvm4HncT8ND --- .../Services/AdviceContentBuilder.CardSections.cs | 5 +++-- src/PlanViewer.Core/Services/NodeTimeAttribution.cs | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/PlanViewer.App/Services/AdviceContentBuilder.CardSections.cs b/src/PlanViewer.App/Services/AdviceContentBuilder.CardSections.cs index ee6b033..34ce092 100644 --- a/src/PlanViewer.App/Services/AdviceContentBuilder.CardSections.cs +++ b/src/PlanViewer.App/Services/AdviceContentBuilder.CardSections.cs @@ -382,8 +382,9 @@ private static void AddParameters(StackPanel body, StatementResult stmt) /// takes — deliberately the same three lines writes for this /// statement, character for character. /// - /// The attribution below mirrors TextFormatter's, which owns it for the text view and - /// keeps it private. Rather than trust two copies to stay in step, + /// Both read the own times PlanAnalyzer put on each , but the + /// filtering, ranking and formatting below mirror TextFormatter's, which keeps its copy + /// private. Rather than trust two copies to stay in step, /// AdviceCardsTests.TheCardsOperatorLinesAreTheReportsOperatorLines reads the report /// and asserts these strings appear in it, so a change to one copy fails loudly instead of /// showing two different top-five lists in the same window. diff --git a/src/PlanViewer.Core/Services/NodeTimeAttribution.cs b/src/PlanViewer.Core/Services/NodeTimeAttribution.cs index 42d0540..9408c88 100644 --- a/src/PlanViewer.Core/Services/NodeTimeAttribution.cs +++ b/src/PlanViewer.Core/Services/NodeTimeAttribution.cs @@ -15,8 +15,9 @@ namespace PlanViewer.Core.Services; /// /// This is the display-grade attribution used for node coloring and the /// properties panel. The analysis pipeline uses the more elaborate per-thread -/// calculation in (GetOperatorOwnElapsedMs / -/// GetOperatorMaxThreadOwnCpuMs); the two are intentionally separate. +/// calculation in (GetOperatorOwnElapsedMs, +/// GetOperatorOwnCpuMs, GetOperatorMaxThreadOwnCpuMs); the two are intentionally +/// separate. /// public static class NodeTimeAttribution { From da1f33e1d87c02fd58ff5a558e0bb3a0ef4b496a Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 19 Oct 2026 13:22:18 -0400 Subject: [PATCH 17/21] Fall back to the estimated mode in the analyzer's own-time batch checks GetOperatorOwnElapsedMs, GetOperatorMaxThreadOwnCpuMs and GetOperatorOwnCpuMs read only ActualExecutionMode for the node itself, while the child paths read ActualExecutionMode ?? ExecutionMode. The parser fills ActualExecutionMode from a per-thread attribute that can be missing, so a batch node without it was subtracted as a batch child but treated as row mode as a parent. All three now use the same fallback as the child paths and NodeTimeAttribution. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01UoaQ2xKMpZEXvm4HncT8ND --- src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs index 2d5c42f..fce8f08 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs @@ -53,7 +53,7 @@ private static void FindMemoryConsumersRecursive(PlanNode node, List<(string Lab /// internal static long GetOperatorOwnElapsedMs(PlanNode node) { - if (node.ActualExecutionMode == "Batch") + if ((node.ActualExecutionMode ?? node.ExecutionMode) == "Batch") return node.ActualElapsedMs; // Parallel plan with per-thread data: calculate self-time per thread @@ -204,7 +204,7 @@ internal static long GetOperatorMaxThreadOwnCpuMs(PlanNode node) // Batch mode reports each thread's own CPU directly. Subtracting the // children would take their standalone CPU off a number that never held it. - if (node.ActualExecutionMode == "Batch") + if ((node.ActualExecutionMode ?? node.ExecutionMode) == "Batch") return WorkThreads(node).Select(t => t.ActualCPUMs).DefaultIfEmpty(0).Max(); if (node.PerThreadStats.Count > 1) @@ -229,7 +229,7 @@ internal static long GetOperatorOwnCpuMs(PlanNode node) { if (!node.HasActualStats || node.ActualCPUMs <= 0) return 0; - if (node.ActualExecutionMode == "Batch") + if ((node.ActualExecutionMode ?? node.ExecutionMode) == "Batch") return node.ActualCPUMs; if (node.PerThreadStats.Count > 1) From 3761fcb45e0498697c1a2e6b0eba09c6ab6c5ecd Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 19 Oct 2026 13:34:00 -0400 Subject: [PATCH 18/21] Keep a batch node's total CPU when it has no per-thread rows GetOperatorMaxThreadOwnCpuMs returned 0 for a batch node with no PerThreadStats. Parsed plans always have at least one thread row when ActualCPUMs is above 0, but a node built in code may not, so fall back to ActualCPUMs as GetOperatorOwnCpuMs does. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01UoaQ2xKMpZEXvm4HncT8ND --- src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs index fce8f08..8f41ad5 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs @@ -204,8 +204,9 @@ internal static long GetOperatorMaxThreadOwnCpuMs(PlanNode node) // Batch mode reports each thread's own CPU directly. Subtracting the // children would take their standalone CPU off a number that never held it. + // A node with no per-thread rows (built in code, not parsed) keeps its total. if ((node.ActualExecutionMode ?? node.ExecutionMode) == "Batch") - return WorkThreads(node).Select(t => t.ActualCPUMs).DefaultIfEmpty(0).Max(); + return WorkThreads(node).Select(t => t.ActualCPUMs).DefaultIfEmpty(node.ActualCPUMs).Max(); if (node.PerThreadStats.Count > 1) return GetPerThreadOwn(node, t => t.ActualCPUMs, n => n.ActualCPUMs); From b59e0740d9c339dd6209bf9cf0686a2f6f5049c5 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 19 Oct 2026 13:49:59 -0400 Subject: [PATCH 19/21] get_expensive_operators ranks by own time, not cumulative elapsed (#658) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01UoaQ2xKMpZEXvm4HncT8ND --- src/PlanViewer.App/Mcp/McpInstructions.cs | 2 +- src/PlanViewer.App/Mcp/McpPlanTools.cs | 5 +- .../ReplSurface/PlanReplModule.cs | 2 +- .../Output/PlanOperationResults.cs | 16 ++++ .../Services/PlanOperations.cs | 25 +++++- .../PlanOperationsTests.cs | 78 ++++++++++++++++++- 6 files changed, 119 insertions(+), 9 deletions(-) diff --git a/src/PlanViewer.App/Mcp/McpInstructions.cs b/src/PlanViewer.App/Mcp/McpInstructions.cs index 5ab3a69..09771be 100644 --- a/src/PlanViewer.App/Mcp/McpInstructions.cs +++ b/src/PlanViewer.App/Mcp/McpInstructions.cs @@ -43,7 +43,7 @@ Each loaded plan gets a unique `session_id`. Use `list_plans` to see all loaded | `get_plan_warnings` | Warnings only, filterable by severity | | `get_missing_indexes` | Missing index suggestions with CREATE INDEX statements | | `get_plan_parameters` | Parameter details with compiled vs runtime value comparison | - | `get_expensive_operators` | Top N costly operators by cost or actual elapsed time | + | `get_expensive_operators` | Top N operators by cost (estimated plans) or own time (actual plans: self time with children subtracted, the larger of own CPU and own elapsed, exchanges excluded) | | `get_plan_xml` | Raw showplan XML | | `compare_plans` | Side-by-side comparison of two plans | | `get_repro_script` | Generates paste-ready T-SQL reproduction script | diff --git a/src/PlanViewer.App/Mcp/McpPlanTools.cs b/src/PlanViewer.App/Mcp/McpPlanTools.cs index b71cf29..1a39cdc 100644 --- a/src/PlanViewer.App/Mcp/McpPlanTools.cs +++ b/src/PlanViewer.App/Mcp/McpPlanTools.cs @@ -227,7 +227,10 @@ public static string GetPlanParameters( [McpServerTool(Name = "get_expensive_operators")] [Description("Returns the top N most expensive operators from a loaded plan, ranked by cost percentage " + - "or actual elapsed time (if available). Useful for quickly finding bottleneck operators.")] + "for estimated plans, or by own (self) time for actual plans: children subtracted, the larger of own CPU " + + "and own elapsed (own_time_ms), with exchange (Parallelism) operators excluded. actual_elapsed_ms and " + + "actual_cpu_ms are cumulative in row mode; use own_elapsed_ms and own_cpu_ms for the operator's own work. " + + "Useful for quickly finding bottleneck operators.")] public static string GetExpensiveOperators( PlanSessionManager sessionManager, PlanOperations operations, diff --git a/src/PlanViewer.Cli/ReplSurface/PlanReplModule.cs b/src/PlanViewer.Cli/ReplSurface/PlanReplModule.cs index b56141d..e8633be 100644 --- a/src/PlanViewer.Cli/ReplSurface/PlanReplModule.cs +++ b/src/PlanViewer.Cli/ReplSurface/PlanReplModule.cs @@ -53,7 +53,7 @@ public void Map(IReplMap map) (string id, CancellationToken cancellationToken, [Description("Number of operators to return (1-100)")] int top = 10) => Execute(() => operations.GetExpensiveOperators(id, top, cancellationToken))) - .WithDescription("List the most expensive operators") + .WithDescription("List the most expensive operators (cost for estimated plans; own time, children subtracted, for actual plans)") .ReadOnly(); scope.Map( diff --git a/src/PlanViewer.Core/Output/PlanOperationResults.cs b/src/PlanViewer.Core/Output/PlanOperationResults.cs index d5cd55d..715b1c1 100644 --- a/src/PlanViewer.Core/Output/PlanOperationResults.cs +++ b/src/PlanViewer.Core/Output/PlanOperationResults.cs @@ -130,12 +130,28 @@ public sealed class ExpensiveOperatorItem [JsonPropertyName("actual_rows")] public long ActualRows { get; init; } + /// + /// Elapsed time reported for the operator. In row mode this is cumulative (it includes every + /// operator below it), so it is not the operator's own work; see . + /// [JsonPropertyName("actual_elapsed_ms")] public long ActualElapsedMs { get; init; } + /// + /// CPU time reported for the operator. In row mode this is cumulative (it includes every + /// operator below it), so it is not the operator's own work; see . + /// [JsonPropertyName("actual_cpu_ms")] public long ActualCpuMs { get; init; } + /// The operator's own elapsed time, children subtracted. 0 when unavailable (estimated plans). + [JsonPropertyName("own_elapsed_ms")] + public long OwnElapsedMs { get; init; } + + /// The operator's own CPU time, children subtracted. 0 when unavailable (estimated plans). + [JsonPropertyName("own_cpu_ms")] + public long OwnCpuMs { get; init; } + [JsonPropertyName("logical_reads")] public long LogicalReads { get; init; } diff --git a/src/PlanViewer.Core/Services/PlanOperations.cs b/src/PlanViewer.Core/Services/PlanOperations.cs index 2a0967c..e16a5a8 100644 --- a/src/PlanViewer.Core/Services/PlanOperations.cs +++ b/src/PlanViewer.Core/Services/PlanOperations.cs @@ -428,7 +428,8 @@ internal ExpensiveOperatorsResult GetExpensiveOperators( { var candidate = new RankedOperator(node, statementText); hasActuals |= node.ActualElapsedMs > 0; - AddRanked(topByActual, candidate, top, rankByActuals: true); + if (IsEligibleForOwnTimeRanking(node)) + AddRanked(topByActual, candidate, top, rankByActuals: true); AddRanked(topByCost, candidate, top, rankByActuals: false); }); } @@ -437,7 +438,7 @@ internal ExpensiveOperatorsResult GetExpensiveOperators( return new ExpensiveOperatorsResult { SessionId = session.SessionId, - RankedBy = hasActuals ? "actual_elapsed_ms" : "cost_percent", + RankedBy = hasActuals ? "own_time_ms" : "cost_percent", Operators = ranked.Select(item => new ExpensiveOperatorItem { NodeId = item.Node.NodeId, @@ -448,6 +449,8 @@ internal ExpensiveOperatorsResult GetExpensiveOperators( ActualRows = item.Node.ActualRows ?? 0, ActualElapsedMs = item.Node.ActualElapsedMs ?? 0, ActualCpuMs = item.Node.ActualCpuMs ?? 0, + OwnElapsedMs = item.Node.OwnElapsedMs ?? 0, + OwnCpuMs = item.Node.OwnCpuMs ?? 0, LogicalReads = item.Node.ActualLogicalReads ?? 0, PhysicalReads = item.Node.ActualPhysicalReads ?? 0, ObjectName = useBareObjectNames ? item.Node.BareObjectName : item.Node.ObjectName, @@ -652,6 +655,20 @@ private static void VisitOperators( } } + // Same inclusion rule as the "Expensive operators" list in TextFormatter.CollectNodeTimings: + // exchanges are skipped; with CPU data only nodes with own CPU count, otherwise own elapsed. + private static bool IsEligibleForOwnTimeRanking(OperatorResult node) + { + if (node.PhysicalOp == "Parallelism") + return false; + return node.ActualCpuMs.HasValue + ? (node.OwnCpuMs ?? 0) > 0 + : (node.OwnElapsedMs ?? 0) > 0; + } + + private static long OwnTimeScore(OperatorResult node) => + Math.Max(node.OwnCpuMs ?? 0, node.OwnElapsedMs ?? 0); + private static void AddRanked( List ranked, RankedOperator candidate, @@ -659,13 +676,13 @@ private static void AddRanked( bool rankByActuals) { var candidateScore = rankByActuals - ? candidate.Node.ActualElapsedMs ?? 0 + ? OwnTimeScore(candidate.Node) : candidate.Node.CostPercent; var insertAt = 0; while (insertAt < ranked.Count) { var item = ranked[insertAt]; - var score = rankByActuals ? item.Node.ActualElapsedMs ?? 0 : item.Node.CostPercent; + var score = rankByActuals ? OwnTimeScore(item.Node) : item.Node.CostPercent; if (candidateScore > score) { break; diff --git a/tests/PlanViewer.Core.Tests/PlanOperationsTests.cs b/tests/PlanViewer.Core.Tests/PlanOperationsTests.cs index 0d43d2c..a8ba6ba 100644 --- a/tests/PlanViewer.Core.Tests/PlanOperationsTests.cs +++ b/tests/PlanViewer.Core.Tests/PlanOperationsTests.cs @@ -183,13 +183,14 @@ public async Task GetExpensiveOperators_RanksEstimatedPlansByCostAndHonorsTop() [Fact] - public void GetExpensiveOperators_UsesActualElapsedTimeAndKeepsStableTies() + public void GetExpensiveOperators_UsesOwnTimeAndKeepsStableTies() { var firstTie = new PlanViewer.Core.Output.OperatorResult { NodeId = 2, PhysicalOp = "First tie", ActualElapsedMs = 10, + OwnElapsedMs = 10, CostPercent = 1 }; var secondTie = new PlanViewer.Core.Output.OperatorResult @@ -197,6 +198,7 @@ public void GetExpensiveOperators_UsesActualElapsedTimeAndKeepsStableTies() NodeId = 3, PhysicalOp = "Second tie", ActualElapsedMs = 10, + OwnElapsedMs = 10, CostPercent = 99 }; var root = new PlanViewer.Core.Output.OperatorResult @@ -229,10 +231,82 @@ public void GetExpensiveOperators_UsesActualElapsedTimeAndKeepsStableTies() var result = operations.GetExpensiveOperators(session, top: 2, TestContext.Current.CancellationToken); - Assert.Equal("actual_elapsed_ms", result.RankedBy); + Assert.Equal("own_time_ms", result.RankedBy); Assert.Equal([2, 3], result.Operators.Select(item => item.NodeId)); } + + private static async Task<(PlanOperations Operations, PlanViewer.Core.Models.PlanSession Session)> OpenFixtureAsync(string planFile) + { + var catalog = new InMemoryPlanCatalog(); + var operations = new PlanOperations(catalog); + var opened = await operations.OpenAsync( + Path.GetFullPath(Path.Combine("Plans", planFile)), TestContext.Current.CancellationToken); + var session = Assert.IsType(catalog.GetSession(opened.SessionId)); + return (operations, session); + } + + private static List ExpensiveOperatorNodeIdsFromText(string text) + { + var ids = new List(); + var lines = text.Split('\n'); + var at = Array.FindIndex(lines, l => l.TrimEnd() == "Expensive operators:"); + Assert.True(at >= 0, "no Expensive operators section"); + for (var i = at + 1; i < lines.Length && lines[i].StartsWith(" "); i++) + { + var match = System.Text.RegularExpressions.Regex.Match(lines[i], @"^ \S.*\(Node (\d+)\):\s*$"); + if (match.Success) + ids.Add(int.Parse(match.Groups[1].Value)); + } + return ids; + } + + [Theory] + [InlineData("param-sniffing-posttypeid2.sqlplan")] + [InlineData("case_predicate_plan.sqlplan")] + public async Task GetExpensiveOperators_MatchesTheTextSummaryExpensiveOperators(string planFile) + { + var (operations, session) = await OpenFixtureAsync(planFile); + var plan = PlanViewer.Core.Tests.PlanTestHelper.LoadAndAnalyze(planFile); + var text = PlanViewer.Core.Output.TextFormatter.Format(PlanViewer.Core.Output.ResultMapper.Map(plan, planFile)); + + var result = operations.GetExpensiveOperators(session, top: 5, TestContext.Current.CancellationToken); + + var expected = ExpensiveOperatorNodeIdsFromText(text); + Assert.NotEmpty(expected); + Assert.Equal("own_time_ms", result.RankedBy); + Assert.Equal(expected, result.Operators.Select(item => item.NodeId)); + Assert.DoesNotContain(result.Operators, item => item.PhysicalOp == "Parallelism"); + } + + [Fact] + public async Task GetExpensiveOperators_WorkerOutranksItsCumulativeParents() + { + var (operations, session) = await OpenFixtureAsync("case_predicate_plan.sqlplan"); + + var result = operations.GetExpensiveOperators(session, top: 5, TestContext.Current.CancellationToken); + + var first = result.Operators[0]; + Assert.Equal(1, first.NodeId); + Assert.Contains("Clustered Index Scan", first.PhysicalOp); + Assert.True(first.OwnCpuMs > 0); + Assert.DoesNotContain(result.Operators, item => item.PhysicalOp == "Parallelism"); + } + + [Fact] + public async Task GetExpensiveOperators_RanksActualPlansByOwnTimeAndEstimatedPlansByCost() + { + var (operations, actual) = await OpenFixtureAsync("case_predicate_plan.sqlplan"); + Assert.Equal("own_time_ms", + operations.GetExpensiveOperators(actual, top: 3, TestContext.Current.CancellationToken).RankedBy); + + var estimatedCatalog = new InMemoryPlanCatalog(); + var estimatedOps = new PlanOperations(estimatedCatalog); + var opened = await estimatedOps.OpenAsync( + Path.GetFullPath(Path.Combine("Plans", "row_goal_plan.sqlplan")), TestContext.Current.CancellationToken); + Assert.Equal("cost_percent", + estimatedOps.GetExpensiveOperators(opened.SessionId, 3, TestContext.Current.CancellationToken).RankedBy); + } [Fact] public async Task GetMissingIndexes_BoundsTheReturnedItemsAndReportsTruncation() { From 6ed2ec7aaa71303ed52338f5aa041b6d4e08a21b Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 19 Oct 2026 14:01:24 -0400 Subject: [PATCH 20/21] deps: apply the 13 patch-and-minor bumps from #660; ignore SelectionChanged raised during XAML load in SettingsWindow Avalonia 12.1.4 raises SelectionChanged while the XAML loads, before the named fields are assigned, so opening Settings threw a NullReferenceException. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01UoaQ2xKMpZEXvm4HncT8ND --- Directory.Packages.props | 26 +++++++++---------- .../Dialogs/SettingsWindow.axaml.cs | 6 +++++ 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index 0c341a5..a2f19eb 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -5,28 +5,28 @@ - + - - - - + + + + - - - - + + + + - - + + - + @@ -43,7 +43,7 @@ - + - 1.30.0 + 1.31.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 1ea67ed..093173e 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.30.0.0")] -[assembly: AssemblyFileVersion("1.30.0.0")] +[assembly: AssemblyVersion("1.31.0.0")] +[assembly: AssemblyFileVersion("1.31.0.0")] diff --git a/src/PlanViewer.Ssms/source.extension.vsixmanifest b/src/PlanViewer.Ssms/source.extension.vsixmanifest index 6f12718..0718f2d 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