Skip to content

Customize codeowner approval logic as a status check workflow#2542

Closed
aleozlx wants to merge 1 commit intomainfrom
aleozlx/coder_owner
Closed

Customize codeowner approval logic as a status check workflow#2542
aleozlx wants to merge 1 commit intomainfrom
aleozlx/coder_owner

Conversation

@aleozlx
Copy link
Copy Markdown
Collaborator

@aleozlx aleozlx commented Feb 12, 2026

📌 Description

🔍 Related Issues

🚀 Pull Request Checklist

Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete.

✅ Pre-commit Checks

  • I have installed pre-commit by running pip install pre-commit (or used your preferred method).
  • I have installed the hooks with pre-commit install.
  • I have run the hooks manually with pre-commit run --all-files and fixed any reported issues.

If you are unsure about how to set up pre-commit, see the pre-commit documentation.

🧪 Tests

  • Tests have been added or updated as needed.
  • All tests are passing (unittest, etc.).

Reviewer Notes

Summary by CodeRabbit

  • Chores
    • Introduced automated CODEOWNER-based approval enforcement for pull requests using GitHub Actions.
    • Pull requests now require approval from designated code owners before merging, with approval status reflected in commit checks.

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Note

Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Feb 12, 2026

📝 Walkthrough

Walkthrough

A new GitHub Actions workflow is introduced that enforces CODEOWNER-based approval requirements for pull requests. The workflow parses CODEOWNERS rules, identifies code owners for changed files, verifies that at least one owner (who is not the PR author) has approved, and sets a commit status accordingly.

Changes

Cohort / File(s) Summary
GitHub Actions Workflow
.github/workflows/codeowner-approval-status.yml
Adds a new workflow triggered on PR review submissions and open/sync events. Parses the CODEOWNERS file, maps changed files to their owners, checks for independent owner approvals, and updates the commit status with the approval outcome.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • nvmbreughe

Poem

🐰 With whiskers twitching, I approve this quest,
A workflow checks who owns the best,
CODEOWNERS parsed, approvals found,
Before the merge can go around!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is a template with all checkboxes marked complete, but the actual description sections (Description and Related Issues) are empty. Fill in the Description section explaining what the CODEOWNER approval workflow does and why it's needed. Link any related issues in the Related Issues section.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: introducing a GitHub Actions workflow that enforces CODEOWNER-based approval as a status check.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch aleozlx/coder_owner

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 and usage tips.

@claude
Copy link
Copy Markdown

claude bot commented Feb 12, 2026

Code Review: Codeowner Approval Status Workflow

Thank you for implementing this custom codeowner approval logic! I've reviewed the workflow and have several observations and suggestions.

Strengths

  1. Clean implementation - The logic for parsing CODEOWNERS and matching files is well-structured
  2. Proper last-match semantics - Correctly implements CODEOWNERS behavior where the last matching rule wins
  3. Self-author exclusion - Good edge case handling prevents PRs from being auto-approved when the author is the only codeowner
  4. Clear status messages - The commit status descriptions are informative

Issues & Concerns

1. Missing Error Handling for Empty CODEOWNERS (Medium Priority)

The workflow silently handles missing CODEOWNERS files but doesn't fail gracefully when the file exists but is empty or malformed (lines 36-40). If CODEOWNERS exists but all rules are invalid, allOwners will be empty and PRs will be blocked with an unclear error message. Recommend adding explicit validation after parsing.

2. Race Condition: Base SHA vs Head SHA (High Priority)

Line 18 checks out the base branch SHA, reading CODEOWNERS from the base branch while the PR might be changing the CODEOWNERS file itself. If a PR adds new paths or modifies ownership rules, those changes won't apply to the PR. Consider using the head SHA instead, or explicitly document this as intended behavior.

3. Review Ordering Assumption (High Priority)

Lines 61-63 build a map of latest reviews assuming the GitHub API returns reviews in chronological order. Recommend sorting by submitted_at explicitly to guarantee correct behavior.

