fix(installer): preserve npm lockfiles during install#4029
Open
jyaunches wants to merge 1 commit into
Open
Conversation
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThis PR resolves lockfile drift in the Changesnpm ci & lockfile validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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-preflight.test.ts`:
- Around line 98-100: The test harness function writeNpmStub and the
installSnippet handlers are not accounting for npm "ci" invocations, causing
many tests to hit the "unexpected npm invocation: ci --ignore-scripts" path;
update writeNpmStub (and any installSnippet blocks used in
test/install-preflight.test.ts) to accept and handle the "ci" command the same
way as "install" (e.g., add "$1" = "ci" branches or normalize "$1" to "install"
inside the stub) so existing test-specific handlers match the new installer
behavior and no longer reject npm ci.
🪄 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: 175adef9-a9c5-45a2-b6ec-9bed3c4f2f55
📒 Files selected for processing (4)
.github/actions/basic-checks/action.yamlscripts/install.shtest/install-preflight.test.tstest/lockfile-ci-guard.test.ts
Comment on lines
+98
to
100
| if [ "$1" = "ci" ] || [ "$1" = "install" ] || [ "$1" = "link" ] || [ "$1" = "uninstall" ] || [ "$1" = "pack" ] || [ "$1" = "run" ]; then | ||
| ${installSnippet} | ||
| fi |
Contributor
There was a problem hiding this comment.
writeNpmStub() still leaves many test-specific handlers rejecting npm ci.
Line 98 now dispatches ci, but a lot of callers in this file still only branch on "$1" = "install" inside installSnippet. Those tests will now fall through to unexpected npm invocation: ci --ignore-scripts, so the suite is still out of sync with the installer change.
🤖 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 `@test/install-preflight.test.ts` around lines 98 - 100, The test harness
function writeNpmStub and the installSnippet handlers are not accounting for npm
"ci" invocations, causing many tests to hit the "unexpected npm invocation: ci
--ignore-scripts" path; update writeNpmStub (and any installSnippet blocks used
in test/install-preflight.test.ts) to accept and handle the "ci" command the
same way as "install" (e.g., add "$1" = "ci" branches or normalize "$1" to
"install" inside the stub) so existing test-specific handlers match the new
installer behavior and no longer reject npm ci.
Contributor
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: This review used the supplied trusted metadata and read-only file inspection only; it did not execute tests, npm commands, installer scripts, or E2E workflows.; The linked issue #3798 acceptance includes lockfile regeneration provenance; the PR diff does not include nemoclaw/package-lock.json, so resync provenance could not be confirmed from changed files.; E2E Advisor reported required cloud jobs were auto-dispatched, but the supplied status rollup did not include their pass/fail conclusions.; The exact unit-vitest-linux failure log was not provided; the test-stub finding is based on diff, grep evidence, unresolved review thread, and failed context metadata.; Open PR overlaps were identified from trusted metadata, but their diffs were not reviewed here. Full advisor summaryPR Review AdvisorBase: Installer and CI lockfile direction is appropriate, but merge is blocked by mergeStateStatus=BLOCKED, an unresolved review thread, a unit-vitest-linux failure tied to stale npm stubs, and missing pass evidence for required cloud E2Es at the head SHA. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26262466547
|
cv
approved these changes
May 22, 2026
12 tasks
wscurran
added
fix
NemoClaw CLI
Contributor
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
3 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
npm installtonpm cifor both the root package and nestednemoclaw/sandbox payload.npm cifor both lockfiles before the install step.npm ci --ignore-scriptswithout global GitHub installs.Supersedes #3840.
Fixes #3798.
Repro
Issue #3798 reproduces when a host-side install mutates the nested sandbox lockfile before the Linux Docker build. On macOS with npm 11.6.2,
cd nemoclaw && npm install --ignore-scriptsprunes Linux-only optional@emnapi/*entries fromnemoclaw/package-lock.json; the subsequent Linuxnode:22-trixie-slimnpm cithen fails with the missing@emnapi/core/@emnapi/runtimeerrors described in the issue.Test Plan
bash -n scripts/install.shgit diff --checknpm test -- test/install-preflight.test.ts test/lockfile-ci-guard.test.ts -t "uses npm ci|lockfile CI guards"(blocked locally:vitest: command not found; this worktree has nonode_modules)Summary by CodeRabbit