Stop analysis-finding alerts delivering their Diagnosis facts twice

[erikdarlingdata]

Analysis-finding alerts render their Diagnosis facts twice on every channel. This is the open review thread on #3302 (PerformanceMonitor.Notifications/AlertDetailText.cs), settled.

The regression

#3302 threaded each alert's prose DetailText through to all five delivery channels — the #3296 field defect was that it reached none. It gates delivery on AlertDetailText.ProseForDelivery, which suppresses the prose when it is textually equal to the flattening of the alert's own structured context.

FindingMessageFormatter is a third producer of (Context, DetailText) pairs and nothing in #3302 touched it. BuildContext puts story / severity / notify threshold / confidence / fact count / database / window into a Diagnosis item; DetailText formats the same values with different labels (Facts in chain vs Facts, one combined severity line vs two fields) and a different window separator. Different text, so the equality gate never fires, so both are delivered — on the largest alert category there is (CPU spikes, plan regressions, index and RCSI findings), on both SKUs, on email, Teams, Slack, PagerDuty and the generic webhook.

What this does, and the seam it rests on

The prose is suppressed at delivery. The persisted value is untouched.

That distinction is the whole point. FindingMessageFormatter.DetailText is also config_alert_log.detail_text for every analysis row: the MCP alert reader, the triage endpoint, the Viewer's detail pane and the Dashboard's no-channel tray row all render it, and AlertMuteContext.PopulateFromDetailText parses it for the mute pre-fill. The alternative repair — make DetailText the flattening of its own context, as the thirteen AlertEngine fire sites do — would change that column for the largest alert category, gaining the Diagnosis heading, a separate Notify threshold field, the advice headline, the remediation-T-SQL item's heading and every drill-down item's fields.

The seam exists, at two separate call sites in each sender, and it is not a new one: deprecated/Dashboard's SendFindingAlertAsync has always omitted the detail from its delivery call while passing it to its tray row. Lite and Darling each call EmailSendCore.TrySendAsync(detailText:) for delivery and IAlertHistoryStore.RecordAlertAsync(... DetailText ...) for the row.

FindingAlert now carries the producer's own declaration — DeliverDetailText, with DeliveredProse resolving it once so the two senders cannot disagree — and the senders route DeliveredProse to the channels and DetailText to the row. Keyed on producer rather than on text, which is what the equality gate cannot do here.

Not widened to "suppress when the context's facts all appear in the prose": that also drops the #2109 AG alerts' SET HADR RESUME remedy, the one case the gate was deliberately built not to drop.

Lite's shell shares one path with the engine, so it takes a deliverProse flag defaulting to true. The two failure directions are not symmetric — delivering a redundant paragraph is cosmetic, withholding one discards an operator's only copy of the remedy, which is #3296 — so a caller that never considers it delivers the prose.

Also in here: AlertDetailText's doc said AlertEngine builds its detail text that way "at eleven fire sites". #3302 took that to thirteen. The numeral is gone rather than corrected — no test derives it, so it is a partial list with a number welded on.

Verification

Lite.Tests and Darling.Tests are net10.0-windows and cannot run on macOS, so the ACTUAL committed test sources were compiled into two net10.0 console harnesses over an xunit shim and RUN. Lite/Services/EmailAlertService.cs is compiled in as the real product source rather than shimmed — only its project is Windows-targeted — because it is the subject of two of the pins.

All three measured harness traps are controlled, not avoided. The shim's --self-check carries 6 controls that must pass (Assert.Equal on distinct-but-equal List<T>, arrays and nested collections; an async Task succeeding after two awaits; a two-case theory) and 5 that must fail, including a failure raised after an await and one thrown below the first await in a nested async call. All 6 green, all 5 red. BaseOutputPath sits inside the worktree so DocCommentHygieneTests' walk-up finds PerformanceMonitor.sln; all 77 of its cases ran and passed, which is how the new doc comments and crefs are checked.

harness	result
Lite.Tests (8 real test files + the real EmailAlertService.cs)	115 pass, 0 fail
Darling.Tests (AlertDeliveryChannelTests, CustomAlertDisplayNameTests, DocCommentHygieneTests, CSharpSourceWalkerTests)	149 pass, 0 fail, 1 SKIP

The SKIP is TheViewerRow_RendersThroughTheSharedDescriber, whose subject is the WPF ViewerAlertRow; it is reported SKIP rather than PASS because a shimmed subject would make the result the harness's. CI runs it for real.

The pins

Both directions, or the fix is indistinguishable from reverting #3302 for this seam.

• Facts once per channel — driven through the real AnalysisNotificationService, not a hand-built FindingAlert: the declaration under test is the producer's. On the Darling side it is the whole path, DarlingFindingAlertSender through the send core, the fan-out and each payload builder, read off the bytes that left the process via the loopback TCP and SMTP sinks.
• Occurrence counts, not absences. Asserting only that the prose is gone would pass equally if the structured context had been dropped instead — the opposite defect and the more expensive one, since the context carries the advice, the remediation T-SQL and the drill-down that the prose does not. Each count is paired with the same builder fed DetailText, the argument Deliver the alert's detail text on every channel #3302 shipped, which must come back with the facts twice.
• A prose that is not a restatement is still delivered, on both SKUs, measured on the wire.
• config_alert_log.detail_text is unchanged — a frozen list of lines on both SKUs, not a re-derivation of the code it pins. Darling.Tests cannot see FindingMessageFormatter at all (Notifications grants InternalsVisibleTo to Lite and Dashboard, not there), which makes that limitation useful.

