From c455d51d7edfdf06d62ae596b5957f7554b1c9b7 Mon Sep 17 00:00:00 2001 From: "devsy-app[bot]" <277138668+devsy-app[bot]@users.noreply.github.com> Date: Sun, 16 Aug 2026 10:49:25 +0000 Subject: [PATCH 1/2] test(ssh): cover helper error classification Extract isSignalInterrupt pure function from handleRunError (behavior-preserving) and add helper_test.go covering ExitError, RunOptions.validate, isSignalInterrupt, handleRunError, and setupContextCancellation. --- pkg/ssh/helper.go | 24 +++++--- pkg/ssh/helper_test.go | 129 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 146 insertions(+), 7 deletions(-) create mode 100644 pkg/ssh/helper_test.go diff --git a/pkg/ssh/helper.go b/pkg/ssh/helper.go index 7c0b36dc9..4f3e368f9 100644 --- a/pkg/ssh/helper.go +++ b/pkg/ssh/helper.go @@ -198,13 +198,10 @@ func handleRunError(ctx context.Context, err error, command string) error { if errors.As(err, &exitErr) { exitCode := exitErr.ExitStatus() - // Exit codes 128+N indicate death by signal N - // 130 = 128 + 2 (SIGINT) - Ctrl+C (user interrupted) - // 129 = 128 + 1 (SIGHUP) - hangup (terminal closed) - // 143 = 128 + 15 (SIGTERM) - graceful termination - // These are "normal" ways to exit an interactive session - if exitCode == 130 || exitCode == 129 || exitCode == 143 { - return nil // Don't treat user interrupts as errors + // Signal-driven exits are normal ways to end an interactive session, + // so they are not treated as errors. + if isSignalInterrupt(exitCode) { + return nil } // Return exit code for all other cases @@ -221,3 +218,16 @@ func handleRunError(ctx context.Context, err error, command string) error { return fmt.Errorf("SSH command failed while running %s: %w", command, err) } + +// isSignalInterrupt reports whether an exit code corresponds to a process +// terminated by a signal that ends an interactive session normally. +// Exit codes follow the 128+N convention: 130 = SIGINT, 129 = SIGHUP, +// 143 = SIGTERM. +func isSignalInterrupt(exitCode int) bool { + switch exitCode { + case 130, 129, 143: + return true + default: + return false + } +} diff --git a/pkg/ssh/helper_test.go b/pkg/ssh/helper_test.go new file mode 100644 index 000000000..05f321ce9 --- /dev/null +++ b/pkg/ssh/helper_test.go @@ -0,0 +1,129 @@ +package ssh + +import ( + "context" + "errors" + "io" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/crypto/ssh" +) + +func TestExitError_ErrorIncludesErrWhenSet(t *testing.T) { + inner := errors.New("permission denied") + err := &ExitError{ExitCode: 1, Err: inner} + + assert.Equal(t, "exit status 1: permission denied", err.Error()) +} + +func TestExitError_ErrorOmitsErrWhenNil(t *testing.T) { + err := &ExitError{ExitCode: 127} + + assert.Equal(t, "exit status 127", err.Error()) +} + +func TestExitError_UnwrapReturnsWrappedErr(t *testing.T) { + inner := errors.New("boom") + err := &ExitError{ExitCode: 2, Err: inner} + + require.ErrorIs(t, err, inner) + assert.Same(t, inner, errors.Unwrap(err)) +} + +func TestRunOptions_ValidateRequiresClient(t *testing.T) { + err := (&RunOptions{Command: "ls"}).validate() + + require.Error(t, err) + assert.Contains(t, err.Error(), "SSH client is required") +} + +func TestRunOptions_ValidateRequiresCommand(t *testing.T) { + err := (&RunOptions{Client: &ssh.Client{}}).validate() + + require.Error(t, err) + assert.Contains(t, err.Error(), "command is required") +} + +func TestRunOptions_ValidatePassesForClientAndCommand(t *testing.T) { + err := (&RunOptions{Client: &ssh.Client{}, Command: "ls"}).validate() + + assert.NoError(t, err) +} + +func TestIsSignalInterrupt(t *testing.T) { + signalExits := []int{130, 129, 143} + for _, code := range signalExits { + assert.True(t, isSignalInterrupt(code), "exit code %d should be a signal interrupt", code) + } + + nonSignalExits := []int{0, 1, 2, 127, 128, 131, 142, 144, 255, -1} + for _, code := range nonSignalExits { + assert.False( + t, + isSignalInterrupt(code), + "exit code %d should not be a signal interrupt", + code, + ) + } +} + +func TestHandleRunError_CancelledContextReturnsContextErr(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err := handleRunError(ctx, io.EOF, "cmd") + + require.ErrorIs(t, err, context.Canceled) +} + +func TestHandleRunError_EOFIsWrappedWithCommand(t *testing.T) { + err := handleRunError(context.Background(), io.EOF, "build") + + require.ErrorIs(t, err, io.EOF) + assert.Contains(t, err.Error(), "SSH session closed unexpectedly while running build") +} + +func TestHandleRunError_GenericErrorIsWrappedWithCommand(t *testing.T) { + inner := errors.New("connection reset") + err := handleRunError(context.Background(), inner, "test") + + require.ErrorIs(t, err, inner) + assert.Contains(t, err.Error(), "SSH command failed while running test") +} + +func TestHandleRunError_ExitErrorIsWrappedInDevsyExitError(t *testing.T) { + // *ssh.ExitError embeds Waitmsg, whose zero value yields ExitStatus() == 0, + // so it is not a signal interrupt and must be wrapped in our ExitError. + sshExitErr := &ssh.ExitError{} + + err := handleRunError(context.Background(), sshExitErr, "run") + + var devsyExit *ExitError + require.ErrorAs(t, err, &devsyExit) + assert.Equal(t, 0, devsyExit.ExitCode) + require.ErrorIs(t, err, sshExitErr) +} + +func TestSetupContextCancellation_AlreadyCancelledReturnsError(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + cleanup, err := setupContextCancellation(ctx, nil) + + require.Error(t, err) + require.ErrorIs(t, err, context.Canceled) + assert.Nil(t, cleanup) +} + +func TestSetupContextCancellation_ReturnsCleanupThatStopsWatcher(t *testing.T) { + cleanup, err := setupContextCancellation(context.Background(), nil) + + require.NoError(t, err) + require.NotNil(t, cleanup) + + // Calling cleanup closes the watcher's done channel so the goroutine exits + // without ever touching the session. It must be safe to call exactly once. + assert.NotPanics(t, cleanup) +} From cce6d80987b71e793cb557ce6daa50c69a33cc0d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 17 Aug 2026 05:37:45 +0000 Subject: [PATCH 2/2] style: update comments Signed-off-by: Samuel K --- pkg/ssh/helper.go | 2 -- pkg/ssh/helper_test.go | 4 ---- 2 files changed, 6 deletions(-) diff --git a/pkg/ssh/helper.go b/pkg/ssh/helper.go index 4f3e368f9..3c6de53d9 100644 --- a/pkg/ssh/helper.go +++ b/pkg/ssh/helper.go @@ -198,8 +198,6 @@ func handleRunError(ctx context.Context, err error, command string) error { if errors.As(err, &exitErr) { exitCode := exitErr.ExitStatus() - // Signal-driven exits are normal ways to end an interactive session, - // so they are not treated as errors. if isSignalInterrupt(exitCode) { return nil } diff --git a/pkg/ssh/helper_test.go b/pkg/ssh/helper_test.go index 05f321ce9..6412e6aaa 100644 --- a/pkg/ssh/helper_test.go +++ b/pkg/ssh/helper_test.go @@ -94,8 +94,6 @@ func TestHandleRunError_GenericErrorIsWrappedWithCommand(t *testing.T) { } func TestHandleRunError_ExitErrorIsWrappedInDevsyExitError(t *testing.T) { - // *ssh.ExitError embeds Waitmsg, whose zero value yields ExitStatus() == 0, - // so it is not a signal interrupt and must be wrapped in our ExitError. sshExitErr := &ssh.ExitError{} err := handleRunError(context.Background(), sshExitErr, "run") @@ -123,7 +121,5 @@ func TestSetupContextCancellation_ReturnsCleanupThatStopsWatcher(t *testing.T) { require.NoError(t, err) require.NotNil(t, cleanup) - // Calling cleanup closes the watcher's done channel so the goroutine exits - // without ever touching the session. It must be safe to call exactly once. assert.NotPanics(t, cleanup) }