diff --git a/CLAUDE.md b/CLAUDE.md index 72e3782..8b5826f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -64,6 +64,7 @@ TAKT (TAKT Agent Koordination Topology) is a multi-agent orchestration system fo | `--pipeline` | Enable pipeline (non-interactive) mode — required for CI/automation | | `-t, --task ` | Task content (as alternative to GitHub issue) | | `-i, --issue ` | GitHub issue number (equivalent to `#N` in interactive mode) | +| `--pr ` | PR number to fetch review comments and fix | | `-w, --piece ` | Piece name or path to piece YAML file | | `-b, --branch ` | Branch name (auto-generated if omitted) | | `--auto-pr` | Create PR after execution (interactive: skip confirmation, pipeline: enable PR) | diff --git a/src/__tests__/cli-routing-pr-resolve.test.ts b/src/__tests__/cli-routing-pr-resolve.test.ts new file mode 100644 index 0000000..9bd20ef --- /dev/null +++ b/src/__tests__/cli-routing-pr-resolve.test.ts @@ -0,0 +1,316 @@ +/** + * Tests for PR resolution in routing module. + * + * Verifies that --pr option fetches review comments, + * passes formatted task to interactive mode, and sets + * the branch for worktree checkout. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +vi.mock('../shared/ui/index.js', () => ({ + info: vi.fn(), + error: vi.fn(), + withProgress: vi.fn(async (_start, _done, operation) => operation()), +})); + +vi.mock('../shared/prompt/index.js', () => ({ +})); + +vi.mock('../shared/utils/index.js', async (importOriginal) => ({ + ...(await importOriginal>()), + createLogger: () => ({ + info: vi.fn(), + debug: vi.fn(), + error: vi.fn(), + }), +})); + +const { mockCheckCliStatus, mockFetchIssue, mockFetchPrReviewComments } = vi.hoisted(() => ({ + mockCheckCliStatus: vi.fn(), + mockFetchIssue: vi.fn(), + mockFetchPrReviewComments: vi.fn(), +})); + +vi.mock('../infra/git/index.js', () => ({ + getGitProvider: () => ({ + checkCliStatus: (...args: unknown[]) => mockCheckCliStatus(...args), + fetchIssue: (...args: unknown[]) => mockFetchIssue(...args), + fetchPrReviewComments: (...args: unknown[]) => mockFetchPrReviewComments(...args), + }), +})); + +vi.mock('../infra/github/issue.js', () => ({ + parseIssueNumbers: vi.fn(() => []), + formatIssueAsTask: vi.fn(), + isIssueReference: vi.fn(), + resolveIssueTask: vi.fn(), +})); + +vi.mock('../features/tasks/index.js', () => ({ + selectAndExecuteTask: vi.fn(), + determinePiece: vi.fn(), + saveTaskFromInteractive: vi.fn(), + createIssueAndSaveTask: vi.fn(), + promptLabelSelection: vi.fn().mockResolvedValue([]), +})); + +vi.mock('../features/pipeline/index.js', () => ({ + executePipeline: vi.fn(), +})); + +vi.mock('../features/interactive/index.js', () => ({ + interactiveMode: vi.fn(), + selectInteractiveMode: vi.fn(() => 'assistant'), + passthroughMode: vi.fn(), + quietMode: vi.fn(), + personaMode: vi.fn(), + resolveLanguage: vi.fn(() => 'en'), + selectRun: vi.fn(() => null), + loadRunSessionContext: vi.fn(), + listRecentRuns: vi.fn(() => []), + normalizeTaskHistorySummary: vi.fn((items: unknown[]) => items), + dispatchConversationAction: vi.fn(async (result: { action: string }, handlers: Record unknown>) => { + return handlers[result.action](result); + }), +})); + +const mockListAllTaskItems = vi.fn(); +const mockIsStaleRunningTask = vi.fn(); +vi.mock('../infra/task/index.js', () => ({ + TaskRunner: vi.fn(() => ({ + listAllTaskItems: mockListAllTaskItems, + })), + isStaleRunningTask: (...args: unknown[]) => mockIsStaleRunningTask(...args), +})); + +vi.mock('../infra/config/index.js', () => ({ + getPieceDescription: vi.fn(() => ({ name: 'default', description: 'test piece', pieceStructure: '', movementPreviews: [] })), + resolveConfigValue: vi.fn((_: string, key: string) => (key === 'piece' ? 'default' : false)), + resolveConfigValues: vi.fn(() => ({ language: 'en', interactivePreviewMovements: 3, provider: 'claude' })), + loadPersonaSessions: vi.fn(() => ({})), +})); + +vi.mock('../shared/constants.js', () => ({ + DEFAULT_PIECE_NAME: 'default', +})); + +const mockOpts: Record = {}; + +vi.mock('../app/cli/program.js', () => { + const chainable = { + opts: vi.fn(() => mockOpts), + argument: vi.fn().mockReturnThis(), + action: vi.fn().mockReturnThis(), + }; + return { + program: chainable, + resolvedCwd: '/test/cwd', + pipelineMode: false, + }; +}); + +vi.mock('../app/cli/helpers.js', () => ({ + resolveAgentOverrides: vi.fn(), + parseCreateWorktreeOption: vi.fn(), + isDirectTask: vi.fn(() => false), +})); + +import { selectAndExecuteTask, determinePiece } from '../features/tasks/index.js'; +import { interactiveMode } from '../features/interactive/index.js'; +import { executePipeline } from '../features/pipeline/index.js'; +import { executeDefaultAction } from '../app/cli/routing.js'; +import { error as logError } from '../shared/ui/index.js'; +import type { PrReviewData } from '../infra/git/index.js'; + +const mockSelectAndExecuteTask = vi.mocked(selectAndExecuteTask); +const mockDeterminePiece = vi.mocked(determinePiece); +const mockInteractiveMode = vi.mocked(interactiveMode); +const mockExecutePipeline = vi.mocked(executePipeline); +const mockLogError = vi.mocked(logError); + +function createMockPrReview(overrides: Partial = {}): PrReviewData { + return { + number: 456, + title: 'Fix auth bug', + body: 'PR description', + url: 'https://github.com/org/repo/pull/456', + headRefName: 'fix/auth-bug', + comments: [{ author: 'commenter1', body: 'Update tests' }], + reviews: [{ author: 'reviewer1', body: 'Fix null check' }], + files: ['src/auth.ts'], + ...overrides, + }; +} + +beforeEach(() => { + vi.clearAllMocks(); + for (const key of Object.keys(mockOpts)) { + delete mockOpts[key]; + } + mockDeterminePiece.mockResolvedValue('default'); + mockInteractiveMode.mockResolvedValue({ action: 'execute', task: 'summarized task' }); + mockListAllTaskItems.mockReturnValue([]); + mockIsStaleRunningTask.mockReturnValue(false); +}); + +describe('PR resolution in routing', () => { + describe('--pr option', () => { + it('should resolve PR review comments and pass to interactive mode', async () => { + // Given + mockOpts.pr = 456; + const prReview = createMockPrReview(); + mockCheckCliStatus.mockReturnValue({ available: true }); + mockFetchPrReviewComments.mockReturnValue(prReview); + + // When + await executeDefaultAction(); + + // Then + expect(mockFetchPrReviewComments).toHaveBeenCalledWith(456); + expect(mockInteractiveMode).toHaveBeenCalledWith( + '/test/cwd', + expect.stringContaining('## PR #456 Review Comments:'), + expect.anything(), + undefined, + ); + }); + + it('should set branch in selectOptions from PR headRefName', async () => { + // Given + mockOpts.pr = 456; + const prReview = createMockPrReview({ headRefName: 'feat/my-pr-branch' }); + mockCheckCliStatus.mockReturnValue({ available: true }); + mockFetchPrReviewComments.mockReturnValue(prReview); + + // When + await executeDefaultAction(); + + // Then + expect(mockSelectAndExecuteTask).toHaveBeenCalledWith( + '/test/cwd', + 'summarized task', + expect.objectContaining({ + branch: 'feat/my-pr-branch', + }), + undefined, + ); + }); + + it('should exit with error when gh CLI is unavailable', async () => { + // Given + mockOpts.pr = 456; + mockCheckCliStatus.mockReturnValue({ + available: false, + error: 'gh CLI is not installed', + }); + + const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit called'); + }); + + // When/Then + await expect(executeDefaultAction()).rejects.toThrow('process.exit called'); + expect(mockExit).toHaveBeenCalledWith(1); + expect(mockInteractiveMode).not.toHaveBeenCalled(); + + mockExit.mockRestore(); + }); + + it('should exit with error when PR has no review comments', async () => { + // Given + mockOpts.pr = 456; + const emptyPrReview = createMockPrReview({ reviews: [], comments: [] }); + mockCheckCliStatus.mockReturnValue({ available: true }); + mockFetchPrReviewComments.mockReturnValue(emptyPrReview); + + const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit called'); + }); + + // When/Then + await expect(executeDefaultAction()).rejects.toThrow('process.exit called'); + expect(mockExit).toHaveBeenCalledWith(1); + expect(mockInteractiveMode).not.toHaveBeenCalled(); + + mockExit.mockRestore(); + }); + + it('should not resolve issues when --pr is specified', async () => { + // Given + mockOpts.pr = 456; + const prReview = createMockPrReview(); + mockCheckCliStatus.mockReturnValue({ available: true }); + mockFetchPrReviewComments.mockReturnValue(prReview); + + // When + await executeDefaultAction(); + + // Then + expect(mockFetchIssue).not.toHaveBeenCalled(); + }); + }); + + describe('--pr and --issue mutual exclusion', () => { + it('should exit with error when both --pr and --issue are specified', async () => { + // Given + mockOpts.pr = 456; + mockOpts.issue = 123; + + const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit called'); + }); + + // When/Then + await expect(executeDefaultAction()).rejects.toThrow('process.exit called'); + expect(mockLogError).toHaveBeenCalledWith('--pr and --issue cannot be used together'); + expect(mockExit).toHaveBeenCalledWith(1); + + mockExit.mockRestore(); + }); + }); + + describe('--pr and --task mutual exclusion', () => { + it('should exit with error when both --pr and --task are specified', async () => { + // Given + mockOpts.pr = 456; + mockOpts.task = 'some task'; + + const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit called'); + }); + + // When/Then + await expect(executeDefaultAction()).rejects.toThrow('process.exit called'); + expect(mockLogError).toHaveBeenCalledWith('--pr and --task cannot be used together'); + expect(mockExit).toHaveBeenCalledWith(1); + + mockExit.mockRestore(); + }); + }); + + describe('--pr in pipeline mode', () => { + it('should pass prNumber to executePipeline', async () => { + // Given: override pipelineMode + const programModule = await import('../app/cli/program.js'); + const originalPipelineMode = programModule.pipelineMode; + Object.defineProperty(programModule, 'pipelineMode', { value: true, writable: true }); + + mockOpts.pr = 456; + mockExecutePipeline.mockResolvedValue(0); + + // When + await executeDefaultAction(); + + // Then + expect(mockExecutePipeline).toHaveBeenCalledWith( + expect.objectContaining({ + prNumber: 456, + }), + ); + + // Cleanup + Object.defineProperty(programModule, 'pipelineMode', { value: originalPipelineMode, writable: true }); + }); + }); +}); diff --git a/src/__tests__/cli-worktree.test.ts b/src/__tests__/cli-worktree.test.ts index 771f275..fd24709 100644 --- a/src/__tests__/cli-worktree.test.ts +++ b/src/__tests__/cli-worktree.test.ts @@ -235,4 +235,39 @@ describe('confirmAndCreateWorktree', () => { expect(mockConfirm).not.toHaveBeenCalled(); expect(result.isWorktree).toBe(true); }); + + it('should pass branchOverride to createSharedClone', async () => { + // Given: branchOverride provided (e.g., PR head branch) + mockSummarizeTaskName.mockResolvedValue('fix-auth'); + mockCreateSharedClone.mockReturnValue({ + path: '/project/../20260128T0504-fix-auth', + branch: 'fix/pr-branch', + }); + + // When + await confirmAndCreateWorktree('/project', 'fix auth', true, 'fix/pr-branch'); + + // Then + expect(mockCreateSharedClone).toHaveBeenCalledWith('/project', expect.objectContaining({ + branch: 'fix/pr-branch', + })); + }); + + it('should not pass branch to createSharedClone when branchOverride is omitted', async () => { + // Given: no branchOverride + mockSummarizeTaskName.mockResolvedValue('fix-auth'); + mockCreateSharedClone.mockReturnValue({ + path: '/project/../20260128T0504-fix-auth', + branch: 'takt/20260128T0504-fix-auth', + }); + + // When + await confirmAndCreateWorktree('/project', 'fix auth', true); + + // Then + expect(mockCreateSharedClone).toHaveBeenCalledWith('/project', { + worktree: true, + taskSlug: 'fix-auth', + }); + }); }); diff --git a/src/__tests__/github-pr.test.ts b/src/__tests__/github-pr.test.ts index 3449713..b930c86 100644 --- a/src/__tests__/github-pr.test.ts +++ b/src/__tests__/github-pr.test.ts @@ -26,8 +26,9 @@ vi.mock('../shared/utils/index.js', async (importOriginal) => ({ getErrorMessage: (e: unknown) => String(e), })); -import { buildPrBody, findExistingPr, createPullRequest } from '../infra/github/pr.js'; +import { buildPrBody, findExistingPr, createPullRequest, fetchPrReviewComments, formatPrReviewAsTask } from '../infra/github/pr.js'; import type { GitHubIssue } from '../infra/github/types.js'; +import type { PrReviewData } from '../infra/git/types.js'; describe('findExistingPr', () => { beforeEach(() => { @@ -176,3 +177,194 @@ describe('buildPrBody', () => { }); }); + +describe('fetchPrReviewComments', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should parse gh pr view JSON and return PrReviewData', () => { + // Given + const ghResponse = { + number: 456, + title: 'Fix auth bug', + body: 'PR description', + url: 'https://github.com/org/repo/pull/456', + headRefName: 'fix/auth-bug', + comments: [ + { author: { login: 'commenter1' }, body: 'Please update tests' }, + ], + reviews: [ + { + author: { login: 'reviewer1' }, + body: 'Looks mostly good', + comments: [ + { body: 'Fix null check here', path: 'src/auth.ts', line: 42, author: { login: 'reviewer1' } }, + ], + }, + { + author: { login: 'reviewer2' }, + body: '', + comments: [], + }, + ], + files: [ + { path: 'src/auth.ts' }, + { path: 'src/auth.test.ts' }, + ], + }; + mockExecFileSync.mockReturnValue(JSON.stringify(ghResponse)); + + // When + const result = fetchPrReviewComments(456); + + // Then + expect(mockExecFileSync).toHaveBeenCalledWith( + 'gh', + ['pr', 'view', '456', '--json', 'number,title,body,url,headRefName,comments,reviews,files'], + expect.objectContaining({ encoding: 'utf-8' }), + ); + expect(result.number).toBe(456); + expect(result.title).toBe('Fix auth bug'); + expect(result.headRefName).toBe('fix/auth-bug'); + expect(result.comments).toEqual([{ author: 'commenter1', body: 'Please update tests' }]); + expect(result.reviews).toEqual([ + { author: 'reviewer1', body: 'Looks mostly good' }, + { author: 'reviewer1', body: 'Fix null check here', path: 'src/auth.ts', line: 42 }, + ]); + expect(result.files).toEqual(['src/auth.ts', 'src/auth.test.ts']); + }); + + it('should skip reviews with empty body', () => { + // Given + const ghResponse = { + number: 10, + title: 'Approved PR', + body: '', + url: 'https://github.com/org/repo/pull/10', + headRefName: 'feat/approved', + comments: [], + reviews: [ + { author: { login: 'approver' }, body: '', comments: [] }, + ], + files: [], + }; + mockExecFileSync.mockReturnValue(JSON.stringify(ghResponse)); + + // When + const result = fetchPrReviewComments(10); + + // Then + expect(result.reviews).toEqual([]); + }); + + it('should throw when gh CLI fails', () => { + // Given + mockExecFileSync.mockImplementation(() => { throw new Error('gh: PR not found'); }); + + // When/Then + expect(() => fetchPrReviewComments(999)).toThrow('gh: PR not found'); + }); +}); + +describe('formatPrReviewAsTask', () => { + it('should format PR review data with all sections', () => { + // Given + const prReview: PrReviewData = { + number: 456, + title: 'Fix auth bug', + body: 'PR description text', + url: 'https://github.com/org/repo/pull/456', + headRefName: 'fix/auth-bug', + comments: [ + { author: 'commenter1', body: 'Can you also update the tests?' }, + ], + reviews: [ + { author: 'reviewer1', body: 'Fix the null check in auth.ts', path: 'src/auth.ts', line: 42 }, + { author: 'reviewer2', body: 'This function should handle edge cases' }, + ], + files: ['src/auth.ts', 'src/auth.test.ts'], + }; + + // When + const result = formatPrReviewAsTask(prReview); + + // Then + expect(result).toContain('## PR #456 Review Comments: Fix auth bug'); + expect(result).toContain('### PR Description'); + expect(result).toContain('PR description text'); + expect(result).toContain('### Review Comments'); + expect(result).toContain('**reviewer1**: Fix the null check in auth.ts'); + expect(result).toContain('File: src/auth.ts, Line: 42'); + expect(result).toContain('**reviewer2**: This function should handle edge cases'); + expect(result).toContain('### Conversation Comments'); + expect(result).toContain('**commenter1**: Can you also update the tests?'); + expect(result).toContain('### Changed Files'); + expect(result).toContain('- src/auth.ts'); + expect(result).toContain('- src/auth.test.ts'); + }); + + it('should omit PR Description when body is empty', () => { + // Given + const prReview: PrReviewData = { + number: 10, + title: 'Quick fix', + body: '', + url: 'https://github.com/org/repo/pull/10', + headRefName: 'fix/quick', + comments: [], + reviews: [{ author: 'reviewer', body: 'Fix this' }], + files: [], + }; + + // When + const result = formatPrReviewAsTask(prReview); + + // Then + expect(result).not.toContain('### PR Description'); + expect(result).toContain('### Review Comments'); + }); + + it('should omit empty sections', () => { + // Given + const prReview: PrReviewData = { + number: 20, + title: 'Empty review', + body: '', + url: 'https://github.com/org/repo/pull/20', + headRefName: 'feat/empty', + comments: [], + reviews: [{ author: 'reviewer', body: 'Add tests' }], + files: [], + }; + + // When + const result = formatPrReviewAsTask(prReview); + + // Then + expect(result).not.toContain('### Conversation Comments'); + expect(result).not.toContain('### Changed Files'); + expect(result).toContain('### Review Comments'); + }); + + it('should format inline comment with path but no line', () => { + // Given + const prReview: PrReviewData = { + number: 30, + title: 'Path only', + body: '', + url: 'https://github.com/org/repo/pull/30', + headRefName: 'feat/path-only', + comments: [], + reviews: [{ author: 'reviewer', body: 'Fix this', path: 'src/index.ts' }], + files: [], + }; + + // When + const result = formatPrReviewAsTask(prReview); + + // Then + expect(result).toContain('File: src/index.ts'); + expect(result).not.toContain('Line:'); + }); +}); diff --git a/src/__tests__/github-provider.test.ts b/src/__tests__/github-provider.test.ts index 5c1301e..7329408 100644 --- a/src/__tests__/github-provider.test.ts +++ b/src/__tests__/github-provider.test.ts @@ -15,6 +15,7 @@ const { mockFindExistingPr, mockCommentOnPr, mockCreatePullRequest, + mockFetchPrReviewComments, } = vi.hoisted(() => ({ mockCheckGhCli: vi.fn(), mockFetchIssue: vi.fn(), @@ -22,6 +23,7 @@ const { mockFindExistingPr: vi.fn(), mockCommentOnPr: vi.fn(), mockCreatePullRequest: vi.fn(), + mockFetchPrReviewComments: vi.fn(), })); vi.mock('../infra/github/issue.js', () => ({ @@ -34,11 +36,12 @@ vi.mock('../infra/github/pr.js', () => ({ findExistingPr: (...args: unknown[]) => mockFindExistingPr(...args), commentOnPr: (...args: unknown[]) => mockCommentOnPr(...args), createPullRequest: (...args: unknown[]) => mockCreatePullRequest(...args), + fetchPrReviewComments: (...args: unknown[]) => mockFetchPrReviewComments(...args), })); import { GitHubProvider } from '../infra/github/GitHubProvider.js'; import { getGitProvider } from '../infra/git/index.js'; -import type { CommentResult } from '../infra/git/index.js'; +import type { CommentResult, PrReviewData } from '../infra/git/index.js'; beforeEach(() => { vi.clearAllMocks(); @@ -206,6 +209,31 @@ describe('GitHubProvider', () => { expect(result.error).toBe('Permission denied'); }); }); + + describe('fetchPrReviewComments', () => { + it('fetchPrReviewComments(n) に委譲し結果を返す', () => { + // Given + const prReview: PrReviewData = { + number: 456, + title: 'Fix bug', + body: 'Description', + url: 'https://github.com/org/repo/pull/456', + headRefName: 'fix/bug', + comments: [], + reviews: [{ author: 'reviewer', body: 'Fix this' }], + files: ['src/index.ts'], + }; + mockFetchPrReviewComments.mockReturnValue(prReview); + const provider = new GitHubProvider(); + + // When + const result = provider.fetchPrReviewComments(456); + + // Then + expect(mockFetchPrReviewComments).toHaveBeenCalledWith(456); + expect(result).toBe(prReview); + }); + }); }); describe('getGitProvider', () => { @@ -217,6 +245,7 @@ describe('getGitProvider', () => { expect(typeof provider.checkCliStatus).toBe('function'); expect(typeof provider.fetchIssue).toBe('function'); expect(typeof provider.createIssue).toBe('function'); + expect(typeof provider.fetchPrReviewComments).toBe('function'); expect(typeof provider.findExistingPr).toBe('function'); expect(typeof provider.createPullRequest).toBe('function'); expect(typeof provider.commentOnPr).toBe('function'); diff --git a/src/__tests__/pipelineExecution.test.ts b/src/__tests__/pipelineExecution.test.ts index 221f780..9b2a208 100644 --- a/src/__tests__/pipelineExecution.test.ts +++ b/src/__tests__/pipelineExecution.test.ts @@ -20,9 +20,15 @@ vi.mock('../infra/github/issue.js', () => ({ const mockCreatePullRequest = vi.fn(); const mockPushBranch = vi.fn(); const mockBuildPrBody = vi.fn(() => 'Default PR body'); +const mockFetchPrReviewComments = vi.fn(); +const mockFormatPrReviewAsTask = vi.fn((pr: { number: number; title: string }) => + `## PR #${pr.number} Review Comments: ${pr.title}` +); vi.mock('../infra/github/pr.js', () => ({ createPullRequest: mockCreatePullRequest, buildPrBody: mockBuildPrBody, + fetchPrReviewComments: (...args: unknown[]) => mockFetchPrReviewComments(...args), + formatPrReviewAsTask: (...args: unknown[]) => mockFormatPrReviewAsTask(...args), })); vi.mock('../infra/task/git.js', async (importOriginal) => ({ @@ -528,7 +534,7 @@ describe('executePipeline', () => { }); expect(exitCode).toBe(0); - expect(mockConfirmAndCreateWorktree).toHaveBeenCalledWith('/tmp/test', 'Fix the bug', true); + expect(mockConfirmAndCreateWorktree).toHaveBeenCalledWith('/tmp/test', 'Fix the bug', true, undefined); expect(mockExecuteTask).toHaveBeenCalledWith({ task: 'Fix the bug', cwd: '/tmp/test-worktree', @@ -812,4 +818,119 @@ describe('executePipeline', () => { ); }); }); + + describe('--pr pipeline', () => { + it('should resolve PR review comments and execute pipeline with PR branch checkout', async () => { + mockFetchPrReviewComments.mockReturnValueOnce({ + number: 456, + title: 'Fix auth bug', + body: 'PR description', + url: 'https://github.com/org/repo/pull/456', + headRefName: 'fix/auth-bug', + comments: [{ author: 'commenter1', body: 'Update tests' }], + reviews: [{ author: 'reviewer1', body: 'Fix null check' }], + files: ['src/auth.ts'], + }); + mockExecuteTask.mockResolvedValueOnce(true); + + const exitCode = await executePipeline({ + prNumber: 456, + piece: 'default', + autoPr: false, + cwd: '/tmp/test', + }); + + expect(exitCode).toBe(0); + expect(mockFetchPrReviewComments).toHaveBeenCalledWith(456); + expect(mockFormatPrReviewAsTask).toHaveBeenCalled(); + // PR branch checkout + const checkoutCall = mockExecFileSync.mock.calls.find( + (call: unknown[]) => call[0] === 'git' && (call[1] as string[])[0] === 'checkout' && (call[1] as string[])[1] === 'fix/auth-bug', + ); + expect(checkoutCall).toBeDefined(); + }); + + it('should return exit code 2 when gh CLI is unavailable for --pr', async () => { + mockCheckGhCli.mockReturnValueOnce({ available: false, error: 'gh not found' }); + + const exitCode = await executePipeline({ + prNumber: 456, + piece: 'default', + autoPr: false, + cwd: '/tmp/test', + }); + + expect(exitCode).toBe(2); + }); + + it('should return exit code 2 when PR has no review comments', async () => { + mockFetchPrReviewComments.mockReturnValueOnce({ + number: 456, + title: 'Fix auth bug', + body: 'PR description', + url: 'https://github.com/org/repo/pull/456', + headRefName: 'fix/auth-bug', + comments: [], + reviews: [], + files: ['src/auth.ts'], + }); + + const exitCode = await executePipeline({ + prNumber: 456, + piece: 'default', + autoPr: false, + cwd: '/tmp/test', + }); + + expect(exitCode).toBe(2); + }); + + it('should return exit code 2 when PR fetch fails', async () => { + mockFetchPrReviewComments.mockImplementationOnce(() => { + throw new Error('PR not found'); + }); + + const exitCode = await executePipeline({ + prNumber: 999, + piece: 'default', + autoPr: false, + cwd: '/tmp/test', + }); + + expect(exitCode).toBe(2); + }); + + it('should checkout PR branch instead of creating new branch', async () => { + mockFetchPrReviewComments.mockReturnValueOnce({ + number: 456, + title: 'Fix auth bug', + body: 'PR description', + url: 'https://github.com/org/repo/pull/456', + headRefName: 'fix/auth-bug', + comments: [{ author: 'reviewer1', body: 'Fix this' }], + reviews: [], + files: ['src/auth.ts'], + }); + mockExecuteTask.mockResolvedValueOnce(true); + + const exitCode = await executePipeline({ + prNumber: 456, + piece: 'default', + autoPr: false, + cwd: '/tmp/test', + }); + + expect(exitCode).toBe(0); + // Should checkout existing branch, not create new + const checkoutNewBranch = mockExecFileSync.mock.calls.find( + (call: unknown[]) => call[0] === 'git' && (call[1] as string[])[0] === 'checkout' && (call[1] as string[])[1] === '-b', + ); + expect(checkoutNewBranch).toBeUndefined(); + // Should checkout existing PR branch + const checkoutPrBranch = mockExecFileSync.mock.calls.find( + (call: unknown[]) => call[0] === 'git' && (call[1] as string[])[0] === 'checkout' && (call[1] as string[])[1] === 'fix/auth-bug', + ); + expect(checkoutPrBranch).toBeDefined(); + }); + }); }); diff --git a/src/app/cli/program.ts b/src/app/cli/program.ts index 4488040..05f0cd2 100644 --- a/src/app/cli/program.ts +++ b/src/app/cli/program.ts @@ -41,6 +41,7 @@ program // --- Global options --- program .option('-i, --issue ', 'GitHub issue number (equivalent to #N)', (val: string) => parseInt(val, 10)) + .option('--pr ', 'PR number to fetch review comments and fix', (val: string) => parseInt(val, 10)) .option('-w, --piece ', 'Piece name or path to piece file') .option('-b, --branch ', 'Branch name (auto-generated if omitted)') .option('--auto-pr', 'Create PR after successful execution') diff --git a/src/app/cli/routing.ts b/src/app/cli/routing.ts index 30b8780..648e583 100644 --- a/src/app/cli/routing.ts +++ b/src/app/cli/routing.ts @@ -8,9 +8,9 @@ import { info, error as logError, withProgress } from '../../shared/ui/index.js'; import { getErrorMessage } from '../../shared/utils/index.js'; import { getLabel } from '../../shared/i18n/index.js'; -import { formatIssueAsTask, parseIssueNumbers } from '../../infra/github/index.js'; +import { formatIssueAsTask, parseIssueNumbers, formatPrReviewAsTask } from '../../infra/github/index.js'; import { getGitProvider } from '../../infra/git/index.js'; -import type { Issue } from '../../infra/git/index.js'; +import type { Issue, PrReviewData } from '../../infra/git/index.js'; import { selectAndExecuteTask, determinePiece, saveTaskFromInteractive, createIssueAndSaveTask, promptLabelSelection, type SelectAndExecuteOptions } from '../../features/tasks/index.js'; import { executePipeline } from '../../features/pipeline/index.js'; import { @@ -76,12 +76,56 @@ async function resolveIssueInput( return null; } +/** + * Resolve PR review comments from `--pr` option. + * + * Fetches review comments and metadata, formats as task text. + * Returns the PR branch name for checkout and the formatted task. + * Throws on gh CLI unavailability or fetch failure. + */ +async function resolvePrInput( + prNumber: number, +): Promise<{ initialInput: string; prBranch: string }> { + const ghStatus = getGitProvider().checkCliStatus(); + if (!ghStatus.available) { + throw new Error(ghStatus.error); + } + + const prReview = await withProgress( + 'Fetching PR review comments...', + (pr: PrReviewData) => `PR fetched: #${pr.number} ${pr.title}`, + async () => getGitProvider().fetchPrReviewComments(prNumber), + ); + + if (prReview.reviews.length === 0 && prReview.comments.length === 0) { + throw new Error(`PR #${prNumber} has no review comments`); + } + + return { + initialInput: formatPrReviewAsTask(prReview), + prBranch: prReview.headRefName, + }; +} + /** * Execute default action: handle task execution, pipeline mode, or interactive mode. * Exported for use in slash-command fallback logic. */ export async function executeDefaultAction(task?: string): Promise { const opts = program.opts(); + const prNumber = opts.pr as number | undefined; + const issueNumber = opts.issue as number | undefined; + + if (prNumber && issueNumber) { + logError('--pr and --issue cannot be used together'); + process.exit(1); + } + + if (prNumber && (opts.task as string | undefined)) { + logError('--pr and --task cannot be used together'); + process.exit(1); + } + const agentOverrides = resolveAgentOverrides(program); const createWorktreeOverride = parseCreateWorktreeOption(opts.createWorktree as string | undefined); const resolvedPipelinePiece = (opts.piece as string | undefined) ?? resolveConfigValue(resolvedCwd, 'piece'); @@ -102,7 +146,8 @@ export async function executeDefaultAction(task?: string): Promise { // --- Pipeline mode (non-interactive): triggered by --pipeline --- if (pipelineMode) { const exitCode = await executePipeline({ - issueNumber: opts.issue as number | undefined, + issueNumber, + prNumber, task: opts.task as string | undefined, piece: resolvedPipelinePiece, branch: opts.branch as string | undefined, @@ -132,18 +177,30 @@ export async function executeDefaultAction(task?: string): Promise { return; } - // Resolve issue references (--issue N or #N positional arg) before interactive mode + // Resolve PR review comments (--pr N) before interactive mode let initialInput: string | undefined = task; - try { - const issueResult = await resolveIssueInput(opts.issue as number | undefined, task); - if (issueResult) { - selectOptions.issues = issueResult.issues; - initialInput = issueResult.initialInput; + if (prNumber) { + try { + const prResult = await resolvePrInput(prNumber); + initialInput = prResult.initialInput; + selectOptions.branch = prResult.prBranch; + } catch (e) { + logError(getErrorMessage(e)); + process.exit(1); + } + } else { + // Resolve issue references (--issue N or #N positional arg) before interactive mode + try { + const issueResult = await resolveIssueInput(issueNumber, task); + if (issueResult) { + selectOptions.issues = issueResult.issues; + initialInput = issueResult.initialInput; + } + } catch (e) { + logError(getErrorMessage(e)); + process.exit(1); } - } catch (e) { - logError(getErrorMessage(e)); - process.exit(1); } // All paths below go through interactive mode diff --git a/src/features/pipeline/execute.ts b/src/features/pipeline/execute.ts index 70182a8..ce9e98b 100644 --- a/src/features/pipeline/execute.ts +++ b/src/features/pipeline/execute.ts @@ -59,7 +59,7 @@ async function runPipeline(options: PipelineExecutionOptions): Promise( + label: string, + fetch: (provider: ReturnType) => T, +): T | undefined { + const gitProvider = getGitProvider(); + const cliStatus = gitProvider.checkCliStatus(); + if (!cliStatus.available) { + error(cliStatus.error ?? 'gh CLI is not available'); + return undefined; + } + try { + return fetch(gitProvider); + } catch (err) { + error(`Failed to fetch ${label}: ${getErrorMessage(err)}`); + return undefined; + } +} + export function resolveTaskContent(options: PipelineExecutionOptions): TaskContent | undefined { + if (options.prNumber) { + info(`Fetching PR #${options.prNumber} review comments...`); + const prReview = fetchGitHubResource( + `PR #${options.prNumber}`, + (provider) => provider.fetchPrReviewComments(options.prNumber!), + ); + if (!prReview) return undefined; + if (prReview.reviews.length === 0 && prReview.comments.length === 0) { + error(`PR #${options.prNumber} has no review comments`); + return undefined; + } + const task = formatPrReviewAsTask(prReview); + success(`PR #${options.prNumber} fetched: "${prReview.title}"`); + return { task, prBranch: prReview.headRefName }; + } if (options.issueNumber) { info(`Fetching issue #${options.issueNumber}...`); - const gitProvider = getGitProvider(); - const cliStatus = gitProvider.checkCliStatus(); - if (!cliStatus.available) { - error(cliStatus.error ?? 'gh CLI is not available'); - return undefined; - } - try { - const issue = gitProvider.fetchIssue(options.issueNumber); - const task = formatIssueAsTask(issue); - success(`Issue #${options.issueNumber} fetched: "${issue.title}"`); - return { task, issue }; - } catch (err) { - error(`Failed to fetch issue #${options.issueNumber}: ${getErrorMessage(err)}`); - return undefined; - } + const issue = fetchGitHubResource( + `issue #${options.issueNumber}`, + (provider) => provider.fetchIssue(options.issueNumber!), + ); + if (!issue) return undefined; + const task = formatIssueAsTask(issue); + success(`Issue #${options.issueNumber} fetched: "${issue.title}"`); + return { task, issue }; } if (options.task) { return { task: options.task }; } - error('Either --issue or --task must be specified'); + error('Either --issue, --pr, or --task must be specified'); return undefined; } @@ -111,9 +140,10 @@ export async function resolveExecutionContext( task: string, options: Pick, pipelineConfig: PipelineConfig | undefined, + prBranch?: string, ): Promise { if (options.createWorktree) { - const result = await confirmAndCreateWorktree(cwd, task, options.createWorktree); + const result = await confirmAndCreateWorktree(cwd, task, options.createWorktree, prBranch); if (result.isWorktree) { success(`Worktree created: ${result.execCwd}`); } @@ -122,6 +152,13 @@ export async function resolveExecutionContext( if (options.skipGit) { return { execCwd: cwd, isWorktree: false }; } + if (prBranch) { + info(`Fetching and checking out PR branch: ${prBranch}`); + execFileSync('git', ['fetch', 'origin', prBranch], { cwd, stdio: 'pipe' }); + execFileSync('git', ['checkout', prBranch], { cwd, stdio: 'pipe' }); + success(`Checked out PR branch: ${prBranch}`); + return { execCwd: cwd, branch: prBranch, baseBranch: resolveBaseBranch(cwd).branch, isWorktree: false }; + } const resolved = resolveBaseBranch(cwd); const branch = options.branch ?? generatePipelineBranchName(pipelineConfig, options.issueNumber); info(`Creating branch: ${branch}`); diff --git a/src/features/tasks/execute/selectAndExecute.ts b/src/features/tasks/execute/selectAndExecute.ts index 54dce3e..965a798 100644 --- a/src/features/tasks/execute/selectAndExecute.ts +++ b/src/features/tasks/execute/selectAndExecute.ts @@ -43,6 +43,7 @@ export async function confirmAndCreateWorktree( cwd: string, task: string, createWorktreeOverride?: boolean | undefined, + branchOverride?: string, ): Promise { const useWorktree = typeof createWorktreeOverride === 'boolean' @@ -67,6 +68,7 @@ export async function confirmAndCreateWorktree( async () => createSharedClone(cwd, { worktree: true, taskSlug, + ...(branchOverride ? { branch: branchOverride } : {}), }), ); @@ -94,6 +96,7 @@ export async function selectAndExecuteTask( cwd, task, options?.createWorktree, + options?.branch, ); // Ask for PR creation BEFORE execution (only if worktree is enabled) diff --git a/src/features/tasks/execute/types.ts b/src/features/tasks/execute/types.ts index 97b8664..ae1dd44 100644 --- a/src/features/tasks/execute/types.ts +++ b/src/features/tasks/execute/types.ts @@ -103,6 +103,8 @@ export interface ExecuteTaskOptions { export interface PipelineExecutionOptions { /** GitHub issue number */ issueNumber?: number; + /** PR number to fetch review comments */ + prNumber?: number; /** Task content (alternative to issue) */ task?: string; /** Piece name or path to piece file */ @@ -139,6 +141,8 @@ export interface SelectAndExecuteOptions { repo?: string; piece?: string; createWorktree?: boolean | undefined; + /** Override branch name (e.g., PR head branch for --pr) */ + branch?: string; /** Enable interactive user input during step transitions */ interactiveUserInput?: boolean; /** Interactive mode result metadata for NDJSON logging */ diff --git a/src/infra/git/index.ts b/src/infra/git/index.ts index a23f5a8..0983682 100644 --- a/src/infra/git/index.ts +++ b/src/infra/git/index.ts @@ -7,7 +7,7 @@ import { GitHubProvider } from '../github/GitHubProvider.js'; import type { GitProvider } from './types.js'; -export type { GitProvider, Issue, CliStatus, ExistingPr, CreatePrOptions, CreatePrResult, CommentResult, CreateIssueOptions, CreateIssueResult } from './types.js'; +export type { GitProvider, Issue, CliStatus, ExistingPr, CreatePrOptions, CreatePrResult, CommentResult, CreateIssueOptions, CreateIssueResult, PrReviewComment, PrReviewData } from './types.js'; let provider: GitProvider | undefined; diff --git a/src/infra/git/types.ts b/src/infra/git/types.ts index 3ff58c0..36ab861 100644 --- a/src/infra/git/types.ts +++ b/src/infra/git/types.ts @@ -69,6 +69,32 @@ export interface CreateIssueResult { error?: string; } +/** PR review comment (conversation or inline) */ +export interface PrReviewComment { + author: string; + body: string; + /** File path for inline comments (undefined for conversation comments) */ + path?: string; + /** Line number for inline comments */ + line?: number; +} + +/** PR review data including metadata and review comments */ +export interface PrReviewData { + number: number; + title: string; + body: string; + url: string; + /** Branch name of the PR head */ + headRefName: string; + /** Conversation comments (non-review) */ + comments: PrReviewComment[]; + /** Review comments (from reviews) */ + reviews: PrReviewComment[]; + /** Changed file paths */ + files: string[]; +} + export interface GitProvider { /** Check CLI tool availability and authentication status */ checkCliStatus(): CliStatus; @@ -77,6 +103,9 @@ export interface GitProvider { createIssue(options: CreateIssueOptions): CreateIssueResult; + /** Fetch PR review comments and metadata */ + fetchPrReviewComments(prNumber: number): PrReviewData; + /** Find an open PR for the given branch. Returns undefined if no PR exists. */ findExistingPr(cwd: string, branch: string): ExistingPr | undefined; diff --git a/src/infra/github/GitHubProvider.ts b/src/infra/github/GitHubProvider.ts index 9af2b6d..b92c6f5 100644 --- a/src/infra/github/GitHubProvider.ts +++ b/src/infra/github/GitHubProvider.ts @@ -7,8 +7,8 @@ */ import { checkGhCli, fetchIssue, createIssue } from './issue.js'; -import { findExistingPr, commentOnPr, createPullRequest } from './pr.js'; -import type { GitProvider, CliStatus, Issue, ExistingPr, CreateIssueOptions, CreateIssueResult, CreatePrOptions, CreatePrResult, CommentResult } from '../git/types.js'; +import { findExistingPr, commentOnPr, createPullRequest, fetchPrReviewComments } from './pr.js'; +import type { GitProvider, CliStatus, Issue, ExistingPr, CreateIssueOptions, CreateIssueResult, CreatePrOptions, CreatePrResult, CommentResult, PrReviewData } from '../git/types.js'; export class GitHubProvider implements GitProvider { checkCliStatus(): CliStatus { @@ -23,6 +23,10 @@ export class GitHubProvider implements GitProvider { return createIssue(options); } + fetchPrReviewComments(prNumber: number): PrReviewData { + return fetchPrReviewComments(prNumber); + } + findExistingPr(cwd: string, branch: string): ExistingPr | undefined { return findExistingPr(cwd, branch); } diff --git a/src/infra/github/index.ts b/src/infra/github/index.ts index 865352f..b9b5e7c 100644 --- a/src/infra/github/index.ts +++ b/src/infra/github/index.ts @@ -9,4 +9,4 @@ export { resolveIssueTask, } from './issue.js'; -export { buildPrBody } from './pr.js'; +export { buildPrBody, formatPrReviewAsTask } from './pr.js'; diff --git a/src/infra/github/pr.ts b/src/infra/github/pr.ts index 5754309..5876e37 100644 --- a/src/infra/github/pr.ts +++ b/src/infra/github/pr.ts @@ -7,7 +7,7 @@ import { execFileSync } from 'node:child_process'; import { createLogger, getErrorMessage } from '../../shared/utils/index.js'; import { checkGhCli } from './issue.js'; -import type { Issue, CreatePrOptions, CreatePrResult, ExistingPr, CommentResult } from '../git/types.js'; +import type { Issue, CreatePrOptions, CreatePrResult, ExistingPr, CommentResult, PrReviewData, PrReviewComment } from '../git/types.js'; const log = createLogger('github-pr'); @@ -52,6 +52,116 @@ export function commentOnPr(cwd: string, prNumber: number, body: string): Commen } } +/** JSON fields requested from `gh pr view` for review data */ +const PR_REVIEW_JSON_FIELDS = 'number,title,body,url,headRefName,comments,reviews,files'; + +/** Raw shape returned by `gh pr view --json` for review data */ +interface GhPrViewReviewResponse { + number: number; + title: string; + body: string; + url: string; + headRefName: string; + comments: Array<{ author: { login: string }; body: string }>; + reviews: Array<{ + author: { login: string }; + body: string; + comments: Array<{ body: string; path: string; line: number; author: { login: string } }>; + }>; + files: Array<{ path: string }>; +} + +/** + * Fetch PR review comments and metadata via `gh pr view`. + * Throws on failure (PR not found, network error, etc.). + */ +export function fetchPrReviewComments(prNumber: number): PrReviewData { + log.debug('Fetching PR review comments', { prNumber }); + + const raw = execFileSync( + 'gh', + ['pr', 'view', String(prNumber), '--json', PR_REVIEW_JSON_FIELDS], + { encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] }, + ); + + const data = JSON.parse(raw) as GhPrViewReviewResponse; + + const comments: PrReviewComment[] = data.comments.map((c) => ({ + author: c.author.login, + body: c.body, + })); + + const reviews: PrReviewComment[] = []; + for (const review of data.reviews) { + if (review.body) { + reviews.push({ author: review.author.login, body: review.body }); + } + for (const comment of review.comments) { + reviews.push({ + author: comment.author.login, + body: comment.body, + path: comment.path, + line: comment.line, + }); + } + } + + return { + number: data.number, + title: data.title, + body: data.body, + url: data.url, + headRefName: data.headRefName, + comments, + reviews, + files: data.files.map((f) => f.path), + }; +} + +/** + * Format PR review data into task text for piece execution. + */ +export function formatPrReviewAsTask(prReview: PrReviewData): string { + const parts: string[] = []; + + parts.push(`## PR #${prReview.number} Review Comments: ${prReview.title}`); + + if (prReview.body) { + parts.push(''); + parts.push('### PR Description'); + parts.push(prReview.body); + } + + if (prReview.reviews.length > 0) { + parts.push(''); + parts.push('### Review Comments'); + for (const review of prReview.reviews) { + const location = review.path + ? `\n File: ${review.path}${review.line ? `, Line: ${review.line}` : ''}` + : ''; + parts.push(`**${review.author}**: ${review.body}${location}`); + } + } + + if (prReview.comments.length > 0) { + parts.push(''); + parts.push('### Conversation Comments'); + for (const comment of prReview.comments) { + parts.push(`**${comment.author}**: ${comment.body}`); + } + } + + if (prReview.files.length > 0) { + parts.push(''); + parts.push('### Changed Files'); + for (const file of prReview.files) { + parts.push(`- ${file}`); + } + } + + return parts.join('\n'); +} + export function createPullRequest(cwd: string, options: CreatePrOptions): CreatePrResult { const ghStatus = checkGhCli(); if (!ghStatus.available) {