feat: make OpenVSCode root configurable with PATH fallback#3061
Conversation
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟢 Good taste - Clean solution that solves a real problem
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Configuration change with backward compatibility. Solves real problem (hardcoded Docker path breaks non-Docker hosts) with simple, well-tested fallback logic. Root precedence over PATH prevents security issues. No agent behavior changes, no eval impact.
VERDICT:
✅ Worth merging - Clean implementation, comprehensive tests, backward compatible
KEY INSIGHT:
Elegant two-tier discovery (configured root → PATH fallback) with explicit precedence prevents both rigidity and security risks.
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
All functional verification passed. The PR successfully achieves its goal: the agent-server can now find openvscode-server on non-Docker hosts via PATH fallback, and the root location is configurable via OH_VSCODE_SERVER_ROOT.
Does this PR achieve its stated goal?
Yes. The PR fixes the hardcoded /openhands/.openvscode-server path that broke the Code tab on non-Docker hosts. With this change:
- The root is now configurable via
OH_VSCODE_SERVER_ROOT(defaults to Docker layout) - When the binary is missing at the configured root, the service falls back to
shutil.which('openvscode-server')to find it on PATH - The configured root takes precedence over PATH (preventing stray system installs from overriding the configured location)
- The discovered binary path is tracked and used when spawning the process
All four behaviors were verified through functional testing on a non-Docker host.
| Phase | Result |
|---|---|
| Environment Setup | ✅ Built successfully with make build |
| CI Status | ✅ 18 checks passing, 8 build jobs in progress, 0 failing |
| Functional Verification | ✅ All scenarios tested and working |
Functional Verification
Test 1: PATH Fallback (configured root doesn't exist)
Step 1 — Establish baseline (simulate non-Docker host):
Checked if the hardcoded path exists:
/openhands/.openvscode-server/bin/openvscode-server: False
This confirms the bug exists on non-Docker hosts — the old code would fail here because it only checked the hardcoded Docker path.
Step 2 — Apply the PR's changes:
The PR branch is already checked out (commit 8cc507b7).
Step 3 — Test PATH fallback with the fix:
Created a mock openvscode-server binary on PATH and ran:
service = VSCodeService(
openvscode_server_root="/nonexistent/openvscode",
port=8001,
connection_token="test-token"
)
available = service._check_vscode_available()Output:
Configured root: /nonexistent/openvscode
Binary available: True
Resolve binary: /tmp/qa-vscode-test/bin/openvscode-server
✅ PASS: Service found openvscode-server on PATH as fallback
This shows the fix works: when the binary isn't at the configured root, the service falls back to PATH and succeeds.
Test 2: Root Precedence (configured root takes priority)
Step 1 — Setup:
Created two binaries:
- One at a configured root:
/tmp/qa-vscode-test/configured-root/bin/openvscode-server - One on PATH:
/tmp/qa-vscode-test/bin/openvscode-server
Step 2 — Test precedence:
Ran:
service = VSCodeService(
openvscode_server_root="/tmp/qa-vscode-test/configured-root",
port=8002,
connection_token="test-token-2"
)
available = service._check_vscode_available()Output:
Configured root: /tmp/qa-vscode-test/configured-root
Binary available: True
Resolved binary: /tmp/qa-vscode-test/configured-root/bin/openvscode-server
✅ PASS: Service used configured root, not PATH
This confirms the configured root takes precedence, preventing a stray system install from overriding the configured location.
Test 3: Configuration via Environment Variable
Step 1 — Test default value:
config = Config()
print(config.vscode_server_root)Output:
/openhands/.openvscode-server
✅ Default value is correct (preserves Docker layout)
Step 2 — Test environment variable override:
export OH_VSCODE_SERVER_ROOT="/custom/vscode/root"config = from_env(Config, "OH")
print(config.vscode_server_root)Output:
/custom/vscode/root
✅ Environment variable override works
This confirms the configuration option works as documented.
Test 4: Integration with get_vscode_service()
Step 1 — Configure environment:
export OH_ENABLE_VSCODE=true
export OH_VSCODE_SERVER_ROOT="/tmp/qa-vscode-test/configured-root"
export OH_VSCODE_PORT=8001Step 2 — Get service via factory function:
service = get_vscode_service()
available = service._check_vscode_available()Output:
Service created: True
Configured root: /tmp/qa-vscode-test/configured-root
Port: 8001
Binary available: True
Resolved binary: /tmp/qa-vscode-test/configured-root/bin/openvscode-server
✅ PASS: get_vscode_service() correctly wired custom configuration
This confirms the configuration flows through the entire stack correctly.
Test 5: Unit Tests
Ran all new unit tests added in this PR:
uv run pytest tests/agent_server/test_vscode_service.py::test_check_vscode_available_uses_shutil_which_fallback \
tests/agent_server/test_vscode_service.py::test_check_vscode_available_root_takes_precedence_over_path \
tests/agent_server/test_vscode_service.py::test_start_vscode_process_uses_resolved_binary \
tests/agent_server/test_vscode_service.py::test_get_vscode_service_passes_server_root_from_config \
tests/agent_server/test_vscode_service.py::test_vscode_server_root_configuration -vOutput:
4 passed, 5 warnings in 0.04s
✅ All new unit tests pass
Issues Found
None. The implementation works as documented and all tests pass.
VascoSch92
left a comment
There was a problem hiding this comment.
I tested locally that the fix works correctly, i.e., env-var override works and shutil.which fallback works.
Just a couple of nits, but otherwise LGTM.
| assert service.process is None | ||
|
|
||
|
|
||
| def test_check_vscode_available_false(vscode_service, tmp_path): |
There was a problem hiding this comment.
Note that this test will now fail on any host with openvscode-server on PATH.
consider to patch shutil.which to return None in the assertion
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_start_no_binary(vscode_service, tmp_path): |
There was a problem hiding this comment.
I think we have the same problem here.
| ) | ||
| cmd = ( | ||
| f"exec {self.openvscode_server_root}/bin/openvscode-server " | ||
| f"exec {binary_path} " |
There was a problem hiding this comment.
f"exec {binary_path} ..." is passed to asyncio.create_subprocess_shell. If a developer sets OH_VSCODE_SERVER_ROOT=/opt/My App/openvscode (or shutil.which returns a path with spaces, common on macOS under Application Support), the resulting command line breaks. The previous Docker-only path made this impossible; the new PATH-discovery path makes it plausible.
If this is a real concern: use shlex.quote(str(binary_path))
Why
The agent-server hardcoded
/openhands/.openvscode-server, which only exists inside the agent-server Docker image. On a non-Docker host the binary check fails,start()short-circuits,connection_tokenstaysNone, and/api/vscode/urlreturns{"url": null}. Downstream this breaks the Code tab in any GUI that runs the agent-server locally.Summary
Config.vscode_server_root(OH_VSCODE_SERVER_ROOT) so the OpenVSCode Server install location is no longer hardcoded.VSCodeService._check_vscode_available, fall back toshutil.which("openvscode-server")when the binary is missing at the configured root. Root takes precedence over PATH._resolved_binaryso_start_vscode_processexecs the actual found path instead of rebuilding the Docker-default path string.get_vscode_serviceto passvscode_server_rootfrom config.How to Test
Please run the agent-server-gui locally using the agent server changes in this branch so we can review and validate them.
Video/Screenshots
Type
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:c5e8908-pythonRun
All tags pushed for this build
About Multi-Architecture Support
c5e8908-python) is a multi-arch manifest supporting both amd64 and arm64c5e8908-python-amd64) are also available if needed