Skip to content

Commit 217ea51

Browse files
Bill LeoutsakosBill Leoutsakos
authored andcommitted
fix(pi): handle update PR finalization races
1 parent 80c62d1 commit 217ea51

3 files changed

Lines changed: 108 additions & 7 deletions

File tree

apps/sim/executor/handlers/pi/cloud-backend.test.ts

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ function existingPullRequestOutput() {
9696
ref: 'feature/existing',
9797
repo_full_name: 'octo/demo',
9898
},
99-
base: { sha: 'b'.repeat(40), ref: 'staging' },
99+
base: { sha: 'b'.repeat(40), ref: 'staging', repo_full_name: 'octo/demo' },
100100
},
101101
}
102102
}
@@ -768,6 +768,98 @@ describe('runCloudPi', () => {
768768
)
769769
})
770770

771+
it('updates a PR created while the branch is being authored instead of creating a duplicate', async () => {
772+
let listCalls = 0
773+
mockExecuteTool.mockImplementation((tool: string) => {
774+
if (tool === 'github_list_prs_v2') {
775+
listCalls += 1
776+
return Promise.resolve({
777+
success: true,
778+
output:
779+
listCalls === 1 ? { items: [], count: 0 } : { items: [{ number: 7 }], count: 1 },
780+
})
781+
}
782+
if (tool === 'github_pr_v2') {
783+
return Promise.resolve(existingPullRequestOutput())
784+
}
785+
if (tool === 'github_create_pr') {
786+
throw new Error('must not create a duplicate PR')
787+
}
788+
return Promise.resolve({ success: true, output: {} })
789+
})
790+
791+
const result = await runCloudBranchPi(branchParams(), { onEvent: vi.fn() })
792+
793+
expect(listCalls).toBe(2)
794+
expect(
795+
mockExecuteTool.mock.calls.some(([tool]: [string]) => tool === 'github_create_pr')
796+
).toBe(false)
797+
expect(result.prUrl).toBe('https://github.com/octo/demo/pull/7')
798+
})
799+
800+
it('does not claim a push happened when no-op authoring is followed by a PR error', async () => {
801+
mockRun.mockImplementation((command: string) => {
802+
if (command.includes('git clone')) {
803+
return Promise.resolve({ stdout: '__BASE_SHA__=abc', stderr: '', exitCode: 0 })
804+
}
805+
if (command.includes('pi -p')) {
806+
return Promise.resolve({ stdout: '', stderr: '', exitCode: 0 })
807+
}
808+
return Promise.resolve({ stdout: '__NO_CHANGES__=1', stderr: '', exitCode: 0 })
809+
})
810+
mockExecuteTool.mockImplementation((tool: string) => {
811+
if (tool === 'github_list_prs_v2') {
812+
return Promise.resolve({ success: true, output: { items: [], count: 0 } })
813+
}
814+
if (tool === 'github_repo_info_v2') {
815+
return Promise.resolve({ success: true, output: { default_branch: 'main' } })
816+
}
817+
return Promise.resolve({ success: false, error: 'permission denied' })
818+
})
819+
820+
const error = (await runCloudBranchPi(branchParams(), {
821+
onEvent: vi.fn(),
822+
}).catch((caught) => caught)) as Error
823+
824+
expect(error.message).toContain(
825+
'PR creation failed for branch feature/existing: permission denied'
826+
)
827+
expect(error.message).not.toContain('pushed')
828+
expect(mockRun.mock.calls.some(([command]: [string]) => command.includes('push'))).toBe(false)
829+
})
830+
831+
it('uses neutral wording when a no-op run cannot update its existing PR', async () => {
832+
mockExistingBranchPullRequest()
833+
mockRun.mockImplementation((command: string) => {
834+
if (command.includes('git clone')) {
835+
return Promise.resolve({ stdout: '__BASE_SHA__=abc', stderr: '', exitCode: 0 })
836+
}
837+
if (command.includes('pi -p')) {
838+
return Promise.resolve({ stdout: '', stderr: '', exitCode: 0 })
839+
}
840+
return Promise.resolve({ stdout: '__NO_CHANGES__=1', stderr: '', exitCode: 0 })
841+
})
842+
mockExecuteTool.mockImplementation((tool: string) => {
843+
if (tool === 'github_pr_v2') {
844+
return Promise.resolve(existingPullRequestOutput())
845+
}
846+
if (tool === 'github_update_pr') {
847+
return Promise.resolve({ success: false, error: 'permission denied' })
848+
}
849+
return Promise.resolve({ success: true, output: {} })
850+
})
851+
852+
const error = (await runCloudBranchPi(branchParams({ prTitle: 'New title' }), {
853+
onEvent: vi.fn(),
854+
}).catch((caught) => caught)) as Error
855+
856+
expect(error.message).toContain(
857+
'PR update failed for branch feature/existing: permission denied'
858+
)
859+
expect(error.message).not.toContain('pushed')
860+
expect(mockRun.mock.calls.some(([command]: [string]) => command.includes('push'))).toBe(false)
861+
})
862+
771863
it('updates explicit metadata and draft state on the exact existing PR', async () => {
772864
mockExistingBranchPullRequest()
773865
const mockFetch = vi

apps/sim/executor/handlers/pi/cloud-backend.ts

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -186,13 +186,11 @@ async function openPullRequest(
186186
)
187187

188188
if (!result.success) {
189-
throw new Error(
190-
`Branch ${branch} pushed but PR creation failed: ${result.error ?? 'unknown error'}`
191-
)
189+
throw new Error(`PR creation failed for branch ${branch}: ${result.error ?? 'unknown error'}`)
192190
}
193191

194192
if (!isRecord(result.output)) {
195-
throw new Error(`Branch ${branch} pushed but PR creation returned an invalid response`)
193+
throw new Error(`PR creation returned an invalid response for branch ${branch}`)
196194
}
197195
const metadata = requiredRecord(result.output, 'metadata', 'GitHub create pull request response')
198196
const rawNumber = metadata.number
@@ -269,7 +267,7 @@ async function updatePullRequest(
269267
)
270268
if (!result.success) {
271269
throw new Error(
272-
`Branch ${params.targetBranch} pushed but PR update failed: ${result.error ?? 'unknown error'}`
270+
`PR update failed for branch ${params.targetBranch}: ${result.error ?? 'unknown error'}`
273271
)
274272
}
275273
}
@@ -301,6 +299,18 @@ async function ensureUpdatePullRequest(
301299
if (existingPullRequest) {
302300
return updatePullRequest(params, existingPullRequest, secrets, signal)
303301
}
302+
const newlyCreatedPullRequest = await findOpenPrForBranch(
303+
{
304+
owner: params.owner,
305+
repo: params.repo,
306+
branch,
307+
githubToken: params.githubToken,
308+
},
309+
signal
310+
)
311+
if (newlyCreatedPullRequest) {
312+
return updatePullRequest(params, newlyCreatedPullRequest, secrets, signal)
313+
}
304314
const base = params.baseBranch?.trim() || (await repositoryDefaultBranch(params, signal))
305315
const draft = params.babysit ? false : params.prState !== 'ready'
306316
return openPullRequest(params, branch, base, draft, totals, secrets, signal)

apps/sim/executor/handlers/pi/pi-handler.test.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -692,7 +692,6 @@ describe('PiBlockHandler', () => {
692692
expect(mockRunCloudBranch.mock.calls[0][0].search).toEqual({
693693
provider: 'exa',
694694
apiKey: 'search-key',
695-
keySource: 'byok',
696695
})
697696
})
698697

0 commit comments

Comments
 (0)