Improve symlink cycle condition

[GangWang01]

Fixes #13792

Context

It uses String.Contains() to check if a path is a subdirectory of another one. In the case as the issue describes, expected files are skipped.

Changes Made

Improve the symlink cycle condition.

Testing

Added new one.

Notes

[Copilot]

Pull request overview

This PR addresses a false-positive in FileMatcher’s symlink cycle-detection guard that could silently skip valid symlinked directories during recursive globbing (as described in #13792), by replacing a substring-based check with a path-boundary-aware “subdirectory” check and adding regression coverage.

Changes:

• Replace BaseDirectory.Contains(linkTarget.FullName) with a subdirectory-style check to avoid prefix-collision false positives.
• Add a new unit test that reproduces the “project dir name is a prefix of symlink target name” scenario and asserts files are still discovered.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/Framework/Utilities/FileMatcher.cs	Updates the symlink cycle-detection condition to use a subdirectory check instead of substring matching.
src/Framework.UnitTests/FileMatcher_Tests.cs	Adds a regression test ensuring symlinked directories aren’t skipped due to prefix-collision in directory names.

[github-actions]

Expert Review — PR #13901: Improve Symlink cycle condition

#	Dimension	Verdict
4	Test Coverage	🟢 1 NIT

✅ 23/24 dimensions clean.

Analysis

The fix is correct. The root cause is properly identified and addressed:

• Old code: BaseDirectory.Contains(linkTarget.FullName) — plain substring match. When BaseDirectory = .../targetTest/mySymlink and linkTarget = .../target, the substring "target" is found inside "targetTest", triggering a false-positive cycle detection.
• New code: IsSubdirectoryOf(BaseDirectory, linkTarget.FullName) — validates directory boundaries. After confirming the prefix match, it verifies the character at position possibleParent.Length in the child path is a directory separator, correctly rejecting targetTest as a child of target.

Cycle detection still works for legitimate cycles (verified via the existing DoNotFollowRecursiveSymlinks test): when a symlink in /a/b/subfolder/link points to /a/b, IsSubdirectoryOf correctly finds a / separator at the boundary.

No ChangeWave needed — this restores intended behavior (files incorrectly skipped are now correctly found). No one should rely on incorrect cycle detection.

Cross-platform: IsSubdirectoryOf uses OrdinalIgnoreCase, which matches the existing call sites in FileMatcher and is appropriate for MSBuild's path handling semantics.

Performance: No concern — IsSubdirectoryOf is a simple StartsWith + boundary check, comparable cost to the replaced Contains.

Test Quality (NIT)

The regression test correctly reproduces the reported bug scenario. The assertion pattern (length check + foreach/ShouldContain) is valid, though Shouldly's fileMatches.ShouldBe(expectedFiles, ignoreOrder: true) would be more idiomatic — this is purely a style nit and the existing pattern matches the adjacent DoNotFollowRecursiveSymlinks test.

---

Good bug fix — correct, well-scoped, properly tested.

Generated by Expert Code Review (on open) for issue #13901 · ● 3.6M