ci: auto pr assign by CODEOWNERS#248
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| permissions: | ||
| pull-requests: write |
There was a problem hiding this comment.
issues: write permission
The workflow calls github.rest.issues.addAssignees (line 132) to set assignees on the PR, but the permissions block only grants pull-requests: write. Assigning users to an issue/PR via the GitHub REST API requires issues: write — without it, the API call will fail with a "Resource not accessible by integration" error, and the core functionality of this workflow (assigning code owners as reviewers) will silently fail.
| permissions: | |
| pull-requests: write | |
| permissions: | |
| pull-requests: write | |
| issues: write |
| matchedPattern = pattern; | ||
| } else if (pattern.endsWith('/')) { | ||
| const dir = pattern.replace(/^\//, '').replace(/\/$/, ''); | ||
| if (file.startsWith(dir + '/') || file.startsWith(dir)) { |
There was a problem hiding this comment.
The condition || file.startsWith(dir) (without a trailing /) causes false-positive matches. For example, a hypothetical file src/db_utils.cpp or src/binding_test.go at the repository root would incorrectly match the /src/db/ or /src/binding/ CODEOWNERS patterns because "src/db_utils.cpp".startsWith("src/db") evaluates to true.
Only the file.startsWith(dir + '/') check is correct for directory patterns. Remove the bare file.startsWith(dir) branch:
| if (file.startsWith(dir + '/') || file.startsWith(dir)) { | |
| if (file.startsWith(dir + '/')) { |
| on: | ||
| pull_request_target: | ||
| types: [opened, reopened, ready_for_review] |
There was a problem hiding this comment.
pull_request_target + checkout requires caution
The workflow uses the pull_request_target trigger, which runs with write permissions against the base repository. Although the actions/checkout@v4 step correctly checks out the base branch (not the PR head), the pulls.listFiles and issues.addAssignees calls operate on data from the PR author's fork. If a malicious contributor crafts a file with a specially crafted filename (e.g., containing shell metacharacters), the logging statements (console.log) in the script could be misused. This is low-risk here since no eval or shell execution is performed on filenames, but it's worth being aware of the pull_request_target attack surface and ensuring no future changes execute arbitrary content from the PR.
| let matchedOwner = null; | ||
| let matchedPattern = null; | ||
| for (const rule of codeowners) { | ||
| const pattern = rule.pattern; | ||
| if (pattern === '*') { | ||
| matchedOwner = rule.owners[0]; | ||
| matchedPattern = pattern; | ||
| } else if (pattern.endsWith('/')) { | ||
| const dir = pattern.replace(/^\//, '').replace(/\/$/, ''); | ||
| if (file.startsWith(dir + '/') || file.startsWith(dir)) { | ||
| matchedOwner = rule.owners[0]; | ||
| matchedPattern = pattern; | ||
| } | ||
| } else if (file === pattern || file === pattern.replace(/^\//, '')) { | ||
| matchedOwner = rule.owners[0]; | ||
| matchedPattern = pattern; | ||
| } |
There was a problem hiding this comment.
The script always uses rule.owners[0], ignoring any additional owners listed on the same CODEOWNERS line. While the current CODEOWNERS file only has one owner per pattern, this is a fragile assumption. If a pattern is later updated to have multiple owners (e.g., /src/core/ @richyreachy @feihongxu0824), only the first will ever be assigned, which may lead to missed review coverage.
Consider using rule.owners (the full array) when building the matchedOwners set:
// Instead of: matchedOwner = rule.owners[0];
rule.owners.forEach(o => matchedOwners.add(o));
| const { data: files } = await github.rest.pulls.listFiles({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: prNumber | ||
| }); |
There was a problem hiding this comment.
listFiles not paginated — files beyond page 1 will be silently skipped
github.rest.pulls.listFiles returns at most 30 files per page by default. For any PR that touches more than 30 files, only the first 30 are fetched here, so CODEOWNERS owners for the remaining files are never computed and never assigned. Use github.paginate to consume all pages:
const files = await github.paginate(github.rest.pulls.listFiles, {
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: prNumber,
per_page: 100
});
const changedFiles = files.map(f => f.filename);1 participant
Greptile Summary
This PR introduces an automated PR assignment system for the
alibaba/zvecrepository, consisting of two workflows (auto-assign.ymlandcommunity-pr-handler.yml), a CODEOWNERS file mapping directory paths to maintainers, and anauto-assign-actionconfiguration. Both workflows use thepull_request_targettrigger — correctly checking out the base branch rather than the PR head to avoid supply-chain risks.Key issues found:
auto-assign.ymlandcommunity-pr-handler.ymlboth callPOST /repos/.../issues/{number}/assignees, which requiresissues: write. Neither workflow grants that permission (onlypull-requests: writeis declared), so all assignee API calls will return "Resource not accessible by integration".community-pr-handler.yml—github.rest.pulls.listFilesreturns at most 30 files per page by default. PRs touching more than 30 files will have their extra files silently ignored, causing their CODEOWNERS owners to never be assigned..gitmodulesCODEOWNERS entry is unanchored — Missing the leading/means it would match any.gitmodulesfile at any depth in the repository, not just the root-level one.file.startsWith(dir)without a trailing/, which can produce false-positive owner matches for similarly-named paths.rule.owners[0]is used, ignoring additional owners on the same CODEOWNERS line.Confidence Score: 2/5
auto-assign.ymlandcommunity-pr-handler.yml) both lack theissues: writepermission required to call the assignees API, meaning the core feature (auto-assigning reviewers) will silently fail entirely. Additionally, thelistFilescall without pagination will miss changed files in any PR touching more than 30 files. These are functional blockers, not just style issues..github/workflows/auto-assign.yml(missingissues: write) and.github/workflows/community-pr-handler.yml(missingissues: write+ no pagination).Important Files Changed
issues: write, overly broadstartsWithmatching, only first owner used) plus a new pagination bug that causes files beyond the first 30 to be silently ignored.kentaro-m/auto-assign-action; missingissues: writepermission will cause the action to fail at runtime..gitmodules, which needs a/.gitmodulesprefix to avoid matching nested files.kentaro-m/auto-assign-action; correctly setsaddAssignees: author, disables reviewer assignment (delegated to CODEOWNERS), and skips draft/WIP PRs.Sequence Diagram
sequenceDiagram participant Author as PR Author (fork) participant GH as GitHub Events participant AutoAssign as auto-assign.yml<br/>(pull_request_target) participant CommunityHandler as community-pr-handler.yml<br/>(pull_request_target) participant API as GitHub REST API Author->>GH: Opens / reopens PR GH->>AutoAssign: Trigger (opened/reopened/ready_for_review) AutoAssign->>AutoAssign: Checkout base branch (sparse: auto-assign-config.yml) AutoAssign->>API: POST /issues/{pr}/assignees<br/>(assignee = PR author)<br/>⚠️ needs issues:write GH->>CommunityHandler: Trigger (opened/reopened/ready_for_review) CommunityHandler->>API: Check author_association API-->>CommunityHandler: OWNER / MEMBER / COLLABORATOR / CONTRIBUTOR ... alt Community contributor CommunityHandler->>CommunityHandler: Checkout base branch (sparse: CODEOWNERS) CommunityHandler->>API: GET /pulls/{pr}/files (page 1 only ⚠️ no pagination) API-->>CommunityHandler: Changed files (max 30) CommunityHandler->>CommunityHandler: Parse CODEOWNERS & match owners<br/>(last-rule-wins per file) CommunityHandler->>API: POST /issues/{pr}/assignees<br/>(matched CODEOWNERS owners)<br/>⚠️ needs issues:write else Maintainer CommunityHandler->>CommunityHandler: Skip — no action needed endLast reviewed commit: "fix"