diff --git a/.github/scripts/update.js b/.github/scripts/update.js new file mode 100644 index 00000000..9a0edca5 --- /dev/null +++ b/.github/scripts/update.js @@ -0,0 +1,409 @@ +// "/update" comments on pull requests (see .github/workflows/update.yaml and CONTRIBUTING.md). +// +// The PR author, or anyone with write access, comments exactly "/update". The bot then: +// - already up to date -> says so, and takes the "needs update" label off +// - main conflicts with it -> lists the files, adds "needs update" and says a maintainer will resolve them +// - it may push the branch -> merges main in with GitHub's update-branch API (a merge, never a rebase). That +// works for forks too, when "Allow edits by maintainers" is on. +// - it may not (a fork without maintainer edits, or GitHub refuses) -> points the author at GitHub's own +// "Update branch" button and adds the label so a maintainer can follow up +// When the branch is pushed again and is up to date, the label comes off by itself (clearLabel). +// +// After its own merge commit the bot restarts the checks, because GitHub treats a commit made with the workflow +// token differently from a person's push: the pull_request runs (CI, Changelog) wait for approval, and +// pull_request_target workflows (the CLA check) don't run at all. So the bot +// - approves the waiting runs, but only when GitHub had already run the checks on the commit before its merge +// (otherwise, for example for a first-time contributor, a maintainer still approves them by hand), and then +// - copies the CLAAssistant result from that commit, which is exactly what the CLA action would decide again: +// the merge adds no commit of anyone else's (main's commits aren't part of the PR) and its own author, +// github-actions[bot], is on the CLA allowlist. +// Both happen only for a commit GitHub itself made: verified, committed by web-flow, its first parent the old head. +// +// It only compares the comment with a fixed string, and never checks out or runs the pull request's code. To find +// conflicting files it fetches the two commits' history into an empty bare repository and asks git to merge them +// in memory (git merge-tree): no working tree, no hooks, nothing from the branch is executed. + +const LABEL = { + name: 'needs update', + color: 'fbca04', + description: 'Behind main or has conflicts; a maintainer will bring it up to date', +}; +// author_association values that may come with write access; the permission itself is checked too (mayUpdate). +const WRITERS = ['OWNER', 'MEMBER', 'COLLABORATOR']; +const CLA_CHECK = 'CLAAssistant'; +const POLL = { tries: 12, ms: 5000 }; + +const code = (s) => `\`${s}\``; + +const MESSAGES = { + updated: (base, sha, checks) => + `Merged the latest ${code(base)} into this branch (${sha.slice(0, 7)}). Nothing else changed. ` + + { + running: 'The checks are running again.', + held: 'GitHub holds the checks for a commit made by a bot, so a maintainer will start them.', + noCla: 'The checks are running again, but the CLA check only runs again with your next push.', + }[checks], + updating: (base) => + `GitHub is merging the latest ${code(base)} into this branch now, and the new commit shows up here in a ` + + 'minute. GitHub holds the checks for a commit made by a bot, so a maintainer will start them.', + upToDate: (base) => `This branch already has everything from ${code(base)}. Nothing to do.`, + conflicts: (base, files, canEdit) => + `${code(base)} has changes that conflict with this branch` + + (files && files.length ? ' in:\n\n' + files.map((f) => `- ${code(f)}`).join('\n') + '\n\n' : '. ') + + `I have labelled it ${code(LABEL.name)}. ` + + (canEdit + ? 'A maintainer will resolve the conflicts for you and comment with exactly what they pushed. ' + + 'You are welcome to resolve them yourself instead, if you prefer.' + : 'A maintainer can resolve them for you once **Allow edits by maintainers** is ticked on this pull ' + + 'request (in the sidebar). Or resolve them yourself, whichever you prefer.'), + useButton: (base, canEdit, clean) => + (canEdit + ? "I can't push to this branch: GitHub refused the update for this fork. " + : "I can't push to this branch, because it lives in a fork that doesn't allow edits by maintainers. ") + + (clean ? `It has no conflicts with ${code(base)}, so click` : 'Click') + + ' **Update branch** near the bottom of this pull request and GitHub merges it in for you. ' + + `I have also labelled it ${code(LABEL.name)} so a maintainer can follow up. ` + + "There's no rush: the merge queue tests your change against the latest main anyway.", + headMoved: 'This branch changed while I was updating it, so I left it alone. Comment `/update` again to retry.', + closed: 'This pull request is closed, so there is nothing to update.', + notAllowed: + 'Only the author of this pull request or a maintainer can use `/update` here. ' + + 'If it needs updating, a maintainer can help.', + failed: (base) => + `Sorry, I could not merge ${code(base)} into this branch. I have labelled it ${code(LABEL.name)} so a ` + + 'maintainer takes a look.', +}; + +/** True only when the whole comment is "/update" (surrounding spaces and line breaks allowed). */ +function parseCommand(body) { + return String(body || '').trim() === '/update'; +} + +/** + * The PR author, or someone with write access, may update a branch. An association of OWNER, MEMBER or + * COLLABORATOR doesn't prove write access (an organisation member or a read-only collaborator has one too), so for + * them the repository permission is looked up as well. + */ +async function mayUpdate(github, { owner, repo, association, commenter, author }) { + if (!commenter) return false; + if (String(commenter).toLowerCase() === String(author || '').toLowerCase()) return true; + if (!WRITERS.includes(String(association || '').toUpperCase())) return false; + try { + const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ owner, repo, username: commenter }); + return ['admin', 'maintain', 'write'].includes(data.permission); + } catch (err) { + if (err.status === 404) return false; + throw err; + } +} + +/** True when the branch lives in another repository (a fork), or the fork is gone. */ +function isFork(pr, owner, repo) { + const name = pr.head && pr.head.repo && pr.head.repo.full_name; + return !name || name.toLowerCase() !== `${owner}/${repo}`.toLowerCase(); +} + +/** + * Decides what to do once the branch's state is known, without touching GitHub. + * behind: commits on the base branch the PR lacks. conflicts: conflicting files ([] for none, null if unknown). + * Returns "upToDate" | "conflicts" | "useButton" | "updateBranch". + */ +function decide({ behind, conflicts, fork, canEdit }) { + if (behind === 0) return 'upToDate'; + if (conflicts && conflicts.length) return 'conflicts'; + // Without "Allow edits by maintainers" nobody but the author can push to a fork's branch. + if (fork && !canEdit) return 'useButton'; + return 'updateBranch'; +} + +/** Commits on the base branch (its current tip, by name) that the PR's head doesn't have yet. */ +async function behindBy(github, { owner, repo, pr }) { + const { data } = await github.rest.repos.compareCommitsWithBasehead({ + owner, + repo, + basehead: `${pr.head.sha}...${pr.base.ref}`, + per_page: 1, + }); + return data.ahead_by; +} + +/** + * Files that conflict when the base branch is merged into the PR, found with `git merge-tree` in a fresh bare + * repository (only git objects, never a checkout). Returns [] when it merges cleanly. + */ +function gitConflicts({ owner, repo, number, base, dir }) { + const { execFileSync } = require('node:child_process'); + const fs = require('node:fs'); + if (!/^[A-Za-z0-9._/-]+$/.test(base) || !/^\d+$/.test(String(number))) throw new Error('unexpected ref'); + fs.rmSync(dir, { recursive: true, force: true }); + const git = (...args) => + execFileSync('git', ['-C', dir, ...args], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + timeout: 120000, + }); + execFileSync('git', ['init', '--bare', '--quiet', dir], { stdio: 'ignore' }); + git('remote', 'add', 'origin', `https://github.com/${owner}/${repo}.git`); + // Fetch commits and trees only; git fetches the few file contents the merge needs on demand. + git('config', 'remote.origin.promisor', 'true'); + git('config', 'remote.origin.partialclonefilter', 'blob:none'); + git('fetch', '--quiet', '--no-tags', '--filter=blob:none', 'origin', + `+refs/heads/${base}:refs/heads/base`, `+refs/pull/${number}/head:refs/heads/pr`); + try { + git('merge-tree', '--write-tree', '--name-only', '--no-messages', 'pr', 'base'); + return []; + } catch (err) { + // Exit code 1 means conflicts: the first line is a tree id, the rest are the conflicting files. + if (err.status !== 1) throw err; + const lines = String(err.stdout || '').split('\n').slice(1).map((l) => l.trim()).filter(Boolean); + return [...new Set(lines)]; + } +} + +async function addLabel(github, { owner, repo, issue_number }) { + try { + await github.rest.issues.getLabel({ owner, repo, name: LABEL.name }); + } catch (err) { + if (err.status !== 404) throw err; + try { + await github.rest.issues.createLabel({ owner, repo, ...LABEL }); + } catch (e) { + if (e.status !== 422) throw e; // 422: someone created it meanwhile + } + } + await github.rest.issues.addLabels({ owner, repo, issue_number, labels: [LABEL.name] }); +} + +async function removeLabel(github, { owner, repo, issue_number }) { + try { + await github.rest.issues.removeLabel({ owner, repo, issue_number, name: LABEL.name }); + return true; + } catch (err) { + if (err.status === 404) return false; // it wasn't there + throw err; + } +} + +/** Waits for update-branch's merge commit to appear (the API answers before the merge is done). */ +async function waitForNewHead(github, { owner, repo, pull_number, oldSha, sleep, poll }) { + for (let i = 0; i < poll.tries; i++) { + await sleep(poll.ms); + const { data } = await github.rest.pulls.get({ owner, repo, pull_number }); + if (data.head.sha !== oldSha) return data.head.sha; + } + return null; +} + +/** True when newSha is GitHub's own merge commit on top of oldSha (not something pushed meanwhile). */ +async function isOurMerge(github, { owner, repo, oldSha, newSha }) { + const { data } = await github.rest.repos.getCommit({ owner, repo, ref: newSha }); + const parents = (data.parents || []).map((p) => p.sha); + return ( + parents.length === 2 && + parents[0] === oldSha && + !!(data.committer && data.committer.login === 'web-flow') && + !!(data.commit && data.commit.verification && data.commit.verification.verified) + ); +} + +/** + * Copies the CLA check's result from the old head to the merge commit (see the top of this file). Called once the + * approved runs have started: GitHub files a check made with the workflow token under one of that commit's workflow + * runs, and one filed before the run was approved is dropped when it starts. Returns true when the check is there. + */ +async function carryClaCheck(github, { owner, repo, oldSha, newSha, core }) { + const latest = async (ref) => + (await github.rest.checks.listForRef({ owner, repo, ref, check_name: CLA_CHECK, filter: 'latest' })).data + .check_runs[0]; + if (await latest(newSha)) { + core.info('The CLA check is already on the merge commit.'); + return true; + } + const old = await latest(oldSha); + if (!old || old.status !== 'completed') { + core.info('No finished CLA check on the old head to copy.'); + return false; + } + await github.rest.checks.create({ + owner, + repo, + name: CLA_CHECK, + head_sha: newSha, + status: 'completed', + conclusion: old.conclusion, + details_url: old.html_url, + output: { + title: `Same as ${oldSha.slice(0, 7)}`, + summary: + `Copied from ${oldSha.slice(0, 7)} by /update: this commit only merges the base branch in, so it adds ` + + 'no new contributor.', + }, + }); + const ok = !!(await latest(newSha)); + core.info(`Copied the CLA check (${old.conclusion}) from ${oldSha.slice(0, 7)}${ok ? '' : ', but GitHub does not list it'}.`); + return ok; +} + +/** + * Approves the pull_request runs GitHub holds on the merge commit, if it had already run them on the old head. + * Returns "running" or "held" (a maintainer approves them). + */ +async function approveHeldRuns(github, { owner, repo, oldSha, newSha, sleep, poll, core }) { + const runs = async (head_sha, status) => + (await github.rest.actions.listWorkflowRunsForRepo({ owner, repo, head_sha, event: 'pull_request', status })) + .data.workflow_runs; + const before = await runs(oldSha); + if (!before.length || before.some((r) => r.conclusion === 'action_required')) { + core.info('The old head\'s checks were not run without approval, so the new ones wait for a maintainer.'); + return 'held'; + } + let held = []; + for (let i = 0; i < poll.tries && !held.length; i++) { + await sleep(poll.ms); + held = await runs(newSha, 'action_required'); + } + if (!held.length) return 'running'; + await sleep(poll.ms); // let GitHub finish creating the rest of them + held = await runs(newSha, 'action_required'); + for (const r of held) { + await github.rest.actions.approveWorkflowRun({ owner, repo, run_id: r.id }); + core.info(`Approved ${r.name} (${r.id}).`); + } + // Wait until they have left the queue, so a check filed under one of them afterwards stays. + for (let i = 0; i < poll.tries; i++) { + const waiting = (await runs(newSha)).filter( + (r) => r.conclusion === 'action_required' || ['queued', 'waiting', 'pending', 'requested'].includes(r.status), + ); + if (!waiting.length) break; + await sleep(poll.ms); + } + return 'running'; +} + +/** + * Entry point for actions/github-script: + * `await require('./.github/scripts/update.js')({ github, context, core })`. + * `findConflicts`, `sleep` and `poll` are swapped out by the tests. + */ +async function run({ github, context, core, findConflicts, sleep, poll = POLL, workDir }) { + const { issue, comment } = context.payload; + if (!issue || !comment) return core.info('Not a comment.'); + if (!issue.pull_request) return core.info('Comment is on an issue, not a pull request: ignored.'); + if (comment.user.type === 'Bot') return core.info('Comment is from a bot: ignored.'); + if (!parseCommand(comment.body)) return core.info('Not a command.'); + sleep = sleep || ((ms) => new Promise((resolve) => setTimeout(resolve, ms))); + + const { owner, repo } = context.repo; + const issue_number = issue.number; + const commenter = comment.user.login; + const say = (text) => github.rest.issues.createComment({ owner, repo, issue_number, body: `@${commenter} ${text}` }); + + const author = issue.user && issue.user.login; + if (!(await mayUpdate(github, { owner, repo, association: comment.author_association, commenter, author }))) { + core.info(`/update by ${commenter} (${comment.author_association}) on #${issue_number}: not allowed`); + return say(MESSAGES.notAllowed); + } + + // Read the pull request now: the payload is a snapshot from when the comment was made. + const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: issue_number }); + if (pr.state !== 'open') return say(MESSAGES.closed); + const base = pr.base.ref; + const oldSha = pr.head.sha; + const fork = isFork(pr, owner, repo); + const canEdit = !fork || !!pr.maintainer_can_modify; + + const behind = await behindBy(github, { owner, repo, pr }); + let conflicts = null; + if (behind > 0) { + try { + conflicts = await (findConflicts || gitConflicts)({ + owner, + repo, + number: issue_number, + base, + dir: workDir || `${process.env.RUNNER_TEMP || require('node:os').tmpdir()}/update-merge-check`, + }); + } catch (err) { + core.warning(`Could not check for conflicts: ${err.message}`); + } + } + let action = decide({ behind, conflicts, fork, canEdit }); + core.info( + `/update by ${commenter} on #${issue_number}: behind ${behind}, fork ${fork}, maintainer edits ${canEdit}, ` + + `conflicts ${conflicts === null ? 'unknown' : conflicts.length} -> ${action}`, + ); + + let newSha = null; + let ourMerge = false; + let checks = 'held'; + if (action === 'updateBranch') { + try { + await github.rest.pulls.updateBranch({ owner, repo, pull_number: issue_number, expected_head_sha: oldSha }); + action = 'updated'; + } catch (err) { + const message = String(err.message || ''); + core.info(`update-branch answered ${err.status}: ${message}`); + if (err.status === 422 && /expected head sha/i.test(message)) action = 'headMoved'; + else if (err.status === 422 && /conflict/i.test(message)) action = 'conflicts'; + // GitHub may still refuse to push to a fork (for example one owned by an organisation). + else if (fork && [403, 404, 422].includes(err.status)) action = 'useButton'; + else action = 'failed'; + } + } + if (action === 'updated') { + newSha = await waitForNewHead(github, { owner, repo, pull_number: issue_number, oldSha, sleep, poll }); + ourMerge = !!newSha && (await isOurMerge(github, { owner, repo, oldSha, newSha })); + if (ourMerge) { + try { + checks = await approveHeldRuns(github, { owner, repo, oldSha, newSha, sleep, poll, core }); + if (checks === 'running' && !(await carryClaCheck(github, { owner, repo, oldSha, newSha, core }))) { + checks = 'noCla'; + } + } catch (err) { + core.warning(`Could not restart the checks: ${err.message}`); + } + } + } + + // Only GitHub's own merge commit proves the branch is current (a push meanwhile could be anything), so + // otherwise the label stays. + if (action === 'upToDate' || (action === 'updated' && ourMerge)) await removeLabel(github, { owner, repo, issue_number }); + if (['conflicts', 'useButton', 'failed'].includes(action)) await addLabel(github, { owner, repo, issue_number }); + + const reply = { + upToDate: () => MESSAGES.upToDate(base), + updated: () => (newSha ? MESSAGES.updated(base, newSha, checks) : MESSAGES.updating(base)), + conflicts: () => MESSAGES.conflicts(base, conflicts, canEdit), + useButton: () => MESSAGES.useButton(base, canEdit, Array.isArray(conflicts) && conflicts.length === 0), + headMoved: () => MESSAGES.headMoved, + failed: () => MESSAGES.failed(base), + }[action](); + core.info(`Result: ${action}${action === 'updated' ? `, checks ${checks}` : ''}`); + return say(reply); +} + +/** + * On a push to a labelled pull request (pull_request_target, synchronize): take "needs update" off once the + * branch has everything from the base branch. Says nothing. + */ +async function clearLabel({ github, context, core }) { + const pr = context.payload.pull_request; + if (!pr) return core.info('Not a pull request event.'); + if (!(pr.labels || []).some((l) => l.name === LABEL.name)) return core.info('No label to clear.'); + const { owner, repo } = context.repo; + const behind = await behindBy(github, { owner, repo, pr }); + core.info(`#${pr.number} is ${behind} commit(s) behind ${pr.base.ref}`); + if (behind === 0 && (await removeLabel(github, { owner, repo, issue_number: pr.number }))) { + core.info(`Removed "${LABEL.name}".`); + } +} + +module.exports = run; +module.exports.clearLabel = clearLabel; +module.exports.parseCommand = parseCommand; +module.exports.mayUpdate = mayUpdate; +module.exports.isFork = isFork; +module.exports.decide = decide; +module.exports.gitConflicts = gitConflicts; +module.exports.MESSAGES = MESSAGES; +module.exports.LABEL = LABEL; diff --git a/.github/scripts/update.test.js b/.github/scripts/update.test.js new file mode 100644 index 00000000..487b254d --- /dev/null +++ b/.github/scripts/update.test.js @@ -0,0 +1,457 @@ +// Run with: node --test ".github/scripts/*.test.js" +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { execFileSync } = require('node:child_process'); +const run = require('./update.js'); +const { clearLabel, parseCommand, mayUpdate, isFork, decide, MESSAGES, LABEL } = run; + +const OLD = 'a'.repeat(40); +const NEW = 'b'.repeat(40); +const MAIN = 'c'.repeat(40); +const core = { info: () => {}, warning: () => {} }; +const noSleep = async () => {}; +const poll = { tries: 3, ms: 0 }; + +function httpError(status, message) { + return Object.assign(new Error(message), { status }); +} + +/** + * A fake GitHub. Options: + * pr: overrides for the live pull request; behind: commits on main it lacks + * update: what update-branch does ("ok", or an error); merge: the merge commit's metadata + * labelExists, oldRuns (pull_request runs on the old head), heldRuns (waiting on the merge commit), + * oldCla (the CLA check on the old head), newCla (one already on the merge commit) + */ +function fakeGithub(opts = {}) { + const calls = []; + const { + behind = 3, + update = 'ok', + labelExists = true, + oldRuns = [{ id: 1, name: 'CI', conclusion: 'success' }], + heldRuns = [{ id: 7, name: 'CI' }, { id: 8, name: 'Changelog' }], + oldCla = { status: 'completed', conclusion: 'success', html_url: 'https://github.com/x/runs/1' }, + newCla = null, + merge = { parents: [{ sha: OLD }, { sha: MAIN }], committer: { login: 'web-flow' }, commit: { verification: { verified: true } } }, + } = opts; + let updated = false; + let created = null; + const approved = new Set(); + const pr = { + state: 'open', + head: { sha: OLD, repo: { full_name: 'existence-master/Sentient' } }, + base: { ref: 'main' }, + maintainer_can_modify: false, + ...opts.pr, + }; + const github = { + rest: { + pulls: { + get: async () => ({ data: { ...pr, head: { ...pr.head, sha: updated ? NEW : pr.head.sha } } }), + updateBranch: async (args) => { + calls.push(['updateBranch', args]); + if (update !== 'ok') throw update; + updated = true; + return { status: 202, data: { message: 'Updating pull request branch.' } }; + }, + }, + repos: { + compareCommitsWithBasehead: async (args) => { + calls.push(['compare', args]); + return { data: { ahead_by: behind } }; + }, + getCommit: async () => ({ data: merge }), + getCollaboratorPermissionLevel: async () => ({ data: { permission: opts.permission || 'read' } }), + }, + checks: { + listForRef: async ({ ref }) => ({ + data: { check_runs: [ref === OLD ? oldCla : newCla || (opts.claListed !== false && created)].filter(Boolean) }, + }), + create: async (args) => { + calls.push(['createCheck', args]); + created = { status: 'completed', conclusion: args.conclusion }; + }, + }, + actions: { + // Held runs wait on the merge commit until approved; then they are in progress. + listWorkflowRunsForRepo: async ({ head_sha, status }) => { + if (head_sha === OLD) return { data: { workflow_runs: oldRuns } }; + const now = heldRuns.map((r) => + approved.has(r.id) ? { ...r, status: 'in_progress', conclusion: null } : { ...r, status: 'completed', conclusion: 'action_required' }, + ); + return { data: { workflow_runs: status === 'action_required' ? now.filter((r) => r.conclusion === 'action_required') : now } }; + }, + approveWorkflowRun: async (args) => { + calls.push(['approve', args.run_id]); + approved.add(args.run_id); + }, + }, + issues: { + getLabel: async () => { + if (!labelExists) throw httpError(404, 'Not Found'); + return { data: LABEL }; + }, + createLabel: async (args) => calls.push(['createLabel', args]), + addLabels: async (args) => calls.push(['addLabels', args.labels]), + removeLabel: async (args) => { + calls.push(['removeLabel', args.name]); + if (opts.labelOnPr === false) throw httpError(404, 'Label does not exist'); + }, + createComment: async (args) => calls.push(['createComment', args.body]), + }, + }, + }; + return { github, calls, names: () => calls.map((c) => c[0]) }; +} + +function context({ body = '/update', login = 'maya-rao', association = 'CONTRIBUTOR', author = 'maya-rao', pr = true, type = 'User' } = {}) { + return { + repo: { owner: 'existence-master', repo: 'Sentient' }, + payload: { + issue: { + number: 42, + user: { login: author }, + ...(pr ? { pull_request: { url: 'https://api.github.com/x' } } : {}), + }, + comment: { body, author_association: association, user: { login, type } }, + }, + }; +} + +function runWith(fake, ctx = context(), conflicts = []) { + const findConflicts = async () => { + if (conflicts instanceof Error) throw conflicts; + return conflicts; + }; + return run({ github: fake.github, context: ctx, core, findConflicts, sleep: noSleep, poll }); +} + +const reply = (fake) => fake.calls.filter((c) => c[0] === 'createComment').map((c) => c[1]); + +test('only an exact /update counts', () => { + assert.equal(parseCommand('/update'), true); + assert.equal(parseCommand(' /update\r\n'), true); + assert.equal(parseCommand('/update please'), false); + assert.equal(parseCommand('please /update'), false); + assert.equal(parseCommand('/Update'), false); + assert.equal(parseCommand('/updates'), false); + assert.equal(parseCommand(''), false); + assert.equal(parseCommand(undefined), false); +}); + +test('the author and people with write access may update, nobody else', async () => { + const lookups = []; + const github = { + rest: { + repos: { + getCollaboratorPermissionLevel: async ({ username }) => { + lookups.push(username); + const permission = { ravi: 'write', ana: 'admin', mei: 'maintain', ola: 'read', kim: 'none' }[username]; + if (!permission) throw httpError(404, 'Not Found'); + return { data: { permission } }; + }, + }, + }, + }; + const may = (association, commenter) => + mayUpdate(github, { owner: 'o', repo: 'r', association, commenter, author: 'maya-rao' }); + assert.equal(await may('CONTRIBUTOR', 'Maya-Rao'), true); + assert.equal(await may('NONE', 'maya-rao'), true); + for (const [association, login] of [['COLLABORATOR', 'ravi'], ['OWNER', 'ana'], ['MEMBER', 'mei']]) { + assert.equal(await may(association, login), true, login); + } + // An organisation member or a collaborator without write access may not. + for (const [association, login] of [['MEMBER', 'ola'], ['COLLABORATOR', 'kim'], ['MEMBER', 'ghost']]) { + assert.equal(await may(association, login), false, login); + } + // Nobody else is even looked up. + lookups.length = 0; + for (const association of ['CONTRIBUTOR', 'FIRST_TIME_CONTRIBUTOR', 'NONE', undefined]) { + assert.equal(await may(association, 'ravi'), false); + } + assert.deepEqual(lookups, []); +}); + +test('isFork and decide', () => { + assert.equal(isFork({ head: { repo: { full_name: 'Existence-Master/sentient' } } }, 'existence-master', 'Sentient'), false); + assert.equal(isFork({ head: { repo: { full_name: 'maya-rao/Sentient' } } }, 'existence-master', 'Sentient'), true); + assert.equal(isFork({ head: { repo: null } }, 'existence-master', 'Sentient'), true); + assert.equal(decide({ behind: 0, conflicts: null, fork: true, canEdit: false }), 'upToDate'); + assert.equal(decide({ behind: 2, conflicts: ['a'], fork: false, canEdit: true }), 'conflicts'); + assert.equal(decide({ behind: 2, conflicts: [], fork: true, canEdit: false }), 'useButton'); + assert.equal(decide({ behind: 2, conflicts: [], fork: true, canEdit: true }), 'updateBranch'); + assert.equal(decide({ behind: 2, conflicts: null, fork: false, canEdit: true }), 'updateBranch'); +}); + +test('comments on issues, from bots, and other text are ignored', async () => { + for (const ctx of [ + context({ pr: false }), + context({ type: 'Bot', login: 'coderabbitai[bot]', association: 'NONE' }), + context({ body: 'can someone /update this' }), + context({ body: '/update\n\nand also rebase' }), + ]) { + const fake = fakeGithub(); + await runWith(fake, ctx); + assert.deepEqual(fake.calls, []); + } +}); + +test('someone else on the pull request is told who can use it, and nothing changes', async () => { + for (const association of ['CONTRIBUTOR', 'MEMBER']) { + const fake = fakeGithub({ permission: 'read' }); + await runWith(fake, context({ login: 'ravi', association })); + assert.deepEqual(fake.names(), ['createComment']); + assert.equal(reply(fake)[0], `@ravi ${MESSAGES.notAllowed}`); + } +}); + +test("a maintainer with write access may update someone else's pull request", async () => { + const fake = fakeGithub({ permission: 'write' }); + await runWith(fake, context({ login: 'ravi', association: 'MEMBER' })); + assert.ok(fake.names().includes('updateBranch')); + assert.match(reply(fake)[0], /^@ravi Merged the latest `main`/); +}); + +test('a branch that is already up to date: says so and clears the label', async () => { + const fake = fakeGithub({ behind: 0 }); + await runWith(fake); + assert.deepEqual(fake.names(), ['compare', 'removeLabel', 'createComment']); + assert.equal(fake.calls[0][1].basehead, `${OLD}...main`); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.upToDate('main')}`); +}); + +test('a closed pull request is left alone', async () => { + const fake = fakeGithub({ pr: { state: 'closed' } }); + await runWith(fake); + assert.deepEqual(fake.names(), ['createComment']); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.closed}`); +}); + +test('a clean same-repo branch: merges main, restarts the checks, clears the label', async () => { + const fake = fakeGithub(); + await runWith(fake); + assert.deepEqual(fake.calls.find((c) => c[0] === 'updateBranch')[1], { + owner: 'existence-master', repo: 'Sentient', pull_number: 42, expected_head_sha: OLD, + }); + const check = fake.calls.find((c) => c[0] === 'createCheck')[1]; + assert.equal(check.name, 'CLAAssistant'); + assert.equal(check.head_sha, NEW); + assert.equal(check.conclusion, 'success'); + assert.deepEqual(fake.calls.filter((c) => c[0] === 'approve').map((c) => c[1]), [7, 8]); + // The CLA check is filed only after the approved runs started, or GitHub drops it when they do. + assert.ok(fake.names().indexOf('createCheck') > fake.names().lastIndexOf('approve')); + assert.ok(fake.names().includes('removeLabel')); + assert.ok(!fake.names().includes('addLabels')); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.updated('main', NEW, 'running')}`); + assert.match(reply(fake)[0], /Merged the latest `main` into this branch \(bbbbbbb\)\. Nothing else changed\. The checks are running again\./); +}); + +test('a fork with maintainer edits on is updated the same way', async () => { + const fake = fakeGithub({ pr: { head: { sha: OLD, repo: { full_name: 'maya-rao/Sentient' } }, maintainer_can_modify: true } }); + await runWith(fake); + assert.ok(fake.names().includes('updateBranch')); + assert.match(reply(fake)[0], /The checks are running again/); +}); + +test('a fork without maintainer edits: points at the button and labels it, no push attempted', async () => { + const fake = fakeGithub({ labelExists: false, pr: { head: { sha: OLD, repo: { full_name: 'maya-rao/Sentient' } } } }); + await runWith(fake); + assert.ok(!fake.names().includes('updateBranch')); + assert.deepEqual(fake.calls.find((c) => c[0] === 'createLabel')[1], { owner: 'existence-master', repo: 'Sentient', ...LABEL }); + assert.deepEqual(fake.calls.find((c) => c[0] === 'addLabels')[1], ['needs update']); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.useButton('main', false, true)}`); + assert.match(reply(fake)[0], /doesn't allow edits by maintainers\. It has no conflicts with `main`, so click \*\*Update branch\*\*/); +}); + +test('a fork GitHub refuses to push to falls back to the button', async () => { + const fake = fakeGithub({ + update: httpError(403, "user doesn't have permission to update head repository"), + pr: { head: { sha: OLD, repo: { full_name: 'maya-org/Sentient' } }, maintainer_can_modify: true }, + }); + await runWith(fake); + assert.ok(fake.names().includes('addLabels')); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.useButton('main', true, true)}`); + assert.match(reply(fake)[0], /GitHub refused the update for this fork/); + assert.doesNotMatch(reply(fake)[0], /doesn't allow edits/); +}); + +test('the button reply claims no conflicts only when they were checked', async () => { + const fake = fakeGithub({ pr: { head: { sha: OLD, repo: { full_name: 'maya-rao/Sentient' } } } }); + await runWith(fake, context(), new Error('fetch failed')); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.useButton('main', false, false)}`); + assert.doesNotMatch(reply(fake)[0], /no conflicts/); + assert.match(reply(fake)[0], /Click \*\*Update branch\*\*/); +}); + +test('conflicts: lists the files, labels it, says a maintainer will resolve them, never pushes', async () => { + const fake = fakeGithub(); + await runWith(fake, context(), ['changes/456.fixed.md', 'docs/API.md']); + assert.ok(!fake.names().includes('updateBranch')); + assert.deepEqual(fake.calls.find((c) => c[0] === 'addLabels')[1], ['needs update']); + const text = reply(fake)[0]; + assert.match(text, /`main` has changes that conflict with this branch in:\n\n- `changes\/456.fixed.md`\n- `docs\/API.md`\n/); + assert.match(text, /A maintainer will resolve the conflicts for you and comment with exactly what they pushed/); +}); + +test('conflicts on a fork without maintainer edits ask for them to be turned on', async () => { + const fake = fakeGithub({ pr: { head: { sha: OLD, repo: { full_name: 'maya-rao/Sentient' } } } }); + await runWith(fake, context(), ['docs/API.md']); + assert.match(reply(fake)[0], /once \*\*Allow edits by maintainers\*\* is ticked/); +}); + +test('when the conflict check fails, update-branch reporting a conflict still gets the label', async () => { + const fake = fakeGithub({ update: httpError(422, 'merge conflict between base and head') }); + await runWith(fake, context(), new Error('fetch failed')); + assert.ok(fake.names().includes('updateBranch')); + assert.ok(fake.names().includes('addLabels')); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.conflicts('main', null, true)}`); +}); + +test('a branch that moved during the update is left alone', async () => { + const fake = fakeGithub({ update: httpError(422, 'expected head sha didn’t match current head ref.') }); + await runWith(fake); + assert.ok(!fake.names().includes('addLabels')); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.headMoved}`); +}); + +test('any other failure on our own repo is labelled for a maintainer', async () => { + const fake = fakeGithub({ update: httpError(500, 'boom') }); + await runWith(fake); + assert.ok(fake.names().includes('addLabels')); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.failed('main')}`); +}); + +test('checks stay held for a maintainer when the old head never ran without approval', async () => { + for (const oldRuns of [[], [{ id: 1, name: 'CI', conclusion: 'action_required' }]]) { + const fake = fakeGithub({ oldRuns }); + await runWith(fake); + assert.ok(!fake.names().includes('approve')); + assert.match(reply(fake)[0], /GitHub holds the checks for a commit made by a bot, so a maintainer will start them/); + } +}); + +test('nothing is approved or copied for a commit that is not GitHub\'s own merge', async () => { + for (const merge of [ + { parents: [{ sha: OLD }], committer: { login: 'web-flow' }, commit: { verification: { verified: true } } }, + { parents: [{ sha: MAIN }, { sha: OLD }], committer: { login: 'web-flow' }, commit: { verification: { verified: true } } }, + { parents: [{ sha: OLD }, { sha: MAIN }], committer: { login: 'maya-rao' }, commit: { verification: { verified: true } } }, + { parents: [{ sha: OLD }, { sha: MAIN }], committer: { login: 'web-flow' }, commit: { verification: { verified: false } } }, + ]) { + const fake = fakeGithub({ merge }); + await runWith(fake); + assert.ok(!fake.names().includes('approve')); + assert.ok(!fake.names().includes('createCheck')); + assert.ok(!fake.names().includes('removeLabel')); + assert.match(reply(fake)[0], /a maintainer will start them/); + } +}); + +test('the CLA check is copied as it was, and not when one is there already or none finished', async () => { + let fake = fakeGithub({ oldCla: { status: 'completed', conclusion: 'failure', html_url: 'u' } }); + await runWith(fake); + assert.equal(fake.calls.find((c) => c[0] === 'createCheck')[1].conclusion, 'failure'); + for (const opts of [{ newCla: { status: 'completed', conclusion: 'success' } }, { oldCla: null }, { oldCla: { status: 'in_progress' } }]) { + fake = fakeGithub(opts); + await runWith(fake); + assert.ok(!fake.names().includes('createCheck')); + } +}); + +test('a copied CLA check GitHub does not list is reported, not claimed', async () => { + const fake = fakeGithub({ claListed: false }); + await runWith(fake); + assert.ok(fake.names().includes('createCheck')); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.updated('main', NEW, 'noCla')}`); + assert.match(reply(fake)[0], /the CLA check only runs again with your next push/); +}); + +test('when the merge commit has not shown up yet, the reply says it is on its way', async () => { + const fake = fakeGithub(); + fake.github.rest.pulls.get = async () => ({ data: { state: 'open', head: { sha: OLD, repo: { full_name: 'existence-master/Sentient' } }, base: { ref: 'main' } } }); + await runWith(fake); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.updating('main')}`); + assert.ok(!fake.names().includes('removeLabel')); +}); + +test('clearLabel takes the label off once the branch is up to date, and only then', async () => { + const ctx = (labels) => ({ + repo: { owner: 'existence-master', repo: 'Sentient' }, + payload: { pull_request: { number: 42, labels: labels.map((name) => ({ name })), head: { sha: OLD }, base: { ref: 'main' } } }, + }); + let fake = fakeGithub({ behind: 0 }); + await clearLabel({ github: fake.github, context: ctx(['needs update']), core }); + assert.deepEqual(fake.names(), ['compare', 'removeLabel']); + fake = fakeGithub({ behind: 2 }); + await clearLabel({ github: fake.github, context: ctx(['needs update']), core }); + assert.deepEqual(fake.names(), ['compare']); + fake = fakeGithub({ behind: 0 }); + await clearLabel({ github: fake.github, context: ctx(['area: docs']), core }); + assert.deepEqual(fake.names(), []); +}); + +test('messages are plain: no em-dashes', () => { + const all = [ + MESSAGES.updated('main', NEW, 'running'), MESSAGES.updated('main', NEW, 'held'), MESSAGES.updated('main', NEW, 'noCla'), + MESSAGES.updating('main'), + MESSAGES.upToDate('main'), MESSAGES.conflicts('main', ['a'], true), MESSAGES.conflicts('main', null, false), + MESSAGES.useButton('main', false, true), MESSAGES.useButton('main', true, false), + MESSAGES.headMoved, MESSAGES.closed, MESSAGES.notAllowed, MESSAGES.failed('main'), + ]; + for (const m of all) assert.ok(!m.includes('—'), m); +}); + +// The real conflict finder against a throwaway local repository (no network): a file both sides changed is +// reported, a file only one side changed is not, and a clean merge gives []. +test('gitConflicts lists exactly the conflicting files', (t) => { + try { + execFileSync('git', ['merge-tree', '-h'], { stdio: 'ignore' }); + } catch (err) { + if (err.status !== 129) return t.skip('git is not available'); + } + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'update-test-')); + t.after(() => fs.rmSync(root, { recursive: true, force: true })); + const src = path.join(root, 'src'); + const env = { ...process.env, GIT_AUTHOR_NAME: 'Maya Rao', GIT_AUTHOR_EMAIL: 'maya@example.com', GIT_COMMITTER_NAME: 'Maya Rao', GIT_COMMITTER_EMAIL: 'maya@example.com' }; + const g = (...args) => execFileSync('git', ['-C', src, ...args], { env, encoding: 'utf8' }); + const write = (f, s) => fs.writeFileSync(path.join(src, f), s); + fs.mkdirSync(src); + g('init', '--quiet', '--initial-branch=main'); + write('a.txt', 'one\n'); write('b.txt', 'one\n'); write('c.txt', 'one\n'); + g('add', '.'); g('commit', '--quiet', '-m', 'start'); + g('switch', '--quiet', '-c', 'feature'); + write('a.txt', 'feature\n'); write('b.txt', 'feature\n'); + g('commit', '--quiet', '-am', 'feature'); + g('update-ref', 'refs/pull/7/head', 'feature'); + g('update-ref', 'refs/pull/8/head', 'main'); + g('switch', '--quiet', 'main'); + write('a.txt', 'main\n'); write('c.txt', 'main\n'); + g('commit', '--quiet', '-am', 'main'); + + // gitConflicts fetches from https://github.com//.git; point that at the local repository. + const prefix = 'https://github.com/'; + const key = 'GIT_CONFIG_COUNT'; + const saved = { [key]: process.env[key], k: process.env.GIT_CONFIG_KEY_0, v: process.env.GIT_CONFIG_VALUE_0 }; + process.env[key] = '1'; + process.env.GIT_CONFIG_KEY_0 = `url.${pathToUrl(root)}/.insteadOf`; + process.env.GIT_CONFIG_VALUE_0 = prefix; + t.after(() => { + for (const [k, v] of [[key, saved[key]], ['GIT_CONFIG_KEY_0', saved.k], ['GIT_CONFIG_VALUE_0', saved.v]]) { + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + } + }); + fs.mkdirSync(path.join(root, 'owner')); + fs.renameSync(src, path.join(root, 'owner', 'repo.git')); + const conflicts = run.gitConflicts({ owner: 'owner', repo: 'repo', number: 7, base: 'main', dir: path.join(root, 'check') }); + assert.deepEqual(conflicts, ['a.txt']); + assert.deepEqual(run.gitConflicts({ owner: 'owner', repo: 'repo', number: 8, base: 'main', dir: path.join(root, 'check') }), []); + assert.throws(() => run.gitConflicts({ owner: 'owner', repo: 'repo', number: '7; rm', base: 'main', dir: path.join(root, 'check') })); +}); + +function pathToUrl(p) { + return 'file://' + (p.startsWith('/') ? '' : '/') + p.replace(/\\/g, '/'); +} diff --git a/.github/workflows/bot-scripts.yaml b/.github/workflows/bot-scripts.yaml index 814c3ed8..be4315bb 100644 --- a/.github/workflows/bot-scripts.yaml +++ b/.github/workflows/bot-scripts.yaml @@ -7,6 +7,7 @@ on: - .github/scripts/** - .github/workflows/assign.yaml - .github/workflows/stale.yaml + - .github/workflows/update.yaml - .github/workflows/bot-scripts.yaml permissions: diff --git a/.github/workflows/update.yaml b/.github/workflows/update.yaml new file mode 100644 index 00000000..48d54c2a --- /dev/null +++ b/.github/workflows/update.yaml @@ -0,0 +1,70 @@ +# "/update" comments on pull requests: bring main into the branch (CONTRIBUTING.md, "We do the chores"). The logic is +# in .github/scripts/update.js (tested by .github/scripts/update.test.js). It reads the comment only to compare it +# with "/update", and never checks out or runs the pull request's code: conflicts are found with git merge-tree on +# the git objects alone. +name: Update pull requests + +on: + issue_comment: + types: [created] + # A push to a pull request labelled "needs update" takes the label off once the branch is up to date (for example + # after the author clicked "Update branch", or a maintainer merged main in by hand). + pull_request_target: + types: [synchronize] + +permissions: {} + +jobs: + update: + # Pull requests only (an issue comment also fires issue_comment), and only a comment that is exactly the command + # (allowing a trailing space or line break), so other comments never queue here. + if: >- + github.event_name == 'issue_comment' && + github.event.issue.pull_request != null && + contains(fromJSON('["/update", "/update ", "/update\n", "/update\r\n"]'), github.event.comment.body) + concurrency: + group: update-pr-${{ github.event.issue.number }} + runs-on: ubuntu-latest + timeout-minutes: 8 + # Each one was tried for real (#464): update-branch needs both contents and pull-requests write, also for a fork + # with maintainer edits on. GitHub holds the checks of a commit made with this token, so restarting them needs + # the other two (see update.js). + permissions: + contents: write # update-branch writes the merge commit to the pull request's branch + pull-requests: write # update-branch, the reply and the "needs update" label + actions: write # approve the held CI runs on the bot's own merge commit + checks: write # copy the CLA check to that merge commit, as pull_request_target doesn't run for it + steps: + # issue_comment always runs on the default branch, so this is our own script, never a contributor's. + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + sparse-checkout: .github/scripts + persist-credentials: false + - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const run = require('./.github/scripts/update.js'); + await run({ github, context, core }); + + clear-label: + if: >- + github.event_name == 'pull_request_target' && + contains(github.event.pull_request.labels.*.name, 'needs update') + concurrency: + group: update-pr-${{ github.event.pull_request.number }} + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + pull-requests: write # removing the label + steps: + # pull_request_target checks out the base branch: our script, never the pull request's code. + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + sparse-checkout: .github/scripts + persist-credentials: false + - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const { clearLabel } = require('./.github/scripts/update.js'); + await clearLabel({ github, context, core }); diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b0fd7f7f..b08e8436 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -74,6 +74,22 @@ Life happens, and that's fine. To keep issues moving for everyone, a bot checks Draft pull requests are never touched, and the clock only runs while we are waiting on you, never while you are waiting on us. +### We do the chores + +You bring the change; we take care of the busywork around it, so a pull request never stalls on housekeeping. + +- **What we may do on your branch:** merge `main` into it, resolve conflicts, rename or add a changelog file, + fix lint or formatting, re-run checks, and make tiny fixes a review asked for. +- **What we always do:** say exactly what we pushed in a comment, never force-push, and only push when + **Allow edits by maintainers** is ticked on your pull request (it is by default). +- **What stays yours:** anything that changes how your code behaves or how it is designed comes back to you as + a review, never as a surprise commit. +- **Your branch doesn't need to be up to date to merge.** The merge queue tests your change on top of the + latest `main` before it lands. If you want `main` merged in anyway, comment `/update` on the pull request: + a bot tries to merge it in (a merge, never a rebase) and tells you what happened. If it can't update your + branch, it shows you how to do it with GitHub's **Update branch** button. If there are conflicts, it lists + the files and labels the pull request `needs update`, and a maintainer resolves them for you. + ### PR titles Use [Conventional Commits](https://www.conventionalcommits.org/) with an area, because the title becomes the diff --git a/docs/AGENT_WORKFLOW.md b/docs/AGENT_WORKFLOW.md index 87a0932d..4c40a740 100644 --- a/docs/AGENT_WORKFLOW.md +++ b/docs/AGENT_WORKFLOW.md @@ -142,3 +142,13 @@ drive (a full one fails dozens of tests with "disk is full"). Give pytest a fold - No keys, tokens, real names or personal data anywhere in the PR. - After opening it, wait for the checks and the CodeRabbit review. Fix the real findings (two rounds at most, then ask a maintainer), and leave merging to a maintainer. + +## Someone else's pull request + +When you work for the maintainers on a contributor's pull request, do the chores and leave the substance to them +([CONTRIBUTING.md](../CONTRIBUTING.md#we-do-the-chores)). Merging `main`, resolving conflicts, the `changes/` +file, lint fixes, re-running checks and tiny fixes a review asked for are yours to push, as merge commits and +only when "Allow edits by maintainers" is on; never force-push, and comment with exactly what you pushed. Anything +that changes behaviour or design goes back to them as a review. Pull requests labelled `needs update` still need +`main` merged in, because of conflicts or because the bot could not update the branch itself; the `/update` bot +takes the label off once the branch is up to date.