fix(uninstall): remove agent-alias CLI shims (nemohermes, nemo-deepagents) (#6098)#6101
Open
jason-ma-nv wants to merge 1 commit into
Open
fix(uninstall): remove agent-alias CLI shims (nemohermes, nemo-deepagents) (#6098)#6101jason-ma-nv wants to merge 1 commit into
jason-ma-nv wants to merge 1 commit into
Conversation
…ents) (#6098) nemoclaw uninstall removed the nemoclaw shim but left the sibling nemohermes / nemo-deepagents shims in ~/.local/bin, so those commands still resolved after a clean uninstall. Generalize the shim classifier to match a managed wrapper by bin name (classifyNemoclawShim/isInstallerManagedWrapperContents/classifyShimPath accept binName, default nemoclaw), enumerate the alias shim paths, and remove them in removeNemoclawCli using the same classification guard so non-managed files of that name are preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Jason Ma <jama@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds support for classifying and removing sibling agent-alias CLI shims (nemohermes, nemo-deepagents) during nemoclaw uninstall. Introduces a ChangesAgent-alias shim removal
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant RunUninstallPlan as run-plan.ts
participant Paths as paths.ts
participant Shims as shims.ts/plan.ts
User->>RunUninstallPlan: runUninstallPlan()
RunUninstallPlan->>Paths: get agentAliasShimPaths
Paths-->>RunUninstallPlan: [{binName, path}, ...]
loop for each alias shim
RunUninstallPlan->>Shims: classifyShimPath(path, deps, binName)
Shims-->>RunUninstallPlan: classification (managed-wrapper / preserve-foreign-file)
alt managed-wrapper
RunUninstallPlan->>RunUninstallPlan: remove shim
else preserve-foreign-file
RunUninstallPlan->>User: warn, leave shim in place
end
end
RunUninstallPlan-->>User: uninstall result
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 68%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
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 — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 2 items to resolve/justify, 0 in-scope improvements
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/actions/uninstall/run-plan.test.ts (1)
165-203: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest only exercises the symlink path, not the new binName-aware wrapper classification.
Both alias shims here are plain symlinks, which
classifyShimPathresolves via theisSymlinkbranch — a path that doesn't depend onbinNameat all. This test doesn't actually prove thatremoveNemoclawCli's newclassifyShimPath(alias.path, {}, alias.binName)call correctly wiresbinNamethrough toisInstallerManagedWrapperContentsfor a wrapper-style shim; only the pure-function unit tests inshims.test.tscover that. Consider adding a wrapper-script variant (installer-managed wrapper content, per-bin) alongside the symlink case to close this gap end-to-end.As per path instructions, tests should provide "behavioral confidence rather than implementation lock-in" and flag "conditionals that make a test pass without exercising its claim."
🤖 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.test.ts` around lines 165 - 203, The uninstall test only covers plain symlink cleanup and does not exercise the new binName-aware wrapper classification path. Update the test around runUninstallPlan/removeNemoclawCli to include a wrapper-script fixture with installer-managed contents and a specific binName so classifyShimPath(alias.path, {}, alias.binName) is actually validated end-to-end, while keeping the existing symlink case as a separate behavioral check. Use the existing symbols removeNemoclawCli, runUninstallPlan, and classifyShimPath to locate the right test area.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 `@src/lib/actions/uninstall/run-plan.test.ts`:
- Around line 165-203: The uninstall test only covers plain symlink cleanup and
does not exercise the new binName-aware wrapper classification path. Update the
test around runUninstallPlan/removeNemoclawCli to include a wrapper-script
fixture with installer-managed contents and a specific binName so
classifyShimPath(alias.path, {}, alias.binName) is actually validated
end-to-end, while keeping the existing symlink case as a separate behavioral
check. Use the existing symbols removeNemoclawCli, runUninstallPlan, and
classifyShimPath to locate the right test area.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e1d32049-1624-44c5-a215-f255824b862f
📒 Files selected for processing (6)
src/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/shims.test.tssrc/lib/domain/uninstall/shims.ts
wscurran
added
area: install
Contributor
wscurran
added
integration: hermes
21 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
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 thenemoclawCLI shim, theopenshell*binaries, and~/.nemoclaw/, but left the sibling agent-alias shims (nemohermes,nemo-deepagents) in~/.local/bin. After a "clean" uninstall those commands still resolved and reported a version. This removes them too, using the same installer-managed-shim safety classification.Related Issue
Fixes #6098
Changes
src/lib/domain/uninstall/shims.ts:classifyNemoclawShim/isInstallerManagedWrapperContentsaccept an optionalbinName(defaultnemoclaw) so a wrapper that execs…/nemohermesis recognized as installer-managed.src/lib/domain/uninstall/paths.ts: addagentAliasShimPaths(nemohermes,nemo-deepagents) under the same bin dir.src/lib/actions/uninstall/plan.ts:classifyShimPaththreadsbinNamethrough both the fd-read and metadata paths.src/lib/actions/uninstall/run-plan.ts:removeNemoclawClinow also classifies and removes each alias shim — reusing the existing guard, so a non-managed file of that name (foreign file) is preserved with a warning, exactly like thenemoclawshim.nemoclawwrapper is not treated as a managednemohermesshim); an end-to-end run-plan test that creates managed alias symlinks and asserts both are removed.Safe by construction: symlinks and installer-managed wrappers are removed; any other regular file at those paths (e.g. an unrelated user script) is preserved. npm-installed alias bins remain handled by the existing
npm uninstall -g nemoclawstep.Type of Change
Quality Gates
classifyNemoclawShimguard (only symlinks + installer-managed wrappers removed; foreign files preserved), scoped to two fixed bin names in the resolved bin dir. 53 uninstall tests pass (51 pre-existing + 2 new), including a foreign-file-preserved assertion.Verification
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: Jason Ma jama@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests