fix: render the VSCode button on self-hosted (local) backends

[harish-chandramowli]

HUMAN:

I tested this end to end locally to get VSCode button working from my mac mini / docker setups

• A human has tested these changes.

AGENT:

---

Migration: this re-opens OpenHands/agent-canvas#1945, closed 2026-07-27 at @neubig's request following Agent Canvas's archival into this repository. The original PR was the render gate alone; review here has since grown it to cover the deployment wiring that makes the rendered button actually reach an editor. This description was rewritten to match the branch — see History at the bottom for what changed and why.

Why

DrawerVSCodeLink is gated on backend.kind === "cloud", and that is its only render site — so the VSCode button never appears on a self-hosted install.

The data path for local backends is already fully implemented. useUnifiedVSCodeUrl has a complete non-cloud branch, so the hook supports local; only the render gate prevents it from mounting.

The gate reads as a workaround for the agent server advertising a URL that could not work, which the hook's own comment states:

Cloud mode: read VSCode URL from the cloud-computed exposed_urls on the conversation's sandbox. The runtime's /api/vscode/url only knows its internal localhost:8001, so calling it returned a URL […]

That is a server-side defect, and it is now fixed. config/defaults.json pins agentServer: 1.39.1, whose get_vscode_url appends the configured base path to the origin it is given (vscode_service.py:110-120, with a docstring naming exactly the path-routing case). With that, the local URL is correct and the reason for gating on backend kind no longer holds.

This matters for the ordinary self-hosting shape — a machine reached through a single hostname (Cloudflare Tunnel, Tailscale Serve, a reverse proxy), where VSCode is routed by path rather than given its own public port.

Summary

Render the button, and gate it on the editor's actual capability state.

• Render DrawerVSCodeLink unconditionally instead of only when backend.kind === "cloud", and drop the cloud-only pr-1 padding since the ref'd wrapper now always has content.
• Add isUnavailable to useUnifiedVSCodeUrl, driven by /api/vscode/status (VSCodeClient.getStatus()) rather than by treating errors as "unavailable". The URL request only runs once the probe says there is an editor, so a deployment with enable_vscode: false gets no button and no failing request — and transport, auth and server faults stay visible as ordinary query errors rather than being silently swallowed.
  • enabled: false — the deployment switch.
  • running: false alongside enabled: true — terminal, not a startup window: VSCodeService.start() returns False when the binary is missing or the port is taken (vscode_service.py:54-62), and the lifespan awaits it before yielding (api.py:205).
  • probe succeeded but the URL query returns url: null.
• Cloud is excluded from isUnavailable, so cloud behavior is unchanged (see Notes).
• Re-measure the tab row when the button appears or disappears. Its width feeds the inline-tab fit calculation, and its presence is now async, so the ResizeObserver watches the button's wrapper as well as the row — otherwise a deployment with enable_vscode:false permanently shows one fewer inline tab than fits.

Make the advertised URL resolvable, from stock configuration.

The button is only useful if <origin>/vscode/… reaches the editor. getVSCodeUrl sends baseUrl: window.location.origin and agent-server appends the prefix to it, so the origin has to route the prefix to the editor port.

• One editor prefix and port, defined in config/defaults.json, flowing to both agent-server (OH_VSCODE_BASE_PATH / OH_VSCODE_PORT) and the proxy route table. The prefix is preserved, not stripped: openvscode-server is launched with --server-base-path and generates its own HTTP and WebSocket URLs beneath it.
• docker/entrypoint.sh collapses VSCODE_* and the pre-existing OH_VSCODE_* into one effective pair before anything reads them, so a deployment that already sets OH_VSCODE_BASE_PATH=/editor moves the editor and the route together.
• OH_VSCODE_BASE_PATH is an explicit argument to buildAgentServerEnv, not a field on its config. It changes what gets advertised, so a launcher must opt in and register the route; the helper is shared by every launcher, and when the prefix was always-on three of them advertised a prefix they did not serve. dev-static now builds its route tables from getLocalServiceRoutes instead of a hand-maintained copy, dev:minimal proxies the prefix through Vite, and dev-extra-backend deliberately stays out (its browser origin belongs to a different stack, so no global prefix can disambiguate).

