feat(agent-server): conversation-scoped runtime APIs and clients - #4966
Conversation
Includes canonical local routes, injectable runtime dispatch, query-route deprecation registration (1.48 to 1.53), deadline enforcement and stack-base CI coverage. openhands+astra requested by @neubig; created by an AI agent (OpenHands) on behalf of @neubig. Co-authored-by: openhands <openhands@all-hands.dev>
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
1 similar comment
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this 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.
Review Summary
Solid foundation PR for conversation-scoped runtime APIs. The route-class injection pattern, per-conversation bash event service lifecycle, and TypeScript client capability detection are well-designed and well-tested.
Key findings: No material bugs, security issues, or design flaws found.
What's good
- Bash service lifecycle: Task tracking with
_tasksset,_closedflag,CancelledErrorhandler that sends SIGKILL to the process group, and proper cleanup inclose(). The testtest_conversation_close_stops_runtime_terminalverifies the process is actually killed. - Workspace validation:
require_local_runtimecorrectly validatespath/workspace_dirparams against the conversation workspace root, and trajectoryconversation_idagainstruntime_conversation_id. The docstring honestly frames these as "routing safeguards, not a sandbox." - Route registration:
create_runtime_routerwithroute_classinjection cleanly separates the API contract from the dispatch adapter, so Docker (#3403) can inject its own route class without touching route definitions. - TS client routing: The
isRuntimePath+/server_infocapability check + URL rewriting logic is correct. Thecidquery-param fallback for non-capable servers preserves backward compatibility. Error handling resetsruntimeRouteson non-404 failures so subsequent requests retry. - CI scripts: The
check_deprecations.pyAST walker for programmaticadd_api_route(deprecated=True)calls and thecheck_sdk_api_breakage.pyaccepted-members dict with reason strings are both well-tested. - Test coverage: 221 lines of runtime router tests covering workspace isolation, terminal history isolation, cwd validation, close cleanup, OpenAPI schema preservation, deprecation enforcement, and MCP probe scope. 71 lines of TS client tests covering scoped/unscoped/capable-server routing.
Minor observations (non-blocking)
- The MCP
runtime_verifiedflag is set toresult.okin the conversation-scoped path, but the probe itself (_probe_mcp_server) is identical to the host-level probe with no conversation context injected. This is accurate for local mode; Docker mode (#3403) may need a context-aware probe. - The
runtimeRoutespromise is cached perHttpClientinstance, so the capability check runs once. This is fine for a single session but won't detect server upgrades mid-session.
Risk Assessment
� LOW � This PR adds a new routing layer without changing existing endpoint behavior, agent logic, prompt templates, or tool execution. Legacy routes remain untouched. The bash service changes (task cancellation on close) are cleanup improvements that don't alter command execution behavior. No version bumps or dependency changes.
VERDICT: � Worth merging
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
The review was successfully posted to GitHub with state GITHUB_REVIEW_POSTED This comment was posted by an AI agent (OpenHands). |
|
🤖 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 conversation-scoped runtime API endpoints (/api/conversations/{id}/...) that mirror the existing global runtime endpoints, with per-conversation workspace validation and bash history isolation. It also introduces TypeScript client infrastructure (RuntimeClient, RuntimeTransport, ServerConnection) that discovers server capabilities and routes requests accordingly, with fallback to legacy ?cid= routing on older servers.
Analysis
Architecture: The approach of reusing existing router endpoints via create_runtime_router() is clean — it avoids duplicating endpoint logic while adding a require_local_runtime dependency for workspace-scoped validation. The route-class injection pattern (route_class parameter) allows the upper Docker layer (#3403) to supply its own dispatch adapter without coupling this foundation to Docker.
Security: The require_local_runtime dependency validates path and workspace_dir parameters are absolute and inside the conversation workspace via Path.resolve().is_relative_to(root). The bash router's _validate_cwd does the same for cwd. The TS RuntimeTransport.scope() adds client-side path traversal protection (rejecting ?, #, \\, ./.. segments). These are correctly characterized as routing safeguards, not a sandbox — appropriate for local runtime where the process boundary is the real isolation.
Bash lifecycle: The BashEventService now tracks in-flight tasks (_tasks set) and properly cancels them on close(), with CancelledError handling that kills the process group before re-raising. EventService.close() closes the per-conversation bash service first. The test test_conversation_close_stops_runtime_terminal verifies that child processes are actually killed. Good.
Backward compatibility: Legacy global routes remain registered alongside the new conversation-scoped routes. The add_legacy_runtime_routes helper defines deprecated Docker compatibility routes with proper deprecation metadata (v1.48.0 → v1.53.0). The deprecation checker (check_deprecations.py) is extended to inspect programmatic add_api_route calls with deprecated=True. Local routes are intentionally not deprecated.
TypeScript client: The ServerConnection.supportsRuntimeRoutes() method memoizes capability discovery with ??=, and on non-404 errors resets the promise to allow retry. The 404 path returns false for older servers. The runtimeServiceConnections() factory correctly resolves shared vs. scoped connections and throws on conflicting conversation IDs. Test coverage is comprehensive (210-line test file covering scoping, legacy fallback, discovery coalescing, retry, and immutability enforcement).
Eval-risk flag
The BashEventService changes (task tracking, CancelledError handling, _closed flag, cwd defaulting) affect the global bash service used by existing REST endpoints, not just the new runtime-scoped one. While the agent's tool execution path goes through the SDK directly (not these REST endpoints), the changes touch terminal/stdout handling. Per repo policy, flagging for a human maintainer to decide whether lightweight evals are needed before merge.
Minor observation
The VSCode status route under the runtime router is registered with only path, endpoint, and methods — omitting response_model, tags, summary, etc. that the main loop copies for other routes. This won't cause functional issues but produces a less detailed OpenAPI schema for /api/conversations/{id}/vscode/status. Not blocking.
Risk Assessment
🟢 Low risk — No critical bugs or security issues found. The PR is well-structured with good test coverage. The main consideration is the eval-risk flag above, which warrants human maintainer judgment.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
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.
Review Summary
This PR introduces conversation-scoped runtime APIs, per-conversation bash history, TypeScript client support with capability discovery, and runtime lifecycle models. The implementation is clean, additive, and backward-compatible.
Key findings
No material bugs or security issues found.
I reviewed the following areas in depth:
-
Bash service lazy creation (
dependencies.py:73-81): The asyncget_bash_event_servicecreates a per-conversationBashEventServiceon first access. Since Python asyncio is single-threaded and there's noawaitbetween theis Nonecheck and the assignment, concurrent requests share a single instance safely. The testtest_concurrent_runtime_requests_share_event_loop_bash_servicevalidates this. -
Path validation (
runtime_router.py:32-57):require_local_runtimevalidatespath/workspace_dirparams against the conversation workspace root usingPath.resolve().is_relative_to(). The docstring correctly notes these are routing safeguards, not a sandbox boundary. TOCTOU between validation and file operation is acceptable given this stated scope. -
Bash task lifecycle (
bash_service.py:261-271, 357-360, 521-528): The_closedflag,_taskstracking set, andCancelledErrorhandler that kills the process group with SIGKILL are correctly implemented.close()cancels all in-flight tasks and waits for them before closing the pubsub. -
Create response recovery (
conversation-client.ts:72-100): The recovery loop only triggers on non-HttpError(network-level failures), never replays the POST, requires a caller-suppliedconversation_id, and breaks on non-404 errors. Well-designed. -
Backward compatibility: All new fields (
runtime_status,can_resume,runtime_error,conversation_runtime,workspace_mode,capabilities) have defaults. Thefile_router/file_discovery_routersplit keeps global discovery endpoints (/api/file/home,/api/file/search_subdirs) available while scoping file operations under conversation routes. Legacy/api/file/*and/api/bash/*routes remain unchanged. -
TypeScript
RuntimeTransport(runtime-transport.ts:27-47): Path traversal protection rejects?,#,\\, and./..path segments. Thecidoverride guard prevents conversation identity injection. Capability discovery with caching and fallback to legacycidquery param is clean.
Eval risk flag
This PR touches bash/terminal execution service lifecycle (task tracking, cancellation, cwd validation, _closed flag). While these changes are to the agent-server HTTP API layer rather than the agent's terminal tool itself, terminal/stdin/stdout handling is in the eval-risk category per the review guidelines. Flagging for a human maintainer to decide whether lightweight evals are needed before merge.
[RISK ASSESSMENT]
- Overall PR: LOW
- The changes are additive and well-tested (228 new test lines in
test_runtime_router.py, TypeScript scope/recovery tests). The main risk is the eval-adjacent bash service changes, which are lifecycle/cleanup-focused rather than changing command execution semantics.
VERDICT: Worth merging. No blocking issues. Human maintainer should confirm eval necessity given the bash service changes.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
Reuse cached server info and existing workspace clients. Move download metadata to its owning routes and split create-response recovery into an independent fix. Co-authored-by: openhands <openhands@all-hands.dev>
Remove the three obsolete weak-schema exceptions while preserving published media types. Co-authored-by: openhands <openhands@all-hands.dev>
Keep the existing host bash service cancellation behavior unchanged while ensuring conversation-owned processes are stopped on release. Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Use the existing weak-schema ratchet for scoped aliases and bind conversation workspaces only after reading the server response. Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
@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. |
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
@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: head 01398e7e
Verdict: APPROVE � No blocking findings.
What changed since the last approval (ae05f6e0)
The head has been significantly refactored and simplified compared to the previously approved SHA. Key changes reviewed:
-
Runtime router renamed and simplified �
require_local_runtime�bind_local_conversation_runtime. It now resolves the conversation viaget_conversation_service+get_event_service, stores the event service onrequest.state, and validatespath/workspace_dirparams against the conversation workspace root usingPath.resolve().is_relative_to(). -
MCP probing removed from scoped routes � The
MCPProbeScope/runtime_verifiedfields and the scoped MCP test endpoint were removed, keeping the runtime router focused on workspace/file/bash/git/desktop/vscode operations only. This is a correct narrowing. -
Runtime fields removed from
ConversationInfo�runtime_status,can_resume, andruntime_errorwere moved off every conversation response and into the dedicatedConversationRuntimeInfomodel exposed only viaGET /runtimeandPOST /runtime/reprovision. Cleaner contract. -
File router split �
file_discovery_router(/home,/search_subdirs) is mounted globally;file_router(download, upload, archive, download-trajectory) is reused under the conversation-scoped prefix. Binary response schemas are typed on their owning routes, withapplication/jsonstripped from scoped FileResponse routes viaRuntimeRouter.add_api_route. -
Bash lifecycle is now intrinsic �
_tasksset,_closedflag, andCancelledError�SIGKILLhandling are part ofBashEventServiceitself (not a wrapper).close()cancels all tracked tasks and awaits them. Thedefault_cwdfield enables per-conversation working-directory isolation, and_validate_cwdinbash_router.pyrejects cwd values outside the workspace. -
TypeScript client transport �
ConversationScopedHttpClientrewrites/api/...paths to/api/conversations/{id}/..., blockscidoverride, rejects path traversal (../.segments, query strings in URL), and falls back to legacycidquery param for older servers. Server-info capability discovery is cached and retried on failure (503 propagates, 404 falls back). -
RemoteConversationworkspace binding � After create/load/fork, the workspace is rebound with the conversation ID from the server response, ensuring all subsequent runtime operations are scoped correctly.
Security verification
- Path containment:
Path.resolve().is_relative_to()correctly handles symlinks and..traversal. Relative paths are rejected (is_absolute()check). Verified across file, vscode, and bash cwd validation. - Conversation isolation: Bash history, working directories, and file access are scoped per conversation. Cross-conversation access returns 422. Test
test_local_runtime_workspace_and_terminal_contextconfirms this. - Process cleanup:
close()cancels tasks and kills process groups. Testtest_conversation_close_stops_runtime_terminalverifies orphaned processes are killed. - Client-side guards:
cidoverride prevention, path traversal rejection, and workspace-session identity enforcement are all tested inconversation-scope.test.ts.
Test results
All relevant tests pass:
test_runtime_router.py: 6/6 passed (workspace isolation, path containment, bash lifecycle, vscode defaults, route registration, concurrent requests)test_conversation_router.py: 2/2 passed (runtime requires existing conversation)test_server_details_router.py: 8/8 passed (capabilities, conversation_runtime field)conversation-scope.test.ts: 10/10 passed (routing, scoping, override rejection, discovery retry, older-server fallback, workspace binding)api-clients.test.ts: 86/86 passed (including new getRuntime/reprovisionRuntime)
The test_remote_conversation_live_server.py cross test has an environment config issue (env_parser IndexError) unrelated to this PR's changes.
Risk assessment: LOW
- All changes are additive API surface and infrastructure.
- Agent terminal execution uses the SDK TerminalTool, not these REST endpoints.
- No dependency changes, no agent loop changes.
- Backward compatibility preserved: legacy routes work, TypeScript clients fall back gracefully.
Generated by OpenHands AI on behalf of the user.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Approved on head commit 72a74b3. No blocking findings — see the comment for the detailed review.
Review of head commit
|
|
@OpenHands Find the repo conventions on the temporary artefacts in Edit the PR description to include a htmlpreview link so we can see it. |
|
I'm on it! enyst can track my progress at all-hands.dev |
5 participants
HUMAN:
I did a test of an automation-driven software factory that was based on PRs stacked on top of this, so I'm pretty sure it works fine.
AGENT:
Why
Canvas must address the workspace and runtime services owned by a specific conversation. This PR supplies that shared contract by reusing the agent server's existing service routers; #3403 adds Docker dispatch and lifecycle behind the same routes.
Summary
/api/conversations/{id}. Local requests resolve the owning conversation, keep Bash history and working directories separate, stop commands when their Bash service closes, and reject workspace paths belonging to another conversation.conversationIdonly to the existing TypeScript clients that perform runtime operations and toRemoteWorkspace. ARemoteConversationbinds its workspace to the ID returned by create, load, or fork. GenericBaseWorkspaceOptionsremains conversation-agnostic because local and standalone workspaces can exist before any conversation.cidbehavior.main.The narrowed implementation deliberately excludes a second runtime facade, a connection abstraction, scoped MCP probing, duplicate runtime fields on every conversation response, a duplicate workspace-mode signal, and a compiled-client feature flag. Lost-create-response recovery is independent #5036.
Review order: #4966 → #5017 → #3403. The profile-schema foundation #4931 is merged. #4998/#5005/#5008 were consolidated under #5016.
REST API contract changes
Compared with base OpenAPI
15a9b8609c12for public/api/**paths.Validation
main; the REST compatibility checker reports no unapproved break..pr/sdk4966-narrow-client/.How to Test
Not included
Docker lifecycle, credentials, runtime recovery and demo stay in #3403. Create-response reconciliation is independent in #5036. Async masking (#4967) and title generation (#4968) are independent PRs, not stack members.
openhands+astra requested by @neubig
This PR was created by an AI agent (OpenHands) on behalf of @neubig.
Issue Number
Part of #5016; split from #3403 at the author’s request.
🐳 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:1e1a610-pythonRun
All tags pushed for this build
About Multi-Architecture Support
1e1a610-python) is a multi-arch manifest supporting both amd64 and arm641e1a610-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.
Current narrowed-client recording · exact revisions and limits. A real model creates and reads a file while Canvas sends all shell operations through the selected conversation route.
Runtime stack cleanup
The current implementation and tests remove 364 net lines from the previous head: three unpublished public wrappers, alternate constructor inputs, a redundant capability cache, and path-specific response metadata handling. Independent create-response reconciliation and its tests moved to #5036. Existing unscoped clients remain supported; no deprecated shim is needed for unpublished PR-only APIs.