Multiple fixes to improve detection - #1974
Conversation
WalkthroughThe checker now resolves dependencies for unknown branches and passes GitHub authentication to npm auditing. npm scans focus on dependency roots and production packages, reuse existing state, inspect npm CLI trees, query advisories, and reconcile advisory aliases. ChangesDependency scanning
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant DependencyResolver
participant NPMAuditChecker
participant npm
participant GitHubAdvisories
participant IssueReconciliation
CLI->>DependencyResolver: Resolve dependencies for repository branch
DependencyResolver->>NPMAuditChecker: Initialize npm scanning with GitHub token
NPMAuditChecker->>npm: Reuse lockfile or install production dependencies
npm-->>NPMAuditChecker: Return audit or installed-tree data
NPMAuditChecker->>GitHubAdvisories: Query advisories for installed npm CLI packages
GitHubAdvisories-->>NPMAuditChecker: Return advisory identifiers and aliases
NPMAuditChecker->>IssueReconciliation: Reconcile vulnerability keys and aliases
IssueReconciliation-->>CLI: Return matched, suppressed, and new issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
dep_checker/npm_audit.py (2)
29-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider sharing the advisory query and client setup with
main.py.
dep_checker/main.pyalready defines agithub_vulnerabilities_queryand builds the sameAIOHTTPTransport+Clientpair forquery_ghad. This file now duplicates both. The two copies can drift when the schema selection changes.Extract the query text and a
build_github_client(gh_token)helper into a shared module, then import it in both files.🤖 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 `@dep_checker/npm_audit.py` around lines 29 - 53, Extract the shared github_vulnerabilities_query definition and AIOHTTPTransport/Client construction into a common module, exposing a build_github_client(gh_token) helper. Update dep_checker/npm_audit.py and main.py, including query_ghad, to import and reuse these shared symbols, removing their duplicated query and client setup while preserving existing behavior.
304-323: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winQuery each package name once, and reuse one client.
packagesis keyed by(name, version), so the same package name at two installed versions produces two identical GraphQL queries.Clientis also constructed per call withfetch_schema_from_transport=True, which adds an introspection round trip.Group the packages by name, run one query per name, and evaluate every installed version of that name against the returned advisory nodes.
🤖 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 `@dep_checker/npm_audit.py` around lines 304 - 323, Update the package vulnerability flow around the GraphQL Client construction and package loop to reuse one Client and query each unique package name only once. Group packages by their name while retaining every installed version, execute github_vulnerabilities_query once per name, then evaluate all grouped versions against the returned advisory nodes; avoid repeated schema-fetching client construction.dep_checker/test_npm_audit.py (3)
306-338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
test_npm_audit_basicre-runs the other tests and ends with an assertion-free block.Two points:
- Lines 308-315 call every focused test. Under pytest each of those tests then runs twice, because pytest also collects them directly.
- Lines 330-336 run the real
check_npm_vulnerabilitiesand catch every exception. The block cannot fail, so it verifies nothing. It can also invoke the realnpmbinary and the network, which makes the suite slow and non-deterministic.Remove the explicit calls and either drop the smoke block or patch
subprocess.runin it. If the manual entry point is still needed, keep the calls underif __name__ == "__main__":instead.🤖 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 `@dep_checker/test_npm_audit.py` around lines 306 - 338, Remove the focused test invocations from test_npm_audit_basic so pytest does not execute them twice. Remove the assertion-free real npm audit smoke block, or make it deterministic by mocking subprocess.run; if retaining manual execution, place those calls behind an if __name__ == "__main__": guard.
143-184: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for
node_modulespresent without a lockfile.This test creates both
package-lock.jsonandnode_modules. The uncovered combination isnode_modulespresent andpackage-lock.jsonabsent. In that statecheck_npm_vulnerabilitiessetspackage_lock_onlytoFalse, skips the install, and the ENOLOCK fallback is gated out. See the comment ondep_checker/npm_audit.pylines 533-549.Add a test with only
node_modules, return anENOLOCKpayload for["npm", "audit", "--omit=dev", "--json"], and assert that the fallback install and re-audit occur.🤖 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 `@dep_checker/test_npm_audit.py` around lines 143 - 184, Add a test alongside test_node_modules_skips_install_and_lockfile_only covering node_modules present without package-lock.json. Configure the fake npm runner to return an ENOLOCK audit payload for ["npm", "audit", "--omit=dev", "--json"], then verify the fallback install and re-audit commands occur and the resulting vulnerability handling is correct.
70-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the
Resultstub and the patch boilerplate.Four tests define an identical
Resultclass and repeat the sameoriginal_run = npm_audit.subprocess.run/try/finallypattern. MoveResultto module scope and add a small context manager for the patch, or use the pytestmonkeypatchfixture.♻️ Proposed helper
import contextlib class Result: def __init__(self, returncode: int = 0, stdout: str = "", stderr: str = ""): self.returncode = returncode self.stdout = stdout self.stderr = stderr `@contextlib.contextmanager` def patched_run(fake_run): original_run = npm_audit.subprocess.run npm_audit.subprocess.run = fake_run try: yield finally: npm_audit.subprocess.run = original_runAlso applies to: 104-108, 146-150, 190-194
🤖 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 `@dep_checker/test_npm_audit.py` around lines 70 - 75, Deduplicate the repeated Result stub and subprocess.run patching in the affected tests. Move Result to module scope and add a shared patched_run context manager, then update the four tests to use it while preserving restoration of npm_audit.subprocess.run after each test.
🤖 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 `@dep_checker/main.py`:
- Around line 133-137: Broaden the exception tuple around the
dep.version_parser(repo_path) probe to also catch KeyError, IndexError,
AttributeError, TypeError, and OSError. Preserve the existing
skipped_dependencies entry and continue behavior so parser failures on unrelated
checkouts do not abort the scan.
In `@dep_checker/npm_audit.py`:
- Around line 387-428: Unify npm vulnerability IDs across the parsing paths,
including the via-item handling around vulnerability_class creation and the
legacy branch. Prefer a GHSA or CVE identifier when present; otherwise normalize
numeric advisory sources and legacy advisory IDs to the shared npm-prefixed
namespace (for example, npm-{source}), and do not use a bare package name as the
ID. Preserve the existing URL, severity, and metadata behavior.
- Around line 160-198: Update run_npm_install so auditing does not imply that
registry-resolved versions represent shipped dependency versions when
package-lock.json is absent. Prefer generating an audit-only lockfile with npm
install --package-lock-only while preserving --ignore-scripts, or explicitly
record these packages in failed_packages as approximate results if the existing
install flow remains. Keep the current npm availability checks and failure
handling intact.
- Around line 141-143: Mark incomplete dependency scans instead of silently
treating them as successful: in dep_checker/npm_audit.py lines 141-143, append
the discovery failure to self.failed_packages before returning []; in
dep_checker/npm_audit.py lines 525-531, append a failure when
get_installed_bundle_packages returns an empty list and continue; in
dep_checker/main.py lines 132-148, print and return the collected
skipped_dependencies so main() sets scan_complete = False when a curated
dependency fails to resolve.
- Around line 533-549: Update the ENOLOCK recovery condition in the
dependency-audit flow to rely solely on is_missing_lockfile_error(audit_data),
removing the package_lock_only requirement so dependencies with node_modules but
no lockfile also run npm install and a second audit. Preserve the existing
install-failure handling and package_lock_only argument for the initial audit;
do not alter the workflow’s handling of the existing vendored node_modules
directory.
- Around line 281-293: Update the dependency traversal around walk to return
immediately when the (name, version) key already exists in seen, before
recursing into children, while preserving the existing version validation and
result recording. Also normalize bundleDependencies handling so the boolean true
form is accepted without passing it to set(), allowing check_npm_vulnerabilities
to scan the npm tree successfully.
- Around line 327-329: Update the advisory handling in the package-audit logic
around normalize_version_range and SpecifierSet to catch packaging
InvalidSpecifier and InvalidVersion exceptions per advisory, so one invalid npm
version or advisory range does not abort the package scan; use
SpecifierSet.contains with prereleases enabled for valid versions and retain the
existing continue behavior when the version is outside the vulnerable range.
---
Nitpick comments:
In `@dep_checker/npm_audit.py`:
- Around line 29-53: Extract the shared github_vulnerabilities_query definition
and AIOHTTPTransport/Client construction into a common module, exposing a
build_github_client(gh_token) helper. Update dep_checker/npm_audit.py and
main.py, including query_ghad, to import and reuse these shared symbols,
removing their duplicated query and client setup while preserving existing
behavior.
- Around line 304-323: Update the package vulnerability flow around the GraphQL
Client construction and package loop to reuse one Client and query each unique
package name only once. Group packages by their name while retaining every
installed version, execute github_vulnerabilities_query once per name, then
evaluate all grouped versions against the returned advisory nodes; avoid
repeated schema-fetching client construction.
In `@dep_checker/test_npm_audit.py`:
- Around line 306-338: Remove the focused test invocations from
test_npm_audit_basic so pytest does not execute them twice. Remove the
assertion-free real npm audit smoke block, or make it deterministic by mocking
subprocess.run; if retaining manual execution, place those calls behind an if
__name__ == "__main__": guard.
- Around line 143-184: Add a test alongside
test_node_modules_skips_install_and_lockfile_only covering node_modules present
without package-lock.json. Configure the fake npm runner to return an ENOLOCK
audit payload for ["npm", "audit", "--omit=dev", "--json"], then verify the
fallback install and re-audit commands occur and the resulting vulnerability
handling is correct.
- Around line 70-75: Deduplicate the repeated Result stub and subprocess.run
patching in the affected tests. Move Result to module scope and add a shared
patched_run context manager, then update the four tests to use it while
preserving restoration of npm_audit.subprocess.run after each test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13f77738-bf50-4958-af71-d11413bb6c2b
📒 Files selected for processing (4)
.github/workflows/check-vulns.ymldep_checker/main.pydep_checker/npm_audit.pydep_checker/test_npm_audit.py
Known branches still use curated dependency lists, but unknown refs now fall back to probing every tracked dependency definition and keeping only the ones whose version parsers succeed against the checked-out tree. This lets the scanner run against release branches and ad hoc refs without having to pre-register each branch name in dependencies_per_branch. Also document the workflow input as an arbitrary N|Solid branch/ref and mark curated scans as incomplete when a curated dependency fails to resolve.
bb5e7ac to
b51b578
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
dep_checker/npm_audit.py (3)
126-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the
ponytail:marker in the comment.The comment prefix
ponytail:is not a recognized marker such asNOTE:orTODO:. It reads as an accidental artifact. The rest of the comment carries useful intent, so keep the text and fix the prefix.♻️ Proposed change
- # ponytail: only audit dep roots; add a nested allowlist if a shipped package ever lives deeper. + # NOTE: only audit dep roots; add a nested allowlist if a shipped package ever lives deeper.🤖 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 `@dep_checker/npm_audit.py` around lines 126 - 133, Update the comment in the package discovery logic around relative_to_deps and dep_root by replacing the unrecognized “ponytail:” prefix with an appropriate recognized marker, while preserving the remaining comment text unchanged.
260-267: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the
problemsfield fromnpm ls.
npm lsexits non-zero when the installed tree has problems, such as missing or invalid dependencies. It still writes valid JSON, so this method accepts the tree. That behavior is correct, because a strict return-code check would discard usable trees. However, an incomplete tree causesget_installed_bundle_packagesto return fewer packages, and the scan then under-reports without any signal.npm ls --jsonreports these conditions in a top-levelproblemsarray.♻️ Proposed change
tree_data = json.loads(result.stdout) + problems = tree_data.get("problems") + if problems: + logger.warning( + f"npm ls reported {len(problems)} problem(s) in {package_dir}: {problems}" + ) logger.info(f"npm ls completed in {package_dir}") return tree_data🤖 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 `@dep_checker/npm_audit.py` around lines 260 - 267, Update the npm ls JSON handling near tree_data and the completion log to inspect the parsed result’s top-level problems field and log any reported problems, while still returning the valid tree regardless of the non-zero exit status. Preserve the existing empty-output handling and successful tree return behavior.
374-386: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the redundant
isdigitbranch.Lines 380-381 and lines 382-383 return the same value,
f"npm-{normalized}". Theisdigit()test changes nothing.Separately, consider guarding
normalize_version_rangeagainst an input that already uses==. For the string==1.0.0, the leading=satisfies the lookbehind, so the result is===1.0.0. PEP 440 reads===as arbitrary equality, which matches only an exact string. GitHub emits= 1.2.3for a single version, so this is not a current defect. A lookahead makes the intent explicit.♻️ Proposed change
- return re.sub(r"(?<![<>=!~])=\s*", "==", version_range) + return re.sub(r"(?<![<>=!~])=(?!=)\s*", "==", version_range)if isinstance(advisory_id, str): normalized = advisory_id.strip() if normalized.startswith("GHSA-") or normalized.startswith("CVE-"): return normalized - if normalized.isdigit(): - return f"npm-{normalized}" if normalized: return f"npm-{normalized}"🤖 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 `@dep_checker/npm_audit.py` around lines 374 - 386, In normalize_npm_advisory_id, remove the redundant isdigit() branch and keep the single non-empty normalized-string path returning npm-{normalized}. Also update normalize_version_range so its equality-prefix handling does not add another “=” when the input already begins with “==”, while preserving conversion of GitHub’s single-version “= 1.2.3” format.dep_checker/test_npm_audit.py (4)
446-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
(name, version)deduplication branch.
get_installed_bundle_packagesskips a node when(name, version)is already inseen.minipassandminipass_duphave different names, so this branch never runs. Add the same package twice at different tree positions to exercise it.♻️ Proposed change
{ "dependencies": { "tar": { "version": "7.5.19", "dependencies": { "minipass": {"version": "7.1.3"}, - "minipass_dup": {"version": "7.1.3"}, + "minipass_dup": { + "version": "7.1.3", + "dependencies": {"minipass": {"version": "7.1.3"}}, + }, }, } } },🤖 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 `@dep_checker/test_npm_audit.py` around lines 446 - 465, Update the test using get_installed_bundle_packages to place the same package name and version at multiple dependency-tree positions, then assert the returned packages contain that pair only once. Keep the existing distinct minipass/minipass_dup coverage and expected package assertions while exercising the seen-based (name, version) deduplication branch.
134-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
checker.failed_packagesis empty in the audit-path tests.
check_npm_vulnerabilitiesandrun_npm_installcatch broadException, so anAssertionErrorraised insidefake_runis swallowed and recorded only inself.failed_packages. A wrong command path can therefore still leavevulnerabilities == []. Add the state assertion in this test and in the tests at lines 143-184, 187-224, 227-266, and 269-312.♻️ Proposed change
vulnerabilities = checker.check_npm_vulnerabilities(Vulnerability) assert vulnerabilities == [], vulnerabilities + assert checker.failed_packages == [], checker.failed_packages🤖 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 `@dep_checker/test_npm_audit.py` around lines 134 - 140, Update the audit-path tests around check_npm_vulnerabilities and run_npm_install to assert checker.failed_packages is empty after each invocation, including the tests covering lines 143-184, 187-224, 227-266, and 269-312. Keep the existing vulnerability assertions and command checks unchanged so swallowed AssertionError failures cannot pass silently.
67-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the subprocess fake into one shared helper.
The
Resultclass and the patch/restore block are duplicated in six tests (lines 67-98, 101-140, 143-184, 187-224, 227-266, 269-312). MoveResultto module level and wrap the patching in a context manager. This removes the repeatedtry/finallyand keeps the flag assertions in one place.♻️ Proposed helper
import contextlib class Result: def __init__(self, returncode: int = 0, stdout: str = "", stderr: str = ""): self.returncode = returncode self.stdout = stdout self.stderr = stderr `@contextlib.contextmanager` def patched_run(fake_run): original_run = npm_audit.subprocess.run npm_audit.subprocess.run = fake_run try: yield finally: npm_audit.subprocess.run = original_run🤖 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 `@dep_checker/test_npm_audit.py` around lines 67 - 98, Extract the duplicated Result class to module scope in dep_checker/test_npm_audit.py and add a patched_run context manager to encapsulate replacing and restoring npm_audit.subprocess.run. Update all six affected tests, including test_npm_commands_omit_dev, to use this helper instead of their local Result definitions and try/finally blocks, while preserving their existing fake behavior and assertions.
520-558: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not re-run every test from
test_npm_audit_basic.pytest collects each
test_*function on its own. This function calls all of them again, so every test runs twice and a failure is attributed totest_npm_audit_basicinstead of the real test. Move the aggregation into a__main__block.The block at lines 550-556 also runs
check_npm_vulnerabilitieswithout patchingsubprocess.run, so it invokes realnpm installandnpm auditwith a 60 second timeout. That makes the suite dependent on npm availability and network access. Patchsubprocess.runhere as well, or mark this part as an opt-in integration test.♻️ Proposed change
-def test_npm_audit_basic() -> None: - print("Testing npm audit functionality...") - test_find_package_json_files_skips_nested_manifests() - test_npm_commands_omit_dev() - test_lockfile_skips_install() - test_node_modules_skips_install_and_lockfile_only() - test_no_lockfile_generates_lockfile_only_audit() - test_enolock_with_node_modules_still_falls_back() - test_enolock_falls_back_to_install_and_normal_audit() - test_invalid_advisory_range_or_version_is_skipped() - test_normalize_npm_advisory_id() - test_parse_audit_results_normalizes_ids() - test_preferred_advisory_id() - test_normalize_version_range() - test_get_installed_bundle_packages_accepts_boolean_bundle_dependencies() - test_npm_cli_uses_installed_tree() - +def test_find_package_json_files_discovers_dep_root() -> None: with tempfile.TemporaryDirectory() as temp_dir:🤖 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 `@dep_checker/test_npm_audit.py` around lines 520 - 558, Update test_npm_audit_basic so pytest no longer executes the other test_* functions from within it; move the aggregation calls into an if __name__ == "__main__" block. Also prevent its check_npm_vulnerabilities integration path from invoking real npm commands by patching subprocess.run with the existing test mock, or make that path explicitly opt-in as an integration test.Source: Linters/SAST tools
🤖 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 `@dep_checker/npm_audit.py`:
- Around line 336-344: Update the InvalidSpecifier/InvalidVersion exception
handler in the advisory matching flow to append the affected package to
self.failed_packages before logging and continuing. Preserve the existing
warning and skip behavior so main() can report the scan as incomplete when
normalization or version parsing fails.
- Around line 367-372: The preferred_advisory_id flow does not preserve
reconciliation across GHSA-to-CVE identifier changes. Add an alias lookup or
migration step before reconcile_issues.py performs exact vuln["id"] matching,
linking each preferred CVE identifier to its prior GHSA issue so the existing
issue is updated rather than recreated and closed.
- Around line 313-332: The package query loop currently recreates and closes the
AIOHTTP transport for each package and aborts the directory scan on request
failures. Refactor the method and its caller to use an async client session via
async with client as session and await session.execute for every package,
preserving one transport session across the loop; catch execution failures per
package so previously collected vulnerability matches remain available.
---
Nitpick comments:
In `@dep_checker/npm_audit.py`:
- Around line 126-133: Update the comment in the package discovery logic around
relative_to_deps and dep_root by replacing the unrecognized “ponytail:” prefix
with an appropriate recognized marker, while preserving the remaining comment
text unchanged.
- Around line 260-267: Update the npm ls JSON handling near tree_data and the
completion log to inspect the parsed result’s top-level problems field and log
any reported problems, while still returning the valid tree regardless of the
non-zero exit status. Preserve the existing empty-output handling and successful
tree return behavior.
- Around line 374-386: In normalize_npm_advisory_id, remove the redundant
isdigit() branch and keep the single non-empty normalized-string path returning
npm-{normalized}. Also update normalize_version_range so its equality-prefix
handling does not add another “=” when the input already begins with “==”, while
preserving conversion of GitHub’s single-version “= 1.2.3” format.
In `@dep_checker/test_npm_audit.py`:
- Around line 446-465: Update the test using get_installed_bundle_packages to
place the same package name and version at multiple dependency-tree positions,
then assert the returned packages contain that pair only once. Keep the existing
distinct minipass/minipass_dup coverage and expected package assertions while
exercising the seen-based (name, version) deduplication branch.
- Around line 134-140: Update the audit-path tests around
check_npm_vulnerabilities and run_npm_install to assert checker.failed_packages
is empty after each invocation, including the tests covering lines 143-184,
187-224, 227-266, and 269-312. Keep the existing vulnerability assertions and
command checks unchanged so swallowed AssertionError failures cannot pass
silently.
- Around line 67-98: Extract the duplicated Result class to module scope in
dep_checker/test_npm_audit.py and add a patched_run context manager to
encapsulate replacing and restoring npm_audit.subprocess.run. Update all six
affected tests, including test_npm_commands_omit_dev, to use this helper instead
of their local Result definitions and try/finally blocks, while preserving their
existing fake behavior and assertions.
- Around line 520-558: Update test_npm_audit_basic so pytest no longer executes
the other test_* functions from within it; move the aggregation calls into an if
__name__ == "__main__" block. Also prevent its check_npm_vulnerabilities
integration path from invoking real npm commands by patching subprocess.run with
the existing test mock, or make that path explicitly opt-in as an integration
test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e458ee60-dcdf-423b-8532-51abfbdd4f2e
📒 Files selected for processing (4)
.github/workflows/check-vulns.ymldep_checker/main.pydep_checker/npm_audit.pydep_checker/test_npm_audit.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/check-vulns.yml
- dep_checker/main.py
Make arbitrary-ref dependency probing more tolerant of parser shape errors and mark curated scans as incomplete when curated dependencies fail to resolve. Tighten npm audit handling by: - recording discovery failures and empty installed bundle trees as incomplete - generating prod-only lockfiles with --package-lock-only when no lockfile is present - retrying ENOLOCK recovery even when node_modules exists without a lockfile - handling boolean bundleDependencies and avoiding duplicate installed-tree walks - skipping invalid GitHub advisory specifiers or versions per advisory while marking the scan incomplete - making per-package GitHub advisory query failures non-fatal - normalizing npm advisory IDs across modern and legacy audit payloads - preferring CVE identifiers when GitHub exposes them Preserve reconciliation when a vendored npm advisory changes from GHSA to CVE by carrying alternate advisory identifiers in the scan payload and matching existing issues through those aliases before create/close decisions.
b51b578 to
555462c
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dep_checker/main.py (1)
359-369: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClassify skipped dependencies before setting
scan_complete.
resolve_dependencies()treats missing files and parser failures identically. For unknown branches,scan_completeremainsTrue, so reconciliation can close issues for dependencies that failed to parse. Keep expected absent dependencies non-fatal, but setscan_complete=Falsefor parser failures. Send the diagnostic to stderr when JSON output is enabled.🤖 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 `@dep_checker/main.py` around lines 359 - 369, Update the dependency-resolution flow around resolve_dependencies and scan_complete to distinguish expected missing dependencies from parser failures before classifying the scan. Keep absent dependencies non-fatal, but mark scans incomplete when parsing fails, including for unknown branches, and route the diagnostic to stderr whenever JSON output is enabled.
🧹 Nitpick comments (5)
dep_checker/test_npm_audit.py (3)
565-581: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the aggregate runner so pytest does not run every test twice.
test_npm_audit_basicstarts withtest_, so pytest collects it. It then calls the other 14 test functions, and each one runs a second time. A failure inside the aggregate is also attributed totest_npm_audit_basicinstead of the failing test.Keep the standalone-script behavior with a non-collected name and a
__main__guard.♻️ Proposed change
-def test_npm_audit_basic() -> None: +def run_all_tests() -> None: print("Testing npm audit functionality...") test_find_package_json_files_skips_nested_manifests()Then add at the end of the file:
if __name__ == "__main__": run_all_tests()🤖 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 `@dep_checker/test_npm_audit.py` around lines 565 - 581, Rename the aggregate function test_npm_audit_basic to a non-test name such as run_all_tests so pytest does not collect it, while preserving its calls to the individual test functions. Add a __main__ guard at the end of the file that invokes run_all_tests to retain standalone script behavior.
67-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the fake subprocess runner into one shared helper.
Resultandfake_runare duplicated in five tests (lines 70-84, 104-118, 146-160, 190-204, 230-246). Each copy also patchesnpm_audit.subprocess.run, which replacesrunon the realsubprocessmodule for the whole process.Define the result type once and use
unittest.mock.patchso restoration happens even if a helper raises before thefinallyblock.♻️ Proposed shared helper
from contextlib import contextmanager from unittest import mock class Result: def __init__(self, returncode: int = 0, stdout: str = "", stderr: str = ""): self.returncode = returncode self.stdout = stdout self.stderr = stderr `@contextmanager` def patched_npm(handler): """Record npm commands and delegate each one to `handler`.""" calls: list[list[str]] = [] def fake_run(cmd, **kwargs): calls.append(cmd) return handler(cmd, calls) with mock.patch.object(npm_audit.subprocess, "run", fake_run): yield callsEach test then supplies only its own
handler.🤖 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 `@dep_checker/test_npm_audit.py` around lines 67 - 98, Extract the duplicated Result type and fake subprocess logic from the five tests into shared Result and patched_npm helpers. Update each test to provide only a command handler and use patched_npm as a context manager, replacing direct npm_audit.subprocess.run assignment and manual restoration while preserving command recording and existing assertions.
89-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilence or replace the hardcoded
/tmppaths flagged by ruff.Ruff reports S108 for the literal
/tmppaths at lines 89-92, 344, 346, 399, 401, 413, 422, 445, 450, 457, and 472, and S106 forgh_token="token"at lines 344, 399, and 551. These tests never touch the filesystem at those paths, so there is no real insecure-temp-file defect. The findings still fail lint if the rules are enabled in CI.Define one module-level fake root and reuse it, for example:
FAKE_REPO = Path(tempfile.gettempdir()) FAKE_TOKEN = "token" # noqa: S105 - inert test valueThen use
FAKE_REPO,FAKE_REPO / "deps" / "npm", andgh_token=FAKE_TOKEN.Confirm which ruff rule set CI enforces before you invest in the change.
#!/bin/bash # Description: Determine whether ruff S1xx rules are enforced for the dep_checker tests. set -euo pipefail fd -H -t f -e toml -e cfg -e ini -e yml -e yaml . -d 3 \ | xargs rg -n -C5 'ruff|\[tool\.ruff|select|extend-select|per-file-ignores' || trueAlso applies to: 344-346
🤖 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 `@dep_checker/test_npm_audit.py` around lines 89 - 92, Replace the hardcoded /tmp paths in the dep_checker tests with a shared module-level fake repository root based on tempfile.gettempdir(), reusing it for repository and nested dependency paths. Replace inert gh_token="token" values with a shared fake-token constant and apply the appropriate S105 suppression if required. First verify the Ruff configuration used by CI and make these changes only if the S108/S105 rules are enforced.Source: Linters/SAST tools
dep_checker/npm_audit.py (2)
395-407: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicate
npm-branches.Lines 401-404 return the same value. The
isdigit()branch is redundant.♻️ Proposed simplification
if isinstance(advisory_id, str): normalized = advisory_id.strip() if normalized.startswith("GHSA-") or normalized.startswith("CVE-"): return normalized - if normalized.isdigit(): - return f"npm-{normalized}" if normalized: return f"npm-{normalized}"🤖 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 `@dep_checker/npm_audit.py` around lines 395 - 407, simplify normalize_npm_advisory_id by removing the redundant isdigit() branch and retaining a single normalized non-empty string path that returns the npm-prefixed identifier; preserve the existing GHSA-/CVE- handling and fallback behavior.
280-285: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlso accept the
bundledDependenciesspelling.npm supports this alias. Otherwise, bundled packages are not scanned.
🤖 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 `@dep_checker/npm_audit.py` around lines 280 - 285, Update the bundle dependency extraction near package_data and bundle_dependencies to accept npm’s bundledDependencies alias in addition to bundleDependencies, using the alias when the primary key is absent while preserving the existing handling for true, lists, and missing values.
🤖 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.
Outside diff comments:
In `@dep_checker/main.py`:
- Around line 359-369: Update the dependency-resolution flow around
resolve_dependencies and scan_complete to distinguish expected missing
dependencies from parser failures before classifying the scan. Keep absent
dependencies non-fatal, but mark scans incomplete when parsing fails, including
for unknown branches, and route the diagnostic to stderr whenever JSON output is
enabled.
---
Nitpick comments:
In `@dep_checker/npm_audit.py`:
- Around line 395-407: simplify normalize_npm_advisory_id by removing the
redundant isdigit() branch and retaining a single normalized non-empty string
path that returns the npm-prefixed identifier; preserve the existing GHSA-/CVE-
handling and fallback behavior.
- Around line 280-285: Update the bundle dependency extraction near package_data
and bundle_dependencies to accept npm’s bundledDependencies alias in addition to
bundleDependencies, using the alias when the primary key is absent while
preserving the existing handling for true, lists, and missing values.
In `@dep_checker/test_npm_audit.py`:
- Around line 565-581: Rename the aggregate function test_npm_audit_basic to a
non-test name such as run_all_tests so pytest does not collect it, while
preserving its calls to the individual test functions. Add a __main__ guard at
the end of the file that invokes run_all_tests to retain standalone script
behavior.
- Around line 67-98: Extract the duplicated Result type and fake subprocess
logic from the five tests into shared Result and patched_npm helpers. Update
each test to provide only a command handler and use patched_npm as a context
manager, replacing direct npm_audit.subprocess.run assignment and manual
restoration while preserving command recording and existing assertions.
- Around line 89-92: Replace the hardcoded /tmp paths in the dep_checker tests
with a shared module-level fake repository root based on tempfile.gettempdir(),
reusing it for repository and nested dependency paths. Replace inert
gh_token="token" values with a shared fake-token constant and apply the
appropriate S105 suppression if required. First verify the Ruff configuration
used by CI and make these changes only if the S108/S105 rules are enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97d7ed5a-41a6-4496-8f6e-455a2ca0a40a
📒 Files selected for processing (4)
dep_checker/main.pydep_checker/npm_audit.pydep_checker/reconcile_issues.pydep_checker/test_npm_audit.py
2 participants
Summary by CodeRabbit
New Features
Bug Fixes