Repository navigation
fix(static-server): expose runtime session key via window global so fresh installs reach onboarding - #1091
Conversation
The published `agent-canvas` binary is built with no `VITE_SESSION_API_KEY` baked in. At runtime, `scripts/static-server.mjs --session-api-key <key>` injects the key into `index.html`, but only as a write to `localStorage["openhands-agent-server-config"].sessionApiKey`. PR #1046 ("Simplify backend registry selection") removed the legacy localStorage fallback from `getAgentServerSessionApiKey()`, leaving it reading only `import.meta.env.VITE_SESSION_API_KEY`. The combination broke the first-launch experience for users who installed the npm package globally: 1. Fresh browser → `localStorage["openhands-backends"]` is null. 2. `readLegacyBackend()` requires `baseUrl` AND `sessionApiKey` in `openhands-agent-server-config`; the injection only writes the key, so it returns null. 3. `makeDefaultLocalBackend()` reads `VITE_SESSION_API_KEY` → null → returns null. 4. Registry seeds empty → `MissingAgentServerScreen` renders the Manage Backends modal ("No extra backends added yet.") with no way out. The fix mirrors how `__AGENT_CANVAS_AUTH_REQUIRED__` already passes the public-mode flag from `static-server.mjs` to the bundle: - `static-server.mjs` now also injects `window.__AGENT_CANVAS_SESSION_API_KEY__ = <key>` in the `<head>` script. The window global is set first so it is available even if the localStorage write throws (private mode, etc.). - `getBakedSessionApiKey()` in `src/api/agent-server-config.ts` falls back to the window global when `VITE_SESSION_API_KEY` is empty. Tests: - Unit tests in `__tests__/api/agent-server-config.test.ts` cover the window-global fallback, env-takes-precedence ordering, whitespace, and non-string injected values. - `__tests__/scripts/static-server.test.ts` asserts the new window assignment lands before the localStorage write. - A new mock-LLM E2E spec in `tests/e2e/mock-llm/mock-llm-auth-modes.spec.ts` exercises the exact user scenario: a fresh browser context with no pre-seeded localStorage must reach the onboarding modal (not the Manage Backends trap modal) when launched against the prebuilt stack. - Updated docstrings in the auth-modes spec and helper to remove references to the deleted `syncBakedSessionApiKey()` function. AGENTS.md updated to document the window-global path and the current key-rotation reconciliation (`syncLauncherDefaultLocalBackend()` in `storage.ts`). Co-authored-by: openhands <openhands@all-hands.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
✅ 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
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ Review Summary
This is a well-structured bug fix that addresses a legitimate user-facing issue where fresh installs of the published agent-canvas npm package would get stuck on the "Manage Backends" modal instead of the expected onboarding flow.
What Changed
| File | Change |
|---|---|
scripts/static-server.mjs |
Injects window.__AGENT_CANVAS_SESSION_API_KEY__ global in <head> script |
src/api/agent-server-config.ts |
Falls back to window global when VITE_SESSION_API_KEY is empty |
| Unit & E2E tests | Comprehensive coverage of the new fallback mechanism |
| AGENTS.md | Updated documentation |
Strengths
-
Follows existing patterns — The
__AGENT_CANVAS_SESSION_API_KEY__mechanism mirrors the existing__AGENT_CANVAS_AUTH_REQUIRED__pattern, making the codebase consistent. -
Defensive coding — The window global is set before the localStorage write, ensuring it's available even if localStorage access throws (e.g., private browsing mode).
-
Proper precedence — The env-baked
VITE_SESSION_API_KEYstill takes priority over the runtime-injected window global, so existing dev workflows remain unaffected. -
Excellent test coverage — Unit tests for all edge cases (env precedence, whitespace, non-string values), static-server integration tests verifying injection order, and a fresh-install E2E test that exactly reproduces the user's scenario.
-
Clean PR description — The root cause analysis and fix explanation are thorough and well-documented.
Risk Assessment
Low risk — This is an additive change that introduces a fallback mechanism without modifying existing behavior. The localStorage write is retained for backwards compatibility with any code paths that still read it.
Minor Observations
- The test cleanup in
afterEachproperly deletes the window global to prevent test pollution — good hygiene. - The type assertion
as unknown as Record<string, unknown>is a reasonable TypeScript approach for accessing dynamic window properties.
LGTM! 🚀
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
📸 Snapshot Test ReportWarning Snapshot comparison step crashed (timeout, OOM, or runner error) — diff results below may be incomplete or absent. ✅ No baseline found on main — all 73 snapshots are new and will become the baseline once this PR merges.
🆕 New snapshots (73)These snapshots have no baseline on main and will become the new baseline once this PR merges.
|
Problem
#2095
A user reported that on the published npm package (
v1.0.0-beta.7):does NOT show the onboarding flow. Instead it lands on the Manage Backends modal with "No extra backends added yet." and there is no way out — even clicking "Done" doesn't dismiss it. Running locally with
npm run devworks correctly.Root cause
scripts/dev-with-automation.mjs(npm run dev) bakes the runtime session key into the Vite dev server viaviteEnv.VITE_SESSION_API_KEY = config.sessionApiKey, so the bundle has the key at module init..github/workflows/npm-publish.ymlrunsnpm run buildwithout that env var, so the published bundle has noVITE_SESSION_API_KEY.bin/agent-canvas.mjsinstead generates a key at launch time and passes it toscripts/static-server.mjs --session-api-key <key>, which injects a<head>script that writes tolocalStorage["openhands-agent-server-config"].sessionApiKey.PR #1046 ("Simplify backend registry selection") deleted
getConfiguredSessionApiKey()— the legacy-localStorage fallback insidegetAgentServerSessionApiKey(). After #1046, the React app reads the session key fromVITE_SESSION_API_KEYonly. The published-binary path therefore takes this walk on a fresh browser:readStoredBackends()(src/api/backend-registry/storage.ts):localStorage["openhands-backends"]isnull.readLegacyBackend()requires bothbaseUrlANDsessionApiKeyto be present inopenhands-agent-server-config. The static-server only writessessionApiKey, so it returnsnull.makeDefaultLocalBackend():getAgentServerSessionApiKey()→getBakedSessionApiKey()→import.meta.env.VITE_SESSION_API_KEY→ empty → returnsnull.[].loadAgentServerInfo()throwsAgentServerUnavailableError.root.tsxrendersMissingAgentServerScreen→<ManageBackendsModal onClose={noop} />→ "No extra backends added yet.", with no way to dismiss it (becauseonCloseis a no-op behind the always-rendered backdrop).The bug is already known internally —
tests/e2e/mock-llm/utils/mock-llm-helpers.tspapers over it inseedLocalStorage()'s docstring:That hides the bug in tests but does nothing for real users on a fresh
agent-canvaslaunch.Fix
Mirror the existing
__AGENT_CANVAS_AUTH_REQUIRED__mechanism thatisAuthRequired()already uses to receive a--auth-requiredflag from the static-server:scripts/static-server.mjs: the--session-api-keyinjection now also setswindow.__AGENT_CANVAS_SESSION_API_KEY__ = "<key>"in the<head>script. The window assignment is emitted before the localStorage write so the global is set even if storage access throws (private mode, etc.).src/api/agent-server-config.ts:getBakedSessionApiKey()falls back towindow.__AGENT_CANVAS_SESSION_API_KEY__whenVITE_SESSION_API_KEYis empty. Whitespace-only and non-string values are treated as missing.Net result: a fresh
agent-canvaslaunch reachesmakeDefaultLocalBackend()→ returns a valid backend → registry is seeded →useConfig()succeeds →OnboardingHostmounts → the user sees the onboarding modal as expected.Test plan
Unit tests:
__tests__/api/agent-server-config.test.ts— six new cases for the runtime-injection fallback (env precedence, runtime fallback, form defaults, missing both, whitespace, non-string).__tests__/scripts/static-server.test.ts— asserts the<head>script containswindow.__AGENT_CANVAS_SESSION_API_KEY__with the right value, that the window assignment lands before the localStorage write, and that nullsessionApiKeystill produces no injection.E2E (mock-LLM):
auth mode: fresh install with runtime-injected keydescribe block intests/e2e/mock-llm/mock-llm-auth-modes.spec.tsexactly reproduces the user's scenario (noseedLocalStorage, fresh browser context) and asserts the user lands ononboarding-step-choose-agent, NOT onagent-server-onboarding-screen(the Manage Backends trap).openhands-backends, whichsyncLauncherDefaultLocalBackend()reconciles against the (now resolvable)makeDefaultLocalBackend().Verification commands:
Manual check against a real built
build/:Docs
VITE_SESSION_API_KEYbaked in.syncBakedSessionApiKey()was removed by Simplify backend registry selection #1046; the active reconciliation function is nowsyncLauncherDefaultLocalBackend()insrc/api/backend-registry/storage.ts.Notes for reviewers
main. The same fix should also be cherry-picked torel-1.0.0for the next beta — happy to do that as a follow-up.localStorage["openhands-agent-server-config"].sessionApiKeywrite is retained intentionally (a) for compatibility with any code path that still reads it, and (b) because the existing key-rotation E2E test asserts the stale stored value is overwritten on boot. Removing the write entirely is a follow-up cleanup.This PR was created by an AI agent (OpenHands) on behalf of @neubig.
@chuckbutkus can click here to continue refining the PR
🐳 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.0a5f40e63f366604b7bdf4a8d0b9e13922b147f5d4aPull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-f40e63fRun
All tags pushed for this build
About Multi-Architecture Support
sha-f40e63f) is a multi-arch manifest supporting both amd64 and arm64sha-f40e63f-amd64) are also available if needed