Skip to content

Fixes #3067 - #3075

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/3067-crossapp-guard-sibling-test-anchor
Sep 6, 2026
Merged

erikdarlingdata merged 1 commit into
devfrom
fix/3067-crossapp-guard-sibling-test-anchor

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 6, 2026 •

Copy link
Copy Markdown
Owner

CrossAppGuardCiGateTests anchored every cross-app path on the app directory, and Lite.Tests is a sibling of Lite rather than a child. So Darling.Tests' reads of Lite's test project matched nothing: the arm opened 454 C# files, found 15 references, and not one of them came from the other SKU's test tree. Nothing distinguished that from health.

The issue names two such reads. There are four, all confirmed against a fresh dev (987c005):

Reference Named in
Lite.Tests/QueryHighDopStaleMaxDopParityTests.cs Darling/Darling.Tests/QueryHighDopStaleMaxDopParityTests.cs:479
Lite.Tests/ParameterSensitivityFiringSignatureParityTests.cs Darling/Darling.Tests/ParameterSensitivityFiringSignatureParityTests.cs:336
Lite.Tests/AnalysisPassTokenThreadingTests.cs Darling/Darling.Tests/CommentFilterAdoptionTests.cs:99
Lite.Tests/LiteSidebarDotRendersTheCardStatusTests.cs Darling/Darling.Tests/CommentFilterAdoptionTests.cs:106

The first two are the twin constants the issue describes. The other two are keys of CommentFilterAdoptionTests' bounded set — <project>/<filename> labels which, for a file sitting directly under Lite.Tests, happen to spell a real repo-rooted path. That class does open both files, so requiring reachability of them is not a false positive; it is the strongest instance, because it enumerates the whole tree.

