From 1bd7e7777f8b19981da7c628363fb8c33a2b3a40 Mon Sep 17 00:00:00 2001 From: Rohan Poudel Date: Wed, 5 Aug 2026 12:37:24 -0600 Subject: [PATCH 1/2] 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 0d545f275..f4b454932 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 ac669cc85..5cdfa84f1 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 153e5d4b3..597a373f3 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"); From bb160f3c12510ad28e1b83aac0d5560683e4bcee Mon Sep 17 00:00:00 2001 From: Rohan Poudel <66029221+rohanpoudel2@users.noreply.github.com> Date: Fri, 21 Aug 2026 17:50:37 -0600 Subject: [PATCH 2/2] fix(contract): pass run warnings through regenerated SARIF exports Re-finalizing an already-complete scan and backfilling legacy finding details both rewrite exports/results.sarif through finalize_scan without the warnings the scan recorded, so a regenerated projection dropped the notifications the first completion had written. Both paths now read the persisted warnings back, matching what regenerated exports already do. Reconcile the projection with the coverage-derived execution status added in #601. Success still comes from coverage completeness and the invocation is still always emitted, while run warnings join the notification list whatever the completeness. Invocation validation accepts an unsuccessful execution and an absent notification list, since a complete scan with no warnings notifies nothing. --- .../references/sarif-adapter.md | 2 +- .../_bundled_plugin/scripts/workbench_db.py | 4 + sdk/typescript/tests-ts/cli-export.test.ts | 2 +- sdk/typescript/tests-ts/scan-recovery.test.ts | 109 +++++++++++++++++- 4 files changed, 114 insertions(+), 3 deletions(-) diff --git a/sdk/typescript/_bundled_plugin/references/sarif-adapter.md b/sdk/typescript/_bundled_plugin/references/sarif-adapter.md index 57feb334d..1eda1438e 100644 --- a/sdk/typescript/_bundled_plugin/references/sarif-adapter.md +++ b/sdk/typescript/_bundled_plugin/references/sarif-adapter.md @@ -20,7 +20,7 @@ The adapter: Lifecycle, rich validation evidence, attack-path context, and coverage are lossy or omitted in SARIF. Preserve them in semantic JSON. -SARIF `executionSuccessful` is true only for complete coverage. Incomplete scans retain findings and warnings. Integrations must read the coverage file referenced by `scan.coverageRef`; manifest status `completed` alone does not mean success. +SARIF `executionSuccessful` comes from coverage and is true only for complete coverage. Notifications report incomplete-coverage reasons and recorded run warnings; a complete scan can therefore succeed while still carrying a warning. Integrations must read the coverage file referenced by `scan.coverageRef`; manifest status `completed` alone does not mean success. Automatic SARIF export during finalization is best-effort so projection errors cannot invalidate a canonical seal. Use the strict adapter entry point when a consumer requires SARIF and should surface export errors. diff --git a/sdk/typescript/_bundled_plugin/scripts/workbench_db.py b/sdk/typescript/_bundled_plugin/scripts/workbench_db.py index 6bca8f832..968d08545 100644 --- a/sdk/typescript/_bundled_plugin/scripts/workbench_db.py +++ b/sdk/typescript/_bundled_plugin/scripts/workbench_db.py @@ -1433,10 +1433,12 @@ def complete_scan_locked( 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"])) + 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 @@ -3368,9 +3370,11 @@ def backfill_legacy_finding_details(connection: sqlite3.Connection, scan: sqlite 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"])) + completion_warnings = json.loads(scan["completion_warnings_json"]) manifest, findings_document, _ = finalize_scan( scan_dir, expected_coverage_mode=expected_coverage_mode(scan), + warnings=completion_warnings, ) verify_manifest_binding(scan, manifest) manifest_digest = published_manifest_digest(scan_dir, manifest) diff --git a/sdk/typescript/tests-ts/cli-export.test.ts b/sdk/typescript/tests-ts/cli-export.test.ts index e8e58c8a4..741a9665a 100644 --- a/sdk/typescript/tests-ts/cli-export.test.ts +++ b/sdk/typescript/tests-ts/cli-export.test.ts @@ -418,7 +418,7 @@ describe("CLI", () => { completeness === "complete" ? undefined : completeness, ); if (completeness === "complete") { - expect(invocation.toolExecutionNotifications).toBeUndefined(); + expect(invocation).not.toHaveProperty("toolExecutionNotifications"); } else { const reasons = hasDeferred ? coverage.deferred.map((item) => item.reason) diff --git a/sdk/typescript/tests-ts/scan-recovery.test.ts b/sdk/typescript/tests-ts/scan-recovery.test.ts index de3df5097..411d41d01 100644 --- a/sdk/typescript/tests-ts/scan-recovery.test.ts +++ b/sdk/typescript/tests-ts/scan-recovery.test.ts @@ -67,7 +67,7 @@ type SarifDocument = { results: Array<{ properties: { severity: string } }>; invocations?: Array<{ executionSuccessful: boolean; - toolExecutionNotifications: Array<{ + toolExecutionNotifications?: Array<{ level: string; message: { text: string }; }>; @@ -1071,6 +1071,43 @@ describe("malformed scan artifact recovery", () => { ]); }); + test("combines incomplete coverage and run warnings without duplicate notifications", async () => { + const fixture = await startDraftScan(); + const findingsPath = join(fixture.scanDir, "findings.json"); + const document = await readJson(findingsPath); + document.findings[0]!.summary = ""; + await writeJson(findingsPath, document); + await writeFile( + join(fixture.repository, "src", "extract.py"), + "# changed while the scan was running\n", + ); + + const completed = await completeScan(fixture); + + const coverage = await readJson( + join(fixture.scanDir, "coverage.json"), + ); + expect(coverage.completeness).toBe("partial"); + const coverageReason = ( + coverage.deferred as Array<{ id: string; reason: string }> + )[0]!.reason; + const runWarning = completed.warnings.find((warning) => + warning.includes("Directory contents changed"), + ); + expect(runWarning).toBeDefined(); + expect(completed.warnings).toContain(coverageReason); + const sarif = await readJson( + join(fixture.scanDir, "exports", "results.sarif"), + ); + const invocation = sarif.runs[0]?.invocations?.[0]; + expect(invocation?.executionSuccessful).toBe(false); + const notificationTexts = invocation?.toolExecutionNotifications?.map( + (notification) => notification.message.text, + ); + expect(notificationTexts).toEqual([coverageReason, runWarning!]); + expect(new Set(notificationTexts!).size).toBe(notificationTexts!.length); + }); + 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 @@ -1137,6 +1174,76 @@ describe("malformed scan artifact recovery", () => { ]); }); + test("keeps run warnings in SARIF when a complete scan is finalized again", async () => { + const fixture = await startDraftScan(); + await writeFile( + join(fixture.repository, "src", "extract.py"), + "# changed while the scan was running\n", + ); + const firstCompletion = await completeScan(fixture); + await rm(join(fixture.scanDir, "exports", "results.sarif")); + + const secondCompletion = await completeScan(fixture); + + expect(secondCompletion.warnings).toEqual(firstCompletion.warnings); + const sarif = await readJson( + join(fixture.scanDir, "exports", "results.sarif"), + ); + expect(sarif.runs[0]?.invocations).toEqual([ + { + executionSuccessful: true, + toolExecutionNotifications: [ + { + level: "warning", + message: { text: firstCompletion.warnings[0]! }, + }, + ], + }, + ]); + }); + + test("keeps run warnings in SARIF during legacy finding backfill", 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); + const madeLegacy = spawnSync( + fixture.python, + [ + "-I", + "-B", + "-c", + [ + "import sqlite3, sys", + "connection = sqlite3.connect(sys.argv[1])", + `connection.execute("UPDATE finding_occurrences SET details_json = '{}' WHERE scan_id = ?", (sys.argv[2],))`, + "connection.commit()", + ].join("\n"), + join(fixture.stateDir, "workbench.sqlite3"), + fixture.scanId, + ], + { encoding: "utf8" }, + ); + expect(madeLegacy.status, madeLegacy.stderr).toBe(0); + await rm(join(fixture.scanDir, "exports", "results.sarif")); + + await workbench(fixture, ["get-scan", "--scan-id", fixture.scanId]); + + 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");