fix(installer): remove nemohermes shim on uninstall - #6161
Closed
Dongni-Yang wants to merge 1 commit into
Closed
Conversation
removeNemoclawCli() only classified and removed paths.nemoclawShimPath (~/.local/bin/nemoclaw). The Hermes install variant additionally writes ~/.local/bin/nemohermes, but no path field or removal block existed for it, so the shim survived uninstall and kept resolving as a command. Add nemohermsShimPath to UninstallPaths / defaultUninstallPaths and add a matching classify-then-remove block in removeNemoclawCli, mirroring the existing nemoclaw shim handling including the preserve-foreign-file guard. Fixes #6098 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds a ChangesNemohermes shim cleanup during uninstall
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant RunUninstallPlan as runUninstallPlan
participant RemoveNemoclawCli as removeNemoclawCli
participant Filesystem
User->>RunUninstallPlan: trigger uninstall
RunUninstallPlan->>RemoveNemoclawCli: invoke with paths (includes nemohermsShimPath)
RemoveNemoclawCli->>RemoveNemoclawCli: classify nemohermsShimPath shim
alt shim removable
RemoveNemoclawCli->>Filesystem: rmSync(nemohermsShimPath)
else preserved foreign file
RemoveNemoclawCli->>RemoveNemoclawCli: log warning
end
RemoveNemoclawCli-->>RunUninstallPlan: cleanup result
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: None Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
PR Review Advisor — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
PR Review Advisor (Nemotron Ultra) — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/actions/uninstall/run-plan.ts (1)
682-695: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate classify/remove/warn logic for shim paths.
The nemoclaw and nemoherms blocks are structurally identical. Extracting a small helper would avoid copy-paste drift if more shims are added later.
♻️ Optional refactor
+function removeManagedShim(shimPath: string, runtime: UninstallRuntime): void { + const shim = classifyShimPath(shimPath); + if (shim.remove) removePath(shimPath, runtime); + else if (shim.kind === "preserve-foreign-file") { + runtime.warn(`Leaving ${shimPath} in place because it is not an installer-managed shim.`); + } +} + function removeNemoclawCli(paths: UninstallPaths, runtime: UninstallRuntime): void { ... - const shim = classifyShimPath(paths.nemoclawShimPath); - if (shim.remove) removePath(paths.nemoclawShimPath, runtime); - else if (shim.kind === "preserve-foreign-file") { - runtime.warn( - `Leaving ${paths.nemoclawShimPath} in place because it is not an installer-managed shim.`, - ); - } - const agentShim = classifyShimPath(paths.nemohermsShimPath); - if (agentShim.remove) removePath(paths.nemohermsShimPath, runtime); - else if (agentShim.kind === "preserve-foreign-file") { - runtime.warn( - `Leaving ${paths.nemohermsShimPath} in place because it is not an installer-managed shim.`, - ); - } + removeManagedShim(paths.nemoclawShimPath, runtime); + removeManagedShim(paths.nemohermsShimPath, runtime); removeNvmLeftovers(paths, runtime); removeAliases(paths, runtime); }🤖 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 682 - 695, The uninstall shim handling in run-plan has duplicated classify/remove/warn logic for nemoclaw and nemoherms, which should be consolidated. Extract a small helper around classifyShimPath, removePath, and runtime.warn that takes the shim path and runtime, then reuse it for both paths in run-plan so future shim additions don’t drift.src/lib/domain/uninstall/paths.ts (1)
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueField name is missing a letter ("nemoherms" vs "nemohermes").
nemohermsShimPathdrops the final "e" from "nemohermes", even though the value correctly resolves to thenemohermesshim path. Slightly confusing given the siblingnemoclawShimPathmatches its shim name exactly.✏️ Optional rename for consistency
- nemohermsShimPath: string; + nemohermesShimPath: string;and correspondingly at line 63, plus all call sites in
run-plan.tsand the test file.Also applies to: 63-63
🤖 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/domain/uninstall/paths.ts` at line 34, The field name in the uninstall paths types is misspelled as nemohermsShimPath and should be renamed to match nemohermes consistently. Update the corresponding definition and any related usage in the uninstall path helpers, then adjust all references in run-plan.ts and the associated test file to use the corrected symbol name so the shim path naming stays aligned with the actual nemohermes shim.
🤖 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 682-695: The uninstall shim handling in run-plan has duplicated
classify/remove/warn logic for nemoclaw and nemoherms, which should be
consolidated. Extract a small helper around classifyShimPath, removePath, and
runtime.warn that takes the shim path and runtime, then reuse it for both paths
in run-plan so future shim additions don’t drift.
In `@src/lib/domain/uninstall/paths.ts`:
- Line 34: The field name in the uninstall paths types is misspelled as
nemohermsShimPath and should be renamed to match nemohermes consistently. Update
the corresponding definition and any related usage in the uninstall path
helpers, then adjust all references in run-plan.ts and the associated test file
to use the corrected symbol name so the shim path naming stays aligned with the
actual nemohermes shim.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 941375ed-2fc1-46c2-b69b-9482a6c49ec7
📒 Files selected for processing (4)
src/lib/actions/uninstall/run-plan.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/domain/uninstall/paths.test.tssrc/lib/domain/uninstall/paths.ts
Contributor
Author
|
Closing as duplicate of #6101. |
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
nemoclaw uninstallremoved thenemoclawshim at~/.local/bin/nemoclawbut left the siblingnemohermesshim installed by the Hermes variant untouched, sonemohermeskept resolving as a command after a clean uninstall. This PR addsnemohermsShimPathtoUninstallPathsand extendsremoveNemoclawCli()to classify and remove it using the same installer-shim guard already applied to thenemoclawshim.Related Issue
Fixes #6098
Changes
src/lib/domain/uninstall/paths.ts— addnemohermsShimPath: stringtoUninstallPathsinterface and populate it as~/.local/bin/nemohermesindefaultUninstallPaths()src/lib/actions/uninstall/run-plan.ts— after the existingnemoclawshim block inremoveNemoclawCli(), add a matching classify-then-remove block forpaths.nemohermsShimPath, including thepreserve-foreign-filewarning for non-installer-managed filessrc/lib/domain/uninstall/paths.test.ts— assertnemohermsShimPath === ~/.local/bin/nemohermessrc/lib/actions/uninstall/run-plan.test.ts— red→green test: creates a real symlink attmpHome/.local/bin/nemohermes, runsrunUninstallPlan, assertsrmSyncis called for it and the symlink is goneType of Change
Quality Gates
nemohermesshim removal pathVerification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Signed-off-by: Dongni Yang dongniy@nvidia.com
Summary by CodeRabbit
nemohermescommand shim when appropriate.nemohermesshim location under the user’s local bin directory.