#2190: count <summary> openings per doc run, not closing-tag pairs

CHANGELOG.md

@@ -30,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **The doc-comment hygiene pin now catches a stacked summary whose first block is never closed** ([#2190]) - `NoMemberCarriesTwoStackedSummaryBlocks` keyed off a `</summary>` immediately followed by a reopening, so it only ever saw a stacked pair when the FIRST block was closed, and two instances were sitting on dev unseen: a duplicated opening tag on `ApplyProcessEnvironment` in the Darling service, and a doc block that #1912's restructure split in `QueryStoreSliceRepairService`, stranding its unclosed head on top of `PromoteRewrittenFileAsync`. The rule now counts `<summary>` OPENINGS inside each contiguous run of `///` lines - a run documents exactly one member, so two openings means two summaries whether or not either is closed, and whether they are written single-line or spread over many. That mixed form is the one a closing-tag matcher cannot see at all, and it is what defeated the first attempt at this fix. Both live instances are repaired: the duplicate tag is deleted, and the stranded head is deleted rather than moved back, because the same restructure had already re-documented `FlushExternalFileCacheAsync` in place with a superset of that one sentence, so moving it would have recreated the exact duplicate this rule exists to forbid. The detector now carries its own self-test as well, pinning the five stacked shapes it must catch and the four legitimate ones it must leave alone, since this was a blind spot in the DETECTOR rather than in anyone's reading of the tree.
 - **The store's compressed plan format is now documented** ([#2171], reported by @argpna) - execution-plan XML has been gzip-compressed in `query_plan_dim.query_plan_gz` since 3.4.0, with `query_plan_xml` nullable, and the release notes never said so. A consumer reading the store directly over SQL therefore got nothing back for anything collected by a current build, with no clue why. The store section of the Darling README now states the format (gzip, magic `1f 8b`), that `query_plan_xml IS NULL` means read the compressed column rather than "no plan", and the three practical ways to get XML back - ask the product via `get_plan_xml`, decompress client-side, or ship a UDF into your own store if your tooling is SQL-only. In-product paths were never affected; this was a contract change for direct SQL consumers that shipped silently.
 - **tempdb no longer reports more than 100% used in FinOps Database Sizes** ([#2169], reported by @CatastropheOps) - the used percentage divided in-database usage by the size recorded in `sys.master_files`, which is the size set at configuration time and does not track autogrowth for tempdb. A tempdb that had grown was therefore measured against its startup size and rendered above 100%. The per-database probe now captures the file's current size in the same round trip it already makes for space-used, and the payload prefers it, so both halves of the ratio come from one snapshot; a database whose probe fails still falls back to the old source rather than vanishing from the grid. Affects the on-prem, RDS, and Managed Instance path - the Azure SQL Database path already read both numbers in-database.
 - **Backing out of Custom Range no longer strands an open calendar - both apps** ([#2154], reported in #2153) - a DatePicker's calendar dropdown is a popup living outside the visual tree's visibility, so collapsing the pickers when the user switched back to a preset range left an already-open calendar floating on screen; the dropdowns now close explicitly alongside the collapse, in Lite's ServerTab and the Darling Viewer's twin alike.
@@ -2672,3 +2673,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#2166]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2166
 [#2167]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2167
 [#2138]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2138
+[#2190]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2190

Darling/Darling.Tests/DocCommentHygieneTests.cs

@@ -9,6 +9,7 @@
 using System;
 using System.Collections.Generic;
 using System.IO;
+using System.Linq;
 using System.Text.RegularExpressions;
 using Xunit;
 
@@ -31,6 +32,15 @@ namespace Darling.Tests;
 /// superseded duplicate safe to simply delete. A blind "remove the extra summary" sweep would have destroyed
 /// documentation at seven of the eight.</para>
 ///
+/// <para><b>Counted by openings, not by closing tags (#2190).</b> The first version of this rule keyed off a
+/// <c>&lt;/summary&gt;</c> immediately followed by a reopening, so it saw a stacked pair only when the FIRST
+/// block was closed. Two instances sat on dev unseen: a duplicated opening tag, and a doc block that an
+/// insertion had split, stranding its head on the following member with no closing tag near it. Both are
+/// caught by counting <c>&lt;summary&gt;</c> OPENINGS inside each contiguous run of <c>///</c> lines — a run
+/// documents exactly one member, so two openings in one run means two summaries, whether or not either is
+/// closed and whether they are written single-line or spread over many. The mixed form is the one a
+/// closing-tag matcher cannot see at all: a single-line summary followed by a multi-line one.</para>
+///
 /// <para><b>Coverage limit, stated rather than assumed.</b> CI path filters are per-project, so this runs on
 /// any pull request that trips the <c>darling</c> or <c>core</c> filter, and on every nightly and release
 /// build — but a change touching ONLY Lite or Installer will not run it, and would be caught on the next
@@ -41,12 +51,13 @@ namespace Darling.Tests;
 public sealed class DocCommentHygieneTests
 {
     /// <summary>
-    /// A <c>&lt;/summary&gt;</c> closed and immediately reopened. Deliberately narrow: it matches only the
-    /// stacked-block shape, never a legitimate <c>&lt;summary&gt;</c> followed by <c>&lt;param&gt;</c>,
-    /// <c>&lt;returns&gt;</c> or <c>&lt;remarks&gt;</c>.
+    /// One <c>&lt;summary&gt;</c> OPENING tag. Counting these per doc run, rather than pairing them against a
+    /// closing tag, is what lets the rule see an unclosed first block. Still deliberately narrow: a summary
+    /// followed by <c>&lt;param&gt;</c>, <c>&lt;returns&gt;</c>, <c>&lt;remarks&gt;</c> or any number of
+    /// <c>&lt;para&gt;</c> blocks is one opening and never matches twice, and an escaped mention in prose
+    /// (<c>&amp;lt;summary&amp;gt;</c>, as used throughout this very file) is not an opening at all.
     /// </summary>
-    private static readonly Regex StackedSummary =
-        new(@"</summary>\s*\r?\n\s*///\s*<summary>", RegexOptions.Compiled);
+    private static readonly Regex SummaryOpening = new(@"<summary\s*>", RegexOptions.Compiled);
 
     [Fact]
     public void NoMemberCarriesTwoStackedSummaryBlocks()
@@ -70,12 +81,13 @@ public void NoMemberCarriesTwoStackedSummaryBlocks()
                 continue;
             }
 
-            var text = File.ReadAllText(file);
-            foreach (Match match in StackedSummary.Matches(text))
+            foreach (var run in StackedSummaryRuns(File.ReadAllLines(file)))
             {
-                /* Line number of the match start, so the failure names somewhere you can actually open. */
-                var line = text.AsSpan(0, match.Index).Count('\n') + 1;
-                offenders.Add($"{Path.GetRelativePath(root!, file)}:{line}");
+                /* Name the run's first line — somewhere you can actually open — and every opening in it, since
+                   the second one is usually the insertion point that caused the stacking. */
+                offenders.Add(
+                    $"{Path.GetRelativePath(root!, file)}:{run.Start} " +
+                    $"(<summary> openings at lines {string.Join(", ", run.Openings)})");
             }
         }
 
@@ -86,9 +98,89 @@ public void NoMemberCarriesTwoStackedSummaryBlocks()
             "an insertion pushed it away from. Seven of the eight found in #1745 were displaced doc blocks " +
             "whose real member had been left undocumented, and deleting them would have lost the documentation " +
             "rather than deduplicating it.\n\n" +
+            "Where an insertion split a block, also check whether the member it came from has since been " +
+            "re-documented in place. If it has, the stray text is a stranded HEAD rather than the whole block, " +
+            "and moving it back would create the very duplicate this rule forbids — confirm sentence by " +
+            "sentence that nothing is lost, then delete it (#2190).\n\n" +
             string.Join("\n", offenders));
     }
 
+    /// <summary>
+    /// The rule's own self-test. #2190 was a blind spot in the DETECTOR rather than in anyone's reading of the
+    /// tree, and it was a synthetic case that exposed it — so the shapes this must catch, and the ones it must
+    /// leave alone, are pinned here instead of being left to whatever the tree happens to contain. Every
+    /// <c>true</c> case below is a real shape that has appeared in this repo.
+    /// </summary>
+    [Theory]
+    /* Closed and immediately reopened: the only shape the pre-#2190 rule could see. */
+    [InlineData(true, "/// <summary>\n/// A.\n/// </summary>\n/// <summary>\n/// B.\n/// </summary>\nvoid M();")]
+    /* A duplicated opening tag, first block never closed. */
+    [InlineData(true, "/// <summary>\n/// <summary>\n/// A.\n/// </summary>\nvoid M();")]
+    /* An insertion split a block, stranding its unclosed head above the next member's whole block. */
+    [InlineData(true, "/// <summary>\n/// A.\n///\n/// <summary>\n/// B.\n/// </summary>\nvoid M();")]
+    /* Single-line followed by multi-line — invisible to a closing-tag matcher, since the reopening does not
+       follow a </summary> on its own line. */
+    [InlineData(true, "/// <summary>A.</summary>\n/// <summary>\n/// B.\n/// </summary>\nvoid M();")]
+    /* A stacked run reaching end of file with no member under it. Not valid C#, but it pins the scan's
+       one-past-the-end step: a run that never meets a non-doc line has to be closed, not dropped. */
+    [InlineData(true, "/// <summary>\n/// A.\n/// <summary>\n/// B.")]
+    /* One summary plus the other doc tags that legitimately follow it. */
+    [InlineData(false, "/// <summary>\n/// A.\n/// </summary>\n/// <param name=\"x\">X.</param>\n/// <returns>Y.</returns>\nvoid M(int x);")]
+    /* One summary carrying several <para> blocks, as most of this repo's docs do. */
+    [InlineData(false, "/// <summary>\n/// A.\n///\n/// <para>B.</para>\n///\n/// <para>C.</para>\n/// </summary>\nvoid M();")]
+    /* Two members, one summary each: the declarations between them end each run. */
+    [InlineData(false, "/// <summary>A.</summary>\nint A;\n/// <summary>B.</summary>\nint B;")]
+    /* Escaped mentions in prose are not openings — this very file is full of them. */
+    [InlineData(false, "/// <summary>\n/// Two &lt;summary&gt; mentions in one &lt;summary&gt; block.\n/// </summary>\nvoid M();")]
+    public void DetectorCountsSummaryOpeningsPerDocRun(bool stacked, string source)
+    {
+        var runs = StackedSummaryRuns(source.Split('\n'));
+
+        Assert.True(
+            runs.Count == (stacked ? 1 : 0),
+            $"Expected {(stacked ? "one stacked run" : "no stacked run")}, found {runs.Count}, in:\n{source}");
+    }
+
+    /// <summary>
+    /// Every contiguous run of <c>///</c> lines carrying more than one <c>&lt;summary&gt;</c> opening, as the
+    /// run's first line and the line of each opening. A run ends at the first line that is not a doc comment,
+    /// which is what ties it to exactly one member: the declaration itself terminates it.
+    /// </summary>
+    private static List<(int Start, List<int> Openings)> StackedSummaryRuns(string[] lines)
+    {
+        var runs = new List<(int Start, List<int> Openings)>();
+
+        /* 0 means "not currently inside a run"; line numbers reported to a human are 1-based. Iterating one
+           past the end closes a run that reaches EOF rather than dropping it. */
+        var start = 0;
+        var openings = new List<int>();
+
+        for (var i = 0; i <= lines.Length; i++)
+        {
+            if (i < lines.Length && lines[i].TrimStart().StartsWith("///", StringComparison.Ordinal))
+            {
+                if (start == 0)
+                {
+                    start = i + 1;
+                    openings = new List<int>();
+                }
+
+                openings.AddRange(Enumerable.Repeat(i + 1, SummaryOpening.Matches(lines[i]).Count));
+            }
+            else if (start != 0)
+            {
+                if (openings.Count > 1)
+                {
+                    runs.Add((start, openings));
+                }
+
+                start = 0;
+            }
+        }
+
+        return runs;
+    }
+
     /// <summary>
     /// Walks up from the test output directory to the repo root — the directory holding
     /// <c>PerformanceMonitor.sln</c>. Same walk-up idiom as <c>ThemeParityTests.FindRepoRoot</c>.

Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs

@@ -2365,7 +2365,6 @@ style of full-pathing every PG tool. */
         }
     }
 
-    /// <summary>
     /// <summary>
     /// Applies the optional per-invocation environment and working directory shared by both process
     /// runners. Values are ADDED to the inherited environment rather than replacing it — a PG tool still

Lite/Services/QueryStoreSliceRepairService.cs

@@ -573,9 +573,6 @@ union_by_name view keeps reading the un-rewritten file with the #1907 read-side
         return file.RowsRemoved;
     }
 
-    /// <summary>
-    /// Drops DuckDB's cached view of every external file, by toggling the cache off and back on.
-    ///
     /// <summary>
     /// Promotes a rewritten file over the original, and makes the store able to READ it — the two are one
     /// operation, which is why they live in one helper rather than being two things a caller must remember.