Skip to content

Fixes #3063 - #3065

Merged
erikdarlingdata merged 10 commits into
devfrom
fix/3063-crossapp-guard-msbuild-population
Sep 6, 2026
Merged

erikdarlingdata merged 10 commits into
devfrom
fix/3063-crossapp-guard-msbuild-population

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 6, 2026 •

Copy link
Copy Markdown
Owner

CrossAppGuardCiGateTests exists to require that when one SKU's test project reads the other's source, that read is reachable by the CI path filter gating its suite. It enumerated *.cs and matched path string literals inside that C#, so it never opened a .csproj — and a cross-app read expressed as a linked compile was structurally invisible to it.

Not because the path spelling was unusual. Because the population was *.cs.

What was invisible

#3059 (#3052) shares one file between the SKUs with a linked compile rather than a copy:

<Compile Include="..\Darling\Darling.Tests\CSharpSourceWalker.cs" Link="CSharpSourceWalker.cs" />

That is a cross-app source read of exactly the kind this guard exists to require be filter-reachable. Neither existing matcher can see it, and the anchor is a red herring on the way to the real reason: an Include= path is relative (..\Darling\…), backslashed, and not repo-rooted, so otherApp[/\\] never fires — but it would not matter if it did, because project XML is not a *.cs file and was never read.

Nothing was unguarded, and this is not a coverage gap. #3059's reference is reachable by the lite filter's existing Darling/Darling.Tests/**/!(*.md) entry, and #3059 records why a darling-filter entry was rejected — naming Lite.Tests/** there would gate the live-PostgreSQL darling-pg job on all 298 of Lite's test files. What was missing is the guard's ability to know that. The next linked compile between the SKUs got no assertion at all.

The failure direction is what justifies the work. This guard fails toward not noticing: a missed reference is silent, and only a red test gets looked at. Same reasoning recorded in #3052 about the identical filter being loud when it drives an assertion and silent when it drives a number.

What changed

One file, Lite.Tests/CrossAppGuardCiGateTests.cs. No workflow, product or filter change.

A second population. Alongside *.cs, the scan now reads the project's own *.csproj files and any Directory.Build.props / Directory.Build.targets from the project directory up to and including the repo root. bin/ and obj/ stay excluded on both stages.

Two resolution bases, because MSBuild has two rules. An item's path (Include / Update / HintPath) resolves against the consuming project's directory — the whole reason $(MSBuildThisFileDirectory) exists — while an Import's Project resolves against the directory of the file containing it. Those coincide for a .csproj, which is its own consumer, and diverge for an imported build file, which is exactly where a cross-app Import would live. ReadMsBuildPaths takes the XML and both bases rather than a path, so the divergence is pinned through the shipped read and not through RepoRooted alone — the wrong base is a silent miss (the reference fails the anchor and vanishes), so a pin that cannot see a caller passing the wrong base is not a pin.

A sibling matcher, RepoRooted, rather than a second anchor. It resolves an MSBuild path attribute against the project directory and expresses it repo-rooted with forward slashes — the form the coverage logic already reasons about — so everything downstream of it is unchanged. Applying the otherApp anchor after normalisation is the whole point: a relative path names no app until it is resolved, and ..\Lite\… and ..\Darling\… are indistinguishable before that.

  • Include, Update, Project (on Import) and HintPath are read. Remove= deliberately is not: dropping an item from a glob is not a read of it.
  • HintPath is read in both MSBuild spellings, and the element may carry its own attributes. Condition is the usual one (a platform-specific hint path). Attributes are skipped by matching quoted values rather than by scanning to the next >, because an attribute value may legally contain a raw > — only < and & must be escaped — and a bracket-scanning matcher would end the tag inside the value. Both cases are pinned.
  • HintPath is read in both MSBuild spellings. It is item metadata, and metadata may be written as an attribute or as a child element — the element form being what Visual Studio emits for a legacy <Reference>, and the form a hand-written cross-app assembly reference would take. Include, Update and Import's Project are item operations rather than metadata and have no element spelling, so one element matcher covers the whole difference between the two grammars. Measured: HintPath appears in no project file in this repository (grep over every .csproj/.props/.targets finds it only in this test file), so nothing is live — but advertising a population wider than the one actually read is the shape of this very defect, one attribute over, so the code now honours the claim rather than the claim being narrowed.
  • An MSBuild wildcard is a real file set, unlike a glob in C# source where the pattern is the assertion (WatermarkPolicyTests pins filter entries, and those are excluded outright). So a wildcard reduces to the directory it enumerates and takes the existing directory-probe arm.
  • Paths resolving outside the repository are dropped — no path filter can name them.
  • $(MSBuildThisFileDirectory) and $(MSBuildProjectDirectory) are expanded. Anything still carrying an MSBuild expression is reported and fails the check, not dropped: a path this guard cannot read has to be visible, given which direction it fails in. Zero on dev today. The recognised forms are all three MSBuild has — a property or property function $(…), item metadata %(…), and an item list or transform @(…) — named in one place rather than accumulated as special cases, because an unrecognised expression falls through as a literal relative path, resolves to nothing, and vanishes silently. An unparenthesised % or @ in a filename stays readable, and that negative is pinned too.

