diff --git a/pkg/workflow/github_cli.go b/pkg/workflow/github_cli.go index 787d33abfca..f52fb7a23a8 100644 --- a/pkg/workflow/github_cli.go +++ b/pkg/workflow/github_cli.go @@ -96,11 +96,12 @@ func enrichGHError(err error) error { return err } -// runGHWithSpinnerContext executes a gh CLI command with context support, a spinner, -// and returns the output. This is the core implementation for RunGHContext. -func runGHWithSpinnerContext(ctx context.Context, spinnerMessage string, combined bool, args ...string) ([]byte, error) { - cmd := ExecGHContext(ctx, args...) - +// runGHWithSpinnerCmd executes an already-constructed *exec.Cmd with an optional spinner +// and returns the output. This is the single shared execution path for all RunGH* variants, +// covering TTY/non-TTY branches, stdout-only and combined stdout+stderr modes, and error +// enrichment. Keeping the logic here guarantees that context and non-context callers behave +// identically. +func runGHWithSpinnerCmd(cmd *exec.Cmd, spinnerMessage string, combined bool) ([]byte, error) { // Show spinner in interactive terminals if tty.IsStderrTerminal() { spinner := console.NewSpinner(spinnerMessage) @@ -124,32 +125,16 @@ func runGHWithSpinnerContext(ctx context.Context, spinnerMessage string, combine return output, enrichGHError(err) } +// runGHWithSpinnerContext executes a gh CLI command with context support, a spinner, +// and returns the output. This is a thin wrapper around runGHWithSpinnerCmd for RunGHContext. +func runGHWithSpinnerContext(ctx context.Context, spinnerMessage string, combined bool, args ...string) ([]byte, error) { + return runGHWithSpinnerCmd(ExecGHContext(ctx, args...), spinnerMessage, combined) +} + // runGHWithSpinner executes a gh CLI command with a spinner and returns the output. -// This is the core implementation shared by RunGH and RunGHCombined. +// This is a thin wrapper around runGHWithSpinnerCmd shared by RunGH and RunGHCombined. func runGHWithSpinner(spinnerMessage string, combined bool, args ...string) ([]byte, error) { - cmd := ExecGH(args...) - - // Show spinner in interactive terminals - if tty.IsStderrTerminal() { - spinner := console.NewSpinner(spinnerMessage) - spinner.Start() - var output []byte - var err error - if combined { - output, err = cmd.CombinedOutput() - } else { - output, err = cmd.Output() - err = enrichGHError(err) - } - spinner.Stop() - return output, err - } - - if combined { - return cmd.CombinedOutput() - } - output, err := cmd.Output() - return output, enrichGHError(err) + return runGHWithSpinnerCmd(ExecGH(args...), spinnerMessage, combined) } // RunGH executes a gh CLI command with a spinner and returns the stdout output. @@ -207,18 +192,7 @@ func RunGHCombinedContext(ctx context.Context, spinnerMessage string, args ...st func RunGHWithHost(spinnerMessage string, host string, args ...string) ([]byte, error) { cmd := ExecGH(args...) SetGHHostEnv(cmd, host) - - if tty.IsStderrTerminal() { - spinner := console.NewSpinner(spinnerMessage) - spinner.Start() - output, err := cmd.Output() - err = enrichGHError(err) - spinner.Stop() - return output, err - } - - output, err := cmd.Output() - return output, enrichGHError(err) + return runGHWithSpinnerCmd(cmd, spinnerMessage, false) } // SetGHHostEnv sets the GH_HOST environment variable on the command for non-github.com hosts. diff --git a/pkg/workflow/github_cli_test.go b/pkg/workflow/github_cli_test.go index 89574c3637a..c8efb20751f 100644 --- a/pkg/workflow/github_cli_test.go +++ b/pkg/workflow/github_cli_test.go @@ -341,6 +341,61 @@ func TestSetupGHCommand(t *testing.T) { } } +// TestRunGHWithSpinnerCmd verifies that runGHWithSpinnerCmd runs a command and returns +// the expected output for both combined=false and combined=true modes. It also verifies +// that error enrichment is applied for stdout-only mode (combined=false). +func TestRunGHWithSpinnerCmd(t *testing.T) { + t.Run("stdout mode returns stdout", func(t *testing.T) { + cmd := exec.Command("sh", "-c", "echo hello") + out, err := runGHWithSpinnerCmd(cmd, "test", false) + require.NoError(t, err, "command should succeed") + assert.Contains(t, string(out), "hello", "should capture stdout") + }) + + t.Run("combined mode returns stdout and stderr", func(t *testing.T) { + cmd := exec.Command("sh", "-c", "echo out; echo err >&2") + out, err := runGHWithSpinnerCmd(cmd, "test", true) + require.NoError(t, err, "command should succeed") + assert.Contains(t, string(out), "out", "should capture stdout in combined mode") + assert.Contains(t, string(out), "err", "should capture stderr in combined mode") + }) + + t.Run("stdout mode enriches error with stderr", func(t *testing.T) { + cmd := exec.Command("sh", "-c", "echo 'oh no' >&2; exit 1") + _, err := runGHWithSpinnerCmd(cmd, "test", false) + require.Error(t, err, "command should fail") + assert.Contains(t, err.Error(), "oh no", "error should be enriched with stderr") + }) + + t.Run("combined mode does not double-enrich error", func(t *testing.T) { + // combined=true captures stderr in output; enrichGHError is intentionally not called, + // so the error returned is the plain *exec.ExitError without stderr appended to it. + cmd := exec.Command("sh", "-c", "echo msg >&2; exit 1") + out, err := runGHWithSpinnerCmd(cmd, "test", true) + require.Error(t, err, "command should fail") + assert.Contains(t, string(out), "msg", "stderr should appear in combined output") + // The error itself must NOT contain the stderr text (no enrichment for combined mode). + assert.NotContains(t, err.Error(), "msg", "combined mode should not enrich the error with stderr") + }) +} + +// TestRunGHWithSpinnerContextParity verifies that ExecGH and ExecGHContext produce commands +// with identical arguments so that runGHWithSpinner and runGHWithSpinnerContext are truly +// interchangeable for the same inputs. +func TestCommandConstructionParity(t *testing.T) { + t.Run("stdout-only mode parity", func(t *testing.T) { + // Both wrappers should produce commands with the same program and arguments so that + // runGHWithSpinnerCmd delivers identical behaviour regardless of how the cmd was built. + args := []string{"api", "/user"} + ctx := context.Background() + cmdNoCtx := ExecGH(args...) + cmdCtx := ExecGHContext(ctx, args...) + + // Both should have the same program and arguments. + assert.Equal(t, cmdNoCtx.Args, cmdCtx.Args, "context and non-context commands should have identical args") + }) +} + // TestRunGHWithSpinner tests the core runGHWithSpinner function // Note: This test validates the function exists and handles arguments correctly // Actual spinner behavior is tested via RunGH and RunGHCombined