feat: implement issue #61 — Compliance: codeowners-org-leads-not-first - #552
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
More reviews will be available in 40 minutes and 11 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE 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 adds a CODEOWNERS validation step to the dev-lead-lint.sh script to ensure that @petry-projects/org-leads is always listed as the first owner, accompanied by comprehensive unit tests. The review feedback points out performance and correctness issues with the current implementation—specifically regarding subshell forks per line, CRLF line endings, and escaped spaces in paths—and provides a pure Bash suggestion to resolve them.
| while IFS= read -r line; do | ||
| [[ "$line" =~ ^[[:space:]]*$ ]] && continue | ||
| [[ "$line" =~ ^[[:space:]]*# ]] && continue | ||
| first_owner=$(printf '%s\n' "$line" | awk '{print $2}') | ||
| if [ "$first_owner" != "@petry-projects/org-leads" ]; then | ||
| echo "FAIL: $codeowners_file — @petry-projects/org-leads must be the first owner on line: $line" | ||
| codeowners_fail=1 | ||
| fi | ||
| done < "$codeowners_file" |
There was a problem hiding this comment.
Running printf and awk in a subshell for every line of the file is highly inefficient in Bash because it forks two new processes per line. This can make the lint script noticeably slow, especially on Windows or macOS environments.
Additionally, this approach has two correctness issues:
- CRLF Line Endings: If the
CODEOWNERSfile has CRLF (\r\n) line endings, the trailing\rwill remain on the line and cause the validation to fail. - Escaped Spaces: If a pattern contains escaped spaces (e.g.,
path\\ with\\ spaces/ @owner),awkwill split on the space and incorrectly identify the second part of the path as the owner.
We can resolve all of these issues efficiently using pure Bash built-ins and array splitting.
| while IFS= read -r line; do | |
| [[ "$line" =~ ^[[:space:]]*$ ]] && continue | |
| [[ "$line" =~ ^[[:space:]]*# ]] && continue | |
| first_owner=$(printf '%s\n' "$line" | awk '{print $2}') | |
| if [ "$first_owner" != "@petry-projects/org-leads" ]; then | |
| echo "FAIL: $codeowners_file — @petry-projects/org-leads must be the first owner on line: $line" | |
| codeowners_fail=1 | |
| fi | |
| done < "$codeowners_file" | |
| while IFS= read -r line; do | |
| line="${line%$'\r'}" | |
| [[ "$line" =~ ^[[:space:]]*$ ]] && continue | |
| [[ "$line" =~ ^[[:space:]]*# ]] && continue | |
| clean_line="${line//\\ /}" | |
| read -r -a parts <<< "$clean_line" | |
| first_owner="${parts[1]:-}" | |
| if [ "$first_owner" != "@petry-projects/org-leads" ]; then | |
| echo "FAIL: $codeowners_file — @petry-projects/org-leads must be the first owner on line: $line" | |
| codeowners_fail=1 | |
| fi | |
| done < "$codeowners_file" |
There was a problem hiding this comment.
Implemented the suggestion in scripts/dev-lead-lint.sh (lines 108–116): replaced the printf '%s\n' "$line" | awk '{print $2}' subshell with pure Bash builtins — added line="${line%$'\r'}" to strip CRLF, clean_line="${line//\\ /}" to handle escaped spaces in paths, and read -r -a parts <<< "$clean_line" to split on whitespace without forking processes. Also added three new bats tests in tests/dev-lead/unit/test_dev_lead_lint.bats covering CRLF handling, escaped-space paths, and CRLF-with-wrong-owner; all 22 tests pass.
There was a problem hiding this comment.
Confirmed implemented. scripts/dev-lead-lint.sh lines 108–119 now use pure Bash builtins matching the suggestion exactly: line="${line%$'\r'}" strips CRLF, clean_line="${line//\\ /}" collapses escaped spaces before splitting, and read -r -a parts <<< "$clean_line" replaces the per-line printf | awk subshell. Three corresponding bats tests (lines 256–279 in tests/dev-lead/unit/test_dev_lead_lint.bats) cover CRLF-clean, escaped-space paths, and CRLF-with-wrong-owner. ShellCheck passes with no warnings.
There was a problem hiding this comment.
Verified as implemented: scripts/dev-lead-lint.sh lines 108–119 now match the suggestion exactly — line="${line%$'\r'}" strips CRLF, clean_line="${line//\\ /}" collapses escaped spaces, and read -r -a parts <<< "$clean_line" replaces the per-line printf | awk subshell. Three bats tests (lines 256–279 in tests/dev-lead/unit/test_dev_lead_lint.bats) cover CRLF-clean, escaped-space paths, and CRLF-with-wrong-owner. ShellCheck passes with no warnings.
There was a problem hiding this comment.
Pull request overview
Implements the compliance requirement from issue #61 by adding a repo-local lint guard that validates CODEOWNERS owner lines always list @petry-projects/org-leads as the first owner, preventing future regressions.
Changes:
- Add CODEOWNERS validation to
scripts/dev-lead-lint.sh(checks.github/CODEOWNERS,CODEOWNERS, ordocs/CODEOWNERSin priority order). - Add Bats unit tests covering pass/fail scenarios for the new CODEOWNERS lint behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/dev-lead/unit/test_dev_lead_lint.bats | Adds unit tests for CODEOWNERS lint scenarios (missing file, valid ordering, invalid ordering, comments/blanks). |
| scripts/dev-lead-lint.sh | Adds a new lint phase to enforce @petry-projects/org-leads as the first owner on each non-comment CODEOWNERS line. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 780fe17a11607cd65be2c86c8e3143fa471ff202
Review mode: triage-approved (single reviewer)
Summary
Adds a CODEOWNERS lint phase to scripts/dev-lead-lint.sh that enforces @petry-projects/org-leads as the first owner, with 9 bats tests covering pass/fail/blank/comment/CRLF/escaped-space cases. Implementation is pure bash (no per-line subshells), strips CR for CRLF, and removes escaped spaces before parsing — directly addressing gemini-code-assist's earlier feedback.
Linked issue analysis
Closes #61 (Compliance: codeowners-org-leads-not-first). The new lint phase implements exactly the compliance check named in the issue, looking at .github/CODEOWNERS / CODEOWNERS / docs/CODEOWNERS in priority order. Scope matches the issue.
Findings
No blocking issues.\n\n- The implementation is pure bash with no per-line forks, handles CRLF endings (${line%$'\\r'}) and escaped spaces in paths (${line//\\\\ /}), and skips blank/comment lines correctly.\n- The repo's own .github/CODEOWNERS already lists @petry-projects/org-leads first, so the new check will not break the repository it ships in.\n- shellcheck passes (no warnings on the modified script).\n- Tests cover: missing file, single owner, multi-owner, wrong first owner, missing org-leads, comments+blanks, CRLF, escaped spaces, and CRLF+wrong order. Coverage is thorough.\n- Minor nit (non-blocking): trailing inline comments on an owner line (e.g. * @owner # note) would be parsed as tokens — but CODEOWNERS rarely uses inline comments and this is consistent with how GitHub's own parser treats them in practice.
CI status
All checks passing: CI (Lint, ShellCheck, Agent Security Scan, Compile agentic workflows, Secret scan), Lint workflow (bats, shellcheck, gh-aw-compile, validate-agent-profiles), Tests (unit-tests), Test Dev-Lead Agent (validate-fixtures, unit, prompt-coverage, concurrency-config, caller-permissions, toplevel-permissions, stub-structure, dependency-audit-stub), CodeQL (actions + python), SonarCloud (quality gate passed), AgentShield, Dev-Lead Agent dispatch, Dependency audit (no applicable ecosystems). External reviews (gemini, copilot) have no unresolved blocking comments; CodeRabbit was rate-limited (not a failure).
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 4.7). Reply if you need a human review.
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #552 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 9d64239fb03affbf3ed1c937c531cb0665ad019e
Review mode: triage-approved (single reviewer)
Summary
Triage confirmation of a small, focused change: adds a CODEOWNERS lint phase to scripts/dev-lead-lint.sh enforcing @petry-projects/org-leads as the first owner, with 9 bats tests (pass/fail/blank/comment/CRLF/escaped-space). Already approved by the cascade at 780fe17; the only commits since are merges from main bringing in unrelated files (feature-ideation, release runbook) — no scope changes.
Linked issue analysis
Closes #61 (Compliance: codeowners-org-leads-not-first). The new lint phase implements exactly the named compliance check, scanning .github/CODEOWNERS / CODEOWNERS / docs/CODEOWNERS in priority order. Scope matches the issue.
Findings
No blocking issues.
- Pure bash with no per-line subshell forks; strips CR for CRLF (
${line%$'\\r'}) and removes escaped spaces (${line//\\\\ /}) before tokenizing — directly addresses gemini-code-assist's earlier feedback. - Repo's own
.github/CODEOWNERSalready lists@petry-projects/org-leadsfirst, so the new check will not break the repository it ships in. - shellcheck and bats both pass on head SHA.
- Test coverage is thorough: missing file, single owner, multi-owner, wrong first owner, missing org-leads, comments+blanks, CRLF, escaped paths, CRLF+wrong order.
- Merges from main since prior review touch only unrelated files (feature-ideation workflow + sources, release runbook) and do not affect PR scope.
CI status
All checks passing on 9d64239: CI (Lint, ShellCheck, Agent Security Scan, Compile agentic workflows, Secret scan), Lint workflow (bats, shellcheck, gh-aw-compile, validate-agent-profiles), Tests (unit-tests), Test Dev-Lead Agent (validate-fixtures, unit, prompt-coverage, concurrency-config, caller-permissions, toplevel-permissions, stub-structure, dependency-audit-stub), CodeQL (actions + python), SonarCloud (quality gate passed, 0 new issues), AgentShield, Dev-Lead Agent dispatch, Dependency audit (no applicable ecosystems). External reviewers: gemini and copilot left no unresolved blockers; CodeRabbit was rate-limited (not a failure). mergeStateStatus: BLOCKED is expected — branch protection requires a fresh approval on the current head.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 4.7). Reply if you need a human review.
Superseded by automated re-review at 9d64239.
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: cbabe7e216583e0d3b315b0ea52369e9a1357960
Review mode: triage-approved (single reviewer)
Summary
Adds a CODEOWNERS validation check to scripts/dev-lead-lint.sh enforcing that @petry-projects/org-leads is the first owner on every owner line, with 9 bats tests covering pass/fail cases, comments, blank lines, CRLF endings, and escaped spaces. Confirms the triage assessment: focused, well-tested, low-risk change. Only delta since the prior cascade review (eb7fa4c) is a merge of main; no PR-owned files changed.
Linked issue analysis
Issue #61 (compliance: codeowners-org-leads-not-first) requires CODEOWNERS owner lines to list @petry-projects/org-leads first. The repo's .github/CODEOWNERS is already compliant ('* @petry-projects/org-leads'); this PR adds ongoing enforcement to the dev-lead lint script so regressions are caught locally. Substantively addresses the finding.
Findings
No blocking issues.
- Resolved prior feedback: the gemini-code-assist performance suggestion (per-line
printf | awksubshell) is implemented in the current diff using pure Bash builtins (${line%$'\r'},${line//\\ /},read -r -a parts). The review thread shows as unresolved in the UI, but the owner confirmed implementation and the code verifiably matches the suggestion. - Minor (non-blocking): under
set -euo pipefail,[ "$codeowners_fail" -eq 0 ] && echo "OK: ..."exits the script immediately on a failed check. Exit status is still non-zero (correct), andfail=1is set on the preceding line, but the pattern bypasses the script'sfailaccumulation and would silently skip any checks added after this block. Suggest anifstatement in a follow-up. - Minor (non-blocking):
while IFS= read -r lineskips a final CODEOWNERS line lacking a trailing newline. Edge case; unlikely in practice.
CI status
All 35 checks green: shellcheck, bats, unit-tests, CodeQL (actions+python), SonarCloud quality gate (0 new issues), gitleaks, Agent Security Scan, agent-shield, lint/validate jobs all SUCCESS; remaining checks SKIPPED (not applicable).
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Superseded by automated re-review at cbabe7e.
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: c50ff372b70978100414d4fd02a31af108a7dbef
Review mode: triage-approved (single reviewer)
Summary
Adds a CODEOWNERS validation check to scripts/dev-lead-lint.sh enforcing that @petry-projects/org-leads is the first owner on every CODEOWNERS line, with 9 new bats tests covering pass/fail, comments/blanks, CRLF, and escaped-space paths. +113/-0 across 2 files.
Linked issue analysis
Closes #61 (Compliance: codeowners-org-leads-not-first), which requires CODEOWNERS owner lines to list @petry-projects/org-leads as the FIRST owner. The new check parses each owner line, skips comments/blanks, strips CR and escaped spaces, and fails if parts[1] != @petry-projects/org-leads — substantively implementing the standard as a repeatable lint gate.
Findings
No issues. Logic is correct: parts[0] is the path pattern, parts[1] the first owner. CRLF handled via
CI status
All checks green (shellcheck, bats, unit-tests, CodeQL, SonarCloud, agent-shield, secret scan, etc.); dependency-audit ecosystem jobs SKIPPED as expected. SonarCloud Quality Gate passed with 0 new issues. donpetry-bot review is APPROVED; no CHANGES_REQUESTED reviews and no open human-reviewer questions.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Superseded by automated re-review at c50ff37.
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 9b23306ee2294318fac8bda4fbfd694eecc4dc12
Review mode: triage-approved (single reviewer)
Summary
Adds a CODEOWNERS lint check to scripts/dev-lead-lint.sh enforcing that @petry-projects/org-leads is the first owner on every owner line, plus 9 bats unit tests. Directly resolves compliance finding #61 (codeowners-org-leads-not-first). +113/-0 across 2 files.
Linked issue analysis
Issue #61 is a compliance-audit finding requiring CODEOWNERS owner lines to list @petry-projects/org-leads first (check: codeowners-org-leads-not-first, standard: standards/codeowners-standard.md). The PR implements exactly this validation and exercises it with passing/failing/edge-case tests. Substantively addressed.
Findings
No blocking findings. The earlier gemini-code-assist high-priority comment (printf/awk subshell per line; CRLF; escaped spaces) was raised against an older commit and is fully resolved at the reviewed head: the implementation now uses a pure-bash while IFS= read -r loop, strips trailing CR via
CI status
All checks green: CodeQL (Analyze actions, Analyze python, CodeQL) SUCCESS; CodeRabbit SUCCESS; SonarQube Cloud Quality Gate passed (0 new issues). mergeStateStatus is BLOCKED only pending an approving review.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
|
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 66164943402b0a91f98ce61b6ce4948f6fcda65a
Review mode: triage-approved (single reviewer)
Summary
Adds a CODEOWNERS validation step to scripts/dev-lead-lint.sh enforcing that @petry-projects/org-leads is listed first on every owner line, plus 9 bats unit tests. +113/-0 across 2 files (lint script + tests). Directly implements the enforcement for compliance check codeowners-org-leads-not-first from issue #61.
Linked issue analysis
Issue #61 (OPEN) is an auto-generated compliance finding requiring CODEOWNERS owner lines to list @petry-projects/org-leads first (check codeowners-org-leads-not-first, standard codeowners-standard.md). This PR addresses it by adding an automated lint check that detects/enforces the standard, backed by comprehensive tests. Substantively addressed as a tooling/enforcement implementation.
Findings
No blocking findings. Lint logic correctly handles edge cases: strips CRLF (
CI status
All checks green or skipped (no failures). Includes shellcheck, ShellCheck, bats, unit-tests, CodeQL, SonarCloud, agent-shield/AgentShield, Agent Security Scan, gitleaks secret scan, CodeRabbit — all SUCCESS. mergeStateStatus is BLOCKED pending the required review gate; mergeable=MERGEABLE.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Closes #61
Implemented by dev-lead agent. Please review.