Revert PR #2190: Enable ACPAgent on RemoteRuntime API#2451
Conversation
This reverts merge commit ca621a4. Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🔴 Needs improvement - Missing critical context for revert decision
The Problem: No Justification for Revert
This PR reverts #2190 but the description only says "This reverts #2190" without explaining why. The validation shows tests pass, but not what problem this solves.
A revert of this magnitude (removing remote runtime support, retry logic, authentication, timeout handling, Docker image support, evaluation workflow integration) needs to answer:
- What broke? What specific bug/issue in #2190 requires a full revert?
- Why full revert? If a specific feature was broken, why not fix just that feature?
- Impact analysis: Who is affected? Are there users already using ACPAgent with remote runtime?
- Path forward: What needs to happen before remote ACPAgent support can be re-introduced?
Critical Issues
1. Breaking Change Without Deprecation
[openhands-agent-server/openhands/agent_server/event_service.py]
Hardcoding Agent.model_validate instead of using type(self.stored.agent) means:
- Any stored conversations with ACPAgent will fail to load
- Remote runtime API silently rejects ACPAgent configs
- No error message to users about why ACPAgent is unsupported
If ACPAgent can't work in remote mode, that limitation should be validated and documented, not silently broken.
2. Lost Production Features Without Explanation
[openhands-sdk/openhands/sdk/agent/acp_agent.py]
Removing retry logic, timeout handling, authentication, and session mode configuration suggests the entire integration was fundamentally broken. If so:
- Why was the approach wrong?
- What would a correct implementation look like?
- Were these features themselves broken, or was it something else?
3. Example Cost Reporting Removed
[examples/01_standalone_sdk/40_acp_agent_example.py]
Why remove EXAMPLE_COST reporting? The example testing workflow expects this output. This seems unrelated to remote runtime issues.
4. Evaluation Risk Category
This PR affects:
- Agent behavior (ACPAgent no longer supported remotely)
- Evaluation workflows (agent_type parameter removed)
- Benchmark execution (Docker images no longer include ACP servers)
🚨 Flagging for human maintainer review + lightweight eval before merge
Verdict
❌ Needs context before merging: The revert may be technically correct, but without understanding what broke and why a full revert is needed (vs. targeted fix), it's impossible to assess whether this is the right solution.
Requested Actions
- Add Evidence section to PR description explaining what broke in #2190
- Document why full revert is needed vs. targeted fix
- Add validation/error messages for unsupported ACPAgent remote usage (if that's the issue)
- Consider whether EXAMPLE_COST removal is intentional or accidental
Key Insight: A revert this large without explanation raises more questions than it answers. Either #2190 had fundamental design flaws (document them), or there's a specific bug that could be fixed without throwing away all the work (fix just that bug).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Revert Analysis: This is a well-justified, cleanly executed revert that fixes a real backward-compatibility break.
Evidence Quality: The PR description comprehensively addresses all previous feedback with concrete evidence (OpenAPI diff, local repro, workflow failures, policy citation). The justification for full revert vs targeted patch is sound given how broadly #2190 spread ACP remote-runtime support.
Technical Execution: The revert appears clean - REST/Python API breakage checks pass, tests updated appropriately, no obvious incomplete cleanup.
Eval-Risk Flag: Per repository code review guidelines, I cannot approve this PR because it affects evaluation behavior (removes ACPAgent remote-runtime support, changes eval workflow parameters). A human maintainer should make the final decision on whether to:
- Proceed with this revert (restoring pre-2190 REST contract immediately), or
- Implement a targeted compatibility fix (preserving ACP remote support with deprecated contract support)
The engineering justification is solid - this is a pragmatic fix for a documented policy violation. But the eval impact puts the approval decision outside my scope.
|
smolpaws here 🐾 I poked this from a downstream client and found a real breakage, not just a schema paper-cut. While updating I verified it locally with the same request shape OpenHands-Tab sends today (plain {
"agent": {
"llm": {"usage_id": "agent", "model": "gpt-4o-mini"},
"tools": [
{"name": "terminal"},
{"name": "file_editor"},
{"name": "task_tracker"}
]
},
"workspace": {"kind": "LocalWorkspace", "working_dir": "/tmp"},
"secrets": {},
"confirmation_policy": {"kind": "NeverConfirm"},
"max_iterations": 1
}Results:
I also checked the schema mutation directly:
So, yep: this is a real REST compatibility break introduced by #2190, and OpenHands-Tab hit it in the wild. Tiny cat verdict: this is not just OpenAPI fluff; older external REST clients really do start getting 422s here. 🐾 |
|
@enyst thanks for taking care of this! |
|
@OpenHands This PR was merged, we only take it as starting point for a discussion, don't modify it. Read all comments in this PR, and the linked PR and all its comments. Understand the problem. Look on main branch, at recent commits the REST API workflow checking compatibility failed, look at the CI log for one of them. Then tell me: how many endpoints were affected? List them. Post a comment on this PR so that we know. |
|
I'm on it! enyst can track my progress at all-hands.dev |
|
I dug through all comments on this PR and #2190, checked a recent main failure of the REST API compatibility workflow (run 23147005570 on Count: 4 REST operations were affected. 1.
|
4 participants
HUMAN Note
I fixed the REST API checks to validate the REST API compatibility policy, and they fail on
mainfrom PR 2190, because of the incompatibility explained below. But if the agent is correct that it only breaks hand-written requests, I'm not sure it's an issue significant enough to revert..., WDYT? 🤔cc: @simonrosenberg @xingyaoww
(OpenHands-GPT-5.4)
Why revert
#2190 added useful ACPAgent remote-runtime support, but it also shipped a breaking change to the public agent-server REST contract without the deprecation runway required by
openhands-agent-server/AGENTS.md.That package policy says the
/api/**REST API is public/backward-compatible, and incompatible request/response schema changes need a deprecation notice plus a 5-minor-release runway.What broke
The public REST/OpenAPI contract changed incompatibly
AgentBase-InputandAgentBase-Outputwere simple refs toAgent-*.ACPAgentimport inopenhands-agent-server/openhands/agent_server/api.pyregistered a secondAgentBasesubtype and changed both schemas into a discriminatedoneOfunion (Agent|ACPAgent) keyed bykind.griffe) passed. So the issue here is specifically REST contract compatibility, not the Python API surface.Real server request validation changed for older REST clients
POST /api/conversationsaccepts a plainagentobject withoutkind.openhands.agent_server.apiis imported; once the real server import path runs, the same payload returns422with:Unknown kind '' for openhands.sdk.agent.base.AgentBase; Expected one of: ['ACPAgent', 'Agent']This is not a confirmed persisted-state restore break for normal SDK
Agentconversationsbase_state.jsonproduced on the reverted code for a normal SDKAgentstill loads on Enable ACPAgent on RemoteRuntime API #2190, because SDK serialization already includeskind: "Agent".Agentpayloads.AgentBaserequest/response boundaries for JSON that omittedkind, not normal SDKmodel_dump()serialization.Evidence
openhands-agent-server/AGENTS.mdexplicitly requires deprecation + a 5-minor-release runway for incompatible REST request/response schema changes.AgentBase-Input/AgentBase-Outputare simple refs toAgent-*oneOfunions overAgent-*andACPAgent-*agentpayload withoutkind->201422base_state.jsonon reverted code for a normalAgentWho is affected
OpenHands/OpenHands
I inspected
OpenHands/OpenHandsinopenhands/app_server/app_conversation/live_status_app_conversation_service.py.The app constructs
StartConversationRequest(...)using the shared models and sendsmodel_dump(mode='json', context={'expose_secrets': True}), then parses responses withConversationInfo.model_validate(...).That means the first-party app likely keeps working on its normal path because it already sends explicit
kindand consumes the same typed models.Other REST clients
The public REST API still broke for other clients:
agentshapeWhy full revert instead of a targeted patch
#2190 spread ACP remote-runtime support across:
A full revert cleanly restores the pre-2190 public REST contract immediately. ACP remote support can then be reintroduced additively (for example with a versioned/parallel contract or a deprecation runway).
Intentional scope of this revert
This intentionally removes the ACP remote-runtime/eval rollout from #2190:
The
EXAMPLE_COST/ eval-related example changes were part of that same rollout and are reverted intentionally with it.Path forward
If ACP remote-runtime support is reintroduced, it should come back in a way that preserves the old REST contract during a migration window. Reasonable options include:
agentcontract working and making ACP support additiveIf we keep the reverted behavior for any period, a follow-up may also be worthwhile to explicitly document/validate that ACPAgent remote-runtime configs are unsupported again, rather than depending on implicit pre-2190 behavior.
Eval status of #2190
SWE-bench validation with ACPAgent on 5 instancesunchecked.Validation on this revert PR
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22golang: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:0643a4e-pythonRun
All tags pushed for this build
About Multi-Architecture Support
0643a4e-python) is a multi-arch manifest supporting both amd64 and arm640643a4e-python-amd64) are also available if needed