diff --git a/main.go b/main.go index 66b4d97e9..8047a202d 100644 --- a/main.go +++ b/main.go @@ -1,6 +1,7 @@ package main import ( + stderrors "errors" "os" "github.com/brevdev/brev-cli/pkg/analytics" @@ -16,6 +17,12 @@ func main() { command := cmd.NewDefaultBrevCommand() if err := command.Execute(); err != nil { + // Not a CLI error: pass the remote command's exit code straight through. + var remoteErr errors.RemoteExitError + if stderrors.As(err, &remoteErr) { + done() + os.Exit(remoteErr.Code) //nolint:gocritic // manually call done + } analytics.CaptureCommandError() cmderrors.DisplayAndHandleError(err) done() diff --git a/pkg/cmd/exec/exec.go b/pkg/cmd/exec/exec.go index e7f0cd924..1cfa7f6b1 100644 --- a/pkg/cmd/exec/exec.go +++ b/pkg/cmd/exec/exec.go @@ -2,6 +2,7 @@ package exec import ( "bufio" + stderrors "errors" "fmt" "os" "os/exec" @@ -52,6 +53,7 @@ type ExecStore interface { refresh.RefreshStore GetOrganizations(options *store.GetOrganizationsOptions) ([]entity.Organization, error) GetWorkspaces(organizationID string, options *store.GetWorkspacesOptions) ([]entity.Workspace, error) + GetAuthTokens() (*entity.AuthTokens, error) } func NewCmdExec(t *terminal.Terminal, store ExecStore, noLoginStartStore ExecStore) *cobra.Command { @@ -86,6 +88,11 @@ func NewCmdExec(t *terminal.Terminal, store ExecStore, noLoginStartStore ExecSto return breverrors.NewValidationError("command is required") } + // Heads-up only: exec can still succeed without credentials if the SSH config is warm. + if hasNoSavedCredentials(store) { + fmt.Fprintf(os.Stderr, "No saved Brev credentials. Trying with your existing SSH config; you'll be prompted to log in if it fails.\n") + } + // Run on each instance var errors error for _, instanceName := range instanceNames { @@ -107,7 +114,7 @@ func NewCmdExec(t *terminal.Terminal, store ExecStore, noLoginStartStore ExecSto } } if errors != nil { - return breverrors.WrapAndTrace(errors) + return breverrors.WrapAndTrace(flattenMultiInstanceErr(errors)) } return nil }, @@ -172,8 +179,45 @@ func parseCommand(command string) (string, error) { return command, nil } +// flattenMultiInstanceErr drops error types so one instance's exit code can't become the process's. +func flattenMultiInstanceErr(err error) error { + if err == nil { + return nil + } + return stderrors.New(err.Error()) +} + +type authTokenGetter interface { + GetAuthTokens() (*entity.AuthTokens, error) +} + +// hasNoSavedCredentials reports whether credentials are missing or empty; it does not validate them. +func hasNoSavedCredentials(sstore authTokenGetter) bool { + tokens, err := sstore.GetAuthTokens() + if err != nil { + var notFound *breverrors.CredentialsFileNotFound + return stderrors.As(err, ¬Found) + } + if tokens == nil { + return true + } + return tokens.AccessToken == "" && tokens.RefreshToken == "" && strings.TrimSpace(tokens.APIKey) == "" +} + const pollTimeout = 10 * time.Minute +// sshConnectionFailedExitCode is ssh's own failure code; any other code is the remote command's. +const sshConnectionFailedExitCode = 255 + +// exitCodeOf returns the process exit code for err, or -1 if err is not an exit error. +func exitCodeOf(err error) int { + var exitErr *exec.ExitError + if stderrors.As(err, &exitErr) { + return exitErr.ExitCode() + } + return -1 +} + func runExecCommand(t *terminal.Terminal, sstore ExecStore, workspaceNameOrID string, host bool, command string) error { // Determine SSH alias: use the workspace name directly (with -host suffix if needed) sshName := workspaceNameOrID @@ -192,6 +236,12 @@ func runExecCommand(t *terminal.Terminal, sstore ExecStore, workspaceNameOrID st return nil } + // The connection worked and the command itself failed, so skip the recovery path. + var remoteErr breverrors.RemoteExitError + if stderrors.As(err, &remoteErr) { + return remoteErr + } + // SSH failed — now check what's going on with the instance fmt.Fprintf(os.Stderr, "Connection failed, checking instance status...\n") @@ -228,12 +278,7 @@ func runExecCommand(t *terminal.Terminal, sstore ExecStore, workspaceNameOrID st if err != nil { return breverrors.WrapAndTrace(err) } - err = runSSH(sshName, command) - if err != nil { - return breverrors.WrapAndTrace(err) - } - go trackExecAnalytics(sstore, workspaceNameOrID) - return nil + return runAndTrack(sstore, sshName, workspaceNameOrID, command) } if workspace.Status != "RUNNING" { @@ -261,12 +306,22 @@ func runExecCommand(t *terminal.Terminal, sstore ExecStore, workspaceNameOrID st "could not connect to instance %q: %w\nPlease check with: brev ls", workspaceNameOrID, err)) } - err = runSSH(sshName, command) - if err != nil { - return breverrors.WrapAndTrace(err) + return runAndTrack(sstore, sshName, workspaceNameOrID, command) +} + +// runAndTrack runs the command after recovery. +func runAndTrack(sstore ExecStore, sshName string, workspaceNameOrID string, command string) error { + err := runSSH(sshName, command) + if err == nil { + go trackExecAnalytics(sstore, workspaceNameOrID) + return nil + } + + var remoteErr breverrors.RemoteExitError + if stderrors.As(err, &remoteErr) { + return remoteErr } - go trackExecAnalytics(sstore, workspaceNameOrID) - return nil + return breverrors.WrapAndTrace(err) } func trackExecAnalytics(sstore ExecStore, workspaceNameOrID string) { @@ -296,18 +351,26 @@ func runSSHWithTimeout(sshAlias string, command string, connectTimeoutSecs int) // -T disables pseudo-terminal allocation (no "Pseudo-terminal will not be allocated" warning) // Only start ssh-agent if one isn't already running (avoids orphaned agent processes) agentCmd := `if [ -z "$SSH_AUTH_SOCK" ]; then eval $(ssh-agent -s) > /dev/null; fi` - cmd := fmt.Sprintf("%s && ssh -T -o ConnectTimeout=%d -o LogLevel=ERROR %s '%s'", agentCmd, connectTimeoutSecs, sshAlias, escapedCmd) + // exec replaces bash with ssh so the exit code we see is ssh's own, not bash's. + cmd := fmt.Sprintf("%s && exec ssh -T -o ConnectTimeout=%d -o LogLevel=ERROR %s '%s'", agentCmd, connectTimeoutSecs, sshAlias, escapedCmd) sshCmd := exec.Command("bash", "-c", cmd) //nolint:gosec //cmd is user input sshCmd.Stderr = os.Stderr sshCmd.Stdout = os.Stdout // Don't attach stdin - exec is non-interactive - err := sshCmd.Run() - if err != nil { - return breverrors.WrapAndTrace(err) + return classifySSHError(sshCmd.Run()) +} + +// classifySSHError separates a remote command failure from an ssh connection failure. +func classifySSHError(err error) error { + if err == nil { + return nil + } + if code := exitCodeOf(err); code > 0 && code != sshConnectionFailedExitCode { + return breverrors.RemoteExitError{Code: code} } - return nil + return breverrors.WrapAndTrace(err) } func runSSH(sshAlias string, command string) error { diff --git a/pkg/cmd/exec/exec_test.go b/pkg/cmd/exec/exec_test.go new file mode 100644 index 000000000..e007d2e6a --- /dev/null +++ b/pkg/cmd/exec/exec_test.go @@ -0,0 +1,111 @@ +package exec + +import ( + stderrors "errors" + "os/exec" + "strconv" + "testing" + + "github.com/brevdev/brev-cli/pkg/entity" + breverrors "github.com/brevdev/brev-cli/pkg/errors" + "github.com/hashicorp/go-multierror" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// exitErrWithCode returns a real *exec.ExitError carrying the given code. +func exitErrWithCode(t *testing.T, code int) *exec.ExitError { + t.Helper() + var exitErr *exec.ExitError + err := exec.Command("bash", "-c", "exit "+strconv.Itoa(code)).Run() + require.ErrorAs(t, err, &exitErr) + return exitErr +} + +func TestClassifySSHError(t *testing.T) { + assert.NoError(t, classifySSHError(nil)) + + // A remote command's own exit code means the connection worked. + for _, code := range []int{1, 2, 7, 127} { + var remoteErr breverrors.RemoteExitError + err := classifySSHError(exitErrWithCode(t, code)) + require.True(t, stderrors.As(err, &remoteErr), "code %d should be a RemoteExitError", code) + assert.Equal(t, code, remoteErr.Code) + } + + // 255 is ssh's own failure code, so it must stay a connection error. + var remoteErr breverrors.RemoteExitError + err := classifySSHError(exitErrWithCode(t, sshConnectionFailedExitCode)) + require.Error(t, err) + assert.False(t, stderrors.As(err, &remoteErr), "255 must not be treated as a remote exit") +} + +func TestExitCodeOf(t *testing.T) { + assert.Equal(t, 3, exitCodeOf(exitErrWithCode(t, 3))) + assert.Equal(t, -1, exitCodeOf(stderrors.New("not an exit error"))) + assert.Equal(t, -1, exitCodeOf(nil)) +} + +// A multi-instance run must not exit with one instance's remote code. +func TestFlattenMultiInstanceErr(t *testing.T) { + assert.NoError(t, flattenMultiInstanceErr(nil)) + + agg := multierror.Append(nil, breverrors.RemoteExitError{Code: 3}, breverrors.RemoteExitError{Code: 7}) + flat := flattenMultiInstanceErr(agg) + + var remoteErr breverrors.RemoteExitError + require.Error(t, flat) + assert.False(t, stderrors.As(flat, &remoteErr), "no exit code may leak from a multi-instance run") + // The per-instance detail is still readable in the message. + assert.Contains(t, flat.Error(), "status 3") + assert.Contains(t, flat.Error(), "status 7") +} + +type fakeTokenStore struct { + tokens *entity.AuthTokens + err error +} + +func (f fakeTokenStore) GetAuthTokens() (*entity.AuthTokens, error) { return f.tokens, f.err } + +func TestHasNoSavedCredentials(t *testing.T) { + cases := map[string]struct { + store fakeTokenStore + want bool + }{ + "no credentials file": {fakeTokenStore{err: &breverrors.CredentialsFileNotFound{}}, true}, + "wrapped not found": {fakeTokenStore{err: breverrors.WrapAndTrace(&breverrors.CredentialsFileNotFound{})}, true}, + "nil tokens": {fakeTokenStore{}, true}, + "empty tokens": {fakeTokenStore{tokens: &entity.AuthTokens{}}, true}, + "has access token": {fakeTokenStore{tokens: &entity.AuthTokens{AccessToken: "a"}}, false}, + "has refresh token": {fakeTokenStore{tokens: &entity.AuthTokens{RefreshToken: "r"}}, false}, + "has api key": {fakeTokenStore{tokens: &entity.AuthTokens{APIKey: "k"}}, false}, + // An unrelated read error must not be reported as missing credentials. + "other error": {fakeTokenStore{err: stderrors.New("permission denied")}, false}, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + assert.Equal(t, tc.want, hasNoSavedCredentials(tc.store)) + }) + } +} + +// RemoteExitError must survive wrapping, since main.go recovers the code with errors.As. +func TestRemoteExitErrorSurvivesWrapping(t *testing.T) { + cases := map[string]error{ + "unwrapped": breverrors.RemoteExitError{Code: 4}, + "wrapped": breverrors.WrapAndTrace(breverrors.RemoteExitError{Code: 4}), + "doubleWrap": breverrors.WrapAndTrace(breverrors.WrapAndTrace(breverrors.RemoteExitError{Code: 4})), + "multierror": multierror.Append(nil, breverrors.RemoteExitError{Code: 4}), + "multiWrapped": multierror.Append(nil, breverrors.WrapAndTrace(breverrors.RemoteExitError{Code: 4})), + } + + for name, err := range cases { + t.Run(name, func(t *testing.T) { + var remoteErr breverrors.RemoteExitError + require.True(t, stderrors.As(err, &remoteErr), "errors.As must match through %s", name) + assert.Equal(t, 4, remoteErr.Code) + }) + } +} diff --git a/pkg/errors/errors.go b/pkg/errors/errors.go index 6579aacea..21d345615 100644 --- a/pkg/errors/errors.go +++ b/pkg/errors/errors.go @@ -295,3 +295,12 @@ func (e NvidiaMigrationError) Directive() string { func NewNvidiaMigrationError(msg string) *NvidiaMigrationError { return &NvidiaMigrationError{Message: msg} } + +// RemoteExitError means a command ran over SSH and exited non-zero; Code is its exit status. +type RemoteExitError struct { + Code int +} + +func (e RemoteExitError) Error() string { + return fmt.Sprintf("command exited with status %d", e.Code) +}