Repository navigation
chore: remove the cloud_proxy router - #3326
Conversation
github-actions
Bot
added
the
release-note-required
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ❌ FAILEDResult: ❌ FAILED
Log excerpt (first 1000 characters) |
Coverage Report •
|
||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable, but I found one compatibility issue: removing /api/cloud-proxy appears to bypass the agent-server REST deprecation runway. Risk: 🟡 MEDIUM because this intentionally turns a public /api/** endpoint into a 404 for external clients.
This review was generated by an AI agent (OpenHands) on behalf of the requester.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/software-agent-sdk/actions/runs/26167755287
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
The PR achieves its goal: the live agent-server no longer serves /api/cloud-proxy, while representative existing /api/* routes still respond on the PR build.
Does this PR achieve its stated goal?
Yes. I started the actual agent-server on both origin/main and PR commit 91812a76dd9934f2b6bd8a25f67ef810302546a1 and made HTTP requests against the running servers. On base, /api/cloud-proxy was present in OpenAPI and handled requests with the old router behavior; on the PR, the same request returns 404 Not Found and OpenAPI has no cloud/proxy route, while settings/profiles/workspaces/file/LLM/git/hooks/MCP route surfaces still respond.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully with uv sync --dev and installed the agent-server package. |
| CI Status | ⏳ GitHub reported 19 successful, 4 skipped, 9 pending, 0 failing checks at review time. |
| Functional Verification | ✅ Started the real server before/after and exercised affected + representative existing API endpoints via HTTP. |
Functional Verification
Test 1: /api/cloud-proxy is removed from the running server
Step 1 — Establish baseline without the PR:
Checked out origin/main, started the actual server with:
uv run --no-sync agent-server --host 127.0.0.1 --port 8766Then queried the live OpenAPI document and posted to the old route with a disallowed host to avoid any upstream call:
curl -i -X POST http://127.0.0.1:8766/api/cloud-proxy -H 'content-type: application/json' --data '{"host":"https://evil.example.com","method":"GET","path":"/whatever"}'Observed:
cloud_proxy_paths= ['/api/cloud-proxy']
HTTP/1.1 403 Forbidden
{"detail":"Cloud proxy host not allowed: https://evil.example.com"}
This confirms the old server registered the cloud-proxy route and the request was handled by that router.
Step 2 — Apply the PR's changes:
Checked out PR commit 91812a76dd9934f2b6bd8a25f67ef810302546a1 and started the actual server with:
uv run --no-sync agent-server --host 127.0.0.1 --port 8767Step 3 — Re-run with the PR in place:
Queried OpenAPI and repeated the same POST:
curl -i -X POST http://127.0.0.1:8767/api/cloud-proxy -H 'content-type: application/json' --data '{"host":"https://evil.example.com","method":"GET","path":"/whatever"}'Observed:
cloud_proxy_paths=[]
HTTP/1.1 404 Not Found
{"detail":"Not Found"}
This confirms the route is no longer registered or served by the PR build.
Test 2: Existing /api/* surfaces still respond on the PR server
Step 1 — Establish baseline without the PR:
On the base server, representative existing routes responded as expected:
GET /api/settings -> HTTP/1.1 200 OK
GET /api/conversations -> HTTP/1.1 422 Unprocessable Content
{"detail":[{"type":"missing","loc":["query","ids"],"msg":"Field required","input":null}]}
The 422 for conversations is useful here: it shows the route exists and is performing request validation.
Step 2 — Apply the PR's changes:
Used the same PR server from Test 1.
Step 3 — Re-run with the PR in place:
Queried representative API endpoints and route categories on the live PR server:
OpenAPI route summary on PR:
cloud_proxy_paths=[]
settings: count=5 first=/api/settings
conversations: count=26 first=/api/acp/conversations
file: count=11 first=/api/conversations/{conversation_id}/switch_profile
bash: count=6 first=/api/bash/bash_events
git: count=2 first=/api/git/changes
hooks: count=1 first=/api/hooks
llm: count=4 first=/api/conversations/{conversation_id}/switch_llm
mcp: count=1 first=/api/mcp/test
profiles: count=4 first=/api/profiles
auth: count=1 first=/api/auth/workspace-session
workspaces: count=2 first=/api/workspaces
Live PR endpoint statuses:
/api/settings -> HTTP 200, 2013 bytes
/api/profiles -> HTTP 200, 37 bytes
/api/workspaces -> HTTP 200, 39 bytes
/api/file/home -> HTTP 200, 184 bytes
/api/llm/models -> HTTP 200, 93910 bytes
/api/git/changes -> HTTP 422, 90 bytes
/api/hooks -> HTTP 405, 31 bytes
/api/mcp/test -> HTTP 405, 31 bytes
The 200 responses confirm several unaffected routes still serve data. The 422/405 responses are expected validation/method responses from registered routes, not router-level 404s, so the related API surfaces remain mounted.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
Thank you! I don't think the agent's problems are truly relevant, so let's do this.
|
@OpenHands the Python griffe check added the label |
|
I'm on it! enyst can track my progress at all-hands.dev |
|
@enyst I looked into the false positive and opened a follow-up PR from I considered a few options:
I chose option 1. It keeps the existing PyPI-baseline compatibility signal for the breakage check, but makes the label/comment attribution PR-specific so unrelated follow-up PRs do not get re-labeled for already-merged unreleased defaults. PR: #3329 This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
Final summary of new work since my last update:
What the new PR changes:
Validation completed:
Conciseness check:
|
|
[Automatic Post]: It has been a while since there was any activity on this PR. @hieptl, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. This comment was created by an AI agent (OpenHands) on behalf of the user. |
2 similar comments
|
[Automatic Post]: It has been a while since there was any activity on this PR. @hieptl, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
[Automatic Post]: It has been a while since there was any activity on this PR. @hieptl, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. This comment was created by an AI agent (OpenHands) on behalf of the user. |
github-actions
Bot
removed
the
release-note-required
enyst
added
the
release-note-required
OpenHands AI
Why
Description
The
cloud_proxyrouter exists for one reason: tunnel browser calls from the bundled GUI toapp.all-hands.devserver-side, because the SaaS blocked CORS fromlocalhost. WithApiKeyAwareCORSMiddlewaredeployed on the SaaS and the GUI now calling cloud directly (see theOpenHandsandagent-server-guiPRs), this router has no consumer. Delete it along with its SSRF-protection plumbing andOH_CLOUD_PROXY_ALLOWED_HOSTSenv var.Acceptance criteria
openhands-agent-server/openhands/agent_server/cloud_proxy_router.pyis removed.api.pyno longer importscloud_proxy_routeror registers it onapi_router.tests/agent_server/test_cloud_proxy_router.pyis removed.cloud_proxy,cloud-proxy,OH_CLOUD_PROXY_ALLOWED_HOSTSremain in the SDK tree./api/*for everything else (conversations, files, bash, git, hooks, llm, mcp, settings, profiles, auth) — nothing else is touched.Risk
Any out-of-tree client still POSTing to
/api/cloud-proxyon a bundled agent-server will now get a 404. The only known consumer is the agent-server-gui, which we update in the same release.Summary
openhands-agent-server/openhands/agent_server/cloud_proxy_router.py(router, SSRF allowlist,OH_CLOUD_PROXY_ALLOWED_HOSTSparsing, response-header stripping).tests/agent_server/test_cloud_proxy_router.py.openhands-agent-server/openhands/agent_server/api.py: drop thefrom openhands.agent_server.cloud_proxy_router import cloud_proxy_routerimport and theapi_router.include_router(cloud_proxy_router)line. Trim the stale comment in the 5xx logging block that used/api/cloud-proxyas an example.tests/agent_server/test_api.py: trim the matching stale comment in theTestHttpExceptionLoggingdocstring.Why
cloud_proxy_routerwas a CORS workaround. The SaaS now exposes permissive CORS for API-key authenticated requests (ApiKeyAwareCORSMiddlewarein theOpenHandsenterprise tree), and the bundled GUI was refactored inagent-server-guito call the cloud host directly viaaxios.request. Keeping the router around would just be dead code plus a foot-gun: the SSRF allowlist would lull future readers into thinking it's still load-bearing.Issue Number
Resolves #3325
How to Test
uv run --no-sync pytest tests/agent_server/test_api.py→ 41 tests passing.rg -n "cloud[_-]proxy|OH_CLOUD_PROXY_ALLOWED_HOSTS" software-agent-sdk→ no matches.POST /api/cloud-proxy→ 404 (expected); hit any other/api/*route → unchanged behavior.Type
Notes
Migration / rollback
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-nodejs22-slimgolang: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:b1ae2f8-pythonRun
All tags pushed for this build
About Multi-Architecture Support
b1ae2f8-python) is a multi-arch manifest supporting both amd64 and arm64b1ae2f8-python-amd64) are also available if needed