feat(compliance-audit): detect stale required-check names in rulesets#96
Merged
feat(compliance-audit): detect stale required-check names in rulesets#96
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new compliance check that fetches required-status-check contexts from branch rulesets and classic branch protection for each audited repo, detects stale pre-centralization check names (with suggested renames), and flags forbidden Claude-related required checks. Changes
Sequence Diagram(s)sequenceDiagram
participant Auditor as Compliance Script
participant GHRules as GitHub Rulesets API
participant GHBranch as GitHub Branch Protection API
participant Emitter as Findings Emitter
Auditor->>GHRules: GET required-status-checks for branches/main
GHRules-->>Auditor: ruleset contexts (or 404)
Auditor->>GHBranch: GET branch protection required status checks
GHBranch-->>Auditor: classic contexts (or 404)
Auditor->>Auditor: merge contexts, skip .github, return if none
Auditor->>Auditor: detect stale names & forbidden Claude checks
Auditor->>Emitter: emit findings for each detected issue
Emitter-->>Auditor: acknowledgment
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)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new compliance-audit check to detect required-status-check configuration drift after workflow centralization, preventing merge gates from referencing nonexistent (or structurally problematic) check contexts.
Changes:
- Introduces
check_centralized_check_names()to scan both rulesets and classic branch protection for stale required-check names and suggest canonical replacements. - Flags
claude-code / claudebeing required as a broken configuration that can deadlock workflow-modifying PRs. - Wires the new check into the audit
main()loop.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 the current code and only fix it if needed.
Inline comments:
In `@scripts/compliance-audit.sh`:
- Around line 581-585: The script currently instructs maintainers to rename the
deprecated "claude" check to "claude-code / claude" (string "claude:claude-code
/ claude") while elsewhere flags "claude-code / claude" as a forbidden/broken
required check, causing contradictory remediation; create a dedicated finding
and message for the "claude" case that tells maintainers to stop requiring the
Claude review check (remove it) instead of suggesting a rename, remove or
replace the "claude:claude-code / claude" entry and any user-facing suggestions
that propose "claude-code / claude", and update the ruleset-application logic
that currently recreates banned checks so it will not re-add the Claude-required
check when enforcing rules.
- Around line 578-612: The script currently marks entries from renames as stale
based only on ruleset/classic contexts (variables
ruleset_contexts/classic_contexts and contexts) in the for-loop that checks each
entry and calls add_finding; change this to compute the repo's actual published
canonical check names using the same logic as scripts/apply-rulesets.sh (the
workflow name/job-name derivation around lines 46-136) or at minimum verify the
centralized stub exists before flagging: for each rename in renames, fetch the
repo workflows and derive the current check context name(s), compare those
derived canonical names to old/new, and only call add_finding if the repo is
publishing the centralized stub (or the derived canonical name equals the
centralized name) rather than relying solely on contexts from rulesets/classic
branch protection.
🪄 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: a5a681b2-6163-4a2d-904e-ce61519cbd36
📒 Files selected for processing (1)
scripts/compliance-audit.sh
don-petry
pushed a commit
that referenced
this pull request
Apr 8, 2026
…claude CodeRabbit on #96: the rename map said `claude` → `claude-code / claude`, but a separate check correctly flags `claude-code / claude` as a forbidden required check (it deadlocks workflow PRs). Following the rename recommendation would have moved a repo from one broken state to another. Fix: split the logic into two distinct sets. 1. `renames[]` — only contains checks where the new name is safe to require (AgentShield, Detect ecosystems). These get a "rename to X" message. 2. `forbidden_required[]` — contains every claude variant (legacy and post-centralization). Any of them as a required check emits a stable per-name finding telling the maintainer to REMOVE it from required checks, not rename it. The Claude review check still runs and surfaces feedback on normal PRs without being a merge gate; only the required-status-checks pin is removed. Each forbidden_required entry maps to a stable check id so findings don't churn across audit runs from slashes in canonical names. Verified locally with stub fixtures for all five cases: stale `claude` -> required-claude-check-broken stale `claude-issue` -> required-claude-issue-check-broken `claude-code / claude` -> required-claude-code-check-broken stale `AgentShield` -> stale-required-check-AgentShield (rename) clean ruleset -> 0 findings Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/compliance-audit.sh (2)
578-596:⚠️ Potential issue | 🟠 MajorDerive the replacement check name before calling a required check “stale.”
This logic assumes every repo now publishes
agent-shield / AgentShieldordependency-audit / Detect ecosystems, but it never verifies that. Repos still on the inline pre-centralization workflow — or stubs whose workflowname:has drifted — can still legitimately publish the old context, so this will raise a falsestale-required-check-*finding and tell maintainers to rename to a check that does not exist yet. Please reuse the same context-derivation logic as the ruleset generator, or at minimum gate this finding on the corresponding centralized stub already being in place.Also applies to: 628-633
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/compliance-audit.sh` around lines 578 - 596, The current stale-required-check detection uses the static renames list (local renames) and marks a required-check as "stale" without verifying the replacement actually exists; update the logic so that before emitting a stale-required-check finding you derive the replacement context the same way the ruleset generator does (reuse its context-derivation helper) or at minimum check that the corresponding centralized stub/workflow is present in the repo; specifically modify the code that iterates over renames (the renames array and the stale-check detection branch) to call the ruleset generator's derivation function or perform a presence check of the centralized stub and only emit the rename remediation when the derived replacement is confirmed present.
654-656:⚠️ Potential issue | 🟠 MajorDon’t point remediation at
apply-rulesets.shuntil it stops emitting Claude required checks.This message sends maintainers to
scripts/apply-rulesets.shas the canonical required-check list, butscripts/apply-rulesets.sh:55-64still addsclaudeor${workflow_name} / claude. Using that remediation path will put repos straight back into a state this audit marks broken, so the finding will churn forever. Please land the generator change in the same PR, or remove this reference until the canonical ruleset list no longer writes Claude review checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/compliance-audit.sh` around lines 654 - 656, The remediation link in the add_finding call points maintainers to scripts/apply-rulesets.sh even though that script still injects `claude`/`${workflow_name} / claude` required checks; update the add_finding message to stop referencing scripts/apply-rulesets.sh (or replace it with guidance to land the generator change in the same PR) until scripts/apply-rulesets.sh is changed to stop writing Claude checks, and ensure the message explicitly mentions `claude` / `${workflow_name} / claude` so maintainers know not to re-add those checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/compliance-audit.sh`:
- Around line 601-606: The current forbidden_required exact-match entries only
block a few hardcoded workflow prefixes; update the checks to reject any
required context that ends with " / claude" or " / claude-issue". Replace the
explicit entries in the forbidden_required array (and the other similar array
around the 647-651 block) with suffix patterns like "* / claude" and "* /
claude-issue" (or keep just " / claude" and " / claude-issue" and change the
comparison to use suffix matching like [[ "$context" == *" / claude" ]] / *" /
claude-issue" ]), so the script matches by suffix rather than exact
workflow-name prefixes. Ensure both arrays and all comparison locations use the
same suffix-matching logic.
---
Duplicate comments:
In `@scripts/compliance-audit.sh`:
- Around line 578-596: The current stale-required-check detection uses the
static renames list (local renames) and marks a required-check as "stale"
without verifying the replacement actually exists; update the logic so that
before emitting a stale-required-check finding you derive the replacement
context the same way the ruleset generator does (reuse its context-derivation
helper) or at minimum check that the corresponding centralized stub/workflow is
present in the repo; specifically modify the code that iterates over renames
(the renames array and the stale-check detection branch) to call the ruleset
generator's derivation function or perform a presence check of the centralized
stub and only emit the rename remediation when the derived replacement is
confirmed present.
- Around line 654-656: The remediation link in the add_finding call points
maintainers to scripts/apply-rulesets.sh even though that script still injects
`claude`/`${workflow_name} / claude` required checks; update the add_finding
message to stop referencing scripts/apply-rulesets.sh (or replace it with
guidance to land the generator change in the same PR) until
scripts/apply-rulesets.sh is changed to stop writing Claude checks, and ensure
the message explicitly mentions `claude` / `${workflow_name} / claude` so
maintainers know not to re-add those checks.
🪄 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: 1ba30ca6-5404-4f34-b4a4-99e5cf653027
📒 Files selected for processing (1)
scripts/compliance-audit.sh
Closes #92. Adds `check_centralized_check_names` to compliance-audit.sh. For every non-`.github` repo in the org, fetches the active required-status-check contexts from BOTH the new ruleset system (gh api .../rules/branches/main) and classic branch protection (gh api .../branches/main/protection), then flags two distinct problems: 1. Stale pre-centralization names (`claude`, `claude-issue`, `AgentShield`, `Detect ecosystems`) — emits `stale-required-check-<old-name>` with the canonical replacement in the message. 2. `claude-code / claude` listed as required — emits `required-claude-code-check-broken` because that check is structurally incompatible with workflow-modifying PRs: claude-code-action's GitHub App refuses to mint a token whenever the PR diff includes a workflow file, so the check fails on every workflow PR and the merge gate becomes a deadlock. This was the exact root cause of the markets/bmad-bgreat-suite stuck PRs from #87 sweep. Tested locally with stub fixtures for all four cases (stale claude, stale AgentShield, broken claude-code/claude, clean ruleset). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…claude CodeRabbit on #96: the rename map said `claude` → `claude-code / claude`, but a separate check correctly flags `claude-code / claude` as a forbidden required check (it deadlocks workflow PRs). Following the rename recommendation would have moved a repo from one broken state to another. Fix: split the logic into two distinct sets. 1. `renames[]` — only contains checks where the new name is safe to require (AgentShield, Detect ecosystems). These get a "rename to X" message. 2. `forbidden_required[]` — contains every claude variant (legacy and post-centralization). Any of them as a required check emits a stable per-name finding telling the maintainer to REMOVE it from required checks, not rename it. The Claude review check still runs and surfaces feedback on normal PRs without being a merge gate; only the required-status-checks pin is removed. Each forbidden_required entry maps to a stable check id so findings don't churn across audit runs from slashes in canonical names. Verified locally with stub fixtures for all five cases: stale `claude` -> required-claude-check-broken stale `claude-issue` -> required-claude-issue-check-broken `claude-code / claude` -> required-claude-code-check-broken stale `AgentShield` -> stale-required-check-AgentShield (rename) clean ruleset -> 0 findings Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address remaining CodeRabbit feedback on #96: the previous exact-match list (`claude-code / claude`, `claude-code / claude-issue`) only caught the canonical caller-job-id. Repos with a custom caller — e.g. a ruleset that pins `Claude Code / claude` (workflow display name) or `review-claude / claude` (custom job id) — would slip past the audit even though they're equally broken. Fix: classify each context line by suffix: - bare `claude` / `claude-issue` → match - `<anything> / claude` → match - `<anything> / claude-issue` → match Then map to a stable check id (claude vs claude-issue) so findings don't churn across audit runs from prefix variation. The full forbidden context string is still echoed in the finding message so maintainers see exactly what to remove. Verified locally with stub fixtures for 8 cases: bare claude / claude-issue canonical claude-code / claude{,-issue} custom-prefix Claude Code / claude weird-prefix review-claude / claude stale AgentShield (rename, unaffected) clean ruleset (no findings) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
don-petry
force-pushed
the
feat/compliance-audit-ruleset-drift
branch
from
39faea0 to
13c8024
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #92.
Adds
check_centralized_check_namesto compliance-audit.sh. Inspects both the new ruleset system and classic branch protection, then flags:claude,claude-issue,AgentShield,Detect ecosystems) — emitsstale-required-check-<old>with the canonical replacement in the message.claude-code / claudeas required — emitsrequired-claude-code-check-brokenbecause that check is structurally incompatible with workflow-modifying PRs (claude-code-action's app-token validation refuses to mint a token whenever the PR diff includes a workflow file). This was the exact root cause of the markets/bmad-bgreat-suite stuck PRs from the feat(workflows): centralize standards via reusable workflows #87 sweep.Test plan
bash -nandshellcheckcleanclaude→ flaggedAgentShield→ flaggedclaude-code / claudelisted as required → flagged with broken-deadlock messageagent-shield / AgentShieldonly) → no finding🤖 Generated with Claude Code
Summary by CodeRabbit