fix(worker): make scan.summary reflect all persisted findings, not just the engine's own count - #307
Merged
Merged
Conversation
…st the engine's own count Root-caused from a Trust Runs screenshot found during marketing work: a run showed "Engine status: completed. 0 finding(s) reported." beside a badge reading "39 findings" for the same run. Both numbers were individually correct, and both are user-facing, which made the run look broken. engineResult.output.summary (parseEngineOutput, output-parser.ts) is built purely from the agentic engine's own vulnerabilities.json artifact. It never accounts for the SCA, secrets, agent-config, or URL scanner findings the orchestrator merges in afterward, nor the false-positive filtering and dedup that follow. run-scan.job.ts persisted that engine-only text to scan.summary even after persisting the full merged set — so on any run where the engine layer alone found nothing, the frozen summary said "0" next to a persisted count that could be dozens. scan.summary is not just a dashboard badge. It is read verbatim by: - The Trust Runs list and scan detail page (apps/web). - The private assurance report and shared report view (report-generator.ts, report-service.ts) — the html "Summary" row would say "0 finding(s) reported" on a report attached to 39 findings. - Scan-completion notifications (notifications.ts) — the body text embeds the wrong summary right next to a title that already correctly says "39 findings" from persistedFindings.length, so the same message contradicts itself. Fix, scoped to the write sites in run-scan.job.ts (not to parseEngineOutput, which is a correctly-named pure parser of the engine's own artifact and stays untouched): build scanSummary once, right after persistedFindings is known. When persistedFindings.length already matches the engine-only findingCount (the common case — no other scanner contributed, nothing was filtered), pass the engine's text through unchanged. When they disagree, append the real persisted count. Every downstream write (prisma.scan.update, the DB write completeScanWithScore also performs, notifyScanCompleted's body, and the job's own return value) now uses this single corrected value instead of four separate references to the engine-only string. completeScanWithScore's summary parameter only ever becomes display text on scan.summary (packages/db/src/score-service.ts:244) — it does not feed computeScore, the breakdown, or the ScoreSnapshot itself, all of which query the live Finding table independently. So this fix carries no risk to the public-scorecard allowlist or the score model. Verified against every existing assertion in run-scan.job.test.ts: the default mock fixtures in this file always have persistFindings resolving to the same count as the mocked engine findingCount (usually both 0), so the corrective branch never fires for them and no existing expectation changes. Added two targeted tests: one reproducing the exact bug shape (engine says 0, three scanner-layer findings persist, summary must state both), and one guardrail confirming the passthrough is untouched when the counts already agree. Sandbox note: no node_modules, npm returns 403, so vitest/tsc/eslint could not run locally. Traced every call site, every consumer of scan.summary, and every existing test assertion in this file by hand; CI is the execution gate. Co-Authored-By: Claude <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe scan job now reconciles engine summaries with retained finding counts. It persists, scores, notifies, and returns the consolidated summary. Tests cover differing counts and unchanged matching counts. ChangesScan summary reconciliation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DRAFT — root-caused while doing the marketing screenshot work in #306. Not blocking that PR (already merged); this is the underlying product bug it surfaced.
The bug
A real Trust Runs screenshot showed:
Both numbers were individually correct for what they measured — which is exactly the problem: nothing in the copy explains why they disagree, so it just looks broken.
Root cause
Two independent pipelines compute "how many findings" for the same scan:
output-parser.ts:824,parseEngineOutput) is built fromvulnerabilities.length— parsed only from the raw agentic engine'svulnerabilities.jsonartifact.scan-service.ts:347,_count.findings) is a live count ofFindingrows actually persisted, which come fromorchestratorResult.allFindings— the combined set across the engine, SCA, secrets, agent-config, and URL scanners, after false-positive filtering and dedup (scanner-orchestrator.ts:56-67).run-scan.job.ts:1269persisted the engine-only text toscan.summaryafter the combined set was already persisted, even though the correct total (persistedFindings.length) was sitting in scope one line above thefindings_persistedevent that already uses it.Why this is bigger than a UI badge
scan.summaryis read verbatim in four places, not one:apps/web)notifyScanCompleted's 4th arg ispersistedFindings.length, already correct) — so the same message contradicts itselfThe fix
Scoped entirely to
run-scan.job.ts.output-parser.tsis untouched —parseEngineOutputis a correctly-named pure parser of the engine's own artifact, and changing its meaning would be a bigger, less obviously correct change.Built
scanSummaryonce, right afterpersistedFindingsis known:persistedFindings.length === engineResult.output.findingCount(the common case — nothing else contributed, nothing filtered) — pass the engine's text through unchanged."Engine status: completed. 0 finding(s) reported. 39 finding(s) retained after all scanner layers and deduplication."All four downstream writes (
prisma.scan.update, the writecompleteScanWithScoreperforms,notifyScanCompleted's body, and the job's return value) now use this one corrected value instead of four separate references to the engine-only string.No scoring risk:
completeScanWithScore'ssummaryparameter is display-text only (score-service.ts:244) — it never touchescomputeScore, the breakdown, or theScoreSnapshotitself, all of which query the liveFindingtable independently. This does not touch the public-scorecard allowlist or the score model.Tests
Traced every existing assertion in
run-scan.job.test.tsby hand: the file's default mocks always havepersistFindingsresolving to the same count as the mockedfindingCount(usually both 0), so the corrective branch never fires for any existing test — zero existing expectations changed. Added two new tests:Verification
Sandbox has no
node_modulesand npm returns 403, sovitest/tsc/eslintcould not run locally. I traced every call site ofengineResult.output.summary, every consumer ofscan.summaryacross the repo, and every existing test assertion in the affected file by hand instead. CI on this PR is the execution gate — will report back once it's green.Summary by CodeRabbit