feat: dispatch automation bundles through SDK conversation runtimes - #449

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):

1. Cancel path lacks error handling for runtime release (router.py:890): cleanup_after_verification is 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.

2. Hardcoded AGENT_SERVER_URL (backends/docker.py:65): The Docker backend hardcodes http://127.0.0.1:8000 instead of using the parent class's sandbox_agent_server_url override. 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 to self.sandbox_agent_server_url or "http://127.0.0.1:8000".

3. 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_PROFILE is 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.

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):

1. api_prefix property constructs a client on every access (conversation.py:32-37): The property instantiates a new AsyncAgentServerClient without an http_client each time it's read. It's called in both get_execution_context (line 68) and verify_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.

2. Missing AUTOMATION_CALLBACK_API_KEY is by design but has a latency cost: build_env_vars deliberately 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. With conversation_max_concurrent_runs defaulting to 2 this is bounded, but worth noting that completion latency is tied to the watchdog scan interval rather than being immediate.

3. SDK git pin must be replaced before merge: The PR description explicitly calls this out. pyproject.toml pins openhands-sdk to git commit 79021c687... 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_PROFILE is 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:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger 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.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. 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 /iterate to 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 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:

• ConversationAgentServerBackend properly 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_id but 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_ID override for subject_key runs takes precedence over the backend's default, and SESSION_API_KEY from build_env_vars is not clobbered by the sandbox-id path (since ctx.sandbox_id is None for conversation runs).

Non-blocking observations

1. cleanup_after_verification uses a default-timeout httpx.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 with verify_run_on_agent_server's 60s client.

2. The api_prefix property (conversation.py:31-37) constructs a throwaway AsyncAgentServerClient on each access. This is called from get_execution_context, verify_run, and cleanup_after_verification. If api_prefix is 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.

3. 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.py validates both runtime kinds end-to-end (context creation, env vars, scoped execution, release).
• test_cancel_run.py covers both cleanup success and failure paths.
• test_local_mode.py and test_execution.py properly 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:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger 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.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. 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 /iterate to 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: 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 pin de5bc17e… → 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] | None
• AsyncRemoteWorkspace.get_runtime_session_key() → str
• AsyncRemoteWorkspace.release_runtime() → None
• RemoteConversation.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.py tests 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_transport fixture correctly intercepts SDK-owned HTTP pools via MockTransport

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.

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Verdict: ✅ APPROVED

🟢 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 throughout the dispatch pipeline.

Key strengths

1. 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. The build_env_vars() guard that raises RuntimeError when runtime_api_key is empty prevents accidental credential leakage. The callback URL conditional in the dispatcher correctly suppresses AUTOMATION_CALLBACK_URL for profile-backed runs.

2. Concurrency limiting is correct. _poll_pending_runs counts active RUNNING agent-mode runs, caps admission to 1 per poll cycle via min(batch_size, 1, agent_capacity), and excludes automations with already-running runs via not_in(active). Script-mode runs remain runnable when agent capacity is exhausted.

3. Conversation ID consistency. The subject-derived conversation_id_for is 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 to run.id.

4. Outcome/turn resolution avoids side effects. fetch_latest_finish_tool_response_for_run and _resolve_agent_server now check isinstance(backend, LocalAgentServerBackend) and call get_api_key() directly instead of get_execution_context() — correctly avoiding provisioning a new conversation just to read state or send a turn. This is a real bug fix since ConversationAgentServerBackend.get_execution_context creates a conversation.

5. Watchdog polling integration. mark_stale_runs now picks up agent-mode runs with bash_command_id on every scan, not just after timeout. _should_cleanup_sandbox_after_terminal correctly triggers Docker runtime release even without sandbox_id.

6. Cancel path. The cancel endpoint wraps cleanup_after_verification in try/except, logging cleanup failures without blocking the cancellation — matching the watchdog path's behavior.

Non-blocking observation

Deferred cleanup skips runtime release for agent-mode runs without sandbox_id (watchdog.py:404-415): When sandbox_cleanup_delay_seconds > 0, the watchdog calls _defer_sandbox_cleanup, which returns early if sandbox_id is None (line 197-198). For agent-mode Docker runs — which have no sandbox_id but do need release_runtime() — this means the runtime release is skipped entirely. With the default sandbox_cleanup_delay_seconds=0 the immediate cleanup_after_verification path runs correctly, so this only affects non-default configurations. Worth a follow-up to route agent-mode cleanup through the immediate path regardless of the delay setting, or to handle runtime release inside _defer_sandbox_cleanup.

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. These are cross-repository prerequisites documented in the PR description.

Risk Assessment

🟢 LOW — The change is scoped to local mode (profile-backed); cloud sandbox dispatch is unaffected. Credential isolation is correctly implemented and tested. Concurrency bounds are enforced at the dispatcher level. Comprehensive test coverage covers local/Docker routing, credential scoping, conversation ID persistence, cancellation cleanup, and runtime release lifecycle.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. 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 /iterate to 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 review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

This PR introduces a ConversationAgentServerBackend that provisions SDK conversations (via RemoteConversation.create / AsyncRemoteWorkspace) for execution_mode: "agent" automations, with bounded concurrency, watchdog-based completion polling, and scoped credential handling. The migration, schema, dispatcher capacity logic, and git-sync serialization are well-integrated.

The core design is sound: conversation IDs are deterministically derived from subject keys, the callback-credential gating prevents leaking the shared service key to profile-scoped workers, and the dispatcher capacity logic correctly prevents overlap for one automation while keeping script runs unblocked.

Findings

1. Docker runtime resource leak when sandbox_cleanup_delay_seconds > 0

_should_cleanup_sandbox_after_terminal now returns True for agent-mode runs even when sandbox_id is None. When sandbox_cleanup_delay_seconds > 0, the watchdog takes the _defer_sandbox_cleanup branch, which returns immediately for runs without a sandbox_id (line 193: if not sandbox_id: return) without stamping sandbox_cleanup_due_at. The janitor (cleanup_due_sandboxes) then never picks up these runs, and even if it did, it guards cleanup on if run.sandbox_id: (line 1031). The result: Docker agent-mode runtimes that complete or time out are never released when a cleanup delay is configured.

With the default (sandbox_cleanup_delay_seconds = 0) the immediate cleanup_after_verification path works correctly. This only affects deployments that explicitly set a delay, but it is a silent resource leak in that configuration. Consider either skipping the deferred path for agent-mode runs (calling cleanup_after_verification directly) or teaching _defer_sandbox_cleanup / cleanup_due_sandboxes to handle runs whose cleanup target is a runtime rather than a sandbox.

2. _fetch_latest_finish_tool_response loads entire conversation event history into memory

The previous implementation used a paginated search endpoint (limit=100, newest-first). The new SDK-based implementation calls conversation.state.events and dumps every event to JSON via model_dump(mode="json") before scanning for the latest finish tool response. For long-running agent conversations (hundreds of iterations), this loads the full event list into memory on every call. If the SDK does not lazy-load or paginate state.events, this is a potential memory and latency regression for outcome lookups.

3. SDK git pin — merge prerequisite

openhands-sdk is pinned to an immutable git commit (e9392e7c...). This is explicitly documented as temporary and must be replaced with the released SDK dependency before merging. Not a blocking review issue, but a release gate.

Risk Assessment

MEDIUM - The deferred cleanup gap (Finding 1) is a real resource leak in a non-default but supported configuration. The event-loading concern (Finding 2) is a performance regression that may not manifest until conversations grow large. Neither blocks the default deployment path, which works correctly.

Verdict

Worth merging - The core implementation is well-structured, the security boundary (callback credential gating) is correct, and tests cover the key scenarios. The deferred cleanup gap should be addressed before enabling sandbox_cleanup_delay_seconds with agent-mode automations, but does not block the default configuration.

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Verdict: Worth merging

Taste Rating: Good taste

This PR introduces a clean ConversationAgentServerBackend that extends LocalAgentServerBackend to provision agent conversations through the SDK's RemoteConversation.create / AsyncRemoteWorkspace APIs. The execution-mode selection is data-driven (a column on Automation / AutomationRun), the runtime discovery is delegated to the server's capabilities endpoint, and credential scoping ensures Docker workers never receive the outer server key. The watchdog polling expansion for agent-mode runs and the dispatcher's concurrency bounding are well-reasoned.

Key observations

No material bugs found. The implementation is sound across all reviewed paths:

1. Credential scoping (conversation.py lines 111-125, 134-148): Docker workers receive only the inner session key from get_runtime_session_key(), never the outer self.api_key. The build_env_vars guard at line 134 raises if credentials haven't been provisioned, preventing a partially-initialized backend from leaking an empty key.

2. Conversation ID consistency (dispatcher.py lines 528-535): The conversation_id is persisted to the run record after successful dispatch, using the same value the backend derived from the subject key. This is correctly guarded by ctx.runtime_conversation_id is not None.

3. Callback suppression (dispatcher.py lines 362-366): Agent-mode backends deliberately omit AUTOMATION_CALLBACK_API_KEY and OPENHANDS_API_KEY from env vars, so the callback URL is not injected when local_api_key is set. Completion is detected through watchdog polling of the SDK runtime. This is the intended design.

4. Watchdog polling (watchdog.py lines 572-578): Agent-mode runs with a bash_command_id are polled on every scan, which is the primary completion path. The timeout_at.isnot(None) guard ensures runs without a timeout are excluded. With max_concurrent_runs=2, the per-scan load is bounded.

5. Concurrency bounding (dispatcher.py lines 134-175): The capacity check counts RUNNING agent runs, limits batch_size to 1 when capacity exists, and excludes agent runs entirely when capacity is 0 -- while still allowing script-mode runs to proceed. This is correct.

6. Migration (025_add_execution_mode.py): Cross-database compatible, uses server_default="script" for both columns, and the downgrade drops both. Clean.

7. Sync SDK wrapping (conversation_outcome.py, conversation_turn.py): Sync RemoteConversation.attach calls are wrapped in asyncio.to_thread with proper finally cleanup (conversation.close(), workspace.reset_client()). Good pattern.

Merge prerequisite (not a code issue)

The SDK dependency is pinned to a git commit from the open software-agent-sdk#5010 PR. The PR description explicitly states this must be replaced with the released SDK before merging. This is acknowledged and tracked -- not a blocker for code review.

[RISK ASSESSMENT]

• Overall PR: MEDIUM
• Rationale: The change introduces a new execution mode that alters the dispatch and completion paths for agent-mode runs. The SDK git pin is an unreleased dependency from an open PR. The watchdog now polls agent runs on every scan, and the cancellation path releases Docker runtimes. All of these are well-tested with focused unit tests covering local/Docker routing, credential scoping, concurrency bounding, and cleanup failure resilience. The risk is medium primarily due to the unreleased SDK dependency and the breadth of the change (32 files, +1377/-538).

KEY INSIGHT: The ConversationAgentServerBackend cleanly separates runtime provisioning (server capabilities query + SDK conversation creation) from execution (shared upload/bash helpers), and the credential boundary between outer server key and inner session key is maintained consistently across creation, dispatch, verification, and cleanup.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger 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.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. 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 /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with thumbs up or thumbs down to give feedback.

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

This PR adds an explicit script/agent execution mode to automation definitions and run snapshots, introduces a ConversationAgentServerBackend that provisions SDK conversations for agent-mode runs, bounds concurrent agent conversations via dispatcher capacity logic, and migrates the upload/bash/verify/outcome/turn helpers from raw HTTP to the SDK's RemoteWorkspace/RemoteConversation APIs.

