# Deep Review: 20260325-141549-pr-248

| | |
|---|---|
| **Date** | 2026-03-25 14:15 |
| **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** | [#248](https://github.com/rancher-sandbox/rancher-desktop-daemon/pull/248) — App controller Stop and Start LimaVM |
| **Branch** | `app-controller-stop-start-limavm` |
| **Commits** | `a53ebd0` Update api_app.md to reflect condition definition<br>`1101535` Add additional BATs for App Controller<br>`ae6bc38` Fix ObservedGeneration in App Controller |
| **Reviewers** | Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro |
| **Verdict** | **Merge with fixes** — two issues need attention: the BATS mirror assertions race the App reconcile, and `ObservedGeneration` copies the child's generation instead of the parent's |
| **Wall-clock time** | `12 min 44 s` |

---

## Executive Summary

This PR enables the App controller to start and stop its owned LimaVM by propagating `spec.running`, mirroring the LimaVM's status conditions onto the App, and upgrading two error paths from log-and-continue to proper error returns. It also rewrites `api_app.md` to document the mirroring semantics and adds BATS tests for the Running condition lifecycle. Two issues block a clean merge: the new BATS assertions read the App object without waiting for the controller to mirror the LimaVM's updated condition, and the `ObservedGeneration` field now copies the child resource's generation instead of the parent's, violating Kubernetes API conventions.

---

## Critical Issues

None.

---

## Important Issues

1. **BATS mirror assertions race the App reconcile** — `bats/tests/32-app-controllers/app.bats:162-166, 203-207` [Codex GPT 5.4, Gemini 3.1 Pro] (important, regression)

```bash
@test "verify App mirrors LimaVM Running condition as True" {
    run -0 rdd ctl get app "${APP_NAME}" -o json
    run -0 jq -r '.status.conditions[] | select(.type == "Running") | .status' <<<"${output}"
    assert_output "True"
}
```

The tests wait for the LimaVM's Running condition to change, then immediately read the App object and assert the mirrored value. The App controller reconciles asynchronously — it must receive a watch event for the LimaVM status change, run a reconcile loop, and update the App's status. Nothing guarantees this completes before the next BATS test reads the App. Codex ran the test suite locally and observed test 24 ("verify App mirrors LimaVM Running condition as False after stop") fail because the App still reported `Unknown`. The same race affects the `True` assertion at line 162 and the timestamp/generation checks at lines 168-190.

The pre-existing Created condition tests at lines 128-148 have the same pattern but predate this PR.

Fix: Insert a `rdd ctl wait` on the App resource before each mirror assertion. For example:

```diff
+@test "wait for App Running condition to become True" {
+    rdd ctl wait --for=condition=Running \
+        app/"${APP_NAME}" --timeout=30s
+}
+
 @test "verify App mirrors LimaVM Running condition as True" {
     run -0 rdd ctl get app "${APP_NAME}" -o json
```

Apply the same pattern before the `False after stop` assertion (line 203) and before the LastTransitionTime/ObservedGeneration checks.

2. **`ObservedGeneration` copies the child's generation instead of the parent's** — `pkg/controllers/app/app/controllers/app_controller.go:193` [Codex GPT 5.4, Gemini 3.1 Pro] (important, regression)

```go
statusChanged = apimeta.SetStatusCondition(&app.Status.Conditions, metav1.Condition{
    Type:               cond.Type,
    Status:             cond.Status,
    Reason:             cond.Reason,
    Message:            base.TruncateConditionMessage(cond.Message),
    ObservedGeneration: cond.ObservedGeneration,
    LastTransitionTime: cond.LastTransitionTime,
})
```

The Kubernetes API convention defines `ObservedGeneration` as the `.metadata.generation` of the resource that owns the condition — here, the App. The previous code set `ObservedGeneration: app.Generation`, which was correct. This PR changes it to `cond.ObservedGeneration`, which copies the LimaVM's value (currently always 0 because the LimaVM controller omits it). Clients that compare `app.status.conditions[].observedGeneration` with `app.metadata.generation` will treat the App's condition as perpetually stale.

The design doc at `docs/design/api_app.md:92` and the BATS test at `app.bats:180-190` both codify this incorrect behavior.

Fix: Revert to `app.Generation`:

```diff
-			ObservedGeneration: cond.ObservedGeneration,
+			ObservedGeneration: app.Generation,
```

Update `api_app.md:92` to exclude `observedGeneration` from the list of mirrored fields, and either remove or rewrite the `app.bats:180-190` test to compare against `app.metadata.generation` instead.

---

## Suggestions

1. **Orphaned footnote definition** — `docs/design/api_app.md:13` [Claude Opus 4.6] (suggestion, regression)

```markdown
[^hardcoded]: The "app-namespace" is only configurable so that it can be tested
that the namespace isn't hardcoded anywhere in the controller.
```

Commit `a53ebd0` rewrote line 11, removing the `[^hardcoded]` reference from the paragraph text. The footnote definition on line 13 is now orphaned — it renders as dead text in Markdown viewers that support footnotes and as a confusing bare line in those that do not.

Fix: Delete line 13 and the blank line 12 before it.

2. **Stop-path tests lack LastTransitionTime and ObservedGeneration checks** — `bats/tests/32-app-controllers/app.bats:198-207` [Claude Opus 4.6] (suggestion, gap)

The start path has four tests (wait for Running=True, verify App mirrors True, verify LastTransitionTime matches, verify ObservedGeneration matches). The stop path has only two (wait for Running=False, verify App mirrors False). Adding symmetric checks for the stop path would catch regressions where the stop transition updates the condition status but corrupts the mirrored metadata.

Fix: Add "verify App Running condition preserves LimaVM LastTransitionTime after stop" and "verify App Running condition observedGeneration matches after stop" tests, following the same pattern as lines 168-190.

3. **Commit bundles unrelated fixes** — commit `ae6bc38` [Claude Opus 4.6] (suggestion, regression)

The commit message says "Fix ObservedGeneration in App Controller," but the diff also changes two `log.Error(...)` calls to `return ctrl.Result{}, fmt.Errorf(...)` at lines 70 and 168. These error-handling improvements are unrelated to ObservedGeneration. Either split them into a separate commit or mention them in the commit message.

---

## Design Observations

**Concerns**

- **ObservedGeneration is always zero on LimaVM conditions** (future) [Claude Opus 4.6] — The LimaVM controller's `setCondition` helper creates conditions without setting `ObservedGeneration`, so the field is always 0 (omitted via `omitempty`). Even after fixing the App controller to use `app.Generation`, the LimaVM itself carries misleading condition metadata. A future change should set `ObservedGeneration: limaVM.Generation` in the LimaVM controller's condition updates.

- **App conditions as child-status transport** (in-scope) [Codex GPT 5.4] — The App API uses standard `metav1.Condition` fields to mirror child status verbatim. This works well for most fields (`type`, `status`, `reason`, `message`, `lastTransitionTime`), but `observedGeneration` has resource-specific semantics that break under mirroring. Keeping this one field App-scoped while mirroring the rest is the right balance.

**Strengths**

- The error-handling upgrades from log-and-continue to return-error-and-retry are correct and important. The previous behavior at line 70 was subtle: a ConfigMap fetch error during cleanup would be logged but then fall through to `RemoveCleanupFinalizer`, permanently removing the finalizer and preventing future cleanup retries. [Claude Opus 4.6, Gemini 3.1 Pro]

- The condition mirroring design — copying all fields verbatim including `lastTransitionTime` — keeps timestamps meaningful for staleness checks and avoids the App controller imposing its own interpretation of LimaVM state. [Claude Opus 4.6]

---

## Testing Assessment

Test coverage for the new start/stop lifecycle is solid — the full cycle (create App with running=false, propagate running=true, wait for LimaVM Running=True, propagate running=false, wait for Running=False) is exercised end-to-end. Once the race condition in the mirror assertions is fixed, the suite will be robust.

Untested scenarios, ranked by risk:

1. **Stop-path metadata mirroring** — LastTransitionTime and ObservedGeneration are not verified after the stop transition (see Suggestion 2).
2. **Error-path retry behavior** — The two error-handling upgrades (lines 70, 168) are not tested. Injecting transient API errors in BATS is impractical, so this is a low-priority gap.

---

## Documentation Assessment

The `api_app.md` rewrite is a substantial improvement. The condition table now reflects actual LimaVM states, and the mirroring semantics are clearly documented. Two items need attention:

- Remove the orphaned `[^hardcoded]` footnote (Suggestion 1).
- Update line 92 to exclude `observedGeneration` from the list of mirrored fields once Finding 2 is fixed.

---

## Commit Structure

Commit `ae6bc38` ("Fix ObservedGeneration in App Controller") bundles two distinct changes: the ObservedGeneration propagation fix and two error-handling improvements. The commit message describes only the first. Ideally these would be separate commits so `git bisect` can distinguish between them, though the scope of both changes is small (see Suggestion 3).

---

## Agent Performance Retro

### [Claude]

Claude produced a thorough review with correct analysis of every changed file. It identified the orphaned footnote, the stop-path test asymmetry, and the commit structure issue — all unique findings. However, it rated the ObservedGeneration change as a future design concern rather than an Important regression, missing that the PR actively changed correct behavior (`app.Generation`) to incorrect behavior (`cond.ObservedGeneration`). It also did not identify the BATS race condition, though it noted the stop-path tests lacked certain checks.

### [Codex]

Codex delivered the strongest review. It correctly identified both Important findings and verified them empirically — it ran the BATS test suite and observed the race condition failure firsthand (test 24). Its analysis of the ObservedGeneration semantics was precise, citing the Kubernetes API convention and explaining why `app.Generation` was correct. It produced no false positives. The review was tightly focused with no filler.

### [Gemini]

Gemini identified the same two Important findings as Codex with clear explanations and concrete fix suggestions. Its analysis was accurate and well-structured. It also noted the error-handling improvements as a design strength. It produced no false positives or unique findings beyond what Codex covered.

### Summary

| Metric | Claude Opus 4.6 | Codex GPT 5.4 | Gemini 3.1 Pro |
|---|---|---|---|
| Duration | 4:42 | 5:20 | 6:16 |
| Critical | 0 | 0 | 0 |
| Important | 0 | 2 | 2 |
| Suggestion | 3 | 0 | 0 |
| Design observations | 1 | 1 | 0 |
| False positives | 0 | 0 | 0 |
| Unique insights | 3 (footnote, test asymmetry, commit structure) | 1 (live test execution) | 0 |
| Files reviewed | 3/3 | 3/3 | 3/3 |
| Coverage misses | 0 | 0 | 0 |

Codex provided the most value this round — it found both Important issues and verified the race condition empirically. Gemini matched Codex on the Important findings. Claude contributed useful Suggestions and design context but underrated the ObservedGeneration regression. The multi-agent approach paid off: combining Claude's granular Suggestions with Codex/Gemini's severity-calibrated Important findings produces a more complete review than any single agent delivered.

---

## Appendix: Original Reviews

### Claude Opus 4.6

Now I have all the context needed. Let me write the review.

---

## Executive Summary

This PR adds stop/start lifecycle mirroring between the App and LimaVM controllers: it fixes ObservedGeneration propagation (was using App generation instead of LimaVM's), upgrades two log-and-continue error paths to proper error returns, adds BATS tests for the Running condition lifecycle, and updates the design doc to reflect the mirroring semantics. The changes are clean and well-scoped.

**Verdict: Approve with minor suggestions.**

---

## Findings

### Critical Issues

None.

### Important Issues

None.

### Suggestions

1. **Stale footnote definition** — `docs/design/api_app.md:13` (suggestion, regression)

```markdown
[^hardcoded]: The "app-namespace" is only configurable so that it can be tested
that the namespace isn't hardcoded anywhere in the controller.
```

Commit `a53ebd0` rewrote line 11, removing the `[^hardcoded]` reference that was in the original text. The footnote definition on line 13 is now orphaned — no text references it. It will render as dead text in Markdown renderers that support footnotes, and as a confusing bare line in those that don't.

Fix: Delete line 13 (and the blank line 12 before it if it becomes a double-blank).

2. **Stop-path tests lack LastTransitionTime and ObservedGeneration checks** — `bats/tests/32-app-controllers/app.bats:198-207` (suggestion, gap)

The start path has four tests (wait for Running=True, verify App mirrors True, verify LastTransitionTime matches, verify ObservedGeneration matches). The stop path has only two (wait for Running=False, verify App mirrors False). The missing checks would catch a regression where the stop transition updates the condition status but corrupts the mirrored metadata fields.

Fix: Add "verify App Running condition preserves LimaVM LastTransitionTime after stop" and "verify App Running condition observedGeneration matches LimaVM after stop" tests, following the same pattern as lines 168-190.

3. **Commit bundles unrelated fixes under one message** — commit `ae6bc38` (suggestion, regression)

The commit message says "Fix ObservedGeneration in App Controller" and the body mentions only the generation propagation change. However, the diff also changes two `log.Error(...)` calls to `return ctrl.Result{}, fmt.Errorf(...)` at lines 70 and 168. These are error-handling improvements unrelated to ObservedGeneration — they should either be in a separate commit or mentioned in the commit message.

---

## Design Observations

### Concerns

**ObservedGeneration is always zero on mirrored conditions** (future) — The LimaVM controller's `setCondition` helper (`limavm_controller.go:427-437`) creates conditions without setting `ObservedGeneration`, so `apimeta.SetStatusCondition` writes 0. The App controller now correctly mirrors that 0 (fixing the previous bug where it used `app.Generation`), but the result is that the App's conditions carry `observedGeneration: 0` — or rather, the field is omitted entirely (`omitempty`). The BATS test at lines 180-190 passes because `null == null`, but it isn't testing a meaningful invariant yet. Once the LimaVM controller starts populating `ObservedGeneration` with `limaVM.Generation`, this test will become load-bearing without any code change.

### Strengths

- The condition mirroring design (copy all fields verbatim, including `lastTransitionTime`) is a good call — it keeps timestamps meaningful for staleness checks and avoids the App controller imposing its own interpretation of LimaVM state.
- The error-handling upgrades from log-and-continue to return-error-and-retry are correct. The previous behavior at line 70 was particularly subtle: a ConfigMap fetch error during cleanup would be logged but then fall through to `RemoveCleanupFinalizer`, permanently removing the finalizer and preventing future cleanup retries.

---

## Testing Assessment

Test coverage for the new behavior is solid. The full start-stop-mirror lifecycle is exercised end-to-end. Untested scenarios, ranked by risk:

1. **Stop-path metadata mirroring** — LastTransitionTime and ObservedGeneration are not verified after the stop transition (see Suggestion 2).
2. **Error-path behavior** — The two `log.Error → return fmt.Errorf` fixes at lines 70 and 168 are not tested (injecting transient API errors in BATS is not straightforward, so this is a low-priority gap).

---

## Documentation Assessment

The design doc update (`api_app.md`) is a substantial improvement — the condition table now reflects the actual LimaVM states, and the mirroring semantics are clearly documented. The one issue is the orphaned `[^hardcoded]` footnote (Suggestion 1).

---

## Commit Structure

Commit `ae6bc38` ("Fix ObservedGeneration in App Controller") bundles two logically distinct changes: the ObservedGeneration fix and two error-handling improvements. The commit message describes only the first. Ideally these would be split so that `git bisect` can identify which change caused a regression, but this is a minor concern given the small scope of both changes (see Suggestion 3).

---

## Coverage Summary

| File | Status |
|------|--------|
| `bats/tests/32-app-controllers/app.bats` | **Suggestion 2** (stop-path test asymmetry) |
| `docs/design/api_app.md` | **Suggestion 1** (stale footnote) |
| `pkg/controllers/app/app/controllers/app_controller.go` | **Suggestion 3** (commit scope); reviewed, code changes are correct |

### Codex GPT 5.4

**Verdict**

Request changes.

### Executive Summary

This PR extends the App controller review surface in the right areas: it documents App/LimaVM condition mirroring, adjusts the controller’s condition copy logic, and adds BATS coverage for start/stop. Two issues block merge: the controller/docs/tests now codify an invalid `metav1.Condition.observedGeneration` contract, and the newly added BATS assertions race the asynchronous App reconcile and already fail locally.

### Findings

**Critical Issues**

None.

**Important Issues**

1. **App conditions now publish the child object's generation, not the App's generation** — `pkg/controllers/app/app/controllers/app_controller.go:188-194` `(important, regression)`

```go
statusChanged = apimeta.SetStatusCondition(&app.Status.Conditions, metav1.Condition{
	Type:               cond.Type,
	Status:             cond.Status,
	Reason:             cond.Reason,
	Message:            base.TruncateConditionMessage(cond.Message),
	ObservedGeneration: cond.ObservedGeneration,
	LastTransitionTime: cond.LastTransitionTime,
})
```

`metav1.Condition.ObservedGeneration` is defined as the containing object's `.metadata.generation`, but line 193 now copies the LimaVM's value into the App's condition. The same behavior is documented in [docs/design/api_app.md](/Users/jan/git/rancher-desktop-daemon/docs/design/api_app.md#L92) and enforced by the new BATS assertion at [bats/tests/32-app-controllers/app.bats](/Users/jan/git/rancher-desktop-daemon/bats/tests/32-app-controllers/app.bats#L180). That breaks generic condition semantics for `App`: a client comparing `app.status.conditions[*].observedGeneration` with `app.metadata.generation` is no longer reading an App-scoped freshness signal. The controller does not need this escape hatch, because it already returns early when `App.spec.running` and `LimaVM.spec.running` differ at [pkg/controllers/app/app/controllers/app_controller.go](/Users/jan/git/rancher-desktop-daemon/pkg/controllers/app/app/controllers/app_controller.go#L171), so by the time lines 188-194 run the mirrored status is already based on the current App spec.

Fix: keep App conditions App-scoped and set `ObservedGeneration: app.Generation`; if you need to surface the LimaVM generation for debugging, expose it in a separate status field rather than overloading `metav1.Condition`. Update the doc sentence at [docs/design/api_app.md](/Users/jan/git/rancher-desktop-daemon/docs/design/api_app.md#L92) and the test at [bats/tests/32-app-controllers/app.bats](/Users/jan/git/rancher-desktop-daemon/bats/tests/32-app-controllers/app.bats#L180) to match.

```diff
 statusChanged = apimeta.SetStatusCondition(&app.Status.Conditions, metav1.Condition{
 	Type:               cond.Type,
 	Status:             cond.Status,
 	Reason:             cond.Reason,
 	Message:            base.TruncateConditionMessage(cond.Message),
-	ObservedGeneration: cond.ObservedGeneration,
+	ObservedGeneration: app.Generation,
 	LastTransitionTime: cond.LastTransitionTime,
 })
```

2. **The new BATS mirror checks race the App reconcile and already fail on the stop path** — `bats/tests/32-app-controllers/app.bats:157-206` `(important, regression)`

```bash
@test "wait for LimaVM Running condition to become false after stop" {
    rdd ctl wait --for=condition=Running=False \
        limavm/"${VM_NAME}" --namespace "${RDD_NAMESPACE}" --timeout=60s
}

@test "verify App mirrors LimaVM Running condition as False after stop" {
    run -0 rdd ctl get app "${APP_NAME}" -o json
    run -0 jq -r '.status.conditions[] | select(.type == "Running") | .status' <<<"${output}"
    assert_output "False"
}
```

These new tests assume that once the child `LimaVM` reaches a condition, the parent `App` has already reconciled and copied it. That is not guaranteed: the App controller only updates its status after it receives and processes the LimaVM watch event. On my local run, `bats/lib/bats-core/bin/bats bats/tests/32-app-controllers/app.bats` failed in test 24 because line 206 expected `False` but the App still reported `Unknown`. The same race pattern is present in the new assertions at lines 162-189 and in the older Created-condition checks at lines 128-148, so fixing only line 203 will leave the suite flaky.

Fix: wait on the App resource itself before asserting mirrored fields, and apply the same pattern to every App-mirroring check in this file.

```diff
+@test "wait for App Running condition to become False after stop" {
+    rdd ctl wait --for=jsonpath='{.status.conditions[?(@.type=="Running")].status}'=False \
+        app/"${APP_NAME}" --timeout=60s
+}
+
 @test "verify App mirrors LimaVM Running condition as False after stop" {
     run -0 rdd ctl get app "${APP_NAME}" -o json
     run -0 jq -r '.status.conditions[] | select(.type == "Running") | .status' <<<"${output}"
     assert_output "False"
 }
```

**Suggestions**

None.

### Design Observations

**Concerns**

- The App API is trying to use standard `metav1.Condition` fields as a verbatim child-status transport. Keeping App conditions App-scoped and exposing any raw LimaVM bookkeeping in separate status fields would preserve compatibility with generic condition consumers and avoid repeating this `observedGeneration` ambiguity elsewhere. `(in-scope)`

### Testing Assessment

Executed `bats/lib/bats-core/bin/bats bats/tests/32-app-controllers/app.bats`; it failed at test 24.

1. The suite does not wait for App convergence before reading mirrored fields, so the stop-path mirror assertions at [app.bats](/Users/jan/git/rancher-desktop-daemon/bats/tests/32-app-controllers/app.bats#L203) and the similar checks at [app.bats](/Users/jan/git/rancher-desktop-daemon/bats/tests/32-app-controllers/app.bats#L128) and [app.bats](/Users/jan/git/rancher-desktop-daemon/bats/tests/32-app-controllers/app.bats#L162) are flaky.
2. There is no test that validates `App.status.conditions[*].observedGeneration` against `App.metadata.generation`, so the controller/doc regression at [app_controller.go](/Users/jan/git/rancher-desktop-daemon/pkg/controllers/app/app/controllers/app_controller.go#L193) shipped unchecked.

### Documentation Assessment

[docs/design/api_app.md](/Users/jan/git/rancher-desktop-daemon/docs/design/api_app.md#L92) should not state that the App copies LimaVM `observedGeneration` verbatim; that conflicts with the semantics of `metav1.Condition` on the App resource.

### Coverage Summary

- `bats/tests/32-app-controllers/app.bats` — Findings 1, 2
- `docs/design/api_app.md` — Finding 1
- `pkg/controllers/app/app/controllers/app_controller.go` — Finding 1

`gh pr view 248` was not usable from this environment because the current GitHub auth could not access `rancher-sandbox/rancher-desktop-daemon`, so I could not inspect PR comments/reviews for unresolved feedback.

### Gemini 3.1 Pro

### Executive Summary

This PR enables the App controller to start and stop the LimaVM by propagating `spec.running` and mirroring the VM's status conditions. While the error handling improvements during the reconcile loop are excellent, the PR introduces a race condition in the new BATS tests and violates Kubernetes API conventions by copying the child resource's `ObservedGeneration` into the parent's status conditions. 

### Critical Issues

None.

### Important Issues

1. **Race condition in BATS tests asserting mirrored conditions** — `bats/tests/32-app-controllers/app.bats:162-166` (important, regression)

```bash
@test "verify App mirrors LimaVM Running condition as True" {
    run -0 rdd ctl get app "${APP_NAME}" -o json
    run -0 jq -r '.status.conditions[] | select(.type == "Running") | .status' <<<"${output}"
    assert_output "True"
}
```

The BATS tests assume that the `App` object instantly reflects the new `LimaVM` condition. In reality, the `App` controller reconciles asynchronously in response to `LimaVM` watch events. Fetching the `App` object immediately after the `LimaVM` condition changes creates a race condition where the test might read the `App` object before the controller has mirrored the new status. This can cause flaky CI runs, as it will occasionally read the old `App` state (or `Unknown`). The exact same race condition exists in the `verify App mirrors LimaVM Running condition as False after stop` test.

Fix: Use `rdd ctl wait` to natively handle the asynchronous delay, guaranteeing the condition has been updated before the subsequent timestamp and generation tests run.

```diff
-@test "verify App mirrors LimaVM Running condition as True" {
-    run -0 rdd ctl get app "${APP_NAME}" -o json
-    run -0 jq -r '.status.conditions[] | select(.type == "Running") | .status' <<<"${output}"
-    assert_output "True"
+@test "wait for App to mirror LimaVM Running condition as True" {
+    rdd ctl wait --for=condition=Running \
+        app/"${APP_NAME}" --timeout=10s
 }
```
(Apply the equivalent fix to the `False after stop` test on line 198).

2. **`ObservedGeneration` incorrectly mirrors the child's generation** — `pkg/controllers/app/app/controllers/app_controller.go:193` (important, regression)

```go
 			Reason:             cond.Reason,
 			Message:            base.TruncateConditionMessage(cond.Message),
 			ObservedGeneration: cond.ObservedGeneration,
 			LastTransitionTime: cond.LastTransitionTime,
```

The Kubernetes API conventions dictate that a condition's `ObservedGeneration` must represent the `.metadata.generation` of the resource the condition is on (in this case, the `App` object). Setting it to `cond.ObservedGeneration` assigns it the `LimaVM`'s generation (or `0`, since the `LimaVM` controller currently omits it). 

Because the `App` and `LimaVM` objects have independent generation sequences, copying the child's generation guarantees it will mismatch the `App`'s `.metadata.generation`. Client tools (like `kubectl`) often compare these fields to detect stale statuses; an artificial mismatch will cause them to incorrectly report the `App` condition as permanently outdated. The previous behavior (`app.Generation`) was semantically correct for the `App` object: it signaled that the controller had observed the latest `App` spec when recording the mirrored condition.

Fix: Revert back to using the `App`'s generation.

```diff
 			Reason:             cond.Reason,
 			Message:            base.TruncateConditionMessage(cond.Message),
-			ObservedGeneration: cond.ObservedGeneration,
+			ObservedGeneration: app.Generation,
 			LastTransitionTime: cond.LastTransitionTime,
```
Additionally, remove the new `verify App Running condition observedGeneration matches LimaVM` test from `bats/tests/32-app-controllers/app.bats`, as they are fundamentally not meant to match. 

### Suggestions

None.

### Design Observations

**Concerns**

None.

**Strengths**

The error handling improvements in `pkg/controllers/app/app/controllers/app_controller.go` (changing `log.Error(cmErr, ...)` to `return ctrl.Result{}, fmt.Errorf(...)` at lines 67 and 165) are excellent. Previously, the controller would log and silently continue, which could lead to incorrectly stripping the cleanup finalizer or proceeding without a necessary ConfigMap. Returning the error correctly bubbles up the failure and triggers a retry.

### Testing Assessment

Test coverage is comprehensive and covers the full start/stop lifecycle of the App propagating downward to the LimaVM. Once the flaky synchronization in the BATS tests is resolved (see Finding 1), the test suite will be robust. 

### Documentation Assessment

The documentation updates in `docs/design/api_app.md` accurately capture the new behavior and clarify the singleton constraints. However, as part of fixing Finding 2, the newly added documentation regarding mirrored fields will need a slight adjustment to exclude `observedGeneration`.

Update `docs/design/api_app.md:92`:
```diff
-- **status.conditions**: Conditions are **mirrored directly from the owned `LimaVM`** resource. The App controller does not set conditions independently; it copies `type`, `status`, `reason`, `message`, `lastTransitionTime`, and `observedGeneration` from the LimaVM's conditions on every reconcile.
+- **status.conditions**: Conditions are **mirrored directly from the owned `LimaVM`** resource. The App controller does not set conditions independently; it copies `type`, `status`, `reason`, `message`, and `lastTransitionTime` from the LimaVM's conditions on every reconcile. `observedGeneration` is set to the `App`'s current generation.
```

### Coverage Summary

- `bats/tests/32-app-controllers/app.bats` — **Finding 1**, **Finding 2**
- `docs/design/api_app.md` — **Finding 2**
- `pkg/controllers/app/app/controllers/app_controller.go` — **Finding 2**