Skip to content

feat(cli): add configurable port overrides via environment variables#1645

Merged
cv merged 54 commits intoNVIDIA:mainfrom
prekshivyas:feat/configurable-ports
Apr 13, 2026
Merged

feat(cli): add configurable port overrides via environment variables#1645
cv merged 54 commits intoNVIDIA:mainfrom
prekshivyas:feat/configurable-ports

Conversation

@prekshivyas
Copy link
Copy Markdown
Contributor

@prekshivyas prekshivyas commented Apr 8, 2026

Summary

  • Add bin/lib/ports.js as central port configuration module
  • All hardcoded service ports can now be overridden via environment variables
  • Defaults are unchanged — zero regression for existing setups
Env var Default Service
NEMOCLAW_GATEWAY_PORT 8080 OpenShell gateway
NEMOCLAW_DASHBOARD_PORT 18789 Dashboard UI
NEMOCLAW_VLLM_PORT 8000 vLLM/NIM inference
NEMOCLAW_OLLAMA_PORT 11434 Ollama inference

Ports are validated to be non-privileged (1024–65535).

Usage

export NEMOCLAW_DASHBOARD_PORT=19000
nemoclaw onboard

Related Issue

Closes #684
Based on the approach from PR #683 by @jnun — thank you for the original design.

Supersedes #683 (rebased on latest main with correct defaults).
Consider closing #703, #357 (subsumed by this).

Test plan

  • npm test passes (135 tests in cli + onboard)
  • nemoclaw onboard works with default ports (no regression)
  • Set NEMOCLAW_DASHBOARD_PORT=19000, verify dashboard uses port 19000
  • Set invalid port (e.g., NEMOCLAW_GATEWAY_PORT=abc), verify startup fails with clear error

Signed-off-by: Prekshi Vyas prekshiv@nvidia.com

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Centralized, configurable ports for dashboard, gateway, vLLM and Ollama via environment variables with validated defaults.
  • Improvements

    • Startup, health checks, diagnostics, uninstall and helper flows now honor configured ports and enforce numeric/range validation with fail-fast errors.
  • Tests

    • Added unit and end-to-end tests covering port parsing, boundaries and override scenarios; CI job added to run E2E port-override tests.
  • Chore

    • Added compatibility shim to surface built artifacts and documented the default dashboard port in the container build.

Add bin/lib/ports.js as central port configuration. All hardcoded
service ports can now be overridden via environment variables:

- NEMOCLAW_GATEWAY_PORT (default 8080)
- NEMOCLAW_DASHBOARD_PORT (default 18789)
- NEMOCLAW_VLLM_PORT (default 8000)
- NEMOCLAW_OLLAMA_PORT (default 11434)

Ports are validated to be non-privileged (1024-65535). Defaults are
unchanged so existing setups are unaffected.

Closes NVIDIA#684
Based on the approach from PR NVIDIA#683

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-Authored-By: jnun <imjasonn@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 8, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Centralized port configuration: added a ports module and runtime shim to parse/validate NEMOCLAW_* env vars; replaced hard-coded ports (8080, 18789, 8000, 11434) across TypeScript, shell scripts, tests, CI, and runtime helpers to use the new port constants.

Changes

Cohort / File(s) Summary
Port modules & runtime shim
src/lib/ports.ts, nemoclaw/src/lib/ports.ts, bin/lib/ports.js
New parsePort(envVar, fallback) and exported constants GATEWAY_PORT, DASHBOARD_PORT, VLLM_PORT, OLLAMA_PORT. bin/lib/ports.js re-exports the compiled implementation.
Core services & preflight
src/lib/dashboard.ts, src/lib/preflight.ts, src/lib/services.ts, src/nemoclaw.ts, src/lib/onboard.ts
Replaced hardcoded dashboard/gateway port literals with imported port constants for defaults, availability checks, forward targets, readiness probes and recovery logic.
Local inference & NIM
src/lib/local-inference.ts, src/lib/nim.ts
Replaced hardcoded vLLM/Ollama ports with VLLM_PORT/OLLAMA_PORT; updated health-check URLs, container port fallbacks, and function default parameter values.
Runner / plugin
nemoclaw/src/blueprint/runner.ts
Switched hardcoded dashboard forward/default port to use shared DASHBOARD_PORT.
Shell scripts & runtime helpers
scripts/nemoclaw-start.sh, scripts/debug.sh, scripts/lib/runtime.sh, uninstall.sh
Read and validate NEMOCLAW_* env vars at runtime; sanitize ports, enforce range 1024–65535, use validated values in lsof/curl/printed URLs; nemoclaw-start fails fast on invalid dashboard port.
Diagnostics & debug
src/lib/debug.ts, scripts/debug.sh
Use DASHBOARD_PORT for lsof/diagnostic captures instead of fixed 18789.
Tests & CI
src/lib/ports.test.ts, test/e2e-port-overrides.sh, .github/workflows/sandbox-images-and-e2e.yaml
Added unit tests for parsePort, an e2e containerized script exercising port overrides/rejections, and a CI job to run the e2e port-override tests.
Dockerfile comment
Dockerfile
Added comment documenting default NEMOCLAW_DASHBOARD_PORT and that it can be overridden at runtime.
Misc shim
bin/lib/ports.js
Thin CommonJS shim that forwards exports to compiled ../../dist/lib/ports.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Env as Environment
participant Ports as ports.ts / bin/lib/ports.js
participant Runtime as Shell scripts / CLI
participant TS as TypeScript services
participant Container as Services (vLLM/Ollama/Dashboard)

Env->>Ports: set NEMOCLAW_* (or unset)
Ports->>Ports: parsePort(var, fallback)\n(trim, /^\d+$/, 1024–65535)
Ports-->>Runtime: export constants\n(DASHBOARD_PORT, VLLM_PORT, ...)
Ports-->>TS: export same constants
Runtime->>Container: start services using validated ports\n(CHAT_UI_URL, PUBLIC_PORT)
TS->>Container: perform health checks using port constants\n(curl / lsof)
Container-->>TS: health responses
Container-->>Runtime: runtime failures or success

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I nudged the ports from fixed to free,
I trim and I check each env-var decree.
From gateway to vLLM I hop and ensure,
Valid numbers only — safe and secure.
Hop on, deploy — your defaults roam with me.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 34.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main objective of the PR: adding configurable port overrides via environment variables, which is the core feature being implemented across all changed files.
Linked Issues check ✅ Passed All coding requirements from issue #684 are met: environment variables (NEMOCLAW_GATEWAY_PORT, NEMOCLAW_DASHBOARD_PORT, NEMOCLAW_VLLM_PORT, NEMOCLAW_OLLAMA_PORT) are implemented with validation (1024-65535 range), sensible defaults are preserved, and runtime usage is updated across JS/TS/shell scripts.
Out of Scope Changes check ✅ Passed All changes are in scope: they either add port configuration modules, update code to use configurable ports instead of hardcoded values, add supporting tests, or document the feature. No unrelated refactoring, cosmetic changes, or tangential features are present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@wscurran wscurran added NemoClaw CLI Use this label to identify issues with the NemoClaw command-line interface (CLI). enhancement: feature Use this label to identify requests for new capabilities in NemoClaw. labels Apr 8, 2026
@brandonpelfrey brandonpelfrey self-assigned this Apr 8, 2026
@brandonpelfrey brandonpelfrey self-requested a review April 8, 2026 23:03
…aths

