diff --git a/.github/workflows/keep-pr-current.yml b/.github/workflows/keep-pr-current.yml new file mode 100644 index 0000000000..e6d3c996c0 --- /dev/null +++ b/.github/workflows/keep-pr-current.yml @@ -0,0 +1,238 @@ +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. +# +# `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. +# +# 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 +# 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: + - develop + pull_request: + types: + - opened + - reopened + - 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: 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. +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' + + # 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 + # 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: + # 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). + const { processPullRequest } = require('./scripts/automation/keep-pr-current.cjs'); + + const { owner, repo } = context.repo; + + // 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 { + // 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.`); + + // 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 = []; + const problems = []; + for (const number of numbers) { + // 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([ + [ + { 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/docs/BRANCHING_STRATEGY.md b/docs/BRANCHING_STRATEGY.md index a19c9c7df1..b8fdc7872b 100644 --- a/docs/BRANCHING_STRATEGY.md +++ b/docs/BRANCHING_STRATEGY.md @@ -1176,3 +1176,5 @@ This section provides comprehensive details for each of the 24 allowed branch ty > For questions or improvements, open an issue or PR in the `.github` repo. *Have questions? Ping us on GitHub! 🐙 Made with 💚 by LightSpeedWP* + + diff --git a/scripts/automation/keep-pr-current.cjs b/scripts/automation/keep-pr-current.cjs new file mode 100644 index 0000000000..df230e0d04 --- /dev/null +++ b/scripts/automation/keep-pr-current.cjs @@ -0,0 +1,378 @@ +/** + * 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, or takes an injected + * Octokit-like client, so the behaviour is testable without a network round trip. + */ + +/** + * Marker used to find and update this workflow's own comment, so repeated runs + * on the same pull request do not stack up duplicates. Deliberately distinct + * from the AI Feedback Validation marker and from the PR template routing + * comment's `## PR Template Routing` heading. + */ +const CONFLICT_COMMENT_MARKER = ''; + +/** + * Should the base branch be merged into this pull request's head branch? + * + * The exclusions mirror what the Mergify rule used to express, and Mergify's + * own documented preconditions (https://docs.mergify.com/workflow/actions/update: + * "A pull request is updated only when it is open, has no conflict, is not in a + * merge queue, and is behind its base branch"). + * + * `mergeableState` is advisory only and never blocks the attempt. GitHub + * reports `unknown` for a short window after the base branch moves, and + * treating that window as "conflicting" would strand every open pull request + * on every push to `develop`. The update API is the real test: it either + * merges, or it reports a conflict, and the conflict path is reported as + * information rather than as a failure. + * + * @param {object} pullRequest - A `pulls.get` response. + * @returns {{attempt: boolean, reason: string}} + */ +function shouldAttemptUpdate(pullRequest) { + if (!pullRequest || pullRequest.state !== 'open') { + return { attempt: false, reason: 'not-open' }; + } + + if (pullRequest.draft) { + return { attempt: false, reason: 'draft' }; + } + + const headOwner = pullRequest.head?.repo?.owner?.login; + const baseOwner = pullRequest.base?.repo?.owner?.login; + if (headOwner && baseOwner && headOwner !== baseOwner) { + return { attempt: false, reason: 'from-fork' }; + } + + if (pullRequest.head?.repo?.archived) { + return { attempt: false, reason: 'archived-head' }; + } + + if (pullRequest.base?.ref && pullRequest.base.ref !== 'develop') { + return { attempt: false, reason: 'not-develop' }; + } + + return { attempt: true, reason: 'behind' }; +} + +/** + * Classify the result of `PUT /repos/{owner}/{repo}/pulls/{pull_number}/update-branch`. + * + * 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`: + * + * | 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 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. + * + * 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'|'gone'|'denied'|'retry'|'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'; + } + + // 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'; + } + if (text.includes('merge conflict between base and head')) { + return 'conflict'; + } + if (text.includes('head ref does not exist')) { + 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. 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 'denied'; + } + + // 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'; +} + +/** + * 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] - Log lines for this pull request only. + * @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(); +} + +/** + * 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|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) { + 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); + 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, + }); + + // 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 }); + } + 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, + }; + } + } + + // 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, +};