diff --git a/src/main/codex/codex-conversation-name-generation.test.ts b/src/main/codex/codex-conversation-name-generation.test.ts index 16f4e50562f..febf0f288c1 100644 --- a/src/main/codex/codex-conversation-name-generation.test.ts +++ b/src/main/codex/codex-conversation-name-generation.test.ts @@ -98,6 +98,72 @@ describe('readCodexGeneratedTitle', () => { }) }) +describe('naming thread hardening', () => { + it('opens the throwaway thread with no approvals and no write access', async () => { + const { connection, calls } = fakeConnection() + + await run(connection, '{"title":"Fix lease probe"}') + + // The prompt embeds untrusted user text and this thread's frames never reach + // the journal, so anything the host would auto-approve would run unseen. + expect(calls.find((call) => call.method === 'thread/start')?.params).toEqual({ + cwd: '/work/repo', + ephemeral: true, + approvalPolicy: 'never', + sandbox: 'read-only' + }) + }) + + it('releases the throwaway thread with the protocol cleanup', async () => { + const { connection, calls } = fakeConnection() + + await run(connection, '{"title":"Fix lease probe"}') + + expect(calls.find((call) => call.method === 'thread/unsubscribe')?.params).toEqual({ + threadId: NAMING + }) + }) + + it('never unsubscribes the user own thread when the reply named it', async () => { + const calls: { method: string; params: Record }[] = [] + const connection: Pick = { + request: vi.fn(async (method: string, params?: Record) => { + calls.push({ method, params: params ?? {} }) + // The reply names the SESSION's thread; unsubscribing it would cut the + // user's chat off from every frame it depends on. + return method === 'thread/start' ? { thread: { id: THREAD } } : {} + }) + } + + await expect(run(connection, '{"title":"Fix lease probe"}')).resolves.toEqual({ + name: null, + settled: false + }) + expect(calls.map((call) => call.method)).not.toContain('thread/unsubscribe') + }) + + it('refuses to parse an answer far larger than any title', async () => { + // The 36-character cap is a schema request to the model, not a bound the + // host enforces on the reply. + const huge = `{"title":"${'a'.repeat(9 * 1024)}"}` + + expect(readCodexGeneratedTitle(huge)).toBeNull() + }) + + it('flattens and bounds the name before it reaches the user Codex thread', async () => { + const { connection, calls } = fakeConnection() + const sprawling = `Fix\nthe lease probe ${'x'.repeat(400)}` + + await run(connection, JSON.stringify({ title: sprawling })) + + const set = calls.find((call) => call.method === 'thread/name/set') + const name = String(set?.params.name) + expect(name).not.toContain('\n') + expect(name.length).toBeLessThanOrEqual(200) + expect(name.startsWith('Fix the lease probe ')).toBe(true) + }) +}) + describe('createCodexNamingTurnCollector', () => { it('reports a completed turn that said nothing as a DECLINE', async () => { const collector = createCodexNamingTurnCollector(60_000) diff --git a/src/main/codex/codex-conversation-name-generation.ts b/src/main/codex/codex-conversation-name-generation.ts index 3082f72870e..d5cb3bc3285 100644 --- a/src/main/codex/codex-conversation-name-generation.ts +++ b/src/main/codex/codex-conversation-name-generation.ts @@ -22,9 +22,25 @@ // takes seconds and another client may have named the thread in the meantime; a // name a person chose must never lose to one Orca inferred. +import { normalizeAgentSessionConversationName } from '../../shared/agent-session-conversation-name' import type { CodexAppServerConnection } from './codex-app-server-connection' import { readCodexThreadId, readCodexThreadName } from './codex-structured-thread-facts' +/** + * The throwaway thread runs with no approvals and no write access. + * + * The prompt embeds the user's own message text, which is untrusted, and this + * thread's frames are deliberately kept out of the journal — so any tool the + * host would auto-approve runs where the user can never see it. Orca refuses + * server requests on this thread as well, but that only covers the ones that + * ask; these two params cover the ones that do not. + */ +const NAMING_THREAD_APPROVAL_POLICY = 'never' +const NAMING_THREAD_SANDBOX = 'read-only' + +/** Past this an answer is not a title, and parsing it is wasted work. */ +const NAMING_ANSWER_MAX_BYTES = 8 * 1024 + /** 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 @@ -98,13 +114,35 @@ export type CodexNamingState = { * has. */ export function isCodexNamingFrame(state: CodexNamingState, frameThreadId: string | null): boolean { + if (isCodexNamingThread(state, frameThreadId)) { + return true + } + return ( + frameThreadId !== null && + frameThreadId !== state.threadId && + state.naming !== null && + state.namingThreadIds.size === 0 + ) +} + +/** + * The exact-match half, for the server-REQUEST path. + * + * The broad pre-id rule must not apply there. A request only arrives from a + * thread with a turn running, and the naming thread's `turn/start` is sent after + * its id is known — so inside the `thread/start` window the broad rule can only + * ever match a genuine sub-agent, whose approval request would then be refused + * with -32001 instead of reaching the user. On the notification path the same + * rule costs at most one wasted naming attempt, which is why it stays there. + */ +export function isCodexNamingThread( + state: CodexNamingState, + frameThreadId: string | null +): boolean { if (frameThreadId === null || frameThreadId === state.threadId) { return false } - if (state.namingThreadIds.has(frameThreadId)) { - return true - } - return state.naming !== null && state.namingThreadIds.size === 0 + return state.namingThreadIds.has(frameThreadId) } /** @@ -191,6 +229,11 @@ export function readCodexGeneratedTitle(answer: string | null): string | null { if (!answer) { return null } + // The 36-character cap is a request to the model, not a bound the host + // enforces: a reply that ignored the schema arrives at whatever size it likes. + if (Buffer.byteLength(answer, 'utf8') > NAMING_ANSWER_MAX_BYTES) { + return null + } let parsed: unknown try { parsed = JSON.parse(answer) @@ -277,13 +320,19 @@ export async function generateAndSetCodexConversationName( const collector = input.openNamingTurn() let result: CodexNamingTurnResult = { outcome: 'failed' } let disposableThreadId: string | null = null + let namingThreadId: string | null = null try { const opened = await connection.request( 'thread/start', - { cwd: input.cwd, ephemeral: true }, + { + cwd: input.cwd, + ephemeral: true, + approvalPolicy: NAMING_THREAD_APPROVAL_POLICY, + sandbox: NAMING_THREAD_SANDBOX + }, { timeoutMs } ) - const namingThreadId = readCodexThreadId(opened) + const openedThreadId = readCodexThreadId(opened) // No usable throwaway thread is a host that could not be asked, not a decline. // 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 @@ -292,9 +341,12 @@ export async function generateAndSetCodexConversationName( // user's chat. A reply naming some OTHER pre-existing thread of the user's // is not guarded and is not treated as a real risk: `thread/start` returns // the thread it just opened. - if (!namingThreadId || namingThreadId === input.threadId) { + if (!openedThreadId || openedThreadId === input.threadId) { return { name: null, settled: false } } + // Held for the cleanup below only once it is known NOT to be the user's own + // thread: unsubscribing that one would cut the chat off from its frames. + namingThreadId = openedThreadId 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 @@ -321,6 +373,15 @@ export async function generateAndSetCodexConversationName( result = await collector.answer } finally { input.closeNamingTurn() + // The protocol's own cleanup for a thread a client is done with. Retaining + // the id stays the load-bearing guard — a turn that timed out is never + // cancelled and can still emit — so this is best-effort on top, never a + // reason to fail the naming attempt. + if (namingThreadId) { + await connection + .request('thread/unsubscribe', { threadId: namingThreadId }, { timeoutMs }) + .catch((error: unknown) => input.onError?.('unsubscribe-naming-thread', error)) + } // 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. @@ -364,10 +425,13 @@ export async function generateAndSetCodexConversationName( if (!isCodexThreadReadablyUnnamed(current)) { return { name: null, settled: true } } - await connection.request( - 'thread/name/set', - { threadId: input.threadId, name: title }, - { timeoutMs } - ) - return { name: title, settled: true } + // Normalized through the same reader Orca's own copy goes through, so the + // name in the user's Codex history cannot be a multi-line or unbounded string + // that only their client would ever render. + const name = normalizeAgentSessionConversationName(title) + if (!name) { + return { name: null, settled: true } + } + await connection.request('thread/name/set', { threadId: input.threadId, name }, { timeoutMs }) + return { name, settled: true } } diff --git a/src/main/codex/codex-structured-session-adapter.ts b/src/main/codex/codex-structured-session-adapter.ts index ecc896eb77a..847dbf578e1 100644 --- a/src/main/codex/codex-structured-session-adapter.ts +++ b/src/main/codex/codex-structured-session-adapter.ts @@ -38,7 +38,7 @@ import { deliverCodexServerRequest, deliverCodexUnhandledFrame } from './codex-structured-provider-events' -import { isCodexNamingFrame } from './codex-conversation-name-generation' +import { isCodexNamingFrame, isCodexNamingThread } from './codex-conversation-name-generation' import { captureCodexConversationName, startCodexConversationNamingForTurn @@ -173,7 +173,10 @@ export class CodexStructuredSessionAdapter implements StructuredAgentSessionAdap request: Parameters[2] ): void { const session = this.sessions.get(sessionId) - if (session && isCodexNamingFrame(session, readCodexThreadId(request.params))) { + // Exact id only: the broad pre-id rule would refuse a SUB-AGENT's approval + // request during the `thread/start` window, since the naming thread has no + // turn running yet and cannot be the one asking. + if (session && isCodexNamingThread(session, readCodexThreadId(request.params))) { // An approval request from the naming turn would become a durable prompt in // the user's chat, for a command they never asked for, left pending forever // once the turn is abandoned. Refuse it so the turn settles instead. 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 21abdd39d90..205a9133f5f 100644 --- a/src/main/codex/codex-structured-session-conversation-name.test.ts +++ b/src/main/codex/codex-structured-session-conversation-name.test.ts @@ -129,6 +129,9 @@ function namingCodex( answer?: string existingName?: string hangNamingTurn?: boolean + /** Never answers the ephemeral `thread/start`, so naming is in flight with + * NO naming thread id known: the window the broad frame rule covers. */ + hangNamingThreadStart?: boolean /** Completes the naming turn having said nothing: a genuine model decline, * which is a different fact from prose that ignored the schema. */ declineNamingTurn?: boolean @@ -148,6 +151,9 @@ function namingCodex( request: async (method: string, params?: Record) => { calls.push({ method, params: params ?? {} }) if (method === 'thread/start' && params?.ephemeral === true) { + if (options.hangNamingThreadStart) { + return await new Promise(() => {}) + } // Leaves the naming turn in flight: the thread id is known, but nothing // ever settles the collector, which is the window sub-agents run in. if (options.hangNamingTurn) { @@ -514,6 +520,28 @@ describe('Codex sub-agent threads survive the naming window', () => { expect(JSON.stringify(events)).toContain('subagent finished its work') }) + it('prompts the user for a sub-agent approval sent during the thread/start window', async () => { + // No naming thread id exists yet, so only the broad rule could match — and + // on the request path it can only ever match a genuine sub-agent, because + // the naming thread has no turn running to ask with. + const codex = namingCodex({ hangNamingThreadStart: true }) + const { events } = await dispatchedAdapter(codex) + await settle() + + codex.connections[0]!.handlers.onServerRequest?.({ + id: 92, + method: 'item/commandExecution/requestApproval', + params: { threadId: SUBAGENT_THREAD, itemId: 'item-subagent-2', command: 'pnpm test' } + }) + await settle() + + expect(codex.replies).toEqual([]) + const prompts = events.filter((event) => (event as { type?: string }).type === 'prompt') + expect(prompts).toEqual([ + expect.objectContaining({ threadId: SUBAGENT_THREAD, codexItemId: 'item-subagent-2' }) + ]) + }) + it('prompts the user for a sub-agent approval instead of auto-refusing it', async () => { const codex = namingCodex({ hangNamingTurn: true }) const { events } = await dispatchedAdapter(codex)