Skip to content

[BUG] DocCommentHygieneTests misses unclosed stacked summaries — two live on dev, one orphaned a member's docs #2190

Description

@erikdarlingdata

Component

Darling Tests

Describe the Bug

DocCommentHygieneTests.NoMemberCarriesTwoStackedSummaryBlocks misses the case where the first <summary> is never CLOSED. It passes on dev today while two members carry stacked summaries, one of which has lost its documentation entirely.

I found this by accident. That pin caught me three times while working #2166, so I wrote a local pre-push version of it — and my first attempt had the same blind spot, keying off /// </summary> and walking forward. It failed its own self-test on a synthetic case, and fixing it to count <summary> OPENINGS inside each contiguous /// run turned up two live instances the CI test does not see.

1. Darling/PerformanceMonitor.Darling.Service/DarlingManagedPostgres.cs ~2368 — a duplicated opening tag on ApplyProcessEnvironment:

    /// <summary>
    /// <summary>
    /// Applies the optional per-invocation environment and working directory shared by both process
    /// runners. ...
    /// </summary>
    private static void ApplyProcessEnvironment(

Two openings, one closing. This one is a straightforward stray line to delete — the prose below it clearly belongs to ApplyProcessEnvironment.

2. Lite/Services/QueryStoreSliceRepairService.cs ~576 — the real one, and it is the #1745 displaced-doc pattern the test's own failure message warns about:

    /// <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, ...
    /// </summary>
    private async Task PromoteRewrittenFileAsync(

That first summary is FlushExternalFileCacheAsync's (declared around line 633), pushed away from its member by an insertion. So PromoteRewrittenFileAsync carries two openings AND FlushExternalFileCacheAsync is left undocumented.

Do not fix #2 by deleting the first block — that would lose real documentation. It needs moving down onto FlushExternalFileCacheAsync.

Expected Behavior

The pin should count <summary> openings within each contiguous run of /// lines, rather than pairing on the closing tag. A contiguous doc run documents exactly one member, so two openings in one run means that member has two summaries — regardless of whether either is closed, and regardless of whether they are written single-line (/// <summary>x</summary>) or spread over several lines.

The mixed form matters and is what fooled my first attempt: the #2166 instance was a single-line summary followed by a multi-line one, which a closing-tag detector cannot see at all.

Actual Behavior

Passes on dev with two stacked blocks present, one of them having orphaned a member's documentation.

Additional Context

Worth noting the pin is genuinely earning its place — it caught three of my own instances in one change set, and every one would have shipped as silently wrong documentation, since XML docs take the LAST block so tooling renders fine and only a human reading the file is misled. This is about widening its net, not doubting it.

Small change: strengthen the assertion, then fix the two it surfaces (one deletion, one move).

Activity

  1. added a commit that references this issue on Aug 11, 2026
  2. erikdarlingdata commented on Aug 11, 2026

    @erikdarlingdata
    OwnerAuthor

    Shipped in #2192. The detector now counts summary OPENINGS per contiguous /// run instead of pairing on closings, so an unclosed stacked summary cannot hide - watched red against the unfixed tree on exactly the two live instances, with the old rule passing the same tree as the control. A 9-case self-test theory pins the shapes (5 it must catch, 4 legitimate ones it must not), since this was a blind spot in the detector rather than in anyone's reading. ONE DELIBERATE DEVIATION from this issue's text, verified before acting: the issue said not to delete instance 2 because FlushExternalFileCacheAsync is left undocumented - git blame shows bb1f48c already gave that member its own complete summary in place (line 630: 'Evicts DuckDB's cached view... The WHY is on PromoteRewrittenFileAsync') when it split PromoteRewrittenFileAsync out of the original block; what sat at line 576 was only the stranded HEAD of the pre-split comment, its one surviving sentence already improved on the correct member. Restoring it would have stacked a SECOND summary on a documented member - recreating the exact defect the rule forbids - so it was deleted, nothing lost, verified sentence by sentence. Reopen if you read that history differently. Two by-products: #2193 (pre-existing analyzer warnings on dev, tracked not dropped) and a repo hazard now known to all active lanes - CHANGELOG.md carries 6 bare CR bytes that editor-normalization silently explodes into a ~5,350-line diff; check the diffstat before committing any CHANGELOG edit.

  3. added a commit that references this issue on Aug 11, 2026
  4. added 2 commits that reference this issue on Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions