From b3fd984e2c4d6ae77f21c1aefe814f515d6f9e34 Mon Sep 17 00:00:00 2001 From: Alec Fong Date: Fri, 26 Jun 2026 16:31:23 -0700 Subject: [PATCH 1/2] fix(shell): clear message when SSHing to a node without access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an org member runs 'brev shell ' 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 --- pkg/cmd/util/externalnode.go | 47 ++++++++++++++++++++++++++++++- pkg/cmd/util/externalnode_test.go | 39 ++++++++++++++++++++++++- 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/util/externalnode.go b/pkg/cmd/util/externalnode.go index ef78d8e4..c899c68f 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.New(fmt.Sprintf( + "you don't have SSH access to node %q.\n"+ + "Ask an org admin 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.New(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.New(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 9e5244ae..765942ff 100644 --- a/pkg/cmd/util/externalnode_test.go +++ b/pkg/cmd/util/externalnode_test.go @@ -2,6 +2,7 @@ package util import ( "fmt" + "strings" "testing" nodev1 "buf.build/gen/go/brevdev/devplane/protocolbuffers/go/devplaneapi/v1" @@ -104,7 +105,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 +113,39 @@ 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) + } + 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) + } +} + +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 +189,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) { From 3e3b6753e017c018d2ff33b0b21953fcae655233 Mon Sep 17 00:00:00 2001 From: Alec Fong Date: Wed, 1 Jul 2026 11:07:18 -0700 Subject: [PATCH 2/2] 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. --- pkg/cmd/cmderrors/cmderrors.go | 7 ++++++- pkg/cmd/util/externalnode.go | 8 ++++---- pkg/cmd/util/externalnode_test.go | 14 ++++++++++++++ 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/cmderrors/cmderrors.go b/pkg/cmd/cmderrors/cmderrors.go index 4b07989a..1347f71c 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 c899c68f..0db4b3ab 100644 --- a/pkg/cmd/util/externalnode.go +++ b/pkg/cmd/util/externalnode.go @@ -190,23 +190,23 @@ func classifyNodeSSHFailure(user *entity.User, node *nodev1.ExternalNode) error if who == "" { who = user.ID } - return breverrors.New(fmt.Sprintf( + return breverrors.NewValidationError(fmt.Sprintf( "you don't have SSH access to node %q.\n"+ - "Ask an org admin to grant you access, e.g.:\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.New(fmt.Sprintf( + 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.New(fmt.Sprintf( + 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 765942ff..39583e26 100644 --- a/pkg/cmd/util/externalnode_test.go +++ b/pkg/cmd/util/externalnode_test.go @@ -8,6 +8,7 @@ import ( 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 @@ -118,12 +119,25 @@ func TestResolveExternalNodeSSH_NoAccess(t *testing.T) { 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) {