# Deep Review: 20260401-120241-pr-269 | | | |---|---| | **Date** | 2026-04-01 12:02 | | **Repo** | [rancher-sandbox/rancher-desktop-daemon](https://github.com/rancher-sandbox/rancher-desktop-daemon) | | **Round** | 2 | | **Author** | [@Nino-K](https://github.com/Nino-K) | | **PR** | [#269](https://github.com/rancher-sandbox/rancher-desktop-daemon/pull/269) — Add containerEngine and kubernetes fields to the App resource | | **Branch** | `implement-k8s-moby-app-resource` | | **Commits** | `d35ce52` Update documents to include specs for Kubernetes and container engine
`aa14a63` Add containerEngine and kubernetes spec fields | | **Reviewers** | Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro | | **Verdict** | **Merge with fixes** — fix the reconcile ordering bug and the nil-map panic; add unit tests for the template helpers | | **Wall-clock time** | `32 min 37 s` | --- ## Executive Summary This PR adds `spec.containerEngine.name` and `spec.kubernetes.{enabled,version}` to the App resource. The App controller injects these as Lima template params at creation and propagates changes to the LimaVM's template ConfigMap at runtime. The approach is sound: the template ConfigMap is the correct integration surface until `LimaVM.Spec.Params` exists. Two bugs need fixing before merge: the template-update early return skips Running-state propagation (causing a spurious VM restart when both change in one update), and empty template data causes a nil-map panic. --- ## Critical Issues None. --- ## Important Issues 1. **Early return from template update skips Running-state propagation** — `pkg/controllers/app/app/controllers/app_controller.go:250` (important, regression) [Codex GPT 5.4, Gemini 3.1 Pro] ```go templateCM.Data[limav1alpha1.TemplateConfigMapKey] = updatedTemplate if err := r.Update(ctx, templateCM); err != nil { return ctrl.Result{}, fmt.Errorf("failed to update template ConfigMap with new params: %w", err) } // ... return ctrl.Result{}, nil } } } // Propagate spec.running from App into the LimaVM. if limaVM.Spec.Running != app.Spec.Running { ``` When a single App update changes both template params and `spec.running`, the template update at line 243 returns early at line 250, skipping the Running propagation at line 256. The LimaVM controller sees the template change while `LimaVM.Spec.Running` is still `true`, triggers a restart, and only receives the stop request on the next App reconcile. A user who sets `spec.running=false` and changes `spec.containerEngine.name` in one update gets a restart followed by a stop, instead of just a stop. Fix: move the Running propagation block (lines 255-262) above the template propagation block (lines 219-253), so Running state is always delivered first. ```diff + // Propagate spec.running from App into the LimaVM. + if limaVM.Spec.Running != app.Spec.Running { + limaVM.Spec.Running = app.Spec.Running + if err := r.Update(ctx, limaVM); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to propagate running state to LimaVM: %w", err) + } + return ctrl.Result{}, nil + } + // Propagate app spec.containerEngine and spec.kubernetes into the LimaVM's // template ConfigMap. if limaVM.Status.TemplateConfigMap != "" { // ... } - // Propagate spec.running from App into the LimaVM. - if limaVM.Spec.Running != app.Spec.Running { - // ... - } ``` 2. **Nil-map panic in `applySpecToTemplate` on empty template data** — `pkg/controllers/app/app/controllers/app_controller.go:65` (important, regression) [Claude Opus 4.6, Gemini 3.1 Pro] ```go func applySpecToTemplate(templateData string, spec v1alpha1.AppSpec) (string, error) { tmpl, params, err := parseTemplateParams(templateData) // ... params["CONTAINER_ENGINE"] = spec.ContainerEngine.Name // safe: params always non-nil // ... tmpl["param"] = params // PANIC if tmpl is nil ``` When `templateData` is empty or null YAML, `yaml.Unmarshal` succeeds but leaves `tmpl` as `nil`. Reading from a nil map is safe in Go, so `parseTemplateParams` works, but `tmpl["param"] = params` at line 65 panics. The runtime path at line 235 reaches this code if the template ConfigMap exists but its `template` key is missing or empty. Fix: guard against nil `tmpl` in `parseTemplateParams`: ```diff func parseTemplateParams(templateData string) (tmpl, params map[string]any, err error) { if err := yaml.Unmarshal([]byte(templateData), &tmpl); err != nil { return nil, nil, fmt.Errorf("failed to parse Lima template: %w", err) } + if tmpl == nil { + tmpl = make(map[string]any) + } params, _ = tmpl["param"].(map[string]any) ``` 3. **No unit tests for template helper functions** — `pkg/controllers/app/app/controllers/` (important, gap) [Claude Opus 4.6] `parseTemplateParams`, `applySpecToTemplate`, and `templateParamsMatch` are pure functions with no external dependencies — ideal candidates for table-driven tests. The entire `pkg/controllers/app/app/controllers/` directory has no test files. Key scenarios to cover: 1. Round-trip: apply params, then verify `templateParamsMatch` returns `true` 2. Param change detection: modify one param, verify `templateParamsMatch` returns `false` 3. Existing params preserved: applying spec params does not drop unrelated params already in the template 4. Empty/nil input: the nil-map fix (finding 2) handles empty template data 5. YAML type fidelity: `KUBERNETES_ENABLED` round-trips as `bool`, not `string` Fix: add `app_controller_test.go` with table-driven tests. --- ## Suggestions 1. **Type mismatch risk in `templateParamsMatch` interface equality** — `pkg/controllers/app/app/controllers/app_controller.go:80-82` (suggestion, regression) [Gemini 3.1 Pro] ```go return params["CONTAINER_ENGINE"] == spec.ContainerEngine.Name && params["KUBERNETES_ENABLED"] == spec.Kubernetes.Enabled && params["KUBERNETES_VERSION"] == spec.Kubernetes.Version, nil ``` The `params` map comes from `yaml.Unmarshal`, which infers Go types from YAML syntax. If someone manually edits the template ConfigMap and writes `KUBERNETES_VERSION: 1.30` (no quotes, one dot), YAML parses it as `float64(1.3)`, and the `==` comparison with the `string` spec field returns `false` on every reconcile — an infinite update loop. In normal operation, the controller writes the ConfigMap, so types stay consistent. Manual editing is the only trigger, and this is a single-user embedded system. Fix: convert both sides to strings via `fmt.Sprintf("%v", ...)` before comparing. 2. **Hardcoded Kubernetes version in multiple files** — `pkg/apis/app/v1alpha1/app_types.go:28-29`, `pkg/controllers/app/app/lima-template.yaml:34` (suggestion, regression) [Claude Opus 4.6] The string `"1.30.2"` appears as a kubebuilder default in `app_types.go` and as the param default in `lima-template.yaml`. Changing the default requires updating both files in lockstep. A constant in the types package would prevent drift. The kubebuilder annotation still needs a literal string, but a comment referencing the constant would help. --- ## Design Observations ### Concerns - **(future)** **`LimaVM.Spec.Params` would eliminate the ordering bug** — The LimaVM design doc describes a `spec.params` field that merges params into the template before validation. This field is designed but not yet implemented in the types. Once it exists, the App controller can set both `spec.running` and `spec.params` on the LimaVM in one update, and the LimaVM controller handles template materialization — eliminating the multi-resource ordering problem in finding 1. [Codex GPT 5.4, Gemini 3.1 Pro] - **(future)** **YAML key reordering on round-trip** — The template helpers round-trip YAML through `sigs.k8s.io/yaml`, which reorders keys alphabetically. The first write reorders the embedded template; subsequent writes are stable. If a future caller passes hand-authored YAML, the cosmetic reordering could produce a spurious template "change" detected by the LimaVM controller. [Claude Opus 4.6] ### Strengths - The create-path injects params into the input ConfigMap before creating the LimaVM, so first boot and runtime updates use the same template representation. [Claude Opus 4.6, Codex GPT 5.4] - The `templateParamsMatch` check before updating avoids spurious ConfigMap writes that would trigger unnecessary LimaVM reconciles and restarts. [Claude Opus 4.6] - The priority chain (template update, running propagation, condition mirroring) is well-ordered aside from the ordering bug; each step returns early so the App's `resourceVersion` stays current. [Claude Opus 4.6] --- ## Testing Assessment The existing BATS tests in `bats/tests/32-app-controllers/app.bats` cover the App lifecycle but do not exercise `containerEngine` or `kubernetes` fields — they rely on defaults. Untested scenarios, ranked by risk: 1. **Concurrent template and running change**: update both `spec.containerEngine.name` and `spec.running=false` in one operation; verify the VM stops without an intermediate restart 2. **Runtime param propagation**: change `spec.containerEngine.name` on a running App; verify the template ConfigMap updates and the LimaVM detects the change 3. **Kubernetes enabled toggle**: change `spec.kubernetes.enabled` from `false` to `true`; verify the template param changes 4. **Initial creation with non-default values**: create an App with `containerEngine.name: containerd`; verify the input ConfigMap has correct params 5. **Unit tests for pure functions**: `parseTemplateParams`, `applySpecToTemplate`, `templateParamsMatch` (see finding 3) --- ## Documentation Assessment The design doc (`docs/design/api_app.md`) documents all three new fields with clear descriptions and a consistent example. The removed status fields (`containerEngine`, `kubernetes`, `progress`, `version`, `onlineStatus`) reflect the shift to a conditions-only status, matching the implementation. One tension: `api_app.md` line 86 documents direct App-to-template-ConfigMap mutation as intended behavior. The LimaVM design doc (`api_lima.md` line 62) permits external modification of the template ConfigMap, so this is not a violation of the LimaVM contract. However, the design doc should note that this pattern is interim — once `LimaVM.Spec.Params` is implemented, the App controller should use it instead. --- ## Commit Structure Two cleanly separated commits: 1. `d35ce52`: design doc updates (spec-first, reasonable for a planned feature) 2. `aa14a63`: implementation (types, controller, CRD, template) Each commit is self-contained, and the messages describe their changes accurately. --- ## Agent Performance Retro ### [Claude] Claude produced the most detailed review with four findings across two severity levels. It correctly identified the nil-map panic (finding 2) and was the only agent to flag the missing unit tests, the hardcoded version duplication, and the CRD YAML quoting concern. It did not identify the reconcile ordering bug — the highest-impact finding in this review. Its design observations (YAML key reordering, priority chain analysis) were accurate and well-supported. ### [Codex] Codex produced the most architecturally focused review. It identified the reconcile ordering bug (finding 1) with a precise trace through the LimaVM controller's restart path, and raised the ownership concern with concrete impact analysis. It did not identify the nil-map panic or the missing unit tests. Codex framed the ownership concern as IMPORTANT rather than CRITICAL, correctly calibrating severity for this repo's context (the LimaVM design doc permits external ConfigMap modification). ### [Gemini] Gemini produced the broadest coverage, identifying both the nil-map panic and the ordering bug. It uniquely identified the type-mismatch risk in `templateParamsMatch`. However, it inflated severity: it rated the nil-map panic as CRITICAL (requires corrupt/empty ConfigMap data — unlikely in normal operation) and rated the ownership concern as CRITICAL despite the LimaVM design doc explicitly permitting external ConfigMap modification. Its CRITICAL classification of the ownership issue contradicts the repo context, which advises calibrating severity for the single-user embedded architecture. ### Summary | Metric | Claude Opus 4.6 | Codex GPT 5.4 | Gemini 3.1 Pro | |--------|-----------------|----------------|----------------| | Duration | 12:57 | 5:02 | 4:02 | | Critical | 0 | 0 | 2 (both downgraded) | | Important | 2 | 2 | 2 | | Suggestion | 2 | 0 | 1 | | Design observations | 4 | 2 | 2 | | False positives | 0 | 0 | 0 | | Unique insights | 2 (unit tests, version duplication) | 0 | 1 (type mismatch) | | Files reviewed | 6 | 6 | 6 | | Coverage misses | 0 | 0 | 0 | Codex provided the highest-value individual finding (the ordering bug with full cross-controller trace). Claude provided the best breadth and the only actionable gap finding (missing tests). Gemini caught the most total issues but needed severity recalibration. All three agents reviewed all changed files. --- ## Appendix: Original Reviews ### Claude Opus 4.6 Here is the complete review. --- ## PR #269: Add containerEngine and kubernetes fields to the App resource **Author:** @Nino-K | **Verdict:** Approve with minor fixes ### Executive Summary This PR adds `spec.containerEngine.name` and `spec.kubernetes.{enabled,version}` to the App resource, with the App controller injecting these as Lima template params at creation and propagating changes to the LimaVM's template ConfigMap at runtime. The approach is sound — the template ConfigMap is the correct integration point since `LimaVM.Spec.Params` does not exist yet. There is one correctness issue (nil map panic on edge input) and the new pure functions lack unit tests. --- ### Findings #### Critical Issues None. #### Important Issues 1. **Nil map write panic in `applySpecToTemplate` when template data is empty** — `pkg/controllers/app/app/controllers/app_controller.go:43-65` (regression) ```go func parseTemplateParams(templateData string) (tmpl, params map[string]any, err error) { if err := yaml.Unmarshal([]byte(templateData), &tmpl); err != nil { return nil, nil, fmt.Errorf("failed to parse Lima template: %w", err) } params, _ = tmpl["param"].(map[string]any) // ... } func applySpecToTemplate(templateData string, spec v1alpha1.AppSpec) (string, error) { tmpl, params, err := parseTemplateParams(templateData) // ... tmpl["param"] = params // PANIC: assignment to nil map } ``` If `templateData` is empty (or a YAML document that doesn't parse to a map, e.g. `"---"`), `yaml.Unmarshal` succeeds but leaves `tmpl` as `nil`. Reading from a nil map is safe in Go (line 48 returns the zero value), but writing to one panics (line 65). This is reachable via the reconcile path at line 229 if the template ConfigMap exists but has an empty `"template"` key. While unlikely in normal operation (the LimaVM webhook validates templates), a manual edit or corruption would cause a controller panic with no recovery. Fix: ```diff func parseTemplateParams(templateData string) (tmpl, params map[string]any, err error) { if err := yaml.Unmarshal([]byte(templateData), &tmpl); err != nil { return nil, nil, fmt.Errorf("failed to parse Lima template: %w", err) } + if tmpl == nil { + tmpl = make(map[string]any) + } params, _ = tmpl["param"].(map[string]any) ``` 2. **No unit tests for the new template functions** — `pkg/controllers/app/app/controllers/` (gap) `parseTemplateParams`, `applySpecToTemplate`, and `templateParamsMatch` are pure functions with no external dependencies — ideal candidates for table-driven unit tests. Key scenarios to cover: - Round-trip: apply params then verify `templateParamsMatch` returns `true` - Param change detection: modify one param and verify `templateParamsMatch` returns `false` - Existing params preserved: verify that applying spec params doesn't drop unrelated params already in the template - Empty/nil input: verify the nil-map fix (finding 1) works correctly - YAML type fidelity: `KUBERNETES_ENABLED` is a YAML boolean and must compare correctly as Go `bool` (not `string("false")`) There are no unit tests anywhere under `pkg/controllers/app/` currently. Fix: Add an `app_controller_test.go` file with table-driven tests for the three functions. #### Suggestions 1. **CRD version default should be quoted** — `pkg/controllers/app/app/crd.yaml:69,79` (regression) ```yaml kubernetes: default: enabled: false version: 1.30.2 ``` The unquoted `1.30.2` works because YAML parses it as a string (two dots make it an invalid float). But it's fragile — a future version like `1.30` (one dot, valid float) would silently become `float64(1.3)`. The Go source uses `+kubebuilder:default="1.30.2"` with quotes, but controller-gen strips them in the generated CRD. Adding quotes explicitly in the CRD (`version: "1.30.2"`) after generation (or using the quoted form in the kubebuilder annotation) makes the intent unambiguous. Since the CRD is generated (`Generated by generate-crds.sh; DO NOT EDIT`), this may need a controller-gen version bump or a post-processing step to handle correctly for future versions. 2. **Hardcoded default Kubernetes version in multiple files** — `pkg/apis/app/v1alpha1/app_types.go:28-29`, `pkg/controllers/app/app/lima-template.yaml:34` (regression) The string `"1.30.2"` appears as a kubebuilder annotation default in `app_types.go` (lines 28-29) and as the param default in `lima-template.yaml` (line 34). Changing the default version requires updating both files in lockstep. A constant in the types package referenced from both locations would prevent drift. Fix: Define a constant like `DefaultKubernetesVersion = "1.30.2"` in `app_types.go` and reference it in the template embedding code. The kubebuilder annotation still needs the literal string, but a comment pointing to the constant would help. --- ### Design Observations **Strengths** - The choice to modify the LimaVM's template ConfigMap directly is the right approach. The LimaVM design doc explicitly describes the template ConfigMap as a mutable integration surface, and `LimaVM.Spec.Params` doesn't exist yet as an API field. This avoids inventing a new mechanism. - The priority chain in the reconciler (template update → running propagation → condition mirroring) is well-ordered. Template changes take priority over running-state changes, which prevents a scenario where the VM starts with stale params. - The `templateParamsMatch` check before updating avoids spurious ConfigMap writes, which would otherwise trigger unnecessary LimaVM reconciles and potential restarts. **Concerns** - **(future)** The three template helper functions (`parseTemplateParams`, `applySpecToTemplate`, `templateParamsMatch`) round-trip the entire template YAML through `sigs.k8s.io/yaml` unmarshal/marshal on every reconcile that reaches line 230. This reorders YAML keys alphabetically (since `sigs.k8s.io/yaml` goes YAML → JSON → Go → JSON → YAML, and Go's JSON encoder sorts map keys). The first round-trip at creation time reorders the embedded template, and subsequent round-trips are stable because the input is already sorted. This is not a bug today, but if a future caller passes a hand-authored YAML to `applySpecToTemplate`, the cosmetic reordering could cause a spurious template "change" detected by the LimaVM controller. Worth noting for future callers. --- ### Testing Assessment The existing BATS tests in `bats/tests/32-app-controllers/app.bats` cover the App lifecycle (creation, running propagation, condition mirroring, deletion) but do not specify `containerEngine` or `kubernetes` fields — they rely on defaults. This means the new code paths are exercised only with default values. Untested scenarios, ranked by risk: 1. **Runtime param change propagation**: Change `spec.containerEngine.name` from `moby` to `containerd` on a running App and verify the template ConfigMap is updated and the LimaVM detects the change (restartNeeded is set). 2. **Kubernetes enabled toggle**: Change `spec.kubernetes.enabled` from `false` to `true` and verify the template param changes. 3. **Initial creation with non-default values**: Create an App with `containerEngine.name: containerd` and verify the input ConfigMap has the correct params. 4. **Template content verification**: After App creation, read the template ConfigMap and verify the provision script contains the correct param references. 5. **Unit tests for `parseTemplateParams`/`applySpecToTemplate`/`templateParamsMatch`**: As noted in Finding 2. ### Documentation Assessment The design doc (`docs/design/api_app.md`) is updated to document all three new fields with clear descriptions of their behavior and propagation mechanism. The example YAML is consistent with the implementation. The removed `status.containerEngine`, `status.kubernetes`, `status.progress`, `status.version`, and `status.onlineStatus` fields from the example reflect a design simplification — status is now purely conditions-based, which matches the actual implementation. No additional documentation gaps. ### Commit Structure The two commits are cleanly separated: 1. `d35ce52`: Design doc updates (spec-first) 2. `aa14a63`: Implementation (types, controller, CRD, template) This is a design-first approach that is reasonable for a planned feature. Each commit is self-contained and the messages accurately describe their changes. ### Acknowledged Limitations No TODO comments or acknowledged limitations in the changed code. ### Unresolved Feedback No review comments exist on this PR. ### Coverage Summary | File | Status | |------|--------| | `docs/design/api_app.md` | Reviewed, no issues | | `pkg/apis/app/v1alpha1/app_types.go` | Finding 2 (hardcoded version) | | `pkg/apis/app/v1alpha1/zz_generated.deepcopy.go` | Trivial — generated code, verified correct for value-only types | | `pkg/controllers/app/app/controllers/app_controller.go` | Finding 1 (nil map), Finding 2 (no tests) | | `pkg/controllers/app/app/crd.yaml` | Finding S1 (unquoted version default) — generated, see note | | `pkg/controllers/app/app/lima-template.yaml` | Finding S2 (hardcoded version) | ### Codex GPT 5.4 **Executive Summary** This PR extends `App.spec` with container engine and Kubernetes settings, injects them into the initial Lima template, and then keeps the LimaVM template ConfigMap in sync at runtime. I found two important issues in the App controller: the new runtime update path can restart a VM even when the same App update asked to stop it, and it introduces a second controller writing directly to a LimaVM-owned ConfigMap, which violates the repo’s controller ownership boundary and is the root cause of that ordering bug. **Findings** **Critical Issues** None. **Important Issues** 1. **Template changes can restart a VM that the same App update asked to stop** — [pkg/controllers/app/app/controllers/app_controller.go:222](\/Users\/jan\/git\/rancher-desktop-daemon\/pkg\/controllers\/app\/app\/controllers\/app_controller.go#L222) (important, regression) ```go if !match { updatedTemplate, err := applySpecToTemplate(currentTemplate, app.Spec) // ... if err := r.Update(ctx, templateCM); err != nil { return ctrl.Result{}, fmt.Errorf("failed to update template ConfigMap with new params: %w", err) } return ctrl.Result{}, nil } ``` The early return at line 250 prevents the later `spec.running` propagation at lines 255-261 from running in the same reconcile. That means a single App update that changes `spec.containerEngine`/`spec.kubernetes` and also sets `spec.running=false` first mutates the template ConfigMap, which the LimaVM controller interprets as a template change while `LimaVM.spec.running` is still `true`. In `handleRunningState()` at [pkg/controllers/lima/limavm/controllers/limavm_lifecycle.go:171](\/Users\/jan\/git\/rancher-desktop-daemon\/pkg\/controllers\/lima\/limavm\/controllers\/limavm_lifecycle.go#L171) and `handleRestartNeeded()` at [pkg/controllers/lima/limavm/controllers/limavm_lifecycle.go:323](\/Users\/jan\/git\/rancher-desktop-daemon\/pkg\/controllers\/lima\/limavm\/controllers\/limavm_lifecycle.go#L323), that stale `shouldRun=true` path stops the VM for restart and then starts it again before the later App reconcile finally delivers the stop request. In practice, “stop and apply settings” becomes “restart, then stop”. Fix: propagate `App.spec.running` to `LimaVM.spec.running` before touching the template ConfigMap, or better, move these new settings onto the LimaVM API so one reconcile can apply both desired-state changes atomically. The minimal local fix is to reorder the App reconcile: ```diff - if limaVM.Status.TemplateConfigMap != "" { - ... - return ctrl.Result{}, nil - } - if limaVM.Spec.Running != app.Spec.Running { limaVM.Spec.Running = app.Spec.Running if err := r.Update(ctx, limaVM); err != nil { return ctrl.Result{}, fmt.Errorf("failed to propagate running state to LimaVM: %w", err) } return ctrl.Result{}, nil } + + if limaVM.Status.TemplateConfigMap != "" { + ... + return ctrl.Result{}, nil + } ``` 2. **The App controller now mutates a LimaVM-owned internal ConfigMap directly** — [pkg/controllers/app/app/controllers/app_controller.go:222](\/Users\/jan\/git\/rancher-desktop-daemon\/pkg\/controllers\/app\/app\/controllers\/app_controller.go#L222) (important, regression) ```go if limaVM.Status.TemplateConfigMap != "" { templateCM := &corev1.ConfigMap{} if err := r.Get(ctx, client.ObjectKey{Name: limaVM.Status.TemplateConfigMap, Namespace: namespace}, templateCM); err == nil { templateCM.Data[limav1alpha1.TemplateConfigMapKey] = updatedTemplate if err := r.Update(ctx, templateCM); err != nil { return ctrl.Result{}, fmt.Errorf("failed to update template ConfigMap with new params: %w", err) } } } ``` Lines 222-244 make the App controller a second writer of the template ConfigMap that [docs/design/api_lima.md:60-68](\/Users\/jan\/git\/rancher-desktop-daemon\/docs\/design\/api_lima.md#L60) describes as LimaVM-owned internal state, and that [docs/design/controllers.md:25-33](\/Users\/jan\/git\/rancher-desktop-daemon\/docs\/design\/controllers.md#L25) says should be managed by the resource’s own controller. This couples App to LimaVM internals (`status.templateConfigMap`, template key layout, update semantics) instead of expressing desired state on the LimaVM resource itself. The practical impact is already visible in the previous bug: once App and LimaVM both mutate different pieces of the same runtime state, reconcile ordering becomes observable and incorrect. Fix: keep the template ConfigMap single-writer. Add explicit fields on `LimaVM.spec` for these params (or implement the documented `spec.params` flow in [docs/design/api_lima.md:64-66](\/Users\/jan\/git\/rancher-desktop-daemon\/docs\/design\/api_lima.md#L64)), have App update only its child `LimaVM`, and let the LimaVM controller remain solely responsible for materializing those fields into its owned ConfigMap and restart logic. **Suggestions** None. **Design Observations** **Strengths** The create path correctly injects the new App fields into the initial input ConfigMap before the LimaVM is created, so first boot and runtime updates use the same template representation instead of inventing a second format. **Testing Assessment** 1. No test covers a single App update that changes template fields and `spec.running` together; that is the highest-risk path because the current reconcile ordering is wrong. 2. No test covers runtime template propagation while the VM is already stopped, to verify the template is updated without triggering restart machinery. 3. No test covers CRD/defaulting behavior for omitted nested `containerEngine`/`kubernetes` fields versus explicitly provided values. `go test ./pkg/controllers/app/... ./pkg/controllers/lima/limavm/...` passes, but `pkg/controllers/app/app/controllers` still has no tests. **Documentation Assessment** `docs/design/api_app.md` was updated, but it now documents direct App-to-template-ConfigMap mutation as intended behavior at [docs/design/api_app.md:86](\/Users\/jan\/git\/rancher-desktop-daemon\/docs\/design\/api_app.md#L86), which conflicts with the controller ownership rules in [docs/design/controllers.md](\/Users\/jan\/git\/rancher-desktop-daemon\/docs\/design\/controllers.md). The docs should either move this flow into a LimaVM API field or explicitly explain why this is a justified exception. **Coverage Summary** - [docs/design/api_app.md](/Users/jan/git/rancher-desktop-daemon/docs/design/api_app.md) — Reviewed, no issues - [pkg/apis/app/v1alpha1/app_types.go](/Users/jan/git/rancher-desktop-daemon/pkg/apis/app/v1alpha1/app_types.go) — Reviewed, no issues - [pkg/apis/app/v1alpha1/zz_generated.deepcopy.go](/Users/jan/git/rancher-desktop-daemon/pkg/apis/app/v1alpha1/zz_generated.deepcopy.go) — Trivial - [pkg/controllers/app/app/controllers/app_controller.go](/Users/jan/git/rancher-desktop-daemon/pkg/controllers/app/app/controllers/app_controller.go) — Finding 1, Finding 2 - [pkg/controllers/app/app/crd.yaml](/Users/jan/git/rancher-desktop-daemon/pkg/controllers/app/app/crd.yaml) — Reviewed, no issues - [pkg/controllers/app/app/lima-template.yaml](/Users/jan/git/rancher-desktop-daemon/pkg/controllers/app/app/lima-template.yaml) — Reviewed, no issues ### Gemini 3.1 Pro This is a deep code review of PR #269. ### Executive Summary This PR adds `containerEngine` and `kubernetes` configurations to the `App` spec, allowing users to define the runtime environment inside the underlying LimaVM. It correctly injects these settings into the Lima template on initial creation; however, it attempts to handle runtime updates by directly mutating a ConfigMap owned by the LimaVM controller — violating the controller framework's ownership boundaries. Additionally, the template manipulation code contains a critical bug that panics on empty or malformed ConfigMaps, and an early-return logic error that drops `Running` state updates during template changes. ### Findings #### Critical Issues 1. **Design Violation: App controller mutates LimaVM-owned ConfigMap** — `pkg/controllers/app/app/controllers/app_controller.go:219-249` (critical, regression) ```go // Propagate app spec.containerEngine and spec.kubernetes into the LimaVM's // template ConfigMap. The LimaVM controller detects the ConfigMap change if limaVM.Status.TemplateConfigMap != "" { templateCM := &corev1.ConfigMap{} if err := r.Get(ctx, client.ObjectKey{Name: limaVM.Status.TemplateConfigMap, Namespace: namespace}, templateCM); err != nil { ... templateCM.Data[limav1alpha1.TemplateConfigMapKey] = updatedTemplate if err := r.Update(ctx, templateCM); err != nil { ``` The App controller directly modifies the `TemplateConfigMap` reported in `limaVM.Status`. As specified in the repo instructions, this ConfigMap is created, owned, and managed by the LimaVM controller's webhook and reconciler. Mutating resources owned by a different controller is a severe design violation. The framework instructions mandate: "the correct fix is to remove the code, not to make it work." Fix: Remove the runtime propagation code block entirely. If `limaVM.Spec.TemplateRef` is immutable by design and LimaVM takes an isolated snapshot, dynamic updates to these parameters will require an upstream architectural change to how LimaVM consumes templates (e.g., exposing mutable parameter fields on `LimaVM.Spec`), rather than bypassing ownership rules. 2. **Panic on nil map assignment in `applySpecToTemplate`** — `pkg/controllers/app/app/controllers/app_controller.go:65` (critical, regression) ```go func applySpecToTemplate(templateData string, spec v1alpha1.AppSpec) (string, error) { tmpl, params, err := parseTemplateParams(templateData) if err != nil { return "", err } params["CONTAINER_ENGINE"] = spec.ContainerEngine.Name ... tmpl["param"] = params ``` When `templateData` is an empty string, `yaml.Unmarshal` successfully sets `tmpl` to `nil`. `parseTemplateParams` initializes a new `params` map but returns the `nil` `tmpl`. When `applySpecToTemplate` executes `tmpl["param"] = params`, it panics with `assignment to entry in nil map`, instantly crashing the controller. This can be triggered if the ConfigMap data goes missing or is intentionally cleared. Fix: Ensure `tmpl` is initialized if nil before modifying it. ```diff - params["CONTAINER_ENGINE"] = spec.ContainerEngine.Name + if tmpl == nil { + tmpl = make(map[string]any) + } + params["CONTAINER_ENGINE"] = spec.ContainerEngine.Name ``` #### Important Issues 1. **Early return skips `Spec.Running` propagation** — `pkg/controllers/app/app/controllers/app_controller.go:245-247` (important, regression) ```go if err := r.Update(ctx, templateCM); err != nil { return ctrl.Result{}, fmt.Errorf("failed to update template ConfigMap with new params: %w", err) } // log.Info(...) return ctrl.Result{}, nil } } } // Propagate spec.running from App into the LimaVM. if limaVM.Spec.Running != app.Spec.Running { ``` If the template does not match and is updated successfully, the controller returns `ctrl.Result{}, nil` immediately. This completely skips the subsequent step that propagates `app.Spec.Running` to `limaVM.Spec.Running`. Because the App controller does not watch the updated `templateCM`, returning early means the controller will stall and drop the `Running` state change until a future, unrelated watch event triggers another reconcile. Fix: Do not return early. Allow the reconcile function to continue and execute the `Running` state propagation. 2. **Type mismatch in interface equality check** — `pkg/controllers/app/app/controllers/app_controller.go:81` (important, regression) ```go func templateParamsMatch(templateData string, spec v1alpha1.AppSpec) (bool, error) { _, params, err := parseTemplateParams(templateData) // ... return params["CONTAINER_ENGINE"] == spec.ContainerEngine.Name && params["KUBERNETES_ENABLED"] == spec.Kubernetes.Enabled && params["KUBERNETES_VERSION"] == spec.Kubernetes.Version, nil } ``` `params` is a `map[string]any` produced by `yaml.Unmarshal`. If a user manually alters the template ConfigMap and defines `KUBERNETES_VERSION: 1.30` (without quotes) or `KUBERNETES_ENABLED: "false"` (with quotes), they unmarshal into a `float64` and `string` respectively. In Go, an `interface{}` equality check `float64(1.30) == "1.30"` evaluates to `false` even if semantically equivalent, resulting in an infinite update loop on every reconcile. Fix: Convert both sides to strings safely using `fmt.Sprintf("%v", ...)` before comparison, or parse types safely with comma-ok assertions before evaluating match logic. #### Suggestions 1. **Stale Documentation resulting from design violation** — `docs/design/api_app.md:78` (suggestion, regression) The API documentation states: *"When changed on a running App, the App controller updates the LimaVM's template ConfigMap"*. Since this is an explicit design violation, the documentation should be reverted until an upstream mechanism supports propagating template parameter changes to a running LimaVM. ### Design Observations **Concerns:** - `(in-scope)` **Immutable `TemplateRef` limits dynamic updates:** LimaVM is architected to clone its `TemplateRef` ConfigMap at creation and considers `TemplateRef` immutable. This cleanly isolates LimaVM instances, but it prevents higher-level orchestrators (like the App controller) from dynamically tuning environment variables. To support runtime reconfiguration, the `LimaVM` API should either expose mutable parameters in `LimaVM.Spec.Params` or provide a legitimate avenue to point to updated templates without forcing downstream owners to hack around cross-controller ownership rules. **Strengths:** - Injecting configurations dynamically into the `lima.yaml` provisioning stage via `param:` neatly decouples the base provisioning script from environment state variations. ### Testing Assessment 1. The YAML parsing/manipulation logic (`parseTemplateParams`, `applySpecToTemplate`, and `templateParamsMatch`) has zero unit tests. Edge cases around empty strings, nil maps, missing `param` keys, and numeric/boolean type conversions are currently untested, directly resulting in the critical bugs discovered in this review. ### Coverage Summary - `docs/design/api_app.md` — Suggestion 1 - `pkg/apis/app/v1alpha1/app_types.go` — Reviewed, no issues - `pkg/apis/app/v1alpha1/zz_generated.deepcopy.go` — Trivial - `pkg/controllers/app/app/controllers/app_controller.go` — Critical 1, Critical 2, Important 1, Important 2 - `pkg/controllers/app/app/crd.yaml` — Trivial - `pkg/controllers/app/app/lima-template.yaml` — Reviewed, no issues