feat: add seedOnly config mode for user-owned OpenClaw runtime config#226
Conversation
Addresses codeready-toolchain#224: some deployments need the operator to keep reconciling openclaw.json forever, others need the user/agent to own it after initial provisioning. Adds an opt-in `seedOnly` mergeMode plus cluster-admin gating, per docs/adr/0021-seed-only-config-mode.md. - New `seedOnly` value for `spec.config.mergeMode`: seeds openclaw.json once on first boot, then treats the PVC file as user-owned. - Two-bucket ownership model in merge.js: Bucket-A (credentials, gateway wiring, proxy/security paths) is reasserted at the field level on every restart; Bucket-B (everything else a user/agent can touch) is only gap-filled for newly CR-declared entries, never overwritten in place. - New namespaced singleton CRD `ClawOperatorConfig` (name "cluster", in the operator's own namespace) lets cluster admins restrict which mergeMode values are allowed; gating fails open (unrestricted) when the singleton is absent, so existing clusters are unaffected by default. - Disallowed mode surfaces as `Ready: False` / `ConfigModeNotAllowed` without erroring, since it's a stable policy mismatch rather than a transient failure (avoids exponential-backoff requeue). - `injectMcpServers` adds a private `_seedOnlyMeta` marker so merge.js can tell which MCP server entries carry credentials/URLs and need full reassertion, since that can't be inferred from JSON shape alone; the marker is stripped before writing to the PVC in every mode. - Differs from a simpler "just skip merging" approach: path-level reassertion (vs. whole-entry) was required so that, e.g., a user's custom models list under a provider isn't clobbered just because its baseUrl/apiKey must stay operator-owned. Signed-off-by: Alexey Kazakov <alkazako@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
… gating) - merge.js: treat null/scalar/array-shaped declared provider, channel, and MCP bucket-A entries as broken and replace them wholesale with the declared structure, instead of silently leaving a hand-corrupted credential entry unrepaired forever - findAllClaws: only the actual ClawOperatorConfig singleton (name + operator namespace) triggers the cluster-wide Claw fan-out, not any same-named object in a tenant namespace or any object in the operator's namespace - Reconcile: propagate a failed status write in the gating-denial path so the Ready=False/ConfigModeNotAllowed condition is retried instead of silently dropped, while a successful denial still returns nil (no backoff) as before - claw_merge_test.go: explicitly clear CLAW_CONFIG_MODE for the "unset mode" test case instead of leaving cmd.Env nil, so it can't inherit an ambient value from the test process's own environment - ADR/design doc: fix overview text overstating MCP/plugin ownership under seedOnly, aligning it with the two-bucket model described later in each document Signed-off-by: Alexey Kazakov <alkazako@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ChangesSeed-only config policy
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
internal/assets/manifests/claw/configmap.yaml (1)
185-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate provider/channel reassertion logic.
The provider and channel loops in
reassertBucketAare structurally identical (declared-check, malformed-check, allowlisted-key overwrite) — could be consolidated into one helper parameterized by the allowlist/collection.♻️ Suggested consolidation
+function reassertBucketAEntries(baseMap, opsMap, keys) { + if (!baseMap) return; + for (const name of Object.keys(opsMap)) { + if (!(name in baseMap)) continue; + if (!isPlainObject(baseMap[name])) { + baseMap[name] = opsMap[name]; + continue; + } + for (const key of keys) { + if (opsMap[name][key] !== undefined) { + baseMap[name][key] = opsMap[name][key]; + } + } + } +} + function reassertBucketA(base, ops, mcpBucketAServers) { const opsProviders = getPath(ops, ["models", "providers"]) || {}; const baseProviders = getPath(base, ["models", "providers"]); - if (baseProviders) { - for (const name of Object.keys(opsProviders)) { - if (!(name in baseProviders)) continue; - if (!isPlainObject(baseProviders[name])) { - baseProviders[name] = opsProviders[name]; - continue; - } - for (const key of PROVIDER_BUCKET_A_KEYS) { - if (opsProviders[name][key] !== undefined) { - baseProviders[name][key] = opsProviders[name][key]; - } - } - } - } + reassertBucketAEntries(baseProviders, opsProviders, PROVIDER_BUCKET_A_KEYS); const opsChannels = ops.channels || {}; const baseChannels = base.channels; - if (baseChannels) { - for (const name of Object.keys(opsChannels)) { - if (!(name in baseChannels)) continue; - if (!isPlainObject(baseChannels[name])) { - baseChannels[name] = opsChannels[name]; - continue; - } - for (const key of CHANNEL_BUCKET_A_KEYS) { - if (opsChannels[name][key] !== undefined) { - baseChannels[name][key] = opsChannels[name][key]; - } - } - } - } + reassertBucketAEntries(baseChannels, opsChannels, CHANNEL_BUCKET_A_KEYS);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/assets/manifests/claw/configmap.yaml` around lines 185 - 230, The provider and channel reassertion blocks inside reassertBucketA duplicate the same declared-check, plain-object fallback, and allowlisted-key merge logic. Extract that repeated loop into a shared helper that takes the source/target collections and the relevant allowlist (PROVIDER_BUCKET_A_KEYS or CHANNEL_BUCKET_A_KEYS), then use it for both models.providers and channels while preserving the existing mcp.servers handling.api/v1alpha1/clawoperatorconfig_types.go (1)
40-50: 📐 Maintainability & Code Quality | 🔵 TrivialConsider enforcing the singleton name at the API level.
Right now, an admin creating a
ClawOperatorConfigwith a name other thanclusteris silently ignored by the controller (per the doc comment andfindAllClawstests) rather than rejected. A CEL validation rule can catch this at admission time instead.♻️ Suggested addition
+// +kubebuilder:validation:XValidation:rule="self.metadata.name == 'cluster'",message="ClawOperatorConfig must be named 'cluster'" type ClawOperatorConfig struct { metav1.TypeMeta `json:",inline"` metav1.ObjectMeta `json:"metadata,omitempty"`Please confirm
self.metadata.nameCEL validators are supported by the controller-gen/Kubernetes version this repo targets (CRD showscontroller-gen.kubebuilder.io/version: v0.19.0) before applying.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1alpha1/clawoperatorconfig_types.go` around lines 40 - 50, Add an admission-time singleton-name validation to ClawOperatorConfig so only the fixed name is accepted instead of silently ignoring other names. Update the ClawOperatorConfig type definition to include a CEL validation on metadata.name (or an equivalent kubebuilder marker) that enforces the "cluster" name, and verify controller-gen v0.19.0/Kubernetes support for self.metadata.name before landing it.internal/controller/claw_mcp_test.go (1)
233-242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the metadata type before comparing values.
This subtest directly type-asserts
_seedOnlyMeta; userequirelike the previous case so a regression fails clearly instead of panicking. As per coding guidelines, usetestify/requirefor fatal setup errors andtestify/assertfor value comparisons.Proposed test cleanup
- meta := config["_seedOnlyMeta"].(map[string]any) + meta, ok := config["_seedOnlyMeta"].(map[string]any) + require.True(t, ok, "_seedOnlyMeta should be set") assert.Empty(t, meta["mcpBucketAServers"])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/claw_mcp_test.go` around lines 233 - 242, Guard the `_seedOnlyMeta` lookup in the "should not include plain command servers in bucket-A list" subtest before doing the value check. In `injectMcpServers`-related test setup, replace the direct type assertion on `config["_seedOnlyMeta"]` with a fatal `require`-style check like the earlier subtest so a missing or malformed metadata map fails clearly instead of panicking, then keep using `assert.Empty` for the `mcpBucketAServers` comparison.Source: Coding guidelines
internal/controller/claw_operator_config_test.go (1)
59-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftFold the repeated cases into table-driven subtests.
These policy/defaulting scenarios are a natural matrix and currently duplicate namespace, config, reconciler, and assertion setup. As per coding guidelines, “Write tests as
Test*functions usingt.Run()subtests,t.Cleanup(), and table-driven cases.”Also applies to: 229-313, 315-331
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/claw_operator_config_test.go` around lines 59 - 226, TestClawOperatorConfigGating currently repeats the same namespace, operator config, reconciler, and assertion setup across multiple t.Run cases; refactor it into a table-driven subtest matrix using t.Run and t.Cleanup, with shared setup in the parent and per-case fields for allowlist, config mode, credentials/secret setup, and expected Ready-condition reason. Keep the existing scenario coverage in ClawOperatorConfigGating and the related cases that follow, but centralize the repeated creation/reconcile/assert logic so each subtest only declares its inputs and expected outcome.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0021-seed-only-config-mode.md`:
- Line 208: The decision-table row in the ADR markdown is malformed because it
only has two cells, which breaks the table layout. Update the row in the section
containing the ClawOperatorConfig behavior table so it includes the missing
first column value, or move that note outside the table entirely. Keep the table
structure consistent with the surrounding rows so the Markdown renders
correctly.
In `@docs/proposals/user-owned-config-design.md`:
- Around line 598-599: The test matrix table is flowing into the following
bullet because there is no separation after the table. Update the markdown
around the user-owned-config design section so the table in that area is
followed by a blank line before the next bullet list, keeping the bullet items
outside the table rendering.
In `@internal/assets/manifests/claw/configmap.yaml`:
- Around line 185-259: In reassertBucketA, the MCP server merge path is
vulnerable because it blindly assigns opsMcpServers entries into baseMcpServers,
allowing unsafe names like __proto__ to cause prototype pollution. Add a name
guard before the assignment so only safe own-property keys are merged, and skip
or reject any unsafe MCP server names. Keep the fix localized to reassertBucketA
and reuse the existing MCP server merge logic without changing the broader
gapFillBucketB behavior.
---
Nitpick comments:
In `@api/v1alpha1/clawoperatorconfig_types.go`:
- Around line 40-50: Add an admission-time singleton-name validation to
ClawOperatorConfig so only the fixed name is accepted instead of silently
ignoring other names. Update the ClawOperatorConfig type definition to include a
CEL validation on metadata.name (or an equivalent kubebuilder marker) that
enforces the "cluster" name, and verify controller-gen v0.19.0/Kubernetes
support for self.metadata.name before landing it.
In `@internal/assets/manifests/claw/configmap.yaml`:
- Around line 185-230: The provider and channel reassertion blocks inside
reassertBucketA duplicate the same declared-check, plain-object fallback, and
allowlisted-key merge logic. Extract that repeated loop into a shared helper
that takes the source/target collections and the relevant allowlist
(PROVIDER_BUCKET_A_KEYS or CHANNEL_BUCKET_A_KEYS), then use it for both
models.providers and channels while preserving the existing mcp.servers
handling.
In `@internal/controller/claw_mcp_test.go`:
- Around line 233-242: Guard the `_seedOnlyMeta` lookup in the "should not
include plain command servers in bucket-A list" subtest before doing the value
check. In `injectMcpServers`-related test setup, replace the direct type
assertion on `config["_seedOnlyMeta"]` with a fatal `require`-style check like
the earlier subtest so a missing or malformed metadata map fails clearly instead
of panicking, then keep using `assert.Empty` for the `mcpBucketAServers`
comparison.
In `@internal/controller/claw_operator_config_test.go`:
- Around line 59-226: TestClawOperatorConfigGating currently repeats the same
namespace, operator config, reconciler, and assertion setup across multiple
t.Run cases; refactor it into a table-driven subtest matrix using t.Run and
t.Cleanup, with shared setup in the parent and per-case fields for allowlist,
config mode, credentials/secret setup, and expected Ready-condition reason. Keep
the existing scenario coverage in ClawOperatorConfigGating and the related cases
that follow, but centralize the repeated creation/reconcile/assert logic so each
subtest only declares its inputs and expected outcome.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 12c95cac-b221-48fc-bb47-b6a7a4257aa6
📒 Files selected for processing (21)
CLAUDE.mdapi/v1alpha1/claw_types.goapi/v1alpha1/clawoperatorconfig_types.goapi/v1alpha1/zz_generated.deepcopy.gocmd/main.goconfig/crd/bases/claw.sandbox.redhat.com_clawoperatorconfigs.yamlconfig/crd/bases/claw.sandbox.redhat.com_claws.yamlconfig/crd/kustomization.yamlconfig/manager/manager.yamlconfig/rbac/role.yamldocs/adr/0021-seed-only-config-mode.mddocs/architecture.mddocs/proposals/user-owned-config-design.mddocs/proposals/user-owned-config-questions.mdinternal/assets/manifests/claw/configmap.yamlinternal/controller/claw_mcp.gointernal/controller/claw_mcp_test.gointernal/controller/claw_merge_test.gointernal/controller/claw_operator_config.gointernal/controller/claw_operator_config_test.gointernal/controller/claw_resource_controller.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
codeready-toolchain/api(manual)codeready-toolchain/toolchain-common(manual)codeready-toolchain/host-operator(manual)codeready-toolchain/toolchain-e2e(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: E2E Tests
- GitHub Check: Unit Tests
🧰 Additional context used
📓 Path-based instructions (6)
config/**/*.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
config/**/*.yaml: Enforce pod security settings in deployment manifests: run containers as non-root (uid 65532), use restricted seccomp, and drop all capabilities.
SetreadOnlyRootFilesystem: trueon the proxy andwait-for-proxycontainers, but not oninit-configor gateway containers.
Files:
config/crd/kustomization.yamlconfig/rbac/role.yamlconfig/crd/bases/claw.sandbox.redhat.com_claws.yamlconfig/manager/manager.yamlconfig/crd/bases/claw.sandbox.redhat.com_clawoperatorconfigs.yaml
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/crd/kustomization.yamlconfig/rbac/role.yamlcmd/main.goCLAUDE.mdinternal/controller/claw_operator_config.goconfig/crd/bases/claw.sandbox.redhat.com_claws.yamlinternal/controller/claw_mcp_test.goconfig/manager/manager.yamlconfig/crd/bases/claw.sandbox.redhat.com_clawoperatorconfigs.yamlapi/v1alpha1/clawoperatorconfig_types.godocs/architecture.mdinternal/controller/claw_mcp.godocs/adr/0021-seed-only-config-mode.mdinternal/controller/claw_operator_config_test.godocs/proposals/user-owned-config-design.mdapi/v1alpha1/zz_generated.deepcopy.gointernal/controller/claw_merge_test.gointernal/controller/claw_resource_controller.godocs/proposals/user-owned-config-questions.mdinternal/assets/manifests/claw/configmap.yamlapi/v1alpha1/claw_types.go
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Include the license header from
hack/boilerplate.go.txtin Go source files.
Files:
cmd/main.gointernal/controller/claw_operator_config.gointernal/controller/claw_mcp_test.goapi/v1alpha1/clawoperatorconfig_types.gointernal/controller/claw_mcp.gointernal/controller/claw_operator_config_test.goapi/v1alpha1/zz_generated.deepcopy.gointernal/controller/claw_merge_test.gointernal/controller/claw_resource_controller.goapi/v1alpha1/claw_types.go
internal/controller/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
internal/controller/**/*.go: Add// +kubebuilder:rbac:...markers on reconciler methods so RBAC is generated from them.
Set owner references on all created resources withcontrollerutil.SetControllerReference.
Files:
internal/controller/claw_operator_config.gointernal/controller/claw_mcp_test.gointernal/controller/claw_mcp.gointernal/controller/claw_operator_config_test.gointernal/controller/claw_merge_test.gointernal/controller/claw_resource_controller.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Usetestify/requirefor fatal setup errors andtestify/assertfor value comparisons.
Write tests asTest*functions usingt.Run()subtests,t.Cleanup(), and table-driven cases.
Use thewaitFor(t, timeout, interval, condition, message)helper for async polling; its standard timeout is 10s with a 250ms interval.
Keep test files separated by resource type (for example,claw_configmap_test.goorclaw_credentials_test.go).
Files:
internal/controller/claw_mcp_test.gointernal/controller/claw_operator_config_test.gointernal/controller/claw_merge_test.go
api/v1alpha1/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
After modifying API types in
api/v1alpha1/, runmake manifeststo regenerate CRD YAML inconfig/crd/bases/andmake generateto regeneratezz_generated.deepcopy.go.
Files:
api/v1alpha1/clawoperatorconfig_types.goapi/v1alpha1/zz_generated.deepcopy.goapi/v1alpha1/claw_types.go
🧠 Learnings (1)
📚 Learning: 2026-05-27T15:29:16.197Z
Learnt from: alexeykazakov
Repo: codeready-toolchain/claw-operator PR: 149
File: internal/assets/manifests/claw/kustomization.yaml:19-19
Timestamp: 2026-05-27T15:29:16.197Z
Learning: In this repo’s claw Kustomize YAML manifests, when an image tag for ghcr.io (specifically ghcr.io/openclaw/openclaw) is being verified via `crane manifest` in an unauthenticated environment, a `not found` result can be caused by missing registry authentication (false positives). Do not flag the tag as missing based solely on that `crane manifest` output; require an authenticated verification step (e.g., GHCR login/token) or another independent check before concluding the tag is absent.
Applied to files:
internal/assets/manifests/claw/configmap.yaml
🪛 LanguageTool
docs/adr/0021-seed-only-config-mode.md
[style] ~123-~123: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...ield unprotected while over-reasserting silently clobbers a user customization, and neit...
(ADVERB_REPETITION_PREMIUM)
docs/proposals/user-owned-config-design.md
[style] ~150-~150: This phrase is redundant. Consider using “outside”.
Context: ...nstall plugins a user installed by hand outside of spec.plugins. ### Bucket B — User-ma...
(OUTSIDE_OF)
[style] ~196-~196: Consider an alternative for the overused word “exactly”.
Context: ...-seeded seedOnly instance, > which is exactly the scenario least likely to get manual...
(EXACTLY_PRECISELY)
[style] ~229-~229: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ... left completely untouched. This is the exact same gap-fill principle injectModelCatalog...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
[style] ~436-~436: This sentence may be long and difficult for your reader to follow. Consider inserting a period and starting a new sentence here.
Context: ...e:** Namespaced (like ToolchainConfig), with the singleton instance required to live in the operator's **own runtime...
(WITH_THE_SENTENCE)
[style] ~594-~594: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...o providers), only one hand-corrupted | Only the corrupted one changes; no cross-con...
(ADVERB_REPETITION_PREMIUM)
docs/proposals/user-owned-config-questions.md
[style] ~97-~97: To make your writing clearer, consider a shorter, more direct phrase.
Context: ...hronous rejection was already ruled out as a consequence of Q1, not a live alternative here. A mor...
(AS_A_CONSEQUENCE_OF)
[style] ~227-~227: Consider an alternative for the overused word “exactly”.
Context: ...-enforced even in seedOnly: these are exactly the keys the issue itself calls "operat...
(EXACTLY_PRECISELY)
[style] ~361-~361: ‘exactly the same’ might be wordy. Consider a shorter alternative.
Context: ...ouched, whatever their content. This is exactly the same gap-fill principle injectModelCatalog...
(EN_WORDINESS_PREMIUM_EXACTLY_THE_SAME)
[style] ~380-~380: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...model under seedOnly, even though the exact same scenario already self-heals under `merg...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
🪛 markdownlint-cli2 (0.22.1)
docs/adr/0021-seed-only-config-mode.md
[warning] 208-208: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
docs/proposals/user-owned-config-design.md
[warning] 598-598: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🔀 Multi-repo context codeready-toolchain/toolchain-common, codeready-toolchain/toolchain-e2e
Linked repositories findings
codeready-toolchain/toolchain-common
[::codeready-toolchain/toolchain-common::] pkg/configuration/load.go:18-21,70 already treats WATCH_NAMESPACE as required for loading operator config/secrets (GetWatchNamespace() errors if unset), so the new operator startup requirement to read WATCH_NAMESPACE is aligned with existing helper expectations.
[::codeready-toolchain/toolchain-common::] pkg/test/config/toolchainconfig.go:744-764 falls back to test.HostOperatorNs when WATCH_NAMESPACE is absent in tests. If test environments now rely on the operator’s namespace being explicitly set, these helpers may hide missing-env issues.
codeready-toolchain/toolchain-e2e
[::codeready-toolchain/toolchain-e2e::] pkg/configuration/load.go:18-21,70 has the same WATCH_NAMESPACE contract as toolchain-common, so the deployment change is consistent with how e2e config is already resolved.
[::codeready-toolchain/toolchain-e2e::] pkg/test/config/toolchainconfig.go:744-764 also defaults to test.HostOperatorNs when WATCH_NAMESPACE is unset, which means e2e tests won’t surface the operator’s new hard fail on missing WATCH_NAMESPACE unless they run against the real deployment env.
codeready-toolchain/api
No direct matches for seedOnly, ClawOperatorConfig, ConfigModeNotAllowed, mergeMode, or WATCH_NAMESPACE were found.
🔇 Additional comments (14)
api/v1alpha1/zz_generated.deepcopy.go (1)
225-301: LGTM!config/crd/bases/claw.sandbox.redhat.com_clawoperatorconfigs.yaml (1)
1-71: LGTM!config/crd/bases/claw.sandbox.redhat.com_claws.yaml (1)
106-125: LGTM!config/crd/kustomization.yaml (1)
7-7: LGTM!config/rbac/role.yaml (1)
84-92: LGTM!internal/assets/manifests/claw/configmap.yaml (2)
41-166: LGTM!
280-357: LGTM!Also applies to: 1397-1407
internal/controller/claw_merge_test.go (1)
25-41: LGTM!Also applies to: 134-141, 492-911
api/v1alpha1/claw_types.go (1)
40-58: LGTM!Also applies to: 98-106, 379-389
cmd/main.go (1)
265-286: LGTM!config/manager/manager.yaml (1)
70-73: LGTM!internal/controller/claw_operator_config.go (1)
1-80: LGTM!internal/controller/claw_resource_controller.go (1)
405-408: LGTM!Also applies to: 419-419, 457-479, 1611-1649
internal/controller/claw_mcp.go (1)
65-99: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/claw_gitsync_script_exec_test.go`:
- Around line 263-289: runGitSyncScriptAllowFailure should preserve the same
generated-script anchor sanity checks as runGitSyncScript before doing any
substitutions. Add the hardcoded path assertions around generateGitSyncScript
output and keep the existing replacement logic, so failures in the anchors for
/etc/proxy-ca/ca.crt, /tmp/combined-ca.crt, and /git-sources/ are caught early
even in negative-path tests. This keeps the failure-tolerant helper aligned with
runGitSyncScript and prevents silent no-op substitutions from masking
regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5b8385b6-a030-40ff-831c-cc35a5df927d
📒 Files selected for processing (4)
internal/controller/claw_gitsync_script_exec_test.gointernal/controller/claw_plugins_script_exec_test.gointernal/controller/claw_seed_script_test.gotest/e2e/e2e_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
codeready-toolchain/api(manual)codeready-toolchain/toolchain-common(manual)codeready-toolchain/host-operator(manual)codeready-toolchain/toolchain-e2e(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: E2E Tests
- GitHub Check: Unit Tests
🧰 Additional context used
📓 Path-based instructions (5)
internal/controller/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
internal/controller/**/*.go: Add// +kubebuilder:rbac:...markers on reconciler methods so RBAC is generated from controller code.
Set owner references on all created resources usingcontrollerutil.SetControllerReference.
Ensure the Route host is resolved before injecting it into the gateway ConfigMap for CORS during the three-phase reconciliation flow.
Use theClawOperatorConfigsingleton to gateClawmergeModevalues according toallowedConfigModes.
Files:
internal/controller/claw_seed_script_test.gointernal/controller/claw_gitsync_script_exec_test.gointernal/controller/claw_plugins_script_exec_test.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Write unit tests withtestify/requirefor fatal setup errors andtestify/assertfor value comparisons, usingenvtestrather than a full cluster.
Name testsTest*, uset.Run()subtests,t.Cleanup(), and prefer table-driven tests.
Use thewaitFor(t, timeout, interval, condition, message)helper for async assertions (10s timeout, 250ms poll).
Keep tests separated by resource type (for example,claw_configmap_test.goandclaw_credentials_test.go).
Files:
internal/controller/claw_seed_script_test.gointernal/controller/claw_gitsync_script_exec_test.gointernal/controller/claw_plugins_script_exec_test.gotest/e2e/e2e_test.go
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Required license header must use the template in
hack/boilerplate.go.txt.
Files:
internal/controller/claw_seed_script_test.gointernal/controller/claw_gitsync_script_exec_test.gointernal/controller/claw_plugins_script_exec_test.gotest/e2e/e2e_test.go
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
internal/controller/claw_seed_script_test.gointernal/controller/claw_gitsync_script_exec_test.gointernal/controller/claw_plugins_script_exec_test.gotest/e2e/e2e_test.go
test/e2e/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Place end-to-end tests under
test/e2e/, and run them against a Kind cluster.
Files:
test/e2e/e2e_test.go
🪛 ast-grep (0.44.1)
internal/controller/claw_seed_script_test.go
[error] 81-81: A shell (sh/bash) is invoked with -c and a dynamically built command string (string concatenation, fmt.Sprintf, or a variable holding the command) passed to exec.Command / exec.CommandContext. Untrusted input embedded in the command lets an attacker inject arbitrary shell commands. Avoid the shell: call the target binary directly with exec.Command(name, arg1, arg2, ...) so each argument is passed as a separate, non-interpreted token, and never build a shell command string from external input.
Context: exec.Command("sh", "-c", script)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-sh-c-go)
internal/controller/claw_gitsync_script_exec_test.go
[error] 126-126: A shell (sh/bash) is invoked with -c and a dynamically built command string (string concatenation, fmt.Sprintf, or a variable holding the command) passed to exec.Command / exec.CommandContext. Untrusted input embedded in the command lets an attacker inject arbitrary shell commands. Avoid the shell: call the target binary directly with exec.Command(name, arg1, arg2, ...) so each argument is passed as a separate, non-interpreted token, and never build a shell command string from external input.
Context: exec.Command("sh", "-c", script)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-sh-c-go)
[error] 234-234: A shell (sh/bash) is invoked with -c and a dynamically built command string (string concatenation, fmt.Sprintf, or a variable holding the command) passed to exec.Command / exec.CommandContext. Untrusted input embedded in the command lets an attacker inject arbitrary shell commands. Avoid the shell: call the target binary directly with exec.Command(name, arg1, arg2, ...) so each argument is passed as a separate, non-interpreted token, and never build a shell command string from external input.
Context: exec.Command("sh", "-c", setupScript)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-sh-c-go)
[error] 280-280: A shell (sh/bash) is invoked with -c and a dynamically built command string (string concatenation, fmt.Sprintf, or a variable holding the command) passed to exec.Command / exec.CommandContext. Untrusted input embedded in the command lets an attacker inject arbitrary shell commands. Avoid the shell: call the target binary directly with exec.Command(name, arg1, arg2, ...) so each argument is passed as a separate, non-interpreted token, and never build a shell command string from external input.
Context: exec.Command("sh", "-c", script)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-sh-c-go)
internal/controller/claw_plugins_script_exec_test.go
[error] 130-130: A shell (sh/bash) is invoked with -c and a dynamically built command string (string concatenation, fmt.Sprintf, or a variable holding the command) passed to exec.Command / exec.CommandContext. Untrusted input embedded in the command lets an attacker inject arbitrary shell commands. Avoid the shell: call the target binary directly with exec.Command(name, arg1, arg2, ...) so each argument is passed as a separate, non-interpreted token, and never build a shell command string from external input.
Context: exec.Command("sh", "-c", script)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-sh-c-go)
[error] 218-218: A shell (sh/bash) is invoked with -c and a dynamically built command string (string concatenation, fmt.Sprintf, or a variable holding the command) passed to exec.Command / exec.CommandContext. Untrusted input embedded in the command lets an attacker inject arbitrary shell commands. Avoid the shell: call the target binary directly with exec.Command(name, arg1, arg2, ...) so each argument is passed as a separate, non-interpreted token, and never build a shell command string from external input.
Context: exec.Command("sh", "-c", script)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-sh-c-go)
[error] 257-257: A shell (sh/bash) is invoked with -c and a dynamically built command string (string concatenation, fmt.Sprintf, or a variable holding the command) passed to exec.Command / exec.CommandContext. Untrusted input embedded in the command lets an attacker inject arbitrary shell commands. Avoid the shell: call the target binary directly with exec.Command(name, arg1, arg2, ...) so each argument is passed as a separate, non-interpreted token, and never build a shell command string from external input.
Context: exec.Command("sh", "-c", script)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-sh-c-go)
🪛 OpenGrep (1.23.0)
internal/controller/claw_seed_script_test.go
[ERROR] 82-82: Dynamic command passed to exec.Command with a shell invocation. Pass arguments directly to exec.Command without a shell wrapper.
(coderabbit.command-injection.go-exec-command)
internal/controller/claw_gitsync_script_exec_test.go
[ERROR] 127-127: Dynamic command passed to exec.Command with a shell invocation. Pass arguments directly to exec.Command without a shell wrapper.
(coderabbit.command-injection.go-exec-command)
[ERROR] 235-235: Dynamic command passed to exec.Command with a shell invocation. Pass arguments directly to exec.Command without a shell wrapper.
(coderabbit.command-injection.go-exec-command)
[ERROR] 281-281: Dynamic command passed to exec.Command with a shell invocation. Pass arguments directly to exec.Command without a shell wrapper.
(coderabbit.command-injection.go-exec-command)
internal/controller/claw_plugins_script_exec_test.go
[ERROR] 131-131: Dynamic command passed to exec.Command with a shell invocation. Pass arguments directly to exec.Command without a shell wrapper.
(coderabbit.command-injection.go-exec-command)
[ERROR] 219-219: Dynamic command passed to exec.Command with a shell invocation. Pass arguments directly to exec.Command without a shell wrapper.
(coderabbit.command-injection.go-exec-command)
[ERROR] 258-258: Dynamic command passed to exec.Command with a shell invocation. Pass arguments directly to exec.Command without a shell wrapper.
(coderabbit.command-injection.go-exec-command)
🔀 Multi-repo context codeready-toolchain/toolchain-common, codeready-toolchain/host-operator, codeready-toolchain/api, codeready-toolchain/toolchain-e2e
Linked repositories findings
codeready-toolchain/toolchain-common [::codeready-toolchain/toolchain-common::]
pkg/configuration/load.go:18-21,70already definesWATCH_NAMESPACEas a required env var and errors if it is missing.pkg/configuration/load_test.go:194,215,369,382andpkg/configuration/cache_test.go:27-37assert the same fail-fast behavior (WATCH_NAMESPACE must be set/ must not be empty).
codeready-toolchain/host-operator [::codeready-toolchain/host-operator::]
config/manager/manager.yaml:82already injectsWATCH_NAMESPACEfrom the pod namespace.- Multiple controller tests explicitly set/unset
WATCH_NAMESPACE(for examplecontrollers/usersignup/mapper_test.go:85,controllers/spacebindingcleanup/spacebinding_cleanup_controller_test.go:110,406), so namespace-scoped operator behavior is already an established contract.
codeready-toolchain/api [::codeready-toolchain/api::]
- No matches for
ClawOperatorConfig,seedOnly,ConfigModeSeedOnly,ConfigModeNotAllowed, orWATCH_NAMESPACE.
codeready-toolchain/toolchain-e2e [::codeready-toolchain/toolchain-e2e::]
- No matches for
ClawOperatorConfig,seedOnly,ConfigModeSeedOnly,ConfigModeNotAllowed, orWATCH_NAMESPACE.
🔇 Additional comments (7)
internal/controller/claw_plugins_script_exec_test.go (2)
1-16: Same license-header verification concern already raised forinternal/controller/claw_gitsync_script_exec_test.go.
46-269: LGTM!internal/controller/claw_seed_script_test.go (2)
1-16: Same license-header verification concern already raised forinternal/controller/claw_gitsync_script_exec_test.go.
47-222: LGTM!test/e2e/e2e_test.go (1)
685-726: LGTM!Also applies to: 1472-1488, 2137-2146
internal/controller/claw_gitsync_script_exec_test.go (2)
163-174: 🎯 Functional CorrectnessVerify SHA-pinning fetch doesn't depend on server-side git config.
git fetch --depth 1 <url> <SHA>for a non-tip commit generally requiresuploadpack.allowReachableSHA1InWant(orallowTipSHA1InWant/allowAnySHA1InWant) to be enabled on the source repo for the smart git protocol; the fixture repo here doesn't configure this. Whether the local/file://transport bypasses this restriction is git-version dependent.Please confirm this test is stable across the git versions used in CI (not just the sandbox), e.g. by searching for current git documentation on
uploadpack.allowReachableSHA1InWantbehavior for local transports.
1-16: License header matches the boilerplate template.> Likely an incorrect or invalid review comment.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #226 +/- ##
==========================================
+ Coverage 79.33% 79.66% +0.33%
==========================================
Files 33 34 +1
Lines 4742 4878 +136
==========================================
+ Hits 3762 3886 +124
- Misses 622 631 +9
- Partials 358 361 +3
🚀 New features to boost your workflow:
|
2 participants
Addresses #224: some deployments need the operator to keep reconciling
openclaw.json forever, others need the user/agent to own it after initial
provisioning. Adds an opt-in
seedOnlymergeMode plus cluster-admingating, per docs/adr/0021-seed-only-config-mode.md.
seedOnlyvalue forspec.config.mergeMode: seeds openclaw.jsononce on first boot, then treats the PVC file as user-owned.
wiring, proxy/security paths) is reasserted at the field level on every
restart; Bucket-B (everything else a user/agent can touch) is only
gap-filled for newly CR-declared entries, never overwritten in place.
ClawOperatorConfig(name "cluster", inthe operator's own namespace) lets cluster admins restrict which
mergeMode values are allowed; gating fails open (unrestricted) when the
singleton is absent, so existing clusters are unaffected by default.
Ready: False/ConfigModeNotAllowedwithout erroring, since it's a stable policy mismatch rather than a
transient failure (avoids exponential-backoff requeue).
injectMcpServersadds a private_seedOnlyMetamarker so merge.js cantell which MCP server entries carry credentials/URLs and need full
reassertion, since that can't be inferred from JSON shape alone; the
marker is stripped before writing to the PVC in every mode.
reassertion (vs. whole-entry) was required so that, e.g., a user's
custom models list under a provider isn't clobbered just because its
baseUrl/apiKey must stay operator-owned.
Summary by CodeRabbit
New Features
seedOnlymerge mode forClawconfiguration (seed once, then preserve user changes while enforcing an operator-managed subset).ClawOperatorConfig(namespaced singletoncluster) withallowedConfigModesto restrict permittedmergeModevalues, applied live.WATCH_NAMESPACEautomatically for policy scoping and validates the singleton at admission time.Bug Fixes
Clawis configured with a disallowed mode, reconciliation halts and the operator idles/scale-downs with a clear status reason.Documentation / Tests