feat: Use OSV-Scalibr scan function directly

[another-rex]

Closes #1769

Behaviour changes / notes:

• OSV-Scalibr's Scan() function does DFS rather than BFS, so the order of files being read in the snapshot changes slightly.
• Their Scan() function also does not deduplicate the output of the extractor, so the SBOM with duplicates will return double the results. I think it's fine to have that behavior change for now unless someone complains about it.
• Unfortunately, we also double log the path to the file in the error, because both the extractor and the walker both prepend the path the error originated from. This is a TODO in osv-scalibr to remove input path references in the extractor errors. (chore: Remove input.Path from extractor errors osv-scalibr#821)

[another-rex]

Annoyingly it seems like osv-scalibr's directory walk is not deterministic on different systems (as it uses the order the filesystem gives when iterating through a directory).

I'm not sure how to fix this for our tests, do we just avoid snapshotting any of the walking logs?

[G-Rath]

I'm not sure how to fix this for our tests, do we just avoid snapshotting any of the walking logs?

I think those logs are an important part of our snapshots since they're output we definitely want to be showing to the user (compared to some of the other logs we're currently filtering) and they aid in debugging since they tell us clearly what files are being scanned and the number of packages extracted.

Is there anyway to make the walking deterministic, even if its controlled via a flag or env variable so that we can opt-in?

[G-Rath]

a possible workaround could be to try buffering the logging, and try to dump it immediately if we error - we could also do the same logging here under a different level like DEBUG so that you could see the actual unbuffered order...

e.g. something like:

// NOT ACTUAL GO CODE
scannedLogs := []string{}

err := scanner.Scan({
	StatsAfterExtractorRun(stats) {
		slog.Debug(fmt.Sprintf(
			"Scanned %s file and found %d %s",
			extractorstats.Path,
			pkgsFound,
			output.Form(pkgsFound, "package", "packages"),
		))

		scannedLogs = append(scannedLogs, fmt.Sprintf(
			"Scanned %s file and found %d %s",
			extractorstats.Path,
			pkgsFound,
			output.Form(pkgsFound, "package", "packages"),
		))
	}
})

for line := range slices.Sort(scannedLogs) {
	slog.Info(line)
}

if err != nil {
	slog.Error(err)
}

[another-rex]

I ended up with something similar to the buffering approach, just on the logs instead so we don't have to touch the code functions that much.

[G-Rath]

nit: the else branches here could be the initial values in the struct

[another-rex]

@G-Rath

I think those logs are an important part of our snapshots since they're output we definitely want to be showing to the user (compared to some of the other logs we're currently filtering) and they aid in debugging since they tell us clearly what files are being scanned and the number of packages extracted.

Yeah I agree.

Is there anyway to make the walking deterministic, even if its controlled via a flag or env variable so that we can opt-in?

I am very hesitant to change the actual code logic for tests, because then we are not really testing for the actual code paths someone running osv-scanner will run into.

---

I ended up with printing a few markers at the start and end of the directory walk (When testing is true), then sorting the lines in-between the markers to check for presence, but not the specific order.

PTAL

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 83.33333% with 24 lines in your changes missing coverage. Please review.

Project coverage is 65.66%. Comparing base (743d8ed) to head (be24113).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/osvscanner/scan.go	85.29%	7 Missing and 3 partials ⚠️
.../requirementsenhancable/requirementsenhanceable.go	61.90%	8 Missing ⚠️
cmd/osv-scanner/internal/testcmd/run.go	83.33%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1936      +/-   ##
==========================================
+ Coverage   65.60%   65.66%   +0.06%     
==========================================
  Files         167      168       +1     
  Lines       16060    16117      +57     
==========================================
+ Hits        10536    10584      +48     
- Misses       4859     4866       +7     
- Partials      665      667       +2     

☔ 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.

[G-Rath]

Is there anyway to make the walking deterministic, even if its controlled via a flag or env variable so that we can opt-in?

I am very hesitant to change the actual code logic for tests, because then we are not really testing for the actual code paths someone running osv-scanner will run into.

fwiw I agree - I was meaning we do that for the actual scanner logic not just for tests