diff --git a/src/main/ipc/filesystem.test.ts b/src/main/ipc/filesystem.test.ts index e94d20d9ce1..5d81b363223 100644 --- a/src/main/ipc/filesystem.test.ts +++ b/src/main/ipc/filesystem.test.ts @@ -28,7 +28,10 @@ const { listWorktreesMock, resolveCommitMessageSettingsMock, generateCommitMessageFromContextMock, + generatePullRequestFieldsFromContextMock, cancelGenerateCommitMessageLocalMock, + cancelGeneratePullRequestFieldsLocalMock, + getPullRequestDraftContextMock, getSshFilesystemProviderMock, getSshGitProviderMock } = vi.hoisted(() => ({ @@ -56,7 +59,10 @@ const { listWorktreesMock: vi.fn(), resolveCommitMessageSettingsMock: vi.fn(), generateCommitMessageFromContextMock: vi.fn(), + generatePullRequestFieldsFromContextMock: vi.fn(), cancelGenerateCommitMessageLocalMock: vi.fn(), + cancelGeneratePullRequestFieldsLocalMock: vi.fn(), + getPullRequestDraftContextMock: vi.fn(), getSshFilesystemProviderMock: vi.fn(), getSshGitProviderMock: vi.fn() })) @@ -110,7 +116,13 @@ vi.mock('../providers/ssh-git-dispatch', () => ({ vi.mock('../text-generation/commit-message-text-generation', () => ({ resolveCommitMessageSettings: resolveCommitMessageSettingsMock, generateCommitMessageFromContext: generateCommitMessageFromContextMock, - cancelGenerateCommitMessageLocal: cancelGenerateCommitMessageLocalMock + generatePullRequestFieldsFromContext: generatePullRequestFieldsFromContextMock, + cancelGenerateCommitMessageLocal: cancelGenerateCommitMessageLocalMock, + cancelGeneratePullRequestFieldsLocal: cancelGeneratePullRequestFieldsLocalMock +})) + +vi.mock('../text-generation/pull-request-context', () => ({ + getPullRequestDraftContext: getPullRequestDraftContextMock })) import { registerFilesystemHandlers } from './filesystem' @@ -186,7 +198,10 @@ describe('registerFilesystemHandlers', () => { listWorktreesMock, resolveCommitMessageSettingsMock, generateCommitMessageFromContextMock, + generatePullRequestFieldsFromContextMock, cancelGenerateCommitMessageLocalMock, + cancelGeneratePullRequestFieldsLocalMock, + getPullRequestDraftContextMock, getSshFilesystemProviderMock, getSshGitProviderMock ]) { @@ -1051,6 +1066,83 @@ describe('registerFilesystemHandlers', () => { expect(generateCommitMessageFromContextMock).not.toHaveBeenCalled() }) + it('generates SSH pull request fields in the pull-request-fields lane', async () => { + const params = { agentId: 'custom', model: '', customAgentCommand: 'agent' } + const context = { + branch: 'feature/pr-fields', + base: 'main', + currentTitle: '', + currentBody: '', + currentDraft: false, + commitSummary: '- feat: add PR fields', + changeSummary: 'M\tREADME.md', + patch: '+hello' + } + const executeCommitMessagePlan = vi.fn().mockResolvedValue({ + stdout: '{"base":"main","title":"Update README","body":"","draft":false}', + stderr: '', + exitCode: 0, + timedOut: false + }) + resolveCommitMessageSettingsMock.mockReturnValue({ ok: true, params }) + getPullRequestDraftContextMock.mockResolvedValue(context) + getSshGitProviderMock.mockReturnValue({ + exec: vi.fn(), + executeCommitMessagePlan + }) + generatePullRequestFieldsFromContextMock.mockResolvedValue({ + success: true, + fields: { base: 'main', title: 'Update README', body: '', draft: false } + }) + + registerFilesystemHandlers(store as never) + + await expect( + handlers.get('git:generatePullRequestFields')!(null, { + worktreePath: '/remote/repo', + connectionId: 'conn-1', + base: 'main', + title: '', + body: '', + draft: false + }) + ).resolves.toMatchObject({ success: true }) + + const target = generatePullRequestFieldsFromContextMock.mock.calls[0]?.[2] as { + execute: ( + plan: { binary: string; args: string[]; stdinPayload: string | null; label: string }, + cwd: string, + timeoutMs: number + ) => Promise + } + await target.execute( + { binary: 'agent', args: [], stdinPayload: null, label: 'agent' }, + '/remote/repo', + 1 + ) + expect(executeCommitMessagePlan).toHaveBeenCalledWith( + { binary: 'agent', args: [], stdinPayload: null, label: 'agent' }, + '/remote/repo', + 1, + 'pull-request-fields' + ) + }) + + it('cancels SSH pull request field generation in the pull-request-fields lane', async () => { + const cancelGenerateCommitMessage = vi.fn().mockResolvedValue(undefined) + getSshGitProviderMock.mockReturnValue({ cancelGenerateCommitMessage }) + + registerFilesystemHandlers(store as never) + + await handlers.get('git:cancelGeneratePullRequestFields')!(null, { + worktreePath: '/remote/repo', + connectionId: 'conn-1' + }) + + expect(cancelGenerateCommitMessage).toHaveBeenCalledWith('/remote/repo', 'pull-request-fields') + expect(cancelGeneratePullRequestFieldsLocalMock).not.toHaveBeenCalled() + }) + it('routes ssh git:commit through the SSH provider instead of local commitChanges', async () => { const sshCommitMock = vi.fn().mockResolvedValue({ success: true }) getSshGitProviderMock.mockReturnValue({ commit: sshCommitMock }) diff --git a/src/main/ipc/filesystem.ts b/src/main/ipc/filesystem.ts index d96f9a733b7..ccc613311b3 100644 --- a/src/main/ipc/filesystem.ts +++ b/src/main/ipc/filesystem.ts @@ -736,7 +736,7 @@ export function registerFilesystemHandlers( kind: 'remote', cwd: args.worktreePath, execute: (plan, cwd, timeoutMs) => - provider.executeCommitMessagePlan(plan, cwd, timeoutMs), + provider.executeCommitMessagePlan(plan, cwd, timeoutMs, 'pull-request-fields'), missingBinaryLocation: 'remote PATH' }) } @@ -777,7 +777,7 @@ export function registerFilesystemHandlers( if (!provider) { return } - await provider.cancelGenerateCommitMessage(args.worktreePath) + await provider.cancelGenerateCommitMessage(args.worktreePath, 'pull-request-fields') return } const worktreePath = await resolveRegisteredWorktreePath(args.worktreePath, store) diff --git a/src/main/providers/ssh-git-provider-agent-exec.test.ts b/src/main/providers/ssh-git-provider-agent-exec.test.ts new file mode 100644 index 00000000000..c196ba4bda8 --- /dev/null +++ b/src/main/providers/ssh-git-provider-agent-exec.test.ts @@ -0,0 +1,130 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { SshGitProvider } from './ssh-git-provider' + +type MockMultiplexer = { + request: ReturnType +} + +describe('SshGitProvider agent exec lanes', () => { + let mux: MockMultiplexer + let provider: SshGitProvider + + beforeEach(() => { + mux = { request: vi.fn().mockResolvedValue(undefined) } + provider = new SshGitProvider('conn-1', mux as never) + }) + + it('reads branch, staged summary, and staged patch remotely', async () => { + mux.request.mockImplementation(async (method, payload) => { + expect(method).toBe('git.exec') + if (payload.args[1] === '--show-current') { + return { stdout: 'feature/ai-commit\n' } + } + if (payload.args[2] === '--name-status') { + return { stdout: 'M\tREADME.md\n' } + } + if (payload.args[2] === '--patch') { + return { stdout: 'diff --git a/README.md b/README.md\n+hello' } + } + throw new Error(`unexpected args: ${payload.args.join(' ')}`) + }) + + const result = await provider.getStagedCommitContext('/home/user/repo') + + expect(result).toEqual({ + branch: 'feature/ai-commit', + stagedSummary: 'M\tREADME.md', + stagedPatch: 'diff --git a/README.md b/README.md\n+hello' + }) + expect(mux.request).toHaveBeenCalledWith('git.exec', { + args: ['diff', '--cached', '--patch', '--minimal', '--no-color', '--no-ext-diff'], + cwd: '/home/user/repo' + }) + }) + + it('returns null when nothing is staged', async () => { + mux.request.mockImplementation(async (_method, payload) => { + if (payload.args[1] === '--show-current') { + return { stdout: 'main\n' } + } + return { stdout: '' } + }) + + await expect(provider.getStagedCommitContext('/home/user/repo')).resolves.toBeNull() + expect(mux.request).toHaveBeenCalledTimes(2) + }) + + it('delegates a commit-message plan to the relay', async () => { + const execResult = { + stdout: 'Update docs', + stderr: '', + exitCode: 0, + timedOut: false + } + mux.request.mockResolvedValue(execResult) + + const result = await provider.executeCommitMessagePlan( + { + binary: 'codex', + args: ['exec', 'PROMPT'], + stdinPayload: null, + label: 'Codex' + }, + '/home/user/repo', + 60_000 + ) + + expect(mux.request).toHaveBeenCalledWith( + 'agent.execNonInteractive', + expect.objectContaining({ cwd: '/home/user/repo', operation: 'commit-message' }) + ) + expect(result).toEqual(execResult) + }) + + it('sends best-effort commit-message cancellation', async () => { + await provider.cancelGenerateCommitMessage('/home/user/repo') + + expect(mux.request).toHaveBeenCalledWith( + 'agent.cancelExec', + expect.objectContaining({ cwd: '/home/user/repo', operation: 'commit-message' }) + ) + }) + + it('can execute a prepared plan in the pull-request-fields lane', async () => { + mux.request.mockResolvedValue({ + stdout: '{"title":"Update README"}', + stderr: '', + exitCode: 0, + timedOut: false + }) + + await provider.executeCommitMessagePlan( + { + binary: 'codex', + args: ['exec'], + stdinPayload: 'prompt', + label: 'Codex' + }, + '/home/user/repo', + 60_000, + 'pull-request-fields' + ) + + expect(mux.request).toHaveBeenCalledWith( + 'agent.execNonInteractive', + expect.objectContaining({ + cwd: '/home/user/repo', + operation: 'pull-request-fields' + }) + ) + }) + + it('can cancel the pull-request-fields lane', async () => { + await provider.cancelGenerateCommitMessage('/home/user/repo', 'pull-request-fields') + + expect(mux.request).toHaveBeenCalledWith('agent.cancelExec', { + cwd: '/home/user/repo', + operation: 'pull-request-fields' + }) + }) +}) diff --git a/src/main/providers/ssh-git-provider.test.ts b/src/main/providers/ssh-git-provider.test.ts index 0d517126438..74a3809b211 100644 --- a/src/main/providers/ssh-git-provider.test.ts +++ b/src/main/providers/ssh-git-provider.test.ts @@ -93,82 +93,6 @@ describe('SshGitProvider', () => { expect(result).toEqual(commitResult) }) - it('getStagedCommitContext reads branch, staged summary, and staged patch remotely', async () => { - mux.request.mockImplementation(async (method, payload) => { - expect(method).toBe('git.exec') - if (payload.args[1] === '--show-current') { - return { stdout: 'feature/ai-commit\n' } - } - if (payload.args[2] === '--name-status') { - return { stdout: 'M\tREADME.md\n' } - } - if (payload.args[2] === '--patch') { - return { stdout: 'diff --git a/README.md b/README.md\n+hello' } - } - throw new Error(`unexpected args: ${payload.args.join(' ')}`) - }) - - const result = await provider.getStagedCommitContext('/home/user/repo') - - expect(result).toEqual({ - branch: 'feature/ai-commit', - stagedSummary: 'M\tREADME.md', - stagedPatch: 'diff --git a/README.md b/README.md\n+hello' - }) - expect(mux.request).toHaveBeenCalledWith('git.exec', { - args: ['diff', '--cached', '--patch', '--minimal', '--no-color', '--no-ext-diff'], - cwd: '/home/user/repo' - }) - }) - - it('getStagedCommitContext returns null when nothing is staged', async () => { - mux.request.mockImplementation(async (_method, payload) => { - if (payload.args[1] === '--show-current') { - return { stdout: 'main\n' } - } - return { stdout: '' } - }) - - await expect(provider.getStagedCommitContext('/home/user/repo')).resolves.toBeNull() - expect(mux.request).toHaveBeenCalledTimes(2) - }) - - it('executeCommitMessagePlan delegates the prepared plan to the relay', async () => { - const execResult = { - stdout: 'Update docs', - stderr: '', - exitCode: 0, - timedOut: false - } - mux.request.mockResolvedValue(execResult) - - const result = await provider.executeCommitMessagePlan( - { - binary: 'codex', - args: ['exec', 'PROMPT'], - stdinPayload: null, - label: 'Codex' - }, - '/home/user/repo', - 60_000 - ) - - expect(mux.request).toHaveBeenCalledWith('agent.execNonInteractive', { - binary: 'codex', - args: ['exec', 'PROMPT'], - cwd: '/home/user/repo', - stdin: null, - timeoutMs: 60_000 - }) - expect(result).toEqual(execResult) - }) - - it('cancelGenerateCommitMessage sends best-effort relay cancellation', async () => { - await provider.cancelGenerateCommitMessage('/home/user/repo') - - expect(mux.request).toHaveBeenCalledWith('agent.cancelExec', { cwd: '/home/user/repo' }) - }) - it('getDiff sends git.diff request', async () => { const diffResult = { kind: 'text', originalContent: '', modifiedContent: 'hello' } mux.request.mockResolvedValue(diffResult) diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index 55f112482e3..c60fcdccde6 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -20,6 +20,8 @@ import type { CommitMessageDraftContext } from '../../shared/commit-message-gene import type { CommitMessagePlan } from '../../shared/commit-message-plan' import type { RemoteCommitMessageExecResult } from '../text-generation/commit-message-text-generation' +export type AgentExecOperation = 'commit-message' | 'pull-request-fields' + export class SshGitProvider implements IGitProvider { private connectionId: string private mux: SshChannelMultiplexer @@ -90,22 +92,27 @@ export class SshGitProvider implements IGitProvider { async executeCommitMessagePlan( plan: CommitMessagePlan, cwd: string, - timeoutMs: number + timeoutMs: number, + operation: AgentExecOperation = 'commit-message' ): Promise { return (await this.mux.request('agent.execNonInteractive', { binary: plan.binary, args: plan.args, cwd, stdin: plan.stdinPayload, - timeoutMs + timeoutMs, + operation })) as RemoteCommitMessageExecResult } - async cancelGenerateCommitMessage(worktreePath: string): Promise { + async cancelGenerateCommitMessage( + worktreePath: string, + operation: AgentExecOperation = 'commit-message' + ): Promise { // Why: best-effort — the relay returns `{canceled: false}` when there is // nothing in flight. Callers should not block UI updates on this. try { - await this.mux.request('agent.cancelExec', { cwd: worktreePath }) + await this.mux.request('agent.cancelExec', { cwd: worktreePath, operation }) } catch { // Swallow: cancellation is a fire-and-forget user intent. The pending // generateCommitMessage promise will still resolve with the kill result. diff --git a/src/main/runtime/orca-runtime-git.test.ts b/src/main/runtime/orca-runtime-git.test.ts index c05c67676fa..bf21093a2d5 100644 --- a/src/main/runtime/orca-runtime-git.test.ts +++ b/src/main/runtime/orca-runtime-git.test.ts @@ -4,13 +4,18 @@ import { join } from 'path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { GlobalSettings } from '../../shared/types' import type * as GitStatusModule from '../git/status' +import type * as SshGitDispatchModule from '../providers/ssh-git-dispatch' import type * as CommitMessageTextGenerationModule from '../text-generation/commit-message-text-generation' +import type * as PullRequestContextModule from '../text-generation/pull-request-context' import { RuntimeGitCommands, type ResolvedRuntimeGitWorktree } from './orca-runtime-git' const mocks = vi.hoisted(() => ({ getStagedCommitContext: vi.fn(), generateCommitMessageFromContext: vi.fn(), - resolveCommitMessageSettings: vi.fn() + generatePullRequestFieldsFromContext: vi.fn(), + resolveCommitMessageSettings: vi.fn(), + getPullRequestDraftContext: vi.fn(), + getSshGitProvider: vi.fn() })) vi.mock('../git/status', async () => ({ @@ -23,9 +28,22 @@ vi.mock('../text-generation/commit-message-text-generation', async () => ({ '../text-generation/commit-message-text-generation' )), generateCommitMessageFromContext: mocks.generateCommitMessageFromContext, + generatePullRequestFieldsFromContext: mocks.generatePullRequestFieldsFromContext, resolveCommitMessageSettings: mocks.resolveCommitMessageSettings })) +vi.mock('../text-generation/pull-request-context', async () => ({ + ...(await vi.importActual( + '../text-generation/pull-request-context' + )), + getPullRequestDraftContext: mocks.getPullRequestDraftContext +})) + +vi.mock('../providers/ssh-git-dispatch', async () => ({ + ...(await vi.importActual('../providers/ssh-git-dispatch')), + getSshGitProvider: mocks.getSshGitProvider +})) + const tempDirs: string[] = [] function makeWorktree(path: string): ResolvedRuntimeGitWorktree { @@ -54,7 +72,10 @@ describe('RuntimeGitCommands', () => { beforeEach(() => { mocks.getStagedCommitContext.mockReset() mocks.generateCommitMessageFromContext.mockReset() + mocks.generatePullRequestFieldsFromContext.mockReset() mocks.resolveCommitMessageSettings.mockReset() + mocks.getPullRequestDraftContext.mockReset() + mocks.getSshGitProvider.mockReset() }) afterEach(() => { @@ -119,4 +140,92 @@ describe('RuntimeGitCommands', () => { }) ) }) + + it('uses the pull-request-fields lane for remote PR generation', async () => { + const worktreePath = '/remote/repo' + const params = { agentId: 'custom', model: '', customAgentCommand: 'agent' } + const context = { + branch: 'feature/pr-fields', + base: 'main', + currentTitle: '', + currentBody: '', + currentDraft: false, + commitSummary: '- feat: add PR fields', + changeSummary: 'M\tREADME.md', + patch: '+hello' + } + const provider = { + exec: vi.fn(), + executeCommitMessagePlan: vi.fn().mockResolvedValue({ + stdout: '{"base":"main","title":"Update README","body":"","draft":false}', + stderr: '', + exitCode: 0, + timedOut: false + }) + } + mocks.resolveCommitMessageSettings.mockReturnValue({ ok: true, params }) + mocks.getSshGitProvider.mockReturnValue(provider) + mocks.getPullRequestDraftContext.mockResolvedValue(context) + mocks.generatePullRequestFieldsFromContext.mockResolvedValue({ + success: true, + fields: { base: 'main', title: 'Update README', body: '', draft: false } + }) + const commands = new RuntimeGitCommands({ + resolveRuntimeGitTarget: async () => ({ + worktree: makeWorktree(worktreePath), + connectionId: 'conn-1' + }), + getRuntimeSettings: () => ({}) as GlobalSettings + }) + + await expect( + commands.generateRuntimePullRequestFields('id:wt-1', { + base: 'main', + title: '', + body: '', + draft: false + }) + ).resolves.toMatchObject({ success: true }) + + const target = mocks.generatePullRequestFieldsFromContext.mock.calls[0]?.[2] as { + execute: ( + plan: { binary: string; args: string[]; stdinPayload: string | null; label: string }, + cwd: string, + timeoutMs: number + ) => Promise + } + await target.execute( + { binary: 'agent', args: [], stdinPayload: null, label: 'agent' }, + worktreePath, + 1 + ) + expect(provider.executeCommitMessagePlan).toHaveBeenCalledWith( + { binary: 'agent', args: [], stdinPayload: null, label: 'agent' }, + worktreePath, + 1, + 'pull-request-fields' + ) + }) + + it('uses the pull-request-fields lane for remote PR generation cancellation', async () => { + const worktreePath = '/remote/repo' + const provider = { cancelGenerateCommitMessage: vi.fn().mockResolvedValue(undefined) } + mocks.getSshGitProvider.mockReturnValue(provider) + const commands = new RuntimeGitCommands({ + resolveRuntimeGitTarget: async () => ({ + worktree: makeWorktree(worktreePath), + connectionId: 'conn-1' + }), + getRuntimeSettings: () => ({}) as GlobalSettings + }) + + await expect(commands.cancelRuntimeGeneratePullRequestFields('id:wt-1')).resolves.toEqual({ + ok: true + }) + + expect(provider.cancelGenerateCommitMessage).toHaveBeenCalledWith( + worktreePath, + 'pull-request-fields' + ) + }) }) diff --git a/src/main/runtime/orca-runtime-git.ts b/src/main/runtime/orca-runtime-git.ts index 46c1f30e9ca..adb0ff86049 100644 --- a/src/main/runtime/orca-runtime-git.ts +++ b/src/main/runtime/orca-runtime-git.ts @@ -429,7 +429,8 @@ export class RuntimeGitCommands { return generatePullRequestFieldsFromContext(context, resolvedSettings.params, { kind: 'remote', cwd: target.worktree.path, - execute: (plan, cwd, timeoutMs) => provider!.executeCommitMessagePlan(plan, cwd, timeoutMs), + execute: (plan, cwd, timeoutMs) => + provider!.executeCommitMessagePlan(plan, cwd, timeoutMs, 'pull-request-fields'), missingBinaryLocation: 'remote PATH' }) } @@ -452,7 +453,7 @@ export class RuntimeGitCommands { const target = await this.host.resolveRuntimeGitTarget(worktreeSelector) const provider = target.connectionId ? getSshGitProvider(target.connectionId) : null if (target.connectionId) { - await provider?.cancelGenerateCommitMessage(target.worktree.path) + await provider?.cancelGenerateCommitMessage(target.worktree.path, 'pull-request-fields') return { ok: true } } cancelGeneratePullRequestFieldsLocal(target.worktree.path) diff --git a/src/main/text-generation/pull-request-context.test.ts b/src/main/text-generation/pull-request-context.test.ts new file mode 100644 index 00000000000..561120fa8f4 --- /dev/null +++ b/src/main/text-generation/pull-request-context.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it, vi } from 'vitest' +import { getPullRequestDraftContext } from './pull-request-context' + +type ExecResult = { stdout: string; stderr?: string } +type ExecGit = Parameters[0] + +function createExecGit(responses: Record): ExecGit { + return vi.fn(async (args: string[]): Promise => { + const key = args.join('\0') + return { stdout: responses[key] ?? '' } + }) +} + +describe('getPullRequestDraftContext', () => { + it('collects branch-level context with argv-safe git commands', async () => { + const execGit = createExecGit({ + ['branch\0--show-current']: 'feature/pr-fields\n', + ['merge-base\0main\0HEAD']: 'abc123\n', + ['log\0--pretty=format:- %s\0--max-count=50\0abc123..HEAD']: '- feat: add PR fields\n', + ['diff\0--name-status\0abc123..HEAD']: 'M\tREADME.md\n', + ['diff\0--patch\0--minimal\0--no-color\0--no-ext-diff\0abc123..HEAD']: + 'diff --git a/README.md b/README.md\n+hello\n' + }) + + await expect( + getPullRequestDraftContext(execGit, { + base: 'main', + currentTitle: 'Draft title', + currentBody: 'Draft body', + currentDraft: false + }) + ).resolves.toEqual({ + branch: 'feature/pr-fields', + base: 'main', + currentTitle: 'Draft title', + currentBody: 'Draft body', + currentDraft: false, + commitSummary: '- feat: add PR fields', + changeSummary: 'M\tREADME.md', + patch: 'diff --git a/README.md b/README.md\n+hello' + }) + + expect(execGit).toHaveBeenCalledWith(['merge-base', 'main', 'HEAD'], { + maxBuffer: 10 * 1024 * 1024 + }) + expect(execGit).toHaveBeenCalledWith( + ['diff', '--patch', '--minimal', '--no-color', '--no-ext-diff', 'abc123..HEAD'], + { maxBuffer: 10 * 1024 * 1024 } + ) + }) + + it('rejects blank or option-like base refs before running git', async () => { + const execGit = createExecGit({}) + + await expect( + getPullRequestDraftContext(execGit, { + base: ' --help ', + currentTitle: '', + currentBody: '', + currentDraft: false + }) + ).resolves.toBeNull() + + expect(execGit).not.toHaveBeenCalled() + }) + + it('returns null when merge-base is unavailable', async () => { + const execGit = createExecGit({ + ['branch\0--show-current']: 'feature/pr-fields\n' + }) + + await expect( + getPullRequestDraftContext(execGit, { + base: 'main', + currentTitle: '', + currentBody: '', + currentDraft: false + }) + ).resolves.toBeNull() + }) +}) diff --git a/src/relay/agent-exec-handler-lanes.test.ts b/src/relay/agent-exec-handler-lanes.test.ts new file mode 100644 index 00000000000..de56b037690 --- /dev/null +++ b/src/relay/agent-exec-handler-lanes.test.ts @@ -0,0 +1,162 @@ +import { EventEmitter } from 'events' +import { exec, spawn } from 'child_process' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as ChildProcess from 'child_process' +import type { MethodHandler, RequestContext } from './dispatcher' +import { AgentExecHandler } from './agent-exec-handler' + +vi.mock('child_process', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + exec: vi.fn(), + spawn: vi.fn() + } +}) + +const spawnMock = vi.mocked(spawn) +const execMock = vi.mocked(exec) + +type FakeChild = EventEmitter & { + pid: number + kill: ReturnType + stdout: EventEmitter + stderr: EventEmitter + stdin: { end: ReturnType } +} + +function createFakeChild(): FakeChild { + return Object.assign(new EventEmitter(), { + pid: 12345, + kill: vi.fn(), + stdout: new EventEmitter(), + stderr: new EventEmitter(), + stdin: { end: vi.fn() } + }) +} + +function createHandlers(): Map { + const handlers = new Map() + new AgentExecHandler({ + onRequest: (method: string, handler: MethodHandler): void => { + handlers.set(method, handler) + } + } as never) + return handlers +} + +function requestContext(clientId = 1): RequestContext { + return { clientId, isStale: () => false } +} + +describe('AgentExecHandler operation lanes', () => { + beforeEach(() => { + spawnMock.mockReset() + execMock.mockReset() + }) + + it('cancels only the requested operation lane for a cwd', async () => { + const commitChild = createFakeChild() + const pullRequestChild = createFakeChild() + spawnMock + .mockReturnValueOnce(commitChild as never) + .mockReturnValueOnce(pullRequestChild as never) + const handlers = createHandlers() + + const commitPending = handlers.get('agent.execNonInteractive')!( + { + binary: 'agent', + args: [], + cwd: '/repo', + stdin: null, + timeoutMs: 5_000, + operation: 'commit-message' + }, + requestContext() + ) + const pullRequestPending = handlers.get('agent.execNonInteractive')!( + { + binary: 'agent', + args: [], + cwd: '/repo', + stdin: null, + timeoutMs: 5_000, + operation: 'pull-request-fields' + }, + requestContext() + ) + + await expect( + handlers.get('agent.cancelExec')!( + { cwd: '/repo', operation: 'pull-request-fields' }, + requestContext() + ) + ).resolves.toEqual({ canceled: true }) + + if (process.platform === 'win32') { + expect(execMock).toHaveBeenCalledTimes(1) + } else { + expect(commitChild.kill).not.toHaveBeenCalled() + expect(pullRequestChild.kill).toHaveBeenCalledWith('SIGKILL') + } + + pullRequestChild.emit('close', null) + commitChild.stdout.emit('data', Buffer.from('Commit message')) + commitChild.emit('close', 0) + + await expect(pullRequestPending).resolves.toMatchObject({ + exitCode: null, + canceled: true + }) + await expect(commitPending).resolves.toMatchObject({ + stdout: 'Commit message', + exitCode: 0, + canceled: false + }) + }) + + it('rejects duplicate execs in the same cwd and operation lane', async () => { + const child = createFakeChild() + spawnMock.mockReturnValue(child as never) + const handlers = createHandlers() + + const pending = handlers.get('agent.execNonInteractive')!( + { + binary: 'agent', + args: [], + cwd: '/repo', + stdin: null, + timeoutMs: 5_000, + operation: 'commit-message' + }, + requestContext() + ) + + await expect( + handlers.get('agent.execNonInteractive')!( + { + binary: 'agent', + args: [], + cwd: '/repo', + stdin: null, + timeoutMs: 5_000, + operation: 'commit-message' + }, + requestContext() + ) + ).resolves.toMatchObject({ + spawnError: 'Agent exec already running for this worktree and operation.' + }) + + expect(spawnMock).toHaveBeenCalledTimes(1) + await expect( + handlers.get('agent.cancelExec')!( + { cwd: '/repo', operation: 'commit-message' }, + requestContext() + ) + ).resolves.toEqual({ canceled: true }) + + child.emit('close', null) + await expect(pending).resolves.toMatchObject({ canceled: true }) + }) +}) diff --git a/src/relay/agent-exec-handler.ts b/src/relay/agent-exec-handler.ts index 38dc75601a4..b095301af55 100644 --- a/src/relay/agent-exec-handler.ts +++ b/src/relay/agent-exec-handler.ts @@ -94,10 +94,12 @@ type ExecParams = { stdin: unknown timeoutMs: unknown env: unknown + operation: unknown } type CancelParams = { cwd: unknown + operation: unknown } type ExecResult = { @@ -111,6 +113,11 @@ type ExecResult = { spawnError?: string } +function makeLaneKey(cwd: string, operation: unknown): string { + const lane = typeof operation === 'string' && operation.trim() ? operation.trim() : 'default' + return `${cwd}\0${lane}` +} + /** * Non-interactive subprocess exec on the remote host. Used by the AI commit * message generator to spawn agent CLIs (claude, codex, …) with the staged @@ -119,10 +126,10 @@ type ExecResult = { * and a clean exit code instead of an interactive session. */ export class AgentExecHandler { - // Why: a single in-flight exec per cwd is enough — the renderer prevents - // double-clicks. Keying by cwd lets `agent.cancelExec({cwd})` find the - // child without the client having to track relay-internal request ids. - private inFlightByCwd = new Map void }>() + // Why: commit-message and PR-field generation may run for the same remote + // worktree. Include an operation lane so stopping one agent run cannot + // kill the other. + private inFlightByLane = new Map void }>() constructor(dispatcher: RelayDispatcher) { dispatcher.onRequest('agent.execNonInteractive', (p) => this.exec(p as ExecParams)) @@ -131,7 +138,7 @@ export class AgentExecHandler { private async cancel(params: CancelParams): Promise<{ canceled: boolean }> { const cwd = typeof params.cwd === 'string' ? params.cwd : '' - const entry = this.inFlightByCwd.get(cwd) + const entry = this.inFlightByLane.get(makeLaneKey(cwd, params.operation)) if (!entry) { return { canceled: false } } @@ -147,6 +154,16 @@ export class AgentExecHandler { } const args = Array.isArray(params.args) ? params.args.map((a) => String(a)) : [] const cwd = typeof params.cwd === 'string' && params.cwd.length > 0 ? params.cwd : undefined + const laneKey = typeof cwd === 'string' ? makeLaneKey(cwd, params.operation) : '' + if (laneKey && this.inFlightByLane.has(laneKey)) { + return { + stdout: '', + stderr: '', + exitCode: null, + timedOut: false, + spawnError: 'Agent exec already running for this worktree and operation.' + } + } const stdinPayload = typeof params.stdin === 'string' ? params.stdin : null const requestedTimeout = typeof params.timeoutMs === 'number' ? params.timeoutMs : DEFAULT_TIMEOUT_MS @@ -185,19 +202,21 @@ export class AgentExecHandler { let timedOut = false let canceled = false let settled = false - const laneKey = typeof cwd === 'string' ? cwd : '' const finish = (result: ExecResult): void => { if (settled) { return } settled = true if (laneKey) { - this.inFlightByCwd.delete(laneKey) + const current = this.inFlightByLane.get(laneKey) + if (current?.child === child) { + this.inFlightByLane.delete(laneKey) + } } resolve(result) } if (laneKey) { - this.inFlightByCwd.set(laneKey, { + this.inFlightByLane.set(laneKey, { child, markCanceled: () => { canceled = true diff --git a/src/renderer/src/components/right-sidebar/CreatePullRequestDialog.tsx b/src/renderer/src/components/right-sidebar/CreatePullRequestDialog.tsx index fe7692ba29f..8e55d523688 100644 --- a/src/renderer/src/components/right-sidebar/CreatePullRequestDialog.tsx +++ b/src/renderer/src/components/right-sidebar/CreatePullRequestDialog.tsx @@ -1,5 +1,5 @@ import React, { useCallback, useEffect, useRef, useState } from 'react' -import { Check, ChevronsUpDown, Loader2, Sparkles, Square, RefreshCw } from 'lucide-react' +import { Check, ChevronsUpDown, Loader2 } from 'lucide-react' import { toast } from 'sonner' import { Button } from '@/components/ui/button' import { @@ -12,7 +12,6 @@ import { } from '@/components/ui/dialog' import { Input } from '@/components/ui/input' import { Label } from '@/components/ui/label' -import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' import { cn } from '@/lib/utils' import { useAppStore } from '@/store' import type { @@ -20,7 +19,9 @@ import type { HostedReviewCreationEligibility } from '../../../../shared/hosted-review' import { normalizeHostedReviewHeadRef } from '../../../../shared/hosted-review-refs' -import { stripBaseRef, useCreatePullRequestDialogFields } from './useCreatePullRequestDialogFields' +import { stripBaseRef } from './pull-request-field-generation' +import { useCreatePullRequestDialogFields } from './useCreatePullRequestDialogFields' +import { CreatePullRequestGenerateButton } from './CreatePullRequestGenerateButton' type CreatePullRequestDialogProps = { open: boolean @@ -207,40 +208,13 @@ export function CreatePullRequestDialog({ Create Pull Request {aiGenerationEnabled ? (
- {generating ? ( - - - - - - Generating PR details. Click to stop. - - - ) : ( - - )} + void handleGenerate()} + onCancelGenerate={handleCancelGenerate} + />
) : null} diff --git a/src/renderer/src/components/right-sidebar/CreatePullRequestGenerateButton.test.tsx b/src/renderer/src/components/right-sidebar/CreatePullRequestGenerateButton.test.tsx new file mode 100644 index 00000000000..51bd3b1146f --- /dev/null +++ b/src/renderer/src/components/right-sidebar/CreatePullRequestGenerateButton.test.tsx @@ -0,0 +1,105 @@ +import { describe, expect, it, vi } from 'vitest' +import { renderToStaticMarkup } from 'react-dom/server' +import type { MouseEvent, ReactElement } from 'react' +import { Button } from '@/components/ui/button' +import { TooltipProvider } from '@/components/ui/tooltip' +import { CreatePullRequestGenerateButton } from './CreatePullRequestGenerateButton' + +type ReactElementLike = { + type: unknown + props: Record +} + +function visit(node: unknown, cb: (node: ReactElementLike) => void): void { + if (node == null || typeof node === 'string' || typeof node === 'number') { + return + } + if (Array.isArray(node)) { + node.forEach((entry) => visit(entry, cb)) + return + } + const element = node as ReactElementLike + cb(element) + if (element.props?.children) { + visit(element.props.children, cb) + } +} + +function findButton(node: unknown): ReactElementLike { + let found: ReactElementLike | null = null + visit(node, (entry) => { + if (entry.type === Button) { + found = entry + } + }) + if (!found) { + throw new Error('button not found') + } + return found +} + +type MinimalMouseEvent = Pick + +function renderMarkup(element: ReactElement): string { + return renderToStaticMarkup({element}) +} + +describe('CreatePullRequestGenerateButton', () => { + it('calls generate when the enabled control is clicked', () => { + const onGenerate = vi.fn() + const element = CreatePullRequestGenerateButton({ + generating: false, + generateDisabled: false, + onGenerate, + onCancelGenerate: vi.fn() + }) + const button = findButton(element) + + ;(button.props.onClick as (event: MinimalMouseEvent) => void)({ + preventDefault: vi.fn() + }) + + expect(onGenerate).toHaveBeenCalledTimes(1) + expect(button.props.disabled).toBeUndefined() + }) + + it('keeps the disabled reason reachable without disabling focus', () => { + const onGenerate = vi.fn() + const preventDefault = vi.fn() + const reason = 'Pick an agent in Settings → Git → AI Commit Messages.' + const element = CreatePullRequestGenerateButton({ + generating: false, + generateDisabled: true, + generateDisabledReason: reason, + onGenerate, + onCancelGenerate: vi.fn() + }) + const button = findButton(element) + + ;(button.props.onClick as (event: MinimalMouseEvent) => void)({ preventDefault }) + + expect(onGenerate).not.toHaveBeenCalled() + expect(preventDefault).toHaveBeenCalledTimes(1) + expect(button.props.disabled).toBeUndefined() + expect(button.props['aria-disabled']).toBe(true) + expect(button.props['aria-describedby']).toBe('create-pr-generate-disabled-reason') + expect(renderMarkup(element)).toContain(reason) + }) + + it('shows and triggers the stop affordance while generating', () => { + const onCancelGenerate = vi.fn() + const element = CreatePullRequestGenerateButton({ + generating: true, + generateDisabled: false, + onGenerate: vi.fn(), + onCancelGenerate + }) + const button = findButton(element) + + ;(button.props.onClick as () => void)() + + expect(onCancelGenerate).toHaveBeenCalledTimes(1) + expect(button.props['aria-label']).toBe('Stop generating pull request details') + expect(renderMarkup(element)).toContain('Stop generating') + }) +}) diff --git a/src/renderer/src/components/right-sidebar/CreatePullRequestGenerateButton.tsx b/src/renderer/src/components/right-sidebar/CreatePullRequestGenerateButton.tsx new file mode 100644 index 00000000000..e40b58c3c3a --- /dev/null +++ b/src/renderer/src/components/right-sidebar/CreatePullRequestGenerateButton.tsx @@ -0,0 +1,84 @@ +import React from 'react' +import { RefreshCw, Sparkles, Square } from 'lucide-react' +import { Button } from '@/components/ui/button' +import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' + +type CreatePullRequestGenerateButtonProps = { + generating: boolean + generateDisabled: boolean + generateDisabledReason?: string + onGenerate: () => void + onCancelGenerate: () => void +} + +export function CreatePullRequestGenerateButton({ + generating, + generateDisabled, + generateDisabledReason, + onGenerate, + onCancelGenerate +}: CreatePullRequestGenerateButtonProps): React.JSX.Element { + if (generating) { + return ( + + + + + + Generating PR details. Click to stop. + + + ) + } + + return ( + + + + + + {generateDisabledReason ?? 'Generate pull request details with AI'} + + + ) +} diff --git a/src/renderer/src/components/right-sidebar/pull-request-field-generation.ts b/src/renderer/src/components/right-sidebar/pull-request-field-generation.ts new file mode 100644 index 00000000000..33dd25cf33f --- /dev/null +++ b/src/renderer/src/components/right-sidebar/pull-request-field-generation.ts @@ -0,0 +1,70 @@ +import { normalizeHostedReviewBaseRef } from '../../../../shared/hosted-review-refs' + +export type PullRequestFieldSnapshot = { + base: string + title: string + body: string + draft: boolean +} + +export type GenerationSeed = PullRequestFieldSnapshot & { + requestId: number +} + +export type GeneratedPullRequestFieldUpdate = + | { + ok: true + fields: PullRequestFieldSnapshot + } + | { + ok: false + error: string + } + +export function stripBaseRef(ref: string): string { + return normalizeHostedReviewBaseRef(ref) +} + +export function buildPullRequestGenerationInput({ + base, + title, + body, + draft +}: PullRequestFieldSnapshot): PullRequestFieldSnapshot { + return { + base: stripBaseRef(base.trim()), + title, + body, + draft + } +} + +export function resolveGeneratedPullRequestFieldUpdate( + seed: GenerationSeed | null, + latestFields: PullRequestFieldSnapshot, + requestId: number, + fields: PullRequestFieldSnapshot +): GeneratedPullRequestFieldUpdate { + if ( + !seed || + seed.requestId !== requestId || + seed.base !== latestFields.base || + seed.title !== latestFields.title || + seed.body !== latestFields.body || + seed.draft !== latestFields.draft + ) { + return { + ok: false, + error: 'Fields changed while generating. Run generate again for a fresh draft.' + } + } + return { + ok: true, + fields: { + base: stripBaseRef(fields.base), + title: fields.title, + body: fields.body, + draft: fields.draft + } + } +} diff --git a/src/renderer/src/components/right-sidebar/pull-request-generation-control.test.ts b/src/renderer/src/components/right-sidebar/pull-request-generation-control.test.ts new file mode 100644 index 00000000000..498c2b3e010 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/pull-request-generation-control.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it } from 'vitest' +import { resolvePullRequestGenerationControl } from './pull-request-generation-control' + +function baseInput( + overrides: Partial[0]> = {} +) { + return { + submitting: false, + aiEnabled: true, + agentId: 'codex', + customAgentCommand: '', + base: 'main', + generating: false, + ...overrides + } +} + +describe('resolvePullRequestGenerationControl', () => { + it('shows an enabled control when PR generation can run', () => { + expect(resolvePullRequestGenerationControl(baseInput())).toEqual({ + visible: true, + disabled: false, + disabledReason: undefined + }) + }) + + it('stays hidden when AI commit-message generation is disabled', () => { + expect(resolvePullRequestGenerationControl(baseInput({ aiEnabled: false }))).toMatchObject({ + visible: false, + disabled: true, + disabledReason: 'Enable AI commit messages in Settings → Git.' + }) + }) + + it('explains missing agent configuration', () => { + expect(resolvePullRequestGenerationControl(baseInput({ agentId: null }))).toMatchObject({ + disabled: true, + disabledReason: 'Pick an agent in Settings → Git → AI Commit Messages.' + }) + }) + + it('requires a custom command for custom agents', () => { + expect( + resolvePullRequestGenerationControl(baseInput({ agentId: 'custom', customAgentCommand: ' ' })) + ).toMatchObject({ + disabled: true, + disabledReason: 'Custom command is empty. Add one in Settings → Git → AI Commit Messages.' + }) + }) + + it('requires a base branch before generating', () => { + expect(resolvePullRequestGenerationControl(baseInput({ base: ' ' }))).toMatchObject({ + disabled: true, + disabledReason: 'Choose a base branch before generating.' + }) + }) + + it('keeps the control clickable while generating so users can cancel', () => { + expect( + resolvePullRequestGenerationControl(baseInput({ submitting: true, generating: true })) + ).toMatchObject({ + visible: true, + disabled: false, + disabledReason: 'Create PR in progress…' + }) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/pull-request-generation-control.ts b/src/renderer/src/components/right-sidebar/pull-request-generation-control.ts new file mode 100644 index 00000000000..5a7d2970460 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/pull-request-generation-control.ts @@ -0,0 +1,46 @@ +import { isCustomAgentId } from '../../../../shared/commit-message-agent-spec' + +type PullRequestGenerationControlInput = { + submitting: boolean + aiEnabled: boolean + agentId: string | null + customAgentCommand: string + base: string + generating: boolean +} + +export type PullRequestGenerationControlState = { + visible: boolean + disabled: boolean + disabledReason?: string +} + +export function resolvePullRequestGenerationControl({ + submitting, + aiEnabled, + agentId, + customAgentCommand, + base, + generating +}: PullRequestGenerationControlInput): PullRequestGenerationControlState { + let disabledReason: string | undefined + if (submitting) { + disabledReason = 'Create PR in progress…' + } else if (!aiEnabled) { + disabledReason = 'Enable AI commit messages in Settings → Git.' + } else if (!agentId) { + disabledReason = 'Pick an agent in Settings → Git → AI Commit Messages.' + } else if (isCustomAgentId(agentId) && !customAgentCommand.trim()) { + disabledReason = 'Custom command is empty. Add one in Settings → Git → AI Commit Messages.' + } else if (!base.trim()) { + disabledReason = 'Choose a base branch before generating.' + } + + return { + visible: aiEnabled, + // Why: the generating state swaps this button into a cancel affordance; + // keeping it enabled is what lets users stop a long agent run. + disabled: !generating && Boolean(disabledReason), + disabledReason + } +} diff --git a/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.test.ts b/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.test.ts new file mode 100644 index 00000000000..a785d69b52e --- /dev/null +++ b/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.test.ts @@ -0,0 +1,77 @@ +import { describe, expect, it } from 'vitest' +import { + buildPullRequestGenerationInput, + resolveGeneratedPullRequestFieldUpdate +} from './pull-request-field-generation' + +describe('buildPullRequestGenerationInput', () => { + it('normalizes the base ref sent to generation', () => { + expect( + buildPullRequestGenerationInput({ + base: ' refs/heads/main ', + title: 'Current title', + body: 'Current body', + draft: false + }) + ).toEqual({ + base: 'main', + title: 'Current title', + body: 'Current body', + draft: false + }) + }) +}) + +describe('resolveGeneratedPullRequestFieldUpdate', () => { + const seed = { + requestId: 1, + base: 'main', + title: '', + body: '', + draft: false + } + + it('normalizes and applies generated fields when the seed still matches', () => { + expect( + resolveGeneratedPullRequestFieldUpdate(seed, seed, 1, { + base: 'refs/heads/main', + title: 'Add PR generation', + body: 'Summary', + draft: true + }) + ).toEqual({ + ok: true, + fields: { + base: 'main', + title: 'Add PR generation', + body: 'Summary', + draft: true + } + }) + }) + + it('rejects generated fields when the user edited a field while generation ran', () => { + expect( + resolveGeneratedPullRequestFieldUpdate(seed, { ...seed, title: 'Manual title' }, 1, { + base: 'main', + title: 'Generated title', + body: 'Generated body', + draft: false + }) + ).toEqual({ + ok: false, + error: 'Fields changed while generating. Run generate again for a fresh draft.' + }) + }) + + it('rejects stale generation responses', () => { + expect( + resolveGeneratedPullRequestFieldUpdate(seed, seed, 2, { + base: 'main', + title: 'Generated title', + body: 'Generated body', + draft: false + }) + ).toMatchObject({ ok: false }) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.ts b/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.ts index 37bee7d330f..16842e34c5e 100644 --- a/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.ts +++ b/src/renderer/src/components/right-sidebar/useCreatePullRequestDialogFields.ts @@ -9,12 +9,15 @@ import { getRuntimeRepoBaseRefDefault, searchRuntimeRepoBaseRefs } from '@/runtime/runtime-repo-client' -import { - isCustomAgentId, - resolveCommitMessageAgentChoice -} from '../../../../shared/commit-message-agent-spec' +import { resolveCommitMessageAgentChoice } from '../../../../shared/commit-message-agent-spec' import type { HostedReviewCreationEligibility } from '../../../../shared/hosted-review' -import { normalizeHostedReviewBaseRef } from '../../../../shared/hosted-review-refs' +import { resolvePullRequestGenerationControl } from './pull-request-generation-control' +import { + buildPullRequestGenerationInput, + resolveGeneratedPullRequestFieldUpdate, + stripBaseRef, + type GenerationSeed +} from './pull-request-field-generation' type UseCreatePullRequestDialogFieldsOptions = { open: boolean @@ -27,18 +30,6 @@ type UseCreatePullRequestDialogFieldsOptions = { submitting: boolean } -type GenerationSeed = { - requestId: number - base: string - title: string - body: string - draft: boolean -} - -export function stripBaseRef(ref: string): string { - return normalizeHostedReviewBaseRef(ref) -} - export function useCreatePullRequestDialogFields({ open, repoId, @@ -163,26 +154,22 @@ export function useCreatePullRequestDialogFields({ } }, [baseQuery, open, repoId, settings]) - let generateDisabledReason: string | undefined - if (submitting) { - generateDisabledReason = 'Create PR in progress...' - } else if (!commitMessageAi?.enabled) { - generateDisabledReason = 'Enable AI commit messages in Settings -> Git.' - } else if (!effectiveCommitMessageAgentId) { - generateDisabledReason = 'Pick an agent in Settings -> Git -> AI Commit Messages.' - } else if (isCustomAgentId(effectiveCommitMessageAgentId)) { - const command = commitMessageAi.customAgentCommand?.trim() ?? '' - if (!command) { - generateDisabledReason = - 'Custom command is empty. Add one in Settings -> Git -> AI Commit Messages.' - } - } else if (!base.trim()) { - generateDisabledReason = 'Choose a base branch before generating.' - } - const generateDisabled = !generating && Boolean(generateDisabledReason) + const generationControl = resolvePullRequestGenerationControl({ + submitting, + aiEnabled: commitMessageAi?.enabled === true, + agentId: effectiveCommitMessageAgentId, + customAgentCommand: commitMessageAi?.customAgentCommand ?? '', + base, + generating + }) const handleGenerate = useCallback(async (): Promise => { - if (!worktreePath || !base.trim() || generateInFlightRef.current || generateDisabled) { + if ( + !worktreePath || + !base.trim() || + generateInFlightRef.current || + generationControl.disabled + ) { return } const requestId = generationRequestIdRef.current + 1 @@ -201,12 +188,7 @@ export function useCreatePullRequestDialogFields({ worktreePath, connectionId }, - { - base: stripBaseRef(base.trim()), - title, - body, - draft - } + buildPullRequestGenerationInput({ base, title, body, draft }) ) if (generationRequestIdRef.current !== requestId) { return @@ -220,25 +202,22 @@ export function useCreatePullRequestDialogFields({ return } - const currentSeed = generationSeedRef.current - const latestFields = latestFieldsRef.current - if ( - !currentSeed || - currentSeed.requestId !== requestId || - currentSeed.base !== latestFields.base || - currentSeed.title !== latestFields.title || - currentSeed.body !== latestFields.body || - currentSeed.draft !== latestFields.draft - ) { - setGenerateError('Fields changed while generating. Run generate again for a fresh draft.') + const update = resolveGeneratedPullRequestFieldUpdate( + generationSeedRef.current, + latestFieldsRef.current, + requestId, + result.fields + ) + if (!update.ok) { + setGenerateError(update.error) return } - setBase(stripBaseRef(result.fields.base)) + setBase(update.fields.base) setBaseQuery('') setBaseResults([]) - setTitle(result.fields.title) - setBody(result.fields.body) - setDraft(result.fields.draft) + setTitle(update.fields.title) + setBody(update.fields.body) + setDraft(update.fields.draft) setGenerateError(null) } catch (error) { if (generationRequestIdRef.current !== requestId) { @@ -254,7 +233,7 @@ export function useCreatePullRequestDialogFields({ setGenerating(false) } } - }, [base, body, draft, generateDisabled, title, worktreeId, worktreePath]) + }, [base, body, draft, generationControl.disabled, title, worktreeId, worktreePath]) const handleCancelGenerate = useCallback((): void => { if (!worktreePath || !generateInFlightRef.current) { @@ -275,7 +254,7 @@ export function useCreatePullRequestDialogFields({ }, [worktreeId, worktreePath]) return { - aiGenerationEnabled: commitMessageAi?.enabled === true, + aiGenerationEnabled: generationControl.visible, base, setBase, title, @@ -291,8 +270,8 @@ export function useCreatePullRequestDialogFields({ baseSearchError, generating, generateError, - generateDisabled, - generateDisabledReason, + generateDisabled: generationControl.disabled, + generateDisabledReason: generationControl.disabledReason, handleGenerate, handleCancelGenerate } diff --git a/src/shared/pull-request-generation.test.ts b/src/shared/pull-request-generation.test.ts index 934be13767e..7a403feb742 100644 --- a/src/shared/pull-request-generation.test.ts +++ b/src/shared/pull-request-generation.test.ts @@ -53,4 +53,27 @@ describe('parseGeneratedPullRequestFields', () => { draft: false }) }) + + it('parses JSON with agent preamble noise', () => { + const fields = parseGeneratedPullRequestFields( + 'Generating...\n{"base":"origin/main","title":"Add PR generation","body":"Summary\\n","draft":false}', + context + ) + + expect(fields).toEqual({ + base: 'origin/main', + title: 'Add PR generation', + body: 'Summary', + draft: false + }) + }) + + it('uses a generic title when neither generated nor current title exists', () => { + const fields = parseGeneratedPullRequestFields('{"title":""}', { + ...context, + currentTitle: '' + }) + + expect(fields.title).toBe('Update project files') + }) })