Skip to content

fix(scan): surface run warnings in structured scan results - #220

Open
rohanpoudel2 wants to merge 2 commits into
openai:mainfrom
rohanpoudel2:fix/scan-warnings
Open

fix(scan): surface run warnings in structured scan results#220
rohanpoudel2 wants to merge 2 commits into
openai:mainfrom
rohanpoudel2:fix/scan-warnings

Conversation

@rohanpoudel2

Copy link
Copy Markdown

Refs #195

Problem

A scan whose target changes mid-run completes against the original snapshot and produces a warning:

Repository HEAD changed while the scan was running; results were saved for the original revision.

That warning reached the onWarning observer and nothing else. The CLI's observer writes it to stderr, while scan --json prints result.toJSON() to stdout — and ScanResult.toJSON() had no warnings key at all. A CI job therefore received exit 0 and a JSON document indistinguishable from a clean scan of the commit it actually checked out. The one signal that the results were for a stale tree was unavailable to any machine consumer.

The same gap affected the other two warning producers, not just target drift:

  • the cost-limit-unverifiable warning from onFinalize
  • the cleanup-failure warnings from warnCleanupFailed

Change

Warnings now travel with the result. ScanResult gains a warnings: readonly string[] field, always emitted by toJSON() and therefore present in --json output.

toJSON() already emits every key unconditionally (sarifPath: null, cost: null), and the workbench's own scans show --json always emits a warnings array, so an always-present array is consistent here. It also lets a CI job write .warnings.length > 0 without having to distinguish "no warnings" from "an older SDK".

Two details worth calling out for review:

  • Collection happens in run(), and the warnings are attached after #run returns. ScanResult is constructed in collectResult() before complete-scan runs, so the drift warning does not exist yet at construction time, and cleanup warnings arrive later still, from #run's finally. run() owns the array and passes a warn reporter into #run; the three emission sites call it instead of notifyObserver directly. Recording is synchronous at the emission site rather than inside the observer callback, because notifyObserver dispatches on a microtask and depending on that ordering would be fragile.
  • Redaction happens where the warning is recorded, not in ScanResult. warn stores redactedErrorMessage(warning) while forwarding the raw string to onWarning, so existing observer behavior is byte-for-byte unchanged. This mirrors the fail-scan path, which redacts before storing because the stored text is read back by scans show.

ScanResult.withWarnings() returns this when there is nothing to add, so the common path allocates nothing.

Deliberately not included

  • No exit-code change. A drifted scan still exits 0. Making drift fail a build — whether by a --fail-on-warning flag or by promoting drift to exit 1 — is a breaking change for existing pipelines and is yours to decide. This PR only makes the condition detectable; A scan whose target changed mid-run exits 0 and reports nothing in --json, so CI cannot detect that the results are for a stale tree #195 asks for both, and I would rather not decide the second half unilaterally.
  • No _bundled_plugin/ change. coverage.completeness stays complete on a drifted run. Degrading it would ripple into the CLI's existing completeness !== "complete" → exit 2 path, which is the same behavior change wearing a different hat.
  • No export change. Warnings live in the workbench's completion_warnings_json column, not in scan-directory artifacts, so surfacing them there would require changing the bundled Python exporter and its output schemas.
  • scans show --json and scans list --json already include warnings; no change was needed there.

Verification

The load-bearing test is in cli.test.ts: it asserts the exact drift string lands in parsed --json stdout, not merely stderr, and that the exit code stays 0. Before the change it failed with Received: undefined; result.test.ts and api.test.ts gained matching assertions.

Full suite: 719 pass / 5 skip / 0 fail (717 baseline plus 2 new tests). pnpm run types and pnpm run format are clean.

A scan whose target changes mid-run completes against the original
snapshot and reports "Repository HEAD changed while the scan was
running; results were saved for the original revision." That warning
only ever reached the `onWarning` observer, and the CLI observer only
writes it to stderr. `ScanResult.toJSON()` had no `warnings` key, so
`codex-security scan --json` exited 0 with a document indistinguishable
from a scan of the commit CI actually checked out. Nothing automated
could tell that the results were for a stale tree.

Collect the warnings a run reports and carry them on `ScanResult`, so
they appear in `toJSON()` and therefore in `--json` output. Warnings are
recorded where they are reported, including the ones cleanup produces
after the result is collected, and are redacted the same way the CLI
already redacts them before printing, because the result is written to
CI logs and artifacts while the observer stream is not. The key is
always present and empty when a run reported nothing, matching
`toJSON()`, which emits every key, and `scans show --json`, which always
emits a `warnings` array.

Observer behavior, exit codes, and every existing key are unchanged.
`scans list --json` and `scans show --json` already carry the stored
warnings and need no change.

Refs openai#195
@github-actions github-actions Bot added the bug Something isn't working label Aug 3, 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.

1 participant