Deep Review: 20260401-120241-pr-269¶

Codex Gemini — important, regression

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.

		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)
			}
			if templateCM.Data == nil {
				templateCM.Data = make(map[string]string)
			}
			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
		}
	}
}

// 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
}

Fix: Move the Running propagation block (lines 255–262) above the template propagation block (lines 219–253), so Running state is always delivered first.

+	// 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 {
-		// ...
-	}

Claude Gemini — important, regression

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.

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)
	if params == nil {
		params = make(map[string]any)
	}

	return tmpl, params, nil
}

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
	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: Guard against nil tmpl in parseTemplateParams:

 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)

Claude — important, gap

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.