# Deep Review: 20260416-144208-pr-316

| | |
|---|---|
| **Date** | 2026-04-16 14:42 |
| **Repo** | [rancher-sandbox/rancher-desktop-daemon](https://github.com/rancher-sandbox/rancher-desktop-daemon) |
| **Round** | 3 |
| **Author** | [@jandubois](https://github.com/jandubois) |
| **PR** | [#316](https://github.com/rancher-sandbox/rancher-desktop-daemon/pull/316) — engine: replace Container.spec.state with action annotations |
| **Branch** | `container-action-annotations` |
| **Commits** | `397d40a` engine: replace Container.spec.state with action annotations |
| **Reviewers** | Claude Opus 4.7, Codex GPT 5.4, Gemini 3.1 Pro |
| **Verdict** | **Merge with fixes** — one webhook gap leaves a stated safeguard unenforced on the update path; stale comments need cleanup. |
| **Wall-clock time** | `27 min 8 s` |

---

## Executive Summary

The core design lands cleanly. Replacing the level-triggered `Container.spec.state` field with a one-shot `containers.rancherdesktop.io/action` annotation plus a `status.lastAction` outcome correctly recognizes that Docker (restart policy, out-of-band `docker start/stop`) is a concurrent writer that a level-triggered mirror cannot co-own. The implementation handles crash-replay (dispatch before status patch, annotation persists until cleanup), informer-cache staleness (pre-checks on pause/unpause), concurrent annotation overwrites (observed-vs-current comparison in `removeActionAnnotation`), and webhook bypass at dispatch time (`IsValid` guard in `processContainerAction`). The SSA/finalizer simplification enabled by the now-empty `ContainerSpec` is a genuine cleanup.

One real correctness gap: the webhook's stated safeguard that "a hand-written Container cannot drive a Docker action" is only enforced on `Create`. A caller can create a placeholder `Container` without the annotation and then `PATCH` the annotation in, and `ValidateUpdate` will accept it. The remaining findings are documentation: two stale code comments still reference the removed `spec.state` / `reconcileContainerSpecs` identifiers, and a third comment ("prevents all modification") no longer describes what the webhook does.

Structure: 0 critical · 1 important · 6 suggestions.

---

## Critical Issues

None.

---

## Important Issues

I1. **Webhook safeguard enforced only on create, not on update** — `pkg/controllers/containers/container/container_webhooks.go:27-52` [Codex GPT 5.4]

```go
func (c *immutableValidator) ValidateCreate(_ context.Context, newContainer *v1alpha1.Container) (warnings ctrlwebhookadmission.Warnings, err error) {
	if _, ok := newContainer.Annotations[v1alpha1.AnnotationAction]; ok {
		return nil, fmt.Errorf("%s annotation is not allowed on create", v1alpha1.AnnotationAction)
	}
	return nil, nil
}

func (c *immutableValidator) ValidateUpdate(_ context.Context, oldContainer, newContainer *v1alpha1.Container) (warnings ctrlwebhookadmission.Warnings, err error) {
	if !equality.Semantic.DeepEqual(oldContainer.Spec, newContainer.Spec) {
		return nil, fmt.Errorf("spec is immutable: old: %v, new: %v", oldContainer.Spec, newContainer.Spec)
	}
	if raw, ok := newContainer.Annotations[v1alpha1.AnnotationAction]; ok {
		if !v1alpha1.ContainerAction(raw).IsValid() {
			return nil, fmt.Errorf("invalid %s annotation %q", v1alpha1.AnnotationAction, raw)
		}
	}
	return nil, nil
}
```

The comment on `ValidateCreate` (lines 24–26) states the intent explicitly: "The engine watcher creates Container mirrors and never sets the action annotation; reject any create that carries one so a hand-written Container cannot drive a Docker action against its metadata.name." `ValidateUpdate` does not enforce that invariant — it only rejects invalid action values. A two-step bypass defeats the guard:

1. Create a `Container` with `metadata.name=<some docker container id>` and no annotation. `ValidateCreate` accepts it (spec is empty, no annotation).
2. `PATCH` the same object to add `metadata.annotations[containers.rancherdesktop.io/action]=stop`. `ValidateUpdate` accepts it.

The reconciler path does not distinguish mirrors from hand-written objects. `reconcileContainerActions` at `engine_reconciler.go:366-385` lists every `Container` in the namespace, and `processContainerAction` at `docker_watcher.go:356-403` dispatches the Docker call against `c.Name`. If the chosen name matches a real Docker container ID, the action runs.

The impact in practice is bounded — on a single-user desktop tool the same principal already has Docker socket access — but the webhook's stated safeguard does not hold. Either the safeguard should hold end-to-end, or the create-time rejection should be dropped so the inconsistency does not mislead future readers.

Fix: apply the same provenance check on update, or drop the `Create` rejection. The cleanest enforcement is to require an engine-owned marker (the `engine.rancherdesktop.io/mirror` finalizer is already added on every `applyContainer`, though a caller can set it themselves) or to inspect `managedFields` to confirm the object was originally created by `engine-controller` before accepting an action annotation.

---

## Suggestions

S1. **Stale comment references the removed `reconcileContainerSpecs` / `spec.state`** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:183-186` [Claude Opus 4.7, Codex GPT 5.4]

```go
// Two passes run per Reconcile: reconcileContainerSpecs drives
// Docker start/stop from Container.spec.state, and
// processFinalizers forwards K8s-side deletes to Docker for any
// mirror still carrying the mirror finalizer.
```

The function was renamed to `reconcileContainerActions` in this PR (called at line 199) and `Container.spec.state` no longer exists. The rest of the block (lines 188-197) was updated to reference the new "action annotation present" watch predicate, but lines 183-184 were missed. A reader following this comment is pointed at code that was deleted in the same commit. Both Claude and Codex flagged this; Codex also included `container_controller.go:81` (see S3) under the same theme.

Fix:

```diff
-	// Two passes run per Reconcile: reconcileContainerSpecs drives
-	// Docker start/stop from Container.spec.state, and
+	// Two passes run per Reconcile: reconcileContainerActions drives
+	// Docker actions from the action annotation, and
 	// processFinalizers forwards K8s-side deletes to Docker for any
 	// mirror still carrying the mirror finalizer.
```

S2. **`ContainerSpec` Godoc first line contradicts the rest of the comment** — `pkg/apis/containers/v1alpha1/container_types.go:124-129` [Claude Opus 4.7]

```go
// ContainerSpec defines the configuration the container was created with.
// Container spec is empty today: actions are requested via the AnnotationAction
// annotation, not a level-triggered desired-state field. Docker's restart
// policy and out-of-band `docker start/stop` are competing writers, so a
// level-triggered desired state would fight them.
type ContainerSpec struct{}
```

Line 124 predates the PR but is now directly contradicted by lines 125-128 (added by the PR): the struct is not "the configuration the container was created with," it is intentionally empty. A reader hitting the first line is told to expect created-time configuration. The PR extended the doc but did not rewrite the opener.

Fix: delete or rewrite the first line so the opener matches the rest.

```diff
-// ContainerSpec defines the configuration the container was created with.
-// Container spec is empty today: actions are requested via the AnnotationAction
+// ContainerSpec is currently empty. Actions are requested via the AnnotationAction
 // annotation, not a level-triggered desired-state field. Docker's restart
 // policy and out-of-band `docker start/stop` are competing writers, so a
 // level-triggered desired state would fight them.
 type ContainerSpec struct{}
```

S3. **"Prevents all modification" comment no longer matches the webhook's behavior** — `pkg/controllers/containers/container/container_controller.go:81` [Codex GPT 5.4]

```go
// set up the container controller with a webhook which prevents all modification.
func (c *controller) setupWebhookWithRuntimeConfig(mgr ctrl.Manager) error {
```

After this PR, `ValidateCreate` allows creates without the action annotation, and `ValidateUpdate` allows updates that add or change a valid action annotation. The webhook no longer "prevents all modification" — it enforces a spec-immutability rule and a valid-action rule. Rewrite the comment to describe what the webhook actually does.

S4. **Invalid-action fallback is silent to the caller** — `pkg/controllers/app/engine/controllers/docker_watcher.go:370-373` [Claude Opus 4.7]

```go
if !action.IsValid() {
    log.Info("Dropping invalid container action annotation", "id", c.Name, "action", raw)
    return w.removeActionAnnotation(ctx, c, raw)
}
```

The webhook normally rejects invalid values at admission, so this branch only fires when the webhook is bypassed (`failurePolicy: Ignore`, or the startup window before registration). When it does fire, the caller gets no feedback: `lastAction` is untouched and the annotation disappears. An `Info` log is invisible to a GUI client. Two options: raise the log to `Error` with a clear message, or emit a Kubernetes Event on the Container so a UI can surface it. Low priority — the webhook catches the normal case — but worth doing because the dropped-annotation branch is specifically the "webhook was bypassed" path, which is exactly when observability matters.

S5. **`ObservedAt` semantics are ambiguous in the doc comment and could be clearer** — `pkg/apis/containers/v1alpha1/container_types.go:88-92`, `pkg/controllers/app/engine/controllers/docker_watcher.go:364, 377-381` [Claude Opus 4.7]

```go
// ObservedAt is when the reconciler observed the action annotation.
// Backlog may delay this relative to the user's write time.
ObservedAt metav1.Time `json:"observedAt,omitempty,omitzero"`
```

In `processContainerAction`, `observedAt := metav1.Now()` is captured after the annotation has already been seen via `List`, just before the `IsValid` check and dispatch. `CompletedAt` is captured after `dispatchContainerAction` returns. If a caller computes `CompletedAt - ObservedAt` to measure user-visible latency, it will get the dispatch duration (which includes a multi-second grace period on restart or stop), not the observe-to-process backlog. Either rename the field to `StartedAt`/`ProcessingStartedAt`, or clarify the Godoc to "when the reconciler began processing the annotation." Keep the design doc text (`docs/design/api_containers.md:160-161`) in sync.

S6. **`removeActionAnnotation` comment asserts an invariant the invalid-action branch does not satisfy** — `pkg/controllers/app/engine/controllers/docker_watcher.go:502-509, 370-373` [Claude Opus 4.7]

```go
// ... The caller passes a fresh Container from patchContainerLastAction
// so the first Patch attempt bypasses the informer cache, which has not
// yet seen the preceding status write. On conflict, the retry re-reads;
// by then the cache has usually caught up.
func (w *dockerWatcher) removeActionAnnotation(ctx context.Context, latest *containersv1alpha1.Container, observed string) error {
```

The `!action.IsValid()` branch in `processContainerAction` passes `c` directly from the cache-backed `List` in `reconcileContainerActions`, not a fresh Container from `patchContainerLastAction`. Functionally the `RetryOnConflict` loop still works (its second attempt re-reads on conflict), but the comment's claim that "the caller passes a fresh Container" is not universally true. Either do a direct (non-cache) `Get` in the invalid-action branch before calling `removeActionAnnotation`, or soften the comment to "callers may pass either a fresh Container or a cached one; the retry loop tolerates cache lag."

S7. **No unit test for the `dispatchContainerAction` default branch** — `pkg/controllers/app/engine/controllers/docker_watcher.go:456-457` [Claude Opus 4.7]

```go
return fmt.Errorf("unknown container action %q", action)
```

`IsValid` and the webhook both gate input, so this path is unreachable under normal operation. The comment at lines 406-408 correctly notes that the default branch only triggers when a new `ContainerAction` constant is added without a matching `switch` case. A tiny unit test that constructs a value not in the switch and asserts the error message would catch future drift between the enum, `IsValid`, and this switch. Low priority.

---

## Design Observations

### Strengths

- `(in-scope)` **Edge-triggered action model resolves the ownership conflict.** The core insight — that Docker's restart policy and out-of-band CLI writes make level-triggered `spec.state` unworkable — is articulated in the commit message and design doc, and the code faithfully implements it. The crash-replay story (dispatch before status patch, annotation persists on crash, idempotent replay for start/stop/pause/unpause) is coherent; the one exception (restart's double-dispatch) is called out explicitly rather than hidden. [Claude Opus 4.7, Codex GPT 5.4, Gemini 3.1 Pro]
- `(in-scope)` **Pre-check for pause/unpause isolates the informer-cache race.** The asymmetric handling (pause skips on `paused`; unpause skips on `!paused` but fails explicitly on `!running`) is unusual, and the justification at `docker_watcher.go:409-416, 461-463` is accurate. The BATS cases ("pause action on a stopped container records failure" and the symmetric unpause case) pin the asymmetry so regressions will surface. [Claude Opus 4.7, Gemini 3.1 Pro]
- `(in-scope)` **Empty-spec simplification of the SSA dance is genuine cleanup.** Removing `finalizerFieldOwner` and collapsing `applyContainer` into a single `ForceOwnership` branch eliminates a subtle invariant (the old split existed specifically to avoid pruning `spec`). Now that `spec` is empty, the simpler model is demonstrably correct. [Claude Opus 4.7, Gemini 3.1 Pro]
- `(in-scope)` **Value-match check in `removeActionAnnotation` preserves superseding actions.** Comparing `observed` against the current annotation value before deletion means a concurrent writer can replace the action mid-flight and the new value survives — no queue, no lost writes. [Codex GPT 5.4, Gemini 3.1 Pro]
- `(in-scope)` **Webhook covers both admission paths.** Registering for `Create` plus `Update` closes two realistic bypass routes, and both paths have explicit BATS coverage. (See I1 for a residual gap on the update path.) [Claude Opus 4.7, Gemini 3.1 Pro]

### Concerns

- `(in-scope)` **Webhook authorization is split between admission and reconciliation, and only the admission half is updated.** Provenance checks for mirror-only behaviors are safer when the reconciler also enforces them, because that removes any dependence on request shape (`create` vs `update`) and keeps the invariant attached to the side effect itself. [Codex GPT 5.4]

---

## Testing Assessment

BATS coverage is solid for the happy paths: start, stop, pause/unpause toggle, pause-on-stopped and unpause-on-stopped failures, restart (with pre/post `StartedAt` comparison), webhook rejection of an invalid action on update, and webhook rejection of the annotation on create. Gaps, in rough order of risk:

1. **No test for the create-then-update bypass path** (see I1). Adding a test that creates a `Container` without the annotation and then patches in a valid action, asserting the update is rejected, would pin the invariant the webhook claims to enforce. [Codex GPT 5.4]
2. **No test for the webhook-bypass drop-path** — the `!action.IsValid()` branch in `processContainerAction` has no integration test because the webhook catches invalid values first. A unit test constructing a `Container` with a bogus annotation and calling `processContainerAction` directly would provide cheap coverage. [Claude Opus 4.7]
3. **No test that `lastAction` persists across a direct `docker stop`.** The design doc (`docs/design/api_containers.md:153-156`) asserts "Persists ... regardless of any observable state changes (e.g. a direct `docker stop`) in between." Simple to add: request `start`, verify `lastAction=start/Succeeded`, `docker stop`, verify `lastAction` unchanged. [Claude Opus 4.7]
4. **No test for concurrent annotation overwrite during dispatch.** The `current != observed` guard in `removeActionAnnotation` is covered only by code comment. Hard to race deterministically in BATS; possible as a unit test with a mock client returning different values on subsequent `Get`s. [Claude Opus 4.7]
5. **`lastAction` timestamp fields and overwrite behavior across consecutive actions are not asserted.** The BATS suite checks `action` and `state` but never reads `observedAt`/`completedAt`. [Codex GPT 5.4]
6. **Restart double-dispatch race has no coverage.** Acknowledged as a known trade-off; likely not worth testing. [Claude Opus 4.7]

---

## Documentation Assessment

The design doc (`docs/design/api_containers.md`) is comprehensive and matches the implementation: the action table, crash-replay semantics, failure example, and `lastAction` schema are all accurate. The CRD matches the Go types.

Stale code-level comments (captured as S1–S3) are the only documentation issues: `engine_reconciler.go:183-186` still names `reconcileContainerSpecs`, `container_types.go:124` still opens with "defines the configuration the container was created with," and `container_controller.go:81` still says the webhook "prevents all modification." Codex grouped these into one finding; Claude split the first two across I-level items; the consolidation treats all three at Suggestion severity.

The PR description's `state=InProgress|Succeeded|Failed` and `requestedAt` nomenclature does not match the landed code (`Succeeded|Failed` and `observedAt`), but only the PR description (not the commit message, design doc, or CRD) is outdated. Worth noting but not actionable in-repo.

---

## Commit Structure

Single commit. The commit message describes the change and motivation accurately, with sections on "API and SSA fallout" and "Docs" that match what the diff does. Clean history.

---

## Acknowledged Limitations

- **Restart double-dispatch on crash or cache-lag replay** — `docker_watcher.go:352-355`. Documented in both the code and `docs/design/api_containers.md:247-251`; the PR makes an explicit trade (accept the edge case rather than add restart-specific deduplication state).
- **O(N) per child event in reconcile loops** — `engine_reconciler.go:186-192`. Pre-existing; the PR adopts the existing pattern for action reconciliation and updates the example in the comment to match the new annotation-driven work.
- **No CLI surface for setting actions** — PR comment from @jandubois: "The feature exists solely for the benefit of the GUI." Consistent with the landed code; tests drive actions through `rdd ctl annotate`.
- **Dropped image/volume events on transient handler errors** — `docker_watcher.go:198-205`. Pre-existing; TODO note calls for a periodic full-sync tick. Not materially worsened by this PR.
- **Windows/WSL2 not covered by these BATS tests** — `engine-docker.bats:14-15` skips on Windows because the Docker socket access pattern is not yet wired. Pre-existing scope limitation.

---

## Unresolved Feedback

- @jandubois commented: "I don't think we need any CLI support for setting container actions. The feature exists solely for the benefit of the GUI." No code change requested; tests already drive actions through `rdd ctl annotate`. Resolved by author intent.

---

## Agent Performance Retro

### [Claude]

Claude produced the most specific catalogue — six findings with precise line anchors and fix diffs — and was the only agent to notice the `ObservedAt`-vs-dispatch-duration semantics gap (S5) and the `removeActionAnnotation` comment-vs-branch invariant mismatch (S6). Its severity calibration ran hot: the two stale-comment findings were flagged Important, but neither affects correctness, and Codex's independent Suggestion rating held up better on review. Documentation misdirection in this project would normally rank Important only when it materially misleads design decisions, which stale identifier names in a comment block do not.

### [Codex]

Codex landed the only finding on the webhook bypass path, and its fix sketch (require an engine-owned marker before accepting an action annotation on update) is the right shape. It also grouped the three stale-comment locations under one finding at Suggestion severity — a tighter framing than Claude's split. It produced no critical-severity false positives and no unique insight beyond the two findings, so it was narrower than Claude in breadth but deeper on the one item it flagged.

### [Gemini]

Gemini returned zero findings. Its Coverage Summary was complete (all 16 changed files accounted for with per-file disposition) and its stderr held only the YOLO-mode banner, so the "None." result reflects its actual assessment, not a truncated run. It did not pick up the webhook-update gap (Codex's I1) despite having `container_webhooks.go` in scope, and its design observations read more as a summary of the PR description than as independent analysis. Gemini's known skip of `git blame` is not the explanation here — I1 is visible from the diff alone, so the miss is analytical.

### Summary

| | Claude Opus 4.7 | Codex GPT 5.4 | Gemini 3.1 Pro |
|---|---|---|---|
| Duration | 21m 19s | 6m 23s | 5m 24s |
| Findings | 6S | 1I 1S | none |
| Tool calls | 41 (Read 20, Bash 12, Grep 9) | 43 (shell 42, stdin 1) | 16 (run_shell_command 9, read_file 4, grep_search 3) |
| Design observations | 4 | 3 | 4 |
| False positives | 0 | 0 | 0 |
| Unique insights | 2 | 1 | 0 |
| Files reviewed | 16 | 16 | 16 |
| Coverage misses | 0 | 0 | 1 |
| **Totals** | **6S** | **1I 1S** | **none** |
| Downgraded | 2 (I→S) | 0 | 0 |
| Dropped | 0 | 0 | 0 |

**Reconciliation.** Claude P1 "Stale comment references the removed function and field" (`engine_reconciler.go:183-184`): Important → Suggestion S1. Claude P1 "ContainerSpec doc comment is internally contradictory" (`container_types.go:124-128`): Important → Suggestion S2. Both were stale-identifier comments with no correctness impact; Codex independently rated the same class of findings at Suggestion, and that calibration held up.

Codex provided the most valuable output: one real finding nobody else caught. Claude provided the most volume of genuinely actionable items, though two were miscalibrated to Important. Gemini's "no findings" result, on a PR where Codex found a webhook bypass visible in the diff, is a weak signal for this review.

---

## Review Process Notes

**Skill improvements**

- **Severity calibration rule for stale-identifier comments.** When a comment references a function, field, or type that was renamed or removed in the same change under review, flag it as a Suggestion unless the reference propagates into user-facing documentation, API surface, or error messages. A stale internal-comment identifier misleads future readers but does not change behavior; upgrading it to Important inflates the severity scale. Add this calibration rule to the "Documentation" review dimension so future reviews rank such findings consistently.

**Repo context updates**

- **[repo] Rule: admission webhooks that claim a safeguard must enforce it on both `Create` and `Update`.** When a webhook comment or method doc asserts that "hand-written objects cannot drive a side effect," verify the invariant holds across the object's lifecycle: `ValidateCreate` rejecting the bad shape is insufficient if `ValidateUpdate` permits the same shape via a two-step create-then-patch. Flag any webhook whose stated intent is enforced only on one admission path. Add this check to `deep-review-context.md` so future reviewers apply it automatically when reading CRD webhooks in this repo.

---

## Resolution

| | |
|---|---|
| **Addressed** | 2026-04-16 |
| **Commit** | `31bc715 engine: replace Container.spec.state with action annotations` (amended into the original) |

| # | Finding | Action |
|---|---------|--------|
| 1 | Important #1: Webhook safeguard enforced only on create, not on update | Commented |
| 2 | Suggestion #1: Stale comment references removed `reconcileContainerSpecs` / `spec.state` | Fixed |
| 3 | Suggestion #2: `ContainerSpec` Godoc opener contradicts the rest | Fixed |
| 4 | Suggestion #3: "Prevents all modification" webhook comment no longer accurate | Fixed |
| 5 | Suggestion #4: Invalid-action fallback is silent to the caller | Skipped |
| 6 | Suggestion #5: `ObservedAt` semantics ambiguous in godoc | Fixed (sharpened godoc, regenerated CRD/apply config) |
| 7 | Suggestion #6: `removeActionAnnotation` comment vs invalid-action branch | Fixed (softened comment) |
| 8 | Suggestion #7: No unit test for `dispatchContainerAction` default branch | Skipped |
| 9 | Testing Gap #1: No test for create-then-update bypass | Skipped (consistent with I1 commented, not fixed) |
| 10 | Testing Gap #2: No test for `!action.IsValid()` fallback | Skipped |
| 11 | Testing Gap #3: No test that `lastAction` persists across direct `docker stop` | Test written |
| 12 | Testing Gap #4: No test for concurrent annotation overwrite during dispatch | Skipped |
| 13 | Testing Gap #5: `lastAction` timestamps and overwrite behavior not asserted | Test written |
| 14 | Testing Gap #6: Restart double-dispatch race has no coverage | Skipped (acknowledged limitation) |

---

## Appendix: Original Reviews

### [Claude Opus 4.7]

Now I have enough context. Let me also quickly check one potentially overlooked area — how the invalid-annotation case interacts with retries when the webhook is offline:

I've completed my analysis. Let me write up the review now.

# Code Review: PR #316 — engine: replace Container.spec.state with action annotations

**Verdict:** Approve with minor fixes

## Executive Summary

This PR replaces the level-triggered `Container.spec.state` field with a one-shot edge-triggered annotation (`containers.rancherdesktop.io/action`) plus a new `status.lastAction` outcome field. The design is sound: it correctly recognizes that Docker (restart policy, direct `docker start/stop`) is a concurrent writer that a level-triggered mirror cannot co-own, and adopts edge semantics that compose naturally with Docker's API. The implementation handles crash-replay, informer-cache staleness (via pre-checks on pause/unpause), concurrent annotation updates (observed-vs-current comparison in `removeActionAnnotation`), and webhook bypass (IsValid check in the reconciler). Findings are limited to stale doc comments introduced by the rename and a couple of minor improvement suggestions.

## Findings

### Critical Issues

None.

### Important Issues

**I1. Stale comment references the removed function and field** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:183-184` (important, regression)

```go
// Two passes run per Reconcile: reconcileContainerSpecs drives
// Docker start/stop from Container.spec.state, and
// processFinalizers forwards K8s-side deletes to Docker for any
// mirror still carrying the mirror finalizer.
```

The function was renamed from `reconcileContainerSpecs` to `reconcileContainerActions` in this same PR (see line 199), and `Container.spec.state` no longer exists. The rest of the comment block (lines 188–197) was correctly updated to say "action annotation present" and "container action", but lines 183-184 were missed. A reader following this comment to understand what the Reconcile loop does will be looking for code that was deleted.

Fix:

```diff
-	// Two passes run per Reconcile: reconcileContainerSpecs drives
-	// Docker start/stop from Container.spec.state, and
+	// Two passes run per Reconcile: reconcileContainerActions drives
+	// Docker actions from the action annotation, and
 	// processFinalizers forwards K8s-side deletes to Docker for any
 	// mirror still carrying the mirror finalizer.
```

**I2. ContainerSpec doc comment is internally contradictory** — `pkg/apis/containers/v1alpha1/container_types.go:124-128` (important, gap)

```go
// ContainerSpec defines the configuration the container was created with.
// Container spec is empty today: actions are requested via the AnnotationAction
// annotation, not a level-triggered desired-state field. Docker's restart
// policy and out-of-band `docker start/stop` are competing writers, so a
// level-triggered desired state would fight them.
type ContainerSpec struct{}
```

Line 124 ("defines the configuration the container was created with") predates the PR but is now directly contradicted by lines 125-128, which the PR added. The PR extended the doc comment but did not revise the first line. The result reads as two inconsistent descriptions stacked on top of each other. A reader hitting the first line is led to expect the struct to describe created-time configuration — when in fact it is an empty placeholder. Line 124 should be deleted or rewritten.

Fix:

```diff
-// ContainerSpec defines the configuration the container was created with.
-// Container spec is empty today: actions are requested via the AnnotationAction
+// ContainerSpec is currently empty. Actions are requested via the AnnotationAction
 // annotation, not a level-triggered desired-state field. Docker's restart
 // policy and out-of-band `docker start/stop` are competing writers, so a
 // level-triggered desired state would fight them.
 type ContainerSpec struct{}
```

### Suggestions

**S1. Invalid-action fallback is silent** — `pkg/controllers/app/engine/controllers/docker_watcher.go:370-373` (suggestion, enhancement)

```go
if !action.IsValid() {
    log.Info("Dropping invalid container action annotation", "id", c.Name, "action", raw)
    return w.removeActionAnnotation(ctx, c, raw)
}
```

The webhook normally rejects invalid action values at admission, so this branch only fires if the webhook is bypassed (e.g., registered with `failurePolicy: Ignore`, or during the startup window before registration). When it does fire, the user gets no feedback — `lastAction` is never updated and the annotation just disappears. An `Info` log is invisible to a GUI client. Since `lastAction.Action` must satisfy the CRD enum, writing a `Failed` outcome with the bogus action as the stored value is not possible, but two alternatives are worth considering: (a) raise the log to `Error` level with a clear message, or (b) emit a Kubernetes Event on the Container so a UI can surface it. Low priority — the webhook catches the normal case.

**S2. Doc comment describes wrong timing semantics for `ObservedAt`** — `pkg/apis/containers/v1alpha1/container_types.go:88-89` (suggestion, gap)

```go
// ObservedAt is when the reconciler observed the action annotation.
// Backlog may delay this relative to the user's write time.
```

In `processContainerAction`, `observedAt := metav1.Now()` (docker_watcher.go:364) is captured after the annotation has already been seen via List and just before validation. It represents "when the reconciler began processing the action," not "when the annotation was observed." The distinction matters because processing can dispatch a multi-second Docker call (e.g., restart's grace period); if a caller times `CompletedAt - ObservedAt` it will roughly measure the dispatch duration, not the observe-to-process backlog. Either rename the field to `StartedAt` (or `ProcessingStartedAt`) or clarify the doc comment to "when the reconciler began processing the annotation." The design doc text ("the reconciler observed the action annotation") should match.

**S3. `removeActionAnnotation` retry on cache-backed Get may thrash when called from the invalid-action branch** — `pkg/controllers/app/engine/controllers/docker_watcher.go:372, 510-520` (suggestion, correctness)

```go
if !action.IsValid() {
    log.Info("Dropping invalid container action annotation", "id", c.Name, "action", raw)
    return w.removeActionAnnotation(ctx, c, raw)
}
```

Here `c` comes from the `ContainerList` retrieved via the cache-backed `List` in `reconcileContainerActions` (engine_reconciler.go:367). The doc on `removeActionAnnotation` (line 506-509) says "The caller passes a fresh Container from `patchContainerLastAction`" to explain why the first Patch attempt can succeed. That invariant holds for the normal path but not for the invalid-action branch — the stale `c.ResourceVersion` may 409-conflict on the first attempt, and the retry's `Get` also reads from the same informer cache. In practice the 50ms RetryOnConflict backoff usually lets the watch catch up, but the invariant the comment relies on is not actually satisfied on this branch. Consider either (a) doing a direct (non-cache) Get in the invalid-action branch before calling `removeActionAnnotation`, or (b) updating the `removeActionAnnotation` comment to note that callers may pass either a fresh Container or a cached one, and that the retry loop tolerates cache lag.

**S4. `dispatchContainerAction` default-case error will never surface as a test** — `pkg/controllers/app/engine/controllers/docker_watcher.go:457` (suggestion, testing)

```go
return fmt.Errorf("unknown container action %q", action)
```

The comment at line 406-408 accurately notes this branch triggers only if a new `ContainerAction` constant is added without updating the switch. Since `IsValid` and the webhook both gate input using the same enumeration, this path is unreachable under normal operation. It's cheap defensive coding and fine as-is, but a tiny unit test that constructs a value not in the switch (e.g., `ContainerAction("invalid-test")`) and asserts the error message would make the intent explicit and catch future drift between `IsValid` and the switch. Low priority.

## Design Observations

### Concerns

None that rise above the findings above.

### Strengths

- **Edge-triggered action model resolves the ownership conflict.** The core design insight — that Docker's restart policy and CLI writes make a level-triggered `spec.state` unworkable — is well articulated in the commit message and design doc, and the code faithfully implements it. The crash-replay story (dispatch before patching status, annotation persists on crash, idempotent replay for start/stop/pause/unpause) is coherent and the one exception (restart's double-dispatch) is called out honestly rather than hidden.
- **Pre-check for pause/unpause isolates the informer-cache race.** The asymmetric handling (pause skips on `paused`; unpause skips on `!paused` but also fails explicitly on `!running`) is unusual but the justification in the comment at lines 409-416 and lines 461-463 is accurate and deliberate. The unit-test-like BATS cases ("pause action on a stopped container records failure" and the symmetric unpause case) pin the asymmetry so regressions will surface.
- **Empty-spec simplification of the SSA dance is genuine cleanup.** Removing `finalizerFieldOwner` and collapsing `applyContainer` into a single ForceOwnership branch eliminates a subtle invariant (the old split had a real reason — releasing spec ownership would have pruned a required field); once spec is empty, the simpler model is demonstrably correct.
- **Webhook covers both admission paths.** Registering for Create (to reject hand-written Containers with action annotations) plus Update (to reject invalid action values) closes the two realistic bypass routes, and both paths have explicit BATS coverage.

## Testing Assessment

BATS coverage is solid. The new suite in `bats/tests/32-app-controllers/engine-docker.bats` exercises start, stop, pause/unpause toggle, pause-on-stopped failure, unpause-on-stopped failure, restart (with a pre/post `StartedAt` comparison that proves the container actually cycled), webhook rejection of an invalid action on update, and webhook rejection of the annotation on create. Gaps, in rough order of risk:

1. **No test for webhook-bypass drop-path** — the `!action.IsValid()` branch in `processContainerAction` (docker_watcher.go:370) has no integration test because the webhook catches invalid values first. Writing a CRD-bypassing test would require disabling the webhook temporarily, which is awkward in BATS. A unit test that constructs a Container with a bogus annotation value and calls `processContainerAction` directly would provide cheap coverage.
2. **No test for concurrent annotation update during dispatch** — the scenario where the user replaces the annotation value between the reconciler's List and `removeActionAnnotation` is documented as the reason for the `current != observed` check (lines 522-524) but has no test. Hard to race deterministically in BATS; possibly a unit test on `removeActionAnnotation` with a mock client returning different values on subsequent Gets.
3. **No test for the restart double-dispatch race** — the acknowledged limitation that cache lag can re-dispatch a restart has no coverage. Likely not worth adding given it's a known trade-off.
4. **No test that lastAction persists across a direct `docker stop`** — the doc comment on line 153-156 states "Persists ... regardless of any observable state changes (e.g. a direct `docker stop`) in between." but no BATS test verifies this invariant. Simple to add: request `start`, verify `lastAction=start/Succeeded`, `docker stop`, verify `lastAction` unchanged.

## Documentation Assessment

Design doc (`docs/design/api_containers.md`) is comprehensive and matches the implementation — the action table, crash-replay semantics, and failure example are all accurate. The stale first line of the `ContainerSpec` Go doc comment (I2) is the only notable issue; the comment in `engine_reconciler.go` (I1) is the other. The PR description's `state=InProgress|Succeeded|Failed` and `requestedAt` nomenclature don't match the actual code (`Succeeded|Failed` and `observedAt`), but the design doc and CRD are correct — only the commit message is outdated.

## Commit Structure

Single commit. The commit message accurately describes the change and motivation. History is clean.

## Acknowledged Limitations

- **Restart double-dispatch on crash or cache-lag replay** — `docker_watcher.go:353-355` ("Restart has no target state to match: a replay sends SIGTERM and waits the grace period a second time, which the controller cannot distinguish from a deliberate re-request."). The limitation is documented in both the code and the design doc; the PR makes an explicit trade (accept the edge case rather than add restart-specific deduplication state).
- **O(N) per child event in reconcile loops** — `engine_reconciler.go:188-192` ("Both passes issue List calls per reconcile … The long-term fix is to split these into per-resource reconcilers with watch predicates (deletionTimestamp set, action annotation present)."). Pre-existing; this PR adopts the existing pattern and updates the watch-predicate example to match the new annotation-driven work.
- **No CLI surface for setting actions** — PR review comment from @jandubois: "The feature exists solely for the benefit of the GUI. Terminal users can use `docker` and `nerdctl` to talk to the container engine directly." Consistent with the code, which exposes the action only via annotation (set through `rdd ctl annotate` in tests); no `rdd` subcommand was added.
- **Windows/WSL2 not covered** — `local_setup_file` in `engine-docker.bats:12-16` skips on Windows because the Docker socket access pattern is not yet wired up. Pre-existing scope limitation, unchanged by this PR.

## Coverage Summary

- `bats/tests/32-app-controllers/engine-docker.bats` — Reviewed, no issues. Comprehensive action-annotation coverage; see Testing Assessment for gaps.
- `docs/design/api_containers.md` — Reviewed, no issues. Matches the implementation.
- `pkg/apis/containers/v1alpha1/applyconfiguration/containers/v1alpha1/container.go` — Reviewed, no issues. Generated; `WithSpec` is dead code but auto-generated.
- `pkg/apis/containers/v1alpha1/applyconfiguration/containers/v1alpha1/containerlastaction.go` — Reviewed, no issues. Generated.
- `pkg/apis/containers/v1alpha1/applyconfiguration/containers/v1alpha1/containerspec.go` (deleted) — Trivial (file removal accompanying empty-struct).
- `pkg/apis/containers/v1alpha1/applyconfiguration/containers/v1alpha1/containerstatus.go` — Reviewed, no issues. Adds `WithLastAction`.
- `pkg/apis/containers/v1alpha1/applyconfiguration/utils.go` — Trivial. Generated kind-registration.
- `pkg/apis/containers/v1alpha1/container_types.go` — **I2, S2**
- `pkg/apis/containers/v1alpha1/zz_generated.deepcopy.go` — Reviewed, no issues. Generated; matches new types.
- `pkg/controllers/app/engine/controllers/docker_watcher.go` — **S1, S3, S4**
- `pkg/controllers/app/engine/controllers/engine_reconciler.go` — **I1**
- `pkg/controllers/app/engine/controllers/sync_containers.go` — Reviewed, no issues. The simplification (single ForceOwnership apply under `controllerName`) is sound now that spec is empty.
- `pkg/controllers/containers/container/container_controller.go` — Reviewed, no issues. Adding `Create` to Operations is the right change to match the new `ValidateCreate` logic. (Pre-existing note: the variable name `mutatingConfig` is misleading since `SetupWebhookForResource` creates a `ValidatingWebhookConfiguration` when only `Validator` is set; unchanged by this PR.)
- `pkg/controllers/containers/container/container_webhooks.go` — Reviewed, no issues. `ValidateCreate` rejects action annotations; `ValidateUpdate` enforces immutable (empty) spec and valid action values.
- `pkg/controllers/containers/container/crd.yaml` — Reviewed, no issues. Generated from the types; removes `spec.state` and top-level `required: [spec]`, adds `lastAction` under status with the correct action enum (matches Go enum).
- `pkg/controllers/mock/container_reconciler.go` — Reviewed, no issues. Dropping `WithSpec` is correct given the new empty struct.
---

### [Codex GPT 5.4]

### Executive Summary

This PR replaces `Container.spec.state` with a one-shot action annotation plus `status.lastAction`, and simplifies the mirror SSA/finalizer flow now that `spec` is empty. The main regression is that the new create webhook only blocks create-time annotations, so a hand-written `Container` can still be created first and then updated with a valid action, which defeats the stated safeguard and lets non-mirror objects drive Docker actions.

### Findings

**Critical Issues**

None.

**Important Issues**

I1. **Create-then-update bypasses the “hand-written Container cannot drive Docker” safeguard** — `pkg/controllers/containers/container/container_webhooks.go:27-31,43-50` (important, regression)

```go
func (c *immutableValidator) ValidateCreate(_ context.Context, newContainer *v1alpha1.Container) (warnings ctrlwebhookadmission.Warnings, err error) {
	if _, ok := newContainer.Annotations[v1alpha1.AnnotationAction]; ok {
		return nil, fmt.Errorf("%s annotation is not allowed on create", v1alpha1.AnnotationAction)
	}
	return nil, nil
}
```

`ValidateCreate()` at `container_webhooks.go:27-31` now allows arbitrary `Container` creates as long as the action annotation is absent, while `ValidateUpdate()` at `container_webhooks.go:47-50` accepts any later update that sets a valid action. That makes the new safeguard ineffective: a caller can create a placeholder `Container` first, then patch `metadata.annotations[containers.rancherdesktop.io/action]` on a second request. The engine path does not distinguish mirrors from hand-written objects: `reconcileContainerActions()` at `engine_reconciler.go:377-380` processes every annotated `Container`, and `processContainerAction()` at `docker_watcher.go:375-390` dispatches the Docker call against `c.Name`. The new comment at `container_webhooks.go:24-26` says the create rejection prevents this exact case, but the two-step flow still works.

Fix: enforce provenance on update and in the reconciler, not just on create. The practical fix is to write an engine-owned marker on mirror creation and require that marker before accepting or processing actions; create-time rejection alone is insufficient.

```diff
 if raw, ok := newContainer.Annotations[v1alpha1.AnnotationAction]; ok {
+	if !hasEngineMirrorMarker(oldContainer) {
+		return nil, fmt.Errorf("%s annotation is only allowed on engine-created mirrors", v1alpha1.AnnotationAction)
+	}
 	if !v1alpha1.ContainerAction(raw).IsValid() {
 		return nil, fmt.Errorf("invalid %s annotation %q", v1alpha1.AnnotationAction, raw)
 	}
 }
```

**Suggestions**

S1. **Comments still describe the removed `spec.state` design** — `pkg/controllers/app/engine/controllers/engine_reconciler.go:183-186` (suggestion, regression)

```go
// Two passes run per Reconcile: reconcileContainerSpecs drives
// Docker start/stop from Container.spec.state, and
// processFinalizers forwards K8s-side deletes to Docker for any
// mirror still carrying the mirror finalizer.
```

The comment at `engine_reconciler.go:183-186` still says this reconcile loop drives `Container.spec.state`, even though the code now calls `reconcileContainerActions()` at `engine_reconciler.go:199` and the whole PR removes `spec.state`. The same stale wording remains in the Godoc opener for `ContainerSpec` at `container_types.go:124-128` (“defines the configuration the container was created with”). In this repo the design docs are treated as the contract boundary, so stale comments like these are more than cosmetic: they point future changes back toward the old ownership model this PR is explicitly removing.

Fix: update the inline comments/Godoc to describe the annotation-triggered flow and the fact that `ContainerSpec` is intentionally empty.

### Design Observations

**Concerns**

- `(in-scope)` The action authorization rule is currently split between admission and reconciliation, but only the create-time half was updated. Provenance checks for mirror-only behaviors are safer when the reconciler also enforces them, because that removes any dependence on request shape (`create` vs `update`) and keeps the invariant attached to the side effect itself.

**Strengths**

- The value-match check in `removeActionAnnotation()` is a good choice. It preserves a superseding action written between dispatch and cleanup instead of accidentally deleting it.
- Moving from level-triggered desired state to one-shot actions is a better fit for Docker’s concurrent writers. It removes the old “mirror owns Docker state” assumption instead of trying to paper over it.

### Testing Assessment

1. The BATS coverage does not exercise the create-then-update path that bypasses the new create-time admission check: create a `Container` without the annotation, then patch in `containers.rancherdesktop.io/action` and assert the request is rejected.
2. There is no integration coverage for action handling on non-mirror `Container` objects. Given the new optional/empty `spec`, that distinction is now part of the correctness boundary.
3. The suite checks `lastAction.action` and `lastAction.state`, but it does not verify the timestamp fields or their overwrite behavior across consecutive actions.

### Documentation Assessment

The design doc update is directionally good, but some code comments/Godoc still describe the removed `spec.state` model:
- `pkg/controllers/app/engine/controllers/engine_reconciler.go:183-186`
- `pkg/apis/containers/v1alpha1/container_types.go:124-128`
- `pkg/controllers/containers/container/container_controller.go:81` still says the webhook “prevents all modification,” which is no longer true.

### Commit Structure

No concerns. The single commit is cohesive and the message matches the diff.

### Acknowledged Limitations

- **PR comment**: The author explicitly declined adding CLI support for container actions, stating that this feature is intended for the GUI and terminal users should talk to Docker/containerd directly. The current diff is consistent with that rationale: there is no new CLI surface, only API/controller/test coverage.
- **Code comment**: `pkg/controllers/app/engine/controllers/docker_watcher.go:204-205` notes: “TODO: add a periodic fullSync tick so dropped image/volume events self-heal without waiting for a watcher restart.” This PR does not materially worsen that limitation; it remains a known recovery gap for one-shot image/volume events.
- **Code comment / design note**: `pkg/controllers/app/engine/controllers/docker_watcher.go:349-355` documents that `restart` may replay after a crash because the controller cannot distinguish retry from re-request. The implementation and docs stay consistent with that stated limitation.

### Coverage Summary

- `bats/tests/32-app-controllers/engine-docker.bats` — Reviewed, no issues
- `docs/design/api_containers.md` — Reviewed, no issues
- `pkg/apis/containers/v1alpha1/applyconfiguration/containers/v1alpha1/container.go` — Reviewed, no issues
- `pkg/apis/containers/v1alpha1/applyconfiguration/containers/v1alpha1/containerlastaction.go` — Trivial
- `pkg/apis/containers/v1alpha1/applyconfiguration/containers/v1alpha1/containerspec.go` — Trivial
- `pkg/apis/containers/v1alpha1/applyconfiguration/containers/v1alpha1/containerstatus.go` — Trivial
- `pkg/apis/containers/v1alpha1/applyconfiguration/utils.go` — Trivial
- `pkg/apis/containers/v1alpha1/container_types.go` — S1
- `pkg/apis/containers/v1alpha1/zz_generated.deepcopy.go` — Trivial
- `pkg/controllers/app/engine/controllers/docker_watcher.go` — I1
- `pkg/controllers/app/engine/controllers/engine_reconciler.go` — I1, S1
- `pkg/controllers/app/engine/controllers/sync_containers.go` — Reviewed, no issues
- `pkg/controllers/containers/container/container_controller.go` — Reviewed, no issues
- `pkg/controllers/containers/container/container_webhooks.go` — I1
- `pkg/controllers/containers/container/crd.yaml` — Reviewed, no issues
- `pkg/controllers/mock/container_reconciler.go` — Reviewed, no issues

---

### [Gemini 3.1 Pro]

### Executive Summary

This change elegantly refactors the Container API from a level-triggered `spec.state` to an imperative, one-shot `AnnotationAction` annotation. This directly resolves the architectural friction between the control plane and Docker's native state mutations (e.g., restart policies and direct CLI commands) by removing competing writers. The implementation is remarkably robust: it correctly leverages Kubernetes primitives, relies on Docker's native idempotency for 304 Not Modified responses, bypasses the informer cache to safely manage annotation lifecycles, and cleanly drops the now-empty `Spec` from Server-Side Apply configurations to avoid clobbering user patches.

### Findings

#### Critical Issues
None.

#### Important Issues
None.

#### Suggestions
None.

### Design Observations

#### Strengths
- **One-Shot Imperative Design**: Moving from declarative level-triggers to one-shot annotations is the exact right modeling for a system where the underlying engine (Docker) is the source of truth and allows external mutation.
- **Idempotency & Replay Safety**: The design gracefully handles mid-flight crashes by retaining the annotation until the Docker action and the subsequent `LastAction` status write succeed. Explicit pre-checks for `pause` and `unpause` prevent failure flashes from duplicate cache reads.
- **Server-Side Apply Boundaries**: By restricting `applyContainer` to only apply the `finalizerOnly` configuration to the main resource, the `engine-controller` field manager completely relinquishes ownership of `metadata.annotations`. This cleanly avoids SSA conflict loops with the user or GUI writing the action annotation.
- **Webhook Completeness**: The validating webhook is correctly wired to intercept both `Create` and `Update` flows. This ensures that the empty `Spec` is truly immutable and hand-crafted resources cannot bypass the controller by front-loading an action on creation.

### Testing Assessment

1. **Concurrent Action Replacement**: The behavior where a user overwrites an annotation while an existing one is still in-flight is difficult to test natively in BATS. However, the logic in `removeActionAnnotation` securely guards against clobbering by validating the annotation value before deletion.
2. **Offline Webhook Bypass**: The `docker_watcher.go` controller code contains defensive validation to drop invalid annotations that manage to bypass an offline webhook. While untested in BATS, the code explicitly implements the guard.

### Documentation Assessment

The updates to `docs/design/api_containers.md` comprehensively explain the new action contract, the semantic meaning behind `LastAction`, and the removal of `Container.spec`. The CRD properties and their JSON examples are aligned.

### Commit Structure

Reviewed, no issues. The commits cleanly segregate the implementation and its supporting test modifications. 

### Acknowledged Limitations

- **Code comment**: `restart` action idempotency (`pkg/controllers/app/engine/controllers/docker_watcher.go`, lines 345-349). The author notes that "Restart has no target state to match: a replay sends SIGTERM and waits the grace period a second time, which the controller cannot distinguish from a deliberate re-request." This is an inherent limitation of the restart action, but safely contained.
- **Code comment**: O(N) Reconcile loop (`pkg/controllers/app/engine/controllers/engine_reconciler.go`, lines 189-192). "The long-term fix is to split these into per-resource reconcilers with watch predicates". This pre-existing limitation remains, as the controller processes action annotations across all containers sequentially.
- **PR comment**: Lack of CLI support for setting container actions. The author states: "I don't think we need any CLI support for setting container actions. The feature exists solely for the benefit of the GUI." This is a reasonable boundary; terminal users can use standard `docker` tooling.
- **Pre-existing limitation**: Docker socket access still needs wiring for Windows/WSL2. (Noted in the `bats/tests/32-app-controllers/engine-docker.bats` setup block). 

### Coverage Summary

- `bats/tests/32-app-controllers/engine-docker.bats`: Reviewed, no issues
- `docs/design/api_containers.md`: Reviewed, no issues
- `pkg/apis/containers/v1alpha1/applyconfiguration/containers/v1alpha1/container.go`: Reviewed, no issues
- `pkg/apis/containers/v1alpha1/applyconfiguration/containers/v1alpha1/containerlastaction.go`: Reviewed, no issues
- `pkg/apis/containers/v1alpha1/applyconfiguration/containers/v1alpha1/containerspec.go`: Reviewed, deleted file
- `pkg/apis/containers/v1alpha1/applyconfiguration/containers/v1alpha1/containerstatus.go`: Reviewed, no issues
- `pkg/apis/containers/v1alpha1/applyconfiguration/utils.go`: Trivial
- `pkg/apis/containers/v1alpha1/container_types.go`: Reviewed, no issues
- `pkg/apis/containers/v1alpha1/zz_generated.deepcopy.go`: Trivial
- `pkg/controllers/app/engine/controllers/docker_watcher.go`: Reviewed, no issues
- `pkg/controllers/app/engine/controllers/engine_reconciler.go`: Reviewed, no issues
- `pkg/controllers/app/engine/controllers/sync_containers.go`: Reviewed, no issues
- `pkg/controllers/containers/container/container_controller.go`: Reviewed, no issues
- `pkg/controllers/containers/container/container_webhooks.go`: Reviewed, no issues
- `pkg/controllers/containers/container/crd.yaml`: Reviewed, no issues
- `pkg/controllers/mock/container_reconciler.go`: Reviewed, no issues