feat: implement issue #800 — Story: BACKFILL + REMEDIATE — bring the fleet to green on the gitignore baseline - #808
Conversation
…fleet to green on the gitignore baseline
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 25 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe compliance remediation dispatcher now automatically fixes gitignore baseline findings by preserving repository-specific content and deploying updates through pull requests. New Bats coverage validates remediation outcomes, and CI now lints and runs these tests. ChangesGitignore baseline remediation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ComplianceRemediate
participant GitignoreBaseline
participant StandardsDeploy
participant GitHub
ComplianceRemediate->>GitignoreBaseline: extract canonical baseline and upsert L1
GitignoreBaseline-->>ComplianceRemediate: updated .gitignore
ComplianceRemediate->>StandardsDeploy: deploy update via PR
StandardsDeploy->>GitHub: create or reuse baseline PR
GitHub-->>StandardsDeploy: return deployment outcome
StandardsDeploy-->>ComplianceRemediate: record remediation or skip result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 automates the remediation of the .gitignore secrets baseline by replacing the manual skip behavior with an automated PR-based sync flow. It introduces the remediate_gitignore_baseline function in scripts/compliance-remediate.sh and adds corresponding tests in test/scripts/compliance-remediate/gitignore-baseline.bats. The review feedback recommends implementing a global tmpfiles array with an EXIT trap to prevent temporary file leaks on premature script termination, and defining a portable base64 decoding helper to ensure tests run successfully on macOS/BSD platforms.
There was a problem hiding this comment.
Pull request overview
Implements the Issue #800 remediation hook to automatically bring repos back to the org-wide .gitignore secrets-baseline (marker-wrapped L1 block) by opening PRs via the shared sd_deploy_via_pr() mechanism, and adds/updates CI + bats coverage to keep this path green.
Changes:
- Adds
remediate_gitignore_baseline()toscripts/compliance-remediate.shand routespush-protection/gitignore_baselinefindings to it (PR-based auto-remediation). - Introduces a dedicated bats suite covering insert/refresh/idempotency/dry-run/failure/“PR already open” scenarios for the remediation path.
- Updates the Gitignore Baseline Tests workflow to include the new remediation coverage and shellcheck the updated script.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
test/scripts/compliance-remediate/routing-messages.bats |
Removes the old “skip” test and documents that this finding is now auto-remediated elsewhere. |
test/scripts/compliance-remediate/gitignore-baseline.bats |
Adds end-to-end-ish remediation tests using a gh stub that asserts the PR flow and payload invariants (L2 preservation, idempotency). |
scripts/compliance-remediate.sh |
Wires in gitignore baseline remediation using gib_extract_baseline_block, upsert_gitignore_baseline, and sd_deploy_via_pr. |
.github/workflows/gitignore-baseline-tests.yml |
Expands the gated suite to include compliance-remediate.sh and the new bats tests. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: da01cedd25611b752c4a0730bb2cc7d1b2537583
Review mode: triage-approved (single reviewer)
Summary
Implements the issue #800 remediation hook: push-protection/gitignore_baseline findings are now auto-remediated by upserting the canonical marker-wrapped L1 secrets-baseline block into the target repo's .gitignore (L2 preserved below END) and shipping the change as a PR via sd_deploy_via_pr(). Adds a 7-case bats suite stubbing the full gh PR flow, retires the old skip-with-guidance test, and wires the script + tests into the Gitignore Baseline Tests workflow gates. All sourced library functions and outcome vocabulary verified present on the head SHA; triage's low-risk assessment confirmed.
Linked issue analysis
Closes #800 (Story: BACKFILL + REMEDIATE). Deliverable 1 (remediation hook wiring upsert_gitignore_baseline() into compliance-remediate.sh, L2 preserved, idempotent, PR-based) is substantively addressed with test coverage for insert, refresh, marker-less preservation, idempotent no-op, dry-run, failed PR creation, and already-open-PR reuse. Deliverables 2 (fleet back-fill) and 3 (verify green) are operational follow-ups executed via this hook, so the issue appropriately stays open until the fleet is green.
Findings
No blocking findings.
- Verified on head SHA: gib_extract_baseline_block / upsert_gitignore_baseline (scripts/lib/gitignore-baseline.sh) and sd_deploy_via_pr (scripts/lib/standards-deploy.sh) exist, and the OPENED/SKIP_PR_OPEN/FAILED case arms match the library's documented outcome vocabulary exactly. All helper functions used (b64_decode, report_pr/skip/fail, info/ok/skip/err) are defined.
- Non-blocking (recommend follow-up): the .gitignore fetch pipeline (gh api | jq | b64_decode || true) treats any failure — including transient network/auth errors — as "file absent" (Copilot flagged this). A transient failure would produce a baseline-only PR that omits the repo's L2 lines. Mitigated: the change ships as a reviewable PR (never a direct push) and idempotency prevents duplicates, but distinguishing HTTP 404 from other errors and calling report_fail otherwise would be more robust.
- Non-blocking nits (Gemini/Copilot, dev-lead posted no-changes): tmpfiles are cleaned on all in-function paths but an EXIT trap would cover premature termination; test helpers use GNU base64 -d, which fails on macOS/BSD (CI runs on Linux).
- Workflow change is safe: path filters, one script added to existing shellcheck invocation, one test dir added to existing bats invocation — no new actions, permissions, or pinning changes.
- Secret-scanning MCP tool unavailable in this environment; the gitleaks CI check passed. No secrets in the diff.
CI status
All completed checks green: Gitignore Baseline Tests (shellcheck + bats incl. the new suite), CI Lint/ShellCheck/Agent Security Scan/Secret scan (gitleaks), CodeQL, SonarCloud (Quality Gate passed, 0 new issues/hotspots), Dependency audits, AgentShield. One dev-lead/dispatch run in progress — agent orchestration reacting to the bot reviews, not a code gate (its prior identical run succeeded). Mergeable; branch is BEHIND main (update before merge).
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 668-693: Update the .gitignore fetch logic in the remediation flow
before existing_file is consumed: distinguish a successful response or genuine
404 from authentication, rate-limit, and other gh api failures. Continue with an
empty file only for 404, but fail fast for all other errors, cleaning up
temporary files and reporting the remediation failure instead of proceeding to
upsert_gitignore_baseline.
In `@test/scripts/compliance-remediate/gitignore-baseline.bats`:
- Around line 1-235: Add failure-path tests for the gitignore_baseline
remediation: configure GITIGNORE_CANONICAL with an incomplete BEGIN/END block to
exercise gib_extract_baseline_block failure, and provide a target .gitignore
containing only one marker to exercise upsert_gitignore_baseline refusal. Assert
each run records gitignore_baseline as FAILED in skipped.md, exits nonzero as
appropriate, and makes no gh pr create call.
🪄 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: 11647280-2283-4dce-8730-cf944e2d9f4b
📒 Files selected for processing (4)
.github/workflows/gitignore-baseline-tests.ymlscripts/compliance-remediate.shtest/scripts/compliance-remediate/gitignore-baseline.batstest/scripts/compliance-remediate/routing-messages.bats
Dev-Lead — review-changes (applied)Changes committed and pushed. |
|
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #808 |
|
Note @don-petry 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. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: e97ffb245a1f717f844f34a48213cc089b74f139
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
Routes the push-protection/gitignore_baseline finding from a manual report_skip to an auto-remediation that upserts the marker-wrapped org secrets baseline (L1) and ships it via the shared, tested sd_deploy_via_pr() primitive, preserving each repo's L2 verbatim. The head (e97ffb2) added fail-fast handling for non-404 gh-api errors so a transient auth/rate-limit/5xx no longer silently replaces a repo's .gitignore with a baseline-only file — the substantive concern is resolved and CodeRabbit re-approved. Remaining advisory nits (test-only hardcoded base64 -d, temp files not registered in a trap) are non-blocking; CI is fully green (bats, shellcheck, CodeQL, gitleaks, agent-shield). Downstream impact: (none). MCP run_secret_scanning was unavailable in this environment, so it was skipped; the gitleaks CI check passed.
Findings
- INFO: Head commit hardened gh-api handling to fail-fast on non-404 errors (auth/rate-limit/5xx) instead of treating them as an absent file, preventing silent loss of a repo's L2 .gitignore content. Backed by a dedicated bats case ('non-404 gh api error ... is reported as a failure'). (
scripts/compliance-remediate.sh:125) - MINOR: Test decode hardcodes
base64 -d, which fails on macOS/BSD (expects-D). Production code correctly uses the portableb64_decodehelper, and CI runs on Linux, so this only affects local macOS dev. Gemini's suggestedb64dhelper would make it consistent with the portableb64()encoder the file already defines. Non-blocking. (test/scripts/compliance-remediate/gitignore-baseline.bats:379) - MINOR: Temp files created in remediate_gitignore_baseline() (block_file, existing_file, api_out, api_err, upserted_file) are cleaned via explicit rm -f on every normal return path, but are not registered in a trap-backed cleanup array, so they can leak on premature termination (SIGINT/set -e). This mirrors the pre-existing untrapped REPORT_DIR mktemp pattern in the same script, so it is consistent, not a new regression. Gemini's global
tmpfilesarray + EXIT trap would harden it. Non-blocking. (scripts/compliance-remediate.sh:109)
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
…ixtures #809 gave upsert_gitignore_baseline a negation re-allow tail (to defend against non-identical L2 globs like **/*.pem re-hiding !public.pem). That changed the idempotent steady state to block + neutralized-L2 + tail. Two older fixtures (#798 sync dry-run, #808 remediate) fed the raw canonical (tail-less) as 'already current', so the current-baseline skip/no-op detection saw a diff and planned a PR. Feed those fixtures the actual upsert steady state instead. Upsert is idempotent from the 2nd application (verified), so this is the true no-op input. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fa6TNhxEXj6WAKpaJLHvNK
…baseline negations + migrate unmarkered baselines (blocks #800 apply) (#815) * feat: implement issue #809 — upsert_gitignore_baseline must preserve baseline negations + migrate unmarkered baselines (blocks #800 apply) * fix(reviews): address review comments [skip ci-relay] * chore: dev-lead update (review-changes) [skip ci-relay] * chore: dev-lead update (review-changes) [skip ci-relay] * test: feed the post-#809 upsert steady state to the already-current fixtures #809 gave upsert_gitignore_baseline a negation re-allow tail (to defend against non-identical L2 globs like **/*.pem re-hiding !public.pem). That changed the idempotent steady state to block + neutralized-L2 + tail. Two older fixtures (#798 sync dry-run, #808 remediate) fed the raw canonical (tail-less) as 'already current', so the current-baseline skip/no-op detection saw a diff and planned a PR. Feed those fixtures the actual upsert steady state instead. Upsert is idempotent from the 2nd application (verified), so this is the true no-op input. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fa6TNhxEXj6WAKpaJLHvNK --------- Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Part of #800
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
New Features
.gitignorebaseline findings.Bug Fixes
Tests