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

Author: patelspratik

pkg/cmd/create/create.go

@@ -59,7 +59,7 @@ func NewCmdCreate(t *terminal.Terminal, createStore CreateStore) *cobra.Command
 				Detached:       detached,
 				InstanceType:   gpu,
 			}, createStore)
-			if err != nil {
+			if err != x nil {
 				if strings.Contains(err.Error(), "duplicate instance with name") {
 					t.Vprint(t.Yellow("try running:"))
 					t.Vprint(t.Yellow("\tbrev start --name [different name] [repo] # or"))

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("corrupt registration file: missing external_node_id or org_id")
+	}
 	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{}
 

pkg/cmd/register/register.go

@@ -5,7 +5,6 @@ import (
 	"context"
 	"fmt"
 	"os"
-	"os/exec"
 	"os/user"
 	"strings"
 	"time"
@@ -18,6 +17,7 @@ import (
 	"github.com/brevdev/brev-cli/pkg/entity"
 	breverrors "github.com/brevdev/brev-cli/pkg/errors"
 	"github.com/brevdev/brev-cli/pkg/externalnode"
+	"github.com/brevdev/brev-cli/pkg/names"
 	"github.com/brevdev/brev-cli/pkg/terminal"
 
 	"github.com/spf13/cobra"
@@ -41,10 +41,14 @@ func (r OSFileReader) ReadFile(path string) ([]byte, error) {
 	return data, nil
 }
 
-// NetBirdManager installs and uninstalls the NetBird network agent.
+// NetBirdManager installs, uninstalls, and monitors the NetBird network agent.
 type NetBirdManager interface {
 	Install() error
 	Uninstall() error
+	// EnsureRunning checks whether the NetBird service is active and
+	// connected, starting or reconnecting it if needed. Returns nil when
+	// the tunnel is healthy.
+	EnsureRunning() error
 }
 
 // SetupRunner runs a setup script on the local machine.
@@ -120,8 +124,8 @@ func runRegister(ctx context.Context, t *terminal.Terminal, s RegisterStore, nam
 		return checkExistingRegistration(ctx, t, s, name, deps)
 	}
 
-	if name == "" {
-		return fmt.Errorf("please provide a name for this device\n\nUsage: brev register <name>\nExample: brev register \"my-DGX-Spark\"")
+	if err := names.ValidateNodeName(name); err != nil {
+		return breverrors.WrapAndTrace(err)
 	}
 
 	brevUser, err := s.GetCurrentUser()
@@ -273,7 +277,9 @@ func checkExistingRegistration(ctx context.Context, t *terminal.Terminal, s Regi
 
 	// Check local netbird service and start it if down.
 	t.Vprint("  Checking local Brev tunnel...")
-	if ensureNetbirdRunning(t) {
+	if err := deps.netbird.EnsureRunning(); err != nil {
+		t.Vprintf("  %s\n", t.Yellow(fmt.Sprintf("Warning: %v", err)))
+	} else {
 		t.Vprint(t.Green("  Brev tunnel is running."))
 	}
 
@@ -282,41 +288,6 @@ func checkExistingRegistration(ctx context.Context, t *terminal.Terminal, s Regi
 	return nil
 }
 
-// ensureNetbirdRunning 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.
-// Returns true if the service is running and connected after the check.
-func ensureNetbirdRunning(t *terminal.Terminal) bool {
-	out, err := exec.Command("systemctl", "is-active", "netbird").Output() //nolint:gosec // fixed service name
-	if err != nil || strings.TrimSpace(string(out)) != "active" {
-		t.Vprintf("  %s\n", t.Yellow("Brev tunnel service is not running. Attempting to start..."))
-		if startErr := exec.Command("sudo", "systemctl", "start", "netbird").Run(); startErr != nil { //nolint:gosec // fixed service name
-			t.Vprintf("  %s\n", t.Yellow(fmt.Sprintf("Warning: failed to start Brev tunnel service: %v", startErr)))
-			return false
-		}
-		t.Vprint(t.Green("  Brev tunnel service started."))
-	}
-
-	// Service is running — now check peer connection status.
-	statusOut, err := exec.Command("netbird", "status").Output() //nolint:gosec // fixed command
-	if err != nil {
-		t.Vprintf("  %s\n", t.Yellow(fmt.Sprintf("Warning: could not check Brev tunnel status: %v", err)))
-		return true // service is running, just can't confirm peer status
-	}
-
-	if netbirdManagementConnected(string(statusOut)) {
-		return true
-	}
-
-	t.Vprintf("  %s\n", t.Yellow("Brev tunnel peer is disconnected. Reconnecting..."))
-	if upErr := exec.Command("sudo", "netbird", "up").Run(); upErr != nil { //nolint:gosec // fixed command
-		t.Vprintf("  %s\n", t.Yellow(fmt.Sprintf("Warning: failed to reconnect Brev tunnel: %v", upErr)))
-		return false
-	}
-	t.Vprint(t.Green("  Brev tunnel reconnected."))
-	return true
-}
-
 // netbirdManagementConnected parses "netbird status" output and returns true
 // when the Management line reports "Connected".
 func netbirdManagementConnected(statusOutput string) bool {