fix(installer): probe the user shim when the npm prefix has no CLI - #8440
fix(installer): probe the user shim when the npm prefix has no CLI#8440Dongni-Yang wants to merge 6 commits into
Conversation
verify_nemoclaw only reached the user-local shim check from inside the npm-prefix branch, so a host whose active npm prefix differs from the one the CLI was installed under -- a Homebrew npm shadowing the installer's nvm npm -- fell through to "binary not found" and exited 1 while the shim the installer had just created was working. The diagnostic even printed the shim path, implying it had been searched when it never was. Probe the shim directly after the npm-prefix branch. Resolution state comes from record_cli_resolution_state so the recovery export directory stays consistent with the sibling stale-PATH branch, then the branch re-checks what the name actually resolves to: the shim itself means the shell is ready, while a rejected binary still shadowing it keeps the PATH-refresh hint that record_cli_resolution_state would otherwise clear once the shim directory is on PATH. Without that re-check print_done() reports a clean install for a shell where the CLI name still runs the binary this function just refused. This clears the spurious exit 1 whenever a working shim exists. It does not explain why the PATH lookup failed on the reporter's host given that ensure_cli_shim calls refresh_path immediately after writing the shim; that remains open and is not reproducible without the macOS host. Refs #8311 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
|
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 (2)
📝 WalkthroughWalkthrough
ChangesCLI shim resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant verify_nemoclaw
participant npm_prefix
participant user_local_shim
participant PATH
verify_nemoclaw->>npm_prefix: Check installed CLI
verify_nemoclaw->>user_local_shim: Check fallback shim
verify_nemoclaw->>PATH: Resolve exact shim
PATH-->>verify_nemoclaw: Return matching or shadowing binary
verify_nemoclaw-->>verify_nemoclaw: Set readiness or PATH guidance
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit a724492 in the TypeScript / code-coverage/cliThe overall coverage in commit a724492 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@scripts/install.sh`:
- Around line 2244-2275: Update the user-facing installation documentation to
describe the shim fallback when the active npm prefix differs, including that
installation succeeds while emitting PATH-refresh guidance when the shell does
not resolve the shim. Document the relevant validation or verification for this
outcome, using the installer’s shim resolution and PATH-refresh behavior as the
reference.
🪄 Autofix
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: 5d1657e8-0649-42e9-ab67-b2a0065f7a48
📒 Files selected for processing (2)
scripts/install.shtest/install-npm-resolution.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
CI status: the 14 failures come from
|
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-8440.docs.buildwithfern.com/nemoclaw |
|
CI classification update: every required check is green for the current branch revision. Two non-required automation lanes failed for reasons outside this change:
Neither unchanged lane is safe or useful to rerun. The installer tests, docs build, required CI, DCO, commit verification, documentation review, and security review pass, and the actionable review thread is resolved. The remaining merge gate is independent maintainer approval. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Maintainer follow-up for the current branch revision (
Nine-category security review: PASS, no findings. (1) no secrets or credentials added; (2) the shim must be executable and pass the existing real-CLI version validation, with an explicit invalid-shim regression; (3) no authentication or authorization behavior changes; (4) no dependency changes; (5) existing failure diagnostics are retained and no sensitive data is logged; (6) no cryptographic changes; (7) no security configuration or header changes; (8) positive, shadowed, on-PATH, absent, and invalid shim paths are covered; (9) the holistic trust boundary is unchanged because execution remains limited to the existing user-local path and validated CLI identity. No CI waiver or merge bypass is requested. The PR should merge only after the fresh required checks pass and the required approval is present. |
|
CI follow-up: the fresh Nemotron advisor lane repeated the same external analysis failure on this branch revision. Setup, trusted checkout, isolated workspace preparation, OpenShell configuration, advisor execution, artifact download/upload, cleanup, and summary publication all passed; only This failure has now reproduced across two advisor runs while the PR code and the other advisor lane validate successfully, so another unchanged rerun is not justified. If this lane is required by the ruleset, its analyzer/output service is the remaining external CI blocker: https://github.com/NVIDIA/NemoClaw/actions/runs/31110751505/job/92647596999 |
<!-- markdownlint-disable MD041 --> ## Summary Fixes installer verification when npm prefix changes leave NemoClaw available only through the user-local shim. The installer now probes the shim after the npm-prefix checks and records whether the calling shell needs a PATH refresh. This replacement preserves Dongni Yang's original commit from #8440 and adds the missing regression coverage. ## Related Issue Closes #8311 ## Changes - Probe the user-local shim when the active npm prefix does not expose the CLI. - Re-check command resolution before recording readiness so a rejected binary that shadows the shim keeps the PATH-refresh guidance. - Cover stale-prefix, shadowing, shim-on-PATH, npm-uninstall, and missing-shim behavior. ## 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: `docs/reference/troubleshooting.mdx` already explains how to source the shell profile and add `~/.local/bin` to `PATH`. The removed installer failure was not documented. - [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: An independent review passed all nine security categories for PR commit `d2b1ea5139076b45bf366dcec04a3a75dd8cf49e` against base SHA `2f297843b8fddad80b046eaf34f2987bdd922711`. The existing shim trust boundary does not change. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: No waiver is recorded. ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: Reviewed `scripts/install.sh`, `test/install-npm-resolution.test.ts`, and the existing user-local shim recovery in `docs/reference/troubleshooting.mdx` for PR commit `d2b1ea5139076b45bf366dcec04a3a75dd8cf49e` against base SHA `2f297843b8fddad80b046eaf34f2987bdd922711`. The change removes an undocumented failure and retains the documented recovery action. The review covered repository terminology, test titles, comments, security wording, and documentation ownership; no documentation or code sample changed. - Agent: Codex Desktop <!-- docs-review-head-sha: d2b1ea5 --> <!-- docs-review-agents-blob-sha: c69aad4 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; `scripts/prepare-dgx-station-host.sh` does not change. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub — both contributor commits and the current-main refresh commit are GitHub Verified. - [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 — normal hooks passed for the contributor fix, remediation, and current-main refresh. - [ ] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — GitHub Actions is authoritative; no standalone local test suite ran for the reviewed replacement commit. - [ ] Applicable broad gate passed — GitHub Actions has not completed for the replacement PR. - [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) — not applicable; no documentation changes. - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) — not applicable; no documentation changes. - [ ] New doc pages include SPDX header and frontmatter (new pages only) — not applicable; no new page. --- Signed-off-by: Dongni Yang <dongniy@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved CLI detection when the active npm installation path does not contain the command. * Added support for valid user-local CLI shims. * Provides clearer onboarding guidance when PATH needs refreshing. * Correctly resolves valid shims and removes conflicting invalid command entries. * Reports a clear failure when no usable CLI installation is found. * **Tests** * Added coverage for local shim discovery, PATH handling, conflicting binaries, and missing installations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Dongni Yang <dongniy@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Dongni Yang <dongniy@nvidia.com>
2 participants
Summary
verify_nemoclawreached the user-local shim check only from inside the npm-prefix branch, so a host whose active npm prefix differs from the prefix the CLI was installed under exited 1 with "Installation failed: nemoclaw binary not found." while the shim the installer had just created was working. The installer now probes$NEMOCLAW_SHIM_DIR/$_CLI_BINafter the npm-prefix branch and continues through that path, so the reported install succeeds and still prints the PATH-refresh hint.Related Issue
Closes #8311
Changes
verify_nemoclawafter the npm-prefix branch, so the shim is reachable when the active npm prefix has no CLI. Previously the shim path was only printed in the "Searched:" diagnostic, which implied a probe that never ran.$_CLI_BINresolves to before recording state.record_cli_resolution_stateclearsNEMOCLAW_CURRENT_SHELL_NEEDS_PATH_REFRESHonce the shim directory is on the initial PATH, which is correct only when the name resolves to that shim. When a rejected binary still shadows it, the branch forces the refresh flag soprint_donekeeps emitting the hint instead of reporting a clean install for a shell where$_CLI_BINruns the refused binary. Theshadowedandprefix-placeholdercases intest/install-npm-resolution.test.tsprotect both arms.NEMOCLAW_READY_NOWstays false on the shadowed path, matching the documented meaning used by the sibling stale-PATH branch: the calling shell cannot resolve$_CLI_BINby name.test/install-npm-resolution.test.ts: the reported stale-prefix repro, a shadowed-shim case, a shim-on-PATH case, and a no-shim control that must still exit 1.Reachability note for review: this changes whether the installer aborts before onboarding runs. It does not change onboarding, preflight, credential, or policy code.
Type of Change
Quality Gates
Documentation Writer Review
docs-updatedDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run test/install-npm-resolution.test.ts→ 10 passed. Six installer suites that exerciseverify_nemoclaw→ 92 passed and 5 skipped (install-npm-resolution,install-station-controller-binding,install-preexisting-sandbox-recovery,install-openshell-upgrade-prompt,install-onboard-exit,install-onboard-yes).npm run typecheck:cliclean;shfmt -i 2 -ci -bnand ShellCheck clean.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Platform verification
Verified on the reported platform (macOS 26.5.2 (25F84) arm64, stock
/bin/bash3.2.57, Homebrew npm 11.12.1 at prefix/opt/homebrew— the condition described in #8311). A harness ran the pre-fixscripts/install.shfrom 422fe95 and this branch's version side by side on identical fixtures in a throwawayHOME:Both arms of the re-check were pinned by xtrace at
install.sh:2264:[[ '' -ef …/nemoclaw ]]when PATH resolves nothing, and[[ …/shadow/nemoclaw -ef …/.local/bin/nemoclaw ]]when a placeholder shadows the shim. Theno-shimandfake-shimcontrols still exit 1 with the original message.Scope caveat: the harness calls
verify_nemoclawdirectly, because in a full installfinish_nemoclaw_installcallsrefresh_pathbefore verification. It shows that the reported symptom, exit code, and message reproduce with the pre-fix installer and are gone with this change. It does not explain how the reporter's shell reached a state where the PATH lookup found nothing;ensure_cli_shimcallsrefresh_pathimmediately after writing the shim, so that precondition remains unexplained. This change makes the installer correct either way, because it probes the shim by absolute path rather than through PATH.Signed-off-by: Dongni Yang dongniy@nvidia.com
Summary by CodeRabbit
Bug Fixes
PATH.PATHwhen another binary shadows the expected CLI.Documentation
PATHrefreshes.Tests