Skip to content

fix(static): honor optionalDependencies in package.json extraction - #774

Merged
rng1995 merged 1 commit into
NVIDIA:mainfrom
drakeo338:claude/770-fix
Oct 14, 2026
Merged

rng1995 merged 1 commit into
NVIDIA:mainfrom
drakeo338:claude/770-fix

Conversation

@drakeo338

Copy link
Copy Markdown
Contributor

Fixes #770.

The package.json extractor never read optionalDependencies, so packages listed only there were not checked. When an entry overrode the same name in dependencies, the overridden dependencies version was checked instead of the one npm installs. This reads optionalDependencies (including in the invalid-JSON fallback scan) and skips the overridden dependencies entry. Tests added in tests/unit/test_patterns_new.py fail on the old code and pass now.

npm installs an optionalDependencies entry in place of a same-name
dependencies entry, but optionalDependencies was never read. Include it
in the extracted sections (and the invalid-JSON fallback scan) and skip
the overridden dependencies entry, so the version that is actually
installed is the one checked.

Fixes NVIDIA#770

Signed-off-by: drakeo338 <paranoyouz@gmail.com>

@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]

@drakeo338 thank you for the focused fix, and for following npm's own precedence rule instead of just adding another section.

Value and readiness

#770 is a real SC4 false negative. Packages listed only in optionalDependencies were never checked, and a same-name override was checked at the dependencies version that npm does not install. The change reads optionalDependencies in both the JSON path and the invalid-JSON fallback. It also drops the overridden dependencies entry, which matches npm's documented rule that optional entries override same-name dependencies entries. Existing budget, pin and line-location handling is reused. Ready for final maintainer review.

Material findings

None blocking.

  1. Optional, src/skillspector/nodes/analyzers/static_patterns_supply_chain.py:1382: overridden = set(optional) also includes keys whose optional value is not a string. For {"dependencies": {"chalk": "4.1.2"}, "optionalDependencies": {"chalk": 5}}, the dependencies entry is skipped (L1391) and the optional one is skipped as a non-string, so chalk is not checked at all; main checks 4.1.2. npm treats non-string specs as invalid, so the impact is small. Still, a malformed sibling should not remove a check. Building the set from string-valued entries only, plus one test, closes it.

PIC tradeoffs

None identified.

Verification and gaps

  • I read the full diff against merge-base ef0f96d7 and the surrounding extractor on main.
  • _package_json_lines iterates _NPM_DEPENDENCY_SECTIONS, so line lookup now starts at the optionalDependencies header. The skip happens before the limit counter, so budgets are unaffected.
  • _apply_locked_versions still applies npm lock versions to optional entries. _NPM_DEPENDENCY_SECTIONS and this extractor are the only package.json dependency parsers in src/.
  • The invalid-JSON fallback reports both versions when a name appears in both sections. That is conservative and acceptable, because npm would not install from an invalid manifest.
  • Tests: optional-only (including a scoped name), same-name override, limit/line numbers, and fallback. The first case returns [] on main, so it would catch a regression.
  • Main (b07f181d) is 54 commits ahead; git merge-tree is clean, and the PR is also clean against #765 and #773.
  • CI: all 6 checks are green on 6bfca4e. I did not run PR code, per policy.

@rng1995
rng1995 merged commit 8427ac5 into NVIDIA:main Oct 14, 2026
6 checks passed
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.

package.json skips optionalDependencies and checks overridden dependencies at the wrong version

2 participants