Repository navigation
Fixes #3063 - #3065
Fixes #3063#3065
Conversation
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.
…d-msbuild-population
|
Reviewed. This is a test-only change confined to I traced the core logic by hand against the actual repo state:
One non-blocking note left inline on 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.
|
Reviewed the widened cross-app guard scan ( Traced through the new code carefully — No security surface here (pure local-repo path scanning over trusted checkout content, no external input) and no performance concern (a handful of Left one non-blocking nitpick inline: |
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.
|
Reviewed the diff (test-only change to Overall the widened MSBuild-aware population, the Left one inline finding: 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.
…d-msbuild-population
|
Reviewed. This PR only touches Traced the new MSBuild-scanning path (
One minor nit left as an inline comment: 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.
|
Reviewed. Scope is entirely Went through the widened MSBuild population, the One inline finding: the new 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.
|
Reviewed. This is a single-file change to Traced the logic in detail (regex matchers,
One minor, low-confidence note left as an inline comment: the "unevaluable MSBuild property" detection only recognizes 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.
|
Reviewed the diff — this PR only touches 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: 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.
|
Reviewed the diff (single file: 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 Correctness: Traced the new code paths by hand against the actual repo state:
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. |
CrossAppGuardCiGateTestsexists 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*.csand 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:
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, sootherApp[/\\]never fires — but it would not matter if it did, because project XML is not a*.csfile and was never read.Nothing was unguarded, and this is not a coverage gap. #3059's reference is reachable by the
litefilter's existingDarling/Darling.Tests/**/!(*.md)entry, and #3059 records why adarling-filter entry was rejected — namingLite.Tests/**there would gate the live-PostgreSQLdarling-pgjob 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*.csprojfiles and anyDirectory.Build.props/Directory.Build.targetsfrom the project directory up to and including the repo root.bin/andobj/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 anImport'sProjectresolves 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-appImportwould live.ReadMsBuildPathstakes the XML and both bases rather than a path, so the divergence is pinned through the shipped read and not throughRepoRootedalone — 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 theotherAppanchor 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(onImport) andHintPathare read.Remove=deliberately is not: dropping an item from a glob is not a read of it.HintPathis read in both MSBuild spellings, and the element may carry its own attributes.Conditionis 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.HintPathis 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,UpdateandImport'sProjectare item operations rather than metadata and have no element spelling, so one element matcher covers the whole difference between the two grammars. Measured:HintPathappears in no project file in this repository (grepover every.csproj/.props/.targetsfinds 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.WatermarkPolicyTestspins filter entries, and those are excluded outright). So a wildcard reduces to the directory it enumerates and takes the existing directory-probe arm.$(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 ondevtoday. 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.Existsanswers according to the host's path rules, and Windows trims trailing periods from a path component — so aDarlingsegment followed by three dots resolves toDarlingthere 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.OnDiskround-trips the resolution, so the answer is identical on all three platforms.ImportedBuildFilestakes the nearest file of each name and stops, which is whatMicrosoft.Common.propsactually does — it probes upward for oneDirectory.Build.propsand 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 anImportwhose 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.
ThePopulationReachesTheProjectFiles_AndNotOnlyTheCSharprequires 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 nobin//obj/copy got in. That last one is non-vacuous: the local verification host below puts a real.csprojunderLite.Tests/bin/.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_ReadBothSpellingspins both grammars against the shipped matchers rather than a retyped copy, including thatRemove=stays out.TheOnDiskProbe_AnswersTheSameOnEveryHostpins the host-independence directly, including the exact three-dot path that reached CI.TheMsBuildPathNormaliser_AgreesWithKnownAnswersself-validates the normaliser against known answers, the same wayTheGlobMatcher_AgreesWithKnownAnswersalready 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 assumedThere is neither file anywhere in this repository today (only
Directory.Packages.props, which carriesPackageVersionitems 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
ItemGroupin one can legally carry a cross-appCompile 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.csprojentry, in a file no.csprojscan 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.propsis read correctly rather than one directory off.Correcting the framing this came from
#3059's adjacent finding described the detector as keying on
Lite/andDarling/"whileLite.Testsis a sibling ofLite". 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 atDarling/Darling.Tests/…, which is insideDarling/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.Teststargetsnet10.0-windowsand cannot run on macOS. The realCrossAppGuardCiGateTests.cswas compiled into a throwawaynet10.0xunit v3 host namedLite.Tests, staged inside the gitignoredLite.Tests/bin/so[CallerFilePath]'s walk-up resolves against the real repo root and every pin reads real repo content. The.csprojwas passed todotnet buildexplicitly.Lite.Testsalso builds clean with-p:EnableWindowsTargeting=true; the warning profile is byte-identical todev'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-testPassedline, so each was additionally run by name on its own and reportedTotal: 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 moved3363 -> 3367across these commits withFailed: 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
devat 01:24:59Z (13061c4eb) while this was being built, so this is measured against the live reference rather than a synthetic.origin/devis merged in here, and theCompile Includeis present atLite.Tests/Lite.Tests.csproj:29.The reference is covered by the
litefilter, so a detecting guard and a blind one both go green on it. The discrimination is to remove the coveringliteentry (Darling/Darling.Tests/**/!(*.md)) and see which collector notices:dev,liteentry removedDarling/Darling.Tests/CSharpSourceWalker.cs(named inLite.Tests/CrossAppGuardCiGateTests.cs,Lite.Tests/Lite.Tests.csproj)dev's collector, same treeCSharpSourceWalker.cs— the defectbuild.ymlwas 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, withIt was not the MSBuild normalisation. Two things say so, both from that same run. The origin list names only
Lite.Tests/CrossAppGuardCiGateTests.csand notLite.Tests/Lite.Tests.csproj— this PR records every file that named a reference precisely so that reads unambiguously. AndTheMsBuildPathNormaliser_AgreesWithKnownAnswers, which assertsAssert.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.RepoRootednever string-trims..; it isPath.GetFullPathagainst 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"ondevtoday. 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
OnDisknow requires the round-trip, so no host's path normalisation can invent a reference again. Nobuild.ymlfilter 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.
.csprojremoved from the populationLite.Tests/Lite.Tests.csproj… Opened: []"TheMsBuildPathNormaliser_AgreesWithKnownAnswers(Expected: "Darling/Darling.Tests/CSharpSourceWalker.cs",Actual: null), and the coverage failure loses the.csprojoriginlitefilter losesDarling/PerformanceMonitor.Darling.Viewer/**/!(*.md).cs-sourced references with their origin files — the existing C# detection is intact, not traded awayDirectory.Build.propscarrying<None Include="..\Darling\Darling.Tests\CSharpSourceWalkerTests.cs" />Directory.Build.props$(RepoRoot)Darling\…Directory.Build.props: $(RepoRoot)Darling\…rather than silently missing it$(MSBuildThisFileDirectory)Darling\…..\..\Darling\X.cs(out of tree),xunit.v3(a package id)Assert.Nullin the normaliser self-checkOnDiskround-trip removedTheOnDiskProbe_AnswersTheSameOnEveryHost—Assert.Nullon the dot-segment case, which is the macOS-runnable twin of the Windows trailing-period caseDirectory.Build.propswith nothing nearerDirectory.Build.propsLite.Tests/Directory.Build.props.csprojdetection is untouched in the same run$([MSBuild]::GetPathOfFileAbove(...))Lite.Tests/Directory.Build.props: $([MSBuild]::GetPathOfFileAbove(…))— loud rather than silently uncollectedCheckfloor printingprobed nowhere for Lite.Tests … Probed: [], opened: []Directory.Build.propsboth present<HintPath>child element injected intoLite.Tests.csprojLite.Tests/Lite.Tests.csprojalone — the MSBuild stage, with no C# originTheMsBuildMatchers_ReadBothSpellings, and the injected<HintPath>reference disappears from the found set<HintPath Condition="'$(Platform)'=='x64'">..\Darling\…</HintPath>injected intoLite.Tests.csprojLite.Tests/Lite.Tests.csprojalone<Compile Include="@(SharedSources)" />injected intoLite.Tests.csprojLite.Tests/Lite.Tests.csproj: @(SharedSources)@(dropped from the recognised expression formsTheEvaluabilityCheck_CoversEveryMsBuildExpressionForm, and that reference disappears from the report entirely<Import Project="Darling\Darling.Tests\Shared.targets" />in a repo-rootDirectory.Build.propsDirectory.Build.propsTheTwoResolutionBases_AreBothUsedWhereTheyDiverge, and the realImportabove vanishes silentlyNot verified
build,Darling Linux buildandDarling PostgreSQL testsare the arbiters for the full suites; only this one class was executed locally.Darling.Testswas not run at all — nothing here changes it.Lite.Testswas not executed. This class is self-contained; it reads the repo andbuild.ymland touches no product code.Directory.Build.props/.targetsarms are proven against a synthesised file, because none exists in the repo. That is the whole reason they are in the population and unfloored.buildis the arbiter.Adjacent finding, deliberately not fixed here
Darling.Testsreads two files underLite.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";, thenFile.Exists+ parse.Darling/Darling.Tests/ParameterSensitivityFiringSignatureParityTests.cs:336— the same shape.For that arm
otherAppisLite, and the anchor isLite[/\\].Lite.Tests/does not matchLite/, so both reads are outside the found set. This one is live, unlike the population gap this PR closes, which had no instance ondevuntil #3059 merged.It is not fixed here because fixing it forces a decision rather than a repair: the
darlingfilter namesLite/**/*.csandLite/**/*.xamlbut notLite.Tests/**, so detecting these makes the guard red, and the two ways out are thedarling-filter entry #3059 already weighed and rejected, or a new "covered bydarling-tree-guards" exemption concept — which, since that job runs the Darling suite whenever thedarlingfilter leaves it skipped, would make the wholeDarling.Testsarm 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 leavesdarling-testsreportingskipped, anddarling-tree-guardsthen 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.