review feedback

Author: patelspratik

Makefile

@@ -213,7 +213,7 @@ full-smoke-test: ci fast-build
 build-linux-amd:
 	$(call print-target)
 	echo ${VERSION}
-	GOOS=linux GOARCH=amd64 go build -o brev -ldflags "-X github.com/brevdev/brev-cli/pkg/cmd/version.Version=${VERSION}"
+	CGO_ENABLED=1 GOOS=linux GOARCH=amd64 go build -o brev -ldflags "-X github.com/brevdev/brev-cli/pkg/cmd/version.Version=${VERSION}"
 
 .PHONY: build-darwin-amd
 build-darwin-amd:

pkg/cmd/register/gpu_nvml.go

@@ -85,8 +85,8 @@ func probeGPUsNVML() ([]GPU, []Interconnect) {
 		arch := ""
 		major, minor, ret := device.GetCudaComputeCapability()
 		if ret == nvml.SUCCESS {
-			if name := archName(major, minor); name != "" {
-				arch = name
+			if archStr := archName(major, minor); archStr != "" {
+				arch = archStr
 			} else {
 				arch = fmt.Sprintf("sm_%d%d", major, minor)
 			}
@@ -124,14 +124,17 @@ func probeGPUsNVML() ([]GPU, []Interconnect) {
 	return gpus, interconnects
 }
 
+// maxNVLinks is the maximum number of NVLink links to probe per device.
+// Blackwell supports up to 18.
+const maxNVLinks = 18
+
 // probeNVLink checks NVLink connections for a device.
 func probeNVLink(device nvml.Device, deviceIdx int) []Interconnect {
 	var ics []Interconnect
 	activeLinks := 0
 
-	// NVLink link count varies by architecture; try up to 18 links.
 	var nvlinkVersion uint32
-	for link := 0; link < 18; link++ {
+	for link := 0; link < maxNVLinks; link++ {
 		state, ret := device.GetNvLinkState(link)
 		if ret != nvml.SUCCESS {
 			break

pkg/cmd/register/hardware.go

@@ -53,6 +53,9 @@ type HardwareProfile struct {
 
 // FormatHardwareProfile returns a human-readable summary of the hardware profile.
 func FormatHardwareProfile(s *HardwareProfile) string {
+	if s == nil {
+		return ""
+	}
 	var b strings.Builder
 	if s.ProductName != "" {
 		_, _ = fmt.Fprintf(&b, "    Product: %s\n", s.ProductName)
@@ -63,6 +66,26 @@ func FormatHardwareProfile(s *HardwareProfile) string {
 	if s.RAMBytes != nil {
 		_, _ = fmt.Fprintf(&b, "    RAM:     %.1f GB\n", float64(*s.RAMBytes)/(1024*1024*1024))
 	}
+	parseGPUs(s, b)
+	_, _ = fmt.Fprintf(&b, "    Arch:    %s\n", s.Architecture)
+	if s.OS != "" || s.OSVersion != "" {
+		_, _ = fmt.Fprintf(&b, "    OS:      %s %s\n", s.OS, s.OSVersion)
+	}
+	parseInterconnects(s, b)
+	for _, st := range s.Storage {
+		_, _ = fmt.Fprintf(&b, "    Storage: %.1f GB", float64(st.StorageBytes)/(1024*1024*1024))
+		if st.StorageType != "" {
+			_, _ = fmt.Fprintf(&b, " (%s)", st.StorageType)
+		}
+		if st.Name != "" {
+			_, _ = fmt.Fprintf(&b, " [%s]", st.Name)
+		}
+		b.WriteString("\n")
+	}
+	return b.String()
+}
+
+func parseGPUs(s *HardwareProfile, b strings.Builder) {
 	for _, gpu := range s.GPUs {
 		if gpu.MemoryBytes != nil {
 			memGB := float64(*gpu.MemoryBytes) / (1024 * 1024 * 1024)
@@ -75,12 +98,19 @@ func FormatHardwareProfile(s *HardwareProfile) string {
 		}
 		b.WriteString("\n")
 	}
-	_, _ = fmt.Fprintf(&b, "    Arch:    %s\n", s.Architecture)
-	if s.OS != "" || s.OSVersion != "" {
-		_, _ = fmt.Fprintf(&b, "    OS:      %s %s\n", s.OS, s.OSVersion)
-	}
+}
+
+func parseInterconnects(s *HardwareProfile, b strings.Builder) {
 	for _, ic := range s.Interconnects {
 		_, _ = fmt.Fprintf(&b, "    Link:    %s", ic.Type)
+		if ic.Generation > 0 || ic.Width > 0 {
+			if ic.Generation > 0 {
+				_, _ = fmt.Fprintf(&b, " Gen%d", ic.Generation)
+			}
+			if ic.Width > 0 {
+				_, _ = fmt.Fprintf(&b, " x%d", ic.Width)
+			}
+		}
 		if ic.Device != "" {
 			_, _ = fmt.Fprintf(&b, " (%s)", ic.Device)
 		}
@@ -89,17 +119,6 @@ func FormatHardwareProfile(s *HardwareProfile) string {
 		}
 		b.WriteString("\n")
 	}
-	for _, st := range s.Storage {
-		_, _ = fmt.Fprintf(&b, "    Storage: %.1f GB", float64(st.StorageBytes)/(1024*1024*1024))
-		if st.StorageType != "" {
-			_, _ = fmt.Fprintf(&b, " (%s)", st.StorageType)
-		}
-		if st.Name != "" {
-			_, _ = fmt.Fprintf(&b, " [%s]", st.Name)
-		}
-		b.WriteString("\n")
-	}
-	return b.String()
 }
 
 // --- Content-parsing helpers (pure functions, used by Linux adapter and tests) ---
@@ -120,6 +139,8 @@ func parseCPUCountContent(content string) (int, error) {
 }
 
 // parseMemInfoContent parses the content of /proc/meminfo.
+// Not used by the current Linux profiler (which uses syscall.Sysinfo), but
+// retained as a tested pure-function fallback for environments without sysinfo.
 func parseMemInfoContent(content string) (int64, error) {
 	scanner := bufio.NewScanner(strings.NewReader(content))
 	for scanner.Scan() {
@@ -163,6 +184,8 @@ func unquote(s string) string {
 
 // parseStorageOutput parses lsblk output (NAME,SIZE,TYPE,ROTA columns),
 // returning one StorageDevice entry per disk device. ROTA=0 → SSD, ROTA=1 → HDD.
+// Not used by the current Linux profiler (which uses probeStorageSysfs), but
+// retained as a tested pure-function fallback for environments without sysfs.
 func parseStorageOutput(output string) []StorageDevice {
 	var devices []StorageDevice
 	scanner := bufio.NewScanner(strings.NewReader(output))

pkg/cmd/register/hardware_darwin.go

@@ -3,6 +3,7 @@
 package register
 
 import (
+	"log"
 	"os/exec"
 	"regexp"
 	"runtime"
@@ -28,6 +29,8 @@ func (p *SystemHardwareProfiler) Profile() (*HardwareProfile, error) {
 	if memSize, err := unix.SysctlUint64("hw.memsize"); err == nil {
 		ramBytes := int64(memSize)
 		hw.RAMBytes = &ramBytes
+	} else {
+		log.Printf("hardware profiler: failed to read memory size: %v", err)
 	}
 
 	hw.OS = readDarwinCommand("sw_vers", "-productName")
@@ -137,21 +140,9 @@ func parseDiskutilSize(val string, dev *StorageDevice) {
 		cleaned := strings.ReplaceAll(m[1], ",", "")
 		if size, err := strconv.ParseInt(cleaned, 10, 64); err == nil {
 			dev.StorageBytes = size
-			return
-		}
-	}
-	// Fallback: extract digits from first token.
-	parts := strings.Fields(val)
-	if len(parts) < 1 {
-		return
-	}
-	var size int64
-	for _, c := range parts[0] {
-		if c >= '0' && c <= '9' {
-			size = size*10 + int64(c-'0')
 		}
 	}
-	dev.StorageBytes = size
+	// If the regex doesn't match, leave StorageBytes as 0 (unknown).
 }
 
 func parseDiskutilSolidState(val string, dev *StorageDevice) {

pkg/cmd/register/hardware_linux.go

@@ -4,6 +4,7 @@ package register
 
 import (
 	"fmt"
+	"log"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -26,10 +27,14 @@ func (p *SystemHardwareProfiler) Profile() (*HardwareProfile, error) {
 	if cpuCount, err := readCPUCount(); err == nil {
 		count32 := int32(cpuCount)
 		hw.CPUCount = &count32
+	} else {
+		log.Printf("hardware profiler: failed to read CPU count: %v", err)
 	}
 
 	if ramBytes, err := readRAMBytes(); err == nil {
 		hw.RAMBytes = &ramBytes
+	} else {
+		log.Printf("hardware profiler: failed to read RAM: %v", err)
 	}
 
 	hw.OS, hw.OSVersion = readOSRelease()

pkg/cmd/register/hardware_test.go

@@ -196,6 +196,29 @@ func Test_FormatHardwareProfile(t *testing.T) {
 	}
 }
 
+func Test_FormatHardwareProfile_Nil(t *testing.T) {
+	output := FormatHardwareProfile(nil)
+	if output != "" {
+		t.Errorf("expected empty string for nil input, got: %q", output)
+	}
+}
+
+func Test_FormatHardwareProfile_PCIeInterconnect(t *testing.T) {
+	s := &HardwareProfile{
+		Architecture: "amd64",
+		Interconnects: []Interconnect{
+			{Type: "PCIe", Device: "GPU 0", Generation: 4, Width: 16},
+		},
+	}
+	output := FormatHardwareProfile(s)
+	if !strings.Contains(output, "PCIe Gen4 x16") {
+		t.Errorf("expected 'PCIe Gen4 x16' in output, got: %s", output)
+	}
+	if !strings.Contains(output, "(GPU 0)") {
+		t.Errorf("expected '(GPU 0)' in output, got: %s", output)
+	}
+}
+
 func Test_FormatHardwareProfile_MinimalFields(t *testing.T) {
 	s := &HardwareProfile{
 		GPUs: []GPU{

pkg/cmd/register/register_test.go

@@ -617,6 +617,81 @@ func Test_runRegister_GrantSSH_no_retry_on_permanent_error(t *testing.T) {
 	}
 }
 
+func Test_runRegister_PlatformIncompatible(t *testing.T) {
+	regStore := &mockRegistrationStore{}
+
+	store := &mockRegisterStore{
+		user:  &entity.User{ID: "user_1"},
+		org:   &entity.Organization{ID: "org_123", Name: "TestOrg"},
+		token: "tok",
+	}
+
+	svc := &fakeNodeService{}
+	deps, server := testRegisterDeps(t, svc, regStore)
+	defer server.Close()
+
+	deps.platform = mockPlatform{compatible: false}
+
+	term := terminal.New()
+	err := runRegister(context.Background(), term, store, "My Spark", deps)
+	if err == nil {
+		t.Fatal("expected error when platform is incompatible")
+	}
+	if !strings.Contains(err.Error(), "only supported on Linux") {
+		t.Errorf("expected platform incompatibility error, got: %v", err)
+	}
+}
+
+func Test_runRegister_HardwareProfilerFailure(t *testing.T) {
+	regStore := &mockRegistrationStore{}
+
+	store := &mockRegisterStore{
+		user:  &entity.User{ID: "user_1"},
+		org:   &entity.Organization{ID: "org_123", Name: "TestOrg"},
+		token: "tok",
+	}
+
+	svc := &fakeNodeService{}
+	deps, server := testRegisterDeps(t, svc, regStore)
+	defer server.Close()
+
+	deps.hardwareProfiler = &mockHardwareProfiler{err: fmt.Errorf("nvml init failed")}
+
+	term := terminal.New()
+	err := runRegister(context.Background(), term, store, "My Spark", deps)
+	if err == nil {
+		t.Fatal("expected error when hardware profiler fails")
+	}
+	if !strings.Contains(err.Error(), "hardware profile") {
+		t.Errorf("expected hardware profile error, got: %v", err)
+	}
+}
+
+func Test_runRegister_NetBirdInstallFailure(t *testing.T) {
+	regStore := &mockRegistrationStore{}
+
+	store := &mockRegisterStore{
+		user:  &entity.User{ID: "user_1"},
+		org:   &entity.Organization{ID: "org_123", Name: "TestOrg"},
+		token: "tok",
+	}
+
+	svc := &fakeNodeService{}
+	deps, server := testRegisterDeps(t, svc, regStore)
+	defer server.Close()
+
+	deps.netbird = mockNetBirdManager{err: fmt.Errorf("install failed")}
+
+	term := terminal.New()
+	err := runRegister(context.Background(), term, store, "My Spark", deps)
+	if err == nil {
+		t.Fatal("expected error when NetBird install fails")
+	}
+	if !strings.Contains(err.Error(), "tunnel setup failed") {
+		t.Errorf("expected tunnel setup error, got: %v", err)
+	}
+}
+
 func Test_runRegister_NoNameNotRegistered(t *testing.T) {
 	regStore := &mockRegistrationStore{}
 

pkg/cmd/register/rpcclient_test.go

@@ -119,6 +119,35 @@ func Test_toProtoNodeSpec(t *testing.T) {
 	}
 }
 
+func Test_toProtoNodeSpec_PCIeInterconnect(t *testing.T) {
+	local := &HardwareProfile{
+		Architecture: "amd64",
+		Interconnects: []Interconnect{
+			{Type: "PCIe", Device: "GPU 0", Generation: 4, Width: 16},
+		},
+	}
+
+	proto := toProtoNodeSpec(local)
+
+	if len(proto.GetInterconnects()) != 1 {
+		t.Fatalf("expected 1 interconnect, got %d", len(proto.GetInterconnects()))
+	}
+	ic := proto.GetInterconnects()[0]
+	if ic.GetDevice() != "GPU 0" {
+		t.Errorf("expected device 'GPU 0', got %s", ic.GetDevice())
+	}
+	pcie := ic.GetPcie()
+	if pcie == nil {
+		t.Fatal("expected PCIe details, got nil")
+	}
+	if pcie.GetGeneration() != 4 {
+		t.Errorf("expected PCIe Gen 4, got %d", pcie.GetGeneration())
+	}
+	if pcie.GetWidth() != 16 {
+		t.Errorf("expected PCIe Width 16, got %d", pcie.GetWidth())
+	}
+}
+
 func Test_toProtoNodeSpec_Nil(t *testing.T) {
 	if toProtoNodeSpec(nil) != nil {
 		t.Error("expected nil for nil input")