Repository navigation
Conversation
…ackend bridge The /app-backends WebSocket bridge accepted the client before connecting upstream and never forwarded the Sec-WebSocket-Protocol offer, so the 101 selected no subprotocol and browsers clients that require one failed the handshake. The bridge now connects to the backend first with the client's offer and accepts the client with the subprotocol the backend selected. Upstream failures still accept and close with 1011. The Docker-runtime socket routes keep their accept-before-bridge behaviour. Drops the known-bug marker on F29.ws-subprotocol in the verify-agent-server map. Fixes #5648 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
1 of 5 tasks
Contributor
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Contributor
Coverage Report •
|
||||||||||||||||||||||||||||||
enyst
marked this pull request as ready for review
October 13, 2026 21:36
all-hands-bot
approved these changes
Oct 13, 2026
all-hands-bot
left a comment
all-hands-bot
left a comment
Collaborator
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope
In-scope. The change is in the Agent Server's own WebSocket bridge (openhands-agent-server/.../docker_runtime/proxy.py and canvas_extensions/bridge.py), which this repository owns per root AGENTS.md. The one design choice the PR raises — what the client sees when the upstream backend refuses the socket — is resolved in-repo: keep existing behaviour (accept with no subprotocol, close 1011), which is documented in the code docstring and the F29.ws-subprotocol/F29.ws-bridge map bullets. That does not require a cross-repository product decision.
What I verified on head 5fe7262
bridge_websocketgains an opt-inaccept_clientflag. With it set, the client's offered subprotocols are forwarded viawebsockets.connect(subprotocols=...), the upstream connection is established first, and the client is accepted withupstream_ws.subprotocol—Nonewhen the backend selects none. This is the fix the linked issue #5648 root-caused.- The docker-runtime callers (
docker_runtime/routers.py) do not set the flag, so their accept-during-auth path is unchanged;subprotocols=Noneand the guardedif accept_client:keep that path equivalent. proxy_app_backend_websocketno longer accepts early and passesaccept_client=Trueinstead.- The new
_close_client_after_upstream_failurepreserves the documented upstream-failure behaviour: it accepts a still-CONNECTINGclient and closes 1011, so the pre-change observable behaviour (open, then close 1011) is retained even though accept now happens later. Existing callers with an already-accepted socket hit the simpleclose(1011)branch. - Linked issue #5648 is OPEN with
bugandready-for-dev; its four acceptance criteria (bridge forwards the offer; selects none when the backend does; real-server test for both;F29.ws-subprotocolmarker removed and passing) are all met by this head. The map bullet drops theknown buglabel and# bugmarker and its prose matches the implemented behaviour.
Test and CI evidence
- Ran
uv run pytest tests/agent_server/canvas_extensions/test_canvas_extension_bridge.py→ 13 passed, including the four newtest_live_websocket_negotiates_backend_subprotocolparametrizations andtest_live_websocket_upstream_refusal_closes_with_1011. - Ran
uv run pytest tests/agent_server/docker_runtime→ 110 passed. - Current-head GitHub Actions: 54 check runs, 0 non-success, 0 non-completed (including
agent-server-tests,sdk-tests,cross-tests,windows-tests,endpoint-audit,coverage-report). - The new live tests use real uvicorn/
websocketsservers rather than mocks, and the PR supplies before/after map evidence (mainxfail, headxpass,map checkok), which is adequate live evidence for this bug.
No material correctness, security, compatibility, or acceptance-criterion defect found on the current head.
✅ APPROVED
…ubprotocol Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
|
📁 PR Artifacts Notice This PR contains a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
HUMAN:
This PR proposes a fix for a bug uncovered during the first full agentic run of
verify-agent-server.AGENT:
Why
The app-backend bridge accepted the browser’s WebSocket before negotiating upstream, so the selected subprotocol was lost and browsers could reject the connection.
Summary
Let the app bridge defer client acceptance until upstream negotiation succeeds. The shared bridge keeps its existing default for authenticated Docker callers; the app route opts into deferred acceptance.
Source: bridge.py · proxy.py.
Issue Number
Fixes #5648
How to Test
Previously recorded verification
tests/agent_server/canvas_extensions/test_canvas_extension_bridge.pyuse real uvicorn andwebsocketsservers.test_live_websocket_negotiates_backend_subprotocolcovers four cases: the backend selects the offeredqa.v1; it picksqa.v1from several offers; the offer is one the backend does not select, so the result is none; there is no offer.test_live_websocket_upstream_refusal_closes_with_1011covers an upstream refusal. On main, 3 of the 5 fail; on this branch all pass.uv run pytest tests/agent_server/canvas_extensions/test_canvas_extension_bridge.py tests/agent_server/docker_runtime: 123 passed.control-agent-server map run --file F29 --only F29.ws-subprotocol --fresh --repeat 2:--checkout, dfd8fe8):xfailboth times.xpassboth times.map checkis ok and the bullet ispass.pass:F29.ws-bridge(root route still closes with 1011),F29.ws-handshake-rejections,F29.session-revoke,F29.backend-lifecycle-revokesandF29.uninstall-revokes.uv run pre-commit run --files <changed files>: all checks pass.Video/Screenshots
n/a
Type
Notes
Failure deliberately retains accept-then-close-1011 behavior rather than refusing the handshake with 403. There is no new reconnect behavior or TypeScript WebSocket client. Previously recorded verification includes an unrelated process-recovery test failure reproduced on main.
Visual explanation by Codex using HumanLayer show-me.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang: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:9cae98a-pythonRun
All tags pushed for this build
About Multi-Architecture Support
9cae98a-python) is a multi-arch manifest supporting both amd64 and arm649cae98a-python-amd64) are also available if needed