Add lock-to-cloud backend setup mode#1389
Open
neubig wants to merge 47 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
| Status | Test | Duration |
|---|
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses)
Contributor
🛑 Mock-LLM E2E Tests6/6 passed · Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
|
| Status | Test | Duration |
|---|
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses)
Contributor
🛑 Mock-LLM E2E Tests8/13 passed · 1 failed · 4 skipped · Commit:
🔍 Failure details (1)❌ chromium › backends/mock-llm-auth-modes.spec.ts › auth mode: public gate › shows the auth screen when no key is configuredPosted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
❌ Mock-LLM E2E Tests55/60 passed · 1 failed · 4 skipped Commit:
🔍 Failure details (1)❌ backends/mock-llm-auth-modes.spec.ts › auth mode: public gate › shows the auth screen when no key is configuredPosted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
|
| Status | Test | Duration |
|---|
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses)
Contributor
❌ Mock-LLM E2E Tests52/60 passed · 3 failed · 5 skipped Commit:
🔍 Failure details (3)❌ backends/mock-llm-auth-modes.spec.ts › auth mode: public gate › shows the auth screen when no key is configured⏱️ onboarding/mock-llm-onboarding-happy-path.spec.ts › onboarding happy path › completes the full onboarding flow and launches a conversation❌ onboarding/mock-llm-onboarding-regressions.spec.ts › onboarding recent regressions › defaults the LLM setup step to OpenAI GPT-5.5Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
|
| Status | Test | Duration |
|---|
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses)
Contributor
❌ Mock-LLM E2E Tests53/60 passed · 3 failed · 4 skipped Commit:
🔍 Failure details (3)❌ backends/mock-llm-auth-modes.spec.ts › auth mode: public gate › rejects an incorrect key with an inline error⏱️ onboarding/mock-llm-onboarding-happy-path.spec.ts › onboarding happy path › completes the full onboarding flow and launches a conversation❌ onboarding/mock-llm-onboarding-regressions.spec.ts › onboarding recent regressions › defaults the LLM setup step to OpenAI GPT-5.5Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
|
| Status | Test | Duration |
|---|
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses)
Contributor
❌ Mock-LLM E2E Tests57/60 passed · 2 failed · 1 skipped Commit:
🔍 Failure details (2)⏱️ onboarding/mock-llm-onboarding-happy-path.spec.ts › onboarding happy path › completes the full onboarding flow and launches a conversation❌ onboarding/mock-llm-onboarding-regressions.spec.ts › onboarding recent regressions › defaults the LLM setup step to OpenAI GPT-5.5Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
|
| Status | Test | Duration |
|---|
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses)
Contributor
✅ Mock-LLM E2E Tests60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
|
| Status | Test | Duration |
|---|
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses)
Contributor
✅ Mock-LLM Docker E2E Test Results60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
neubig
force-pushed
the
fix-public-onboarding
branch
from
e81bcd6 to
012625d
Compare
neubig
force-pushed
the
lock-to-cloud-backend
branch
from
6f994e3 to
42af044
Compare
neubig
added
qa-this
all-hands-bot
requested changes
Jun 21, 2026
all-hands-bot
left a comment
all-hands-bot
left a comment
Contributor
There was a problem hiding this comment.
Code review (HEAD 53e80a7)
🔴 Needs improvement — the lock-to-Cloud bypass surface is wider than the PR description implies. The two previously-flagged Critical concerns (PRRT_kwDOSLDBcc6LG07b and PRRT_kwDOSLDBcc6LG07d) are still unaddressed on HEAD. Inline replies on those threads point at the specific lines; the summary is below. I'm leaving the lock decisions to one round of fixes — the cleanup suggestions are small and can ride along.
What this PR does well
- The compile-time
VITE_LOCK_TO_CLOUD+ runtimewindow.__AGENT_CANVAS_LOCK_TO_CLOUD__split is a sensible two-stage lock (operator builds immutable image, ops can still rotate the host without rebuild). Good. normalizeCloudHostrejects bare IPs/paths and forces https — eliminates a whole class of injection edge cases. Good.- The test coverage is genuinely strong: real
localStoragestate, bakedVITE_SESSION_API_KEY,SEEDED_DEFAULT_BACKEND_IDexclusion,openhands-onboardedstale flag, and E2E helpers that adapt to the new optional backend step. Not just mocks. - The docblocks on the lock-sensitive call sites (
root.tsxL184-214,OnboardingHostL11-37) explain why the localStorage flag and LLM-ready fast path can't be trusted in locked mode. That's the kind of comment that earns its keep. - The
isLockedToCloud()cleanup from the previous review round is in (commitaddda40). Confirmed zero remaining call sites.
Why I'm still blocking
The lock has two bypass routes that the existing tests don't cover, and the intent of the feature (single Cloud URL, no manual setup) is undermined by both.
-
root.tsxL214-232 definesisBackendLlmReadywith only one carve-out (SEEDED_DEFAULT_BACKEND_ID).lockedNeedsOnboarding(L188) istruefor any non-Cloud active backend, butshouldShowFirstRunOnboarding(L240) then becomeslockedNeedsOnboarding ? !isBackendLlmReady : .... A user-added Local backend (not the seeded id) with a configured LLM makesisBackendLlmReady === trueand thereforeshouldShowFirstRunOnboarding === false. The lock is silently defeated — OnboardingHost never mounts, the user lands in the app with their Local backend. The "stale persisted Local backend" test (root.test.tsx) covers the seeded default id, which the carve-out already handles; a user-added Local with LLM configured would fail the lock. Gate the ready-backend fast path on the active backend being the configured Cloud host. -
onboarding-modal.tsxL133-137 setsskipBackendStep = !noBackendSelected && healthByBackendId[backend.id]?.isConnected === true. The health probe is the only gate — there is no comparison against the normalizedgetLockedCloudHost(). So if first-run onboarding does mount (e.g.authMissingroute or no-baked-key), and the active backend is a stale-but-reachable user-added Local, the backend slide collapses out of the flow and the user goes Choose Agent → LLM → Hello with their Local backend intact, never seeing the path that would replace it with the locked Cloud backend. Same fix shape as #1: only skip the backend slide when the active backend's host matchesgetLockedCloudHost()(or skip first-run onboarding entirely when the host doesn't match and the lock is on).
Suggested regression tests before re-review:
- root.test.tsx: VITE_LOCK_TO_CLOUD set, user-added (non-seeded) Local backend with
llm_api_key_set: truepersisted — expect first-run onboarding to mount, expect no Manage Backends recovery modal. - onboarding-modal.test.tsx (or new): same setup, render modal — expect
CheckBackendStepto remain in the slide rail, expect advancing past LLM to be blocked until the backend is replaced with the locked Cloud host.
Minor cleanup suggestions (inline)
These are 🟡 nits; they don't change the verdict but they're easy and the PR is already touching all of these files.
src/api/agent-server-config.tsL8 — unused export, see inline.src/root.tsxL214 +src/components/features/onboarding/onboarding-host.tsxL37 — duplicatedisBackendLlmReadybody with a docblock that explicitly says "Must stay in sync"; extract to a single shared helper.src/components/features/onboarding/steps/setup-llm-step.tsxL51 — pointlessinitialValueOverridesintermediate; passONBOARDING_LLM_OVERRIDESdirectly.src/root.tsxL188 vsonboarding-modal.tsxL163 —Boolean(getLockedCloudHost()) &&vsgetLockedCloudHost() !== null; pick one.
[RISK ASSESSMENT]
⚠️ Risk Assessment: 🟡 MEDIUM
The feature is a frontend-only lock for a single-operator deployment (the user is the operator of their own browser), so there is no privilege boundary crossed by the bypasses — but the intent of the feature is "single Cloud URL, no manual setup," and the two above routes undermine that for any deployment that ships pre-baked agent-server images with an LLM env (the realistic case). Until the lock actually forces Cloud login + replacement of any non-Cloud backend, treat it as a soft "default to Cloud" rather than a hard lock.
Recommendation: Do not auto-merge. Address items 1 and 2 above and add the regression tests before re-review.
VERDICT:
❌ Needs rework — two open Critical bypass routes from the prior review round remain on HEAD.
KEY INSIGHT:
The lock currently decides whether to skip onboarding based on the active backend's LLM readiness, not on whether the active backend is the configured Cloud host. In locked mode those should be the same condition, and they aren't.
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 review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Contributor
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. Verdict: REQUEST_CHANGES — two prior Critical concerns remain unaddressed on HEAD
Three 🟡 cleanup suggestions also posted inline:
Suggested regression tests before re-review: (a) VITE_LOCK_TO_CLOUD set + user-added Local backend with Full review (with risk assessment): see the PR review comment. |
Critical fixes for the locked-to-Cloud flow (PR #1389 review): 1. root.tsx: the ready-backend fast-path in locked mode now requires the active backend to match the locked Cloud host (normalized via the new isSameCloudHost helper), not just . A reachable stale Local backend (or a Cloud backend on a different host) that reports a configured LLM no longer bypasses the Cloud login/replacement flow. The markCompleted effect is also guarded so it only persists completion for the legitimate locked Cloud host. 2. onboarding-modal.tsx: in locked mode, CheckBackendStep is only skipped when the active backend IS the locked Cloud host. A reachable stale Local backend keeps the backend slide visible so Cloud login can replace it. Also addresses minor review suggestions: - LOCK_TO_CLOUD_WINDOW_KEY is now module-private (only getLockedCloudHost reads it; static-server.mjs/tests use the literal string). - Extract shared isBackendLlmReady helper into its own module (is-backend-llm-ready.ts) so root.tsx and OnboardingHost stay in sync without duplicating the rule and without pulling the onboarding modal graph into root's eager bundle. - Inline the no-op initialValueOverrides intermediate in setup-llm-step. Adds regression tests for the stale-Local-backend and other-Cloud-host scenarios in both root.test.tsx and onboarding-modal.test.tsx. Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
|
PR Artifacts Notice This PR contains a
|
Contributor
✅ Mock-LLM E2E Tests60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
✅ Mock-LLM Docker E2E Test Results60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
|
🔍 Review in progress… We are performing the review through OpenHands Cloud Automation. You can log in and view the conversation here. |
Member
Author
|
CI is green on |
Contributor
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
requested changes
Jun 22, 2026
all-hands-bot
left a comment
all-hands-bot
left a comment
Contributor
There was a problem hiding this comment.
🔴 Taste Rating: Needs improvement — the lock-to-cloud design is mostly in place, but one stale-backend shortcut still defeats the lock.
[CRITICAL ISSUES]
- [src/components/features/onboarding/steps/check-backend-step.tsx, Line 137] Correctness: A reachable non-locked backend still gets the connected-backend shortcut inside
CheckBackendStep. The parent now keeps the backend slide visible, but this component immediately hides the Cloud login and shows Next, so users with a stale Local backend can continue as Local even with--lock-to-cloudpresent.
[TESTING GAPS]
- [tests/components/onboarding/onboarding-modal.test.tsx, Line 401] The stale-backend locked-mode test only proves the slide remains mounted. It should also assert that the locked Cloud login UI is visible and the connected-backend Next shortcut is not offered for a non-locked backend; that assertion would catch the bug above.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This is frontend-only onboarding/auth routing, so blast radius is limited to public/locked deployments. However, the specific failure mode bypasses the central product promise of--lock-to-cloudfor users with existing localStorage state, so it should be fixed before merge.
VERDICT:
❌ Needs rework: Locked mode must force replacement of stale/non-matching backends all the way through the backend step, not just keep that step visible.
KEY INSIGHT:
Keeping the backend slide in the carousel is insufficient if the slide still treats a stale Local backend as valid and lets the user advance.
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 review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
…kendStep (#1389) PR-review bot pointed out (HEAD 55d382b) that keeping the backend slide visible for a non-matching backend in locked mode is insufficient: CheckBackendStep itself still hits its connected-backend shortcut for a reachable stale Local backend, hiding the Cloud login UI and showing a Next button that lets the user continue as Local. Apply the same host-match guard inside CheckBackendStep. A new local `treatAsNoBackend` (= noBackendSelected || lockedCloudHostMismatch) drives: - title: ONBOARDING$LOGIN_TO_CLOUD_TITLE (not BACKEND_TITLE) - render: BackendConnectionOptions (Cloud login UI), no ConnectionBanner - no "Show configuration" toggle and no Next-shortcut action row `noBackendSelected` still governs whether handleConnected calls `addBackend` or `updateBackend`, so the stale backend is replaced rather than duplicated. Strengthen the regression test the bot flagged: it now asserts the Cloud login title and login button are visible, and that the `onboarding-backend-show-configuration` toggle, `onboarding-backend-next` button, and the (misleading) Connected subtitle are all absent. All 3,249 unit tests pass; lint and typecheck are clean. Co-authored-by: openhands <openhands@all-hands.dev>
Member
Author
|
Addressed the critical |
Contributor
|
PR Artifacts Notice This PR contains a
|
Contributor
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
requested changes
Jun 22, 2026
all-hands-bot
left a comment
all-hands-bot
left a comment
Contributor
There was a problem hiding this comment.
🟡 Taste Rating: Acceptable — the locked-Cloud flow is mostly wired through, but one state carry-over case can strand users after login.
I found one additional blocking issue beyond the existing unresolved threads: replacing a mismatched Cloud backend does not clear its stale org selection, so the newly locked Cloud backend can keep sending an invalid X-Org-Id on subsequent Cloud API calls.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This changes first-run/backend selection gates and Cloud login state, which are core entry paths for new and returning users. The main logic is covered with useful regression tests, but the active org selection is part of the same persisted backend state and needs to be reset when a different Cloud host is replaced.
VERDICT:
❌ Needs rework: Clear stale org selection when replacing a mismatched Cloud backend before this merges.
KEY INSIGHT:
Locked-backend correctness depends on replacing the full active backend context, not only the backend row fields.
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 review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Contributor
✅ Mock-LLM E2E Tests60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
…kend host (#1389) PR-review bot raised one remaining state carry-over: replacing a mismatched Cloud backend updates its host/apiKey via `updateBackend`, but the persisted `active.orgId` (X-Org-Id) is keyed to the OLD host's org list. The newly-locked Cloud backend would keep sending an invalid `X-Org-Id` until the user manually re-picked an org. Fix in CheckBackendStep.handleConnected: when the submitted payload's host differs from the previously-active backend's host, call `setActive(backend.id, null)` to drop the now-invalid org selection. The user re-picks an org on the new host via the usual org switcher. Local-only edits are unaffected because Local backends always carry `active.orgId === null`, so the conditional is a no-op there. New regression test seeds a Cloud backend at other-cloud.example.com with `orgId="stale-org-from-other-host"`, drives the Cloud login button, and asserts `getActiveSelection().orgId === null` while the backend row is updated in place (same id). All 3,250 unit tests pass; lint and typecheck are clean. Co-authored-by: openhands <openhands@all-hands.dev>
Member
Author
|
Addressed the org_id carry-over in 745ed26. CheckBackendStep.handleConnected now clears |
Contributor
|
PR Artifacts Notice This PR contains a
|
Contributor
🛑 Mock-LLM Docker E2E Test Results44/44 passed · Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
|
🔍 Review in progress… We are performing the review through OpenHands Cloud Automation. You can log in and view the conversation here. |
Contributor
❌ Mock-LLM E2E Tests58/60 passed · 2 failed Commit:
🔍 Failure details (2)❌ onboarding/mock-llm-onboarding-happy-path.spec.ts › onboarding happy path › completes the full onboarding flow and launches a conversation❌ onboarding/mock-llm-onboarding-regressions.spec.ts › onboarding recent regressions › defaults the LLM setup step to OpenAI GPT-5.5Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
✅ Mock-LLM Docker E2E Test Results60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
✅ Mock-LLM E2E Tests60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Member
Author
|
/codereview |
hieptl
requested changes
Jun 22, 2026
hieptl
left a comment
hieptl
left a comment
Contributor
There was a problem hiding this comment.
Hello @neubig,
While testing the changes locally, I encountered another issue.
After logging into OpenHands Cloud, the onboarding modal appears to behave inconsistently. It repeatedly appears and disappears, resulting in a flickering effect.
Please see the video below for more details.
issue.mov
Thank you very much! 🙏
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
5 participants
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:
I tested this manually and it works as expected, only showing the cloud login when this option is present.
AGENT:
Why
Frontend-only/public deployments sometimes need backend setup locked to one OpenHands Cloud URL so users cannot configure arbitrary local or custom backend targets during onboarding or Add Backend.
Fixes #1392.
Summary
VITE_LOCK_TO_CLOUD=<cloud-url>configuration for locking backend setup to a single OpenHands Cloud URL.scripts/static-server.mjs --lock-to-cloud <cloud-url>support for pre-built/frontend-only deployments.fix-public-onboardingparent branch intolock-to-cloud-backend.Stacked on #1385. Current stack heads: parent
e81bcd6a, child6f994e3b.How to Test
PATH=/usr/bin:$PATH npm run typecheckPATH=/usr/bin:$PATH npm run test -- __tests__/api/agent-server-config.test.ts __tests__/components/backends/backend-form-modal.test.tsx __tests__/components/onboarding/onboarding-modal.test.tsx __tests__/scripts/static-server.test.ts __tests__/root.test.tsxPATH=/usr/bin:$PATH npm run buildManual QA
--lock-to-cloud https://app.all-hands.devonhttps://work-2-tuizajshdoegydst.prod-runtime.all-hands.dev/: first-run onboarding shows only OpenHands Cloud login, with no local backend form and no Advanced custom Cloud URL field.This PR description was updated 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.28.1-pythonopenhands-automation==1.0.0a10745ed2661f1baa75fcae9743382471b430c15e92Pull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-745ed26Run
All tags pushed for this build
About Multi-Architecture Support
sha-745ed26) is a multi-arch manifest supporting both amd64 and arm64sha-745ed26-amd64) are also available if needed