The reverse arm is genuinely fine, and that is measured rather than reasoned: with otherApp = Darling, Darling/Darling.Tests sits inside Darling/, and the Lite arm finds 7 references under it (including #3059's linked compile) — all reachable via the lite filter's Darling/Darling.Tests/**/!(*.md) entry. Widening that arm's anchor to the SKU pair leaves its found set byte-identical at 22 references, because the second root is subsumed by the first.

The exit taken: (c)

The anchor now matches every root a SKU owns, on both populations (the C# matchers and RepoRooted), derived from the two arms' own constants rather than spelled a third time. The four references it now sees are not reachable by darling, and they are exempted with a stated bound rather than named in a filter.

build.yml is not touched by this PR at all, so the inline prohibition at lines 48-52 is not engaged.

The measurement, and a premise that does not hold

The issue frames the (a)-vs-(c) decision as "how often would a Lite.Tests-only change be forced to stand up the darling-pg cluster". The answer is zero, and it cannot be otherwise: darling-pg declares its own darling filter (build.yml:672-678) naming only Darling/**/!(*.md), .github/workflows/build.yml and .github/workflows/nightly.yml. darling-linux does the same (build.yml:853-859). There are three independent darling: filter blocks in that file with three different pattern sets, and the one the guard reads — the first, at line 162 — gates only the build job's steps. No entry added there can start a TimescaleDB cluster. That claim is also written into CommentFilterAdoptionTests' doc comment as the reason #3059 declined the entry; this PR corrects it.

So here is the countable version, over the 100 pull requests merged to dev before this branch (#2919 through #3073, file lists from the API):

touched Lite.Tests/** at all 26
touched Lite.Tests/** without Darling/** 6
of those 6, how many already ran the Darling suite 6 (5 lit darling via Lite/**/*.cs, core or root; 1 ran it in darling-tree-guards)
newly lit by adding Lite.Tests/**/!(*.md) to the build job's darling filter 1
newly lit by naming the four files exactly 0
darling-pg cluster stands up today 89
darling-pg after adding Lite.Tests/** to the build job's filter 89 (unchanged — separate filter block)

6% is not the large fraction the issue's decision rule was looking for, so cost is not what decides this. Coverage is: (a) buys zero additional guard execution. All 6 of those PRs already ran both twin guards. Adding the entry would change only where the suite ran, for 1 PR in 100, and would pay for it by making a Lite test-file edit compile the Darling Viewer and publish both Darling artifacts — the build job's darling gate also controls Build Darling (line 315), Publish Darling Service (377) and Publish Darling Viewer (381). Changing what CI compiles and publishes in order to satisfy a test asserting a property CI already satisfies by another means is the wrong direction; the test is what should learn about the other means.

Against (b). A blanket exemption is defensible on the facts — darling-tree-guards runs the full Darling.Tests suite whenever the darling filter did not fire, so filter-reachability of anything Darling.Tests reads is unconditionally redundant, not just these four. What sinks it is that the redundancy rests on an unpinned property. TheDarlingSuite_RunsWhereTheAreaFiltersDoNotReach asserted the invocation as a substring, which a -class filter appended to it would still satisfy. Under (b) the arm would be gone, that narrowing would be silent, and nothing would notice. (c) keeps the arm live for the next reference and turns the bound into something checked.

Against (a) beyond the numbers: two of the four references are whole-tree enumerations, and the only filter entry that reaches a tree enumeration is Lite.Tests/** — the widest possible one, for the category build.yml's own comment (lines 965-980) assigns to darling-tree-guards. Of the two mechanisms that file uses for markdown reachability, this is the darling-side one: exclude and lean on the backstop, not the lite-side one of naming the file because a pin parses it.

What changed

  • SkuTrees — one SKU as the named pair of trees it owns. Named rather than positional because two different things are read off it and reading the wrong one is silent both ways. Roots is sorted longest-first; that changes no answer today (a required separator already rejects Lite.Tests/X.cs for the Lite root, and .NET's alternation backtracks) and exists so a rewrite to a non-backtracking matcher cannot silently start letting the shorter root absorb the token under test.
  • CSharpPaths(text, other) — the C# stage extracted from Scan so the pin is fed the shipped matcher, the way MsBuildPaths already is.
  • RepoRooted — the same widening on the MSBuild side, which had the identical gap: a linked compile of ..\..\Lite.Tests\X.cs out of Darling/Darling.Tests normalised correctly and was then thrown away.
  • An anti-vacuity floor per arm: at least one reference must be found under the other SKU's test tree. Floored on that tree rather than on the reference total, because a total stays comfortably non-zero while a whole tree drops out of it, which is exactly how this went unnoticed.
  • DarlingTestsBackstopped — the four entries, each with its bound, compared for set equality against what the filter could not reach, and honoured only while WholeTreeBackstopIsIntact(yaml) holds.
  • TheDarlingSuite_RunsWhereTheAreaFiltersDoNotReach — the run assertion is now whole-line equality, and it also asserts the predicate the exemptions are gated on agrees, so the two cannot drift.
  • CommentFilterAdoptionTests' doc comment — corrected: the darling-pg claim, the job's name (darling-tree-guards, not whole-tree-guards), a frozen file count removed, and the note that CrossAppGuardCiGateTests "cannot see this reference either way", which is no longer true.

Red-proof

Run on macOS by compiling the actual .cs under test into a throwaway net10.0 xunit v3 harness staged inside the gitignored Lite.Tests/bin/. Every run deletes the dll, builds, asserts the build succeeded and the dll was recreated as a step distinct from running, prints its mtime, then runs — so a failed build cannot leave a stale binary printing a clean summary. Baseline before the change: Total: 9, Failed: 0, Not Run: 0. After: Total: 10 (the new pin, visible as a counted delta), Failed: 0, Not Run: 0, Skipped: 0.

Mutations were applied only after committing, and every run verifies the tree restored clean and that the mutation actually changed a file.

Mutation Claim under test Result
Roots = { appDir } — revert the anchor the widening is what makes the sibling read visible 3 FAIL. The known-answer set at index 0; the floor with the #3067 diagnosis and the reference count dropping 19 → 15 in its own message; the MSBuild sibling answer as null
drop the separator from the C# anchor the widening did not degrade into a prefix match 2 FAIL. Known-answer set at index 3 (Lite.Tests/Fixtures/SystemHealth → Lite.Tests), plus a newly unreachable Lite.Tests (directory)
floor read off AppDir instead of TestsDir, alone — PASSES. A mutation the assertion does not read, so not a valid red-proof — recorded because it is the trap
same, plus the app-only anchor the floor has to be aimed at the TEST tree floor goes silent on live #3067; caught only by the staleness half (4 FAIL). This is why the pair is named, not indexed
add a fifth allow-list key for a read nobody has staleness 1 FAIL, naming the bogus key
retarget a real twin constant in Darling.Tests source staleness against real source, not a synthetic key 1 FAIL, naming the entry to delete
add a new quoted Lite.Tests/… read to a Darling.Tests file non-vacuity: a new unlisted read reds 1 FAIL, "the 'darling' filter does not reach it"
pass backstopped: null the exemptions are load-bearing, i.e. the widening really does red the guard 4 FAIL — the issue's premise, reproduced
append -- -class "…" to the tree-guards run line the exemptions lapse when their bound lapses 2 FAIL. All four exemptions revoked, and the whole-line assertion. On that same yaml the old substring form measured True and the new whole-line form False — the cheapest way that pin passed while the thing it guards was broken, made red
RepoRooted root check → AppDir only MSBuild-side sibling anchor 1 FAIL
RepoRooted root check → StartsWith(root), no separator MSBuild-side root boundary 1 FAIL, Lite.TestsExtra/Nope.cs no longer null

Darling.Tests cannot run on macOS, so the two guards there that read Lite.Tests were compiled into a second harness and run: CommentFilterAdoptionTests + DocCommentHygieneTests, Total: 42, Failed: 0, Not Run: 0. That harness was itself red-proofed — inserting a .StartsWith("//") site into Lite.Tests/CrossAppGuardCiGateTests.cs made CommentFilterAdoptionTests fail naming exactly that file, so the clean run is a real pass rather than a sweep that read the wrong root.

Not fixed here

Darling/Darling.Tests/ControlPlaneReloadDurabilityTests.cs:346 enumerates Lite, Lite.Tests and PerformanceMonitor.Common as bare directory names with no separator. No anchor keyed on a root plus a separator can see those, before or after this change, and that is symmetric — the same shape hides a "Darling" read from the Lite arm. It is a third spelling of an invisible cross-app read, distinct from #3063's population and from this anchor, and widening to bare tokens would pull in every prose mention of Lite and Darling in the tree. The new known-answer pin records the limitation explicitly (bareDirectory is in the fixture and absent from the expected set) rather than leaving it implied. Filed as #3076.

Fixes #3067

…tree-guards backstop covers

CrossAppGuardCiGateTests anchored every cross-app path on the app directory, so
Darling.Tests' four reads of Lite.Tests were invisible: Lite.Tests is a sibling
of Lite, not a child. The anchor is now every root a SKU owns, on both
populations, and each arm is floored on finding at least one reference under the
other SKU's test tree.

The four references it now sees are not filter-reachable and are exempted rather
than named in the darling filter: that filter decides what gets compiled and
published, and darling-tree-guards already runs the whole Darling suite on
exactly the changes in question. The exemptions carry their bound and are
honoured only while that backstop is verifiably intact.
@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Review

Scope: this PR touches only CI test infrastructure (Lite.Tests/CrossAppGuardCiGateTests.cs, Darling/Darling.Tests/CommentFilterAdoptionTests.cs doc comment) — no T-SQL, no Lite/Darling application code, so the T-SQL style conventions and app-level parity concerns don't apply here. The change itself is about Lite/Darling test-suite parity guarding, so I focused on whether the new logic is actually sound rather than just self-consistent.

Traced through the new pieces by hand against the current repo state:

  • SkuTrees (app dir + tests dir, longest-root-first): verified the ordering claim — a required separator already prevents Lite from claiming Lite.Tests/X.cs (. isn't a separator), so Roots order is cosmetic today, as the comment says.
  • CSharpPaths / RepoRooted widening: walked the regex/anchor logic against the new pinned fixtures (TheCSharpMatchers_SeeASiblingTestProject_AndNothingMerelyPrefixedByIt, the new RepoRooted assertions) by hand — the separator-boundary requirement correctly rejects Lite.TestsExtra/ and LiteTests/ while accepting the genuine sibling reads. No false positives/negatives found.
  • DarlingTestsBackstopped + WholeTreeBackstopIsIntact: confirmed the two "NAMED READ" keys against the actual twin constants (Darling/Darling.Tests/QueryHighDopStaleMaxDopParityTests.cs:479, ParameterSensitivityFiringSignatureParityTests.cs:336), and confirmed BackstopRunLine matches .github/workflows/build.yml:1074 (the darling-tree-guards job) exactly, verbatim — not the build job's own invocation at line 341 or the darling-pg job's invocation at line 795 (both correctly excluded since WholeTreeBackstopIsIntact slices from the needs.build.outputs.darling-tests consumer onward).
  • Stale-exemption detection in Check(): the set-equality logic (unreachable-but-exempted vs. exempted-but-no-longer-needed) is correct in both directions and only activates for the Darling arm (backstopped: null for Lite), matching the asymmetry that only darling-tree-guards exists as a backstop.
  • CommentFilterAdoptionTests.cs doc comment: "the two Lite.Tests keys below" correctly refers to that file's own s_bounded dictionary (Lite.Tests/AnalysisPassTokenThreadingTests.cs, Lite.Tests/LiteSidebarDotRendersTheCardStatusTests.cs), not the 4-entry DarlingTestsBackstopped set — consistent, not a stale cross-reference.

No correctness bugs, no Lite/Darling parity drift, and no security concerns found (pure repo-local file/text scanning, no external input). The PR's own "red-proof" mutation-testing table is thorough and I didn't find a gap it missed — in particular the documented limitation (bare directory names with no separator, e.g. in ControlPlaneReloadDurabilityTests.cs) is real but explicitly disclosed as out of scope and filed separately, not silently swept under the rug.

Nothing blocking from this pass.

@erikdarlingdata
erikdarlingdata merged commit 0f888e9 into dev Sep 6, 2026
10 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/3067-crossapp-guard-sibling-test-anchor branch September 6, 2026 04:30
erikdarlingdata added a commit that referenced this pull request Sep 6, 2026
#3075 rewrote CommentFilterAdoptionTests' CLASS summary (the build.yml filter
paragraph). This branch edits s_bounded's own doc comment and the map below it, so
the two do not overlap and every count derived here is unaffected: 4 COLLECTS + 1
stated bound + 1 SQL + 1 demonstrator = 7 keys, re-counted after the merge rather
than assumed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant