From 99fd12c0335ec2090bd813c71d3a752e7c8e74cd Mon Sep 17 00:00:00 2001 From: itsskofficial Date: Sun, 18 Oct 2026 12:41:00 +0530 Subject: [PATCH 1/7] ci(bots): add a /update comment command for pull requests --- .github/scripts/update.js | 280 ++++++++++++++++++++++++++++++++++ .github/workflows/update.yaml | 65 ++++++++ 2 files changed, 345 insertions(+) create mode 100644 .github/scripts/update.js create mode 100644 .github/workflows/update.yaml diff --git a/.github/scripts/update.js b/.github/scripts/update.js new file mode 100644 index 00000000..ddc00c0d --- /dev/null +++ b/.github/scripts/update.js @@ -0,0 +1,280 @@ +// "/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 can push the branch -> merges main in with GitHub's update-branch API (a merge, never a rebase) +// - it can't push the branch (a fork) -> points the author at GitHub's own "Update branch" button and adds the +// label so a maintainer can do it instead +// When the branch is pushed again and is up to date, the label comes off by itself (clearLabel). +// +// 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 for people with write access (the PR author may always use the command too). +const WRITERS = ['OWNER', 'MEMBER', 'COLLABORATOR']; +const POLL = { tries: 12, ms: 5000 }; + +const code = (s) => `\`${s}\``; + +const MESSAGES = { + updated: (base, sha) => + `Merged the latest ${code(base)} into this branch` + (sha ? ` (${sha.slice(0, 7)})` : '') + '. ' + + 'Nothing else changed. GitHub does not start checks for a commit made by a bot, so they run again with ' + + 'your next push.', + updating: (base) => + `GitHub is merging the latest ${code(base)} into this branch now; the new commit shows up here in a minute. ` + + 'GitHub does not start checks for a commit made by a bot, so they run again with your next push.', + 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) => + `I can't push to this branch from here, because it lives in your fork. It has no conflicts with ` + + `${code(base)}, so 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 do it if you'd rather not. ` + + 'You don\'t have to: 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. */ +function mayUpdate({ association, commenter, author }) { + if (WRITERS.includes(String(association || '').toUpperCase())) return true; + return !!commenter && String(commenter).toLowerCase() === String(author || '').toLowerCase(); +} + +/** 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; +} + +/** + * Entry point for actions/github-script: + * `await require('./.github/scripts/update.js')({ github, context, core })`. + * `findConflicts` and `sleep` 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.'); + + 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}` }); + + if (!mayUpdate({ association: comment.author_association, commenter, author: issue.user && issue.user.login })) { + 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 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; + if (action === 'updateBranch') { + try { + await github.rest.pulls.updateBranch({ owner, repo, pull_number: issue_number, expected_head_sha: pr.head.sha }); + newSha = await waitForNewHead(github, { + owner, + repo, + pull_number: issue_number, + oldSha: pr.head.sha, + sleep: sleep || ((ms) => new Promise((r) => setTimeout(r, ms))), + poll, + }); + 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'; + // The workflow's token may not push to a fork, even with maintainer edits allowed. + else if (fork && [403, 404, 422].includes(err.status)) action = 'useButton'; + else action = 'failed'; + } + } + + if (action === 'upToDate' || action === 'updated') 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) : MESSAGES.updating(base)), + conflicts: () => MESSAGES.conflicts(base, conflicts, canEdit), + useButton: () => MESSAGES.useButton(base), + headMoved: () => MESSAGES.headMoved, + failed: () => MESSAGES.failed(base), + }[action](); + core.info(`Result: ${action}`); + 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/workflows/update.yaml b/.github/workflows/update.yaml new file mode 100644 index 00000000..7eebad39 --- /dev/null +++ b/.github/workflows/update.yaml @@ -0,0 +1,65 @@ +# "/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: 5 + permissions: + contents: write # update-branch writes the merge commit to the pull request's branch + pull-requests: write # the reply and the "needs update" label + 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 }); From af00a3b97db879a716097491eff60ba5c61ef275 Mon Sep 17 00:00:00 2001 From: itsskofficial Date: Sun, 18 Oct 2026 12:57:42 +0530 Subject: [PATCH 2/7] chore(repo): /update merges main into a PR, and we do the chores on contributor PRs CONTRIBUTING.md now says what maintainers may push to a contributor's branch (merging main, conflicts, the changes/ file, lint, re-running checks, tiny review fixes), what they always do (say what they pushed, never force-push, only with maintainer edits on) and what stays the contributor's (behaviour and design). docs/AGENT_WORKFLOW.md has the same for agents. A "/update" comment on a pull request, by its author or anyone with write access, merges main in with GitHub's update-branch API. On conflicts the bot lists the files (git merge-tree on the objects only, never a checkout) and labels the PR "needs update" for a maintainer; the label comes off once the branch is up to date. A fork without maintainer edits gets pointed at the "Update branch" button. GitHub holds the checks of a commit made with the workflow token and skips pull_request_target, so after its own merge commit the bot approves the held runs (only if the old head ran without approval) and copies the CLA check from the old head. Closes #464 --- .github/scripts/update.js | 137 ++++++++-- .github/scripts/update.test.js | 387 +++++++++++++++++++++++++++++ .github/workflows/bot-scripts.yaml | 1 + .github/workflows/update.yaml | 9 +- CONTRIBUTING.md | 15 ++ docs/AGENT_WORKFLOW.md | 9 + 6 files changed, 530 insertions(+), 28 deletions(-) create mode 100644 .github/scripts/update.test.js diff --git a/.github/scripts/update.js b/.github/scripts/update.js index ddc00c0d..0cb5e2ae 100644 --- a/.github/scripts/update.js +++ b/.github/scripts/update.js @@ -3,11 +3,22 @@ // 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 can push the branch -> merges main in with GitHub's update-branch API (a merge, never a rebase) -// - it can't push the branch (a fork) -> points the author at GitHub's own "Update branch" button and adds the -// label so a maintainer can do it instead +// - 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 +// - 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. @@ -19,18 +30,20 @@ const LABEL = { }; // author_association values for people with write access (the PR author may always use the command too). const WRITERS = ['OWNER', 'MEMBER', 'COLLABORATOR']; +const CLA_CHECK = 'CLAAssistant'; const POLL = { tries: 12, ms: 5000 }; const code = (s) => `\`${s}\``; const MESSAGES = { - updated: (base, sha) => - `Merged the latest ${code(base)} into this branch` + (sha ? ` (${sha.slice(0, 7)})` : '') + '. ' + - 'Nothing else changed. GitHub does not start checks for a commit made by a bot, so they run again with ' + - 'your next push.', + updated: (base, sha, checks) => + `Merged the latest ${code(base)} into this branch (${sha.slice(0, 7)}). Nothing else changed. ` + + (checks === 'running' + ? 'The checks are running again.' + : 'GitHub holds the checks for a commit made by a bot, so a maintainer will start them.'), updating: (base) => - `GitHub is merging the latest ${code(base)} into this branch now; the new commit shows up here in a minute. ` + - 'GitHub does not start checks for a commit made by a bot, so they run again with your next push.', + `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` + @@ -42,10 +55,10 @@ const MESSAGES = { : '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) => - `I can't push to this branch from here, because it lives in your fork. It has no conflicts with ` + - `${code(base)}, so 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 do it if you'd rather not. ` + - 'You don\'t have to: the merge queue tests your change against the latest main anyway.', + `I can't push to this branch, because it lives in a fork that doesn't allow edits by maintainers. It has ` + + `no conflicts with ${code(base)}, so 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: @@ -164,10 +177,76 @@ async function waitForNewHead(github, { owner, repo, pull_number, oldSha, sleep, 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). */ +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)) return core.info('The CLA check already ran on the merge commit.'); + const old = await latest(oldSha); + if (!old || old.status !== 'completed') return core.info('No finished CLA check on the old head to copy.'); + 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.', + }, + }); + core.info(`Copied the CLA check (${old.conclusion}) from ${oldSha.slice(0, 7)}.`); +} + +/** + * 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}).`); + } + return 'running'; +} + /** * Entry point for actions/github-script: * `await require('./.github/scripts/update.js')({ github, context, core })`. - * `findConflicts` and `sleep` are swapped out by the tests. + * `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; @@ -175,6 +254,7 @@ async function run({ github, context, core, findConflicts, sleep, poll = POLL, w 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; @@ -190,6 +270,7 @@ async function run({ github, context, core, findConflicts, sleep, poll = POLL, w 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; @@ -215,41 +296,45 @@ async function run({ github, context, core, findConflicts, sleep, poll = POLL, w ); let newSha = null; + let checks = 'held'; if (action === 'updateBranch') { try { - await github.rest.pulls.updateBranch({ owner, repo, pull_number: issue_number, expected_head_sha: pr.head.sha }); - newSha = await waitForNewHead(github, { - owner, - repo, - pull_number: issue_number, - oldSha: pr.head.sha, - sleep: sleep || ((ms) => new Promise((r) => setTimeout(r, ms))), - poll, - }); + 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'; - // The workflow's token may not push to a fork, even with maintainer edits allowed. + // 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 }); + if (newSha && (await isOurMerge(github, { owner, repo, oldSha, newSha }))) { + try { + await carryClaCheck(github, { owner, repo, oldSha, newSha, core }); + checks = await approveHeldRuns(github, { owner, repo, oldSha, newSha, sleep, poll, core }); + } catch (err) { + core.warning(`Could not restart the checks: ${err.message}`); + } + } + } if (action === 'upToDate' || action === 'updated') 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) : MESSAGES.updating(base)), + updated: () => (newSha ? MESSAGES.updated(base, newSha, checks) : MESSAGES.updating(base)), conflicts: () => MESSAGES.conflicts(base, conflicts, canEdit), useButton: () => MESSAGES.useButton(base), headMoved: () => MESSAGES.headMoved, failed: () => MESSAGES.failed(base), }[action](); - core.info(`Result: ${action}`); + core.info(`Result: ${action}${action === 'updated' ? `, checks ${checks}` : ''}`); return say(reply); } diff --git a/.github/scripts/update.test.js b/.github/scripts/update.test.js new file mode 100644 index 00000000..51f594da --- /dev/null +++ b/.github/scripts/update.test.js @@ -0,0 +1,387 @@ +// 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; + 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 }), + }, + checks: { + listForRef: async ({ ref }) => ({ + data: { check_runs: [ref === OLD ? oldCla : newCla].filter(Boolean) }, + }), + create: async (args) => calls.push(['createCheck', args]), + }, + actions: { + listWorkflowRunsForRepo: async ({ head_sha, status }) => ({ + data: { workflow_runs: head_sha === OLD ? oldRuns : status === 'action_required' ? heldRuns : [] }, + }), + approveWorkflowRun: async (args) => calls.push(['approve', 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', () => { + assert.equal(mayUpdate({ association: 'CONTRIBUTOR', commenter: 'Maya-Rao', author: 'maya-rao' }), true); + for (const association of ['OWNER', 'MEMBER', 'COLLABORATOR']) { + assert.equal(mayUpdate({ association, commenter: 'ravi', author: 'maya-rao' }), true); + } + for (const association of ['CONTRIBUTOR', 'FIRST_TIME_CONTRIBUTOR', 'NONE', undefined]) { + assert.equal(mayUpdate({ association, commenter: 'ravi', author: 'maya-rao' }), false); + } +}); + +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 () => { + const fake = fakeGithub(); + await runWith(fake, context({ login: 'ravi', association: 'CONTRIBUTOR' })); + assert.deepEqual(fake.names(), ['createComment']); + assert.equal(reply(fake)[0], `@ravi ${MESSAGES.notAllowed}`); +}); + +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]); + 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')}`); + assert.match(reply(fake)[0], /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')}`); +}); + +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.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('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')}`); +}); + +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.updating('main'), + MESSAGES.upToDate('main'), MESSAGES.conflicts('main', ['a'], true), MESSAGES.conflicts('main', null, false), + MESSAGES.useButton('main'), 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 index 7eebad39..48d54c2a 100644 --- a/.github/workflows/update.yaml +++ b/.github/workflows/update.yaml @@ -25,10 +25,15 @@ jobs: concurrency: group: update-pr-${{ github.event.issue.number }} runs-on: ubuntu-latest - timeout-minutes: 5 + 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 # the reply and the "needs update" label + 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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b0fd7f7f..faaab191 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -74,6 +74,21 @@ 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 merges it in (a merge, never a rebase) and tells you what happened. 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..bcc1a292 100644 --- a/docs/AGENT_WORKFLOW.md +++ b/docs/AGENT_WORKFLOW.md @@ -142,3 +142,12 @@ 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` are the +ones whose conflicts are waiting for you; the `/update` bot takes the label off once the branch is up to date. From b290da22bf8c773eab61137a775629c5c88efdb9 Mon Sep 17 00:00:00 2001 From: itsskofficial Date: Sun, 18 Oct 2026 13:29:57 +0530 Subject: [PATCH 3/7] fix(repo): copy the CLA check only after the approved runs started A check run made with the workflow token is filed under one of the commit's workflow runs, and one filed before that run was approved is dropped from the latest checks when it starts. Approve first, wait for the runs to leave the queue, then copy the CLA check and confirm GitHub lists it. --- .github/scripts/update.js | 42 ++++++++++++++++++++++++++-------- .github/scripts/update.test.js | 38 ++++++++++++++++++++++++------ 2 files changed, 64 insertions(+), 16 deletions(-) diff --git a/.github/scripts/update.js b/.github/scripts/update.js index 0cb5e2ae..42cc61b1 100644 --- a/.github/scripts/update.js +++ b/.github/scripts/update.js @@ -13,7 +13,7 @@ // 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 +// (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. @@ -38,9 +38,11 @@ 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. ` + - (checks === 'running' - ? 'The checks are running again.' - : 'GitHub holds the checks for a commit made by a bot, so a maintainer will start them.'), + { + 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.', @@ -189,14 +191,24 @@ async function isOurMerge(github, { owner, repo, oldSha, newSha }) { ); } -/** Copies the CLA check's result from the old head to the merge commit (see the top of this file). */ +/** + * 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)) return core.info('The CLA check already ran on the merge commit.'); + 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') return core.info('No finished CLA check on the old head to copy.'); + 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, @@ -212,7 +224,9 @@ async function carryClaCheck(github, { owner, repo, oldSha, newSha, core }) { 'no new contributor.', }, }); - core.info(`Copied the CLA check (${old.conclusion}) from ${oldSha.slice(0, 7)}.`); + 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; } /** @@ -240,6 +254,14 @@ async function approveHeldRuns(github, { owner, repo, oldSha, newSha, sleep, pol 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'; } @@ -315,8 +337,10 @@ async function run({ github, context, core, findConflicts, sleep, poll = POLL, w newSha = await waitForNewHead(github, { owner, repo, pull_number: issue_number, oldSha, sleep, poll }); if (newSha && (await isOurMerge(github, { owner, repo, oldSha, newSha }))) { try { - await carryClaCheck(github, { owner, repo, oldSha, newSha, core }); 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}`); } diff --git a/.github/scripts/update.test.js b/.github/scripts/update.test.js index 51f594da..510dc2c8 100644 --- a/.github/scripts/update.test.js +++ b/.github/scripts/update.test.js @@ -39,6 +39,8 @@ function fakeGithub(opts = {}) { 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' } }, @@ -66,15 +68,26 @@ function fakeGithub(opts = {}) { }, checks: { listForRef: async ({ ref }) => ({ - data: { check_runs: [ref === OLD ? oldCla : newCla].filter(Boolean) }, + data: { check_runs: [ref === OLD ? oldCla : newCla || (opts.claListed !== false && created)].filter(Boolean) }, }), - create: async (args) => calls.push(['createCheck', args]), + create: async (args) => { + calls.push(['createCheck', args]); + created = { status: 'completed', conclusion: args.conclusion }; + }, }, actions: { - listWorkflowRunsForRepo: async ({ head_sha, status }) => ({ - data: { workflow_runs: head_sha === OLD ? oldRuns : status === 'action_required' ? heldRuns : [] }, - }), - approveWorkflowRun: async (args) => calls.push(['approve', args.run_id]), + // 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 () => { @@ -196,6 +209,8 @@ test('a clean same-repo branch: merges main, restarts the checks, clears the lab 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')}`); @@ -302,6 +317,14 @@ test('the CLA check is copied as it was, and not when one is there already or no } }); +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' } } }); @@ -327,7 +350,8 @@ test('clearLabel takes the label off once the branch is up to date, and only the test('messages are plain: no em-dashes', () => { const all = [ - MESSAGES.updated('main', NEW, 'running'), MESSAGES.updated('main', NEW, 'held'), MESSAGES.updating('main'), + 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'), MESSAGES.headMoved, MESSAGES.closed, MESSAGES.notAllowed, MESSAGES.failed('main'), ]; From 229246f888ffa4e23d29d0ec9601eafee48302fd Mon Sep 17 00:00:00 2001 From: itsskofficial Date: Sun, 18 Oct 2026 13:42:16 +0530 Subject: [PATCH 4/7] fix(repo): the button reply only states what the bot knows Say GitHub refused the push when maintainer edits are on, and claim no conflicts only when they were checked. The docs say the bot tries the merge and may point at the Update branch button, and what the needs update label means. --- .github/scripts/update.js | 13 ++++++++----- .github/scripts/update.test.js | 19 +++++++++++++++---- CONTRIBUTING.md | 3 ++- docs/AGENT_WORKFLOW.md | 5 +++-- 4 files changed, 28 insertions(+), 12 deletions(-) diff --git a/.github/scripts/update.js b/.github/scripts/update.js index 42cc61b1..354aeb14 100644 --- a/.github/scripts/update.js +++ b/.github/scripts/update.js @@ -56,10 +56,13 @@ const MESSAGES = { '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) => - `I can't push to this branch, because it lives in a fork that doesn't allow edits by maintainers. It has ` + - `no conflicts with ${code(base)}, so 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. ` + + 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.', @@ -354,7 +357,7 @@ async function run({ github, context, core, findConflicts, sleep, poll = POLL, w upToDate: () => MESSAGES.upToDate(base), updated: () => (newSha ? MESSAGES.updated(base, newSha, checks) : MESSAGES.updating(base)), conflicts: () => MESSAGES.conflicts(base, conflicts, canEdit), - useButton: () => MESSAGES.useButton(base), + useButton: () => MESSAGES.useButton(base, canEdit, Array.isArray(conflicts) && conflicts.length === 0), headMoved: () => MESSAGES.headMoved, failed: () => MESSAGES.failed(base), }[action](); diff --git a/.github/scripts/update.test.js b/.github/scripts/update.test.js index 510dc2c8..6a414ae8 100644 --- a/.github/scripts/update.test.js +++ b/.github/scripts/update.test.js @@ -230,8 +230,8 @@ test('a fork without maintainer edits: points at the button and labels it, no pu 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')}`); - assert.match(reply(fake)[0], /click \*\*Update branch\*\*/); + 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 () => { @@ -241,7 +241,17 @@ test('a fork GitHub refuses to push to falls back to the button', async () => { }); await runWith(fake); assert.ok(fake.names().includes('addLabels')); - assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.useButton('main')}`); + 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 () => { @@ -353,7 +363,8 @@ test('messages are plain: no em-dashes', () => { 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'), MESSAGES.headMoved, MESSAGES.closed, MESSAGES.notAllowed, MESSAGES.failed('main'), + 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); }); diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index faaab191..b08e8436 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -86,7 +86,8 @@ You bring the change; we take care of the busywork around it, so a pull request 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 merges it in (a merge, never a rebase) and tells you what happened. If there are conflicts, it lists + 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 diff --git a/docs/AGENT_WORKFLOW.md b/docs/AGENT_WORKFLOW.md index bcc1a292..4c40a740 100644 --- a/docs/AGENT_WORKFLOW.md +++ b/docs/AGENT_WORKFLOW.md @@ -149,5 +149,6 @@ When you work for the maintainers on a contributor's pull request, do the chores ([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` are the -ones whose conflicts are waiting for you; the `/update` bot takes the label off once the branch is up to date. +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. From 2a3061dc94a0b6887b8f3ad8acb98b43c913de3b Mon Sep 17 00:00:00 2001 From: itsskofficial Date: Sun, 18 Oct 2026 13:53:01 +0530 Subject: [PATCH 5/7] fix(repo): keep needs update until the merge commit shows up --- .github/scripts/update.js | 3 ++- .github/scripts/update.test.js | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/scripts/update.js b/.github/scripts/update.js index 354aeb14..c53baa11 100644 --- a/.github/scripts/update.js +++ b/.github/scripts/update.js @@ -350,7 +350,8 @@ async function run({ github, context, core, findConflicts, sleep, poll = POLL, w } } - if (action === 'upToDate' || action === 'updated') await removeLabel(github, { owner, repo, issue_number }); + // Without the new head in sight the branch isn't known to be current yet, so the label stays. + if (action === 'upToDate' || (action === 'updated' && newSha)) await removeLabel(github, { owner, repo, issue_number }); if (['conflicts', 'useButton', 'failed'].includes(action)) await addLabel(github, { owner, repo, issue_number }); const reply = { diff --git a/.github/scripts/update.test.js b/.github/scripts/update.test.js index 6a414ae8..be066b0c 100644 --- a/.github/scripts/update.test.js +++ b/.github/scripts/update.test.js @@ -340,6 +340,7 @@ test('when the merge commit has not shown up yet, the reply says it is on its wa 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 () => { From ae6e7337660c72136602ae0ce22892bacbf89021 Mon Sep 17 00:00:00 2001 From: itsskofficial Date: Sun, 18 Oct 2026 14:05:32 +0530 Subject: [PATCH 6/7] fix(repo): /update checks real write access, not just the association OWNER, MEMBER and COLLABORATOR don't prove write access: an organisation member or a read-only collaborator has one too. For anyone but the PR author the bot now looks up the repository permission and needs admin, maintain or write. Tried for real: the workflow token reads it (admin for the owners, read for an outsider, none for an app). --- .github/scripts/update.js | 25 +++++++++++++---- .github/scripts/update.test.js | 51 ++++++++++++++++++++++++++++------ 2 files changed, 61 insertions(+), 15 deletions(-) diff --git a/.github/scripts/update.js b/.github/scripts/update.js index c53baa11..dd323e66 100644 --- a/.github/scripts/update.js +++ b/.github/scripts/update.js @@ -28,7 +28,7 @@ const LABEL = { color: 'fbca04', description: 'Behind main or has conflicts; a maintainer will bring it up to date', }; -// author_association values for people with write access (the PR author may always use the command too). +// 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 }; @@ -79,10 +79,22 @@ function parseCommand(body) { return String(body || '').trim() === '/update'; } -/** The PR author or someone with write access may update a branch. */ -function mayUpdate({ association, commenter, author }) { - if (WRITERS.includes(String(association || '').toUpperCase())) return true; - return !!commenter && String(commenter).toLowerCase() === String(author || '').toLowerCase(); +/** + * 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. */ @@ -286,7 +298,8 @@ async function run({ github, context, core, findConflicts, sleep, poll = POLL, w const commenter = comment.user.login; const say = (text) => github.rest.issues.createComment({ owner, repo, issue_number, body: `@${commenter} ${text}` }); - if (!mayUpdate({ association: comment.author_association, commenter, author: issue.user && issue.user.login })) { + 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); } diff --git a/.github/scripts/update.test.js b/.github/scripts/update.test.js index be066b0c..32bd0167 100644 --- a/.github/scripts/update.test.js +++ b/.github/scripts/update.test.js @@ -65,6 +65,7 @@ function fakeGithub(opts = {}) { return { data: { ahead_by: behind } }; }, getCommit: async () => ({ data: merge }), + getCollaboratorPermissionLevel: async () => ({ data: { permission: opts.permission || 'read' } }), }, checks: { listForRef: async ({ ref }) => ({ @@ -142,14 +143,37 @@ test('only an exact /update counts', () => { assert.equal(parseCommand(undefined), false); }); -test('the author and people with write access may update, nobody else', () => { - assert.equal(mayUpdate({ association: 'CONTRIBUTOR', commenter: 'Maya-Rao', author: 'maya-rao' }), true); - for (const association of ['OWNER', 'MEMBER', 'COLLABORATOR']) { - assert.equal(mayUpdate({ association, commenter: 'ravi', author: 'maya-rao' }), true); +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(mayUpdate({ association, commenter: 'ravi', author: 'maya-rao' }), false); + assert.equal(await may(association, 'ravi'), false); } + assert.deepEqual(lookups, []); }); test('isFork and decide', () => { @@ -177,10 +201,19 @@ test('comments on issues, from bots, and other text are ignored', async () => { }); test('someone else on the pull request is told who can use it, and nothing changes', async () => { - const fake = fakeGithub(); - await runWith(fake, context({ login: 'ravi', association: 'CONTRIBUTOR' })); - assert.deepEqual(fake.names(), ['createComment']); - assert.equal(reply(fake)[0], `@ravi ${MESSAGES.notAllowed}`); + 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 () => { From aa55bb0004cbb2f245d2f1b5af17ee099dea4a54 Mon Sep 17 00:00:00 2001 From: itsskofficial Date: Sun, 18 Oct 2026 14:26:32 +0530 Subject: [PATCH 7/7] fix(repo): take needs update off only after GitHub's own merge commit --- .github/scripts/update.js | 9 ++++++--- .github/scripts/update.test.js | 1 + 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/.github/scripts/update.js b/.github/scripts/update.js index dd323e66..9a0edca5 100644 --- a/.github/scripts/update.js +++ b/.github/scripts/update.js @@ -334,6 +334,7 @@ async function run({ github, context, core, findConflicts, sleep, poll = POLL, w ); let newSha = null; + let ourMerge = false; let checks = 'held'; if (action === 'updateBranch') { try { @@ -351,7 +352,8 @@ async function run({ github, context, core, findConflicts, sleep, poll = POLL, w } if (action === 'updated') { newSha = await waitForNewHead(github, { owner, repo, pull_number: issue_number, oldSha, sleep, poll }); - if (newSha && (await isOurMerge(github, { owner, repo, oldSha, newSha }))) { + 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 }))) { @@ -363,8 +365,9 @@ async function run({ github, context, core, findConflicts, sleep, poll = POLL, w } } - // Without the new head in sight the branch isn't known to be current yet, so the label stays. - if (action === 'upToDate' || (action === 'updated' && newSha)) await removeLabel(github, { owner, repo, issue_number }); + // 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 = { diff --git a/.github/scripts/update.test.js b/.github/scripts/update.test.js index 32bd0167..487b254d 100644 --- a/.github/scripts/update.test.js +++ b/.github/scripts/update.test.js @@ -345,6 +345,7 @@ test('nothing is approved or copied for a commit that is not GitHub\'s own 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/); } });