Skip to content

fix(multiscan): record run warnings on bulk-scan receipts - #255

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

fix(multiscan): record run warnings on bulk-scan receipts#255
rohanpoudel2 wants to merge 1 commit into
openai:mainfrom
rohanpoudel2:fix/multiscan-warnings

Conversation

@rohanpoudel2

Copy link
Copy Markdown

Fixes #248

Problem

runMultiscan never asks for a repository's warnings, and the per-attempt ledger receipt has nowhere to put them:

const result = await security.run(checkout, {
  ...(task.scope === undefined ? {} : { target: [task.scope] }),
  ...
  mode: task.mode,
  outputDir: scanDir,
  ...(options.signal === undefined ? {} : { signal: options.signal }),
});
`${JSON.stringify({
  ...task,
  status,
  attempt,
  outputDir: scanDir,
  ...(cost === null ? {} : { cost }),
  ...(failure === undefined ? {} : { error: failure }),
})}\n`

No onWarning, so every warning the run reports is dropped on the floor. A repository whose target drifted mid-run keeps coverage.completeness === "complete", so the attempt genuinely succeeds: it is written as status: "completed", bulk-scan exits 0, and the campaign summary counts it among completed. There is no failure, no non-zero status, and no field a consumer could read to notice the results describe a stale tree. The same holds for the other two warning producers — a cost limit that could not be verified, and the cleanup failures run() reports from its finally block.

Note for reviewers: ScanResult does not carry a warnings field on main. The only channel for these warnings is the onWarning observer.

Change

  • Each attempt installs an onWarning observer that collects that attempt's warnings into a fresh array. The array is per attempt, so a retry does not inherit the previous attempt's warnings.
  • Collected warnings go through redactedErrorMessage, the same way error does. The CLI's own observer only sanitizes for stderr; these strings are about to be written to disk and read back on resume, and the comment on warnCleanupFailed is explicit that the warning text has so far never been persisted, so it has never been redacted upstream.
  • The receipt gains an optional warnings?: string[], written only when the attempt produced warnings.
  • MultiscanResult gains warned: number beside failed. The exit code is untouched: a warning is not a failure.

Warnings from a failed attempt are captured too, and deliberately so. run()'s cleanup warnings are emitted from its finally block, so they happen whether the attempt returned a result or threw, and the completion and cost-limit warnings are emitted before a later step can throw. Reading result.warnings — even if that field existed — would therefore lose exactly the warnings that accompany a failure. The observer is also immune to the two multiscan-side throws that happen after run() returns (the scope-escape check and the coverage.completeness check), whose receipts now carry both error and warnings.

On ordering: run() dispatches observers on a microtask. The receipt is serialized after this loop's finally { await rm(checkout, ...) }, which runs once run() has settled, so every queued warning has landed before the receipt is written.

Why an omitted-when-empty warnings?: string[] and not an unconditional array

The alternative is to always emit the key, warnings: [] included, matching scan --json, which gets its unconditional warnings straight from workbench_db.py. Rejected, for two reasons:

  1. The receipt's own convention settles it. cost and error — and scope on the task half — are all omitted when absent. warnings: [] on every one of thousands of receipts would be the only field in the record that insists on being present in order to say nothing. scans show / scans list already omit the key when completion_warnings_json is "[]", so this side of the split is not novel either.
  2. The ledger is append-only and read back. Omission keeps a quiet attempt's receipt byte-identical to what releases before this change wrote, which keeps the diff between an old ledger and a new one confined to attempts that actually warned. "warnings" in receipt becomes a meaningful "this attempt warned" test rather than a tautology.

Ledger compatibility

  • An older ledger without the field still resumes. readReceipts does an unchecked JSON.parse(line) as MultiscanReceipt — there is no schema and no validator — and the resume path reads only id, status, outputDir and attempt. warnings is optional in exactly the way error is, so a receipt written before this change type-checks and resumes unchanged. The new warned counter reads it through Array.isArray(receipt.warnings) && receipt.warnings.length > 0, which is false for a missing key and also for a hand-edited ledger holding a non-array there. Covered by a test that strips the key from a ledger and resumes: the repository stays skipped, is not rescanned, and the campaign reports warned: 0.
  • A newer ledger read by older code is unaffected. warnings is an extra key on a line that older readReceipts parses with JSON.parse and casts; unknown keys are carried along and ignored. No existing field changed name, type, or emission condition.

Impact, stated plainly

A drifted repository is still recorded as completed, because it did complete — but its receipt now names the drift, and the campaign summary reports warned: 1. A scripted campaign can act on warned without parsing the ledger; a human can read the offending message off the receipt. bulk-scan's exit code, the completed/failed/skipped counts, the manifest, and the receipt fields that already existed are all unchanged. onProgress is deliberately left alone — this change adds a durable record, not a new stream of stderr chatter.

Verification

From sdk/typescript:

  • bun test --timeout 30000 ./tests-ts776 pass, 5 skip, 0 fail (781 across 34 files).
  • multiscan.test.ts alone — 17 pass, 0 fail.
  • With src/multiscan.ts reverted to its state on main and the new tests left in place — 14 pass, 3 fail: each of the three new tests fails without the fix, and every pre-existing multiscan test still passes.
  • tsc --noEmit clean; generate:models:check clean; prettier --check reports "All matched files use Prettier code style!".

Three tests added to sdk/typescript/tests-ts/multiscan.test.ts, in its existing mock-outcome style:

  1. records a completed attempt's warnings on its receipt and in the summary — two repositories, one warns. Asserts the warned receipt carries warnings, that the quiet receipt has no warnings property at all, that the summary reports warned: 1 with failed: 0, and that a synthetic sk-proj- credential inside the warning does not reach the ledger.
  2. records a failed attempt's warnings and counts its repository once — attempt 1 warns then throws, attempt 2 warns then completes. Asserts both receipts carry their own warning, that the failed one carries error alongside, and that warned counts the repository once rather than per attempt.
  3. resumes receipts written before the ledger carried warnings — resumes a ledger with the field, then rewrites it without the key and resumes again.

runMultiscan called security.run() without an onWarning observer, and the
per-attempt JSONL receipt recorded status, attempt, outputDir, cost and error
but nothing about warnings. A repository whose target drifted mid-run kept
coverage.completeness "complete", so the attempt genuinely succeeded and was
written as status "completed" with the warning discarded: no failure, no
non-zero exit, and no field a campaign consumer could read.

Each attempt now installs an onWarning observer that collects the run's
warnings, redacts them the way error is redacted because they are about to be
persisted, and writes them to the receipt as an optional warnings array. The
observer is used rather than the returned ScanResult because ScanResult does
not carry warnings, and because it is the only channel that also sees the
warnings run() emits from its finally block, which a failed attempt produces
too. The key is omitted when an attempt warned about nothing, matching error
and cost, so receipts for quiet attempts stay byte-identical to what earlier
releases wrote and the resume path never requires the field.

MultiscanResult gains a warned count beside failed: repositories that reported
at least one warning during this run, plus those resumed from a receipt that
carries warnings, the same way completed counts skipped repositories. Warnings
are not failures, so the bulk-scan exit code is unchanged.
@github-actions github-actions Bot added the bug Something isn't working label Aug 4, 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.

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

1 participant