Skip to content

Key severity overrides on the rule that emitted the finding (#575) - #583

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/severity-overrides-575
Sep 27, 2026
Merged

erikdarlingdata merged 1 commit into
devfrom
fix/severity-overrides-575

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

What does this PR do?

Fixes #575. A severity override now reaches every finding that its rule emits.

TryOverrideSeverity found a finding's rule by matching WarningType against the RuleWarningTypes table. The table had no entry for these findings, so an override for them did nothing and gave no error:

  • rules 34, 35, 36, 37 and 39 (every finding they emit)
  • rule 30's Low Impact Index and Duplicate Index Suggestions (the table had only Wide Index Suggestion)
  • rule 10's RID Lookup (the table had only Key Lookup). The issue did not list this one. I found it while I tagged the rules.

The changes:

  1. PlanWarning has a new RuleNumber property (int?). Each rule sets it on each finding that it adds. That is 48 places in PlanAnalyzer.Node.cs and PlanAnalyzer.Statement.cs. A script took each number from the enclosing RuleNN_... method. I checked the result by hand against the rule gates.
  2. TryOverrideSeverity reads RuleNumber and does not look at the name.
  3. The RuleWarningTypes table is gone. So are its reverse map WarningTypeToRule, which nothing read, and the static constructor that built it.

Some things did not change:

  • The analyzer still never overrides the engine's own warnings (Source = SqlServer), as the issue asks. Those warnings carry no rule number.
  • Rules 7 and 29 add no findings of their own. They adjust the engine's spill and implicit conversion warnings. So an override for rule 7 or rule 29 still has no effect, as before this PR.
  • The wait-stats findings from BenefitScorer come from no rule, so they carry no number. The scorer runs after the analyzer, so overrides never reached them before this PR either.
  • The JSON and MCP output do not include the new property. ResultMapper copies named fields into its own types.

Which component(s) does this affect?

  • Desktop App (PlanViewer.App)
  • Core Library (PlanViewer.Core)
  • CLI Tool (PlanViewer.Cli)
  • SSMS Extension (PlanViewer.Ssms)
  • Tests
  • Documentation

How was this tested?

  • New SeverityOverrideTests, 70 cases:
    • the issue's repro (rule 36 set to Info and rule 30 set to Critical)
    • rules 36, 37 and 39 and all three rule 30 finding types, built in code
    • rule 10's RID Lookup, rule 34 and rule 35, from fixture plans
    • one case for each of the 60 fixture plans. Each analyzer finding must carry a rule number. When that rule is disabled, the analyzer must add no finding with that number. When every rule is set to Info, and then to Critical, every analyzer finding must take that severity. The engine's warnings must keep their severity in both runs.
  • Against the old name lookup, 29 of the 70 cases fail. The 41 that pass include Wide Index Suggestion, which the old table had, and the fixture plans with no unmapped finding.
  • Full suite after the rebase onto dev with Fix four analyzer rule gates (#577, #576, #579, #578) #582: 889 passed, 0 failed, 1 skipped. The warning and comparison golden files did not change. The Release build has 0 compiler warnings. Windows only.

Checklist

  • I have read the contributing guide
  • My code builds with zero warnings (Release build through dotnet test)
  • All tests pass (dotnet test)
  • I have not introduced any hardcoded credentials or server names

🤖 Generated with Claude Code

https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR

Each analyzer rule now stamps its number on the findings it adds (PlanWarning.RuleNumber),
and TryOverrideSeverity reads that number instead of matching WarningType against a
rule-to-name table. The table had no entry for rules 34-37 and 39, for rule 30's Low Impact
Index and Duplicate Index Suggestions, or for rule 10's RID Lookup, so overrides for them
were silently ignored. The table, its unused reverse map and the static constructor that
built it are gone. Engine warnings are still never overridden.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR
@claude

claude Bot commented Sep 27, 2026

Copy link
Copy Markdown

Reviewed. Verified against the full pre/post file contents (not just the diff hunks):

  • All 48 new PlanWarning { ... } construction sites across PlanAnalyzer.Node.cs (26) and PlanAnalyzer.Statement.cs (22) now set RuleNumber, matching the PR description's count exactly.
  • Rules 7 (Rule07_SpillSeverity) and 29 (Rule29_ImplicitConversionSeek) confirmed to only mutate pre-existing engine warnings via node.Warnings.ToList() — they never call .Add, so it's correct that they carry no RuleNumber and that overrides for them remain no-ops, as documented.
  • TryOverrideSeverity correctly bails on Source == SqlServer before checking RuleNumber, so engine warnings still can't be targeted by rule-number overrides.
  • AnalyzerConfig.GetSeverityOverride/RulesConfig.SeverityOverrides (untouched by this PR) already key on int, consistent with the new int? RuleNumber field — no type mismatch.
  • Removing RuleWarningTypes/WarningTypeToRule and the static ctor also cleans up a stray orphaned /// <summary> doc comment that was sitting in front of private record ScanImpact — good catch.
  • Test coverage is solid: the corpus-wide theory (EveryAnalyzerFindingFollowsItsRule) asserts every PerformanceStudio-sourced finding across all 60 fixtures carries a rule number, that disabling a rule removes only its own findings, and that a blanket override changes severity without adding/removing/reordering findings — good regression protection against a future rule that forgets to stamp its number.

No T-SQL, no version-bump files, no PlanViewer.Web linked-compile changes in this PR, so those checklist items don't apply. No concerns.

@erikdarlingdata
erikdarlingdata merged commit 126aeb3 into dev Sep 27, 2026
3 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/severity-overrides-575 branch September 27, 2026 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant