diff --git a/.github/workflows/breaking-change-check.yml b/.github/workflows/breaking-change-check.yml new file mode 100644 index 0000000000..d268fdf19f --- /dev/null +++ b/.github/workflows/breaking-change-check.yml @@ -0,0 +1,75 @@ +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 }} + fetch-depth: 1 + - 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 diff --git a/.github/workflows/tests-pr.yml b/.github/workflows/tests-pr.yml index 6b8c4fc502..2d0e11896d 100644 --- a/.github/workflows/tests-pr.yml +++ b/.github/workflows/tests-pr.yml @@ -295,41 +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 }} - fetch-depth: 1 - - 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. diff --git a/workspace/src/major-change-check.js b/workspace/src/major-change-check.js index 03000746a8..671667c351 100644 --- a/workspace/src/major-change-check.js +++ b/workspace/src/major-change-check.js @@ -109,22 +109,185 @@ export async function resolveContext({ 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(`compare API returned ${res.status} — falling back to full scan`) + logMessage(`GitHub API ${url} returned ${res.status}`) return null } return await res.json() } catch (error) { - logMessage(`compare API failed: ${error.message} — falling back to full scan`) + 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 // --------------------------------------------------------------------------- @@ -593,7 +756,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. > 💬 Head to [#help-dev-platform](https://shopify.enterprise.slack.com/archives/C07UJ7UNMTK) to discuss timing and plan the release. @@ -704,14 +867,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}) } diff --git a/workspace/src/major-change-check.test.js b/workspace/src/major-change-check.test.js index b13ad34bf2..1686d17c04 100644 --- a/workspace/src/major-change-check.test.js +++ b/workspace/src/major-change-check.test.js @@ -16,7 +16,15 @@ import {mkdtemp, writeFile, rm} from 'node:fs/promises' import {tmpdir} from 'node:os' import {join} from 'node:path' -import {extractSchemaFields, resolveContext, stripStringsAndComments} from './major-change-check.js' +import { + codeownersPatternToRegExp, + extractSchemaFields, + findCodeownerApproval, + ownersForFiles, + parseCodeowners, + resolveContext, + stripStringsAndComments, +} from './major-change-check.js' test('extracts top-level keys from a flat .object({...})', () => { const src = ` @@ -200,3 +208,118 @@ test('resolveContext: no event payload falls back to scanning main', async () => assert.equal(ctx.baselineRef, 'main') assert.equal(ctx.changedFiles, null) }) + +// --------------------------------------------------------------------------- +// CODEOWNERS parsing & matching +// --------------------------------------------------------------------------- + +test('parseCodeowners strips comments and blank lines', () => { + const content = ` +# top of file +* @shopify/dev_experience + +# section +/.github/CODEOWNERS @shopify/developer-platforms @shopify/dev_experience +packages/theme/** @shopify/theme # inline comment +` + const rules = parseCodeowners(content) + assert.deepEqual(rules, [ + {pattern: '*', owners: ['@shopify/dev_experience']}, + {pattern: '/.github/CODEOWNERS', owners: ['@shopify/developer-platforms', '@shopify/dev_experience']}, + {pattern: 'packages/theme/**', owners: ['@shopify/theme']}, + ]) +}) + +test('codeownersPatternToRegExp matches like CODEOWNERS does', () => { + // `*` covers everything + assert.match('packages/app/foo.ts', codeownersPatternToRegExp('*')) + + // Anchored path + const cliRule = codeownersPatternToRegExp('/.github/CODEOWNERS') + assert.match('.github/CODEOWNERS', cliRule) + assert.doesNotMatch('foo/.github/CODEOWNERS', cliRule) + + // Directory glob + const themeRule = codeownersPatternToRegExp('packages/theme/**') + assert.match('packages/theme/index.ts', themeRule) + assert.match('packages/theme/sub/dir/file.ts', themeRule) + assert.doesNotMatch('packages/app/foo.ts', themeRule) +}) + +test('ownersForFiles: last matching rule wins (CODEOWNERS semantics)', () => { + const rules = parseCodeowners(` +* @shopify/dev_experience +packages/theme/** @shopify/theme +`) + const themeOwners = ownersForFiles(rules, ['packages/theme/foo.ts']) + assert.deepEqual([...themeOwners], ['@shopify/theme'], 'theme rule overrides catch-all') + + const appOwners = ownersForFiles(rules, ['packages/app/foo.ts']) + assert.deepEqual([...appOwners], ['@shopify/dev_experience'], 'falls through to catch-all') +}) + +// --------------------------------------------------------------------------- +// findCodeownerApproval() +// --------------------------------------------------------------------------- + +const fakeRepo = {owner: 'Shopify', name: 'cli'} + +test('findCodeownerApproval: approver with write access overrides', async () => { + const result = await findCodeownerApproval({ + repo: fakeRepo, + prNumber: 1, + changedFiles: new Set(['packages/app/foo.ts']), + fetchReviews: async () => [ + {state: 'COMMENTED', user: {login: 'someone'}}, + {state: 'APPROVED', user: {login: 'isaac'}}, + ], + fetchCodeowners: async () => '* @shopify/dev_experience', + fetchPermission: async (_repo, user) => (user === 'isaac' ? 'write' : 'read'), + }) + assert.equal(result.approved, true) + assert.equal(result.approver, 'isaac') +}) + +test('findCodeownerApproval: latest review per author wins (changes_requested cancels earlier approval)', async () => { + const result = await findCodeownerApproval({ + repo: fakeRepo, + prNumber: 2, + changedFiles: new Set(['packages/app/foo.ts']), + fetchReviews: async () => [ + {state: 'APPROVED', user: {login: 'isaac'}, submitted_at: '2025-01-01'}, + {state: 'CHANGES_REQUESTED', user: {login: 'isaac'}, submitted_at: '2025-01-02'}, + ], + fetchCodeowners: async () => '* @shopify/dev_experience', + fetchPermission: async () => 'write', + }) + assert.equal(result.approved, false, "withdrawn approval should not count") +}) + +test('findCodeownerApproval: no approving reviewer with write access => not approved', async () => { + const result = await findCodeownerApproval({ + repo: fakeRepo, + prNumber: 3, + changedFiles: new Set(['packages/app/foo.ts']), + fetchReviews: async () => [{state: 'APPROVED', user: {login: 'external'}}], + fetchCodeowners: async () => '* @shopify/dev_experience', + fetchPermission: async () => 'read', + }) + assert.equal(result.approved, false) + assert.equal(result.approver, null) +}) + +test('findCodeownerApproval: missing PR number bails immediately', async () => { + const result = await findCodeownerApproval({repo: fakeRepo, prNumber: null}) + assert.equal(result.approved, false) + assert.match(result.reason, /no PR number/) +}) + +test('findCodeownerApproval: reviews API failure bails (does not auto-approve)', async () => { + const result = await findCodeownerApproval({ + repo: fakeRepo, + prNumber: 4, + changedFiles: new Set(['x']), + fetchReviews: async () => null, + }) + assert.equal(result.approved, false, 'API failure must NOT silently grant override') +})