From 1bd7e7777f8b19981da7c628363fb8c33a2b3a40 Mon Sep 17 00:00:00 2001 From: Rohan Poudel Date: Wed, 5 Aug 2026 12:37:24 -0600 Subject: [PATCH] fix(contract): surface run warnings in the SARIF projection 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. --- .../scripts/finalize_scan_contract.py | 86 ++++++++++++++----- .../_bundled_plugin/scripts/workbench_db.py | 8 +- sdk/typescript/tests-ts/scan-recovery.test.ts | 66 ++++++++++++++ 3 files changed, 138 insertions(+), 22 deletions(-) diff --git a/sdk/typescript/_bundled_plugin/scripts/finalize_scan_contract.py b/sdk/typescript/_bundled_plugin/scripts/finalize_scan_contract.py index 0d545f27..f4b45493 100644 --- a/sdk/typescript/_bundled_plugin/scripts/finalize_scan_contract.py +++ b/sdk/typescript/_bundled_plugin/scripts/finalize_scan_contract.py @@ -17,7 +17,7 @@ import secrets import stat import sys -from collections.abc import Iterator +from collections.abc import Iterator, Sequence from datetime import datetime, timezone from pathlib import Path, PurePosixPath from typing import Any, BinaryIO, TextIO @@ -1979,6 +1979,32 @@ def _validate_sarif(sarif: dict[str, Any]) -> None: raise ContractError("SARIF: result references an unknown rule") if not result.get("partialFingerprints"): raise ContractError("SARIF: result is missing partialFingerprints") + _validate_sarif_invocations(run.get("invocations")) + + +def _validate_sarif_invocations(invocations: Any) -> None: + if invocations is None: + return + if not isinstance(invocations, list) or not invocations: + raise ContractError("SARIF: invocations must be a non-empty array when present") + for invocation in invocations: + if not isinstance(invocation, dict): + raise ContractError("SARIF: expected an invocation object") + if invocation.get("executionSuccessful") is not True: + raise ContractError("SARIF: invocation is missing executionSuccessful") + notifications = invocation.get("toolExecutionNotifications") + if not isinstance(notifications, list) or not notifications: + raise ContractError("SARIF: invocation has no toolExecutionNotifications") + for notification in notifications: + if not isinstance(notification, dict): + raise ContractError("SARIF: expected a notification object") + if notification.get("level") not in {"none", "note", "warning", "error"}: + raise ContractError("SARIF: notification has an unsupported level") + message = notification.get("message") + if not isinstance(message, dict) or not isinstance(message.get("text"), str): + raise ContractError("SARIF: notification is missing message text") + if not message["text"].strip(): + raise ContractError("SARIF: notification message text is empty") def _artifact_record( @@ -2085,7 +2111,10 @@ def _read_sealed_scan( def build_sarif_projection( - scan_dir: Path, source_root: Path | None = None, schema_dir: Path | None = None + scan_dir: Path, + source_root: Path | None = None, + schema_dir: Path | None = None, + warnings: Sequence[str] | None = None, ) -> dict[str, Any]: if source_root is not None: try: @@ -2097,27 +2126,39 @@ def build_sarif_projection( raise ContractError("source root: expected an existing directory") manifest, findings, coverage, _ = _read_sealed_scan(scan_dir, schema_dir, "SARIF projection") sarif = build_sarif(manifest, findings, source_root) + run = sarif["runs"][0] + notifications: list[dict[str, Any]] = [] + reported: set[str] = set() 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"] - ], - } - ] + for item in coverage["deferred"]: + reported.add(item["reason"]) + notifications.append({"level": "warning", "message": {"text": item["reason"]}}) + # Run warnings are reported whatever the completeness. A scan whose target drifted + # reviewed everything it set out to review, so it stays complete; the tree simply moved + # underneath it, and `toolExecutionNotifications` is where SARIF expects to read that. + # Deferred coverage already contributes its reason verbatim as a warning, so the same + # text is not notified twice. + for warning in warnings or (): + if not isinstance(warning, str) or not warning or warning in reported: + continue + reported.add(warning) + notifications.append({"level": "warning", "message": {"text": warning}}) + if notifications: + run["invocations"] = [ + {"executionSuccessful": True, "toolExecutionNotifications": notifications} + ] _validate_sarif(sarif) return sarif def write_sarif_projection( - scan_dir: Path, source_root: Path | None = None, schema_dir: Path | None = None + scan_dir: Path, + source_root: Path | None = None, + schema_dir: Path | None = None, + warnings: Sequence[str] | None = None, ) -> None: - sarif = build_sarif_projection(scan_dir, source_root, schema_dir) + sarif = build_sarif_projection(scan_dir, source_root, schema_dir, warnings) _write_scan_local_json(scan_dir, "exports/results.sarif", sarif) @@ -2296,10 +2337,13 @@ def write_export_output(scan_dir: Path, output: Path, export_format: str, conten def _write_sarif_projection_if_possible( - scan_dir: Path, source_root: Path | None = None, schema_dir: Path | None = None + scan_dir: Path, + source_root: Path | None = None, + schema_dir: Path | None = None, + warnings: Sequence[str] | None = None, ) -> None: try: - write_sarif_projection(scan_dir, source_root, schema_dir) + write_sarif_projection(scan_dir, source_root, schema_dir, warnings) except (ContractError, OSError) as error: print( f"codex-security: warning: automatic SARIF export failed: {error}. " @@ -2444,6 +2488,7 @@ def _prepare_scan_finalization( def _write_prepared_scan_finalization( prepared: PreparedScanFinalization, source_root: Path | None = None, + warnings: Sequence[str] | None = None, ) -> tuple[dict[str, Any], dict[str, Any], dict[str, Any]]: """Write a previously validated scan finalization result.""" @@ -2460,7 +2505,7 @@ def _write_prepared_scan_finalization( if was_sealed: write_scan_local_bytes(scan_dir, "report.md", report_markdown_bytes) _remove_scan_local_file_if_exists(scan_dir, "report.html") - _write_sarif_projection_if_possible(scan_dir, source_root, schema_dir) + _write_sarif_projection_if_possible(scan_dir, source_root, schema_dir, warnings) return manifest, findings, coverage _write_scan_local_json(scan_dir, "findings.json", findings) @@ -2469,7 +2514,7 @@ def _write_prepared_scan_finalization( _remove_scan_local_file_if_exists(scan_dir, "report.html") _write_scan_local_json(scan_dir, "scan-manifest.json", manifest) _validate_existing_seal(scan_dir, scan) - _write_sarif_projection_if_possible(scan_dir, source_root, schema_dir) + _write_sarif_projection_if_possible(scan_dir, source_root, schema_dir, warnings) return manifest, findings, coverage @@ -2480,6 +2525,7 @@ def finalize_scan( *, expected_coverage_mode: str | None = None, completion_binding: dict[str, Any] | None = None, + warnings: Sequence[str] | None = None, ) -> tuple[dict[str, Any], dict[str, Any], dict[str, Any]]: prepared = _prepare_scan_finalization( scan_dir, @@ -2487,7 +2533,7 @@ def finalize_scan( expected_coverage_mode=expected_coverage_mode, completion_binding=completion_binding, ) - return _write_prepared_scan_finalization(prepared, source_root) + return _write_prepared_scan_finalization(prepared, source_root, warnings) def main() -> int: diff --git a/sdk/typescript/_bundled_plugin/scripts/workbench_db.py b/sdk/typescript/_bundled_plugin/scripts/workbench_db.py index ac669cc8..5cdfa84f 100644 --- a/sdk/typescript/_bundled_plugin/scripts/workbench_db.py +++ b/sdk/typescript/_bundled_plugin/scripts/workbench_db.py @@ -1461,7 +1461,7 @@ def complete_scan_locked( target_warnings.append(warning) if warning not in warnings: warnings.append(warning) - manifest, findings, _ = _write_prepared_scan_finalization(prepared) + manifest, findings, _ = _write_prepared_scan_finalization(prepared, warnings=warnings) except ContractError as exc: raise SystemExit(str(exc)) from exc artifacts = { @@ -2362,10 +2362,14 @@ def export_findings(connection: sqlite3.Connection, args: argparse.Namespace) -> scan_dir = require_canonical_scan_directory(Path(scan["scan_dir"])) require_recorded_manifest_digest(scan, scan_dir) verify_manifest_binding(scan, read_json_object(scan_dir / ARTIFACTS["manifest"])) + # Re-exporting has to reproduce the notifications the scan recorded at completion, + # so the warnings are read back rather than left behind with the completed scan. + completion_warnings = json.loads(scan["completion_warnings_json"]) try: manifest, _, _ = finalize_scan( scan_dir, expected_coverage_mode=expected_coverage_mode(scan), + warnings=completion_warnings, ) except ContractError as exc: raise SystemExit(str(exc)) from exc @@ -2376,7 +2380,7 @@ def export_findings(connection: sqlite3.Connection, args: argparse.Namespace) -> path = artifact_path(scan_dir, ARTIFACTS["findings"], required=True) elif args.format == "sarif": try: - write_sarif_projection(scan_dir) + write_sarif_projection(scan_dir, warnings=completion_warnings) except ContractError as exc: raise SystemExit(str(exc)) from exc path = artifact_path(scan_dir, "exports/results.sarif", required=True) diff --git a/sdk/typescript/tests-ts/scan-recovery.test.ts b/sdk/typescript/tests-ts/scan-recovery.test.ts index 153e5d4b..597a373f 100644 --- a/sdk/typescript/tests-ts/scan-recovery.test.ts +++ b/sdk/typescript/tests-ts/scan-recovery.test.ts @@ -765,6 +765,72 @@ describe("malformed scan artifact recovery", () => { ]); }); + test("reports a drifted target in SARIF while coverage stays complete", async () => { + const fixture = await startDraftScan(); + // Drift the target after registration recorded its snapshot digest. The scan still + // reviewed everything it set out to review, so completeness stays complete and the + // deferred-coverage route that used to be the only source of notifications is empty. + await writeFile( + join(fixture.repository, "src", "extract.py"), + "# changed while the scan was running\n", + ); + + const completed = await completeScan(fixture); + + expect(completed.progress.status).toBe("complete"); + expect(completed.warnings).toHaveLength(1); + expect(completed.warnings[0]).toContain("Directory contents changed"); + const coverage = await readJson( + join(fixture.scanDir, "coverage.json"), + ); + expect(coverage.completeness).toBe("complete"); + expect(coverage.deferred).toEqual([]); + const sarif = await readJson( + join(fixture.scanDir, "exports", "results.sarif"), + ); + expect( + sarif.runs[0]?.properties.codexSecurityCoverageCompleteness, + ).toBeUndefined(); + expect(sarif.runs[0]?.invocations).toEqual([ + { + executionSuccessful: true, + toolExecutionNotifications: [ + { level: "warning", message: { text: completed.warnings[0]! } }, + ], + }, + ]); + }); + + test("keeps run warnings in SARIF when the export is regenerated", async () => { + const fixture = await startDraftScan(); + await writeFile( + join(fixture.repository, "src", "extract.py"), + "# changed while the scan was running\n", + ); + const completed = await completeScan(fixture); + await rm(join(fixture.scanDir, "exports", "results.sarif")); + + await workbench(fixture, [ + "export-findings", + "--scan-id", + fixture.scanId, + "--format", + "sarif", + ]); + + const sarif = await readJson( + join(fixture.scanDir, "exports", "results.sarif"), + ); + expect(sarif.runs[0]?.invocations).toEqual([ + { + executionSuccessful: true, + toolExecutionNotifications: [ + { level: "warning", message: { text: completed.warnings[0]! } }, + ], + }, + ]); + }); + test("keeps findings while removing invalid or duplicate writeups", async () => { const fixture = await startDraftScan(); const path = join(fixture.scanDir, "findings.json");