feat: implement issue #338 — Compliance: ruleset-drift-pr-quality-require_code_owner_review - #372
feat: implement issue #338 — Compliance: ruleset-drift-pr-quality-require_code_owner_review#372don-petry wants to merge 3 commits into
Conversation
…uire_code_owner_review
|
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: 49 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
![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 updates the ruleset setup script and its corresponding BATS tests to ensure that require_code_owner_review is set to true, in compliance with issue #338. Feedback suggests using defensive programming (such as .get() and fallbacks) when parsing the JSON payload in the new test to prevent unhandled exceptions and ensure clearer failure messages.
| run python3 - "$payload_file" << 'PY' | ||
| import json, sys | ||
| with open(sys.argv[1]) as f: | ||
| d = json.load(f) | ||
| pr_rule = next(r for r in d["rules"] if r["type"] == "pull_request") | ||
| val = pr_rule["parameters"]["require_code_owner_review"] | ||
| assert val is True, f"expected require_code_owner_review true, got {val!r}" | ||
| print("ok") | ||
| PY |
There was a problem hiding this comment.
When parsing JSON payloads in test assertions, it is safer to use defensive programming with safe navigation and fallbacks (e.g., .get()). This prevents unhandled KeyError, TypeError, or StopIteration exceptions and ensures that assertions fail with clear, descriptive messages if the payload structure is unexpected or empty.
run python3 - "$payload_file" << 'PY'
import json, sys
with open(sys.argv[1]) as f:
d = json.load(f) or {}
rules = d.get("rules") or []
pr_rule = next((r for r in rules if isinstance(r, dict) and r.get("type") == "pull_request"), None)
assert pr_rule is not None, "pull_request rule not found"
val = pr_rule.get("parameters", {}).get("require_code_owner_review")
assert val is True, f"expected require_code_owner_review true, got {val!r}"
print("ok")
PY
References
- When writing test assertions that parse structured files (like JSON or YAML), ensure that all parsed data lookups use safe navigation and fallbacks to prevent unhandled KeyError or TypeError exceptions.
There was a problem hiding this comment.
Applied the defensive-programming pattern in scripts/tests/setup-rulesets.bats (lines 113–121): replaced bare dict key access with .get() and or fallbacks, added an isinstance guard on each rule, switched next() to the two-argument form with a None default, and added an explicit assertion for the None case so failures report "pull_request rule not found" instead of a bare StopIteration or KeyError.
There was a problem hiding this comment.
The defensive-programming pattern was already applied by don-petry at lines 113–121 of scripts/tests/setup-rulesets.bats (confirmed in their reply above): .get() with or {} fallbacks, isinstance guard on each rule, next() with a None default, and an explicit assert pr_rule is not None for a clear failure message. No further code changes needed here.
Dev-Lead — review-changes (applied)Changes committed and pushed. |
There was a problem hiding this comment.
Pull request overview
This PR aims to address compliance issue #338 by ensuring the pr-quality ruleset configuration enforces require_code_owner_review: true, and by adding regression coverage/documentation around that requirement.
Changes:
- Added a Bats test asserting
require_code_owner_reviewistruein the generatedpr-qualityruleset payload. - Added/expanded documentation comments describing why
require_code_owner_reviewmust be enabled forpr-quality.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scripts/tests/setup-rulesets.bats | Adds a dedicated drift-focused test for require_code_owner_review and expands header commentary for compliance traceability. |
| scripts/setup-rulesets.sh | Adds explanatory comments tying require_code_owner_review to compliance issue #338. |
| # ── Drifted-parameter test (the finding in issue #338) ───────────────────────── | ||
|
|
||
| @test "pr-quality payload sets require_code_owner_review to true" { | ||
| run bash "$BATS_TEST_DIRNAME/../setup-rulesets.sh" | ||
| [ "$status" -eq 0 ] |
There was a problem hiding this comment.
Consolidated in scripts/tests/setup-rulesets.bats: removed the standalone "pr-quality payload sets require_code_owner_review to true" test and added a compliance annotation (issue #338 + #339) to the "pr-quality payload matches all codified pull_request parameters" section comment, which already asserts require_code_owner_review: True in its expected dict. All 78 tests pass.
| # dismiss_stale_reviews_on_push MUST be true: it re-requests review after any | ||
| # push so approvals cannot be inherited by unreviewed code (compliance: #339). | ||
| # | ||
| # require_code_owner_review MUST be true: PRs cannot merge without review from a | ||
| # CODEOWNERS-designated owner (compliance: #338). | ||
| # |
There was a problem hiding this comment.
Acknowledged. The payload in setup-rulesets.sh already sets require_code_owner_review: true and the PR adds regression test coverage so the setting is never accidentally dropped. Actually applying the ruleset to the live repo (i.e., running setup-rulesets.sh) is a deployment step done outside this PR — that run closes the live drift. The PR title/description can be updated to clarify it's adding compliance regression coverage for #338.
Dev-Lead — review-changes (applied)Changes committed and pushed. |
|
|
Auto-rebase failed — merge conflict — this branch has conflicts with dev-lead will attempt to resolve this automatically. If it cannot, a follow-up comment will explain what needs manual attention. To resolve manually instead: |
|
Auto-rebase failed — merge conflict — this branch has conflicts with dev-lead will attempt to resolve this automatically. If it cannot, a follow-up comment will explain what needs manual attention. To resolve manually instead: |
Closes #338
Implemented by dev-lead agent. Please review.