From 5dffad16889ef0d94392d3c3548cc9b7a4587fa9 Mon Sep 17 00:00:00 2001 From: Richard McDaniel <1130888+rmcdaniel@users.noreply.github.com> Date: Thu, 12 Nov 2026 19:19:15 +0000 Subject: [PATCH 1/2] CI: skip queue suites already passed on the exact same merge (#1130) A merge-queue group built straight onto the base its pull request run was tested on merges the same two parents into the same tree. When that pull request run passed, PHPUnit Feature now skips only its feature suite and Shared Server fixtures skips its fixture jobs. Repository contracts, the central action policy, Public Boundary and the Sonar queue analysis still run on every group. A passing same-repository pull request run records a marker artifact named for the workflow, the merge's first parent and its tree. The group checks that a marker exists for its own first parent and tree, and that the run that uploaded it is a completed, successful pull_request run of the same workflow file, from this repository (no forks), on exactly the group's second parent. Groups stacked on a queued predecessor, forks, failures and any API doubt run every suite. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/phpunit-feature.yml | 80 +++- .github/workflows/server-parity.yml | 43 ++- scripts/ci/tested-merge.mjs | 132 +++++++ scripts/ci/tested-merge.sh | 5 + .../conformance/server-parity/aggregate.mjs | 8 +- .../PhpunitFeatureWorkflowContractTest.php | 6 +- tests/Unit/ServerParityAggregateTest.mjs | 21 +- tests/Unit/TestedMergeTest.mjs | 355 ++++++++++++++++++ 8 files changed, 639 insertions(+), 11 deletions(-) create mode 100644 scripts/ci/tested-merge.mjs create mode 100755 scripts/ci/tested-merge.sh create mode 100644 tests/Unit/TestedMergeTest.mjs diff --git a/.github/workflows/phpunit-feature.yml b/.github/workflows/phpunit-feature.yml index 244ebed7b..106bdfaa7 100644 --- a/.github/workflows/phpunit-feature.yml +++ b/.github/workflows/phpunit-feature.yml @@ -112,7 +112,7 @@ jobs: run: node --test tests/Unit/PatchCohortMaintenanceTest.mjs - name: Run the required checks for merge queue groups - run: node --test tests/Unit/MergeGroupTriggersTest.mjs tests/Unit/MergeQueueSonarTest.mjs + run: node --test tests/Unit/MergeGroupTriggersTest.mjs tests/Unit/MergeQueueSonarTest.mjs tests/Unit/TestedMergeTest.mjs - name: Report one Shared Server fixtures result on every pull request run: node --test tests/Unit/ServerParityAggregateTest.mjs tests/Unit/ServerParityPartsTest.mjs @@ -192,11 +192,54 @@ jobs: - name: Require all PHP replay runtime cells and exact artifact identity run: python3 -m unittest tests/Unit/Support/php_replay_published_test.py tests/Unit/Support/php_condition_wait_test.py + # A merge-queue group built straight onto the base a pull request run was + # tested on merges the same two parents into the same tree. When that pull + # request run passed, the queue does not repeat the feature suite (Server + # #1130). Repository contracts and the central action policy still run on + # every group, and any doubt runs the suite. See scripts/ci/tested-merge.mjs. + tested-merge: + name: Tested merge + runs-on: ubuntu-latest + timeout-minutes: 3 + # Only the merge-group check reads workflow runs and artifacts. + permissions: + contents: read + actions: read + outputs: + marker: ${{ steps.record.outputs.marker }} + tested: ${{ steps.check.outputs.tested }} + steps: + - name: Check out the merge and its parents + uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 + with: + fetch-depth: 2 + persist-credentials: false + sparse-checkout: | + scripts/ci/tested-merge.sh + scripts/ci/tested-merge.mjs + sparse-checkout-cone-mode: false + + - name: Name the marker a passing pull request run records + id: record + if: ${{ github.event_name == 'pull_request' }} + env: + PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: bash scripts/ci/tested-merge.sh record phpunit-feature .github/workflows/phpunit-feature.yml >> "$GITHUB_OUTPUT" + + - name: Find a passing pull request run of this exact merge + id: check + if: ${{ github.event_name == 'merge_group' }} + env: + MERGE_GROUP_HEAD_SHA: ${{ github.event.merge_group.head_sha }} + RUNS_TOKEN: ${{ github.token }} + run: bash scripts/ci/tested-merge.sh check phpunit-feature .github/workflows/phpunit-feature.yml >> "$GITHUB_OUTPUT" + feature: name: PHPUnit feature suite - needs: action-policy - # Report policy failures while allowing cancelled revisions to stop. - if: ${{ always() && !cancelled() }} + needs: [action-policy, tested-merge] + # Report policy failures while allowing cancelled revisions to stop. A + # failed tested-merge check reports nothing, so the suite runs. + if: ${{ always() && !cancelled() && needs.tested-merge.outputs.tested != 'true' }} runs-on: ubuntu-latest timeout-minutes: 25 steps: @@ -490,7 +533,7 @@ jobs: qualification: name: Feature source qualification - needs: [preflight, feature] + needs: [preflight, tested-merge, feature] if: ${{ always() && !cancelled() }} runs-on: ubuntu-latest timeout-minutes: 2 @@ -498,14 +541,41 @@ jobs: steps: - name: Require repository contracts and feature tests env: + EVENT_NAME: ${{ github.event_name }} PREFLIGHT_RESULT: ${{ needs.preflight.result }} FEATURE_RESULT: ${{ needs.feature.result }} + TESTED_MERGE: ${{ needs.tested-merge.outputs.tested }} run: | set -euo pipefail test "$PREFLIGHT_RESULT" = success + if [ "$EVENT_NAME" = merge_group ] && [ "$TESTED_MERGE" = true ] && [ "$FEATURE_RESULT" = skipped ]; then + echo "Server repository contracts succeeded; the feature tests already passed on this exact merge in its pull request run." + exit 0 + fi test "$FEATURE_RESULT" = success echo "Server repository contracts and feature tests succeeded." + # Only after every check above passed. An upload failure only means the + # queue runs the suite again. + - name: Write the tested-merge marker + if: ${{ success() && github.event_name == 'pull_request' && needs.tested-merge.outputs.marker != '' }} + env: + MARKER: ${{ needs.tested-merge.outputs.marker }} + run: | + mkdir -p "$RUNNER_TEMP/tested-merge" + printf '%s\n' "$MARKER" > "$RUNNER_TEMP/tested-merge/marker.txt" + + - name: Record that this exact merge passed + if: ${{ success() && github.event_name == 'pull_request' && needs.tested-merge.outputs.marker != '' }} + continue-on-error: true + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 + with: + name: ${{ needs.tested-merge.outputs.marker }} + path: ${{ runner.temp }}/tested-merge/marker.txt + if-no-files-found: error + retention-days: 7 + overwrite: true + prepared-local-source: name: Candidate cancellation and prepared-local source qualification needs: [action-policy, preflight] diff --git a/.github/workflows/server-parity.yml b/.github/workflows/server-parity.yml index af4f34272..cef777ce3 100644 --- a/.github/workflows/server-parity.yml +++ b/.github/workflows/server-parity.yml @@ -169,13 +169,17 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 5 # Reading this workflow's own runs lets a push to main that the merge queue - # already qualified skip repeating that qualification. + # already qualified skip repeating that qualification. Reading runs and + # artifacts lets a merge group that is exactly a merge a pull request run + # already passed skip repeating it (Server #1130). permissions: contents: read actions: read outputs: - run: ${{ steps.classify.outputs.run }} + run: ${{ steps.tested.outputs.tested == 'true' && 'false' || steps.classify.outputs.run }} full: ${{ steps.classify.outputs.full }} + tested: ${{ steps.tested.outputs.tested }} + marker: ${{ steps.record.outputs.marker }} steps: - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 with: @@ -183,6 +187,22 @@ jobs: persist-credentials: false - name: Reject unsafe Docker ownership before starting fixture jobs run: python3 scripts/ci/check-docker-workflow-isolation.py + # The result job records this name only after every fixture job passed. + - name: Name the marker a passing pull request run records + id: record + if: ${{ github.event_name == 'pull_request' }} + env: + PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: bash scripts/ci/tested-merge.sh record server-parity .github/workflows/server-parity.yml >> "$GITHUB_OUTPUT" + # Same parents and tree as a passing pull request run: nothing new to + # qualify. Any doubt reports tested=false and the classifier decides. + - name: Find a passing pull request run of this exact merge + id: tested + if: ${{ github.event_name == 'merge_group' }} + env: + MERGE_GROUP_HEAD_SHA: ${{ github.event.merge_group.head_sha }} + RUNS_TOKEN: ${{ github.token }} + run: bash scripts/ci/tested-merge.sh check server-parity .github/workflows/server-parity.yml >> "$GITHUB_OUTPUT" - name: Decide whether fixture inputs changed id: classify env: @@ -3006,3 +3026,22 @@ jobs: env: NEEDS: ${{ toJSON(needs) }} run: bash scripts/conformance/server-parity/aggregate.sh + # Only after the result passed. An upload failure only means the queue + # runs the fixtures again. + - name: Write the tested-merge marker + if: ${{ success() && github.event_name == 'pull_request' && needs.changes.outputs.marker != '' }} + env: + MARKER: ${{ needs.changes.outputs.marker }} + run: | + mkdir -p "$RUNNER_TEMP/tested-merge" + printf '%s\n' "$MARKER" > "$RUNNER_TEMP/tested-merge/marker.txt" + - name: Record that this exact merge passed + if: ${{ success() && github.event_name == 'pull_request' && needs.changes.outputs.marker != '' }} + continue-on-error: true + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 + with: + name: ${{ needs.changes.outputs.marker }} + path: ${{ runner.temp }}/tested-merge/marker.txt + if-no-files-found: error + retention-days: 7 + overwrite: true diff --git a/scripts/ci/tested-merge.mjs b/scripts/ci/tested-merge.mjs new file mode 100644 index 000000000..8abe1b159 --- /dev/null +++ b/scripts/ci/tested-merge.mjs @@ -0,0 +1,132 @@ +#!/usr/bin/env node +// Lets a merge-queue group skip a suite that already passed on exactly the +// same merge (Server #1130). +// +// A pull request run checks out GitHub's test merge of the pull request head +// into its base. When a merge-queue group is built straight onto that same +// base, the group commit merges the same two parents and has the same tree: +// the files, workflows and base revisions the suite sees are identical, so +// running it again proves nothing new. +// +// `record` runs in the pull request run. It names a marker artifact after the +// workflow, the merge's first parent and its tree. The workflow uploads the +// marker only after its suites passed. +// +// `check` runs in the merge group. It skips only when all of these hold: +// - the checkout is this merge group's head commit, a two-parent merge; +// - a marker exists for this workflow, this group's first parent and tree; +// - the run that uploaded it is a completed, successful `pull_request` run +// of this same workflow file, in this repository, from a branch in this +// repository (never a fork), on exactly the group's second parent. +// A group stacked on an unmerged predecessor has a first parent no pull +// request was tested on, so it runs everything. Any other doubt, error or +// missing permission runs everything. +import {execFileSync} from 'node:child_process'; +import {pathToFileURL} from 'node:url'; + +const SHA = /^[0-9a-f]{40}$/; +const KEY = /^[a-z0-9-]{1,40}$/; + +export function markerName(key, base, tree) { + if (!KEY.test(key ?? '')) throw new Error('Invalid workflow key'); + if (!SHA.test(base ?? '') || !SHA.test(tree ?? '')) throw new Error('Invalid merge identity'); + return `tested-merge-${key}-${base}-${tree}`; +} + +// The checked-out merge commit: its two parents and its tree. Git runs +// without a shell, from the current directory. +export function localMerge(git = (...args) => execFileSync('git', args, {encoding: 'utf8'})) { + const [commit, ...parents] = git('rev-list', '--parents', '-n', '1', 'HEAD').trim().split(' '); + const tree = git('rev-parse', 'HEAD^{tree}').trim(); + if (!SHA.test(commit) || parents.length !== 2 || !parents.every(parent => SHA.test(parent)) || !SHA.test(tree)) { + throw new Error('HEAD is not a two-parent merge commit'); + } + return {commit, base: parents[0], head: parents[1], tree}; +} + +// The marker this pull request run may record, or an error. +export function recordName({key, prHeadSha, merge}) { + if (!SHA.test(prHeadSha ?? '') || merge.head !== prHeadSha) { + throw new Error('HEAD is not this pull request\'s test merge'); + } + return markerName(key, merge.base, merge.tree); +} + +// Whether a successful pull request run of `workflowPath` already tested this +// merge group's exact merge. Returns {tested, reason}; every doubt is false. +export async function testedRun({key, workflowPath, merge, mergeGroupHeadSha, repository, repositoryId, token, + apiUrl = 'https://api.github.com', fetchImpl = fetch}) { + const no = reason => ({tested: false, reason}); + if (!SHA.test(mergeGroupHeadSha ?? '') || merge?.commit !== mergeGroupHeadSha) return no('HEAD is not this merge group'); + if (!/^\.github\/workflows\/[a-z0-9-]+\.yml$/.test(workflowPath ?? '')) return no('invalid workflow path'); + if (!/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(repository ?? '') || !/^[1-9]\d*$/.test(String(repositoryId ?? ''))) { + return no('invalid repository identity'); + } + if (!token) return no('no token to read workflow runs'); + if (!/^https:\/\/[A-Za-z0-9.-]+(?::\d+)?(?:\/[A-Za-z0-9._\/-]*)?$/.test(apiUrl)) return no('invalid API URL'); + let name; + try { + name = markerName(key, merge.base, merge.tree); + } catch (error) { + return no(error.message); + } + const get = async path => { + const response = await fetchImpl(`${apiUrl}/repos/${repository}${path}`, { + headers: {authorization: `Bearer ${token}`, accept: 'application/vnd.github+json', 'x-github-api-version': '2022-11-28'}, + signal: AbortSignal.timeout(30_000), + }); + if (!response.ok) throw new Error(`GitHub API answered ${response.status}`); + return response.json(); + }; + const id = Number(repositoryId); + try { + const listing = await get(`/actions/artifacts?${new URLSearchParams({name, per_page: '100'})}`); + const candidates = (Array.isArray(listing?.artifacts) ? listing.artifacts : []).filter(artifact => artifact?.name === name + && artifact?.expired === false && artifact?.workflow_run?.head_sha === merge.head + && Number.isSafeInteger(artifact?.workflow_run?.id) && artifact.workflow_run.id > 0); + for (const runId of new Set(candidates.map(artifact => artifact.workflow_run.id))) { + const run = await get(`/actions/runs/${runId}`); + if (run?.id === runId && run?.event === 'pull_request' && run?.status === 'completed' && run?.conclusion === 'success' + && run?.path === workflowPath && run?.head_sha === merge.head + && run?.repository?.id === id && run?.head_repository?.id === id) { + return {tested: true, runId, reason: `pull request run ${runId} already passed this exact merge (${name})`}; + } + } + return no(`no successful same-repository pull request run recorded ${name}`); + } catch { + // Network errors may echo request details; the decision needs none. + return no('workflow runs could not be read'); + } +} + +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + const [mode, key, workflowPath] = process.argv.slice(2); + const env = process.env; + if (mode === 'record') { + // Never fails the suite: without a marker the queue simply runs it again. + try { + const name = recordName({key, prHeadSha: env.PR_HEAD_SHA, merge: localMerge()}); + console.error(`A passing run records ${name}`); + console.log(`marker=${name}`); + } catch (error) { + console.error(`No tested-merge marker: ${error.message}`); + console.log('marker='); + } + } else if (mode === 'check') { + let decision; + try { + decision = await testedRun({ + key, workflowPath, merge: localMerge(), mergeGroupHeadSha: env.MERGE_GROUP_HEAD_SHA, + repository: env.GITHUB_REPOSITORY, repositoryId: env.GITHUB_REPOSITORY_ID, token: env.RUNS_TOKEN, + apiUrl: env.GITHUB_API_URL ?? 'https://api.github.com', + }); + } catch (error) { + decision = {tested: false, reason: error.message}; + } + console.error(decision.tested ? `Skipping: ${decision.reason}` : `Running the suite: ${decision.reason}`); + console.log(`tested=${decision.tested}`); + } else { + console.error('Usage: tested-merge.mjs record|check '); + process.exitCode = 2; + } +} diff --git a/scripts/ci/tested-merge.sh b/scripts/ci/tested-merge.sh new file mode 100755 index 000000000..5154d7cdb --- /dev/null +++ b/scripts/ci/tested-merge.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash +# Prints the tested-merge decision for $GITHUB_OUTPUT, from the repository +# root. The rules live in tested-merge.mjs. +set -euo pipefail +exec node scripts/ci/tested-merge.mjs "$@" diff --git a/scripts/conformance/server-parity/aggregate.mjs b/scripts/conformance/server-parity/aggregate.mjs index d1e746fcc..14616ceb3 100644 --- a/scripts/conformance/server-parity/aggregate.mjs +++ b/scripts/conformance/server-parity/aggregate.mjs @@ -3,7 +3,8 @@ // context as JSON. // // Passes only when every fixture job passed, or when the classifier reported -// that no fixture input changed and every fixture job was skipped for that +// that no fixture input changed, or that a pull request run already passed +// this exact merge (tested=true), and every fixture job was skipped for that // reason. A push to main the merge queue already qualified (full=false) // passes when exactly the cache-saving jobs passed and every other job was // skipped. A failed, cancelled or unexpectedly skipped job fails the result, @@ -60,7 +61,10 @@ export function decide(needs) { } return { pass: true, - reason: run === 'true' ? `Every fixture job passed: ${summary}.` : `No fixture input changed; every fixture job was skipped: ${summary}.`, + reason: run === 'true' ? `Every fixture job passed: ${summary}.` + : changes.outputs?.tested === 'true' + ? `A pull request run already passed this exact merge; every fixture job was skipped: ${summary}.` + : `No fixture input changed; every fixture job was skipped: ${summary}.`, }; } diff --git a/tests/Unit/PhpunitFeatureWorkflowContractTest.php b/tests/Unit/PhpunitFeatureWorkflowContractTest.php index 1f4fd37b4..ebb0f7612 100644 --- a/tests/Unit/PhpunitFeatureWorkflowContractTest.php +++ b/tests/Unit/PhpunitFeatureWorkflowContractTest.php @@ -211,10 +211,14 @@ public function test_final_gate_requires_preflight_and_feature_suite(): void foreach ([ 'qualification:', 'name: Feature source qualification', - 'needs: [preflight, feature]', + 'needs: [preflight, tested-merge, feature]', 'if: ${{ always() }}', 'test "$PREFLIGHT_RESULT" = success', 'test "$FEATURE_RESULT" = success', + // Only a merge group a pull request run already passed exactly + // (tests/Unit/TestedMergeTest.mjs) may skip the feature suite. + 'if [ "$EVENT_NAME" = merge_group ] && [ "$TESTED_MERGE" = true ] && [ "$FEATURE_RESULT" = skipped ]; then', + "if: \${{ always() && !cancelled() && needs.tested-merge.outputs.tested != 'true' }}", ] as $needle) { $this->assertStringContainsString($needle, $this->workflow); } diff --git a/tests/Unit/ServerParityAggregateTest.mjs b/tests/Unit/ServerParityAggregateTest.mjs index 718cb16d7..b707ae2db 100644 --- a/tests/Unit/ServerParityAggregateTest.mjs +++ b/tests/Unit/ServerParityAggregateTest.mjs @@ -166,7 +166,10 @@ test('the fixture jobs run only through the classifier and the result waits for const ids = Object.keys(blocks); assert.equal(ids[0], 'changes'); assert.match(blocks.changes, /^ {4}timeout-minutes: \d+$/m); - assert.match(blocks.changes, /run: \$\{\{ steps\.classify\.outputs\.run \}\}/); + // A merge group a pull request run already passed exactly skips the fixture + // jobs; TestedMergeTest.mjs covers that decision. + assert.ok(blocks.changes.split('\n').includes( + " run: ${{ steps.tested.outputs.tested == 'true' && 'false' || steps.classify.outputs.run }}")); assert.match(blocks.changes, /run: bash scripts\/conformance\/server-parity\/changes\.sh >> "\$GITHUB_OUTPUT"$/m); assert.match(blocks.changes, /full: \$\{\{ steps\.classify\.outputs\.full \}\}/); // The root fixture jobs run only through the classifier. Only the cache @@ -235,6 +238,22 @@ const fixture = (result, run) => ({ 'execution-and-codec': {result, outputs: {}}, }); +test('the result names an exact tested merge as the reason every fixture job was skipped', () => { + const skipped = fixture('skipped', 'false'); + skipped.changes.outputs.tested = 'true'; + assert.match(decide(skipped).reason, /^A pull request run already passed this exact merge; every fixture job was skipped/); + assert.equal(decide(skipped).pass, true); + // Being tested never excuses a fixture job that ran and failed or one that + // was skipped while the classifier asked for it. + const ran = fixture('skipped', 'true'); + ran.changes.outputs.tested = 'true'; + assert.equal(decide(ran).pass, false); + const failed = fixture('skipped', 'false'); + failed.changes.outputs.tested = 'true'; + failed['mysql-execution'] = {result: 'failure', outputs: {}}; + assert.equal(decide(failed).pass, false); +}); + test('the result passes when every fixture job passed', () => { const outcome = runAggregate(fixture('success', 'true')); assert.equal(outcome.status, 0, outcome.stderr); diff --git a/tests/Unit/TestedMergeTest.mjs b/tests/Unit/TestedMergeTest.mjs new file mode 100644 index 000000000..bcc09cb8d --- /dev/null +++ b/tests/Unit/TestedMergeTest.mjs @@ -0,0 +1,355 @@ +// A merge-queue group skips PHPUnit Feature and the Shared Server fixtures +// only when a successful same-repository pull request run of the same +// workflow already passed the exact same merge: same two parents, same tree +// (Server #1130). Everything else runs the suites. +import test from 'node:test'; +import assert from 'node:assert/strict'; +import {execFileSync, spawnSync} from 'node:child_process'; +import {mkdtempSync, readFileSync, rmSync, writeFileSync} from 'node:fs'; +import {tmpdir} from 'node:os'; +import {join, resolve} from 'node:path'; +import {localMerge, markerName, recordName, testedRun} from '../../scripts/ci/tested-merge.mjs'; + +const script = resolve('scripts/ci/tested-merge.mjs'); +const workflow = name => readFileSync(`.github/workflows/${name}`, 'utf8'); + +// Top-level job ids and each job's block of text. +function jobs(text) { + const body = text.slice(text.indexOf('\njobs:\n') + '\njobs:\n'.length); + const blocks = {}; + let current; + for (const line of body.split('\n')) { + const header = /^ {2}([a-z0-9-]+):$/.exec(line); + if (header) { + current = header[1]; + blocks[current] = ''; + } else if (current) { + blocks[current] += `${line}\n`; + } + } + return blocks; +} + +// A repository with main, a pull request head, GitHub's test merge of the +// head into main, and a later queue-group merge of the same two parents. +function repository() { + const root = mkdtempSync(join(tmpdir(), 'tested-merge-')); + const git = (...args) => execFileSync('git', ['-C', root, ...args], {encoding: 'utf8'}).trim(); + git('init', '-q', '-b', 'main'); + git('config', 'user.email', 'ci@example.invalid'); + git('config', 'user.name', 'CI'); + git('config', 'commit.gpgsign', 'false'); + const write = (path, text) => writeFileSync(join(root, path), text); + write('README.md', 'base\n'); + git('add', '-A'); + git('commit', '-q', '-m', 'base'); + git('checkout', '-q', '-b', 'head'); + write('feature.txt', 'feature\n'); + git('add', '-A'); + git('commit', '-q', '-m', 'head'); + const head = git('rev-parse', 'HEAD'); + git('checkout', '-q', 'main'); + write('MAIN.md', 'main moved\n'); + git('add', '-A'); + git('commit', '-q', '-m', 'main'); + const base = git('rev-parse', 'HEAD'); + const merge = message => { + git('checkout', '-q', '--detach', base); + git('merge', '-q', '--no-ff', '-m', message, head); + return git('rev-parse', 'HEAD'); + }; + return {root, git, write, head, base, merge}; +} + +const inRepository = (root, fn) => { + const git = (...args) => execFileSync('git', ['-C', root, ...args], {encoding: 'utf8'}); + return fn(git); +}; + +test('marker names carry the workflow, the first parent and the tree, and nothing else', () => { + const a = 'a'.repeat(40); + const b = 'b'.repeat(40); + assert.equal(markerName('phpunit-feature', a, b), `tested-merge-phpunit-feature-${a}-${b}`); + for (const [key, base, tree] of [['', a, b], ['Bad Key', a, b], ['../x', a, b], ['k', 'a'.repeat(39), b], + ['k', a, `${b}\n`], ['k', undefined, b], ['k', a, 'B'.repeat(40)]]) { + assert.throws(() => markerName(key, base, tree), /Invalid/); + } +}); + +test('a pull request test merge and a queue group of the same parents share one marker', () => { + const repo = repository(); + try { + const pullRequestMerge = repo.merge('Merge head into main'); + const prMerge = inRepository(repo.root, git => localMerge((...args) => git(...args))); + const groupMerge = repo.merge('Merge pull request #1 from head'); + assert.notEqual(pullRequestMerge, groupMerge, 'distinct commits'); + const group = inRepository(repo.root, git => localMerge((...args) => git(...args))); + assert.deepEqual([prMerge.base, prMerge.head], [repo.base, repo.head]); + assert.equal(group.commit, groupMerge); + assert.equal(prMerge.tree, group.tree); + assert.equal(recordName({key: 'server-parity', prHeadSha: repo.head, merge: prMerge}), + markerName('server-parity', group.base, group.tree)); + } finally { + rmSync(repo.root, {recursive: true, force: true}); + } +}); + +test('a group stacked on a queued predecessor has a marker no pull request run recorded', () => { + const repo = repository(); + try { + repo.merge('Merge head into main'); + const prMerge = inRepository(repo.root, git => localMerge((...args) => git(...args))); + // A predecessor entry lands first in the same queue group. + repo.git('checkout', '-q', '--detach', repo.base); + repo.write('OTHER.md', 'predecessor\n'); + repo.git('add', '-A'); + repo.git('commit', '-q', '-m', 'predecessor'); + repo.git('merge', '-q', '--no-ff', '-m', 'group', repo.head); + const stacked = inRepository(repo.root, git => localMerge((...args) => git(...args))); + assert.equal(stacked.head, repo.head); + assert.notEqual(markerName('k', stacked.base, stacked.tree), markerName('k', prMerge.base, prMerge.tree)); + } finally { + rmSync(repo.root, {recursive: true, force: true}); + } +}); + +test('only a two-parent merge of this pull request head records a marker', () => { + const repo = repository(); + try { + // A plain commit is not a test merge. + assert.throws(() => inRepository(repo.root, git => localMerge((...args) => git(...args))), /two-parent merge/); + repo.merge('Merge head into main'); + const merge = inRepository(repo.root, git => localMerge((...args) => git(...args))); + for (const prHeadSha of ['', undefined, '0'.repeat(40), repo.base, `${repo.head}\n`]) { + assert.throws(() => recordName({key: 'k', prHeadSha, merge}), /not this pull request/); + } + } finally { + rmSync(repo.root, {recursive: true, force: true}); + } +}); + +const ID = 4242; +const base = 'b'.repeat(40); +const head = 'c'.repeat(40); +const tree = 'd'.repeat(40); +const commit = 'e'.repeat(40); +const merge = {commit, base, head, tree}; +const path = '.github/workflows/phpunit-feature.yml'; +const name = markerName('phpunit-feature', base, tree); +const artifact = (overrides = {}) => ({name, expired: false, workflow_run: {id: 7, head_sha: head}, ...overrides}); +const run = (overrides = {}) => ({id: 7, event: 'pull_request', status: 'completed', conclusion: 'success', path, + head_sha: head, repository: {id: ID}, head_repository: {id: ID}, ...overrides}); + +function api({artifacts = [artifact()], runs = {7: run()}, ok = true, throws = false} = {}) { + const requests = []; + const fetchImpl = async (url, init) => { + requests.push({url, init}); + if (throws) throw new Error(`boom ${url}`); + const runMatch = /\/actions\/runs\/(\d+)$/.exec(url); + const body = runMatch ? runs[runMatch[1]] ?? {} : {artifacts}; + return {ok, json: async () => body}; + }; + return {requests, fetchImpl}; +} + +const ask = (fetchImpl, overrides = {}) => testedRun({key: 'phpunit-feature', workflowPath: path, merge, + mergeGroupHeadSha: commit, repository: 'durable-workflow/server', repositoryId: String(ID), token: 't', fetchImpl, + ...overrides}); + +test('a successful same-repository pull request run of this workflow on this exact merge skips the suite', async () => { + const {requests, fetchImpl} = api(); + const decision = await ask(fetchImpl); + assert.equal(decision.tested, true, decision.reason); + assert.equal(decision.runId, 7); + assert.equal(requests[0].url, `https://api.github.com/repos/durable-workflow/server/actions/artifacts?name=${name}&per_page=100`); + assert.equal(requests[1].url, 'https://api.github.com/repos/durable-workflow/server/actions/runs/7'); + assert.equal(requests[0].init.headers.authorization, 'Bearer t'); +}); + +for (const [label, overrides] of [ + ['a push run', {event: 'push'}], + ['a merge-group run', {event: 'merge_group'}], + ['a pull_request_target run', {event: 'pull_request_target'}], + ['a dispatched run', {event: 'workflow_dispatch'}], + ['a failed run', {conclusion: 'failure'}], + ['a cancelled run', {conclusion: 'cancelled'}], + ['a run still in progress', {status: 'in_progress', conclusion: null}], + ['another workflow', {path: '.github/workflows/server-parity.yml'}], + ['another pull request head', {head_sha: 'f'.repeat(40)}], + ['a fork', {head_repository: {id: ID + 1}}], + ['another repository', {repository: {id: ID + 1}}], + ['a different run id than the artifact named', {id: 8}], + ['no run', null], +]) { + test(`${label} never skips the suite`, async () => { + const {fetchImpl} = api({runs: {7: overrides === null ? {} : run(overrides)}}); + assert.equal((await ask(fetchImpl)).tested, false); + }); +} + +for (const [label, artifacts] of [ + ['no marker', []], + ['an expired marker', [artifact({expired: true})]], + ['a marker from another head', [artifact({workflow_run: {id: 7, head_sha: 'f'.repeat(40)}})]], + ['a marker with another name', [artifact({name: `${name}x`})]], + ['a marker without a run', [artifact({workflow_run: {head_sha: head}})]], + ['a malformed listing', 'nope'], +]) { + test(`${label} never skips the suite`, async () => { + const {fetchImpl} = api({artifacts}); + assert.equal((await ask(fetchImpl)).tested, false); + }); +} + +test('one good run among bad ones skips; the bad ones alone never do', async () => { + const runs = {5: run({id: 5, conclusion: 'failure'}), 7: run()}; + const artifacts = [artifact({workflow_run: {id: 5, head_sha: head}}), artifact()]; + assert.equal((await ask(api({artifacts, runs}).fetchImpl)).tested, true); + assert.equal((await ask(api({artifacts: artifacts.slice(0, 1), runs}).fetchImpl)).tested, false); +}); + +test('API errors, missing identity and a checkout that is not this group run the suite', async () => { + assert.equal((await ask(api({ok: false}).fetchImpl)).tested, false); + const failing = api({throws: true}); + const decision = await ask(failing.fetchImpl); + assert.equal(decision.tested, false); + assert.doesNotMatch(decision.reason, /boom|https:/); + for (const overrides of [ + {token: ''}, + {mergeGroupHeadSha: 'f'.repeat(40)}, + {mergeGroupHeadSha: undefined}, + {repositoryId: ''}, + {repositoryId: '0'}, + {repository: 'durable-workflow/server/../x'}, + {workflowPath: '.github/workflows/../x.yml'}, + {key: 'Bad Key'}, + {apiUrl: 'http://api.github.com'}, + ]) { + const {requests, fetchImpl} = api(); + assert.equal((await ask(fetchImpl, overrides)).tested, false, JSON.stringify(overrides)); + assert.equal(requests.length, 0, JSON.stringify(overrides)); + } +}); + +function cli(root, args, env) { + const base = {...process.env}; + for (const key of ['RUNS_TOKEN', 'PR_HEAD_SHA', 'MERGE_GROUP_HEAD_SHA', 'GITHUB_REPOSITORY', 'GITHUB_REPOSITORY_ID', 'GITHUB_API_URL']) { + delete base[key]; + } + return spawnSync('node', [script, ...args], {cwd: root, env: {...base, ...env}, encoding: 'utf8'}); +} + +test('the record command prints the marker for this pull request merge and nothing otherwise', () => { + const repo = repository(); + try { + repo.merge('Merge head into main'); + const tree = repo.git('rev-parse', 'HEAD^{tree}'); + const ok = cli(repo.root, ['record', 'server-parity', '.github/workflows/server-parity.yml'], {PR_HEAD_SHA: repo.head}); + assert.equal(ok.status, 0, ok.stderr); + assert.equal(ok.stdout, `marker=${markerName('server-parity', repo.base, tree)}\n`); + // The workflows call the wrapper, from the repository root. + const wrapped = spawnSync('bash', [resolve('scripts/ci/tested-merge.sh'), 'record', 'server-parity', '.github/workflows/server-parity.yml'], + {cwd: resolve('.'), env: {...process.env, PR_HEAD_SHA: repo.head, GIT_DIR: join(repo.root, '.git'), GIT_WORK_TREE: repo.root}, encoding: 'utf8'}); + assert.equal(wrapped.stdout, ok.stdout, wrapped.stderr); + const wrong = cli(repo.root, ['record', 'server-parity', '.github/workflows/server-parity.yml'], {PR_HEAD_SHA: repo.base}); + assert.equal(wrong.status, 0, wrong.stderr); + assert.equal(wrong.stdout, 'marker=\n'); + } finally { + rmSync(repo.root, {recursive: true, force: true}); + } +}); + +test('the check command runs the suite whenever it cannot prove the merge was tested', () => { + const repo = repository(); + try { + const groupHead = repo.merge('Merge pull request #1'); + for (const env of [ + {MERGE_GROUP_HEAD_SHA: groupHead, GITHUB_REPOSITORY: 'o/r', GITHUB_REPOSITORY_ID: '1'}, + {MERGE_GROUP_HEAD_SHA: groupHead, GITHUB_REPOSITORY: 'o/r', GITHUB_REPOSITORY_ID: '1', RUNS_TOKEN: 't', + GITHUB_API_URL: 'https://127.0.0.1:9'}, + {MERGE_GROUP_HEAD_SHA: repo.head, GITHUB_REPOSITORY: 'o/r', GITHUB_REPOSITORY_ID: '1', RUNS_TOKEN: 't'}, + ]) { + const outcome = cli(repo.root, ['check', 'phpunit-feature', path], env); + assert.equal(outcome.status, 0, outcome.stderr); + assert.equal(outcome.stdout, 'tested=false\n', JSON.stringify(env)); + } + } finally { + rmSync(repo.root, {recursive: true, force: true}); + } +}); + +test('PHPUnit Feature skips only the feature suite, only for a tested merge group', () => { + const text = workflow('phpunit-feature.yml'); + const blocks = jobs(text); + const tested = blocks['tested-merge']; + assert.match(tested, /^ {4}permissions:\n {6}contents: read\n {6}actions: read$/m); + assert.match(tested, /fetch-depth: 2\n {10}persist-credentials: false/); + assert.match(tested, /sparse-checkout: \|\n {12}scripts\/ci\/tested-merge\.sh\n {12}scripts\/ci\/tested-merge\.mjs\n/); + assert.match(tested, /if: \$\{\{ github\.event_name == 'pull_request' \}\}\n {8}env:\n {10}PR_HEAD_SHA: \$\{\{ github\.event\.pull_request\.head\.sha \}\}\n {8}run: bash scripts\/ci\/tested-merge\.sh record phpunit-feature \.github\/workflows\/phpunit-feature\.yml >> "\$GITHUB_OUTPUT"/); + assert.match(tested, /if: \$\{\{ github\.event_name == 'merge_group' \}\}\n {8}env:\n {10}MERGE_GROUP_HEAD_SHA: \$\{\{ github\.event\.merge_group\.head_sha \}\}\n {10}RUNS_TOKEN: \$\{\{ github\.token \}\}\n {8}run: bash scripts\/ci\/tested-merge\.sh check phpunit-feature \.github\/workflows\/phpunit-feature\.yml >> "\$GITHUB_OUTPUT"/); + // Only the tested-merge job may read Actions data. + assert.equal(text.match(/actions: read/g).length, 1); + assert.match(blocks.feature, /^ {4}needs: \[action-policy, tested-merge\]$/m); + assert.match(blocks.feature, /^ {4}if: \$\{\{ always\(\) && !cancelled\(\) && needs\.tested-merge\.outputs\.tested != 'true' \}\}$/m); + assert.match(blocks.qualification, /^ {4}needs: \[preflight, tested-merge, feature\]$/m); + // Repository contracts, the action policy and every other job always run + // on a group: no other job reads the decision. + const code = block => block.split('\n').filter(line => !/^\s*#/.test(line)).join('\n'); + for (const id of Object.keys(blocks).filter(id => !['tested-merge', 'feature', 'qualification'].includes(id))) { + assert.doesNotMatch(code(blocks[id]), /needs\.tested-merge|tested-merge\.mjs/, id); + } +}); + +// The qualification gate, run as the workflow runs it. +function qualify(env) { + const block = jobs(workflow('phpunit-feature.yml')).qualification; + const body = /run: \|\n((?: {10}.*\n)+)/.exec(block)[1].replace(/^ {10}/gm, ''); + return spawnSync('bash', ['-c', body], {env: {PATH: process.env.PATH, ...env}, encoding: 'utf8'}).status; +} + +test('the qualification gate accepts a skipped suite only for a tested merge group', () => { + const pass = {EVENT_NAME: 'merge_group', PREFLIGHT_RESULT: 'success', FEATURE_RESULT: 'skipped', TESTED_MERGE: 'true'}; + assert.equal(qualify(pass), 0); + assert.equal(qualify({...pass, FEATURE_RESULT: 'success', TESTED_MERGE: ''}), 0); + assert.equal(qualify({...pass, EVENT_NAME: 'pull_request', FEATURE_RESULT: 'success', TESTED_MERGE: ''}), 0); + for (const overrides of [ + {PREFLIGHT_RESULT: 'failure'}, + {PREFLIGHT_RESULT: 'skipped'}, + {TESTED_MERGE: ''}, + {TESTED_MERGE: 'false'}, + {EVENT_NAME: 'pull_request'}, + {EVENT_NAME: 'push'}, + {FEATURE_RESULT: 'failure'}, + {FEATURE_RESULT: 'cancelled'}, + {FEATURE_RESULT: 'failure', TESTED_MERGE: 'false'}, + ]) { + assert.notEqual(qualify({...pass, ...overrides}), 0, JSON.stringify(overrides)); + } +}); + +for (const [file, gateJob, markerSource] of [ + ['phpunit-feature.yml', 'qualification', 'needs.tested-merge.outputs.marker'], + ['server-parity.yml', 'result', 'needs.changes.outputs.marker'], +]) { + test(`${file} records a marker only from a passing pull request run`, () => { + const block = jobs(workflow(file))[gateJob]; + const condition = `if: \${{ success() && github.event_name == 'pull_request' && ${markerSource} != '' }}`; + assert.equal(block.split(condition).length - 1, 2, 'write and upload share one condition'); + const upload = block.slice(block.indexOf('Record that this exact merge passed')); + assert.match(upload, /continue-on-error: true/); + assert.match(upload, /uses: actions\/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a/); + assert.match(upload, new RegExp(`name: \\$\\{\\{ ${markerSource.replaceAll('.', '\\.')} \\}\\}`)); + assert.match(upload, /retention-days: 7/); + // The marker is written after the gate itself. + assert.ok(block.indexOf('Record that this exact merge passed') > block.indexOf('run: '), file); + }); +} + +test('Shared Server fixtures skip every fixture job only for a tested merge group', () => { + const changes = jobs(workflow('server-parity.yml')).changes; + assert.ok(changes.split('\n').includes( + " run: ${{ steps.tested.outputs.tested == 'true' && 'false' || steps.classify.outputs.run }}")); + assert.ok(changes.split('\n').includes(' tested: ${{ steps.tested.outputs.tested }}')); + assert.match(changes, /id: tested\n {8}if: \$\{\{ github\.event_name == 'merge_group' \}\}\n {8}env:\n {10}MERGE_GROUP_HEAD_SHA: \$\{\{ github\.event\.merge_group\.head_sha \}\}\n {10}RUNS_TOKEN: \$\{\{ github\.token \}\}\n {8}run: bash scripts\/ci\/tested-merge\.sh check server-parity \.github\/workflows\/server-parity\.yml >> "\$GITHUB_OUTPUT"/); + assert.match(changes, /id: record\n {8}if: \$\{\{ github\.event_name == 'pull_request' \}\}\n {8}env:\n {10}PR_HEAD_SHA: \$\{\{ github\.event\.pull_request\.head\.sha \}\}\n {8}run: bash scripts\/ci\/tested-merge\.sh record server-parity \.github\/workflows\/server-parity\.yml >> "\$GITHUB_OUTPUT"/); +}); From f8279101def7d3b9b7e690bd03ac773805459248 Mon Sep 17 00:00:00 2001 From: Richard McDaniel <1130888+rmcdaniel@users.noreply.github.com> Date: Thu, 12 Nov 2026 19:24:52 +0000 Subject: [PATCH 2/2] CI: call the runner's fixed Git in the tested-merge check Addresses SonarCloud S4036 (PATH lookup) and S6535 on #1176. Co-Authored-By: Claude Opus 5.5 --- scripts/ci/tested-merge.mjs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/ci/tested-merge.mjs b/scripts/ci/tested-merge.mjs index 8abe1b159..ff6ba5738 100644 --- a/scripts/ci/tested-merge.mjs +++ b/scripts/ci/tested-merge.mjs @@ -33,9 +33,9 @@ export function markerName(key, base, tree) { return `tested-merge-${key}-${base}-${tree}`; } -// The checked-out merge commit: its two parents and its tree. Git runs -// without a shell, from the current directory. -export function localMerge(git = (...args) => execFileSync('git', args, {encoding: 'utf8'})) { +// The checked-out merge commit: its two parents and its tree. The runner's +// fixed Git runs without a shell or PATH lookup, from the current directory. +export function localMerge(git = (...args) => execFileSync('/usr/bin/git', args, {encoding: 'utf8'})) { const [commit, ...parents] = git('rev-list', '--parents', '-n', '1', 'HEAD').trim().split(' '); const tree = git('rev-parse', 'HEAD^{tree}').trim(); if (!SHA.test(commit) || parents.length !== 2 || !parents.every(parent => SHA.test(parent)) || !SHA.test(tree)) { @@ -63,7 +63,7 @@ export async function testedRun({key, workflowPath, merge, mergeGroupHeadSha, re return no('invalid repository identity'); } if (!token) return no('no token to read workflow runs'); - if (!/^https:\/\/[A-Za-z0-9.-]+(?::\d+)?(?:\/[A-Za-z0-9._\/-]*)?$/.test(apiUrl)) return no('invalid API URL'); + if (!/^https:\/\/[A-Za-z0-9.-]+(?::\d+)?(?:\/[A-Za-z0-9._/-]*)?$/.test(apiUrl)) return no('invalid API URL'); let name; try { name = markerName(key, merge.base, merge.tree);