fix: install package CLI alias shims - #6084
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdates the installer to create shims for all packaged CLI aliases, and expands the npm-resolution test suite with cross-platform Bash handling, path normalization, and assertions for alias shim creation and platform-specific behavior. ChangesMulti-alias shim installation
Estimated code review effort: 2 (Simple) | ~12 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
4d0663f to
0744ea7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/install-npm-resolution.test.ts`:
- Around line 99-144: The shim coverage in install-npm-resolution.test.ts only
checks that each alias created by ensure_nemoclaw_shim is executable, but it
does not verify that every shim points to its own packaged binary. Update the
test to assert the generated shim content for nemoclaw, nemohermes, and
nemo-deepagents embeds the correct target path under prefixBin for each alias,
following the install-preflight.test.ts content-check pattern so a miswired
alias cannot pass.
- Around line 14-24: The resolveBashBin() helper assumes spawnSync("where.exe",
["bash"]) always returns a string stdout, but on spawn failure it can be null
and the split call will throw. Update resolveBashBin() to defensively handle a
null or missing stdout before splitting, and only use the discovered path when
stdout is present; otherwise fall back to "bash" as intended.
🪄 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: 0b34f559-9c00-4206-ad60-2d4ca00c253a
📒 Files selected for processing (2)
scripts/install.shtest/install-npm-resolution.test.ts
0744ea7 to
bc9cfef
Compare
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 `@test/install-npm-resolution.test.ts`:
- Around line 27-39: Guard buildIsolatedSystemPath() in
test/install-npm-resolution.test.ts so Windows symlink creation failures do not
abort the test file at module load. Update the symlinkSync handling inside
buildIsolatedSystemPath() (and any related setup that builds TEST_SYSTEM_PATH)
to treat EPERM like EEXIST, or skip symlink creation on win32 when privileges
are unavailable, while still continuing to build the isolated path for the
remaining entries.
🪄 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: 5dcaf488-4ba2-4499-904c-2ca3a8701298
📒 Files selected for processing (2)
scripts/install.shtest/install-npm-resolution.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/install.sh
bc9cfef to
53ceedc
Compare
|
✨ Thanks for the fix. Adding user-local shims for all packaged CLI aliases and regression coverage for the default install path is a solid improvement for Hermes mode. Related open issues: |
Replay the validated contributor change onto current main and apply the repository formatter output. Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ea0eaf4 to
01c1cac
Compare
ericksoa
left a comment
There was a problem hiding this comment.
Reviewed exact replayed head 01c1cac. No blocking findings. The installer alias behavior is covered by focused tests, all 34 check contexts are green, DCO and GitHub verification pass, CodeRabbit is clean, and both manual PR Review Advisor lanes recommend merge_as_is.
<!-- markdownlint-disable MD041 --> ## Summary Fix the source installer so every packaged NemoClaw CLI alias is made available through the user-local shim directory. This restores `nemohermes` after a default `nemoclaw` install while preserving the active CLI's failure semantics. ## Related Issue Closes NVIDIA#6041. ## Changes - Create best-effort user-local shims for `nemoclaw`, `nemohermes`, and `nemo-deepagents` after installing the active CLI. - Verify that every generated alias shim points to its corresponding packaged binary. - Keep the installer test fixture portable across POSIX and Windows development shells. - Replay the validated contributor change onto current `main` with repository formatter output applied. ## 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 <!-- Check all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [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 — installer restores already-packaged CLI aliases without changing the documented interface - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — installer shim targets and best-effort failure boundaries reviewed; focused alias tests pass 5/5 - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [x] PR description includes the DCO sign-off declaration and every replayed commit is expected to appear as `Verified` in GitHub - [ ] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes — formatting and read-only hooks passed; the broad local coverage lane did not settle under shared host load, so exact-head CI remains required - [x] Targeted tests pass for changed behavior — `npx vitest run --project integration test/install-npm-resolution.test.ts` (5/5) - [ ] 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](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Installer now creates local command shims for all bundled CLI aliases, so more entry points are available after setup. * **Bug Fixes** * Improved installer behavior across platforms, including better Windows support and more reliable path handling. * Fallback installation paths are now reported more consistently when npm isn’t available. * **Tests** * Expanded installer coverage for alias creation, fallback behavior, and cross-platform path normalization. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Summary
Fix the source installer so every packaged NemoClaw CLI alias is made available through the user-local shim directory. This restores
nemohermesafter a defaultnemoclawinstall while preserving the active CLI's failure semantics.Related Issue
Closes #6041.
Changes
nemoclaw,nemohermes, andnemo-deepagentsafter installing the active CLI.mainwith repository formatter output applied.Type of Change
Quality Gates
Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpasses — formatting and read-only hooks passed; the broad local coverage lane did not settle under shared host load, so exact-head CI remains requirednpx vitest run --project integration test/install-npm-resolution.test.ts(5/5)npm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Signed-off-by: WilliamK112 164879897+WilliamK112@users.noreply.github.com
Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit