feat: implement issue #252 — Compliance: secret_scanning_non_provider_patterns#422
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 34 minutes and 5 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ 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 (3)
📝 WalkthroughWalkthroughThis PR extends the compliance-remediate script with a new handler function to remediate repository-level ChangesSecurity Analysis Remediation Handler
Sequence DiagramsequenceDiagram
participant Dispatcher as remediate_finding
participant Handler as remediate_security_analysis_setting
participant GH as gh api
participant Report as remediation-report.md / skipped.md
Dispatcher->>Handler: push-protection/non_provider_patterns_enabled
alt DRY_RUN mode
Handler->>Report: log to remediation-report.md
else Normal mode
Handler->>GH: PATCH repos/$ORG/$repo
Note over GH: security_and_analysis.secret_scanning_non_provider_patterns.status = enabled
alt PATCH succeeds
Handler->>Report: record to remediation-report.md
else PATCH fails
Handler->>Report: record FAILED to skipped.md
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
![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 introduces a new function remediate_security_analysis_setting to compliance-remediate.sh to handle nested security_and_analysis settings via the GitHub API, specifically targeting the secret_scanning_non_provider_patterns push-protection finding. It also adds a comprehensive test suite using bats and a mock gh stub. The review feedback correctly identifies that the actual check name produced by the audit script is non_provider_patterns_enabled rather than secret_scanning_non_provider_patterns. It is recommended to update the script to support both names for backward compatibility and to update the test cases to use the canonical check name.
There was a problem hiding this comment.
Pull request overview
Implements automated remediation for compliance finding push-protection/secret_scanning_non_provider_patterns by PATCHing the repo’s nested security_and_analysis settings via the GitHub API, and adds Bats coverage + a gh stub to lock in expected behavior.
Changes:
- Add
remediate_security_analysis_setting()to PATCHsecurity_and_analysis.<setting>.statususing a nested JSON payload overgh api --input -. - Route
push-protection/secret_scanning_non_provider_patternsfindings through the new remediation path (preserving thepush-protection/*catch-all skip behavior). - Add Bats tests + test helpers/stubs for the new remediation path (apply, dry-run, failure, and catch-all cases).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
scripts/compliance-remediate.sh |
Adds nested security_and_analysis PATCH helper and wires in the new push-protection remediation case. |
test/scripts/compliance-remediate/security-analysis.bats |
New Bats suite validating apply/dry-run/failure behavior and ensuring other push-protection findings still skip. |
test/scripts/compliance-remediate/helpers/setup.bash |
Shared Bats helpers for temp dirs, installing the stub gh, and generating a minimal findings JSON file. |
test/scripts/compliance-remediate/stubs/gh |
Adds a simple env-driven gh stub with argv + stdin logging for deterministic tests. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #422 |
|
Note I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@scripts/compliance-remediate.sh`:
- Around line 157-170: In remediate_security_analysis_setting(): replace the
POSIX test with Bash conditional [[ "$DRY_RUN" == "true" ]] for consistency when
checking DRY_RUN, and after constructing payload with payload=$(jq -n --arg key
"$setting" --arg status "$expected_status" '{security_and_analysis: {($key):
{status: $status}}}'), immediately verify jq succeeded and payload is non-empty
(e.g., test the exit status and that payload is not empty) and bail/skip/return
before calling gh api if the check fails; ensure the gh api PATCH invocation
that consumes --input - only runs when payload is valid.
In `@test/scripts/compliance-remediate/helpers/setup.bash`:
- Around line 1-3: Add Bash strict mode to the top of the helper by enabling
"set -euo pipefail" immediately after the existing shebang line so the script
fails fast on errors, treats unset variables as errors, and makes pipelines fail
properly; update the file beginning in
test/scripts/compliance-remediate/helpers/setup.bash (after the "#!/usr/bin/env
bash" line) to include the strict mode invocation and ensure any callers that
rely on relaxed semantics are adjusted accordingly.
- Around line 4-11: The top-level path variables TT_REPO_ROOT, TT_SCRIPT, and
TT_STUBS_DIR should be made immutable: after you assign and export each variable
in setup.bash (the TT_REPO_ROOT, TT_SCRIPT, TT_STUBS_DIR assignments), mark them
readonly (e.g., with readonly or declare -r) so they cannot be modified later;
keep the export calls and simply apply readonly to those three symbols to
satisfy the coding guideline.
- Around line 18-31: The tt_cleanup_tmpdir and tt_install_gh_stub helpers should
fail fast when TT_TMP is unset and use Bash conditionals: update both functions
to reference TT_TMP using the parameter expansion form ${TT_TMP:?"TT_TMP is
required"} (or similar message) so the script exits immediately if TT_TMP is
missing, and replace test brackets [ ... ] with Bash [[ ... ]] in
tt_cleanup_tmpdir (e.g., [[ -n "${TT_TMP}" && -d "${TT_TMP}" ]]) to ensure
Bash-safe conditionals while leaving the existing logic (rm -rf, mkdir -p, cp,
chmod, and PATH export) intact.
In `@test/scripts/compliance-remediate/stubs/gh`:
- Around line 27-30: The current stub swallows failures when writing stdin to
GH_STUB_STDIN_LOG via the line "cat >>\"$GH_STUB_STDIN_LOG\" || true"; change
this so write errors are not suppressed: remove the "|| true", capture the cat
exit status and, on failure, emit a clear stderr message (including
$GH_STUB_STDIN_LOG) and exit non‑zero so test instrumentation failures surface;
update the if block that checks GH_STUB_STDIN_LOG accordingly (refer to the
GH_STUB_STDIN_LOG variable and the cat >>"$GH_STUB_STDIN_LOG" invocation).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1421b4f6-2e31-45c2-baa6-5d5c2360a020
📒 Files selected for processing (4)
scripts/compliance-remediate.shtest/scripts/compliance-remediate/helpers/setup.bashtest/scripts/compliance-remediate/security-analysis.batstest/scripts/compliance-remediate/stubs/gh
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
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: MEDIUM
Reviewed commit: 2568860f9fa73fe966af868fce7f7890ec65fae2
Review mode: triage-approved (single reviewer)
Summary
Adds an auto-remediation path for the push-protection secret_scanning_non_provider_patterns finding in scripts/compliance-remediate.sh. A new remediate_security_analysis_setting() helper builds a nested security_and_analysis payload with jq -n --arg and PATCHes it via gh api ... --input - (since gh api -F only sends flat fields). The dispatcher now routes both the canonical non_provider_patterns_enabled and the legacy secret_scanning_non_provider_patterns check names to the new helper, while the rest of the push-protection/* catch-all is preserved. New bats suite covers apply, dry-run, failure (non-zero gh exit), and the preserved catch-all.
Linked issue analysis
Closes #252. The audit reopens the finding weekly because secret_scanning_non_provider_patterns is disabled on this repo and compliance-remediate.sh previously routed every push-protection/* finding to report_skip. This PR adds the missing auto-remediation path, scoped to the single setting the audit reports, consistent with the issue's stated scope. The dev-lead comments correctly flag that the actual setting flip on petry-projects/.github still requires running the script with a repo-admin token (out of scope for this PR).
Findings
- scripts/compliance-remediate.sh:148-184 —
remediate_security_analysis_setting()usesjq -n --argfor safe JSON construction, guards ajqfailure / empty payload, honorsDRY_RUN, and reports a failure on non-zeroghexit. No injection vectors (noeval, no shell interpolation of user data into the payload). - scripts/compliance-remediate.sh:726-728 — Dispatcher case is placed above the
push-protection/*catch-all and matches both the canonical (non_provider_patterns_enabled) and legacy (secret_scanning_non_provider_patterns) names; catch-all preserved for all other push-protection findings. - test/scripts/compliance-remediate/ — New bats suite (4 cases) pins apply, dry-run, failure, and catch-all behaviours. Helper uses
set -euo pipefail,readonlyconstants, and${var:?}guards as required by the repo's shell standards.ghstub is env-driven, logs argv and stdin, surfaces log-write failures (no silent swallow). - Prior bot review threads from gemini-code-assist and coderabbitai (canonical-name fix, jq guard, strict-mode helper, readonly constants,
${var:?}guards, stub stdin-log handling) are all resolved. - No org/standards violations; no GitHub Actions security smells; no
.env*or secret material touched; no migrations.
CI status
All checks green: Lint, ShellCheck, Secret scan (gitleaks), Agent Security Scan, CodeQL (actions), SonarCloud, AgentShield, dev-lead dispatch, pr-auto-review/check-and-dispatch, CodeRabbit. Dependency-audit ecosystem jobs (npm/pnpm/cargo/pip/govuln) correctly SKIPPED — no manifests for those ecosystems. mergeStateStatus: BEHIND (branch is behind main); no merge conflict, just needs an update from main.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 4.7). Reply if you need a human review.
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
|
…_patterns (#422) * feat: implement issue #252 — Compliance: secret_scanning_non_provider_patterns * fix(reviews): address review comments [skip ci-relay] * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Co-authored-by: Don Petry Bot <donpetry+bot@gmail.com>
Closes #252
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
New Features
Tests