A reference counts only when the path it resolved to spells it back. Directory.Exists answers according to the host's path rules, and Windows trims trailing periods from a path component — so a Darling segment followed by three dots resolves to Darling there and to nothing on macOS or Linux. This guard runs on Windows in CI and gets verified on macOS, and the divergence was not hypothetical: see the CI round below. OnDisk round-trips the resolution, so the answer is identical on all three platforms.

ImportedBuildFiles takes the nearest file of each name and stops, which is what Microsoft.Common.props actually does — it probes upward for one Directory.Build.props and does not chain, and the two names probe independently. Collecting every one above the project would attribute an item group to a project that never imports it, and a guard whose findings are not trustworthy stops being read. A nearest file that deliberately chains to its parent does so through an Import whose path is a property function, which the unevaluable check above reports as a failure rather than ignoring — so the chain is loud, not silently uncollected.

Every stage is floored, and the build-file stage is floored on where it LOOKED. A widened collector that opens nothing passes exactly as the narrow one did — that is the defect committed by its own fix, and it applies to each half separately.

  • The project-file half: ThePopulationReachesTheProjectFiles_AndNotOnlyTheCSharp requires the specific manifest (Lite.Tests/Lite.Tests.csproj, Darling/Darling.Tests/Darling.Tests.csproj) to be in the population rather than merely counting files, and asserts no bin//obj/ copy got in. That last one is non-vacuous: the local verification host below puts a real .csproj under Lite.Tests/bin/.
  • The imported-build-file half cannot take a count floor. Nothing in this repository imports a Directory.Build.props, so requiring a non-empty found set would red on a clean tree — and it still would not distinguish probed nowhere from probed and found none, which is the only distinction that matters. So the walk records every location it looked at, and the pin floors that: the two names in every directory from the project up to and including the repo root, re-derived in the test from the project path rather than read back off the walk. What it opened is pinned as exactly the nearest existing probe of each name — an equality that holds at zero, so a clean tree passes because it was asked, not because it wasn't. The probe and found lists are both in the failure message.

TheMsBuildMatchers_ReadBothSpellings pins both grammars against the shipped matchers rather than a retyped copy, including that Remove= stays out.

TheOnDiskProbe_AnswersTheSameOnEveryHost pins the host-independence directly, including the exact three-dot path that reached CI.

TheMsBuildPathNormaliser_AgreesWithKnownAnswers self-validates the normaliser against known answers, the same way TheGlobMatcher_AgreesWithKnownAnswers already does and for the same reason — a normaliser that resolved to the wrong repo-rooted path would match no filter pattern, find no file on disk, be dropped, and every coverage check would pass having seen nothing.

Failures now name every file that named the reference. The two populations can name the same path — a linked compile and a pin that reads the linked file both do — and reporting only the first would say it came from the C# and leave the project entry looking unread.

Directory.Build.props / .targets: established, not assumed

There is neither file anywhere in this repository today (only Directory.Packages.props, which carries PackageVersion items and no paths). They are in the population anyway, and nothing asserts they exist.

The reason they belong is mechanical rather than hypothetical: MSBuild imports them into every project without being named, and an ItemGroup in one can legally carry a cross-app Compile Include. For a file outside the project cone there is no duplicate-item conflict with the SDK's later default glob, so it works — the same invisible shape as a .csproj entry, in a file no .csproj scan would open. Proven both ways below.

One correctness note: MSBuild resolves a relative item path in an imported file against the consuming project's directory, not the imported file's own. These resolve against the project root accordingly, which is why a repo-root Directory.Build.props is read correctly rather than one directory off.

Correcting the framing this came from

#3059's adjacent finding described the detector as keying on Lite/ and Darling/ "while Lite.Tests is a sibling of Lite". Reading the source, the sibling relationship is not the operative cause of #3059's invisibility — the *.cs-only population is, and #3059's own reference lives at Darling/Darling.Tests/…, which is inside Darling/ and would have anchored fine. Fixed here as a population problem.

The sibling asymmetry is real, though, and it is not empty — see below. It is a separate fix that forces a decision this PR is not the place to take.

Verification

Lite.Tests targets net10.0-windows and cannot run on macOS. The real CrossAppGuardCiGateTests.cs was compiled into a throwaway net10.0 xunit v3 host named Lite.Tests, staged inside the gitignored Lite.Tests/bin/ so [CallerFilePath]'s walk-up resolves against the real repo root and every pin reads real repo content. The .csproj was passed to dotnet build explicitly.

Lite.Tests also builds clean with -p:EnableWindowsTargeting=true; the warning profile is byte-identical to dev's (9 warnings, none in this file).

Green: 9 of 9. Total: 9, Errors: 0, Failed: 0, Skipped: 0, Not Run: 0. MTP prints no per-test Passed line, so each was additionally run by name on its own and reported Total: 1, Failed: 0, Not Run: 0 — a silent grep against a green check cannot tell a pass from a test that never ran. On CI the whole Lite suite moved 3363 -> 3367 across these commits with Failed: 0, Skipped: 0, Not Run: 0, which is the count-based confirmation that the four added tests really executed there.

The load-bearing proof, on the real tree

#3059 merged into dev at 01:24:59Z (13061c4eb) while this was being built, so this is measured against the live reference rather than a synthetic. origin/dev is merged in here, and the Compile Include is present at Lite.Tests/Lite.Tests.csproj:29.

The reference is covered by the lite filter, so a detecting guard and a blind one both go green on it. The discrimination is to remove the covering lite entry (Darling/Darling.Tests/**/!(*.md)) and see which collector notices:

On merged dev, lite entry removed Result
this PR's collector fails, naming Darling/Darling.Tests/CSharpSourceWalker.cs (named in Lite.Tests/CrossAppGuardCiGateTests.cs, Lite.Tests/Lite.Tests.csproj)
dev's collector, same tree fails on other references, and never mentions CSharpSourceWalker.cs — the defect

build.yml was restored and the working tree verified clean afterwards.

The CI round, and what it actually caught

The first push went red on build: Total: 3365, Failed: 1, with

Lite.Tests reads Darling/... (directory) (named in Lite.Tests/CrossAppGuardCiGateTests.cs)
  but the 'lite' filter does not reach it (probe path: Darling/.../CoverageProbe.cs)

It was not the MSBuild normalisation. Two things say so, both from that same run. The origin list names only Lite.Tests/CrossAppGuardCiGateTests.cs and not Lite.Tests/Lite.Tests.csproj — this PR records every file that named a reference precisely so that reads unambiguously. And TheMsBuildPathNormaliser_AgreesWithKnownAnswers, which asserts Assert.Equal("Darling/Darling.Tests/CSharpSourceWalker.cs", RepoRooted(…, @"..\Darling\Darling.Tests\CSharpSourceWalker.cs", "Darling")) — the resolved form, exactly, not merely that something was found — passed on Windows in that run. RepoRooted never string-trims ..; it is Path.GetFullPath against the project directory followed by a repo-relative rebase, and the ellipsis is not a spelling it can produce.

What produced it was a comment in this file. I had written an illustrative three-dot path inside a quoted string while explaining why the wildcard case uses a fictional directory, and the pre-existing C# string-literal matcher — unchanged from dev — matched it, exactly as it matches "Darling/Some/Path.cs" on dev today. Those existing decoys are inert only because they resolve to nothing. This one resolved on Windows and not on macOS, which is why every local run was green.

So it is a real defect in a real direction, and the fix is the category rather than the instance: the comment no longer spells a path, and OnDisk now requires the round-trip, so no host's path normalisation can invent a reference again. No build.yml filter entry was added — that would have encoded the malformed path into CI config permanently.

Red-proofed twenty-five more ways

Each mutation fails a different named assertion, and each was reverted and re-run green.

