Repository navigation
Fixes #3052 - #3059
Merged
Merged
Fixes #3052#3059
Conversation
Six pins separated code from comment by line prefix. That works for `//` and `///`, which are prefixed on
every line, and not for `/* … */`, because this codebase puts no asterisk on the continuation lines: only
the first line starts with `/`, and every line after it is indented prose the filter hands over as code.
`CSharpSourceWalker.StripCommentsAndStrings` already answers the question exactly, and 23 files already ask
it. These pins now ask it too, with two exceptions that are not the same question:
- `AuroraOnlySqlIsGatedTests` looks for Aurora-only surface NAMES, which live in SQL string literals. The
stripper blanks literal text, so adopting it alone takes the found set from four files to none. It asks
the walk for the comments and reads both non-comment views instead: the code and the literal bodies.
- `AnalysisPassTokenThreadingTests` (both SKUs) carried the prefix form in `DocBlockAbove`, which COLLECTS
a `///` run to read an exemption marker out of it — the prefix is the definition of the run there, and
stripping comments is the one thing that would break it. What actually read prose as code was the call
and catch sweeps, which matched raw lines with no filter at all. Those now read the walked code; the
doc-block read still reads the file as written.
`Pg18IoBytesTests` is not an instance: its filter runs over SQL text already extracted from a raw string
literal, and the C# walk cannot read SQL.
`TimescaleEnableConnectionSafetyTests` also stops skipping its own file. The skip existed because the file
names the method throughout in prose and in regex literals; the walk removes both, so an unsafe call
written there is now caught like any other. Its own `IsCheckedOnTheSpot` note carries a worked example of
the defect — an indented `await TimescaleSupport.TryEnableAsync(...)` inside a block comment, which the
prefix filter reads as an unchecked call.
`CommentFilterAdoptionTests` is the guard on the guards: every prefix filter left in either test project is
listed with the bound it rests on, and the build re-derives the set. Four kinds are legitimate — collecting
a doc run, a stated and measured bound, a filter over SQL, and the control fixture in the guard itself —
and each entry says which. `Assert.StartsWith("/*", sql)` is excluded by construction, because asserting
that generated T-SQL opens with a disclosure header is the opposite act, and the census got that wrong
twice.
Every instance UNDER-strips, so the failure is a spurious red rather than a missed offender, and the
covered set is unchanged on `dev` today for all six.
|
Reviewed the diff (test-infrastructure only -- no T-SQL, no production code touched, so the T-SQL style section doesn't apply here). Summary: this is a well-executed fix. Adopting Things I specifically verified rather than took on faith:
No correctness, security, or performance issues found. Nothing here required an inline comment. |
The read walks a file to separate comments from code, and it enumerates every `*.cs` file in the repository — 1,345 of them — once per test, four times over. Walking unconditionally took the class from 0.55s to 5.53s, a 10x regression on a guard that is not the thing under test; gated it is 0.49s. The gate is exact rather than approximate, which is what makes skipping the walk on it safe: blanking only ever replaces a character with a space or a newline, and no surface name contains either, so a needle absent from the raw text is absent from every view of it. Roughly six files get past it. The walk also moves out of the `Any` lambda, where it ran once per surface name rather than once per file.
|
Reviewed this PR against CONTRIBUTING.md conventions and the correctness/parity/security/performance criteria. Summary: no issues found; this is test-infrastructure-only (no T-SQL, no collector/product code touched), so the T-SQL style rules don't apply here. What I checked and verified by hand:
No inline comments filed -- didn't find anything rising to an actionable issue. |
This was referenced Sep 6, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six source-scanning pins separated "code" from "comment" by LINE PREFIX. That works for
//and///, which are prefixed on every line. It does not work for block comments, because this codebase puts no asterisk on the continuation lines — only the first line starts with/, and every line after it is indented prose the filter hands to the scanner as code.CSharpSourceWalker.StripCommentsAndStringsalready answers the question exactly, and 23 files already ask it. These pins now ask it too — with two exceptions that are not the same question, both of which the census got wrong and both of which are load-bearing.Failure direction, stated plainly, because it decides how urgent this is
Every instance UNDER-strips. A comment mention reads as code, the offender list gains an entry that is not one, and an
Assert.Emptyfails loudly. That is a false positive, not a hidden offender — worth fixing, not urgent. A red test gets looked at.The instance that actually cost something was not an assertion at all. 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 nearly produced a wrong verdict about whether that PR touched code. The general rule this issue records is the same filter is loud when it drives an assertion and silent when it drives a number. The silent half lives in ad-hoc review checks that nobody commits and no test can reach. This PR fixes the half that is committed, and says so rather than claiming more.What each pin covers, before and after
Measured, not asserted. A throwaway harness ran the OLD predicate and the NEW predicate over the same real product source and diffed the result sets.
McpConfigReadAvoidsSecretColumnsTests.TheMcpSurfaceUsesNeitherSmtpNorWebhooksDarling.Service/McpServerIdentityFromStoreTests.TheServiceNoLongerDerivesIdentityServerIdentityFromStoreTests.IdentityIsMintedInExactlyThreePlacesTimescaleEnableConnectionSafetyTests.NoLiveTest_…Darling.TestsAnalysisPassTokenThreadingTests(Darling)Darling.AnalysisAnalysisPassTokenThreadingTests(Lite)Lite/AnalysisAuroraOnlySqlIsGatedTests.FilesNamingAuroraSurfacesNothing moved. No pin was passing for the wrong reason on
devtoday, and none was failing spuriously. The defect is entirely latent: it fires the moment somebody writes a block comment naming a banned token, which is exactly the thing this codebase's comment style encourages.The one exception, and it is the reason this needed a before/after rather than a "should be fine": adopting
StripCommentsAndStringsinAuroraOnlySqlIsGatedTeststakes its found set from 4 files to 0. See below.Two pins the census got wrong, and one it missed
AuroraOnlySqlIsGatedTests(Lite.Tests) was missed, and it is the interesting one. It drops//,*and/*lines from every non-test*.csin the repo, then looks for Aurora-only surface names. It is a genuine instance — and the issue's named fix would break it, because every Aurora surface name in this repo is SQL inside a string literal, which the stripper blanks. Measured:foundgoes 4 → 0, which failsAssert.NotEmptyand makes all four allow-list entries read as stale. So it asks the walk only for what the walk is exact about — where the comments are — and reads both non-comment views: the code, and the literal bodies.Pg18IoBytesTestsis not an instance. Its filter runs overDarlingPgIoReader.PgIoSql— SQL text already extracted from a raw string literal — and drops SQL block-comment lines from the outer SELECT list before counting its items.CSharpSourceWalkercannot read SQL, so adopting it there would be a category error. Left alone, recorded in the guard's allow-list with the reason and with the bound it currently rests on: the window it cuts holds no comment today, so the/*and*arms are inert; a comment added there whose continuation line contains" AS "would over-count and fail the19pin loudly.AnalysisPassTokenThreadingTests(both SKUs) was diagnosed in the wrong place. Its only prefix filter isDocBlockAbove, which collects the contiguous///block above a declaration to read the#2443exemption marker out of it. A doc comment is prefixed on every line by definition, and the marker lives in a comment — stripping comments there is the one thing that would break it. What actually read prose as code was the call and catch sweeps, which matched raw lines with no comment filter at all.AnalysisSources()now yields two line-for-line aligned views: the walked code for every regex, and the file as written for the doc-block read and the offender messages.Lite.Testsgets the walker by linkedCompile, not by a copyLite.TestscannotProjectReferenceDarling.Tests— both are xunit executables under Microsoft.Testing.Platform, and CI would discover the Darling suite twice. A second copy is exactly what #2913 removed: five pins each carried their own walk, they drifted, and only one of them had grown the newline stop that bounds a mis-read literal.So
Lite.Tests.csprojcompiles the one file:One file, one authority, and no parity pin needed because there is nothing to keep in parity — the implementation Lite compiles is byte-identical to the one
CSharpSourceWalkerTestsexercises. It brings itsDarling.Testsnamespace with it, which is why the two adopters carry ausing Darling.Tests. Thelitepath filter already namesDarling/Darling.Tests/**/!(*.md), so a change to the walker fires the Lite suite.TimescaleEnableConnectionSafetyTestsstops skipping its own fileThe skip existed because the file names the method throughout in prose and in regex literals. The walk removes both, so the exemption stopped earning its place and an unsafe call written there is now caught like any other.
This matters more than it sounds: the file that most needed the fix was the one exempting itself. Its
IsCheckedOnTheSpotnote carries a worked example of the defect — an indentedawait TimescaleSupport.TryEnableAsync(connection, null, ct);inside a block comment, no asterisk — which the prefix filter reads as an unchecked call. Restoring the old filter with the narrowed skip list reports it as a live offender.Guard on the guards
CommentFilterAdoptionTestslists every prefix filter left in either test project with the bound it rests on, and the build re-derives the set — the same agreementCommandDeadlineScannerAdoptionTestsholds over the command-timeout family. Four kinds are legitimate and each entry says which: collecting a doc run (×2), a stated and measured bound, a filter over SQL, and the control fixture in the guard itself.The shape is not banned outright, because
Lite.Tests/LiteSidebarDotRendersTheCardStatusTests.cs:206is a legitimate exception. Its doc comment states the measured bound —ServerConnection.cscarries exactly one block comment, the licence header. It also turns out to have a second, stronger reason to keep the prefix form: it asserts that four status words appear in no string literal there, so blanking literal text would satisfyAssert.DoesNotContainvacuously. Adopting the walker would convert it into a pass for the wrong reason.Assert.StartsWith("/*", sql)is excluded by construction, with a negative lookbehind. Asserting that generated T-SQL opens with a risk-disclosure header is the opposite act from filtering comments out of source, and the census put four files on the list for having the substring. Dropping the lookbehind makes the guard flagDarlingMcpToolsTests,ViewerRecommendationsTests,LiteRecommendationsReaderTestsandMcpAnalysisFindingsCommandTests— so the exclusion is load-bearing and proven.No
build.ymlfilter entry, deliberately: the guard reads both test projects, and namingLite.Tests/**in thedarlingfilter would also gate the live-Postgresdarling-pgjob on all 298 of Lite's test files. It belongs in the categorywhole-tree-guardsalready exists for, and that job runs this suite exactly when the build job's step reportsskipped.Verification
Darling.TestsandLite.Teststargetnet10.0-windowsand cannot run on macOS. The real test files were compiled into throwawaynet10.0xunit v3 hosts namedDarling.Tests/Lite.Tests, staged outside the tree with their output inside the (gitignored) projectbin/so the pins'AppContext.BaseDirectoryand[CallerFilePath]walk-ups resolve against the real repo root.Both projects also build clean with
-p:EnableWindowsTargeting=true, and the warning profile is byte-identical todev's.Green: 30 tests in the Darling host (1 skip — the live
DARLING_TEST_PGround-trip), 9 in the Lite host. Re-run after mergingorigin/devin; unchanged.CI ran all eight checks green twice — on
ce76919e5(before the cost gate) and again on the head — includingbuild(Windows, both suites),Darling PostgreSQL testsagainst a live Postgres, andDarling whole-tree guards. The gate is therefore a cost fix over an already-green substance, and it is red-proofed above as answer-preserving.Cost, because the first green run measured it
The Aurora read walks a file to separate comments from code, and it enumerates every
*.csfile in the repository — 1,345 of them — once per test, four times over. The first version walked unconditionally and took that class from 0.55s to 5.53s locally. On CI, measured within this branch across the two commits, the WindowsRun Lite testsstep went 5m37s ungated → 4m44s gated (a comparable run on another branch was 4m22s, so the residual is branch-to-branch noise rather than a remaining regression).It is now gated on the raw text first, which is exact rather than approximate and is what makes skipping the walk safe: blanking only ever replaces a character with a space or a newline, and no surface name contains either, so a needle absent from the raw text is absent from every view of it. Roughly six files get past the gate, and the class is back to 0.49s. The walk also moved out of the
Anylambda, where it had been running once per surface name rather than once per file.Red-proofed eleven ways, each against a copy of the tree at a committed SHA, each failing a different named assertion. Two are two-sided — a block comment naming the banned token is injected into real product source, and the pin is run with the walker (green) and with the prefix filter (red) — and the last pair proves the cost gate changes cost and not answers:
TimescaleEnableConnectionSafetyTestsNoLiveTest_…— two offenders, both block-comment continuation lines in its own fileAuroraOnlySqlIsGatedTestsadoptsStripCommentsAndStringsaloneTheAllowListHasNoStaleEntries(all 4 entries),EveryFileNamingAnAuroraOnlySurface_IsAccountedFor(Assert.NotEmpty),TheReadFindsASurfaceInSql_AndNotOneInABlockComment,APairedFileReallyReadsTheVanillaViewToo×2s_boundedentryEveryLinePrefixCommentFilter_StatesTheBoundItRestsOn— stale-entry sideLite.TestsfileTheLinePrefixForm_ReadsABlockCommentsBodyAsCode_AndTheWalkDoesNot(?<!Assert)lookbehindDarlingMcpToolsTestsconfig.Smtpin a block comment, prefix filter restoredTheMcpSurfaceUsesNeitherSmtpNorWebhooks—["DarlingMcpServerAdminTools.cs"]GetDeterministicHashCode(in a block comment, prefix filter restoredTheServiceNoLongerDerivesIdentityandIdentityIsMintedInExactlyThreePlaces(a fourth minting site)reader.ReadAsync()in a block comment, walk reverted, both SKUsNoStoreCallOnTheAnalysisPassRunsWithoutThePassTokenin eachAssert.NotEmptyWith the walker in place and the injected comments still present, all of those pass — so the controls fail in the intended direction rather than merely failing.
Not verified:
build,Darling Linux buildandDarling PostgreSQL testsare the arbiters for the full ~4,800-test suites; only the eight pin classes touched here were executed locally.Lite.Tests/AnalysisPassTokenThreadingTests.cs(AnAbandonmentIsAFiredTokenAndACancellationShape_…,TheReadLockWaitIsAbandonableWhileAWriterHoldsIt) were filtered out: they needPerformanceMonitorLite, which is WPF. Neither is touched by this change.Lite.Tests/LiteSidebarDotRendersTheCardStatusTestswas not executed. It is unchanged, and the guard's own run proves it is not flagged.Adjacent finding, not fixed here
CrossAppGuardCiGateTestscannot see aDarling.Tests→Lite.Testsreference at all. Its detector keys on"Lite/…"andPath.Combine("Lite", …), andLite.Testsis a sibling ofLiterather than a directory inside it — so the class of cross-app read this PR introduces is invisible to the pin whose stated job is to require it be filter-reachable. Covered here bywhole-tree-guardsrather than by a filter entry, so nothing is unguarded, but the detector gap is real and pre-existing.CHANGELOG entry text
Not applied — every lane appends to the same
[Unreleased]block.