feat: dispatch automation bundles through SDK conversation runtimes - #449
feat: dispatch automation bundles through SDK conversation runtimes#449neubig wants to merge 1 commit into
Conversation
|
Warning Your comment is too long (maximum is 65536 characters), so the coverage report was not added. See the job log for how to reduce it. |
|
🤖 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
This PR adds an opt-in Docker conversation backend for local automation runs. Each run gets its own agent-server conversation, a scoped runtime credential, and no access to the host control key or shared callback key. Completion is verified via scoped bash history by the watchdog, and concurrency is limited by docker_max_concurrent_runs.
The design is sound: credential isolation is correct (the bundle receives only the inner SESSION_API_KEY, not the outer host key or callback key), the concurrency limiter prevents overload, and the watchdog's expanded stale detection is necessary since the bundle lacks the callback credential. Tests exercise real code paths with MockTransport rather than just asserting mocked calls.
Non-blocking observations (not preventing approval):
-
Cancel path lacks error handling for runtime release (
router.py:890):cleanup_after_verificationis called without a try/except in the cancel endpoint. If the Docker runtime release fails, the cancel endpoint returns 500 even though the run was already marked CANCELLED. The watchdog path wraps this call in try/except - the cancel path should do the same for consistency. -
Hardcoded
AGENT_SERVER_URL(backends/docker.py:65): The Docker backend hardcodeshttp://127.0.0.1:8000instead of using the parent class'ssandbox_agent_server_urloverride. This is fine for the stated single-process target, but will surprise anyone running the agent server on a non-default port. Consider falling back toself.sandbox_agent_server_url or "http://127.0.0.1:8000". -
Hardcoded
max_iterations: 160(backends/docker.py:44): Not configurable per-automation. Acceptable for an initial opt-in feature, but worth a follow-up to make it overridable.
None of these are blocking - the core logic, security model, and test coverage are solid.
[RISK ASSESSMENT]
- Overall PR: LOW
- Opt-in only: no change to existing Cloud or shared local modes unless
AUTOMATION_DOCKER_AGENT_PROFILEis explicitly set. - Credential isolation verified: host key and callback key are not forwarded to the bundle.
- Concurrency control prevents resource exhaustion.
VERDICT: Worth merging
KEY INSIGHT: The credential isolation model - provisioning a scoped runtime key via /runtime/credentials and withholding the host control key - is the correct security boundary for running untrusted bundle code in isolated Docker conversations.
|
Posted an APPROVED review on PR #449 (commit f8ca706) with 2 inline comments and a full review body. Verdict: Worth merging (APPROVED) Summary of findings:
Non-blocking observations posted as inline comments:
Review URL: #449 (review) This comment was posted by an AI agent (OpenHands). |
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
neubig
changed the title
|
🤖 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
This PR consolidates the SDK conversation dispatch into a single ConversationAgentServerBackend that works across both local and Docker workspace runtimes. The design is sound: credential isolation is correct (the bundle receives only the scoped runtime key, never the host key or callback key), the concurrency limiter prevents overload and per-automation overlap, and the watchdog's expanded stale detection covers the missing-callback case.
The previous review's two findings (cancel path error handling, hardcoded URL) are both addressed in this commit — cancel now wraps cleanup in try/except, and the Docker backend is consolidated into the conversation backend with sandbox_agent_server_url fallback.
Non-blocking observations (not preventing approval):
-
api_prefixproperty constructs a client on every access (conversation.py:32-37): The property instantiates a newAsyncAgentServerClientwithout anhttp_clienteach time it's read. It's called in bothget_execution_context(line 68) andverify_run(line 116). Since it only reads a computed string, this could be cached on the instance after first resolution to avoid repeated object creation. -
Missing
AUTOMATION_CALLBACK_API_KEYis by design but has a latency cost:build_env_varsdeliberately omits the callback key (the README confirms: "never the outer server key or shared callback key"). This means all completions go through the watchdog, not the callback path. The watchdog's expanded stale detection (bash_command_id.isnot(None) | timeout_at < now) handles this, but every running conversation-mode run will be verified on each watchdog scan. Withconversation_max_concurrent_runsdefaulting to 2 this is bounded, but worth noting that completion latency is tied to the watchdog scan interval rather than being immediate. -
SDK git pin must be replaced before merge: The PR description explicitly calls this out.
pyproject.tomlpinsopenhands-sdkto git commit79021c687...instead of a released version. This is a known pre-merge requirement, not a code issue.
[RISK ASSESSMENT]
- Overall PR: 🟢 LOW
- Opt-in only: no change to existing Cloud or shared local modes unless
AUTOMATION_AGENT_PROFILEis explicitly set. - Credential isolation verified: host key and callback key are not forwarded to the bundle in Docker mode.
- Concurrency control prevents resource exhaustion (default max 2 concurrent runs).
- Cancel path correctly handles cleanup failures without affecting the state transition.
- The watchdog expansion is bounded by the concurrency limiter.
VERDICT: ✅ Worth merging — replace the SDK git pin with the released version as the PR description states.
KEY INSIGHT: The unified ConversationAgentServerBackend cleanly replaces the previous Docker-specific backend by discovering the runtime kind from server capabilities and branching only on credential provisioning and workspace path, keeping the execution contract identical for the bundle.
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.
|
🤖 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
🟢 Good taste — The conversation backend cleanly unifies local and Docker workspace execution behind one bundle-facing contract. The design reuses the existing backend abstraction, dispatcher pipeline, and verification helpers without duplicating logic. Runtime credential scoping (host key for local, inner session key for Docker) is correct and well-tested.
Key findings
No blocking issues found. The implementation is sound:
ConversationAgentServerBackendproperly resolves the runtime kind once and caches it, provisions scoped credentials per mode, and releases Docker runtimes while preserving local history.- The dispatcher's concurrency limiting (
min(batch_size, 1, max_concurrent - active)) correctly bounds admission to one new run per poll cycle while respecting the global cap. - Cancellation correctly commits the state transition before attempting runtime cleanup, and cleanup failures are caught and logged without reverting the cancellation — the right trade-off.
- The watchdog's expanded stale condition (runs with
bash_command_idbut not yet timed out) is a deliberate design choice: without a sandbox to observe, the only way to detect completion is to query the agent server. This adds periodic verification load but is the correct approach for the conversation backend. - Env var layering is correct: the dispatcher's
AUTOMATION_CONVERSATION_IDoverride forsubject_keyruns takes precedence over the backend's default, andSESSION_API_KEYfrombuild_env_varsis not clobbered by the sandbox-id path (sincectx.sandbox_idisNonefor conversation runs).
Non-blocking observations
-
cleanup_after_verificationuses a default-timeouthttpx.AsyncClient()(conversation.py:120). The default httpx timeout is 5s; Docker runtime release could exceed that. Since errors are caught upstream, this won't crash but could silently leave Docker resources unreleased. Consider passing an explicit timeout (e.g., 30s) consistent withverify_run_on_agent_server's 60s client. -
The
api_prefixproperty (conversation.py:31-37) constructs a throwawayAsyncAgentServerClienton each access. This is called fromget_execution_context,verify_run, andcleanup_after_verification. Ifapi_prefixis a locally computed string (as the test suggests), this is wasteful but harmless. If it ever involves a network call, it would need to be async. -
Temporary SDK git pin — The PR description explicitly states this must be replaced with the SDK release before merging. This is a known pre-merge gate, not a code defect.
Test coverage
Tests are well-structured and exercise real code paths through mock transports rather than mocking the unit under test:
test_conversation_backend.pyvalidates both runtime kinds end-to-end (context creation, env vars, scoped execution, release).test_cancel_run.pycovers both cleanup success and failure paths.test_local_mode.pyandtest_execution.pyproperly test through the SDK's HTTP transport layer.
Risk Assessment
[RISK ASSESSMENT]
- Overall PR: 🟡 MEDIUM
- New execution backend with dispatcher/watchdog/router changes across 17 files. Well-tested with good error handling boundaries. The temporary SDK git pin and the expanded watchdog scanning scope are the main risk vectors, both acknowledged in the PR description. No security issues found — credential scoping is correct (Docker gets inner session key only, never the host key).
VERDICT: ✅ Worth merging — Core logic is sound, credential isolation is correct, and error handling is appropriately defensive. Replace the SDK git pin with the release before merging as noted in the PR description.
KEY INSIGHT: The conversation backend's inheritance from LocalAgentServerBackend is the right abstraction — it reuses workspace resolution and env var structure while overriding only the runtime-aware methods, keeping the surface area minimal.
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.
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
Code review for PR #449 (feat: dispatch automation bundles through SDK conversation runtimes) has been posted to GitHub as APPROVED. Review verdict: ✅ Worth merging (🟡 MEDIUM risk) Key findings:
Review URL: #449 (review) 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
This PR adds a ConversationAgentServerBackend that unifies local and Docker workspace execution through SDK conversation runtimes (RemoteConversation.create / AsyncRemoteWorkspace). The design is sound:
- Credential isolation is correct: Docker workers receive only the inner
SESSION_API_KEYfromget_runtime_session_key(), never the host key or callback key. Local runs retain the host key since they share the persistent server. - Concurrency bounding in
_poll_pending_runslimits dispatch to 1 run per poll cycle and caps total running atconversation_max_concurrent_runs, with overlap prevention vianot_in(active). - Watchdog expansion polls all profile-backed RUNNING runs with a
bash_command_idon every scan — necessary since workers lack the callback credential. - Cancel path correctly wraps runtime cleanup in try/except so a failed Docker release doesn't undo the already-committed CANCELLED state.
- Tests exercise real SDK HTTP transport paths via
MockTransportrather than asserting mocked calls, covering local/Docker routing, credential scoping, upload rejection, and download failure.
One should-fix issue (inline comment below): the conversation_id written to the DB after successful dispatch uses str(run.id) instead of the subject-derived conversation ID, producing wrong metadata for continue_conversation runs.
Non-blocking observations (not preventing approval):
-
SDK git pin (
pyproject.toml):openhands-sdkis pinned to an immutable git commit. The PR description and code comment both state this must be replaced with the SDK release before merging. This is an explicit merge prerequisite, not a code defect. -
Hardcoded
http://127.0.0.1:8000(conversation.py:115): Docker fallbackAGENT_SERVER_URLis hardcoded. Fine for the single-process target, but will surprise anyone running the agent server on a non-default port. Thesandbox_agent_server_urloverride already exists. -
min(batch_size, 1)serialization (dispatcher.py:148-152): Profile mode always dispatches at most 1 run per poll cycle regardless ofconversation_max_concurrent_runs. Conservative but correct — the aggressive watchdog polling compensates for completion latency.
[RISK ASSESSMENT]
- Overall PR: 🟡 MEDIUM
- The conversation backend introduces a new execution path with credential scoping and runtime lifecycle management. The design is well-structured with proper cleanup and error handling. The SDK git pin is a temporary measure with an explicit replacement plan. The conversation_id metadata mismatch is the only correctness issue, and it doesn't affect functional routing or verification.
VERDICT: ✅ Worth merging — core logic is sound, credential isolation is correct, and the one should-fix issue is metadata-only. Replace the SDK git pin with the released dependency as stated in the merge prerequisites.
KEY INSIGHT: The unified backend correctly isolates credentials by deriving the runtime session key from the Docker runtime and never exposing the host key, while the watchdog's expanded polling closes the completion-detection gap left by removing callback credentials from workers.
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. 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.
|
Review posted successfully to PR #449. Verdict: ✅ APPROVED (Worth merging, 🟡 MEDIUM risk) Summary of findings:
The design is sound: credential isolation prevents Docker workers from seeing the host key or callback key, concurrency bounding works, watchdog polling compensates for removed callback credentials, and tests exercise real SDK HTTP transport paths. This comment was posted by an AI agent (OpenHands). |
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.
Verdict: Worth merging
Taste Rating: Good taste — The conversation backend is a clean, well-factored abstraction that unifies local and Docker workspace execution behind one SDK contract. Credential scoping is correct: Docker workers receive only the runtime session key, never the shared Automation callback key. Concurrency admission is properly bounded, and the watchdog polling path reuses existing verification infrastructure.
Key findings
1. SDK git pin (merge prerequisite, acknowledged)
pyproject.toml line 29 replaces openhands-sdk==1.46.0 with a git pin to an unreleased commit. The PR description explicitly states this must be replaced with the released SDK before merging. This is the only blocking item, and it is already documented as a merge prerequisite. No action needed from the author beyond the planned SDK release swap.
2. Full event loading in _fetch_latest_finish_tool_response
conversation_outcome.py line 70 loads all conversation events via conversation.state.events and materializes them into a list. The previous implementation used a paginated search API with limit=100. For long-running conversations with many events, this could be memory-intensive. This is a minor performance concern given the bounded concurrency (default 2 concurrent runs) and the best-effort nature of the finish-tool-response lookup. Not blocking, but worth noting if conversation lengths grow significantly.
3. Connection pool per operation
The migration from a shared httpx.AsyncClient to per-operation AsyncRemoteWorkspace context managers means each upload/bash/start call creates and tears down a separate HTTP connection pool. This is less efficient than the previous shared-client pattern but functionally correct. Given these are per-run operations (not high-frequency), the impact is negligible.
What works well
- Concurrency control: The dispatcher correctly limits batch size to
min(batch_size, 1, max_concurrent - active)and prevents overlap for the same automation. Thenot_in(active)filter ensures no automation gets a second concurrent run. - Credential isolation: Docker workers get a scoped
runtime_session_key; the host key is never exposed in env vars. Thebuild_env_varsguard (if not self.runtime_api_key: raise RuntimeError) prevents credential-less dispatch. - Callback gating: The condition at dispatcher line 356 correctly omits callback URLs for profile-backed runs when
local_api_keyis set, routing completion through watchdog polling instead. - Cancel cleanup: The cancel endpoint commits the state transition before attempting runtime cleanup, and logs cleanup failures without reverting the cancellation. Correct priority: state first, cleanup best-effort.
- Watchdog polling: Profile-backed runs are verified on every scan when
bash_command_idis set, not just after timeout. This provides timely completion detection for runs that deliberately do not receive callback credentials. - Subject-derived conversation IDs: The
conversation_idproperty consistently usesconversation_id_for()across creation, verification, and cleanup, ensuringcontinue_conversationruns address the right thread.
Risk Assessment
LOW — No critical bugs, security issues, or design flaws. The architecture is sound and the credential model is well-designed. The only merge blocker is the unreleased SDK pin, which is an explicitly acknowledged prerequisite. The performance observations are minor and acceptable at the expected scale.
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 is merge-ready.Was this review helpful? React with thumbs up or thumbs down to give feedback.
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
Review posted successfully to PR #449 as APPROVED (review ID: 5192488753). Summary of review:
The review includes one non-blocking inline comment on 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.
Review: feat: dispatch automation bundles through SDK conversation runtimes
Taste Rating: 🟢 Good taste — The migration from direct HTTP calls to SDK-typed conversation/workspace APIs is well-structured. One contract across local and Docker runtimes, proper credential isolation, and conservative concurrency control.
Architecture Assessment
The ConversationAgentServerBackend cleanly extends LocalAgentServerBackend, reusing the base class while overriding only the runtime-specific pieces (conversation creation, credential provisioning, runtime release). The dual-mode design (local vs docker discovered from server capabilities) is a good data-structure-first approach — the runtime kind drives all behavior differences through a single _runtime_kind field.
Credential isolation is sound: Docker workers receive only the inner session key (get_runtime_session_key()), never the host server key. The callback URL conditional in the dispatcher correctly suppresses AUTOMATION_CALLBACK_URL for profile-backed runs, since those rely on watchdog polling instead.
The concurrency limiter (min(batch_size, 1, max - len(active)) with not_in(active)) ensures at most one in-flight run per automation and respects the global cap. Conservative (1 per poll cycle) but appropriate for the default max of 2.
Merge Prerequisite (acknowledged)
The SDK is pinned to an unreleased git commit (de5bc17e...). The PR description explicitly states this must be replaced with the released SDK dependency before merging, and lists the required SDK PRs (#4966, #5010, #3403). This is a known, documented blocker — not a code defect.
Non-blocking Observations
-
Redundant
AUTOMATION_CONVERSATION_IDcomputation: The dispatcher'sif run.subject_key:block (line ~378) recomputesconversation_id_for(...)and overwrites the value already set byConversationAgentServerBackend.build_env_vars(). Both produce the same UUID from the same inputs, so this is harmless but fragile — if one computation path changes, they could diverge silently. Consider having the dispatcher read fromctx.runtime_conversation_id(already computed by the backend) instead of recomputing. -
get_command_outputreturn type assumption:agent_server.pytreats the SDK'sworkspace.get_command_output()return as a dict (.get("exit_code")). This depends on the unreleased SDK's actual return type. The tests mock it as dict-like, which wouldn't catch a type mismatch with the real SDK. Low risk given the live testing evidence in the PR description. -
Watchdog polls all runs with
bash_command_idon every scan whenagent_profileis set. This is by design (profile-backed completion is watchdog-driven), but it means every 60s the watchdog makes an HTTP call per running run. Fine at the default concurrency cap of 2, but worth noting ifconversation_max_concurrent_runsis raised significantly.
Testing
Tests are thorough: the new test_conversation_backend.py exercises both local and docker runtimes with subject/no-subject variants, validates the env var contract, scoped execution paths, and release lifecycle. The sdk_http_transport fixture correctly intercepts SDK-owned HTTP pools. The cancel-run test covers both success and cleanup-failure paths. Dispatcher tests verify conversation ID linking and callback URL suppression.
Risk Assessment
[RISK ASSESSMENT]
- Overall PR: 🟡 MEDIUM (due to unreleased SDK dependency and substantial execution-path changes)
- The core logic is sound and well-tested. The medium rating reflects the integration risk with unreleased SDK APIs and the fact that this replaces the entire execution helper layer (upload, bash, verify) with SDK calls. Live Canvas evidence in the PR description mitigates this.
VERDICT: ✅ Worth merging — after replacing the SDK git pin with the released dependency.
KEY INSIGHT: The single-conversation-contract design eliminates the local/docker branching that would otherwise permeate the dispatch pipeline, and the credential isolation (inner session key for Docker workers) is a clean security boundary.
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.
|
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.
Verdict: ✅ Worth merging
🟢 Good taste — The PR cleanly introduces a unified ConversationAgentServerBackend that inherits from LocalAgentServerBackend, providing one bundle-facing contract across local and Docker workspace runtimes. The design eliminates special cases rather than adding conditional branches.
Key strengths
-
Credential isolation is sound. Docker workers receive only the inner session key (
get_runtime_session_key()), never the host server key or shared callback credential. Thebuild_env_vars()guard that raisesRuntimeErrorwhenruntime_api_keyis empty prevents accidental credential leakage. -
Concurrency limiting is correct.
_poll_pending_runscounts active RUNNING runs, caps admission to 1 per poll cycle, and excludes automations with already-running runs vianot_in(active). Themin(batch_size, 1, max_concurrent - active)logic correctly returns[]when at capacity. -
Conversation ID consistency. The subject-derived
conversation_id_foris used identically across backend creation, worker context, verification, and cleanup. The dispatcher persists the same ID the worker uses. Non-subject runs correctly fall back torun.id. -
Outcome/turn resolution avoids side effects.
fetch_latest_finish_tool_response_for_runand_resolve_agent_servernow checkisinstance(backend, LocalAgentServerBackend)and callget_api_key()directly instead ofget_execution_context()— correctly avoiding provisioning a new conversation just to read state or send a turn. This is a real bug fix sinceConversationAgentServerBackend.get_execution_contextcreates a conversation. -
Callback conditional is well-reasoned. Profile-backed workers don't receive
AUTOMATION_CALLBACK_URLwhenlocal_api_keyis set, relying on watchdog polling instead. The condition covers the legacy path (AUTOMATION_CALLBACK_API_KEYorOPENHANDS_API_KEYpresent) and the open-local path (nolocal_api_key). -
Watchdog polling integration.
mark_stale_runsnow picks up profile-backed runs withbash_command_idon every scan, not just after timeout._should_cleanup_sandbox_after_terminalcorrectly triggers Docker runtime release even withoutsandbox_id.
Merge prerequisites (acknowledged in PR description)
The openhands-sdk git pin (de5bc17e...) must be replaced with the released SDK dependency before merging. SDK PRs #4966 and #5010 must be released first. Docker deployments additionally need SDK #3403. These are cross-repository prerequisites, not issues with this PR's code.
Minor observations (non-blocking)
- Each
_upload/_bash/_start_bashcall creates a newAsyncRemoteWorkspacecontext, whereas the previous code shared a singlehttpx.AsyncClient. The SDK likely manages connection pooling internally, so this is a cleanliness trade-off, not a perf concern at the default 2 concurrent runs. _resolve_runtimeaccessesinfo["conversation_runtime"]without a.get()fallback — a server that doesn't advertise this field will raiseKeyError. This is a server API contract, so raising loudly is the right behavior, but worth noting for operational awareness.
[RISK ASSESSMENT]
- Overall PR: 🟢 LOW
- Credential isolation is correctly implemented and tested.
- Concurrency bounds are enforced at the dispatcher level.
- The change is scoped to local mode (profile-backed); cloud sandbox dispatch is unaffected.
- Comprehensive test coverage: 72+ tests covering local/Docker routing, credential scoping, upload rejection, conversation ID persistence, cancellation cleanup, and runtime release lifecycle.
- The only merge gate is the SDK release dependency, which the PR explicitly documents.
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.
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
Code review for PR #449 (feat: dispatch automation bundles through SDK conversation runtimes) has been posted to GitHub as APPROVED. Verdict: ✅ Worth merging — 🟢 LOW risk Key findings:
Merge prerequisite noted: The temporary Per the repo's custom code review guide, the review was submitted as APPROVED (not COMMENTED) since the verdict is "Worth merging" with LOW risk and no blocking issues. This comment was posted by an AI agent (OpenHands). |
|
@all-hands-bot please review the current head after the SDK pin refresh and explicitly approve it if there are no material findings. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review: feat: dispatch automation bundles through SDK conversation runtimes
Head commit reviewed: 9b4ede3ad01beffd8e247950658b10c64a768772
Scope of this review
Reviewed the current head after the SDK pin refresh (e238a22 → 9b4ede3). The only change since the last reviewed commit is the SDK git pin update in pyproject.toml and uv.lock (de5bc17e → e9392e7c) — no application code changes. I re-reviewed the full PR diff against main and ran the test suite.
Verdict: ✅ APPROVED — no material findings
What the pin refresh changes
The commit chore: refresh SDK integration pin updates only:
pyproject.toml: SDK git pinde5bc17e…→e9392e7c…uv.lock: corresponding lockfile update
No source files were touched. The new SDK revision (version 1.47.0) preserves the typed APIs this PR depends on:
AsyncRemoteWorkspace.file_upload()→FileOperationResult(with.success/.error)AsyncRemoteWorkspace.execute_command()→CommandResult(with.exit_code/.stdout/.stderr)AsyncRemoteWorkspace.start_command()→str(command ID)AsyncRemoteWorkspace.get_command_output()→dict[str, Any] | NoneAsyncRemoteWorkspace.get_runtime_session_key()→strAsyncRemoteWorkspace.release_runtime()→NoneRemoteConversation.create()/.attach()/.close()/.set_title()/.send_message()/.run()
All return types match the usage in conversation.py, execution.py, agent_server.py, conversation_outcome.py, and conversation_turn.py.
Full PR assessment (unchanged from prior reviews)
Architecture: The ConversationAgentServerBackend cleanly extends LocalAgentServerBackend, providing one bundle-facing contract across local and Docker runtimes. Runtime kind is discovered from server capabilities and drives all behavior differences through a single _runtime_kind field.
Credential isolation: Sound. Docker workers receive only the inner session key (get_runtime_session_key()), never the host server key. build_env_vars() raises RuntimeError if runtime_api_key is empty, preventing accidental credential leakage. The callback URL conditional correctly suppresses AUTOMATION_CALLBACK_URL for profile-backed runs.
Concurrency control: Correct. min(batch_size, 1, max_concurrent - active) with not_in(active) ensures at most one in-flight run per automation and respects the global cap. Returns [] when at capacity.
Conversation ID consistency: The subject-derived conversation_id_for is used identically across backend creation, worker context, verification, and cleanup. Non-subject runs fall back to run.id.
Cancel path: The router.py cancel path now wraps cleanup_after_verification in try/except, logging cleanup failures without blocking the cancellation — matching the watchdog path's behavior.
Outcome/turn resolution: fetch_latest_finish_tool_response_for_run and _resolve_agent_server correctly check isinstance(backend, LocalAgentServerBackend) and call get_api_key() directly instead of get_execution_context() — avoiding provisioning a new conversation just to read state or send a turn.
Testing
- 6/6
test_conversation_backend.pytests pass (local/docker × subject/no-subject + backend selection + cleanup timeout) - 153 passed, 0 failed in PR-relevant test files (66 errors are all Docker testcontainer setup failures — no Docker daemon in this environment; these are environment limitations, not code defects)
- CI on GitHub: 7 successful, 2 skipped, 0 failing
- The
sdk_http_transportfixture correctly intercepts SDK-owned HTTP pools viaMockTransport
Merge prerequisite (acknowledged, not a code defect)
The openhands-sdk git pin (e9392e7c…) must be replaced with the released SDK dependency before merging. SDK PRs #4966 and #5010 must be released first. Docker deployments additionally need SDK #3403. These are cross-repository prerequisites documented in the PR description, not issues with this PR's code.
Risk Assessment
🟢 LOW — The pin refresh is a pure dependency update with no code changes. The application code is sound, well-tested, and all CI checks pass. The only merge gate is the SDK release dependency, which the PR explicitly documents.
Generated by OpenHands AI on behalf of the user.
|
I've reviewed the current head ( SummaryThe only change since the last reviewed commit ( No material findings. The PR is approved. Key points confirmed:
Generated by OpenHands AI on behalf of the user. |
Co-authored-by: openhands <openhands@all-hands.dev>
9b4ede3 to
d49da84
Compare
3 participants
Why
Automation bundles need one SDK execution contract in local and Docker workspaces, with isolated conversation state and bounded worker admission.
Summary
RemoteConversation.create(workspace, request=StartConversationRequest(...))and use scopedAsyncRemoteWorkspaceAPIs, reusing the existing scheduler, backend interface, run records, upload/execution helpers, and runtime verification.This consolidates #451 so the PR contains the shared SDK implementation directly. Unreleased Docker aliases and the host profile-override map are removed. Per-automation profile selection is layered separately in #453.
The backend uses the dispatcher’s subject-derived conversation ID for continuing runs, consistently across creation, worker context, verification and cleanup. The existing implementation, SDK attachment/outcome helpers, and callback/runtime-link handling were moved from child #453 so this PR works independently. The dispatcher persists the same conversation ID that the worker uses.
Issue Number
Closes #448. Closes #450.
How to Test
The earlier SDK migration passed 71 focused backend/execution/local-mode checks, including local/Docker routing, private environment handling, delayed runtime cleanup, upload rejection, and nonzero download failure. All commit hooks pass. The earlier SDK migration has live Canvas parity proof: the same uploaded bundle completed a real agent task with exit 0 in local and Docker workspaces, using the canonical SDK objects. The explicit entry-point update passes six local/Docker backend tests, including one POST with no existence probe and canonical server request validation. The complete parent passes 39 combined backend, dispatch, follow-up, outcome and ORM tests, including real PostgreSQL verification of the persisted subject-derived ID. Refreshed live validation passed with the explicit create/attach API in both runtime modes. Earlier admission/runtime evidence remains linked separately below.
Existing SDK
file_uploadandexecute_commandreturn typed failures, which are checked before dispatch can succeed. They do not preserve HTTP-specific 429 classification; detached command start and output polling still propagate HTTP errors. Failed jobs remain visible to the existing scheduler/watchdog.Dependencies and review order
Native stack #454: review this PR before #453. The merged SDK runtime contract (#4966) and the #5010 workspace/conversation methods must be released, and the temporary SDK git pin replaced, before merging. Docker deployments additionally use SDK #3403. The original-author MinIO registry repair from #447 has merged into main and is included with attribution; its 17 S3 integration tests pass.
Live Agent Canvas evidence
After moving subject-ID support into #449, the assembled #453 production tree remains
9de630f76614a6a26ce8a9088e7907bab1865f54, identical to the published live recording. Tests additionally verify persisted subject-derived conversation metadata. The two subject-keyed backend cases fail before the parent fix and pass afterward.Current explicit API: live local/Docker demonstration · Exact revisions, bundle, results and limits. The identical uploaded bundle completed a real agent task with
RemoteConversation.create/.attachin both runtimes, exited 0, and produced the expected file in Canvas. The report retains the initial request-validation failure and successful rerun after the SDK serialization fix. All private workers and services were released afterward. This validates the explicit API migration; earlier admission, queued-profile and credential-scope recordings remain separate.Earlier SDK migration: 24-second local/Docker demonstration · Exact revisions, executed bundle, results and limits. Both runs completed a real DeepSeek task, wrote and independently read back the expected file, and appeared as Successful in Canvas. The identical bundle and SDK source hashes were verified in both workers; all private runtimes were released afterward.
The current recording validates dispatch/upload/execution parity; it does not repeat the earlier concurrency/admission or credential-scope matrix. Earlier evidence follows:
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.
Merge and release prerequisites
Must be released first: merged software-agent-sdk#4966 and open software-agent-sdk#5010, which must also merge. Replace the temporary SDK source pin with the released dependency before merging this PR. Docker deployments additionally need software-agent-sdk#3403; profile-scoped factory credentials need software-agent-sdk#4931 / software-agent-sdk#5017. These cross-repository prerequisites are not represented by native stack #454.
Final split audit
The SDK source pin now follows #5010 current head
e9392e7cf35fcb011acf2eca825a80f9a22774ea; this updates only dependency identity after that branch refresh. The execution, attachment, and runtime-identity implementation remains unchanged from the published explicit-API live proof. The parent’s 66 focused conversation/backend tests pass against that exact SDK head; the standalone dispatch proof above remains applicable. The released SDK dependency is still a merge prerequisite.