From 97c1df55a0218b2dd814165369ffae357cd1db03 Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Thu, 10 Sep 2026 01:24:09 -0700 Subject: [PATCH] Report Codex thread confirmations so an unlisted model keeps its name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Withholding unlisted ids regressed Codex. On main the reader fabricated a row for an unlisted `current.model`, so the pill named it; dropping that fabrication left the name to provenance, and Codex reported none — `readCodexStructuredSessionOptions` emitted no `confirmed` ids, so `result.current.confirmed ?? []` recorded even a model the thread was genuinely running as `dispatched`. Probed against origin/main: a resumed thread on `gpt-unlisted` rendered `gpt-unlisted` there and `Model` here. That is the bug this branch fixes for Claude, recreated for Codex. `input.current` cannot answer it alone: it merges two sources. `session.options` holds a pick restored from a previous session or a write of ours the thread has not echoed, while `session.reportedOptions` has one writer — the opened or resumed thread — and is the agent's own state. Only `readLiveCodexSessionOptions` knows which answered, so it now says, and the reader passes it on. `confirmed` is an existing optional wire field whose absence already means "unconfirmed", so a host that predates this sends nothing and old clients read exactly what they read now. A model the reader substitutes because nothing was current is never confirmed: that id is our choice, not a report. --- .../codex-structured-session-options.test.ts | 46 +++++++++++++++++++ .../codex/codex-structured-session-options.ts | 25 ++++++++-- .../structured-agent-session-options.test.ts | 21 +++++++-- 3 files changed, 85 insertions(+), 7 deletions(-) diff --git a/src/main/codex/codex-structured-session-options.test.ts b/src/main/codex/codex-structured-session-options.test.ts index e3a0e2f2ca7..4917391d3ee 100644 --- a/src/main/codex/codex-structured-session-options.test.ts +++ b/src/main/codex/codex-structured-session-options.test.ts @@ -4,6 +4,7 @@ import { CodexAcquisitionWindow } from './codex-structured-acquisition-window' import { applyCodexStructuredSessionOption, readCodexStructuredSessionOptions, + readLiveCodexSessionOptions, reportedCodexThreadOptions, restoredCodexSessionOptions } from './codex-structured-session-options' @@ -142,6 +143,51 @@ describe('structured Codex session options', () => { }) }) + it('confirms a model the thread reported, but not a pick we restored', async () => { + // The display rule names an unlisted model only when the agent reported it, so the + // reader has to say which of the two answered. `session.options` is a restored pick + // or an unechoed write of ours; `reportedOptions` is the opened thread's own state. + const request = vi.fn(async () => ({ + data: [{ model: 'gpt-live', displayName: 'GPT Live', isDefault: true }], + nextCursor: null + })) + const session = ( + options: Map, + reported: Record + ): CodexSession => + ({ connection: { request }, options, reportedOptions: reported }) as unknown as CodexSession + + await expect( + readLiveCodexSessionOptions(session(new Map(), { model: 'gpt-unlisted' }), undefined) + ).resolves.toMatchObject({ current: { model: 'gpt-unlisted', confirmed: ['model'] } }) + + const restored = await readLiveCodexSessionOptions( + session(new Map([['model', 'gpt-stale']]), {}), + undefined + ) + expect(restored.current).toEqual({ model: 'gpt-stale' }) + expect(restored.current.confirmed).toBeUndefined() + }) + + it('does not confirm a model it substituted rather than read', async () => { + // With nothing current the reader picks the default; that is our choice, not a report. + const request = vi.fn(async () => ({ + data: [{ model: 'gpt-live', displayName: 'GPT Live', isDefault: true }], + nextCursor: null + })) + + await expect( + readCodexStructuredSessionOptions({ + connection: { request } as never, + current: {}, + confirmed: ['model'] + }) + ).resolves.toEqual({ + models: [{ id: 'gpt-live', label: 'GPT Live', isDefault: true, efforts: [] }], + current: { model: 'gpt-live' } + }) + }) + it('hydrates current values from thread start or resume', () => { expect( reportedCodexThreadOptions({ diff --git a/src/main/codex/codex-structured-session-options.ts b/src/main/codex/codex-structured-session-options.ts index cca31866a70..a6985f0d143 100644 --- a/src/main/codex/codex-structured-session-options.ts +++ b/src/main/codex/codex-structured-session-options.ts @@ -78,6 +78,9 @@ function modelOption(value: unknown): AgentSessionModelOption | null { export async function readCodexStructuredSessionOptions(input: { connection: Pick current: { model?: string; effort?: string } + /** Ids in `current` that came from the thread's own state rather than from a value we + * wrote. Only the caller knows which answered, so it must say. */ + confirmed?: readonly string[] timeoutMs?: number }): Promise { const models: AgentSessionModelOption[] = [] @@ -108,9 +111,15 @@ export async function readCodexStructuredSessionOptions(input: { if (!model) { throw new Error('codex app-server returned no available models') } + // Never pass on a confirmation for a model this function substituted rather than read. + const confirmed = input.confirmed?.filter((id) => id !== 'model' || model === input.current.model) return { models, - current: { model, ...(input.current.effort ? { effort: input.current.effort } : {}) } + current: { + model, + ...(input.current.effort ? { effort: input.current.effort } : {}), + ...(confirmed?.length ? { confirmed } : {}) + } } } @@ -127,11 +136,21 @@ export function readLiveCodexSessionOptions( session: CodexSession, timeoutMs: number | undefined ): Promise { - const model = session.options.get('model') ?? session.reportedOptions.model - const effort = session.options.get('effort') ?? session.reportedOptions.effort + const pendingModel = session.options.get('model') + const pendingEffort = session.options.get('effort') + const model = pendingModel ?? session.reportedOptions.model + const effort = pendingEffort ?? session.reportedOptions.effort + // Why: `session.options` holds a restored pick or a write of ours the thread has not + // echoed — neither is the agent speaking. A value that fell through to the opened + // thread's own state is, which is what lets an unlisted model still be named. + const confirmed = [ + ...(!pendingModel && model ? ['model'] : []), + ...(!pendingEffort && effort ? ['effort'] : []) + ] return readCodexStructuredSessionOptions({ connection: session.connection, current: { ...(model ? { model } : {}), ...(effort ? { effort } : {}) }, + ...(confirmed.length > 0 ? { confirmed } : {}), timeoutMs }) } diff --git a/src/shared/structured-agent-session-options.test.ts b/src/shared/structured-agent-session-options.test.ts index ec2444bd696..a7ebd1a42cf 100644 --- a/src/shared/structured-agent-session-options.test.ts +++ b/src/shared/structured-agent-session-options.test.ts @@ -97,10 +97,8 @@ describe('structured agent session options', () => { { models: [], current: { model: 'gpt-5.9-secret' } } ) - // Codex's reader emits no `confirmed` ids, so even a model the thread genuinely runs - // records as `dispatched`, not `reported` — which is why the name stays withheld here - // while the equivalent Claude case is named. Pinned so that when the reader starts - // confirming, this reddens and the name can be turned on deliberately. + // Unconfirmed: a pick restored from a previous session, which the thread has not + // echoed. An empty list must not promote it to a name it was never owed. expect(state.record.model).toEqual({ value: 'gpt-5.9-secret', source: 'dispatched' }) const snapshot = structuredAgentSessionOptionSnapshot(state) expect(snapshot.map((descriptor) => descriptor.id)).toEqual(['model']) @@ -109,6 +107,21 @@ describe('structured agent session options', () => { expect(model.kind.type === 'select' ? model.kind.choices : null).toEqual([]) }) + it('names the model an empty-listing provider reported, without offering it', () => { + // The regression this rule exists to prevent: a thread whose `model/list` came back + // empty still runs a model and says so, so the pill must name it rather than blank. + const state = applyStructuredAgentSessionOptions( + createStructuredAgentSessionOptionState('codex'), + CODEX_SESSION_OPTION_CATALOG, + { models: [], current: { model: 'gpt-5.9-secret', confirmed: ['model'] } } + ) + + const model = structuredAgentSessionOptionSnapshot(state)[0] + expect(model).toMatchObject({ valueSource: 'reported' }) + expect(model.kind.type === 'select' ? model.kind.currentValue : null).toBe('gpt-5.9-secret') + expect(model.kind.type === 'select' ? model.kind.choices : null).toEqual([]) + }) + it('projects live options as directly settable descriptors', () => { const state = applyStructuredAgentSessionOptions( createStructuredAgentSessionOptionState('codex'),