Skip to content

fix(contract): surface run warnings in the SARIF projection - #285

Open
rohanpoudel2 wants to merge 1 commit into
openai:mainfrom
rohanpoudel2:fix/sarif-run-warnings
Open

fix(contract): surface run warnings in the SARIF projection#285
rohanpoudel2 wants to merge 1 commit into
openai:mainfrom
rohanpoudel2:fix/sarif-run-warnings

Conversation

@rohanpoudel2

Copy link
Copy Markdown

Fixes #251

Problem

build_sarif_projection built the whole invocations block inside a single completeness guard:

if coverage["completeness"] != "complete":
    run = sarif["runs"][0]
    run["properties"]["codexSecurityCoverageCompleteness"] = coverage["completeness"]
    if coverage["deferred"]:
        run["invocations"] = [
            {
                "executionSuccessful": True,
                "toolExecutionNotifications": [
                    {"level": "warning", "message": {"text": item["reason"]}}
                    for item in coverage["deferred"]
                ],
            }
        ]

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_warning fires when the tree moved underneath a running scan, but the scan still reviewed everything it set out to review, so completeness stays complete and deferred stays 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, and build_sarif_projection reads only the sealed scan directory through _read_sealed_scan — it never saw them at any completeness. Measured on upstream/main @ 18a183f with a directory scan drifted between register-cli-scan and complete-scan:

status       : complete
warnings     : ["Directory contents changed while the scan was running; ..."]
completeness : complete
deferred     : []
invocations  : undefined     <-- the warning exists and SARIF cannot express it

Change

build_sarif_projection takes an optional warnings sequence 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, and export_findings, which reads it back from the scan row so a regenerated export reproduces the same notifications rather than silently dropping them.

_validate_sarif gained _validate_sarif_invocations, which checks the block it never previously had to: non-empty array, executionSuccessful true, 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_dir with 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 completeness is forced to partial whenever deferred is 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_locked could 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 one set in the place that builds both halves.

Impact, stated plainly

SARIF consumers now see drift as a level: "warning" notification with executionSuccessful: 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. codexSecurityCoverageCompleteness is 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.py passes warnings= by keyword, so reverting finalize_scan_contract.py alone raises TypeError: _write_prepared_scan_finalization() got an unexpected keyword argument 'warnings' rather than failing an assertion. Reverting workbench_db.py alone 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.

State tests-ts/scan-recovery.test.ts
With the fix 24 pass, 0 fail
Both Python layers reverted 22 pass, 2 fail
workbench_db.py reverted only 22 pass, 2 fail

Full suite with the fix: 909 pass, 11 skip, 0 fail across 36 files (907 before; the two added tests are the difference). pnpm run types and pnpm run format are both clean.

Both failures in the reverted runs are the new tests, and both fail on the same assertion — invocations is undefined where 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 complete drifts the target between registration and completion, then asserts the combination the old guard made unreachable: completeness is complete, deferred is [], codexSecurityCoverageCompleteness is absent, and invocations carries the drift warning.
  • keeps run warnings in SARIF when the export is regenerated deletes exports/results.sarif, re-runs export-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 malformed test 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.

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.
@github-actions github-actions Bot added the bug Something isn't working label Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Run warnings never reach the SARIF projection, so drift is invisible to SARIF consumers

1 participant