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

workspace/src/major-change-check.js

@@ -7,7 +7,19 @@
  * 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:
+ *   - On `pull_request` events the baseline is the merge-base of the PR
+ *     head and the base branch. This avoids false positives when `main`
+ *     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).
+ *   - On `merge_group` events the baseline is `merge_group.base_sha`,
+ *     which is exactly the commit the merge queue is testing against.
+ *   - Otherwise, falls back to `main`'s current tip (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.
  */
 
@@ -23,6 +35,96 @@ 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
+// ---------------------------------------------------------------------------
+
+/**
+ * Reads `GITHUB_EVENT_PATH` and returns the baseline + diff context for the
+ * check. Works for `pull_request`, `pull_request_review`, and `merge_group`
+ * events; otherwise returns a legacy fallback that compares against `main`.
+ *
+ * Returned shape:
+ *   {
+ *     baselineRef: string,           // SHA or branch name to clone/checkout as baseline
+ *     headSha: string | null,        // PR head SHA, or null in fallback mode
+ *     repo: {owner, name},           // for GitHub API calls
+ *     prNumber: number | null,       // for review/codeowner lookups
+ *     changedFiles: Set<string> | null, // null = scan everything (fallback)
+ *   }
+ */
+export async function resolveContext({
+  eventName = process.env.GITHUB_EVENT_NAME,
+  eventPath = process.env.GITHUB_EVENT_PATH,
+  fetchCompare = defaultFetchCompare,
+} = {}) {
+  const repoSlug = process.env.GITHUB_REPOSITORY || 'Shopify/cli'
+  const [owner, name] = repoSlug.split('/')
+  const repo = {owner, name}
+
+  // No event payload => running locally or in an unsupported event. Fall
+  // back to legacy behavior so this script remains usable as a manual tool.
+  if (!eventPath) {
+    return {baselineRef: 'main', headSha: null, repo, prNumber: null, changedFiles: null}
+  }
+
+  let event
+  try {
+    event = JSON.parse(await fs.readFile(eventPath, 'utf-8'))
+  } catch {
+    return {baselineRef: 'main', headSha: null, repo, prNumber: null, changedFiles: null}
+  }
+
+  if ((eventName === 'pull_request' || eventName === 'pull_request_review') && event.pull_request) {
+    const headSha = event.pull_request.head.sha
+    const baseSha = event.pull_request.base.sha
+    const prNumber = event.pull_request.number
+    const compare = await fetchCompare(repo, baseSha, headSha)
+    const baselineRef = compare?.merge_base_commit?.sha || baseSha
+    // When the compare API fails we fall back to scanning everything
+    // (changedFiles=null). Narrowing to an empty set would silently
+    // suppress real breaking changes, which is unsafe.
+    const changedFiles = compare?.files ? new Set(compare.files.map((f) => f.filename)) : null
+    return {baselineRef, headSha, repo, prNumber, changedFiles}
+  }
+
+  if (eventName === 'merge_group' && event.merge_group) {
+    const headSha = event.merge_group.head_sha
+    const baselineRef = event.merge_group.base_sha
+    const compare = await fetchCompare(repo, baselineRef, headSha)
+    const changedFiles = compare?.files ? new Set(compare.files.map((f) => f.filename)) : null
+    return {baselineRef, headSha, repo, prNumber: null, changedFiles}
+  }
+
+  return {baselineRef: 'main', headSha: null, repo, prNumber: null, changedFiles: null}
+}
+
+/**
+ * Default GitHub compare-API fetcher. Uses the standard `GITHUB_TOKEN` so
+ * it works on first-party PRs; falls back to unauthenticated for forks
+ * (still works for public-repo compare). On any failure we return null and
+ * the caller falls back to scanning everything — i.e. degrades to legacy
+ * behavior, never silently ignores potential breaking changes.
+ */
+async function defaultFetchCompare(repo, base, head) {
+  if (!base || !head) return null
+  const url = `https://api.github.com/repos/${repo.owner}/${repo.name}/compare/${base}...${head}`
+  const headers = {Accept: 'application/vnd.github+json'}
+  const token = process.env.GITHUB_TOKEN
+  if (token) headers.Authorization = `Bearer ${token}`
+  try {
+    const res = await fetch(url, {headers})
+    if (!res.ok) {
+      logMessage(`compare API returned ${res.status} — falling back to full scan`)
+      return null
+    }
+    return await res.json()
+  } catch (error) {
+    logMessage(`compare API failed: ${error.message} — falling back to full scan`)
+    return null
+  }
+}
+
 // ---------------------------------------------------------------------------
 // 1. Check changesets for major bumps
 // ---------------------------------------------------------------------------
