Skip to content

feat(scanner): track scan completeness - #908

Open
luojiyin1987 wants to merge 8 commits into
OWASP:mainfrom
luojiyin1987:feat/scanner-track-scan-completeness
Open

feat(scanner): track scan completeness#908
luojiyin1987 wants to merge 8 commits into
OWASP:mainfrom
luojiyin1987:feat/scanner-track-scan-completeness

Conversation

@luojiyin1987

Copy link
Copy Markdown
Collaborator

Summary

Add scan completeness diagnostics to the scanner result.

  • Track OSV detail lookup failures.
  • Track packument, remediation, and chain resolution failures.
  • Return required completeness data from single-folder and multi-folder scans.
  • Keep output, report, aggregation, and ratchet behavior out of this PR.

Part of #907.

Validation

  • npm run build
  • npm test -- --runInBand tests/npm-registry.test.ts tests/scanner-cache.test.ts tests/multi-folder-scan.test.ts

@luojiyin1987
luojiyin1987 marked this pull request as ready for review July 28, 2026 01:40
@luojiyin1987
luojiyin1987 requested a review from sonukapoor as a code owner July 28, 2026 01:40

@sonukapoor sonukapoor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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:

  1. This does more than the "model, diagnostics collection, registry observer support, and result propagation" that #907 scopes for this PR. In src/scanner.ts the results.map became results.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 empty vulnerabilities array and unresolvedAdvisoryIds, plus the "N could not be resolved" warning. Now it disappears from findings[] and completeness reports complete: true with no diagnostic. That is a real output change (finding counts, anything reading findings.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?

  2. chainResolutionFailureCount / CHAIN_RESOLUTION_FAILURE is implemented but I do not see a test that forces resolveChainFix to 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 for REMEDIATION_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.

@luojiyin1987
luojiyin1987 requested a review from sonukapoor July 28, 2026 14:34
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.

2 participants