CliWrap trialled and rejected — close the gaps in ProcessTasks instead

[ChrisonSimtian]

Decision

CliWrap was trialled in Fallout.Migrate and rejected. Process execution stays on the in-house ProcessTasks (src/Fallout.Tooling/ProcessTasks.cs). No new package dependency.

Trialled and reverted in #576, which needed to shell out to dotnet tool uninstall / dotnet tool install from SwitchGlobalToolStep.

What the comparison showed

Both versions were built and run side by side against dotnet tool list --global, a deliberately failing dotnet tool uninstall, and a 1 ms timeout. Identical exit codes, identical stdout, identical stderr, and both return null on timeout. The difference is only in what the calling code costs to write.

	Lines (non-blank)
CliWrap	18
Raw System.Diagnostics.Process, written correctly	47
ProcessTasks (what we shipped)	31

With CliWrap:

using var cancellation = new CancellationTokenSource(commandTimeout);

return await Cli.Wrap("dotnet")
    .WithArguments(arguments)
    .WithValidation(CommandResultValidation.None)
    .ExecuteBufferedAsync(cancellation.Token);

With ProcessTasks:

var argumentLine = arguments.Select(x => x.DoubleQuoteIfNeeded()).JoinSpace();

using var process = ProcessTasks.StartProcess(
    "dotnet", argumentLine, timeout: (int)commandTimeout.TotalMilliseconds,
    logOutput: false, logInvocation: false);

if (process == null || !process.WaitForExit())
    return null;

return new CommandResult(
    process.ExitCode,
    Join(process.Output, OutputType.Std),
    Join(process.Output, OutputType.Err));

What CliWrap actually solves

Worth recording, because these are the traps a hand-rolled runner falls into:

1. The pipe-buffer deadlock. The obvious raw implementation is WaitForExit() then StandardOutput.ReadToEnd(). That hangs once the child fills the ~64KB pipe buffer while the parent is blocked in WaitForExit. Avoiding it needs event-based reads plus counting two EOF sentinels. ProcessTasks already handles this, which is the main reason it was a viable replacement.
2. Cancellation that actually kills. WaitForExitAsync(token) returns on timeout but leaves the child running. An explicit Kill(entireProcessTree: true) is required or a timeout orphans the process.
3. A single result object carrying exit code plus both streams.

What CliWrap does not solve any more

Its reputation is dated on two points:

• Argument escaping — ProcessStartInfo.ArgumentList has handled quoting since .NET Core 2.1.
• Async — WaitForExitAsync has been in the BCL since .NET 5.

What using ProcessTasks instead costs

Honest list, since these are the gaps a future improvement would close:

• No CancellationToken. Only an int millisecond timeout.
• Synchronous only. No WaitForExitAsync, so a step that is otherwise async has to wrap it.
• Arguments are one pre-quoted string. No ArgumentList equivalent, so every caller does its own DoubleQuoteIfNeeded(). This is the one place the raw BCL is now better than our own layer.
• Output is one interleaved collection filtered by OutputType, rather than separate stdout and stderr.
• Logs through Serilog by default. A tool that renders through Spectre must pass logOutput: false, logInvocation: false.
• Asserts instead of returning null when the executable is not on PATH, so callers need a try/catch.
• Pulls Serilog and NuGet.Packaging into anything referencing Fallout.Tooling. For Fallout.Migrate this swapped one direct dependency for two transitive ones.

Acceptance criteria

• ProcessTasks (or its successor) accepts a CancellationToken alongside the millisecond timeout.
• An async overload exists, so callers do not have to block a thread.
• An argument-list overload exists that quotes each argument, instead of every caller calling DoubleQuoteIfNeeded().
• Not finding the executable on PATH is reportable without a try/catch around the call.
• docs/dependencies.md keeps pointing here as the reason there is no third-party process runner.

Notes

• Related: [Foundation] FT-5: Context-scope tool-path resolvers and process defaults #310 (FT-5, context-scope tool-path resolvers and process defaults) is the natural home for the resolver half of this.
• Do not re-litigate adding CliWrap without new information. The measured difference is 18 lines against 31, for one call site.

[ChrisonSimtian]

not sure what happened to the original issue, but here we go again.
My idea was to replace our home baked Process wrapper with CliWrap simply so that we dont have to maintain something thats not focused on our core IP: building and delivering dotnet code.
There might be some friction in this particular case because we DO use a lot of Cli, so maybe its justified to make our own. But maybe we can also learn and adopt some things from CliWrap