diff --git a/.github/workflows/keep-pr-current.yml b/.github/workflows/keep-pr-current.yml new file mode 100644 index 0000000000..a859615440 --- /dev/null +++ b/.github/workflows/keep-pr-current.yml @@ -0,0 +1,179 @@ +name: Keep Pull Request Current + +# Replaces the Mergify `update` rule that used to merge `develop` into open +# pull requests (see `.github/mergify.yml` and #3574). +# +# Why this is a workflow and not a Mergify rule: Mergify reports a `failure` +# check whenever it cannot perform the update, with no option to change that. +# The schema accepts only `bot_account` for the action +# (https://docs.mergify.com/mergify-configuration-schema.json, `$defs.UpdateActionModel`, +# `additionalProperties: false`), so the red check could only be avoided by +# never attempting an update. That outcome is not gated — the attempt itself is +# what produced the failures. This workflow performs the same merge through +# GitHub's `update-branch` API and reports a conflict as a comment, so the +# check is green whether or not the merge was possible. +# +# The up-to-date requirement is unchanged: `develop-branch-ruleset` still sets +# `strict_required_status_checks_policy`, so a branch that is behind `develop` +# and cannot be updated is still blocked from merging. Nothing here relaxes the +# ruleset, and this workflow is deliberately not a required check. +on: + push: + branches: + - develop + pull_request: + types: + - opened + - reopened + - ready_for_review + workflow_dispatch: + +permissions: + contents: write + pull-requests: write + +# One run per repository state. Queued behind any run already in flight so a +# burst of pushes to `develop` cannot have two runs merging the same branch. +concurrency: + group: keep-pr-current + cancel-in-progress: false + +jobs: + keep-current: + name: Keep pull request current + runs-on: ubuntu-latest + timeout-minutes: 10 + + steps: + # Sparse checkout: the decision logic in scripts/ is all this needs, and + # no pull request code is ever checked out. + - name: Checkout repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + fetch-depth: 1 + sparse-checkout: | + scripts/automation + + - name: Set up Node.js + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version-file: '.nvmrc' + + - name: Merge develop into open pull requests + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + // Relative specifier, matching the other workflows here + // (ai-feedback-validation.yml, orchestrate-phase-progression.yml). + const { + CONFLICT_COMMENT_MARKER, + buildConflictComment, + classifyUpdateResult, + normaliseCommentBody, + shouldAttemptUpdate, + } = require('./scripts/automation/keep-pr-current.cjs'); + + const { owner, repo } = context.repo; + const log = []; + + // A pull_request event carries its own number; a push to develop has + // to enumerate what is open against it. + let numbers = []; + if (context.eventName === 'pull_request') { + numbers = [context.payload.pull_request.number]; + } else { + const open = await github.paginate(github.rest.pulls.list, { + owner, + repo, + state: 'open', + base: 'develop', + per_page: 100, + }); + numbers = open.map((item) => item.number); + } + + core.info(`Considering ${numbers.length} pull request(s) against develop.`); + + for (const number of numbers) { + let pullRequest; + try { + ({ data: pullRequest } = await github.rest.pulls.get({ + owner, + repo, + pull_number: number, + })); + } catch (error) { + // A pull request merged or deleted between listing and reading. + core.info(`#${number}: skipped, could not read (${error.status}).`); + continue; + } + + const decision = shouldAttemptUpdate(pullRequest); + if (!decision.attempt) { + core.info(`#${number}: skipped (${decision.reason}).`); + continue; + } + + let status; + let message = ''; + try { + // expected_head_sha guards against merging into a branch that + // moved while this run was in flight. + const { status: responseStatus } = await github.rest.pulls.updateBranch({ + owner, + repo, + pull_number: number, + expected_head_sha: pullRequest.head.sha, + }); + status = responseStatus; + } catch (error) { + status = error.status; + // The API's own message is what distinguishes the two 422 + // responses, so prefer it over Octokit's wrapper text. + message = error.response?.data?.message || error.message || ''; + } + + const outcome = classifyUpdateResult({ status, message }); + core.info(`#${number}: ${outcome} (HTTP ${status}${message ? `, ${message}` : ''}).`); + + if (outcome === 'conflict') { + log.push(`#${number} HTTP ${status}${message ? `: ${message}` : ''}`); + + const body = buildConflictComment({ + pullNumber: number, + mergeableState: pullRequest.mergeable_state || 'unknown', + log, + }); + + // Upsert rather than append, so a pull request that stays + // conflicting across several pushes to develop carries one + // comment instead of one per run. + const { data: comments } = await github.rest.issues.listComments({ + owner, + repo, + issue_number: number, + per_page: 100, + }); + const existing = comments.find((comment) => + comment.user.type === 'Bot' && + comment.body.includes(CONFLICT_COMMENT_MARKER) + ); + + if (existing && normaliseCommentBody(existing.body) === normaliseCommentBody(body)) { + core.info(`#${number}: conflict comment already up to date.`); + continue; + } + if (existing) { + await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }); + } else { + await github.rest.issues.createComment({ owner, repo, issue_number: number, body }); + } + } + + // Nothing below this point reports a failure. A conflict is + // written as a comment and every other outcome is logged, so no + // outcome of this workflow turns a check red. See #3574. + } + + core.info(`Processed ${numbers.length} pull request(s).`); diff --git a/scripts/automation/__tests__/keep-pr-current.test.js b/scripts/automation/__tests__/keep-pr-current.test.js new file mode 100644 index 0000000000..e151041ba1 --- /dev/null +++ b/scripts/automation/__tests__/keep-pr-current.test.js @@ -0,0 +1,262 @@ +const fs = require('node:fs'); +const path = require('node:path'); + +const { + CONFLICT_COMMENT_MARKER, + buildConflictComment, + classifyUpdateResult, + normaliseCommentBody, + shouldAttemptUpdate, +} = require('../keep-pr-current.cjs'); + +const repositoryRoot = path.resolve(__dirname, '../../..'); +const workflowPath = path.join(repositoryRoot, '.github/workflows/keep-pr-current.yml'); +const mergifyConfigPath = path.join(repositoryRoot, '.github/mergify.yml'); + +/** + * A pull request as `pulls.get` returns it. Overrides are merged one level deep + * so a test can vary the head repository without restating the whole object. + */ +function pullRequest(overrides = {}) { + const base = { + state: 'open', + draft: false, + mergeable_state: 'behind', + head: { sha: 'headsha123', repo: { owner: { login: 'lightspeedwp' }, archived: false } }, + base: { ref: 'develop', repo: { owner: { login: 'lightspeedwp' } } }, + }; + + const merged = { ...base, ...overrides }; + if (overrides.head) { + merged.head = { ...base.head, ...overrides.head }; + } + if (overrides.base) { + merged.base = { ...base.base, ...overrides.base }; + } + return merged; +} + +describe('shouldAttemptUpdate', () => { + test('attempts the update for a same-repository pull request on develop', () => { + expect(shouldAttemptUpdate(pullRequest())).toEqual({ attempt: true, reason: 'behind' }); + }); + + // The exclusions the removed Mergify rule expressed as -draft and + // -from-fork. Both are behaviour a reader of this file would otherwise have + // to take on trust from the old rule. + test('skips drafts', () => { + expect(shouldAttemptUpdate(pullRequest({ draft: true })).reason).toBe('draft'); + }); + + test('skips pull requests from a fork', () => { + const forked = pullRequest({ + head: { repo: { owner: { login: 'anupamme' }, archived: false } }, + }); + + expect(shouldAttemptUpdate(forked).reason).toBe('from-fork'); + }); + + test('skips a merged pull request, which is what produced "head ref does not exist"', () => { + expect(shouldAttemptUpdate(pullRequest({ state: 'closed' })).reason).toBe('not-open'); + }); + + test('skips archived head repositories', () => { + const archived = pullRequest({ + head: { repo: { owner: { login: 'lightspeedwp' }, archived: true } }, + }); + + expect(shouldAttemptUpdate(archived).reason).toBe('archived-head'); + }); + + test('skips pull requests against another base branch', () => { + expect(shouldAttemptUpdate(pullRequest({ base: { ref: 'main' } })).reason).toBe('not-develop'); + }); + + // The regression this guards. After the base branch moves, GitHub reports + // `unknown` until mergeability is recomputed. The update API is the real + // test, so that window must not stop the attempt: skipping on `unknown` + // would strand every open pull request on every push to develop. + test('still attempts the update while mergeability is unknown', () => { + expect(shouldAttemptUpdate(pullRequest({ mergeable_state: 'unknown' })).attempt).toBe(true); + }); + + test('still attempts the update when the pull request is already known to conflict', () => { + // Reported as a comment, not suppressed: the outcome is information, and + // the API call is what turns it into a fact rather than a stale state. + expect(shouldAttemptUpdate(pullRequest({ mergeable_state: 'dirty' })).attempt).toBe(true); + }); + + test('tolerates a pull request with no repository information', () => { + expect(shouldAttemptUpdate({ state: 'open', draft: false, head: {}, base: {} }).attempt).toBe( + true + ); + }); +}); + +describe('classifyUpdateResult', () => { + test('202 is the base branch being merged in', () => { + expect(classifyUpdateResult({ status: 202 })).toBe('updated'); + }); + + // Both messages below were observed against this repository on 2026-09-30, + // from `PUT /pulls/{n}/update-branch`. GitHub sends both as 422, so the + // message is what separates them. + test('422 with a merge-conflict message is a conflict', () => { + expect( + classifyUpdateResult({ status: 422, message: 'merge conflict between base and head' }) + ).toBe('conflict'); + }); + + // The regression this guards. Most open pull requests are not behind develop, + // so this is the response the workflow sees most often. Classifying it as a + // conflict would post a false conflict comment on every open pull request + // every time develop is pushed to. + test('422 saying there are no new commits means already current', () => { + expect( + classifyUpdateResult({ status: 422, message: 'There are no new commits on the base branch.' }) + ).toBe('current'); + }); + + test('422 from a concurrent push to the head is a benign retry', () => { + expect( + classifyUpdateResult({ + status: 422, + message: 'Head branch was modified. Review and try the merge again.', + }) + ).toBe('retry'); + }); + + test('409 is treated as a conflict', () => { + expect(classifyUpdateResult({ status: 409 })).toBe('conflict'); + }); + + test('an unrecognised 422 is still surfaced rather than swallowed', () => { + expect(classifyUpdateResult({ status: 422, message: 'something new' })).toBe('conflict'); + }); + + test('404 is the head branch having been deleted, not an error', () => { + expect(classifyUpdateResult({ status: 404 })).toBe('gone'); + }); + + test('403 is treated as gone rather than as an error to report', () => { + expect(classifyUpdateResult({ status: 403 })).toBe('gone'); + }); + + test('anything else is an error', () => { + expect(classifyUpdateResult({ status: 500, message: 'boom' })).toBe('error'); + }); + + test('no status at all is an error, not a silent success', () => { + expect(classifyUpdateResult({})).toBe('error'); + }); +}); + +describe('buildConflictComment', () => { + const body = buildConflictComment({ pullNumber: 3524, mergeableState: 'dirty' }); + + test('carries the marker the workflow upserts on', () => { + expect(body).toContain(CONFLICT_COMMENT_MARKER); + }); + + test('names the pull request state that caused it', () => { + expect(body).toContain('dirty'); + expect(body).toContain('3524'); + }); + + test('explains that this is not a failing check', () => { + expect(body).toMatch(/not a failure of any check/i); + }); + + test('does not leave conflict markers in its own output', () => { + expect(body).not.toMatch(/^<{7}/m); + }); +}); + +describe('normaliseCommentBody', () => { + // URLs carry the pull request number, so a comment whose only difference is + // its URL would otherwise be rewritten on every run. Comments for different + // pull requests still differ in the number in the prose. + test('ignores URL-only differences so repeated runs do not repost', () => { + const first = buildConflictComment({ pullNumber: 3524, mergeableState: 'dirty' }); + const second = `${first}\nhttps://github.com/lightspeedwp/.github/pull/3524/conflicts\n`; + + expect(normaliseCommentBody(first)).toBe(normaliseCommentBody(second)); + }); + + test('still distinguishes comments for different pull requests', () => { + const first = normaliseCommentBody( + buildConflictComment({ pullNumber: 3524, mergeableState: 'dirty' }) + ); + const second = normaliseCommentBody( + buildConflictComment({ pullNumber: 3532, mergeableState: 'dirty' }) + ); + + expect(first).not.toBe(second); + }); +}); + +describe('the removed Mergify rule', () => { + test('is no longer configured', () => { + const config = fs.readFileSync(mergifyConfigPath, 'utf8'); + + expect(config).not.toContain('Keep same-repository pull requests on develop current'); + expect(config).not.toMatch(/^\s*update:\s*\{\}/m); + }); + + test('leaves Dependabot auto-merge in place', () => { + const config = fs.readFileSync(mergifyConfigPath, 'utf8'); + + expect(config).toContain('merge_protections_settings'); + expect(config).toContain('author~=^(dependabot\\[bot\\]|app/dependabot)$'); + }); + + // Without this the workflow could silently stop being wired to anything and + // branches would quietly stop being kept current. + test('is replaced by a workflow on the same base branch', () => { + const config = fs.readFileSync(mergifyConfigPath, 'utf8'); + + expect(config).toContain('.github/workflows/keep-pr-current.yml'); + expect(fs.existsSync(workflowPath)).toBe(true); + }); +}); + +describe('keep-pr-current workflow', () => { + const workflow = fs.readFileSync(workflowPath, 'utf8'); + + // The defect being fixed. A workflow that called setFailed on a conflict + // would reproduce the red check under a different name. + test('never fails the job on a conflict', () => { + expect(workflow).not.toMatch(/core\.setFailed/); + expect(workflow).not.toMatch(/process\.exit\(1\)/); + expect(workflow).not.toMatch(/exit 1/); + }); + + test('runs when develop moves, which is when branches fall behind', () => { + expect(workflow).toMatch(/^\s*push:\s*$/m); + expect(workflow).toMatch(/^\s*- develop\s*$/m); + }); + + test('has the write permissions the update API needs', () => { + expect(workflow).toMatch(/^permissions:\s*$/m); + expect(workflow).toMatch(/^\s{2}contents:\s*write\s*$/m); + expect(workflow).toMatch(/^\s{2}pull-requests:\s*write\s*$/m); + }); + + test('guards the update with the head SHA it read', () => { + expect(workflow).toContain('expected_head_sha: pullRequest.head.sha'); + }); + + test('never checks out pull request code', () => { + expect(workflow).toContain('persist-credentials: false'); + expect(workflow).toMatch(/uses:\s*actions\/github-script@/); + }); + + test('pins every action to a commit', () => { + const uses = workflow.match(/uses:\s*\S+/g) || []; + + expect(uses.length).toBeGreaterThan(0); + for (const line of uses) { + expect(line).toMatch(/@[0-9a-f]{40}(\s|$)/); + } + }); +}); diff --git a/scripts/automation/keep-pr-current.cjs b/scripts/automation/keep-pr-current.cjs new file mode 100644 index 0000000000..a5fd13bd96 --- /dev/null +++ b/scripts/automation/keep-pr-current.cjs @@ -0,0 +1,210 @@ +/** + * Decide whether to merge `develop` into a pull request branch, and how to + * report the outcome. + * + * This replaces the Mergify `update` rule that used to own this job + * (`.github/mergify.yml`, removed in the pull request that added this file). + * The behaviour is deliberately identical — merge the base in, skip drafts and + * forks, act on any staleness — but the *reporting* is different, and that is + * the whole point of the change. + * + * Mergify's `update` action has no option that affects how a failure is + * reported. Its schema accepts only `bot_account` + * (https://docs.mergify.com/mergify-configuration-schema.json, `$defs.UpdateActionModel`, + * `additionalProperties: false`), and when Mergify cannot perform the update it + * reports the rule's check run with conclusion `failure`. That happens both + * when the update genuinely conflicts and when Mergify declines for its own + * reasons, so a red `Base branch update has failed` is left behind on a pull + * request that nobody can act on. See #3574. + * + * Everything here is a pure function of its arguments: the workflow performs + * the API calls and passes in what it read, so the decision can be tested + * without a network round trip. + */ + +/** + * Marker used to find and update this workflow's own comment, so repeated runs + * on the same pull request do not stack up duplicates. Deliberately distinct + * from the AI Feedback Validation marker and from the PR template routing + * comment's `## PR Template Routing` heading. + */ +const CONFLICT_COMMENT_MARKER = ''; + +/** + * Should the base branch be merged into this pull request's head branch? + * + * The exclusions mirror what the Mergify rule used to express, and Mergify's + * own documented preconditions (https://docs.mergify.com/workflow/actions/update: + * "A pull request is updated only when it is open, has no conflict, is not in a + * merge queue, and is behind its base branch"). + * + * `mergeableState` is advisory only and never blocks the attempt. GitHub + * reports `unknown` for a short window after the base branch moves, and + * treating that window as "conflicting" would strand every open pull request + * on every push to `develop`. The update API is the real test: it either + * merges, or it reports a conflict, and the conflict path is reported as + * information rather than as a failure. + * + * @param {object} pullRequest - A `pulls.get` response. + * @returns {{attempt: boolean, reason: string}} + */ +function shouldAttemptUpdate(pullRequest) { + if (!pullRequest || pullRequest.state !== 'open') { + return { attempt: false, reason: 'not-open' }; + } + + if (pullRequest.draft) { + return { attempt: false, reason: 'draft' }; + } + + const headOwner = pullRequest.head?.repo?.owner?.login; + const baseOwner = pullRequest.base?.repo?.owner?.login; + if (headOwner && baseOwner && headOwner !== baseOwner) { + return { attempt: false, reason: 'from-fork' }; + } + + if (pullRequest.head?.repo?.archived) { + return { attempt: false, reason: 'archived-head' }; + } + + if (pullRequest.base?.ref && pullRequest.base.ref !== 'develop') { + return { attempt: false, reason: 'not-develop' }; + } + + return { attempt: true, reason: 'behind' }; +} + +/** + * Classify the result of `PUT /repos/{owner}/{repo}/pulls/{pull_number}/update-branch`. + * + * Status alone is not enough to act on. GitHub answers **422** for two very + * different situations, both observed against this repository on 2026-09-30: + * + * - `{"message": "merge conflict between base and head", "status": "422"}` + * - `{"message": "There are no new commits on the base branch.", "status": "422"}` + * + * The second is the common case on a push to `develop`, because most open pull + * requests are not behind. Reading 422 as "conflict" would post a false + * conflict comment on every pull request in the repository, so the message is + * part of the classification. + * + * Every outcome other than a genuine infrastructure error is a success from + * this workflow's point of view. A conflict is a fact about the repository, not + * a failure of the automation, and the caller must never turn one into a red + * check — that is the defect being fixed. + * + * @param {{status?: number, message?: string}} outcome + * @returns {'updated'|'current'|'conflict'|'retry'|'gone'|'error'} + */ +function classifyUpdateResult({ status, message = '' } = {}) { + const text = String(message).toLowerCase(); + + // 202 Accepted — the base branch was merged into the head branch. + if (status === 202) { + return 'updated'; + } + + // Already current. Not a conflict, and not worth a comment. Checked before + // the status because it arrives as 422, same as a real conflict. + if (text.includes('no new commits on the base branch')) { + return 'current'; + } + + // A real conflict. GitHub sent this as 422 with exactly this message; 409 is + // matched too because it is the status the same condition is documented under. + if (status === 409 || text.includes('merge conflict between base and head')) { + return 'conflict'; + } + + // A push to the head branch between reading it and calling the API. Benign: + // the next run reads the new head and tries again. + if (text.includes('head branch was modified')) { + return 'retry'; + } + + // 404 Not Found. In practice this is the head branch having been deleted + // after the pull request merged, which GitHub does automatically here + // (`delete_branch_on_merge` is on). Mergify reported the equivalent as + // "head ref does not exist" — a failure on a pull request that had already + // been merged, and that no one could act on. + if (status === 404) { + return 'gone'; + } + + // 403 Forbidden — the token cannot write this head branch, e.g. a fork whose + // author has not enabled maintainer edits. Same reasoning as 404: the merge + // is not going to happen, and that is not a failure to report. + if (status === 403) { + return 'gone'; + } + + // Any other 422 is treated as a conflict to report rather than swallowed, so + // an unfamiliar rejection is still visible to a human. + if (status === 422) { + return 'conflict'; + } + + return 'error'; +} + +/** + * Build the comment posted when a pull request cannot be updated automatically. + * + * @param {object} params + * @param {number} params.pullNumber + * @param {string} params.mergeableState - `mergeable_state` from the pull request. + * @param {string[]} [params.log] - Per-pull-request log lines already emitted. + * @returns {string} Markdown comment body. + */ +function buildConflictComment({ pullNumber, mergeableState = 'unknown', log = [] }) { + const conflictUrl = `https://github.com/lightspeedwp/.github/pull/${pullNumber}/conflicts`; + + return [ + CONFLICT_COMMENT_MARKER, + `## Pull request #${pullNumber} could not be updated automatically`, + '', + `\`develop\` cannot be merged into this branch automatically because the two conflict`, + `(\`mergeable_state: ${mergeableState}\`). Merging cannot resolve a conflict, so this needs`, + 'a human. This is a property of the branch, not a failure of any check — no check is', + 'reporting red because of it, and the up-to-date requirement on `develop` is unchanged.', + '', + '**To resolve it:**', + '', + `1. Merge \`develop\` into this branch locally (\`git merge origin/develop\`) and push the`, + ' result, or rebase the branch onto `develop`.', + '2. Open the **Resolve conflicts** view to jump straight to the conflicting files:', + ` ${conflictUrl}`, + '', + 'Common conflict sources in this repository are the per-pull-request files that every', + 'pull request rewrites (`FEEDBACK_RESPONSE.md`) and the shared `[Unreleased]` list in', + '`CHANGELOG.md`. `CHANGELOG.md` is union-merged via `.gitattributes`, so it merges cleanly;', + 'a conflict there means a change larger than an added entry.', + ...(log.length + ? ['', '
Log', '', '```', ...log, '```', '
'] + : []), + '', + ].join('\n'); +} + +/** + * The subset of {@link buildConflictComment} that changes as the branch + * changes, used to decide whether an existing comment still needs updating. + * + * @param {string} body + * @returns {string} + */ +function normaliseCommentBody(body) { + return String(body || '') + .split('\n') + .filter((line) => !/^\s*https:\/\/github\.com\//.test(line)) + .join('\n') + .trim(); +} + +module.exports = { + CONFLICT_COMMENT_MARKER, + buildConflictComment, + classifyUpdateResult, + normaliseCommentBody, + shouldAttemptUpdate, +};