Skip to content

Conversation

@another-rex
Copy link
Collaborator

@another-rex another-rex commented Oct 31, 2025

No more single lockfile extraction logic, just use the osv-scalibr scan function.

To keep the behaviour similar to what we currently have (mostly to make testing for the correct behaviour easier, we can change it later).

Some notes:

  • OSV-Scalibr does not deduplicate the files being walked, so to prevent files from being extracted multiple times, we exclude child directories if they are covered by another scan directory. Ideally this is done in osv-scalibr.
  • We keep track of all the specific files that are passed in via -L or --sbom, so that we can make sure to error if they fail to extract. This required using the stats collector to see all files that the extractor has ran on, which breaks the custom extractor input. We'll likely refactor this soon, so just left the field there for now.

@another-rex another-rex requested review from G-Rath, cuixq and michaelkedar and removed request for michaelkedar October 31, 2025 05:27
@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2025

Codecov Report

❌ Patch coverage is 92.91339% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.02%. Comparing base (a388a97) to head (99e1d9a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/osvscanner/scan.go 95.29% 2 Missing and 2 partials ⚠️
pkg/osvscanner/internal/scanners/lockfile.go 84.21% 2 Missing and 1 partial ⚠️
pkg/osvscanner/stats.go 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2313      +/-   ##
==========================================
+ Coverage   67.94%   68.02%   +0.08%     
==========================================
  Files         172      172              
  Lines       12849    12810      -39     
==========================================
- Hits         8730     8714      -16     
+ Misses       3447     3430      -17     
+ Partials      672      666       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@cuixq cuixq left a comment

Choose a reason for hiding this comment

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

Is there a particular reason that we want to specifically track the error of specified files? I feel the code to handle this makes the whole scan logic a bit harder to understand.

@another-rex
Copy link
Collaborator Author

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request is a significant and valuable refactoring that centralizes scanning logic by using OSV-Scalibr's Scan function directly. This simplifies the codebase and makes it more maintainable. The approach to handle different scan targets like directories, lockfiles, SBOMs, and git commits is much cleaner now. I've identified a couple of issues related to path handling in multi-root scan scenarios and error checking, for which I've provided detailed feedback and suggestions. Once these are addressed, this will be a solid improvement.

@another-rex
Copy link
Collaborator Author

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request is a significant and valuable refactoring that centralizes scanning logic by using OSV-Scalibr directly. The move away from custom lockfile extraction logic is a great simplification, and the improved error handling for user-specified file paths is a welcome enhancement for user experience. I've identified one high-severity issue related to the handling of the --recursive flag, which appears to be broken by these changes, and have provided a suggestion to address it. Overall, this is a solid improvement.

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.

5 participants