From dba35c7bfd04cdd19f6c6ed3a9f74bcd23cc67d1 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 25 Jul 2026 19:31:30 -0700 Subject: [PATCH] Keep cached linkedIssue when metadata is unavailable Return undefined from getWorktreeLinkedIssue when live metadata cannot be read (store not ready), distinguishing it from null (unlinked). The caller now falls back to the cached worktree value instead of treating unavailable as unlinked. Also extract the linked-issue echo generator as a shared e2e test helper. --- src/main/ipc/filesystem.test.ts | 2 + src/main/runtime/orca-runtime-git.test.ts | 24 ++++++++++++ src/main/runtime/orca-runtime-git.ts | 11 ++++-- src/main/runtime/orca-runtime.ts | 11 +++++- .../SourceControlTextGenerationDialog.test.ts | 17 ++++----- .../helpers/source-control-ai-generators.ts | 24 ++++++++++++ .../source-control-commit-message-ai.spec.ts | 25 +------------ .../source-control-pr-linked-issue-ai.spec.ts | 37 ++++++------------- 8 files changed, 87 insertions(+), 64 deletions(-) diff --git a/src/main/ipc/filesystem.test.ts b/src/main/ipc/filesystem.test.ts index 7aabea4adf1..b3465ac1617 100644 --- a/src/main/ipc/filesystem.test.ts +++ b/src/main/ipc/filesystem.test.ts @@ -2157,6 +2157,8 @@ describe('registerFilesystemHandlers', () => { }) expect(getWorktreeMeta).not.toHaveBeenCalled() + // Why: without this the assertion below passes vacuously on an early return. + expect(generateCommitMessageFromContextMock.mock.calls).toHaveLength(1) expect(generateCommitMessageFromContextMock.mock.calls[0]?.[0]).not.toHaveProperty( 'linkedIssue' ) diff --git a/src/main/runtime/orca-runtime-git.test.ts b/src/main/runtime/orca-runtime-git.test.ts index 665d207a1f1..1103852ed8a 100644 --- a/src/main/runtime/orca-runtime-git.test.ts +++ b/src/main/runtime/orca-runtime-git.test.ts @@ -692,6 +692,30 @@ describe('RuntimeGitCommands', () => { ) }) + it('keeps the cached issue number when live meta is unavailable rather than unlinked', async () => { + const worktreePath = mkdtempSync(join(tmpdir(), 'orca-runtime-git-')) + tempDirs.push(worktreePath) + const context = { branch: 'main', stagedSummary: 'M\tREADME.md', stagedPatch: '+hello' } + const params = { agentId: 'codex', model: 'gpt-5.4-mini' } + mocks.resolveCommitMessageSettings.mockReturnValue({ ok: true, params }) + mocks.getStagedCommitContext.mockResolvedValue(context) + mocks.generateCommitMessageFromContext.mockResolvedValue({ success: true, message: 'docs' }) + const commands = new RuntimeGitCommands({ + resolveRuntimeGitTarget: async () => ({ worktree: makeWorktree(worktreePath, 123) }), + getRuntimeSettings: () => ({}) as GlobalSettings, + // Why: what the host reports when its store is not initialized yet. + getWorktreeLinkedIssue: () => undefined + }) + + await commands.generateRuntimeCommitMessage('id:wt-1') + + expect(mocks.generateCommitMessageFromContext).toHaveBeenCalledWith( + { ...context, linkedIssue: 123 }, + params, + expect.objectContaining({ kind: 'local' }) + ) + }) + it('reads pull-request linked issues from live meta too', async () => { const worktreePath = mkdtempSync(join(tmpdir(), 'orca-runtime-git-')) tempDirs.push(worktreePath) diff --git a/src/main/runtime/orca-runtime-git.ts b/src/main/runtime/orca-runtime-git.ts index 670d59cdeaf..a2dd698444f 100644 --- a/src/main/runtime/orca-runtime-git.ts +++ b/src/main/runtime/orca-runtime-git.ts @@ -165,17 +165,20 @@ export type RuntimeGitCommandHost = { * Live linked-issue read by worktree id. Resolved worktrees come from a * short-TTL cache, so link/unlink would otherwise lag generation; hosts that * implement this are authoritative, including the `null` unlinked answer. + * Return `undefined` when metadata is unavailable (store not ready) so the + * caller keeps the resolved worktree's cached value instead of reading it as + * unlinked. */ - getWorktreeLinkedIssue?(worktreeId: string): number | null + getWorktreeLinkedIssue?(worktreeId: string): number | null | undefined } export class RuntimeGitCommands { constructor(private readonly host: RuntimeGitCommandHost) {} private linkedIssueForTarget(target: RuntimeGitTarget): number | null | undefined { - return this.host.getWorktreeLinkedIssue - ? this.host.getWorktreeLinkedIssue(target.worktree.id) - : target.worktree.linkedIssue + const live = this.host.getWorktreeLinkedIssue?.(target.worktree.id) + // Why: `undefined` means the host could not answer, not "unlinked". + return live === undefined ? target.worktree.linkedIssue : live } async getRuntimeGitStatus( diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index dddfd90600a..d3a9cb25785 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -7065,8 +7065,15 @@ export class OrcaRuntimeService { getCommitMessageAgentEnvironment: () => this.commitMessageAgentEnv ?? undefined, // Why: resolved worktrees are cached for a second, so link/unlink would lag // generation; meta is keyed by the same id the resolver returns. - getWorktreeLinkedIssue: (worktreeId) => - this.store?.getWorktreeMeta?.(worktreeId)?.linkedIssue ?? null + getWorktreeLinkedIssue: (worktreeId) => { + const store = this.store + // Why: an unreadable store is "unknown", not "unlinked" — undefined keeps + // the resolver's cached linkedIssue instead of suppressing {linkedIssue}. + if (!store?.getWorktreeMeta) { + return undefined + } + return store.getWorktreeMeta(worktreeId)?.linkedIssue ?? null + } }) getRuntimeGitStatus: RuntimeGitCommands['getRuntimeGitStatus'] = diff --git a/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialog.test.ts b/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialog.test.ts index 6e46d53451e..1af9d5d8bc9 100644 --- a/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialog.test.ts +++ b/src/renderer/src/components/right-sidebar/SourceControlTextGenerationDialog.test.ts @@ -110,9 +110,8 @@ 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. + // Why: with no preview the chips fall back to the synthetic `123`, promising an unlinked + // workspace `Fixes #123` where it renders `Fixes #`. it.each([ { label: 'the linked issue', linkedIssue: 42, expected: '42' }, { label: 'an empty value when unlinked', linkedIssue: null, expected: '' } @@ -161,8 +160,8 @@ describe('buildCommitMessageGenerationParams', () => { expect(markup).not.toContain('linkedIssue') }) - // Why: previews are chip-only. The recipe is saved repo- or globally scoped, so gating - // Save/Generate on the active workspace's empty `{linkedIssue}` would block a global write. + // Why: the recipe saves repo- or globally scoped, so gating on the active workspace's + // empty `{linkedIssue}` would block a global write. it.each([ { label: 'a linked workspace', linkedIssue: 42 }, { label: 'an unlinked workspace', linkedIssue: null } @@ -172,16 +171,16 @@ describe('buildCommitMessageGenerationParams', () => { linkedIssue }) - // Why: `disabled=""` is the only observable gate here — the "Command input is empty." - // copy is click-driven `generationError` state, so static markup can never show it. + // Why: the "Command input is empty." copy is click-driven state, so `disabled=""` is the + // only gate static markup can observe. expect(markup).toContain('Save defaults') expect(markup).toContain('Generate commit message') expect(markup).not.toContain('disabled=""') }) it('still disables both actions for a template that renders empty for everyone', () => { - // Why: the negative control for the two cases above — without it they pass even if the - // buttons could never be disabled at all. + // Why: negative control — without it the two cases above pass even if the buttons could + // never disable at all. const markup = renderTextGenerationForm({ commandInputTemplate: ' ', linkedIssue: 42 }) expect(markup).toContain('disabled=""') diff --git a/tests/e2e/helpers/source-control-ai-generators.ts b/tests/e2e/helpers/source-control-ai-generators.ts index 67d2133cac0..be3f1b43247 100644 --- a/tests/e2e/helpers/source-control-ai-generators.ts +++ b/tests/e2e/helpers/source-control-ai-generators.ts @@ -27,6 +27,30 @@ async function setCustomGenerator(page: Page, scriptPath: string): Promise }, scriptPath) } +/** + * Writes a generator that 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. `emitPayload` lines run with a captured `issue` + * const in scope and must write the payload the caller's generation path expects. + */ +export function writeLinkedIssueEchoGenerator(scriptPath: string, emitPayload: 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')", + // Why: capture the whole line, not `\d*` — a `\d*` capture matches zero digits before + // an unexpanded `{linkedIssue}` and reports it as `empty`, hiding a literal token. + ' const match = prompt.match(/ORCA_E2E_ISSUE=([^\\r\\n]*)/)', + " const issue = match ? match[1] || 'empty' : 'missing'", + ...emitPayload, + '})' + ].join('\n') + ) +} + export async function installDelayedPrGenerator( page: Page, generatorScriptPath: string, diff --git a/tests/e2e/source-control-commit-message-ai.spec.ts b/tests/e2e/source-control-commit-message-ai.spec.ts index 5d861c94fa5..f6201bc6a75 100644 --- a/tests/e2e/source-control-commit-message-ai.spec.ts +++ b/tests/e2e/source-control-commit-message-ai.spec.ts @@ -3,6 +3,7 @@ import { rmSync, writeFileSync } from 'node:fs' import os from 'node:os' import path from 'node:path' import { test, expect } from './helpers/orca-app' +import { writeLinkedIssueEchoGenerator } from './helpers/source-control-ai-generators' import { waitForSessionReady } from './helpers/store' import { openSourceControlForWorktree } from './helpers/worktree-registration' @@ -40,28 +41,6 @@ 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')", - // Why: capture the whole line, not `\d*` — a `\d*` capture matches zero digits before - // an unexpanded `{linkedIssue}` and reports it as `empty`, hiding a literal token. - ' const match = prompt.match(/ORCA_E2E_ISSUE=([^\\r\\n]*)/)', - " process.stdout.write(`saw-issue:${match ? match[1] || 'empty' : 'missing'}`)", - '})' - ].join('\n') - ) -} - test.describe('Source Control AI commit messages', () => { // Why: the unlinked case separates a real resolver from one that always returns a number, // and — because the generator echoes the whole line — a literal `{linkedIssue}` reaches the @@ -77,7 +56,7 @@ test.describe('Source Control AI commit messages', () => { test(`${label} 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) + writeLinkedIssueEchoGenerator(generatorPath, [' process.stdout.write(`saw-issue:${issue}`)']) try { await waitForSessionReady(orcaPage) diff --git a/tests/e2e/source-control-pr-linked-issue-ai.spec.ts b/tests/e2e/source-control-pr-linked-issue-ai.spec.ts index 82b276dfd2d..625d19d58b8 100644 --- a/tests/e2e/source-control-pr-linked-issue-ai.spec.ts +++ b/tests/e2e/source-control-pr-linked-issue-ai.spec.ts @@ -1,4 +1,4 @@ -import { rmSync, writeFileSync } from 'node:fs' +import { rmSync } from 'node:fs' import os from 'node:os' import path from 'node:path' import { test, expect } from './helpers/orca-app' @@ -7,34 +7,19 @@ import { openSourceControl, seedCreatePrComposer } from './helpers/source-control-ai-generation' +import { writeLinkedIssueEchoGenerator } from './helpers/source-control-ai-generators' import { waitForSessionReady } from './helpers/store' -/** - * Echoes back whichever issue number reached the PR prompt as the generated - * title, so the assertion covers renderer → IPC → meta → template → agent stdin - * for the pull-request path (the commit twin lives in source-control-commit-message-ai). - */ +// Why: the PR path reads the echoed issue from the generated title. function writeLinkedIssuePrEchoGenerator(scriptPath: string, base: 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')", - // Why: capture the whole line, not `\d*` — a `\d*` capture matches zero digits before - // an unexpanded `{linkedIssue}` and reports it as `empty`, hiding a literal token. - ' const match = prompt.match(/ORCA_E2E_ISSUE=([^\\r\\n]*)/)', - " const issue = match ? match[1] || 'empty' : 'missing'", - ' process.stdout.write(JSON.stringify({', - ` base: ${JSON.stringify(base)},`, - ' title: `saw-issue:${issue}`,', - " body: 'linked-issue e2e body',", - ' draft: false', - ' }))', - '})' - ].join('\n') - ) + writeLinkedIssueEchoGenerator(scriptPath, [ + ' process.stdout.write(JSON.stringify({', + ` base: ${JSON.stringify(base)},`, + ' title: `saw-issue:${issue}`,', + " body: 'linked-issue e2e body',", + ' draft: false', + ' }))' + ]) } test.describe('Source Control AI pull request linkedIssue', () => {