diff --git a/pkg/cmd/cmderrors/cmderrors.go b/pkg/cmd/cmderrors/cmderrors.go index 4b07989a8..1347f71c8 100644 --- a/pkg/cmd/cmderrors/cmderrors.go +++ b/pkg/cmd/cmderrors/cmderrors.go @@ -29,6 +29,10 @@ func DisplayAndHandleError(err error) { if err != nil { t := terminal.New() prettyErr := "" + // showTrace gates whether we dump the full wrapped error (with stack + // frames) in dev/debug builds. Only unexpected, Sentry-reported errors + // warrant a trace; expected user-facing errors always print cleanly. + showTrace := false switch errors.Cause(err).(type) { case breverrors.ValidationError: // do not report error @@ -65,9 +69,10 @@ func DisplayAndHandleError(err error) { } else { er.ReportError(err) prettyErr = (t.Red(errors.Cause(err).Error())) + showTrace = true } } - if featureflag.Debug() || featureflag.IsDev() { + if showTrace && (featureflag.Debug() || featureflag.IsDev()) { fmt.Fprintln(os.Stderr, err) } else { fmt.Fprintln(os.Stderr, prettyErr) diff --git a/pkg/cmd/util/externalnode.go b/pkg/cmd/util/externalnode.go index ef78d8e43..0db4b3abc 100644 --- a/pkg/cmd/util/externalnode.go +++ b/pkg/cmd/util/externalnode.go @@ -156,7 +156,7 @@ func ResolveExternalNodeSSH(store ExternalNodeStore, node *nodev1.ExternalNode) entry := ResolveNodeSSHEntry(user.ID, node) if entry == nil { - return nil, breverrors.New(fmt.Sprintf("cannot resolve SSH for node %q — no access, no port, or no hostname", node.GetName())) + return nil, classifyNodeSSHFailure(user, node) } return &ExternalNodeSSHInfo{ @@ -166,3 +166,48 @@ func ResolveExternalNodeSSH(store ExternalNodeStore, node *nodev1.ExternalNode) Port: entry.Port, }, nil } + +// classifyNodeSSHFailure returns an actionable error explaining why SSH cannot +// be resolved for the user on this node. The cases mirror ResolveNodeSSHEntry: +// the user may have no access grant, an access grant whose port is not yet +// allocated, or a port that has not yet reported its hostname. Each case has a +// distinct remedy, so we surface them separately rather than as one catch-all. +func classifyNodeSSHFailure(user *entity.User, node *nodev1.ExternalNode) error { + nodeName := node.GetName() + + var access *nodev1.SSHAccess + for _, a := range node.GetSshAccess() { + if a.GetUserId() == user.ID { + access = a + break + } + } + + // Most common case (per the issue): the node is visible in `brev ls nodes` + // because the user is in the org, but no SSH access has been granted. + if access == nil || access.GetLinuxUser() == "" { + who := user.Email + if who == "" { + who = user.ID + } + return breverrors.NewValidationError(fmt.Sprintf( + "you don't have SSH access to node %q.\n"+ + "Ask someone with SSH access to this node to grant you access, e.g.:\n"+ + " brev grant-ssh --node %s --user %s", + nodeName, nodeName, who)) + } + + port := resolvePortForSSHAccess(node, access) + if port == nil { + return breverrors.NewValidationError(fmt.Sprintf( + "SSH access to node %q is granted but its SSH port isn't allocated yet.\n"+ + "The node may still be connecting — try again shortly, or run 'brev refresh'.", + nodeName)) + } + + // Access and port exist, but the port has no hostname yet. + return breverrors.NewValidationError(fmt.Sprintf( + "SSH access to node %q is granted but the connection details aren't ready yet.\n"+ + "The node may still be connecting — try again shortly, or run 'brev refresh'.", + nodeName)) +} diff --git a/pkg/cmd/util/externalnode_test.go b/pkg/cmd/util/externalnode_test.go index 9e5244ae9..39583e260 100644 --- a/pkg/cmd/util/externalnode_test.go +++ b/pkg/cmd/util/externalnode_test.go @@ -2,11 +2,13 @@ package util import ( "fmt" + "strings" "testing" nodev1 "buf.build/gen/go/brevdev/devplane/protocolbuffers/go/devplaneapi/v1" "github.com/brevdev/brev-cli/pkg/entity" + breverrors "github.com/brevdev/brev-cli/pkg/errors" ) // mockExternalNodeStore satisfies ExternalNodeStore for unit tests that @@ -104,7 +106,7 @@ func TestResolveExternalNodeSSH_UsesServerPortNotPortNumber(t *testing.T) { func TestResolveExternalNodeSSH_NoAccess(t *testing.T) { store := &mockExternalNodeStore{ - user: &entity.User{ID: "user_1"}, + user: &entity.User{ID: "user_1", Email: "me@example.com"}, } node := makeTestNode("box", "other_user", "ubuntu", "10.0.0.5", 22) @@ -112,6 +114,52 @@ func TestResolveExternalNodeSSH_NoAccess(t *testing.T) { if err == nil { t.Fatal("expected error for no SSH access") } + // The message should tell the user they lack access and how to get it, + // not the old confusing "no access, no port, or no hostname". + if !strings.Contains(err.Error(), "don't have SSH access") { + t.Errorf("expected a 'no access' message, got: %v", err) + } + // It should point the user at someone who can grant access, not an org admin. + if !strings.Contains(err.Error(), "someone with SSH access to this node") { + t.Errorf("expected the message to ask someone with SSH access, got: %v", err) + } + if strings.Contains(err.Error(), "org admin") { + t.Errorf("did not expect the message to reference an org admin, got: %v", err) + } + if !strings.Contains(err.Error(), "brev grant-ssh") { + t.Errorf("expected the message to suggest 'brev grant-ssh', got: %v", err) + } + if !strings.Contains(err.Error(), "me@example.com") { + t.Errorf("expected the message to reference the user's email, got: %v", err) + } + // It must be a ValidationError so the top-level handler prints it cleanly + // (no stack trace, no Sentry report) even in dev builds. + var valErr breverrors.ValidationError + if !breverrors.As(err, &valErr) { + t.Errorf("expected a ValidationError, got %T: %v", err, err) + } +} + +func TestResolveExternalNodeSSH_NoPortAllocated(t *testing.T) { + store := &mockExternalNodeStore{ + user: &entity.User{ID: "user_1"}, + } + // User has an access grant, but no port matches its PortId. + node := &nodev1.ExternalNode{ + Name: "box", + SshAccess: []*nodev1.SSHAccess{ + {UserId: "user_1", LinuxUser: "ubuntu", PortId: "port_missing"}, + }, + Ports: []*nodev1.Port{}, + } + + _, err := ResolveExternalNodeSSH(store, node) + if err == nil { + t.Fatal("expected error when access exists but no port is allocated") + } + if !strings.Contains(err.Error(), "is granted") { + t.Errorf("expected a 'granted but not ready' message, got: %v", err) + } } func TestResolveExternalNodeSSH_UsesAccessPortNotProtocol(t *testing.T) { @@ -155,6 +203,9 @@ func TestResolveExternalNodeSSH_EmptyHostname(t *testing.T) { if err == nil { t.Fatal("expected error for empty hostname") } + if !strings.Contains(err.Error(), "is granted") { + t.Errorf("expected a 'granted but not ready' message, got: %v", err) + } } func TestResolveExternalNodeSSH_GetUserError(t *testing.T) {