4. Logic Flaw: hasQualifiedApprover (Medium Priority)

Line 58 has a subtle issue. When a file is owned by only the PR author, the error message says "no qualified approver. update CODEOWNERS!" which is misleading. Recommend splitting error messages: "no codeowners defined for changed files" vs "author is the only codeowner - requires another reviewer".

5. Missing Null Check (Critical)

Lines 77-78 can throw "Cannot read property 'user' of undefined" if .find() returns undefined. Add optional chaining to prevent crashes.

6. Minimatch Version Pinning (Low Priority)

Line 19 pins to major version 9. Consider pinning to a specific minor/patch version for reproducibility (e.g., minimatch@9.0.5).

Additional Suggestions

7. Team Support - Current implementation only supports individual GitHub users. If CODEOWNERS uses teams, this workflow won't work correctly. Document this limitation or add team expansion via the GitHub API.

8. Debug Logging - For troubleshooting, consider adding console output for changed files, required approvers, and latest reviews.

9. Workflow Trigger Optimization - Consider adding "reopened" and "edited" to the PR triggers for better coverage.

Security Considerations

  • Permissions are minimal (pull-requests: read and statuses: write) - good!
  • No code execution from PR (checking out base SHA prevents malicious PR code from running)
  • Verify GITHUB_TOKEN has statuses: write permission in repo settings

Testing Recommendations

Before merging, test these scenarios:

  1. PR where author is the only codeowner
  2. PR where author is one of multiple codeowners
  3. PR modifying files with no CODEOWNERS entry
  4. PR where CODEOWNERS file is modified
  5. PR from non-codeowner (should require approval)
  6. Multiple reviews from same person (approve → request changes → approve)
  7. Review from non-codeowner followed by review from codeowner

Summary

Issue Severity Lines
Base SHA checkout (CODEOWNERS from wrong ref) High 18
Missing null check on approver lookup Critical 77-78
Review ordering assumption High 61-63
Misleading error message for author-only owner Medium 58, 74
No validation for empty CODEOWNERS Medium 36-40
Minimatch version not fully pinned Low 19

Overall, this is a solid implementation with good logic. The main concerns are around edge cases and error messages. Please address the critical and high-priority issues before merging.


Review generated with assistance from Claude Code

