feat(uninstall): sweep every gateway port and report the ones left behind - #7901
Conversation
…hind Uninstall is scoped to one gateway port, so the other ports on a host stay bound with nothing to explain why. Name those environments and print the command that removes each one, and add --all-gateway-ports (or NEMOCLAW_UNINSTALL_ALL_GATEWAY_PORTS=1) to remove all of them in one run. Each port runs as its own uninstall so every port-scoped value resolves from that port, and the selected port runs last so its pass can still complete the host-shared cleanup. Signed-off-by: Tinson Lai <tinsonl@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:
📝 WalkthroughWalkthroughThe uninstall command now supports an opt-in sweep across all gateway-port environments. It processes the selected port last, reports remaining environments, preserves failed ports during cleanup, and documents the related flags and environment variable. ChangesGateway uninstall flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant InternalUninstallRunPlanCommand
participant runUninstallAllGatewayPorts
participant runUninstallPlan
participant GatewayEnvironmentInspection
User->>InternalUninstallRunPlanCommand: invoke --all-gateway-ports
InternalUninstallRunPlanCommand->>runUninstallAllGatewayPorts: dispatch uninstall options
runUninstallAllGatewayPorts->>GatewayEnvironmentInspection: enumerate gateway ports
runUninstallAllGatewayPorts->>runUninstallPlan: uninstall non-selected ports
runUninstallAllGatewayPorts->>runUninstallPlan: uninstall selected port last
runUninstallPlan-->>runUninstallAllGatewayPorts: return port result
runUninstallAllGatewayPorts-->>InternalUninstallRunPlanCommand: return aggregate exit code
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-7901.docs.buildwithfern.com/nemoclaw |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 489a368 in the TypeScript / code-coverage/cliThe overall coverage in commit 489a368 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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/lib/actions/uninstall/all-gateway-ports.ts (2)
85-98: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the child-pass
spawnSync.
defaultRunPortPassblocks indefinitely if a child uninstall pass hangs (e.g., a stalled Docker/OpenShell teardown call for a sibling port). Since this sweep is explicitly meant for whole-host/CI cleanup (#7791), an unbounded hang here stalls the entire sweep with no recourse short of killing the parent process.🕐 Proposed fix
return spawnExitCode( spawnSync(process.execPath, [entry, ...uninstallChildArgs(options)], { env: uninstallChildEnv(env, port), stdio: "inherit", + timeout: PORT_PASS_TIMEOUT_MS, }), );🤖 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/all-gateway-ports.ts` around lines 85 - 98, Update defaultRunPortPass so its spawnSync invocation enforces a finite timeout for the child uninstall pass, using the existing timeout configuration or an appropriate cleanup-specific limit. Preserve inherited stdio, environment, arguments, and exit-code handling while ensuring a hung child cannot block the host-wide sweep indefinitely.
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefault subprocess-spawning implementations lack direct test coverage. Every sweep test injects fakes for
runPortPass/listGatewayPorts, sodefaultRunPortPassanddefaultListGatewayPorts— the code paths actually used in production — are never exercised.
src/lib/actions/uninstall/all-gateway-ports.ts#L63-98: this is the untested production code (defaultListGatewayPorts,uninstallChildArgs,uninstallChildEnv,defaultRunPortPass).src/lib/actions/uninstall/all-gateway-ports.test.ts#L1-147: add a test that mocksnode:child_process'sspawnSync(and stubsprocess.argv[1]) to verifydefaultRunPortPassbuilds the expected command/env and mapsspawnSync's result correctly, plus a case for the missing-entry (process.argv[1]undefined) fallback.🤖 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/all-gateway-ports.ts` at line 1, Extend all-gateway-ports.test.ts with direct coverage for the production defaults defaultRunPortPass and defaultListGatewayPorts by mocking node:child_process.spawnSync and stubbing process.argv[1]. Verify defaultRunPortPass constructs the expected uninstall command and environment, maps spawnSync results correctly, and uses the missing process.argv[1] fallback; retain existing injected-fake sweep tests.
🤖 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 `@docs/manage-sandboxes/uninstall-nemoclaw.mdx`:
- Around line 37-43: Update the sibling-environment reporting text in the
uninstall documentation to clarify that removal commands are printed only when
an environment’s port is known. Preserve the existing name/reporting behavior
and explicitly account for remaining environments whose ports are unavailable or
unidentified.
- Around line 42-43: Update the two uninstall command examples in the documented
gateway-port section to use the shared CLI placeholder $$nemoclaw instead of the
literal nemoclaw, preserving the existing arguments and environment-variable
usage.
In `@docs/reference/commands.mdx`:
- Line 3427: Update the headings in docs/reference/commands.mdx lines 3427-3427
and docs/reference/troubleshooting.mdx lines 372-372 to title case: capitalize
“Every Gateway Port” and “Gateway Port Stays Bound After Uninstall or
Re-Onboard” respectively.
In `@docs/reference/host-files-and-state.mdx`:
- Around line 83-84: Update the uninstall documentation sentence describing
--all-gateway-ports and NEMOCLAW_UNINSTALL_ALL_GATEWAY_PORTS so it states that
every gateway port is removed, while shared host state is removed only when
--keep-openshell is not used and no failed or live port prevents final cleanup.
In `@docs/reference/troubleshooting.mdx`:
- Line 401: Update the “Uninstall NemoClaw” link in the troubleshooting
documentation to use the published Fern route
../manage-sandboxes/operate-sandboxes/uninstall-nemoclaw, preserving the
existing link text and surrounding content.
---
Nitpick comments:
In `@src/lib/actions/uninstall/all-gateway-ports.ts`:
- Around line 85-98: Update defaultRunPortPass so its spawnSync invocation
enforces a finite timeout for the child uninstall pass, using the existing
timeout configuration or an appropriate cleanup-specific limit. Preserve
inherited stdio, environment, arguments, and exit-code handling while ensuring a
hung child cannot block the host-wide sweep indefinitely.
- Line 1: Extend all-gateway-ports.test.ts with direct coverage for the
production defaults defaultRunPortPass and defaultListGatewayPorts by mocking
node:child_process.spawnSync and stubbing process.argv[1]. Verify
defaultRunPortPass constructs the expected uninstall command and environment,
maps spawnSync results correctly, and uses the missing process.argv[1] fallback;
retain existing injected-fake sweep tests.
🪄 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: e7c8ca0f-d69d-42ec-b2ba-e39aaf1fae77
📒 Files selected for processing (13)
ci/source-architecture-budget.jsondocs/manage-sandboxes/uninstall-nemoclaw.mdxdocs/reference/commands.mdxdocs/reference/host-files-and-state.mdxdocs/reference/troubleshooting.mdxsrc/commands/internal/uninstall/run-plan.tssrc/lib/actions/root-help.tssrc/lib/actions/uninstall/all-gateway-ports.test.tssrc/lib/actions/uninstall/all-gateway-ports.tssrc/lib/actions/uninstall/run-plan-other-gateway-report.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/tunnel/gateway-port-release.tsuninstall.sh
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
5 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
3 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
laitingsheng
added
NV QA
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/reference/commands.mdx`:
- Line 3430: Verify the uninstall reporter in the checked-in source and update
this documentation sentence to match its behavior: since the command is emitted
only for the first sibling gateway-port environment, say it provides a command
to remove one of them rather than each listed environment. Preserve the existing
references to naming the environments and the whole-host sweep.
🪄 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: 7d144147-4153-4301-9269-0f457537bd91
📒 Files selected for processing (8)
docs/manage-sandboxes/uninstall-nemoclaw.mdxdocs/reference/commands.mdxdocs/reference/host-files-and-state.mdxdocs/reference/troubleshooting.mdxsrc/lib/actions/uninstall/all-gateway-ports.test.tssrc/lib/actions/uninstall/all-gateway-ports.tssrc/lib/actions/uninstall/run-plan-other-gateway-report.test.tssrc/lib/actions/uninstall/run-plan.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/reference/host-files-and-state.mdx
- docs/reference/troubleshooting.mdx
- docs/manage-sandboxes/uninstall-nemoclaw.mdx
…orter Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Maintainer review at exact head |
|
Exact-head babysitting update for |
senthilr-nv
left a comment
senthilr-nv
left a comment
There was a problem hiding this comment.
Reviewed Files changed at commit 0a2837f. Product scope is approved, and the security and documentation findings are resolved. Approved.
|
Automated-review disposition for commit
This is recorded as an optional advisor infrastructure/timeout failure, not a required-check waiver and not evidence that the Nemotron review passed. Failed job: https://github.com/NVIDIA/NemoClaw/actions/runs/30834952807/job/91757748397 |
|
Product scope is approved independently of GitHub merge status for commit 60434b2. The approved surface is an explicit opt-in whole-host uninstall across discovered gateway ports; ordinary uninstall remains port-scoped. Discovery and incomplete cleanup fail closed, shared resources remain preserved when work is incomplete, external supervision and user-data preservation still apply, and completed independent cleanup is not rolled back. Decision record: #7791 (comment) |
|
Optional-check disposition for commit 60434b2: Nemotron 3 Ultra analysis did not complete in workflow run https://github.com/NVIDIA/NemoClaw/actions/runs/30866895968. It published no summary, review thread, or actionable finding. Repository policy treats this advisor as optional; the required GPT-5.6 Terra advisor passed, so this optional infrastructure failure is reported separately and is not a merge blocker. |
|
Product-scope approval applies to current commit aa46e8d on base 6a838ff. The signed upstream/main refresh left the reviewed PR patch byte-for-byte unchanged. The approved contract remains: ordinary uninstall is sticky and gateway-port scoped; the explicit opt-in whole-host sweep may remove all discovered gateway-port environments, fails closed on incomplete or unidentified cleanup, preserves shared resources when cleanup is incomplete, respects preservation and external-supervision controls, and does not roll back completed independent cleanup. This product decision is recorded independently of GitHub mergeStateStatus. |
|
Product-scope approval applies to current commit 134b2a3 on base 5f87afa. The signed upstream/main refresh left the reviewed PR patch byte-for-byte unchanged. The approved contract remains: ordinary uninstall is sticky and gateway-port scoped; the explicit opt-in whole-host sweep may remove all discovered gateway-port environments, fails closed on incomplete or unidentified cleanup, preserves shared resources when cleanup is incomplete, respects preservation and external-supervision controls, and does not roll back completed independent cleanup. This product decision is recorded independently of GitHub mergeStateStatus. |
|
Product-scope approval remains in force for current commit e493d7a on base 7e60e11. The approved contract is unchanged: ordinary uninstall remains port-scoped; the whole-host sweep is explicit opt-in, fails closed on incomplete or unidentified cleanup, preserves shared resources when work remains, respects preservation and external-supervision controls, and does not roll back completed independent cleanup. Decision record: #7791 (comment). This product decision is recorded independently of mergeStateStatus. |
|
Product-scope approval remains in force for current commit 27e0a86 on base 95a9d6f. The approved contract is unchanged: ordinary uninstall remains port-scoped; the whole-host sweep is explicit opt-in, fails closed on incomplete or unidentified cleanup, preserves shared resources when work remains, respects preservation and external-supervision controls, and does not roll back completed independent cleanup. Decision record: #7791 (comment). This product decision is recorded independently of mergeStateStatus. |
senthilr-nv
left a comment
senthilr-nv
left a comment
There was a problem hiding this comment.
Reviewed Files changed at commit 27e0a86. Product scope remains approved; the refreshed base does not change the intended uninstall behavior, and the security and documentation findings are resolved. Approved.
|
Product-scope decision for current commit |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
Product-scope decision and finding disposition for current commit |
5 participants
Summary
Adds an explicit whole-host uninstall mode for hosts with NemoClaw environments on more than one gateway port. Ordinary uninstall remains scoped to the selected port; the opt-in sweep processes discovered ports independently, keeps shared resources after any incomplete cleanup, and reports a nonzero outcome when work remains.
Related Issue
Related to #7791.
Changes
--all-gateway-portsandNEMOCLAW_UNINSTALL_ALL_GATEWAY_PORTS=1as explicit sweep controls while preserving the existing one-port default.--gatewaybefore cleanup and fail closed when gateway-process cleanup is incomplete or an unenumerated environment remains.Product Scope
approvedby the current maintainer in the PR preparation task on 2026-08-03.mergeStateStatus; it is not inferred from passing tests, review, or CI. The refresh to basea5562015029fd8cdbebdce5664e8b8bfda9d6ba8did not change the intended behavior; the stable patch ID and all six original PR commits remain unchanged.Automated Review Disposition
keyRemains:falseandselectedRootRemains:falsewith exit code 0.merge_as_iswith medium confidence, Nemotron recommendsmerge_as_iswith high confidence, and both canonical finding ledgers are empty. Their runtime-validation recommendation is covered by the required E2E gates tracked below.sweep,scoped uninstall,sibling gateway-port environment,host-global, andgateway portare used consistently with the current controlled vocabulary; neither advisor recorded a terminology finding.base64capability probe accepts FreeBSDbase64, so three tests reach a laterawkrejection instead of the expected early diagnostic; every invalid payload still fails nonzero. Linux CI is authoritative, and the probe should be tightened in a separate base follow-up rather than adding unrelated work here.server.close(); the bounded-response implementation and all feat(uninstall): sweep every gateway port and report the ones left behind #7901 interaction assertions pass. This teardown issue belongs in a separate base follow-up.Type of Change
Quality Gates
489a368a5d0ada868cd01c0dacfc1a99e64a15f1against basea5562015029fd8cdbebdce5664e8b8bfda9d6ba8; no PR-diff security finding, 112 focused tests passed, and unrelated base-test portability warnings were dispositioned above. This is an agent review, not a human sensitive-path owner review or hardware review.Documentation Writer Review
docs-updateddocs/manage-sandboxes/uninstall-nemoclaw.mdx,docs/reference/commands.mdx,docs/reference/host-files-and-state.mdx, anddocs/reference/troubleshooting.mdx; all 15 changed files and all 12 OpenClaw, Hermes, and Deep Agents generated pages were reviewed;npm run docspassed with 0 errors and 2 unchanged nonblocking Fern warnings; focused documentation/uninstall validation passed 28/28; the non-default-selected sweep reproduction removed the host-global dual-Station key.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: not applicable; the change is confined to uninstall coordination and uses focused CLI/integration suites plus repository hooks. CI is authoritative.npm run docsbuilds without warnings (doc changes only)Current Commit and Base Evidence
489a368a5d0ada868cd01c0dacfc1a99e64a15f1a5562015029fd8cdbebdce5664e8b8bfda9d6ba8e9b83f35b395d76409a14d4bd036f841841124a6from currentupstream/main, followed by signed fix commit489a368a5d0ada868cd01c0dacfc1a99e64a15f1; both raw commit objects contain SSH signatures.6e980dca7f59b4af87c4e402dffc2d0b447d3884; current binary-diff SHA-256f4c2ed0334f2490f5b7fa5471f88520f679544afcc32721f41913e3c5b18cf86. The prior six original PR commits remain unchanged in range-diff; the new fix commit resolves the fix(inference): recover dual-Station runtime ownership #8158 composition finding without rewriting existing history.Verified(verified=true, reasonvalid) after the non-force push.Signed-off-by: Tinson Lai tinsonl@nvidia.com