Plan analyzer regexes and plan XML attributes are bounded; CSV export and copied rows write formula-looking text as text; SMTP without encryption warns by erikdarlingdata · Pull Request #5699 · erikdarlingdata/PerformanceMonitor

erikdarlingdata · 2026-10-12T16:20:33Z

Plan analyzer regexes get a match timeout, caller-supplied plan XML gets a size cap, CSV export and copied rows write formula-looking text as text, and an SMTP setup without encryption shows a warning.

What the behavior is now

Plan analyzer

Every regular expression in the plan-analysis project has a 250 ms match timeout, including the ones in the parameter card, the repro script builder and the query modification detector; each goes through PlanRegex.
Each plan also gets one time limit of 2 seconds of wall-clock time, counted from the start of its analysis. It covers all the work of the analysis, in regex calls and out of them: the regex calls check it, and so does each step of the CONVERT_IMPLICIT read and of the search for its closing parenthesis. When a match times out, or the limit has passed, that check is skipped and each statement gets one Info note, "Analysis Checks Skipped" (rule 40), that names the checks and says the plan was too large or complex to finish within the analysis time limit. The note shows wherever a statement's warnings show. An operator who disables rule 40 in the analyzer config gets no note (the record of skipped checks is still cleared, so it does not carry into the next statement).
A skipped check is a missing result and never a stated fact. The OPTIMIZE FOR UNKNOWN, MAXDOP 1 (serial plan), MAXDOP 2, NOT IN, cursor LOCAL and PROBE-only checks fall back to a plain text search over the statement or predicate text when the regex is skipped, so they give the same answer as the regex and are not named in the note. The checks with no text-search answer (the lookup-branch column check, the column-reference check, the function-on-column check, the cursor declaration) report nothing found, and the note names them. A predicate whose non-SARGable checks were cut short gets the warning worded as not fully checked, and is never reported as holding a column or a function on a column.
Match enumeration is lazy again, so a loop that exits early (the AND/OR split of a predicate, the column-reference loops) stops the pass.
The CONVERT_IMPLICIT check reads each conversion's arguments in place, between the parentheses, instead of copying them into a new string for every match.
The repro script header comment replaces control characters with a plain character loop instead of a regex.
Plan XML handed in by a caller (the analyze_plan_xml tools in Lite and Darling, and the plan viewers' paste, open and drop prompts) is refused when any one element has more than 1,000 attributes. The refusal names the limit and comes back before the XML is parsed. A plan just under the limit still analyzes. Caller plan XML that holds a DOCTYPE declaration is also refused, with a message saying showplan XML never has one (CallerPlanXml.cs:37-38).

CSV export and copy

Grid CSV exports, chart CSV exports, and Copy Row / Copy All Rows write a text cell that starts with =, +, -, @, a tab or a carriage return with a leading apostrophe, so a spreadsheet shows it as text. Numbers and dates are left alone, including negative numbers.
Copy Row, Copy All Rows and Ctrl+C on selected rows also turn every run of tabs and line breaks inside a cell into one space before that rule runs, so a cell stays one field and one line when it is pasted. The web dashboard's Copy row and Copy table already did that, and now apply the same apostrophe rule (a plain number such as -1,234.5 is left alone). CSV exports keep the line break inside a quoted field.
A CSV field is quoted when it holds the separator, a double quote, a line break, a comma, a semicolon or a tab, whatever separator the export uses; quotes inside it are doubled. The web dashboard's CSV download quotes the same set.
The apostrophe rule looks at the first character that is not white space or a control character: a text cell such as =SUM(1), a line break then @SUM(1), or a tab then -cmd gets the apostrophe in front of the whole text, in the desktop apps and in the web dashboard's Copy row, Copy table and CSV. A cell whose first character is a tab or a carriage return still gets it. A plain number after white space ( -5) is left alone.
Copy Cell copies a single cell exactly as it is, because it is often query text on its way into SSMS.
The Darling viewer and Lite share one rule (SpreadsheetText), and the Lite chart CSV keeps its UTC-offset column and the series name goes through the same rule.
The web dashboard's plain-number test accepts the same strings as SpreadsheetText.IsPlainNumber: an optional sign, a run of digits, . and , with at least one digit, and an optional exponent. A value such as -.5 is left alone, and -.e5 or -1e still gets the apostrophe.
Under deprecated/ only one Dashboard test assertion changes (below). The deprecated Dashboard calls the shared DataGridExport and its themes set DataGridClipboardBehavior.FixHeaderCopy, so its CSV export, Copy Cell, Copy Row, Copy All Rows and Ctrl+C follow the same rules as Lite and the Darling viewer. FixHeaderCopy now passes every text cell of a copied data row through SpreadsheetText.NeutralizeClipboardCell (tabs and line breaks become one space, and a cell that starts with a formula character is pasted as text; numbers and dates are left as they are), and it still replaces a StackPanel column header with its TextBlock text.

SMTP settings

The Darling service logs the same warning (Warning level, naming the SMTP host and port) at start and on every config reload when the SMTP settings it holds have SSL off and a username and password set. That covers an SMTP block in darling.json and a setup saved before the settings windows warned. Lite logs it at start for a setup it loads.
In Lite and the Darling viewer, Save and Send Test show a Yes/No warning when SSL is off and a login is set (a typed password, or a stored one on Save). Yes carries on, so an internal relay on port 25 still works. Lite's Validate button shows the same warning in its status line instead of "Settings look good", and the Darling viewer's Settings window Validate does the same (SettingsWindow.xaml.cs around :1584, through the same CleartextCredentialsWarning call, counting a stored password too). The rule is one shared predicate, EmailSendCore.CleartextCredentialsWarning.

Already on dev

Nothing was dropped. I checked 3efc0dbee..origin/dev for the plan analyzer, the export and clipboard helpers, and the email send core and SMTP settings windows. Dev has no match timeout on the plan analyzer's regexes, no attribute limit on caller plan XML, no formula handling in exports, and no warning for SMTP without encryption, so every part stays.

Merge notes

Lite/Helpers/ContextMenuHelper.cs: dev moved the chart CSV line into ChartCsvLine (with the zone and UTC offset). I kept dev's method and put the series-name rule inside it.
Darling.Viewer/SettingsWindow.xaml.cs: dev now keeps the stored SMTP password on _loadedNotification; the warning reads it from there.
Two of dev's new test rules caught the new tests, so they now follow them: the plan-limit timing test is in the timing collection, the spreadsheet test uses the shared STA thread, and the SMTP test class carries the Reads / Darling trait.
No store migrations are added.

Test plan

Changelog

SECTION: Changed
ENTRY:

Plan analysis and exports are bounded, spreadsheet-safe and warn on unencrypted mail ([Plan analyzer regexes and plan XML attributes are bounded; CSV export and copied rows write formula-looking text as text; SMTP without encryption warns #5699]) - The plan analyzer's regular expressions now time out after 250 ms and share a 2 second wall-clock limit per plan (a check that is cut short is named in a rule 40 note on its statement unless rule 40 is disabled, and is never reported as a finding), and plan XML passed to the analyzer is refused when one element has more than 1,000 attributes or holds a DOCTYPE declaration. CSV exports and copied rows write a text cell that starts with =, +, - or @ as text (copied rows first turn tabs and line breaks inside a cell into a space, and the rule looks past leading white space; the web dashboard's Copy row and Copy table do the same; CSV fields holding a comma, semicolon, tab, quote or line break are quoted), while Copy Cell copies exactly one cell as it is (the deprecated Dashboard gets the same export and copy rules through the shared helpers). The SMTP settings in Lite and the Darling viewer warn when a login would be sent without encryption (at Save and Send Test, and on Lite's Validate), and the Darling service and Lite log the same warning at start (the service also on config reload).
REF:
[Plan analyzer regexes and plan XML attributes are bounded; CSV export and copied rows write formula-looking text as text; SMTP without encryption warns #5699]: Plan analyzer regexes and plan XML attributes are bounded; CSV export and copied rows write formula-looking text as text; SMTP without encryption warns #5699

… and copied rows write formula-looking text as text; SMTP without encryption warns The plan analyzer's regular expressions have a 250 ms match timeout, and plan XML passed in by a caller is refused when one element has too many attributes. CSV exports and copied rows in Lite and the Darling viewer write cells that start with a formula character as text; Copy Cell copies one cell exactly as it is. The SMTP settings in Lite and the Darling viewer warn at Save and Send Test (and on Lite's Validate) when a login would be sent without encryption.

…re the formula-text rule; CSV quotes commas, semicolons and tabs; web Copy row and Copy table write formula-looking text as text

…they skipped; every plan-analysis regex has a timeout; SMTP setups without encryption are named in the service and Lite logs

claude · 2026-10-12T18:21:46Z

+
+        stmt.PlanWarnings.Add(new PlanWarning
+        {
+            RuleNumber = 40,


Rule 40 is the only stamped rule with no AnalyzerConfig guard — and the census that exists to catch exactly that is vacuous for it.

NoteSkippedChecks(PlanStatement stmt) takes no AnalyzerConfig, so there is no IsRuleDisabled(40) anywhere. Every other stamped rule has one — grep -o "IsRuleDisabled([0-9]*)" PlanAnalyzer.cs returns 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 22 23 24 26 27 28 29 30 32 33 34 35 36 37 38 39, and 40 is absent.

That breaks the contract PlanSync4535RuleNumberCensusTests states it proves:

(c) With every rule number the code census in (a) found stamped somewhere disabled, running the fixtures yields zero analyzer findings … Proves each rule's guard, not just its stamp, reaches AnalyzerConfig.

DisablingEveryStampedRule_YieldsZeroAnalyzerFindings_OnTheFixtures walks new PlanWarning constructions, reads RuleNumber = 40 off this initializer via RuleNumberAt, and puts 40 in RulesConfig.Disabled. It then passes — not because the guard works, but because its two fixtures (a 1-statement Filter/Table Scan plan and a direct cursor/truncated-text plan) never spend the 2 s budget, so TakeSkipped() is empty and the note never fires. The pin cannot fail on the missing guard. That is the .github/REVIEW-TRAPS.md shape verbatim:

A pin that encodes the lie: ACappedResult_WithholdsTheUnsatisfiedSummary_… handed the builder two facets at limit = 2 and asserted truncated — the >= limit inference as its own proof (#3699). Ask what a pin would fail ON.

Failure scenario: an operator sets rules.disabled: [40] to silence the note on a store full of large plans. Every statement of every budget-spending plan still carries the Info "Analysis Checks Skipped" warning, in the viewer and in every MCP plan payload, with nothing in config able to turn it off.

Fix is mechanical — thread cfg through and gate it, so the census stops being vacuous:

MarkLegacyWarnings(stmt); NoteSkippedChecks(stmt, cfg);

…with if (cfg.IsRuleDisabled(40) || skipped.Count == 0) return;. Worth adding a fixture to (c) (or a direct test) that actually makes the note fire under TimeSpan.Zero, so the disable list is proved rather than assumed — PlanRegexTimeoutTests.Analyze_WithAnEmptyBudget_NamesTheChecksOnEveryStatement already builds exactly that plan.

claude · 2026-10-12T18:22:00Z

            {
                var text = MaskCommentsAndLiterals(stmt.StatementText); // #4524
-                var hasMaxdop1InText = Regex.IsMatch(text, @"MAXDOP\s+1\b", RegexOptions.IgnoreCase);
+                var hasMaxdop1InText = PlanRegex.IsMatch(text, @"MAXDOP\s+1\b", RegexOptions.IgnoreCase, "MAXDOP 1 hint in the statement text");


This is the one call site where the timeout answer DROPS a warning rather than keeping it.

PlanRegex.cs:19 states the rule for the whole helper:

A timeout or a skipped call never throws and never quietly changes the answer … and each call site picks the answer a timeout gives, on the side that keeps a warning rather than drops it.

Here onTimeout is left at its false default, and false is the drop side: lines 365–386 add a Warning when hasMaxdop1InText, an Info when isTruncated, and nothing at all otherwise ("else: not truncated, no MAXDOP 1 in text — server/db/RG setting, suppress entirely"). Compare the sibling MAXDOP 2 site at :2345, where the author annotated onTimeout: false // no hint found keeps the warning — there false keeps it, because the test is if (!hasMaxdop2Hint). The polarity is inverted here and the default was taken as-is.

Failure scenario: a plan with one hostile predicate (the case this PR exists for) spends the 2 s budget during statement 1. Statement 7 has NonParallelPlanReason="MaxDOPSetToOne", untruncated text containing OPTION (MAXDOP 1). CanRun returns false, hasMaxdop1InText is false, isTruncated is false → the rule-3 "Query running serially" warning is not emitted at all. Before this PR it was. The statement does get the rule-40 note naming "MAXDOP 1 hint in the statement text", so it is not silent — but the warning itself is gone, which is the outcome the helper's contract says no site should pick.

onTimeout: true here reproduces the pre-PR answer for an untruncated statement (the Warning-severity rule 3), which is the conservative direction. Either way the direction deserves the same inline annotation the other sites carry, since the default is wrong for this one.

claude · 2026-10-12T18:22:16Z

+        }

-        return null;
+        return incomplete ? UnverifiedNonSargableReason : null;


A spent budget turns this into a non-null reason for EVERY rowstore scan, which suppresses rule 11 across the whole plan and downgrades a possibly-Critical finding to a Warning.

Once the budget is spent, nothing in this method can run: TryIsMatch(CaseInPredicateRegex, …) returns false → incomplete = true, conversions/isnullRun/funcRun all yield nothing with Incomplete true, TryIsMatch(LeadingWildcardLikeRegex, …) returns false → this line returns UnverifiedNonSargableReason for every predicate it is handed, benign or not.

GetNonSargableReason is read twice at the node level (:1114), and rule 11 is gated on it being null (:1147):

if (!cfg.IsRuleDisabled(11) && nonSargableReason == null && IsRowstoreScan(node) && !string.IsNullOrEmpty(node.Predicate) && …

So for a plan whose budget is spent:

every rowstore scan with a predicate gains a rule-12 "Non-SARGable Predicate" / PlanWarningSeverity.Warning saying it "may be non-SARGable", including [t].[id]=(1);

every one of those scans loses its rule-11 "Scan with residual predicate" finding, which carries BuildScanImpactDetails and is elevated to PlanWarningSeverity.Critical at CostPct >= 90, ElapsedPct >= 90, or on significant I/O waits at CostPct >= 50 (:1154–1175), plus the dynamic-cursor hint.

Net effect on a big plan: a Critical scan finding with impact numbers is replaced by a generic Warning with no numbers, on every scan node, because one unrelated predicate elsewhere in the plan exhausted the budget. The PR body says only "a predicate whose non-SARGable checks were cut short gets a warning worded as not fully checked, instead of none" — it does not mention that the same value also suppresses rule 11, which is the larger behaviour change.

Two options that keep the stated intent without the collateral: have the unverified reason not satisfy rule 11's suppression (rule 11 is suppressed to avoid double-reporting a known non-SARGable predicate, which this is not), or return the unverified reason only when a check was cut short on a predicate that had something to find. Either way this deserves a line in the body and the changelog, since it is what an operator will actually see.

claude · 2026-10-12T18:22:28Z

+/// (<see cref="PlanBudgetMilliseconds"/>), counted as the time spent inside regex calls. Once it is spent,
+/// every further call is skipped.</description></item>
+/// </list>
+/// A timeout or a skipped call never throws and never quietly changes the answer: the check is recorded


Non-blocking, but this sentence does not hold for three of the five files the PR routes through the helper.

A timeout or a skipped call never throws and never quietly changes the answer: the check is recorded (TakeSkipped), and the analyzer adds one note to the statement that names the checks it skipped.

The record and the note both live on the thread-static Budget, and the only BeginPlan scope in the product is PlanAnalyzer.Analyze (PlanAnalyzer.cs:120). PlanAnalysisPipeline.Run (PlanAnalysisPipeline.cs:60) wraps only the analyzer — ParameterCard.Annotations, ReproScriptBuilder and QueryModificationDetector are called by the UI and the MCP formatters outside it. On those paths t_budget is null, so RecordTimeout is a no-op, TakeSkipped() returns Array.Empty<string>(), and no rule-40 note is ever produced. A 250 ms timeout in ParameterCard.FindUnresolvedVariables or ReproScriptBuilder.IsValidDataType (onTimeout: false → the parameter is dropped from the generated script) changes the answer with nothing recorded anywhere.

ABudget_IsEndedWithItsScope_AndANestedScopeSharesTheOuterOne pins this on purpose ("Outside any plan there is no budget, and nothing is recorded"), so the behaviour is intentional — it is the class summary's unqualified wording that over-promises. Either scope those three entry points too, or qualify the sentence with "inside BeginPlan", the way the budget bullet above it already is.

claude · 2026-10-12T18:22:41Z

+   ("-5", "+3", "1e-3", "-1,234.5"); any other string is neutralised. */
 const FORMULA_LEAD = /^[=+\-@\t\r]/;
-const PLAIN_NUMBER = /^[+-]?\d+(\.\d+)?([eE][+-]?\d+)?$/;
+const PLAIN_NUMBER = /^[+-]?\d[\d,]*(\.\d+)?([eE][+-]?\d+)?$/;


Non-blocking: this is now a second hand-written copy of one rule, and the two copies already disagree.

SpreadsheetText.IsPlainNumber (PerformanceMonitor.Ui/SpreadsheetText.cs:107) walks [0-9.,] freely after the sign, then an optional exponent. This regex requires the separators before the decimal point and allows exactly one (\.\d+). They part company on, e.g., -1.2.3: the C# reader calls it a plain number and leaves it alone, the web reader neutralises it to '-1.2.3. Same for -1,2.3,4.

Both answers are safe (neither is a formula), so nothing is broken today — but the trap this shape belongs to is in .github/REVIEW-TRAPS.md:

Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy.

Here the copies are not even pinned equal: SpreadsheetTextTests.PlainNumbers_StayNumbers and GridToolsBehaviourTests.CopyRowAndCopyTable_…'s cells list each assert their own side, with no shared table of inputs. The C# half of this PR did the right thing in the other direction (ContextMenuHelper.CsvEscape and ViewerServerTab.CsvEscape are now one-line aliases of DataGridExport.CsvEscape) — worth the same treatment here to the extent a JS/C# boundary allows: one shared list of inputs and expected outputs, asserted by both suites, so the next edit to either rule fails the other's pin.

…acters, in the desktop apps and the web grid; the web CSV quotes semicolons and tabs

…ded as missing and decided by a text search where one can; regex ratchet covers fully qualified calls

…ext-search tests run in the timing collection

claude · 2026-10-12T18:49:55Z

-            || value.Contains('"')
-            || value.Contains('\n')
-            || value.Contains('\r'))
+            || value.AsSpan().IndexOfAny(",;\t\"\r\n".AsSpan()) >= 0)


CsvEscape now quotes on ,, ; and \t whatever the separator is — and the deprecated Dashboard is a caller of this shared method that the change did not re-read.

deprecated/Dashboard.Tests/DataGridExportTests.cs:103-107 pins the old rule by name:

[Fact] public void CsvEscape_QuotesWhenValueContainsSeparator() { // With a semicolon separator a comma is NOT special, but a semicolon is. Assert.Equal("a,b", DataGridExport.CsvEscape("a,b", ";")); Assert.Equal("\"a;b\"", DataGridExport.CsvEscape("a;b", ";")); }

With this line the first assertion gets "a,b" (quoted) and the test fails. Lite.Tests/SpreadsheetTextTests.cs:399 — added by this PR — asserts the exact opposite for the same two inputs, so the two pins now contradict each other.

That suite runs on this PR. .github/workflows/build.yml:632 puts PerformanceMonitor.Ui/**/!(*.md) in the core filter, and both Build Dashboard.Tests (line 928) and Run Dashboard tests (line 978) are gated on steps.filter.outputs.core == 'true'; the run is dotnet run --project deprecated/Dashboard.Tests/... --no-build with no -class- filter, so every test in it executes. The test plan lists Lite.Tests and Darling.Tests and excludes only Installer.Tests — Dashboard.Tests is the one suite that was neither run nor updated.

Related, on the body's "Nothing under deprecated/ is changed, so the deprecated Dashboard's CSV export is as it was": no file under deprecated/ changed, but the behaviour did. The Dashboard calls straight into this class — deprecated/Dashboard/WaitDrillDownWindow.xaml.cs:378-385, CollectorScheduleWindow.xaml.cs:328-335, ManageServersWindow.xaml.cs:404-411, QueryExecutionHistoryWindow.xaml.cs:355-362, CollectionLogWindow.xaml.cs:194-201 all use DataGridExport.CopyCell / CopyRow / CopyAllRows / ExportToCsv — and deprecated/Dashboard/Themes/{Light,Dark,CoolBreeze}Theme.xaml wire DataGridClipboardBehavior.FixHeaderCopy, which this PR changed from header-only to "also neutralize every data row". So the Dashboard's CSV quoting, Copy Cell, Copy Row and Ctrl+C all move with this change.

Either update the deprecated pin to the new rule (and drop that sentence from the body), or keep the deprecated caller on its old contract.

claude · 2026-10-12T18:50:03Z

+
+        stmt.PlanWarnings.Add(new PlanWarning
+        {
+            RuleNumber = 40,


Non-blocking: rule 40 is the only stamped rule with no IsRuleDisabled guard. NoteSkippedChecks takes no AnalyzerConfig and adds the note unconditionally, so an operator who disables rule 40 still gets it — unlike every other number in the file.

PlanSync4535RuleNumberCensusTests.DisablingEveryStampedRule_YieldsZeroAnalyzerFindings_OnTheFixtures collects rule numbers from this source walk, so 40 goes into its disable list, but its two fixtures never exhaust the budget and never time out, so no rule-40 note is produced and the census passes without proving the guard. The census only pins "stamp without guard" in the other direction (rules 7 and 29 are carved out as guard-without-stamp).

Passing cfg down and returning early on cfg.IsRuleDisabled(40) would close it and make the (c) arm mean what it says for this rule too.

claude

Verdict: CHANGES REQUESTED

Claims

VERIFIED -- every regex in the project carries a 250 ms timeout and goes through PlanRegex -- 17 [GeneratedRegex] with matchTimeoutMilliseconds: PlanRegex.TimeoutMilliseconds, 2 compiled fields with PlanRegex.DefaultTimeout (QueryModificationDetector.cs:102,127), no static Regex.* call left; pinned by PlanRegexTimeoutTests.PlanAnalysisProject_EveryRegexCarriesTheTimeout / _RunsNoRegexOutsideTheHelpers
VERIFIED -- one 2 s per-plan budget, checked by the helpers and the two non-regex loops -- PlanRegex.cs:49,128, PlanAnalyzer.cs:124,1989,3196
VERIFIED -- one Info note per statement, rule 40 -- PlanAnalyzer.cs:155-168, TakeSkipped per statement at :142; pinned by Analyze_WithAnEmptyBudget_NamesTheSkippedChecks
VERIFIED -- the six named checks fall back to a text search, unrecorded -- PlanAnalyzer.cs:1727,1740,2664,2674,2697,2707 via the record: false overloads (PlanRegex.cs:207,238); PlanTextSearch.cs:21-60; pinned by PlanTextSearchTests
VERIFIED -- a cut-short check never states a fact -- ReadsAnotherInput false (:2027) is rule 15's suppressing side (:1370); non-SARGable returns UnverifiedNonSargableReason (:1972) and every undecided branch returns false first (:2911, :3036, :3064, :3091); pinned by Analyze_WhereverTheTimeLimitFalls_NeverReportsWhatTheTextLacks
VERIFIED -- lazy enumeration, and CONVERT_IMPLICIT args read in place -- PlanRegex.cs:289-310, PlanAnalyzer.cs:3113 feeding the span overloads (:3034, PlanRegex.cs:244)
VERIFIED -- the repro comment filter's regex-to-char-loop swap is behaviour-neutral -- char.IsControl is exactly UnicodeCategory.Control, the class the pattern matched
VERIFIED -- caller plan XML refused above 1,000 attributes per element, before any parse -- CallerPlanXml.cs:25,72, wired at Lite/Mcp/McpPlanTools.cs:256, DarlingMcpPlanTools.cs:193 and six viewer reads (StandalonePlanViewerController.cs:305,337,481,515,530, PlanViewerControl.Interaction.cs:230, Lite/Controls/ServerTab.xaml.cs:499); pinned by CallerPlanXmlTests.EveryViewerReadOfCallerText_IsGated and StoredPlanPaths_AreNotGated
VERIFIED -- exports and row copies neutralize formula text, numbers untouched, Copy Cell raw -- SpreadsheetText.cs:38-63,127, DataGridExport.cs:183,219,229, ContextMenuHelper.cs:67,198, ViewerServerTab.ChartContextMenu.cs:105; pinned by SpreadsheetTextTests
VERIFIED -- grid-tools.js:30-38 is the same language as the C# scanner with the same leading-skip class; pinned by the harness tsvFormulas list
VERIFIED -- the SMTP warning at service start and reload (DarlingWorker.cs:3148,6292), Lite start (App.xaml.cs:570), Save/Send/Validate in both windows (Lite/Windows/SettingsWindow.xaml.cs:277,1194,1245; Darling/.../SettingsWindow.xaml.cs:1584,1635,1939), one predicate (EmailSendCore.cs:453)
UNVERIFIED -- the attribute-bomb timings (CallerPlanXml.cs:16-23) -- a Release-build measurement
UNVERIFIED -- the Test plan counts (0 warnings, 495, 1,517, 1,010) -- needs a build and a test host
UNVERIFIED -- "Nothing was dropped. I checked 3efc0dbee..origin/dev" and the merge notes -- a cross-branch diff this pass does not run
REFUTED -- "Nothing under deprecated/ is changed, so the deprecated Dashboard's CSV export is as it was." No deprecated file changed, but the behaviour did and its suite breaks. Five deprecated/Dashboard/*Window.xaml.cs call DataGridExport.CopyCell/CopyRow/CopyAllRows/ExportToCsv, and three deprecated/Dashboard/Themes/*.xaml wire DataGridClipboardBehavior.FixHeaderCopy, so the widened CsvEscape (DataGridExport.cs:251), the raw Copy Cell (:183) and the per-data-row neutralizing (DataGridClipboardBehavior.cs:60-79) all reach it. deprecated/Dashboard.Tests/DataGridExportTests.cs:106 asserts CsvEscape("a,b", ";") == "a,b" and now fails, while Lite.Tests/SpreadsheetTextTests.cs:399 added here asserts the opposite. build.yml:632 puts PerformanceMonitor.Ui in core and :928/:978 gate Build/Run Dashboard tests on core == 'true' with no class filter, so CI runs it here

Riskiest lines

PerformanceMonitor.Ui/DataGridExport.cs:251 -- quoting widened to comma, semicolon, tab, quote and line breaks whatever the separator, in a helper three apps read -- pinned by SpreadsheetTextTests.CsvEscape_QuotesTheSeparatorQuoteBreakAndTheCommonDelimiters_WhateverTheSeparatorIs, contradicted by deprecated/Dashboard.Tests/DataGridExportTests.CsvEscape_QuotesWhenValueContainsSeparator
PerformanceMonitor.Ui/DataGridClipboardBehavior.cs:60-79 -- FixHeaderCopy lost its header-only early return and now rewrites every string cell of every data row of every DataGrid in all three apps -- pinned by SpreadsheetTextTests.KeyboardCopy_*, nothing covers the Dashboard's grids
PerformanceMonitor.PlanAnalysis/PlanAnalyzer.cs:1972 -- a non-null reason on a cut-short pass is also what rule 11's guard reads as non-SARGable (:1153), so a timeout silently suppresses rule 11 -- the rule-12 wording is pinned, the rule-11 interaction is UNPINNED

Traps near this diff

"Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" -- does not apply -- both halves landed: McpPlanTools.cs:256 with DarlingMcpPlanTools.cs:193, three SMTP handlers in both windows, ContextMenuHelper.cs:198 with ViewerServerTab.ChartContextMenu.cs:105; the one unshared paste path (Lite/Controls/ServerTab.xaml.cs:499) has no viewer counterpart
"Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." -- does not apply -- the diff aliases instead (ViewerServerTab.ChartContextMenu.cs:126, ContextMenuHelper.cs:159-161); ContextMenuHelper.GetCellValue:33 is still a copy of DataGridExport.ReadCell:186, pre-existing and untouched
"InternalsVisibleTo on PerformanceMonitor.Analysis names Lite.Tests only -- a helper Darling.Tests must call has to be public (#3688)" -- does not apply -- PerformanceMonitor.PlanAnalysis.csproj:31,35 and PerformanceMonitor.Ui.csproj:35,37 name both suites
"A test that reads the clock twice measures the CI scheduler (#3676). One anchor owns both ends of an interval." -- does not apply -- every new clocked class is in the serialized timing collection (CallerPlanXmlTests.cs:37, PlanRegexTimeoutTests.cs:466, PlanTextSearchTests.cs:20, CallerPlanXmlLiteTests), which TimingTestCensusTests enforces, and the tight assertions use a thread-static fake clock (PlanRegex.cs:107)
"A pin that encodes the lie ... Ask what a pin would fail ON. (#3699)" -- does not apply -- CallerPlanXmlTests.cs:62 fails if the constant drops toward real plans, :199 if the counter over-counts a real plan, PlanRegexTimeoutTests.cs:474 if the slow input stops being slow
"A text cut at a UTF-16 index can land between the halves of a surrogate pair (#3625)." -- does not apply -- nothing truncates; SpreadsheetText.cs:62 only prepends and :95-110 replaces whole runs of tab, CR and LF, all BMP

Divergence (blind read vs PR body)

The scope claim is the finding. "Nothing under deprecated/ is changed, so the deprecated Dashboard's CSV export is as it was" reads as a claim about behaviour and is false: the Dashboard calls DataGridExport directly and consumes FixHeaderCopy, so its CSV quoting, Copy Cell, Copy Row and Ctrl+C all move, and deprecated/Dashboard.Tests/DataGridExportTests.cs:106 fails. The test plan shows the blind spot: it names Lite.Tests and Darling.Tests and excludes only Installer.Tests, so the one suite that breaks is the one nobody ran, and core routes it into CI anyway.
CallerPlanXml.RefusalReason also refuses a DOCTYPE outright (CallerPlanXml.cs:37-38), pinned by ADoctype_IsRefused. Neither the body nor the changelog mentions entity expansion -- a second gate on caller input the account omits.
The body credits Validate only to Lite; the Darling viewer does the same (Darling/.../SettingsWindow.xaml.cs:1584) and the new SmtpCleartextWarningTests.EachSettingsWindowHandler_ChecksTheWarning asserts both. The body understates its own parity.

Not divergence, but worth saying: the "never a stated fact" rule also holds where the safe direction is true rather than false -- QueryModificationDetector.HasExecKeyword returns statements.Incomplete, and ReproScriptBuilder.IsValidParameterName / IsValidDataType / IsSafeLiteral pass onTimeout: false. Inverting either would have been a security bug; both are right.

Reviewer checklist

Run dotnet run --project deprecated/Dashboard.Tests/Dashboard.Tests.csproj -c Release and confirm CsvEscape_QuotesWhenValueContainsSeparator is the only failure, then decide whether the deprecated pin moves or the new rule carves that caller out.
Re-measure on a Release build: one element at 1,000 attributes still sub-second, and a 16M-character plan at 1,000 attributes per element still about half a second.
Open a real multi-megabyte .sqlplan in both viewers: no false refusal, and a refused file leaves the empty sub-tab reusable.
Paste a Copy Row into Excel and into SSMS, and open a grid CSV export in Excel with the separator set to a semicolon.
With SSL off and a login set, start the Darling service: one Warning naming host and port at start, one after a reload, no password material in the log.

…t expects the quoted semicolon field

claude

Verdict: LGTM

Claims

VERIFIED — "An operator who disables rule 40 in the analyzer config gets no note (the record of skipped checks is still cleared, so it does not carry into the next statement)" — PlanAnalyzer.cs:155-162: TakeSkipped() drains the thread-local budget unconditionally before the cfg.IsRuleDisabled(40) check short-circuits the warning, matching the inline comment "The record is drained even when rule 40 is disabled, so it cannot carry into the next statement."
VERIFIED — "PlanRegexTimeoutTests has a new test (an empty budget with rule 40 disabled gives no note, with it enabled gives the note)" — Analyze_WithAnEmptyBudget_AddsNoSkippedChecksNote_WhenRule40IsDisabled at PlanRegexTimeoutTests.cs:306-323, which calls PlanAnalyzer.Analyze directly with AnalyzerConfig { Rules = new RulesConfig { Disabled = [40] } } (bypassing the test helper that hardcodes cfg = null), asserting no SkippedChecksWarningType/RuleNumber == 40 warning, then re-running the same statement through the default config and asserting the note IS present.
VERIFIED — "GridFilterBehaviourTests expects "red; hot" quoted in the comma CSV" — GridFilterBehaviourTests.cs:126 now asserts \"red; hot\" / \"blue; hot\"; this matches the unchanged production rule in grid-tools.js:47 (csvField), which quotes any field matching /[",;\t\r\n]/ — the Tags values contain ;, so this is a test correction to match already-correct shipped behavior, not a behavior change (grid-tools.js is not in this diff).
UNVERIFIED — the quoted test-run counts in the "Rule 40 and CSV quoting follow-up" bullet (702/0 failed, 192/0 failed, etc.) — requires actually running the test suites, not checkable from a read-only tree.
UNVERIFIED — "deprecated/Dashboard.Tests ... 1 failed, DataGridExportTests.CsvEscape_QuotesWhenValueContainsSeparator ... being updated separately" — same; a live test run, not checkable here, and that file is outside this diff's range regardless.

Riskiest lines

PerformanceMonitor.PlanAnalysis/PlanAnalyzer.cs:159 — the disabled-rule-40 short-circuit sits AFTER the unconditional PlanRegex.TakeSkipped() drain; if the order were reversed, a disabled rule 40 would leak the prior statement's skip list into the next one — pinned by Analyze_WithAnEmptyBudget_AddsNoSkippedChecksNote_WhenRule40IsDisabled (PlanRegexTimeoutTests.cs:306).
PerformanceMonitor.PlanAnalysis/PlanAnalyzer.cs:142 — NoteSkippedChecks(stmt, cfg) now threads cfg through instead of the parameterless call — pinned by the same test, which is the only one in the suite exercising a non-default AnalyzerConfig through the 4-arg Analyze overload.
Darling/Darling.Tests/GridFilterBehaviourTests.cs:126 — the expected CSV string requires the quotes to land on exactly the Tags field and nowhere else — pinned by CopyAndCsv_CarryTheFilteredRows_InSortOrder_WithEveryColumn itself, cross-checked against the unchanged csvField regex in grid-tools.js:47-51.

Traps near this diff

"A pin that encodes the lie: ... the >= limit inference as its own proof" (Tests and censuses) — does not apply — the GridFilterBehaviourTests change corrects an assertion to match the unchanged, independently-defined csvField quoting regex (grid-tools.js:47), not a self-proving inference drawn from the test's own setup.
"A test that constructs a ... fixture is testing a shape no live run produces" (Tests and censuses, #3709) — does not apply — the new rule-40 test exercises one real cursor statement against the real AnalyzerConfig.IsRuleDisabled gate used by every other rule in PlanAnalyzer.cs, not a synthetic multi-anomaly fixture.
"Lite and Darling ship in parity ... a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding" (Repository mechanics, #3594) — does not apply — PerformanceMonitor.PlanAnalysis is a shared project consumed by both SKUs, so the cfg threading fixes both at once, and the CSV grid (panels.js/grid-tools.js) exists only in Darling's web dashboard, so there is no Lite counterpart to update.

Divergence (blind read vs PR body)

None — the body's "Rule 40 and CSV quoting follow-up" bullet describes exactly these two commits, and the diff implements exactly what it claims: the disabled-rule-40 note suppression with the drain-regardless behavior, and the corrected CSV quoting expectation.

Reviewer checklist

Run Darling.Tests (PlanRegexTimeoutTests, GridFilterBehaviourTests) and Lite.Tests locally/in CI to confirm the reported pass counts (702/0 failed, 192/0 failed) and that GridFilterBehaviourTests actually executes against Node (it skips when Node isn't installed).

…ator, as the shared export now writes it

Claude posting for Erik Darling. Answered in later commits; the bot's LGTM at 1c22f8f covers them.

erikdarlingdata added 3 commits October 12, 2026 12:20

Copied rows turn tabs and line breaks inside a cell into a space befo…

3805706

…re the formula-text rule; CSV quotes commas, semicolons and tabs; web Copy row and Copy table write formula-looking text as text

Plan regex checks share one time budget per plan and name the checks …

aff506d

…they skipped; every plan-analysis regex has a timeout; SMTP setups without encryption are named in the service and Lite logs

erikdarlingdata marked this pull request as ready for review October 12, 2026 17:57

The SMTP startup warning class is named DarlingSmtpWarning

e38b62f

claude Bot reviewed Oct 12, 2026

View reviewed changes

erikdarlingdata added 3 commits October 12, 2026 14:25

The formula-text rule looks past leading white space and control char…

3d0ecd6

…acters, in the desktop apps and the web grid; the web CSV quotes semicolons and tabs

Plan analysis has one wall-clock time limit; a skipped check is recor…

2d1415b

…ded as missing and decided by a text search where one can; regex ratchet covers fully qualified calls

The web grid accepts the plain numbers the desktop apps accept; the t…

78d181d

…ext-search tests run in the timing collection

claude Bot reviewed Oct 12, 2026

View reviewed changes

claude Bot previously requested changes Oct 12, 2026

View reviewed changes

Rule 40 respects the disabled-rules config; the filtered-rows CSV tes…

1c22f8f

…t expects the quoted semicolon field

claude Bot reviewed Oct 12, 2026

View reviewed changes

The Dashboard CSV test expects a comma quoted under a semicolon separ…

9170e20

…ator, as the shared export now writes it

erikdarlingdata merged commit a7a0fb9 into dev Oct 12, 2026
27 of 28 checks passed

erikdarlingdata deleted the fix/plan-analyzer-and-export-limits branch October 12, 2026 20:39

Navigation Menu

Repository navigation

Plan analyzer regexes and plan XML attributes are bounded; CSV export and copied rows write formula-looking text as text; SMTP without encryption warns - #5699

Plan analyzer regexes and plan XML attributes are bounded; CSV export and copied rows write formula-looking text as text; SMTP without encryption warns#5699
erikdarlingdata merged 9 commits into
devfrom
fix/plan-analyzer-and-export-limits

erikdarlingdata commented Oct 12, 2026 •

edited

Loading

Uh oh!

claude Bot Oct 12, 2026

Uh oh!

claude Bot Oct 12, 2026

Uh oh!

claude Bot Oct 12, 2026

Uh oh!

claude Bot Oct 12, 2026

Uh oh!

claude Bot Oct 12, 2026

Uh oh!

claude Bot Oct 12, 2026

Uh oh!

claude Bot Oct 12, 2026

Uh oh!

claude Bot left a comment

Uh oh!

claude Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

erikdarlingdata commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What the behavior is now

Already on dev

Merge notes

Test plan

Changelog

Uh oh!

claude Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Verdict: CHANGES REQUESTED

Claims

Riskiest lines

Traps near this diff

Divergence (blind read vs PR body)

Reviewer checklist

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Verdict: LGTM

Claims

Riskiest lines

Traps near this diff

Divergence (blind read vs PR body)

Reviewer checklist

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

erikdarlingdata commented Oct 12, 2026 •

edited

Loading