fix(onboard): always inject NEMOCLAW_DASHBOARD_PORT into sandbox env#2440
Merged
Conversation
…2267) When the user sets CHAT_UI_URL with a custom port (e.g. :18790), the dashboard port forward and Dockerfile ARG are correctly configured, but NEMOCLAW_DASHBOARD_PORT was only injected into the sandbox if the host env var was explicitly set. Without it, nemoclaw-start.sh defaults _DASHBOARD_PORT to 18789 and the gateway listens on the wrong port. Always derive the effective port from chatUiUrl via getDashboardForwardPort() and inject it unconditionally. Also bump vitest timeouts on flaky tests that probe the openshell gateway — when openshell is installed but the gateway is down, the CLI and onboard helpers take longer than the default 5s to time out. Closes #2267. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe onboard flow now always derives the dashboard forward/listen port from Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client (CLI)
participant Onboard as Onboard service
participant Gateway as Gateway / Port Forwarder
participant Sandbox as Sandbox / Container
CLI->>Onboard: Run `nemoclaw onboard` with CHAT_UI_URL
Onboard->>Onboard: getDashboardForwardPort(CHAT_UI_URL) -> port
Onboard->>Gateway: request forward for computed port
Gateway-->>Onboard: forward created / active
Onboard->>Sandbox: create sandbox (env: NEMOCLAW_DASHBOARD_PORT=port, CHAT_UI_URL)
Sandbox-->>Onboard: startup logs / dashboard binds to port
Onboard->>Gateway: verify active forward for port
Gateway-->>Onboard: confirmation (active)
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 unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/cli.test.ts (1)
86-125: Align inner command timeout with the new 35s test timeout.Only the Vitest timeout was increased;
runWithEnv("start", ...)still defaults to 10s and can fail early.Suggested patch
- const r = runWithEnv("start", { + const r = runWithEnv("start", { HOME: home, PATH: `${localBin}:${process.env.PATH || ""}`, NVIDIA_API_KEY: "", TELEGRAM_BOT_TOKEN: "", - }); + }, 30000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.ts` around lines 86 - 125, The test increases Vitest's timeout to 35000ms but the helper runWithEnv("start", ...) still uses its 10s default; update the call site in the test (runWithEnv) to pass a matching timeout (e.g., runWithEnv("start", { ... }, { timeout: 35000 }) or change runWithEnv's default timeout to 35000) so the inner command timeout aligns with the top-level test timeout and prevents premature failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/cli.test.ts`:
- Around line 86-125: The test increases Vitest's timeout to 35000ms but the
helper runWithEnv("start", ...) still uses its 10s default; update the call site
in the test (runWithEnv) to pass a matching timeout (e.g., runWithEnv("start", {
... }, { timeout: 35000 }) or change runWithEnv's default timeout to 35000) so
the inner command timeout aligns with the top-level test timeout and prevents
premature failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c4442781-8c51-4af5-b19d-46a0c5644345
📒 Files selected for processing (3)
src/lib/onboard.tstest/cli.test.tstest/onboard.test.ts
Add a test that exercises the exact #2267 scenario: CHAT_UI_URL is set to a custom port but NEMOCLAW_DASHBOARD_PORT is not in the environment. Verifies the port is derived from CHAT_UI_URL and injected into sandbox create envArgs unconditionally. Also update docs (quickstart, troubleshooting, commands reference) to recommend CHAT_UI_URL for port overrides and note that NEMOCLAW_DASHBOARD_PORT is auto-derived when CHAT_UI_URL is set. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/reference/troubleshooting.md (1)
604-609: Use one sentence per line in source for the new custom-port section.Line 604-609 and Line 611-612 wrap single sentences across multiple lines. Please keep each sentence on a single line to match docs style.
As per coding guidelines: “One sentence per line in source (makes diffs readable).”
Also applies to: 611-612
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/troubleshooting.md` around lines 604 - 609, The paragraph starting with "If you ran `nemoclaw onboard` with a custom dashboard port..." and the subsequent sentence(s) in the new custom-port section are wrapped across multiple source lines; split each sentence into its own physical line so each sentence is a single line in the source (e.g., ensure the sentence about the sandbox being created with an older NemoClaw version, the gateway listening on default port 18789, and the SSH tunnel forwarding the custom port are each placed on separate lines), and do the same for the sentences currently spanning lines 611-612 so every sentence in that section occupies one line in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/get-started/quickstart.md`:
- Around line 303-306: Replace the bash code fences and bare CLI lines with
console fences and $ prompts: change the ```bash (or ```shell) fences around the
nemoclaw onboarding examples to ```console and prepend each command line (e.g.,
"nemoclaw onboard", "CHAT_UI_URL=http://127.0.0.1:19000 nemoclaw onboard", and
"NEMOCLAW_DASHBOARD_PORT=19000 nemoclaw onboard") with "$ " so the examples
follow the docs standard; apply the same replacement to the other affected block
at the noted nearby lines.
In `@test/onboard.test.ts`:
- Around line 2833-2835: Remove the unnecessary ESLint suppression comment above
the environment destructuring; specifically delete the line "//
eslint-disable-next-line `@typescript-eslint/no-unused-vars`" that precedes the
destructuring of process.env into _stripped, _stripped2, and inheritedEnv. The
variables _stripped and _stripped2 are already prefixed with underscores to
indicate intentional unused status, so just remove the suppression comment in
the test file where the destructuring occurs to rely on existing project lint
rules.
---
Nitpick comments:
In `@docs/reference/troubleshooting.md`:
- Around line 604-609: The paragraph starting with "If you ran `nemoclaw
onboard` with a custom dashboard port..." and the subsequent sentence(s) in the
new custom-port section are wrapped across multiple source lines; split each
sentence into its own physical line so each sentence is a single line in the
source (e.g., ensure the sentence about the sandbox being created with an older
NemoClaw version, the gateway listening on default port 18789, and the SSH
tunnel forwarding the custom port are each placed on separate lines), and do the
same for the sentences currently spanning lines 611-612 so every sentence in
that section occupies one line in the file.
🪄 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: abebb5b0-5d61-4019-a91c-f4f6ac96a558
📒 Files selected for processing (7)
.agents/skills/nemoclaw-user-get-started/SKILL.md.agents/skills/nemoclaw-user-reference/references/commands.md.agents/skills/nemoclaw-user-reference/references/troubleshooting.mddocs/get-started/quickstart.mddocs/reference/commands.mddocs/reference/troubleshooting.mdtest/onboard.test.ts
✅ Files skipped from review due to trivial changes (2)
- .agents/skills/nemoclaw-user-reference/references/commands.md
- .agents/skills/nemoclaw-user-get-started/SKILL.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/onboard.test.ts (1)
2754-2756:⚠️ Potential issue | 🟡 MinorRemove the unsupported ESLint suppression on Line [2754].
_strippedand_stripped2are already intentionally-unused variables (underscore-prefixed), so this suppression is unnecessary and currently invalid.#!/bin/bash set -euo pipefail # Confirm the unsupported suppression is present in the changed test. rg -n '@typescript-eslint/no-unused-vars' test/onboard.test.ts # Inspect ESLint config files for registered rules/plugins. fd -i 'eslint.config.*' --exec rg -n 'typescript-eslint|no-unused-vars' {}Proposed fix
- // eslint-disable-next-line `@typescript-eslint/no-unused-vars` const { CHAT_UI_URL: _stripped, NEMOCLAW_DASHBOARD_PORT: _stripped2, ...inheritedEnv } = process.env;As per coding guidelines,
**/*.{js,ts,tsx}: Unused variables must be prefixed with_in JavaScript and TypeScript files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` around lines 2754 - 2756, Remove the unsupported ESLint suppression and rely on the intentional underscore-prefixed unused variables: delete the "// eslint-disable-next-line `@typescript-eslint/no-unused-vars`" before the destructuring that declares _stripped, _stripped2, and inheritedEnv so the line "const { CHAT_UI_URL: _stripped, NEMOCLAW_DASHBOARD_PORT: _stripped2, ...inheritedEnv } = process.env;" remains and satisfies the project's rule that unused vars be prefixed with an underscore.
🤖 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 4068-4074: The readiness probe is still using the old port
variable (effectivePort) instead of the computed effective dashboard port;
update the readiness probe curl/check to use the effectiveDashboardPort value
produced by getDashboardForwardPort and assigned to NEMOCLAW_DASHBOARD_PORT (see
effectiveDashboardPort, getDashboardForwardPort, formatEnvAssignment) so the
probe targets the same port forwarded into the container and avoids false
failures during startup.
- Around line 4073-4074: The call to getDashboardForwardPort(chatUiUrl) is
invalid because that function doesn't exist; either implement and export
getDashboardForwardPort in src/lib/dashboard-contract.ts or replace the call
with the existing port-resolution logic (e.g., use the resolvePort pattern used
in dashboard-contract) to derive effectiveDashboardPort from chatUiUrl before
passing it to formatEnvAssignment("NEMOCLAW_DASHBOARD_PORT",
effectiveDashboardPort); update the import/destructure near
buildChain/buildControlUiUrls if you add and export getDashboardForwardPort so
the symbol is available where effectiveDashboardPort is computed.
---
Duplicate comments:
In `@test/onboard.test.ts`:
- Around line 2754-2756: Remove the unsupported ESLint suppression and rely on
the intentional underscore-prefixed unused variables: delete the "//
eslint-disable-next-line `@typescript-eslint/no-unused-vars`" before the
destructuring that declares _stripped, _stripped2, and inheritedEnv so the line
"const { CHAT_UI_URL: _stripped, NEMOCLAW_DASHBOARD_PORT: _stripped2,
...inheritedEnv } = process.env;" remains and satisfies the project's rule that
unused vars be prefixed with an underscore.
🪄 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: a2fe6219-4ec3-4617-9215-2d783495560d
📒 Files selected for processing (2)
src/lib/onboard.tstest/onboard.test.ts
Four interrelated fixes — the first is a real runtime-breaking bug that also explains every failure in test/onboard.test.ts on CI. 1. Add `getDashboardForwardPort` to `src/lib/dashboard-contract.ts` and destructure it alongside `buildChain`/`buildControlUiUrls` in `src/lib/onboard.ts`. The PR's original change at `onboard.ts:4073` called `getDashboardForwardPort(chatUiUrl)` but the function was neither defined in-file nor imported anywhere — it only existed as a stale artefact in `dist/lib/onboard.js`. Runtime hit `ReferenceError: getDashboardForwardPort is not defined` the first time createSandbox ran — exactly what all 13 failing cases in `test/onboard.test.ts` were surfacing on CI. Implementing the function as a thin wrapper around `buildChain(...).port` keeps a single source of truth for port derivation. 2. `src/lib/onboard.ts` readiness probe now uses the derived `effectiveDashboardPort` (from CHAT_UI_URL) instead of `effectivePort` (agent.forwardPort default). With custom CHAT_UI_URL ports the old probe curled the wrong port and timed out with a misleading "Dashboard taking longer than expected to start" warning even when the gateway was healthy. 3. `docs/get-started/quickstart.md` — flipped the two CLI examples from ` ```bash` to ` ```console` with `$` prompt prefixes per repo convention. 4. `test/onboard.test.ts` — dropped the stale `eslint-disable-next-line @typescript-eslint/no-unused-vars` directive. The repo's ESLint config for test files uses plain `no-unused-vars` (already disabled for tests) and does not register the `@typescript-eslint` plugin, so the suppression was a no-op. `npm run build:cli` compiles cleanly; `npx vitest run test/onboard.test.ts` reports 135/135 passing after the rebuild (was 122/135 before). Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/nemoclaw-user-get-started/SKILL.md:
- Around line 248-252: The docs example incorrectly implies
NEMOCLAW_DASHBOARD_PORT alone controls the dashboard port; update the source
docs in docs/ so the example reflects actual runtime behavior used by
createSandbox which derives the effective port from CHAT_UI_URL (see
createSandbox and CHAT_UI_URL usage in src/lib/onboard.ts), either by showing
the compatible command using CHAT_UI_URL (e.g.
CHAT_UI_URL=http://localhost:19000 ...) or by clarifying the semantic that
NEMOCLAW_DASHBOARD_PORT only applies when CHAT_UI_URL is not set; after editing
the canonical docs regenerate the .agents/skills/nemoclaw-user-*/.md files so
the SKILL.md is updated.
🪄 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: 0245bc63-25a2-4451-92bd-27693cab1481
📒 Files selected for processing (5)
.agents/skills/nemoclaw-user-get-started/SKILL.mddocs/get-started/quickstart.mdsrc/lib/dashboard-contract.tssrc/lib/onboard.tstest/onboard.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/get-started/quickstart.md
- test/onboard.test.ts
ericksoa
previously requested changes
Apr 24, 2026
ericksoa
left a comment
Contributor
There was a problem hiding this comment.
The fix logic is correct and the test coverage is good, but CI is failing with a build error that needs to be resolved before this can merge.
Blocking: TypeScript build failure
src/lib/onboard.ts(6717,41): error TS2300: Duplicate identifier 'getDashboardForwardPort'.
src/lib/onboard.ts(6911,10): error TS2300: Duplicate identifier 'getDashboardForwardPort'.
The branch imports getDashboardForwardPort from dashboard-contract.ts at line 6717:
const { buildChain, buildControlUiUrls, getDashboardForwardPort } = dashboardContract;But getDashboardForwardPort also exists as a local function or is re-exported at line 6911 in module.exports. This is likely a stale-branch issue — PR #2398 deleted the old local getDashboardForwardPort from onboard.ts and this branch was based on a version of main that still had it. A rebase onto current main should fix this.
checks, macos-e2e, and wsl-e2e all fail for the same root cause.
Code review (assuming the build issue is fixed)
The core fix is sound:
getDashboardForwardPort(chatUiUrl)indashboard-contract.ts— thin wrapper overbuildChain().port. Clean, reuses existing logic.createSandbox— changed from conditional injection (if (process.env.NEMOCLAW_DASHBOARD_PORT)) to unconditional derivation fromchatUiUrl. This is the right fix for #2267.- Readiness probe — now uses
effectiveDashboardPortinstead ofeffectivePort, matching the port the gateway actually listens on. - Test — the #2267 regression test is thorough: spawns a child process with
CHAT_UI_URL=http://127.0.0.1:18790but noNEMOCLAW_DASHBOARD_PORT, then asserts thesandbox createcommand includesNEMOCLAW_DASHBOARD_PORT=18790.
Docs updates are correct — CHAT_UI_URL is now the recommended way to set custom ports, with NEMOCLAW_DASHBOARD_PORT as a fallback.
Minor (non-blocking)
-
Unnecessary eslint-disable in
test/onboard.test.ts:2754— the_stripped/_stripped2underscore prefix already satisfies the unused-var rule. Remove the// eslint-disable-next-linecomment. -
CodeRabbit noted the inner command timeout in the
#2164test wasn't increased to match the new 35s vitest timeout — therunWithEnvdefault is 10s and could still time out before the outer test does. Looks like the second iteration did pass30000torunWithEnv, so verify that's in the final commit.
Please rebase and re-push — happy to approve once CI is green.
An automated main-merge on the PR branch pulled in a local `function getDashboardForwardPort()` at onboard.ts:6911 as the single source of truth. My earlier commit (6f7f8a4) added a parallel definition via destructure from dashboardContract plus a helper export on dashboard-contract.ts — both now redundant and conflicting with the local function. TypeScript rejected the duplicate: src/lib/onboard.ts(6717,41): error TS2300: Duplicate identifier 'getDashboardForwardPort'. src/lib/onboard.ts(6911,10): error TS2300: Duplicate identifier 'getDashboardForwardPort'. Reverting both additions: - `src/lib/onboard.ts:6717` — drop `getDashboardForwardPort` from the `dashboardContract` destructure; keep `buildChain` and `buildControlUiUrls`. - `src/lib/dashboard-contract.ts` — drop the exported `getDashboardForwardPort` wrapper I added. The other three fixes from 6f7f8a4 (readiness-probe port, docs console fences, eslint-disable cleanup) are unchanged. `npm run build:cli` is clean; `test/onboard.test.ts` still 135/135. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@ericksoa Thanks for the thorough review. All three points addressed:
Tests pass locally: Signed-off-by: Prekshi Vyas prekshiv@nvidia.com |
CI's `TypeScript (CLI)` check reports:
test/onboard.test.ts(2903,50): error TS7006: Parameter 'entry' implicitly has an 'any' type.
test/onboard.test.ts(2913,10): error TS7006: Parameter 'entry' implicitly has an 'any' type.
The `payload.commands` predicates in the #2267 regression test were the
only untyped ones in the file — matching the `(entry: CommandEntry) => …`
pattern used by the surrounding tests (2236, 2570, 2778, 2796, 3022, …)
lets implicit-any stay off.
No runtime change.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-push hook regenerates the compiled JS from the TypeScript source. The TS was updated by a recent main-merge (PR #2422 adds the "attempted tunnel with undefined host" comment) but the JS committed lagged, blocking push on every commit. No behavioural change — this is the exact output of tsc on the current .ts source. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
The "uses npm install + npm link for a source checkout (no -g)" test at
test/install-preflight.test.ts:483 spawns the full install.sh bash
script via spawnSync. Under CI's 2-core runners and under local
resource pressure (pre-push hook running the full vitest suite in
parallel) the script can exceed vitest's default 5s timeout and block
commits on a pre-existing flake unrelated to the change being tested.
Raising to 20s mirrors the pattern used elsewhere in the suite for
spawn-heavy tests (e.g. `{ timeout: 35000 }` on the bare-unknown-name
#2164 test in test/cli.test.ts:117 and the #2267 CHAT_UI_URL-port
derivation in test/onboard.test.ts:2805). A local passing run finished
in 1.66s — 20s is generous and won't mask real regressions.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI's `TypeScript (CLI)` step failed with 8 strict-mode errors in the
Slack tokenFormat tests that landed on main earlier today:
- TS2345 on lines 5185, 5204, 5296, 5314: `JSON.parse(x.split("\n").pop())`
passes `string | undefined` where `JSON.parse` requires `string`.
- TS7006 on lines 5211, 5324, 5328, 5344: predicate parameter `c` was
implicitly `any`.
Minimal fixes, no behaviour change:
- `.pop() ?? "{}"` on the four `JSON.parse(...)` sites so `.pop()`'s
`undefined` case parses as an empty object instead of blowing up.
`assert.equal(result.status, 0)` immediately above each site already
guarantees stdout is non-empty, so the fallback is unreachable at
runtime — it's purely type-level.
- `(c: { key: string })` on the `out.saveCalls` predicates and
`(c: { name: string })` on the `MESSAGING_CHANNELS.find` predicate,
matching the shape the surrounding assertions rely on.
`npm run build:cli` + `npx tsc --noEmit -p tsconfig.cli.json` clean;
`test/onboard.test.ts` now reports 138/138.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…om-chat-ui-url-2267 Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> # Conflicts: # test/onboard.test.ts
Build issue resolved after merge from main.
ericksoa
approved these changes
Apr 24, 2026
ericksoa
left a comment
Contributor
There was a problem hiding this comment.
Build issue resolved after merge from main. The duplicate getDashboardForwardPort identifier is gone — dashboard-contract.ts already has the function on main now, and the old local definition in onboard.ts was removed by #2398.
All checks green except wsl-e2e still pending. Core fix and test unchanged from prior review — logic is correct. The additional timeout bump in install-preflight.test.ts is fine.
Previous review notes still apply as minor non-blocking items (unnecessary eslint-disable in test, inner command timeout alignment).
DemianHeyGen
pushed a commit
to DemianHeyGen/NemoClaw
that referenced
this pull request
Apr 30, 2026
…VIDIA#2440) ## Summary - When `CHAT_UI_URL` specifies a custom port, `NEMOCLAW_DASHBOARD_PORT` was only injected into the sandbox if the host env var was explicitly set, causing the gateway to listen on the wrong port - Always derive the effective port from `chatUiUrl` via `getDashboardForwardPort()` and inject it unconditionally - Bump vitest timeouts on flaky openshell gateway tests Closes NVIDIA#2267 ## Test plan - [x] All 2302 existing tests pass - [ ] Verify sandbox starts with correct `NEMOCLAW_DASHBOARD_PORT` when `CHAT_UI_URL` uses a non-default port (e.g. `:18790`) - [ ] Verify default port (`18789`) still works when `CHAT_UI_URL` is unset 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Dashboard forwarding now consistently derives and injects the dashboard port from CHAT_UI_URL (including custom ports), preventing mismatches that could block dashboard access. * **New Features** * Deterministic dashboard port derivation from CHAT_UI_URL for sandbox startup and health checks. * **Documentation** * Updated onboarding, quickstart, reference, and troubleshooting guides and examples to prefer CHAT_UI_URL with auto-derived ports. * **Tests** * Added/adjusted tests and extended timeouts to validate port injection and improve CLI/onboard reliability. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Ubuntu <ubuntu@nemoclaw-f6a691-inst-3cs11f3ytmmzubymi5nzdncnwia.us-west1-a.c.brevdevprod.internal> Co-authored-by: Aaron Erickson 🦞 <aerickson@nvidia.com>
This was referenced May 4, 2026
senthilr-nv
added a commit
to senthilr-nv/NemoClaw
that referenced
this pull request
May 4, 2026
…roxy access When CHAT_UI_URL is set to an HTTPS URL without an explicit port (as documented for Brev deployments), onboard injects the internal dashboard port (e.g. :18789) into the origin. Reverse proxies (Brev Cloudflare Tunnel, nginx, Caddy) serve on standard :443, so the browser origin doesn't carry the port — causing "origin not allowed" on the dashboard. Add the portless origin alongside the port-annotated one in allowedOrigins for non-loopback URLs. This is safe (same host) and covers both direct and proxied access paths. Prior art: PR NVIDIA#812 and PR NVIDIA#2440 fixed the CHAT_UI_URL plumbing but the port-override gap remained. Fixes NVIDIA#3000 Ref: NVIDIA#795, NVIDIA#20 Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
senthilr-nv
added a commit
to senthilr-nv/NemoClaw
that referenced
this pull request
May 4, 2026
…roxy access When CHAT_UI_URL is set to an HTTPS URL without an explicit port (as documented for Brev deployments), onboard injects the internal dashboard port (e.g. :18789) into the origin. Reverse proxies (Brev Cloudflare Tunnel, nginx, Caddy) serve on standard :443, so the browser origin doesn't carry the port — causing "origin not allowed" on the dashboard. Add the portless origin alongside the port-annotated one in allowedOrigins for non-loopback URLs. This is safe (same host) and covers both direct and proxied access paths. Prior art: PR NVIDIA#812 and PR NVIDIA#2440 fixed the CHAT_UI_URL plumbing but the port-override gap remained. Fixes NVIDIA#3000 Ref: NVIDIA#795, NVIDIA#20 Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
senthilr-nv
added a commit
to senthilr-nv/NemoClaw
that referenced
this pull request
May 4, 2026
…roxy access When CHAT_UI_URL is set to an HTTPS URL without an explicit port (as documented for Brev deployments), onboard injects the internal dashboard port (e.g. :18789) into the origin. Reverse proxies (Brev Cloudflare Tunnel, nginx, Caddy) serve on standard :443, so the browser origin doesn't carry the port — causing "origin not allowed" on the dashboard. Add the portless origin alongside the port-annotated one in allowedOrigins for non-loopback URLs. This is safe (same host) and covers both direct and proxied access paths. Prior art: PR NVIDIA#812 and PR NVIDIA#2440 fixed the CHAT_UI_URL plumbing but the port-override gap remained. Fixes NVIDIA#3000 Ref: NVIDIA#795, NVIDIA#20 Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
senthilr-nv
added a commit
to senthilr-nv/NemoClaw
that referenced
this pull request
May 4, 2026
…roxy access When CHAT_UI_URL is set to an HTTPS URL without an explicit port (as documented for Brev deployments), onboard injects the internal dashboard port (e.g. :18789) into the origin. Reverse proxies (Brev Cloudflare Tunnel, nginx, Caddy) serve on standard :443, so the browser origin doesn't carry the port — causing "origin not allowed" on the dashboard. Add the portless origin alongside the port-annotated one in allowedOrigins for non-loopback URLs. This is safe (same host) and covers both direct and proxied access paths. Prior art: PR NVIDIA#812 and PR NVIDIA#2440 fixed the CHAT_UI_URL plumbing but the port-override gap remained. Fixes NVIDIA#3000 Ref: NVIDIA#795, NVIDIA#20 Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
senthilr-nv
added a commit
to senthilr-nv/NemoClaw
that referenced
this pull request
May 4, 2026
…roxy access When CHAT_UI_URL is set to an HTTPS URL without an explicit port (as documented for Brev deployments), onboard injects the internal dashboard port (e.g. :18789) into the origin. Reverse proxies (Brev Cloudflare Tunnel, nginx, Caddy) serve on standard :443, so the browser origin doesn't carry the port — causing "origin not allowed" on the dashboard. Add the portless origin alongside the port-annotated one in allowedOrigins for non-loopback URLs. This is safe (same host) and covers both direct and proxied access paths. Prior art: PR NVIDIA#812 and PR NVIDIA#2440 fixed the CHAT_UI_URL plumbing but the port-override gap remained. Fixes NVIDIA#3000 Ref: NVIDIA#795, NVIDIA#20 Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
ericksoa
pushed a commit
that referenced
this pull request
May 4, 2026
…roxy access (#3002) ## Summary When `CHAT_UI_URL` is set to an HTTPS URL without an explicit port — as [documented](https://docs.nvidia.com/nemoclaw/latest/deployment/deploy-to-remote-gpu.html#remote-dashboard-access) for Brev deployments — onboard injects the internal dashboard port (`:18789`) into the origin via `onboard.ts:4002`. Reverse proxies (Brev Cloudflare Tunnel, nginx, Caddy, Tailscale Funnel) serve on standard `:443`, so the browser sends origin `https://host` which doesn't match `https://host:18789` in `allowedOrigins` — causing "origin not allowed" on the dashboard. **Root cause:** `generate-openclaw-config.py` builds `allowedOrigins` from the port-overridden URL only. **Fix:** Also include the portless origin for non-loopback URLs. This is safe (same host) and covers both direct and reverse-proxy access. ## Prior art PR #812 and PR #2440 fixed the `CHAT_UI_URL` plumbing but the port-override gap remained. This PR closes the remaining gap from the #795 / #20 lineage. ## Changes | File | Change | |------|--------| | `scripts/generate-openclaw-config.py` | Add portless origin to `allowedOrigins` for non-loopback URLs when a port is present | | `test/generate-openclaw-config.test.ts` | Update existing test to expect portless origin; add new test for reverse-proxy case | ## Testing - **Unit tests:** 43/43 passing (`vitest run test/generate-openclaw-config.test.ts`) - **Manual verification:** On Brev GCP (`nemoclaw-gcp`, n2-standard-4), confirmed that adding the portless origin to `allowedOrigins` resolves the CORS error when accessing `https://brev-nc-xxx.brevlab.com/chat?session=main` ## What this does NOT fix - `onboard.ts:4002` still unconditionally overrides the port. A future improvement could skip the port override when the URL is HTTPS with no explicit port, but this config-level fix is sufficient and lower-risk. ## Related - Fixes #3000 - Ref: #795 (Brev dashboard inaccessible — closed) - Ref: #20 (Remote dashboard access — closed) - Ref: PR #812 (bake CHAT_UI_URL into Docker build — merged) - Ref: PR #2440 (inject NEMOCLAW_DASHBOARD_PORT — merged) Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved origin handling: when a configured UI URL includes an explicit port and is non-loopback, the app accepts both the full origin (with port) and a portless origin; IPv6 hostnames keep bracket formatting and duplicate entries are removed. Loopback behavior is unchanged. * **Tests** * Expanded tests to cover ported and portless origins, reverse-proxy scenarios, IPv6 behavior (including loopback) and resilience to malformed port values. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
ksapru
pushed a commit
to ksapru/NemoClaw
that referenced
this pull request
May 12, 2026
…VIDIA#2440) ## Summary - When `CHAT_UI_URL` specifies a custom port, `NEMOCLAW_DASHBOARD_PORT` was only injected into the sandbox if the host env var was explicitly set, causing the gateway to listen on the wrong port - Always derive the effective port from `chatUiUrl` via `getDashboardForwardPort()` and inject it unconditionally - Bump vitest timeouts on flaky openshell gateway tests Closes NVIDIA#2267 ## Test plan - [x] All 2302 existing tests pass - [ ] Verify sandbox starts with correct `NEMOCLAW_DASHBOARD_PORT` when `CHAT_UI_URL` uses a non-default port (e.g. `:18790`) - [ ] Verify default port (`18789`) still works when `CHAT_UI_URL` is unset 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Dashboard forwarding now consistently derives and injects the dashboard port from CHAT_UI_URL (including custom ports), preventing mismatches that could block dashboard access. * **New Features** * Deterministic dashboard port derivation from CHAT_UI_URL for sandbox startup and health checks. * **Documentation** * Updated onboarding, quickstart, reference, and troubleshooting guides and examples to prefer CHAT_UI_URL with auto-derived ports. * **Tests** * Added/adjusted tests and extended timeouts to validate port injection and improve CLI/onboard reliability. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Ubuntu <ubuntu@nemoclaw-f6a691-inst-3cs11f3ytmmzubymi5nzdncnwia.us-west1-a.c.brevdevprod.internal> Co-authored-by: Aaron Erickson 🦞 <aerickson@nvidia.com>
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
CHAT_UI_URLspecifies a custom port,NEMOCLAW_DASHBOARD_PORTwas only injected into the sandbox if the host env var was explicitly set, causing the gateway to listen on the wrong portchatUiUrlviagetDashboardForwardPort()and inject it unconditionallyCloses #2267
Test plan
NEMOCLAW_DASHBOARD_PORTwhenCHAT_UI_URLuses a non-default port (e.g.:18790)18789) still works whenCHAT_UI_URLis unset🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests