Resolve QuarantineTools repo root via git instead of a .git directory probe - #19157
Resolve QuarantineTools repo root via git instead of a .git directory probe#19157Adam Ratzman (adamint) wants to merge 8 commits into
Conversation
QuarantineTools located the repository root by walking up the directory tree looking for a `.git` *directory*. In a linked git worktree `.git` is a regular file holding a `gitdir:` pointer, so the walk stepped over the worktree root and kept climbing. When the worktree was nested inside another checkout of the same repository, the walk terminated on the outer checkout's real `.git` directory and the tool rewrote test sources in the wrong tree. The failure was silent: the edit succeeded and the tool reported success with a repo-relative path, which looks identical to a correct run. Against a file with uncommitted work the stray edit would merge into it unnoticed. Ask `git rev-parse --show-toplevel` instead, and keep the directory walk as a fallback for when git is unavailable - now matching `.git` as a file or a directory so it stops at the worktree root too. Also refuse to run when the resolved root is not the working directory or one of its ancestors, which turns a silent wrong-tree write into an error. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19157Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19157" |
There was a problem hiding this comment.
Pull request overview
Resolves QuarantineTools repository roots through Git, preventing edits to an outer checkout from nested worktrees.
Changes:
- Adds Git-based root discovery with timeout and fallback.
- Adds ancestor safety validation.
- Adds linked-worktree regression tests.
Show a summary per file
| File | Description |
|---|---|
tools/QuarantineTools/QuarantineTools.csproj |
Exposes internals to tests. |
tools/QuarantineTools/Quarantine.cs |
Implements root discovery and validation. |
tests/QuarantineTools.Tests/RepoRootTests.cs |
Tests worktree and path scenarios. |
tests/QuarantineTools.Tests/QuarantineTools.Tests.csproj |
References the production tool. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
Two fixes in the repo-root resolution added by this PR. IsSameOrAncestorDirectory decided case sensitivity from the operating system, which is not a reliable proxy for the volume: macOS APFS can be formatted case-sensitive, Windows exposes a per-directory case-sensitivity flag that WSL sets, and Linux can mount case-insensitive volumes. Folding case on such a volume lets two genuinely different trees whose paths differ only by case satisfy the guard, which is the one outcome it exists to prevent. Comparing ordinally everywhere is not the answer either, because the Windows current directory keeps whatever casing the process was given while git canonicalizes --show-toplevel, so an ordinary run could then be refused over a meaningless difference. Probe the volume instead. TryGetGitTopLevelAsync linked the probe timeout to the caller's token and then caught every OperationCanceledException, so Ctrl+C was reported as an ordinary probe failure. FindRepoRootAsync then ran the fallback walk and ExecuteAsync enumerated the whole tests tree before cancellation was next observed. Re-throw caller cancellation; fall back only for the timeout. Both are covered by tests that fail without the change: the case test returns True instead of False, and the cancellation test throws nothing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (1)
tools/QuarantineTools/Quarantine.cs:517
- The linked-worktree tests always have
gitavailable, so they return from the primary probe and never exercise this fallback. Reverting onlyFile.Exists(gitPath)would therefore leave the suite green and reintroduce wrong-tree writes whenever git is unavailable. Add a regression test that forces the git probe to fail and verifies the nested worktree's.gitfile is selected instead of the outer checkout.
if (Directory.Exists(gitPath) || File.Exists(gitPath))
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Follow-up to the repo-root fix, from review feedback: - Return a distinct exit code (4) when refusing to edit a tree the caller is not standing in. 2 already means "tests folder not found", so reusing it made the two indistinguishable to a caller that branches on the exit code. - Resolve symlinks before giving up on the ancestor check. A Windows junction or `subst` drive lets the caller's directory read as `D:\src\aspire` while git reports `C:/src/aspire`, and refusing that would block a legitimate run. This runs only on the failure path, so a gap in it can only rescue a run that was already being refused, never block one that was about to succeed. Resolution proceeds one hop at a time because a link's stored target may be relative and may itself route through another link, and is depth-capped for cycles. - Extract the marker walk as FindRepoRootByMarker and test it directly. The git probe answers first, so the `.git`-as-a-file branch - the line the bug lived on - was not otherwise reachable from a test. - Assert the refusal message names both the resolved root and the working directory, so a later reword cannot quietly drop one. - Skip the git-dependent tests when git is absent instead of hard-failing, and make test cleanup non-throwing so a mid-setup failure cannot mask the real assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (1)
tools/QuarantineTools/Quarantine.cs:690
remainingDepthis decremented for every ordinary parent component, not only when following a link. A valid path with more than 40 components therefore exhausts the link budget before reaching a symlinked ancestor, leaves that ancestor unresolved, and makes the new wrong-tree guard reject a legitimate run. Keep the budget unchanged while walking parents; only decrement it when substituting a link target.
? Path.Combine(CanonicalizeCore(parent.FullName, remainingDepth - 1), info.Name)
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
The existing coverage pins the ExitCodeWrongTree constant, but a constant-pinning test cannot fail when the `return` changes, so the exit-code contract was effectively untested. This runs the built tool as a child process with a stale GIT_DIR/GIT_WORK_TREE pointing at a second repository and asserts the exit code, that the message names both paths, and that the other tree's file is byte identical afterwards. ExitCodeWrongTree becomes internal so the test binds to the contract instead of repeating the literal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (2)
tools/QuarantineTools/Quarantine.cs:775
- This probe still accepts a wrong tree when a case-sensitive parent contains two checkouts named with inverse casing (for example,
OUTERandouter). The flipped path exists because it is a distinct sibling, so this reports case-insensitive behavior;IsSameOrAncestorDirectorythen approvesOUTERas an ancestor ofouter/toolsand permits the cross-tree write. The new rejection test bypasses this probe by passingcaseSensitive: true. Distinguish an alternate lookup of the same directory from a distinct sibling, or conservatively reject this ambiguous case.
// A case-sensitive volume that happens to hold a real sibling differing only by case is read as
// case-insensitive here. That is acceptable: it only restores the behavior this guard had
// before the probe existed, and such a pair is not something a checkout layout produces.
return !Directory.Exists(Path.Combine(parent, flipped));
tools/QuarantineTools/Quarantine.cs:691
remainingDepthis intended to cap symlink hops, but this branch decrements it for every ordinary parent component. From a path deeper than 40 segments, canonicalization can stop before reaching a symlink near the root, leaving equivalent real/link spellings different and incorrectly returning exit code 4. Keep the counter unchanged while walking non-link parents; only decrement it when resolving a link.
return info.Parent is { } parent
? Path.Combine(CanonicalizeCore(parent.FullName, remainingDepth - 1), info.Name)
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (1)
tools/QuarantineTools/Quarantine.cs:602
Trim()removes valid whitespace from the repository path, not just git's line terminator. On Unix a checkout directory can end in a space, so this resolves a different path and the new guard incorrectly refuses a valid run. Remove only CR/LF terminators.
var trimmed = standardOutputTask.Result.Trim();
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (2)
tools/QuarantineTools/Quarantine.cs:693
remainingDepthis decremented for every ordinary parent component, not only when a link is followed. A path deeper than 40 components therefore exhausts the link budget before reaching a symlink near the root, leaving the two spellings uncanonicalized and incorrectly refusing a valid run. Preserve the budget while walking ordinary parents; only decrement it when resolving a link.
return info.Parent is { } parent
? Path.Combine(CanonicalizeCore(parent.FullName, remainingDepth - 1), info.Name)
: Path.TrimEndingDirectorySeparator(info.FullName);
tools/QuarantineTools/Quarantine.cs:821
- This new Windows-native case-sensitivity path is not exercised by CI:
QuarantineTools.Tests.csprojexplicitly disables both Windows and macOS runs, while the delegate-based test only verifies dispatch and the filesystem test runs on Linux. Enable platform runs for this project or move focused tests to platform-enabled projects so the P/Invoke behavior and macOS fallback/symlink behavior are covered.
using var handle = CreateFile(
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (3)
tools/QuarantineTools/Quarantine.cs:563
- Clearing these overrides makes
Tool_RefusesWithTheWrongTreeExitCode_AndLeavesTheOtherTreeUntouchedunable to reach the new guard. Its working directory is outside a repository, so afterGIT_DIR/GIT_WORK_TREEare removed,gitfails, the marker fallback returns null, andExecuteAsyncsubstitutes the current directory; the tool then exits with code 2 for the missingtestsfolder rather than code 4. The new test therefore fails. Either update the intended behavior/test to treat ignored overrides as a normal no-repository result, or detect and reject the conflicting ambient repository before running the cleaned probe.
ClearRepositoryLocationEnvironment(startInfo);
tools/QuarantineTools/Quarantine.cs:834
- This new Windows-only native path is not exercised by CI:
QuarantineTools.Tests.csprojexplicitly setsRunOnGithubActionsWindowstofalse, while the injectable test bypasses this P/Invoke. A bad entry point/signature or Native AOT interop regression would therefore ship undetected. Enable the existing filesystem probe test on Windows (or add equivalent Windows coverage in a project that runs there).
using var handle = CreateFile(
tools/QuarantineTools/Quarantine.cs:705
remainingDepthis decremented for every ordinary path component, not only when following a link. Consequently, a valid path deeper than 40 components stops canonicalizing its higher ancestors; if one of those ancestors is a symlink, equivalent paths remain different and the safety guard incorrectly refuses the run. Preserve the budget while walking normal parents soMaxLinkDepthactually limits only symlink traversals.
? Path.Combine(CanonicalizeCore(parent.FullName, remainingDepth - 1), info.Name)
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (4)
tools/QuarantineTools/Quarantine.cs:907
- The Git probe still accepts command-scoped configuration from the parent process.
GIT_CONFIG_COUNT/GIT_CONFIG_KEY_n/GIT_CONFIG_VALUE_n(andGIT_CONFIG_PARAMETERS) can injectcore.worktree, causingrev-parse --show-toplevelto return an outer checkout; because that path is an ancestor, the new guard accepts it and the original wrong-tree write recurs. Clear these config-injection variables as well and add the ancestor regression usingcore.worktree.
private static readonly string[] s_gitRepositoryLocationEnvironmentVariables =
[
"GIT_DIR",
"GIT_WORK_TREE",
"GIT_COMMON_DIR",
tools/QuarantineTools/Quarantine.cs:705
- The symlink-depth budget is also decremented while walking ordinary parent directories. For a valid path with more than 40 components below a symlink, canonicalization stops before reaching the link and the safety guard incorrectly exits with code 4. Parent traversal cannot cycle, so preserve the budget there and decrement it only when following a link.
? Path.Combine(CanonicalizeCore(parent.FullName, remainingDepth - 1), info.Name)
tools/QuarantineTools/Quarantine.cs:824
- This Windows-only metadata path and the macOS-sensitive fallback are not exercised by PR CI:
QuarantineTools.Tests.csproj:10-11explicitly disables both Windows and macOS, so the new safety-critical platform behavior is validated only on Linux. Enable focused Windows/macOS test legs (or move these tests to projects scheduled on those platforms) so failures in the P/Invoke and volume-casing logic are caught automatically.
private static bool? TryGetKnownDirectoryCaseSensitivity(string directory)
{
if (!OperatingSystem.IsWindows())
{
return null;
tools/QuarantineTools/Quarantine.cs:605
Trim()removes valid whitespace from the repository path, not just Git's line terminator. On Unix a checkout whose root directory ends in a space is valid, but this produces a nonexistent root and the guard refuses the run. Strip only\r/\nfrom the command output.
var trimmed = standardOutputTask.Result.Trim();
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Description
tools/QuarantineToolsresolved the repository root by walking up the directory tree looking for a.gitdirectory:In a linked git worktree,
.gitis a regular file containing agitdir: <path>pointer, not a directory. The walk therefore stepped over the worktree root and kept climbing. When the worktree was nested inside another checkout of the same repository, the walk terminated on the outer checkout's real.gitdirectory and the tool rewrote test sources in the wrong tree.The failure is silent. The edit succeeds, and the tool reports success with a repo-relative path, which is indistinguishable from a correct run:
Against a file with uncommitted work, the stray edit merges into it unnoticed. This was hit while quarantining the tests in #19153: the tool was run from a worktree nested under a main checkout and modified
tests/Aspire.Cli.Tests/Projects/ProcessGuestLauncherTests.csin the main checkout, which had unrelated uncommitted work.Nesting worktrees inside a checkout is common, and
AGENTS.mdencourages worktrees for parallel work, so the setup that triggers this is one the repo actively recommends.Fix
git rev-parse --show-toplevel. Git owns the definition of "working tree root" and handles linked worktrees, submodules, and symlinks correctly..gitas a file or a directory so it also stops at the worktree root.4) and names both the resolved root and the working directory, so a caller can tell it apart from the existing2(notestsfolder) and3(no matching test).The
gitprobe is bounded by a timeout and falls back to the walk rather than blocking, and both streams are drained concurrently so a full pipe buffer cannot deadlock it.Verification
FindRepoRootAsyncandIsSameOrAncestorDirectoryare nowinternalwithInternalsVisibleTo, andtests/QuarantineTools.Testspicks up aProjectReferenceto the tool so the tests exercise the real implementation rather than a copy. NewRepoRootTestsbuilds a real main checkout with a real nested worktree and covers root resolution from the worktree root, from a subdirectory of it, from the main checkout, and from outside any repository.Confirmed the new tests fail against the old logic. Reverting only the two changed lines produces:
Full project: 35/35 passing (27 pre-existing, 8 new).
End-to-end against a fixture with a worktree nested inside a main checkout, running the built tool and checking which tree was modified:
gitnot onPATHAlso verified with the native AOT publish (
PublishAot=true), since the fix shells out togit.The layers are covered independently rather than only through the git-backed fixtures:
.git-as-file worktree, with nogiton the path at all. That is the branch the bug actually lived on, and going through git would never reach it.GIT_DIR/GIT_WORK_TREEpointing at a different checkout, which is the realistic way git returns a root outside the caller's tree. It refuses, names both paths, and writes nothing.The mechanism in one line:
stat -f %HTreports a linked worktree's.gitasRegular Fileand a main checkout's asDirectory, and the old probe only accepted the latter.Notes
The ancestor guard on its own would not have caught the reported bug, because the outer checkout genuinely is an ancestor of a nested worktree. The git probe does the real work; the guard is a net for the
GIT_DIR/GIT_WORK_TREEcase.Canonicalization runs only after a plain comparison already failed, never before it. That ordering is deliberate: a bug in the canonicalizer can then only rescue a run that was about to be refused, and can never block a run that was about to succeed.
Filed separately from #19154 on purpose. That PR is an 18-line workflow-only change with its own fork-based before/after evidence, and the two bugs do not interact: the workflow checks out into a plain, non-nested workspace, where this root-resolution bug is unreachable. Keeping them apart lets the workflow fix merge on its own review.
Checklist
<remarks />and<code />elements on your triple slash comments?