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)
This commit is contained in:
Tomohisa Takaoka 2026-02-22 03:37:14 -08:00 committed by GitHub
parent 4a4a8efaf7
commit a08adadfb3
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
13 changed files with 166 additions and 32 deletions

View File

@ -94,7 +94,8 @@ describe('E2E: Config priority (piece / autoPr)', () => {
timeout: 240_000, 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); const task = readFirstTask(testRepo.path);
expect(task['auto_pr']).toBe(true); expect(task['auto_pr']).toBe(true);
}, 240_000); }, 240_000);
@ -145,7 +146,8 @@ describe('E2E: Config priority (piece / autoPr)', () => {
timeout: 240_000, 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); const task = readFirstTask(testRepo.path);
expect(task['auto_pr']).toBe(true); expect(task['auto_pr']).toBe(true);
}, 240_000); }, 240_000);

View File

@ -18,6 +18,12 @@ vi.mock('../infra/task/git.js', () => ({
vi.mock('../infra/task/clone.js', () => ({ vi.mock('../infra/task/clone.js', () => ({
createSharedClone: vi.fn(), createSharedClone: vi.fn(),
removeClone: 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', () => ({ vi.mock('../infra/task/autoCommit.js', () => ({

View File

@ -362,15 +362,18 @@ describe('resolveBaseBranch', () => {
expect(fetchCalls).toHaveLength(0); expect(fetchCalls).toHaveLength(0);
}); });
it('should use current branch as base when no base_branch config', () => { it('should use remote default branch as base when no base_branch config', () => {
// Given: HEAD is on develop // Given: remote default branch is develop (via symbolic-ref)
const cloneCalls: string[][] = []; const cloneCalls: string[][] = [];
mockExecFileSync.mockImplementation((_cmd, args) => { mockExecFileSync.mockImplementation((_cmd, args) => {
const argsArr = args as string[]; 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') { if (argsArr[0] === 'rev-parse' && argsArr[1] === '--abbrev-ref') {
return 'develop\n'; return 'feature-branch\n';
} }
if (argsArr[0] === 'clone') { if (argsArr[0] === 'clone') {
cloneCalls.push(argsArr); cloneCalls.push(argsArr);
@ -391,10 +394,10 @@ describe('resolveBaseBranch', () => {
// When // When
createSharedClone('/project', { createSharedClone('/project', {
worktree: true, 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).toHaveLength(1);
expect(cloneCalls[0]).toContain('--branch'); expect(cloneCalls[0]).toContain('--branch');
expect(cloneCalls[0]).toContain('develop'); expect(cloneCalls[0]).toContain('develop');

View File

@ -256,10 +256,10 @@ describe('executePipeline', () => {
}); });
it('should pass baseBranch as base to createPullRequest', async () => { 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[]) => { mockExecFileSync.mockImplementation((_cmd: string, args: string[]) => {
if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref') { if (args[0] === 'symbolic-ref' && args[1] === 'refs/remotes/origin/HEAD') {
return 'develop\n'; return 'refs/remotes/origin/develop\n';
} }
return 'abc1234\n'; return 'abc1234\n';
}); });

View File

@ -141,6 +141,38 @@ describe('postExecutionFlow', () => {
expect.objectContaining({ draft: false }), 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', () => { describe('resolveDraftPr', () => {

View File

@ -90,6 +90,11 @@ vi.mock('../infra/task/clone.js', async (importOriginal) => ({
removeClone: vi.fn(), removeClone: vi.fn(),
})); }));
vi.mock('../infra/task/branchList.js', async (importOriginal) => ({
...(await importOriginal<Record<string, unknown>>()),
detectDefaultBranch: vi.fn(() => 'main'),
}));
vi.mock('../infra/task/git.js', async (importOriginal) => ({ vi.mock('../infra/task/git.js', async (importOriginal) => ({
...(await importOriginal<Record<string, unknown>>()), ...(await importOriginal<Record<string, unknown>>()),
getCurrentBranch: vi.fn(() => 'main'), getCurrentBranch: vi.fn(() => 'main'),

View File

@ -42,6 +42,8 @@ vi.mock('../infra/task/index.js', () => ({
autoCommitAndPush: vi.fn(), autoCommitAndPush: vi.fn(),
summarizeTaskName: vi.fn(), summarizeTaskName: vi.fn(),
getCurrentBranch: vi.fn(() => 'main'), getCurrentBranch: vi.fn(() => 'main'),
detectDefaultBranch: vi.fn(() => 'main'),
resolveBaseBranch: vi.fn(() => ({ branch: 'main' })),
TaskRunner: vi.fn(() => ({ TaskRunner: vi.fn(() => ({
addTask: (...args: unknown[]) => mockAddTask(...args), addTask: (...args: unknown[]) => mockAddTask(...args),
completeTask: (...args: unknown[]) => mockCompleteTask(...args), completeTask: (...args: unknown[]) => mockCompleteTask(...args),

View File

@ -150,4 +150,74 @@ describe('executeAndCompleteTask', () => {
}); });
expect(pieceExecutionOptions?.providerOptionsSource).toBe('project'); 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',
}),
);
});
}); });

View File

@ -64,6 +64,8 @@ export interface PostExecutionOptions {
export interface PostExecutionResult { export interface PostExecutionResult {
prUrl?: string; prUrl?: string;
prFailed?: boolean;
prError?: string;
} }
/** /**
@ -96,6 +98,7 @@ export async function postExecutionFlow(options: PostExecutionOptions): Promise<
return { prUrl: existingPr.url }; return { prUrl: existingPr.url };
} else { } else {
error(`PR comment failed: ${commentResult.error}`); error(`PR comment failed: ${commentResult.error}`);
return { prFailed: true, prError: commentResult.error };
} }
} else { } else {
info('Creating pull request...'); info('Creating pull request...');
@ -113,6 +116,7 @@ export async function postExecutionFlow(options: PostExecutionOptions): Promise<
return { prUrl: prResult.url }; return { prUrl: prResult.url };
} else { } else {
error(`PR creation failed: ${prResult.error}`); error(`PR creation failed: ${prResult.error}`);
return { prFailed: true, prError: prResult.error };
} }
} }
} }

View File

@ -5,7 +5,7 @@
import * as fs from 'node:fs'; import * as fs from 'node:fs';
import * as path from 'node:path'; import * as path from 'node:path';
import { resolvePieceConfigValue } from '../../../infra/config/index.js'; 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 { fetchIssue, checkGhCli } from '../../../infra/github/index.js';
import { withProgress } from '../../../shared/ui/index.js'; import { withProgress } from '../../../shared/ui/index.js';
import { createLogger, getErrorMessage } from '../../../shared/utils/index.js'; import { createLogger, getErrorMessage } from '../../../shared/utils/index.js';
@ -124,7 +124,7 @@ export async function resolveTaskExecution(
if (data.worktree) { if (data.worktree) {
throwIfAborted(abortSignal); throwIfAborted(abortSignal);
baseBranch = getCurrentBranch(defaultCwd); baseBranch = detectDefaultBranch(defaultCwd);
if (task.worktreePath && fs.existsSync(task.worktreePath)) { if (task.worktreePath && fs.existsSync(task.worktreePath)) {
// Reuse existing worktree (clone still on disk from previous execution) // Reuse existing worktree (clone still on disk from previous execution)

View File

@ -11,7 +11,7 @@ import {
isPiecePath, isPiecePath,
} from '../../../infra/config/index.js'; } from '../../../infra/config/index.js';
import { confirm } from '../../../shared/prompt/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 { info, error, withProgress } from '../../../shared/ui/index.js';
import { createLogger } from '../../../shared/utils/index.js'; import { createLogger } from '../../../shared/utils/index.js';
import { executeTask } from './taskExecution.js'; import { executeTask } from './taskExecution.js';
@ -53,7 +53,7 @@ export async function confirmAndCreateWorktree(
return { execCwd: cwd, isWorktree: false }; return { execCwd: cwd, isWorktree: false };
} }
const baseBranch = getCurrentBranch(cwd); const baseBranch = resolveBaseBranch(cwd).branch;
const taskSlug = await withProgress( const taskSlug = await withProgress(
'Generating branch name...', 'Generating branch name...',
@ -140,20 +140,10 @@ export async function selectAndExecuteTask(
const completedAt = new Date().toISOString(); const completedAt = new Date().toISOString();
const taskResult = buildBooleanTaskResult({ let prFailed = false;
task: taskRecord, let prError: string | undefined;
taskSuccess,
successResponse: 'Task completed successfully',
failureResponse: 'Task failed',
startedAt,
completedAt,
branch,
...(isWorktree ? { worktreePath: execCwd } : {}),
});
persistTaskResult(taskRunner, taskResult);
if (taskSuccess && isWorktree) { if (taskSuccess && isWorktree) {
await postExecutionFlow({ const postResult = await postExecutionFlow({
execCwd, execCwd,
projectCwd: cwd, projectCwd: cwd,
task, task,
@ -165,9 +155,24 @@ export async function selectAndExecuteTask(
issues: options?.issues, issues: options?.issues,
repo: options?.repo, 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); process.exit(1);
} }
} }

View File

@ -169,6 +169,7 @@ export async function executeAndCompleteTask(
const completedAt = new Date().toISOString(); const completedAt = new Date().toISOString();
let prUrl: string | undefined; let prUrl: string | undefined;
let effectiveRunResult = taskRunResult;
if (taskSuccess && isWorktree) { if (taskSuccess && isWorktree) {
const issues = resolveTaskIssue(issueNumber); const issues = resolveTaskIssue(issueNumber);
const postResult = await postExecutionFlow({ const postResult = await postExecutionFlow({
@ -183,11 +184,14 @@ export async function executeAndCompleteTask(
issues, issues,
}); });
prUrl = postResult.prUrl; prUrl = postResult.prUrl;
if (postResult.prFailed) {
effectiveRunResult = { success: false, reason: `PR creation failed: ${postResult.prError}` };
}
} }
const taskResult = buildTaskResult({ const taskResult = buildTaskResult({
task, task,
runResult: taskRunResult, runResult: effectiveRunResult,
startedAt, startedAt,
completedAt, completedAt,
branch, branch,
@ -196,7 +200,7 @@ export async function executeAndCompleteTask(
}); });
persistTaskResult(taskRunner, taskResult); persistTaskResult(taskRunner, taskResult);
return taskSuccess; return effectiveRunResult.success;
} catch (err) { } catch (err) {
const completedAt = new Date().toISOString(); const completedAt = new Date().toISOString();
persistTaskError(taskRunner, task, startedAt, completedAt, err); persistTaskError(taskRunner, task, startedAt, completedAt, err);

View File

@ -12,6 +12,7 @@ import * as path from 'node:path';
import { execFileSync } from 'node:child_process'; import { execFileSync } from 'node:child_process';
import { createLogger } from '../../shared/utils/index.js'; import { createLogger } from '../../shared/utils/index.js';
import { resolveConfigValue } from '../config/index.js'; import { resolveConfigValue } from '../config/index.js';
import { detectDefaultBranch } from './branchList.js';
import type { WorktreeOptions, WorktreeResult } from './types.js'; import type { WorktreeOptions, WorktreeResult } from './types.js';
export type { WorktreeOptions, WorktreeResult }; export type { WorktreeOptions, WorktreeResult };
@ -150,8 +151,8 @@ export class CloneManager {
const configBaseBranch = resolveConfigValue(projectDir, 'baseBranch') as string | undefined; const configBaseBranch = resolveConfigValue(projectDir, 'baseBranch') as string | undefined;
const autoFetch = resolveConfigValue(projectDir, 'autoFetch') as boolean | undefined; const autoFetch = resolveConfigValue(projectDir, 'autoFetch') as boolean | undefined;
// Determine base branch: config base_branch → current branch // Determine base branch: config base_branch → remote default branch
const baseBranch = configBaseBranch ?? CloneManager.getCurrentBranch(projectDir); const baseBranch = configBaseBranch ?? detectDefaultBranch(projectDir);
if (!autoFetch) { if (!autoFetch) {
return { branch: baseBranch }; return { branch: baseBranch };