# Deep Review: 20260420-095326-pr-328

| | |
|---|---|
| **Date** | 2026-04-20 10:02 |
| **Repo** | [rancher-sandbox/rancher-desktop-daemon](https://github.com/rancher-sandbox/rancher-desktop-daemon) |
| **Round** | 1 |
| **Author** | [@jandubois](https://github.com/jandubois) |
| **PR** | [#328](https://github.com/rancher-sandbox/rancher-desktop-daemon/pull/328) — engine: port docker_context to docker/cli ContextStore |
| **Branch** | `docker-cli-contextstore-eval` |
| **Commits** | `a0a752f` engine: port docker_context to docker/cli ContextStore |
| **Reviewers** | Claude Opus 4.7 (effort: xhigh), Codex GPT 5.4 (effort: high), Gemini 3.1 Pro (effort: default) |
| **Verdict** | **Merge as-is** — no critical or important issues; suggestions are polish |
| **Wall-clock time** | `12 min 3 s` |

## Executive Summary

The PR replaces the hand-rolled `~/.docker/contexts` writer with `docker/cli`'s `ContextStore` and `cli/config.ConfigFile`. The runtime path through `engine_reconciler.go` keeps its previous shape, the package tests pass, and the on-disk schema now tracks the upstream definitions. All three reviewers agreed: no critical or important findings, and the call to skip `cli/command` (and its OpenTelemetry chain) by passing a nil context-metadata `TypeGetter` is a notable design strength.

The remaining suggestions are documentation polish (one stale comment, two narrow comments, two unsurfaced behavior changes in the commit message) and one bit of dead defensive code (`errdefs.IsNotFound` guard that the upstream `Remove` path cannot satisfy).

## Critical Issues

None.

## Important Issues

None.

## Suggestions

S1. **`errdefs.IsNotFound` guard in `deleteDockerContext` is unreachable** — `pkg/controllers/app/engine/controllers/docker_context.go:72-81` [Claude Opus 4.7]

```go
func deleteDockerContext(name string) error {
    s, err := newContextStore()
    if err != nil {
        return err
    }
    if err := s.Remove(name); err != nil && !errdefs.IsNotFound(err) {
        return err
    }
    return nil
}
```

`store.ContextStore.Remove` calls `metadataStore.remove` and `tlsStore.remove`; both wrap `os.RemoveAll`, which returns nil when the path is absent. The non-nil wrappers (`fmt.Errorf("failed to remove metadata: %w", err)` and `failed to remove TLS data`) carry no `notFound` interface, so `errdefs.IsNotFound` cannot match.

I verified this against `cli@v29.2.0+incompatible/cli/context/store/{store.go:140-150,metadatastore.go:99-104,tlsstore.go:49-54}`. The "second delete is a no-op" assertion in `docker_context_test.go:68` passes because `Remove` returns nil unconditionally for a missing context, not because the guard fires.

Fix: drop the guard, or attach a one-line comment explaining the intent is defensive against a future upstream change.

```diff
-    if err := s.Remove(name); err != nil && !errdefs.IsNotFound(err) {
-        return err
-    }
-    return nil
+    return s.Remove(name)
```

S2. **`config.Load` now validates `auths` entries — corrupt config breaks reconcile** — `pkg/controllers/app/engine/controllers/docker_context.go:108-150` [Claude Opus 4.7]

`configfile.LoadFromReader` decodes every `auths[*].auth` value with `decodeAuth`, which fails on invalid base64 or on a decoded payload missing `:` (`cli@v29.2.0+incompatible/cli/config/configfile/file.go:95-112,265-285`). The hand-rolled code only parsed `currentContext` and ignored the rest. A user with one corrupt auth entry will now hit an error inside `getCurrentDockerContext` / `setCurrentDockerContext` / `clearCurrentDockerContext` where the old code stayed quiet.

Impact is low and self-healing — one log line per reconcile until the user fixes their config — and the trade is reasonable for the smaller surface. Worth a sentence in the commit message and consider logging at info rather than error in `manageDockerContext` so a bad user-side config does not look like an `rdd` fault.

S3. **`Description` metadata is a new user-visible field with no test pinning it** — `pkg/controllers/app/engine/controllers/docker_context.go:63-69` [Claude Opus 4.7]

```go
return s.CreateOrUpdate(store.Metadata{
    Name:     name,
    Metadata: map[string]any{"Description": "Rancher Desktop " + name},
    Endpoints: map[string]any{
        docker.DockerEndpoint: docker.EndpointMeta{Host: "unix://" + socketPath},
    },
})
```

The previous code passed `Metadata: map[string]any{}`. The new code injects a `Description`, which `docker context ls` surfaces as a column. Probably welcome, but the change is not mentioned in the commit message and no test reads it back, so a future refactor could drop it silently.

Fix: extend `Test_createReplaceDockerContext` to read the metadata back and assert the description round-trips, or note the intent in the commit message. The BATS test `bats/tests/32-app-controllers/engine-docker.bats:576` (`moby engine creates Docker context for the instance`) is a natural place to assert the field on an integration run.

S4. **Context metadata permissions loosened from `0o700/0o600` to `0o755/0o644`** — `pkg/controllers/app/engine/controllers/docker_context.go:58-70` [Claude Opus 4.7]

The pre-PR `updateDockerConfig` (commit `d1c228f` line 60/74) used `MkdirAll(..., 0o700)` and `WriteFile(..., 0o600)`. The new path delegates to `metadataStore.createOrUpdate`, which uses `MkdirAll(contextDir, 0o755)` and `atomicwriter.WriteFile(..., 0o644)` (`cli@v29.2.0+incompatible/cli/context/store/metadatastore.go:35,42`). The contents are only socket paths — not sensitive — and the new modes match Docker CLI's defaults, so this is almost certainly intentional interop. Worth calling out so a future security scan does not flag it as a regression and so the rationale lives somewhere outside reviewer memory.

Fix: a sentence in the commit message noting the permission alignment with docker CLI is enough.

S5. **`getDockerContextHost` comment narrower than the behavior** — `pkg/controllers/app/engine/controllers/docker_context.go:98-103` [Claude Opus 4.7]

```go
ep, err := docker.EndpointFromContext(md)
if err != nil {
    // No docker endpoint on this context — treat as empty, not an error.
    return "", nil
}
```

`docker.EndpointFromContext` (`cli@v29.2.0+incompatible/cli/context/docker/load.go:152-160`) returns an error in two cases: the endpoint is absent (`"cannot find docker endpoint in context"`) and the endpoint value has the wrong type (`"endpoint %q is not of type EndpointMeta"`). The comment only covers the first case. The configured `EndpointTypeGetter` should keep the type assertion succeeding, but a future reader who relies on the comment would be misled.

Fix: broaden the comment.

```diff
-        // No docker endpoint on this context — treat as empty, not an error.
+        // Missing or mistyped docker endpoint — treat as empty, not an error.
+        // The configured EndpointTypeGetter normally prevents the mistyped case.
```

S6. **`dockerConfigDir` doc comment omits the `DOCKER_CONFIG` override** — `pkg/controllers/app/engine/controllers/docker_context.go:24-36` [Codex GPT 5.4]

```go
// dockerConfigDir returns ~/.docker. We compute this ourselves rather than
// calling config.Dir() because that caches its result via sync.Once, which
// defeats t.Setenv("HOME", …) in tests.
func dockerConfigDir() (string, error) {
    if dir := os.Getenv("DOCKER_CONFIG"); dir != "" {
        return dir, nil
    }
```

Every helper in this file routes through `dockerConfigDir`. The function honours `DOCKER_CONFIG` first, but the comment still says "returns `~/.docker`" with no mention of the env override.

Fix: state the precedence explicitly.

```diff
-// dockerConfigDir returns ~/.docker. We compute this ourselves rather than
-// calling config.Dir() because that caches its result via sync.Once, which
-// defeats t.Setenv("HOME", …) in tests.
+// dockerConfigDir returns $DOCKER_CONFIG when it is set, otherwise ~/.docker.
+// We compute this ourselves rather than calling config.Dir() because that
+// caches its result via sync.Once, which defeats t.Setenv("HOME", …) in tests.
```

S7. **`set preserves other keys` test seeds a known field, not an unknown one** — `pkg/controllers/app/engine/controllers/docker_context_test.go:102-115` [Gemini 3.1 Pro]

The test's comment reads `// Seed a config with an unrelated field.`, but `auths` is a typed top-level field that `ConfigFile` knows about. The PR description acknowledges that unknown top-level keys are dropped on save. Seeding with a truly unknown field (e.g. `{"myCustomField": "value"}`) would document that limitation directly; the current test only proves a known field round-trips. Comment vs. intent mismatch — no logic failure.

Fix: rename the seed to a known-irrelevant field (`{"experimental":"enabled"}`) and update the comment, or add a second sub-test that asserts an unknown field is dropped.

## Design Observations

### Strengths

- **Skipping `cli/command` via a nil context-metadata `TypeGetter` (in-scope)** [Claude Opus 4.7, Codex GPT 5.4, Gemini 3.1 Pro] — `store.NewConfig(nil, ...)` decodes `Metadata` as `map[string]any`, which is all the caller needs. It avoids importing `cli/command` and the entire `otel/sdk/metric`, `go-metrics`, `gorilla/mux`, `backoff/v5`, `morikuni/aec` chain. This is the design call that makes the port net-positive on binary size.

- **Atomic write semantics inherited from upstream (in-scope)** [Gemini 3.1 Pro] — the old hand-rolled writer used a bare `os.Rename` after a `Chmod`. The new path delegates to `moby/sys/atomicwriter`, which resolves symlinks and matches Docker CLI's behavior on Windows and Unix. Closes a few edge cases the old code did not handle.

- **Test isolation against `docker/cli`'s env caching (in-scope)** [Claude Opus 4.7, Gemini 3.1 Pro] — `newDockerTestDir` sets `HOME`, `USERPROFILE`, and clears `DOCKER_CONFIG` in one place. The `dockerConfigDir` shim that bypasses `config.Dir()`'s `sync.Once` is a small but pragmatic workaround documented inline.

### Concerns

None raised by any reviewer.

## Testing Assessment

Unit coverage is adequate for the happy paths (create/replace, delete, missing-lookup, set/get/clear, key preservation on save). Gaps that survived consolidation:

1. **No assertion on the new `Description` metadata** — see S3. Easiest user-visible regression to introduce later.
2. **No test for the `DOCKER_CONFIG` override branch** — `newDockerTestDir` clears that variable, so the new code path at `docker_context.go:28-29` never executes in the unit suite.
3. **No test for the short-circuit at `docker_context.go:129-131` / `145-147`** — `setCurrentDockerContext` and `clearCurrentDockerContext` skip the save when the context already matches. A seeded file plus an mtime check would pin the behavior.
4. **No test for malformed `auths` in `config.json`** — see S2. A single seed-file test would document the new failure mode.
5. **No test for `clearCurrentDockerContext` preserving other keys** — `Test_currentDockerContext` proves preservation for `setCurrentDockerContext` but not the clear path called at shutdown.

## Documentation Assessment

- The exported (well, package-private) symbols touched all carry doc comments, and the new `dockerConfigDir` and `newContextStore` comments correctly explain *why* they deviate from upstream defaults — a pattern worth keeping.
- Three doc comments drifted: see S5 (`getDockerContextHost`), S6 (`dockerConfigDir`).
- The commit message accurately covers the dependency impact and the dropped-keys caveat. Consider extending it with the S2 (corrupt `auths`) and S4 (permission change) observations.

## Commit Structure

Single, self-contained commit with a descriptive subject. No concerns.

## Acknowledged Limitations

- **Commit message / PR description**: "Saving through docker/cli's typed `ConfigFile` drops unknown top-level keys from `config.json` and injects empty `auths` and `credsStore` fields. `docker context use` does the same today, so users of Docker CLI already see this behavior."
  - The "drops unknown keys" half is accurate — `ConfigFile` is a closed struct, so anything not in the Go definition is lost on save.
  - The "injects `credsStore`" half is slightly overstated. `ConfigFile.CredentialsStore` carries `json:"credsStore,omitempty"` (`cli/config/configfile/file.go:30`), so an empty value is omitted. Only `"auths"` (no `omitempty`) gets written when previously absent. Not a finding — just a small inaccuracy in the rationale.

## Agent Performance Retro

### [Claude]

Did the heavy upstream tracing — every substantive claim (S1–S5) cited the exact upstream file and line, and all four spot-checks against `docker/cli@v29.2.0+incompatible` matched what I verified independently. Caught the only finding any reviewer raised that was not pure documentation: the unreachable `errdefs.IsNotFound` guard. Also surfaced the permission change and the auth-validation regression that the other two reviewers missed.

### [Codex]

Single suggestion (the `dockerConfigDir` doc drift) was real and clean, with a tight diff. Built `make build-rdd` and ran the package tests before reporting — the only reviewer to mention live verification. Coverage was correct but narrow: missed the upstream-behavior consequences (S1, S2, S4, S5) entirely, leaning on "I traced the only caller" as sufficient.

### [Gemini]

Produced no findings, only design observations and one comment-vs-intent quibble (S7). Did not perform `git blame` or upstream tracing (the known daily-quota gap), which is consistent with missing the unreachable guard and the permission/auth changes. The unknown-vs-known-field observation is a sharp framing point worth keeping even though it is not a bug.

### Summary

| | Claude Opus 4.7 | Codex GPT 5.4 | Gemini 3.1 Pro |
|---|---|---|---|
| Duration | 6m 06s | 4m 45s | 5m 17s |
| Findings | 5S | 1S | 1S |
| Tool calls | 27 (Bash 19, Read 5, Grep 3) | 43 (shell 40, stdin 3) | 40 (run_shell_command 30, grep_search 6, read_file 4) |
| Design observations | 3 | 2 | 3 |
| False positives | 0 | 0 | 0 |
| Unique insights | 5 | 1 | 1 |
| Files reviewed | 4 | 4 | 4 |
| Coverage misses | 0 | 0 | 0 |
| **Totals** | **5S** | **1S** | **1S** |
| Downgraded | 0 | 0 | 0 |
| Dropped | 0 | 0 | 0 |


Claude carried this review. The other two were directionally correct (clean PR, no blockers) but only Claude's upstream tracing turned up actionable suggestions beyond comment polish.

## Review Process Notes

### Skill improvements

- **Flag dead defensive code that depends on upstream sentinel errors.** When the reviewed code uses `errdefs.IsNotFound`, `errors.Is(err, os.ErrNotExist)`, or similar guards on errors returned from a third-party library, verify by reading the upstream that the guarded sentinel can actually surface from that call site. Wrappers like `fmt.Errorf("failed to ...: %w", err)` often hide the original interface, and `os.RemoveAll` on a missing path returns nil with no error at all. A guard that cannot fire is dead code; flag it as a suggestion with a citation to the upstream source.

- **Verify file/dir permission changes when delegating writes to a library.** When a hand-rolled writer using `MkdirAll(..., 0o7XX)` / `WriteFile(..., 0o6XX)` is replaced by a library call, the new mode is whatever the library uses. Diff the old constants against the library's defaults and surface any change. Even when the contents are not sensitive, an unexplained mode change in a security-relevant directory will trip future security scans and reviewer questions.

### Repo context updates

None. The existing context entries held up against this review.

## Appendix: Original Reviews

### [Claude Opus 4.7]

Now I have enough context. Let me finalize the review.

# Deep Review: PR #328 — engine: port docker_context to docker/cli ContextStore

## Executive Summary

This PR replaces the hand-rolled `~/.docker/contexts` writers with the upstream `docker/cli` `ContextStore` and `cli/config.ConfigFile`. The port is small, well-factored, and keeps the on-disk schema interoperable with `docker context use`. The rollout is clean; the issues found are minor.

## Findings

### Critical Issues

None.

### Important Issues

None.

### Suggestions

**S1. `errdefs.IsNotFound` guard in `deleteDockerContext` is unreachable** — `pkg/controllers/app/engine/controllers/docker_context.go:72-81` (suggestion, regression)

```go
func deleteDockerContext(name string) error {
    s, err := newContextStore()
    if err != nil {
        return err
    }
    if err := s.Remove(name); err != nil && !errdefs.IsNotFound(err) {
        return err
    }
    return nil
}
```

`store.ContextStore.Remove` (store.go) calls `metadataStore.remove` and `tlsStore.remove`, both of which use `os.RemoveAll` — that never returns `ErrNotExist` for a missing path. The only errors it surfaces are genuine I/O failures, wrapped by `fmt.Errorf("failed to remove ...: %w", err)`, none of which satisfy `errdefs.IsNotFound`. The comment-less idiom reads like a real sentinel, but it can't fire. The test at line 68 of `docker_context_test.go` passes because the second `Remove` already returns `nil` unconditionally.

Fix: drop the guard so the intent is explicit, or add a comment explaining that it's defensive against a future upstream change.

```diff
-    if err := s.Remove(name); err != nil && !errdefs.IsNotFound(err) {
-        return err
-    }
-    return nil
+    return s.Remove(name)
```

**S2. `getDockerContextHost` comment narrower than the behavior** — `pkg/controllers/app/engine/controllers/docker_context.go:98-102` (suggestion, regression)

```go
ep, err := docker.EndpointFromContext(md)
if err != nil {
    // No docker endpoint on this context — treat as empty, not an error.
    return "", nil
}
```

`docker.EndpointFromContext` (cli/context/docker/load.go) returns an error in two cases: the endpoint is absent (`"cannot find docker endpoint in context"`) *and* the endpoint value is the wrong type (`"endpoint %q is not of type EndpointMeta"`). The comment only covers the first case, so the second is silently swallowed. In practice the configured `EndpointTypeGetter` guarantees the type assertion succeeds, but the comment misleads a future reader who might rely on it.

Fix: broaden the comment, or log at debug level when `err` is not the "not found" case.

```diff
-        // No docker endpoint on this context — treat as empty, not an error.
+        // Missing or mistyped docker endpoint — treat as empty, not an error.
+        // The configured EndpointTypeGetter normally prevents the mistyped case.
         return "", nil
```

**S3. New `Description` metadata is a user-visible change with no test coverage** — `pkg/controllers/app/engine/controllers/docker_context.go:63-69` (suggestion, regression)

```go
return s.CreateOrUpdate(store.Metadata{
    Name:     name,
    Metadata: map[string]any{"Description": "Rancher Desktop " + name},
    Endpoints: map[string]any{
        docker.DockerEndpoint: docker.EndpointMeta{Host: "unix://" + socketPath},
    },
})
```

The old code passed `Metadata: map[string]any{}`; the new code injects a `Description`. `docker context ls` surfaces this field as a column, so the change is user-visible. It's probably a welcome addition, but it's not mentioned in the commit message and has no test pinning the value. A future refactor could drop it silently.

Fix: either add an assertion in `Test_createReplaceDockerContext` (read metadata back, confirm the Description round-trips) or note the intent in the commit message / a code comment. BATS test `moby engine creates Docker context for the instance` (`bats/tests/32-app-controllers/engine-docker.bats:576`) is a natural place to assert it on an integration run.

**S4. Context metadata permissions loosened from `0o700/0o600` to `0o755/0o644`** — `pkg/controllers/app/engine/controllers/docker_context.go:58-70` (suggestion, regression)

The old hand-rolled writer used `MkdirAll(..., 0o700)` and `os.WriteFile(..., 0o600)`. The new path delegates to `store.metadataStore.createOrUpdate` which uses `0o755` for the dir and `0o644` (via `moby/sys/atomicwriter`) for `meta.json`. The contents are only the socket path — not sensitive — and these values match docker CLI's own defaults, so this is almost certainly intentional interoperability. Worth calling out because it wasn't flagged in the commit message and a security scanner may pick it up.

Fix: if intentional, a sentence in the commit message noting the permission alignment with docker CLI would save a future reader from wondering. Otherwise no change.

**S5. New dependency on `config.Load` validating `auths`** — `pkg/controllers/app/engine/controllers/docker_context.go:108-150` (suggestion, regression)

```go
cf, err := config.Load(dir)
if err != nil {
    return "", err
}
return cf.CurrentContext, nil
```

`configfile.LoadFromReader` calls `decodeAuth(ac.Auth)` for every entry under `auths` and returns an error if any value is not valid base64 of `user:pass`. The old code only parsed `currentContext` and ignored the rest of the file. A user with a corrupt auth entry will now see `getCurrentDockerContext` / `setCurrentDockerContext` fail where they used to succeed. Impact is low (one log line per reconcile, self-heals once the user fixes their config) and the tradeoff is reasonable for a small surface. Worth a line in the commit message.

Fix: no code change needed. Consider logging at info, not error, in `manageDockerContext` if you want to avoid noisy error logs for a user-side problem outside rdd's control.

## Design Observations

### Strengths

- The `dockerConfigDir` shim (lines 24-36) is a pragmatic workaround for `config.Dir()`'s `sync.Once` caching, with a short comment explaining exactly why it exists. It also handles `DOCKER_CONFIG`, which the old hand-rolled path did not — an incidental improvement.
- Choosing a `nil` context-metadata `TypeGetter` to sidestep the `cli/command` chain (noted in code and commit message) is a good judgment call: it keeps the binary lean without losing interop with `docker context`.
- The test refactor (single `newDockerTestDir` helper that sets `HOME`, `USERPROFILE`, and `DOCKER_CONFIG`) correctly handles both Unix and Windows lookup paths and keeps each subtest small.

## Testing Assessment

Unit test coverage is adequate for the happy paths (create/replace, delete, missing lookup, set/get/clear, other-key preservation). Gaps, ranked:

1. **No test for the `Description` metadata** — the most user-visible new behavior. See S3.
2. **No test for the short-circuit** at `docker_context.go:129-131` / `145-147` (set/clear when `currentContext` already matches). Easy to add: write a seed file, call `setCurrentDockerContext` with the same name, confirm the file mtime didn't change.
3. **No test for malformed `auths` in config.json** — see S5. A single seed-file test that writes `{"auths":{"example.com":{"auth":"not-base64"}}}` and asserts `getCurrentDockerContext` returns an error would document the new behavior.
4. **Windows code path is not exercised.** `manageDockerContext` returns early on Windows (`runtime.GOOS == "windows"`), so the store functions are never called on that platform — but the test helper sets `USERPROFILE` just in case. Fine as is.

## Documentation Assessment

- The exported symbols touched (`getDockerContextHost`, `getCurrentDockerContext`) keep their docstrings and the new doc comment on `dockerConfigDir` and `newContextStore` explain *why* they deviate from upstream — good practice.
- The commit message accurately covers the deps impact, the dropped-keys caveat, and the reason the `TypeGetter` is nil. Consider extending it with S4/S5 notes.

## Commit Structure

Single clean commit, descriptive subject, self-contained. No concerns.

## Acknowledged Limitations

- **Commit message, PR description**: "Saving through docker/cli's typed `ConfigFile` drops unknown top-level keys from `config.json` and injects empty `auths` and `credsStore` fields. `docker context use` does the same today, so users of Docker CLI already see this behavior."
  - The "drops unknown keys" half is accurate — `ConfigFile` is a closed struct so any field not in the Go definition is lost on round-trip. The "injects `credsStore`" half is slightly overstated: `CredentialsStore` carries `json:"credsStore,omitempty"` (cli/config/configfile/file.go:30), so an empty value is not written. Only `"auths"` (no `omitempty`) is injected. Not a finding — just a minor inaccuracy in the rationale.

## Coverage Summary

- `go.mod` — Trivial (direct-dep promotion + transitive bumps as described in commit message)
- `go.sum` — Trivial (matches go.mod)
- `pkg/controllers/app/engine/controllers/docker_context.go` — S1, S2, S3, S4, S5
- `pkg/controllers/app/engine/controllers/docker_context_test.go` — S3 (missing Description assertion); otherwise reviewed, no issues

### [Codex GPT 5.4]

### Executive Summary

This PR replaces the bespoke Docker context metadata/config writers with Docker CLI's `ContextStore` and typed `ConfigFile`. I did not find a functional regression in the runtime path after tracing the only caller in `engine_reconciler.go`; the package tests and `make build-rdd` both passed locally. The only issue I found is a stale comment in `dockerConfigDir()` that no longer describes the new `DOCKER_CONFIG` override behavior.

### Findings

**Critical Issues**

None.

**Important Issues**

None.

**Suggestions**

S1. **`dockerConfigDir` comment no longer matches the function's behavior** — `pkg/controllers/app/engine/controllers/docker_context.go:24-29` (suggestion, regression)

```go
// dockerConfigDir returns ~/.docker. We compute this ourselves rather than
// calling config.Dir() because that caches its result via sync.Once, which
// defeats t.Setenv("HOME", …) in tests.
func dockerConfigDir() (string, error) {
    if dir := os.Getenv("DOCKER_CONFIG"); dir != "" {
        return dir, nil
    }
```

The comment at lines 24-26 still says `dockerConfigDir()` returns `~/.docker`, but line 28 now prefers `DOCKER_CONFIG` when it is set. That mismatch is small but real: every config/context helper in this file now routes through `dockerConfigDir()` (lines 42, 109, 121, and 137), so future readers will look in the wrong place when debugging env-specific behavior or extending the tests.

Fix: update the comment so it documents the env override explicitly.

```diff
-// dockerConfigDir returns ~/.docker. We compute this ourselves rather than
-// calling config.Dir() because that caches its result via sync.Once, which
-// defeats t.Setenv("HOME", …) in tests.
+// dockerConfigDir returns $DOCKER_CONFIG when it is set, otherwise ~/.docker.
+// We compute this ourselves rather than calling config.Dir() because that
+// caches its result via sync.Once, which defeats t.Setenv("HOME", …) in tests.
```

### Design Observations

**Strengths**

Using Docker CLI's `ContextStore` and `ConfigFile` here is the right abstraction. It removes the hand-rolled on-disk format logic and delegates atomic writes, hashing/layout, and env-var override behavior to the implementation Rancher Desktop is trying to interoperate with.

The `newContextStore()` setup is also a good compromise: `store.EndpointTypeGetter()` keeps endpoint decoding typed and compatible, while the nil context-metadata getter avoids pulling in the much larger `cli/command` dependency chain.

### Testing Assessment

1. The new `DOCKER_CONFIG` branch at `docker_context.go:28-29` is untested. `newDockerTestDir()` explicitly clears that variable in `docker_context_test.go:26-27`, so the new override path never executes in the unit suite.
2. The clear path is not checked for config preservation. `docker_context_test.go:102-115` verifies that `setCurrentDockerContext()` keeps unrelated keys, but there is no matching assertion for `clearCurrentDockerContext()`, even though shutdown calls that path in `engine_reconciler.go:355`.

### Documentation Assessment

S1 is the only documentation drift I found. I did not find any design-doc mismatch requiring updates in `docs/design/api_containers.md`.

### Coverage Summary

- `go.mod` — Reviewed, no issues
- `go.sum` — Trivial
- `pkg/controllers/app/engine/controllers/docker_context.go` — S1
- `pkg/controllers/app/engine/controllers/docker_context_test.go` — Reviewed, no issues

### [Gemini 3.1 Pro]

**Executive Summary**

This PR replaces the custom, hand-rolled Docker context management with `docker/cli`'s standard `ContextStore`. The port correctly aligns the metadata layout and atomic file writes with Docker CLI's upstream logic, eliminating potential Windows-specific path and renaming issues. The design employs a clever workaround to bypass heavy upstream telemetry dependencies, and the testing isolation perfectly handles `docker/cli`'s environment variable caching.

### Findings

#### Critical Issues
None.

#### Important Issues
None.

#### Suggestions
None.

### Design Observations

**Strengths**
- **Dependency Optimization:** Passing `nil` for the `TypeGetter` in `store.NewConfig` to cause `docker/cli` to decode `Metadata` as a `map[string]any` is an exceptionally elegant architectural decision. It avoids pulling in `cli/command` and its heavy transitive dependency chain (`otel/sdk/metric`, `go-metrics`, `gorilla/mux`, etc.) while fully leveraging the upstream store logic.
- **Improved Storage Semantics:** The old hand-rolled `updateDockerConfig` relied on a naive `os.Rename` for its atomic write, which can fail or behave differently on Windows depending on file locks and existing destinations. Delegating the save operation to `config.Save()` adopts Docker's `atomicwriter` and natively resolves symlinks, closing several edge-case windows.
- **Test Isolation:** Explicitly unsetting `DOCKER_CONFIG` and overriding both `HOME` and `USERPROFILE` in `newDockerTestDir` robustly guards against test state leaking into or being clobbered by the host's actual `docker/cli` environment variables.

### Testing Assessment

The test suite accurately maps the behavioral changes of the new upstream library. Tests safely share a configuration directory by executing sequentially and manage state boundaries effectively.

1. **Test Naming:** The `Test_currentDockerContext` test uses `{"auths":{"example.com":{}}}` to verify existing keys are retained. While the test's comment states `// Seed a config with an unrelated field.`, `auths` is actually a *known* top-level field in `docker/cli`. Testing with a truly unknown field (e.g. `{"myCustomField": "value"}`) would show that unknown fields are dropped, consistent with the acknowledged limitation. This is a semantic mismatch in the comment, not a logic failure.

### Acknowledged Limitations

- **PR comment**: The PR author explicitly states: "Saving through docker/cli's typed ConfigFile drops unknown top-level keys from config.json and injects empty auths and credsStore fields. docker context use does the same today, so users of Docker CLI already see this behavior." The implementation aligns correctly with this expected behavior.

### Coverage Summary

- `go.mod` — Reviewed, no issues
- `go.sum` — Reviewed, no issues
- `pkg/controllers/app/engine/controllers/docker_context.go` — Reviewed, no issues
- `pkg/controllers/app/engine/controllers/docker_context_test.go` — Reviewed, no issues

## Resolution

| | |
|---|---|
| **Addressed** | 2026-04-20 |
| **Commit** | `0640c9c engine: port docker_context to docker/cli ContextStore` (fixup autosquashed into the PR's single commit) |

| # | Finding | Action |
|---|---------|--------|
| 1 | Suggestion #1: `errdefs.IsNotFound` guard in `deleteDockerContext` is unreachable | Fixed |
| 2 | Suggestion #2: `config.Load` now validates `auths` entries — corrupt config breaks reconcile | Commented |
| 3 | Suggestion #3: `Description` metadata is a new user-visible field with no test pinning it | Test written |
| 4 | Suggestion #4: Context metadata permissions loosened from `0o700/0o600` to `0o755/0o644` | Skipped |
| 5 | Suggestion #5: `getDockerContextHost` comment narrower than the behavior | Fixed |
| 6 | Suggestion #6: `dockerConfigDir` doc comment omits the `DOCKER_CONFIG` override | Fixed |
| 7 | Suggestion #7: `set preserves other keys` test seeds a known field, not an unknown one | Fixed |
| 8 | Testing Gap #1: No assertion on the new `Description` metadata | Test written (folded into #3) |
| 9 | Testing Gap #2: No test for the `DOCKER_CONFIG` override branch | Test written |
| 10 | Testing Gap #3: No test for the set/clear short-circuit when context already matches | Skipped |
| 11 | Testing Gap #4: No test for malformed `auths` in `config.json` | Test written |
| 12 | Testing Gap #5: No test for `clearCurrentDockerContext` preserving other keys | Test written |
| 13 | Documentation Gap: Commit message could extend with S2 and S4 observations | Skipped |