Extend port configurability beyond bin/lib/*.js to cover the full
runtime: TypeScript source, shell scripts, Dockerfile, and utilities.

New file:
- src/lib/ports.ts — TypeScript counterpart of bin/lib/ports.js

Updated files (13 total):
- src/lib/local-inference.ts — 15 hardcoded 8000/11434 → VLLM_PORT/OLLAMA_PORT
- src/lib/nim.ts — 6 hardcoded 8000 → VLLM_PORT
- src/lib/dashboard.ts — CONTROL_UI_PORT uses DASHBOARD_PORT
- src/lib/preflight.ts — fallback uses DASHBOARD_PORT
- src/lib/services.ts — fixed env var name (DASHBOARD_PORT → NEMOCLAW_DASHBOARD_PORT)
- src/lib/debug.ts — lsof uses DASHBOARD_PORT
- nemoclaw/src/blueprint/runner.ts — forward_ports uses DASHBOARD_PORT
- scripts/nemoclaw-start.sh — CHAT_UI_URL/PUBLIC_PORT read NEMOCLAW_DASHBOARD_PORT
- scripts/lib/runtime.sh — provider URLs read NEMOCLAW_VLLM_PORT/OLLAMA_PORT
- scripts/debug.sh, uninstall.sh — use env var with fallback
- Dockerfile — added comment noting env var override

All 1199 tests pass. Zero behavior change when env vars are unset.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-Authored-By: jnun <imjasonn@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
src/lib/debug.ts (1)

377-377: Consider updating the label to reflect the actual port.

The command now uses the configurable DASHBOARD_PORT, but the label remains hardcoded as "lsof-18789". When users override the dashboard port (e.g., to 19000), the output file will still be named lsof-18789.txt while probing port 19000, which could cause confusion in debug tarballs.

Proposed fix
-  collect(collectDir, "lsof-18789", "lsof", ["-i", `:${DASHBOARD_PORT}`]);
+  collect(collectDir, `lsof-${DASHBOARD_PORT}`, "lsof", ["-i", `:${DASHBOARD_PORT}`]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/lib/debug.ts` at line 377, The label passed to collect is hardcoded as
"lsof-18789" while the command probes the configurable DASHBOARD_PORT; update
the label to reflect the actual port (e.g., replace the literal "lsof-18789"
with a dynamic label like `lsof-${DASHBOARD_PORT}`) so collect(collectDir, ...,
"lsof-<port>", "lsof", ["-i", `:${DASHBOARD_PORT}`]) produces filenames that
match the port being inspected; refer to the collect call and the DASHBOARD_PORT
symbol to make the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@nemoclaw/src/blueprint/runner.ts`:
- Around line 25-26: Replace the current naive DASHBOARD_PORT assignment with a
local validated parser: add a parsePort function (e.g., parsePort(envVar:
string, fallback: number)) that reads process.env[envVar], trims and ensures it
matches /^\d+$/, converts to Number, checks the allowed range 1024–65535 and
throws a descriptive Error on invalid input, and then set const DASHBOARD_PORT =
parsePort("NEMOCLAW_DASHBOARD_PORT", 18789); this ensures numeric, non-negative,
in-range ports and mirrors the validation logic of the original src/lib/ports.ts
without importing it.

In `@scripts/lib/runtime.sh`:
- Around line 241-245: Add shell-side validation for NEMOCLAW_VLLM_PORT and
NEMOCLAW_OLLAMA_PORT by creating/using a validate_port function that checks the
value is a non-empty integer and within 1-65535; call validate_port for each env
var before assigning local vllm_port and ollama_port and before using them in
the provider case branches (vllm-local, ollama-local) so invalid values print a
clear error and exit non‑zero. Apply the same validation calls to the analogous
code block referenced (the other provider branch around the 253-260 region) so
both usages fail fast on bad port input.

In `@src/lib/nim.ts`:
- Line 201: The docker run maps host port Number(port) to ${VLLM_PORT} but the
container actually listens on the fixed internal port 8000, breaking mappings;
update the docker invocation in src/lib/nim.ts so the host port maps to the
container's internal port 8000 (e.g., use `${Number(port)}:8000` or replace
VLLM_PORT with a constant CONTAINER_PORT = 8000) and ensure any related health
check or URL construction that currently uses VLLM_PORT is adjusted to use the
container internal port or the same CONTAINER_PORT constant (refer to the
variables port and VLLM_PORT and the docker run template string).

---

Nitpick comments:
In `@src/lib/debug.ts`:
- Line 377: The label passed to collect is hardcoded as "lsof-18789" while the
command probes the configurable DASHBOARD_PORT; update the label to reflect the
actual port (e.g., replace the literal "lsof-18789" with a dynamic label like
`lsof-${DASHBOARD_PORT}`) so collect(collectDir, ..., "lsof-<port>", "lsof",
["-i", `:${DASHBOARD_PORT}`]) produces filenames that match the port being
inspected; refer to the collect call and the DASHBOARD_PORT symbol to make the
change.
🪄 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: Pro

Run ID: f4af93c2-0e0f-4041-a645-59ba067bf7d1

📥 Commits

Reviewing files that changed from the base of the PR and between de3fe1b and 7833aba.

📒 Files selected for processing (13)
  • Dockerfile
  • nemoclaw/src/blueprint/runner.ts
  • scripts/debug.sh
  • scripts/lib/runtime.sh
  • scripts/nemoclaw-start.sh
  • src/lib/dashboard.ts
  • src/lib/debug.ts
  • src/lib/local-inference.ts
  • src/lib/nim.ts
  • src/lib/ports.ts
  • src/lib/preflight.ts
  • src/lib/services.ts
  • uninstall.sh
✅ Files skipped from review due to trivial changes (1)
  • Dockerfile

Comment thread nemoclaw/src/blueprint/runner.ts Outdated
Comment thread scripts/lib/runtime.sh
Comment thread src/lib/nim.ts Outdated
@prekshivyas
Copy link
Copy Markdown
Contributor Author

Audit response to @bpelfrey's review

All production code paths are now wired to the configurable port constants. Here's the full scorecard against your list:

Production code paths — all fixed

Item Status
runner.ts:195,234 ✅ Uses DASHBOARD_PORT from env var
dashboard.ts:10 ✅ Imports from ports.ts
nemoclaw-start.sh:143-144 ✅ Reads NEMOCLAW_DASHBOARD_PORT with fallback
local-inference.ts (all 8000/11434) ✅ Imports VLLM_PORT, OLLAMA_PORT from ports.ts
runtime.sh (8000/11434) ✅ Reads NEMOCLAW_VLLM_PORT/NEMOCLAW_OLLAMA_PORT with fallbacks
services.ts:249 ✅ Fixed — imports DASHBOARD_PORT from ports.ts (was reading wrong env var name)
preflight.ts:509 ✅ Imports DASHBOARD_PORT from ports.ts
debug.ts:375 ✅ Uses DASHBOARD_PORT constant
debug.sh:324 ✅ Uses ${NEMOCLAW_DASHBOARD_PORT:-18789}
uninstall.sh:265 ✅ Uses ${NEMOCLAW_DASHBOARD_PORT:-18789}

Blueprint/config files — intentional defaults, not changed

Item Why left as-is
blueprint.yaml:24 (18789) Blueprint default consumed at sandbox creation. The runtime path (runner.ts) already reads DASHBOARD_PORT when building forward_ports.
blueprint.yaml:45,53,66 (8000) Internal k8s service ports (NIM/vLLM inside the cluster), not host-facing ports.
Dockerfile:60 (18789) Build-time ARG default — already overridable via --build-arg CHAT_UI_URL=....
Dockerfile:128-129 (18789) Python fallback inside build RUN — would need ARG plumbing to parameterize. Build-time only, not runtime.
k8s/nemoclaw-k8s.yaml:50,76,83 (8000) K8s deployment manifest — internal cluster ports, separate from host port configuration.

All 1199 tests pass. Setting any NEMOCLAW_*_PORT env var now propagates through the full runtime: JS CLI → TS source → shell scripts → debug/cleanup utilities.

prekshivyas and others added 4 commits April 8, 2026 17:10
Address CodeRabbit review:
- Fix NIM docker run: map host port to container internal port 8000
  (not VLLM_PORT, which is the host port). Container always listens
  on 8000 internally regardless of host port override.
- Fix docker port query: query container port 8000, not VLLM_PORT.
- Add _validate_port() helper in runtime.sh to fail fast on invalid
  NEMOCLAW_VLLM_PORT / NEMOCLAW_OLLAMA_PORT values.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-Authored-By: jnun <imjasonn@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@cv cv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Review — WARNING (3 issues to fix)

Good architectural direction — centralizing ports into validated modules is the right call. But the validation isn't applied consistently.

Required changes

1. runner.ts bypasses validation (CRITICAL):
nemoclaw/src/blueprint/runner.ts uses Number(process.env.NEMOCLAW_DASHBOARD_PORT) || 18789 — no range check, accepts privileged ports, different fallback behavior than everywhere else. Replace with an import from src/lib/ports.ts.

2. Shell scripts lack validation (MEDIUM):
uninstall.sh, debug.sh, and nemoclaw-start.sh read ${NEMOCLAW_DASHBOARD_PORT:-18789} without validation. The pgrep -f usage in uninstall.sh is particularly concerning — a value like .* would match all openshell processes. Add _validate_port calls (already defined in runtime.sh) or source a shared function.

3. No unit tests (MEDIUM):
parsePort() is the security gate for the entire feature. Needs tests for: valid range, below 1024 (rejected), above 65535 (rejected), non-numeric input, empty/undefined, whitespace-padded values.

What's good

  • parsePort() regex /^\d+$/ + 1024-65535 range check is correct in the JS paths
  • No traffic redirection risk — hostname is always hardcoded to localhost
  • Dockerfile change is comment-only, correct
  • All CI checks green

@prekshivyas
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 9, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

prekshivyas and others added 2 commits April 8, 2026 20:00
Address security review (cv):
1. runner.ts: replace unvalidated Number() || with inline
   parseDashboardPort() that checks range 1024-65535
2. Shell scripts: add validation for NEMOCLAW_DASHBOARD_PORT in
   nemoclaw-start.sh (range check with fallback), debug.sh, and
   uninstall.sh (sanitize before pgrep to prevent pattern injection)
3. Add 11 unit tests for parsePort(): valid range, below 1024,
   above 65535, non-numeric, empty, whitespace, special chars

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-Authored-By: jnun <imjasonn@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@prekshivyas prekshivyas requested a review from cv April 9, 2026 03:16
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/lib/ports.test.ts (1)

11-17: Restore pre-existing TEST_PORT instead of always deleting it.

This suite currently removes TEST_PORT unconditionally, which can leak state if the variable was set by the outer test environment. Capture and restore the original value for safer isolation.

♻️ Proposed fix
 describe("parsePort", () => {
   const ENV_KEY = "TEST_PORT";
+  let previousEnvValue: string | undefined;
 
   beforeEach(() => {
+    previousEnvValue = process.env[ENV_KEY];
     delete process.env[ENV_KEY];
   });
 
   afterEach(() => {
-    delete process.env[ENV_KEY];
+    if (previousEnvValue === undefined) {
+      delete process.env[ENV_KEY];
+    } else {
+      process.env[ENV_KEY] = previousEnvValue;
+    }
   });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/lib/ports.test.ts` around lines 11 - 17, Capture the original value of
process.env[ENV_KEY] in the test suite setup and restore it after each test
instead of unconditionally deleting it: in beforeEach save const originalEnv =
process.env[ENV_KEY] (or a suite-scoped variable) and then set/delete
process.env[ENV_KEY] for test isolation as needed; in afterEach restore
process.env[ENV_KEY] = originalEnv if it was defined or delete it if it was
undefined. Update the beforeEach/afterEach logic around ENV_KEY to reference the
saved variable so external environment values are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@nemoclaw/src/blueprint/runner.ts`:
- Around line 25-40: Add unit tests for parseDashboardPort() (and validate
behavior of DASHBOARD_PORT if needed) in the co-located test file: cover unset
env var and empty-string cases to assert default 18789, non-numeric values (e.g.
"abc") to assert it throws with message containing NEMOCLAW_DASHBOARD_PORT and
range text, numeric values below 1024 and above 65535 to assert it throws with
the same message, and valid numeric strings (e.g. "1024", "65535", " 18789 ") to
assert correct parsed numbers; ensure tests temporarily set and restore
process.env.NEMOCLAW_DASHBOARD_PORT and assert thrown Error messages include the
expected text.

In `@scripts/nemoclaw-start.sh`:
- Around line 143-156: The current validation for NEMOCLAW_DASHBOARD_PORT in
nemoclaw-start.sh silently falls back to 18789; change it to match
bin/lib/ports.js by failing fast: when NEMOCLAW_DASHBOARD_PORT (and the derived
_DASHBOARD_PORT) is non-numeric or outside 1024-65535, print a clear error to
stderr and exit 1 instead of resetting to 18789; only set CHAT_UI_URL and
PUBLIC_PORT after successful validation of _DASHBOARD_PORT so the script does
not continue with a hidden default.

---

Nitpick comments:
In `@src/lib/ports.test.ts`:
- Around line 11-17: Capture the original value of process.env[ENV_KEY] in the
test suite setup and restore it after each test instead of unconditionally
deleting it: in beforeEach save const originalEnv = process.env[ENV_KEY] (or a
suite-scoped variable) and then set/delete process.env[ENV_KEY] for test
isolation as needed; in afterEach restore process.env[ENV_KEY] = originalEnv if
it was defined or delete it if it was undefined. Update the beforeEach/afterEach
logic around ENV_KEY to reference the saved variable so external environment
values are preserved.
🪄 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: Pro

Run ID: d2ac1753-9223-4f73-81fc-23e2dc0022a8

📥 Commits

Reviewing files that changed from the base of the PR and between 3c63423 and adf8274.

📒 Files selected for processing (5)
  • nemoclaw/src/blueprint/runner.ts
  • scripts/debug.sh
  • scripts/nemoclaw-start.sh
  • src/lib/ports.test.ts
  • uninstall.sh
🚧 Files skipped from review as they are similar to previous changes (2)
  • uninstall.sh
  • scripts/debug.sh

Comment thread nemoclaw/src/blueprint/runner.ts Outdated
Comment on lines +25 to +40
/** Dashboard port — validated from NEMOCLAW_DASHBOARD_PORT env var, defaults to 18789. */
function parseDashboardPort(): number {
const raw = process.env.NEMOCLAW_DASHBOARD_PORT;
if (raw === undefined || raw === "") return 18789;
const trimmed = raw.trim();
if (!/^\d+$/.test(trimmed)) {
throw new Error(`Invalid NEMOCLAW_DASHBOARD_PORT="${raw}" — must be 1024-65535`);
}
const parsed = Number(trimmed);
if (parsed < 1024 || parsed > 65535) {
throw new Error(`Invalid NEMOCLAW_DASHBOARD_PORT="${raw}" — must be 1024-65535`);
}
return parsed;
}
const DASHBOARD_PORT = parseDashboardPort();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Co-located tests near runner.ts =="
fd -a -i 'runner*.test.ts' nemoclaw/src || true

