Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
327 changes: 327 additions & 0 deletions .github/workflows/validate-pr.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,327 @@
name: Validate PR

on:
pull_request_target:
types: [opened, reopened]

jobs:
validate-non-maintainer-pr:
name: Validate Non-Maintainer PR
runs-on: ubuntu-24.04
permissions:
pull-requests: write
contents: write
outputs:
was-closed: ${{ steps.validate.outputs.was-closed }}
steps:
- name: Generate GitHub App token
id: app-token
uses: actions/create-github-app-token@f8d387b68d61c58ab83c6c016672934102569859 # v2
with:
app-id: ${{ vars.SDK_MAINTAINER_BOT_APP_ID }}
private-key: ${{ secrets.SDK_MAINTAINER_BOT_PRIVATE_KEY }}

- name: Validate PR
id: validate
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1
with:
github-token: ${{ steps.app-token.outputs.token }}
script: |
const pullRequest = context.payload.pull_request;
const repo = context.repo;
const prAuthor = pullRequest.user.login;
const contributingUrl = `https://github.com/${repo.owner}/${repo.repo}/blob/${context.payload.repository.default_branch}/CONTRIBUTING.md`;

// --- Helper: check if a user has admin or maintain permission on a repo (cached) ---
const maintainerCache = new Map();
async function isMaintainer(owner, repoName, username) {
const key = `${owner}/${repoName}:${username}`;
if (maintainerCache.has(key)) return maintainerCache.get(key);
let result = false;
try {
const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
owner,
repo: repoName,
username,
});
// permission field uses legacy values (admin/write/read/none) where
// maintain maps to write. Use role_name for the actual role.
result = ['admin', 'maintain'].includes(data.role_name);
} catch {
// noop — result stays false
}
maintainerCache.set(key, result);
return result;
}

// --- Step 1: Check if PR author is a maintainer (admin or maintain role) ---
const authorIsMaintainer = await isMaintainer(repo.owner, repo.repo, prAuthor);
if (authorIsMaintainer) {
core.info(`PR author ${prAuthor} has admin/maintain access. Skipping.`);
return;
}
core.info(`PR author ${prAuthor} is not a maintainer.`);

// --- Step 2: Parse issue references from PR body ---
const body = pullRequest.body || '';

// Match all issue reference formats:
// #123, Fixes #123, getsentry/repo#123, Fixes getsentry/repo#123
// https://github.com/getsentry/repo/issues/123
const issueRefs = [];
const seen = new Set();

// Pattern 1: Full GitHub URLs
const urlPattern = /https?:\/\/github\.com\/(getsentry)\/([\w.-]+)\/issues\/(\d+)/gi;
for (const match of body.matchAll(urlPattern)) {
const key = `${match[1]}/${match[2]}#${match[3]}`;
if (!seen.has(key)) {
seen.add(key);
issueRefs.push({ owner: match[1], repo: match[2], number: parseInt(match[3]) });
}
}

// Pattern 2: Cross-repo references (getsentry/repo#123)
const crossRepoPattern = /(?:(?:fix|fixes|fixed|close|closes|closed|resolve|resolves|resolved)\s+)?(getsentry)\/([\w.-]+)#(\d+)/gi;
for (const match of body.matchAll(crossRepoPattern)) {
const key = `${match[1]}/${match[2]}#${match[3]}`;
if (!seen.has(key)) {
seen.add(key);
issueRefs.push({ owner: match[1], repo: match[2], number: parseInt(match[3]) });
}
}

// Pattern 3: Same-repo references (#123)
// Negative lookbehind to avoid matching cross-repo refs or URLs already captured
const sameRepoPattern = /(?:(?:fix|fixes|fixed|close|closes|closed|resolve|resolves|resolved)\s+)?(?<![/\w])#(\d+)/gi;
for (const match of body.matchAll(sameRepoPattern)) {
const key = `${repo.owner}/${repo.repo}#${match[1]}`;
if (!seen.has(key)) {
seen.add(key);
issueRefs.push({ owner: repo.owner, repo: repo.repo, number: parseInt(match[1]) });
}
}

core.info(`Found ${issueRefs.length} issue reference(s): ${[...seen].join(', ')}`);

// --- Helper: close PR with comment and labels ---
async function closePR(message, reasonLabel) {
await github.rest.issues.addLabels({
...repo,
issue_number: pullRequest.number,
labels: ['violating-contribution-guidelines', reasonLabel],
});

await github.rest.issues.createComment({
...repo,
issue_number: pullRequest.number,
body: message,
});

await github.rest.pulls.update({
...repo,
pull_number: pullRequest.number,
state: 'closed',
});

core.setOutput('was-closed', 'true');
}

// --- Step 3: No issue references ---
if (issueRefs.length === 0) {
core.info('No issue references found. Closing PR.');
await closePR([
'This PR has been automatically closed. All non-maintainer contributions must reference an existing GitHub issue.',
'',
'**Next steps:**',
'1. Find or open an issue describing the problem or feature',
'2. Discuss the approach with a maintainer in the issue',
'3. Once a maintainer has acknowledged your proposed approach, open a new PR referencing the issue',
'',
`Please review our [contributing guidelines](${contributingUrl}) for more details.`,
].join('\n'), 'missing-issue-reference');
return;
}

// --- Step 4: Validate each referenced issue ---
// A PR is valid if ANY referenced issue passes all checks.
let hasAssigneeConflict = false;
let hasNoDiscussion = false;

for (const ref of issueRefs) {
core.info(`Checking issue ${ref.owner}/${ref.repo}#${ref.number}...`);

let issue;
try {
const { data } = await github.rest.issues.get({
owner: ref.owner,
repo: ref.repo,
issue_number: ref.number,
});
issue = data;
} catch (e) {
core.warning(`Could not fetch issue ${ref.owner}/${ref.repo}#${ref.number}: ${e.message}`);
continue;
}

// Check assignee: if assigned to someone other than PR author, flag it
if (issue.assignees && issue.assignees.length > 0) {
const assignedToAuthor = issue.assignees.some(a => a.login === prAuthor);
if (!assignedToAuthor) {
core.info(`Issue ${ref.owner}/${ref.repo}#${ref.number} is assigned to someone else.`);
hasAssigneeConflict = true;
continue;
}
}

// Check discussion: both PR author and a maintainer must have commented
const comments = await github.paginate(github.rest.issues.listComments, {
owner: ref.owner,
repo: ref.repo,
issue_number: ref.number,
per_page: 100,
});

// Also consider the issue author as a participant (opening the issue is a form of discussion)
// Guard against null user (deleted/suspended GitHub accounts)
const prAuthorParticipated =
issue.user?.login === prAuthor ||
comments.some(c => c.user?.login === prAuthor);

let maintainerParticipated = false;
if (prAuthorParticipated) {
// Check each commenter (and issue author) for admin/maintain access on the issue's repo
const usersToCheck = new Set();
if (issue.user?.login) usersToCheck.add(issue.user.login);
for (const comment of comments) {
if (comment.user?.login && comment.user.login !== prAuthor) {
usersToCheck.add(comment.user.login);
}
}

for (const user of usersToCheck) {
if (user === prAuthor) continue;
if (await isMaintainer(repo.owner, repo.repo, user)) {

Check failure on line 204 in .github/workflows/validate-pr.yml

View check run for this annotation

@sentry/warden / warden: code-review

Maintainer check uses PR repo instead of issue repo for cross-repo references

At line 204, `isMaintainer(repo.owner, repo.repo, user)` checks maintainer status against the PR's repository, but when the referenced issue is from a different repository (e.g., `getsentry/sentry-python#123` referenced from `getsentry/some-other-repo`), the maintainer check should be against the issue's repository (`ref.owner`, `ref.repo`). This could incorrectly allow or block PRs when the maintainer status differs between repositories.

Check failure on line 204 in .github/workflows/validate-pr.yml

View check run for this annotation

@sentry/warden / warden: find-bugs

Maintainer check uses wrong repository for cross-repo issue references

At line 204, `isMaintainer(repo.owner, repo.repo, user)` checks if a user is a maintainer of the PR's repository instead of the issue's repository (`ref.owner, ref.repo`). When a PR references an issue from a different getsentry repository (e.g., `getsentry/sentry#123` in a PR to `getsentry/sentry-python`), this incorrectly validates maintainer participation against the wrong repository, potentially closing valid PRs or accepting invalid ones.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The maintainer check at line 204 incorrectly uses the PR's repository (repo.owner, repo.repo) instead of the issue's repository (ref.owner, ref.repo) for validation.
Severity: HIGH

Suggested Fix

Update the call to the isMaintainer function to use the correct variables for the issue's repository. Change isMaintainer(repo.owner, repo.repo, user) to isMaintainer(ref.owner, ref.repo, user).

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: .github/workflows/validate-pr.yml#L204

Potential issue: The workflow is designed to check if a maintainer has participated in a
linked issue, even if that issue is in a different repository. However, the call to
`isMaintainer` on line 204 incorrectly uses the PR's repository details (`repo.owner`,
`repo.repo`) instead of the issue's repository details (`ref.owner`, `ref.repo`). This
causes the check to fail when a maintainer of the issue's repository (but not the PR's
repository) comments on the issue, leading to the PR being incorrectly closed for
lacking maintainer discussion.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maintainer check uses PR's repo instead of issue's repo

Medium Severity

The comment on line 193 says "admin/maintain access on the issue's repo," but isMaintainer is called with repo.owner, repo.repo (the PR's target repo) instead of ref.owner, ref.repo (the referenced issue's repo). For cross-repo references (e.g., a PR to sentry-dotnet referencing getsentry/sentry#123), a sentry maintainer who discussed the issue would not be recognized, incorrectly closing the PR for "missing maintainer discussion."

Additional Locations (1)
Fix in Cursor Fix in Web

maintainerParticipated = true;
core.info(`Maintainer ${user} participated in ${ref.owner}/${ref.repo}#${ref.number}.`);
break;
}
}
}

