Conversation
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
⚠️ QA Report: PASS WITH ISSUES
Docker runtime mode works for the core create/proxy/WebSocket/delete flow, but I found two conversation API compatibility regressions in docker mode.
Does this PR achieve its stated goal?
Partially. I verified a real outer uvicorn agent-server in OH_CONVERSATION_RUNTIME=docker mode pulled the documented ghcr.io/openhands/agent-server:latest-python image, created a per-conversation Docker container, rewrote the workspace to /workspace, proxied GET /api/conversations/{id}, bridged /sockets/events/{id}, and removed the container on DELETE. However, two claimed preserved endpoints do not match local-mode behavior: GET /api/conversations?ids=<id> returns 500 in docker mode, and /api/conversations/count changes the response shape from a raw number to an object.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build succeeded; Docker daemon was available (28.0.4) and the documented runtime image pulled successfully. |
| CI Status | pre-commit was failing and several jobs were still pending; multiple tests/checks were green. I did not rerun CI tests. |
| Functional Verification |
Functional Verification
Test 1: Baseline local-mode API contract
Step 1 — Establish baseline (local mode):
Started the server with OH_CONVERSATION_RUNTIME=local and created a conversation using the normal HTTP API. Then queried the existing list/count endpoints:
curl "http://127.0.0.1:18081/api/conversations?ids=$LCID"
# HTTP 200, body: [{"id":"526d00e9-fefa-45a2-b355-dfdc9f53802f", ...}]
curl "http://127.0.0.1:18081/api/conversations/count"
# HTTP 200, body: 1This establishes the existing client-visible contract: ids lookup returns a JSON array, and count returns a raw JSON number.
Test 2: PR docker runtime core flow
Step 2 — Apply PR behavior:
Started the PR server with:
OH_CONVERSATION_RUNTIME=docker OH_CONVERSATION_CONTAINER_STARTUP_TIMEOUT=90 uv run uvicorn openhands.agent_server.api:create_app --factory --host 127.0.0.1 --port 18080Created a conversation through the outer server:
curl -H 'Content-Type: application/json' --data @/tmp/pr-start.json http://127.0.0.1:18080/api/conversations
# HTTP 201, id=1e74d784-b1c0-4fad-b142-27e7c1bc7343,
# workspace.working_dir=/workspace
docker ps --filter 'name=oh-conv-'
# ebbdda2c8f99 oh-conv-1e74d784b1c04fadb14227e7c1bc7343-79ca09c1 ... 0.0.0.0:30450->8000/tcpThis confirms the PR creates a real per-conversation container and rewrites the workspace path into the container.
Step 3 — Exercise proxied traffic:
curl "http://127.0.0.1:18080/api/conversations/$CID"
# HTTP 200, returned the created conversation with workspace.working_dir=/workspace
curl "http://127.0.0.1:18080/api/conversations/search"
# HTTP 200, returned items containing id=1e74d784-b1c0-4fad-b142-27e7c1bc7343
uv run python /tmp/qa_ws_check.py
# connected
# {"id":"5d0e05be-01b2-441e-9f76-975d9f00673c","timestamp":"2026-05-27T14...
curl -X DELETE "http://127.0.0.1:18080/api/conversations/$CID"
# HTTP 200, body: {"success":true}
docker ps --filter 'name=oh-conv-'
# no remaining QA containersThis confirms root HTTP proxying, search aggregation, WebSocket bridging, and DELETE cleanup work in a real Docker-backed run.
Test 3: Reproduced docker-mode compatibility regressions
Step 1 — Baseline: local mode returned HTTP 200 with a JSON array for GET /api/conversations?ids=<id> and raw 1 for /count.
Step 2 — PR docker mode: the same user-facing endpoints behaved differently:
curl "http://127.0.0.1:18080/api/conversations?ids=$CID"
# HTTP 500
# {"detail":"Internal Server Error","exception":"'list' object has no attribute 'get'"}
curl "http://127.0.0.1:18080/api/conversations/count"
# HTTP 200
# {"count":1}This shows docker mode does not fully preserve the existing conversation endpoint contract promised in the PR description.
Issues Found
- 🟠 Issue:
GET /api/conversations?ids=<conversation_id>returns 500 in docker mode instead of the local-mode JSON array response. - 🟠 Issue:
GET /api/conversations/countchanges response shape from raw JSON number (1) to object ({"count":1}).
This review was created by an AI agent (OpenHands) on behalf of the user.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable direction, but I found a few docker-mode issues that need attention before this is safe to merge: auth bypass, exposed inner servers, and REST/auth contract regressions.
This review was created by an AI agent (OpenHands) on behalf of the user.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🔴 HIGH — this is opt-in, but it changes request routing/authentication and starts network-reachable per-conversation servers.
VERDICT: ❌ Needs rework before merging.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/software-agent-sdk/actions/runs/26516796937
Six fixes for the per-conversation docker runtime, driven by reviewer findings on PR #3403: 1. Bind inner container ports to loopback only (-p 127.0.0.1:HOST:8000) so the per-conversation agent-servers can only be reached through the outer server's authenticated proxy. (R3311480573) 2. Authenticate the WebSocket bridge against the OUTER server's session keys before opening the upstream connection. Reuses the existing sockets.py helper (header / query / first-message auth), and the bridge no longer calls accept() a second time. (R3311480598) 3. Preserve the local GET /api/conversations?ids=... contract: route is batch-get-by-id, requires the ids query param, returns list[ConversationInfo | None]. Looks each id up in the registry and fetches from its container (None for missing). (R3311480555, R3311480542) 4. Preserve the local /api/conversations/count contract: returns a bare JSON integer (not {"count": N}), honors ?status= by forwarding the query to each inner container and summing their integers. (R3311480576, R3311480571) 5. ContainerManager.start() now returns (running, is_new). The POST route only tears down the container on inner 4xx / connection error when is_new=True, so a retried create against an existing conversation can no longer kill the live container. (R3311480570) 6. Workspace static-file routes mount under the workspace-cookie auth group in docker mode via a new docker_workspace_router. The workspace router is now registered before the header-only api_router so the more specific path wins; browser iframe/<img> embeds with the oh_workspace_session_key cookie continue to work. (R3311480585) Tests: * test_container_manager: assert loopback port binding; updated for the (running, is_new) return tuple, plus an explicit is_new=False assert on the idempotent second start. * test_docker_routers: new tests for batch-get-by-ids (incl. 422 on missing ids, null slots for unknown ids), bare-int /count contract, WS rejects wrong key, WS rejects missing first-message auth, WS accepts with valid outer key, POST retry preserves existing container on inner 4xx, fresh-create cleans up on inner 4xx, workspace route registered before the catch-all. Fake inner app reordered so /search and /count aren't shadowed by /{cid}. 22 docker_runtime tests pass; 144 tests in api / conversation / workspace / docker_runtime all green. Co-authored-by: openhands <openhands@all-hands.dev>
|
Pushed e7ec1a7 addressing all 8 review threads (now resolved). Summary: Critical (security)
Important (API contracts)
Tests This comment was posted by an AI agent (OpenHands) on behalf of the user. |
|
Pushed 1. Drop
|
| Concern | ContainerManager |
DockerWorkspace |
|---|---|---|
docker run wrapping |
bespoke argv builder | yes |
| Free port allocation | bespoke | yes |
| Image pulls & cleanup | bespoke | yes (cleanup_image) |
| Network / GPU / platform | partial | yes |
| Volume mounts | bespoke | yes |
| Forwarded env | bespoke | yes (forward_env + extra_env) |
| Log streaming | bespoke | yes (detach_logs) |
| Healthcheck wait | bespoke urlopen loop |
yes (health_check_timeout) |
| Lifecycle / cleanup | bespoke | yes (cleanup) |
I added one small field to DockerWorkspace to cover the one capability that wasn't already there:
bind_host: str— host interface to publish on. Default""keeps-p HOST_PORT:8000; setting"127.0.0.1"gives-p 127.0.0.1:HOST_PORT:8000. The docker registry pins this to127.0.0.1so only the outer agent-server can reach the inner — defense-in-depth around the proxy auth.
2. Drop fan-out across containers; read shared disk instead
Per the review pushback, fan-out was the wrong shape — it was N container hops for what's fundamentally a cheap directory walk. The outer's ConversationService now has a read_only_metadata mode that:
- Skips
EventServicestartup in__aenter__(no leases acquired, no in-memory state, no lease-renewal task). - On every
get/search/count/batch_get, re-walksconversations_pathand readsmeta.json+base_state.jsonstraight off disk. Falls back to a synthesized state for conversations whosebase_state.jsonhasn't been flushed yet. - Mutation methods aren't expected to be called (the docker proxy router intercepts them before they reach
ConversationService).
Bind-mount layout is per-cid: each sub-container only sees its own conversations/{cid_hex} subdirectory. The outer sees all of them. The .openhands settings/secrets dir is shared so OH_SECRET_KEY round-trips correctly.
Other changes asked for in the review
?cid=for global routers (bash/git/file/vscode/desktop/hooks/mcp/skills/tools/llm): registered one specific route per prefix indocker_global_proxy_routerso the catch-all doesn't shadow/api/conversations//api/settings/ etc. Missing?cid=→ clear 400 telling the client what they need to do.X-Session-API-Keyforwarding: outer and inner share the sameOH_SESSION_API_KEYS_0viaconversation_container_forward_env(now includes that key in the default list). The proxy passes through whatever header the client sent; for cookie-authed workspace static files it synthesizes the header fromworkspace.api_key(read out of the outer's env) so the inner static file server is happy.- No outer-side services touched for the simpler approach. The outer still runs
tmux/ vscode / desktop / sockets / settings / profiles in-process — those just aren't conversation-scoped.
Verification
The new architecture still answers every API the user asked about:
| Endpoint group | Where it runs in docker mode |
|---|---|
POST /api/conversations |
proxy → sub-container (spawns it first) |
| `GET /api/conversations[/count | /search |
Per-cid mutations (/run, /pause, /events, …) |
proxy → sub-container |
Workspace static files (/conversations/{cid}/workspace/…) |
proxy → sub-container, cookie-auth preserved |
Global routers (/bash, /git, /file, …) |
proxy → sub-container, requires ?cid= |
WS /sockets/events/{cid} |
outer authenticates, then bridges to sub-container |
Stats
13 files changed, 1083 insertions(+), 1215 deletions(-)
Net -132 LoC even though new tests were added. Locally:
tests/agent_server/test_conversation_service.py 80 passed (4 new read-only-mode tests)
tests/agent_server/test_conversation_router.py 69 passed (no changes)
tests/agent_server/docker_runtime/test_docker_routers 17 passed (rewritten for new registry)
ruff check, ruff format, and pyright all clean on the changed files.
This comment was created by an AI agent (OpenHands) on behalf of the PR author.
|
@OpenHands read this PR description and investigate all you need to understand it; use examples/ dir, agent-server examples, to test as you need, maybe make a new one for this PR in CI is failing for precommit, fix that too and push. Then tell us WDYT: can the PR be simplified? Does it handle docker lifecycle correctly for multi-docker? /codereview it. Post as a comment on github. Then take a deep breath. And propose fixes for the issues you uncovered and commit/push them. Post again what you did. |
|
I'm on it! enyst can track my progress at all-hands.dev |
|
This PR comment was created by an AI agent (OpenHands) on behalf of the user. Deep breath taken. Here is my current 🟡 Taste Rating: Acceptable, but not mergeable yet The second rewrite is pointed in the right direction: dropping fan-out for list/search/count and letting the outer server read shared persisted metadata is much simpler than making every metadata endpoint coordinate across containers. The route split is also mostly clean: mutations/proxy/websocket go to the inner server; settings/profiles/auth/etc. remain local. [CRITICAL ISSUES]
[IMPROVEMENT OPPORTUNITIES]
[RISK ASSESSMENT]
VERDICT: KEY INSIGHT: I’m going to fix the pre-commit failure and the lifecycle/concurrency issues above, then push and post a follow-up with the exact changes and validation. |
|
This PR comment was created by an AI agent (OpenHands) on behalf of the user. Pushed What changed:
Validation run locally: make build
uv run pre-commit run --files openhands-agent-server/openhands/agent_server/api.py openhands-agent-server/openhands/agent_server/docker_runtime/__init__.py openhands-agent-server/openhands/agent_server/docker_runtime/proxy.py openhands-agent-server/openhands/agent_server/docker_runtime/registry.py openhands-agent-server/openhands/agent_server/docker_runtime/routers.py openhands-agent-server/pyproject.toml openhands-workspace/openhands/workspace/docker/workspace.py tests/agent_server/docker_runtime/test_docker_routers.py tests/agent_server/docker_runtime/test_registry.py
uv run pytest tests/agent_server/docker_runtime -q
uv run pytest tests/agent_server/test_conversation_service.py -k 'read_only_metadata' -qResults:
CI checks had not been reported yet immediately after the push when I checked with |
|
OpenHands encountered an error: **Failed to send message to agent server: HTTP 503 error: no available server See the conversation for more information. |
|
[Automatic Post]: It has been a while since there was any activity on this PR. @rbren, 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. This comment was created by an AI agent (OpenHands) on behalf of the user. |
2 similar comments
|
[Automatic Post]: It has been a while since there was any activity on this PR. @rbren, 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. This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
[Automatic Post]: It has been a while since there was any activity on this PR. @rbren, 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. This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
This PR is stale because it has been open for 40 days with no activity. Remove the stale label or leave a comment, otherwise it will be closed in 10 days. |
b62b828 to
c419f1b
Compare
|
c419f1b to
806c43f
Compare
neubig
changed the title
8a547af to
b153900
Compare
b153900 to
b62b828
Compare
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR adds a Docker runtime mode (conversation_runtime: "docker") that provisions per-conversation containers with isolated credentials, proxied HTTP/WebSocket traffic, and a Unix-socket credential broker. The default remains "local", so existing behavior is unchanged.
Assessment
The security boundary is well-designed:
- Per-conversation encryption keys and API keys (never the outer server's
OH_SECRET_KEY) - HMAC-authenticated credential broker on a Unix socket with grant-scoped routes
- Hop-by-hop header stripping + auth query param stripping in the proxy layer
- Symlink protection in
RuntimeProvisioningStore._direct_child - Container hardening:
--cap-drop ALL,--security-opt no-new-privileges, loopback-only port binding _materializerecursively resolvesLookupSecrettoStaticSecretand rejects embedded control keys before provisioning- Rollback logic restores the original identity on container start failure or inner-server rejection
No blocking issues found. No version bumps, no new dependencies, no # type: ignore, no changes to agent behavior/prompts/tools.
Risk Assessment
LOW - The PR adds an opt-in runtime mode behind a config flag defaulting to "local". Existing users are unaffected. The Docker-specific code paths are only activated when conversation_runtime == "docker".
One observation (not blocking): the _materialize substring check for control keys at mediation.py:80 could theoretically reject a legitimate secret whose value happens to contain a session API key as a substring. This is an acceptable security trade-off given the comment explains the rationale, and the probability is negligible with secrets.token_urlsafe(32) keys.
VERDICT: Worth merging. Core infrastructure is sound, security model is well-constructed, and existing behavior is preserved.
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
Posted an APPROVE review to GitHub PR #3403 (review ID: 5192823367) against commit Review Summary:
This comment was posted by an AI agent (OpenHands). |
|
@all-hands-bot please review the current head. If there are no blocking findings, please explicitly approve this PR rather than leaving only a comment. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review: feat(agent-server): add docker runtime mode for per-conversation containers
Verdict: ✅ APPROVE
Reviewed the current head (ff7b8d7cb) — all 57 changed files (+5584/−531), including the full docker_runtime/ module, SDK health polling, workspace changes, and the test suite.
Summary
This PR adds an opt-in Docker runtime mode (conversation_runtime: "docker") where each conversation runs in a dedicated, isolated agent-server container. The outer server owns container lifecycle, proxies HTTP/WebSocket traffic, and brokers credentials through a Unix-socket boundary. The default remains "local", so existing behavior is unchanged.
Architecture Assessment
The design is well-structured with clean module separation:
| Module | Responsibility |
|---|---|
registry.py |
Container lifecycle — idempotent startup, concurrent starts for different cids, dedup for same cid, failed-start cleanup, stale container reaping on restart |
provisioning.py |
Per-conversation identity — independent encryption keys, API keys, broker tokens; symlink-resistant path validation; atomic manifest writes |
broker.py |
Unix-socket credential broker — HMAC bearer-token auth, grant-scoped routes, request size limits |
mediation.py |
Secret materialization — resolves LookupSecret references locally, strips MCP OAuth state, blocks outer control credentials via substring check |
proxy.py |
HTTP/WS forwarding — hop-by-hop header stripping (including authorization, cookie, set-cookie), auth query param stripping, inner API key synthesis |
routers.py |
Route injection — mutations → inner container, metadata reads → outer (disk), workspace cookie auth preserved, global routes require ?cid= |
runtime_route.py |
Scoped runtime API route with inner-image capability verification |
Security Boundary
The security model is carefully designed:
- Container hardening:
--cap-drop ALL,--security-opt no-new-privileges,--rm, memory/CPU/pids limits, loopback-only port binding (127.0.0.1::8000) - Credential isolation: Per-conversation encryption keys and session API keys — never the outer server's
OH_SECRET_KEYor session keys - Credential broker: 0o600 Unix socket with HMAC-authenticated bearer tokens and grant-based authorization per endpoint
- Proxy hygiene: Hop-by-hop headers stripped, auth query params filtered,
x-expose-secretsrejected on runtime routes - Secret materialization:
_materialize()validatesLookupSecretURLs against the internal server URL (SSRF protection), strips MCP OAuth state, and blocks outer control credentials from being provisioned - Symlink protection:
RuntimeProvisioningStore._direct_childvalidates that mount paths don't follow symlinks - Forced local runtime: Inner containers get
OH_CONVERSATION_RUNTIME=localto prevent recursive docker-runtime containers
Lifecycle Correctness
- Starts for different conversation IDs proceed concurrently; same-id starts deduplicate to one task
- Containers that start but fail
/healthare stopped immediately - Failed startup tasks are removed so retries can actually retry
stop()/shutdown()handle in-flight startup tasks and clean up containers that finish starting during teardown- Stale containers from a previous server instance are reaped via Docker labels on startup
Previous Review Findings — Resolved
The comment history shows multiple rounds of review. All previously identified critical issues have been addressed:
- Package boundary violation (
agent_server → workspace) — fixed by removing theDockerWorkspacedependency - Startup container leaks — fixed with cleanup in the exception path
- Serialized multi-container startup — fixed with per-cid concurrency
OH_SECRET_KEYforwarding — fixed with per-conversation encryption keys- Auth bypass / exposed inner servers — fixed with loopback-only binding and pre-accept WebSocket auth
Non-Blocking Maintenance Risk
The MCP OAuth refresh path in broker.py uses FastMCP private methods (_initialize(), _refresh_token(), _handle_refresh_response()). This is acknowledged with an inline comment and covered by test_broker_refresh.py. A future FastMCP upgrade could break this path, but it is not a blocking issue for this PR.
Test Coverage
2,710 lines of tests across 9 test files covering broker, mediation, provisioning, proxy, registry, routers, scoped routes, legacy routes, and broker refresh. Coverage report shows 83% overall, with docker_runtime modules ranging from 63–96%.
CI Status
Most checks are passing (REST API breakage, cross-tests, sdk-tests, workspace-tests, tools-tests, stress tests). Some checks (pre-commit, agent-server-tests, build jobs) were still pending at review time for the latest head.
Conclusion
No blocking findings. The PR is well-architected, the security boundaries are carefully designed, previous review issues have been resolved, and the opt-in nature means no risk to existing local-mode behavior. Approving.
Generated by OpenHands AI on behalf of the user.
|
@all-hands-bot please review the current head. If there are no blocking findings, please explicitly approve this PR rather than leaving only a comment. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review: approve
I reviewed the current head (9aa915a47) of this PR, which adds a Docker conversation runtime mode where each conversation runs in a dedicated, hardened agent-server container with the outer server handling routing, lifecycle, and credential brokering.
What I checked
Architecture & security design
- Per-conversation containers with strong isolation:
--cap-drop ALL,--security-opt no-new-privileges, memory/CPU/PID limits, loopback-only port publishing (127.0.0.1::8000), and--usermatching the host UID/GID. - Per-conversation
RuntimeIdentitywith distinct API key, encryption key, and broker token — all encrypted at rest via the server cipher. The manifest is never bind-mounted into the container. - Credential delivery via a per-conversation Unix socket broker (
broker.py) with bearer-token auth and route-level grant enforcement. Refresh tokens are stripped from subscription responses. Body-size and concurrency limits are applied. _materializeinmediation.pyresolvesLookupSecretreferences toStaticSecretonly for internal URLs, rejects external lookups, and blocks embedded control credentials using substring matching (not just exact equality). Profile-secret filtering is applied before materialization.- The proxy strips all hop-by-hop headers, re-mints the inner
X-Session-API-Keyfrom the workspace's provisioned key (never forwarding the caller's outer credential), and explicitly rejectsx-expose-secrets. - Symlink protection on every filesystem path resolution (
_direct_child). - Inner containers are forced to
OH_CONVERSATION_RUNTIME=localto prevent recursive container creation.
Lifecycle & concurrency
- Concurrent starts for different conversation IDs proceed in parallel; same-ID starts deduplicate to a single task.
- Failed startup tasks are removed so retries can actually retry. Containers that fail health checks are stopped immediately.
shutdown()is best-effort — one broken container doesn't block cleanup of the rest. In-flight starts are awaited and then stopped.cleanup_stale_containers()uses server-scoped Docker labels to reap only containers owned by this server instance.
Routing
- Conversation-scoped routes (
/api/conversations/{id}/{service}/...) with legacy?cid=compatibility routes deprecated v1.48→v1.53. - Mutation routes (switch_profile, switch_llm, secrets, security_analyzer) are mediated through the outer server with grant updates committed only on success and rolled back on failure.
- Fork/children/credential-bindings explicitly return 501. Unsupported mutations return 501.
- Capability check on first scoped-route use (
conversation_runtime_routes_v1).
Test results
- 134 of 135 new docker_runtime tests pass. The one failure (
test_failed_docker_run_surfaces_stderr) is because Docker is not installed in this review sandbox — the test mockssubprocess.runbut not theexecute_command(["docker", "version"])precheck, which requires real Docker. This is an environment limitation, not a code defect. - 282 existing agent_server tests pass with no regressions.
- 327 of 328 TypeScript tests pass. The one failure (
test-utils.test.ts) is caused by this sandbox's non-defaultOH_INTERNAL_SERVER_URL, unrelated to the PR. - Comprehensive test coverage: ~2,900 lines of new tests covering broker, mediation, proxy, registry, routers, provisioning, scoped routes, legacy routes, and broker refresh.
Previous review feedback
All items from the prior review rounds have been addressed: the package-boundary violation was fixed, container hardening was added, scoped secrets replaced the shared persistence mount, runtime ownership persistence and reconciliation were implemented, and MCP/global routing was corrected.
Non-blocking observations (not requesting changes)
broker.pyusesoauth._initialize()andoauth._refresh_token()which are private FastMCP APIs. This is acknowledged in a comment and covered by a regression test, but it's a coupling risk for future FastMCP upgrades.- The
test_failed_docker_run_surfaces_stderrtest would be more robust if it also mockedexecute_commandor thedocker versionavailability check, so it doesn't require real Docker. test_secret_mutation_filters_before_lookuphardcodeshttp://127.0.0.1:8000but the code readsOH_INTERNAL_SERVER_URLfrom the environment — minor test fragility.
No blocking findings. Approving.
Generated by OpenHands AI on behalf of the user.
Review: approveI reviewed the current head ( What I checkedArchitecture & security design
Lifecycle & concurrency
Routing
Test results
Previous review feedback Non-blocking observations (not requesting changes)
No blocking findings. Approving. Generated by OpenHands AI on behalf of the user. |
f5a9e7a to
ec8ac16
Compare
5bff578 to
7bf8d0d
Compare
ec8ac16 to
5d6e1cc
Compare
c7c019a to
c8bdabb
Compare
05c3bc2 to
91a259d
Compare
c8bdabb to
2d8894b
Compare
Co-authored-by: openhands <openhands@all-hands.dev>
Expose an explicit host workspace namespace and route SDK host operations through it so scanner automations remain host-side while their selected subject agents use conversation containers. Co-authored-by: openhands <openhands@all-hands.dev>
2d8894b to
92a8871
Compare
5 participants
HUMAN:
AGENT:
Why
Automations need isolated, bounded Docker execution while retaining the shared conversation API and profile-secret rules.
Summary
Issue Number
Closes #5016; incorporates the Docker work tracked by #5004/#5007. Contributor history is preserved.
How to Test
uv run pytest tests/agent_server/docker_runtime -qFocused historical suites cover proxy, API/router/session-socket, and readiness/workspace behavior. The current Docker runtime suite passes 110 runtime, mediation, proxy, registry, and broker tests. A broader stack run passed 231 Docker, service, and health tests with one skip.
Live Docker probes created profile-selected conversations, executed runtime commands, verified selected-secret delivery, and released the containers. The integrated factory completed neubig/box-clone#8 and independently delivered neubig/airbnb-clone#29, #30, and #35. The deployed SDK combines this PR with its shared-contract and client prerequisites.
Type
Notes
Native stack #5018 now has #5017 → #5046 → #5081 → #3403; #4966 and independent profile schema #4931 are merged into main. The parent refresh at
f5a9e7a04preserves the entire previously validated source tree. Docker is the final implementation layer. Existing query-scoped APIs retain explicit v1.48 → v1.53 deprecation. Independent reliability/client PRs remain separate.🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimnikolaik/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:ec8ac16-pythonRun
All tags pushed for this build
About Multi-Architecture Support
ec8ac16-python) is a multi-arch manifest supporting both amd64 and arm64ec8ac16-python-amd64) are also available if neededLive Agent Canvas evidence
Animated recording · Scenario, exact revisions and limits · Allowlisted observations. Direct integrated enhancement demonstration with a real agent and synthetic secrets; the report distinguishes the owning PR from companion SDK/Automation/Canvas changes.
Credential-scope review follow-up
The Docker runtime consumes the same persisted
LaunchedAgentProfile.secret_refsas local conversations. It filters resumed and explicitly updated user secrets before lookup/transport, gates outer Codex broker grants and inner attachment, and preserves separate OpenHands model/MCP authentication. Profile edits cannot widen a resumed runtime. Validation: 67 Docker mediation/router tests passed (one opt-in real-Docker test skipped), plus 38 broker/provisioning/service tests passed. Local launch/resume before/after proof for the prerequisite; this follow-up does not claim a new live Docker capture.Merge and release prerequisites
Native predecessors: software-agent-sdk#5017 → software-agent-sdk#5046 → software-agent-sdk#5081. The independent profile prerequisite software-agent-sdk#4931 is now merged into main. Docker remains the last layer after the runtime and secret contracts.
Runtime stack cleanup
Docker-only query-scoped compatibility registration now lives beside its callers, with the same v1.48→v1.53 deadline enforced by the existing repository checker. Its tests moved with it. TypeScript lost-create-response reconciliation was removed from this stack and is now the independent main-based #5036, so this diff does not introduce that client policy. The proxy comment now describes the actual per-runtime key, not the outer server credential.
Validation: 76 focused Docker/router/deprecation checks passed; one opt-in real-Docker test skipped. Repository hooks passed. Final head
66c34c4936d8f15d41b20a7b1eea6abdda2c72f9incorporates current main through the native parent; its production source is identical to the tested cleanup commit8ff58e4d4. The 231-test run used871e2d06e; the later parent merge only adjusts OpenAPI compatibility metadata and simplifies when the TypeScript workspace binds. Existing immutable live evidence remains at its recorded revisions. This reorganization has focused contract tests and no new live-server recording.Current head
9aa915a47contains the narrowed #4966 and #5017 parents. Unused scoped MCP-probe routing and its outer-side materialization helper were removed; configured MCP tools still run normally inside each conversation runtime. The existing real FastMCP OAuth refresh compatibility regressions passed again; no duplicate test or new dependency pin was added. The 231-test run used871e2d06e; the later parent merge only adjusts OpenAPI compatibility metadata and simplifies when the TypeScript workspace binds. Existing immutable live evidence remains at its recorded revisions.