echo "== Tests mentioning dashboard port env/default forwarding =="
rg -n --type ts -C2 'NEMOCLAW_DASHBOARD_PORT|DASHBOARD_PORT|forward_ports' --iglob '*test.ts' nemoclaw/src || true

Repository: NVIDIA/NemoClaw

Length of output: 1056


🏁 Script executed:

cat -n nemoclaw/src/blueprint/runner.test.ts | head -250

Repository: NVIDIA/NemoClaw

Length of output: 9398


🏁 Script executed:

tail -n +250 nemoclaw/src/blueprint/runner.test.ts | head -200

Repository: NVIDIA/NemoClaw

Length of output: 7369


🏁 Script executed:

rg -n 'parseDashboardPort|NEMOCLAW_DASHBOARD_PORT' nemoclaw/src/blueprint/runner.test.ts

Repository: NVIDIA/NemoClaw

Length of output: 41


🏁 Script executed:

wc -l nemoclaw/src/blueprint/runner.test.ts && echo "---" && tail -100 nemoclaw/src/blueprint/runner.test.ts

Repository: NVIDIA/NemoClaw

Length of output: 3541


🏁 Script executed:

rg -r 'parseDashboardPort|NEMOCLAW_DASHBOARD_PORT' nemoclaw/src --type ts

