fix: configure OpenShell gateway bind address and port#3425
Merged
Conversation
ericksoa
added
enhancement: feature
ericksoa
added
enhancement: feature
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds bind-address and gateway-port parsing/validation, host-aware port probing and endpoint helpers, centralizes Docker-driver gateway env handling, integrates helpers into onboarding/readiness, normalizes subprocess envs, updates tests/harness, and updates docs. ChangesGateway port and bind-address configuration
Estimated code review effort: Possibly related PRs:
Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/onboard.test.ts (1)
430-467:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake this test hermetic against ambient gateway env vars.
At Line 451+, only
NEMOCLAW_DISABLE_OVERLAY_FIXis controlled. IfNEMOCLAW_GATEWAY_PORTorNEMOCLAW_GATEWAY_BIND_ADDRESSis set in the parent shell/CI, these fixed127.0.0.1:8080assertions can fail nondeterministically.Suggested patch
+ const originalGatewayBindAddress = process.env.NEMOCLAW_GATEWAY_BIND_ADDRESS; + const originalGatewayPort = process.env.NEMOCLAW_GATEWAY_PORT; const originalOverlayFix = process.env.NEMOCLAW_DISABLE_OVERLAY_FIX; + delete process.env.NEMOCLAW_GATEWAY_BIND_ADDRESS; + delete process.env.NEMOCLAW_GATEWAY_PORT; process.env.NEMOCLAW_DISABLE_OVERLAY_FIX = "1"; try { expect(getGatewayStartEnv()).toMatchObject({ OPENSHELL_BIND_ADDRESS: "127.0.0.1", OPENSHELL_SERVER_PORT: "8080", OPENSHELL_SSH_GATEWAY_HOST: "127.0.0.1", OPENSHELL_SSH_GATEWAY_PORT: "8080", }); } finally { + if (originalGatewayBindAddress === undefined) { + delete process.env.NEMOCLAW_GATEWAY_BIND_ADDRESS; + } else { + process.env.NEMOCLAW_GATEWAY_BIND_ADDRESS = originalGatewayBindAddress; + } + if (originalGatewayPort === undefined) { + delete process.env.NEMOCLAW_GATEWAY_PORT; + } else { + process.env.NEMOCLAW_GATEWAY_PORT = originalGatewayPort; + } if (originalOverlayFix === undefined) { delete process.env.NEMOCLAW_DISABLE_OVERLAY_FIX; } else { process.env.NEMOCLAW_DISABLE_OVERLAY_FIX = originalOverlayFix; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/onboard.test.ts` around lines 430 - 467, The test isn't hermetic because it only controls NEMOCLAW_DISABLE_OVERLAY_FIX but not ambient NEMOCLAW_GATEWAY_PORT or NEMOCLAW_GATEWAY_BIND_ADDRESS, causing the assertions on 127.0.0.1:8080 to flake; fix getGatewayStartEnv test by saving original values of process.env.NEMOCLAW_GATEWAY_PORT and process.env.NEMOCLAW_GATEWAY_BIND_ADDRESS, set them to "8080" and "127.0.0.1" (respectively) alongside NEMOCLAW_DISABLE_OVERLAY_FIX before the assertion, then restore all three originals in the finally block; locate this change around the getGatewayStartEnv call in the test (references: getGatewayStartEnv, NEMOCLAW_DISABLE_OVERLAY_FIX).src/lib/onboard.ts (1)
3811-3823:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist the new gateway networking env vars in the DEB override path.
getDockerDriverGatewayEnv()now makes bind address, port, gRPC endpoint, and SSH gateway host/port part of the runtime contract, butwriteDockerGatewayDebEnvOverride()still only writes the old driver/supervisor keys. On distro installs, a service restart will come back on the stale/default networking config, and the next drift check will immediately treat it as stale again.Suggested follow-up
function writeDockerGatewayDebEnvOverride(): void { const servicePath = "/usr/lib/systemd/user/openshell-gateway.service"; const legacyServicePath = "/lib/systemd/user/openshell-gateway.service"; @@ const preserved = existing .split("\n") .filter( (line: string) => line.trim() && - !/^OPENSHELL_(DRIVERS|DOCKER_SUPERVISOR_IMAGE|DOCKER_SUPERVISOR_BIN)=/.test(line), + !/^OPENSHELL_(DRIVERS|BIND_ADDRESS|SERVER_PORT|GRPC_ENDPOINT|SSH_GATEWAY_HOST|SSH_GATEWAY_PORT|DOCKER_SUPERVISOR_IMAGE|DOCKER_SUPERVISOR_BIN)=/.test(line), ); const override = getDockerDriverGatewayEnv(); const next = [ ...preserved, `OPENSHELL_DRIVERS=${override.OPENSHELL_DRIVERS}`, + `OPENSHELL_BIND_ADDRESS=${override.OPENSHELL_BIND_ADDRESS}`, + `OPENSHELL_SERVER_PORT=${override.OPENSHELL_SERVER_PORT}`, + `OPENSHELL_GRPC_ENDPOINT=${override.OPENSHELL_GRPC_ENDPOINT}`, + `OPENSHELL_SSH_GATEWAY_HOST=${override.OPENSHELL_SSH_GATEWAY_HOST}`, + `OPENSHELL_SSH_GATEWAY_PORT=${override.OPENSHELL_SSH_GATEWAY_PORT}`, `OPENSHELL_DOCKER_SUPERVISOR_IMAGE=${override.OPENSHELL_DOCKER_SUPERVISOR_IMAGE}`, ...(override.OPENSHELL_DOCKER_SUPERVISOR_BIN ? [`OPENSHELL_DOCKER_SUPERVISOR_BIN=${override.OPENSHELL_DOCKER_SUPERVISOR_BIN}`] : []), ].join("\n");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard.ts` around lines 3811 - 3823, The DEB override writer writeDockerGatewayDebEnvOverride() currently only persists the old driver/supervisor keys so new networking env vars from getDockerDriverGatewayEnv() (OPENSHELL_BIND_ADDRESS, OPENSHELL_SERVER_PORT, OPENSHELL_GRPC_ENDPOINT, OPENSHELL_SSH_GATEWAY_HOST, OPENSHELL_SSH_GATEWAY_PORT) are not saved; update writeDockerGatewayDebEnvOverride() to include these keys and their computed values (from getDockerDriverGatewayEnv() or the same computation used when building env in onboard.ts) when writing the DEB override file so distro installs will restart with the correct bind address/port, gRPC endpoint and SSH gateway host/port and avoid immediate drift detection.
🧹 Nitpick comments (1)
docs/reference/commands.md (1)
188-188: ⚡ Quick winUse active voice in the gateway-reuse sentence.
Line 188 starts with passive phrasing (“is detected”). Prefer an active subject, e.g., “When NemoClaw detects an existing gateway for reuse, …”.
As per coding guidelines: "Active voice required. Flag passive constructions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/reference/commands.md` at line 188, Replace the passive sentence "When an existing gateway is detected for reuse, NemoClaw probes the host gateway HTTP endpoint..." with an active-voice construction starting with the agent (e.g., "When NemoClaw detects an existing gateway for reuse, NemoClaw probes the host gateway HTTP endpoint before declaring it reusable...") so the subject "NemoClaw" performs the action; ensure the rest of the sentence (probing the host gateway HTTP endpoint and rebuilding if upstream is warming up) remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/core/ports.ts`:
- Around line 103-112: parseGatewayPort currently calls validateGatewayPort only
when hasExplicitEnvValue(envVar) is true, so implicitly-resolved gateway ports
can collide with other services; always run validation after resolving the port.
Change parseGatewayPort to call parsePort(envVar, fallback) and then
unconditionally call validateGatewayPort(envVar, port, options) (remove the
hasExplicitEnvValue guard) so validateGatewayPort always checks the resolved
port; preserve return of port.
In `@src/lib/onboard.ts`:
- Around line 3476-3478: Extract the bind-aware gateway port probe into a single
helper (e.g., isGatewayPortAvailable or probeGatewayPort) that encapsulates
calling checkPortAvailable(GATEWAY_PORT, { host: GATEWAY_BIND_ADDRESS }) and
returning the same result/shape; then replace the three in-file call sites that
currently inline checkPortAvailable(..., { host: GATEWAY_BIND_ADDRESS }) (the
reuse-detection, preflight, and startup usages in src/lib/onboard.ts) to call
this new helper. Ensure the helper is exported/defined near other onboarding
utilities and preserves error/return semantics so existing callers (reuse
detection, preflight, startup) need only swap the call without changing
downstream logic.
In `@src/lib/onboard/gateway-tcp-readiness.test.ts`:
- Around line 97-100: The test currently only checks the return type, which
doesn't verify that isGatewayTcpReady(undefined, 200) actually uses the default
GATEWAY_PORT; update the test to deterministically assert default-port wiring by
spying/mocking the TCP connect call (or the internal helper that opens sockets)
and asserting it was invoked with GATEWAY_PORT when the function is called with
undefined, while still asserting the resolved boolean result; reference the
isGatewayTcpReady function and the GATEWAY_PORT constant when implementing the
spy/mock to validate the port argument.
---
Outside diff comments:
In `@src/lib/onboard.ts`:
- Around line 3811-3823: The DEB override writer
writeDockerGatewayDebEnvOverride() currently only persists the old
driver/supervisor keys so new networking env vars from
getDockerDriverGatewayEnv() (OPENSHELL_BIND_ADDRESS, OPENSHELL_SERVER_PORT,
OPENSHELL_GRPC_ENDPOINT, OPENSHELL_SSH_GATEWAY_HOST, OPENSHELL_SSH_GATEWAY_PORT)
are not saved; update writeDockerGatewayDebEnvOverride() to include these keys
and their computed values (from getDockerDriverGatewayEnv() or the same
computation used when building env in onboard.ts) when writing the DEB override
file so distro installs will restart with the correct bind address/port, gRPC
endpoint and SSH gateway host/port and avoid immediate drift detection.
In `@test/onboard.test.ts`:
- Around line 430-467: The test isn't hermetic because it only controls
NEMOCLAW_DISABLE_OVERLAY_FIX but not ambient NEMOCLAW_GATEWAY_PORT or
NEMOCLAW_GATEWAY_BIND_ADDRESS, causing the assertions on 127.0.0.1:8080 to
flake; fix getGatewayStartEnv test by saving original values of
process.env.NEMOCLAW_GATEWAY_PORT and process.env.NEMOCLAW_GATEWAY_BIND_ADDRESS,
set them to "8080" and "127.0.0.1" (respectively) alongside
NEMOCLAW_DISABLE_OVERLAY_FIX before the assertion, then restore all three
originals in the finally block; locate this change around the getGatewayStartEnv
call in the test (references: getGatewayStartEnv, NEMOCLAW_DISABLE_OVERLAY_FIX).
---
Nitpick comments:
In `@docs/reference/commands.md`:
- Line 188: Replace the passive sentence "When an existing gateway is detected
for reuse, NemoClaw probes the host gateway HTTP endpoint..." with an
active-voice construction starting with the agent (e.g., "When NemoClaw detects
an existing gateway for reuse, NemoClaw probes the host gateway HTTP endpoint
before declaring it reusable...") so the subject "NemoClaw" performs the action;
ensure the rest of the sentence (probing the host gateway HTTP endpoint and
rebuilding if upstream is warming up) remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c7a11e0c-5d3a-48bb-aa11-8deaefdbc69a
📒 Files selected for processing (15)
docs/reference/commands.mddocs/reference/troubleshooting.mddocs/security/best-practices.mdsrc/lib/core/gateway-address.test.tssrc/lib/core/gateway-address.tssrc/lib/core/ports.test.tssrc/lib/core/ports.tssrc/lib/onboard.tssrc/lib/onboard/gateway-http-readiness.tssrc/lib/onboard/gateway-tcp-readiness.test.tssrc/lib/onboard/gateway-tcp-readiness.tssrc/lib/onboard/preflight.test.tssrc/lib/onboard/preflight.tstest/gateway-start-wait.test.tstest/onboard.test.ts
ericksoa
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/onboard.ts`:
- Line 4730: Replace uses of getDockerDriverGatewayEndpoint() inside the
non-Docker diagnostics branches with the legacy helper getGatewayLocalEndpoint()
so the diagnostic messages point to the correct host/scheme when the
Docker-driver gateway is disabled; specifically update the interpolated message
string that currently reads "Gateway container is running but
${getDockerDriverGatewayEndpoint()}/ is not responding. Recreating..." and any
other similar diagnostics that call getDockerDriverGatewayEndpoint() (also
present at the other noted occurrences) to use getGatewayLocalEndpoint()
instead, leaving the surrounding logic in functions handling gateway diagnostics
unchanged.
In `@src/lib/onboard/docker-driver-gateway-env.ts`:
- Around line 132-135: The current write uses fs.writeFileSync(envFile,
buildDockerGatewayDebEnvFile(existing, override), { mode: 0o600 }) but mode only
applies on create, leaving an existing gateway.env with permissive bits
unchanged; after calling fs.writeFileSync (or before), call
fs.chmodSync(envFile, 0o600) to enforce restrictive permissions for the existing
file (use the same envFile variable and buildDockerGatewayDebEnvFile call), and
ensure you handle/propagate any errors from chmod appropriately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b8496acd-cddf-41c2-bd99-39aace6299f2
📒 Files selected for processing (8)
docs/reference/commands.mdsrc/lib/core/ports.test.tssrc/lib/core/ports.tssrc/lib/onboard.tssrc/lib/onboard/docker-driver-gateway-env.test.tssrc/lib/onboard/docker-driver-gateway-env.tssrc/lib/onboard/gateway-tcp-readiness.test.tstest/gateway-start-wait.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/onboard/docker-driver-gateway-env.test.ts
- docs/reference/commands.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/core/ports.test.ts
- test/gateway-start-wait.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/lib/onboard/docker-driver-gateway-env.ts (1)
154-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce permissions for pre-existing gateway env dir/file.
On Line 154 and Line 156,
modeis applied at creation time; existing~/.config/openshell/gateway.envcan remain overly permissive. Please explicitlychmodboth after creation/write.Suggested minimal patch
- const envFile = path.join(os.homedir(), ".config", "openshell", "gateway.env"); - fs.mkdirSync(path.dirname(envFile), { recursive: true, mode: 0o700 }); + const envDir = path.join(os.homedir(), ".config", "openshell"); + const envFile = path.join(envDir, "gateway.env"); + fs.mkdirSync(envDir, { recursive: true, mode: 0o700 }); + fs.chmodSync(envDir, 0o700); const existing = readTextFileIfPresent(envFile); fs.writeFileSync(envFile, buildDockerGatewayDebEnvFile(existing, override), { encoding: "utf-8", mode: 0o600, }); + fs.chmodSync(envFile, 0o600);In Node.js fs.writeFileSync, does the `mode` option only affect newly created files, and should fs.chmodSync be used to enforce permissions on existing files?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard/docker-driver-gateway-env.ts` around lines 154 - 159, The directory/file permission flags passed to fs.mkdirSync and fs.writeFileSync only affect newly created items; enforce permissions for existing ones by calling fs.chmodSync after creation/write: after fs.mkdirSync(...) ensure the directory at path.dirname(envFile) is chmod'd to 0o700 with fs.chmodSync, and after writing the envFile with buildDockerGatewayDebEnvFile(...) via fs.writeFileSync(...) call fs.chmodSync(envFile, 0o600); locate usages around the envFile variable and functions readTextFileIfPresent and buildDockerGatewayDebEnvFile to add these explicit chmod calls.src/lib/onboard.ts (1)
4749-4749:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the legacy gateway endpoint in these non-Docker diagnostics.
These branches only execute when the Docker-driver gateway is disabled, but the messages still interpolate
getDockerDriverGatewayEndpoint(). That points users at the wrong endpoint while debugging legacy gateway failures. UsegetGatewayLocalEndpoint()consistently here.Suggested fix
- ` Gateway container is running but ${getDockerDriverGatewayEndpoint()}/ is not responding. Recreating...`, + ` Gateway container is running but ${getGatewayLocalEndpoint()}/ is not responding. Recreating...`, - ` Gateway metadata reports healthy but ${getDockerDriverGatewayEndpoint()}/ is not responding. Starting a fresh gateway...`, + ` Gateway metadata reports healthy but ${getGatewayLocalEndpoint()}/ is not responding. Starting a fresh gateway...`, - ` Error: could not verify gateway container state and ${getDockerDriverGatewayEndpoint()}/ is not responding.`, + ` Error: could not verify gateway container state and ${getGatewayLocalEndpoint()}/ is not responding.`, - ` Gateway container is running but ${getDockerDriverGatewayEndpoint()}/ is not responding. Recreating...`, + ` Gateway container is running but ${getGatewayLocalEndpoint()}/ is not responding. Recreating...`,Also applies to: 5028-5028, 11187-11187, 11200-11200
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard.ts` at line 4749, Replace usages of getDockerDriverGatewayEndpoint() in the non-Docker diagnostic message branches with getGatewayLocalEndpoint() so diagnostics point to the legacy/local gateway endpoint; locate the diagnostic strings that currently interpolate getDockerDriverGatewayEndpoint() (e.g., the message template "`Gateway container is running but ${getDockerDriverGatewayEndpoint()}/ is not responding. Recreating...`") and update them to use getGatewayLocalEndpoint() instead, ensuring all non-Docker paths (the same pattern also at the other occurrences) consistently reference getGatewayLocalEndpoint().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/lib/onboard.ts`:
- Line 4749: Replace usages of getDockerDriverGatewayEndpoint() in the
non-Docker diagnostic message branches with getGatewayLocalEndpoint() so
diagnostics point to the legacy/local gateway endpoint; locate the diagnostic
strings that currently interpolate getDockerDriverGatewayEndpoint() (e.g., the
message template "`Gateway container is running but
${getDockerDriverGatewayEndpoint()}/ is not responding. Recreating...`") and
update them to use getGatewayLocalEndpoint() instead, ensuring all non-Docker
paths (the same pattern also at the other occurrences) consistently reference
getGatewayLocalEndpoint().
In `@src/lib/onboard/docker-driver-gateway-env.ts`:
- Around line 154-159: The directory/file permission flags passed to
fs.mkdirSync and fs.writeFileSync only affect newly created items; enforce
permissions for existing ones by calling fs.chmodSync after creation/write:
after fs.mkdirSync(...) ensure the directory at path.dirname(envFile) is chmod'd
to 0o700 with fs.chmodSync, and after writing the envFile with
buildDockerGatewayDebEnvFile(...) via fs.writeFileSync(...) call
fs.chmodSync(envFile, 0o600); locate usages around the envFile variable and
functions readTextFileIfPresent and buildDockerGatewayDebEnvFile to add these
explicit chmod calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0ec72966-4b5f-478f-a8ac-af1e82974cf0
📒 Files selected for processing (4)
src/lib/onboard.tssrc/lib/onboard/docker-driver-gateway-env.test.tssrc/lib/onboard/docker-driver-gateway-env.tstest/onboard.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard/docker-driver-gateway-env.test.ts
- test/onboard.test.ts
…-config Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25767133910
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25768132522
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25771873587
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25772601822
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25772816411
|
Contributor
E2E Advisor RecommendationRequired E2E: Full advisor summaryPi Semantic E2E AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/lib/onboard.ts (1)
4796-4798:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the helper-derived local endpoint in this Docker-driver warning.
This is the one remaining reuse diagnostic that still hardcodes
http://127.0.0.1:${GATEWAY_PORT}/. With configurable bind/connect host and HTTPS endpoint support, it can point users at the wrong address, port, or scheme while debugging a failed reuse. Switch it togetGatewayLocalEndpoint()like the other gateway diagnostics.Suggested fix
- ` Docker-driver gateway metadata reports healthy but http://127.0.0.1:${GATEWAY_PORT}/ is not responding. Starting a fresh gateway...`, + ` Docker-driver gateway metadata reports healthy but ${getGatewayLocalEndpoint()}/ is not responding. Starting a fresh gateway...`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard.ts` around lines 4796 - 4798, Replace the hardcoded URL in the Docker-driver gateway warning with the helper-derived local endpoint: use getGatewayLocalEndpoint() instead of constructing `http://127.0.0.1:${GATEWAY_PORT}/`. Locate the log emitting that message (the console.log that mentions "Docker-driver gateway metadata reports healthy but http://127.0.0.1:${GATEWAY_PORT}/ is not responding. Starting a fresh gateway...") and build the message using getGatewayLocalEndpoint() so it reflects configurable host, port and scheme; keep the rest of the wording intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/core/gateway-address.ts`:
- Around line 18-21: The current check uses raw before trimming so
whitespace-only values like " " don't fall back; update the logic to normalize
first (e.g., compute trimmed = String(raw ?? "").trim()), then treat trimmed ===
"" as the empty case and return fallback, and keep the existing comparisons
against DEFAULT_GATEWAY_BIND_ADDRESS and WILDCARD_GATEWAY_BIND_ADDRESS using
trimmed to decide returns.
In `@test/onboard.test.ts`:
- Around line 452-467: Test assumes default gateway envs but can be influenced
by NEMOCLAW_GATEWAY_BIND_ADDRESS/NEMOCLAW_GATEWAY_PORT; before calling
getGatewayStartEnv() snapshot the current values of
process.env.NEMOCLAW_GATEWAY_BIND_ADDRESS and process.env.NEMOCLAW_GATEWAY_PORT
(like originalOverlayFix for NEMOCLAW_DISABLE_OVERLAY_FIX), then clear them
(delete if undefined) so getGatewayStartEnv() sees defaults, perform the expect
on getGatewayStartEnv(), and finally restore the two env vars to their original
values in the finally block; reference getGatewayStartEnv,
NEMOCLAW_GATEWAY_BIND_ADDRESS, NEMOCLAW_GATEWAY_PORT, and
NEMOCLAW_DISABLE_OVERLAY_FIX.
---
Duplicate comments:
In `@src/lib/onboard.ts`:
- Around line 4796-4798: Replace the hardcoded URL in the Docker-driver gateway
warning with the helper-derived local endpoint: use getGatewayLocalEndpoint()
instead of constructing `http://127.0.0.1:${GATEWAY_PORT}/`. Locate the log
emitting that message (the console.log that mentions "Docker-driver gateway
metadata reports healthy but http://127.0.0.1:${GATEWAY_PORT}/ is not
responding. Starting a fresh gateway...") and build the message using
getGatewayLocalEndpoint() so it reflects configurable host, port and scheme;
keep the rest of the wording intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 152d2a68-ee02-44ef-96f2-5735031ac874
📒 Files selected for processing (10)
docs/reference/commands.mddocs/reference/troubleshooting.mdsrc/lib/core/gateway-address.tssrc/lib/onboard.tssrc/lib/onboard/docker-driver-gateway-env.test.tssrc/lib/onboard/gateway-http-readiness.tssrc/lib/runner.tstest/e2e/test-openshell-gateway-upgrade.shtest/onboard-selection.test.tstest/onboard.test.ts
✅ Files skipped from review due to trivial changes (4)
- test/e2e/test-openshell-gateway-upgrade.sh
- src/lib/runner.ts
- docs/reference/troubleshooting.md
- docs/reference/commands.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard/docker-driver-gateway-env.test.ts
Comment on lines
+18
to
+21
| if (raw === undefined || raw === "") return fallback; | ||
| const trimmed = String(raw).trim(); | ||
| if (trimmed === DEFAULT_GATEWAY_BIND_ADDRESS) return DEFAULT_GATEWAY_BIND_ADDRESS; | ||
| if (trimmed === WILDCARD_GATEWAY_BIND_ADDRESS) return WILDCARD_GATEWAY_BIND_ADDRESS; |
Contributor
There was a problem hiding this comment.
Treat whitespace-only bind values as empty input.
At Line 18, blank handling is checked before trimming, so NEMOCLAW_GATEWAY_BIND_ADDRESS=" " throws instead of using the fallback. Normalizing empties after trim avoids surprising config failures.
Suggested patch
export function parseGatewayBindAddress(
envVar = "NEMOCLAW_GATEWAY_BIND_ADDRESS",
fallback: GatewayBindAddress = DEFAULT_GATEWAY_BIND_ADDRESS,
): GatewayBindAddress {
const raw = process.env[envVar];
- if (raw === undefined || raw === "") return fallback;
const trimmed = String(raw).trim();
+ if (raw === undefined || trimmed === "") return fallback;
if (trimmed === DEFAULT_GATEWAY_BIND_ADDRESS) return DEFAULT_GATEWAY_BIND_ADDRESS;
if (trimmed === WILDCARD_GATEWAY_BIND_ADDRESS) return WILDCARD_GATEWAY_BIND_ADDRESS;
throw new Error(
`Invalid gateway bind address: ${envVar}="${raw}" — must be either ${DEFAULT_GATEWAY_BIND_ADDRESS} or ${WILDCARD_GATEWAY_BIND_ADDRESS}`,
);
}📝 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 (raw === undefined || raw === "") return fallback; | |
| const trimmed = String(raw).trim(); | |
| if (trimmed === DEFAULT_GATEWAY_BIND_ADDRESS) return DEFAULT_GATEWAY_BIND_ADDRESS; | |
| if (trimmed === WILDCARD_GATEWAY_BIND_ADDRESS) return WILDCARD_GATEWAY_BIND_ADDRESS; | |
| const trimmed = String(raw).trim(); | |
| if (raw === undefined || trimmed === "") return fallback; | |
| if (trimmed === DEFAULT_GATEWAY_BIND_ADDRESS) return DEFAULT_GATEWAY_BIND_ADDRESS; | |
| if (trimmed === WILDCARD_GATEWAY_BIND_ADDRESS) return WILDCARD_GATEWAY_BIND_ADDRESS; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/core/gateway-address.ts` around lines 18 - 21, The current check uses
raw before trimming so whitespace-only values like " " don't fall back; update
the logic to normalize first (e.g., compute trimmed = String(raw ?? "").trim()),
then treat trimmed === "" as the empty case and return fallback, and keep the
existing comparisons against DEFAULT_GATEWAY_BIND_ADDRESS and
WILDCARD_GATEWAY_BIND_ADDRESS using trimmed to decide returns.
Comment on lines
+452
to
+467
| const originalOverlayFix = process.env.NEMOCLAW_DISABLE_OVERLAY_FIX; | ||
| process.env.NEMOCLAW_DISABLE_OVERLAY_FIX = "1"; | ||
| try { | ||
| expect(getGatewayStartEnv()).toMatchObject({ | ||
| OPENSHELL_BIND_ADDRESS: "127.0.0.1", | ||
| OPENSHELL_SERVER_PORT: "8080", | ||
| OPENSHELL_SSH_GATEWAY_HOST: "127.0.0.1", | ||
| OPENSHELL_SSH_GATEWAY_PORT: "8080", | ||
| }); | ||
| } finally { | ||
| if (originalOverlayFix === undefined) { | ||
| delete process.env.NEMOCLAW_DISABLE_OVERLAY_FIX; | ||
| } else { | ||
| process.env.NEMOCLAW_DISABLE_OVERLAY_FIX = originalOverlayFix; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Make gateway env assertions hermetic
Line 455 currently assumes default gateway bind/port behavior, but inherited NEMOCLAW_GATEWAY_BIND_ADDRESS or NEMOCLAW_GATEWAY_PORT can make this test fail nondeterministically. Snapshot and clear them in this test (same pattern you used for NEMOCLAW_DISABLE_OVERLAY_FIX) before asserting defaults.
Suggested patch
- const originalOverlayFix = process.env.NEMOCLAW_DISABLE_OVERLAY_FIX;
+ const originalOverlayFix = process.env.NEMOCLAW_DISABLE_OVERLAY_FIX;
+ const originalGatewayBindAddress = process.env.NEMOCLAW_GATEWAY_BIND_ADDRESS;
+ const originalGatewayPort = process.env.NEMOCLAW_GATEWAY_PORT;
process.env.NEMOCLAW_DISABLE_OVERLAY_FIX = "1";
+ delete process.env.NEMOCLAW_GATEWAY_BIND_ADDRESS;
+ delete process.env.NEMOCLAW_GATEWAY_PORT;
try {
expect(getGatewayStartEnv()).toMatchObject({
OPENSHELL_BIND_ADDRESS: "127.0.0.1",
OPENSHELL_SERVER_PORT: "8080",
OPENSHELL_SSH_GATEWAY_HOST: "127.0.0.1",
@@
if (originalOverlayFix === undefined) {
delete process.env.NEMOCLAW_DISABLE_OVERLAY_FIX;
} else {
process.env.NEMOCLAW_DISABLE_OVERLAY_FIX = originalOverlayFix;
}
+ if (originalGatewayBindAddress === undefined) {
+ delete process.env.NEMOCLAW_GATEWAY_BIND_ADDRESS;
+ } else {
+ process.env.NEMOCLAW_GATEWAY_BIND_ADDRESS = originalGatewayBindAddress;
+ }
+ if (originalGatewayPort === undefined) {
+ delete process.env.NEMOCLAW_GATEWAY_PORT;
+ } else {
+ process.env.NEMOCLAW_GATEWAY_PORT = originalGatewayPort;
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/onboard.test.ts` around lines 452 - 467, Test assumes default gateway
envs but can be influenced by
NEMOCLAW_GATEWAY_BIND_ADDRESS/NEMOCLAW_GATEWAY_PORT; before calling
getGatewayStartEnv() snapshot the current values of
process.env.NEMOCLAW_GATEWAY_BIND_ADDRESS and process.env.NEMOCLAW_GATEWAY_PORT
(like originalOverlayFix for NEMOCLAW_DISABLE_OVERLAY_FIX), then clear them
(delete if undefined) so getGatewayStartEnv() sees defaults, perform the expect
on getGatewayStartEnv(), and finally restore the two env vars to their original
values in the finally block; reference getGatewayStartEnv,
NEMOCLAW_GATEWAY_BIND_ADDRESS, NEMOCLAW_GATEWAY_PORT, and
NEMOCLAW_DISABLE_OVERLAY_FIX.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
4605-4637: Run the gateway/onboarding E2Es for this path before merge.This diff now threads bind-address/port config through gateway reuse, env persistence, startup, and recovery inside the onboarding core. The unit coverage is solid, but I’d still run at least the gateway-focused E2Es (
sandbox-operations-e2eandopenshell-gateway-upgrade-e2e) to catch cross-step regressions.As per coding guidelines,
src/lib/onboard.ts: "This file contains core onboarding logic. Changes here affect the full sandbox creation and configuration flow." The same guideline recommendssandbox-operations-e2eandopenshell-gateway-upgrade-e2e.Also applies to: 4750-4752
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard.ts` around lines 4605 - 4637, Run the gateway-focused end-to-end tests against the updated onboarding path in src/lib/onboard.ts to catch cross-step regressions introduced by threading bind-address/port and env persistence through gateway reuse/restart: specifically run sandbox-operations-e2e and openshell-gateway-upgrade-e2e exercising the flows that call runCaptureOpenshell, getDockerDriverGatewayEnv, isGatewayHealthy, getDockerDriverGatewayPid/getDockerDriverGatewayRuntimeDrift, restartDockerDriverGatewayProcessForDrift, registerDockerDriverGatewayEndpoint, isDockerDriverGatewayHttpReady, checkGatewayPortAvailable and getDockerDriverGatewayPortListenerPid to validate startup, reuse, drift recovery and port/listener behavior; if failures appear, iterate locally until these E2Es pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 4605-4637: Run the gateway-focused end-to-end tests against the
updated onboarding path in src/lib/onboard.ts to catch cross-step regressions
introduced by threading bind-address/port and env persistence through gateway
reuse/restart: specifically run sandbox-operations-e2e and
openshell-gateway-upgrade-e2e exercising the flows that call
runCaptureOpenshell, getDockerDriverGatewayEnv, isGatewayHealthy,
getDockerDriverGatewayPid/getDockerDriverGatewayRuntimeDrift,
restartDockerDriverGatewayProcessForDrift, registerDockerDriverGatewayEndpoint,
isDockerDriverGatewayHttpReady, checkGatewayPortAvailable and
getDockerDriverGatewayPortListenerPid to validate startup, reuse, drift recovery
and port/listener behavior; if failures appear, iterate locally until these E2Es
pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 749f93fb-f35c-4f22-8b6c-4d8ce1f91da7
📒 Files selected for processing (2)
src/lib/onboard.tstest/onboard.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/onboard.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Closes #2779.
This adds bounded OpenShell gateway network configuration for onboarding:
127.0.0.1:8080NEMOCLAW_GATEWAY_BIND_ADDRESS=127.0.0.1or0.0.0.00.0.0.0NEMOCLAW_GATEWAY_PORTvalues that overlap the dashboard allocation range, configured NemoClaw service ports, or the default inference/proxy portsValidation
npm run build:clinpx vitest run src/lib/core/ports.test.ts src/lib/core/gateway-address.test.ts src/lib/onboard/preflight.test.ts src/lib/onboard/gateway-tcp-readiness.test.ts test/gateway-start-wait.test.ts test/gateway-http-reuse-wait.test.ts test/check-env-var-docs.test.ts test/onboard.test.tsnpm run checksgit diff --checkSummary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests