Report a scan's CI blocking-rule verdict from corgea list - #155
Report a scan's CI blocking-rule verdict from corgea list#155Ibrahimrahhal wants to merge 2 commits into
corgea list#155Conversation
A pipeline that skips a duplicate scan for a commit it has already scanned has no verdict to gate on: --block-on evaluates the CI rules against the scan it is running, so the skip path was left re-deriving a pass/fail from the previous scan's vulnerability counts, which is not the question the rules answer. corgea list --block-on <slugs> asks that question about a scan that already ran, through the same endpoint corgea scan --block-on gates on. Every listed scan gains a blocking_verdict: the rules asked for, whether they blocked, how many issues did, and which rules tripped. A verdict is only as good as its status, so only 'complete' is final. A server still resolving license dependencies reports 'pending', and a scan that never completed reports 'unavailable' with a reason -- both with block null rather than a field a consumer would read as a pass. Scans that have not finished are never evaluated, since blocking rules see only the findings recorded so far. An evaluation error exits 1 for the same reason: an unknown, inactive, or PR-scoped slug must not read as a pass. --sha narrows the listing to one commit, which is how a duplicate-skip path reaches its scan in a single request. It takes the full SHA because the server matches exactly and a prefix would answer 'no scans', which that path reads as 'never scanned'; the returned scans are re-checked against it so a backend that ignores the filter cannot attribute another commit's verdict to this one. An empty page under --sha now reports an unscanned commit rather than the project miss a full listing would. A verdict costs one request per scan and the server re-evaluates every finding in that scan, so the pass covers at most ten scans and --block-on shrinks the default page to what it evaluates. Co-authored-by: ibrahim <ibrahim@corgea.com>
Both are new backward-compatible flags, so SemVer puts this at a minor bump. Cargo.toml is the single source of truth: PyPI reads it via maturin and npm takes its version from the release tag. Co-authored-by: ibrahim <ibrahim@corgea.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } else { | ||
| VERDICT_STATUS_PENDING | ||
| }, | ||
| "block": response.block, |
There was a problem hiding this comment.
pending currently serializes the backend's block value (the existing response contract uses false while dependency resolution is pending), even though this PR's README says pending and unavailable both leave block null. That defeats the advertised fail-closed shape for any consumer that reads block without checking status: an unfinished evaluation looks like a pass. Serialize a boolean only for complete responses and add an e2e assertion for a pending blocking-rules response.
| "block": response.block, | |
| "block": response.is_complete().then_some(response.block), |
| }, | ||
| "block": response.block, | ||
| "blocked_issues": response.blocked_count(), | ||
| "triggered_rules": triggered_slugs(&response.blocking_issues), |
There was a problem hiding this comment.
This reports only the rules found on page 1. The call above passes page: None (page 1), BlockingRuleResponse exposes total_pages, and triggered_slugs only examines this response's paginated blocking_issues. If (for example) page 1 contains only criticals violations and page 2 contains a malicious-deps violation, block and blocked_issues describe the whole result while triggered_rules incorrectly omits malicious-deps. Since the new JSON contract promises which rules triggered, fetch pages 2 through total_pages and union their slugs (or obtain an aggregate from the API), with a multi-page test.
| /// "never scanned". | ||
| pub fn normalize_sha(raw: &str) -> Result<String, String> { | ||
| let sha = raw.trim(); | ||
| if !(40..=64).contains(&sha.len()) || !sha.chars().all(|c| c.is_ascii_hexdigit()) { |
There was a problem hiding this comment.
This accepts every hex length from 40 through 64, but Git's full object IDs are exactly 40 characters (SHA-1) or 64 characters (SHA-256). A 41–63 character hex typo is therefore sent to the server's exact-match filter and returns an empty result—the same silent miss this validator is intended to prevent. Restrict the accepted lengths and cover an in-between length in the rejection test.
| if !(40..=64).contains(&sha.len()) || !sha.chars().all(|c| c.is_ascii_hexdigit()) { | |
| if !matches!(sha.len(), 40 | 64) || !sha.chars().all(|c| c.is_ascii_hexdigit()) { |
![corgea-staging[bot]](https://avatars.githubusercontent.com/u/69728621?s=60&v=4){kind=small}
| /// | ||
| /// Falls back to rule ids against backends that do not send slugs yet. | ||
| pub fn triggered_slug_summary(issues: &[utils::api::BlockingIssue]) -> String { | ||
| pub fn triggered_slugs(issues: &[utils::api::BlockingIssue]) -> Vec<String> { |
There was a problem hiding this comment.
🧹 Quality - The code uses "Vec::contains" inside nested loops to de-duplicate strings, causing O(n²) performance. This can slow down "triggered_slugs" when processing many issues or rules. View in Corgea ↗
More Details
🎟️Issue Explanation: The code uses "Vec::contains" inside nested loops to de-duplicate strings, causing O(n²) performance. This can slow down "triggered_slugs" when processing many issues or rules.
- The "triggered_slugs" function repeatedly calls "names.contains()" inside nested loops, leading to quadratic time complexity as "names" grows.
- Quadratic complexity makes "triggered_slugs" slower on large inputs, impacting scan speed when many blocking issues and rules exist.
- The inefficiency adds overhead when generating the list of unique triggered slugs, increasing runtime and reducing maintainability due to implicit intent.
🪄Fix Explanation: Replaces repeated linear searches in the result vector with a "HashSet" for efficient identifier deduplication. The change preserves insertion order while improving performance for large issue collections.
-"seen" provides average O(1) membership checks, replacing "names.contains", which scans the vector in O(n).
-"seen.insert(identifier.clone())" returns whether the identifier is new, simplifying the deduplication condition.
-Identifiers are still appended to "names" only once, preserving the existing output order and behavior.
-The separate set and ordered vector clearly express their distinct responsibilities: uniqueness tracking and result ordering.
diff --git a/src/scanners/blast.rs b/src/scanners/blast.rs
index 88ee833..2b7b2eb 100644
--- a/src/scanners/blast.rs
+++ b/src/scanners/blast.rs
@@ -648,13 +648,14 @@ pub fn normalize_block_on(block_on: Option<&str>) -> Result<Option<String>, Stri
/// Falls back to rule ids against backends that do not send slugs yet.
pub fn triggered_slugs(issues: &[utils::api::BlockingIssue]) -> Vec<String> {
let mut names: Vec<String> = Vec::new();
+ let mut seen = std::collections::HashSet::new();
for issue in issues {
let identifiers = match &issue.triggered_by_slugs {
Some(slugs) if !slugs.is_empty() => slugs.clone(),
_ => issue.triggered_by_rules.clone(),
};
for identifier in identifiers {
- if !names.contains(&identifier) {
+ if seen.insert(identifier.clone()) {
names.push(identifier);
}
}
To apply the fix, Download .patch.
2 participants
What this adds
A scan does not have one verdict — it has a verdict per rule set, so the answer has to be scoped to the slugs the caller gates on, exactly as
--block-onis.corgea list --block-on <slugs>asks that question about a scan that already ran, through the same endpointcorgea scan --block-onuses:corgea ls --sha "$(git rev-parse HEAD)" --block-on criticals,malicious-deps --jsonNo backend change.
GET /api/v1/scan/<id>/check_blocking_rules?block_on=<slugs>already accepts any scan id and any slugs, and/api/v1/scansalready accepts ashafilter — the CLI just stopped sending it when--skip-if-scannedwas cut from COR-1639.Not answering is a distinct answer
statusis what says whetherblockcan be trusted, and onlycompleteis final:statusblockcompletetrue/falsependingfalse, not yet finalunavailablereasonnullThe failure modes are deliberately loud, because every one of them is a chance to silently pass a pipeline that should have failed:
blockisnull, notfalse, whenever there is no verdict.corgea scan --block-on, the listing does not poll: it reportspendingfor the caller to retry rather than blocking for up to 15 minutes per scan.--shaNarrows the listing to one commit, so the duplicate-skip lookup is a single request instead of paging. It requires the full SHA — the server matches exactly, and a prefix would answer "no scans", which that path reads as "never scanned". Returned scans are re-checked against the SHA client-side so a backend that ignores the filter cannot attribute another commit's verdict to this one. An empty page under
--shanow reports an unscanned commit instead of the "No Corgea project found" miss a full listing reports.Cost
A verdict costs one request per scan, and the server re-evaluates every finding in that scan (up to 5000 issues plus a dependency walk for license rules). So the pass covers at most the first 10 scans of a page,
--block-onshrinks the default page to 10, and anything beyond the cap reportsunavailablewith areasonpointing at--sha/--page-size.Non-JSON output gets a Blocking column:
Testing
blocknull on unavailable, SHA normalization, the client-side SHA re-check, the Blocking column.--shasent and re-checked; a short SHA rejected before dialing the API; the flags rejected on issue listings; an evaluation error exits 1; an empty--shapage is an empty result rather than a project miss../harness cipasses in full locally (strict clippy, format, dep audit, 697 tests, coverage gate).Still open
The second half of his thread —
--out-format/--out-filebeing dropped when--block-ontrips, because report generation runs after the gate'sexit(1)— is a separate fix and is not in this PR.