Mutations

Committed before every one; each applied, run, reverted, and the revert verified by blob hash against HEAD plus a whole-tree clean check. Every run goes through a driver that BUILDS first — a source-only restore leaves the previous mutation's DLL in bin, and the previous lane lost a round to reds that were its harness's stale state.

#	mutation	result
1	the producer declares the prose independent (the shipped behaviour)	RED — 2 Lite + 2 Darling
2	DeliveredProse never suppresses	RED — 3 Lite + 3 Darling
3	DeliveredProse always suppresses	RED — the other-direction pin, both SKUs
4	Darling sender delivers DetailText instead of DeliveredProse	RED — 3 Darling, Lite green (separate seam)
5	Darling sender PERSISTS the suppressed value	RED — the constraint this approach rests on
6	Lite shell ignores deliverProse	RED — the Lite shell pin
7	Lite shell PERSISTS the suppressed value	RED — 2 Lite
8	Lite sender hardcodes deliverProse: false	RED
9	Lite sender hardcodes deliverProse: true	RED
10	BuildContext drops the Database field (the opposite defect)	RED — 1 Lite + 2 Darling; the counts are not satisfied by an absence
11	DetailText becomes the flattening of its own context (the declined approach)	RED — the persistence pin, both SKUs
12	ProseForDelivery always returns null	RED — 11 Lite + 3 Darling, including 9 of #3302's own pins and every control half

Mutation 12 is what shows the control halves are load-bearing rather than decoration, and mutation 10 that the "once" claim discriminates which rendering survived. TheSurvivingRendering_IsTheStructuredContext_NotTheProse stays green under 12, correctly — that one is about which copy remains, not how many.

Mechanics

Fresh worktree off origin/dev; origin/dev merged IN at 2a52dd5b (no overlap with any file here), never rebased, never force-pushed. Solution builds with -p:EnableWindowsTargeting=true, 0 warnings. global.json untouched — 10.0.302 is installed and the pin edit is a stale ritual. packages.lock.json churn from the macOS restore reverted. Named paths only, never git add -A. No CHANGELOG.md edit.

One correction of my own, recorded because the artefact is durable: the branch was first pushed as fix/3313-..., a number I constructed as "next" rather than read. #3313 is an open issue about a webhook batch re-rendering already-delivered incidents. Branch renamed, the wrong remote ref deleted, and five code comments that referenced it rewritten to name #3302 instead. No issue is filed for this regression — it is carried here.

CHANGELOG entry text (for the coordinator, not committed)

- Fixed analysis-finding alerts rendering their Diagnosis facts twice on every delivery channel. The
  finding's prose detail restates its own structured context under different labels, so the delivery
  gate's text comparison could not see the duplication. The prose is now suppressed at delivery for
  that producer; `config_alert_log.detail_text` is unchanged, so the MCP reader, the triage page, the
  Viewer's detail pane and the mute pre-fill all read what they always did.

[claude]

Reviewed the diff (C# only — no T-SQL touched, so the T-SQL style rules don't apply here).

Correctness / parity: Verified end to end and it holds up.

• FindingAlert gains a new required positional field DeliverDetailText — the only production construction site (AnalysisNotificationService.cs) is updated; deprecated/Dashboard's EmailAlertService only consumes FindingAlert and never constructs one, so it needed no change (and it already never delivered DetailText, matching the PR description).
• Lite (EmailAlertService.TrySendAlertEmailAsync → new deliverProse param, default true) and Darling (DarlingFindingAlertSender → alert.DeliveredProse) implement the same semantics through different but equivalent shapes: channels get the suppressed value, RecordAlertAsync/history always gets the full alert.DetailText. Confirmed both persist the un-suppressed text.
• Confirmed the core factual claim: FindingMessageFormatter.DetailText (Story/Severity/Confidence/Facts/Database/Window) is a strict subset of what BuildContext's "Diagnosis" item renders, just under different labels and a different window separator (- vs →), so hardcoding DeliverDetailText: false for every analysis finding is correct today — the two are always redundant, never just coincidentally equal.
• EmailSendCore.TrySendAsync threads the single detailText argument uniformly into email + Teams/Slack/generic/PagerDuty, so suppressing it once suppresses it on every channel consistently for both SKUs — no channel-specific gap.
• The #2109 AG-alert non-suppression case (AProseThatIsNotARestatement_IsStillDelivered / Darling's mirror test) correctly exercises the "producer declares independent prose" path, which is the one the old textual-equality gate couldn't drop and the new gate also doesn't.

Security: No new input handling, no secrets, no file/process/network surface beyond the existing test-only loopback TCP/SMTP fixtures used to observe wire bytes — fine for test code.

Style/process: New file Lite.Tests/AnalysisProseDeliveryTests.cs has the copyright header. No missing-index DMV suggestions here (n/a — no T-SQL). Didn't find any stale doc references to the old "eleven/thirteen fire sites" wording after the edit.

No findings to flag — this looks correct and keeps Lite/Darling in parity.