diff --git a/.github/workflows/automation-group-pull-request-target.yml b/.github/workflows/automation-group-pull-request-target.yml index fcedadd..d9bf0f2 100644 --- a/.github/workflows/automation-group-pull-request-target.yml +++ b/.github/workflows/automation-group-pull-request-target.yml @@ -46,7 +46,7 @@ jobs: LE_BOT_PRIVATE_KEY: ${{ secrets.LE_BOT_PRIVATE_KEY }} contributor-pr-reply: name: Contributor PR Reply - if: ${{ github.event_name == 'pull_request_target' && github.event.action == 'opened' }} + if: ${{ github.event_name == 'pull_request_target' && (github.event.action == 'opened' || (github.event.action == 'edited' && github.event.changes.body)) }} uses: learningequality/.github/.github/workflows/contributor-pr-reply.yml@main permissions: contents: read diff --git a/.github/workflows/automation.yml b/.github/workflows/automation.yml index 70def85..8c7e19a 100644 --- a/.github/workflows/automation.yml +++ b/.github/workflows/automation.yml @@ -32,7 +32,7 @@ on: jobs: pull-request-target: name: Pull Request Target - if: ${{ (github.event_name == 'pull_request_target' && github.event.action == 'review_requested') || (github.event_name == 'pull_request_target' && github.event.action == 'labeled') || (github.event_name == 'pull_request_target' && github.event.action == 'opened' && github.event.pull_request.user.login == 'dependabot[bot]') || (github.event_name == 'pull_request_target' && github.event.action == 'opened') || (github.event_name == 'pull_request_target' && contains(fromJSON('["opened","edited","reopened"]'), github.event.action)) }} + if: ${{ (github.event_name == 'pull_request_target' && github.event.action == 'review_requested') || (github.event_name == 'pull_request_target' && github.event.action == 'labeled') || (github.event_name == 'pull_request_target' && github.event.action == 'opened' && github.event.pull_request.user.login == 'dependabot[bot]') || (github.event_name == 'pull_request_target' && (github.event.action == 'opened' || (github.event.action == 'edited' && github.event.changes.body))) || (github.event_name == 'pull_request_target' && contains(fromJSON('["opened","edited","reopened"]'), github.event.action)) }} uses: learningequality/.github/.github/workflows/automation-group-pull-request-target.yml@main permissions: contents: read diff --git a/.github/workflows/contributor-pr-reply.yml b/.github/workflows/contributor-pr-reply.yml index ed7e387..071dace 100644 --- a/.github/workflows/contributor-pr-reply.yml +++ b/.github/workflows/contributor-pr-reply.yml @@ -28,6 +28,9 @@ jobs: needs: [check-if-contributor] if: ${{ needs.check-if-contributor.outputs.is_contributor == 'true' }} runs-on: ubuntu-latest + concurrency: + group: contributor-pr-reply-${{ github.event.pull_request.number }}-${{ github.event.action }} + cancel-in-progress: false steps: - name: Generate App Token id: generate-token diff --git a/.github/workflows/unassign-inactive-issues.yaml b/.github/workflows/unassign-inactive-issues.yaml index 8952c14..7ed5f95 100644 --- a/.github/workflows/unassign-inactive-issues.yaml +++ b/.github/workflows/unassign-inactive-issues.yaml @@ -24,7 +24,7 @@ jobs: private-key: ${{ secrets.LE_BOT_PRIVATE_KEY }} permission-issues: write permission-members: read - permission-pull-requests: read + permission-pull-requests: write - name: Checkout .github repository uses: actions/checkout@v7 with: diff --git a/automation-registry.yml b/automation-registry.yml index 3ca3ab6..dee6802 100644 --- a/automation-registry.yml +++ b/automation-registry.yml @@ -57,8 +57,8 @@ automations: leaf: contributor-pr-reply.yml on: pull_request_target: - types: [opened] - if: "github.event_name == 'pull_request_target' && github.event.action == 'opened'" + types: [opened, edited] + if: "github.event_name == 'pull_request_target' && (github.event.action == 'opened' || (github.event.action == 'edited' && github.event.changes.body))" secrets: LE_BOT_APP_ID: required LE_BOT_PRIVATE_KEY: required diff --git a/docs/community-automations.md b/docs/community-automations.md index 25860db..2af3466 100644 --- a/docs/community-automations.md +++ b/docs/community-automations.md @@ -65,18 +65,23 @@ Sends reply to a community pull request, and acts on the issue that pull request What happens depends on who the linked issue is assigned to: -- assigned to someone else: posts a closing message instead of the reply, closes the pull request, and notifies Slack - assigned to the author: sends the reply, then requests `rtibblesbot` for review and adds the `community-review` label -- not assigned, or no issue linked: sends the reply +- assigned to someone else, or to no one: posts a closing message instead of the reply, closes the pull request, and notifies Slack +- no issue linked: sends the reply, asks for an issue to be linked under `## References`, and adds the `unlinked` label -An issue counts as linked only when the description closes it, for example `Fixes #123`. Assignments on any other issue are ignored. A pull request with no linked issue is asked to add one under `## References`. +An issue counts as linked only when the description closes it, for example `Fixes #123`. Assignments on any other issue are ignored. If the author is assigned to any linked issue, the pull request is not closed. Close contributors are never closed or labelled `unlinked`. -If the author is assigned to any linked issue, the pull request is not closed. +It runs on `opened`, and again on `edited` when the description changes. An edit sends no reply: it closes, labels, or removes `unlinked` by the rules above, and starts review only on a pull request that carried `unlinked`. + +The weekly `unassign-inactive-issues` run closes every open pull request labelled `unlinked` for longer than `PR_UNLINKED_MAX_DAYS` days, except a close contributor's. It re-reads the links first, because a link added from the Development sidebar fires no `edited` event: a pull request now linked to its author's issue loses the label and starts review instead. In `scripts/constants.js` set: - `BOT_MESSAGE_PULL_REQUEST`: Message text - `BOT_MESSAGE_LINK_ISSUE`: Text added when no issue is linked -- `BOT_MESSAGE_PULL_REQUEST_CLOSED`: Closing message text +- `BOT_MESSAGE_PULL_REQUEST_CLOSED`: Closing message when the issue is assigned to someone else +- `BOT_MESSAGE_PULL_REQUEST_UNASSIGNED`: Closing message when the issue is assigned to no one +- `BOT_MESSAGE_PULL_REQUEST_UNLINKED_CLOSED`: Closing message from the sweep when still no issue is linked +- `PR_UNLINKED_MAX_DAYS`: How long `unlinked` stays on before the sweep closes the pull request # `pull-request-template` diff --git a/scripts/constants.js b/scripts/constants.js index 58bdb8f..3675fff 100644 --- a/scripts/constants.js +++ b/scripts/constants.js @@ -163,7 +163,18 @@ const BOT_MESSAGE_KEYWORD_GOOD_FIRST_ISSUE = const BOT_MESSAGE_PULL_REQUEST = author => `👋 Hi @${author}, thanks for contributing! \n\n **For the review process to begin, please verify that the following is satisfied:**\n\n- [ ] **Contribution is aligned with our [contributing guidelines](https://learningequality.org/contributing-to-our-open-code-base)**\n- [ ] **Pull request description has correctly filled _AI usage_ section & follows our AI guidance:**\n\n
\n AI guidance\n\n
\n\n **State explicitly whether you didn't use or used AI & how.**\n\n If you used it, ensure that the PR is aligned with [Using AI](https://learningequality.org/contributing-to-our-open-code-base/#using-generative-ai) as well as our DEEP framework. DEEP asks you:\n\n - **Disclose** — Be open about when you've used AI for support.\n - **Engage critically** — Question what is generated. Review code for correctness and unnecessary complexity.\n - **Edit** — Review and refine AI output. Remove unnecessary code and verify it still works after your edits.\n - **Process sharing** — Explain how you used the AI so others can learn.\n\n
\n\n Examples of good disclosures:\n\n > "I used Claude Code to implement the component, prompting it to follow the pattern in ComponentX. I reviewed the generated code, removed unnecessary error handling, and verified the tests pass."\n\n > "I brainstormed the approach with Gemini, then had it write failing tests for the feature. After reviewing the tests, I used Claude Code to generate the implementation. I refactored the output to reduce verbosity and ran the full test suite."\n\n
\n\n**📢✨ Before we assign a reviewer:**\n\n- **\`@rtibblesbot\` will pre-review this pull request.** Its comments are generated by an LLM, and should be evaluated accordingly.\n- **We'll also invite community pre-review.** See the [community review guidance](https://github.com/learningequality/.github/blob/main/docs/community-review.md) for both authors and reviewers.\n\nAlso check that issue requirements are satisfied & you ran \`pre-commit\` locally.\n\n**Pull requests that don't follow the guidelines will be closed.**\n\n**Reviewer assignment can take up to 2 weeks.**`; -const BOT_MESSAGE_LINK_ISSUE = `**📎 We couldn't find an issue linked to this pull request.** Please edit the description and link one under **References**, for example \`Fixes #123\` or \`Closes #123\`. A plain \`#123\` mention doesn't link the PR to the issue.`; +const LABEL_UNLINKED = 'unlinked'; +const PR_UNLINKED_MAX_DAYS = 7; + +const BOT_MESSAGE_LINK_ISSUE = `**📎 We couldn't find an issue linked to this pull request.** Please edit the description and link the issue assigned to you under **References**, for example \`Fixes #123\` or \`Closes #123\`. A plain \`#123\` mention doesn't link the PR to the issue. **Pull requests with no linked issue are closed after ${PR_UNLINKED_MAX_DAYS} days.**`; + +const BOT_MESSAGE_PULL_REQUEST_UNASSIGNED = (author, issueUrl) => + `👋 Hi @${author}, thanks for your interest! \n\n **We're closing this pull request because ${issueUrl} isn't assigned to you.** We check and assign issues before work on them starts. Comment on the issue to ask to be assigned, and open a new pull request once you are. Visit [Contributing guidelines](https://learningequality.org/contributing-to-our-open-code-base) to learn about the contributing process.${GSOC_NOTE}`; + +const BOT_MESSAGE_PULL_REQUEST_UNLINKED_CLOSED = + `**Closing this pull request because no issue has been linked to it after ${PR_UNLINKED_MAX_DAYS} days.** ` + + `Pull requests must close an issue assigned to their author. Once you're assigned to one, ` + + `open a new pull request that links it.`; const BOT_MESSAGE_PULL_REQUEST_CLOSED = (author, issueUrl) => `👋 Hi @${author}, thanks for your interest! \n\n **We're closing this pull request because ${issueUrl} is assigned to someone else.** Visit [Contributing guidelines](https://learningequality.org/contributing-to-our-open-code-base) to learn about the contributing process and how to find suitable issues. If there are no unassigned 'help wanted' issues available, please wait until new ones are added. \n\n We really appreciate your willingness to help. 😊${GSOC_NOTE}`; @@ -244,6 +255,10 @@ module.exports = { BOT_MESSAGE_PULL_REQUEST, BOT_MESSAGE_LINK_ISSUE, BOT_MESSAGE_PULL_REQUEST_CLOSED, + BOT_MESSAGE_PULL_REQUEST_UNASSIGNED, + BOT_MESSAGE_PULL_REQUEST_UNLINKED_CLOSED, + LABEL_UNLINKED, + PR_UNLINKED_MAX_DAYS, BOT_MESSAGE_RTIBBLESBOT_REVIEW, RTIBBLESBOT_USERNAME, LABEL_COMMUNITY_REVIEW, diff --git a/scripts/contributor-pr-reply.js b/scripts/contributor-pr-reply.js index e16c5c7..78b7021 100644 --- a/scripts/contributor-pr-reply.js +++ b/scripts/contributor-pr-reply.js @@ -1,88 +1,85 @@ -const { - BOT_MESSAGE_PULL_REQUEST, - BOT_MESSAGE_LINK_ISSUE, - BOT_MESSAGE_PULL_REQUEST_CLOSED, - LABEL_COMMUNITY_REVIEW, - RTIBBLESBOT_USERNAME, -} = require('./constants'); +const { BOT_MESSAGE_PULL_REQUEST, BOT_MESSAGE_LINK_ISSUE } = require('./constants'); const { sendBotMessage, getLinkedIssues, escapeIssueTitleForSlackMessage } = require('./utils'); +const { linkStatus, planLink, applyLink } = require('./pull-request-link'); + +function slackLine(context, text) { + const { repo } = context.repo; + const { html_url: url, title } = context.payload.pull_request; + return `*[${repo}] ${text}: <${url}|${escapeIssueTitleForSlackMessage(title)}>*`; +} + +async function sendReply({ github, context, core }, unlinked) { + const { number, user } = context.payload.pull_request; + const body = unlinked + ? `${BOT_MESSAGE_PULL_REQUEST(user.login)}\n\n${BOT_MESSAGE_LINK_ISSUE}` + : BOT_MESSAGE_PULL_REQUEST(user.login); + const botMessageUrl = await sendBotMessage(number, body, { github, context, core }); + core.setOutput( + 'slack_notification', + slackLine(context, `<${botMessageUrl}|Reply sent> on pull request`), + ); +} + +async function act({ github, context, core }, plan, link) { + const { owner, repo } = context.repo; + const { number } = context.payload.pull_request; + try { + const closingUrl = await applyLink(owner, repo, number, plan, { github, core }); + if (plan.action === 'close') { + const reason = link.status === 'unassigned' ? 'unassigned' : 'assigned'; + core.setOutput( + 'slack_notification', + slackLine( + context, + `<${closingUrl}|Closed> pull request on ${reason} issue <${link.issue.url}|#${link.issue.number}>`, + ), + ); + } + } catch (error) { + if (plan.action === 'close') { + core.setOutput('slack_notification', slackLine(context, 'Failed to close pull request')); + } + core.setFailed(`Failed to ${plan.action} the pull request: ${error.message}`); + } +} module.exports = async ({ github, context, core }) => { try { - const owner = context.repo.owner; - const repo = context.repo.repo; - const number = context.payload.pull_request.number; - const url = context.payload.pull_request.html_url; - const title = escapeIssueTitleForSlackMessage(context.payload.pull_request.title); - const author = context.payload.pull_request.user.login; + const { owner, repo } = context.repo; + const pullRequest = context.payload.pull_request; + const { number } = pullRequest; + const author = pullRequest.user.login; + const opening = context.payload.action !== 'edited'; + core.setOutput('slack_notification', ''); - const lookup = await getLinkedIssues(number, { github, context, core }); - const linkedIssues = lookup || []; - const authorAssigned = linkedIssues.some(issue => issue.assignees.includes(author)); - const assignedElsewhere = linkedIssues.find( - issue => issue.assignees.length > 0 && !issue.assignees.includes(author), - ); + if (!opening && pullRequest.state !== 'open') { + core.info(`Pull request #${number} is ${pullRequest.state}.`); + return; + } - if (!authorAssigned && assignedElsewhere) { - const botMessageUrl = await sendBotMessage( - number, - BOT_MESSAGE_PULL_REQUEST_CLOSED(author, assignedElsewhere.url), - { github, context, core }, - ); - try { - await github.rest.pulls.update({ - owner, - repo, - pull_number: number, - state: 'closed', - }); - core.setOutput( - 'slack_notification', - `*[${repo}] <${botMessageUrl}|Closed> pull request on assigned issue <${assignedElsewhere.url}|#${assignedElsewhere.number}>: <${url}|${title}>*`, - ); - } catch (error) { - core.setOutput( - 'slack_notification', - `*[${repo}] Failed to close pull request <${url}|${title}>, but the closing message was posted*`, - ); - core.setFailed(`Failed to close the pull request: ${error.message}`); + const plan = async link => + planLink(owner, repo, pullRequest, link, { github, core }, { opening }); + + const lookup = await getLinkedIssues(number, { github, context, core }); + if (lookup === null) { + if (opening) { + await sendReply({ github, context, core }, false); } return; } - const reply = - lookup !== null && lookup.length === 0 - ? `${BOT_MESSAGE_PULL_REQUEST(author)}\n\n${BOT_MESSAGE_LINK_ISSUE}` - : BOT_MESSAGE_PULL_REQUEST(author); - const botMessageUrl = await sendBotMessage(number, reply, { - github, - context, - core, - }); - - if (botMessageUrl) { - const slackMessage = `*[${repo}] <${botMessageUrl}|Reply sent> on pull request: <${url}|${title}>*`; - core.setOutput('slack_notification', slackMessage); - } else { - core.setOutput('slack_notification', ''); + const link = linkStatus(lookup, author); + const first = await plan(link); + if (opening && first.action !== 'close') { + await sendReply({ github, context, core }, link.status === 'unlinked'); } + await act({ github, context, core }, first, link); - if (authorAssigned) { - try { - await github.rest.pulls.requestReviewers({ - owner, - repo, - pull_number: number, - reviewers: [RTIBBLESBOT_USERNAME], - }); - await github.rest.issues.addLabels({ - owner, - repo, - issue_number: number, - labels: [LABEL_COMMUNITY_REVIEW], - }); - } catch (error) { - core.setFailed(`Failed to request review or add the label: ${error.message}`); + if (opening && first.action === 'label') { + const relookup = await getLinkedIssues(number, { github, context, core }); + if (relookup?.length) { + const relink = linkStatus(relookup, author); + await act({ github, context, core }, await plan(relink), relink); } } } catch (error) { diff --git a/scripts/contributor-pr-reply.test.js b/scripts/contributor-pr-reply.test.js index 042524d..6013989 100644 --- a/scripts/contributor-pr-reply.test.js +++ b/scripts/contributor-pr-reply.test.js @@ -2,24 +2,40 @@ const test = require('node:test'); const assert = require('node:assert/strict'); const contributorPrReply = require('./contributor-pr-reply'); -const { LABEL_COMMUNITY_REVIEW, RTIBBLESBOT_USERNAME } = require('./constants'); +const { + LABEL_COMMUNITY_REVIEW, + LABEL_UNLINKED, + RTIBBLESBOT_USERNAME, + CLOSE_CONTRIBUTORS, +} = require('./constants'); const AUTHOR = 'outsider'; +const CLOSE_CONTRIBUTOR = CLOSE_CONTRIBUTORS[0]; -function fakeContext(author = AUTHOR, title = 'Fix it') { +function fakeContext( + author = AUTHOR, + title = 'Fix it', + { action = 'opened', state = 'open', labels = [] } = {}, +) { return { repo: { owner: 'learningequality', repo: 'kolibri' }, payload: { + action, pull_request: { number: 42, html_url: 'https://github.com/learningequality/kolibri/pull/42', title, + state, + labels: labels.map(name => ({ name })), user: { login: author }, }, }, }; } +const edited = (author = AUTHOR, options = {}) => + fakeContext(author, 'Fix it', { action: 'edited', ...options }); + function fakeCore() { const outputs = {}; const warnings = []; @@ -41,19 +57,25 @@ function fakeCore() { }; } -function fakeGithub(linkedIssues = [], { graphqlError } = {}) { - const calls = { comments: [], updates: [], reviewers: [], labels: [], order: [] }; +function fakeGithub(linkedIssues = [], { graphqlError, relinked } = {}) { + const calls = { comments: [], updates: [], reviewers: [], labels: [], removed: [], order: [] }; + const current = new Set(); + let lookups = 0; return { calls, + current, + paginate: async (fn, params) => fn(params), graphql: async () => { if (graphqlError) { throw new Error(graphqlError); } + lookups += 1; + const issues = lookups > 1 && relinked ? relinked : linkedIssues; return { repository: { pullRequest: { closingIssuesReferences: { - nodes: linkedIssues.map(issue => ({ + nodes: issues.map(issue => ({ number: issue.number, title: `Issue ${issue.number}`, url: `https://github.com/learningequality/kolibri/issues/${issue.number}`, @@ -74,7 +96,13 @@ function fakeGithub(linkedIssues = [], { graphqlError } = {}) { addLabels: async ({ labels }) => { calls.labels.push(...labels); calls.order.push('label'); + labels.forEach(name => current.add(name)); + }, + removeLabel: async ({ name }) => { + calls.removed.push(name); + current.delete(name); }, + listLabelsOnIssue: async () => [...current].map(name => ({ name })), }, pulls: { update: async ({ pull_number, state }) => { @@ -91,6 +119,7 @@ function fakeGithub(linkedIssues = [], { graphqlError } = {}) { } async function run(github, context = fakeContext()) { + context.payload.pull_request.labels.forEach(({ name }) => github.current.add(name)); const core = fakeCore(); await contributorPrReply({ github, context, core }); return core; @@ -123,7 +152,7 @@ test('a failed close still reports to Slack', async () => { }; const core = await run(github); - assert.equal(github.calls.comments.length, 1); + assert.deepEqual(github.calls.comments, []); assert.match(core.outputs.slack_notification, /Failed to close/); assert.equal(core.failures.length, 1); }); @@ -185,38 +214,130 @@ test('an issue assigned to the author outweighs another assigned elsewhere', asy assert.deepEqual(github.calls.reviewers, [RTIBBLESBOT_USERNAME]); }); -test('no linked issue leaves the pull request alone', async () => { +test('no linked issue leaves the pull request open and labels it unlinked', async () => { const github = fakeGithub([]); const core = await run(github); assert.deepEqual(github.calls.updates, []); assert.deepEqual(github.calls.reviewers, []); - assert.deepEqual(github.calls.labels, []); + assert.deepEqual(github.calls.labels, [LABEL_UNLINKED]); assert.equal(github.calls.comments.length, 1); assert.match(core.outputs.slack_notification, /Reply sent/); }); -test('a linked issue with no assignee leaves the pull request alone', async () => { +test('a linked issue with no assignee closes the pull request', async () => { const github = fakeGithub([{ number: 7, assignees: [] }]); - await run(github); + const core = await run(github); - assert.deepEqual(github.calls.updates, []); + assert.deepEqual(github.calls.updates, [{ pull_number: 42, state: 'closed' }]); + assert.equal(github.calls.comments.length, 1); + assert.match(github.calls.comments[0].body, /issues\/7 isn't assigned to you/); + assert.match(core.outputs.slack_notification, /Closed> pull request on unassigned issue/); assert.deepEqual(github.calls.reviewers, []); - assert.deepEqual(github.calls.labels, []); }); test('an unlinked pull request is asked to link an issue', async () => { const github = fakeGithub([]); await run(github); - assert.match(github.calls.comments[0].body, /link one under \*\*References\*\*/); + assert.match(github.calls.comments[0].body, /link the issue assigned to you under/); + assert.match(github.calls.comments[0].body, /closed after 7 days/); }); test('a linked pull request is not asked to link an issue', async () => { - const github = fakeGithub([{ number: 7, assignees: [] }]); + const github = fakeGithub([{ number: 7, assignees: [AUTHOR] }]); await run(github); - assert.doesNotMatch(github.calls.comments[0].body, /link one under/); + assert.doesNotMatch(github.calls.comments[0].body, /link the issue/); +}); + +test('a close contributor is never closed', async () => { + for (const assignees of [[], ['someone-else']]) { + const github = fakeGithub([{ number: 7, assignees }]); + await run(github, fakeContext(CLOSE_CONTRIBUTOR)); + + assert.deepEqual(github.calls.updates, []); + assert.match(github.calls.comments[0].body, /For the review process to begin/); + } +}); + +test('a close contributor with no linked issue is not labelled unlinked', async () => { + const github = fakeGithub([]); + await run(github, fakeContext(CLOSE_CONTRIBUTOR)); + + assert.deepEqual(github.calls.labels, []); + assert.match(github.calls.comments[0].body, /link the issue/); +}); + +test('an edit linking the issue assigned to the author starts review without a reply', async () => { + const github = fakeGithub([{ number: 7, assignees: [AUTHOR] }]); + const core = await run(github, edited(AUTHOR, { labels: [LABEL_UNLINKED] })); + + assert.deepEqual(github.calls.comments, []); + assert.deepEqual(github.calls.removed, [LABEL_UNLINKED]); + assert.deepEqual(github.calls.reviewers, [RTIBBLESBOT_USERNAME]); + assert.deepEqual(github.calls.labels, [LABEL_COMMUNITY_REVIEW]); + assert.equal(core.outputs.slack_notification, ''); +}); + +test('a failed review request leaves the unlinked label for a retry', async () => { + const github = fakeGithub([{ number: 7, assignees: [AUTHOR] }]); + github.rest.pulls.requestReviewers = async () => { + throw new Error('Reviewer cannot be requested'); + }; + const core = await run(github, edited(AUTHOR, { labels: [LABEL_UNLINKED] })); + + assert.deepEqual(github.calls.removed, []); + assert.equal(core.failures.length, 1); +}); + +test('a link added while the opening run labels it unlinked still starts review', async () => { + const github = fakeGithub([], { relinked: [{ number: 7, assignees: [AUTHOR] }] }); + await run(github); + + assert.deepEqual(github.calls.reviewers, [RTIBBLESBOT_USERNAME]); + assert.deepEqual(github.calls.removed, [LABEL_UNLINKED]); + assert.deepEqual([...github.current], [LABEL_COMMUNITY_REVIEW]); +}); + +test('an edit to an assigned pull request that was never unlinked starts no review', async () => { + const github = fakeGithub([{ number: 7, assignees: [AUTHOR] }]); + await run(github, edited(AUTHOR, { labels: [] })); + + assert.deepEqual(github.calls.reviewers, []); + assert.deepEqual(github.calls.labels, []); +}); + +test('an edit to a pull request already under review requests no second review', async () => { + const github = fakeGithub([{ number: 7, assignees: [AUTHOR] }]); + await run(github, edited(AUTHOR, { labels: [LABEL_COMMUNITY_REVIEW] })); + + assert.deepEqual(github.calls.reviewers, []); + assert.deepEqual(github.calls.labels, []); +}); + +test('an edit linking an unassigned issue closes the pull request', async () => { + const github = fakeGithub([{ number: 7, assignees: [] }]); + await run(github, edited()); + + assert.deepEqual(github.calls.updates, [{ pull_number: 42, state: 'closed' }]); + assert.match(github.calls.comments[0].body, /isn't assigned to you/); +}); + +test('an edit that leaves the pull request unlinked labels it without a reply', async () => { + const github = fakeGithub([]); + await run(github, edited()); + + assert.deepEqual(github.calls.comments, []); + assert.deepEqual(github.calls.labels, [LABEL_UNLINKED]); +}); + +test('an edit to a closed pull request does nothing', async () => { + const github = fakeGithub([{ number: 7, assignees: [] }]); + await run(github, edited(AUTHOR, { state: 'closed' })); + + assert.deepEqual(github.calls.comments, []); + assert.deepEqual(github.calls.updates, []); }); test('a failed lookup falls back to the standard reply', async () => { @@ -251,5 +372,5 @@ test('a failed lookup does not ask the author to link an issue', async () => { const github = fakeGithub([], { graphqlError: 'API down' }); await run(github); - assert.doesNotMatch(github.calls.comments[0].body, /link one under/); + assert.doesNotMatch(github.calls.comments[0].body, /link the issue/); }); diff --git a/scripts/issue-review-sweep.js b/scripts/issue-review-sweep.js index 147df20..663c105 100644 --- a/scripts/issue-review-sweep.js +++ b/scripts/issue-review-sweep.js @@ -6,8 +6,12 @@ const { LABEL_NEEDS_TEMPLATE, ISSUE_TEMPLATE_MAX_DAYS, BOT_MESSAGE_ISSUE_TEMPLATE_CLOSED, + LABEL_UNLINKED, + PR_UNLINKED_MAX_DAYS, + BOT_MESSAGE_PULL_REQUEST_UNLINKED_CLOSED, } = require('./constants'); -const { removeLabel } = require('./utils'); +const { removeLabel, getLinkedIssues } = require('./utils'); +const { linkStatus, planLink, applyLink } = require('./pull-request-link'); const MS_PER_DAY = 24 * 60 * 60 * 1000; @@ -134,4 +138,45 @@ async function closeIssuesMissingTemplate(owner, repo, { github, core }, now = D return closed; } -module.exports = { sweepIssueReviewLabels, closeIssuesMissingTemplate }; +async function closeUnlinkedPullRequests(owner, repo, { github, context, core }, now = Date.now()) { + const stale = await staleIssues( + LABEL_UNLINKED, + PR_UNLINKED_MAX_DAYS, + owner, + repo, + { github, core }, + now, + ); + const closed = []; + for (const { issue: pullRequest } of stale.filter(({ issue }) => issue.pull_request)) { + const { number } = pullRequest; + const linked = await getLinkedIssues(number, { github, context, core }); + if (linked === null) { + continue; + } + const status = linkStatus(linked, pullRequest.user.login); + const link = { ...status, message: status.message ?? BOT_MESSAGE_PULL_REQUEST_UNLINKED_CLOSED }; + try { + const plan = await planLink( + owner, + repo, + pullRequest, + link, + { github, core }, + { + opening: false, + }, + ); + await applyLink(owner, repo, number, plan, { github, core }); + if (plan.action === 'close') { + closed.push(number); + core.info(`Closed pull request #${number}: ${link.status}.`); + } + } catch (error) { + core.warning(`Failed to act on pull request #${number}: ${error.message}`); + } + } + return closed; +} + +module.exports = { sweepIssueReviewLabels, closeIssuesMissingTemplate, closeUnlinkedPullRequests }; diff --git a/scripts/issue-review-sweep.test.js b/scripts/issue-review-sweep.test.js index e069e6d..7603d1a 100644 --- a/scripts/issue-review-sweep.test.js +++ b/scripts/issue-review-sweep.test.js @@ -1,13 +1,23 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { sweepIssueReviewLabels, closeIssuesMissingTemplate } = require('./issue-review-sweep'); +const { + sweepIssueReviewLabels, + closeIssuesMissingTemplate, + closeUnlinkedPullRequests, +} = require('./issue-review-sweep'); const { LABEL_COMMUNITY_ISSUE_REVIEW, ISSUE_REVIEW_MAX_DAYS, LABEL_NEEDS_TEMPLATE, ISSUE_TEMPLATE_MAX_DAYS, BOT_MESSAGE_ISSUE_TEMPLATE_CLOSED, + LABEL_UNLINKED, + PR_UNLINKED_MAX_DAYS, + BOT_MESSAGE_PULL_REQUEST_UNLINKED_CLOSED, + LABEL_COMMUNITY_REVIEW, + RTIBBLESBOT_USERNAME, + CLOSE_CONTRIBUTORS, } = require('./constants'); const NOW = Date.parse('2026-10-26T12:00:00Z'); @@ -387,3 +397,147 @@ test('a pull request carrying the template label is not closed', async () => { assert.deepEqual(closed, []); }); + +const AUTHOR = 'outsider'; +const prContext = { repo: { owner: 'learningequality', repo: 'kolibri' } }; +const unlinkedPr = number => ({ + number, + labels: [LABEL_UNLINKED], + pull_request: {}, + user: { login: AUTHOR }, +}); + +function prGithub({ issues, linked = {}, graphqlFails = false }) { + const github = fakeGithub({ + issues, + timelines: Object.fromEntries( + issues.map(({ number }) => [ + number, + [labeled(daysAgo(PR_UNLINKED_MAX_DAYS + 1), LABEL_UNLINKED)], + ]), + ), + }); + github.commented = []; + github.closed = []; + github.reviewers = []; + github.labelled = []; + github.graphql = async (query, { number }) => { + if (graphqlFails) throw new Error('API down'); + const nodes = (linked[number] || []).map(issue => ({ + number: issue.number, + url: `https://github.com/learningequality/kolibri/issues/${issue.number}`, + assignees: { nodes: issue.assignees.map(login => ({ login })) }, + })); + return { repository: { pullRequest: { closingIssuesReferences: { nodes } } } }; + }; + github.rest.issues.createComment = async ({ issue_number, body }) => { + github.commented.push({ issue_number, body }); + return { + data: { html_url: `https://github.com/learningequality/kolibri/pull/${issue_number}` }, + }; + }; + github.rest.issues.addLabels = async ({ labels }) => { + github.labelled.push(...labels); + }; + github.rest.issues.listLabelsOnIssue = async ({ issue_number }) => + issues.find(({ number }) => number === issue_number).labels.map(name => ({ name })); + github.rest.pulls = { + update: async ({ pull_number, state }) => { + github.closed.push({ pull_number, state }); + }, + requestReviewers: async ({ reviewers }) => { + github.reviewers.push(...reviewers); + }, + }; + return github; +} + +const sweepPrs = (github, core = fakeCore()) => + closeUnlinkedPullRequests( + 'learningequality', + 'kolibri', + { github, context: prContext, core }, + NOW, + ); + +test('a pull request unlinked past the limit is closed with a comment', async () => { + const github = prGithub({ issues: [unlinkedPr(42)] }); + + assert.deepEqual(await sweepPrs(github), [42]); + assert.deepEqual(github.commented, [ + { issue_number: 42, body: BOT_MESSAGE_PULL_REQUEST_UNLINKED_CLOSED }, + ]); + assert.deepEqual(github.closed, [{ pull_number: 42, state: 'closed' }]); +}); + +test('a pull request since linked to the issue assigned to its author starts review', async () => { + const github = prGithub({ + issues: [unlinkedPr(42)], + linked: { 42: [{ number: 7, assignees: [AUTHOR] }] }, + }); + + assert.deepEqual(await sweepPrs(github), []); + assert.deepEqual(github.removed, [{ name: LABEL_UNLINKED, issue_number: 42 }]); + assert.deepEqual(github.reviewers, [RTIBBLESBOT_USERNAME]); + assert.deepEqual(github.labelled, [LABEL_COMMUNITY_REVIEW]); +}); + +test('a linked pull request already under review gets no second request', async () => { + const github = prGithub({ + issues: [{ ...unlinkedPr(42), labels: [LABEL_UNLINKED, LABEL_COMMUNITY_REVIEW] }], + linked: { 42: [{ number: 7, assignees: [AUTHOR] }] }, + }); + + assert.deepEqual(await sweepPrs(github), []); + assert.deepEqual(github.reviewers, []); +}); + +test('a failed review request leaves the unlinked label for the next sweep', async () => { + const github = prGithub({ + issues: [unlinkedPr(42)], + linked: { 42: [{ number: 7, assignees: [AUTHOR] }] }, + }); + github.rest.pulls.requestReviewers = async () => { + throw new Error('Reviewer cannot be requested'); + }; + const core = fakeCore(); + + assert.deepEqual(await sweepPrs(github, core), []); + assert.deepEqual(github.removed, []); + assert.match(core.warnings[0], /Reviewer cannot be requested/); +}); + +test('a close contributor carrying the unlinked label is not closed', async () => { + const github = prGithub({ + issues: [{ ...unlinkedPr(42), user: { login: CLOSE_CONTRIBUTORS[0] } }], + }); + + assert.deepEqual(await sweepPrs(github), []); + assert.deepEqual(github.commented, []); + assert.deepEqual(github.closed, []); +}); + +test('a pull request since linked to an unassigned issue is closed for that', async () => { + const github = prGithub({ + issues: [unlinkedPr(42)], + linked: { 42: [{ number: 7, assignees: [] }] }, + }); + + assert.deepEqual(await sweepPrs(github), [42]); + assert.match(github.commented[0].body, /isn't assigned to you/); +}); + +test('an issue carrying the unlinked label is not swept', async () => { + const github = prGithub({ issues: [{ number: 7, labels: [LABEL_UNLINKED] }] }); + + assert.deepEqual(await sweepPrs(github), []); + assert.deepEqual(github.commented, []); +}); + +test('a pull request whose links cannot be read is left open', async () => { + const github = prGithub({ issues: [unlinkedPr(42)], graphqlFails: true }); + + assert.deepEqual(await sweepPrs(github), []); + assert.deepEqual(github.commented, []); + assert.deepEqual(github.closed, []); +}); diff --git a/scripts/pull-request-link.js b/scripts/pull-request-link.js new file mode 100644 index 0000000..12ec65d --- /dev/null +++ b/scripts/pull-request-link.js @@ -0,0 +1,89 @@ +const { + BOT_MESSAGE_PULL_REQUEST_CLOSED, + BOT_MESSAGE_PULL_REQUEST_UNASSIGNED, + LABEL_COMMUNITY_REVIEW, + LABEL_UNLINKED, + RTIBBLESBOT_USERNAME, +} = require('./constants'); +const { addLabel, removeLabel, isCloseContributor } = require('./utils'); + +function linkStatus(linkedIssues, author) { + if (!linkedIssues.length) return { status: 'unlinked' }; + if (linkedIssues.some(issue => issue.assignees.includes(author))) return { status: 'assigned' }; + const elsewhere = linkedIssues.find(issue => issue.assignees.length > 0); + if (elsewhere) { + return { + status: 'assigned elsewhere', + issue: elsewhere, + message: BOT_MESSAGE_PULL_REQUEST_CLOSED(author, elsewhere.url), + }; + } + return { + status: 'unassigned', + issue: linkedIssues[0], + message: BOT_MESSAGE_PULL_REQUEST_UNASSIGNED(author, linkedIssues[0].url), + }; +} + +async function currentLabels(owner, repo, number, github) { + const labels = await github.paginate(github.rest.issues.listLabelsOnIssue, { + owner, + repo, + issue_number: number, + per_page: 100, + }); + return labels.map(label => label.name.toLowerCase()); +} + +async function planLink(owner, repo, pullRequest, link, { github, core }, { opening }) { + if (link.status === 'assigned') { + const labels = await currentLabels(owner, repo, pullRequest.number, github); + const unlinked = labels.includes(LABEL_UNLINKED); + if (!opening && !unlinked) { + return { action: 'none' }; + } + return { action: 'review', underReview: labels.includes(LABEL_COMMUNITY_REVIEW), unlinked }; + } + if (await isCloseContributor(pullRequest.user.login, { core })) { + return { action: 'none' }; + } + return link.message ? { action: 'close', message: link.message } : { action: 'label' }; +} + +async function applyLink(owner, repo, number, plan, { github, core }) { + if (plan.action === 'close') { + await github.rest.pulls.update({ owner, repo, pull_number: number, state: 'closed' }); + const { data } = await github.rest.issues.createComment({ + owner, + repo, + issue_number: number, + body: plan.message, + }); + return data.html_url; + } + if (plan.action === 'label') { + await addLabel(LABEL_UNLINKED, owner, repo, number, github, core); + } + if (plan.action === 'review') { + if (!plan.underReview) { + await github.rest.pulls.requestReviewers({ + owner, + repo, + pull_number: number, + reviewers: [RTIBBLESBOT_USERNAME], + }); + await github.rest.issues.addLabels({ + owner, + repo, + issue_number: number, + labels: [LABEL_COMMUNITY_REVIEW], + }); + } + if (plan.unlinked) { + await removeLabel(LABEL_UNLINKED, owner, repo, number, github, core); + } + } + return null; +} + +module.exports = { linkStatus, planLink, applyLink }; diff --git a/scripts/unassign-inactive-issues.js b/scripts/unassign-inactive-issues.js index 0be01de..3dc7ce5 100644 --- a/scripts/unassign-inactive-issues.js +++ b/scripts/unassign-inactive-issues.js @@ -1,4 +1,8 @@ -const { sweepIssueReviewLabels, closeIssuesMissingTemplate } = require('./issue-review-sweep'); +const { + sweepIssueReviewLabels, + closeIssuesMissingTemplate, + closeUnlinkedPullRequests, +} = require('./issue-review-sweep'); const formatUnassignments = unassignments => { if (unassignments.length === 0) return ''; @@ -260,6 +264,7 @@ module.exports = async ({ github, context, core }) => { await sweepIssueReviewLabels(owner, repo, { github, core }); await closeIssuesMissingTemplate(owner, repo, { github, core }); + await closeUnlinkedPullRequests(owner, repo, { github, context, core }); const issues = await getAllIssues(github, owner, repo); //console.log(`Processing ${issues.length} open issues`);