refactor(uninstall): run uninstall through internal command#3083
Merged
Conversation
This reverts commit 4ebeae4.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a Node.js-based uninstaller flow and three internal oclif commands (classify-shim, plan, run-plan); expands uninstall path shapes and plan actions to cover Docker containers/images and managed swap; replaces the large Bash uninstaller with a lightweight wrapper delegating to the Node uninstaller. ChangesUninstall Refactor: CLI Commands, Orchestration, and Path Resolution
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant RunPlanCmd as run-plan Command
participant Runtime as UninstallRuntime
participant Executor as executePlan
participant Docker as Docker
participant FS as Filesystem
participant Tools as System Tools
User->>RunPlanCmd: invoke with flags (--yes, --delete-models, --gateway, ...)
RunPlanCmd->>Runtime: buildRunPlan(options)
Runtime->>FS: resolve paths (shim, config, state, repoRoot)
Runtime->>Runtime: classifyShimPath()
Runtime-->>RunPlanCmd: { paths, plan }
RunPlanCmd->>Runtime: confirm(optional)
alt confirmed
RunPlanCmd->>Executor: executePlan(plan, runtime)
Executor->>Tools: stop helper services / kill processes
Executor->>FS: remove runtime temp globs, shims, binaries
Executor->>Docker: rm containers
Executor->>Docker: rmi images
Executor->>Docker: volume rm
Executor->>Tools: remove managed swap
Executor->>Tools: delete Ollama models (conditional)
Executor-->>RunPlanCmd: completion
else aborted
RunPlanCmd-->>User: "Aborted."
end
RunPlanCmd-->>User: exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/actions/uninstall-run-plan.ts`:
- Line 155: The interactive uninstall always aborts because buildRuntime
currently sets readLine to a no-op (() => null); update buildRuntime so the
readLine default is an actual prompt reader (e.g. a function that reads from
stdin/TTY or delegates to the CLI's readline helper) instead of returning null,
and ensure confirm() receives that real reader; adjust the same defaulting logic
referenced around lines 173-184 so both places use the real stdin prompt
implementation (identify the buildRuntime function and the readLine property
used when constructing the runtime and the confirm(...) call) so typed
confirmations are accepted when no explicit reader is injected.
- Around line 391-393: The success message is emitted unconditionally after
calling runtime.run for "swapoff" and "rm", so change the logic in
uninstall-run-plan.ts around the runtime.run calls (the runtime.run("sudo",
["swapoff", "/swapfile"], ...) and runtime.run("sudo", ["rm", "-f",
"/swapfile"], ...) lines) to check each command's result or catch exceptions
before logging; specifically run both commands in a try/catch (or inspect their
returned exit/status), ensure stdio or captured output is available for error
reporting instead of "ignore", and only call runtime.log("Swap file removed")
when both commands succeeded—otherwise log/throw an error with the command
output/details.
In `@src/lib/domain/uninstall/paths.ts`:
- Line 53: The default repoRoot currently uses path.resolve(__dirname, "..",
"..", "..") which resolves to the dist/ directory after compilation; update the
default in the repoRoot assignment (the options.repoRoot fallback) to go up one
additional directory (use path.resolve(__dirname, "..", "..", "..", "..")) so
compiled code in dist/lib/domain/uninstall resolves to the repository root and
not dist/, ensuring repo-relative assets like scripts/start-services.sh are
found.
🪄 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: 8be31edf-ed87-46f0-bcb8-9fe8f64d020e
📒 Files selected for processing (14)
src/commands/internal/uninstall/classify-shim.tssrc/commands/internal/uninstall/plan.tssrc/commands/internal/uninstall/run-plan.tssrc/lib/actions/uninstall-plan.tssrc/lib/actions/uninstall-run-plan.test.tssrc/lib/actions/uninstall-run-plan.tssrc/lib/domain/uninstall/paths.tssrc/lib/domain/uninstall/plan.test.tssrc/lib/domain/uninstall/plan.tssrc/lib/domain/uninstall/shims.tstest/gateway-cleanup.test.tstest/internal-cli.test.tstest/uninstall.test.tsuninstall.sh
Collaborator
Author
Automated PR review summaryReviewed PR #3083: refactor(uninstall): run uninstall through internal command Recommendation
Installation and setup findings
What was validated
Failing tests and unresolved impact
Passing tests and why they matteredPassing test 1: Internal uninstall plan exposes new TypeScript cleanup actions
Passing test 2: uninstall.sh remains a compatibility wrapper for internal run-plan
Passing test 3: classify-shim preserves foreign regular files while identifying managed symlinks
Passing test 4: Installed OpenShell sandbox remains reachable for real runtime inspection
Bottom line
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
prekshivyas
approved these changes
May 6, 2026
prekshivyas
left a comment
Contributor
There was a problem hiding this comment.
Solid bash→TS port of the uninstaller behind internal uninstall {plan,run-plan,classify-shim} (all hidden), with uninstall.sh reduced to a Node bootstrap wrapper. Public nemoclaw uninstall flag surface preserved. New plan actions (delete-related-docker-containers, -images, delete-managed-swap) modeled in the typed plan and tested. Five new unit tests in uninstall-run-plan.test.ts cover plan-build, dry-run with fakes, interactive-yes/no, and swap-disable failure. Stale bash-helper tests in test/uninstall.test.ts correctly removed in step with the helpers themselves; gateway-cleanup.test.ts updated to assert the TS plan.
Approving with the following undeclared behavior trims as follow-up nits — none are blockers, but they're worth tracking for a follow-up PR:
/etc/fstabcleanup is gone. Oldremove_nemoclaw_swapransudo sed -i ... /etc/fstabto strip/swapfileentries afterswapoff+rm. NewremoveManagedSwapskips this step → users may end up with dangling fstab entries causing boot-time warnings.managed_swapmarker content validation dropped. Old bash refused to act unless the marker file's contents equaled/swapfile. New TS only checks marker existence.stop_orphaned_openshell_processesnarrower. Old bash also pgrep'dsshprocesses whose cmdline matchedopenshell ssh-proxy/openshell-(introduced in #1940). New TS only matchesopenshell (sandbox create|ssh-proxy)directly — orphan ssh sessions won't be reaped.- Visual UX downgraded. Lost the ASCII-art banner, NVIDIA-green styling, spinner wrapper,
[N/6]step headers with─rules, and the styled goodbye footer. New output is plainconsole.log. Probably worth re-adding via the existingterminal-style.tshelper in a follow-up. - Bootstrap dependency.
uninstall.shnow requires eitherdist/nemoclaw.jsto exist ORnemoclawto be on PATH. Partial-install scenarios where the global package is half-removed could fail with exit 127. Probably fine for the common case but worth a fallback path.
PR description claims "preserving the public nemoclaw uninstall flow" — accurate at the flag/argument level, but the UX and the fstab/orphan-ssh trims should ideally be called out in the description so reviewers know what to expect.
CI: pr.yaml rollup checks pass (commit-lint, dco, layer-boundary, check-hash). Self-hosted: 3 prior runs success on pull-request/3083. macos-e2e / wsl-e2e / current self-hosted run still in progress at approval time.
This was referenced May 6, 2026
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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
Moves the uninstaller execution path behind hidden oclif-native internal commands and turns
uninstall.shinto a compatibility wrapper. The uninstall plan is now both inspectable as JSON and executable from TypeScript while preserving the publicnemoclaw uninstallflow.Changes
internal uninstall plan,internal uninstall run-plan, andinternal uninstall classify-shim.uninstall.shto a thin wrapper aroundnemoclaw internal uninstall run-plan.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Tests
Chores