feat: implement issue #580 — Retire/delineate the detection-based apply-rulesets.sh — it diverges from the codified ruleset (#575 debt)#583
Conversation
…ly-rulesets.sh — it diverges from the codified ruleset (#575 debt)
|
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: 20 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 (3)
✨ 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. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
![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 refactors scripts/apply-rulesets.sh by wrapping its execution logic in a main function to allow sourcing, and removes the automatic injection of the Dev-Lead Agent / dev-lead required status check to align with codified standards. It also introduces a BATS test suite to verify this behavior. The review feedback suggests several improvements, including initializing mock variables in the test setup to prevent unbound variable errors under set -u, guarding printf against empty arrays, declaring variables as local in main to avoid global scope pollution, and using the optional/try operator ? in jq filters to prevent potential indexing errors.
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #580 by preventing the legacy/detection-based scripts/apply-rulesets.sh from diverging from the codified ruleset source-of-truth, specifically by removing the non-codified Dev-Lead Agent / dev-lead required-check injection. It also adds a small test harness and CI workflow to guard against regressions in the detection logic.
Changes:
- Removed the
Dev-Lead Agent / dev-leadrequired-status-check injection fromdetect_required_checks(). - Refactored
scripts/apply-rulesets.shto use amain()entrypoint with a “run only when executed” guard (enables sourcing in tests). - Added a bats test suite plus a dedicated GitHub Actions workflow to run ShellCheck + bats for this script.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
test/scripts/apply-rulesets/dev-lead-divergence.bats |
Adds bats coverage to ensure Dev-Lead Agent / dev-lead is not emitted and to compare detected contexts to the codified ruleset. |
scripts/apply-rulesets.sh |
Removes Dev-Lead required-check injection; adds legacy annotations and a guarded main() for testability. |
.github/workflows/apply-rulesets-tests.yml |
Introduces CI gates (ShellCheck + bats) for PRs touching the script/tests. |
| */contents/.github/workflows/sonarcloud.yml) | ||
| # No top-level `name:` → workflow_name() returns empty → detection uses | ||
| # the bare `SonarCloud` fallback, matching the codified code-quality.json. | ||
| printf 'on: push\njobs:\n scan: {}\n' | base64 | ||
| ;; |
There was a problem hiding this comment.
Fixed in two places: (1) Updated the sonarcloud.yml stub (bats line ~46) to include name: SonarCloud Analysis and job name: SonarCloud, matching the org-standard standards/workflows/sonarcloud.yml. (2) Fixed detect_required_checks() in scripts/apply-rulesets.sh to always emit SonarCloud directly — SonarCloud registers its check context via the SonarCloud API as the bare job name regardless of the workflow's top-level name:, so the workflow_name()-derived compound SonarCloud Analysis / SonarCloud was incorrect. Added a new test sonarcloud.yml with 'name: SonarCloud Analysis' emits bare 'SonarCloud' context that explicitly exercises this realistic path.
There was a problem hiding this comment.
Confirmed implemented and verified. The sonarcloud.yml stub in test/scripts/apply-rulesets/dev-lead-divergence.bats (line ~46-52) now includes name: SonarCloud Analysis and job name: SonarCloud, matching the org-standard standards/workflows/sonarcloud.yml. The new test sonarcloud.yml with 'name: SonarCloud Analysis' emits bare 'SonarCloud' context (lines 90-100) exercises the realistic named-workflow path and asserts the compound form is never emitted. detect_required_checks() in scripts/apply-rulesets.sh (lines 89-97) emits SonarCloud directly — independent of the workflow's top-level name:. All 5 bats tests pass; ShellCheck --severity=warning clean.
| @test "detected set matches the codified code-quality contexts for these workflows" { | ||
| # With sonarcloud + agent-shield + dependency-audit + configured CodeQL, the | ||
| # detection output must equal the codified standards/rulesets/code-quality.json | ||
| # context set (order-independent) — no extra Dev-Lead injection. | ||
| MOCK_WORKFLOWS=("sonarcloud.yml" "agent-shield.yml" "dependency-audit.yml" "dev-lead.yml") | ||
| MOCK_CODEQL_STATE="configured" | ||
| run detect_required_checks "somerepo" |
There was a problem hiding this comment.
Reconciled all three sources: fixed detect_required_checks() in scripts/apply-rulesets.sh to always emit SonarCloud (never SonarCloud Analysis / SonarCloud) — SonarCloud registers its check directly via the SonarCloud API as SonarCloud, independent of the workflow's top-level name:. The stub now uses realistic content (name: SonarCloud Analysis, job name: SonarCloud), the test still asserts against the codified code-quality.json (which correctly lists SonarCloud), and the new named-workflow test verifies no compound form is emitted. All five tests pass.
There was a problem hiding this comment.
Confirmed reconciled and verified. detect_required_checks() in scripts/apply-rulesets.sh (lines 89-97) always emits the bare SonarCloud context — never SonarCloud Analysis / SonarCloud. The stub at bats line ~46-52 uses realistic content (name: SonarCloud Analysis, job name: SonarCloud). The existing test at line 116-130 still asserts the detected set equals standards/rulesets/code-quality.json (which correctly lists SonarCloud), and the new test at line 90-100 explicitly verifies no compound form is emitted. All 5 bats tests pass; ShellCheck --severity=warning clean.
![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 — review-changes (applied)Changes committed and pushed. |
|
|
Superseded by #590 — which implements the decided Option 1 (retire) rather than the annotate here. That PR salvages this one's divergence-guard intent as a bats test against the codified |
Pull request was closed
…plier canonical in .github (#580) (#590) * feat(rulesets): retire the detection-based apply-rulesets.sh; codified file-driven applier canonical in .github (#580) Implements the user's #580 decision (Option 1 — retire, not annotate). Supersedes the Option-2 PR #583. scripts/apply-rulesets.sh — REPLACE the detection-based builder (which probed each repo's workflows and *generated* the ruleset in-code, diverging from the codified set — e.g. injecting `Dev-Lead Agent`) with the codified file-driven applier: - reads the local standards/rulesets/*.json source of truth (co-located in .github), - --repo owner/repo | <repo-name> (back-compat positional) | --all | --dry-run, optional [<name>...] ruleset filter, - find-or-create per ruleset (PUT by id / POST), idempotent convergence. Net -400 lines (the detect_required_checks / build_ruleset_json machinery is gone). "All detection items in the file-driven approach" (nothing org-wide lost): SonarCloud, CodeQL, agent-shield/AgentShield, dependency-audit/Detect ecosystems → carried statically in code-quality.json. Dev-Lead Agent → intentionally excluded (the retired divergence). CI Pipeline (build-and-test) → repo-specific → per-repo branch protection. Secret scan (gitleaks) + coverage → template-produced; staged fleet-wide via #581. Tests + CI (salvages #583's divergence-guard intent, against the codified JSON): test/scripts/apply-rulesets/apply-rulesets.bats (13 tests incl. the "no Dev-Lead Agent context" guard) + .github/workflows/apply-rulesets-tests.yml (shellcheck+bats). Reference reconciliation (detection language → codified model): - github-settings.md: intro + Required-Check preamble + SonarCloud row (was detection-derived `SonarCloud Analysis / SonarCloud`; codified is `SonarCloud`). - ci-standards.md §CodeQL, push-protection.md step 4, compliance-audit.sh remediation message, ruleset-remediation-runbook.md. - pull-request-limits-adr.md: dated correction note (the "no static JSON / generated in-code" premise no longer holds; §7.2/7.3 conclusion unaffected). Verified: shellcheck + yamllint + actionlint + markdownlint clean; a live --dry-run against petry-projects/.github-private resolves the LOCAL standards/rulesets/ and updates the existing rulesets in place (PUT by id, no recreate). Remaining follow-up (noted, not in scope): consolidate .github-private's bootstrap applier (it now reads the same codified source via fetch, so no divergence — the full single-physical-applier "cross-repo bootstrap story" is a separate step). Closes #580. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(bot): address bot feedback [skip ci-relay] --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
Closes #580
Implemented by dev-lead agent. Please review.