fix(sandbox): try Docker socket before CLI binary check

[ymcrcat]

Problem

The sandbox detection in check_docker() called which docker first and returned NotInstalled if the CLI binary was absent — even when the Docker daemon was reachable via a bind-mounted socket. This broke container-in-container deployments (e.g., Nomad shards with /var/run/docker.sock mounted) where bollard can talk to the daemon but no CLI is installed in the slim image.

Solution

Reorder check_docker() to call connect_docker() (bollard socket ping) first. If the daemon responds, return Available immediately. The which docker CLI check is now used only as a fallback for error-message quality when the socket connection fails.

Detection order (new):

1. Try bollard socket ping → if OK, return Available
2. Check CLI binary on PATH → if absent, return NotInstalled; otherwise return NotRunning

Files Changed

• src/sandbox/detect.rs — reordered checks, updated doc comments

---

Generated with Warp

Co-Authored-By: Oz oz-agent@warp.dev

[gemini-code-assist]

Code Review

This pull request updates the Docker detection mechanism to attempt a direct socket connection before checking for the CLI binary, which improves compatibility with container-in-container setups. A review comment correctly identified that the implementation lacks an actual ping to verify the daemon's responsiveness, suggesting the addition of a .ping() call to match the intended behavior and documentation.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d79e2e3e0a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[henrypark133]

Review: Docker detection reorder for DinD support (Risk: medium)

Good fix — the three-tier detection (fast-path → bollard ping → binary fallback) is well-structured, and connect_docker() does actually ping the daemon (container.rs:651-652), so Gemini's prior concern is resolved. The fast-path correctly avoids the 120s bollard timeout on hosts with no Docker at all. Comments and documentation are clear.

Positives:

• Correctly solves the DinD use case (socket bind-mounted, no CLI binary)
• Fast-path avoids unnecessary bollard timeout on non-Docker hosts
• Well-documented rationale in both module and function doc comments

Suggestion: Extract decision logic for testability [Testing]

File: src/sandbox/detect.rs

The new branching logic in check_docker() would benefit from a small refactor to make it testable without Docker. Extract the decision into a pure function:

fn resolve_status(
    binary_found: bool,
    docker_host_set: bool,
    socket_exists: bool,
    daemon_reachable: bool,
) -> DockerStatus {
    if !binary_found && !docker_host_set && !socket_exists {
        return DockerStatus::NotInstalled;
    }
    if daemon_reachable {
        return DockerStatus::Available;
    }
    if !binary_found {
        return DockerStatus::NotInstalled;
    }
    DockerStatus::NotRunning
}

Then check_docker() gathers the booleans and delegates. This lets you test the decision table with zero I/O:

#[test]
fn test_resolve_status_dind() {
    // Socket exists, no binary, daemon up → Available (the DinD fix)
    assert_eq!(resolve_status(false, false, true, true), DockerStatus::Available);
}

#[test]
fn test_resolve_status_nothing_installed() {
    // No socket, no binary, no DOCKER_HOST → NotInstalled (fast path)
    assert_eq!(resolve_status(false, false, false, false), DockerStatus::NotInstalled);
}

#[test]
fn test_resolve_status_daemon_down() {
    // Binary exists, daemon unreachable → NotRunning
    assert_eq!(resolve_status(true, false, false, false), DockerStatus::NotRunning);
}

Non-blocking — the code is correct as-is, but this would give good regression coverage for the new logic with minimal effort.