🟢 Good taste — The execution-mode field is a clean, explicit discriminator that replaces implicit backend selection. The capacity gating in the dispatcher correctly keeps host-side scripts dispatchable while bounding agent conversations. The watchdog's per-scan polling of agent runs (without sharing the callback credential) is a sensible primary completion path. The SDK migration eliminates hand-rolled HTTP plumbing in favor of typed SDK methods.

Findings

No material bugs, security problems, or design flaws found.

Noted (non-blocking): The pyproject.toml pins openhands-sdk to a git commit on the software-agent-sdk feature branch (e9392e7c...). The PR description explicitly documents this as a temporary integration pin that must be replaced with the SDK release before merge. This is a first-party package (same org), pinned to an immutable commit, so it is reproducible. Confirm the SDK release is published before merging.

[RISK ASSESSMENT]

• Overall PR: 🟡 MEDIUM — The change touches core dispatch, execution, verification, and completion paths. The temporary SDK git pin is a merge prerequisite that must be resolved. The core logic is sound and well-tested.

VERDICT: ✅ Worth merging — Core logic is sound, test coverage is comprehensive across both execution modes and runtime kinds.

KEY INSIGHT: Making execution mode an explicit, snapshotted field on both the automation and its runs cleanly separates deterministic script execution from agent conversation provisioning without coupling backend selection to the presence of a profile.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger 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.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. 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 /iterate to 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 review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

This PR introduces an explicit execution_scope (run vs conversation) that separates execution mode from agent-server provisioning. The design cleanly splits the old monolithic ExecutionBackend into AgentServerProvider (cloud sandbox vs existing server) and ExecutionBackend (run-scoped vs conversation-scoped), composed at dispatch time. The conversation backend provisions a profile-backed conversation, injects scoped runtime credentials (never the shared callback key), and relies on watchdog polling for completion. Run-scoped execution retains its existing callback path.

Assessment

Taste: Good taste. The provider/backend separation eliminates the old is_local_mode branching throughout the codebase. The execution_scope snapshot on AutomationRun ensures later definition edits do not affect in-flight runs. The migration is backward-compatible with server_default="run".

Key observations (non-blocking)

1. SDK git pin (pyproject.toml): openhands-sdk is pinned to a git commit instead of a released version. This is explicitly acknowledged as a merge prerequisite in the PR description and README. The pin must be replaced with the SDK release before merging.

2. Local-mode batch throttling (dispatcher.py): When conversation capacity > 0 in local mode, batch_size = min(batch_size, 1, conversation_capacity) throttles ALL pending runs (including run-scoped) to 1 per poll. Reasonable for single-tenant local mode but could slow run-scoped dispatch. Not a bug.

3. Watchdog polling (watchdog.py): Conversation-scoped runs are verified on every scan (default 60s) regardless of timeout. This is the intended primary completion path. The SQL condition is correct.

4. Callback URL conditioning (dispatcher.py): AUTOMATION_CALLBACK_URL is set only when the worker has callback credentials or no local_api_key is configured. ConversationBackend workers deliberately do not receive callback credentials, falling back to watchdog polling. Sound.

5. Runtime credential isolation (conversation.py): Docker workers receive only the inner session key, never the host server key. Tests explicitly assert this. Good security boundary.

No blocking issues found

• No KeyError risk at env_vars["AUTOMATION_CONVERSATION_ID"]: when ctx.runtime_conversation_id is non-None (conversation backend), build_env_vars() always sets that key.
• Cancel path for conversation runs is best-effort with proper exception handling.
• Migration uses generic SQLAlchemy types (cross-database compatible).
• Tests exercise real code paths with mock HTTP transports, not just mock-call assertions.

Risk Assessment

MEDIUM - Large architectural change (38 files, +1663/-706) introducing a new execution path. The SDK git pin is a merge prerequisite. No bugs or security issues found. The design is clean and well-tested.

Verdict: Worth merging (after replacing the SDK git pin with a release).

Key insight: The provider/backend separation is the right abstraction - it makes execution scope and server provisioning orthogonal concerns, eliminating mode-specific branching throughout the dispatcher and watchdog.