diff --git a/workspace/src/major-change-check.js b/workspace/src/major-change-check.js index 0baf21178fa..a43e91030b8 100644 --- a/workspace/src/major-change-check.js +++ b/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 | 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}) } + + diff --git a/workspace/src/major-change-check.test.js b/workspace/src/major-change-check.test.js index 1f20037e789..b13ad34bf22 100644 --- a/workspace/src/major-change-check.test.js +++ b/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) +}) 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 } +