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 <e2e@test.local>
(cherry picked from commit 9f0bf39b04)
This commit is contained in:
Jinwoo Hong
2026-08-12 17:13:27 -07:00
committed by Jinjing
parent d6d4a02689
commit a498b35bad
3 changed files with 527 additions and 22 deletions
@@ -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: {} } }) }
}
+430 -7
View File
@@ -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<typeof GitHubEnterpriseRepositoryModule>()),
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({
+79 -15
View File
@@ -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<typeof ghRepoExecOptions>
): Promise<PullRequestLookupData | null> {
ghOptions: ReturnType<typeof ghRepoExecOptions>,
options: { requireUsableStackMetadata?: boolean } = {}
): Promise<PullRequestLookupData> {
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.'