fix(onboard): dashboard health probe and CORS for non-loopback URLs (#2342) - #2401
Closed
jyaunches wants to merge 2 commits into
Closed
fix(onboard): dashboard health probe and CORS for non-loopback URLs (#2342)#2401jyaunches wants to merge 2 commits into
jyaunches wants to merge 2 commits into
Conversation
…ORS origin for non-loopback URLs (#2342) Phase 1: Change dashboard readiness probe from `curl -sf localhost:PORT/` to `curl -sf localhost:PORT/health` with a fallback that accepts any HTTP response (including 401) from `/`. The root path returns 401 when device auth is enabled (standard for Brev Launchable), causing the probe to false-negative for 30s. Phase 2: Auto-detect when CHAT_UI_URL is non-loopback (e.g. Brev public URL like https://nemoclaw0-xxx.brevlab.com) and inject NEMOCLAW_CORS_ORIGIN into the sandbox env args. This causes nemoclaw-start.sh's apply_cors_override() to add the browser's origin to gateway.controlUi.allowedOrigins at startup, fixing WebSocket/API connection rejections from external URLs. Fixes: #2342 Related: #1178, #2258, #2042, #2390
Contributor
📝 WalkthroughWalkthroughThe sandbox creation now forwards CORS origin via NEMOCLAW_CORS_ORIGIN (derived from CHAT_UI_URL when non-loopback). Dashboard readiness probing was changed to first check GET /health expecting 200, then fall back to / (accepting any HTTP status) to confirm the server is listening. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/onboard.test.ts (1)
2545-2725: Consider adding one explicit-precedence test forNEMOCLAW_CORS_ORIGIN.You already cover loopback vs non-loopback auto-injection well. A small follow-up test asserting that explicit
NEMOCLAW_CORS_ORIGINoverridesCHAT_UI_URLwould lock in the precedence contract and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` around lines 2545 - 2725, Add a new test alongside the two existing cases that verifies explicit NEMOCLAW_CORS_ORIGIN in the environment takes precedence over CHAT_UI_URL: replicate the sandbox test scaffolding (reuse runner mock, childProcess.spawn hook, and createSandbox invocation) but set both process.env.CHAT_UI_URL = "https://nemoclaw0-abc123.brevlab.com" and process.env.NEMOCLAW_CORS_ORIGIN = "https://explicit-origin.example.com", run createSandbox, parse the recorded commands and assert the sandbox create command includes NEMOCLAW_CORS_ORIGIN=https://explicit-origin.example.com (and not the CHAT_UI_URL origin); reference the existing test helpers and symbols like createSandbox, NEMOCLAW_CORS_ORIGIN, CHAT_UI_URL, runner.run, and the captured commands to locate where to add the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/onboard.ts`:
- Around line 3820-3836: The CORS origin logic that sets NEMOCLAW_CORS_ORIGIN
using chatUiUrl, isLoopbackHostname, formatEnvAssignment, and envArgs only runs
on the sandbox create path and is skipped when reusing a ready sandbox, causing
allowedOrigins to drift; move or duplicate this origin-detection and envArgs
update so it runs for both the create and reuse branches (where the
ready-sandbox early returns occur) — ensure the code that computes parsed = new
URL(chatUiUrl), checks isLoopbackHostname(parsed.hostname), builds origin =
`${parsed.protocol}//${parsed.host}`, and pushes
formatEnvAssignment("NEMOCLAW_CORS_ORIGIN", origin) into envArgs is executed
before any early returns for sandbox reuse so envArgs is always rebuilt when
CHAT_UI_URL changes.
- Line 3826: Remove the direct GitHub issue URL from the inline comment(s) that
reference third-party repositories (e.g., the comment containing "See:
https://github.com/NVIDIA/NemoClaw/issues/2342" and the similar comment at the
later occurrence); either delete the URL and leave a short generic note such as
"See related upstream issue" or remove the comment entirely. Locate the
offending comment text in src/lib/onboard.ts (the comment string with "See:
https://github.com/…") and update it to comply with guidelines by removing the
third-party link, and scan nearby comments to ensure no other external
repository URLs remain.
- Around line 3959-3967: The current health probe uses runCaptureOpenshell(...)
and treats any non-empty output as success, which misinterprets curl error
output; update the probe invoked in the health check (the runCaptureOpenshell
call that sets healthMatch) to explicitly check the HTTP status code: run curl
with flags to output only the HTTP status (e.g., -s -o /dev/null -w
"%{http_code}") against `http://localhost:${effectivePort}/health`, then
consider the dashboard live only when the captured healthMatch equals "200" (or
numeric 200) instead of truthiness; keep references to runCaptureOpenshell,
healthMatch, and effectivePort when making the change.
In `@test/onboard.test.ts`:
- Around line 2616-2622: The test's env spread is inheriting
NEMOCLAW_CORS_ORIGIN from the parent environment, making CORS injection
assertions flaky; update the env object used when spawning the process (the
block that spreads ...process.env and sets HOME, PATH, NEMOCLAW_NON_INTERACTIVE)
to explicitly unset or clear NEMOCLAW_CORS_ORIGIN (e.g., set it to an empty
string or undefined) so the test solely relies on CHAT_UI_URL-driven behavior,
and apply the same change to both test cases that use that env pattern around
the spawn calls referenced in onboard.test.ts.
---
Nitpick comments:
In `@test/onboard.test.ts`:
- Around line 2545-2725: Add a new test alongside the two existing cases that
verifies explicit NEMOCLAW_CORS_ORIGIN in the environment takes precedence over
CHAT_UI_URL: replicate the sandbox test scaffolding (reuse runner mock,
childProcess.spawn hook, and createSandbox invocation) but set both
process.env.CHAT_UI_URL = "https://nemoclaw0-abc123.brevlab.com" and
process.env.NEMOCLAW_CORS_ORIGIN = "https://explicit-origin.example.com", run
createSandbox, parse the recorded commands and assert the sandbox create command
includes NEMOCLAW_CORS_ORIGIN=https://explicit-origin.example.com (and not the
CHAT_UI_URL origin); reference the existing test helpers and symbols like
createSandbox, NEMOCLAW_CORS_ORIGIN, CHAT_UI_URL, runner.run, and the captured
commands to locate where to add the new test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fc2b87b8-90b7-4206-8837-4c58d031dd60
📒 Files selected for processing (2)
src/lib/onboard.tstest/onboard.test.ts
Comment on lines
+3820
to
+3836
| // When CHAT_UI_URL points to a non-loopback address (Brev Launchable, | ||
| // remote host, custom domain), pass NEMOCLAW_CORS_ORIGIN into the sandbox | ||
| // so nemoclaw-start.sh's apply_cors_override() adds the browser's origin | ||
| // to gateway.controlUi.allowedOrigins at startup. Without this, the | ||
| // Dockerfile-baked allowedOrigins only contains http://127.0.0.1:PORT | ||
| // and the gateway rejects WebSocket/API connections from the external URL. | ||
| // See: https://github.com/NVIDIA/NemoClaw/issues/2342 | ||
| const corsOrigin = process.env.NEMOCLAW_CORS_ORIGIN; | ||
| if (corsOrigin) { | ||
| envArgs.push(formatEnvAssignment("NEMOCLAW_CORS_ORIGIN", corsOrigin)); | ||
| } else { | ||
| try { | ||
| const parsed = new URL(chatUiUrl); | ||
| if (!isLoopbackHostname(parsed.hostname)) { | ||
| const origin = `${parsed.protocol}//${parsed.host}`; | ||
| envArgs.push(formatEnvAssignment("NEMOCLAW_CORS_ORIGIN", origin)); | ||
| } |
Contributor
There was a problem hiding this comment.
Handle CORS origin drift on sandbox reuse.
This only runs on the create path. The ready-sandbox reuse branches above return before envArgs are rebuilt, so changing CHAT_UI_URL from loopback to a public origin and re-running onboard leaves the old allowedOrigins in place unless the user forces --recreate-sandbox.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/onboard.ts` around lines 3820 - 3836, The CORS origin logic that sets
NEMOCLAW_CORS_ORIGIN using chatUiUrl, isLoopbackHostname, formatEnvAssignment,
and envArgs only runs on the sandbox create path and is skipped when reusing a
ready sandbox, causing allowedOrigins to drift; move or duplicate this
origin-detection and envArgs update so it runs for both the create and reuse
branches (where the ready-sandbox early returns occur) — ensure the code that
computes parsed = new URL(chatUiUrl), checks
isLoopbackHostname(parsed.hostname), builds origin =
`${parsed.protocol}//${parsed.host}`, and pushes
formatEnvAssignment("NEMOCLAW_CORS_ORIGIN", origin) into envArgs is executed
before any early returns for sandbox reuse so envArgs is always rebuilt when
CHAT_UI_URL changes.
- Remove GitHub issue URLs from source comments (repo guidelines) - Check /health by HTTP status code (200), not truthy output — runCaptureOpenshell merges stdout/stderr so curl errors could false-positive on a truthy check - Strip inherited NEMOCLAW_CORS_ORIGIN in test env to make CORS injection tests deterministic - Update NEMOCLAW_DASHBOARD_PORT test mock for new probe pattern
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/onboard.test.ts (1)
2616-2622:⚠️ Potential issue | 🟡 MinorStrip inherited dashboard-port env for deterministic CORS tests.
Line 2617 and Line 2709 still spread
process.env. IfNEMOCLAW_DASHBOARD_PORTis set in the parent shell, these tests can probe a different port than the hardcoded18789mock and flake.🛠️ Suggested stabilization
+ const { + NEMOCLAW_CORS_ORIGIN: _ignoredCorsOrigin, + NEMOCLAW_DASHBOARD_PORT: _ignoredDashboardPort, + ...inheritedEnv + } = process.env; const result = spawnSync(process.execPath, [scriptPath], { cwd: repoRoot, encoding: "utf-8", env: { - ...process.env, + ...inheritedEnv, HOME: tmpDir, PATH: `${fakeBin}:${process.env.PATH || ""}`, NEMOCLAW_NON_INTERACTIVE: "1", - NEMOCLAW_CORS_ORIGIN: "", }, });Also applies to: 2708-2714
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` around lines 2616 - 2622, The tests spread process.env into the child env which allows an external NEMOCLAW_DASHBOARD_PORT to leak in and make CORS/port assertions flaky; in the env objects (the spawn option blocks that currently include ...process.env and set HOME, PATH, NEMOCLAW_NON_INTERACTIVE, NEMOCLAW_CORS_ORIGIN) remove the spread or explicitly unset/override NEMOCLAW_DASHBOARD_PORT (e.g., do not spread process.env or set NEMOCLAW_DASHBOARD_PORT: ""/undefined) so the child process always uses the hardcoded mock port 18789; apply the same change to both occurrences around the env blocks (the one around tmpDir/fakeBin and the second at 2708-2714) to ensure deterministic CORS tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/onboard.test.ts`:
- Around line 2616-2622: The tests spread process.env into the child env which
allows an external NEMOCLAW_DASHBOARD_PORT to leak in and make CORS/port
assertions flaky; in the env objects (the spawn option blocks that currently
include ...process.env and set HOME, PATH, NEMOCLAW_NON_INTERACTIVE,
NEMOCLAW_CORS_ORIGIN) remove the spread or explicitly unset/override
NEMOCLAW_DASHBOARD_PORT (e.g., do not spread process.env or set
NEMOCLAW_DASHBOARD_PORT: ""/undefined) so the child process always uses the
hardcoded mock port 18789; apply the same change to both occurrences around the
env blocks (the one around tmpDir/fakeBin and the second at 2708-2714) to ensure
deterministic CORS tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 10791cd1-e99e-4ca3-950d-de8662a11d97
📒 Files selected for processing (2)
src/lib/onboard.tstest/onboard.test.ts
Contributor
Author
|
Re: CodeRabbit comment on CORS origin drift on sandbox reuse — Valid concern. This PR scopes to the fresh-create path (the #2342 bug report is a first-time Brev Launchable deploy, not a reuse scenario). The reuse-path drift — where |
jyaunches
changed the title
Contributor
Author
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
3 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.
Summary
Fixes the Brev Launchable dashboard showing "Version n/a" / "Health Offline" / "Disconnected from gateway" after a successful deployment.
Two root causes addressed:
1. Dashboard readiness probe false-negatives on auth-enabled deployments
The post-create readiness check curled
/which returns HTTP 401 when device auth is enabled (standard for Brev Launchable and headless deployments). The probe treated 401 as "not ready" and timed out after 30s.Fix: Probe
/healthfirst (returns 200 unconditionally when the gateway is up). Falls back to accepting any HTTP status code (including 401) from/as proof the server is listening.2. CORS
allowedOriginsmissing the external access URLThe
openclaw.jsonbaked into the sandbox image only includeshttp://127.0.0.1:18789inallowedOrigins. When the browser loads the dashboard from a Brev public URL (e.g.,https://nemoclaw0-xxx.brevlab.com), the gateway rejects WebSocket/API connections because that origin is not allowed.Fix: Auto-detect when
CHAT_UI_URLis non-loopback and injectNEMOCLAW_CORS_ORIGINinto the sandbox env args. This causesnemoclaw-start.sh's existingapply_cors_override()to add the browser's origin toallowedOriginsat container startup. ExplicitNEMOCLAW_CORS_ORIGINenv var takes precedence if set.Changes
src/lib/onboard.ts— Health probe:/→/healthwith 401-accepting fallback. CORS: auto-injectNEMOCLAW_CORS_ORIGINfor non-loopbackCHAT_UI_URL.test/onboard.test.ts— Updated mock matchers for/healthprobe. +2 new tests: CORS origin injected for Brev URLs, NOT injected for loopback.Testing
allowedOriginscorrectly includes non-loopback origin,/healthreturns{"ok":true,"status":"live"}Brev CORS verification
With
CHAT_UI_URL=http://10.0.0.1:18789(simulating non-loopback Brev URL):Related Issues
Summary by CodeRabbit
New Features
Bug Fixes