From 6163c88c08222178533b44d2597aef1a4c0f44d8 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 9 Sep 2026 12:15:37 -0700 Subject: [PATCH 1/8] refactor: remove provider management from workflow runner --- .gitignore | 4 + README.md | 45 ++- cmd/apply.go | 2 +- cmd/apply_service.go | 5 +- cmd/delete.go | 161 ++------- cmd/delete_test.go | 148 +++++---- cmd/plan_test.go | 12 +- cmd/workflow_apply.go | 53 +-- cmd/workflow_apply_test.go | 28 +- docs/README.md | 1 + internal/config/env.go | 12 +- internal/config/env_test.go | 42 --- .../config/testdata/fact-dev.v1alpha1.yaml | 5 +- internal/config/types.go | 16 +- internal/openshell/client.go | 16 - internal/openshell/sdkclient/client.go | 1 - internal/openshell/sdkclient/provider.go | 76 +---- .../openshell/sdkclient/provider_e2e_test.go | 185 ----------- internal/openshell/sdkclient/provider_test.go | 189 ++--------- internal/openshell/types.go | 15 +- internal/plan/ownership.go | 22 -- internal/plan/plan.go | 101 +----- internal/plan/plan_test.go | 260 +-------------- internal/plan/render_test.go | 4 +- internal/plan/state_test.go | 16 - internal/reconcile/provider.go | 127 ------- internal/reconcile/provider_test.go | 314 ------------------ internal/status/status.go | 8 - main.go | 2 +- test/configs/harness-v1alpha1.yaml | 2 +- 30 files changed, 270 insertions(+), 1602 deletions(-) delete mode 100644 internal/openshell/sdkclient/provider_e2e_test.go delete mode 100644 internal/plan/ownership.go delete mode 100644 internal/reconcile/provider.go delete mode 100644 internal/reconcile/provider_test.go diff --git a/.gitignore b/.gitignore index 6e185c8..98eaf85 100644 --- a/.gitignore +++ b/.gitignore @@ -32,3 +32,7 @@ harness.yaml test.yaml reviewer.yaml skills-lock.json + +# Local audit working notes +docs/audit-*.md +docs/code-audit.md diff --git a/README.md b/README.md index 94b0efb..7765995 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,8 @@ > **Experimental.** Built on [OpenShell](https://github.com/NVIDIA/OpenShell), which is itself alpha software. Expect breaking changes in both. -Declarative workflow layer for OpenShell AI agent sandboxes. +Run workflows in OpenShell AI agent sandboxes. The current focus is portable +PR review with trusted, repository-controlled skills. ## Quick Start @@ -79,11 +80,22 @@ The command writes results to stdout. For retained sandboxes, use ## Why this exists -[OpenShell](https://github.com/NVIDIA/OpenShell) provides a strict, secure sandbox runtime — deny-by-default L7 network policy, credential proxying, Landlock filesystem isolation, and inference routing. It also provisions the gateway itself (the local installer, or `helm install openshell` on a cluster). What it doesn't provide is the developer workflow layer on top: the config that wires up providers, the declarative reconciliation that makes a gateway match your intent, or the CI harness that catches breakage before developers hit it. +[OpenShell](https://github.com/NVIDIA/OpenShell) owns gateway provisioning, +sandbox isolation, credential proxying, provider lifecycle, and network policy. +Harness prepares workflow inputs, stages source and skills, runs an agent, and +reports the result while cleaning up its sandbox. -Without a shared harness layer, every team building on OpenShell independently solves the same problems — writing shell scripts to register providers, hand-rolling container images, re-deriving inference routing. The configs diverge, the security posture varies, and nobody catches regressions until something breaks in production. +The next portability milestone is running the same PR-review package in a +second repository with that repository's trusted skill. Users should customize +review behavior through skills; maintained integrations and platform setup +should supply provider credentials and native OpenShell policy. The current +review example still has repository-local orchestration and requires credential +wiring improvements before it meets that goal. -**The design boundary**: managing a gateway is OpenShell's problem; the harness is a declarative setup/run layer with zero compute-backend opinion. It never provisions or tears down a gateway — it declares providers, inference, and policy against one OpenShell already stood up, and runs agents in it. The workflow remains portable because its target can be overridden by standard gateway and workspace flags or environment variables. +Workflows target a local OpenShell gateway or a configured HyperShell gateway. +Provider references are read-only. Inference reconciliation remains supported +while existing callers migrate to platform-configured routes. See the +[code audit](docs/code-audit.md) for the dependency inventory and remaining cuts. **The core design constraint**: if the developer harness isn't running and live-tested in CI, the developer experience can't be maintained. OpenShell, agent CLIs, and provider APIs all change frequently — often multiple times per week. A harness that works today and isn't continuously validated will silently break. CI exercises the workflow against local and Kind gateways on Linux. OpenShift remains a manually credentialed integration target. @@ -130,14 +142,14 @@ spec: `plan` is read-only and may render desired state while the gateway is offline. `apply` requires the effective gateway to be reachable, verifies referenced providers before sandbox creation, and disables OpenShell provider auto-discovery. -Managed providers may be updated or explicitly adopted, but apply does not create -credentialed providers; platform bootstrap owns their creation. Relative payload +Providers are read-only references; OpenShell/platform bootstrap owns their +creation, updates, and deletion. Relative payload and policy paths resolve from the workflow file's directory. Workflow schema essentials: -- `spec.providers` declares provider resources; `spec.sandbox.providers` attaches provider capabilities to the sandbox runtime. -- `management: referenced` requires an already-registered provider; managed providers can set `adopt: true` to take ownership of a pre-existing provider. +- `spec.providers` verifies existing provider references; `spec.sandbox.providers` attaches provider capabilities to the sandbox runtime. +- `management: referenced` is optional and is the only supported management mode. Provider configuration belongs in OpenShell/platform bootstrap. - `spec.inference.verify: true` enforces inference-route endpoint checks during inference route writes. - `spec.source.repo` is cloned outside the sandbox and uploaded; `spec.payloads[*].source` and `spec.sandbox.policy.file` resolve relative to the workflow file. - Pin `spec.source.ref` to a full commit SHA for repeatable source inputs. Branches and tags resolve at preparation time; an omitted ref uses remote HEAD. Apply reports the actual prepared commit from the host checkout, including the commit behind an annotated tag. Missing refs fail instead of falling back to HEAD. This identifies the initial checkout, not later agent edits or payload overlays, and is not yet a structured run-result artifact. @@ -189,7 +201,7 @@ review artifact bundle. (OpenShell has already provisioned the gateway; you selected it) harness apply -f config.yaml | - +-> Verify/reconcile declared providers and inference + +-> Verify provider references and configure declared inference +-> Create sandbox (isolated container, deny-by-default network) +-> Upload payloads (CLAUDE.md, MCP config, skills) +-> Run task (agent executes, outputs results) @@ -212,7 +224,7 @@ openshell term # interactive policy terminal - OpenShell CLI and gateway service at the repo-pinned version (see `make openshell` and `.openshell-version`). - An active OpenShell gateway registration (`openshell gateway add ...`, `openshell gateway select ...`). -- Provider credentials already reconciled on the gateway for any referenced providers. +- Providers already configured on the gateway for any references. ## Install @@ -263,8 +275,13 @@ harness apply -f harness.yaml # same YAML, cluster gateway Tear the gateway down with `helm uninstall openshell` and `openshell gateway remove my-cluster`. The harness `delete` command removes -sandboxes; add `--providers` (or `--all`) to remove providers too. It never -removes the gateway. +sandboxes only. Use `openshell provider delete` to remove providers and upstream +tools to remove the gateway. + +Provider-management migration: `management: managed`, provider `adopt`/`config`, +and `harness delete --providers`/`--all` are removed. Configure providers with +OpenShell and reference their names in workflows. Use `delete --sandboxes` only +for a dedicated workspace, or delete individual sandbox names. > **Migration:** `harness deploy`, `harness teardown`, `harness status`, and > `delete --k8s` are removed. Provision the gateway with OpenShell (the @@ -281,13 +298,13 @@ removes the gateway. | `harness doctor` | Validate gateway reachability and referenced providers | | `harness apply -f FILE` | Deploy a sandbox from config | | `harness apply -f FILE --attach` | Interactive TTY mode | -| `harness apply -f FILE --setup-only` | Reconcile providers and inference only (skip sandbox run) | +| `harness apply -f FILE --setup-only` | Verify provider references and configure inference (skip sandbox run) | | `harness apply -f FILE --dry-run` | Render the v1alpha1 action plan without mutating | | `harness apply -f FILE -o yaml` | Output resolved config with interpolated and credential-bearing map values redacted | | `harness get gateways` | Show active gateway only (name, endpoint, status, version) | | `harness get agents\|providers` | List resources | | `harness describe ` | Sandbox details | -| `harness delete [--all\|--sandboxes\|--providers]` | Delete targeted or bulk resources | +| `harness delete ` / `harness delete --sandboxes` | Delete named sandboxes or all sandboxes in the selected workspace | | `harness plan -f FILE` | Read-only reconciliation plan (mutates nothing) | ### Credentials diff --git a/cmd/apply.go b/cmd/apply.go index 5b9221e..24c6ddb 100644 --- a/cmd/apply.go +++ b/cmd/apply.go @@ -43,7 +43,7 @@ host-interpolated and credential-bearing map values redacted.`, cmd.Flags().StringVar(&entrypoint, "entrypoint", "", "Override the agent executable") cmd.Flags().BoolVar(&attach, "attach", false, "Attach a TTY for interactive execution") cmd.Flags().BoolVar(&dryRun, "dry-run", false, "Render the action plan without mutating anything") - cmd.Flags().BoolVar(&setupOnly, "setup-only", false, "Reconcile providers and inference without running a sandbox") + cmd.Flags().BoolVar(&setupOnly, "setup-only", false, "Verify provider references and configure inference without running a sandbox") cmd.Flags().StringVarP(&output, "output", "o", "", "Output format: yaml or json (dry-run also supports table)") cmd.Flags().StringVar(&resultFile, "result-file", "", "Write host-derived execution result JSON to a new file") gatewayName, workspace = registerTargetFlags(cmd) diff --git a/cmd/apply_service.go b/cmd/apply_service.go index 3615351..fb1fed4 100644 --- a/cmd/apply_service.go +++ b/cmd/apply_service.go @@ -124,7 +124,7 @@ func executeResolvedWorkflow(ctx context.Context, workflow *resolvedWorkflow, p if client == nil || !current.Reachable { return fmt.Errorf("%s is not reachable or authenticated", targetDescription(workflow.Target)) } - if err := preflightPlan(workflow.Desired, p); err != nil { + if err := preflightPlan(p); err != nil { return err } if err := verifySandboxProviders(ctx, client, workflow.Desired); err != nil { @@ -149,9 +149,6 @@ func executeResolvedWorkflow(ctx context.Context, workflow *resolvedWorkflow, p } opts.Result.setPhase("reconcile") - if err := reconcileProviders(ctx, client, workflow.Desired.Spec.Providers); err != nil { - return err - } if inferenceConfigured(workflow.Desired.Spec.Inference) { result, err := reconcile.ReconcileInference(ctx, client, workflow.Desired.Spec.Inference) if err != nil { diff --git a/cmd/delete.go b/cmd/delete.go index 7479c46..24e7d7b 100644 --- a/cmd/delete.go +++ b/cmd/delete.go @@ -2,178 +2,71 @@ package cmd import ( "context" + "errors" "fmt" "os" - "time" "github.com/spf13/cobra" "github.com/stackrox/harness-openshell/internal/openshell" - "github.com/stackrox/harness-openshell/internal/status" ) func NewDeleteCmd(newClient openshell.Factory) *cobra.Command { - var ( - all bool - sandboxes bool - providers bool - ) + var sandboxes bool var gatewayName, workspace *string cmd := &cobra.Command{ - Use: "delete [NAME...] [--all] [--sandboxes] [--providers]", - Short: "Delete sandboxes or providers", - Long: `Delete specific sandboxes by name, or use flags for bulk operations. + Use: "delete [NAME...] [--sandboxes]", + Short: "Delete sandboxes", + Long: `Delete specific sandboxes by name, or use --sandboxes to delete all +sandboxes in the selected workspace. Examples: - harness delete my-sandbox Delete a specific sandbox - harness delete agent test Delete multiple sandboxes - harness delete --all Delete all sandboxes and providers - harness delete --sandboxes Delete all sandboxes - harness delete --providers Delete all providers (no running sandboxes allowed)`, + harness delete my-sandbox Delete a specific sandbox + harness delete agent test Delete multiple sandboxes + harness delete --sandboxes Delete all sandboxes`, RunE: func(cmd *cobra.Command, args []string) error { - if len(args) == 0 && !all && !sandboxes && !providers { - return fmt.Errorf("specify sandbox name(s) or use --all, --sandboxes, --providers") + if len(args) == 0 && !sandboxes { + return fmt.Errorf("specify sandbox name(s) or use --sandboxes") + } + if len(args) > 0 && sandboxes { + return fmt.Errorf("sandbox names cannot be combined with --sandboxes") } ctx := cmd.Context() target := openshell.ResolveTarget(*gatewayName, *workspace, "", "", os.Getenv) - // When neither --gateway nor $OPENSHELL_GATEWAY pins a gateway, the - // SDK resolves the active gateway from openshell config - // (gateway.LoadConfig("") in sdkclient.New), surfacing - // ErrNoActiveGateway if none is selected — the same selection apply - // runs against. client, err := newClient(ctx, target) if err != nil { return fmt.Errorf("create OpenShell client: %w", err) } defer client.Close() - // Targeted sandbox deletion - if len(args) > 0 { - for _, name := range args { - if err := client.DeleteSandbox(ctx, name); err != nil { - status.Failf("%s: %v", name, err) - } else { - status.OKf("Deleted sandbox %s", name) - } - } - if !all && !providers { - return nil - } - } - - // Banner the gateway we're sweeping. target.Gateway is empty when - // resolved from the active-gateway marker by the SDK, so ask the - // client for the resolved name (GatewayInfo carries what - // LoadConfig resolved). Best-effort: a banner must never block the - // sweep, so fall back to the pinned name on any error. - if gwName := resolvedGatewayName(ctx, client, target); gwName != "" { - status.Infof("Active gateway: %s", gwName) - fmt.Println() - } - - if all || sandboxes { - deleteSandboxesSDK(ctx, client) - } - if all || providers { - if err := deleteProvidersSDK(ctx, client); err != nil { - return err - } - } - - status.Done("Done.") - return nil + return deleteSandboxes(ctx, client, args, sandboxes) }, } - cmd.Flags().BoolVar(&all, "all", false, "Delete all sandboxes and providers") cmd.Flags().BoolVar(&sandboxes, "sandboxes", false, "Delete all sandboxes") - cmd.Flags().BoolVar(&providers, "providers", false, "Delete all providers") gatewayName, workspace = registerTargetFlags(cmd) return cmd } -// resolvedGatewayName reports the gateway name to banner. When --gateway or -// $OPENSHELL_GATEWAY pinned one, target.Gateway holds it directly; otherwise the -// SDK resolved the active gateway and only the client knows the resolved name -// (via GatewayInfo). It is best-effort — the banner is cosmetic, so a failed -// GatewayInfo lookup yields "" and the caller simply skips the banner. A lookup -// failure is surfaced as a warning rather than swallowed, so it stays -// distinguishable from a gateway that legitimately reports no name. -func resolvedGatewayName(ctx context.Context, client openshell.Client, target openshell.Target) string { - if target.Gateway != "" { - return target.Gateway - } - info, err := client.GatewayInfo(ctx) - if err != nil { - status.Warnf("could not resolve active gateway name for banner: %v", err) - return "" - } - return info.Name -} - -// deleteSandboxesSDK sweeps every sandbox in the target workspace over the -// OpenShell SDK. It is the sole owner of the bulk sandbox sweep. The caller has -// already resolved a non-empty gateway, so there is no no-gateway case here. -func deleteSandboxesSDK(ctx context.Context, client openshell.Client) { - status.Section("Sandboxes") - sandboxes, err := client.Sandboxes(ctx) - if err != nil { - status.Fail(fmt.Sprintf("could not list sandboxes: %v", err)) - fmt.Println() - return - } - if len(sandboxes) == 0 { - status.Info("None running") - } else { - for _, s := range sandboxes { - status.Infof("Deleting %s", s.Name) - if err := client.DeleteSandbox(ctx, s.Name); err != nil { - status.Failf("failed to delete %s: %v", s.Name, err) - } - } - } - fmt.Println() -} - -// deleteProvidersSDK sweeps every provider over the SDK. Providers are refused -// while any sandbox is still up, with one brief retry to absorb a mid-deletion -// race. It is the sole owner of the bulk provider sweep. The caller has already -// resolved a non-empty gateway, so there is no no-gateway case here. -func deleteProvidersSDK(ctx context.Context, client openshell.Client) error { - status.Section("Providers") - remaining, err := client.Sandboxes(ctx) - if err != nil { - return fmt.Errorf("could not check for running sandboxes: %w", err) - } - if len(remaining) > 0 { - // Sandbox may be mid-deletion — wait briefly and retry. - time.Sleep(2 * time.Second) - remaining, err = client.Sandboxes(ctx) +func deleteSandboxes(ctx context.Context, client openshell.Client, names []string, all bool) error { + if all { + sandboxes, err := client.Sandboxes(ctx) if err != nil { - return fmt.Errorf("rechecking sandboxes: %w", err) + return fmt.Errorf("listing sandboxes: %w", err) } - if len(remaining) > 0 { - return fmt.Errorf("cannot delete providers with running sandboxes — run: harness delete --sandboxes") + names = make([]string, len(sandboxes)) + for i, sandbox := range sandboxes { + names[i] = sandbox.Name } } - providers, err := client.Providers(ctx) - if err != nil { - return fmt.Errorf("could not list providers: %w", err) - } - if len(providers) == 0 { - status.Info("None registered") - } else { - for _, p := range providers { - status.Infof("Deleting %s", p.Name) - if err := client.DeleteProvider(ctx, p.Name); err != nil { - status.Failf("failed to delete %s: %v", p.Name, err) - } + var errs []error + for _, name := range names { + if err := client.DeleteSandbox(ctx, name); err != nil { + errs = append(errs, fmt.Errorf("deleting sandbox %q: %w", name, err)) } } - - fmt.Println() - return nil + return errors.Join(errs...) } diff --git a/cmd/delete_test.go b/cmd/delete_test.go index 294959e..8d71535 100644 --- a/cmd/delete_test.go +++ b/cmd/delete_test.go @@ -2,6 +2,8 @@ package cmd import ( "context" + "errors" + "strings" "testing" "github.com/NVIDIA/OpenShell/sdk/go/openshell/v1/types" @@ -18,9 +20,8 @@ func keepOpenFactory(client openshell.Client) openshell.Factory { return testutil.FakeFactory(noCloseClient{client}) } -// The delete tests use keepOpenFactory (executor_inference_test.go) so the -// command's deferred Close doesn't shut the shared fake before the test can -// assert the resources were actually removed, not merely that a log line printed. +// The delete tests use keepOpenFactory so the command's deferred Close doesn't +// shut the shared fake before the test can inspect the resulting resources. func sandboxNames(t *testing.T, c openshell.Client) []string { t.Helper() @@ -65,6 +66,22 @@ func TestDeleteTargeted(t *testing.T) { } } +func TestDeleteTargetedContinuesAfterFailure(t *testing.T) { + base, fc := testutil.NewFakeClient("default") + fc.AddSandbox("default", &types.Sandbox{Name: "agent-a", Status: types.SandboxStatus{Phase: types.SandboxReady}}) + client := &deleteErrorClient{Client: base, name: "missing", err: errors.New("sandbox missing")} + + cmd := NewDeleteCmd(keepOpenFactory(client)) + cmd.SetArgs([]string{"missing", "agent-a", "--gateway", "prod"}) + _, err := captureStdout(t, cmd.Execute) + if err == nil || !strings.Contains(err.Error(), `deleting sandbox "missing"`) { + t.Fatalf("targeted delete should report the missing sandbox: %v", err) + } + if names := sandboxNames(t, client); len(names) != 0 { + t.Errorf("targeted deletion should continue after a failure, got %v", names) + } +} + func TestDeleteSandboxesSweep(t *testing.T) { client, fc := testutil.NewFakeClient("default") fc.AddSandbox("default", &types.Sandbox{Name: "agent-a", Status: types.SandboxStatus{Phase: types.SandboxReady}}) @@ -81,64 +98,94 @@ func TestDeleteSandboxesSweep(t *testing.T) { } } -func TestDeleteProvidersGuard(t *testing.T) { +func TestDeleteSandboxesLeavesProvidersUntouched(t *testing.T) { client, fc := testutil.NewFakeClient("default") - fc.AddSandbox("default", &types.Sandbox{Name: "agent-a", Status: types.SandboxStatus{Phase: types.SandboxReady}}) fc.AddProvider("default", &types.Provider{Name: "github", Type: "github"}) + fc.AddSandbox("default", &types.Sandbox{Name: "agent-a", Status: types.SandboxStatus{Phase: types.SandboxReady}}) cmd := NewDeleteCmd(keepOpenFactory(client)) - cmd.SetArgs([]string{"--providers", "--gateway", "prod"}) - _, err := captureStdout(t, cmd.Execute) - if err == nil { - t.Fatal("deleting providers with a running sandbox should be refused") - } - if !contains(err.Error(), "running sandboxes") { - t.Errorf("unexpected guard error: %v", err) + cmd.SetArgs([]string{"--sandboxes", "--gateway", "prod"}) + if _, err := captureStdout(t, cmd.Execute); err != nil { + t.Fatalf("delete --sandboxes: %v", err) } - - // The guard must prevent deletion, not delete-then-error: the provider survives. if names := providerNames(t, client); len(names) != 1 || names[0] != "github" { - t.Errorf("guard should leave the provider untouched, got %v", names) + t.Errorf("sandbox deletion should leave providers untouched, got %v", names) } } -// Bulk deletion with no gateway resolvable must fail loudly rather than skip -// both sweeps and report success — otherwise sandboxes/providers silently -// survive. With no --gateway/$OPENSHELL_GATEWAY the SDK resolves the active -// gateway (gateway.LoadConfig("")); when none is selected the Factory returns an -// error, which delete must propagate before sweeping anything. -func TestDeleteBulkNoGatewayErrors(t *testing.T) { - t.Setenv("OPENSHELL_GATEWAY", "") // no flag, no env → no gateway - client, fc := testutil.NewFakeClient("default") - fc.AddSandbox("default", &types.Sandbox{Name: "agent-a", Status: types.SandboxStatus{Phase: types.SandboxReady}}) +func TestDeleteRejectsRemovedProviderFlagsBeforeClientCreation(t *testing.T) { + for _, flag := range []string{"--all", "--providers"} { + t.Run(flag, func(t *testing.T) { + called := false + factory := func(context.Context, openshell.Target) (openshell.Client, error) { + called = true + return nil, errors.New("factory should not be called") + } + cmd := NewDeleteCmd(factory) + cmd.SetArgs([]string{flag}) + if _, err := captureStdout(t, cmd.Execute); err == nil { + t.Fatalf("%s should be rejected", flag) + } + if called { + t.Fatalf("%s should fail before creating a client", flag) + } + }) + } +} - // Factory that fails as sdkclient.New does when no active gateway is set. - noGateway := func(context.Context, openshell.Target) (openshell.Client, error) { - return nil, openshell.ErrConfig +func TestDeleteRejectsNamesWithSandboxSweep(t *testing.T) { + called := false + factory := func(context.Context, openshell.Target) (openshell.Client, error) { + called = true + return nil, errors.New("factory should not be called") + } + cmd := NewDeleteCmd(factory) + cmd.SetArgs([]string{"agent-a", "--sandboxes"}) + if _, err := captureStdout(t, cmd.Execute); err == nil { + t.Fatal("names and --sandboxes should be rejected") + } + if called { + t.Fatal("invalid delete combination should fail before creating a client") } +} - cmd := NewDeleteCmd(noGateway) - cmd.SetArgs([]string{"--all"}) +func TestDeleteSandboxesListFailurePropagates(t *testing.T) { + listErr := errors.New("gateway list failed") + client := &listErrorClient{Client: testutil.NewFake("default"), err: listErr} + cmd := NewDeleteCmd(keepOpenFactory(client)) + cmd.SetArgs([]string{"--sandboxes", "--gateway", "prod"}) _, err := captureStdout(t, cmd.Execute) - if err == nil { - t.Fatal("delete --all with no gateway should error, not report success") - } - if !contains(err.Error(), "create OpenShell client") { - t.Errorf("unexpected error: %v", err) + if !errors.Is(err, listErr) { + t.Fatalf("list failure = %v, want wrapped %v", err, listErr) } +} + +type listErrorClient struct { + openshell.Client + err error +} + +func (c *listErrorClient) Sandboxes(context.Context) ([]openshell.Sandbox, error) { + return nil, c.err +} + +type deleteErrorClient struct { + openshell.Client + name string + err error +} - // Nothing was swept. - if names := sandboxNames(t, client); len(names) != 1 { - t.Errorf("no-gateway delete must not touch resources, got %v", names) +func (c *deleteErrorClient) DeleteSandbox(ctx context.Context, name string) error { + if name == c.name { + return c.err } + return c.Client.DeleteSandbox(ctx, name) } -// With no --gateway flag and no $OPENSHELL_GATEWAY, delete relies on the SDK to -// resolve the active gateway (gateway.LoadConfig("")) and must still sweep — not -// error, and not silently skip. This is the exact case the local integration -// teardowns hit: a gateway is selected but not pinned per-command. +// With no --gateway flag and no $OPENSHELL_GATEWAY, delete relies on the SDK +// to resolve the active gateway and must still sweep when the factory succeeds. func TestDeleteUsesActiveGateway(t *testing.T) { - t.Setenv("OPENSHELL_GATEWAY", "") // no flag, no env → SDK resolves the active gateway + t.Setenv("OPENSHELL_GATEWAY", "") client, fc := testutil.NewFakeClient("default") fc.AddSandbox("default", &types.Sandbox{Name: "agent-a", Status: types.SandboxStatus{Phase: types.SandboxReady}}) @@ -147,24 +194,7 @@ func TestDeleteUsesActiveGateway(t *testing.T) { if _, err := captureStdout(t, cmd.Execute); err != nil { t.Fatalf("delete --sandboxes with an active gateway: %v", err) } - if names := sandboxNames(t, client); len(names) != 0 { t.Errorf("active-gateway resolution should sweep every sandbox, got %v", names) } } - -func TestDeleteProvidersSweep(t *testing.T) { - client, fc := testutil.NewFakeClient("default") - fc.AddProvider("default", &types.Provider{Name: "github", Type: "github"}) - fc.AddProvider("default", &types.Provider{Name: "vertex", Type: "google-vertex-ai"}) - - cmd := NewDeleteCmd(keepOpenFactory(client)) - cmd.SetArgs([]string{"--providers", "--gateway", "prod"}) - if _, err := captureStdout(t, cmd.Execute); err != nil { - t.Fatalf("delete --providers: %v", err) - } - - if names := providerNames(t, client); len(names) != 0 { - t.Errorf("--providers should sweep every provider, got %v", names) - } -} diff --git a/cmd/plan_test.go b/cmd/plan_test.go index 779ac4d..49e9c6b 100644 --- a/cmd/plan_test.go +++ b/cmd/plan_test.go @@ -70,7 +70,7 @@ spec: providers: - name: test-provider type: vertex-ai - management: managed + management: referenced inference: provider: test-provider model: claude-haiku-4-5 @@ -196,7 +196,7 @@ spec: providers: - name: test-provider type: vertex-ai - management: managed + management: referenced ` if err := os.WriteFile(configPath, []byte(configContent), 0o644); err != nil { t.Fatalf("write config: %v", err) @@ -248,7 +248,7 @@ spec: providers: - name: test-provider type: custom-provider - management: managed + management: referenced ` if err := os.WriteFile(configPath, []byte(configContent), 0o644); err != nil { t.Fatalf("write config: %v", err) @@ -488,7 +488,7 @@ spec: providers: - name: test-provider type: vertex-ai - management: managed + management: referenced ` if err := os.WriteFile(configPath, []byte(configContent), 0o644); err != nil { t.Fatalf("write config: %v", err) @@ -549,7 +549,7 @@ spec: providers: - name: test-provider type: vertex-ai - management: managed + management: referenced ` if err := os.WriteFile(configPath, []byte(configContent), 0o644); err != nil { t.Fatalf("write config: %v", err) @@ -594,7 +594,7 @@ spec: providers: - name: test-provider type: vertex-ai - management: managed + management: referenced ` if err := os.WriteFile(configPath, []byte(configContent), 0o644); err != nil { t.Fatalf("write config: %v", err) diff --git a/cmd/workflow_apply.go b/cmd/workflow_apply.go index 5450bbe..cfe2b11 100644 --- a/cmd/workflow_apply.go +++ b/cmd/workflow_apply.go @@ -11,7 +11,6 @@ import ( "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/reconcile" "github.com/stackrox/harness-openshell/internal/run" "github.com/stackrox/harness-openshell/internal/source" "github.com/stackrox/harness-openshell/internal/status" @@ -47,13 +46,13 @@ func targetDescription(target openshell.Target) string { return fmt.Sprintf("gateway %q", target.Gateway) } -// verifySandboxProviders checks capabilities attached directly to the sandbox. -// They are intentionally distinct from spec.providers (desired resources), so a -// workflow may consume a platform-owned provider without declaring ownership or -// configuration for it. +// verifySandboxProviders checks declared and attached provider references once. func verifySandboxProviders(ctx context.Context, client openshell.Client, desired *config.Harness) error { declared := make(map[string]struct{}, len(desired.Spec.Providers)) for _, provider := range desired.Spec.Providers { + if _, err := client.GetProvider(ctx, provider.Name); err != nil { + return fmt.Errorf("verifying referenced provider %q: %w", provider.Name, err) + } declared[provider.Name] = struct{}{} } for _, name := range desired.Spec.Sandbox.Providers { @@ -63,30 +62,20 @@ func verifySandboxProviders(ctx context.Context, client openshell.Client, desire if _, err := client.GetProvider(ctx, name); err != nil { return fmt.Errorf("verifying sandbox provider %q: %w", name, err) } + declared[name] = struct{}{} } return nil } -// preflightPlan rejects actions this execution path cannot safely -// realize before any provider or inference write occurs. Reconcile repeats its -// reads to remain race-safe, but it uses the same action functions. -func preflightPlan(desired *config.Harness, p *plan.Plan) error { - management := make(map[string]string, len(desired.Spec.Providers)) - for _, provider := range desired.Spec.Providers { - management[provider.Name] = provider.Management - } +// preflightPlan rejects missing references before any inference write occurs. +func preflightPlan(p *plan.Plan) error { for _, group := range p.Groups { for _, resource := range group.Resources { switch { case group.Section == plan.SectionTarget && resource.Action == plan.ActionLoginRequired: return fmt.Errorf("gateway %q is not reachable or authenticated", p.Target.Gateway) - case group.Section == plan.SectionProviders && resource.Action == plan.ActionCreate: - return fmt.Errorf("managed provider %q does not exist; create it through the platform bootstrap path before apply", resource.Name) - case group.Section == plan.SectionProviders && resource.Action == plan.ActionAdoptionRequired: - if management[resource.Name] == "referenced" { - return fmt.Errorf("referenced provider %q does not exist", resource.Name) - } - return fmt.Errorf("provider %q requires explicit adoption before apply", resource.Name) + case group.Section == plan.SectionProviders && resource.Action == plan.ActionMissing: + return fmt.Errorf("referenced provider %q does not exist; create it through platform bootstrap before apply", resource.Name) case group.Section == plan.SectionInference && resource.Action == plan.ActionValidate: return fmt.Errorf("gateway does not support inference route reconciliation") } @@ -95,24 +84,6 @@ func preflightPlan(desired *config.Harness, p *plan.Plan) error { return nil } -func reconcileProviders(ctx context.Context, client openshell.Client, desired []config.Provider) error { - results, err := reconcile.ReconcileProviders(ctx, client, desired) - if err != nil { - return fmt.Errorf("reconciling providers: %w", err) - } - for _, result := range results { - switch result.Action { - case plan.ActionCreate: - return fmt.Errorf("managed provider %q does not exist; create it through the platform bootstrap path before apply", result.Name) - case plan.ActionAdoptionRequired: - return fmt.Errorf("provider %q requires explicit adoption before apply", result.Name) - default: - status.OKf("provider %s: %s", result.Name, result.Action) - } - } - return nil -} - func inferenceConfigured(inf config.Inference) bool { return inf.Route != "" || inf.Provider != "" || inf.Model != "" || inf.Timeout != "" } @@ -293,8 +264,8 @@ func renderWorkflow(workflow *resolvedWorkflow, output string) error { } // redactedWorkflow keeps the resolved document shape while exposing only keys -// for maps whose values cross a credential boundary. Provider config and -// sandbox environment values may originate in the host environment and must +// for maps whose values cross a credential boundary. Sandbox environment +// values may originate in the host environment and must // never be serialized by -o yaml/json. func redactedWorkflow(resolved, input *config.Harness) *config.Harness { out := &config.Harness{ @@ -369,8 +340,6 @@ func redactedProviders(resolved, input []config.Provider) []config.Provider { Name: redactInterpolated(provider.Name, raw.Name), Type: redactInterpolated(provider.Type, raw.Type), Management: redactInterpolated(provider.Management, raw.Management), - Adopt: provider.Adopt, - Config: redactedStringMap(provider.Config), } } return out diff --git a/cmd/workflow_apply_test.go b/cmd/workflow_apply_test.go index 232bc88..411bb32 100644 --- a/cmd/workflow_apply_test.go +++ b/cmd/workflow_apply_test.go @@ -235,6 +235,32 @@ spec: } } +func TestApplyRejectsProviderManagementBeforeGatewayAccess(t *testing.T) { + for _, field := range []string{"management: managed", "adopt: true", "config: {region: global}"} { + t.Run(field, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "workflow.yaml") + writeTestFile(t, path, `apiVersion: harness.openshell.dev/v1alpha1 +kind: Harness +metadata: + name: provider-management +spec: + providers: + - name: existing + `+field+` +`) + factory := func(context.Context, openshell.Target) (openshell.Client, error) { + t.Fatal("removed provider management must fail before gateway access") + return nil, nil + } + command := NewApplyCmd(factory) + command.SetArgs([]string{"-f", path}) + if err := command.Execute(); err == nil { + t.Fatal("removed provider management was accepted") + } + }) + } +} + func TestApplyStructuredOutputRedactsCredentialBearingMaps(t *testing.T) { secret := "secret-value-that-must-not-leak" t.Setenv("WORKFLOW_SECRET", secret) @@ -247,8 +273,6 @@ spec: providers: - name: existing management: referenced - config: - API_TOKEN: ${WORKFLOW_SECRET} sandbox: env: API_TOKEN: ${WORKFLOW_SECRET} diff --git a/docs/README.md b/docs/README.md index c4e4884..e3aedf9 100644 --- a/docs/README.md +++ b/docs/README.md @@ -8,6 +8,7 @@ are intentionally not tracked here. |---|---| | [ci.md](ci.md) | HyperShell CI bootstrap and repository/secret contract | | [compatibility.md](compatibility.md) | Tested and observed OpenShell, ACP, and Go versions | +| [code-audit.md](code-audit.md) | Workflow-focused simplification, function/dependency inventory, and implemented removals | See also the top-level [README.md](../README.md) (usage) and [AGENTS.md](../AGENTS.md) (coding, upstream-alignment, and validation rules). diff --git a/internal/config/env.go b/internal/config/env.go index 54c4119..bb47f86 100644 --- a/internal/config/env.go +++ b/internal/config/env.go @@ -147,16 +147,12 @@ func Resolve(h *Harness, getenv func(string) string) (*Harness, error) { switch np.Management { case "": np.Management = "referenced" - case "managed", "referenced": + case "referenced": // valid + case "managed": + errs = append(errs, fmt.Sprintf("%s.management: managed providers are no longer supported; create/bootstrap the provider in OpenShell and use management: referenced", base)) default: - errs = append(errs, fmt.Sprintf("%s.management: %q is invalid (want \"managed\" or \"referenced\")", base, np.Management)) - } - if len(p.Config) > 0 { - np.Config = make(map[string]string, len(p.Config)) - for k, v := range p.Config { - np.Config[k] = exp(base+".config."+k, v) - } + errs = append(errs, fmt.Sprintf("%s.management: %q is invalid (want \"referenced\")", base, np.Management)) } s.Providers[i] = np } diff --git a/internal/config/env_test.go b/internal/config/env_test.go index b1e7031..19f652d 100644 --- a/internal/config/env_test.go +++ b/internal/config/env_test.go @@ -349,48 +349,6 @@ func TestResolveNonSecretField(t *testing.T) { } } -func TestResolveProviderConfig(t *testing.T) { - // Test resolving provider config map - h := &Harness{ - APIVersion: "harness.openshell.dev/v1alpha1", - Kind: "Harness", - Metadata: Metadata{Name: "test"}, - Spec: Spec{ - Providers: []Provider{ - { - Name: "vertex", - Type: "vertex", - Management: "managed", - Config: map[string]string{ - "PROJECT_ID": "${VERTEX_PROJECT_ID}", - "LOCATION": "us-central1", - }, - }, - }, - }, - } - - getenv := func(name string) string { - if name == "VERTEX_PROJECT_ID" { - return "my-gcp-project" - } - return "" - } - - resolved, err := Resolve(h, getenv) - if err != nil { - t.Fatalf("Resolve failed: %v", err) - } - - config := resolved.Spec.Providers[0].Config - if config["PROJECT_ID"] != "my-gcp-project" { - t.Errorf("PROJECT_ID should be resolved: got %q, want %q", config["PROJECT_ID"], "my-gcp-project") - } - if config["LOCATION"] != "us-central1" { - t.Errorf("LOCATION should stay as-is: got %q, want %q", config["LOCATION"], "us-central1") - } -} - func TestResolveMultipleMissingVars(t *testing.T) { // Test that Resolve aggregates all missing vars into one error h := &Harness{ diff --git a/internal/config/testdata/fact-dev.v1alpha1.yaml b/internal/config/testdata/fact-dev.v1alpha1.yaml index 6f439bf..2b8773c 100644 --- a/internal/config/testdata/fact-dev.v1alpha1.yaml +++ b/internal/config/testdata/fact-dev.v1alpha1.yaml @@ -9,10 +9,7 @@ spec: providers: - name: my-gcp type: google-vertex-ai - management: managed - config: - VERTEX_AI_PROJECT_ID: my-project - VERTEX_AI_REGION: global + management: referenced - name: github-fact management: referenced inference: diff --git a/internal/config/types.go b/internal/config/types.go index beb1f06..d5a4810 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -56,17 +56,13 @@ type OIDC struct { Audience string `yaml:"audience,omitempty"` } -// Provider represents a desired provider resource. +// Provider references a provider configured through OpenShell/bootstrap. type Provider struct { - Name string `yaml:"name"` - Type string `yaml:"type,omitempty"` - Management string `yaml:"management"` // "managed" or "referenced"; empty → referenced - // Adopt authorizes reconcile to take over an existing provider that does not - // carry this harness's owner label. Without it, a matching-but-unowned - // provider is reported adoption-required and never overwritten. It is the - // operator's explicit opt-in to manage a pre-existing provider. - Adopt bool `yaml:"adopt,omitempty"` - Config map[string]string `yaml:"config,omitempty"` + Name string `yaml:"name"` + Type string `yaml:"type,omitempty"` + // Management is retained for manifest compatibility. Only referenced + // providers are supported; bootstrap owns provider credentials. + Management string `yaml:"management"` } // Inference specifies the LLM inference route configuration. diff --git a/internal/openshell/client.go b/internal/openshell/client.go index e1146df..1571dc8 100644 --- a/internal/openshell/client.go +++ b/internal/openshell/client.go @@ -28,8 +28,6 @@ type Client interface { GetSandbox(ctx context.Context, name string) (Sandbox, error) // DeleteSandbox removes the named sandbox in the bound workspace. DeleteSandbox(ctx context.Context, name string) error - // DeleteProvider removes the named provider in the bound workspace. - DeleteProvider(ctx context.Context, name string) error // GatewayInfo introspects the active gateway (name, endpoint, status, // version). The SDK offers no gateway list; this reports the single gateway // the client is bound to. @@ -38,14 +36,6 @@ type Client interface { // ErrNotFound when no such provider exists (requires the "provider:read" // role). GetProvider(ctx context.Context, name string) (Provider, error) - // UpdateProvider writes the desired non-secret Config, Labels, and Type of an - // existing provider, preserving its stored credentials. It is - // credential-preserving by construction: the harness Provider carries no - // credentials, and sdkclient overlays only those non-secret fields onto the - // provider's current server object (see sdkclient.UpdateProvider). Reconcile - // issues it only on a real non-secret delta. Requires the workspace "admin" role plus - // "provider:write"; a caller lacking either gets ErrPermission. - UpdateProvider(ctx context.Context, p Provider) (Provider, error) // GetInferenceRoute reads the named inference route in the bound workspace. // An empty route targets the gateway default route. Returns ErrNotFound when // no such route exists (requires the workspace "user" role). @@ -86,12 +76,6 @@ type InferenceRouteReader interface { GetInferenceRoute(ctx context.Context, route string) (InferenceRoute, error) } -// ProviderReconciler reads and updates non-secret provider configuration. -type ProviderReconciler interface { - GetProvider(ctx context.Context, name string) (Provider, error) - UpdateProvider(ctx context.Context, p Provider) (Provider, error) -} - // InferenceReconciler reads and upserts inference routes. type InferenceReconciler interface { InferenceRouteReader diff --git a/internal/openshell/sdkclient/client.go b/internal/openshell/sdkclient/client.go index 46b133d..ee2b913 100644 --- a/internal/openshell/sdkclient/client.go +++ b/internal/openshell/sdkclient/client.go @@ -28,7 +28,6 @@ var ( _ openshell.Client = (*client)(nil) _ openshell.SandboxExecutionClient = (*client)(nil) _ openshell.StateReader = (*client)(nil) - _ openshell.ProviderReconciler = (*client)(nil) _ openshell.InferenceReconciler = (*client)(nil) ) diff --git a/internal/openshell/sdkclient/provider.go b/internal/openshell/sdkclient/provider.go index 6631042..316a336 100644 --- a/internal/openshell/sdkclient/provider.go +++ b/internal/openshell/sdkclient/provider.go @@ -8,32 +8,10 @@ import ( "github.com/stackrox/harness-openshell/internal/openshell" ) -// fromSDKProvider maps the SDK provider view to the harness Provider. It copies -// only the non-secret Config and Labels (as fresh maps, never aliasing the SDK -// object); Spec.Credentials, CredentialHandles, and ResourceVersion are -// deliberately dropped at this boundary (least-exposure firewall — see the -// openshell.Provider doc). Widen only when a consumer genuinely needs more -// fields, changing this and openshell.Provider together. +// fromSDKProvider exposes provider identity only. Credentials and provider +// configuration remain gateway-owned and never cross the harness boundary. func fromSDKProvider(p *v1.Provider) openshell.Provider { - return openshell.Provider{ - Name: p.Name, - Type: p.Type, - Config: copyStringMap(p.Spec.Config), - Labels: copyStringMap(p.Labels), - } -} - -// copyStringMap returns a fresh copy of m, or nil when m is empty, so the -// harness view never aliases the SDK object's maps. -func copyStringMap(m map[string]string) map[string]string { - if len(m) == 0 { - return nil - } - out := make(map[string]string, len(m)) - for k, v := range m { - out[k] = v - } - return out + return openshell.Provider{Name: p.Name, Type: p.Type} } // GetProvider reads the named provider in the bound workspace. @@ -44,51 +22,3 @@ func (c *client) GetProvider(ctx context.Context, name string) (openshell.Provid } return fromSDKProvider(p), nil } - -// DeleteProvider removes the named provider in the bound workspace. -func (c *client) DeleteProvider(ctx context.Context, name string) error { - return translate(c.raw.Providers().Delete(ctx, c.workspace, name)) -} - -// UpdateProvider writes the desired non-secret Config/Labels of an existing -// provider while preserving everything else the gateway holds — this is the -// single credential-preserving-update site (spec §8.5). -// -// It re-Gets the provider's current server object and overlays the non-secret -// managed fields (Config, Labels, and Type) onto it, then Updates. The -// credential-bearing spec fields -// (Credentials, CredentialHandles, CredentialExpiresAt, ProfileWorkspace) and -// the ResourceVersion are carried through from that Get verbatim; the harness -// never authors them. Because the harness openshell.Provider has no credentials -// field, this function is the ONLY place a full SDK provider object is -// assembled for a write, and it cannot introduce or drop a secret. -// -// The irreducible caveat lives here: a real gateway's Get never returns raw -// Credentials (they are write-only), so the object sent to Update carries an -// empty Credentials map. Whether the gateway reads that as "leave" or "wipe" is -// a server semantic no unit test can reach — it is proven by the gated -// TestLiveProviderUpdatePreservesCredentials. Reconcile bounds the risk by -// issuing this only on a real Config/Label delta. -func (c *client) UpdateProvider(ctx context.Context, p openshell.Provider) (openshell.Provider, error) { - cur, err := c.raw.Providers().Get(ctx, c.workspace, p.Name) - if err != nil { - return openshell.Provider{}, translate(err) - } - // Overlay only the non-secret managed fields onto the server's own object; - // everything else (creds, handles, expiry, profile workspace, RV) is left - // exactly as Get returned it. - cur.Spec.Config = copyStringMap(p.Config) - cur.Labels = copyStringMap(p.Labels) - if p.Type != "" { - // Type is a non-secret managed field. ProviderAction returns Update on a - // type delta (plan.ProviderAction), so this is the write that actually - // applies it — overlaid only when declared, so an unset desired Type never - // wipes the stored one. - cur.Type = p.Type - } - updated, err := c.raw.Providers().Update(ctx, c.workspace, cur) - if err != nil { - return openshell.Provider{}, translate(err) - } - return fromSDKProvider(updated), nil -} diff --git a/internal/openshell/sdkclient/provider_e2e_test.go b/internal/openshell/sdkclient/provider_e2e_test.go deleted file mode 100644 index 6b385b3..0000000 --- a/internal/openshell/sdkclient/provider_e2e_test.go +++ /dev/null @@ -1,185 +0,0 @@ -package sdkclient - -import ( - "context" - "errors" - "os" - "reflect" - "testing" - "time" - - "github.com/stackrox/harness-openshell/internal/openshell" -) - -// TestLiveProviderUpdatePreservesCredentials is the S1-risk gate for PR4a: it -// proves, against a real gateway, that the credential-preserving copy-through in -// UpdateProvider (which sends an EMPTY credentials map, because a real Get never -// returns raw credentials) leaves the provider's stored credentials intact -// rather than wiping them — the one server semantic no unit test can reach — and -// that the mTLS identity actually holds provider:write. -// -// It is skipped unless HARNESS_E2E_GATEWAY names a registered mTLS gateway, and -// again unless HARNESS_E2E_MANAGED_PROVIDER names a real, credentialed managed -// provider in the workspace. It mutates only that provider's Config/Labels and -// restores them on every exit path, so it never leaves drift behind. Optional -// HARNESS_E2E_WORKSPACE overrides the workspace. -// -// HARNESS_E2E_GATEWAY=openshell HARNESS_E2E_MANAGED_PROVIDER=google-vertex-ai \ -// go test ./internal/openshell/sdkclient/ -run LiveProviderUpdatePreservesCredentials -v -func TestLiveProviderUpdatePreservesCredentials(t *testing.T) { - gw := os.Getenv("HARNESS_E2E_GATEWAY") - if gw == "" { - t.Skip("set HARNESS_E2E_GATEWAY to a registered mTLS gateway to run the provider write gate") - } - name := os.Getenv("HARNESS_E2E_MANAGED_PROVIDER") - if name == "" { - t.Skip("set HARNESS_E2E_MANAGED_PROVIDER to a real credentialed managed provider to probe provider:write") - } - - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() - - oc, err := New(ctx, openshell.Target{Gateway: gw, Workspace: os.Getenv("HARNESS_E2E_WORKSPACE")}) - if err != nil { - t.Fatalf("New(%q): %v", gw, err) - } - // Close via t.Cleanup, not defer: t.Cleanup runs in LIFO after the test's - // deferred calls, so a deferred Close would shut the gRPC connection before - // the restoration cleanup registered below could use it. Registered first, - // it runs last. (Same ordering fix as TestLiveInferenceRoleProbe.) - t.Cleanup(func() { - if err := oc.Close(); err != nil { - t.Errorf("closing client: %v", err) - } - }) - // In-package access to the raw SDK client, so the probe can observe the - // credential handles the firewall Provider deliberately hides. - raw := oc.(*client) - - before, err := oc.GetProvider(ctx, name) - if err != nil { - t.Fatalf("GetProvider(%q): %v (is it registered in the workspace?)", name, err) - } - beforeRaw, err := raw.raw.Providers().Get(ctx, raw.workspace, name) - if err != nil { - t.Fatalf("raw Get(%q): %v", name, err) - } - beforeHandles := beforeRaw.Spec.CredentialHandles - beforeExpiry := beforeRaw.Spec.CredentialExpiresAt - if len(beforeHandles) == 0 { - t.Logf("WARNING: provider %q reports no credential handles; the handle-survival "+ - "assertion is inconclusive. Point HARNESS_E2E_INFERENCE_PROVIDER at this "+ - "provider and run the inference probe for the definitive credential-works proof.", name) - } - - // Register restoration BEFORE the write. UpdateProvider persists at the - // gateway before its response returns, so even a failed write may have - // changed state; t.Cleanup runs on every exit path (fresh context, since ctx - // may be spent). Skipped only on a pre-write permission denial: nothing was - // written and the identity lacks the role the restore would need. - permissionDenied := false - t.Cleanup(func() { - if permissionDenied { - return - } - cctx, ccancel := context.WithTimeout(context.Background(), 30*time.Second) - defer ccancel() - if _, err := oc.UpdateProvider(cctx, openshell.Provider{ - Name: name, Config: before.Config, Labels: before.Labels, - }); err != nil { - t.Errorf("restoring provider Config/Labels: %v", err) - } - }) - - // Config-only mutation: carry the current Labels through unchanged (Update - // overlays both fields) and flip one probe key in Config. This is the exact - // production destructive path — an empty-credentials copy-through Update. - probeConfig := copyStringMap(before.Config) - if probeConfig == nil { - probeConfig = map[string]string{} - } - probeConfig["harness.openshell.dev/e2e-probe"] = "1" - _, setErr := oc.UpdateProvider(ctx, openshell.Provider{ - Name: name, Config: probeConfig, Labels: before.Labels, - }) - switch { - case setErr == nil: - t.Logf("WRITE path OK on gateway %q: identity HAS provider:write", gw) - case errors.Is(setErr, openshell.ErrPermission): - permissionDenied = true - t.Fatalf("WRITE path DENIED on gateway %q: identity LACKS provider:write "+ - "(provider reconcile-write will fail until granted): %v", gw, setErr) - default: - t.Fatalf("UpdateProvider returned an unexpected error: %v", setErr) - } - - afterRaw, err := raw.raw.Providers().Get(ctx, raw.workspace, name) - if err != nil { - t.Fatalf("raw Get(%q) after update: %v", name, err) - } - if !reflect.DeepEqual(beforeHandles, afterRaw.Spec.CredentialHandles) { - t.Fatalf("CREDENTIALS WIPED: credential handles changed after a Config-only "+ - "update.\n before: %v\n after: %v\nempty-map-means-WIPE — the "+ - "copy-through Update is UNSAFE on this gateway; disable managed provider "+ - "Update until a config-only RPC or cred-resupply path exists.", - beforeHandles, afterRaw.Spec.CredentialHandles) - } - if !reflect.DeepEqual(beforeExpiry, afterRaw.Spec.CredentialExpiresAt) { - t.Fatalf("CREDENTIALS ROTATED/WIPED: credential expiry changed after a "+ - "Config-only update.\n before: %v\n after: %v", beforeExpiry, afterRaw.Spec.CredentialExpiresAt) - } - if afterRaw.Spec.Config["harness.openshell.dev/e2e-probe"] != "1" { - t.Errorf("probe config key not persisted: %v", afterRaw.Spec.Config) - } - t.Logf("PASS on gateway %q: empty-map credentials Update = LEAVE-UNTOUCHED; "+ - "copy-through provider Update is safe.", gw) - - // Stronger, definitive proof (slice S2 step 4): unchanged handles show the - // credentials survived STRUCTURALLY, but only a real call proves they still - // AUTHENTICATE. When this same provider also backs inference, a verify-mode - // route write (NoVerify:false) makes the gateway call the provider endpoint - // with its stored credentials; success after the config-only update is the - // end-to-end guarantee. It needs a model known-good for the provider - // (HARNESS_E2E_INFERENCE_MODEL) so a failure means "credentials broke", not - // "unknown model" — without one this layer is skipped, not guessed. - if os.Getenv("HARNESS_E2E_INFERENCE_PROVIDER") != name { - return - } - model := os.Getenv("HARNESS_E2E_INFERENCE_MODEL") - if model == "" { - t.Logf("skipping downstream inference-verify proof: set HARNESS_E2E_INFERENCE_MODEL "+ - "to a model valid for %q to enable the definitive credentials-still-authenticate check", name) - return - } - - const verifyRoute = "inference.local" // the only route name a real gateway accepts (see inference_e2e_test.go) - beforeRoute, beforeRouteErr := oc.GetInferenceRoute(ctx, verifyRoute) - if beforeRouteErr != nil && !errors.Is(beforeRouteErr, openshell.ErrNotFound) { - t.Fatalf("pre-verify GetInferenceRoute(%q): %v", verifyRoute, beforeRouteErr) - } - routeExisted := beforeRouteErr == nil - // Registered last → runs first (LIFO), so the route is restored while the - // client is still open, ahead of the provider-config restore and Close above. - t.Cleanup(func() { - cctx, ccancel := context.WithTimeout(context.Background(), 30*time.Second) - defer ccancel() - if routeExisted { - if _, err := oc.SetInferenceRoute(cctx, openshell.InferenceRouteConfig{ - Provider: beforeRoute.Provider, Model: beforeRoute.Model, Route: verifyRoute, - NoVerify: true, TimeoutSecs: beforeRoute.TimeoutSecs, - }); err != nil { - t.Errorf("restoring inference route: %v", err) - } - } else if err := oc.DeleteInferenceRoute(cctx, verifyRoute); err != nil { - t.Errorf("cleanup DeleteInferenceRoute(%q): %v", verifyRoute, err) - } - }) - if _, err := oc.SetInferenceRoute(ctx, openshell.InferenceRouteConfig{ - Provider: name, Model: model, Route: verifyRoute, NoVerify: false, - }); err != nil { - t.Fatalf("DOWNSTREAM VERIFY FAILED after the config-only update: provider %q's "+ - "credentials no longer authenticate (verify route %q/%q): %v", name, verifyRoute, model, err) - } - t.Logf("DOWNSTREAM VERIFY OK: provider %q credentials still authenticate after the update — "+ - "copy-through Update is safe end-to-end.", name) -} diff --git a/internal/openshell/sdkclient/provider_test.go b/internal/openshell/sdkclient/provider_test.go index 40fa455..b66ef8e 100644 --- a/internal/openshell/sdkclient/provider_test.go +++ b/internal/openshell/sdkclient/provider_test.go @@ -2,200 +2,53 @@ package sdkclient import ( "context" + "encoding/json" "errors" + "strings" "testing" - "time" + v1 "github.com/NVIDIA/OpenShell/sdk/go/openshell/v1" fake "github.com/NVIDIA/OpenShell/sdk/go/openshell/v1/fake" "github.com/NVIDIA/OpenShell/sdk/go/openshell/v1/types" - "github.com/stackrox/harness-openshell/internal/openshell" ) -// TestFromSDKProviderMapsConfigAndLabels pins the S1 read-widening: the harness -// Provider carries the SDK provider's non-secret Config and Labels, as fresh -// copies, and never the secret Spec fields. -func TestFromSDKProviderMapsConfigAndLabels(t *testing.T) { - sdkConfig := map[string]string{"VERTEX_AI_REGION": "global"} - sdkLabels := map[string]string{"harness.openshell.dev/managed-by": "harness"} - p := &types.Provider{ - Name: "google-vertex-ai", - Type: "google-vertex-ai", - ResourceVersion: 7, - Labels: sdkLabels, +func TestFromSDKProviderExposesIdentityOnly(t *testing.T) { + got := fromSDKProvider(&v1.Provider{ + Name: "github", + Type: "github", + Labels: map[string]string{"owner": "test"}, Spec: types.ProviderSpec{ - Config: sdkConfig, - Credentials: map[string]string{"API_KEY": "secret"}, // must NOT cross - CredentialHandles: map[string]types.CredentialHandle{ - "API_KEY": {Driver: "vault", Handle: "h1"}, - }, - CredentialExpiresAt: map[string]time.Time{"API_KEY": {}}, + Config: map[string]string{"endpoint": "https://example.invalid"}, + Credentials: map[string]string{"token": "must-not-cross-boundary"}, }, + }) + if got.Name != "github" || got.Type != "github" { + t.Fatalf("identity = %#v", got) } - - got := fromSDKProvider(p) - - if got.Name != "google-vertex-ai" || got.Type != "google-vertex-ai" { - t.Fatalf("name/type not mapped: %+v", got) - } - if got.Config["VERTEX_AI_REGION"] != "global" { - t.Errorf("Config not mapped: %v", got.Config) - } - if got.Labels["harness.openshell.dev/managed-by"] != "harness" { - t.Errorf("Labels not mapped: %v", got.Labels) - } - - // Fresh copies: mutating the source must not affect the harness view. - sdkConfig["VERTEX_AI_REGION"] = "us-east1" - sdkLabels["harness.openshell.dev/managed-by"] = "someone-else" - if got.Config["VERTEX_AI_REGION"] != "global" { - t.Errorf("Config aliases the SDK map: %v", got.Config) - } - if got.Labels["harness.openshell.dev/managed-by"] != "harness" { - t.Errorf("Labels aliases the SDK map: %v", got.Labels) - } -} - -// TestFromSDKProviderEmptyMapsAreNil keeps the harness view tidy: absent -// Config/Labels map to nil, not empty non-nil maps. -func TestFromSDKProviderEmptyMapsAreNil(t *testing.T) { - got := fromSDKProvider(&types.Provider{Name: "p", Type: "openai"}) - if got.Config != nil { - t.Errorf("expected nil Config, got %v", got.Config) + data, err := json.Marshal(got) + if err != nil { + t.Fatal(err) } - if got.Labels != nil { - t.Errorf("expected nil Labels, got %v", got.Labels) + if strings.Contains(string(data), "must-not-cross-boundary") || strings.Contains(string(data), "example.invalid") { + t.Fatal("provider credentials or configuration crossed the read boundary") } } -// TestGetProvider covers the read path: fields map through, and a missing -// provider surfaces as openshell.ErrNotFound. func TestGetProvider(t *testing.T) { ctx := context.Background() fc := fake.NewClient() - fc.AddProvider("default", &types.Provider{ - Name: "github", Type: "github", - Spec: types.ProviderSpec{Config: map[string]string{"k": "v"}}, - }) + fc.AddProvider("default", &types.Provider{Name: "github", Type: "github"}) c := NewFromClient(fc, "default") - + defer c.Close() got, err := c.GetProvider(ctx, "github") if err != nil { t.Fatalf("GetProvider: %v", err) } - if got.Name != "github" || got.Config["k"] != "v" { + if got.Name != "github" || got.Type != "github" { t.Errorf("unexpected provider: %+v", got) } - if _, err := c.GetProvider(ctx, "absent"); !errors.Is(err, openshell.ErrNotFound) { t.Errorf("GetProvider(absent): want ErrNotFound, got %v", err) } } - -// TestUpdateProviderOverlaysConfigPreservesCredentials pins the copy-through: -// UpdateProvider changes only Config/Labels and leaves the stored credentials -// and handles intact. -// -// NOTE ON THE FAKE: this passes because the fake's Get RETURNS the stored -// credentials, so the copy-through carries them back. A real gateway's Get -// returns an EMPTY credentials map (write-only), so this test proves the -// harness overlay logic never drops what Get gave it — NOT the real -// empty-map-means-leave semantic, which only the gated -// TestLiveProviderUpdatePreservesCredentials can prove. The fake also does not -// enforce ResourceVersion OCC, so this asserts neither. -func TestUpdateProviderOverlaysConfigPreservesCredentials(t *testing.T) { - ctx := context.Background() - fc := fake.NewClient() - fc.AddProvider("default", &types.Provider{ - Name: "google-vertex-ai", Type: "google-vertex-ai", - Spec: types.ProviderSpec{ - Config: map[string]string{"VERTEX_AI_REGION": "global"}, - Credentials: map[string]string{"API_KEY": "secret"}, - CredentialHandles: map[string]types.CredentialHandle{ - "API_KEY": {Driver: "vault", Handle: "h1"}, - }, - }, - }) - c := NewFromClient(fc, "default") - - out, err := c.UpdateProvider(ctx, openshell.Provider{ - Name: "google-vertex-ai", - Config: map[string]string{"VERTEX_AI_REGION": "us-east1"}, - Labels: map[string]string{"harness.openshell.dev/managed-by": "harness"}, - }) - if err != nil { - t.Fatalf("UpdateProvider: %v", err) - } - if out.Config["VERTEX_AI_REGION"] != "us-east1" { - t.Errorf("Config not updated in returned view: %v", out.Config) - } - - // Inspect the raw stored object: creds + handles survived the overlay. - stored, err := fc.Providers().Get(ctx, "default", "google-vertex-ai") - if err != nil { - t.Fatalf("raw Get: %v", err) - } - if stored.Spec.Credentials["API_KEY"] != "secret" { - t.Errorf("credentials clobbered by update: %v", stored.Spec.Credentials) - } - if _, ok := stored.Spec.CredentialHandles["API_KEY"]; !ok { - t.Errorf("credential handles clobbered by update: %v", stored.Spec.CredentialHandles) - } - if stored.Spec.Config["VERTEX_AI_REGION"] != "us-east1" { - t.Errorf("stored Config not updated: %v", stored.Spec.Config) - } - if stored.Labels["harness.openshell.dev/managed-by"] != "harness" { - t.Errorf("stored Labels not updated: %v", stored.Labels) - } -} - -// TestUpdateProviderWritesType: a declared Type is overlaid onto the stored -// provider, so a type delta reported as Update by plan.ProviderAction actually -// converges. An empty desired Type leaves the stored one untouched. -func TestUpdateProviderWritesType(t *testing.T) { - ctx := context.Background() - fc := fake.NewClient() - fc.AddProvider("default", &types.Provider{ - Name: "gcp", Type: "old-type", - Spec: types.ProviderSpec{Config: map[string]string{"K": "v"}}, - }) - c := NewFromClient(fc, "default") - - // Declared Type is written. - if _, err := c.UpdateProvider(ctx, openshell.Provider{ - Name: "gcp", Type: "new-type", Config: map[string]string{"K": "v"}, - }); err != nil { - t.Fatalf("UpdateProvider: %v", err) - } - stored, err := fc.Providers().Get(ctx, "default", "gcp") - if err != nil { - t.Fatalf("raw Get: %v", err) - } - if stored.Type != "new-type" { - t.Errorf("Type not written: got %q, want new-type", stored.Type) - } - - // Empty desired Type preserves the stored one. - if _, err := c.UpdateProvider(ctx, openshell.Provider{ - Name: "gcp", Config: map[string]string{"K": "v2"}, - }); err != nil { - t.Fatalf("UpdateProvider (empty type): %v", err) - } - stored, err = fc.Providers().Get(ctx, "default", "gcp") - if err != nil { - t.Fatalf("raw Get: %v", err) - } - if stored.Type != "new-type" { - t.Errorf("empty desired Type wiped stored Type: got %q, want new-type", stored.Type) - } -} - -// TestUpdateProviderNotFound: updating an absent provider surfaces ErrNotFound -// from the internal Get, never a nil-object write. -func TestUpdateProviderNotFound(t *testing.T) { - ctx := context.Background() - c := NewFromClient(fake.NewClient(), "default") - if _, err := c.UpdateProvider(ctx, openshell.Provider{Name: "absent"}); !errors.Is(err, openshell.ErrNotFound) { - t.Errorf("UpdateProvider(absent): want ErrNotFound, got %v", err) - } -} diff --git a/internal/openshell/types.go b/internal/openshell/types.go index d369dea..f3365a7 100644 --- a/internal/openshell/types.go +++ b/internal/openshell/types.go @@ -34,18 +34,11 @@ type Health struct { // Provider is the harness view of a registered provider. // -// Deliberately narrow (least-exposure firewall): it carries only the non-secret -// fields the harness diffs, reports, or writes. It has NO Credentials field — -// credentials are write-only and never returned by the SDK's Get, so keeping -// them off this type makes credential-clobber-by-reconcile impossible by -// construction (a reconcile can neither read nor author a secret). It has NO -// ResourceVersion field either: the OCC token is an SDK detail owned entirely by -// sdkclient.UpdateProvider's copy-through, never surfaced to callers. +// Deliberately narrow (least-exposure firewall): it carries provider identity +// only. Credentials and provider configuration remain gateway-owned. type Provider struct { - Name string - Type string - Config map[string]string // non-secret managed configuration - Labels map[string]string // ownership + metadata (see plan.OwnerLabelKey) + Name string + Type string } // Sandbox is the harness view of a sandbox for the read UX (get/describe). diff --git a/internal/plan/ownership.go b/internal/plan/ownership.go deleted file mode 100644 index 733fad1..0000000 --- a/internal/plan/ownership.go +++ /dev/null @@ -1,22 +0,0 @@ -package plan - -import "github.com/stackrox/harness-openshell/internal/openshell" - -// Ownership labels mark a provider as reconcile-managed by this harness. They are -// the single vocabulary for "the harness owns this provider" — the diff rule -// (ProviderAction) reads them to decide adoption, and reconcile stamps them on -// the providers it updates. Kept here as the one owner so the plan and the write -// path can never disagree on what "owned" means. -const ( - // OwnerLabelKey is the label key stamped on harness-managed providers. - OwnerLabelKey = "harness.openshell.dev/managed-by" - // OwnerLabelValue is the value OwnerLabelKey must carry to count as owned. - OwnerLabelValue = "harness" -) - -// IsOwned reports whether the provider carries this harness's ownership label. -// It checks both key and value: a foreign managed-by value (another controller) -// is deliberately not ours, so reconcile will not silently take it over. -func IsOwned(p openshell.Provider) bool { - return p.Labels[OwnerLabelKey] == OwnerLabelValue -} diff --git a/internal/plan/plan.go b/internal/plan/plan.go index e75d687..81ab5f4 100644 --- a/internal/plan/plan.go +++ b/internal/plan/plan.go @@ -16,16 +16,16 @@ import ( type Action string const ( - ActionNoop Action = "noop" - ActionCreate Action = "create" - ActionUpdate Action = "update" - ActionValidate Action = "validate" - ActionLoginRequired Action = "login-required" - ActionAdoptionRequired Action = "adoption-required" - ActionCreateSandbox Action = "create-sandbox" - ActionUpload Action = "upload" - ActionExecute Action = "execute" - ActionDeleteSandbox Action = "delete-sandbox" + ActionNoop Action = "noop" + ActionCreate Action = "create" + ActionUpdate Action = "update" + ActionValidate Action = "validate" + ActionLoginRequired Action = "login-required" + ActionMissing Action = "missing" + ActionCreateSandbox Action = "create-sandbox" + ActionUpload Action = "upload" + ActionExecute Action = "execute" + ActionDeleteSandbox Action = "delete-sandbox" ) // Section names a group of plan resources. @@ -119,78 +119,8 @@ func buildTargetGroup(desired *config.Harness, current CurrentState) Group { } } -// isManaged reports whether a desired provider is harness-managed. Management is -// "managed" or "referenced"; empty defaults to referenced (never auto-create, -// never overwrite — the safe default, enforced at config.Resolve). Only "managed" -// providers are created or updated by reconcile. -func isManaged(p config.Provider) bool { - return p.Management == "managed" -} - -// ProviderAction is the single owner of the provider create/adopt/update/noop -// rule (invariant 22). Both harness plan (buildProvidersGroup) and -// internal/reconcile call it, so the plan preview and the reconcile write can -// never disagree on what a change is. -// -// cur is the matching current provider by name, or nil when none exists. -// -// Referenced providers are never written: an existing one is a noop whoever owns -// it, and an absent one is adoption-required (it must be created/adopted out of -// band). Managed providers are where ownership matters — reconcile must never -// overwrite one it does not own. A managed provider carrying no harness owner -// label (plan.IsOwned false) is therefore adoption-required until the operator -// opts in with `adopt: true`; only then does managed drift (type/config) or the -// still-missing owner label become an Update. Stamping the owner label on that -// first adopting update is itself the Label delta the credential-preserving -// copy-through then carries (see the spec's credential-preservation note); -// reconcile issues no Update without one of these real deltas. -func ProviderAction(desired config.Provider, cur *openshell.Provider) Action { - managed := isManaged(desired) - - if cur == nil { - if managed { - return ActionCreate - } - return ActionAdoptionRequired // referenced/unknown: never auto-create - } - - // Referenced providers are never written: if it exists we simply reference it, - // regardless of who owns it. - if !managed { - return ActionNoop - } - - // Managed, but not ours and the operator has not authorized taking it over: - // never overwrite a provider another owner (or a human) created. - if !IsOwned(*cur) && !desired.Adopt { - return ActionAdoptionRequired - } - - // We own it, or the operator authorized adoption. A missing owner label here - // means we are adopting (adopt=true), so stamping it is a real Update. - typeMismatch := desired.Type != "" && cur.Type != desired.Type - if typeMismatch || configDrifts(desired.Config, cur.Config) || !IsOwned(*cur) { - return ActionUpdate - } - return ActionNoop -} - -// configDrifts reports whether the current provider config is missing or differs -// from any key the desired config declares. The harness owns only the keys it -// declares: extra keys the gateway or provider carries are not drift, so this is -// a subset check (desired ⊆ current), not equality. -func configDrifts(desired, current map[string]string) bool { - for k, v := range desired { - if current[k] != v { - return true - } - } - return false -} - // buildProvidersGroup returns the PROVIDERS group. It matches desired providers -// by name against current.Providers and defers every per-provider decision to -// ProviderAction, so plan and reconcile share one rule. +// by name against current.Providers without proposing provider writes. func buildProvidersGroup(desired *config.Harness, current CurrentState) Group { group := Group{Section: SectionProviders} @@ -203,14 +133,14 @@ func buildProvidersGroup(desired *config.Harness, current CurrentState) Group { for i := range desired.Spec.Providers { desiredProv := desired.Spec.Providers[i] - var cur *openshell.Provider - if c, exists := currentByName[desiredProv.Name]; exists { - cur = &c + action := ActionMissing + if _, exists := currentByName[desiredProv.Name]; exists { + action = ActionNoop } group.Resources = append(group.Resources, Resource{ Name: desiredProv.Name, - Action: ProviderAction(desiredProv, cur), + Action: action, Detail: buildProviderDetail(&desiredProv), }) } @@ -224,7 +154,6 @@ func buildProviderDetail(prov *config.Provider) string { if detail == "" { detail = "(type unspecified)" } - detail += "; management: " + prov.Management return detail } diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go index 826d2d6..e38be74 100644 --- a/internal/plan/plan_test.go +++ b/internal/plan/plan_test.go @@ -67,253 +67,23 @@ func TestBuild_TargetLoginRequiredWhenUnreachable(t *testing.T) { } } -func TestBuild_ProviderPresentNoop(t *testing.T) { - desired := &config.Harness{ - Spec: config.Spec{ - Target: config.Target{Gateway: "test-gateway"}, - Providers: []config.Provider{ - { - Name: "github", - Type: "github", - Management: "managed", - }, - }, - }, - } - current := CurrentState{ - Reachable: true, - Health: openshell.Health{Healthy: true, Version: "0.0.110"}, - Providers: []openshell.Provider{ - // Owned by the harness (carries the owner label), so a matching managed - // provider is a noop. An unowned match would be adoption-required — see - // the ProviderAction table. - {Name: "github", Type: "github", Labels: map[string]string{OwnerLabelKey: OwnerLabelValue}}, - }, - } - - plan := Build(desired, current) - - var provGroup *Group - for i := range plan.Groups { - if plan.Groups[i].Section == SectionProviders { - provGroup = &plan.Groups[i] - break +func TestBuildReferencedProviders(t *testing.T) { + desired := &config.Harness{Spec: config.Spec{Providers: []config.Provider{ + {Name: "present", Type: "github"}, + {Name: "absent"}, + }}} + p := Build(desired, CurrentState{Providers: []openshell.Provider{{Name: "present"}}}) + for _, group := range p.Groups { + if group.Section != SectionProviders { + continue } - } - - if provGroup == nil { - t.Fatal("expected PROVIDERS group") - } - if len(provGroup.Resources) != 1 { - t.Errorf("expected 1 provider resource, got %d", len(provGroup.Resources)) - } - - res := provGroup.Resources[0] - if res.Action != ActionNoop { - t.Errorf("expected ActionNoop, got %s", res.Action) - } - if res.Name != "github" { - t.Errorf("expected name 'github', got %s", res.Name) - } -} - -func TestBuild_ProviderAbsentManaged(t *testing.T) { - desired := &config.Harness{ - Spec: config.Spec{ - Target: config.Target{Gateway: "test-gateway"}, - Providers: []config.Provider{ - { - Name: "gcp", - Type: "google-vertex-ai", - Management: "managed", - }, - }, - }, - } - current := CurrentState{ - Reachable: true, - Health: openshell.Health{Healthy: true, Version: "0.0.110"}, - Providers: []openshell.Provider{}, - } - - plan := Build(desired, current) - - var provGroup *Group - for i := range plan.Groups { - if plan.Groups[i].Section == SectionProviders { - provGroup = &plan.Groups[i] - break + if len(group.Resources) != 2 || group.Resources[0].Name != "present" || group.Resources[0].Action != ActionNoop || + group.Resources[1].Name != "absent" || group.Resources[1].Action != ActionMissing { + t.Fatalf("unexpected provider plan: %+v", group.Resources) } + return } - - res := provGroup.Resources[0] - if res.Action != ActionCreate { - t.Errorf("expected ActionCreate, got %s", res.Action) - } -} - -func TestBuild_ProviderAbsentReferenced(t *testing.T) { - desired := &config.Harness{ - Spec: config.Spec{ - Target: config.Target{Gateway: "test-gateway"}, - Providers: []config.Provider{ - { - Name: "external", - Management: "referenced", - }, - }, - }, - } - current := CurrentState{ - Reachable: true, - Health: openshell.Health{Healthy: true, Version: "0.0.110"}, - Providers: []openshell.Provider{}, - } - - plan := Build(desired, current) - - var provGroup *Group - for i := range plan.Groups { - if plan.Groups[i].Section == SectionProviders { - provGroup = &plan.Groups[i] - break - } - } - - res := provGroup.Resources[0] - if res.Action != ActionAdoptionRequired { - t.Errorf("expected ActionAdoptionRequired, got %s", res.Action) - } -} - -func TestBuild_ProviderTypeUpdate(t *testing.T) { - desired := &config.Harness{ - Spec: config.Spec{ - Target: config.Target{Gateway: "test-gateway"}, - Providers: []config.Provider{ - { - Name: "github", - Type: "github-new", - Management: "managed", - }, - }, - }, - } - current := CurrentState{ - Reachable: true, - Health: openshell.Health{Healthy: true, Version: "0.0.110"}, - Providers: []openshell.Provider{ - // Owned, so a type mismatch is an in-place update. An unowned provider - // with a type mismatch would be adoption-required, not overwritten. - {Name: "github", Type: "github-old", Labels: map[string]string{OwnerLabelKey: OwnerLabelValue}}, - }, - } - - plan := Build(desired, current) - - var provGroup *Group - for i := range plan.Groups { - if plan.Groups[i].Section == SectionProviders { - provGroup = &plan.Groups[i] - break - } - } - - res := provGroup.Resources[0] - if res.Action != ActionUpdate { - t.Errorf("expected ActionUpdate, got %s", res.Action) - } -} - -// TestProviderAction is the single-owner diff-rule table (invariant 22). It -// pins every branch of the create/adopt/update/noop rule, including the -// ownership gate that keeps reconcile from overwriting a provider it does not -// own. -func TestProviderAction(t *testing.T) { - owned := map[string]string{OwnerLabelKey: OwnerLabelValue} - foreign := map[string]string{OwnerLabelKey: "someone-else"} - - tests := []struct { - name string - desired config.Provider - cur *openshell.Provider - want Action - }{ - { - name: "managed absent creates", - desired: config.Provider{Name: "gcp", Type: "google-vertex-ai", Management: "managed"}, - cur: nil, - want: ActionCreate, - }, - { - name: "referenced absent requires adoption", - desired: config.Provider{Name: "ext", Management: "referenced"}, - cur: nil, - want: ActionAdoptionRequired, - }, - { - name: "empty management treated as referenced (absent) requires adoption", - desired: config.Provider{Name: "ext"}, - cur: nil, - want: ActionAdoptionRequired, - }, - { - name: "unowned existing requires adoption (no overwrite)", - desired: config.Provider{Name: "gh", Type: "github", Management: "managed"}, - cur: &openshell.Provider{Name: "gh", Type: "github"}, - want: ActionAdoptionRequired, - }, - { - name: "foreign-owned existing requires adoption", - desired: config.Provider{Name: "gh", Type: "github", Management: "managed"}, - cur: &openshell.Provider{Name: "gh", Type: "github", Labels: foreign}, - want: ActionAdoptionRequired, - }, - { - name: "adopt authorizes taking over an unowned provider (label stamp is an update)", - desired: config.Provider{Name: "gh", Type: "github", Management: "managed", Adopt: true}, - cur: &openshell.Provider{Name: "gh", Type: "github"}, - want: ActionUpdate, - }, - { - name: "owned type mismatch updates", - desired: config.Provider{Name: "gh", Type: "github-new", Management: "managed"}, - cur: &openshell.Provider{Name: "gh", Type: "github-old", Labels: owned}, - want: ActionUpdate, - }, - { - name: "owned config drift updates", - desired: config.Provider{Name: "gcp", Type: "google-vertex-ai", Management: "managed", Config: map[string]string{"VERTEX_AI_REGION": "us-east1"}}, - cur: &openshell.Provider{Name: "gcp", Type: "google-vertex-ai", Labels: owned, Config: map[string]string{"VERTEX_AI_REGION": "global"}}, - want: ActionUpdate, - }, - { - name: "owned matching is noop (extra current config keys are not drift)", - desired: config.Provider{Name: "gcp", Type: "google-vertex-ai", Management: "managed", Config: map[string]string{"VERTEX_AI_REGION": "global"}}, - cur: &openshell.Provider{Name: "gcp", Type: "google-vertex-ai", Labels: owned, Config: map[string]string{"VERTEX_AI_REGION": "global", "EXTRA": "x"}}, - want: ActionNoop, - }, - { - name: "referenced existing and owned is noop (never updated)", - desired: config.Provider{Name: "ext", Type: "custom", Management: "referenced"}, - cur: &openshell.Provider{Name: "ext", Type: "different", Labels: owned}, - want: ActionNoop, - }, - { - name: "referenced existing and unowned is noop (referenced is never written)", - desired: config.Provider{Name: "ext", Type: "custom", Management: "referenced"}, - cur: &openshell.Provider{Name: "ext", Type: "different"}, - want: ActionNoop, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := ProviderAction(tt.desired, tt.cur); got != tt.want { - t.Errorf("ProviderAction() = %s, want %s", got, tt.want) - } - }) - } + t.Fatal("missing providers group") } func TestBuild_InferenceGroupWhenConfigured(t *testing.T) { @@ -695,7 +465,7 @@ func TestPlan_TableSections(t *testing.T) { Spec: config.Spec{ Target: config.Target{Gateway: "test-gateway"}, Providers: []config.Provider{ - {Name: "github", Type: "github", Management: "managed"}, + {Name: "github", Type: "github", Management: "referenced"}, }, }, } diff --git a/internal/plan/render_test.go b/internal/plan/render_test.go index 8a4f2f4..59e4269 100644 --- a/internal/plan/render_test.go +++ b/internal/plan/render_test.go @@ -21,12 +21,12 @@ func TestTableSections_RepresentativePlan(t *testing.T) { { Name: "github", Type: "github", - Management: "managed", + Management: "referenced", }, { Name: "gcp", Type: "google-vertex-ai", - Management: "managed", + Management: "referenced", }, }, Inference: config.Inference{ diff --git a/internal/plan/state_test.go b/internal/plan/state_test.go index 934afa8..0ab259e 100644 --- a/internal/plan/state_test.go +++ b/internal/plan/state_test.go @@ -326,18 +326,10 @@ func (r *recordingClient) DeleteSandbox(ctx context.Context, name string) error return r.wrapped.DeleteSandbox(ctx, name) } -func (r *recordingClient) DeleteProvider(ctx context.Context, name string) error { - return r.wrapped.DeleteProvider(ctx, name) -} - func (r *recordingClient) GatewayInfo(ctx context.Context) (openshell.GatewayInfo, error) { return r.wrapped.GatewayInfo(ctx) } -func (r *recordingClient) UpdateProvider(ctx context.Context, p openshell.Provider) (openshell.Provider, error) { - return r.wrapped.UpdateProvider(ctx, p) -} - func (r *recordingClient) GetInferenceRoute(ctx context.Context, route string) (openshell.InferenceRoute, error) { return r.wrapped.GetInferenceRoute(ctx, route) } @@ -384,18 +376,10 @@ func (e *errorClient) DeleteSandbox(ctx context.Context, name string) error { return e.err } -func (e *errorClient) DeleteProvider(ctx context.Context, name string) error { - return e.err -} - func (e *errorClient) GatewayInfo(ctx context.Context) (openshell.GatewayInfo, error) { return openshell.GatewayInfo{}, e.err } -func (e *errorClient) UpdateProvider(ctx context.Context, p openshell.Provider) (openshell.Provider, error) { - return openshell.Provider{}, e.err -} - func (e *errorClient) GetInferenceRoute(ctx context.Context, route string) (openshell.InferenceRoute, error) { return openshell.InferenceRoute{}, e.err } diff --git a/internal/reconcile/provider.go b/internal/reconcile/provider.go deleted file mode 100644 index 3727799..0000000 --- a/internal/reconcile/provider.go +++ /dev/null @@ -1,127 +0,0 @@ -package reconcile - -import ( - "context" - "errors" - "fmt" - - "github.com/stackrox/harness-openshell/internal/config" - "github.com/stackrox/harness-openshell/internal/openshell" - "github.com/stackrox/harness-openshell/internal/plan" -) - -// ProviderResult reports what ReconcileProviders decided for one provider and the -// resulting (or current) firewall view of it. -// -// Provider holds: the gateway's response on Update; the current provider on Noop -// and on AdoptionRequired (read at diff time); and a bare {Name, Type} echo on -// Create, which reconcile deliberately does NOT write (invariant 26 — credentialed -// creation belongs to platform bootstrap). -type ProviderResult struct { - Name string - Action plan.Action - Provider openshell.Provider - // Adopted is true when this Update took ownership of a provider that existed - // but carried no harness owner label (adopt: true authorized it). It is a - // takeover, not an ordinary drift-correcting update, so callers surface it - // distinctly for the operator to audit. - Adopted bool -} - -// ReconcileProviders drives each desired provider toward the gateway state, -// routing every decision through the shared plan.ProviderAction rule so the -// read-only plan and this write path can never disagree (invariant 22). Like -// ReconcileInference it does not degrade: any non-NotFound read error, or any -// write error, is returned so the caller learns the reconcile did not complete. -// -// It never creates a credentialed provider and never deletes (invariant 26): -// - Create (managed absent) is reported without writing; platform bootstrap -// performs credentialed creation, after which a re-run sees it present. -// - AdoptionRequired for an existing-but-unowned provider is reported without -// writing (drift the operator must resolve with `adopt: true`). -// - AdoptionRequired for an ABSENT referenced provider is a hard error: a -// referenced provider that does not exist is unusable. -// -// On Update the write is credential-preserving by construction (the firewall -// Provider has no credentials field) and is reached only on a real non-secret -// delta, so the empty-credential copy-through is never sent spuriously. -func ReconcileProviders(ctx context.Context, c openshell.ProviderReconciler, desired []config.Provider) ([]ProviderResult, error) { - results := make([]ProviderResult, 0, len(desired)) - - for _, d := range desired { - cur, err := c.GetProvider(ctx, d.Name) - var curPtr *openshell.Provider - switch { - case err == nil: - curPtr = &cur - case errors.Is(err, openshell.ErrNotFound): - curPtr = nil - default: - return nil, fmt.Errorf("reading provider %q: %w", d.Name, err) - } - - action := plan.ProviderAction(d, curPtr) - switch action { - case plan.ActionNoop: - // cur is guaranteed present here (Noop implies an existing provider); a - // successful Get is itself the referenced-present verification. - results = append(results, ProviderResult{Name: d.Name, Action: action, Provider: cur}) - - case plan.ActionUpdate: - // An Update on a not-yet-owned provider is an adoption (adopt: true - // stamping the owner label for the first time), not a routine update. - adopted := curPtr != nil && !plan.IsOwned(*curPtr) - updated, err := c.UpdateProvider(ctx, managedProvider(d, curPtr)) - if err != nil { - return nil, fmt.Errorf("updating provider %q: %w", d.Name, err) - } - results = append(results, ProviderResult{Name: d.Name, Action: action, Provider: updated, Adopted: adopted}) - - case plan.ActionCreate: - // Managed absent: report the intended create; do NOT SDK-create. - results = append(results, ProviderResult{ - Name: d.Name, Action: action, - Provider: openshell.Provider{Name: d.Name, Type: d.Type}, - }) - - case plan.ActionAdoptionRequired: - if curPtr == nil { - // Referenced (or unknown-management) but absent: unusable. - return nil, fmt.Errorf("referenced provider %q does not exist: %w", d.Name, openshell.ErrNotFound) - } - // Exists but unowned and not adopted: report drift, write nothing. - results = append(results, ProviderResult{Name: d.Name, Action: action, Provider: cur}) - - default: - return nil, fmt.Errorf("unexpected provider action %q for %q", action, d.Name) - } - } - - return results, nil -} - -// managedProvider builds the openshell.Provider written on an Update. It merges -// the desired Config and the owner label ONTO the current provider's fields -// rather than replacing them, so an update triggered by one managed key never -// wipes config keys or labels the harness does not manage. This keeps the write -// consistent with the diff rule's subset semantics (plan.configDrifts): the -// harness owns only the keys it declares. The owner label is always stamped — -// on first adoption that stamp is itself the Label delta that made this an -// Update. cur is non-nil here (Update implies an existing provider). -func managedProvider(d config.Provider, cur *openshell.Provider) openshell.Provider { - cfg := map[string]string{} - for k, v := range cur.Config { - cfg[k] = v - } - for k, v := range d.Config { - cfg[k] = v - } - - labels := map[string]string{} - for k, v := range cur.Labels { - labels[k] = v - } - labels[plan.OwnerLabelKey] = plan.OwnerLabelValue - - return openshell.Provider{Name: d.Name, Type: d.Type, Config: cfg, Labels: labels} -} diff --git a/internal/reconcile/provider_test.go b/internal/reconcile/provider_test.go deleted file mode 100644 index 8c8bef8..0000000 --- a/internal/reconcile/provider_test.go +++ /dev/null @@ -1,314 +0,0 @@ -package reconcile - -import ( - "context" - "errors" - "testing" - - "github.com/NVIDIA/OpenShell/sdk/go/openshell/v1/types" - - "github.com/stackrox/harness-openshell/internal/config" - "github.com/stackrox/harness-openshell/internal/openshell" - "github.com/stackrox/harness-openshell/internal/plan" -) - -// ownerLabels returns the harness owner label set, marking a seeded provider as -// harness-owned so managed reconcile treats it as adoptable-in-place, not drift. -func ownerLabels() map[string]string { - return map[string]string{plan.OwnerLabelKey: plan.OwnerLabelValue} -} - -// TestReconcileProviders_ReferencedVerify: a referenced provider that exists is a -// noop — the successful Get is the verification — and nothing is written. -func TestReconcileProviders_ReferencedVerify(t *testing.T) { - ctx := context.Background() - c, raw := healthyClient(t) - raw.AddProvider("default", &types.Provider{Name: "ext", Type: "custom"}) - rec := &capturingProviderClient{Client: c} - - res, err := ReconcileProviders(ctx, rec, []config.Provider{ - {Name: "ext", Type: "custom", Management: "referenced"}, - }) - if err != nil { - t.Fatalf("ReconcileProviders: %v", err) - } - if len(res) != 1 || res[0].Action != plan.ActionNoop { - t.Fatalf("want one noop result, got %+v", res) - } - if rec.updateCalled { - t.Error("referenced verify must not write") - } -} - -// TestReconcileProviders_ManagedNoop: an owned managed provider whose config -// matches is a noop with no write. -func TestReconcileProviders_ManagedNoop(t *testing.T) { - ctx := context.Background() - c, raw := healthyClient(t) - raw.AddProvider("default", &types.Provider{ - Name: "gcp", Type: "google-vertex-ai", Labels: ownerLabels(), - Spec: types.ProviderSpec{Config: map[string]string{"VERTEX_AI_REGION": "global"}}, - }) - rec := &capturingProviderClient{Client: c} - - res, err := ReconcileProviders(ctx, rec, []config.Provider{ - {Name: "gcp", Type: "google-vertex-ai", Management: "managed", Config: map[string]string{"VERTEX_AI_REGION": "global"}}, - }) - if err != nil { - t.Fatalf("ReconcileProviders: %v", err) - } - if res[0].Action != plan.ActionNoop { - t.Errorf("action = %s, want noop", res[0].Action) - } - if rec.updateCalled { - t.Error("matching managed provider must not write") - } -} - -// TestReconcileProviders_ManagedUpdateConfigDrift: an owned managed provider with -// drifted config is updated, and the Provider handed to UpdateProvider carries -// the desired config merged over the current, plus the owner label — and extra -// unmanaged keys survive. -func TestReconcileProviders_ManagedUpdateConfigDrift(t *testing.T) { - ctx := context.Background() - c, raw := healthyClient(t) - raw.AddProvider("default", &types.Provider{ - Name: "gcp", Type: "google-vertex-ai", Labels: ownerLabels(), - Spec: types.ProviderSpec{ - Config: map[string]string{"VERTEX_AI_REGION": "global", "UNMANAGED": "keep"}, - Credentials: map[string]string{"API_KEY": "secret"}, - }, - }) - rec := &capturingProviderClient{Client: c} - - res, err := ReconcileProviders(ctx, rec, []config.Provider{ - {Name: "gcp", Type: "google-vertex-ai", Management: "managed", Config: map[string]string{"VERTEX_AI_REGION": "us-east1"}}, - }) - if err != nil { - t.Fatalf("ReconcileProviders: %v", err) - } - if res[0].Action != plan.ActionUpdate { - t.Fatalf("action = %s, want update", res[0].Action) - } - if !rec.updateCalled { - t.Fatal("config drift must trigger a write") - } - got := rec.updateArg - if got.Config["VERTEX_AI_REGION"] != "us-east1" { - t.Errorf("desired config not carried to Update: %v", got.Config) - } - if got.Config["UNMANAGED"] != "keep" { - t.Errorf("unmanaged config key wiped by update: %v", got.Config) - } - if !plan.IsOwned(got) { - t.Errorf("owner label not stamped on Update: %v", got.Labels) - } - // The fake's Get returns stored credentials, so the copy-through preserves - // them; this proves the overlay logic, not the real empty-map semantic (that - // is the S2 live gate's job). - stored, err := raw.Providers().Get(ctx, "default", "gcp") - if err != nil { - t.Fatalf("raw Get: %v", err) - } - if stored.Spec.Credentials["API_KEY"] != "secret" { - t.Errorf("credentials clobbered by update: %v", stored.Spec.Credentials) - } -} - -// TestReconcileProviders_AdoptStampsOwnerLabel: with adopt:true an unowned -// provider is taken over — the write stamps the owner label (the label delta that -// made this an update). -func TestReconcileProviders_AdoptStampsOwnerLabel(t *testing.T) { - ctx := context.Background() - c, raw := healthyClient(t) - raw.AddProvider("default", &types.Provider{Name: "gcp", Type: "google-vertex-ai"}) // no owner label - rec := &capturingProviderClient{Client: c} - - res, err := ReconcileProviders(ctx, rec, []config.Provider{ - {Name: "gcp", Type: "google-vertex-ai", Management: "managed", Adopt: true}, - }) - if err != nil { - t.Fatalf("ReconcileProviders: %v", err) - } - if res[0].Action != plan.ActionUpdate { - t.Fatalf("action = %s, want update", res[0].Action) - } - if !plan.IsOwned(rec.updateArg) { - t.Errorf("adopt did not stamp the owner label: %v", rec.updateArg.Labels) - } -} - -// TestReconcileProviders_UnownedAdoptionRequiredNoWrite: an existing unowned -// managed provider without adopt is reported adoption-required and never written. -func TestReconcileProviders_UnownedAdoptionRequiredNoWrite(t *testing.T) { - ctx := context.Background() - c, raw := healthyClient(t) - raw.AddProvider("default", &types.Provider{Name: "gcp", Type: "google-vertex-ai"}) - rec := &capturingProviderClient{Client: c} - - res, err := ReconcileProviders(ctx, rec, []config.Provider{ - {Name: "gcp", Type: "google-vertex-ai", Management: "managed"}, - }) - if err != nil { - t.Fatalf("ReconcileProviders: %v", err) - } - if res[0].Action != plan.ActionAdoptionRequired { - t.Errorf("action = %s, want adoption-required", res[0].Action) - } - if rec.updateCalled { - t.Error("adoption-required must not write an unowned provider") - } -} - -// TestReconcileProviders_ManagedAbsentReturnsCreateNoWrite: a managed provider -// that does not exist is reported create without any SDK write (invariant 26). -func TestReconcileProviders_ManagedAbsentReturnsCreateNoWrite(t *testing.T) { - ctx := context.Background() - c, _ := healthyClient(t) - rec := &capturingProviderClient{Client: c} - - res, err := ReconcileProviders(ctx, rec, []config.Provider{ - {Name: "gcp", Type: "google-vertex-ai", Management: "managed"}, - }) - if err != nil { - t.Fatalf("ReconcileProviders: %v", err) - } - if res[0].Action != plan.ActionCreate { - t.Errorf("action = %s, want create", res[0].Action) - } - if res[0].Provider.Name != "gcp" || res[0].Provider.Type != "google-vertex-ai" { - t.Errorf("create result should echo name/type: %+v", res[0].Provider) - } - if rec.updateCalled { - t.Error("reconcile must not SDK-create (invariant 26)") - } -} - -// TestReconcileProviders_ReferencedAbsentErrors: a referenced provider that does -// not exist is a hard error (unusable). -func TestReconcileProviders_ReferencedAbsentErrors(t *testing.T) { - ctx := context.Background() - c, _ := healthyClient(t) - - _, err := ReconcileProviders(ctx, c, []config.Provider{ - {Name: "ext", Management: "referenced"}, - }) - if !errors.Is(err, openshell.ErrNotFound) { - t.Fatalf("want ErrNotFound for absent referenced provider, got %v", err) - } -} - -// TestReconcileProviders_ReadErrorPropagates: a non-NotFound read error is -// returned, never degraded (a write path must report it did not run). -func TestReconcileProviders_ReadErrorPropagates(t *testing.T) { - ctx := context.Background() - base, _ := healthyClient(t) - for _, want := range []error{openshell.ErrUnavailable, openshell.ErrPermission} { - c := &providerGetErrClient{Client: base, err: want} - _, err := ReconcileProviders(ctx, c, []config.Provider{ - {Name: "gcp", Type: "google-vertex-ai", Management: "managed"}, - }) - if !errors.Is(err, want) { - t.Errorf("expected %v to propagate, got %v", want, err) - } - } -} - -// TestReconcileProviders_WriteErrorPropagates: an Update failure is returned. -func TestReconcileProviders_WriteErrorPropagates(t *testing.T) { - ctx := context.Background() - base, raw := healthyClient(t) - raw.AddProvider("default", &types.Provider{Name: "gcp", Type: "google-vertex-ai", Labels: ownerLabels()}) - c := &providerUpdateErrClient{Client: base, err: openshell.ErrPermission} - - _, err := ReconcileProviders(ctx, c, []config.Provider{ - {Name: "gcp", Type: "google-vertex-ai", Management: "managed", Config: map[string]string{"K": "v"}}, - }) - if !errors.Is(err, openshell.ErrPermission) { - t.Fatalf("expected write ErrPermission to propagate, got %v", err) - } -} - -// TestReconcileMatchesPlanProviderAction locks invariant 22: the read-only plan -// and the reconcile write agree on the action for the same gateway state (both -// route through plan.ProviderAction). -func TestReconcileMatchesPlanProviderAction(t *testing.T) { - ctx := context.Background() - desired := config.Provider{Name: "gcp", Type: "google-vertex-ai", Management: "managed"} - - cases := []struct { - name string - seed *types.Provider // nil = absent - want plan.Action - }{ - {name: "absent -> create", seed: nil, want: plan.ActionCreate}, - {name: "unowned -> adoption-required", seed: &types.Provider{Name: "gcp", Type: "google-vertex-ai"}, want: plan.ActionAdoptionRequired}, - {name: "owned match -> noop", seed: &types.Provider{Name: "gcp", Type: "google-vertex-ai", Labels: ownerLabels()}, want: plan.ActionNoop}, - {name: "owned type drift -> update", seed: &types.Provider{Name: "gcp", Type: "old", Labels: ownerLabels()}, want: plan.ActionUpdate}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - c, raw := healthyClient(t) - if tc.seed != nil { - raw.AddProvider("default", tc.seed) - } - - // Plan action from the read path. - var curPtr *openshell.Provider - cur, err := c.GetProvider(ctx, desired.Name) - switch { - case err == nil: - curPtr = &cur - case errors.Is(err, openshell.ErrNotFound): - curPtr = nil - default: - t.Fatalf("GetProvider: %v", err) - } - planAction := plan.ProviderAction(desired, curPtr) - - // Reconcile action from the write path against the same state. - res, err := ReconcileProviders(ctx, c, []config.Provider{desired}) - if err != nil { - t.Fatalf("ReconcileProviders: %v", err) - } - if planAction != tc.want || res[0].Action != tc.want { - t.Errorf("plan=%s reconcile=%s, want %s", planAction, res[0].Action, tc.want) - } - }) - } -} - -// capturingProviderClient records the last UpdateProvider argument while -// delegating to a real fake-backed client, so the outbound Provider can be -// asserted and writes can be detected. -type capturingProviderClient struct { - openshell.Client - updateCalled bool - updateArg openshell.Provider -} - -func (c *capturingProviderClient) UpdateProvider(ctx context.Context, p openshell.Provider) (openshell.Provider, error) { - c.updateCalled = true - c.updateArg = p - return c.Client.UpdateProvider(ctx, p) -} - -// providerGetErrClient forces GetProvider to a chosen error. -type providerGetErrClient struct { - openshell.Client - err error -} - -func (c *providerGetErrClient) GetProvider(context.Context, string) (openshell.Provider, error) { - return openshell.Provider{}, c.err -} - -// providerUpdateErrClient reads normally but forces UpdateProvider to an error. -type providerUpdateErrClient struct { - openshell.Client - err error -} - -func (c *providerUpdateErrClient) UpdateProvider(context.Context, openshell.Provider) (openshell.Provider, error) { - return openshell.Provider{}, c.err -} diff --git a/internal/status/status.go b/internal/status/status.go index f195a80..1610aae 100644 --- a/internal/status/status.go +++ b/internal/status/status.go @@ -84,16 +84,8 @@ func redactFromLiteral(value string) string { func OK(msg string) { fmt.Println(" ✓ " + msg) } func OKf(format string, a ...any) { fmt.Printf(" ✓ "+format+"\n", a...) } -func Fail(msg string) { fmt.Println(" ✗ " + msg) } -func Failf(format string, a ...any) { fmt.Printf(" ✗ "+format+"\n", a...) } func Warnf(format string, a ...any) { fmt.Printf(" ! "+format+"\n", a...) } -func Info(msg string) { fmt.Println(" - " + msg) } func Infof(format string, a ...any) { fmt.Printf(" - "+format+"\n", a...) } -func Section(title string) { fmt.Printf("\n=== %s ===\n", title) } -func Done(msg string) { - fmt.Println() - fmt.Println(msg) -} func Header(title string) { fmt.Printf("\n%s\n", title) diff --git a/main.go b/main.go index 15a0092..b8f5fc5 100644 --- a/main.go +++ b/main.go @@ -24,7 +24,7 @@ func main() { root := &cobra.Command{ Use: "harness", - Short: "OpenShell Harness — deploy and manage AI agent sandboxes", + Short: "Run agent workflows in OpenShell sandboxes", Version: version, SilenceErrors: true, SilenceUsage: true, diff --git a/test/configs/harness-v1alpha1.yaml b/test/configs/harness-v1alpha1.yaml index b1b9e20..6341674 100644 --- a/test/configs/harness-v1alpha1.yaml +++ b/test/configs/harness-v1alpha1.yaml @@ -9,7 +9,7 @@ spec: providers: - name: my-gcp type: google-vertex-ai - management: managed + management: referenced - name: github-fact management: referenced inference: From c5e37a0e09a3c5d71342b1d738fe38c4d42783ff Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 9 Sep 2026 12:31:24 -0700 Subject: [PATCH 2/8] fix: tolerate recoverable review comment locations --- examples/github-pr-reviewer/skills/pr-review/SKILL.md | 5 +++++ scripts/pr-review.sh | 4 +--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/examples/github-pr-reviewer/skills/pr-review/SKILL.md b/examples/github-pr-reviewer/skills/pr-review/SKILL.md index f0b115b..1bde7b2 100644 --- a/examples/github-pr-reviewer/skills/pr-review/SKILL.md +++ b/examples/github-pr-reviewer/skills/pr-review/SKILL.md @@ -30,4 +30,9 @@ the selected side, not the diff position or a guess. Target only added or context lines GitHub can resolve. Omit unresolvable locations. Use multi-line ranges only when both endpoints are present in the same hunk. +Before posting, verify that the target file and line are present in the current +diff and are on the RIGHT side. Do not post a guessed comment for a deleted file, +deleted line, or a line outside the supplied diff. If GitHub rejects a location, +continue the review without retrying that location. + If no substantive defect is supported, say so. Never reproduce secrets. diff --git a/scripts/pr-review.sh b/scripts/pr-review.sh index 35b0124..668c1ba 100644 --- a/scripts/pr-review.sh +++ b/scripts/pr-review.sh @@ -93,9 +93,7 @@ validate_agent_output() { any(.[]; .type == "text" and (.part.text | type == "string" and test("\\S"))) and any(.[]; .type == "step_finish" and .part.reason == "stop") and all(.[]; .type != "error" and - (.type != "tool_use" or - (.part.state.status == "completed" and - (.part.state.metadata.exit // -1) == 0)) and + (.type != "tool_use" or .part.state.status == "completed") and (.type != "step_finish" or .part.reason == "stop" or .part.reason == "tool-calls")) ' "$REVIEW_DIR/agent.ndjson" >/dev/null } From 6fe7ad175a9a69803c7294043bee49138ab608ad Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 9 Sep 2026 12:38:11 -0700 Subject: [PATCH 3/8] ci: make AI review opt-out label persistent --- .github/workflows/ai-review.yml | 13 ++++++++++++- scripts/pr-review.sh | 6 +++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml index 2e92ef7..69a2685 100644 --- a/.github/workflows/ai-review.yml +++ b/.github/workflows/ai-review.yml @@ -2,13 +2,24 @@ name: AI review on: pull_request_target: - types: [labeled, unlabeled, synchronize, reopened, ready_for_review, converted_to_draft, closed] + types: [opened, labeled, unlabeled, synchronize, reopened, ready_for_review, converted_to_draft, closed] permissions: contents: read pull-requests: write jobs: + ensure-label: + if: github.event.action == 'opened' + runs-on: ubuntu-latest + permissions: + pull-requests: write + steps: + - name: Add opt-in review label + env: + GH_TOKEN: ${{ github.token }} + run: gh pr edit "$GITHUB_SERVER_URL/$GITHUB_REPOSITORY/pull/${{ github.event.pull_request.number }}" --add-label ai-review + review: if: >- ((github.event.action == 'labeled' || github.event.action == 'unlabeled') && github.event.label.name == 'ai-review') || diff --git a/scripts/pr-review.sh b/scripts/pr-review.sh index 668c1ba..b9aaa3f 100644 --- a/scripts/pr-review.sh +++ b/scripts/pr-review.sh @@ -93,7 +93,11 @@ validate_agent_output() { any(.[]; .type == "text" and (.part.text | type == "string" and test("\\S"))) and any(.[]; .type == "step_finish" and .part.reason == "stop") and all(.[]; .type != "error" and - (.type != "tool_use" or .part.state.status == "completed") and + (.type != "tool_use" or + (.part.state.status == "completed" and + ((.part.state.metadata.exit // 0) == 0 or + ((.part.state.metadata.exit // 0) == 1 and + ((.part.state.output // .part.state.error // "") | test("422|unprocessable entity|comment.*(position|line)"; "i")))))) and (.type != "step_finish" or .part.reason == "stop" or .part.reason == "tool-calls")) ' "$REVIEW_DIR/agent.ndjson" >/dev/null } From f20eb97cc8f5c86ff7af27e6c8c623209c846561 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 9 Sep 2026 12:39:51 -0700 Subject: [PATCH 4/8] security: inject GitHub token through provider masking --- examples/github-pr-reviewer/opencode-harness.yaml | 5 ++++- scripts/pr-review.sh | 15 +++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/examples/github-pr-reviewer/opencode-harness.yaml b/examples/github-pr-reviewer/opencode-harness.yaml index 269a9f9..cf08140 100644 --- a/examples/github-pr-reviewer/opencode-harness.yaml +++ b/examples/github-pr-reviewer/opencode-harness.yaml @@ -7,6 +7,9 @@ spec: - name: vertex-review type: google-vertex-ai management: referenced + - name: github-review + type: github + management: referenced inference: route: inference.local provider: vertex-review @@ -15,10 +18,10 @@ spec: image: quay.io/rcochran/openshell@sha256:eda3ebb4a6a44de3715016cf912840f98f378083690f0c0c56f459b44b0dbdc8 policy: file: ${REVIEW_POLICY} + providers: [github-review] env: OPENCODE_CONFIG: /sandbox/opencode-review.json OPENCODE_VERTEX_API_KEY: sk-openshell-proxy-managed - GITHUB_TOKEN: ${GITHUB_TOKEN} REVIEW_REPOSITORY: ${REVIEW_REPOSITORY} REVIEW_PR: ${REVIEW_PR} REVIEW_HEAD: ${REVIEW_HEAD} diff --git a/scripts/pr-review.sh b/scripts/pr-review.sh index b9aaa3f..9f47f6e 100644 --- a/scripts/pr-review.sh +++ b/scripts/pr-review.sh @@ -11,7 +11,8 @@ mode="${1:?usage: pr-review.sh prepare|run}" gateway="${OPENSHELL_GATEWAY:-openshell}" workspace="rev-$RANDOM-$$" created_workspace=false -created_provider=false +created_vertex_provider=false +created_github_provider=false apply_pid="" head="${REVIEW_HEAD:-}" base="" @@ -41,9 +42,12 @@ cleanup_runtime() { fi if $created_workspace; then timeout 30s ./harness delete --gateway "$gateway" --workspace "$workspace" --sandboxes || cleanup_status=1 - if $created_provider; then + if $created_vertex_provider; then timeout 30s openshell provider delete --gateway "$gateway" --workspace "$workspace" vertex-review || cleanup_status=1 fi + if $created_github_provider; then + timeout 30s openshell provider delete --gateway "$gateway" --workspace "$workspace" github-review || cleanup_status=1 + fi timeout 30s openshell workspace delete --gateway "$gateway" "$workspace" || cleanup_status=1 fi return "$cleanup_status" @@ -108,14 +112,17 @@ run_review() { (cd "$REVIEW_DIR" && shasum -a 256 -c pr.diff.sha256 >/dev/null) ensure_current : "${GOOGLE_VERTEX_AI_TOKEN:?set a short-lived Vertex token}" "${VERTEX_AI_PROJECT_ID:?set Vertex project}" - : "${GITHUB_TOKEN:?set the workflow GitHub token}" + : "${GITHUB_TOKEN:?set the workflow GitHub token for provider bootstrap}" timeout 60s openshell workspace create --gateway "$gateway" --name "$workspace" created_workspace=true timeout 60s openshell provider create --gateway "$gateway" --workspace "$workspace" \ --name vertex-review --type google-vertex-ai --from-existing \ --config "VERTEX_AI_PROJECT_ID=$VERTEX_AI_PROJECT_ID" --config "VERTEX_AI_REGION=${VERTEX_AI_REGION:-global}" - created_provider=true + created_vertex_provider=true + timeout 60s openshell provider create --gateway "$gateway" --workspace "$workspace" \ + --name github-review --type github --credential GITHUB_TOKEN + created_github_provider=true timeout 60s openshell inference set --gateway "$gateway" --workspace "$workspace" \ --provider vertex-review --model gemini-2.5-pro --no-verify From 6460db54314408413ce8bd489960e5e2ef1cd965 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 9 Sep 2026 12:42:11 -0700 Subject: [PATCH 5/8] feat: add reusable PR review workflow --- .github/workflows/pr-review-reusable.yml | 100 +++++++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 .github/workflows/pr-review-reusable.yml diff --git a/.github/workflows/pr-review-reusable.yml b/.github/workflows/pr-review-reusable.yml new file mode 100644 index 0000000..f69c65f --- /dev/null +++ b/.github/workflows/pr-review-reusable.yml @@ -0,0 +1,100 @@ +name: Harness PR review + +on: + workflow_call: + inputs: + harness-ref: + description: Harness tag or commit to execute + required: false + type: string + default: main + skill-path: + description: Path to the trusted review skill in the caller repository + required: false + type: string + default: .github/skills/pr-review/SKILL.md + secrets: + VERTEX_AI_SERVICE_ACCOUNT_KEY: + required: true + +permissions: + contents: read + pull-requests: write + +jobs: + review: + if: >- + ((github.event.action == 'labeled' || github.event.action == 'unlabeled') && github.event.label.name == 'ai-review') || + (github.event.action != 'labeled' && github.event.action != 'unlabeled' && contains(github.event.pull_request.labels.*.name, 'ai-review')) + runs-on: ubuntu-latest + timeout-minutes: 15 + concurrency: + group: ai-review-${{ github.repository }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + steps: + # Checkout only default-branch content from the caller. The PR head is data. + - uses: actions/checkout@v7 + with: + ref: ${{ github.event.repository.default_branch }} + path: caller + persist-credentials: false + - uses: actions/checkout@v7 + with: + repository: stackrox/harness-openshell + ref: ${{ inputs.harness-ref }} + path: harness + persist-credentials: false + - name: Install caller skill + env: + SKILL_PATH: ${{ inputs.skill-path }} + run: | + [[ "$SKILL_PATH" != /* && "$SKILL_PATH" != *..* ]] || exit 1 + test -f "caller/$SKILL_PATH" + install -D -m 0644 "caller/$SKILL_PATH" harness/skills/pr-review/SKILL.md + - uses: actions/setup-go@v7 + with: + go-version-file: harness/go.mod + - name: Build trusted harness + working-directory: harness + run: echo "REVIEW_DIR=$RUNNER_TEMP/ai-review" >> "$GITHUB_ENV"; make cli + - name: Check label and fetch exact diff as data + id: prepare + working-directory: harness + env: + GH_TOKEN: ${{ github.token }} + REVIEW_REPOSITORY: ${{ github.repository }} + REVIEW_PR: ${{ github.event.pull_request.number }} + REVIEW_HEAD: ${{ github.event.pull_request.head.sha }} + run: bash scripts/pr-review.sh prepare + - uses: ./harness/.github/actions/setup-openshell + if: steps.prepare.outputs.eligible == 'true' + - name: Authenticate to Google Cloud + if: steps.prepare.outputs.eligible == 'true' + uses: google-github-actions/auth@7c6bc770dae815cd3e89ee6cdf493a5fab2cc093 # v3.0.0 + with: + project_id: ${{ vars.VERTEX_AI_PROJECT_ID }} + credentials_json: ${{ secrets.VERTEX_AI_SERVICE_ACCOUNT_KEY }} + - uses: google-github-actions/setup-gcloud@aa5489c8933f4cc7a4f7d45035b3b1440c9c10db # v3.0.1 + if: steps.prepare.outputs.eligible == 'true' + - name: Review in isolated OpenShell workspace + if: steps.prepare.outputs.eligible == 'true' + working-directory: harness + env: + GH_TOKEN: ${{ github.token }} + GITHUB_TOKEN: ${{ github.token }} + REVIEW_REPOSITORY: ${{ github.repository }} + REVIEW_PR: ${{ github.event.pull_request.number }} + REVIEW_HEAD: ${{ github.event.pull_request.head.sha }} + VERTEX_AI_PROJECT_ID: ${{ vars.VERTEX_AI_PROJECT_ID }} + VERTEX_AI_REGION: ${{ vars.VERTEX_AI_REGION }} + run: | + token="$(gcloud auth print-access-token)" + echo "::add-mask::$token" + export GOOGLE_VERTEX_AI_TOKEN="$token" + bash scripts/pr-review.sh run + - uses: actions/upload-artifact@v7 + if: always() && steps.prepare.outputs.eligible == 'true' + with: + name: ai-review-pr-${{ github.event.pull_request.number }}-${{ github.event.pull_request.head.sha }}-${{ github.run_attempt }} + path: ${{ runner.temp }}/ai-review/ + retention-days: 7 From c7c94cb1565dd1e68cb5439a22b2cd2a565bec1d Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 9 Sep 2026 12:42:54 -0700 Subject: [PATCH 6/8] ci: close review validation gaps --- .github/workflows/ai-review.yml | 7 +++++-- test/pr_review_test.go | 3 ++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml index 69a2685..797f944 100644 --- a/.github/workflows/ai-review.yml +++ b/.github/workflows/ai-review.yml @@ -21,9 +21,12 @@ jobs: run: gh pr edit "$GITHUB_SERVER_URL/$GITHUB_REPOSITORY/pull/${{ github.event.pull_request.number }}" --add-label ai-review review: + needs: ensure-label if: >- - ((github.event.action == 'labeled' || github.event.action == 'unlabeled') && github.event.label.name == 'ai-review') || - (github.event.action != 'labeled' && github.event.action != 'unlabeled' && contains(github.event.pull_request.labels.*.name, 'ai-review')) + always() && + (github.event.action != 'opened' || needs.ensure-label.result == 'success') && + (((github.event.action == 'labeled' || github.event.action == 'unlabeled') && github.event.label.name == 'ai-review') || + (github.event.action != 'labeled' && github.event.action != 'unlabeled' && contains(github.event.pull_request.labels.*.name, 'ai-review'))) concurrency: group: ai-review-${{ github.event.pull_request.number }} cancel-in-progress: true diff --git a/test/pr_review_test.go b/test/pr_review_test.go index 19e0b33..da32720 100644 --- a/test/pr_review_test.go +++ b/test/pr_review_test.go @@ -17,7 +17,7 @@ func TestPRReview(t *testing.T) { if err != nil { t.Fatal(err) } - for _, scenario := range []string{"success", "unlabeled", "stale", "oversized", "tampered", "agent-failure", "provider-failure", "cleanup-failure", "cancel", "truncated", "incomplete", "empty", "error", "tool_use"} { + for _, scenario := range []string{"success", "unlabeled", "stale", "oversized", "tampered", "agent-failure", "provider-failure", "cleanup-failure", "cancel", "truncated", "incomplete", "empty", "error", "tool_use", "tool_exit"} { t.Run(scenario, func(t *testing.T) { root := t.TempDir() stepSummary := filepath.Join(root, "step-summary") @@ -147,6 +147,7 @@ case "$1 ${2:-}" in incomplete) exit 0 ;; truncated) printf '%s\n' '{"type":"step_finish","part":{"reason":"length"}}' ;; error|tool_use) printf '{"type":"%s"}\n' "$FAKE_SCENARIO" ;; + tool_exit) printf '%s\n' '{"type":"tool_use","part":{"state":{"status":"completed","metadata":{"exit":7},"output":"ordinary command failed"}}}' ;; *) printf '%s\n' '{"type":"step_finish","part":{"reason":"stop"}}' ;; esac ;; esac From 42d1dc1bf5a579a033b4db8c2bcdce265b349821 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 9 Sep 2026 15:33:48 -0700 Subject: [PATCH 7/8] ci: pin reusable workflow revisions --- .github/workflows/pr-review-reusable.yml | 9 ++++++--- scripts/pr-review.sh | 4 ++-- test/pr_review_test.go | 3 ++- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/.github/workflows/pr-review-reusable.yml b/.github/workflows/pr-review-reusable.yml index f69c65f..3a8c0c1 100644 --- a/.github/workflows/pr-review-reusable.yml +++ b/.github/workflows/pr-review-reusable.yml @@ -4,10 +4,9 @@ on: workflow_call: inputs: harness-ref: - description: Harness tag or commit to execute - required: false + description: Immutable 40-character Harness commit SHA to execute + required: true type: string - default: main skill-path: description: Path to the trusted review skill in the caller repository required: false @@ -32,6 +31,10 @@ jobs: group: ai-review-${{ github.repository }}-${{ github.event.pull_request.number }} cancel-in-progress: true steps: + - name: Validate immutable Harness revision + env: + HARNESS_REF: ${{ inputs.harness-ref }} + run: '[[ "$HARNESS_REF" =~ ^[0-9a-f]{40}$ ]]' # Checkout only default-branch content from the caller. The PR head is data. - uses: actions/checkout@v7 with: diff --git a/scripts/pr-review.sh b/scripts/pr-review.sh index 9f47f6e..5e9d9ce 100644 --- a/scripts/pr-review.sh +++ b/scripts/pr-review.sh @@ -99,8 +99,8 @@ validate_agent_output() { all(.[]; .type != "error" and (.type != "tool_use" or (.part.state.status == "completed" and - ((.part.state.metadata.exit // 0) == 0 or - ((.part.state.metadata.exit // 0) == 1 and + ((.part.state.metadata.exit // -1) == 0 or + ((.part.state.metadata.exit // -1) == 1 and ((.part.state.output // .part.state.error // "") | test("422|unprocessable entity|comment.*(position|line)"; "i")))))) and (.type != "step_finish" or .part.reason == "stop" or .part.reason == "tool-calls")) ' "$REVIEW_DIR/agent.ndjson" >/dev/null diff --git a/test/pr_review_test.go b/test/pr_review_test.go index da32720..825778d 100644 --- a/test/pr_review_test.go +++ b/test/pr_review_test.go @@ -17,7 +17,7 @@ func TestPRReview(t *testing.T) { if err != nil { t.Fatal(err) } - for _, scenario := range []string{"success", "unlabeled", "stale", "oversized", "tampered", "agent-failure", "provider-failure", "cleanup-failure", "cancel", "truncated", "incomplete", "empty", "error", "tool_use", "tool_exit"} { + for _, scenario := range []string{"success", "unlabeled", "stale", "oversized", "tampered", "agent-failure", "provider-failure", "cleanup-failure", "cancel", "truncated", "incomplete", "empty", "error", "tool_use", "tool_exit", "tool_missing_exit"} { t.Run(scenario, func(t *testing.T) { root := t.TempDir() stepSummary := filepath.Join(root, "step-summary") @@ -148,6 +148,7 @@ case "$1 ${2:-}" in truncated) printf '%s\n' '{"type":"step_finish","part":{"reason":"length"}}' ;; error|tool_use) printf '{"type":"%s"}\n' "$FAKE_SCENARIO" ;; tool_exit) printf '%s\n' '{"type":"tool_use","part":{"state":{"status":"completed","metadata":{"exit":7},"output":"ordinary command failed"}}}' ;; + tool_missing_exit) printf '%s\n' '{"type":"tool_use","part":{"state":{"status":"completed","metadata":{},"output":"missing exit"}}}' ;; *) printf '%s\n' '{"type":"step_finish","part":{"reason":"stop"}}' ;; esac ;; esac From dc4b5e4e4b4be6c53709bb406251a6a63c527711 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Wed, 9 Sep 2026 16:12:02 -0700 Subject: [PATCH 8/8] ci: make caller review skill optional --- .github/workflows/pr-review-reusable.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/pr-review-reusable.yml b/.github/workflows/pr-review-reusable.yml index 3a8c0c1..d18ad44 100644 --- a/.github/workflows/pr-review-reusable.yml +++ b/.github/workflows/pr-review-reusable.yml @@ -11,7 +11,7 @@ on: description: Path to the trusted review skill in the caller repository required: false type: string - default: .github/skills/pr-review/SKILL.md + default: '' secrets: VERTEX_AI_SERVICE_ACCOUNT_KEY: required: true @@ -47,7 +47,11 @@ jobs: ref: ${{ inputs.harness-ref }} path: harness persist-credentials: false + - name: Prepare default review skill + working-directory: harness + run: install -D -m 0644 examples/github-pr-reviewer/skills/pr-review/SKILL.md skills/pr-review/SKILL.md - name: Install caller skill + if: inputs.skill-path != '' env: SKILL_PATH: ${{ inputs.skill-path }} run: |