feat(onboard): add configurable port overrides via environment variables#621
Closed
jnun wants to merge 15 commits intoNVIDIA:mainfrom
Closed
feat(onboard): add configurable port overrides via environment variables#621jnun wants to merge 15 commits intoNVIDIA:mainfrom
jnun wants to merge 15 commits intoNVIDIA:mainfrom
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.
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>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…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>
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>
- scripts/nemoclaw-start.sh, scripts/debug.sh: add .env/.env.local loading before port vars are used - scripts/brev-setup.sh: add missing .env.local loading - bin/lib/env.js: export parseEnvFile and findGitRoot for testing - README.md: document .env.local as the override mechanism - test/env.test.js: add tests for env parser, findGitRoot, and first-write-wins semantics Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace hardcoded 8080 with the GATEWAY_PORT constant and load env/ports modules so stale-gateway tests respect port overrides. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe changes introduce configurable network port support by adding an environment variable loader, centralizing port constants, and replacing hardcoded port values across Node.js modules, shell scripts, and Python orchestration code with environment-driven configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as nemoclaw CLI
participant EnvLoader as env.js
participant PortModule as ports.js
participant Preflight as onboard.js<br/>(preflight)
participant Gateway as Gateway Service
participant Dashboard as Dashboard<br/>(Port: DASHBOARD_PORT)
participant NIM as NIM/vLLM<br/>(Port: VLLM_PORT)
participant Ollama as Ollama<br/>(Port: OLLAMA_PORT)
User->>CLI: nemoclaw onboard
CLI->>EnvLoader: require('./lib/env')
EnvLoader->>EnvLoader: scan .git root,<br/>load .env & .env.local
EnvLoader->>EnvLoader: populate process.env<br/>(non-overwrite)
CLI->>PortModule: import DASHBOARD_PORT,<br/>GATEWAY_PORT, VLLM_PORT,<br/>OLLAMA_PORT
PortModule->>PortModule: parse env vars<br/>with fallbacks
PortModule-->>CLI: return port constants
CLI->>Preflight: preflight() check
Preflight->>Preflight: validate all 4 ports<br/>available via lsof
Preflight->>Preflight: bootstrap .env.example<br/>→ .env on first run
alt Port conflict
Preflight-->>User: error + env var guidance
else Ports OK
Preflight-->>CLI: pass
end
CLI->>Gateway: startGateway(GATEWAY_PORT)
Gateway-->>Gateway: openshell gateway start<br/>--port GATEWAY_PORT
CLI->>Dashboard: sandbox forwarding<br/>to DASHBOARD_PORT
Dashboard-->>Dashboard: listen on<br/>DASHBOARD_PORT
CLI->>NIM: local inference setup<br/>(VLLM_PORT)
NIM-->>NIM: health check<br/>localhost:VLLM_PORT
CLI->>Ollama: Ollama service<br/>(OLLAMA_PORT)
Ollama-->>Ollama: bind 0.0.0.0:OLLAMA_PORT
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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. Important Merge conflicts detected (Beta)
✨ 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: 4
🧹 Nitpick comments (5)
uninstall.sh (1)
30-49: Loading order differs from other scripts but achieves the same result.This custom
_load_envfunction loads.env.localbefore.envbut uses first-wins semantics (line 43: only set if not already defined). Other scripts in this PR use the opposite file order with last-wins semantics (set -a && . .env && . .env.local).Both approaches give
.env.localprecedence, but the inconsistent patterns could confuse future maintainers. Consider adding a comment explaining the first-wins behavior:📝 Suggested comment
+# Load .env.local first; first-wins semantics means it takes precedence over .env _load_env "$SCRIPT_DIR/.env.local" _load_env "$SCRIPT_DIR/.env"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@uninstall.sh` around lines 30 - 49, The _load_env function loads $SCRIPT_DIR/.env.local before $SCRIPT_DIR/.env and uses first-wins semantics (it only exports a variable if it's not already set), which differs from the other scripts that source .env then .env.local with last-wins behavior; add a brief inline comment above the _load_env definition explaining the load order and that first-wins is intentional (i.e., .env.local takes precedence here) and note the difference vs the other scripts so future maintainers understand the behavior and why it differs.docs/reference/troubleshooting.md (1)
93-115: Minor style suggestions.
Lines 96, 103: Colons are used before code blocks. The style guide states colons should only introduce lists. Consider rephrasing to avoid colons before code examples:
- Line 96: "To resolve, either stop the conflicting process." (remove colon)
- Line 103: "Or set the corresponding environment variable before onboarding." (remove colon)
Line 103: Missing period at the end of the sentence before the code block.
📝 Suggested changes
-To resolve, either stop the conflicting process: +To resolve, either stop the conflicting process.-Or use a different port by setting the corresponding environment variable before onboarding: +Or set the corresponding environment variable before onboarding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/troubleshooting.md` around lines 93 - 115, Update the two sentences that introduce code blocks so they follow the style guide: change "To resolve, either stop the conflicting process:" to "To resolve, either stop the conflicting process." and change "Or use a different port by setting the corresponding environment variable before onboarding:" to "Or use a different port by setting the corresponding environment variable before onboarding." also ensure the second sentence ends with a period; these lines are the ones immediately before the lsof/kill example and before the NEMOCLAW_GATEWAY_PORT export + nemoclaw onboard example (referenced ENV var NEMOCLAW_GATEWAY_PORT and the nemoclaw onboard command) so you can locate and edit them accordingly.scripts/setup.sh (1)
35-37: Consider adding shellcheck directives for consistency.Other scripts in this PR (e.g.,
brev-setup.sh,nemoclaw-start.sh) include# shellcheck source=/dev/nullbefore sourcing.envfiles. Adding the same here would maintain consistency and suppress potential shellcheck warnings.♻️ Suggested change
# Load port overrides if present (.env.local takes precedence) +# shellcheck source=/dev/null [ -f "${REPO_DIR}/.env" ] && set -a && . "${REPO_DIR}/.env" && set +a +# shellcheck source=/dev/null [ -f "${REPO_DIR}/.env.local" ] && set -a && . "${REPO_DIR}/.env.local" && set +a🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup.sh` around lines 35 - 37, Add the shellcheck source suppression comment before the lines that source environment files: insert "# shellcheck source=/dev/null" immediately above the two lines that source ".env" and ".env.local" (the lines starting with "[ -f \"${REPO_DIR}/.env\" ] && set -a && . \"${REPO_DIR}/.env\" && set +a" and the similar ".env.local" line) so shellcheck warnings are suppressed and the script matches the other scripts' pattern.bin/lib/ports.js (1)
19-24: Consider clarifying error messages for fallback chain failures.If an intermediate fallback variable (e.g.,
DASHBOARD_PORTorPUBLIC_PORT) contains an invalid value, the error will reference that variable name, which might confuse users who only setNEMOCLAW_DASHBOARD_PORT. This is a minor edge case since users typically only set one of these variables.The implementation is otherwise correct and the fallback chain matches the documented behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/ports.js` around lines 19 - 24, The parsePort fallback chain may throw errors that only name the intermediate env var (e.g., DASHBOARD_PORT or PUBLIC_PORT), which can confuse users who set NEMOCLAW_DASHBOARD_PORT; fix by surfacing the full fallback context when an error occurs: update the parsePort function to accept/propagate an optional context or chain label (or catch and rethrow errors) so errors include the original requested variable and the fallback chain (e.g., "NEMOCLAW_DASHBOARD_PORT (fallbacks: DASHBOARD_PORT → PUBLIC_PORT → 18789)"). Adjust the call that assigns DASHBOARD_PORT (the parsePort invocation that uses "NEMOCLAW_DASHBOARD_PORT" and nested parsePort calls) so it passes/propagates the chain information into parsePort so thrown errors reference the full chain instead of only the intermediate variable name.scripts/check-ports.sh (1)
105-111: Consider validating custom port arguments.Custom ports passed via CLI arguments are not validated for integer format or port range before being passed to
lsof. Whilelsofwill handle invalid input gracefully, adding validation would provide clearer error messages.♻️ Optional: Validate custom ports
if [[ $# -gt 0 ]]; then echo "" echo "Custom ports:" for p in "$@"; do + validate_port "custom" "$p" check_port "$p" || true done fi🤖 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 - 111, Validate each custom port argument before calling check_port: in the for loop that iterates "$@", ensure the value matches a numeric regex (e.g. only digits) and that it falls within the valid port range 1–65535; if a value fails, print a clear error like "Invalid port: <value> (must be integer 1-65535)" and skip or exit with non-zero as appropriate, then only call check_port "<port>" for validated ports (this prevents passing malformed values to lsof and gives clearer messages).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/env.js`:
- Around line 57-70: The inline-comment logic currently removes " #..." before
stripping quotes, which truncates quoted values like KEY="value # inside";
change the order and condition so comments are stripped only when the raw value
is not quoted: first detect if value starts and ends with matching quotes
(value.startsWith('"') && value.endsWith('"') or same for single quotes) and
when it is quoted, skip the hash-trimming and only remove surrounding quotes;
when it is not quoted, perform the existing hashIndex check (value.indexOf("
#")) and trim the comment, then trim whitespace. Locate the code handling the
variable named value in bin/lib/env.js to implement this conditional flow.
In `@README.md`:
- Around line 188-192: Add a single blank line after the blockquote that starts
with "**⚠️ Network exposure**" and before the horizontal rule ("---") to satisfy
the markdown linter (MD028); edit the README block containing that blockquote
header and ensure there is an empty line between the end of the quoted paragraph
and the "---" separator.
In `@scripts/debug.sh`:
- Line 294: The lsof diagnostic call only uses NEMOCLAW_DASHBOARD_PORT (or
18789) and misses other env fallbacks; update the collect invocation that runs
lsof -i to use the same fallback chain as elsewhere (check DASHBOARD_PORT, then
PUBLIC_PORT, then NEMOCLAW_DASHBOARD_PORT, then default 18789) so it inspects
the effective dashboard port; modify the variable expansion in the collect
"lsof-dashboard" lsof -i :"${...}" invocation accordingly (refer to the collect
call and the environment variable names DASHBOARD_PORT, PUBLIC_PORT,
NEMOCLAW_DASHBOARD_PORT).
In `@scripts/start-services.sh`:
- Line 24: The DASHBOARD_PORT assignment currently falls back only to
DASHBOARD_PORT and 18789, causing mismatch with bin/lib/ports.js which also
considers PUBLIC_PORT; update the fallback chain for the DASHBOARD_PORT variable
assignment in start-services.sh so it first checks NEMOCLAW_DASHBOARD_PORT, then
PUBLIC_PORT, then DASHBOARD_PORT, then the default 18789 (i.e., include
PUBLIC_PORT in the parameter expansion order) so the shell script and
bin/lib/ports.js resolve the same port.
---
Nitpick comments:
In `@bin/lib/ports.js`:
- Around line 19-24: The parsePort fallback chain may throw errors that only
name the intermediate env var (e.g., DASHBOARD_PORT or PUBLIC_PORT), which can
confuse users who set NEMOCLAW_DASHBOARD_PORT; fix by surfacing the full
fallback context when an error occurs: update the parsePort function to
accept/propagate an optional context or chain label (or catch and rethrow
errors) so errors include the original requested variable and the fallback chain
(e.g., "NEMOCLAW_DASHBOARD_PORT (fallbacks: DASHBOARD_PORT → PUBLIC_PORT →
18789)"). Adjust the call that assigns DASHBOARD_PORT (the parsePort invocation
that uses "NEMOCLAW_DASHBOARD_PORT" and nested parsePort calls) so it
passes/propagates the chain information into parsePort so thrown errors
reference the full chain instead of only the intermediate variable name.
In `@docs/reference/troubleshooting.md`:
- Around line 93-115: Update the two sentences that introduce code blocks so
they follow the style guide: change "To resolve, either stop the conflicting
process:" to "To resolve, either stop the conflicting process." and change "Or
use a different port by setting the corresponding environment variable before
onboarding:" to "Or use a different port by setting the corresponding
environment variable before onboarding." also ensure the second sentence ends
with a period; these lines are the ones immediately before the lsof/kill example
and before the NEMOCLAW_GATEWAY_PORT export + nemoclaw onboard example
(referenced ENV var NEMOCLAW_GATEWAY_PORT and the nemoclaw onboard command) so
you can locate and edit them accordingly.
In `@scripts/check-ports.sh`:
- Around line 105-111: Validate each custom port argument before calling
check_port: in the for loop that iterates "$@", ensure the value matches a
numeric regex (e.g. only digits) and that it falls within the valid port range
1–65535; if a value fails, print a clear error like "Invalid port: <value> (must
be integer 1-65535)" and skip or exit with non-zero as appropriate, then only
call check_port "<port>" for validated ports (this prevents passing malformed
values to lsof and gives clearer messages).
In `@scripts/setup.sh`:
- Around line 35-37: Add the shellcheck source suppression comment before the
lines that source environment files: insert "# shellcheck source=/dev/null"
immediately above the two lines that source ".env" and ".env.local" (the lines
starting with "[ -f \"${REPO_DIR}/.env\" ] && set -a && . \"${REPO_DIR}/.env\"
&& set +a" and the similar ".env.local" line) so shellcheck warnings are
suppressed and the script matches the other scripts' pattern.
In `@uninstall.sh`:
- Around line 30-49: The _load_env function loads $SCRIPT_DIR/.env.local before
$SCRIPT_DIR/.env and uses first-wins semantics (it only exports a variable if
it's not already set), which differs from the other scripts that source .env
then .env.local with last-wins behavior; add a brief inline comment above the
_load_env definition explaining the load order and that first-wins is
intentional (i.e., .env.local takes precedence here) and note the difference vs
the other scripts so future maintainers understand the behavior and why it
differs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22784033-f298-444d-b594-0be22c80c1f5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
.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.shscripts/telegram-bridge.jstest/e2e/test-double-onboard.shtest/env.test.jstest/local-inference.test.jstest/onboard-readiness.test.jstest/onboard-selection.test.jstest/preflight.test.jstest/runtime-shell.test.jsuninstall.sh
💤 Files with no reviewable changes (1)
- scripts/telegram-bridge.js
Comment on lines
+57
to
+70
| // Remove inline comments for unquoted values first, then strip quotes. | ||
| // This handles cases like KEY='value' # comment correctly. | ||
| const hashIndex = value.indexOf(" #"); | ||
| if (hashIndex !== -1) { | ||
| value = value.slice(0, hashIndex).trim(); | ||
| } | ||
|
|
||
| // Strip surrounding quotes | ||
| if ( | ||
| (value.startsWith('"') && value.endsWith('"')) || | ||
| (value.startsWith("'") && value.endsWith("'")) | ||
| ) { | ||
| value = value.slice(1, -1); | ||
| } |
Contributor
There was a problem hiding this comment.
Inline comment stripping may truncate quoted values containing #.
The current logic strips inline comments before stripping quotes. For KEY="value # inside", this results in "value (malformed). Most .env parsers preserve content within quotes.
This edge case may be acceptable if your .env files never contain # within quoted values, but it's worth documenting or handling.
🔧 Proposed fix: Strip comments only for unquoted values
let value = line.slice(eqIndex + 1).trim();
- // Remove inline comments for unquoted values first, then strip quotes.
- // This handles cases like KEY='value' # comment correctly.
- const hashIndex = value.indexOf(" #");
- if (hashIndex !== -1) {
- value = value.slice(0, hashIndex).trim();
- }
-
// Strip surrounding quotes
if (
(value.startsWith('"') && value.endsWith('"')) ||
(value.startsWith("'") && value.endsWith("'"))
) {
value = value.slice(1, -1);
+ } else {
+ // Remove inline comments only for unquoted values
+ const hashIndex = value.indexOf(" #");
+ if (hashIndex !== -1) {
+ value = value.slice(0, hashIndex).trim();
+ }
}📝 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
| // Remove inline comments for unquoted values first, then strip quotes. | |
| // This handles cases like KEY='value' # comment correctly. | |
| const hashIndex = value.indexOf(" #"); | |
| if (hashIndex !== -1) { | |
| value = value.slice(0, hashIndex).trim(); | |
| } | |
| // Strip surrounding quotes | |
| if ( | |
| (value.startsWith('"') && value.endsWith('"')) || | |
| (value.startsWith("'") && value.endsWith("'")) | |
| ) { | |
| value = value.slice(1, -1); | |
| } | |
| // Strip surrounding quotes | |
| if ( | |
| (value.startsWith('"') && value.endsWith('"')) || | |
| (value.startsWith("'") && value.endsWith("'")) | |
| ) { | |
| value = value.slice(1, -1); | |
| } else { | |
| // Remove inline comments only for unquoted values | |
| const hashIndex = value.indexOf(" #"); | |
| if (hashIndex !== -1) { | |
| value = value.slice(0, hashIndex).trim(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/env.js` around lines 57 - 70, The inline-comment logic currently
removes " #..." before stripping quotes, which truncates quoted values like
KEY="value # inside"; change the order and condition so comments are stripped
only when the raw value is not quoted: first detect if value starts and ends
with matching quotes (value.startsWith('"') && value.endsWith('"') or same for
single quotes) and when it is quoted, skip the hash-trimming and only remove
surrounding quotes; when it is not quoted, perform the existing hashIndex check
(value.indexOf(" #")) and trim the comment, then trim whitespace. Locate the
code handling the variable named value in bin/lib/env.js to implement this
conditional flow.
Comment on lines
+188
to
+192
| > **⚠️ Network exposure** | ||
| > | ||
| > When using local inference (Ollama or vLLM), the inference service binds to `0.0.0.0` so that containers can reach it via `host.openshell.internal`. This means the service is reachable from your local network, not just localhost. This is required for the sandbox architecture but should be considered in shared or untrusted network environments. | ||
| --- | ||
|
|
Contributor
There was a problem hiding this comment.
Fix markdown lint warning: missing blank line after blockquote.
The static analysis tool flagged a blank line issue inside/after the blockquote. Add a blank line between the warning block (line 190) and the horizontal rule (line 191) to fix the MD028 violation.
📝 Proposed fix
> When using local inference (Ollama or vLLM), the inference service binds to `0.0.0.0` so that containers can reach it via `host.openshell.internal`. This means the service is reachable from your local network, not just localhost. This is required for the sandbox architecture but should be considered in shared or untrusted network environments.
+
---📝 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
| > **⚠️ Network exposure** | |
| > | |
| > When using local inference (Ollama or vLLM), the inference service binds to `0.0.0.0` so that containers can reach it via `host.openshell.internal`. This means the service is reachable from your local network, not just localhost. This is required for the sandbox architecture but should be considered in shared or untrusted network environments. | |
| --- | |
| > **⚠️ Network exposure** | |
| > | |
| > When using local inference (Ollama or vLLM), the inference service binds to `0.0.0.0` so that containers can reach it via `host.openshell.internal`. This means the service is reachable from your local network, not just localhost. This is required for the sandbox architecture but should be considered in shared or untrusted network environments. | |
| --- |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 188 - 192, Add a single blank line after the
blockquote that starts with "**⚠️ Network exposure**" and before the horizontal
rule ("---") to satisfy the markdown linter (MD028); edit the README block
containing that blockquote header and ensure there is an empty line between the
end of the quoted paragraph and the "---" separator.
| collect "curl-models" sh -c 'code=$(curl -s -o /dev/null -w "%{http_code}" https://integrate.api.nvidia.com/v1/models); echo "HTTP $code"; if [ "$code" -ge 200 ] && [ "$code" -lt 500 ]; then echo "NIM API reachable"; else echo "NIM API unreachable"; exit 1; fi' | ||
| collect "lsof-net" sh -c 'lsof -i -P -n 2>/dev/null | head -50' | ||
| collect "lsof-18789" lsof -i :18789 | ||
| collect "lsof-dashboard" lsof -i :"${NEMOCLAW_DASHBOARD_PORT:-18789}" |
Contributor
There was a problem hiding this comment.
Use the same dashboard-port fallback chain for diagnostics.
Line 294 only checks NEMOCLAW_DASHBOARD_PORT (or 18789). If DASHBOARD_PORT/PUBLIC_PORT is used, the debug report can inspect the wrong port.
Proposed fix
- collect "lsof-dashboard" lsof -i :"${NEMOCLAW_DASHBOARD_PORT:-18789}"
+ dashboard_port="${NEMOCLAW_DASHBOARD_PORT:-${DASHBOARD_PORT:-${PUBLIC_PORT:-18789}}}"
+ collect "lsof-dashboard" lsof -i :"${dashboard_port}"📝 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
| collect "lsof-dashboard" lsof -i :"${NEMOCLAW_DASHBOARD_PORT:-18789}" | |
| dashboard_port="${NEMOCLAW_DASHBOARD_PORT:-${DASHBOARD_PORT:-${PUBLIC_PORT:-18789}}}" | |
| collect "lsof-dashboard" lsof -i :"${dashboard_port}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/debug.sh` at line 294, The lsof diagnostic call only uses
NEMOCLAW_DASHBOARD_PORT (or 18789) and misses other env fallbacks; update the
collect invocation that runs lsof -i to use the same fallback chain as elsewhere
(check DASHBOARD_PORT, then PUBLIC_PORT, then NEMOCLAW_DASHBOARD_PORT, then
default 18789) so it inspects the effective dashboard port; modify the variable
expansion in the collect "lsof-dashboard" lsof -i :"${...}" invocation
accordingly (refer to the collect call and the environment variable names
DASHBOARD_PORT, PUBLIC_PORT, NEMOCLAW_DASHBOARD_PORT).
| [ -f "${REPO_DIR}/.env" ] && set -a && . "${REPO_DIR}/.env" && set +a | ||
| [ -f "${REPO_DIR}/.env.local" ] && set -a && . "${REPO_DIR}/.env.local" && set +a | ||
|
|
||
| DASHBOARD_PORT="${NEMOCLAW_DASHBOARD_PORT:-${DASHBOARD_PORT:-18789}}" |
Contributor
There was a problem hiding this comment.
Align Line 24 fallback chain with bin/lib/ports.js to avoid port drift.
Line 24 currently ignores PUBLIC_PORT, but bin/lib/ports.js includes it. If only PUBLIC_PORT is configured, cloudflared may tunnel to the wrong local port.
Proposed fix
-DASHBOARD_PORT="${NEMOCLAW_DASHBOARD_PORT:-${DASHBOARD_PORT:-18789}}"
+DASHBOARD_PORT="${NEMOCLAW_DASHBOARD_PORT:-${DASHBOARD_PORT:-${PUBLIC_PORT:-18789}}}"📝 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
| DASHBOARD_PORT="${NEMOCLAW_DASHBOARD_PORT:-${DASHBOARD_PORT:-18789}}" | |
| DASHBOARD_PORT="${NEMOCLAW_DASHBOARD_PORT:-${DASHBOARD_PORT:-${PUBLIC_PORT:-18789}}}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/start-services.sh` at line 24, The DASHBOARD_PORT assignment
currently falls back only to DASHBOARD_PORT and 18789, causing mismatch with
bin/lib/ports.js which also considers PUBLIC_PORT; update the fallback chain for
the DASHBOARD_PORT variable assignment in start-services.sh so it first checks
NEMOCLAW_DASHBOARD_PORT, then PUBLIC_PORT, then DASHBOARD_PORT, then the default
18789 (i.e., include PUBLIC_PORT in the parameter expansion order) so the shell
script and bin/lib/ports.js resolve the same port.
Contributor
Author
|
Closing to improve alignment with the latest main branch. |
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
Add environment-variable-based port configuration so users on shared machines or non-standard environments can override the default gateway, NIM, and service ports without editing source files. This also fixes several onboarding bugs uncovered during implementation: stale
.envloading on fresh installs, spurious port-conflict warnings, command injection in CLI entry points, and missing env bootstrap in shell scripts.Related Issue
Supersedes #598 (closed).
Changes
bin/lib/ports.js(new) — single source of truth for all port constants, loaded from env vars with validated defaults.bin/lib/env.js(new) —.env/.env.localloader that searches both git root and CWD, with fallback behavior for fresh installs.bin/lib/onboard.js— port-conflict detection now validates against configured ports; shows override guidance when a conflict is found; env loaded at startup.bin/lib/local-inference.js,bin/lib/nim.js,bin/lib/preflight.js,bin/nemoclaw.js— consume ports fromports.jsinstead of hardcoded values.nemoclaw-blueprint/blueprint.yaml+orchestrator/runner.py— propagate port env vars into the blueprint/sandbox.scripts/brev-setup.sh,check-ports.sh(new),debug.sh,nemoclaw-start.sh,setup.sh,start-services.sh,lib/runtime.sh,uninstall.sh) — source.envbefore using ports; newcheck-ports.shhelper..env.example— documents all overridable port variables.docs/reference/port-configuration.md(new) — user-facing doc for port overrides.docs/reference/troubleshooting.md— updated with port-conflict resolution steps.scripts/telegram-bridge.js— removed unusedcryptoimport.bin/lib/onboard.js+ CLI entry points — fixed command-injection vulnerability (fix(security): command injection across all CLI entry points #584).test/env.test.js; updatedonboard-readiness,onboard-selection,local-inference,preflight,runtime-shell, and e2e tests to use configurable ports.Type of Change
Testing
make checkpasses.npm testpasses.Checklist
General
Code Changes
make formatapplied (TypeScript and Python).Doc Changes
Summary by CodeRabbit
Release Notes
New Features
.envfiles or environment variablesDocumentation
Tests