Slack analysis pages stay inside the 50-block message limit: details are budgeted like the prose, dropped from the end with the omission stated (#3612)

[erikdarlingdata]

For #3612 — the watcher closes it at merge via the keyword below.

Three lost pages, and the read that named the breach

Three analysis notification pages died on Slack with HTTP 400: invalid_attachments across two builds — Sep 17 00:52Z and Sep 18 01:21Z on .444, Sep 18 15:42Z on 3.8.0 — while a three-fact analysis page delivered through the same webhook the same hour. All three were the same five-fact story rooted in a plan regression (PLAN_REGRESSION→CPU_SPIKE→PARAMETER_SENSITIVITY→QUERY_SPILLS→TEMPDB_USAGE, severity 1.75, confidence 0.80): the product's highest-severity compound finding, and the push is the point of it. The rows persisted honestly with their transport error (the #3446 arm), so this arrived as evidence.

The brief's leading hypothesis was that the Advice body had crossed Slack's 3,000-character text-object ceiling — the #3493 failure one shelf over, on a text object #3493 never touched. The monitoring seat's store read refuted it before a line was written, and that read is the load-bearing fact of this PR: across all fifteen analysis pages in two days of config_alert_log, the three failed rows' longest body was 2,105–2,122 characters and a delivered row carried a 2,835-character body. Text length does not separate them. Detail count does, cleanly: the failed pages carried 19, 20 and 21 detail items; the largest delivered page carried 18.

The mechanism, measured from source

BuildSlackPayload renders every structured detail as a divider plus one or more sections — a body detail costs two blocks, a field-bearing detail costs one plus a section per ten fields — and the total was never checked against Slack's 50-block message limit. Slack rejects a fifty-first block with the same invalid_attachments string it uses for an oversized text object, which is how #3612 hid behind #3493 for two builds.

I rebuilt the lost page from source rather than guessing: the five-fact story, the seven drill-downs the Lite collector attaches for those fact keys (spike_peak, queries_at_spike, top_cpu_queries, top_spilling_queries, tempdb_breakdown, parameter_sensitive_queries, regressed_queries), five rows each with 500-character query text, the value-stated advice frozen into StoryText by FactAdvice.Compose, all through FindingMessageFormatter.BuildContext — the same call the notification service makes. The producer emits ten fixed items (Diagnosis, Advice, the Remediation T-SQL pointer, seven drill-downs) and then one incident item per distinct query_hash across the three hash-bearing drill-downs, up to fifteen. That is exactly what varies between 19 and 21 items in production.

Executed on the built assembly (the Notifications library targets plain net10.0, so it runs here):

shape	items	blocks (before)	longest section text	longest field text
3 incidents	13	43	597 (Advice body)	318
6 incidents	16	49 — last shape that fits	597	318
7 incidents	17	51 — the failure	597	318
9 / 10 / 11 incidents	19 / 20 / 21 — the three lost pages	55 / 57 / 59	597	318
15 incidents (max producible)	25	67	597	318

The fixed cost is exactly countable: head = 2 (one header block, one field section — the lead fields are five to seven, never past ten), footer = 2 (the triage actions button, the context line; 1 without a triage URL), the ten fixed items = 33 (Diagnosis 2, Advice 2, T-SQL 2, Spike Peak 2, Queries At Spike 5, Top Cpu Queries 4, Top Spilling Queries 3, Tempdb Breakdown 3, Parameter Sensitive Queries 5, Regressed Queries 5), each incident 2. Blocks = 37 + 2×incidents; the seventh incident is the fifty-first block. No text object came within 2,400 characters of its ceiling — the 2,777-character body on the page is the Remediation T-SQL, which Slack never inlines (it renders the fixed pointer). The per-text-object limits never touched this failure mode.

One caveat the numbers force: production's 18-versus-19 separator was a property of those pages' widths, not a rule. My 18-item fixture costs 53 blocks (its drill-downs are wider than whatever the delivered 18-item page carried), and a four-fact story with verbose drill-downs reaches the line with fewer items. So the budget is on blocks, computed from the rendered runs, not on any count upstream of them — and there is a test arm that pins the same 18 items landing either side of the line depending on width.

The fix: one discipline, not two

#3493 bounded the alert's prose; the analysis pages deliver no prose at all (DeliverDetailText: false) and carry the whole finding as details. Rather than a second, detail-side contract to keep in step with the first, the details path now composes under #3493's discipline, reusing its machinery:

• The block budget is the fix. BuildSlackPayload composes the message in three runs, budgeted in the order of what is fixed: head (2), footer (1–2), then the details take what remains under 50 — less one block held for the prose when there is prose — and the prose packs last into exactly what is left. Before this change the prose's Math.Max(1, …) floor was applied after the details had spent the budget, which is itself a fifty-first block when both halves are heavy; a test arm pins that case landing on exactly 50 with both omissions stated.
• What drops first. AddSlackDetailBlocks renders every detail whole, and if the total fits, appends the runs exactly as the old loop did. If not, it keeps the longest leading run that fits beside the omission item. Producers already order most-essential-first — the analysis path emits Diagnosis, Advice, the T-SQL pointer, then drill-downs, then incident fingerprints — so the finding survives and the incidents' tail goes. Nothing is skipped to fit a smaller item behind it: a reader who sees "Incident 5 of 11" and then the omission knows exactly where the message stopped. A body detail at the boundary is not dropped whole; it takes the blocks that remain and states its own line omission, because on an analysis page the body is the advice and a shortened advice beats a missing one. BuildContext's doc now says its order is load-bearing on Slack.
• The omission is stated and names the items: a divider and one section — *Omitted from this message* / N more details did not fit Slack's 50-block message limit: Incident 6 of 11; …; Incident 11 of 11 - see email or in-app Alert Details for the full text. Headings are a detail's identity, so the reader knows what to open the alert for; the list is capped at 1,000 characters so the omission section respects the ceiling it exists for. Its two-block cost is charged before any detail is kept, so it is always affordable when needed. The pointer text is now one constant shared by all three omission sites (prose lines, dropped details, cut fields), so they cannot drift.
• Per-text-object caps, as hygiene. AddSlackProseSections takes its header as a parameter, so a detail body routes through the same splitter as the prose with *Heading* as its first-section header — same packing, same continuation marker, same omission wording. Field texts are capped at Slack's 2,000-character field limit with the cut stated inline (… (N more characters - see email or in-app Alert Details for the full text)), two-pass so the note's own digits cannot push it over. Headings are bounded at 500 so the splitter's capacity is always positive. Plainly: no observed page has come within reach of any of these; the three lost pages would have been lost with them alone.

A page that fits is byte-identical. The 3- and 6-incident fixtures serialize to the same bytes before and after (diffed against a build of origin/dev), and the test restates the pre-#3612 loop verbatim as its oracle. Every engine alert and every analysis page that ever delivered lives on this path.

After the fix every shape from 7 to 15 incidents lands at 49 blocks: the ten fixed items, five incidents, the omission item, head and footer.

Teams and email

Teams is an O365 MessageCard, not an Adaptive Card. The format has no section-count limit (the reference's "don't include more than 10 sections" is a readability guideline, not a rejection); the connector's one hard ceiling is the ~28 KB payload. Measured: the lost 21-item page renders at 16.6 KB, the heaviest producible shape (25 items, fifteen incidents) at 17.4 KB — about 210 bytes per incident, so the cap is some fifty incidents past anything the producers can emit. No Teams change ships; the margin is documented at the Teams builder and pinned by a test so a producer that widens the page fails there rather than in a lost delivery. Email renders the same details as HTML with no such limit; nothing to do.

Tests

Lite.Tests/SlackDetailsSizeTests.cs, thirteen arms beside #3493's SlackProseSplitTests.cs: the fitting shapes byte-for-byte against the restated old loop; the failing shapes (7 — the first past the line — and 9/10/11 — the three production pages — and 15) inside every ceiling with the finding whole, incidents kept as an ordered prefix, and the omission naming the count and every dropped heading, each arm guarded by an assertion that the old rendering exceeds 50 so it cannot pass vacuously; blocks-not-count (the same 18 items fitting or not by width); a body at the boundary taking what remains; heavy prose plus heavy details landing on exactly 50; body split reassembly under its heading; the field cut's arithmetic (kept + omitted = original); the Teams cap pin.

These cannot run in Lite.Tests on this Mac (net10.0-windows), so I compiled the same test file — plus SlackProseSplitTests, AlertIncidentRenderTests, TriageLinkTests, WebhookPayloadBrandingTests and CustomAlertRenderTests — into a throwaway net10.0 xunit project named Lite.Tests against the built Notifications assembly: 66 green. Lite.Tests itself builds under -c Release -p:EnableWindowsTargeting=true with 0 warnings. CI is the first Windows execution.

Evidence credit: the monitoring seat's config_alert_log read (tick record 20260918T164232Z and the follow-up length/count discriminator) is what re-aimed this lane from text length to block count before any code was written. Related: #3493 / #3494 (the prose half of this discipline, and the tail-side note that predicted this), #3466.

Closes #3612

[claude]

Non-blocking robustness note: this blockBudget (and the one passed into AddSlackDetailBlocks a few lines up, and the remaining - 1 passed into RenderSlackDetail from inside AddSlackDetailBlocks) is only ever >= 1 because of careful arithmetic across three call sites — there's no local guard. PackProseLines's repack loop (finalBlock = texts.Count == blockBudget - 1) depends on eventually hitting finalBlock == true to break before it walks placed past the end of lines; if a blockBudget <= 0 ever reached it, finalBlock would never be true and the trailing lines[placed] would throw once placed == lines.Count. Today that's unreachable (head/footer are small fixed constants and the remaining < 2 check upstream keeps body budgets >= 1), but the invariant lives entirely in caller-side subtraction rather than in PackProseLines/AddSlackProseSections themselves. A Math.Max(1, blockBudget) at the top of one of those two functions would make the safety local instead of relying on every future edit to this budget math getting it right.

[erikdarlingdata]

Taken, in 4e20f4a: AddSlackProseSections now passes Math.Max(1, blockBudget) into PackProseLines, with the reason stated at the site (the repack loop terminates by reaching its final block, which a budget of zero would never present, and no section can render under a budget of one anyway). Every caller still hands in at least one — the head/footer arithmetic and the remaining < 2 guard are unchanged — so no payload changes shape; the invariant just stops living only in three call sites' subtraction. The #3493 doc block's sentence about the floor was updated to match. Re-ran the 66-arm shim against the rebuilt assembly: green.

[claude]

Reviewed the diff (WebhookAlertService.cs, AnalysisNotificationService.cs, Lite.Tests/SlackDetailsSizeTests.cs).

Correctness: Traced the block-budget arithmetic across BuildSlackPayload → AddSlackDetailBlocks → RenderSlackDetail → AddSlackProseSections → PackProseLines. The invariant that blockBudget passed into the prose packer is always >= 1 holds at every call site (head/footer are small fixed constants, and the remaining < 2 guard in AddSlackDetailBlocks keeps the body-truncation budget positive), so the new detail-omission/truncation logic doesn't overflow the 50-block cap and doesn't silently drop content without stating it. Left one non-blocking inline note about that invariant being enforced only by caller-side arithmetic rather than locally in PackProseLines.

Parity: PerformanceMonitor.Notifications is a shared library referenced by both Lite and Darling (and AnalysisNotificationService.BuildContext is called from both Lite/Services/CollectionBackgroundService.cs and Darling/PerformanceMonitor.Darling.Service/DarlingFindingAlertSender.cs), so this fix applies to both apps with no drift. No SQL changes in this PR.

Security: No new external input, file, network, or process surfaces; payload values go through anonymous-object → JsonSerializer (no raw string-concatenated JSON), so no injection concerns.

Tests: SlackDetailsSizeTests is unusually thorough — it pins byte-for-byte parity with the pre-fix rendering for shapes that fit, exercises the exact boundary (6 vs 7 distinct hashes / 49 vs 51 blocks), verifies the omission always names what was dropped (never silent), and checks the Teams payload stays comfortably under its 28 KB cap. Verified the math in the doc comments (head=2, footer=2, 10 fixed items=33 blocks, 2 per incident) against the test's InlineData and it's internally consistent.

No blocking issues found.

[claude]

LGTM — traced the Slack block-budget arithmetic (BuildSlackPayload → AddSlackDetailBlocks → RenderSlackDetail → AddSlackProseSections → PackProseLines) end to end, confirmed the invariants that keep it within the 50-block cap and never silently drop content, verified Lite/Darling parity (shared Notifications library, called from both apps), checked security surface (no new external input/injection risk), and spot-checked the new test suite's math against the implementation. One non-blocking robustness note left inline.

[claude]

Correctness: a fully-fitting page can get a false "0 more details did not fit" omission block.

The while loop can reach kept == details.Count (i.e. every detail ends up kept) via the body-shrink branch (line 1177) rather than only by hitting the loop's natural end. That happens whenever the unbounded render of all details exceeds blockBudget (so the fast path at line ~1149 is skipped), but shrinking a body detail (e.g. Diagnosis or Advice, which are body-type and processed first in FindingMessageFormatter.BuildContext) recovers enough room that everything — including every detail after it — ultimately fits under blockBudget - omissionCost.

In that case dropped = details.Count - kept is 0, but the code doesn't check that before line 1214-1219: it unconditionally appends a divider + "*Omitted from this message*\n0 more details did not fit Slack's 50-block message limit: {SlackOmissionPointer}" (empty heading list, since the loop at 1196 never executes). This wastes 2 blocks and shows the recipient a factually wrong statement — nothing was dropped, one body was merely truncated (and that truncation is already stated inline by AddSlackProseSections's own line-level omission).

Minimal repro: two details where detail 0 is a small fixed-size field detail and detail 1 is a body whose full render is much larger than the budget but whose shrunk render (via RenderSlackDetail(detail, bodyBlockBudget: remaining - 1)) fits in what's left — with nothing after it. kept reaches 2 (== details.Count) through the shrink path, yet the function still emits a spurious "0 more details did not fit" section.

None of the new tests in SlackDetailsSizeTests.cs hit this: the "byte-identical" arms only exercise the fast path (total <= blockBudget), and the boundary-body arm (ABodyDetailAtTheBoundary_TakesWhatRemains_AndStatesItsOwnOmission) deliberately adds trailing details after the shrunk body so dropped is always > 0 there.

Suggest guarding on dropped == 0 (return before building the omission text) in addition to the existing omissionCost == 0 check.

[erikdarlingdata]

Correct, and thank you — fixed in 350cdaf, and the defect was one step wider than the repro: the reserve was charged against the LAST detail too, so besides the false "0 more details" item after a last body shrink, a last SMALL detail that would have fit in the reclaimed two blocks was dropped and replaced by an omission item of the same cost (less told for the same blocks). Now the last detail is offered the whole remainder (remaining = blockBudget - spent - (last ? 0 : omissionCost)) and the function returns before building the item when dropped == 0. For every non-last detail the reserve stands, and a body shrunk into it fills the remainder exactly, so the next detail necessarily drops and the item is always earned. Two new arms pin both shapes at exactly 50 blocks with no omission item (ALastBodyDetailPastTheBudget_ShrinksIntoTheWholeRemainder_WithNoOmissionItem — your repro — and ALastSmallDetail_FitsInTheReclaimedReserve_AndIsKeptRatherThanAnnounced). Re-executed the 68-arm shim and the five-fact harness: the fitting shapes remain byte-identical, the over-budget shapes still land at 49.

[claude]

Minor/non-blocking: if blockBudget <= 0 (omissionCost becomes 0 via Math.Max(0, blockBudget)), this returns having appended nothing at all — every detail is silently dropped with no stated omission, which is exactly the "never silent" invariant this whole PR is built around. Currently unreachable in practice (head is 2 blocks, footer is 1-2, proseReserve is 0-1, so blockBudget is always comfortably positive today), so I wouldn't block on it, but it's worth a defensive note or an assertion so a future change to the head/footer composition can't silently regress this.

[erikdarlingdata]

Agreed it is unreachable (head 2, footer ≤ 2, reserve ≤ 1 → the caller hands in ≥ 45), and agreed the invariant deserves to be written down where it would break: 350cdaf adds the note at the omissionCost site — a budget of zero or less leaves no block to state anything in, so appending nothing is the only honest rendering of a budget that cannot exist, and a change to the head/footer composition is what the comment is for. Kept as a note rather than an assertion because the site is a webhook payload builder on the alert path, and throwing there would lose the delivery outright — the outcome this PR exists to prevent.

[claude]

Reviewed the Slack block-budget fix (WebhookAlertService.BuildSlackPayload / AddSlackDetailBlocks / AddSlackProseSections / RenderSlackDetail / SlackFieldText) plus the new SlackDetailsSizeTests.cs. The design and most of the edge-case handling (byte-identical fast path, block-not-count budgeting, per-text-object caps, field-cut arithmetic, Teams cap) is solid and well tested. One substantive bug found:

1. False "0 more details did not fit" omission (AddSlackDetailBlocks, WebhookAlertService.cs ~1164-1219). When the over-budget prefix loop reaches kept == details.Count via the body-shrink branch (a body detail like Diagnosis/Advice gets shrunk to fit rather than dropped, and everything after it then also fits), dropped is 0 but the code still unconditionally appends a divider + "Omitted from this message\n0 more details did not fit Slack's 50-block message limit: ..." section. This wastes 2 blocks and shows recipients a factually wrong statement on a page where nothing was actually dropped — the mirror-image of the bug this PR is fixing. None of the new tests exercise this path (the boundary-body test always has trailing details after the shrunk body, so dropped > 0 there). See inline comment for a concrete repro and suggested fix (guard on dropped == 0).

Also left a non-blocking inline note about a latent (currently unreachable) silent-full-drop path when blockBudget <= 0.

Remediated in 350cdaf: the omission reserve is no longer charged against the last detail and no item is emitted when dropped == 0; both shapes pinned. The zero-budget note is in place. Dismissed so the guard re-evaluates against the new head's review.

[claude]

Minor/non-blocking: value[..keep] (and the sibling truncation points — SlackHeading's heading[..SlackDetailHeadingLimit] at line 1284, the fragment cut at line 1072, and raw.Substring(start, take) at line 990) all cut on char (UTF-16 code unit) boundaries. If a query-text field lands a cut in the middle of a surrogate pair (astral-plane characters, e.g. some emoji, are plausible inside arbitrary SQL text/comments), the result carries a lone surrogate. JsonSerializer.Serialize here won't throw on that (unlike JsonEncodedText.Encode, per the comment above EscapeForJson), so it isn't a crash risk — worst case is a stray replacement glyph in the rendered card. Not worth blocking on given how narrow the trigger is, but might be worth a shared "trim to char boundary" helper if this pattern gets a fifth call site.

[erikdarlingdata]

Agreed on the mechanics and the severity (a stray glyph, never a throw — System.Text.Json's serializer replaces a lone surrogate rather than rejecting it). Not taking it in this PR: two of the four cut points are #3493's (the omission fragment and the hard-split continuation) and predate this change, so a shared trim-to-char-boundary helper is properly its own small PR touching both halves of the discipline at once rather than two of four sites here. I'll carry it in the lane report as a follow-up with your four line references.

[claude]

LGTM — reviewed the Slack block-budget algorithm (AddSlackDetailBlocks/AddSlackProseSections/PackProseLines/SlackFieldText) in WebhookAlertService.cs against the new SlackDetailsSizeTests.cs, hand-traced several of the trickier boundary cases (last-detail-gets-whole-remainder, last-small-item-reclaims-the-omission-reserve, prose-vs-details budget split), and confirmed the byte-identical-when-fitting claim against the old unbounded loop via the diff. No correctness issues found. This lives entirely in PerformanceMonitor.Notifications, a project shared by both Lite and Darling, so there's no Lite/Darling parity drift to flag. Left one non-blocking inline note about char-boundary truncation on a few of the new cut points (cosmetic risk only, not a crash).