Add --block-on to select which blocking rules gate a CI scan - #149
Conversation
--fail evaluated every active blocking rule, so a pipeline could not opt into a specific gate and any rule meant for CI also blocked pull requests. --block-on takes a comma-separated list of rule slugs and fails the scan only on those. It is blast-only and mutually exclusive with --fail and --fail-on. Unknown, inactive, and PR-scoped slugs are reported by name and exit 1 rather than passing silently, so a typo cannot quietly disable the gate. --fail still works but now warns that it is deprecated. Co-authored-by: Cursor <cursoragent@cursor.com>
Resolves src/list.rs: main moved the check_blocking_rules call onto an `id` binding that skips the enrichment for code-quality listings, while this branch added the block_on parameter. Kept both, passing None so `corgea ls` retains the legacy all-rules evaluation. Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| utils::terminal::TerminalColor::Green | ||
| ) | ||
| ); | ||
| std::process::exit(1); |
There was a problem hiding this comment.
[P2] Generate requested artifacts before exiting the gate
A blocking response exits here, but report generation starts at line 364 and SBOM generation at line 458. As a result, the newly documented command corgea scan --block-on no-criticals --out-format sarif --out-file results.sarif never writes results.sarif when the rule actually trips; --sbom is skipped too. Those failing CI runs are exactly where the diagnostic artifacts are needed, and --fail-on already avoids this by evaluating its exit after artifact generation. Store the blocking result, generate requested outputs, then exit 1 (while still failing immediately on API/configuration errors), and add a stubbed CLI test combining a blocking response with --out-file or --sbom.
There was a problem hiding this comment.
I agree with this finding and think it should be addressed.
high: Generate requested artifacts before exiting the gate
The blocking branch exits before report and SBOM generation later in the function. Consequently, the newly documented combination of --block-on with SARIF produces no SARIF precisely when the gate detects violations. Preserve the blocking result, generate requested artifacts, and exit afterward; API and configuration errors can still fail immediately.
Proof or reproduction:
`corgea scan --block-on no-criticals --out-format sarif --out-file results.sarif` calls `std::process::exit(1)` in this branch, so execution never reaches the later out_file handling.
| if status == reqwest::StatusCode::BAD_REQUEST { | ||
| if let Ok(block_on_error) = serde_json::from_str::<BlockOnError>(&response_text) { | ||
| return Err(block_on_error.describe().into()); | ||
| } | ||
| } |
There was a problem hiding this comment.
[P2] Restrict structured parsing to block-on errors
This function is also called with block_on == None by legacy --fail and corgea ls. Because every BlockOnError field has #[serde(default)] and Serde ignores unknown fields, any JSON-object 400 (for example {"detail":"invalid scan"}) deserializes successfully here and is rewritten as Invalid --block-on value. even when that flag was never used. That regresses existing error handling and can hide the real request failure. Only interpret this schema for a block_on request and only when at least one recognized field is present; otherwise preserve the ordinary status error.
| if status == reqwest::StatusCode::BAD_REQUEST { | |
| if let Ok(block_on_error) = serde_json::from_str::<BlockOnError>(&response_text) { | |
| return Err(block_on_error.describe().into()); | |
| } | |
| } | |
| if block_on.is_some() && status == reqwest::StatusCode::BAD_REQUEST { | |
| if let Ok(block_on_error) = serde_json::from_str::<BlockOnError>(&response_text) { | |
| if block_on_error.message.is_some() || !block_on_error.is_empty() { | |
| return Err(block_on_error.describe().into()); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
I agree with this finding and think it should be addressed.
nitpick: Restrict structured parsing to block-on errors
All BlockOnError fields have defaults and unknown JSON fields are ignored, so unrelated JSON-object 400 responses deserialize as an empty BlockOnError. This rewrites errors from legacy --fail and list requests as Invalid --block-on value. even when --block-on was absent. Parse this schema only when block_on is present and at least one recognized field was returned.
Proof or reproduction:
A 400 body `{"detail":"invalid scan"}` successfully deserializes into default-valued BlockOnError and `describe()` returns `Invalid --block-on value.`.
| let query_params = vec![("page", page.to_string())]; | ||
| let mut query_params = vec![("page", page.to_string())]; | ||
| if let Some(block_on) = block_on { | ||
| query_params.push(("block_on", block_on.to_string())); |
There was a problem hiding this comment.
[P2] Fail closed when the backend does not support block_on
The PR description confirms that an older backend ignores this query parameter, but the CLI has no capability check. That defeats the stated hard-error guarantee during a staggered rollout: --block-on typo-rule can receive a successful legacy all-rules response, and if no unrelated active rule trips, lines 358–361 print that no issues violated typo-rule and exit 0 instead of rejecting the unknown slug. Release ordering alone does not let the client distinguish support, especially for configured/custom API URLs. Have the new endpoint echo an explicit capability or the accepted slug set in every response and require it whenever block_on is supplied; if it is absent, exit with an unsupported-backend error. Add an HTTP-level test using a legacy 200 response to pin this fail-closed behavior.
There was a problem hiding this comment.
I agree with this finding and think it should be addressed.
high: Fail closed when the backend ignores block_on
A legacy backend can ignore the new query parameter and return 200 using all active rules. The CLI then treats that response as validation of the requested slugs, so an unknown slug may print a success message and exit 0. This defeats the gate's documented hard-error behavior. Require an explicit capability or accepted-slug field in responses to block_on requests and reject responses without it.
Proof or reproduction:
Given `--block-on typo-rule`, a legacy response such as `{"block":false,"blocking_issues":[],"total_pages":1}` reaches the success branch and exits 0 even though `typo-rule` was never validated.
There was a problem hiding this comment.
Automated review risk: 4/5.
The new CI gate can silently pass against an older backend and skips requested diagnostic artifacts when it blocks. Error handling and paginated reporting also need correction.
Critical or high-priority changes must be addressed.
Automatic approval was not submitted: automated review found critical or high-priority findings.
…lure The --block-on message printed blocking_issues.len() and the triggered rule slugs from page 1 only, but the endpoint pages at 20 issues. A blocked scan with more than 20 issues reported "20 issue(s)" whatever the real total, and a rule that only tripped on a later page was left out of the list of rules blamed for the failure. The exit code was right; the explanation was not. collect_blocking_issues walks pages 2..=total_pages and dedupes by issue id, following the loop corgea ls already uses against the same endpoint. A page that fails to load is warned about and skipped rather than aborting, so a transient pagination error cannot downgrade a blocked scan to a pass. Co-authored-by: Cursor <cursoragent@cursor.com>
"--block-on no-criticals" reads as "block when there are no criticals", the opposite of what it does. Name the example rules for the condition that trips the gate: --block-on criticals,malicious-deps. Applied to the --help text and the empty-slug error alongside the skill docs, since all three carried the same examples, and recorded the naming guidance next to the note that slugs derive from the rule name. Co-authored-by: Cursor <cursoragent@cursor.com>
--block-on is a new backward-compatible flag and --fail is deprecated rather than removed, 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, so only the manifest and lockfile change. Co-authored-by: Cursor <cursoragent@cursor.com>
…fetches The endpoint already returns the pre-pagination total in stats.blocked_issues, which the CLI simply was not deserializing. Reading it gives an exact count from the single request the gate already makes, so walking pages 2..=total_pages was both unnecessary and unsound: each request re-paginates a list rebuilt from querysets with no ORDER BY, so pages from separate requests need not line up. The CLI never uses the issue records themselves, only their count and the rule slugs, so the returned page remains sufficient to name the rules at fault. Co-authored-by: Cursor <cursoragent@cursor.com>
The stub stopped reading at the header terminator, then replied with Connection: close while the client was still writing its body. Small bodies survived on socket buffering alone, but the streamed multipart uploads (POST /start-scan and the chunk PATCH) did not, so the client got a request-body error instead of a response. That is what made scan_sbom_unwritable_path_errors_cleanly fail in CI while passing locally: it asserts on the SBOM write error, and the scan died at upload instead. Coverage instrumentation slows the client enough to widen the window. read_http_request now follows Content-Length, or reads to the zero-length chunk for chunked bodies, and carries a read timeout so a client that never finishes cannot park the single-threaded stub. Adds unit tests for all three cases, since the e2e symptom only reproduces under CI timing. Co-authored-by: Cursor <cursoragent@cursor.com>
3 participants
Description
--failevaluated every active blocking rule on the company, so a pipeline had no way to opt into a specific gate, and any rule authored for CI also gated pull requests through the native integration.--block-onlets a pipeline name the rules it wants enforced.Blocking rules now carry a company-scoped slug and an "Applies To" target of either Pull Requests or CI (see the companion doghouse PR).
--block-onpasses the slugs tocheck_blocking_rulesas ablock_onquery parameter and fails the scan only on those rules.Unusable slugs are a hard error, not a silent pass. This is the main design decision worth understanding. If a slug is unknown, inactive, or scoped to pull requests, the backend returns a structured 400 and the CLI exits 1 naming the offending slugs per category:
The alternative — skipping slugs we cannot resolve — would let a typo or a renamed rule quietly disable a security gate while the build stayed green. Failing loudly is the safer default for something whose whole job is to block. Note this makes renaming a rule in the web app a breaking change for any pipeline naming its old slug, since the slug is derived from the name; that trade-off is called out in the skill docs.
normalize_block_ontrims and de-duplicates the list and rejects empty entries locally, so a stray trailing comma is reported as a clear message rather than an opaque server error.--block-onis blast-only and mutually exclusive with--failand--fail-on.--failis deprecated but unchanged. It still evaluates every active rule and still exits 1 the same way; it now prints a deprecation warning pointing at--block-on. Existing pipelines keep working.check_blocking_rulesgained anOption<&str>parameter rather than a new function, and omitting it preserves the legacy all-rules behavior — that is what--failandcorgea lspass.BlockingIssue::triggered_by_slugsisOptionwith#[serde(default)], and the failure summary falls back to rule ids, so a newer CLI still works against a backend deployed before slugs exist.Related Issues
Requires the companion doghouse PR (slug +
applies_tomodel fields, migration0048, andblock_onresolution oncheck_blocking_rules). That must deploy before any release containing this change — against an older backend,block_onis ignored and the scan would be gated by every active rule instead of the named ones.Type of Change
Testing
Seven new unit tests in
src/scanners/blast.rscovering slug normalization (absent flag, trimming, de-duplication, and each empty-entry shape), the triggered-slug summary (de-duplication across issues and the rule-id fallback), andBlockOnError::describe(all three categories, plus the bare-server-message fallback).Note these live in a
#[cfg(test)]module in the binary target, socargo test --libdoes not pick them up; use--bins.Not yet exercised end-to-end against a running backend — the error paths are covered at the unit level from the serialized 400 body rather than over the wire.
Documentation
skills/corgea/SKILL.mddocuments--block-on, the CI-only restriction, the hard-error behavior, the rename-changes-the-slug caveat, and the--faildeprecation.Made with Cursor