fix(plugin): capture stderr during invocation#32207
Conversation
Capture process stderr in plugin invocations via executeCmd. Closes helm#31843 Signed-off-by: Maksym Kondratenko <[email protected]>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves plugin error reporting by capturing subprocess stderr and including it in InvokeExecError, then bubbling that richer error up to the CLI.
Changes:
- Capture and store subprocess stderr in
executeCmdand include it inInvokeExecError. - Enhance
InvokeExecError.Error()to append quoted stderr when present. - Add tests to validate stderr capture and “teeing” behavior when an existing
cmd.Stderris provided.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/cmd/load_plugins.go | Returns the enriched InvokeExecError to callers so stderr context is preserved. |
| internal/plugin/runtime_subprocess.go | Implements stderr buffering/teeing and passes captured stderr into InvokeExecError. |
| internal/plugin/error.go | Extends error type to hold stderr and renders it in the error string. |
| internal/plugin/runtime_subprocess_test.go | Adds tests for stderr capture and tee behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestExecuteCmdCapturesStderr(t *testing.T) { | ||
| cmd := exec.Command("sh", "-c", `echo "plugin error" >&2; exit 1`) | ||
| err := executeCmd(cmd, "test-plugin") |
| if eerr, ok := err.(*exec.ExitError); ok { | ||
| stderr := bytes.TrimSpace(stderrBuf.Bytes()) |
There was a problem hiding this comment.
The size is usually identical, rarely a bit smaller. Furthermore, this function is not used frequently, so the optimization would not provide much savings, but increase complexity.
| if len(e.Stderr) > 0 { | ||
| return e.Err.Error() + ": " + strconv.Quote(string(e.Stderr)) | ||
| } |
There was a problem hiding this comment.
I would leave this up to maintainers to decide. Me personally - I would be frustrated if the stderr from a failed plugin would be truncated.
Replace the sh call in plugin stderr capture tests with go native binary for cross platform test compatibility. Signed-off-by: Maksym Kondratenko <[email protected]>
Capture process stderr in plugin invocations via executeCmd.
Closes #31843
What this PR does / why we need it:
Currently, there is an issue with all types of plugins not displaying the stderr on fail, which is the main way the user is
supposed to find out the reason for the failure. This PR fixes the stderr capture for both buffer and fd backed stderr
targets.
Special notes for your reviewer:
InvokeExecError structure has been changed to include stderr.
If applicable:
docs neededlabel should be applied if so)