From ba393da93f49d02231d265f34d820463f1163261 Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 30 Sep 2026 08:24:27 +0200 Subject: [PATCH 01/13] ci(mergify): report update conflicts without a failing check The Mergify `update` rule reported conclusion `failure` whenever it could not merge develop into a pull request branch, and the action's schema accepts only `bot_account`, so nothing in the rule could change that. Observed across the 394 pull requests opened between 2026-09-03 and 2026-09-29: four failures, split between a genuine conflict (#3524, #3532) and an update attempted on a merged pull request whose branch had already been deleted (#3580, #3662). Merging develop into open branches is now done by a workflow using GitHub's update-branch API. A conflict is written as a comment naming mergeable_state instead of being reported as a red check, and the job reports success on every outcome. Same exclusions as the rule it replaces: non-draft, same-repository, non-archived, base develop, any staleness. The up-to-date requirement is unchanged: develop-branch-ruleset still sets strict_required_status_checks_policy, so a branch behind develop that cannot be updated is still blocked from merging. The workflow is not a required check and no ruleset setting was changed. Closes #3574 --- .github/mergify.yml | 136 +++++----- .github/workflows/keep-pr-current.yml | 176 +++++++++++++ .../__tests__/keep-pr-current.test.js | 235 ++++++++++++++++++ scripts/automation/keep-pr-current.cjs | 184 ++++++++++++++ 4 files changed, 656 insertions(+), 75 deletions(-) create mode 100644 .github/workflows/keep-pr-current.yml create mode 100644 scripts/automation/__tests__/keep-pr-current.test.js create mode 100644 scripts/automation/keep-pr-current.cjs diff --git a/.github/mergify.yml b/.github/mergify.yml index 609156e649..ce3acd8282 100644 --- a/.github/mergify.yml +++ b/.github/mergify.yml @@ -1,87 +1,73 @@ # Mergify configuration. # Schema: https://docs.mergify.com/mergify-configuration-schema.json # -# Scope: keep pull requests current against develop, and auto-merge Dependabot -# updates once the develop ruleset is satisfied. The ruleset still requires the -# required checks and one code-owner approval, so Mergify never merges anything -# a human has not approved. +# Scope: auto-merge Dependabot updates once the develop ruleset is satisfied. +# The ruleset still requires the required checks and one code-owner approval, +# so Mergify never merges anything a human has not approved. # # Removed (#3476): the queue, auto-approve, imgbot and meta-agent rules. # They gated on a check named "All Checks Passed" that no workflow has # produced since the workflow consolidation, and the imgbot and # lightspeed-bot authors have opened no PRs recently. The only mechanism # that actually merged Dependabot PRs is auto_merge below, so it is kept. - -pull_request_rules: - # Every open same-repository pull request against develop merges it in as - # soon as it falls behind, so nothing is ever blocked on staleness alone. - # - # develop's ruleset uses a strict up-to-date policy, so a PR one commit - # behind is blocked from merging exactly as much as one twenty behind. An - # earlier revision of this rule waited until a PR was more than five commits - # behind; that left a band of PRs permanently stuck until they drifted past - # the threshold. The threshold is gone so the rule fires at any staleness. - # - # This replaced a Dependabot-only rule. It is a strict superset of it, so - # keeping both would have matched every Dependabot pull request twice and - # run the same update action from two rules. - # - # `-draft` excludes work-in-progress, following Mergify's own documented - # linear history example. A draft is not waiting to merge, and re-running CI - # on every open draft on every develop push is pure cost. - # - # `-from-fork` excludes forks because Mergify can only write a fork head - # branch when the contributor enabled maintainer edits. Without that - # permission the action fails and the pull request stays behind, so the rule - # must not claim to cover every fork. - # - # `update` rather than `rebase`: `rebase` has to impersonate a GitHub user - # and cannot impersonate a bot-owned account, so it fails outright on - # Dependabot pull requests, and it refuses fork pull requests entirely. - # `update` merges the base in and needs no impersonation. - # - # Mergify skips pull requests in a conflict or already in the merge queue - # (`queue-position = -1`), so one needing manual conflict resolution still - # reports DIRTY, and a queued one is left to the queue. Mergify applies the - # same no-conflict requirement itself, independent of `-conflict` here, so - # that condition is belt-and-braces rather than the only guard. - # - # A pull request that is behind *and* conflicting is the one case this rule - # cannot help, and the `update` check stays red until someone resolves it by - # hand. Two separate things are going on, and it is worth keeping them apart: - # - # 1. `update` merges the base in, and merging cannot resolve a conflict, so - # this rule has nothing to contribute once the pull request is already - # classified as conflicting. It also declines earlier than that: it asks - # the API whether the pull request is mergeable, so on a conflicting one - # it reports "nothing to do" without attempting a merge. - # 2. What the human then has to do used to be resolve a CHANGELOG.md - # conflict by hand, because the changelog gate requires an `[Unreleased]` - # entry on nearly every pull request and every entry lands in the same - # short list (#3574). `.gitattributes` now marks that file `merge=union`, - # so a git merge keeps both sides' additions and that step is just a - # merge and a push. - # - # Step 2 only helps merges git performs. It does not make this rule able to - # update a conflicting pull request, and the red check still appears in that - # state — #3574 tracks that separately. `changelog-merge-strategy.test.js` - # covers the git merge and the attribute; nothing here tests the automated - # update, because the automated update does not consult `.gitattributes`. - # - # Cost: develop's ruleset sets `dismiss_stale_reviews_on_push`, so this - # action dismisses existing approvals on the pull request it touches. That is - # correct rather than incidental — the code the reviewer approved has moved - # underneath them — but it does mean a review must be given again after each - # merge to develop. - - name: Keep same-repository pull requests on develop current - conditions: - - base=develop - - -draft - - -from-fork - - -conflict - - '#commits-behind > 0' - actions: - update: {} +# +# Removed (#3574): the `update` rule "Keep same-repository pull requests on +# develop current". Merging develop into open branches is now done by +# `.github/workflows/keep-pr-current.yml`, for the reasons below. +# +# Keeping this repository's branches current is unchanged in intent and is not +# dropped with the rule: +# +# * `develop-branch-ruleset` sets `strict_required_status_checks_policy`, so a +# branch behind develop is still blocked from merging. That requirement is +# untouched and is what makes the update worth doing at all. +# * `.github/workflows/keep-pr-current.yml` performs the merge, through +# GitHub's `PUT /pulls/{n}/update-branch` API, on the same exclusions the +# rule used: non-draft, same-repository, non-archived, base develop, any +# staleness. +# +# Why the rule had to go rather than be adjusted. The `update` action's schema +# accepts only `bot_account` (`additionalProperties: false` in +# `$defs.UpdateActionModel`, https://docs.mergify.com/mergify-configuration-schema.json), +# so there is no option for how a failure is reported. Mergify reports the +# rule's check run with conclusion `failure` whenever it cannot perform the +# update — including when it declines for its own reasons, not only when the +# merge conflicts. Observed on the 394 pull requests opened between 2026-09-03 +# and 2026-09-29: +# +# * #3524 and #3532 — "merge conflict between base and head". Both were +# conflicting in `FEEDBACK_RESPONSE.md` only, one commit behind develop. +# * #3580 and #3662 — "head ref does not exist", recorded 5 and 6 seconds +# after those pull requests merged and their branches were auto-deleted +# (`delete_branch_on_merge` is enabled on this repository). A failure on a +# merged pull request, actionable by nobody. +# +# `-conflict` did not prevent the first pair. Mergify's own preconditions +# already include "has no conflict" +# (https://docs.mergify.com/workflow/actions/update), so `-conflict` and the +# precondition read the same signal and share the same gap: for a short window +# after develop moves, the pull request's mergeability is not yet computed, the +# condition does not hold, and the update is attempted. On #3524 the rule was +# correctly skipped at 04:33 UTC when Mergify already knew the pull request +# conflicted, then fired at 05:35 UTC — 44 seconds after #3689 landed on +# develop — and failed. No additional condition can close that window, because +# the only available signal is the one that is briefly unavailable. +# +# The merge queue was considered and rejected: Mergify documents +# `strict_required_status_checks_policy` as incompatible with its queue +# (https://docs.mergify.com/merge-queue/github-rulesets — "Preferred: disable +# the Require branches to be up to date before merging setting"), and the only +# queue designs that avoid the clash rely on batch or in-place pull requests. +# Adopting one would mean weakening a ruleset setting, which is the opposite of +# what this change is for. +# +# `merge=union` on CHANGELOG.md (#3581) remains in place and remains the right +# fix for the changelog specifically: it stops two pull requests each adding an +# `[Unreleased]` entry from colliding, which is the highest-churn conflict in +# this repository. It is not a fix for the reporting problem above, because +# `FEEDBACK_RESPONSE.md` is rewritten whole by each pull request that carries +# AI feedback and union-merging two such documents would produce a meaningless +# hybrid rather than a resolved file. merge_protections_settings: reporting_method: check-runs diff --git a/.github/workflows/keep-pr-current.yml b/.github/workflows/keep-pr-current.yml new file mode 100644 index 0000000000..0bfa2905c7 --- /dev/null +++ b/.github/workflows/keep-pr-current.yml @@ -0,0 +1,176 @@ +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: | + 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; GitHub answers 422 rather + // than writing a merge onto a head nobody reviewed. + 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; + 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..dfbc448297 --- /dev/null +++ b/scripts/automation/__tests__/keep-pr-current.test.js @@ -0,0 +1,235 @@ +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'); + }); + + // This is the case Mergify reported as a failing check. + test('409 is a conflict', () => { + expect(classifyUpdateResult({ status: 409 })).toBe('conflict'); + }); + + test('422 is treated as a conflict, so a lost race does not turn the check red', () => { + expect(classifyUpdateResult({ status: 422 })).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..92004a81aa --- /dev/null +++ b/scripts/automation/keep-pr-current.cjs @@ -0,0 +1,184 @@ +/** + * 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/{n}/update-branch`. + * + * 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'|'conflict'|'gone'|'error'} + */ +function classifyUpdateResult({ status, message = '' } = {}) { + // 202 Accepted — the base branch was merged into the head branch. + if (status === 202) { + return 'updated'; + } + + // 409 Conflict — the merge cannot be performed as-is. This is the case + // Mergify reported as a failed check. + if (status === 409) { + return 'conflict'; + } + + // 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'; + } + + // 422 Unprocessable Entity. GitHub returns this both for a concurrent push + // that moved the head out from under `expected_head_sha` and for + // mergeability GitHub will not accept. Treat it as a conflict to report + // rather than as an error, so a lost race does not turn the check red. + if (status === 422) { + return 'conflict'; + } + + if (status === 403) { + return 'gone'; + } + + return message ? 'error' : '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, +}; From 1d243a643cb38cdb8cf1b4f8b9a4c3dc953245bd Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 30 Sep 2026 08:24:34 +0200 Subject: [PATCH 02/13] ci(mergify): validate the config against Mergify's published schema `npm run validate:mergify` checks .github/mergify.yml against https://docs.mergify.com/mergify-configuration-schema.json and names the offending path on failure. The schema is a 176 kB download, so it is fetched rather than vendored; the test asserts the same thing offline only when a fixture is present. The suite also probes the schema for any option that would change how a failed update is reported. Today `actions.update` rejects every property but `bot_account`, which is the evidence for removing the rule rather than reconfiguring it, and the test to revisit if that ever changes. --- CHANGELOG.md | 1 + package.json | 1 + .../__tests__/validate-mergify-config.test.js | 71 ++++++++++++++ .../validation/validate-mergify-config.cjs | 94 +++++++++++++++++++ 4 files changed, 167 insertions(+) create mode 100644 scripts/validation/__tests__/validate-mergify-config.test.js create mode 100644 scripts/validation/validate-mergify-config.cjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d1b42ae51..bb2fdbe5e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,6 +72,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **No Red Check on Stale Pull Requests** — A workflow now merges develop into open branches and reports a conflict as a comment, replacing the Mergify rule that reported it as a failing check nobody could act on. The up-to-date rule is unchanged. ([#3574](https://github.com/lightspeedwp/.github/issues/3574)) - **Docs Bot Skips Workflows Directory** — Regeneration skips `.github/workflows/`, so its pushes stop failing for want of App token `workflows` permission. (#3687) - **Footer Shape Figures Corrected** — Workflow comment: 899 flagged files, 255 holding fewer than two distinct known footer phrases. The runtime notice calls 28.4% a share of flagged files, not a false-positive rate. (#3682, #3604) - **Merged Advisory and Spec Defects Corrected** — The advisories newsletter form survives a failed request, tells a blocked address from a fault and counts its rate limit atomically; spec 018 defers auto-approved branch deletion. (#1396) diff --git a/package.json b/package.json index d1d621a421..3124df587f 100644 --- a/package.json +++ b/package.json @@ -103,6 +103,7 @@ "validate:frontmatter:changed": "node scripts/validation/validate-frontmatter.js", "validate:workflows": "node scripts/validation/validate-workflows.js", "validate:changelog": "node scripts/validation/validate-changelog-safety.js", + "validate:mergify": "node scripts/validation/validate-mergify-config.cjs", "validate:mermaid-syntax": "node scripts/validation/validate-mermaid-syntax.js", "validate:mermaid-accessibility": "node scripts/validation/validate-mermaid-accessibility.js", "validate:footers": "node scripts/dedupe-footers.js --check", diff --git a/scripts/validation/__tests__/validate-mergify-config.test.js b/scripts/validation/__tests__/validate-mergify-config.test.js new file mode 100644 index 0000000000..fdf8b06eee --- /dev/null +++ b/scripts/validation/__tests__/validate-mergify-config.test.js @@ -0,0 +1,71 @@ +const fs = require('node:fs'); +const path = require('node:path'); + +const { buildValidator, configPath, readConfig } = require('../validate-mergify-config.cjs'); + +const repositoryRoot = path.resolve(__dirname, '../../..'); + +// Deliberately not committed: the published schema is ~176 kB, and vendoring it +// is a repository-footprint decision that belongs to a maintainer rather than to +// this change. `npm run validate:mergify` performs the same validation against +// the live schema and prints the result; drop a copy of it at this path to have +// this suite assert the same thing offline. +const schemaPath = path.join(repositoryRoot, 'tests/fixtures/mergify-configuration-schema.json'); +const schemaExists = fs.existsSync(schemaPath); + +// Read lazily inside the tests: `describe.skip` still evaluates its callback, so +// reading the fixture at describe-body scope would throw when it is absent. +function loadSchema() { + return JSON.parse(fs.readFileSync(schemaPath, 'utf8')); +} + +describe('validate-mergify-config', () => { + test('points at the repository configuration', () => { + expect(configPath).toBe(path.join(repositoryRoot, '.github/mergify.yml')); + }); + + test('reads .github/mergify.yml as an object', () => { + const config = readConfig(); + + expect(typeof config).toBe('object'); + expect(config).not.toBeNull(); + }); +}); + +(schemaExists ? describe : describe.skip)('mergify.yml against the published schema', () => { + test('the fixture is the Mergify schema', () => { + expect(loadSchema().properties).toHaveProperty('pull_request_rules'); + }); + + test('the configuration validates', () => { + const validate = buildValidator(loadSchema()); + const valid = validate(readConfig()); + + if (!valid) { + // Printed so a failure names the offending path rather than only + // reporting that something is wrong somewhere. + console.error(JSON.stringify(validate.errors, null, 2)); + } + expect(valid).toBe(true); + }); + + // The evidence behind removing the rule rather than reconfiguring it. If a + // future schema revision adds an option that changes how a failed update is + // reported, this is the test to revisit. + test('the update action still accepts no reporting option', () => { + const validate = buildValidator(loadSchema()); + const probe = { + pull_request_rules: [ + { + name: 'probe', + conditions: ['base=develop'], + actions: { update: { method: 'rebase' } }, + }, + ], + }; + const valid = validate(probe); + + expect(valid).toBe(false); + expect(validate.errors.some((error) => error.keyword === 'additionalProperties')).toBe(true); + }); +}); diff --git a/scripts/validation/validate-mergify-config.cjs b/scripts/validation/validate-mergify-config.cjs new file mode 100644 index 0000000000..caeb4703a7 --- /dev/null +++ b/scripts/validation/validate-mergify-config.cjs @@ -0,0 +1,94 @@ +#!/usr/bin/env node +/** + * Validate .github/mergify.yml against Mergify's published JSON schema. + * + * Schema: https://docs.mergify.com/mergify-configuration-schema.json + * + * Not wired into npm run validate:* — the repo does not otherwise depend on a + * schema download, and this script is the reproducible form of that check for + * anyone editing the config. Pass a schema path as the first argument to + * validate against a local copy instead of fetching. + * + * Usage: + * node scripts/validation/validate-mergify-config.cjs + * node scripts/validation/validate-mergify-config.cjs /path/to/schema.json + */ +const fs = require('node:fs'); +const https = require('node:https'); +const path = require('node:path'); + +const SCHEMA_URL = 'https://docs.mergify.com/mergify-configuration-schema.json'; +const repositoryRoot = path.resolve(__dirname, '../..'); +const configPath = path.join(repositoryRoot, '.github/mergify.yml'); + +function fetchSchema(url) { + return new Promise((resolve, reject) => { + https + .get(url, (response) => { + if (response.statusCode !== 200) { + reject(new Error(`GET ${url} responded ${response.statusCode}`)); + response.resume(); + return; + } + response.setEncoding('utf8'); + let body = ''; + response.on('data', (chunk) => { + body += chunk; + }); + response.on('end', () => resolve(body)); + }) + .on('error', reject); + }); +} + +function buildValidator(schema) { + // The published schema is JSON Schema 2020-12, which ajv exposes through a + // separate entry point from the default draft-07 build. + const Ajv2020 = require('ajv/dist/2020'); + // The published schema uses custom formats (`simple-template`, `regex`, + // `template`, `duration`) that ajv does not implement. `logger: false` drops + // ajv's "unknown format ignored" notices for those; they do not affect the + // keywords this validation depends on. + const ajv = new Ajv2020({ strict: false, allErrors: true, logger: false }); + return ajv.compile(schema); +} + +function readConfig() { + const YAML = require('yaml'); + return YAML.parse(fs.readFileSync(configPath, 'utf8')); +} + +async function main() { + const schemaPath = process.argv[2]; + const rawSchema = schemaPath + ? fs.readFileSync(schemaPath, 'utf8') + : await fetchSchema(SCHEMA_URL); + const schema = JSON.parse(rawSchema); + const validate = buildValidator(schema); + + const config = readConfig(); + const valid = validate(config); + + console.log(`schema: ${schemaPath || SCHEMA_URL}`); + console.log(`config: ${path.relative(repositoryRoot, configPath)}`); + console.log(`result: ${valid ? 'valid' : 'INVALID'}`); + + if (!valid) { + for (const error of validate.errors || []) { + console.error(` ${error.instancePath || '/'} ${error.message}`); + } + process.exitCode = 1; + return; + } + + console.log(`top-level keys: ${Object.keys(config).join(', ') || '(none)'}`); +} + +module.exports = { buildValidator, readConfig, configPath, SCHEMA_URL }; + +if (require.main === module) { + main().catch((error) => { + console.error(`validate-mergify-config: ${error.message}`); + process.exitCode = 1; + }); +} From 27c0c16da115a4c6506d2744d892952fd14be19f Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 30 Sep 2026 08:29:18 +0200 Subject: [PATCH 03/13] ci(mergify): note why the relative require path is correct actions/github-script runs the script with the workspace as its working directory, which is why the relative specifier resolves. Recorded so the next reader does not 'fix' it to an absolute path and break it. --- .github/workflows/keep-pr-current.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/keep-pr-current.yml b/.github/workflows/keep-pr-current.yml index 0bfa2905c7..13efb12898 100644 --- a/.github/workflows/keep-pr-current.yml +++ b/.github/workflows/keep-pr-current.yml @@ -64,6 +64,8 @@ jobs: 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, From 22ba5c8f79308723206c63ef496277ab7452255b Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 30 Sep 2026 08:31:52 +0200 Subject: [PATCH 04/13] fix(mergify): separate a real conflict from an already-current pull request GitHub answers 422 for both, so status alone cannot classify the result. 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 what most open pull requests return on a push to develop, because most of them are not behind. Reading every 422 as a conflict would post a false conflict comment on every open pull request each time develop is pushed to, so the message is now part of the classification and only a genuine conflict produces a comment. Also captures the API's own message rather than Octokit's wrapper text, since that message is what the classification depends on. --- .github/workflows/keep-pr-current.yml | 7 +-- .../__tests__/keep-pr-current.test.js | 35 ++++++++++-- scripts/automation/keep-pr-current.cjs | 54 ++++++++++++++----- 3 files changed, 75 insertions(+), 21 deletions(-) diff --git a/.github/workflows/keep-pr-current.yml b/.github/workflows/keep-pr-current.yml index 13efb12898..a859615440 100644 --- a/.github/workflows/keep-pr-current.yml +++ b/.github/workflows/keep-pr-current.yml @@ -119,8 +119,7 @@ jobs: let message = ''; try { // expected_head_sha guards against merging into a branch that - // moved while this run was in flight; GitHub answers 422 rather - // than writing a merge onto a head nobody reviewed. + // moved while this run was in flight. const { status: responseStatus } = await github.rest.pulls.updateBranch({ owner, repo, @@ -130,7 +129,9 @@ jobs: status = responseStatus; } catch (error) { status = error.status; - message = error.message || ''; + // 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 }); diff --git a/scripts/automation/__tests__/keep-pr-current.test.js b/scripts/automation/__tests__/keep-pr-current.test.js index dfbc448297..e151041ba1 100644 --- a/scripts/automation/__tests__/keep-pr-current.test.js +++ b/scripts/automation/__tests__/keep-pr-current.test.js @@ -98,13 +98,40 @@ describe('classifyUpdateResult', () => { expect(classifyUpdateResult({ status: 202 })).toBe('updated'); }); - // This is the case Mergify reported as a failing check. - test('409 is a conflict', () => { + // 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('422 is treated as a conflict, so a lost race does not turn the check red', () => { - expect(classifyUpdateResult({ status: 422 })).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', () => { diff --git a/scripts/automation/keep-pr-current.cjs b/scripts/automation/keep-pr-current.cjs index 92004a81aa..a5fd13bd96 100644 --- a/scripts/automation/keep-pr-current.cjs +++ b/scripts/automation/keep-pr-current.cjs @@ -75,7 +75,18 @@ function shouldAttemptUpdate(pullRequest) { } /** - * Classify the result of `PUT /repos/{owner}/{repo}/pulls/{n}/update-branch`. + * 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 @@ -83,20 +94,34 @@ function shouldAttemptUpdate(pullRequest) { * check — that is the defect being fixed. * * @param {{status?: number, message?: string}} outcome - * @returns {'updated'|'conflict'|'gone'|'error'} + * @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'; } - // 409 Conflict — the merge cannot be performed as-is. This is the case - // Mergify reported as a failed check. - if (status === 409) { + // 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 @@ -106,19 +131,20 @@ function classifyUpdateResult({ status, message = '' } = {}) { return 'gone'; } - // 422 Unprocessable Entity. GitHub returns this both for a concurrent push - // that moved the head out from under `expected_head_sha` and for - // mergeability GitHub will not accept. Treat it as a conflict to report - // rather than as an error, so a lost race does not turn the check red. - if (status === 422) { - return 'conflict'; - } - + // 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'; } - return message ? 'error' : 'error'; + // 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'; } /** From 00f63199bfd00c96634e7ff9c808781159526155 Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 30 Sep 2026 08:40:17 +0200 Subject: [PATCH 05/13] docs(changelog): keep the new entry within the 250 character limit The pr_submission validator reports CHK_MAX_LENGTH as a critical rule, so the entry was adding one new failure against the base branch. Now 10 failures on both base and this branch. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bb2fdbe5e7..8374b2a0e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,7 +72,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- **No Red Check on Stale Pull Requests** — A workflow now merges develop into open branches and reports a conflict as a comment, replacing the Mergify rule that reported it as a failing check nobody could act on. The up-to-date rule is unchanged. ([#3574](https://github.com/lightspeedwp/.github/issues/3574)) +- **No Red Check on Stale Pull Requests** — A workflow merges develop into open branches and reports a conflict as a comment, replacing the Mergify rule that reported a failing check. ([#3574](https://github.com/lightspeedwp/.github/issues/3574)) - **Docs Bot Skips Workflows Directory** — Regeneration skips `.github/workflows/`, so its pushes stop failing for want of App token `workflows` permission. (#3687) - **Footer Shape Figures Corrected** — Workflow comment: 899 flagged files, 255 holding fewer than two distinct known footer phrases. The runtime notice calls 28.4% a share of flagged files, not a false-positive rate. (#3682, #3604) - **Merged Advisory and Spec Defects Corrected** — The advisories newsletter form survives a failed request, tells a blocked address from a fault and counts its rate limit atomically; spec 018 defers auto-approved branch deletion. (#1396) From 52245c9716072a01911bb7e9f7381a4e8ad69416 Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 30 Sep 2026 13:56:57 +0200 Subject: [PATCH 06/13] fix(mergify): scope conflict state per pull request and classify by message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects in the keep-current path, all of the same shape: state or a guess that does not belong where it was. The log array was declared outside the loop over open pull requests, so each conflict comment carried every earlier pull request's API response as well — publishing one pull request's status on another. The per-pull-request work now lives in processPullRequest behind an injected client, which owns its own log lines, and the workflow is a thin loop. Reintroducing a shared array makes two tests fail, so the defect is covered rather than removed. update-branch answers 422 for three unrelated situations. All were observed here against a deliberately wrong expected_head_sha: 202 Updating pull request branch. 422 There are no new commits on the base branch. 422 merge conflict between base and head 422 head ref does not exist 404 Not Found The third is the text Mergify reported on #3580 and #3662 seconds after each merged; read as a conflict it would have commented on a merged pull request telling a human to resolve a conflict that does not exist. Unknown messages now classify as an error rather than as a conflict, and a throttled call is retryable, because GitHub documents 422 as "Validation failed, or the endpoint has been spammed". Anything that is not a clean update, a no-op or a conflict is raised as a warning and listed in the job summary rather than logged quietly. expected_head_sha is documented as the guard against a head that moved, but it was not enforced when probed: a deliberately wrong value still returned 202 and updated the branch. The comment claimed otherwise and now says what was actually observed, so it reads as defence in depth. Comment lookup is paginated, so a pull request with more than a page of earlier comments still finds its own and updates it instead of duplicating. --- .github/workflows/keep-pr-current.yml | 114 ++----- .../__tests__/process-pull-request.test.js | 315 ++++++++++++++++++ scripts/automation/keep-pr-current.cjs | 206 +++++++++--- 3 files changed, 505 insertions(+), 130 deletions(-) create mode 100644 scripts/automation/__tests__/process-pull-request.test.js diff --git a/.github/workflows/keep-pr-current.yml b/.github/workflows/keep-pr-current.yml index a859615440..e86a9ce3b6 100644 --- a/.github/workflows/keep-pr-current.yml +++ b/.github/workflows/keep-pr-current.yml @@ -17,6 +17,17 @@ name: Keep Pull Request Current # `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. +# +# `synchronize` is deliberately absent from the `pull_request` types. Updating a +# branch is itself a push to it, so including `synchronize` would make the +# update re-trigger this workflow and loop. Branches fall behind because +# `develop` moves, which is why `push` is the trigger that matters. +# +# GitHub cannot run `pull_request` workflows at all on a pull request that +# conflicts (`refs/pull//merge` is 404 because the merge ref cannot be +# built), so a conflicting pull request is only ever reached from a push to +# `develop`. The `pull_request` trigger exists to cover the opened/reopened and +# ready-for-review cases. on: push: branches: @@ -66,16 +77,9 @@ jobs: 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 { processPullRequest } = 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. @@ -95,85 +99,23 @@ jobs: core.info(`Considering ${numbers.length} pull request(s) against develop.`); + // Each pull request is handled independently by processPullRequest, + // which owns its own log lines and its own comment. Nothing is + // carried between iterations. + const summary = []; 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. + const result = await processPullRequest({ github, owner, repo, number, logger: core }); + summary.push(`#${number}: ${result.outcome}`); } core.info(`Processed ${numbers.length} pull request(s).`); + await core.summary + .addHeading('Keep pull request current') + .addTable([ + [ + { data: 'Pull request', header: true }, + { data: 'Outcome', header: true }, + ], + ...summary.map((line) => [line.split(':')[0], line.split(':').slice(1).join(':').trim()]), + ]) + .write(); diff --git a/scripts/automation/__tests__/process-pull-request.test.js b/scripts/automation/__tests__/process-pull-request.test.js new file mode 100644 index 0000000000..88870e5fbf --- /dev/null +++ b/scripts/automation/__tests__/process-pull-request.test.js @@ -0,0 +1,315 @@ +const { CONFLICT_COMMENT_MARKER, processPullRequest } = require('../keep-pr-current.cjs'); + +/** + * A pull request as `pulls.get` returns it. + */ +function pullRequest(number, overrides = {}) { + return { + number, + state: 'open', + draft: false, + mergeable_state: 'dirty', + head: { + sha: `sha${number}`, + repo: { owner: { login: 'lightspeedwp' }, archived: false }, + }, + base: { ref: 'develop', repo: { owner: { login: 'lightspeedwp' } } }, + ...overrides, + }; +} + +/** + * An Octokit-like client for `processPullRequest`. + * + * `results` maps a pull request number to the outcome its `updateBranch` call + * should produce. `comments` is the existing comment set per pull request, and + * the client records what was written where. + */ +function mockGithub({ results = {}, pulls = {}, comments = {} } = {}) { + const written = { created: [], updated: [] }; + + const github = { + paginate: jest.fn(async (method, params) => { + // Mirrors actions/github-script's paginate: calls the route and resolves + // with the first page of data. + return method(params).then((response) => response.data); + }), + rest: { + pulls: { + get: jest.fn(async ({ pull_number: number }) => ({ + data: pulls[number] || pullRequest(number), + })), + updateBranch: jest.fn(async ({ pull_number: number }) => { + const result = results[number] || { + status: 202, + }; + if (result.status >= 400) { + const error = new Error(result.message || 'failed'); + error.status = result.status; + error.response = { data: { message: result.message || '' } }; + throw error; + } + return { status: result.status }; + }), + }, + issues: { + listComments: jest.fn(async ({ issue_number: number }) => ({ + data: comments[number] || [], + })), + createComment: jest.fn(async (params) => { + written.created.push(params); + return { data: { id: written.created.length } }; + }), + updateComment: jest.fn(async (params) => { + written.updated.push(params); + return { data: { id: 1 } }; + }), + }, + }, + }; + + return { github, written }; +} + +const silentLogger = { info: () => {}, warning: () => {} }; + +function conflict(message = 'merge conflict between base and head') { + return { status: 422, message }; +} + +/** + * The log block of a conflict comment: the fenced list under the Log summary. + * The heading and the conflict URL always name the pull request itself, so an + * assertion about cross-contamination has to look here and only here. + */ +function logBlock(body) { + const match = String(body).match(/
Log<\/summary>[\s\S]*?```([\s\S]*?)```/); + return match ? match[1] : ''; +} + +describe('processPullRequest', () => { + test('a conflicting pull request gets a comment naming only itself', async () => { + const { github, written } = mockGithub({ results: { 101: conflict() } }); + + const result = await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 101, + logger: silentLogger, + }); + + expect(result.outcome).toBe('conflict'); + expect(written.created).toHaveLength(1); + expect(logBlock(written.created[0].body)).toContain('#101'); + }); + + // The regression this guards. The caller iterates over every open pull + // request in one run, and the log lines used to live in an array declared + // outside that loop, so each comment carried every earlier pull request's + // status too — publishing one pull request's API response on another. + test('a later pull request never sees an earlier one in its comment', async () => { + const { github, written } = mockGithub({ + results: { 101: conflict(), 202: conflict() }, + }); + + await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 101, + logger: silentLogger, + }); + await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 202, + logger: silentLogger, + }); + + expect(written.created).toHaveLength(2); + const [first, second] = written.created.map((call) => logBlock(call.body)); + + expect(first).toContain('#101'); + expect(first).not.toContain('#202'); + expect(second).toContain('#202'); + expect(second).not.toContain('#101'); + }); + + test('an already-current pull request gets no comment', async () => { + const { github, written } = mockGithub({ + results: { 303: { status: 422, message: 'There are no new commits on the base branch.' } }, + }); + + const result = await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 303, + logger: silentLogger, + }); + + expect(result.outcome).toBe('current'); + expect(written.created).toHaveLength(0); + }); + + // Mergify reported this one on #3580 and #3662, seconds after those pull + // requests merged. Treating it as a conflict would comment on a merged pull + // request telling a human to resolve a conflict that does not exist. + test('a merged pull request with a deleted branch gets no conflict comment', async () => { + const { github, written } = mockGithub({ + results: { 404: { status: 422, message: 'head ref does not exist' } }, + }); + + const result = await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 404, + logger: silentLogger, + }); + + expect(result.outcome).toBe('gone'); + expect(written.created).toHaveLength(0); + }); + + test('a successful update reports updated and posts nothing', async () => { + const { github, written } = mockGithub({ results: { 505: { status: 202 } } }); + + const result = await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 505, + logger: silentLogger, + }); + + expect(result.outcome).toBe('updated'); + expect(written.created).toHaveLength(0); + }); + + // An unrecognised response must not look like a clean success and must not + // become a conflict comment on a pull request that may be fine. + test('an unrecognised response is reported and warns', async () => { + const warnings = []; + const { github, written } = mockGithub({ + results: { 606: { status: 503, message: 'Service unavailable' } }, + }); + + const result = await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 606, + logger: { info: () => {}, warning: (line) => warnings.push(line) }, + }); + + expect(result.outcome).toBe('error'); + expect(written.created).toHaveLength(0); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('#606'); + expect(warnings[0]).toContain('was not updated'); + }); + + test('an unknown 422 message is an error, not a conflict', async () => { + const { github, written } = mockGithub({ + results: { 707: { status: 422, message: 'Something GitHub has not said before' } }, + }); + + const result = await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 707, + logger: silentLogger, + }); + + expect(result.outcome).toBe('error'); + expect(written.created).toHaveLength(0); + }); + + test('a draft is skipped without calling the API', async () => { + const { github } = mockGithub({ pulls: { 808: pullRequest(808, { draft: true }) } }); + + const result = await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 808, + logger: silentLogger, + }); + + expect(result.outcome).toBe('skipped'); + expect(github.rest.pulls.updateBranch).not.toHaveBeenCalled(); + }); + + test('an existing comment is updated rather than duplicated', async () => { + const existing = { + user: { type: 'Bot' }, + body: `${CONFLICT_COMMENT_MARKER}\n## Pull request #909 could not be updated automatically\n\nold body\n`, + }; + const { github, written } = mockGithub({ + results: { 909: conflict() }, + comments: { 909: [existing] }, + }); + + const result = await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 909, + logger: silentLogger, + }); + + expect(result.commented).toBe(true); + expect(written.created).toHaveLength(0); + expect(written.updated).toHaveLength(1); + expect(written.updated[0].comment_id).toBeUndefined(); + }); + + test('an unchanged comment is left alone', async () => { + const { github, written } = mockGithub({ results: { 1: conflict() } }); + await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 1, + logger: silentLogger, + }); + + const body = written.created[0].body; + const second = mockGithub({ + results: { 1: conflict() }, + comments: { 1: [{ user: { type: 'Bot' }, body }] }, + }); + const result = await processPullRequest({ + github: second.github, + owner: 'lightspeedwp', + repo: '.github', + number: 1, + logger: silentLogger, + }); + + expect(result.commented).toBe(false); + expect(second.written.updated).toHaveLength(0); + expect(second.written.created).toHaveLength(0); + }); + + test('comments are paginated, so an older marker comment is still found', async () => { + const { github } = mockGithub({ results: { 10: conflict() } }); + + await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 10, + logger: silentLogger, + }); + + expect(github.paginate).toHaveBeenCalledWith( + github.rest.issues.listComments, + expect.any(Object) + ); + }); +}); diff --git a/scripts/automation/keep-pr-current.cjs b/scripts/automation/keep-pr-current.cjs index a5fd13bd96..ee4b415c68 100644 --- a/scripts/automation/keep-pr-current.cjs +++ b/scripts/automation/keep-pr-current.cjs @@ -17,9 +17,8 @@ * 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. + * Everything here is a pure function of its arguments, or takes an injected + * Octokit-like client, so the behaviour is testable without a network round trip. */ /** @@ -77,24 +76,35 @@ function shouldAttemptUpdate(pullRequest) { /** * 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: + * The status alone is not enough, because GitHub answers **422 for three + * unrelated situations**. All four responses below were observed against this + * repository on 2026-09-30 against a deliberately wrong `expected_head_sha`: * - * - `{"message": "merge conflict between base and head", "status": "422"}` - * - `{"message": "There are no new commits on the base branch.", "status": "422"}` + * | HTTP | message | when | + * | ---- | --------------------------------------------------- | ------------------------------------- | + * | 202 | `Updating pull request branch.` | the merge was started | + * | 422 | `There are no new commits on the base branch.` | already current | + * | 422 | `merge conflict between base and head` | head conflicts with base | + * | 422 | `head ref does not exist` | head branch deleted after the merge | + * | 404 | `Not Found` | the pull request itself is gone | * - * 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. + * The third is the same text Mergify reported on #3580 and #3662, seconds + * after those pull requests merged and their branches were auto-deleted. Reading + * it as a conflict would post a false conflict comment on a merged pull + * request, so the message is what decides. * - * 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. + * GitHub documents only 202, 403 and 422 for this endpoint + * (https://docs.github.com/en/rest/pulls/pulls#update-a-pull-request-branch), + * where 422 is described as "Validation failed, or the endpoint has been + * spammed" — so a secondary rate limit can also arrive as 422 and is treated as + * retryable rather than as a conflict. + * + * An unrecognised response is `error`. It is surfaced as a warning and in the + * job summary, never as a passing no-op, and never as a conflict comment on a + * pull request that may have nothing wrong with it. * * @param {{status?: number, message?: string}} outcome - * @returns {'updated'|'current'|'conflict'|'retry'|'gone'|'error'} + * @returns {'updated'|'current'|'conflict'|'gone'|'denied'|'retry'|'error'} */ function classifyUpdateResult({ status, message = '' } = {}) { const text = String(message).toLowerCase(); @@ -104,46 +114,41 @@ function classifyUpdateResult({ status, message = '' } = {}) { 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. + // Message checks come before status checks, because the 422 cases are + // distinguished only by their message. 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')) { + if (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'; + if (text.includes('head ref does not exist')) { + return 'gone'; } - // 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'; + // Rate limiting and endpoint throttling: documented as 403/422, and the + // message is the only reliable signal. Not observed on this repository. + if ( + status === 429 || + /rate limit|abuse detection|try again later|endpoint has been spammed/.test(text) + ) { + return 'retry'; } - // 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. + // 403 Forbidden. GitHub documents the status but not the message; the most + // common cause is a token that cannot write this head branch, for example a + // fork whose author has not enabled maintainer edits. Not observed here. if (status === 403) { - return 'gone'; + return 'denied'; } - // 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'; + // 404 Not Found. Observed for a pull request that no longer exists. + if (status === 404) { + return 'gone'; } + // 5xx and anything unrecognised, including a 422 whose message is not one of + // the three above. return 'error'; } @@ -153,7 +158,7 @@ function classifyUpdateResult({ status, message = '' } = {}) { * @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. + * @param {string[]} [params.log] - Log lines for this pull request only. * @returns {string} Markdown comment body. */ function buildConflictComment({ pullNumber, mergeableState = 'unknown', log = [] }) { @@ -170,7 +175,7 @@ function buildConflictComment({ pullNumber, mergeableState = 'unknown', log = [] '', '**To resolve it:**', '', - `1. Merge \`develop\` into this branch locally (\`git merge origin/develop\`) and push the`, + '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}`, @@ -201,10 +206,123 @@ function normaliseCommentBody(body) { .trim(); } +/** + * Run the update for one pull request and report the outcome. + * + * The per-pull-request log is built here rather than by the caller, so state + * cannot carry from one pull request to the next: the caller iterates over many + * pull requests in a single run, and a log array shared across iterations put + * every earlier pull request's status into a later one's comment. + * + * @param {object} params + * @param {object} params.github - Octokit-like client (actions/github-script `github`). + * @param {string} params.owner + * @param {string} params.repo + * @param {number} params.number - Pull request number. + * @param {{info: Function, warning: Function}} [params.logger] - actions/core-like. + * @returns {Promise<{outcome: string, status?: number, message?: string, commented?: boolean}>} + */ +async function processPullRequest({ github, owner, repo, number, logger = console }) { + let pullRequest; + try { + ({ data: pullRequest } = await github.rest.pulls.get({ owner, repo, pull_number: number })); + } catch (error) { + // Merged or deleted between listing and reading. Nothing to do, and not a + // failure of this workflow. + logger.info(`#${number}: skipped, could not read (${error.status}).`); + return { outcome: 'unreadable', status: error.status, message: error.message || '' }; + } + + const decision = shouldAttemptUpdate(pullRequest); + if (!decision.attempt) { + logger.info(`#${number}: skipped (${decision.reason}).`); + return { outcome: 'skipped', message: decision.reason }; + } + + let status; + let message = ''; + try { + // expected_head_sha is documented as the guard against merging into a head + // that moved + // (https://docs.github.com/en/rest/pulls/pulls#update-a-pull-request-branch: + // "If the expected SHA does not match the pull request's HEAD, you will + // receive a 422 Unprocessable Entity status"). It was **not** observed to + // be enforced on 2026-09-30 — a deliberately wrong value still returned 202 + // and updated the branch — so it is defence in depth, not the thing that + // makes this call safe. The staleness check also runs first, which is why a + // wrong value on an already-current pull request still returns the + // "no new commits" 422. + const response = await github.rest.pulls.updateBranch({ + owner, + repo, + pull_number: number, + expected_head_sha: pullRequest.head.sha, + }); + status = response.status; + } catch (error) { + status = error.status; + // The API's own message is what distinguishes the 422 cases, so prefer it + // over Octokit's wrapper text. + message = error.response?.data?.message || error.message || ''; + } + + const outcome = classifyUpdateResult({ status, message }); + logger.info(`#${number}: ${outcome} (HTTP ${status}${message ? `, ${message}` : ''}).`); + + if (outcome === 'conflict') { + // Scoped to this pull request. Deliberately not accumulated across calls. + const log = [`#${number} HTTP ${status}: ${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. Paginated, so a pull request with more than a page of earlier + // comments still finds its own. + const comments = await github.paginate(github.rest.issues.listComments, { + owner, + repo, + issue_number: number, + per_page: 100, + }); + const existing = comments.find( + (comment) => + comment.user?.type === 'Bot' && String(comment.body || '').includes(CONFLICT_COMMENT_MARKER) + ); + + if (existing && normaliseCommentBody(existing.body) === normaliseCommentBody(body)) { + logger.info(`#${number}: conflict comment already up to date.`); + return { outcome, status, message, commented: false }; + } + 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 }); + } + return { outcome, status, message, commented: true }; + } + + // Surfaced rather than logged quietly. The job still exits 0, because a + // repository or permission problem is not a reason to fail every pull + // request's check, but it must not look like a clean success either. + if (outcome === 'error' || outcome === 'denied' || outcome === 'retry') { + logger.warning( + `#${number}: ${outcome} (HTTP ${status}${message ? `, ${message}` : ''}) — ` + + 'no conflict comment was posted and this pull request was not updated.' + ); + } + + return { outcome, status, message }; +} + module.exports = { CONFLICT_COMMENT_MARKER, buildConflictComment, classifyUpdateResult, normaliseCommentBody, + processPullRequest, shouldAttemptUpdate, }; From 13b10138c12688a12f88d4ec0ec43e81c2229d8f Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 30 Sep 2026 13:57:08 +0200 Subject: [PATCH 07/13] docs(validation): correct the stale validate:mergify header The header claimed the script was "not wired into npm run validate:*", which stopped being true when package.json gained the validate:mergify script in this series. It now states what is actually true: it is wired as npm run validate:mergify, it is deliberately not one of the ten steps in validate:all, and no workflow runs it, because it downloads the schema and every validate:all step works offline. --- scripts/validation/validate-mergify-config.cjs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/scripts/validation/validate-mergify-config.cjs b/scripts/validation/validate-mergify-config.cjs index caeb4703a7..9cd10af288 100644 --- a/scripts/validation/validate-mergify-config.cjs +++ b/scripts/validation/validate-mergify-config.cjs @@ -4,13 +4,17 @@ * * Schema: https://docs.mergify.com/mergify-configuration-schema.json * - * Not wired into npm run validate:* — the repo does not otherwise depend on a - * schema download, and this script is the reproducible form of that check for - * anyone editing the config. Pass a schema path as the first argument to - * validate against a local copy instead of fetching. + * Wired as `npm run validate:mergify`. It is deliberately **not** one of the + * steps in `npm run validate:all` and no workflow runs it, because it downloads + * the schema over the network and every step in `validate:all` works offline. + * That makes it a maintainer-run check: run it by hand after editing + * `.github/mergify.yml`, or add it to a workflow that may reach the network. + * + * Pass a schema path as the first argument to validate against a local copy + * instead of fetching. * * Usage: - * node scripts/validation/validate-mergify-config.cjs + * npm run validate:mergify * node scripts/validation/validate-mergify-config.cjs /path/to/schema.json */ const fs = require('node:fs'); From 003a7c61518a4be9e67206b23e378970cc8c9953 Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 30 Sep 2026 13:57:39 +0200 Subject: [PATCH 08/13] test(mergify): cover the update-branch responses actually observed Replaces the two classifier assertions that rested on unverified messages: - "head branch was modified" for a stale expected_head_sha was never observed on this repository; a deliberately wrong value returned 202 and updated the branch. The test now pins the three 422 texts that were observed, so an unverified guess cannot drift back in. - 403 asserted "gone", which merges two different problems: a token that cannot write the branch, and a throttled call. It is now "denied", with rate-limit text and 429 classified as retryable first. A 422 whose message is none of the three is an error, not a conflict, so an unfamiliar rejection is surfaced rather than posted on a pull request that may have nothing wrong with it. Also asserts the workflow keeps no state outside the per-pull-request loop and delegates the API call to the module instead of duplicating it. --- .../__tests__/keep-pr-current.test.js | 66 ++++++++++++++----- 1 file changed, 51 insertions(+), 15 deletions(-) diff --git a/scripts/automation/__tests__/keep-pr-current.test.js b/scripts/automation/__tests__/keep-pr-current.test.js index e151041ba1..1aa4046232 100644 --- a/scripts/automation/__tests__/keep-pr-current.test.js +++ b/scripts/automation/__tests__/keep-pr-current.test.js @@ -117,29 +117,56 @@ describe('classifyUpdateResult', () => { ).toBe('current'); }); - test('422 from a concurrent push to the head is a benign retry', () => { + // Observed against this repository on 2026-09-30, on #3580 — a pull request + // merged four days earlier whose head branch was auto-deleted. This is the + // same text Mergify reported on #3580 and #3662 seconds after each merge. + test('422 saying the head ref does not exist means gone, not conflict', () => { + expect(classifyUpdateResult({ status: 422, message: 'head ref does not exist' })).toBe('gone'); + }); + + // A wrong expected_head_sha did not produce this. On a genuinely behind pull + // request a deliberately wrong value still returned 202 and updated the + // branch, so the documented 422 mismatch was not reproducible and there is + // nothing to match on. + test('a response mentioning the expected head SHA is not silently accepted', () => { + expect(classifyUpdateResult({ status: 422, message: 'Expected head sha does not match' })).toBe( + 'error' + ); + }); + + // GitHub documents 422 as "Validation failed, or the endpoint has been + // spammed", so a throttled endpoint can arrive as 422. It must not be read + // as a conflict, which would comment on every pull request while throttled. + test('a throttled response is retryable whatever the status', () => { expect( - classifyUpdateResult({ - status: 422, - message: 'Head branch was modified. Review and try the merge again.', - }) + classifyUpdateResult({ status: 422, message: 'You have exceeded a secondary rate limit' }) ).toBe('retry'); + expect(classifyUpdateResult({ status: 429, message: 'Too Many Requests' })).toBe('retry'); + expect(classifyUpdateResult({ status: 403, message: 'rate limit exceeded' })).toBe('retry'); }); - test('409 is treated as a conflict', () => { - expect(classifyUpdateResult({ status: 409 })).toBe('conflict'); + // Not one of the three observed 422 messages, so it is surfaced as an error + // rather than turned into a conflict comment on a pull request that may have + // nothing wrong with it. + test('an unrecognised 422 is an error rather than a conflict', () => { + expect(classifyUpdateResult({ status: 422, message: 'something new' })).toBe('error'); }); - test('an unrecognised 422 is still surfaced rather than swallowed', () => { - expect(classifyUpdateResult({ status: 422, message: 'something new' })).toBe('conflict'); + test('403 is denied rather than gone: they mean different things', () => { + expect(classifyUpdateResult({ status: 403 })).toBe('denied'); }); - test('404 is the head branch having been deleted, not an error', () => { - expect(classifyUpdateResult({ status: 404 })).toBe('gone'); + test('a 5xx is an error', () => { + expect(classifyUpdateResult({ status: 503, message: 'Service unavailable' })).toBe('error'); + expect(classifyUpdateResult({ status: 500 })).toBe('error'); }); - test('403 is treated as gone rather than as an error to report', () => { - expect(classifyUpdateResult({ status: 403 })).toBe('gone'); + test('no status at all is an error, not a silent success', () => { + expect(classifyUpdateResult({})).toBe('error'); + }); + + test('404 is the head branch having been deleted, not an error', () => { + expect(classifyUpdateResult({ status: 404 })).toBe('gone'); }); test('anything else is an error', () => { @@ -242,8 +269,17 @@ describe('keep-pr-current workflow', () => { 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'); + // The API call moved into the module so it can be unit tested; the workflow + // must not reach back into the API itself. + test('delegates the API call to the module rather than duplicating it', () => { + expect(workflow).toContain('processPullRequest'); + expect(workflow).not.toContain('updateBranch'); + }); + + // The defect CodeRabbit raised: a log array declared outside the loop put one + // pull request's API response into a later one's comment. + test('keeps no state outside the per-pull-request loop', () => { + expect(workflow).not.toContain('const log = []'); }); test('never checks out pull request code', () => { From 08a375d6becfe2003460473d93d34266a5318d3a Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 30 Sep 2026 17:50:27 +0200 Subject: [PATCH 09/13] fix(mergify): update branches with the bot App token, not GITHUB_TOKEN A branch update is a push. A push made with the workflow's own GITHUB_TOKEN leaves the new head's pull_request runs unusable, so a pull request this workflow updates would sit BLOCKED with its required checks missing. Measured on a scratch same-repository draft pull request on 2026-09-30, over two separate updates. Before 039ac1b5 -> 1489e936 and 1489e936 -> 30105ade, each new head had ten pull_request workflow runs, every one of them action_required, and therefore no check runs at all: no "Route PR template and apply labels", no "Validate changelog on PR", no actionlint. That is documented behaviour, not a fault. From https://docs.github.com/en/actions/concepts/security/github_token : "pull_request events with the opened, synchronize, or reopened activity types: when a workflow using GITHUB_TOKEN creates or updates a pull request, the resulting pull_request event creates workflow runs in an approval-required state. The pull request displays a banner in the merge box, and a user with write access to the repository can start the runs by selecting Approve workflows to run." The merge commit is attributed to github-actions[bot], which is what makes the push a GITHUB_TOKEN push. Fixed with the same GitHub App documentation.yml already uses for exactly this reason -- its comment reads "App token (not GITHUB_TOKEN) so the regeneration PR triggers CI and can satisfy develop's required checks" -- with the same two existing repository secrets, BOT_PR_APP_CLIENT_ID and BOT_PR_APP_PRIVATE_KEY. No new secret and no new App permission is required: updateBranch needs contents:write, and upserting a comment needs "Pull requests: write" or "Issues: write" (https://docs.github.com/en/rest/issues/comments), both of which the App already holds. permission-workflows stays absent, as in documentation.yml. The job's own token drops to contents:read so a checkout cannot reach a write-scoped token. The token step is skipped for forks, where repository secrets are not exposed; those pull requests are skipped before any write. --- .github/workflows/keep-pr-current.yml | 50 +++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-) diff --git a/.github/workflows/keep-pr-current.yml b/.github/workflows/keep-pr-current.yml index e86a9ce3b6..111124b317 100644 --- a/.github/workflows/keep-pr-current.yml +++ b/.github/workflows/keep-pr-current.yml @@ -28,6 +28,29 @@ name: Keep Pull Request Current # built), so a conflicting pull request is only ever reached from a push to # `develop`. The `pull_request` trigger exists to cover the opened/reopened and # ready-for-review cases. +# +# Why a GitHub App token and not GITHUB_TOKEN. A branch update is a push, and a +# push made with GITHUB_TOKEN does not leave the new head's checks in a usable +# state. Measured on a scratch pull request on 2026-09-30: after an update the +# new head had ten `pull_request` workflow runs, every one of them +# `action_required`, and therefore no check runs at all — no +# `Route PR template and apply labels`, no `Validate changelog on PR`, no +# `actionlint`, so the pull request sat BLOCKED with its required checks +# missing. GitHub documents exactly this, at +# https://docs.github.com/en/actions/concepts/security/github_token: +# "pull_request events with the opened, synchronize, or reopened activity types: +# when a workflow using GITHUB_TOKEN creates or updates a pull request, the +# resulting pull_request event creates workflow runs in an approval-required +# state." Reproduced twice, on two separate updates. +# +# The App is the same one `.github/workflows/documentation.yml` already uses for +# the same reason — its comment reads "App token (not GITHUB_TOKEN) so the +# regeneration PR triggers CI and can satisfy develop's required checks" — with +# the same two repository secrets. `updateBranch` needs `contents: write`; +# upserting a comment needs `Pull requests: write` or `Issues: write` +# (https://docs.github.com/en/rest/issues/comments), so no additional +# permission is requested. `permission-workflows` is deliberately absent, as in +# documentation.yml: this workflow does not change workflow files. on: push: branches: @@ -39,9 +62,11 @@ on: - ready_for_review workflow_dispatch: +# The job's own GITHUB_TOKEN is read-only: it only needs to check the helper out. +# Every write goes through the App token below, so a compromised checkout cannot +# push with a workflow-scoped write token. permissions: - contents: write - pull-requests: write + contents: read # 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. @@ -71,9 +96,30 @@ jobs: with: node-version-file: '.nvmrc' + # Skipped for pull requests from forks, where repository secrets are not + # exposed. Those are skipped by shouldAttemptUpdate before any write, so + # falling back to the read-only GITHUB_TOKEN below is safe. + - name: Generate GitHub App token + id: app-token + if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + # Same secrets as documentation.yml. Not invented for this workflow. + client-id: ${{ secrets.BOT_PR_APP_CLIENT_ID }} + private-key: ${{ secrets.BOT_PR_APP_PRIVATE_KEY }} + # Least privilege: merge develop into the head branch, and upsert one + # comment. `permission-workflows` is deliberately absent — see the + # header comment. + permission-contents: write + permission-pull-requests: write + - name: Merge develop into open pull requests uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: + # The App token, so the update's push triggers the pull request's + # checks. Falls back to GITHUB_TOKEN (read-only here) only for forks, + # which are never written to. + github-token: ${{ steps.app-token.outputs.token || github.token }} script: | // Relative specifier, matching the other workflows here // (ai-feedback-validation.yml, orchestrate-phase-progression.yml). From 5f849a62af28f999b690eb7cfa073a9764a7baa1 Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 30 Sep 2026 17:50:42 +0200 Subject: [PATCH 10/13] test(mergify): make the update and token assertions mutation-sensitive Three assertions in these files passed on a broken implementation. The upsert test asserted that comment_id was undefined, which is exactly what a call that dropped it would produce, and its fixture comment had no id at all so the two were indistinguishable. Dropping comment_id from the updateComment call left 11 of 11 tests passing. The fixture now carries a real id and the assertion checks for it: the same mutation now fails 1 of 11. The token assertions matched the workflow as text, so the header comment -- which mentions updateBranch, permission-workflows and contents: write while explaining why they are absent -- could satisfy or break them. They now parse the workflow and assert on the parsed step: the token inputs, the permissions granted and not granted, the github-token expression, the fork guard, and that the script body does not call the API itself. Verified by mutation, each reverting one of the three changes above: no app-token step -> 3 failed permission-workflows granted -> 1 failed job token contents: write -> 1 failed --- .../__tests__/keep-pr-current.test.js | 64 +++++++++++++++++-- .../__tests__/process-pull-request.test.js | 8 ++- 2 files changed, 65 insertions(+), 7 deletions(-) diff --git a/scripts/automation/__tests__/keep-pr-current.test.js b/scripts/automation/__tests__/keep-pr-current.test.js index 1aa4046232..28a88d2b14 100644 --- a/scripts/automation/__tests__/keep-pr-current.test.js +++ b/scripts/automation/__tests__/keep-pr-current.test.js @@ -1,4 +1,5 @@ const fs = require('node:fs'); +const YAML = require('yaml'); const path = require('node:path'); const { @@ -263,17 +264,68 @@ describe('keep-pr-current workflow', () => { 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); + // Parsed rather than matched as text: these assertions must not be fooled by + // the header comment, which mentions updateBranch, permission-workflows and + // "contents: write" while explaining why they are absent. Asserting on the + // parsed document means a prose edit can neither satisfy nor break them. + const doc = YAML.parse(workflow); + const job = doc.jobs['keep-current']; + const scriptStep = job.steps.find( + (step) => step.uses && step.uses.startsWith('actions/github-script') + ); + const tokenStep = job.steps.find( + (step) => step.uses && step.uses.startsWith('actions/create-github-app-token') + ); + + // The job's own token only checks the helper out; every write goes through + // the App token. A workflow-scoped `contents: write` would otherwise still + // be available to whatever the checkout brought in. + test('keeps the job token read-only', () => { + expect(doc.permissions).toEqual({ contents: 'read' }); + }); + + // A branch update is a push. A push made with GITHUB_TOKEN leaves the new + // head's pull_request runs in `action_required`, so no required check runs + // appear and the pull request sits BLOCKED. Measured on a scratch pull + // request on 2026-09-30, twice. See the workflow header and + // https://docs.github.com/en/actions/concepts/security/github_token + test('updates branches with a GitHub App token, not GITHUB_TOKEN', () => { + expect(tokenStep).toBeDefined(); + expect(tokenStep.with['client-id']).toBe('${{ secrets.BOT_PR_APP_CLIENT_ID }}'); + expect(tokenStep.with['private-key']).toBe('${{ secrets.BOT_PR_APP_PRIVATE_KEY }}'); + expect(scriptStep.with['github-token']).toBe( + '${{ steps.app-token.outputs.token || github.token }}' + ); + }); + + test('asks the App for only the two permissions it uses', () => { + expect(Object.keys(tokenStep.with).sort()).toEqual([ + 'client-id', + 'permission-contents', + 'permission-pull-requests', + 'private-key', + ]); + expect(tokenStep.with['permission-contents']).toBe('write'); + expect(tokenStep.with['permission-pull-requests']).toBe('write'); + // Never grant permission-workflows to work around a push failure: it would + // let the App change workflow files, which this job has no reason to do. + expect(tokenStep.with['permission-workflows']).toBeUndefined(); + }); + + // Repository secrets are not exposed to workflows from forks, so the token + // step has to be skipped there rather than failing the job. + test('skips the App token step for fork pull requests', () => { + expect(tokenStep.if).toBe( + "github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository" + ); }); // The API call moved into the module so it can be unit tested; the workflow // must not reach back into the API itself. test('delegates the API call to the module rather than duplicating it', () => { - expect(workflow).toContain('processPullRequest'); - expect(workflow).not.toContain('updateBranch'); + expect(scriptStep.with.script).toContain('processPullRequest'); + // The script body must not call the API itself; that belongs in the module. + expect(scriptStep.with.script).not.toContain('updateBranch'); }); // The defect CodeRabbit raised: a log array declared outside the loop put one diff --git a/scripts/automation/__tests__/process-pull-request.test.js b/scripts/automation/__tests__/process-pull-request.test.js index 88870e5fbf..de335ba88b 100644 --- a/scripts/automation/__tests__/process-pull-request.test.js +++ b/scripts/automation/__tests__/process-pull-request.test.js @@ -246,6 +246,10 @@ describe('processPullRequest', () => { test('an existing comment is updated rather than duplicated', async () => { const existing = { + // A real id: without one the fixture cannot tell a correct update from a + // call that forgot `comment_id`, and the assertion below would pass either + // way. + id: 4242, user: { type: 'Bot' }, body: `${CONFLICT_COMMENT_MARKER}\n## Pull request #909 could not be updated automatically\n\nold body\n`, }; @@ -265,7 +269,9 @@ describe('processPullRequest', () => { expect(result.commented).toBe(true); expect(written.created).toHaveLength(0); expect(written.updated).toHaveLength(1); - expect(written.updated[0].comment_id).toBeUndefined(); + // The id of the comment that was found, not an assertion that it is absent. + expect(written.updated[0].comment_id).toBe(4242); + expect(written.updated[0].body).toContain('#909'); }); test('an unchanged comment is left alone', async () => { From 4e977e51d36d73976ca893bed66d7d16d848450f Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 30 Sep 2026 18:55:24 +0200 Subject: [PATCH 11/13] fix(mergify): isolate a comment-write failure to its own pull request paginate, createComment and updateComment were unguarded. The caller loops over every open pull request in one run, so a rejection on the comment path -- a transient 5xx, or a secondary rate limit on the shared token -- escaped processPullRequest, stopped the remaining pull requests, and failed the job, which contradicts the never-red contract this workflow is built on. Reproduced first: with the comment API throwing for pull request 111 while 112 was still to be processed, five tests failed, including the one asserting that the second pull request is still reached. Now 16/16 pass. The conflict itself is still reported. Only the comment is missed, and the miss is visible three ways: a warning annotation, an outcome label in the job summary that reads "conflict (comment not written, HTTP 503)", and commentFailed/commentStatus on the returned result. pulls.get is now split by status. A 404 means the pull request was merged or deleted and is logged as gone; anything else is transient, so it warns and returns retry instead of the previous "unreadable" that logged quietly through info and looked like a clean pass. The workflow's own loop now catches anything that still throws, so a single pull request cannot stop the others even if a future edit leaves a path unguarded. The pulls.list enumeration is guarded for the same reason: it is the one call that decides how many pull requests there are. Also tightens the existing-comment lookup, which matched on body.includes(marker). Other bot comments here are type: Bot and mention this workflow by name -- the Linear review comment carries the branch and workflow names -- so a substring match could adopt and rewrite one of them. Now anchored to the start of the body. Two reviewers have raised this across rounds; the earlier deferral is now closed. --- .../__tests__/process-pull-request.test.js | 105 ++++++++++++++++- scripts/automation/keep-pr-current.cjs | 106 +++++++++++++----- 2 files changed, 179 insertions(+), 32 deletions(-) diff --git a/scripts/automation/__tests__/process-pull-request.test.js b/scripts/automation/__tests__/process-pull-request.test.js index de335ba88b..8e8ab0e3b3 100644 --- a/scripts/automation/__tests__/process-pull-request.test.js +++ b/scripts/automation/__tests__/process-pull-request.test.js @@ -25,7 +25,9 @@ function pullRequest(number, overrides = {}) { * should produce. `comments` is the existing comment set per pull request, and * the client records what was written where. */ -function mockGithub({ results = {}, pulls = {}, comments = {} } = {}) { +function mockGithub({ results = {}, pulls = {}, comments = {}, commentStatus = {} } = {}) { + // `commentStatus` makes one comment call fail for a given pull request, to + // model a transient 5xx or a secondary rate limit on the comment path. const written = { created: [], updated: [] }; const github = { @@ -53,14 +55,27 @@ function mockGithub({ results = {}, pulls = {}, comments = {} } = {}) { }), }, issues: { - listComments: jest.fn(async ({ issue_number: number }) => ({ - data: comments[number] || [], - })), + listComments: jest.fn(async ({ issue_number: number }) => { + if (commentStatus[number]?.listComments) { + throw Object.assign(new Error('boom'), { status: commentStatus[number].listComments }); + } + return { data: comments[number] || [] }; + }), createComment: jest.fn(async (params) => { + if (commentStatus[params.issue_number]?.createComment) { + throw Object.assign(new Error('boom'), { + status: commentStatus[params.issue_number].createComment, + }); + } written.created.push(params); return { data: { id: written.created.length } }; }), updateComment: jest.fn(async (params) => { + if (commentStatus[params.comment_id]?.updateComment) { + throw Object.assign(new Error('boom'), { + status: commentStatus[params.comment_id].updateComment, + }); + } written.updated.push(params); return { data: { id: 1 } }; }), @@ -302,6 +317,88 @@ describe('processPullRequest', () => { expect(second.written.created).toHaveLength(0); }); + // The defect this covers. The caller loops over every open pull request in one + // run, so a rejection on the comment path for one pull request must not stop + // the rest, and must not fail the job: the workflow's contract is that no + // outcome turns a check red. + test('a failing comment call does not stop the next pull request', async () => { + const { github, written } = mockGithub({ + results: { 111: conflict(), 222: conflict() }, + commentStatus: { 111: { createComment: 502 } }, + }); + + const first = await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 111, + logger: silentLogger, + }); + const second = await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 222, + logger: silentLogger, + }); + + // The first still reports the real outcome... + expect(first.outcome).toBe('conflict'); + // ...and did not throw, so the loop reached the second. + expect(second.outcome).toBe('conflict'); + expect(written.created).toHaveLength(1); + expect(written.created[0].body).toContain('#222'); + }); + + test.each([ + ['listComments', 'listing comments'], + ['createComment', 'creating a comment'], + ['updateComment', 'updating a comment'], + ])('a failing %s is reported and does not throw', async (call) => { + const existing = { id: 77, user: { type: 'Bot' }, body: '\nold\n' }; + const warnings = []; + const status = call === 'updateComment' ? 502 : call === 'createComment' ? 429 : 500; + const comments = call === 'updateComment' ? { 333: [existing] } : {}; + const { github } = mockGithub({ + results: { 333: conflict() }, + comments, + commentStatus: { [call === 'updateComment' ? 77 : 333]: { [call]: status } }, + }); + + const result = await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 333, + logger: { info: () => {}, warning: (line) => warnings.push(line) }, + }); + + expect(result.outcome).toBe('conflict'); + expect(result.commented).toBe(false); + expect(warnings.join('\n')).toContain('#333'); + expect(warnings.join('\n')).toContain(String(status)); + }); + + // A failure writing the comment must be visible as more than a log line, so + // the job summary counts it rather than reporting every pull request as done. + test('a failed comment is reflected in the returned result', async () => { + const { github } = mockGithub({ + results: { 444: conflict() }, + commentStatus: { 444: { createComment: 503 } }, + }); + + const result = await processPullRequest({ + github, + owner: 'lightspeedwp', + repo: '.github', + number: 444, + logger: silentLogger, + }); + + expect(result.commentFailed).toBe(true); + expect(result.commentStatus).toBe(503); + }); + test('comments are paginated, so an older marker comment is still found', async () => { const { github } = mockGithub({ results: { 10: conflict() } }); diff --git a/scripts/automation/keep-pr-current.cjs b/scripts/automation/keep-pr-current.cjs index ee4b415c68..df230e0d04 100644 --- a/scripts/automation/keep-pr-current.cjs +++ b/scripts/automation/keep-pr-current.cjs @@ -220,17 +220,33 @@ function normaliseCommentBody(body) { * @param {string} params.repo * @param {number} params.number - Pull request number. * @param {{info: Function, warning: Function}} [params.logger] - actions/core-like. - * @returns {Promise<{outcome: string, status?: number, message?: string, commented?: boolean}>} + * @returns {Promise<{outcome: string, status?: number|null, message?: string, + * commented?: boolean, commentFailed?: boolean, commentStatus?: number|null}>} + * + * Never throws for an API rejection. The caller iterates over every open pull + * request in one run, so any escape would stop the rest of them; each failure is + * returned instead, classified, and raised as a warning. */ async function processPullRequest({ github, owner, repo, number, logger = console }) { let pullRequest; try { ({ data: pullRequest } = await github.rest.pulls.get({ owner, repo, pull_number: number })); } catch (error) { - // Merged or deleted between listing and reading. Nothing to do, and not a - // failure of this workflow. - logger.info(`#${number}: skipped, could not read (${error.status}).`); - return { outcome: 'unreadable', status: error.status, message: error.message || '' }; + const status = error.status ?? null; + if (status === 404) { + // Merged or deleted between listing and reading. Nothing to do, and not a + // failure of this workflow. + logger.info(`#${number}: gone, could not read (404).`); + return { outcome: 'gone', status, message: error.message || '' }; + } + // Anything else — a 5xx, a throttled call — is transient and must be + // visible. It is reported as retryable so the next run tries again, and it + // is raised as a warning so it is not mistaken for a clean pass. + logger.warning( + `#${number}: could not read the pull request (HTTP ${status ?? 'unknown'}` + + `${error.message ? `, ${error.message}` : ''}). Retried on the next run.` + ); + return { outcome: 'retry', status, message: error.message || '' }; } const decision = shouldAttemptUpdate(pullRequest); @@ -278,31 +294,65 @@ async function processPullRequest({ github, owner, repo, number, logger = consol 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. Paginated, so a pull request with more than a page of earlier - // comments still finds its own. - const comments = await github.paginate(github.rest.issues.listComments, { - owner, - repo, - issue_number: number, - per_page: 100, - }); - const existing = comments.find( - (comment) => - comment.user?.type === 'Bot' && String(comment.body || '').includes(CONFLICT_COMMENT_MARKER) - ); + // Every call on the comment path is guarded. The caller loops over every + // open pull request in one run, so a rejection here — a transient 5xx, or a + // secondary rate limit on a shared token — must not escape and stop the + // remaining pull requests, and must not fail the job. The conflict itself + // is still reported; only the comment is missed, and the miss is visible. + try { + // Upsert rather than append, so a pull request that stays conflicting + // across several pushes to develop carries one comment instead of one per + // run. Paginated, so a pull request with more than a page of earlier + // comments still finds its own. + const comments = await github.paginate(github.rest.issues.listComments, { + owner, + repo, + issue_number: number, + per_page: 100, + }); + // Anchored to the start of the body, not `includes`. Other bot comments + // on this repository are `type: Bot` and mention this workflow by name -- + // the Linear review comment carries the branch and workflow names -- so a + // substring match would treat one of those as this workflow's comment and + // rewrite it. Anchoring is used rather than an App-login comparison + // because the App login is not known to this module and would have to be + // threaded in for no extra safety: only a comment this workflow wrote + // starts with the marker. + const existing = comments.find( + (comment) => + comment.user?.type === 'Bot' && + String(comment.body || '') + .trimStart() + .startsWith(CONFLICT_COMMENT_MARKER) + ); - if (existing && normaliseCommentBody(existing.body) === normaliseCommentBody(body)) { - logger.info(`#${number}: conflict comment already up to date.`); - return { outcome, status, message, commented: false }; - } - 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 }); + if (existing && normaliseCommentBody(existing.body) === normaliseCommentBody(body)) { + logger.info(`#${number}: conflict comment already up to date.`); + return { outcome, status, message, commented: false }; + } + 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 }); + } + return { outcome, status, message, commented: true }; + } catch (error) { + // Surfaced three ways so it cannot pass unnoticed: a warning annotation, + // the outcome the caller puts in its summary, and the return value. + logger.warning( + `#${number}: conflict comment could not be written (HTTP ${error.status ?? 'unknown'}` + + `${error.message ? `, ${error.message}` : ''}). The conflict is real and this ` + + 'pull request was not updated; the comment will be retried on the next run.' + ); + return { + outcome, + status, + message, + commented: false, + commentFailed: true, + commentStatus: error.status ?? null, + }; } - return { outcome, status, message, commented: true }; } // Surfaced rather than logged quietly. The job still exits 0, because a From 61d6ad07cbf19a769a7a3028d9530d80f37a25f9 Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 30 Sep 2026 18:55:49 +0200 Subject: [PATCH 12/13] fix(mergify): keep a missing App secret or a failed listing off the red path Four findings from `coderabbit review --agent` on the pending diff, all valid. The App token step gains continue-on-error. Without it, absent or invalid BOT_PR_APP_* secrets fail the job on every run; with it the token is empty, github-token falls back to the read-only GITHUB_TOKEN, the run updates nothing and says so. The pulls.list enumeration is wrapped. It is the one call that decides how many pull requests there are, so leaving it to throw failed the whole job; it now warns and proceeds with an empty list. fetchSchema is bounded. https.get sets no timeout of its own, so a stalled connection left the promise pending forever and the validator hung instead of reporting a failure. The request is destroyed at 30s, which rejects the promise. A periodic `schedule` was considered as a backstop for the gap a `synchronize` push leaves -- a pull request can sit behind develop after its author resolves a conflict -- and rejected on evidence, recorded in the workflow header. With 15 open pull requests, */15 is 96 runs a day and ~1,440 update-branch calls, almost all wasted on the "no new commits" 422. The cost that matters is that develop-branch-ruleset sets dismiss_stale_reviews_on_push: every update moves the head and dismisses the approval, so a fixed cadence would repeatedly void approvals given against an exact head. Scheduled workflows also run only from the default branch and are disabled after 60 days of inactivity, making them the least reliable trigger available. workflow_dispatch is the backstop instead. --- .github/workflows/keep-pr-current.yml | 91 +++++++++++++++++-- .../__tests__/keep-pr-current.test.js | 38 ++++++++ .../validation/validate-mergify-config.cjs | 39 +++++--- 3 files changed, 143 insertions(+), 25 deletions(-) diff --git a/.github/workflows/keep-pr-current.yml b/.github/workflows/keep-pr-current.yml index 111124b317..e6d3c996c0 100644 --- a/.github/workflows/keep-pr-current.yml +++ b/.github/workflows/keep-pr-current.yml @@ -29,6 +29,30 @@ name: Keep Pull Request Current # `develop`. The `pull_request` trigger exists to cover the opened/reopened and # ready-for-review cases. # +# A periodic `schedule` was considered as a backstop and deliberately rejected. +# The gap it would close is real: a pull request can sit behind `develop` after +# its author pushes a conflict resolution, and that push is a `synchronize` +# event, which this workflow ignores. Measured against this repository on +# 2026-09-30, with 15 open pull requests and `develop` moving 218 commits in the +# preceding 7 days: +# +# * `*/15` is 96 runs a day, each enumerating 15 pull requests, so ~1,440 +# `update-branch` calls a day. Almost all resolve to the 422 +# "There are no new commits on the base branch" path, which is one wasted API +# call per pull request per run. +# * The cost that matters is not minutes. `develop-branch-ruleset` sets +# `dismiss_stale_reviews_on_push`, so every branch update moves the head and +# dismisses the approval. A schedule that updated pull requests merely +# because they were behind would churn heads on a fixed cadence and +# repeatedly void approvals that were given against an exact head -- turning +# the strict rule this workflow exists to satisfy into the thing breaking it. +# * `schedule` runs only from the default branch and GitHub disables scheduled +# workflows after 60 days of repository inactivity, so it would also be the +# least reliable of the available triggers. +# +# `workflow_dispatch` is the backstop instead: one click, run on demand, and it +# changes nothing that was not already stale. +# # Why a GitHub App token and not GITHUB_TOKEN. A branch update is a push, and a # push made with GITHUB_TOKEN does not leave the new head's checks in a usable # state. Measured on a scratch pull request on 2026-09-30: after an update the @@ -101,6 +125,11 @@ jobs: # falling back to the read-only GITHUB_TOKEN below is safe. - name: Generate GitHub App token id: app-token + # continue-on-error: if the App secrets are absent or invalid, the token + # is empty and github-token falls back to GITHUB_TOKEN, which is + # read-only. The run then updates nothing and says so, rather than + # failing every pull request's check over a missing secret. + continue-on-error: true if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 with: @@ -133,14 +162,26 @@ jobs: 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); + // Guarded so a listing failure cannot fail the job. The + // per-pull-request work is untouched, because this is the one call + // that decides how many pull requests there are. + try { + const open = await github.paginate(github.rest.pulls.list, { + owner, + repo, + state: 'open', + base: 'develop', + per_page: 100, + }); + numbers = open.map((item) => item.number); + } catch (error) { + const status = error?.status ?? 'unknown'; + core.warning( + `Could not list open pull requests (${status}): ${error?.message}. ` + + 'Nothing was updated; retry with workflow_dispatch.' + ); + numbers = []; + } } core.info(`Considering ${numbers.length} pull request(s) against develop.`); @@ -149,12 +190,42 @@ jobs: // which owns its own log lines and its own comment. Nothing is // carried between iterations. const summary = []; + const problems = []; for (const number of numbers) { - const result = await processPullRequest({ github, owner, repo, number, logger: core }); - summary.push(`#${number}: ${result.outcome}`); + // Belt and braces. processPullRequest handles every API rejection + // itself, but a throw here would stop every later pull request in + // the run, so the loop never depends on that remaining true. + let result; + try { + result = await processPullRequest({ github, owner, repo, number, logger: core }); + } catch (error) { + const status = error?.status ?? 'unknown'; + core.warning(`#${number}: unexpected failure (${status}): ${error?.message}`); + result = { outcome: 'error', status, message: error?.message || '' }; + } + // A conflict whose comment could not be written is a problem worth + // counting, not a completed pull request. + const label = result.commentFailed + ? `${result.outcome} (comment not written, HTTP ${result.commentStatus})` + : result.outcome; + summary.push(`#${number}: ${label}`); + if (result.commentFailed || ['error', 'retry', 'denied'].includes(result.outcome)) { + problems.push(`#${number}: ${label}`); + } } core.info(`Processed ${numbers.length} pull request(s).`); + + if (problems.length) { + // Visible on the run page. The job still exits 0, because a + // repository or permission problem must not turn every pull + // request's check red, but it must not look like a clean pass + // either. + core.warning( + `${problems.length} of ${numbers.length} pull request(s) were not fully processed: ` + + `${problems.join('; ')}` + ); + } await core.summary .addHeading('Keep pull request current') .addTable([ diff --git a/scripts/automation/__tests__/keep-pr-current.test.js b/scripts/automation/__tests__/keep-pr-current.test.js index 28a88d2b14..2c89abc8d2 100644 --- a/scripts/automation/__tests__/keep-pr-current.test.js +++ b/scripts/automation/__tests__/keep-pr-current.test.js @@ -339,6 +339,44 @@ describe('keep-pr-current workflow', () => { expect(workflow).toMatch(/uses:\s*actions\/github-script@/); }); + // A bot comment that merely mentions this workflow must not be adopted as its + // own. The Linear review comment is type: Bot and carries the branch and + // workflow names, so a substring match would rewrite it. + test('only claims a comment that starts with the marker', () => { + // This lives in the module, not the workflow: the marker match decides which + // comment is this workflow's, and a substring match would adopt any bot + // comment mentioning it. + const module = fs.readFileSync( + path.join(repositoryRoot, 'scripts/automation/keep-pr-current.cjs'), + 'utf8' + ); + expect(module).toContain('.startsWith(CONFLICT_COMMENT_MARKER)'); + expect(module).not.toMatch(/includes\(CONFLICT_COMMENT_MARKER\)/); + }); + + // A missing or invalid App secret must not turn every pull request's check + // red: the run falls back to the read-only GITHUB_TOKEN and updates nothing. + test('survives the App token step failing', () => { + expect(tokenStep['continue-on-error']).toBe(true); + }); + + // The enumeration is the one call that decides how many pull requests there + // are, so it cannot be left to throw. + test('guards the pull request enumeration', () => { + expect(scriptStep.with.script).toMatch( + /try\s*\{[\s\S]*?github\.paginate\(github\.rest\.pulls\.list/ + ); + expect(scriptStep.with.script).toContain('Could not list open pull requests'); + }); + + // Every API call in the script body must be inside a try/catch or delegated + // to the module, so no rejection can escape the loop. + test('the script body delegates all API work to the module', () => { + expect(scriptStep.with.script).toContain('processPullRequest'); + expect(scriptStep.with.script).not.toContain('updateBranch'); + expect(scriptStep.with.script).not.toContain('listComments'); + }); + test('pins every action to a commit', () => { const uses = workflow.match(/uses:\s*\S+/g) || []; diff --git a/scripts/validation/validate-mergify-config.cjs b/scripts/validation/validate-mergify-config.cjs index 9cd10af288..c7102d8c48 100644 --- a/scripts/validation/validate-mergify-config.cjs +++ b/scripts/validation/validate-mergify-config.cjs @@ -25,23 +25,32 @@ const SCHEMA_URL = 'https://docs.mergify.com/mergify-configuration-schema.json'; const repositoryRoot = path.resolve(__dirname, '../..'); const configPath = path.join(repositoryRoot, '.github/mergify.yml'); +const SCHEMA_FETCH_TIMEOUT_MS = 30_000; + function fetchSchema(url) { return new Promise((resolve, reject) => { - https - .get(url, (response) => { - if (response.statusCode !== 200) { - reject(new Error(`GET ${url} responded ${response.statusCode}`)); - response.resume(); - return; - } - response.setEncoding('utf8'); - let body = ''; - response.on('data', (chunk) => { - body += chunk; - }); - response.on('end', () => resolve(body)); - }) - .on('error', reject); + // Bounded, and the request is destroyed on timeout. https.get sets no + // timeout of its own, so without this a stalled connection leaves the + // promise pending forever and the validator hangs rather than reporting a + // failure. + const request = https.get(url, (response) => { + if (response.statusCode !== 200) { + request.destroy(new Error(`GET ${url} responded ${response.statusCode}`)); + response.resume(); + return; + } + response.setEncoding('utf8'); + let body = ''; + response.on('data', (chunk) => { + body += chunk; + }); + response.on('end', () => resolve(body)); + }); + + request.setTimeout(SCHEMA_FETCH_TIMEOUT_MS, () => { + request.destroy(new Error(`GET ${url} timed out after ${SCHEMA_FETCH_TIMEOUT_MS}ms`)); + }); + request.on('error', reject); }); } From a70aa30d4c2366a8353d8bb9040d16693e297021 Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 30 Sep 2026 19:05:13 +0200 Subject: [PATCH 13/13] docs(mergify): correct the synchronize loop claim An already-current result makes no further push, so a synchronize-triggered run would terminate rather than loop. The reason for excluding it stands -- each update is itself a push and would double the runs for no gain -- but the loop claim was wrong and is corrected, with the de-confliction gap named instead. Raised in review. --- .github/workflows/keep-pr-current.yml | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/.github/workflows/keep-pr-current.yml b/.github/workflows/keep-pr-current.yml index e6d3c996c0..74e13e8e5e 100644 --- a/.github/workflows/keep-pr-current.yml +++ b/.github/workflows/keep-pr-current.yml @@ -18,10 +18,13 @@ name: Keep Pull Request Current # and cannot be updated is still blocked from merging. Nothing here relaxes the # ruleset, and this workflow is deliberately not a required check. # -# `synchronize` is deliberately absent from the `pull_request` types. Updating a -# branch is itself a push to it, so including `synchronize` would make the -# update re-trigger this workflow and loop. Branches fall behind because -# `develop` moves, which is why `push` is the trigger that matters. +# `synchronize` is deliberately absent from the `pull_request` types, not because +# it would loop -- an already-current result makes no further push, so the chain +# would terminate -- but because every update is itself a push and would double the +# runs for no gain. Branches fall behind because `develop` moves, which is why +# `push` is the trigger that matters. What `synchronize` would additionally cover +# is a pull request its author has just de-conflicted; that gap is accepted, see +# the schedule decision below. # # GitHub cannot run `pull_request` workflows at all on a pull request that # conflicts (`refs/pull//merge` is 404 because the merge ref cannot be