feat(agent): implement AgentDelivery interface#265
Merged
Conversation
…elivery Introduces a pluggable delivery strategy that replaces the monolithic inject.sh handshake with phase-aware delivery methods, eliminating race conditions identified in DEVSY-082. Three implementations: - LocalDockerDelivery: named volume pre-placement before container start - RemoteDockerDelivery: docker cp post-start for remote daemons - LegacyShellDelivery: wraps existing inject.sh path for custom drivers Includes factory, integration into setup.go/single.go, and E2E tests.
✅ Deploy Preview for devsydev canceled.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR introduces a pluggable agent delivery system for devsy that abstracts the mechanism of injecting agent binaries into containers. It provides three implementations—legacy shell injection for non-Docker drivers, local Docker volume-based pre-start delivery, and remote Docker post-start binary copying—selected via a factory based on driver type and Docker configuration. The system is integrated into the existing container setup lifecycle with error-handling fallbacks. ChangesAgent Delivery System Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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 |
The existing up-* tests already exercise the delivery path through setupContainer → injectAgentIntoContainer. No need for separate tests.
Move Docker-requiring tests to integration build tag only. Unit tests now run without Docker available.
Windows Podman uses npipe:// DOCKER_HOST which was incorrectly classified as remote. Also adds fallback to legacy inject when post-start delivery fails, ensuring existing paths keep working.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
pkg/agent/delivery/delivery_integration_test.go (1)
28-29: ⚡ Quick winUse unique resource names and
t.Cleanupfor deterministic integration tests.Static names (
delivery-test-integration,delivery-test-remote) and non-deferred cleanup can cause collisions/leaks on failed runs.Proposed refactor
import ( "context" "os/exec" + "fmt" + "time" "testing" @@ - workspaceID := "delivery-test-integration" + workspaceID := fmt.Sprintf("delivery-test-integration-%d", time.Now().UnixNano()) + t.Cleanup(func() { _ = d.Cleanup(ctx, workspaceID) }) @@ - // Cleanup - err = d.Cleanup(ctx, workspaceID) - require.NoError(t, err) + err = d.Cleanup(ctx, workspaceID) + require.NoError(t, err) @@ - containerID := "delivery-test-remote" + containerID := fmt.Sprintf("delivery-test-remote-%d", time.Now().UnixNano()) @@ - defer func() { + t.Cleanup(func() { _ = exec.CommandContext(ctx, "docker", "rm", "-f", containerID).Run() - }() + })Also applies to: 62-69, 78-88
🤖 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/agent/delivery/delivery_integration_test.go` around lines 28 - 29, The test uses static resource names (e.g., workspaceID := "delivery-test-integration" and similar remoteID values) and performs non-deferred cleanup, which can cause collisions and leaks; change to generate unique names using t.Name() or a timestamp (e.g., fmt.Sprintf("%s-%s", t.Name(), uuid/ts)) for workspaceID and remoteIDs, and register teardown with t.Cleanup to delete created resources (call the existing workspace/remote deletion helpers inside t.Cleanup) so cleanup always runs even on failure; update any helper functions referenced in the test (workspaceID, remoteID, and the delete/deleteRemote helpers) accordingly.pkg/devcontainer/single.go (1)
270-276: ⚡ Quick winConsider logging errors from
buildRunOptionsForDelivery.If
buildRunOptionsForDeliveryfails at line 270, the error is silently ignored. While pre-start delivery is best-effort, logging the failure reason would aid debugging and help identify configuration issues.♻️ Proposed improvement
runOptions, err := r.buildRunOptionsForDelivery(mergedConfig, p.substitutionContext, buildInfo) - if err == nil { + if err != nil { + log.Debugf("skipping pre-start delivery, could not build run options: %v", err) + } else { if preStartErr := r.deliverPreStart(ctx, runOptions); preStartErr != nil { log.Debugf("pre-start delivery skipped or failed, will use post-start: %v", preStartErr) } }🤖 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/devcontainer/single.go` around lines 270 - 276, The call to r.buildRunOptionsForDelivery can fail but its error is ignored; update the logic around buildRunOptionsForDelivery so that when it returns a non-nil err you log the error (e.g., using log.Debugf or log.Errorf) with context (mentioning mergedConfig or buildInfo) before skipping pre-start delivery, while preserving the existing behavior of calling deliverPreStart when err == nil; reference r.buildRunOptionsForDelivery, runOptions, and deliverPreStart to locate the change.pkg/agent/delivery/factory.go (2)
72-80: ⚡ Quick winPrefer
strings.HasPrefixover custom implementation.The
hasPrefixclosure reimplementsstrings.HasPrefixfrom the standard library. Using the standard library function improves readability and reduces cognitive load.♻️ Proposed refactor
+import "strings" + func isLocalDockerHost(host string) bool { if host == "" { return true } - hasPrefix := func(s, prefix string) bool { - return len(s) >= len(prefix) && s[:len(prefix)] == prefix - } - return hasPrefix(host, "unix://") || hasPrefix(host, "npipe://") + return strings.HasPrefix(host, "unix://") || strings.HasPrefix(host, "npipe://") }🤖 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/agent/delivery/factory.go` around lines 72 - 80, Replace the custom hasPrefix closure in isLocalDockerHost with the standard library function strings.HasPrefix: import "strings" if not already imported, remove the hasPrefix closure, and return strings.HasPrefix(host, "unix://") || strings.HasPrefix(host, "npipe://") while preserving the empty-host check and overall behavior in isLocalDockerHost.
44-44: ⚡ Quick winRemove unused parameter in
isDockerLocal.The
DockerCommandargument passed toisDockerLocalis ignored (line 67 uses_ string). Since Docker daemon locality is determined solely by theDOCKER_HOSTenvironment variable, the parameter serves no purpose.♻️ Proposed fix
- if isDockerLocal(opts.DockerCommand) { + if isDockerLocal() {And update the function signature:
-func isDockerLocal(_ string) bool { +func isDockerLocal() bool {🤖 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/agent/delivery/factory.go` at line 44, The isDockerLocal function currently accepts a DockerCommand parameter that is unused; update the function signature of isDockerLocal (remove the parameter instead of using `_ string`) and adjust all call sites such as the call in factory.go (currently isDockerLocal(opts.DockerCommand)) to call isDockerLocal() with no arguments; inside the isDockerLocal implementation remove the unused parameter reference and keep the DOCKER_HOST-only logic, and run a quick grep to update any other usages to the new zero-arg signature.pkg/agent/delivery/factory_test.go (1)
99-104: ⚡ Quick winRename test to match what it validates.
The test name
TestDeliver_PreStartis misleading. It validates thatDeliverreturns an error whenPostStartOptionsare missing for aPostStart-phase delivery (LegacyShellDelivery). Consider renaming toTestDeliver_MissingPostStartOptionsfor clarity.♻️ Proposed rename
-func TestDeliver_PreStart(t *testing.T) { +func TestDeliver_MissingPostStartOptions(t *testing.T) { d := &LegacyShellDelivery{} err := Deliver(context.Background(), d, nil, nil) require.Error(t, err) assert.Contains(t, err.Error(), "post-start options required") }🤖 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/agent/delivery/factory_test.go` around lines 99 - 104, Rename the test function TestDeliver_PreStart to a name that reflects its behavior, e.g., TestDeliver_MissingPostStartOptions: locate the test function TestDeliver_PreStart in pkg/agent/delivery/factory_test.go and change its identifier to TestDeliver_MissingPostStartOptions so it clearly indicates it validates Deliver returning an error for LegacyShellDelivery when PostStartOptions are missing (related symbols: Deliver, LegacyShellDelivery, PostStartOptions, PostStart).
🤖 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/agent/delivery/legacy_shell.go`:
- Around line 70-76: ExecFuncFromDriver currently returns a closure that will
panic if cmdFn is nil; add a nil guard so the returned inject.ExecFunc checks if
cmdFn == nil and returns a clear error (e.g., errors.New or fmt.Errorf with a
message like "driver command function is nil/misconfigured") instead of calling
it, thereby preventing a panic when the closure is invoked; update the
ExecFuncFromDriver function to perform this nil check before delegating to
cmdFn.
In `@pkg/agent/delivery/local_docker.go`:
- Around line 37-39: The rollback is calling d.removeVolume with an
already-prefixed volumeName causing double-prefixing and leaking volumes; update
the cleanup to pass the unprefixed/original volume identifier to removeVolume
(or ensure you strip the prefix before calling) in the error paths around
populateVolume and the similar block at the second occurrence (the code calling
d.populateVolume and the cleanup that follows); reference the populateVolume
call and the d.removeVolume(ctx, volumeName) invocation and ensure the argument
is the raw volume name the removeVolume implementation expects.
- Around line 44-53: The code mutates opts.RunOptions without guarding for nil;
add a nil-check and initialize opts.RunOptions before accessing Mounts or Env
(e.g. if opts.RunOptions == nil { opts.RunOptions = &config.RunOptions{} }) so
subsequent lines that append to opts.RunOptions.Mounts and set
opts.RunOptions.Env["DEVSY_AGENT_PATH"] cannot panic; ensure you allocate
RunOptions.Mounts and Env if they are nil (initialize Mounts slice and Env map)
before appending/assigning.
In `@pkg/agent/delivery/remote_docker.go`:
- Around line 27-33: The receiver field d.ContainerID should not be overwritten
with request-scoped IDs; instead, in DeliverPostStart (and the other blocks that
set d.ContainerID around the same area), read the container ID into a local
variable (e.g., containerID) by preferring the existing d.ContainerID and
falling back to opts.ContainerDetails.ID, validate it and use that local
variable for all subsequent operations, and remove any assignment that writes
opts.ContainerDetails.ID back into d.ContainerID so the delivery instance state
is not mutated across requests.
In `@pkg/devcontainer/setup.go`:
- Around line 95-102: When constructing delivery.FactoryOptions in Setup (the
call to delivery.NewAgentDelivery), set the IsRemoteDocker field by detecting
whether the Docker daemon is remote: read the DOCKER_HOST env var and treat it
as remote unless it points to a local Unix socket (e.g., starts with "unix://",
"unix:/", or empty) or a Windows named pipe (e.g., "npipe://"); populate
FactoryOptions.IsRemoteDocker accordingly before calling
delivery.NewAgentDelivery. You can either reuse the existing detection logic by
exporting isLocalDockerHost from factory.go or reimplement the same check inline
in setup.go; ensure the field name IsRemoteDocker on FactoryOptions is assigned
the boolean result.
---
Nitpick comments:
In `@pkg/agent/delivery/delivery_integration_test.go`:
- Around line 28-29: The test uses static resource names (e.g., workspaceID :=
"delivery-test-integration" and similar remoteID values) and performs
non-deferred cleanup, which can cause collisions and leaks; change to generate
unique names using t.Name() or a timestamp (e.g., fmt.Sprintf("%s-%s", t.Name(),
uuid/ts)) for workspaceID and remoteIDs, and register teardown with t.Cleanup to
delete created resources (call the existing workspace/remote deletion helpers
inside t.Cleanup) so cleanup always runs even on failure; update any helper
functions referenced in the test (workspaceID, remoteID, and the
delete/deleteRemote helpers) accordingly.
In `@pkg/agent/delivery/factory_test.go`:
- Around line 99-104: Rename the test function TestDeliver_PreStart to a name
that reflects its behavior, e.g., TestDeliver_MissingPostStartOptions: locate
the test function TestDeliver_PreStart in pkg/agent/delivery/factory_test.go and
change its identifier to TestDeliver_MissingPostStartOptions so it clearly
indicates it validates Deliver returning an error for LegacyShellDelivery when
PostStartOptions are missing (related symbols: Deliver, LegacyShellDelivery,
PostStartOptions, PostStart).
In `@pkg/agent/delivery/factory.go`:
- Around line 72-80: Replace the custom hasPrefix closure in isLocalDockerHost
with the standard library function strings.HasPrefix: import "strings" if not
already imported, remove the hasPrefix closure, and return
strings.HasPrefix(host, "unix://") || strings.HasPrefix(host, "npipe://") while
preserving the empty-host check and overall behavior in isLocalDockerHost.
- Line 44: The isDockerLocal function currently accepts a DockerCommand
parameter that is unused; update the function signature of isDockerLocal (remove
the parameter instead of using `_ string`) and adjust all call sites such as the
call in factory.go (currently isDockerLocal(opts.DockerCommand)) to call
isDockerLocal() with no arguments; inside the isDockerLocal implementation
remove the unused parameter reference and keep the DOCKER_HOST-only logic, and
run a quick grep to update any other usages to the new zero-arg signature.
In `@pkg/devcontainer/single.go`:
- Around line 270-276: The call to r.buildRunOptionsForDelivery can fail but its
error is ignored; update the logic around buildRunOptionsForDelivery so that
when it returns a non-nil err you log the error (e.g., using log.Debugf or
log.Errorf) with context (mentioning mergedConfig or buildInfo) before skipping
pre-start delivery, while preserving the existing behavior of calling
deliverPreStart when err == nil; reference r.buildRunOptionsForDelivery,
runOptions, and deliverPreStart to locate the change.
🪄 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: f7b45f17-46ef-49c8-9e55-331ddc81484a
📒 Files selected for processing (12)
pkg/agent/delivery/delivery.gopkg/agent/delivery/delivery_integration_test.gopkg/agent/delivery/factory.gopkg/agent/delivery/factory_test.gopkg/agent/delivery/legacy_shell.gopkg/agent/delivery/legacy_shell_test.gopkg/agent/delivery/local_docker.gopkg/agent/delivery/local_docker_test.gopkg/agent/delivery/remote_docker.gopkg/agent/delivery/remote_docker_test.gopkg/devcontainer/setup.gopkg/devcontainer/single.go
Comment on lines
+70
to
+76
| func ExecFuncFromDriver( | ||
| cmdFn func(ctx context.Context, user, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error, | ||
| user string, | ||
| ) inject.ExecFunc { | ||
| return func(ctx context.Context, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error { | ||
| return cmdFn(ctx, user, command, stdin, stdout, stderr) | ||
| } |
There was a problem hiding this comment.
Add a nil guard in ExecFuncFromDriver to avoid panic on misconfiguration.
If cmdFn is nil, the returned closure will panic when executed.
Proposed fix
func ExecFuncFromDriver(
cmdFn func(ctx context.Context, user, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error,
user string,
) inject.ExecFunc {
return func(ctx context.Context, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error {
+ if cmdFn == nil {
+ return fmt.Errorf("driver command function is required")
+ }
return cmdFn(ctx, user, command, stdin, stdout, stderr)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func ExecFuncFromDriver( | |
| cmdFn func(ctx context.Context, user, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error, | |
| user string, | |
| ) inject.ExecFunc { | |
| return func(ctx context.Context, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error { | |
| return cmdFn(ctx, user, command, stdin, stdout, stderr) | |
| } | |
| func ExecFuncFromDriver( | |
| cmdFn func(ctx context.Context, user, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error, | |
| user string, | |
| ) inject.ExecFunc { | |
| return func(ctx context.Context, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error { | |
| if cmdFn == nil { | |
| return fmt.Errorf("driver command function is required") | |
| } | |
| return cmdFn(ctx, user, command, stdin, stdout, stderr) | |
| } | |
| } |
🤖 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/agent/delivery/legacy_shell.go` around lines 70 - 76, ExecFuncFromDriver
currently returns a closure that will panic if cmdFn is nil; add a nil guard so
the returned inject.ExecFunc checks if cmdFn == nil and returns a clear error
(e.g., errors.New or fmt.Errorf with a message like "driver command function is
nil/misconfigured") instead of calling it, thereby preventing a panic when the
closure is invoked; update the ExecFuncFromDriver function to perform this nil
check before delegating to cmdFn.
Comment on lines
+44
to
+53
| opts.RunOptions.Mounts = append(opts.RunOptions.Mounts, &config.Mount{ | ||
| Type: "volume", | ||
| Source: volumeName, | ||
| Target: volumeMountPath, | ||
| }) | ||
|
|
||
| if opts.RunOptions.Env == nil { | ||
| opts.RunOptions.Env = make(map[string]string) | ||
| } | ||
| opts.RunOptions.Env["DEVSY_AGENT_PATH"] = volumeMountPath + "/" + binaryName() |
There was a problem hiding this comment.
Guard RunOptions before mutation to prevent nil dereference.
opts.RunOptions is dereferenced unconditionally; a nil pointer here will panic.
Proposed fix
func (d *LocalDockerDelivery) DeliverPreStart(ctx context.Context, opts PreStartOptions) error {
+ if opts.RunOptions == nil {
+ return fmt.Errorf("run options are required for local docker delivery")
+ }
+
volumeName := volumePrefix + opts.WorkspaceID🤖 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/agent/delivery/local_docker.go` around lines 44 - 53, The code mutates
opts.RunOptions without guarding for nil; add a nil-check and initialize
opts.RunOptions before accessing Mounts or Env (e.g. if opts.RunOptions == nil {
opts.RunOptions = &config.RunOptions{} }) so subsequent lines that append to
opts.RunOptions.Mounts and set opts.RunOptions.Env["DEVSY_AGENT_PATH"] cannot
panic; ensure you allocate RunOptions.Mounts and Env if they are nil (initialize
Mounts slice and Env map) before appending/assigning.
Comment on lines
+27
to
+33
| func (d *RemoteDockerDelivery) DeliverPostStart(ctx context.Context, opts PostStartOptions) error { | ||
| if d.ContainerID == "" && opts.ContainerDetails != nil { | ||
| d.ContainerID = opts.ContainerDetails.ID | ||
| } | ||
| if d.ContainerID == "" { | ||
| return fmt.Errorf("container ID is required for remote docker delivery") | ||
| } |
There was a problem hiding this comment.
Avoid persisting request-scoped container IDs on the receiver.
DeliverPostStart writes opts.ContainerDetails.ID into d.ContainerID. Reusing the same delivery instance can then copy/chmod into a stale container on later calls.
Proposed fix
func (d *RemoteDockerDelivery) DeliverPostStart(ctx context.Context, opts PostStartOptions) error {
- if d.ContainerID == "" && opts.ContainerDetails != nil {
- d.ContainerID = opts.ContainerDetails.ID
- }
- if d.ContainerID == "" {
+ containerID := d.ContainerID
+ if opts.ContainerDetails != nil && opts.ContainerDetails.ID != "" {
+ containerID = opts.ContainerDetails.ID
+ }
+ if containerID == "" {
return fmt.Errorf("container ID is required for remote docker delivery")
}
destPath := agent.ContainerDevsyHelperLocation
- if err := d.copyBinary(ctx, opts.BinaryPath, destPath); err != nil {
+ if err := d.copyBinary(ctx, containerID, opts.BinaryPath, destPath); err != nil {
return fmt.Errorf("copy binary to container: %w", err)
}
- if err := d.chmodBinary(ctx, destPath); err != nil {
+ if err := d.chmodBinary(ctx, containerID, destPath); err != nil {
return fmt.Errorf("chmod binary in container: %w", err)
}
- log.Debugf("delivered agent binary to remote container %s via docker cp", d.ContainerID)
+ log.Debugf("delivered agent binary to remote container %s via docker cp", containerID)
return nil
}
-func (d *RemoteDockerDelivery) copyBinary(ctx context.Context, srcPath, destPath string) error {
+func (d *RemoteDockerDelivery) copyBinary(ctx context.Context, containerID, srcPath, destPath string) error {
src := srcPath
- dest := fmt.Sprintf("%s:%s", d.ContainerID, destPath)
+ dest := fmt.Sprintf("%s:%s", containerID, destPath)
...
}
-func (d *RemoteDockerDelivery) chmodBinary(ctx context.Context, destPath string) error {
- out, err := d.cmd(ctx, "exec", d.ContainerID, "chmod", "755", destPath).CombinedOutput()
+func (d *RemoteDockerDelivery) chmodBinary(ctx context.Context, containerID, destPath string) error {
+ out, err := d.cmd(ctx, "exec", containerID, "chmod", "755", destPath).CombinedOutput()Also applies to: 37-43, 53-70
🤖 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/agent/delivery/remote_docker.go` around lines 27 - 33, The receiver field
d.ContainerID should not be overwritten with request-scoped IDs; instead, in
DeliverPostStart (and the other blocks that set d.ContainerID around the same
area), read the container ID into a local variable (e.g., containerID) by
preferring the existing d.ContainerID and falling back to
opts.ContainerDetails.ID, validate it and use that local variable for all
subsequent operations, and remove any assignment that writes
opts.ContainerDetails.ID back into d.ContainerID so the delivery instance state
is not mutated across requests.
Comment on lines
+95
to
+102
| return delivery.NewAgentDelivery(delivery.FactoryOptions{ | ||
| WorkspaceConfig: r.WorkspaceConfig, | ||
| WorkspaceID: r.ID, | ||
| DockerCommand: dockerCmd, | ||
| DockerEnv: dockerEnv, | ||
| ContainerID: r.ID, | ||
| ExecFunc: execFn, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for IsRemoteDocker usage and Docker host detection logic
# Find where IsRemoteDocker might be set or determined
rg -n "IsRemoteDocker" --type=go -C3
# Look for Docker host detection patterns
rg -n "DOCKER_HOST|isRemote|isLocal.*[Dd]ocker" --type=go -C2Repository: devsy-org/devsy
Length of output: 5038
🏁 Script executed:
# Check the setup.go file and its package declaration
head -20 pkg/devcontainer/setup.go
# Also get the full newAgentDelivery function
sed -n '83,103p' pkg/devcontainer/setup.goRepository: devsy-org/devsy
Length of output: 1128
🏁 Script executed:
# Check what's exported from factory.go and the package structure
head -30 pkg/agent/delivery/factory.go
# Get the isLocalDockerHost function
sed -n '66,85p' pkg/agent/delivery/factory.goRepository: devsy-org/devsy
Length of output: 1238
🏁 Script executed:
# Search for where IsRemoteDocker is being set in the codebase
rg -n "IsRemoteDocker.*=" --type=go -B2 -A2Repository: devsy-org/devsy
Length of output: 41
🏁 Script executed:
# Check if there's a public function in delivery package for detecting remote Docker
rg -n "^func Is|^func Is[A-Z]" pkg/agent/delivery/factory.goRepository: devsy-org/devsy
Length of output: 41
🏁 Script executed:
# Try broader search for IsRemoteDocker usage
rg "IsRemoteDocker" --type=go -B2 -A2Repository: devsy-org/devsy
Length of output: 856
🏁 Script executed:
# Check all public functions (capitalized) in factory.go
rg "^func [A-Z]" pkg/agent/delivery/factory.goRepository: devsy-org/devsy
Length of output: 150
🏁 Script executed:
# List all Go files in delivery package to see if there's another file handling this
ls -la pkg/agent/delivery/Repository: devsy-org/devsy
Length of output: 842
🏁 Script executed:
# Check the full switch statement in NewAgentDelivery to see all branches
sed -n '23,50p' pkg/agent/delivery/factory.goRepository: devsy-org/devsy
Length of output: 879
🏁 Script executed:
# Check if setup.go is used only for devcontainer or if it's a common pattern
# Find all callers of newAgentDelivery
rg "newAgentDelivery\|NewAgentDelivery" --type=go -B2 -A2 pkg/devcontainer/Repository: devsy-org/devsy
Length of output: 41
🏁 Script executed:
# Search for all occurrences of NewAgentDelivery in the entire codebase
rg "NewAgentDelivery" --type=go -B3 -A3Repository: devsy-org/devsy
Length of output: 4537
🏁 Script executed:
# Check if there are other implementations setting IsRemoteDocker
rg "FactoryOptions\|IsRemoteDocker" --type=go -lRepository: devsy-org/devsy
Length of output: 41
🏁 Script executed:
# Get the complete isLocalDockerHost function to understand the logic
sed -n '72,85p' pkg/agent/delivery/factory.goRepository: devsy-org/devsy
Length of output: 440
🏁 Script executed:
# Check if there are any other places that need to set IsRemoteDocker or if this is the only place
grep -r "NewAgentDelivery\|FactoryOptions" pkg/ --include="*.go" | grep -v test | grep -v "\.pb\.go"Repository: devsy-org/devsy
Length of output: 292
Set IsRemoteDocker field in FactoryOptions.
The IsRemoteDocker field is not set when constructing FactoryOptions at lines 95-102. The factory uses this field to select between RemoteDockerDelivery and LocalDockerDelivery (see factory.go line 35). Without it, the factory always defaults to local delivery, which could cause failures for remote Docker daemons.
To fix this, detect whether the Docker daemon is remote and set the field. The detection logic should check if the DOCKER_HOST environment variable is set to a remote address (anything other than local Unix or named pipe sockets).
Note: The helper function isLocalDockerHost in factory.go is currently private. Either export it as public or duplicate the detection logic in setup.go.
🤖 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/devcontainer/setup.go` around lines 95 - 102, When constructing
delivery.FactoryOptions in Setup (the call to delivery.NewAgentDelivery), set
the IsRemoteDocker field by detecting whether the Docker daemon is remote: read
the DOCKER_HOST env var and treat it as remote unless it points to a local Unix
socket (e.g., starts with "unix://", "unix:/", or empty) or a Windows named pipe
(e.g., "npipe://"); populate FactoryOptions.IsRemoteDocker accordingly before
calling delivery.NewAgentDelivery. You can either reuse the existing detection
logic by exporting isLocalDockerHost from factory.go or reimplement the same
check inline in setup.go; ensure the field name IsRemoteDocker on FactoryOptions
is assigned the boolean result.
…isition LocalDockerDelivery and RemoteDockerDelivery were receiving the container destination path (/usr/local/bin/devsy) as BinaryPath, then trying to os.Open it on the host where it doesn't exist. This caused every delivery attempt to fail silently and fall back to the legacy inject path. Replace BinaryPath with a BinarySourceFunc that uses agent.BinaryManager to acquire the binary via the existing source chain (local executable, file cache, HTTP download). This matches how the legacy inject path already acquires the binary.
This was referenced May 12, 2026
This was referenced May 16, 2026
This was referenced Jun 24, 2026
Merged
This was referenced Jul 6, 2026
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
pkg/agent/delivery/package with a pluggableAgentDeliveryinterface that replaces the monolithic inject.sh handshake protocol for delivering the devsy agent binary into containersLocalDockerDelivery(named volume pre-start),RemoteDockerDelivery(docker cp post-start), andLegacyShellDelivery(inject.sh fallback for custom drivers)setup.goandsingle.gowith a factory that auto-selects the appropriate strategy based on driver type and Docker daemon localitySummary by CodeRabbit
Release Notes
New Features
Tests