feat(gateway): manage the default gateway service#7319
Conversation
Signed-off-by: San Dang <sdang@nvidia.com>
|
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:
📝 WalkthroughWalkthroughThis PR adds managed OpenShell gateway installation and lifecycle handling across Linux and Apple Silicon macOS, including service ownership, port validation, secure environment-file updates, uninstall cleanup, reboot recovery, installer pinning, tests, and documentation. ChangesGateway lifecycle and platform support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-7319.docs.buildwithfern.com/nemoclaw |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit dab4958 in the TypeScript / code-coverage/cliThe overall coverage in commit dab4958 in the Show a code coverage summary of the most impacted files.
Updated |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
sandl99
changed the title
Signed-off-by: San Dang <sdang@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
scripts/install-openshell.sh (2)
650-651: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHarden the curl fallback with timeouts and TLS options.
Unlike
fetch_fileinscripts/check-installer-hash.sh(which uses--proto '=https' --tlsv1.2 --connect-timeout 10 --max-time 30 --retry), this fallback runs a barecurl -fL -sS. A stalled endpoint can hang the installer indefinitely, and the downloadedopenshell.rbis subsequently executed by Homebrew. Consider matching the hardened flags for consistent timeout/TLS behavior.🛡️ Proposed change
- curl -fL -sS "https://github.com/NVIDIA/OpenShell/releases/download/${release_tag}/openshell.rb" \ - -o "$output" + curl --proto '=https' --tlsv1.2 -fL -sS \ + --connect-timeout 10 --max-time 30 --retry 3 --retry-delay 1 --retry-all-errors \ + "https://github.com/NVIDIA/OpenShell/releases/download/${release_tag}/openshell.rb" \ + -o "$output"🤖 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 `@scripts/install-openshell.sh` around lines 650 - 651, Harden the curl invocation in the fallback download flow by adding enforced HTTPS/TLS 1.2, connection and total timeouts, and retry behavior consistent with fetch_file. Keep the existing release URL, output path, and fail/silent/show-error behavior unchanged.
781-786: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the standalone-fallback warning conditional on the missing
brew.The
warn "Homebrew is not installed…"is placed unconditionally after the innerif, so it is correct only becauseinstall_macos_homebrew_formulaalwaysexits. If that function is ever changed toreturnon a path, this branch would print "Homebrew is not installed" on a host where Homebrew is installed and then fall through to the standalone gateway. Move the warning into an explicitelse.♻️ Proposed structure
if [ "$OS" = "Darwin" ]; then if command -v brew >/dev/null 2>&1; then install_macos_homebrew_formula + else + warn "Homebrew is not installed; installing the standalone OpenShell gateway without reboot persistence." fi - warn "Homebrew is not installed; installing the standalone OpenShell gateway without reboot persistence." fi🤖 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 `@scripts/install-openshell.sh` around lines 781 - 786, In the Darwin branch, update the Homebrew check around install_macos_homebrew_formula so the standalone-fallback warning runs only in an explicit else branch when brew is unavailable. Preserve the existing formula installation path without warning or falling through when command -v brew succeeds.scripts/install.sh (1)
1366-1388: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the OpenShell unit definition single-sourced
scripts/install.sh:1366-1388duplicatesbuildNemoclawOpenShellGatewayUserServiceinsrc/lib/onboard/docker-driver-gateway-service.ts; add a parity test or generate one from the other so the marker,EnvironmentFile, andExecStartPreargs don’t drift.🤖 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 `@scripts/install.sh` around lines 1366 - 1388, The OpenShell gateway user-service definition is duplicated between the installer heredoc and buildNemoclawOpenShellGatewayUserService. Make one source authoritative or add a parity test covering the marker, EnvironmentFile, and ExecStartPre certificate-generation arguments, so both definitions remain synchronized.src/lib/onboard.ts (1)
1984-2015: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove port-ownership policy into a focused gateway module.
These helpers add ownership decisions and lifecycle failure policy to
src/lib/onboard.ts; keep this entrypoint to dependency wiring and invoke a focused service/helper instead.As per path instructions,
src/lib/onboard.tsmust remain entry setup and dependency wiring, while lifecycle effects belong in focused services.🤖 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 1984 - 2015, Move reportUntrustedGatewayPort and validateServicePortOwner out of the onboard entrypoint into a focused gateway port-ownership service/helper. Keep the ownership validation and exit/throw behavior unchanged, and update src/lib/onboard.ts to wire dependencies and invoke that service rather than implementing lifecycle policy locally.Source: Path instructions
src/lib/actions/uninstall/run-plan.ts (1)
763-862: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
fs.*calls bypass the existing runtime DI pattern.
removeNemoclawOpenShellGatewayUserServiceandremoveNemoclawOpenShellGatewayEnvcallfs.lstatSync,fs.readFileSync,fs.writeFileSync, andfs.chmodSyncdirectly, while every other filesystem/process interaction in this file goes through the injectableUninstallRuntime(existsSync,rmSync,run). This is why the companion test (run-plan-gateway-service.test.ts) has to fall back tovi.spyOn(fs, "readFileSync")on the real module to simulate a read failure, instead of injecting a fake throughdeps/runtimelike the rest of the suite does.Consider adding these as injectable functions on
UninstallRunDeps/UninstallRuntimefor consistency with the rest of the file and to avoid global module monkey-patching in tests.As per path instructions, "actions orchestrate, domain modules make pure decisions, adapters own host/process/network boundaries, and state modules own persisted files and state I/O," and
src/lib/README.mdnotes to "avoid adding host-boundary/process/filesystem/OS concerns to domain logic... those belong insrc/lib/adapters/**so tests can inject fakes."🤖 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/actions/uninstall/run-plan.ts` around lines 763 - 862, Replace the direct filesystem calls in removeNemoclawOpenShellGatewayUserService and removeNemoclawOpenShellGatewayEnv with injectable operations exposed through UninstallRunDeps and UninstallRuntime, including lstat, read, write, and chmod as needed. Thread the implementations through the runtime construction and update callers/tests to inject fakes, eliminating reliance on global fs spies while preserving the existing behavior and error handling.Source: Path instructions
src/lib/actions/uninstall/run-plan-gateway-segregation.test.ts (1)
519-522: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winImport the shared gateway marker constant here. This fixture duplicates the marker string; reusing
NEMOCLAW_OPENSHELL_GATEWAY_USER_SERVICE_MARKER_LINEkeeps the test aligned with the source of truth and avoids future drift.🤖 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/actions/uninstall/run-plan-gateway-segregation.test.ts` around lines 519 - 522, Update the fixture in the relevant uninstall plan test to import and reuse NEMOCLAW_OPENSHELL_GATEWAY_USER_SERVICE_MARKER_LINE instead of duplicating the gateway marker string in fs.writeFileSync. Preserve the existing service content and formatting while sourcing the marker from the shared constant.Source: Path instructions
🤖 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 `@scripts/install-openshell.sh`:
- Around line 650-651: Harden the curl invocation in the fallback download flow
by adding enforced HTTPS/TLS 1.2, connection and total timeouts, and retry
behavior consistent with fetch_file. Keep the existing release URL, output path,
and fail/silent/show-error behavior unchanged.
- Around line 781-786: In the Darwin branch, update the Homebrew check around
install_macos_homebrew_formula so the standalone-fallback warning runs only in
an explicit else branch when brew is unavailable. Preserve the existing formula
installation path without warning or falling through when command -v brew
succeeds.
In `@scripts/install.sh`:
- Around line 1366-1388: The OpenShell gateway user-service definition is
duplicated between the installer heredoc and
buildNemoclawOpenShellGatewayUserService. Make one source authoritative or add a
parity test covering the marker, EnvironmentFile, and ExecStartPre
certificate-generation arguments, so both definitions remain synchronized.
In `@src/lib/actions/uninstall/run-plan-gateway-segregation.test.ts`:
- Around line 519-522: Update the fixture in the relevant uninstall plan test to
import and reuse NEMOCLAW_OPENSHELL_GATEWAY_USER_SERVICE_MARKER_LINE instead of
duplicating the gateway marker string in fs.writeFileSync. Preserve the existing
service content and formatting while sourcing the marker from the shared
constant.
In `@src/lib/actions/uninstall/run-plan.ts`:
- Around line 763-862: Replace the direct filesystem calls in
removeNemoclawOpenShellGatewayUserService and removeNemoclawOpenShellGatewayEnv
with injectable operations exposed through UninstallRunDeps and
UninstallRuntime, including lstat, read, write, and chmod as needed. Thread the
implementations through the runtime construction and update callers/tests to
inject fakes, eliminating reliance on global fs spies while preserving the
existing behavior and error handling.
In `@src/lib/onboard.ts`:
- Around line 1984-2015: Move reportUntrustedGatewayPort and
validateServicePortOwner out of the onboard entrypoint into a focused gateway
port-ownership service/helper. Keep the ownership validation and exit/throw
behavior unchanged, and update src/lib/onboard.ts to wire dependencies and
invoke that service rather than implementing lifecycle policy locally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0af3104d-53b3-4139-8b54-de15ab5b5eba
📒 Files selected for processing (40)
ci/platform-matrix.jsonci/source-shape-test-budget.jsondocs/get-started/prerequisites.mdxdocs/manage-sandboxes/uninstall-nemoclaw.mdxdocs/reference/architecture.mdxdocs/reference/commands.mdxdocs/reference/platform-support.mdxdocs/reference/troubleshooting.mdxscripts/check-installer-hash.shscripts/checks/extract-installer-pins.mtsscripts/install-openshell.shscripts/install.shsrc/lib/actions/uninstall/run-plan-gateway-segregation.test.tssrc/lib/actions/uninstall/run-plan-gateway-service.test.tssrc/lib/actions/uninstall/run-plan.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/onboard.tssrc/lib/onboard/docker-driver-gateway-env-deb-override.test.tssrc/lib/onboard/docker-driver-gateway-env-service.test.tssrc/lib/onboard/docker-driver-gateway-env.test.tssrc/lib/onboard/docker-driver-gateway-env.tssrc/lib/onboard/docker-driver-gateway-port-listener.test.tssrc/lib/onboard/docker-driver-gateway-port-listener.tssrc/lib/onboard/docker-driver-gateway-service.test.tssrc/lib/onboard/docker-driver-gateway-service.tssrc/lib/onboard/gateway-binding.test.tssrc/lib/onboard/gateway-binding.tssrc/lib/onboard/gateway-http-readiness.test.tssrc/lib/onboard/gateway-http-readiness.tssrc/lib/onboard/openshell-install.tstest/e2e/fixtures/phases/lifecycle.tstest/e2e/registry/definitions/baseline.tstest/e2e/registry/expected-states.tstest/e2e/support/e2e-expected-state.test.tstest/e2e/support/e2e-phase-lifecycle.test.tstest/install-openshell-gateway-service.test.tstest/install-openshell-version-check.test.tstest/installer-hash-check.test.tstest/onboard-gateway-prelaunch-cutover.test.tstest/reboot-recovery-docs.test.ts
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/adapters/fs/regular-file.ts (1)
36-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
readUtf8is not idempotent across repeated calls.
fs.readFileSync(descriptor, ...)reads from the fd's current position and advances it; a secondreadUtf8()call on the sameOpenRegularFilewould return an empty string rather than the file content again, since the descriptor is already at EOF.replaceUtf8avoids this by using an explicit position (0), butreadUtf8doesn't. No current caller reads twice, but this is a silent-data-loss footgun for a shared, reusable adapter (used for credential/env files) if a future caller reads more than once per instance.🛡️ Proposed fix: read via explicit position
return { close, - readUtf8: () => String(fs.readFileSync(descriptor, "utf-8")), + readUtf8: () => { + const size = fs.fstatSync(descriptor).size; + const buffer = Buffer.alloc(size); + fs.readSync(descriptor, buffer, 0, size, 0); + return buffer.toString("utf-8"); + }, replaceUtf8: (contents, mode) => {Node's own docs/issue tracker confirm read-via-fd continues from the current position rather than always starting at the beginning, which is the mechanism behind this concern.
🤖 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/adapters/fs/regular-file.ts` around lines 36 - 46, Update the readUtf8 method in the returned OpenRegularFile adapter to read from offset 0 explicitly, matching replaceUtf8’s positioned I/O, so repeated calls always return the full file contents without depending on the descriptor’s current position.src/lib/actions/uninstall/run-plan.ts (1)
767-883: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the two Linux gateway-cleanup helpers to reduce complexity.
removeNemoclawOpenShellGatewayUserServiceandremoveNemoclawOpenShellGatewayEnveach mix open/read/validate/disable/remove/reload steps in one function body, matching the tool's owncode_block_complexity_highsignal for this range. Extracting the "read + validate marker" step from the "disable/remove/reload" step (and similarly for the env cleanup) would make each piece independently testable and easier to reason about.As per coding guidelines, "Keep function complexity low and avoid introducing unnecessary complexity hotspots."
🤖 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/actions/uninstall/run-plan.ts` around lines 767 - 883, Split removeNemoclawOpenShellGatewayUserService into focused helpers for reading and validating the managed service marker and for disabling, removing, and reloading the service, preserving its current return and warning behavior. Similarly decompose removeNemoclawOpenShellGatewayEnv so file access/content preservation and deletion are handled by smaller independently testable helpers. Keep the existing public helper names and cleanup outcomes unchanged.Source: Coding guidelines
🤖 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/actions/uninstall/run-plan.ts`:
- Around line 767-883: Split removeNemoclawOpenShellGatewayUserService into
focused helpers for reading and validating the managed service marker and for
disabling, removing, and reloading the service, preserving its current return
and warning behavior. Similarly decompose removeNemoclawOpenShellGatewayEnv so
file access/content preservation and deletion are handled by smaller
independently testable helpers. Keep the existing public helper names and
cleanup outcomes unchanged.
In `@src/lib/adapters/fs/regular-file.ts`:
- Around line 36-46: Update the readUtf8 method in the returned OpenRegularFile
adapter to read from offset 0 explicitly, matching replaceUtf8’s positioned I/O,
so repeated calls always return the full file contents without depending on the
descriptor’s current position.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f766cc62-694b-4697-8e22-b8cc0acd6c0b
📒 Files selected for processing (8)
scripts/checks/extract-installer-pins.mtsscripts/install-openshell.shsrc/lib/actions/uninstall/run-plan-gateway-segregation.test.tssrc/lib/actions/uninstall/run-plan-gateway-service.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/adapters/fs/regular-file.test.tssrc/lib/adapters/fs/regular-file.tstest/install-openshell-gateway-service.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/install-openshell-gateway-service.test.ts
- scripts/checks/extract-installer-pins.mts
- scripts/install-openshell.sh
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Signed-off-by: San Dang <sdang@nvidia.com>
wscurran
added
area: install
# Conflicts: # ci/platform-matrix.json # docs/reference/platform-support.mdx
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
# Conflicts: # src/lib/actions/uninstall/run-plan.test.ts
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
6 participants
Summary
NemoClaw now gives its default OpenShell gateway on port
8080an explicit host-service owner: a validated systemd user service on Linux or the official Homebrew service on Apple Silicon. NemoClaw-managed custom ports retain the detached-process lifecycle, while a declared external supervisor retains authority on any matching port.Related Issue
Fixes #6903
Product Scope
Verdict: PASS. Issue #6903 contains the member-authored lifecycle, ownership, compatibility, security, and validation decision for the managed default gateway, and the decision is incorporated into the #7209 release-validation tracker. The refreshed implementation also preserves main's separately accepted external-supervisor contract from #7246.
This product-scope verdict is based on those design records. It is separate from, and does not infer approval from, GitHub's
mergeStateStatus.Changes
XDG_BIN_HOMEandXDG_CONFIG_HOMEvalues, while retaining their default paths.The service-selection path is required by #6903 so reboot ownership is explicit. Starting another detached process cannot provide that contract; the gateway service, installer service, uninstall, and lifecycle suites protect the selection and ownership boundaries.
Type of Change
Quality Gates
3b8176c43eand basef0f23ade57; independent human sensitive-path review remains outstanding.Documentation Writer Review
docs-updateddocs/get-started/prerequisites.mdx,docs/manage-sandboxes/uninstall-nemoclaw.mdx,docs/reference/architecture.mdx,docs/reference/commands.mdx,docs/reference/platform-support.mdx, anddocs/reference/troubleshooting.mdxat PR SHA3b8176c43against base SHAf0f23ade5. The retry commit has the same tree as its reviewed parenteb5144607; verified terminology, structure, product-scope boundaries, lifecycle and uninstall accuracy, recovery commands, and code-sample presentation againstWRITING.mdanddocs/CONTRIBUTING.md;git diff --checkpassed./root/pr7319_docs_review_3b817DGX Station Hardware Evidence
Not applicable;
scripts/prepare-dgx-station-host.shis unchanged.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailabled07a0576b: 8 authority tests passed; the macOS E2E command passed 9 tests with 2 environment-skipped. Credential-sanitization fix: E2E-support passed 1,595 tests with 12 skipped, and the focused live test collected successfully without executing credentialed actions. Exact refreshed head33bb483259: gateway-service runtime tests passed 21/21, installer service tests passed 9/9, polling-helper tests passed 3/3, sandbox-provisioning tests passed 62/62, the live test collected successfully, test-size and CLI type checks passed, the documentation build passed, and the complete canonical-base diff-aware hook gate passed. Exact headd5e9e2d916: all 8 GPU rollback tests passed with deterministic terminal-phase fixtures. Exact refreshed head2788106318: 82 focused CLI tests and 68 provisioning/uninstall integration tests passed, the documentation build passed, and normal pre-commit and commit-message hooks passed. Exact headea29c8b28b: all 7 gateway-service uninstall tests passed after the current-main fixture update, and normal pre-commit and commit-message hooks passed. Exact refreshed headda79a35c73: 45 CLI recovery/service tests and 10 process-recovery integration tests passed. Exact refreshed headb0e9c86ceb: 133 focused Perl-runtime, sandbox-provisioning, runner, and workflow-boundary tests passed outside the restricted sandbox; the canonical-base pre-commit, commit-message, and pre-push hook suites passed. Exact head9a3148c0aa: 20 focused lifecycle/E2E-support tests passed after formatter output, staged pre-commit/security hooks passed, canonical-base pre-push checks passed, and the docs build passed. Exact refreshed headed20dfd741: 49 focused lifecycle and E2E scorecard tests passed; canonical-base pre-commit/security, commit-message, and pre-push hooks passed; the docs build passed. Exact refreshed head34fbcfb4ad: 182 focused service, lifecycle, installer, and E2E scorecard tests passed outside the restricted sandbox; canonical-base pre-commit/security, commit-message, and pre-push hooks passed; the docs build passed. Exact refreshed head611b8fe05f: the expanded service, lifecycle, installer, Hermes, and workflow suite passed 259 tests with 12 expected skips; canonical-base pre-commit/security, commit-message, and pre-push hooks passed; the docs build passed. Exact refreshed head0a69b289ac: the expanded service, lifecycle, installer, Hermes, and workflow suite passed 261 tests with 12 expected skips; canonical-base pre-commit/security, commit-message, and pre-push hooks passed; the docs build passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: not run locally; the required full CI and exact-pair E2E gates are being monitored.npm run docsbuilds without warnings (doc changes only) — passed with 0 errors and 2 existing Fern warnings.Signed-off-by: San Dang sdang@nvidia.com