Repository: NVIDIA/NemoClaw

Length of output: 243


🏁 Script executed:

sed -n '1,80p' nemoclaw/src/blueprint/runner.ts && echo "---" && sed -n '200,260p' nemoclaw/src/blueprint/runner.ts

Repository: NVIDIA/NemoClaw

Length of output: 4039


🏁 Script executed:

find nemoclaw/src/blueprint -name '*.test.ts' -o -name '*.test.js'

Repository: NVIDIA/NemoClaw

Length of output: 209


🏁 Script executed:

rg -n 'parseDashboardPort|NEMOCLAW_DASHBOARD_PORT|forward_ports.*DASHBOARD' nemoclaw/src/blueprint/*.test.ts

Repository: NVIDIA/NemoClaw

Length of output: 41


🏁 Script executed:

grep -n 'const DASHBOARD_PORT' nemoclaw/src/blueprint/runner.ts && grep -n 'DASHBOARD_PORT' nemoclaw/src/blueprint/runner.ts | head -20

Repository: NVIDIA/NemoClaw

Length of output: 624


Add unit tests for parseDashboardPort() validation and environment variable handling.

The dashboard port parser lacks dedicated test coverage in the co-located test file. Existing tests verify the fallback behavior works (forward_ports defaults to [18789]), but there are no tests for the parser function itself—specifically for unset/empty defaults, invalid values (non-numeric, <1024, >65535), and error messages.

As security-sensitive sandbox initialization code, this warrants explicit unit tests per the coding guidelines.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@nemoclaw/src/blueprint/runner.ts` around lines 25 - 40, Add unit tests for
parseDashboardPort() (and validate behavior of DASHBOARD_PORT if needed) in the
co-located test file: cover unset env var and empty-string cases to assert
default 18789, non-numeric values (e.g. "abc") to assert it throws with message
containing NEMOCLAW_DASHBOARD_PORT and range text, numeric values below 1024 and
above 65535 to assert it throws with the same message, and valid numeric strings
(e.g. "1024", "65535", " 18789 ") to assert correct parsed numbers; ensure tests
temporarily set and restore process.env.NEMOCLAW_DASHBOARD_PORT and assert
thrown Error messages include the expected text.

Comment thread scripts/nemoclaw-start.sh Outdated
prekshivyas and others added 3 commits April 8, 2026 20:24
Add test/e2e-port-overrides.sh with 11 tests covering:
- Default ports unchanged when no env vars set
- NEMOCLAW_DASHBOARD_PORT overrides PUBLIC_PORT and CHAT_UI_URL
- Invalid ports (non-numeric, privileged, out of range) fall back
- Pattern injection (e.g., ".*") sanitized before pgrep
- Node.js ports.js validates and propagates all 4 port overrides
- Boundary values (1024, 65535) accepted

Runs as a separate parallel CI job (test-e2e-port-overrides) using
the production image, alongside existing sandbox and isolation tests.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-Authored-By: jnun <imjasonn@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add tests 12-13 verifying that NIM docker run maps host port to
the fixed internal container port 8000 (not VLLM_PORT), and that
docker port queries also use 8000. Catches the CodeRabbit critical
finding on nim.ts.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address CodeRabbit review:
- nemoclaw-start.sh now exits 1 on invalid NEMOCLAW_DASHBOARD_PORT
  instead of silently reverting to 18789. Matches ports.js behavior
  (fail fast, don't hide bad config).
- Trims whitespace from env var value before validation.
- Update e2e tests 3-6 to expect failure (exit 1) not fallback.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-Authored-By: jnun <imjasonn@gmail.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment thread bin/lib/ports.js
cv and others added 4 commits April 8, 2026 20:52
Address Carlos's review: bin/lib/ports.js was a full standalone
JavaScript module duplicating the logic already in src/lib/ports.ts.
Replace with a thin re-export shim (module.exports = require(dist))
following the established pattern used by all other bin/lib/ files.

Single source of truth is now src/lib/ports.ts only.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@prekshivyas
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 13, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

prekshivyas and others added 2 commits April 13, 2026 10:46
Plugin branch coverage dropped below 86% threshold because
nemoclaw/src/lib/ports.ts had 20% branch coverage. Added 9 tests
covering all parsePort() branches.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cv cv added the v0.0.15 Release target label Apr 13, 2026
@cv
Copy link
Copy Markdown
Contributor

cv commented Apr 13, 2026

Quick review update after checking the latest branch state locally.

Good progress overall — the earlier validation gaps are mostly fixed now. But I still see one blocker and one follow-up bug:

1. Blocker: NEMOCLAW_GATEWAY_PORT is parsed/tested but not actually wired into gateway startup

This PR advertises 4 configurable ports, including the gateway port, and GATEWAY_PORT is now:

  • defined in src/lib/ports.ts
  • checked in preflight at src/lib/onboard.ts:1939-1947
  • exercised in test/e2e-port-overrides.sh

But the real gateway start paths still don't use it:

  • src/lib/onboard.ts:2087const gwArgs = ["--name", GATEWAY_NAME];
  • src/lib/onboard.ts:2209runOpenshell(["gateway", "start", "--name", GATEWAY_NAME], ... )

I checked openshell gateway start --help, and it does support --port <PORT>, so this looks like missing plumbing rather than an upstream limitation.

Why this matters: with NEMOCLAW_GATEWAY_PORT=9090, this branch will parse and preflight-check 9090, but the actual gateway start/restart path still won't use it.

Recommendation: either wire GATEWAY_PORT through every gateway start/restart path and add a real runtime test for it, or drop gateway-port support from this PR.

2. Follow-up bug: default-agent runtime health checks still hardcode dashboard port 18789

src/lib/agent-runtime.ts:45-47 still returns http://127.0.0.1:18789/ for the default OpenClaw agent.

That means a custom NEMOCLAW_DASHBOARD_PORT can still confuse default-agent status/recovery paths even though the rest of the stack has mostly been updated.

Recommendation: switch that fallback to the shared DASHBOARD_PORT constant and audit remaining OpenClaw fallback defaults.

Test note

The open CodeRabbit thread on test/e2e-port-overrides.sh is directionally correct: much of that test file validates copied shell snippets / exported constants instead of driving the real runtime entrypoints. That's likely why the current CI passes despite the gateway-port gap above.

I wouldn't merge this as-is yet.

prekshivyas and others added 2 commits April 13, 2026 13:23
…ded dashboard port

GATEWAY_PORT was validated in preflight but never passed to the actual
gateway start/restart commands. Wire --port into both start paths.
Replace hardcoded 18789 fallbacks in agent-runtime and agent-defs with
the DASHBOARD_PORT constant so custom port overrides take effect.

Addresses review feedback from Carlos on PR NVIDIA#1645.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cv
Copy link
Copy Markdown
Contributor

cv commented Apr 13, 2026

Follow-up after re-checking the latest PR head (a289b173).

My earlier comment is no longer current as a merge blocker:

  • The NEMOCLAW_GATEWAY_PORT blocker is fixed. src/lib/onboard.ts now passes --port ${GATEWAY_PORT} through both gateway start paths.
  • The hardcoded dashboard-port fallback is fixed. src/lib/agent-runtime.ts and src/lib/agent-defs.ts now use DASHBOARD_PORT instead of hardcoded 18789 defaults.

So the two concrete correctness issues I called out earlier appear resolved now.

The only residual concern I still have is test depth: test/e2e-port-overrides.sh still looks more like env/module propagation coverage than a true end-to-end runtime check that the real gateway startup path honors a custom gateway port. I think that is worth improving, but at this point it reads as a coverage gap, not a blocker.

Replace reimplemented shell validation snippets in tests 2-6 and 9-10
with calls to the actual nemoclaw-start entrypoint. This ensures tests
break if the runtime validation logic changes without updating tests.

Rejection tests verify the entrypoint exits non-zero with the expected
error message. Acceptance tests verify no [SECURITY] port error appears.

Addresses Carlos's review feedback on test depth.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cv
Copy link
Copy Markdown
Contributor

cv commented Apr 13, 2026

One more follow-up after re-checking the latest head (d34a94e4).

My remaining test-depth concern is narrower now.

The updated test/e2e-port-overrides.sh is materially better: tests 1-6 and 9-10 now drive the real nemoclaw-start entrypoint instead of reimplementing the shell validation logic, so that part of my earlier criticism is addressed.

What I still do not see is a true runtime test for NEMOCLAW_GATEWAY_PORT specifically. The current coverage still appears to prove gateway-port propagation through the Node ports module, but not that the real onboard/gateway startup path actually honors a custom gateway port end-to-end.

So at this point I would treat the earlier test-depth concern as mostly resolved, with one remaining coverage gap around runtime validation of custom gateway startup on the overridden port.

Add source-level assertion that both gateway start paths (primary and
recovery) pass --port GATEWAY_PORT. This ensures a custom
NEMOCLAW_GATEWAY_PORT flows through to the actual openshell commands,
not just the Node.js ports module.

Addresses Carlos's remaining test-depth concern on PR NVIDIA#1645.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cv
Copy link
Copy Markdown
Contributor

cv commented Apr 13, 2026

Final follow-up after checking the latest head (d3ddb566).

This addresses my remaining concern well enough for review purposes.

The new test/onboard.test.ts assertion is not a full end-to-end runtime test, but it does add a targeted regression guard on the actual onboarding source so both gateway start paths must keep passing --port ${GATEWAY_PORT}:

  • the primary gwArgs start path
  • the recovery runOpenshell(["gateway", "start", ...]) path

That closes the specific plumbing gap I was still worried about. At this point I consider my earlier concerns resolved.

@cv cv merged commit 3da97a2 into NVIDIA:main Apr 13, 2026
11 checks passed
@prekshivyas prekshivyas deleted the feat/configurable-ports branch April 13, 2026 22:32
ericksoa added a commit to cheese-head/NemoClaw that referenced this pull request Apr 14, 2026
…VIDIA#1645)

## Summary

- Add `bin/lib/ports.js` as central port configuration module
- All hardcoded service ports can now be overridden via environment
variables
- Defaults are unchanged — zero regression for existing setups

| Env var | Default | Service |
|---------|---------|---------|
| `NEMOCLAW_GATEWAY_PORT` | 8080 | OpenShell gateway |
| `NEMOCLAW_DASHBOARD_PORT` | 18789 | Dashboard UI |
| `NEMOCLAW_VLLM_PORT` | 8000 | vLLM/NIM inference |
| `NEMOCLAW_OLLAMA_PORT` | 11434 | Ollama inference |

Ports are validated to be non-privileged (1024–65535).

### Usage

```bash
export NEMOCLAW_DASHBOARD_PORT=19000
nemoclaw onboard
```

## Related Issue

Closes NVIDIA#684
Based on the approach from PR NVIDIA#683 by @jnun — thank you for the original
design.

Supersedes NVIDIA#683 (rebased on latest main with correct defaults).
Consider closing NVIDIA#703, NVIDIA#357 (subsumed by this).

## Test plan

- [ ] `npm test` passes (135 tests in cli + onboard)
- [ ] `nemoclaw onboard` works with default ports (no regression)
- [ ] Set `NEMOCLAW_DASHBOARD_PORT=19000`, verify dashboard uses port
19000
- [ ] Set invalid port (e.g., `NEMOCLAW_GATEWAY_PORT=abc`), verify
startup fails with clear error

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Centralized, configurable ports for dashboard, gateway, vLLM and
Ollama via environment variables with validated defaults.

* **Improvements**
* Startup, health checks, diagnostics, uninstall and helper flows now
honor configured ports and enforce numeric/range validation with
fail-fast errors.

* **Tests**
* Added unit and end-to-end tests covering port parsing, boundaries and
override scenarios; CI job added to run E2E port-override tests.

* **Chore**
* Added compatibility shim to surface built artifacts and documented the
default dashboard port in the container build.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: jnun <imjasonn@gmail.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
cv pushed a commit that referenced this pull request Apr 14, 2026
## Summary
- Document tier-based policy selector (Restricted/Balanced/Open) in
commands, network policies, and customize-network-policy pages (from
#1753)
- Document configurable port overrides via environment variables
(`NEMOCLAW_GATEWAY_PORT`, `NEMOCLAW_DASHBOARD_PORT`,
`NEMOCLAW_VLLM_PORT`, `NEMOCLAW_OLLAMA_PORT`) (from #1645)
- Document `nemoclaw <sandbox> skill install <path>` command (from
#1845, #1856)
- Document reserved sandbox name validation — CLI command collision
check (from #1773)
- Bump doc version switcher through 0.0.15
- Remove `--dangerously-skip-permissions` from onboard usage synopsis
(docs-skip violation)
- Regenerate agent skills from updated docs

## Test plan
- [x] `make docs` builds without warnings
- [x] All pre-commit hooks pass
- [ ] Verify rendered pages in docs build output
- [ ] Cross-references resolve correctly (`policy-tiers` anchor,
`environment-variables` section)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ColinM-sys pushed a commit to ColinM-sys/NemoClaw that referenced this pull request Apr 14, 2026
…VIDIA#1645)

## Summary

- Add `bin/lib/ports.js` as central port configuration module
- All hardcoded service ports can now be overridden via environment
variables
- Defaults are unchanged — zero regression for existing setups

| Env var | Default | Service |
|---------|---------|---------|
| `NEMOCLAW_GATEWAY_PORT` | 8080 | OpenShell gateway |
| `NEMOCLAW_DASHBOARD_PORT` | 18789 | Dashboard UI |
| `NEMOCLAW_VLLM_PORT` | 8000 | vLLM/NIM inference |
| `NEMOCLAW_OLLAMA_PORT` | 11434 | Ollama inference |

Ports are validated to be non-privileged (1024–65535).

### Usage

```bash
export NEMOCLAW_DASHBOARD_PORT=19000
nemoclaw onboard
```

## Related Issue

Closes NVIDIA#684
Based on the approach from PR NVIDIA#683 by @jnun — thank you for the original
design.

Supersedes NVIDIA#683 (rebased on latest main with correct defaults).
Consider closing NVIDIA#703, NVIDIA#357 (subsumed by this).

## Test plan

- [ ] `npm test` passes (135 tests in cli + onboard)
- [ ] `nemoclaw onboard` works with default ports (no regression)
- [ ] Set `NEMOCLAW_DASHBOARD_PORT=19000`, verify dashboard uses port
19000
- [ ] Set invalid port (e.g., `NEMOCLAW_GATEWAY_PORT=abc`), verify
startup fails with clear error

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Centralized, configurable ports for dashboard, gateway, vLLM and
Ollama via environment variables with validated defaults.

* **Improvements**
* Startup, health checks, diagnostics, uninstall and helper flows now
honor configured ports and enforce numeric/range validation with
fail-fast errors.

* **Tests**
* Added unit and end-to-end tests covering port parsing, boundaries and
override scenarios; CI job added to run E2E port-override tests.

* **Chore**
* Added compatibility shim to surface built artifacts and documented the
default dashboard port in the container build.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: jnun <imjasonn@gmail.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: ColinM-sys <cmcdonough@50words.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement: feature Use this label to identify requests for new capabilities in NemoClaw. NemoClaw CLI Use this label to identify issues with the NemoClaw command-line interface (CLI). v0.0.15 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Default ports conflict with common services (8080, 8000)

5 participants