@@ -107,9 +209,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 +500,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 +512,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 +670,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 or merge-queue base)`)
+    }
+    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

@@ -12,8 +12,11 @@
 
 import {test} from 'node:test'
 import assert from 'node:assert/strict'
+import {mkdtemp, writeFile, rm} from 'node:fs/promises'
+import {tmpdir} from 'node:os'
+import {join} from 'node:path'
 
-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 +138,65 @@ test('stripStringsAndComments preserves length and newlines', () => {
   assert.equal(stripped.includes('hello'), false)
   assert.equal(stripped.includes('trailing'), false)
 })
+
+// ---------------------------------------------------------------------------
+// resolveContext()
+// ---------------------------------------------------------------------------
+
+async function withEventFile(payload, fn) {
+  const dir = await mkdtemp(join(tmpdir(), 'major-change-evt-'))
+  const file = join(dir, 'event.json')
+  await writeFile(file, JSON.stringify(payload), 'utf-8')
+  try {
+    return await fn(file)
+  } finally {
+    await rm(dir, {recursive: true, force: true})
+  }
+}
+
+test('resolveContext: pull_request resolves baseline to the merge-base', async () => {
+  const payload = {pull_request: {number: 42, head: {sha: 'aaa'}, base: {sha: 'bbb'}}}
+  await withEventFile(payload, async (eventPath) => {
+    const fetchCompare = async (_repo, base, head) => {
+      assert.equal(base, 'bbb')
+      assert.equal(head, 'aaa')
+      return {merge_base_commit: {sha: 'mergebase123'}, files: [{filename: 'packages/app/foo.ts'}]}
+    }
+    const ctx = await resolveContext({eventName: 'pull_request', eventPath, fetchCompare})
+    assert.equal(ctx.baselineRef, 'mergebase123', 'uses merge-base, not base.sha')
+    assert.equal(ctx.headSha, 'aaa')
+    assert.equal(ctx.prNumber, 42)
+    assert.deepEqual([...ctx.changedFiles], ['packages/app/foo.ts'])
+  })
+})
+
+test('resolveContext: merge_group uses merge_group.base_sha as baseline', async () => {
+  const payload = {merge_group: {base_sha: 'qsbase', head_sha: 'qshead'}}
+  await withEventFile(payload, async (eventPath) => {
+    const fetchCompare = async () => ({files: [{filename: 'packages/cli/oclif.manifest.json'}]})
+    const ctx = await resolveContext({eventName: 'merge_group', eventPath, fetchCompare})
+    assert.equal(ctx.baselineRef, 'qsbase')
+    assert.equal(ctx.headSha, 'qshead')
+    assert.equal(ctx.prNumber, null)
+    assert.deepEqual([...ctx.changedFiles], ['packages/cli/oclif.manifest.json'])
+  })
+})
+
+test('resolveContext: compare API failure degrades to scanning everything', async () => {
+  const payload = {pull_request: {number: 1, head: {sha: 'h'}, base: {sha: 'b'}}}
+  await withEventFile(payload, async (eventPath) => {
+    const fetchCompare = async () => null
+    const ctx = await resolveContext({eventName: 'pull_request', eventPath, fetchCompare})
+    // Falls back to base.sha when merge_base_commit isn't available, and
+    // crucially leaves changedFiles=null so the scan widens rather than
+    // narrows — we'd rather over-flag than silently miss a real removal.
+    assert.equal(ctx.baselineRef, 'b')
+    assert.equal(ctx.changedFiles, null, 'compare failure must NOT collapse to an empty diff set')
+  })
+})
+
+test('resolveContext: no event payload falls back to scanning main', async () => {
+  const ctx = await resolveContext({eventName: undefined, eventPath: undefined})
+  assert.equal(ctx.baselineRef, 'main')
+  assert.equal(ctx.changedFiles, null)
+})

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