Fix breaking-change check: compare against merge-base, scope to PR diff

.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/<base> 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:

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/<base> 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/<baseRef> 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<string> | 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})
 }
+
+

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/<base> 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')
+})

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<string>} 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
 }
+