Keep the editor's credential inside one perimeter.

agent-server sets the editor's connection token to session_api_keys[0] (vscode_service.py:247-249) and puts it in the URL query string (:120) — so the editor's tkn is also the agent-server API key.

• The editor route is not registered on the PUBLIC_MODE_PORT static server. --auth-required only controls whether the session key is injected into the served HTML; the dispatcher matches routes and returns before consulting it, so it does not gate proxied paths. The other routes there are fine because agent-server checks the key itself; the editor is not.
• --no-referrer-prefix on static-server and ingress sends Referrer-Policy: no-referrer on the editor path only. The workbench renders webviews, previews and extension content from the document whose URL carries the token.
• normalize_base_path now rejects prefixes that would collide with an existing route (/api silently replaced the agent-server route, since the table is keyed by prefix and the editor route is registered last), multi-segment paths, and anything outside [A-Za-z0-9._-]. VSCODE_PORT is checked as numeric.

Issue Number

Relates to #15434 (self-hosted VSCode button unreachable) — deliberately not "Fixes", since that issue is written up as the URL/port problem and this PR addresses the render gate plus the deployment wiring around it.

How to Test

npm ci
npm run make-i18n
npx vitest run __tests__/scripts/ __tests__/components/features/conversation/ __tests__/hooks/use-unified-vscode-url.test.tsx
# 32 files, 383 tests passed
npm run lint

End-to-end through a stock launcher. No manual INGRESS_ROUTES or OH_VSCODE_BASE_PATH — that is the point of the wiring above:

node bin/agent-canvas.mjs

Then start a conversation and open the right panel.

• Before: no button in the tab bar; document.querySelector('[data-testid="drawer-vscode-link"]') returns null.
• After: the button renders and opens the workbench at <origin>/vscode/?tkn=…&folder=… in a new tab.
• With OH_ENABLE_VSCODE=false: no button, and no error toast.

Two things worth knowing if you reproduce this:

1. A path prefix is required for the test to mean anything. Both getVSCodeUrl call sites send baseUrl: window.location.origin, so with no vscode_base_path the server is told the origin and returns <origin>/?tkn=… — agent-canvas itself, not the editor. Verified on a patched build: GET /api/vscode/url?base_url=http://localhost:18300 → http://localhost:18300/vscode/?tkn=…, and that URL gives 302 → vscode-tkn cookie → 200 workbench (2504 bytes).
2. openvscode-server publishes Linux-only builds (the 1.109.5 release has exactly three assets, all linux-*), and agent-server resolves it from a hardcoded /openhands/.openvscode-server. On a Linux host, symlink that path and agent-server spawns the editor itself. On macOS it cannot, so in the recording below the editor was run in Docker (gitpod/openvscode-server, linux/arm64) on :19000 with the same flags agent-server would pass — --server-base-path /vscode --connection-token <session key> — with the canvas ingress routing /vscode to it. The URL construction and routing under test are unchanged by that; only who spawns the process differs.

Video/Screenshots

Two recordings from the stack described above — same build, same backend, the only difference being enable_vscode:

1. enable_vscode on — a conversation on a local backend, right panel open, the VSCode button present in the tab bar. Clicking it opens http://localhost:18300/vscode/?tkn=…&folder=…, which loads the real workbench (.monaco-workbench, with the conversation's workspace folder in the Explorer).
2. enable_vscode: false — identical layout and the same tab bar, with no VSCode button and no error toast.

01-button-works-enable_vscode-true.webm
02-button-hidden-enable_vscode-false.webm

Both are also asserted by unit tests, but the recordings are what show the base path actually resolving end to end.

Type

• Bug fix
• Feature
• Refactor
• Breaking change
• Docs / chore

Notes

This changes an existing tested behavior, deliberately. conversation-tabs.test.tsx asserted should hide the vscode link when the active backend is local; both it and the gate arrived together in OpenHands/agent-canvas#1288 ("UI polish: drawer tabs, empty states, and browser chrome"). If hiding the button on local backends was an intentional product decision rather than a consequence of the unusable server URL, then this PR is wrong and I would rather know that than have it merged.

Sequencing is resolved. The earlier revision of this description said it depended on software-agent-sdk#4222. That is merged and released: config/defaults.json pins agentServer: 1.39.1, whose get_vscode_url emits the base path. No hold needed.

Cloud is deliberately left alone. A cloud sandbox that is still STARTING has no VSCODE entry in exposed_urls yet and will get one shortly, so treating that as "unavailable" would hide the button during startup. isUnavailable is therefore hardcoded false on the cloud branch and cloud rendering is byte-identical to today. That does leave cloud's own dead-button case (a sandbox that never exposes VSCode) unaddressed; happy to extend it there in a follow-up if you consider that a real state rather than a transient one.

docker run --network host still reaches the editor port directly. The image does not EXPOSE 8001 and a test asserts that, but openvscode-server binds --host 0.0.0.0 (vscode_service.py:170), so under host networking — which playwright.mock-llm-docker.config.ts:52 uses — port 8001 is host-reachable with only ?tkn= in front of it. Not introduced here, but worth stating plainly rather than claiming the port is unreachable.

History

The branch has been reworked twice under review, and the description above describes only the current state. For reviewers returning to it:

• The original diff suppressed the 503 from a VSCode-disabled backend with meta.disableToast. That was replaced with the /api/vscode/status probe, on @neubig's suggestion — it removes the failing request instead of hiding it, and keeps genuine faults loud.
• The Docker/launcher wiring (route registration, one normalized port/prefix pair, prefix opt-in, public-mode exclusion, prefix validation) was added in response to review, and is the larger half of the diff. It is what makes the rendered button reach an editor from a stock install rather than only from a hand-configured one.

[neubig]

🟡 Acceptable

The direction is useful: removing a cloud-only presentation gate is reasonable once the local deployment advertises and serves the editor consistently. The remaining work can reuse machinery that is already present in this repository.

[CRITICAL ISSUES]

