From a498b35bad4f513b38b184c46225192ebbd5eeaf Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Tue, 11 Aug 2026 16:42:56 -0700 Subject: [PATCH] fix(github): fail closed when stack metadata is unavailable (#13866) * fix(github): fail closed on unavailable stack metadata * fix(github): validate REST pull request response shape * fix(github): allow omitted stack metadata * test(github): cover enterprise stack probe failure * test(github): cover null ordinary stack metadata --------- Co-authored-by: E2E Test (cherry picked from commit 9f0bf39b04fa1f567157eb6817822ea681073996) --- .../github/client-pr-local-runtime.test.ts | 18 + src/main/github/client.test.ts | 437 +++++++++++++++++- src/main/github/client.ts | 94 +++- 3 files changed, 527 insertions(+), 22 deletions(-) diff --git a/src/main/github/client-pr-local-runtime.test.ts b/src/main/github/client-pr-local-runtime.test.ts index 0e7d4013d2e..543d6f239f2 100644 --- a/src/main/github/client-pr-local-runtime.test.ts +++ b/src/main/github/client-pr-local-runtime.test.ts @@ -234,6 +234,15 @@ describe('GitHub PR local runtime routing', () => { stdout: JSON.stringify({ id: 13, node_id: 'PRRC_inline_13', user: null, body: 'Inline' }) } } + if (args.length === 2 && endpoint === 'repos/acme/orca/pulls/7') { + return { + stdout: JSON.stringify({ + number: 7, + head: { ref: 'feature', sha: 'head-oid' }, + base: { ref: 'main', sha: 'base-oid' } + }) + } + } return { stdout: '', stderr: '' } }) @@ -467,6 +476,15 @@ describe('GitHub PR local runtime routing', () => { if (endpoint.endsWith('/check-runs/88/annotations?per_page=20')) { return { stdout: '[]' } } + if (args.length === 2 && endpoint === 'repos/team/orca/pulls/7') { + return { + stdout: JSON.stringify({ + number: 7, + head: { ref: 'feature', sha: 'head-sha' }, + base: { ref: 'main', sha: 'base-sha' } + }) + } + } if (query) { return { stdout: JSON.stringify({ data: { repository: {} } }) } } diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index b462b77a9cd..d139c345b59 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -1,6 +1,7 @@ /* oxlint-disable max-lines -- Why: GitHub client fixtures cover local and SSH repo identity paths in one suite so mocked CLI behavior stays consistent. */ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type * as GithubApiRepositoryModule from './github-api-repository' +import type * as GitHubEnterpriseRepositoryModule from './github-enterprise-repository' type RateLimitGuardResult = | { blocked: false } @@ -103,6 +104,11 @@ vi.mock('./local-git-config-signature', () => ({ readLocalGitConfigSignature: readLocalGitConfigSignatureMock })) +vi.mock('./github-enterprise-repository', async (importOriginal) => ({ + ...(await importOriginal()), + isGitHubHostAuthenticated: vi.fn().mockResolvedValue(true) +})) + vi.mock('./rate-limit', () => ({ getRateLimit: getRateLimitMock, rateLimitGuard: rateLimitGuardMock, @@ -4143,6 +4149,8 @@ describe('GitHub GraphQL rate-limit guard', () => { __resetPRConflictSummaryCachesForTests() }) + afterEach(() => vi.restoreAllMocks()) + it('skips PR review-thread GraphQL fetch while preserving REST comments', async () => { rateLimitGuardMock.mockImplementation(((bucket: string) => bucket === 'graphql' @@ -4479,20 +4487,38 @@ describe('GitHub GraphQL rate-limit guard', () => { expect(mergeQueueMetadataCall?.[0]).toEqual(expect.arrayContaining(['-f', 'branch=main'])) }) - it('uses async merge only for GitHub-registered stacks', async () => { + const validStackHeadSha = 'a'.repeat(40) + const validStackBaseSha = 'b'.repeat(40) + const validSha256HeadSha = 'c'.repeat(64) + + it.each([ + { + objectFormat: 'SHA-1 with a base SHA', + headSha: validStackHeadSha, + baseSha: validStackBaseSha + }, + { + objectFormat: 'SHA-256 without a base SHA', + headSha: validSha256HeadSha, + baseSha: undefined + } + ])('uses async merge only for GitHub-registered stacks using $objectFormat', async (scenario) => { ghExecFileAsyncMock .mockResolvedValueOnce({ stdout: JSON.stringify({ number: 202, title: 'Stack API', state: 'open', - head: { ref: 'stack/api', sha: 'api-sha' }, + head: { ref: 'stack/api', sha: scenario.headSha }, base: { ref: 'stack/models', sha: 'models-sha' }, stack: { number: 51, position: 2, size: 2, - base: { ref: 'main', sha: 'main-sha' } + base: { + ref: 'main', + ...(scenario.baseSha ? { sha: scenario.baseSha } : {}) + } } }) }) @@ -4519,7 +4545,7 @@ describe('GitHub GraphQL rate-limit guard', () => { 'PUT', 'repos/stablyai/orca/pulls/202/merge-async', 'merge_action=merge_queue', - 'sha=api-sha' + `sha=${scenario.headSha}` ]) ) expect(mergeCall?.[0]).not.toContain('merge_method=squash') @@ -4536,13 +4562,13 @@ describe('GitHub GraphQL rate-limit guard', () => { stdout: JSON.stringify({ number: 202, state: 'open', - head: { ref: 'stack/api', sha: 'api-sha' }, + head: { ref: 'stack/api', sha: validStackHeadSha }, base: { ref: 'stack/models', sha: 'models-sha' }, stack: { number: 51, position: 2, size: 2, - base: { ref: 'main', sha: 'main-sha' } + base: { ref: 'main', sha: validStackBaseSha } } }) }) @@ -4563,6 +4589,403 @@ describe('GitHub GraphQL rate-limit guard', () => { ).toBe(false) }) + const stackMetadataUnavailableError = + 'Could not verify GitHub pull request stack metadata. Refresh and try again.' + + it.each([ + { + failure: 'local transport failure', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: new Error('stack metadata unavailable'), + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'stack metadata unavailable', + expectedOptions: { cwd: '/repo-root', host: 'github.com' } + }, + { + failure: 'GitHub Enterprise transport failure', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: new Error('enterprise stack metadata unavailable'), + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'enterprise stack metadata unavailable', + expectedOptions: { cwd: '/repo-root', host: 'github.enterprise.test' } + }, + { + failure: 'unparsable probe response over SSH', + repoPath: '/remote/repo-root', + connectionId: 'ssh-1', + probeResponse: { stdout: '' }, + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'invalid JSON response', + expectedOptions: { host: 'github.com' } + }, + { + failure: 'valid JSON with a non-object payload', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: { stdout: 'null' }, + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'invalid response shape', + expectedOptions: { cwd: '/repo-root', host: 'github.com' } + }, + { + failure: 'valid JSON with an array payload', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: { stdout: '[]' }, + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'invalid response shape', + expectedOptions: { cwd: '/repo-root', host: 'github.com' } + }, + { + failure: 'present primitive stack metadata', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: { + stdout: JSON.stringify({ + number: 202, + head: { sha: validStackHeadSha }, + stack: true + }) + }, + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'malformed stack', + expectedOptions: { cwd: '/repo-root', host: 'github.com' } + }, + { + failure: 'malformed stack response', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: { + stdout: JSON.stringify({ + number: 202, + head: { sha: validStackHeadSha }, + stack: { + number: 51, + position: 2, + size: '2', + base: { ref: 'main', sha: validStackBaseSha } + } + }) + }, + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'malformed stack', + expectedOptions: { cwd: '/repo-root', host: 'github.com' } + }, + { + failure: 'incoherent stack position', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: { + stdout: JSON.stringify({ + number: 202, + head: { sha: validStackHeadSha }, + stack: { + number: 51, + position: 3, + size: 2, + base: { ref: 'main', sha: validStackBaseSha } + } + }) + }, + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'malformed stack', + expectedOptions: { cwd: '/repo-root', host: 'github.com' } + }, + { + failure: 'non-positive stack number', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: { + stdout: JSON.stringify({ + number: 202, + head: { sha: validStackHeadSha }, + stack: { + number: 0, + position: 1, + size: 2, + base: { ref: 'main', sha: validStackBaseSha } + } + }) + }, + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'malformed stack', + expectedOptions: { cwd: '/repo-root', host: 'github.com' } + }, + { + failure: 'fractional stack size', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: { + stdout: JSON.stringify({ + number: 202, + head: { sha: validStackHeadSha }, + stack: { + number: 51, + position: 1, + size: 1.5, + base: { ref: 'main', sha: validStackBaseSha } + } + }) + }, + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'malformed stack', + expectedOptions: { cwd: '/repo-root', host: 'github.com' } + }, + { + failure: 'unsafe stack number', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: { + stdout: JSON.stringify({ + number: 202, + head: { sha: validStackHeadSha }, + stack: { + number: Number.MAX_SAFE_INTEGER + 1, + position: 1, + size: 2, + base: { ref: 'main', sha: validStackBaseSha } + } + }) + }, + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'malformed stack', + expectedOptions: { cwd: '/repo-root', host: 'github.com' } + }, + { + failure: 'blank stack base ref', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: { + stdout: JSON.stringify({ + number: 202, + head: { sha: validStackHeadSha }, + stack: { + number: 51, + position: 2, + size: 2, + base: { ref: ' ', sha: validStackBaseSha } + } + }) + }, + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'malformed stack', + expectedOptions: { cwd: '/repo-root', host: 'github.com' } + }, + { + failure: 'invalid stack base SHA', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: { + stdout: JSON.stringify({ + number: 202, + head: { sha: validStackHeadSha }, + stack: { + number: 51, + position: 2, + size: 2, + base: { ref: 'main', sha: 123 } + } + }) + }, + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'malformed stack', + expectedOptions: { cwd: '/repo-root', host: 'github.com' } + }, + { + failure: 'invalid string stack base SHA', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: { + stdout: JSON.stringify({ + number: 202, + head: { sha: validStackHeadSha }, + stack: { + number: 51, + position: 2, + size: 2, + base: { ref: 'main', sha: 'not-a-git-object-id' } + } + }) + }, + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'malformed stack', + expectedOptions: { cwd: '/repo-root', host: 'github.com' } + }, + { + failure: 'registered stack response without a head SHA', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: { + stdout: JSON.stringify({ + number: 202, + head: { ref: 'stack/api' }, + stack: { + number: 51, + position: 2, + size: 2, + base: { ref: 'main', sha: validStackBaseSha } + } + }) + }, + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'missing head SHA', + expectedOptions: { cwd: '/repo-root', host: 'github.com' } + }, + { + failure: 'registered stack response with an invalid head SHA', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: { + stdout: JSON.stringify({ + number: 202, + head: { ref: 'stack/api', sha: {} }, + stack: { + number: 51, + position: 2, + size: 2, + base: { ref: 'main', sha: validStackBaseSha } + } + }) + }, + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'missing head SHA', + expectedOptions: { cwd: '/repo-root', host: 'github.com' } + }, + { + failure: 'registered stack response with an invalid string head SHA', + repoPath: '/repo-root', + connectionId: undefined, + probeResponse: { + stdout: JSON.stringify({ + number: 202, + head: { ref: 'stack/api', sha: 'not-a-git-object-id' }, + stack: { + number: 51, + position: 2, + size: 2, + base: { ref: 'main', sha: validStackBaseSha } + } + }) + }, + expectedError: stackMetadataUnavailableError, + expectedDiagnostic: 'missing head SHA', + expectedOptions: { cwd: '/repo-root', host: 'github.com' } + } + ])('fails closed on $failure', async (scenario) => { + const consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined) + if (scenario.probeResponse instanceof Error) { + ghExecFileAsyncMock.mockRejectedValueOnce(scenario.probeResponse) + } else { + ghExecFileAsyncMock.mockResolvedValueOnce(scenario.probeResponse) + } + // Why: disabling the guard must expose the legacy merge fallthrough, not fail on an unstubbed call. + ghExecFileAsyncMock + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 202, + title: 'Stack API', + state: 'OPEN', + url: 'https://github.com/stablyai/orca/pull/202', + statusCheckRollup: [], + updatedAt: '2026-08-10T00:00:00Z', + isDraft: false, + mergeable: 'MERGEABLE', + baseRefName: 'stack/models', + headRefName: 'stack/api', + headRefOid: 'api-sha' + }) + }) + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + + const result = await mergePR(scenario.repoPath, 202, 'squash', scenario.connectionId, { + owner: 'stablyai', + repo: 'orca', + host: scenario.expectedOptions.host + }) + + expect.soft(result).toEqual({ ok: false, error: scenario.expectedError }) + expect + .soft(ghExecFileAsyncMock.mock.calls.map(([args]) => args)) + .toEqual([['api', 'repos/stablyai/orca/pulls/202']]) + expect.soft(ghExecFileAsyncMock.mock.calls[0]?.[1]).toEqual(scenario.expectedOptions) + expect + .soft( + ghExecFileAsyncMock.mock.calls.some(([args]) => + args.includes('repos/stablyai/orca/pulls/202/merge-async') + ) + ) + .toBe(false) + expect + .soft( + ghExecFileAsyncMock.mock.calls.some(([args]) => args[0] === 'pr' && args[1] === 'merge') + ) + .toBe(false) + expect.soft(acquireMock).toHaveBeenCalledTimes(1) + expect.soft(releaseMock).toHaveBeenCalledTimes(1) + expect + .soft(consoleWarnSpy) + .toHaveBeenCalledWith( + 'mergePR stack metadata probe failed for stablyai/orca#202:', + scenario.expectedDiagnostic + ) + expect.soft(consoleWarnSpy).toHaveBeenCalledTimes(1) + }) + + it.each([ + { stackShape: 'omits stack', stackField: {} }, + { stackShape: 'sets stack to null', stackField: { stack: null } } + ])('keeps legacy merge when an ordinary GitHub response $stackShape', async (scenario) => { + ghExecFileAsyncMock + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 13866, + title: 'Fail closed on unavailable stack metadata', + state: 'open', + head: { + ref: 'sta-3924-stack-merge-fail-closed', + sha: validStackHeadSha + }, + base: { ref: 'main', sha: validStackBaseSha }, + ...scenario.stackField + }) + }) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 13866, + title: 'Fail closed on unavailable stack metadata', + state: 'OPEN', + url: 'https://github.com/stablyai/orca/pull/13866', + statusCheckRollup: [], + updatedAt: '2026-08-11T00:00:00Z', + isDraft: false, + mergeable: 'MERGEABLE', + baseRefName: 'main', + baseRefOid: validStackBaseSha, + headRefName: 'sta-3924-stack-merge-fail-closed', + headRefOid: validStackHeadSha + }) + }) + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + + await expect( + mergePR('/repo-root', 13866, 'squash', undefined, { + owner: 'stablyai', + repo: 'orca', + host: 'github.com' + }) + ).resolves.toEqual({ ok: true }) + + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 3, + ['pr', 'merge', '13866', '--squash', '--repo', 'stablyai/orca'], + expect.objectContaining({ env: expect.objectContaining({ GH_PROMPT_DISABLED: '1' }) }) + ) + expect(acquireMock).toHaveBeenCalledTimes(1) + expect(releaseMock).toHaveBeenCalledTimes(1) + }) + it('keeps legacy merge for unregistered dependent PR chains', async () => { ghExecFileAsyncMock .mockResolvedValueOnce({ diff --git a/src/main/github/client.ts b/src/main/github/client.ts index f95bfe7fdd8..068d3ccc419 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -193,6 +193,8 @@ const repositoryMergeMetadataCache = new Map< >() const PR_STACK_SUMMARY_CACHE_TTL_MS = 60_000 const PR_STACK_SUMMARY_CACHE_MAX_ENTRIES = 512 +const STACK_METADATA_UNAVAILABLE_ERROR = + 'Could not verify GitHub pull request stack metadata. Refresh and try again.' const prStackSummaryCache = new Map< string, { value: GitHubPRStack | undefined; expiresAt: number } @@ -2947,16 +2949,69 @@ async function lookupPRByBranchName(args: { } } +function isPositiveSafeInteger(value: unknown): value is number { + return typeof value === 'number' && Number.isSafeInteger(value) && value > 0 +} + +function isGitObjectId(value: unknown): value is string { + return typeof value === 'string' && /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(value) +} + +function isUsableRestStackMetadata(value: unknown): boolean { + if (!value || typeof value !== 'object' || Array.isArray(value)) { + return false + } + const stack = value as { + number?: unknown + position?: unknown + size?: unknown + base?: unknown + } + if (!stack.base || typeof stack.base !== 'object' || Array.isArray(stack.base)) { + return false + } + const base = stack.base as { ref?: unknown; sha?: unknown } + return ( + isPositiveSafeInteger(stack.number) && + isPositiveSafeInteger(stack.position) && + isPositiveSafeInteger(stack.size) && + stack.position <= stack.size && + typeof base.ref === 'string' && + base.ref.trim().length > 0 && + (base.sha === undefined || isGitObjectId(base.sha)) + ) +} + async function getRestPRByNumber( ownerRepo: GitHubApiRepository, number: number, - ghOptions: ReturnType -): Promise { + ghOptions: ReturnType, + options: { requireUsableStackMetadata?: boolean } = {} +): Promise { const { stdout } = await ghExecFileAsync( ['api', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls/${number}`], { ...ghOptions, ...githubHostExecOptions(ownerRepo) } ) - return mapRestPullRequest(JSON.parse(stdout) as RestPullRequest) + const parsed = JSON.parse(stdout) as unknown + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { + throw new Error('invalid response shape') + } + const restData = parsed as RestPullRequest + const mapped = mapRestPullRequest(restData) + if ( + options.requireUsableStackMetadata && + restData.stack !== undefined && + restData.stack !== null + ) { + // Why: GitHub omits stack for ordinary PRs; only unusable non-null metadata is unsafe. + if (!isUsableRestStackMetadata(restData.stack) || !mapped.stack) { + throw new Error('malformed stack') + } + if (!isGitObjectId(restData.head?.sha)) { + throw new Error('missing head SHA') + } + } + return mapped } function prunePRStackSummaryCache(now = Date.now()): void { @@ -2991,7 +3046,7 @@ async function getCachedGitHubPRStackSummary( if (existing) { return existing } - const request = getRestPRByNumber(ownerRepo, number, ghOptions).then((pr) => pr?.stack) + const request = getRestPRByNumber(ownerRepo, number, ghOptions).then((pr) => pr.stack) prStackSummaryInFlight.set(key, request) try { const value = await request @@ -5015,19 +5070,28 @@ export async function mergePR( await acquire() let concurrencySlotHeld = true try { - let stackSummary: GitHubPRStack | undefined - let stackHeadSha: string | undefined + let restData: PullRequestLookupData try { - const restData = await getRestPRByNumber(ownerRepo, prNumber, ghOptions) - stackSummary = restData?.stack - stackHeadSha = restData?.headRefOid - } catch { - // GitHub remains authoritative when stack metadata cannot be read. + restData = await getRestPRByNumber(ownerRepo, prNumber, ghOptions, { + requireUsableStackMetadata: true + }) + } catch (err) { + const diagnostic = + err instanceof SyntaxError + ? 'invalid JSON response' + : err instanceof Error + ? err.message + : String(err) + console.warn( + `mergePR stack metadata probe failed for ${ownerRepo.owner}/${ownerRepo.repo}#${String(prNumber)}:`, + diagnostic + ) + return { ok: false, error: STACK_METADATA_UNAVAILABLE_ERROR } } - if (stackSummary) { + if (restData.stack) { const mergeMetadata = await detectRepositoryMergeMetadata( ownerRepo, - stackSummary.baseRefName, + restData.stack.baseRefName, ghOptions, githubPRStackExecutionScope(connectionId, localGitOptions) ) @@ -5038,7 +5102,7 @@ export async function mergePR( prNumber, method, mergeAction: mergeMetadata.mergeQueueRequired === true ? 'merge_queue' : 'direct_merge', - headSha: stackHeadSha, + headSha: restData.headRefOid, ghOptions }) } @@ -5185,7 +5249,7 @@ async function enablePRAutoMerge( if (ownerRepo) { try { const restData = await getRestPRByNumber(ownerRepo, prNumber, ghOptions) - if (restData?.stack) { + if (restData.stack) { return { ok: false, error: 'GitHub does not support auto-merge for stacked pull requests.'