if (prAuthorParticipated && maintainerParticipated) {
core.info(`Issue ${ref.owner}/${ref.repo}#${ref.number} has valid discussion. PR is allowed.`);
return; // PR is valid — at least one issue passes all checks
}

core.info(`Issue ${ref.owner}/${ref.repo}#${ref.number} lacks discussion between author and maintainer.`);
hasNoDiscussion = true;
}

// --- Step 5: No valid issue found — close with the most relevant reason ---
if (hasAssigneeConflict) {
core.info('Closing PR: referenced issue is assigned to someone else.');
await closePR([
'This PR has been automatically closed. The referenced issue is already assigned to someone else.',
'',
'If you believe this assignment is outdated, please comment on the issue to discuss before opening a new PR.',
'',
`Please review our [contributing guidelines](${contributingUrl}) for more details.`,
].join('\n'), 'issue-already-assigned');
return;
}

if (hasNoDiscussion) {
core.info('Closing PR: no discussion between PR author and a maintainer in the referenced issue.');
await closePR([
'This PR has been automatically closed. The referenced issue does not show a discussion between you and a maintainer.',
'',
'To avoid wasted effort on both sides, please discuss your proposed approach in the issue first and wait for a maintainer to respond before opening a PR.',
'',
`Please review our [contributing guidelines](${contributingUrl}) for more details.`,
].join('\n'), 'missing-maintainer-discussion');
return;
}

// If we get here, all issue refs were unfetchable
core.info('Could not validate any referenced issues. Closing PR.');
await closePR([
'This PR has been automatically closed. The referenced issue(s) could not be found.',
'',
'**Next steps:**',
'1. Ensure the issue exists and is in a `getsentry` repository',
'2. Discuss the approach with a maintainer in the issue',
'3. Once a maintainer has acknowledged your proposed approach, open a new PR referencing the issue',
'',
`Please review our [contributing guidelines](${contributingUrl}) for more details.`,
].join('\n'), 'missing-issue-reference');