• [scripts/dev-with-automation.mjs, docker/entrypoint.sh] Complete the stock /vscode route before exposing the control. The current E2E command proves the desired topology, but only by manually supplying OH_VSCODE_BASE_PATH=/vscode and INGRESS_ROUTES. A typical single-origin deployment looks like this:

  browser: https://host/vscode/*
      -> ingress prefix route /vscode (path preserved; HTTP and WebSocket)
      -> http://127.0.0.1:<vscode-port>/vscode/*
  

  The prefix should be preserved, not stripped, because OpenVSCode is launched with --server-base-path /vscode and generates its HTTP and WebSocket URLs beneath that path. The existing proxy stack already does the hard parts: createRouter() chooses the longest matching prefix, httpxy forwards the original request path, and both ingress.mjs and static-server.mjs handle HTTP plus WebSocket upgrades. This should therefore be configuration, not a new proxy implementation:

  1. Define one stable base path (for example /vscode) alongside the existing centralized launcher defaults.
  2. Add [basePath, http://localhost:${config.vscodePort}] to getLocalServiceRoutes(config) when agent-server is launched. That covers the outer npm ingress and the static-server route list built from the same function.
  3. Pass the same value as OH_VSCODE_BASE_PATH in the agent-server environment; OH_VSCODE_PORT is already populated by buildAgentServerEnv().
  4. In docker/entrypoint.sh, set/export the corresponding VSCode port and base path, and add the same --route to both normal and public-mode static-server.mjs invocations.

  With those values aligned, getVSCodeUrl({ baseUrl: window.location.origin }) can advertise https://host/vscode/?..., and that URL reaches the service without exposing a second public port. Without this wiring, the normal launcher still returns the Canvas origin and the button does not reach the editor. Also hold the UI change until software-agent-sdk#4222 is included in an agent-server release and the centralized config/defaults.json pin is advanced through the release workflow; the current 1.37.0 pin does not contain that fix.

[IMPROVEMENT OPPORTUNITIES]

• [src/hooks/query/use-unified-vscode-url.ts, lines 62-65 and 119-124] Use the existing status API instead of treating every error as “unavailable.” isUnavailable = isError || ... makes a disabled editor indistinguishable from authentication, connectivity, proxy, or server failures, and meta.disableToast then suppresses all of them. @openhands/typescript-client@1.34.0 already exposes VSCodeClient.getStatus() for /api/vscode/status, returning { enabled, running, message? }.

  A simpler state flow would be:

  1. After the runtime is ready, query getStatus() using the same getAgentServerClientOptions({ conversationUrl, sessionApiKey }) overrides.
  2. Hide the control only for an explicit terminal capability state (enabled === false, or the agreed policy for running === false).
  3. Enable the URL query only when status says the editor is available.
  4. Leave transport/auth/server failures as query errors so normal retry and error reporting still apply.

  This removes the blanket toast suppression and the error-derived isUnavailable special case, detects a configured editor whose process failed to start, and avoids repeatedly running the primary URL request plus its fallback before concluding the editor is disabled.

[TESTING GAPS]

• [Launcher tests] Add focused coverage that the npm and Docker stock configurations register /vscode, preserve the prefix, forward WebSocket upgrades, and pass matching port/base-path values to agent-server.
• [__tests__/hooks/use-unified-vscode-url.test.tsx] Replace the mocked-503-as-capability case with explicit status cases: disabled hides the button, running exposes it, and a transport/auth failure remains observable as an error.
• [End-to-end] Keep the existing workbench assertion, but run it through an unmodified npm or Docker launcher without manual INGRESS_ROUTES / OH_VSCODE_BASE_PATH. That is the regression test for the actual self-hosted install path.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM

The UI change itself is small, but it spans the frontend, agent-server release compatibility, and two shared deployment entrypoints. The risk becomes manageable once the route, advertised base path, and status model are driven from the same configuration and exercised through a stock launcher.

VERDICT:
❌ Needs rework: Complete the existing ingress/status integration and release sequencing, then the render-gate removal should be straightforward.

KEY INSIGHT:
Treat /vscode as one explicitly configured service route and availability as one typed capability state; the repository already contains the proxy and client machinery needed for both.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger 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.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. 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 /iterate to automatically drive this PR through CI, review, and QA until it is merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

---

[harish-chandramowli]

Both confirmed against the code and fixed in d477beb.

Advertised route absent on the direct Vite origin. Correct. startVite set VITE_VSCODE_BASE_PATH and nothing else, and vite.config.ts:448 spreads the proxy entry only when VITE_VSCODE_BASE_PATH && VITE_VSCODE_TARGET. What settles that the Vite origin is a supported endpoint rather than an internal detail is AUTOMATION_CORS_ORIGINS at dev-with-automation.mjs:973, which allowlists http://localhost:3001 and http://127.0.0.1:3001 alongside the ingress. So the advertisement was reaching an origin with no route behind it. Now set together in one block, targeting http://127.0.0.1:${config.vscodePort} as dev-safe.mjs:1015 does.

Collision guard compares normalized and raw paths. Correct, and the takeover is real: static-server.mjs:134 runs --base-path through normalizeBasePath, which turns canvas into /canvas, while the reserved-prefix loop compared it raw against an already-normalized VSCODE_BASE_PATH. Both sides now go through normalize_base_path.

One thing I did beyond the literal fix. AGENT_CANVAS_BASE_PATH was resolved above the >>> vscode-config markers, so the extracted block the docker test executes never saw its default — the guard could only ever be tested against values a test injected, and the likeliest real instance of this bug is a deployment that moves only the editor and never sets AGENT_CANVAS_BASE_PATH at all. Moved its resolution inside the block so that case is reachable; guards the default canvas mount without being told it covers it.

Both testing gaps covered:

• full-stack dev advertises the prefix only where Vite also proxies it — asserts both env vars are set inside the same guarded block, and that the target is the editor's own port rather than the backend host.
• The collision guard now runs against canvas, /canvas, /canvas/ and //canvas//, plus a non-colliding noncanonical mount (canvas/ + editor) so normalizing both sides doesn't start rejecting layouts that are fine.

Each new assertion was checked to fail with its own fix reverted — 3 of the 4 spellings did, /canvas being already covered by the existing case. Typecheck clean, bash -n and shellcheck clean on the entrypoint (only pre-existing SC2329 on the trap-invoked cleanup). Full suite 4135 passed / 13 failed, and those 13 reproduce identically with this branch's changes stashed — conversation-runtime-info, recommended-automations and use-websocket, all failing on localStorage.clear() being undefined in this environment.

[github-actions]

✅ Mock-LLM E2E Tests

60/60 passed

Commit: d477beb1 · Workflow run · Test artifacts

Details

Status	Test	Duration
✅	automations/mock-llm-automation.spec.ts › mock-LLM automation lifecycle › step 1: setup LLM profile and register automation trajectory	6.6s
✅	automations/mock-llm-automation.spec.ts › mock-LLM automation lifecycle › step 2: create automation and dispatch run via the UI	28.5s
✅	automations/mock-llm-automation.spec.ts › mock-LLM automation lifecycle › step 3: verify automation and run on the automations page	6.4s
✅	automations/mock-llm-preset-automation.spec.ts › preset automation → slash command conversation › automation card sends the correct slash command to a conversation	15.3s
✅	automations/mock-llm-preset-automation.spec.ts › preset automation → slash command conversation › direct slash command from home page triggers skill activation	13.0s
✅	backends/mock-llm-auth-modes.spec.ts › auth mode: fresh install with runtime-injected key › reaches the onboarding modal without pre-seeded localStorage	1.4s
✅	backends/mock-llm-auth-modes.spec.ts › auth mode: non-public key rotation › recovers when localStorage has a stale session API key	5.4s
✅	backends/mock-llm-auth-modes.spec.ts › auth mode: public gate › shows first-run onboarding before the auth screen when no key is configured	1.5s
✅	backends/mock-llm-auth-modes.spec.ts › auth mode: public gate › rejects an incorrect key with an inline error	1.6s
✅	backends/mock-llm-auth-modes.spec.ts › auth mode: public gate › allows access after pasting the correct key	1.7s
✅	backends/mock-llm-auth-modes.spec.ts › auth mode: public gate › skips auth screen for returning user with valid stored key	830ms
✅	backends/mock-llm-auth-modes.spec.ts › auth mode: public gate › re-prompts when the server rotates its key (stale localStorage)	1.4s
✅	backends/mock-llm-cross-connect.spec.ts › cross-connect: frontend-only → backend-only › frontend-only connects to a separate backend-only instance	17.0s
✅	backends/mock-llm-cross-connect.spec.ts › cross-connect: frontend-only → multiple backends › connects to two separate backends and switches between them	20.9s
✅	backends/mock-llm-partial-stack.spec.ts › partial stack: --frontend-only › serves the frontend but returns 503 for backend routes	7.4s
✅	backends/mock-llm-partial-stack.spec.ts › partial stack: --backend-only › serves backend APIs but returns 503 for the frontend root	15.1s
✅	backends/mock-llm-partial-stack.spec.ts › partial stack: port conflict › fails with a clear error when the ingress port is occupied	109ms
✅	backends/mock-llm-partial-stack.spec.ts › partial stack: port conflict › starts successfully on a free port after a conflict	6.0s
✅	conversations/mock-llm-conversation.spec.ts › mock-LLM agent-server conversation › step 1: create an LLM profile pointing at the mock LLM server	6.3s
✅	conversations/mock-llm-conversation.spec.ts › mock-LLM agent-server conversation › step 2: activate the mock-llm profile and verify settings API	7.9s
✅	conversations/mock-llm-conversation.spec.ts › mock-LLM agent-server conversation › step 3: run a conversation with the mock LLM	7.3s
✅	conversations/mock-llm-conversation.spec.ts › mock-LLM agent-server conversation › step 4: resume conversation from sidebar after navigating away	5.9s
✅	conversations/mock-llm-image-upload.spec.ts › mock-LLM image upload › attaching an image embeds it as base64 in the LLM completion call	13.1s
✅	files/mock-llm-files-and-git.spec.ts › files tab, git control bar, and browser tab › step 1: ensure mock LLM profile is configured	6.6s
✅	files/mock-llm-files-and-git.spec.ts › files tab, git control bar, and browser tab › step 2: start conversation and attach workspace metadata	11.8s
✅	files/mock-llm-files-and-git.spec.ts › files tab, git control bar, and browser tab › step 3: git control bar shows workspace pill and git actions	25.4s
✅	files/mock-llm-files-and-git.spec.ts › files tab, git control bar, and browser tab › step 4: files tab can enable diff view for attached workspace	5.9s
✅	files/mock-llm-files-and-git.spec.ts › files tab, git control bar, and browser tab › step 5: browser tab shows empty state	6.3s
✅	files/mock-llm-files-and-git.spec.ts › files tab, git control bar, and browser tab › step 6: files tab defaults to file-tree view without attached workspace	8.3s
✅	home/mock-llm-folder-workspace.spec.ts › mock-LLM folder browser → workspace → conversation › step 1: browse to a folder, add it as a workspace, and launch a conversation with the correct working_dir	8.7s
✅	mcp/mock-llm-mcp-github.spec.ts › MCP GitHub server install flow › step 1: GitHub card is visible on the MCP marketplace page	5.5s
✅	mcp/mock-llm-mcp-github.spec.ts › MCP GitHub server install flow › step 2: clicking GitHub add control opens the install modal with correct fields	5.7s
✅	mcp/mock-llm-mcp-github.spec.ts › MCP GitHub server install flow › step 3: full install flow — fill PAT, submit, verify installed	12.4s
✅	mcp/mock-llm-mcp-github.spec.ts › MCP GitHub server install flow › step 4: installed GitHub server can be deleted	6.0s
✅	mcp/mock-llm-mcp-slack-credentials.spec.ts › MCP Test Connection credential verification (Slack) › install: invalid Slack credentials are blocked with a credential-check error	5.9s
✅	mcp/mock-llm-mcp-slack-credentials.spec.ts › MCP Test Connection credential verification (Slack) › install: a valid token missing only a scope still installs (missing_scope is not a credential failure)	5.9s
✅	mcp/mock-llm-mcp-slack-credentials.spec.ts › MCP Test Connection credential verification (Slack) › install: an older agent server that omits tool_result still installs (compat)	6.0s
✅	mcp/mock-llm-mcp-slack-credentials.spec.ts › MCP Test Connection credential verification (Slack) › edit: Test Connection verifies the stored credentials and surfaces a credential failure	5.8s
✅	mcp/mock-llm-mcp-slack-credentials.spec.ts › MCP Test Connection credential verification (Slack) › edit: Test Connection reports success for valid stored credentials	5.9s
✅	mcp/mock-llm-mcp-slack-credentials.spec.ts › MCP Test Connection credential verification (Slack) › custom (non-catalog) server: Test Connection attaches no verification probe	5.8s
✅	onboarding/mock-llm-onboarding-happy-path.spec.ts › onboarding happy path › completes the full onboarding flow and launches a conversation	4.4s
✅	onboarding/mock-llm-onboarding-regressions.spec.ts › onboarding recent regressions › keeps the modal open on backdrop click and Escape	1.5s
✅	onboarding/mock-llm-onboarding-regressions.spec.ts › onboarding recent regressions › defaults the LLM setup step to OpenAI GPT-5.5	1.6s
✅	regressions/mock-llm-ui-regressions.spec.ts › UI regressions › scopes standalone styles to the agent-server-ui shell	1.3s
✅	regressions/mock-llm-ui-regressions.spec.ts › UI regressions › renders critic results on agent messages and finish actions	1.6s
✅	regressions/mock-llm-ui-regressions.spec.ts › UI regressions › loads older events when scrolling up	1.8s
✅	regressions/mock-llm-ui-regressions.spec.ts › UI regressions › selected workspace persists after navigating away and returning	2.1s
✅	regressions/mock-llm-ui-regressions.spec.ts › UI regressions › cleared sessionStorage yields empty workspace selection	1.0s
✅	settings/mock-llm-acp-agent.spec.ts › mock-LLM ACP agent conversation › step 1: configure ACP agent via Settings → Agent UI	13.2s
✅	settings/mock-llm-acp-agent.spec.ts › mock-LLM ACP agent conversation › step 2: reload and verify ACP settings are persisted in UI	5.8s
✅	settings/mock-llm-acp-agent.spec.ts › mock-LLM ACP agent conversation › step 3: start ACP conversation and verify agent reply	6.7s
✅	settings/mock-llm-acp-agent.spec.ts › mock-LLM ACP agent conversation › step 4: resume ACP conversation from sidebar after navigating away	5.8s
✅	settings/mock-llm-model-switch.spec.ts › mock-LLM /model slash command › step 1: configure LLM, create switch-target profile, register trajectory	7.2s
✅	settings/mock-llm-model-switch.spec.ts › mock-LLM /model slash command › step 2: start conversation, switch profile via /model, verify switch	7.1s
✅	settings/mock-llm-profile-management.spec.ts › active profile deletion + reconciliation › active profile is deletable and reconciliation activates another profile	8.1s
✅	settings/mock-llm-profile-management.spec.ts › same-model profile identity › chat header shows the correct profile when two profiles share the same model	14.9s
✅	settings/mock-llm-profile-management.spec.ts › OpenHands provider hidden base_url preservation › re-saving an OpenHands profile from Basic view preserves hidden base_url	7.6s
✅	skills/mock-llm-skills.spec.ts › skill loading: project, user, and deletion › project skill in workspace/.agents/skills/ triggers on matching keyword	13.1s
✅	skills/mock-llm-skills.spec.ts › skill loading: project, user, and deletion › user skill in ~/.openhands/skills/ triggers on matching keyword	8.6s
✅	skills/mock-llm-skills.spec.ts › skill loading: project, user, and deletion › deleting a user skill removes it from subsequent conversations	8.0s

_{Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses)}

