From e7720d98bf876cdba9ba172516b6cf70199192e8 Mon Sep 17 00:00:00 2001 From: nrslib <38722970+nrslib@users.noreply.github.com> Date: Thu, 26 Feb 2026 12:00:11 +0900 Subject: [PATCH] takt: github-issue-395-add-pull-from --- src/__tests__/taskPullAction.test.ts | 307 ++++++++++++++++++++ src/__tests__/taskSyncAction.test.ts | 36 +++ src/features/tasks/list/index.ts | 4 + src/features/tasks/list/taskActionTarget.ts | 42 ++- src/features/tasks/list/taskActions.ts | 2 + src/features/tasks/list/taskDiffActions.ts | 1 + src/features/tasks/list/taskPullAction.ts | 95 ++++++ src/features/tasks/list/taskSyncAction.ts | 45 +-- 8 files changed, 509 insertions(+), 23 deletions(-) create mode 100644 src/__tests__/taskPullAction.test.ts create mode 100644 src/features/tasks/list/taskPullAction.ts diff --git a/src/__tests__/taskPullAction.test.ts b/src/__tests__/taskPullAction.test.ts new file mode 100644 index 00000000..ca3d96b9 --- /dev/null +++ b/src/__tests__/taskPullAction.test.ts @@ -0,0 +1,307 @@ +import { describe, expect, it, vi, beforeEach } from 'vitest'; + +vi.mock('node:fs', () => ({ + existsSync: vi.fn(), +})); + +vi.mock('node:child_process', () => ({ + execFileSync: vi.fn(), +})); + +vi.mock('../shared/ui/index.js', () => ({ + success: vi.fn(), + error: vi.fn(), +})); + +vi.mock('../shared/utils/index.js', () => ({ + createLogger: vi.fn(() => ({ + info: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + })), + getErrorMessage: vi.fn((err) => String(err)), +})); + +vi.mock('../infra/task/index.js', async (importOriginal) => ({ + ...(await importOriginal>()), + pushBranch: vi.fn(), +})); + +import * as fs from 'node:fs'; +import { execFileSync } from 'node:child_process'; +import { error as logError, success } from '../shared/ui/index.js'; +import { pushBranch } from '../infra/task/index.js'; +import { pullFromRemote } from '../features/tasks/list/taskPullAction.js'; +import type { TaskListItem } from '../infra/task/index.js'; + +const mockExistsSync = vi.mocked(fs.existsSync); +const mockExecFileSync = vi.mocked(execFileSync); +const mockLogError = vi.mocked(logError); +const mockSuccess = vi.mocked(success); +const mockPushBranch = vi.mocked(pushBranch); + +const PROJECT_DIR = '/project'; +const ORIGIN_URL = 'git@github.com:user/repo.git'; + +function makeTask(overrides: Partial = {}): TaskListItem { + return { + kind: 'completed', + name: 'test-task', + branch: 'task/test-task', + createdAt: '2026-01-01T00:00:00Z', + filePath: '/project/.takt/tasks.yaml', + content: 'Implement feature X', + worktreePath: '/project-worktrees/test-task', + ...overrides, + }; +} + +describe('pullFromRemote', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockExistsSync.mockReturnValue(true); + mockExecFileSync.mockReturnValue('' as never); + }); + + it('should throw when called with a non-task BranchActionTarget', () => { + const branchTarget = { + info: { branch: 'some-branch', commit: 'abc123' }, + originalInstruction: 'Do something', + }; + + expect( + () => pullFromRemote(PROJECT_DIR, branchTarget as never), + ).toThrow('Pull requires a task target.'); + }); + + it('should return false and log error when worktreePath is missing', () => { + const task = makeTask({ worktreePath: undefined }); + + const result = pullFromRemote(PROJECT_DIR, task); + + expect(result).toBe(false); + expect(mockLogError).toHaveBeenCalledWith( + expect.stringContaining('Worktree directory does not exist'), + ); + expect(mockExecFileSync).not.toHaveBeenCalled(); + }); + + it('should return false and log error when worktreePath does not exist on disk', () => { + const task = makeTask(); + mockExistsSync.mockReturnValue(false); + + const result = pullFromRemote(PROJECT_DIR, task); + + expect(result).toBe(false); + expect(mockLogError).toHaveBeenCalledWith( + expect.stringContaining('Worktree directory does not exist'), + ); + }); + + it('should get origin URL from projectDir', () => { + const task = makeTask(); + mockExecFileSync.mockImplementation((cmd, args) => { + const argsArr = args as string[]; + if (argsArr[0] === 'config') return `${ORIGIN_URL}\n` as never; + return '' as never; + }); + + pullFromRemote(PROJECT_DIR, task); + + expect(mockExecFileSync).toHaveBeenCalledWith( + 'git', ['config', '--get', 'remote.origin.url'], + expect.objectContaining({ cwd: PROJECT_DIR }), + ); + }); + + it('should add temporary origin, pull, and remove origin', () => { + const task = makeTask(); + const calls: string[][] = []; + mockExecFileSync.mockImplementation((cmd, args) => { + const argsArr = args as string[]; + calls.push(argsArr); + if (argsArr[0] === 'config') return `${ORIGIN_URL}\n` as never; + return '' as never; + }); + + const result = pullFromRemote(PROJECT_DIR, task); + + expect(result).toBe(true); + expect(mockSuccess).toHaveBeenCalledWith('Pulled & pushed.'); + + // Verify git remote add was called on worktree + expect(mockExecFileSync).toHaveBeenCalledWith( + 'git', ['remote', 'add', 'origin', ORIGIN_URL], + expect.objectContaining({ cwd: task.worktreePath }), + ); + + // Verify git pull --ff-only was called on worktree + expect(mockExecFileSync).toHaveBeenCalledWith( + 'git', ['pull', '--ff-only', 'origin', 'task/test-task'], + expect.objectContaining({ cwd: task.worktreePath }), + ); + + // Verify git remote remove was called on worktree + expect(mockExecFileSync).toHaveBeenCalledWith( + 'git', ['remote', 'remove', 'origin'], + expect.objectContaining({ cwd: task.worktreePath }), + ); + }); + + it('should push to projectDir then to origin after successful pull', () => { + const task = makeTask(); + mockExecFileSync.mockImplementation((_cmd, args) => { + const argsArr = args as string[]; + if (argsArr[0] === 'config') return `${ORIGIN_URL}\n` as never; + return '' as never; + }); + + pullFromRemote(PROJECT_DIR, task); + + // worktree → project push + expect(mockExecFileSync).toHaveBeenCalledWith( + 'git', ['push', PROJECT_DIR, 'HEAD'], + expect.objectContaining({ cwd: task.worktreePath }), + ); + // project → origin push + expect(mockPushBranch).toHaveBeenCalledWith(PROJECT_DIR, 'task/test-task'); + }); + + it('should return false and suggest sync when pull fails (not fast-forwardable)', () => { + const task = makeTask(); + mockExecFileSync.mockImplementation((_cmd, args) => { + const argsArr = args as string[]; + if (argsArr[0] === 'config') return `${ORIGIN_URL}\n` as never; + if (argsArr[0] === 'pull') throw new Error('fatal: Not possible to fast-forward'); + return '' as never; + }); + + const result = pullFromRemote(PROJECT_DIR, task); + + expect(result).toBe(false); + expect(mockLogError).toHaveBeenCalledWith( + expect.stringContaining('Pull failed'), + ); + expect(mockLogError).toHaveBeenCalledWith( + expect.stringContaining('Sync with root'), + ); + // Should NOT push when pull fails + expect(mockPushBranch).not.toHaveBeenCalled(); + }); + + it('should remove temporary remote even when pull fails', () => { + const task = makeTask(); + mockExecFileSync.mockImplementation((_cmd, args) => { + const argsArr = args as string[]; + if (argsArr[0] === 'config') return `${ORIGIN_URL}\n` as never; + if (argsArr[0] === 'pull') throw new Error('fatal: Not possible to fast-forward'); + return '' as never; + }); + + pullFromRemote(PROJECT_DIR, task); + + // Verify remote remove was still called (cleanup in finally) + expect(mockExecFileSync).toHaveBeenCalledWith( + 'git', ['remote', 'remove', 'origin'], + expect.objectContaining({ cwd: task.worktreePath }), + ); + }); + + it('should not throw when git remote remove itself fails', () => { + const task = makeTask(); + mockExecFileSync.mockImplementation((_cmd, args) => { + const argsArr = args as string[]; + if (argsArr[0] === 'config') return `${ORIGIN_URL}\n` as never; + if (argsArr[0] === 'pull') throw new Error('pull failed'); + if (argsArr[0] === 'remote' && argsArr[1] === 'remove') throw new Error('remove failed'); + return '' as never; + }); + + const result = pullFromRemote(PROJECT_DIR, task); + + expect(result).toBe(false); + }); + + it('should return false when getOriginUrl fails (root repo has no origin)', () => { + const task = makeTask(); + mockExecFileSync.mockImplementation((_cmd, args) => { + const argsArr = args as string[]; + if (argsArr[0] === 'config') throw new Error('fatal: No such remote \'origin\''); + return '' as never; + }); + + const result = pullFromRemote(PROJECT_DIR, task); + + expect(result).toBe(false); + expect(mockLogError).toHaveBeenCalledWith( + expect.stringContaining('Failed to get origin URL'), + ); + // Should not attempt remote add or pull + expect(mockExecFileSync).not.toHaveBeenCalledWith( + 'git', expect.arrayContaining(['remote', 'add']), + expect.anything(), + ); + }); + + it('should return false when git remote add fails', () => { + const task = makeTask(); + mockExecFileSync.mockImplementation((_cmd, args) => { + const argsArr = args as string[]; + if (argsArr[0] === 'config') return `${ORIGIN_URL}\n` as never; + if (argsArr[0] === 'remote' && argsArr[1] === 'add') throw new Error('fatal: remote origin already exists'); + return '' as never; + }); + + const result = pullFromRemote(PROJECT_DIR, task); + + expect(result).toBe(false); + expect(mockLogError).toHaveBeenCalledWith( + expect.stringContaining('Failed to add temporary remote'), + ); + // Should still attempt remote remove (finally block) + expect(mockExecFileSync).toHaveBeenCalledWith( + 'git', ['remote', 'remove', 'origin'], + expect.objectContaining({ cwd: task.worktreePath }), + ); + // Should not push + expect(mockPushBranch).not.toHaveBeenCalled(); + }); + + it('should return false when git push to projectDir fails after pull', () => { + const task = makeTask(); + mockExecFileSync.mockImplementation((_cmd, args) => { + const argsArr = args as string[]; + if (argsArr[0] === 'config') return `${ORIGIN_URL}\n` as never; + if (argsArr[0] === 'push') throw new Error('push failed'); + return '' as never; + }); + + const result = pullFromRemote(PROJECT_DIR, task); + + expect(result).toBe(false); + expect(mockLogError).toHaveBeenCalledWith( + expect.stringContaining('Push failed after pull'), + ); + expect(mockSuccess).not.toHaveBeenCalled(); + }); + + it('should return false when pushBranch fails after pull', () => { + const task = makeTask(); + mockExecFileSync.mockImplementation((_cmd, args) => { + const argsArr = args as string[]; + if (argsArr[0] === 'config') return `${ORIGIN_URL}\n` as never; + return '' as never; + }); + mockPushBranch.mockImplementation(() => { + throw new Error('push to origin failed'); + }); + + const result = pullFromRemote(PROJECT_DIR, task); + + expect(result).toBe(false); + expect(mockLogError).toHaveBeenCalledWith( + expect.stringContaining('Push failed after pull'), + ); + expect(mockSuccess).not.toHaveBeenCalled(); + }); +}); diff --git a/src/__tests__/taskSyncAction.test.ts b/src/__tests__/taskSyncAction.test.ts index b2447138..e51f9983 100644 --- a/src/__tests__/taskSyncAction.test.ts +++ b/src/__tests__/taskSyncAction.test.ts @@ -219,6 +219,42 @@ describe('syncBranchWithRoot', () => { expect(result).toBe(false); }); + it('returns false when push fails after successful merge', async () => { + const task = makeTask(); + mockExecFileSync.mockImplementation((_cmd, args) => { + const argsArr = args as string[]; + if (argsArr[0] === 'push') throw new Error('push failed'); + return '' as never; + }); + + const result = await syncBranchWithRoot(PROJECT_DIR, task); + + expect(result).toBe(false); + expect(mockLogError).toHaveBeenCalledWith( + expect.stringContaining('Push failed after sync'), + ); + expect(mockSuccess).not.toHaveBeenCalledWith('Synced & pushed.'); + }); + + it('returns false when push fails after AI conflict resolution', async () => { + const task = makeTask(); + mockExecFileSync.mockImplementation((_cmd, args) => { + const argsArr = args as string[]; + if (argsArr[0] === 'merge' && !argsArr.includes('--abort')) throw new Error('CONFLICT'); + if (argsArr[0] === 'push') throw new Error('push failed'); + return '' as never; + }); + mockAgentCall.mockResolvedValue(makeAgentResponse({ status: 'done' })); + + const result = await syncBranchWithRoot(PROJECT_DIR, task); + + expect(result).toBe(false); + expect(mockLogError).toHaveBeenCalledWith( + expect.stringContaining('Push failed after sync'), + ); + expect(mockSuccess).not.toHaveBeenCalledWith('Conflicts resolved & pushed.'); + }); + it('fetches from projectDir using local path ref', async () => { const task = makeTask(); mockExecFileSync.mockReturnValue('' as never); diff --git a/src/features/tasks/list/index.ts b/src/features/tasks/list/index.ts index cedfe040..fce7f617 100644 --- a/src/features/tasks/list/index.ts +++ b/src/features/tasks/list/index.ts @@ -13,6 +13,7 @@ import { mergeBranch, instructBranch, syncBranchWithRoot, + pullFromRemote, } from './taskActions.js'; import { deletePendingTask, deleteFailedTask, deleteCompletedTask, deleteAllTasks } from './taskDeleteActions.js'; import { retryFailedTask } from './taskRetryActions.js'; @@ -171,6 +172,9 @@ export async function listTasks( case 'sync': await syncBranchWithRoot(cwd, task); break; + case 'pull': + pullFromRemote(cwd, task); + break; case 'try': tryMergeBranch(cwd, task); break; diff --git a/src/features/tasks/list/taskActionTarget.ts b/src/features/tasks/list/taskActionTarget.ts index 9f1a1608..63391400 100644 --- a/src/features/tasks/list/taskActionTarget.ts +++ b/src/features/tasks/list/taskActionTarget.ts @@ -1,6 +1,13 @@ +import * as fs from 'node:fs'; +import { execFileSync } from 'node:child_process'; +import { error as logError } from '../../../shared/ui/index.js'; +import { createLogger } from '../../../shared/utils/index.js'; +import { pushBranch } from '../../../infra/task/index.js'; import type { BranchListItem, TaskListItem } from '../../../infra/task/index.js'; -export type ListAction = 'diff' | 'instruct' | 'sync' | 'try' | 'merge' | 'delete'; +const log = createLogger('list-tasks'); + +export type ListAction = 'diff' | 'instruct' | 'sync' | 'pull' | 'try' | 'merge' | 'delete'; export type BranchActionTarget = TaskListItem | Pick; @@ -27,3 +34,36 @@ export function resolveTargetInstruction(target: BranchActionTarget): string { } return target.originalInstruction; } + +/** + * Validates that the target is a task target with a valid worktree path. + * Returns `false` with an error log if validation fails. + * Throws if the target is not a task target (programming error). + */ +export function validateWorktreeTarget( + target: BranchActionTarget, + actionName: string, +): target is TaskListItem & { worktreePath: string } { + if (!('kind' in target)) { + throw new Error(`${actionName} requires a task target.`); + } + + if (!target.worktreePath || !fs.existsSync(target.worktreePath)) { + logError(`Worktree directory does not exist for task: ${target.name}`); + return false; + } + return true; +} + +/** Push worktree → project dir, then project dir → origin */ +export function pushWorktreeToOrigin(worktreePath: string, projectDir: string, branch: string): void { + execFileSync('git', ['push', projectDir, 'HEAD'], { + cwd: worktreePath, + encoding: 'utf-8', + stdio: 'pipe', + }); + log.info('Pushed to main repo', { worktreePath, projectDir }); + + pushBranch(projectDir, branch); + log.info('Pushed to origin', { projectDir, branch }); +} diff --git a/src/features/tasks/list/taskActions.ts b/src/features/tasks/list/taskActions.ts index 655c42f6..405f4bf2 100644 --- a/src/features/tasks/list/taskActions.ts +++ b/src/features/tasks/list/taskActions.ts @@ -19,3 +19,5 @@ export { export { instructBranch } from './taskInstructionActions.js'; export { syncBranchWithRoot } from './taskSyncAction.js'; + +export { pullFromRemote } from './taskPullAction.js'; diff --git a/src/features/tasks/list/taskDiffActions.ts b/src/features/tasks/list/taskDiffActions.ts index 13b48863..ac6ae550 100644 --- a/src/features/tasks/list/taskDiffActions.ts +++ b/src/features/tasks/list/taskDiffActions.ts @@ -67,6 +67,7 @@ export async function showDiffAndPromptActionForTask( { label: 'View diff', value: 'diff', description: 'Show full diff in pager' }, { label: 'Instruct', value: 'instruct', description: 'Craft additional instructions and requeue this task' }, { label: 'Sync with root', value: 'sync', description: 'Merge root HEAD into worktree branch; auto-resolve conflicts with AI' }, + { label: 'Pull from remote', value: 'pull', description: 'Pull latest changes from remote origin (fast-forward only)' }, { label: 'Try merge', value: 'try', description: 'Squash merge (stage changes without commit)' }, { label: 'Merge & cleanup', value: 'merge', description: 'Merge and delete branch' }, { label: 'Delete', value: 'delete', description: 'Discard changes, delete branch' }, diff --git a/src/features/tasks/list/taskPullAction.ts b/src/features/tasks/list/taskPullAction.ts new file mode 100644 index 00000000..9858c61d --- /dev/null +++ b/src/features/tasks/list/taskPullAction.ts @@ -0,0 +1,95 @@ +import { execFileSync } from 'node:child_process'; +import { success, error as logError } from '../../../shared/ui/index.js'; +import { createLogger, getErrorMessage } from '../../../shared/utils/index.js'; +import { + type BranchActionTarget, + resolveTargetBranch, + validateWorktreeTarget, + pushWorktreeToOrigin, +} from './taskActionTarget.js'; + +const log = createLogger('list-tasks'); + +const TEMP_REMOTE_NAME = 'origin'; + +function getOriginUrl(projectDir: string): string { + return execFileSync( + 'git', ['config', '--get', 'remote.origin.url'], + { cwd: projectDir, encoding: 'utf-8', stdio: 'pipe' }, + ).trim(); +} + +export function pullFromRemote( + projectDir: string, + target: BranchActionTarget, +): boolean { + if (!validateWorktreeTarget(target, 'Pull')) { + return false; + } + const worktreePath = target.worktreePath; + const branch = resolveTargetBranch(target); + + let originUrl: string; + try { + originUrl = getOriginUrl(projectDir); + } catch (err) { + logError(`Failed to get origin URL: ${getErrorMessage(err)}`); + log.error('getOriginUrl failed', { projectDir, error: getErrorMessage(err) }); + return false; + } + log.info('Retrieved origin URL from root repo', { projectDir, originUrl }); + + try { + execFileSync('git', ['remote', 'add', TEMP_REMOTE_NAME, originUrl], { + cwd: worktreePath, + encoding: 'utf-8', + stdio: 'pipe', + }); + log.info('Added temporary origin remote', { worktreePath, originUrl }); + + try { + execFileSync('git', ['pull', '--ff-only', TEMP_REMOTE_NAME, branch], { + cwd: worktreePath, + encoding: 'utf-8', + stdio: 'pipe', + }); + log.info('Pull succeeded', { worktreePath, branch }); + } catch (err) { + const msg = getErrorMessage(err); + logError(`Pull failed (not fast-forwardable?): ${msg}`); + logError('If the branch has diverged, use "Sync with root" instead.'); + log.error('git pull --ff-only failed', { worktreePath, branch, error: msg }); + return false; + } + } catch (err) { + logError(`Failed to add temporary remote: ${getErrorMessage(err)}`); + log.error('git remote add failed', { worktreePath, originUrl, error: getErrorMessage(err) }); + return false; + } finally { + removeTemporaryRemote(worktreePath); + } + + try { + pushWorktreeToOrigin(worktreePath, projectDir, branch); + } catch (err) { + logError(`Push failed after pull: ${getErrorMessage(err)}`); + log.error('pushWorktreeToOrigin failed', { worktreePath, projectDir, branch, error: getErrorMessage(err) }); + return false; + } + success('Pulled & pushed.'); + return true; +} + +function removeTemporaryRemote(worktreePath: string): void { + try { + execFileSync('git', ['remote', 'remove', TEMP_REMOTE_NAME], { + cwd: worktreePath, + encoding: 'utf-8', + stdio: 'pipe', + }); + log.info('Removed temporary origin remote', { worktreePath }); + } catch (err) { + logError(`Failed to remove temporary remote: ${getErrorMessage(err)}`); + log.error('git remote remove failed', { worktreePath, error: getErrorMessage(err) }); + } +} diff --git a/src/features/tasks/list/taskSyncAction.ts b/src/features/tasks/list/taskSyncAction.ts index 4501f037..1a3333f1 100644 --- a/src/features/tasks/list/taskSyncAction.ts +++ b/src/features/tasks/list/taskSyncAction.ts @@ -1,13 +1,17 @@ -import * as fs from 'node:fs'; import { execFileSync } from 'node:child_process'; import { success, error as logError, StreamDisplay } from '../../../shared/ui/index.js'; import { createLogger, getErrorMessage } from '../../../shared/utils/index.js'; import { getProvider, type ProviderType } from '../../../infra/providers/index.js'; import { resolveConfigValues } from '../../../infra/config/index.js'; -import { pushBranch } from '../../../infra/task/index.js'; import { loadTemplate } from '../../../shared/prompts/index.js'; import { getLanguage } from '../../../infra/config/index.js'; -import { type BranchActionTarget, resolveTargetBranch, resolveTargetInstruction } from './taskActionTarget.js'; +import { + type BranchActionTarget, + resolveTargetBranch, + resolveTargetInstruction, + validateWorktreeTarget, + pushWorktreeToOrigin, +} from './taskActionTarget.js'; const log = createLogger('list-tasks'); @@ -17,12 +21,7 @@ export async function syncBranchWithRoot( projectDir: string, target: BranchActionTarget, ): Promise { - if (!('kind' in target)) { - throw new Error('Sync requires a task target.'); - } - - if (!target.worktreePath || !fs.existsSync(target.worktreePath)) { - logError(`Worktree directory does not exist for task: ${target.name}`); + if (!validateWorktreeTarget(target, 'Sync')) { return false; } const worktreePath = target.worktreePath; @@ -58,7 +57,9 @@ export async function syncBranchWithRoot( } if (!mergeConflict) { - pushSynced(worktreePath, projectDir, target); + if (!pushSynced(worktreePath, projectDir, target)) { + return false; + } success('Synced & pushed.'); log.info('Merge succeeded without conflicts', { worktreePath }); return true; @@ -86,7 +87,9 @@ export async function syncBranchWithRoot( }); if (response.status === 'done') { - pushSynced(worktreePath, projectDir, target); + if (!pushSynced(worktreePath, projectDir, target)) { + return false; + } success('Conflicts resolved & pushed.'); log.info('AI conflict resolution succeeded', { worktreePath }); return true; @@ -102,18 +105,16 @@ async function autoApproveBash(request: { toolName: string; input: Record