feat: implement issue #355 — Compliance: secret_scanning_ai_detection#435
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
More reviews will be available in 10 minutes and 7 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTwo new warning-severity entries ( ChangesPush-protection security_and_analysis enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates scripts/lib/push-protection.sh to add detailed explanations for required security settings and introduces a verification step to confirm that settings are successfully applied after a PATCH request. The review feedback recommends optimizing this verification step by parsing the JSON response once into an associative array, which avoids spawning multiple jq processes in a loop and improves execution speed.
There was a problem hiding this comment.
Pull request overview
This PR updates the push-protection compliance library to add additional context around certain security_and_analysis settings (notably secret_scanning_ai_detection and secret_scanning_non_provider_patterns) and to verify whether a successful PATCH actually resulted in the desired state.
Changes:
- Expanded the audit/apply “human detail” messages for AI detection and non-provider patterns to include plan/feature-availability caveats.
- Added a post-
PATCHre-fetch step inpp_apply_security_and_analysis()to warn when settings remain non-compliant after the API call is accepted.
Dev-Lead — review-changes (applied)Changes committed and pushed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — review-changes (applied)Changes committed and pushed. |
|
…al repos gh release download uses GITHUB_TOKEN which is repo-scoped and returns 401 Unauthorized when downloading releases from external repos (gitleaks/gitleaks). Replace with a direct curl call which works for public release assets without authentication. Update standards/push-protection.md template to match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — fix-bot-comment (applied)Changes committed and pushed. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: ff531750afa8eee8c48cb4b3bf3f813544359ada
Review mode: triage-approved (single reviewer)
Summary
Fixes a non-actionable compliance finding (#355) by teaching pp_check_security_and_analysis() to skip warning-severity, plan-gated keys (secret_scanning_ai_detection, secret_scanning_non_provider_patterns, dependabot_security_updates) when their API status is null — i.e. the feature is unavailable for the org plan and cannot be remediated. Also adds post-PATCH verification of applied settings and a new bats test suite. +284/-8 across 3 files (1 lib, 2 test).
Linked issue analysis
Closes #355, which was an auto-generated audit finding flagging secret_scanning_ai_detection as null vs expected enabled. The root cause is that the feature is plan-gated (requires Copilot/GHAS), so a null status is not actionable. The PR addresses this precisely: null status for an explicitly allowlisted plan-gated key no longer produces a finding, while a disabled status (feature available but off) still does. Substantively resolved.
Findings
No blocking issues. Prior bot review findings were all raised and resolved:
- (gemini) per-iteration jq forking in the verify loop — optimized.
- (copilot) issue intent: distinguish null (plan-gated) from disabled — implemented via early
continue. - (copilot/coderabbit) post-PATCH loop used
warn, which is outside the pp_apply_* caller contract and also fired for error-severity keys — switched toinfo. - (coderabbit) null-suppression too broad — narrowed via explicit PP_PLAN_GATED_KEYS allowlist so error-severity keys (secret_scanning, push_protection) still report on null.
The suppression is correctly scoped to warning-severity plan-gated keys with null status only; error-severity settings are unaffected. Test coverage exercises null-suppressed, disabled-still-flagged, and error-severity paths.
CI status
All required checks green: ShellCheck, Lint, CodeQL, SonarCloud, Secret scan (gitleaks), Agent Security Scan, AgentShield, CodeRabbit all SUCCESS. Language-specific audits (npm/pip/cargo/go/pnpm) appropriately SKIPPED. All 5 review threads resolved; CodeRabbit's latest review is APPROVED (2026-06-15). mergeStateStatus is BLOCKED only pending this required review.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
|
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 5882f65636816073c578d3b98da804f9df7a240d
Review mode: triage-approved (single reviewer)
Summary
Refines the push-protection compliance audit so that plan-gated, warning-severity security settings (secret_scanning_ai_detection, secret_scanning_non_provider_patterns, dependabot_security_updates) no longer emit non-actionable findings when the GitHub API reports their status as null/absent (i.e. the org plan does not offer the feature). Also adds post-PATCH verification in pp_apply_security_and_analysis that re-fetches each setting and reports any key the API silently ignored, plus a new bats test suite. 284/-8 across 3 files.
Linked issue analysis
Closes #355, which is the auto-filed compliance finding 'secret_scanning_ai_detection should be enabled (current: null, expected: enabled)'. The root cause was that an absent (null) status for a plan-gated feature was treated as non-compliance even though it cannot be remediated without a plan upgrade. The PR suppresses exactly this case (plan-gated key AND actual == null), directly resolving the issue. Tests confirm the finding is still emitted when the same feature is present but disabled, and error-severity settings (secret_scanning, push_protection) are never suppressed.
Findings
No blocking issues.
- Suppression is narrowly scoped: it only skips when the key is in PP_PLAN_GATED_KEYS and actual ==
null. Adisabledstatus still produces a finding, and error-severity keys are never plan-gated — verified by the new bats tests. - The post-PATCH verification loop is a net security-posture improvement: it surfaces settings the API accepts (HTTP 200) but silently ignores due to plan limits, rather than reporting blanket success.
- Nit (non-blocking): the warning-detail strings now embed plan-requirement context, which is helpful; no action needed.
CI status
Effectively green. All substantive checks pass: CodeQL, ShellCheck, Lint, SonarCloud, Agent Security Scan, Secret scan (gitleaks), AgentShield, pr-auto-review all SUCCESS. CANCELLED entries (Lint, ShellCheck, Agent Security Scan, gitleaks, dev-lead dispatch/ci-relay) are concurrency-cancelled duplicates — each non-dev-lead one has a SUCCESS counterpart, and dev-lead's are the known concurrency-cancel case (per #608/#609). Language audits (npm/pnpm/pip/cargo/go) SKIPPED as not applicable. reviewDecision=APPROVED; CodeRabbit ultimately APPROVED; no unresolved review threads.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Closes #355
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests