Allow code-owner approval to override breaking-change check

.github/workflows/breaking-change-check.yml

@@ -0,0 +1,80 @@
+name: Breaking change check
+
+# Runs the breaking-change detector on PR opens, updates, AND on review
+# submissions. Re-running on review submissions lets the script flip from
+# red to green when a code-owner approves the PR — see the override logic
+# in workspace/src/major-change-check.js.
+#
+# Lives in a dedicated workflow so review events don't re-trigger the full
+# unit/e2e/type-diff suite.
+on:
+  pull_request:
+  pull_request_review:
+    types: [submitted, dismissed, edited]
+  merge_group:
+
+concurrency:
+  group: shopify-cli-breaking-change-${{ github.event.pull_request.number || github.event.merge_group.head_sha || github.run_id }}
+  cancel-in-progress: true
+
+env:
+  DEBUG: '1'
+  SHOPIFY_CLI_ENV: development
+  SHOPIFY_CONFIG: debug
+  PNPM_VERSION: '10.11.1'
+  BUNDLE_WITHOUT: 'test:development'
+  GH_TOKEN: ${{ secrets.SHOPIFY_GH_READ_CONTENT_TOKEN }}
+  GH_TOKEN_SHOP: ${{ secrets.SHOP_GH_READ_CONTENT_TOKEN }}
+  DEFAULT_NODE_VERSION: '24.1.0'
+
+jobs:
+  major-change-check:
+    # Skip fork PRs — fork tokens are read-only and the codeowner-override
+    # API calls would fail.
+    if: github.event.pull_request.head.repo.full_name == github.repository || github.event_name == 'merge_group'
+    name: 'Breaking change detection'
+    runs-on: ubuntu-latest
+    timeout-minutes: 30
+    permissions:
+      contents: read
+      pull-requests: write
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          repository: ${{ github.event.pull_request.head.repo.full_name || github.event.repository.full_name }}
+          ref: ${{ github.event.pull_request.head.sha || github.event.merge_group.head_sha }}
+          # Full history so `git merge-base origin/<base> HEAD` (used by
+          # major-change-check.js to scope the diff to PR-only changes) can
+          # reach the divergence point. With fetch-depth: 1 the merge-base
+          # call fails and the script falls back to scanning everything,
+          # surfacing every pre-existing major changeset as "new".
+          fetch-depth: 0
+      - name: Setup deps
+        uses: ./.github/actions/setup-cli-deps
+        with:
+          node-version: ${{ env.DEFAULT_NODE_VERSION }}
+      - name: Build
+        run: pnpm nx run-many --all --skip-nx-cache --target=build --output-style=stream
+      - name: Check for breaking changes
+        id: check
+        env:
+          # The default GITHUB_TOKEN can read PR reviews and the repo's
+          # CODEOWNERS file, which is all the override needs.
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+        run: node workspace/src/major-change-check.js
+      - uses: marocchino/sticky-pull-request-comment@fcf6fe9e4a0409cd9316a5011435be0f3327f1e1 # v2.3.1
+        if: steps.check.outputs.has_breaking_changes == 'true'
+        with:
+          header: Breaking-change-detection
+          message: ${{ steps.check.outputs.report }}
+          recreate: true
+      - uses: marocchino/sticky-pull-request-comment@fcf6fe9e4a0409cd9316a5011435be0f3327f1e1 # v2.3.1
+        if: steps.check.outputs.has_breaking_changes != 'true'
+        with:
+          header: Breaking-change-detection
+          delete: true
+      - name: Fail if breaking changes detected
+        if: steps.check.outputs.has_breaking_changes == 'true'
+        run: |
+          echo '::error::Breaking changes detected. See the sticky comment on the PR for details. A code-owner approval on this PR will turn this check green.'
+          exit 1

.github/workflows/tests-pr.yml

@@ -295,43 +295,6 @@ jobs:
           message: ${{ steps.type-diff.outputs.report }}
           recreate: true
 
-  major-change-check:
-    if: github.event.pull_request.head.repo.full_name == github.repository
-    name: 'Breaking change detection'
-    runs-on: ubuntu-latest
-    timeout-minutes: 30
-    outputs:
-      has_breaking_changes: ${{ steps.check.outputs.has_breaking_changes }}
-    steps:
-      - uses: actions/checkout@v3
-        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 }}
-          # 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:
-          node-version: ${{ env.DEFAULT_NODE_VERSION }}
-      - name: Build
-        run: pnpm nx run-many --all --skip-nx-cache --target=build --output-style=stream
-      - name: Check for breaking changes
-        id: check
-        run: node workspace/src/major-change-check.js
-      - uses: marocchino/sticky-pull-request-comment@fcf6fe9e4a0409cd9316a5011435be0f3327f1e1 # v2.3.1
-        if: steps.check.outputs.has_breaking_changes == 'true'
-        with:
-          header: Breaking-change-detection
-          message: ${{ steps.check.outputs.report }}
-          recreate: true
-      - uses: marocchino/sticky-pull-request-comment@fcf6fe9e4a0409cd9316a5011435be0f3327f1e1 # v2.3.1
-        if: steps.check.outputs.has_breaking_changes != 'true'
-        with:
-          header: Breaking-change-detection
-          delete: true
-      - name: Fail if breaking changes detected
-        if: steps.check.outputs.has_breaking_changes == 'true'
-        run: |
-          echo '::error::Breaking changes detected. See the sticky comment on the PR for details.'
-          exit 1
+  # The breaking-change check moved to .github/workflows/breaking-change-check.yml
+  # so it can re-run on `pull_request_review` events without re-triggering
+  # the rest of this workflow.

