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: d1fd25d40f

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

Addressed the three latest testing-harness findings in 4599871: AlwaysRun tests honor direct caller cancellation without inheriting production engine-failure cancellation; command timeouts now span interceptors and process execution; OpenRead retains shared-reader semantics while generic Open stays exclusive. Validation: ModularPipelines.Testing.UnitTests 37/37; CommandTests 16/16; Testing solution Release build 0 errors; changed production files warning-free. @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 459987170d

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

Closed four runtime-parity gaps in 0a6b816: exclusive handles now block direct reads/copies, interceptor timeouts are rechecked after handlers return, and physical ZIP create/extract preserves file timestamps. Validation: Testing suite 39/39; ZipTests 5/5; CommandTests 16/16; scoped whitespace clean. @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0a6b81647b

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 71a9361ba8

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4ce822ea80

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 49500511bf

ℹ️ 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: 3af70686f7

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

Use CreateNew when overwrite is disabled

When overwriteFiles is false and another process or parallel module creates destinationPath after the existence check, opening it with FileMode.Create silently truncates that new file instead of reporting the required conflict. The previous ExtractToFile(..., overwrite: false) path performed this check atomically; select FileMode.CreateNew when overwriting is disabled.

Useful? React with 👍 / 👎.

[thomhurst]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0d7b7247d9

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

Treat backslashes as separators only on Windows

On Unix, \ is a valid filename character rather than a directory separator, so this classifies a sibling such as /tmp/root\outside/file.txt as a descendant of /tmp/root. Consequently recursive enumeration, deletion, or directory moves can include and mutate that sibling even though the system filesystem would not; use the platform directory separator rules when checking the boundary.

Useful? React with 👍 / 👎.