refactor(frontend): call OpenHands Cloud directly, drop cloud-proxy hop#661
refactor(frontend): call OpenHands Cloud directly, drop cloud-proxy hop#661hieptl wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: Drop cloud-proxy hop, call SaaS directly
🔴 Needs improvement - Architecture is sound but critical test coverage was deleted instead of updated.
[CRITICAL ISSUES]
Test Coverage Loss:
-
[tests/api/device-flow-client.test.ts] Deleted 487 lines of comprehensive tests including ALL error handling, security validation (
isOpenHandsCloudHostdomain checks), RFC 8628 compliance (polling,slow_down,authorization_pending), DoS protection, abort signals, timeout handling, and network retry logic. Implementation has 310 lines of complex stateful logic but only 2 trivial happy-path tests remain. The removed tests covered:expired_token,access_denied, polling interval adjustment, 30s cap, type confusion protection, and network resilience. This is unacceptable for production code - restore and update the test suite to match the new direct-call pattern. -
[tests/api/cloud/organization-me.test.ts] Completely removed (57 lines) but
getCloudOrganizationMe()still exists and is actively used insrc/hooks/query/use-cloud-current-user-id.ts. Function now has zero test coverage. Should have been updated to useaxios.requestinstead of deleted. -
[tests/api/cloud/secrets-service.test.ts] Completely removed (126 lines) but all cloud secrets methods (
fetchCloudSecrets,createCloudSecret,updateCloudSecret,deleteCloudSecret) still exist and are heavily used by 4 hooks +SecretsService+agent-server-adapter. Lost comprehensive coverage of pagination, CRUD operations, URL encoding, and error handling. Zero test coverage for production code. Should have been updated, not deleted.
Auth Validation:
- [src/api/cloud/proxy.ts, line 54] When
authMode === "session-api-key"andsessionApiKeyis null/undefined/empty,buildUpstreamAuthHeaders()returns an empty object, allowing unauthenticated requests to proceed (which will 401 upstream). Should fail fast:
function buildUpstreamAuthHeaders(req: CloudProxyRequest): Record<string, string> {
const mode = req.authMode ?? "bearer";
if (mode === "bearer") return { Authorization: `Bearer ${req.backend.apiKey}` };
if (mode === "session-api-key") {
if (!req.sessionApiKey) throw new Error("sessionApiKey required when authMode is session-api-key");
return { "X-Session-API-Key": req.sessionApiKey };
}
return {};
}[IMPROVEMENT OPPORTUNITIES]
-
[src/api/device-flow-client.ts, line 93]
body?: unknownparameter accepts any type, but logic only handles string/object correctly. If a caller passes a Buffer or other BodyInit,JSON.stringify()will produce incorrect output. Constrain tobody?: string | objector add explicit type guards. -
[src/api/cloud/proxy.ts, line 92] Using
data: req.body ?? nullconverts undefined to null. Axios treats these differently - undefined omits the body, null may serialize as "null". For DELETE requests this could matter. Considerdata: req.bodyto follow axios conventions. -
[tests/api/cloud/proxy.test.ts] Missing tests for
authMode: 'none',timeoutSeconds,responseType: 'blob', custom headers merging, request body, and error handling. Tests cover the 4 main scenarios but leave edge cases untested.
[TESTING GAPS]
Device Flow Tests: No tests for startDeviceFlow error paths (HTTP errors, missing field validation, network errors) or pollForToken actual polling behavior (authorization_pending handling, slow_down logic, abort signal, timeout). Tests only cover immediate success.
Cloud API Tests: organization-me.test.ts and secrets-service.test.ts should be restored and updated to the new pattern, not left deleted.
[RISK ASSESSMENT]
This PR touches a critical integration point (browser → cloud API) and refactors 35 files with a net -680 lines. While the architecture is sound and the public API is preserved, the deletion of 670+ lines of test coverage (instead of updating it) leaves core functionality unverified:
- Device flow OAuth has 2% of its original test coverage
- Cloud org/secrets APIs have zero test coverage
- Error paths, security validation, and edge cases are untested
Recommendation: Do not merge until test coverage is restored. The implementation looks correct, but without comprehensive tests we cannot verify:
- OAuth flow handles all RFC 8628 states correctly
- Cloud secrets/org APIs behave identically to the proxy-based version
- Error handling works for network failures, auth errors, and malformed responses
VERDICT
❌ Needs rework: Restore deleted test files and update them to match the new direct-call pattern. The architectural change is solid, but production code cannot ship without its test coverage.
KEY INSIGHT
The direct-call refactor eliminates ~120 lines of envelope plumbing and simplifies error handling, but test coverage must be updated, not deleted. All 12 updated test files show the correct transformation pattern (axios.post → axios.request, envelope assertions → direct URL assertions) - apply the same approach to the 3 deleted test files.
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 (e.g., "Security concerns about X do not apply here because Y"). 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's merge-ready.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/agent-canvas/actions/runs/26167363814
Code Review Reply — agent-canvas PR #661
The bot left a single PR-level review with three buckets — CRITICAL 1 — Deleted device-flow tests must be restoredReviewer's claim:
Decision: Valid — applied.
Changes
The two original happy-path tests were retained; I consolidated the older mock styles (
CRITICAL 2 —
|
| # | Bucket | Feedback | Decision | Files touched |
|---|---|---|---|---|
| 1 | CRITICAL | Restore device-flow tests | Applied (14 new tests) | __tests__/api/device-flow-client.test.ts |
| 2 | CRITICAL | Restore getCloudOrganizationMe test |
Applied (extended existing file) | __tests__/api/cloud/organization-service.test.ts |
| 3 | CRITICAL | Restore cloud secrets CRUD tests | Applied (new file, 4 tests) | __tests__/api/cloud/secrets-service.test.ts |
| 4 | CRITICAL | Fail fast on missing sessionApiKey |
Applied (throw + regression test) | src/api/cloud/proxy.ts, __tests__/api/cloud/proxy.test.ts |
| 5 | IMPROVEMENT | Tighten body?: unknown in device-flow helper |
Rejected (file-private, callers controlled) | — |
| 6 | IMPROVEMENT | Drop ?? null on axios data |
Applied | src/api/cloud/proxy.ts |
| 7 | IMPROVEMENT | Proxy edge case tests | Partially applied (authMode: 'none', responseType: 'blob') |
__tests__/api/cloud/proxy.test.ts |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: Drop cloud-proxy hop, call SaaS directly
🟡 Acceptable - Architecture simplification is sound, but deployment risk requires human oversight.
[IMPROVEMENT OPPORTUNITIES]
[src/api/device-flow-client.ts] 🟡 Test coverage validation: While the new tests are comprehensive and cover RFC 8628 compliance well (including authorization_pending, slow_down, DoS protection, abort signals, and timeout handling), the PR reduced test file size from 487 to 328 lines. Verify that no critical edge cases were lost in the refactor. The coverage looks good in my review, but a final manual check against RFC 8628 section 3.5 would be prudent.
[PR Description, Manual Testing section] 🟡 Evidence: The "How to Test" section describes manual steps but doesn't include concrete runtime artifacts. For a change of this scope (touching 26+ cloud service callers), consider adding:
- Network devtools screenshot showing direct
https://staging.all-hands.dev/api/...requests withAuthorization: BearerandX-Org-Idheaders - Example response showing upstream status codes are now surfaced directly (not 502-wrapped)
- Evidence that the device-flow path (
/oauth/device/authorize,/oauth/device/token) works end-to-end
This isn't blocking given the test coverage, but would strengthen confidence for reviewers.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🔴 HIGH
Key Risk Factors:
- Hard cutover with no fallback: If the SaaS CORS middleware (
ApiKeyAwareCORSMiddleware) isn't deployed or has issues, every cloud API call breaks. There's no gradual rollout or feature flag. - Broad scope: Changes affect 26+ cloud service methods across 35 files, including critical paths like conversation creation, organization switching, settings, secrets, and device-flow authentication.
- Security model change: Auth tokens now travel client-side (browser → cloud) instead of server-side (browser → local agent-server → cloud). While the PR description explains this is safe (
allow_credentials=Falseon SaaS CORS), it's a meaningful shift in the trust boundary. - Deployment sequencing: The SaaS must deploy CORS changes before this PR merges, or the GUI becomes unusable for cloud users. The PR description mentions "separate OpenHands PR" but doesn't confirm deployment status.
- Error surface change: Upstream status codes now appear directly in the browser (e.g., 401 from expired keys, 403 from org mismatches). While cleaner, any internal docs or runbooks referencing "502 from cloud-proxy" will need updates.
Why Not Lower Risk:
- This isn't a gradual migration (both paths don't coexist)
- Test coverage is strong but can't catch deployment-sequencing failures
- Manual testing was against staging, not production
Recommendation: Do not auto-merge. Request review from a human maintainer who can:
- Confirm the SaaS CORS middleware is deployed to production
- Validate the deployment sequence (SaaS first, then this PR)
- Approve a rollback plan (reverting this PR if CORS issues arise post-merge)
- Monitor for CORS-related errors in the first 24 hours post-deployment
If you have confidence the SaaS changes are live and tested, this refactor is well-executed and ready to land.
VERDICT:
✅ Worth merging - Code quality is high, tests are solid, X-Org-Id logic is preserved, and the architecture is simpler. The risk is purely deployment-sequencing and can be mitigated with proper coordination.
KEY INSIGHT:
This PR removes ~120 lines of envelope plumbing and surfaces cleaner error codes, but it's a one-way door: if the SaaS CORS isn't ready, the only fix is a full revert. The engineering is sound; the risk is operational.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/agent-canvas/actions/runs/26171568363
📸 Snapshot Test ReportWarning Snapshot comparison step crashed (timeout, OOM, or runner error) — diff results below may be incomplete or absent. ✅ All snapshots match the main branch baselines.
✅ Unchanged snapshots (73)
Generated by the Snapshot Tests workflow. This comment was created by an AI agent (OpenHands) on behalf of the repo maintainers. |
3 participants
Why
Description
The GUI currently funnels every cloud request through
POST /api/cloud-proxyon the bundled local agent-server, which then forwards it toapp.all-hands.devserver-side. The hop exists because the SaaS used to block CORS fromlocalhost. With the newApiKeyAwareCORSMiddlewareon the SaaS, browsers can call the cloud directly when authenticating by API key.Drop the hop:
callCloudProxy()keeps its public signature (so the 26 cloud-service callers don't change) but now issuesaxios.requestagainstupstreamHost + path, with bearer or session-api-key headers attached client-side.startDeviceFlow,pollForToken) calls/oauth/device/*on the cloud directly.This removes ~120 lines of envelope plumbing, surfaces cleaner upstream error codes (no more 502-from-the-local-proxy aliasing), and lets us delete the entire
cloud_proxy_routerfromsoftware-agent-sdk.Acceptance criteria
callCloudProxy()issues exactly oneaxios.requestagainst the upstream host; no local-server hop.X-Org-Idis sent only when targeting the active backend and anorgIdis selected (preserves the fan-out invariant so per-backend bookkeeping calls don't 403 on org/key mismatch).authMode: "session-api-key"plushostOverridekeeps working for runtime-sandbox calls.startDeviceFlowandpollForTokenissue directfetchto${host}/oauth/device/{authorize,token}.axios.requestshape; no test asserts on/api/cloud-proxyanymore.tests/e2e/regressions/event-pagination.spec.tscloud branch waits on the direct cloud URL.Out of scope
OpenHandsPR).software-agent-sdk(separate PR).Summary
src/api/cloud/proxy.ts—callCloudProxy()now issuesaxios.request({ method, url, data, headers, timeout, responseType })againstupstreamHost + path. DropsgetEffectiveLocalBackend(),getAgentServerHeaders(), and the local auth header plumbing. Public signature unchanged so all 26 callers insrc/api/cloud/*.api.tsandsrc/api/event-service/event-service.api.tskeep working as-is.src/api/device-flow-client.ts—makeProxiedRequestreplaced withmakeDeviceFlowRequest, which does a directfetchto${host}/oauth/device/{authorize,token}. The SaaS treats those endpoints as credential-less and permits them cross-origin.src/mocks/auth-handlers.ts,src/mocks/conversation-handlers.ts) updated to intercept the direct cloud URLs rather than/api/cloud-proxy.src/api/cloud/*.api.ts,src/api/event-service/event-service.api.ts,src/hooks/query/use-backends-health.ts,src/api/conversation-service/agent-server-conversation-service.api.ts,AGENTS.md, and.agents/skills/custom-codereview-guide.md.__tests__/api/cloud/proxy.test.ts(direct URL + bearer + X-Org-Id; X-Org-Id omitted on non-active backend; session-api-key + hostOverride for runtime sandbox) and new__tests__/api/device-flow-client.test.ts(direct cloud URLs + form-encoded device_token body). All 12 existing__tests__/api/cloud/*.test.tsfiles plus__tests__/api/agent-server-conversation-service.test.tsupdated to mockaxios.requestand assert on the direct cloud URL.tests/e2e/regressions/event-pagination.spec.tscloud branch waits on the direct GET instead of the proxy POST.Why this is safe
callCloudProxy()is unchanged. Callers pass the same{ backend, method, path, headers, body, authMode, sessionApiKey, hostOverride, timeoutSeconds, responseType }and get the same return value.X-Org-Idrule is preserved exactly: only attached whengetActiveBackend().backend.id === req.backend.id && active.orgId. This still protects fan-out calls (e.g.useAllCloudOrganizations) from sending the active backend's orgId across an unrelated API key.allow_credentials=False, so the browser cannot attach cookies — the only way the upstream sees a request is with the explicit API key the user pasted. Same security model ascurl.Issue Number
Resolves #660
How to Test
npx vitest run __tests__/api→ 279 tests passing (43 files), including the newproxy.test.tsanddevice-flow-client.test.ts.npx tsc --noEmit→ no errors introduced by this PR (the pre-existingI18nKeyerrors onagent-settingsetc. are out of scope).export PROJECTS_PATH=~/Documents/openhands && npm run dev:docker:dynamichttps://staging.all-hands.dev/api/...directly, withAuthorization: Bearer …andX-Org-Id. No requests to/api/cloud-proxy./oauth/device/authorizeand/oauth/device/tokenhit the cloud host directly.Type
Notes
Risk / rollback
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.23.0-pythonopenhands-automation==1.0.0a3ef3565fe8f0b6e274426782f674d17a2acf88deaPull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-ef3565fRun
All tags pushed for this build
About Multi-Architecture Support
sha-ef3565f) is a multi-arch manifest supporting both amd64 and arm64sha-ef3565f-amd64) are also available if needed