ci: block only on confirmed scanner findings - #222
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe pull request adds finding-only Sonar and Snyk CI gates with shared outcome classification, bounded execution, timeout handling, sanitized reporting, workflow contracts, CodeRabbit settings, tests, and security documentation. ChangesScanner gate implementation
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest as Pull request
participant Workflow as GitHub Actions
participant SonarGate as runSonarCi
participant SnykGate as runSnykCi
participant ScannerPolicy as scanner-gate-policy
PullRequest->>Workflow: start security jobs
Workflow->>SonarGate: run security:sonar:ci
SonarGate->>ScannerPolicy: execute bounded Sonar command
ScannerPolicy-->>SonarGate: classify and sanitize result
Workflow->>SnykGate: run security:snyk:ci
SnykGate->>ScannerPolicy: execute bounded Snyk phases
ScannerPolicy-->>SnykGate: classify and sanitize result
SonarGate-->>Workflow: gate outcome
SnykGate-->>Workflow: gate outcome
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
docs/superpowers/specs/2026-07-31-finding-only-scanner-gates-design.md (1)
57-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the outcome-count mismatch: this doc calls it a "three-state policy" but describes four outcomes.
Line 57 states "an explicit three-state policy" and lists Clean, Finding, Unavailable. Lines 66-69 then describe configuration/contract failures as a distinct, separately blocking category — a fourth outcome, not a variant of the three listed.
This contradicts other files in the same PR:
- The implementation plan states: "four explicit states: clean, finding, unavailable, and configuration failure."
scripts/scanner-gate-policy.mjsdefinesSCANNER_OUTCOMEwith four frozen values.docs/SECURITY.mddocuments a four-row Clean/Finding/Unavailable/Configuration table.Update the wording here to say "four-state policy" (or explicitly number Configuration as state 4) so this spec matches the plan, the code, and
docs/SECURITY.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-07-31-finding-only-scanner-gates-design.md` around lines 57 - 69, Update the policy description in the scanner-gate outcome section to identify four states, explicitly including configuration or contract failure as the fourth state alongside Clean, Finding, and Unavailable; keep the existing outcome behavior and blocking semantics unchanged.scripts/sonar-reviewed-issues.test.mjs (1)
680-697: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace source-text substring matching with a behavioral assertion.
This test reads the raw text of
run-sonar-ci.mjsand locates literal substrings ("if ('branch' in scope)",'await reconcile({','await readIssues') to prove that reconciliation only runs for branch scope and precedes the open-finding gate. This is fragile: a harmless refactor (renamingscope, reformatting, changing quote style) breaks the test without any real regression, and a change that reorders behavior while keeping these exact substrings intact would not be caught.
scripts/run-sonar-ci.test.mjsalready proves this same guarantee behaviorally ("reconciles reviewed findings exactly once only for the test branch" via mockedreconcile/readIssues/checkGatecall-order assertions). ImportrunSonarCihere too and assert on mocked call order instead of indexing into source text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/sonar-reviewed-issues.test.mjs` around lines 680 - 697, Replace the source-text inspection in the Sonar CI runner test with a behavioral test using the exported runSonarCi function. Import runSonarCi, mock reconcile, readIssues, and checkGate, then assert reconciliation occurs exactly once only for the test-branch scope and in the required call order before the open-finding gate, matching the existing behavioral coverage in scripts/run-sonar-ci.test.mjs.scripts/security-workflow-contract.test.mjs (1)
15-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard
jobbefore readingjob.steps.If a job key is renamed or removed,
jobisundefinedand line 16 throwsTypeError: Cannot read properties of undefined. The test then reports a type error instead of the intended contract failure.♻️ Proposed guard
const findStep = (job, name) => { + assert.ok(job?.steps, `missing workflow job for step: ${name}`); const step = job.steps.find((candidate) => candidate.name === name); assert.ok(step, `missing workflow step: ${name}`); return step; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/security-workflow-contract.test.mjs` around lines 15 - 19, Update findStep to validate that job exists before accessing job.steps, using the existing assertion style to report the missing job as a contract failure; preserve the current step lookup and missing-step assertion once job has been validated.scripts/sonar-reviewed-issues.mjs (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving the shared timeout bounds and clock helper into
scanner-gate-policy.mjs.
DEFAULT_REQUEST_TIMEOUT_MS,MAX_REQUEST_TIMEOUT_MS,DEFAULT_TIMEOUT_MS,MAX_TIMEOUT_MS, andmonotonicNoware now duplicated across the Sonar and Snyk scripts. A single export keeps the bounds aligned when one value changes.Also applies to: 41-41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/sonar-reviewed-issues.mjs` around lines 11 - 14, Move the duplicated timeout constants and monotonicNow helper from the Sonar and Snyk scripts into scanner-gate-policy.mjs, export them there, and update both consumers to import and reuse those shared exports. Remove the local duplicate definitions while preserving existing timeout behavior.scripts/run-snyk-ci.mjs (1)
96-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving
phaseTimeoutintoscanner-gate-policy.mjs.
run-sonar-ci.mjsdefines the same helper at lines 96-100 with an extramaximumparameter. One shared implementation keeps the deadline behavior identical for both gates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run-snyk-ci.mjs` around lines 96 - 100, Move the shared phaseTimeout implementation from run-snyk-ci.mjs into scanner-gate-policy.mjs, extending it to accept the maximum timeout parameter used by run-sonar-ci.mjs. Update both run-snyk-ci.mjs and run-sonar-ci.mjs to import and reuse this shared helper, passing their respective command timeout limits while preserving the aggregate-deadline behavior.scripts/sonar-open-findings.test.mjs (1)
146-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer behavior assertions over source-text assertions for the ordering contract.
These tests read
run-sonar-ci.mjsas text and compareindexOfpositions. The assertions pass even if the phases never execute, and they break on any rename or message edit.runSonarCialready acceptsrunCommand,waitAnalysis,reconcile,readIssues, andcheckGateas injectable options. Inject fakes that push a label into an array, then assert the recorded order.♻️ Sketch of an order-recording test
test('the Sonar CI gate runs phases in order', async () => { const calls = []; const result = await runSonarCi({ argv: ['--branch=test'], env: { SONAR_TOKEN: TOKEN, SONAR_ORGANIZATION: 'org' }, runCommand: async () => (calls.push('upload'), { code: 0, output: '' }), waitAnalysis: async () => calls.push('wait'), reconcile: async () => calls.push('reconcile'), readIssues: async () => (calls.push('issues'), { summary: { open: [] } }), checkGate: async () => calls.push('gate'), }); assert.deepEqual(calls, ['upload', 'wait', 'reconcile', 'issues', 'gate']); assert.equal(result.outcome, SCANNER_OUTCOME.CLEAN); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/sonar-open-findings.test.mjs` around lines 146 - 165, Replace the source-text index checks in the Sonar CI ordering test with behavioral assertions using runSonarCi’s injectable runCommand, waitAnalysis, reconcile, readIssues, and checkGate options. Record each fake’s invocation in order, assert the expected phase sequence, and preserve validation of the scanner’s non-blocking quality-gate configuration plus the clean outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.coderabbit.yaml:
- Around line 2-7: Update the auto_review configuration in .coderabbit.yaml so
request_changes_workflow cannot remain blocked after fixes go unreviewed: raise
auto_pause_after_reviewed_commits above 2, or document the required manual
re-review step in docs/DEVELOPMENT.md. Preserve the existing automatic review
settings.
In `@scripts/run-snyk-ci.mjs`:
- Around line 22-31: Update the Snyk policy definitions around SCAN_POLICY and
MONITOR_POLICY so exit codes 2, 3, and missing-credential outcomes have explicit
documented policy handling consistent with the intended blocking behavior; do
not leave them to the generic UNAVAILABLE fallback. Ensure both policy outcomes
and their documentation clearly reflect how these cases are classified.
In `@scripts/scanner-gate-policy.mjs`:
- Around line 49-53: Preserve transient-pattern evidence independently of the
bounded output buffer: while processing command-output chunks, evaluate each
chunk against the configured transient pattern and retain a boolean streamed
match flag. Update classifyCommandResult to treat either the streamed flag or
policy.transientOutput matching the final bounded result as UNAVAILABLE. Add a
regression test in scanner-gate-policy.test.mjs covering transient text followed
by more than maxOutputBytes of filler and a non-zero, non-documented exit code.
In `@scripts/security-workflow-contract.test.mjs`:
- Line 24: Update the assertion for build.jobs['package-windows'].needs to use a
strict assertion that distinguishes undefined from null, ensuring the test
verifies the needs key is truly absent rather than merely unset.
---
Nitpick comments:
In `@docs/superpowers/specs/2026-07-31-finding-only-scanner-gates-design.md`:
- Around line 57-69: Update the policy description in the scanner-gate outcome
section to identify four states, explicitly including configuration or contract
failure as the fourth state alongside Clean, Finding, and Unavailable; keep the
existing outcome behavior and blocking semantics unchanged.
In `@scripts/run-snyk-ci.mjs`:
- Around line 96-100: Move the shared phaseTimeout implementation from
run-snyk-ci.mjs into scanner-gate-policy.mjs, extending it to accept the maximum
timeout parameter used by run-sonar-ci.mjs. Update both run-snyk-ci.mjs and
run-sonar-ci.mjs to import and reuse this shared helper, passing their
respective command timeout limits while preserving the aggregate-deadline
behavior.
In `@scripts/security-workflow-contract.test.mjs`:
- Around line 15-19: Update findStep to validate that job exists before
accessing job.steps, using the existing assertion style to report the missing
job as a contract failure; preserve the current step lookup and missing-step
assertion once job has been validated.
In `@scripts/sonar-open-findings.test.mjs`:
- Around line 146-165: Replace the source-text index checks in the Sonar CI
ordering test with behavioral assertions using runSonarCi’s injectable
runCommand, waitAnalysis, reconcile, readIssues, and checkGate options. Record
each fake’s invocation in order, assert the expected phase sequence, and
preserve validation of the scanner’s non-blocking quality-gate configuration
plus the clean outcome.
In `@scripts/sonar-reviewed-issues.mjs`:
- Around line 11-14: Move the duplicated timeout constants and monotonicNow
helper from the Sonar and Snyk scripts into scanner-gate-policy.mjs, export them
there, and update both consumers to import and reuse those shared exports.
Remove the local duplicate definitions while preserving existing timeout
behavior.
In `@scripts/sonar-reviewed-issues.test.mjs`:
- Around line 680-697: Replace the source-text inspection in the Sonar CI runner
test with a behavioral test using the exported runSonarCi function. Import
runSonarCi, mock reconcile, readIssues, and checkGate, then assert
reconciliation occurs exactly once only for the test-branch scope and in the
required call order before the open-finding gate, matching the existing
behavioral coverage in scripts/run-sonar-ci.test.mjs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 03498de7-1bf0-4c80-8cd5-3c2c194fcdab
📒 Files selected for processing (21)
.coderabbit.yaml.github/workflows/security.ymldocs/DEVELOPMENT.mddocs/SECURITY.mddocs/superpowers/plans/2026-07-31-finding-only-scanner-gates.mddocs/superpowers/specs/2026-07-31-finding-only-scanner-gates-design.mdpackage.jsonscripts/coderabbit-config-contract.test.mjsscripts/run-snyk-ci.mjsscripts/run-snyk-ci.test.mjsscripts/run-sonar-ci.mjsscripts/run-sonar-ci.test.mjsscripts/scanner-gate-policy.mjsscripts/scanner-gate-policy.test.mjsscripts/security-workflow-contract.test.mjsscripts/sonar-open-findings.mjsscripts/sonar-open-findings.test.mjsscripts/sonar-quality-gate.mjsscripts/sonar-quality-gate.test.mjsscripts/sonar-reviewed-issues.mjsscripts/sonar-reviewed-issues.test.mjs
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
1 participant
What changed
Why
Fast pushes should not lose the newest Windows build merely because a free external service is rate-limited or temporarily unavailable. A green unavailable result explicitly says that no security decision was produced; release revisions still require real clean scanner decisions.
Validation
npm run typechecknpm run lintnpm run format:checknpm test(5,624 passed; one existing skip)npm run buildnpm audit --audit-level=high --omit=dev(0 vulnerabilities)Summary by CodeRabbit
Security & CI
Documentation
Quality