fix(contract): surface run warnings in the SARIF projection - #285
Open
rohanpoudel2 wants to merge 1 commit into
Open
fix(contract): surface run warnings in the SARIF projection#285rohanpoudel2 wants to merge 1 commit into
rohanpoudel2 wants to merge 1 commit into
Conversation
A scan whose target drifted mid-run records a warning, but the SARIF projection only ever built toolExecutionNotifications from deferred coverage rows, and only when completeness was not complete. A drifted target leaves completeness at complete, so no invocations block was emitted and the warning had no route into SARIF at all. Run warnings now reach the projection independently of completeness, deduplicated against the deferred reasons that already notify verbatim.
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.
Fixes #251
Problem
build_sarif_projectionbuilt the wholeinvocationsblock inside a single completeness guard:Two separate things had to be true before SARIF carried a notification: coverage had to be incomplete, and the reason had to come from a deferred coverage row.
A drifted target satisfies neither.
scan_target_warningfires when the tree moved underneath a running scan, but the scan still reviewed everything it set out to review, socompletenessstayscompleteanddeferredstays empty. The warning was recorded, and then had nowhere to go.The route was missing rather than merely gated. Run warnings live in
scans.completion_warnings_json, andbuild_sarif_projectionreads only the sealed scan directory through_read_sealed_scan— it never saw them at any completeness. Measured onupstream/main@18a183fwith a directory scan drifted betweenregister-cli-scanandcomplete-scan:Change
build_sarif_projectiontakes an optionalwarningssequence and collects notifications from both sources into one list. Deferred coverage keeps its existing behaviour exactly — still gated on completeness, still contributing its reason verbatim. Run warnings are appended whatever the completeness, skipping any text a deferred row already notified.The two call sites that can actually know the warnings now pass them:
complete_scan_locked, which already holds the merged list it is about to persist, andexport_findings, which reads it back from the scan row so a regenerated export reproduces the same notifications rather than silently dropping them._validate_sarifgained_validate_sarif_invocations, which checks the block it never previously had to: non-empty array,executionSuccessfultrue, at least one notification, a level within the SARIF enum, and non-blank message text.Why not put the warnings in the sealed artifacts
Persisting warnings into the manifest or coverage document would let the projection read them straight from
scan_dirwith no parameter threading. It would also change what is sealed, so every existing scan directory would project differently after upgrade and the seal digests would have to be revised. The warnings already have a durable home in the database; SARIF is a projection, and projecting from the recorded state is what the rest of this function already does.Why not drop the completeness guard entirely
Making deferred coverage notify unconditionally would collapse the two branches into one and read more simply. But
completenessis forced topartialwheneverdeferredis non-empty, so the guard is load-bearing documentation of that invariant rather than dead weight, and removing it would change what a complete scan with a stale deferred row projects. This keeps the existing branch untouched and adds the run-level warnings alongside, which is what #251 proposed.Why not deduplicate in the callers
complete_scan_lockedcould filter the warnings it passes. It would have to know which strings the coverage document is going to notify, which is precisely what the projection knows and the caller does not. The dedupe is onesetin the place that builds both halves.Impact, stated plainly
SARIF consumers now see drift as a
level: "warning"notification withexecutionSuccessful: true, which is the SARIF idiom for "the run completed but read this before trusting it". A CI system that previously saw a clean complete run over a moved tree now has a machine-readable signal.Nothing about findings, results, fingerprints, or the seal changes. A scan with no run warnings and no deferred coverage projects byte-identically to before.
codexSecurityCoverageCompletenessis still written only when coverage is incomplete, so a drifted-but-complete scan gains the notification without gaining a property that would misreport its coverage.The two Python layers are not independently revertible:
workbench_db.pypasseswarnings=by keyword, so revertingfinalize_scan_contract.pyalone raisesTypeError: _write_prepared_scan_finalization() got an unexpected keyword argument 'warnings'rather than failing an assertion. Revertingworkbench_db.pyalone is clean and is measured below.Verification
macOS 15 (Darwin 25.5.0), bun 1.3.14, Node 24.11.1, Python 3.14.5, branched from
upstream/main@18a183f.tests-ts/scan-recovery.test.tsworkbench_db.pyreverted onlyFull suite with the fix: 909 pass, 11 skip, 0 fail across 36 files (907 before; the two added tests are the difference).
pnpm run typesandpnpm run formatare both clean.Both failures in the reverted runs are the new tests, and both fail on the same assertion —
invocationsisundefinedwhere the notification is expected — rather than erroring out, so they fail for the reason they are meant to catch.Two tests were added to
scan-recovery.test.ts:reports a drifted target in SARIF while coverage stays completedrifts the target between registration and completion, then asserts the combination the old guard made unreachable:completenessiscomplete,deferredis[],codexSecurityCoverageCompletenessis absent, andinvocationscarries the drift warning.keeps run warnings in SARIF when the export is regenerateddeletesexports/results.sarif, re-runsexport-findings --format sarif, and asserts the notification comes back — covering the read-back path rather than only the write at completion.The existing
completes scans when every draft finding is malformedtest is the dedupe guard and was left unchanged: there the run warning and the deferred reason are the same string, and it still asserts exactly one notification. It passes with the fix, which is what shows the dedupe holds.