[enyst]

👋 I'm an AI agent (Opus 5) reviewing this PR on behalf of Engel Nyst (@enyst).

🔴 Needs improvement - The routing mechanics are carefully synchronized, but the design removes the browser security boundary between Canvas and the bundled editor.

[CRITICAL ISSUES]

• [docker/entrypoint.sh, Line 382; scripts/dev-with-automation.mjs, Lines 701-712] Credential isolation regression: Proxying OpenVSCode at /vscode puts it on exactly the same browser origin as Canvas. Paths are not security boundaries: JavaScript served anywhere on that origin can read the same localStorage and make same-origin API calls. Canvas persists the complete backend registry, including every configured backend's apiKey, in localStorage (src/api/backend-registry/storage.ts, Lines 95-99), so an editor-side XSS, compromised OpenVSCode asset, or unsafe editor/extension web content can now exfiltrate credentials unrelated to the current editor session. Referrer-Policy: no-referrer only stops the token-bearing URL from being sent as a Referer; it does nothing to isolate storage or JavaScript authority. Keep the editor on a distinct origin (a separate host/subdomain or port), or first redesign credential storage and establish an equally strong browser isolation boundary; a path-prefix proxy cannot provide one.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH
  This changes the trust boundary around authentication material across every bundled local/Docker deployment. The blast radius includes all backend API keys saved in the Canvas registry, while the editor is a large, extensible application with a materially broader content and dependency surface. Recommendation: Do not auto-merge. Request review from a human security architect/reviewer to validate the editor/Canvas origin model and credential exposure.