workspace/src/major-change-check.js

@@ -89,6 +89,195 @@ async function defaultRunGit(args) {
   return stdout
 }
 
+/**
+ * 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}`
+  return githubGet(url)
+}
+
+async function githubGet(url) {
+  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(`GitHub API ${url} returned ${res.status}`)
+      return null
+    }
+    return await res.json()
+  } catch (error) {
+    logMessage(`GitHub API ${url} failed: ${error.message}`)
+    return null
+  }
+}
+
+// ---------------------------------------------------------------------------
+// Code-owner approval override
+// ---------------------------------------------------------------------------
+
+/**
+ * Parses a CODEOWNERS file body and returns an array of
+ * `{pattern, owners: ["@org/team", "@user", ...]}`.
+ *
+ * Comments and blank lines are skipped. Order is preserved (the last
+ * matching rule wins, per CODEOWNERS semantics).
+ */
+export function parseCodeowners(content) {
+  const rules = []
+  for (const rawLine of content.split(/\r?\n/)) {
+    const line = rawLine.replace(/#.*$/, '').trim()
+    if (!line) continue
+    const tokens = line.split(/\s+/)
+    if (tokens.length < 2) continue
+    const [pattern, ...owners] = tokens
+    rules.push({pattern, owners})
+  }
+  return rules
+}
+
+/**
+ * Convert a CODEOWNERS pattern into a RegExp.
+ *
+ * This is a deliberately conservative translation: it covers the patterns
+ * actually used in this repo (leading `*`, leading `/`, directory globs)
+ * but does not aim for full gitignore compatibility. CODEOWNERS
+ * mis-matches here only affect whether the override is *granted*; the
+ * detection itself is unchanged, so a too-narrow regex just falls back to
+ * "no override" rather than silently approving the wrong thing.
+ */
+export function codeownersPatternToRegExp(pattern) {
+  let p = pattern
+  // A pattern that doesn't start with `/` matches anywhere in the tree.
+  const anchored = p.startsWith('/')
+  if (anchored) p = p.slice(1)
+  // A trailing `/` means "this directory and everything inside it".
+  if (p.endsWith('/')) p = `${p}**`
+  // `**` => any path segments, `*` => anything except `/`.
+  let regex = ''
+  let i = 0
+  while (i < p.length) {
+    const ch = p[i]
+    if (ch === '*' && p[i + 1] === '*') {
+      regex += '.*'
+      i += 2
+      if (p[i] === '/') i++ // consume the `/` after `**`
+    } else if (ch === '*') {
+      regex += '[^/]*'
+      i++
+    } else if ('.+?^$()[]{}|\\'.includes(ch)) {
+      regex += `\\${ch}`
+      i++
+    } else {
+      regex += ch
+      i++
+    }
+  }
+  return new RegExp(anchored ? `^${regex}$` : `(^|/)${regex}$`)
+}
+
+/**
+ * Returns the set of CODEOWNERS owner handles (teams and users, with the
+ * leading `@`) responsible for any of the given changed files. Per
+ * CODEOWNERS semantics the *last* matching rule wins.
+ */
+export function ownersForFiles(rules, files) {
+  const compiled = rules.map((r) => ({...r, regex: codeownersPatternToRegExp(r.pattern)}))
+  const owners = new Set()
+  for (const file of files) {
+    let match = null
+    for (const rule of compiled) {
+      if (rule.regex.test(file)) match = rule
+    }
+    if (match) match.owners.forEach((o) => owners.add(o))
+  }
+  return owners
+}
+
+/**
+ * Checks whether the PR has been approved by anyone who can plausibly
+ * count as a code-owner approval. Returns `{approved: bool, approver: string | null}`.
+ *
+ * "Plausibly" intentionally means "a member of the org with write access
+ * to this repo". The default GITHUB_TOKEN can't list arbitrary team
+ * memberships across the org, but it can read repo-level permissions, and
+ * for Shopify/cli the CODEOWNERS file is `* @shopify/dev_experience`
+ * top-level, with team-only overrides for a couple of paths. Any human
+ * with write access on this repo is in one of those teams (the team grant
+ * is what gives them write access). This is the same security posture as
+ * branch protection's "require code-owner review" rule, just evaluated
+ * earlier so the check can flip green without manually re-running CI.
+ */
+export async function findCodeownerApproval({
+  repo,
+  prNumber,
+  changedFiles,
+  fetchReviews = defaultFetchReviews,
+  fetchCodeowners = defaultFetchCodeowners,
+  fetchPermission = defaultFetchPermission,
+} = {}) {
+  if (!prNumber) return {approved: false, approver: null, reason: 'no PR number in event'}
+
+  const reviews = await fetchReviews(repo, prNumber)
+  if (!reviews) return {approved: false, approver: null, reason: 'reviews API failed'}
+
+  // Last review per author wins (matches GitHub's own "latest review" semantics).
+  const latestByUser = new Map()
+  for (const review of reviews) {
+    if (!review.user?.login) continue
+    latestByUser.set(review.user.login, review)
+  }
+  const approvers = [...latestByUser.values()].filter((r) => r.state === 'APPROVED').map((r) => r.user.login)
+  if (approvers.length === 0) return {approved: false, approver: null, reason: 'no approving reviews'}
+
+  // Optional refinement: if we have the CODEOWNERS file and the changed
+  // files, only count approvers whose teams own a path in the diff. We
+  // log it for transparency but don't gate on it — see the function
+  // doc-comment for why.
+  const codeowners = await fetchCodeowners(repo)
+  if (codeowners && changedFiles && changedFiles.size > 0) {
+    const owners = ownersForFiles(parseCodeowners(codeowners), [...changedFiles])
+    if (owners.size > 0) logMessage(`Code owners for changed files: ${[...owners].join(', ')}`)
+  }
+
+  for (const approver of approvers) {
+    const permission = await fetchPermission(repo, approver)
+    if (permission === 'admin' || permission === 'write' || permission === 'maintain') {
+      return {approved: true, approver, reason: `${approver} has ${permission} access`}
+    }
+  }
+  return {approved: false, approver: null, reason: 'no approving reviewer has write access'}
+}
+
+async function defaultFetchReviews(repo, prNumber) {
+  return githubGet(`https://api.github.com/repos/${repo.owner}/${repo.name}/pulls/${prNumber}/reviews?per_page=100`)
+}
+
+async function defaultFetchCodeowners(repo) {
+  // Try the canonical locations (root, .github, docs).
+  for (const candidate of ['.github/CODEOWNERS', 'CODEOWNERS', 'docs/CODEOWNERS']) {
+    const data = await githubGet(
+      `https://api.github.com/repos/${repo.owner}/${repo.name}/contents/${candidate}`,
+    )
+    if (data?.content) return Buffer.from(data.content, 'base64').toString('utf-8')
+  }
+  return null
+}
+
+async function defaultFetchPermission(repo, username) {
+  const data = await githubGet(
+    `https://api.github.com/repos/${repo.owner}/${repo.name}/collaborators/${username}/permission`,
+  )
+  return data?.permission || null
+}
+
 // ---------------------------------------------------------------------------
 // 1. Check changesets for major bumps
 // ---------------------------------------------------------------------------
@@ -571,7 +760,7 @@ function buildReport({majorChangesets, manifestChanges, schemaChanges}) {
 
 This PR contains changes that may break the existing contract.
 
-**@shopify/dev_experience** — this PR contains breaking changes that require coordination for the next major release.
+**@shopify/dev_experience** — this PR contains breaking changes that require coordination for the next major release. To unblock this check, an approving review from a code owner of the changed files (typically a \`@shopify/dev_experience\` member) is sufficient — the check will re-run automatically when the review is submitted and turn green.
 
 `
 
@@ -680,14 +869,32 @@ async function runMain() {
 
     const report = buildReport({majorChangesets, manifestChanges, schemaChanges})
 
-    if (report) {
-      logSection('\n⚠️  Breaking changes detected!')
-      setOutput('has_breaking_changes', 'true')
-      setOutput('report', report)
-    } else {
+    if (!report) {
       logSection('\n✅ No breaking changes detected')
       setOutput('has_breaking_changes', 'false')
+      return
+    }
+
+    // Breaking changes were detected. If a code-owner has already approved
+    // the PR, treat that as sign-off and turn the check green. The script
+    // re-runs on `pull_request_review` events (see breaking-change-check.yml)
+    // so a fresh approval will flip this check without manual CI re-runs.
+    const override = await findCodeownerApproval({
+      repo: context.repo,
+      prNumber: context.prNumber,
+      changedFiles: context.changedFiles,
+    })
+    if (override.approved) {
+      logSection(`\n✅ Breaking changes detected, but overridden by ${override.approver}'s approval`)
+      setOutput('has_breaking_changes', 'false')
+      setOutput('report', `${report}\n---\n✅ Override: approved by @${override.approver}.\n`)
+      return
     }
+
+    logSection('\n⚠️  Breaking changes detected!')
+    if (override.reason) logMessage(`Override not granted: ${override.reason}`)
+    setOutput('has_breaking_changes', 'true')
+    setOutput('report', report)
   } finally {
     await rm(tmpDir, {recursive: true, force: true, maxRetries: 2})
   }