Add supported module testing harness

[thomhurst]

Summary

• add ModularPipelines.Testing with isolated ModuleTester typed/untyped APIs and dependency seeding
• add public command interception/recording, in-memory filesystem, and no-op console/progress defaults
• add package/test solutions, README registration, and replace broken testing docs

Validation

• guarded Release build: src/ModularPipelines.Testing/ModularPipelines.Testing.sln
• 13 ModularPipelines.Testing.UnitTests passed with coverage
• 76 affected core tests passed
• guarded Build project compile passed
• Docusaurus production build passed
• core and Testing warning-level analyzer gates passed
• package smoke pack passed

Closes #3538

[claude]

Code review

Reviewed the new ModularPipelines.Testing harness (ModuleTester, RecordingCommandInterceptor, InMemoryFileSystemProvider, NoOpConsoleServices) and the supporting core changes (ICommandInterceptor, File.Exists/Folder.Exists routing through IFileSystemProvider, CommandResult.Ok). No CLAUDE.md violations found. Found several issues, ranked by severity — all verified against the code as it currently exists on this branch.

1. Command interception silently defeats ThrowOnNonZeroExitCode (bug)

ModularPipelines/src/ModularPipelines/Context/Command.cs

Lines 92 to 102 in 8a5cc93

	foreach (var interceptor in _commandInterceptors)
	{
	if (await interceptor.InterceptAsync(invocation, cancellationToken).ConfigureAwait(false) is { } intercepted)
	{
	var result = ApplyCommandMetadata(intercepted, command);
	LogInterceptedCommand(options, execOpts, result);
	return result;
	}
	}

When an ICommandInterceptor returns a result, the method returns immediately at line 99 — before Of(...) (the only caller of CreateCommandFailure, which throws CommandException when ExitCode != 0 && ThrowOnNonZeroExitCode, default true). So a module (or a test) that intercepts a command and returns a non-zero-exit-code CommandResult gets that result back silently instead of the CommandException a real failing command would throw. Any module whose error handling relies on the throw (which is the framework default) behaves differently under a stubbed command than it would in production — the exact opposite of what a test double should guarantee.

Suggested fix: route the intercepted result through the same failure-evaluation step (CreateCommandFailure, or a shared helper) instead of returning early. This is worth fixing at the abstraction level, not just for tests — ICommandInterceptor is a public seam usable outside tests too (per docs/docs/how-to/testing.md), so any production use of interception has the same silent-failure hole.

2. InMemoryFileSystemProvider.IsDescendant never matches anything under a root path (bug)

ModularPipelines/src/ModularPipelines.Testing/InMemoryFileSystemProvider.cs

Lines 335 to 383 in 8a5cc93

	private string Normalize(string path)
	{
	ArgumentException.ThrowIfNullOrWhiteSpace(path);
	var fullPath = Path.GetFullPath(path);
	var root = Path.GetPathRoot(fullPath);
	return root is not null && _pathComparer.Equals(root, fullPath)
	? fullPath
	: fullPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar);
	}
	private IEnumerable<string> EnumerateEntries(
	IEnumerable<string> entries,
	string path,
	string searchPattern,
	SearchOption searchOption)
	{
	var root = Normalize(path);
	if (!_directories.ContainsKey(root))
	{
	throw new DirectoryNotFoundException(root);
	}
	return [.. entries
	.Where(entry => IsDescendant(entry, root))
	.Where(entry => searchOption == SearchOption.AllDirectories
	|| !Path.GetRelativePath(root, entry)
	.Contains(Path.DirectorySeparatorChar, StringComparison.Ordinal))
	.Where(entry => System.IO.Enumeration.FileSystemName.MatchesSimpleExpression(
	searchPattern,
	Path.GetFileName(entry),
	_pathComparison == StringComparison.OrdinalIgnoreCase))
	.OrderBy(static entry => entry, _pathComparer)];
	}
	private IEnumerable<string> GetDescendantFiles(string path) =>
	_files.Keys.Where(entry => IsDescendant(entry, path));
	private IEnumerable<string> GetDescendantDirectories(string path) =>
	_directories.Keys.Where(entry => IsDescendant(entry, path));
	private bool IsDescendant(string candidate, string parent)
	{
	if (!candidate.StartsWith(parent, _pathComparison) || candidate.Length <= parent.Length)
	{
	return false;
	}
	return candidate[parent.Length] is '/' or '\\';
	}

Normalize keeps the trailing separator only for root paths (/, C:\), so IsDescendant("/tmp/x.txt", "/") checks candidate[parent.Length], which lands on 't' (the first char of the child name) rather than a separator, and returns false. Since the constructor pre-registers the root as a directory, this is reachable: EnumerateFiles("/", "*", AllDirectories) silently returns empty even with files present, and DeleteDirectory("/", recursive: true) reports success while deleting none of the root's contents (only the root's own dictionary entry is removed). No existing test exercises operations against the root — the test suite only touches paths under GetTempPath().

Suggested fix: special-case parents that already end in a separator in IsDescendant (treat the boundary as already consumed), e.g. if (parent[^1] is '/' or '\\') return true; before the boundary-character check.

3. Filesystem virtualization is incomplete — Exists is virtualized, most everything else isn't (design gap)

ModularPipelines/src/ModularPipelines/FileSystem/Folder.cs

Lines 512 to 524 in 8a5cc93

	public IEnumerable<File> ListFiles()
	{
	return DirectoryInfo.EnumerateFiles("*", SearchOption.TopDirectoryOnly)
	.Select(x => new File(x))
	.Distinct();
	}
	public IEnumerable<Folder> ListFolders()
	{
	return DirectoryInfo.EnumerateDirectories("*", SearchOption.TopDirectoryOnly)
	.Select(x => new Folder(x))
	.Distinct();
	}

This PR routes File.Exists/Folder.Exists through _provider, but Folder.GetFiles/GetFolders/ListFiles/ListFolders/Clean and File.Length/Attributes/CreationTime/LastWriteTimeUtc/Extension/IsReadOnly/Hidden/Name all still go straight to DirectoryInfo/FileInfo/System.IO.Directory, bypassing IFileSystemProvider entirely. Under InMemoryFileSystemProvider, a module can observe folder.Exists == true (in-memory) and then have folder.GetFiles() throw DirectoryNotFoundException or silently read unrelated files from the real disk — an inconsistent, half-virtualized filesystem that's worse than not virtualizing at all, because it looks trustworthy.

