From a08adadfb3d452ad354621ccb9352c185a0142f6 Mon Sep 17 00:00:00 2001 From: Tomohisa Takaoka Date: Sun, 22 Feb 2026 03:37:14 -0800 Subject: [PATCH] fix: PR creation failure handling + use default branch for base (#345) * fix: mark task as failed when PR creation fails Previously, when PR creation failed (e.g. invalid base branch), the task was still marked as 'completed' even though the PR was not created. This fix ensures: - postExecutionFlow returns prFailed/prError on failure - executeAndCompleteTask marks the task as failed when PR fails - selectAndExecuteTask runs postExecution before persisting result The pipeline path (executePipeline) already handled this correctly via EXIT_PR_CREATION_FAILED. * fix: use detectDefaultBranch instead of getCurrentBranch for PR base Previously, baseBranch for PR creation was set to HEAD's current branch via getCurrentBranch(). When the user was on a feature branch like 'codex/pr-16-review', PRs were created with --base codex/pr-16-review, which fails because it doesn't exist on the remote. Now uses detectDefaultBranch() (via git symbolic-ref refs/remotes/origin/HEAD) to always use the actual default branch (main/master) as the PR base. Affected paths: - resolveTask.ts (takt run) - selectAndExecute.ts (interactive mode) - pipeline/execute.ts (takt pipeline) --- e2e/specs/config-priority.e2e.ts | 6 +- src/__tests__/cli-worktree.test.ts | 6 ++ src/__tests__/clone.test.ts | 13 ++-- src/__tests__/pipelineExecution.test.ts | 6 +- src/__tests__/postExecution.test.ts | 32 +++++++++ src/__tests__/runAllTasks-concurrency.test.ts | 5 ++ src/__tests__/selectAndExecute-autoPr.test.ts | 2 + src/__tests__/taskExecution.test.ts | 70 +++++++++++++++++++ src/features/tasks/execute/postExecution.ts | 4 ++ src/features/tasks/execute/resolveTask.ts | 4 +- .../tasks/execute/selectAndExecute.ts | 37 +++++----- src/features/tasks/execute/taskExecution.ts | 8 ++- src/infra/task/clone.ts | 5 +- 13 files changed, 166 insertions(+), 32 deletions(-) diff --git a/e2e/specs/config-priority.e2e.ts b/e2e/specs/config-priority.e2e.ts index a74ded6..525ff36 100644 --- a/e2e/specs/config-priority.e2e.ts +++ b/e2e/specs/config-priority.e2e.ts @@ -94,7 +94,8 @@ describe('E2E: Config priority (piece / autoPr)', () => { timeout: 240_000, }); - expect(result.exitCode).toBe(0); + // PR creation fails in test env (no gh remote), so exit code 1 is expected + // when auto_pr defaults to true. The task record is still persisted. const task = readFirstTask(testRepo.path); expect(task['auto_pr']).toBe(true); }, 240_000); @@ -145,7 +146,8 @@ describe('E2E: Config priority (piece / autoPr)', () => { timeout: 240_000, }); - expect(result.exitCode).toBe(0); + // PR creation fails in test env (no gh remote), so exit code 1 is expected + // when auto_pr is overridden to true. The task record is still persisted. const task = readFirstTask(testRepo.path); expect(task['auto_pr']).toBe(true); }, 240_000); diff --git a/src/__tests__/cli-worktree.test.ts b/src/__tests__/cli-worktree.test.ts index 405f939..771f275 100644 --- a/src/__tests__/cli-worktree.test.ts +++ b/src/__tests__/cli-worktree.test.ts @@ -18,6 +18,12 @@ vi.mock('../infra/task/git.js', () => ({ vi.mock('../infra/task/clone.js', () => ({ createSharedClone: vi.fn(), removeClone: vi.fn(), + resolveBaseBranch: vi.fn(() => ({ branch: 'main' })), +})); + +vi.mock('../infra/task/branchList.js', () => ({ + detectDefaultBranch: vi.fn(() => 'main'), + BranchManager: vi.fn(), })); vi.mock('../infra/task/autoCommit.js', () => ({ diff --git a/src/__tests__/clone.test.ts b/src/__tests__/clone.test.ts index 522593e..acdf199 100644 --- a/src/__tests__/clone.test.ts +++ b/src/__tests__/clone.test.ts @@ -362,15 +362,18 @@ describe('resolveBaseBranch', () => { expect(fetchCalls).toHaveLength(0); }); - it('should use current branch as base when no base_branch config', () => { - // Given: HEAD is on develop + it('should use remote default branch as base when no base_branch config', () => { + // Given: remote default branch is develop (via symbolic-ref) const cloneCalls: string[][] = []; mockExecFileSync.mockImplementation((_cmd, args) => { const argsArr = args as string[]; + if (argsArr[0] === 'symbolic-ref' && argsArr[1] === 'refs/remotes/origin/HEAD') { + return 'refs/remotes/origin/develop\n'; + } if (argsArr[0] === 'rev-parse' && argsArr[1] === '--abbrev-ref') { - return 'develop\n'; + return 'feature-branch\n'; } if (argsArr[0] === 'clone') { cloneCalls.push(argsArr); @@ -391,10 +394,10 @@ describe('resolveBaseBranch', () => { // When createSharedClone('/project', { worktree: true, - taskSlug: 'use-current-branch', + taskSlug: 'use-default-branch', }); - // Then: clone was called with --branch develop (current branch) + // Then: clone was called with --branch develop (remote default branch, not current branch) expect(cloneCalls).toHaveLength(1); expect(cloneCalls[0]).toContain('--branch'); expect(cloneCalls[0]).toContain('develop'); diff --git a/src/__tests__/pipelineExecution.test.ts b/src/__tests__/pipelineExecution.test.ts index 6209ff8..5a4d918 100644 --- a/src/__tests__/pipelineExecution.test.ts +++ b/src/__tests__/pipelineExecution.test.ts @@ -256,10 +256,10 @@ describe('executePipeline', () => { }); it('should pass baseBranch as base to createPullRequest', async () => { - // Given: getCurrentBranch returns 'develop' before branch creation + // Given: detectDefaultBranch returns 'develop' (via symbolic-ref) mockExecFileSync.mockImplementation((_cmd: string, args: string[]) => { - if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref') { - return 'develop\n'; + if (args[0] === 'symbolic-ref' && args[1] === 'refs/remotes/origin/HEAD') { + return 'refs/remotes/origin/develop\n'; } return 'abc1234\n'; }); diff --git a/src/__tests__/postExecution.test.ts b/src/__tests__/postExecution.test.ts index eb6ac52..284c0bb 100644 --- a/src/__tests__/postExecution.test.ts +++ b/src/__tests__/postExecution.test.ts @@ -141,6 +141,38 @@ describe('postExecutionFlow', () => { expect.objectContaining({ draft: false }), ); }); + + it('PR作成失敗時に prFailed: true を返す', async () => { + mockFindExistingPr.mockReturnValue(undefined); + mockCreatePullRequest.mockReturnValue({ success: false, error: 'Base ref must be a branch' }); + + const result = await postExecutionFlow(baseOptions); + + expect(result.prFailed).toBe(true); + expect(result.prError).toBe('Base ref must be a branch'); + expect(result.prUrl).toBeUndefined(); + }); + + it('PRコメント失敗時に prFailed: true を返す', async () => { + mockFindExistingPr.mockReturnValue({ number: 42, url: 'https://github.com/org/repo/pull/42' }); + mockCommentOnPr.mockReturnValue({ success: false, error: 'Permission denied' }); + + const result = await postExecutionFlow(baseOptions); + + expect(result.prFailed).toBe(true); + expect(result.prError).toBe('Permission denied'); + expect(result.prUrl).toBeUndefined(); + }); + + it('PR作成成功時は prFailed を返さない', async () => { + mockFindExistingPr.mockReturnValue(undefined); + mockCreatePullRequest.mockReturnValue({ success: true, url: 'https://github.com/org/repo/pull/1' }); + + const result = await postExecutionFlow(baseOptions); + + expect(result.prFailed).toBeUndefined(); + expect(result.prUrl).toBe('https://github.com/org/repo/pull/1'); + }); }); describe('resolveDraftPr', () => { diff --git a/src/__tests__/runAllTasks-concurrency.test.ts b/src/__tests__/runAllTasks-concurrency.test.ts index f299aa7..be9634a 100644 --- a/src/__tests__/runAllTasks-concurrency.test.ts +++ b/src/__tests__/runAllTasks-concurrency.test.ts @@ -90,6 +90,11 @@ vi.mock('../infra/task/clone.js', async (importOriginal) => ({ removeClone: vi.fn(), })); +vi.mock('../infra/task/branchList.js', async (importOriginal) => ({ + ...(await importOriginal>()), + detectDefaultBranch: vi.fn(() => 'main'), +})); + vi.mock('../infra/task/git.js', async (importOriginal) => ({ ...(await importOriginal>()), getCurrentBranch: vi.fn(() => 'main'), diff --git a/src/__tests__/selectAndExecute-autoPr.test.ts b/src/__tests__/selectAndExecute-autoPr.test.ts index bae7003..f0b2a98 100644 --- a/src/__tests__/selectAndExecute-autoPr.test.ts +++ b/src/__tests__/selectAndExecute-autoPr.test.ts @@ -42,6 +42,8 @@ vi.mock('../infra/task/index.js', () => ({ autoCommitAndPush: vi.fn(), summarizeTaskName: vi.fn(), getCurrentBranch: vi.fn(() => 'main'), + detectDefaultBranch: vi.fn(() => 'main'), + resolveBaseBranch: vi.fn(() => ({ branch: 'main' })), TaskRunner: vi.fn(() => ({ addTask: (...args: unknown[]) => mockAddTask(...args), completeTask: (...args: unknown[]) => mockCompleteTask(...args), diff --git a/src/__tests__/taskExecution.test.ts b/src/__tests__/taskExecution.test.ts index c254faf..1139a07 100644 --- a/src/__tests__/taskExecution.test.ts +++ b/src/__tests__/taskExecution.test.ts @@ -150,4 +150,74 @@ describe('executeAndCompleteTask', () => { }); expect(pieceExecutionOptions?.providerOptionsSource).toBe('project'); }); + + it('should mark task as failed when PR creation fails', async () => { + // Given: worktree mode with autoPr enabled, PR creation fails + const task = createTask('task-with-pr-failure'); + mockResolveTaskExecution.mockResolvedValue({ + execCwd: '/worktree/clone', + execPiece: 'default', + isWorktree: true, + autoPr: true, + draftPr: false, + taskPrompt: undefined, + reportDirName: undefined, + branch: 'takt/task-with-pr-failure', + worktreePath: '/worktree/clone', + baseBranch: 'main', + startMovement: undefined, + retryNote: undefined, + issueNumber: undefined, + }); + mockExecutePiece.mockResolvedValue({ success: true }); + mockPostExecutionFlow.mockResolvedValue({ prFailed: true, prError: 'Base ref must be a branch' }); + + // When + const result = await executeAndCompleteTask(task, {} as never, '/project', 'default'); + + // Then: task should be marked as failed + expect(result).toBe(false); + expect(mockBuildTaskResult).toHaveBeenCalledWith( + expect.objectContaining({ + runResult: expect.objectContaining({ + success: false, + reason: 'PR creation failed: Base ref must be a branch', + }), + }), + ); + }); + + it('should mark task as completed when PR creation succeeds', async () => { + // Given: worktree mode with autoPr enabled, PR creation succeeds + const task = createTask('task-with-pr-success'); + mockResolveTaskExecution.mockResolvedValue({ + execCwd: '/worktree/clone', + execPiece: 'default', + isWorktree: true, + autoPr: true, + draftPr: false, + taskPrompt: undefined, + reportDirName: undefined, + branch: 'takt/task-with-pr-success', + worktreePath: '/worktree/clone', + baseBranch: 'main', + startMovement: undefined, + retryNote: undefined, + issueNumber: undefined, + }); + mockExecutePiece.mockResolvedValue({ success: true }); + mockPostExecutionFlow.mockResolvedValue({ prUrl: 'https://github.com/org/repo/pull/1' }); + + // When + const result = await executeAndCompleteTask(task, {} as never, '/project', 'default'); + + // Then: task should be marked as completed + expect(result).toBe(true); + expect(mockBuildTaskResult).toHaveBeenCalledWith( + expect.objectContaining({ + runResult: expect.objectContaining({ success: true }), + prUrl: 'https://github.com/org/repo/pull/1', + }), + ); + }); }); diff --git a/src/features/tasks/execute/postExecution.ts b/src/features/tasks/execute/postExecution.ts index cffb6c6..fbc03b5 100644 --- a/src/features/tasks/execute/postExecution.ts +++ b/src/features/tasks/execute/postExecution.ts @@ -64,6 +64,8 @@ export interface PostExecutionOptions { export interface PostExecutionResult { prUrl?: string; + prFailed?: boolean; + prError?: string; } /** @@ -96,6 +98,7 @@ export async function postExecutionFlow(options: PostExecutionOptions): Promise< return { prUrl: existingPr.url }; } else { error(`PR comment failed: ${commentResult.error}`); + return { prFailed: true, prError: commentResult.error }; } } else { info('Creating pull request...'); @@ -113,6 +116,7 @@ export async function postExecutionFlow(options: PostExecutionOptions): Promise< return { prUrl: prResult.url }; } else { error(`PR creation failed: ${prResult.error}`); + return { prFailed: true, prError: prResult.error }; } } } diff --git a/src/features/tasks/execute/resolveTask.ts b/src/features/tasks/execute/resolveTask.ts index e94f432..3e29dd9 100644 --- a/src/features/tasks/execute/resolveTask.ts +++ b/src/features/tasks/execute/resolveTask.ts @@ -5,7 +5,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import { resolvePieceConfigValue } from '../../../infra/config/index.js'; -import { type TaskInfo, createSharedClone, summarizeTaskName, getCurrentBranch } from '../../../infra/task/index.js'; +import { type TaskInfo, createSharedClone, summarizeTaskName, detectDefaultBranch } from '../../../infra/task/index.js'; import { fetchIssue, checkGhCli } from '../../../infra/github/index.js'; import { withProgress } from '../../../shared/ui/index.js'; import { createLogger, getErrorMessage } from '../../../shared/utils/index.js'; @@ -124,7 +124,7 @@ export async function resolveTaskExecution( if (data.worktree) { throwIfAborted(abortSignal); - baseBranch = getCurrentBranch(defaultCwd); + baseBranch = detectDefaultBranch(defaultCwd); if (task.worktreePath && fs.existsSync(task.worktreePath)) { // Reuse existing worktree (clone still on disk from previous execution) diff --git a/src/features/tasks/execute/selectAndExecute.ts b/src/features/tasks/execute/selectAndExecute.ts index c717593..f5308da 100644 --- a/src/features/tasks/execute/selectAndExecute.ts +++ b/src/features/tasks/execute/selectAndExecute.ts @@ -11,7 +11,7 @@ import { isPiecePath, } from '../../../infra/config/index.js'; import { confirm } from '../../../shared/prompt/index.js'; -import { createSharedClone, summarizeTaskName, getCurrentBranch, TaskRunner } from '../../../infra/task/index.js'; +import { createSharedClone, summarizeTaskName, resolveBaseBranch, TaskRunner } from '../../../infra/task/index.js'; import { info, error, withProgress } from '../../../shared/ui/index.js'; import { createLogger } from '../../../shared/utils/index.js'; import { executeTask } from './taskExecution.js'; @@ -53,7 +53,7 @@ export async function confirmAndCreateWorktree( return { execCwd: cwd, isWorktree: false }; } - const baseBranch = getCurrentBranch(cwd); + const baseBranch = resolveBaseBranch(cwd).branch; const taskSlug = await withProgress( 'Generating branch name...', @@ -140,20 +140,10 @@ export async function selectAndExecuteTask( const completedAt = new Date().toISOString(); - const taskResult = buildBooleanTaskResult({ - task: taskRecord, - taskSuccess, - successResponse: 'Task completed successfully', - failureResponse: 'Task failed', - startedAt, - completedAt, - branch, - ...(isWorktree ? { worktreePath: execCwd } : {}), - }); - persistTaskResult(taskRunner, taskResult); - + let prFailed = false; + let prError: string | undefined; if (taskSuccess && isWorktree) { - await postExecutionFlow({ + const postResult = await postExecutionFlow({ execCwd, projectCwd: cwd, task, @@ -165,9 +155,24 @@ export async function selectAndExecuteTask( issues: options?.issues, repo: options?.repo, }); + prFailed = postResult.prFailed ?? false; + prError = postResult.prError; } - if (!taskSuccess) { + const effectiveSuccess = taskSuccess && !prFailed; + const taskResult = buildBooleanTaskResult({ + task: taskRecord, + taskSuccess: effectiveSuccess, + successResponse: 'Task completed successfully', + failureResponse: prFailed ? `PR creation failed: ${prError}` : 'Task failed', + startedAt, + completedAt, + branch, + ...(isWorktree ? { worktreePath: execCwd } : {}), + }); + persistTaskResult(taskRunner, taskResult); + + if (!effectiveSuccess) { process.exit(1); } } diff --git a/src/features/tasks/execute/taskExecution.ts b/src/features/tasks/execute/taskExecution.ts index e630c13..d21cf90 100644 --- a/src/features/tasks/execute/taskExecution.ts +++ b/src/features/tasks/execute/taskExecution.ts @@ -169,6 +169,7 @@ export async function executeAndCompleteTask( const completedAt = new Date().toISOString(); let prUrl: string | undefined; + let effectiveRunResult = taskRunResult; if (taskSuccess && isWorktree) { const issues = resolveTaskIssue(issueNumber); const postResult = await postExecutionFlow({ @@ -183,11 +184,14 @@ export async function executeAndCompleteTask( issues, }); prUrl = postResult.prUrl; + if (postResult.prFailed) { + effectiveRunResult = { success: false, reason: `PR creation failed: ${postResult.prError}` }; + } } const taskResult = buildTaskResult({ task, - runResult: taskRunResult, + runResult: effectiveRunResult, startedAt, completedAt, branch, @@ -196,7 +200,7 @@ export async function executeAndCompleteTask( }); persistTaskResult(taskRunner, taskResult); - return taskSuccess; + return effectiveRunResult.success; } catch (err) { const completedAt = new Date().toISOString(); persistTaskError(taskRunner, task, startedAt, completedAt, err); diff --git a/src/infra/task/clone.ts b/src/infra/task/clone.ts index af627ea..8ae9c2b 100644 --- a/src/infra/task/clone.ts +++ b/src/infra/task/clone.ts @@ -12,6 +12,7 @@ import * as path from 'node:path'; import { execFileSync } from 'node:child_process'; import { createLogger } from '../../shared/utils/index.js'; import { resolveConfigValue } from '../config/index.js'; +import { detectDefaultBranch } from './branchList.js'; import type { WorktreeOptions, WorktreeResult } from './types.js'; export type { WorktreeOptions, WorktreeResult }; @@ -150,8 +151,8 @@ export class CloneManager { const configBaseBranch = resolveConfigValue(projectDir, 'baseBranch') as string | undefined; const autoFetch = resolveConfigValue(projectDir, 'autoFetch') as boolean | undefined; - // Determine base branch: config base_branch → current branch - const baseBranch = configBaseBranch ?? CloneManager.getCurrentBranch(projectDir); + // Determine base branch: config base_branch → remote default branch + const baseBranch = configBaseBranch ?? detectDefaultBranch(projectDir); if (!autoFetch) { return { branch: baseBranch };