diff --git a/CLAUDE.md b/CLAUDE.md index aebb031d..b6ad2dd7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -5,8 +5,9 @@ Kubernetes operator (Go, Kubebuilder/Operator SDK) that manages OpenClaw instances on OpenShift/Kubernetes. Detailed architecture reference in `docs/architecture.md`. **CRDs** (API group `claw.sandbox.redhat.com/v1alpha1`): -- `Claw` — Main CRD. Spec: `config` (ConfigSpec: raw RawExtension, mergeMode merge/overwrite), `credentials` ([]CredentialSpec), `auth` (AuthSpec: mode token/password, passwordSecretRef, disableDevicePairing). Status: `conditions`, `url`, `gatewayTokenSecretRef` +- `Claw` — Main CRD. Spec: `config` (ConfigSpec: raw RawExtension, mergeMode merge/overwrite/seedOnly), `credentials` ([]CredentialSpec), `auth` (AuthSpec: mode token/password, passwordSecretRef, disableDevicePairing). Status: `conditions`, `url`, `gatewayTokenSecretRef` - `ClawDevicePairingRequest` — Device pairing. Spec: `requestID`, `selector` (LabelSelector). Controller matches exactly one pod +- `ClawOperatorConfig` — Cluster-admin policy singleton (name `cluster`, in the operator's own namespace). Spec: `allowedConfigModes` ([]ConfigMode, empty = unrestricted). Gates which `mergeMode` values `Claw` CRs may use; see [ADR-0021](docs/adr/0021-seed-only-config-mode.md) ## Common Commands @@ -47,11 +48,11 @@ Detailed architecture reference: @docs/architecture.md ## Key Directories -- `api/v1alpha1/` -- CRD types (`claw_types.go`, `clawdevicepairingrequest_types.go`). Condition/annotation constants live here -- `internal/controller/` -- Reconciler + tests. Key files: `claw_resource_controller.go` (main reconciler), `claw_credentials.go` (credential validation, gateway secret), `claw_providers.go` (centralized `knownProviders` registry, provider defaults/routing), `claw_proxy.go` (proxy config), `claw_deployment.go` (deployment configuration), `claw_status.go` (status updates), `claw_models.go` (model catalog accessor, delegates to `knownProviders`), `claw_auth.go` (auth mode + device pairing) +- `api/v1alpha1/` -- CRD types (`claw_types.go`, `clawdevicepairingrequest_types.go`, `clawoperatorconfig_types.go`). Condition/annotation constants live here +- `internal/controller/` -- Reconciler + tests. Key files: `claw_resource_controller.go` (main reconciler), `claw_credentials.go` (credential validation, gateway secret), `claw_providers.go` (centralized `knownProviders` registry, provider defaults/routing), `claw_proxy.go` (proxy config), `claw_deployment.go` (deployment configuration), `claw_status.go` (status updates), `claw_models.go` (model catalog accessor, delegates to `knownProviders`), `claw_auth.go` (auth mode + device pairing), `claw_operator_config.go` (ClawOperatorConfig mergeMode gating) - `internal/proxy/` -- MITM proxy library - `internal/assets/manifests/` -- Embedded Kustomize manifests (two components: `claw/`, `claw-proxy/`) -- `cmd/main.go` -- Manager entrypoint (reads `PROXY_IMAGE`, `KUBECTL_IMAGE`, `IMAGE_PULL_POLICY` env vars) +- `cmd/main.go` -- Manager entrypoint (reads `PROXY_IMAGE`, `KUBECTL_IMAGE`, `IMAGE_PULL_POLICY`, `WATCH_NAMESPACE` env vars) - `cmd/proxy/main.go` -- Proxy binary entrypoint - `config/` -- Kustomize overlays for CRDs, RBAC, manager deployment diff --git a/api/v1alpha1/claw_types.go b/api/v1alpha1/claw_types.go index ba79f182..efe8a727 100644 --- a/api/v1alpha1/claw_types.go +++ b/api/v1alpha1/claw_types.go @@ -37,16 +37,25 @@ const ( ) // ConfigMode controls how operator.json is merged into the user's openclaw.json -// at pod start time. -// +kubebuilder:validation:Enum=merge;overwrite +// at pod start time. Not to be confused with SeedMode below, which governs +// per-file workspace seeding (spec.workspace.*.mode) — ConfigMode governs the +// whole openclaw.json file (spec.config.mergeMode). +// +kubebuilder:validation:Enum=merge;overwrite;seedOnly type ConfigMode string const ( ConfigModeMerge ConfigMode = "merge" ConfigModeOverwrite ConfigMode = "overwrite" + // ConfigModeSeedOnly seeds openclaw.json once on first boot, then leaves + // user/agent-owned keys untouched on subsequent restarts; a narrow, + // always-enforced subset of infrastructure/credential keys is still + // reasserted on every restart. See docs/adr/0021-seed-only-config-mode.md. + ConfigModeSeedOnly ConfigMode = "seedOnly" ) -// SeedMode controls how workspace files are seeded into the PVC. +// SeedMode controls how workspace files are seeded into the PVC +// (spec.workspace.*.mode) — a different, per-file mechanism from ConfigMode's +// seedOnly value above, despite the similar naming. // +kubebuilder:validation:Enum=overwrite;seedIfMissing type SeedMode string @@ -86,14 +95,16 @@ const ( // Condition reasons for Claw status. const ( - ConditionReasonReady = "Ready" - ConditionReasonProvisioning = "Provisioning" - ConditionReasonResolved = "Resolved" - ConditionReasonValidationFailed = "ValidationFailed" - ConditionReasonConfigured = "Configured" - ConditionReasonConfigFailed = "ConfigFailed" - ConditionReasonIdle = "Idle" - ConditionReasonIdledByRequest = "IdledByRequest" + ConditionReasonReady = "Ready" + ConditionReasonProvisioning = "Provisioning" + ConditionReasonResolved = "Resolved" + ConditionReasonValidationFailed = "ValidationFailed" + ConditionReasonConfigured = "Configured" + ConditionReasonConfigFailed = "ConfigFailed" + ConditionReasonIdle = "Idle" + ConditionReasonIdledByRequest = "IdledByRequest" + ConditionReasonIdledByPolicy = "IdledByPolicy" + ConditionReasonConfigModeNotAllowed = "ConfigModeNotAllowed" ) // SecretRefEntry references a specific key in a Secret. @@ -369,9 +380,12 @@ type ConfigSpec struct { // MergeMode controls how operator config is applied on pod start. // "merge" (default) deep-merges operator settings into the existing // user config, preserving user-owned keys. "overwrite" fully replaces - // the config on every pod start. + // the config on every pod start. "seedOnly" seeds the config once on + // first boot, then leaves it untouched except for a narrow, + // always-enforced infrastructure/credential subset. A cluster admin may + // restrict which modes are allowed via ClawOperatorConfig. // +optional - // +kubebuilder:validation:Enum=merge;overwrite + // +kubebuilder:validation:Enum=merge;overwrite;seedOnly // +kubebuilder:default=merge MergeMode ConfigMode `json:"mergeMode,omitempty"` } diff --git a/api/v1alpha1/clawoperatorconfig_types.go b/api/v1alpha1/clawoperatorconfig_types.go new file mode 100644 index 00000000..abcfe147 --- /dev/null +++ b/api/v1alpha1/clawoperatorconfig_types.go @@ -0,0 +1,65 @@ +/* +Copyright 2026 Red Hat. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// ClawOperatorConfigSingletonName is the required name of the single +// ClawOperatorConfig instance the operator reads. Any other name is ignored. +const ClawOperatorConfigSingletonName = "cluster" + +// ClawOperatorConfigSpec defines cluster-admin policy for the claw-operator. +type ClawOperatorConfigSpec struct { + // AllowedConfigModes restricts which spec.config.mergeMode values Claw CRs + // in this cluster may use. Empty/absent means all modes are allowed + // (fail-open) — this matches today's unrestricted behavior until an admin + // explicitly opts in to restricting it. + // +optional + AllowedConfigModes []ConfigMode `json:"allowedConfigModes,omitempty"` +} + +// +kubebuilder:object:root=true +// +kubebuilder:resource:path=clawoperatorconfigs,scope=Namespaced +// +kubebuilder:validation:XValidation:rule="self.metadata.name == 'cluster'",message="ClawOperatorConfig must be named 'cluster'" + +// ClawOperatorConfig is the Schema for the clawoperatorconfigs API. A single +// instance named "cluster" must live in the operator's own runtime namespace +// (resolved via the WATCH_NAMESPACE environment variable) — it is never +// looked up in, or honored from, a tenant namespace. The name is enforced +// at admission time by the XValidation rule above (ClawOperatorConfigSingletonName). +type ClawOperatorConfig struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + + // +kubebuilder:validation:Required + Spec ClawOperatorConfigSpec `json:"spec"` +} + +// +kubebuilder:object:root=true + +// ClawOperatorConfigList contains a list of ClawOperatorConfig +type ClawOperatorConfigList struct { + metav1.TypeMeta `json:",inline"` + metav1.ListMeta `json:"metadata,omitempty"` + Items []ClawOperatorConfig `json:"items"` +} + +func init() { + SchemeBuilder.Register(&ClawOperatorConfig{}, &ClawOperatorConfigList{}) +} diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 2db795fb..db5d8a0b 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -222,6 +222,84 @@ func (in *ClawList) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ClawOperatorConfig) DeepCopyInto(out *ClawOperatorConfig) { + *out = *in + out.TypeMeta = in.TypeMeta + in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) + in.Spec.DeepCopyInto(&out.Spec) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClawOperatorConfig. +func (in *ClawOperatorConfig) DeepCopy() *ClawOperatorConfig { + if in == nil { + return nil + } + out := new(ClawOperatorConfig) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *ClawOperatorConfig) DeepCopyObject() runtime.Object { + if c := in.DeepCopy(); c != nil { + return c + } + return nil +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ClawOperatorConfigList) DeepCopyInto(out *ClawOperatorConfigList) { + *out = *in + out.TypeMeta = in.TypeMeta + in.ListMeta.DeepCopyInto(&out.ListMeta) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]ClawOperatorConfig, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClawOperatorConfigList. +func (in *ClawOperatorConfigList) DeepCopy() *ClawOperatorConfigList { + if in == nil { + return nil + } + out := new(ClawOperatorConfigList) + in.DeepCopyInto(out) + return out +} + +// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. +func (in *ClawOperatorConfigList) DeepCopyObject() runtime.Object { + if c := in.DeepCopy(); c != nil { + return c + } + return nil +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ClawOperatorConfigSpec) DeepCopyInto(out *ClawOperatorConfigSpec) { + *out = *in + if in.AllowedConfigModes != nil { + in, out := &in.AllowedConfigModes, &out.AllowedConfigModes + *out = make([]ConfigMode, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClawOperatorConfigSpec. +func (in *ClawOperatorConfigSpec) DeepCopy() *ClawOperatorConfigSpec { + if in == nil { + return nil + } + out := new(ClawOperatorConfigSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ClawSpec) DeepCopyInto(out *ClawSpec) { *out = *in diff --git a/cmd/main.go b/cmd/main.go index 177f2df1..e4d78569 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -262,6 +262,17 @@ func main() { os.Exit(1) } + // WATCH_NAMESPACE identifies the operator's own runtime namespace (populated + // via the Kubernetes downward API), used to look up the ClawOperatorConfig + // admin-policy singleton — never a tenant namespace. Required: fails fast if + // unset, since a silently-empty value would make the singleton lookup + // ambiguous. + operatorNamespace := os.Getenv("WATCH_NAMESPACE") + if operatorNamespace == "" { + setupLog.Error(nil, "WATCH_NAMESPACE environment variable must be set") + os.Exit(1) + } + clawReconciler := &controller.ClawResourceReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), @@ -272,6 +283,7 @@ func main() { GitSyncImage: os.Getenv("GIT_SYNC_IMAGE"), OTelCollectorImage: os.Getenv("OTEL_COLLECTOR_IMAGE"), ImagePullPolicy: imagePullPolicy, + OperatorNamespace: operatorNamespace, MetricsRefreshed: make(chan struct{}), } if err = clawReconciler.SetupWithManager(mgr); err != nil { diff --git a/config/crd/bases/claw.sandbox.redhat.com_clawoperatorconfigs.yaml b/config/crd/bases/claw.sandbox.redhat.com_clawoperatorconfigs.yaml new file mode 100644 index 00000000..ce79ca28 --- /dev/null +++ b/config/crd/bases/claw.sandbox.redhat.com_clawoperatorconfigs.yaml @@ -0,0 +1,74 @@ +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.19.0 + name: clawoperatorconfigs.claw.sandbox.redhat.com +spec: + group: claw.sandbox.redhat.com + names: + kind: ClawOperatorConfig + listKind: ClawOperatorConfigList + plural: clawoperatorconfigs + singular: clawoperatorconfig + scope: Namespaced + versions: + - name: v1alpha1 + schema: + openAPIV3Schema: + description: |- + ClawOperatorConfig is the Schema for the clawoperatorconfigs API. A single + instance named "cluster" must live in the operator's own runtime namespace + (resolved via the WATCH_NAMESPACE environment variable) — it is never + looked up in, or honored from, a tenant namespace. The name is enforced + at admission time by the XValidation rule above (ClawOperatorConfigSingletonName). + properties: + apiVersion: + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources + type: string + kind: + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds + type: string + metadata: + type: object + spec: + description: ClawOperatorConfigSpec defines cluster-admin policy for the + claw-operator. + properties: + allowedConfigModes: + description: |- + AllowedConfigModes restricts which spec.config.mergeMode values Claw CRs + in this cluster may use. Empty/absent means all modes are allowed + (fail-open) — this matches today's unrestricted behavior until an admin + explicitly opts in to restricting it. + items: + description: |- + ConfigMode controls how operator.json is merged into the user's openclaw.json + at pod start time. Not to be confused with SeedMode below, which governs + per-file workspace seeding (spec.workspace.*.mode) — ConfigMode governs the + whole openclaw.json file (spec.config.mergeMode). + enum: + - merge + - overwrite + - seedOnly + type: string + type: array + type: object + required: + - spec + type: object + x-kubernetes-validations: + - message: ClawOperatorConfig must be named 'cluster' + rule: self.metadata.name == 'cluster' + served: true + storage: true diff --git a/config/crd/bases/claw.sandbox.redhat.com_claws.yaml b/config/crd/bases/claw.sandbox.redhat.com_claws.yaml index dd8e1123..58729f6e 100644 --- a/config/crd/bases/claw.sandbox.redhat.com_claws.yaml +++ b/config/crd/bases/claw.sandbox.redhat.com_claws.yaml @@ -108,15 +108,20 @@ spec: - enum: - merge - overwrite + - seedOnly - enum: - merge - overwrite + - seedOnly default: merge description: |- MergeMode controls how operator config is applied on pod start. "merge" (default) deep-merges operator settings into the existing user config, preserving user-owned keys. "overwrite" fully replaces - the config on every pod start. + the config on every pod start. "seedOnly" seeds the config once on + first boot, then leaves it untouched except for a narrow, + always-enforced infrastructure/credential subset. A cluster admin may + restrict which modes are allowed via ClawOperatorConfig. type: string raw: description: |- diff --git a/config/crd/kustomization.yaml b/config/crd/kustomization.yaml index 9c1d236e..cc18ffb9 100644 --- a/config/crd/kustomization.yaml +++ b/config/crd/kustomization.yaml @@ -4,6 +4,7 @@ resources: - bases/claw.sandbox.redhat.com_claws.yaml - bases/claw.sandbox.redhat.com_clawdevicepairingrequests.yaml +- bases/claw.sandbox.redhat.com_clawoperatorconfigs.yaml # +kubebuilder:scaffold:crdkustomizeresource # [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix. diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index 128ac8be..e6447d18 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -67,6 +67,10 @@ spec: imagePullPolicy: IfNotPresent name: manager env: + - name: WATCH_NAMESPACE + valueFrom: + fieldRef: + fieldPath: metadata.namespace - name: PROXY_IMAGE value: claw-proxy:latest - name: KUBECTL_IMAGE diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index cd8d392a..c6c78c8a 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -8,10 +8,11 @@ rules: - "" resources: - configmaps - - persistentvolumeclaims - - secrets + - serviceaccounts + - services verbs: - create + - delete - get - list - patch @@ -20,28 +21,27 @@ rules: - apiGroups: - "" resources: - - pods + - persistentvolumeclaims + - secrets verbs: + - create + - get - list + - patch + - update + - watch - apiGroups: - "" resources: - - pods/exec + - pods verbs: - - create + - list - apiGroups: - "" resources: - - serviceaccounts - - services + - pods/exec verbs: - create - - delete - - get - - list - - patch - - update - - watch - apiGroups: - apps resources: @@ -84,6 +84,7 @@ rules: - apiGroups: - claw.sandbox.redhat.com resources: + - clawoperatorconfigs - claws verbs: - get diff --git a/docs/adr/0021-seed-only-config-mode.md b/docs/adr/0021-seed-only-config-mode.md new file mode 100644 index 00000000..ea6bfc04 --- /dev/null +++ b/docs/adr/0021-seed-only-config-mode.md @@ -0,0 +1,244 @@ +# ADR-0021: `seedOnly` Config Mode + `ClawOperatorConfig` Admin Gating + +**Status:** Implemented +**Date:** 2026-07-06 + +## Overview + +`spec.config.mergeMode` today offers two behaviors, both of which re-apply +operator-managed `openclaw.json` keys (`gateway.*`, `models.providers`, +`channels.*`, `mcp.servers`, model catalog, etc.) on **every** pod restart, +forever: + +- `merge` (default): operator keys always win on collision; user-owned keys + (agents list, non-declared plugins/channels, tools, cron, etc.) persist. +- `overwrite`: PVC state is ignored entirely; config is rebuilt from scratch + every restart. + +This addresses [GitHub issue #224](https://github.com/codeready-toolchain/claw-operator/issues/224), +which asks for a third mode where the operator seeds `openclaw.json` **once** +on first boot, then treats the file as belonging to the user/agent from then +on. A narrow set of infrastructure/security keys remain operator-enforced +regardless of mode, since those aren't safe to delegate — this includes +credentialed or proxy-routed MCP servers, whose entry is reasserted on every +restart just like a declared provider or channel (see the two-bucket model +below); everything else, including MCP servers with no credential or proxy +constraint, becomes durable user state instead of "whatever key the operator +doesn't currently recognize." + +This matters for interactive/agentic workflows where the running OpenClaw +instance (or the user, via the UI/CLI) legitimately mutates its own +configuration over time — installing plugins, registering custom providers, +tuning MCP servers — and expects those changes to survive restarts. + +This ADR records the as-implemented decisions from the original design +exploration (two-bucket ownership analysis and five resolved open questions +around gating, ownership boundary, convergence, and naming). + +> **Naming disambiguation:** `seedOnly` (this ADR) is a `ConfigMode` value +> (`spec.config.mergeMode`) governing the *whole* `openclaw.json` file. It is +> unrelated to the pre-existing `SeedMode` type's `seedIfMissing` value +> (`spec.workspace.*.mode`), which governs *per-file* workspace seeding. The +> similar names are coincidental; see the doc-comments on `ConfigMode` and +> `SeedMode` in `api/v1alpha1/claw_types.go` for the cross-reference. + +## Design Principles + +1. **Opt-in, not default.** Existing `merge`/`overwrite` behavior is + unchanged. A CR must explicitly request `seedOnly`. +2. **Security/network boundaries are never delegated.** Regardless of mode, + the operator remains the sole authority over gateway bind/port, auth mode, + credential injection (proxy), Secrets, NetworkPolicies, and RBAC. +3. **"Seed once" is a natural extension of existing behavior**, not a new + concept — first-run seeding into an empty PVC already happens today. The + new mode changes what happens on *subsequent* restarts only. +4. **No new convergence mechanism is needed.** The existing + reconcile → recompute `operator.json` → hash → conditional rollout + pipeline already delivers fresh operator-desired state to every pod on + every relevant trigger. `seedOnly` only changes what `merge.js` *does* + with that fresh state once it reaches the pod. +5. **Fail safe, not silent.** If a cluster admin disallows a mode, a Claw + that requests it gets clear, visible feedback (`Ready: False`), not a + silent fallback to a different mode. This applies whether the Claw is + brand new or already running — see "Cluster-Admin Gating" below. + +## Ownership Boundary: The Two-Bucket Model + +Several keys that look like ordinary "operator-declared app config" +(`models.providers`, credential-bearing `channels.*`/`mcp.servers` entries) +**cannot actually be operated by a user hand-editing the file**, in any mode +— e.g. a provider's `apiKey` is always the literal placeholder string the +proxy intercepts, and the pod's `NetworkPolicy` only permits egress the CR +declares. Treating these as "user-owned" would be a promise the architecture +can't keep. + +This gives a precise test: **would hand-editing this field directly in +`openclaw.json`, with no CR change, actually produce a working +configuration?** If no (because it needs a Secret, NetworkPolicy rule, proxy +route, or Deployment env var only reconcile can create), the field is +**Bucket A** — always operator-managed, in every mode. If yes, it's **Bucket +B** — a genuine candidate for `seedOnly` freezing. + +### Bucket A — always operator-managed + +- **Pure infrastructure/security**, reasserted unconditionally by `merge.js` + in every mode: `gateway.mode`/`.bind`/`.port`/`.controlUi.enabled`, + `gateway.auth.mode`, `gateway.controlUi.dangerouslyDisableDeviceAuth`, + `gateway.trustedProxies`, the Route-host entry in + `gateway.controlUi.allowedOrigins` (append-only), `tools.web.search.*`, + `tools.web.fetch.enabled`, `agents.defaults.memorySearch.*`, + `diagnostics.otel.{metrics,metricsEndpoint}`. Credential injection itself + never touches `openclaw.json` at all — it happens transparently at the + MITM proxy layer. +- **Credential/routing-critical sub-fields** of a CR-declared + provider/channel/MCP-server entry — only these specific sub-fields are + reasserted; everything else in the same entry is Bucket B: + - `models.providers..{baseUrl,apiKey,api}` + - `channels..{enabled,botToken,token,appToken}` (the existing + `protectedChannelKeys` allowlist) + - `mcp.servers.` — **full-entry** replace, but only for servers using + `envFrom`/`credentialRef` or reached via URL (proxy-routed). `merge.js` + can't infer this from JSON shape alone (a `command`+`env` entry looks + identical either way), so `injectMcpServers` (`claw_mcp.go`) writes a + private `_seedOnlyMeta.mcpBucketAServers` marker into `operator.json` for + `merge.js` to read — stripped before the file ever reaches the PVC, in + every mode. + +### Bucket B — user-manageable, gated by mode + +Everything that passes the hand-edit test: `agents.defaults.model.primary`/ +`.fallbacks`, `agents.defaults.models` (catalog aliases), a provider's local +`.models` array, a channel's non-Bucket-A fields (`dmPolicy`, `allowFrom`, +etc.), a command+inline-env MCP server's full entry, `agents.list`, `cron.*`, +non-declared channels/MCP servers/plugins, and skill docs. + +### Reassertion + gap-fill mechanism + +For each provider/channel/MCP-server key present in **both** the freshly +computed `operator.json` and the existing PVC file, `merge.js` overwrites +only the Bucket-A sub-fields above, leaving the rest of the entry untouched. +This is a **path-level** reassertion, not `merge` mode's whole-key +`deepMerge` — a materially more error-prone shape of logic, since +under-reasserting silently leaves a security field unprotected while +over-reasserting silently clobbers a user customization, and neither failure +surfaces until the *second-or-later* restart of an already-seeded instance. +See `internal/controller/claw_merge_test.go`'s `TestMergeJSSeedOnly` for the +exhaustive test matrix this required. + +A second, shallow **gap-fill pass** adds any top-level key present in +`operator.json`'s Bucket-B collections (`models.providers`, `channels`, +`mcp.servers`, `agents.defaults.models`) but absent from the PVC file — this +is what makes "add a new provider/channel/MCP server to the CR" work +automatically, without requiring an opt-in step. It never recurses into an +already-existing entry's fields. The same pass fills +`agents.defaults.model.primary`/`.fallbacks` only if currently absent/empty, +mirroring the pre-existing `merge`-mode carve-out. + +**Known accepted rough edges** (documented, not solved by this ADR): +- Removing a declared credential/channel/MCP server from the CR after + seeding leaves an orphaned stub in the PVC file — its Bucket-A fields + point at a proxy route/Secret that no longer exists and start failing at + the network layer, but nothing deletes or reasserts it. +- Updating an **already-existing** Bucket-B entry's content (not adding a + new one) has no automatic path — deferred to a future opt-in resync + annotation (not implemented in this change). +- Bucket-B schema-breaking migrations (an image upgrade that changes the + *shape* of an existing key) are explicitly out of scope. + +## Cluster-Admin Gating: `ClawOperatorConfig` + +No mechanism existed for a cluster admin to restrict which `mergeMode` +values a `Claw` CR author may use. This ADR adds `ClawOperatorConfig`, a new +CRD (`api/v1alpha1/clawoperatorconfig_types.go`) that a reconcile-time check +enforces: + +- **Scope:** Namespaced, with the singleton instance (name + `ClawOperatorConfigSingletonName` = `"cluster"`) required to live in the + operator's own runtime namespace — resolved dynamically via the + `WATCH_NAMESPACE` env var (Kubernetes downward API, + `fieldRef: metadata.namespace`), never hardcoded, and never looked up in a + tenant namespace. +- **Why Namespaced, not Cluster-scoped:** Both are equally safe from + accidental tenant access; Namespaced wins on RBAC footprint (a + `Role`/`RoleBinding` in one namespace vs. a `ClusterRole`), consistent with + this project's minimal-privilege conventions. +- **Why not a webhook:** No webhook infrastructure exists anywhere in this + operator today, and reconcile-time status-condition enforcement is the + established pattern for equivalent self-service gating in the closest + upstream project. +- **Fail-open by design:** if the singleton doesn't exist, or exists with an + empty `allowedConfigModes`, every mode is allowed — preserving today's + unrestricted behavior until an admin explicitly opts in to restricting it. + +`ClawResourceReconciler.checkConfigModeAllowed` +(`internal/controller/claw_operator_config.go`) runs early in `Reconcile`, +before credential resolution. On denial, it reuses the existing `Ready` +condition — no new condition type — setting `Ready: False` with reason +`ConfigModeNotAllowed`, and delegates to `handlePolicyIdle` +(`internal/controller/claw_idle.go`) to actively scale the Claw's Deployments +to zero replicas, the same mechanism `spec.idle` already uses (a new `Idle` +condition reason, `IdledByPolicy`, distinguishes the two triggers). This +makes policy enforceable fleet-wide, not just for brand-new Claws: an +already-running instance whose mode becomes disallowed after the fact (an +admin tightens policy retroactively) is scaled to zero on its next +reconcile, not left running the disallowed mode indefinitely. The scale-down +is non-destructive — Secrets, the PVC, and the Claw CR's `spec` are never +touched (in particular, `spec.config.mergeMode` is never silently rewritten +on the user's behalf) — so fixing the mode, or widening the policy again, +brings the instance straight back with no data loss. Both `handlePolicyIdle` +and the gating check itself return `nil` (not an error) on a stable denial, +so controller-runtime doesn't apply exponential-backoff retries to what's a +policy mismatch, not a transient failure. + +`SetupWithManager` also watches `ClawOperatorConfig` and enqueues every +`Claw` CR in the cluster on change, so a policy edit is reflected promptly +instead of waiting for an unrelated reconcile trigger — including scaling +down every now-non-compliant Claw at once. + +## Summary Table + +| Row | Current `merge` | Current `overwrite` | `seedOnly` | +|---|---|---|---| +| **Bucket A** (infra/auth + declared-entry credential sub-fields) | Reasserted every restart, but at whole-entry granularity (resets Bucket-B fields of the same entry too) | Reasserted every restart as part of the full-file rebuild | Reasserted every restart, at sub-field granularity only — doesn't touch the entry's Bucket-B fields | +| **Bucket B — declared-entry fields** (`dmPolicy`, a provider's local `.models`, etc.) | Reset every restart as a side effect of whole-entry overwrite, except `agents.defaults.model.primary`/`.fallbacks` (pre-existing carve-out) | Reset every restart — full rebuild, PVC not read | Frozen once seeded; a brand-new CR-declared entry still gap-fills automatically; updating an existing entry needs a future opt-in resync | +| **Bucket B — freeform** (`agents.list`, `cron.*`, non-declared channels/MCP servers) | Preserved (side effect of `deepMerge` only touching known paths) | Wiped — full rebuild ignores the PVC file | Preserved — same outcome, now a designed guarantee | +| **Skill docs** (`PLATFORM.md`, `KUBERNETES.md`, `_skill_*`) | `copyAlways` — overwritten every restart | Same — overwritten every restart | `seedIfMissing` — seed once; user/agent edits or deletions persist | +| **Workspace files** (`AGENTS.md`, `SOUL.md`, `BOOTSTRAP.md`) | `seedIfMissing`, unaffected by `mergeMode` | Same — unaffected | No change — this mechanism never reads `CLAW_CONFIG_MODE` | + +## Decisions + +| # | Question | Decision | +|---|----------|----------| +| Q1 | Mechanism for cluster-admin gating | `ClawOperatorConfig`, a Namespaced singleton CRD in the operator's own runtime namespace (resolved via `WATCH_NAMESPACE`) | +| Q2 | How gating violations are surfaced | Reuse the existing `Ready` condition (no new condition type); actively scale the instance to zero replicas (reusing the `spec.idle` mechanism) rather than only blocking new Claws; halt without returning an error | +| Q3 | Ownership boundary | Two-bucket model, with sub-field-level reassertion for declared providers/channels/MCP servers | +| Q4 | Update/convergence mechanism | Bucket A: covered by the existing reconcile→hash→rollout pipeline, no new mechanism. Bucket B: automatic gap-fill for *new* entries; opt-in resync for *existing* entries is a deferred fast-follow; schema-breaking migrations are explicit out-of-scope future work | +| Q5 | Naming for the new `mergeMode` value | `seedOnly` — disambiguated from the unrelated `SeedMode`/`seedIfMissing` via doc-comments rather than a rename | +| Missing `ClawOperatorConfig` behavior | Fail-open — no singleton means every mode is allowed | + +## Implementation Notes + +- `api/v1alpha1/claw_types.go`: `ConfigMode` enum gains `seedOnly`; + `ConditionReasonConfigModeNotAllowed` added. +- `api/v1alpha1/clawoperatorconfig_types.go`: new CRD, no status subresource + (a misconfigured/missing singleton surfaces via the *Claw*'s own `Ready` + condition, not this type's). +- `internal/controller/claw_operator_config.go`: `effectiveConfigMode` and + `checkConfigModeAllowed` gating logic. +- `internal/controller/claw_idle.go`: `handlePolicyIdle` scales an + out-of-policy Claw's Deployments to zero, sharing + `scaleToZeroAndSetIdleStatus` with the pre-existing `spec.idle` path + (`handleIdle`); `ConditionReasonIdledByPolicy` distinguishes it from a + user-requested idle (`ConditionReasonIdledByRequest`) on the `Idle` + condition. +- `internal/controller/claw_mcp.go`: `injectMcpServers` writes the private + `_seedOnlyMeta` marker; `mcpServerIsBucketA` decides membership + (`command == ""` i.e. URL-based, or non-empty `EnvFrom`, or + `CredentialRef` set). +- `internal/assets/manifests/claw/configmap.yaml`'s `merge.js`: new + `seedOnly` branch (`reassertInfrastructureKeys`, `reassertBucketA`, + `gapFillBucketB`), plus a `seedIfMissing` skill-doc helper. The + `_seedOnlyMeta` marker is stripped immediately after being read, in every + mode, so it never reaches the user-facing `openclaw.json`. +- `cmd/main.go` / `config/manager/manager.yaml`: `WATCH_NAMESPACE` wired via + the downward API; the manager fails fast at startup if unset. diff --git a/docs/architecture.md b/docs/architecture.md index 533f8adf..9218d51d 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -37,6 +37,20 @@ Because deep-merge operates at the key level, operator-managed entries (e.g., `c In `overwrite` mode, the PVC config is ignored and `operator.json` is merged into the seed `openclaw.json` from the ConfigMap. User edits are wiped on every restart. +### `seedOnly` mode + +A third `spec.config.mergeMode` value seeds `openclaw.json` once on first boot, then treats the file as belonging to the user/agent — only a narrow, always-enforced subset of infrastructure/credential sub-fields is reasserted on later restarts. See [ADR-0021](adr/0021-seed-only-config-mode.md) for the full two-bucket ownership model and rationale; not to be confused with the unrelated `SeedMode`/`seedIfMissing` value that governs per-file workspace seeding (`spec.workspace.*.mode`). + +| Owner | Sections | `seedOnly` restart behavior | +|---|---|---| +| Operator (sub-field only) | `gateway.*` infra/auth keys, Route-host entry in `allowedOrigins`, `tools.web.search.*`, `tools.web.fetch.enabled`, `agents.defaults.memorySearch.*`, `diagnostics.otel.{metrics,metricsEndpoint}` | Reasserted every restart, same as `merge`/`overwrite` | +| Operator (sub-field only) | `models.providers..{baseUrl,apiKey,api}`, `channels..{enabled,botToken,token,appToken}`, `mcp.servers.` (full entry, only for `envFrom`/`credentialRef`/URL-based servers) | Reasserted every restart; the rest of each entry is left untouched | +| Operator → User (gap-fill) | New CR-declared provider/channel/MCP server, `agents.defaults.model.primary`/`.fallbacks` when absent | Added automatically the first time it's absent from the PVC file; never touched again once present | +| User | Everything else in a declared entry (`dmPolicy`, `allowFrom`, a provider's local `.models`), `agents.list`, non-declared channels/MCP servers/plugins, `tools.*` beyond web search, `cron.*` | Frozen after first seed — never reset | +| User (skill docs) | `PLATFORM.md`, `KUBERNETES.md`, `_skill_*` | `seedIfMissing` instead of the `merge`/`overwrite` modes' `copyAlways` — seeded once, edits/deletions persist | + +A cluster admin can restrict which `mergeMode` values are permitted via the `ClawOperatorConfig` singleton CRD (see ADR-0021). If a Claw requests a disallowed mode — whether it's brand new or already running — `Ready` reports `False` with reason `ConfigModeNotAllowed` and the instance is scaled to zero replicas (the same non-destructive mechanism `spec.idle` uses; Secrets/PVC/CR spec are untouched). Fixing the mode, or widening the policy, brings it straight back. Missing `ClawOperatorConfig` fails open (all modes allowed). + ## Multi-Instance Support Resource names in the embedded Kustomize manifests use a `CLAW_INSTANCE_NAME` placeholder. At build time, the controller replaces this with the Claw CR name, so multiple Claw instances in the same namespace get distinct resource names. Instance labels (`claw.sandbox.redhat.com/instance`) are injected into all resource metadata, Deployment selectors, Service selectors, and NetworkPolicy selectors to ensure isolation between instances. diff --git a/internal/assets/manifests/claw/configmap.yaml b/internal/assets/manifests/claw/configmap.yaml index c74493a5..ceee35e4 100644 --- a/internal/assets/manifests/claw/configmap.yaml +++ b/internal/assets/manifests/claw/configmap.yaml @@ -72,16 +72,228 @@ data: console.log(`[init-config] copied ${dest}`); } + // seedIfMissing copies src to dest only if dest doesn't already exist — + // used for skill docs under seedOnly mode, so a user/agent's edits or + // deletions of an operator-authored skill doc persist across restarts. + function seedIfMissing(src, dest) { + if (fs.existsSync(dest)) { + return; + } + const dir = path.dirname(dest); + fs.mkdirSync(dir, { recursive: true }); + fs.copyFileSync(src, dest); + console.log(`[init-config] seeded ${dest}`); + } + + // getPath/setPath walk a dotted path (as an array of keys) through a + // plain-object tree, used by the seedOnly reassertion/gap-fill passes + // below to touch only specific sub-fields without disturbing siblings. + function getPath(obj, pathParts) { + let cur = obj; + for (const key of pathParts) { + if (cur === null || typeof cur !== "object") return undefined; + cur = cur[key]; + } + return cur; + } + + function setPath(obj, pathParts, value) { + let cur = obj; + for (let i = 0; i < pathParts.length - 1; i++) { + const key = pathParts[i]; + if (typeof cur[key] !== "object" || cur[key] === null || Array.isArray(cur[key])) { + cur[key] = {}; + } + cur = cur[key]; + } + cur[pathParts[pathParts.length - 1]] = value; + } + + function appendAllMissing(base, additions) { + const list = Array.isArray(base) ? base.slice() : []; + for (const item of Array.isArray(additions) ? additions : []) { + if (!list.includes(item)) list.push(item); + } + return list; + } + + // Bucket-A sub-field allowlists for declared providers/channels — see + // docs/adr/0021-seed-only-config-mode.md's ownership-boundary table. + // Kept in sync by hand with protectedChannelKeys (claw_channels.go) and + // injectProviders (claw_resource_controller.go); both sides are covered + // by tests that would catch drift in practice. + const PROVIDER_BUCKET_A_KEYS = ["baseUrl", "apiKey", "api"]; + const CHANNEL_BUCKET_A_KEYS = ["enabled", "botToken", "token", "appToken"]; + + // reassertInfrastructureKeys unconditionally overwrites the pure + // infrastructure/security subset from the freshly-computed operator.json + // onto the existing PVC file. These have no Bucket-B sub-fields at all, + // so a whole-path overwrite is safe and correct in every restart. + function reassertInfrastructureKeys(base, ops) { + const fixedPaths = [ + ["gateway", "mode"], + ["gateway", "bind"], + ["gateway", "port"], + ["gateway", "controlUi", "enabled"], + ["gateway", "auth", "mode"], + ["gateway", "controlUi", "dangerouslyDisableDeviceAuth"], + ["gateway", "trustedProxies"], + ["tools", "web", "search", "enabled"], + ["tools", "web", "search", "provider"], + ["tools", "web", "fetch", "enabled"], + ["agents", "defaults", "memorySearch", "provider"], + ["agents", "defaults", "memorySearch", "enabled"], + ["diagnostics", "otel", "metrics"], + ["diagnostics", "otel", "metricsEndpoint"], + ]; + for (const p of fixedPaths) { + const value = getPath(ops, p); + if (value !== undefined) { + setPath(base, p, value); + } + } + + // gateway.controlUi.allowedOrigins: append-only. The Route host (and + // any spec.config.raw-declared origins, which are CR-owned, not + // PVC-owned) get appended if missing; any additional origin a + // user/agent added directly to the running file is left untouched. + const opsOrigins = getPath(ops, ["gateway", "controlUi", "allowedOrigins"]); + if (Array.isArray(opsOrigins) && opsOrigins.length > 0) { + const baseOrigins = getPath(base, ["gateway", "controlUi", "allowedOrigins"]); + setPath(base, ["gateway", "controlUi", "allowedOrigins"], appendAllMissing(baseOrigins, opsOrigins)); + } + } + + function isPlainObject(value) { + return typeof value === "object" && value !== null && !Array.isArray(value); + } + + // hasOwn guards every declared-entry lookup below against dangerous + // names like "__proto__"/"constructor"/"prototype": the `in` operator + // (used previously) matches these via the prototype chain even when + // they're not real own entries, and reading/writing through them (e.g. + // base[name] = ops[name]) can reassign an object's own prototype or, if + // base[name] happens to resolve to Object.prototype itself, pollute it + // globally for the rest of this script's execution. Provider/channel/MCP + // server names are CR-author-controlled strings, so this is a real + // reachable input, not just theoretical. + function hasOwn(obj, name) { + return Object.prototype.hasOwnProperty.call(obj, name); + } + + // reassertBucketAEntries overwrites only the allowlisted sub-fields of + // declared providers/channels (i.e. present in both ops and the + // existing PVC file). Everything else in each entry (Bucket B) is left + // completely untouched. Entries present only in the PVC file (not + // CR-declared) are never touched. If a declared entry's existing value + // is malformed (null, a scalar, or an array instead of an object) it's + // treated as broken and replaced wholesale with the declared entry, + // rather than silently skipped — a hand-corrupted Bucket-A entry (e.g. a + // provider with no baseUrl/apiKey) must not be left permanently + // unrepaired. + function reassertBucketAEntries(baseMap, opsMap, bucketAKeys) { + if (!baseMap) return; + for (const name of Object.keys(opsMap)) { + if (!hasOwn(baseMap, name)) continue; + if (!isPlainObject(baseMap[name])) { + baseMap[name] = opsMap[name]; + continue; + } + for (const key of bucketAKeys) { + if (opsMap[name][key] !== undefined) { + baseMap[name][key] = opsMap[name][key]; + } + } + } + } + + // reassertBucketA applies reassertBucketAEntries to declared + // providers/channels, plus the analogous full-entry reassertion for MCP + // servers. mcpBucketAServers is a Set of MCP server names needing + // full-entry reassertion (see injectMcpServers's _seedOnlyMeta marker) + // — this can't be inferred from JSON shape alone (a command+env entry + // looks the same whether or not env came from envFrom). + function reassertBucketA(base, ops, mcpBucketAServers) { + reassertBucketAEntries(getPath(base, ["models", "providers"]), getPath(ops, ["models", "providers"]) || {}, PROVIDER_BUCKET_A_KEYS); + reassertBucketAEntries(base.channels, ops.channels || {}, CHANNEL_BUCKET_A_KEYS); + + const opsMcpServers = getPath(ops, ["mcp", "servers"]) || {}; + const baseMcpServers = getPath(base, ["mcp", "servers"]); + if (baseMcpServers) { + for (const name of Object.keys(opsMcpServers)) { + if (!hasOwn(baseMcpServers, name)) continue; + if (mcpBucketAServers.has(name)) { + baseMcpServers[name] = opsMcpServers[name]; + } + } + } + } + + // gapFillBucketB adds top-level keys present in operator.json's Bucket-B + // collections but absent from the existing PVC file — e.g. a brand-new + // CR-declared provider/channel/MCP server, or a newly catalog-eligible + // model. This is deliberately shallow: it must never recurse into an + // already-existing entry's fields, or it would silently reintroduce the + // exact Bucket-B-clobbering failure mode reassertBucketA is designed to + // avoid. + function gapFillBucketB(base, ops) { + const collections = [ + ["models", "providers"], + ["channels"], + ["mcp", "servers"], + ["agents", "defaults", "models"], + ]; + for (const collectionPath of collections) { + const opsCollection = getPath(ops, collectionPath); + if (!opsCollection || typeof opsCollection !== "object") continue; + let baseCollection = getPath(base, collectionPath); + if (!baseCollection || typeof baseCollection !== "object") { + baseCollection = {}; + setPath(base, collectionPath, baseCollection); + } + for (const key of Object.keys(opsCollection)) { + if (!(key in baseCollection)) { + baseCollection[key] = opsCollection[key]; + } + } + } + + // agents.defaults.model.primary/.fallbacks: fill only if currently + // absent/empty — mirrors the merge-mode carve-out's fill-if-empty + // semantics (never touched once either has any value, catalog-derived + // or hand-picked). + const opsPrimary = getPath(ops, ["agents", "defaults", "model", "primary"]); + const basePrimary = getPath(base, ["agents", "defaults", "model", "primary"]); + if (!basePrimary && opsPrimary) { + setPath(base, ["agents", "defaults", "model", "primary"], opsPrimary); + } + + const opsFallbacks = getPath(ops, ["agents", "defaults", "model", "fallbacks"]); + const baseFallbacks = getPath(base, ["agents", "defaults", "model", "fallbacks"]); + const baseFallbacksEmpty = !baseFallbacks || (Array.isArray(baseFallbacks) && baseFallbacks.length === 0); + const opsFallbacksNonEmpty = opsFallbacks && (!Array.isArray(opsFallbacks) || opsFallbacks.length > 0); + if (baseFallbacksEmpty && opsFallbacksNonEmpty) { + setPath(base, ["agents", "defaults", "model", "fallbacks"], opsFallbacks); + } + } + // Ensure directories exist for (const sub of ["workspace", ".local", ".cache", ".config/codex"]) { fs.mkdirSync(path.join(pvcDir, sub), { recursive: true }); } - // Merge or overwrite openclaw.json + // Merge, overwrite, or seed-only openclaw.json const ops = JSON.parse(fs.readFileSync(path.join(configDir, "operator.json"), "utf8")); const seed = JSON.parse(fs.readFileSync(path.join(configDir, "openclaw.json"), "utf8")); const pvcPath = path.join(pvcDir, "openclaw.json"); + // _seedOnlyMeta is private plumbing for this script (see + // injectMcpServers's doc comment) — it must never reach the user-facing + // openclaw.json, in any mode, so it's stripped immediately after use. + const seedOnlyMeta = ops._seedOnlyMeta || {}; + delete ops._seedOnlyMeta; + const mcpBucketAServers = new Set(seedOnlyMeta.mcpBucketAServers || []); + if (mode === "merge" && fs.existsSync(pvcPath)) { let base = seed; try { @@ -105,13 +317,29 @@ data: } fs.writeFileSync(pvcPath, JSON.stringify(merged, null, 2), { mode: 0o600 }); console.log("[init-config] merged operator.json into existing openclaw.json"); + } else if (mode === "seedOnly" && fs.existsSync(pvcPath)) { + let base = seed; + try { + base = JSON.parse(fs.readFileSync(pvcPath, "utf8")); + } catch (err) { + console.warn("[init-config] existing openclaw.json is invalid JSON; falling back to seed"); + } + reassertInfrastructureKeys(base, ops); + reassertBucketA(base, ops, mcpBucketAServers); + gapFillBucketB(base, ops); + fs.writeFileSync(pvcPath, JSON.stringify(base, null, 2), { mode: 0o600 }); + console.log("[init-config] seedOnly: reasserted bucket-A keys and gap-filled new entries"); } else { fs.writeFileSync(pvcPath, JSON.stringify(deepMerge(seed, ops), null, 2), { mode: 0o600 }); console.log(`[init-config] wrote openclaw.json (mode=${mode}, first_run=${!fs.existsSync(pvcPath)})`); } - // Copy operator-managed skill files (always overwritten) - copyAlways( + // Skill docs: seed-once under seedOnly mode (user/agent edits or + // deletions persist), always-overwritten in every other mode. + const copySkillDoc = mode === "seedOnly" ? seedIfMissing : copyAlways; + + // Copy operator-managed skill files + copySkillDoc( path.join(configDir, "PLATFORM.md"), path.join(workspaceDir, "skills", "platform", "SKILL.md") ); @@ -119,14 +347,14 @@ data: // Copy Kubernetes skill if present in ConfigMap const k8sSkill = path.join(configDir, "KUBERNETES.md"); if (fs.existsSync(k8sSkill)) { - copyAlways(k8sSkill, path.join(workspaceDir, "skills", "kubernetes", "SKILL.md")); + copySkillDoc(k8sSkill, path.join(workspaceDir, "skills", "kubernetes", "SKILL.md")); } // Copy operator-managed skills (_skill_* keys) for (const f of fs.readdirSync(configDir)) { if (!f.startsWith("_skill_")) continue; const skillName = f.slice(7); - copyAlways( + copySkillDoc( path.join(configDir, f), path.join(workspaceDir, "skills", skillName, "SKILL.md") ); @@ -1166,6 +1394,17 @@ data: - `merge` (default) — deep-merge into existing PVC config, preserving runtime changes (primary model choice, plugin installs, UI settings) - `overwrite` — fully replace PVC config on every pod start (clean slate) + - `seedOnly` — seed `openclaw.json` once on first boot, then leave it + untouched on later restarts, except for a narrow, always-enforced + subset (gateway/auth wiring, and the credential/routing sub-fields of + declared providers/channels/MCP servers — not the whole entry). A new + provider/channel/MCP server added to the CR after seeding is added + automatically on the next restart; updating an already-existing + entry's non-credential content is not automatically synced. A cluster + admin may restrict which modes are allowed via `ClawOperatorConfig` — + if `seedOnly` is requested but not allowed, `Ready` reports `False` + with reason `ConfigModeNotAllowed`. Not to be confused with per-file + workspace seeding (`spec.workspace.*.mode: seedIfMissing`). ## `spec.config.raw` vs `openclaw config patch` diff --git a/internal/controller/claw_gitsync_script_exec_test.go b/internal/controller/claw_gitsync_script_exec_test.go new file mode 100644 index 00000000..3d95a38a --- /dev/null +++ b/internal/controller/claw_gitsync_script_exec_test.go @@ -0,0 +1,295 @@ +/* +Copyright 2026 Red Hat. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +// TestGenerateGitSyncScript (claw_workspace_test.go) only ever string-matches +// the generated script. These tests instead execute it for real under sh +// against real local git repositories (file:// transport — no network +// access required), which is the only way to actually prove the shallow +// clone / SHA pinning / branch selection logic works, not just that the +// right substrings appear in the script text. +// +// The three hardcoded absolute path families the real script depends on +// (/etc/proxy-ca/ca.crt, /tmp/combined-ca.crt, /git-sources/) are +// redirected into per-test temp directories via string replacement, the +// same technique used throughout this package's other script-execution +// tests (see claw_merge_test.go, claw_seed_script_test.go). + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + clawv1alpha1 "github.com/codeready-toolchain/claw-operator/api/v1alpha1" +) + +func runGitCmd(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) //nolint:gosec + cmd.Dir = dir + cmd.Env = append(os.Environ(), "GIT_AUTHOR_NAME=test", "GIT_AUTHOR_EMAIL=test@example.com", + "GIT_COMMITTER_NAME=test", "GIT_COMMITTER_EMAIL=test@example.com") + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %s failed: %s", strings.Join(args, " "), string(out)) + return strings.TrimSpace(string(out)) +} + +// gitFixture is a local repo with three points of reference: the initial +// commit SHA (content "v1"), a second commit on main (content "v2", HEAD), +// and a "feature" branch (content "feature"). +type gitFixture struct { + repoPath string + firstSHA string + secondSHA string +} + +func createGitFixture(t *testing.T) gitFixture { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not found in PATH, skipping git-sync execution tests") + } + + repoPath := t.TempDir() + runGitCmd(t, repoPath, "init", "-q", "-b", "main") + require.NoError(t, os.WriteFile(filepath.Join(repoPath, "SOUL.md"), []byte("v1"), 0o644)) + runGitCmd(t, repoPath, "add", ".") + runGitCmd(t, repoPath, "commit", "-q", "-m", "c1") + firstSHA := runGitCmd(t, repoPath, "rev-parse", "HEAD") + + require.NoError(t, os.WriteFile(filepath.Join(repoPath, "SOUL.md"), []byte("v2"), 0o644)) + runGitCmd(t, repoPath, "add", ".") + runGitCmd(t, repoPath, "commit", "-q", "-m", "c2") + secondSHA := runGitCmd(t, repoPath, "rev-parse", "HEAD") + + runGitCmd(t, repoPath, "checkout", "-q", "-b", "feature") + require.NoError(t, os.WriteFile(filepath.Join(repoPath, "SOUL.md"), []byte("feature"), 0o644)) + runGitCmd(t, repoPath, "add", ".") + runGitCmd(t, repoPath, "commit", "-q", "-m", "c3") + runGitCmd(t, repoPath, "checkout", "-q", "main") + + return gitFixture{repoPath: repoPath, firstSHA: firstSHA, secondSHA: secondSHA} +} + +type gitSyncScriptResult struct { + stdout, stderr string + sourcesDir string +} + +// runGitSyncScript generates the real script for gitSources, redirects its +// hardcoded paths into tmpDir, and executes it for real via sh. extraEnv is +// merged into the child process environment (used for GIT_TOKEN_N and test +// canaries). +func runGitSyncScript(t *testing.T, gitSources []clawv1alpha1.GitSource, extraEnv ...string) gitSyncScriptResult { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not found in PATH, skipping git-sync execution tests") + } + + tmpDir := t.TempDir() + sourcesDir := filepath.Join(tmpDir, "git-sources") + require.NoError(t, os.MkdirAll(sourcesDir, 0o755)) + + fakeCA := filepath.Join(tmpDir, "fake-ca.crt") + require.NoError(t, os.WriteFile(fakeCA, []byte("dummy CA cert\n"), 0o644)) + combinedCA := filepath.Join(tmpDir, "combined-ca.crt") + + script := generateGitSyncScript(gitSources) + require.Contains(t, script, "/etc/proxy-ca/ca.crt", + "generateGitSyncScript proxy CA anchor changed — update this test's path substitution") + require.Contains(t, script, "/tmp/combined-ca.crt", + "generateGitSyncScript combined CA anchor changed — update this test's path substitution") + require.Contains(t, script, "/git-sources/", + "generateGitSyncScript destination anchor changed — update this test's path substitution") + script = strings.ReplaceAll(script, "/etc/proxy-ca/ca.crt", fakeCA) + script = strings.ReplaceAll(script, "/tmp/combined-ca.crt", combinedCA) + script = strings.ReplaceAll(script, "/git-sources/", sourcesDir+"/") + + cmd := exec.Command("sh", "-c", script) //nolint:gosec + cmd.Env = append(os.Environ(), extraEnv...) + var stdout, stderr strings.Builder + cmd.Stdout = &stdout + cmd.Stderr = &stderr + err := cmd.Run() + require.NoError(t, err, "git sync script failed: stdout=%s stderr=%s", stdout.String(), stderr.String()) + + return gitSyncScriptResult{stdout: stdout.String(), stderr: stderr.String(), sourcesDir: sourcesDir} +} + +func TestGitSyncScriptExecution(t *testing.T) { + t.Run("clones the default branch when no ref is given", func(t *testing.T) { + fixture := createGitFixture(t) + + result := runGitSyncScript(t, []clawv1alpha1.GitSource{ + {URL: "file://" + fixture.repoPath}, + }) + + content, err := os.ReadFile(filepath.Join(result.sourcesDir, "0", "SOUL.md")) + require.NoError(t, err) + assert.Equal(t, "v2", string(content), "should clone the tip of the default branch") + }) + + t.Run("checks out the requested branch, not the default", func(t *testing.T) { + fixture := createGitFixture(t) + + result := runGitSyncScript(t, []clawv1alpha1.GitSource{ + {URL: "file://" + fixture.repoPath, Ref: "feature"}, + }) + + content, err := os.ReadFile(filepath.Join(result.sourcesDir, "0", "SOUL.md")) + require.NoError(t, err) + assert.Equal(t, "feature", string(content)) + }) + + t.Run("pins to an exact commit SHA, not the branch tip", func(t *testing.T) { + fixture := createGitFixture(t) + + result := runGitSyncScript(t, []clawv1alpha1.GitSource{ + {URL: "file://" + fixture.repoPath, Ref: fixture.firstSHA}, + }) + + content, err := os.ReadFile(filepath.Join(result.sourcesDir, "0", "SOUL.md")) + require.NoError(t, err, "stdout=%s stderr=%s", result.stdout, result.stderr) + assert.Equal(t, "v1", string(content), + "SHA ref must fetch that exact commit, even though a newer commit exists on the branch") + }) + + t.Run("clones multiple sources independently into their own destinations", func(t *testing.T) { + fixtureA := createGitFixture(t) + fixtureB := createGitFixture(t) + + result := runGitSyncScript(t, []clawv1alpha1.GitSource{ + {URL: "file://" + fixtureA.repoPath, Ref: "feature"}, + {URL: "file://" + fixtureB.repoPath, Ref: fixtureB.firstSHA}, + }) + + contentA, err := os.ReadFile(filepath.Join(result.sourcesDir, "0", "SOUL.md")) + require.NoError(t, err) + assert.Equal(t, "feature", string(contentA)) + + contentB, err := os.ReadFile(filepath.Join(result.sourcesDir, "1", "SOUL.md")) + require.NoError(t, err) + assert.Equal(t, "v1", string(contentB)) + }) + + t.Run("private repo path clones successfully with the full ASKPASS scaffolding active", func(t *testing.T) { + fixture := createGitFixture(t) + + // file:// transport never actually prompts for credentials, but this + // still exercises every line of the SecretRef branch for real — + // mktemp, the printf'd askpass script, chmod +x, and the rm -f + // cleanup — proving there's no syntax or quoting error in that path. + result := runGitSyncScript(t, []clawv1alpha1.GitSource{ + { + URL: "file://" + fixture.repoPath, + SecretRef: &clawv1alpha1.SecretRefEntry{ + Name: "git-creds", + Key: "token", + }, + }, + }, "GIT_TOKEN_0=fake-token-value") + + content, err := os.ReadFile(filepath.Join(result.sourcesDir, "0", "SOUL.md")) + require.NoError(t, err, "stdout=%s stderr=%s", result.stdout, result.stderr) + assert.Equal(t, "v2", string(content)) + }) + + t.Run("askpass helper echoes exactly the configured token", func(t *testing.T) { + // Rather than requiring a full authenticating git server to exercise + // this over the wire, render just the askpass helper fragment the + // real script generates and invoke it directly with GIT_TOKEN_0 set + // — this is the actual security-sensitive logic (credentials must + // never leak into argv or the clone URL) and it's fully testable in + // isolation this way. + script := generateGitSyncScript([]clawv1alpha1.GitSource{ + { + URL: "https://git.example.com/team/repo.git", + SecretRef: &clawv1alpha1.SecretRefEntry{Name: "git-creds", Key: "token"}, + }, + }) + require.Contains(t, script, `printf '#!/bin/sh\necho "${GIT_TOKEN_0}"\n'`) + + tmpDir := t.TempDir() + askpassPath := filepath.Join(tmpDir, "askpass.sh") + setupScript := fmt.Sprintf(`printf '#!/bin/sh\necho "${GIT_TOKEN_0}"\n' > %q && chmod +x %q`, + askpassPath, askpassPath) + cmd := exec.Command("sh", "-c", setupScript) //nolint:gosec + require.NoError(t, cmd.Run()) + + // The rendered helper reads GIT_TOKEN_0 from its own environment. + runCmd := exec.Command(askpassPath) //nolint:gosec + runCmd.Env = append(os.Environ(), "GIT_TOKEN_0=super-secret-token") + out, err := runCmd.Output() + require.NoError(t, err) + assert.Equal(t, "super-secret-token\n", string(out)) + }) + + t.Run("does not execute shell metacharacters embedded in a ref", func(t *testing.T) { + fixture := createGitFixture(t) + tmpDir := t.TempDir() + canary := filepath.Join(tmpDir, "pwned") + + // An invalid ref just fails the clone (expected); the point is that + // it fails as a *git error*, not by running the injected command. + _ = runGitSyncScriptAllowFailure(t, []clawv1alpha1.GitSource{ + {URL: "file://" + fixture.repoPath, Ref: `nonexistent'; touch "$CANARY"; echo '`}, + }, "CANARY="+canary) + + _, statErr := os.Stat(canary) + assert.True(t, os.IsNotExist(statErr), + "shell metacharacters in a ref must never execute — canary file should not have been created") + }) +} + +// runGitSyncScriptAllowFailure is like runGitSyncScript but tolerates the +// script exiting non-zero (used for negative/injection tests where the +// clone is expected to fail). +func runGitSyncScriptAllowFailure(t *testing.T, gitSources []clawv1alpha1.GitSource, extraEnv ...string) gitSyncScriptResult { + t.Helper() + tmpDir := t.TempDir() + sourcesDir := filepath.Join(tmpDir, "git-sources") + require.NoError(t, os.MkdirAll(sourcesDir, 0o755)) + + fakeCA := filepath.Join(tmpDir, "fake-ca.crt") + require.NoError(t, os.WriteFile(fakeCA, []byte("dummy CA cert\n"), 0o644)) + combinedCA := filepath.Join(tmpDir, "combined-ca.crt") + + script := generateGitSyncScript(gitSources) + require.Contains(t, script, "/etc/proxy-ca/ca.crt", + "generateGitSyncScript proxy CA anchor changed — update this test's path substitution") + require.Contains(t, script, "/tmp/combined-ca.crt", + "generateGitSyncScript combined CA anchor changed — update this test's path substitution") + require.Contains(t, script, "/git-sources/", + "generateGitSyncScript destination anchor changed — update this test's path substitution") + script = strings.ReplaceAll(script, "/etc/proxy-ca/ca.crt", fakeCA) + script = strings.ReplaceAll(script, "/tmp/combined-ca.crt", combinedCA) + script = strings.ReplaceAll(script, "/git-sources/", sourcesDir+"/") + + cmd := exec.Command("sh", "-c", script) //nolint:gosec + cmd.Env = append(os.Environ(), extraEnv...) + var stdout, stderr strings.Builder + cmd.Stdout = &stdout + cmd.Stderr = &stderr + _ = cmd.Run() + + return gitSyncScriptResult{stdout: stdout.String(), stderr: stderr.String(), sourcesDir: sourcesDir} +} diff --git a/internal/controller/claw_idle.go b/internal/controller/claw_idle.go index 8f4a9304..31f23cf4 100644 --- a/internal/controller/claw_idle.go +++ b/internal/controller/claw_idle.go @@ -37,8 +37,39 @@ import ( // It scales all managed Deployments to zero replicas and updates status // to reflect the idled state. func (r *ClawResourceReconciler) handleIdle(ctx context.Context, instance *clawv1alpha1.Claw) (ctrl.Result, error) { + return r.scaleToZeroAndSetIdleStatus(ctx, instance, + clawv1alpha1.ConditionReasonIdledByRequest, "Instance scaled to zero by spec.idle", + clawv1alpha1.ConditionReasonIdle, "Instance is idled — set spec.idle to false to resume") +} + +// handlePolicyIdle short-circuits the reconcile loop when spec.config.mergeMode +// is disallowed by the ClawOperatorConfig singleton. Unlike gating alone (which +// only ever blocked a Claw's *first* reconcile, leaving an already-running +// instance's Deployment — and the mode it was last reconciled with — untouched +// forever if policy tightens later), this actively converges an existing +// instance to zero replicas so cluster-admin policy is enforceable fleet-wide, +// not just for brand-new Claws. It's non-destructive: Secrets, the PVC, and +// the Claw CR itself are never touched, so fixing the mode (or the policy) +// brings the instance straight back on the next reconcile with no data loss. +// See docs/adr/0021-seed-only-config-mode.md. +func (r *ClawResourceReconciler) handlePolicyIdle(ctx context.Context, instance *clawv1alpha1.Claw) (ctrl.Result, error) { + msg := fmt.Sprintf("mergeMode %q is not allowed by ClawOperatorConfig", effectiveConfigMode(instance)) + return r.scaleToZeroAndSetIdleStatus(ctx, instance, + clawv1alpha1.ConditionReasonIdledByPolicy, "Instance scaled to zero — "+msg, + clawv1alpha1.ConditionReasonConfigModeNotAllowed, msg) +} + +// scaleToZeroAndSetIdleStatus is the shared implementation behind handleIdle +// and handlePolicyIdle: it scales all managed Deployments to zero replicas +// and sets the Idle/Ready conditions accordingly, using caller-supplied +// reasons/messages so status accurately reflects *why* the instance is idle. +func (r *ClawResourceReconciler) scaleToZeroAndSetIdleStatus( + ctx context.Context, + instance *clawv1alpha1.Claw, + idleReason, idleMessage, readyReason, readyMessage string, +) (ctrl.Result, error) { logger := log.FromContext(ctx) - logger.Info("Instance is idled, scaling deployments to zero") + logger.Info("Instance is idled, scaling deployments to zero", "reason", idleReason) deploymentNames := []string{ getClawDeploymentName(instance.Name), @@ -53,19 +84,16 @@ func (r *ClawResourceReconciler) handleIdle(ctx context.Context, instance *clawv idleCond := meta.FindStatusCondition(instance.Status.Conditions, clawv1alpha1.ConditionTypeIdle) readyCond := meta.FindStatusCondition(instance.Status.Conditions, clawv1alpha1.ConditionTypeReady) - alreadyIdled := idleCond != nil && idleCond.Status == metav1.ConditionTrue && - readyCond != nil && readyCond.Status == metav1.ConditionFalse && - readyCond.Reason == clawv1alpha1.ConditionReasonIdle && + alreadyIdled := idleCond != nil && idleCond.Status == metav1.ConditionTrue && idleCond.Reason == idleReason && + readyCond != nil && readyCond.Status == metav1.ConditionFalse && readyCond.Reason == readyReason && instance.Status.URL == "" //nolint:staticcheck // deprecated but still checked if alreadyIdled { return ctrl.Result{}, nil } - setCondition(instance, clawv1alpha1.ConditionTypeIdle, metav1.ConditionTrue, - clawv1alpha1.ConditionReasonIdledByRequest, "Instance scaled to zero by spec.idle") - setCondition(instance, clawv1alpha1.ConditionTypeReady, metav1.ConditionFalse, - clawv1alpha1.ConditionReasonIdle, "Instance is idled — set spec.idle to false to resume") + setCondition(instance, clawv1alpha1.ConditionTypeIdle, metav1.ConditionTrue, idleReason, idleMessage) + setCondition(instance, clawv1alpha1.ConditionTypeReady, metav1.ConditionFalse, readyReason, readyMessage) instance.Status.URL = "" //nolint:staticcheck // deprecated but still populated instance.Status.GatewayURL = "" diff --git a/internal/controller/claw_mcp.go b/internal/controller/claw_mcp.go index c96a0c00..ed64e75c 100644 --- a/internal/controller/claw_mcp.go +++ b/internal/controller/claw_mcp.go @@ -62,17 +62,41 @@ func (r *ClawResourceReconciler) validateMcpServerSecrets(ctx context.Context, i // injectMcpServers injects MCP server configuration for all entries in // spec.mcpServers. Always-win: operator overwrites mcp.servers unconditionally. +// +// Also records which server names need full-entry Bucket-A reassertion under +// seedOnly mode (see docs/adr/0021-seed-only-config-mode.md) via a private +// "_seedOnlyMeta" key: servers using envFrom/credentialRef, or reached over a +// URL (proxy-routed, domain-allowlist-gated), can never be safely +// hand-authored by a user — merge.js can't tell this from the entry's JSON +// shape alone (a command+env entry looks the same whether or not env came +// from envFrom), so it's flagged here instead. merge.js strips this key +// before writing the final openclaw.json to the PVC; it must never reach the +// user-facing file. Command-based servers with only inline env are Bucket B +// — safe to hand-add/edit directly in the file, in any mode. func injectMcpServers(config map[string]any, instance *clawv1alpha1.Claw) { if len(instance.Spec.McpServers) == 0 { return } servers := make(map[string]any, len(instance.Spec.McpServers)) + var bucketAServers []any for name, spec := range instance.Spec.McpServers { servers[name] = buildMcpServerConfig(spec) + if mcpServerIsBucketA(spec) { + bucketAServers = append(bucketAServers, name) + } } config["mcp"] = map[string]any{"servers": servers} + config["_seedOnlyMeta"] = map[string]any{"mcpBucketAServers": bucketAServers} +} + +// mcpServerIsBucketA reports whether an MCP server entry needs full-entry +// reassertion under seedOnly mode: any server reached via URL (proxy-routed) +// or using envFrom/credentialRef (Secret- or proxy-backed) can never be a +// genuine hand-edit candidate, in any mode. +func mcpServerIsBucketA(spec clawv1alpha1.McpServerSpec) bool { + return spec.Command == "" || len(spec.EnvFrom) > 0 || spec.CredentialRef != "" } // buildMcpServerConfig builds the JSON-ready config for a single MCP server entry. diff --git a/internal/controller/claw_mcp_test.go b/internal/controller/claw_mcp_test.go index 31928156..9caa8999 100644 --- a/internal/controller/claw_mcp_test.go +++ b/internal/controller/claw_mcp_test.go @@ -204,6 +204,44 @@ func TestInjectMcpServers(t *testing.T) { assert.Len(t, mcp, 1) assert.Contains(t, mcp, "servers") }) + + t.Run("should flag URL-based and envFrom servers as bucket-A in _seedOnlyMeta", func(t *testing.T) { + config := map[string]any{} + instance := testClawWithMcpServers(map[string]clawv1alpha1.McpServerSpec{ + "remote": {URL: "https://example.com/mcp"}, + "credentialed": {URL: "https://example.com/mcp2", CredentialRef: "some-cred"}, + "secret-backed": { + Command: "node", + Args: []string{"server.js"}, + EnvFrom: []clawv1alpha1.McpEnvFromSecret{ + {Name: "API_KEY", SecretRef: clawv1alpha1.SecretRefEntry{Name: "s", Key: "k"}}, + }, + }, + "plain": {Command: "node", Args: []string{"server.js"}, Env: map[string]string{"FOO": "bar"}}, + }) + + injectMcpServers(config, instance) + + meta, ok := config["_seedOnlyMeta"].(map[string]any) + require.True(t, ok, "_seedOnlyMeta should be set") + bucketA, ok := meta["mcpBucketAServers"].([]any) + require.True(t, ok, "mcpBucketAServers should be a slice") + assert.ElementsMatch(t, []any{"remote", "credentialed", "secret-backed"}, bucketA) + assert.NotContains(t, bucketA, "plain") + }) + + t.Run("should not include plain command servers in bucket-A list", func(t *testing.T) { + config := map[string]any{} + instance := testClawWithMcpServers(map[string]clawv1alpha1.McpServerSpec{ + "plain": {Command: "node", Args: []string{"server.js"}, Env: map[string]string{"FOO": "bar"}}, + }) + + injectMcpServers(config, instance) + + meta, ok := config["_seedOnlyMeta"].(map[string]any) + require.True(t, ok, "_seedOnlyMeta should be set") + assert.Empty(t, meta["mcpBucketAServers"]) + }) } func TestBuildMcpServerConfig(t *testing.T) { diff --git a/internal/controller/claw_merge_test.go b/internal/controller/claw_merge_test.go index eb01de4b..d1697252 100644 --- a/internal/controller/claw_merge_test.go +++ b/internal/controller/claw_merge_test.go @@ -22,6 +22,7 @@ import ( "os" "os/exec" "path/filepath" + "slices" "strings" "testing" @@ -30,6 +31,14 @@ import ( "gopkg.in/yaml.v3" ) +// operatorJSONWithPrimaryAndFallback is a small operator.json fixture +// declaring a primary model and a single fallback, shared by several tests +// covering agents.defaults.model.primary/.fallbacks fill-if-empty semantics. +const operatorJSONWithPrimaryAndFallback = `{ + "gateway": {"port": 18789}, + "agents": {"defaults": {"model": {"primary": "google/gemini-3.1-pro-preview", "fallbacks": ["google/gemini-3-flash-preview"]}}} +}` + type configMapYAML struct { Data map[string]string `yaml:"data"` } @@ -122,6 +131,13 @@ func runMergeJS(t *testing.T, setup mergeTestSetup) mergeTestResult { cmd := exec.Command("node", scriptPath) //nolint:gosec if setup.configMode != "" { cmd.Env = append(os.Environ(), "CLAW_CONFIG_MODE="+setup.configMode) + } else { + // Explicitly unset rather than leaving cmd.Env nil, so the child can't + // inherit an ambient CLAW_CONFIG_MODE from the test process's own + // environment — setup.configMode == "" must always mean "unset". + cmd.Env = slices.DeleteFunc(os.Environ(), func(e string) bool { + return strings.HasPrefix(e, "CLAW_CONFIG_MODE=") + }) } var stdout, stderr strings.Builder @@ -414,10 +430,7 @@ func TestMergeJS(t *testing.T) { }) t.Run("fallbacks not preserved in overwrite mode", func(t *testing.T) { - operatorJSON := `{ - "gateway": {"port": 18789}, - "agents": {"defaults": {"model": {"primary": "google/gemini-3.1-pro-preview", "fallbacks": ["google/gemini-3-flash-preview"]}}} - }` + operatorJSON := operatorJSONWithPrimaryAndFallback pvcJSON := `{ "agents": {"defaults": {"model": {"primary": "google/gemini-3.1-pro-preview", "fallbacks": ["anthropic/claude-sonnet-4-6"]}}} }` @@ -476,4 +489,458 @@ func TestMergeJS(t *testing.T) { require.NoError(t, err) assert.Equal(t, "# Quotes\nBuild quotes...", string(quotes)) }) + + t.Run("_seedOnlyMeta never leaks into written openclaw.json in any mode", func(t *testing.T) { + operatorJSON := `{ + "gateway": {"port": 18789}, + "mcp": {"servers": {"db": {"command": "node"}}}, + "_seedOnlyMeta": {"mcpBucketAServers": ["db"]} + }` + for _, mode := range []string{"", "merge", "overwrite", "seedOnly"} { + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, configMode: mode}) + assert.NotContains(t, result.config, "_seedOnlyMeta", "mode=%q should not leak _seedOnlyMeta", mode) + } + }) +} + +// TestMergeJSSeedOnly exhaustively covers the seedOnly mode test matrix from +// docs/adr/0021-seed-only-config-mode.md. Failure modes here are silent and +// asymmetric (under-reasserting leaves a security/auth field unprotected; +// over-reasserting silently clobbers a user/agent's customization) and only +// surface on the second-or-later restart of an already-seeded instance — so +// this suite intentionally covers every row of the matrix, not just one +// representative case. +func TestMergeJSSeedOnly(t *testing.T) { + if _, err := exec.LookPath("node"); err != nil { + t.Skip("node not found in PATH, skipping merge.js tests") + } + + t.Run("first boot with no existing PVC file seeds correctly", func(t *testing.T) { + result := runMergeJS(t, mergeTestSetup{configMode: "seedOnly"}) + + _, hasGateway := nestedValue(result.config, "gateway") + assert.True(t, hasGateway, "result should have gateway section from operator.json") + + agentsList, hasAgentsList := nestedValue(result.config, "agents.list") + assert.True(t, hasAgentsList, "result should have agents.list from seed") + list, ok := agentsList.([]any) + assert.True(t, ok && len(list) > 0, "agents.list should be a non-empty array") + }) + + t.Run("declared provider's baseUrl/apiKey/api corrected, local models preserved", func(t *testing.T) { + operatorJSON := `{ + "gateway": {"port": 18789}, + "models": {"providers": {"google": {"baseUrl": "https://real.example.com", "apiKey": "ah-ah-ah-you-didnt-say-the-magic-word", "api": "openai-completions"}}} + }` + pvcJSON := `{ + "models": {"providers": {"google": { + "baseUrl": "https://hacked.example.com", + "apiKey": "stolen-key", + "api": "anthropic-messages", + "models": {"custom-model": {"alias": "Custom"}} + }}} + }` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + baseURL, _ := nestedValue(result.config, "models.providers.google.baseUrl") + assert.Equal(t, "https://real.example.com", baseURL, "baseUrl should be corrected to operator's value") + apiKey, _ := nestedValue(result.config, "models.providers.google.apiKey") + assert.Equal(t, "ah-ah-ah-you-didnt-say-the-magic-word", apiKey, "apiKey should be corrected") + api, _ := nestedValue(result.config, "models.providers.google.api") + assert.Equal(t, "openai-completions", api, "api should be corrected") + + customModel, hasCustomModel := nestedValue(result.config, "models.providers.google.models.custom-model") + assert.True(t, hasCustomModel, "hand-populated local .models array should be preserved (Bucket B)") + assert.Equal(t, map[string]any{"alias": "Custom"}, customModel) + }) + + t.Run("malformed declared provider/channel entries are replaced with the declared structure", func(t *testing.T) { + operatorJSON := `{ + "gateway": {"port": 18789}, + "models": {"providers": { + "google": {"baseUrl": "https://real.example.com", "apiKey": "real-key", "api": "openai-completions"}, + "openai": {"baseUrl": "https://openai.example.com", "apiKey": "openai-key", "api": "openai-completions"}, + "acme": {"baseUrl": "https://acme.example.com", "apiKey": "acme-key", "api": "openai-completions"} + }}, + "channels": {"telegram": {"enabled": true, "botToken": "placeholder"}} + }` + pvcJSON := `{ + "models": {"providers": {"google": null, "openai": ["not", "an", "object"], "acme": "corrupted-string"}}, + "channels": {"telegram": "corrupted-string"} + }` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + google, _ := nestedValue(result.config, "models.providers.google") + assert.Equal(t, map[string]any{"baseUrl": "https://real.example.com", "apiKey": "real-key", "api": "openai-completions"}, google, + "a null provider entry should be repaired with the full declared structure") + openai, _ := nestedValue(result.config, "models.providers.openai") + assert.Equal(t, map[string]any{"baseUrl": "https://openai.example.com", "apiKey": "openai-key", "api": "openai-completions"}, openai, + "an array-shaped provider entry should be repaired with the full declared structure") + acme, _ := nestedValue(result.config, "models.providers.acme") + assert.Equal(t, map[string]any{"baseUrl": "https://acme.example.com", "apiKey": "acme-key", "api": "openai-completions"}, acme, + "a scalar-shaped provider entry should be repaired with the full declared structure") + telegram, _ := nestedValue(result.config, "channels.telegram") + assert.Equal(t, map[string]any{"enabled": true, "botToken": "placeholder"}, telegram, + "a scalar-shaped channel entry should be repaired with the full declared structure") + }) + + t.Run("malformed declared MCP bucket-A entry is replaced with the declared structure", func(t *testing.T) { + operatorJSON := `{ + "gateway": {"port": 18789}, + "mcp": {"servers": {"db": {"command": "node", "args": ["db-mcp-server.js"], "env": {"DB_PASSWORD": "DB_PASSWORD"}}}}, + "_seedOnlyMeta": {"mcpBucketAServers": ["db"]} + }` + pvcJSON := `{"mcp": {"servers": {"db": null}}}` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + db, _ := nestedValue(result.config, "mcp.servers.db") + assert.Equal(t, map[string]any{"command": "node", "args": []any{"db-mcp-server.js"}, "env": map[string]any{"DB_PASSWORD": "DB_PASSWORD"}}, db, + "a null bucket-A MCP server entry should be repaired with the full declared structure") + }) + + t.Run("dangerous provider/channel/MCP server names are never merged as own entries", func(t *testing.T) { + operatorJSON := `{ + "gateway": {"port": 18789}, + "models": {"providers": { + "google": {"baseUrl": "https://real.example.com", "apiKey": "real-key", "api": "openai-completions"}, + "__proto__": {"baseUrl": "https://evil.example.com", "apiKey": "evil-key", "api": "openai-completions"}, + "constructor": {"baseUrl": "https://evil2.example.com", "apiKey": "evil-key-2", "api": "openai-completions"} + }}, + "mcp": {"servers": {"db": {"command": "node"}, "__proto__": {"command": "evil"}}}, + "_seedOnlyMeta": {"mcpBucketAServers": ["db", "__proto__"]} + }` + pvcJSON := `{ + "models": {"providers": {"google": {"baseUrl": "https://real.example.com", "apiKey": "real-key", "api": "openai-completions"}}}, + "mcp": {"servers": {"db": {"command": "node"}}} + }` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + providers, ok := nestedValue(result.config, "models.providers") + require.True(t, ok) + providersMap, ok := providers.(map[string]any) + require.True(t, ok) + assert.NotContains(t, providersMap, "__proto__", "__proto__ must never be treated as a mergeable provider name") + assert.NotContains(t, providersMap, "constructor", "constructor must never be treated as a mergeable provider name") + + mcpServers, ok := nestedValue(result.config, "mcp.servers") + require.True(t, ok) + mcpServersMap, ok := mcpServers.(map[string]any) + require.True(t, ok) + assert.NotContains(t, mcpServersMap, "__proto__", "__proto__ must never be treated as a mergeable MCP server name") + + google, _ := nestedValue(result.config, "models.providers.google.baseUrl") + assert.Equal(t, "https://real.example.com", google, "legitimate sibling entries must remain unaffected") + }) + + t.Run("declared channel's botToken/enabled corrected, dmPolicy/allowFrom preserved", func(t *testing.T) { + operatorJSON := `{ + "gateway": {"port": 18789}, + "channels": {"telegram": {"enabled": true, "botToken": "placeholder", "dmPolicy": "open", "allowFrom": ["*"]}} + }` + pvcJSON := `{ + "channels": {"telegram": {"enabled": false, "botToken": "hijacked", "dmPolicy": "allowlist", "allowFrom": [12345]}} + }` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + enabled, _ := nestedValue(result.config, "channels.telegram.enabled") + assert.Equal(t, true, enabled, "enabled should be corrected back to operator's value") + botToken, _ := nestedValue(result.config, "channels.telegram.botToken") + assert.Equal(t, "placeholder", botToken, "botToken should be corrected back") + + dmPolicy, _ := nestedValue(result.config, "channels.telegram.dmPolicy") + assert.Equal(t, "allowlist", dmPolicy, "hand-edited dmPolicy should be preserved (Bucket B)") + allowFrom, _ := nestedValue(result.config, "channels.telegram.allowFrom") + assert.Equal(t, []any{float64(12345)}, allowFrom, "hand-edited allowFrom should be preserved (Bucket B)") + }) + + t.Run("declared credentialed MCP server (envFrom) corrected on restart", func(t *testing.T) { + operatorJSON := `{ + "gateway": {"port": 18789}, + "mcp": {"servers": {"db": {"command": "node", "args": ["db-mcp-server.js"], "env": {"DB_HOST": "postgres.internal", "DB_PASSWORD": "DB_PASSWORD"}}}}, + "_seedOnlyMeta": {"mcpBucketAServers": ["db"]} + }` + pvcJSON := `{ + "mcp": {"servers": {"db": {"command": "malicious", "args": ["evil.js"], "env": {"DB_PASSWORD": "hacked"}}}} + }` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + server, _ := nestedValue(result.config, "mcp.servers.db") + serverMap, ok := server.(map[string]any) + require.True(t, ok) + assert.Equal(t, "node", serverMap["command"], "envFrom-backed MCP server should be fully reasserted") + assert.Equal(t, []any{"db-mcp-server.js"}, serverMap["args"]) + }) + + t.Run("hand-added non-declared MCP server is preserved untouched", func(t *testing.T) { + operatorJSON := `{"gateway": {"port": 18789}}` + pvcJSON := `{ + "mcp": {"servers": {"custom": {"command": "node", "args": ["my-own-server.js"]}}} + }` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + server, hasServer := nestedValue(result.config, "mcp.servers.custom") + assert.True(t, hasServer, "hand-added, non-CR-declared MCP server should be preserved") + assert.Equal(t, map[string]any{"command": "node", "args": []any{"my-own-server.js"}}, server) + }) + + t.Run("new CR-declared provider gap-fills automatically", func(t *testing.T) { + operatorJSON := `{ + "gateway": {"port": 18789}, + "models": {"providers": { + "google": {"baseUrl": "https://google.example.com", "apiKey": "placeholder", "api": "openai-completions"}, + "openai": {"baseUrl": "https://openai.example.com", "apiKey": "placeholder", "api": "openai-completions"} + }} + }` + pvcJSON := `{ + "models": {"providers": {"google": {"baseUrl": "https://google.example.com", "apiKey": "placeholder", "api": "openai-completions"}}} + }` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + openai, hasOpenai := nestedValue(result.config, "models.providers.openai") + assert.True(t, hasOpenai, "new CR-declared provider should be gap-filled automatically") + assert.Equal(t, map[string]any{"baseUrl": "https://openai.example.com", "apiKey": "placeholder", "api": "openai-completions"}, openai) + }) + + t.Run("newly catalog-eligible agents.defaults.models entry gap-fills automatically", func(t *testing.T) { + operatorJSON := `{ + "gateway": {"port": 18789}, + "agents": {"defaults": {"models": { + "anthropic/claude-opus-4-7": {"alias": "Claude Opus"}, + "openai/gpt-5": {"alias": "GPT-5"} + }}} + }` + pvcJSON := `{ + "agents": {"defaults": {"models": {"anthropic/claude-opus-4-7": {"alias": "Claude Opus"}}}} + }` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + gpt5, hasGpt5 := nestedValue(result.config, "agents.defaults.models.openai/gpt-5") + assert.True(t, hasGpt5, "newly catalog-eligible model should be gap-filled automatically") + assert.Equal(t, map[string]any{"alias": "GPT-5"}, gpt5) + }) + + t.Run("existing entry's CR-side content change does not propagate", func(t *testing.T) { + operatorJSON := `{ + "gateway": {"port": 18789}, + "models": {"providers": {"acme": {"baseUrl": "https://acme.example.com", "apiKey": "placeholder", "api": "openai-completions"}}} + }` + pvcJSON := `{ + "models": {"providers": {"acme": { + "baseUrl": "https://acme.example.com", "apiKey": "placeholder", "api": "openai-completions", + "models": {"acme-model-v1": {"alias": "Acme V1"}} + }}} + }` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + models, _ := nestedValue(result.config, "models.providers.acme.models") + assert.Equal(t, map[string]any{"acme-model-v1": map[string]any{"alias": "Acme V1"}}, models, + "gap-fill must not touch an already-existing entry's Bucket-B content") + }) + + t.Run("gap-fill and reassertion do not cross-contaminate sibling entries", func(t *testing.T) { + operatorJSON := `{ + "gateway": {"port": 18789}, + "models": {"providers": { + "google": {"baseUrl": "https://google.example.com", "apiKey": "placeholder", "api": "openai-completions"}, + "newprov": {"baseUrl": "https://newprov.example.com", "apiKey": "placeholder", "api": "openai-completions"} + }} + }` + pvcJSON := `{ + "models": {"providers": {"google": { + "baseUrl": "https://google.example.com", "apiKey": "placeholder", "api": "openai-completions", + "models": {"custom-model": {"alias": "Custom"}} + }}} + }` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + newprov, hasNewprov := nestedValue(result.config, "models.providers.newprov") + assert.True(t, hasNewprov, "new sibling entry should appear via gap-fill") + assert.Equal(t, "https://newprov.example.com", newprov.(map[string]any)["baseUrl"]) + + googleModels, _ := nestedValue(result.config, "models.providers.google.models") + assert.Equal(t, map[string]any{"custom-model": map[string]any{"alias": "Custom"}}, googleModels, + "existing sibling's customization must remain untouched") + }) + + t.Run("hand-edited primary/fallbacks preserved", func(t *testing.T) { + operatorJSON := operatorJSONWithPrimaryAndFallback + pvcJSON := `{ + "agents": {"defaults": {"model": {"primary": "anthropic/claude-opus-4-7", "fallbacks": ["anthropic/claude-sonnet-4-6"]}}} + }` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + primary, _ := nestedValue(result.config, "agents.defaults.model.primary") + assert.Equal(t, "anthropic/claude-opus-4-7", primary, "hand-edited primary should be preserved") + fallbacks, _ := nestedValue(result.config, "agents.defaults.model.fallbacks") + assert.Equal(t, []any{"anthropic/claude-sonnet-4-6"}, fallbacks, "hand-edited fallbacks should be preserved") + }) + + t.Run("primary/fallbacks gap-filled when absent/empty and a catalog-eligible credential is added", func(t *testing.T) { + operatorJSON := operatorJSONWithPrimaryAndFallback + pvcJSON := `{ + "agents": {"defaults": {"workspace": "~/.openclaw/workspace"}} + }` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + primary, hasPrimary := nestedValue(result.config, "agents.defaults.model.primary") + assert.True(t, hasPrimary, "absent primary should be gap-filled") + assert.Equal(t, "google/gemini-3.1-pro-preview", primary) + fallbacks, hasFallbacks := nestedValue(result.config, "agents.defaults.model.fallbacks") + assert.True(t, hasFallbacks, "absent fallbacks should be gap-filled") + assert.Equal(t, []any{"google/gemini-3-flash-preview"}, fallbacks) + }) + + t.Run("only the corrupted entry changes among multiple declared providers", func(t *testing.T) { + operatorJSON := `{ + "gateway": {"port": 18789}, + "models": {"providers": { + "google": {"baseUrl": "https://google.example.com", "apiKey": "placeholder-g", "api": "openai-completions"}, + "openai": {"baseUrl": "https://openai.example.com", "apiKey": "placeholder-o", "api": "openai-completions"} + }} + }` + pvcJSON := `{ + "models": {"providers": { + "google": {"baseUrl": "https://hacked.example.com", "apiKey": "wrong", "api": "wrong"}, + "openai": {"baseUrl": "https://openai.example.com", "apiKey": "placeholder-o", "api": "openai-completions", "models": {"gpt-x": {"alias": "GPT-X"}}} + }} + }` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + googleBaseURL, _ := nestedValue(result.config, "models.providers.google.baseUrl") + assert.Equal(t, "https://google.example.com", googleBaseURL, "corrupted google provider should be corrected") + + openaiModels, _ := nestedValue(result.config, "models.providers.openai.models") + assert.Equal(t, map[string]any{"gpt-x": map[string]any{"alias": "GPT-X"}}, openaiModels, + "unaffected sibling's Bucket-B content should not be touched") + }) + + t.Run("infra keys and route host corrected, non-route origins preserved", func(t *testing.T) { + operatorJSON := `{ + "gateway": { + "mode": "local", "bind": "lan", "port": 18789, + "auth": {"mode": "token"}, + "controlUi": {"enabled": true, "allowedOrigins": ["https://route.example.com"], "dangerouslyDisableDeviceAuth": false}, + "trustedProxies": ["10.0.0.0/8", "172.16.0.0/12"] + }, + "tools": {"web": {"search": {"enabled": true, "provider": "tavily"}, "fetch": {"enabled": true}}}, + "agents": {"defaults": {"memorySearch": {"provider": "openai", "enabled": true}}}, + "diagnostics": {"otel": {"metrics": true, "metricsEndpoint": "http://otel.example.com:4318"}} + }` + pvcJSON := `{ + "gateway": { + "mode": "hacked", "bind": "0.0.0.0", "port": 9999, + "auth": {"mode": "password"}, + "controlUi": {"enabled": false, "allowedOrigins": ["https://user-custom.example.com"], "dangerouslyDisableDeviceAuth": true}, + "trustedProxies": ["1.2.3.4/32"] + }, + "tools": {"web": {"search": {"enabled": false, "provider": "hacked"}, "fetch": {"enabled": false}}}, + "agents": {"defaults": {"memorySearch": {"provider": "hacked", "enabled": false}}}, + "diagnostics": {"otel": {"metrics": false, "metricsEndpoint": "http://hacked.example.com"}} + }` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + mode, _ := nestedValue(result.config, "gateway.mode") + assert.Equal(t, "local", mode) + bind, _ := nestedValue(result.config, "gateway.bind") + assert.Equal(t, "lan", bind) + port, _ := nestedValue(result.config, "gateway.port") + assert.Equal(t, float64(18789), port) + authMode, _ := nestedValue(result.config, "gateway.auth.mode") + assert.Equal(t, "token", authMode) + enabled, _ := nestedValue(result.config, "gateway.controlUi.enabled") + assert.Equal(t, true, enabled) + deviceAuth, _ := nestedValue(result.config, "gateway.controlUi.dangerouslyDisableDeviceAuth") + assert.Equal(t, false, deviceAuth) + trustedProxies, _ := nestedValue(result.config, "gateway.trustedProxies") + assert.Equal(t, []any{"10.0.0.0/8", "172.16.0.0/12"}, trustedProxies) + + origins, _ := nestedValue(result.config, "gateway.controlUi.allowedOrigins") + assert.ElementsMatch(t, []any{"https://user-custom.example.com", "https://route.example.com"}, origins, + "route host should be appended; hand-added origin should be preserved") + + searchEnabled, _ := nestedValue(result.config, "tools.web.search.enabled") + assert.Equal(t, true, searchEnabled) + searchProvider, _ := nestedValue(result.config, "tools.web.search.provider") + assert.Equal(t, "tavily", searchProvider) + fetchEnabled, _ := nestedValue(result.config, "tools.web.fetch.enabled") + assert.Equal(t, true, fetchEnabled) + memProvider, _ := nestedValue(result.config, "agents.defaults.memorySearch.provider") + assert.Equal(t, "openai", memProvider) + memEnabled, _ := nestedValue(result.config, "agents.defaults.memorySearch.enabled") + assert.Equal(t, true, memEnabled) + otelMetrics, _ := nestedValue(result.config, "diagnostics.otel.metrics") + assert.Equal(t, true, otelMetrics) + otelEndpoint, _ := nestedValue(result.config, "diagnostics.otel.metricsEndpoint") + assert.Equal(t, "http://otel.example.com:4318", otelEndpoint) + }) + + t.Run("orphaned entry removed from CR is left untouched without erroring", func(t *testing.T) { + operatorJSON := `{"gateway": {"port": 18789}, "models": {"providers": {}}}` + pvcJSON := `{ + "models": {"providers": {"google": {"baseUrl": "https://google.example.com", "apiKey": "placeholder", "api": "openai-completions"}}} + }` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + google, hasGoogle := nestedValue(result.config, "models.providers.google") + assert.True(t, hasGoogle, "orphaned entry should remain in the file, untouched") + assert.Equal(t, "https://google.example.com", google.(map[string]any)["baseUrl"]) + }) + + t.Run("declared entry missing entirely from PVC file is added without erroring", func(t *testing.T) { + operatorJSON := `{ + "gateway": {"port": 18789}, + "channels": {"telegram": {"enabled": true, "botToken": "placeholder", "dmPolicy": "open", "allowFrom": ["*"]}} + }` + pvcJSON := `{"agents": {"defaults": {"workspace": "~/.openclaw/workspace"}}}` + + result := runMergeJS(t, mergeTestSetup{operatorJSON: operatorJSON, pvcJSON: pvcJSON, configMode: "seedOnly"}) + + telegram, hasTelegram := nestedValue(result.config, "channels.telegram") + assert.True(t, hasTelegram, "entry missing entirely from the PVC file should be added via gap-fill") + assert.Equal(t, true, telegram.(map[string]any)["enabled"]) + }) + + t.Run("skill docs are seeded once, user edits persist", func(t *testing.T) { + cmData := extractConfigMapData(t) + customSkill := "# My Custom Platform Notes\nEdited by the agent." + + result := runMergeJS(t, mergeTestSetup{ + configMode: "seedOnly", + pvcJSON: `{"agents": {"defaults": {"workspace": "~/.openclaw/workspace"}}}`, + pvcFiles: map[string]string{ + "workspace/skills/platform/SKILL.md": customSkill, + }, + }) + + content, err := os.ReadFile(filepath.Join(result.pvcDir, "workspace", "skills", "platform", "SKILL.md")) + require.NoError(t, err) + assert.Equal(t, customSkill, string(content), "seedOnly should not overwrite an existing skill doc") + assert.NotEqual(t, cmData["PLATFORM.md"], string(content)) + }) + + t.Run("skill docs are seeded on first boot under seedOnly", func(t *testing.T) { + cmData := extractConfigMapData(t) + + result := runMergeJS(t, mergeTestSetup{configMode: "seedOnly"}) + + content, err := os.ReadFile(filepath.Join(result.pvcDir, "workspace", "skills", "platform", "SKILL.md")) + require.NoError(t, err, "skill doc should be seeded on first boot even under seedOnly") + assert.Equal(t, cmData["PLATFORM.md"], string(content)) + }) } diff --git a/internal/controller/claw_operator_config.go b/internal/controller/claw_operator_config.go new file mode 100644 index 00000000..827e56f8 --- /dev/null +++ b/internal/controller/claw_operator_config.go @@ -0,0 +1,80 @@ +/* +Copyright 2026 Red Hat. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "fmt" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "sigs.k8s.io/controller-runtime/pkg/client" + + clawv1alpha1 "github.com/codeready-toolchain/claw-operator/api/v1alpha1" +) + +// effectiveConfigMode returns the mergeMode that will actually be used for +// this instance, defaulting to ConfigModeMerge when unset. +func effectiveConfigMode(instance *clawv1alpha1.Claw) clawv1alpha1.ConfigMode { + if instance.Spec.Config == nil || instance.Spec.Config.MergeMode == "" { + return clawv1alpha1.ConfigModeMerge + } + return instance.Spec.Config.MergeMode +} + +// checkConfigModeAllowed enforces cluster-admin policy set via the +// ClawOperatorConfig singleton (named ClawOperatorConfigSingletonName, in the +// operator's own runtime namespace — see docs/adr/0021-seed-only-config-mode.md). +// +// This fails open by design: if the singleton doesn't exist, or exists with +// an empty AllowedConfigModes, every mode is allowed. This preserves today's +// unrestricted behavior (no gating mechanism exists yet) until a cluster +// admin explicitly opts in to restricting it. Only a genuine API error (not +// "not found") is surfaced as an error — a missing or unrestricted policy is +// not a failure. +func (r *ClawResourceReconciler) checkConfigModeAllowed(ctx context.Context, instance *clawv1alpha1.Claw) (bool, error) { + // OperatorNamespace is unset in most unit tests (which don't exercise this + // gating) and would otherwise make the lookup below ambiguous/invalid; not + // knowing the operator's own namespace is equivalent to "no policy is + // configured" for gating purposes. + if r.OperatorNamespace == "" { + return true, nil + } + + mode := effectiveConfigMode(instance) + + opConfig := &clawv1alpha1.ClawOperatorConfig{} + err := r.Get(ctx, client.ObjectKey{ + Namespace: r.OperatorNamespace, + Name: clawv1alpha1.ClawOperatorConfigSingletonName, + }, opConfig) + if apierrors.IsNotFound(err) { + return true, nil + } + if err != nil { + return false, fmt.Errorf("failed to get ClawOperatorConfig %q: %w", clawv1alpha1.ClawOperatorConfigSingletonName, err) + } + + if len(opConfig.Spec.AllowedConfigModes) == 0 { + return true, nil + } + for _, allowedMode := range opConfig.Spec.AllowedConfigModes { + if allowedMode == mode { + return true, nil + } + } + return false, nil +} diff --git a/internal/controller/claw_operator_config_test.go b/internal/controller/claw_operator_config_test.go new file mode 100644 index 00000000..be1e519c --- /dev/null +++ b/internal/controller/claw_operator_config_test.go @@ -0,0 +1,423 @@ +/* +Copyright 2026 Red Hat. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + clawv1alpha1 "github.com/codeready-toolchain/claw-operator/api/v1alpha1" +) + +// setupOperatorNamespace creates a fresh namespace to stand in for the +// operator's own runtime namespace (WATCH_NAMESPACE), so ClawOperatorConfig +// gating tests don't collide with each other or with the "default" tenant +// namespace used by other tests. +func setupOperatorNamespace(t *testing.T, ctx context.Context, name string) string { + t.Helper() + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: name}} + require.NoError(t, k8sClient.Create(ctx, ns)) + t.Cleanup(func() { + _ = k8sClient.Delete(ctx, ns) + }) + return name +} + +func testClawWithMergeMode(name, namespace string, mode clawv1alpha1.ConfigMode) *clawv1alpha1.Claw { + instance := &clawv1alpha1.Claw{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, + } + if mode != "" { + instance.Spec.Config = &clawv1alpha1.ConfigSpec{MergeMode: mode} + } + return instance +} + +// TestClawOperatorConfigGating table-drives the four ways checkConfigModeAllowed +// can resolve for a given Claw: an explicit deny, an explicit allow, fail-open +// on a missing singleton, and fail-open on an empty allowlist. Each case +// shares the same create-singleton/create-Claw/reconcile/assert-condition +// shape; only the singleton's presence/allowlist, the Claw's mergeMode, and +// the expected outcome vary per case. +func TestClawOperatorConfigGating(t *testing.T) { + ctx := context.Background() + + tests := []struct { + name string + // singleton is nil when no ClawOperatorConfig should be created at + // all (exercises the fail-open "missing singleton" path); otherwise + // it's the AllowedConfigModes to set on the created singleton (an + // empty-but-non-nil slice exercises the fail-open "empty allowlist" + // path). + singleton []clawv1alpha1.ConfigMode + clawMode clawv1alpha1.ConfigMode + wantDenied bool + }{ + { + name: "disallowed mode sets Ready False with ConfigModeNotAllowed", + singleton: []clawv1alpha1.ConfigMode{clawv1alpha1.ConfigModeMerge}, + clawMode: clawv1alpha1.ConfigModeSeedOnly, + wantDenied: true, + }, + { + name: "allowed mode has no gating effect", + singleton: []clawv1alpha1.ConfigMode{clawv1alpha1.ConfigModeMerge, clawv1alpha1.ConfigModeSeedOnly}, + clawMode: clawv1alpha1.ConfigModeSeedOnly, + wantDenied: false, + }, + { + name: "no ClawOperatorConfig singleton fails open", + singleton: nil, + clawMode: clawv1alpha1.ConfigModeOverwrite, + wantDenied: false, + }, + { + name: "singleton with empty allowedConfigModes allows everything", + singleton: []clawv1alpha1.ConfigMode{}, + clawMode: clawv1alpha1.ConfigModeSeedOnly, + wantDenied: false, + }, + } + + for i, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + resourceName := fmt.Sprintf("gating-case-%d", i) + t.Cleanup(func() { deleteAndWaitAllResources(t, namespace, resourceName) }) + + opNamespace := setupOperatorNamespace(t, ctx, resourceName+"-op") + if tc.singleton != nil { + opConfig := &clawv1alpha1.ClawOperatorConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: clawv1alpha1.ClawOperatorConfigSingletonName, + Namespace: opNamespace, + }, + Spec: clawv1alpha1.ClawOperatorConfigSpec{AllowedConfigModes: tc.singleton}, + } + require.NoError(t, k8sClient.Create(ctx, opConfig)) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, opConfig) }) + } + + instance := testClawWithMergeMode(resourceName, namespace, tc.clawMode) + if !tc.wantDenied { + // Only the allowed/fail-open cases reconcile far enough to need + // real credentials; the denied case halts before credential + // resolution, so a missing secret there would false-pass. + secret := createTestAPIKeySecret(aiModelSecret, namespace, aiModelSecretKey, aiModelSecretValue) + require.NoError(t, k8sClient.Create(ctx, secret)) + instance.Spec.Credentials = testCredentials() + } + require.NoError(t, k8sClient.Create(ctx, instance)) + + reconciler := &ClawResourceReconciler{ + Client: k8sClient, + Scheme: scheme.Scheme, + UserSecretReader: k8sClient, + OperatorNamespace: opNamespace, + } + + _, err := reconciler.Reconcile(ctx, ctrl.Request{ + NamespacedName: client.ObjectKey{Name: resourceName, Namespace: namespace}, + }) + require.NoError(t, err, "reconcile should not return an error regardless of gating outcome "+ + "(a denial is a stable policy state, not a transient failure)") + + updated := &clawv1alpha1.Claw{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Name: resourceName, Namespace: namespace}, updated)) + cond := meta.FindStatusCondition(updated.Status.Conditions, clawv1alpha1.ConditionTypeReady) + if tc.wantDenied { + require.NotNil(t, cond, "Ready condition should be set") + assert.Equal(t, metav1.ConditionFalse, cond.Status) + assert.Equal(t, clawv1alpha1.ConditionReasonConfigModeNotAllowed, cond.Reason) + } else if cond != nil { + assert.NotEqual(t, clawv1alpha1.ConditionReasonConfigModeNotAllowed, cond.Reason, + "an allowed/fail-open case must never produce the gating failure reason") + } + }) + } +} + +// TestConfigModeRetroactiveEnforcement covers the gap left by gating alone: +// checkConfigModeAllowed only ever ran on a Claw's very first reconcile, so +// an already-running instance whose mode became disallowed later (an admin +// tightening policy) kept running in the disallowed mode forever, with only +// a Ready:False status flag to show for it. handlePolicyIdle closes that gap +// by actively scaling an out-of-policy instance to zero, the same way +// spec.idle does, so policy is enforceable fleet-wide and not just for +// brand-new Claws. See docs/adr/0021-seed-only-config-mode.md. +func TestConfigModeRetroactiveEnforcement(t *testing.T) { + ctx := context.Background() + resourceName := "retroactive-enforcement" + t.Cleanup(func() { deleteAndWaitAllResources(t, namespace, resourceName) }) + + opNamespace := setupOperatorNamespace(t, ctx, resourceName+"-op") + opConfig := &clawv1alpha1.ClawOperatorConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: clawv1alpha1.ClawOperatorConfigSingletonName, + Namespace: opNamespace, + }, + Spec: clawv1alpha1.ClawOperatorConfigSpec{ + AllowedConfigModes: []clawv1alpha1.ConfigMode{clawv1alpha1.ConfigModeMerge, clawv1alpha1.ConfigModeSeedOnly}, + }, + } + require.NoError(t, k8sClient.Create(ctx, opConfig)) + + secret := createTestAPIKeySecret(aiModelSecret, namespace, aiModelSecretKey, aiModelSecretValue) + require.NoError(t, k8sClient.Create(ctx, secret)) + + instance := testClawWithMergeMode(resourceName, namespace, clawv1alpha1.ConfigModeSeedOnly) + instance.Spec.Credentials = testCredentials() + require.NoError(t, k8sClient.Create(ctx, instance)) + + reconciler := &ClawResourceReconciler{ + Client: k8sClient, + Scheme: scheme.Scheme, + UserSecretReader: k8sClient, + OperatorNamespace: opNamespace, + } + + coreDeployments := []string{ + getClawDeploymentName(resourceName), + getProxyDeploymentName(resourceName), + } + + t.Run("runs normally while seedOnly is allowed", func(t *testing.T) { + reconcileClaw(t, ctx, reconciler, resourceName, namespace) + setCoreDeploymentsAvailable(t, ctx, resourceName, namespace) + reconcileClaw(t, ctx, reconciler, resourceName, namespace) + + for _, name := range coreDeployments { + deployment := &appsv1.Deployment{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, deployment)) + require.NotNil(t, deployment.Spec.Replicas) + assert.Equal(t, int32(1), *deployment.Spec.Replicas, "expected 1 replica on %s", name) + } + + updated := &clawv1alpha1.Claw{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Name: resourceName, Namespace: namespace}, updated)) + readyCond := meta.FindStatusCondition(updated.Status.Conditions, clawv1alpha1.ConditionTypeReady) + require.NotNil(t, readyCond) + assert.Equal(t, metav1.ConditionTrue, readyCond.Status) + }) + + t.Run("tightening policy scales the running instance to zero", func(t *testing.T) { + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{ + Name: clawv1alpha1.ClawOperatorConfigSingletonName, Namespace: opNamespace, + }, opConfig)) + opConfig.Spec.AllowedConfigModes = []clawv1alpha1.ConfigMode{clawv1alpha1.ConfigModeMerge} + require.NoError(t, k8sClient.Update(ctx, opConfig)) + + reconcileClaw(t, ctx, reconciler, resourceName, namespace) + + for _, name := range coreDeployments { + deployment := &appsv1.Deployment{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, deployment)) + require.NotNil(t, deployment.Spec.Replicas) + assert.Equal(t, int32(0), *deployment.Spec.Replicas, + "expected %s to be scaled to zero once seedOnly became disallowed", name) + } + + updated := &clawv1alpha1.Claw{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Name: resourceName, Namespace: namespace}, updated)) + + readyCond := meta.FindStatusCondition(updated.Status.Conditions, clawv1alpha1.ConditionTypeReady) + require.NotNil(t, readyCond) + assert.Equal(t, metav1.ConditionFalse, readyCond.Status) + assert.Equal(t, clawv1alpha1.ConditionReasonConfigModeNotAllowed, readyCond.Reason) + + idleCond := meta.FindStatusCondition(updated.Status.Conditions, clawv1alpha1.ConditionTypeIdle) + require.NotNil(t, idleCond, "Idle condition should be set when scaled to zero by policy") + assert.Equal(t, metav1.ConditionTrue, idleCond.Status) + assert.Equal(t, clawv1alpha1.ConditionReasonIdledByPolicy, idleCond.Reason) + + // Spec is never mutated on the user's behalf — the CR still literally + // requests seedOnly; only the running Deployments were touched. + assert.Equal(t, clawv1alpha1.ConfigModeSeedOnly, updated.Spec.Config.MergeMode) + }) + + t.Run("widening policy again restores normal operation", func(t *testing.T) { + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{ + Name: clawv1alpha1.ClawOperatorConfigSingletonName, Namespace: opNamespace, + }, opConfig)) + opConfig.Spec.AllowedConfigModes = []clawv1alpha1.ConfigMode{clawv1alpha1.ConfigModeMerge, clawv1alpha1.ConfigModeSeedOnly} + require.NoError(t, k8sClient.Update(ctx, opConfig)) + + reconcileClaw(t, ctx, reconciler, resourceName, namespace) + setCoreDeploymentsAvailable(t, ctx, resourceName, namespace) + reconcileClaw(t, ctx, reconciler, resourceName, namespace) + + for _, name := range coreDeployments { + deployment := &appsv1.Deployment{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, deployment)) + require.NotNil(t, deployment.Spec.Replicas) + assert.Equal(t, int32(1), *deployment.Spec.Replicas, "expected %s restored to 1 replica", name) + } + + updated := &clawv1alpha1.Claw{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Name: resourceName, Namespace: namespace}, updated)) + + assert.Nil(t, meta.FindStatusCondition(updated.Status.Conditions, clawv1alpha1.ConditionTypeIdle), + "Idle condition should be removed once policy allows the mode again") + + readyCond := meta.FindStatusCondition(updated.Status.Conditions, clawv1alpha1.ConditionTypeReady) + require.NotNil(t, readyCond) + assert.Equal(t, metav1.ConditionTrue, readyCond.Status) + }) +} + +func TestClawOperatorConfigNameValidation(t *testing.T) { + ctx := context.Background() + + t.Run("rejects a name other than the singleton at admission time", func(t *testing.T) { + opNamespace := setupOperatorNamespace(t, ctx, "name-validation-op") + opConfig := &clawv1alpha1.ClawOperatorConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "not-cluster", Namespace: opNamespace}, + } + + err := k8sClient.Create(ctx, opConfig) + require.Error(t, err, "the API server must reject a non-singleton name via the CEL XValidation rule") + assert.Contains(t, err.Error(), "must be named 'cluster'") + }) + + t.Run("accepts the singleton name", func(t *testing.T) { + opNamespace := setupOperatorNamespace(t, ctx, "name-validation-ok-op") + opConfig := &clawv1alpha1.ClawOperatorConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: clawv1alpha1.ClawOperatorConfigSingletonName, + Namespace: opNamespace, + }, + } + + require.NoError(t, k8sClient.Create(ctx, opConfig)) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, opConfig) }) + }) +} + +func TestFindAllClaws(t *testing.T) { + ctx := context.Background() + + t.Run("should map the singleton ClawOperatorConfig change to every Claw CR", func(t *testing.T) { + t.Cleanup(func() { deleteAndWaitAllResources(t, namespace) }) + createClawInstance(t, ctx, testInstanceName, namespace) + reconciler := &ClawResourceReconciler{ + Client: k8sClient, + Scheme: scheme.Scheme, + UserSecretReader: k8sClient, + OperatorNamespace: namespace, + } + + opConfig := &clawv1alpha1.ClawOperatorConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: clawv1alpha1.ClawOperatorConfigSingletonName, + Namespace: namespace, + }, + } + + requests := reconciler.findAllClaws(ctx, opConfig) + require.Len(t, requests, 1) + assert.Equal(t, testInstanceName, requests[0].Name) + assert.Equal(t, namespace, requests[0].Namespace) + }) + + t.Run("should return empty when no Claw CRs exist", func(t *testing.T) { + t.Cleanup(func() { deleteAndWaitAllResources(t, namespace) }) + reconciler := &ClawResourceReconciler{ + Client: k8sClient, + Scheme: scheme.Scheme, + UserSecretReader: k8sClient, + OperatorNamespace: namespace, + } + + opConfig := &clawv1alpha1.ClawOperatorConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: clawv1alpha1.ClawOperatorConfigSingletonName, + Namespace: namespace, + }, + } + + requests := reconciler.findAllClaws(ctx, opConfig) + assert.Empty(t, requests) + }) + + t.Run("should ignore a ClawOperatorConfig with the wrong name", func(t *testing.T) { + t.Cleanup(func() { deleteAndWaitAllResources(t, namespace) }) + createClawInstance(t, ctx, testInstanceName, namespace) + reconciler := &ClawResourceReconciler{ + Client: k8sClient, + Scheme: scheme.Scheme, + UserSecretReader: k8sClient, + OperatorNamespace: namespace, + } + + notTheSingleton := &clawv1alpha1.ClawOperatorConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "not-cluster", Namespace: namespace}, + } + + requests := reconciler.findAllClaws(ctx, notTheSingleton) + assert.Empty(t, requests, "only the singleton name should trigger a cluster-wide fan-out") + }) + + t.Run("should ignore a same-named ClawOperatorConfig outside the operator namespace", func(t *testing.T) { + t.Cleanup(func() { deleteAndWaitAllResources(t, namespace) }) + createClawInstance(t, ctx, testInstanceName, namespace) + reconciler := &ClawResourceReconciler{ + Client: k8sClient, + Scheme: scheme.Scheme, + UserSecretReader: k8sClient, + OperatorNamespace: "some-other-operator-namespace", + } + + wrongNamespace := &clawv1alpha1.ClawOperatorConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: clawv1alpha1.ClawOperatorConfigSingletonName, + Namespace: namespace, + }, + } + + requests := reconciler.findAllClaws(ctx, wrongNamespace) + assert.Empty(t, requests, "a same-named object outside the operator's own namespace is not the policy singleton") + }) +} + +func TestEffectiveConfigMode(t *testing.T) { + t.Run("defaults to merge when config is nil", func(t *testing.T) { + instance := &clawv1alpha1.Claw{} + assert.Equal(t, clawv1alpha1.ConfigModeMerge, effectiveConfigMode(instance)) + }) + + t.Run("defaults to merge when mergeMode is empty", func(t *testing.T) { + instance := &clawv1alpha1.Claw{Spec: clawv1alpha1.ClawSpec{Config: &clawv1alpha1.ConfigSpec{}}} + assert.Equal(t, clawv1alpha1.ConfigModeMerge, effectiveConfigMode(instance)) + }) + + t.Run("returns the explicit mergeMode when set", func(t *testing.T) { + instance := &clawv1alpha1.Claw{ + Spec: clawv1alpha1.ClawSpec{Config: &clawv1alpha1.ConfigSpec{MergeMode: clawv1alpha1.ConfigModeSeedOnly}}, + } + assert.Equal(t, clawv1alpha1.ConfigModeSeedOnly, effectiveConfigMode(instance)) + }) +} diff --git a/internal/controller/claw_plugins.go b/internal/controller/claw_plugins.go index ff2fac0d..4f38bad5 100644 --- a/internal/controller/claw_plugins.go +++ b/internal/controller/claw_plugins.go @@ -127,7 +127,56 @@ func requiredProviderPlugins(instance *clawv1alpha1.Claw) ([]string, error) { func generatePluginInstallScript(plugins []string) string { var b strings.Builder - b.WriteString(`set -e + + desiredPkgs := make([]string, 0, len(plugins)) + seenPkgs := make(map[string]bool, len(plugins)) + for _, p := range plugins { + pkg := pluginPackageName(p) + if !seenPkgs[pkg] { + seenPkgs[pkg] = true + desiredPkgs = append(desiredPkgs, pkg) + } + } + // Exported (not just assigned) because the node -e snippets below read it + // via process.env — a plain shell assignment is invisible to child processes. + fmt.Fprintf(&b, "set -e\nexport DESIRED_PKGS=%s\n", shellQuote(strings.Join(desiredPkgs, "\n"))) + + b.WriteString(` +# Some plugins (e.g. the Vertex AI SDK provider plugins) are backed by a +# scoped npm package that never materializes a directory under +# ~/.openclaw/extensions — openclaw installs their code under +# ~/.openclaw/npm/projects/ instead and tracks the install record +# purely in its internal registry (persisted in ~/.openclaw's sqlite state, +# not a plain file we could diff). The $EXT-directory diff below can never +# detect these as orphaned. +# +# REGISTRY_MANIFEST is our OWN record of exactly which plugin ids the +# operator itself installed last time — never the live registry as a whole. +# This matters because the registry can also contain plugins installed +# through some other channel (e.g. a marketplace/ClawHub install done +# directly against the running instance); we must never uninstall those. +# Only ids we ourselves previously wrote to this file, and that are no +# longer desired, get uninstalled — mirroring the same safety property the +# $EXT/.operator-managed manifest below already provides for directory-based +# plugins. +REGISTRY_MANIFEST="/home/node/.openclaw/.operator-managed-plugins" +if [ -f "$REGISTRY_MANIFEST" ]; then + node -e ' +const fs = require("fs"); +const desired = new Set((process.env.DESIRED_PKGS || "").split("\n").filter(Boolean)); +for (const line of fs.readFileSync(process.argv[1], "utf8").split("\n")) { + const tab = line.indexOf("\t"); + if (tab < 0) continue; + const id = line.slice(0, tab); + const pkg = line.slice(tab + 1); + if (id && pkg && !desired.has(pkg)) console.log(id); +} +' "$REGISTRY_MANIFEST" | while IFS= read -r orphan_id; do + [ -n "$orphan_id" ] && openclaw plugins uninstall "$orphan_id" --force >/dev/null 2>&1 + true + done +fi + EXT="/home/node/.openclaw/extensions" MANIFEST="$EXT/.operator-managed" if [ -f "$MANIFEST" ]; then @@ -147,12 +196,46 @@ else find "$EXT" -mindepth 1 -maxdepth 1 -type d -exec rm -rf {} + 2>/dev/null || true fi mkdir -p "$EXT" +# The openclaw CLI also tracks per-package install state under +# ~/.openclaw/npm/projects/, separate from $EXT above. It refuses to +# reinstall ("plugin already exists ... delete it first") if a stale project +# dir survives from a prior boot, so it's wiped unconditionally here — it's a +# pure install cache the CLI recreates from scratch on every install. +rm -rf "/home/node/.openclaw/npm/projects" ls "$EXT" 2>/dev/null | sort > /tmp/before-plugins.txt `) for _, pkg := range plugins { fmt.Fprintf(&b, "openclaw plugins install %s\n", shellQuote(pkg)) } b.WriteString(`ls "$EXT" | sort | comm -13 /tmp/before-plugins.txt - > "$MANIFEST" + +# Rebuild REGISTRY_MANIFEST from the registry, but only keep records whose +# package is one we ourselves just asked to have installed above — this is +# what guarantees the manifest can never "adopt" a plugin the operator +# didn't itself install, however it got into the registry. The file is +# written by node itself (fs.writeFileSync) rather than via shell stdout +# redirection, since the latter is not reliably flushed in all environments. +openclaw plugins registry --json 2>/dev/null | node -e ' +const fs = require("fs"); +const desired = new Set((process.env.DESIRED_PKGS || "").split("\n").filter(Boolean)); +let data = ""; +process.stdin.on("data", (c) => { data += c; }); +process.stdin.on("end", () => { + let registry; + try { registry = JSON.parse(data); } catch { registry = {}; } + const records = (registry.persisted && registry.persisted.installRecords) || {}; + const lines = []; + for (const [id, rec] of Object.entries(records)) { + const spec = rec.resolvedName || rec.spec || ""; + const idx = spec.lastIndexOf("@"); + const pkg = idx > 0 ? spec.slice(0, idx) : spec; + if (pkg && desired.has(pkg)) lines.push(id + "\t" + pkg); + } + try { + fs.writeFileSync(process.argv[1], lines.length ? lines.join("\n") + "\n" : ""); + } catch {} +}); +' "$REGISTRY_MANIFEST" 2>/dev/null || true `) return b.String() } diff --git a/internal/controller/claw_plugins_script_exec_test.go b/internal/controller/claw_plugins_script_exec_test.go new file mode 100644 index 00000000..58ec9a5d --- /dev/null +++ b/internal/controller/claw_plugins_script_exec_test.go @@ -0,0 +1,403 @@ +/* +Copyright 2026 Red Hat. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +// TestGeneratePluginInstallScript (claw_plugins_test.go) only ever +// string-matches the generated script; it can prove the right substrings are +// present but not that the shell control flow — manifest-based cleanup, the +// before/after directory diff, or the shell-quoting of untrusted plugin +// names — actually behaves correctly when a real shell runs it. That gap is +// exactly how the npm/projects cache idempotency bug (see +// generatePluginInstallScript's own comments) slipped through once already. +// +// These tests execute the real generated script under sh, redirecting its +// two hardcoded absolute paths (extensions dir, npm project cache) into a +// temp directory via string replacement — the same technique +// claw_merge_test.go and claw_seed_script_test.go use. The real `openclaw` +// CLI is replaced by a tiny fake on $PATH that just materializes a directory +// per "install" call, which is all the script's own logic depends on. + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeOpenclawScript stands in for the real openclaw CLI's +// `plugins install ` subcommand: it materializes a directory under +// $FAKE_EXT named after a filesystem-safe encoding of the package spec it +// was given, so tests can assert on exactly what the real script asked to +// have installed. +// +// It also stands in for `plugins registry --json` and `plugins uninstall +// `, backed by a flat "id\tpackage" file at $FAKE_REGISTRY: `install` +// appends a record keyed by a fake id derived from the package name, +// `registry --json` renders those records as the real CLI's +// persisted.installRecords shape, and `uninstall` removes the matching +// record and its $FAKE_EXT directory — mirroring the real CLI's behavior of +// cleaning up the npm project and the config registration together. +const fakeOpenclawScript = `#!/bin/sh +set -e +if [ "$1" = "plugins" ] && [ "$2" = "install" ]; then + safe=$(printf '%s' "$3" | tr -c 'a-zA-Z0-9_.-' '_') + mkdir -p "$FAKE_EXT/$safe" + touch "$FAKE_EXT/$safe/.installed" + pkg=$(printf '%s' "$3" | sed 's/@[^@/]*$//') + printf '%s\t%s\n' "$safe" "$pkg" >> "$FAKE_REGISTRY" + exit 0 +fi +if [ "$1" = "plugins" ] && [ "$2" = "registry" ]; then + printf '{"persisted":{"installRecords":{' + first=1 + if [ -f "$FAKE_REGISTRY" ]; then + while IFS="$(printf '\t')" read -r id pkg; do + [ -z "$id" ] && continue + [ "$first" = 1 ] || printf ',' + first=0 + printf '"%s":{"resolvedName":"%s"}' "$id" "$pkg" + done < "$FAKE_REGISTRY" + fi + printf '}}}\n' + exit 0 +fi +if [ "$1" = "plugins" ] && [ "$2" = "uninstall" ]; then + id="$3" + rm -rf "$FAKE_EXT/$id" + if [ -f "$FAKE_REGISTRY" ]; then + grep -v "^$id " "$FAKE_REGISTRY" > "$FAKE_REGISTRY.tmp" 2>/dev/null || true + mv "$FAKE_REGISTRY.tmp" "$FAKE_REGISTRY" + fi + exit 0 +fi +echo "unexpected fake openclaw invocation: $*" >&2 +exit 1 +` + +// sanitizePluginDirName mirrors fakeOpenclawScript's own sanitization so +// tests can predict the directory name a given plugin spec produces. +func sanitizePluginDirName(pkg string) string { + var b strings.Builder + for _, r := range pkg { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9', r == '_', r == '.', r == '-': + b.WriteRune(r) + default: + b.WriteRune('_') + } + } + return b.String() +} + +type pluginScriptResult struct { + stdout, stderr string + extDir string + manifestPath string + registryPath string + registryManifestPath string +} + +// runPluginInstallScript generates the real install script for plugins, +// pre-seeds a fake extensions directory per existingManifest/preExistingDirs, +// a fake *live* openclaw plugin registry per preExistingLiveRegistry, and +// the operator's own record of what it previously installed per +// preExistingOperatorManifest, then executes it for real against the +// fakeOpenclawScript stand-in. +// existingManifest == nil means no .operator-managed manifest file is +// written at all (exercising the "no prior manifest" cleanup branch). +// preExistingLiveRegistry and preExistingOperatorManifest entries are +// "id\tpackageName" lines. They are deliberately separate: the live registry +// simulates everything openclaw currently has installed (from any source), +// while the operator manifest simulates only what the operator itself +// recorded installing in a previous run — the script must only ever +// uninstall entries present in the latter. +func runPluginInstallScript( + t *testing.T, + plugins []string, + existingManifest []string, + preExistingExtDirs []string, + npmProjectsExists bool, + preExistingLiveRegistry []string, + preExistingOperatorManifest []string, +) pluginScriptResult { + t.Helper() + if _, err := exec.LookPath("sh"); err != nil { + t.Skip("sh not found in PATH, skipping plugin install script tests") + } + + tmpDir := t.TempDir() + extDir := filepath.Join(tmpDir, "extensions") + require.NoError(t, os.MkdirAll(extDir, 0o755)) + npmProjectsDir := filepath.Join(tmpDir, "npm-projects") + + for _, d := range preExistingExtDirs { + require.NoError(t, os.MkdirAll(filepath.Join(extDir, d), 0o755)) + } + manifestPath := filepath.Join(extDir, ".operator-managed") + if existingManifest != nil { + require.NoError(t, os.WriteFile(manifestPath, []byte(strings.Join(existingManifest, "\n")+"\n"), 0o644)) + } + if npmProjectsExists { + require.NoError(t, os.MkdirAll(npmProjectsDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(npmProjectsDir, "stale-cache-entry"), []byte("stale"), 0o644)) + } + + fakeBinDir := filepath.Join(tmpDir, "bin") + require.NoError(t, os.MkdirAll(fakeBinDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(fakeBinDir, "openclaw"), []byte(fakeOpenclawScript), 0o755)) + fakeRegistry := filepath.Join(tmpDir, "registry.tsv") + if preExistingLiveRegistry != nil { + require.NoError(t, os.WriteFile(fakeRegistry, []byte(strings.Join(preExistingLiveRegistry, "\n")+"\n"), 0o644)) + } + registryManifestPath := filepath.Join(tmpDir, "operator-managed-plugins") + if preExistingOperatorManifest != nil { + require.NoError(t, os.WriteFile(registryManifestPath, + []byte(strings.Join(preExistingOperatorManifest, "\n")+"\n"), 0o644)) + } + + script := generatePluginInstallScript(plugins) + require.Contains(t, script, `EXT="/home/node/.openclaw/extensions"`, + "generatePluginInstallScript EXT anchor changed — update this test's path substitution") + require.Contains(t, script, `rm -rf "/home/node/.openclaw/npm/projects"`, + "generatePluginInstallScript npm cache anchor changed — update this test's path substitution") + require.Contains(t, script, `REGISTRY_MANIFEST="/home/node/.openclaw/.operator-managed-plugins"`, + "generatePluginInstallScript REGISTRY_MANIFEST anchor changed — update this test's path substitution") + script = strings.Replace(script, "/home/node/.openclaw/extensions", extDir, 1) + script = strings.Replace(script, `rm -rf "/home/node/.openclaw/npm/projects"`, + fmt.Sprintf("rm -rf %q", npmProjectsDir), 1) + script = strings.Replace(script, "/home/node/.openclaw/.operator-managed-plugins", registryManifestPath, 1) + + if _, err := exec.LookPath("node"); err != nil { + t.Skip("node not found in PATH, skipping plugin install script tests") + } + cmd := exec.Command("sh", "-c", script) //nolint:gosec + cmd.Env = append(os.Environ(), + "PATH="+fakeBinDir+":"+os.Getenv("PATH"), "FAKE_EXT="+extDir, "FAKE_REGISTRY="+fakeRegistry) + var stdout, stderr strings.Builder + cmd.Stdout = &stdout + cmd.Stderr = &stderr + err := cmd.Run() + require.NoError(t, err, "plugin install script failed: stdout=%s stderr=%s", stdout.String(), stderr.String()) + + return pluginScriptResult{ + stdout: stdout.String(), stderr: stderr.String(), + extDir: extDir, manifestPath: manifestPath, + registryPath: fakeRegistry, registryManifestPath: registryManifestPath, + } +} + +func readManifestLines(t *testing.T, path string) []string { + t.Helper() + data, err := os.ReadFile(path) + require.NoError(t, err, "manifest file should exist after install") + return strings.Fields(string(data)) +} + +func TestGeneratePluginInstallScriptExecution(t *testing.T) { + t.Run("installs a plugin and records exactly it in the manifest", func(t *testing.T) { + result := runPluginInstallScript(t, []string{"@openclaw/matrix"}, nil, nil, false, nil, nil) + + dirName := sanitizePluginDirName("@openclaw/matrix") + _, err := os.Stat(filepath.Join(result.extDir, dirName, ".installed")) + require.NoError(t, err, "fake openclaw should have created the plugin dir") + + assert.Equal(t, []string{dirName}, readManifestLines(t, result.manifestPath)) + }) + + t.Run("removes only manifest-listed dirs and leaves unmanaged dirs alone", func(t *testing.T) { + result := runPluginInstallScript(t, []string{"@openclaw/new-plugin"}, + []string{"old-plugin-dir"}, + []string{"old-plugin-dir", "user-created-dir"}, + false, nil, nil) + + _, err := os.Stat(filepath.Join(result.extDir, "old-plugin-dir")) + assert.True(t, os.IsNotExist(err), "manifest-listed dir from a previous install should be removed") + + _, err = os.Stat(filepath.Join(result.extDir, "user-created-dir")) + assert.NoError(t, err, "a dir not tracked by the manifest should be left untouched") + + newDirName := sanitizePluginDirName("@openclaw/new-plugin") + manifest := readManifestLines(t, result.manifestPath) + assert.Equal(t, []string{newDirName}, manifest, + "new manifest should record only what was actually installed this run, not pre-existing untracked dirs") + }) + + t.Run("wipes all extension dirs when no manifest exists (orphan cleanup)", func(t *testing.T) { + result := runPluginInstallScript(t, []string{"@openclaw/x"}, nil, + []string{"orphan1", "orphan2"}, false, nil, nil) + + for _, orphan := range []string{"orphan1", "orphan2"} { + _, err := os.Stat(filepath.Join(result.extDir, orphan)) + assert.True(t, os.IsNotExist(err), "orphaned dir %q should be wiped when no manifest is present", orphan) + } + assert.Equal(t, []string{sanitizePluginDirName("@openclaw/x")}, readManifestLines(t, result.manifestPath)) + }) + + t.Run("unconditionally wipes the npm project install cache", func(t *testing.T) { + result := runPluginInstallScript(t, []string{"@openclaw/matrix"}, nil, nil, true, nil, nil) + + // npmProjectsDir is a sibling of extDir under the same temp root, + // matching how runPluginInstallScript lays out its fixtures. + npmProjectsDir := filepath.Join(filepath.Dir(result.extDir), "npm-projects") + _, err := os.Stat(npmProjectsDir) + assert.True(t, os.IsNotExist(err), + "npm project cache must be wiped unconditionally to avoid stale 'plugin already exists' errors") + }) + + t.Run("uninstalls an operator-managed registry-tracked plugin no longer desired, even with no $EXT footprint", + func(t *testing.T) { + // Simulates a provider plugin (e.g. the Vertex AI SDK providers) + // that openclaw tracks purely in its registry, never as a + // directory under $EXT — the pre-existing registry entry has no + // corresponding preExistingExtDirs entry. Both the live registry + // and the operator's own manifest agree the operator installed + // it previously, so it's safe to uninstall. + liveRegistry := []string{"anthropic-vertex\t@openclaw/anthropic-vertex-provider"} + result := runPluginInstallScript(t, []string{"@openclaw/new-plugin"}, nil, nil, false, + liveRegistry, liveRegistry) + + registryContent, err := os.ReadFile(result.registryPath) + require.NoError(t, err) + assert.NotContains(t, string(registryContent), "anthropic-vertex", + "an operator-managed registry-tracked plugin no longer desired should be uninstalled via the CLI") + assert.Contains(t, string(registryContent), sanitizePluginDirName("@openclaw/new-plugin"), + "the currently desired plugin should still be recorded in the registry") + + newManifest, err := os.ReadFile(result.registryManifestPath) + require.NoError(t, err) + assert.NotContains(t, string(newManifest), "anthropic-vertex", + "the rebuilt operator manifest must drop entries that are no longer desired") + assert.Contains(t, string(newManifest), sanitizePluginDirName("@openclaw/new-plugin"), + "the rebuilt operator manifest should record the newly installed plugin") + }) + + t.Run("leaves an operator-managed registry-tracked plugin alone when it is still desired", func(t *testing.T) { + liveRegistry := []string{"anthropic-vertex\t@openclaw/anthropic-vertex-provider"} + result := runPluginInstallScript(t, []string{"@openclaw/anthropic-vertex-provider@2026.6.11"}, nil, nil, false, + liveRegistry, liveRegistry) + + registryContent, err := os.ReadFile(result.registryPath) + require.NoError(t, err) + assert.Contains(t, string(registryContent), "anthropic-vertex", + "a still-desired registry-tracked plugin must not be uninstalled") + + newManifest, err := os.ReadFile(result.registryManifestPath) + require.NoError(t, err) + assert.Contains(t, string(newManifest), "anthropic-vertex", + "a still-desired plugin should remain recorded in the rebuilt operator manifest") + }) + + t.Run("never uninstalls a registry entry the operator did not itself install", func(t *testing.T) { + // The live registry has a plugin (e.g. installed directly by a user + // or via some other channel outside the operator's control) that is + // NOT in the operator's own manifest — simulating no prior manifest + // entry for it at all. Even though it's not in the current desired + // list either, the script must leave it alone: only entries the + // operator itself previously recorded are candidates for cleanup. + result := runPluginInstallScript(t, []string{"@openclaw/new-plugin"}, nil, nil, false, + []string{"user-installed\t@some-org/user-plugin"}, nil) + + registryContent, err := os.ReadFile(result.registryPath) + require.NoError(t, err) + assert.Contains(t, string(registryContent), "user-installed", + "a plugin present in the live registry but absent from the operator's own manifest must never be uninstalled") + }) + + t.Run("does not execute shell metacharacters embedded in a plugin name", func(t *testing.T) { + if _, err := exec.LookPath("sh"); err != nil { + t.Skip("sh not found in PATH") + } + tmpDir := t.TempDir() + canary := filepath.Join(tmpDir, "pwned") + + extDir := filepath.Join(tmpDir, "extensions") + require.NoError(t, os.MkdirAll(extDir, 0o755)) + fakeBinDir := filepath.Join(tmpDir, "bin") + require.NoError(t, os.MkdirAll(fakeBinDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(fakeBinDir, "openclaw"), []byte(fakeOpenclawScript), 0o755)) + + maliciousPlugin := `x'; touch "$CANARY"; echo '` + script := generatePluginInstallScript([]string{maliciousPlugin}) + script = strings.Replace(script, "/home/node/.openclaw/extensions", extDir, 1) + script = strings.Replace(script, `rm -rf "/home/node/.openclaw/npm/projects"`, + fmt.Sprintf("rm -rf %q", filepath.Join(tmpDir, "npm-projects")), 1) + script = strings.Replace(script, "/home/node/.openclaw/.operator-managed-plugins", + filepath.Join(tmpDir, "operator-managed-plugins"), 1) + + cmd := exec.Command("sh", "-c", script) //nolint:gosec + cmd.Env = append(os.Environ(), + "PATH="+fakeBinDir+":"+os.Getenv("PATH"), + "FAKE_EXT="+extDir, + "FAKE_REGISTRY="+filepath.Join(tmpDir, "registry.tsv"), + "CANARY="+canary, + ) + var stdout, stderr strings.Builder + cmd.Stdout = &stdout + cmd.Stderr = &stderr + err := cmd.Run() + require.NoError(t, err, "script should still succeed by treating the payload as an opaque package name: "+ + "stdout=%s stderr=%s", stdout.String(), stderr.String()) + + _, statErr := os.Stat(canary) + assert.True(t, os.IsNotExist(statErr), + "shell metacharacters in a plugin name must never execute — canary file should not have been created") + }) + + t.Run("does not delete outside $EXT even if a manifest entry contains a path-traversal segment", func(t *testing.T) { + tmpDir := t.TempDir() + extDir := filepath.Join(tmpDir, "extensions") + require.NoError(t, os.MkdirAll(extDir, 0o755)) + + outside := filepath.Join(tmpDir, "evil-outside") + require.NoError(t, os.MkdirAll(outside, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(outside, "marker"), []byte("still here"), 0o644)) + + require.NoError(t, os.WriteFile(filepath.Join(extDir, ".operator-managed"), + []byte("../evil-outside\n"), 0o644)) + + fakeBinDir := filepath.Join(tmpDir, "bin") + require.NoError(t, os.MkdirAll(fakeBinDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(fakeBinDir, "openclaw"), []byte(fakeOpenclawScript), 0o755)) + + script := generatePluginInstallScript([]string{"@openclaw/matrix"}) + script = strings.Replace(script, "/home/node/.openclaw/extensions", extDir, 1) + script = strings.Replace(script, `rm -rf "/home/node/.openclaw/npm/projects"`, + fmt.Sprintf("rm -rf %q", filepath.Join(tmpDir, "npm-projects")), 1) + script = strings.Replace(script, "/home/node/.openclaw/.operator-managed-plugins", + filepath.Join(tmpDir, "operator-managed-plugins"), 1) + + cmd := exec.Command("sh", "-c", script) //nolint:gosec + cmd.Env = append(os.Environ(), + "PATH="+fakeBinDir+":"+os.Getenv("PATH"), + "FAKE_EXT="+extDir, + "FAKE_REGISTRY="+filepath.Join(tmpDir, "registry.tsv"), + ) + var stdout, stderr strings.Builder + cmd.Stdout = &stdout + cmd.Stderr = &stderr + err := cmd.Run() + require.NoError(t, err, "stdout=%s stderr=%s", stdout.String(), stderr.String()) + + _, err = os.Stat(filepath.Join(outside, "marker")) + assert.NoError(t, err, "path-traversal manifest entry must not cause deletion outside $EXT") + }) +} diff --git a/internal/controller/claw_plugins_test.go b/internal/controller/claw_plugins_test.go index 028460db..86f3188b 100644 --- a/internal/controller/claw_plugins_test.go +++ b/internal/controller/claw_plugins_test.go @@ -175,6 +175,17 @@ func TestGeneratePluginInstallScript(t *testing.T) { assert.Contains(t, script, `mkdir -p "$EXT"`) assert.Contains(t, script, `ls "$EXT"`) }) + + t.Run("should clear the openclaw CLI's npm project cache before installing", func(t *testing.T) { + script := generatePluginInstallScript([]string{"@openclaw/matrix"}) + assert.Contains(t, script, `rm -rf "/home/node/.openclaw/npm/projects"`) + + npmCleanupIdx := strings.Index(script, `rm -rf "/home/node/.openclaw/npm/projects"`) + installIdx := strings.Index(script, "openclaw plugins install") + require.Greater(t, npmCleanupIdx, 0) + require.Greater(t, installIdx, 0) + assert.Less(t, npmCleanupIdx, installIdx, "npm project cache cleanup should happen before install") + }) } // --- configurePluginsInitContainer tests --- diff --git a/internal/controller/claw_resource_controller.go b/internal/controller/claw_resource_controller.go index f6f5dd6b..9a1254dd 100644 --- a/internal/controller/claw_resource_controller.go +++ b/internal/controller/claw_resource_controller.go @@ -402,6 +402,10 @@ type ClawResourceReconciler struct { GitSyncImage string OTelCollectorImage string ImagePullPolicy string + // OperatorNamespace is the namespace the operator itself runs in (from the + // WATCH_NAMESPACE downward-API env var). The ClawOperatorConfig singleton + // is only ever looked up here, never in a tenant namespace. + OperatorNamespace string // MetricsRefreshed is closed by Start() after the initial metrics refresh. // Reconcile() waits on it so no reconciliation runs before metrics are populated. MetricsRefreshed chan struct{} @@ -412,7 +416,8 @@ type ClawResourceReconciler struct { // +kubebuilder:rbac:groups=claw.sandbox.redhat.com,resources=claws,verbs=get;list;watch // +kubebuilder:rbac:groups=claw.sandbox.redhat.com,resources=claws/status,verbs=get;update;patch // +kubebuilder:rbac:groups=claw.sandbox.redhat.com,resources=claws/finalizers,verbs=update -// +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch;create;update;patch +// +kubebuilder:rbac:groups=claw.sandbox.redhat.com,resources=clawoperatorconfigs,verbs=get;list;watch +// +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch // +kubebuilder:rbac:groups="",resources=persistentvolumeclaims,verbs=get;list;watch;create;update;patch // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;patch;delete @@ -449,6 +454,22 @@ func (r *ClawResourceReconciler) Reconcile(ctx context.Context, req ctrl.Request return ctrl.Result{}, err } + // Enforce cluster-admin policy on which spec.config.mergeMode values are + // allowed (ClawOperatorConfig). Fails open when no policy is configured. + // A denial doesn't just block a brand-new Claw's first reconcile — it + // actively scales an already-running instance to zero (handlePolicyIdle), + // so policy is enforceable fleet-wide, not only at creation time. + allowed, err := r.checkConfigModeAllowed(ctx, instance) + if err != nil { + logger.Error(err, "Failed to evaluate ClawOperatorConfig policy") + return ctrl.Result{}, err + } + if !allowed { + logger.Info("mergeMode not allowed by ClawOperatorConfig, idling instance", + "mode", effectiveConfigMode(instance)) + return r.handlePolicyIdle(ctx, instance) + } + // Short-circuit when idled — scale deployments to zero and return if instance.Spec.Idle { return r.handleIdle(ctx, instance) @@ -1580,10 +1601,46 @@ func (r *ClawResourceReconciler) SetupWithManager(mgr ctrl.Manager) error { handler.EnqueueRequestsFromMapFunc(r.findClawsReferencingSecret), builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}), ). + Watches( + &clawv1alpha1.ClawOperatorConfig{}, + handler.EnqueueRequestsFromMapFunc(r.findAllClaws), + builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}), + ). Named("claw"). Complete(r) } +// findAllClaws maps a ClawOperatorConfig change to every Claw CR in the +// cluster, so tightening/loosening admin policy is reflected in each Claw's +// Ready condition promptly instead of waiting for an unrelated reconcile +// trigger (e.g. a CR edit or credential rotation). Only the singleton +// ClawOperatorConfig actually consulted by checkConfigModeAllowed (name +// ClawOperatorConfigSingletonName, in the operator's own namespace) +// triggers this cluster-wide fan-out; any other ClawOperatorConfig object +// (e.g. a stray one in a tenant namespace) is never looked up by the +// reconciler and so is ignored here too. +func (r *ClawResourceReconciler) findAllClaws(ctx context.Context, obj client.Object) []reconcile.Request { + if obj.GetName() != clawv1alpha1.ClawOperatorConfigSingletonName || obj.GetNamespace() != r.OperatorNamespace { + return nil + } + + openClawList := &clawv1alpha1.ClawList{} + if err := r.List(ctx, openClawList); err != nil { + return nil + } + + requests := make([]reconcile.Request, 0, len(openClawList.Items)) + for _, instance := range openClawList.Items { + requests = append(requests, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: instance.Name, + Namespace: instance.Namespace, + }, + }) + } + return requests +} + // findClawsReferencingSecret maps a Secret change to the Claw(s) that need // re-reconciliation. Operator-owned Secrets (with an owner ref pointing to a // Claw) are handled by Owns(&corev1.Secret{}) and skipped here. For diff --git a/internal/controller/claw_seed_script_test.go b/internal/controller/claw_seed_script_test.go new file mode 100644 index 00000000..1bf0602d --- /dev/null +++ b/internal/controller/claw_seed_script_test.go @@ -0,0 +1,222 @@ +/* +Copyright 2026 Red Hat. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +// seedScript (internal/controller/claw_plugins.go) had zero test coverage of +// any kind before this file: not even string-matching. It's real logic that +// runs in the init-seed container on every single pod boot (shell control +// flow, tab-delimited IFS parsing via an embedded `node -e` one-liner, +// seedIfMissing vs overwrite semantics), so it deserves the same "actually +// execute it" treatment claw_merge_test.go already gives merge.js. +// +// The two hardcoded absolute paths the real script uses in production +// (MANIFEST, WORKSPACE) are redirected to per-test temp directories via +// targeted string replacement before execution — the same technique +// claw_merge_test.go uses for merge.js's configDir/pvcDir. Manifest "source" +// entries already are, by design, absolute paths the real script just +// `cp`'s from, so tests can point them straight at temp fixture files +// without any further indirection. + +import ( + "encoding/json" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type seedScriptResult struct { + stdout string + workspaceDir string +} + +// runSeedScript executes the real seedScript constant under sh, with MANIFEST +// and WORKSPACE redirected into tmpDir. Passing a nil manifest skips writing +// the manifest file entirely, exercising the "no manifest found" fast path; +// pass an empty (non-nil) slice to exercise an empty-but-present manifest. +func runSeedScript(t *testing.T, tmpDir string, manifest []seedManifestEntry) seedScriptResult { + t.Helper() + if _, err := exec.LookPath("node"); err != nil { + t.Skip("node not found in PATH, skipping seedScript tests") + } + + workspaceDir := filepath.Join(tmpDir, "workspace") + require.NoError(t, os.MkdirAll(workspaceDir, 0o755)) + + manifestPath := filepath.Join(tmpDir, "_seed_manifest.json") + if manifest != nil { + data, err := json.Marshal(manifest) + require.NoError(t, err) + require.NoError(t, os.WriteFile(manifestPath, data, 0o644)) + } + + script := seedScript + require.Contains(t, script, `MANIFEST="/config/_seed_manifest.json"`, + "seedScript MANIFEST anchor changed — update this test's path substitution") + require.Contains(t, script, `WORKSPACE="/home/node/.openclaw/workspace"`, + "seedScript WORKSPACE anchor changed — update this test's path substitution") + script = strings.Replace(script, `MANIFEST="/config/_seed_manifest.json"`, + fmt.Sprintf("MANIFEST=%q", manifestPath), 1) + script = strings.Replace(script, `WORKSPACE="/home/node/.openclaw/workspace"`, + fmt.Sprintf("WORKSPACE=%q", workspaceDir), 1) + + cmd := exec.Command("sh", "-c", script) //nolint:gosec + var stdout, stderr strings.Builder + cmd.Stdout = &stdout + cmd.Stderr = &stderr + err := cmd.Run() + require.NoError(t, err, "seedScript failed: stdout=%s stderr=%s", stdout.String(), stderr.String()) + + return seedScriptResult{stdout: stdout.String(), workspaceDir: workspaceDir} +} + +func writeSourceFile(t *testing.T, dir, name, content string) string { + t.Helper() + path := filepath.Join(dir, name) + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + return path +} + +func TestSeedScript(t *testing.T) { + if _, err := exec.LookPath("node"); err != nil { + t.Skip("node not found in PATH, skipping seedScript tests") + } + + t.Run("seeds a missing target file", func(t *testing.T) { + tmpDir := t.TempDir() + src := writeSourceFile(t, tmpDir, "AGENTS.md", "hello agents") + + result := runSeedScript(t, tmpDir, []seedManifestEntry{ + {Source: src, Target: "AGENTS.md", Mode: "seedIfMissing"}, + }) + + content, err := os.ReadFile(filepath.Join(result.workspaceDir, "AGENTS.md")) + require.NoError(t, err) + assert.Equal(t, "hello agents", string(content)) + assert.Contains(t, result.stdout, "seeded: AGENTS.md") + }) + + t.Run("seedIfMissing skips an existing target and preserves its content", func(t *testing.T) { + tmpDir := t.TempDir() + src := writeSourceFile(t, tmpDir, "SOUL.md", "operator default soul") + require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, "workspace"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "workspace", "SOUL.md"), []byte("user's own soul"), 0o644)) + + result := runSeedScript(t, tmpDir, []seedManifestEntry{ + {Source: src, Target: "SOUL.md", Mode: "seedIfMissing"}, + }) + + content, err := os.ReadFile(filepath.Join(result.workspaceDir, "SOUL.md")) + require.NoError(t, err) + assert.Equal(t, "user's own soul", string(content), "existing user file must not be overwritten") + assert.Contains(t, result.stdout, "skip (exists): SOUL.md") + }) + + t.Run("overwrite mode always replaces the target", func(t *testing.T) { + tmpDir := t.TempDir() + src := writeSourceFile(t, tmpDir, "TOOLS.md", "new tools content") + require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, "workspace"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "workspace", "TOOLS.md"), []byte("old tools content"), 0o644)) + + result := runSeedScript(t, tmpDir, []seedManifestEntry{ + {Source: src, Target: "TOOLS.md", Mode: "overwrite"}, + }) + + content, err := os.ReadFile(filepath.Join(result.workspaceDir, "TOOLS.md")) + require.NoError(t, err) + assert.Equal(t, "new tools content", string(content)) + }) + + t.Run("creates nested target directories on demand", func(t *testing.T) { + tmpDir := t.TempDir() + src := writeSourceFile(t, tmpDir, "nested.md", "nested content") + + result := runSeedScript(t, tmpDir, []seedManifestEntry{ + {Source: src, Target: "a/b/c/nested.md", Mode: "overwrite"}, + }) + + content, err := os.ReadFile(filepath.Join(result.workspaceDir, "a", "b", "c", "nested.md")) + require.NoError(t, err) + assert.Equal(t, "nested content", string(content)) + }) + + t.Run("warns and continues past a missing source without failing the script", func(t *testing.T) { + tmpDir := t.TempDir() + goodSrc := writeSourceFile(t, tmpDir, "good.md", "good content") + missingSrc := filepath.Join(tmpDir, "does-not-exist.md") + + result := runSeedScript(t, tmpDir, []seedManifestEntry{ + {Source: missingSrc, Target: "missing.md", Mode: "overwrite"}, + {Source: goodSrc, Target: "good.md", Mode: "overwrite"}, + }) + + assert.Contains(t, result.stdout, "WARN: source not found") + _, err := os.Stat(filepath.Join(result.workspaceDir, "missing.md")) + assert.True(t, os.IsNotExist(err), "no file should be created for a missing source") + + content, err := os.ReadFile(filepath.Join(result.workspaceDir, "good.md")) + require.NoError(t, err, "processing must continue to subsequent manifest entries after a missing source") + assert.Equal(t, "good content", string(content)) + }) + + t.Run("handles multiple manifest entries independently", func(t *testing.T) { + tmpDir := t.TempDir() + src1 := writeSourceFile(t, tmpDir, "one.md", "content one") + src2 := writeSourceFile(t, tmpDir, "two.md", "content two") + + result := runSeedScript(t, tmpDir, []seedManifestEntry{ + {Source: src1, Target: "one.md", Mode: "overwrite"}, + {Source: src2, Target: "two.md", Mode: "overwrite"}, + }) + + c1, err := os.ReadFile(filepath.Join(result.workspaceDir, "one.md")) + require.NoError(t, err) + assert.Equal(t, "content one", string(c1)) + c2, err := os.ReadFile(filepath.Join(result.workspaceDir, "two.md")) + require.NoError(t, err) + assert.Equal(t, "content two", string(c2)) + }) + + t.Run("skips gracefully when no manifest file exists at all", func(t *testing.T) { + tmpDir := t.TempDir() + + result := runSeedScript(t, tmpDir, nil) + + assert.Contains(t, result.stdout, "no seed manifest found, skipping") + entries, err := os.ReadDir(result.workspaceDir) + require.NoError(t, err) + assert.Empty(t, entries, "workspace should be untouched when there is no manifest") + }) + + t.Run("preserves spaces in target paths despite tab-delimited IFS parsing", func(t *testing.T) { + tmpDir := t.TempDir() + src := writeSourceFile(t, tmpDir, "notes.md", "note content") + + result := runSeedScript(t, tmpDir, []seedManifestEntry{ + {Source: src, Target: "my notes/AGENTS.md", Mode: "overwrite"}, + }) + + content, err := os.ReadFile(filepath.Join(result.workspaceDir, "my notes", "AGENTS.md")) + require.NoError(t, err, "target path containing a space must be handled correctly by the tab-only IFS read loop") + assert.Equal(t, "note content", string(content)) + }) +} diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 5f130b43..a2fe4911 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -682,6 +682,47 @@ func TestManager(t *testing.T) { //nolint:gocyclo kubeMd, err := utils.Run(t, cmd) require.NoError(t, err) assert.Empty(t, kubeMd, "KUBERNETES.md should not exist without kubernetes credentials") + + // Everything above only inspects the *inputs* to merge.js (the ConfigMap + // keys and the init-config command line) — it never confirms merge.js + // actually ran correctly and produced the right file on the PVC. Wait for + // the pod and exec in to check the real output. + t.Log("waiting for gateway pod to be Running (init-config must have succeeded)") + err = wait.PollUntilContextTimeout(ctx, pollInterval, defaultTimeout, true, + func(ctx context.Context) (bool, error) { + cmd := exec.Command("kubectl", "get", "pods", + "-l", "app=claw", + "-o", "jsonpath={.items[0].status.phase}", + "-n", userNamespace) + output, err := utils.Run(t, cmd) + return err == nil && output == podPhaseRunning, nil + }) + require.NoError(t, err, "Gateway pod did not reach Running — init-config/merge.js may have failed") + + cmd = exec.Command("kubectl", "get", "pods", "-l", "app=claw", + "-o", "jsonpath={.items[0].metadata.name}", "-n", userNamespace) + podName, err := utils.Run(t, cmd) + require.NoError(t, err) + + t.Log("verifying merge.js actually produced a merged openclaw.json on the PVC " + + "(not just that the right ConfigMap keys and init command exist)") + cmd = exec.Command("kubectl", "exec", podName, "-c", controller.ClawGatewayContainerName, + "-n", userNamespace, "--", "cat", "/home/node/.openclaw/openclaw.json") + mergedOnDisk, err := utils.Run(t, cmd) + require.NoError(t, err, "failed to read merged openclaw.json from the gateway container") + + var merged map[string]any + require.NoError(t, json.Unmarshal([]byte(mergedOnDisk), &merged), + "merged openclaw.json on disk should be valid JSON") + assert.Contains(t, merged, "gateway", + "merged config on disk should have the gateway section (from operator.json)") + agentsOnDisk, ok := merged["agents"].(map[string]any) + require.True(t, ok, "merged config on disk should have an agents section") + defaultsOnDisk, ok := agentsOnDisk["defaults"].(map[string]any) + require.True(t, ok, "merged config on disk should have agents.defaults") + assert.NotEmpty(t, defaultsOnDisk["models"], + "merged config on disk should have the operator-injected model catalog, "+ + "proving merge.js actually merged operator.json into the PVC copy rather than just seeding it") }) t.Run("should wire credential env var with correct Secret reference", func(t *testing.T) { @@ -1428,6 +1469,37 @@ spec: require.NoError(t, err, "failed to get init-plugins logs") assert.Contains(t, pluginLogs, "anthropic-vertex-provider", "init-plugins logs should mention the anthropic-vertex-provider plugin") + + // The Vertex AI SDK provider plugin is a scoped npm package that + // openclaw installs under ~/.openclaw/npm/projects (tracked + // purely in its internal registry), not as a directory under + // ~/.openclaw/extensions — so verify it there instead, plus in + // the operator's own install-tracking manifest and the CLI's + // live registry, rather than asserting on the (irrelevant) + // extensions directory. + t.Log("verifying the vertex plugin actually landed on the PVC, not just in the logs") + cmd = exec.Command("kubectl", "exec", podName, "-c", controller.ClawGatewayContainerName, + "-n", userNamespace, "--", "sh", "-c", "ls /home/node/.openclaw/npm/projects") + npmLs, err := utils.Run(t, cmd) + require.NoError(t, err, "failed to list the npm projects directory on the pod") + assert.NotEmpty(t, strings.TrimSpace(npmLs), + "npm projects directory on the PVC should contain the installed vertex plugin's project") + + t.Log("verifying the operator's own plugin-tracking manifest records the installed plugin") + cmd = exec.Command("kubectl", "exec", podName, "-c", controller.ClawGatewayContainerName, + "-n", userNamespace, "--", "cat", "/home/node/.openclaw/.operator-managed-plugins") + manifestOnDisk, err := utils.Run(t, cmd) + require.NoError(t, err, "failed to read the operator's plugin-tracking manifest from the pod") + assert.Contains(t, manifestOnDisk, "anthropic-vertex-provider", + "operator plugin-tracking manifest should record the installed vertex plugin package") + + t.Log("verifying the openclaw CLI's own registry records the installed plugin") + cmd = exec.Command("kubectl", "exec", podName, "-c", controller.ClawGatewayContainerName, + "-n", userNamespace, "--", "openclaw", "plugins", "registry", "--json") + registryJSON, err := utils.Run(t, cmd) + require.NoError(t, err, "failed to query the openclaw plugin registry from the pod") + assert.Contains(t, registryJSON, "anthropic-vertex-provider", + "openclaw's own plugin registry should record the installed vertex plugin") }) t.Run("should wire Slack dual-token credential with separate env vars per role", func(t *testing.T) { @@ -2077,6 +2149,15 @@ spec: require.NoError(t, err, "failed to get init-seed logs") assert.Contains(t, seedLogs, "SOUL.md", "init-seed logs should mention SOUL.md") + + t.Log("verifying SOUL.md was actually seeded onto the PVC with the ConfigMap source's content " + + "(logs only prove a message was printed, not that the right bytes landed on disk)") + cmd = exec.Command("kubectl", "exec", podName, "-c", controller.ClawGatewayContainerName, + "-n", userNamespace, "--", "cat", "/home/node/.openclaw/workspace/SOUL.md") + soulOnDisk, err := utils.Run(t, cmd) + require.NoError(t, err, "failed to read seeded SOUL.md from the gateway container") + assert.Equal(t, "# Enterprise Soul\nYou are an enterprise assistant.", strings.TrimRight(soulOnDisk, "\n"), + "seeded SOUL.md content on disk should exactly match the ConfigMap source") }) t.Run("should wire git source init container and volumes", func(t *testing.T) {