enforce-draft:
name: Enforce Draft PR
needs: [validate-non-maintainer-pr]
if: |
always()
&& github.event.pull_request.draft == false
&& needs.validate-non-maintainer-pr.outputs.was-closed != 'true'
Comment on lines +262 to +265
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The enforce-draft job incorrectly runs on maintainer PRs because the was-closed output is unset, causing the conditional check to pass erroneously.
Severity: MEDIUM

Suggested Fix

The validate-non-maintainer-pr job should produce an output to indicate if the author was a maintainer. The enforce-draft job's conditional check should be updated to use this new output to skip execution for maintainers.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: .github/workflows/validate-pr.yml#L262-L265

Potential issue: When a maintainer opens a pull request, the
`validate-non-maintainer-pr` job correctly exits early but fails to set the `was-closed`
output. This output defaults to an empty string. The subsequent `enforce-draft` job has
a condition `needs.validate-non-maintainer-pr.outputs.was-closed != 'true'`, which
evaluates to `true` because an empty string is not equal to `'true'`. As a result, the
`enforce-draft` job incorrectly runs and converts the maintainer's non-draft PR into a
draft, which is contrary to the intended design.

Did we get this right? 👍 / 👎 to inform future reviews.

runs-on: ubuntu-24.04
permissions:
pull-requests: write
contents: write
steps:
- name: Generate GitHub App token
id: app-token
uses: actions/create-github-app-token@f8d387b68d61c58ab83c6c016672934102569859 # v2
with:
app-id: ${{ vars.SDK_MAINTAINER_BOT_APP_ID }}
private-key: ${{ secrets.SDK_MAINTAINER_BOT_PRIVATE_KEY }}

- name: Convert PR to draft
env:
GH_TOKEN: ${{github.token}}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Wrong token used for converting PR to draft

High Severity

The Convert PR to draft step sets GH_TOKEN to ${{github.token}} (the default GITHUB_TOKEN), but gh pr ready --undo uses the convertPullRequestToDraft GraphQL mutation, which is known to fail with the default GITHUB_TOKEN — even with pull-requests: write permission. The app token is generated in the previous step specifically for this purpose but goes unused here. GH_TOKEN needs to use ${{ steps.app-token.outputs.token }} instead.

Additional Locations (1)
Fix in Cursor Fix in Web

PR_URL: ${{ github.event.pull_request.html_url }}
run: |
gh pr ready "$PR_URL" --undo

- name: Label and comment
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1
with:
github-token: ${{ steps.app-token.outputs.token }}
script: |
const pullRequest = context.payload.pull_request;
const repo = context.repo;

// Label the PR so maintainers can filter/track violations
await github.rest.issues.addLabels({
...repo,
issue_number: pullRequest.number,
labels: ['converted-to-draft'],
});

// Check for existing bot comment to avoid duplicates on reopen
const comments = await github.rest.issues.listComments({
...repo,
issue_number: pullRequest.number,
});
const botComment = comments.data.find(c =>
c.user.type === 'Bot' &&

Check warning on line 306 in .github/workflows/validate-pr.yml

View check run for this annotation

@sentry/warden / warden: find-bugs

Potential runtime error when checking for bot comment due to null user

At line 306, `c.user.type === 'Bot'` accesses `type` without null checking, but GitHub comments can have null users (deleted/suspended accounts). Earlier code (lines 189, 197) correctly uses optional chaining (`c.user?.login`). This inconsistency could cause a runtime error that crashes the workflow step.
c.body.includes('automatically converted to draft')
);
if (botComment) {
core.info('Bot comment already exists, skipping.');
return;
}

const contributingUrl = `https://github.com/${repo.owner}/${repo.repo}/blob/${context.payload.repository.default_branch}/CONTRIBUTING.md`;

await github.rest.issues.createComment({
...repo,
issue_number: pullRequest.number,
body: [
`This PR has been automatically converted to draft. All PRs must start as drafts per our [contributing guidelines](${contributingUrl}).`,
'',
'**Next steps:**',
'1. Ensure CI passes',
'2. Fill in the PR description completely',
'3. Mark as "Ready for review" when you\'re done'
].join('\n')
});
Loading