# Deep Review: 20260331-145703-pr-269 | | | |---|---| | **Date** | 2026-03-31 14:57 | | **Repo** | [rancher-sandbox/rancher-desktop-daemon](https://github.com/rancher-sandbox/rancher-desktop-daemon) | | **Round** | 1 | | **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** | `3bf43b1` Add containerEngine and kubernetes spec fields
`fc0ac74` Update documents to include specs for Kubernetes and container engine | | **Reviewers** | Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro | | **Verdict** | **Merge with fixes** — template version mismatch and nil-map panics need attention; design and propagation logic are sound | | **Wall-clock time** | `14 min 12 s` | --- ## Executive Summary This PR adds `spec.containerEngine.name` and `spec.kubernetes.{enabled,version}` to the App resource. The controller injects these as Lima template parameters on creation and propagates changes at runtime by updating the LimaVM's template ConfigMap. The implementation follows the existing one-mutation-per-reconcile pattern and the design doc's intended architecture. Key issues: a version default mismatch in the embedded template, nil-map panics reachable through corrupted ConfigMap data, and a duplicate paragraph in the design doc. --- ## Critical Issues None. --- ## Important Issues 1. **Default `KUBERNETES_VERSION` mismatch in embedded template** — `pkg/controllers/app/app/lima-template.yaml:34` [Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro] (important, regression) ```yaml param: CONTAINER_ENGINE: moby KUBERNETES_ENABLED: false KUBERNETES_VERSION: 1.34.5 ``` The embedded template defaults `KUBERNETES_VERSION` to `1.34.5`, but the CRD default (`crd.yaml:70`), the Go type default (`app_types.go:29`), and the design doc (`api_app.md:90`) all specify `1.30.2`. Although `applySpecToTemplate` always overwrites the param from the App spec, the template file serves as the canonical reference for Lima template structure. A reader encountering `1.34.5` will assume it is the intended default. Git blame confirms this value was introduced by commit `3bf43b1`. Fix: Align the template default to `1.30.2`. ```diff param: CONTAINER_ENGINE: moby KUBERNETES_ENABLED: false - KUBERNETES_VERSION: 1.34.5 + KUBERNETES_VERSION: 1.30.2 ``` 2. **Nil map panic in `applySpecToTemplate` on empty template data** — `pkg/controllers/app/app/controllers/app_controller.go:44-55` [Gemini 3.1 Pro] (important, regression) ```go var tmpl map[string]any if err := yaml.Unmarshal([]byte(templateData), &tmpl); err != nil { return "", fmt.Errorf("failed to parse Lima template: %w", err) } params, _ := tmpl["param"].(map[string]any) if params == nil { params = make(map[string]any) } // ... tmpl["param"] = params // panics if tmpl is nil ``` When `templateData` is an empty string, `yaml.Unmarshal` returns nil error and leaves `tmpl` nil. The nil guard at line 49 covers `params` but not `tmpl` itself. The assignment `tmpl["param"] = params` at line 55 panics on a nil map. Reachable via the runtime propagation path (line 227) when the LimaVM template ConfigMap exists but its `template` key is missing or empty — possible through out-of-band modification. The creation path (line 150) uses the embedded template, which is never empty. Downgraded from Gemini's CRITICAL to IMPORTANT: in RDD's single-user embedded context, out-of-band ConfigMap corruption is unlikely. The controller would crash-loop until corrected, but the window is narrow. Fix: Initialize `tmpl` after unmarshal. ```diff if err := yaml.Unmarshal([]byte(templateData), &tmpl); err != nil { return "", fmt.Errorf("failed to parse Lima template: %w", err) } +if tmpl == nil { + tmpl = make(map[string]any) +} params, _ := tmpl["param"].(map[string]any) ``` 3. **Nil map assignment on `templateCM.Data`** — `pkg/controllers/app/app/controllers/app_controller.go:231` [Gemini 3.1 Pro] (important, regression) ```go templateCM.Data[limav1alpha1.TemplateConfigMapKey] = updatedTemplate ``` If the template ConfigMap has no `data` field (nil map), this assignment panics. In practice the LimaVM controller always creates this ConfigMap with data, but the guard costs one line and prevents a crash-loop if the ConfigMap is ever corrupted. Downgraded from Gemini's CRITICAL to IMPORTANT for the same reasons as finding 2. Fix: Initialize the map before assignment. ```diff +if templateCM.Data == nil { + templateCM.Data = make(map[string]string) +} templateCM.Data[limav1alpha1.TemplateConfigMapKey] = updatedTemplate ``` 4. **Duplicate `status.conditions` paragraph in design doc** — `docs/design/api_app.md:92-94` [Claude Opus 4.6] (important, regression) ```markdown - **status.conditions**: Conditions are **mirrored from the owned `LimaVM`**... - **status.conditions**: Conditions are **mirrored from the owned `LimaVM`**... ``` Git blame shows line 92 was added by commit `fc0ac74` (this PR) and line 94 is pre-existing (commit `3b08c6b9`). The new paragraph duplicates the old rather than replacing it. Fix: Remove one of the two identical paragraphs. --- ## Suggestions 1. **YAML parse/extract duplication across helper functions** — `pkg/controllers/app/app/controllers/app_controller.go:43-75` [Claude Opus 4.6] (suggestion, regression) ```go func applySpecToTemplate(templateData string, spec v1alpha1.AppSpec) (string, error) { var tmpl map[string]any if err := yaml.Unmarshal([]byte(templateData), &tmpl); err != nil { ... } params, _ := tmpl["param"].(map[string]any) ... } func templateParamsMatch(templateData string, spec v1alpha1.AppSpec) (bool, error) { var tmpl map[string]any if err := yaml.Unmarshal([]byte(templateData), &tmpl); err != nil { ... } params, _ := tmpl["param"].(map[string]any) ... } ``` Both functions unmarshal the template YAML and extract the `param` map with identical code. A small helper like `parseTemplateParams(templateData string) (map[string]any, map[string]any, error)` would consolidate the duplication and ensure the nil-map fix from finding 2 applies in one place. Minor, since there are only two call sites. 2. **Input ConfigMap not reconciled on retry** — `pkg/controllers/app/app/controllers/app_controller.go:143-168` [Codex GPT 5.4] (suggestion, regression) ```go if apierrors.IsNotFound(err) { templateData, err := applySpecToTemplate(r.LimaTemplateData, app.Spec) ... if err := r.Create(ctx, inputCM); err != nil { ... } } // Create LimaVM... ``` If the input ConfigMap was created on a previous reconcile but the LimaVM creation failed, the next reconcile skips the ConfigMap (it already exists) and creates the LimaVM with potentially stale template data. This matters only if the user changes the App spec between the failed ConfigMap creation and the retry — a narrow window. The runtime propagation path (lines 214-242) self-heals the mismatch once the LimaVM is up. Fix (if desired): Always reconcile the input ConfigMap contents against the current spec before creating the LimaVM. --- ## Design Observations ### Concerns - **App controller updates a ConfigMap owned by LimaVM** (in-scope) [Codex GPT 5.4, Gemini 3.1 Pro]: The runtime propagation at lines 214-242 reaches through `limaVM.Status.TemplateConfigMap` and directly updates a ConfigMap managed by the LimaVM controller. The project's controller ownership model (documented in `docs/design/controllers.md`) states each controller manages only its own resources. However, the design doc for this PR (`api_app.md:86`) explicitly endorses this pattern for parameter propagation. The pragmatic benefit — reusing LimaVM's existing template-change/restart path instead of inventing a parallel mechanism — justifies the boundary crossing. A future `LimaVMSpec.Params` field (as Codex and Gemini suggest) would resolve this cleanly. The current approach works correctly but couples App to LimaVM's internal storage layout. ### Strengths - The priority ordering in the reconcile loop (template params before running state) ensures the template is consistent before a start/stop transition. The one-mutation-per-reconcile pattern is correct: the LimaVM status update triggers the next App reconcile, which then propagates the running state. [Claude Opus 4.6] - The creation path applies spec params to the template before creating the input ConfigMap, so the first boot uses the correct configuration. [Claude Opus 4.6] - `templateParamsMatch` avoids unnecessary ConfigMap writes when params are already in sync, preventing spurious LimaVM reconciles. [Claude Opus 4.6] --- ## Testing Assessment No new tests accompany this PR. The existing BATS tests in `bats/tests/32-app-controllers/app.bats` create the App with only `namespace` and `running`, exercising CRD defaults implicitly but never verifying template params. Untested scenarios, ranked by risk: 1. **Changing `containerEngine.name` on a running App triggers a template ConfigMap update and LimaVM restart** — the core runtime behavior this PR adds. 2. **Creating an App with non-default `kubernetes.version` produces the correct template params** — verifies the creation path. 3. **`templateParamsMatch` correctly detects in-sync params and skips the ConfigMap update** — the idempotency path. 4. **Handling of corrupted or empty template ConfigMap data** — the nil-map panic path from findings 2 and 3. --- ## Documentation Assessment The design doc (`api_app.md`) now documents all three new spec fields with clear descriptions of their behavior and propagation mechanism. The example YAML was updated appropriately. Fix the duplicate `status.conditions` paragraph (finding 4) before merge. --- ## Commit Structure The two commits have a reasonable split: `fc0ac74` for doc changes (design doc), `3bf43b1` for code changes (types, deepcopy, controller, CRD, template). The doc-first ordering describes intent before implementation. The commit ordering in git history shows the doc commit first, which reads well. --- ## Agent Performance Retro ### [Claude] Claude Opus 4.6 delivered a thorough, well-calibrated review. It identified the version mismatch (shared finding), the duplicate design doc paragraph (unique), and the code duplication suggestion. Its design observations about the one-mutation-per-reconcile pattern and creation-path correctness demonstrated good understanding of the controller architecture. Claude ran `git blame` to verify attributions. No false positives. ### [Codex] Codex GPT 5.4 identified two findings: the input ConfigMap retry staleness (unique, valid but low-risk) and the cross-controller ownership concern (shared with Gemini as a design observation). It also caught the version mismatch. Its cross-controller ownership analysis was thorough, referencing the LimaVM types and searching for other consumers. However, Codex rated the ownership concern as IMPORTANT when the design doc explicitly endorses it — the finding should have been a design observation. It missed the duplicate design doc paragraph and the nil-map panics. ### [Gemini] Gemini 3.1 Pro produced the deepest correctness analysis, uniquely identifying both nil-map panic paths. These are real bugs, verified by running the Go code. However, Gemini overrated them as CRITICAL — in RDD's single-user embedded context, out-of-band ConfigMap corruption is unlikely, warranting IMPORTANT. Gemini also raised an early-return stall finding (Important #3) that is a false positive: the one-mutation-per-reconcile pattern is standard in controller-runtime, and the LimaVM status update triggers the next reconcile. Gemini missed the duplicate design doc paragraph. ### Summary | Metric | Claude Opus 4.6 | Codex GPT 5.4 | Gemini 3.1 Pro | |--------|-----------------|---------------|----------------| | Duration | 5:22 | 4:35 | 5:24 | | Critical | 0 | 0 | 2 (downgraded) | | Important | 2 | 2 | 2 | | Suggestion | 1 | 1 | 0 | | Design observations | 2 | 1 | 1 | | False positives | 0 | 0 | 1 | | Unique insights | 2 | 1 | 2 | | Files reviewed | 6/6 | 6/6 | 6/6 | | Coverage misses | 0 | 0 | 0 | All three agents achieved full file coverage. Claude provided the most balanced and accurate review with zero false positives. Gemini contributed the most valuable unique findings (nil-map panics) but suffered from severity inflation and one false positive. Codex was solid but missed both the nil-map panics and the doc duplication. --- ## Review Process Notes No suggestions for this round. The review prompt and repo context performed well — agents calibrated severity appropriately for the embedded single-user context (with one exception from Gemini), traced into related controllers, and followed the ownership model guidance. --- ## Appendix: Original Reviews ### Claude Opus 4.6 --- ## PR #269 Review: Add containerEngine and kubernetes fields to the App resource ### Executive Summary This PR adds `spec.containerEngine.name` and `spec.kubernetes.{enabled,version}` to the App resource, with the controller injecting these as Lima template params on creation and propagating changes to the LimaVM's template ConfigMap at runtime. The implementation is straightforward and follows the existing controller patterns. Key concerns are a default version mismatch in the template file, a duplicate paragraph in the design doc, and missing test coverage for the new fields. **Verdict: Approve with minor fixes** — the code is functionally correct; the issues below are all low-risk. --- ### Findings #### Critical Issues None. #### Important Issues 1. **Misleading default `KUBERNETES_VERSION` in template** — `pkg/controllers/app/app/lima-template.yaml:34` (important, regression) ```yaml param: CONTAINER_ENGINE: moby KUBERNETES_ENABLED: false KUBERNETES_VERSION: 1.34.5 ``` The template file defaults `KUBERNETES_VERSION` to `1.34.5`, while the CRD default (at `pkg/controllers/app/app/crd.yaml:70`) and the Go type default (at `pkg/apis/app/v1alpha1/app_types.go:29`) both specify `1.30.2`. Although `applySpecToTemplate` (line 54 in `app_controller.go`) always overwrites the param from the spec, the template file is the canonical reference for what params the Lima template accepts. A reader encountering `1.34.5` here will reasonably assume it is the intended default version. If this was intentional (e.g., a test value), it should have a comment explaining why it differs. Fix: Align the template default to `1.30.2` for consistency, or add a comment explaining the deliberate divergence. ```diff param: CONTAINER_ENGINE: moby KUBERNETES_ENABLED: false - KUBERNETES_VERSION: 1.34.5 + KUBERNETES_VERSION: 1.30.2 ``` 2. **Duplicate `status.conditions` paragraph in design doc** — `docs/design/api_app.md:92-94` (important, regression) ```markdown - **status.conditions**: Conditions are **mirrored from the owned `LimaVM`** resource. The App controller copies `type`, `status`, `reason`, `message`, and `lastTransitionTime` from the LimaVM's conditions. - **status.conditions**: Conditions are **mirrored from the owned `LimaVM`** resource. The App controller copies `type`, `status`, `reason`, `message`, and `lastTransitionTime` from the LimaVM's conditions. ``` Git blame confirms: line 92 was introduced by this PR (commit `fc0ac74`), while line 94 is pre-existing (commit `3b08c6b9`). The new paragraph was inserted above the existing one rather than replacing it. Fix: Remove the duplicate paragraph at line 94 (or 92 — they're identical). #### Suggestions 1. **YAML parse/extract duplication across `applySpecToTemplate` and `templateParamsMatch`** — `pkg/controllers/app/app/controllers/app_controller.go:43-75` (suggestion, regression) ```go func applySpecToTemplate(templateData string, spec v1alpha1.AppSpec) (string, error) { var tmpl map[string]any if err := yaml.Unmarshal([]byte(templateData), &tmpl); err != nil { return "", fmt.Errorf("failed to parse Lima template: %w", err) } params, _ := tmpl["param"].(map[string]any) // ... } func templateParamsMatch(templateData string, spec v1alpha1.AppSpec) (bool, error) { var tmpl map[string]any if err := yaml.Unmarshal([]byte(templateData), &tmpl); err != nil { return false, fmt.Errorf("failed to parse Lima template for comparison: %w", err) } params, _ := tmpl["param"].(map[string]any) // ... } ``` Both functions unmarshal the template YAML and extract the `param` map with the same pattern. If the key name or structure changes, both need updating independently. A small helper like `parseTemplateParams(templateData string) (fullMap map[string]any, params map[string]any, err error)` would remove the duplication. Minor, since there are only two call sites. --- ### Design Observations **Concerns** - **App controller updates a ConfigMap owned by LimaVM** (in-scope): The template propagation at lines 214–242 of `app_controller.go` updates a ConfigMap that has an owner reference pointing to the LimaVM, which normally falls under the LimaVM controller's domain. The design doc at `api_app.md:86` explicitly describes this as intentional. This is a pragmatic choice — the ConfigMap serves as a cross-controller communication channel, and the LimaVM controller's `Owns(&corev1.ConfigMap{})` watch (at `limavm_controller.go:466`) ensures it detects the change immediately. No action needed, but future contributors should understand this is a deliberate boundary crossing, not an accident. A brief inline comment at the update site (line 232) would help future readers. **Strengths** - The priority ordering in the reconcile loop (template params before running state, at lines 211–242) is correct: it ensures the template is consistent before a start/stop transition can happen. The one-mutation-per-reconcile pattern (returning after the ConfigMap update at line 239) is clean and lets the LimaVM controller handle the restart path. - The creation path (line 150) applies spec params to the template before creating the input ConfigMap, ensuring the first boot already has the correct params rather than relying on a post-creation update cycle. - The `templateParamsMatch` comparison avoids unnecessary ConfigMap writes when params are already in sync, preventing spurious LimaVM reconciles. --- ### Testing Assessment No new tests accompany this PR. The existing BATS tests in `bats/tests/32-app-controllers/app.bats` create the App with only `namespace` and `running` (line 27–35 of `app.bats`), so they exercise the CRD defaults implicitly but never verify the template ConfigMap contains the correct params. Untested scenarios, ranked by risk: 1. **Changing `containerEngine.name` on a running App triggers a LimaVM restart** — this is the core runtime behavior the PR adds. Without coverage, a regression could silently break the propagation path. 2. **Creating an App with non-default `kubernetes.version` produces the correct template params** — verifies the creation path. 3. **`templateParamsMatch` correctly detects in-sync params and skips the ConfigMap update** — the idempotency path. 4. **Creating an App with `kubernetes.enabled: true`** — verifies the boolean param roundtrips correctly through YAML marshal/unmarshal. --- ### Documentation Assessment - The design doc (`api_app.md`) now documents all three new spec fields with clear descriptions of their behavior and propagation mechanism. The example YAML was cleaned up to remove stale status fields and add `observedGeneration` to conditions. - The duplicate `status.conditions` paragraph (finding #2) should be fixed before merge. --- ### Commit Structure The two commits have a reasonable split: - `fc0ac74` — doc changes (design doc update) - `3bf43b1` — code changes (types, deepcopy, controller, CRD, template) The doc-first ordering is fine since the design doc describes the intended behavior. The CRD YAML change belongs in the code commit since it's generated from the Go types. --- ### Unresolved Feedback No review comments on the PR. No comments on the referenced issue #259. --- ### Coverage Summary | File | Status | |------|--------| | `docs/design/api_app.md` | **Finding 2** (duplicate paragraph) | | `pkg/apis/app/v1alpha1/app_types.go` | Reviewed, no issues | | `pkg/apis/app/v1alpha1/zz_generated.deepcopy.go` | Trivial — auto-generated by controller-gen; value-only types, correct | | `pkg/controllers/app/app/controllers/app_controller.go` | **Finding 1** (via template defaults), **Suggestion 1** (code duplication) | | `pkg/controllers/app/app/crd.yaml` | Trivial — generated from Go types, consistent with `app_types.go` | | `pkg/controllers/app/app/lima-template.yaml` | **Finding 1** (version mismatch) | ### Codex GPT 5.4 Verdict: Needs changes ### Executive Summary This PR adds `spec.containerEngine.name` and `spec.kubernetes.{enabled,version}` to `App`, injects them into the Lima template on create, and propagates later changes by rewriting the LimaVM template ConfigMap. The main problems are in the App controller: one retry path can boot a VM from stale settings, and the runtime propagation is implemented by mutating a child controller’s private ConfigMap instead of expressing intent through the LimaVM API. ### Findings **Critical Issues** None. **Important Issues** 1. **Retries can create a LimaVM from stale App settings** — `pkg/controllers/app/app/controllers/app_controller.go:149-188` (important, regression) ```go if apierrors.IsNotFound(err) { templateData, err := applySpecToTemplate(r.LimaTemplateData, app.Spec) ... if err := r.Create(ctx, inputCM); err != nil { return ctrl.Result{}, fmt.Errorf("failed to create input ConfigMap: %w", err) } } ... if err := r.Create(ctx, limaVM); err != nil { return ctrl.Result{}, fmt.Errorf("failed to create LimaVM: %w", err) } ``` The controller only reapplies `app.Spec` when the input ConfigMap is missing at line 149, so any retry after `r.Create(ctx, inputCM)` succeeds at line 166 but before `r.Create(ctx, limaVM)` succeeds at line 188 will reuse whatever template data was already in `rd`. That matters now because the new settings are baked into that ConfigMap at lines 150-160: if the first reconcile used old `spec.containerEngine` / `spec.kubernetes` values and the create later retries, the LimaVM webhook will copy stale params into the real template and the first VM boot will come up with the wrong configuration. The later sync at lines 214-239 repairs the template only after `status.templateConfigMap` exists, so it cannot prevent that incorrect first boot. Fix: before creating the LimaVM, always reconcile the existing input ConfigMap contents against the current `app.Spec`, not just when the ConfigMap is absent. For example: ```diff - if apierrors.IsNotFound(err) { - templateData, err := applySpecToTemplate(r.LimaTemplateData, app.Spec) - ... - if err := r.Create(ctx, inputCM); err != nil { ... } - } + templateData, err := applySpecToTemplate(r.LimaTemplateData, app.Spec) + if err != nil { ... } + if apierrors.IsNotFound(err) { + inputCM = &corev1.ConfigMap{ ... Data: map[string]string{limav1alpha1.TemplateConfigMapKey: templateData} } + if err := r.Create(ctx, inputCM); err != nil { ... } + } else if inputCM.Data[limav1alpha1.TemplateConfigMapKey] != templateData { + inputCM.Data[limav1alpha1.TemplateConfigMapKey] = templateData + if err := r.Update(ctx, inputCM); err != nil { ... } + } ``` 2. **App now mutates a LimaVM-owned implementation detail instead of the LimaVM API** — `pkg/controllers/app/app/controllers/app_controller.go:211-233` (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) } } } ``` The new runtime propagation path at lines 214-233 reaches through `limaVM.Status.TemplateConfigMap` and overwrites `data.template` directly, but `pkg/apis/lima/v1alpha1/limavm_types.go:63-68` documents that ConfigMap as LimaVM-managed and says `status.templateConfigMap` is informational only. `rg` shows no other controller besides LimaVM itself consuming `status.templateConfigMap`; this PR makes App the first cross-controller writer. In this codebase’s controller model, that is the wrong ownership boundary: App should update the child resource, and LimaVM should remain the sole writer of its own dependents. Practically, this couples App to LimaVM’s storage layout and controller ordering, and it creates avoidable cross-controller update conflicts on the same ConfigMap resource. Fix: add an explicit LimaVM API for these settings, then have the LimaVM controller render and update its own template ConfigMap. For example, extend `LimaVMSpec` with a params/settings field and let App update only the `LimaVM` object. **Suggestions** 1. **Embedded template default disagrees with the App CRD default** — `pkg/controllers/app/app/lima-template.yaml:31-34` (suggestion, regression) ```yaml param: CONTAINER_ENGINE: moby KUBERNETES_ENABLED: false KUBERNETES_VERSION: 1.34.5 ``` The embedded template hard-codes `KUBERNETES_VERSION: 1.34.5` at line 34, but the API default is `1.30.2` in `pkg/apis/app/v1alpha1/app_types.go:27-30`, `pkg/controllers/app/app/crd.yaml:67-80`, and `docs/design/api_app.md:90`. The happy path currently masks this because `applySpecToTemplate()` rewrites the template, but the codebase now has two different “defaults” for the same field. Fix: define the default once in Go and use it to generate both the CRD/docs-facing default and the embedded template value. ### Design Observations **Strengths** - Reusing LimaVM’s existing template-change/restart path is a good direction; it avoids inventing a second restart mechanism and keeps the restart semantics in one controller. - The App design doc was updated alongside the API changes, which makes the intended surface area much easier to review. ### Testing Assessment I did not see any new tests in the diff, and the existing App/Lima BATS coverage does not exercise these new fields. 1. No test covers the retry/crash window where `rd` already exists but the `LimaVM` does not; that is the highest-risk path because it can produce an incorrectly configured first boot. 2. No App-controller test patches `spec.containerEngine` or `spec.kubernetes` and verifies that the LimaVM template ConfigMap changes and the running VM restarts, or that a stopped VM updates its on-disk template without starting. 3. No validation/defaulting test covers the new schema fields, especially the `containerEngine.name` enum and the `kubernetes.version` default. ### Documentation Assessment `docs/design/api_app.md` now contains the `status.conditions` bullet twice at lines 92 and 94. The docs should also be reconciled with the embedded template so the Kubernetes default version is described consistently across design doc, CRD, and template source. ### Coverage Summary - `docs/design/api_app.md` — Reviewed, no issues - `pkg/apis/app/v1alpha1/app_types.go` — Finding 1, Suggestion 1 - `pkg/apis/app/v1alpha1/zz_generated.deepcopy.go` — Trivial - `pkg/controllers/app/app/controllers/app_controller.go` — Findings 1, 2 - `pkg/controllers/app/app/crd.yaml` — Suggestion 1 - `pkg/controllers/app/app/lima-template.yaml` — Suggestion 1 ### Gemini 3.1 Pro ### Executive Summary This PR introduces `containerEngine` and `kubernetes` configuration fields to the `App` resource, propagating them to the underlying LimaVM template's parameter map via YAML manipulation. The core logic contains a few critical nil map panics when handling empty or missing ConfigMap data, and an early return bug that breaks single-pass state synchronization. ### Findings #### Critical Issues 1. **Nil map panic in `applySpecToTemplate`** — `pkg/controllers/app/app/controllers/app_controller.go:48-55` `(critical, regression)` ```go if err := yaml.Unmarshal([]byte(templateData), &tmpl); err != nil { return "", fmt.Errorf("failed to parse Lima template: %w", err) } params, _ := tmpl["param"].(map[string]any) // ... params["KUBERNETES_VERSION"] = spec.Kubernetes.Version tmpl["param"] = params ``` If `templateData` is an empty string (e.g. if the `templateCM.Data` value was cleared or the key is missing), `yaml.Unmarshal` returns `nil` for both the error and the `tmpl` map. The assignment to `tmpl["param"]` then panics on a `nil` map, causing the controller to endlessly crash loop. Fix: Ensure the root template map is initialized. ```diff if err := yaml.Unmarshal([]byte(templateData), &tmpl); err != nil { return "", fmt.Errorf("failed to parse Lima template: %w", err) } + if tmpl == nil { + tmpl = make(map[string]any) + } params, _ := tmpl["param"].(map[string]any) ``` 2. **Nil map panic when updating template ConfigMap** — `pkg/controllers/app/app/controllers/app_controller.go:227` `(critical, regression)` ```go updatedTemplate, err := applySpecToTemplate(currentTemplate, app.Spec) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to apply spec params to template ConfigMap: %w", err) } templateCM.Data[limav1alpha1.TemplateConfigMapKey] = updatedTemplate ``` If `templateCM` is retrieved with an empty `Data` block (e.g., modified out of band or by another controller), `templateCM.Data` will be `nil`. Assigning the key directly to it causes a nil map panic, putting the controller in a crash loop. Fix: Initialize the map before assignment. ```diff if err != nil { return ctrl.Result{}, fmt.Errorf("failed to apply spec params to template ConfigMap: %w", err) } + if templateCM.Data == nil { + templateCM.Data = make(map[string]string) + } templateCM.Data[limav1alpha1.TemplateConfigMapKey] = updatedTemplate ``` #### Important Issues 3. **Controller stalls by skipping propagation on template update** — `pkg/controllers/app/app/controllers/app_controller.go:235-236` `(important, regression)` ```go log.Info("Updated template ConfigMap with new params", "containerEngine", app.Spec.ContainerEngine.Name, "kubernetesEnabled", app.Spec.Kubernetes.Enabled, "kubernetesVersion", app.Spec.Kubernetes.Version) return ctrl.Result{}, nil } } } // Propagate spec.running from App into the LimaVM. if limaVM.Spec.Running != app.Spec.Running { ``` Returning early after updating the ConfigMap skips the rest of the reconcile loop, including `limaVM.Spec.Running` propagation and condition mirroring. Since the App controller does not watch ConfigMaps, it is relying purely on a side-effect (the LimaVM controller picking up the ConfigMap and producing a LimaVM status event) to trigger the next App reconcile. If a user updates both `spec.kubernetes` and `spec.running` simultaneously, the `running` state will stall indefinitely if the LimaVM controller does not immediately trigger an event. Fix: Remove the early return so the controller continues its single-pass state synchronization. ```diff log.Info("Updated template ConfigMap with new params", "containerEngine", app.Spec.ContainerEngine.Name, "kubernetesEnabled", app.Spec.Kubernetes.Enabled, "kubernetesVersion", app.Spec.Kubernetes.Version) - return ctrl.Result{}, nil } ``` 4. **Mismatch between CRD default and lima-template.yaml** — `pkg/controllers/app/app/lima-template.yaml:34` `(important, gap)` ```yaml param: CONTAINER_ENGINE: moby KUBERNETES_ENABLED: false KUBERNETES_VERSION: 1.34.5 ``` The embedded `lima-template.yaml` uses `1.34.5` as the Kubernetes version, but `pkg/apis/app/v1alpha1/app_types.go` defines the default as `1.30.2`. On the first reconcile, the API Server applies the CRD default of `1.30.2` to the `App` object, which then immediately overwrites the `1.34.5` value in the template. The `lima-template.yaml` should align with the codebase defaults to prevent misleading discrepancies. Fix: Change `1.34.5` to `1.30.2` in `lima-template.yaml`. #### Suggestions None. ### Design Observations #### Concerns - **Coupling to template internals** `(future)`: The App controller mutates the LimaVM template by unmarshaling the raw YAML, updating a nested `param` map, and remarshaling it. This strips YAML comments, alters formatting, and tightly couples the App controller to the internal structure of the `lima-template.yaml` payload. The `docs/design/api_lima.md` design document explicitly describes a `spec.params` field on `LimaVM` that overrides parameters gracefully, bypassing the need to modify the ConfigMap entirely. `LimaVMSpec` does not currently implement this field, but once it does, this logic should be refactored to simply propagate the engine/k8s settings into `limaVM.Spec.Params`. ### Testing Assessment 1. BATS tests in `bats/tests/32-app-controllers/app.bats` lack coverage for the new configuration fields. Specifically, applying a change to `spec.kubernetes.enabled` or `spec.containerEngine` on a running App is untested. 2. Handling of corrupted or missing template ConfigMaps is untested, which allowed the nil map panics to slip through. ### Documentation Assessment The updates to `docs/design/api_app.md` thoroughly explain the new fields and clearly describe the runtime propagation mechanism to the template ConfigMap. ### Coverage Summary - `docs/design/api_app.md`: Reviewed, no issues - `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`: Finding 1, 2, 3 - `pkg/controllers/app/app/crd.yaml`: Reviewed, no issues - `pkg/controllers/app/app/lima-template.yaml`: Finding 4