From 4820565f4fea1901a950db3da3e9b74696c17e8d Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Mon, 24 Aug 2026 09:01:51 -0700 Subject: [PATCH 1/2] feat(openshell): add ResolveTarget as the one owner of target resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce openshell.ResolveTarget(flagGateway, flagWorkspace, getenv) — the single, pure place flag > env > empty precedence builds a Target. Adds the EnvGateway/EnvWorkspace constants so cmd help text and tests name the OPENSHELL_* env tier without re-declaring strings. Resolution takes an injected getenv, so internal/openshell stays SDK-free and cobra-free. ResolveTarget does no defaulting: an unset workspace stays "" and sdkclient remains the single owner of "" -> "default". Folds in the PR1 audit nit by removing the redundant workspace default in sdkclient.New (pass t.Workspace straight to NewFromClient), pinned by TestNewFromClientDefaultsWorkspace. --- internal/openshell/sdkclient/client.go | 9 +-- internal/openshell/sdkclient/client_test.go | 19 ++++++ internal/openshell/target.go | 36 ++++++++++ internal/openshell/target_test.go | 73 +++++++++++++++++++++ 4 files changed, 131 insertions(+), 6 deletions(-) create mode 100644 internal/openshell/target.go create mode 100644 internal/openshell/target_test.go diff --git a/internal/openshell/sdkclient/client.go b/internal/openshell/sdkclient/client.go index 49f385d..16f3651 100644 --- a/internal/openshell/sdkclient/client.go +++ b/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 diff --git a/internal/openshell/sdkclient/client_test.go b/internal/openshell/sdkclient/client_test.go index cf78787..588b088 100644 --- a/internal/openshell/sdkclient/client_test.go +++ b/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() diff --git a/internal/openshell/target.go b/internal/openshell/target.go new file mode 100644 index 0000000..b332e43 --- /dev/null +++ b/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) +} diff --git a/internal/openshell/target_test.go b/internal/openshell/target_test.go new file mode 100644 index 0000000..fbde9ce --- /dev/null +++ b/internal/openshell/target_test.go @@ -0,0 +1,73 @@ +package openshell + +import "testing" + +// mapEnv returns a getenv closure backed by m, so ResolveTarget can be tested +// without touching the process environment (the function is pure). +func mapEnv(m map[string]string) func(string) string { + return func(k string) string { return m[k] } +} + +func TestResolveTarget(t *testing.T) { + tests := []struct { + name string + flagGateway string + flagWorkspace string + env map[string]string + wantGateway string + wantWorkspace string + }{ + { + name: "flag wins over env", + flagGateway: "flag-gw", + flagWorkspace: "flag-ws", + env: map[string]string{EnvGateway: "env-gw", EnvWorkspace: "env-ws"}, + wantGateway: "flag-gw", + wantWorkspace: "flag-ws", + }, + { + name: "env wins when flag empty", + flagGateway: "", + flagWorkspace: "", + env: map[string]string{EnvGateway: "env-gw", EnvWorkspace: "env-ws"}, + wantGateway: "env-gw", + wantWorkspace: "env-ws", + }, + { + name: "empty flag and env stays empty (no defaulting)", + flagGateway: "", + flagWorkspace: "", + env: map[string]string{}, + wantGateway: "", + wantWorkspace: "", + }, + { + name: "flag wins when env empty", + flagGateway: "flag-gw", + flagWorkspace: "flag-ws", + env: map[string]string{}, + wantGateway: "flag-gw", + wantWorkspace: "flag-ws", + }, + { + name: "fields resolve independently", + flagGateway: "flag-gw", + flagWorkspace: "", + env: map[string]string{EnvWorkspace: "env-ws"}, + wantGateway: "flag-gw", + wantWorkspace: "env-ws", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ResolveTarget(tt.flagGateway, tt.flagWorkspace, mapEnv(tt.env)) + if got.Gateway != tt.wantGateway { + t.Errorf("Gateway = %q, want %q", got.Gateway, tt.wantGateway) + } + if got.Workspace != tt.wantWorkspace { + t.Errorf("Workspace = %q, want %q", got.Workspace, tt.wantWorkspace) + } + }) + } +} From d0ca316bd6bf23bce70a5b08fb71a7e7ae665cac Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Mon, 24 Aug 2026 09:04:37 -0700 Subject: [PATCH 2/2] feat(doctor): migrate onto openshell.ResolveTarget; add shared flag helper Route doctor's --gateway/--workspace through the new openshell.ResolveTarget and a reusable cmd/target.go flag helper (registerTargetFlags) that every future SDK-backed command will share. The flags stay per-command, not root-persistent, so legacy CLI-path commands never carry them. Delete doctor's private resolveOnlineFlag and defaultDoctorWorkspace (hard cutover, no dual path); runOnlineChecks now takes an openshell.Target and skips Phase 2 when target.Gateway is empty. Precedence coverage moves from the deleted TestResolveOnlineFlag to S1's TestResolveTarget. Adds a gateway-isolation test: --gateway A constructs A exactly once and never touches B. Live mTLS smoke: doctor --gateway openshell and OPENSHELL_GATEWAY=openshell doctor both report gateway status connected. --- cmd/doctor.go | 49 +++++++---------------- cmd/doctor_test.go | 97 ++++++++++++++++++++++++++++++++++++++-------- cmd/target.go | 26 +++++++++++++ 3 files changed, 120 insertions(+), 52 deletions(-) create mode 100644 cmd/target.go diff --git a/cmd/doctor.go b/cmd/doctor.go index 437847d..61c273f 100644 --- a/cmd/doctor.go +++ b/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", diff --git a/cmd/doctor_test.go b/cmd/doctor_test.go index 6c20365..9a17fdc 100644 --- a/cmd/doctor_test.go +++ b/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) } } diff --git a/cmd/target.go b/cmd/target.go new file mode 100644 index 0000000..aa97111 --- /dev/null +++ b/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 +}