Suggested approach: rather than patching individual members opportunistically (which is how this partial state arose), treat IFileSystemProvider as the single seam for all of File/Folder's disk interaction — extend the provider interface to cover enumeration and metadata, and route every member through it. If full coverage isn't feasible in this PR, the safer interim behavior is for InMemoryFileSystemProvider-backed File/Folder instances to throw NotSupportedException on the un-virtualized members, so gaps fail loudly during test authoring instead of silently falling through to the real filesystem.

4. AppendAllTextAsync/CopyFile/MoveFile aren't actually thread-safe, despite the class doc comment (bug vs. documented contract)

ModularPipelines/src/ModularPipelines.Testing/InMemoryFileSystemProvider.cs

Lines 91 to 101 in 8a5cc93

	/// <inheritdoc />
	public Task AppendAllTextAsync(
	string path,
	string contents,
	CancellationToken cancellationToken = default)
	{
	cancellationToken.ThrowIfCancellationRequested();
	var existing = FileExists(path) ? Encoding.UTF8.GetString(GetFile(path)) : string.Empty;
	SetFile(path, Encoding.UTF8.GetBytes(existing + contents));
	return Task.CompletedTask;
	}

The class is documented as "Thread-safe" on the strength of using ConcurrentDictionary, but AppendAllTextAsync does an unguarded read-modify-write (FileExists → GetFile → SetFile), and CopyFile/MoveFile do check-then-act (ContainsKey then SetFile/TryRemove). ConcurrentDictionary only makes each individual operation atomic — it does not make a sequence of operations atomic. Two concurrent appends to the same path can lose one; overwrite: false on CopyFile can still be raced into an overwrite.

Suggested fix: use ConcurrentDictionary.AddOrUpdate (with a CAS-retry factory) for the append path, and guard copy/move with a per-path lock or an atomic TryAdd-first pattern. If true thread-safety isn't actually needed for this harness's use case (single module, sequential execution), the simpler and more honest fix is to correct the doc comment rather than leave a contract the code doesn't meet.

5. Unseeded [DependsOn<T>] dependency hangs for up to 30 minutes instead of failing fast (API design)

ModularPipelines/src/ModularPipelines.Testing/ModuleTester.cs

Lines 148 to 191 in 8a5cc93

	builder.AddModule<TModule>();
	foreach (var registration in _registrations)
	{
	registration(builder);
	}
	await using var pipeline = await builder.BuildAsync().ConfigureAwait(false);
	foreach (var dependencySeed in _dependencySeeds)
	{
	dependencySeed.Apply(pipeline.Services);
	}
	var module = pipeline.Services.GetServices<IModule>()
	.OfType<TModule>()
	.Single();
	var executionContext = ExecutionContextFactory.Create(module, typeof(TModule));
	var pipelineContext = pipeline.Services.GetRequiredService<IPipelineContext>();
	var logger = GetModuleLogger(pipeline.Services);
	var moduleContext = new ModuleContext(pipelineContext, module, executionContext, logger);
	var executionPipeline = pipeline.Services.GetRequiredService<IModuleExecutionPipeline>();
	var executor = ModuleExecutionDelegateFactory.GetExecutor(module.ResultType);
	await using var loggerScope = new ModuleLoggerScope(logger, typeof(TModule));
	IModuleResult result;
	try
	{
	result = await executor(
	executionPipeline,
	module,
	executionContext,
	moduleContext,
	cancellationToken)
	.ConfigureAwait(false);
	}
	catch when (executionContext.ExecutionTask.IsCompletedSuccessfully)
	{
	result = await executionContext.ExecutionTask.ConfigureAwait(false);
	}
	pipeline.Services.GetRequiredService<IModuleResultRegistry>()
	.RegisterResult(typeof(TModule), result);

ModuleTester auto-registers a module's dependencies in DI but only resolves their results if seeded via WithDependencyResult (which internally forces the result via TrySetDistributedResult). It executes the module directly through IModuleExecutionPipeline, bypassing the scheduler (IDependencyWaiter) entirely. If a test forgets to seed a dependency and the module awaits it (context.GetModule<Dep>()), nothing ever completes that dependency's TaskCompletionSource — the await blocks until PipelineOptions.DefaultModuleTimeout (30 minutes) elapses and throws ModuleTimeoutException, instead of an immediate, clear "dependency not seeded" error. For a unit-testing library, a fast, precise failure on a missing test setup is the whole point — a half-hour hang is close to worst-case UX for a typo'd or forgotten WithDependencyResult call.

Suggested fix: validate at ExecuteAsync/build time that every [DependsOn<T>] dependency of the module under test has either been seeded or is otherwise resolvable, and throw immediately with a clear message naming the missing dependency if not.

Related, lower-severity architectural note: ModuleTester reimplements the single-module execution path by hand (ExecutionContextFactory → ModuleExecutionDelegateFactory.GetExecutor → IModuleExecutionPipeline) rather than reusing ModuleRunner, so ModuleRunner-owned behavior — attribute-based lifecycle handlers (IModuleStartHandler/IModuleEndHandler), global IModuleEventReceivers, dependency-skip propagation, mediator notifications — silently doesn't run under test even though it runs in production. Today's docs/docs/how-to/testing.md claims are narrow enough to still be technically accurate (it only promises skip conditions/timeouts/retries/direct hooks, which do run), but the gap is real and will only grow as more is added to ModuleRunner over time. A narrower shared seam — e.g. an internal single-module execution entry point used by both ModuleRunner and ModuleTester — would prevent this from silently drifting further as the engine evolves, versus maintaining two parallel orchestration implementations.

---

Nice addition overall — the builder API (ModuleTester.For<T>(), WithDependencyResult, InterceptCommands) is a clean, discoverable shape for module authors, and the File.Exists/Folder.Exists fix (routing through _provider instead of hitting FileInfo/DirectoryInfo directly) is a genuine pre-existing bug fix. The issues above are mostly about the harness's guarantees not fully matching what it implies (thread-safety, full filesystem virtualization, production-parity execution, fail-fast on missing setup) — worth tightening before this becomes the documented, supported way to test modules.

[thomhurst]

Addressed all five review findings in beb5acf and rebased onto current main.\n\n- intercepted nonzero command results now honor ThrowOnNonZeroExitCode and throw CommandException after logging\n- root paths now match descendants correctly\n- File/Folder enumeration, globbing, and Clean route through IFileSystemProvider; unsupported physical metadata/copy semantics fail loudly instead of touching disk\n- in-memory compound mutations are synchronized, including append/copy/move\n- required dependency results are validated before execution with a named, immediate setup error\n- docs describe dependency, command, and metadata contracts\n\nValidation: ModularPipelines.Testing.UnitTests 19/19; FolderTests 25/25; FileTests 29/29; changed-file analyzer gate passes at warning severity; whitespace gate passes. @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: beb5acff91

ℹ️ 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

