Skip to content

fix(multiscan): record run warnings in the campaign ledger - #295

Open
ppcvote wants to merge 2 commits into
openai:mainfrom
ppcvote:fix/multiscan-run-warnings
Open

fix(multiscan): record run warnings in the campaign ledger#295
ppcvote wants to merge 2 commits into
openai:mainfrom
ppcvote:fix/multiscan-run-warnings

Conversation

@ppcvote

@ppcvote ppcvote commented Aug 6, 2026

Copy link
Copy Markdown

Fixes #248.

runMultiscan called security.run() with no onWarning observer, so every warning the scan raised was discarded. A repository whose target drifted mid-run still completes, so it landed in results.jsonl as status: "completed" with nothing in the campaign output recording that the results describe a tree that moved.

The receipt now carries a warnings array when the scan raised any:

{"id":"drifted","status":"completed","attempt":1,"outputDir":"...",
 "warnings":[{"message":"Scan target changed during the run.","kind":"target_changed"}]}

kind comes from ScanWarningDetails, so a consumer can single out drift without matching on message text — the same distinction the CLI already makes at its own onWarning.

Three decisions worth review

Redaction. Warnings go through redactedErrorMessage, the redaction the failure path already applies. The ledger is a file on disk and a warning can quote a remote or a token. The CLI uses sanitizeDiagnosticValue for this, but that is private to cli.ts; redactedErrorMessage is the equivalent already imported into multiscan.ts. Happy to export the CLI one instead if you would rather have a single sanitizer.

Per attempt. The array is declared beside failure and cost inside the retry loop, so a second attempt does not inherit the first attempt's warnings.

status is untouched. The scan completed; promoting a warning to a failure would be a louder change than the issue asks for and would break resume, since completed receipts are what let a rerun skip finished repositories. The key is omitted entirely when nothing was raised, so a quiet run produces byte-identical output to today.

Verification

Windows 10, bun 1.3.11, bun test tests-ts/multiscan.test.ts:

with fix without fix
records run warnings in the receipt pass fail
redacts credential-shaped values pass fail
omits warnings when none raised pass pass (control)
totals 16 pass, 1 fail 14 pass, 3 fail

The two failing-without-the-fix tests are the ones that matter; the third is a control that passes either way and guards against warnings appearing on a quiet run.

The single unrelated failure in both columns is rejects output-directory symlinks, which needs symlink privileges this host does not grant. It fails identically on unmodified main, so it is not residue from this change.

Related

This is the multiscan half of the same shape as #251 and #195: the tool knew something and the knowledge did not reach the artifact a consumer reads. Those are about the single-scan --json and SARIF surfaces; this one is the ledger. I have not touched either of those paths — #251 already has someone working on it.

Fixes openai#248.

`runMultiscan` called `security.run()` with no `onWarning` observer, so every
warning the scan raised was dropped. A repository whose target drifted mid-run
still completes, so it landed in results.jsonl as `status: "completed"` with
nothing anywhere in the campaign output saying the results describe a tree that
moved. bulk-scan is the mode where nobody is watching an individual repository,
which is what makes the silence expensive.

The receipt now carries a `warnings` array when the scan raised any, each entry
`{ message, kind? }`. `kind` is preserved from ScanWarningDetails so a consumer
can single out drift without matching on message text, which is what the CLI
already does at the other call site.

Three details worth review:

- Warnings go through `redactedErrorMessage`, the same redaction the failure
  path already applies. The ledger is a file on disk, and a warning can quote a
  remote or a token, so it gets the same treatment as an error. The CLI runs its
  warnings through `sanitizeDiagnosticValue`, but that is private to cli.ts;
  `redactedErrorMessage` is the equivalent already imported here.
- The array is per attempt, declared beside `failure` and `cost`, so a retry
  does not inherit the previous attempt's warnings.
- `status` is untouched. The scan did complete, and turning a warning into a
  failure would be a different and much louder change than the issue asks for.
  The key is omitted entirely when nothing was raised, so existing consumers
  see no new field on a quiet run.

Three tests added. Two of them fail against the current code, which is the
point; the third is the quiet-run control that passes either way and guards
against warnings appearing spuriously.

Verified on Windows 10, bun 1.3.11: 16 pass with the fix, 14 pass and the two
new warning tests fail without it. The single unrelated failure in both runs is
"rejects output-directory symlinks", which needs symlink privileges this host
does not grant and fails identically on unmodified main.
@github-actions github-actions Bot added the bug Something isn't working label Aug 6, 2026
main added a coverage warning path since this branch opened: `warning?: string`
on the receipt, a `completed_with_incomplete_coverage` status, and an
`incomplete` tally. Three conflicts, all in multiscan.ts.

Resolved by keeping both signals separate rather than folding one into the
other, because their semantics differ in a way that matters:

`warning` is derived locally from `result.coverage.completeness`, and its
presence flips `status` to `completed_with_incomplete_coverage`. The scan's own
warnings must not do that. A repository whose target drifted mid-run has
*complete* coverage — the scan reviewed everything it set out to review, the
tree moved underneath it — so routing a drift warning into `warning` would
report incomplete coverage for a run whose coverage was fine.

So the observer-sourced warnings land in `scanWarnings`, renamed from
`warnings` to remove the collision with main's singular `warning`. The name now
says which layer raised it: the campaign derived `warning`, the scan raised
`scanWarnings`.

main's `scanPrompt` / `postScanPrompt` options are kept as-is; the `onWarning`
observer is added alongside them.

The underlying defect from openai#248 is untouched by main's work: `security.run()`
still receives no `onWarning`, so warnings the scan raises are still discarded.
Coverage was a locally computed signal and never went through that path.

38 pass. The single failure is `rejects output-directory symlinks`, which needs
symlink privileges this host does not grant and fails identically on unmodified
main.
@ppcvote
ppcvote force-pushed the fix/multiscan-run-warnings branch from 6cfdf7b to 6fdb94d Compare August 8, 2026 19:52
@ppcvote

ppcvote commented Aug 8, 2026

Copy link
Copy Markdown
Author

Rebuilt against main, which had moved into this area and conflicted. Worth spelling out what changed, because the overlap is real but partial.

main now derives a warning from result.coverage.completeness, adds a completed_with_incomplete_coverage status and an incomplete tally. That covers the coverage case.

It does not cover what this PR is about. security.run() still receives no onWarning observer, so warnings the scan itself raises are still discarded. Coverage is computed locally from the returned result and never went through the observer path, so nothing about that change reaches the drift case in #248.

I kept the two signals separate rather than folding mine into main's warning, because their semantics differ in a way that would produce a wrong status:

  • warning is coverage-derived, and its presence flips status to completed_with_incomplete_coverage.
  • A repository whose target drifted mid-run has complete coverage. The scan reviewed everything it set out to review; the tree moved underneath it. Routing a drift warning into warning would report incomplete coverage for a run whose coverage was fine, and move it out of the completed tally.

So the observer-sourced entries land in scanWarnings, renamed from warnings to remove the collision. The names now say which layer raised it: the campaign derives warning, the scan raises scanWarnings.

scanPrompt / postScanPrompt from main are kept as-is; onWarning is added alongside them.

38 pass. The one failure is rejects output-directory symlinks, which needs symlink privileges this host does not grant and fails identically on unmodified main.

Happy to merge the two into a single field if you would rather have one, but it would need the status coupling loosened first, and that felt like a larger decision than this PR should make on its own.

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.

bulk-scan discards run warnings, so a drifted repository is recorded as completed with no signal

1 participant