Add pipeline-scoped working directories

[thomhurst]

Closes #3770

Summary

• add a pipeline-scoped working directory used by command and file contexts
• add built-in pipeline-project and Git-root resolvers
• remove template CWD mutation and its 53-line helper

Validation

• ModularPipelines.slnx Release build: 0 warnings, 0 errors
• PipelineWorkingDirectoryTests: 3/3 passed
• PipelineBuilderRegistrationTests: 16/16 passed
• targeted formatter verification passed

The focused test project currently needs the unrelated stale RunReportTests initializers for removed PrintLogo/PrintResults properties omitted locally to compile; those lines were restored and are not part of this PR.

[thomhurst]

CI diagnosis: all six failures fan out from Fast fail. The template pipeline itself succeeds, then the smoke test fails test ! -d "$RUNNER_TEMP/template-caller/artifacts" because existing CI auto run-report output still resolves relative to the process CWD after this PR removes the template's global CWD mutation. This is the sibling run-report anchoring problem tracked and implemented by #3840, not a command/file-context defect in this PR. Avoiding duplicate/conflicting run-report changes here; refresh this branch from main after #3840 merges, then rerun.

[thomhurst]

Refreshed onto current main at ccea314 after the prior Fast-fail artifact-upload failure. Current-head validation: PipelineWorkingDirectoryTests 3/3; PipelineBuilderRegistrationTests 16/16; core Release build 0 warnings / 0 errors. Please re-review current head.

[claude]

Code review

Nice cleanup — this replaces the template's 53-line PipelineProjectDirectory + Environment.CurrentDirectory mutation hack with a proper PipelineWorkingDirectory abstraction that's injected through DI and consistently threaded through Command, FilesContext, FileSystemContext, and EnvironmentContext. The new PipelineWorkingDirectoryTests do a good job of proving the process CWD is left untouched while pipeline-internal operations correctly scope to the configured directory.

One design point worth discussing before merge, plus one follow-up suggestion:

Pipeline.CreateBuilder(args) now silently changes its default working directory for all existing pipelines, not just the template

Pipeline.CreateBuilder (src/ModularPipelines/Pipeline.cs:34-38) now always populates PipelineBuilderOptions.WorkingDirectory via PipelineDirectory.TryFindPipelineProject(sourceFilePath), using the [CallerFilePath]-captured, compile-time directory of Program.cs. Previously (confirmed against origin/main), this overload just forwarded args and the working directory came purely from Directory.GetCurrentDirectory().

Since TryFindPipelineProject succeeds whenever Program.cs sits next to appsettings.json + a .csproj — which is true for essentially every generated pipeline project, including this repo's own src/ModularPipelines.Build — this is a behavioral change for every pre-existing pipeline that upgrades, not only for template-generated ones. If a pipeline binary/​dotnet run is ever invoked from a directory other than its own source directory (a wrapper script, a monorepo task runner, a CI step that cds elsewhere first), context.Environment.WorkingDirectory, relative file resolution, and command working directories will now silently point at the pipeline's source directory instead of the directory the process was actually launched from.

I can see this matches the ask in #3770 ("The template then shrinks to Pipeline.CreateBuilder(args) + AddModule calls"), so it's intentional — but it's a meaningful, essentially non-optional default-behavior change hidden inside an existing overload's default parameter (the only opt-out is switching to the PipelineBuilderOptions overload and leaving WorkingDirectory unset, since both PipelineBuilder constructors are internal). Worth calling out explicitly in release notes, and/or worth updating CreateBuilder's XML doc (which still just shows the old CWD-implying example) to describe the new resolution order, so existing consumers aren't surprised on upgrade.

Suggestion: the working-directory abstraction doesn't yet reach module caching/artifacts

