add sync-pr-labels #6
Conversation
📝 WalkthroughWalkthroughA new GitHub Actions workflow named "Sync PR Labels" has been introduced at Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.github/workflows/sync-pr-labels.yml (2)
19-22: Remove unnecessaryfetch-depth: 0.The workflow doesn't analyze git history, so fetching the entire repository history is unnecessary. This slows down workflow execution without providing any benefit.
🔎 Proposed fix
- name: Checkout repository uses: actions/checkout@v4 - with: - fetch-depth: 0
66-190: Consider adding label existence checks.The workflow assumes all labels exist in the repository. If a label doesn't exist, the
addLabelsAPI call will fail. Consider either:
- Creating labels automatically if they don't exist, or
- Filtering to only apply existing labels
This prevents workflow failures when labels are not pre-configured.
🔎 Example: Filter to existing labels
Add a helper step that fetches all repository labels, then filter the labels to apply:
// Get all available labels in the repository const { data: repoLabels } = await github.rest.issues.listLabelsForRepo({ owner: context.repo.owner, repo: context.repo.repo }); const availableLabels = repoLabels.map(l => l.name); // Filter labels to only those that exist const labelsToApply = fileLabels.filter(label => availableLabels.includes(label)); if (labelsToApply.length > 0) { await github.rest.issues.addLabels({ owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, labels: labelsToApply }); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/sync-pr-labels.yml
🔇 Additional comments (4)
.github/workflows/sync-pr-labels.yml (4)
66-100: LGTM!The issue-based labeling logic correctly fetches labels from the linked issue and handles both string and object label formats. Error handling is appropriate.
102-118: LGTM!The fallback labeling for PRs without linked issues is correctly implemented.
258-279: LGTM!The summary step provides useful observability by logging all applied labels.
193-227: Configure EXTERNAL_LABELLER_TOKEN with org read permissions or organization membership checks will silently fail.The
orgs.checkMembershipForUserAPI call (line 215) requires OAuth scoperead:orgor equivalent org-level permissions, which the defaultGITHUB_TOKENcannot have. WithoutEXTERNAL_LABELLER_TOKENproperly configured as a repository secret with the required scopes, this call will fail and the catch block (line 220) will treat all users as non-members, incorrectly labeling organization members as external contributors or first-time contributors.
| on: | ||
| pull_request: | ||
| types: [opened, reopened, synchronize, edited] | ||
| pull_request_target: | ||
| types: [opened, reopened, synchronize, edited] | ||
|
|
||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| issues: read | ||
|
|
||
| jobs: | ||
| sync-labels: | ||
| if: ${{ github.repository_owner == 'AOSSIE-Org' }} | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
Critical: Remove pull_request_target to prevent security vulnerability.
Using pull_request_target with repository checkout creates a serious security risk. The pull_request_target trigger runs workflows from the base branch with write permissions, and checking out the code (line 19-22) allows malicious code from forked PRs to execute with elevated privileges.
Since this workflow only needs to read PR metadata and apply labels (no untrusted code execution), use only the pull_request trigger. The pull-requests: write permission is sufficient for labeling operations.
🔎 Recommended fix
on:
pull_request:
types: [opened, reopened, synchronize, edited]
- pull_request_target:
- types: [opened, reopened, synchronize, edited]📝 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.
| on: | |
| pull_request: | |
| types: [opened, reopened, synchronize, edited] | |
| pull_request_target: | |
| types: [opened, reopened, synchronize, edited] | |
| permissions: | |
| contents: read | |
| pull-requests: write | |
| issues: read | |
| jobs: | |
| sync-labels: | |
| if: ${{ github.repository_owner == 'AOSSIE-Org' }} | |
| runs-on: ubuntu-latest | |
| steps: | |
| - name: Checkout repository | |
| uses: actions/checkout@v4 | |
| with: | |
| fetch-depth: 0 | |
| on: | |
| pull_request: | |
| types: [opened, reopened, synchronize, edited] | |
| permissions: | |
| contents: read | |
| pull-requests: write | |
| issues: read | |
| jobs: | |
| sync-labels: | |
| if: ${{ github.repository_owner == 'AOSSIE-Org' }} | |
| runs-on: ubuntu-latest | |
| steps: | |
| - name: Checkout repository | |
| uses: actions/checkout@v4 | |
| with: | |
| fetch-depth: 0 |
🤖 Prompt for AI Agents
.github/workflows/sync-pr-labels.yml lines 3-22: the workflow uses the
pull_request_target trigger which combined with actions/checkout is a security
risk; remove the entire pull_request_target block so only pull_request remains,
keep the types array (opened, reopened, synchronize, edited) on pull_request,
and ensure permissions remain minimal (contents: read and pull-requests: write)
for labeling operations; do not check out PR code in this workflow.
| script: | | ||
| const prBody = context.payload.pull_request.body || ''; | ||
|
|
||
| // Match patterns: Fixes #123, Closes #123, Resolves #123, etc. | ||
| const issuePatterns = [ | ||
| /(?:fix|fixes|fixed|close|closes|closed|resolve|resolves|resolved)\s+#(\d+)/gi, | ||
| /#(\d+)/g | ||
| ]; | ||
|
|
||
| let issueNumber = null; | ||
| for (const pattern of issuePatterns) { | ||
| const match = prBody.match(pattern); | ||
| if (match) { | ||
| const numbers = match.map(m => m.match(/\d+/)[0]); | ||
| issueNumber = numbers[0]; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| core.setOutput('issue_number', issueNumber || ''); | ||
| return issueNumber; |
There was a problem hiding this comment.
Fix overly broad issue number pattern.
The regex pattern /#(\d+)/g on line 50 will match ANY #number in the PR body, causing false positives. For example, it would incorrectly extract issue numbers from version tags, commit references, or arbitrary text containing # followed by digits.
The first pattern already handles standard linking keywords. If you want to support bare #123 references, ensure they appear in a linking context (e.g., at the start of a line or after whitespace).
🔎 Recommended fix
// Match patterns: Fixes #123, Closes #123, Resolves #123, etc.
const issuePatterns = [
/(?:fix|fixes|fixed|close|closes|closed|resolve|resolves|resolved)\s+#(\d+)/gi,
- /#(\d+)/g
+ /(?:^|\s)#(\d+)(?:\s|$)/gm
];This revised pattern matches #number only when it appears at the start of a line or after whitespace, and is followed by whitespace or end of line.
📝 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.
| script: | | |
| const prBody = context.payload.pull_request.body || ''; | |
| // Match patterns: Fixes #123, Closes #123, Resolves #123, etc. | |
| const issuePatterns = [ | |
| /(?:fix|fixes|fixed|close|closes|closed|resolve|resolves|resolved)\s+#(\d+)/gi, | |
| /#(\d+)/g | |
| ]; | |
| let issueNumber = null; | |
| for (const pattern of issuePatterns) { | |
| const match = prBody.match(pattern); | |
| if (match) { | |
| const numbers = match.map(m => m.match(/\d+/)[0]); | |
| issueNumber = numbers[0]; | |
| break; | |
| } | |
| } | |
| core.setOutput('issue_number', issueNumber || ''); | |
| return issueNumber; | |
| script: | | |
| const prBody = context.payload.pull_request.body || ''; | |
| // Match patterns: Fixes #123, Closes #123, Resolves #123, etc. | |
| const issuePatterns = [ | |
| /(?:fix|fixes|fixed|close|closes|closed|resolve|resolves|resolved)\s+#(\d+)/gi, | |
| /(?:^|\s)#(\d+)(?:\s|$)/gm | |
| ]; | |
| let issueNumber = null; | |
| for (const pattern of issuePatterns) { | |
| const match = prBody.match(pattern); | |
| if (match) { | |
| const numbers = match.map(m => m.match(/\d+/)[0]); | |
| issueNumber = numbers[0]; | |
| break; | |
| } | |
| } | |
| core.setOutput('issue_number', issueNumber || ''); | |
| return issueNumber; |
🤖 Prompt for AI Agents
In .github/workflows/sync-pr-labels.yml around lines 44 to 64 the second regex
/#(\d+)/g is too broad and matches any “#number” (versions, refs) causing false
positives; replace it with a stricter pattern that only matches bare issue refs
at start-of-line or after whitespace and followed by whitespace or end-of-line
(e.g. use a regex like (?:^|\s)#(\d+)(?=\s|$)), keep the loop logic the same and
use the capture group to set issueNumber so only true bare #123 references are
picked up.
| // Check each file against label mappings | ||
| for (const file of changedFiles) { | ||
| for (const [label, patterns] of Object.entries(labelMappings)) { | ||
| for (const pattern of patterns) { | ||
| if (file.includes(pattern) || file.endsWith(pattern)) { | ||
| if (!fileLabels.includes(label)) { | ||
| fileLabels.push(label); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix file pattern matching to prevent false positives.
The pattern matching logic on line 170 uses file.includes(pattern), which will cause false positives:
'test/'matches'my-test-utils.js'and'contest/foo.js''.test.'matches'latest.release.js'- Directory patterns don't distinguish between path segments
This could apply incorrect labels to PRs.
🔎 Recommended fix
// Check each file against label mappings
for (const file of changedFiles) {
for (const [label, patterns] of Object.entries(labelMappings)) {
for (const pattern of patterns) {
- if (file.includes(pattern) || file.endsWith(pattern)) {
+ let matches = false;
+ if (pattern.endsWith('/')) {
+ // Directory pattern
+ matches = file.startsWith(pattern) || file.includes(`/${pattern}`);
+ } else if (pattern.startsWith('.') && !pattern.includes('/')) {
+ // Extension pattern
+ matches = file.endsWith(pattern);
+ } else {
+ // Filename or substring pattern
+ matches = file.includes(pattern);
+ }
+
+ if (matches) {
if (!fileLabels.includes(label)) {
fileLabels.push(label);
}
}
}
}
}📝 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.
| // Check each file against label mappings | |
| for (const file of changedFiles) { | |
| for (const [label, patterns] of Object.entries(labelMappings)) { | |
| for (const pattern of patterns) { | |
| if (file.includes(pattern) || file.endsWith(pattern)) { | |
| if (!fileLabels.includes(label)) { | |
| fileLabels.push(label); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| // Check each file against label mappings | |
| for (const file of changedFiles) { | |
| for (const [label, patterns] of Object.entries(labelMappings)) { | |
| for (const pattern of patterns) { | |
| let matches = false; | |
| if (pattern.endsWith('/')) { | |
| // Directory pattern | |
| matches = file.startsWith(pattern) || file.includes(`/${pattern}`); | |
| } else if (pattern.startsWith('.') && !pattern.includes('/')) { | |
| // Extension pattern | |
| matches = file.endsWith(pattern); | |
| } else { | |
| // Filename or substring pattern | |
| matches = file.includes(pattern); | |
| } | |
| if (matches) { | |
| if (!fileLabels.includes(label)) { | |
| fileLabels.push(label); | |
| } | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In .github/workflows/sync-pr-labels.yml around lines 166-177, the current file
matching uses file.includes(pattern) which creates false positives (e.g.,
'test/' matching 'my-test-utils.js'); change the logic to do precise matches:
treat patterns ending with '/' as directory prefixes (use normalized path and
file.split('/') checks or file.startsWith(normalizedDir)), treat patterns that
look like file extensions or exact filenames using file.endsWith(pattern) or
exact equality, and for complex globs add a proper glob matcher
(minimatch/micromatch) and use that instead of includes so only true
path/filename matches produce labels.
| const contributorLabels = []; | ||
|
|
||
| // Check if contributor is a member of the organization | ||
| try { | ||
| await github.rest.orgs.checkMembershipForUser({ | ||
| org: context.repo.owner, | ||
| username: prAuthor | ||
| }); | ||
| contributorLabels.push('member'); | ||
| } catch (error) { | ||
| // Not a member | ||
| if (commits.data.length <= 1) { | ||
| contributorLabels.push('first-time-contributor'); | ||
| } else { | ||
| contributorLabels.push('external-contributor'); | ||
| } | ||
| } | ||
|
|
||
| // Check if PR author is a collaborator | ||
| try { | ||
| const permissionLevel = await github.rest.repos.getCollaboratorPermissionLevel({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| username: prAuthor | ||
| }); | ||
|
|
||
| if (permissionLevel.data.permission === 'admin' || permissionLevel.data.permission === 'maintain') { | ||
| contributorLabels.push('maintainer'); | ||
| } | ||
| } catch (error) { | ||
| console.log('Could not check collaborator status'); | ||
| } |
There was a problem hiding this comment.
Prevent overlapping contributor labels.
A user can receive both member and maintainer labels (lines 219 and 238), which is redundant. The logic should apply only one label based on the highest privilege level.
Consider this hierarchy: maintainer > member > external-contributor > first-time-contributor.
🔎 Recommended fix
const contributorLabels = [];
+ let contributorType = null;
// Check if contributor is a member of the organization
try {
await github.rest.orgs.checkMembershipForUser({
org: context.repo.owner,
username: prAuthor
});
- contributorLabels.push('member');
+ contributorType = 'member';
} catch (error) {
// Not a member
if (commits.data.length <= 1) {
- contributorLabels.push('first-time-contributor');
+ contributorType = 'first-time-contributor';
} else {
- contributorLabels.push('external-contributor');
+ contributorType = 'external-contributor';
}
}
// Check if PR author is a collaborator
try {
const permissionLevel = await github.rest.repos.getCollaboratorPermissionLevel({
owner: context.repo.owner,
repo: context.repo.repo,
username: prAuthor
});
if (permissionLevel.data.permission === 'admin' || permissionLevel.data.permission === 'maintain') {
- contributorLabels.push('maintainer');
+ contributorType = 'maintainer';
}
} catch (error) {
console.log('Could not check collaborator status');
}
- if (contributorLabels.length > 0) {
- console.log(`Applying contributor-based labels: ${contributorLabels.join(', ')}`);
+ if (contributorType) {
+ console.log(`Applying contributor-based label: ${contributorType}`);
await github.rest.issues.addLabels({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: prNumber,
- labels: contributorLabels
+ labels: [contributorType]
});
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/workflows/sync-pr-labels.yml around lines 211 to 242, the current
logic can push multiple overlapping contributor labels (e.g., both "member" and
"maintainer"); instead determine a single highest-priority label per the
hierarchy maintainer > member > external-contributor > first-time-contributor.
Change the flow to compute a single role variable (start as null), set it to
"member" only if membership check succeeds, then run the collaborator permission
check and if permission is "admin" or "maintain" override the role to
"maintainer"; if membership check failed, set role to "first-time-contributor"
or "external-contributor" based on commit count, and finally push only that
single role label (and avoid pushing duplicates or multiple contributor labels).
|
superseded: #11 |
2 participants
The workflow applies labels in the following strict order:
Issue-based labels (Step 1)
no-issue-linkedlabelFixes #123,Closes #123,Resolves #123, or#123File-based labels (Step 2)
documentation- .md, README, CONTRIBUTING filesfrontend- .html, .css, .jsx, .tsx filesbackend- .py, .java, .go, .rb filesjavascript- .js, .ts filespython- .py filesconfiguration- .yml, .json, .toml filesgithub-actions- workflow filesdependencies- package.json, requirements.txttests- test files and directoriesdocker- Dockerfile, docker-composeci-cd- CI/CD configuration filesContributor-based labels (Step 3)
first-time-contributor- For new contributorsexternal-contributor- For returning external contributorsmember- For organization membersmaintainer- For repository maintainersTo create EXTERNAL_LABELLER_TOKEN in step 3:
repoandread:orgscopesEXTERNAL_LABELLER_TOKENhow step 3 operate??
1.Reads PR number and author username from the GitHub event payload.
2.Calls GitHub API: is this user a member of the org?
3.Who are you?
├─ org member → member
├─ external + new → first-time-contributor(if commitCount ≤ 1)
├─ external + existing → external-contributor
└─ maintainer → maintainer (extra trust)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.