Repository navigation
Stacked <summary> doc comments: six on dev, invisible to every automated check #1745
Description
Activity
- added a commit that references this issue
on Jul 27, 2026 Fixed and pinned in #1751, merged to
devat6fa21e8c(all four checks green). Closing manually rather than by keyword — GitHub only auto-closes on a merge to the default branch, and this repo merges todev.Two corrections to the issue as filed, both found during the audit.
It was eight, not six. The inventory here was taken from
devbefore #1744 merged. Re-scanningdevatf54485eefound two more —DarlingWorker.csandDarlingSelfAlertEvaluator.cs, the two files #1739's alert-surfacing fix touched. The class recurred inside the PR sequence that was fixing two instances of it, and shipped. That is a better argument for the guard than anything in the original filing.The defect is displacement, not duplication — which inverts the fix. An edit inserts a new member and its summary above an existing summary, stranding that summary on the wrong member. There are two casualties each time, not one: the new member reads with a description of something else, and the member the orphaned block actually described is left with no documentation at all.
RunCollectionLoopAsyncandEvaluateCompressionJobsAsyncwere both undocumented ondevfor exactly this reason.So the obvious remedy is wrong almost every time: seven of the eight needed the orphaned block moved back to its real member; only one was a superseded duplicate safe to delete. A blind "remove the extra summary" sweep would have destroyed real documentation at seven of eight sites. The guard's failure message now says so outright, so the next person to hit it does not reach for the obvious wrong fix.
Per-site disposition (all eight, at
f54485ee)# Site Orphaned block describes Member left undocumented Verdict 1 DarlingWorker.cs:582post-bootstrap store/collection loop RunCollectionLoopAsync:605MOVE — introduced by #1739/#1744 2 DarlingSelfAlertEvaluator.cs:1127isolating compression-job entry point EvaluateCompressionJobsAsync:1235MOVE — introduced by #1739/#1744 3 TimescaleSupport.cs:369query_stats DAILY hierarchical CAGG CreateQueryStatsDailySql:422MOVE 4 MainWindow.xaml.cs:768visible-tab refresh + overlap guard RefreshVisibleAsync:786MOVE 5 ViewerDataService.FinOps.Inventory.cs:130Server Inventory base rows ServerInventorySql:147MOVE 6 ThemeParityTests.cs:194repo-root walk-up FindRepoRoot:198MOVE 7 RemoteCollectorService.cs:30the base collector service class class RemoteCollectorService:99MOVE 8 LocalDataService.QueryStore.cs:418superseded per-query history text none — same member DELETE Site 8 is the only genuine duplicate: both summaries describe
GetQueryStoreHistoryAsync, the doc was rewritten from "a specific query" to "ALL plans of a query", and the new text was added below the old instead of replacing it. Verified there is no per-plan sibling and that the preceding method kept its own docs, so nothing was displaced.Every MOVE was verified before being touched: locate the member the orphaned prose actually describes, confirm that member currently has no doc comment, then move the block to it. Nothing was deleted on the assumption it was redundant.
The guard
Darling/Darling.Tests/DocCommentHygieneTests.cswalks every.csoutsidebin/objfrom the repo root and fails on</summary>immediately followed by<summary>— the regex proposed here.It fails rather than skips when it cannot locate the tree. A
Skipwould mean the rule silently stops being enforced the day the output layout changes, which is the same shape as the defect it guards.Verified in both directions, since a pin nobody has tried to break is not evidence:
green on the fixed tree Passed! - Failed: 0, Passed: 1 stacked block re-introduced in Lite [FAIL] Lite\Services\RemoteCollectorService.cs:29 reverted Passed! - Failed: 0, Passed: 1That red is a mutation in Lite caught by a test living in Darling.Tests, which is the cross-project reach this rule needs.
Coverage limit, documented in the test rather than assumed: CI path filters are per-project, so it runs on any pull request tripping the
darlingorcorefilter 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 nightly instead.- added a commit that references this issue
on Jul 29, 2026 - added a commit that references this issue
on Aug 19, 2026 - added 4 commits that reference this issue
on Aug 19, 2026
Surfaced while fixing two instances of this in #1739/#1744. Filing the class rather than the two, because the two are already fixed and the other six are not.
The defect class
A member ends up with two
<summary>elements stacked, usually because an edit added a new summary above an old one instead of replacing it:XML documentation takes the LAST summary. So IntelliSense, generated docs, and every analyzer render the correct text. The build is clean, the tests pass, nothing warns. The only reader who is misled is a human reading the file top-down — and the first thing they read is the wrong description, attached to the wrong member.
That is what makes this worth a guard rather than a cleanup: it is invisible to every automated check we currently run, and re-reading does not find it. In #1739 the same region was read at least three times by two people over several hours without either of us noticing the duplicate; a regex found it in one pass.
Verified inventory on dev (6 sites)
Confirmed by walking every
.cs(excludingobj/bin) on dev atf54485ee:Four projects, all pre-existing, none introduced by #1706.
Two of them are actively misleading, not benign duplicates
RemoteCollectorService.cs— the file's old service-level header ("Base service for collecting performance data from remote SQL Servers") was never removed when a DTO was added at the top of the file, soCollectorHealthEntry, a small health-tracking class, now carries the description of the whole service.TimescaleSupport.cs— a summary describing the daily hierarchical CAGG sits directly above one describing the per-database rollup (Darling viewer: built-in tabs silently lose history past 4 days - CAGG read-routing only covers Custom Views #1661). Two different objects; a top-down reader gets the daily-CAGG explanation attached to the per-database member. Both summaries are long and detailed, which makes the wrong one more convincing, not less.I have not audited the other four.
Proposed guard: one regex
The repo already has the idiom for pinning things no behavioral test can see —
HostHeaderGuardTests(middleware ordering, #1648) andDarlingStoreUpgradeTests(the store-upgrade wiring pins, #1739). This one is cheaper than both, because it needs no fixture wiring and can walk the source tree directly:One assertion over the
.csfiles, milliseconds to run, and it would have caught both of tonight's and all six of these. Probably the best guard-per-defect ratio of anything we have added.Suggested order
Not urgent and nothing is broken at runtime. Worth doing before someone quotes the wrong summary in a code review or a doc.
🤖 Generated with Claude Code