VERDICT:
❌ Needs rework: Correct route plumbing does not compensate for collapsing two applications into one browser origin.

KEY INSIGHT:
A URL path separates routing, not trust; browser storage and script authority are scoped to the entire origin.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger 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.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. 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 /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

[harish-chandramowli]

Re: the same-origin credential review — I'm not implementing this one, because the fix it prescribes is the design a previous review on this same PR rejected. I'd like a maintainer to break the tie before I move more code. @enyst @neubig

The finding is technically correct

• The editor is genuinely new on this origin. git show origin/main:docker/entrypoint.sh | grep -i vscode returns nothing — the stock image never exposed the bundled editor to a browser at all. So this isn't a pre-existing property I can wave off; serving it at /vscode on the Canvas origin is introduced by this branch (579a507e6).
• The credentials are genuinely reachable. src/api/backend-registry/storage.ts:98 writes the whole backend array to localStorage, and Backend.apiKey is a plain string (src/api/backend-registry/types.ts:8). Same origin means editor-side script authority reads every configured backend's key, not just this one's.

And the principle is right: a path prefix routes, it doesn't isolate. Referrer-Policy: no-referrer addresses URL leakage, not storage or script authority. No disagreement there.

But it contradicts the earlier review on this PR

Review from 2026-07-30 13:14, same reviewer, essentially this architecture:

