From c868b9f05ec27b11305fb15434396ddfbfdcbfe8 Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Mon, 7 Sep 2026 11:22:34 -0700 Subject: [PATCH] fix(native-chat): stop a text-free first message burning the naming attempt Both providers claimed namingAttempted before deriving the prompt text, so a caption-free screenshot as the first message left the chat on its placeholder for the session's whole life. Claim the flag after the text is in hand; the prompt reader is now total by construction, so deriving it before the promise cannot fail a delivered message. Also reattaches the JSDoc the max-lines extraction left on the wrapper, and softens a 'never titled on its own' claim to what was actually observed. --- .../claude-conversation-name-turn.test.ts | 27 +++++++++++ .../claude/claude-conversation-name-turn.ts | 22 +++++---- .../codex/codex-conversation-name-turn.ts | 33 +++++++------ ...ructured-session-conversation-name.test.ts | 47 +++++++++++++++++-- .../agent-session-naming-prompt-text.ts | 11 +++++ 5 files changed, 113 insertions(+), 27 deletions(-) diff --git a/src/main/claude/claude-conversation-name-turn.test.ts b/src/main/claude/claude-conversation-name-turn.test.ts index dd04f2af5b0..bbd392ed667 100644 --- a/src/main/claude/claude-conversation-name-turn.test.ts +++ b/src/main/claude/claude-conversation-name-turn.test.ts @@ -46,6 +46,33 @@ describe('startClaudeConversationNaming', () => { expect(onConversationName).toHaveBeenCalledExactlyOnceWith(SESSION, 'Lease probe flake') }) + it('leaves the attempt unspent when the first message carries no text', async () => { + const generateSessionTitle = vi.fn(async () => ({ + outcome: 'named' as const, + title: 'Lease probe flake' + })) + const session = sessionWith(generateSessionTitle) + const imageOnly = { + kind: 'message', + role: 'user', + blocks: [{ type: 'image', path: '/tmp/shot.png' }] + } as unknown as AgentJournalMessageItem + + startClaudeConversationNaming(SESSION, session, imageOnly, { onConversationName: vi.fn() }) + await settle() + + expect(generateSessionTitle).not.toHaveBeenCalled() + // The conversation must stay nameable: a caption-free screenshot is not an + // answer, so the next message with text still gets to ask. + expect(session.namingAttempted).toBe(false) + + const onConversationName = vi.fn() + startClaudeConversationNaming(SESSION, session, USER_TURN, { onConversationName }) + await settle() + + expect(onConversationName).toHaveBeenCalledExactlyOnceWith(SESSION, 'Lease probe flake') + }) + it('passes the user text alone, imposing no title style of its own', async () => { const generateSessionTitle = vi.fn(async () => ({ outcome: 'named' as const, diff --git a/src/main/claude/claude-conversation-name-turn.ts b/src/main/claude/claude-conversation-name-turn.ts index 71445cf519d..fee9434e165 100644 --- a/src/main/claude/claude-conversation-name-turn.ts +++ b/src/main/claude/claude-conversation-name-turn.ts @@ -1,8 +1,8 @@ // Naming a Claude conversation. // // Claude's stream-json protocol carries no title frame, and the CLI's own -// auto-titling lives in its interactive UI: a session driven over stream-json is -// never titled on its own. The Agent SDK exposes the request directly, so Orca +// auto-titling does not fire in practice for a session driven over stream-json +// on current builds. The Agent SDK exposes the request directly, so Orca // asks once, and `persist` makes the CLI write the answer into its transcript as // the `ai-title` record a later attach reads back. // @@ -31,9 +31,10 @@ export type ClaudeConversationNamingDeps = { /** * Names the session once, off the turn's critical path. * - * Asked at most once per CONVERSATION rather than once per session object, and - * every step runs inside the promise: this sits on the send path, and nothing - * here may turn a delivered message into a reported failure. + * Asked at most once per CONVERSATION rather than once per session object. + * Nothing here may turn a delivered message into a reported failure: this sits + * on the send path, so the provider call runs inside the promise and the prompt + * reader is total by construction. */ export function startClaudeConversationNaming( sessionId: string, @@ -44,6 +45,13 @@ export function startClaudeConversationNaming( if (session.namingAttempted || !deps.onConversationName) { return } + // Claimed only once there is text to name from. A caption-free screenshot as + // the first message would otherwise spend the conversation's one attempt and + // leave it on the placeholder for good. + const description = agentSessionNamingPromptText(body) + if (!description) { + return + } session.namingAttempted = true void Promise.resolve() .then(async () => { @@ -51,10 +59,6 @@ export function startClaudeConversationNaming( if (durable?.conversationName || durable?.namingAttempted) { return } - const description = agentSessionNamingPromptText(body) - if (!description) { - return - } const result = await session.connection.generateSessionTitle(description, { persist: true, ...(deps.requestTimeoutMs ? { timeoutMs: deps.requestTimeoutMs } : {}) diff --git a/src/main/codex/codex-conversation-name-turn.ts b/src/main/codex/codex-conversation-name-turn.ts index e770aa9c148..bbc13f2715b 100644 --- a/src/main/codex/codex-conversation-name-turn.ts +++ b/src/main/codex/codex-conversation-name-turn.ts @@ -28,17 +28,6 @@ export type CodexConversationNamingInput = { onError?: (scope: string, error: unknown) => void } -/** - * Names the thread once, off the turn's critical path. - * - * Asked at most once per CONVERSATION, not once per session object: the durable - * marker means a thread the model declined to name, and a name a person - * deliberately cleared, are not re-asked after an eviction or a restart. - * - * Everything runs inside the promise, including reading the user's text: this - * sits on the send path, and nothing here may turn a delivered message into a - * reported failure. - */ /** Shapes the adapter's optional naming deps into a naming turn, one dispatch at a time. */ export function startCodexConversationNamingForTurn( sessionId: string, @@ -58,21 +47,35 @@ export function startCodexConversationNamingForTurn( }) } +/** + * Names the thread once, off the turn's critical path. + * + * Asked at most once per CONVERSATION, not once per session object: the durable + * marker means a thread the model declined to name, and a name a person + * deliberately cleared, are not re-asked after an eviction or a restart. + * + * Nothing here may turn a delivered message into a reported failure: this sits + * on the send path, so the provider call runs inside the promise and the prompt + * reader is total by construction. + */ export function startCodexConversationNaming(input: CodexConversationNamingInput): void { const { session, sessionId } = input if (session.namingAttempted || session.conversationName || !input.onConversationName) { return } + // Claimed only once there is text to name from. A caption-free screenshot as + // the first message would otherwise spend the conversation's one attempt and + // leave it on the placeholder for good. + const prompt = agentSessionNamingPromptText(input.body) + if (!prompt) { + return + } session.namingAttempted = true void Promise.resolve() .then(async () => { if (input.readNamingAttempted?.(sessionId)) { return } - const prompt = agentSessionNamingPromptText(input.body) - if (!prompt) { - return - } const outcome = await generateAndSetCodexConversationName({ connection: session.connection, cwd: session.cwd, diff --git a/src/main/codex/codex-structured-session-conversation-name.test.ts b/src/main/codex/codex-structured-session-conversation-name.test.ts index 108e2ccc92f..21abdd39d90 100644 --- a/src/main/codex/codex-structured-session-conversation-name.test.ts +++ b/src/main/codex/codex-structured-session-conversation-name.test.ts @@ -207,10 +207,21 @@ const USER_TURN = { blocks: [{ type: 'text', text: 'fix the flaky lease probe' }] } as const +const IMAGE_ONLY_TURN = { + kind: 'message', + role: 'user', + blocks: [{ type: 'image', path: '/tmp/shot.png' }] +} as const + async function dispatchedAdapter( codex: ReturnType, - naming: { readNamingAttempted?: () => boolean; markNamingAttempted?: () => void } = {} + naming: { + readNamingAttempted?: () => boolean + markNamingAttempted?: () => void + body?: unknown + } = {} ) { + const { body = USER_TURN, ...namingDeps } = naming const onConversationName = vi.fn() const events: unknown[] = [] const adapter = new CodexStructuredSessionAdapter({ @@ -225,13 +236,13 @@ async function dispatchedAdapter( readProcessStartTime: async () => 1_700_000_000_000, onEvent: (event) => events.push(event), onConversationName, - ...naming + ...namingDeps }) await adapter.acquire({ identity, fence: 7, spawnToken: 'spawn-9' }) await adapter.dispatch({ sessionId: SESSION, clientMessageId: 'client-1', - body: USER_TURN as never, + body: body as never, fence: 7 }) return { adapter, onConversationName, events } @@ -332,6 +343,36 @@ describe('Codex conversation-name generation', () => { }) }) +describe('Codex naming attempt accounting', () => { + it('leaves the attempt unspent when the first message carries no text', async () => { + const codex = namingCodex() + const markNamingAttempted = vi.fn() + const { adapter, onConversationName } = await dispatchedAdapter(codex, { + body: IMAGE_ONLY_TURN, + markNamingAttempted + }) + await settle() + + const ephemeralStarts = () => + codex.calls.filter((call) => call.method === 'thread/start' && call.params.ephemeral === true) + expect(ephemeralStarts()).toHaveLength(0) + expect(markNamingAttempted).not.toHaveBeenCalled() + + // The conversation must stay nameable: a caption-free screenshot is not an + // answer, so the next message with text still gets to ask. + await adapter.dispatch({ + sessionId: SESSION, + clientMessageId: 'client-2', + body: USER_TURN as never, + fence: 7 + }) + await settle() + + expect(ephemeralStarts()).toHaveLength(1) + expect(onConversationName).toHaveBeenCalledExactlyOnceWith(SESSION, 'Fix lease probe') + }) +}) + describe('Codex naming-turn isolation', () => { /** Every event the adapter emitted for the user's session, by thread. */ function emittedThreads(events: unknown[]): string[] { diff --git a/src/main/native-chat/agent-session-wire/agent-session-naming-prompt-text.ts b/src/main/native-chat/agent-session-wire/agent-session-naming-prompt-text.ts index 6537fb9d735..b2cfa1a0c99 100644 --- a/src/main/native-chat/agent-session-wire/agent-session-naming-prompt-text.ts +++ b/src/main/native-chat/agent-session-wire/agent-session-naming-prompt-text.ts @@ -14,6 +14,17 @@ export function agentSessionNamingPromptText(body: AgentJournalMessageItem): str // `blocks` reaches here as provider/journal data, not something the type system // verified: only the RPC send path runs it through a schema. A non-array here // would throw on the send path and turn a delivered message into a failed one. + // The guards below cover every shape this reads; the catch makes that + // structural, so callers may derive the text before the naming attempt is + // claimed rather than only from inside the naming promise. + try { + return readNamingPromptText(body) + } catch { + return null + } +} + +function readNamingPromptText(body: AgentJournalMessageItem): string | null { if (!Array.isArray(body?.blocks)) { return null }