Repository navigation
Fixes #3082 - #3084
Merged
Merged
Fixes #3082#3084
Conversation
…what it cannot see The matcher's three arms stay as they are. What changes is that the boundary is stated with the measurement behind it, and every shape outside it has a case. TheCSharpMatchers_StillCannotSeeTheseSpellings grows from three unnamed texts to a seven-shape census, each entry naming its live instances, each floored on spelling the root it is about and asserted by equality against an empty answer. The shape names are required distinct, because a duplicated name drops a shape out of the census while every remaining entry still reports a pass. Three doc claims about the #3076 measurement were wrong and are corrected: the bare-token count on the Lite arm is 21 rather than 20, and the claim that all of them are labels, assertion keys or this class's own root constants is false for thirteen - ten are first-argument Path.Combine reads the second arm resolves, and three name a tree in a spelling no arm reaches.
|
Reviewed. This PR only touches I traced each of the 7 new census entries (
The new CONTRIBUTING.md subsection's description of the three visible spellings matches the shipped No correctness bugs, no security concerns (no new file/network/process/SQL surface — the MSBuild shell-out paths are unchanged), and nothing here creates Lite/Darling drift; the file already tests both SKUs symmetrically via |
erikdarlingdata
added a commit
that referenced
this pull request
Sep 6, 2026
PR #3084 landed while this was open and wrote the three spellings CrossAppGuardCiGateTests can see into CONTRIBUTING.md, naming a segment array declared in another member as silent by construction. That is the shape this file's root list has, and the anchors are the first of the three - so the claim now points at the document rather than at the guard's own doc comment, and the red-proof was re-run against the merged matcher.
Merged
erikdarlingdata
added a commit
that referenced
this pull request
Sep 6, 2026
* Guard the checkable half of CONTRIBUTING's T-SQL conventions CONTRIBUTING.md's T-SQL Style list opens "All T-SQL code must follow" and nothing enforced any of it: PR #3078 introduced COUNT(*) and COUNT(DISTINCT ...) into a new collector query and passed the Linux build, the PostgreSQL tests, all four whole-tree guards, the command-deadline family, review and verify. TsqlConventionGuardTests scans string literals that OPEN a T-SQL statement across the shared libraries and both SKUs' app trees, and enforces four of the nine bullets: COUNT_BIG() over COUNT(), ROWCOUNT_BIG() over @@rowcount, block comments over --, no tabs, and lowercase data types. The other five bullets, and the abbreviation half of the data-type bullet, are recorded with their reasons and carry a fixture each whose expected answer is empty. The nine pre-existing violations of the covered subset are fixed here, so the guard lands with no waiver list: two COUNT(*) in DatabaseSizeStatsCollector and one inside sp_executesql in Lite's FinOps recommendations, one -- comment in DarlingServerConnector's T-SQL probe (its PostgreSQL twin keeps its own, correctly), and NVARCHAR(500), VARBINARY(64), NVARCHAR(128) and two nvarchar(MAX) spellings. * Make every required bullet phrase load-bearing on its own The Functions bullet's phrase list asked for COUNT_BIG(), and rewriting CONTRIBUTING.md's COUNT_BIG() to COUNTBIG() left the check green: two words later ROWCOUNT_BIG() still contains COUNT_BIG() as a substring, so the Contains was satisfied by the wrong token. Found by mutating the document rather than by reading the list. The phrases now carry the backticks the document writes them with, and EveryRequiredPhrase_IsLoadBearingOnItsOwn deletes each one from the real bullet text in turn and requires the check to red - so a phrase absorbed by a longer neighbour cannot pass again. * Give the nesting walk and the length-spec anchor fixtures that can fail Two claims had no case that could distinguish them, and a mutation of each survived. The nested-block-comment fixture put its -- BEFORE the inner close, where a nesting and a non-nesting walk both read it as comment. It now sits after the inner close and before the outer one, which only the nesting walk keeps inside a comment. The MAX length spec's requirement that the name in front of it be a data type changed no answer on any fixture, because none had (MAX) after a non-type identifier. MAX(MAX) - legal T-SQL, since MAX is not reserved and a column may be named it - is textually IDENT ( max ) and reds without the anchor. * Pin the COUNT( lookbehind with the only shape it decides Removing the lookbehind from the COUNT( pattern changed no answer on any fixture. COUNT_BIG( and ROWCOUNT_BIG() are rescued by the underscore that follows COUNT rather than by what precedes it, and @@rowcount is never followed by an open parenthesis - so the lookbehind's whole job is an identifier that ends in Count and is then called. dbo.RowCount(...) is that shape. * Anchor each scanned tree on a named file, and say what holds reachability Two things, one change. A per-tree floor is satisfied by any file in the tree, so each group now names an anchor whose T-SQL must stay visible: the file #3078's violation landed in, and the two non-shared files this change fixed. A glob that reaches part of a tree, or a marker set that goes blind on one file's shape, now fails by name. And the anchors are separator-bearing path literals, which is the first arm of CrossAppGuardCiGateTests' C# matcher. The bare "Lite" in ScannedTrees is the collection-declared-in-one-member shape that matcher records as invisible, so before the anchors existed this file's read of Lite was not in its found set at all - pointing a root at Lite.Tests, which the darling filter cannot reach, changed nothing. With the anchors it reds. * Record that the reachability red-proof needs a path that exists CrossAppGuardCiGateTests drops references that do not resolve on disk, so the first attempt at proving the anchor reaches its found set used a filename that was not there and reported a pass that meant nothing. * Derive the abbreviation rule's out-of-scope cost instead of asserting it The Data types bullet's abbreviation half is left uncovered on a cost - 33 int-for-integer sites across 7 files - and a cost written down once goes stale by exactly the mechanism this guard exists to stop. The detector for the rule now lives beside the enforced ones, reads the same population and the same code view, and its count is compared against the figure the scope note states. The direction that matters is downward: when those sites are fixed the pin says to move the rule to the covered side, rather than leaving an unguarded rule behind a reason that has expired. * Drop the unpinned counts from the summary, and date the ones that stay "twenty-three times" was 21, which is what an unpinned numeral in prose does. That one and the command-deadline family's test count are gone; the figure they were decorating was never the point. The .sql-tree measurement stays because it is the evidence for a scope decision, but it now says it was taken once and not re-taken, so nobody reads it as a live figure. The only live count left is the abbreviation cost, and a pin holds that one. * State the cost of keeping string values in the code view The token checks read single-quoted spans other than XQuery arguments, so a string VALUE containing a banned token would be reported as the token. There is none, and the direction is a spurious red on correct SQL rather than a hidden violation - but the alternative is missing every violation inside dynamic SQL, and one sp_executesql body held one of the nine fixed here. Worth saying out loud rather than leaving to be rediscovered. * Replace the remaining prose counts with names, and pin the split pair An audit of every numeral in the file found three more that were simply wrong: "three bullets are split" (two are), "six files carry a -- inside a block comment" (five do), and "four N'' literals inside one sp_executesql" (three). Each was decorating a point the numeral was not making. The split bullets are now named rather than counted, and a set-equality assertion holds the naming, so a third one reds instead of leaving the prose incomplete. The rest are named instances or structural facts. * Cite CONTRIBUTING's new cross-app section for the reachability claim PR #3084 landed while this was open and wrote the three spellings CrossAppGuardCiGateTests can see into CONTRIBUTING.md, naming a segment array declared in another member as silent by construction. That is the shape this file's root list has, and the anchors are the first of the three - so the claim now points at the document rather than at the guard's own doc comment, and the red-proof was re-run against the merged matcher. * Enumerate the marker set's residue, which found real T-SQL outside it The summary claimed every read of a monitored SQL Server goes through a DMV, a catalog view or a built-in, so nothing T-SQL could fall outside the population. That is a universal quantifier, and testing it instead of stating it found two counter-examples in the population's complement. One mattered: DarlingXeSessions' provisioning DDL - CREATE/ALTER/DROP EVENT SESSION with sqlserver.* events and package0.ring_buffer targets - is real T-SQL sent to production servers on every session repair, and carried no marker at all. TsqlMarkers now reaches EVENT SESSION, sqlserver. and package0., and two of those statements are population fixtures. They carry no violation of the covered rules. What remains is FactRemediation's cursor fetch, twice, and TheTsqlTheMarkerSetCannotSee_IsTheseAndNoOthers holds it at set equality over the complement so a third cannot arrive unnoticed. The complement is 500-odd literals; the discriminator that makes it readable is the parameter spelling, @name being the one thing neither store dialect writes. Also closes the ]] escape in the quoted-identifier walk, which the review of this change flagged as the asymmetry with the '' arm beside it. * Drop the two XE markers no statement depends on sqlserver. and package0. went in beside EVENT SESSION. Measured: zero statements in the corpus carry either without also carrying EVENT SESSION, so no fixture could show them mattering and a mutation deleting them stayed green. A marker nothing depends on is a claim nothing holds. EVENT SESSION is the one that earns its line, and it is red-proofed.
This was referenced Sep 6, 2026
Closed
Merged
Closed
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.
The C# matcher's three arms are unchanged. This is the decision #3082 asks for, delivered as option 2: the boundary stands, it is stated as a decision rather than a gap, and every shape outside it now has a case.
The shape chosen: accept the boundary
Not a fifth widening, and the reason is a measurement rather than fatigue.
The corpus shape enumeration
Every quoted occurrence of a cross-app root in both test trees was enumerated and read individually — 37 bare tokens plus the separator-carrying spellings in
Darling.Tests, 21 plus the same inLite.Tests. Attribution was taken by feeding the shippedCSharpPathsby reflection and blanking one site at a time, so FOUND/UNFOUND is the matcher's own answer rather than a re-implementation of it.The corpus names a cross-app file in seven shapes no arm reaches, and #3082's is one of them:
VariableBaseCollectorStateContractTests,QueryStoreStatePruneTests,FactCollectorFailureReportingTests; on the Lite armWatermarkPolicyTests,ThemeParityLiteDarlingTestsPath.Combine's first argument (this issue)TerminalRootLiteralCommentFilterAdoptionTestsSegmentsSplitAcrossTheHelperBoundaryStoreSqlClockDisciplineTests, definition + 4 call sitesVariadicSegmentHelperFileGrowthAlertStoreTestsPath.Combineand no file API in the expression — a user helper, andDarling.Testscarries 34 separate privateReadRepoFiledeclarations, so the name is not a token to anchor onSegmentCollectionScopedLoadOrderingTests(s_finOps,s_liteWindows)SegmentArrayAsASiblingArgumentViewerScopedPaintGuardTestsCrossMemberCollectionXamlStaticResourceHygieneTestsWhat a fifth widening buys, measured
Built the widest regex this arm could plausibly become — root literal at ANY
Path.Combineargument position including the last, one level of call nesting allowed in the base — and ran it over the census and over the tree.SegmentsSplitAcrossTheHelperBoundaryresolves to a bareLite, reporting the whole tree for a read that names one directory, because the segment after the root is a variable. That is the precision trade CrossAppGuardCiGateTests cannot see a cross-app read spelled as a bare directory name, and there is a live one #3076 refused.ParitySourcelocates the repo root by probingDirectory.Existson a root name, and the widened arm reports that as a whole-tree read ofDarling, on the Lite arm, which has no exemption list and no backstop job.The only ways to green that are
Darling/**in thelitefilter — every Darling change builds and tests Lite — or one more carve-out on this arm. Precision is bought with structure, and the bill arrives again each time.Why not the other two options
Roslyn semantic analysis is the only thing that reaches the four member-crossing shapes, and it is the right answer if the guard should ever see them. It is also a significant new dependency in a test project, and that is not a call this change makes. Left as a recorded option in the doc comment.
Inverting the guard to assert on files a test actually opens at runtime does not work here, on two measurements. It is circular: knowing what a test opens requires running the suite, and the failure being guarded is a suite the filter skipped — a
Darling/PerformanceMonitor.Darling.Storage-only change fires neitherlitenordarling's Lite entries, so the observation would never run on the change that made a read unreachable. And instrumenting the reads means routing ~1,040 direct BCL file-API call sites across the two test trees (445File.ReadAllText, 351File.Exists, 150Directory.Exists, 84 enumerations) through one helper, where every site missed is a silent hole in the same failure direction.Nothing is unguarded, and that is now measured from both sides
Every unseen instance resolves to a tree the gating filter already reaches. The Darling arm's land under the
darlingfilter's derivedLite/**/*.csentry, or under theLite.TeststreeDarlingTestsBackstoppedalready exempts; the Lite arm's two land under the Viewer and Service trees thelitefilter names outright. Confirmed independently by the widening run above: it added three references per arm andEveryCrossAppSourceRead_IsReachableByTheFilterThatGatesItsSuiteaccepted all six as reachable — only the false positive failed.build.ymlis unchanged. All threedarling:blocks (build,darling-pg,darling-linux) plus the unfiltereddarling-tree-guardswere read; no entry is needed, because no read is unreachable.What remains unfound, and where that is recorded
All seven shapes, in
TheCSharpMatchers_StillCannotSeeTheseSpellings, one text each, expected answer empty, asserted by equality with the shape name in the failure message. The census replaces three unnamed texts.Red-proof, per pin
Each mutation was compiled as its own copy — no
git checkout --restores, so no mutation can survive one — and every run assertsBuild succeededseparately from the result. Baseline:Total: 12, Failed: 0, Not Run: 0.Failed: 1— "the 'VariableBase' spelling is now visible to the C# matcher""Lite"→"Litte"inVariadicSegmentHelperFailed: 1—Not found: ""Lite""TerminalRootLiteraltoken →LiteTrees.AppDirFailed: 1—Not found: ""Lite""against a text spelling"Lite.Tests"VariadicSegmentHelper→VariableBaseFailed: 1—Expected: 7, Actual: 6Failed: 4— controlExpected: ["Lite/Analysis/NotSeen.cs"], Actual: []The greedy-group hazard does not apply: no regex changed. The wrong-tree floor hazard does not apply either — the population floors in
Checkare untouched, and this pin's population is its own literal array, floored by the distinctness count and the per-case token check.One self-reference caught in review of this change: a first draft of the
VariableBasecomment spelled the shape out as aPath.Combinecall with a quoted root in it, which under any widening of that arm would have harvested a whole-treeDarlingreference out of this file's own prose — on the arm with no exemption list. Measured, then reworded; the comment now says why it does not spell the call.Doc corrections
Three claims about #3076's measurement were wrong in the tree and are load-bearing, since they are where the "precision is bought with structure" argument is recorded:
Path.Combinereads the second arm resolves, and three name a tree in a spelling no arm reaches;The arm's answer on the Lite arm is still zero and still correct; only the reason given for it was wrong.
CONTRIBUTING.md
A short subsection under Testing Guidelines: the three spellings the guard sees, that anything else is a decision rather than a gap, and the one actionable consequence — spell the path one of those three ways at the site that reads it, because the invisible direction is silent by construction.
Verification
Lite.Testsisnet10.0-windowswithUseWPFand cannot run on macOS.Lite.Tests/Lite.Tests.csprojbuilds clean (0 errors, 9 pre-existing warnings, none in this file), and the actualCrossAppGuardCiGateTests.cswas compiled into a throwawaynet10.0xunit v3 console harness inside the gitignoredbin/and run there:Total: 12, Errors: 0, Failed: 0, Skipped: 0, Not Run: 0.[CallerFilePath]resolves to the real worktree path, soRepoRoot()scans the live tree and the MSBuild evaluation stage runs for real.