@aleozlx aleozlx marked this pull request as ready for review February 12, 2026 01:12
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🤖 Fix all issues with AI agents
In @.github/workflows/codeowner-approval-status.yml:
- Line 90: Remove the trailing whitespace from the reported line in the workflow
file so the pre-commit `trailing-whitespace` hook passes; open
.github/workflows/codeowner-approval-status.yml, locate the
blank/whitespace-only characters at the end of the failing line and delete them
(ensure the line ends with no spaces or tabs), then save and commit the change.
- Around line 8-10: The workflow's explicit permissions block omits contents,
causing actions/checkout to fail; update the permissions mapping by adding
"contents: read" alongside the existing "pull-requests: read" and "statuses:
write" entries so the actions/checkout step can clone the repo and read
.github/CODEOWNERS (adjust the top-level permissions block where "pull-requests"
and "statuses" are defined).
- Around line 45-53: The CODEOWNERS matching loop uses minimatch(f.filename,
rule.pattern) which does not emulate CODEOWNERS/.gitignore semantics for
patterns without slashes; update the match call to pass the option { matchBase:
true } (or normalize rule.patterns to '**/pattern' before matching) so patterns
like '*.py' match files in subdirectories; modify the inner loop where minimatch
is invoked (referencing variables minimatch, f.filename, rule.pattern,
lastMatch) to include the option and keep the rest of the lastMatch/allOwners
logic unchanged.
- Around line 3-4: The workflow's pull_request_review trigger only listens for
"submitted" events, so dismissed reviews won't re-run the check; update the
pull_request_review.types array to include "dismissed" in addition to
"submitted" (i.e., modify the pull_request_review -> types setting) so the
workflow is retriggered when a review is dismissed.
- Around line 2-6: The workflow uses the pull_request event which provides a
read-only GITHUB_TOKEN for forked PRs, causing createCommitStatus to fail on
external contributions; switch the trigger from pull_request to
pull_request_target so the job runs in the base-branch context with a writable
token, keep the existing checkout of the base SHA (the step that checks out the
base ref), and ensure no steps execute untrusted code from the fork—only call
GitHub API functions like createCommitStatus and read base-branch files so the
token remains safe to use.
- Around line 35-41: The catch currently swallows FS errors when reading
'.github/CODEOWNERS', causing misleading "update CODEOWNERS!" messages and also
the comment filter uses startsWith on the untrimmed line; update the
.readFileSync block to (1) change the filter to use trimmed lines: .filter(l =>
l.trim() && !l.trim().startsWith('#')).map(...) so indented comments are
ignored, and (2) in the catch, record/log the error (e.g., set a boolean like
codeownersReadError or call the existing logger) instead of silently swallowing
it so downstream logic that inspects codeowners/allOwners can produce a
distinct, accurate status message when the file couldn't be read versus when
CODEOWNERS truly has no owners.
- Around line 36-38: The parsing drops patterns when lines have leading
whitespace because you split on the raw line; change the pipeline to trim each
line first and use that trimmed string for both filtering and splitting (e.g.
replace usages of l in the .filter and .map callbacks with a trimmed variable),
so the .filter uses t && !t.startsWith('#') and the .map splits t (const
[pattern, ...owners] = t.split(/\s+/)) before returning { pattern, owners } —
update the callbacks around .split('\n'), .filter(...), and .map(...)
accordingly.
- Around line 81-89: The commit status description can be silently truncated by
GitHub and may show a double emoji when display already starts with one; before
calling github.rest.repos.createCommitStatus (the createCommitStatus call that
sets description using approved, approver, and display), build a
sanitizedDescription: trim/normalize display to a short form, strip any leading
emoji or leading "❌" when approved is false to avoid doubling, then truncate the
final description to 140 chars (append ellipsis if trimmed). Use that
sanitizedDescription in the description field instead of constructing it inline.

Comment on lines +2 to +6
on:
pull_request_review:
types: [submitted]
pull_request:
types: [opened, synchronize]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Workflow cannot set statuses on fork PRs.

pull_request events from forks receive a read-only GITHUB_TOKEN, so createCommitStatus will fail for external contributions. Since this workflow already checks out the base SHA and doesn't execute fork code, switching to pull_request_target would grant the needed write token safely:

Proposed fix
 on:
   pull_request_review:
     types: [submitted]
-  pull_request:
+  pull_request_target:
     types: [opened, synchronize]

Note: pull_request_target runs in the base-branch context, which is already the intent here (Line 18 checks out the base SHA). The script only calls GitHub APIs and reads base-branch files — no fork-supplied code is executed — so this is safe.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
on:
pull_request_review:
types: [submitted]
pull_request:
types: [opened, synchronize]
on:
pull_request_review:
types: [submitted]
pull_request_target:
types: [opened, synchronize]
🤖 Prompt for AI Agents
In @.github/workflows/codeowner-approval-status.yml around lines 2 - 6, The
workflow uses the pull_request event which provides a read-only GITHUB_TOKEN for
forked PRs, causing createCommitStatus to fail on external contributions; switch
the trigger from pull_request to pull_request_target so the job runs in the
base-branch context with a writable token, keep the existing checkout of the
base SHA (the step that checks out the base ref), and ensure no steps execute
untrusted code from the fork—only call GitHub API functions like
createCommitStatus and read base-branch files so the token remains safe to use.

Comment on lines +3 to +4
pull_request_review:
types: [submitted]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Dismissed reviews won't re-trigger this check.

If an approving review is dismissed, the commit status will remain success until another event (like a new push) re-triggers the workflow. Add dismissed to the pull_request_review types to handle this case.

Proposed fix
   pull_request_review:
-    types: [submitted]
+    types: [submitted, dismissed]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
pull_request_review:
types: [submitted]
pull_request_review:
types: [submitted, dismissed]
🤖 Prompt for AI Agents
In @.github/workflows/codeowner-approval-status.yml around lines 3 - 4, The
workflow's pull_request_review trigger only listens for "submitted" events, so
dismissed reviews won't re-run the check; update the pull_request_review.types
array to include "dismissed" in addition to "submitted" (i.e., modify the
pull_request_review -> types setting) so the workflow is retriggered when a
review is dismissed.

Comment on lines +8 to +10
permissions:
pull-requests: read
statuses: write
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Missing contents: read permission — actions/checkout will fail.

When the permissions key is explicitly specified, any unmentioned permission defaults to none. The checkout step needs contents: read to clone the repo and read .github/CODEOWNERS.

Proposed fix
 permissions:
+  contents: read
   pull-requests: read
   statuses: write
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
permissions:
pull-requests: read
statuses: write
permissions:
contents: read
pull-requests: read
statuses: write
🤖 Prompt for AI Agents
In @.github/workflows/codeowner-approval-status.yml around lines 8 - 10, The
workflow's explicit permissions block omits contents, causing actions/checkout
to fail; update the permissions mapping by adding "contents: read" alongside the
existing "pull-requests: read" and "statuses: write" entries so the
actions/checkout step can clone the repo and read .github/CODEOWNERS (adjust the
top-level permissions block where "pull-requests" and "statuses" are defined).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

why do i need to clone repo? codeowners = fs.readFileSync('.github/CODEOWNERS', 'utf8') seems to be working in an experimental repo

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# View the complete workflow file to see all steps
cat .github/workflows/codeowner-approval-status.yml

Length of output: 3695


@aleozlx You're right that fs.readFileSync works to read the file—but only after the actions/checkout@v4 step successfully clones the repository at line 16. The issue is that the checkout action itself requires contents: read permission to clone the repo in the first place.

When you explicitly define a permissions: block in a workflow, GitHub Actions sets all unmentioned permissions to none by default. So your current config gives:

  • contents: none ← blocks actions/checkout
  • pull-requests: read
  • statuses: write

If your experimental repo worked without contents: read, it likely either:

  1. Didn't have an explicit permissions: block (so default permissions applied), or
  2. Hasn't actually run with this exact configuration yet

You can verify this by checking the Actions logs—without contents: read, the checkout step will fail with a permission error like:

Error: Resource not accessible by integration

TL;DR: contents: read is needed for line 16's actions/checkout@v4 to succeed, which makes the file available for fs.readFileSync at line 38.

Comment on lines +35 to +41
try {
codeowners = fs.readFileSync('.github/CODEOWNERS', 'utf8')
.split('\n').filter(l => l.trim() && !l.startsWith('#'))
.map(l => { const [pattern, ...owners] = l.split(/\s+/); return { pattern, owners }; });
} catch (e) {
// Do nothing, but let the same logic handle the error later.
}
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Silent catch produces a misleading failure message when CODEOWNERS is missing.

