Repository navigation
fix(sc4): scope npm lock resolution to owning projects - #940
chrisknvidia wants to merge 4 commits into
Conversation
rng1995
left a comment
There was a problem hiding this comment.
[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.
2be1653 to
675ed53
Compare
rng1995
left a comment
There was a problem hiding this comment.
[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>
675ed53 to
26d3b7e
Compare
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 head26d3b7ed. 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:
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.