fix(multiscan): record run warnings in the campaign ledger - #295
Open
ppcvote wants to merge 1 commit into
Open
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #248.
runMultiscancalledsecurity.run()with noonWarningobserver, so every warning the scan raised was discarded. A repository whose target drifted mid-run still completes, so it landed inresults.jsonlasstatus: "completed"with nothing in the campaign output recording that the results describe a tree that moved.The receipt now carries a
warningsarray when the scan raised any:{"id":"drifted","status":"completed","attempt":1,"outputDir":"...", "warnings":[{"message":"Scan target changed during the run.","kind":"target_changed"}]}kindcomes fromScanWarningDetails, so a consumer can single out drift without matching on message text — the same distinction the CLI already makes at its ownonWarning.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 usessanitizeDiagnosticValuefor this, but that is private tocli.ts;redactedErrorMessageis the equivalent already imported intomultiscan.ts. Happy to export the CLI one instead if you would rather have a single sanitizer.Per attempt. The array is declared beside
failureandcostinside the retry loop, so a second attempt does not inherit the first attempt's warnings.statusis untouched. The scan completed; promoting a warning to a failure would be a louder change than the issue asks for and would break resume, sincecompletedreceipts 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:warningswhen none raisedThe 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 unmodifiedmain, 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
--jsonand SARIF surfaces; this one is the ledger. I have not touched either of those paths — #251 already has someone working on it.