Closed
Conversation
Replace hardcoded ports (8000, 8080, 11434, 18789) with env-var-driven configuration via NEMOCLAW_*_PORT variables and .env file support. - Add central port config module (ports.js), .env loader (env.js), and port conflict diagnostics (check-ports.sh) - Update all Node.js modules, shell scripts, blueprint runner, and tests to use configurable ports instead of hardcoded values - Add port configuration reference docs and README section
Align port-configuration.md H1 with title.page frontmatter value. Apply ruff auto-format to runner.py.
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:
📝 WalkthroughWalkthroughAdds environment-driven port configuration (Dashboard, Gateway, vLLM/NIM, Ollama), a centralized ports module and .env loader, replaces hard-coded ports across code and scripts, adds a port-conflict checker, updates blueprint/runtime overrides, and adjusts docs and tests to honor runtime port overrides. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as Nemoclaw CLI
participant Env as .env loader / ports
participant Orchestrator as Blueprint/Runner
participant Sandbox as OpenShell Sandbox
participant Provider as Local Inference (vLLM/Ollama)
User->>CLI: run `nemoclaw onboard` (or start)
CLI->>Env: load `.env.local` / `.env`
Env->>CLI: provide DASHBOARD/GATEWAY/VLLM/OLLAMA ports
CLI->>Orchestrator: mutate blueprint (apply port overrides)
CLI->>Sandbox: start sandbox / forward dashboard using DASHBOARD_PORT
CLI->>Provider: start/check local provider on VLLM/OLLAMA port
Provider-->>CLI: health/status
CLI-->>User: report URLs (using configured ports) / warnings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 6
🧹 Nitpick comments (2)
uninstall.sh (1)
168-168: Consider loading.envfor consistent port detection.The pattern uses
${NEMOCLAW_DASHBOARD_PORT:-18789}but the script doesn't source.envfiles likebrev-setup.shdoes. If the user configured a custom port via.env(rather than exporting it to the shell), the uninstall script may fail to find and stop the correctopenshell forwardprocesses.💡 Suggested fix: Source .env near the top of the script
SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" + +# Load port overrides if present +[ -f "${SCRIPT_DIR}/.env" ] && set -a && . "${SCRIPT_DIR}/.env" && set +a + NEMOCLAW_STATE_DIR="${HOME}/.nemoclaw"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@uninstall.sh` at line 168, The uninstall script uses ${NEMOCLAW_DASHBOARD_PORT:-18789} but never sources environment files, so custom ports from a .env won't be picked up; update uninstall.sh to load the same .env handling used by brev-setup.sh near the top of the script (before any use of NEMOCLAW_DASHBOARD_PORT), e.g., source the project .env (or call the existing env-loading helper) so the pgrep pattern "openshell.*forward.*${NEMOCLAW_DASHBOARD_PORT:-18789}" correctly detects processes when NEMOCLAW_DASHBOARD_PORT is set in .env.bin/lib/env.js (1)
39-51: Minor edge case in quote/comment handling.If a value has quotes but also has a trailing inline comment (e.g.,
KEY='value' # comment), the quote check fails (doesn't end with'), so the inline comment is stripped but the quotes remain, resulting in'value'instead ofvalue.This is an unlikely edge case in practice, and standard
.envfiles don't typically mix quotes with inline comments this way. Consider documenting this limitation in the header comment if you want to avoid surprises.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/env.js` around lines 39 - 51, The parsing currently checks surrounding quotes before stripping inline comments, causing values like KEY='value' # comment to keep quotes; change the logic in the value handling block so you remove inline comments from value first (find the " #" hashIndex and slice/trim if present), then perform the surrounding-quote check and strip quotes (the existing value.startsWith/endsWith and value = value.slice(1, -1) logic). Update the code that references the variable value in this block in bin/lib/env.js so comment-stripping happens prior to the quote-stripping branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 7-8: Reorder the two environment variable entries so they are
alphabetically ordered to satisfy dotenv-linter: change the order of
NEMOCLAW_VLLM_PORT and NEMOCLAW_OLLAMA_PORT so NEMOCLAW_OLLAMA_PORT appears
before NEMOCLAW_VLLM_PORT in the .env.example file.
In `@bin/lib/onboard.js`:
- Around line 708-733: The port-in-use warning for local providers is executed
after startup/selection so it falsely warns when the process you just started is
the expected listener; move the checkPortAvailable(VLLM_PORT) and
checkPortAvailable(OLLAMA_PORT) probe earlier (before invoking
nim.startNimContainer() / ollama serve or before marking provider as selected)
or suppress the message when provider matches the listener you just
started/adopted by checking provider (e.g., provider === "vllm-local" or
"ollama-local") and whether you started/adopted that service; update the logic
around provider, checkPortAvailable, VLLM_PORT and OLLAMA_PORT so the probe runs
pre-start or the warning is gated by “not started-by-us”/“not adopted” state to
avoid spurious warnings.
In `@docs/reference/port-configuration.md`:
- Around line 89-99: The fenced code block example showing "Checking NemoClaw
ports..." is missing a language tag; update that specific fenced block in
docs/reference/port-configuration.md by adding a language identifier (e.g.,
"text") after the opening triple backticks so the block becomes ```text ... ```
to comply with the documentation style guide.
In `@docs/reference/troubleshooting.md`:
- Line 93: Split the single source line that currently reads "NemoClaw uses four
ports (see the [Port Configuration](../../README.md#port-configuration) section
in the README). If another process is bound to one of these ports, onboarding
fails with a message identifying the conflicting process." into two separate
source lines so each sentence is on its own line: one line for "NemoClaw uses
four ports (see the [Port Configuration](../../README.md#port-configuration)
section in the README)." and a second line for "If another process is bound to
one of these ports, onboarding fails with a message identifying the conflicting
process."
In `@README.md`:
- Around line 184-191: Remove the blank line separating the two blockquotes so
the blockquote markers are contiguous (merge the "**Note**" and "**Network
exposure**" lines into a single continuous blockquote), or alternatively replace
the blank line with a horizontal rule (---) and stop using blockquote for the
second section; update the two headings ("**Note**" and "**Network exposure**")
accordingly in README.md so the markdown blockquote continuity is preserved.
In `@scripts/check-ports.sh`:
- Around line 45-48: Validate each port variable (DASHBOARD_PORT, GATEWAY_PORT,
VLLM_PORT, OLLAMA_PORT) before running the lsof availability checks: ensure the
value is an integer and within the 1024–65535 range (reject "0", non-numeric
like "abc", and >65535), and verify the probe tool (lsof) exists before using
it; if any port is invalid or lsof is missing, print an error and exit non‑zero
immediately instead of falling through to the "ok" branch so the script cannot
report "All ports available." for bad overrides.
---
Nitpick comments:
In `@bin/lib/env.js`:
- Around line 39-51: The parsing currently checks surrounding quotes before
stripping inline comments, causing values like KEY='value' # comment to keep
quotes; change the logic in the value handling block so you remove inline
comments from value first (find the " #" hashIndex and slice/trim if present),
then perform the surrounding-quote check and strip quotes (the existing
value.startsWith/endsWith and value = value.slice(1, -1) logic). Update the code
that references the variable value in this block in bin/lib/env.js so
comment-stripping happens prior to the quote-stripping branch.
In `@uninstall.sh`:
- Line 168: The uninstall script uses ${NEMOCLAW_DASHBOARD_PORT:-18789} but
never sources environment files, so custom ports from a .env won't be picked up;
update uninstall.sh to load the same .env handling used by brev-setup.sh near
the top of the script (before any use of NEMOCLAW_DASHBOARD_PORT), e.g., source
the project .env (or call the existing env-loading helper) so the pgrep pattern
"openshell.*forward.*${NEMOCLAW_DASHBOARD_PORT:-18789}" correctly detects
processes when NEMOCLAW_DASHBOARD_PORT is set in .env.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 514afe32-614b-486c-8449-26b47d825aa6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.env.example.gitignoreREADME.mdbin/lib/env.jsbin/lib/local-inference.jsbin/lib/nim.jsbin/lib/onboard.jsbin/lib/ports.jsbin/lib/preflight.jsbin/nemoclaw.jsdocs/reference/port-configuration.mddocs/reference/troubleshooting.mdnemoclaw-blueprint/blueprint.yamlnemoclaw-blueprint/orchestrator/runner.pyscripts/brev-setup.shscripts/check-ports.shscripts/debug.shscripts/lib/runtime.shscripts/nemoclaw-start.shscripts/setup.shscripts/start-services.shtest/e2e/test-double-onboard.shtest/local-inference.test.jstest/onboard-selection.test.jstest/preflight.test.jstest/runtime-shell.test.jsuninstall.sh
Remove empty line for stylecheck. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
) * security: fix command injection across all CLI entry points Comprehensive fix for shell injection vulnerabilities where user input (instance names, sandbox names, model names, API keys) was interpolated unsanitized into shell commands via run()/runInteractive()/execSync(). Changes: - Add shellQuote() and validateName() to runner.js as shared utilities - Replace all execSync() with execFileSync() in deploy (no shell) - Apply shellQuote() to every user-controlled variable in shell commands across nemoclaw.js, onboard.js, nim.js, policies.js - Add RFC 1123 name validation at CLI dispatch for sandbox/instance names - Fix path traversal in policies.js loadPreset() - Replace predictable temp file with mkdtempSync() - Remove duplicate shellQuote() definitions (now single source in runner.js) - 9 new test cases for shellQuote, validateName, and path traversal Supersedes NVIDIA#55, NVIDIA#110, NVIDIA#475, NVIDIA#540, NVIDIA#48, NVIDIA#171. * fix: deduplicate shellQuote in local-inference.js Import shellQuote from runner.js instead of defining a local copy. Single source of truth for shell quoting across the codebase. * security: fix telegram bridge injection + add regression guards Telegram bridge: - Replace execSync with execFileSync for ssh-config retrieval - shellQuote message, API key, and session ID in remote command - Validate SANDBOX_NAME at startup - Use mkdtempSync for temp SSH config (not predictable path) Regression tests: - nemoclaw.js must not use execSync - Single shellQuote definition in bin/ - CLI rejects malicious sandbox names (e2e, no mocking) - telegram-bridge.js validates SANDBOX_NAME and avoids execSync * security: address CodeRabbit review findings on shell injection PR - Shell-quote secret values written to .env before remote source - Wrap scp upload in try/finally to guarantee temp secret cleanup - Shell-quote CHAT_UI_URL and NVIDIA_API_KEY env args in onboard - Replace predictable Date.now() temp path with mkdtempSync in policies - Strengthen e2e test with canary file to prove payload never executes - Fix merge-introduced test expectations for shellQuote single-quote format
* docs: add community feedback invitation for policy presets * docs: link baseline policy reference to the YAML file on GitHub * docs: apply suggestion from @naderkhalil --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
* docs: rename strict policy tier to default * docs: remove remaining "strict" language from docs and comments
Add missing docstrings to runner.py (log, progress, emit_run_id, load_blueprint, main) to meet the 80% coverage threshold. Normalize README blockquote headings to match existing emoji + bold style. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bin/lib/local-inference.js (1)
33-42: Docker command constructs port values safely but image reference is unquoted.The
CONTAINER_REACHABILITY_IMAGEconstant (curlimages/curl:8.10.1) contains no shell metacharacters, but if this value ever becomes configurable via environment, it could introduce injection. Consider quoting for defense-in-depth.🛡️ Optional: quote image reference for future-proofing
- return `docker run --rm --add-host host.openshell.internal:host-gateway ${CONTAINER_REACHABILITY_IMAGE} -sf http://host.openshell.internal:${VLLM_PORT}/v1/models 2>/dev/null`; + return `docker run --rm --add-host host.openshell.internal:host-gateway ${shellQuote(CONTAINER_REACHABILITY_IMAGE)} -sf http://host.openshell.internal:${VLLM_PORT}/v1/models 2>/dev/null`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/local-inference.js` around lines 33 - 42, The docker image identifier in getLocalProviderContainerReachabilityCheck is inserted unquoted into the shell command, so update the template strings for the "vllm-local" and "ollama-local" cases to quote CONTAINER_REACHABILITY_IMAGE (e.g., wrap it in single quotes inside the template) to prevent possible shell injection if that constant becomes configurable; keep the rest of the command intact (including VLLM_PORT and OLLAMA_PORT) and ensure the final string still produces a valid docker run invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/telegram-bridge.js`:
- Around line 20-23: Remove the unused crypto import in
scripts/telegram-bridge.js: delete the "const crypto = require('crypto');" line
since crypto is not referenced anywhere, while keeping the secure execFileSync
usage and validateName checks intact; scan the file for any leftover references
to crypto (none expected) and run the linter/tests to ensure no breakage after
removing that import.
---
Nitpick comments:
In `@bin/lib/local-inference.js`:
- Around line 33-42: The docker image identifier in
getLocalProviderContainerReachabilityCheck is inserted unquoted into the shell
command, so update the template strings for the "vllm-local" and "ollama-local"
cases to quote CONTAINER_REACHABILITY_IMAGE (e.g., wrap it in single quotes
inside the template) to prevent possible shell injection if that constant
becomes configurable; keep the rest of the command intact (including VLLM_PORT
and OLLAMA_PORT) and ensure the final string still produces a valid docker run
invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e62b08e3-5384-4a4e-ae3d-0c5899498c25
📒 Files selected for processing (18)
README.mdbin/lib/local-inference.jsbin/lib/nim.jsbin/lib/onboard.jsbin/lib/policies.jsbin/lib/runner.jsbin/nemoclaw.jsdocs/about/how-it-works.mddocs/about/overview.mddocs/reference/architecture.mddocs/reference/network-policies.mdnemoclaw-blueprint/orchestrator/runner.pynemoclaw-blueprint/policies/openclaw-sandbox.yamlscripts/telegram-bridge.jsscripts/walkthrough.shtest/onboard-readiness.test.jstest/policies.test.jstest/runner.test.js
✅ Files skipped from review due to trivial changes (7)
- docs/about/overview.md
- scripts/walkthrough.sh
- docs/reference/architecture.md
- docs/about/how-it-works.md
- test/onboard-readiness.test.js
- nemoclaw-blueprint/policies/openclaw-sandbox.yaml
- docs/reference/network-policies.md
🚧 Files skipped from review as they are similar to previous changes (3)
- bin/nemoclaw.js
- bin/lib/nim.js
- README.md
Co-Authored-By: Claude Opus 4.6 (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 `@scripts/telegram-bridge.js`:
- Around line 104-108: The cmd string is still embedding sensitive values
(API_KEY and message) into argv/env; change the implementation so
safeSessionId/sessionId, API_KEY and message are sent over stdin (or another
non-argv channel) instead of building them into cmd. Concretely: stop exporting
NVIDIA_API_KEY and stop passing -m/message in the constructed cmd (the symbol
cmd), write the API key and message to the child/ssh process stdin (or use a
here-doc) and update the remote entrypoint (nemoclaw-start openclaw agent
--agent main --local --session-id ...) to read the key and message from stdin;
keep safeSessionId only for generating the session-id value passed in a
sanitized form but pass the actual session data via stdin rather than using
shellQuote on API_KEY or message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b141bcb-7007-4b17-9805-c7a1f55d0844
📒 Files selected for processing (1)
scripts/telegram-bridge.js
Comment on lines
+104
to
+108
| // Pass message and API key via stdin to avoid shell interpolation. | ||
| // The remote command reads them from environment/stdin rather than | ||
| // embedding user content in a shell string. | ||
| const safeSessionId = String(sessionId).replace(/[^a-zA-Z0-9-]/g, ""); | ||
| const cmd = `export NVIDIA_API_KEY=${shellQuote(API_KEY)} && nemoclaw-start openclaw agent --agent main --local -m ${shellQuote(message)} --session-id ${shellQuote("tg-" + safeSessionId)}`; |
Contributor
There was a problem hiding this comment.
Keep the API key and Telegram message out of argv.
The comment says stdin, but Line 108 still embeds both values into cmd. shellQuote helps with injection, but it does not stop NVIDIA_API_KEY from showing up in the local ssh process arguments or the user message from landing in local/remote argv via ps//proc on shared hosts. Please move both values to stdin or another non-argv channel and have the remote side read them there instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/telegram-bridge.js` around lines 104 - 108, The cmd string is still
embedding sensitive values (API_KEY and message) into argv/env; change the
implementation so safeSessionId/sessionId, API_KEY and message are sent over
stdin (or another non-argv channel) instead of building them into cmd.
Concretely: stop exporting NVIDIA_API_KEY and stop passing -m/message in the
constructed cmd (the symbol cmd), write the API key and message to the child/ssh
process stdin (or use a here-doc) and update the remote entrypoint
(nemoclaw-start openclaw agent --agent main --local --session-id ...) to read
the key and message from stdin; keep safeSessionId only for generating the
session-id value passed in a sanitized form but pass the actual session data via
stdin rather than using shellQuote on API_KEY or message.
…oad env in uninstall Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
scripts/check-ports.sh (1)
105-110:⚠️ Potential issue | 🟠 MajorValidate custom CLI ports before
lsofchecks.Line 109 calls
check_port "$p"withoutvalidate_port, so invalid args likeabc/0/70000can still printokand end with a misleading success summary.Proposed fix
if [[ $# -gt 0 ]]; then echo "" echo "Custom ports:" for p in "$@"; do - check_port "$p" || true + validate_port "CUSTOM_PORT" "$p" + check_port "$p" || true done fi#!/usr/bin/env bash # Verify custom-port path does not validate before check_port. rg -n -C2 'for p in "\$@"|validate_port "CUSTOM_PORT"|check_port "\$p"' scripts/check-ports.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-ports.sh` around lines 105 - 110, The loop handling custom CLI ports calls check_port("$p") without first validating the value, allowing invalid inputs like "abc", "0", or "70000" to be treated as OK; update the for-loop to call validate_port (the same validation used elsewhere, e.g., validate_port "CUSTOM_PORT") for each p before invoking check_port, and skip or report invalid ports instead of passing them to check_port so the summary and lsof checks only reflect validated numeric ports.
🧹 Nitpick comments (1)
uninstall.sh (1)
29-49: Consider centralizing.envparsing to avoid drift.This
_load_env()logic is now duplicated across scripts. A shared helper would reduce subtle behavior divergence over time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@uninstall.sh` around lines 29 - 49, Duplicate _load_env() parsing should be centralized: extract the _load_env implementation into a single shared helper (e.g., env helper script) and have uninstall.sh (and other scripts that currently implement _load_env) source that helper instead of duplicating logic; preserve current semantics of _load_env (trim whitespace, strip comments, handle quoted values, only export when the variable is unset) and keep calls like _load_env "$SCRIPT_DIR/.env.local" and _load_env "$SCRIPT_DIR/.env" so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/check-ports.sh`:
- Line 58: The numeric check fails for leading-zero ports because Bash treats
numbers with a leading zero as octal; update the arithmetic conditional that
uses the variable "value" (the (( value < 1024 || value > 65535 )) check) to
force base-10 interpretation by prefixing the variable with the base indicator
inside the arithmetic context (i.e., use the 10# prefix for value) so
leading-zero strings are parsed as decimal; alternatively, normalize value to a
decimal integer before the comparison (e.g., strip leading zeros or reassign
using printf -v) and then perform the same range check.
In `@uninstall.sh`:
- Line 191: Resolve the dashboard port using the same fallback chain as
scripts/check-ports.sh (check NEMOCLAW_DASHBOARD_PORT, then DASHBOARD_PORT, then
PUBLIC_PORT, then default 18789), validate that the resulting value is an
integer in range 1024–65535, and only then interpolate it into the pgrep -f
pattern; if validation fails, fall back to the default port. Replace the direct
use of ${NEMOCLAW_DASHBOARD_PORT:-18789} in the pgrep invocation with the
sanitized variable (e.g., RESOLVED_DASHBOARD_PORT) so no raw env containing
special regex chars is used in the pgrep pattern
"openshell.*forward.*${RESOLVED_DASHBOARD_PORT}".
---
Duplicate comments:
In `@scripts/check-ports.sh`:
- Around line 105-110: The loop handling custom CLI ports calls check_port("$p")
without first validating the value, allowing invalid inputs like "abc", "0", or
"70000" to be treated as OK; update the for-loop to call validate_port (the same
validation used elsewhere, e.g., validate_port "CUSTOM_PORT") for each p before
invoking check_port, and skip or report invalid ports instead of passing them to
check_port so the summary and lsof checks only reflect validated numeric ports.
---
Nitpick comments:
In `@uninstall.sh`:
- Around line 29-49: Duplicate _load_env() parsing should be centralized:
extract the _load_env implementation into a single shared helper (e.g., env
helper script) and have uninstall.sh (and other scripts that currently implement
_load_env) source that helper instead of duplicating logic; preserve current
semantics of _load_env (trim whitespace, strip comments, handle quoted values,
only export when the variable is unset) and keep calls like _load_env
"$SCRIPT_DIR/.env.local" and _load_env "$SCRIPT_DIR/.env" so behavior remains
identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6d50149-3a6c-4132-afea-e8ccdde50f7b
📒 Files selected for processing (6)
bin/lib/env.jsbin/lib/onboard.jsdocs/reference/port-configuration.mddocs/reference/troubleshooting.mdscripts/check-ports.shuninstall.sh
✅ Files skipped from review due to trivial changes (3)
- docs/reference/troubleshooting.md
- bin/lib/env.js
- docs/reference/port-configuration.md
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/lib/onboard.js
| echo "Error: $name is not a valid integer: '$value'" >&2 | ||
| exit 1 | ||
| fi | ||
| if (( value < 1024 || value > 65535 )); then |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
# Reproduce Bash numeric parsing behavior without running repo scripts.
bash -lc 'v=08080; (( v < 65535 )); echo "exit=$?"'Repository: NVIDIA/NemoClaw
Length of output: 256
🏁 Script executed:
cat -n scripts/check-ports.sh | head -70Repository: NVIDIA/NemoClaw
Length of output: 2949
Use base-10 arithmetic when validating numeric strings.
Line 58 uses (( value < ... )), which in Bash interprets leading-zero values as octal. This will cause an error for port numbers like 08080 (e.g., bash: ((: 08080: value too great for base)). Although the regex on line 54 requires all digits, it accepts leading zeros, allowing such values to reach line 58 where they fail in arithmetic context.
Proposed fix
- if (( value < 1024 || value > 65535 )); then
+ if (( 10#$value < 1024 || 10#$value > 65535 )); then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (( value < 1024 || value > 65535 )); then | |
| if (( 10#$value < 1024 || 10#$value > 65535 )); then |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/check-ports.sh` at line 58, The numeric check fails for leading-zero
ports because Bash treats numbers with a leading zero as octal; update the
arithmetic conditional that uses the variable "value" (the (( value < 1024 ||
value > 65535 )) check) to force base-10 interpretation by prefixing the
variable with the base indicator inside the arithmetic context (i.e., use the
10# prefix for value) so leading-zero strings are parsed as decimal;
alternatively, normalize value to a decimal integer before the comparison (e.g.,
strip leading zeros or reassign using printf -v) and then perform the same range
check.
| [ -n "$pid" ] || continue | ||
| pids+=("$pid") | ||
| done < <(pgrep -f 'openshell.*forward.*18789' 2>/dev/null || true) | ||
| done < <(pgrep -f "openshell.*forward.*${NEMOCLAW_DASHBOARD_PORT:-18789}" 2>/dev/null || true) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
# Compare dashboard port resolution/usage between scripts.
rg -n -C2 'NEMOCLAW_DASHBOARD_PORT|DASHBOARD_PORT|PUBLIC_PORT|pgrep -f "openshell\.\*forward' uninstall.sh scripts/check-ports.shRepository: NVIDIA/NemoClaw
Length of output: 1254
🏁 Script executed:
# Extract the validate_port function to understand validation approach
rg -A 15 'validate_port\s*\(' scripts/check-ports.sh | head -30Repository: NVIDIA/NemoClaw
Length of output: 501
🏁 Script executed:
# Check the full uninstall.sh context around line 191
sed -n '185,200p' uninstall.shRepository: NVIDIA/NemoClaw
Length of output: 533
🏁 Script executed:
# Search for other pgrep patterns or uses of port variables in regexes
rg 'pgrep|NEMOCLAW_DASHBOARD_PORT|DASHBOARD_PORT' uninstall.shRepository: NVIDIA/NemoClaw
Length of output: 280
Sanitize and resolve dashboard port before building the pgrep -f pattern.
Line 191 interpolates raw env into a regex without validation, allowing special regex characters to overmatch and target unintended processes. It also skips the fallback chain (DASHBOARD_PORT / PUBLIC_PORT) used in scripts/check-ports.sh line 45. Validate the port (numeric, range 1024–65535) and use the consistent fallback chain before interpolating into the pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@uninstall.sh` at line 191, Resolve the dashboard port using the same fallback
chain as scripts/check-ports.sh (check NEMOCLAW_DASHBOARD_PORT, then
DASHBOARD_PORT, then PUBLIC_PORT, then default 18789), validate that the
resulting value is an integer in range 1024–65535, and only then interpolate it
into the pgrep -f pattern; if validation fails, fall back to the default port.
Replace the direct use of ${NEMOCLAW_DASHBOARD_PORT:-18789} in the pgrep
invocation with the sanitized variable (e.g., RESOLVED_DASHBOARD_PORT) so no raw
env containing special regex chars is used in the pgrep pattern
"openshell.*forward.*${RESOLVED_DASHBOARD_PORT}".
The env loader only searched relative to __dirname, which resolves to the sandbox source directory where gitignored .env.local is absent. Now also checks the git repo root and CWD so user overrides are found without manually copying .env.local into .nemoclaw/source/. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
On a fresh clone neither .env nor .env.local exist (both gitignored), so ports.js falls back to hardcoded defaults. If the default port is occupied the preflight error gave no hint about .env.local overrides. - Copy .env.example → .env during preflight when .env is missing - Add .env.local override instructions to the port conflict message Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
11 tasks
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
2 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
NemoClaw's four network ports are hardcoded throughout the codebase, making it
impossible to run alongside services that bind the same ports.
This PR centralizes port configuration behind
NEMOCLAW_*_PORTenvironmentvariables with
.envfile support so users can resolve conflicts without patchingsource.
Changes
bin/lib/ports.js) that parses and validates envvars (range 1024–65535)
.env/.env.localloader (bin/lib/env.js) that runs beforeany module reads
process.envonboard.js,nemoclaw.js,preflight.js,local-inference.js,nim.js)setup.sh,start-services.sh,brev-setup.sh,nemoclaw-start.sh,runtime.sh,debug.sh,uninstall.sh)_apply_port_overrides()to blueprint runner (runner.py) to patch YAML atruntime from env vars
.env.exampletemplate andscripts/check-ports.shport conflict diagnosticdocs/reference/port-configuration.md)README.mdwith defaults and conflict risktable
docs/reference/troubleshooting.mdports.jsvalues instead of hardcoded literalsType of Change
Testing
npm testpasses (174/174).make checkpasses. TypeScript lint, format, and type check pass. Python lintand type check pass. Python format applied via
ruff format.Checklist
General
Code Changes
make formatapplied (TypeScript via Prettier, Python viaruff format).defaults, new features, bug fixes that contradict existing docs).
Doc Changes
Summary by CodeRabbit
New Features
Tools
Documentation
Chores