fix(native-chat): harden the Codex naming thread and narrow its request gate

Open the throwaway naming thread with approvalPolicy 'never' and sandbox
'read-only'. The prompt embeds untrusted user message text and the thread's
frames are diverted from the journal, so any tool the host would auto-approve
ran where the user could never see it; refusing server requests only covered
the tools that ask.

Require an exact naming-thread id on the server-REQUEST path. During the
thread/start window no naming thread has a turn running, so the broad pre-id
rule could only ever match a genuine sub-agent, whose approval request was
auto-refused with -32001 instead of reaching the user.

Also caps the structured answer before parsing it, normalizes the generated
name before it is written to the user's real thread, and releases the
throwaway thread with the protocol's own thread/unsubscribe.
This commit is contained in:
Merge Sim
2026-09-07 11:25:12 -07:00
parent c868b9f05e
commit 97830d40a1
4 changed files with 176 additions and 15 deletions
@@ -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<string, unknown> }[] = []
const connection: Pick<CodexAppServerConnection, 'request'> = {
request: vi.fn(async (method: string, params?: Record<string, unknown>) => {
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)
@@ -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 }
}
@@ -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<typeof deliverCodexServerRequest>[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.
@@ -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<string, unknown>) => {
calls.push({ method, params: params ?? {} })
if (method === 'thread/start' && params?.ephemeral === true) {
if (options.hangNamingThreadStart) {
return await new Promise<never>(() => {})
}
// 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)