From dc330fee2faa815ce878db5c46f196735d117474 Mon Sep 17 00:00:00 2001 From: Rebecca Chen Date: Fri, 30 Oct 2026 01:11:54 -0700 Subject: [PATCH 1/4] Add PR automation that requires the "I am a human" checkbox in PR descriptions (#5458) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: * Adds a check to the automation_pr_state workflow to require the checkbox. * A PR that fails the check is left in the waiting-for-author state, and a bot-posted comment explains why. The comment is posted only when the check fails. * The check runs when a PR has a cla-signed label added, when a draft PR is moved to non-draft, and when a PR with the bot comment is updated. This means that new PRs get the check, and the comment is kept up-to-date, but old PRs are left untouched. Here are the comments that the check leaves on PRs: When the description does not contain the "I am a human" declaration: ⚠️ **Action needed**: this PR is missing required items from our [pull request checklist](https://github.com/facebook/pyrefly/blob/main/CONTRIBUTING.md#making-a-pull-request): - Add this checkbox to the PR description and check it: `- [x] I am a human, and I fully understand the code I am submitting.` See our [AI policy](https://github.com/facebook/pyrefly/blob/main/AI_POLICY.md). Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour. When the description contains the declaration but the box is not checked: ⚠️ **Action needed**: this PR is missing required items from our [pull request checklist](https://github.com/facebook/pyrefly/blob/main/CONTRIBUTING.md#making-a-pull-request): - Check the "I am a human" box in the PR description. See our [AI policy](https://github.com/facebook/pyrefly/blob/main/AI_POLICY.md). Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour. When the description previously had an issue but now contains the declaration with the box checked: ✅ This PR follows our [pull request checklist](https://github.com/facebook/pyrefly/blob/main/CONTRIBUTING.md#making-a-pull-request). Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour. Differential Revision: D128488447 --- .github/workflows/automation_pr_state.yml | 150 ++++++++++++++++++++-- CONTRIBUTING.md | 8 +- 2 files changed, 143 insertions(+), 15 deletions(-) diff --git a/.github/workflows/automation_pr_state.yml b/.github/workflows/automation_pr_state.yml index 75a12f4adb..10bcc2d63d 100644 --- a/.github/workflows/automation_pr_state.yml +++ b/.github/workflows/automation_pr_state.yml @@ -3,6 +3,9 @@ name: "Automation: PR state" # Runs automation based on the current PR state: # - Labels the PR's size. # - Updates "waiting-for-*" labels to indicate who should act next. +# - Checks ready PRs from outside the team against the pull request checklist +# in CONTRIBUTING.md. PRs that fail are kept in "waiting-for-author", and +# problems are reported in a single comment that is edited in place. on: pull_request_target: @@ -10,6 +13,7 @@ on: - opened - synchronize - reopened + - edited - closed - ready_for_review - converted_to_draft @@ -30,13 +34,13 @@ on: description: 'Operation' type: choice options: - - Update state label on one PR + - Update state on one PR - Reconcile PRs updated since last sweep pr_number: description: 'PR number (only for updating one PR)' type: number dry_run: - description: 'Dry run (preview labels without applying)' + description: 'Dry run (preview without applying)' type: boolean default: true @@ -67,7 +71,8 @@ jobs: && contains(github.event.comment.body, '@')) || (github.event.comment.user.login == 'meta-cla[bot]' && github.event.comment.user.type == 'Bot' - && contains(github.event.comment.body, '# Action Required'))))) }} + && contains(github.event.comment.body, '# Action Required'))))) + && (github.event.action != 'edited' || github.event.changes.body != null) }} runs-on: ubuntu-latest permissions: actions: read # find the last successful sweep @@ -76,6 +81,7 @@ jobs: outputs: prs: ${{ steps.select.outputs.result }} owners: ${{ steps.select.outputs.owners }} + since: ${{ steps.select.outputs.since }} steps: - name: Select PRs to evaluate id: select @@ -103,7 +109,7 @@ jobs: || context.payload.inputs?.scenario === 'Reconcile PRs updated since last sweep'; if (context.eventName === 'workflow_dispatch') { const { scenario, pr_number } = context.payload.inputs; - if (scenario === 'Update state label on one PR') { + if (scenario === 'Update state on one PR') { if (!pr_number) throw new Error('pr_number is required to update one PR'); return [Number(pr_number)]; } @@ -121,6 +127,8 @@ jobs: // A run is created before it selects PRs, even when it is re-run, // so every update after the previous sweep's selection is covered. if (last) since = Date.parse(last.created_at); + // Without a previous sweep, don't enroll PRs from historical CLA events. + core.setOutput('since', Number.isFinite(since) ? since : Date.now()); } const prs = await github.paginate(github.rest.pulls.list, { owner, repo, state: 'open', sort: 'updated', direction: 'desc', per_page: 100, @@ -154,14 +162,118 @@ jobs: group: pr-state-${{ matrix.pr }} runs-on: ubuntu-latest permissions: - pull-requests: write # read the PR, reviews, labels, and timeline, and update labels + pull-requests: write # read the PR, reviews, labels, and timeline, update labels, and comment checks: read # distinguish unsigned CLAs from pending checks steps: + # The next step reacts to this step's result: "passed", "failed", or + # "not run". The "changed" output is true when the result differs from + # the one previously reported in the checklist comment, which makes the + # next step re-evaluate the state as it does for a lifecycle event. The + # "summary" output describes the result and the comment for the run + # summary. + - name: Check the PR against the pull request checklist + id: checklist + uses: actions/github-script@v8 + env: + PR_NUMBER: ${{ matrix.pr }} + OWNERS_JSON: ${{ needs.select-prs.outputs.owners }} + SWEEP_SINCE: ${{ needs.select-prs.outputs.since }} + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + result-encoding: string + script: | + const dryRun = process.env.DRY_RUN === 'true'; + const { owner, repo } = dryRun ? { owner: 'facebook', repo: 'pyrefly' } : context.repo; + const pull_number = Number(process.env.PR_NUMBER); + const issue_number = pull_number; + const MARKER = ''; + + const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number }); + const owners = JSON.parse(process.env.OWNERS_JSON); + const author = pr.user.login.toLowerCase(); + const labels = (await github.paginate(github.rest.issues.listLabelsOnIssue, { + owner, repo, issue_number, per_page: 100, + })).map(l => l.name); + const waitingForMerge = labels.includes('waiting-for-merge'); + if (pr.user.type === 'Bot' || pr.head.ref.startsWith('export-D') || owners.includes(author) + || pr.state !== 'open' || pr.draft + || !labels.includes('cla signed')) { + return 'not run'; + } + const timeline = await github.paginate(github.rest.issues.listEventsForTimeline, { + owner, repo, issue_number, per_page: 100, + }); + const existingCheck = timeline.find(c => c.event === 'commented' && c.user?.login === 'github-actions[bot]' + && c.user?.type === 'Bot' && c.body?.includes(MARKER)); + // PRs waiting for merge only receive updates to an existing checklist comment. + if (waitingForMerge && !existingCheck) { + return 'not run'; + } + const sweep = context.eventName === 'schedule' + || context.payload.inputs?.scenario === 'Reconcile PRs updated since last sweep'; + const manual = context.eventName === 'workflow_dispatch' && !sweep; + const initialCheck = (context.eventName === 'pull_request_target' + && (context.payload.action === 'ready_for_review' + || (context.payload.action === 'labeled' && context.payload.label.name === 'cla signed'))) + || (sweep && timeline.some(e => (e.event === 'ready_for_review' + || (e.event === 'labeled' && e.label?.name === 'cla signed')) + && Date.parse(e.created_at) >= Number(process.env.SWEEP_SINCE))); + if (!manual && !initialCheck && !existingCheck) { + return 'not run'; + } + + // "x" if the box is checked, " " if it is unchecked, or undefined if + // it is missing. + const box = (pr.body || '').replace(//g, '') + .match(/^\s*[-*]\s*\[([ x])\]\s*I am a human/im)?.[1].toLowerCase(); + let problem; + if (box !== 'x') { + const policy = `See our [AI policy](https://github.com/${owner}/${repo}/blob/main/AI_POLICY.md).`; + problem = box === undefined + ? `Add this checkbox to the PR description and check it: \`- [x] I am a human, and I fully understand the code I am submitting.\` ${policy}` + : `Check the "I am a human" box in the PR description. ${policy}`; + } + + const CHECKLIST = `[pull request checklist](https://github.com/${owner}/${repo}/blob/main/CONTRIBUTING.md#making-a-pull-request)`; + const report = [ + MARKER, + problem + ? `⚠️ **Action needed**: this PR is missing required items from our ${CHECKLIST}:\n\n- ${problem}` + : `✅ This PR follows our ${CHECKLIST}.`, + 'Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour.', + ].join('\n\n'); + const writeComment = existingCheck ? existingCheck.body !== report : Boolean(problem); + if (!dryRun && writeComment) { + if (existingCheck) { + await github.rest.issues.updateComment({ owner, repo, comment_id: existingCheck.id, body: report }); + } else { + await github.rest.issues.createComment({ owner, repo, issue_number, body: report }); + } + } + const result = problem ? 'failed' : 'passed'; + const comment = !writeComment ? (existingCheck ? 'comment unchanged' : 'no comment') + : `comment ${dryRun ? 'would be ' : ''}${existingCheck ? 'updated' : 'posted'}`; + core.setOutput('summary', `${result}, ${comment}`); + // A PR without a checklist comment counts as passed, because no + // problem has been reported to the author. + const previous = existingCheck?.body.includes('⚠️ **Action needed**') ? 'failed' : 'passed'; + console.log(`#${pull_number}: checklist ${previous} -> ${result}, ${comment}`); + // Comment updates on PRs waiting for merge do not affect state labels. + if (waitingForMerge) { + return 'not run'; + } + core.setOutput('changed', previous !== result); + return result; + - name: Label the PR uses: actions/github-script@v8 env: PR_NUMBER: ${{ matrix.pr }} OWNERS_JSON: ${{ needs.select-prs.outputs.owners }} + SWEEP_SINCE: ${{ needs.select-prs.outputs.since }} + CHECKLIST: ${{ steps.checklist.outputs.result }} + CHECKLIST_CHANGED: ${{ steps.checklist.outputs.changed }} + CHECKLIST_SUMMARY: ${{ steps.checklist.outputs.summary }} with: github-token: ${{ secrets.GITHUB_TOKEN }} script: | @@ -187,6 +299,7 @@ jobs: const sweep = context.eventName === 'schedule' || context.payload.inputs?.scenario === 'Reconcile PRs updated since last sweep'; const manual = context.eventName === 'workflow_dispatch' && !sweep; + const checklist = process.env.CHECKLIST; let timelineRequest; const listTimeline = () => timelineRequest ??= github.paginate(github.rest.issues.listEventsForTimeline, { owner, repo, issue_number, per_page: 100, @@ -196,13 +309,21 @@ jobs: const changedAt = Math.max(...(sweep || context.eventName === 'workflow_run' ? await listTimeline() : []) .filter(e => ['labeled', 'unlabeled'].includes(e.event) && STATE_LABELS.includes(e.label?.name)) .map(e => Date.parse(e.created_at))); + const isChecklistComment = e => e.event === 'commented' && e.user?.login === 'github-actions[bot]' + && e.user?.type === 'Bot' && e.body?.includes(''); // A sweep stands in for event-triggered runs that may have been // missed, so it handles each newer triggering event the way that - // event's run does. + // event's run does. The checklist comment is edited in place, so + // its latest edit is its event. Only the first successful sweep + // after an edit handles it, because later sweeps start after it. const newEvents = sweep - ? (await listTimeline()).filter(e => Date.parse(e.created_at ?? e.submitted_at) > changedAt) + ? (await listTimeline()).filter(e => isChecklistComment(e) + ? Date.parse(e.updated_at) > Math.max(changedAt, Number(process.env.SWEEP_SINCE)) + : Date.parse(e.created_at ?? e.submitted_at) > changedAt) : []; - if (sweep && newEvents.length === 0) { + const checklistChanged = checklist !== 'not run' + && (process.env.CHECKLIST_CHANGED === 'true' || newEvents.some(isChecklistComment)); + if (sweep && newEvents.length === 0 && !checklistChanged) { console.log(`#${pull_number}: no events since the latest state label change, nothing to do`); return; } @@ -320,7 +441,8 @@ jobs: console.log(`#${pull_number}: review request cannot approve or undo waiting-for-merge, leaving as-is`); return; } - } else if (context.eventName === 'workflow_run' || (sweep && !lifecycle && !claChanged)) { + } else if (!checklistChanged + && (context.eventName === 'workflow_run' || (sweep && !lifecycle && !claChanged))) { // A relay run can also represent an ignored thread reply. // Only reviews, dismissals, and re-requests newer than the // latest state label change can change it. @@ -348,11 +470,13 @@ jobs: console.log(`#${pull_number}: re-request cannot undo waiting-for-merge, leaving as-is`); return; } - } else if (!lifecycle && !claChanged && !manual) { + } else if (!lifecycle && !claChanged && !checklistChanged && !manual) { console.log(`#${pull_number}: event does not affect the state, nothing to do`); return; } - if (!claSigned) state = 'waiting-for-author'; + if (!claSigned || (checklist === 'failed' && state === 'waiting-for-reviewer')) { + state = 'waiting-for-author'; + } } } @@ -369,7 +493,7 @@ jobs: const action = remove.length || add ? (dryRun ? 'Would update' : 'Updated') : 'Unchanged'; console.log(`#${pull_number}: ${action}: ${current.join(', ') || 'none'} -> ${state || 'none'}`); await core.summary.addTable([ - ['PR', 'Current labels', 'New label', 'Action'].map(data => ({ data, header: true })), + ['PR', 'Current labels', 'New label', 'Checklist'].map(data => ({ data, header: true })), [`#${pull_number}`, - current.join(', ') || 'none', state || 'none', action], + current.join(', ') || 'none', state || 'none', process.env.CHECKLIST_SUMMARY || 'not run'], ]).write(); diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5f0a77a4c9..a718834eab 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -54,10 +54,14 @@ How it works: **Please note:** Claiming issues helps other contributors and maintainers see what is being worked on. If you do not claim an issue you're working on, multiple people may work on the same issue at the same time. This can lead to multiple PRs for the same task and increased review burden on maintainers. In cases where multiple PRs are opened for the same task, maintainers will prioritise reviewing the PR from the author who #claim-ed the issue. For issues marked with the `good first issue` tag, please only claim and work on one issue at a time to allow other newcomers to also work on issues. +### PR readiness checks + +Automated readiness checks run on newly opened PRs, checking items from our [PR checklist](#making-a-pull-request). If an "⚠️ **Action needed**" comment is posted on your PR, please follow the steps in the comment to ensure the PR is ready for review. The check automatically reruns when the PR is updated, with a lag of up to half an hour. + ### PR turn tracking PRs opened by non-maintainers are automatically labeled with who should act next: -* "waiting-for-author" means the PR is waiting for action by the author, usually to sign the CLA or address review feedback. +* "waiting-for-author" means the PR is waiting for action by the author, usually to sign the CLA, address failed [readiness checks](#pr-readiness-checks), or respond to review feedback. * "waiting-for-reviewer" means the PR is waiting for action by the reviewer, usually to leave a code review. * "waiting-for-merge" means the PR has been approved and is waiting to be merged by a maintainer. @@ -204,7 +208,7 @@ Contributing a pull request (PR) is the main way to propose changes to Pyrefly. - [ ] **IMPORTANT** [Claim the issue](#repository-automation) before starting work on it. - [ ] Update or add new tests to cover your changes (see testing section for details). - [ ] Limit your PR to a single purpose or issue. Avoid mixing unrelated changes, as this makes review harder. If the PR is large, consider [splitting it](#splitting-a-pull-request) for faster reviews. -- [ ] Write a clear PR description following the [template](.github/pull_request_template.md). +- [ ] Write a clear PR description following the [template](.github/pull_request_template.md), including checking the "I am a human" box to attest that you've followed our [AI policy](AI_POLICY.md). - [ ] Make sure all continuous integration (CI) checks pass. Fix any errors or warnings, or ask us about any CI results you don't understand. If the contributor license agreement (CLA) check fails, look for a comment from the meta-cla bot with instructions on how to sign the CLA. We aim to respond to all PRs in a timely manner, but please note we prioritise reviews for work that is highest priority (e.g. critical bug fixes, upcoming milestones). If you're waiting on a review for more than a week, feel free to `@` one of the [maintainers](https://github.com/facebook/pyrefly/blob/main/.github/owners.json) in a comment on your PR or ask for a review in the `#dev` channel of our Discord server. From bf611aa320edf4d0899e1cfc6f21f17dfb5a2258 Mon Sep 17 00:00:00 2001 From: Rebecca Chen Date: Fri, 30 Oct 2026 01:11:54 -0700 Subject: [PATCH 2/4] Recommend splitting large PRs in the PR readiness check (#5459) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: The automated readiness check on new PRs now prints an optional recommendation to split PRs that are large (150+ lines of non-test code) or extra-large (300+ lines of non-test code). For large PRs, we include a gentle nudge to split into separate PRs if possible. For extra-large PRs, we include a stronger nudge to split into separate PRs or small commits. Here's what the comment looks like: If the PR passes all other checks but is large: ✅ This PR meets all requirements from our [pull request checklist](https://github.com/facebook/pyrefly/blob/main/CONTRIBUTING.md#making-a-pull-request). **Optional recommendations**: - This PR is on the larger side. If parts of it could be merged on their own, consider [splitting it](https://github.com/facebook/pyrefly/blob/main/CONTRIBUTING.md#splitting-a-pull-request) into separate PRs for faster review. If you've already split it as far as is practical, no action is needed. Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour. If it passes all other checks but is extra-large: ✅ This PR meets all requirements from our [pull request checklist](https://github.com/facebook/pyrefly/blob/main/CONTRIBUTING.md#making-a-pull-request). **Optional recommendations**: - This PR is very large, which makes it slow and difficult to review. We strongly recommend [splitting it](https://github.com/facebook/pyrefly/blob/main/CONTRIBUTING.md#splitting-a-pull-request) into smaller PRs that can be merged independently. If that isn't possible, please structure it as a series of small commits. If you've already split it as far as is practical, no action is needed. Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour. If the PR fails some other check and is also large: ⚠️ **Action needed**: this PR is missing required items from our [pull request checklist](https://github.com/facebook/pyrefly/blob/main/CONTRIBUTING.md#making-a-pull-request): - Check the "I am a human" box in the PR description. See our [AI policy](https://github.com/facebook/pyrefly/blob/main/AI_POLICY.md). **Optional recommendations**: - This PR is on the larger side. If parts of it could be merged on their own, consider [splitting it](https://github.com/facebook/pyrefly/blob/main/CONTRIBUTING.md#splitting-a-pull-request) into separate PRs for faster review. If you've already split it as far as is practical, no action is needed. Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour. Differential Revision: D128528925 --- .github/workflows/automation_pr_state.yml | 28 ++++++++++++++++------- CONTRIBUTING.md | 2 +- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/.github/workflows/automation_pr_state.yml b/.github/workflows/automation_pr_state.yml index 10bcc2d63d..9f88de2673 100644 --- a/.github/workflows/automation_pr_state.yml +++ b/.github/workflows/automation_pr_state.yml @@ -63,7 +63,7 @@ jobs: select-prs: if: >- - ${{ (!contains(fromJSON('["opened", "synchronize", "labeled", "unlabeled"]'), github.event.action) + ${{ (!contains(fromJSON('["labeled", "unlabeled"]'), github.event.action) || github.event.label.name == 'cla signed') && (github.event_name != 'issue_comment' || (github.event.issue.pull_request != null @@ -150,8 +150,9 @@ jobs: .map(pr => pr.number); evaluate: - needs: select-prs - if: ${{ needs.select-prs.outputs.prs != '[]' }} + # Waits for the size job so that the checklist sees the current size label. + needs: [select-prs, size] + if: ${{ !cancelled() && needs.select-prs.result == 'success' && needs.select-prs.outputs.prs != '[]' }} strategy: fail-fast: false max-parallel: 4 @@ -212,8 +213,10 @@ jobs: const sweep = context.eventName === 'schedule' || context.payload.inputs?.scenario === 'Reconcile PRs updated since last sweep'; const manual = context.eventName === 'workflow_dispatch' && !sweep; + // "opened" runs wait for the size job, so they recheck PRs whose + // "cla signed" label arrived before the size label. const initialCheck = (context.eventName === 'pull_request_target' - && (context.payload.action === 'ready_for_review' + && (['opened', 'ready_for_review'].includes(context.payload.action) || (context.payload.action === 'labeled' && context.payload.label.name === 'cla signed'))) || (sweep && timeline.some(e => (e.event === 'ready_for_review' || (e.event === 'labeled' && e.label?.name === 'cla signed')) @@ -233,16 +236,25 @@ jobs: ? `Add this checkbox to the PR description and check it: \`- [x] I am a human, and I fully understand the code I am submitting.\` ${policy}` : `Check the "I am a human" box in the PR description. ${policy}`; } + const CONTRIBUTING = `https://github.com/${owner}/${repo}/blob/main/CONTRIBUTING.md`; + const split = `[splitting it](${CONTRIBUTING}#splitting-a-pull-request)`; + let recommendation; + if (labels.includes('size/xl')) { + recommendation = `This PR is very large, which makes it slow and difficult to review. We strongly recommend ${split} into smaller PRs that can be merged independently. If that isn't possible, please structure it as a series of small commits. If you've already split it as far as is practical, no action is needed.`; + } else if (labels.includes('size/l')) { + recommendation = `This PR is on the larger side. If parts of it could be merged on their own, consider ${split} into separate PRs for faster review. If you've already split it as far as is practical, no action is needed.`; + } - const CHECKLIST = `[pull request checklist](https://github.com/${owner}/${repo}/blob/main/CONTRIBUTING.md#making-a-pull-request)`; + const CHECKLIST = `[pull request checklist](${CONTRIBUTING}#making-a-pull-request)`; const report = [ MARKER, problem ? `⚠️ **Action needed**: this PR is missing required items from our ${CHECKLIST}:\n\n- ${problem}` - : `✅ This PR follows our ${CHECKLIST}.`, + : `✅ This PR meets all requirements from our ${CHECKLIST}.`, + recommendation && `**Optional recommendations**:\n\n- ${recommendation}`, 'Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour.', - ].join('\n\n'); - const writeComment = existingCheck ? existingCheck.body !== report : Boolean(problem); + ].filter(Boolean).join('\n\n'); + const writeComment = existingCheck ? existingCheck.body !== report : Boolean(problem || recommendation); if (!dryRun && writeComment) { if (existingCheck) { await github.rest.issues.updateComment({ owner, repo, comment_id: existingCheck.id, body: report }); diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a718834eab..29ba269c3b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -56,7 +56,7 @@ How it works: ### PR readiness checks -Automated readiness checks run on newly opened PRs, checking items from our [PR checklist](#making-a-pull-request). If an "⚠️ **Action needed**" comment is posted on your PR, please follow the steps in the comment to ensure the PR is ready for review. The check automatically reruns when the PR is updated, with a lag of up to half an hour. +Automated readiness checks run on newly opened PRs, checking items from our [PR checklist](#making-a-pull-request). If an "⚠️ **Action needed**" comment is posted on your PR, please follow the required steps in the comment to ensure the PR is ready for review. The comment may also include optional recommendations, which don't block review. The check automatically reruns when the PR is updated, with a lag of up to half an hour. ### PR turn tracking From 50450c78efa152c1915d606aca2de49affbac7df Mon Sep 17 00:00:00 2001 From: Rebecca Chen Date: Fri, 30 Oct 2026 01:11:54 -0700 Subject: [PATCH 3/4] Require PR authors to claim the issues they reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Updates the PR readiness check to check that the PR author has claimed the issue they are working on: * If the PR description references at least one open, unclaimed issue, claiming the issue is listed as a required action. * If the PR description references only issues claimed by other people, getting one of the issues reassigned to the PR author is listed as a required action. * If the PR description references no issues or only closed issues, referencing and claiming an issue is listed as an optional action. It's maybe a little weird that we're stricter about referenced issues than not referencing an issue at all? My thinking is that we haven't really had problems with PRs not referencing issues, and it would be annoying if, say, someone who just wanted to fix a typo in our README got a comment saying that opening an issue first was required. Examples of posted comments: When the description references an open, unclaimed issue: ⚠️ **Action needed**: this PR is missing required items from our [pull request checklist](https://github.com/meta/pyrefly/blob/main/CONTRIBUTING.md#making-a-pull-request): - [Claim](https://github.com/meta/pyrefly/blob/main/CONTRIBUTING.md#claiming-issues-claim--unclaim) the issue that this PR addresses by commenting `#claim` on it. Unclaimed issues referenced in the PR description: #101. Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour. When the description references an issue claimed by someone else: ⚠️ **Action needed**: this PR is missing required items from our [pull request checklist](https://github.com/meta/pyrefly/blob/main/CONTRIBUTING.md#making-a-pull-request): - Every open issue referenced in the PR description is already assigned to someone else: #101. If you're working on one of these issues, please [request](https://github.com/meta/pyrefly/blob/main/CONTRIBUTING.md#claiming-issues-claim--unclaim) that it be reassigned to you. Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour. When the description does not reference an issue: ✅ This PR meets all requirements from our [pull request checklist](https://github.com/meta/pyrefly/blob/main/CONTRIBUTING.md#making-a-pull-request). **Optional recommendations**: - If this PR addresses an open issue, please link the issue in the PR description and [claim](https://github.com/meta/pyrefly/blob/main/CONTRIBUTING.md#claiming-issues-claim--unclaim) it. Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour. When the description references only closed issues: ✅ This PR meets all requirements from our [pull request checklist](https://github.com/meta/pyrefly/blob/main/CONTRIBUTING.md#making-a-pull-request). **Optional recommendations**: - Every issue referenced in the PR description is already closed: #101. If this PR addresses an open issue, please link the issue in the PR description and [claim](https://github.com/meta/pyrefly/blob/main/CONTRIBUTING.md#claiming-issues-claim--unclaim) it. Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour. Differential Revision: D128542616 --- .github/workflows/automation_pr_state.yml | 81 +++++++++++++++++------ 1 file changed, 62 insertions(+), 19 deletions(-) diff --git a/.github/workflows/automation_pr_state.yml b/.github/workflows/automation_pr_state.yml index 9f88de2673..6f94595d32 100644 --- a/.github/workflows/automation_pr_state.yml +++ b/.github/workflows/automation_pr_state.yml @@ -77,6 +77,7 @@ jobs: permissions: actions: read # find the last successful sweep contents: read # read owners.json once for the run + issues: read # find issues updated since the last sweep pull-requests: read outputs: prs: ${{ steps.select.outputs.result }} @@ -131,15 +132,24 @@ jobs: core.setOutput('since', Number.isFinite(since) ? since : Date.now()); } const prs = await github.paginate(github.rest.pulls.list, { - owner, repo, state: 'open', sort: 'updated', direction: 'desc', per_page: 100, - }, (response, done) => { - const page = response.data.filter(pr => Date.parse(pr.updated_at) >= since); - if (page.length < response.data.length) done(); - return page; + owner, repo, state: 'open', per_page: 100, }); if (sweep) { - // An update during pagination can list a PR on two pages. - return [...new Set(prs.filter(pr => pr.user.type !== 'Bot' + // Claiming an issue doesn't update the PR, so also select PRs + // whose authors are assigned to issues updated since the last + // sweep. Without a last sweep, every open PR is selected anyway. + const assignees = new Set(); + if (Number.isFinite(since)) { + for (const issue of await github.paginate(github.rest.issues.listForRepo, { + owner, repo, state: 'open', since: new Date(since).toISOString(), per_page: 100, + })) { + if (!issue.pull_request) issue.assignees.forEach(a => assignees.add(a.login.toLowerCase())); + } + } + // A PR opened during pagination can list another PR on two pages. + return [...new Set(prs.filter(pr => (Date.parse(pr.updated_at) >= since + || assignees.has(pr.user.login.toLowerCase())) + && pr.user.type !== 'Bot' && !pr.head.ref.startsWith('export-D') && !owners.includes(pr.user.login.toLowerCase())) .map(pr => pr.number))]; @@ -164,6 +174,7 @@ jobs: runs-on: ubuntu-latest permissions: pull-requests: write # read the PR, reviews, labels, and timeline, update labels, and comment + issues: read # read issues referenced in the PR description checks: read # distinguish unsigned CLAs from pending checks steps: # The next step reacts to this step's result: "passed", "failed", or @@ -189,7 +200,10 @@ jobs: const issue_number = pull_number; const MARKER = ''; - const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number }); + // The "full" format adds `body_html`, the rendered description. + const { data: pr } = await github.rest.pulls.get({ + owner, repo, pull_number, mediaType: { format: 'full' }, + }); const owners = JSON.parse(process.env.OWNERS_JSON); const author = pr.user.login.toLowerCase(); const labels = (await github.paginate(github.rest.issues.listLabelsOnIssue, { @@ -229,32 +243,61 @@ jobs: // it is missing. const box = (pr.body || '').replace(//g, '') .match(/^\s*[-*]\s*\[([ x])\]\s*I am a human/im)?.[1].toLowerCase(); - let problem; + const problems = []; if (box !== 'x') { const policy = `See our [AI policy](https://github.com/${owner}/${repo}/blob/main/AI_POLICY.md).`; - problem = box === undefined + problems.push(box === undefined ? `Add this checkbox to the PR description and check it: \`- [x] I am a human, and I fully understand the code I am submitting.\` ${policy}` - : `Check the "I am a human" box in the PR description. ${policy}`; + : `Check the "I am a human" box in the PR description. ${policy}`); } const CONTRIBUTING = `https://github.com/${owner}/${repo}/blob/main/CONTRIBUTING.md`; + // Issues referenced in the description are read from the links that + // GitHub renders for them, so that GitHub decides what counts as a + // reference. + const issueNumbers = new Set([...(pr.body_html || '').matchAll( + new RegExp(`data-hovercard-url="/${pr.base.repo.full_name}/issues/(\\d+)/hovercard"`, 'gi'))] + .map(([, number]) => Number(number))); + const referenced = []; + for (const number of issueNumbers) { + const { data: issue } = await github.rest.issues.get({ owner, repo, issue_number: number }); + referenced.push(issue); + } + const issues = referenced.filter(i => i.state === 'open'); + const CLAIMING = `${CONTRIBUTING}#claiming-issues-claim--unclaim`; + const refs = list => list.map(i => `#${i.number}`).join(', '); + const recommendations = []; + if (issues.length === 0) { + const allClosed = referenced.length > 0 + ? `Every issue referenced in the PR description is already closed: ${refs(referenced)}. ` + : ''; + recommendations.push(`${allClosed}If this PR addresses an open issue, please link the issue in the PR description and [claim](${CLAIMING}) it.`); + } else if (!issues.some(i => i.assignees.some(a => a.login.toLowerCase() === author))) { + const unassigned = issues.filter(i => i.assignees.length === 0); + // `#claim` refuses assigned issues, so when every issue is + // assigned, the author is pointed to the reassignment process + // instead. Assignees are not mentioned, to avoid notifying them. + problems.push(unassigned.length > 0 + ? `[Claim](${CLAIMING}) the issue that this PR addresses by commenting \`#claim\` on it. Unclaimed issues referenced in the PR description: ${refs(unassigned)}.` + : `Every open issue referenced in the PR description is already assigned to someone else: ${refs(issues)}. If you're working on one of these issues, please [request](${CLAIMING}) that it be reassigned to you.`); + } const split = `[splitting it](${CONTRIBUTING}#splitting-a-pull-request)`; - let recommendation; if (labels.includes('size/xl')) { - recommendation = `This PR is very large, which makes it slow and difficult to review. We strongly recommend ${split} into smaller PRs that can be merged independently. If that isn't possible, please structure it as a series of small commits. If you've already split it as far as is practical, no action is needed.`; + recommendations.push(`This PR is very large, which makes it slow and difficult to review. We strongly recommend ${split} into smaller PRs that can be merged independently. If that isn't possible, please structure it as a series of small commits. If you've already split it as far as is practical, no action is needed.`); } else if (labels.includes('size/l')) { - recommendation = `This PR is on the larger side. If parts of it could be merged on their own, consider ${split} into separate PRs for faster review. If you've already split it as far as is practical, no action is needed.`; + recommendations.push(`This PR is on the larger side. If parts of it could be merged on their own, consider ${split} into separate PRs for faster review. If you've already split it as far as is practical, no action is needed.`); } const CHECKLIST = `[pull request checklist](${CONTRIBUTING}#making-a-pull-request)`; + const list = items => items.map(item => `- ${item}`).join('\n'); const report = [ MARKER, - problem - ? `⚠️ **Action needed**: this PR is missing required items from our ${CHECKLIST}:\n\n- ${problem}` + problems.length > 0 + ? `⚠️ **Action needed**: this PR is missing required items from our ${CHECKLIST}:\n\n${list(problems)}` : `✅ This PR meets all requirements from our ${CHECKLIST}.`, - recommendation && `**Optional recommendations**:\n\n- ${recommendation}`, + recommendations.length > 0 && `**Optional recommendations**:\n\n${list(recommendations)}`, 'Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour.', ].filter(Boolean).join('\n\n'); - const writeComment = existingCheck ? existingCheck.body !== report : Boolean(problem || recommendation); + const writeComment = existingCheck ? existingCheck.body !== report : problems.length + recommendations.length > 0; if (!dryRun && writeComment) { if (existingCheck) { await github.rest.issues.updateComment({ owner, repo, comment_id: existingCheck.id, body: report }); @@ -262,7 +305,7 @@ jobs: await github.rest.issues.createComment({ owner, repo, issue_number, body: report }); } } - const result = problem ? 'failed' : 'passed'; + const result = problems.length > 0 ? 'failed' : 'passed'; const comment = !writeComment ? (existingCheck ? 'comment unchanged' : 'no comment') : `comment ${dryRun ? 'would be ' : ''}${existingCheck ? 'updated' : 'posted'}`; core.setOutput('summary', `${result}, ${comment}`); From bce0294990cb49ebae937e16215153282bf776dc Mon Sep 17 00:00:00 2001 From: Rebecca Chen Date: Fri, 30 Oct 2026 01:26:27 -0700 Subject: [PATCH 4/4] TEST TEST TEST --- .github/owners.json | 1 - .github/workflows/automation_pr_state.yml | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/owners.json b/.github/owners.json index 50515346f2..de07dc7ce0 100644 --- a/.github/owners.json +++ b/.github/owners.json @@ -3,7 +3,6 @@ "stroxler", "kinto0", "samwgoldman", - "rchen152", "lolpack", "connernilsen", "maggiemoss", diff --git a/.github/workflows/automation_pr_state.yml b/.github/workflows/automation_pr_state.yml index 6f94595d32..448bdb64c6 100644 --- a/.github/workflows/automation_pr_state.yml +++ b/.github/workflows/automation_pr_state.yml @@ -123,7 +123,7 @@ jobs: if (sweep) { const { data: { workflow_runs: [last] } } = await github.rest.actions.listWorkflowRuns({ owner, repo, workflow_id: 'automation_pr_state.yml', - event: 'schedule', status: 'success', per_page: 1, + event: 'workflow_dispatch', status: 'success', per_page: 1, }); // A run is created before it selects PRs, even when it is re-run, // so every update after the previous sweep's selection is covered.