fix(installer): preserve npm lockfiles during install#3840
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 (1)
📝 WalkthroughWalkthroughAdds a CI "Validate npm lockfiles" dry-run step for root and ChangesLockfile Sync Validation and Installation Safety
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
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 |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
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 1383-1385: The pre_extract_openclaw() workaround is being
nullified because npm ci deletes node_modules; adjust the install flow so the
pre-extracted package is restored after any npm ci runs. Specifically, for the
blocks that run npm ci (the spin calls that execute "cd
\"$NEMOCLAW_SOURCE_ROOT\" && npm ci --ignore-scripts" and the similar plugin npm
ci under "$NEMOCLAW_SOURCE_ROOT"/nemoclaw), move or re-run
pre_extract_openclaw() after each npm ci (or add an explicit restore step that
copies the pre-extracted node_modules/openclaw into place) so that the openclaw
tarball extraction workaround is present before the subsequent build steps
(e.g., before npm run --if-present build:cli and before npm run build in the
plugin). Ensure you reference the existing pre_extract_openclaw() helper and the
spin invocations when making the change.
🪄 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: dcd60f47-41d0-44ba-9533-ae20ce150def
📒 Files selected for processing (4)
.github/actions/basic-checks/action.yamlscripts/install.shtest/install-preflight.test.tstest/lockfile-ci-guard.test.ts
Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: This advisory review used trusted metadata, the provided diff, and readonly repository inspection only; no PR scripts, tests, package-manager commands, Docker builds, or workflows were executed by this review.; PR titles, bodies, comments, branch names, and issue text were treated as untrusted evidence; acceptance mapping relies on trusted diff/test evidence where available.; The selective E2E success evidence in comments targets ceb0573, not the current head SHA f86c9f2.; The trusted changedFiles list does not include Full advisor summaryPR Review AdvisorBase: The npm-ci lockfile-preservation change is directionally good and tested with unit/shape coverage, but mergeability is BLOCKED and the E2E Advisor-required cloud-e2e job is not proven passing for head SHA f86c9f2. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
Selective E2E Results — ✅ All requested jobs passedRun: 26256277839
|
|
@chengjiew thanks for the fix here. This PR is blocked by commit signature requirements, so I opened #4029 with the same/squashed work from a signed commit and will close this one to unblock the change. For your next PR, please configure GitHub-verified commit signing before opening it so the required signature check passes on the original PR. GitHub docs: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits |
|
Closing in favor of #4029 because this PR is blocked by commit signature requirements. |
Pull request was closed
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.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
npx vitest run test/install-preflight.test.ts test/lockfile-ci-guard.test.ts -t 'uses npm ci|lockfile CI guards'npm run source-shape:checkbash -n scripts/install.sh && git diff --checknpm ci --ignore-scripts --dry-run && cd nemoclaw && npm ci --ignore-scripts --dry-rundocker run --rm -v "$PWD":/repo -w /repo node:22-trixie-slim sh -c 'npm ci --ignore-scripts --dry-run >/tmp/root.log && cd nemoclaw && npm ci --ignore-scripts --dry-run >/tmp/sub.log && echo linux_root_and_subdir_npm_ci_dry_run=PASS'Fixes #3798
Summary by CodeRabbit
Chores
Tests
Signed-off-by: Chengjie Wang chengjiew@nvidia.com