fix+refactor: 12-expert review — bugs, dead code, safety, quality

Makefile

@@ -14,7 +14,7 @@ SANDBOX_IMAGE  := $(REGISTRY):sandbox
 LAUNCHER_IMAGE := $(REGISTRY):launcher
 
 .PHONY: cli sandbox push-sandbox cli-launcher launcher push-launcher \
-        test-unit test test-podman test-ocp test-all validate clean help
+        vet lint test-unit test test-podman test-ocp test-all validate clean help
 
 ## ── CLI ──────────────────────────────────────────────────────────────
 
@@ -47,6 +47,22 @@ launcher: cli-launcher sandbox/launcher/Dockerfile sandbox/launcher/openshell
 push-launcher: launcher
 	docker push $(LAUNCHER_IMAGE)
 
+## ── Lint targets ─────────────────────────────────────────────────────
+
+## Run go vet
+vet:
+	go vet ./...
+	cd sandbox/launcher && go vet ./...
+
+## Run golangci-lint (install: https://golangci-lint.run/usage/install/)
+lint:
+	@if command -v golangci-lint >/dev/null 2>&1; then \
+		golangci-lint run ./...; \
+	else \
+		echo "golangci-lint not installed, running go vet instead"; \
+		$(MAKE) vet; \
+	fi
+
 ## ── Test targets ─────────────────────────────────────────────────────
 
 ## Unit tests only (no live gateway, fast)

cmd/deploy.go

@@ -48,7 +48,12 @@ func NewDeployCmd(harnessDir, cli string) *cobra.Command {
 	return cmd
 }
 
