v2 review fixes for Pipe/runner branch#63
Open
znull wants to merge 5 commits into
Open
Conversation
Ensure pre-start validation failures cancel the derived pipeline context, and include the offending stage name in invalid stream-requirement errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Release any previously configured stdout stream before replacing it with the buffer used by Output, preserving ownership of WithStdoutCloser. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add Function options for declaring stdin and stdout stream requirements, so Function stages can participate in file-preference negotiation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Explain the known deadlock risk for borrowed non-file stdin readers feeding a command that exits without draining stdin. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address gocritic, revive, and unused-code findings in the Pipe/runner helper code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR ports review-driven fixes onto the Pipe / runner / Config branch, tightening lifecycle cleanup and making stream-requirement negotiation and related error reporting more robust and user-facing.
Changes:
- Ensure pipeline context is canceled on pre-start failures and include stage names in invalid stream requirement errors.
- Make
Output()close any previously configuredWithStdoutCloserdestination before swapping stdout to an internal capture buffer. - Expose
WithStdinRequirement/WithStdoutRequirementforFunctionstages, and document a known stdin limitation for non-file readers feeding command stages.
Show a summary per file
| File | Description |
|---|---|
| pipe/runner.go | Closes any previously configured stdout closer before capturing output into a buffer. |
| pipe/pipeline.go | Adds a lifecycle guard to prevent calling Output() after the pipeline has started. |
| pipe/pipeline_test.go | Adds/updates tests for stdout-closer behavior, function stream requirements, and improved invalid-requirement error messages. |
| pipe/pipe.go | Cancels derived context on startup failures and improves invalid stream requirement errors with stage names. |
| pipe/options.go | Documents a known limitation when using borrowed non-file stdin readers with command stages. |
| pipe/function.go | Exposes function-stage stream requirement options and reuses them in forbid helpers. |
| pipe/event_error.go | Minor control-flow cleanup in Error() formatting. |
| pipe/config.go | Fixes option-slice append logic to preserve Config immutability. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a rebased version of #62 targeted at #61: Fixes issues discovered doing a review of the latest stage-v2-clean state.
/cc @mhagger
Copilot Summary
This PR ports the v2 review-fix commits onto PR #61's
Pipe/runner/Configbranch:Pipecontext on pre-start failures and include the offending stage name in invalid stream requirement errors.Output()replaces stdout with its capture buffer, adapted torunner.output()/Pipeline.Output().WithStdinRequirementandWithStdoutRequirementsoFunctionstages can participate in stream requirement negotiation without reimplementingStage.