If the file doesn't exist or can't be read, allOwners stays empty and the status shows "no qualified approver. update CODEOWNERS!" — which is misleading (the file may exist but just wasn't found at the checked-out ref). Consider logging the error or setting a distinct description.

Also, !l.startsWith('#') doesn't handle indented comments (e.g., # comment). Using l.trim() for the comment check would be safer:

Proposed fix
             try {
               codeowners = fs.readFileSync('.github/CODEOWNERS', 'utf8')
-                .split('\n').filter(l => l.trim() && !l.startsWith('#'))
+                .split('\n').filter(l => l.trim() && !l.trim().startsWith('#'))
                 .map(l => { const [pattern, ...owners] = l.split(/\s+/); return { pattern, owners }; });
             } catch (e) {
-              // Do nothing, but let the same logic handle the error later.
+              core.warning(`Could not read .github/CODEOWNERS: ${e.message}`);
             }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
try {
codeowners = fs.readFileSync('.github/CODEOWNERS', 'utf8')
.split('\n').filter(l => l.trim() && !l.startsWith('#'))
.map(l => { const [pattern, ...owners] = l.split(/\s+/); return { pattern, owners }; });
} catch (e) {
// Do nothing, but let the same logic handle the error later.
}
try {
codeowners = fs.readFileSync('.github/CODEOWNERS', 'utf8')
.split('\n').filter(l => l.trim() && !l.trim().startsWith('#'))
.map(l => { const [pattern, ...owners] = l.split(/\s+/); return { pattern, owners }; });
} catch (e) {
core.warning(`Could not read .github/CODEOWNERS: ${e.message}`);
}
🤖 Prompt for AI Agents
In @.github/workflows/codeowner-approval-status.yml around lines 35 - 41, The
catch currently swallows FS errors when reading '.github/CODEOWNERS', causing
misleading "update CODEOWNERS!" messages and also the comment filter uses
startsWith on the untrimmed line; update the .readFileSync block to (1) change
the filter to use trimmed lines: .filter(l => l.trim() &&
!l.trim().startsWith('#')).map(...) so indented comments are ignored, and (2) in
the catch, record/log the error (e.g., set a boolean like codeownersReadError or
call the existing logger) instead of silently swallowing it so downstream logic
that inspects codeowners/allOwners can produce a distinct, accurate status
message when the file couldn't be read versus when CODEOWNERS truly has no
owners.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

intentional for simplicity. The maintainer will check the file if it doesn't exist etc and discover the problem by debugging

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@aleozlx, understood — I'll defer to your judgment on keeping the error handling simple for this workflow. If the file is missing or unreadable, the resulting empty owner set will naturally trigger a failure status, signaling that something needs attention.

Comment on lines +36 to +38
codeowners = fs.readFileSync('.github/CODEOWNERS', 'utf8')
.split('\n').filter(l => l.trim() && !l.startsWith('#'))
.map(l => { const [pattern, ...owners] = l.split(/\s+/); return { pattern, owners }; });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Lines with leading whitespace will be misparsed.

l.split(/\s+/) on a line like *.py@owner`` produces ['', '*.py', '@owner'], so `pattern` becomes `''` (silently dropping the rule). Trim each line before splitting:

Proposed fix
             codeowners = fs.readFileSync('.github/CODEOWNERS', 'utf8')
-                .split('\n').filter(l => l.trim() && !l.startsWith('#'))
-                .map(l => { const [pattern, ...owners] = l.split(/\s+/); return { pattern, owners }; });
+                .split('\n').map(l => l.trim()).filter(l => l && !l.startsWith('#'))
+                .map(l => { const [pattern, ...owners] = l.split(/\s+/); return { pattern, owners }; });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
codeowners = fs.readFileSync('.github/CODEOWNERS', 'utf8')
.split('\n').filter(l => l.trim() && !l.startsWith('#'))
.map(l => { const [pattern, ...owners] = l.split(/\s+/); return { pattern, owners }; });
codeowners = fs.readFileSync('.github/CODEOWNERS', 'utf8')
.split('\n').map(l => l.trim()).filter(l => l && !l.startsWith('#'))
.map(l => { const [pattern, ...owners] = l.split(/\s+/); return { pattern, owners }; });
🤖 Prompt for AI Agents
In @.github/workflows/codeowner-approval-status.yml around lines 36 - 38, The
parsing drops patterns when lines have leading whitespace because you split on
the raw line; change the pipeline to trim each line first and use that trimmed
string for both filtering and splitting (e.g. replace usages of l in the .filter
and .map callbacks with a trimmed variable), so the .filter uses t &&
!t.startsWith('#') and the .map splits t (const [pattern, ...owners] =
t.split(/\s+/)) before returning { pattern, owners } — update the callbacks
around .split('\n'), .filter(...), and .map(...) accordingly.

Comment on lines +45 to +53
for (const f of files) {
// Each file's code owner is determined by the last matched rule
// This a native CODEOWNERS file behavior
let lastMatch = null;
for (const rule of codeowners) {
if (minimatch(f.filename, rule.pattern)) lastMatch = rule;
}
if (lastMatch) lastMatch.owners.forEach(o => allOwners.add(o.replace('@', '')));
}
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

minimatch glob semantics differ from CODEOWNERS pattern semantics.

CODEOWNERS patterns follow .gitignore-style rules: a pattern without a / (e.g., *.py) matches anywhere in the path tree (foo/bar.py). By default, minimatch('foo/bar.py', '*.py') returns false because it doesn't match across directory separators. You likely need { matchBase: true } (or convert patterns yourself) to replicate the expected behavior.

Proposed fix
-              if (minimatch(f.filename, rule.pattern)) lastMatch = rule;
+              if (minimatch(f.filename, rule.pattern, { matchBase: true })) lastMatch = rule;
🤖 Prompt for AI Agents
In @.github/workflows/codeowner-approval-status.yml around lines 45 - 53, The
CODEOWNERS matching loop uses minimatch(f.filename, rule.pattern) which does not
emulate CODEOWNERS/.gitignore semantics for patterns without slashes; update the
match call to pass the option { matchBase: true } (or normalize rule.patterns to
'**/pattern' before matching) so patterns like '*.py' match files in
subdirectories; modify the inner loop where minimatch is invoked (referencing
variables minimatch, f.filename, rule.pattern, lastMatch) to include the option
and keep the rest of the lastMatch/allOwners logic unchanged.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

apply the change

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@aleozlx, I'll apply the suggested fix to ensure CODEOWNERS patterns match correctly.

Task: Add matchBase option to minimatch call

  • Update the minimatch call in .github/workflows/codeowner-approval-status.yml to include { matchBase: true } option
  • This ensures patterns like *.py match files in subdirectories, consistent with CODEOWNERS/.gitignore semantics

💡 For code changes across multiple files, use @coderabbitai plan in an issue comment

Comment on lines +81 to +89
await github.rest.repos.createCommitStatus({
owner: context.repo.owner, repo: context.repo.repo,
sha: context.payload.pull_request.head.sha,
state: approved ? 'success' : 'failure',
context: 'codeowner-approval',
description: approved
? `Approved by ${approver}`
: `❌ need approval: ${display}`,
});
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

GitHub commit-status description has a 140-character limit.

If there are many owners with long usernames, description may be silently truncated by GitHub. Also, when hasQualifiedApprover is false, the description becomes "❌ need approval: ❌ no qualified approver…" — the doubled emoji is a minor visual glitch.

Consider truncating explicitly and keeping the display path simpler:

Proposed fix
+            const descriptionRaw = approved
+              ? `Approved by ${approver}`
+              : hasQualifiedApprover
+                ? `Need approval from: ${owners_array.length <= 3 ? owners_array.map(o => `@${o}`).join(', ') : `@${owners_array.slice(0, 3).join(', @')} +${owners_array.length - 3} more`}`
+                : 'No qualified approver found — update CODEOWNERS';
+            const description = descriptionRaw.slice(0, 140);
+
             await github.rest.repos.createCommitStatus({
               owner: context.repo.owner, repo: context.repo.repo,
               sha: context.payload.pull_request.head.sha,
               state: approved ? 'success' : 'failure',
               context: 'codeowner-approval',
-              description: approved
-                ? `Approved by ${approver}`
-                : `❌ need approval: ${display}`,
+              description,
             });
🤖 Prompt for AI Agents
In @.github/workflows/codeowner-approval-status.yml around lines 81 - 89, The
commit status description can be silently truncated by GitHub and may show a
double emoji when display already starts with one; before calling
github.rest.repos.createCommitStatus (the createCommitStatus call that sets
description using approved, approver, and display), build a
sanitizedDescription: trim/normalize display to a short form, strip any leading
emoji or leading "❌" when approved is false to avoid doubling, then truncate the
final description to 140 chars (append ellipsis if trimmed). Use that
sanitizedDescription in the description field instead of constructing it inline.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

i already truncate this to top-3 in the code

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@aleozlx You're right that limiting to top-3 owners significantly reduces the risk!

Two minor edge cases remain:

  1. 140-char limit: Three long usernames could still exceed it (GitHub usernames can be up to 39 chars). For example: "❌ need approval: @very-long-username-here, @another-long-one, @third-long-name +2 more" could approach or exceed 140 characters.
  2. Double emoji: When there are no qualified approvers, display is set to "❌ no qualified approver...", so the final description becomes "❌ need approval: ❌ no qualified approver...".

These are quite rare edge cases, so if you're comfortable with the current approach, it's reasonable to keep it as is. If you'd like to be extra defensive, adding an explicit .slice(0, 140) and tweaking the display logic to avoid the double emoji would eliminate these edge cases entirely.


🧠 Learnings used
Learnt from: yongwww
Repo: flashinfer-ai/flashinfer PR: 2500
File: .github/workflows/pr-test-runner.yml:48-50
Timestamp: 2026-02-06T06:23:23.483Z
Learning: In GitHub Actions workflows with check runs, check runs are per-SHA. When a new commit is pushed to a PR, old check runs associated with previous commit SHAs are automatically hidden from the PR status panel and only the latest commit's check runs are displayed. Therefore, orphaned in-progress check runs from cancelled workflows do not clutter the PR status or affect the workflow.

? `Approved by ${approver}`
: `❌ need approval: ${display}`,
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix trailing whitespace — CI is failing.

The pipeline reports a trailing-whitespace pre-commit hook failure on this line. Remove the trailing whitespace to pass CI.

🤖 Prompt for AI Agents
In @.github/workflows/codeowner-approval-status.yml at line 90, Remove the
trailing whitespace from the reported line in the workflow file so the
pre-commit `trailing-whitespace` hook passes; open
.github/workflows/codeowner-approval-status.yml, locate the
blank/whitespace-only characters at the end of the failing line and delete them
(ensure the line ends with no spaces or tabs), then save and commit the change.

Copy link
Copy Markdown
Collaborator

@yzh119 yzh119 left a comment

Choose a reason for hiding this comment

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

I read through this PR (with the help of claude) and it looks good to me. Thanks for working on this!

@yzh119
Copy link
Copy Markdown
Collaborator

yzh119 commented Feb 19, 2026

@aleozlx do you know how to get around "❌ need approval: ❌ no qualified approver. update CODEOWNERS!"?

@aleozlx
Copy link
Copy Markdown
Collaborator Author

aleozlx commented Feb 20, 2026

@aleozlx do you know how to get around "❌ need approval: ❌ no qualified approver. update CODEOWNERS!"?

to unblock such problem reported once this goes into effect

  • either someone like you who may be able to force merge anyway; OR
  • or we'd have to make CODEOWNERS cover it (an on demand update etc). which could be an annoyance if the whole PR is adding one new file... and doesn't even hit any mapped team folders.
    • but i think it's possibly an edge case (such as this PR itself) and if so the force merge is the more simplistic solution without overcomplicating it. i'm more worried more code will lead to more bugs here...

or do you mean the double emoji? yes i could remove one of them, but i thought it makes it more obvious where the blocker is and doesn't hurt. so i considered that a feature rather than bug

@aleozlx
Copy link
Copy Markdown
Collaborator Author

aleozlx commented Mar 18, 2026

a potentially simpler plan is mentioned above, closing for now

@aleozlx aleozlx closed this Mar 18, 2026
@zhyncs zhyncs deleted the aleozlx/coder_owner branch March 21, 2026 20:39
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.

2 participants