From 4f11d72d088eb605aca561a93961f14ac27c5e4b Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Fri, 21 Aug 2026 21:14:35 -0400 Subject: [PATCH 1/3] feat(doctor): rewire online checks onto the SDK via injected Factory (PR1 S5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Doctor's Phase 2 (online) checks now read gateway health and provider registration through the OpenShell Go SDK via an injected openshell.Factory, instead of scraping the openshell CLI's active gateway. main.go wires sdkclient.New as the production Factory; tests wire testutil.FakeFactory. New flags: --gateway openshell registration name (NOT the harness profile); empty skips Phase 2 --workspace workspace for provider registration checks (default "default") checkOnline is deleted (hard cutover, no compat shim) and replaced by: - runOnlineChecks: non-fatal orchestrator — missing --gateway or any client-construction error yields a single warn (Phase 2 skipped), preserving doctor's long-standing non-fatal online contract - checkOnlineSDK: Health maps healthy->pass, ErrUnauthenticated->fail, ErrUnavailable/other->warn; providers map registered->pass, absent->warn All offline checks stay byte-for-byte; --output json/yaml shape unchanged. 13 existing offline tests pass unchanged; 5 new fake-backed online tests added. Firewall intact: doctor.go imports only the openshell vocabulary, no SDK. --- cmd/doctor.go | 112 ++++++++++++++++++++++++++++++++++++--------- cmd/doctor_test.go | 65 ++++++++++++++++++++++++++ main.go | 11 +++-- 3 files changed, 162 insertions(+), 26 deletions(-) diff --git a/cmd/doctor.go b/cmd/doctor.go index 4560c3a..d6a70c3 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -1,15 +1,17 @@ package cmd import ( + "context" + "errors" "fmt" "os" "os/exec" "path/filepath" "strings" - "github.com/stackrox/harness-openshell/internal/agent" - "github.com/stackrox/harness-openshell/internal/gateway" "github.com/spf13/cobra" + "github.com/stackrox/harness-openshell/internal/agent" + "github.com/stackrox/harness-openshell/internal/openshell" "gopkg.in/yaml.v3" ) @@ -22,11 +24,13 @@ type CheckResult struct { type CheckFunc func(cfg *agent.AgentConfig, cli, harnessDir string) []CheckResult -func NewDoctorCmd(harnessDir, cli string) *cobra.Command { +func NewDoctorCmd(harnessDir, cli string, newClient openshell.Factory) *cobra.Command { var ( - agentFile string - agentName string - output string + agentFile string + agentName string + output string + gatewayName string + workspace string ) cmd := &cobra.Command{ @@ -60,7 +64,11 @@ Phase 2 (online): if the gateway is reachable, checks provider registration.`, results = append(results, fn(h.Agent, cli, harnessDir)...) } - results = append(results, checkOnline(h.Agent, cli)...) + providerProfiles := make([]string, 0, len(h.Agent.Providers)) + for _, p := range h.Agent.Providers { + providerProfiles = append(providerProfiles, p.Profile) + } + results = append(results, runOnlineChecks(cmd.Context(), newClient, gatewayName, workspace, providerProfiles)...) if format != formatTable { return printStructured(format, results) @@ -80,6 +88,8 @@ 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). Empty skips online checks.") + cmd.Flags().StringVar(&workspace, "workspace", "default", "Workspace for provider registration checks") return cmd } @@ -179,8 +189,8 @@ func checkRemoteDeps() []CheckResult { } type providerProfile struct { - ID string `yaml:"id"` - DisplayName string `yaml:"display_name"` + ID string `yaml:"id"` + DisplayName string `yaml:"display_name"` Credentials []providerCredential `yaml:"credentials"` } @@ -328,47 +338,107 @@ func loadProfileFromDisk(name, harnessDir string) *providerProfile { return nil } -func checkOnline(cfg *agent.AgentConfig, cli string) []CheckResult { - gw := gateway.New(cli) - if gw.ActiveGateway() == "" { +// 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 == "" { return []CheckResult{{ Group: "gateway", Name: "status", Status: "warn", - Message: "no active gateway (Phase 2 checks skipped)", + Message: "Phase 2 (online) checks skipped: no --gateway specified", }} } - _, err := gw.ProviderList() + client, err := newClient(ctx, openshell.Target{Gateway: gatewayName, Workspace: workspace}) if err != nil { + return []CheckResult{{ + Group: "gateway", + Name: "status", + Status: "warn", + Message: fmt.Sprintf("Phase 2 (online) checks skipped: %v", err), + }} + } + defer client.Close() + + return checkOnlineSDK(ctx, client, providers) +} + +// checkOnlineSDK reads gateway health and provider registration through the +// SDK client. Health maps: healthy -> pass; ErrUnauthenticated -> fail (the +// one actionable online failure); ErrUnavailable and any other error -> warn +// (non-fatal). Provider rows are only produced when the gateway is healthy. +func checkOnlineSDK(ctx context.Context, client openshell.Client, providers []string) []CheckResult { + h, err := client.Health(ctx) + switch { + case err == nil && h.Healthy: + // fall through to provider checks below + case errors.Is(err, openshell.ErrUnauthenticated): + return []CheckResult{{ + Group: "gateway", + Name: "status", + Status: "fail", + Message: "authentication failed — check gateway credentials", + }} + case errors.Is(err, openshell.ErrUnavailable): return []CheckResult{{ Group: "gateway", Name: "status", Status: "warn", Message: "gateway not reachable (Phase 2 checks skipped)", }} + case err != nil: + return []CheckResult{{ + Group: "gateway", + Name: "status", + Status: "warn", + Message: fmt.Sprintf("gateway health check failed: %v (Phase 2 checks skipped)", err), + }} + default: // err == nil but not healthy + return []CheckResult{{ + Group: "gateway", + Name: "status", + Status: "warn", + Message: "gateway reports unhealthy (Phase 2 checks skipped)", + }} } - var results []CheckResult - results = append(results, CheckResult{ + results := []CheckResult{{ Group: "gateway", Name: "status", Status: "pass", Message: "connected", - }) + }} - for _, p := range cfg.Providers { - if gw.ProviderGet(p.Profile) == nil { + provs, err := client.Providers(ctx) + if err != nil { + results = append(results, CheckResult{ + Group: "gateway", + Name: "providers", + Status: "warn", + Message: fmt.Sprintf("could not list providers: %v", err), + }) + return results + } + + registered := make(map[string]bool, len(provs)) + for _, p := range provs { + registered[p.Name] = true + } + for _, name := range providers { + if registered[name] { results = append(results, CheckResult{ Group: "gateway", - Name: p.Profile, + Name: name, Status: "pass", Message: "registered", }) } else { results = append(results, CheckResult{ Group: "gateway", - Name: p.Profile, + Name: name, Status: "warn", Message: "not registered (will be registered on apply)", }) diff --git a/cmd/doctor_test.go b/cmd/doctor_test.go index 517d42e..82870f0 100644 --- a/cmd/doctor_test.go +++ b/cmd/doctor_test.go @@ -1,11 +1,16 @@ package cmd import ( + "context" "os" "path/filepath" "testing" + fake "github.com/NVIDIA/OpenShell/sdk/go/openshell/v1/fake" + "github.com/NVIDIA/OpenShell/sdk/go/openshell/v1/types" "github.com/stackrox/harness-openshell/internal/agent" + "github.com/stackrox/harness-openshell/internal/openshell" + "github.com/stackrox/harness-openshell/internal/testutil" ) func TestCheckOpenShell_Found(t *testing.T) { @@ -251,3 +256,63 @@ func writeProviderProfile(t *testing.T, harnessDir, name, content string) { t.Fatal(err) } } + +func TestCheckOnlineSDK_Healthy(t *testing.T) { + c := testutil.NewFake("default", fake.WithHealthResult(&types.HealthResult{Healthy: true, Version: "1.0.0"})) + results := checkOnlineSDK(context.Background(), c, nil) + if len(results) != 1 { + t.Fatalf("expected 1 result, got %d", len(results)) + } + if results[0].Group != "gateway" || results[0].Name != "status" || results[0].Status != "pass" { + t.Errorf("unexpected status result: %+v", results[0]) + } +} + +func TestCheckOnlineSDK_ProviderRegistered(t *testing.T) { + c, raw := testutil.NewFakeClient("default", fake.WithHealthResult(&types.HealthResult{Healthy: true})) + raw.AddProvider("default", &types.Provider{Name: "github", Type: "git"}) + results := checkOnlineSDK(context.Background(), c, []string{"github"}) + // results[0] is gateway/status pass; results[1] is the provider row. + if len(results) != 2 { + t.Fatalf("expected 2 results, got %d: %+v", len(results), results) + } + if results[1].Name != "github" || results[1].Status != "pass" || results[1].Message != "registered" { + t.Errorf("expected github registered pass, got %+v", results[1]) + } +} + +func TestCheckOnlineSDK_ProviderNotRegistered(t *testing.T) { + c := testutil.NewFake("default", fake.WithHealthResult(&types.HealthResult{Healthy: true})) + results := checkOnlineSDK(context.Background(), c, []string{"github"}) + if len(results) != 2 { + t.Fatalf("expected 2 results, got %d: %+v", len(results), results) + } + if results[1].Name != "github" || results[1].Status != "warn" { + t.Errorf("expected github not-registered warn, got %+v", results[1]) + } +} + +func TestRunOnlineChecks_NoGateway(t *testing.T) { + // No --gateway: single non-fatal warn, factory never called. + called := false + f := func(ctx context.Context, tgt openshell.Target) (openshell.Client, error) { + called = true + return nil, nil + } + results := runOnlineChecks(context.Background(), f, "", "default", nil) + if len(results) != 1 || results[0].Status != "warn" { + t.Fatalf("expected 1 warn result, got %+v", results) + } + if called { + t.Error("factory should not be called when --gateway is empty") + } +} + +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) + if len(results) != 1 || results[0].Status != "pass" { + t.Fatalf("expected 1 pass result, got %+v", results) + } +} diff --git a/main.go b/main.go index b8c6efb..b778bed 100644 --- a/main.go +++ b/main.go @@ -6,9 +6,10 @@ import ( "os" "path/filepath" + "github.com/spf13/cobra" "github.com/stackrox/harness-openshell/cmd" + "github.com/stackrox/harness-openshell/internal/openshell/sdkclient" "github.com/stackrox/harness-openshell/internal/status" - "github.com/spf13/cobra" ) var version = "dev" @@ -54,9 +55,9 @@ func main() { cmd.Version = version cmd.DefaultAgentConfig = defaultAgentConfig cmd.EmbeddedGatewayProfiles = map[string][]byte{ - "local-container": localContainerGatewayProfile, - "helm": helmNodeportGatewayProfile, - "openshift": helmOpenshiftRouteGatewayProfile, + "local-container": localContainerGatewayProfile, + "helm": helmNodeportGatewayProfile, + "openshift": helmOpenshiftRouteGatewayProfile, } root.CompletionOptions.HiddenDefaultCmd = true @@ -66,7 +67,7 @@ func main() { cmd.NewDescribeCmd(harnessDir, cli), cmd.NewDeleteCmd(harnessDir, cli), cmd.NewDeployCmd(harnessDir, cli), - cmd.NewDoctorCmd(harnessDir, cli), + cmd.NewDoctorCmd(harnessDir, cli, sdkclient.New), cmd.NewInitCmd(harnessDir), ) From 304393da7cc49ff147f7116e98769c51c95f89c8 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Fri, 21 Aug 2026 21:24:24 -0400 Subject: [PATCH 2/3] feat(doctor): resolve --gateway/--workspace via flag > env > default Apply the repo's standard flag-resolution order (AGENTS.md: explicit flag > OPENSHELL_* env var > default) to doctor's new online flags. Previously they used bare Cobra defaults, so $OPENSHELL_GATEWAY was never consulted and the "default" Cobra default on --workspace made an explicit value indistinguishable from unset, blocking the env fallback. Both flags now default to "" and are resolved in RunE via resolveOnlineFlag: --gateway -> flag > $OPENSHELL_GATEWAY > "" (empty skips Phase 2) --workspace -> flag > $OPENSHELL_WORKSPACE > "default" No config-file tier is wired: the only harness config gateway value is the harness *profile* (agent.AgentConfig.Gateway), a distinct namespace from the openshell registration name and never conflated with it. (CodeRabbit PR #93) --- cmd/doctor.go | 30 +++++++++++++++++++++++++++--- cmd/doctor_test.go | 21 +++++++++++++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/cmd/doctor.go b/cmd/doctor.go index d6a70c3..437847d 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -68,7 +68,12 @@ Phase 2 (online): if the gateway is reachable, checks provider registration.`, for _, p := range h.Agent.Providers { providerProfiles = append(providerProfiles, p.Profile) } - results = append(results, runOnlineChecks(cmd.Context(), newClient, gatewayName, workspace, providerProfiles)...) + // 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)...) if format != formatTable { return printStructured(format, results) @@ -88,8 +93,8 @@ 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). Empty skips online checks.") - cmd.Flags().StringVar(&workspace, "workspace", "default", "Workspace for provider registration checks") + 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)") return cmd } @@ -338,6 +343,25 @@ 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 diff --git a/cmd/doctor_test.go b/cmd/doctor_test.go index 82870f0..6c20365 100644 --- a/cmd/doctor_test.go +++ b/cmd/doctor_test.go @@ -316,3 +316,24 @@ func TestRunOnlineChecks_HealthyViaFactory(t *testing.T) { 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) + } + + // 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) + } + + // 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) + } +} From 3fdac0ac0915b848e1ed83e05d7de8dcbf79e0f6 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Fri, 21 Aug 2026 21:41:30 -0400 Subject: [PATCH 3/3] ci: re-trigger CI (lint runner hung on prior SHA) No code change. The lint lane on 304393d hung in golangci-lint-action (~15min vs the usual ~30s) and would not cancel; this empty commit starts a fresh run. Squashed away on merge.