refactor(cli): move dev shim install behind internal command - #3090
Conversation
This reverts commit 4ebeae4.
|
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 (8)
📝 WalkthroughWalkthroughThis PR refactors npm-link-or-shim functionality by simplifying a shell script wrapper to delegate to a new internal oclif command, which in turn calls a TypeScript module that attempts npm link with a fallback to creating a user-local bash shim. Domain utilities support shim classification and content generation, with comprehensive test coverage across all layers. Changesnpm-link-or-shim Refactoring
Sequence DiagramsequenceDiagram
actor User
participant Shell as scripts/npm-link-or-shim.sh
participant Node as Node.js
participant CLI as oclif CLI
participant Action as runNpmLinkOrShim()
participant npm as npm process
participant Shim as ~/.local/bin/nemoclaw
User->>Shell: Execute npm-link-or-shim.sh
Shell->>Shell: Validate Node & paths
Shell->>Shell: Set NEMOCLAW_INSTALLING guard
Shell->>Node: Exec Node with CLI JS
Node->>CLI: Load & route to internal dev command
CLI->>Action: Call runNpmLinkOrShim(repoRoot)
Action->>npm: Attempt npm link
npm-->>Action: Success (status 0)
Action-->>CLI: Return status 0
alt npm link fails
Action->>Action: Build shim contents (with marker & exec)
Action->>Shim: Write shim to ~/.local/bin/nemoclaw
Action->>Action: Validate PATH & permissions
Action-->>CLI: Return status 0 or error
end
CLI->>Node: Exit with status
Node->>User: Return exit code
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
cv
added
NemoClaw CLI
refactor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Automated PR review summaryReviewed PR #3090: refactor(cli): move dev shim install behind internal command Recommendation
Installation and setup findings
What was validated
Failing tests and unresolved impact
Passing tests and why they matteredPassing test 1: Wrapper fallback still creates a working dev shim
Passing test 2: Foreign user shim is not overwritten on fallback
Passing test 3: Internal command is routable but not exposed in top-level help
Bottom line
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Clean shell→TS migration matching the pattern in #3083 (uninstall) and #3088 (debug). scripts/npm-link-or-shim.sh reduced from 87→33 lines; the bash shim builder, foreign-file classifier, mktemp atomic-rename, PATH lookup, and NEMOCLAW_INSTALLING recursion guard are all reproduced in runNpmLinkOrShim at src/lib/actions/dev-shim.ts:362. The shim format (DEV_SHIM_MARKER + bash header + export PATH=\"$NODE_DIR:$PATH\" + exec \"$BIN_PATH\" \"$@\") is byte-identical to the old script.
Verified the package.json prepare chain still satisfies the new dist/ dependency: npm run build:cli runs before bash scripts/npm-link-or-shim.sh, so dist/nemoclaw.js exists by the time the wrapper exec's the internal command.
Net test coverage grew, not shrunk:
test/npm-link-or-shim.test.ts: 6→3 cases at the wrapper integration level (kept fallback-on-fail, no-shim-on-success, NEMOCLAW_INSTALLING-recursion-guard)src/lib/actions/dev-shim.test.tsadds back the foreign-overwrite-refusal, refresh-managed-shim, and broken-.local-dir cases at the unit level with injectable deps (6 cases)src/lib/domain/dev-shim.test.tsadds pure-function tests forclassifyDevShim/buildDevShimContents/pathContainsDirectory(3 cases)test/internal-cli.test.tsadds the oclif routing assertion forinternal dev npm-link-or-shim --help
Total: 9 new TS unit tests + 1 routing test in exchange for 3 removed bash-driven integration tests. Cleaner isolation, no regressions.
Tiny nit (non-blocking, deliberate): Old bash script could run npm link directly with only bin/nemoclaw.js present — useful if a dev manually invoked the script outside npm install. New wrapper requires dist/nemoclaw.js to exist first; if missing, logs Run npm run build:cli and retry. and exits 0. The npm prepare flow guarantees this, but standalone invocation now needs the build step. Acceptable contract change since the script header explicitly documents "invoked by npm install via package.json prepare".
CI: pr.yaml rollup checks pass (commit-lint, dco, layer-boundary, check-hash, CodeRabbit, changes). Self-hosted: 1 prior success on pull-request/3090. build-sandbox-images / macos-e2e / wsl-e2e / current self-hosted run still in progress at approval time.
wscurran
added
area: cli
3 participants
Summary
Moves the
npm link/ user-local dev shim fallback out ofscripts/npm-link-or-shim.shand into a hidden oclif internal command. The shell script is now a small source-checkout wrapper while shim classification and writing are covered by TypeScript unit tests.Changes
nemoclaw internal dev npm-link-or-shimfor dev checkout CLI exposure.npm linkfallback behavior.scripts/npm-link-or-shim.shto node/compiled-CLI resolution and delegation.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
~/.local/bin/nemoclawto ensure accessibility in restricted environments.Tests