Skip to content
Closed
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;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

m: can we extract the inline scripts in this workflow to a proper JS/TS files? This should make spotting bugs in there much easier than trying to parse JS in yml files 😅

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;
}
Comment on lines +130 to +144
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this will likely lead to race conditions with another workflow we have that automatically opens an issue when a PR is created without referencing an issue in the description. So one of the two things could happen:

  1. we close the issue before the other workflow creates an issue
  2. we don't close the issue because the other workflow was faster

Either way is probably not what we want. Should we remove our other workflow? We added it to get better visibility for community-opened PRs. But if we require an issue with discussion prior to external contributors opening a PR, we probably don't need our workflow anymore. WDYT?


// --- 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)) {
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 for a linked issue incorrectly uses the PR's repository context instead of the issue's repository context, failing cross-repository validations.
Severity: HIGH

Suggested Fix

Modify the call to isMaintainer on line 204 to use the issue's repository context, which is stored in the ref variable. The line should be changed to if (await 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: When validating if a maintainer has commented on a linked issue, the
code incorrectly checks for maintainer status on the pull request's repository instead
of the issue's repository. The code at line 204 calls `isMaintainer(repo.owner,
repo.repo, user)`, where `repo` refers to the PR's repository. For cross-repository
issue references (e.g., a PR in `sentry-javascript` referencing an issue in `sentry`),
this logic is flawed. A maintainer of the `sentry` repo who comments on the issue would
not be recognized, causing the PR to be incorrectly closed for
'missing-maintainer-discussion'. This contradicts the code's intent, which explicitly
parses cross-repo issue links.

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

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'
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 uses ${{github.token}} (the default GITHUB_TOKEN) instead of the GitHub App token generated in the previous step (${{ steps.app-token.outputs.token }}). The gh pr ready --undo command uses the convertPullRequestToDraft GraphQL mutation, which has known compatibility issues with the default GITHUB_TOKEN. The app token is generated specifically for this job but goes unused here, while the subsequent "Label and comment" step correctly uses steps.app-token.outputs.token.

Additional Locations (1)
Fix in Cursor Fix in Web

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is bugbot right here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No it isn't - it works like this (and we tried many other approached that all failed)

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' &&
Comment on lines +305 to +306
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 code accesses c.user.type without checking if c.user is null, which can occur if a comment was made by a deleted or suspended GitHub account.
Severity: MEDIUM

Suggested Fix

Use optional chaining to safely access the type property. Change c.user.type === 'Bot' to c.user?.type === 'Bot' on line 306.

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#L305-L306

Potential issue: In the `enforce-draft` job, the code at line 306 attempts to access
`c.user.type` directly within a `find` operation on an array of comments. The GitHub API
returns `null` for the `user` field on comments made by deleted or suspended accounts.
This will cause a `TypeError: Cannot read property 'type' of null`, which will crash the
workflow step. Evidence from earlier in the same file (lines 186-199) shows awareness of
this issue, where optional chaining (`?.`) is used to safely access user properties.
This crash would prevent an informational comment from being posted on the PR.

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

c.body.includes('automatically converted to draft')
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing null check on comment user property

Low Severity

c.user.type is accessed without optional chaining at line 306, but c.user can be null for deleted or suspended GitHub accounts. The validate step in the same file correctly guards against this with c.user?.login (lines 189, 197), but this check in the enforce-draft job does not use c.user?.type, which would cause a runtime error if encountered.

Fix in Cursor Fix in Web

);
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