diff --git a/CHANGELOG.md b/CHANGELOG.md index b02316c..f222658 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,9 @@ ### Features -- Add validate-pr composite action for validating non-maintainer PRs against contribution guidelines and enforcing draft status ([#153](https://github.com/getsentry/github-workflows/pull/153)) +- Validate PR - Action is advisory: it posts a single friendly comment on community PRs that don't reference an issue with maintainer discussion. PRs are not closed and no labels are applied. Recommended trigger is `types: [opened]`. +- Validate PR - Skip validation for PRs with fewer than 100 lines changed, excluding common lock files (`Cargo.lock`, `yarn.lock`, `package-lock.json`, `Pipfile.lock`, etc.). Tiny PRs no longer go through the issue-discussion loop. +- Add validate-pr composite action for validating non-maintainer PRs against contribution guidelines ([#153](https://github.com/getsentry/github-workflows/pull/153)) ## 3.3.0 diff --git a/validate-pr/README.md b/validate-pr/README.md index e94a5f3..779b179 100644 --- a/validate-pr/README.md +++ b/validate-pr/README.md @@ -1,12 +1,12 @@ # Validate PR -Validates non-maintainer pull requests against contribution guidelines. +Advisory validation for non-maintainer pull requests against contribution guidelines. Posts a single friendly comment when a PR doesn't reference an issue with prior maintainer discussion. **PRs are never closed, and no labels are applied.** ## What it does -**Validates issue references** — Non-maintainer PRs must reference a GitHub issue where the PR author and a maintainer have discussed the approach. PRs that don't meet this requirement are automatically closed with a descriptive comment. +For PRs from non-maintainer authors, the action checks that the PR body references a GitHub issue where the PR author and a maintainer have discussed the approach. When that's not the case, the action posts one short advisory comment inviting the contributor to start with an issue. Maintainers (`admin` or `maintain` role) and a hard-coded list of trusted bots are exempt. -Maintainers (users with `admin` or `maintain` role) are exempt from validation. When a maintainer reopens a previously closed PR, all checks are skipped — this allows maintainers to override the action's decision. +Small PRs (< 100 lines changed, excluding lock files) are skipped entirely — typo fixes and tiny bug fixes don't need to go through the issue-discussion loop. ## Usage @@ -17,7 +17,7 @@ name: Validate PR on: pull_request_target: - types: [opened, reopened] + types: [opened] jobs: validate-pr: @@ -25,12 +25,14 @@ jobs: permissions: pull-requests: write steps: - - uses: getsentry/github-workflows/validate-pr@v3 + - uses: getsentry/github-workflows/validate-pr@ with: app-id: ${{ vars.SDK_MAINTAINER_BOT_APP_ID }} private-key: ${{ secrets.SDK_MAINTAINER_BOT_PRIVATE_KEY }} ``` +Pin to a specific commit SHA (consumers in `getsentry/*` already follow this convention). The `pull-requests: write` permission is needed because the action posts comments on the PR. + ## Inputs | Input | Required | Description | @@ -38,33 +40,57 @@ jobs: | `app-id` | Yes | GitHub App ID for the SDK Maintainer Bot | | `private-key` | Yes | GitHub App private key for the SDK Maintainer Bot | -## Outputs +## Validation rules -| Output | Description | -|--------|-------------| -| `was-closed` | `'true'` if the PR was closed by validation, unset otherwise | +### Skipped entirely -## Validation rules +- PR author is in the trusted-bot allowlist (Dependabot, Renovate, Codecov AI, etc.) +- PR author has `admin`, `maintain`, `push`, or `write` access on the repo +- PR has fewer than 100 lines changed (`additions + deletions`), excluding common lock files + +### Lock files excluded from line counts + +Matched by basename, case-insensitive: + +| Ecosystem | File | +|-----------|------| +| Rust | `Cargo.lock` | +| JS | `package-lock.json`, `yarn.lock`, `pnpm-lock.yaml` | +| Python | `Pipfile.lock`, `poetry.lock`, `uv.lock` | +| Ruby | `Gemfile.lock` | +| PHP | `composer.lock` | +| Go | `go.sum` (`go.mod` is hand-edited and stays counted) | +| Elixir | `mix.lock` | +| Dart/Flutter | `pubspec.lock` | +| .NET/NuGet | `packages.lock.json` | +| CocoaPods | `Podfile.lock` | +| Nix | `flake.lock` | ### Issue reference check -The PR body is scanned for issue references in these formats: +For PRs that reach validation, the action scans the PR body for issue references in these formats: - `#123` (same-repo) - `getsentry/repo#123` (cross-repo) - `https://github.com/getsentry/repo/issues/123` (full URL) - With optional keywords: `Fixes #123`, `Closes getsentry/repo#123`, etc. -A PR is valid if **any** referenced issue passes all checks: +The PR is considered compliant if **any** referenced issue passes all of: + - The issue is fetchable and in a `getsentry` repository -- If the issue has assignees, the PR author must be one of them +- If the issue has assignees, the PR author is one of them - Both the PR author and a maintainer have participated in the issue discussion -## Labels +If no referenced issue passes, the action posts one advisory comment. The PR remains open and reviewable; no labels or status checks are applied. The comment is idempotent — workflow re-runs on the same PR will not produce duplicates. + +## Updating from earlier revisions + +Earlier revisions of this action closed non-compliant PRs and applied labels (`violating-contribution-guidelines`, `missing-issue-reference`, `missing-maintainer-discussion`, `issue-already-assigned`). The current version does neither — it only posts a comment. + +To update an existing consumer: -The action creates these labels automatically (they don't need to exist beforehand): +- Bump the pinned commit SHA to the latest on `main`. +- Change `types: [opened, reopened]` → `types: [opened]`. +- Remove any code that reads the `was-closed` output (it no longer exists). -- `violating-contribution-guidelines` — added to all closed PRs -- `missing-issue-reference` — PR body has no issue references -- `missing-maintainer-discussion` — referenced issue lacks author + maintainer discussion -- `issue-already-assigned` — referenced issue is assigned to someone else +Existing labels on old PRs are not removed automatically. Clean them up with a one-off script if desired. diff --git a/validate-pr/action.yml b/validate-pr/action.yml index b305549..d5b78b3 100644 --- a/validate-pr/action.yml +++ b/validate-pr/action.yml @@ -10,11 +10,6 @@ inputs: description: 'GitHub App private key for the SDK Maintainer Bot' required: true -outputs: - was-closed: - description: 'Whether the PR was closed by the validation step' - value: ${{ steps.validate.outputs.was-closed }} - runs: using: 'composite' steps: @@ -28,6 +23,8 @@ runs: - name: Validate PR id: validate uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + env: + APP_SLUG: ${{ steps.app-token.outputs.app-slug }} with: github-token: ${{ steps.app-token.outputs.token }} script: | diff --git a/validate-pr/scripts/validate-pr.js b/validate-pr/scripts/validate-pr.js index 113e55b..ac823c7 100644 --- a/validate-pr/scripts/validate-pr.js +++ b/validate-pr/scripts/validate-pr.js @@ -1,10 +1,9 @@ // @ts-check /** - * Validates non-maintainer PRs by checking that they reference a GitHub issue - * with prior discussion between the author and a maintainer. - * - * Closes PRs that don't meet contribution guidelines. + * Advisory validation for community PRs. Posts a single friendly comment + * when the PR doesn't reference an issue with maintainer discussion. + * Never closes PRs, never applies labels. * * @param {object} params * @param {import('@actions/github').getOctokit} params.github @@ -30,11 +29,10 @@ module.exports = async ({ github, context, core }) => { ]; if (ALLOWED_BOTS.includes(prAuthor)) { core.info(`PR author ${prAuthor} is an allowed bot. Skipping.`); - core.setOutput('skipped', 'true'); return; } - // --- Helpers: check user permission on a repo (cached) --- + // --- Helpers: cached collaborator-role lookup --- const roleCache = new Map(); async function getRole(owner, repoName, username) { const key = `${owner}/${repoName}:${username}`; @@ -56,7 +54,6 @@ module.exports = async ({ github, context, core }) => { async function hasWriteAccess(owner, repoName, username) { const role = await getRole(owner, repoName, username); - // role_name values: admin, maintain, push, triage, pull (+ custom roles) return ['admin', 'maintain', 'push', 'write'].includes(role); } @@ -65,36 +62,79 @@ module.exports = async ({ github, context, core }) => { return ['admin', 'maintain'].includes(role); } - // --- Step 1: Skip if a maintainer reopened the PR --- - if (context.payload.action === 'reopened') { - const sender = context.payload.sender.login; - const senderIsMaintainer = await isMaintainer(repo.owner, repo.repo, sender); - if (senderIsMaintainer) { - core.info(`PR reopened by maintainer ${sender}. Skipping all checks.`); - core.setOutput('skipped', 'true'); - return; - } - } - - // --- Step 2: Check if PR author has write access (admin, maintain, or write role) --- - const authorHasWriteAccess = await hasWriteAccess(repo.owner, repo.repo, prAuthor); - if (authorHasWriteAccess) { + // --- Step 1: Skip if PR author has write+ access --- + if (await hasWriteAccess(repo.owner, repo.repo, prAuthor)) { core.info(`PR author ${prAuthor} has write+ access. Skipping.`); - core.setOutput('skipped', 'true'); return; } core.info(`PR author ${prAuthor} does not have write access.`); + // --- Step 2: Small-PR bypass (excluding lock files) --- + const LOCK_FILE_BASENAMES = new Set([ + 'cargo.lock', + 'package-lock.json', + 'yarn.lock', + 'pnpm-lock.yaml', + 'pipfile.lock', + 'poetry.lock', + 'uv.lock', + 'gemfile.lock', + 'composer.lock', + 'go.sum', + 'mix.lock', + 'pubspec.lock', + 'packages.lock.json', + 'podfile.lock', + 'flake.lock', + ]); + const SMALL_PR_THRESHOLD = 100; + + const aggregateLOC = (pullRequest.additions || 0) + (pullRequest.deletions || 0); + if (aggregateLOC < SMALL_PR_THRESHOLD) { + core.info( + `PR has ${aggregateLOC} lines changed (< ${SMALL_PR_THRESHOLD}). Skipping.` + ); + return; + } + + // Stage 2: fetch file list and recompute excluding lock files. On API + // failure, fall through to validation rather than aborting — a transient + // GitHub error shouldn't break the action. + let files = null; + try { + files = await github.paginate(github.rest.pulls.listFiles, { + owner: repo.owner, + repo: repo.repo, + pull_number: pullRequest.number, + per_page: 100, + }); + } catch (e) { + core.warning( + `Could not fetch PR file list (${e.message}); skipping lock-file exclusion and proceeding to validation.` + ); + } + if (files) { + function basenameLower(path) { + const idx = path.lastIndexOf('/'); + return (idx >= 0 ? path.slice(idx + 1) : path).toLowerCase(); + } + const nonLockLOC = files + .filter((f) => !LOCK_FILE_BASENAMES.has(basenameLower(f.filename))) + .reduce((sum, f) => sum + (f.additions || 0) + (f.deletions || 0), 0); + if (nonLockLOC < SMALL_PR_THRESHOLD) { + core.info( + `PR has ${nonLockLOC} non-lock-file lines changed (< ${SMALL_PR_THRESHOLD}). Skipping.` + ); + return; + } + } + // --- Step 3: 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 + // 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]}`; @@ -104,7 +144,7 @@ module.exports = async ({ github, context, core }) => { } } - // Pattern 2: Cross-repo references (getsentry/repo#123) + // 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]}`; @@ -114,8 +154,7 @@ module.exports = async ({ github, context, core }) => { } } - // Pattern 3: Same-repo references (#123) - // Negative lookbehind to avoid matching cross-repo refs or URLs already captured + // Pattern 3: same-repo references (#123) const sameRepoPattern = /(?:(?:fix|fixes|fixed|close|closes|closed|resolve|resolves|resolved)\s+)?(? { 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 4: 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 5: Validate each referenced issue --- - // A PR is valid if ANY referenced issue passes all checks. - let hasAssigneeConflict = false; - let hasNoDiscussion = false; - + // --- Step 4: Validate referenced issues; succeed if any pass all checks --- for (const ref of issueRefs) { core.info(`Checking issue ${ref.owner}/${ref.repo}#${ref.number}...`); @@ -187,17 +183,16 @@ module.exports = async ({ github, context, core }) => { continue; } - // Check assignee: if assigned to someone other than PR author, flag it + // Assignee check: skip this ref if assigned to someone other than PR author. if (issue.assignees && issue.assignees.length > 0) { - const assignedToAuthor = issue.assignees.some(a => a.login === prAuthor); + 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 + // Discussion check: PR author and a maintainer must both have participated. const comments = await github.paginate(github.rest.issues.listComments, { owner: ref.owner, repo: ref.repo, @@ -205,77 +200,78 @@ module.exports = async ({ github, context, core }) => { 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); + comments.some((c) => c.user?.login === prAuthor); - let maintainerParticipated = false; - if (prAuthorParticipated) { - // Check each commenter (and issue author) for admin/maintain access on the target 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); - } + if (!prAuthorParticipated) { + core.info(`Issue ${ref.owner}/${ref.repo}#${ref.number} has no PR author participation.`); + continue; + } + + const usersToCheck = new Set(); + if (issue.user?.login && issue.user.login !== prAuthor) { + 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)) { - maintainerParticipated = true; - core.info(`Maintainer ${user} participated in ${ref.owner}/${ref.repo}#${ref.number}.`); - break; - } + let maintainerParticipated = false; + for (const user of usersToCheck) { + if (await isMaintainer(repo.owner, repo.repo, user)) { + maintainerParticipated = true; + core.info(`Maintainer ${user} participated in ${ref.owner}/${ref.repo}#${ref.number}.`); + break; } } - if (prAuthorParticipated && maintainerParticipated) { + if (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 + return; } - - core.info(`Issue ${ref.owner}/${ref.repo}#${ref.number} lacks discussion between author and maintainer.`); - hasNoDiscussion = true; + core.info(`Issue ${ref.owner}/${ref.repo}#${ref.number} lacks maintainer participation.`); } - // --- Step 6: 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'); + // --- Step 5: Validation failed — post one warm comment (idempotent) --- + // The bot's GitHub login is `${app.slug}[bot]`. We get the slug from the + // composite action via the APP_SLUG env var (sourced from + // create-github-app-token's `app-slug` output) — calling + // apps.getAuthenticated() here would fail because it requires JWT auth + // and the github client is authenticated with an installation token. + const appSlug = process.env.APP_SLUG; + if (!appSlug) { + core.setFailed( + 'APP_SLUG env var is not set. The validate-pr composite action must pass app-slug from create-github-app-token to the script.' + ); return; } + const botLogin = `${appSlug}[bot]`; - 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'); + const existing = await github.paginate(github.rest.issues.listComments, { + ...repo, + issue_number: pullRequest.number, + per_page: 100, + }); + if (existing.some((c) => c.user?.login === botLogin)) { + core.info(`Bot ${botLogin} already commented on this PR. Skipping.`); 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.', + const commentBody = [ + '👋 Thanks for sending this our way! Before a maintainer reviews the code, we ask community contributors to align with us on the approach first — it keeps your time pointed at changes we can land.', '', - '**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', + 'The easiest way is to open or find a GitHub issue and discuss the approach with a maintainer there, then link that issue from this PR. If the issue is already assigned to someone else, please check in with them (or with us) before continuing — otherwise two people may end up working on the same task.', '', - `Please review our [contributing guidelines](${contributingUrl}) for more details.`, - ].join('\n'), 'missing-issue-reference'); + `See our [contributing guidelines](${contributingUrl}) for the full picture.`, + ].join('\n'); + + await github.rest.issues.createComment({ + ...repo, + issue_number: pullRequest.number, + body: commentBody, + }); + core.info('Posted advisory comment. PR remains open.'); };