Deep Review: PR #269¶

pkg/controllers/app/app/lima-template.yaml:34 Claude Codex Gemini — important, regression

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.

  hostSocket: "{{.Home}}/.rd/docker.sock"
param:
  CONTAINER_ENGINE: moby
  KUBERNETES_ENABLED: false
  KUBERNETES_VERSION: 1.34.5
provision:

Fix: Align the template default to 1.30.2.

 param:
   CONTAINER_ENGINE: moby
   KUBERNETES_ENABLED: false
-  KUBERNETES_VERSION: 1.34.5
+  KUBERNETES_VERSION: 1.30.2

pkg/controllers/app/app/controllers/app_controller.go:44-55 Gemini — important, regression

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.

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)
	if params == nil {
		params = make(map[string]any)
	}
	params["CONTAINER_ENGINE"] = spec.ContainerEngine.Name
	params["KUBERNETES_ENABLED"] = spec.Kubernetes.Enabled
	params["KUBERNETES_VERSION"] = spec.Kubernetes.Version
	tmpl["param"] = params
	data, err := yaml.Marshal(tmpl)
	if err != nil {
		return "", fmt.Errorf("failed to marshal Lima template: %w", err)
	}
	return string(data), nil
}

Fix: Initialize tmpl after unmarshal.

 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)

pkg/controllers/app/app/controllers/app_controller.go:231 Gemini — important, regression

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.

	if limaVM.Status.TemplateConfigMap != "" {
		templateCM := &corev1.ConfigMap{}
		if err := r.Get(ctx, client.ObjectKey{Name: limaVM.Status.TemplateConfigMap, Namespace: namespace}, templateCM); err != nil {
			if !apierrors.IsNotFound(err) {
				return ctrl.Result{}, fmt.Errorf("failed to fetch LimaVM template ConfigMap: %w", err)
			}
		} else {
			currentTemplate := templateCM.Data[limav1alpha1.TemplateConfigMapKey]
			match, err := templateParamsMatch(currentTemplate, app.Spec)
			if err != nil {
				return ctrl.Result{}, fmt.Errorf("failed to check template params: %w", err)
			}
			if !match {
				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 err := r.Update(ctx, templateCM); err != nil {
					return ctrl.Result{}, fmt.Errorf("failed to update template ConfigMap with new params: %w", err)
				}
				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
			}
		}
	}

Fix: Initialize the map before assignment.

+if templateCM.Data == nil {
+    templateCM.Data = make(map[string]string)
+}
 templateCM.Data[limav1alpha1.TemplateConfigMapKey] = updatedTemplate

docs/design/api_app.md:92-94 Claude — important, regression

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.

- **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.

Fix: Remove one of the two identical paragraphs.