Follow ups on the RunCommandCaptureAll / RunSensitiveCommandCaptureAll API added in #94. All verified against a build of master.
Capturing output gives you a step that can never fail
The shape in the README is:
run (fun ctx -> async {
let! output = ctx.RunCommandCaptureAll "dotnet --version"
printfn "%d" output.ExitCode
})That returns Async<unit>, so the stage always reports success. Measured end to end with the same failing command:
| call |
pipeline exit code |
ctx.RunCommand |
1 |
ctx.RunCommandCaptureAll |
0 |
Opting back in is awkward: IsAcceptableExitCode and MapExitCodeToResult live in StageContextExtensionsInternal, which is not [<AutoOpen>], so a user who set acceptExitCodes [0; 1] has no supported way to honour it and will hand-roll if ExitCode <> 0. Moving those two members to the auto opened module would close this, and make return ctx.MapExitCodeToResult output.ExitCode the documented ending for the block above.
Cancellation is indistinguishable from failure
RunCommand and RunCommandCaptureOutput both return Ok when ct.IsCancellationRequested, which StageBuilder documents as "can be used to cancel a command and mark it as success". Neither CaptureAll member does this check, and CommandOutput has no cancellation flag. A cancelled command comes back as ExitCode = 143 on Linux or -1 on Windows with a truncated stdout prefix, and the caller cannot tell that from a genuine failure.
Process.StartAsync is a breaking change
The return type went from struct {| ExitCode: int; StandardOutput: string |} to the reference record CommandOutput. The member is public on System.Diagnostics.Process through the [<AutoOpen>] Fun.Build.ProcessExtensions module, so this is both source and binary breaking: an assembly compiled against 1.1.18 that calls it throws MissingMethodException against the new dll. Worth a changelog note and a version bump beyond a patch.
Edited to drop three items I no longer think are worth acting on:
- "
run (fun ctx -> ctx.RunCommandCaptureAll "echo hi") does not compile." It shouldn't. The natural call for "run a command and fail on a bad exit code" is run "echo hi". RunCommandCaptureAll is for when you want the output, and capturing forces a redirect, which loses the child's colours, so an overload would only make a strictly worse call compile. The FS0041 is the type system pointing at the right member.
- "Captured output gains a newline the command never emitted." True, but
.Trim() already handles it, and every common command (echo, git rev-parse, dotnet --version) emits a trailing newline anyway. Fixing it properly means reading the raw stream instead of OutputDataReceived, which stops splitting on a bare \r and turns progress output from docker pull, npm install and friends into one buffered blob. Not worth it.
- "Two adjacent string parameters at a security boundary." Still true of
RunCommandCaptureAllInternal, and still only reachable from two internal call sites. Making it unrepresentable means a public DU in the signature of a public extension on System.Diagnostics.Process, which is a lot of public API for a footgun nobody has hit.
Follow ups on the
RunCommandCaptureAll/RunSensitiveCommandCaptureAllAPI added in #94. All verified against a build of master.Capturing output gives you a step that can never fail
The shape in the README is:
That returns
Async<unit>, so the stage always reports success. Measured end to end with the same failing command:ctx.RunCommandctx.RunCommandCaptureAllOpting back in is awkward:
IsAcceptableExitCodeandMapExitCodeToResultlive inStageContextExtensionsInternal, which is not[<AutoOpen>], so a user who setacceptExitCodes [0; 1]has no supported way to honour it and will hand-rollif ExitCode <> 0. Moving those two members to the auto opened module would close this, and makereturn ctx.MapExitCodeToResult output.ExitCodethe documented ending for the block above.Cancellation is indistinguishable from failure
RunCommandandRunCommandCaptureOutputboth returnOkwhenct.IsCancellationRequested, whichStageBuilderdocuments as "can be used to cancel a command and mark it as success". Neither CaptureAll member does this check, andCommandOutputhas no cancellation flag. A cancelled command comes back asExitCode = 143on Linux or-1on Windows with a truncated stdout prefix, and the caller cannot tell that from a genuine failure.Process.StartAsyncis a breaking changeThe return type went from
struct {| ExitCode: int; StandardOutput: string |}to the reference recordCommandOutput. The member is public onSystem.Diagnostics.Processthrough the[<AutoOpen>]Fun.Build.ProcessExtensionsmodule, so this is both source and binary breaking: an assembly compiled against 1.1.18 that calls it throwsMissingMethodExceptionagainst the new dll. Worth a changelog note and a version bump beyond a patch.Edited to drop three items I no longer think are worth acting on:
run (fun ctx -> ctx.RunCommandCaptureAll "echo hi")does not compile." It shouldn't. The natural call for "run a command and fail on a bad exit code" isrun "echo hi".RunCommandCaptureAllis for when you want the output, and capturing forces a redirect, which loses the child's colours, so an overload would only make a strictly worse call compile. TheFS0041is the type system pointing at the right member..Trim()already handles it, and every common command (echo,git rev-parse,dotnet --version) emits a trailing newline anyway. Fixing it properly means reading the raw stream instead ofOutputDataReceived, which stops splitting on a bare\rand turns progress output fromdocker pull,npm installand friends into one buffered blob. Not worth it.RunCommandCaptureAllInternal, and still only reachable from two internal call sites. Making it unrepresentable means a public DU in the signature of a public extension onSystem.Diagnostics.Process, which is a lot of public API for a footgun nobody has hit.