# Deep Review: 20260417-113401-pr-315

| | |
|---|---|
| **Date** | 2026-04-17 11:34 |
| **Repo** | [rancher-sandbox/rancher-desktop-daemon](https://github.com/rancher-sandbox/rancher-desktop-daemon) |
| **Round** | 2 (of target) |
| **Author** | [@jandubois](https://github.com/jandubois) |
| **PR** | [#315](https://github.com/rancher-sandbox/rancher-desktop-daemon/pull/315) — app: aggregate Ready and Settled conditions, wait for Settled in `rdd set` |
| **Commits** | `27d7dd3` app: add Settled condition, wait for it in `rdd set` |
| **Reviewers** | Claude Opus 4.7 (effort: xhigh), Codex GPT 5.4 (effort: high), Gemini 3.1 Pro (effort: default) |
| **Verdict** | **Merge with fixes** — replace the now-stale "Ready/Settled" mentions with "Settled" only (PR title, body, and code comment), and decide whether to address or accept the documented template-CM race that can let `rdd set containerEngine.name=…` return before the VM has restarted. |
| **Wall-clock time** | `13 min 50 s` |

## Executive Summary

This PR introduces an `AppConditionSettled` aggregate that the App reconciler computes from the mirrored LimaVM `Running` condition and the engine-written `ContainerEngineReady`, then switches `rdd set` to a single wait predicate against `Settled=True && observedGeneration >= post-write`. The implementation collapses the old per-property branching cleanly, the unit-test matrix covers the state machine well, and the `EngineEnabled` flag correctly gates the engine dependency in builds where it is not registered.

Three issues stand out. (1) The PR title and body promise both `Ready` and `Settled`, and a new in-code comment also says "aggregate Ready/Settled" — but only `Settled` was implemented; this needs reconciling before merge. (2) A template-CM update race acknowledged by the author is more impactful than the comment suggests: when `rdd set containerEngine.name=…` triggers a backend swap, the App reconciler can mirror LimaVM's stale `Running=Started` into Settled at the new generation before LimaVM observes the ConfigMap change, so the CLI returns before the VM has restarted — directly defeating the PR's stated motivation. (3) `EngineEnabled` is captured from a process-local registry at startup, which works for today's binary topologies but silently breaks the moment engine ships in a separate manager.

Structure: 0 critical, 3 important, 3 suggestions, plus design observations on the convergence model.

---

## Critical Issues

None.

---

## Important Issues

I1. **Template-CM update race lets `rdd set containerEngine.name=…` return before the VM restarts** — `pkg/controllers/app/app/controllers/app_controller.go:282-302`, `314-344` [Codex GPT 5.4, Gemini 3.1 Pro, Claude Opus 4.7]

```go
if templateCM.Data[limav1alpha1.TemplateConfigMapKey] != desired {
    ...
    // A racing `rdd set` may see Settled=True at the new
    // generation before LimaVM observes the ConfigMap change.
    // LimaVM then sets restartNeeded on its next reconcile,
    // flipping Settled back to False.
    return ctrl.Result{Requeue: true}, nil
}
```

The PR's stated motivation is `rdd set containerEngine.name=containerd` waiting for the VM restart. After the App reconciler patches the template ConfigMap and requeues, the next pass copies LimaVM's still-`Running=Started` condition into the App and re-stamps it with the new `App.Generation` at line 334. `computeSettledCondition` then sees a current-generation `Running=Started` and, on engine-disabled paths (Windows, `--controllers=-engine`) immediately returns `Settled=True`. On engine-enabled paths the engine condition's `ObservedGeneration` is also stale, which holds Settled at `False` via the `EngineStale` branch — but that masking depends on the engine controller running in the same process, not on a real Lima convergence signal.

The new comment at lines 297-300 acknowledges the race as a brief blip, but `rdd set`'s `waitForDesiredState` returns the moment it sees `Settled=True` at the new generation. There is no minimum dwell, no integration test exercising the running-VM backend swap (`engine-docker.bats:534-559` starts from a stopped VM), and no Lima-side condition the App can synchronise on. The result is exactly the early-return behaviour the PR removes for `running=true/false` but reintroduces for backend / kubernetes spec changes on a running VM.

Fix: gate `Settled` on a Lima-owned convergence signal rather than the restamped `Running` mirror. Either (a) add `LimaVMStatus.ObservedTemplateResourceVersion` and require `limaVM.Status.ObservedTemplateResourceVersion == templateCM.ResourceVersion` before computing `Settled=True`, or (b) have LimaVM publish a dedicated "reconciled with current template" condition that the App controller aggregates. As an interim, set Settled to `False` with reason `RestartPending` on the same reconcile that patches the template CM, and persist a marker so the next pass does not flip it back to True until LimaVM has reacted.

I2. **`EngineEnabled` captured from process-local registry at startup ignores cluster-wide topology** — `pkg/controllers/app/app/controller.go:149-154`, `pkg/controllers/base/controller.go:122-157` [Codex GPT 5.4, Gemini 3.1 Pro]

```go
if err := (&controllers.AppReconciler{
    Client:           mgr.GetClient(),
    Scheme:           mgr.GetScheme(),
    LimaTemplateData: limaTemplateData(),
    EngineEnabled:    base.IsControllerEnabled("engine"),
}).SetupWithManager(mgr); err != nil {
```

`base.IsControllerEnabled` is documented as "started in this OS process" (`controller.go:122-157`), populated only from each manager's local set in `service.go:761-772` and `external/main.go:86-92`. The repo ships separate `cmd/app-controller` and `cmd/containers-controller` standalone binaries; `docs/design/discovery.md:56-64` allows external controllers to attach later. Whenever the engine controller runs in a different manager from the App controller — including the future where engine ships in `cmd/containers-controller` — `EngineEnabled` stays `false`, `computeSettledCondition` skips the `ContainerEngineReady` gate at lines 393-405, and `cmd/rdd/set.go:477-486` returns before the engine has connected. This regresses the prior CLI-side behaviour, which queried the discovery ConfigMap to decide whether to wait on the engine condition.

The split-manager scenario is not exploitable today because engine is not blank-imported by any standalone binary (only `pkg/service/cmd/service.go`'s embedded path imports it). That makes this latent rather than active — but the architectural assumption "engine is enabled iff it's in this process" is built into the wait contract just as engine is being prepared for external execution.

Fix: derive engine availability from the shared discovery ConfigMap (`controllers.NewControllerManagerDiscovery(mgr.GetConfig())`) and check it during reconciliation, so late-attaching managers can take effect without a controller restart. Cache the result with a short TTL if the lookup cost is a concern.

I3. **PR title, body, and a new in-code comment claim a `Ready` condition that does not exist** — `pkg/controllers/app/app/controllers/app_controller.go:314`, `pkg/apis/app/v1alpha1/app_types.go:14-39`, PR title and body [Claude Opus 4.7]

```go
// pkg/controllers/app/app/controllers/app_controller.go:314
// Mirror LimaVM status conditions and aggregate Ready/Settled. The
// engine reconciler writes ContainerEngineReady on the same object,
```

The PR title is "app: aggregate Ready and Settled conditions, wait for Settled in `rdd set`" and the body has a two-paragraph "Ready" section explaining the contract. But `app_types.go` defines only `AppConditionSettled` (line 38); there is no `AppConditionReady`, nothing in `computeSettledCondition` produces a Ready entry, and the unit tests only assert the Settled path. The stale comment on `app_controller.go:314` was introduced by this commit and will mislead readers into looking for a Ready aggregator.

This matters for the review itself: a reviewer cannot tell from the PR whether `Ready` was an intentional scope reduction or an implementation oversight — Gemini's executive summary in this round explicitly described the PR as "introduces the `Ready` and `Settled` conditions", taking the PR description at face value.

Fix: decide whether `Ready` is in scope. If not, drop "Ready" from the PR title and body, and correct the comment:

```diff
-	// Mirror LimaVM status conditions and aggregate Ready/Settled. The
-	// engine reconciler writes ContainerEngineReady on the same object,
+	// Mirror LimaVM status conditions and compute Settled. The
+	// engine reconciler writes ContainerEngineReady on the same object,
```

If `Ready` is still intended, add the constant in `app_types.go`, compute it alongside Settled, and extend the test matrix.

---

## Suggestions

S1. **Hardcoded `"engine"` controller name creates silent coupling** — `pkg/controllers/app/app/controller.go:153`, `pkg/controllers/app/app/controllers/app_controller.go:53`, `pkg/service/cmd/service.go:771` [Claude Opus 4.7]

```go
EngineEnabled:    base.IsControllerEnabled("engine"),
```

The App controller queries the engine controller by literal name. Engine's own name constant — `controllerName` at `pkg/controllers/app/engine/controller.go:33` — is unexported, so a rename on the engine side would silently turn `EngineEnabled` into a permanent `false`, regressing `Settled` to the pre-engine path with no build or test failure. The literal is repeated in the doc comment at `app_controller.go:53` and in `service.go:771`.

Fix: define `EngineControllerName = "engine"` in `pkg/apis/app/v1alpha1/app_types.go` (alongside `AppConditionContainerEngineReady`) and reference it from both sides. Importing `pkg/controllers/app/engine` directly from `pkg/controllers/app/app` is undesirable because it would trigger engine's `init()` registration in the app-controller binary, which targets a different API group.

S2. **Settled `Reason` table entry says "*(varies)*" but the values are well-defined** — `docs/design/api_app.md:115` [Claude Opus 4.7]

```markdown
| `Settled`              | `False`   | *(varies)*       | Reconciliation in progress; reason mirrors the blocking condition |
```

`computeSettledCondition` emits an enumerable set of fixed reasons: `WaitingForLimaVM`, `WaitingForEngine`, `EngineStale`, plus the forwarded LimaVM `Running` reason or the engine `ContainerEngineReady` reason. Documenting them lets consumers (GUI, tests, other controllers) branch on specific values without reading the source.

Fix: expand the row into explicit rows for `WaitingForLimaVM`, `WaitingForEngine`, `EngineStale`, and a catch-all "forwarded from Running/ContainerEngineReady" row.

S3. **`runningLimaVMMessage` uses string-suffix matching for failure detection** — `pkg/controllers/app/app/controllers/app_controller.go:430-435` [Claude Opus 4.7]

```go
func runningLimaVMMessage(runningCond *metav1.Condition, desired string) string {
	if strings.HasSuffix(runningCond.Reason, "Failed") && runningCond.Message != "" {
		return runningCond.Message
	}
	return "LimaVM has not yet reached " + desired
}
```

The heuristic couples Settled's behaviour to a LimaVM naming convention. A new failure reason that doesn't end in `Failed` would silently lose its diagnostic message; a non-failure reason ending in `Failed` (none today, but nothing prevents one) would leak a confusing operational message into Settled.

Fix: switch on the known failure reasons, or check `runningCond.Status == metav1.ConditionFalse && runningCond.Message != ""`. The LimaVM reason constants would need to be exported or mirrored; a unit test pinning the App-side strings to the LimaVM values keeps drift visible.

---

## Design Observations

### Concerns

- **Settled infers downstream convergence from mirrored status instead of controller-owned signals** *(in-scope)* [Codex GPT 5.4, Claude Opus 4.7]. The current design lets the App controller guess whether Lima and engine have caught up by mirroring their status into the App and combining that with a process-topology hint. A stronger structure would have Lima and engine each publish an explicit "observed App generation X and finished applying it" condition that App aggregates. This removes both the split-manager dependency in I2 and the stale-restamp race in I1, and gives `rdd set` a true end-to-end synchronisation contract instead of a heuristic.

- **`Settled` absence hangs `rdd set` for the full timeout when the App reconciler is not running** *(future)* [Claude Opus 4.7]. If the App controller is excluded by `--controllers=-app` or fails to start, no one writes Settled and `rdd set` blocks until the 5-minute default timeout expires. `set.bats` papers over this by passing `--wait=false`, but a real deployment that disabled the app controller would hang. The CLI could check for the App controller's presence via the discovery ConfigMap (the same data path the pre-PR `isEngineControllerEnabled` used) and degrade gracefully.

### Strengths

- **`ObservedGeneration` as part of the wait contract** [Codex GPT 5.4]. Once the writers stamp the right generation at the right time, this cleanly prevents stale success from a previous spec from satisfying the wait. The primitive is right; the open issues are about which writer stamps it and when.
- **Conflict-retry on shared status object** [Codex GPT 5.4, Gemini 3.1 Pro]. The multi-writer App status updates at `app_controller.go:320-344` continue to use `retry.RetryOnConflict` plus re-`Get`, consistent with `EngineReconciler.setEngineCondition`. This avoids reintroducing the 409 loop that the prior architecture had between concurrent writers.
- **Stopped-VM short-circuit handles engine teardown contract correctly** [Claude Opus 4.7]. Short-circuiting `!desiredRunning` when `runningCond.Reason == "Stopped"` (`app_controller.go:397-405`) handles the "engine stops writing ContainerEngineReady after watcher teardown" contract that would otherwise deadlock a stopped app.
- **Pure-function condition computation with broad unit coverage** [Claude Opus 4.7]. `computeSettledCondition` is a pure function over the App's status plus one bool; `app_controller_test.go` covers each branch with 11 cases. This is the right shape for the logic and the right way to test it.

---

## Testing Assessment

Unit coverage of `computeSettledCondition` is thorough. Integration gaps, ranked by risk:

1. **No integration test for `rdd set containerEngine.name=…` while the VM is running.** The new Settled semantics explicitly target this case, but `engine-docker.bats:534-559` starts from a stopped VM, sidestepping the I1 race. An end-to-end test that sets the backend on a running VM and asserts that `rdd set` returns only after the restart has actually completed would both validate the PR and surface the race.
2. **No integration test exercises `EngineEnabled=false` against a real control plane.** The Windows / `--controllers=-engine` branch is unit-tested, but no integration test runs against a service started with `--controllers=-engine` to exercise the full `base.IsControllerEnabled("engine")` discovery path.
3. **No test verifying Settled re-fires on engine-only condition changes.** The App reconciler must re-stamp Settled when engine writes `ContainerEngineReady`. `ctrl.NewControllerManagedBy(mgr).For(&v1alpha1.App{})` lacks an explicit predicate, so it relies on controller-runtime's default of firing on any event. A test that writes `ContainerEngineReady` at a new generation and asserts `Settled.ObservedGeneration` catches up would guard against a `GenerationChangedPredicate` creeping in and silently breaking this PR's contract.
4. **No coverage for split-manager / late external-registration topologies** [Codex GPT 5.4]. Even though no standalone binary ships engine today, `docs/design/discovery.md:56-64` documents support for external controllers attaching after startup. Without a test, the I2 latent regression cannot fail closed.

`go test ./pkg/controllers/app/app/controllers` passes locally [Codex GPT 5.4].

---

## Documentation Assessment

- `docs/design/api_app.md:97-119` and `docs/design/cmd_app.md:13-17` now promise stronger semantics than the code provides: every property change waits until the full reconcile chain has caught up, but I1 and I2 show that is not yet true on the template-update path or in split-manager deployments. The doc should either narrow the contract or note the known race until I1 lands.
- `docs/design/api_app.md:97` does not mention that `Settled` is the single signal `rdd set` waits on; a reader has to cross-reference `cmd_app.md` to infer this.
- `docs/design/cmd_app.md` loses the previous documentation about what "stops waiting" meant for each property class. A sentence clarifying that the wait now covers backend swaps (the previously-open TODO) would make the contract change explicit.
- See S2 for the Settled-reason table gap.

---

## Commit Structure

Clean. Single commit, message accurately describes the change, stacks correctly on #316 and #309 as advertised. The PR title and body need updating per I3 to match what was implemented.

---

## Acknowledged Limitations

- **`app_controller.go:297-300`** — comment on the template-CM update path acknowledges the race in I1. The author has flagged it; the PR does not attempt to close the window. Reviewers note the comment understates user impact: the CLI can return *during* the stale window, not merely transit through Settled=True briefly.
- **`set.bats:10-15`** — `--wait=false` on every `rdd set` because the suite runs with only the app controller registered, so Settled is never written. Acknowledged and consistent with routing meaningful wait coverage through `engine-docker.bats`.
- **PR body** — author notes Windows / `EngineEnabled=false` is unit-tested but not exercised end-to-end on Windows; trusting CI. The `engineEnabled=false` branch in the unit test matrix is the path Windows takes.

---

## Unresolved Feedback

PR description has no review comments listed; no inline reviewer feedback to reconcile.

---

## Agent Performance Retro

### [Claude]

Best executive-summary calibration of the three: caught that the PR title/body promise `Ready` while only `Settled` was implemented (I3), which Gemini swallowed and Codex only mentioned in passing. Strongest on local code quality (S1 hardcoded "engine" string, S2 docs table, S3 string-suffix matching) — these are the kind of fix-now-cheaply suggestions that compound over a codebase. Treated the template-CM race as a future design observation rather than promoting it; with hindsight, "Important regression" is the right calibration since the PR's stated purpose is the backend-swap wait. One miss: did not raise the process-local `EngineEnabled` concern that both Codex and Gemini caught.

### [Codex]

Tied with Gemini on identifying the two structural issues (template-CM race, process-local `EngineEnabled`), and gave the cleanest write-up of both — including the right callouts to `cmd/containers-controller/main.go` and `docs/design/discovery.md`. Correctly classified the template race as Important regression rather than Critical, which matched the merged severity. Mentioned the missing `Ready` once in the Documentation Assessment but did not promote it to a finding. Ran `go test` for live verification, which strengthened the testing-assessment section.

### [Gemini]

Found the same two structural issues as Codex (race condition, process-local EngineEnabled), with independent diagnoses. Promoted the template-CM race to Critical, which is defensible given the PR's stated goal but harsher than the author's acknowledged-limitation framing warrants. Two miscalibrations: (a) its I2 (engine missing from `cmd/app-controller/main.go`) is misdirected — engine's API group is `containers`, not `app`, so adding it to `cmd/app-controller` would panic at startup via the API-group check in `external/main.go:65-68`; the underlying observation about engine missing from `cmd/containers-controller` is true but pre-existing. (b) Took the PR description at face value and described the PR as "introduces the Ready and Settled conditions" in its executive summary, missing the Ready-vs-Settled discrepancy that Claude caught. As expected per the repo memory, did not run `git blame`.

### Summary

| | Claude Opus 4.7 | Codex GPT 5.4 | Gemini 3.1 Pro |
|---|---|---|---|
| Duration | 9m 15s | 6m 37s | 5m 22s |
| Findings | 1I 3S | 2I | 2I |
| Tool calls | 46 (Read 23, Grep 15, Bash 8) | 64 (shell 61, plan 2, stdin 1) | 22 (grep_search 12, read_file 7, run_shell_command 3) |
| Design observations | 5 | 3 | 2 |
| False positives | 0 | 0 | 1 |
| Unique insights | 5 | 1 | 0 |
| Files reviewed | 12 | 12 | 12 |
| Coverage misses | 1 | 1 | 1 |
| **Totals** | **1I 3S** | **2I** | **2I** |
| Downgraded | 0 | 0 | 1 (I→S) |
| Dropped | 0 | 0 | 1 |


Claude and Codex both delivered actionable reports; Codex's findings were the most precisely scoped and Claude's local-code suggestions added the most readability/maintainability value. Gemini's two structural findings overlap fully with Codex's, and its misdirected I2 plus its missed I3 reduce its marginal contribution on this PR. For a single-agent review of this kind of change (controller boundaries + condition contract), Codex would have been sufficient; Claude added meaningful local-quality suggestions that Codex missed.

---

## Review Process Notes

### Repo context updates

- **REMOVE** the rule "When reviewing a new controller under `pkg/controllers/`, verify its `GetAPIGroup()` matches the grouping convention of sibling controllers and that the `--controllers` help text in `pkg/service/controllers/options.go` lists the group" — currently in `deep-review-context.md`. The point is correct in principle but agents have started over-applying it: in this round, Gemini used it to recommend adding the engine controller to `cmd/app-controller/main.go`, which would actually panic at startup because engine's API group is `containers`. Replace with a tighter rule: "When checking that every blank-imported controller appears in the matching standalone binary, verify the controller's `GetAPIGroup()` matches the binary's group argument first — `external.RunControllers` panics on a group mismatch." This keeps the import-coverage check while preventing the wrong-binary fix recommendation.

- **ADD** a rule about cross-controller convergence signals: "When a controller publishes an aggregate condition that depends on another controller having reacted to a recent change (`Settled`, `Ready`, `Reconciled`), verify the aggregate is gated on a signal the dependent controller writes — not on data the aggregating controller computes from its own observation. A reconciler-restamped mirror of someone else's status is not a convergence signal: between (a) the change being requested and (b) the dependent controller observing it, the aggregator can produce a fresh `True` from stale data." This codifies the I1 pattern; agents should look for it whenever a PR adds a new aggregate condition.

- **ADD** a rule about process-local registry queries in cross-controller logic: "Flag `base.IsControllerEnabled(...)` (or any `init()`-time registry query) used to gate a cluster-wide contract. The result is process-local — it answers 'is this controller in my OS process,' not 'is this controller running anywhere in the cluster.' Cluster-wide questions belong on the discovery ConfigMap. Process-local checks are appropriate only for in-process plumbing (e.g., wiring a watch or skipping a setup step)." Two of three agents flagged this in the current PR; making the heuristic explicit in the repo context will make future flags consistent.

### Skill improvements

None this round.

---

## Appendix: Original Reviews

### [Claude] — Pass 1

# Code Review — PR #315 (app: aggregate Ready and Settled conditions, wait for Settled in `rdd set`)

## Executive Summary

The PR adds an `AppConditionSettled` condition, computed by the App reconciler as an aggregate of the mirrored LimaVM `Running` condition and the engine-written `ContainerEngineReady`, and switches `rdd set` to wait on it for every property change. The implementation is cohesive and well tested at the unit level, but the PR title, description, and an in-code comment all reference a `Ready` condition that was never implemented, and a known race between the template-CM update and the LimaVM restart is left as a documented limitation that can let `rdd set containerEngine.name=…` return prematurely.

## Findings

### Critical Issues

None.

### Important Issues

**I1. Settled description, PR title, and in-code comment claim a `Ready` condition that doesn't exist** — `pkg/apis/app/v1alpha1/app_types.go:14-39`, `pkg/controllers/app/app/controllers/app_controller.go:314`, PR title/body

```go
// pkg/controllers/app/app/controllers/app_controller.go:314
// Mirror LimaVM status conditions and aggregate Ready/Settled. The
// engine reconciler writes ContainerEngineReady on the same object,
```

The PR title is "app: aggregate Ready and Settled conditions, wait for Settled in `rdd set`", and the PR body contains a two-paragraph "Ready" section explaining that `Ready` answers "is the app operational now?". But `pkg/apis/app/v1alpha1/app_types.go` only defines `AppConditionSettled` (line 38); there is no `AppConditionReady`, nothing in `computeSettledCondition` (lines 370-424 of `app_controller.go`) produces a `Ready` entry, and `pkg/controllers/app/app/controllers/app_controller_test.go` only asserts the Settled path. The stale comment at `app_controller.go:314` ("aggregate Ready/Settled") was introduced by this commit (27d7dd3) — `git blame` shows the entire block is authored by this PR — and will confuse future readers into looking for a Ready aggregator. This also leaves the reviewer unable to confirm whether the dropped `Ready` was an intentional scope reduction or an implementation oversight.

Fix: decide whether `Ready` is in scope. If not, update the PR title/body and correct the comment:

```diff
-	// Mirror LimaVM status conditions and aggregate Ready/Settled. The
-	// engine reconciler writes ContainerEngineReady on the same object,
+	// Mirror LimaVM status conditions and compute Settled. The
+	// engine reconciler writes ContainerEngineReady on the same object,
```

If `Ready` is still intended, add the constant in `app_types.go`, compute it alongside Settled, and add test cases. (important, regression)

### Suggestions

**S1. Hardcoded `"engine"` controller name creates silent coupling** — `pkg/controllers/app/app/controller.go:153`

```go
EngineEnabled:    base.IsControllerEnabled("engine"),
```

The App controller queries the engine controller by its literal name. `engine`'s own name constant — `controllerName` at `pkg/controllers/app/engine/controller.go:33` — is unexported, so a rename on the engine side would silently turn `EngineEnabled` into a permanent `false`, regressing `Settled` to the pre-engine path with no build or test failure. The hardcoded string is also repeated verbatim in the doc comment on `app_controller.go:53` and in `service.go:771`.

Fix: export `engine.ControllerName` (rename to `ControllerName` in `pkg/controllers/app/engine/controller.go`) and reference it here. Note that importing `pkg/controllers/app/engine` from `pkg/controllers/app/app` would trigger engine's `init()` registration in the app-controller binary, which is undesirable — so either (a) move the constant to a small shared package (e.g., `pkg/apis/app/v1alpha1` alongside `AppConditionContainerEngineReady`), or (b) keep the literal and add a compile-time check in a test that imports engine and asserts the literal matches `engine.controllerName` via a deliberately-shared constant. A shared constant in `v1alpha1` is the cleanest option because it already holds `AppConditionContainerEngineReady`:

```go
// pkg/apis/app/v1alpha1/app_types.go
// EngineControllerName is the registry name of the engine controller.
const EngineControllerName = "engine"
```

Then `EngineEnabled: base.IsControllerEnabled(v1alpha1.EngineControllerName)`, and the engine controller uses the same constant internally. (suggestion, gap)

**S2. `Settled` reason docs table says "*(varies)*" but reasons are well-defined** — `docs/design/api_app.md:115`

```markdown
| `Settled`              | `False`   | *(varies)*       | Reconciliation in progress; reason mirrors the blocking condition |
```

`computeSettledCondition` emits a small, enumerable set of fixed Settled reasons — `WaitingForLimaVM`, `WaitingForEngine`, `EngineStale`, and the forwarded LimaVM Running or engine `ContainerEngineReady` reason. Documenting them lets consumers (GUI, tests, other controllers) branch on specific values without reading the source. Fix: expand the row into explicit rows for `WaitingForLimaVM`, `WaitingForEngine`, `EngineStale`, and a catch-all "forwarded from Running/ContainerEngineReady" row. (suggestion, gap)

**S3. `runningLimaVMMessage` uses string-suffix matching for failure detection** — `pkg/controllers/app/app/controllers/app_controller.go:430-435`

```go
func runningLimaVMMessage(runningCond *metav1.Condition, desired string) string {
	if strings.HasSuffix(runningCond.Reason, "Failed") && runningCond.Message != "" {
		return runningCond.Message
	}
	return "LimaVM has not yet reached " + desired
}
```

The heuristic couples Settled's behaviour to a LimaVM naming convention. Any new failure reason that doesn't end in `Failed` would silently lose its diagnostic message to the generic fallback, and a non-failure reason ending in `Failed` (none today, but nothing prevents one) would leak a confusing operational message into Settled. Fix: switch on the known failure reasons explicitly or check `runningCond.Status == metav1.ConditionFalse && runningCond.Message != ""`:

```diff
-	if strings.HasSuffix(runningCond.Reason, "Failed") && runningCond.Message != "" {
-		return runningCond.Message
-	}
-	return "LimaVM has not yet reached " + desired
+	switch runningCond.Reason {
+	case ReasonStartFailed, ReasonStopFailed:
+		if runningCond.Message != "" {
+			return runningCond.Message
+		}
+	}
+	return "LimaVM has not yet reached " + desired
```

(The LimaVM reason constants would need to be exported or mirrored to avoid a cross-package import cycle — referencing them via string constants on the App side is acceptable if a unit test pins them to the LimaVM values.) (suggestion, enhancement)

## Design Observations

**Concerns**

- **Template-CM update race undermines the Settled contract for backend-only changes** *(future)* — `app_controller.go:297-301`. The `rdd set containerEngine.name=containerd` path relies on the sequence (a) App patches the template CM → (b) LimaVM observes the change and sets `restartNeeded=true` → (c) LimaVM stops the VM, setting `Running=False/Stopped`. Between (a) and (b) the App reconciler can run, mirror LimaVM's still-`Started` condition into the App, compute `Settled=True` at the new generation, and satisfy the CLI's wait. The author's comment at `app_controller.go:297-300` acknowledges this as a known race, but it defeats the PR's stated motivation for backend-only changes. A tighter design would hold Settled False until LimaVM's `Status.ObservedTemplateResourceVersion` catches up to the patched CM's `resourceVersion`: the App reconciler already fetches both objects, so the comparison is local. This would make Settled a true "reconcile-chain caught up" signal without the time-of-check window the current implementation leaves open. Flagged as `(future)` since the comment admits the limitation, but the comment understates the user impact — the wait can return during the stale window, not just transit through `True` and flip back.

- **`Settled` absence when the App reconciler is not running hangs `rdd set` for the full timeout** *(future)* — `cmd/rdd/set.go:477-487`. If the App controller is excluded by `--controllers=-app` or fails to start, no one writes Settled and `rdd set` blocks until the 5-minute default timeout expires. `set.bats` papers over this by passing `--wait=false` everywhere (lines 84, 99, 114, 134, 159), but a real deployment that disabled the app controller would see the same hang. The CLI could check for the app controller's presence via the discovery ConfigMap (the same data path the pre-PR `isEngineControllerEnabled` used) and degrade gracefully; the infrastructure is still in `pkg/service/controllers/discovery.go`.

**Strengths**

- Short-circuiting `!desiredRunning` when `runningCond.Reason == "Stopped"` (`app_controller.go:397-405`) correctly handles the engine's "stops writing ContainerEngineReady after teardown" contract, which would otherwise deadlock a stopped app.
- The `SetEnabledControllers` → `RegisterWithManager` ordering in both `pkg/service/cmd/service.go` (lines 772, 811) and `pkg/external/main.go` (lines 92, 121) is correctly sequenced: the shared set is populated before any controller calls `IsControllerEnabled`.
- `computeSettledCondition` is a pure function over the App's status plus one bool; the unit test at `pkg/controllers/app/app/controllers/app_controller_test.go` covers each branch, which is the right shape for this logic.

## Testing Assessment

Unit coverage of `computeSettledCondition` is good — 11 cases spanning engine-enabled/disabled, stale generation, missing conditions, transient and terminal states. Gaps, ranked by risk:

1. **No integration test for the containerEngine-only change wait** — the new Settled semantics explicitly target `rdd set containerEngine.name=containerd` returning only after the restart, but `bats/tests/32-app-controllers/engine-docker.bats:534-559` starts from a stopped VM, avoiding the restart path. An end-to-end test that sets backend while running and asserts `Settled=True` only after the VM has actually restarted would both validate the PR and surface the race identified above.
2. **No test for `EngineEnabled=false` against a truly-running control plane** — the Windows/`--controllers=-engine` branch of `computeSettledCondition` is unit-tested, but no integration test runs against a service started with `--controllers=-engine` to exercise the full `base.IsControllerEnabled("engine")` discovery path.
3. **No test verifying Settled generation advances after engine-only condition changes** — the App reconciler needs to fire on engine's `ContainerEngineReady` writes to re-stamp Settled. `ctrl.NewControllerManagedBy(mgr).For(&v1alpha1.App{})` lacks an explicit predicate, so it relies on the controller-runtime default of firing on any event. A test that writes `ContainerEngineReady` at a new generation and asserts `Settled.ObservedGeneration` catches up would guard against a future `GenerationChangedPredicate` creeping in and silently breaking this PR's contract.

## Documentation Assessment

- `docs/design/api_app.md:97` updates the multi-writer description but does not mention that `Settled` is the single signal `rdd set` now waits on; a reader comparing the doc to `cmd_app.md` has to cross-reference to infer this.
- `docs/design/cmd_app.md:13-17` is clear on the new wait semantics but loses the helpful old documentation about what "stops waiting" means for each property class. A sentence clarifying that the wait covers backend swaps (previously the open TODO) would make the contract-change more explicit.
- See S2 for the docs table gap.

## Commit Structure

Clean. Single commit, message accurately describes the change, stacks correctly on #316 and #309 as advertised.

## Acknowledged Limitations

- **`app_controller.go:297-300`** — comment on the template-CM update path:
  > A racing `rdd set` may see Settled=True at the new generation before LimaVM observes the ConfigMap change. LimaVM then sets restartNeeded on its next reconcile, flipping Settled back to False.

  This is the race discussed under Design Observations. The author has flagged it; the current PR does not attempt to close the window. The comment understates user impact — the CLI can return during the stale window, not merely pass through Settled=True briefly — but the phenomenon itself is acknowledged.

- **`set.bats:10-15`** — `--wait=false` on every `rdd set` because the suite runs with only the app controller registered, so Settled is never written. Acknowledged and consistent with the PR's decision to route meaningful wait coverage through `engine-docker.bats`.

- **PR body** — author states:
  > Windows — `EngineEnabled=false` keeps Settled working without the engine controller. Not run locally.

  Untested on Windows; trusting CI. The unit test at `app_controller_test.go:86-95` exercises the `engineEnabled=false` branch, which is the code the Windows path takes.

## Unresolved Feedback

None. PR description has no review comments listed.

## Coverage Summary

| File | Status |
|---|---|
| `bats/tests/32-app-controllers/engine-docker.bats` | Reviewed, no issues (comment-only updates matching new semantics) |
| `bats/tests/32-app-controllers/set.bats` | Reviewed, no issues (acknowledged via the file's own comment) |
| `cmd/rdd/set.go` | Reviewed, no issues |
| `docs/design/api_app.md` | S2 |
| `docs/design/cmd_app.md` | Reviewed, noted under Documentation Assessment |
| `pkg/apis/app/v1alpha1/app_types.go` | I1 |
| `pkg/controllers/app/app/controller.go` | S1 |
| `pkg/controllers/app/app/controllers/app_controller.go` | I1, S3 |
| `pkg/controllers/app/app/controllers/app_controller_test.go` | Reviewed, no issues |
| `pkg/controllers/base/controller.go` | Reviewed, no issues |
| `pkg/external/main.go` | Reviewed, no issues |
| `pkg/service/cmd/service.go` | Reviewed, no issues |

### [Codex] — Pass 1

### Executive Summary

This change replaces the old property-specific `rdd set` wait logic with a single App-level `Settled` condition and threads an `EngineEnabled` hint into the App reconciler. The overall direction is sound, but two important regressions remain: `Settled` ignores engine controllers that run in a different controller manager, and it can still go `True` before Lima has observed a template-driven spec change, so `rdd set` can return early for the restart-inducing updates this PR is meant to cover.

### Findings

**Critical Issues**

None.

**Important Issues**

I1. **Process-local `EngineEnabled` ignores external engine managers** — `pkg/controllers/app/app/controller.go:149-154`, `pkg/controllers/base/controller.go:122-157` (important, regression)

```go
if err := (&controllers.AppReconciler{
    Client:           mgr.GetClient(),
    Scheme:           mgr.GetScheme(),
    LimaTemplateData: limaTemplateData(),
    EngineEnabled:    base.IsControllerEnabled("engine"),
}).SetupWithManager(mgr); err != nil {
```

`pkg/controllers/app/app/controller.go:153` now freezes `EngineEnabled` from `base.IsControllerEnabled("engine")`, but `pkg/controllers/base/controller.go:122-157` explicitly defines that API as “which controllers started in this process.” The only writers are `pkg/service/cmd/service.go:761-772` and `pkg/external/main.go:86-92`, which populate the set from each manager’s local controller list. That is not equivalent to “is an engine controller running anywhere”: the repo ships separate `app-controller` and `containers-controller` binaries at `cmd/app-controller/main.go:5-20` and `cmd/containers-controller/main.go:5-17`, and `docs/design/discovery.md:56-64` says external controllers can attach later. In those supported topologies, `computeSettledCondition()` in `pkg/controllers/app/app/controllers/app_controller.go:393-405` will ignore `ContainerEngineReady`, and `cmd/rdd/set.go:477-486` now trusts only `Settled`, so `rdd set` can return before an external engine controller has connected and stamped the current generation. This also regresses the old CLI-side discovery behavior that queried all controller managers before deciding whether to wait on the engine.

Fix: derive engine availability from the shared discovery ConfigMap across all controller managers, not from a per-process registry snapshot. The decision also needs to be made during reconciliation, or from a globally maintained status field, so late-starting external managers can start influencing `Settled` without restarting the App controller.

I2. **`Settled` can still flip true before Lima applies template-driven changes** — `pkg/controllers/app/app/controllers/app_controller.go:291-339`, `cmd/rdd/set.go:467-486` (important, regression)

```go
if templateCM.Data[limav1alpha1.TemplateConfigMapKey] != desired {
    ...
    return ctrl.Result{Requeue: true}, nil
}
...
changed = apimeta.SetStatusCondition(&latest.Status.Conditions, metav1.Condition{
    Type:               cond.Type,
    Status:             cond.Status,
    Reason:             cond.Reason,
    Message:            msg,
    ObservedGeneration: latest.Generation,
}) || changed
```

After updating the Lima template `ConfigMap` at `pkg/controllers/app/app/controllers/app_controller.go:282-301`, the App reconciler immediately requeues itself, but it does not wait for the Lima controller’s `Owns(&corev1.ConfigMap{})` watch at `pkg/controllers/lima/limavm/controllers/limavm_controller.go:480-487` to run. On that next App reconcile, the status-mirroring block at `pkg/controllers/app/app/controllers/app_controller.go:320-339` copies the old Lima `Running` condition forward and restamps it with `ObservedGeneration: latest.Generation` at line 334, even though Lima has not yet processed the new template. `computeSettledCondition()` at `pkg/controllers/app/app/controllers/app_controller.go:380-423` therefore sees a current-generation `Running=Started`, and `cmd/rdd/set.go:477-486` returns as soon as the engine condition is current, while the Lima controller only notices the drift later in `pkg/controllers/lima/limavm/controllers/limavm_controller.go:264-270` and `:396-423` when it sets `status.restartNeeded` and rewrites the on-disk template. The new comment at `pkg/controllers/app/app/controllers/app_controller.go:297-300` acknowledges the race, but because this PR made `Settled` the only wait predicate, that race now recreates the exact early-return behavior the PR is supposed to remove for `containerEngine.name` / `kubernetes.*` updates on a running VM.

Fix: make `Settled` depend on a Lima-owned convergence signal instead of a restamped `Running` mirror. Concretely, the App controller should not advance `Settled` after a template change until Lima has observed the new template resource version and cleared any pending restart, or Lima should publish a dedicated “reconciled with current template” condition that the App controller aggregates.

**Suggestions**

None.

### Design Observations

**Concerns**

1. **`Settled` is inferring downstream convergence from mirrored status instead of controller-owned convergence signals** `(in-scope)`

The current design lets the App controller guess whether Lima and engine are “caught up” by mirroring their status into App and then combining that with a process-topology hint. A stronger structure would have Lima and engine each publish an explicit condition like “observed App generation X and finished applying it,” with App only aggregating those conditions. That removes the split-manager dependency in I1 and the stale-restamp race in I2, and it gives `rdd set` a true end-to-end synchronization contract instead of a heuristic.

**Strengths**

1. Using `ObservedGeneration` as part of the wait contract is the right primitive. Once the writers stamp the right generation at the right time, it cleanly prevents stale success from a previous spec.

2. The multi-writer App status updates in `pkg/controllers/app/app/controllers/app_controller.go:320-344` continue to use `retry.RetryOnConflict` plus re-`Get`, which is consistent with the existing engine-controller pattern and avoids reintroducing the old 409 loop.

### Testing Assessment

I ran `go test ./pkg/controllers/app/app/controllers`; the new unit test passes. The highest-risk gaps are still:

1. No integration test exercises `rdd set` against a running VM for a template-driven change (`kubernetes.enabled`, `kubernetes.version`, or `containerEngine.name`) and verifies the command does not return until the restart has actually been observed and completed.

2. No test covers split-manager deployments where `app-controller` and `containers-controller` run in separate processes, even though the new `EngineEnabled` behavior depends on that topology.

3. No test covers late external controller registration after the App controller has already started, which is explicitly supported by `docs/design/discovery.md:56-64`.

### Documentation Assessment

`docs/design/api_app.md:97-119` and `docs/design/cmd_app.md:13-17` now promise stronger semantics than the code currently provides: they say every property change waits until the full reconcile chain has caught up, but I1 and I2 show that is not yet true in split-manager and template-update paths. Also, the comment at `pkg/controllers/app/app/controllers/app_controller.go:314` says the reconciler “aggregate[s] Ready/Settled,” but this change only computes `Settled`; that comment should either be corrected or the missing `Ready` work should land before merge.

### Acknowledged Limitations

- **PR comment**: On stacked PR #309, the author said they did not plan to address prior AI-review findings unless convinced otherwise. This PR is layered on that stack; the findings above are distinct regressions introduced by `27d7dd3`, not a restatement of earlier dismissed items.

- **PR comment**: On stacked PR #316, the author declined adding CLI support for container actions, with the rationale that the feature exists for the GUI and terminal users should use `docker` / `nerdctl` directly. This PR does not change that scope decision.

- **PR comment**: On stacked PR #316, the author said they had already dropped the Docker call table and improved the error message and did not want to do more. This PR stays within that narrowed container-action scope.

- **PR comment**: The author noted a Windows `bats-lima` failure that looked like an `nxadm/tail` rotation flake and deferred it to separate investigation. This PR does not touch that path.

### Coverage Summary

- `bats/tests/32-app-controllers/engine-docker.bats` — Reviewed, no issues
- `bats/tests/32-app-controllers/set.bats` — Reviewed, no issues
- `cmd/rdd/set.go` — I1, I2
- `docs/design/api_app.md` — I1, I2
- `docs/design/cmd_app.md` — I1, I2
- `pkg/apis/app/v1alpha1/app_types.go` — Reviewed, no issues
- `pkg/controllers/app/app/controller.go` — I1
- `pkg/controllers/app/app/controllers/app_controller.go` — I1, I2
- `pkg/controllers/app/app/controllers/app_controller_test.go` — Reviewed, no issues
- `pkg/controllers/base/controller.go` — I1
- `pkg/external/main.go` — I1
- `pkg/service/cmd/service.go` — I1


### [Gemini] — Pass 1

### Executive Summary

This PR introduces the `Ready` and `Settled` conditions on the `App` resource to provide a single, robust synchronization point for `rdd set`, aiming to fix a bug where config-only changes would exit before the Lima VM restarted. However, a critical race condition completely defeats this goal by evaluating `Settled=True` against stale LimaVM state before the VM even begins restarting. Additionally, checking for the `engine` controller via process-local registry state breaks distributed setups and highlights a missing import in the standalone test binaries.

### Findings

#### Critical Issues

C1. **Config-change synchronizations are broken by a race condition that prematurely sets `Settled=True`** — `pkg/controllers/app/app/controllers/app_controller.go:293-302`, `314-340` (critical, regression)

```go
				// ConfigMaps are not watched, so requeue to let the
				// reconciler evaluate remaining spec fields (e.g. running).
				// A racing `rdd set` may see Settled=True at the new
				// generation before LimaVM observes the ConfigMap change.
				// LimaVM then sets restartNeeded on its next reconcile,
				// flipping Settled back to False.
				return ctrl.Result{Requeue: true}, nil
```

The stated goal of the PR is to use `Settled` as a synchronization point so `rdd set containerEngine.name=containerd` does not "walk away before the VM had restarted." However, when `templateCM` is updated, the AppReconciler requeues and on its next pass copies `limaVM`'s *stale* `Running=Started` status (from before the config change), artificially stamps it with the new `App.Generation`, and calculates `Settled=True`. `rdd set` then sees this new generation and exits immediately before the VM has restarted.

Although a comment acknowledges this race, it mischaracterizes the impact: the race entirely defeats the primary objective of this PR for config-only changes. `rdd set` still walks away prematurely.

Fix: Since `AppReconciler` cannot independently verify that `LimaVM` has reconciled the new ConfigMap, fixing this requires a cross-resource handshake. Add a field to `LimaVMStatus` (e.g., `ObservedTemplateResourceVersion`) that the LimaVM controller updates when it reconciles `templateCM`. Then, `AppReconciler` must gate `Settled=True` on `limaVM.Status.ObservedTemplateResourceVersion == templateCM.ResourceVersion`. In the interim, to prevent the CLI from exiting, set `Settled=False` with `Reason="RestartNeeded"` explicitly during the `templateCM` update pass. Note: to prevent the next pass from incorrectly flipping it back to `True` before `LimaVM` sets `RestartNeeded`, you must track pending config changes explicitly via annotations on the App.

#### Important Issues

I1. **Process-local `IsControllerEnabled` breaks `Settled` evaluation in standalone controller modes** — `pkg/controllers/app/app/controller.go:153` (important, gap)

```go
	if err := (&controllers.AppReconciler{
		Client:           mgr.GetClient(),
		Scheme:           mgr.GetScheme(),
		LimaTemplateData: limaTemplateData(),
		EngineEnabled:    base.IsControllerEnabled("engine"),
	}).SetupWithManager(mgr); err != nil {
```

`AppReconciler` hardcodes its `EngineEnabled` state at startup using `base.IsControllerEnabled("engine")`. As documented in `pkg/controllers/base/controller.go`, this function only checks if the controller was started *in the exact same OS process*. If the `app` and `engine` controllers are run as separate standalone processes (e.g., `bin/app-controller` and a hypothetical `bin/engine-controller`), `AppReconciler` will incorrectly assume the engine is disabled and calculate `Settled=True` without waiting for `ContainerEngineReady`. This violates the loosely-coupled design of Kubernetes controllers and breaks client synchronization.

Fix: Instead of baking process-local state into the reconciler at initialization, retrieve the running controllers dynamically using the cluster-wide discovery ConfigMap `controllers.NewControllerManagerDiscovery(mgr.GetConfig()).GetEnabledControllers(ctx)` (the approach previously used by `rdd set`).

I2. **`engine` controller missing from standalone `app-controller` binary** — `cmd/app-controller/main.go:12-15` (important, gap)

```go
	// Import app controller packages to trigger init() functions.
	_ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/app/app"
	_ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/app/demo"
```

Following the repository's rules ("A missing import in either entrypoint is a regression"), the `engine` controller was added to the `app` API group in PR #309 and correctly imported in the embedded mode (`pkg/service/cmd/service.go:59`), but the blank import was never added to the standalone `cmd/app-controller/main.go`. Without this import, the `engine` controller cannot be run in the external test/development mode for the `app` group.

Fix: Add the missing blank import:
```go
	_ "github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/app/engine"
```

#### Suggestions

None.

### Design Observations

#### Concerns

**Process-local Registration Couples Independent Controllers**: `AppReconciler`'s reliance on `base.SetEnabledControllers` couples the controller logic to process topologies. This works in an embedded mono-binary context but violates the K8s decoupled design principles, explicitly breaking distributed development/testing modes. Relying exclusively on K8s primitives (e.g. discovery ConfigMaps or directly watching `engine` endpoints) eliminates a class of topology-related bugs. (in-scope)

#### Strengths

**Staleness Management via Conflict Retries**: Excellent use of `retry.RetryOnConflict` with a `re-Get` when mirroring `LimaVM` status in `AppReconciler`. This ensures concurrent status writers (such as `EngineReconciler` writing `ContainerEngineReady`) do not fall into aggressive 409 requeue loops.

### Testing Assessment

1. `rdd set` on config-only changes while the VM is running (`rdd set containerEngine.name=containerd`) is completely untested. Current tests safely bypass the race condition raised in C1 by changing `running=true/false` alongside the config (e.g. `rdd set containerEngine.name=containerd running=true` implicitly gates the wait on a successful `Running=True/Started` condition).

### Documentation Assessment

The `docs/design/cmd_app.md` currently documents that `Settled` goes `False` on generation updates. This accurately reflects intent, but contradicts the active behavior caused by the C1 race condition.

### Commit Structure

No issues. The PR is cleanly structured with relevant commits.

### Acknowledged Limitations

- **Code comment**: `pkg/controllers/app/app/controllers/app_controller.go:293-296` acknowledges that "A racing rdd set may see Settled=True at the new generation before LimaVM observes the ConfigMap change". *Note: Promoted to finding C1 because this race entirely breaks the synchronization feature the PR was intended to add.*
- **PR comment**: Author explicitly declined adding CLI support for setting container actions. Changes are consistent with this rationale.

### Coverage Summary

- `bats/tests/32-app-controllers/engine-docker.bats`: Reviewed, no issues
- `bats/tests/32-app-controllers/set.bats`: Reviewed, no issues
- `cmd/rdd/set.go`: Reviewed, no issues
- `docs/design/api_app.md`: Reviewed, no issues
- `docs/design/cmd_app.md`: Reviewed, no issues
- `pkg/apis/app/v1alpha1/app_types.go`: Reviewed, no issues
- `pkg/controllers/app/app/controller.go`: I1
- `pkg/controllers/app/app/controllers/app_controller.go`: C1
- `pkg/controllers/app/app/controllers/app_controller_test.go`: Reviewed, no issues
- `pkg/controllers/base/controller.go`: I1
- `pkg/external/main.go`: I1
- `pkg/service/cmd/service.go`: I1

## Resolution

| | |
|---|---|
| **Addressed** | 2026-04-17 |
| **Commit** | `02efd32 app: address PR review feedback on Settled` |

| # | Finding | Action |
|---|---------|--------|
| 1 | Important #1: Template-CM update race lets `rdd set containerEngine.name=…` return before the VM restarts | Skipped (deferred to followup PR; LimaVM will publish its own `Settled` condition that App mirrors) |
| 2 | Important #2: `EngineEnabled` captured from process-local registry at startup | Fixed (App reconciler now queries `ControllerManagerDiscovery` per reconcile; `IsControllerEnabled` renamed to `IsControllerInProcess` to make the per-process semantics explicit) |
| 3 | Important #3: PR title, body, and a new in-code comment claim a `Ready` condition that does not exist | Fixed in code (comment changed to "compute Settled"); PR title/body update on GitHub still owed |
| 4 | Suggestion #1: Hardcoded `"engine"` controller name | Fixed (`EngineControllerName` const in `pkg/apis/app/v1alpha1`, referenced from both engine controller and app reconciler) |
| 5 | Suggestion #2: Settled `Reason` table says "*(varies)*" | Documentation updated (table expanded to enumerate `WaitingForLimaVM`, `WaitingForEngine`, `EngineStale`, and forwarded reasons) |
| 6 | Suggestion #3: `runningLimaVMMessage` uses string-suffix matching | Skipped (LimaVM-Settled followup will rework the message logic) |
| 7 | Testing Gap #1: No integration test for `rdd set containerEngine.name=…` while VM running | Skipped (deferred with I1) |
| 8 | Testing Gap #2: No integration test exercises `EngineEnabled=false` against a real control plane | Skipped (deferred with I2 followup work) |
| 9 | Testing Gap #3: No test verifying Settled re-fires on engine-only condition changes | Skipped (deferred) |
| 10 | Testing Gap #4: No coverage for split-manager / late external-registration topologies | Skipped (engine not shipped in any standalone binary today; followup) |
| 11 | Documentation Gap #1: api_app.md doesn't mention Settled is the single signal `rdd set` waits on | Documentation updated (status-conditions paragraph rewritten; `cmd_app.md` already covers the per-property change scope) |
| 12 | Documentation Gap #2: cmd_app.md lost per-property wait docs | Skipped (already covered in current `cmd_app.md` line 13 — every property change waits on Settled) |