fix: Workaround org teams perm issue for approval purposes#2816
fix: Workaround org teams perm issue for approval purposes#2816aleozlx merged 3 commits intoflashinfer-ai:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces the auto-generated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a significant change to the project's Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 replaces the auto-generated CODEOWNERS file with a manually maintained version to address a permissions issue with GitHub organization teams. The change is a reasonable workaround. My review includes a couple of suggestions to improve the clarity and consistency of the new CODEOWNERS file, specifically regarding a confusing rule in the header and the sorting of owner lists.
.github/CODEOWNERS
Outdated
| # Rules: | ||
| # - Last matching pattern wins (GitHub semantics). | ||
| # - Catch-all ensures every PR gets at least one core-maintainer review. | ||
| # - Prefer @flashinfer-ai/<team> over individual users. |
There was a problem hiding this comment.
This rule contradicts the changes in this file, which replace team-based ownership with individual users as a workaround. This can be confusing for future contributors. To avoid confusion, consider rephrasing this to reflect it's an aspirational goal (e.g., # - Aspirational: Prefer @flashinfer-ai/<team> over individual users.) or removing it if the current state is not expected to change soon.
| tests/test_helpers/ @nvmbreughe @yzh119 | ||
| tests/utils/ @yzh119 @bkryu @nvmbreughe @kahyunnam @cyx-6 | ||
| # ── Catch-all: core maintainers review everything not covered below ── | ||
| * @yzh119 @sricketts @aleozlx @yongwww @cyx-6 @saltyminty @bkryu @yyihuang @kahyunnam @jimmyzho @nv-yunzheq |
There was a problem hiding this comment.
The list of code owners is not sorted alphabetically, which violates the rule on line 8 (- Keep entries sorted alphabetically within each section.). This applies to all owner lists in this file. Please sort them to improve maintainability.
* @aleozlx @bkryu @cyx-6 @jimmyzho @kahyunnam @nv-yunzheq @saltyminty @sricketts @yongwww @yyihuang @yzh119
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/CODEOWNERS (1)
10-11: Large catch-all reviewer list may cause notification fatigue.Having 11 individuals in the catch-all pattern means all 11 will be auto-requested for review on every PR touching uncovered paths. This can dilute ownership and create noise.
If intent is "any one of these can approve," consider:
- Creating a GitHub team for core maintainers once permissions are resolved
- Using CODEOWNERS purely for required reviewers and relying on branch protection for approval counts
This is a trade-off decision—flagging for awareness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/CODEOWNERS around lines 10 - 11, The CODEOWNERS catch-all entry uses a wildcard "*" with an explicit list of 11 usernames (e.g., `@yzh119` `@sricketts` `@aleozlx` `@yongwww` `@cyx-6` `@saltyminty` `@bkryu` `@yyihuang` `@kahyunnam` `@jimmyzho` `@nv-yunzheq`) which will request all of them on every uncovered PR; replace the long user list with a single GitHub team or narrow the pattern to only required files (or remove the wildcard and rely on branch protection) so that approvals can be provided by any one core maintainer instead of notifying all individuals—create/use a team (e.g., `@org/core-maintainers`) and update the "*" entry to reference that team or prune the list to only required reviewers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/CODEOWNERS:
- Around line 1-8: The header in CODEOWNERS references a non-existent
"CODEOWNER_MIGRATION_PLAN.md" and claims a preference for
"@flashinfer-ai/<team>" while the file actually lists individual usernames;
update the header comment to match the implementation or add the migration plan:
either create and commit a "CODEOWNER_MIGRATION_PLAN.md" that documents the
rationale and the preference for team handles, then ensure the header line
references it correctly, or edit the header text in CODEOWNERS to remove or
rephrase the migration-plan reference and change the sentence about preferring
team handles to accurately state that individual usernames are used (so the
header and the entries are consistent).
---
Nitpick comments:
In @.github/CODEOWNERS:
- Around line 10-11: The CODEOWNERS catch-all entry uses a wildcard "*" with an
explicit list of 11 usernames (e.g., `@yzh119` `@sricketts` `@aleozlx` `@yongwww` `@cyx-6`
`@saltyminty` `@bkryu` `@yyihuang` `@kahyunnam` `@jimmyzho` `@nv-yunzheq`) which will
request all of them on every uncovered PR; replace the long user list with a
single GitHub team or narrow the pattern to only required files (or remove the
wildcard and rely on branch protection) so that approvals can be provided by any
one core maintainer instead of notifying all individuals—create/use a team
(e.g., `@org/core-maintainers`) and update the "*" entry to reference that team or
prune the list to only required reviewers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a416116e-8610-4a08-9db4-d3fb51426fec
📒 Files selected for processing (2)
.github/CODEOWNERS.github/workflows/pr-test.yml
.github/CODEOWNERS
Outdated
| # Minimum commits threshold: 1 | ||
| # Manual overrides applied from overrides file | ||
| # CODEOWNERS — manually maintained, not auto-generated. | ||
| # See CODEOWNER_MIGRATION_PLAN.md for rationale. |
There was a problem hiding this comment.
proabbly remove this, since the CODEOWNER_MIGRATION_PLAN.md is not public available
There was a problem hiding this comment.
ah yes. i did remove it locally but forgot to push. it is now removed in the latest
4 participants
📌 Description
Workaround org teams perm issue for approval purposes
🔍 Related Issues
#2815
🚀 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
pre-commitby runningpip install pre-commit(or used your preferred method).pre-commit install.pre-commit run --all-filesand fixed any reported issues.🧪 Tests
unittest, etc.).Reviewer Notes
Summary by CodeRabbit