Fix issue with remote docker workspaces.

[hamiltop]

When using a remote docker environment, we need to connect to the docker host rather than localhost.

Summary

I'm not sure why this was hardcoded to localhost when the original implementation used the app server. I'm also not sure if we need the utility function to replace localhost with the app server url. For reference, here's the original implementation:

https://github.com/OpenHands/OpenHands/blob/6c5ef256fd24f1108f919613f52d5f2426f8df5c/openhands/app_server/sandbox/docker_sandbox_service.py#L197C24-L203

Checklist

• If the PR is changing/adding functionality, are there tests to reflect this?
• If there is an example, have you run the example to make sure that it works?
• If there are instructions on how to run the code, have you followed the instructions and made sure that it works?
• If the feature is significant enough to require documentation, is there a PR open on the OpenHands/docs repository with the same branch name?
• Is the github CI passing?

[all-hands-bot]

The PR successfully addresses remote docker support. Found a few issues: an important inconsistency with localhost vs 127.0.0.1, a misleading comment, and missing test coverage for the new behavior. Details in inline comments.

[all-hands-bot]

[Automatic Post]: It has been a while since there was any activity on this PR. @hamiltop, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up.

[all-hands-bot]

[Automatic Post]: It has been a while since there was any activity on this PR. @hamiltop, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up.

[all-hands-bot]

[Automatic Post]: It has been a while since there was any activity on this PR. @hamiltop, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up.

[hamiltop]

I would like to get this merged in. It's a pretty regression from the pre v1 openhands.

[enyst]

Thank you, and sorry for the delay on this!

[all-hands-bot]

🟡 Code Review: Acceptable with Improvements Needed

Taste Rating: The core logic seems sound, but unclear design and missing tests make this risky for production.

---

Critical Analysis

🔴 CRITICAL: No Tests for Critical Feature

Your PR title says "Fix issue with remote docker workspaces" but there is zero test coverage for this scenario. The existing tests in test_docker_workspace.py are heavily mocked and dont verify:

1. That self.host is correctly preserved when pre-set
2. That _wait_for_health uses the correct URL in remote scenarios
3. That the localhost fallback works when self.host is empty

Required tests:

def test_docker_workspace_respects_preset_host():
    """When host is pre-set, _start_container should not override it."""
    
def test_docker_workspace_uses_localhost_when_host_empty():
    """When host is empty, _start_container defaults to localhost."""
    
def test_wait_for_health_uses_correct_url():
    """_wait_for_health should construct URL from self.host correctly."""

Without these, how do we know this actually fixes the issue? How do we prevent regression?

---

🟠 IMPORTANT: Misleading Comment and Potential Bug

Lines 307-310 - The comment says "or fallback to localhost" but theres no fallback logic:

# We can construct the health URL based on self.host if available,
# or fallback to localhost    # <-- This is a lie
base_url = self.host.rstrip("/")  # <-- No fallback here
health_url = f"{base_url}/health"

The bug: If self.host is still an empty string (its default value), then self.host.rstrip("/") returns "", making health_url = "/health", which will cause urlopen() to fail with an invalid URL.

Two options:

Option 1 - Actually implement the fallback:

base_url = self.host.rstrip("/") if self.host else f"http://127.0.0.1:{self.host_port}"
health_url = f"{base_url}/health"

Option 2 - Fail fast (I prefer this):

if not self.host:
    raise RuntimeError("_wait_for_health called before host was initialized")
base_url = self.host.rstrip("/")
health_url = f"{base_url}/health"

Fail-fast design catches initialization bugs immediately instead of masking them with fallbacks.

---

🟡 SUGGESTION: Document the Design Pattern

Lines 266-267 - The conditional check if not self.host: reveals an undocumented design pattern:

if not self.host:
    object.__setattr__(self, "host", f"http://127.0.0.1:{self.host_port}")

Questions:

• When would self.host already be set before _start_container runs?
• Is the use case that users pass host="http://remote-docker:8000" to the constructor?
• Why 127.0.0.1 instead of localhost? (IPv6 resolution issues?)

Your PR description says "Im not sure why this was hardcoded to localhost when the original implementation used the app server" - this uncertainty is a red flag. If youre not sure why, neither will future maintainers.

Recommendation: Add clear documentation:

# For remote Docker environments (e.g., Docker-in-Docker), self.host 
# may be pre-set via constructor to point to the docker host URL.
# Only default to localhost if not explicitly specified.
if not self.host:
    # Use 127.0.0.1 instead of localhost to avoid IPv6 resolution issues
    object.__setattr__(self, "host", f"http://127.0.0.1:{self.host_port}")

And update the class docstring:

class DockerWorkspace(RemoteWorkspace):
    """...
    
    For remote Docker environments (e.g., Docker-in-Docker scenarios),
    set the `host` parameter to point to the Docker host:
    
        workspace = DockerWorkspace(
            server_image="...",
            host="http://docker-host:8000",  # Pre-set for remote access
            host_port=8000
        )
    """

---

Verdict

✅ Worth merging - The core logic appears sound for the intended use case

BUT: This needs tests and clearer documentation before its production-ready. The fact that you referenced the original implementation but said "Im not sure" suggests the design intent wasnt clear, which means it wont be clear to future maintainers either.

---

Key Insight

The real issue is unclear state management. Why does self.host sometimes come pre-set and sometimes not? This conditional initialization pattern suggests the class handles two scenarios (local vs remote docker), but this duality isnt documented or tested.

Consider whether these should be:

1. Separate classes (LocalDockerWorkspace vs RemoteDockerWorkspace)
2. Explicit initialization modes (constructor parameter like remote_docker=True)
3. Better documented with examples showing both use cases

Right now, its implicit and fragile.

[enyst]

@hamiltop I think the reviewer agent is a bit confused, but I sympathize that the code is a bit unclear, could we maybe document enough to help it understand the intention behind?

[neubig]

[automated message] @xingyaoww assigned for review according to git blame