From eb905eb0cb57b4d5b4fc1b88ff5665e52e2e0809 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Tue, 25 Aug 2026 12:20:36 -0700 Subject: [PATCH 1/8] feat(openshell): inference route wrapper on the SDK firewall (PR4b S1) Add read/write inference-route support to the internal/openshell firewall, the seam later PR4b slices build on (plan diff, reconcile). - internal/openshell: InferenceRoute/InferenceRouteConfig harness types and three Client methods (Get/Set/DeleteInferenceRoute), no workspace arg (the client is bound to one workspace, as with Providers). - sdkclient/inference.go: implementation via c.raw.Inference(), a least-exposure fromSDKInferenceRoute mapper, all errors through translate. - Map gRPC InvalidArgument to a new ErrInvalidArgument sentinel: inference is the first firewall method whose user-supplied required fields can trigger it, so the raw SDK error no longer leaks past the firewall. - Tests (SDK fake): get/set/update-version/delete round trip, named-route isolation, idempotent delete, and closed-client + invalid-argument translation proving the sentinels are wired (not raw SDK errors). - Gated live Admin-role probe (TestLiveInferenceRoleProbe, HARNESS_E2E_GATEWAY) measuring whether the mTLS identity can write routes; skipped in CI. --- internal/openshell/client.go | 12 ++ internal/openshell/errors.go | 4 + internal/openshell/sdkclient/client_test.go | 8 + internal/openshell/sdkclient/errors.go | 2 + internal/openshell/sdkclient/inference.go | 53 ++++++ .../openshell/sdkclient/inference_e2e_test.go | 73 ++++++++ .../openshell/sdkclient/inference_test.go | 161 ++++++++++++++++++ internal/openshell/types.go | 25 +++ internal/plan/state_test.go | 24 +++ 9 files changed, 362 insertions(+) create mode 100644 internal/openshell/sdkclient/inference.go create mode 100644 internal/openshell/sdkclient/inference_e2e_test.go create mode 100644 internal/openshell/sdkclient/inference_test.go diff --git a/internal/openshell/client.go b/internal/openshell/client.go index cb4d432..b991031 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) + // 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). + GetInferenceRoute(ctx context.Context, route string) (InferenceRoute, error) + // SetInferenceRoute creates or updates an inference route in the bound + // workspace (upsert). Returns the resulting route. Requires the workspace + // "admin" role; a caller lacking it gets ErrPermission. + SetInferenceRoute(ctx context.Context, cfg InferenceRouteConfig) (InferenceRoute, error) + // DeleteInferenceRoute removes the named inference route in the bound + // workspace. Idempotent: deleting a missing route is not an error. Requires + // the workspace "admin" role. + DeleteInferenceRoute(ctx context.Context, route string) error // Close releases any resources held by the client. Close() error } diff --git a/internal/openshell/errors.go b/internal/openshell/errors.go index ac0ac20..591fa5a 100644 --- a/internal/openshell/errors.go +++ b/internal/openshell/errors.go @@ -19,6 +19,10 @@ var ( // ErrUnsupported is returned when the gateway does not implement the // requested RPC (gRPC Unimplemented). ErrUnsupported = errors.New("openshell: not supported by gateway") + // ErrInvalidArgument is returned when the caller supplied invalid input + // (e.g. a required field left empty), rejected by the gateway before any + // state change (gRPC InvalidArgument). + ErrInvalidArgument = errors.New("openshell: invalid argument") // ErrConfig is returned when the gateway config cannot be loaded, parsed, or // its auth mode cannot be satisfied. ErrConfig = errors.New("openshell: gateway config error") diff --git a/internal/openshell/sdkclient/client_test.go b/internal/openshell/sdkclient/client_test.go index 588b088..b2b3fae 100644 --- a/internal/openshell/sdkclient/client_test.go +++ b/internal/openshell/sdkclient/client_test.go @@ -214,6 +214,14 @@ func TestTranslate(t *testing.T) { }, expectSent: openshell.ErrPermission, }, + { + name: "invalid argument", + err: &types.StatusError{ + Code: types.ErrorInvalidArgument, + Message: "invalid argument", + }, + expectSent: openshell.ErrInvalidArgument, + }, } for _, tt := range tests { diff --git a/internal/openshell/sdkclient/errors.go b/internal/openshell/sdkclient/errors.go index 428255e..2a811cb 100644 --- a/internal/openshell/sdkclient/errors.go +++ b/internal/openshell/sdkclient/errors.go @@ -25,6 +25,8 @@ func translate(err error) error { return fmt.Errorf("%w: %v", openshell.ErrUnauthenticated, err) case v1.IsPermissionDenied(err): return fmt.Errorf("%w: %v", openshell.ErrPermission, err) + case v1.IsInvalidArgument(err): + return fmt.Errorf("%w: %v", openshell.ErrInvalidArgument, err) default: return err } diff --git a/internal/openshell/sdkclient/inference.go b/internal/openshell/sdkclient/inference.go new file mode 100644 index 0000000..5800382 --- /dev/null +++ b/internal/openshell/sdkclient/inference.go @@ -0,0 +1,53 @@ +package sdkclient + +import ( + "context" + + v1 "github.com/NVIDIA/OpenShell/sdk/go/openshell/v1" + + "github.com/stackrox/harness-openshell/internal/openshell" +) + +// fromSDKInferenceRoute maps the SDK route view to the minimal harness view. +// Deliberately narrow (least-exposure firewall); widen only when a consumer +// genuinely needs more fields, changing this and openshell.InferenceRoute +// together. +func fromSDKInferenceRoute(r *v1.InferenceRoute) openshell.InferenceRoute { + return openshell.InferenceRoute{ + Provider: r.ProviderName, + Model: r.ModelID, + Route: r.RouteName, + TimeoutSecs: r.TimeoutSecs, + Version: r.Version, + } +} + +// GetInferenceRoute reads the named inference route in the bound workspace. +func (c *client) GetInferenceRoute(ctx context.Context, route string) (openshell.InferenceRoute, error) { + r, err := c.raw.Inference().GetRoute(ctx, c.workspace, route) + if err != nil { + return openshell.InferenceRoute{}, translate(err) + } + return fromSDKInferenceRoute(r), nil +} + +// SetInferenceRoute creates or updates (upserts) an inference route in the bound +// workspace. +func (c *client) SetInferenceRoute(ctx context.Context, cfg openshell.InferenceRouteConfig) (openshell.InferenceRoute, error) { + r, err := c.raw.Inference().SetRoute(ctx, c.workspace, &v1.InferenceRouteConfig{ + ProviderName: cfg.Provider, + ModelID: cfg.Model, + RouteName: cfg.Route, + NoVerify: cfg.NoVerify, + TimeoutSecs: cfg.TimeoutSecs, + }) + if err != nil { + return openshell.InferenceRoute{}, translate(err) + } + return fromSDKInferenceRoute(r), nil +} + +// DeleteInferenceRoute removes the named inference route in the bound workspace. +func (c *client) DeleteInferenceRoute(ctx context.Context, route string) error { + return translate(c.raw.Inference().DeleteRoute(ctx, c.workspace, route)) +} diff --git a/internal/openshell/sdkclient/inference_e2e_test.go b/internal/openshell/sdkclient/inference_e2e_test.go new file mode 100644 index 0000000..60905ec --- /dev/null +++ b/internal/openshell/sdkclient/inference_e2e_test.go @@ -0,0 +1,73 @@ +package sdkclient_test + +import ( + "context" + "errors" + "os" + "testing" + "time" + + "github.com/stackrox/harness-openshell/internal/openshell" + "github.com/stackrox/harness-openshell/internal/openshell/sdkclient" +) + +// TestLiveInferenceRoleProbe probes whether the harness mTLS identity holds the +// workspace "admin" role required to write inference routes. This is the S1 risk +// gate for PR4b: SetInferenceRoute/DeleteInferenceRoute require admin, while +// GetInferenceRoute only needs the user role. Slice 3's reconcile-write cannot +// succeed on a real gateway if the identity lacks admin. +// +// It is skipped unless HARNESS_E2E_GATEWAY names a registered mTLS gateway (the +// same gate as the other live checks). Optional HARNESS_E2E_WORKSPACE overrides +// the workspace. The probe uses a SCRATCH route name and cleans it up; it never +// touches the default route. +// +// HARNESS_E2E_GATEWAY=openshell go test ./internal/openshell/sdkclient/ -run LiveInferenceRoleProbe -v +func TestLiveInferenceRoleProbe(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 inference role probe") + } + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + c, err := sdkclient.New(ctx, openshell.Target{ + Gateway: gw, + Workspace: os.Getenv("HARNESS_E2E_WORKSPACE"), + }) + if err != nil { + t.Fatalf("sdkclient.New(%q): %v", gw, err) + } + defer c.Close() + + const scratch = "harness-probe" + + // Read path requires only the user role; ErrNotFound is a success signal + // (the identity can read; the scratch route just doesn't exist yet). + if _, err := c.GetInferenceRoute(ctx, scratch); err != nil && !errors.Is(err, openshell.ErrNotFound) { + t.Fatalf("GetInferenceRoute (user role) failed unexpectedly: %v", err) + } + t.Logf("read path OK on gateway %q (user role confirmed)", gw) + + // Write path requires the admin role. Either outcome is a recordable probe + // result; ErrPermission is exactly the risk we are measuring, not a bug. + _, setErr := c.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ + Provider: "gcp", + Model: "claude-opus-4-8", + Route: scratch, + NoVerify: true, + }) + switch { + case setErr == nil: + t.Logf("WRITE path OK on gateway %q: identity HAS the workspace admin role", gw) + if delErr := c.DeleteInferenceRoute(ctx, scratch); delErr != nil { + t.Errorf("cleanup DeleteInferenceRoute(%q): %v", scratch, delErr) + } + case errors.Is(setErr, openshell.ErrPermission): + t.Fatalf("WRITE path DENIED on gateway %q: identity LACKS the workspace admin role "+ + "(PR4b Slice 3 reconcile-write will fail until the mTLS identity is granted admin): %v", gw, setErr) + default: + t.Fatalf("SetInferenceRoute returned an unexpected error: %v", setErr) + } +} diff --git a/internal/openshell/sdkclient/inference_test.go b/internal/openshell/sdkclient/inference_test.go new file mode 100644 index 0000000..fe8b824 --- /dev/null +++ b/internal/openshell/sdkclient/inference_test.go @@ -0,0 +1,161 @@ +package sdkclient + +import ( + "context" + "errors" + "testing" + + fake "github.com/NVIDIA/OpenShell/sdk/go/openshell/v1/fake" + + "github.com/stackrox/harness-openshell/internal/openshell" +) + +// TestInferenceRoundTrip exercises the full get/set/update/delete lifecycle +// against the SDK fake through the real sdkclient mapping and translation. It +// pins the server-assigned version semantics (1 on create, monotonic on update) +// and that a missing route surfaces the harness ErrNotFound sentinel rather than +// a raw SDK error. +func TestInferenceRoundTrip(t *testing.T) { + ctx := context.Background() + c := NewFromClient(fake.NewClient(), "default") + + // Get on an empty gateway -> ErrNotFound (translated sentinel). + if _, err := c.GetInferenceRoute(ctx, ""); !errors.Is(err, openshell.ErrNotFound) { + t.Fatalf("GetInferenceRoute on empty gateway: want ErrNotFound, got %v", err) + } + + // Set creates the route at version 1. + created, err := c.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ + Provider: "gcp", + Model: "claude-opus-4-8", + NoVerify: true, + TimeoutSecs: 90, + }) + if err != nil { + t.Fatalf("SetInferenceRoute create: %v", err) + } + if created.Version != 1 { + t.Errorf("created version: want 1, got %d", created.Version) + } + if created.Provider != "gcp" || created.Model != "claude-opus-4-8" { + t.Errorf("created route mismatch: %+v", created) + } + if created.TimeoutSecs != 90 { + t.Errorf("created TimeoutSecs: want 90, got %d", created.TimeoutSecs) + } + + // Get returns the created route. + got, err := c.GetInferenceRoute(ctx, "") + if err != nil { + t.Fatalf("GetInferenceRoute after create: %v", err) + } + if got.Provider != created.Provider || got.Model != created.Model || got.Version != created.Version { + t.Errorf("round-trip mismatch: set %+v, got %+v", created, got) + } + + // Set with a changed model upserts and bumps the version to 2. + updated, err := c.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ + Provider: "gcp", + Model: "claude-sonnet-5", + NoVerify: true, + }) + if err != nil { + t.Fatalf("SetInferenceRoute update: %v", err) + } + if updated.Version != 2 { + t.Errorf("updated version: want 2, got %d", updated.Version) + } + if updated.Model != "claude-sonnet-5" { + t.Errorf("updated model: want claude-sonnet-5, got %q", updated.Model) + } + + // Delete removes the route; a subsequent get is ErrNotFound again. + if err := c.DeleteInferenceRoute(ctx, ""); err != nil { + t.Fatalf("DeleteInferenceRoute: %v", err) + } + if _, err := c.GetInferenceRoute(ctx, ""); !errors.Is(err, openshell.ErrNotFound) { + t.Fatalf("GetInferenceRoute after delete: want ErrNotFound, got %v", err) + } +} + +// TestInferenceDeleteIdempotent pins the firewall contract that deleting a +// missing route is not an error (mirrors the SDK's idempotent DeleteRoute). +func TestInferenceDeleteIdempotent(t *testing.T) { + ctx := context.Background() + c := NewFromClient(fake.NewClient(), "default") + + if err := c.DeleteInferenceRoute(ctx, ""); err != nil { + t.Fatalf("DeleteInferenceRoute on empty gateway: want nil, got %v", err) + } +} + +// TestInferenceErrorsTranslated proves all three methods route SDK errors +// through translate to harness sentinels (not raw SDK errors). It uses the +// closed-client trick — the same convention as TestHealthErrorTranslated / +// TestProvidersErrorTranslated — under which the fake returns ErrorUnavailable. +// Without this, a regression dropping translate() on the Set/Delete success +// wrappers would go unnoticed (their success paths never error). +func TestInferenceErrorsTranslated(t *testing.T) { + ctx := context.Background() + c := NewFromClient(fake.NewClient(), "default") + if err := c.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + if _, err := c.GetInferenceRoute(ctx, ""); !errors.Is(err, openshell.ErrUnavailable) { + t.Errorf("GetInferenceRoute on closed client: want ErrUnavailable, got %v", err) + } + if _, err := c.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ + Provider: "gcp", Model: "claude-opus-4-8", + }); !errors.Is(err, openshell.ErrUnavailable) { + t.Errorf("SetInferenceRoute on closed client: want ErrUnavailable, got %v", err) + } + if err := c.DeleteInferenceRoute(ctx, ""); !errors.Is(err, openshell.ErrUnavailable) { + t.Errorf("DeleteInferenceRoute on closed client: want ErrUnavailable, got %v", err) + } +} + +// TestInferenceInvalidArgumentTranslated proves user-supplied invalid input +// (a required field left empty) surfaces the harness ErrInvalidArgument sentinel +// rather than a raw SDK *StatusError. Inference is the first firewall caller +// whose required-field input can trigger this. +func TestInferenceInvalidArgumentTranslated(t *testing.T) { + ctx := context.Background() + c := NewFromClient(fake.NewClient(), "default") + + // Empty model (required) -> InvalidArgument from the SDK. + if _, err := c.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ + Provider: "gcp", + }); !errors.Is(err, openshell.ErrInvalidArgument) { + t.Errorf("SetInferenceRoute with empty model: want ErrInvalidArgument, got %v", err) + } +} + +// TestInferenceNamedRoute proves a non-default route name round-trips +// independently of the default route. +func TestInferenceNamedRoute(t *testing.T) { + ctx := context.Background() + c := NewFromClient(fake.NewClient(), "default") + + if _, err := c.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ + Provider: "gcp", + Model: "claude-opus-4-8", + Route: "scratch", + NoVerify: true, + }); err != nil { + t.Fatalf("SetInferenceRoute named: %v", err) + } + + got, err := c.GetInferenceRoute(ctx, "scratch") + if err != nil { + t.Fatalf("GetInferenceRoute named: %v", err) + } + if got.Route != "scratch" { + t.Errorf("route name: want scratch, got %q", got.Route) + } + + // The default route remains absent. + if _, err := c.GetInferenceRoute(ctx, ""); !errors.Is(err, openshell.ErrNotFound) { + t.Fatalf("default route should be absent: got %v", err) + } +} diff --git a/internal/openshell/types.go b/internal/openshell/types.go index cf5c308..076dc5e 100644 --- a/internal/openshell/types.go +++ b/internal/openshell/types.go @@ -26,3 +26,28 @@ type Provider struct { Name string Type string } + +// InferenceRoute is the harness view of an inference route read from a gateway. +// +// Deliberately minimal (least-exposure firewall): only the fields the harness +// diffs or reports. Version is server-assigned and increments on every write. +type InferenceRoute struct { + Provider string + Model string + Route string + TimeoutSecs uint64 + Version uint64 +} + +// InferenceRouteConfig is a desired inference route to write. +// +// Provider and Model are required. Route "" targets the gateway default route. +// NoVerify skips the gateway's synchronous endpoint validation. TimeoutSecs 0 +// lets the gateway apply its default. +type InferenceRouteConfig struct { + Provider string + Model string + Route string + NoVerify bool + TimeoutSecs uint64 +} diff --git a/internal/plan/state_test.go b/internal/plan/state_test.go index cf3f5e8..cb376b9 100644 --- a/internal/plan/state_test.go +++ b/internal/plan/state_test.go @@ -183,6 +183,18 @@ func (r *recordingClient) Providers(ctx context.Context) ([]openshell.Provider, return r.wrapped.Providers(ctx) } +func (r *recordingClient) GetInferenceRoute(ctx context.Context, route string) (openshell.InferenceRoute, error) { + return r.wrapped.GetInferenceRoute(ctx, route) +} + +func (r *recordingClient) SetInferenceRoute(ctx context.Context, cfg openshell.InferenceRouteConfig) (openshell.InferenceRoute, error) { + return r.wrapped.SetInferenceRoute(ctx, cfg) +} + +func (r *recordingClient) DeleteInferenceRoute(ctx context.Context, route string) error { + return r.wrapped.DeleteInferenceRoute(ctx, route) +} + func (r *recordingClient) Close() error { r.closeCalled = true return r.wrapped.Close() @@ -201,6 +213,18 @@ func (e *errorClient) Providers(ctx context.Context) ([]openshell.Provider, erro return nil, e.err } +func (e *errorClient) GetInferenceRoute(ctx context.Context, route string) (openshell.InferenceRoute, error) { + return openshell.InferenceRoute{}, e.err +} + +func (e *errorClient) SetInferenceRoute(ctx context.Context, cfg openshell.InferenceRouteConfig) (openshell.InferenceRoute, error) { + return openshell.InferenceRoute{}, e.err +} + +func (e *errorClient) DeleteInferenceRoute(ctx context.Context, route string) error { + return e.err +} + func (e *errorClient) Close() error { return nil } From f4c39d2e167bfb2cf9aff928d92ebe9a43f5bbdd Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Tue, 25 Aug 2026 12:34:40 -0700 Subject: [PATCH 2/8] plan: real inference route diff (PR4b slice 2) Replace the flat inference validate with a real create/update/noop diff against the gateway's current route. - config.Inference gains TimeoutSecs() (duration string -> whole seconds), validated once at Resolve time so the pure plan builder can rely on it. - plan.ReadCurrentState reads the current inference route (only when inference is configured) into a widened InferenceState. A transient inference-read failure degrades to the not-capable validate fallback instead of flipping Reachable, since health and providers already proved the gateway reachable. - InferenceAction is the single owner of the create/update/noop rule, shared by the plan and (later) reconcile. An unset desired timeout means 'let the gateway default' and never forces an update. - isInferenceConfigured no longer counts Verify (a write modifier, not a route), so a verify-only config triggers no read or create. golangci-lint clean; go test ./... green. --- cmd/plan_test.go | 51 +++++++++++++ internal/config/env.go | 5 ++ internal/config/env_test.go | 36 +++++++++ internal/config/types.go | 26 ++++++- internal/config/types_test.go | 39 ++++++++++ internal/plan/plan.go | 104 ++++++++++++++++++-------- internal/plan/plan_test.go | 133 ++++++++++++++++++++++++++++++++++ internal/plan/state.go | 85 +++++++++++++++++++--- internal/plan/state_test.go | 131 ++++++++++++++++++++++++++++++++- 9 files changed, 565 insertions(+), 45 deletions(-) create mode 100644 internal/config/types_test.go diff --git a/cmd/plan_test.go b/cmd/plan_test.go index 7e0100c..25285e4 100644 --- a/cmd/plan_test.go +++ b/cmd/plan_test.go @@ -11,6 +11,7 @@ import ( 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" ) @@ -132,6 +133,56 @@ spec: } } +// TestPlanCmd_InferenceRealDiff proves the inference row reflects the gateway's +// actual route: a matching seeded route renders noop, not the old flat validate. +func TestPlanCmd_InferenceRealDiff(t *testing.T) { + tmpDir := t.TempDir() + + configPath := filepath.Join(tmpDir, "plan-test.yaml") + configContent := `apiVersion: harness.openshell.dev/v1alpha1 +kind: Harness +metadata: + name: plan-test +spec: + target: + gateway: test-gateway + inference: + provider: test-provider + model: claude-haiku-4-5 +` + if err := os.WriteFile(configPath, []byte(configContent), 0o644); err != nil { + t.Fatalf("write config: %v", err) + } + + fakeClient := testutil.NewFake("default", fake.WithHealthResult(&types.HealthResult{ + Healthy: true, Version: "1.0.0", + })) + // Seed a route matching the desired config, under the resolved default name. + if _, err := fakeClient.SetInferenceRoute(context.Background(), openshell.InferenceRouteConfig{ + Provider: "test-provider", Model: "claude-haiku-4-5", Route: plan.DefaultInferenceRoute, NoVerify: true, + }); err != nil { + t.Fatalf("seed route: %v", err) + } + + cmd := NewPlanCmd(tmpDir, testutil.FakeFactory(fakeClient)) + cmd.SetArgs([]string{"-f", configPath, "-o", "table"}) + + output, err := captureStdout(t, func() error { return cmd.Execute() }) + if err != nil { + t.Fatalf("cmd.Execute: %v", err) + } + + if !contains(output, "INFERENCE") { + t.Fatalf("output missing INFERENCE section:\n%s", output) + } + if !contains(output, "noop") { + t.Errorf("expected inference noop for a matching route:\n%s", output) + } + if contains(output, "does not report inference state") { + t.Errorf("capable gateway should not render the config-only caveat:\n%s", output) + } +} + // TestPlanCmd_JSONOutput tests JSON output format. func TestPlanCmd_JSONOutput(t *testing.T) { tmpDir := t.TempDir() diff --git a/internal/config/env.go b/internal/config/env.go index ae4f687..a156a4b 100644 --- a/internal/config/env.go +++ b/internal/config/env.go @@ -112,6 +112,11 @@ func Resolve(h *Harness, getenv func(string) string) (*Harness, error) { 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) + // Validate the (now expanded) timeout once, here at resolve time, so the plan + // diff and reconcile write can parse it without handling an error. + if _, err := s.Inference.TimeoutSecs(); err != nil { + errs = append(errs, fmt.Sprintf("spec.inference.timeout: %v", err)) + } s.Sandbox.Image = exp("spec.sandbox.image", h.Spec.Sandbox.Image) if p := h.Spec.Sandbox.Policy; p != nil { diff --git a/internal/config/env_test.go b/internal/config/env_test.go index e9eaf7c..fb5cda8 100644 --- a/internal/config/env_test.go +++ b/internal/config/env_test.go @@ -123,6 +123,42 @@ func TestResolveEmptyString(t *testing.T) { } } +func TestResolveInvalidTimeout(t *testing.T) { + h := &Harness{ + APIVersion: "harness.openshell.dev/v1alpha1", + Kind: "Harness", + Metadata: Metadata{Name: "test"}, + Spec: Spec{Inference: Inference{Timeout: "60"}}, // bare integer, no unit + } + + if _, err := Resolve(h, func(string) string { return "" }); err == nil { + t.Fatal("expected Resolve to reject a unitless inference timeout") + } +} + +func TestResolveValidTimeout(t *testing.T) { + h := &Harness{ + APIVersion: "harness.openshell.dev/v1alpha1", + Kind: "Harness", + Metadata: Metadata{Name: "test"}, + Spec: Spec{Inference: Inference{Timeout: "${INF_TIMEOUT}"}}, + } + + resolved, err := Resolve(h, func(name string) string { + if name == "INF_TIMEOUT" { + return "90s" + } + return "" + }) + if err != nil { + t.Fatalf("Resolve failed: %v", err) + } + secs, err := resolved.Spec.Inference.TimeoutSecs() + if err != nil || secs != 90 { + t.Errorf("resolved+parsed timeout = %d (err %v), want 90", secs, err) + } +} + func TestResolveNonSecretField(t *testing.T) { // Build Harness with ${SECRET_ISH} in non-secret field h := &Harness{ diff --git a/internal/config/types.go b/internal/config/types.go index 700e5e8..c4a56a6 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -5,7 +5,11 @@ // the source (e.g. "gcloud-adc"). package config -import "strings" +import ( + "fmt" + "strings" + "time" +) // Harness is the root v1alpha1 configuration document. type Harness struct { @@ -91,6 +95,26 @@ type Inference struct { Verify bool `yaml:"verify,omitempty"` } +// TimeoutSecs parses Timeout (a Go duration string like "60s" or "2m") into +// whole seconds. Empty → 0, meaning "let the gateway apply its default". A bare +// number without a unit (e.g. "60") is an error — the unit is required so the +// meaning is unambiguous. This is the single owner of the Timeout → seconds +// conversion; the plan diff and the reconcile write both call it. Validated at +// Resolve time, so by plan/reconcile time it cannot fail. +func (inf Inference) TimeoutSecs() (uint64, error) { + if inf.Timeout == "" { + return 0, nil + } + d, err := time.ParseDuration(inf.Timeout) + if err != nil { + return 0, fmt.Errorf("invalid timeout %q: want a duration string like \"60s\" or \"2m\"", inf.Timeout) + } + if d < 0 { + return 0, fmt.Errorf("invalid timeout %q: must not be negative", inf.Timeout) + } + return uint64(d.Round(time.Second) / time.Second), nil +} + // Sandbox describes the execution sandbox for this run. type Sandbox struct { Image string `yaml:"image,omitempty"` diff --git a/internal/config/types_test.go b/internal/config/types_test.go new file mode 100644 index 0000000..fdbdc1c --- /dev/null +++ b/internal/config/types_test.go @@ -0,0 +1,39 @@ +package config + +import "testing" + +func TestInferenceTimeoutSecs(t *testing.T) { + tests := []struct { + name string + timeout string + want uint64 + wantErr bool + }{ + {name: "empty is zero", timeout: "", want: 0}, + {name: "seconds", timeout: "60s", want: 60}, + {name: "minutes", timeout: "2m", want: 120}, + {name: "mixed", timeout: "1m30s", want: 90}, + {name: "sub-second rounds", timeout: "1500ms", want: 2}, + {name: "bare integer is an error", timeout: "60", wantErr: true}, + {name: "garbage is an error", timeout: "soon", wantErr: true}, + {name: "negative is an error", timeout: "-5s", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := Inference{Timeout: tt.timeout}.TimeoutSecs() + if tt.wantErr { + if err == nil { + t.Fatalf("expected error for %q, got %d", tt.timeout, got) + } + return + } + if err != nil { + t.Fatalf("unexpected error for %q: %v", tt.timeout, err) + } + if got != tt.want { + t.Errorf("TimeoutSecs(%q) = %d, want %d", tt.timeout, got, tt.want) + } + }) + } +} diff --git a/internal/plan/plan.go b/internal/plan/plan.go index 9c4b570..b4748bd 100644 --- a/internal/plan/plan.go +++ b/internal/plan/plan.go @@ -180,46 +180,85 @@ func buildProviderDetail(prov *config.Provider) string { return detail } -// buildInferenceGroup returns the INFERENCE group. The action is always -// validate: the gateway does not report inference state, so the plan can only -// validate the configured route against the desired config. current is threaded -// for uniformity with the other build* helpers and to carry the future -// inference-state read (see InferenceState) without a signature change. -func buildInferenceGroup(desired *config.Harness, current CurrentState) Group { - group := Group{Section: SectionInference} - - detail := buildInferenceDetail(desired.Spec.Inference) +// InferenceAction is the single owner of the inference create/update/noop rule. +// Both buildInferenceGroup (harness plan) and internal/reconcile call it, so the +// plan and the reconcile write can never disagree on what a change is. +// +// A gateway that does not serve inference state (cur.Capable false) yields +// validate — the plan can only echo the desired config. Verify is deliberately +// not part of the diff: the gateway does not report validation intent, so it +// only affects the write, never whether a change is needed. +// +// Precondition: desired must be Resolve-validated (config.Resolve), so its +// Timeout parses. An unresolved config with an invalid timeout silently degrades +// that term to 0; callers other than the plan/reconcile path must not rely on +// that behavior. +func InferenceAction(desired config.Inference, cur InferenceState) Action { + if !cur.Capable { + return ActionValidate + } + if !cur.Present { + return ActionCreate + } + // An unset desired timeout means "let the gateway apply its default" (0 == + // don't care), so it must not force an update against whatever nonzero + // default the gateway reports back. Only an explicitly configured timeout + // participates in the diff. "" is the sole "don't care" marker — "0s" parses + // to 0 too but is an explicit choice. + desiredSecs, _ := desired.TimeoutSecs() // Resolve-validated; error → 0 + timeoutDiffers := desired.Timeout != "" && desiredSecs != cur.TimeoutSecs + if desired.Provider != cur.Provider || + desired.Model != cur.Model || + timeoutDiffers { + return ActionUpdate + } + return ActionNoop +} - group.Resources = append(group.Resources, Resource{ - Name: "inference", - Action: ActionValidate, - Detail: detail, - }) +// buildInferenceGroup returns the INFERENCE group with the real diff action. +func buildInferenceGroup(desired *config.Harness, current CurrentState) Group { + inf := desired.Spec.Inference + action := InferenceAction(inf, current.Inference) - return group + return Group{ + Section: SectionInference, + Resources: []Resource{{ + Name: "inference", + Action: action, + Detail: buildInferenceDetail(inf, action), + }}, + } } -// buildInferenceDetail constructs a detail string for inference config. -func buildInferenceDetail(inf config.Inference) string { - detail := "" - +// buildInferenceDetail constructs a redaction-safe detail string for the +// inference action. provider/model (and timeout, if set) for a real change; a +// short note for noop; the config-only caveat only on the validate fallback. +func buildInferenceDetail(inf config.Inference, action Action) string { + pm := "" if inf.Provider != "" { - detail += inf.Provider + pm = inf.Provider } if inf.Model != "" { - if detail != "" { - detail += "/" + if pm != "" { + pm += "/" } - detail += inf.Model + pm += inf.Model } - - if detail == "" { - detail = "(provider/model unspecified)" + if pm == "" { + pm = "(provider/model unspecified)" } - detail += "; config only (gateway does not report inference state)" - - return detail + switch action { + case ActionNoop: + return pm + "; matches gateway" + case ActionValidate: + return pm + "; config only (gateway does not report inference state)" + default: // create / update + if inf.Timeout != "" { + pm += "; timeout " + inf.Timeout + } + return pm + } } // buildRunGroup returns the RUN group with descriptive actions. @@ -288,9 +327,12 @@ func buildRunGroup(desired *config.Harness) Group { return group } -// isInferenceConfigured returns true if inference config has any meaningful fields set. +// isInferenceConfigured reports whether inference is meaningfully configured. +// Verify is deliberately excluded: it is a modifier on how a route is written, +// not a route on its own, so a config that only sets verify (no provider/model/ +// route/timeout) has nothing to reconcile and must not trigger a read or create. func isInferenceConfigured(inf config.Inference) bool { - return inf.Route != "" || inf.Provider != "" || inf.Model != "" || inf.Timeout != "" || inf.Verify + return inf.Route != "" || inf.Provider != "" || inf.Model != "" || inf.Timeout != "" } // hasRunConfig returns true if any run-related config is present. diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go index c85e544..405bb67 100644 --- a/internal/plan/plan_test.go +++ b/internal/plan/plan_test.go @@ -305,6 +305,139 @@ func TestBuild_InferenceGroupWhenConfigured(t *testing.T) { } } +func TestInferenceAction(t *testing.T) { + desired := config.Inference{Provider: "gcp", Model: "claude-opus-4-8", Timeout: "60s"} + + tests := []struct { + name string + cur InferenceState + want Action + }{ + { + name: "not capable falls back to validate", + cur: InferenceState{Capable: false}, + want: ActionValidate, + }, + { + name: "capable but absent creates", + cur: InferenceState{Capable: true, Present: false}, + want: ActionCreate, + }, + { + name: "model mismatch updates", + cur: InferenceState{Capable: true, Present: true, Provider: "gcp", Model: "claude-sonnet-5", TimeoutSecs: 60}, + want: ActionUpdate, + }, + { + name: "provider mismatch updates", + cur: InferenceState{Capable: true, Present: true, Provider: "aws", Model: "claude-opus-4-8", TimeoutSecs: 60}, + want: ActionUpdate, + }, + { + name: "timeout mismatch updates", + cur: InferenceState{Capable: true, Present: true, Provider: "gcp", Model: "claude-opus-4-8", TimeoutSecs: 30}, + want: ActionUpdate, + }, + { + name: "exact match noops", + cur: InferenceState{Capable: true, Present: true, Provider: "gcp", Model: "claude-opus-4-8", TimeoutSecs: 60}, + want: ActionNoop, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := InferenceAction(desired, tt.cur); got != tt.want { + t.Errorf("InferenceAction = %s, want %s", got, tt.want) + } + }) + } +} + +// TestInferenceAction_UnsetTimeoutMatchesZero pins that an unset desired timeout +// (secs 0, gateway default) noops against a route the gateway reports as 0. +func TestInferenceAction_UnsetTimeoutMatchesZero(t *testing.T) { + desired := config.Inference{Provider: "gcp", Model: "claude-opus-4-8"} // no timeout + cur := InferenceState{Capable: true, Present: true, Provider: "gcp", Model: "claude-opus-4-8", TimeoutSecs: 0} + if got := InferenceAction(desired, cur); got != ActionNoop { + t.Errorf("InferenceAction = %s, want noop", got) + } +} + +// TestInferenceAction_UnsetTimeoutIgnoresGatewayDefault is the finding-1 +// regression: an unset desired timeout means "don't care", so it must noop even +// when the gateway reports a nonzero default it applied — otherwise the plan +// reports a perpetual update it can never resolve. +func TestInferenceAction_UnsetTimeoutIgnoresGatewayDefault(t *testing.T) { + desired := config.Inference{Provider: "gcp", Model: "claude-opus-4-8"} // no timeout + cur := InferenceState{Capable: true, Present: true, Provider: "gcp", Model: "claude-opus-4-8", TimeoutSecs: 300} + if got := InferenceAction(desired, cur); got != ActionNoop { + t.Errorf("InferenceAction = %s, want noop (unset timeout must not chase the gateway default)", got) + } +} + +// TestInferenceAction_ExplicitTimeoutStillDiffs guards that the finding-1 fix did +// not neuter the timeout diff: an explicitly configured timeout still updates +// against a mismatched gateway value. +func TestInferenceAction_ExplicitTimeoutStillDiffs(t *testing.T) { + desired := config.Inference{Provider: "gcp", Model: "claude-opus-4-8", Timeout: "60s"} + cur := InferenceState{Capable: true, Present: true, Provider: "gcp", Model: "claude-opus-4-8", TimeoutSecs: 300} + if got := InferenceAction(desired, cur); got != ActionUpdate { + t.Errorf("InferenceAction = %s, want update (explicit timeout must still diff)", got) + } +} + +func TestResolveInferenceRoute(t *testing.T) { + if got := resolveInferenceRoute(""); got != DefaultInferenceRoute { + t.Errorf("empty route: got %q, want %q", got, DefaultInferenceRoute) + } + if got := resolveInferenceRoute("custom-route"); got != "custom-route" { + t.Errorf("explicit route: got %q, want %q", got, "custom-route") + } +} + +func TestBuild_InferenceRealDiff(t *testing.T) { + desired := &config.Harness{ + Spec: config.Spec{ + Target: config.Target{Gateway: "test-gateway"}, + Inference: config.Inference{Provider: "gcp", Model: "claude-opus-4-8"}, + }, + } + + infGroup := func(p *Plan) *Resource { + for i := range p.Groups { + if p.Groups[i].Section == SectionInference { + return &p.Groups[i].Resources[0] + } + } + return nil + } + + // Capable + absent → create. + res := infGroup(Build(desired, CurrentState{ + Reachable: true, + Inference: InferenceState{Capable: true, Present: false}, + })) + if res == nil || res.Action != ActionCreate { + t.Fatalf("absent route: want create, got %+v", res) + } + if strings.Contains(res.Detail, "config only") { + t.Errorf("create detail should not carry the config-only caveat: %s", res.Detail) + } + + // Capable + matching → noop. + res = infGroup(Build(desired, CurrentState{ + Reachable: true, + Inference: InferenceState{Capable: true, Present: true, Provider: "gcp", Model: "claude-opus-4-8"}, + })) + if res == nil || res.Action != ActionNoop { + t.Fatalf("matching route: want noop, got %+v", res) + } + if !strings.Contains(res.Detail, "matches gateway") { + t.Errorf("noop detail should say it matches: %s", res.Detail) + } +} + func TestBuild_NoInferenceGroupWhenEmpty(t *testing.T) { desired := &config.Harness{ Spec: config.Spec{ diff --git a/internal/plan/state.go b/internal/plan/state.go index 8eaaeb0..e624cae 100644 --- a/internal/plan/state.go +++ b/internal/plan/state.go @@ -8,16 +8,26 @@ import ( "github.com/stackrox/harness-openshell/internal/openshell" ) -// InferenceState captures the gateway's reported inference capabilities. +// DefaultInferenceRoute is the route name the gateway assigns when a config +// leaves spec.inference.route empty. The harness resolves "" to this name so the +// plan read and the reconcile write address the same route (the SDK fake does +// not default an empty name; a real gateway does). Single owner: reconcile reads +// it from here rather than redefining it. +const DefaultInferenceRoute = "inference.local" + +// InferenceState is the gateway's current inference route, read at plan time. // -// It is the reserved seam for a gateway inference-state read: today no gateway -// reports its inference route, so Capable is always false and the plan renders -// inference from desired config alone. When a capable gateway lands, this struct -// (and the desired arg to ReadCurrentState) carry the read without reshaping -// CurrentState or the pure Build signature. +// Capable reports whether the gateway serves inference route state at all: an +// older gateway that does not implement the RPC leaves Capable false and the +// plan falls back to a config-only validate. When Capable is true, Present says +// whether a route exists, and the remaining fields carry it for the diff. type InferenceState struct { - Capable bool // whether the gateway reports its inference route - Route string // the reported route, set only when Capable is true + Capable bool // the gateway serves inference route state + Present bool // a route exists (only meaningful when Capable) + Provider string // populated when Present + Model string // populated when Present + Route string // populated when Present + TimeoutSecs uint64 // populated when Present } // CurrentState is a snapshot of the gateway's current state, read at plan time. @@ -31,9 +41,8 @@ type CurrentState struct { // ReadCurrentState reads the current gateway state and returns a snapshot. It is // the only I/O in the package. It degrades gracefully: if the gateway is // unreachable or unauthenticated, Reachable is set to false and a nil error is -// returned; other errors are escalated. Inference.Capable is left false because -// the gateway does not report its inference route; desired is the reserved seam -// for the future inference-state read (see InferenceState). +// returned; other errors are escalated. When desired configures inference, it +// reads the current route so the plan can show a real create/update/noop diff. func ReadCurrentState(ctx context.Context, c openshell.Client, desired *config.Harness) (CurrentState, error) { var state CurrentState @@ -60,5 +69,59 @@ func ReadCurrentState(ctx context.Context, c openshell.Client, desired *config.H } state.Providers = providers + // Read the inference route, but only when desired configures inference (no + // point probing an unused subsystem). Health and providers already proved the + // gateway reachable, so a transient inference-read failure must not flip + // Reachable (that would render a misleading login-required target next to + // populated providers). Instead it degrades to the not-capable validate + // fallback — the same config-only outcome as an older gateway that does not + // serve inference state at all. + if isInferenceConfigured(desired.Spec.Inference) { + inf, err := readInferenceState(ctx, c, desired.Spec.Inference) + if err != nil { + if errors.Is(err, openshell.ErrUnavailable) || errors.Is(err, openshell.ErrUnauthenticated) { + state.Inference = InferenceState{Capable: false} + } else { + return state, err + } + } else { + state.Inference = inf + } + } + return state, nil } + +// readInferenceState reads the current inference route for the desired config. +// An absent route is not an error (Capable, not Present); a gateway that does +// not serve inference (ErrUnsupported) leaves Capable false so the plan falls +// back to a config-only validate. +func readInferenceState(ctx context.Context, c openshell.Client, desired config.Inference) (InferenceState, error) { + route, err := c.GetInferenceRoute(ctx, resolveInferenceRoute(desired.Route)) + switch { + case err == nil: + return InferenceState{ + Capable: true, + Present: true, + Provider: route.Provider, + Model: route.Model, + Route: route.Route, + TimeoutSecs: route.TimeoutSecs, + }, nil + case errors.Is(err, openshell.ErrNotFound): + return InferenceState{Capable: true, Present: false}, nil + case errors.Is(err, openshell.ErrUnsupported): + return InferenceState{Capable: false}, nil + default: + return InferenceState{}, err + } +} + +// resolveInferenceRoute maps an empty configured route to the gateway default so +// the read and the write address the same route. +func resolveInferenceRoute(route string) string { + if route == "" { + return DefaultInferenceRoute + } + return route +} diff --git a/internal/plan/state_test.go b/internal/plan/state_test.go index cb376b9..117271d 100644 --- a/internal/plan/state_test.go +++ b/internal/plan/state_test.go @@ -116,7 +116,10 @@ func TestReadCurrentState_OtherErrorEscalates(t *testing.T) { } } -func TestReadCurrentState_InferenceAlwaysFalse(t *testing.T) { +// TestReadCurrentState_InferenceNotReadWhenUnconfigured pins that an unused +// inference subsystem is never probed: with no desired inference config, the +// route is not read and Inference stays zero (Capable=false). +func TestReadCurrentState_InferenceNotReadWhenUnconfigured(t *testing.T) { ctx := context.Background() client := testutil.NewFake("default", fake.WithHealthResult(&types.HealthResult{Healthy: true, Version: "0.0.110"}), @@ -129,13 +132,137 @@ func TestReadCurrentState_InferenceAlwaysFalse(t *testing.T) { } if state.Inference.Capable { - t.Error("expected Inference.Capable=false") + t.Error("expected Inference.Capable=false when inference is unconfigured") } if state.Inference.Route != "" { t.Error("expected Inference.Route empty") } } +// inferenceDesired is a minimal harness with inference configured (default +// route), for exercising the inference read path. +func inferenceDesired() *config.Harness { + return &config.Harness{ + Spec: config.Spec{ + Inference: config.Inference{Provider: "gcp", Model: "claude-opus-4-8"}, + }, + } +} + +func TestReadCurrentState_InferencePresent(t *testing.T) { + ctx := context.Background() + client, _ := testutil.NewFakeClient("default", + fake.WithHealthResult(&types.HealthResult{Healthy: true, Version: "0.0.110"}), + ) + // Seed the route under the resolved default name so the read finds it. + if _, err := client.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ + Provider: "gcp", Model: "claude-opus-4-8", Route: DefaultInferenceRoute, TimeoutSecs: 60, NoVerify: true, + }); err != nil { + t.Fatalf("seed SetInferenceRoute: %v", err) + } + + state, err := ReadCurrentState(ctx, client, inferenceDesired()) + if err != nil { + t.Fatalf("ReadCurrentState: %v", err) + } + + got := state.Inference + if !got.Capable || !got.Present { + t.Fatalf("want Capable && Present, got %+v", got) + } + if got.Provider != "gcp" || got.Model != "claude-opus-4-8" || got.TimeoutSecs != 60 { + t.Errorf("route not populated: %+v", got) + } + if got.Route != DefaultInferenceRoute { + t.Errorf("route name: want %q, got %q", DefaultInferenceRoute, got.Route) + } +} + +func TestReadCurrentState_InferenceAbsent(t *testing.T) { + ctx := context.Background() + client := testutil.NewFake("default", + fake.WithHealthResult(&types.HealthResult{Healthy: true, Version: "0.0.110"}), + ) + + state, err := ReadCurrentState(ctx, client, inferenceDesired()) + if err != nil { + t.Fatalf("ReadCurrentState: %v", err) + } + + if !state.Inference.Capable { + t.Error("expected Capable=true (gateway serves inference, route just absent)") + } + if state.Inference.Present { + t.Error("expected Present=false for a gateway with no route") + } +} + +func TestReadCurrentState_InferenceUnsupportedNotCapable(t *testing.T) { + ctx := context.Background() + base := testutil.NewFake("default", + fake.WithHealthResult(&types.HealthResult{Healthy: true, Version: "0.0.110"}), + ) + client := &inferenceErrClient{Client: base, getErr: openshell.ErrUnsupported} + + state, err := ReadCurrentState(ctx, client, inferenceDesired()) + if err != nil { + t.Fatalf("ReadCurrentState should degrade on ErrUnsupported: %v", err) + } + + if state.Inference.Capable { + t.Error("expected Capable=false when the gateway does not serve inference") + } +} + +// TestReadCurrentState_InferenceTransientErrorKeepsReachable pins finding-4 +// behavior: health and providers already proved the gateway reachable, so a +// transient inference-read failure degrades inference to the not-capable +// validate fallback rather than flipping Reachable to false. +func TestReadCurrentState_InferenceTransientErrorKeepsReachable(t *testing.T) { + ctx := context.Background() + for _, transient := range []error{openshell.ErrUnavailable, openshell.ErrUnauthenticated} { + base := testutil.NewFake("default", + fake.WithHealthResult(&types.HealthResult{Healthy: true, Version: "0.0.110"}), + ) + client := &inferenceErrClient{Client: base, getErr: transient} + + state, err := ReadCurrentState(ctx, client, inferenceDesired()) + if err != nil { + t.Fatalf("%v: ReadCurrentState should degrade, got %v", transient, err) + } + if !state.Reachable { + t.Errorf("%v: expected Reachable=true (health+providers succeeded)", transient) + } + if state.Inference.Capable { + t.Errorf("%v: expected Inference.Capable=false (validate fallback)", transient) + } + } +} + +func TestReadCurrentState_InferenceOtherErrorEscalates(t *testing.T) { + ctx := context.Background() + base := testutil.NewFake("default", + fake.WithHealthResult(&types.HealthResult{Healthy: true, Version: "0.0.110"}), + ) + client := &inferenceErrClient{Client: base, getErr: openshell.ErrPermission} + + if _, err := ReadCurrentState(ctx, client, inferenceDesired()); !errors.Is(err, openshell.ErrPermission) { + t.Fatalf("expected ErrPermission to escalate, got %v", err) + } +} + +// inferenceErrClient wraps a healthy client but forces GetInferenceRoute to +// return a chosen error, exercising read paths the SDK fake cannot produce +// (e.g. ErrUnsupported). +type inferenceErrClient struct { + openshell.Client + getErr error +} + +func (c *inferenceErrClient) GetInferenceRoute(context.Context, string) (openshell.InferenceRoute, error) { + return openshell.InferenceRoute{}, c.getErr +} + func TestReadCurrentState_OnlyReadMethodsCalled(t *testing.T) { ctx := context.Background() client, fakeClient := testutil.NewFakeClient("default", From 06562a15d470ceaa1fab19f576283d04fb3dc1e0 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Tue, 25 Aug 2026 13:09:22 -0700 Subject: [PATCH 3/8] reconcile: SDK inference reconcile engine + verify *bool (PR4b slice 3) Add internal/reconcile, the SDK-free write path that drives the gateway's inference route to match desired config. It shares plan.InferenceAction (single diff-rule owner) and, unlike the read-only plan, does not degrade: transient/permission/unsupported errors propagate so a caller learns the write did not happen. - config.Inference.Verify becomes *bool with VerifyEnabled() (nil->true), so unset means verify (the safe default) instead of the old always-skip. reconcile computes NoVerify = !VerifyEnabled() at the one mapping site, retiring the hardcoded --no-verify default. - plan.ReadInferenceState / plan.ResolveInferenceRoute are exported so reconcile reuses the route read and resolution (one owner each). Per user decision (option A), the physical swap of the legacy write site (cmd/providers.go:151) is deferred to PR4a: that apply path has no openshell target, so swapping it is the apply-on-SDK migration PR4b excludes. The engine ships ready for PR4a to call. golangci-lint clean; go test ./... green; internal/reconcile imports no SDK or cobra. --- internal/config/types.go | 14 +- internal/plan/plan_test.go | 4 +- internal/plan/state.go | 16 +- internal/reconcile/inference.go | 89 ++++++++++ internal/reconcile/inference_test.go | 238 +++++++++++++++++++++++++++ 5 files changed, 351 insertions(+), 10 deletions(-) create mode 100644 internal/reconcile/inference.go create mode 100644 internal/reconcile/inference_test.go diff --git a/internal/config/types.go b/internal/config/types.go index c4a56a6..7b8feaa 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -92,7 +92,19 @@ type Inference struct { Provider string `yaml:"provider,omitempty"` Model string `yaml:"model,omitempty"` Timeout string `yaml:"timeout,omitempty"` - Verify bool `yaml:"verify,omitempty"` + // Verify controls the gateway's synchronous endpoint validation on write. + // It is a *bool so "unset" is distinct from "false": unset (nil) means + // verify (the safe default), so only an explicit `verify: false` skips it. + // Verify is not part of the reconcile diff, so changing only this field does + // not by itself trigger a re-write; it takes effect on the next write caused + // by a provider/model/timeout change. + Verify *bool `yaml:"verify,omitempty"` +} + +// VerifyEnabled reports whether endpoint verification should run on write. +// Unset (nil) defaults to true; this is the single owner of the nil→verify rule. +func (inf Inference) VerifyEnabled() bool { + return inf.Verify == nil || *inf.Verify } // TimeoutSecs parses Timeout (a Go duration string like "60s" or "2m") into diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go index 405bb67..6b2512a 100644 --- a/internal/plan/plan_test.go +++ b/internal/plan/plan_test.go @@ -388,10 +388,10 @@ func TestInferenceAction_ExplicitTimeoutStillDiffs(t *testing.T) { } func TestResolveInferenceRoute(t *testing.T) { - if got := resolveInferenceRoute(""); got != DefaultInferenceRoute { + if got := ResolveInferenceRoute(""); got != DefaultInferenceRoute { t.Errorf("empty route: got %q, want %q", got, DefaultInferenceRoute) } - if got := resolveInferenceRoute("custom-route"); got != "custom-route" { + if got := ResolveInferenceRoute("custom-route"); got != "custom-route" { t.Errorf("explicit route: got %q, want %q", got, "custom-route") } } diff --git a/internal/plan/state.go b/internal/plan/state.go index e624cae..df9f47b 100644 --- a/internal/plan/state.go +++ b/internal/plan/state.go @@ -77,7 +77,7 @@ func ReadCurrentState(ctx context.Context, c openshell.Client, desired *config.H // fallback — the same config-only outcome as an older gateway that does not // serve inference state at all. if isInferenceConfigured(desired.Spec.Inference) { - inf, err := readInferenceState(ctx, c, desired.Spec.Inference) + inf, err := ReadInferenceState(ctx, c, desired.Spec.Inference) if err != nil { if errors.Is(err, openshell.ErrUnavailable) || errors.Is(err, openshell.ErrUnauthenticated) { state.Inference = InferenceState{Capable: false} @@ -92,12 +92,14 @@ func ReadCurrentState(ctx context.Context, c openshell.Client, desired *config.H return state, nil } -// readInferenceState reads the current inference route for the desired config. +// ReadInferenceState reads the current inference route for the desired config. // An absent route is not an error (Capable, not Present); a gateway that does // not serve inference (ErrUnsupported) leaves Capable false so the plan falls -// back to a config-only validate. -func readInferenceState(ctx context.Context, c openshell.Client, desired config.Inference) (InferenceState, error) { - route, err := c.GetInferenceRoute(ctx, resolveInferenceRoute(desired.Route)) +// back to a config-only validate. Transient errors (unavailable/unauthenticated) +// and ErrPermission are propagated so the caller decides whether to degrade +// (the read-only plan) or fail (the reconcile write path). +func ReadInferenceState(ctx context.Context, c openshell.Client, desired config.Inference) (InferenceState, error) { + route, err := c.GetInferenceRoute(ctx, ResolveInferenceRoute(desired.Route)) switch { case err == nil: return InferenceState{ @@ -117,9 +119,9 @@ func readInferenceState(ctx context.Context, c openshell.Client, desired config. } } -// resolveInferenceRoute maps an empty configured route to the gateway default so +// ResolveInferenceRoute maps an empty configured route to the gateway default so // the read and the write address the same route. -func resolveInferenceRoute(route string) string { +func ResolveInferenceRoute(route string) string { if route == "" { return DefaultInferenceRoute } diff --git a/internal/reconcile/inference.go b/internal/reconcile/inference.go new file mode 100644 index 0000000..d056025 --- /dev/null +++ b/internal/reconcile/inference.go @@ -0,0 +1,89 @@ +// Package reconcile drives desired harness config to actual gateway state +// through the openshell firewall. It is SDK-free and cobra-free: it speaks only +// the openshell vocabulary, config, and the shared plan diff rule, so the write +// path and the read-only plan can never disagree on what a change is. +package reconcile + +import ( + "context" + "fmt" + + "github.com/stackrox/harness-openshell/internal/config" + "github.com/stackrox/harness-openshell/internal/openshell" + "github.com/stackrox/harness-openshell/internal/plan" +) + +// InferenceResult reports what ReconcileInference did and the resulting route. +// +// On create/update, Route is the gateway's authoritative response (Version and +// all metadata populated). On noop, Route is a partial echo of the current route +// read at plan time — Provider/Model/Route/TimeoutSecs only, no Version or +// validation metadata (the plan read does not carry them). Do not compare +// Route.Version across actions. +type InferenceResult struct { + Action plan.Action // create / update / noop + Route openshell.InferenceRoute // the route now on the gateway +} + +// ReconcileInference drives the gateway's inference route to match desired. It +// reads the current route, computes the action via the shared plan.InferenceAction +// rule, and writes only when the action is create or update. Unlike the read-only +// plan it does not degrade: a transient/permission/unsupported read error is +// returned, so the caller learns the write did not happen. +// +// Precondition: desired must be Resolve-validated (config.Resolve) so its timeout +// parses; ReconcileInference re-parses defensively and errors if it does not. +// +// Verify is not part of the diff (the gateway does not report validation intent), +// so a config that only flips verify with provider/model/timeout unchanged yields +// noop and the new verify value does not take effect until some other field also +// changes. Likewise, an update triggered by a provider/model change with an unset +// timeout writes 0, resetting any non-default gateway timeout to the default — +// "unset timeout" always means "let the gateway decide". +func ReconcileInference(ctx context.Context, c openshell.Client, desired config.Inference) (InferenceResult, error) { + cur, err := plan.ReadInferenceState(ctx, c, desired) + if err != nil { + return InferenceResult{}, fmt.Errorf("reading inference route: %w", err) + } + + action := plan.InferenceAction(desired, cur) + switch action { + case plan.ActionValidate: + // InferenceAction only yields validate when the gateway does not serve + // inference route state. There is nothing to write. + return InferenceResult{}, fmt.Errorf("inference route configuration: %w", openshell.ErrUnsupported) + + case plan.ActionNoop: + return InferenceResult{ + Action: plan.ActionNoop, + Route: openshell.InferenceRoute{ + Provider: cur.Provider, + Model: cur.Model, + Route: cur.Route, + TimeoutSecs: cur.TimeoutSecs, + }, + }, nil + + case plan.ActionCreate, plan.ActionUpdate: + secs, err := desired.TimeoutSecs() + if err != nil { + return InferenceResult{}, fmt.Errorf("inference timeout: %w", err) + } + // The single site mapping positive-sense Verify to the SDK's negative + // NoVerify. SetInferenceRoute is an upsert, so it serves create and update. + route, err := c.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ + Provider: desired.Provider, + Model: desired.Model, + Route: plan.ResolveInferenceRoute(desired.Route), + NoVerify: !desired.VerifyEnabled(), + TimeoutSecs: secs, + }) + if err != nil { + return InferenceResult{}, fmt.Errorf("setting inference route: %w", err) + } + return InferenceResult{Action: action, Route: route}, nil + + default: + return InferenceResult{}, fmt.Errorf("unexpected inference action %q", action) + } +} diff --git a/internal/reconcile/inference_test.go b/internal/reconcile/inference_test.go new file mode 100644 index 0000000..0d743df --- /dev/null +++ b/internal/reconcile/inference_test.go @@ -0,0 +1,238 @@ +package reconcile + +import ( + "context" + "errors" + "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/config" + "github.com/stackrox/harness-openshell/internal/openshell" + "github.com/stackrox/harness-openshell/internal/plan" + "github.com/stackrox/harness-openshell/internal/testutil" +) + +func healthyClient(t *testing.T) (openshell.Client, *fake.Client) { + t.Helper() + return testutil.NewFakeClient("default", + fake.WithHealthResult(&types.HealthResult{Healthy: true, Version: "0.0.110"}), + ) +} + +func TestReconcileInference_Create(t *testing.T) { + ctx := context.Background() + c, _ := healthyClient(t) + desired := config.Inference{Provider: "gcp", Model: "claude-opus-4-8"} + + res, err := ReconcileInference(ctx, c, desired) + if err != nil { + t.Fatalf("ReconcileInference: %v", err) + } + if res.Action != plan.ActionCreate { + t.Errorf("action = %s, want create", res.Action) + } + if res.Route.Provider != "gcp" || res.Route.Model != "claude-opus-4-8" { + t.Errorf("route not populated: %+v", res.Route) + } + // The route is now readable under the resolved default name. + got, err := c.GetInferenceRoute(ctx, plan.DefaultInferenceRoute) + if err != nil { + t.Fatalf("GetInferenceRoute after create: %v", err) + } + if got.Model != "claude-opus-4-8" { + t.Errorf("persisted model = %q, want claude-opus-4-8", got.Model) + } +} + +func TestReconcileInference_Update(t *testing.T) { + ctx := context.Background() + c, _ := healthyClient(t) + // Seed a route with a stale model under the resolved default name. + if _, err := c.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ + Provider: "gcp", Model: "claude-sonnet-5", Route: plan.DefaultInferenceRoute, + }); err != nil { + t.Fatalf("seed: %v", err) + } + + res, err := ReconcileInference(ctx, c, config.Inference{Provider: "gcp", Model: "claude-opus-4-8"}) + if err != nil { + t.Fatalf("ReconcileInference: %v", err) + } + if res.Action != plan.ActionUpdate { + t.Errorf("action = %s, want update", res.Action) + } + if res.Route.Model != "claude-opus-4-8" { + t.Errorf("updated model = %q, want claude-opus-4-8", res.Route.Model) + } +} + +func TestReconcileInference_Noop(t *testing.T) { + ctx := context.Background() + c, _ := healthyClient(t) + seed, err := c.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ + Provider: "gcp", Model: "claude-opus-4-8", Route: plan.DefaultInferenceRoute, + }) + if err != nil { + t.Fatalf("seed: %v", err) + } + + res, err := ReconcileInference(ctx, c, config.Inference{Provider: "gcp", Model: "claude-opus-4-8"}) + if err != nil { + t.Fatalf("ReconcileInference: %v", err) + } + if res.Action != plan.ActionNoop { + t.Errorf("action = %s, want noop", res.Action) + } + // A noop must not write: SetRoute increments Version, so the persisted + // version must equal the seed's. + got, err := c.GetInferenceRoute(ctx, plan.DefaultInferenceRoute) + if err != nil { + t.Fatalf("GetInferenceRoute: %v", err) + } + if got.Version != seed.Version { + t.Errorf("noop wrote the route: version %d -> %d", seed.Version, got.Version) + } +} + +// TestReconcileInference_VerifyMapping pins the single NoVerify mapping site: +// unset verify → verify (NoVerify false); explicit false → NoVerify true. +func TestReconcileInference_VerifyMapping(t *testing.T) { + ctx := context.Background() + tru, fls := true, false + tests := []struct { + name string + verify *bool + wantNoVerify bool + }{ + {name: "unset defaults to verify", verify: nil, wantNoVerify: false}, + {name: "explicit true verifies", verify: &tru, wantNoVerify: false}, + {name: "explicit false skips", verify: &fls, wantNoVerify: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + base, _ := healthyClient(t) + rec := &capturingClient{Client: base} + _, err := ReconcileInference(ctx, rec, config.Inference{ + Provider: "gcp", Model: "claude-opus-4-8", Verify: tt.verify, + }) + if err != nil { + t.Fatalf("ReconcileInference: %v", err) + } + if !rec.setCalled { + t.Fatal("expected SetInferenceRoute to be called") + } + if rec.setCfg.NoVerify != tt.wantNoVerify { + t.Errorf("NoVerify = %v, want %v", rec.setCfg.NoVerify, tt.wantNoVerify) + } + }) + } +} + +func TestReconcileInference_ExplicitTimeoutMapped(t *testing.T) { + ctx := context.Background() + base, _ := healthyClient(t) + rec := &capturingClient{Client: base} + if _, err := ReconcileInference(ctx, rec, config.Inference{ + Provider: "gcp", Model: "claude-opus-4-8", Timeout: "90s", + }); err != nil { + t.Fatalf("ReconcileInference: %v", err) + } + if rec.setCfg.TimeoutSecs != 90 { + t.Errorf("TimeoutSecs = %d, want 90", rec.setCfg.TimeoutSecs) + } +} + +// TestReconcileInference_UpdateUnsetTimeoutWritesDefault pins the intended +// semantics (review finding #3): an update triggered by a model change with an +// unset desired timeout writes 0, i.e. resets the stored timeout to the gateway +// default. "Unset timeout" always means "let the gateway decide", even mid-update. +func TestReconcileInference_UpdateUnsetTimeoutWritesDefault(t *testing.T) { + ctx := context.Background() + base, _ := healthyClient(t) + // Seed a route with a non-default timeout. + if _, err := base.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ + Provider: "gcp", Model: "claude-sonnet-5", Route: plan.DefaultInferenceRoute, TimeoutSecs: 300, + }); err != nil { + t.Fatalf("seed: %v", err) + } + rec := &capturingClient{Client: base} + // Change only the model, leaving timeout unset. + res, err := ReconcileInference(ctx, rec, config.Inference{Provider: "gcp", Model: "claude-opus-4-8"}) + if err != nil { + t.Fatalf("ReconcileInference: %v", err) + } + if res.Action != plan.ActionUpdate { + t.Fatalf("action = %s, want update", res.Action) + } + if rec.setCfg.TimeoutSecs != 0 { + t.Errorf("TimeoutSecs written = %d, want 0 (unset resets to gateway default)", rec.setCfg.TimeoutSecs) + } +} + +func TestReconcileInference_UnsupportedErrors(t *testing.T) { + ctx := context.Background() + base, _ := healthyClient(t) + c := &getErrClient{Client: base, err: openshell.ErrUnsupported} + _, err := ReconcileInference(ctx, c, config.Inference{Provider: "gcp", Model: "claude-opus-4-8"}) + if !errors.Is(err, openshell.ErrUnsupported) { + t.Fatalf("expected ErrUnsupported, got %v", err) + } +} + +func TestReconcileInference_ReadErrorPropagates(t *testing.T) { + ctx := context.Background() + base, _ := healthyClient(t) + for _, want := range []error{openshell.ErrUnavailable, openshell.ErrPermission} { + c := &getErrClient{Client: base, err: want} + _, err := ReconcileInference(ctx, c, config.Inference{Provider: "gcp", Model: "claude-opus-4-8"}) + if !errors.Is(err, want) { + t.Errorf("expected %v to propagate (no degradation), got %v", want, err) + } + } +} + +func TestReconcileInference_WriteErrorPropagates(t *testing.T) { + ctx := context.Background() + base, _ := healthyClient(t) + c := &setErrClient{Client: base, err: openshell.ErrPermission} + _, err := ReconcileInference(ctx, c, config.Inference{Provider: "gcp", Model: "claude-opus-4-8"}) + if !errors.Is(err, openshell.ErrPermission) { + t.Fatalf("expected write ErrPermission to propagate, got %v", err) + } +} + +// capturingClient records the last SetInferenceRoute config while delegating to +// a real fake-backed client, so the outbound mapping can be asserted. +type capturingClient struct { + openshell.Client + setCalled bool + setCfg openshell.InferenceRouteConfig +} + +func (c *capturingClient) SetInferenceRoute(ctx context.Context, cfg openshell.InferenceRouteConfig) (openshell.InferenceRoute, error) { + c.setCalled = true + c.setCfg = cfg + return c.Client.SetInferenceRoute(ctx, cfg) +} + +// getErrClient forces GetInferenceRoute to a chosen error. +type getErrClient struct { + openshell.Client + err error +} + +func (c *getErrClient) GetInferenceRoute(context.Context, string) (openshell.InferenceRoute, error) { + return openshell.InferenceRoute{}, c.err +} + +// setErrClient reads normally but forces SetInferenceRoute to a chosen error. +type setErrClient struct { + openshell.Client + err error +} + +func (c *setErrClient) SetInferenceRoute(context.Context, openshell.InferenceRouteConfig) (openshell.InferenceRoute, error) { + return openshell.InferenceRoute{}, c.err +} From 4e44978d1d3127cdfca5d5417ce43604738d71b1 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Tue, 25 Aug 2026 13:13:41 -0700 Subject: [PATCH 4/8] plan,config,reconcile: whole-spec review fixes (PR4b) - plan.InferenceAction: treat a 0-second desired timeout as 'don't care' by guarding on the value, not the string. '0s' resolved to 0 but slipped the old desired.Timeout != "" guard, so it churned a perpetual update against the gateway's nonzero default (0 always stores the default, so '0s' can never be a stored value). '' and '0s' now behave identically. - config.Resolve: deep-copy Inference.Verify so the resolved struct never aliases the input's *bool, matching the PolicyRef pattern; add a parse+resolve round-trip test for verify:false. - reconcile: document that ReconcileInference does not gate on configured-ness (empty desired -> ErrInvalidArgument) and add a cross-path test locking plan and reconcile to the same InferenceAction. go test ./... green; golangci-lint clean. --- internal/config/env.go | 4 +++ internal/config/env_test.go | 29 +++++++++++++++++++ internal/plan/plan.go | 13 +++++---- internal/plan/plan_test.go | 12 ++++++++ internal/reconcile/inference.go | 5 +++- internal/reconcile/inference_test.go | 42 ++++++++++++++++++++++++++++ 6 files changed, 98 insertions(+), 7 deletions(-) diff --git a/internal/config/env.go b/internal/config/env.go index a156a4b..1759f8b 100644 --- a/internal/config/env.go +++ b/internal/config/env.go @@ -117,6 +117,10 @@ func Resolve(h *Harness, getenv func(string) string) (*Harness, error) { if _, err := s.Inference.TimeoutSecs(); err != nil { errs = append(errs, fmt.Sprintf("spec.inference.timeout: %v", err)) } + if v := h.Spec.Inference.Verify; v != nil { + b := *v // copy so the resolved struct never aliases the input's *bool + s.Inference.Verify = &b + } s.Sandbox.Image = exp("spec.sandbox.image", h.Spec.Sandbox.Image) if p := h.Spec.Sandbox.Policy; p != nil { diff --git a/internal/config/env_test.go b/internal/config/env_test.go index fb5cda8..6f59ddf 100644 --- a/internal/config/env_test.go +++ b/internal/config/env_test.go @@ -159,6 +159,35 @@ func TestResolveValidTimeout(t *testing.T) { } } +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. + src := `apiVersion: harness.openshell.dev/v1alpha1 +kind: Harness +metadata: + name: test +spec: + inference: + provider: gcp + model: claude-opus-4-8 + verify: false +` + h, err := Parse([]byte(src)) + if err != nil { + t.Fatalf("Parse: %v", err) + } + resolved, err := Resolve(h, func(string) string { return "" }) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if resolved.Spec.Inference.VerifyEnabled() { + t.Error("verify:false should resolve to VerifyEnabled()==false") + } + if resolved.Spec.Inference.Verify == h.Spec.Inference.Verify { + t.Error("resolved Verify aliases the input's *bool pointer") + } +} + func TestResolveNonSecretField(t *testing.T) { // Build Harness with ${SECRET_ISH} in non-secret field h := &Harness{ diff --git a/internal/plan/plan.go b/internal/plan/plan.go index b4748bd..174c41f 100644 --- a/internal/plan/plan.go +++ b/internal/plan/plan.go @@ -200,13 +200,14 @@ func InferenceAction(desired config.Inference, cur InferenceState) Action { if !cur.Present { return ActionCreate } - // An unset desired timeout means "let the gateway apply its default" (0 == - // don't care), so it must not force an update against whatever nonzero - // default the gateway reports back. Only an explicitly configured timeout - // participates in the diff. "" is the sole "don't care" marker — "0s" parses - // to 0 too but is an explicit choice. + // A desired timeout of 0 seconds means "let the gateway apply its default", + // so it must not force an update against whatever nonzero default the gateway + // reports back. Both "" (unset) and "0s" resolve to 0 here, and neither can + // ever be a stored gateway value (writing 0 stores the gateway's nonzero + // default), so both are treated as "don't care" — guarding on the value, not + // the string, is what keeps "0s" from churning an update forever. desiredSecs, _ := desired.TimeoutSecs() // Resolve-validated; error → 0 - timeoutDiffers := desired.Timeout != "" && desiredSecs != cur.TimeoutSecs + timeoutDiffers := desiredSecs != 0 && desiredSecs != cur.TimeoutSecs if desired.Provider != cur.Provider || desired.Model != cur.Model || timeoutDiffers { diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go index 6b2512a..7d3b563 100644 --- a/internal/plan/plan_test.go +++ b/internal/plan/plan_test.go @@ -376,6 +376,18 @@ func TestInferenceAction_UnsetTimeoutIgnoresGatewayDefault(t *testing.T) { } } +// TestInferenceAction_ZeroSecondsTimeoutIsDontCare pins the whole-spec finding: +// "0s" resolves to 0 seconds, which the gateway can never store (0 => default), +// so it must be treated as "don't care" just like "" — otherwise it churns an +// update forever against the gateway's nonzero default. +func TestInferenceAction_ZeroSecondsTimeoutIsDontCare(t *testing.T) { + desired := config.Inference{Provider: "gcp", Model: "claude-opus-4-8", Timeout: "0s"} + cur := InferenceState{Capable: true, Present: true, Provider: "gcp", Model: "claude-opus-4-8", TimeoutSecs: 60} + if got := InferenceAction(desired, cur); got != ActionNoop { + t.Errorf("InferenceAction = %s, want noop (\"0s\" must not chase the gateway default)", got) + } +} + // TestInferenceAction_ExplicitTimeoutStillDiffs guards that the finding-1 fix did // not neuter the timeout diff: an explicitly configured timeout still updates // against a mismatched gateway value. diff --git a/internal/reconcile/inference.go b/internal/reconcile/inference.go index d056025..0b61212 100644 --- a/internal/reconcile/inference.go +++ b/internal/reconcile/inference.go @@ -32,7 +32,10 @@ type InferenceResult struct { // returned, so the caller learns the write did not happen. // // Precondition: desired must be Resolve-validated (config.Resolve) so its timeout -// parses; ReconcileInference re-parses defensively and errors if it does not. +// parses; ReconcileInference re-parses defensively and errors if it does not. It +// does not gate on inference being configured (that is the plan's job via +// isInferenceConfigured) — a caller that passes an empty desired will attempt a +// create with empty provider/model and get openshell.ErrInvalidArgument. // // Verify is not part of the diff (the gateway does not report validation intent), // so a config that only flips verify with provider/model/timeout unchanged yields diff --git a/internal/reconcile/inference_test.go b/internal/reconcile/inference_test.go index 0d743df..6969ae7 100644 --- a/internal/reconcile/inference_test.go +++ b/internal/reconcile/inference_test.go @@ -203,6 +203,48 @@ func TestReconcileInference_WriteErrorPropagates(t *testing.T) { } } +// TestReconcileMatchesPlanAction locks the feature-level invariant that the +// read-only plan and the reconcile write agree on the action for the same gateway +// state (both route through plan.InferenceAction). Guards against future drift. +func TestReconcileMatchesPlanAction(t *testing.T) { + ctx := context.Background() + desired := config.Inference{Provider: "gcp", Model: "claude-opus-4-8"} + + cases := []struct { + name string + seed *openshell.InferenceRouteConfig // nil = no route + want plan.Action + }{ + {name: "absent -> create", seed: nil, want: plan.ActionCreate}, + {name: "matching -> noop", seed: &openshell.InferenceRouteConfig{Provider: "gcp", Model: "claude-opus-4-8", Route: plan.DefaultInferenceRoute}, want: plan.ActionNoop}, + {name: "stale -> update", seed: &openshell.InferenceRouteConfig{Provider: "gcp", Model: "claude-sonnet-5", Route: plan.DefaultInferenceRoute}, want: plan.ActionUpdate}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c, _ := healthyClient(t) + if tc.seed != nil { + if _, err := c.SetInferenceRoute(ctx, *tc.seed); err != nil { + t.Fatalf("seed: %v", err) + } + } + // Plan action from the read-only path. + cur, err := plan.ReadInferenceState(ctx, c, desired) + if err != nil { + t.Fatalf("ReadInferenceState: %v", err) + } + planAction := plan.InferenceAction(desired, cur) + // Reconcile action from the write path against the same state. + res, err := ReconcileInference(ctx, c, desired) + if err != nil { + t.Fatalf("ReconcileInference: %v", err) + } + if planAction != tc.want || res.Action != tc.want { + t.Errorf("plan=%s reconcile=%s, want %s", planAction, res.Action, tc.want) + } + }) + } +} + // capturingClient records the last SetInferenceRoute config while delegating to // a real fake-backed client, so the outbound mapping can be asserted. type capturingClient struct { From f623d954183f062a7142f26b0b12878fa423e6fa Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Tue, 25 Aug 2026 13:52:19 -0700 Subject: [PATCH 5/8] openshell,test: live-validate PR4b inference reconcile on 0.0.110 OCP Deployed OpenShell 0.0.110 to a real OpenShift gateway and drove the shipped reconcile engine against it (see the validate skill). Confirmed: the mTLS identity holds the workspace admin role (S1 risk retired), inference.local is the gateway default route, and create -> noop -> update -> delete all behave as designed. Two fixes the validation surfaced: - profiles/gateways/openshift.yaml: chart 0.0.85 -> 0.0.110. The 0.0.85 gateway returns Unimplemented on the inference gRPC; the profile was never bumped in the version re-baseline. Now in lockstep with .openshell-version. - inference_e2e_test.go: rewrite TestLiveInferenceRoleProbe. A real gateway accepts only a fixed route set and checks Set preconditions (route name, provider, credential) before the role, so the original scratch-route probe could never pass live. Read path runs always; the admin-write path is gated on HARNESS_E2E_INFERENCE_PROVIDER and restores pre-probe state. --- .../openshell/sdkclient/inference_e2e_test.go | 76 +++++++++++++------ profiles/gateways/openshift.yaml | 5 +- 2 files changed, 58 insertions(+), 23 deletions(-) diff --git a/internal/openshell/sdkclient/inference_e2e_test.go b/internal/openshell/sdkclient/inference_e2e_test.go index 60905ec..fb260b3 100644 --- a/internal/openshell/sdkclient/inference_e2e_test.go +++ b/internal/openshell/sdkclient/inference_e2e_test.go @@ -11,16 +11,29 @@ import ( "github.com/stackrox/harness-openshell/internal/openshell/sdkclient" ) -// TestLiveInferenceRoleProbe probes whether the harness mTLS identity holds the -// workspace "admin" role required to write inference routes. This is the S1 risk -// gate for PR4b: SetInferenceRoute/DeleteInferenceRoute require admin, while -// GetInferenceRoute only needs the user role. Slice 3's reconcile-write cannot -// succeed on a real gateway if the identity lacks admin. +// defaultRoute mirrors plan.DefaultInferenceRoute. It is duplicated here (not +// imported) to keep the firewall's e2e test from depending on the plan package. +// A real 0.0.110 gateway accepts only a fixed set of route names — +// "inference.local" and "sandbox-system" — and rejects any other with +// InvalidArgument (verified live 2026-08-25); the harness only ever uses +// "inference.local". +const defaultRoute = "inference.local" + +// TestLiveInferenceRoleProbe verifies the harness mTLS identity can serve the +// inference surface reconcile depends on: the user-role read path always, and — +// when a credentialed provider is supplied — the admin-role write path. // // It is skipped unless HARNESS_E2E_GATEWAY names a registered mTLS gateway (the // same gate as the other live checks). Optional HARNESS_E2E_WORKSPACE overrides -// the workspace. The probe uses a SCRATCH route name and cleans it up; it never -// touches the default route. +// the workspace. +// +// The admin-role write is the S1 risk gate for PR4b: SetInferenceRoute requires +// the workspace "admin" role, GetInferenceRoute only "user". But a live gateway +// checks Set's preconditions BEFORE the role — the route name must be valid, the +// provider must exist in the workspace, and it must carry a usable credential — +// so the role can only be probed once a credentialed provider exists. Supply its +// name via HARNESS_E2E_INFERENCE_PROVIDER to exercise the write path; without it +// the write probe is skipped (admin was confirmed present on OCP 2026-08-25). // // HARNESS_E2E_GATEWAY=openshell go test ./internal/openshell/sdkclient/ -run LiveInferenceRoleProbe -v func TestLiveInferenceRoleProbe(t *testing.T) { @@ -41,33 +54,52 @@ func TestLiveInferenceRoleProbe(t *testing.T) { } defer c.Close() - const scratch = "harness-probe" + // Read path (user role). On the default route the gateway returns the route + // if configured, or ErrNotFound if not — both mean the identity can read and + // the gateway serves inference. ErrPermission/ErrUnavailable/InvalidArgument + // are all failures of the surface the harness needs. + if _, err := c.GetInferenceRoute(ctx, defaultRoute); err != nil && !errors.Is(err, openshell.ErrNotFound) { + t.Fatalf("GetInferenceRoute(%q) read path failed: %v", defaultRoute, err) + } + t.Logf("read path OK on gateway %q (user role confirmed, gateway serves inference)", gw) - // Read path requires only the user role; ErrNotFound is a success signal - // (the identity can read; the scratch route just doesn't exist yet). - if _, err := c.GetInferenceRoute(ctx, scratch); err != nil && !errors.Is(err, openshell.ErrNotFound) { - t.Fatalf("GetInferenceRoute (user role) failed unexpectedly: %v", err) + provider := os.Getenv("HARNESS_E2E_INFERENCE_PROVIDER") + if provider == "" { + t.Skip("set HARNESS_E2E_INFERENCE_PROVIDER to a registered, credentialed provider to probe the admin write path") } - t.Logf("read path OK on gateway %q (user role confirmed)", gw) - // Write path requires the admin role. Either outcome is a recordable probe - // result; ErrPermission is exactly the risk we are measuring, not a bug. + // Write path (admin role) on the default route. NoVerify skips endpoint + // validation so the probe measures the role, not the provider's credentials. + // Read the current route first so cleanup can restore it (the write bumps + // Version); if it was unconfigured, delete to restore that state. + before, beforeErr := c.GetInferenceRoute(ctx, defaultRoute) + existed := beforeErr == nil + _, setErr := c.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ - Provider: "gcp", - Model: "claude-opus-4-8", - Route: scratch, + Provider: provider, + Model: "probe-model", + Route: defaultRoute, NoVerify: true, }) switch { case setErr == nil: t.Logf("WRITE path OK on gateway %q: identity HAS the workspace admin role", gw) - if delErr := c.DeleteInferenceRoute(ctx, scratch); delErr != nil { - t.Errorf("cleanup DeleteInferenceRoute(%q): %v", scratch, delErr) - } case errors.Is(setErr, openshell.ErrPermission): t.Fatalf("WRITE path DENIED on gateway %q: identity LACKS the workspace admin role "+ - "(PR4b Slice 3 reconcile-write will fail until the mTLS identity is granted admin): %v", gw, setErr) + "(reconcile-write will fail until the mTLS identity is granted admin): %v", gw, setErr) default: t.Fatalf("SetInferenceRoute returned an unexpected error: %v", setErr) } + + // Restore the pre-probe state. + if existed { + if _, err := c.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ + Provider: before.Provider, Model: before.Model, Route: defaultRoute, + NoVerify: true, TimeoutSecs: before.TimeoutSecs, + }); err != nil { + t.Errorf("restoring inference route: %v", err) + } + } else if err := c.DeleteInferenceRoute(ctx, defaultRoute); err != nil { + t.Errorf("cleanup DeleteInferenceRoute(%q): %v", defaultRoute, err) + } } diff --git a/profiles/gateways/openshift.yaml b/profiles/gateways/openshift.yaml index 51a78dd..e4f5bea 100644 --- a/profiles/gateways/openshift.yaml +++ b/profiles/gateways/openshift.yaml @@ -11,7 +11,10 @@ gateway: name: openshell-remote-ocp chart: - version: "0.0.85" + # Keep in lockstep with .openshell-version / gateway.MinOpenShellVersion. A + # chart behind the CLI pin can leave the inference gRPC Unimplemented and skews + # the supervisor image. Verified live at 0.0.110 on OCP 2026-08-25. + version: "0.0.110" helm: values: From 662d59beb6ac88eaf325932314e8a3b369464e96 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Tue, 25 Aug 2026 14:06:27 -0700 Subject: [PATCH 6/8] =?UTF-8?q?fix(config,test):=20address=20PR4b=20review?= =?UTF-8?q?=20=E2=80=94=20require=20provider+model,=20guard=20probe=20read?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - config.Resolve now rejects a configured inference block that omits provider or model, instead of surfacing a late ErrInvalidArgument from the gateway on apply (CodeRabbit, plan.go:337). Mirrors the existing resolve-time timeout validation. - TestLiveInferenceRoleProbe: fail on any non-NotFound pre-write read error so cleanup never deletes a route that only failed to read transiently (CodeRabbit, inference_e2e_test.go:76). --- internal/config/env.go | 14 ++++++++++++++ internal/config/env_test.go | 4 +++- internal/openshell/sdkclient/inference_e2e_test.go | 6 ++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/internal/config/env.go b/internal/config/env.go index 1759f8b..5bf748f 100644 --- a/internal/config/env.go +++ b/internal/config/env.go @@ -117,6 +117,20 @@ func Resolve(h *Harness, getenv func(string) string) (*Harness, error) { if _, err := s.Inference.TimeoutSecs(); err != nil { errs = append(errs, fmt.Sprintf("spec.inference.timeout: %v", err)) } + // A configured inference block must name both a provider and a model: the + // gateway rejects a route write that lacks either, and reconcile has nothing + // to write without them. Catch it here at resolve time with a clear message + // instead of surfacing a late ErrInvalidArgument on apply. Route/timeout alone + // (or verify alone) don't identify a route to reconcile. Kept in step with + // plan.isInferenceConfigured. + if s.Inference.Route != "" || s.Inference.Provider != "" || s.Inference.Model != "" || s.Inference.Timeout != "" { + if s.Inference.Provider == "" { + errs = append(errs, "spec.inference.provider: required when inference is configured") + } + if s.Inference.Model == "" { + errs = append(errs, "spec.inference.model: required when inference is configured") + } + } if v := h.Spec.Inference.Verify; v != nil { b := *v // copy so the resolved struct never aliases the input's *bool s.Inference.Verify = &b diff --git a/internal/config/env_test.go b/internal/config/env_test.go index 6f59ddf..b97abaa 100644 --- a/internal/config/env_test.go +++ b/internal/config/env_test.go @@ -141,7 +141,9 @@ func TestResolveValidTimeout(t *testing.T) { APIVersion: "harness.openshell.dev/v1alpha1", Kind: "Harness", Metadata: Metadata{Name: "test"}, - Spec: Spec{Inference: Inference{Timeout: "${INF_TIMEOUT}"}}, + // Provider+model are required whenever the inference block is configured; + // this test only exercises timeout expansion, so supply them as fixtures. + Spec: Spec{Inference: Inference{Provider: "gcp", Model: "claude-opus-4-8", Timeout: "${INF_TIMEOUT}"}}, } resolved, err := Resolve(h, func(name string) string { diff --git a/internal/openshell/sdkclient/inference_e2e_test.go b/internal/openshell/sdkclient/inference_e2e_test.go index fb260b3..da163b5 100644 --- a/internal/openshell/sdkclient/inference_e2e_test.go +++ b/internal/openshell/sdkclient/inference_e2e_test.go @@ -73,6 +73,12 @@ func TestLiveInferenceRoleProbe(t *testing.T) { // Read the current route first so cleanup can restore it (the write bumps // Version); if it was unconfigured, delete to restore that state. before, beforeErr := c.GetInferenceRoute(ctx, defaultRoute) + // Only ErrNotFound means "no route to restore"; any other read error is a real + // failure. Treating it as absent would make cleanup delete a route that was + // actually there (the read merely failed transiently) after the write succeeds. + if beforeErr != nil && !errors.Is(beforeErr, openshell.ErrNotFound) { + t.Fatalf("GetInferenceRoute(%q) pre-write read failed: %v", defaultRoute, beforeErr) + } existed := beforeErr == nil _, setErr := c.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ From 65fc60532a747ac4ab5b3d0771e9cf0d3aca6956 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Tue, 25 Aug 2026 14:14:31 -0700 Subject: [PATCH 7/8] test(e2e): restore inference route via t.Cleanup on every write outcome MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SetInferenceRoute persists at the gateway before its gRPC response returns, so a write reporting a transport/unexpected error may still have changed state. The previous t.Fatalf in that path skipped restoration, potentially leaving inference.local pointing at probe-model. Register restoration with t.Cleanup before the write (fresh context, since ctx may be spent), skipping it only on a pre-write permission denial — where no write applied and the identity lacks the admin role the restore itself needs (CodeRabbit). --- .../openshell/sdkclient/inference_e2e_test.go | 40 +++++++++++++------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/internal/openshell/sdkclient/inference_e2e_test.go b/internal/openshell/sdkclient/inference_e2e_test.go index da163b5..b0a37be 100644 --- a/internal/openshell/sdkclient/inference_e2e_test.go +++ b/internal/openshell/sdkclient/inference_e2e_test.go @@ -81,6 +81,33 @@ func TestLiveInferenceRoleProbe(t *testing.T) { } existed := beforeErr == nil + // Register restoration BEFORE the write. SetInferenceRoute persists the route + // at the gateway before its gRPC response returns, so a write that reports a + // transport/unexpected error may still have changed state; t.Cleanup runs on + // every exit path (including t.Fatalf) so the probe never leaves inference.local + // pointing at probe-model. It uses a fresh context because ctx may be spent by + // the time cleanup runs. It is skipped only on a pre-write permission denial: + // then no write applied and the identity lacks the admin role the restore + // itself would need, so attempting it would just log a spurious error. + permissionDenied := false + t.Cleanup(func() { + if permissionDenied { + return + } + cctx, ccancel := context.WithTimeout(context.Background(), 30*time.Second) + defer ccancel() + if existed { + if _, err := c.SetInferenceRoute(cctx, openshell.InferenceRouteConfig{ + Provider: before.Provider, Model: before.Model, Route: defaultRoute, + NoVerify: true, TimeoutSecs: before.TimeoutSecs, + }); err != nil { + t.Errorf("restoring inference route: %v", err) + } + } else if err := c.DeleteInferenceRoute(cctx, defaultRoute); err != nil { + t.Errorf("cleanup DeleteInferenceRoute(%q): %v", defaultRoute, err) + } + }) + _, setErr := c.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ Provider: provider, Model: "probe-model", @@ -91,21 +118,10 @@ func TestLiveInferenceRoleProbe(t *testing.T) { case setErr == nil: t.Logf("WRITE path OK on gateway %q: identity HAS the workspace admin role", gw) case errors.Is(setErr, openshell.ErrPermission): + permissionDenied = true t.Fatalf("WRITE path DENIED on gateway %q: identity LACKS the workspace admin role "+ "(reconcile-write will fail until the mTLS identity is granted admin): %v", gw, setErr) default: t.Fatalf("SetInferenceRoute returned an unexpected error: %v", setErr) } - - // Restore the pre-probe state. - if existed { - if _, err := c.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ - Provider: before.Provider, Model: before.Model, Route: defaultRoute, - NoVerify: true, TimeoutSecs: before.TimeoutSecs, - }); err != nil { - t.Errorf("restoring inference route: %v", err) - } - } else if err := c.DeleteInferenceRoute(ctx, defaultRoute); err != nil { - t.Errorf("cleanup DeleteInferenceRoute(%q): %v", defaultRoute, err) - } } From 38af3655cdaf48b49817b0a0b4078de36785752d Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Tue, 25 Aug 2026 14:22:20 -0700 Subject: [PATCH 8/8] test(e2e): close client via t.Cleanup so restoration runs first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit defer c.Close() runs during the test's goexit unwinding, before t.Cleanup callbacks, so it would shut the client's gRPC connection before the route-restoration cleanup could call Set/DeleteInferenceRoute. Register Close with t.Cleanup instead: registered first, LIFO ordering runs it last — after restoration (CodeRabbit). --- internal/openshell/sdkclient/inference_e2e_test.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/internal/openshell/sdkclient/inference_e2e_test.go b/internal/openshell/sdkclient/inference_e2e_test.go index b0a37be..a62de94 100644 --- a/internal/openshell/sdkclient/inference_e2e_test.go +++ b/internal/openshell/sdkclient/inference_e2e_test.go @@ -52,7 +52,15 @@ func TestLiveInferenceRoleProbe(t *testing.T) { if err != nil { t.Fatalf("sdkclient.New(%q): %v", gw, err) } - defer c.Close() + // Close via t.Cleanup, not defer: t.Cleanup callbacks run in LIFO order after + // the test's deferred calls, so a deferred Close would shut the client's gRPC + // connection before the route-restoration cleanup registered below could use + // it. Registered first here, it runs last — after restoration. + t.Cleanup(func() { + if err := c.Close(); err != nil { + t.Errorf("closing client: %v", err) + } + }) // Read path (user role). On the default route the gateway returns the route // if configured, or ErrNotFound if not — both mean the identity can read and