fix(installer): preserve sandbox payload lockfile#6628
Open
chengjiew wants to merge 3 commits into
Open
Conversation
Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
|
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. |
Contributor
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe installer now uses Changesnpm ci lockfile handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the branch. TypeScript / code-coverage/cliThe overall coverage in the branch remains at 78%, unchanged from the branch. Show a code coverage summary of the most impacted files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 29077214367
|
![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 605-606: The assertions in the relevant install preflight test
only verify npm command invocations, not lockfile preservation. Extend the test
around the existing log assertions to read and hash nemoclaw/package-lock.json
before and after the operation, then assert the hashes match, using the test’s
existing filesystem and hashing utilities where available.
🪄 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: 6a74ef1f-1498-4afc-8609-58940c7620bb
📒 Files selected for processing (4)
.github/actions/ci-static-checks/action.yamlscripts/install.shtest/install-preflight.test.tstest/pr-workflow-contract.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/install-preflight.test.ts (1)
576-608: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAdd equivalent lockfile coverage for the GitHub-clone path.
This test forces the source-checkout path and explicitly verifies that the GitHub install URL is absent. The PR objective requires preserving the nested lockfile for both source-checkout and GitHub-clone installations; add the same sentinel-preservation assertion to a clone-mode case, or confirm that an existing test already covers it.
As per path instructions, keep this regression test focused on observable behavior at the public installer boundary.
🤖 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 576 - 608, Add equivalent nested package-lock.json sentinel-preservation coverage for the GitHub-clone installation path. Locate the existing clone-mode installer test, add a sentinel under the cloned payload’s nemoclaw directory before invoking the installer, then assert the file remains unchanged afterward alongside the existing observable installer assertions; avoid testing internal implementation details.Source: Path instructions
🤖 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.
Nitpick comments:
In `@test/install-preflight.test.ts`:
- Around line 576-608: Add equivalent nested package-lock.json
sentinel-preservation coverage for the GitHub-clone installation path. Locate
the existing clone-mode installer test, add a sentinel under the cloned
payload’s nemoclaw directory before invoking the installer, then assert the file
remains unchanged afterward alongside the existing observable installer
assertions; avoid testing internal implementation details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f69bd5a4-b428-4d38-a388-36dfcced187c
📒 Files selected for processing (1)
test/install-preflight.test.ts
Contributor
E2E Target Results — ✅ All selected jobs passedRun: 29139480097
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
E2E Target Results — ✅ All selected jobs passedRun: 29140037914
|
Collaborator
|
Exact-head maintainer adjudication for the Nemotron findings on a8bb355:
The outstanding CodeRabbit content-preservation thread was replied to and resolved because line 608 now directly byte-compares the nested payload lockfile after the installer run. |
cv
approved these changes
Jul 11, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Approved on exact head a8bb355. All required checks pass, CodeRabbit has no unresolved threads, and exact cloud E2E run 29140037914 passed. The remaining red cancel-superseded context is a controller page-limit infrastructure failure: no product/E2E test ran there, and the exact-head replacement E2E is green. Advisor concerns were adjudicated against the identical validated lockfile blob and runtime evidence.
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
2 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
Prevent macOS npm releases from rewriting the nested sandbox payload lockfile before Docker builds it on Linux. The installer now uses the committed lockfile for the payload, and CI rejects future nested lockfile drift.
Related Issue
Fixes #3798
Changes
npm ci --ignore-scriptsfor the nestednemoclaw/payload in both source-checkout and GitHub-clone installer paths.nemoclaw/package-lock.jsonwith a Linux CI dry run before dependency installation can mask drift.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailableLC_ALL=C LANG=C npx vitest run test/install-preflight.test.ts --testTimeout 60000(95 passed); focusedtest/pr-workflow-contract.test.tslockfile-guard contract (passed); npm 11.6.2 lockfile hash preservation and Linuxnode:22-trixie-slimnpm ci --dry-run(passed); requiredcloud-onboardE2E run29077214367(passed)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)Signed-off-by: Chengjie Wang chengjiew@nvidia.com
Summary by CodeRabbit
npm ci --ignore-scripts) before building.npm ci-based flows and ensure the sandbox payload lockfile is preserved withnpm ci --ignore-scripts.