Add --skip-if-commit-scanned-recently to reuse a recent scan of the commit - #157
Conversation
There was a problem hiding this comment.
Automated review risk: 4/5.
High risk: reuse can substitute scans that do not represent the current worktree, requested scope, scanner configuration, or policy, allowing security gates to pass on incomplete results.
Critical or high-priority changes must be addressed.
Automatic approval was not submitted: automated review found critical or high-priority findings.
d403c24 to
2d8162f
Compare
yhoztak
left a comment
There was a problem hiding this comment.
LGTM
once some comments are addressed
…ommit A pipeline that re-runs on an unchanged commit currently pays for a full scan that can only reproduce the previous run's findings. Teams work around it by querying `corgea list --json` for the last scan's SHA and re-deriving a verdict from issue counts, which has no idea which blocking rules apply or which findings carry exceptions. Skipping the scan is only half the job: the run still has to gate on --block-on and still has to write --out-file. So the reused scan takes the new scan's place for the rest of the command rather than short-circuiting it, and the blocking rules are evaluated against it exactly as they would be against a fresh scan. Recency is a policy, not a technicality: unchanged code is still exposed to advisories published since it was last scanned, so --scanned-within (24h by default) bounds how old a reusable scan may be. Reuse is refused whenever the evidence is incomplete — a failed or running scan, a scan of a dirty worktree, an unreadable timestamp, a lookup the platform could not answer — since scanning again is the safe outcome. The one hard failure is an unresolvable commit, because the flag asks a question about the commit and quietly scanning would hide that the pipeline is not getting the behavior it asked for. The scan list is re-checked client-side against the requested SHA: a backend predating the `sha` filter answers with every scan of the project, and acting on that would skip one commit's scan because a different commit was scanned. CORGEA_SCAN_SKIPPED=true/false (plus CORGEA_SCAN_ID on a reuse) is printed once per run so a later pipeline step can branch on whether a scan happened. Co-authored-by: ibrahim <ibrahim@corgea.com>
Review found four ways a reused scan could stand in for a scan it does not
represent, each letting --block-on pass on results the run never produced.
Doghouse already settled what scan identity means for its own server-side
dedupe (ScanManager._find_reusable_scan): same commit, not a pull request, an
explicitly clean worktree, matching scan configuration and policies. These
are the client-side half of that rule.
- The dirtiness check read status_dirty, the signal that drives the user
notice, not the dirty signal an upload actually sends. status_dirty cannot
see assume-unchanged/skip-worktree files, dirty submodules, or an index it
failed to read, so a run whose upload would be marked dirty could reuse a
clean scan of the commit and gate on files it does not contain.
- A candidate's worktree_dirty had to be other than Some(true), which read
"not reported" as "clean". It is not: platform and scheduled scans do
record false, so what None mostly identifies is the partial --target
upload of an older CLI, indistinguishable from a whole-commit scan from
here. Doghouse draws the same line ("only explicit clean may dedupe").
- Pull-request scans and other engines were eligible. A PR scan answers a
question about a proposed merge and may be scoped to the diff; an uploaded
third-party report covers whatever that scanner found.
- --scan-type/--policy were silently dropped on a reuse hit, and a candidate
could itself have been secrets-only. No read endpoint exposes a scan's
scan configs or target policies, so the match cannot be checked: the flag
now conflicts with both rather than reusing a scan that may have covered
less. Restricting candidates to default-config scans needs the API to
expose that identity first.
The scan list carries no scan_errors, so a scan that finished with a
scanner's results missing is indistinguishable there from a clean one. One
confirmation read of GET /scan/{id} closes that: a degraded scan is not
reused at all, since the alternative is simply to scan, which may also clear
a transient failure. A candidate rejected there sends the run to a real scan
rather than to the next-oldest, because a commit whose recent scans are all
degraded wants a fresh scan anyway.
The lookup now paginates. The window ends the search rather than a page
boundary: the list is newest first, so a page whose oldest scan is already
outside the window has nothing after it, and the page ceiling only bounds one
commit with more in-window scans than fit on a page.
Co-authored-by: ibrahim <ibrahim@corgea.com>
2d8162f to
8af5e8a
Compare
| if classify_scan_status(&scan.status) != ScanState::Completed { | ||
| return Err(format!("its status is '{}'", scan.status)); | ||
| } | ||
| if !scan.engine.eq_ignore_ascii_case(BLAST_ENGINE) { |
There was a problem hiding this comment.
The candidate-side configuration mismatch is still open. This check only proves the candidate used the BLAST engine; the remaining checks cover PR scope, dirtiness, and age, while ScanResponse has no scan_configs or target_policies. A clean corgea scan --scan-type secrets upload still sends scan_configs separately in upload_zip and retains engine = corgea-blast plus worktree_dirty = false, so a later default run can select it here. The new Clap conflicts only prevent the current invocation from being custom; they do not identify how the candidate was produced. --block-on can therefore pass on a secrets-only result that never ran the default base/policy scanners.
Do not reuse until candidate identity can be proved: expose normalized scan_configs, target_policies, and image/scope identity in the list/detail API and require an exact default-run match, or move candidate selection to the backend's existing dedupe predicate and return only a scan that predicate certifies. Add an E2E case where the list returns a same-SHA, clean, corgea-blast candidate created with a narrowed config and assert that a new scan starts.
| if let Err(reason) = confirm_reusable_scan(config, &scan.id) { | ||
| log::warn!( | ||
| "Not reusing scan {}: {}. Running a new scan.", | ||
| scan.id, | ||
| reason | ||
| ); | ||
| print_skipped_marker(None); | ||
| return None; | ||
| } |
There was a problem hiding this comment.
The worktree is sampled only once at lines 118–121, before the scan-list and detail network requests. After those requests this block accepts the candidate without re-reading HEAD or dirtiness. If another process checks out a commit or modifies a tracked file while either request is in flight, this run can reuse the old clean scan even though the tree it is gating no longer matches that SHA. The fresh-scan path already avoids the analogous race by reconciling samples from before and after packaging.
Re-sample after confirmation and require the same clean SHA; otherwise fall through to a real scan. A delayed stub response plus a concurrent file edit/checkout should cover the race.
| if let Err(reason) = confirm_reusable_scan(config, &scan.id) { | |
| log::warn!( | |
| "Not reusing scan {}: {}. Running a new scan.", | |
| scan.id, | |
| reason | |
| ); | |
| print_skipped_marker(None); | |
| return None; | |
| } | |
| if let Err(reason) = confirm_reusable_scan(config, &scan.id) { | |
| log::warn!( | |
| "Not reusing scan {}: {}. Running a new scan.", | |
| scan.id, | |
| reason | |
| ); | |
| print_skipped_marker(None); | |
| return None; | |
| } | |
| let same_clean_commit = utils::generic::get_repo_info_for_scan("./") | |
| .ok() | |
| .flatten() | |
| .is_some_and(|info| info.sha.as_deref() == Some(sha.as_str()) && !info.dirty); | |
| if !same_clean_commit { | |
| log::warn!( | |
| "HEAD or the working tree changed while checking scan {}; running a new scan.", | |
| scan.id | |
| ); | |
| print_skipped_marker(None); | |
| return None; | |
| } |
| ("page", page.to_string()), | ||
| ("page_size", page_size.to_string()), | ||
| ("project", project.to_string()), | ||
| ("sha", sha.to_string()), |
There was a problem hiding this comment.
This lookup still does not constrain file scope. The documented GET /scans API supports scan_type=full|partial, but this request sends only project and SHA, and the response model exposes no scope field for a client-side check. Requiring worktree_dirty == false excludes current CLI target uploads, but it does not exclude platform/integration partial scans produced from a clean worktree with worktree_dirty: false. Such a scan can replace this default whole-project run, so its report and --block-on gate omit files that were never scanned.
Request only full scans and assert the parameter in commit_lookup; also add an E2E response representing a clean partial platform scan. This is fail-safe on older servers that ignore the parameter because the existing client-side checks remain in place.
| ("sha", sha.to_string()), | |
| ("sha", sha.to_string()), | |
| ("scan_type", "full".to_string()), |
--exclude is normally a fixed line in a pipeline template rather than a per-run selection, so refusing the flag outright cost that pipeline shape the feature. Reusing a wider scan can also only over-report: the whole-commit scan is a superset of what an excluded scan would have found, so a gate can fail on an excluded file but can never miss a finding. It still cannot be matched, though. An --exclude upload is recorded as not an exact commit snapshot, so those scans are never reuse candidates themselves, and what gets reused is always a scan of the whole commit. That is worth saying at the point of reuse rather than leaving the extra findings to be discovered when the gate trips. --target keeps the hard conflict: it selects a file set per run, so a reused whole-commit scan is a different question rather than a wider answer to the same one. Co-authored-by: ibrahim <ibrahim@corgea.com>
What
Two new flags on
corgea scan:--skip-if-commit-scanned-recently— do not start a new scan when the project already has a reusable scan of the current commit inside the window.--scanned-within <DURATION>— what "recently" means (90s,30m,24h,7d; a bare number is hours). Defaults to 24h, because unchanged code is still exposed to advisories published since it was last scanned.Why
This replaces the duplicate-scan skip logic a customer hand-rolled in their Harness pipeline:
corgea list --json, comparegit_shaagainstCI_COMMIT_SHA, apply a 24h staleness window, paginatelist --issuesfor prior counts, then re-derive a pass/fail from those counts. That last step is the broken part — it has no idea which blocking rules apply to CI or which findings had valid exceptions, and with--block-ondriving enforcement there was no way to replay the prior scan's verdict.How it behaves
Skipping the scan is only half the job, so the reused scan takes the new scan's place for the rest of the command rather than short-circuiting it:
--block-ongate and its exit code,--out-file/--out-format,--fail-on, and--sbomall run against the reused scanTwo lines are printed once per run so a later pipeline step (e.g. an ingest) can branch on the outcome:
…or
CORGEA_SCAN_SKIPPED=falsewhen a scan actually ran.What counts as reusable
A scan may only stand in for another when it answers the same question, which is stricter than "same commit". Doghouse already settled what that means for its own server-side dedupe (
ScanManager._find_reusable_scan): same commit, not a pull-request scan, an explicitly clean worktree, matching scan configuration and policies. These checks are the client-side half of that rule. A candidate must be:corgea-blastengine — an uploaded third-party report covers whatever that scanner foundworktree_dirty == false).nullmeans the client never reported, which is not the same as clean: platform and scheduled scans do recordfalse, so whatnullmostly identifies is the partial--targetupload of an older CLIGET /scan/{id}, since the scan list carries noscan_errorsand a degraded scan is indistinguishable there from a clean oneAnything else runs a real scan, including a worktree that does not match the commit (the check reads the same
dirtysignal the upload sends, so assume-unchanged/skip-worktree files and dirty submodules count even thoughgit statushides them) and a lookup the platform could not answer.Two hard errors instead: an unresolvable commit exits 1 rather than silently scanning, and a run that changes what gets scanned is rejected at parse time — the API exposes neither a scan's scan configs and target policies nor whether it bundled an image, so
--scan-type,--policy,--include-image,--only-uncommittedand--targetconflict with the flag.--excludeis the exception: it is normally a fixed line in a pipeline template rather than a per-run selection, and reusing a wider scan can only over-report, never miss a finding. It cannot be matched either — an--excludeupload is recorded as not an exact commit snapshot, so those scans are never candidates themselves and what gets reused is always a whole-commit scan — so the run continues and warns that the results and gate can cover files it would have skipped.Implementation notes
GET /api/v1/scans?project=…&sha=…, which doghouse already filters server-side. No backend change is needed.git_shais re-checked client-side, and a page with no scan of this commit ends the walk.ScanResponsegainsworktree_dirtyandpull_request_id, both already returned by the list endpoint.blast::run(results, report, SBOM, gates) is now shared by both paths; packaging/upload/wait moved intostart_new_scan. The moved block is the one Write the scan report and SBOM before the blocking-rule gates exit #156 and Addcorgea scan --include-imageto scan fully built container images #153 left behind, ordering included — no behavior change on the normal path.Known gap, needs an API follow-up
Reuse cannot yet verify that a candidate ran the same scan configuration — a recent secrets-only scan of the commit is accepted by a default run, because no read endpoint exposes a scan's
configs/target_policies. This PR closes the direction it can (a narrowed run is refused outright, and the engine must match); restricting candidates to default-config scans needs the scan list to expose that identity, or a server-side filter. Doghouse's own dedupe already computes exactly this comparison, so the data exists. The same addition is what would let--targetand--excludebe matched properly rather than refused or warned about.Testing
./harness ci— strict clippy, format, dep audit, 738 tests, coverage gate.src/skip_scan.rscover window parsing (including values that would silently disable the check), newest-first selection and fall-through past a failed scan, the wrong-commit guard, every rejection above, the pagination stop rules, every timestamp shape the API emits, window edges, and clock skew.tests/cloud_commands_e2e/scan_skip.rsassert the exact request sequence, so "no new scan was started" is proven by the absence of the upload calls: a skipped scan still exits 1 on the prior scan's blocking rules and still writes its SARIF report first; a stale scan, a shorter--scanned-within, a degraded prior scan, a dirty worktree, and a file hidden fromgit statusall fall back to a real scan;--excludereuses but warns; an unresolvable commit fails before any upload; and every narrowing flag plus a lone--scanned-withinis rejected.