From 988024c1a36fcdd41093e0402742b2081ede37c4 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 7 Jun 2026 20:28:45 -0400 Subject: [PATCH] fix(#837): three-dot diff in ci-test-scope so docs-only PRs skip the heavy matrix (#841) CI test-scope detection diffed changed files with a two-dot `git diff --name-only base head`, where base is the moving tip of `next`. A PR branch cut from a slightly older `next` surfaced every product file `next` had gained since the merge-base, flipping product_changed/full_matrix and running the full Windows/macOS matrix + coverage on docs-only PRs. Switch to a three-dot `git diff --name-only base...head` (vs the merge-base), matching GitHub's PR "Files changed" semantics. Add a regression test that builds a stale-base topology, plus a guard test pinning `fetch-depth: 0` on the `changes` job (required for the merge-base to be locally available). Closes #837 Co-authored-by: Claude Opus 4.8 --- scripts/ci-test-scope.cjs | 7 ++- tests/ci-test-scope.test.cjs | 104 +++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 1 deletion(-) diff --git a/scripts/ci-test-scope.cjs b/scripts/ci-test-scope.cjs index 1028377a4..3e9435e6b 100644 --- a/scripts/ci-test-scope.cjs +++ b/scripts/ci-test-scope.cjs @@ -289,7 +289,12 @@ function changedFiles(args) { if (!args.base || !args.head) { throw new Error('--base/--head or --files is required'); } - const stdout = execFileSync('git', ['diff', '--name-only', args.base, args.head], { + // Three-dot diff (merge-base...head) matches GitHub's PR "Files changed" semantics. + // A two-dot `git diff base head` would surface every file `next` gained after this + // branch's merge-base, mis-flagging product_changed/full_matrix on docs-only PRs cut + // from a slightly stale base (#837). The `changes` job checks out with fetch-depth: 0, + // so the merge-base is always available. + const stdout = execFileSync('git', ['diff', '--name-only', `${args.base}...${args.head}`], { encoding: 'utf8', }); return splitFiles(stdout); diff --git a/tests/ci-test-scope.test.cjs b/tests/ci-test-scope.test.cjs index a242cee4e..d6d456028 100644 --- a/tests/ci-test-scope.test.cjs +++ b/tests/ci-test-scope.test.cjs @@ -5,6 +5,8 @@ const assert = require('node:assert/strict'); const { spawnSync } = require('child_process'); const path = require('path'); const fs = require('fs'); +const os = require('node:os'); +const { cleanup } = require('./helpers.cjs'); const ROOT = path.join(__dirname, '..'); const SCRIPT = path.join(ROOT, 'scripts', 'ci-test-scope.cjs'); @@ -207,6 +209,74 @@ describe('ci-test-scope.cjs', () => { assert.deepStrictEqual(result.targeted_tests, ['unit'], 'targeted_tests must be [\'unit\'] when code changed but no rule matched'); }); + + test('three-dot diff: docs-only PR on a stale base ignores product commits next gained after the merge-base', () => { + // Reproduces #837: a docs-only PR branched from a slightly older `next`. + // After the branch point, `next` advances with a PRODUCT commit. A two-dot + // `git diff base head` would surface that product file (flipping product_changed/ + // full_matrix true); a three-dot `git diff base...head` (vs the merge-base, which is + // GitHub's PR semantics) must see ONLY the docs change. + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'ci-scope-837-')); + try { + const git = (...a) => { + const r = spawnSync('git', a, { cwd: tmp, encoding: 'utf8' }); + assert.strictEqual(r.status, 0, `git ${a.join(' ')} failed: ${r.stderr}`); + return r.stdout.trim(); + }; + git('init', '-q'); + git('config', 'user.email', 'test@example.com'); + git('config', 'user.name', 'Test'); + git('config', 'commit.gpgsign', 'false'); + + // merge-base: a docs file + a product file (package.json) + fs.mkdirSync(path.join(tmp, 'tests'), { recursive: true }); // existingTests() reads tests/ + fs.mkdirSync(path.join(tmp, 'docs'), { recursive: true }); + fs.writeFileSync(path.join(tmp, 'docs', 'a.md'), 'base\n'); + fs.writeFileSync(path.join(tmp, 'package.json'), '{"name":"x","version":"1.0.0"}\n'); + git('add', '-A'); + git('commit', '-qm', 'merge-base'); + const baseBranch = git('rev-parse', '--abbrev-ref', 'HEAD'); + + // PR branch (head): docs-only change + git('checkout', '-q', '-b', 'feature'); + fs.writeFileSync(path.join(tmp, 'docs', 'a.md'), 'base\nnew docs line\n'); + git('add', '-A'); + git('commit', '-qm', 'docs: add line'); + const head = git('rev-parse', 'HEAD'); + + // base advances (next gains a PRODUCT commit after the merge-base) + git('checkout', '-q', baseBranch); + fs.writeFileSync(path.join(tmp, 'package.json'), '{"name":"x","version":"2.0.0"}\n'); + git('add', '-A'); + git('commit', '-qm', 'chore: bump version on next'); + const base = git('rev-parse', 'HEAD'); + + const r = spawnSync(process.execPath, [SCRIPT, '--base', base, '--head', head], { + cwd: tmp, + encoding: 'utf8', + }); + assert.strictEqual(r.status, 0, `script failed: stderr=${r.stderr}\nstdout=${r.stdout}`); + const result = JSON.parse(r.stdout); + + assert.deepStrictEqual( + result.changed_files, + ['docs/a.md'], + `expected three-dot diff to see only the docs file, got: ${JSON.stringify(result.changed_files)}`, + ); + assert.strictEqual( + result.product_changed, + false, + `docs-only PR must not set product_changed even on a stale base, got: ${JSON.stringify(result)}`, + ); + assert.strictEqual( + result.full_matrix, + false, + `docs-only PR must not set full_matrix even on a stale base, got: ${JSON.stringify(result)}`, + ); + } finally { + cleanup(tmp); + } + }); }); describe('ci-test-scope superset invariant (#494)', () => { @@ -335,6 +405,40 @@ describe('INERT_WORKFLOWS allowlist integrity guard', () => { }); }); +describe('test.yml changes job contract (#837)', () => { + // ci-test-scope.cjs uses a three-dot `git diff base...head`, which requires the + // merge-base commit to be locally present. The `changes` job in test.yml guarantees + // this via `fetch-depth: 0` on its checkout step. This test pins that contract so + // any future reduction of fetch-depth fails CI loudly (#837). + test('changes job checkout step sets fetch-depth: 0 (required for three-dot diff merge-base)', () => { + const workflowPath = path.join(WORKFLOWS_DIR, 'test.yml'); + const text = fs.readFileSync(workflowPath, 'utf8'); + const lines = text.split('\n'); + + // Locate the `changes:` job (two-space-indented top-level job key). + const jobStart = lines.findIndex(l => /^ {2}changes:\s*$/.test(l)); + assert.ok(jobStart !== -1, 'Could not find ` changes:` job in test.yml'); + + // Find the next top-level job key at the same two-space indentation to bound the region. + let jobEnd = lines.length; + for (let i = jobStart + 1; i < lines.length; i++) { + if (/^ {2}[A-Za-z0-9_-]+:\s*$/.test(lines[i])) { + jobEnd = i; + break; + } + } + + const changesJobText = lines.slice(jobStart, jobEnd).join('\n'); + + assert.ok( + /fetch-depth:\s*0/.test(changesJobText), + 'changes job checkout must set `fetch-depth: 0` so the three-dot `git diff base...head` ' + + 'in ci-test-scope.cjs can resolve the merge-base locally (#837). ' + + 'Reducing fetch-depth breaks the three-dot diff and causes incorrect scope detection.', + ); + }); +}); + describe('code_changed=false implies clean output invariant', () => { // Fix 1: when code_changed is false, full_matrix, targeted_tests, windows_tests // must ALL be empty/false — even if a docs path coincidentally