Mutation Fails
.csproj removed from the population both floors: "the MSBuild stage opened no project files under Lite.Tests", and "did not open Lite.Tests/Lite.Tests.csproj … Opened: []"
normalisation resolves against the repo root instead of the project directory TheMsBuildPathNormaliser_AgreesWithKnownAnswers (Expected: "Darling/Darling.Tests/CSharpSourceWalker.cs", Actual: null), and the coverage failure loses the .csproj origin
lite filter loses Darling/PerformanceMonitor.Darling.Viewer/**/!(*.md) the coverage check, naming 10 .cs-sourced references with their origin files — the existing C# detection is intact, not traded away
a repo-root Directory.Build.props carrying <None Include="..\Darling\Darling.Tests\CSharpSourceWalkerTests.cs" /> detected and attributed to Directory.Build.props
…with the walk-up cut to the project directory only the same reference disappears — the walk-up range is load-bearing
the same include spelled $(RepoRoot)Darling\… the unevaluable check, printing Directory.Build.props: $(RepoRoot)Darling\… rather than silently missing it
the same include spelled $(MSBuildThisFileDirectory)Darling\… detected — the expansion works
..\..\Darling\X.cs (out of tree), xunit.v3 (a package id) Assert.Null in the normaliser self-check
a wildcard over the other app reduced to its directory by the self-check's known answer
the OnDisk round-trip removed TheOnDiskProbe_AnswersTheSameOnEveryHost — Assert.Null on the dot-segment case, which is the macOS-runnable twin of the Windows trailing-period case
a repo-root Directory.Build.props with nothing nearer detected, attributed to Directory.Build.props
the same, plus a nearer Lite.Tests/Directory.Build.props the root file is no longer collected — nearest-wins holds, and the .csproj detection is untouched in the same run
the nearer file chain-imports its parent via $([MSBuild]::GetPathOfFileAbove(...)) the unevaluable check, printing Lite.Tests/Directory.Build.props: $([MSBuild]::GetPathOfFileAbove(…)) — loud rather than silently uncollected
the build-file walk records no probe (the stage goes dead) two assertions: the probe-location equality, and the Check floor printing probed nowhere for Lite.Tests … Probed: [], opened: []
the walk stops at the project directory instead of the repo root the probe-location equality — a short reach is not a legal answer
nearest-wins reverted to collect-everything, with a root and a nearer Directory.Build.props both present the nearest-existing-per-name equality, at index 0. With nearest-wins intact the same tree is green
a real cross-app <HintPath> child element injected into Lite.Tests.csproj detected and attributed to Lite.Tests/Lite.Tests.csproj alone — the MSBuild stage, with no C# origin
the element matcher dropped from the shipped matcher set TheMsBuildMatchers_ReadBothSpellings, and the injected <HintPath> reference disappears from the found set
a real <HintPath Condition="'$(Platform)'=='x64'">..\Darling\…</HintPath> injected into Lite.Tests.csproj detected and attributed to Lite.Tests/Lite.Tests.csproj alone
the element matcher narrowed back to the attribute-free form the spelling pin, and the Condition-bearing reference vanishes from the found set
a real <Compile Include="@(SharedSources)" /> injected into Lite.Tests.csproj the unevaluable check, reporting Lite.Tests/Lite.Tests.csproj: @(SharedSources)
@( dropped from the recognised expression forms TheEvaluabilityCheck_CoversEveryMsBuildExpressionForm, and that reference disappears from the report entirely
a real <Import Project="Darling\Darling.Tests\Shared.targets" /> in a repo-root Directory.Build.props detected and attributed to Directory.Build.props
one resolution base for everything — the consuming project TheTwoResolutionBases_AreBothUsedWhereTheyDiverge, and the real Import above vanishes silently
one resolution base for everything — the file's own directory the same pin, from the other side: the item climbs out of the repository

Not verified

  • Nothing ran on Windows or Linux. CI's build, Darling Linux build and Darling PostgreSQL tests are the arbiters for the full suites; only this one class was executed locally. Darling.Tests was not run at all — nothing here changes it.
  • The rest of Lite.Tests was not executed. This class is self-contained; it reads the repo and build.yml and touches no product code.
  • No live store, monitored server or workflow run was involved. Test-only change.
  • The Directory.Build.props / .targets arms are proven against a synthesised file, because none exists in the repo. That is the whole reason they are in the population and unfloored.
  • The Windows trailing-period behaviour itself was not re-executed on Windows after the fix; the round-trip is pinned by its macOS-runnable twin (a dot segment, which resolves on every host and still is not the path it was written as) and by the reference no longer appearing. CI's build is the arbiter.

Adjacent finding, deliberately not fixed here

Darling.Tests reads two files under Lite.Tests/ today, and the guard cannot see either — via the anchor, not the population.

  • Darling/Darling.Tests/QueryHighDopStaleMaxDopParityTests.cs:479 — const string twin = "Lite.Tests/QueryHighDopStaleMaxDopParityTests.cs";, then File.Exists + parse.
  • Darling/Darling.Tests/ParameterSensitivityFiringSignatureParityTests.cs:336 — the same shape.

For that arm otherApp is Lite, and the anchor is Lite[/\\]. Lite.Tests/ does not match Lite/, so both reads are outside the found set. This one is live, unlike the population gap this PR closes, which had no instance on dev until #3059 merged.

It is not fixed here because fixing it forces a decision rather than a repair: the darling filter names Lite/**/*.cs and Lite/**/*.xaml but not Lite.Tests/**, so detecting these makes the guard red, and the two ways out are the darling-filter entry #3059 already weighed and rejected, or a new "covered by darling-tree-guards" exemption concept — which, since that job runs the Darling suite whenever the darling filter leaves it skipped, would make the whole Darling.Tests arm of this guard vacuous. Both are design calls, and neither is a population fix.

Nothing is unguarded by it either, for that same reason: a Lite.Tests-only change leaves darling-tests reporting skipped, and darling-tree-guards then runs the whole Darling suite. Filed as #3067 with the three options and the measurement that should choose between them, rather than smuggled in here.

CHANGELOG entry text

Not applied — every lane appends to the same [Unreleased] block.

- The cross-app CI-gate guard now reads project files as well as C# source, so a cross-app source read
  expressed as an MSBuild `Compile Include` is seen rather than silently missed. Both scan stages are
  floored by name, because a widened scan that opens no project files would pass exactly as the narrow
  one did. (#3063)

The collector enumerated *.cs and matched path string literals inside it, so a cross-app source read
expressed as an MSBuild Compile Include was structurally invisible - the file holding it was never
opened. The scan now reads the project's own .csproj files and the Directory.Build.props /
Directory.Build.targets MSBuild imports into it, normalising a relative Include= back to the
repo-rooted form the coverage logic already reasons about.

Both stages are floored by name, because a widened collector that opens no project files passes
exactly as the narrow one did.
Comment thread Lite.Tests/CrossAppGuardCiGateTests.cs Outdated
@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Reviewed. This is a test-only change confined to Lite.Tests/CrossAppGuardCiGateTests.cs — no T-SQL, no product code, so the T-SQL style conventions don't apply and there's no Lite/Darling parity drift to flag (the guard intentionally lives in one place and symmetrically scans both Lite.Tests and Darling/Darling.Tests, per the class's own doc comments on why it isn't duplicated into Darling.Tests).

I traced the core logic by hand against the actual repo state:

  • RepoRooted's relative-path normalization, wildcard-to-directory reduction, and repo-boundary check all behave correctly on the real cases in the tree today, including the linked compile at Lite.Tests/Lite.Tests.csproj:29 (..\Darling\Darling.Tests\CSharpSourceWalker.cs), which is exactly the reference CrossAppGuardCiGateTests scans only *.cs, so a cross-app reference expressed as an MSBuild Compile Include is invisible to it #3063 was filed over and is confirmed reachable via the lite filter's Darling/Darling.Tests/**/!(*.md) entry.
  • The otherApp + "/" prefix check correctly avoids false-positives on sibling-but-distinct names (e.g. Lite.Tests doesn't spuriously match an otherApp of Lite), and correctly excludes same-app references and non-path values (package ids) via how they resolve relative to the project directory rather than by substring matching.
  • Resolve's on-disk existence check correctly filters out the synthetic/non-existent paths used in the self-validation test (e.g. Darling/NoSuchArea/Fixtures) so those don't leak into the real coverage check.
  • Both new population floors (CSharpFiles > 0, ProjectFiles.Count > 0) and the UnevaluablePaths loud-failure path look correctly wired to avoid the exact "widened scan reaching nothing still passes" defect the PR is fixing.

One non-blocking note left inline on ImportedBuildFiles: it collects every Directory.Build.props/.targets from the project directory up to the repo root, which is slightly more permissive than MSBuild's actual nearest-file-wins import resolution. Doesn't matter today since neither file exists in the repo, and it fails in the safe (loud) direction, so I'd call it a latent nuance rather than a bug.

No security concerns — this only reads files already inside the repo tree it's scoped to (no untrusted input, no network/process/secrets involvement).

…imported build file

Directory.Exists answers according to the host's path rules: Windows trims trailing periods from a
path component, so a Darling segment followed by three dots resolves to the Darling directory there
and to nothing on macOS or Linux. A reference now counts only when the path it resolved to spells it
back, so the answer is the same on every host. The three-dot path that reached CI came from an
illustrative string in a comment here, matched by the C# stage; the comment no longer spells one.

ImportedBuildFiles takes the nearest file of each name and stops, which is what
Microsoft.Common.props does. Collecting every one above the project would attribute an item group to
a project that never imports it. A nearest file that chains to its parent does so through an Import
whose path is a property function, which the unevaluable check reports rather than ignores.
Comment thread Lite.Tests/CrossAppGuardCiGateTests.cs
@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Reviewed the widened cross-app guard scan (Lite.Tests/CrossAppGuardCiGateTests.cs). This is test-infrastructure-only (no T-SQL, no product code, no Lite/Darling parity concern since the scan logic is symmetric and shared for both apps via the same Check()/Scan() calls).

Traced through the new code carefully — RepoRooted's wildcard reduction, the MSBuild property expansion ($(MSBuildThisFileDirectory)/$(MSBuildProjectDirectory)) against the correct directory per file vs. per consuming project, the ImportedBuildFiles nearest-wins-per-name walk, and the host-independent OnDisk round-trip check for the Windows trailing-dot quirk. All of it checks out against the actual Lite.Tests.csproj/Darling.Tests.csproj on dev (no $(...) property paths in real Include= attributes today, so UnevaluablePaths stays empty; no Directory.Build.props/.targets exist, consistent with the PR description). The added self-validation tests (TheMsBuildPathNormaliser_AgreesWithKnownAnswers, TheOnDiskProbe_AnswersTheSameOnEveryHost, ThePopulationReachesTheProjectFiles_AndNotOnlyTheCSharp) cover the discriminations the fix actually depends on.

No security surface here (pure local-repo path scanning over trusted checkout content, no external input) and no performance concern (a handful of .csproj/.cs files read per test run).

Left one non-blocking nitpick inline: CrossAppScan.ImportedBuildFiles is populated but never asserted on or surfaced in any message, unlike its sibling ProjectFiles.

The build files were collected and never asserted on, so that half of the widened population could
silently collect nothing while every test passed - the same shape the .csproj floor exists to
prevent for the other half. A count floor is the wrong instrument here: nothing in this repository
imports a Directory.Build.props, so requiring a non-empty found set would red on a clean tree and
still would not tell "probed nowhere" from "probed and found none".

So the walk now records every location it looked at, and the pin floors that: the two names in
every directory from the project up to and including the repo root, re-derived from the project
path rather than read back off the walk. What it opened is pinned as exactly the nearest existing
probe of each name, which holds at zero.
Comment thread Lite.Tests/CrossAppGuardCiGateTests.cs Outdated
@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Reviewed the diff (test-only change to Lite.Tests/CrossAppGuardCiGateTests.cs, no product code, no T-SQL). This isn't a Lite/Darling product-code change, so the usual parity rules don't apply directly — the guard has always lived in Lite.Tests alone and scans both directions from there, which is unchanged by this PR (the Darling.Tests mention of this class is just a comment referencing it, not a duplicate implementation).

Overall the widened MSBuild-aware population, the RepoRooted/OnDisk normalisation, and the "floor what was looked at, not just what was found" tests are careful and well-justified — the walk-up-to-repo-root logic, the nearest-wins-per-name build-file resolution, and the host-independence round-trip (Windows trailing-dot vs. macOS/Linux) all check out against the stated intent, and I traced the directory-chain math in ThePopulationReachesTheProjectFiles_AndNotOnlyTheCSharp for both Lite.Tests (single segment) and Darling/Darling.Tests (two segments) — both match what ImportedBuildFiles's parent walk actually produces.

Left one inline finding: HintPath is documented as one of the four attributes this scan reads, but the regex only matches the attribute-spelling (HintPath="..."), not the far more common child-element spelling VS actually emits (<HintPath>...</HintPath>). Not a live gap (no HintPath anywhere in this repo's .csproj files today), but it's the same "invisible until it's the next linked file" shape this PR exists to close, just under a different attribute name.

No security, SQL-injection, or performance concerns — this is local, test-only file/text scanning with no external input.

HintPath was in the documented attribute set while only the attribute spelling was matched. It is
item metadata, and MSBuild lets metadata be written as an attribute or as a child element - the
element form being what Visual Studio emits for a legacy Reference, and the form a hand-written
cross-app assembly reference would take. Advertising a population wider than the one actually read
is the shape of the defect this file was already being fixed for, one attribute over.

Include, Update and Import's Project are item operations rather than metadata and have no element
spelling, so one element matcher covers the difference between the two grammars. The pin is fed the
shipped matchers rather than a retyped copy.
Comment thread Lite.Tests/CrossAppGuardCiGateTests.cs
@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Reviewed. This PR only touches Lite.Tests/CrossAppGuardCiGateTests.cs — pure C#/MSBuild test-infrastructure, no T-SQL and no product code, so the Lite/Darling app-parity and SQL-style conventions don't apply here directly; the file itself is the shared parity guard for both suites and treats Lite/Darling symmetrically throughout (Scan, RepoRooted, Check are all parameterized by otherApp), so there's no drift between how the two projects are scanned.

Traced the new MSBuild-scanning path (Scan → ReadMsBuildPaths → RepoRooted → Resolve/OnDisk) against the two real .csproj files it will actually run against (Lite.Tests/Lite.Tests.csproj, Darling/Darling.Tests/Darling.Tests.csproj):

  • No PackageReference/ProjectReference/None/Compile entry in either file produces a spurious cross-app match — the one genuine cross-app read (Lite.Tests' linked Compile Include="..\Darling\Darling.Tests\CSharpSourceWalker.cs") resolves correctly to Darling/Darling.Tests/CSharpSourceWalker.cs and is already covered by the existing lite filter's Darling/Darling.Tests/**/!(*.md) entry, matching the PR description.
  • No Directory.Build.props/.targets exist in the repo today, matching the "reported, not floored" / probe-list-floor design for that stage, and the re-derived probe-path pin in ThePopulationReachesTheProjectFiles_AndNotOnlyTheCSharp matches what ImportedBuildFiles's directory walk actually produces for both Lite.Tests (1 segment) and Darling/Darling.Tests (2 segments).
  • The Windows-trailing-dot round-trip fix in OnDisk (patch 2) and the HintPath element-vs-attribute matcher (patch 4) both look correct and are exercised by dedicated self-validation tests.
  • No injection/security surface — everything here is local repo-file scanning inside a test assembly, not shipped product code, with no untrusted input.

One minor nit left as an inline comment: ImportedBuildFiles's XML doc comment closes </summary> mid-comment and then has a trailing <para> sibling with no enclosing tag, so that paragraph will likely be invisible in IDE hover tooltips even though it compiles fine.

No correctness, parity, security, or performance issues found otherwise — this is an unusually well self-validated change (matcher behavior, path normalization, and population coverage are all pinned with dedicated known-answer tests).

A trailing <para> sibling of </summary> is well-formed XML that IDE tooltips do not render, so the
paragraph explaining why the walk returns both lists was invisible exactly where it is wanted.
Comment thread Lite.Tests/CrossAppGuardCiGateTests.cs Outdated
@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Reviewed. Scope is entirely Lite.Tests/CrossAppGuardCiGateTests.cs — a CI-gate test/tooling file, not T-SQL or app code, so the T-SQL style conventions and Lite/Darling app-parity concerns don't apply here directly (this class itself is the single shared authority scanning both Lite.Tests and Darling/Darling.Tests, by existing design predating this PR — no counterpart file needed).

Went through the widened MSBuild population, the RepoRooted normaliser, the nearest-wins Directory.Build.props/.targets walk, the host-independent OnDisk round-trip, and the probe-vs-found flooring logic. All of it holds up under close reading, and the edge cases called out in the PR description (trailing-period Windows resolution, nearest-file-wins, unevaluable $()/%() properties, wildcard-as-directory reduction) are each correctly handled and match their stated intent.

One inline finding: the new HintPath element matcher (<HintPath\s*>...</HintPath\s*>) only matches a bare element with no attributes, so a <HintPath Condition="..."> (a common real pattern for platform-specific hint paths) would still be silently invisible to the scan — the same defect shape this PR was written to close, just one attribute-on-the-element-form over. No live instance in the repo today, so nothing breaks now; left as a narrow suggestion rather than a blocker.

No correctness, security, or Lite/Darling parity issues found beyond that.

The element matcher demanded the closing angle bracket immediately after the tag name, so a
HintPath with a Condition on it - the usual reason to write one, for a platform-specific hint path -
matched nothing and the reference was dropped. Attributes are skipped by matching quoted values
rather than by scanning to the next angle bracket, because an attribute value may legally contain a
raw > and that would end the tag inside it. Both cases are pinned.
Comment thread Lite.Tests/CrossAppGuardCiGateTests.cs Outdated
@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Reviewed. This is a single-file change to Lite.Tests/CrossAppGuardCiGateTests.cs — a C#/MSBuild CI-gate meta-test, not T-SQL, so the T-SQL style conventions don't apply here. No Lite/Darling parity concern: this test already lives in exactly one place and symmetrically scans both Lite.Tests and Darling/Darling.Tests (no duplicate copy needed on the Darling side), and no other files changed.

Traced the logic in detail (regex matchers, RepoRooted path normalization, the ImportedBuildFiles upward walk, the OnDisk round-trip check) against the actual repo state:

  • Verified the motivating gap is real and fixed: Lite.Tests/Lite.Tests.csproj's <Compile Include="..\Darling\Darling.Tests\CSharpSourceWalker.cs" Link="..." /> is exactly the kind of reference the old *.cs-only scan couldn't see, and the new MSBuild-XML scan now opens .csproj files and finds it via RepoRooted.
  • Confirmed that reference is already covered by the lite filter's existing Darling/Darling.Tests/**/!(*.md) entry in build.yml, so the new stages shouldn't flip this test red on dev.
  • Confirmed there are currently no Directory.Build.props/.targets anywhere in the repo and no $(...)-property paths or HintPath usage in either test project's .csproj, so the new "probed but not floored on count" and "unevaluable path" assertions are dormant but correctly non-firing on a clean tree.
  • Walked through the trickier bits (wildcard-to-directory reduction in RepoRooted, the Windows trailing-dot/Directory.Exists fix via OnDisk's round-trip spelling check, the attribute-vs-element HintPath matcher handling an embedded > in a Condition value) — all correctly handled and specifically pinned in the new self-validation tests.

One minor, low-confidence note left as an inline comment: the "unevaluable MSBuild property" detection only recognizes $( / %(, not the @(ItemName) item-batch syntax — a theoretical, currently-inert blind spot given the guard's own "fail loud" design goal. Not blocking.

No security, correctness, or performance concerns otherwise — this only reads repo-local files during test execution.

The evaluability check looked for a property or item-metadata reference and missed an item list or
transform - @(SharedSources), or @(SharedSources->'...'). Neither carries $( or %( anywhere, so such
a value fell through as a literal relative path, resolved to nothing, and vanished from the found
set: this guard's own defect class, in the arm written to prevent it.

The three forms MSBuild evaluates are named in one place and pinned, including that an unparenthesised
% or @ in a filename is still readable.
Comment thread Lite.Tests/CrossAppGuardCiGateTests.cs Outdated
@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Reviewed the diff — this PR only touches Lite.Tests/CrossAppGuardCiGateTests.cs (test infrastructure, no T-SQL and no product code in either app), so the SQL style/parity conventions in CONTRIBUTING.md don't really apply here, and there's no Lite/Darling app-code drift to flag: the guard logic is inherently symmetric (it's parameterized over both LiteTestsDir/Darling and DarlingTestsDir/Lite), and the tests exercise both directions.

The commit series is unusually well self-tested (each widening of the scan population is pinned with known-answer tests, floored so a stage that silently stops scanning fails loudly). I traced the resolution logic in detail and left one inline finding worth a look: Import's Project= attribute is resolved against the wrong base directory when it's read out of a found Directory.Build.props/.targets rather than the project's own .csproj — MSBuild resolves a relative Import path relative to the file containing it, not the consuming project (unlike Include/Update/HintPath, which the code correctly resolves against the consuming project). Not live today since the repo has no Directory.Build.props/.targets yet, but it would silently reintroduce the exact "not noticing" bug class this file exists to close, the day one gets added with a plain relative cross-app Import.

No security, injection, or SQL/T-SQL concerns — this is a build-time/test-time file-system and regex scan over the repo's own tracked files, no untrusted input.

… project

MSBuild uses two different rules and the scan used one. An item's path - Include, Update, HintPath -
resolves against the consuming project's directory, which is why $(MSBuildThisFileDirectory) has to
exist at all. An Import's Project resolves against the directory of the file containing it. Those
coincide for a .csproj, which is its own consumer, and diverge for an imported build file, which is
exactly where a cross-app Import would live: a repo-root Directory.Build.props importing
Darling\Shared.targets was being resolved under Lite.Tests, failing the anchor and vanishing.

ReadMsBuildPaths now takes the XML and both bases rather than a path, so the divergence is pinned
through the shipped read instead of through RepoRooted alone. The pin is two-sided in both
directions at once: neither single base produces both answers.
@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Reviewed the diff (single file: Lite.Tests/CrossAppGuardCiGateTests.cs).

Scope check: Test-only change to a CI-gate guard; no T-SQL, no product code, no Lite/Darling parity drift — the guard is a single shared file parameterized over both Lite.Tests and Darling.Tests, so there's no second copy to fall out of sync.

Correctness: Traced the new code paths by hand against the actual repo state:

  • RepoRooted's two-base resolution (projectDir for items vs thisFileDir for Import's Project) matches MSBuild's real behavior, and the wildcard-reduction arithmetic (LastIndexOf('/', wildcard), cut <= 0 bailout) checks out for both a bare wildcard and a Darling/*.cs-shaped one.
  • Verified against the real .csproj files: Lite.Tests.csproj:29's linked CSharpSourceWalker.cs compile resolves to Darling/Darling.Tests/CSharpSourceWalker.cs (covered by the lite filter), and none of the other Include=/ProjectReference entries in either test project's .csproj resolve to the other app, so this doesn't introduce a new CI failure on merge.
  • ImportedBuildFiles's upward walk and the BuildFileNames probe-list reconstruction in ThePopulationReachesTheProjectFiles_AndNotOnlyTheCSharp agree exactly for both a one-level (Lite.Tests) and two-level (Darling/Darling.Tests) project path.
  • MsBuildPathAttribute/MsBuildPathElement regex matches against the TheMsBuildMatchers_ReadBothSpellings fixture come out in the same document order the test expects, including the Remove= exclusion and the raw > inside a quoted Condition value.
  • No repo file currently has an Include=/HintPath-bearing XML comment that the (inherently regex-based, not real-XML-parser-based) matchers would false-positive on.

Didn't find a correctness, security, or parity issue to flag. The regex-over-XML approach is fragile in the abstract (comments, CDATA, entity-encoded values could confuse it), but that's consistent with the rest of this file's existing string-matching design and isn't exercised by anything currently in the tree.

@erikdarlingdata erikdarlingdata mentioned this pull request Sep 6, 2026
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