From d3822599e0a43d046cfba221a95accff6ece3e5c Mon Sep 17 00:00:00 2001 From: Sarthak Karandikar Date: Mon, 26 Oct 2026 23:21:46 +0530 Subject: [PATCH 1/3] fix(ci): /update says why it can't update a fork, and logs GitHub's answer --- .github/scripts/update.js | 128 ++++++++++++++++++++++++------ .github/scripts/update.test.js | 137 +++++++++++++++++++++++++++++---- .github/workflows/update.yaml | 3 +- CONTRIBUTING.md | 9 ++- docs/AGENT_WORKFLOW.md | 15 ++++ 5 files changed, 251 insertions(+), 41 deletions(-) diff --git a/.github/scripts/update.js b/.github/scripts/update.js index 9a0edca5a..800f3cec0 100644 --- a/.github/scripts/update.js +++ b/.github/scripts/update.js @@ -4,9 +4,15 @@ // - 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. +// works for forks too, anyone's, when "Allow edits by maintainers" is on. +// - a fork, and the merge changes a workflow file -> doesn't try: GitHub refuses the workflow token any change +// under .github/workflows/ in a fork, whoever owns it ("refusing to allow a GitHub +// App to create or update workflow ... without `workflows` permission", #473), and +// GITHUB_TOKEN can't be given that permission. (A branch in this repository is fine.) +// Names the files, points at the "Update branch" button and adds the label. // - 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 +// Whenever update-branch fails, GitHub's status and message go into the run log (as a warning) and the job summary. // 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 @@ -32,9 +38,18 @@ const LABEL = { const WRITERS = ['OWNER', 'MEMBER', 'COLLABORATOR']; const CLA_CHECK = 'CLAAssistant'; const POLL = { tries: 12, ms: 5000 }; +// In a fork GitHub only lets a token with the `workflows` permission write here, and the workflow token can't have it. +const WORKFLOWS_DIR = '.github/workflows/'; const code = (s) => `\`${s}\``; +// The end of every reply that hands the update to the author or a maintainer. +const buttonHint = (base, clean) => + (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."; + const MESSAGES = { updated: (base, sha, checks) => `Merged the latest ${code(base)} into this branch (${sha.slice(0, 7)}). Nothing else changed. ` + @@ -60,10 +75,12 @@ const MESSAGES = { (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.", + buttonHint(base, clean), + workflows: (base, files, clean) => + `I can't merge ${code(base)} into this branch myself: it would change the repository's workflow files` + + (files && files.length ? ` (${files.map(code).join(', ')})` : '') + + ', and GitHub never lets a bot push those to a fork. ' + + buttonHint(base, clean), 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: @@ -106,13 +123,17 @@ function isFork(pr, owner, repo) { /** * 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". + * workflows: files under .github/workflows/ the merge would change ([] for none, null if unknown). + * Returns "upToDate" | "conflicts" | "useButton" | "workflows" | "updateBranch". */ -function decide({ behind, conflicts, fork, canEdit }) { +function decide({ behind, conflicts, workflows, 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'; + // On a fork (anyone's, the owner's too) GitHub refuses the workflow token any change to a workflow file, so + // don't try. A branch in this repository is updated fine (#473). + if (fork && workflows && workflows.length) return 'workflows'; return 'updateBranch'; } @@ -128,10 +149,11 @@ async function behindBy(github, { owner, repo, pr }) { } /** - * 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. + * Merges the base branch into the PR with `git merge-tree` in a fresh bare repository (only git objects, never a + * checkout). Returns { conflicts, workflows }: the conflicting files ([] when it merges cleanly), and the files + * under .github/workflows/ that the merge changes on the PR's branch. */ -function gitConflicts({ owner, repo, number, base, dir }) { +function gitMergeCheck({ 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'); @@ -149,15 +171,24 @@ function gitConflicts({ owner, repo, number, base, dir }) { 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`); + let out; + let conflicts = []; try { - git('merge-tree', '--write-tree', '--name-only', '--no-messages', 'pr', 'base'); - return []; + out = git('merge-tree', '--write-tree', '--name-only', '--no-messages', 'pr', 'base'); } 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)]; + out = String(err.stdout || ''); + conflicts = [...new Set(out.split('\n').slice(1).map((l) => l.trim()).filter(Boolean))]; } + const tree = out.split('\n')[0].trim(); + if (!/^[0-9a-f]{40,64}$/.test(tree)) throw new Error('git merge-tree gave no tree'); + // Compares tree entries only (no rename detection), so no file contents are fetched. + const workflows = git('diff-tree', '-r', '--name-only', 'pr', tree, '--', WORKFLOWS_DIR) + .split('\n') + .map((l) => l.trim()) + .filter(Boolean); + return { conflicts, workflows }; } async function addLabel(github, { owner, repo, issue_number }) { @@ -280,12 +311,40 @@ async function approveHeldRuns(github, { owner, repo, oldSha, newSha, sleep, pol return 'running'; } +/** + * GitHub's answer when update-branch fails, for the run log and the job summary: the status, GitHub's message and + * the request id (which GitHub support can look up). Never the request itself, whose headers hold the token. + */ +function describeError(err) { + const response = (err && err.response) || {}; + const data = response.data || {}; + const message = String(data.message || (err && err.message) || 'no message').replace(/\s+/g, ' ').trim(); + const id = response.headers && response.headers['x-github-request-id']; + return `${(err && err.status) || 'no status'} ${message}${id ? ` (request ${id})` : ''}`; +} + +/** Writes what happened to the job summary, so nobody has to dig through the log. Never fails the run. */ +async function writeSummary(core, rows) { + if (!core.summary) return; + try { + await core.summary + .addHeading('/update', 3) + .addTable([ + [{ data: 'What', header: true }, { data: 'Result', header: true }], + ...rows.map(([k, v]) => [k, String(v)]), + ]) + .write(); + } catch (err) { + core.info(`Could not write the job summary: ${err.message}`); + } +} + /** * 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. + * `checkMerge`, `sleep` and `poll` are swapped out by the tests. */ -async function run({ github, context, core, findConflicts, sleep, poll = POLL, workDir }) { +async function run({ github, context, core, checkMerge, 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.'); @@ -314,24 +373,28 @@ async function run({ github, context, core, findConflicts, sleep, poll = POLL, w const behind = await behindBy(github, { owner, repo, pr }); let conflicts = null; + let workflows = null; if (behind > 0) { try { - conflicts = await (findConflicts || gitConflicts)({ + ({ conflicts, workflows } = await (checkMerge || gitMergeCheck)({ 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}`); + // Unknown, so update-branch is still tried: it may well work. + core.warning(`Could not check for conflicts or workflow changes: ${err.message}`); } } - let action = decide({ behind, conflicts, fork, canEdit }); + let action = decide({ behind, conflicts, workflows, fork, canEdit }); + const known = (list) => (list === null ? 'unknown' : list.length ? list.join(', ') : 'none'); core.info( `/update by ${commenter} on #${issue_number}: behind ${behind}, fork ${fork}, maintainer edits ${canEdit}, ` + - `conflicts ${conflicts === null ? 'unknown' : conflicts.length} -> ${action}`, + `conflicts ${known(conflicts)}, workflow changes ${known(workflows)} -> ${action}`, ); + let refused = null; let newSha = null; let ourMerge = false; @@ -342,9 +405,12 @@ async function run({ github, context, core, findConflicts, sleep, poll = POLL, w action = 'updated'; } catch (err) { const message = String(err.message || ''); - core.info(`update-branch answered ${err.status}: ${message}`); + refused = describeError(err); + core.warning(`update-branch was refused. GitHub answered ${refused}`); if (err.status === 422 && /expected head sha/i.test(message)) action = 'headMoved'; else if (err.status === 422 && /conflict/i.test(message)) action = 'conflicts'; + // A workflow file the up-front check couldn't see (it failed, or GitHub counts the change differently). + else if (fork && err.status === 403 && /workflow/i.test(message)) action = 'workflows'; // 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'; @@ -368,17 +434,30 @@ async function run({ github, context, core, findConflicts, sleep, poll = POLL, w // 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 }); + if (['conflicts', 'useButton', 'workflows', '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), + workflows: () => MESSAGES.workflows(base, workflows, Array.isArray(conflicts) && conflicts.length === 0), headMoved: () => MESSAGES.headMoved, failed: () => MESSAGES.failed(base), }[action](); core.info(`Result: ${action}${action === 'updated' ? `, checks ${checks}` : ''}`); + await writeSummary(core, [ + ['Pull request', `#${issue_number} from ${pr.head && pr.head.repo ? pr.head.repo.full_name : 'a deleted fork'}`], + ['Asked by', commenter], + ['Behind', `${behind} commit(s) on ${base}`], + ['Maintainer edits', fork ? String(canEdit) : 'not needed (same repository)'], + ['Conflicts', known(conflicts)], + ['Workflow files the merge changes', known(workflows)], + ['update-branch', refused ? `refused: ${refused}` : action === 'updated' ? 'accepted' : 'not tried'], + ['Result', action === 'updated' ? `updated, checks ${checks}` : action], + ]); return say(reply); } @@ -404,6 +483,7 @@ module.exports.parseCommand = parseCommand; module.exports.mayUpdate = mayUpdate; module.exports.isFork = isFork; module.exports.decide = decide; -module.exports.gitConflicts = gitConflicts; +module.exports.gitMergeCheck = gitMergeCheck; +module.exports.describeError = describeError; module.exports.MESSAGES = MESSAGES; module.exports.LABEL = LABEL; diff --git a/.github/scripts/update.test.js b/.github/scripts/update.test.js index 487b254d7..201a4538b 100644 --- a/.github/scripts/update.test.js +++ b/.github/scripts/update.test.js @@ -122,14 +122,35 @@ function context({ body = '/update', login = 'maya-rao', association = 'CONTRIBU }; } -function runWith(fake, ctx = context(), conflicts = []) { - const findConflicts = async () => { +// conflicts: the conflicting files, or an Error for a merge check that fails. workflows: workflow files it changes. +function runWith(fake, ctx = context(), conflicts = [], { workflows = [], log = core } = {}) { + const checkMerge = async () => { if (conflicts instanceof Error) throw conflicts; - return conflicts; + return { conflicts, workflows }; }; - return run({ github: fake.github, context: ctx, core, findConflicts, sleep: noSleep, poll }); + return run({ github: fake.github, context: ctx, core: log, checkMerge, sleep: noSleep, poll }); } +/** A core that records its log lines and the job summary's rows. */ +function recordingCore() { + const lines = []; + const rows = []; + const summary = { + addHeading: () => summary, + addTable: (t) => (rows.push(...t.slice(1)), summary), + write: async () => summary, + }; + return { + lines, + rows, + info: (m) => lines.push(['info', m]), + warning: (m) => lines.push(['warning', m]), + summary, + }; +} + +const FORK = { head: { sha: OLD, repo: { full_name: 'maya-rao/Sentient' } }, maintainer_can_modify: true }; + const reply = (fake) => fake.calls.filter((c) => c[0] === 'createComment').map((c) => c[1]); test('only an exact /update counts', () => { @@ -185,6 +206,13 @@ test('isFork and decide', () => { 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'); + // A workflow change stops the bot on a fork only; unknown (null) still tries. + const wf = ['.github/workflows/ci.yaml']; + assert.equal(decide({ behind: 2, conflicts: [], workflows: wf, fork: true, canEdit: true }), 'workflows'); + assert.equal(decide({ behind: 2, conflicts: [], workflows: wf, fork: false, canEdit: true }), 'updateBranch'); + assert.equal(decide({ behind: 2, conflicts: [], workflows: null, fork: true, canEdit: true }), 'updateBranch'); + assert.equal(decide({ behind: 2, conflicts: ['a'], workflows: wf, fork: true, canEdit: true }), 'conflicts'); + assert.equal(decide({ behind: 2, conflicts: [], workflows: wf, fork: true, canEdit: false }), 'useButton'); }); test('comments on issues, from bots, and other text are ignored', async () => { @@ -279,6 +307,74 @@ test('a fork GitHub refuses to push to falls back to the button', async () => { assert.doesNotMatch(reply(fake)[0], /doesn't allow edits/); }); +test('a fork whose merge changes a workflow file: says why, points at the button, labels it, no push tried', async () => { + const fake = fakeGithub({ pr: FORK }); + const log = recordingCore(); + await runWith(fake, context(), [], { workflows: ['.github/workflows/assign.yaml', '.github/workflows/ci.yaml'], log }); + assert.ok(!fake.names().includes('updateBranch')); + assert.deepEqual(fake.calls.find((c) => c[0] === 'addLabels')[1], ['needs update']); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.workflows('main', ['.github/workflows/assign.yaml', '.github/workflows/ci.yaml'], true)}`); + assert.match( + reply(fake)[0], + /it would change the repository's workflow files \(`\.github\/workflows\/assign\.yaml`, `\.github\/workflows\/ci\.yaml`\), and GitHub never lets a bot push those to a fork\. It has no conflicts with `main`, so click \*\*Update branch\*\*/, + ); + assert.deepEqual(log.rows.find((r) => r[0] === 'update-branch'), ['update-branch', 'not tried']); + assert.deepEqual(log.rows.find((r) => r[0] === 'Result'), ['Result', 'workflows']); +}); + +test('a same-repo branch whose merge changes a workflow file is still updated', async () => { + const fake = fakeGithub(); + await runWith(fake, context(), [], { workflows: ['.github/workflows/ci.yaml'] }); + assert.ok(fake.names().includes('updateBranch')); + assert.match(reply(fake)[0], /^@maya-rao Merged the latest `main`/); +}); + +test("GitHub's refusal is logged as a warning and in the summary, with the status, message and request id", async () => { + const err = httpError( + 403, + 'refusing to allow a GitHub App to create or update workflow `.github/workflows/ci.yaml` without `workflows` permission - https://docs.github.com/rest/pulls/pulls#update-a-pull-request-branch', + ); + err.response = { + data: { message: 'refusing to allow a GitHub App to create or update workflow `.github/workflows/ci.yaml` without `workflows` permission' }, + headers: { 'x-github-request-id': 'E450:16DDE7' }, + }; + err.request = { headers: { authorization: 'token ghs_secret' } }; + const fake = fakeGithub({ update: err, pr: FORK }); + const log = recordingCore(); + // The up-front check failed, so the bot tried, and maps GitHub's answer to the same plain reply. + await runWith(fake, context(), new Error('fetch failed'), { log }); + assert.ok(fake.names().includes('updateBranch')); + const want = '403 refusing to allow a GitHub App to create or update workflow `.github/workflows/ci.yaml` without `workflows` permission (request E450:16DDE7)'; + assert.deepEqual(log.lines.find((l) => /update-branch/.test(l[1])), ['warning', `update-branch was refused. GitHub answered ${want}`]); + assert.deepEqual(log.rows.find((r) => r[0] === 'update-branch'), ['update-branch', `refused: ${want}`]); + assert.ok(!JSON.stringify([log.lines, log.rows]).includes('ghs_secret')); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.workflows('main', null, false)}`); + assert.match(reply(fake)[0], /workflow files, and GitHub never lets a bot push those to a fork\. Click \*\*Update branch\*\*/); + assert.ok(fake.names().includes('addLabels')); +}); + +test('describeError copes with errors that have no response', () => { + assert.equal(run.describeError(httpError(500, 'boom')), '500 boom'); + assert.equal(run.describeError(new Error('socket hang up')), 'no status socket hang up'); + assert.equal(run.describeError(undefined), 'no status no message'); +}); + +test('a successful update is in the summary too, and a summary that fails to write does not fail the run', async () => { + let fake = fakeGithub(); + const log = recordingCore(); + await runWith(fake, context(), [], { log }); + assert.deepEqual(log.rows.find((r) => r[0] === 'update-branch'), ['update-branch', 'accepted']); + assert.deepEqual(log.rows.find((r) => r[0] === 'Result'), ['Result', 'updated, checks running']); + assert.deepEqual(log.rows.find((r) => r[0] === 'Maintainer edits'), ['Maintainer edits', 'not needed (same repository)']); + fake = fakeGithub(); + const broken = recordingCore(); + broken.summary.write = async () => { + throw new Error('GITHUB_STEP_SUMMARY is not set'); + }; + await runWith(fake, context(), [], { log: broken }); + assert.equal(reply(fake).length, 1); +}); + 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')); @@ -399,14 +495,16 @@ test('messages are plain: no em-dashes', () => { 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.workflows('main', ['.github/workflows/ci.yaml'], true), MESSAGES.workflows('main', null, 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) => { +// The real merge check against a throwaway local repository (no network): a file both sides changed is a conflict, +// a file only one side changed is not, a clean merge has none, and a workflow file is listed only when the merge +// changes it on the pull request's branch. +test('gitMergeCheck lists exactly the conflicting files and the workflow files the merge changes', (t) => { try { execFileSync('git', ['merge-tree', '-h'], { stdio: 'ignore' }); } catch (err) { @@ -429,9 +527,19 @@ test('gitConflicts lists exactly the conflicting files', (t) => { 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'); + fs.mkdirSync(path.join(src, '.github', 'workflows'), { recursive: true }); + write('.github/workflows/ci.yaml', 'name: CI\n'); + g('add', '.'); g('commit', '--quiet', '-m', 'main'); + // A branch that already has main's workflow file and changes something else. + g('switch', '--quiet', '-c', 'later'); + write('b.txt', 'later\n'); + g('commit', '--quiet', '-am', 'later'); + g('update-ref', 'refs/pull/9/head', 'later'); + g('switch', '--quiet', 'main'); + write('c.txt', 'main again\n'); + g('commit', '--quiet', '-am', 'main again'); - // gitConflicts fetches from https://github.com//.git; point that at the local repository. + // gitMergeCheck 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 }; @@ -446,10 +554,13 @@ test('gitConflicts lists exactly the conflicting files', (t) => { }); 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') })); + const check = (number) => + run.gitMergeCheck({ owner: 'owner', repo: 'repo', number, base: 'main', dir: path.join(root, 'check') }); + const wf = ['.github/workflows/ci.yaml']; + assert.deepEqual(check(7), { conflicts: ['a.txt'], workflows: wf }); + assert.deepEqual(check(8), { conflicts: [], workflows: wf }); + assert.deepEqual(check(9), { conflicts: [], workflows: [] }); + assert.throws(() => check('7; rm')); }); function pathToUrl(p) { diff --git a/.github/workflows/update.yaml b/.github/workflows/update.yaml index 48d54c2a3..a9af979dd 100644 --- a/.github/workflows/update.yaml +++ b/.github/workflows/update.yaml @@ -28,7 +28,8 @@ jobs: 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). + # the other two (see update.js). There is no `workflows` permission for this token, so GitHub refuses any merge + # that changes a fork's .github/workflows/ files; update.js spots that first and says so (#473). 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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b08e8436f..1abea9a45 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -86,9 +86,12 @@ 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 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. + a bot tries to merge it in (a merge, never a rebase) and tells you what happened. It can update a branch in + your fork when **Allow edits by maintainers** is ticked, except when `main` has changed a file in + `.github/workflows/` since your branch started: GitHub never lets a bot push workflow files to a fork. Then, + and whenever else it can't update your branch, it tells you why, shows you GitHub's **Update branch** button + and labels the pull request `needs update` so a maintainer can do it for you. 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 4c40a740b..a269cffda 100644 --- a/docs/AGENT_WORKFLOW.md +++ b/docs/AGENT_WORKFLOW.md @@ -152,3 +152,18 @@ only when "Allow edits by maintainers" is on; never force-push, and comment with 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. + +The `/update` bot runs on the built-in `GITHUB_TOKEN`. That token can merge `main` into a branch in this +repository, and into a fork's branch (anyone's fork) when "Allow edits by maintainers" is on, but GitHub refuses +it any change to a fork's `.github/workflows/` files, and the token can't be given the `workflows` permission it +would need. So when `main` changed a workflow file since a fork's branch started, the bot doesn't try: it says +why, points at the **Update branch** button and adds `needs update`. Without conflicts, a maintainer then +merges `main` in with their own credentials. These need the `workflow` scope: `gh auth status` lists the scopes, +and `gh auth refresh -s workflow` adds it. + +```bash +gh api -X PUT repos/existence-master/Sentient/pulls//update-branch -f expected_head_sha= +``` + +When GitHub refuses the bot for any reason, its status and message are in the run's log (as a warning) and in +the run's summary. From 5f7ebcc278636b0d5d4f4ab1f3e09b261e7b67dd Mon Sep 17 00:00:00 2001 From: itsskofficial Date: Mon, 26 Oct 2026 23:33:24 +0530 Subject: [PATCH 2/3] fix(ci): /update still tries a fork, since GitHub only refuses workflow commits the fork lacks --- .github/scripts/update.js | 40 ++++++++++--------- .github/scripts/update.test.js | 70 +++++++++++++++++----------------- .github/workflows/update.yaml | 4 +- CONTRIBUTING.md | 11 +++--- docs/AGENT_WORKFLOW.md | 13 ++++--- 5 files changed, 73 insertions(+), 65 deletions(-) diff --git a/.github/scripts/update.js b/.github/scripts/update.js index 800f3cec0..b4027e9af 100644 --- a/.github/scripts/update.js +++ b/.github/scripts/update.js @@ -5,11 +5,13 @@ // - 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, anyone's, when "Allow edits by maintainers" is on. -// - a fork, and the merge changes a workflow file -> doesn't try: GitHub refuses the workflow token any change -// under .github/workflows/ in a fork, whoever owns it ("refusing to allow a GitHub -// App to create or update workflow ... without `workflows` permission", #473), and -// GITHUB_TOKEN can't be given that permission. (A branch in this repository is fine.) -// Names the files, points at the "Update branch" button and adds the label. +// - GitHub refuses a fork because of workflow files -> says so plainly, names the files, points at the "Update +// branch" button and adds the label. GitHub refuses the workflow token a push that +// brings commits changing .github/workflows/ into a fork that doesn't have them yet +// ("refusing to allow a GitHub App to create or update workflow ... without +// `workflows` permission", #473), whoever owns the fork, and GITHUB_TOKEN can't be +// given that permission. A fork that already has them (synced) is updated fine, and +// so is a branch in this repository, so the bot still tries. // - 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 // Whenever update-branch fails, GitHub's status and message go into the run log (as a warning) and the job summary. @@ -38,7 +40,7 @@ const LABEL = { const WRITERS = ['OWNER', 'MEMBER', 'COLLABORATOR']; const CLA_CHECK = 'CLAAssistant'; const POLL = { tries: 12, ms: 5000 }; -// In a fork GitHub only lets a token with the `workflows` permission write here, and the workflow token can't have it. +// Bringing new changes here into a fork needs the `workflows` permission, which the workflow token can't have. const WORKFLOWS_DIR = '.github/workflows/'; const code = (s) => `\`${s}\``; @@ -77,9 +79,10 @@ const MESSAGES = { : "I can't push to this branch, because it lives in a fork that doesn't allow edits by maintainers. ") + buttonHint(base, clean), workflows: (base, files, clean) => - `I can't merge ${code(base)} into this branch myself: it would change the repository's workflow files` + + `GitHub didn't let me merge ${code(base)} into this branch: ${code(base)} has changes to the repository's ` + + 'workflow files' + (files && files.length ? ` (${files.map(code).join(', ')})` : '') + - ', and GitHub never lets a bot push those to a fork. ' + + " that your fork doesn't have yet, and GitHub never lets a bot bring those into a fork. " + buttonHint(base, clean), 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.', @@ -123,17 +126,14 @@ function isFork(pr, owner, repo) { /** * 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). - * workflows: files under .github/workflows/ the merge would change ([] for none, null if unknown). - * Returns "upToDate" | "conflicts" | "useButton" | "workflows" | "updateBranch". + * Returns "upToDate" | "conflicts" | "useButton" | "updateBranch". A change to a workflow file doesn't stop it: on + * a fork GitHub may refuse it, but only when the fork lacks those commits, which can't be told from here (#473). */ -function decide({ behind, conflicts, workflows, fork, canEdit }) { +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'; - // On a fork (anyone's, the owner's too) GitHub refuses the workflow token any change to a workflow file, so - // don't try. A branch in this repository is updated fine (#473). - if (fork && workflows && workflows.length) return 'workflows'; return 'updateBranch'; } @@ -388,7 +388,7 @@ async function run({ github, context, core, checkMerge, sleep, poll = POLL, work core.warning(`Could not check for conflicts or workflow changes: ${err.message}`); } } - let action = decide({ behind, conflicts, workflows, fork, canEdit }); + let action = decide({ behind, conflicts, fork, canEdit }); const known = (list) => (list === null ? 'unknown' : list.length ? list.join(', ') : 'none'); core.info( `/update by ${commenter} on #${issue_number}: behind ${behind}, fork ${fork}, maintainer edits ${canEdit}, ` + @@ -409,8 +409,14 @@ async function run({ github, context, core, checkMerge, sleep, poll = POLL, work core.warning(`update-branch was refused. GitHub answered ${refused}`); if (err.status === 422 && /expected head sha/i.test(message)) action = 'headMoved'; else if (err.status === 422 && /conflict/i.test(message)) action = 'conflicts'; - // A workflow file the up-front check couldn't see (it failed, or GitHub counts the change differently). - else if (fork && err.status === 403 && /workflow/i.test(message)) action = 'workflows'; + // The fork lacks main's commits that change a workflow file (see the top of this file). + else if (fork && err.status === 403 && /workflow/i.test(message)) { + action = 'workflows'; + if (!workflows || !workflows.length) { + const named = message.match(/workflow `([^`]+)`/); + workflows = named ? [named[1]] : null; + } + } // 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'; diff --git a/.github/scripts/update.test.js b/.github/scripts/update.test.js index 201a4538b..3bedd1cac 100644 --- a/.github/scripts/update.test.js +++ b/.github/scripts/update.test.js @@ -206,13 +206,6 @@ test('isFork and decide', () => { 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'); - // A workflow change stops the bot on a fork only; unknown (null) still tries. - const wf = ['.github/workflows/ci.yaml']; - assert.equal(decide({ behind: 2, conflicts: [], workflows: wf, fork: true, canEdit: true }), 'workflows'); - assert.equal(decide({ behind: 2, conflicts: [], workflows: wf, fork: false, canEdit: true }), 'updateBranch'); - assert.equal(decide({ behind: 2, conflicts: [], workflows: null, fork: true, canEdit: true }), 'updateBranch'); - assert.equal(decide({ behind: 2, conflicts: ['a'], workflows: wf, fork: true, canEdit: true }), 'conflicts'); - assert.equal(decide({ behind: 2, conflicts: [], workflows: wf, fork: true, canEdit: false }), 'useButton'); }); test('comments on issues, from bots, and other text are ignored', async () => { @@ -307,52 +300,59 @@ test('a fork GitHub refuses to push to falls back to the button', async () => { assert.doesNotMatch(reply(fake)[0], /doesn't allow edits/); }); -test('a fork whose merge changes a workflow file: says why, points at the button, labels it, no push tried', async () => { +// What GitHub answered on #499 and #521 (#473), as Octokit's RequestError carries it. +function workflowRefusal(file) { + const msg = `refusing to allow a GitHub App to create or update workflow \`${file}\` without \`workflows\` permission`; + const err = httpError(403, `${msg} - https://docs.github.com/rest/pulls/pulls#update-a-pull-request-branch`); + err.response = { data: { message: msg }, headers: { 'x-github-request-id': 'E450:16DDE7' } }; + err.request = { headers: { authorization: 'token ghs_secret' } }; + return err; +} + +test('a fork whose merge changes a workflow file is still tried, because a synced fork is updated fine', async () => { const fake = fakeGithub({ pr: FORK }); + await runWith(fake, context(), [], { workflows: ['.github/workflows/ci.yaml'] }); + assert.ok(fake.names().includes('updateBranch')); + assert.match(reply(fake)[0], /^@maya-rao Merged the latest `main`/); +}); + +test('GitHub refusing a fork over workflow files: says why plainly, names the files, button and label', async () => { + const fake = fakeGithub({ update: workflowRefusal('.github/workflows/ci.yaml'), pr: FORK }); const log = recordingCore(); - await runWith(fake, context(), [], { workflows: ['.github/workflows/assign.yaml', '.github/workflows/ci.yaml'], log }); - assert.ok(!fake.names().includes('updateBranch')); + const files = ['.github/workflows/assign.yaml', '.github/workflows/ci.yaml']; + await runWith(fake, context(), [], { workflows: files, log }); assert.deepEqual(fake.calls.find((c) => c[0] === 'addLabels')[1], ['needs update']); - assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.workflows('main', ['.github/workflows/assign.yaml', '.github/workflows/ci.yaml'], true)}`); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.workflows('main', files, true)}`); assert.match( reply(fake)[0], - /it would change the repository's workflow files \(`\.github\/workflows\/assign\.yaml`, `\.github\/workflows\/ci\.yaml`\), and GitHub never lets a bot push those to a fork\. It has no conflicts with `main`, so click \*\*Update branch\*\*/, + /^@maya-rao GitHub didn't let me merge `main` into this branch: `main` has changes to the repository's workflow files \(`\.github\/workflows\/assign\.yaml`, `\.github\/workflows\/ci\.yaml`\) that your fork doesn't have yet, and GitHub never lets a bot bring those into a fork\. It has no conflicts with `main`, so click \*\*Update branch\*\*/, ); - assert.deepEqual(log.rows.find((r) => r[0] === 'update-branch'), ['update-branch', 'not tried']); + assert.doesNotMatch(reply(fake)[0], /refused the update for this fork/); assert.deepEqual(log.rows.find((r) => r[0] === 'Result'), ['Result', 'workflows']); }); -test('a same-repo branch whose merge changes a workflow file is still updated', async () => { - const fake = fakeGithub(); - await runWith(fake, context(), [], { workflows: ['.github/workflows/ci.yaml'] }); - assert.ok(fake.names().includes('updateBranch')); - assert.match(reply(fake)[0], /^@maya-rao Merged the latest `main`/); -}); - test("GitHub's refusal is logged as a warning and in the summary, with the status, message and request id", async () => { - const err = httpError( - 403, - 'refusing to allow a GitHub App to create or update workflow `.github/workflows/ci.yaml` without `workflows` permission - https://docs.github.com/rest/pulls/pulls#update-a-pull-request-branch', - ); - err.response = { - data: { message: 'refusing to allow a GitHub App to create or update workflow `.github/workflows/ci.yaml` without `workflows` permission' }, - headers: { 'x-github-request-id': 'E450:16DDE7' }, - }; - err.request = { headers: { authorization: 'token ghs_secret' } }; - const fake = fakeGithub({ update: err, pr: FORK }); + const fake = fakeGithub({ update: workflowRefusal('.github/workflows/ci.yaml'), pr: FORK }); const log = recordingCore(); - // The up-front check failed, so the bot tried, and maps GitHub's answer to the same plain reply. + // With the merge check failed, the file comes from GitHub's own message. await runWith(fake, context(), new Error('fetch failed'), { log }); - assert.ok(fake.names().includes('updateBranch')); - const want = '403 refusing to allow a GitHub App to create or update workflow `.github/workflows/ci.yaml` without `workflows` permission (request E450:16DDE7)'; + const want = + '403 refusing to allow a GitHub App to create or update workflow `.github/workflows/ci.yaml` without ' + + '`workflows` permission (request E450:16DDE7)'; assert.deepEqual(log.lines.find((l) => /update-branch/.test(l[1])), ['warning', `update-branch was refused. GitHub answered ${want}`]); assert.deepEqual(log.rows.find((r) => r[0] === 'update-branch'), ['update-branch', `refused: ${want}`]); assert.ok(!JSON.stringify([log.lines, log.rows]).includes('ghs_secret')); - assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.workflows('main', null, false)}`); - assert.match(reply(fake)[0], /workflow files, and GitHub never lets a bot push those to a fork\. Click \*\*Update branch\*\*/); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.workflows('main', ['.github/workflows/ci.yaml'], false)}`); + assert.match(reply(fake)[0], /\(`\.github\/workflows\/ci\.yaml`\) that your fork doesn't have yet.*Click \*\*Update branch\*\*/); assert.ok(fake.names().includes('addLabels')); }); +test('a workflow refusal on a same-repo branch is not blamed on a fork', async () => { + const fake = fakeGithub({ update: workflowRefusal('.github/workflows/ci.yaml') }); + await runWith(fake); + assert.equal(reply(fake)[0], `@maya-rao ${MESSAGES.failed('main')}`); +}); + test('describeError copes with errors that have no response', () => { assert.equal(run.describeError(httpError(500, 'boom')), '500 boom'); assert.equal(run.describeError(new Error('socket hang up')), 'no status socket hang up'); diff --git a/.github/workflows/update.yaml b/.github/workflows/update.yaml index a9af979dd..93c8da0aa 100644 --- a/.github/workflows/update.yaml +++ b/.github/workflows/update.yaml @@ -28,8 +28,8 @@ jobs: 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). There is no `workflows` permission for this token, so GitHub refuses any merge - # that changes a fork's .github/workflows/ files; update.js spots that first and says so (#473). + # the other two (see update.js). There is no `workflows` permission for this token, so GitHub refuses a merge + # that brings new .github/workflows/ changes into a fork; update.js then says so plainly (#473). 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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1abea9a45..0fcbd7595 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -87,11 +87,12 @@ You bring the change; we take care of the busywork around it, so a pull request - **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. It can update a branch in - your fork when **Allow edits by maintainers** is ticked, except when `main` has changed a file in - `.github/workflows/` since your branch started: GitHub never lets a bot push workflow files to a fork. Then, - and whenever else it can't update your branch, it tells you why, shows you GitHub's **Update branch** button - and labels the pull request `needs update` so a maintainer can do it for you. If there are conflicts, it - lists the files and labels the pull request `needs update`, and a maintainer resolves them for you. + your fork when **Allow edits by maintainers** is ticked, except when `main` has changes to files in + `.github/workflows/` that your fork doesn't have yet: GitHub never lets a bot bring those into a fork. + (Clicking **Sync fork** on your fork's page brings them in, and then `/update` works.) Then, and whenever else + it can't update your branch, it tells you why, shows you GitHub's **Update branch** button and labels the pull + request `needs update` so a maintainer can do it for you. 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 a269cffda..6d21f975d 100644 --- a/docs/AGENT_WORKFLOW.md +++ b/docs/AGENT_WORKFLOW.md @@ -154,12 +154,13 @@ that changes behaviour or design goes back to them as a review. Pull requests la takes the label off once the branch is up to date. The `/update` bot runs on the built-in `GITHUB_TOKEN`. That token can merge `main` into a branch in this -repository, and into a fork's branch (anyone's fork) when "Allow edits by maintainers" is on, but GitHub refuses -it any change to a fork's `.github/workflows/` files, and the token can't be given the `workflows` permission it -would need. So when `main` changed a workflow file since a fork's branch started, the bot doesn't try: it says -why, points at the **Update branch** button and adds `needs update`. Without conflicts, a maintainer then -merges `main` in with their own credentials. These need the `workflow` scope: `gh auth status` lists the scopes, -and `gh auth refresh -s workflow` adds it. +repository, and into a fork's branch (anyone's fork, the owner's too) when "Allow edits by maintainers" is on. +GitHub refuses it one thing: a merge that brings commits changing `.github/workflows/` into a fork that doesn't +have those commits yet. The token can't be given the `workflows` permission that would allow it, and we don't add +an app or a secret for it. The bot still tries, because a synced fork works; when GitHub refuses, it says why, +points at the **Update branch** button and adds `needs update`. Without conflicts, a maintainer then merges `main` +in with their own credentials. These need the `workflow` scope: `gh auth status` lists the scopes, and +`gh auth refresh -s workflow` adds it. ```bash gh api -X PUT repos/existence-master/Sentient/pulls//update-branch -f expected_head_sha= From 5072eb239e827b7d55e427abb5f40c1e7769e0fa Mon Sep 17 00:00:00 2001 From: itsskofficial Date: Mon, 26 Oct 2026 23:51:30 +0530 Subject: [PATCH 3/3] docs: say what /update replies in each case, and give a runnable maintainer command --- CONTRIBUTING.md | 10 ++++++---- docs/AGENT_WORKFLOW.md | 7 +++++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0fcbd7595..99c7faae0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -89,10 +89,12 @@ You bring the change; we take care of the busywork around it, so a pull request a bot tries to merge it in (a merge, never a rebase) and tells you what happened. It can update a branch in your fork when **Allow edits by maintainers** is ticked, except when `main` has changes to files in `.github/workflows/` that your fork doesn't have yet: GitHub never lets a bot bring those into a fork. - (Clicking **Sync fork** on your fork's page brings them in, and then `/update` works.) Then, and whenever else - it can't update your branch, it tells you why, shows you GitHub's **Update branch** button and labels the pull - request `needs update` so a maintainer can do it for you. If there are conflicts, it lists the files and - labels the pull request `needs update`, and a maintainer resolves them for you. + (Clicking **Sync fork** on your fork's page brings them in, and then `/update` works.) In that case, and when + your fork doesn't allow edits by maintainers, it tells you why, shows you GitHub's **Update branch** button + and labels the pull request `needs update` so a maintainer can do it for you. If there are conflicts, it lists + the files and labels the pull request `needs update`, and a maintainer resolves them for you. If the branch + changed while it was working, it leaves it alone and you can comment `/update` again. If anything else goes + wrong, it says so and labels the pull request `needs update` for a maintainer. ### PR titles diff --git a/docs/AGENT_WORKFLOW.md b/docs/AGENT_WORKFLOW.md index 6d21f975d..617fdf0fb 100644 --- a/docs/AGENT_WORKFLOW.md +++ b/docs/AGENT_WORKFLOW.md @@ -160,10 +160,13 @@ have those commits yet. The token can't be given the `workflows` permission that an app or a secret for it. The bot still tries, because a synced fork works; when GitHub refuses, it says why, points at the **Update branch** button and adds `needs update`. Without conflicts, a maintainer then merges `main` in with their own credentials. These need the `workflow` scope: `gh auth status` lists the scopes, and -`gh auth refresh -s workflow` adds it. +`gh auth refresh -s workflow` adds it. Set `n` to the pull request's number; the head sha makes GitHub refuse the +merge if someone pushed meanwhile: ```bash -gh api -X PUT repos/existence-master/Sentient/pulls//update-branch -f expected_head_sha= +n=123 +sha=$(gh pr view "$n" -R existence-master/Sentient --json headRefOid --jq .headRefOid) +gh api -X PUT "repos/existence-master/Sentient/pulls/$n/update-branch" -f expected_head_sha="$sha" ``` When GitHub refuses the bot for any reason, its status and message are in the run's log (as a warning) and in