Repository navigation
Pins that split code from comments by line prefix miscount block-comment bodies, which carry no asterisk in this codebase #3052
Description
Several source-scanning pins separate "code" from "comment" by line prefix — dropping lines that start with //, ///, / or *. That works for C# line comments and doc comments, which are prefixed on every line. It does not work for block comments, because this codebase's /* … */ continuation lines carry no * prefix:
/* Released here rather than at the closing brace: the six sub-tab loads at the end of this method
run after these five have finished, so they do not contend with them. */Only the first line starts with /. Every continuation line is indented prose that a prefix filter classifies as code.
Verified instances
Darling/Darling.Tests/ServerIdentityFromStoreTests.cs:180-182 — drops /// and *, then flags any file whose remaining lines contain GetDeterministicHashCode(. A block comment mentioning that method on a continuation line makes the file an offender.
Darling/Darling.Tests/McpConfigReadAvoidsSecretColumnsTests.cs:80 — drops lines starting with / or *, then flags .Smtp / .Webhooks. Its own inline comment says "The comment explaining the skip names both types, so only real USES count", which is exactly the intent the filter fails to deliver for a block comment.
The failure direction differs by use, and that is the part worth getting right
For both pins above the assertion is Assert.Empty(offenders), so a comment mention makes the test fail loudly. That is the safe direction — a spurious red, not a missed defect. The cost is real but modest: somebody eventually "fixes" it by rewording a comment, and the pin quietly becomes a constraint on prose.
The dangerous use of the same filter is measurement rather than assertion. During #3047's review a check of this shape reported 45 changed lines in CollectorScheduleDefaults.cs for a comments-only diff. Nothing failed; the number was simply wrong, and it nearly produced a wrong verdict about whether that PR touched code. A filter that over-reports silently in a count is a different severity from one that over-reports loudly in an assertion, and this shape is used both ways.
Not a defect, and worth leaving alone
Lite.Tests/LiteSidebarDotRendersTheCardStatusTests.cs:206 drops only //-prefixed lines, and its doc comment states the bound it rests on: "ServerConnection.cs carries exactly one block comment — the licence header at the top — so dropping //-prefixed lines is sufficient and cannot eat a string literal." That is a measured claim about a named file rather than an assumption, and it is correct today. It is measurement-dependent, not permanent: a second block comment in that file makes it wrong, which argues for it eventually adopting the shared walker rather than for changing it now.
The grep also turns up many StartsWith("FROM ") / StartsWith("JOIN") / StartsWith("--") filters. Those are SQL clause and SQL-comment handling, not this problem.
The fix already exists
Darling/Darling.Tests/CSharpSourceWalker.cs blanks comments and string literal text while preserving newlines and interpolation holes — its own doc comment says it "cannot MISS code that happens to be written inside" and cannot "match prose in a comment or text in a literal". The two verified pins should use it. Several other pins already do.
Worth considering a guard on the guards: a pin asserting that no test in Darling.Tests or Lite.Tests separates code from comment by line prefix without a stated bound like the Lite one's. That is the shape that would stop this recurring, and it is cheap because the correct helper is already there to point at.
Provenance
Found by fable-loop while verifying #3047's "comments and one test only" claim, and confirmed here by reading the three candidate call sites. Filed rather than fixed because it spans two projects and because the right fix is adopting the shared walker, which changes what those pins scan and so needs its own before/after on the offender lists.
Activity
erikdarlingdata commentedon Sep 5, 2026
Claude posting for Erik Darling
The census is wider than what I first verified: eight committed test files carry the shape, not three. From fable-loop, cross-checked against
origin/dev:AnalysisPassTokenThreadingTests(both Darling and Lite),DarlingMcpToolsTests,McpConfigReadAvoidsSecretColumnsTests,Pg18IoBytesTests,ServerIdentityFromStoreTests,TimescaleEnableConnectionSafetyTests,ViewerRecommendationsTests.And the correct helper is already the majority idiom: 23 files use
CSharpSourceWalker.StripCommentsAndStrings. So this is adopting the established shape in eight stragglers rather than introducing anything.The direction was checked per instance, and it matters
Every instance read so far under-strips — a block-comment continuation line survives the filter and is read as code. None over-strips. That is the important asymmetry: over-stripping would hide a real offender silently, while under-stripping produces a false positive.
McpConfigReadAvoidsSecretColumnsTests— a comment mentioning.Smtp/.Webhooksreads as a use. Red build, someone investigates.Pg18IoBytesTests— excludes/*,*andSELECTprefixes and counts the remainder as columns, so an unstripped prose line over-counts and trips the expected count. Same loud direction.So: worth fixing, not urgent. A red test gets looked at.
The severity is in the third use, and it is not an assertion at all
The instance that actually cost something had no assertion behind it. During #3047's review a check of this shape reported 45 changed lines in a comments-only diff of
CollectorScheduleDefaults.cs. Nothing went red, because nothing was asserting — the number fed a human judgement, and it nearly produced a NO-GO on prose.fable-loop's own framing, which is the right one to keep: "My case was worse than any of these precisely because the over-count fed a human judgement instead of an assertion: nothing went red, and I nearly NO-GOd your PR on prose."
That is the general rule this issue is really about. The same filter is loud when it drives an assertion and silent when it drives a number. Fixing the eight pins removes the noisy half; the quiet half lives in ad-hoc review checks that nobody commits, and the only defence there is comparing the parsed thing rather than the line count — as the eventual #3047 check did, extracting all 69
["name"] = new(...)entries from both revisions and finding them byte-identical.erikdarlingdata commentedon Sep 5, 2026
Claude posting for Erik Darling
Correcting my own comment above: the census is six files, not eight.
DarlingMcpToolsTestsandViewerRecommendationsTestsdo not carry this shape and should not be on the list.What they carry is
Assert.StartsWith("/*", sql)— assertions that a generated T-SQL string begins with a block comment, because the copy-paste recommendation prepends a two-sided risk disclosure header above the runnable statement. That is a test asserting output has a comment, not a filter separating comments out of source. Opposite direction, same substring.The six that do carry it:
AnalysisPassTokenThreadingTests(Darling and Lite),McpConfigReadAvoidsSecretColumnsTests,Pg18IoBytesTests,ServerIdentityFromStoreTests,TimescaleEnableConnectionSafetyTests. And the correct helper is used by 24 files, not 23.I should have caught this before posting. My own verification table printed
prefix-filters: 0for both of those files, and I adopted the count of eight anyway instead of asking why two came back zero. A zero in a column I added specifically to check the claim is the one number I had no excuse for skipping past.The irony is worth recording rather than smoothing over: this is the same error the issue is about. A pattern matched, the meaning behind it was not checked, and a number came out wrong —
StartsWith("/*"counted as a comment filter exactly the way a comment continuation line counts as code. Both times the fix is to look at what the matched text is for rather than that it matched.Nothing else in that comment changes. The direction analysis holds for all six, the severity argument holds, and the fix is still adopting
CSharpSourceWalker.StripCommentsAndStrings.erikdarlingdata commentedon Sep 5, 2026
Claude posting for Erik Darling
One clarification so two numbers do not sit in the record unexplained: 23 versus 24 are both right and the difference is definitional. Qualified call sites of
CSharpSourceWalker.StripCommentsAndStringsnumber 23. The count of 24 files includesDarling/Darling.Tests/CSharpSourceWalker.csitself, which defines the method rather than calling it.Counting the definition as an adopter is the same category error as counting
Assert.StartsWith("/*", sql)as a comment filter — the name appears, the role is different. Worth stating because "24 files already use it" and "23 files already use it" would otherwise read as one of us being wrong about the tree.Running tally of instances of this issue's own defect committed while investigating it, since the pattern is the point: fable-loop's 45-line count on a comments-only diff, its two false positives in the census, and my adopting the count of eight after my own verification column printed zero for both of them. Four, across two sessions, in one evening. Every one was "the regex fired, therefore the claim" — and the fix in each case was reading what the matched text was for.
Fixes #3052 (#3059)
erikdarlingdata commentedon Sep 6, 2026
Claude posting for Erik Darling
Closed by #3059, merged to
devatd1af095cc31534dcf67aa000289ebbbc0d0f5550. Closing by hand because a closing keyword does not fire againstdev.Nothing moved, and that is the finding. The old and new predicate were run over the same real product source and the offender sets diffed: byte-identical on all seven pins — 0→0, 0→0, 3→same 3, 40→same 40, 16/7/27 identical, 15/5/27 identical, 4→same 4. No pin was passing for the wrong reason and none was failing spuriously. The defect was entirely latent, which is the opposite of what this issue's framing implied. Reporting that rather than claiming six fixes is what makes the rest of it credible.
This issue was wrong at most of its named sites, and the lane established that with reasons rather than by declining the work:
Lite.Tests/AuroraOnlySqlIsGatedTests.cs:170— the census missed it, and it is the one place the prescribed fix is actively wrong. Every Aurora surface name is SQL inside a string literal, whichStripCommentsAndStringsblanks: adopting the walker there takesfoundfrom 4 files to 0. It now asks the walk only where comments are, and reads both non-comment views — code, and literal bodies. It also pins why it must not adopt the walker, asserting the literal is found AND that the stripped text does not contain it, so anyone "simplifying" it on this issue's authority reds.Pg18IoBytesTestsis not an instance — its filter runs over SQL already extracted from a raw literal, and a C# walk cannot read SQL. Same category as theStartsWith("--")hits this issue itself set aside.AnalysisPassTokenThreadingTestswas diagnosed in the wrong place, in both SKUs. Its prefix filter isDocBlockAbove, which collects a///run to read an exemption marker — so stripping there is the one thing that would break it. The real gap was the call and catch sweeps matching raw lines with no filter at all. The census's own instrument committed this issue's error a third time.Three files genuinely adopted it. The Lite side takes a linked
Compilerather than a copy — aProjectReferenceis impossible since both are xunit executables and CI would discover the Darling suite twice, and a second copy is exactly what #2913 removed after five private walks drifted and only one had grown the newline stop. One file means there is nothing to keep in parity, hence no parity pin.A cost regression was introduced, measured, and then mis-stated before being caught. The Aurora read walks 1,345 files four times; walking unconditionally took that class 0.55s → 5.53s, now gated on the raw text first — exact rather than approximate, because blanking only substitutes spaces and newlines. The lane first wrote "over 25 minutes" into a doc comment and a commit message from a misjudged wall clock; the real figure is 5m37s → 4m44s. Caught before pushing. Worth recording the shape: an unmeasured number in a comment, on the issue about exactly that.
A guard blind spot this PR walked into is filed as #3063:
CrossAppGuardCiGateTestsenumerates*.csonly, so the linkedCompile Includeintroduced here is invisible to the pin whose job is to require such cross-app reads be filter-reachable. Nothing is unguarded —whole-tree-guardsruns regardless of area filters — but the guard cannot know that.The general rule worth keeping from this issue: the same filter is loud when it drives an assertion and silent when it drives a number. Every instance here was an assertion, so every failure would have been a red build. The instance that actually cost something was a review check with no assertion behind it, which reported 45 changed lines in a comments-only diff and nearly produced a wrong verdict.
Read both source files through CSharpSourceWalker rather than a comme…
Fixes #3062 (#3064)