Re-reviewed after the beb5acff91 fix commit ("honor runtime contracts"), which addresses my previous review. Comparing against each of the five previously flagged issues:

1. Command interception bypassing ThrowOnNonZeroExitCode — fixed. The intercepted-result path now throws CommandException when ExitCode != 0 && ThrowOnNonZeroExitCode, matching the real-process path:

   ModularPipelines/src/ModularPipelines/Context/Command.cs

   Lines 92 to 103 in beb5acf

   	foreach (var interceptor in _commandInterceptors)
   	{
   	if (await interceptor.InterceptAsync(invocation, cancellationToken).ConfigureAwait(false) is { } intercepted)
   	{
   	var result = ApplyCommandMetadata(intercepted, command);
   	LogInterceptedCommand(options, execOpts, result);
   	if (result.ExitCode != 0 && execOpts.ThrowOnNonZeroExitCode)
   	{
   	throw new CommandException(result);
   	}

2. IsDescendant never matching root-level children — fixed. IsDescendant now short-circuits when the parent already ends in a separator:

   ModularPipelines/src/ModularPipelines.Testing/InMemoryFileSystemProvider.cs

   Lines 404 to 412 in beb5acf

   	private bool IsDescendant(string candidate, string parent)
   	{
   	if (!candidate.StartsWith(parent, _pathComparison) || candidate.Length <= parent.Length)
   	{
   	return false;
   	}
   	return parent[^1] is '/' or '\\'
   	|| candidate[parent.Length] is '/' or '\\';

   , and it's covered by a new EnumeratesFilesUnderFileSystemRoot test.
3. Half-virtualized filesystem — fixed via the interim approach I suggested: enumeration/globbing/Clean now route through IFileSystemProvider, and the remaining physical-metadata members (Attributes, Length, timestamps, etc.) throw NotSupportedException under a non-SystemFileSystemProvider instance instead of silently touching disk:

   ModularPipelines/src/ModularPipelines/FileSystem/Folder.cs

   Lines 671 to 680 in beb5acf

   	private DirectoryInfo GetPhysicalDirectoryInfo()
   	{
   	EnsurePhysicalMetadataSupported();
   	return DirectoryInfo;
   	}
   	private void EnsurePhysicalMetadataSupported()
   	{
   	if (!ReferenceEquals(_provider, SystemFileSystemProvider.Instance))
   	{

4. Non-atomic AppendAllTextAsync/CopyFile/MoveFile — fixed. All three now hold _sync around their read-modify-write/check-then-act sequences, and a new ConcurrentAppendsDoNotLoseWrites test (100 concurrent appends) backs it.
5. Unseeded dependency hanging for up to 30 minutes — fixed. ValidateRequiredDependencyResults now walks the module's non-optional dependencies before execution and throws InvalidOperationException immediately, naming the missing dependency:

   ModularPipelines/src/ModularPipelines.Testing/ModuleTester.cs

   Lines 208 to 233 in beb5acf

   	private static void ValidateRequiredDependencyResults(
   	IModule module,
   	IServiceProvider services)
   	{
   	var registeredModuleTypes = services.GetServices<IModule>()
   	.Select(static registeredModule => registeredModule.GetType())
   	.ToArray();
   	var dependencyRegistry = services.GetRequiredService<IModuleDependencyRegistry>();
   	var metadataRegistry = services.GetRequiredService<IModuleMetadataRegistry>();
   	var resultRegistry = services.GetRequiredService<IModuleResultRegistry>();
   	var missingDependencies = ModuleDependencyResolver
   	.GetAllDependencies(
   	module,
   	registeredModuleTypes,
   	dependencyRegistry,
   	metadataRegistry)
   	.Where(static dependency => !dependency.Optional)
   	.Select(static dependency => dependency.DependencyType)
   	.Distinct()
   	.Where(dependencyType => resultRegistry.GetResult(dependencyType) is null)
   	.Select(static dependencyType => dependencyType.Name)
   	.Order(StringComparer.Ordinal)
   	.ToArray();
   	if (missingDependencies.Length > 0)
   	{

   — covered by MissingRequiredDependencyFailsFast.

All five are re-verified against the current code, not just the commit message, and each has direct test coverage. No CLAUDE.md violations found.

Two design points remain worth tracking, neither blocking:

A. Physical-metadata gating is identity-based, not capability-based. GetPhysicalFileInfo/GetPhysicalDirectoryInfo decide whether Attributes/Length/timestamps are supported by ReferenceEquals(_provider, SystemFileSystemProvider.Instance) (

ModularPipelines/src/ModularPipelines/FileSystem/File.cs

Lines 464 to 473 in beb5acf

	private FileInfo GetPhysicalFileInfo()
	{
	if (!ReferenceEquals(_provider, SystemFileSystemProvider.Instance))
	{
	throw new NotSupportedException(
	"File metadata is unavailable through the configured IFileSystemProvider.");
	}
	return FileInfo;
	}

). That's safe today only because SystemFileSystemProvider is sealed with a private constructor, so the real filesystem can only ever be reached through that one singleton. But it means any future provider that legitimately backs onto real disk — a logging/auditing decorator around SystemFileSystemProvider, a chroot'd or read-only wrapper, etc. — would be misclassified as "unsupported" purely because it isn't that exact instance, even though FileInfo/DirectoryInfo would work fine against it. A capability contract (e.g. an optional interface the provider implements, or extending IFileSystemProvider itself to expose these members as the original review suggested) would let correctness be a property of what a provider can do rather than which concrete singleton it happens to be, so this doesn't need revisiting every time a new provider wraps or extends the physical one.

B. ModuleTester still reimplements single-module execution instead of reusing ModuleRunner. This was called out as a related, lower-severity note last round and wasn't part of the required five, so it's understandable it's still open. ExecuteCoreAsync hand-assembles ExecutionContextFactory → ModuleExecutionDelegateFactory.GetExecutor → IModuleExecutionPipeline (

ModularPipelines/src/ModularPipelines.Testing/ModuleTester.cs

Lines 120 to 167 in beb5acf

	internal async Task<ExecutionOutcome> ExecuteCoreAsync(CancellationToken cancellationToken)
	{
	var fileSystem = new InMemoryFileSystemProvider();
	var recorder = new RecordingCommandInterceptor();
	if (_commandHandler is not null)
	{
	recorder.SetHandler(_commandHandler);
	}
	using var builder = Pipeline.CreateBuilder(new PipelineBuilderOptions
	{
	EnableCommandLineOptions = false,
	});
	builder.ConfigurePipelineOptions(options => options with
	{
	ShowProgressInConsole = false,
	PrintResults = false,
	PrintLogo = false,
	PrintDependencyChains = false,
	ThrowOnPipelineFailure = false,
	});
	builder.Services.AddLogging(logging => logging.ClearProviders());
	builder.Services.Replace(ServiceDescriptor.Singleton<IFileSystemProvider>(fileSystem));
	builder.Services.AddSingleton(recorder);
	builder.Services.AddSingleton<ICommandInterceptor>(recorder);
	builder.Services.AddSingleton<IConsoleCoordinator>(NoOpConsoleServices.Instance);
	builder.Services.AddSingleton<IOutputCoordinator>(NoOpConsoleServices.Instance);
	builder.Services.AddSingleton<IProgressDisplay>(NoOpConsoleServices.Instance);
	builder.AddModule<TModule>();
	foreach (var registration in _registrations)
	{
	registration(builder);
	}
	await using var pipeline = await builder.BuildAsync().ConfigureAwait(false);
	foreach (var dependencySeed in _dependencySeeds)
	{
	dependencySeed.Apply(pipeline.Services);
	}
	var module = pipeline.Services.GetServices<IModule>()
	.OfType<TModule>()
	.Single();
	ValidateRequiredDependencyResults(module, pipeline.Services);
	var executionContext = ExecutionContextFactory.Create(module, typeof(TModule));

) rather than going through ModuleRunner, so ModuleRunner-owned behavior (IModuleStartHandler/IModuleEndHandler, global IModuleEventReceivers, dependency-skip propagation) still doesn't run under test. The docs are scoped narrowly enough to stay accurate today, but as more responsibility accretes onto ModuleRunner over time, this is the kind of gap that grows silently rather than failing loudly. Worth eventually factoring out a shared single-module execution seam used by both ModuleRunner and ModuleTester, so the two orchestration paths can't drift apart.

Nice follow-through — all five issues were fixed at the root cause (not papered over) and each got direct regression coverage rather than just a manual claim in the PR description.

All five requested changes were fixed in beb5acf; Claude re-reviewed the current implementation and explicitly confirmed every finding resolved in issue comment 5137754897.

[thomhurst]

Removed the two remaining autofix-style null-check findings in 78d8a23. FolderTests 25/25 and FileTests 29/29 pass; focused IDE0041 and whitespace verification pass. @codex review

[claude]

Code review

Re-checked after the latest commit (78d8a239, "style(filesystem): simplify null checks"), the only change since my previous review comment.

That commit swaps ReferenceEquals(null, other) for other is null in File.Equals and Folder.Equals. Both perform a pure reference-based null check with no operator overload involved, so this is behavior-preserving — no issue.

All 5 previously-flagged issues (command interception bypassing ThrowOnNonZeroExitCode, IsDescendant root-matching, half-virtualized filesystem, non-atomic in-memory file ops, unseeded-dependency hangs) remain fixed and test-covered as of beb5acff91, and nothing in this new commit reopens them. No CLAUDE.md violations.

No new issues found.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 78d8a23954

ℹ️ 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]

@codex review

[claude]

Code review

Re-reviewed at the current head (051f626e, "refactor(testing): simplify file opening"). The five issues from my first pass and the two from the second pass remain fixed and test-covered — nothing in 051f626e (a pure Open-method extraction: ValidateOpenArguments/GetInitialContents/ShouldInitializeFile/CreateStream) changes that behavior.

Since my last review, chatgpt-codex-connector posted six new P2 findings against this same head. I independently verified each against the current code; all six are real and none has been addressed yet:

1. GetFiles/GetFolders regression — full recursive walk before exclusion, SafeWalk semantics lost (src/ModularPipelines/FileSystem/Folder.cs:493-524)
On main, these methods use SafeWalk.EnumerateFolders/EnumerateFiles, which prune excluded directories before recursing into them and tolerate inaccessible directories. This PR replaces that with _provider.EnumerateDirectories(Path, "*", SearchOption.AllDirectories).Where(path => !IsExcludedByDirectoryFilter(...)). For SystemFileSystemProvider, that's Directory.EnumerateFiles(path, "*", AllDirectories) — it walks the entire tree first, then the predicate filters results after the fact. This is the one finding here that isn't testing-library-specific: it's a behavior change to the core File/Folder API used in production. Any pipeline excluding bin/obj/.git/node_modules/a permission-restricted directory specifically to avoid descending into it can now throw UnauthorizedAccessException (no more IgnoreInaccessible) or just pay for a much more expensive walk than before, exactly where the exclusion filter existed to prevent that. Worth fixing before merge, since it's a silent regression for any existing pipeline module, not just an edge case in the new harness.

2-5. InMemoryFileSystemProvider fidelity gaps (all reachable, all diverge from SystemFileSystemProvider):

• SetFile (line 376) doesn't check _directories, and CreateDirectory doesn't check _files — a file and a directory can coexist at the same path, which a real filesystem rejects.
• SetFile auto-creates the entire parent directory chain (CreateDirectory(Path.GetDirectoryName(normalized) ...), line 381) instead of requiring the parent to already exist. Real File.WriteAllText/Create throw DirectoryNotFoundException when the parent is missing; a module that forgot to create its output directory will pass under the harness and fail in production.
• CommittingMemoryStream (line 440) only guards writes via EnsureWritable(); it never restricts reads. Open(path, mode, FileAccess.Write) returns a stream whose CanRead is still true (inherited from MemoryStream) and whose Read/ReadByte still succeed, where a real FileStream opened write-only throws on read.
• JoinLines (line 365) always appends Environment.NewLine, so WriteAllLinesAsync/AppendAllLinesAsync with an empty sequence write a single newline character. System.IO.File.WriteAllLines with an empty sequence produces a genuinely empty (0-byte) file.

6. RecordingCommandInterceptor records the pre-metadata result, not what the module receives (src/ModularPipelines.Testing/RecordingCommandInterceptor.cs:25, src/ModularPipelines/Context/Command.cs:92-103)
InterceptAsync enqueues the handler's raw CommandResult into _commands before Command.ExecuteCommandLineToolAsync calls ApplyCommandMetadata(intercepted, command). ApplyCommandMetadata overwrites CommandInput/WorkingDirectory/EnvironmentVariables on the result actually returned to the module — so run.Commands[i].Result in a test permanently reflects the interceptor's raw stub (e.g. CommandResult.Ok()'s defaults) rather than the enriched result the module code actually saw. A test asserting on the recorded working directory or env vars will get the wrong values.

Architectural observation

Items 2-5 are the third consecutive round where InMemoryFileSystemProvider ships a divergence from SystemFileSystemProvider that a reviewer had to spot by manual inspection (root-path matching, non-atomic ops, and now directory/file collisions, parent-existence, read/write access, and empty-line joining). Each round fixes the specific instances flagged, but the underlying cause — no mechanism that forces the two providers to agree — stays in place, so the next behavioral gap surfaces the same way. A parameterized/shared contract test suite ([Theory] over both provider instances, run against a real temp directory for the system provider and a fresh InMemoryFileSystemProvider for the other, asserting identical results and identical exception types for the same sequence of operations) would catch this class of bug systematically instead of one divergence at a time, and is worth adding before InMemoryFileSystemProvider grows further. The two design points I raised in the previous round (identity-based physical-metadata gating, ModuleTester reimplementing single-module execution instead of reusing ModuleRunner) are unchanged and still non-blocking.

No CLAUDE.md violations found. The extraction in 051f626e itself is clean and behavior-preserving.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 051f626e74

ℹ️ 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]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 478346f025

ℹ️ 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

Re-checked after the two commits since my last review comment (78d8a239 / d49c05c, "style: simplify null checks"):

• 9f610383 — refactor(testing): simplify file opening
• 478346f0 — fix(testing): match filesystem contracts

What changed:

1. InMemoryFileSystemProvider.Open was split into ValidateOpenArguments/GetInitialContents/ShouldInitializeFile/CreateStream helpers — behavior-preserving extraction.
2. CommittingMemoryStream now gates Read*/Seek/Position/SetLength on the requested FileAccess, and rejects seeking before the append boundary — matches real FileStream semantics and is backed by the new WriteOnlyStreamsRejectReads / AppendStreamsRejectSeekingBeforeOriginalEnd tests.
3. SetFile, CreateDirectory, and MoveDirectory now require the parent directory to exist and reject file/directory path collisions (ValidateFileDestination, RejectsFilesWithoutExistingParent, RejectsFileAndDirectoryPathCollisions, MoveDirectoryRequiresExistingDestinationParent), replacing the old auto-create-parent behavior. This matches real filesystem contracts (File.Open/Directory.Move throw DirectoryNotFoundException on a missing parent) and the accompanying test updates (file.Folder!.Create(), provider.CreateDirectory(root)) confirm call sites were updated accordingly.
4. Folder.CopyTo/CopyToAsync now only touch FileInfo/DirectoryInfo physical metadata when _provider is the real SystemFileSystemProvider — previously EnsurePhysicalMetadataSupported() and attribute copying ran unconditionally, which would throw for any non-system provider (including the in-memory one) even when preserveTimestamps was false. Covered by the new CopiesFoldersInVirtualFileSystem test.
5. Command.cs/CommandInvocation/RecordingCommandInterceptor now thread an "effective" CommandInput/WorkingDirectory/EnvironmentVariables through recorded commands. I traced this one closely since the interceptor returns the raw handler result rather than the corrected copy it enqueues — but Command.ExecuteCommandLineToolAsync re-applies ApplyCommandMetadata to whatever the interceptor returns, recomputing the same fields from the same underlying CliWrap.Command, so the recorded copy and the value returned to callers stay consistent. No bug.

Validation: built src/ModularPipelines.Testing/ModularPipelines.Testing.sln in Release (0 warnings, 0 errors) and ran ModularPipelines.Testing.UnitTests — 26/26 passed, including the new tests added in these two commits.

No new issues found, and no CLAUDE.md violations. The two design notes from my previous review (capability- vs identity-based physical-metadata gating, and ModuleTester reimplementing single-module execution instead of reusing ModuleRunner) remain open but are unrelated to this batch and non-blocking.

[thomhurst]

Rebased onto current main. Relevant tests pass 66/66; Testing package and core Release builds pass. @codex review

[claude]

Code review

Checked the current head (4d393752) against the last commit I reviewed (478346f0). The only difference is a rebase onto current main, which pulled in an unrelated merged PR (#3568, "Add required ModuleResult value access") — visible as the buildResult.ValueOrDefault! → buildResult.Value doc/README tweaks and the ModuleResult.cs/ModuleResultContractTests.cs diff noise. None of that originates from this PR; gh pr diff --name-only confirms those two files aren't part of this PR's actual change set.

No files owned by this PR (ModularPipelines.Testing/*, Command.cs, File.cs, Folder.cs, CommandResult.cs, ICommandInterceptor.cs, CommandInvocation.cs, testing.md, etc.) changed content since 478346f0. All previously-flagged issues across both review rounds (command interception bypassing ThrowOnNonZeroExitCode, IsDescendant root-matching, half-virtualized filesystem, non-atomic in-memory file ops, unseeded-dependency hangs, GetFiles/GetFolders walking the full tree before exclusion, InMemoryFileSystemProvider fidelity gaps, RecordingCommandInterceptor metadata ordering) remain fixed and test-covered. No CLAUDE.md violations.

No new issues found — nothing to re-review since the rebase carried no functional changes for this PR.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4d3937524d

ℹ️ 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".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c11a465b79

ℹ️ 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

Reviewed the commit pushed since my last review comment (against c11a465b79): ae08d4660c ("fix(testing): close filesystem parity gaps"), which appears to be the current head. Also spot-checked the peripheral files that hadn't been called out explicitly in the 25+ prior review rounds on this PR (FindProjectsModule.cs, README.md, ModularPipelines.csproj) — these are routine, mechanical registrations (new project added to the build's project list, README table row, InternalsVisibleTo) with no issues.

ae08d4660c changes, both correct and well-tested:

1. InMemoryFileSystemProvider.DeleteFile now checks the directory-form path. Previously it looked up _directories.ContainsKey(normalized) using the file-normalized path, which retains a trailing separator when the caller passes one (e.g. DeleteFile("dir" + Path.DirectorySeparatorChar)), but _directories keys are stored without a trailing separator. So a directory-form delete request silently fell through to _files.TryRemove (a no-op, since no such file key exists) instead of throwing UnauthorizedAccessException like real File.Delete does against a directory. Normalizing to directory form before the lookup fixes it, and it's the same class of trailing-separator bug fixed for ValidateFileDestination a few commits ago, just in the delete path this time. Covered by the new DeleteFileRejectsDirectoryPathsWithTrailingSeparators test.

2. Zip.ZipFolder's directory-vs-file output heuristic no longer touches the physical filesystem. This is the real fix, and it closes a gap flagged by chatgpt-codex-connector several rounds ago ("Determine ZIP output type through the configured provider") that a prior commit only partially addressed. The old code was:

   var outputIsDirectory = _fileSystemProvider.DirectoryExists(outputPath)
                           || (!_fileSystemProvider.FileExists(outputPath)
                               && outputPath.GetPathType() == PathType.Directory);

   GetPathType() (src/ModularPipelines/Helpers/PathHelpers.cs) starts with System.IO.File.Exists/Directory.Exists — real disk checks — before falling back to the separator/extension heuristic. Since the provider-based checks (DirectoryExists/FileExists) already short-circuit for genuine matches, GetPathType()'s own physical checks were pure redundancy for SystemFileSystemProvider, but for InMemoryFileSystemProvider they let real, coincidental disk state leak into a decision that's supposed to be governed entirely by the virtual provider — breaking the isolation guarantee that's the whole point of this harness. The new private IsDirectoryPath helper reimplements only the separator/extension heuristic, with no disk I/O. I confirmed GetPathType() has no other production call sites (only its own dedicated PathHelpersTests), so this was the last remaining leak of that kind.

   The accompanying test change is a good regression test, not just a rename: it creates a real directory on disk at the same path the in-memory provider would resolve the zip output to (host-collision.zip), then asserts the module's in-memory zip output still resolves to that exact file path rather than being treated as a directory and nested under it. This would have failed under the old code (physical Directory.Exists would have returned true, real disk collision leaking into virtual behavior) and passes under the new one. Directory.Delete(root, recursive: true) in a finally cleans up the physical artifact regardless of outcome.

No new issues found, and no CLAUDE.md violations (review was diff/API-level; no builds were needed to verify correctness here since the logic changes are narrow and the reasoning traces cleanly through the code).

This is the 25+ round mark on this PR, and it's been an unusually thorough back-and-forth — codex and I independently caught overlapping and complementary issues across InMemoryFileSystemProvider fidelity, Command interception semantics, Zip virtualization, and ModuleTester fail-fast behavior, and each was fixed at the root cause with direct regression coverage rather than papered over. The two longer-running architectural notes from earlier rounds remain open and still non-blocking for merge, worth tracking as follow-ups rather than blockers:

• Physical-metadata gating in File/Folder is identity-based (ReferenceEquals against the SystemFileSystemProvider singleton) rather than capability-based — a future provider that legitimately wraps real disk (e.g. a logging/audit decorator) would be misclassified as unsupported.
• ModuleTester still hand-assembles single-module execution (ExecutionContextFactory → ModuleExecutionDelegateFactory → IModuleExecutionPipeline) instead of routing through ModuleRunner, so ModuleRunner-owned behavior (lifecycle handlers, global event receivers, dependency-skip propagation) doesn't run under test. Worth eventually factoring a shared single-module execution seam so the two orchestration paths can't silently drift apart as ModuleRunner grows.
• InMemoryFileSystemProvider has grown large (~870+ lines) through many incremental System.IO-parity fixes. Each fix so far has been correct and well-tested, but the set of edge cases it needs to match is open-ended (symlinks, FileAttributes, case-preserving-but-insensitive filesystems, etc.). A parameterized contract-test suite run against both providers (asserting identical results and exception types for the same operation sequence) would catch future divergences systematically rather than one at a time, and might be worth adding now that the provider surface is mostly stable.

Mergeable from a correctness standpoint.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ae08d4660c

ℹ️ 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

Reviewed the one commit not yet covered by prior review rounds (against ae08d4660c, reviewed in the previous pass): c30c7163dc ("fix(testing): preserve temp path separator").

Change: InMemoryFileSystemProvider.GetTempPath() now appends a trailing Path.DirectorySeparatorChar, matching real System.IO.Path.GetTempPath(), which always returns a path with a trailing separator on every platform .NET supports. Previously the in-memory provider's temp path had no trailing separator, a divergence from the real filesystem contract that callers doing direct string concatenation (rather than Path.Combine) against GetTempPath() could be silently relying on incorrectly against the real provider but getting away with it against the fake one.

Traced the call sites:

• CreateDirectory(GetTempPath()) in the constructor and all production call sites (File.cs, Folder.cs, Downloader.cs, FileSystemContext.cs) go through Combine/Path.Combine, which already collapses a trailing separator correctly — no double-separator regression.
• NormalizeDirectoryPath/NormalizeFilePath both route through Path.GetFullPath + TrimEnd(DirectorySeparatorChar, AltDirectorySeparatorChar), so the trailing separator on the stored temp-path key is normalized consistently with every other directory lookup in the provider.

The new TempPathEndsWithDirectorySeparator test is a good regression guard: it exercises the case direct-Combine callers wouldn't — concatenating a filename directly onto GetTempPath() without Path.Combine — and asserts both Path.EndsInDirectorySeparator and that the write actually resolves to the same directory. This is exactly the divergence a real-vs-fake-provider parity bug would hide in.

No new issues found in this commit.

Status of previously-flagged architectural notes (unchanged by this commit, still open, still non-blocking per prior rounds — repeating briefly for visibility since this is a good stopping point after 25+ review rounds):

• ModuleTester still hand-assembles single-module execution instead of routing through ModuleRunner, so ModuleRunner-owned behavior (lifecycle handlers, global event receivers, dependency-skip propagation) doesn't run under test. A shared single-module execution seam would prevent the two orchestration paths from silently drifting apart as ModuleRunner grows.
• Physical-metadata gating in File/Folder is identity-based (ReferenceEquals against the SystemFileSystemProvider singleton) rather than capability-based, so a future provider that legitimately wraps real disk would be misclassified as unsupported.
• InMemoryFileSystemProvider has grown large (~870+ lines) through many correct, well-tested, but incremental System.IO-parity fixes (this commit included). A parameterized contract-test suite run against both providers (same operation sequence, asserting identical results and exception types) would catch future divergences systematically instead of one at a time.

None of these are blockers — they're the kind of investment worth making once the provider surface stabilizes rather than mid-flight. Mergeable from a correctness standpoint.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c30c7163dc

ℹ️ 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

Reviewed the single commit pushed since my last review comment (against c30c7163dc): 27812717ec ("fix(testing): preserve partial line writes").

Change: WriteAllLinesAsync/AppendAllLinesAsync in InMemoryFileSystemProvider.cs previously built the entire joined string in memory first (JoinLines → StringBuilder) and only then wrote it via WriteAllTextAsync/AppendAllTextAsync. That made the in-memory provider atomic in a way the real filesystem isn't: if the caller's IEnumerable<string> threw or was cancelled partway through, the old code discarded everything and left the file untouched. Real File.WriteAllLinesAsync/AppendAllLinesAsync stream line-by-line through a StreamWriter over the destination FileStream, so a failure mid-enumeration leaves whatever was already written on disk. This commit closes that parity gap by opening the destination via the existing Open(path, FileMode.Create/Append, FileAccess.Write) and streaming lines directly into it through a new WriteLinesAsync(Stream, ...) helper.

I traced the commit path closely since this is exactly the kind of "silent partial write" behavior that's easy to get wrong: the StreamWriter wraps the stream with leaveOpen: true, and both the writer and the outer CommittingMemoryStream are opened with await using, so DisposeAsync runs during exception unwinding (cancellation or enumerator exception) just as reliably as on the happy path — the writer flushes its buffer into the stream, then the stream's Dispose commits ToArray() back into the backing dictionary via _commit, regardless of which path triggered disposal. For FileMode.Create, Open() already truncates the backing entry synchronously before the first write, matching File.Create's immediate-truncate semantics rather than deferring truncation to commit time. Both new tests (LineWritesPreserveLinesWrittenBeforeEnumerationFails, and the updated cancellation test) exercise this correctly and would have failed pre-fix.

Validation:

• dotnet run --project test/ModularPipelines.Testing.UnitTests -c Release --framework net10.0: 68/69 passed.
• The one failure, SynchronousInterceptorCannotReturnAfterExecutionTimeout, is unrelated to this commit — it lives in ModuleTesterTests.cs, which this commit doesn't touch. This is the same test I flagged as fragile in an earlier review round (100ms ExecutionTimeout vs. a synchronous Thread.Sleep(200) in the interceptor, asserting the timeout wins the race). It's now failed twice in a row in this environment rather than being a one-off, so I'd upgrade this from "worth watching" to worth fixing directly: widen the margin (e.g. 100ms timeout vs. a multi-second sleep) or, better, replace the wall-clock race with a deterministic signal (a TaskCompletionSource/ManualResetEvent the test can control) so the assertion doesn't depend on scheduler timing on a loaded CI box. A timing-based test that intermittently asserts the wrong thing will erode trust in the suite's red/green signal over time, which is worse than a slightly less "realistic" synchronous-blocking scenario.

No new correctness bugs in this commit, and no CLAUDE.md violations (validation was scoped to the single affected test project, per repo guidance — no full-solution build was needed).

Previously flagged, still open, still non-blocking (repeating briefly since this is a natural checkpoint after many rounds):

• ModuleTester still hand-assembles single-module execution instead of routing through ModuleRunner, so ModuleRunner-owned behavior (lifecycle handlers, global event receivers, dependency-skip propagation) doesn't run under test.
• Physical-metadata gating in File/Folder is identity-based (ReferenceEquals against the SystemFileSystemProvider singleton) rather than capability-based.
• InMemoryFileSystemProvider continues to grow through incremental System.IO-parity fixes (this one included). A parameterized contract-test suite run against both providers would catch future divergences systematically instead of one at a time — worth a deliberate decision once the provider surface stabilizes.

Mergeable from a correctness standpoint. The flaky timeout test is the one item from this round I'd actually act on before merge, since it's now reproduced twice.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 27812717ec

ℹ️ 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

Reviewed the two commits pushed since my last review comment (against 27812717ec): bfafd72964 ("fix(testing): preserve command invocation order") and the merge commit d367bbb905 ("Merge remote-tracking branch 'origin/main'").

bfafd72964 — RecordingCommandInterceptor invocation ordering. Commands previously returned recordings in completion order (ConcurrentQueue enqueue order), so two commands started concurrently but completing out of order (e.g. a fast command started second finishing before a slow one started first) would appear reordered to test assertions. The fix stamps each invocation with a monotonic Interlocked.Increment sequence number before awaiting the handler, then sorts by that sequence when materializing Commands. I checked the sequence is captured prior to the await _handler(...) call, so it reflects invocation order regardless of which command's handler resolves first — exactly what the new ConcurrentCommandsPreserveInvocationOrder test exercises (second-tool's handler resolves first and releases first-tool, yet Commands[0] is still asserted to be first-tool). OrderBy is a stable sort, so ties can't occur here anyway since sequence numbers are unique. This is a clean, minimal, correct fix.

d367bbb905 — pure merge from main, no functional changes for this PR. I diffed it against bfafd72964 and confirmed the only non-trivial hunks (Command.cs's DeferredCommandLoggingFailures, ModuleExecutionPipeline.cs's Status.Skipped log level) already exist on origin/main independently of this branch — they're merge noise from unrelated merged PRs, not changes originating in this PR's file set (ModularPipelines.Testing/*, File.cs, Folder.cs, CommandResult.cs, testing.md, etc.), none of which changed in this merge.

No new issues found. No CLAUDE.md violations. This PR continues to look mergeable from a correctness standpoint — all previously-flagged issues across the review history remain fixed and test-covered, and the two carried-forward architectural notes (non-blocking) still stand: InMemoryFileSystemProvider.MoveFile lacks the explicit same-source/destination guard CopyFile has (fails correctly today via the existing-file check, just with a generic error message), and the provider's continued growth (now ~870+ lines) through incremental System.IO-parity fixes is a design tradeoff worth a deliberate decision at some point.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d367bbb905

ℹ️ 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

Reviewed the single commit pushed since my last review comment (against d367bbb905): cb48f4e45 ("test(analyzers): preserve expected formatting").

Change: Updates the fixedSource fixture string in Nested_Helper_Code_Fix_Adds_Attribute_To_Enclosing_Module (src/ModularPipelines.Analyzers/ModularPipelines.Analyzers.Test/ModularPipelinesAnalyzersUnitTests.cs) to add a blank line after namespace Example; and to split the ExecuteAsync expression-bodied members onto two lines, matching the formatting the DependsOn code fix now actually preserves after the d367bbb905 merge from main (which pulled in #3613, "Preserve formatting in DependsOn code fix"). It's a test-fixture-only change — no production code touched, no new behavior introduced. I checked the source and other fixtures in the same file already use this two-line => style consistently, so the updated fixedSource is internally consistent with the rest of the test and with what the current code fix produces.

No new issues, and no CLAUDE.md violations.

This closes out the review for the PR as a whole. Across 25+ rounds, every issue raised (by this review and independently by chatgpt-codex-connector) — command interception bypassing ThrowOnNonZeroExitCode, IsDescendant root-matching, half-virtualized filesystem, non-atomic in-memory file ops, unseeded-dependency hangs, GetFiles/GetFolders losing SafeWalk pruning semantics, file/directory path collisions, parent-existence checks, read/write stream access, empty-line joining, command-recording ordering, temp-path separator parity, partial-line-write semantics — was fixed at the root cause with direct regression coverage. The three architectural notes raised along the way (identity- vs capability-based physical-metadata gating, ModuleTester reimplementing single-module execution instead of routing through ModuleRunner, and the case for a parameterized dual-provider contract-test suite as InMemoryFileSystemProvider continues to grow) remain open but are non-blocking follow-ups rather than defects in this PR. Mergeable from a correctness standpoint.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cb48f4e45f

ℹ️ 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

Re-reviewed at the current head (af021d9a2, "docs(testing): document harness outcomes"). This is the only commit since my last review comment (against cb48f4e45).

New change: adds a 2-sentence note above the "Assert skipped and failed runs" example in docs/docs/how-to/testing.md stating that the harness sets ThrowOnPipelineFailure = false (so failed modules are returned instead of thrown) and that successful runs expose Status.Successful. Verified against the implementation — ModuleTester.cs:140 sets ThrowOnPipelineFailure = false in the pipeline options, and ModuleTester.cs:277 seeds dependency results with ModuleStatus = Status.Successful. The doc text accurately describes the code. No issues.

Overall assessment: across the ~20 prior review rounds (mine and Codex's), every flagged correctness issue — command interception bypassing ThrowOnNonZeroExitCode, descendant-path matching, parent-directory requirements on file/directory creation, exclusive-handle enforcement, append-boundary protection, stream access-mode enforcement, cancellation propagation, path-traversal/whitespace probes, ZIP timestamp/extraction semantics, ordering of recorded commands, etc. — has been fixed and covered by a corresponding test. I re-checked a sample of these against the current head and they remain fixed; nothing has regressed.

One architectural point worth considering (not a blocker for this PR): the amount of iteration needed to get InMemoryFileSystemProvider (953 lines) to match System.IO's edge-case semantics — write-only stream read rejection, append-position clamping, empty-sequence line joining, whitespace/invalid path probing, directory-vs-file collision handling, exclusive-handle tracking, path-traversal boundaries — suggests hand-rolling a parity-perfect virtual filesystem is inherently whack-a-mole. The repo already references TestableIO.System.IO.Abstractions and TestableIO.System.IO.Abstractions.TestingHelpers in test/ModularPipelines.UnitTests.csproj (currently unused there), which ships a community-maintained MockFileSystem that has already solved most of these exact edge cases. A future refactor could have InMemoryFileSystemProvider adapt IFileSystemProvider on top of MockFileSystem rather than reimplementing System.IO semantics by hand — it would shrink the custom surface area to a thin adapter and offload edge-case correctness to a library that's already exercised against real System.IO behavior by a wider user base, rather than this package's own test suite discovering each gap one commit at a time. Given how mature and well-tested the current implementation now is after this review cycle, this is a "nice to have for next time" rather than something to hold up merging on.

No blocking issues. The harness design (isolated DI-built pipeline per module, ModuleTestBuilder<TModule>/ModuleTestBuilder<TModule, TResult> fluent builders, RecordingCommandInterceptor for command assertions, NoOpConsoleServices for silent output) is sound and the public surface is minimal and well-documented.

[thomhurst]

Pushed cc1e6d89de for both unresolved P2 findings. Validation: 72/72 testing-harness tests; ModularPipelines.Testing.sln Release build clean; changed-file whitespace and warning-level analyzer checks clean. Inline replies include regression details; threads stay open for the next bot cycle.

[claude]

Code review

Re-reviewed at the current head (cc1e6d89de, "fix: harden module test isolation"). This is the only commit since my last review comment (against af021d9a2), and it addresses the two chatgpt-codex-connector P2 findings that were still open at that point ("Obfuscate intercepted failures before throwing" and "Reject Windows-invalid file-name characters"). I independently verified both against the code rather than trusting the commit message:

1. Intercepted command failures now obfuscate secrets before throwing (src/ModularPipelines/Context/Command.cs:158-171)
Previously throw new CommandException(result) used the interceptor's result directly, which — after ApplyCommandMetadata — carries CommandInput/EnvironmentVariables derived from new CommandResult(command) (i.e. raw, unobfuscated command.ToString()), unlike the real-process failure path which always routes through CreateFailureResult/_secretObfuscator.Obfuscate. Now the intercepted path also calls CreateFailureResult with the intercepted result's fields, so CommandInput, StandardOutput, StandardError, and EnvironmentVariables are all obfuscated before being wrapped in CommandException. I checked this doesn't regress the non-throwing return path: a successful (or non-throwing) intercepted CommandResult still carries the raw CommandInput/EnvironmentVariables, but that's consistent with the real execution path too (Command.cs:410, new CommandResult(command, result, standardOutput, standardError), also unobfuscated) — obfuscation has only ever applied to logs and thrown exceptions, not to values handed back to the calling module, since the module already possesses whatever secrets it passed in. New test InterceptedCommandFailureObfuscatesExceptionResult exercises all four fields with a custom ISecretObfuscator. Correct fix.

2. In-memory provider now rejects platform-invalid file-name characters (src/ModularPipelines.Testing/InMemoryFileSystemProvider.cs:655-706)
New ValidateFileNameSegments splits the path (root stripped) into segments and checks each against Path.GetInvalidFileNameChars(), wired into both NormalizeDirectoryPath and NormalizeFilePath — the two funnel points every other public method already goes through, so coverage is complete. Using Path.GetInvalidFileNameChars() is the right call here since it's genuinely platform-sensitive (on Linux it's effectively just \0, so artifact?.txt legitimately stays valid there, matching SystemFileSystemProvider), and the new RejectsPlatformInvalidFileNameCharacters test is correctly gated behind OperatingSystem.IsWindows() to match. This closes the gap where a module test could pass with an output path that would always fail on a real Windows pipeline.

Validation: traced both changes end-to-end against their call sites and the pre-existing obfuscation/normalization patterns elsewhere in the same files; the author's stated 72/72 testing-harness test pass and clean Release build are consistent with what I'd expect from these diffs. No CLAUDE.md violations.

This closes out the review for the PR as a whole. Across 25+ rounds (mine and chatgpt-codex-connector's), every issue raised — command interception bypassing ThrowOnNonZeroExitCode, IsDescendant root-matching, half-virtualized filesystem, non-atomic in-memory file ops, unseeded-dependency hangs, GetFiles/GetFolders losing SafeWalk pruning semantics, file/directory path collisions, parent-existence checks, read/write stream access, empty-line joining, command-recording ordering, temp-path separator parity, partial-line-write semantics, intercepted-failure secret obfuscation, and Windows file-name validation — has been fixed at the root cause with direct regression coverage. The architectural notes raised along the way (identity- vs capability-based physical-metadata gating, ModuleTester reimplementing single-module execution instead of routing through ModuleRunner, and the case for adapting InMemoryFileSystemProvider onto a mature library like System.IO.Abstractions.TestingHelpers' MockFileSystem instead of continuing to hand-roll System.IO parity) remain open but are non-blocking follow-ups, not defects in this PR. Mergeable from a correctness standpoint.