KEY INSIGHT: Proxying the editor behind the authenticated agent-server preserves one security perimeter instead of creating a second public port.

VERDICT: ✅ Worth merging

Review from 2026-07-31 04:10:

KEY INSIGHT: A URL path separates routing, not trust; browser storage and script authority are scoped to the entire origin.

Keep the editor on a distinct origin (a separate host/subdomain or port)

A separate port is the thing the first review counted as the win and the second one requires. I can't satisfy both, and picking one myself would mean discarding four rounds of review that were given on the assumption of the other.

Three options, as I see them

1. Separate origin (what the latest review asks for). Correct isolation, and it matches how Cloud already works — the editor lives on a different host there, which is precisely why Cloud never had this problem. Cost: reopens the second-public-port posture the earlier review objected to, and for the all-in-one Docker image it means a second published port and a second thing to secure in every self-hosted deployment.
2. Keep the path proxy, fix the actual hazard. What makes the shared origin dangerous is not that the editor is on it — the editor already has full filesystem and shell access to the workspace, so its own session is not what's being protected. It's that Canvas persists other backends' API keys in localStorage where editor-side script can reach them. Scoping or removing that storage closes the cross-backend exposure without a second origin. This is a Canvas credential-storage change, though — separate PR, and larger than this one.
3. Ship the editor off by default on the shared origin, opt-in per deployment with the tradeoff documented. Smallest change; leaves the sharp edge in place for anyone who opts in.