-func deployRemote(harnessDir string, gw gateway.Gateway, kc, clusterRunner k8s.Runner) error {
+func deployRemote(harnessDir string, gw gateway.Gateway, kc, clusterRunner k8s.Runner) (retErr error) {
+	defer func() {
+		if retErr != nil {
+			fmt.Fprintf(os.Stderr, "\nDeploy failed. Clean up with: harness teardown --k8s\n")
+		}
+	}()
 	ctx := context.Background()
 	namespace := k8s.DefaultNamespace()
 
@@ -128,7 +133,7 @@ func deployRemote(harnessDir string, gw gateway.Gateway, kc, clusterRunner k8s.R
 	if sps := os.Getenv("SANDBOX_PULL_SECRET"); sps != "" {
 		helmArgs = append(helmArgs, "--set", "server.sandboxImagePullSecrets[0].name="+sps)
 	}
-	if _, err := kc.RunHelm(ctx, helmArgs...); err != nil {
+	if err := kc.RunHelm(ctx, helmArgs...); err != nil {
 		return fmt.Errorf("helm install failed: %w", err)
 	}
 
@@ -166,7 +171,10 @@ func deployRemote(harnessDir string, gw gateway.Gateway, kc, clusterRunner k8s.R
 	}
 
 	// Extract mTLS certs
-	home, _ := os.UserHomeDir()
+	home, err := os.UserHomeDir()
+	if err != nil {
+		return fmt.Errorf("determining home directory: %w", err)
+	}
 	mtlsDir := filepath.Join(home, ".config", "openshell", "gateways", gatewayName, "mtls")
 	for _, field := range []string{"ca.crt", "tls.crt", "tls.key"} {
 		data, err := kc.GetSecretField(ctx, "openshell-client-tls", field)
@@ -178,21 +186,26 @@ func deployRemote(harnessDir string, gw gateway.Gateway, kc, clusterRunner k8s.R
 		}
 	}
 
-	gw.GatewaySelect(gatewayName)
+	if err := gw.GatewaySelect(gatewayName); err != nil {
+		return fmt.Errorf("selecting gateway %s: %w", gatewayName, err)
+	}
 	status.OKf("%s registered (certs from cluster)", gatewayName)
 
 	// Wait for gateway to be reachable
 	fmt.Print("  Waiting for gateway...")
-	for i := range 30 {
+	var gwReachable bool
+	for range 30 {
 		if gw.InferenceGet() == nil {
+			gwReachable = true
 			status.OK("reachable")
 			break
 		}
 		time.Sleep(2 * time.Second)
 		fmt.Print(".")
-		if i == 29 {
-			status.Fail("timed out (try: openshell inference get)")
-		}
+	}
+	if !gwReachable {
+		fmt.Println()
+		return fmt.Errorf("gateway not reachable after 60s (try: openshell inference get)")
 	}
 
 	fmt.Println()

cmd/helpers_test.go

@@ -0,0 +1,84 @@
+package cmd
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/robbycochran/harness-openshell/internal/gateway"
+)
+
+type mockGW struct {
+	inferenceErr      error
+	providers         map[string]bool
+	providerList      []string
+	providerErr       error
+	createErr         error
+	createCalls       int
+	createOpts        []gateway.SandboxCreateOpts
+	deletedNames      []string
+	gatewayListResult []gateway.GatewayInfo
+	onGatewayRemove   func(string)
+}
+
+func (m *mockGW) InferenceGet() error { return m.inferenceErr }
+func (m *mockGW) ProviderGet(name string) error {
+	if m.providers[name] {
+		return nil
+	}
+	return fmt.Errorf("not found")
+}
+func (m *mockGW) ProviderList() ([]string, error) { return m.providerList, m.providerErr }
+func (m *mockGW) SandboxCreate(opts gateway.SandboxCreateOpts) error {
+	m.createCalls++
+	m.createOpts = append(m.createOpts, opts)
+	if m.createErr != nil && m.createCalls == 1 {
+		return m.createErr
+	}
+	return nil
+}
+func (m *mockGW) SandboxDelete(name string) error {
+	m.deletedNames = append(m.deletedNames, name)
+	return nil
+}
+func (m *mockGW) CLIVersion() string                                              { return "openshell v0.0.55" }
+func (m *mockGW) CLIPath() string                                                 { return "/usr/bin/openshell" }
+func (m *mockGW) InferenceModel() string                                          { return "" }
+func (m *mockGW) InferenceSet(string, string) error                               { return nil }
+func (m *mockGW) InferenceRemove() error                                          { return nil }
+func (m *mockGW) ActiveGateway() string                                           { return "" }
+func (m *mockGW) ProviderCreate(string, string, gateway.ProviderCreateOpts) error { return nil }
+func (m *mockGW) ProviderDelete(string) error                                     { return nil }
+func (m *mockGW) ProviderProfileImport(string) error                              { return nil }
+func (m *mockGW) ProviderProfileDelete(string) error                              { return nil }
+func (m *mockGW) SettingsSet(string, string) error                                { return nil }
+func (m *mockGW) SandboxList() ([]string, error)                                  { return nil, nil }
+func (m *mockGW) SandboxConnect(string) error                                     { return nil }
+func (m *mockGW) GatewayAdd(string, string, bool) error                          { return nil }
+func (m *mockGW) GatewayRemove(name string) error {
+	if m.onGatewayRemove != nil {
+		m.onGatewayRemove(name)
+	}
+	return nil
+}
+func (m *mockGW) GatewayList() ([]gateway.GatewayInfo, error) {
+	return m.gatewayListResult, nil
+}
+func (m *mockGW) GatewaySelect(string) error { return nil }
+
+func setupTestProfile(t *testing.T) string {
+	t.Helper()
+	dir := t.TempDir()
+	os.MkdirAll(filepath.Join(dir, "profiles"), 0o755)
+	os.WriteFile(filepath.Join(dir, "profiles", "default.toml"), []byte(`
+name = "test-agent"
+image = "quay.io/test:latest"
+command = "claude --bare"
+providers = ["github", "vertex-local", "atlassian"]
+
+[env]
+FOO = "bar"
+`), 0o644)
+	return dir
+}

cmd/new.go

@@ -30,6 +30,9 @@ func NewNewCmd(harnessDir, cli string) *cobra.Command {
 		Short: "Create a new sandbox",
 		Long:  "Deploy gateway and providers if needed, then create a sandbox from a profile.",
 		RunE: func(cmd *cobra.Command, args []string) error {
+			if local && remote {
+				return fmt.Errorf("--local and --remote are mutually exclusive")
+			}
 			if len(args) > 0 && sandboxName == "" {
 				sandboxName = args[0]
 			}
@@ -278,10 +281,11 @@ func newLocal(opts newLocalOpts) error {
 	}
 
 	// 5. Stage files
-	harnessUploadDir := "/tmp/openshell"
-	if err := os.RemoveAll(harnessUploadDir); err != nil {
-		return fmt.Errorf("cleaning staging dir: %w", err)
+	harnessUploadDir, err := os.MkdirTemp("", "harness-")
+	if err != nil {
+		return fmt.Errorf("creating staging dir: %w", err)
 	}
+	defer os.RemoveAll(harnessUploadDir)
 	if err := profile.StageHarnessDir(cfg, harnessUploadDir); err != nil {
 		return fmt.Errorf("staging files: %w", err)
 	}
@@ -320,5 +324,5 @@ func newLocal(opts newLocalOpts) error {
 		}
 		time.Sleep(opts.retrySleep)
 	}
-	return nil
+	return nil // unreachable but required by compiler
 }

cmd/new_test.go

@@ -6,86 +6,8 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
-
-	"github.com/robbycochran/harness-openshell/internal/gateway"
 )
 
-type mockGW struct {
-	inferenceErr      error
-	providers         map[string]bool
-	providerList      []string
-	providerErr       error
-	createErr         error
-	createCalls       int
-	createOpts        []gateway.SandboxCreateOpts
-	deletedNames      []string
-	gatewayListResult []gateway.GatewayInfo
-	onGatewayRemove   func(string)
-}
-
-func (m *mockGW) InferenceGet() error { return m.inferenceErr }
-func (m *mockGW) ProviderGet(name string) error {
-	if m.providers[name] {
-		return nil
-	}
-	return fmt.Errorf("not found")
-}
-func (m *mockGW) ProviderList() ([]string, error) { return m.providerList, m.providerErr }
-func (m *mockGW) SandboxCreate(opts gateway.SandboxCreateOpts) error {
-	m.createCalls++
-	m.createOpts = append(m.createOpts, opts)
-	if m.createErr != nil && m.createCalls == 1 {
-		return m.createErr
-	}
-	return nil
-}
-func (m *mockGW) SandboxDelete(name string) error {
-	m.deletedNames = append(m.deletedNames, name)
-	return nil
-}
-func (m *mockGW) CLIVersion() string                                            { return "openshell v0.0.55" }
-func (m *mockGW) CLIPath() string                                               { return "/usr/bin/openshell" }
-func (m *mockGW) InferenceModel() string                                        { return "" }
-func (m *mockGW) InferenceSet(string, string) error                             { return nil }
-func (m *mockGW) InferenceRemove() error                                        { return nil }
-func (m *mockGW) ActiveGateway() string                                         { return "" }
-func (m *mockGW) ProviderCreate(string, string, gateway.ProviderCreateOpts) error { return nil }
-func (m *mockGW) ProviderDelete(string) error                                   { return nil }
-func (m *mockGW) ProviderProfileImport(string) error                            { return nil }
-func (m *mockGW) ProviderProfileDelete(string) error                            { return nil }
-func (m *mockGW) SettingsSet(string, string) error                              { return nil }
-func (m *mockGW) SandboxList() ([]string, error)                                { return nil, nil }
-func (m *mockGW) SandboxConnect(string) error                                   { return nil }
-func (m *mockGW) SandboxUpload(string, string, string) error                    { return nil }
-func (m *mockGW) SandboxExec(string, ...string) error                           { return nil }
-func (m *mockGW) GatewayAdd(string, string, bool) error { return nil }
-func (m *mockGW) GatewayRemove(name string) error {
-	if m.onGatewayRemove != nil {
-		m.onGatewayRemove(name)
-	}
-	return nil
-}
-func (m *mockGW) GatewayList() ([]gateway.GatewayInfo, error) {
-	return m.gatewayListResult, nil
-}
-func (m *mockGW) GatewaySelect(string) error { return nil }
-
-func setupTestProfile(t *testing.T) string {
-	t.Helper()
-	dir := t.TempDir()
-	os.MkdirAll(filepath.Join(dir, "profiles"), 0o755)
-	os.WriteFile(filepath.Join(dir, "profiles", "default.toml"), []byte(`
-name = "test-agent"
-image = "quay.io/test:latest"
-command = "claude --bare"
-providers = ["github", "vertex-local", "atlassian"]
-
-[env]
-FOO = "bar"
-`), 0o644)
-	return dir
-}
-
 func TestNewLocal_NoGateway(t *testing.T) {
 	dir := setupTestProfile(t)
 	gw := &mockGW{inferenceErr: fmt.Errorf("connection refused")}

cmd/preflight.go

@@ -1,6 +1,8 @@
 package cmd
 
 import (
+	"fmt"
+
 	"github.com/robbycochran/harness-openshell/internal/gateway"
 	"github.com/robbycochran/harness-openshell/internal/preflight"
 	"github.com/spf13/cobra"
@@ -13,13 +15,14 @@ func NewPreflightCmd(harnessDir, cli string) *cobra.Command {
 		Use:   "preflight",
 		Short: "Check environment prerequisites",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			// Support subcommands: available, names
 			if len(args) > 0 {
 				switch args[0] {
 				case "available":
 					return preflight.RunAvailable(harnessDir)
 				case "names":
 					return preflight.RunNames(harnessDir)
+				default:
+					return fmt.Errorf("unknown preflight subcommand: %s (use 'available' or 'names')", args[0])
 				}
 			}
 			gw := gateway.New(cli)

cmd/providers.go

@@ -10,7 +10,6 @@ import (
 
 	"github.com/robbycochran/harness-openshell/internal/gateway"
 	"github.com/robbycochran/harness-openshell/internal/status"
-	"github.com/robbycochran/harness-openshell/internal/util"
 	"github.com/spf13/cobra"
 )
 
@@ -98,7 +97,7 @@ func registerProviders(harnessDir string, gw gateway.Gateway, force bool) error
 		project = readADCProject(adcPath)
 	}
 
-	if util.FileExists(adcPath) && project != "" {
+	if fileExists(adcPath) && project != "" {
 		if gw.ProviderGet("vertex-local") != nil {
 			if err := gw.ProviderCreate("vertex-local", "google-vertex-ai", gateway.ProviderCreateOpts{
 				FromADC: true,
@@ -117,7 +116,7 @@ func registerProviders(harnessDir string, gw gateway.Gateway, force bool) error
 			return fmt.Errorf("setting inference: %w", err)
 		}
 		status.OKf("inference: model %s", model)
-	} else if !util.FileExists(adcPath) {
+	} else if !fileExists(adcPath) {
 		status.Infof("vertex-local: skipped (no ADC file at %s)", adcPath)
 	} else {
 		status.Info("vertex-local: skipped (no project ID — set ANTHROPIC_VERTEX_PROJECT_ID)")
@@ -190,6 +189,11 @@ func extractYAMLID(path string) string {
 	return ""
 }
 
+func fileExists(path string) bool {
+	_, err := os.Stat(path)
+	return err == nil
+}
+
 func readADCProject(path string) string {
 	data, err := os.ReadFile(path)
 	if err != nil {

cmd/teardown.go

@@ -24,9 +24,7 @@ func NewTeardownCmd(harnessDir, cli string) *cobra.Command {
 		Short: "Tear down sandboxes, providers, or k8s resources",
 		RunE: func(cmd *cobra.Command, args []string) error {
 			if !sandboxes && !providers && !k8sFlag {
-				sandboxes = true
-				providers = true
-				k8sFlag = true
+				return fmt.Errorf("specify at least one of --sandboxes, --providers, or --k8s")
 			}
 
 			gw := gateway.New(cli)
@@ -149,7 +147,7 @@ func teardownK8s(gw gateway.Gateway, kc, clusterRunner k8s.Runner) {
 
 	// Helm release
 	status.Section("Helm release")
-	if _, err := kc.RunHelm(ctx, "uninstall", "openshell"); err == nil {
+	if err := kc.RunHelm(ctx, "uninstall", "openshell"); err == nil {
 		status.Info("Uninstalled")
 	} else {
 		status.Info("Not installed")