Skip to content

feat(standards): harden CODEOWNERS and pr-quality ruleset enforcement - #1379

Open
don-petry wants to merge 2 commits into
mainfrom
fix/codeowners-and-pr-quality-ruleset-enforcement
Open

don-petry wants to merge 2 commits into
mainfrom
fix/codeowners-and-pr-quality-ruleset-enforcement

Conversation

@don-petry

@don-petry don-petry commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Centralizes the CODEOWNERS and pr-quality ruleset compliance checks in combsmith/.github so downstream repos (such as combsmith/bmad-bgreat-suite) can retire divergent repo-local ruleset test scripts and local ruleset forks:

  1. standards/rulesets/pr-quality.json & standards/rulesets/README.md:
    • Codifies {"type": "deletion"} and {"type": "non_fast_forward"} in standards/rulesets/pr-quality.json, matching standards/github-settings.md (Allow force pushes: No, Allow deletions: No) and OpenSSF Scorecard Branch-Protection.
  2. _ruleset_contents_one() in scripts/compliance-audit.sh:
    • Verifies that any parameterless rule types codified in standards/rulesets/<name>.json (deletion and non_fast_forward in pr-quality.json) exist on the live ruleset, emitting ruleset-drift-<name>-missing-rule-<type> when absent (even if the primary $rtype rule is also missing).
  3. check_codeowners() in scripts/compliance-audit.sh & standards/codeowners-standard.md:
    • Probes CODEOWNERS in GitHub's actual evaluation order (.github/CODEOWNERS → CODEOWNERS → docs/CODEOWNERS).
    • Strips carriage returns (\r) and inline # comments on owner lines, and normalizes escaped spaces (\ ) in path patterns before tokenizing owners.
    • Validates that every owner token ($2..$NF) matches ^@${ORG}/[a-z0-9-]+$ (codeowners-individual-users), catching individual @usernames, bare usernames/emails without @, malformed team slugs, and teams outside @combsmith/.
    • Validates and documents in standards/codeowners-standard.md that the default * catch-all line lists ONLY @combsmith/org-leads (codeowners-catchall-extra-owners), preventing extra teams on * from satisfying require_code_owner_review repo-wide.

Verification

  • shellcheck --severity=warning -x scripts/compliance-audit.sh scripts/apply-rulesets.sh
  • bats test/scripts/apply-rulesets/ test/scripts/compliance-audit/codeowners-*.bats test/scripts/compliance-audit/ruleset-contents-drift.bats (71/71 passing)

Refs combsmith/bmad-bgreat-suite#520, combsmith/bmad-bgreat-suite#336

@don-petry
don-petry requested a review from a team as a code owner October 26, 2026 01:43
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-26T02:07:23.815233Z 59c2290 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 37 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 067b44e1-fac9-49c8-bd84-b42841c100d3
Commits

Reviewing files that changed from the base of the PR and between ab9072e and 59c2290.

Files selected for processing: 7
  • scripts/compliance-audit.sh
  • standards/codeowners-standard.md
  • standards/rulesets/README.md
  • standards/rulesets/pr-quality.json
  • test/scripts/apply-rulesets/apply-rulesets.bats
  • test/scripts/compliance-audit/codeowners-catchall.bats
  • test/scripts/compliance-audit/ruleset-contents-drift.bats
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@don-petry don-petry left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full diff and ran the suite locally (69/69 bats passing, shellcheck --severity=warning clean). The core approach — codifying deletion/non_fast_forward, checking parameterless rules, GitHub CODEOWNERS precedence, and inline-comment stripping — is sound. I left 5 line comments: 2 false-positive risks in the new CODEOWNERS tokenization (CRLF, escaped-space patterns), 1 reachability gap in the new parameterless-ruleset check, 1 standard/doc drift for the new catch-all rule, and 1 nit on team-slug validation. None block the ruleset JSON change itself.

Comment thread scripts/compliance-audit.sh Outdated
Comment thread scripts/compliance-audit.sh
Comment thread scripts/compliance-audit.sh
Comment thread scripts/compliance-audit.sh
Comment thread scripts/compliance-audit.sh Outdated
@sonarqubecloud

Copy link
Copy Markdown

@don-petry don-petry left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the full diff at 59c22904 across all 7 files and verified locally (71/71 bats tests passing, shellcheck --severity=warning -x clean):

  1. scripts/compliance-audit.sh (_ruleset_contents_one): Parameterless rule drift check (deletion, non_fast_forward) now runs immediately after live ruleset fetch/validation and before the has_rule != true early return, ensuring all missing codified rules are reported in a single run.
  2. scripts/compliance-audit.sh (check_codeowners): Probes .github/CODEOWNERS → CODEOWNERS → docs/CODEOWNERS in GitHub precedence order, strips \r (CRLF) and inline # comments, normalizes \ escaped spaces in path patterns before awk field splitting, validates owner tokens against ^@${ORG}/[a-z0-9-]+$, and rejects extra teams on the * catch-all line.
  3. standards/codeowners-standard.md, standards/rulesets/pr-quality.json, standards/rulesets/README.md: Documentation and codified ruleset JSON are aligned with the audit checks.
  4. Tests (apply-rulesets.bats, codeowners-catchall.bats, ruleset-contents-drift.bats): All 5 review items have dedicated regression test coverage (71/71 passing).

LGTM — ready for merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant