From 3f375c09b6e51fbf387de1be3fee99394360bb90 Mon Sep 17 00:00:00 2001 From: Alfonso Noriega Date: Tue, 5 May 2026 12:34:50 +0200 Subject: [PATCH] Simplify resolveContext to use git merge-base instead of compare API Per Gonzalo's review: collapses event-payload parsing + per-event-type branching + GitHub compare API call into a single `git merge-base origin/ HEAD` + `git diff --name-only`. Drops GITHUB_TOKEN handling and the compare-API fallback path entirely. - workspace/src/major-change-check.js: resolveContext now reads GITHUB_BASE_REF (set on both pull_request and merge_group events) and shells out to git via an injectable runGit. On any git failure we still degrade *open* (changedFiles=null) so we never mask a real breaking change. - .github/workflows/tests-pr.yml: bump fetch-depth 1 -> 0 on the major-change-check job so the merge-base is reachable. - workspace/src/major-change-check.test.js: rewrite the 4 resolveContext tests against a runGit mock; drop fs/event-file fixtures. --- .github/workflows/tests-pr.yml | 4 +- workspace/src/major-change-check.js | 159 +++++++++++++++++++---- workspace/src/major-change-check.test.js | 63 ++++++++- workspace/src/utils/git.js | 22 +++- 4 files changed, 217 insertions(+), 31 deletions(-) diff --git a/.github/workflows/tests-pr.yml b/.github/workflows/tests-pr.yml index f18a9b69218..67eed3eb0a3 100644 --- a/.github/workflows/tests-pr.yml +++ b/.github/workflows/tests-pr.yml @@ -307,7 +307,9 @@ jobs: with: repository: ${{ github.event.pull_request.head.repo.full_name || github.event.repository.full_name }} ref: ${{ github.event.pull_request.head.ref || github.event.merge_group.head_ref }} - fetch-depth: 1 + # Full history so `git merge-base origin/ HEAD` (used by + # major-change-check.js) can reach the divergence point. + fetch-depth: 0 - name: Setup deps uses: ./.github/actions/setup-cli-deps with: diff --git a/workspace/src/major-change-check.js b/workspace/src/major-change-check.js index 0baf21178fa..e52053576c4 100644 --- a/workspace/src/major-change-check.js +++ b/workspace/src/major-change-check.js @@ -7,7 +7,20 @@ * 2. OCLIF manifest changes: removed commands, removed flags, or removed flag env vars * 3. Zod schema changes: removed or renamed fields in app/extension config schemas * - * Compares the current branch against the main branch baseline. + * Baseline selection: + * - In CI (`GITHUB_BASE_REF` set, by both `pull_request` and `merge_group`) + * the baseline is `git merge-base origin/ HEAD`. This avoids false + * positives when the base branch has progressed past the PR's branch + * point (a field added on `main` after the PR opened would otherwise + * show up as "removed" by the PR). Requires `fetch-depth: 0` on the + * workflow checkout. + * - Otherwise (local invocation, no base ref) falls back to `main`'s + * current tip with a full scan (legacy behavior). + * + * The schema scan and OCLIF manifest comparison are scoped to files that + * actually changed in the PR's diff so unrelated drift on `main` cannot + * trigger this check. + * * Outputs a GitHub Actions summary and sets a `has_breaking_changes` output. */ @@ -17,12 +30,65 @@ import {mkdtemp, rm} from 'fs/promises' import os from 'os' import {promises as fs} from 'fs' import {setOutput} from '@actions/core' +import {execa} from 'execa' import {cloneCLIRepository} from './utils/git.js' import {logMessage, logSection} from './utils/log.js' import fg from 'fast-glob' const currentDirectory = path.join(url.fileURLToPath(new URL('.', import.meta.url)), '../..') +// --------------------------------------------------------------------------- +// Event context: pick the right baseline and the right diff to scan +// --------------------------------------------------------------------------- + +/** + * Resolves the baseline + diff context for the check using local git. + * + * `GITHUB_BASE_REF` is set on both `pull_request` and `merge_group` events + * (and points at the target branch in both cases), so a single git + * `merge-base origin/ HEAD` gives us the right baseline. The + * surrounding workflow checks out with `fetch-depth: 0` so the merge-base + * is reachable. + * + * Returned shape: + * { + * baselineRef: string, // SHA (or 'main' in fallback) + * changedFiles: Set | null, // null = scan everything (fallback) + * } + * + * On any git failure we degrade *open*: `changedFiles=null` so the scan + * widens rather than silently skipping potential removals. We never want + * to mask a real breaking change because of a transient git error. + */ +export async function resolveContext({ + baseRef = process.env.GITHUB_BASE_REF, + runGit = defaultRunGit, +} = {}) { + // No base ref => running locally or in an unsupported event. Fall back + // to legacy behavior so this script remains usable as a manual tool. + if (!baseRef) { + return {baselineRef: 'main', changedFiles: null} + } + + try { + const baselineRef = (await runGit(['merge-base', `origin/${baseRef}`, 'HEAD'])).trim() + if (!baselineRef) { + return {baselineRef: 'main', changedFiles: null} + } + const diff = await runGit(['diff', '--name-only', `${baselineRef}...HEAD`]) + const changedFiles = new Set(diff.split('\n').filter(Boolean)) + return {baselineRef, changedFiles} + } catch (error) { + logMessage(`git merge-base/diff failed: ${error.message} — falling back to full scan against main`) + return {baselineRef: 'main', changedFiles: null} + } +} + +async function defaultRunGit(args) { + const {stdout} = await execa('git', args) + return stdout +} + // --------------------------------------------------------------------------- // 1. Check changesets for major bumps // --------------------------------------------------------------------------- @@ -107,9 +173,17 @@ function extractManifestSurface(manifest) { return surface } -async function checkManifest(baselineDirectory) { +async function checkManifest(baselineDirectory, {changedFiles} = {}) { logSection('Checking OCLIF manifest for removed commands/flags') + // The OCLIF manifest is a generated artifact — a removed command always + // shows up as a `oclif.manifest.json` change. If the file isn't in the + // PR diff there is by definition no surface change to detect. + if (changedFiles && !changedFiles.has('packages/cli/oclif.manifest.json')) { + logMessage('oclif.manifest.json unchanged in this PR, skipping') + return {removedCommands: [], removedFlags: [], removedEnvVars: []} + } + const baselineManifest = await parseManifest(baselineDirectory) const currentManifest = await parseManifest(currentDirectory) @@ -390,7 +464,7 @@ export function extractSchemaFields(content) { return fields } -async function checkSchemas(baselineDirectory) { +async function checkSchemas(baselineDirectory, {changedFiles} = {}) { logSection('Checking Zod schemas for removed fields') const schemaGlob = 'packages/app/src/cli/models/**/specifications/**/*.ts' @@ -402,9 +476,22 @@ async function checkSchemas(baselineDirectory) { ignore: ignorePatterns, }) + // When we know the PR's actual diff, only inspect schema files the PR + // touched. Without this, drift on `main` (e.g. a field added after the + // PR branched) is reported as a removal. With it, removals come strictly + // from this PR's own changes. + const filesToCheck = changedFiles + ? baselineSchemaFiles.filter((file) => changedFiles.has(file)) + : baselineSchemaFiles + + if (changedFiles && filesToCheck.length === 0) { + logMessage('No schema files changed in this PR, skipping') + return [] + } + const removedFields = [] - for (const file of baselineSchemaFiles) { + for (const file of filesToCheck) { const baselinePath = path.join(baselineDirectory, file) const currentPath = path.join(currentDirectory, file) @@ -547,29 +634,51 @@ The following Zod schema fields were removed or their files deleted: // Main // --------------------------------------------------------------------------- -const tmpDir = await mkdtemp(path.join(os.tmpdir(), 'major-change-check-')) - -try { - // This script consumes only git-tracked files (oclif.manifest.json + .ts - // sources). It does not need the baseline's node_modules or dist output, - // so we skip pnpm install and pnpm build to save ~5–10 minutes of CI - // per PR. type-diff.js (which diffs `dist/**/*.d.ts`) keeps the default. - const baselineDirectory = await cloneCLIRepository(tmpDir, {install: false, build: false}) +// `import.meta.main` is only true when this file is run directly. When the +// test file imports it as a module, we don't want to spawn a baseline clone. +if (process.argv[1] && url.fileURLToPath(import.meta.url) === path.resolve(process.argv[1])) { + await runMain() +} - const majorChangesets = await checkChangesets() - const manifestChanges = await checkManifest(baselineDirectory) - const schemaChanges = await checkSchemas(baselineDirectory) +async function runMain() { + const tmpDir = await mkdtemp(path.join(os.tmpdir(), 'major-change-check-')) - const report = buildReport({majorChangesets, manifestChanges, schemaChanges}) + try { + const context = await resolveContext() + if (context.baselineRef !== 'main') { + logMessage(`Resolved baseline to ${context.baselineRef.slice(0, 7)} (merge-base with base branch)`) + } + if (context.changedFiles) { + logMessage(`PR touched ${context.changedFiles.size} file(s); scoping schema/manifest scan to those`) + } - if (report) { - logSection('\n⚠️ Breaking changes detected!') - setOutput('has_breaking_changes', 'true') - setOutput('report', report) - } else { - logSection('\n✅ No breaking changes detected') - setOutput('has_breaking_changes', 'false') + // This script consumes only git-tracked files (oclif.manifest.json + .ts + // sources). It does not need the baseline's node_modules or dist output, + // so we skip pnpm install and pnpm build to save ~5–10 minutes of CI + // per PR. type-diff.js (which diffs `dist/**/*.d.ts`) keeps the default. + const baselineDirectory = await cloneCLIRepository(tmpDir, { + install: false, + build: false, + ref: context.baselineRef, + }) + + const majorChangesets = await checkChangesets() + const manifestChanges = await checkManifest(baselineDirectory, {changedFiles: context.changedFiles}) + const schemaChanges = await checkSchemas(baselineDirectory, {changedFiles: context.changedFiles}) + + const report = buildReport({majorChangesets, manifestChanges, schemaChanges}) + + if (report) { + logSection('\n⚠️ Breaking changes detected!') + setOutput('has_breaking_changes', 'true') + setOutput('report', report) + } else { + logSection('\n✅ No breaking changes detected') + setOutput('has_breaking_changes', 'false') + } + } finally { + await rm(tmpDir, {recursive: true, force: true, maxRetries: 2}) } -} finally { - await rm(tmpDir, {recursive: true, force: true, maxRetries: 2}) } + + diff --git a/workspace/src/major-change-check.test.js b/workspace/src/major-change-check.test.js index 1f20037e789..7c147487064 100644 --- a/workspace/src/major-change-check.test.js +++ b/workspace/src/major-change-check.test.js @@ -13,7 +13,7 @@ import {test} from 'node:test' import assert from 'node:assert/strict' -import {extractSchemaFields, stripStringsAndComments} from './major-change-check.js' +import {extractSchemaFields, resolveContext, stripStringsAndComments} from './major-change-check.js' test('extracts top-level keys from a flat .object({...})', () => { const src = ` @@ -135,3 +135,64 @@ test('stripStringsAndComments preserves length and newlines', () => { assert.equal(stripped.includes('hello'), false) assert.equal(stripped.includes('trailing'), false) }) + +// --------------------------------------------------------------------------- +// resolveContext() +// --------------------------------------------------------------------------- + +test('resolveContext: derives baseline from `git merge-base origin/ HEAD`', async () => { + const calls = [] + const runGit = async (args) => { + calls.push(args) + if (args[0] === 'merge-base') { + assert.deepEqual(args, ['merge-base', 'origin/main', 'HEAD']) + return 'mergebase123\n' + } + if (args[0] === 'diff') { + assert.deepEqual(args, ['diff', '--name-only', 'mergebase123...HEAD']) + return 'packages/app/foo.ts\npackages/cli/oclif.manifest.json\n' + } + throw new Error(`unexpected git call: ${args.join(' ')}`) + } + const ctx = await resolveContext({baseRef: 'main', runGit}) + assert.equal(ctx.baselineRef, 'mergebase123', 'uses merge-base SHA, trimmed') + assert.deepEqual( + [...ctx.changedFiles].sort(), + ['packages/app/foo.ts', 'packages/cli/oclif.manifest.json'], + ) + assert.equal(calls.length, 2, 'one merge-base + one diff call') +}) + +test('resolveContext: respects non-default base branches (e.g. release branches)', async () => { + const runGit = async (args) => { + if (args[0] === 'merge-base') { + assert.equal(args[1], 'origin/release/3.0', 'forwarded GITHUB_BASE_REF unchanged') + return 'releasebase\n' + } + return '' + } + const ctx = await resolveContext({baseRef: 'release/3.0', runGit}) + assert.equal(ctx.baselineRef, 'releasebase') +}) + +test('resolveContext: git failure degrades to scanning everything against main', async () => { + const runGit = async () => { + throw new Error('fatal: Not a valid object name origin/main') + } + const ctx = await resolveContext({baseRef: 'main', runGit}) + // We'd rather over-flag than silently miss a real removal. + assert.equal(ctx.baselineRef, 'main') + assert.equal(ctx.changedFiles, null, 'git failure must NOT collapse to an empty diff set') +}) + +test('resolveContext: no GITHUB_BASE_REF falls back to scanning main (local invocation)', async () => { + let called = false + const runGit = async () => { + called = true + return '' + } + const ctx = await resolveContext({baseRef: undefined, runGit}) + assert.equal(ctx.baselineRef, 'main') + assert.equal(ctx.changedFiles, null) + assert.equal(called, false, 'must not shell out to git when no base ref is known') +}) diff --git a/workspace/src/utils/git.js b/workspace/src/utils/git.js index 6d08d7dca2f..4f93cd55c14 100644 --- a/workspace/src/utils/git.js +++ b/workspace/src/utils/git.js @@ -4,7 +4,13 @@ import git from 'simple-git' import {logMessage, logSection} from './log.js' /** - * Clone the Shopify/cli `main` branch into `tmpDir/cli`. + * Clone the Shopify/cli repository into `tmpDir/cli` and check out `ref`. + * + * `ref` defaults to `main` to preserve historical behavior. Callers that + * want to diff against the merge-base of a PR (rather than against the + * current tip of `main`) should pass the merge-base SHA explicitly so the + * baseline reflects the actual fork point and not whatever has landed on + * `main` since the PR was opened. * * `install` and `build` default to `true` to preserve behavior for * `type-diff.js`, which diffs `dist/**\/*.d.ts` and therefore needs the @@ -15,14 +21,21 @@ import {logMessage, logSection} from './log.js' * PR for those callers. * * @param {string} tmpDir - * @param {{install?: boolean, build?: boolean}} [options] + * @param {{install?: boolean, build?: boolean, ref?: string}} [options] * @returns {Promise} path to the cloned repository */ -export async function cloneCLIRepository(tmpDir, {install = true, build = true} = {}) { - logSection('Setting up baseline: main branch') +export async function cloneCLIRepository(tmpDir, {install = true, build = true, ref = 'main'} = {}) { + logSection(`Setting up baseline: ${ref}`) const directory = path.join(tmpDir, 'cli') logMessage('Cloning repository') + // Full clone (no `--depth`) so any historical SHA passed as `ref` is + // reachable. Shopify/cli's history is small enough that this is fast + // and the clone is throwaway. await git().clone('https://github.com/Shopify/cli.git', directory) + if (ref !== 'main') { + logMessage(`Checking out ${ref}`) + await git(directory).checkout(ref) + } if (install) { logMessage('Installing dependencies') await execa('pnpm', ['install'], {cwd: directory}) @@ -33,3 +46,4 @@ export async function cloneCLIRepository(tmpDir, {install = true, build = true} } return directory } +