sync docker credentials to container

[areebahmeddd]

Summary

after docker login runs while the model runner container is already up, the container continues using a stale copy of ~/.docker/config.json until restarted.

before each push, pull, or compose model download, copy the host's ~/.docker/config.json into the container so the latest credentials are used. this only applies to docker engine (moby), since desktop and cloud manage credentials independently.

Fixes #943

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• In SyncDockerConfigToContainer, the guard condition still returns early when _MODEL_RUNNER_TREAT_DESKTOP_AS_MOBY is set, which contradicts the intent of “treat desktop as moby”; consider moving the env check outside the early-return for Desktop/Cloud or updating the comment/behavior to match the intended semantics.
• syncDockerConfigForRegistry silently returns on DockerClientForContext errors, which may make diagnosing context/connection issues harder; consider emitting a brief warning or log entry in that case, similar to the warning when SyncDockerConfigToContainer fails.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In SyncDockerConfigToContainer, the guard condition still returns early when `_MODEL_RUNNER_TREAT_DESKTOP_AS_MOBY` is set, which contradicts the intent of “treat desktop as moby”; consider moving the env check outside the early-return for Desktop/Cloud or updating the comment/behavior to match the intended semantics.
- syncDockerConfigForRegistry silently returns on DockerClientForContext errors, which may make diagnosing context/connection issues harder; consider emitting a brief warning or log entry in that case, similar to the warning when SyncDockerConfigToContainer fails.

## Individual Comments

### Comment 1
<location path="cmd/cli/pkg/standalone/containers.go" line_range="31-35" />
<code_context>
-// and sets up proper ownership and permissions for the modelrunner user.
-// It does nothing for Desktop and Cloud engine kinds.
-func copyDockerConfigToContainer(ctx context.Context, dockerClient *client.Client, containerID string, engineKind types.ModelRunnerEngineKind) error {
+// SyncDockerConfigToContainer copies the host's ~/.docker/config.json into the
+// running container. It is a no-op for Desktop and Cloud engine kinds.
+func SyncDockerConfigToContainer(ctx context.Context, dockerClient *client.Client, containerID string, engineKind types.ModelRunnerEngineKind) error {
 	// Do nothing for Desktop and Cloud engine kinds
 	if engineKind == types.ModelRunnerEngineKindDesktop || engineKind == types.ModelRunnerEngineKindCloud ||
 		os.Getenv("_MODEL_RUNNER_TREAT_DESKTOP_AS_MOBY") == "1" {
</code_context>
<issue_to_address>
**issue (bug_risk):** Environment flag handling contradicts the name and comment and may disable syncing on Moby unintentionally.

The implementation short-circuits whenever `_MODEL_RUNNER_TREAT_DESKTOP_AS_MOBY == "1"`, regardless of `engineKind`, so `ModelRunnerEngineKindMoby` with this env var set will also skip syncing. To avoid unintentionally disabling Moby syncing, consider limiting the env override to the Desktop case only (e.g. `if engineKind == Desktop && env == "1" { ... }`), or rename the flag/comment to accurately describe the behavior.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[gemini-code-assist]

Code Review

This pull request introduces host Docker configuration synchronization into the running container for Moby engine setups during up, pull, and push commands, exporting the SyncDockerConfigToContainer function and adding corresponding unit tests. The review feedback correctly identifies two critical nil pointer dereference risks: one involving dockerCLI in utils.go, and another involving dockerClient in containers.go, both of which require defensive nil checks to ensure stability.

[sathiraumesh]

I checked with the previous implementation and the current one they have different ways internally for example if someone hase set DOCKER_CONFIG env it would break. May be this current change is the correct way need to check with the authors @ericcurtin @ilopezluna

[ericcurtin]

I suggest comparing against what moby/containerd does and copying that

[areebahmeddd]

i used config.Dir() from docker/cli here because that's the same logic docker uses to locate its config directory

for the moby/containerd point.. my understanding is that this part is handled on the cli side rather than by the daemon itself

[sathiraumesh]

@areebahmeddd yes this might be the normal way but always make sure nothing breaks for backwards compatibility. if you check the implementations they differ a bit that's why i pointed it out.

[areebahmeddd]

could you point out for me? 🤔