diff --git a/src/main/claude/claude-agent-sdk-control-requests.test.ts b/src/main/claude/claude-agent-sdk-control-requests.test.ts index f76650d8151..7755af87ce9 100644 --- a/src/main/claude/claude-agent-sdk-control-requests.test.ts +++ b/src/main/claude/claude-agent-sdk-control-requests.test.ts @@ -24,3 +24,32 @@ describe('createClaudeControlSurface stopTask', () => { expect(stopTask).toHaveBeenCalledTimes(2) }) }) + +describe('createClaudeControlSurface generateSessionTitle', () => { + it('answers null when the CLI exposes no title request at all', async () => { + // The real degradation path: the shipped Query declaration omits the method, + // and an older CLI genuinely does not have it. The surface still exposes the + // method so callers never probe, and answers null rather than throwing. + const surface = createClaudeControlSurface({} as unknown as Query) + + await expect(surface.generateSessionTitle('fix the lease probe')).resolves.toBeNull() + }) + + it('asks the CLI to persist the title and trims what comes back', async () => { + const generateSessionTitle = vi.fn(async () => ' Lease probe flake ') + const surface = createClaudeControlSurface({ generateSessionTitle } as unknown as Query) + + await expect( + surface.generateSessionTitle('fix the lease probe', { persist: true }) + ).resolves.toBe('Lease probe flake') + expect(generateSessionTitle).toHaveBeenCalledWith('fix the lease probe', { persist: true }) + }) + + it('treats a blank title as no title', async () => { + const surface = createClaudeControlSurface({ + generateSessionTitle: async () => ' ' + } as unknown as Query) + + await expect(surface.generateSessionTitle('fix the lease probe')).resolves.toBeNull() + }) +}) diff --git a/src/main/claude/claude-conversation-name-turn.test.ts b/src/main/claude/claude-conversation-name-turn.test.ts index 146d5725c22..39656221bfc 100644 --- a/src/main/claude/claude-conversation-name-turn.test.ts +++ b/src/main/claude/claude-conversation-name-turn.test.ts @@ -59,19 +59,28 @@ describe('startClaudeConversationNaming', () => { ) }) - it('asks only once per session', async () => { + it('asks only once across a re-acquisition, which builds a NEW session object', async () => { const generateSessionTitle = vi.fn(async () => null) - const session = sessionWith(generateSessionTitle) + // Passing the same object twice could only prove the in-memory flag; an + // eviction hands the next send a fresh session, which is the real case. + let attempted = false + const deps = { + onConversationName: vi.fn(), + readNamingState: () => ({ conversationName: null, namingAttempted: attempted }), + markNamingAttempted: () => { + attempted = true + } + } - startClaudeConversationNaming(SESSION, session, USER_TURN, { onConversationName: vi.fn() }) + startClaudeConversationNaming(SESSION, sessionWith(generateSessionTitle), USER_TURN, deps) await settle() - startClaudeConversationNaming(SESSION, session, USER_TURN, { onConversationName: vi.fn() }) + startClaudeConversationNaming(SESSION, sessionWith(generateSessionTitle), USER_TURN, deps) await settle() expect(generateSessionTitle).toHaveBeenCalledOnce() }) - it('reports nothing when the CLI exposes no title request', async () => { + it('reports nothing when the title request answers null', async () => { const onConversationName = vi.fn() startClaudeConversationNaming(SESSION, sessionWith(vi.fn(async () => null)), USER_TURN, { @@ -139,7 +148,7 @@ describe('startClaudeConversationNaming across re-acquisitions', () => { startClaudeConversationNaming(SESSION, reacquired, USER_TURN, { onConversationName: vi.fn(), - readConversationName: () => 'Lease probe flake' + readNamingState: () => ({ conversationName: 'Lease probe flake', namingAttempted: true }) }) await settle() @@ -152,7 +161,7 @@ describe('startClaudeConversationNaming across re-acquisitions', () => { startClaudeConversationNaming(SESSION, sessionWith(generateSessionTitle), USER_TURN, { onConversationName, - readConversationName: () => null + readNamingState: () => ({ conversationName: null, namingAttempted: false }) }) await settle() diff --git a/src/main/claude/claude-conversation-name-turn.ts b/src/main/claude/claude-conversation-name-turn.ts index aeae407e726..143123cf7d6 100644 --- a/src/main/claude/claude-conversation-name-turn.ts +++ b/src/main/claude/claude-conversation-name-turn.ts @@ -9,10 +9,6 @@ // Deliberately NOT Codex's imperative-verb style: Claude's own titling is a short // noun phrase in sentence case, and the SDK call already produces that. Passing // the user's text as the description and nothing else keeps it that way. -// -// Asked at most once per NAMED CONVERSATION, not once per session object: Claude -// rebuilds its session on every acquisition, so the in-memory flag alone would -// retitle the chat — and pay for it — on the second message after every eviction. import type { AgentJournalMessageItem } from '../../shared/agent-session-journal-types' import { agentSessionNamingPromptText } from '../native-chat/agent-session-wire/agent-session-naming-prompt-text' @@ -21,17 +17,23 @@ import type { ClaudeSession } from './claude-structured-session-state' export type ClaudeConversationNamingDeps = { requestTimeoutMs?: number onConversationName?: (sessionId: string, conversationName: string) => void - /** The name already recorded for this session, if any. Claude's session object - * is rebuilt on every acquisition, so the in-memory one-shot flag alone would - * re-title the conversation once per eviction cycle. */ - readConversationName?: (sessionId: string) => string | null + /** The durable naming state. Claude rebuilds its session object on every + * acquisition, so an in-memory flag alone would retitle the conversation — + * and pay for it — on the second message after every eviction. */ + readNamingState?: (sessionId: string) => { + conversationName: string | null + namingAttempted: boolean + } + markNamingAttempted?: (sessionId: string) => void + onError?: (scope: string, error: unknown) => void } /** * Names the session once, off the turn's critical path. * - * One attempt only: a CLI that exposes no title request, or a turn the model - * declined to name, must not be re-asked on every later turn. + * 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. */ export function startClaudeConversationNaming( sessionId: string, @@ -42,33 +44,25 @@ export function startClaudeConversationNaming( if (session.namingAttempted || !deps.onConversationName) { return } - // The durable record outlives the session object; a conversation named on any - // earlier acquisition is never renamed, and never paid for twice. - if (deps.readConversationName?.(sessionId)) { - session.namingAttempted = true - return - } - const description = agentSessionNamingPromptText(body) - if (!description) { - return - } session.namingAttempted = true - // Started inside a promise so NOTHING here can reach the caller. This runs on - // the send path, and an integration fake without the method turned a delivered - // message into a reported failure — a synchronous throw from any cause would do - // the same. A chat with no name beats a send that claims it failed. void Promise.resolve() - .then(() => - session.connection.generateSessionTitle(description, { + .then(async () => { + const durable = deps.readNamingState?.(sessionId) + if (durable?.conversationName || durable?.namingAttempted) { + return + } + const description = agentSessionNamingPromptText(body) + if (!description) { + return + } + deps.markNamingAttempted?.(sessionId) + const title = await session.connection.generateSessionTitle(description, { persist: true, ...(deps.requestTimeoutMs ? { timeoutMs: deps.requestTimeoutMs } : {}) }) - ) - .then((title) => { if (title) { deps.onConversationName?.(sessionId, title) } }) - // A session without a name is the state this started in; never surface it. - .catch(() => undefined) + .catch((error: unknown) => deps.onError?.('claude-conversation-naming', error)) } diff --git a/src/main/claude/claude-structured-session-adapter.ts b/src/main/claude/claude-structured-session-adapter.ts index 85435792d40..d262dda4ac1 100644 --- a/src/main/claude/claude-structured-session-adapter.ts +++ b/src/main/claude/claude-structured-session-adapter.ts @@ -230,7 +230,10 @@ export class ClaudeStructuredSessionAdapter implements StructuredAgentSessionAda if (outcome.state === 'accepted') { // The accepted user message is the only text this session is sure the CLI // received, and the first thing worth naming the conversation after. - startClaudeConversationNaming(input.sessionId, session, input.body, this.deps) + startClaudeConversationNaming(input.sessionId, session, input.body, { + ...this.deps, + ...(this.deps.onNamingError ? { onError: this.deps.onNamingError } : {}) + }) } return outcome } diff --git a/src/main/claude/claude-structured-session-state.ts b/src/main/claude/claude-structured-session-state.ts index e2bdb703808..d374556e018 100644 --- a/src/main/claude/claude-structured-session-state.ts +++ b/src/main/claude/claude-structured-session-state.ts @@ -76,8 +76,13 @@ export type ClaudeStructuredSessionAdapterDeps = { }) => Promise /** Claude named (or the user renamed) the conversation behind this session. */ onConversationName?: (sessionId: string, conversationName: string) => void - /** The name already recorded for a session, so a re-acquisition does not retitle it. */ - readConversationName?: (sessionId: string) => string | null + /** The durable naming state, so a re-acquisition does not retitle. */ + readNamingState?: (sessionId: string) => { + conversationName: string | null + namingAttempted: boolean + } + markNamingAttempted?: (sessionId: string) => void + onNamingError?: (scope: string, error: unknown) => void /** The name Claude already persisted for this provider session, if any. Its * stream carries no title frame, so the transcript is the only source. */ readTranscriptConversationName?: (input: { @@ -136,7 +141,8 @@ export type ClaudeSession = { /** Provider uuid of the most recently admitted turn, if one is active. */ activeTurnId?: string backgroundTasks: ClaudeBackgroundTaskTracker - /** One naming attempt per session; see claude-conversation-name-turn. */ + /** Guards a second attempt within this live session only; the durable marker + * on the record is what survives eviction. See claude-conversation-name-turn. */ namingAttempted: boolean /** Monotonic fence advanced when a dispatch starts, including unresolved dispatches. */ dispatchSequence: number diff --git a/src/main/claude/claude-transcript-conversation-name.ts b/src/main/claude/claude-transcript-conversation-name.ts index 59df1afd358..80387267f61 100644 --- a/src/main/claude/claude-transcript-conversation-name.ts +++ b/src/main/claude/claude-transcript-conversation-name.ts @@ -3,41 +3,47 @@ // // Claude's stream-json protocol carries no title frame, so the transcript is the // only place a name it generated (or a name the user set from the CLI) survives. -// The records are read through the AI Vault session parser rather than a second -// one, so `custom-title` / `ai-title` keep meaning exactly what they mean there. +// `custom-title` and `ai-title` are appended, last-wins records, so the file is +// read backwards in bounded chunks: a full parse on every acquisition competes +// with the attach it runs alongside, on files that reach many megabytes. +// +// Bounded means a title older than the tail limit is NOT found. That reads as +// "no name yet", never as "this conversation has no name" — and once any read +// succeeds the name is on the durable record, so the scan is not repeated. -import { createInterface } from 'node:readline' -import { openTranscriptReadStream } from '../native-chat/wsl-transcript-fs-access' -import { - consumeClaudeSessionLine, - createClaudeSessionParseState -} from '../ai-vault/session-scanner-primary-parsers' +import { normalizeTitleText, parseJsonObject } from '../ai-vault/session-scanner-values' +import { claudeTranscriptTailLines } from './claude-transcript-tail-scan' /** - * The transcript's stored name, or null when it holds none. + * The transcript's stored name, or null when its tail holds none. * * A user's own `custom-title` outranks the generated `ai-title`, matching the - * precedence the CLI itself applies when it shows the session's name. + * precedence the CLI itself applies. Read newest-first, so the first record of + * each kind is the current one. */ export async function readClaudeTranscriptConversationName( transcriptPath: string ): Promise { - const state = createClaudeSessionParseState({ - path: transcriptPath, - mtimeMs: 0, - modifiedAt: new Date(0).toISOString() - }) - const stream = openTranscriptReadStream(transcriptPath, { encoding: 'utf-8' }, 'scan') - const lines = createInterface({ input: stream, crlfDelay: Infinity }) - try { - for await (const line of lines) { - consumeClaudeSessionLine(state, line) + let generated: string | null = null + for await (const line of claudeTranscriptTailLines(transcriptPath)) { + if (!line.includes('-title')) { + continue + } + const record = parseJsonObject(line) + if (!record) { + continue + } + if (record.type === 'custom-title') { + const title = normalizeTitleText(String(record.customTitle ?? '')) + if (title) { + return title + } + } + if (record.type === 'ai-title' && !generated) { + generated = normalizeTitleText(String(record.aiTitle ?? '')) || null } - } finally { - lines.close() - stream.destroy() } - return state.accumulator.title || state.generatedTitle || null + return generated } export type ClaudeConversationNameReporter = { diff --git a/src/main/claude/claude-transcript-tail-scan.ts b/src/main/claude/claude-transcript-tail-scan.ts new file mode 100644 index 00000000000..2a129d1a861 --- /dev/null +++ b/src/main/claude/claude-transcript-tail-scan.ts @@ -0,0 +1,49 @@ +// Reading a Claude transcript backwards, in bounded chunks. +// +// These files reach many megabytes on a long conversation, and the records Orca +// wants from them — the leaf uuid, the session's name — are appended, so the tail +// holds them. Reading the whole file to find a trailing record costs a full parse +// on every acquisition, competing with the attach it runs alongside. +// +// The bound is deliberate: a record older than the limit is NOT found. Every +// caller here treats "not found" as "no answer yet", never as a negative fact. + +import { open } from 'node:fs/promises' + +const TRANSCRIPT_TAIL_CHUNK_BYTES = 64 * 1024 +export const TRANSCRIPT_TAIL_READ_LIMIT_BYTES = 4 * 1024 * 1024 + +/** + * Every non-empty line of the transcript's tail, newest first, up to the limit. + * + * A chunk boundary can split a line, so the leading partial of each chunk is + * carried into the next (earlier) one rather than parsed as a whole line. + */ +export async function* claudeTranscriptTailLines( + transcriptPath: string +): AsyncGenerator { + const file = await open(transcriptPath, 'r') + try { + const { size } = await file.stat() + let position = size + let suffix = '' + let scanned = 0 + while (position > 0 && scanned < TRANSCRIPT_TAIL_READ_LIMIT_BYTES) { + const length = Math.min(TRANSCRIPT_TAIL_CHUNK_BYTES, position) + position -= length + scanned += length + const buffer = Buffer.alloc(length) + await file.read(buffer, 0, length, position) + const lines = `${buffer.toString('utf8')}${suffix}`.split(/\r?\n/) + suffix = position > 0 ? (lines.shift() ?? '') : '' + for (let index = lines.length - 1; index >= 0; index -= 1) { + const line = lines[index]?.trim() + if (line) { + yield line + } + } + } + } finally { + await file.close() + } +} diff --git a/src/main/claude/claude-tui-exit.ts b/src/main/claude/claude-tui-exit.ts index 3772e9c5e52..0edba7c5afe 100644 --- a/src/main/claude/claude-tui-exit.ts +++ b/src/main/claude/claude-tui-exit.ts @@ -1,10 +1,7 @@ -import { open } from 'node:fs/promises' import type { AgentSessionProviderHandleLink } from '../../shared/agent-session-provider-handle' +import { claudeTranscriptTailLines } from './claude-transcript-tail-scan' import { claudeProviderHandleLink } from './claude-structured-owner-identity' -const TRANSCRIPT_TAIL_CHUNK_BYTES = 64 * 1024 -const TRANSCRIPT_TAIL_READ_LIMIT_BYTES = 4 * 1024 * 1024 - type TranscriptLeafCandidate = { leafUuid: string; authoritative: boolean } function validLeafUuid(value: unknown): string | null { @@ -41,40 +38,15 @@ function readLeafCandidate(line: string): TranscriptLeafCandidate | null { } export async function readClaudeTranscriptLeafUuid(transcriptPath: string): Promise { - const file = await open(transcriptPath, 'r') - try { - const { size } = await file.stat() - let position = size - let suffix = '' - let fallback: string | null = null - let scanned = 0 - while (position > 0 && scanned < TRANSCRIPT_TAIL_READ_LIMIT_BYTES) { - const length = Math.min(TRANSCRIPT_TAIL_CHUNK_BYTES, position) - position -= length - scanned += length - const buffer = Buffer.alloc(length) - await file.read(buffer, 0, length, position) - const lines = `${buffer.toString('utf8')}${suffix}`.split(/\r?\n/) - suffix = position > 0 ? (lines.shift() ?? '') : '' - for (let index = lines.length - 1; index >= 0; index -= 1) { - const line = lines[index]?.trim() - if (!line) { - continue - } - const candidate = readLeafCandidate(line) - if (!candidate) { - continue - } - if (candidate.authoritative) { - return candidate.leafUuid - } - fallback ??= candidate.leafUuid - } + let fallback: string | null = null + for await (const line of claudeTranscriptTailLines(transcriptPath)) { + const candidate = readLeafCandidate(line) + if (candidate?.authoritative) { + return candidate.leafUuid } - return fallback - } finally { - await file.close() + fallback ??= candidate?.leafUuid ?? fallback } + return fallback } export type ClaudeTuiChildExit = { diff --git a/src/main/codex/codex-conversation-name-generation.test.ts b/src/main/codex/codex-conversation-name-generation.test.ts index 70e934f727e..5c8a2064388 100644 --- a/src/main/codex/codex-conversation-name-generation.test.ts +++ b/src/main/codex/codex-conversation-name-generation.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from 'vitest' import type { CodexAppServerConnection } from './codex-app-server-connection' import { createCodexNamingTurnCollector, + isTerminalCodexTurnError, generateAndSetCodexConversationName, readCodexGeneratedTitle } from './codex-conversation-name-generation' @@ -10,22 +11,38 @@ const THREAD = 'thread-user' const NAMING = 'thread-naming' /** An app-server that answers the naming flow's four requests. */ -function fakeConnection(options: { nameOnReRead?: string | null; answer?: string | null } = {}) { +function fakeConnection( + options: { + nameOnReRead?: string | null + answer?: string | null + /** The app-server ignored `ephemeral` and persisted the throwaway thread. */ + persistDespiteEphemeral?: boolean + /** A `thread/read` reply shape this build may or may not understand. */ + threadReadReply?: unknown + } = {} +) { const calls: { method: string; params: Record }[] = [] const connection: Pick = { request: vi.fn(async (method: string, params?: Record) => { calls.push({ method, params: params ?? {} }) if (method === 'thread/start') { - return { thread: { id: NAMING, ephemeral: params?.ephemeral === true } } - } - if (method === 'thread/read') { return { thread: { - id: THREAD, - ...(options.nameOnReRead ? { name: options.nameOnReRead } : {}) + id: NAMING, + ...(options.persistDespiteEphemeral ? {} : { ephemeral: params?.ephemeral === true }) } } } + if (method === 'thread/read') { + return options.threadReadReply !== undefined + ? options.threadReadReply + : { + thread: { + id: THREAD, + ...(options.nameOnReRead ? { name: options.nameOnReRead } : {}) + } + } + } return {} }) } @@ -181,3 +198,75 @@ describe('generateAndSetCodexConversationName', () => { expect(calls.some((call) => call.method === 'thread/name/set')).toBe(false) }) }) + +describe('naming-turn cleanup and fail-closed re-read', () => { + it('deletes a throwaway thread the app-server persisted despite ephemeral', async () => { + const { connection, calls } = fakeConnection({ persistDespiteEphemeral: true }) + + await run(connection, '{"title":"Fix flaky lease probe"}') + + // Otherwise every named chat leaves a junk thread and rollout file behind. + expect(calls.find((call) => call.method === 'thread/delete')?.params).toEqual({ + threadId: NAMING + }) + }) + + it('does not try to delete a genuinely ephemeral thread', async () => { + const { connection, calls } = fakeConnection() + + await run(connection, '{"title":"Fix flaky lease probe"}') + + // The app-server refuses to delete one, so attempting it would log a failure + // on every successful naming. + expect(calls.some((call) => call.method === 'thread/delete')).toBe(false) + }) + + // NOTE: a name under an unrecognised KEY on an otherwise-readable reply is not + // detectable — by construction this build does not know the key. What fails + // closed is an unrecognised reply SHAPE, which is the case it can decide. + it.each([ + ['a reply with no thread object', { threadId: 'thread-user' }], + ['a reply that is not an object', 'thread-user'] + ])( + 'refuses to overwrite a name it cannot positively read as absent: %s', + async (_label, reply) => { + const { connection, calls } = fakeConnection({ threadReadReply: reply }) + + await expect(run(connection, '{"title":"Fix flaky lease probe"}')).resolves.toBeNull() + + // Fails CLOSED. Skipping a name is a non-event; clobbering a rename is not. + expect(calls.some((call) => call.method === 'thread/name/set')).toBe(false) + } + ) + + it('still names a thread a readable reply shows as unnamed', async () => { + const { connection, calls } = fakeConnection() + + await expect(run(connection, '{"title":"Fix flaky lease probe"}')).resolves.toBe( + 'Fix flaky lease probe' + ) + + expect(calls.find((call) => call.method === 'thread/name/set')?.params).toEqual({ + threadId: THREAD, + name: 'Fix flaky lease probe' + }) + }) +}) + +describe('isTerminalCodexTurnError', () => { + it('ends the turn on a non-retryable error', () => { + expect(isTerminalCodexTurnError('error', { threadId: 't', willRetry: false })).toBe(true) + }) + + it('does NOT end the turn on a retryable one', () => { + // A retryable error explicitly does not interrupt the turn; settling here + // would abandon a naming turn that was about to succeed. + expect(isTerminalCodexTurnError('error', { threadId: 't', willRetry: true })).toBe(false) + expect(isTerminalCodexTurnError('error', { threadId: 't', will_retry: true })).toBe(false) + }) + + it('ignores anything that is not an error frame', () => { + expect(isTerminalCodexTurnError('turn/started', { willRetry: false })).toBe(false) + expect(isTerminalCodexTurnError('error', null)).toBe(false) + }) +}) diff --git a/src/main/codex/codex-conversation-name-generation.ts b/src/main/codex/codex-conversation-name-generation.ts index c73be1a58f4..4ac81b2e6e6 100644 --- a/src/main/codex/codex-conversation-name-generation.ts +++ b/src/main/codex/codex-conversation-name-generation.ts @@ -25,7 +25,7 @@ import type { CodexAppServerConnection } from './codex-app-server-connection' import { readCodexThreadId, readCodexThreadName } from './codex-structured-thread-facts' -/** Upstream's own bound for a thread name; keeps the tab strip readable. */ +/** Short enough to read as a tab label at a glance; also the schema's own cap. */ export const CODEX_CONVERSATION_NAME_MAX_LENGTH = 36 export const CODEX_CONVERSATION_NAME_SCHEMA = { @@ -82,6 +82,25 @@ export type CodexNamingTurnCollector = { answer: Promise } +/** + * Whether an `error` frame ends the turn. There is no `turn/failed`; a refused or + * rate-limited turn arrives as `error`. A RETRYABLE one explicitly does not + * interrupt the turn, so settling on it would abandon a naming turn that was + * about to succeed. + */ +export function isTerminalCodexTurnError(method: string, params: unknown): boolean { + if (method !== 'error') { + return false + } + if (typeof params !== 'object' || params === null) { + return false + } + return ( + (params as { willRetry?: unknown; will_retry?: unknown }).willRetry !== true && + (params as { will_retry?: unknown }).will_retry !== true + ) +} + function agentMessageText(params: unknown): string | null { if (typeof params !== 'object' || params === null) { return null @@ -109,10 +128,7 @@ export function createCodexNamingTurnCollector(timeoutMs: number): CodexNamingTu handle: (method, params) => { if (method === 'item/completed') { latest = agentMessageText(params) ?? latest - } else if (method === 'turn/completed' || method === 'error') { - // `error` is how the app-server reports a refused or rate-limited turn; - // there is no `turn/failed`. Without it a failed turn would hold this - // open for the full timeout. + } else if (method === 'turn/completed' || isTerminalCodexTurnError(method, params)) { settle(latest) } }, @@ -138,6 +154,38 @@ export function readCodexGeneratedTitle(answer: string | null): string | null { return typeof title === 'string' && title.trim() ? title.trim() : null } +/** + * True only when the reply is one this build understands AND carries no name. + * An unrecognised shape is not evidence of an unnamed thread. + */ +export function isCodexThreadReadablyUnnamed(read: unknown): boolean { + if (typeof read !== 'object' || read === null) { + return false + } + const thread = (read as { thread?: unknown }).thread + if (typeof thread !== 'object' || thread === null) { + return false + } + // A thread reply this build can read always names the thread it describes. + if (typeof (thread as { id?: unknown }).id !== 'string') { + return false + } + return readCodexThreadName(read) === null +} + +/** Whether the app-server confirmed the thread it opened is throwaway. */ +export function readCodexThreadIsEphemeral(opened: unknown): boolean { + if (typeof opened !== 'object' || opened === null) { + return false + } + const thread = (opened as { thread?: unknown }).thread + return ( + typeof thread === 'object' && + thread !== null && + (thread as { ephemeral?: unknown }).ephemeral === true + ) +} + export type CodexConversationNameGeneration = { connection: Pick cwd: string @@ -151,6 +199,8 @@ export type CodexConversationNameGeneration = { * arriving after this flow gives up are dropped rather than journaled. */ retainNamingThread: (namingThreadId: string) => void closeNamingTurn: () => void + /** Diagnostics only; naming never surfaces to the user. */ + onError?: (scope: string, error: unknown) => void } /** @@ -163,6 +213,7 @@ export async function generateAndSetCodexConversationName( const { connection, timeoutMs } = input const collector = input.openNamingTurn() let answer: string | null + let disposableThreadId: string | null = null try { const opened = await connection.request( 'thread/start', @@ -170,13 +221,21 @@ export async function generateAndSetCodexConversationName( { timeoutMs } ) const namingThreadId = readCodexThreadId(opened) - // Never the user's own thread: an app-server that ignored `ephemeral` would - // otherwise have this turn's prompt and JSON answer land in their transcript - // and their history, which is the one outcome this whole path exists to avoid. + // An app-server that ignored `ephemeral` hands back a NEW PERSISTED thread, + // not the user's — so the id check below is not what protects them; the + // delete in the finally block is. The check covers only a reply that names + // the session's own thread, which would put this turn straight into the + // user's chat. if (!namingThreadId || namingThreadId === input.threadId) { return null } input.retainNamingThread(namingThreadId) + // Only when `ephemeral` was NOT honoured. A truly ephemeral thread refuses + // deletion ("thread is not persisted and cannot be deleted"), so attempting + // it unconditionally would log a failure on every successful naming. + if (!readCodexThreadIsEphemeral(opened)) { + disposableThreadId = namingThreadId + } await connection.request( 'turn/start', { @@ -190,6 +249,14 @@ export async function generateAndSetCodexConversationName( answer = await collector.answer } finally { input.closeNamingTurn() + // Set only when the app-server persisted the thread despite `ephemeral`. + // Without this, every named chat would leave a junk thread and rollout file + // in the user's Codex history that Orca never shows and never reclaims. + if (disposableThreadId) { + await connection + .request('thread/delete', { threadId: disposableThreadId }, { timeoutMs }) + .catch((error: unknown) => input.onError?.('delete-naming-thread', error)) + } } const title = readCodexGeneratedTitle(answer) if (!title) { @@ -202,7 +269,10 @@ export async function generateAndSetCodexConversationName( { threadId: input.threadId }, { timeoutMs } ) - if (readCodexThreadName(current)) { + // Fails CLOSED. Only a reply this build can positively read as unnamed permits + // the write: a shape it does not recognise would otherwise read as "unnamed" + // and clobber a name a person chose. Skipping a name is a non-event. + if (!isCodexThreadReadablyUnnamed(current)) { return null } await connection.request( diff --git a/src/main/codex/codex-conversation-name-turn.ts b/src/main/codex/codex-conversation-name-turn.ts index ff7ba99d46e..2475179696c 100644 --- a/src/main/codex/codex-conversation-name-turn.ts +++ b/src/main/codex/codex-conversation-name-turn.ts @@ -2,12 +2,16 @@ // the answer goes. The generation flow itself lives beside this. import type { AgentJournalMessageItem } from '../../shared/agent-session-journal-types' +import { agentSessionNamingPromptText } from '../native-chat/agent-session-wire/agent-session-naming-prompt-text' import { createCodexNamingTurnCollector, generateAndSetCodexConversationName } from './codex-conversation-name-generation' -import { agentSessionNamingPromptText } from '../native-chat/agent-session-wire/agent-session-naming-prompt-text' -import type { CodexSession } from './codex-structured-session-state' +import { readCodexThreadId, readCodexThreadName } from './codex-structured-thread-facts' +import type { + CodexSession, + CodexStructuredSessionAdapterDeps +} from './codex-structured-session-state' /** A naming turn outlives no session: past this the chat keeps its placeholder. */ const NAMING_TURN_TIMEOUT_MS = 60_000 @@ -18,41 +22,56 @@ export type CodexConversationNamingInput = { body: AgentJournalMessageItem requestTimeoutMs?: number onConversationName?: (sessionId: string, conversationName: string) => void + /** Durable "we already asked", so an eviction or restart does not re-ask. */ + readNamingAttempted?: (sessionId: string) => boolean + markNamingAttempted?: (sessionId: string) => void + onError?: (scope: string, error: unknown) => void } /** - * Names the thread once per session, off the turn's critical path. + * Names the thread once, off the turn's critical path. * - * One attempt only: a thread the model declined to name, or one a person - * deliberately cleared, must not be re-asked on every later turn. + * 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. */ export function startCodexConversationNaming(input: CodexConversationNamingInput): void { const { session, sessionId } = input if (session.namingAttempted || session.conversationName || !input.onConversationName) { return } - const prompt = agentSessionNamingPromptText(input.body) - if (!prompt) { - return - } session.namingAttempted = true - void generateAndSetCodexConversationName({ - connection: session.connection, - cwd: session.cwd, - threadId: session.threadId, - prompt, - ...(input.requestTimeoutMs ? { timeoutMs: input.requestTimeoutMs } : {}), - openNamingTurn: () => { - const collector = createCodexNamingTurnCollector(NAMING_TURN_TIMEOUT_MS) - session.naming = collector - return collector - }, - retainNamingThread: (namingThreadId) => session.namingThreadIds.add(namingThreadId), - closeNamingTurn: () => { - session.naming = null - } - }) - .then((name) => { + void Promise.resolve() + .then(async () => { + if (input.readNamingAttempted?.(sessionId)) { + return + } + const prompt = agentSessionNamingPromptText(input.body) + if (!prompt) { + return + } + input.markNamingAttempted?.(sessionId) + const name = await generateAndSetCodexConversationName({ + connection: session.connection, + cwd: session.cwd, + threadId: session.threadId, + prompt, + ...(input.requestTimeoutMs ? { timeoutMs: input.requestTimeoutMs } : {}), + ...(input.onError ? { onError: input.onError } : {}), + openNamingTurn: () => { + const collector = createCodexNamingTurnCollector(NAMING_TURN_TIMEOUT_MS) + session.naming = collector + return collector + }, + retainNamingThread: (namingThreadId) => session.namingThreadIds.add(namingThreadId), + closeNamingTurn: () => { + session.naming = null + } + }) // `thread/name/set` echoes back as `thread/name/updated`, but only while // this session still holds the connection; report directly so a name set // just before a close is not lost. @@ -61,8 +80,44 @@ export function startCodexConversationNaming(input: CodexConversationNamingInput input.onConversationName?.(sessionId, name) } }) - // A thread with no name is the state this started in; never surface it. - .catch(() => { + .catch((error: unknown) => { session.naming = null + input.onError?.('codex-conversation-naming', error) }) } + +/** + * Records a name Codex reported for THIS session's thread, or the clearing of it. + * + * Codex broadcasts `thread/name/updated` for every thread it has stored, so a + * frame naming another thread must not relabel this chat. A frame for this thread + * carrying no name is a deletion: leaving the old one would keep rendering a name + * the user removed. + */ +export function captureCodexConversationName( + sessionId: string, + session: CodexSession, + method: string, + params: unknown, + deps: Pick +): void { + if (method !== 'thread/name/updated') { + return + } + if ((readCodexThreadId(params) ?? session.threadId) !== session.threadId) { + return + } + const conversationName = readCodexThreadName(params) + if (!conversationName) { + if (session.conversationName !== null) { + session.conversationName = null + deps.onConversationNameCleared?.(sessionId) + } + return + } + if (conversationName === session.conversationName) { + return + } + session.conversationName = conversationName + deps.onConversationName?.(sessionId, conversationName) +} diff --git a/src/main/codex/codex-structured-session-adapter.ts b/src/main/codex/codex-structured-session-adapter.ts index 270a568bfc3..6b6e42f750a 100644 --- a/src/main/codex/codex-structured-session-adapter.ts +++ b/src/main/codex/codex-structured-session-adapter.ts @@ -36,8 +36,11 @@ import { deliverCodexUnhandledFrame } from './codex-structured-provider-events' import { isCodexNamingFrame } from './codex-conversation-name-generation' -import { startCodexConversationNaming } from './codex-conversation-name-turn' -import { readCodexThreadId, readCodexThreadName } from './codex-structured-thread-facts' +import { + captureCodexConversationName, + startCodexConversationNaming +} from './codex-conversation-name-turn' +import { readCodexThreadId } from './codex-structured-thread-facts' import { CodexStructuredTurnCancellation } from './codex-structured-turn-cancellation' import { createCodexStructuredNotificationRetry } from './codex-structured-notification-retry' import { acquireCodexStructuredSession } from './codex-structured-session-acquire' @@ -121,41 +124,19 @@ export class CodexStructuredSessionAdapter implements StructuredAgentSessionAdap if (this.turnCancellation.handleNotification(sessionId, session, method, params)) { return { accepted: true } } - // Before every other handler: a naming turn runs on a throwaway thread over - // this same connection, and the item translator journals items from ANY + // Before anything can journal it: a naming turn runs on a throwaway thread + // over this same connection, and the item translator journals items from ANY // thread. Routed here, its prompt and its JSON answer never reach the chat. if (isCodexNamingFrame(session, readCodexThreadId(params))) { session.naming?.handle(method, params) return { accepted: true } } - this.captureConversationName(sessionId, session, method, params) + captureCodexConversationName(sessionId, session, method, params, this.deps) return deliverCodexNotification(sessionId, session, method, params, (current, event) => this.emit(current, event) ) } - /** Codex broadcasts `thread/name/updated` for every thread it has stored, so a - * frame naming another thread must not relabel this session's chat. */ - private captureConversationName( - sessionId: string, - session: CodexSession, - method: string, - params: unknown - ): void { - if (method !== 'thread/name/updated') { - return - } - if ((readCodexThreadId(params) ?? session.threadId) !== session.threadId) { - return - } - const conversationName = readCodexThreadName(params) - if (!conversationName || conversationName === session.conversationName) { - return - } - session.conversationName = conversationName - this.deps.onConversationName?.(sessionId, conversationName) - } - /** Journal first so observers never see an event ahead of its durable row. */ private emit( session: CodexSession, @@ -225,7 +206,14 @@ export class CodexStructuredSessionAdapter implements StructuredAgentSessionAdap ...(this.deps.requestTimeoutMs ? { requestTimeoutMs: this.deps.requestTimeoutMs } : {}), ...(this.deps.onConversationName ? { onConversationName: this.deps.onConversationName } - : {}) + : {}), + ...(this.deps.readNamingAttempted + ? { readNamingAttempted: this.deps.readNamingAttempted } + : {}), + ...(this.deps.markNamingAttempted + ? { markNamingAttempted: this.deps.markNamingAttempted } + : {}), + ...(this.deps.onNamingError ? { onError: this.deps.onNamingError } : {}) }) } return outcome 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 8d6bd66bcdc..df73480dca0 100644 --- a/src/main/codex/codex-structured-session-conversation-name.test.ts +++ b/src/main/codex/codex-structured-session-conversation-name.test.ts @@ -188,7 +188,10 @@ const USER_TURN = { blocks: [{ type: 'text', text: 'fix the flaky lease probe' }] } as const -async function dispatchedAdapter(codex: ReturnType) { +async function dispatchedAdapter( + codex: ReturnType, + naming: { readNamingAttempted?: () => boolean; markNamingAttempted?: () => void } = {} +) { const onConversationName = vi.fn() const events: unknown[] = [] const adapter = new CodexStructuredSessionAdapter({ @@ -202,7 +205,8 @@ async function dispatchedAdapter(codex: ReturnType) { openConnection: codex.openConnection, readProcessStartTime: async () => 1_700_000_000_000, onEvent: (event) => events.push(event), - onConversationName + onConversationName, + ...naming }) await adapter.acquire({ identity, fence: 7, spawnToken: 'spawn-9' }) await adapter.dispatch({ @@ -249,6 +253,30 @@ describe('Codex conversation-name generation', () => { expect(JSON.stringify(events)).not.toContain('Fix lease probe') }) + it('asks only once across a re-acquisition, which builds a NEW session', async () => { + // A second acquisition rebuilds the session object, so the in-memory flag + // resets. Only the durable marker stops the user's next message paying for + // a second naming turn — and re-imposing a name they may have cleared. + let attempted = false + const naming = { + readNamingAttempted: () => attempted, + markNamingAttempted: () => { + attempted = true + } + } + const first = namingCodex({ answer: 'I could not think of one' }) + await dispatchedAdapter(first, naming) + await settle() + const second = namingCodex({ answer: 'I could not think of one' }) + await dispatchedAdapter(second, naming) + await settle() + + const ephemeralStarts = (codex: ReturnType) => + codex.calls.filter((call) => call.method === 'thread/start' && call.params.ephemeral === true) + expect(ephemeralStarts(first)).toHaveLength(1) + expect(ephemeralStarts(second)).toHaveLength(0) + }) + it('asks only once per session, even when the first attempt produced no name', async () => { // A model that declines to answer leaves `conversationName` null, so the // one-shot flag is the ONLY thing stopping a second attempt. With a name set diff --git a/src/main/codex/codex-structured-session-state.ts b/src/main/codex/codex-structured-session-state.ts index b76cfbfc29a..b5b5175bee1 100644 --- a/src/main/codex/codex-structured-session-state.ts +++ b/src/main/codex/codex-structured-session-state.ts @@ -45,6 +45,11 @@ export type CodexStructuredSessionAdapterDeps = { onEvent?: (event: CodexStructuredSessionEvent) => void /** Codex named (or renamed) the thread behind this session. */ onConversationName?: (sessionId: string, conversationName: string) => void + /** Codex reports the thread has no name any more. */ + onConversationNameCleared?: (sessionId: string) => void + readNamingAttempted?: (sessionId: string) => boolean + markNamingAttempted?: (sessionId: string) => void + onNamingError?: (scope: string, error: unknown) => void openConnection?: typeof openCodexAppServerConnection readProcessStartTime?: (pid: number) => Promise mintLinkId?: () => string @@ -76,8 +81,9 @@ export type CodexSession = { /** Every throwaway thread this session opened for naming. Retained for the * session's life: an abandoned turn is never cancelled and can still emit. */ namingThreadIds: Set - /** One naming attempt per session: a thread the model declined to name, or one - * a person deliberately cleared, must not be re-asked on every later turn. */ + /** Guards a SECOND attempt within this live session only. The durable answer + * to "have we asked" lives on the record; this just fences concurrent sends + * before that write lands. */ namingAttempted: boolean prompts: CodexAcquisitionWindow['prompts'] options: Map diff --git a/src/main/native-chat/agent-session-wire/agent-session-naming-prompt-text.test.ts b/src/main/native-chat/agent-session-wire/agent-session-naming-prompt-text.test.ts index 777b352d96e..554fc692fe7 100644 --- a/src/main/native-chat/agent-session-wire/agent-session-naming-prompt-text.test.ts +++ b/src/main/native-chat/agent-session-wire/agent-session-naming-prompt-text.test.ts @@ -42,3 +42,20 @@ describe('agentSessionNamingPromptText', () => { expect(text).toHaveLength(2_000) }) }) + +describe('agentSessionNamingPromptText hostile input', () => { + it.each([ + ['absent blocks', {}], + ['null blocks', { blocks: null }], + ['a non-array blocks', { blocks: 'fix the probe' }], + ['an absent body', undefined] + ])('reports null rather than throwing on %s', (_label, body) => { + // This runs on the send path. Only the RPC send schema guarantees an array; + // journal-replay and resend callers do not pass through it, and a throw here + // would turn a delivered message into a reported failure. + expect(() => + agentSessionNamingPromptText(body as unknown as AgentJournalMessageItem) + ).not.toThrow() + expect(agentSessionNamingPromptText(body as unknown as AgentJournalMessageItem)).toBeNull() + }) +}) 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 9a63b1a44ed..89c9e5ca063 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 @@ -11,6 +11,12 @@ const MAX_NAMING_PROMPT_LENGTH = 2_000 * path would leak a filesystem location into a generated label. */ export function agentSessionNamingPromptText(body: AgentJournalMessageItem): string | null { + // `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. + if (!Array.isArray(body?.blocks)) { + return null + } const text = (body.blocks as NativeChatBlock[]) .filter((block): block is Extract => block.type === 'text') .map((block) => block.text) diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-conversation-name.test.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-conversation-name.test.ts index 1447fc695eb..71f4eea06ad 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-conversation-name.test.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-conversation-name.test.ts @@ -5,73 +5,129 @@ import { StructuredAgentSessionConversationNames } from './structured-agent-sess const SESSION = 'session-1' function harness( - options: { - stored?: string | undefined - setConversationName?: ( - sessionId: string, - conversationName: string, - now: number - ) => Promise - } = {} + stored: { conversationName?: string; conversationNamingAttempted?: boolean } = {}, + options: { failWrites?: boolean } = {} ) { - const stored = { conversationName: options.stored } - const setConversationName = - options.setConversationName ?? - vi.fn(async (_sessionId: string, conversationName: string) => { - stored.conversationName = conversationName + const record = { ...stored } + const applyConversationNaming = vi.fn( + async (_sessionId: string, change: { conversationName?: string | null; attempted?: true }) => { + if (options.failWrites) { + throw new Error('agent_session_identity_required') + } + if (change.conversationName === null) { + delete record.conversationName + } else if (change.conversationName !== undefined) { + record.conversationName = change.conversationName + } + if (change.attempted) { + record.conversationNamingAttempted = true + } return {} as AgentSessionRecord - }) + } + ) const onChanged = vi.fn() + const onError = vi.fn() const names = new StructuredAgentSessionConversationNames({ store: { - getRecord: () => stored as AgentSessionRecord, - setConversationName: setConversationName as never + getRecord: () => record as AgentSessionRecord, + applyConversationNaming: applyConversationNaming as never }, now: () => 5, - onChanged + onChanged, + onError }) - return { names, onChanged, setConversationName, stored } + return { names, onChanged, onError, applyConversationNaming, record } } describe('StructuredAgentSessionConversationNames', () => { it('persists a published name and announces the change once', async () => { - const { names, onChanged, setConversationName, stored } = harness() + const { names, onChanged, record } = harness() await names.publish(SESSION, ' Fix the\nlease probe ') - expect(setConversationName).toHaveBeenCalledWith(SESSION, 'Fix the lease probe', 5) - expect(stored.conversationName).toBe('Fix the lease probe') + expect(record.conversationName).toBe('Fix the lease probe') expect(onChanged).toHaveBeenCalledExactlyOnceWith(SESSION, 'Fix the lease probe') }) it('does not rewrite or re-announce a name the record already holds', async () => { - const { names, onChanged, setConversationName } = harness({ stored: 'Fix the lease probe' }) + const { names, onChanged, applyConversationNaming } = harness({ + conversationName: 'Fix the lease probe' + }) await names.publish(SESSION, 'Fix the lease probe') - expect(setConversationName).not.toHaveBeenCalled() + expect(applyConversationNaming).not.toHaveBeenCalled() expect(onChanged).not.toHaveBeenCalled() }) it('ignores a report that carries no usable name', async () => { - const { names, onChanged, setConversationName } = harness() + const { names, onChanged, applyConversationNaming } = harness() await names.publish(SESSION, '') await names.publish(SESSION, null) await names.publish(SESSION, { name: 'nope' }) - expect(setConversationName).not.toHaveBeenCalled() + expect(applyConversationNaming).not.toHaveBeenCalled() expect(onChanged).not.toHaveBeenCalled() }) - it('keeps a store failure off the caller and announces nothing', async () => { - const { names, onChanged } = harness({ - setConversationName: vi.fn(async () => { - throw new Error('agent_session_identity_required') - }) + it('clears a name the provider says is gone, and announces the clear', async () => { + const { names, onChanged, record } = harness({ conversationName: 'Fix the lease probe' }) + + await names.clear(SESSION) + + // A name the user deleted elsewhere must not linger here and keep rendering. + expect(record.conversationName).toBeUndefined() + expect(onChanged).toHaveBeenCalledExactlyOnceWith(SESSION, null) + }) + + it('does nothing when clearing a conversation that has no name', async () => { + const { names, onChanged, applyConversationNaming } = harness() + + await names.clear(SESSION) + + expect(applyConversationNaming).not.toHaveBeenCalled() + expect(onChanged).not.toHaveBeenCalled() + }) + + it('records the attempt durably, and reads it back', async () => { + const { names, record } = harness() + + expect(names.read(SESSION).namingAttempted).toBe(false) + await names.markAttempted(SESSION) + + expect(record.conversationNamingAttempted).toBe(true) + expect(names.read(SESSION).namingAttempted).toBe(true) + }) + + it('reads the stored name and attempted marker together', () => { + const { names } = harness({ + conversationName: 'Fix the lease probe', + conversationNamingAttempted: true }) + expect(names.read(SESSION)).toEqual({ + conversationName: 'Fix the lease probe', + namingAttempted: true + }) + }) + + it('keeps a store failure off the caller, announces nothing, and reports it', async () => { + const { names, onChanged, onError } = harness({}, { failWrites: true }) + await expect(names.publish(SESSION, 'Fix the lease probe')).resolves.toBeUndefined() + expect(onChanged).not.toHaveBeenCalled() + // Silent to the user, never silent to the log: a host whose store refuses + // the write must not look like a model that declined to name anything. + expect(onError).toHaveBeenCalledWith('apply-name', expect.any(Error)) + }) + + it('reports a failed attempt marker rather than swallowing it', async () => { + const { names, onError } = harness({}, { failWrites: true }) + + await names.markAttempted(SESSION) + + expect(onError).toHaveBeenCalledWith('mark-attempted', expect.any(Error)) }) }) diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-conversation-name.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-conversation-name.ts index 1e8d1777196..211a05aa99b 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-conversation-name.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-conversation-name.ts @@ -1,42 +1,91 @@ // Where a provider's name for a conversation becomes the structured chat's label. // // Providers PUSH: Codex reports the thread's name when it opens one and again on -// every rename, and Claude's name is read out of its transcript once a session -// is live. Nothing polls, so the name has one path in and one way out — the -// durable record — and only a name that actually CHANGED notifies. A re-read on -// every attach must not re-publish an unchanged label. +// every rename or clear, and Claude's name is read out of its transcript once a +// session is live. Nothing polls, so the name has one path in and one way out — +// the durable record. +// +// The record is also where "we already asked" lives. Both providers rebuild their +// session object on every acquisition, so an in-memory flag would re-ask after +// every eviction: re-imposing a name the user cleared, and paying for it again. // // Nothing here may fail an attach or a turn: the name is display metadata, so a -// store write that loses a race with a close is dropped rather than retried. +// write that loses a race with a close is logged and dropped, never retried. import { normalizeAgentSessionConversationName } from '../../../shared/agent-session-conversation-name' import type { AgentSessionRecordStore } from '../../runtime/agent-session-record-store' +export type StructuredAgentSessionNamingState = { + conversationName: string | null + namingAttempted: boolean +} + export type StructuredAgentSessionConversationNameDeps = { - store: Pick + store: Pick now: () => number - onChanged: (sessionId: string, conversationName: string) => void + onChanged: (sessionId: string, conversationName: string | null) => void + /** Diagnostics only. Naming never surfaces to the user, but a host whose + * app-server refuses to name threads must not be indistinguishable from a + * model that simply declined. */ + onError?: (scope: string, error: unknown) => void } export class StructuredAgentSessionConversationNames { constructor(private readonly deps: StructuredAgentSessionConversationNameDeps) {} - /** Records a name a provider published. Ignores anything that is not a usable name. */ + /** What a provider needs to know before deciding whether to ask for a name. */ + read = (sessionId: string): StructuredAgentSessionNamingState => { + const record = this.deps.store.getRecord(sessionId) + return { + conversationName: record?.conversationName ?? null, + namingAttempted: record?.conversationNamingAttempted === true + } + } + + /** Records a name a provider published, or clears it when the provider says so. */ publish = async (sessionId: string, reported: unknown): Promise => { const conversationName = normalizeAgentSessionConversationName(reported) if (!conversationName) { return } - // Read first so an unchanged name costs no durable transaction and no fan-out. - if (this.deps.store.getRecord(sessionId)?.conversationName === conversationName) { + await this.apply(sessionId, { conversationName }, conversationName) + } + + /** The provider reports this conversation has no name any more. */ + clear = async (sessionId: string): Promise => { + if (this.read(sessionId).conversationName === null) { + return + } + await this.apply(sessionId, { conversationName: null }, null) + } + + /** Durably marks that a naming attempt happened, so no later session repeats it. */ + markAttempted = async (sessionId: string): Promise => { + if (this.read(sessionId).namingAttempted) { return } try { - await this.deps.store.setConversationName(sessionId, conversationName, this.deps.now()) - } catch { - // The record is gone or reconciling. A label is never worth surfacing a failure for. + await this.deps.store.applyConversationNaming(sessionId, { attempted: true }, this.deps.now()) + } catch (error) { + this.deps.onError?.('mark-attempted', error) + } + } + + private apply = async ( + sessionId: string, + change: { conversationName: string | null }, + next: string | null + ): Promise => { + // Read first so an unchanged name costs no durable transaction and no fan-out. + if (this.read(sessionId).conversationName === next) { return } - this.deps.onChanged(sessionId, conversationName) + try { + await this.deps.store.applyConversationNaming(sessionId, change, this.deps.now()) + } catch (error) { + this.deps.onError?.('apply-name', error) + return + } + this.deps.onChanged(sessionId, next) } } diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-host.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-host.ts index 9630a9f3e3f..4f9686f0c94 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-host.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-host.ts @@ -329,8 +329,9 @@ export class StructuredAgentSessionHost { ) => this.backgroundTasks.publish(sessionId, state) unsubscribe = (sessionId: string, id: string): void => this.subscribers.close(sessionId, id) - /** Re-projects one session after its RECORD changed; journal writes publish themselves. */ - republishStatus = (sessionId: string): void => this.statusFeed.publish(sessionId) + /** The provider's name for one conversation, for a surface publishing its tab. */ + readConversationName = (sessionId: string): string | null => + this.deps.store.getRecord(sessionId)?.conversationName ?? null /** Every session's projected status for session lists; unlike `subscribe`, retains nothing. */ subscribeStatus = ( diff --git a/src/main/runtime/agent-session-conversation-name-store.test.ts b/src/main/runtime/agent-session-conversation-name-store.test.ts index 6fb0f1564cb..4d36d504467 100644 --- a/src/main/runtime/agent-session-conversation-name-store.test.ts +++ b/src/main/runtime/agent-session-conversation-name-store.test.ts @@ -49,7 +49,11 @@ describe('agent session conversation name', () => { const store = await AgentSessionRecordStore.open({ directory, hostId: 'local' }) await store.reserveOwner(request()) - await store.setConversationName(SESSION, 'Fix the lease probe', NOW + 1) + await store.applyConversationNaming( + SESSION, + { conversationName: 'Fix the lease probe' }, + NOW + 1 + ) const reopened = await AgentSessionRecordStore.open({ directory, hostId: 'local' }) expect(reopened.getRecord(SESSION)?.conversationName).toBe('Fix the lease probe') @@ -58,9 +62,17 @@ describe('agent session conversation name', () => { it('leaves the record untouched when the name it is given is the stored one', async () => { const store = await AgentSessionRecordStore.open({ directory, hostId: 'local' }) await store.reserveOwner(request()) - const named = await store.setConversationName(SESSION, 'Fix the lease probe', NOW + 1) + const named = await store.applyConversationNaming( + SESSION, + { conversationName: 'Fix the lease probe' }, + NOW + 1 + ) - const again = await store.setConversationName(SESSION, 'Fix the lease probe', NOW + 2) + const again = await store.applyConversationNaming( + SESSION, + { conversationName: 'Fix the lease probe' }, + NOW + 2 + ) expect(again.updatedAt).toBe(named.updatedAt) }) @@ -68,9 +80,9 @@ describe('agent session conversation name', () => { it('refuses a name for a session that has no record', async () => { const store = await AgentSessionRecordStore.open({ directory, hostId: 'local' }) - await expect(store.setConversationName('missing-session', 'name', NOW)).rejects.toThrow( - 'agent_session_identity_required' - ) + await expect( + store.applyConversationNaming('missing-session', { conversationName: 'name' }, NOW) + ).rejects.toThrow('agent_session_identity_required') }) }) diff --git a/src/main/runtime/agent-session-record-conversation-name.ts b/src/main/runtime/agent-session-record-conversation-name.ts index e564e72c642..5556d473cff 100644 --- a/src/main/runtime/agent-session-record-conversation-name.ts +++ b/src/main/runtime/agent-session-record-conversation-name.ts @@ -1,18 +1,57 @@ import type { AgentSessionRecord } from '../../shared/agent-session-record' /** - * Records the provider's name for a conversation. + * Records the provider's name for a conversation, or clears it. * * Unfenced on purpose: a name is display metadata, not ownership, so a reader - * that learned it must not have to win the lease to keep it. An unchanged name + * that learned it must not have to win the lease to keep it. An unchanged value * is returned as-is, so a re-read costs no durable write. + * + * `null` clears: a user who deletes the name in another client must not have it + * linger here and keep rendering. */ export function setAgentSessionRecordConversationName( record: AgentSessionRecord, - conversationName: string, + conversationName: string | null, now: number ): AgentSessionRecord { + if (conversationName === null) { + if (record.conversationName === undefined) { + return record + } + const { conversationName: _cleared, ...rest } = record + return { ...rest, updatedAt: now } + } return record.conversationName === conversationName ? record : { ...record, conversationName, updatedAt: now } } + +/** Marks that a naming attempt has been made, so no later session repeats it. */ +export function markAgentSessionRecordConversationNamingAttempted( + record: AgentSessionRecord, + now: number +): AgentSessionRecord { + return record.conversationNamingAttempted === true + ? record + : { ...record, conversationNamingAttempted: true, updatedAt: now } +} + +/** What one naming update changes: the name, the attempted marker, or both. */ +export type AgentSessionConversationNamingChange = { + /** A string sets the name; `null` clears it; omitted leaves it alone. */ + conversationName?: string | null + attempted?: true +} + +export function applyAgentSessionRecordConversationNaming( + record: AgentSessionRecord, + change: AgentSessionConversationNamingChange, + now: number +): AgentSessionRecord { + const named = + change.conversationName === undefined + ? record + : setAgentSessionRecordConversationName(record, change.conversationName, now) + return change.attempted ? markAgentSessionRecordConversationNamingAttempted(named, now) : named +} diff --git a/src/main/runtime/agent-session-record-store.ts b/src/main/runtime/agent-session-record-store.ts index 2af0c32dda6..7d21a00b3f6 100644 --- a/src/main/runtime/agent-session-record-store.ts +++ b/src/main/runtime/agent-session-record-store.ts @@ -1,7 +1,6 @@ /** Durable single-writer session records and their operation ledger. */ import { - agentSessionOperationKey, settleAgentSessionOperation, type AgentSessionOperationDecision, type AgentSessionOperationOutcome, @@ -47,7 +46,10 @@ import { isAgentSessionClaimKeyVerifiable, retireAgentSessionClaimKey } from './agent-session-claim-key-retention' -import { setAgentSessionRecordConversationName } from './agent-session-record-conversation-name' +import { + applyAgentSessionRecordConversationNaming, + type AgentSessionConversationNamingChange +} from './agent-session-record-conversation-name' import { listVisibleAgentSessionIds, setAgentSessionTabVisibility, @@ -59,10 +61,7 @@ import { type AgentSessionReservationProcesslessProof } from './agent-session-processless-reservation' import { - admitPendingAgentSessionReservationReplay, - applyAgentSessionReservation, - evaluateAgentSessionReserveOperation, - requireAgentSessionRecordForReplay, + commitAgentSessionReservation, type AgentSessionReserveRequest, type AgentSessionReserveResult } from './agent-session-reservation-admission' @@ -162,26 +161,9 @@ export class AgentSessionRecordStore { * operation returns the recorded outcome and never reaches the reservation. */ async reserveOwner(request: AgentSessionReserveRequest): Promise { - return this.transact(() => { - const decision = evaluateAgentSessionReserveOperation(this.state, request) - if (decision.decision === 'refused') { - throw new Error(decision.code) - } - if (decision.decision === 'replay') { - let record = requireAgentSessionRecordForReplay(this.state, decision.row, request.sessionId) - if (decision.row.outcome.status === 'pending' && request.handoffOperationId !== null) { - record = admitPendingAgentSessionReservationReplay(record, request) - } - return { record, disposition: 'replayed' as const, operationRow: decision.row } - } - const result = applyAgentSessionReservation(this.state, request, AGENT_SESSION_LEASE_TTL_MS) - this.state.operations.set( - agentSessionOperationKey(request.operation.callerKey, request.operation.operationId), - decision.row - ) - this.state.records.set(result.record.sessionId, result.record) - return { ...result, operationRow: decision.row } - }) + return this.transact(() => + commitAgentSessionReservation(this.state, request, AGENT_SESSION_LEASE_TTL_MS) + ) } async commitProcessIdentity( @@ -310,13 +292,13 @@ export class AgentSessionRecordStore { replaceSessionOptions = (args: AgentSessionOptionsReplacement): Promise => this.mutate(args.sessionId, (record) => replaceAgentSessionRecordOptions(record, args)) - setConversationName = ( + applyConversationNaming = ( sessionId: string, - conversationName: string, + change: AgentSessionConversationNamingChange, now: number ): Promise => this.mutate(sessionId, (record) => - setAgentSessionRecordConversationName(record, conversationName, now) + applyAgentSessionRecordConversationNaming(record, change, now) ) async retireClaimKey(keyId: string, now: number): Promise { diff --git a/src/main/runtime/agent-session-reservation-admission.ts b/src/main/runtime/agent-session-reservation-admission.ts index f1be94a2c0e..9bb4326f74d 100644 --- a/src/main/runtime/agent-session-reservation-admission.ts +++ b/src/main/runtime/agent-session-reservation-admission.ts @@ -6,6 +6,7 @@ * the result inside one transaction; nothing here mutates. */ +import { agentSessionOperationKey } from '../../shared/agent-session-operation-ledger' import { evaluateAgentSessionOperation, pruneAgentSessionOperationRows, @@ -216,3 +217,35 @@ function createAgentSessionRecord( } } } + +/** + * The whole reservation transition: adjudicate the operation, replay a recorded + * outcome, or apply a fresh reservation and record its ledger row. + * + * Lives beside the pieces it orchestrates rather than in the store, which owns + * durability and serialization rather than reservation policy. + */ +export function commitAgentSessionReservation( + state: AgentSessionStoreState, + request: AgentSessionReserveRequest, + leaseTtlMs: number +): AgentSessionReserveResult { + const decision = evaluateAgentSessionReserveOperation(state, request) + if (decision.decision === 'refused') { + throw new Error(decision.code) + } + if (decision.decision === 'replay') { + let record = requireAgentSessionRecordForReplay(state, decision.row, request.sessionId) + if (decision.row.outcome.status === 'pending' && request.handoffOperationId !== null) { + record = admitPendingAgentSessionReservationReplay(record, request) + } + return { record, disposition: 'replayed' as const, operationRow: decision.row } + } + const result = applyAgentSessionReservation(state, request, leaseTtlMs) + state.operations.set( + agentSessionOperationKey(request.operation.callerKey, request.operation.operationId), + decision.row + ) + state.records.set(result.record.sessionId, result.record) + return { ...result, operationRow: decision.row } +} diff --git a/src/main/runtime/orca-runtime-restore-structured-agent-session-tabs-once.ts b/src/main/runtime/orca-runtime-restore-structured-agent-session-tabs-once.ts index 3e544cde7e3..84383bb0729 100644 --- a/src/main/runtime/orca-runtime-restore-structured-agent-session-tabs-once.ts +++ b/src/main/runtime/orca-runtime-restore-structured-agent-session-tabs-once.ts @@ -49,6 +49,10 @@ export class OrcaRuntimeWithRestoreStructuredAgentSessionTabsOnce extends OrcaRu if (session.agent !== 'codex' && session.agent !== 'claude') { continue } + // Belt-and-braces: a session id cannot contain ':' (SESSION_ID_PATTERN in + // agent-session-record), so a prefixed key cannot reach the host's map. + // Stripped here rather than at the lookup so the tab id and the record it + // is labelled from can never disagree. let sessionId = session.sessionId while (sessionId.startsWith('agent-session:')) { sessionId = sessionId.slice('agent-session:'.length) @@ -75,10 +79,18 @@ export class OrcaRuntimeWithRestoreStructuredAgentSessionTabsOnce extends OrcaRu if (typeof host?.setSessionTabVisibility === 'function') { await host.setSessionTabVisibility(input.sessionId, true) } + // Every publication labels itself from the record, so a revealed or reopened + // chat shows the name it already has. Only the startup sweep passes a title, + // and an unchanged name never fans out, so nothing else would repair it. + const title = + input.title?.trim() || + (typeof host?.readConversationName === 'function' + ? (host.readConversationName(input.sessionId) ?? '') + : '') const existing = this.mobileSessionTabsByWorktree.get(input.workspaceId) const id = `agent-session:${input.sessionId}` if (existing?.tabs.some((tab) => tab.id === id)) { - const conversationName = input.title?.trim() + const conversationName = title if (!input.activate) { // Republishing an already-open tab is how a restored session hands over // the name it was persisted with; the rest of the snapshot is unchanged. @@ -121,7 +133,7 @@ export class OrcaRuntimeWithRestoreStructuredAgentSessionTabsOnce extends OrcaRu const tab: RuntimeMobileSessionAgentTab = { type: 'agent-session', id, - title: input.title?.trim() || defaultAgentChatLabel(input.agent), + title: title || defaultAgentChatLabel(input.agent), sessionId: input.sessionId, agent: input.agent, isActive: input.activate diff --git a/src/main/runtime/orca-runtime-structured-session-restore.test.ts b/src/main/runtime/orca-runtime-structured-session-restore.test.ts index 1ad175808b0..3861203c86e 100644 --- a/src/main/runtime/orca-runtime-structured-session-restore.test.ts +++ b/src/main/runtime/orca-runtime-structured-session-restore.test.ts @@ -462,6 +462,48 @@ describe('structured session cold restoration', () => { expect(again.snapshotVersion).toBe(settled.snapshotVersion) }) + it('labels a revealed chat with the name its record already holds', async () => { + const runtime = new OrcaRuntimeService() + // Reveal, re-open and create publish no title. Only the startup sweep did, + // and resume reports the same name so the unchanged-name short circuit means + // nothing ever repairs the label afterwards. + setStructuredAgentSessionHost({ + setSessionTabVisibility: async () => undefined, + readConversationName: () => 'Fix the lease probe' + } as never) + + await runtime.publishStructuredAgentSessionTab({ + workspaceId: 'workspace-1', + sessionId: 'revealed-codex', + agent: 'codex', + activate: true + }) + + const snapshot = await runtime.listMobileSessionTabs('id:workspace-1') + expect(snapshot.tabs[0]).toMatchObject({ + id: 'agent-session:revealed-codex', + title: 'Fix the lease probe' + }) + }) + + it('keeps the placeholder for a chat whose record holds no name', async () => { + const runtime = new OrcaRuntimeService() + setStructuredAgentSessionHost({ + setSessionTabVisibility: async () => undefined, + readConversationName: () => null + } as never) + + await runtime.publishStructuredAgentSessionTab({ + workspaceId: 'workspace-1', + sessionId: 'unnamed-codex', + agent: 'codex', + activate: true + }) + + const snapshot = await runtime.listMobileSessionTabs('id:workspace-1') + expect(snapshot.tabs[0]).toMatchObject({ title: 'Codex Chat' }) + }) + it('commits the host close when the renderer already removed the structured tab', async () => { const runtime = new OrcaRuntimeService() runtime.setNotifier({ diff --git a/src/main/runtime/structured-agent-session-runtime.ts b/src/main/runtime/structured-agent-session-runtime.ts index 819b6124939..b22ea82c867 100644 --- a/src/main/runtime/structured-agent-session-runtime.ts +++ b/src/main/runtime/structured-agent-session-runtime.ts @@ -81,7 +81,8 @@ export type StructuredAgentSessionRuntimeDeps = { onConversationName?: (input: { sessionId: string workspaceId: string - conversationName: string + /** Null when the provider cleared the name; the tab returns to its placeholder. */ + conversationName: string | null }) => void handoffTransport?: StructuredAgentSessionHandoffTransport reapOrphanChildren?: typeof stopOrphanAgentSessionChildren @@ -224,12 +225,15 @@ async function install(deps: StructuredAgentSessionRuntimeDeps): Promise Date.now(), onChanged: (sessionId, conversationName) => { - host?.republishStatus(sessionId) + // The tab snapshot is the only carrier: the status summary has no name + // field, so re-projecting it would broadcast a byte-identical summary. const workspaceId = store.getRecord(sessionId)?.location.workspaceId if (workspaceId) { deps.onConversationName?.({ sessionId, workspaceId, conversationName }) } - } + }, + onError: (scope, error) => + console.warn(`[agent-session] conversation naming failed (${scope})`, error) }) const codex = new CodexStructuredSessionAdapter({ resolveLaunch: createCodexStructuredLaunchResolver({ @@ -242,6 +246,11 @@ async function install(deps: StructuredAgentSessionRuntimeDeps): Promise void conversationNames.publish(sessionId, conversationName), + onConversationNameCleared: (sessionId) => void conversationNames.clear(sessionId), + readNamingAttempted: (sessionId) => conversationNames.read(sessionId).namingAttempted, + markNamingAttempted: (sessionId) => void conversationNames.markAttempted(sessionId), + onNamingError: (scope, error) => + console.warn(`[agent-session] conversation naming failed (${scope})`, error), onEvent: (event) => { if (event.type !== 'ended' || !('cause' in event) || event.cause !== 'unexpected-exit') { return @@ -285,6 +294,10 @@ async function install(deps: StructuredAgentSessionRuntimeDeps): Promise void conversationNames.publish(sessionId, conversationName), + readNamingState: (sessionId) => conversationNames.read(sessionId), + markNamingAttempted: (sessionId) => void conversationNames.markAttempted(sessionId), + onNamingError: (scope, error) => + console.warn(`[agent-session] conversation naming failed (${scope})`, error), ...(deps.openClaudeConnection ? { openClaudeConnection: deps.openClaudeConnection } : {}), ...(deps.readProcessStartTime ? { readProcessStartTime: deps.readProcessStartTime } : {}) }) diff --git a/src/main/runtime/structured-claude-runtime-adapter.ts b/src/main/runtime/structured-claude-runtime-adapter.ts index 79d39a7968b..61ab2a3ed7f 100644 --- a/src/main/runtime/structured-claude-runtime-adapter.ts +++ b/src/main/runtime/structured-claude-runtime-adapter.ts @@ -36,6 +36,12 @@ export type StructuredClaudeRuntimeAdapterDeps = { state: AgentSessionBackgroundTaskState | null ) => void onConversationName?: (sessionId: string, conversationName: string) => void + readNamingState?: (sessionId: string) => { + conversationName: string | null + namingAttempted: boolean + } + markNamingAttempted?: (sessionId: string) => void + onNamingError?: (scope: string, error: unknown) => void } export function createStructuredClaudeRuntimeAdapter( @@ -80,7 +86,9 @@ export function createStructuredClaudeRuntimeAdapter( : null }, ...(deps.onConversationName ? { onConversationName: deps.onConversationName } : {}), - readConversationName: (sessionId) => store.getRecord(sessionId)?.conversationName ?? null, + ...(deps.readNamingState ? { readNamingState: deps.readNamingState } : {}), + ...(deps.markNamingAttempted ? { markNamingAttempted: deps.markNamingAttempted } : {}), + ...(deps.onNamingError ? { onNamingError: deps.onNamingError } : {}), readTranscriptConversationName: async ({ providerSessionId, claudeConfigDir }) => { const transcriptPath = await resolveSessionFilePath('claude', providerSessionId, { claudeProjectsDir: join(claudeConfigDir, 'projects') diff --git a/src/shared/agent-session-record.ts b/src/shared/agent-session-record.ts index 1e0d11a4823..0d90307d45f 100644 --- a/src/shared/agent-session-record.ts +++ b/src/shared/agent-session-record.ts @@ -129,6 +129,11 @@ export type AgentSessionRecord = { /** Name the PROVIDER gave this conversation. A user's own rename lives on the * client tab and always outranks it; nothing here may overwrite that. */ conversationName?: string + /** Set once Orca has asked a provider to name this conversation. Durable on + * purpose: the session object is rebuilt on every acquisition, so an eviction + * or a restart would otherwise re-ask — re-imposing a name the user cleared, + * and paying for it again. */ + conversationNamingAttempted?: boolean launchArgs?: AgentSessionLaunchArgs lease: AgentSessionLease createdAt: number @@ -341,6 +346,8 @@ export function isAgentSessionRecord(value: unknown): value is AgentSessionRecor (record.options === undefined || isAgentSessionOptions(record.options)) && (record.conversationName === undefined || isAgentSessionConversationName(record.conversationName)) && + (record.conversationNamingAttempted === undefined || + typeof record.conversationNamingAttempted === 'boolean') && (record.launchArgs === undefined || isAgentSessionLaunchArgs(record.launchArgs)) && !Object.hasOwn(record, 'launchEnv') && isAgentSessionLease(record.lease) &&