fix(shell): clear message when SSHing to a node without access (#418)

* fix(shell): clear message when SSHing to a node without access

When an org member runs 'brev shell <node>' on a BYON node they can see in
'brev ls nodes' but have not been granted SSH access, they hit a confusing
catch-all error:

  cannot resolve SSH for node "dgx-station" — no access, no port, or no hostname

Replace it with classifyNodeSSHFailure, which mirrors ResolveNodeSSHEntry's
resolution logic and surfaces the actual cause with a remedy:

- no access grant (the common case): tell the user they lack access and show
  the 'brev grant-ssh' command an org admin can run, with their email prefilled
- access granted but port not allocated / hostname not ready: explain the node
  may still be connecting and suggest 'brev refresh'

This flows through shell, exec, copy, open, and port-forward via
ResolveExternalNodeSSH. ResolveNodeSSHEntry (used by refresh for bulk config
generation, where silent skipping is correct) is unchanged.

Fixes BRE2-953

* fix(shell): reword no-access hint and stop tracing expected SSH errors

Point the user at someone who already has SSH access to the node rather
than an org admin, since granting access does not require org admin.

Return the SSH-access resolution failures as ValidationErrors and only
dump a stack trace in dev/debug builds for unexpected, Sentry-reported
errors, so these expected user-facing messages print cleanly without the
[error] ...WrapAndTrace frames.

Author: theFong

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)

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))
+}

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,14 +106,60 @@ 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)
 
 	_, err := ResolveExternalNodeSSH(store, node)
 	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) {