feat: PR2 — target resolution seam (openshell.ResolveTarget)

cmd/doctor.go

@@ -26,12 +26,12 @@ type CheckFunc func(cfg *agent.AgentConfig, cli, harnessDir string) []CheckResul
 
 func NewDoctorCmd(harnessDir, cli string, newClient openshell.Factory) *cobra.Command {
 	var (
-		agentFile   string
-		agentName   string
-		output      string
-		gatewayName string
-		workspace   string
+		agentFile string
+		agentName string
+		output    string
 	)
+	// Assigned by registerTargetFlags below; RunE reads them at execution time.
+	var gatewayName, workspace *string
 
 	cmd := &cobra.Command{
 		Use:   "doctor",
@@ -68,12 +68,11 @@ Phase 2 (online): if the gateway is reachable, checks provider registration.`,
 			for _, p := range h.Agent.Providers {
 				providerProfiles = append(providerProfiles, p.Profile)
 			}
-			// Flag resolution order (AGENTS.md): explicit flag > OPENSHELL_* env
-			// var > default. Empty flag defaults let the env fallback apply; an
-			// unset gateway (flag and env both empty) skips Phase 2.
-			gw := resolveOnlineFlag(gatewayName, "OPENSHELL_GATEWAY", "")
-			ws := resolveOnlineFlag(workspace, "OPENSHELL_WORKSPACE", defaultDoctorWorkspace)
-			results = append(results, runOnlineChecks(cmd.Context(), newClient, gw, ws, providerProfiles)...)
+			// Flag/env precedence (flag > env > empty) is owned by
+			// openshell.ResolveTarget; an unset gateway (flag and env both empty)
+			// skips Phase 2.
+			target := openshell.ResolveTarget(*gatewayName, *workspace, os.Getenv)
+			results = append(results, runOnlineChecks(cmd.Context(), newClient, target, providerProfiles)...)
 
 			if format != formatTable {
 				return printStructured(format, results)
@@ -93,8 +92,7 @@ Phase 2 (online): if the gateway is reachable, checks provider registration.`,
 	cmd.Flags().StringVarP(&agentFile, "file", "f", "", "Path to harness YAML")
 	cmd.Flags().StringVar(&agentName, "agent", "default", "Agent config name")
 	cmd.Flags().StringVarP(&output, "output", "o", "", "Output format (table, json, yaml)")
-	cmd.Flags().StringVar(&gatewayName, "gateway", "", "OpenShell registration name for online checks (Phase 2). Defaults to $OPENSHELL_GATEWAY; empty skips online checks.")
-	cmd.Flags().StringVar(&workspace, "workspace", "", "Workspace for provider registration checks (default \"default\"; overridable via $OPENSHELL_WORKSPACE)")
+	gatewayName, workspace = registerTargetFlags(cmd)
 
 	return cmd
 }
@@ -343,31 +341,12 @@ func loadProfileFromDisk(name, harnessDir string) *providerProfile {
 	return nil
 }
 
-// defaultDoctorWorkspace is the workspace used when neither --workspace nor
-// $OPENSHELL_WORKSPACE is set. sdkclient also defaults "" -> "default"; this
-// keeps the resolved value explicit for logging and table output.
-const defaultDoctorWorkspace = "default"
-
-// resolveOnlineFlag applies the repo's standard flag-resolution order
-// (AGENTS.md: explicit flag > OPENSHELL_* env var > default) for doctor's
-// online flags. An empty flag value is treated as "unset" so the env var can
-// take effect.
-func resolveOnlineFlag(flagVal, envKey, def string) string {
-	if flagVal != "" {
-		return flagVal
-	}
-	if v := os.Getenv(envKey); v != "" {
-		return v
-	}
-	return def
-}
-
 // runOnlineChecks performs Phase 2 (online) checks via the SDK. It is
 // non-fatal by construction: a missing --gateway or any client-construction
 // failure yields a single warn (Phase 2 skipped), never a fail, preserving
 // doctor's long-standing "online failures don't break the build" contract.
-func runOnlineChecks(ctx context.Context, newClient openshell.Factory, gatewayName, workspace string, providers []string) []CheckResult {
-	if gatewayName == "" {
+func runOnlineChecks(ctx context.Context, newClient openshell.Factory, target openshell.Target, providers []string) []CheckResult {
+	if target.Gateway == "" {
 		return []CheckResult{{
 			Group:   "gateway",
 			Name:    "status",
@@ -376,7 +355,7 @@ func runOnlineChecks(ctx context.Context, newClient openshell.Factory, gatewayNa
 		}}
 	}
 
-	client, err := newClient(ctx, openshell.Target{Gateway: gatewayName, Workspace: workspace})
+	client, err := newClient(ctx, target)
 	if err != nil {
 		return []CheckResult{{
 			Group:   "gateway",

cmd/doctor_test.go

@@ -236,6 +236,62 @@ credentials:
 	}
 }
 
+// TestDoctorCmd_TargetFlagWiring exercises the full flag-pointer path through
+// cobra: registerTargetFlags populates the *string pointers on Parse, and the
+// RunE closure feeds them to openshell.ResolveTarget. It guards the plumbing at
+// doctor.go's flag registration and dereference that the direct runOnlineChecks
+// tests never touch. Offline checks may fail against the test env; we assert
+// only which gateway the Factory was constructed for.
+func TestDoctorCmd_TargetFlagWiring(t *testing.T) {
+	tests := []struct {
+		name        string
+		args        []string
+		env         string // value for $OPENSHELL_GATEWAY ("" = unset)
+		wantGateway string
+	}{
+		{name: "flag", args: []string{"--gateway", "from-flag"}, wantGateway: "from-flag"},
+		{name: "env fallback", args: nil, env: "from-env", wantGateway: "from-env"},
+		{name: "flag wins over env", args: []string{"--gateway", "from-flag"}, env: "from-env", wantGateway: "from-flag"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// doctor's offline phase resolves a harness config; provide an
+			// embedded default so RunE reaches the online path we are testing.
+			DefaultAgentConfig = []byte("name: wiring-test\nentrypoint: claude\n")
+			t.Cleanup(func() { DefaultAgentConfig = nil })
+
+			if tt.env != "" {
+				t.Setenv(openshell.EnvGateway, tt.env)
+			}
+			c := testutil.NewFake("default", fake.WithHealthResult(&types.HealthResult{Healthy: true}))
+			var gotGateway string
+			factoryCalled := false
+			recording := func(_ context.Context, tgt openshell.Target) (openshell.Client, error) {
+				factoryCalled = true
+				gotGateway = tgt.Gateway
+				return c, nil
+			}
+
+			cmd := NewDoctorCmd(t.TempDir(), "nonexistent-cli", recording)
+			cmd.SilenceUsage = true
+			cmd.SilenceErrors = true
+			cmd.SetArgs(append(tt.args, "--output", "json"))
+			// Offline checks may report failures in the test environment; RunE
+			// then returns a non-nil error after the online path has run. We only
+			// care that the Factory saw the resolved gateway.
+			_ = cmd.Execute()
+
+			if !factoryCalled {
+				t.Fatal("Factory was never called; online path did not run")
+			}
+			if gotGateway != tt.wantGateway {
+				t.Errorf("Factory got gateway %q, want %q", gotGateway, tt.wantGateway)
+			}
+		})
+	}
+}
+
 // --- helpers ---
 
 func testAgentConfig(t *testing.T) *agent.AgentConfig {
@@ -299,7 +355,7 @@ func TestRunOnlineChecks_NoGateway(t *testing.T) {
 		called = true
 		return nil, nil
 	}
-	results := runOnlineChecks(context.Background(), f, "", "default", nil)
+	results := runOnlineChecks(context.Background(), f, openshell.Target{}, nil)
 	if len(results) != 1 || results[0].Status != "warn" {
 		t.Fatalf("expected 1 warn result, got %+v", results)
 	}
@@ -311,29 +367,36 @@ func TestRunOnlineChecks_NoGateway(t *testing.T) {
 func TestRunOnlineChecks_HealthyViaFactory(t *testing.T) {
 	c := testutil.NewFake("default", fake.WithHealthResult(&types.HealthResult{Healthy: true, Version: "1.0.0"}))
 	f := testutil.FakeFactory(c)
-	results := runOnlineChecks(context.Background(), f, "some-gateway", "default", nil)
+	results := runOnlineChecks(context.Background(), f, openshell.Target{Gateway: "some-gateway", Workspace: "default"}, nil)
 	if len(results) != 1 || results[0].Status != "pass" {
 		t.Fatalf("expected 1 pass result, got %+v", results)
 	}
 }
 
-func TestResolveOnlineFlag(t *testing.T) {
-	const envKey = "OPENSHELL_TEST_RESOLVE"
-
-	// Explicit flag wins over env var and default.
-	t.Setenv(envKey, "from-env")
-	if got := resolveOnlineFlag("from-flag", envKey, "from-default"); got != "from-flag" {
-		t.Errorf("flag precedence: got %q, want from-flag", got)
+// TestRunOnlineChecks_GatewayIsolation is the plan's acceptance test: naming one
+// gateway never constructs or queries another. A recording Factory captures every
+// Target.Gateway it is asked to build; doctor's online path is run with gateway A
+// and the recorder must show exactly one construction, for A only — B is never
+// touched.
+func TestRunOnlineChecks_GatewayIsolation(t *testing.T) {
+	fakeA := testutil.NewFake("default", fake.WithHealthResult(&types.HealthResult{Healthy: true}))
+	fakeB := testutil.NewFake("default", fake.WithHealthResult(&types.HealthResult{Healthy: true}))
+	clients := map[string]openshell.Client{"A": fakeA, "B": fakeB}
+
+	var constructed []string
+	recording := func(_ context.Context, tgt openshell.Target) (openshell.Client, error) {
+		constructed = append(constructed, tgt.Gateway)
+		c, ok := clients[tgt.Gateway]
+		if !ok {
+			t.Fatalf("factory asked for unknown gateway %q", tgt.Gateway)
+		}
+		return c, nil
 	}
 
-	// Empty flag falls back to the env var over the default.
-	if got := resolveOnlineFlag("", envKey, "from-default"); got != "from-env" {
-		t.Errorf("env precedence: got %q, want from-env", got)
-	}
+	runOnlineChecks(context.Background(), recording,
+		openshell.Target{Gateway: "A", Workspace: "default"}, nil)
 
-	// Empty flag and unset env fall back to the default.
-	t.Setenv(envKey, "")
-	if got := resolveOnlineFlag("", envKey, "from-default"); got != "from-default" {
-		t.Errorf("default fallback: got %q, want from-default", got)
+	if len(constructed) != 1 || constructed[0] != "A" {
+		t.Fatalf("expected exactly one construction for gateway A, got %v", constructed)
 	}
 }

cmd/target.go

@@ -0,0 +1,26 @@
+package cmd
+
+import (
+	"fmt"
+
+	"github.com/spf13/cobra"
+	"github.com/stackrox/harness-openshell/internal/openshell"
+)
+
+// registerTargetFlags adds the standard --gateway/--workspace flags to cmd and
+// returns pointers to their values. Every SDK-backed command registers its
+// target flags through this one helper so the flag names, help text, and the
+// OPENSHELL_* env fallbacks stay identical across commands. The flags are
+// per-command (not root-persistent) so legacy CLI-path commands never carry
+// them.
+//
+// The returned pointers are meant to be fed to openshell.ResolveTarget together
+// with os.Getenv; resolution (flag > env > empty) and the workspace default live
+// there and in sdkclient, not here.
+func registerTargetFlags(cmd *cobra.Command) (gateway, workspace *string) {
+	gateway = cmd.Flags().String("gateway", "",
+		fmt.Sprintf("OpenShell gateway registration name (falls back to $%s).", openshell.EnvGateway))
+	workspace = cmd.Flags().String("workspace", "",
+		fmt.Sprintf("OpenShell workspace (defaults to %q; falls back to $%s).", "default", openshell.EnvWorkspace))
+	return gateway, workspace
+}

internal/openshell/sdkclient/client.go

@@ -50,11 +50,6 @@ func New(ctx context.Context, t openshell.Target) (openshell.Client, error) {
 		return nil, fmt.Errorf("%w: load gateway %q: %v", openshell.ErrConfig, t.Gateway, err)
 	}
 
-	ws := t.Workspace
-	if ws == "" {
-		ws = defaultWorkspace
-	}
-
 	plan, err := planConnection(cfg, os.Getenv)
 	if err != nil {
 		return nil, err
@@ -65,7 +60,9 @@ func New(ctx context.Context, t openshell.Target) (openshell.Client, error) {
 		return nil, err
 	}
 
-	return NewFromClient(raw, ws), nil
+	// NewFromClient is the single owner of the "" -> defaultWorkspace default;
+	// pass t.Workspace straight through.
+	return NewFromClient(raw, t.Workspace), nil
 }
 
 // NewFromClient wraps an existing SDK client (or the SDK fake) bound to a

internal/openshell/sdkclient/client_test.go

@@ -128,6 +128,25 @@ func TestProviders(t *testing.T) {
 	}
 }
 
+// TestNewFromClientDefaultsWorkspace pins the single-owner workspace default:
+// an empty workspace binds the client to "default". A provider registered under
+// "default" is visible through a client constructed with "" — proving the
+// default lives here and nowhere else (New passes t.Workspace straight through).
+func TestNewFromClientDefaultsWorkspace(t *testing.T) {
+	ctx := context.Background()
+	fc := fake.NewClient()
+	fc.AddProvider("default", &types.Provider{Name: "p1", Type: "openai"})
+
+	c := NewFromClient(fc, "")
+	providers, err := c.Providers(ctx)
+	if err != nil {
+		t.Fatalf("Providers() returned unexpected error: %v", err)
+	}
+	if len(providers) != 1 || providers[0].Name != "p1" {
+		t.Errorf("empty workspace should bind to %q; got providers %+v", defaultWorkspace, providers)
+	}
+}
+
 func TestProvidersErrorTranslated(t *testing.T) {
 	ctx := context.Background()
 	fc := fake.NewClient()

internal/openshell/target.go

@@ -0,0 +1,36 @@
+package openshell
+
+// Environment variables consulted by ResolveTarget, in the repo's standard
+// flag-resolution order (AGENTS.md: explicit flag > OPENSHELL_* env var >
+// default). Exported so cmd help text and tests can name them without
+// re-declaring the strings.
+const (
+	EnvGateway   = "OPENSHELL_GATEWAY"
+	EnvWorkspace = "OPENSHELL_WORKSPACE"
+)
+
+// ResolveTarget builds a Target from explicit flag values and the environment,
+// applying flag > env > empty for each field independently.
+//
+// It does NOT default the workspace: an unset workspace stays "" and sdkclient
+// maps "" -> "default" at construction (the single owner of that default). An
+// empty Gateway is likewise left empty — a caller decision (e.g. doctor skips
+// its online checks), never a silent fallback to the CLI's active gateway.
+//
+// Resolution is pure: getenv is injected (production passes os.Getenv, tests
+// pass a map closure) so this package imports neither os nor any CLI framework.
+func ResolveTarget(flagGateway, flagWorkspace string, getenv func(string) string) Target {
+	return Target{
+		Gateway:   resolveField(flagGateway, EnvGateway, getenv),
+		Workspace: resolveField(flagWorkspace, EnvWorkspace, getenv),
+	}
+}
+
+// resolveField applies flag > env > empty for one field. An empty flag value is
+// treated as unset so the env var can take effect.
+func resolveField(flagVal, envKey string, getenv func(string) string) string {
+	if flagVal != "" {
+		return flagVal
+	}
+	return getenv(envKey)
+}