Support remote agent server backends#1005
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
PR Artifacts Cleaned Up The |
✅ Mock-LLM Docker E2E Test Results12/12 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Co-authored-by: openhands <openhands@all-hands.dev>
✅ Mock-LLM Docker E2E Test Results12/12 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable — The three-way backend split (local / remote / cloud) is the right model, and the isAgentServerBackend() guard applied consistently across 15+ files is a clean abstraction. No critical issues. A few improvement opportunities and a stale name worth cleaning up.
Summary
This PR introduces a remote BackendKind for standalone agent-server URLs, keeping local for the canvas-managed bundled server and cloud for OpenHands Cloud. The central isAgentServerBackend() predicate unifies local and remote for code paths that apply to both, while keeping Cloud-only paths narrow. The approach is sound and applied consistently.
Improvement Opportunities
1. Dead code in recommended-automations-launcher.tsx (lines 118–120)
The component hard-guards on line 217 with if (activeBackend.backend.kind !== "local") return null, so handleSelectAutomation is never attached to any DOM element for remote backends. The kind === "remote" ? "local" : kind ternary passed to buildAutomationPrompt is therefore unreachable. It's harmless, but it creates a confusing "what if?" reading of the code.
Two clean options:
- Remove the guard-mapped ternary and just pass
activeBackend.backend.kind as "local" | "cloud"— TypeScript is already happy since the early-return guard narrows kind to"local". - If the intent is to eventually support remote backends here, add a
// TODO:comment explaining when this branch would be live.
2. Stale function name pickLocalBackend (active-store.ts, line 31)
The function now finds the first agent-server backend (local or remote). The name pickLocalBackend implies it only returns local backends, which will mislead future readers. pickAgentServerBackend or pickFallbackBackend would be accurate.
3. CLOUD_BACKEND_HOSTS is a static allowlist (backend-form-modal.tsx, line 38)
The set-based check is a real improvement over the prior substring match — it avoids false positives on tunneled/hosted agent-server URLs like work-2-xxx.prod-runtime.all-hands.dev. The trade-off is that any new canonical Cloud hostname (e.g. a future eu.app.all-hands.dev) requires a code change here. Worth a short comment to make the maintenance contract explicit.
4. Stale JSDoc in auth.ts
The line "Local agent-server uses X-Session-API-Key" is now incomplete — remote agent-server backends also use X-Session-API-Key (the fallback case on line 28). Suggest updating to:
Local and remote agent-servers use X-Session-API-Key. Cloud expects a bearer token in the Authorization header.
5. i18n/translation.json — all non-English locales stub to "Remote"
Acceptable placeholder, but worth opening a follow-up issue so the untranslated string doesn't get lost.
6. Unrelated fix bundled in this branch (dropdown.tsx)
The useEffect cleanup for closeTimerRef is a good fix (prevents a timer leak on unmount) but is unrelated to the remote backend feature. Not a blocker, but worth a note in the commit message so it doesn't get lost in future bisects.
Testing
The new test "treats hosted non-cloud agent server URLs as remote" directly exercises inferKindFromHost through the UI, which is the most important new logic in this PR. ✅
The existing tests updated kind: "local" → kind: "remote" correctly reflect the new semantic. ✅
Not tested (non-blocking given the browser QA in the PR description):
isAgentServerBackend()directly — trivial, but a unit test would document the contract- The
use-automation-healthshort-circuit for theremotekind getEffectiveLocalBackendwhen the active backend isremote
Risk Assessment
- Overall PR
⚠️ Risk: 🟢 LOW- Frontend-only change. No existing
localorcloudpaths are altered — the newremotekind is additive. TypeScript enforces exhaustiveness at call sites. TheisAgentServerBackendpredicate is simple and correctly replaces all directkind === "local"checks.
- Frontend-only change. No existing
VERDICT: ✅ Worth merging. The design is correct and the implementation is consistent. The items above are all non-blocking cleanups; they can go in a follow-up.
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.
Was this review helpful? React with 👍 or 👎.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed review cleanup in b0c30c4: removed the unreachable remote automation prompt mapping, renamed the fallback picker, documented the Cloud host allowlist, and updated agent-server auth docs. CI is re-running on the new SHA.\n\n_This comment was created by an AI agent (OpenHands) on behalf of the user._ |
✅ Mock-LLM Docker E2E Test Results12/12 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review — Support remote agent server backends
The overall design is clean and well-reasoned. The isAgentServerBackend() helper avoids scattering kind === "local" || kind === "remote" checks across the codebase, and the CLOUD_BACKEND_HOSTS allowlist is a more robust approach than the previous substring match. The local/remote/cloud taxonomy is clearly motivated by the PR description and the QA demos back it up.
Three items worth addressing before merge:
- Missing test for the core new path in
use-automation-health. The remote-backend short-circuit (returning a synthetic error without callingcheckHealth) is the most novel behaviour in this PR and has no unit test. - Stale variable name
localsinbackend-selector.tsxnow silently includes remote backends. - Implicit mapping of
remote → localin the marketplace utility could use a one-line comment so future readers don't have to reverse-engineer the intent.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed the changes requested in 966a319: added remote automation-health coverage, renamed the backend selector collection to agentServers, and documented the remote marketplace availability mapping. Re-requesting review now.\n\n_This comment was created by an AI agent (OpenHands) on behalf of the user._ |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review — Support remote agent server backends
All three items from the previous CHANGES_REQUESTED review have been resolved:
- ✅
use-automation-health.test.tsxnow covers the remote backend short-circuit (returns a synthetic error without callingcheckHealth) - ✅
localsrenamed toagentServersinbackend-selector.tsx - ✅ The
remote → localavailability mapping inmcp-marketplace-utils.tsis now commented inline
All six first-pass suggestions from the initial review were also addressed in b0c30c4 (dead code removed, stale pickLocalBackend renamed, CLOUD_BACKEND_HOSTS comment added, JSDoc updated). The overall design is clean — the isAgentServerBackend() predicate applied consistently across 15+ files is far better than scattering kind === "local" || kind === "remote" checks, and the tighter CLOUD_BACKEND_HOSTS allowlist correctly treats tunnelled runtime URLs as remote agent servers rather than Cloud.
Remaining minor items
1. isAgentServerBackend should be a TypeScript type predicate
The function is declared to return boolean but callers cannot benefit from type narrowing. Typing it as a predicate would let TypeScript narrow backend.kind automatically at every call site:
export function isAgentServerBackend(
backend: Pick<Backend, "kind">,
): backend is Pick<Backend, "kind"> & { kind: "local" | "remote" } {
return backend.kind === "local" || backend.kind === "remote";
}2. isLocalBackend / isLocal variable names are now misleading at 7 call sites
After this change, const isLocalBackend = isAgentServerBackend(backend) is true for both local and remote backends. The name implies a local-only check and will mislead future maintainers. Consider renaming the local variables to isAgentServer at each call site, or adding a brief comment.
Affected files: git-control-bar.tsx, home-chat-launcher.tsx, use-model-interceptor.ts, setup-llm-step.tsx, use-has-git-commits.ts, use-local-git-info.ts, use-workspace-session.ts.
3. BACKEND$KIND_REMOTE i18n is stubbed as "Remote" across all 14 non-English locales
Worth a follow-up issue before shipping so translators can address it.
4. dropdown.tsx timer-cleanup fix is still bundled in this branch
Good fix, but unrelated to remote agent-server support. Harmless here; just worth noting for the record.
None of the above are blockers. The implementation is correct, tests cover the new code paths, and browser QA was performed on both the local and remote flows.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
❌ Mock-LLM Docker E2E Test Results11/12 passed · 1 failed Commit:
🔍 Failure details (1)❌ mock-llm-conversation.spec.ts › mock-LLM agent-server conversation › step 4: resume conversation from sidebar after navigating awayPosted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
✅ Mock-LLM Docker E2E Test Results12/12 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed the latest suggestions in c5b5932: This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review — Support remote agent server backends
This is a clean, well-executed feature. The three-way split (local / remote / cloud) is the right model, and the isAgentServerBackend() type predicate now provides proper TypeScript narrowing at every call site. All three rounds of prior feedback have been addressed.
What's good:
isAgentServerBackend()as a type predicate cleanly centralises thelocal || remotecheck across 15+ files without scatteringkind === "local" || kind === "remote"conditionals.CLOUD_BACKEND_HOSTSallowlist with its maintenance comment correctly prevents false positives forwork-*tunnel URLs that share theall-hands.devdomain.useAutomationHealthreturns a clear synthetic error for remote backends without hitting the sidecar, and the new test case covers that path.- The
inferKindFromHostfallback to"remote"on parse errors is the right defensive default. - The
Dropdowntimer cleanup on unmount is a good hygiene fix.
One minor gap (non-blocking):
The new BACKEND$KIND_REMOTE i18n key ships with all non-English locales set to the English literal "Remote". Compare with BACKEND$KIND_LOCAL which has proper translations ("ローカル", "本地", "원격", etc.). "Remote" is recognisable across most locales, so this won't break anything, but it creates inconsistency with the other backend-kind labels. Worth filing a follow-up to add translations.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
❌ Mock-LLM Docker E2E Test Results11/12 passed · 1 failed Commit:
🔍 Failure details (1)❌ mock-llm-conversation.spec.ts › mock-LLM agent-server conversation › step 4: resume conversation from sidebar after navigating awayPosted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
📸 Snapshot Test Report✅ 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. |
Summary
remotebackend kind for standalone agent servers while preservinglocalfor the canvas-managed agent server andcloudfor OpenHands Cloud.QA
npm run typechecknpx vitest run __tests__/components/backends/add-backend-modal.test.tsx --reporter=verbosenpm test(initial full-suite run hit two expected port-conflict failures because the QA mock LLM was occupying port 9999)npx vitest run __tests__/scripts/dev-safe.test.ts __tests__/scripts/ingress.test.tsafter freeing port 9999npm run buildnpm run devcanvas-managed agent server, mock LLM profile, conversation created, agent response received.Demos
Local canvas-managed agent server
Remote standalone agent server
This PR was created by an AI agent (OpenHands) on behalf of the user.
🐳 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.24.0-pythonopenhands-automation==1.0.0a5c5b59324ad659d186f887af220240339c87ea761Pull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-c5b5932Run
All tags pushed for this build
About Multi-Architecture Support
sha-c5b5932) is a multi-arch manifest supporting both amd64 and arm64sha-c5b5932-amd64) are also available if needed