fix(uninstall): remove agent-alias CLI shims (nemohermes, nemo-deepagents) (#6098) - #6101
Merged
Merged
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 during nemoclaw uninstall. The uninstall path model, shim classification helpers, run-plan flow, and tests all propagate alias bin names alongside the original nemoclaw shim. ChangesAgent-alias shim removal
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant RunPlan as run-plan.ts
participant Paths as paths.ts
participant Shims as shims.ts/plan.ts
User->>RunPlan: runUninstallPlan()
RunPlan->>Paths: get agentAliasShimPaths
Paths-->>RunPlan: [{binName, path}, ...]
loop for each alias shim
RunPlan->>Shims: classifyShimPath(path, deps, binName)
Shims-->>RunPlan: classification (managed-wrapper / preserve-foreign-file)
alt managed-wrapper
RunPlan->>RunPlan: remove shim
else preserve-foreign-file
RunPlan->>User: warn, leave shim in place
end
end
RunPlan-->>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 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
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: 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 — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 3 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
…ppers (#6098) The existing agent-alias test (#6098) only creates symlinks, which classify via classifyShimPath's metadata fast path. It does not exercise the fd-read branch that reads the wrapper contents and matches against the per-alias binName — the new behavior this PR introduces. Add a second run-plan test that writes real installer-managed wrapper files (with the correct 'exec ..../{binName}' line) at each alias path and asserts both are removed. CodeRabbit follow-up on #6101. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/actions/uninstall/run-plan.test.ts (1)
165-251: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deduplicating the two alias-shim uninstall tests.
This new test (lines 204-251) duplicates nearly all boilerplate from the adjacent symlink test (lines 165-202) — temp dir setup, runtime options object, assertions, and teardown — differing only in how the shim artifact is created (symlink vs. wrapper script). Extracting a shared helper that takes a
createShim(path, binName)callback would reduce duplication and keep both cases easy to maintain as more alias binaries are added.Separately, since the fixtures already use the real filesystem (
symlinkSync,writeFileSync), consider asserting on real post-run filesystem state (fs.existsSync(hermesShim)etc.) in addition to/instead of the mockedrmSynccall capture — this exercises the same publicrunUninstallPlanentrypoint while reducing reliance on mock-call assertions. That said, this repo already uses injectedrmSyncfor testability per its actions/adapters layering, so the current approach is consistent with existing conventions and not a functional problem.The static-analysis path-traversal warnings on lines 219-221 are false positives — the paths originate from
mkdtempSync-generated temp directories combined with hardcoded bin names, not external input.🤖 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 - 251, The two alias-shim uninstall tests in run-plan.test.ts are duplicating the same setup, options, assertions, and cleanup, differing only in how the shim is created. Refactor them by extracting a shared helper around runUninstallPlan that accepts a createShim(path, binName) callback (or similar) and reuse it for both the symlink and wrapper-script cases. While touching the tests, consider asserting the real filesystem state after runUninstallPlan in addition to the injected rmSync capture, using the existing hermesShim and deepagentsShim fixtures to keep the checks focused and maintainable.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-251: The two alias-shim uninstall tests in run-plan.test.ts
are duplicating the same setup, options, assertions, and cleanup, differing only
in how the shim is created. Refactor them by extracting a shared helper around
runUninstallPlan that accepts a createShim(path, binName) callback (or similar)
and reuse it for both the symlink and wrapper-script cases. While touching the
tests, consider asserting the real filesystem state after runUninstallPlan in
addition to the injected rmSync capture, using the existing hermesShim and
deepagentsShim fixtures to keep the checks focused and maintainable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a13f1847-7bad-4929-9ce5-1bb9d028e561
📒 Files selected for processing (1)
src/lib/actions/uninstall/run-plan.test.ts
cv
approved these changes
Jul 3, 2026
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
…ents) (NVIDIA#6098) (NVIDIA#6101) ## Summary `nemoclaw uninstall` removed the `nemoclaw` CLI shim, the `openshell*` 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 NVIDIA#6098 ## Changes - `src/lib/domain/uninstall/shims.ts`: `classifyNemoclawShim` / `isInstallerManagedWrapperContents` accept an optional `binName` (default `nemoclaw`) so a wrapper that execs `…/nemohermes` is recognized as installer-managed. - `src/lib/domain/uninstall/paths.ts`: add `agentAliasShimPaths` (`nemohermes`, `nemo-deepagents`) under the same bin dir. - `src/lib/actions/uninstall/plan.ts`: `classifyShimPath` threads `binName` through both the fd-read and metadata paths. - `src/lib/actions/uninstall/run-plan.ts`: `removeNemoclawCli` now 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 the `nemoclaw` shim. - Tests: per-bin-name classification (managed wrapper matched by its own name, and a `nemoclaw` wrapper is *not* treated as a managed `nemohermes` shim); 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 nemoclaw` step. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: completes existing uninstall cleanup; no command/flag surface change. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: uninstall/file-deletion path. Removal reuses the pre-existing `classifyNemoclawShim` guard (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. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the style guide (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Jason Ma <jama@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Uninstall now removes supported CLI alias shims alongside the main binary. * Shim detection now supports alias wrappers for different command names. * **Bug Fixes** * Improved shim classification and uninstall handling for missing files, symlinks, and other edge cases involving aliases. * Preserved “foreign” shims are now recognized more reliably when alias wrappers are involved. * **Tests** * Added coverage for removing alias shims and correctly classifying alias wrapper contents. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Jason Ma <jama@nvidia.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Charan Jagwani <cjagwani@nvidia.com>
This was referenced Aug 18, 2026
prekshivyas
added a commit
that referenced
this pull request
Aug 19, 2026
) ## Summary `nemoclaw uninstall` reports success while leaving the NemoClaw CLI binaries installed under every nvm node version other than the one selected at uninstall time. `removeNvmLeftovers` matched only `entry.isFile()` on a path ending in `bin/nemoclaw`, but `fs.readdirSync(…, { withFileTypes: true })` uses `lstat` semantics and npm publishes every declared bin as a symlink, so that branch never ran. It also only matched `nemoclaw`, never the `nemohermes` and `nemo-deepagents` bins that #6098 taught the `~/.local/bin` sweep about. After this change the sweep removes all three names whether they are regular files or symlinks, so the commands stop resolving. ## Related Issue Fixes #9500 ## Changes - `src/lib/actions/uninstall/run-plan.ts` — `removeNvmLeftovers` enumerates each direct Node-version directory under `$NVM_DIR/versions/node` and inspects only `<version>/lib/node_modules` and `<version>/bin`, instead of walking the whole tree with `entry.isFile() && target.endsWith(…)`. Alias names come from `paths.agentAliasShimPaths`, so no new module edge or constant is introduced. - `src/lib/actions/uninstall/run-plan.test.ts` — extended the existing `removes agent-alias CLI shims … (#6098)` case rather than adding a parallel one. No abstraction, configuration, fallback, migration, or compatibility path is added. ### Scope of deletion Only an entry named exactly `nemoclaw`, `nemohermes`, or `nemo-deepagents` that is a regular file or a symlink, whose parent is the `bin` directory of a direct child of `$HOME/.nvm/versions/node`. Deletion stays `rmSync(target, { force: true })` with no `recursive`, so a symlink is unlinked and never followed. A nested `<version>/lib/node_modules/<package>/bin/<name>` is not reachable — the sweep never descends there. The module branch still matches a real directory named `nemoclaw` directly under `<version>/lib/node_modules`, so an `npm link`-ed source checkout, which is a symlink, is left in place. `node`, `npm`, other global packages' bins, and the user's nvm installation are untouched. The regression test asserts the exact set of created paths that the run removes, including three foreign package bins and an unrelated `<version>/bin/tsc` that must survive. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run --project cli src/lib/actions/uninstall/run-plan.test.ts` → 40 passed. The extended `#6098` case fails on `main` (the three `.nvm/versions/node/v22.19.0/bin/*` paths are absent from the removed set) and passes with this change. `npm run typecheck:cli` and `npx oxlint src/lib/actions/uninstall/` are clean. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the style guide (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) ### Notes for reviewers - `src/lib/actions/uninstall/run-plan.test.ts` goes 1495 → 1500 lines, exactly at `ci/test-file-size-budget.json`'s `defaultMaxLines`. The existing setup comment was re-worded in place to pay for the added lines inside the same case, so no new scenario or helper was created. - Prior art: #6098 / #6101 fixed the same class for `~/.local/bin` and introduced `AGENT_ALIAS_CLI_BINARIES`, which this change reuses. #7905 is a related open report on a different code path (gateway scoping on a Homebrew node) and is intentionally not closed here. --- Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved uninstall reliability when sandbox deletion reports that a resource is already absent. * Portable cleanup now completes in the correct order and avoids unnecessary work for non-portable installations. * Improved removal of NVM-related CLI links and packages, including broken links. * Uninstall now preserves unrelated packages, binaries, linked packages, and files across Node.js versions. * Cleanup safely skips inaccessible directories while allowing uninstall to finish successfully. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
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
4 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