feat(scanner): track scan completeness - #908
Conversation
sonukapoor
left a comment
There was a problem hiding this comment.
Thanks for splitting this out of #893 @luojiyin1987, it is much easier to review in focused pieces. The AsyncLocalStorage-based registry observer is a clean way to get failure signals out of parent-upgrade.ts, npm-transitive-resolution.ts, and transitive-chain-resolver.ts without threading a callback through every signature, and fetchPackument's public return type is unchanged. The observer test coverage (concurrent dedupe, cache-hit notification, retry after a transient failure and after a 404) is genuinely thorough.
Two things before this can go in:
-
This does more than the "model, diagnostics collection, registry observer support, and result propagation" that #907 scopes for this PR. In
src/scanner.tstheresults.mapbecameresults.flatMap, and a finding is now dropped entirely when every advisory ID for a package is confirmed missing from OSV (vulnerabilities.length === 0 && !hasUnconfirmedMissingDetail). Before this change that package still surfaced as a finding with an emptyvulnerabilitiesarray andunresolvedAdvisoryIds, plus the "N could not be resolved" warning. Now it disappears fromfindings[]andcompletenessreportscomplete: truewith no diagnostic. That is a real output change (finding counts, anything readingfindings.length, ratchet baselines on the next re-baseline), not diagnostics collection, and it is backwards from the transparency this PR is adding: the one genuinely ambiguous case gets silently dropped and reported complete. Could you pull that out into its own PR with its own tests and a note on the OSV-staleness rationale, or, if it stays, at least surface a diagnostic instead of reporting complete? -
chainResolutionFailureCount/CHAIN_RESOLUTION_FAILUREis implemented but I do not see a test that forcesresolveChainFixto throw and asserts the diagnostic appears. It is one of the four failure types in the description and the only one currently untested. Could you add one the same way you did forREMEDIATION_FAILURE?
A few smaller things, not blocking: the remediation block got wrapped in runWithObserver(...) without re-indenting the body, which makes the diff harder to follow; buildCompletenessDiagnostics / formatDiagnosticMessage are pure and might fit better in their own file given how we usually extract these; and PACKUMENT_FETCH_FAILURE's count is really "distinct packages" while REMEDIATION_FAILURE splits affectedPackageCount out separately, so worth normalizing what "count" means before an output PR builds on it.
I ran the full suite on the branch (1218 passing) and the build is clean, so nothing is mechanically broken. I just want the finding-suppression pulled out or reworked and the chain-resolution path tested before merge.
Summary
Add scan completeness diagnostics to the scanner result.
Part of #907.
Validation
npm run buildnpm test -- --runInBand tests/npm-registry.test.ts tests/scanner-cache.test.ts tests/multi-folder-scan.test.ts