feat: implement issue #580 — Retire/delineate the detection-based apply-rulesets.sh — it diverges from the codified ruleset (#575 debt)#591
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: 48 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 (6)
✨ 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 — 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 to wrap its execution logic in a main function, allowing it to be sourced for unit testing, and updates it to omit the Dev-Lead Agent / dev-lead check from the required status checks. It also updates the corresponding documentation and adds comprehensive BATS regression tests with a mock gh binary. Feedback on the changes highlights a critical issue in the test assertions where a failing loop iteration could be masked, and suggests using the optional/try operator ? in jq filters to safely handle potentially null nested properties.
| while IFS= read -r ctx; do | ||
| [ -z "$ctx" ] && continue | ||
| grep -qxF "$ctx" <<< "$codified" || { | ||
| echo "detector emitted non-codified context: '$ctx'" >&2 | ||
| false | ||
| } | ||
| done <<< "$output" |
There was a problem hiding this comment.
In Bash, a while loop's exit status is determined by the last executed command in the loop. If a non-codified context is encountered in an earlier iteration, false is executed, but the loop continues to the next iteration. If the last iteration succeeds, the loop exits with status 0, causing the test to silently pass despite the failure.
To fix this, track the failure state using a local variable and assert its value after the loop completes.
local failed=0
while IFS= read -r ctx; do
[ -z "$ctx" ] && continue
grep -qxF "$ctx" <<< "$codified" || {
echo "detector emitted non-codified context: '$ctx'" >&2
failed=1
}
done <<< "$output"
[ "$failed" -eq 0 ]
| codified="$(jq -r '.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks[].context' \ | ||
| "${TT_REPO_ROOT}/standards/rulesets/code-quality.json")" |
There was a problem hiding this comment.
When accessing nested properties of an object that may be null or missing in jq filters, use the optional/try operator ? to prevent fatal 'Cannot index null with string' errors.
codified="$(jq -r '.rules[] | select(.type=="required_status_checks") | .parameters?.required_status_checks[]?.context?' \\
"${TT_REPO_ROOT}/standards/rulesets/code-quality.json")"
References
- In
jqfilters within shell scripts, when accessing nested properties of an object that may be null (e.g.,.author.login), use the optional/try operator?(e.g.,.author.login?) to prevent fatal 'Cannot index null with string' errors.
| run build_ruleset_json "code-quality" "CodeQL" "agent-shield / AgentShield" | ||
| [ "$status" -eq 0 ] | ||
|
|
||
| contexts="$(echo "$output" | jq -r '.rules[] | select(.type=="required_status_checks") | .parameters.required_status_checks[].context')" |
There was a problem hiding this comment.
When accessing nested properties of an object that may be null or missing in jq filters, use the optional/try operator ? to prevent fatal 'Cannot index null with string' errors.
contexts="$(echo "$output" | jq -r '.rules[] | select(.type=="required_status_checks") | .parameters?.required_status_checks[]?.context?')"
References
- In
jqfilters within shell scripts, when accessing nested properties of an object that may be null (e.g.,.author.login), use the optional/try operator?(e.g.,.author.login?) to prevent fatal 'Cannot index null with string' errors.
There was a problem hiding this comment.
Pull request overview
Aligns the legacy, detection-based scripts/apply-rulesets.sh with the codified standards/rulesets/code-quality.json by eliminating the non-codified Dev-Lead Agent / dev-lead required-check injection, and adds regression tests + CI to prevent drift from reappearing.
Changes:
- Remove
Dev-Lead Agent / dev-leadfromdetect_required_checks()output and document the rationale (avoids workflow-touching PR deadlocks). - Add a bats test suite (with a stubbed
gh) to assert the detector’s emitted contexts are a strict subset of the codifiedcode-quality.json. - Add a dedicated GitHub Actions workflow to run ShellCheck and the new bats tests when relevant files change.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/apply-rulesets.sh |
Stops injecting the Dev-Lead Agent required context; adds a guarded main() so functions can be sourced/tested. |
test/scripts/apply-rulesets/dev-lead-not-required.bats |
Regression tests ensuring the detector doesn’t emit Dev-Lead Agent and remains aligned with codified contexts. |
test/scripts/apply-rulesets/helpers/setup.bash |
Shared bats helpers for repo/script path setup and installing the gh stub on PATH. |
test/scripts/apply-rulesets/stubs/gh |
Test double for gh api to simulate workflow presence and CodeQL state during unit tests. |
standards/github-settings.md |
Updates documentation to explicitly state Dev-Lead Agent is not a required merge-gate context. |
.github/workflows/apply-rulesets-tests.yml |
CI workflow to run ShellCheck + bats suite for the apply-rulesets script/tests. |
| # Locate the api path: the first positional argument after the `api` subcommand. | ||
| path="" | ||
| prev="" | ||
| for arg in "$@"; do | ||
| if [ "$prev" = "api" ]; then | ||
| path="$arg" | ||
| break | ||
| fi | ||
| prev="$arg" | ||
| done |
| The table below covers the **unconditional fleet-wide required check contexts** — the base set `detect_required_checks()` applies when each org-standard workflow file is present. | ||
| The script also conditionally adds **Dev-Lead Agent** (when `dev-lead.yml` exists) and **Secret scan** (when `ci.yml` contains the gitleaks action), but those are in staged rollout; see below. | ||
| The script also conditionally adds **Secret scan** (when `ci.yml` contains the gitleaks action), which is in staged rollout; see below. It does **not** add **Dev-Lead Agent** as a required context — that is per-PR review, not a merge gate ([#579](https://github.com/petry-projects/.github/issues/579), [#580](https://github.com/petry-projects/.github/issues/580)); see the table row below. | ||
|
|
Review — fix requested (cycle 1/3)The automated review identified the following issues. Please address each one: Findings to fixAutomated review — NEEDS HUMAN REVIEWRisk: MEDIUM SummaryPR removes the non-codified Findings
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. Additional tasks
The review cascade will automatically re-review after new commits are pushed. |
|
Superseded by #590, which retired the detection-based |
Pull request was closed
Closes #580
Implemented by dev-lead agent. Please review.