Skip to content

fix(review): exit code reflects filtered findings (#312)#318

Merged
ajianaz merged 1 commit into
developfrom
fix/312-review-exit-code
Jun 17, 2026
Merged

fix(review): exit code reflects filtered findings (#312)#318
ajianaz merged 1 commit into
developfrom
fix/312-review-exit-code

Conversation

@ajianaz

@ajianaz ajianaz commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #312cora review exits code 2 despite valid SARIF and zero blocking findings on larger diffs.

Root cause

should_block was computed by the engine using config.hook.min_severity. The exit-code logic then used the same should_block flag without re-checking against the filtered issue list (after CLI --severity filtering).

So when a Major finding existed but the user passed --severity critical, the filtered output shown to the user had 0 findings, yet cora review still exited 2 (EXIT_BLOCKED) because should_block was still true. This broke required-status-check CI configs.

Changes

src/commands/review.rs

  • Recompute should_block on filtered_response after severity filtering, in both the single-chunk path and the --auto-chunk path. Exit code now matches what the user sees.
  • Extracted exit-code decision into a pure helper compute_exit_code(gate_status, ci, filtered_response, hook_mode) — no hidden state, easy to test.
  • Added 8 unit tests covering: gate pass/fail override, CI mode with 0 vs ≥1 findings, hook block vs non-block, and the core regression (bug: cora review exits code 2 despite valid SARIF and zero blocking findings (larger diffs) #312) where filtered issues are empty.

Tests

  • 8 new unit tests in src/commands/review.rs.
  • cargo test commands::review — 8/8 pass.
  • cargo clippy -- -D warnings clean, cargo fmt applied.

Versioning

Patch bump 0.6.00.6.1 (shares the release with #316 and #313).

⚠️ Merge note: CHANGELOG and Cargo.toml will conflict with #317 (scan fix) and the upcoming #313 PR. Suggested merge order: #317 → this PR → #313. The conflicts are confined to the ## [0.6.1] block of CHANGELOG.md and the version line in Cargo.toml/Cargo.lock.

Checklist

  • cargo test passes
  • cargo clippy -- -D warnings clean
  • cargo fmt applied
  • CHANGELOG entry under ## [0.6.1]### Fixed — Review
  • Version bumped in Cargo.toml + Cargo.lock

Previously `cora review` could exit 2 (EXIT_BLOCKED) even when the
`--severity` filter removed all blocking findings and the SARIF output
contained 0 error-level results. This blocked PR merges in CI when Cora
Review was configured as a required status check.

Root cause: `should_block` was computed by the engine using
`config.hook.min_severity`, but the exit-code logic used the same
`should_block` flag without re-checking against the **filtered** issue
list (after CLI `--severity` filtering). So a Major finding with
`--severity critical` would still cause exit 2 even though the filtered
output shown to the user had no findings.

Fix:
- Recompute `should_block` on `filtered_response` after severity
  filtering, so exit code matches what the user sees (both in the
  single-chunk and `--auto-chunk` paths).
- Extract exit-code logic into `compute_exit_code()` pure helper with
  8 unit tests (gate pass/fail, CI mode, hook block vs non-block).

Bump 0.6.0 -> 0.6.1 (patch).
@ajianaz ajianaz force-pushed the fix/312-review-exit-code branch from ec5855b to 0939d90 Compare June 17, 2026 06:14
@ajianaz ajianaz merged commit 22374c4 into develop Jun 17, 2026
10 checks passed
@ajianaz ajianaz deleted the fix/312-review-exit-code branch June 17, 2026 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: cora review exits code 2 despite valid SARIF and zero blocking findings (larger diffs)

1 participant