Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## prime #342 +/- ##
==========================================
+ Coverage 92.45% 93.60% +1.15%
==========================================
Files 26 26
Lines 1166 1173 +7
Branches 69 69
==========================================
+ Hits 1078 1098 +20
+ Misses 65 53 -12
+ Partials 23 22 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors CliWrap’s execution and streaming pipelines to ensure spawned processes are forcefully terminated when their owning execution scope is abandoned (e.g., iterator disposal / observable unsubscribe), and to make piping failure handling more deterministic.
Changes:
- Reworked
Command.ExecuteAsynccoordination between process exit and stdio piping, adding “panic” cancellation paths when pipes fail. - Updated pull-/push-based event stream implementations to forcefully terminate the process when the consumer abandons the stream.
- Broad documentation cleanup/normalization via
<inheritdoc />, plus API-internal renames and a new regression test for pipe-failure large-output hangs.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| CliWrap/Utils/ProcessEx.cs | Simplifies exit waiting by awaiting the internal exit TCS directly. |
| CliWrap/PipeTarget.cs | Refactors pipe fan-out cancellation flow and updates factory overload docs/names. |
| CliWrap/PipeSource.cs | Renames parameters for clarity and normalizes docs with <inheritdoc />. |
| CliWrap/EventStream/PushEventStreamCommandExtensions.cs | Ensures unsubscription triggers forceful termination; refactors to Wrap. |
| CliWrap/EventStream/PullEventStreamCommandExtensions.cs | Ensures iterator abandonment triggers forceful termination and joins the command task. |
| CliWrap/CommandTask.cs | Replaces Bind with Wrap to chain without awaiting and supports new call patterns. |
| CliWrap/CommandResult.cs | Doc-only updates for implicit conversion docs. |
| CliWrap/Command.PipeOperators.cs | Doc-only updates via <inheritdoc /> for operator overloads. |
| CliWrap/Command.Execution.cs | Major rework of execution lifecycle, piping task coordination, and termination semantics. |
| CliWrap/Command.cs | Doc-only update via <inheritdoc /> for WithArguments overload. |
| CliWrap/Builders/ResourcePolicyBuilder.cs | Minor wording fixes in remarks. |
| CliWrap/Builders/EnvironmentVariablesBuilder.cs | Uses <inheritdoc /> for overload documentation. |
| CliWrap/Builders/CredentialsBuilder.cs | Minor wording fixes in remarks. |
| CliWrap/Builders/ArgumentsBuilder.cs | Uses <inheritdoc /> for overload documentation and minor wording tweak. |
| CliWrap/Buffered/BufferedCommandResult.cs | Doc-only update for implicit conversion docs. |
| CliWrap/Buffered/BufferedCommandExtensions.cs | Refactors buffered execution flow to Wrap and clarifies comments. |
| CliWrap.Tests/PipingSpecs.cs | Adds regression test ensuring no hang on large output when stdout pipe throws. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
CliWrap/Command.Execution.cs:300
processTaskis awaited unconditionally infinallywith no timeout. SinceProcessEx.Kill()intentionally swallows failures, a kill that doesn’t actually terminate the process can causeExecuteAsync()to hang indefinitely (including on cancellation paths). Consider keeping a bounded wait (or otherwise surfacing a clear timeout) once forceful termination has been requested, to avoid unbounded hangs.
finally
{
// The process must never outlive the execution of this method
await processTask.ConfigureAwait(false);
}
Improves upon #339