(feat): Add pnpm-lock.yaml support#4
Conversation
|
Could also provide some tests? |
Will do, there were no tests for package-lock.json either, but never too late to start |
|
@holgerb tests were added, would be nice to have the PNPM support in |
There was a problem hiding this comment.
Pull request overview
Adds pnpm-lock.yaml scanning support across the Python and Node.js scanners, updates the test harness/fixtures to validate lockfile detection, and cleans up repository hygiene around Node dependencies.
Changes:
- Extend scanners to recognize and parse
pnpm-lock.yamlin addition topackage.json/package-lock.json. - Update test runner and Node tests to use Node’s built-in test runner (
node --test), and add new fixtures for lockfile-based detection. - Remove vendored
node_modulescontent from the repo and add ignore/editor configuration files.
Reviewed changes
Copilot reviewed 10 out of 57 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_python_scanner.py | Adds mocked affected-package DB and new tests for package-lock.json and pnpm-lock.yaml fixture scanning. |
| tests/test_nodejs_scanner.js | Migrates tests to node:test and updates to new scanner API (scanPackage). |
| tests/run_tests.sh | Runs Node tests via node --test and tweaks summary output. |
| tests/fixtures/pnpm_lock_shai_hulud_2.yaml | Adds pnpm lock fixture used by Python tests. |
| tests/fixtures/package_lock_shai_hulud_2.json | Adds npm lock fixture used by Python tests. |
| shai_hulud_scanner.py | Adds YAML parsing + pnpm lock scanning path and directory discovery for pnpm-lock.yaml. |
| shai_hulud_scanner.js | Renames package scanning entrypoint and adds pnpm dispatch / CLI support (but currently incomplete). |
| package.json | Updates Node test script to run via node --test. |
| node_modules/js-yaml/README.md | Removes vendored dependency file from repository. |
| node_modules/js-yaml/package.json | Removes vendored dependency file from repository. |
| node_modules/js-yaml/LICENSE | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/type/timestamp.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/type/str.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/type/set.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/type/seq.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/type/pairs.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/type/omap.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/type/null.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/type/merge.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/type/map.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/type/int.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/type/float.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/type/bool.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/type/binary.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/type.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/snippet.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/schema/json.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/schema/failsafe.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/schema/default.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/schema/core.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/schema.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/exception.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/dumper.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/lib/common.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/index.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/dist/js-yaml.min.js | Removes vendored dependency file from repository. |
| node_modules/js-yaml/CHANGELOG.md | Removes vendored dependency file from repository. |
| node_modules/js-yaml/bin/js-yaml.js | Removes vendored dependency file from repository. |
| node_modules/argparse/README.md | Removes vendored dependency file from repository. |
| node_modules/argparse/package.json | Removes vendored dependency file from repository. |
| node_modules/argparse/LICENSE | Removes vendored dependency file from repository. |
| node_modules/argparse/lib/textwrap.js | Removes vendored dependency file from repository. |
| node_modules/argparse/lib/sub.js | Removes vendored dependency file from repository. |
| node_modules/argparse/CHANGELOG.md | Removes vendored dependency file from repository. |
| node_modules/.package-lock.json | Removes vendored dependency lock data from repository. |
| .vscode/settings.json | Adds VS Code settings for running unittest. |
| .prettierignore | Ignores formatting for shai_hulud_scanner.js. |
| .gitignore | Adds standard ignores (venv, pycache, node_modules, etc.). |
Comments suppressed due to low confidence (1)
shai_hulud_scanner.js:405
- The file renames
scanPackageJsontoscanPackage, but other code paths in this module still callscanPackageJson(e.g. directory and single-file scanning). This will throw at runtime unless an alias is kept or all call sites are updated.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 56 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (9)
shai_hulud_scanner.py:1
- The docstring no longer matches behavior since
scan_package_jsonnow supportspnpm-lock.yaml(and generally.yaml). Update the docstring to include pnpm lockfiles (and/or clarify supported file types/extensions) so callers understand the accepted inputs.
shai_hulud_scanner.py:1 yaml.safe_load(...)andyaml.YAMLErrorrequireyamlto be in scope. Ifyamlis not imported at module level, this will raiseNameError(and theexcept (..., yaml.YAMLError)can mask the original error). Ensureimport yamlexists at module scope (preferred), or adjust exception handling to avoid referencingyamlwhen it isn’t available.
shai_hulud_scanner.py:1yaml.safe_load(...)andyaml.YAMLErrorrequireyamlto be in scope. Ifyamlis not imported at module level, this will raiseNameError(and theexcept (..., yaml.YAMLError)can mask the original error). Ensureimport yamlexists at module scope (preferred), or adjust exception handling to avoid referencingyamlwhen it isn’t available.
tests/test_python_scanner.py:1- These
print(...)statements will add noisy output to the test run and can make failures harder to spot. Prefer removing them, or switch to logging that is only emitted on failure (e.g., include paths in assertion messages).
tests/test_python_scanner.py:1 - These
print(...)statements will add noisy output to the test run and can make failures harder to spot. Prefer removing them, or switch to logging that is only emitted on failure (e.g., include paths in assertion messages).
tests/test_python_scanner.py:1 - Indexing
exact_matches[0]makes the tests order-dependent. The scan order can change (e.g., dict iteration differences, future refactors), causing flaky tests even when results are correct. Assert that some match inexact_matcheshas the expected{name, installed_version}(e.g., search/filter the list) rather than requiring it to be the first element.
tests/test_python_scanner.py:1 - Indexing
exact_matches[0]makes the tests order-dependent. The scan order can change (e.g., dict iteration differences, future refactors), causing flaky tests even when results are correct. Assert that some match inexact_matcheshas the expected{name, installed_version}(e.g., search/filter the list) rather than requiring it to be the first element.
tests/run_tests.sh:1 - Running
npm installinside the test runner introduces network/dependency resolution variability and slows down every test invocation. Prefernpm ci(more reproducible) when a lockfile exists, and/or run dependency installation as a separate CI step so the test script only executes tests.
package.json:1 - Bumping the Node engine requirement from
>=14to>=22is a breaking compatibility change for consumers and CI environments. If the only new requirement isnode --test, consider setting the minimum to the actual needed version (Node 18+ for the built-in test runner) or justify/encode the stricter requirement in documentation/release notes.
Add pnpm-lock.yaml support for specific version scanning.
Adds support in both JS and Python scanner.