From 8573197aef89d58c70bc9119fe82c3977c0d9631 Mon Sep 17 00:00:00 2001 From: Declan Carroll Date: Thu, 7 May 2026 12:41:35 +0100 Subject: [PATCH] ci: Scope path-filter and janitor diff to PR-only changes (#29993) Co-authored-by: Claude Opus 4.7 (1M context) --- .../ci-filter/__tests__/ci-filter.test.ts | 72 ++++++++++++++++++- .github/actions/ci-filter/action.yml | 3 + .github/actions/ci-filter/ci-filter.mjs | 23 +++++- .github/workflows/ci-pull-requests.yml | 5 +- 4 files changed, 97 insertions(+), 6 deletions(-) diff --git a/.github/actions/ci-filter/__tests__/ci-filter.test.ts b/.github/actions/ci-filter/__tests__/ci-filter.test.ts index 4bd2162d252..41c33b312ae 100644 --- a/.github/actions/ci-filter/__tests__/ci-filter.test.ts +++ b/.github/actions/ci-filter/__tests__/ci-filter.test.ts @@ -1,6 +1,10 @@ -import { describe, it } from 'node:test'; +import { describe, it, before, after } from 'node:test'; import assert from 'node:assert/strict'; -import { matchGlob, parseFilters, evaluateFilter, runValidate } from '../ci-filter.mjs'; +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { matchGlob, parseFilters, evaluateFilter, runValidate, getChangedFiles, getMergeBase } from '../ci-filter.mjs'; // --- matchGlob --- @@ -172,6 +176,70 @@ describe('evaluateFilter', () => { }); }); +// --- getChangedFiles + getMergeBase (integration, exercises real git) --- + +describe('getChangedFiles', () => { + const repoDir = mkdtempSync(join(tmpdir(), 'ci-filter-')); + const remoteDir = mkdtempSync(join(tmpdir(), 'ci-filter-remote-')); + const originalCwd = process.cwd(); + const git = (args: string[], cwd: string = repoDir) => + execFileSync('git', args, { cwd, stdio: 'pipe' }).toString().trim(); + + before(() => { + // Bare remote so the action's `git fetch origin ` works + execFileSync('git', ['init', '--bare', '-b', 'main', remoteDir], { stdio: 'pipe' }); + git(['init', '-b', 'main'], repoDir); + git(['config', 'user.email', 'test@test.local']); + git(['config', 'user.name', 'test']); + git(['remote', 'add', 'origin', remoteDir]); + + // Common ancestor commit + writeFileSync(join(repoDir, 'shared.ts'), 'shared\n'); + git(['add', '.']); + git(['commit', '-m', 'root']); + git(['push', 'origin', 'main']); + + // PR branches off main, adds a file + git(['checkout', '-b', 'pr-branch']); + writeFileSync(join(repoDir, 'pr-only.ts'), 'pr\n'); + git(['add', '.']); + git(['commit', '-m', 'PR change']); + + // Master drifts forward, modifying shared.ts (the pre-fix bug surface) + git(['checkout', 'main']); + writeFileSync(join(repoDir, 'shared.ts'), 'shared\ndrift-from-master\n'); + git(['commit', '-am', 'master moves']); + git(['push', 'origin', 'main']); + + // Sit on the PR branch as if running CI + git(['checkout', 'pr-branch']); + process.chdir(repoDir); + }); + + after(() => { + process.chdir(originalCwd); + rmSync(repoDir, { recursive: true, force: true }); + rmSync(remoteDir, { recursive: true, force: true }); + }); + + it('returns only PR-introduced files (master drift does not pollute)', () => { + const changed = getChangedFiles('main'); + assert.deepEqual(changed, ['pr-only.ts']); + }); + + it('getMergeBase returns the common ancestor commit', () => { + const mergeBase = getMergeBase(); + assert.match(mergeBase, /^[a-f0-9]{40}$/); + const expected = git(['merge-base', 'FETCH_HEAD', 'HEAD']); + assert.equal(mergeBase, expected); + }); + + it('rejects unsafe base refs', () => { + assert.throws(() => getChangedFiles('main; rm -rf /'), /Unsafe/); + assert.throws(() => getChangedFiles('main$evil'), /Unsafe/); + }); +}); + // --- runValidate --- describe('runValidate', () => { diff --git a/.github/actions/ci-filter/action.yml b/.github/actions/ci-filter/action.yml index 10ccf6ac6a9..456ccf9c054 100644 --- a/.github/actions/ci-filter/action.yml +++ b/.github/actions/ci-filter/action.yml @@ -30,6 +30,9 @@ outputs: base-ref: description: 'Resolved base ref used for the diff (filter mode only)' value: ${{ steps.run.outputs.base-ref }} + merge-base: + description: 'Merge-base SHA between FETCH_HEAD and HEAD (filter mode only)' + value: ${{ steps.run.outputs.merge-base }} runs: using: 'composite' diff --git a/.github/actions/ci-filter/ci-filter.mjs b/.github/actions/ci-filter/ci-filter.mjs index f43352377e7..d447078a936 100644 --- a/.github/actions/ci-filter/ci-filter.mjs +++ b/.github/actions/ci-filter/ci-filter.mjs @@ -98,14 +98,30 @@ export function getChangedFiles(baseRef) { if (!SAFE_REF.test(baseRef)) { throw new Error(`Unsafe base ref: "${baseRef}"`); } - execSync(`git fetch --depth=1 origin ${baseRef}`, { stdio: 'pipe' }); - const output = execSync('git diff --name-only FETCH_HEAD HEAD', { encoding: 'utf-8' }); + // Deepen the fetch so the merge base is reachable from this shallow clone. + // A 2-dot diff (FETCH_HEAD HEAD) reports anything that differs in either + // direction, so files added to base-branch after the PR diverged show up as + // "changed" — spuriously triggering path-filtered jobs. The merge base + // scopes the diff to PR-only changes. + execSync(`git fetch --no-tags --prune --deepen=200 origin ${baseRef}`, { stdio: 'pipe' }); + const output = execSync('git diff --name-only --merge-base FETCH_HEAD HEAD', { + encoding: 'utf-8', + }); return output .split('\n') .map((f) => f.trim()) .filter(Boolean); } +/** + * Resolve the merge-base SHA between FETCH_HEAD and HEAD. + * Used to give downstream tools (e.g. janitor's AST diff) a stable, PR-only + * comparison point that doesn't drift when the base branch moves forward. + */ +export function getMergeBase() { + return execSync('git merge-base FETCH_HEAD HEAD', { encoding: 'utf-8' }).trim(); +} + // --- Filter evaluation --- /** @@ -155,7 +171,9 @@ export function runFilter() { const filters = parseFilters(filtersInput); const changedFiles = getChangedFiles(baseRef); + const mergeBase = getMergeBase(); + console.log(`Merge base: ${mergeBase}`); console.log(`Changed files (${changedFiles.length}):`); for (const f of changedFiles) { console.log(` ${f}`); @@ -172,6 +190,7 @@ export function runFilter() { setOutput('results', JSON.stringify(results)); setOutput('changed-files', changedFiles.join('\n')); setOutput('base-ref', baseRef); + setOutput('merge-base', mergeBase); } // --- Mode: validate --- diff --git a/.github/workflows/ci-pull-requests.yml b/.github/workflows/ci-pull-requests.yml index db03e8edac2..3d567a5b9a4 100644 --- a/.github/workflows/ci-pull-requests.yml +++ b/.github/workflows/ci-pull-requests.yml @@ -30,6 +30,7 @@ jobs: e2e_performance: ${{ fromJSON(steps.ci-filter.outputs.results)['e2e-performance'] == true }} instance_ai_workflow_eval: ${{ fromJSON(steps.ci-filter.outputs.results)['instance-ai-workflow-eval'] == true }} commit_sha: ${{ steps.commit-sha.outputs.sha }} + merge_base: ${{ steps.ci-filter.outputs.merge-base }} matrix: ${{ steps.generate-matrix.outputs.matrix }} skip_tests: ${{ steps.generate-matrix.outputs.skip-tests }} steps: @@ -73,7 +74,6 @@ jobs: packages/testing/playwright/playwright-projects.ts packages/testing/playwright/package.json .github/workflows/test-dev-server-smoke-reusable.yml - .github/workflows/ci-pull-requests.yml workflows: .github/** workflow-scripts: .github/scripts/** performance: @@ -120,9 +120,10 @@ jobs: if: fromJSON(steps.ci-filter.outputs.results).ci || fromJSON(steps.ci-filter.outputs.results).e2e env: CHANGED_FILES: ${{ steps.ci-filter.outputs.changed-files }} + MERGE_BASE: ${{ steps.ci-filter.outputs.merge-base }} run: | FILES_CSV=$(echo "$CHANGED_FILES" | tr '\n' ',' | sed 's/,$//') - MATRIX=$(node packages/testing/playwright/scripts/distribute-tests.mjs --matrix 16 --orchestrate --impact "--files=$FILES_CSV" --base=FETCH_HEAD) + MATRIX=$(node packages/testing/playwright/scripts/distribute-tests.mjs --matrix 16 --orchestrate --impact "--files=$FILES_CSV" "--base=$MERGE_BASE") echo "matrix=$MATRIX" >> "$GITHUB_OUTPUT" echo "skip-tests=$(node -e "process.stdout.write(JSON.parse(process.argv[1])[0]?.skip === true ? 'true' : 'false')" "$MATRIX")" >> "$GITHUB_OUTPUT"