fix(BRE2-804): various polish (#315)

* fix(BRE2-804): various polish: name sanitization, interface cleanliness, etc

* caching email

Author: patelspratik

pkg/cmd/cmd.go

@@ -58,6 +58,7 @@ import (
 	"github.com/brevdev/brev-cli/pkg/cmd/workspacegroups"
 	"github.com/brevdev/brev-cli/pkg/cmd/writeconnectionevent"
 	"github.com/brevdev/brev-cli/pkg/config"
+	"github.com/brevdev/brev-cli/pkg/entity"
 	"github.com/brevdev/brev-cli/pkg/featureflag"
 	"github.com/brevdev/brev-cli/pkg/files"
 	"github.com/brevdev/brev-cli/pkg/remoteversion"
@@ -257,8 +258,20 @@ func NewBrevCommand() *cobra.Command { //nolint:funlen,gocognit,gocyclo // defin
 	cmds.SetUsageTemplate(usageTemplate)
 
 	// In-memory auth for external node commands — never touches credentials.json.
-	memAuthStore := store.NewMemoryAuthStore()
-	memAuthenticator := auth.StandardLogin("", "", nil)
+	// Pre-fill the cached email so the user sees a confirmation prompt instead of
+	// having to type it from scratch every time.
+	cachedEmail, _ := fsStore.GetCachedEmail()
+	memAuthenticator := auth.StandardLogin("", cachedEmail, nil)
+	if cachedEmail != "" {
+		if kas, ok := memAuthenticator.(auth.KasAuthenticator); ok {
+			kas.ShouldPromptEmail = true
+			memAuthenticator = kas
+		}
+	}
+	memAuthStore := &emailCachingAuthStore{
+		MemoryAuthStore: store.NewMemoryAuthStore(),
+		fileStore:       fsStore,
+	}
 	memLoginAuth := auth.NewLoginAuth(memAuthStore, memAuthenticator)
 	memLoginAuth.WithShouldLogin(func() (bool, error) { return true, nil })
 
@@ -555,4 +568,22 @@ var (
 	_ store.Auth     = auth.NoLoginAuth{}
 	_ auth.AuthStore = store.FileStore{}
 	_ auth.AuthStore = &store.MemoryAuthStore{}
+	_ auth.AuthStore = &emailCachingAuthStore{}
 )
+
+// emailCachingAuthStore wraps MemoryAuthStore and persists the login email
+// to ~/.brev/cached-email after each successful authentication.
+type emailCachingAuthStore struct {
+	*store.MemoryAuthStore
+	fileStore *store.FileStore
+}
+
+func (e *emailCachingAuthStore) SaveAuthTokens(tokens entity.AuthTokens) error {
+	if err := e.MemoryAuthStore.SaveAuthTokens(tokens); err != nil {
+		return breverrors.WrapAndTrace(err)
+	}
+	if email := auth.GetEmailFromToken(tokens.AccessToken); email != "" {
+		_ = e.fileStore.SaveCachedEmail(email)
+	}
+	return nil
+}

pkg/cmd/cmd_test.go

@@ -0,0 +1,75 @@
+package cmd
+
+import (
+	"encoding/base64"
+	"encoding/json"
+	"testing"
+
+	"github.com/brevdev/brev-cli/pkg/entity"
+	"github.com/brevdev/brev-cli/pkg/store"
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// fakeJWT builds an unsigned JWT with the given claims (header.payload.signature).
+func fakeJWT(t *testing.T, claims map[string]interface{}) string {
+	t.Helper()
+	header := base64.RawURLEncoding.EncodeToString([]byte(`{"alg":"none","typ":"JWT"}`))
+	payload, err := json.Marshal(claims)
+	require.NoError(t, err)
+	return header + "." + base64.RawURLEncoding.EncodeToString(payload) + "."
+}
+
+func newTestFileStore(t *testing.T) *store.FileStore {
+	t.Helper()
+	fs := afero.NewMemMapFs()
+	err := fs.MkdirAll("/home/testuser/.brev", 0o755)
+	require.NoError(t, err)
+	return store.NewBasicStore().WithFileSystem(fs).WithUserHomeDirGetter(
+		func() (string, error) { return "/home/testuser", nil },
+	)
+}
+
+func TestEmailCachingAuthStore_SaveCachesEmail(t *testing.T) {
+	fs := newTestFileStore(t)
+	s := &emailCachingAuthStore{
+		MemoryAuthStore: store.NewMemoryAuthStore(),
+		fileStore:       fs,
+	}
+
+	token := fakeJWT(t, map[string]interface{}{"email": "user@example.com"})
+	err := s.SaveAuthTokens(entity.AuthTokens{AccessToken: token})
+	require.NoError(t, err)
+
+	cached, err := fs.GetCachedEmail()
+	require.NoError(t, err)
+	assert.Equal(t, "user@example.com", cached)
+}
+
+func TestEmailCachingAuthStore_NoEmailInToken(t *testing.T) {
+	fs := newTestFileStore(t)
+	s := &emailCachingAuthStore{
+		MemoryAuthStore: store.NewMemoryAuthStore(),
+		fileStore:       fs,
+	}
+
+	token := fakeJWT(t, map[string]interface{}{"sub": "12345"})
+	err := s.SaveAuthTokens(entity.AuthTokens{AccessToken: token})
+	require.NoError(t, err)
+
+	cached, err := fs.GetCachedEmail()
+	require.NoError(t, err)
+	assert.Equal(t, "", cached)
+}
+
+func TestEmailCachingAuthStore_EmptyAccessToken(t *testing.T) {
+	fs := newTestFileStore(t)
+	s := &emailCachingAuthStore{
+		MemoryAuthStore: store.NewMemoryAuthStore(),
+		fileStore:       fs,
+	}
+
+	err := s.SaveAuthTokens(entity.AuthTokens{AccessToken: ""})
+	require.Error(t, err)
+}

pkg/cmd/deregister/deregister_test.go

@@ -91,8 +91,9 @@ type mockNetBirdManager struct {
 	err    error
 }
 
-func (m *mockNetBirdManager) Install() error   { return m.err }
-func (m *mockNetBirdManager) Uninstall() error { m.called = true; return m.err }
+func (m *mockNetBirdManager) Install() error       { return m.err }
+func (m *mockNetBirdManager) Uninstall() error     { m.called = true; return m.err }
+func (m *mockNetBirdManager) EnsureRunning() error { return m.err }
 
 type mockNodeClientFactory struct {
 	serverURL string

pkg/cmd/gpucreate/gpucreate.go

@@ -18,6 +18,7 @@ import (
 	"github.com/brevdev/brev-cli/pkg/entity"
 	breverrors "github.com/brevdev/brev-cli/pkg/errors"
 	"github.com/brevdev/brev-cli/pkg/featureflag"
+	"github.com/brevdev/brev-cli/pkg/names"
 	"github.com/brevdev/brev-cli/pkg/store"
 	"github.com/brevdev/brev-cli/pkg/terminal"
 	"github.com/spf13/cobra"
@@ -194,8 +195,8 @@ func NewCmdGPUCreate(t *terminal.Terminal, gpuCreateStore GPUCreateStore) *cobra
 				}
 			}
 
-			if name == "" {
-				return breverrors.NewValidationError("name is required (as argument or --name flag)")
+			if err := names.ValidateNodeName(name); err != nil {
+				return breverrors.WrapAndTrace(err)
 			}
 
 			if count < 1 {

pkg/cmd/register/device_registration_store.go

@@ -85,6 +85,9 @@ func (s *FileRegistrationStore) Load() (*DeviceRegistration, error) {
 	if err := files.ReadJSON(files.AppFs, path, &reg); err != nil {
 		return nil, breverrors.WrapAndTrace(err)
 	}
+	if reg.ExternalNodeID == "" || reg.OrgID == "" {
+		return nil, breverrors.New("malformed registration")
+	}
 	return &reg, nil
 }
 
@@ -125,6 +128,9 @@ func sudoWriteFile(path string, data []byte) error {
 	if err := cmd.Run(); err != nil {
 		return fmt.Errorf("sudo tee %s failed: %w", path, err)
 	}
+	if err := exec.Command("sudo", "chmod", "644", path).Run(); err != nil { //nolint:gosec // fixed base path
+		return fmt.Errorf("sudo chmod %s failed: %w", path, err)
+	}
 	return nil
 }
 

pkg/cmd/register/device_registration_store_test.go

@@ -144,6 +144,48 @@ func Test_LoadRegistration_FailsWhenMissing(t *testing.T) {
 	}
 }
 
+func Test_LoadRegistration_RejectsMissingExternalNodeID(t *testing.T) {
+	cleanup := setupTestFs(t)
+	defer cleanup()
+
+	store := NewFileRegistrationStore()
+
+	reg := &DeviceRegistration{
+		ExternalNodeID: "",
+		DisplayName:    "Test",
+		OrgID:          "org_xyz",
+	}
+	if err := store.Save(reg); err != nil {
+		t.Fatalf("Save failed: %v", err)
+	}
+
+	_, err := store.Load()
+	if err == nil {
+		t.Fatal("expected error loading registration with empty ExternalNodeID")
+	}
+}
+
+func Test_LoadRegistration_RejectsMissingOrgID(t *testing.T) {
+	cleanup := setupTestFs(t)
+	defer cleanup()
+
+	store := NewFileRegistrationStore()
+
+	reg := &DeviceRegistration{
+		ExternalNodeID: "unode_abc",
+		DisplayName:    "Test",
+		OrgID:          "",
+	}
+	if err := store.Save(reg); err != nil {
+		t.Fatalf("Save failed: %v", err)
+	}
+
+	_, err := store.Load()
+	if err == nil {
+		t.Fatal("expected error loading registration with empty OrgID")
+	}
+}
+
 func Test_DeleteRegistration_FailsWhenMissing(t *testing.T) {
 	cleanup := setupTestFs(t)
 	defer cleanup()

pkg/cmd/register/providers.go

@@ -1,7 +1,10 @@
 package register
 
 import (
+	"fmt"
+	"os/exec"
 	"runtime"
+	"strings"
 
 	nodev1connect "buf.build/gen/go/brevdev/devplane/connectrpc/go/devplaneapi/v1/devplaneapiv1connect"
 
@@ -38,6 +41,33 @@ type Netbird struct{}
 func (Netbird) Install() error   { return InstallNetbird() }
 func (Netbird) Uninstall() error { return UninstallNetbird() }
 
+// EnsureRunning checks if the netbird systemd service is active and attempts
+// to start it if it is not. It also checks the netbird peer connection status
+// and runs "netbird up" if the peer is disconnected.
+func (Netbird) EnsureRunning() error {
+	out, err := exec.Command("systemctl", "is-active", "netbird").Output() //nolint:gosec // fixed service name
+	if err != nil || strings.TrimSpace(string(out)) != "active" {
+		if startErr := exec.Command("sudo", "systemctl", "start", "netbird").Run(); startErr != nil { //nolint:gosec // fixed service name
+			return fmt.Errorf("failed to start Brev tunnel service: %w", startErr)
+		}
+	}
+
+	statusOut, err := exec.Command("netbird", "status").Output() //nolint:gosec // fixed command
+	if err != nil {
+		// Service is running, just can't confirm peer status.
+		return nil
+	}
+
+	if netbirdManagementConnected(string(statusOut)) {
+		return nil
+	}
+
+	if upErr := exec.Command("sudo", "netbird", "up").Run(); upErr != nil { //nolint:gosec // fixed command
+		return fmt.Errorf("failed to reconnect Brev tunnel: %w", upErr)
+	}
+	return nil
+}
+
 // ShellSetupRunner runs setup scripts via shell.
 type ShellSetupRunner struct{}