ModuleCacheOptions.WorkingDirectory (src/ModularPipelines/Caching/ModuleCacheOptions.cs) still defaults to raw Directory.GetCurrentDirectory(), and ArtifactLifecycleManager's fallback does the same — neither is wired to the new PipelineWorkingDirectory singleton. This is pre-existing code the PR doesn't touch, so not a regression from this diff, but it does mean that once a pipeline sets PipelineBuilderOptions.WorkingDirectory to something other than the process CWD (exactly the scenario this PR's own test exercises), [CacheInputs] glob resolution and artifact snapshotting will resolve against a different base directory than context.Files/context.Cmd do. Might be worth a fast-follow to post-configure ModuleCacheOptions.WorkingDirectory from the same PipelineWorkingDirectory singleton so there's one consistent notion of "the pipeline's directory" everywhere, rather than two.

Everything else — the Command/FilesContext/FileSystemContext resolution changes, the PipelineDirectory static helper, and the DI wiring — looks correct and consistent; I didn't find any compile or logic issues in the diff itself.

[thomhurst]

Addressed the current-head review at 15e9687778: both Pipeline.CreateBuilder overloads now explicitly document the intentional working-directory behavior, resolution/fallback order, and how callers preserve process-CWD semantics. The cache/artifact suggestion remains outside #3770's command/file API scope (run-report anchoring is likewise a sibling issue).

Revalidation: PipelineWorkingDirectoryTests 3/3; exact external-temp template smoke exits 0, writes the publish DLL under the pipeline project, and leaves the caller artifact-free; core Release build 0 warnings/errors; format and diff checks clean.

@claude review
@codex review

[claude]

Code review (re-review at 15e9687778)

Re-reviewed the current head against the earlier review on this PR. Both points raised previously have been handled reasonably:

1. "Pipeline.CreateBuilder(args) silently changes the default working directory for existing pipelines" — now addressed. Both CreateBuilder overloads (src/ModularPipelines/Pipeline.cs:9-45) carry <remarks> explaining the resolution/fallback order and how to opt out (use the PipelineBuilderOptions overload and leave WorkingDirectory unset). I also confirmed this repo's own src/ModularPipelines.Build/Program.cs:20 uses Pipeline.CreateBuilder(args) and is invoked from its own source directory per CLAUDE.md, so the new caller-path inference is a no-op for it — no self-inflicted regression.
2. Module cache / artifact working-directory wiring — the author's response that this is out of scope for Give pipelines a first-class working-directory concept so templates stop mutating Environment.CurrentDirectory #3770 (a sibling concern, same as the run-report anchoring issue closed out in Stabilize run-report storage paths #3840) is reasonable; not a regression introduced by this diff, just a known gap worth a fast-follow.

Fresh pass over the current diff

Traced the PipelineWorkingDirectory singleton end-to-end: PipelineBuilder → DI registration (PipelineBuilder.cs:358) → Command, FilesContext, FileSystemContext, EnvironmentContext, EnvironmentDomainContext. Path resolution is consistently applied once per call site (no double-resolution, since ResolvePath is idempotent for already-absolute paths), and the new _configuration.SetBasePath(_environment.WorkingDirectory) correctly lets the template's AddJsonFile("appsettings.json") resolve without the old CWD mutation. I also double-checked the FirstNonEmpty(fallback, candidates...) precedence in CreateHostEnvironment (PipelineBuilder.cs:293-301) — despite Directory.GetCurrentDirectory() appearing textually first in the call, it's bound to the fallback parameter, so options.WorkingDirectory correctly takes priority; no bug there despite how it reads at a glance.

One minor, non-blocking observation: IEnvironmentContext.WorkingDirectory (EnvironmentContext.cs) and IEnvironmentDomainContext.WorkingDirectory (EnvironmentDomainContext.cs) are two separate properties on two separate context interfaces that now both just read PipelineWorkingDirectory.Path. That duplication predates this PR, but this PR touches both call sites to wire them independently to the same singleton rather than having one delegate to the other — a small consolidation opportunity (e.g. domain context sourcing from IEnvironmentContext) if either interface is touched again, not worth blocking on here.

No compile/logic issues found in the diff. Tests (PipelineWorkingDirectoryTests, updated PipelineBuilderRegistrationTests) look like they exercise the right invariants: process CWD left untouched, command/file/environment resolution scoped to the configured directory, and caller-path project inference. Good to merge from my read.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 15e9687778

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

@claude review

[thomhurst]

@codex review

[claude]

Code review (re-review at 48b417cc)

Re-reviewed the diff since the last review (15e9687778 → 48b417cc), which is a focused fix commit ("complete working directory scoping").

The main outstanding concern from earlier is now resolved architecturally, not just documented

Previously, Pipeline.CreateBuilder(args) silently inferred the working directory from the caller's [CallerFilePath]-captured source location for every pipeline, which was a behavioral change for existing consumers hidden inside an overload's default parameter. This commit splits that into two explicit methods (src/ModularPipelines/Pipeline.cs:9-51):

• CreateBuilder(string[]? args = null) — reverted to the original, pre-PR behavior: just forwards args, no source-path inference. This is what src/ModularPipelines.Build/Program.cs and every doc example already use, so it's a genuine no-op for existing consumers rather than a "trust the docstring" fix.
• CreateBuilderFromSource(args, [CallerFilePath] sourceFilePath) — the new opt-in method that does the project-directory inference. Only the source-generator template (src/ModularPipelines.Templates/templates/modularpipeline/Program.cs:9) uses it.

This is a better fix than the docs-only approach from the previous round: the two behaviors are now distinguished by method name instead of by an implicit default parameter, so a reader (or IntelliSense) sees the working-directory inference happening explicitly at the call site. There's also a new reflection-based test (PipelineWorkingDirectoryTests.CreateBuilderRetainsSingleArgumentBinarySignature) guarding the plain CreateBuilder(string[]) signature stays intact.

PipelineDirectory.TryFindPipelineProject now walks ancestors from the source directory

src/ModularPipelines/PipelineDirectory.cs:45-50 changed from checking only the exact source directory before falling back to AppContext.BaseDirectory, to walking all ancestors of the source directory first via FindAncestor. This is an intentional generalization to support pipeline entry points living in a nested folder (e.g. src/Pipeline/Program.cs), and is covered by the updated CreateBuilderRetainsSourceInferenceFromNestedDirectory test. Reasonable — no issue.

Checksum/Zip now correctly resolve relative paths through PipelineWorkingDirectory

Checksum.Md5 and Zip.ZipFolder/UnZipToFolder (src/ModularPipelines/Context/Checksum.cs, src/ModularPipelines/Context/Zip.cs) now resolve their path arguments via the injected PipelineWorkingDirectory before touching the filesystem, closing a gap where these two context APIs previously didn't participate in the new working-directory abstraction while Command/FilesContext/FileSystemContext/EnvironmentContext already did. The new PipelineWorkingDirectoryTests observation record now asserts checksum/zip/unzip paths resolve against the configured pipeline working directory, not just environment/file/command paths as before.

No compile or logic issues found in this diff. The fix commit is well-targeted at exactly the concern raised previously, and the design (explicit opt-in method vs. implicit default-parameter behavior change) is the right call — good to merge from my read.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 48b417cc26

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

Warning

Review limit reached

@thomhurst, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 8 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7824dddc-186e-47e4-98b9-d0d2b5852e05

📥 Commits

Reviewing files that changed from the base of the PR and between 173198a and cf8c27a.

📒 Files selected for processing (28)

• src/ModularPipelines.Templates/templates/modularpipeline/PipelineProjectDirectory.cs
• src/ModularPipelines.Templates/templates/modularpipeline/Program.cs
• src/ModularPipelines/Context/Checksum.cs
• src/ModularPipelines/Context/Command.cs
• src/ModularPipelines/Context/Domains/IEnvironmentDomainContext.cs
• src/ModularPipelines/Context/Domains/Implementations/EnvironmentDomainContext.cs
• src/ModularPipelines/Context/Domains/Implementations/FilesContext.cs
• src/ModularPipelines/Context/Downloader.cs
• src/ModularPipelines/Context/EnvironmentContext.cs
• src/ModularPipelines/Context/FileSystemContext.cs
• src/ModularPipelines/Context/IEnvironmentContext.cs
• src/ModularPipelines/Context/Zip.cs
• src/ModularPipelines/DependencyInjection/DependencyInjectionSetup.cs
• src/ModularPipelines/Engine/RunReportPathResolver.cs
• src/ModularPipelines/Pipeline.cs
• src/ModularPipelines/PipelineBuilder.cs
• src/ModularPipelines/PipelineBuilderOptions.cs
• src/ModularPipelines/PipelineDirectory.cs
• src/ModularPipelines/PipelineWorkingDirectory.cs
• src/ModularPipelines/Requirements/Require.cs
• test/ModularPipelines.UnitTests/Context/FilesContextTests.cs
• test/ModularPipelines.UnitTests/FileSystem/FileSystemContextTests.cs
• test/ModularPipelines.UnitTests/Helpers/ChecksumTests.cs
• test/ModularPipelines.UnitTests/Helpers/DownloaderTests.cs
• test/ModularPipelines.UnitTests/Helpers/ZipTests.cs
• test/ModularPipelines.UnitTests/Registration/PipelineBuilderRegistrationTests.cs
• test/ModularPipelines.UnitTests/Registration/PipelineWorkingDirectoryTests.cs
• test/ModularPipelines.UnitTests/Requirements/RequireFactoryTests.cs

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Code review (re-review at 9c1cbed2)

This is a follow-up to my three previous review rounds on this PR. The prior rounds thoroughly covered the working-directory abstraction (PipelineWorkingDirectory, Command/FilesContext/FileSystemContext/EnvironmentContext wiring, the CreateBuilder/CreateBuilderFromSource split, and Checksum/Zip resolution) and I found no outstanding issues in that part of the diff. This round only reviews the new commit since then, fix(cache): scope defaults to pipeline directory (91b1c46 → 9c1cbed2).

The cache follow-up I suggested is correctly implemented

Two rounds ago I flagged that ModuleCacheOptions.WorkingDirectory still defaulted to raw Directory.GetCurrentDirectory() and wasn't wired to the new PipelineWorkingDirectory singleton, calling it a fast-follow rather than a blocker. This commit closes that gap with a minimal, well-targeted change (src/ModularPipelines/PipelineBuilder.cs:344-345):

services.Configure<ModuleCacheOptions>(options =>
    options.WorkingDirectory = _environment.WorkingDirectory);

I traced the registration ordering to confirm the override semantics are correct rather than accidental:

• This Configure<ModuleCacheOptions> call runs unconditionally, early in ConfigureServices, against the host's real IServiceCollection.
• AddModuleCache<TStore>(configure) (PipelineBuilderExtensions.cs:254) registers any user-supplied configure delegate against builder.Services (a separate ServiceCollection field, _services), which is only copied into the host's collection after this new default-setting line, via the foreach (var descriptor in _services) loop a few lines down.
• Since Microsoft.Extensions.Options applies registered IConfigureOptions<T> delegates in registration order (last unconditional write wins), the user's explicit AddModuleCache<T>(options => options.WorkingDirectory = ...) correctly overrides the new pipeline-directory default rather than being clobbered by it.

This ordering is exercised by the two new tests in PipelineWorkingDirectoryTests.cs — WorkingDirectory... (asserting CacheWorkingDirectory now equals the pipeline directory) and the new ExplicitModuleCacheWorkingDirectoryOverridesPipelineDirectory (asserting an explicit AddModuleCache configure still wins). Both cases are correctly distinguished, which is exactly what I'd want to see given how easy it is to get Options-registration ordering backwards.

Bonus: this also closes the ArtifactLifecycleManager gap I flagged, as a side effect

I also previously noted ArtifactLifecycleManager's fallback used raw Directory.GetCurrentDirectory(). Checking current main, that class already has a DI-selected constructor overload that takes IOptions<ModuleCacheOptions> cacheOptions and derives _workingDirectory from cacheOptions.Value.WorkingDirectory (the plain Directory.GetCurrentDirectory() constructor is a fallback that the built-in service provider won't select once the richer overload's dependencies are all resolvable, which they always are). That wiring predates this PR, but it means today's one-line fix here now also correctly threads the pipeline's working directory through to artifact snapshotting — not just module caching — for free. Worth being aware of, not something to change.

No issues found

No compile or logic issues in this delta. The fix is minimal, correctly ordered relative to explicit user overrides, and has direct test coverage for both the default and override paths. Good to merge from my read.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9c1cbed275

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code review (full pass, current HEAD 566cb791)

The prior four review rounds on this PR were all incremental ("diff since last review"), which means a full re-read of the whole PipelineWorkingDirectory feature turned up a few things earlier rounds didn't revisit because they landed in the first commit (4f7972423) and weren't touched again. Flagging them now rather than letting them slide through unreviewed.

This round's actual delta (9c1cbed2 → 566cb791, "fix(network): scope download save paths")

src/ModularPipelines/Context/Downloader.cs:132 now resolves options.SavePath through the injected PipelineWorkingDirectory before the directory-vs-file heuristics run, consistent with how Command/FilesContext/Checksum/Zip already do it. The null/whitespace short-circuit (GetSaveLocation, line 126) runs before ResolvePath is called, so this change doesn't reintroduce the "resolve-before-null-check" issue below. New tests in DownloaderTests.cs cover the resolution. No issues in this delta.

Findings from the full-diff pass (not previously flagged)

1. FilesContext.ExistsAsync now throws on empty/whitespace input instead of returning false (src/ModularPipelines/Context/Domains/Implementations/FilesContext.cs:62). Pre-PR, this method called _fileSystemProvider.FileExists(path) directly, and File.Exists("") returns false rather than throwing. Now _workingDirectory.ResolvePath(path) runs first, and Path.GetFullPath("", basePath) throws ArgumentException. Any caller using this as a defensive "does this possibly-empty path exist?" check will now crash instead of getting false. Worth guarding with string.IsNullOrWhiteSpace(path) → false before resolving, mirroring the Downloader fix in this same PR.

2. Checksum.Md5 has the same before/after ordering problem, but changes the exception type (src/ModularPipelines/Context/Checksum.cs:13). Pre-PR, an empty/whitespace filePath hit FileExists("") → false → the documented FileNotFoundException. Now workingDirectory.ResolvePath(filePath) throws ArgumentException first, so callers catching FileNotFoundException (the exception this method's own throw statement promises) won't catch it for this input. Same fix shape as Feature/initial work #1: check IsNullOrWhiteSpace before resolving, or let ResolvePath normalize invalid input to something that still reaches the FileExists check.

3. PipelineDirectory.TryFindPipelineProject failing silently changes CreateBuilderFromSource's failure mode (src/ModularPipelines/Pipeline.cs:46, src/ModularPipelines/PipelineDirectory.cs:31). CreateBuilderFromSource uses the Try* variant, so if no ancestor of the source file (or AppContext.BaseDirectory) contains appsettings.json + a .csproj, WorkingDirectory is just null and PipelineBuilder falls back to Directory.GetCurrentDirectory() silently — no error, unlike PipelineDirectory.FindPipelineProject() (the public API right above it) which throws a clear InvalidOperationException for the same condition. Since the template (src/ModularPipelines.Templates/templates/modularpipeline/Program.cs) is the only consumer of CreateBuilderFromSource, this mostly matters for atypical layouts (e.g. entry point launched from a wrapper/CI step with a different CWD) — worth confirming this silent-fallback is intentional rather than an oversight, since the sibling method one has a fail-fast contract.

4. File/Folder public constructors bypass PipelineWorkingDirectory entirely (src/ModularPipelines/FileSystem/File.cs:29, and the equivalent in Folder.cs). This PR routes FileSystemContext.GetFile/GetFolder through PipelineWorkingDirectory.ResolvePath, but new File("relative/path") / new Folder("relative/path") still resolve relative paths via new FileInfo(path) against the real process CWD. Since this PR removes the old global Environment.CurrentDirectory mutation the template used to do, any module code that constructs File/Folder directly (instead of via context.Files.GetFile(...)) will silently resolve against a different directory than the rest of the pipeline now uses. Worth either routing these constructors through the ambient working directory too, or (if that's not feasible for a type with no DI) flagging in XML docs that direct construction doesn't participate in working-directory scoping.

5. PipelineWorkingDirectory.ResolvePath can throw on Windows for drive-relative paths (src/ModularPipelines/PipelineWorkingDirectory.cs:5). Path.GetFullPath(path, basePath) throws ArgumentException when path is drive-relative (e.g. "D:tools") and its drive differs from basePath's drive — a real edge case on Windows, not just theoretical. Low priority given most consumers are POSIX-style relative paths, but worth a defensive test if Windows CI matters here.

6. PipelineDirectory's git-root walk duplicates GitRootFinder (src/ModularPipelines/PipelineDirectory.cs:61 vs src/ModularPipelines/Distributed/Serialization/GitRootFinder.cs). Two separate "walk up looking for .git" implementations now exist in the same assembly with slightly different defaults (one throws, one returns null; different default start directories). Not a bug, but a maintenance trap — a future fix to one (e.g. worktree .git file handling, which both already handle identically today) is likely to only be applied to whichever one the fixer finds first. Consider having PipelineDirectory.IsGitRoot/FindGitRoot delegate to GitRootFinder instead of reimplementing.

Summary

Nothing blocking in this round's actual delta (Downloader scoping is correct and tested). Items 1–2 are real behavioral regressions worth fixing before merge since they change exception behavior for existing public APIs (ExistsAsync, Checksum.Md5); items 3–6 are lower-priority correctness/consistency gaps in the broader working-directory feature that are fine to track as fast-follows if the team prefers to ship now.

[thomhurst]

Fixed current-head CI/review findings in 1319e7f after rebasing onto current main:

• register a process-CWD fallback PipelineWorkingDirectory inside DependencyInjectionSetup, fixing DependencyInjectionTests.Validate
• preserve Files.ExistsAsync blank-path behavior (false) before path resolution
• preserve Checksum.Md5 blank-path FileNotFoundException behavior before path resolution
• add focused blank-path regressions

Validation: DependencyInjectionTests 3/3; FilesContextTests 2/2; ChecksumTests 3/3; PipelineWorkingDirectoryTests 5/5; DownloaderTests 15/15; core Release build 0 warnings/errors; targeted format verification clean. Resolved the two older cache/downloader threads after exact-head Claude confirmed those fixes.

@codex review

@claude review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: 1319e7fa35

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code review (current head 1319e7fa35)

Confirmed the last fix commit (1319e7fa35) addresses the two behavioral-regression items from the prior full-diff review: FilesContext.ExistsAsync and Checksum.Md5 now short-circuit on blank paths before resolution, matching pre-PR behavior, and both have regression tests. CreateBuilderFromSource's process-CWD fallback is also now documented in its <remarks> (Pipeline.cs), so that's addressed as intentional, documented behavior rather than a silent fallback.

Two items remain outstanding, one of them newly introduced by this last fix commit:

1. New: double registration of PipelineWorkingDirectory (introduced by this round's DI fix)

DependencyInjectionSetup.RegisterPipelineContextServices (src/ModularPipelines/DependencyInjection/DependencyInjectionSetup.cs:138) now does:

services.TryAddSingleton(new PipelineWorkingDirectory(Directory.GetCurrentDirectory()));

...and PipelineBuilder.BuildPipelineAsync (src/ModularPipelines/PipelineBuilder.cs:361) later does:

services.AddSingleton(new PipelineWorkingDirectory(_environment.WorkingDirectory))

Both register the same service type in the same ConfigureServices call. DependencyInjectionSetup.Initialize runs first, so today the correct (_environment.WorkingDirectory) instance ends up last and wins on resolution — but this is order-dependent DI behavior, not a deterministic single source of truth. If RegisterPipelineContextServices is ever called after the builder's own registration (several unit tests already call DependencyInjectionSetup.Initialize directly), or the two calls are reordered, the wrong working directory would resolve silently with no compiler/runtime signal, plus there's a wasted allocation for the discarded instance every run. Suggest either making the builder's registration a parameter passed into Initialize/RegisterPipelineContextServices, or having the fallback only register if nothing else provides _environment.WorkingDirectory (e.g. via a factory that reads from IPipelineEnvironment if already registered, falling back to CWD) so there's one deterministic registration path instead of relying on "last registration wins."

2. Still open: PipelineDirectory's git-root walk duplicates GitRootFinder

PipelineDirectory.IsGitRoot/FindAncestor/FindGitRoot (src/ModularPipelines/PipelineDirectory.cs:61-82) reimplements the same "walk up looking for .git" logic that already exists in GitRootFinder.Find (src/ModularPipelines/Distributed/Serialization/GitRootFinder.cs). Both are internal to the same assembly, so PipelineDirectory.FindGitRoot could delegate to GitRootFinder.Find directly. This was flagged in the prior review round and hasn't changed. Worth consolidating so a future fix to worktree/submodule/symlinked-.git handling doesn't need to land in two places.

Not re-flagging (known, previously discussed)

File/Folder public constructors (src/ModularPipelines/FileSystem/File.cs:29, Folder.cs) still resolve relative paths via new FileInfo(path)/new DirectoryInfo(path) against the real process CWD rather than PipelineWorkingDirectory. This is unchanged from the prior review round (item 4) — restating only for visibility, since it's a real but pre-existing gap: any module code constructing File/Folder directly instead of via context.Files.GetFile(...) won't participate in working-directory scoping.

Summary

Nothing blocking. The double-registration in DependencyInjectionSetup/PipelineBuilder is the one new item from this round worth tightening before merge since it's a correctness-by-coincidence pattern; the GitRootFinder duplication remains a reasonable fast-follow.

[thomhurst]

Ubuntu pipeline failure is unrelated to this working-directory PR: ModuleTimeoutTests.Pipeline_Default_Module_Timeout_Is_Applied received the raw TimeoutException instead of the expected ModuleFailedException, matching the timeout-classification race being addressed separately. The workflow run 31355257657 still has macOS pending, so failed-job rerun is not available yet.

[thomhurst]

Reran the failed Ubuntu job once. The prior failure was the unrelated Pipeline_Default_Module_Timeout_Is_Applied timing test; macOS, Windows, analyzers, AOT, and static checks passed on exact head 1319e7fa35.

[thomhurst]

@claude review

[thomhurst]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1319e7fa35

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

Refreshed onto current main and fixed the run-report root gap in 7629eeb693. The original six commits are range-diff exact; the added commit scopes run-report resolution to the pipeline working directory/Git root and removes duplicate working-directory DI registration.

Validation:

• PipelineWorkingDirectoryTests: 5/5
• DependencyInjectionTests: 3/3
• RunReportTests: 74/74
• ModularPipelines.slnx Release build: 0 warnings, 0 errors
• targeted formatting: changed files clean; only the existing unsupported F# project notice and an unchanged PipelineBuilder.cs:280 info diagnostic remain

Targeted test compilation used the known current-main local-only ThrowingPipelineExceptionValidator async-signature adapter; it was reverted before push.

@codex review
@claude review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: 7629eeb693

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code review (current head 7629eeb693)

This is a follow-up to the prior six review rounds. Verified against a fresh clone of the current HEAD rather than relying on general impressions.

Previously flagged issue is now resolved

The double registration of PipelineWorkingDirectory flagged in the last round (at 1319e7fa35) is fixed by this round's commit (fix(reports): scope paths to pipeline root). PipelineBuilder.cs:343 now calls services.AddSingleton(new PipelineWorkingDirectory(_environment.WorkingDirectory)) before DependencyInjectionSetup.Initialize(services) runs, so RegisterPipelineContextServices's TryAddSingleton(new PipelineWorkingDirectory(Directory.GetCurrentDirectory())) (DependencyInjectionSetup.cs:138) now correctly no-ops instead of racing with it. Good fix — this is now a deliberate "default unless already provided" pattern rather than order-dependent last-registration-wins behavior.

New findings from this round

1. FilesContext.ReadAsync/WriteAsync still have the blank-path regression that ExistsAsync was just fixed for, in the same file (src/ModularPipelines/Context/Domains/Implementations/FilesContext.cs:49-56). The "preserve scoped path contracts" commit added an IsNullOrWhiteSpace guard to ExistsAsync (line 60) so it returns false instead of throwing on a blank path — but ReadAsync/WriteAsync a few lines above still call _workingDirectory.ResolvePath(path) unguarded. Pre-PR, WriteAllTextAsync("", content) threw an immediate ArgumentException ("path is empty"). Now ResolvePath("") resolves to the working directory itself (verified: Path.GetFullPath("", basePath) returns basePath unchanged, no exception), so WriteAsync("", content) attempts to write a file at the pipeline's working directory path and throws a confusing UnauthorizedAccessException/IOException instead. Since the sibling method in the same class was just hardened for exactly this case, this looks like the fix just missed two call sites rather than being an intentional decision — worth the same one-line guard.

2. FileSystemContext.GetFile/GetFolder have the equivalent gap (src/ModularPipelines/Context/FileSystemContext.cs:39,51). Pre-PR, GetFile("") threw immediately from new FileInfo("") (ArgumentException). Now it resolves via ResolvePath to the working directory and returns a File object that's actually pointing at a directory, silently, with no exception until a later .OpenRead()/.CopyTo() call fails somewhere else in the pipeline with a much less clear error. Same root cause and same fix shape as Feature/initial work #1.

3. PipelineDirectory.FindGitRoot/IsGitRoot (new public API in this PR) duplicates GitRootFinder.Find, and has no production caller (src/ModularPipelines/PipelineDirectory.cs:29,82-84 vs. src/ModularPipelines/Distributed/Serialization/GitRootFinder.cs). Both walk up the directory tree checking for .git, using different traversal mechanics (DirectoryInfo.Parent vs. Directory.GetParent(string).FullName) — they can drift on edge cases. GitRootFinder is internal in the same assembly, so PipelineDirectory.FindGitRoot could delegate to GitRootFinder.Find(GetSearchDirectory(sourceFilePath)) ?? throw ... instead of re-implementing the walk. Grepping the repo, FindGitRoot currently has zero callers outside its own unit test — worth confirming it's actually needed as public API before it ships, since a duplicated implementation with no consumer is upkeep for no current benefit.

Minor / low-priority

• CliCommandFactory.ResolveWindowsCommandScript's workingDirectory ?? Environment.CurrentDirectory fallback (src/ModularPipelines/Helpers/Internal/CliCommandFactory.cs:101) is dead on the only production path now that Command.cs:72-74 always populates a non-null, pipeline-scoped WorkingDirectory before calling in. Not urgent, but if this static helper is ever called directly with null again, it'll silently revert to raw process CWD instead of the configured pipeline directory — consider threading PipelineWorkingDirectory through instead of the raw fallback, for consistency with the rest of this PR's approach.
• ModuleResultSerializer.cs:30 (GitRootFinder.Find() with no argument, so it resolves from raw Environment.CurrentDirectory) and CommandResult.Ok(...) (CommandResult.cs:23, hardcodes Environment.CurrentDirectory) are both untouched by this PR — not regressions here — but they're the same class of gap this PR has been closing everywhere else (distributed git-root resolution, command result stubbing). Flagging as a candidate fast-follow rather than blocking on this PR, same as the module-cache and download-path items were handled as fast-follows in earlier rounds.

Nothing above blocks merge on its own — items 1 and 2 are the most worth fixing since they're direct, easily-triggered regressions in already-published public API (context.Files.WriteAsync, context.Files.GetFile, etc.) right next to a sibling method that already got the correct fix.

[thomhurst]

Addressed the current-head Claude findings in f61492d:

• ReadAsync/WriteAsync now reject null, empty, and whitespace paths before working-directory resolution
• GetFile/GetFolder preserve the same immediate argument validation
• public PipelineDirectory.FindGitRoot remains available but delegates to the existing internal GitRootFinder implementation

Added focused coverage for all four blank-path APIs. Validation: 9/9 focused tests; core Release build 0 warnings/errors.

@claude review

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f61492d834

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code review (current head f61492d834, re-review of round 8)

This is a follow-up to the seven prior review rounds on this PR. I re-verified the full current diff against main from scratch (not just the incremental delta) rather than trusting prior summaries, and cross-checked every finding against the actual files in this checkout.

Previously-flagged issues are all resolved at this head

• FilesContext.ReadAsync/WriteAsync/GetFile/GetFolder now throw immediately on null/empty/whitespace via ArgumentException.ThrowIfNullOrWhiteSpace before resolving through PipelineWorkingDirectory, matching pre-PR behavior (FilesContext.cs:49-56, FileSystemContext.cs:43,49). Confirmed with the new focused blank-path tests.
• PipelineDirectory.FindGitRoot now delegates to GitRootFinder.Find instead of re-implementing the walk (PipelineDirectory.cs:33-34) — the duplication flagged two rounds ago is gone.
• The double DI registration of PipelineWorkingDirectory (DependencyInjectionSetup vs PipelineBuilder) is resolved: PipelineBuilder.BuildPipelineAsync registers it with AddSingleton before DependencyInjectionSetup.Initialize runs, so the setup's TryAddSingleton(new PipelineWorkingDirectory(Directory.GetCurrentDirectory())) correctly no-ops as a "default unless already provided" fallback rather than racing.
• Re-verified the FirstNonEmpty(fallback, candidates...) call in CreateHostEnvironment (PipelineBuilder.cs:293-301) myself against its actual signature (private static string FirstNonEmpty(string fallback, params string?[] candidates)) — options.WorkingDirectory is genuinely a candidate checked before the Directory.GetCurrentDirectory() fallback, so explicit configuration correctly wins. No bug despite how the call site reads.
• RunReportPathResolver's new PipelineWorkingDirectory-based constructor preferring a discovered Git root over the explicit working directory is intentional, not a regression — it mirrors the existing parameterless constructor's behavior and is directly covered by RunReportRootUsesGitRootForRepositoriesAndWorktrees in RunReportTests.cs.

Fresh pass over this round's actual delta (fix(files): preserve blank-path validation, 9c1cbed2/1319e7fa35 → f61492d8)

Traced Downloader.GetSaveLocation (Downloader.cs:129-145) since it now resolves options.SavePath through PipelineWorkingDirectory — the directory-vs-file heuristics still correctly check the original options.SavePath for a trailing separator (as a pure intent signal) while performing filesystem operations against the resolved savePath, so no behavior drift there. No new issues found in this delta.

CI failure on this head is unrelated to this PR — no action needed here

The .NET / pipeline (ubuntu-latest) job on the latest run fails with:

ValidationTests.cs(196,63): error CS0535: 'ValidationTests.ThrowingPipelineExceptionValidator' does not implement interface member 'IPipelineValidator.ValidateAsync(IServiceProvider)'

I traced this back rather than assuming it's a regression from this branch: test/ModularPipelines.UnitTests/Validation/ValidationTests.cs is untouched by every commit on this branch, and the same broken synchronous Validate(IServiceProvider) (instead of ValidateAsync) already exists on main at this PR's merge-base (485c3d3d, 08:03 UTC). A separate PR (fix(tests): update validator contract, 173198a1) fixed it on main at 09:14 UTC — after this branch's CI run started at 08:09. So this is transient main-branch breakage this PR's CI window landed in, not something introduced here. A rebase/merge onto current main (or just a rerun once merged) should pick up the fix and go green.

Still-open, non-blocking (already discussed, restating only for visibility)

• File/Folder public constructors (FileSystem/File.cs:30, Folder.cs) still resolve relative paths via new FileInfo(path)/new DirectoryInfo(path) against the real process CWD rather than PipelineWorkingDirectory. Confirmed still true at this head. Module code constructing these directly (vs. context.Files.GetFile(...)) won't participate in working-directory scoping — a real but pre-existing, already-acknowledged gap.
• The minor CliCommandFactory/ModuleResultSerializer/CommandResult raw-Environment.CurrentDirectory spots flagged a couple of rounds back remain untouched — fine as fast-follows per the earlier discussion, not regressions from this PR.

Summary

Nothing blocking in the code itself. All substantive design and correctness issues raised across the prior seven rounds are fixed and verified against the actual current diff. The one red CI check is unrelated main flakiness that's already fixed upstream — good to merge once a rebase/rerun picks that up.

[thomhurst]

Fresh P2 fixed at exact head cf8c27a. Eight prior commits are range-diff exact after rebasing onto current main. Requirement factory tests pass 9/9; core Release build passes 0 warnings and 0 errors. @claude review the exact head. @codex review the exact head.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: cf8c27a1f7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code review (current head cf8c27a1)

This is a follow-up to the eight prior review rounds on this PR. Reviewed the delta since the last-reviewed head (the rebase is range-diff exact for the eight prior commits — confirmed by git log), which is this one new commit: fix(requirements): scope relative paths (cf8c27a1).

What changed

Require.FileExists / Require.DirectoryExists (src/ModularPipelines/Requirements/Require.cs:118-150) previously called File.Exists/Directory.Exists directly against the raw path (i.e. resolved against the real process CWD, not the pipeline's scoped working directory — the exact class of bug this PR exists to fix elsewhere). They now route through context.Files.GetFile(filePath).Exists / context.Files.GetFolder(directoryPath).Exists, which resolves relative paths via PipelineWorkingDirectory like every other file-system entry point already touched in this PR.

Verified:

• FileSystemContext.GetFile/GetFolder (src/ModularPipelines/Context/FileSystemContext.cs:43-63) throw ArgumentException on null/whitespace paths, so the added !string.IsNullOrWhiteSpace(...) guards are load-bearing, not defensive fluff — they preserve the old File.Exists/Directory.Exists semantics of returning false for a blank path instead of throwing out of a requirement evaluator.
• The lambda stays synchronous (Func<IPipelineContext, bool>), matching the existing DelegateRequirement sync overload used elsewhere in this file — no accidental signature drift.
• Removing [ExcludeFromCodeCoverage] from both methods is justified: they're now testable through the DI-provided IFilesContext fake/real working directory rather than hitting the real filesystem directly, and the new FileSystemRequirementsResolveFromPipelineWorkingDirectory test (test/ModularPipelines.UnitTests/Requirements/RequireFactoryTests.cs:150-183) exercises exactly that: relative Require.FileExists("./appsettings.json")/Require.DirectoryExists("./artifacts") resolve against a PipelineBuilderOptions.WorkingDirectory temp dir, the pipeline succeeds, and Environment.CurrentDirectory is asserted unchanged — consistent with this PR's broader goal of never mutating process CWD.

No new issues found in this delta. All prior review rounds' findings were already confirmed fixed as of the last review; this commit closes the one remaining scoping gap (Require.FileExists/DirectoryExists) using the same established pattern as the rest of the PR.