Skip to content

fix(sc4): scope npm lock resolution to owning projects - #940

Open
chrisknvidia wants to merge 4 commits into
NVIDIA:mainfrom
chrisknvidia:feat/christopherk/npm-project-lock-scope
Open

chrisknvidia wants to merge 4 commits into
NVIDIA:mainfrom
chrisknvidia:feat/christopherk/npm-project-lock-scope

Conversation

@chrisknvidia

@chrisknvidia chrisknvidia commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #937.

npm manifests now resolve dependency ranges within their owning lockfile's installation tree. This prevents sibling and independent nested projects from borrowing unrelated pins, while preserving workspace hoisting and nearest-install precedence. An own lockfile remains a boundary even when empty or malformed. Shrinkwrap takes precedence for manifest resolution; independent lockfile vulnerability audits and explicit manifest pins remain intact.

Refreshed onto main ea5aeef6; current head 26d3b7ed. All four feature commits are unchanged by range-diff. Current-head CI is running; historical failures are not current-head validation.

Validation at this head:

  • 1,941 affected and malformed-manifest CLI tests passed. Full Ruff lint/format passed.
  • 72 actual CLI scans passed across source and a fresh noneditable wheel. Covered npm lock v1/v2/v3, sibling/independent roots, modern/legacy workspaces, hoisting, own malformed/empty locks, aliases, shrinkwrap, uppercase filenames and strict finding/completeness exits.
  • All 36 source/wheel case outcomes matched, including 20 JSON behavior pairs. 32 SARIF reports passed project-model validation. All 71 fixture files remained unchanged.
  • Two additional live OSV controls used fresh caches and real batch/detail requests: minimist 0.0.8 returned two advisories and 1.2.8 returned none.
  • All 148 package files and 158 installed distribution versions matched across frozen environments. Independent code/security review found no actionable findings.

Validation used static scans on macOS and real OSV requests. Existing genuine npm fixture lockfiles were reused; no fresh npm install, lifecycle execution or dependency-content sandbox qualification was performed. No full integration suite, live model, customer production or Windows qualification was run. SARIF validation uses project models, not external full-schema/service ingestion. Native typed review bots were unavailable.

@chrisknvidia
chrisknvidia marked this pull request as ready for review October 28, 2026 21:56

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Value and readiness

Ready. Per-project ChainMaps replace the global npm name/version map, preserving workspace hoisting while preventing sibling or independent descendants from borrowing unrelated lock pins.

Material findings

No required changes found.

PIC tradeoffs

Independent lockfile audits remain intact even when shrinkwrap takes precedence for manifest resolution. Unsupported alias inference remains unresolved as documented; explicit pins retain precedence. Shared installation maps avoid per-workspace table copies.

Verification and gaps

Read the full diff, #937 and pinned extraction/application/node context. Traced modern and legacy workspace roots, nearest-install precedence, own malformed-lock ownership, shrinkwrap precedence, traversal rejection, aliases, exact pins and budget exits. Reviewed every added regression and both adjusted mapping assertions; the changed paths preserve independent lock vulnerability extraction. Hosted checks: changes SUCCESS, lint SUCCESS, test-unit SUCCESS, OpenCode TypeScript Tests SUCCESS, DCO Check SUCCESS, docker-smoke SUCCESS. This is a read-only static review; no contributor code, tests, imports, builds or scanner invocations were executed.

Reviewed head: 47dea3fa1adf22a807aa4a224a8e8047b742c20a.

@chrisknvidia
chrisknvidia force-pushed the feat/christopherk/npm-project-lock-scope branch 4 times, most recently from 2be1653 to 675ed53 Compare November 2, 2026 20:08

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Value and readiness

Ready for maintainer review. npm manifests now resolve ranges within their owning installation tree, preserving workspace hoisting without borrowing unrelated sibling or independent descendant pins. The prior marked approval had no required changes; this full re-review found none at the new head.

Material findings

No required changes found. static_patterns_supply_chain.py:2199-2310 reserves project-owned lock directories before resolution, gives shrinkwrap precedence, rejects traversal-shaped workspace locations, and builds nearest-install ChainMaps. The call at line 4355 supplies only the manifest directory's map. Explicit manifest pins and independent lockfile audits remain intact; malformed/deep own locks cannot cause ancestor borrowing. Regressions cover v1/v2/v3 locks, component ordering, modern/legacy workspaces, nested installs, uppercase basenames, malformed own locks, shrinkwrap, aliases, bounds and source/parse-error accounting.

PIC tradeoffs

Unsupported alias inference remains conservative as documented. Independent lockfile auditing continues even when another lock controls manifest resolution; shared installation maps avoid workspace table copies.

Verification and gaps

Read the complete four-file diff, exact-head analyzer source, all marked review history and comments/threads, and regression assertions. Exact-head CI run 41516460409: changes, lint, OpenCode TypeScript Tests, DCO and docker-smoke succeeded. test-unit reports 17,833 passed and 15 failed; all 15 failed test IDs exactly match main e14c450b run 41513800480, comprising PowerShell/quote-ownership cases. This establishes no additional failing test ID, while the suite remains red and GitHub reports MERGEABLE/UNSTABLE. No required-check names are configured. Static review only; no contributor code, tests, imports, builds, setup or scanner invocations were executed.

Reviewed head: 675ed530cf2cba91440a9ad0ebb49abc8e25d065.

Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
@chrisknvidia
chrisknvidia force-pushed the feat/christopherk/npm-project-lock-scope branch from 675ed53 to 26d3b7e Compare November 9, 2026 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sibling npm lockfiles supply versions to unrelated project manifests

2 participants