refactor(docker): split driver into cohesive files; fix label match in FindContainerJSON#521
Merged
Conversation
…ures Split pkg/driver/docker/docker.go (1322 LOC) into responsibility-grouped files matching the package's existing build.go/diagnostics.go convention: - docker.go: struct, factory, core helpers - lifecycle.go: container lifecycle operations - runargs.go: docker run argument construction - useruid.go: container UID/GID remapping Idiomatic cleanups (no new abstractions): - Flatten runArgsBuilder's half-fluent chain into a uniform step list. - Collapse the per-IDE JetBrains switch into a constructor lookup table. - Rename the config2/provider2 import aliases to pkgconfig/provider. Promote CommandDevContainer's 7 positional args to a driver.CommandParams struct on the Driver interface, consistent with RunDockerDevContainerParams. Updates the docker, custom, and kubernetes drivers, the delivery adapter, and all call sites. Tests split to mirror the source (runargs_test.go, useruid_test.go).
FindContainerJSON is the fallback container lookup used when 'docker ps --filter' fails. Two defects: - The label loop reassigned the match result each iteration instead of requiring every label to match, so a multi-label query honored only the last label and could select the wrong container. - containers[0] was accessed whenever inspect returned no error, but InspectContainers can return an empty slice, causing a panic. Require all labels to match and skip entries with no inspect result. Adds a regression test covering both cases.
✅ Deploy Preview for images-devsy-sh canceled.
|
✅ Deploy Preview for devsydev canceled.
|
📝 WalkthroughWalkthroughThe PR introduces a ChangesDriver Interface Refactor and Docker Driver Split
FindContainerJSON Label Matching Bug Fix
Sequence Diagram(s)sequenceDiagram
rect rgba(135, 206, 235, 0.5)
note over Caller,UpdateContainerUserUID: RunDockerDevContainer flow
participant Caller
participant RunDockerDevContainer as dockerDriver.RunDockerDevContainer
participant EnsureImage as dockerDriver.EnsureImage
participant buildRunArgs as runArgsBuilder.buildRunArgs
participant startContainer as startContainer
participant UpdateContainerUserUID
Caller->>RunDockerDevContainer: RunDockerDevContainer(ctx, params)
RunDockerDevContainer->>EnsureImage: EnsureImage(ctx, options)
EnsureImage-->>RunDockerDevContainer: image ready (pulled if missing)
RunDockerDevContainer->>buildRunArgs: buildRunArgs(workspaceId, options, parsedConfig)
buildRunArgs-->>RunDockerDevContainer: []string docker run args
RunDockerDevContainer->>startContainer: startContainer(ctx, args, workspaceFolder)
startContainer-->>RunDockerDevContainer: container started
RunDockerDevContainer->>UpdateContainerUserUID: UpdateContainerUserUID(ctx, workspaceId, config)
UpdateContainerUserUID-->>RunDockerDevContainer: UID/GID updated
RunDockerDevContainer-->>Caller: nil or error
end
sequenceDiagram
rect rgba(255, 200, 100, 0.5)
note over Caller,dockerDriver: CommandDevContainer (updated signature)
participant Caller
participant Driver as Driver interface
participant dockerDriver as dockerDriver.CommandDevContainer
participant ensureContainerRunning
participant DockerExec as docker exec
Caller->>Driver: CommandDevContainer(ctx, *CommandParams)
Driver->>dockerDriver: dispatch with params
dockerDriver->>ensureContainerRunning: ensureContainerRunning(ctx, container)
ensureContainerRunning-->>dockerDriver: container running
dockerDriver->>DockerExec: exec params.Command as params.User
DockerExec-->>dockerDriver: stdout/stderr to params.Stdout/Stderr
dockerDriver-->>Caller: error or nil
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
skevetter
marked this pull request as ready for review
June 24, 2026 00:48
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/docker/helper_test.go (1)
122-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit inspect-error assertion path in this regression test.
This test validates the empty-inspect branch, but the new guard also handles
InspectContainerserrors. Adding one container whoseinspectexits non-zero would pin both fixed branches in one place.Suggested test extension
func TestFindContainerJSON_MatchesAllLabels(t *testing.T) { @@ - // last label (an earlier label differs); c3 inspect returns an empty array. + // last label (an earlier label differs); c3 inspect returns an empty array; + // c4 inspect returns an error. bin := writeScript(t, tmp, "docker-fake", `#!/bin/sh case "$1" in - ps) printf 'c1\nc2\nc3\n' ;; + ps) printf 'c1\nc2\nc3\nc4\n' ;; inspect) case "$4" in c1) echo '[{"ID":"c1","Config":{"Labels":{"a":"x","b":"y"}}}]' ;; c2) echo '[{"ID":"c2","Config":{"Labels":{"a":"zzz","b":"y"}}}]' ;; c3) echo '[]' ;; + c4) exit 1 ;; esac ;; esac `)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/docker/helper_test.go` around lines 122 - 147, The regression test TestFindContainerJSON_MatchesAllLabels currently validates the empty-inspect branch (c3 returning an empty array) but does not test the error path when InspectContainers fails. Add a fourth container (e.g., c4) to the docker-fake script's inspect case statement that exits with a non-zero status to simulate an inspect error, and update the final assertion to confirm that the function properly handles both the empty-inspect result and the inspect-error cases while still correctly filtering and returning only the c1 container that matches all labels.pkg/driver/docker/runargs.go (1)
477-496: 🎯 Functional Correctness | 🔵 Trivial
EnsurePathmutates the caller'sMountin place and should operate on a copy.It rewrites
path.Sourceon the shared pointer (options.WorkspaceMount) and returns the same pointer. While the current call path creates freshRunOptionson each invocation, the mutation pattern is fragile and violates the principle of immutability for function inputs. Consider operating on a copy to make the function idempotent and easier to reason about.♻️ Avoid in-place mutation
func (d *dockerDriver) EnsurePath(path *config.Mount) *config.Mount { // Local Windows to remote Linux over TCP requires manual path conversion. if runtime.GOOS == "windows" { for _, v := range d.Docker.Environment { if strings.Contains(v, "DOCKER_HOST=tcp://") { unixPath := path.Source unixPath = strings.Replace(unixPath, "C:", "c", 1) unixPath = strings.ReplaceAll(unixPath, "\\", "/") unixPath = "/mnt/" + unixPath - - path.Source = unixPath - - return path + clone := *path + clone.Source = unixPath + return &clone } } } return path }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/driver/docker/runargs.go` around lines 477 - 496, The EnsurePath method is mutating the input path parameter directly by modifying path.Source in place. To fix this, create a copy of the input *config.Mount parameter at the start of the function and perform all modifications on the copy instead of the original. Update all assignments to path.Source to assign to the copy's Source field instead, and ensure both return statements return the copy rather than the original path pointer. This will make the function idempotent and prevent unintended mutations of the caller's data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/driver/docker/lifecycle.go`:
- Around line 250-255: The log.Infof calls that log docker command arguments
expose sensitive information such as environment variables and secrets in the
logs. Create a helper function to redact sensitive arguments (typically those
following -e or --env flags) from the args slice before logging, and use this
redacted version in both the log.Infof call around the first docker command
logging and the second location where docker arguments are logged. This prevents
secrets and PII from being leaked into the logs while still providing useful
debugging information about the command being executed.
---
Nitpick comments:
In `@pkg/docker/helper_test.go`:
- Around line 122-147: The regression test
TestFindContainerJSON_MatchesAllLabels currently validates the empty-inspect
branch (c3 returning an empty array) but does not test the error path when
InspectContainers fails. Add a fourth container (e.g., c4) to the docker-fake
script's inspect case statement that exits with a non-zero status to simulate an
inspect error, and update the final assertion to confirm that the function
properly handles both the empty-inspect result and the inspect-error cases while
still correctly filtering and returning only the c1 container that matches all
labels.
In `@pkg/driver/docker/runargs.go`:
- Around line 477-496: The EnsurePath method is mutating the input path
parameter directly by modifying path.Source in place. To fix this, create a copy
of the input *config.Mount parameter at the start of the function and perform
all modifications on the copy instead of the original. Update all assignments to
path.Source to assign to the copy's Source field instead, and ensure both return
statements return the copy rather than the original path pointer. This will make
the function idempotent and prevent unintended mutations of the caller's data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd59bc22-b410-4619-b8a4-54937f9a1688
📒 Files selected for processing (16)
pkg/agent/delivery/factory.gopkg/devcontainer/delete_test.gopkg/devcontainer/run.gopkg/devcontainer/setup.gopkg/docker/helper.gopkg/docker/helper_test.gopkg/driver/custom/custom.gopkg/driver/docker/docker.gopkg/driver/docker/docker_test.gopkg/driver/docker/lifecycle.gopkg/driver/docker/runargs.gopkg/driver/docker/runargs_test.gopkg/driver/docker/useruid.gopkg/driver/docker/useruid_test.gopkg/driver/kubernetes/driver.gopkg/driver/types.go
Comment on lines
+250
to
+255
| log.Infof( | ||
| "running docker command: command=%s, args=%s, cwd=%s", | ||
| d.Docker.DockerCommand, | ||
| strings.Join(args, " "), | ||
| dir, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact docker run arguments from startup/error logs.
Line 250 and Line 263 log the full args list. These arguments can include sensitive env values, so this leaks secrets/PII to logs.
🔧 Proposed fix
log.Infof(
- "running docker command: command=%s, args=%s, cwd=%s",
+ "running docker command: command=%s, cwd=%s",
d.Docker.DockerCommand,
- strings.Join(args, " "),
dir,
)
@@
log.Errorf(
- "docker container failed to start: error=%v, command=%s, args=%s, cwd=%s",
+ "docker container failed to start: error=%v, command=%s, cwd=%s",
err,
d.Docker.DockerCommand,
- strings.Join(args, " "),
dir,
)Also applies to: 262-267
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/driver/docker/lifecycle.go` around lines 250 - 255, The log.Infof calls
that log docker command arguments expose sensitive information such as
environment variables and secrets in the logs. Create a helper function to
redact sensitive arguments (typically those following -e or --env flags) from
the args slice before logging, and use this redacted version in both the
log.Infof call around the first docker command logging and the second location
where docker arguments are logged. This prevents secrets and PII from being
leaked into the logs while still providing useful debugging information about
the command being executed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactors the Docker driver for cohesion and fixes a latent bug found while reviewing the surrounding helper layer. Two logical commits.
1. Refactor —
refactor(docker): split driver into cohesive files and tighten signaturesSplits the 1322-line
pkg/driver/docker/docker.gointo responsibility-grouped files, following the package's existingbuild.go/diagnostics.goconvention:docker.goNewDockerDriverfactory, core helperslifecycle.gorunargs.godocker runargument constructionuseruid.goIdiomatic cleanups (no new abstractions — the consumer-side
driver.DockerDriver/Driverinterfaces already exist):runArgsBuilder's half-fluent/half-error chain into a uniform step list.switchinto a constructor lookup table.config2/provider2import aliases topkgconfig/provider.Promotes
CommandDevContainer's 7 positional args to adriver.CommandParamsstruct on theDriverinterface, consistent with the existingRunDockerDevContainerParams/BuildRequestpattern. Updates the docker, custom, and kubernetes drivers, the delivery adapter, and all call sites. Tests split to mirror the source.2. Bug fix —
fix(docker): match all labels in FindContainerJSON fallbackFindContainerJSONis the fallback container lookup used whendocker ps --filterfails. Two defects fixed:containers[0]was accessed whenever inspect returned no error, butInspectContainerscan return an empty slice, causing a panic.Adds a regression test covering both cases (verified red against the old code, green against the fix).
Verification
go build ./...,go vet, and the affected test suites all pass.golangci-lint run --new-from-rev=main ./...reports 0 new issues (matches CI'sonly-new-issuesbehavior).Summary by CodeRabbit
New Features
Bug Fixes