From f8621483c0ae42cd4e005f0bf1707f94f4a38031 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 08:14:53 -0700 Subject: [PATCH 1/9] feat(openshell): widen firewall Provider with Config/Labels (PR4a S1) Widen the harness Provider read view with non-secret Config and Labels, and extend fromSDKProvider to copy them as fresh maps. Deliberately no Credentials field (write-only; never returned by Get) and no ResourceVersion (OCC token stays inside sdkclient), so reconcile can neither read nor author a secret. Unblocks the provider diff rule and reconcile engine. --- internal/openshell/sdkclient/provider.go | 29 +++++++-- internal/openshell/sdkclient/provider_test.go | 64 +++++++++++++++++++ internal/openshell/types.go | 17 +++-- 3 files changed, 101 insertions(+), 9 deletions(-) create mode 100644 internal/openshell/sdkclient/provider_test.go diff --git a/internal/openshell/sdkclient/provider.go b/internal/openshell/sdkclient/provider.go index ba55804..1035d08 100644 --- a/internal/openshell/sdkclient/provider.go +++ b/internal/openshell/sdkclient/provider.go @@ -6,9 +6,30 @@ import ( "github.com/stackrox/harness-openshell/internal/openshell" ) -// fromSDKProvider maps the SDK provider view to the minimal harness Provider. -// Deliberately narrow (least-exposure firewall); widen only when a consumer -// genuinely needs more fields, changing this and openshell.Provider together. +// fromSDKProvider maps the SDK provider view to the harness Provider. It copies +// only the non-secret Config and Labels (as fresh maps, never aliasing the SDK +// object); Spec.Credentials, CredentialHandles, and ResourceVersion are +// deliberately dropped at this boundary (least-exposure firewall — see the +// openshell.Provider doc). Widen only when a consumer genuinely needs more +// fields, changing this and openshell.Provider together. func fromSDKProvider(p *v1.Provider) openshell.Provider { - return openshell.Provider{Name: p.Name, Type: p.Type} + return openshell.Provider{ + Name: p.Name, + Type: p.Type, + Config: copyStringMap(p.Spec.Config), + Labels: copyStringMap(p.Labels), + } +} + +// copyStringMap returns a fresh copy of m, or nil when m is empty, so the +// harness view never aliases the SDK object's maps. +func copyStringMap(m map[string]string) map[string]string { + if len(m) == 0 { + return nil + } + out := make(map[string]string, len(m)) + for k, v := range m { + out[k] = v + } + return out } diff --git a/internal/openshell/sdkclient/provider_test.go b/internal/openshell/sdkclient/provider_test.go new file mode 100644 index 0000000..6e0ec20 --- /dev/null +++ b/internal/openshell/sdkclient/provider_test.go @@ -0,0 +1,64 @@ +package sdkclient + +import ( + "testing" + "time" + + "github.com/NVIDIA/OpenShell/sdk/go/openshell/v1/types" +) + +// TestFromSDKProviderMapsConfigAndLabels pins the S1 read-widening: the harness +// Provider carries the SDK provider's non-secret Config and Labels, as fresh +// copies, and never the secret Spec fields. +func TestFromSDKProviderMapsConfigAndLabels(t *testing.T) { + sdkConfig := map[string]string{"VERTEX_AI_REGION": "global"} + sdkLabels := map[string]string{"harness.openshell.dev/managed-by": "harness"} + p := &types.Provider{ + Name: "google-vertex-ai", + Type: "google-vertex-ai", + ResourceVersion: 7, + Labels: sdkLabels, + Spec: types.ProviderSpec{ + Config: sdkConfig, + Credentials: map[string]string{"API_KEY": "secret"}, // must NOT cross + CredentialHandles: map[string]types.CredentialHandle{ + "API_KEY": {Driver: "vault", Handle: "h1"}, + }, + CredentialExpiresAt: map[string]time.Time{"API_KEY": {}}, + }, + } + + got := fromSDKProvider(p) + + if got.Name != "google-vertex-ai" || got.Type != "google-vertex-ai" { + t.Fatalf("name/type not mapped: %+v", got) + } + if got.Config["VERTEX_AI_REGION"] != "global" { + t.Errorf("Config not mapped: %v", got.Config) + } + if got.Labels["harness.openshell.dev/managed-by"] != "harness" { + t.Errorf("Labels not mapped: %v", got.Labels) + } + + // Fresh copies: mutating the source must not affect the harness view. + sdkConfig["VERTEX_AI_REGION"] = "us-east1" + sdkLabels["harness.openshell.dev/managed-by"] = "someone-else" + if got.Config["VERTEX_AI_REGION"] != "global" { + t.Errorf("Config aliases the SDK map: %v", got.Config) + } + if got.Labels["harness.openshell.dev/managed-by"] != "harness" { + t.Errorf("Labels aliases the SDK map: %v", got.Labels) + } +} + +// TestFromSDKProviderEmptyMapsAreNil keeps the harness view tidy: absent +// Config/Labels map to nil, not empty non-nil maps. +func TestFromSDKProviderEmptyMapsAreNil(t *testing.T) { + got := fromSDKProvider(&types.Provider{Name: "p", Type: "openai"}) + if got.Config != nil { + t.Errorf("expected nil Config, got %v", got.Config) + } + if got.Labels != nil { + t.Errorf("expected nil Labels, got %v", got.Labels) + } +} diff --git a/internal/openshell/types.go b/internal/openshell/types.go index 076dc5e..c9c4d3f 100644 --- a/internal/openshell/types.go +++ b/internal/openshell/types.go @@ -18,13 +18,20 @@ type Health struct { Version string } -// Provider is the minimal harness view of a registered provider. +// Provider is the harness view of a registered provider. // -// Deliberately minimal; widened only as consumers need more fields -// (least-exposure firewall). +// Deliberately narrow (least-exposure firewall): it carries only the non-secret +// fields the harness diffs, reports, or writes. It has NO Credentials field — +// credentials are write-only and never returned by the SDK's Get, so keeping +// them off this type makes credential-clobber-by-reconcile impossible by +// construction (a reconcile can neither read nor author a secret). It has NO +// ResourceVersion field either: the OCC token is an SDK detail owned entirely by +// sdkclient.UpdateProvider's copy-through, never surfaced to callers. type Provider struct { - Name string - Type string + Name string + Type string + Config map[string]string // non-secret managed configuration + Labels map[string]string // ownership + metadata (see plan.OwnerLabelKey) } // InferenceRoute is the harness view of an inference route read from a gateway. From d2d360329a3b9dc5591afb63f349a0c2f2422d81 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 08:22:36 -0700 Subject: [PATCH 2/9] feat(openshell): credential-preserving provider write + live gate (PR4a S2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add GetProvider/UpdateProvider to the firewall Client. UpdateProvider is the single credential-preserving copy-through (spec §8.5): it re-Gets the server's full provider object, overlays only Config/Labels, and carries creds/handles/ expiry/RV through verbatim — the harness Provider has no credentials field, so this site cannot introduce or drop a secret. No CreateProvider/DeleteProvider (invariant 26: reconcile never creates credentialed providers or deletes). Fake unit tests prove the overlay logic (with in-test caveats: the fake enforces no OCC and strips no creds). The empty-credentials-map = leave-untouched server semantic and the provider:write role are proven by the gated in-package TestLiveProviderUpdatePreservesCredentials, with an optional downstream inference-verify layer for the definitive credentials-still-authenticate proof. --- internal/openshell/client.go | 12 ++ internal/openshell/sdkclient/provider.go | 46 +++++ .../openshell/sdkclient/provider_e2e_test.go | 185 ++++++++++++++++++ internal/openshell/sdkclient/provider_test.go | 96 +++++++++ internal/plan/state_test.go | 16 ++ 5 files changed, 355 insertions(+) create mode 100644 internal/openshell/sdkclient/provider_e2e_test.go diff --git a/internal/openshell/client.go b/internal/openshell/client.go index b991031..d486eff 100644 --- a/internal/openshell/client.go +++ b/internal/openshell/client.go @@ -18,6 +18,18 @@ type Client interface { Health(ctx context.Context) (Health, error) // Providers lists the providers registered in the bound workspace. Providers(ctx context.Context) ([]Provider, error) + // GetProvider reads the named provider in the bound workspace. Returns + // ErrNotFound when no such provider exists (requires the "provider:read" + // role). + GetProvider(ctx context.Context, name string) (Provider, error) + // UpdateProvider writes the desired non-secret Config and Labels of an + // existing provider, preserving its stored credentials. It is + // credential-preserving by construction: the harness Provider carries no + // credentials, and sdkclient overlays only Config/Labels onto the provider's + // current server object (see sdkclient.UpdateProvider). Reconcile issues it + // only on a real non-secret delta. Requires the workspace "admin" role plus + // "provider:write"; a caller lacking either gets ErrPermission. + UpdateProvider(ctx context.Context, p Provider) (Provider, error) // GetInferenceRoute reads the named inference route in the bound workspace. // An empty route targets the gateway default route. Returns ErrNotFound when // no such route exists (requires the workspace "user" role). diff --git a/internal/openshell/sdkclient/provider.go b/internal/openshell/sdkclient/provider.go index 1035d08..af0880b 100644 --- a/internal/openshell/sdkclient/provider.go +++ b/internal/openshell/sdkclient/provider.go @@ -1,6 +1,8 @@ package sdkclient import ( + "context" + v1 "github.com/NVIDIA/OpenShell/sdk/go/openshell/v1" "github.com/stackrox/harness-openshell/internal/openshell" @@ -33,3 +35,47 @@ func copyStringMap(m map[string]string) map[string]string { } return out } + +// GetProvider reads the named provider in the bound workspace. +func (c *client) GetProvider(ctx context.Context, name string) (openshell.Provider, error) { + p, err := c.raw.Providers().Get(ctx, c.workspace, name) + if err != nil { + return openshell.Provider{}, translate(err) + } + return fromSDKProvider(p), nil +} + +// UpdateProvider writes the desired non-secret Config/Labels of an existing +// provider while preserving everything else the gateway holds — this is the +// single credential-preserving-update site (spec §8.5). +// +// It re-Gets the provider's current server object and overlays only Config and +// Labels onto it, then Updates. The credential-bearing spec fields +// (Credentials, CredentialHandles, CredentialExpiresAt, ProfileWorkspace) and +// the ResourceVersion are carried through from that Get verbatim; the harness +// never authors them. Because the harness openshell.Provider has no credentials +// field, this function is the ONLY place a full SDK provider object is +// assembled for a write, and it cannot introduce or drop a secret. +// +// The irreducible caveat lives here: a real gateway's Get never returns raw +// Credentials (they are write-only), so the object sent to Update carries an +// empty Credentials map. Whether the gateway reads that as "leave" or "wipe" is +// a server semantic no unit test can reach — it is proven by the gated +// TestLiveProviderUpdatePreservesCredentials. Reconcile bounds the risk by +// issuing this only on a real Config/Label delta. +func (c *client) UpdateProvider(ctx context.Context, p openshell.Provider) (openshell.Provider, error) { + cur, err := c.raw.Providers().Get(ctx, c.workspace, p.Name) + if err != nil { + return openshell.Provider{}, translate(err) + } + // Overlay only the non-secret managed fields onto the server's own object; + // everything else (creds, handles, expiry, profile workspace, RV) is left + // exactly as Get returned it. + cur.Spec.Config = copyStringMap(p.Config) + cur.Labels = copyStringMap(p.Labels) + updated, err := c.raw.Providers().Update(ctx, c.workspace, cur) + if err != nil { + return openshell.Provider{}, translate(err) + } + return fromSDKProvider(updated), nil +} diff --git a/internal/openshell/sdkclient/provider_e2e_test.go b/internal/openshell/sdkclient/provider_e2e_test.go new file mode 100644 index 0000000..6b385b3 --- /dev/null +++ b/internal/openshell/sdkclient/provider_e2e_test.go @@ -0,0 +1,185 @@ +package sdkclient + +import ( + "context" + "errors" + "os" + "reflect" + "testing" + "time" + + "github.com/stackrox/harness-openshell/internal/openshell" +) + +// TestLiveProviderUpdatePreservesCredentials is the S1-risk gate for PR4a: it +// proves, against a real gateway, that the credential-preserving copy-through in +// UpdateProvider (which sends an EMPTY credentials map, because a real Get never +// returns raw credentials) leaves the provider's stored credentials intact +// rather than wiping them — the one server semantic no unit test can reach — and +// that the mTLS identity actually holds provider:write. +// +// It is skipped unless HARNESS_E2E_GATEWAY names a registered mTLS gateway, and +// again unless HARNESS_E2E_MANAGED_PROVIDER names a real, credentialed managed +// provider in the workspace. It mutates only that provider's Config/Labels and +// restores them on every exit path, so it never leaves drift behind. Optional +// HARNESS_E2E_WORKSPACE overrides the workspace. +// +// HARNESS_E2E_GATEWAY=openshell HARNESS_E2E_MANAGED_PROVIDER=google-vertex-ai \ +// go test ./internal/openshell/sdkclient/ -run LiveProviderUpdatePreservesCredentials -v +func TestLiveProviderUpdatePreservesCredentials(t *testing.T) { + gw := os.Getenv("HARNESS_E2E_GATEWAY") + if gw == "" { + t.Skip("set HARNESS_E2E_GATEWAY to a registered mTLS gateway to run the provider write gate") + } + name := os.Getenv("HARNESS_E2E_MANAGED_PROVIDER") + if name == "" { + t.Skip("set HARNESS_E2E_MANAGED_PROVIDER to a real credentialed managed provider to probe provider:write") + } + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + oc, err := New(ctx, openshell.Target{Gateway: gw, Workspace: os.Getenv("HARNESS_E2E_WORKSPACE")}) + if err != nil { + t.Fatalf("New(%q): %v", gw, err) + } + // Close via t.Cleanup, not defer: t.Cleanup runs in LIFO after the test's + // deferred calls, so a deferred Close would shut the gRPC connection before + // the restoration cleanup registered below could use it. Registered first, + // it runs last. (Same ordering fix as TestLiveInferenceRoleProbe.) + t.Cleanup(func() { + if err := oc.Close(); err != nil { + t.Errorf("closing client: %v", err) + } + }) + // In-package access to the raw SDK client, so the probe can observe the + // credential handles the firewall Provider deliberately hides. + raw := oc.(*client) + + before, err := oc.GetProvider(ctx, name) + if err != nil { + t.Fatalf("GetProvider(%q): %v (is it registered in the workspace?)", name, err) + } + beforeRaw, err := raw.raw.Providers().Get(ctx, raw.workspace, name) + if err != nil { + t.Fatalf("raw Get(%q): %v", name, err) + } + beforeHandles := beforeRaw.Spec.CredentialHandles + beforeExpiry := beforeRaw.Spec.CredentialExpiresAt + if len(beforeHandles) == 0 { + t.Logf("WARNING: provider %q reports no credential handles; the handle-survival "+ + "assertion is inconclusive. Point HARNESS_E2E_INFERENCE_PROVIDER at this "+ + "provider and run the inference probe for the definitive credential-works proof.", name) + } + + // Register restoration BEFORE the write. UpdateProvider persists at the + // gateway before its response returns, so even a failed write may have + // changed state; t.Cleanup runs on every exit path (fresh context, since ctx + // may be spent). Skipped only on a pre-write permission denial: nothing was + // written and the identity lacks the role the restore would need. + permissionDenied := false + t.Cleanup(func() { + if permissionDenied { + return + } + cctx, ccancel := context.WithTimeout(context.Background(), 30*time.Second) + defer ccancel() + if _, err := oc.UpdateProvider(cctx, openshell.Provider{ + Name: name, Config: before.Config, Labels: before.Labels, + }); err != nil { + t.Errorf("restoring provider Config/Labels: %v", err) + } + }) + + // Config-only mutation: carry the current Labels through unchanged (Update + // overlays both fields) and flip one probe key in Config. This is the exact + // production destructive path — an empty-credentials copy-through Update. + probeConfig := copyStringMap(before.Config) + if probeConfig == nil { + probeConfig = map[string]string{} + } + probeConfig["harness.openshell.dev/e2e-probe"] = "1" + _, setErr := oc.UpdateProvider(ctx, openshell.Provider{ + Name: name, Config: probeConfig, Labels: before.Labels, + }) + switch { + case setErr == nil: + t.Logf("WRITE path OK on gateway %q: identity HAS provider:write", gw) + case errors.Is(setErr, openshell.ErrPermission): + permissionDenied = true + t.Fatalf("WRITE path DENIED on gateway %q: identity LACKS provider:write "+ + "(provider reconcile-write will fail until granted): %v", gw, setErr) + default: + t.Fatalf("UpdateProvider returned an unexpected error: %v", setErr) + } + + afterRaw, err := raw.raw.Providers().Get(ctx, raw.workspace, name) + if err != nil { + t.Fatalf("raw Get(%q) after update: %v", name, err) + } + if !reflect.DeepEqual(beforeHandles, afterRaw.Spec.CredentialHandles) { + t.Fatalf("CREDENTIALS WIPED: credential handles changed after a Config-only "+ + "update.\n before: %v\n after: %v\nempty-map-means-WIPE — the "+ + "copy-through Update is UNSAFE on this gateway; disable managed provider "+ + "Update until a config-only RPC or cred-resupply path exists.", + beforeHandles, afterRaw.Spec.CredentialHandles) + } + if !reflect.DeepEqual(beforeExpiry, afterRaw.Spec.CredentialExpiresAt) { + t.Fatalf("CREDENTIALS ROTATED/WIPED: credential expiry changed after a "+ + "Config-only update.\n before: %v\n after: %v", beforeExpiry, afterRaw.Spec.CredentialExpiresAt) + } + if afterRaw.Spec.Config["harness.openshell.dev/e2e-probe"] != "1" { + t.Errorf("probe config key not persisted: %v", afterRaw.Spec.Config) + } + t.Logf("PASS on gateway %q: empty-map credentials Update = LEAVE-UNTOUCHED; "+ + "copy-through provider Update is safe.", gw) + + // Stronger, definitive proof (slice S2 step 4): unchanged handles show the + // credentials survived STRUCTURALLY, but only a real call proves they still + // AUTHENTICATE. When this same provider also backs inference, a verify-mode + // route write (NoVerify:false) makes the gateway call the provider endpoint + // with its stored credentials; success after the config-only update is the + // end-to-end guarantee. It needs a model known-good for the provider + // (HARNESS_E2E_INFERENCE_MODEL) so a failure means "credentials broke", not + // "unknown model" — without one this layer is skipped, not guessed. + if os.Getenv("HARNESS_E2E_INFERENCE_PROVIDER") != name { + return + } + model := os.Getenv("HARNESS_E2E_INFERENCE_MODEL") + if model == "" { + t.Logf("skipping downstream inference-verify proof: set HARNESS_E2E_INFERENCE_MODEL "+ + "to a model valid for %q to enable the definitive credentials-still-authenticate check", name) + return + } + + const verifyRoute = "inference.local" // the only route name a real gateway accepts (see inference_e2e_test.go) + beforeRoute, beforeRouteErr := oc.GetInferenceRoute(ctx, verifyRoute) + if beforeRouteErr != nil && !errors.Is(beforeRouteErr, openshell.ErrNotFound) { + t.Fatalf("pre-verify GetInferenceRoute(%q): %v", verifyRoute, beforeRouteErr) + } + routeExisted := beforeRouteErr == nil + // Registered last → runs first (LIFO), so the route is restored while the + // client is still open, ahead of the provider-config restore and Close above. + t.Cleanup(func() { + cctx, ccancel := context.WithTimeout(context.Background(), 30*time.Second) + defer ccancel() + if routeExisted { + if _, err := oc.SetInferenceRoute(cctx, openshell.InferenceRouteConfig{ + Provider: beforeRoute.Provider, Model: beforeRoute.Model, Route: verifyRoute, + NoVerify: true, TimeoutSecs: beforeRoute.TimeoutSecs, + }); err != nil { + t.Errorf("restoring inference route: %v", err) + } + } else if err := oc.DeleteInferenceRoute(cctx, verifyRoute); err != nil { + t.Errorf("cleanup DeleteInferenceRoute(%q): %v", verifyRoute, err) + } + }) + if _, err := oc.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ + Provider: name, Model: model, Route: verifyRoute, NoVerify: false, + }); err != nil { + t.Fatalf("DOWNSTREAM VERIFY FAILED after the config-only update: provider %q's "+ + "credentials no longer authenticate (verify route %q/%q): %v", name, verifyRoute, model, err) + } + t.Logf("DOWNSTREAM VERIFY OK: provider %q credentials still authenticate after the update — "+ + "copy-through Update is safe end-to-end.", name) +} diff --git a/internal/openshell/sdkclient/provider_test.go b/internal/openshell/sdkclient/provider_test.go index 6e0ec20..8bafbf3 100644 --- a/internal/openshell/sdkclient/provider_test.go +++ b/internal/openshell/sdkclient/provider_test.go @@ -1,10 +1,15 @@ package sdkclient import ( + "context" + "errors" "testing" "time" + 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/openshell" ) // TestFromSDKProviderMapsConfigAndLabels pins the S1 read-widening: the harness @@ -62,3 +67,94 @@ func TestFromSDKProviderEmptyMapsAreNil(t *testing.T) { t.Errorf("expected nil Labels, got %v", got.Labels) } } + +// TestGetProvider covers the read path: fields map through, and a missing +// provider surfaces as openshell.ErrNotFound. +func TestGetProvider(t *testing.T) { + ctx := context.Background() + fc := fake.NewClient() + fc.AddProvider("default", &types.Provider{ + Name: "github", Type: "github", + Spec: types.ProviderSpec{Config: map[string]string{"k": "v"}}, + }) + c := NewFromClient(fc, "default") + + got, err := c.GetProvider(ctx, "github") + if err != nil { + t.Fatalf("GetProvider: %v", err) + } + if got.Name != "github" || got.Config["k"] != "v" { + t.Errorf("unexpected provider: %+v", got) + } + + if _, err := c.GetProvider(ctx, "absent"); !errors.Is(err, openshell.ErrNotFound) { + t.Errorf("GetProvider(absent): want ErrNotFound, got %v", err) + } +} + +// TestUpdateProviderOverlaysConfigPreservesCredentials pins the copy-through: +// UpdateProvider changes only Config/Labels and leaves the stored credentials +// and handles intact. +// +// NOTE ON THE FAKE: this passes because the fake's Get RETURNS the stored +// credentials, so the copy-through carries them back. A real gateway's Get +// returns an EMPTY credentials map (write-only), so this test proves the +// harness overlay logic never drops what Get gave it — NOT the real +// empty-map-means-leave semantic, which only the gated +// TestLiveProviderUpdatePreservesCredentials can prove. The fake also does not +// enforce ResourceVersion OCC, so this asserts neither. +func TestUpdateProviderOverlaysConfigPreservesCredentials(t *testing.T) { + ctx := context.Background() + fc := fake.NewClient() + fc.AddProvider("default", &types.Provider{ + Name: "google-vertex-ai", Type: "google-vertex-ai", + Spec: types.ProviderSpec{ + Config: map[string]string{"VERTEX_AI_REGION": "global"}, + Credentials: map[string]string{"API_KEY": "secret"}, + CredentialHandles: map[string]types.CredentialHandle{ + "API_KEY": {Driver: "vault", Handle: "h1"}, + }, + }, + }) + c := NewFromClient(fc, "default") + + out, err := c.UpdateProvider(ctx, openshell.Provider{ + Name: "google-vertex-ai", + Config: map[string]string{"VERTEX_AI_REGION": "us-east1"}, + Labels: map[string]string{"harness.openshell.dev/managed-by": "harness"}, + }) + if err != nil { + t.Fatalf("UpdateProvider: %v", err) + } + if out.Config["VERTEX_AI_REGION"] != "us-east1" { + t.Errorf("Config not updated in returned view: %v", out.Config) + } + + // Inspect the raw stored object: creds + handles survived the overlay. + stored, err := fc.Providers().Get(ctx, "default", "google-vertex-ai") + if err != nil { + t.Fatalf("raw Get: %v", err) + } + if stored.Spec.Credentials["API_KEY"] != "secret" { + t.Errorf("credentials clobbered by update: %v", stored.Spec.Credentials) + } + if _, ok := stored.Spec.CredentialHandles["API_KEY"]; !ok { + t.Errorf("credential handles clobbered by update: %v", stored.Spec.CredentialHandles) + } + if stored.Spec.Config["VERTEX_AI_REGION"] != "us-east1" { + t.Errorf("stored Config not updated: %v", stored.Spec.Config) + } + if stored.Labels["harness.openshell.dev/managed-by"] != "harness" { + t.Errorf("stored Labels not updated: %v", stored.Labels) + } +} + +// TestUpdateProviderNotFound: updating an absent provider surfaces ErrNotFound +// from the internal Get, never a nil-object write. +func TestUpdateProviderNotFound(t *testing.T) { + ctx := context.Background() + c := NewFromClient(fake.NewClient(), "default") + if _, err := c.UpdateProvider(ctx, openshell.Provider{Name: "absent"}); !errors.Is(err, openshell.ErrNotFound) { + t.Errorf("UpdateProvider(absent): want ErrNotFound, got %v", err) + } +} diff --git a/internal/plan/state_test.go b/internal/plan/state_test.go index 117271d..553a8e3 100644 --- a/internal/plan/state_test.go +++ b/internal/plan/state_test.go @@ -310,6 +310,14 @@ func (r *recordingClient) Providers(ctx context.Context) ([]openshell.Provider, return r.wrapped.Providers(ctx) } +func (r *recordingClient) GetProvider(ctx context.Context, name string) (openshell.Provider, error) { + return r.wrapped.GetProvider(ctx, name) +} + +func (r *recordingClient) UpdateProvider(ctx context.Context, p openshell.Provider) (openshell.Provider, error) { + return r.wrapped.UpdateProvider(ctx, p) +} + func (r *recordingClient) GetInferenceRoute(ctx context.Context, route string) (openshell.InferenceRoute, error) { return r.wrapped.GetInferenceRoute(ctx, route) } @@ -340,6 +348,14 @@ func (e *errorClient) Providers(ctx context.Context) ([]openshell.Provider, erro return nil, e.err } +func (e *errorClient) GetProvider(ctx context.Context, name string) (openshell.Provider, error) { + return openshell.Provider{}, e.err +} + +func (e *errorClient) UpdateProvider(ctx context.Context, p openshell.Provider) (openshell.Provider, error) { + return openshell.Provider{}, e.err +} + func (e *errorClient) GetInferenceRoute(ctx context.Context, route string) (openshell.InferenceRoute, error) { return openshell.InferenceRoute{}, e.err } From 343d0726ebc94731f5563757d9ac38e656e4eff5 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 08:30:40 -0700 Subject: [PATCH 3/9] feat(plan,config): provider diff rule with ownership + config validation (PR4a S3) Add plan.ProviderAction, the single owner of the provider create/adopt/update/ noop rule (invariant 22), shared by harness plan and internal/reconcile. It is conservative about ownership: reconcile never overwrites a provider it does not own. plan.IsOwned + the owner-label constants (plan/ownership.go) are the one vocabulary for managed-by-harness. New ownership semantics (deliberate contract change over pre-ownership behavior): - managed + existing but unowned -> adoption-required until 'adopt: true' (was noop/update); the two existing plan tests now label their current providers owned to keep pinning owned->noop / owned+type-mismatch->update. - referenced + existing -> always noop (never written, ownership irrelevant). - config drift is a subset check (desired keys must match; extra current keys are not drift). config.Provider gains Adopt. config.Resolve now defaults empty management to referenced, rejects invalid management values, and format-checks a non-empty inference route (no allowlist; gateway stays authority). --- internal/config/env.go | 23 ++++++++ internal/config/env_test.go | 69 +++++++++++++++++++++++ internal/config/types.go | 11 +++- internal/plan/ownership.go | 22 ++++++++ internal/plan/plan.go | 107 +++++++++++++++++++++++++++--------- internal/plan/plan_test.go | 100 ++++++++++++++++++++++++++++++++- 6 files changed, 300 insertions(+), 32 deletions(-) create mode 100644 internal/plan/ownership.go diff --git a/internal/config/env.go b/internal/config/env.go index 5bf748f..f8b6f2c 100644 --- a/internal/config/env.go +++ b/internal/config/env.go @@ -2,9 +2,17 @@ package config import ( "fmt" + "regexp" "strings" ) +// routeNamePattern is a format-only guard for inference route names: a +// DNS-label-ish token, optionally dotted (e.g. "inference.local"). It rejects +// empty/whitespace/leading-or-trailing-punctuation garbage at load time; it is +// NOT an allowlist — the gateway stays the authority on which names exist and +// returns ErrInvalidArgument for unknown ones at apply. +var routeNamePattern = regexp.MustCompile(`^[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$`) + // Expand interpolates ${VAR} references in raw using getenv. A referenced but // unset variable is an error (strict — never os.ExpandEnv, which is lenient). // A $$ sequence and a bare $ not followed by { are non-special and left as-is. @@ -98,6 +106,16 @@ func Resolve(h *Harness, getenv func(string) string) (*Harness, error) { np.Name = exp(base+".name", p.Name) np.Type = exp(base+".type", p.Type) np.Management = exp(base+".management", p.Management) + // Empty management defaults to referenced (the safe default: never + // auto-creates, never overwrites). Reject only non-empty invalid values. + switch np.Management { + case "": + np.Management = "referenced" + case "managed", "referenced": + // valid + default: + errs = append(errs, fmt.Sprintf("%s.management: %q is invalid (want \"managed\" or \"referenced\")", base, np.Management)) + } if len(p.Config) > 0 { np.Config = make(map[string]string, len(p.Config)) for k, v := range p.Config { @@ -109,6 +127,11 @@ func Resolve(h *Harness, getenv func(string) string) (*Harness, error) { } s.Inference.Route = exp("spec.inference.route", h.Spec.Inference.Route) + // Format-only check: reject a malformed route name at load time; the gateway + // remains the authority on which names actually exist (no allowlist here). + if s.Inference.Route != "" && !routeNamePattern.MatchString(s.Inference.Route) { + errs = append(errs, fmt.Sprintf("spec.inference.route: %q is malformed (want a DNS-label-like name such as \"inference.local\")", s.Inference.Route)) + } s.Inference.Provider = exp("spec.inference.provider", h.Spec.Inference.Provider) s.Inference.Model = exp("spec.inference.model", h.Spec.Inference.Model) s.Inference.Timeout = exp("spec.inference.timeout", h.Spec.Inference.Timeout) diff --git a/internal/config/env_test.go b/internal/config/env_test.go index b97abaa..af2c142 100644 --- a/internal/config/env_test.go +++ b/internal/config/env_test.go @@ -161,6 +161,75 @@ func TestResolveValidTimeout(t *testing.T) { } } +func TestResolve_RejectsBadManagement(t *testing.T) { + h := &Harness{ + APIVersion: "harness.openshell.dev/v1alpha1", + Kind: "Harness", + Metadata: Metadata{Name: "test"}, + Spec: Spec{Providers: []Provider{ + {Name: "gh", Type: "github", Management: "bogus"}, + }}, + } + + _, err := Resolve(h, func(string) string { return "" }) + if err == nil { + t.Fatal("expected Resolve to reject an invalid management value") + } + if !strings.Contains(err.Error(), "management") || !strings.Contains(err.Error(), "bogus") { + t.Errorf("error should name the field and bad value: %v", err) + } +} + +func TestResolve_DefaultsEmptyManagementToReferenced(t *testing.T) { + h := &Harness{ + APIVersion: "harness.openshell.dev/v1alpha1", + Kind: "Harness", + Metadata: Metadata{Name: "test"}, + Spec: Spec{Providers: []Provider{ + {Name: "gh", Type: "github"}, // no management + }}, + } + + resolved, err := Resolve(h, func(string) string { return "" }) + if err != nil { + t.Fatalf("Resolve failed: %v", err) + } + if got := resolved.Spec.Providers[0].Management; got != "referenced" { + t.Errorf("empty management should default to referenced, got %q", got) + } +} + +func TestResolve_RejectsMalformedRoute(t *testing.T) { + h := &Harness{ + APIVersion: "harness.openshell.dev/v1alpha1", + Kind: "Harness", + Metadata: Metadata{Name: "test"}, + // Provider+model supplied so only the route-format error can fire. + Spec: Spec{Inference: Inference{Provider: "gcp", Model: "claude-opus-4-8", Route: "bad route"}}, + } + + _, err := Resolve(h, func(string) string { return "" }) + if err == nil { + t.Fatal("expected Resolve to reject a malformed route name") + } + if !strings.Contains(err.Error(), "route") { + t.Errorf("error should name the route field: %v", err) + } +} + +func TestResolve_AcceptsDottedRoute(t *testing.T) { + h := &Harness{ + APIVersion: "harness.openshell.dev/v1alpha1", + Kind: "Harness", + Metadata: Metadata{Name: "test"}, + Spec: Spec{Inference: Inference{Provider: "gcp", Model: "claude-opus-4-8", Route: "inference.local"}}, + } + + if _, err := Resolve(h, func(string) string { return "" }); err != nil { + t.Fatalf("Resolve rejected a valid dotted route: %v", err) + } +} + func TestResolveVerifyRoundTrips(t *testing.T) { // verify:false must survive YAML parse + Resolve as an explicit false, not // collapse to the nil→true default, and must not alias the input pointer. diff --git a/internal/config/types.go b/internal/config/types.go index 7b8feaa..e3384b1 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -79,9 +79,14 @@ func (s SecretRef) Describe() string { // Provider represents a desired provider resource. type Provider struct { - Name string `yaml:"name"` - Type string `yaml:"type,omitempty"` - Management string `yaml:"management"` // "managed" or "referenced" + Name string `yaml:"name"` + Type string `yaml:"type,omitempty"` + Management string `yaml:"management"` // "managed" or "referenced"; empty → referenced + // Adopt authorizes reconcile to take over an existing provider that does not + // carry this harness's owner label. Without it, a matching-but-unowned + // provider is reported adoption-required and never overwritten. It is the + // operator's explicit opt-in to manage a pre-existing provider. + Adopt bool `yaml:"adopt,omitempty"` Credentials *SecretRef `yaml:"credentials,omitempty"` Config map[string]string `yaml:"config,omitempty"` } diff --git a/internal/plan/ownership.go b/internal/plan/ownership.go new file mode 100644 index 0000000..733fad1 --- /dev/null +++ b/internal/plan/ownership.go @@ -0,0 +1,22 @@ +package plan + +import "github.com/stackrox/harness-openshell/internal/openshell" + +// Ownership labels mark a provider as reconcile-managed by this harness. They are +// the single vocabulary for "the harness owns this provider" — the diff rule +// (ProviderAction) reads them to decide adoption, and reconcile stamps them on +// the providers it updates. Kept here as the one owner so the plan and the write +// path can never disagree on what "owned" means. +const ( + // OwnerLabelKey is the label key stamped on harness-managed providers. + OwnerLabelKey = "harness.openshell.dev/managed-by" + // OwnerLabelValue is the value OwnerLabelKey must carry to count as owned. + OwnerLabelValue = "harness" +) + +// IsOwned reports whether the provider carries this harness's ownership label. +// It checks both key and value: a foreign managed-by value (another controller) +// is deliberately not ours, so reconcile will not silently take it over. +func IsOwned(p openshell.Provider) bool { + return p.Labels[OwnerLabelKey] == OwnerLabelValue +} diff --git a/internal/plan/plan.go b/internal/plan/plan.go index 174c41f..9f6a77d 100644 --- a/internal/plan/plan.go +++ b/internal/plan/plan.go @@ -119,8 +119,78 @@ func buildTargetGroup(desired *config.Harness, current CurrentState) Group { } } -// buildProvidersGroup returns the PROVIDERS group. -// Matches desired providers by name against current.Providers. +// isManaged reports whether a desired provider is harness-managed. Management is +// "managed" or "referenced"; empty defaults to referenced (never auto-create, +// never overwrite — the safe default, enforced at config.Resolve). Only "managed" +// providers are created or updated by reconcile. +func isManaged(p config.Provider) bool { + return p.Management == "managed" +} + +// ProviderAction is the single owner of the provider create/adopt/update/noop +// rule (invariant 22). Both harness plan (buildProvidersGroup) and +// internal/reconcile call it, so the plan preview and the reconcile write can +// never disagree on what a change is. +// +// cur is the matching current provider by name, or nil when none exists. +// +// Referenced providers are never written: an existing one is a noop whoever owns +// it, and an absent one is adoption-required (it must be created/adopted out of +// band). Managed providers are where ownership matters — reconcile must never +// overwrite one it does not own. A managed provider carrying no harness owner +// label (plan.IsOwned false) is therefore adoption-required until the operator +// opts in with `adopt: true`; only then does managed drift (type/config) or the +// still-missing owner label become an Update. Stamping the owner label on that +// first adopting update is itself the Label delta the credential-preserving +// copy-through then carries (see the spec's credential-preservation note); +// reconcile issues no Update without one of these real deltas. +func ProviderAction(desired config.Provider, cur *openshell.Provider) Action { + managed := isManaged(desired) + + if cur == nil { + if managed { + return ActionCreate + } + return ActionAdoptionRequired // referenced/unknown: never auto-create + } + + // Referenced providers are never written: if it exists we simply reference it, + // regardless of who owns it. + if !managed { + return ActionNoop + } + + // Managed, but not ours and the operator has not authorized taking it over: + // never overwrite a provider another owner (or a human) created. + if !IsOwned(*cur) && !desired.Adopt { + return ActionAdoptionRequired + } + + // We own it, or the operator authorized adoption. A missing owner label here + // means we are adopting (adopt=true), so stamping it is a real Update. + typeMismatch := desired.Type != "" && cur.Type != desired.Type + if typeMismatch || configDrifts(desired.Config, cur.Config) || !IsOwned(*cur) { + return ActionUpdate + } + return ActionNoop +} + +// configDrifts reports whether the current provider config is missing or differs +// from any key the desired config declares. The harness owns only the keys it +// declares: extra keys the gateway or provider carries are not drift, so this is +// a subset check (desired ⊆ current), not equality. +func configDrifts(desired, current map[string]string) bool { + for k, v := range desired { + if current[k] != v { + return true + } + } + return false +} + +// buildProvidersGroup returns the PROVIDERS group. It matches desired providers +// by name against current.Providers and defers every per-provider decision to +// ProviderAction, so plan and reconcile share one rule. func buildProvidersGroup(desired *config.Harness, current CurrentState) Group { group := Group{Section: SectionProviders} @@ -130,35 +200,18 @@ func buildProvidersGroup(desired *config.Harness, current CurrentState) Group { currentByName[p.Name] = p } - for _, desiredProv := range desired.Spec.Providers { - var action Action - var detail string - - currentProv, exists := currentByName[desiredProv.Name] - - if !exists { - // Provider not present in current state. - if desiredProv.Management == "managed" { - action = ActionCreate - } else { - // referenced or unknown management - action = ActionAdoptionRequired - } - } else if desiredProv.Type != "" && currentProv.Type != desiredProv.Type { - // Type mismatch. - action = ActionUpdate - } else { - // Provider exists and type matches (or desired type is empty). - action = ActionNoop - } + for i := range desired.Spec.Providers { + desiredProv := desired.Spec.Providers[i] - // Build detail string: type + management + credentials source if applicable. - detail = buildProviderDetail(&desiredProv) + var cur *openshell.Provider + if c, exists := currentByName[desiredProv.Name]; exists { + cur = &c + } group.Resources = append(group.Resources, Resource{ Name: desiredProv.Name, - Action: action, - Detail: detail, + Action: ProviderAction(desiredProv, cur), + Detail: buildProviderDetail(&desiredProv), }) } diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go index 7d3b563..3aca061 100644 --- a/internal/plan/plan_test.go +++ b/internal/plan/plan_test.go @@ -84,7 +84,10 @@ func TestBuild_ProviderPresentNoop(t *testing.T) { Reachable: true, Health: openshell.Health{Healthy: true, Version: "0.0.110"}, Providers: []openshell.Provider{ - {Name: "github", Type: "github"}, + // Owned by the harness (carries the owner label), so a matching managed + // provider is a noop. An unowned match would be adoption-required — see + // the ProviderAction table. + {Name: "github", Type: "github", Labels: map[string]string{OwnerLabelKey: OwnerLabelValue}}, }, } @@ -200,7 +203,9 @@ func TestBuild_ProviderTypeUpdate(t *testing.T) { Reachable: true, Health: openshell.Health{Healthy: true, Version: "0.0.110"}, Providers: []openshell.Provider{ - {Name: "github", Type: "github-old"}, + // Owned, so a type mismatch is an in-place update. An unowned provider + // with a type mismatch would be adoption-required, not overwritten. + {Name: "github", Type: "github-old", Labels: map[string]string{OwnerLabelKey: OwnerLabelValue}}, }, } @@ -261,6 +266,97 @@ func TestBuild_ProviderDetailIncludesCredentials(t *testing.T) { } } +// TestProviderAction is the single-owner diff-rule table (invariant 22). It +// pins every branch of the create/adopt/update/noop rule, including the +// ownership gate that keeps reconcile from overwriting a provider it does not +// own. +func TestProviderAction(t *testing.T) { + owned := map[string]string{OwnerLabelKey: OwnerLabelValue} + foreign := map[string]string{OwnerLabelKey: "someone-else"} + + tests := []struct { + name string + desired config.Provider + cur *openshell.Provider + want Action + }{ + { + name: "managed absent creates", + desired: config.Provider{Name: "gcp", Type: "google-vertex-ai", Management: "managed"}, + cur: nil, + want: ActionCreate, + }, + { + name: "referenced absent requires adoption", + desired: config.Provider{Name: "ext", Management: "referenced"}, + cur: nil, + want: ActionAdoptionRequired, + }, + { + name: "empty management treated as referenced (absent) requires adoption", + desired: config.Provider{Name: "ext"}, + cur: nil, + want: ActionAdoptionRequired, + }, + { + name: "unowned existing requires adoption (no overwrite)", + desired: config.Provider{Name: "gh", Type: "github", Management: "managed"}, + cur: &openshell.Provider{Name: "gh", Type: "github"}, + want: ActionAdoptionRequired, + }, + { + name: "foreign-owned existing requires adoption", + desired: config.Provider{Name: "gh", Type: "github", Management: "managed"}, + cur: &openshell.Provider{Name: "gh", Type: "github", Labels: foreign}, + want: ActionAdoptionRequired, + }, + { + name: "adopt authorizes taking over an unowned provider (label stamp is an update)", + desired: config.Provider{Name: "gh", Type: "github", Management: "managed", Adopt: true}, + cur: &openshell.Provider{Name: "gh", Type: "github"}, + want: ActionUpdate, + }, + { + name: "owned type mismatch updates", + desired: config.Provider{Name: "gh", Type: "github-new", Management: "managed"}, + cur: &openshell.Provider{Name: "gh", Type: "github-old", Labels: owned}, + want: ActionUpdate, + }, + { + name: "owned config drift updates", + desired: config.Provider{Name: "gcp", Type: "google-vertex-ai", Management: "managed", Config: map[string]string{"VERTEX_AI_REGION": "us-east1"}}, + cur: &openshell.Provider{Name: "gcp", Type: "google-vertex-ai", Labels: owned, Config: map[string]string{"VERTEX_AI_REGION": "global"}}, + want: ActionUpdate, + }, + { + name: "owned matching is noop (extra current config keys are not drift)", + desired: config.Provider{Name: "gcp", Type: "google-vertex-ai", Management: "managed", Config: map[string]string{"VERTEX_AI_REGION": "global"}}, + cur: &openshell.Provider{Name: "gcp", Type: "google-vertex-ai", Labels: owned, Config: map[string]string{"VERTEX_AI_REGION": "global", "EXTRA": "x"}}, + want: ActionNoop, + }, + { + name: "referenced existing and owned is noop (never updated)", + desired: config.Provider{Name: "ext", Type: "custom", Management: "referenced"}, + cur: &openshell.Provider{Name: "ext", Type: "different", Labels: owned}, + want: ActionNoop, + }, + { + name: "referenced existing and unowned is noop (referenced is never written)", + desired: config.Provider{Name: "ext", Type: "custom", Management: "referenced"}, + cur: &openshell.Provider{Name: "ext", Type: "different"}, + want: ActionNoop, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ProviderAction(tt.desired, tt.cur); got != tt.want { + t.Errorf("ProviderAction() = %s, want %s", got, tt.want) + } + }) + } +} + func TestBuild_InferenceGroupWhenConfigured(t *testing.T) { desired := &config.Harness{ Spec: config.Spec{ From 276a781f156e64060c529f0301c9aadc19a37aaa Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 08:38:02 -0700 Subject: [PATCH 4/9] S4: provider reconcile engine (SDK-free) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add internal/reconcile/provider.go mirroring ReconcileInference: routes every decision through plan.ProviderAction (invariant 22) so the read-only plan and this write path can never disagree, and never degrades — any non-NotFound read error or write error is returned. Never creates credentialed providers and never deletes (invariant 26): - Create (managed absent) is reported without writing; providerCreatePlan (S6) does the credentialed create. - AdoptionRequired for an existing-but-unowned provider is reported, no write. - AdoptionRequired for an absent referenced provider is a hard error. Update builds the payload via managedProvider(), which merges current + desired config/labels rather than sending desired alone — sdkclient.UpdateProvider overlays wholesale, so a merge is required to preserve unmanaged keys and honor the diff rule's subset-drift semantics. The owner label is always stamped; on first adoption that stamp is the Label delta that made this an Update. Fake tests via testutil.NewFakeClient (real sdkclient translation) cover referenced-verify, managed-noop, config-drift update (asserting the outbound Provider carries desired config + owner label + preserves unmanaged keys, and is reached only on a real delta), adopt-stamps-label, unowned-adoption-no-write, managed-absent-create-no-write, referenced-absent-errors, and read/write error propagation. TestReconcileMatchesPlanProviderAction locks invariant 22. --- internal/reconcile/provider.go | 120 +++++++++++ internal/reconcile/provider_test.go | 314 ++++++++++++++++++++++++++++ 2 files changed, 434 insertions(+) create mode 100644 internal/reconcile/provider.go create mode 100644 internal/reconcile/provider_test.go diff --git a/internal/reconcile/provider.go b/internal/reconcile/provider.go new file mode 100644 index 0000000..090654d --- /dev/null +++ b/internal/reconcile/provider.go @@ -0,0 +1,120 @@ +package reconcile + +import ( + "context" + "errors" + "fmt" + + "github.com/stackrox/harness-openshell/internal/config" + "github.com/stackrox/harness-openshell/internal/openshell" + "github.com/stackrox/harness-openshell/internal/plan" +) + +// ProviderResult reports what ReconcileProviders decided for one provider and the +// resulting (or current) firewall view of it. +// +// Provider holds: the gateway's response on Update; the current provider on Noop +// and on AdoptionRequired (read at diff time); and a bare {Name, Type} echo on +// Create, which reconcile deliberately does NOT write (invariant 26 — credentialed +// creation is the CLI bridge's job, done upstream by providerCreatePlan). +type ProviderResult struct { + Name string + Action plan.Action + Provider openshell.Provider +} + +// ReconcileProviders drives each desired provider toward the gateway state, +// routing every decision through the shared plan.ProviderAction rule so the +// read-only plan and this write path can never disagree (invariant 22). Like +// ReconcileInference it does not degrade: any non-NotFound read error, or any +// write error, is returned so the caller learns the reconcile did not complete. +// +// It never creates a credentialed provider and never deletes (invariant 26): +// - Create (managed absent) is reported without writing; providerCreatePlan +// (the CLI bridge, S6) does the credentialed create, after which a re-run +// sees the provider present. +// - AdoptionRequired for an existing-but-unowned provider is reported without +// writing (drift the operator must resolve with `adopt: true`). +// - AdoptionRequired for an ABSENT referenced provider is a hard error: a +// referenced provider that does not exist is unusable. +// +// On Update the write is credential-preserving by construction (the firewall +// Provider has no credentials field) and is reached only on a real non-secret +// delta, so the empty-credential copy-through is never sent spuriously. +func ReconcileProviders(ctx context.Context, c openshell.Client, desired []config.Provider) ([]ProviderResult, error) { + results := make([]ProviderResult, 0, len(desired)) + + for _, d := range desired { + cur, err := c.GetProvider(ctx, d.Name) + var curPtr *openshell.Provider + switch { + case err == nil: + curPtr = &cur + case errors.Is(err, openshell.ErrNotFound): + curPtr = nil + default: + return nil, fmt.Errorf("reading provider %q: %w", d.Name, err) + } + + action := plan.ProviderAction(d, curPtr) + switch action { + case plan.ActionNoop: + // cur is guaranteed present here (Noop implies an existing provider); a + // successful Get is itself the referenced-present verification. + results = append(results, ProviderResult{Name: d.Name, Action: action, Provider: cur}) + + case plan.ActionUpdate: + updated, err := c.UpdateProvider(ctx, managedProvider(d, curPtr)) + if err != nil { + return nil, fmt.Errorf("updating provider %q: %w", d.Name, err) + } + results = append(results, ProviderResult{Name: d.Name, Action: action, Provider: updated}) + + case plan.ActionCreate: + // Managed absent: report the intended create; do NOT SDK-create. + results = append(results, ProviderResult{ + Name: d.Name, Action: action, + Provider: openshell.Provider{Name: d.Name, Type: d.Type}, + }) + + case plan.ActionAdoptionRequired: + if curPtr == nil { + // Referenced (or unknown-management) but absent: unusable. + return nil, fmt.Errorf("referenced provider %q does not exist: %w", d.Name, openshell.ErrNotFound) + } + // Exists but unowned and not adopted: report drift, write nothing. + results = append(results, ProviderResult{Name: d.Name, Action: action, Provider: cur}) + + default: + return nil, fmt.Errorf("unexpected provider action %q for %q", action, d.Name) + } + } + + return results, nil +} + +// managedProvider builds the openshell.Provider written on an Update. It merges +// the desired Config and the owner label ONTO the current provider's fields +// rather than replacing them, so an update triggered by one managed key never +// wipes config keys or labels the harness does not manage. This keeps the write +// consistent with the diff rule's subset semantics (plan.configDrifts): the +// harness owns only the keys it declares. The owner label is always stamped — +// on first adoption that stamp is itself the Label delta that made this an +// Update. cur is non-nil here (Update implies an existing provider). +func managedProvider(d config.Provider, cur *openshell.Provider) openshell.Provider { + cfg := map[string]string{} + for k, v := range cur.Config { + cfg[k] = v + } + for k, v := range d.Config { + cfg[k] = v + } + + labels := map[string]string{} + for k, v := range cur.Labels { + labels[k] = v + } + labels[plan.OwnerLabelKey] = plan.OwnerLabelValue + + return openshell.Provider{Name: d.Name, Type: d.Type, Config: cfg, Labels: labels} +} diff --git a/internal/reconcile/provider_test.go b/internal/reconcile/provider_test.go new file mode 100644 index 0000000..8c8bef8 --- /dev/null +++ b/internal/reconcile/provider_test.go @@ -0,0 +1,314 @@ +package reconcile + +import ( + "context" + "errors" + "testing" + + "github.com/NVIDIA/OpenShell/sdk/go/openshell/v1/types" + + "github.com/stackrox/harness-openshell/internal/config" + "github.com/stackrox/harness-openshell/internal/openshell" + "github.com/stackrox/harness-openshell/internal/plan" +) + +// ownerLabels returns the harness owner label set, marking a seeded provider as +// harness-owned so managed reconcile treats it as adoptable-in-place, not drift. +func ownerLabels() map[string]string { + return map[string]string{plan.OwnerLabelKey: plan.OwnerLabelValue} +} + +// TestReconcileProviders_ReferencedVerify: a referenced provider that exists is a +// noop — the successful Get is the verification — and nothing is written. +func TestReconcileProviders_ReferencedVerify(t *testing.T) { + ctx := context.Background() + c, raw := healthyClient(t) + raw.AddProvider("default", &types.Provider{Name: "ext", Type: "custom"}) + rec := &capturingProviderClient{Client: c} + + res, err := ReconcileProviders(ctx, rec, []config.Provider{ + {Name: "ext", Type: "custom", Management: "referenced"}, + }) + if err != nil { + t.Fatalf("ReconcileProviders: %v", err) + } + if len(res) != 1 || res[0].Action != plan.ActionNoop { + t.Fatalf("want one noop result, got %+v", res) + } + if rec.updateCalled { + t.Error("referenced verify must not write") + } +} + +// TestReconcileProviders_ManagedNoop: an owned managed provider whose config +// matches is a noop with no write. +func TestReconcileProviders_ManagedNoop(t *testing.T) { + ctx := context.Background() + c, raw := healthyClient(t) + raw.AddProvider("default", &types.Provider{ + Name: "gcp", Type: "google-vertex-ai", Labels: ownerLabels(), + Spec: types.ProviderSpec{Config: map[string]string{"VERTEX_AI_REGION": "global"}}, + }) + rec := &capturingProviderClient{Client: c} + + res, err := ReconcileProviders(ctx, rec, []config.Provider{ + {Name: "gcp", Type: "google-vertex-ai", Management: "managed", Config: map[string]string{"VERTEX_AI_REGION": "global"}}, + }) + if err != nil { + t.Fatalf("ReconcileProviders: %v", err) + } + if res[0].Action != plan.ActionNoop { + t.Errorf("action = %s, want noop", res[0].Action) + } + if rec.updateCalled { + t.Error("matching managed provider must not write") + } +} + +// TestReconcileProviders_ManagedUpdateConfigDrift: an owned managed provider with +// drifted config is updated, and the Provider handed to UpdateProvider carries +// the desired config merged over the current, plus the owner label — and extra +// unmanaged keys survive. +func TestReconcileProviders_ManagedUpdateConfigDrift(t *testing.T) { + ctx := context.Background() + c, raw := healthyClient(t) + raw.AddProvider("default", &types.Provider{ + Name: "gcp", Type: "google-vertex-ai", Labels: ownerLabels(), + Spec: types.ProviderSpec{ + Config: map[string]string{"VERTEX_AI_REGION": "global", "UNMANAGED": "keep"}, + Credentials: map[string]string{"API_KEY": "secret"}, + }, + }) + rec := &capturingProviderClient{Client: c} + + res, err := ReconcileProviders(ctx, rec, []config.Provider{ + {Name: "gcp", Type: "google-vertex-ai", Management: "managed", Config: map[string]string{"VERTEX_AI_REGION": "us-east1"}}, + }) + if err != nil { + t.Fatalf("ReconcileProviders: %v", err) + } + if res[0].Action != plan.ActionUpdate { + t.Fatalf("action = %s, want update", res[0].Action) + } + if !rec.updateCalled { + t.Fatal("config drift must trigger a write") + } + got := rec.updateArg + if got.Config["VERTEX_AI_REGION"] != "us-east1" { + t.Errorf("desired config not carried to Update: %v", got.Config) + } + if got.Config["UNMANAGED"] != "keep" { + t.Errorf("unmanaged config key wiped by update: %v", got.Config) + } + if !plan.IsOwned(got) { + t.Errorf("owner label not stamped on Update: %v", got.Labels) + } + // The fake's Get returns stored credentials, so the copy-through preserves + // them; this proves the overlay logic, not the real empty-map semantic (that + // is the S2 live gate's job). + stored, err := raw.Providers().Get(ctx, "default", "gcp") + if err != nil { + t.Fatalf("raw Get: %v", err) + } + if stored.Spec.Credentials["API_KEY"] != "secret" { + t.Errorf("credentials clobbered by update: %v", stored.Spec.Credentials) + } +} + +// TestReconcileProviders_AdoptStampsOwnerLabel: with adopt:true an unowned +// provider is taken over — the write stamps the owner label (the label delta that +// made this an update). +func TestReconcileProviders_AdoptStampsOwnerLabel(t *testing.T) { + ctx := context.Background() + c, raw := healthyClient(t) + raw.AddProvider("default", &types.Provider{Name: "gcp", Type: "google-vertex-ai"}) // no owner label + rec := &capturingProviderClient{Client: c} + + res, err := ReconcileProviders(ctx, rec, []config.Provider{ + {Name: "gcp", Type: "google-vertex-ai", Management: "managed", Adopt: true}, + }) + if err != nil { + t.Fatalf("ReconcileProviders: %v", err) + } + if res[0].Action != plan.ActionUpdate { + t.Fatalf("action = %s, want update", res[0].Action) + } + if !plan.IsOwned(rec.updateArg) { + t.Errorf("adopt did not stamp the owner label: %v", rec.updateArg.Labels) + } +} + +// TestReconcileProviders_UnownedAdoptionRequiredNoWrite: an existing unowned +// managed provider without adopt is reported adoption-required and never written. +func TestReconcileProviders_UnownedAdoptionRequiredNoWrite(t *testing.T) { + ctx := context.Background() + c, raw := healthyClient(t) + raw.AddProvider("default", &types.Provider{Name: "gcp", Type: "google-vertex-ai"}) + rec := &capturingProviderClient{Client: c} + + res, err := ReconcileProviders(ctx, rec, []config.Provider{ + {Name: "gcp", Type: "google-vertex-ai", Management: "managed"}, + }) + if err != nil { + t.Fatalf("ReconcileProviders: %v", err) + } + if res[0].Action != plan.ActionAdoptionRequired { + t.Errorf("action = %s, want adoption-required", res[0].Action) + } + if rec.updateCalled { + t.Error("adoption-required must not write an unowned provider") + } +} + +// TestReconcileProviders_ManagedAbsentReturnsCreateNoWrite: a managed provider +// that does not exist is reported create without any SDK write (invariant 26). +func TestReconcileProviders_ManagedAbsentReturnsCreateNoWrite(t *testing.T) { + ctx := context.Background() + c, _ := healthyClient(t) + rec := &capturingProviderClient{Client: c} + + res, err := ReconcileProviders(ctx, rec, []config.Provider{ + {Name: "gcp", Type: "google-vertex-ai", Management: "managed"}, + }) + if err != nil { + t.Fatalf("ReconcileProviders: %v", err) + } + if res[0].Action != plan.ActionCreate { + t.Errorf("action = %s, want create", res[0].Action) + } + if res[0].Provider.Name != "gcp" || res[0].Provider.Type != "google-vertex-ai" { + t.Errorf("create result should echo name/type: %+v", res[0].Provider) + } + if rec.updateCalled { + t.Error("reconcile must not SDK-create (invariant 26)") + } +} + +// TestReconcileProviders_ReferencedAbsentErrors: a referenced provider that does +// not exist is a hard error (unusable). +func TestReconcileProviders_ReferencedAbsentErrors(t *testing.T) { + ctx := context.Background() + c, _ := healthyClient(t) + + _, err := ReconcileProviders(ctx, c, []config.Provider{ + {Name: "ext", Management: "referenced"}, + }) + if !errors.Is(err, openshell.ErrNotFound) { + t.Fatalf("want ErrNotFound for absent referenced provider, got %v", err) + } +} + +// TestReconcileProviders_ReadErrorPropagates: a non-NotFound read error is +// returned, never degraded (a write path must report it did not run). +func TestReconcileProviders_ReadErrorPropagates(t *testing.T) { + ctx := context.Background() + base, _ := healthyClient(t) + for _, want := range []error{openshell.ErrUnavailable, openshell.ErrPermission} { + c := &providerGetErrClient{Client: base, err: want} + _, err := ReconcileProviders(ctx, c, []config.Provider{ + {Name: "gcp", Type: "google-vertex-ai", Management: "managed"}, + }) + if !errors.Is(err, want) { + t.Errorf("expected %v to propagate, got %v", want, err) + } + } +} + +// TestReconcileProviders_WriteErrorPropagates: an Update failure is returned. +func TestReconcileProviders_WriteErrorPropagates(t *testing.T) { + ctx := context.Background() + base, raw := healthyClient(t) + raw.AddProvider("default", &types.Provider{Name: "gcp", Type: "google-vertex-ai", Labels: ownerLabels()}) + c := &providerUpdateErrClient{Client: base, err: openshell.ErrPermission} + + _, err := ReconcileProviders(ctx, c, []config.Provider{ + {Name: "gcp", Type: "google-vertex-ai", Management: "managed", Config: map[string]string{"K": "v"}}, + }) + if !errors.Is(err, openshell.ErrPermission) { + t.Fatalf("expected write ErrPermission to propagate, got %v", err) + } +} + +// TestReconcileMatchesPlanProviderAction locks invariant 22: the read-only plan +// and the reconcile write agree on the action for the same gateway state (both +// route through plan.ProviderAction). +func TestReconcileMatchesPlanProviderAction(t *testing.T) { + ctx := context.Background() + desired := config.Provider{Name: "gcp", Type: "google-vertex-ai", Management: "managed"} + + cases := []struct { + name string + seed *types.Provider // nil = absent + want plan.Action + }{ + {name: "absent -> create", seed: nil, want: plan.ActionCreate}, + {name: "unowned -> adoption-required", seed: &types.Provider{Name: "gcp", Type: "google-vertex-ai"}, want: plan.ActionAdoptionRequired}, + {name: "owned match -> noop", seed: &types.Provider{Name: "gcp", Type: "google-vertex-ai", Labels: ownerLabels()}, want: plan.ActionNoop}, + {name: "owned type drift -> update", seed: &types.Provider{Name: "gcp", Type: "old", Labels: ownerLabels()}, want: plan.ActionUpdate}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c, raw := healthyClient(t) + if tc.seed != nil { + raw.AddProvider("default", tc.seed) + } + + // Plan action from the read path. + var curPtr *openshell.Provider + cur, err := c.GetProvider(ctx, desired.Name) + switch { + case err == nil: + curPtr = &cur + case errors.Is(err, openshell.ErrNotFound): + curPtr = nil + default: + t.Fatalf("GetProvider: %v", err) + } + planAction := plan.ProviderAction(desired, curPtr) + + // Reconcile action from the write path against the same state. + res, err := ReconcileProviders(ctx, c, []config.Provider{desired}) + if err != nil { + t.Fatalf("ReconcileProviders: %v", err) + } + if planAction != tc.want || res[0].Action != tc.want { + t.Errorf("plan=%s reconcile=%s, want %s", planAction, res[0].Action, tc.want) + } + }) + } +} + +// capturingProviderClient records the last UpdateProvider argument while +// delegating to a real fake-backed client, so the outbound Provider can be +// asserted and writes can be detected. +type capturingProviderClient struct { + openshell.Client + updateCalled bool + updateArg openshell.Provider +} + +func (c *capturingProviderClient) UpdateProvider(ctx context.Context, p openshell.Provider) (openshell.Provider, error) { + c.updateCalled = true + c.updateArg = p + return c.Client.UpdateProvider(ctx, p) +} + +// providerGetErrClient forces GetProvider to a chosen error. +type providerGetErrClient struct { + openshell.Client + err error +} + +func (c *providerGetErrClient) GetProvider(context.Context, string) (openshell.Provider, error) { + return openshell.Provider{}, c.err +} + +// providerUpdateErrClient reads normally but forces UpdateProvider to an error. +type providerUpdateErrClient struct { + openshell.Client + err error +} + +func (c *providerUpdateErrClient) UpdateProvider(context.Context, openshell.Provider) (openshell.Provider, error) { + return openshell.Provider{}, c.err +} From f2d0d180f86312e6d4c00bf6cba13f3b45b756a9 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 08:40:37 -0700 Subject: [PATCH 5/9] feat(cmd): apply SDK seam + inference reconcile swap (PR4a S5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thread the openshell.Factory seam through apply and swap the legacy gw.InferenceSet write for the SDK reconcile path (reconcile.ReconcileInference): - NewApplyCmd takes newClient openshell.Factory; main.go wires sdkclient.New. - cmd/target.go: resolveApplyTarget derives the SDK target from the CLI's active gateway (apply's --gateway names a deploy profile, not a registration); empty active gateway is an error. - cmd/desired.go: desiredFromAgent bridges agent.AgentConfig -> config.Provider / config.Inference — the single seam between the agent-config and reconcile worlds, deleted when apply migrates to config.Harness. - upLocal reconciles inference after ensureProviders via the new client; construction/reconcile failure degrades to a warning (non-fatal), mirroring the provider path. - Delete gw.InferenceSet from the Gateway interface, cli.go, the mock, and its test; the route write now lives in the reconcile path. - Add --setup-only: deploy + reconcile providers/inference, skip sandbox create. Behavior change: apply now verifies inference routes by default (the legacy InferenceSet hardcoded --no-verify). The escape hatch (inference.verify: false) lives in the config.Harness path today. --- cmd/apply.go | 7 +- cmd/desired.go | 63 ++++++++++++++ cmd/desired_test.go | 96 ++++++++++++++++++++ cmd/executor.go | 62 +++++++++++++ cmd/executor_inference_test.go | 155 +++++++++++++++++++++++++++++++++ cmd/helpers_test.go | 4 +- cmd/providers.go | 14 ++- cmd/target.go | 22 +++++ cmd/target_test.go | 33 +++++++ internal/gateway/cli.go | 4 - internal/gateway/cli_test.go | 22 ----- internal/gateway/gateway.go | 5 +- main.go | 2 +- 13 files changed, 450 insertions(+), 39 deletions(-) create mode 100644 cmd/desired.go create mode 100644 cmd/desired_test.go create mode 100644 cmd/executor_inference_test.go create mode 100644 cmd/target_test.go diff --git a/cmd/apply.go b/cmd/apply.go index 34487e3..93a076e 100644 --- a/cmd/apply.go +++ b/cmd/apply.go @@ -10,11 +10,12 @@ import ( "github.com/stackrox/harness-openshell/internal/agent" "github.com/stackrox/harness-openshell/internal/gateway" + "github.com/stackrox/harness-openshell/internal/openshell" "github.com/stackrox/harness-openshell/internal/status" "github.com/spf13/cobra" ) -func NewApplyCmd(harnessDir, cli string) *cobra.Command { +func NewApplyCmd(harnessDir, cli string, newClient openshell.Factory) *cobra.Command { var ( file string agentName string @@ -26,6 +27,7 @@ func NewApplyCmd(harnessDir, cli string) *cobra.Command { attach bool providerRefresh bool dryRun bool + setupOnly bool output string ) @@ -138,7 +140,9 @@ then deploy a sandbox. Use --dry-run to validate without deploying, or sandboxName: sandboxName, noTTY: !attach, providerRefresh: providerRefresh, + setupOnly: setupOnly, harness: harness, + newClient: newClient, retrySleep: 5 * time.Second, }) }, @@ -154,6 +158,7 @@ then deploy a sandbox. Use --dry-run to validate without deploying, or cmd.Flags().BoolVar(&attach, "attach", false, "Attach TTY after creation (interactive mode)") cmd.Flags().BoolVar(&providerRefresh, "provider-refresh", false, "Delete and recreate all providers") cmd.Flags().BoolVar(&dryRun, "dry-run", false, "Validate configuration without deploying") + cmd.Flags().BoolVar(&setupOnly, "setup-only", false, "Deploy the gateway and reconcile providers/inference, but do not create a sandbox or run the agent") cmd.Flags().StringVarP(&output, "output", "o", "", "Output format: yaml or json") return cmd diff --git a/cmd/desired.go b/cmd/desired.go new file mode 100644 index 0000000..a1b14c4 --- /dev/null +++ b/cmd/desired.go @@ -0,0 +1,63 @@ +package cmd + +import ( + "github.com/stackrox/harness-openshell/internal/agent" + "github.com/stackrox/harness-openshell/internal/config" +) + +// desiredFromAgent bridges the legacy agent-config world (agent.AgentConfig, +// agent.ProviderRef) into the reconcile world (config.Provider, config.Inference). +// +// It is the single seam between the two config models. Today apply is driven by +// agent.AgentConfig while the SDK reconcile path (plan/reconcile) speaks +// config.Harness; until apply is migrated to author config.Harness directly this +// function is where the two meet. When that migration lands, this function — and +// only this function — is deleted. +// +// It is a classifier, not per-provider credential logic: it maps profile names +// to desired resources and derives the inference route from whichever configured +// provider serves inference. It never materializes secrets and never contacts a +// gateway. getenv is injected (production passes os.Getenv) so the OPENSHELL_MODEL +// default is testable. +func desiredFromAgent(agentCfg *agent.AgentConfig, getenv func(string) string) ([]config.Provider, config.Inference) { + model := getenv("OPENSHELL_MODEL") + if model == "" { + model = "claude-sonnet-4-6" + } + + var providers []config.Provider + var inference config.Inference + for _, p := range agentCfg.Providers { + providers = append(providers, config.Provider{ + Name: p.Profile, + Management: managementFor(p.Profile), + }) + // The inference route points at whichever provider serves inference. + // Verify is left unset so config.Inference.VerifyEnabled defaults to + // true — verify-by-default. There is deliberately no agent-config field + // to opt out yet; the escape hatch (inference.verify: false) lives in the + // config.Harness world consumed by `harness plan`/reconcile. + if inferenceProviders[p.Profile] { + inference = config.Inference{ + Provider: p.Profile, + Model: model, + } + } + } + return providers, inference +} + +// managementFor classifies a provider profile as "managed" (the harness owns its +// lifecycle — credentials and refresh flow through the gateway) or "referenced" +// (the harness only points at an existing registration). This mirrors the legacy +// registration split in registerProviders: ADC/OAuth-refresh providers are +// managed; the rest are referenced. It is provisional — the authoritative +// classification arrives with the provider reconcile (S6). +func managementFor(profile string) string { + switch profile { + case "google-vertex-ai", "google-workspace": + return "managed" + default: + return "referenced" + } +} diff --git a/cmd/desired_test.go b/cmd/desired_test.go new file mode 100644 index 0000000..b2652e4 --- /dev/null +++ b/cmd/desired_test.go @@ -0,0 +1,96 @@ +package cmd + +import ( + "testing" + + "github.com/stackrox/harness-openshell/internal/agent" +) + +func noEnv(string) string { return "" } + +func TestDesiredFromAgent_InferenceFromVertexProvider(t *testing.T) { + agentCfg := &agent.AgentConfig{ + Providers: []agent.ProviderRef{ + {Profile: "github"}, + {Profile: "google-vertex-ai"}, + {Profile: "atlassian"}, + }, + } + + _, inf := desiredFromAgent(agentCfg, noEnv) + + if inf.Provider != "google-vertex-ai" { + t.Errorf("inference provider = %q, want google-vertex-ai", inf.Provider) + } + if inf.Model != "claude-sonnet-4-6" { + t.Errorf("inference model = %q, want default claude-sonnet-4-6", inf.Model) + } + // Verify unset → verify-by-default (the S5 behavior change). + if inf.Verify != nil { + t.Errorf("inference Verify = %v, want nil (verify-by-default)", *inf.Verify) + } + if !inf.VerifyEnabled() { + t.Error("VerifyEnabled() = false, want true for unset Verify") + } +} + +func TestDesiredFromAgent_ModelFromEnv(t *testing.T) { + agentCfg := &agent.AgentConfig{ + Providers: []agent.ProviderRef{{Profile: "google-vertex-ai"}}, + } + getenv := func(k string) string { + if k == "OPENSHELL_MODEL" { + return "claude-opus-4-8" + } + return "" + } + + _, inf := desiredFromAgent(agentCfg, getenv) + + if inf.Model != "claude-opus-4-8" { + t.Errorf("inference model = %q, want claude-opus-4-8 from env", inf.Model) + } +} + +func TestDesiredFromAgent_NoInferenceProvider(t *testing.T) { + agentCfg := &agent.AgentConfig{ + Providers: []agent.ProviderRef{ + {Profile: "github"}, + {Profile: "atlassian"}, + }, + } + + _, inf := desiredFromAgent(agentCfg, noEnv) + + if inf.Provider != "" || inf.Model != "" { + t.Errorf("inference = %+v, want empty (no inference provider configured)", inf) + } +} + +func TestDesiredFromAgent_ProviderClassification(t *testing.T) { + agentCfg := &agent.AgentConfig{ + Providers: []agent.ProviderRef{ + {Profile: "github"}, + {Profile: "google-vertex-ai"}, + {Profile: "google-workspace"}, + {Profile: "atlassian"}, + }, + } + + providers, _ := desiredFromAgent(agentCfg, noEnv) + + if len(providers) != 4 { + t.Fatalf("got %d providers, want 4", len(providers)) + } + want := map[string]string{ + "github": "referenced", + "google-vertex-ai": "managed", + "google-workspace": "managed", + "atlassian": "referenced", + } + for _, p := range providers { + if p.Management != want[p.Name] { + t.Errorf("%s: Management = %q, want %q", p.Name, p.Management, want[p.Name]) + } + } +} diff --git a/cmd/executor.go b/cmd/executor.go index 3e95dfe..a4f67ca 100644 --- a/cmd/executor.go +++ b/cmd/executor.go @@ -1,6 +1,7 @@ package cmd import ( + "context" "fmt" "os" "os/exec" @@ -12,6 +13,8 @@ import ( "github.com/stackrox/harness-openshell/internal/agent" "github.com/stackrox/harness-openshell/internal/gateway" "github.com/stackrox/harness-openshell/internal/k8s" + "github.com/stackrox/harness-openshell/internal/openshell" + "github.com/stackrox/harness-openshell/internal/reconcile" "github.com/stackrox/harness-openshell/internal/status" ) @@ -29,7 +32,9 @@ type upLocalOpts struct { sandboxName string noTTY bool providerRefresh bool + setupOnly bool harness *agent.Harness + newClient openshell.Factory retrySleep time.Duration } @@ -79,6 +84,17 @@ func upLocal(opts upLocalOpts) error { status.Warn("No inference provider configured — the agent will not be able to authenticate. Add google-vertex-ai to providers.") } + reconcileInference(opts, agentCfg) + + // --setup-only stops here: the gateway is deployed and providers/inference + // are reconciled, but no sandbox is created and no agent is run. This leaves + // a clean seam for the provider reconcile (S6) to land alongside inference + // above without disturbing the sandbox path below. + if opts.setupOnly { + status.OK("Setup complete (--setup-only): skipping sandbox creation") + return nil + } + // Clone repo outside the sandbox so git credentials never enter it. var repoUpload *gateway.Upload if agentCfg.Repo != "" { @@ -289,6 +305,52 @@ func initSubmodules(dir string) error { return nil } +// reconcileInference drives the gateway's inference route to match the agent +// config through the SDK reconcile path. It replaces the legacy fire-and-forget +// gw.InferenceSet write that used to live in registerADC. +// +// Behavior change (PR4a S5): the legacy write always passed --no-verify; the +// reconcile path verifies by default (see config.Inference.VerifyEnabled). A +// route write is therefore validated against the provider endpoint. The apply +// path has no opt-out field yet — the escape hatch (inference.verify: false) +// lives in the config.Harness path consumed by `harness plan`/reconcile, and a +// future agent-config field can be threaded through desiredFromAgent if needed. +// +// It is non-fatal by construction, mirroring the provider path: if no inference +// is configured it is a no-op, and any client-construction or reconcile failure +// degrades to a warning rather than aborting apply — provider registration has +// already happened and the sandbox can still be created. +func reconcileInference(opts upLocalOpts, agentCfg *agent.AgentConfig) { + _, desired := desiredFromAgent(agentCfg, os.Getenv) + if desired.Provider == "" { + return // no inference provider in this agent — nothing to reconcile + } + + if opts.newClient == nil { + status.Warn("inference reconcile skipped: no SDK client factory") + return + } + target, err := resolveApplyTarget(opts.gw) + if err != nil { + status.Warnf("inference reconcile skipped: %v", err) + return + } + ctx := context.Background() + client, err := opts.newClient(ctx, target) + if err != nil { + status.Warnf("inference reconcile skipped: %v", err) + return + } + defer client.Close() + + result, err := reconcile.ReconcileInference(ctx, client, desired) + if err != nil { + status.Warnf("inference reconcile: %v", err) + return + } + status.OKf("inference: %s (model %s)", result.Action, desired.Model) +} + var inferenceProviders = map[string]bool{ "google-vertex-ai": true, } diff --git a/cmd/executor_inference_test.go b/cmd/executor_inference_test.go new file mode 100644 index 0000000..3229664 --- /dev/null +++ b/cmd/executor_inference_test.go @@ -0,0 +1,155 @@ +package cmd + +import ( + "context" + "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/openshell" + "github.com/stackrox/harness-openshell/internal/plan" + "github.com/stackrox/harness-openshell/internal/testutil" +) + +// vertexGW returns a mockGW that has all of setupTestAgent's providers already +// registered and an active gateway, so upLocal reaches inference reconcile. +func vertexGW() *mockGW { + return &mockGW{ + providers: map[string]bool{"github": true, "google-vertex-ai": true, "atlassian": true}, + activeGateway: "test-gw", + } +} + +// noCloseClient wraps an openshell.Client with a no-op Close so a test can keep +// reading the shared fake after upLocal's defer closes its client. (The SDK +// fake's Close marks the underlying raw client closed for all wrappers.) +type noCloseClient struct{ openshell.Client } + +func (noCloseClient) Close() error { return nil } + +// keepOpenFactory returns a Factory that hands out c (Close-guarded) on every +// call, so upLocal reconciles against the same fake the test inspects afterward. +func keepOpenFactory(c openshell.Client) openshell.Factory { + return testutil.FakeFactory(noCloseClient{c}) +} + +func TestUpLocal_InferenceReconcile_Create(t *testing.T) { + dir := setupTestAgent(t) + gw := vertexGW() + fakeClient := testutil.NewFake("default", fake.WithHealthResult(&types.HealthResult{Healthy: true})) + + err := upLocal(upLocalOpts{ + harnessDir: dir, + gw: gw, + agentPath: filepath.Join(dir, "agents", "default.yaml"), + noTTY: true, + newClient: keepOpenFactory(fakeClient), + }) + if err != nil { + t.Fatalf("upLocal: %v", err) + } + + route, err := fakeClient.GetInferenceRoute(context.Background(), plan.DefaultInferenceRoute) + if err != nil { + t.Fatalf("GetInferenceRoute: %v", err) + } + if route.Provider != "google-vertex-ai" { + t.Errorf("route provider = %q, want google-vertex-ai", route.Provider) + } + if route.Model != "claude-sonnet-4-6" { + t.Errorf("route model = %q, want claude-sonnet-4-6", route.Model) + } + // The route must still be created — apply always reconciles inference. + if gw.createCalls != 1 { + t.Errorf("createCalls = %d, want 1 (sandbox still created)", gw.createCalls) + } +} + +func TestUpLocal_InferenceReconcile_ModelChange(t *testing.T) { + dir := setupTestAgent(t) + gw := vertexGW() + fakeClient := testutil.NewFake("default", fake.WithHealthResult(&types.HealthResult{Healthy: true})) + + // Seed a route with a stale model so reconcile must update it. + if _, err := fakeClient.SetInferenceRoute(context.Background(), openshell.InferenceRouteConfig{ + Provider: "google-vertex-ai", Model: "claude-old-1", Route: plan.DefaultInferenceRoute, NoVerify: true, + }); err != nil { + t.Fatalf("seed route: %v", err) + } + + err := upLocal(upLocalOpts{ + harnessDir: dir, + gw: gw, + agentPath: filepath.Join(dir, "agents", "default.yaml"), + noTTY: true, + newClient: keepOpenFactory(fakeClient), + }) + if err != nil { + t.Fatalf("upLocal: %v", err) + } + + route, err := fakeClient.GetInferenceRoute(context.Background(), plan.DefaultInferenceRoute) + if err != nil { + t.Fatalf("GetInferenceRoute: %v", err) + } + if route.Model != "claude-sonnet-4-6" { + t.Errorf("route model = %q, want claude-sonnet-4-6 after update", route.Model) + } +} + +func TestUpLocal_InferenceReconcile_ClientFailureDegrades(t *testing.T) { + dir := setupTestAgent(t) + gw := vertexGW() + + errFactory := func(context.Context, openshell.Target) (openshell.Client, error) { + return nil, openshell.ErrUnavailable + } + + err := upLocal(upLocalOpts{ + harnessDir: dir, + gw: gw, + agentPath: filepath.Join(dir, "agents", "default.yaml"), + noTTY: true, + newClient: errFactory, + }) + // A client-construction failure must not abort apply: provider registration + // already happened and the sandbox must still be created. + if err != nil { + t.Fatalf("upLocal should degrade on client failure, got: %v", err) + } + if gw.createCalls != 1 { + t.Errorf("createCalls = %d, want 1 (sandbox created despite inference failure)", gw.createCalls) + } +} + +func TestUpLocal_SetupOnly_SkipsSandbox(t *testing.T) { + dir := setupTestAgent(t) + gw := vertexGW() + fakeClient := testutil.NewFake("default", fake.WithHealthResult(&types.HealthResult{Healthy: true})) + + err := upLocal(upLocalOpts{ + harnessDir: dir, + gw: gw, + agentPath: filepath.Join(dir, "agents", "default.yaml"), + noTTY: true, + setupOnly: true, + newClient: keepOpenFactory(fakeClient), + }) + if err != nil { + t.Fatalf("upLocal: %v", err) + } + + // --setup-only must not create a sandbox... + if gw.createCalls != 0 { + t.Errorf("createCalls = %d, want 0 (--setup-only skips sandbox)", gw.createCalls) + } + // ...but must still reconcile inference. + route, err := fakeClient.GetInferenceRoute(context.Background(), plan.DefaultInferenceRoute) + if err != nil { + t.Fatalf("GetInferenceRoute: %v", err) + } + if route.Provider != "google-vertex-ai" { + t.Errorf("route provider = %q, want google-vertex-ai (inference reconciled under --setup-only)", route.Provider) + } +} diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index ba35721..6f7a3af 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -27,6 +27,7 @@ type mockGW struct { createOpts []gateway.SandboxCreateOpts deletedNames []string gatewayListResult []gateway.GatewayInfo + activeGateway string onGatewayRemove func(string) onSandboxCreate func(gateway.SandboxCreateOpts) error } @@ -58,9 +59,8 @@ func (m *mockGW) SandboxDelete(name string) error { } func (m *mockGW) CLIVersion() string { return "openshell v0.0.59" } func (m *mockGW) CLIPath() string { return "/usr/bin/openshell" } -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) ActiveGateway() string { return m.activeGateway } 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 } diff --git a/cmd/providers.go b/cmd/providers.go index b04eb08..c443022 100644 --- a/cmd/providers.go +++ b/cmd/providers.go @@ -18,8 +18,6 @@ import ( // the gateway. Only providers in the agent YAML are registered. Provider // config values are passed via --config during registration. func registerProviders(harnessDir string, gw gateway.Gateway, force bool, providers []agent.ProviderRef) error { - model := envOr("OPENSHELL_MODEL", "claude-sonnet-4-6") - wanted := make(map[string]*agent.ProviderRef, len(providers)) for i := range providers { wanted[providers[i].Profile] = &providers[i] @@ -67,7 +65,7 @@ func registerProviders(harnessDir string, gw gateway.Gateway, force bool, provid configs = append(configs, "VERTEX_AI_PROJECT_ID="+project) } configs = append(configs, "VERTEX_AI_REGION="+region) - if err := registerADC("google-vertex-ai", "google-vertex-ai", model, gw, configs); err != nil { + if err := registerADC("google-vertex-ai", "google-vertex-ai", gw, configs); err != nil { return err } } @@ -136,7 +134,11 @@ func registerStandard(name, profileType string, gw gateway.Gateway, configs []st return nil } -func registerADC(name, profileType, model string, gw gateway.Gateway, configs []string) error { +// registerADC creates a provider from gcloud Application Default Credentials. +// It no longer sets the inference route: that write moved to the SDK reconcile +// path (reconcileInference in executor.go) as part of PR4a S5, so provider +// registration and inference reconciliation are now separate concerns. +func registerADC(name, profileType string, gw gateway.Gateway, configs []string) error { if gw.ProviderGet(name) == nil { status.Infof("%s: exists", name) return nil @@ -148,10 +150,6 @@ func registerADC(name, profileType, model string, gw gateway.Gateway, configs [] return fmt.Errorf("%s: registration failed: %w", name, err) } status.OKf("%s: registered", name) - if err := gw.InferenceSet(name, model); err != nil { - return fmt.Errorf("inference: %w", err) - } - status.OKf("inference: model %s", model) return nil } diff --git a/cmd/target.go b/cmd/target.go index aa97111..190d27c 100644 --- a/cmd/target.go +++ b/cmd/target.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/spf13/cobra" + "github.com/stackrox/harness-openshell/internal/gateway" "github.com/stackrox/harness-openshell/internal/openshell" ) @@ -24,3 +25,24 @@ func registerTargetFlags(cmd *cobra.Command) (gateway, workspace *string) { fmt.Sprintf("OpenShell workspace (defaults to %q; falls back to $%s).", "default", openshell.EnvWorkspace)) return gateway, workspace } + +// resolveApplyTarget builds the SDK openshell.Target for the apply command from +// the CLI's currently-active gateway registration. +// +// Apply deliberately does NOT register the standard --gateway/--workspace target +// flags: apply's own --gateway flag names a deploy profile (e.g. "openshift"), +// not an openshell registration, so reusing it as the SDK target would connect +// to the wrong thing. Instead the registration name is read from the active +// gateway the CLI already selected. +// +// An empty active gateway is an error, not a silent skip: without a registration +// name the SDK client cannot connect and inference reconcile would quietly +// no-op, hiding a misconfiguration. Workspace is left "" so sdkclient applies +// its "default" default (the single owner of that rule). +func resolveApplyTarget(gw gateway.Gateway) (openshell.Target, error) { + name := gw.ActiveGateway() + if name == "" { + return openshell.Target{}, fmt.Errorf("no active openshell gateway — deploy or select one first") + } + return openshell.Target{Gateway: name, Workspace: ""}, nil +} diff --git a/cmd/target_test.go b/cmd/target_test.go new file mode 100644 index 0000000..5b324cb --- /dev/null +++ b/cmd/target_test.go @@ -0,0 +1,33 @@ +package cmd + +import ( + "strings" + "testing" +) + +func TestResolveApplyTarget_FromActiveGateway(t *testing.T) { + gw := &mockGW{activeGateway: "prod-gw"} + + target, err := resolveApplyTarget(gw) + if err != nil { + t.Fatalf("resolveApplyTarget: %v", err) + } + if target.Gateway != "prod-gw" { + t.Errorf("Gateway = %q, want prod-gw", target.Gateway) + } + if target.Workspace != "" { + t.Errorf("Workspace = %q, want empty (sdkclient defaults it)", target.Workspace) + } +} + +func TestResolveApplyTarget_EmptyActiveGatewayErrors(t *testing.T) { + gw := &mockGW{activeGateway: ""} + + _, err := resolveApplyTarget(gw) + if err == nil { + t.Fatal("expected error for empty active gateway, got nil") + } + if !strings.Contains(err.Error(), "no active openshell gateway") { + t.Errorf("error = %q, want mention of no active gateway", err) + } +} diff --git a/internal/gateway/cli.go b/internal/gateway/cli.go index 6487529..87e8257 100644 --- a/internal/gateway/cli.go +++ b/internal/gateway/cli.go @@ -193,10 +193,6 @@ func (c *CLI) InferenceRemove() error { return c.silent("inference", "remove") } -func (c *CLI) InferenceSet(provider, model string) error { - return c.passthrough("inference", "set", "--provider", provider, "--model", model, "--no-verify") -} - func (c *CLI) SettingsSet(key, value string) error { return c.passthrough("settings", "set", "--global", "--key", key, "--value", value, "--yes") } diff --git a/internal/gateway/cli_test.go b/internal/gateway/cli_test.go index b1416ff..34cc50f 100644 --- a/internal/gateway/cli_test.go +++ b/internal/gateway/cli_test.go @@ -500,28 +500,6 @@ printf '%s\n' "$*" > `+argsFile+` } } -func TestInferenceSet_Args(t *testing.T) { - dir := t.TempDir() - argsFile := filepath.Join(dir, "args") - bin := writeStub(t, `#!/bin/bash -printf '%s\n' "$*" > `+argsFile+` -`) - gw := New(bin) - gw.InferenceSet("google-vertex-ai", "claude-sonnet-4-6") - data, _ := os.ReadFile(argsFile) - args := strings.TrimSpace(string(data)) - for _, want := range []string{ - "inference set", - "--provider google-vertex-ai", - "--model claude-sonnet-4-6", - "--no-verify", - } { - if !strings.Contains(args, want) { - t.Errorf("missing %q in: %s", want, args) - } - } -} - func TestGatewayAdd_Args(t *testing.T) { dir := t.TempDir() argsFile := filepath.Join(dir, "args") diff --git a/internal/gateway/gateway.go b/internal/gateway/gateway.go index 07537bd..86880fe 100644 --- a/internal/gateway/gateway.go +++ b/internal/gateway/gateway.go @@ -22,8 +22,11 @@ type Gateway interface { PolicySet(name, policyFile string) error // Inference + // + // Setting the inference route moved to the SDK reconcile path + // (reconcile.ReconcileInference) in PR4a S5; the legacy InferenceSet write + // was removed. Get/Remove remain for reachability checks and teardown. InferenceGet() error - InferenceSet(provider, model string) error InferenceRemove() error // Gateway management diff --git a/main.go b/main.go index 64ae5b8..ed48fc6 100644 --- a/main.go +++ b/main.go @@ -62,7 +62,7 @@ func main() { root.CompletionOptions.HiddenDefaultCmd = true root.AddCommand( - cmd.NewApplyCmd(harnessDir, cli), + cmd.NewApplyCmd(harnessDir, cli, sdkclient.New), cmd.NewGetCmd(harnessDir, cli), cmd.NewDescribeCmd(harnessDir, cli), cmd.NewDeleteCmd(harnessDir, cli), From 12ad022110eb3e14f972dd698f460f7fd0723a17 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 08:56:55 -0700 Subject: [PATCH 6/9] PR4a S6: wire provider reconcile into apply + providerCreatePlan bootstrap Retire the hard-coded provider machinery in favor of the SDK reconcile path: - providerCreatePlan (cmd/providers.go) is the single owner of "which create strategy" for a not-yet-existing provider, keyed on Credentials.Source (gcloud-adc -> ADC) and type (google-workspace -> OAuth), default reference. This is the CLI-bridge create fork invariant 26 points at; once a provider exists the SDK reconcile owns verify/update/adoption. - registerProviders now bootstrap-creates absent providers via that strategy and performs no destructive delete and no SDK write. Removed the name-keyed switch, the providers_v2_enabled SettingsSet call, and the force-delete block (plus the now-dead deleteCustomProfiles/extractYAMLID, which lint would flag as unused). - reconcileInference is unified into reconcileGateway: one SDK client for the resolved target, then reconcileProvidersStep (credential-preserving update / owner adoption) followed by reconcileInferenceStep. Both steps degrade to a warning; the engines never degrade. - desiredFromAgent marks managed providers Adopt: true so the first reconcile after a CLI-bridge bootstrap adopts them in place (the bridge cannot stamp the SDK owner label), instead of reporting adoption-required forever. Gates: go build/vet/test ./... green; golangci-lint 0 issues; firewall grep (internal/reconcile|plan|config, non-test) empty. --- cmd/desired.go | 39 +++++++-- cmd/executor.go | 81 ++++++++++++------ cmd/executor_provider_test.go | 89 +++++++++++++++++++ cmd/helpers_test.go | 14 ++- cmd/providers.go | 157 ++++++++++++++++------------------ cmd/providers_test.go | 144 ++++++++++++------------------- 6 files changed, 319 insertions(+), 205 deletions(-) create mode 100644 cmd/executor_provider_test.go diff --git a/cmd/desired.go b/cmd/desired.go index a1b14c4..bcb74fe 100644 --- a/cmd/desired.go +++ b/cmd/desired.go @@ -28,10 +28,23 @@ func desiredFromAgent(agentCfg *agent.AgentConfig, getenv func(string) string) ( var providers []config.Provider var inference config.Inference for _, p := range agentCfg.Providers { - providers = append(providers, config.Provider{ - Name: p.Profile, - Management: managementFor(p.Profile), - }) + desired := config.Provider{ + Name: p.Profile, + // For the well-known agent-config profiles the profile name IS the + // gateway provider type; the config.Harness world can distinguish them, + // but here they coincide. + Type: p.Profile, + Management: managementFor(p.Profile), + Credentials: credentialSourceFor(p.Profile), + } + // A managed provider is one the harness bootstraps and owns; authorize + // reconcile to adopt it (stamp the owner label) on first pass, since the + // CLI-bridge create cannot stamp the SDK owner label itself. Referenced + // providers are never written, so Adopt is irrelevant to them. + if desired.Management == "managed" { + desired.Adopt = true + } + providers = append(providers, desired) // The inference route points at whichever provider serves inference. // Verify is left unset so config.Inference.VerifyEnabled defaults to // true — verify-by-default. There is deliberately no agent-config field @@ -51,8 +64,7 @@ func desiredFromAgent(agentCfg *agent.AgentConfig, getenv func(string) string) ( // lifecycle — credentials and refresh flow through the gateway) or "referenced" // (the harness only points at an existing registration). This mirrors the legacy // registration split in registerProviders: ADC/OAuth-refresh providers are -// managed; the rest are referenced. It is provisional — the authoritative -// classification arrives with the provider reconcile (S6). +// managed; the rest are referenced. func managementFor(profile string) string { switch profile { case "google-vertex-ai", "google-workspace": @@ -61,3 +73,18 @@ func managementFor(profile string) string { return "referenced" } } + +// credentialSourceFor records how a managed profile's credentials are acquired, +// without materializing any secret — it is the key providerCreatePlan dispatches +// on to pick the CLI-bridge create strategy. Vertex uses gcloud Application +// Default Credentials; google-workspace's OAuth path is keyed on its type, not a +// SecretRef, so it carries none; referenced providers have no harness-owned +// credentials. +func credentialSourceFor(profile string) *config.SecretRef { + switch profile { + case "google-vertex-ai": + return &config.SecretRef{Source: "gcloud-adc"} + default: + return nil + } +} diff --git a/cmd/executor.go b/cmd/executor.go index a4f67ca..c85d39f 100644 --- a/cmd/executor.go +++ b/cmd/executor.go @@ -11,9 +11,11 @@ import ( "time" "github.com/stackrox/harness-openshell/internal/agent" + "github.com/stackrox/harness-openshell/internal/config" "github.com/stackrox/harness-openshell/internal/gateway" "github.com/stackrox/harness-openshell/internal/k8s" "github.com/stackrox/harness-openshell/internal/openshell" + "github.com/stackrox/harness-openshell/internal/plan" "github.com/stackrox/harness-openshell/internal/reconcile" "github.com/stackrox/harness-openshell/internal/status" ) @@ -84,12 +86,10 @@ func upLocal(opts upLocalOpts) error { status.Warn("No inference provider configured — the agent will not be able to authenticate. Add google-vertex-ai to providers.") } - reconcileInference(opts, agentCfg) + reconcileGateway(opts, agentCfg) // --setup-only stops here: the gateway is deployed and providers/inference - // are reconciled, but no sandbox is created and no agent is run. This leaves - // a clean seam for the provider reconcile (S6) to land alongside inference - // above without disturbing the sandbox path below. + // are reconciled, but no sandbox is created and no agent is run. if opts.setupOnly { status.OK("Setup complete (--setup-only): skipping sandbox creation") return nil @@ -305,44 +305,77 @@ func initSubmodules(dir string) error { return nil } -// reconcileInference drives the gateway's inference route to match the agent -// config through the SDK reconcile path. It replaces the legacy fire-and-forget -// gw.InferenceSet write that used to live in registerADC. +// reconcileGateway drives the gateway's providers and inference route to match +// the agent config through the SDK reconcile path. It is the single apply-spine +// owner of SDK contact: it constructs one client for the resolved target and runs +// the provider reconcile (credential-preserving update / owner adoption for the +// providers registerProviders bootstrapped) followed by the inference reconcile. // -// Behavior change (PR4a S5): the legacy write always passed --no-verify; the -// reconcile path verifies by default (see config.Inference.VerifyEnabled). A -// route write is therefore validated against the provider endpoint. The apply -// path has no opt-out field yet — the escape hatch (inference.verify: false) -// lives in the config.Harness path consumed by `harness plan`/reconcile, and a -// future agent-config field can be threaded through desiredFromAgent if needed. +// Behavior change (PR4a S5): the legacy inference write always passed --no-verify; +// the reconcile path verifies by default (see config.Inference.VerifyEnabled), so +// a route write is validated against the provider endpoint. The apply path has no +// opt-out field yet — the escape hatch (inference.verify: false) lives in the +// config.Harness path consumed by `harness plan`/reconcile. // -// It is non-fatal by construction, mirroring the provider path: if no inference -// is configured it is a no-op, and any client-construction or reconcile failure -// degrades to a warning rather than aborting apply — provider registration has -// already happened and the sandbox can still be created. -func reconcileInference(opts upLocalOpts, agentCfg *agent.AgentConfig) { - _, desired := desiredFromAgent(agentCfg, os.Getenv) - if desired.Provider == "" { - return // no inference provider in this agent — nothing to reconcile +// It is non-fatal by construction: if nothing is configured it is a no-op, and any +// client-construction or reconcile failure degrades to a warning rather than +// aborting apply — provider registration (the CLI-bridge create) has already +// happened and the sandbox can still be created. The engines themselves never +// degrade; the best-effort posture is the caller's. +func reconcileGateway(opts upLocalOpts, agentCfg *agent.AgentConfig) { + providers, inference := desiredFromAgent(agentCfg, os.Getenv) + if len(providers) == 0 && inference.Provider == "" { + return // nothing to reconcile } if opts.newClient == nil { - status.Warn("inference reconcile skipped: no SDK client factory") + status.Warn("gateway reconcile skipped: no SDK client factory") return } target, err := resolveApplyTarget(opts.gw) if err != nil { - status.Warnf("inference reconcile skipped: %v", err) + status.Warnf("gateway reconcile skipped: %v", err) return } ctx := context.Background() client, err := opts.newClient(ctx, target) if err != nil { - status.Warnf("inference reconcile skipped: %v", err) + status.Warnf("gateway reconcile skipped: %v", err) return } defer client.Close() + reconcileProvidersStep(ctx, client, providers) + reconcileInferenceStep(ctx, client, inference) +} + +// reconcileProvidersStep verifies/updates/adopts the desired providers against the +// gateway. Each result is reported; a reconcile error degrades to a warning. +func reconcileProvidersStep(ctx context.Context, client openshell.Client, providers []config.Provider) { + if len(providers) == 0 { + return + } + results, err := reconcile.ReconcileProviders(ctx, client, providers) + if err != nil { + status.Warnf("provider reconcile: %v", err) + return + } + for _, r := range results { + switch r.Action { + case plan.ActionAdoptionRequired: + status.Warnf("provider %s: %s (set adopt: true or re-create managed)", r.Name, r.Action) + default: + status.OKf("provider %s: %s", r.Name, r.Action) + } + } +} + +// reconcileInferenceStep drives the inference route to match desired. A no-provider +// desired is a no-op; a reconcile error degrades to a warning. +func reconcileInferenceStep(ctx context.Context, client openshell.Client, desired config.Inference) { + if desired.Provider == "" { + return // no inference provider in this agent — nothing to reconcile + } result, err := reconcile.ReconcileInference(ctx, client, desired) if err != nil { status.Warnf("inference reconcile: %v", err) diff --git a/cmd/executor_provider_test.go b/cmd/executor_provider_test.go new file mode 100644 index 0000000..e272e5c --- /dev/null +++ b/cmd/executor_provider_test.go @@ -0,0 +1,89 @@ +package cmd + +import ( + "context" + "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/plan" + "github.com/stackrox/harness-openshell/internal/testutil" +) + +// seedGatewayProviders adds setupTestAgent's referenced providers (github, +// atlassian) to the fake so their reconcile is a clean verify-noop, leaving the +// managed vertex provider as the interesting case. vertexLabels is stamped on the +// seeded vertex provider (nil = unowned). +func seedGatewayProviders(raw *fake.Client, vertexLabels map[string]string) { + raw.AddProvider("default", &types.Provider{Name: "github", Type: "github"}) + raw.AddProvider("default", &types.Provider{Name: "atlassian", Type: "atlassian"}) + raw.AddProvider("default", &types.Provider{ + Name: "google-vertex-ai", Type: "google-vertex-ai", Labels: vertexLabels, + }) +} + +// TestUpLocal_ProviderReconcile_AdoptsBootstrapped: apply drives desiredFromAgent, +// which marks the managed vertex provider adopt:true (the harness bootstrapped it +// on the CLI bridge, which cannot stamp the SDK owner label). The provider reconcile +// therefore adopts the unowned provider in place — an Update that stamps the owner +// label — rather than reporting adoption-required forever. +func TestUpLocal_ProviderReconcile_AdoptsBootstrapped(t *testing.T) { + dir := setupTestAgent(t) + gw := vertexGW() + fakeClient, raw := testutil.NewFakeClient("default", fake.WithHealthResult(&types.HealthResult{Healthy: true})) + seedGatewayProviders(raw, nil) // vertex unowned + + err := upLocal(upLocalOpts{ + harnessDir: dir, + gw: gw, + agentPath: filepath.Join(dir, "agents", "default.yaml"), + noTTY: true, + setupOnly: true, + newClient: keepOpenFactory(fakeClient), + }) + if err != nil { + t.Fatalf("upLocal: %v", err) + } + + stored, err := raw.Providers().Get(context.Background(), "default", "google-vertex-ai") + if err != nil { + t.Fatalf("raw Get: %v", err) + } + if stored.Labels[plan.OwnerLabelKey] != plan.OwnerLabelValue { + t.Errorf("vertex not adopted: labels = %v, want owner label stamped", stored.Labels) + } +} + +// TestUpLocal_ProviderReconcile_OwnedNoop: an already-owned managed vertex provider +// with no config drift is a noop — apply completes and creates the sandbox with no +// spurious rewrite. Referenced providers verify cleanly. +func TestUpLocal_ProviderReconcile_OwnedNoop(t *testing.T) { + dir := setupTestAgent(t) + gw := vertexGW() + fakeClient, raw := testutil.NewFakeClient("default", fake.WithHealthResult(&types.HealthResult{Healthy: true})) + seedGatewayProviders(raw, map[string]string{plan.OwnerLabelKey: plan.OwnerLabelValue}) + + err := upLocal(upLocalOpts{ + harnessDir: dir, + gw: gw, + agentPath: filepath.Join(dir, "agents", "default.yaml"), + noTTY: true, + newClient: keepOpenFactory(fakeClient), + }) + if err != nil { + t.Fatalf("upLocal: %v", err) + } + // The provider reconcile must not have stranded apply: the sandbox is created. + if gw.createCalls != 1 { + t.Errorf("createCalls = %d, want 1 (sandbox created after clean reconcile)", gw.createCalls) + } + // The owner label survives an owned-noop pass. + stored, err := raw.Providers().Get(context.Background(), "default", "google-vertex-ai") + if err != nil { + t.Fatalf("raw Get: %v", err) + } + if stored.Labels[plan.OwnerLabelKey] != plan.OwnerLabelValue { + t.Errorf("owner label lost on noop: %v", stored.Labels) + } +} diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index 6f7a3af..5200bf7 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -30,6 +30,15 @@ type mockGW struct { activeGateway string onGatewayRemove func(string) onSandboxCreate func(gateway.SandboxCreateOpts) error + providerCreates []providerCreateCall +} + +// providerCreateCall records one ProviderCreate for assertions on which +// bootstrap strategy fired. +type providerCreateCall struct { + name string + profileType string + opts gateway.ProviderCreateOpts } func (m *mockGW) InferenceGet() error { return m.inferenceErr } @@ -61,7 +70,10 @@ func (m *mockGW) CLIVersion() string func (m *mockGW) CLIPath() string { return "/usr/bin/openshell" } func (m *mockGW) InferenceRemove() error { return nil } func (m *mockGW) ActiveGateway() string { return m.activeGateway } -func (m *mockGW) ProviderCreate(string, string, gateway.ProviderCreateOpts) error { return nil } +func (m *mockGW) ProviderCreate(name, profileType string, opts gateway.ProviderCreateOpts) error { + m.providerCreates = append(m.providerCreates, providerCreateCall{name, profileType, opts}) + 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 } diff --git a/cmd/providers.go b/cmd/providers.go index c443022..0474bcf 100644 --- a/cmd/providers.go +++ b/cmd/providers.go @@ -9,78 +9,96 @@ import ( "strings" "github.com/stackrox/harness-openshell/internal/agent" + "github.com/stackrox/harness-openshell/internal/config" "github.com/stackrox/harness-openshell/internal/gateway" "github.com/stackrox/harness-openshell/internal/status" "gopkg.in/yaml.v3" ) -// registerProviders registers the providers listed in the agent config with -// the gateway. Only providers in the agent YAML are registered. Provider -// config values are passed via --config during registration. -func registerProviders(harnessDir string, gw gateway.Gateway, force bool, providers []agent.ProviderRef) error { - wanted := make(map[string]*agent.ProviderRef, len(providers)) - for i := range providers { - wanted[providers[i].Profile] = &providers[i] - } +// createStrategy names how a not-yet-existing provider is bootstrapped on the CLI +// bridge. Credentialed creation (ADC/OAuth) has to stay on the bridge — the +// firewall Provider type cannot carry a secret (invariant 26) — so this is the one +// place the SDK-vs-bridge fork for *creation* lives. Once a provider exists the SDK +// reconcile (reconcile.ReconcileProviders) owns verify/update/adoption. +type createStrategy int - if force { - sandboxes, err := gw.SandboxList() - if err != nil { - return fmt.Errorf("listing sandboxes: %w", err) - } - if len(sandboxes) > 0 { - return fmt.Errorf("cannot --provider-refresh with running sandboxes — delete them first") - } - for _, p := range providers { - gw.ProviderDelete(p.Profile) - } - deleteCustomProfiles(harnessDir, gw) - status.Info("Deleted existing providers") +const ( + // strategyReference registers a provider from an existing credential already + // present in the environment (github, atlassian). It is the default. + strategyReference createStrategy = iota + // strategyADC creates a provider from gcloud Application Default Credentials + // (google-vertex-ai). + strategyADC + // strategyOAuth creates a provider with a gateway-managed OAuth refresh flow + // (google-workspace). + strategyOAuth +) + +// providerCreatePlan is the single owner of "which create strategy" for a desired +// provider. It keys on how credentials are acquired (Credentials.Source) and the +// provider type — never on a hard-coded per-profile switch. This is the classifier +// invariant 26 points at: ADC/OAuth acquisition stays on the CLI bridge because the +// harness cannot express a credentialed create through the firewall by design. +func providerCreatePlan(p config.Provider) createStrategy { + switch { + case p.Credentials != nil && p.Credentials.Source == "gcloud-adc": + return strategyADC + case p.Type == "google-workspace" || p.Name == "google-workspace": + return strategyOAuth + default: + return strategyReference } +} +// registerProviders bootstrap-creates every desired provider that does not yet +// exist on the gateway, dispatching each through providerCreatePlan. It performs +// no destructive delete and no SDK write: credential-preserving update and owner +// adoption are the SDK reconcile's job (reconcile.ReconcileProviders, run from +// upLocal after this). The register* helpers each no-op when their provider +// already exists, so this is safe to call on every apply. +func registerProviders(harnessDir string, gw gateway.Gateway, desired []config.Provider) error { status.Header("Providers") - if err := gw.SettingsSet("providers_v2_enabled", "true"); err != nil { - return fmt.Errorf("enabling providers v2: %w", err) - } - profilesDir := filepath.Join(harnessDir, "profiles", "providers") if err := gw.ProviderProfileImport(profilesDir); err != nil { status.Warnf("provider profile import: %v", err) } - if _, ok := wanted["github"]; ok { - if err := registerStandard("github", "github", gw, nil); err != nil { - return err - } - } - if _, ok := wanted["google-vertex-ai"]; ok { - home, _ := os.UserHomeDir() - adcPath := envOr("GOOGLE_APPLICATION_CREDENTIALS", - filepath.Join(home, ".config", "gcloud", "application_default_credentials.json")) - project := envOr("ANTHROPIC_VERTEX_PROJECT_ID", readADCProject(adcPath)) - region := envOr("CLOUD_ML_REGION", "global") - var configs []string - if project != "" { - configs = append(configs, "VERTEX_AI_PROJECT_ID="+project) - } - configs = append(configs, "VERTEX_AI_REGION="+region) - if err := registerADC("google-vertex-ai", "google-vertex-ai", gw, configs); err != nil { - return err - } - } - if _, ok := wanted["atlassian"]; ok { - if err := registerStandard("atlassian", "atlassian", gw, nil); err != nil { + for _, p := range desired { + if err := bootstrapProvider(harnessDir, gw, p); err != nil { return err } } - if _, ok := wanted["google-workspace"]; ok { - if err := registerGWS(harnessDir, gw); err != nil { - return err - } + return nil +} + +// bootstrapProvider creates one absent provider via its create strategy. +func bootstrapProvider(harnessDir string, gw gateway.Gateway, p config.Provider) error { + switch providerCreatePlan(p) { + case strategyADC: + return registerADC(p.Name, p.Type, gw, adcConfigs()) + case strategyOAuth: + return registerGWS(harnessDir, gw) + default: + return registerStandard(p.Name, p.Type, gw, nil) } +} - return nil +// adcConfigs resolves the Vertex project/region config passed to the ADC create, +// preserving the legacy resolution order: explicit env overrides first, then the +// ADC file's quota project, then a "global" region default. +func adcConfigs() []string { + home, _ := os.UserHomeDir() + adcPath := envOr("GOOGLE_APPLICATION_CREDENTIALS", + filepath.Join(home, ".config", "gcloud", "application_default_credentials.json")) + project := envOr("ANTHROPIC_VERTEX_PROJECT_ID", readADCProject(adcPath)) + region := envOr("CLOUD_ML_REGION", "global") + var configs []string + if project != "" { + configs = append(configs, "VERTEX_AI_PROJECT_ID="+project) + } + configs = append(configs, "VERTEX_AI_REGION="+region) + return configs } func ensureProviders(harnessDir string, gw gateway.Gateway, agentCfg *agent.AgentConfig, forceRefresh bool, h *agent.Harness) []string { @@ -104,7 +122,8 @@ func ensureProviders(harnessDir string, gw gateway.Gateway, agentCfg *agent.Agen registered, missing := gateway.ValidateProviders(providerNames, gw) if len(missing) > 0 || forceRefresh { - if err := registerProviders(harnessDir, gw, forceRefresh, agentCfg.Providers); err != nil { + desired, _ := desiredFromAgent(agentCfg, os.Getenv) + if err := registerProviders(harnessDir, gw, desired); err != nil { status.Warnf("provider registration: %v", err) } registered, missing = gateway.ValidateProviders(providerNames, gw) @@ -136,7 +155,7 @@ func registerStandard(name, profileType string, gw gateway.Gateway, configs []st // registerADC creates a provider from gcloud Application Default Credentials. // It no longer sets the inference route: that write moved to the SDK reconcile -// path (reconcileInference in executor.go) as part of PR4a S5, so provider +// path (reconcileGateway in executor.go) as part of PR4a S5/S6, so provider // registration and inference reconciliation are now separate concerns. func registerADC(name, profileType string, gw gateway.Gateway, configs []string) error { if gw.ProviderGet(name) == nil { @@ -246,36 +265,6 @@ func gwsProfileScopes(harnessDir string) string { return strings.Join(profile.Credentials[0].Refresh.Scopes, " ") } -func deleteCustomProfiles(harnessDir string, gw gateway.Gateway) { - profilesDir := filepath.Join(harnessDir, "profiles", "providers") - entries, err := os.ReadDir(profilesDir) - if err != nil { - return - } - for _, e := range entries { - if e.IsDir() || !strings.HasSuffix(e.Name(), ".yaml") { - continue - } - id := extractYAMLID(filepath.Join(profilesDir, e.Name())) - if id != "" { - gw.ProviderProfileDelete(id) - } - } -} - -func extractYAMLID(path string) string { - data, err := os.ReadFile(path) - if err != nil { - return "" - } - for _, line := range strings.Split(string(data), "\n") { - if id, ok := strings.CutPrefix(line, "id:"); ok { - return strings.TrimSpace(id) - } - } - return "" -} - func envOr(key, fallback string) string { if v := os.Getenv(key); v != "" { return v diff --git a/cmd/providers_test.go b/cmd/providers_test.go index effbe6c..31a437b 100644 --- a/cmd/providers_test.go +++ b/cmd/providers_test.go @@ -3,10 +3,9 @@ package cmd import ( "os" "path/filepath" - "strings" "testing" - "github.com/stackrox/harness-openshell/internal/agent" + "github.com/stackrox/harness-openshell/internal/config" ) func setupProvidersTest(t *testing.T) string { @@ -16,132 +15,97 @@ func setupProvidersTest(t *testing.T) string { return dir } -func TestRegisterProviders_GitHubWhenTokenSet(t *testing.T) { - dir := setupProvidersTest(t) - t.Setenv("GITHUB_TOKEN", "ghp_test123") - - gw := &mockGW{providers: map[string]bool{}} - - err := registerProviders(dir, gw, false, []agent.ProviderRef{ - {Profile: "github"}, - }) - if err != nil { - t.Fatalf("registerProviders: %v", err) +// TestProviderCreatePlan pins the single owner of "which create strategy": it keys +// on Credentials.Source and provider type, never on a hard-coded profile switch. +func TestProviderCreatePlan(t *testing.T) { + cases := []struct { + name string + p config.Provider + want createStrategy + }{ + {"github references existing", config.Provider{Name: "github", Type: "github"}, strategyReference}, + {"atlassian references existing", config.Provider{Name: "atlassian", Type: "atlassian"}, strategyReference}, + { + "vertex uses ADC via credential source", + config.Provider{Name: "google-vertex-ai", Type: "google-vertex-ai", Credentials: &config.SecretRef{Source: "gcloud-adc"}}, + strategyADC, + }, + {"workspace uses OAuth via type", config.Provider{Name: "google-workspace", Type: "google-workspace"}, strategyOAuth}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := providerCreatePlan(tc.p); got != tc.want { + t.Errorf("providerCreatePlan(%+v) = %v, want %v", tc.p, got, tc.want) + } + }) } } -func TestRegisterProviders_SkipsWhenTokenMissing(t *testing.T) { +func TestRegisterProviders_BootstrapsAbsentReference(t *testing.T) { dir := setupProvidersTest(t) - t.Setenv("GITHUB_TOKEN", "") - gw := &mockGW{providers: map[string]bool{}} - err := registerProviders(dir, gw, false, []agent.ProviderRef{ - {Profile: "github"}, + err := registerProviders(dir, gw, []config.Provider{ + {Name: "github", Type: "github"}, }) if err != nil { t.Fatalf("registerProviders: %v", err) } -} - -func TestRegisterProviders_SkipsExistingProvider(t *testing.T) { - dir := setupProvidersTest(t) - t.Setenv("GITHUB_TOKEN", "ghp_test123") - - gw := &mockGW{providers: map[string]bool{"github": true}} - - err := registerProviders(dir, gw, false, []agent.ProviderRef{ - {Profile: "github"}, - }) - if err != nil { - t.Fatalf("registerProviders: %v", err) + if len(gw.providerCreates) != 1 { + t.Fatalf("providerCreates = %d, want 1", len(gw.providerCreates)) } -} - -func TestRegisterProviders_ForceWithRunningSandboxes(t *testing.T) { - dir := setupProvidersTest(t) - - gw := &mockGWWithSandboxes{ - mockGW: &mockGW{providers: map[string]bool{"github": true}}, - sandboxes: []string{"test-sandbox"}, + c := gw.providerCreates[0] + if c.name != "github" || c.profileType != "github" { + t.Errorf("create = %q/%q, want github/github", c.name, c.profileType) } - - err := registerProviders(dir, gw, true, []agent.ProviderRef{ - {Profile: "github"}, - }) - if err == nil { - t.Fatal("expected error with --force and running sandboxes") - } - if !strings.Contains(err.Error(), "cannot --provider-refresh") { - t.Errorf("error = %q, want 'cannot --provider-refresh'", err) + if !c.opts.FromExisting { + t.Errorf("github should register FromExisting, got %+v", c.opts) } } -func TestRegisterProviders_ForceDeletesAndRecreates(t *testing.T) { +func TestRegisterProviders_VertexUsesADC(t *testing.T) { dir := setupProvidersTest(t) - t.Setenv("GITHUB_TOKEN", "ghp_test123") - gw := &mockGW{providers: map[string]bool{}} - err := registerProviders(dir, gw, true, []agent.ProviderRef{ - {Profile: "github"}, + err := registerProviders(dir, gw, []config.Provider{ + {Name: "google-vertex-ai", Type: "google-vertex-ai", Credentials: &config.SecretRef{Source: "gcloud-adc"}}, }) if err != nil { t.Fatalf("registerProviders: %v", err) } -} - -func TestRegisterProviders_OnlyRegistersRequestedProviders(t *testing.T) { - dir := setupProvidersTest(t) - t.Setenv("GITHUB_TOKEN", "ghp_test123") - t.Setenv("JIRA_API_TOKEN", "jira_test") - - gw := &mockGW{providers: map[string]bool{}} - - err := registerProviders(dir, gw, false, []agent.ProviderRef{ - {Profile: "github"}, - }) - if err != nil { - t.Fatalf("registerProviders: %v", err) + if len(gw.providerCreates) != 1 { + t.Fatalf("providerCreates = %d, want 1", len(gw.providerCreates)) + } + c := gw.providerCreates[0] + if c.name != "google-vertex-ai" || !c.opts.FromADC { + t.Errorf("vertex create = %q FromADC=%v, want google-vertex-ai FromADC=true", c.name, c.opts.FromADC) } } -func TestRegisterProviders_PassesConfigToProvider(t *testing.T) { +func TestRegisterProviders_SkipsExistingProvider(t *testing.T) { dir := setupProvidersTest(t) - t.Setenv("JIRA_API_TOKEN", "jira_test") - t.Setenv("JIRA_URL", "https://test.atlassian.net") - t.Setenv("JIRA_USERNAME", "test@example.com") - - gw := &mockGW{providers: map[string]bool{}} + gw := &mockGW{providers: map[string]bool{"github": true}} - err := registerProviders(dir, gw, false, []agent.ProviderRef{ - {Profile: "atlassian", Env: map[string]string{ - "JIRA_URL": "${JIRA_URL}", - "JIRA_USERNAME": "${JIRA_USERNAME}", - }}, + err := registerProviders(dir, gw, []config.Provider{ + {Name: "github", Type: "github"}, }) if err != nil { t.Fatalf("registerProviders: %v", err) } + if len(gw.providerCreates) != 0 { + t.Errorf("providerCreates = %d, want 0 (already exists)", len(gw.providerCreates)) + } } func TestRegisterProviders_EmptyList(t *testing.T) { dir := setupProvidersTest(t) gw := &mockGW{providers: map[string]bool{}} - err := registerProviders(dir, gw, false, nil) + err := registerProviders(dir, gw, nil) if err != nil { t.Fatalf("registerProviders: %v", err) } + if len(gw.providerCreates) != 0 { + t.Errorf("providerCreates = %d, want 0", len(gw.providerCreates)) + } } - -// mockGWWithSandboxes wraps mockGW to return a non-empty sandbox list. -type mockGWWithSandboxes struct { - *mockGW - sandboxes []string -} - -func (m *mockGWWithSandboxes) SandboxList() ([]string, error) { - return m.sandboxes, nil -} -func (m *mockGWWithSandboxes) PolicySet(string, string) error { return nil } From dd0dcfa26e250a70e4be1deb35675c90a2b51363 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 09:00:55 -0700 Subject: [PATCH 7/9] =?UTF-8?q?PR4a=20S7:=20hard-cutover=20cleanup=20?= =?UTF-8?q?=E2=80=94=20remove=20dead=20legacy=20interface=20surface?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every replacement is wired and green, so these are pure deletions (each keeps the tree compiling): - InferenceRemove: dropped its only call site (the teardown inference-clear block) and the interface method / CLI impl / mock. An orphaned inference route is inert and overwritten on the next apply, so teardown no longer clears it (delegated decision: drop vs re-implement via a firewall DeleteInferenceRoute — dropped). - SettingsSet: its only non-test caller went in S6; removed the interface method, CLI impl, and mock (providers_v2_enabled is fully gone). - --provider-refresh: removed the flag, upLocalOpts.providerRefresh, and ensureProviders' forceRefresh parameter. Its destructive force-delete went in S6; with reconcile running every apply a forced re-reconcile is redundant, and test/test-flow.sh does not reference the flag (delegated decision: drop vs keep as idempotent re-reconcile — dropped, hard cutover). Gates: go build/vet/test ./... green; golangci-lint 0 issues; firewall grep empty; grep proves providers_v2_enabled/InferenceSet/InferenceRemove/SettingsSet/ provider-refresh are gone from production code. --- cmd/apply.go | 3 --- cmd/executor.go | 3 +-- cmd/helpers_test.go | 2 -- cmd/providers.go | 6 +++--- cmd/teardown.go | 6 ------ internal/gateway/cli.go | 8 -------- internal/gateway/gateway.go | 9 ++++----- 7 files changed, 8 insertions(+), 29 deletions(-) diff --git a/cmd/apply.go b/cmd/apply.go index 93a076e..0beacc0 100644 --- a/cmd/apply.go +++ b/cmd/apply.go @@ -25,7 +25,6 @@ func NewApplyCmd(harnessDir, cli string, newClient openshell.Factory) *cobra.Com task string entrypoint string attach bool - providerRefresh bool dryRun bool setupOnly bool output string @@ -139,7 +138,6 @@ then deploy a sandbox. Use --dry-run to validate without deploying, or agentPath: agentPath, sandboxName: sandboxName, noTTY: !attach, - providerRefresh: providerRefresh, setupOnly: setupOnly, harness: harness, newClient: newClient, @@ -156,7 +154,6 @@ then deploy a sandbox. Use --dry-run to validate without deploying, or cmd.Flags().StringVar(&task, "task", "", "Task to pass to the agent (inline text or @filepath)") cmd.Flags().StringVar(&entrypoint, "entrypoint", "", "Override agent entrypoint (claude, opencode, bash)") cmd.Flags().BoolVar(&attach, "attach", false, "Attach TTY after creation (interactive mode)") - cmd.Flags().BoolVar(&providerRefresh, "provider-refresh", false, "Delete and recreate all providers") cmd.Flags().BoolVar(&dryRun, "dry-run", false, "Validate configuration without deploying") cmd.Flags().BoolVar(&setupOnly, "setup-only", false, "Deploy the gateway and reconcile providers/inference, but do not create a sandbox or run the agent") cmd.Flags().StringVarP(&output, "output", "o", "", "Output format: yaml or json") diff --git a/cmd/executor.go b/cmd/executor.go index c85d39f..ae7ca1f 100644 --- a/cmd/executor.go +++ b/cmd/executor.go @@ -33,7 +33,6 @@ type upLocalOpts struct { agentPath string sandboxName string noTTY bool - providerRefresh bool setupOnly bool harness *agent.Harness newClient openshell.Factory @@ -80,7 +79,7 @@ func upLocal(opts upLocalOpts) error { } } - registered := ensureProviders(opts.harnessDir, gw, agentCfg, opts.providerRefresh, opts.harness) + registered := ensureProviders(opts.harnessDir, gw, agentCfg, opts.harness) if needsInference(agentCfg.EffectiveEntrypoint()) && !hasInferenceProvider(agentCfg.Providers) { status.Warn("No inference provider configured — the agent will not be able to authenticate. Add google-vertex-ai to providers.") diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index 5200bf7..0b598f3 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -68,7 +68,6 @@ func (m *mockGW) SandboxDelete(name string) error { } func (m *mockGW) CLIVersion() string { return "openshell v0.0.59" } func (m *mockGW) CLIPath() string { return "/usr/bin/openshell" } -func (m *mockGW) InferenceRemove() error { return nil } func (m *mockGW) ActiveGateway() string { return m.activeGateway } func (m *mockGW) ProviderCreate(name, profileType string, opts gateway.ProviderCreateOpts) error { m.providerCreates = append(m.providerCreates, providerCreateCall{name, profileType, opts}) @@ -77,7 +76,6 @@ func (m *mockGW) ProviderCreate(name, profileType string, opts gateway.ProviderC 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) SandboxStatus() ([]gateway.SandboxInfo, error) { return nil, nil } func (m *mockGW) GatewayAdd(string, string, bool, bool) error { return nil } diff --git a/cmd/providers.go b/cmd/providers.go index 0474bcf..558d6df 100644 --- a/cmd/providers.go +++ b/cmd/providers.go @@ -101,7 +101,7 @@ func adcConfigs() []string { return configs } -func ensureProviders(harnessDir string, gw gateway.Gateway, agentCfg *agent.AgentConfig, forceRefresh bool, h *agent.Harness) []string { +func ensureProviders(harnessDir string, gw gateway.Gateway, agentCfg *agent.AgentConfig, h *agent.Harness) []string { providerNames := agentCfg.ProviderNames() if len(providerNames) == 0 { return nil @@ -121,7 +121,7 @@ func ensureProviders(harnessDir string, gw gateway.Gateway, agentCfg *agent.Agen } registered, missing := gateway.ValidateProviders(providerNames, gw) - if len(missing) > 0 || forceRefresh { + if len(missing) > 0 { desired, _ := desiredFromAgent(agentCfg, os.Getenv) if err := registerProviders(harnessDir, gw, desired); err != nil { status.Warnf("provider registration: %v", err) @@ -174,7 +174,7 @@ func registerADC(name, profileType string, gw gateway.Gateway, configs []string) func registerGWS(harnessDir string, gw gateway.Gateway) error { if gw.ProviderGet("google-workspace") == nil { - status.Info("google-workspace: exists (use --provider-refresh to recreate)") + status.Info("google-workspace: exists") return nil } diff --git a/cmd/teardown.go b/cmd/teardown.go index d0ed5c8..7d77cce 100644 --- a/cmd/teardown.go +++ b/cmd/teardown.go @@ -129,12 +129,6 @@ func teardownProviders(gw gateway.Gateway, activeGW string) error { } } - status.Section("Inference") - if gw.InferenceRemove() == nil { - status.Info("Cleared") - } else { - status.Info("Already cleared") - } fmt.Println() return nil } diff --git a/internal/gateway/cli.go b/internal/gateway/cli.go index 87e8257..37ec112 100644 --- a/internal/gateway/cli.go +++ b/internal/gateway/cli.go @@ -189,14 +189,6 @@ func (c *CLI) ProviderList() ([]string, error) { return parseFirstColumn(out), nil } -func (c *CLI) InferenceRemove() error { - return c.silent("inference", "remove") -} - -func (c *CLI) SettingsSet(key, value string) error { - return c.passthrough("settings", "set", "--global", "--key", key, "--value", value, "--yes") -} - func (c *CLI) SandboxList() ([]string, error) { out, err := c.output("sandbox", "list") if err != nil { diff --git a/internal/gateway/gateway.go b/internal/gateway/gateway.go index 86880fe..aaa6b5c 100644 --- a/internal/gateway/gateway.go +++ b/internal/gateway/gateway.go @@ -23,11 +23,11 @@ type Gateway interface { // Inference // - // Setting the inference route moved to the SDK reconcile path - // (reconcile.ReconcileInference) in PR4a S5; the legacy InferenceSet write - // was removed. Get/Remove remain for reachability checks and teardown. + // The inference route is owned by the SDK reconcile path + // (reconcile.ReconcileInference), which sets it (PR4a S5). Teardown no longer + // clears it — an orphaned route is inert and overwritten on the next apply. + // Only the reachability check remains on the CLI bridge. InferenceGet() error - InferenceRemove() error // Gateway management CLIVersion() string @@ -37,7 +37,6 @@ type Gateway interface { GatewayRemove(name string) error GatewayList() ([]GatewayInfo, error) GatewaySelect(name string) error - SettingsSet(key, value string) error } // ProviderChecker is the subset of Gateway needed to check provider registration. From cf7c970eb0fa1b0209f4dfdaf0f87f53e406c043 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 09:09:10 -0700 Subject: [PATCH 8/9] PR4a whole-spec review: honor type delta on update; audit adoptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes from the finished-feature review over the full main..HEAD diff: - UpdateProvider now overlays Type (non-secret managed field), not just Config/Labels. plan.ProviderAction returns Update on a type delta and both doc contracts promised Update covers type, but the write dropped it — a type mismatch was reported as an update and never converged. Overlaid only when desired Type is non-empty so an unset Type never wipes the stored one. - ReconcileProviders marks an Update that takes over a previously-unowned provider as Adopted; reconcileProvidersStep surfaces it as a distinct warning instead of a silent "update", making adopt-by-name (the managed auto-adopt choice) auditable. --- cmd/executor.go | 6 ++- internal/openshell/client.go | 8 ++-- internal/openshell/sdkclient/provider.go | 12 +++++- internal/openshell/sdkclient/provider_test.go | 41 +++++++++++++++++++ internal/reconcile/provider.go | 10 ++++- 5 files changed, 68 insertions(+), 9 deletions(-) diff --git a/cmd/executor.go b/cmd/executor.go index ae7ca1f..7ab8cfc 100644 --- a/cmd/executor.go +++ b/cmd/executor.go @@ -360,9 +360,11 @@ func reconcileProvidersStep(ctx context.Context, client openshell.Client, provid return } for _, r := range results { - switch r.Action { - case plan.ActionAdoptionRequired: + switch { + case r.Action == plan.ActionAdoptionRequired: status.Warnf("provider %s: %s (set adopt: true or re-create managed)", r.Name, r.Action) + case r.Adopted: + status.Warnf("provider %s: adopted (was unowned — harness now owns it)", r.Name) default: status.OKf("provider %s: %s", r.Name, r.Action) } diff --git a/internal/openshell/client.go b/internal/openshell/client.go index d486eff..327de3d 100644 --- a/internal/openshell/client.go +++ b/internal/openshell/client.go @@ -22,12 +22,12 @@ type Client interface { // ErrNotFound when no such provider exists (requires the "provider:read" // role). GetProvider(ctx context.Context, name string) (Provider, error) - // UpdateProvider writes the desired non-secret Config and Labels of an + // UpdateProvider writes the desired non-secret Config, Labels, and Type of an // existing provider, preserving its stored credentials. It is // credential-preserving by construction: the harness Provider carries no - // credentials, and sdkclient overlays only Config/Labels onto the provider's - // current server object (see sdkclient.UpdateProvider). Reconcile issues it - // only on a real non-secret delta. Requires the workspace "admin" role plus + // credentials, and sdkclient overlays only those non-secret fields onto the + // provider's current server object (see sdkclient.UpdateProvider). Reconcile + // issues it only on a real non-secret delta. Requires the workspace "admin" role plus // "provider:write"; a caller lacking either gets ErrPermission. UpdateProvider(ctx context.Context, p Provider) (Provider, error) // GetInferenceRoute reads the named inference route in the bound workspace. diff --git a/internal/openshell/sdkclient/provider.go b/internal/openshell/sdkclient/provider.go index af0880b..3b82741 100644 --- a/internal/openshell/sdkclient/provider.go +++ b/internal/openshell/sdkclient/provider.go @@ -49,8 +49,9 @@ func (c *client) GetProvider(ctx context.Context, name string) (openshell.Provid // provider while preserving everything else the gateway holds — this is the // single credential-preserving-update site (spec §8.5). // -// It re-Gets the provider's current server object and overlays only Config and -// Labels onto it, then Updates. The credential-bearing spec fields +// It re-Gets the provider's current server object and overlays the non-secret +// managed fields (Config, Labels, and Type) onto it, then Updates. The +// credential-bearing spec fields // (Credentials, CredentialHandles, CredentialExpiresAt, ProfileWorkspace) and // the ResourceVersion are carried through from that Get verbatim; the harness // never authors them. Because the harness openshell.Provider has no credentials @@ -73,6 +74,13 @@ func (c *client) UpdateProvider(ctx context.Context, p openshell.Provider) (open // exactly as Get returned it. cur.Spec.Config = copyStringMap(p.Config) cur.Labels = copyStringMap(p.Labels) + if p.Type != "" { + // Type is a non-secret managed field. ProviderAction returns Update on a + // type delta (plan.ProviderAction), so this is the write that actually + // applies it — overlaid only when declared, so an unset desired Type never + // wipes the stored one. + cur.Type = p.Type + } updated, err := c.raw.Providers().Update(ctx, c.workspace, cur) if err != nil { return openshell.Provider{}, translate(err) diff --git a/internal/openshell/sdkclient/provider_test.go b/internal/openshell/sdkclient/provider_test.go index 8bafbf3..40fa455 100644 --- a/internal/openshell/sdkclient/provider_test.go +++ b/internal/openshell/sdkclient/provider_test.go @@ -149,6 +149,47 @@ func TestUpdateProviderOverlaysConfigPreservesCredentials(t *testing.T) { } } +// TestUpdateProviderWritesType: a declared Type is overlaid onto the stored +// provider, so a type delta reported as Update by plan.ProviderAction actually +// converges. An empty desired Type leaves the stored one untouched. +func TestUpdateProviderWritesType(t *testing.T) { + ctx := context.Background() + fc := fake.NewClient() + fc.AddProvider("default", &types.Provider{ + Name: "gcp", Type: "old-type", + Spec: types.ProviderSpec{Config: map[string]string{"K": "v"}}, + }) + c := NewFromClient(fc, "default") + + // Declared Type is written. + if _, err := c.UpdateProvider(ctx, openshell.Provider{ + Name: "gcp", Type: "new-type", Config: map[string]string{"K": "v"}, + }); err != nil { + t.Fatalf("UpdateProvider: %v", err) + } + stored, err := fc.Providers().Get(ctx, "default", "gcp") + if err != nil { + t.Fatalf("raw Get: %v", err) + } + if stored.Type != "new-type" { + t.Errorf("Type not written: got %q, want new-type", stored.Type) + } + + // Empty desired Type preserves the stored one. + if _, err := c.UpdateProvider(ctx, openshell.Provider{ + Name: "gcp", Config: map[string]string{"K": "v2"}, + }); err != nil { + t.Fatalf("UpdateProvider (empty type): %v", err) + } + stored, err = fc.Providers().Get(ctx, "default", "gcp") + if err != nil { + t.Fatalf("raw Get: %v", err) + } + if stored.Type != "new-type" { + t.Errorf("empty desired Type wiped stored Type: got %q, want new-type", stored.Type) + } +} + // TestUpdateProviderNotFound: updating an absent provider surfaces ErrNotFound // from the internal Get, never a nil-object write. func TestUpdateProviderNotFound(t *testing.T) { diff --git a/internal/reconcile/provider.go b/internal/reconcile/provider.go index 090654d..3f0ef34 100644 --- a/internal/reconcile/provider.go +++ b/internal/reconcile/provider.go @@ -21,6 +21,11 @@ type ProviderResult struct { Name string Action plan.Action Provider openshell.Provider + // Adopted is true when this Update took ownership of a provider that existed + // but carried no harness owner label (adopt: true authorized it). It is a + // takeover, not an ordinary drift-correcting update, so callers surface it + // distinctly for the operator to audit. + Adopted bool } // ReconcileProviders drives each desired provider toward the gateway state, @@ -64,11 +69,14 @@ func ReconcileProviders(ctx context.Context, c openshell.Client, desired []confi results = append(results, ProviderResult{Name: d.Name, Action: action, Provider: cur}) case plan.ActionUpdate: + // An Update on a not-yet-owned provider is an adoption (adopt: true + // stamping the owner label for the first time), not a routine update. + adopted := curPtr != nil && !plan.IsOwned(*curPtr) updated, err := c.UpdateProvider(ctx, managedProvider(d, curPtr)) if err != nil { return nil, fmt.Errorf("updating provider %q: %w", d.Name, err) } - results = append(results, ProviderResult{Name: d.Name, Action: action, Provider: updated}) + results = append(results, ProviderResult{Name: d.Name, Action: action, Provider: updated, Adopted: adopted}) case plan.ActionCreate: // Managed absent: report the intended create; do NOT SDK-create. From 685b5454bd43cdbf7ecb9befe8c0a70c2ef6bc02 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 26 Aug 2026 09:31:21 -0700 Subject: [PATCH 9/9] PR4a review: bound reconcile ctx, warn on absent create, fix SPEC flags Addresses the three CodeRabbit findings on PR #101: - reconcileGateway wrapped its SDK calls in context.Background() with no deadline; verify-by-default makes the inference write contact the provider endpoint synchronously, so a stalled gateway/endpoint hung apply forever while every other failure degrades to a warning. Bound it (60s) so a stall degrades the same way. - reconcileProvidersStep reported ActionCreate via status.OKf, printing "provider X: create" when reconcile deliberately does NOT create and no provider exists (reachable when the CLI-bridge bootstrap didn't create it). Surface it as a warning instead. - SPEC.md documented the removed --provider-refresh flag; replaced with the shipped --setup-only. --- SPEC.md | 4 ++-- cmd/executor.go | 18 +++++++++++++++++- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/SPEC.md b/SPEC.md index 053e9c8..3594098 100644 --- a/SPEC.md +++ b/SPEC.md @@ -80,7 +80,7 @@ Documents are dispatched by `kind` field. No `kind` field = agent (backwards com ## CLI -### `harness apply [-f FILE] [--agent NAME] [--gateway NAME] [--gateway-profile FILE] [--name SANDBOX] [--attach] [--provider-refresh] [--dry-run] [-o yaml|json]` +### `harness apply [-f FILE] [--agent NAME] [--gateway NAME] [--gateway-profile FILE] [--name SANDBOX] [--attach] [--setup-only] [--dry-run] [-o yaml|json]` Primary command. Resolves an agent config, deploys the gateway and providers, creates a sandbox. @@ -99,7 +99,7 @@ Primary command. Resolves an agent config, deploys the gateway and providers, cr Default is non-interactive (headless). Use `--attach` for TTY mode. -`--provider-refresh` deletes and recreates all providers. +`--setup-only` deploys the gateway and reconciles providers/inference, then stops before creating a sandbox or running the agent. ### `harness get [-o table|json|yaml]` diff --git a/cmd/executor.go b/cmd/executor.go index 7ab8cfc..3e84210 100644 --- a/cmd/executor.go +++ b/cmd/executor.go @@ -22,6 +22,11 @@ import ( var Version = "dev" +// reconcileTimeout bounds the SDK reconcile (provider Get/Update + inference +// verify) so a stalled gateway or provider endpoint degrades to a warning +// instead of hanging apply indefinitely. +const reconcileTimeout = 60 * time.Second + var DefaultAgentConfig []byte type upLocalOpts struct { @@ -336,7 +341,12 @@ func reconcileGateway(opts upLocalOpts, agentCfg *agent.AgentConfig) { status.Warnf("gateway reconcile skipped: %v", err) return } - ctx := context.Background() + // Bound the whole reconcile: verify-by-default makes the inference write + // contact the provider endpoint synchronously, so a stalled gateway or + // endpoint would otherwise hang apply with no deadline. Every other failure + // here degrades to a warning; a timeout must too. + ctx, cancel := context.WithTimeout(context.Background(), reconcileTimeout) + defer cancel() client, err := opts.newClient(ctx, target) if err != nil { status.Warnf("gateway reconcile skipped: %v", err) @@ -363,6 +373,12 @@ func reconcileProvidersStep(ctx context.Context, client openshell.Client, provid switch { case r.Action == plan.ActionAdoptionRequired: status.Warnf("provider %s: %s (set adopt: true or re-create managed)", r.Name, r.Action) + case r.Action == plan.ActionCreate: + // Reconcile never SDK-creates: an ActionCreate means the managed + // provider is absent and the CLI-bridge bootstrap did not create it + // (e.g. gws missing, or an ADC create that ensureProviders degraded + // to a warning). Surface it, never report the missing provider as OK. + status.Warnf("provider %s: not present on the gateway (bootstrap did not create it)", r.Name) case r.Adopted: status.Warnf("provider %s: adopted (was unowned — harness now owns it)", r.Name) default: