diff --git a/src/main/ipc/filesystem.test.ts b/src/main/ipc/filesystem.test.ts index cab2d3750e9..a4e39147057 100644 --- a/src/main/ipc/filesystem.test.ts +++ b/src/main/ipc/filesystem.test.ts @@ -42,6 +42,8 @@ const { discoverCommitMessageModelsRemoteMock, cancelGenerateCommitMessageLocalMock, cancelGeneratePullRequestFieldsLocalMock, + getPullRequestDraftContextMock, + resolveHostedReviewBodyForGenerationMock, getSshFilesystemProviderMock, getSshGitProviderMock, tryDeleteWslUncPathMock, @@ -85,6 +87,8 @@ const { discoverCommitMessageModelsRemoteMock: vi.fn(), cancelGenerateCommitMessageLocalMock: vi.fn(), cancelGeneratePullRequestFieldsLocalMock: vi.fn(), + getPullRequestDraftContextMock: vi.fn(), + resolveHostedReviewBodyForGenerationMock: vi.fn(), getSshFilesystemProviderMock: vi.fn(), getSshGitProviderMock: vi.fn(), tryDeleteWslUncPathMock: vi.fn(), @@ -189,6 +193,16 @@ vi.mock('../text-generation/commit-message-text-generation', () => ({ cancelGeneratePullRequestFieldsLocal: cancelGeneratePullRequestFieldsLocalMock })) +vi.mock('../text-generation/pull-request-context', () => ({ + getPullRequestDraftContext: getPullRequestDraftContextMock +})) + +vi.mock('../source-control/pull-request-template', () => ({ + readHostedPullRequestTemplate: vi.fn(), + readHostedReviewTemplate: vi.fn(), + resolveHostedReviewBodyForGeneration: resolveHostedReviewBodyForGenerationMock +})) + import { registerFilesystemHandlers } from './filesystem' import { invalidateAuthorizedRootsCache, registerWorktreeRootsForRepo } from './filesystem-auth' @@ -289,6 +303,8 @@ describe('registerFilesystemHandlers', () => { resolveCommitMessageSettingsMock, generateCommitMessageFromContextMock, generatePullRequestFieldsFromContextMock, + getPullRequestDraftContextMock, + resolveHostedReviewBodyForGenerationMock, discoverCommitMessageModelsLocalMock, discoverCommitMessageModelsRemoteMock, cancelGenerateCommitMessageLocalMock, @@ -2086,7 +2102,9 @@ describe('registerFilesystemHandlers', () => { ) }) - it('ignores a worktree id that does not own the requested worktree path', async () => { + // Why: the renderer derives worktreePath from worktreeId, so a mismatched pair + // models an independent caller (relay/CLI/future), not a stale renderer context. + it('ignores an independently supplied id that does not own the requested worktree path', async () => { const context = { branch: 'feature/ai', stagedSummary: 'M\tREADME.md', @@ -2141,6 +2159,95 @@ describe('registerFilesystemHandlers', () => { ) }) + describe('git:generatePullRequestFields linked issue', () => { + const PULL_REQUEST_CONTEXT = { + base: 'main', + branch: 'feature/ai', + branchChangedByPreparation: false, + commitSummary: 'a1b2c3d Add generation', + changeSummary: 'README.md | 2 +-', + patch: '+hello', + currentTitle: '', + currentBody: '', + currentDraft: false + } + const PULL_REQUEST_ARGS = { base: 'main', title: '', body: '', draft: false } + const params = { agentId: 'codex', model: 'gpt-5.4-mini' } + + beforeEach(() => { + resolveCommitMessageSettingsMock.mockReturnValue({ ok: true, params }) + resolveHostedReviewBodyForGenerationMock.mockResolvedValue('') + getPullRequestDraftContextMock.mockResolvedValue(PULL_REQUEST_CONTEXT) + generatePullRequestFieldsFromContextMock.mockResolvedValue({ success: true, fields: {} }) + }) + + it('enriches the local pull-request context with a validated worktree linked issue', async () => { + const worktreeId = `repo-1::${WORKTREE_FEATURE_PATH}` + const linkedStore = { + ...store, + getWorktreeMeta: (id: string) => (id === worktreeId ? { linkedIssue: 123 } : undefined) + } + + registerFilesystemHandlers(linkedStore as never) + + await handlers.get('git:generatePullRequestFields')!(null, { + ...PULL_REQUEST_ARGS, + worktreePath: WORKTREE_FEATURE_PATH, + worktreeId + }) + + expect(generatePullRequestFieldsFromContextMock).toHaveBeenCalledWith( + { ...PULL_REQUEST_CONTEXT, linkedIssue: 123 }, + params, + expect.objectContaining({ kind: 'local' }) + ) + }) + + it('enriches the SSH pull-request context from host meta using the remote path', async () => { + const worktreeId = 'repo-1::/remote/repo' + getSshGitProviderMock.mockReturnValue({ + exec: vi.fn(), + executeCommitMessagePlan: vi.fn() + }) + const linkedStore = { + ...store, + getWorktreeMeta: (id: string) => (id === worktreeId ? { linkedIssue: 77 } : undefined) + } + + registerFilesystemHandlers(linkedStore as never) + + await handlers.get('git:generatePullRequestFields')!(null, { + ...PULL_REQUEST_ARGS, + worktreePath: '/remote/repo', + worktreeId, + connectionId: 'conn-1' + }) + + expect(generatePullRequestFieldsFromContextMock).toHaveBeenCalledWith( + { ...PULL_REQUEST_CONTEXT, linkedIssue: 77 }, + params, + expect.objectContaining({ kind: 'remote' }) + ) + }) + + it('ignores a pull-request worktree id that does not own the requested path', async () => { + const getWorktreeMeta = vi.fn(() => ({ linkedIssue: 123 })) + + registerFilesystemHandlers({ ...store, getWorktreeMeta } as never) + + await handlers.get('git:generatePullRequestFields')!(null, { + ...PULL_REQUEST_ARGS, + worktreePath: WORKTREE_FEATURE_PATH, + worktreeId: `repo-1::${path.resolve('/workspace/repo-other')}` + }) + + expect(getWorktreeMeta).not.toHaveBeenCalled() + expect(generatePullRequestFieldsFromContextMock.mock.calls[0]?.[0]).not.toHaveProperty( + 'linkedIssue' + ) + }) + }) + it('returns a sanitized error when local agent account preparation fails', async () => { const context = { branch: 'feature/ai', diff --git a/src/main/ipc/filesystem.ts b/src/main/ipc/filesystem.ts index db5dd4ec5fd..726d5b0b89d 100644 --- a/src/main/ipc/filesystem.ts +++ b/src/main/ipc/filesystem.ts @@ -87,6 +87,7 @@ import { assertGitPushTargetShape } from '../../shared/git-push-target-validatio import { getCommitMessageModelDiscoveryHostKey } from '../../shared/commit-message-host-key' import type { HostedReviewProvider } from '../../shared/hosted-review' import type { ResolvedSourceControlAiGenerationParams } from '../../shared/source-control-ai' +import { withLinkedIssueDraftContext } from '../../shared/source-control-ai-action-variables' import { validateGitPushTarget } from '../git/push-target-validation' import { getRemoteCommitUrl, getRemoteFileUrl } from '../git/repo' import { @@ -101,7 +102,6 @@ import { registerFilesystemMutationHandlers } from './filesystem-mutations' import { searchWithGitGrep } from './filesystem-search-git' import { getLocalGitOptionsForRegisteredWorktree } from './local-worktree-runtime-options' import { resolveSourceControlAiLinkedIssue } from './source-control-ai-linked-issue' -import { withLinkedIssueDraftContext } from '../../shared/source-control-ai-actions' import { listMarkdownDocuments, markdownDocumentsFromRelativePaths } from './markdown-documents' import { checkRgAvailable } from './rg-availability' import { diff --git a/src/main/ipc/source-control-ai-linked-issue.test.ts b/src/main/ipc/source-control-ai-linked-issue.test.ts index e3e32d61cd4..1a5291fc6d7 100644 --- a/src/main/ipc/source-control-ai-linked-issue.test.ts +++ b/src/main/ipc/source-control-ai-linked-issue.test.ts @@ -41,7 +41,11 @@ describe('resolveSourceControlAiLinkedIssue', () => { expect(store.getWorktreeMeta).toHaveBeenCalledWith(instanceId) }) - it('ignores an id whose path does not match the request', () => { + // Why: the desktop renderer derives `worktreePath` from `worktreeId`, so it can + // never send a mismatched pair — these cases model an independent caller (relay, + // CLI, future in-process caller) and assert the guard fails closed for them. + // They are not evidence that a stale renderer context is rejected; it is not. + it('rejects an independently supplied id whose path does not match the request', () => { const store = makeStore({ [LOCAL_ID]: { linkedIssue: 123 } }) expect( @@ -65,7 +69,7 @@ describe('resolveSourceControlAiLinkedIssue', () => { ).toBe(5) }) - it('ignores an id whose repoId contradicts the request repoId', () => { + it('rejects an independently supplied id whose repoId contradicts the request repoId', () => { const store = makeStore({ [LOCAL_ID]: { linkedIssue: 123 } }) expect( @@ -78,6 +82,19 @@ describe('resolveSourceControlAiLinkedIssue', () => { expect(store.getWorktreeMeta).not.toHaveBeenCalled() }) + it('fails closed on an empty-string repoId instead of skipping the cross-check', () => { + const store = makeStore({ [LOCAL_ID]: { linkedIssue: 123 } }) + + expect( + resolveSourceControlAiLinkedIssue(store, { + worktreeId: LOCAL_ID, + worktreePath: LOCAL_PATH, + repoId: '' + }) + ).toBeNull() + expect(store.getWorktreeMeta).not.toHaveBeenCalled() + }) + it('compares SSH remote paths as raw strings', () => { const store = makeStore({ [REMOTE_ID]: { linkedIssue: 77 } }) @@ -124,14 +141,14 @@ describe('resolveSourceControlAiLinkedIssue', () => { ).toBeNull() }) - it('returns null for unparsable ids and unlinked or non-numeric meta', () => { + it('returns null for unparsable ids and unlinked or unusable meta', () => { expect( resolveSourceControlAiLinkedIssue(makeStore({}), { worktreeId: 'no-separator', worktreePath: LOCAL_PATH }) ).toBeNull() - for (const linkedIssue of [null, undefined, Number.NaN]) { + for (const linkedIssue of [null, undefined, Number.NaN, 0, -7, 12.9]) { expect( resolveSourceControlAiLinkedIssue(makeStore({ [LOCAL_ID]: { linkedIssue } }), { worktreeId: LOCAL_ID, diff --git a/src/main/ipc/source-control-ai-linked-issue.ts b/src/main/ipc/source-control-ai-linked-issue.ts index cd3a3e2f639..559d39c7399 100644 --- a/src/main/ipc/source-control-ai-linked-issue.ts +++ b/src/main/ipc/source-control-ai-linked-issue.ts @@ -1,5 +1,6 @@ import { resolve } from 'node:path' import type { Store } from '../persistence' +import { isLinkedIssueNumber } from '../../shared/source-control-ai-action-variables' import { splitWorktreeIdForFilesystem } from '../../shared/worktree-id' export type LinkedIssueLookupArgs = { @@ -39,8 +40,17 @@ function matchesRequestPath( * * The renderer-supplied `worktreeId` is advisory — the same trust model as * `getRepoForSourceControlAi` — so it is validated against the request's path - * (and `repoId`) before the meta read. A stale id after a workspace switch would - * otherwise pull another workspace's issue number into this commit message. + * (and `repoId`) before the meta read. + * + * Scope of that guarantee: today's desktop renderer derives `worktreePath` from + * `worktreeId` (`resolveLocalWorktreePath`), so its two operands always agree and + * this check cannot reject a renderer call — including a stale id after a + * workspace switch, which produces a matching stale *pair* (git then runs in that + * same stale worktree, so the number still belongs to the tree being committed). + * The validation exists for callers that supply id and path independently — a + * relay, the CLI, or a future in-process caller — where a mismatched pair really + * would read another workspace's meta. Keep it: it is cheap and fails closed. + * * Meta is keyed by the raw id: the `::workspace:` suffix of folder-repo * workspace instances is part of the key, while validation uses the stripped path. */ @@ -59,7 +69,9 @@ export function resolveSourceControlAiLinkedIssue( if (!parsed) { return null } - if (args.repoId && parsed.repoId !== args.repoId) { + // Why: `typeof` rather than truthiness, so an empty-string repoId fails closed + // instead of silently disabling the cross-check. + if (typeof args.repoId === 'string' && parsed.repoId !== args.repoId) { return null } if (!matchesRequestPath(parsed.worktreePath, args, resolvedWorktreePath)) { @@ -67,5 +79,5 @@ export function resolveSourceControlAiLinkedIssue( } const linkedIssue = store.getWorktreeMeta(args.worktreeId)?.linkedIssue // Why: GitHub only in v1 — no `linkedGitLabIssue` dual-read. - return typeof linkedIssue === 'number' && Number.isFinite(linkedIssue) ? linkedIssue : null + return isLinkedIssueNumber(linkedIssue) ? linkedIssue : null } diff --git a/src/main/runtime/orca-runtime-git.test.ts b/src/main/runtime/orca-runtime-git.test.ts index 223cb490d65..469e53d29fc 100644 --- a/src/main/runtime/orca-runtime-git.test.ts +++ b/src/main/runtime/orca-runtime-git.test.ts @@ -61,7 +61,11 @@ vi.mock('../source-control/pull-request-template', () => ({ const tempDirs: string[] = [] function makeWorktree(path: string, linkedIssue: number | null = null): ResolvedRuntimeGitWorktree { - return { + // Why: `satisfies Partial<…>` keeps every field name and type checked against the + // real worktree shape (the widening cast alone would let these tests keep passing + // against a `linkedIssue` key production no longer has) while still allowing the + // fixture to omit the fields these tests never read. + const worktree = { id: 'wt-1', repoId: 'repo-1', path, @@ -69,11 +73,12 @@ function makeWorktree(path: string, linkedIssue: number | null = null): Resolved git: { path, branch: 'main', - bare: false, - detached: false, + isBare: false, + isMainWorktree: false, head: 'a'.repeat(40) } - } as unknown as ResolvedRuntimeGitWorktree + } satisfies Partial + return worktree as unknown as ResolvedRuntimeGitWorktree } function makeCommands(worktreePath: string): RuntimeGitCommands { @@ -701,4 +706,33 @@ describe('RuntimeGitCommands', () => { expect(call[0]).toEqual({ ...context, linkedIssue: 55 }) } }) + + it('leaves the pull-request context untouched when no issue is linked', async () => { + const worktreePath = mkdtempSync(join(tmpdir(), 'orca-runtime-git-')) + tempDirs.push(worktreePath) + const context = { + base: 'main', + branch: 'feature/login', + commitSummary: 'abc123 feat: test', + changeSummary: 'M README.md', + patch: '+hello', + currentTitle: '', + currentBody: '', + currentDraft: false + } + const params = { agentId: 'codex' as const, model: 'gpt-5.5' } + mocks.getPullRequestDraftContext.mockResolvedValue(context) + mocks.generatePullRequestFieldsFromContext.mockResolvedValue({ success: true, fields: {} }) + + await makeCommands(worktreePath).generateRuntimePullRequestFields( + 'id:wt-1', + { base: 'main', title: '', body: '', draft: false }, + { sourceControlAiResolvedParams: params } + ) + + expect(mocks.generatePullRequestFieldsFromContext.mock.calls).toHaveLength(1) + expect(mocks.generatePullRequestFieldsFromContext.mock.calls[0][0]).not.toHaveProperty( + 'linkedIssue' + ) + }) }) diff --git a/src/main/runtime/orca-runtime-git.ts b/src/main/runtime/orca-runtime-git.ts index a5c160a2cc9..ae8c09eb804 100644 --- a/src/main/runtime/orca-runtime-git.ts +++ b/src/main/runtime/orca-runtime-git.ts @@ -23,8 +23,8 @@ import { mergeLegacyCommitMessageAiIntoSourceControlAi, type ResolvedSourceControlAiGenerationParams } from '../../shared/source-control-ai' +import { withLinkedIssueDraftContext } from '../../shared/source-control-ai-action-variables' import type { SourceControlAiOperation } from '../../shared/source-control-ai-types' -import { withLinkedIssueDraftContext } from '../../shared/source-control-ai-actions' import type { GitProviderStatusOptions } from '../providers/types' import { getRemoteCommitUrl, getRemoteFileUrl } from '../git/repo' import { diff --git a/src/main/text-generation/commit-message-text-generation.test.ts b/src/main/text-generation/commit-message-text-generation.test.ts index fec7b17f3e1..13feda4006c 100644 --- a/src/main/text-generation/commit-message-text-generation.test.ts +++ b/src/main/text-generation/commit-message-text-generation.test.ts @@ -1970,17 +1970,41 @@ describe('linkedIssue template substitution', () => { } }) + // Why: a fixture-unique sentinel — a short number like 42 also appears in the + // character counts that truncateDiffForPrompt/limitSection emit, so growing any + // fixture past its limit would fail these guards for reasons unrelated to leakage. + const BUILT_IN_PROMPT_SENTINEL_ISSUE = 987654 + const builtInPromptParams = { + agentId: 'custom' as const, + model: '', + customAgentCommand: 'agent' + } + it('leaves the built-in commit prompt free of issue guidance', async () => { let prompt = '' await generateCommitMessageFromContext( - { ...COMMIT_CONTEXT, linkedIssue: 42 }, - { agentId: 'custom', model: '', customAgentCommand: 'agent' }, + { ...COMMIT_CONTEXT, linkedIssue: BUILT_IN_PROMPT_SENTINEL_ISSUE }, + builtInPromptParams, capturingTarget((value) => { prompt = value }) ) - expect(prompt).not.toContain('42') + expect(prompt).not.toContain(String(BUILT_IN_PROMPT_SENTINEL_ISSUE)) + expect(prompt).not.toContain('linkedIssue') + }) + + it('leaves the built-in pull-request prompt free of issue guidance', async () => { + let prompt = '' + await generatePullRequestFieldsFromContext( + { ...PULL_REQUEST_CONTEXT, linkedIssue: BUILT_IN_PROMPT_SENTINEL_ISSUE }, + builtInPromptParams, + capturingTarget((value) => { + prompt = value + }) + ) + + expect(prompt).not.toContain(String(BUILT_IN_PROMPT_SENTINEL_ISSUE)) expect(prompt).not.toContain('linkedIssue') }) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 6fea1c659cc..9887b10dac7 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -6323,6 +6323,7 @@ function SourceControlInner(): React.JSX.Element { settings={settings} repo={activeRepo ?? null} discoveryHostKey={sourceControlAiDiscoveryHostKey} + linkedIssue={activeWorktree?.linkedIssue ?? null} onGenerate={(params) => { void handleGenerate({ sourceControlAiResolvedParams: params }) }} @@ -6344,6 +6345,7 @@ function SourceControlInner(): React.JSX.Element { settings={settings} repo={activeRepo ?? null} discoveryHostKey={sourceControlAiDiscoveryHostKey} + linkedIssue={activeWorktree?.linkedIssue ?? null} onGenerate={(params) => { void handleGeneratePullRequestFields({ sourceControlAiResolvedParams: params }) }} diff --git a/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialog.test.ts b/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialog.test.ts index f17eaea0845..92d7fbb1995 100644 --- a/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialog.test.ts +++ b/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialog.test.ts @@ -38,6 +38,11 @@ vi.mock('@/components/ui/select', () => ({ SelectValue: () => React.createElement('span') })) +/** The mocked chips serialize their previews into an attribute, so quotes arrive escaped. */ +function escapeHtml(value: string): string { + return value.replace(/"/g, '"') +} + describe('buildCommitMessageGenerationParams', () => { it('defaults saved text-generation recipes to the global target when repo and global are available', () => { expect( @@ -79,6 +84,57 @@ describe('buildCommitMessageGenerationParams', () => { expect(markup).toContain('You are generating a single git commit message.') }) + // Why: the chips fall back to the synthetic `123` sample when no preview is supplied, + // which told an unlinked workspace it would render `Fixes #123` and then produced + // `Fixes #`. Workspace-scoped callers must preview their own value, empty included. + it.each([ + { label: 'the linked issue', linkedIssue: 42, expected: '42' }, + { label: 'an empty value when unlinked', linkedIssue: null, expected: '' } + ])('previews $label for a workspace-scoped dialog', ({ linkedIssue, expected }) => { + const markup = renderToStaticMarkup( + React.createElement(SourceControlTextGenerationDialogForm, { + actionId: 'commitMessage', + generateLabel: 'Generate', + settings: null, + repo: null, + baseParams: { + agentId: 'codex', + model: 'gpt-5.4-mini', + commandInputTemplate: '{basePrompt}' + }, + linkedIssue, + saveTargets: [], + onGenerate: () => {}, + onOpenChange: () => {}, + onSaveDefaults: () => {} + }) + ) + + expect(markup).toContain(escapeHtml(JSON.stringify({ linkedIssue: expected }))) + }) + + it('leaves the issue preview to the synthetic sample when no workspace is in scope', () => { + const markup = renderToStaticMarkup( + React.createElement(SourceControlTextGenerationDialogForm, { + actionId: 'commitMessage', + generateLabel: 'Generate', + settings: null, + repo: null, + baseParams: { + agentId: 'codex', + model: 'gpt-5.4-mini', + commandInputTemplate: '{basePrompt}' + }, + saveTargets: [], + onGenerate: () => {}, + onOpenChange: () => {}, + onSaveDefaults: () => {} + }) + ) + + expect(markup).not.toContain('linkedIssue') + }) + it('preserves the resolved model and thinking level for the selected agent', () => { expect( buildCommitMessageGenerationParams({ diff --git a/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialog.tsx b/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialog.tsx index f3bb32767ab..56c3e52f8b9 100644 --- a/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialog.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialog.tsx @@ -31,6 +31,8 @@ type SourceControlTextGenerationBaseDialogProps = { settings: GlobalSettings | null repo?: Pick | null discoveryHostKey: string + /** Omitted by workspace-less callers (Settings dry-run); `null` means "linked to nothing". */ + linkedIssue?: number | null onGenerate: (params: ResolvedSourceControlAiGenerationParams) => void onSaveDefaults: ( target: SourceControlAiWriteTarget, @@ -89,6 +91,7 @@ export function SourceControlTextGenerationDialog({ settings, repo, discoveryHostKey, + linkedIssue, onGenerate, onSaveDefaults }: SourceControlTextGenerationDialogProps): React.JSX.Element { @@ -179,6 +182,7 @@ export function SourceControlTextGenerationDialog({ repo={repo ?? null} baseParams={baseParams} basePromptPreview={buildBasePromptPreview(actionId)} + linkedIssue={linkedIssue} saveTargets={saveTargets} onGenerate={onGenerate} onOpenChange={onOpenChange} diff --git a/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialogForm.tsx b/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialogForm.tsx index 7315ba66207..18ddce0b417 100644 --- a/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialogForm.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialogForm.tsx @@ -19,6 +19,7 @@ import { listCommitMessageAgentCapabilities } from '../../../../shared/commit-message-agent-spec' import type { ResolvedSourceControlAiGenerationParams } from '../../../../shared/source-control-ai' +import { formatLinkedIssueTemplateValue } from '../../../../shared/source-control-ai-action-variables' import type { SourceControlTextActionId } from '../../../../shared/source-control-ai-actions' import type { SourceControlAiWriteTarget } from '../../../../shared/source-control-ai-recipe-save' import type { GlobalSettings, Repo, TuiAgent } from '../../../../shared/types' @@ -46,6 +47,8 @@ type SourceControlTextGenerationDialogFormProps = { repo: Pick | null baseParams: ResolvedSourceControlAiGenerationParams | null basePromptPreview?: string + /** Omitted by workspace-less callers (Settings dry-run); `null` means "linked to nothing". */ + linkedIssue?: number | null saveTargets: SourceControlTextGenerationSaveTarget[] onGenerate: (params: ResolvedSourceControlAiGenerationParams) => void onOpenChange: (open: boolean) => void @@ -80,6 +83,7 @@ export function SourceControlTextGenerationDialogForm({ repo, baseParams, basePromptPreview, + linkedIssue, saveTargets, onGenerate, onOpenChange, @@ -114,7 +118,21 @@ export function SourceControlTextGenerationDialogForm({ settings, customAgentCommand: baseParams?.customAgentCommand }) - const paramsPlanResult = params ? planSourceControlTextGeneration(actionId, params) : null + // Why: the workspace's own issue, not the synthetic `123`, so an unlinked workspace + // previews what it will actually generate. + const variablePreviews = useMemo(() => { + const previews: Record = {} + if (basePromptPreview) { + previews.basePrompt = basePromptPreview + } + if (linkedIssue !== undefined) { + previews.linkedIssue = formatLinkedIssueTemplateValue(linkedIssue) + } + return Object.keys(previews).length > 0 ? previews : undefined + }, [basePromptPreview, linkedIssue]) + const paramsPlanResult = params + ? planSourceControlTextGeneration(actionId, params, variablePreviews) + : null const canRunGeneration = Boolean(params && paramsPlanResult?.ok) const saving = savingTargetKey !== null const defaultsAlreadySaved = Boolean( @@ -287,7 +305,7 @@ export function SourceControlTextGenerationDialogForm({ /> { const separator = commandTemplate.endsWith('\n') || commandTemplate.length === 0 ? '' : ' ' diff --git a/src/renderer/src/lib/source-control-generation-plan.test.ts b/src/renderer/src/lib/source-control-generation-plan.test.ts index 68d8b19f1b0..0f2faf1caed 100644 --- a/src/renderer/src/lib/source-control-generation-plan.test.ts +++ b/src/renderer/src/lib/source-control-generation-plan.test.ts @@ -64,6 +64,28 @@ describe('planSourceControlCommitMessageGeneration', () => { } }) + it('prefers caller-supplied variable values over the synthetic sample', () => { + for (const [linkedIssue, expected] of [ + ['7', 'echo Fixes #7'], + // Why: an unlinked workspace must preview the empty expansion it will really send, + // not the synthetic `123`. + ['', 'echo Fixes #'] + ]) { + const result = planSourceControlTextGeneration( + 'commitMessage', + { + agentId: 'custom', + model: '', + customAgentCommand: 'echo {prompt}', + commandInputTemplate: 'Fixes #{linkedIssue}' + }, + { linkedIssue } + ) + + expect(result.ok && result.commandLabel).toBe(expected) + } + }) + it('leaves linkedIssue literal in branch-name plan previews', () => { const result = planSourceControlTextGeneration('branchName', { agentId: 'custom', diff --git a/src/renderer/src/lib/source-control-generation-plan.ts b/src/renderer/src/lib/source-control-generation-plan.ts index 87f747b36b7..9edf9809cc4 100644 --- a/src/renderer/src/lib/source-control-generation-plan.ts +++ b/src/renderer/src/lib/source-control-generation-plan.ts @@ -50,16 +50,23 @@ const SYNTHETIC_BASE_PROMPTS: Record = { branchName: 'Generate a git branch name for a synthetic task.' } +/** + * `variableOverrides` lets a workspace-scoped caller (the in-app generation dialog) + * preview real values where it has them, so an unlinked workspace does not preview + * `Fixes #123` and then generate `Fixes #`. Settings dry-runs pass nothing and stay + * fully synthetic, since no workspace is in scope there. + */ export function planSourceControlTextGeneration( actionId: SourceControlTextActionId, - params: ResolvedSourceControlAiGenerationParams + params: ResolvedSourceControlAiGenerationParams, + variableOverrides?: Record ): SourceControlGenerationPlanResult { const prompt = params.commandInputTemplate !== undefined - ? renderSourceControlActionCommandTemplate( - params.commandInputTemplate, - SYNTHETIC_TEXT_GENERATION_CONTEXT[actionId] - ) + ? renderSourceControlActionCommandTemplate(params.commandInputTemplate, { + ...SYNTHETIC_TEXT_GENERATION_CONTEXT[actionId], + ...variableOverrides + }) : SYNTHETIC_BASE_PROMPTS[actionId] if (!prompt.trim()) { return { diff --git a/src/shared/source-control-ai-action-variables.ts b/src/shared/source-control-ai-action-variables.ts new file mode 100644 index 00000000000..e03fa54d0a1 --- /dev/null +++ b/src/shared/source-control-ai-action-variables.ts @@ -0,0 +1,120 @@ +import type { SourceControlActionId } from './source-control-ai-actions' + +export const SOURCE_CONTROL_ACTION_VARIABLES: Record = { + commitMessage: ['basePrompt', 'branch', 'stagedFiles', 'stagedPatch', 'linkedIssue'], + pullRequest: [ + 'basePrompt', + 'branch', + 'baseBranch', + 'currentTitle', + 'currentBody', + 'commitSummary', + 'changedFiles', + 'patch', + 'linkedIssue' + ], + branchName: ['basePrompt', 'firstPrompt', 'assistantMessage'], + fixCommitFailure: ['basePrompt'], + fixPushFailure: ['basePrompt'], + fixChecks: ['basePrompt'], + resolveConflicts: ['basePrompt'], + resolveComments: ['basePrompt'] +} + +export type SourceControlActionVariableInfo = { + description: string + example: string +} + +export const SOURCE_CONTROL_ACTION_VARIABLE_INFO: Record = + { + basePrompt: { + description: + 'Orca’s built-in prompt for this action, including the context Orca knows how to gather safely.', + example: + 'Commit messages include staged diff guidance; PR details include branch comparison guidance; fix actions include the failure summary.' + }, + branch: { + description: 'The current source-control branch name.', + example: 'feature/source-control-ai-recipes' + }, + stagedFiles: { + description: 'A newline-separated list of staged files for commit-message generation.', + example: 'M src/shared/source-control-ai.ts\nA src/shared/source-control-ai-actions.ts' + }, + stagedPatch: { + description: 'The staged git patch used for commit-message generation.', + example: 'diff --git a/src/app.ts b/src/app.ts\n+addActionRecipeDefaults()' + }, + baseBranch: { + description: 'The target branch selected in the Create PR composer.', + example: 'main' + }, + currentTitle: { + description: 'The PR title currently typed in the composer before generation starts.', + example: 'Improve Source Control AI customization' + }, + currentBody: { + description: 'The PR description currently typed in the composer before generation starts.', + example: 'Adds configurable agents and command templates for Source Control actions.' + }, + commitSummary: { + description: 'A newline-separated list of commits on the branch compared to the base.', + example: 'a1b2c3d Add action recipe defaults\nd4e5f6a Render command templates' + }, + changedFiles: { + description: 'A summary of files changed between the branch and the base branch.', + example: + 'src/shared/source-control-ai-actions.ts | 24 +++++\nsrc/main/text-generation.ts | 8 +-' + }, + patch: { + description: 'The branch diff against the base branch used for PR-details generation.', + example: 'diff --git a/src/app.ts b/src/app.ts\n+renderSourceControlActionCommandTemplate()' + }, + firstPrompt: { + description: 'The first user request that created the Orca workspace.', + example: 'Fix CI and commit the result' + }, + assistantMessage: { + description: 'The initial agent response, when Orca has one available.', + example: 'I will inspect the failing check, patch the issue, and run tests.' + }, + linkedIssue: { + description: + 'The GitHub issue number linked to this workspace. Empty when no GitHub issue is linked (including GitLab-linked workspaces). Prefer instructional templates: a bare "Fixes #{linkedIssue}" becomes "Fixes #" when unlinked.', + example: '123' + } + } + +/** + * Issue numbers are positive integers on every supported provider, so anything + * else (negative, zero, fractional, non-finite) is corrupt metadata rather than + * a renderable issue reference — `Fixes #-7` is worse output than `Fixes #`. + * The safe-integer bound also keeps the rendering in decimal notation: `String` + * switches to exponent form (`1e+21`) above it. + */ +export function isLinkedIssueNumber(linkedIssue: unknown): linkedIssue is number { + return typeof linkedIssue === 'number' && Number.isSafeInteger(linkedIssue) && linkedIssue > 0 +} + +/** + * Render the workspace-linked GitHub issue for template substitution. Anything + * that is not a positive integer becomes `''` so the token expands to nothing + * instead of leaking into the prompt. + */ +export function formatLinkedIssueTemplateValue(linkedIssue: number | null | undefined): string { + return isLinkedIssueNumber(linkedIssue) ? String(linkedIssue) : '' +} + +/** + * Attach a resolved issue number to a draft context. Returns the context untouched + * when nothing resolves, so unlinked workspaces keep their existing context shape. + * The `linkedIssue`-bearing constraint keeps the attach off contexts that do not + * declare the field (branch-name generation), where it would be silently unread. + */ +export function withLinkedIssueDraftContext( + context: T, + linkedIssue: number | null | undefined +): T { + return isLinkedIssueNumber(linkedIssue) ? { ...context, linkedIssue } : context +} diff --git a/src/shared/source-control-ai-actions.test.ts b/src/shared/source-control-ai-actions.test.ts index d9f3cd0e043..0178d2407da 100644 --- a/src/shared/source-control-ai-actions.test.ts +++ b/src/shared/source-control-ai-actions.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from 'vitest' +import type { CommitMessageDraftContext } from './commit-message-generation' import { formatLinkedIssueTemplateValue, normalizeSourceControlAiActionDefaults, @@ -200,18 +201,26 @@ describe('source-control AI variable registry', () => { }) describe('formatLinkedIssueTemplateValue', () => { - it('renders finite numbers as decimal strings', () => { + it('renders positive integers as decimal strings', () => { expect(formatLinkedIssueTemplateValue(123)).toBe('123') - expect(formatLinkedIssueTemplateValue(0)).toBe('0') - expect(formatLinkedIssueTemplateValue(-7)).toBe('-7') - expect(formatLinkedIssueTemplateValue(12.9)).toBe('12') + expect(formatLinkedIssueTemplateValue(1)).toBe('1') }) - it('renders missing and non-finite values as an empty string', () => { - expect(formatLinkedIssueTemplateValue(null)).toBe('') - expect(formatLinkedIssueTemplateValue(undefined)).toBe('') - expect(formatLinkedIssueTemplateValue(Number.NaN)).toBe('') - expect(formatLinkedIssueTemplateValue(Number.POSITIVE_INFINITY)).toBe('') + it('renders anything that is not a positive integer as an empty string', () => { + // Why: `Fixes #-7` / `Fixes #1e+21` are worse output than `Fixes #`, so corrupt + // metadata degrades to the unlinked rendering instead of a nonsense reference. + for (const value of [ + 0, + -7, + 12.9, + 1e21, + null, + undefined, + Number.NaN, + Number.POSITIVE_INFINITY + ]) { + expect(formatLinkedIssueTemplateValue(value)).toBe('') + } }) it('expands both brace forms and never leaves the token literal', () => { @@ -230,12 +239,16 @@ describe('formatLinkedIssueTemplateValue', () => { }) describe('withLinkedIssueDraftContext', () => { - it('attaches only finite numbers and leaves the context untouched otherwise', () => { - const context = { branch: 'main', stagedSummary: 'M a.ts', stagedPatch: 'diff' } + it('attaches only positive integers and leaves the context untouched otherwise', () => { + const context: CommitMessageDraftContext = { + branch: 'main', + stagedSummary: 'M a.ts', + stagedPatch: 'diff' + } expect(withLinkedIssueDraftContext(context, 42)).toEqual({ ...context, linkedIssue: 42 }) - expect(withLinkedIssueDraftContext(context, null)).toBe(context) - expect(withLinkedIssueDraftContext(context, undefined)).toBe(context) - expect(withLinkedIssueDraftContext(context, Number.NaN)).toBe(context) + for (const value of [null, undefined, Number.NaN, 0, -7, 12.9]) { + expect(withLinkedIssueDraftContext(context, value)).toBe(context) + } }) }) diff --git a/src/shared/source-control-ai-actions.ts b/src/shared/source-control-ai-actions.ts index 3b39d2cbea6..a4ade895280 100644 --- a/src/shared/source-control-ai-actions.ts +++ b/src/shared/source-control-ai-actions.ts @@ -2,6 +2,17 @@ import { isCustomAgentId, type CustomAgentId } from './commit-message-agent-spec import { isTuiAgent } from './tui-agent-config' import type { TuiAgent } from './types' +// Why: the variable registry lives in its own module for max-lines headroom; +// re-exported here so every existing chip/render consumer keeps one import site. +export { + SOURCE_CONTROL_ACTION_VARIABLES, + SOURCE_CONTROL_ACTION_VARIABLE_INFO, + formatLinkedIssueTemplateValue, + isLinkedIssueNumber, + withLinkedIssueDraftContext, + type SourceControlActionVariableInfo +} from './source-control-ai-action-variables' + export type SourceControlTextActionId = 'commitMessage' | 'pullRequest' | 'branchName' export type SourceControlLaunchActionId = @@ -75,115 +86,6 @@ export const DEFAULT_SOURCE_CONTROL_ACTION_COMMAND_TEMPLATES: Record< resolveComments: '{basePrompt}' } -export const SOURCE_CONTROL_ACTION_VARIABLES: Record = { - commitMessage: ['basePrompt', 'branch', 'stagedFiles', 'stagedPatch', 'linkedIssue'], - pullRequest: [ - 'basePrompt', - 'branch', - 'baseBranch', - 'currentTitle', - 'currentBody', - 'commitSummary', - 'changedFiles', - 'patch', - 'linkedIssue' - ], - branchName: ['basePrompt', 'firstPrompt', 'assistantMessage'], - fixCommitFailure: ['basePrompt'], - fixPushFailure: ['basePrompt'], - fixChecks: ['basePrompt'], - resolveConflicts: ['basePrompt'], - resolveComments: ['basePrompt'] -} - -export type SourceControlActionVariableInfo = { - description: string - example: string -} - -export const SOURCE_CONTROL_ACTION_VARIABLE_INFO: Record = - { - basePrompt: { - description: - 'Orca’s built-in prompt for this action, including the context Orca knows how to gather safely.', - example: - 'Commit messages include staged diff guidance; PR details include branch comparison guidance; fix actions include the failure summary.' - }, - branch: { - description: 'The current source-control branch name.', - example: 'feature/source-control-ai-recipes' - }, - stagedFiles: { - description: 'A newline-separated list of staged files for commit-message generation.', - example: 'M src/shared/source-control-ai.ts\nA src/shared/source-control-ai-actions.ts' - }, - stagedPatch: { - description: 'The staged git patch used for commit-message generation.', - example: 'diff --git a/src/app.ts b/src/app.ts\n+addActionRecipeDefaults()' - }, - baseBranch: { - description: 'The target branch selected in the Create PR composer.', - example: 'main' - }, - currentTitle: { - description: 'The PR title currently typed in the composer before generation starts.', - example: 'Improve Source Control AI customization' - }, - currentBody: { - description: 'The PR description currently typed in the composer before generation starts.', - example: 'Adds configurable agents and command templates for Source Control actions.' - }, - commitSummary: { - description: 'A newline-separated list of commits on the branch compared to the base.', - example: 'a1b2c3d Add action recipe defaults\nd4e5f6a Render command templates' - }, - changedFiles: { - description: 'A summary of files changed between the branch and the base branch.', - example: - 'src/shared/source-control-ai-actions.ts | 24 +++++\nsrc/main/text-generation.ts | 8 +-' - }, - patch: { - description: 'The branch diff against the base branch used for PR-details generation.', - example: 'diff --git a/src/app.ts b/src/app.ts\n+renderSourceControlActionCommandTemplate()' - }, - firstPrompt: { - description: 'The first user request that created the Orca workspace.', - example: 'Fix CI and commit the result' - }, - assistantMessage: { - description: 'The initial agent response, when Orca has one available.', - example: 'I will inspect the failing check, patch the issue, and run tests.' - }, - linkedIssue: { - description: - 'The GitHub issue number linked to this workspace. Empty when no GitHub issue is linked (including GitLab-linked workspaces). Prefer instructional templates: a bare "Fixes #{linkedIssue}" becomes "Fixes #" when unlinked.', - example: '123' - } - } - -/** - * Render the workspace-linked GitHub issue for template substitution. Non-numbers - * become `''` so the token expands to nothing instead of leaking into the prompt. - */ -export function formatLinkedIssueTemplateValue(linkedIssue: number | null | undefined): string { - return typeof linkedIssue === 'number' && Number.isFinite(linkedIssue) - ? String(Math.trunc(linkedIssue)) - : '' -} - -/** - * Attach a resolved issue number to a draft context. Returns the context untouched - * when nothing resolves, so unlinked workspaces keep their existing context shape. - */ -export function withLinkedIssueDraftContext( - context: T, - linkedIssue: number | null | undefined -): T { - return typeof linkedIssue === 'number' && Number.isFinite(linkedIssue) - ? { ...context, linkedIssue } - : context -} - const ACTION_ID_SET = new Set(SOURCE_CONTROL_ACTION_IDS) function isRecord(value: unknown): value is Record { diff --git a/tests/e2e/source-control-commit-message-ai.spec.ts b/tests/e2e/source-control-commit-message-ai.spec.ts index 60fd8c49dc5..2682b8446ef 100644 --- a/tests/e2e/source-control-commit-message-ai.spec.ts +++ b/tests/e2e/source-control-commit-message-ai.spec.ts @@ -40,7 +40,86 @@ function cleanupWorktree(repoPath: string, worktreePath: string, branchName: str } } +/** + * Echoes back whichever issue number reached the prompt, so the assertion covers the + * whole chain (renderer → IPC → worktree meta → template render → agent stdin) rather + * than any single hop. + */ +function writeLinkedIssueEchoGenerator(scriptPath: string): void { + writeFileSync( + scriptPath, + [ + 'const chunks = []', + "process.stdin.on('data', (chunk) => chunks.push(chunk))", + "process.stdin.on('end', () => {", + " const prompt = Buffer.concat(chunks).toString('utf8')", + ' const match = prompt.match(/ORCA_E2E_ISSUE=(\\d*)/)', + " process.stdout.write(`saw-issue:${match ? match[1] || 'empty' : 'missing'}`)", + '})' + ].join('\n') + ) +} + test.describe('Source Control AI commit messages', () => { + test('substitutes the workspace-linked issue into the commit-message recipe', async ({ + orcaPage, + testRepoPath + }) => { + const { branchName, worktreePath } = createWorktreeWithStagedChange(testRepoPath) + const generatorPath = path.join(os.tmpdir(), `${branchName}-linked-issue-generator.cjs`) + writeLinkedIssueEchoGenerator(generatorPath) + + try { + await waitForSessionReady(orcaPage) + await openSourceControlForWorktree(orcaPage, testRepoPath, worktreePath) + + await orcaPage.evaluate( + async ({ generatorPath, linkedIssue }) => { + const store = window.__store + if (!store) { + throw new Error('window.__store is not available') + } + const worktreeId = store.getState().activeWorktreeId + if (!worktreeId) { + throw new Error('No worktree was active after opening Source Control') + } + await window.api.worktrees.updateMeta({ worktreeId, updates: { linkedIssue } }) + const customAgentCommand = `node ${JSON.stringify(generatorPath)}` + await store.getState().updateSettings({ + activeRuntimeEnvironmentId: null, + sourceControlAi: { + enabled: true, + agentId: 'custom' as const, + selectedModelByAgent: {}, + selectedThinkingByModel: {}, + customAgentCommand, + instructionsByOperation: {}, + actions: { + commitMessage: { + agentId: 'custom' as const, + commandInputTemplate: 'ORCA_E2E_ISSUE={linkedIssue}\n\n{basePrompt}' + } + } + } + }) + }, + { generatorPath, linkedIssue: 4242 } + ) + + const textarea = orcaPage.getByRole('textbox', { name: 'Commit message' }) + await expect(textarea).toBeVisible({ timeout: 10_000 }) + + const generate = orcaPage.getByRole('button', { name: 'Generate commit message with AI' }) + await expect(generate).toBeEnabled() + await generate.click() + + await expect(textarea).toHaveValue('saw-issue:4242', { timeout: 15_000 }) + } finally { + rmSync(generatorPath, { force: true }) + cleanupWorktree(testRepoPath, worktreePath, branchName) + } + }) + test('generates a commit message from staged changes through the Source Control UI', async ({ orcaPage, testRepoPath