osv-scanner icon indicating copy to clipboard operation
osv-scanner copied to clipboard

deduplicate sarif outputs for GitHub

Open brabster opened this issue 4 weeks ago • 4 comments

Fixes #2331

I did use Copilot coding agent to help me with this but I've tweaked and sense checked the output. I hope it's acceptable, let me know if there are any problems :pray:

brabster avatar Nov 08 '25 19:11 brabster

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Nov 08 '25 19:11 google-cla[bot]

Hmm not sure what to do about "Copilot" not having signed the CLA? I guess if this solution looks good I can go rewrite the history back to me

brabster avatar Nov 08 '25 19:11 brabster

To make the snapshot tests pass, you need to run with UPDATE_SNAPS=true env variable when running the tests.

another-rex avatar Nov 14 '25 02:11 another-rex

Thanks @another-rex!

  • I did refactor those tests to use more of a matrix approach - I think it's easier to see what's going on and be sure that all combinations are tested. I expect it to be less brittle for you going forward too.
  • I also changed the signature tests to depend on the invariance rather than the specific hash values - again, I hope that means it's easier to see what's being tested, that it's comprehensive, and fewer changes to make if edge cases emerge.
  • I think the snapshots are good

I'm not sure what to do about Copilot's authorship and the CLA - if I need to rewrite the history and force push or something I can do that, let me know.

brabster avatar Nov 14 '25 08:11 brabster

Yes, you would have to rewrite copilots commit and force push here, thanks!

another-rex avatar Nov 16 '25 21:11 another-rex

@another-rex hopefully the ownership is sorted now, CLA check seems happy now

brabster avatar Nov 17 '25 20:11 brabster

OK I think that's snapshots updated and a couple of linter issues fixed.

brabster avatar Nov 17 '25 23:11 brabster

OS-specific tests are failing, I think that's because the fingerprints depend on file paths, which vary by OS (both location and OS-specific formatting). Not sure what to do for the best - is a fingerprint be the same if the path to the file is different? Does it matter that a partialFingerprint for the same package doesn't match when you compare Windows and Ubuntu, but does match when it's seen more than once in the same OS? I don't think so?

A relatively simple way forward: remove the file element, so you get a duplicate when the CVE and package match regardless of where the file is. I think that should be robust across OS and fairly confident it solves the problem I have (screenshot) image.

Alternative - remove the comparison from the snapshot tests - probably trickier, may eliminate useful tests

Complex - try and deal with the cross-OS case - not sure this is really doable robustly, how would I compare CVE-1234-5678 occurring in file://var/lib/dpkg/status with some equivalent on Windows?

I'll go with the first option unless advised otherwise

brabster avatar Nov 18 '25 08:11 brabster

Sorry for the noise @another-rex that lints and tests OK for me. I've enabled the checks pipeline in my own repo but I get failures uploading coverage reports and I can't tell whether the tests are passing otherwise. Anyway, hopefully they are!

brabster avatar Nov 18 '25 11:11 brabster

Hmm... I think a fingerprint should depend on the filepath (I believe you would want to have two entries show up if you had two files with the same package/version),

For the snapshot tests, we can just replace it with a placeholder variable so it doesn't match. I pushed up a change with this replacement (needed to move a few packages around), let's see if this works.

another-rex avatar Nov 18 '25 22:11 another-rex

Codecov Report

:x: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review. :white_check_mark: Project coverage is 67.82%. Comparing base (b8a7531) to head (b37363e).

Files with missing lines Patch % Lines
internal/testutility/jsonreplace.go 80.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2333      +/-   ##
==========================================
+ Coverage   67.80%   67.82%   +0.02%     
==========================================
  Files         171      171              
  Lines       13005    13018      +13     
==========================================
+ Hits         8818     8830      +12     
- Misses       3502     3503       +1     
  Partials      685      685              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov-commenter avatar Nov 18 '25 22:11 codecov-commenter

Seems to work first try! Let me know if you're happy with this.

@G-Rath Can you take a quick look at my additional changes in case I missed anything obvious.

another-rex avatar Nov 18 '25 22:11 another-rex

Works for me @another-rex thanks!! Now I don't have to write a hacky dedupe script in my own project :D

brabster avatar Nov 19 '25 06:11 brabster