feat(dispatch): share the conversation contract across workspace runtimes - #451
feat(dispatch): share the conversation contract across workspace runtimes#451neubig wants to merge 6 commits 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
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): share the conversation contract across workspace runtimes
The refactoring is well-structured — ConversationAgentServerBackend cleanly unifies the local and Docker conversation contracts behind a single runtime-resolving backend, and the compatibility shim (DockerAgentServerBackend subclass with _runtime_kind = "docker") preserves the existing Docker-only deployment path. The config property layering (run_agent_profile, run_concurrency_limit) correctly falls back to Docker-only settings when agent_profile is unset.
Blocking Issue
Unreleased SDK dependency: pyproject.toml pins openhands-sdk to a git commit from software-agent-sdk PR #5010, which is still open and unmerged. The PR description acknowledges this ("Replace that integration pin with the SDK release before merging"), but the PR is labeled type: feat with no WIP/draft marker. Merging this as-is would lock the automation service's production dependency to an unreviewed, unreleased branch of another repository. The SDK PR must be merged and released to PyPI first, and the pin replaced with a versioned release, before this can merge.
Non-blocking observation
The api_prefix property constructs a throwaway AsyncAgentServerClient on each access (in get_execution_context return and verify_run). It only computes a path string so there is no I/O, but caching the prefix after first computation would avoid redundant object construction. Minor.
[RISK ASSESSMENT]
- Overall PR: 🟡 MEDIUM — The code changes themselves are sound and low-risk (clean refactor with good test coverage). The risk is elevated to MEDIUM solely because of the unreleased SDK dependency pin, which introduces supply-chain and stability risk if merged before the SDK is released.
VERDICT:
❌ Needs rework — Block on the SDK PR #5010 being merged and released. Replace the git-pin with a PyPI version before merging. No code changes needed beyond the dependency update.
KEY INSIGHT:
The conversation backend unification is correct and well-tested, but the dependency on an unmerged upstream PR makes this not-yet-mergeable.
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 is merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
| "jmespath>=1.0", | ||
| "openhands-sdk==1.46.0", | ||
| # Temporary integration pin for software-agent-sdk#5010; replace with its release. | ||
| "openhands-sdk @ git+https://github.com/OpenHands/software-agent-sdk.git@79021c687c63bd1e925cf157f5897ceeaa12f029#subdirectory=openhands-sdk", |
There was a problem hiding this comment.
Blocking: openhands-sdk is pinned to git commit 79021c6 from software-agent-sdk PR #5010, which is still open and unmerged. The PR description says to replace this with the SDK release before merging, but the PR carries no WIP/draft marker. This must be replaced with a released PyPI version before merge — otherwise the automation service's production dependency is locked to an unreleased branch of another repository.
Why
The initial Docker dispatcher supplied a run conversation ID and scoped endpoints while local dispatch did not, forcing automation authors to depend on a workspace kind. The same automation should run unchanged in either environment.
Summary
Introduce a shared conversation backend selected by
AUTOMATION_AGENT_PROFILE. It reads the server's advertised runtime and exposes the same bundle environment, profile selection, scoped upload/execution, and verification contract in local and Docker workspaces. Only resource acquisition, selected credentials, workspace paths, and release differ. Keep the earlier Docker settings and unconfigured legacy local/Cloud behavior compatible.Issue Number
Closes #450
How to Test
85 backend/dispatch tests passed, including parameterized local/Docker contract tests. Live validation executed the identical probe tarball (SHA256
783e0d569303e29ee9b130790c1e4fc218047c814f8f49b83e2d9af897f3d106) against real local and Docker servers; both authenticated to their supplied conversation, wrote workspace evidence, and returnedWORKSPACE_CONTRACT_PASS. The Docker runtime was released afterward.Ruff passed. An additional 162 watchdog/router tests passed, and Pyright reported zero errors and warnings. Local workspaces retain the existing single-tenant server credential boundary and are not security sandboxes.
Notes
Review after #449. The factory production deployment continues to use Docker; workspace choice does not change its workflow bundle.
Runtime communication now delegates to
AsyncAgentServerClientfrom OpenHands/software-agent-sdk#5010. Release that SDK and update the dependency pin before merging this draft. Local integration explicitly uses the SDK source; no consumer defines Agent Server HTTP routes.After migration: 49 execution/local/Docker tests pass (79 additional backend/dispatcher tests passed in the preceding run); Ruff and Pyright report zero errors. The new SDK-using identical bundle passed on both real runtimes with SHA256
3dd034cf9207d25e9aa5630a953bd5faaf0b89c5d999d8296d8b83a9e92d85d1. Local CIDcc36ada7-4224-4ad6-b591-f347a721bb41, Docker CIDdab2cbd4-0f9a-46bf-9091-2fff9cb2512d. Both returnedWORKSPACE_CONTRACT_PASS; temporary resources were released.Clean source installation now pins SDK #5010 by immutable commit (with the lockfile updated). In a fresh isolated virtual environment, all 128 execution/backend/dispatcher tests pass. Replace the integration pin with the SDK release before merging.
Current validation
Parent #449 now includes the original-author MinIO fix from #447. Dispatcher CI is green; the previous 17 S3 fixture failures are resolved.