My preference is (1) if the second port is acceptable to maintainers, since it's the only one that's actually a boundary — but that's a project posture call about the self-hosted deployment story, not something the PR author should decide unilaterally.

[smolpaws]

👋 Hi, I'm smolpaws — a small AI cat agent (OpenHands under the hood), here on Engel's behalf. 🐾

Thanks, @harish-chandramowli — you were right to stop and ask for a human call rather than pick an architecture unilaterally, and the same-origin finding is real (I checked it in the code).

Where we landed:

Ship it. We want the functionality, and the practical risk is narrow. The editor already has full shell + filesystem access to its workspace, so its own session isn't the concern. The new exposure is that editor-side script could read other backends' API keys from localStorage — but that needs the editor XSS'd first (or a hostile extension / compromised asset), and this is single-user self-hosted. Small, acceptable edge for a typical install.

Two things we're documenting rather than fixing here:

• Backend keys in origin-readable localStorage isn't great — but it's already true today, independent of this PR; the editor is just one more surface. Real fix tracked in Backend API keys live in origin-readable localStorage — any same-origin script can read all of them #16492.
• OH_ENABLE_VSCODE=0 already turns the button and editor off for anyone who wants it.

So: merge with the shared-origin tradeoff documented (a short deployment-docs note + pointer to #16492), rather than blocking on the credential redesign here.

Only asks before merge: it needs a rebase (currently conflicting), and a one-line note naming the tradeoff so the next person isn't surprised. Thanks again for the careful work. 🐾

[enyst]

(HUMAN:)

LGTM, thank you for this @harish-chandramowli !

A note: we could, as you suggest in your last comment, make it opt-in, rather than opt-out. But personally, I'm torn whether we really want to, considering that the risk that the agents grumble about is dependent on some other exploit, and we do aim to fix the local storage anyway.

If I may suggest, rather, after it's ready give it another spin just to see that we didn't break things with the recent fixes, and please let me know.

Thank you again!