fix: contain scan matching failures to a single batch - #120
Conversation
`scans match --all` aborted the whole campaign when one batch produced conflicting confirmed and uncertain findings. The throw fires while building `comparisons` for a batch, which is after every earlier batch has already called `save-scan-comparison`, so the run exited nonzero with partially persisted match state and no way to resume. Contain each batch instead. A batch that cannot be matched is counted, warned about, and skipped; the remaining batches still save. When nothing matched at all the first failure is rethrown, so an unwritable workbench or a rejected credential still surfaces as an error rather than a warning attached to a successful no-op. The conflict check itself is unchanged: refusing to persist conflicting confirmed and uncertain findings is deliberate, and its existing test passes unmodified.
8f81631 to
edb564d
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87781f3bdf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| unmatchedBatches += 1; | ||
| firstFailure ??= error; | ||
| onWarning?.( | ||
| `Could not match findings against scan ${afterScanId}: ${cliErrorMessage(error)}`, |
There was a problem hiding this comment.
At the exact merged head, cliErrorMessage no longer exists because current main replaced it with the imported redactedErrorMessage. Whenever any batch fails, evaluating this warning therefore raises a ReferenceError, escapes the catch block, and prevents later healthy batches from being processed—the behavior this change is intended to preserve. Use redactedErrorMessage(error) here.
Useful? React with 👍 / 👎.
| (count, { beforeOccurrenceIds, afterOccurrenceIds }) => | ||
| count + beforeOccurrenceIds.length * afterOccurrenceIds.length, | ||
| 0, | ||
| } catch (error) { |
There was a problem hiding this comment.
This catch also contains failures from save-scan-comparison, not just matching or validation failures. If one earlier pair was saved and a later database write fails (for example because the workbench becomes unwritable), matchedPairs remains nonzero, line 2167 does not rethrow, and the command exits successfully despite a systemic persistence failure and incomplete comparisons. Restrict containment to matcher/batch-validation errors, while allowing workbench save errors to fail the command.
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
2 participants
Summary
codex-security scans match --allaborts the entire campaign when one batch produces conflicting confirmed and uncertain findings, and it does so after earlier batches have already been written.matchAllScansthrows atsrc/cli.ts:2006while buildingcomparisonsinsidebeforeScans.map(...). That is before the failing batch's own save loop, but after every earlier iteration of the outerfor (const ... of batches)loop already calledsave-scan-comparison. The result is a nonzero exit with partially persisted match state and no resume path — rerunning re-derives the unsaved pairs but the operator has no way to tell how far the previous run got.What this changes
Each batch is now contained. A batch that cannot be matched is counted, warned about on stderr, and skipped; the remaining batches still save.
If nothing matched, the first failure is rethrown. This keeps a systemic problem — unwritable workbench, rejected credential — from being downgraded to a warning on an apparently successful no-op, and it preserves today's behavior exactly in the single-batch case.
The summary gains
unmatchedBatcheswhen nonzero, rendered in the TTY match-all view asN scans could not be matched; rerun to retryalongside the existingscans unavailableline.What this deliberately does not change
The conflict check itself. Refusing to persist conflicting confirmed and uncertain findings is intentional and test-covered by
tests-ts/cli-workbench.test.ts→ "does not save conflicting confirmed and uncertain matches". That test passes unmodified — that was the acceptance signal for this change, and I'd treat any need to edit it as a sign the fix had overreached.Open question for maintainers (not addressed here)
There is a real disagreement between two layers that this PR leaves alone:
src/scan-comparison.ts:246-257withallowHistoricalUncertainty: truedeliberately permits anuncertainpair whoseafterOccurrenceIdis already claimed by a confirmed match — it only ever rejects the before-side collision viamatchedBefore. The flag exists so an after-occurrence can be confirmed against one earlier scan and uncertain against another. ButvalidateComparisonhas no notion of scan boundaries, so it also admits the collision within a single before-scan, whichmatchAllScansthen treats as unusable.That shape is not self-contradictory — "after-finding A definitely corresponds to X, and may also correspond to Y" is a coherent claim about two earlier findings merged into one — so a well-behaved model can produce it and get a skipped batch. Either
scan-comparisonshould reject it ormatchAllScansshould drop the pair. I did not pick, because inverting a documented product invariant isn't mine to decide. Happy to follow up either way.Testing / QA instructions
Baseline before this branch: 470 pass / 6 skip / 0 fail. After: 472 pass / 6 skip / 0 fail (two added tests).
Full gate, from
sdk/typescript. Run pnpm from inside this directory, not the repo root — from the root,pnpm --dirresolves your global pnpm instead of the pinnedpackageManagerand the next in-directory command tries to purgenode_modules:The assertion that matters most — pre-existing behavior is intact
Expect
1 pass / 0 fail. This file is unmodified apart from added tests and one added import; if this fails, the containment scope is wrong.New coverage
keeps matching later scans after one batch conflicts— two batches, the first conflicting. Asserts exactly onesave-scan-comparisoncall, that it is for the healthy batch (before-ok), that stderr names the skipped scan (after-conflict), and that the JSON summary reportsmatchedPairs: 1, unmatchedBatches: 1. Onmainthis exits 2 and saves nothing.reports the underlying failure when no batch could be matched— everysave-scan-comparisonthrows a workbench-database error. Asserts exit 2 and that the underlying message reaches stderr rather than being swallowed into a zero-exit summary.Manual check of the rendered line
The
unmatchedBatchesline only renders on a TTY with--format toon(default). To see it without a real workbench, call the renderer directly:Expect a
1 scans could not be matched; rerun to retryline. Confirm--format jsonoutput is unaffected (the field is simply absent when zero).Regression surface to watch
--format jsonconsumers now see an extra optional key. It is omitted entirely when zero, so existingtoEqualassertions on clean runs are unaffected (verified: the pre-existingmatches every unmatched pairtest asserts the full object withtoEqualand still passes).