From afebd1c384992c32f30e1b09f066ab265bdaf619 Mon Sep 17 00:00:00 2001 From: Jeffrey Chen Date: Fri, 14 Aug 2026 22:30:19 +0000 Subject: [PATCH 1/2] fix(ci): compare registry against the PR merge result The extension registry check compared the latest base registry against the PR branch's own snapshot, so a release merged by a concurrent PR looked like a removal and wrongly demanded core-team review. Resolve the comparison refs as a pair from GitHub's merge preview (`refs/pull//merge`): its first parent is the exact base the preview was built from, and the merge commit is the state the registry would have once the PR lands. Verify the preview's second parent matches the head being evaluated, retry briefly while GitHub recomputes it, and fail closed with recovery guidance otherwise. Fixes #9593 --- .github/scripts/src/ext-registry-check.js | 151 ++++++--- .../scripts/test/ext-registry-check.test.js | 294 ++++++++++++++++-- 2 files changed, 377 insertions(+), 68 deletions(-) diff --git a/.github/scripts/src/ext-registry-check.js b/.github/scripts/src/ext-registry-check.js index 991381c4dad..499bd38eb41 100644 --- a/.github/scripts/src/ext-registry-check.js +++ b/.github/scripts/src/ext-registry-check.js @@ -42,6 +42,11 @@ const ALLOWED_ARTIFACT_URL_PREFIX = `${ALLOWED_ARTIFACT_URL_ORIGIN}${ALLOWED_ART // understand those other fields, just preserve the status quo. const ALLOWED_EXTENSION_METADATA_CHANGES = new Set(['displayName', 'description', 'tags']); +// GitHub recomputes `refs/pull//merge` asynchronously, so it can briefly be missing or +// point at an older PR head. We retry a few times rather than failing the check on that lag. +const MERGE_PREVIEW_ATTEMPTS = 3; +const MERGE_PREVIEW_RETRY_DELAY_MS = 2_000; + // GitHub action types /** @@ -89,12 +94,26 @@ const ALLOWED_EXTENSION_METADATA_CHANGES = new Set(['displayName', 'description' */ /** - * @param {{ github: Octokit, context: Context, core: Core, coreTeam?: Set, registryBaseRef?: string }} args + * The two commits the registry policy compares: the base the PR would merge into, and + * the state the registry would have after that merge. + * + * @typedef {object} RegistryComparisonRefs + * @property {string} baseRef + * @property {string} proposedRef + */ + +/** + * @param {{ + * github: Octokit, + * context: Context, + * core: Core, + * coreTeam?: Set, + * registryComparisonRefs?: RegistryComparisonRefs, + * }} args */ -async function run({ github: octokit, context, core, coreTeam, registryBaseRef }) { +async function run({ github: octokit, context, core, coreTeam, registryComparisonRefs }) { try { assertHasPullRequest(context); - const baseRef = registryBaseRef ?? context.payload.pull_request['base']?.sha ?? 'main'; const coreReviewers = coreTeam ?? getCoreReviewers({ core }); // no extra checks needed if a registry maintainer authored the PR. @@ -116,19 +135,23 @@ async function run({ github: octokit, context, core, coreTeam, registryBaseRef } // Simple release-only registry changes can proceed without core review. Deleted registries // are reported by diffChangedFiles above and skipped here, since there's nothing to fetch - // at the PR head. + // from the proposed merge result. const changedRegistryPaths = [...REGISTRY_JSON_PATHS] .filter((registryPath) => changedFiles.some( (file) => file.filename === registryPath && file.status !== 'removed')); const registryReviewReasons = []; - for (const registryPath of changedRegistryPaths) { - const reasons = await isAllowedRegistryJsonUpdate({ - octokit, - context, - registryBaseRef: baseRef, - registryPath, - }); - registryReviewReasons.push(...reasons.map((reason) => `${registryPath}: ${reason}`)); + if (changedRegistryPaths.length > 0) { + const comparisonRefs = registryComparisonRefs ?? + await getRegistryComparisonRefs({ octokit, context }); + for (const registryPath of changedRegistryPaths) { + const reasons = await isAllowedRegistryJsonUpdate({ + octokit, + context, + registryComparisonRefs: comparisonRefs, + registryPath, + }); + registryReviewReasons.push(...reasons.map((reason) => `${registryPath}: ${reason}`)); + } } const reviewReasons = changedFileReviewReasons.concat(registryReviewReasons); @@ -256,41 +279,93 @@ function isCreatedByCoreTeam({ context, core, coreTeam }) { /** * Checks whether the registry update is simple enough to proceed without core-team review. * - * @param {{ octokit: Octokit, context: Context, registryPath: string, registryBaseRef: string }} args + * Both sides are read from the base repository at a matched pair of commits, so the + * comparison describes exactly what this PR changes, and isn't skewed by registry updates + * that landed on the base branch after the PR branch was created. + * + * @param {{ + * octokit: Octokit, + * context: Context, + * registryPath: string, + * registryComparisonRefs: RegistryComparisonRefs, + * }} args * @returns {Promise} the reasons core review is needed; empty means the change is approved */ async function isAllowedRegistryJsonUpdate({ octokit, context, registryPath, - registryBaseRef, + registryComparisonRefs, }) { + const [baseRegistry, proposedRegistry] = await Promise.all([ + getRegistryJson({ octokit, ...context.repo, ref: registryComparisonRefs.baseRef, registryPath }), + getRegistryJson({ octokit, ...context.repo, ref: registryComparisonRefs.proposedRef, registryPath }), + ]); + + return diffRegistry(baseRegistry, proposedRegistry); +} + +/** + * Resolves the commits the registry policy should compare, using GitHub's synthetic merge + * commit (`refs/pull//merge`) as the proposed state. + * + * The refs are deliberately taken as a pair: the merge commit's first parent is the exact + * base the preview was built from, so the diff always describes what this PR changes, + * never what other PRs merged in the meantime. + * + * The second parent must match the head this workflow run is evaluating, otherwise the + * preview is stale and could approve content we never looked at. + * + * @param {{ octokit: Octokit, context: Context }} args + * @returns {Promise} + */ +async function getRegistryComparisonRefs({ octokit, context }) { assertHasPullRequest(context); - const pr = context.payload.pull_request; - - const mainRegistry = await getRegistryJson({ - octokit, - owner: context.repo.owner, - repo: context.repo.repo, - ref: registryBaseRef, - registryPath, - }); - const head = pr['head']; - const ref = head?.sha ?? head?.ref; - if (!ref) { - throw new Error('Unable to determine PR head ref for registry.json update check'); + const headSha = context.payload.pull_request['head']?.sha; + if (!headSha) { + throw new Error('Unable to determine PR head commit for registry.json update check'); } - const prRegistry = await getRegistryJson({ - octokit, - owner: head?.repo?.owner?.login ?? context.repo.owner, - repo: head?.repo?.name ?? context.repo.repo, - ref, - registryPath, - }); + const mergeRef = `refs/pull/${context.payload.pull_request.number}/merge`; + /** @type {Error | undefined} */ + let lastError; + + for (let attempt = 1; attempt <= MERGE_PREVIEW_ATTEMPTS; attempt++) { + if (attempt > 1) { + await new Promise((resolve) => setTimeout(resolve, MERGE_PREVIEW_RETRY_DELAY_MS)); + } + + try { + const { data: mergeCommit } = await octokit.rest.repos.getCommit({ + ...context.repo, + ref: mergeRef, + }); + const [baseParent, headParent] = mergeCommit.parents; - return diffRegistry(mainRegistry, prRegistry); + if (!baseParent?.sha || !headParent?.sha) { + lastError = new Error(`GitHub's merge preview for this PR is not a two-parent merge commit`); + continue; + } + + if (headParent.sha !== headSha) { + lastError = new Error(`GitHub's merge preview is stale for the current PR head (${headSha})`); + continue; + } + + return { + baseRef: baseParent.sha, + proposedRef: mergeCommit.sha, + }; + } catch (err) { + lastError = err instanceof Error ? err : new Error(String(err)); + } + } + + throw new Error( + `Unable to load a current GitHub merge preview for this PR after ${MERGE_PREVIEW_ATTEMPTS} attempts. ` + + `Resolve any merge conflicts or re-run the check after GitHub computes the preview: ${lastError?.message}`, + ); } /** @@ -416,15 +491,15 @@ async function getRegistryJson({ octokit, owner, repo, ref, registryPath }) { } /** - * Diffs the base (main) registry against the registry proposed by a PR and decides + * Diffs the base-branch registry against the registry the PR would produce, and decides * whether the change can proceed without core review, or whether a core reviewer * needs to review it. * * New releases can proceed without core review when they keep the previous release's * capabilities and providers, and only add a new release to an existing extension. * - * @param {RegistryJson} baseRegistry registry.json as it exists on main - * @param {RegistryJson} prRegistry registry.json as proposed by the PR + * @param {RegistryJson} baseRegistry registry.json as it exists on the base branch + * @param {RegistryJson} prRegistry registry.json as it would exist once the PR merges * @returns {string[]} the reasons core review is needed; empty means the change is approved */ function diffRegistry(baseRegistry, prRegistry) { diff --git a/.github/scripts/test/ext-registry-check.test.js b/.github/scripts/test/ext-registry-check.test.js index eae7fcc9faf..edf9f701873 100644 --- a/.github/scripts/test/ext-registry-check.test.js +++ b/.github/scripts/test/ext-registry-check.test.js @@ -45,6 +45,12 @@ const DEV_REGISTRY_PATH = 'cli/azd/extensions/registry.dev.json'; const FIG_SPEC_SNAPSHOT_PATH = 'cli/azd/cmd/testdata/TestFigSpec.ts'; const REGISTRY_PATH_LIST = `${PROD_REGISTRY_PATH}, ${DEV_REGISTRY_PATH}`; +// The three commits the fixtures model: the PR head from the event payload, the base branch +// tip the merge preview was built from, and GitHub's synthetic merge commit. +const HEAD_SHA = 'abc123'; +const MERGE_BASE_SHA = 'current-base'; +const MERGE_RESULT_SHA = 'merge-result'; + /** * @param {object} [opts] * @param {string[]} [opts.capabilities] @@ -517,7 +523,7 @@ describe('diffRegistry', () => { }); describe('isAllowedRegistryJsonUpdate', () => { - it('loads main and PR registry.json and applies the registry policy', async () => { + it('loads the base and proposed registry.json and applies the registry policy', async () => { const base = registry([extension({ id: 'ext.one' })]); const pr = registry([{ ...extension({ id: 'ext.one' }), namespace: 'other' }]); const octokit = createRegistryOctokit({ base, pr }); @@ -527,7 +533,10 @@ describe('isAllowedRegistryJsonUpdate', () => { octokit, context, registryPath: PROD_REGISTRY_PATH, - registryBaseRef: 'main', + registryComparisonRefs: { + baseRef: 'main', + proposedRef: MERGE_RESULT_SHA, + }, })).resolves.toContainEqual(expect.stringContaining('changes metadata that requires core review')); expect(octokit.rest.repos.getContent).toHaveBeenCalledWith(expect.objectContaining({ owner: 'Azure', @@ -535,12 +544,95 @@ describe('isAllowedRegistryJsonUpdate', () => { ref: 'main', })); expect(octokit.rest.repos.getContent).toHaveBeenCalledWith(expect.objectContaining({ - owner: 'fork-owner', - repo: 'azure-dev-fork', - ref: 'abc123', + owner: 'Azure', + repo: 'azure-dev', + ref: MERGE_RESULT_SHA, + })); + }); + + it('approves a release update when the base has an unrelated concurrently merged release', async () => { + const base = registry([ + extension({ + id: 'ext.one', + versions: [version({ version: '1.0.0' })], + }), + extension({ + id: 'ext.concurrent', + versions: [version({ version: '2.0.0' })], + }), + ]); + const merged = registry([ + extension({ + id: 'ext.one', + versions: [ + version({ version: '1.0.0' }), + version({ version: '1.1.0' }), + ], + }), + extension({ + id: 'ext.concurrent', + versions: [version({ version: '2.0.0' })], + }), + ]); + const staleHead = registry([ + extension({ + id: 'ext.one', + versions: [ + version({ version: '1.0.0' }), + version({ version: '1.1.0' }), + ], + }), + ]); + const octokit = createRegistryOctokit({ base, pr: merged, head: staleHead }); + + await expect(isAllowedRegistryJsonUpdate({ + octokit, + context: createRegistryContext(), + registryPath: PROD_REGISTRY_PATH, + registryComparisonRefs: { + baseRef: MERGE_BASE_SHA, + proposedRef: MERGE_RESULT_SHA, + }, + })).resolves.toEqual([]); + expect(octokit.rest.repos.getContent).toHaveBeenCalledWith(expect.objectContaining({ + ref: MERGE_BASE_SHA, + })); + expect(octokit.rest.repos.getContent).toHaveBeenCalledWith(expect.objectContaining({ + ref: MERGE_RESULT_SHA, })); }); + it('requires review when the synthetic merge result removes a published release', async () => { + const base = registry([ + extension({ + id: 'ext.one', + versions: [ + version({ version: '1.0.0' }), + version({ version: '1.1.0' }), + ], + }), + ]); + const merged = registry([ + extension({ + id: 'ext.one', + versions: [version({ version: '1.0.0' })], + }), + ]); + const octokit = createRegistryOctokit({ base, pr: merged }); + + await expect(isAllowedRegistryJsonUpdate({ + octokit, + context: createRegistryContext(), + registryPath: PROD_REGISTRY_PATH, + registryComparisonRefs: { + baseRef: 'main', + proposedRef: MERGE_RESULT_SHA, + }, + })).resolves.toContainEqual( + expect.stringContaining("release '1.1.0' was removed; published releases are immutable"), + ); + }); + it('can load the base registry from a supplied commit-ish', async () => { const base = registry([extension({ id: 'ext.one' })]); const pr = registry([{ ...extension({ id: 'ext.one' }), namespace: 'other' }]); @@ -551,7 +643,10 @@ describe('isAllowedRegistryJsonUpdate', () => { octokit, context, registryPath: PROD_REGISTRY_PATH, - registryBaseRef: 'base-before-pr', + registryComparisonRefs: { + baseRef: 'base-before-pr', + proposedRef: MERGE_RESULT_SHA, + }, })).resolves.toContainEqual(expect.stringContaining('changes metadata that requires core review')); expect(octokit.rest.repos.getContent).toHaveBeenCalledWith(expect.objectContaining({ owner: 'Azure', @@ -569,7 +664,10 @@ describe('isAllowedRegistryJsonUpdate', () => { octokit, context: createRegistryContext(), registryPath: DEV_REGISTRY_PATH, - registryBaseRef: 'main', + registryComparisonRefs: { + baseRef: 'main', + proposedRef: MERGE_RESULT_SHA, + }, })).resolves.toContainEqual(expect.stringContaining('changes metadata that requires core review')); expect(octokit.rest.repos.getContent).toHaveBeenCalledWith(expect.objectContaining({ path: 'cli/azd/extensions/registry.dev.json', @@ -585,7 +683,10 @@ describe('isAllowedRegistryJsonUpdate', () => { octokit, context: createRegistryContext(), registryPath: PROD_REGISTRY_PATH, - registryBaseRef: 'main', + registryComparisonRefs: { + baseRef: 'main', + proposedRef: MERGE_RESULT_SHA, + }, }); expect(reasons).toContainEqual(expect.stringContaining('changes capabilities')); @@ -984,7 +1085,7 @@ describe('run', () => { expect(octokit.rest.repos.getContent).not.toHaveBeenCalled(); }); - it('uses the pull request base sha as the registry comparison base by default', async () => { + it('uses the merge preview base as the registry comparison base by default', async () => { const core = createNoopCore(); const octokit = createRegistryOctokit({ base: registry([extension()]), pr: registry([extension()]) }); const context = createRegistryContext(); @@ -1000,10 +1101,76 @@ describe('run', () => { expect(octokit.rest.repos.getContent).toHaveBeenCalledWith(expect.objectContaining({ owner: 'Azure', repo: 'azure-dev', - ref: 'base-before-pr', + ref: MERGE_BASE_SHA, + })); + expect(octokit.rest.repos.getContent).toHaveBeenCalledWith(expect.objectContaining({ + owner: 'Azure', + repo: 'azure-dev', + ref: MERGE_RESULT_SHA, })); }); + it('fails closed when GitHub returns a merge preview for an older PR head', async () => { + const core = createNoopCore(); + const octokit = createRegistryOctokit({ + base: registry([extension()]), + pr: registry([extension()]), + mergeHeadShas: ['older-head'], + }); + + await runWithoutRetryDelay({ + github: octokit, + context: createRegistryContext(), + core, + coreTeam: new Set(['core-member']), + }); + + expect(core.setFailed).toHaveBeenCalledWith( + expect.stringContaining(`GitHub's merge preview is stale for the current PR head`), + ); + expect(octokit.rest.repos.getContent).not.toHaveBeenCalled(); + }); + + it('retries until GitHub returns a merge preview for the current PR head', async () => { + const core = createNoopCore(); + const octokit = createRegistryOctokit({ + base: registry([extension()]), + pr: registry([extension()]), + mergeHeadShas: ['older-head', HEAD_SHA], + }); + + await runWithoutRetryDelay({ + github: octokit, + context: createRegistryContext(), + core, + coreTeam: new Set(['core-member']), + }); + + expect(core.setFailed).not.toHaveBeenCalled(); + expect(octokit.rest.repos.getCommit).toHaveBeenCalledTimes(2); + }); + + it('reports how to recover when GitHub has no merge preview', async () => { + const core = createNoopCore(); + const octokit = createRegistryOctokit({ + base: registry([extension()]), + pr: registry([extension()]), + mergeError: new Error('Not Found'), + }); + + await runWithoutRetryDelay({ + github: octokit, + context: createRegistryContext(), + core, + coreTeam: new Set(['core-member']), + }); + + expect(core.setFailed).toHaveBeenCalledWith( + expect.stringContaining('Resolve any merge conflicts or re-run the check'), + ); + expect(octokit.rest.repos.getContent).not.toHaveBeenCalled(); + }); + it('requires review when the PR changes files outside the extension registries', async () => { const core = createNoopCore(); const octokit = createRegistryOctokit({ @@ -1174,19 +1341,36 @@ describe('REGISTRY_JSON_PATHS', () => { * @param {{ * base?: RegistryJson, * pr?: RegistryJson, + * head?: RegistryJson, * registries?: Object, * files?: { filename: string, previous_filename?: string, status?: string }[], * reviews?: { user: { login: string }, state: string, commit_id: string }[], + * mergeHeadShas?: string[], + * mergeError?: Error, * }} args * @returns {Octokit} */ -function createRegistryOctokit({ base, pr, registries, files = [{ filename: PROD_REGISTRY_PATH }], reviews = [] }) { +function createRegistryOctokit({ + base, + pr, + head = pr, + registries, + files = [{ filename: PROD_REGISTRY_PATH }], + reviews = [], + mergeHeadShas = [HEAD_SHA], + mergeError, +}) { + let mergeAttempt = 0; // Per-path fixtures let a test drive each registry independently. When they're not // supplied, every registry path shares the same base/pr content. - /** @type {Record} */ + /** @type {Record} */ const registriesByPath = registries ?? { - [PROD_REGISTRY_PATH]: { base, pr }, - [DEV_REGISTRY_PATH]: { base, pr }, + [PROD_REGISTRY_PATH]: { base, pr, head }, + [DEV_REGISTRY_PATH]: { base, pr, head }, }; const octokit = { @@ -1196,14 +1380,38 @@ function createRegistryOctokit({ base, pr, registries, files = [{ filename: PROD listFiles: vi.fn(), }, repos: { + getCommit: vi.fn(() => { + if (mergeError) { + return Promise.reject(mergeError); + } + + // Each call walks one step further into `mergeHeadShas`, sticking on the last + // entry, so a test can model a merge preview that refreshes between attempts. + const headParentSha = mergeHeadShas[Math.min(mergeAttempt++, mergeHeadShas.length - 1)]; + + return Promise.resolve({ + data: { + sha: MERGE_RESULT_SHA, + parents: [{ sha: MERGE_BASE_SHA }, { sha: headParentSha }], + }, + }); + }), getContent: vi.fn(({ path, ref }) => { const fixture = registriesByPath[path]; if (fixture == null) { throw new Error(`No registry fixture configured for ${path}`); } + // Three distinct states: the base branch tip, the (possibly stale) PR head, and + // the merge result the check is supposed to evaluate. + const registryForRef = ref === MERGE_RESULT_SHA + ? fixture.pr + : ref === HEAD_SHA + ? fixture.head + : fixture.base; + return Promise.resolve({ - data: JSON.stringify(ref === 'abc123' ? fixture.pr : fixture.base), + data: JSON.stringify(registryForRef), }); }), }, @@ -1237,7 +1445,7 @@ function createRegistryContext({ author = 'contributor' } = {}) { number: 1, base: { sha: 'base-before-pr' }, head: { - sha: 'abc123', + sha: HEAD_SHA, repo: { name: 'azure-dev-fork', owner: { login: 'fork-owner' }, @@ -1249,6 +1457,23 @@ function createRegistryContext({ author = 'contributor' } = {}) { })); } +/** + * Runs the check under fake timers so the merge-preview retry backoff resolves immediately. + * + * @param {Parameters[0]} args + */ +async function runWithoutRetryDelay(args) { + vi.useFakeTimers(); + + try { + const runPromise = run(args); + await vi.runAllTimersAsync(); + await runPromise; + } finally { + vi.useRealTimers(); + } +} + const LIVE_TEST_OWNER = 'Azure'; const LIVE_TEST_REPO = 'azure-dev'; const RUN_LIVE_TESTS = process.env['RUN_LIVE_TESTS'] === '1'; @@ -1330,15 +1555,18 @@ async function createLiveContext(octokit, prNumber, { owner = LIVE_TEST_OWNER, r // to try against the real deal, with a real octokit instance. liveDescribe('[live] registry diff PR scenarios', () => { /** - * Returns the base-branch commit to compare against for the live PR sample. - * Live samples are intentionally limited to closed-unmerged PRs and - * squash-merged PRs. + * Returns the registry comparison refs for the live PR sample. + * Live samples are intentionally limited to closed-unmerged PRs and + * squash-merged PRs. * * @param {Octokit} octokit * @param {number} prNumber - * @returns {Promise} + * @returns {Promise<{ + * baseRef: string, + * proposedRef: string, + * }>} */ - async function getLiveRegistryBaseRef(octokit, prNumber) { + async function getLiveRegistryComparisonRefs(octokit, prNumber) { const { data: pr } = await octokit.rest.pulls.get({ owner: LIVE_TEST_OWNER, repo: LIVE_TEST_REPO, @@ -1350,11 +1578,14 @@ liveDescribe('[live] registry diff PR scenarios', () => { } if (pr.merged_at == null) { - if (!pr.base.sha) { - throw new Error(`Unable to determine the base commit for PR ${prNumber}`); + if (!pr.base.sha || !pr.head.sha) { + throw new Error(`Unable to determine the comparison refs for PR ${prNumber}`); } - return pr.base.sha; + return { + baseRef: pr.base.sha, + proposedRef: pr.head.sha, + }; } if (!pr.merge_commit_sha) { @@ -1376,7 +1607,10 @@ liveDescribe('[live] registry diff PR scenarios', () => { throw new Error(`Unable to determine the base commit before PR ${prNumber}`); } - return parent.sha; + return { + baseRef: parent.sha, + proposedRef: pr.merge_commit_sha, + }; } @@ -1386,13 +1620,13 @@ liveDescribe('[live] registry diff PR scenarios', () => { async function runTestAgainstLivePr(sample) { const octokit = await createLiveOctokit(); const context = await createLiveContext(octokit, sample.number); - const registryBaseRef = await getLiveRegistryBaseRef(octokit, sample.number); + const registryComparisonRefs = await getLiveRegistryComparisonRefs(octokit, sample.number); const core = createNoopCore(); if (sample.coreTeam) { - await run({ github: octokit, context, core, coreTeam: sample.coreTeam, registryBaseRef }); + await run({ github: octokit, context, core, coreTeam: sample.coreTeam, registryComparisonRefs }); } else { - await run({ github: octokit, context, core, registryBaseRef }); + await run({ github: octokit, context, core, registryComparisonRefs }); } if (sample.noReviewRequired) { @@ -1427,8 +1661,8 @@ liveDescribe('[live] registry diff PR scenarios', () => { }, 90_000); // https://github.com/Azure/azure-dev/pull/8972 - it('[live] PR 8972 => core review required because the PR changes another file', async () => { - await runTestAgainstLivePr({ number: 8972, noReviewRequired: false }); + it('[live] PR 8972 => no review required for an allowed TestFigSpec snapshot update', async () => { + await runTestAgainstLivePr({ number: 8972, noReviewRequired: true }); }, 90_000); }); From 04e906aa03ae8861b36fb95c9e748cc0ad4b194f Mon Sep 17 00:00:00 2001 From: Jeffrey Chen Date: Fri, 14 Aug 2026 23:02:17 +0000 Subject: [PATCH 2/2] fix(ci): report merge-preview failures as PR issues A pull request whose merge preview can't be resolved is almost always conflicting, which is the contributor's to fix, but the failure surfaced as "Internal failure in script" and read like the check itself was broken. Raise a distinct error for an unavailable or permanently stale merge preview and report its message directly, leaving the internal-failure wrapper for genuine script bugs. --- .github/scripts/src/ext-registry-check.js | 22 ++++++++++++++++++- .../scripts/test/ext-registry-check.test.js | 2 ++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/.github/scripts/src/ext-registry-check.js b/.github/scripts/src/ext-registry-check.js index 499bd38eb41..2b953ebb27f 100644 --- a/.github/scripts/src/ext-registry-check.js +++ b/.github/scripts/src/ext-registry-check.js @@ -47,6 +47,19 @@ const ALLOWED_EXTENSION_METADATA_CHANGES = new Set(['displayName', 'description' const MERGE_PREVIEW_ATTEMPTS = 3; const MERGE_PREVIEW_RETRY_DELAY_MS = 2_000; +/** + * Raised when GitHub can't give us a merge preview that matches the head being evaluated. + * Usually the pull request has merge conflicts, so the failure is reported to the contributor + * directly instead of being labelled an internal script error. + */ +class MergePreviewUnavailableError extends Error { + /** @param {string} message */ + constructor(message) { + super(message); + this.name = 'MergePreviewUnavailableError'; + } +} + // GitHub action types /** @@ -169,6 +182,13 @@ async function run({ github: octokit, context, core, coreTeam, registryCompariso `2. After approval, re-run this build step so it'll re-evaluate the PR - no commits or pushes needed.` ); } catch (err) { + // A missing or stale merge preview is normally the contributor's PR to fix (most often a + // merge conflict), so it's reported as-is rather than as a script bug. + if (err instanceof MergePreviewUnavailableError) { + core.setFailed(err.message); + return; + } + core.setFailed(`Internal failure in script: ${err instanceof Error ? err.message : err}`); } } @@ -362,7 +382,7 @@ async function getRegistryComparisonRefs({ octokit, context }) { } } - throw new Error( + throw new MergePreviewUnavailableError( `Unable to load a current GitHub merge preview for this PR after ${MERGE_PREVIEW_ATTEMPTS} attempts. ` + `Resolve any merge conflicts or re-run the check after GitHub computes the preview: ${lastError?.message}`, ); diff --git a/.github/scripts/test/ext-registry-check.test.js b/.github/scripts/test/ext-registry-check.test.js index edf9f701873..ceaacd5e76a 100644 --- a/.github/scripts/test/ext-registry-check.test.js +++ b/.github/scripts/test/ext-registry-check.test.js @@ -1128,6 +1128,7 @@ describe('run', () => { expect(core.setFailed).toHaveBeenCalledWith( expect.stringContaining(`GitHub's merge preview is stale for the current PR head`), ); + expect(core.setFailed).not.toHaveBeenCalledWith(expect.stringContaining('Internal failure in script')); expect(octokit.rest.repos.getContent).not.toHaveBeenCalled(); }); @@ -1168,6 +1169,7 @@ describe('run', () => { expect(core.setFailed).toHaveBeenCalledWith( expect.stringContaining('Resolve any merge conflicts or re-run the check'), ); + expect(core.setFailed).not.toHaveBeenCalledWith(expect.stringContaining('Internal failure in script')); expect(octokit.rest.repos.getContent).not.toHaveBeenCalled(); });