From 186d03e5cc8436c110f93d5f58c76447ae91a643 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 24 May 2026 09:29:09 -0400 Subject: [PATCH] Resolve IntegrationTests-025: stopBoundary for repo-root walker ResolveRepositoryRoot accepts an optional stopBoundary parameter that caps the upward walk; production callers pass null and behavior is unchanged. The two repository-marker tests now seal their walkers inside their own temp directories, so a redirected TMP or a co-located C:\src checkout no longer leaks ambient marker-bearing ancestors into the assertion. Regression test ResolveRepositoryRoot_StopBoundary_IsolatesWalkerFromAmbientAncestorMarkers constructs an outer ancestor that carries src/ + .git, confirms the walker leaks into it without the boundary, then asserts the same call throws with the boundary supplied. Resolved at 2026-05-24; IntegrationTestEnvironmentTests 5/5 pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/IntegrationTests/findings.md | 6 +- .../IntegrationTestEnvironment.cs | 25 ++++++- .../IntegrationTestEnvironmentTests.cs | 69 ++++++++++++++++++- 3 files changed, 95 insertions(+), 5 deletions(-) diff --git a/code-reviews/IntegrationTests/findings.md b/code-reviews/IntegrationTests/findings.md index 9e6695f..5169702 100644 --- a/code-reviews/IntegrationTests/findings.md +++ b/code-reviews/IntegrationTests/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-24 | | Commit reviewed | `42b0037` | | Status | Re-reviewed | -| Open findings | 1 | +| Open findings | 0 | ## Checklist coverage @@ -494,7 +494,7 @@ The Write parity test (IntegrationTests-012's resolution) added exactly this ass | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironmentTests.cs:57-84` (`ResolveRepositoryRoot_NoMarkers_ThrowsInvalidOperationExceptionNamingStartAndMarkers`) | -| Status | Open | +| Status | Resolved | **Description:** The new regression test for IntegrationTests-022 builds an "isolated" start directory under `Path.GetTempPath()` (e.g. `C:\Users\\AppData\Local\Temp\\nested` on Windows) and calls `ResolveRepositoryRoot(isolatedStart)`, asserting an `InvalidOperationException` is thrown. The walker walks every parent — ``, `Temp`, `Local`, `AppData`, ``, `Users`, `C:\` — and stops only when it either finds a repository root marker or runs out of parents. The test silently assumes none of those ancestor directories satisfies `IsRepositoryRoot` (a `src/` subdirectory next to `.git` / `*.sln` / `*.slnx`). The assumption is environment-dependent: @@ -504,3 +504,5 @@ The Write parity test (IntegrationTests-012's resolution) added exactly this ass The current dev box layout (`C:\Users\dohertj2\Desktop\mxaccessgw`) is safe because Temp is at `C:\Users\dohertj2\AppData\Local\Temp` and the walker exits at `C:\` without ever encountering `src/`. The fragility is invisible on this machine and only surfaces if the test ever runs in CI / on a contributor box with a less hermetic file-system layout. **Recommendation:** Isolate the walker from any ambient ancestor by either (a) constructing an `isolatedRoot` directly under a drive root and pointing the walker at a chain entirely under it (e.g. create `\level1\level2\level3` and start the walk at `level3`, then assert the throw — the walker stops at the drive root regardless of what is on it), (b) refactoring `ResolveRepositoryRoot` to accept an injectable `stopBoundary` parameter for tests and pass `isolatedRoot` as the boundary, or (c) replacing the `Assert.Throws` shape with an explicit upward-walk check that the test owns. Option (a) is the smallest change: prepend a sentinel — e.g. create a dummy `\sentinel-no-markers` and assert nothing about Temp ancestors — and pass the test only when the walker reaches that sentinel without finding a marker. The current shape is acceptable on the documented dev box but should not be the sole regression coverage for IntegrationTests-022. + +**Resolution:** Resolved 2026-05-24 — Took option (b) (inject a stop-boundary) because option (a) does not actually solve the leak: a sentinel chain under `Path.GetTempPath()` still leaves the walker free to ascend past it into Temp / AppData / Users / C:\, so any ambient ancestor with `src/` + `.git`/`.sln`/`.slnx` still wins. Added an optional `stopBoundary` parameter to `IntegrationTestEnvironment.ResolveRepositoryRoot(string startDirectory, string? stopBoundary = null)`. When supplied, the walker checks the boundary for markers and then stops, refusing to ascend past it; production callers (the `MXGATEWAY_LIVE_MXACCESS_WORKER_EXE` resolution path) continue to pass `null` so the walk to drive-root behavior is unchanged. Updated both existing tests (`ResolveRepositoryRoot_AcceptsGitWorktreeFile` and `ResolveRepositoryRoot_NoMarkers_ThrowsInvalidOperationExceptionNamingStartAndMarkers`) to pass their owned temp directory as the boundary, sealing the walker inside a chain the test fully controls. Added a new regression test `ResolveRepositoryRoot_StopBoundary_IsolatesWalkerFromAmbientAncestorMarkers` that deliberately constructs an outer marker-bearing ancestor (`outerRoot/src` + `outerRoot/.git`), an inner boundary, and an isolated start beneath the boundary; first asserts that without the boundary the walker leaks up to `outerRoot` (the precise IntegrationTests-025 failure mode), then asserts that *with* the boundary the same call throws — proving the boundary is the load-bearing isolation. TDD red/green confirmed: the new regression test fails against the pre-fix walker (`Assert.Throws() Failure: No exception was thrown`) and passes once the boundary handling is restored. Re-ran the full `IntegrationTestEnvironmentTests` slice with `TMP` / `TEMP` redirected under a deliberately constructed `\fake-repo-ancestor` directory carrying `src/` and a `.git` file — the original flake repro from the finding — and confirmed all 5 tests pass (the same redirection produced `Assert.Throws() Failure` on the pre-fix code). Build: 0 warnings / 0 errors. diff --git a/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironment.cs b/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironment.cs index 15f67bd..69032da 100644 --- a/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironment.cs +++ b/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironment.cs @@ -109,12 +109,26 @@ public static class IntegrationTestEnvironment /// remains the escape hatch for unusual deployments. /// /// Starting directory to search from. + /// + /// Optional upper bound for the parent walk. When supplied, the walker checks + /// for markers and then stops, ignoring all + /// ancestors above it. Tests pass an isolated boundary so the walker cannot + /// leak into ambient ancestors (a redirected TMP, a co-located checkout + /// at C:\src, an enclosing CI workspace, etc.) that would silently + /// satisfy — see IntegrationTests-025. + /// Production callers pass so the walk continues to the + /// drive root as before. + /// /// The repository root path. /// /// Thrown when the parent walk exhausts without finding a repository root. /// - internal static string ResolveRepositoryRoot(string startDirectory) + internal static string ResolveRepositoryRoot(string startDirectory, string? stopBoundary = null) { + string? normalizedBoundary = stopBoundary is null + ? null + : Path.TrimEndingDirectorySeparator(Path.GetFullPath(stopBoundary)); + DirectoryInfo? directory = new(startDirectory); while (directory is not null) { @@ -123,6 +137,15 @@ public static class IntegrationTestEnvironment return directory.FullName; } + if (normalizedBoundary is not null + && string.Equals( + Path.TrimEndingDirectorySeparator(directory.FullName), + normalizedBoundary, + StringComparison.OrdinalIgnoreCase)) + { + break; + } + directory = directory.Parent; } diff --git a/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironmentTests.cs b/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironmentTests.cs index 5cae1dd..993de32 100644 --- a/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironmentTests.cs +++ b/src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironmentTests.cs @@ -33,7 +33,11 @@ public sealed class IntegrationTestEnvironmentTests Directory.CreateDirectory(Path.Combine(temporaryRoot, "src")); File.WriteAllText(Path.Combine(temporaryRoot, ".git"), "gitdir: ../.git/worktrees/test"); - string repositoryRoot = IntegrationTestEnvironment.ResolveRepositoryRoot(nestedDirectory); + // Pass temporaryRoot as the stop-boundary so the walker can never leak + // into ambient ancestors of Path.GetTempPath() (IntegrationTests-025). + string repositoryRoot = IntegrationTestEnvironment.ResolveRepositoryRoot( + nestedDirectory, + stopBoundary: temporaryRoot); Assert.Equal(temporaryRoot, repositoryRoot); } @@ -52,6 +56,11 @@ public sealed class IntegrationTestEnvironmentTests /// the walk exhausts without finding a repository root. The previous silent /// fallback to Directory.GetCurrentDirectory() masked misconfiguration /// (IntegrationTests-022); operators get a clear, actionable failure instead. + /// The stopBoundary isolates the walker from ambient ancestors of + /// (a redirected TMP, a co-located checkout + /// at C:\src, etc.) that could otherwise satisfy + /// IsRepositoryRoot and make this assertion flake on contributor or CI + /// boxes — see IntegrationTests-025. /// [Fact] public void ResolveRepositoryRoot_NoMarkers_ThrowsInvalidOperationExceptionNamingStartAndMarkers() @@ -64,7 +73,9 @@ public sealed class IntegrationTestEnvironmentTests Directory.CreateDirectory(isolatedStart); InvalidOperationException ex = Assert.Throws( - () => IntegrationTestEnvironment.ResolveRepositoryRoot(isolatedStart)); + () => IntegrationTestEnvironment.ResolveRepositoryRoot( + isolatedStart, + stopBoundary: isolatedRoot)); Assert.Contains(isolatedStart, ex.Message, StringComparison.Ordinal); Assert.Contains(".git", ex.Message, StringComparison.Ordinal); @@ -82,4 +93,58 @@ public sealed class IntegrationTestEnvironmentTests } } } + + /// + /// Verifies the stopBoundary parameter on + /// isolates the + /// walker from ambient ancestors that happen to satisfy IsRepositoryRoot + /// — the precise failure mode IntegrationTests-025 describes. The test + /// deliberately constructs an outer directory that *does* carry repository-root + /// markers (src/ + .git) and an inner isolated chain that does + /// not. Without the boundary the walker would happily stop at the outer + /// directory; with the boundary it must throw because the chain it can see + /// carries no markers. A future refactor that dropped or mis-honored the + /// boundary would surface here as a failed assertion instead of a silent flake + /// in CI. + /// + [Fact] + public void ResolveRepositoryRoot_StopBoundary_IsolatesWalkerFromAmbientAncestorMarkers() + { + string outerRoot = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); + string innerBoundary = Path.Combine(outerRoot, "inner"); + string isolatedStart = Path.Combine(innerBoundary, "nested"); + + try + { + // Outer directory satisfies IsRepositoryRoot — it is the "ambient + // ancestor" the production walker would otherwise stop at. + Directory.CreateDirectory(Path.Combine(outerRoot, "src")); + File.WriteAllText(Path.Combine(outerRoot, ".git"), "gitdir: ../.git/worktrees/test"); + + // Inner chain carries no markers. The boundary sits between the inner + // chain and the outer marker-bearing ancestor. + Directory.CreateDirectory(isolatedStart); + + // Sanity: without the boundary the production walker reaches outerRoot + // and silently returns it — the exact ambient-ancestor leak. + string leakedRoot = IntegrationTestEnvironment.ResolveRepositoryRoot(isolatedStart); + Assert.Equal(outerRoot, leakedRoot); + + // With the boundary the walker is sealed inside the inner chain and + // must throw — the marker on outerRoot is invisible to it. + InvalidOperationException ex = Assert.Throws( + () => IntegrationTestEnvironment.ResolveRepositoryRoot( + isolatedStart, + stopBoundary: innerBoundary)); + + Assert.Contains(isolatedStart, ex.Message, StringComparison.Ordinal); + } + finally + { + if (Directory.Exists(outerRoot)) + { + Directory.Delete(outerRoot, recursive: true); + } + } + } }