From 38cff6316060de3975e260200171a94fd6819e77 Mon Sep 17 00:00:00 2001 From: Badcuban <108198679+badcuban@users.noreply.github.com> Date: Sat, 14 Nov 2026 14:07:39 -0500 Subject: [PATCH 1/3] feat(codex): the gateway's model is in the picker, and titles go through the gateway When Codex runs on a gateway (its config's `model_provider` is not OpenAI), the model picker offered only OpenAI's models and started new threads on one the gateway may not serve, and titles and commit messages written by Codex went to OpenAI: those one-shot `codex exec` runs leave out the user's config on purpose (so their MCP servers and notify hooks don't fire for a title), and the gateway went with it. - The status check reads Codex's config when the default provider is not OpenAI. The model it names is in the model list as the one default, shown once even when the user had added it by hand. - One-shot runs are handed the gateway alone as `-c` overrides, beside `--ignore-user-config`. A plain token or header in the provider's definition travels in the run's environment (through `env_key` and `env_http_headers`), never in its arguments. - No routing where it can't be done safely, which leaves the run on OpenAI as before: a secret only arguments could carry (a credential in the address or `query_params`, or among the `auth` command's arguments), and a Codex that starts through cmd.exe, where the settings' quotes could act as commands. - A run the gateway refuses for its model is tried once more on the config's model: the built-in pick for titles is an OpenAI model. - A run asked for before the instance's first status check ends waits for it, up to 20 seconds. A remembered gateway survives a check that could not ask Codex, lapses after 15 minutes unconfirmed, and is forgotten at once when Codex says it is on OpenAI. Not included: a Codex agent answering on the side in a room still runs on OpenAI with the borrowed ChatGPT sign-in. --- apps/server/src/guide/topics.ts | 2 +- .../src/provider/Drivers/CodexDriver.ts | 25 +- .../src/provider/Layers/CodexProvider.ts | 259 +++++++++++++++--- .../provider/Layers/ProviderRegistry.test.ts | 205 ++++++++++++++ .../provider/codexModelProviderLaunch.test.ts | 158 +++++++++++ .../src/provider/codexModelProviderLaunch.ts | 257 +++++++++++++++++ .../src/provider/providerStatusCache.test.ts | 40 +++ .../src/provider/providerStatusCache.ts | 11 +- .../CodexTextGeneration.test.ts | 123 ++++++++- .../src/textGeneration/CodexTextGeneration.ts | 134 +++++++-- .../settings/ProviderInstanceCard.test.ts | 18 ++ .../settings/ProviderInstanceCard.tsx | 23 +- docs/providers/codex.md | 21 +- 13 files changed, 1198 insertions(+), 78 deletions(-) create mode 100644 apps/server/src/provider/codexModelProviderLaunch.test.ts create mode 100644 apps/server/src/provider/codexModelProviderLaunch.ts diff --git a/apps/server/src/guide/topics.ts b/apps/server/src/guide/topics.ts index d8d6b5064..ff4cc1877 100644 --- a/apps/server/src/guide/topics.ts +++ b/apps/server/src/guide/topics.ts @@ -250,7 +250,7 @@ A Claude sign-in made with Claude's own sign-in (not an API key or a long-lived When a Codex ChatGPT sign-in belongs to more than one workspace (a personal one and a company's, say), the Codex row shows the one in use, and its Account & Sign-in has Workspace: pick another and Switch. Switching first stops that Codex's running chats and holds new ones until it's done. A workspace behind company sign-on opens a sign-in page; Cancel keeps the current workspace. A chat whose data the old workspace keeps in another region may not continue after a switch; start a new thread for it. It needs Codex 0.167 or later. -When Codex's own config points it at a gateway or another model provider instead of OpenAI, the Codex row says Runs on and the provider's name, and Account & Sign-in has a Model provider line. Threadlines only shows it; change it in Codex's config. A ChatGPT sign-in beside it keeps its email, workspace and Sign out. It needs Codex 0.168 or later. +When Codex's own config points it at a gateway or another model provider instead of OpenAI, the Codex row says Runs on and the provider's name, and Account & Sign-in has a Model provider line. Threadlines only shows it; change it in Codex's config. A ChatGPT sign-in beside it keeps its email, workspace and Sign out. The model Codex's config names is in the model picker and is the default for new threads, and titles and commit messages written by Codex go through the gateway too. A Codex agent answering on the side in a room still runs on OpenAI. It needs Codex 0.168 or later. When OpenAI asks a ChatGPT account to do something about its security, the Codex row says Security notice, and Account & Sign-in shows OpenAI's words with a button that opens the page on OpenAI's site. It needs Codex 0.169 or later. diff --git a/apps/server/src/provider/Drivers/CodexDriver.ts b/apps/server/src/provider/Drivers/CodexDriver.ts index 711dc024d..63ddddcd4 100644 --- a/apps/server/src/provider/Drivers/CodexDriver.ts +++ b/apps/server/src/provider/Drivers/CodexDriver.ts @@ -24,6 +24,7 @@ import { CodexSettings, ProviderDriverKind, type ServerProvider } from "@threadlines/contracts"; import * as Clock from "effect/Clock"; import * as DateTime from "effect/DateTime"; +import * as Deferred from "effect/Deferred"; import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; import * as FileSystem from "effect/FileSystem"; @@ -45,6 +46,8 @@ import { makeCodexAdapter } from "../Layers/CodexAdapter.ts"; import { freshOrCurrentSnapshot } from "../freshProviderSnapshot.ts"; import { checkCodexProviderStatus, + currentCodexModelProvider, + type CodexModelProviderMemory, type CodexSecurityNoticeMemory, type CodexWorkspaceMemory, consumeCodexRateLimitResetCredit, @@ -83,6 +86,12 @@ const decodeCodexSettings = Schema.decodeSync(CodexSettings); const DRIVER_KIND = ProviderDriverKind.make("codex"); const SNAPSHOT_REFRESH_INTERVAL = Duration.minutes(5); +/** + * How long a title or commit message waits for the instance's first status + * check, which normally takes a second or two. Past it the run goes ahead + * without the check's answer rather than hold the user's commit. + */ +const FIRST_CHECK_WAIT = Duration.seconds(20); const WINDOWS_NATIVE_INSTALL = makeWindowsNativeInstaller({ url: "https://chatgpt.com/codex/install.ps1", lockKey: "codex-native-win32", @@ -193,7 +202,19 @@ export const CodexDriver: ProviderDriver = { // here; the registry only has to worry about snapshot-build and // spawner-availability failures surfaced from `checkCodexProviderStatus` // below. - const textGeneration = yield* makeCodexTextGeneration(effectiveConfig, processEnv); + // Filled in by each status check; titles and commit messages read it + // at the moment they run. The first check starts with the instance, and + // a run asked for before it ends waits for it: without its answer the + // run would go to OpenAI, gateway or not. + const modelProviderMemory: CodexModelProviderMemory = {}; + const firstCheckEnded = yield* Deferred.make(); + const textGeneration = yield* makeCodexTextGeneration(effectiveConfig, processEnv, { + modelProviderLaunch: Deferred.await(firstCheckEnded).pipe( + Effect.timeoutOption(FIRST_CHECK_WAIT), + Effect.andThen(Clock.currentTimeMillis), + Effect.map((nowMs) => currentCodexModelProvider(modelProviderMemory, nowMs)?.launch), + ), + }); // Build a managed snapshot whose settings never change — mutations come // in as instance rebuilds from the registry rather than in-place @@ -207,9 +228,11 @@ export const CodexDriver: ProviderDriver = { processEnv, workspaceMemory, securityNoticeMemory, + modelProviderMemory, ).pipe( Effect.map(stampIdentity), Effect.provideService(ChildProcessSpawner.ChildProcessSpawner, spawner), + Effect.ensuring(Deferred.succeed(firstCheckEnded, undefined)), ); const snapshot = yield* makeManagedServerProvider({ get maintenanceCapabilities() { diff --git a/apps/server/src/provider/Layers/CodexProvider.ts b/apps/server/src/provider/Layers/CodexProvider.ts index 1b3e08c20..a30d36e01 100644 --- a/apps/server/src/provider/Layers/CodexProvider.ts +++ b/apps/server/src/provider/Layers/CodexProvider.ts @@ -1,3 +1,4 @@ +import * as Clock from "effect/Clock"; import * as DateTime from "effect/DateTime"; import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; @@ -38,6 +39,10 @@ import { createModelCapabilities } from "@threadlines/shared/model"; import { isCommandAvailable } from "@threadlines/shared/shell"; import { DEFAULT_CODEX_SERVICE_TIER_SELECTION } from "../../codexServiceTier.ts"; import { CODEX_APP_SERVER_ARGS, codexAppServerCommandOptions } from "../codexAppServerArgs.ts"; +import { + codexModelProviderLaunch, + type CodexModelProviderLaunch, +} from "../codexModelProviderLaunch.ts"; import { detectProviderBinary } from "../providerDetection.ts"; import { AUTH_PROBE_TIMEOUT_MS, @@ -70,6 +75,12 @@ export interface CodexAppServerProviderSnapshot { readonly tokenUsage?: CodexSchema.V2GetAccountTokenUsageResponse; /** Codex's model providers and which one new chats use (`modelProvider/list`). */ readonly modelProviders?: CodexSchema.V2ModelProviderListResponse; + /** + * What Codex's config says about the provider its chats run on, when that + * is not OpenAI (`config/read`). Absent on OpenAI, or when Codex gave no + * answer. + */ + readonly gateway?: CodexGatewayConfig; /** * What OpenAI asks this account to do about its security * (`account/securitySetup/read`). Absent when Codex gave no answer; a @@ -81,6 +92,24 @@ export interface CodexAppServerProviderSnapshot { readonly skills: ReadonlyArray; } +/** The parts of Codex's config that matter when its chats run on another provider. */ +export interface CodexGatewayConfig { + readonly providerId: string; + /** + * Whether Codex answered with its config. When it did not, the model and + * definition below are unknown, not absent. + */ + readonly configRead: boolean; + /** The config's `model`: what Codex itself uses on that provider. */ + readonly model?: string; + /** + * The provider's entry in the config's `model_providers`, as Codex reports + * it. It can hold secrets, so it stays on the server. Absent for a provider + * Codex has built in. + */ + readonly definition?: unknown; +} + const REASONING_EFFORT_LABELS: Record = { none: "None", minimal: "Minimal", @@ -772,42 +801,42 @@ const probeCodexAppServerProvider = Effect.fn("probeCodexAppServerProvider")(fun const versionMatch = initialize.userAgent.match(/\/([^\s]+)/); const version = versionMatch ? versionMatch[1] : undefined; - const accountResponse = yield* client.request("account/read", {}); + // The provider list is read beside the account, ahead of the signed-out + // return below: a Codex moved back to OpenAI while signed out must still + // say so, or the gateway it left would stay remembered. + const [accountResponse, modelProviders] = yield* Effect.all( + [client.request("account/read", {}), readCodexModelProviders(client)], + { concurrency: "unbounded" }, + ); if (!accountResponse.account && accountResponse.requiresOpenaiAuth) { return { account: accountResponse, + ...(modelProviders ? { modelProviders } : {}), version, models: appendCustomCodexModels([], input.customModels ?? []), skills: [], } satisfies CodexAppServerProviderSnapshot; } - const [ - skillsResponse, - models, - rateLimits, - tokenUsage, - accountSessions, - modelProviders, - securitySetup, - ] = yield* Effect.all( - [ - client.request("skills/list", { - cwds: [input.cwd], - }), - requestAllCodexModels(client), - client.request("account/rateLimits/read", null).pipe(Effect.orElseSucceed(() => undefined)), - client.request("account/usage/read", {}).pipe(Effect.orElseSucceed(() => undefined)), - accountResponse.account?.type === "chatgpt" - ? readCodexAccountSessions(client) - : Effect.succeed(undefined), - readCodexModelProviders(client), - accountResponse.account?.type === "chatgpt" - ? readCodexSecuritySetup(client) - : Effect.succeed(undefined), - ], - { concurrency: "unbounded" }, - ); + const [skillsResponse, models, rateLimits, tokenUsage, accountSessions, gateway, securitySetup] = + yield* Effect.all( + [ + client.request("skills/list", { + cwds: [input.cwd], + }), + requestAllCodexModels(client), + client.request("account/rateLimits/read", null).pipe(Effect.orElseSucceed(() => undefined)), + client.request("account/usage/read", {}).pipe(Effect.orElseSucceed(() => undefined)), + accountResponse.account?.type === "chatgpt" + ? readCodexAccountSessions(client) + : Effect.succeed(undefined), + readCodexGatewayConfig(client, modelProviders), + accountResponse.account?.type === "chatgpt" + ? readCodexSecuritySetup(client) + : Effect.succeed(undefined), + ], + { concurrency: "unbounded" }, + ); return { account: accountResponse, @@ -816,6 +845,7 @@ const probeCodexAppServerProvider = Effect.fn("probeCodexAppServerProvider")(fun ...(tokenUsage ? { tokenUsage } : {}), ...(modelProviders ? { modelProviders } : {}), ...(securitySetup ? { securitySetup } : {}), + ...(gateway ? { gateway } : {}), version, models: appendCustomCodexModels(models, input.customModels ?? []), skills: parseCodexSkillsListResponse(skillsResponse, input.cwd), @@ -872,6 +902,166 @@ export const readCodexSecuritySetup = (client: CodexClient.CodexAppServerClientS Effect.orElseSucceed(() => undefined), ); +/** Codex's default model provider, when it is not OpenAI's own. */ +function codexNonOpenAiDefaultProvider( + providers: CodexSchema.V2ModelProviderListResponse | undefined, +) { + const provider = providers?.data.find((entry) => entry.id === providers.defaultProviderId); + return provider && provider.settings.type !== "openai" && provider.id.trim() !== "" + ? provider + : undefined; +} + +/** + * What Codex's config says about the provider its chats run on: the model it + * names, and the provider's own definition. `config` is the answer to + * `config/read`, or undefined when Codex gave none. Undefined on OpenAI. + */ +export function codexGatewayConfigFromReads( + providers: CodexSchema.V2ModelProviderListResponse | undefined, + config: CodexSchema.V2ConfigReadResponse | undefined, +): CodexGatewayConfig | undefined { + const provider = codexNonOpenAiDefaultProvider(providers); + if (!provider) return undefined; + if (!config) return { providerId: provider.id, configRead: false }; + const model = optionalString(config.config.model); + const definitions: unknown = config.config.model_providers; + const definition = + typeof definitions === "object" && definitions !== null + ? (definitions as { readonly [id: string]: unknown })[provider.id] + : undefined; + return { + providerId: provider.id, + configRead: true, + ...(model ? { model } : {}), + ...(definition !== undefined && definition !== null ? { definition } : {}), + }; +} + +/** Reads Codex's config only when its chats run somewhere other than OpenAI. */ +const readCodexGatewayConfig = ( + client: CodexClient.CodexAppServerClientShape, + providers: CodexSchema.V2ModelProviderListResponse | undefined, +) => + codexNonOpenAiDefaultProvider(providers) === undefined + ? Effect.succeed(undefined) + : client.request("config/read", {}).pipe( + Effect.timeoutOption(CODEX_OPTIONAL_READ_TIMEOUT), + Effect.map(Option.getOrUndefined), + Effect.orElseSucceed(() => undefined), + Effect.map((config) => codexGatewayConfigFromReads(providers, config)), + ); + +/** + * The model list with the model Codex's config names, as the default. On a + * gateway Codex still lists OpenAI's models and marks one of them default, + * which the gateway may not serve; the config's model is the one known to + * work there. Added when the list lacks it, shown when the list hides it, + * and no longer a custom model when the user had added it by hand. + */ +export function withCodexConfigModel( + models: ReadonlyArray, + configModel: string | undefined, +): ReadonlyArray { + const slug = configModel?.trim(); + if (!slug) return models; + const withoutDefault = models.map(({ isDefault: _isDefault, ...model }) => model); + const index = withoutDefault.findIndex((model) => model.slug === slug); + if (index >= 0) { + return withoutDefault.map((model, position) => { + if (position !== index) return model; + // One the user also added by hand is Codex's own from here on: only a + // model that is not custom can be the default. + const { isHidden: _isHidden, ...shown } = model; + return { ...shown, isCustom: false, isDefault: true }; + }); + } + return [ + ...withoutDefault, + { + slug, + name: slug, + description: "The model set in Codex's config.", + isCustom: false, + isDefault: true, + capabilities: models.find((model) => model.capabilities)?.capabilities ?? null, + }, + ]; +} + +/** + * What the last status checks learned about the provider Codex's chats run + * on, for what reads it between checks: one-shot runs (titles, commit + * messages) and the model list. One per Codex instance. Read it through + * `currentCodexModelProvider`, which lets it lapse. + */ +export interface CodexModelProviderMemory { + last?: + | { + readonly providerId: string; + /** When Codex last named this provider as its default. */ + readonly confirmedAtMs: number; + readonly model?: string | undefined; + /** Absent when the config was never read, or holds a secret only arguments could carry. */ + readonly launch?: CodexModelProviderLaunch | undefined; + } + | undefined; +} + +/** + * How long a gateway stays remembered without Codex naming it again: three + * scheduled checks. Past it, a Codex that can no longer be asked (an older + * version put back, a check that keeps failing) is not assumed to still be on + * a gateway the user may have removed. + */ +const CODEX_MODEL_PROVIDER_MEMORY_MS = 15 * 60_000; + +/** The remembered gateway, while Codex named it recently enough to go by. */ +export function currentCodexModelProvider( + memory: CodexModelProviderMemory, + nowMs: number, +): CodexModelProviderMemory["last"] { + const last = memory.last; + if (last && nowMs - last.confirmedAtMs > CODEX_MODEL_PROVIDER_MEMORY_MS) { + memory.last = undefined; + } + return memory.last; +} + +/** + * The gateway a check goes by, remembered for the next one. A check that + * could not ask Codex which providers it has says nothing, so what was known + * stands for a while; one that got the provider but not its config keeps + * what was known of that same provider. Codex's own answer that it is on + * OpenAI forgets it at once. + */ +function codexGatewayForCheck( + snapshot: Pick, + memory: CodexModelProviderMemory, + environment: NodeJS.ProcessEnv, + nowMs: number, +): CodexModelProviderMemory["last"] { + if (snapshot.modelProviders === undefined) return currentCodexModelProvider(memory, nowMs); + const gateway = snapshot.gateway; + if (!gateway) { + memory.last = undefined; + } else if (gateway.configRead) { + memory.last = { + providerId: gateway.providerId, + confirmedAtMs: nowMs, + model: gateway.model, + launch: codexModelProviderLaunch({ ...gateway, environment }), + }; + } else { + memory.last = { + ...(memory.last?.providerId === gateway.providerId ? memory.last : {}), + providerId: gateway.providerId, + confirmedAtMs: nowMs, + }; + } + return memory.last; +} + /** * Where new Codex chats run, when that is not OpenAI: the default model * provider of the user's Codex config, such as a company gateway. None for @@ -885,10 +1075,9 @@ export function codexModelProviderForCheck( if (!providers) return undefined; const accountType = account.account?.type; if (accountType === "amazonBedrock" || accountType === "gcp") return undefined; - const provider = providers.data.find((entry) => entry.id === providers.defaultProviderId); - if (!provider || provider.settings.type === "openai") return undefined; + const provider = codexNonOpenAiDefaultProvider(providers); + if (!provider) return undefined; const id = provider.id.trim(); - if (!id) return undefined; return { id, name: singleLine(provider.name, MAX_MODEL_PROVIDER_NAME) ?? id }; } @@ -1209,6 +1398,8 @@ export const checkCodexProviderStatus = Effect.fn("checkCodexProviderStatus")(fu workspaceMemory?: CodexWorkspaceMemory, /** The instance's last security notice, kept the same way. */ securityNoticeMemory?: CodexSecurityNoticeMemory, + /** What the instance knows of the provider Codex is on, kept between checks. */ + modelProviderMemory: CodexModelProviderMemory = {}, ): Effect.fn.Return< ServerProviderDraft, ServerSettingsError, @@ -1294,6 +1485,12 @@ export const checkCodexProviderStatus = Effect.fn("checkCodexProviderStatus")(fu } const snapshot = probeResult.success.value; + const gateway = codexGatewayForCheck( + snapshot, + modelProviderMemory, + environment, + yield* Clock.currentTimeMillis, + ); const accountStatus = accountProbeStatus(snapshot, workspaceMemory, securityNoticeMemory); const accountUsage = normalizeCodexAccountUsage( snapshot.rateLimits, @@ -1305,7 +1502,7 @@ export const checkCodexProviderStatus = Effect.fn("checkCodexProviderStatus")(fu presentation: CODEX_PRESENTATION, enabled: codexSettings.enabled, checkedAt, - models: snapshot.models, + models: withCodexConfigModel(snapshot.models, gateway?.model), skills: snapshot.skills, probe: { installed: true, diff --git a/apps/server/src/provider/Layers/ProviderRegistry.test.ts b/apps/server/src/provider/Layers/ProviderRegistry.test.ts index a1e58db7e..42191a94a 100644 --- a/apps/server/src/provider/Layers/ProviderRegistry.test.ts +++ b/apps/server/src/provider/Layers/ProviderRegistry.test.ts @@ -39,7 +39,10 @@ import { applyServerSettingsPatch } from "@threadlines/shared/serverSettings"; import { checkCodexProviderStatus, parseCodexModelListResponse, + codexGatewayConfigFromReads, + withCodexConfigModel, type CodexAppServerProviderSnapshot, + type CodexModelProviderMemory, type CodexSecurityNoticeMemory, type CodexWorkspaceMemory, } from "./CodexProvider.ts"; @@ -524,6 +527,208 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsService.layerTest(), T }), ); + it("makes the model Codex's config names the one default on a gateway", () => { + const catalog = [ + { slug: "gpt-6.1-sol", name: "GPT-6.1-Sol", isCustom: false, isDefault: true }, + { slug: "gpt-6-astra", name: "GPT-6-Astra", isCustom: false, isHidden: true }, + ].map((model) => ({ ...model, capabilities: null })); + + // Not in Codex's list: added, and the only default. + const added = withCodexConfigModel(catalog, " acme-coder-1 "); + assert.deepStrictEqual( + added.map((model) => [model.slug, model.isDefault === true, model.isCustom]), + [ + ["gpt-6.1-sol", false, false], + ["gpt-6-astra", false, false], + ["acme-coder-1", true, false], + ], + ); + + // Already listed, even hidden: shown, and the only default. + const listed = withCodexConfigModel(catalog, "gpt-6-astra"); + assert.deepStrictEqual( + listed.map((model) => [model.slug, model.isDefault === true, model.isHidden === true]), + [ + ["gpt-6.1-sol", false, false], + ["gpt-6-astra", true, false], + ], + ); + + // Added by hand before: Codex's own from here on, so it can be the default. + const custom = withCodexConfigModel( + [ + ...catalog, + { slug: "acme-coder-1", name: "acme-coder-1", isCustom: true, capabilities: null }, + ], + "acme-coder-1", + ); + assert.deepStrictEqual( + custom + .filter((model) => model.isDefault === true) + .map((model) => [model.slug, model.isCustom]), + [["acme-coder-1", false]], + ); + + // No model in the config: Codex's own list stands. + assert.strictEqual(withCodexConfigModel(catalog, undefined), catalog); + }); + + it("reads the gateway's model and definition out of Codex's config", () => { + const providers = { + canChangeDefault: true, + defaultProviderId: "acme", + data: [ + { + id: "acme", + name: "Acme Gateway", + canEdit: true, + settings: { + type: "customGateway" as const, + auth: { mode: "externallyConfigured" as const, envKey: null }, + }, + }, + { id: "openai", name: "OpenAI", canEdit: false, settings: { type: "openai" as const } }, + ], + }; + const definition = { + name: "Acme Gateway", + base_url: "https://llm.acme.example/v1", + env_key: null, + experimental_bearer_token: "sk-secret-token", + }; + // As `config/read` answers: the keys Codex's schema names, and the rest as JSON. + const config = { + config: { + model: "acme-coder-1", + model_provider: "acme", + model_providers: { acme: definition }, + }, + origins: {}, + }; + + assert.deepStrictEqual(codexGatewayConfigFromReads(providers, config), { + providerId: "acme", + configRead: true, + model: "acme-coder-1", + definition, + }); + // No answer from `config/read`: the provider is known, the rest is not. + assert.deepStrictEqual(codexGatewayConfigFromReads(providers, undefined), { + providerId: "acme", + configRead: false, + }); + // On OpenAI, or with no provider list at all, there is no gateway. + assert.strictEqual( + codexGatewayConfigFromReads({ ...providers, defaultProviderId: "openai" }, config), + undefined, + ); + assert.strictEqual(codexGatewayConfigFromReads(undefined, config), undefined); + }); + + it.effect("remembers the gateway until Codex itself says it is on OpenAI", () => + Effect.gen(function* () { + const modelProviderMemory: CodexModelProviderMemory = {}; + const check = (probe: Parameters[1]) => + checkCodexProviderStatus( + defaultCodexSettings, + probe, + {}, + undefined, + undefined, + modelProviderMemory, + ); + const providers = (defaultProviderId: string) => ({ + canChangeDefault: true, + defaultProviderId, + data: [ + { + id: "acme", + name: "Acme Gateway", + canEdit: true, + settings: { + type: "customGateway" as const, + auth: { mode: "environmentVariable" as const, envKey: "ACME_API_KEY" }, + }, + }, + { + id: "openai", + name: "OpenAI", + canEdit: false, + settings: { type: "openai" as const }, + }, + ], + }); + const defaultModels = (status: Pick) => + status.models.filter((model) => model.isDefault === true).map((model) => model.slug); + const onGateway = makeCodexProbeSnapshot({ + modelProviders: providers("acme"), + gateway: { + providerId: "acme", + configRead: true, + model: "acme-coder-1", + definition: { base_url: "https://llm.acme.example/v1", env_key: "ACME_API_KEY" }, + }, + }); + + const first = yield* check(() => Effect.succeed(onGateway)); + assert.deepStrictEqual(defaultModels(first), ["acme-coder-1"]); + assert.deepStrictEqual(modelProviderMemory.last?.launch?.args, [ + "-c", + 'model_provider="acme"', + "-c", + 'model_providers={"acme" = {"base_url" = "https://llm.acme.example/v1", "env_key" = "ACME_API_KEY"}}', + ]); + + // Codex did not say which providers it has: nothing is known to have changed. + const noProviderList = yield* check(() => Effect.succeed(makeCodexProbeSnapshot())); + assert.deepStrictEqual(defaultModels(noProviderList), ["acme-coder-1"]); + assert.strictEqual(modelProviderMemory.last?.launch?.providerId, "acme"); + + // It named the gateway but did not answer with its config: what was + // known of that gateway stands. + const noConfig = yield* check(() => + Effect.succeed( + makeCodexProbeSnapshot({ + modelProviders: providers("acme"), + gateway: { providerId: "acme", configRead: false }, + }), + ), + ); + assert.deepStrictEqual(defaultModels(noConfig), ["acme-coder-1"]); + assert.strictEqual(modelProviderMemory.last?.launch?.providerId, "acme"); + + // A check that could not reach Codex at all says nothing either. + yield* check(() => + Effect.fail( + new CodexErrors.CodexAppServerSpawnError({ command: "codex", cause: "missing" }), + ), + ); + assert.strictEqual(modelProviderMemory.last?.launch?.providerId, "acme"); + + // Codex's own answer that it is on OpenAI forgets the gateway. + const onOpenAi = yield* check(() => + Effect.succeed(makeCodexProbeSnapshot({ modelProviders: providers("openai") })), + ); + assert.strictEqual(modelProviderMemory.last, undefined); + assert.strictEqual( + onOpenAi.models.some((model) => model.slug === "acme-coder-1"), + false, + ); + + // A gateway Codex has not named for a while is not assumed to be + // there still: the user may have removed it behind a Codex that can + // no longer be asked. + yield* check(() => Effect.succeed(onGateway)); + yield* TestClock.adjust("16 minutes"); + const lapsed = yield* check(() => Effect.succeed(makeCodexProbeSnapshot())); + assert.strictEqual(modelProviderMemory.last, undefined); + assert.strictEqual( + lapsed.models.some((model) => model.slug === "acme-coder-1"), + false, + ); + }), + ); + it.effect("shows OpenAI's security notice only to the account it is for", () => Effect.gen(function* () { const securityNoticeMemory: CodexSecurityNoticeMemory = {}; diff --git a/apps/server/src/provider/codexModelProviderLaunch.test.ts b/apps/server/src/provider/codexModelProviderLaunch.test.ts new file mode 100644 index 000000000..8e0c1c8b8 --- /dev/null +++ b/apps/server/src/provider/codexModelProviderLaunch.test.ts @@ -0,0 +1,158 @@ +import { describe, expect, it } from "vite-plus/test"; + +import { codexModelProviderLaunch } from "./codexModelProviderLaunch.ts"; + +describe("codexModelProviderLaunch", () => { + it("hands over the provider's definition as one table, keyed by its quoted id", () => { + const launch = codexModelProviderLaunch({ + providerId: "acme.gw v2", + model: "acme-coder-1", + definition: { + name: 'Acme "GW"', + base_url: "https://llm.acme.example/v1", + wire_api: "responses", + env_key: "ACME_API_KEY", + model_catalog_url: null, + query_params: { "api-version": "2026-01-01" }, + request_max_retries: 2, + supports_websockets: false, + auth: { command: "acme-token", args: ["--audience", "llm"] }, + }, + environment: { ACME_API_KEY: "sk-env" }, + }); + + expect(launch).toEqual({ + providerId: "acme.gw v2", + model: "acme-coder-1", + args: [ + "-c", + 'model_provider="acme.gw v2"', + "-c", + 'model_providers={"acme.gw v2" = {"name" = "Acme \\"GW\\"", "base_url" = "https://llm.acme.example/v1", "wire_api" = "responses", "env_key" = "ACME_API_KEY", "query_params" = {"api-version" = "2026-01-01"}, "request_max_retries" = 2, "supports_websockets" = false, "auth" = {"command" = "acme-token", "args" = ["--audience", "llm"]}}}', + ], + environment: {}, + }); + }); + + it("moves a plain token and plain headers into the environment", () => { + const launch = codexModelProviderLaunch({ + providerId: "acme", + definition: { + base_url: "https://llm.acme.example/v1", + experimental_bearer_token: "sk-secret-token", + http_headers: { "X-Org": "org-secret", "X-Team": "team-secret" }, + env_http_headers: { "X-Trace": "TRACE_ENV" }, + }, + environment: {}, + }); + + const args = launch?.args.join(" ") ?? ""; + expect(args).not.toContain("sk-secret-token"); + expect(args).not.toContain("org-secret"); + expect(args).not.toContain("team-secret"); + expect(args).toContain('"env_key" = "THREADLINES_CODEX_PROVIDER_TOKEN"'); + expect(args).toContain( + '"env_http_headers" = {"X-Trace" = "TRACE_ENV", "X-Org" = "THREADLINES_CODEX_PROVIDER_HEADER", "X-Team" = "THREADLINES_CODEX_PROVIDER_HEADER_1"}', + ); + expect(launch?.environment).toEqual({ + THREADLINES_CODEX_PROVIDER_TOKEN: "sk-secret-token", + THREADLINES_CODEX_PROVIDER_HEADER: "org-secret", + THREADLINES_CODEX_PROVIDER_HEADER_1: "team-secret", + }); + }); + + // As Codex resolves them itself: `env_key` wins over a plain token, and a + // header named both ways takes its variable when that variable is set. + it("keeps Codex's own precedence between a variable and a plain value", () => { + const definition = { + env_key: "ACME_API_KEY", + experimental_bearer_token: "sk-unused-token", + http_headers: { "x-org": "plain-org" }, + env_http_headers: { "X-Org": "ORG_ENV" }, + }; + const withVariable = codexModelProviderLaunch({ + providerId: "acme", + definition, + environment: { ORG_ENV: "from-env" }, + }); + expect(withVariable?.environment).toEqual({}); + expect(withVariable?.args.join(" ")).toContain('"env_http_headers" = {"X-Org" = "ORG_ENV"}'); + expect(withVariable?.args.join(" ")).toContain('"env_key" = "ACME_API_KEY"'); + expect(withVariable?.args.join(" ")).not.toContain("sk-unused-token"); + + const withoutVariable = codexModelProviderLaunch({ + providerId: "acme", + definition, + environment: {}, + }); + expect(withoutVariable?.environment).toEqual({ + THREADLINES_CODEX_PROVIDER_HEADER: "plain-org", + }); + expect(withoutVariable?.args.join(" ")).toContain( + '"env_http_headers" = {"x-org" = "THREADLINES_CODEX_PROVIDER_HEADER"}', + ); + }); + + // A name of ours that the run or the definition already uses would hand + // Codex the wrong value: here, a header's value as the bearer token. + it("never reuses a variable name the run or the definition already has", () => { + const launch = codexModelProviderLaunch({ + providerId: "acme", + definition: { + env_key: "THREADLINES_CODEX_PROVIDER_HEADER", + http_headers: { "X-Org": "org-secret" }, + }, + environment: { + THREADLINES_CODEX_PROVIDER_HEADER: "sk-real-token", + THREADLINES_CODEX_PROVIDER_HEADER_1: "something else", + }, + }); + expect(launch?.environment).toEqual({ THREADLINES_CODEX_PROVIDER_HEADER_2: "org-secret" }); + expect(launch?.args.join(" ")).toContain('"env_key" = "THREADLINES_CODEX_PROVIDER_HEADER"'); + + // Windows reads variable names without letter case. + const otherCase = codexModelProviderLaunch({ + providerId: "acme", + definition: { + env_key: "threadlines_codex_provider_header", + http_headers: { "X-Org": "org-secret" }, + }, + environment: {}, + }); + expect(otherCase?.environment).toEqual({ THREADLINES_CODEX_PROVIDER_HEADER_1: "org-secret" }); + }); + + // Codex reads these as plain values only, so nothing but the command line + // could carry them. The run then goes without the gateway. + it("gives no launch when a secret could only travel as an argument", () => { + for (const definition of [ + { base_url: "https://llm.acme.example/v1", query_params: { "api-key": "sk-secret" } }, + { base_url: "https://user:sk-secret@llm.acme.example/v1" }, + { base_url: "https://llm.acme.example/v1", model_catalog_url: "https://u:p@acme.example" }, + { + base_url: "https://llm.acme.example/v1", + model_catalog_url: "https://acme.example/catalog.json?token=sk-secret", + }, + { + base_url: "https://llm.acme.example/v1", + auth: { command: "acme-token", args: ["--client-secret", "sk-secret"] }, + }, + { + base_url: "https://llm.acme.example/v1", + auth: { command: "acme-token", env: { ACME_PASSWORD: "sk-secret" } }, + }, + ]) { + expect( + codexModelProviderLaunch({ providerId: "acme", definition, environment: {} }), + ).toBeUndefined(); + } + }); + + it("names a built-in provider by its id alone", () => { + expect(codexModelProviderLaunch({ providerId: "ollama", environment: {} })).toEqual({ + providerId: "ollama", + args: ["-c", 'model_provider="ollama"'], + environment: {}, + }); + }); +}); diff --git a/apps/server/src/provider/codexModelProviderLaunch.ts b/apps/server/src/provider/codexModelProviderLaunch.ts new file mode 100644 index 000000000..e70ea45d0 --- /dev/null +++ b/apps/server/src/provider/codexModelProviderLaunch.ts @@ -0,0 +1,257 @@ +/** + * Points a one-shot `codex exec` at the model provider the user's Codex runs + * on, when that is not OpenAI (a company gateway, say). + * + * Titles, commit messages and the like run `codex exec --ignore-user-config`, + * so that the user's MCP servers, notify hooks and memories stay out of a + * one-line answer. That also leaves out `model_provider`, and the run goes to + * OpenAI whatever Codex's chats use. Loading the whole config back in brings + * the MCP servers with it (overriding `mcp_servers` on the command line does + * not stop them), so the provider alone is handed over as `-c` overrides: its + * id, and its definition as Codex's own `config/read` reports it. + * + * A definition can hold secrets as plain values (`experimental_bearer_token`, + * `http_headers`). Arguments show in the process list, so those never go + * there: each is moved to an environment variable of the run, which Codex + * reads through `env_key` and `env_http_headers` to the same effect. Where a + * secret sits in a setting with no such variable form (a key in + * `query_params` or in an address, a password in an address, a secret among + * the `auth` command's arguments), there is no launch at all, and the run + * goes to OpenAI as it did before. + * + * @module codexModelProviderLaunch + */ + +/** What to add to a `codex exec` run so it uses the provider. */ +export interface CodexModelProviderLaunch { + readonly providerId: string; + /** The model Codex's config names: the one known to work on this provider. */ + readonly model?: string; + /** `-c` overrides, free of secret values. */ + readonly args: ReadonlyArray; + /** Secret values the overrides name, to set in the run's environment. */ + readonly environment: Readonly>; +} + +const TOKEN_ENV = "THREADLINES_CODEX_PROVIDER_TOKEN"; +const HEADER_ENV = "THREADLINES_CODEX_PROVIDER_HEADER"; + +/** A parameter, flag or variable whose name says it carries a credential. */ +const CREDENTIAL_NAME = /key|token|secret|sig|password|passwd|auth|credential/iu; + +type JsonRecord = { readonly [key: string]: unknown }; + +function isRecord(value: unknown): value is JsonRecord { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +function nonEmptyString(value: unknown): string | undefined { + return typeof value === "string" && value !== "" ? value : undefined; +} + +/** + * A TOML basic string. JSON's escapes are all TOML's; DEL is the one control + * character JSON leaves bare and TOML refuses. + */ +function tomlString(value: string): string { + return JSON.stringify(value).replaceAll("\u007f", "\\u007F"); +} + +/** + * A JSON value as inline TOML, or undefined for what TOML cannot hold (null, + * a number that is not finite). Tables and arrays leave such entries out. + */ +function tomlValue(value: unknown): string | undefined { + switch (typeof value) { + case "string": + return tomlString(value); + case "boolean": + return value ? "true" : "false"; + case "number": + return Number.isFinite(value) ? String(value) : undefined; + case "object": { + if (value === null) return undefined; + if (Array.isArray(value)) { + const items = value.flatMap((item) => tomlValue(item) ?? []); + return `[${items.join(", ")}]`; + } + const entries = Object.entries(value).flatMap(([key, entry]) => { + const encoded = tomlValue(entry); + return encoded === undefined ? [] : [`${tomlString(key)} = ${encoded}`]; + }); + return `{${entries.join(", ")}}`; + } + default: + return undefined; + } +} + +/** + * Whether an address carries a credential: a user name or password, or a + * query parameter named like one. Text that is not a URL carries none. + */ +function hasCredentialInAddress(value: unknown): boolean { + if (typeof value !== "string") return false; + let url: URL; + try { + url = new URL(value); + } catch { + return false; + } + return ( + url.username !== "" || + url.password !== "" || + [...url.searchParams.keys()].some((name) => CREDENTIAL_NAME.test(name)) + ); +} + +/** + * Whether anything in the `auth` command reads like a credential: a flag such + * as `--client-secret`, or a variable it sets. The command's own name and + * plain arguments (an audience, a profile) do not. + */ +function hasCredentialInAuthCommand(auth: unknown): boolean { + if (!isRecord(auth)) return false; + return Object.entries(auth).some(([field, value]) => { + if (field === "command" || field === "cwd") return false; + if (Array.isArray(value)) { + return value.some((entry) => typeof entry === "string" && CREDENTIAL_NAME.test(entry)); + } + return isRecord(value) && Object.keys(value).some((name) => CREDENTIAL_NAME.test(name)); + }); +} + +/** + * Whether the definition holds a secret that only the command line could + * carry: Codex reads these settings as plain values only. + */ +function hasSecretOnlyArgumentsCanCarry(definition: JsonRecord): boolean { + const queryParams = definition.query_params; + return ( + (isRecord(queryParams) && + Object.keys(queryParams).some((name) => CREDENTIAL_NAME.test(name))) || + hasCredentialInAddress(definition.base_url) || + hasCredentialInAddress(definition.model_catalog_url) || + hasCredentialInAuthCommand(definition.auth) + ); +} + +/** Every environment variable the definition names for Codex to read. */ +function referencedVariables(definition: JsonRecord): ReadonlyArray { + const envHeaders = isRecord(definition.env_http_headers) + ? Object.values(definition.env_http_headers) + : []; + return [definition.env_key, ...envHeaders].flatMap((name) => nonEmptyString(name) ?? []); +} + +/** Whether a table already names `header`, as HTTP does: whatever its letter case. */ +function findHeader(table: JsonRecord, header: string): string | undefined { + const wanted = header.toLowerCase(); + return Object.keys(table).find((name) => name.toLowerCase() === wanted); +} + +/** + * The provider's definition with its plain secrets taken out and named by + * environment variables instead, as Codex itself would resolve them: + * + * - `env_key` wins over `experimental_bearer_token` (and a missing variable + * is an error, not a fallback), so the token only matters without one. + * - A header set both ways takes its `env_http_headers` value when that + * variable is set, its plain one otherwise. + */ +function withSecretsInEnvironment( + definition: JsonRecord, + runEnvironment: NodeJS.ProcessEnv, +): { readonly definition: JsonRecord; readonly environment: Record } { + const { + experimental_bearer_token: bearerToken, + http_headers: plainHeaders, + ...rest + } = definition; + const next: Record = { ...rest }; + const environment: Record = {}; + // A name of ours must not be one the run already has, or one the definition + // reads: it would hand Codex the wrong value under that name. Compared + // without letter case, as Windows compares variable names. + const taken = new Set( + [...Object.keys(runEnvironment), ...referencedVariables(definition)].map((name) => + name.toUpperCase(), + ), + ); + const allocate = (base: string): string => { + let name = base; + for (let suffix = 1; taken.has(name.toUpperCase()); suffix += 1) { + name = `${base}_${suffix}`; + } + taken.add(name.toUpperCase()); + return name; + }; + + const token = nonEmptyString(bearerToken); + if (token !== undefined && nonEmptyString(rest.env_key) === undefined) { + const variable = allocate(TOKEN_ENV); + next.env_key = variable; + environment[variable] = token; + } + + if (isRecord(plainHeaders)) { + const envHeaders: Record = isRecord(rest.env_http_headers) + ? { ...rest.env_http_headers } + : {}; + for (const [header, value] of Object.entries(plainHeaders)) { + if (typeof value !== "string") continue; + const existing = findHeader(envHeaders, header); + if (existing !== undefined) { + const variable = nonEmptyString(envHeaders[existing]); + if (variable !== undefined && nonEmptyString(runEnvironment[variable]) !== undefined) { + continue; + } + delete envHeaders[existing]; + } + const variable = allocate(HEADER_ENV); + envHeaders[header] = variable; + environment[variable] = value; + } + if (Object.keys(envHeaders).length > 0) { + next.env_http_headers = envHeaders; + } + } + + return { definition: next, environment }; +} + +/** + * The overrides for a `codex exec` run on provider `providerId`. + * `definition` is the provider's entry in `config/read`'s `model_providers`; + * leave it out for a provider Codex has built in, which needs only its id. + * `environment` is the one the run starts with, to tell which of the + * definition's variables are set. Undefined when the definition holds a + * secret that could only travel on the command line. + */ +export function codexModelProviderLaunch(input: { + readonly providerId: string; + readonly model?: string | undefined; + readonly definition?: unknown; + readonly environment: NodeJS.ProcessEnv; +}): CodexModelProviderLaunch | undefined { + const base = { + providerId: input.providerId, + ...(input.model ? { model: input.model } : {}), + }; + const selectProvider = ["-c", `model_provider=${tomlString(input.providerId)}`]; + if (!isRecord(input.definition)) { + return { ...base, args: selectProvider, environment: {} }; + } + if (hasSecretOnlyArgumentsCanCarry(input.definition)) { + return undefined; + } + const { definition, environment } = withSecretsInEnvironment(input.definition, input.environment); + // The whole table, keyed by the quoted id: a dotted path would split an id + // that has a dot in it. + const table = `{${tomlString(input.providerId)} = ${tomlValue(definition) ?? "{}"}}`; + return { + ...base, + args: [...selectProvider, "-c", `model_providers=${table}`], + environment, + }; +} diff --git a/apps/server/src/provider/providerStatusCache.test.ts b/apps/server/src/provider/providerStatusCache.test.ts index 7422be5cd..79da99414 100644 --- a/apps/server/src/provider/providerStatusCache.test.ts +++ b/apps/server/src/provider/providerStatusCache.test.ts @@ -243,6 +243,46 @@ it.layer(NodeServices.layer)("providerStatusCache", (it) => { ); }); + // On a gateway, Codex's default is the model its config names, which the + // user may also have added by hand. Before the first check the fallback + // holds only that custom entry: the cached default must not be lost to it. + it("keeps a cached default that shares its slug with a custom model", () => { + const cachedCodex = makeProvider(CODEX_DRIVER, { + models: [ + { slug: "gpt-5.4", name: "GPT-5.4", isCustom: false, capabilities: emptyCapabilities }, + { + slug: "acme-coder-1", + name: "acme-coder-1", + isCustom: false, + isDefault: true, + capabilities: emptyCapabilities, + }, + ], + }); + const fallbackCodex = makeProvider(CODEX_DRIVER, { + models: [ + { + slug: "acme-coder-1", + name: "acme-coder-1", + isCustom: true, + capabilities: emptyCapabilities, + }, + ], + }); + + const hydrated = hydrateCachedProvider({ + cachedProvider: cachedCodex, + fallbackProvider: fallbackCodex, + }); + assert.deepStrictEqual( + hydrated.models.map((model) => [model.slug, model.isDefault === true, model.isCustom]), + [ + ["acme-coder-1", true, false], + ["gpt-5.4", false, false], + ], + ); + }); + it("ignores stale cached enabled state when the provider is now disabled", () => { const cachedCodex = makeProvider(CODEX_DRIVER, { checkedAt: "2026-04-10T12:00:00.000Z", diff --git a/apps/server/src/provider/providerStatusCache.ts b/apps/server/src/provider/providerStatusCache.ts index b0c3b7b3b..ec8daac66 100644 --- a/apps/server/src/provider/providerStatusCache.ts +++ b/apps/server/src/provider/providerStatusCache.ts @@ -21,7 +21,16 @@ const mergeProviderModels = ( cachedModels: ReadonlyArray, ): ReadonlyArray => { const fallbackSlugs = new Set(fallbackModels.map((model) => model.slug)); - return [...fallbackModels, ...cachedModels.filter((model) => !fallbackSlugs.has(model.slug))]; + // The fallback names no default of its own (Codex's is just the user's + // custom models): a cached default with the same slug as one of them stays + // the default, or the list would come back with none until the first check. + const cachedDefault = fallbackModels.some((model) => model.isDefault === true) + ? undefined + : cachedModels.find((model) => model.isDefault === true && fallbackSlugs.has(model.slug)); + return [ + ...fallbackModels.map((model) => (model.slug === cachedDefault?.slug ? cachedDefault : model)), + ...cachedModels.filter((model) => !fallbackSlugs.has(model.slug)), + ]; }; export const orderProviderSnapshots = ( diff --git a/apps/server/src/textGeneration/CodexTextGeneration.test.ts b/apps/server/src/textGeneration/CodexTextGeneration.test.ts index d640b13a7..4e0ae9175 100644 --- a/apps/server/src/textGeneration/CodexTextGeneration.test.ts +++ b/apps/server/src/textGeneration/CodexTextGeneration.test.ts @@ -20,6 +20,7 @@ import { import { ServerConfig } from "../config.ts"; import { type TextGenerationShape } from "./TextGeneration.ts"; import { makeCodexTextGeneration } from "./CodexTextGeneration.ts"; +import { codexModelProviderLaunch } from "../provider/codexModelProviderLaunch.ts"; const decodeCodexSettings = Schema.decodeSync(CodexSettings); const DEFAULT_TEST_MODEL_SELECTION = createModelSelection( @@ -56,6 +57,7 @@ function makeFakeCodexBinary( failForModelExitCode?: number; stdinMustContain?: string; stdinMustNotContain?: string; + recordInvocationTo?: string; }, ) { return Effect.gen(function* () { @@ -268,6 +270,15 @@ function makeFakeCodexBinary( codexPath, [ "#!/bin/sh", + ...(input.recordInvocationTo !== undefined + ? [ + // The arguments one per line, then the provider's secret variables. + // @effect-diagnostics-next-line preferSchemaOverJson:off + `printf "%s\n" "$@" > ${JSON.stringify(`${input.recordInvocationTo}.args`)}`, + // @effect-diagnostics-next-line preferSchemaOverJson:off + `env | grep "^THREADLINES_CODEX_PROVIDER_" > ${JSON.stringify(`${input.recordInvocationTo}.env`)}`, + ] + : []), 'output_path=""', 'seen_image="0"', 'seen_fast_service_tier="0"', @@ -471,15 +482,18 @@ function withFakeCodexEnv( failForModelExitCode?: number; stdinMustContain?: string; stdinMustNotContain?: string; + /** POSIX only: writes the run's arguments and provider variables to `.args` and `.env`. */ + recordInvocationTo?: string; }, effectFn: (textGeneration: TextGenerationShape) => Effect.Effect, + options?: Parameters[2], ) { return Effect.gen(function* () { const fs = yield* FileSystem.FileSystem; const tempDir = yield* fs.makeTempDirectoryScoped({ prefix: "threadlines-codex-text-" }); const codexPath = yield* makeFakeCodexBinary(tempDir, input); const config = decodeCodexSettings({ binaryPath: codexPath }); - const textGeneration = yield* makeCodexTextGeneration(config); + const textGeneration = yield* makeCodexTextGeneration(config, process.env, options); return yield* effectFn(textGeneration); }).pipe(Effect.scoped); } @@ -673,6 +687,67 @@ it.layer(CodexTextGenerationTestLayer)("CodexTextGeneration", (it) => { ), ); + // The built-in pick for titles is an OpenAI model, which a gateway may not + // serve. The model Codex's own config names is the one known to work there. + it.effect("falls back to the gateway's own model when the picked one is refused", () => + withFakeCodexEnv( + { + output: JSON.stringify({ title: "Investigate reconnect regressions" }), + failForModel: DEFAULT_GIT_TEXT_GENERATION_MODEL, + failForModelStderr: "ERROR: unknown model", + requireModel: "acme-coder-1", + }, + (textGeneration) => + Effect.gen(function* () { + const generated = yield* textGeneration.generateThreadTitle({ + cwd: process.cwd(), + message: "Please investigate websocket reconnect regressions.", + modelSelection: DEFAULT_TEST_MODEL_SELECTION, + }); + + expect(generated.title).toBe("Investigate reconnect regressions"); + }), + { + modelProviderLaunch: Effect.succeed( + codexModelProviderLaunch({ providerId: "acme", model: "acme-coder-1", environment: {} }), + ), + }, + ), + ); + + // Another model would not help a failed sign-in: the failure is reported as + // it is. Codex prints the prompt above its error, and a line of the prompt + // that reads like a model error is not one. + it.effect("does not try the gateway's model for a failure that names no model", () => + withFakeCodexEnv( + { + output: JSON.stringify({ title: "unused" }), + failForModel: DEFAULT_GIT_TEXT_GENERATION_MODEL, + failForModelStderr: "user\nERROR: model cache is corrupt\nERROR: 401 Unauthorized", + }, + (textGeneration) => + Effect.gen(function* () { + const result = yield* textGeneration + .generateThreadTitle({ + cwd: process.cwd(), + message: "Please investigate websocket reconnect regressions.", + modelSelection: DEFAULT_TEST_MODEL_SELECTION, + }) + .pipe(Effect.result); + + expect(Result.isFailure(result)).toBe(true); + if (Result.isFailure(result)) { + expect(result.failure.detail).toContain("401 Unauthorized"); + } + }), + { + modelProviderLaunch: Effect.succeed( + codexModelProviderLaunch({ providerId: "acme", model: "acme-coder-1", environment: {} }), + ), + }, + ), + ); + it.effect("generates commit message with branch when includeBranch is true", () => withFakeCodexEnv( { @@ -769,6 +844,52 @@ it.layer(CodexTextGenerationTestLayer)("CodexTextGeneration", (it) => { ), ); + // Titles and commit messages leave the user's Codex config out, and with it + // the gateway Codex runs on. The gateway alone is handed back, and a secret + // in its definition travels in the environment, never on the command line. + it.effect.skipIf(process.platform === "win32")( + "runs on the gateway Codex is configured for, with its secrets out of the arguments", + () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const recordDir = yield* fs.makeTempDirectoryScoped({ prefix: "threadlines-codex-args-" }); + const record = path.join(recordDir, "run"); + const launch = codexModelProviderLaunch({ + providerId: "acme", + definition: { + name: "Acme Gateway", + base_url: "https://llm.acme.example/v1", + experimental_bearer_token: "sk-secret-token", + env_key: null, + }, + environment: {}, + }); + yield* withFakeCodexEnv( + { + output: JSON.stringify({ title: "Investigate reconnect regressions" }), + requireIgnoreUserConfig: true, + recordInvocationTo: record, + }, + (textGeneration) => + textGeneration.generateThreadTitle({ + cwd: process.cwd(), + message: "Please investigate websocket reconnect regressions.", + modelSelection: DEFAULT_TEST_MODEL_SELECTION, + }), + { modelProviderLaunch: Effect.succeed(launch) }, + ); + + const args = (yield* fs.readFileString(`${record}.args`)).split("\n"); + expect(args).toContain('model_provider="acme"'); + expect(args.some((arg) => arg.startsWith('model_providers={"acme" = {'))).toBe(true); + expect(args.join("\n")).not.toContain("sk-secret-token"); + expect(yield* fs.readFileString(`${record}.env`)).toContain( + "THREADLINES_CODEX_PROVIDER_TOKEN=sk-secret-token", + ); + }).pipe(Effect.scoped), + ); + it.effect("retries without --ignore-user-config when the codex CLI rejects the flag", () => withFakeCodexEnv( { diff --git a/apps/server/src/textGeneration/CodexTextGeneration.ts b/apps/server/src/textGeneration/CodexTextGeneration.ts index d4e1c5efb..5a88ed326 100644 --- a/apps/server/src/textGeneration/CodexTextGeneration.ts +++ b/apps/server/src/textGeneration/CodexTextGeneration.ts @@ -20,6 +20,7 @@ import { sanitizeBranchFragment, sanitizeFeatureBranchName } from "@threadlines/ import { resolveAttachmentPath } from "../attachmentStore.ts"; import { ServerConfig } from "../config.ts"; import { expandHomePath } from "../pathExpansion.ts"; +import type { CodexModelProviderLaunch } from "../provider/codexModelProviderLaunch.ts"; import { TextGenerationError } from "@threadlines/contracts"; import { type BranchNameGenerationInput, @@ -66,6 +67,8 @@ type RunCodexJsonInput = { imagePaths?: ReadonlyArray; cleanupPaths?: ReadonlyArray; modelSelection: ModelSelection; + /** The gateway to run on, asked once for the whole generation. */ + providerLaunch?: CodexModelProviderLaunch | undefined; }; function isCodexModelCapacityError(error: TextGenerationError): boolean { @@ -76,6 +79,19 @@ function isCodexModelCapacityError(error: TextGenerationError): boolean { ); } +/** + * Whether Codex ran and the service refused the model: its closing `ERROR:` + * line names the model. A failed sign-in or a broken config ends on another + * line, and another model would not help those. Only the last line counts: + * Codex prints the prompt above it, which can hold anything. + */ +function isCodexModelRefusal(error: TextGenerationError): boolean { + const prefix = "Codex CLI command failed: "; + if (!error.detail.startsWith(prefix)) return false; + const lastLine = error.detail.slice(prefix.length).trimEnd().split("\n").at(-1)?.trim() ?? ""; + return /^ERROR:.*\bmodel\b/iu.test(lastLine); +} + function isUnsupportedIgnoreUserConfigFlagError(error: TextGenerationError): boolean { const detail = error.detail.toLowerCase(); return ( @@ -105,6 +121,14 @@ function withDefaultCodexModel(modelSelection: ModelSelection): ModelSelection { export const makeCodexTextGeneration = Effect.fn("makeCodexTextGeneration")(function* ( codexConfig: CodexSettings, environment: NodeJS.ProcessEnv = process.env, + textGenerationOptions?: { + /** + * The provider Codex's chats run on when it is not OpenAI, as the last + * status check found it, asked at each run. Without it a run goes to + * OpenAI, which is where `--ignore-user-config` leaves it. + */ + readonly modelProviderLaunch?: Effect.Effect; + }, ) { const fileSystem = yield* FileSystem.FileSystem; const path = yield* Path.Path; @@ -212,6 +236,7 @@ export const makeCodexTextGeneration = Effect.fn("makeCodexTextGeneration")(func imagePaths = [], cleanupPaths = [], modelSelection, + providerLaunch: gatewayLaunch, }: RunCodexJsonInput): Effect.fn.Return< S["Type"], TextGenerationError, @@ -237,37 +262,47 @@ export const makeCodexTextGeneration = Effect.fn("makeCodexTextGeneration")(func getModelSelectionStringOptionValue(modelSelection, "reasoningEffort") ?? DEFAULT_GIT_TEXT_GENERATION_REASONING_EFFORT; const serviceTier = resolveCodexCliServiceTier(modelSelection); - const env = { + const baseEnv = { ...environment, ...(codexConfig.homePath ? { CODEX_HOME: expandHomePath(codexConfig.homePath) } : {}), }; - const plan = planCliSpawn( - codexConfig.binaryPath || "codex", - [ - "exec", - "--ephemeral", - // One-shot text generation should not inherit agent-oriented user - // config (MCP servers, notify hooks, memories) from - // $CODEX_HOME/config.toml; auth still uses CODEX_HOME. - ...(options.ignoreUserConfig ? ["--ignore-user-config"] : []), - "--skip-git-repo-check", - ...(operation === "generateCommitMessage" ? ["--ignore-rules"] : []), - "-s", - "read-only", - "--model", - modelSelection.model, - "--config", - `model_reasoning_effort="${reasoningEffort}"`, - ...(serviceTier ? ["--config", `service_tier="${serviceTier}"`] : []), - "--output-schema", - schemaPath, - "--output-last-message", - outputPath, - ...imagePaths.flatMap((imagePath) => ["--image", imagePath]), - "-", - ], - env, - ); + const planFor = (providerLaunch: CodexModelProviderLaunch | undefined) => { + const env = { ...baseEnv, ...providerLaunch?.environment }; + return { + env, + plan: planCliSpawn( + codexConfig.binaryPath || "codex", + [ + "exec", + "--ephemeral", + // One-shot text generation should not inherit agent-oriented user + // config (MCP servers, notify hooks, memories) from + // $CODEX_HOME/config.toml; auth still uses CODEX_HOME. + ...(options.ignoreUserConfig ? ["--ignore-user-config"] : []), + ...(providerLaunch?.args ?? []), + "--skip-git-repo-check", + ...(operation === "generateCommitMessage" ? ["--ignore-rules"] : []), + "-s", + "read-only", + "--model", + modelSelection.model, + "--config", + `model_reasoning_effort="${reasoningEffort}"`, + ...(serviceTier ? ["--config", `service_tier="${serviceTier}"`] : []), + "--output-schema", + schemaPath, + "--output-last-message", + outputPath, + ...imagePaths.flatMap((imagePath) => ["--image", imagePath]), + "-", + ], + env, + ), + }; + }; + // The gateway goes along only beside --ignore-user-config: a run without + // the flag loads the user's own config, provider and all. + const { env, plan } = planFor(options.ignoreUserConfig ? gatewayLaunch : undefined); const command = ChildProcess.make( plan.command, [...plan.args], @@ -374,7 +409,46 @@ export const makeCodexTextGeneration = Effect.fn("makeCodexTextGeneration")(func >( input: RunCodexJsonInput, ): Effect.fn.Return { - return yield* runCodexJson(input).pipe( + const knownLaunch = textGenerationOptions?.modelProviderLaunch + ? yield* textGenerationOptions.modelProviderLaunch + : undefined; + // A batch file that only cmd.exe can start takes its arguments as one + // command line, where the quotes in the gateway's settings would let an + // `&` or a `%NAME%` in them (or in its model's name) act as a command. + // Such a Codex runs without the gateway, as it did before. + const startsThroughShell = + knownLaunch !== undefined && + planCliSpawn(codexConfig.binaryPath || "codex", [], { + ...environment, + ...(codexConfig.homePath ? { CODEX_HOME: expandHomePath(codexConfig.homePath) } : {}), + }).options.shell === true; + const providerLaunch = startsThroughShell ? undefined : knownLaunch; + const run = { ...input, providerLaunch }; + // On a gateway, the model picked for titles and commit messages may be one + // only OpenAI serves (the built-in pick is). The model Codex's config names + // is known to work there, so a run refused for its model is tried once + // more on that one. + const gatewayModel = providerLaunch?.model; + return yield* runCodexJson(run).pipe( + Effect.catchIf( + (error) => + gatewayModel !== undefined && + input.modelSelection.model !== gatewayModel && + isCodexModelRefusal(error), + () => + Effect.logWarning("retrying Codex text generation with the gateway's model", { + operation: input.operation, + failedModel: input.modelSelection.model, + fallbackModel: gatewayModel, + }).pipe( + Effect.andThen( + runCodexJson({ + ...run, + modelSelection: { ...input.modelSelection, model: gatewayModel ?? DEFAULT_MODEL }, + }), + ), + ), + ), Effect.catchIf( (error) => shouldRetryWithDefaultCodexModel(input.modelSelection, error), () => @@ -385,7 +459,7 @@ export const makeCodexTextGeneration = Effect.fn("makeCodexTextGeneration")(func }).pipe( Effect.andThen( runCodexJson({ - ...input, + ...run, modelSelection: withDefaultCodexModel(input.modelSelection), }), ), diff --git a/apps/web/src/components/settings/ProviderInstanceCard.test.ts b/apps/web/src/components/settings/ProviderInstanceCard.test.ts index 7c35dae55..93a464914 100644 --- a/apps/web/src/components/settings/ProviderInstanceCard.test.ts +++ b/apps/web/src/components/settings/ProviderInstanceCard.test.ts @@ -47,6 +47,24 @@ describe("deriveProviderModelsForDisplay", () => { }).map((model) => model.slug), ).toEqual(["server-model", "kept-custom"]); }); + + // Codex on a gateway lists the model from its own config, which the user + // may have added by hand before: one row, the provider's. + it("shows a model the provider lists itself once, not again as a custom one", () => { + const liveModels: ReadonlyArray = [ + { slug: "acme-coder-1", name: "acme-coder-1", isCustom: false, capabilities: null }, + ]; + + expect( + deriveProviderModelsForDisplay({ + liveModels, + customModels: ["acme-coder-1", "acme-coder-mini"], + }).map((model) => [model.slug, model.isCustom]), + ).toEqual([ + ["acme-coder-1", false], + ["acme-coder-mini", true], + ]); + }); }); describe("Claude credential preference helpers", () => { diff --git a/apps/web/src/components/settings/ProviderInstanceCard.tsx b/apps/web/src/components/settings/ProviderInstanceCard.tsx index 54eff9d23..187fa42fd 100644 --- a/apps/web/src/components/settings/ProviderInstanceCard.tsx +++ b/apps/web/src/components/settings/ProviderInstanceCard.tsx @@ -211,15 +211,20 @@ export function deriveProviderModelsForDisplay(input: { .map((model) => [model.slug, model] as const), ); const serverModels = input.liveModels?.filter((model) => !model.isCustom) ?? []; - const customModels = input.customModels.map( - (slug) => - liveCustomModelsBySlug.get(slug) ?? { - slug, - name: slug, - isCustom: true, - capabilities: null, - }, - ); + // A model the provider now lists itself (Codex's own config model, say) is + // shown once, as the provider's: the user's entry of the same id is spent. + const serverSlugs = new Set(serverModels.map((model) => model.slug)); + const customModels = input.customModels + .filter((slug) => !serverSlugs.has(slug)) + .map( + (slug) => + liveCustomModelsBySlug.get(slug) ?? { + slug, + name: slug, + isCustom: true, + capabilities: null, + }, + ); return [...serverModels, ...customModels]; } diff --git a/docs/providers/codex.md b/docs/providers/codex.md index 3beb096ee..cf4e81d00 100644 --- a/docs/providers/codex.md +++ b/docs/providers/codex.md @@ -85,10 +85,23 @@ later. - A ChatGPT sign-in can sit beside a gateway. Its email, workspace and `Sign out` stay on the row. - With no account at all the row reads `No sign-in needed`: Codex is ready on the gateway alone. Threadlines can't check the gateway's key, so a wrong key shows up when a chat fails. -- Codex lists OpenAI's models whatever the provider. If your gateway serves other models, add - their ids under the Codex row's `Models` as custom models. -- Titles and commit messages written by Codex still go to OpenAI, not the gateway. Pick another - agent for them in `Settings` if Codex has no OpenAI sign-in. +- The model Codex's config names (`model`) is in the model picker and is the default for new + threads. Codex lists OpenAI's models beside it whatever the provider. If your gateway serves + others, add their ids under the Codex row's `Models` as custom models. +- Titles and commit messages written by Codex go through the gateway too. If the gateway refuses + the model picked for them in `Settings`, Threadlines tries once more with the model from + Codex's config. +- Those short runs leave the rest of your Codex config out (MCP servers, notify hooks), as they + always have. A token or header written into the provider's entry in `config.toml` reaches them + through their environment, never their command line. +- Two setups keep those short runs on OpenAI, as before. One is a provider with a secret that + only the command line could carry: a password or key in its address or `query_params`, or a + secret among its `auth` command's arguments. The other is, on Windows, a Codex that starts + through a batch file other than npm's own launcher. +- If Codex can't be asked which provider it is on for 15 minutes, Threadlines stops assuming the + gateway until Codex names it again. +- A Codex agent answering on the side in a room still runs on OpenAI with your ChatGPT sign-in, + not on the gateway. ## Security Notices From OpenAI From 3347aa39c08aae1f029ef238e500f0725a8179a7 Mon Sep 17 00:00:00 2001 From: Badcuban <108198679+badcuban@users.noreply.github.com> Date: Sat, 14 Nov 2026 14:25:49 -0500 Subject: [PATCH 2/3] fix(codex): a gateway's settings reach one-shot runs in a private home, never as arguments The gateway's definition went to `codex exec` as `-c` overrides, with a name-based filter for what looked like a secret. A credential under a name the filter did not know (an Azure Functions `code` key in `query_params`) still ended up in the process list. A run on a gateway now gets a Codex home of its own: a folder only its owner can open, holding a config with the provider alone, removed when the run ends. Nothing from the definition is an argument, so there is nothing to filter, no variable names to invent, and nothing for cmd.exe to misread. The home stands in for --ignore-user-config: it has no user config to ignore. A home made for one run has no sign-in, so a provider that signs in with what Codex's own home stores (`requires_openai_auth`, a gateway's OAuth) gets no routing and its runs stay on OpenAI, as before. --- .../src/provider/Layers/CodexProvider.ts | 6 +- .../provider/Layers/ProviderRegistry.test.ts | 14 +- .../provider/codexModelProviderLaunch.test.ts | 146 ++---------- .../src/provider/codexModelProviderLaunch.ts | 207 +++--------------- .../CodexTextGeneration.test.ts | 47 ++-- .../src/textGeneration/CodexTextGeneration.ts | 134 +++++++----- docs/providers/codex.md | 12 +- 7 files changed, 181 insertions(+), 385 deletions(-) diff --git a/apps/server/src/provider/Layers/CodexProvider.ts b/apps/server/src/provider/Layers/CodexProvider.ts index a30d36e01..fb6d0d7a0 100644 --- a/apps/server/src/provider/Layers/CodexProvider.ts +++ b/apps/server/src/provider/Layers/CodexProvider.ts @@ -1002,7 +1002,7 @@ export interface CodexModelProviderMemory { /** When Codex last named this provider as its default. */ readonly confirmedAtMs: number; readonly model?: string | undefined; - /** Absent when the config was never read, or holds a secret only arguments could carry. */ + /** Absent when the config was never read, or the provider needs Codex's stored sign-in. */ readonly launch?: CodexModelProviderLaunch | undefined; } | undefined; @@ -1038,7 +1038,6 @@ export function currentCodexModelProvider( function codexGatewayForCheck( snapshot: Pick, memory: CodexModelProviderMemory, - environment: NodeJS.ProcessEnv, nowMs: number, ): CodexModelProviderMemory["last"] { if (snapshot.modelProviders === undefined) return currentCodexModelProvider(memory, nowMs); @@ -1050,7 +1049,7 @@ function codexGatewayForCheck( providerId: gateway.providerId, confirmedAtMs: nowMs, model: gateway.model, - launch: codexModelProviderLaunch({ ...gateway, environment }), + launch: codexModelProviderLaunch(gateway), }; } else { memory.last = { @@ -1488,7 +1487,6 @@ export const checkCodexProviderStatus = Effect.fn("checkCodexProviderStatus")(fu const gateway = codexGatewayForCheck( snapshot, modelProviderMemory, - environment, yield* Clock.currentTimeMillis, ); const accountStatus = accountProbeStatus(snapshot, workspaceMemory, securityNoticeMemory); diff --git a/apps/server/src/provider/Layers/ProviderRegistry.test.ts b/apps/server/src/provider/Layers/ProviderRegistry.test.ts index 42191a94a..8e958ba21 100644 --- a/apps/server/src/provider/Layers/ProviderRegistry.test.ts +++ b/apps/server/src/provider/Layers/ProviderRegistry.test.ts @@ -672,12 +672,14 @@ it.layer(Layer.mergeAll(NodeServices.layer, ServerSettingsService.layerTest(), T const first = yield* check(() => Effect.succeed(onGateway)); assert.deepStrictEqual(defaultModels(first), ["acme-coder-1"]); - assert.deepStrictEqual(modelProviderMemory.last?.launch?.args, [ - "-c", - 'model_provider="acme"', - "-c", - 'model_providers={"acme" = {"base_url" = "https://llm.acme.example/v1", "env_key" = "ACME_API_KEY"}}', - ]); + assert.strictEqual( + modelProviderMemory.last?.launch?.config, + [ + 'model_provider = "acme"', + 'model_providers = {"acme" = {"base_url" = "https://llm.acme.example/v1", "env_key" = "ACME_API_KEY"}}', + "", + ].join("\n"), + ); // Codex did not say which providers it has: nothing is known to have changed. const noProviderList = yield* check(() => Effect.succeed(makeCodexProbeSnapshot())); diff --git a/apps/server/src/provider/codexModelProviderLaunch.test.ts b/apps/server/src/provider/codexModelProviderLaunch.test.ts index 8e0c1c8b8..5a81526c8 100644 --- a/apps/server/src/provider/codexModelProviderLaunch.test.ts +++ b/apps/server/src/provider/codexModelProviderLaunch.test.ts @@ -3,7 +3,7 @@ import { describe, expect, it } from "vite-plus/test"; import { codexModelProviderLaunch } from "./codexModelProviderLaunch.ts"; describe("codexModelProviderLaunch", () => { - it("hands over the provider's definition as one table, keyed by its quoted id", () => { + it("writes a config with the provider alone, its definition whole", () => { const launch = codexModelProviderLaunch({ providerId: "acme.gw v2", model: "acme-coder-1", @@ -11,148 +11,44 @@ describe("codexModelProviderLaunch", () => { name: 'Acme "GW"', base_url: "https://llm.acme.example/v1", wire_api: "responses", - env_key: "ACME_API_KEY", + env_key: null, model_catalog_url: null, - query_params: { "api-version": "2026-01-01" }, + experimental_bearer_token: "sk-secret-token", + http_headers: { "X-Org": "org-secret" }, + query_params: { "api-version": "2026-01-01", code: "fn-key" }, request_max_retries: 2, - supports_websockets: false, - auth: { command: "acme-token", args: ["--audience", "llm"] }, + requires_openai_auth: false, + gateway_oauth: null, + auth: { command: "acme-token", args: ["--client-secret", "s3cret"] }, }, - environment: { ACME_API_KEY: "sk-env" }, }); expect(launch).toEqual({ providerId: "acme.gw v2", model: "acme-coder-1", - args: [ - "-c", - 'model_provider="acme.gw v2"', - "-c", - 'model_providers={"acme.gw v2" = {"name" = "Acme \\"GW\\"", "base_url" = "https://llm.acme.example/v1", "wire_api" = "responses", "env_key" = "ACME_API_KEY", "query_params" = {"api-version" = "2026-01-01"}, "request_max_retries" = 2, "supports_websockets" = false, "auth" = {"command" = "acme-token", "args" = ["--audience", "llm"]}}}', - ], - environment: {}, + config: [ + 'model_provider = "acme.gw v2"', + 'model_providers = {"acme.gw v2" = {"name" = "Acme \\"GW\\"", "base_url" = "https://llm.acme.example/v1", "wire_api" = "responses", "experimental_bearer_token" = "sk-secret-token", "http_headers" = {"X-Org" = "org-secret"}, "query_params" = {"api-version" = "2026-01-01", "code" = "fn-key"}, "request_max_retries" = 2, "requires_openai_auth" = false, "auth" = {"command" = "acme-token", "args" = ["--client-secret", "s3cret"]}}}', + "", + ].join("\n"), }); }); - it("moves a plain token and plain headers into the environment", () => { - const launch = codexModelProviderLaunch({ - providerId: "acme", - definition: { - base_url: "https://llm.acme.example/v1", - experimental_bearer_token: "sk-secret-token", - http_headers: { "X-Org": "org-secret", "X-Team": "team-secret" }, - env_http_headers: { "X-Trace": "TRACE_ENV" }, - }, - environment: {}, - }); - - const args = launch?.args.join(" ") ?? ""; - expect(args).not.toContain("sk-secret-token"); - expect(args).not.toContain("org-secret"); - expect(args).not.toContain("team-secret"); - expect(args).toContain('"env_key" = "THREADLINES_CODEX_PROVIDER_TOKEN"'); - expect(args).toContain( - '"env_http_headers" = {"X-Trace" = "TRACE_ENV", "X-Org" = "THREADLINES_CODEX_PROVIDER_HEADER", "X-Team" = "THREADLINES_CODEX_PROVIDER_HEADER_1"}', - ); - expect(launch?.environment).toEqual({ - THREADLINES_CODEX_PROVIDER_TOKEN: "sk-secret-token", - THREADLINES_CODEX_PROVIDER_HEADER: "org-secret", - THREADLINES_CODEX_PROVIDER_HEADER_1: "team-secret", - }); - }); - - // As Codex resolves them itself: `env_key` wins over a plain token, and a - // header named both ways takes its variable when that variable is set. - it("keeps Codex's own precedence between a variable and a plain value", () => { - const definition = { - env_key: "ACME_API_KEY", - experimental_bearer_token: "sk-unused-token", - http_headers: { "x-org": "plain-org" }, - env_http_headers: { "X-Org": "ORG_ENV" }, - }; - const withVariable = codexModelProviderLaunch({ - providerId: "acme", - definition, - environment: { ORG_ENV: "from-env" }, - }); - expect(withVariable?.environment).toEqual({}); - expect(withVariable?.args.join(" ")).toContain('"env_http_headers" = {"X-Org" = "ORG_ENV"}'); - expect(withVariable?.args.join(" ")).toContain('"env_key" = "ACME_API_KEY"'); - expect(withVariable?.args.join(" ")).not.toContain("sk-unused-token"); - - const withoutVariable = codexModelProviderLaunch({ - providerId: "acme", - definition, - environment: {}, - }); - expect(withoutVariable?.environment).toEqual({ - THREADLINES_CODEX_PROVIDER_HEADER: "plain-org", - }); - expect(withoutVariable?.args.join(" ")).toContain( - '"env_http_headers" = {"x-org" = "THREADLINES_CODEX_PROVIDER_HEADER"}', - ); - }); - - // A name of ours that the run or the definition already uses would hand - // Codex the wrong value: here, a header's value as the bearer token. - it("never reuses a variable name the run or the definition already has", () => { - const launch = codexModelProviderLaunch({ - providerId: "acme", - definition: { - env_key: "THREADLINES_CODEX_PROVIDER_HEADER", - http_headers: { "X-Org": "org-secret" }, - }, - environment: { - THREADLINES_CODEX_PROVIDER_HEADER: "sk-real-token", - THREADLINES_CODEX_PROVIDER_HEADER_1: "something else", - }, - }); - expect(launch?.environment).toEqual({ THREADLINES_CODEX_PROVIDER_HEADER_2: "org-secret" }); - expect(launch?.args.join(" ")).toContain('"env_key" = "THREADLINES_CODEX_PROVIDER_HEADER"'); - - // Windows reads variable names without letter case. - const otherCase = codexModelProviderLaunch({ - providerId: "acme", - definition: { - env_key: "threadlines_codex_provider_header", - http_headers: { "X-Org": "org-secret" }, - }, - environment: {}, - }); - expect(otherCase?.environment).toEqual({ THREADLINES_CODEX_PROVIDER_HEADER_1: "org-secret" }); - }); - - // Codex reads these as plain values only, so nothing but the command line - // could carry them. The run then goes without the gateway. - it("gives no launch when a secret could only travel as an argument", () => { + // A run's own home has no sign-in: these providers use the one stored in + // the user's Codex home, so their runs stay as they were. + it("gives no launch for a provider that needs Codex's stored sign-in", () => { for (const definition of [ - { base_url: "https://llm.acme.example/v1", query_params: { "api-key": "sk-secret" } }, - { base_url: "https://user:sk-secret@llm.acme.example/v1" }, - { base_url: "https://llm.acme.example/v1", model_catalog_url: "https://u:p@acme.example" }, - { - base_url: "https://llm.acme.example/v1", - model_catalog_url: "https://acme.example/catalog.json?token=sk-secret", - }, - { - base_url: "https://llm.acme.example/v1", - auth: { command: "acme-token", args: ["--client-secret", "sk-secret"] }, - }, - { - base_url: "https://llm.acme.example/v1", - auth: { command: "acme-token", env: { ACME_PASSWORD: "sk-secret" } }, - }, + { base_url: "https://proxy.acme.example/v1", requires_openai_auth: true }, + { base_url: "https://llm.acme.example/v1", gateway_oauth: { issuer: "https://id.acme" } }, ]) { - expect( - codexModelProviderLaunch({ providerId: "acme", definition, environment: {} }), - ).toBeUndefined(); + expect(codexModelProviderLaunch({ providerId: "acme", definition })).toBeUndefined(); } }); it("names a built-in provider by its id alone", () => { - expect(codexModelProviderLaunch({ providerId: "ollama", environment: {} })).toEqual({ + expect(codexModelProviderLaunch({ providerId: "ollama" })).toEqual({ providerId: "ollama", - args: ["-c", 'model_provider="ollama"'], - environment: {}, + config: 'model_provider = "ollama"\n', }); }); }); diff --git a/apps/server/src/provider/codexModelProviderLaunch.ts b/apps/server/src/provider/codexModelProviderLaunch.ts index e70ea45d0..a02afc51d 100644 --- a/apps/server/src/provider/codexModelProviderLaunch.ts +++ b/apps/server/src/provider/codexModelProviderLaunch.ts @@ -7,48 +7,39 @@ * one-line answer. That also leaves out `model_provider`, and the run goes to * OpenAI whatever Codex's chats use. Loading the whole config back in brings * the MCP servers with it (overriding `mcp_servers` on the command line does - * not stop them), so the provider alone is handed over as `-c` overrides: its - * id, and its definition as Codex's own `config/read` reports it. + * not stop them). * - * A definition can hold secrets as plain values (`experimental_bearer_token`, - * `http_headers`). Arguments show in the process list, so those never go - * there: each is moved to an environment variable of the run, which Codex - * reads through `env_key` and `env_http_headers` to the same effect. Where a - * secret sits in a setting with no such variable form (a key in - * `query_params` or in an address, a password in an address, a secret among - * the `auth` command's arguments), there is no launch at all, and the run - * goes to OpenAI as it did before. + * So the run gets a Codex home of its own, holding one file: a config with + * the provider alone, its id and its definition as Codex's own `config/read` + * reports it. A definition can hold secrets (a token, headers, a key in its + * address), which is why it travels in a file only its owner can read and + * never as `-c` arguments, which show in the process list. + * + * A home of its own has no sign-in. A provider that signs in through Codex's + * stored sign-in (`requires_openai_auth`, or a gateway's OAuth) therefore + * gets no launch, and its runs go to OpenAI as they did before. * * @module codexModelProviderLaunch */ -/** What to add to a `codex exec` run so it uses the provider. */ +/** What a `codex exec` run needs to use the provider. */ export interface CodexModelProviderLaunch { readonly providerId: string; /** The model Codex's config names: the one known to work on this provider. */ readonly model?: string; - /** `-c` overrides, free of secret values. */ - readonly args: ReadonlyArray; - /** Secret values the overrides name, to set in the run's environment. */ - readonly environment: Readonly>; + /** + * The text of the run's own `config.toml`. It can hold the provider's + * secrets: keep it out of logs, arguments and anything sent to a client. + */ + readonly config: string; } -const TOKEN_ENV = "THREADLINES_CODEX_PROVIDER_TOKEN"; -const HEADER_ENV = "THREADLINES_CODEX_PROVIDER_HEADER"; - -/** A parameter, flag or variable whose name says it carries a credential. */ -const CREDENTIAL_NAME = /key|token|secret|sig|password|passwd|auth|credential/iu; - type JsonRecord = { readonly [key: string]: unknown }; function isRecord(value: unknown): value is JsonRecord { return typeof value === "object" && value !== null && !Array.isArray(value); } -function nonEmptyString(value: unknown): string | undefined { - return typeof value === "string" && value !== "" ? value : undefined; -} - /** * A TOML basic string. JSON's escapes are all TOML's; DEL is the one control * character JSON leaves bare and TOML refuses. @@ -87,171 +78,39 @@ function tomlValue(value: unknown): string | undefined { } /** - * Whether an address carries a credential: a user name or password, or a - * query parameter named like one. Text that is not a URL carries none. - */ -function hasCredentialInAddress(value: unknown): boolean { - if (typeof value !== "string") return false; - let url: URL; - try { - url = new URL(value); - } catch { - return false; - } - return ( - url.username !== "" || - url.password !== "" || - [...url.searchParams.keys()].some((name) => CREDENTIAL_NAME.test(name)) - ); -} - -/** - * Whether anything in the `auth` command reads like a credential: a flag such - * as `--client-secret`, or a variable it sets. The command's own name and - * plain arguments (an audience, a profile) do not. + * Whether the provider signs in with what the user's own Codex home stores: + * a ChatGPT or API-key sign-in, or a gateway's OAuth tokens. A home made for + * one run holds neither. */ -function hasCredentialInAuthCommand(auth: unknown): boolean { - if (!isRecord(auth)) return false; - return Object.entries(auth).some(([field, value]) => { - if (field === "command" || field === "cwd") return false; - if (Array.isArray(value)) { - return value.some((entry) => typeof entry === "string" && CREDENTIAL_NAME.test(entry)); - } - return isRecord(value) && Object.keys(value).some((name) => CREDENTIAL_NAME.test(name)); - }); +function needsStoredSignIn(definition: JsonRecord): boolean { + const oauth = definition.gateway_oauth; + return definition.requires_openai_auth === true || (oauth !== undefined && oauth !== null); } /** - * Whether the definition holds a secret that only the command line could - * carry: Codex reads these settings as plain values only. - */ -function hasSecretOnlyArgumentsCanCarry(definition: JsonRecord): boolean { - const queryParams = definition.query_params; - return ( - (isRecord(queryParams) && - Object.keys(queryParams).some((name) => CREDENTIAL_NAME.test(name))) || - hasCredentialInAddress(definition.base_url) || - hasCredentialInAddress(definition.model_catalog_url) || - hasCredentialInAuthCommand(definition.auth) - ); -} - -/** Every environment variable the definition names for Codex to read. */ -function referencedVariables(definition: JsonRecord): ReadonlyArray { - const envHeaders = isRecord(definition.env_http_headers) - ? Object.values(definition.env_http_headers) - : []; - return [definition.env_key, ...envHeaders].flatMap((name) => nonEmptyString(name) ?? []); -} - -/** Whether a table already names `header`, as HTTP does: whatever its letter case. */ -function findHeader(table: JsonRecord, header: string): string | undefined { - const wanted = header.toLowerCase(); - return Object.keys(table).find((name) => name.toLowerCase() === wanted); -} - -/** - * The provider's definition with its plain secrets taken out and named by - * environment variables instead, as Codex itself would resolve them: - * - * - `env_key` wins over `experimental_bearer_token` (and a missing variable - * is an error, not a fallback), so the token only matters without one. - * - A header set both ways takes its `env_http_headers` value when that - * variable is set, its plain one otherwise. - */ -function withSecretsInEnvironment( - definition: JsonRecord, - runEnvironment: NodeJS.ProcessEnv, -): { readonly definition: JsonRecord; readonly environment: Record } { - const { - experimental_bearer_token: bearerToken, - http_headers: plainHeaders, - ...rest - } = definition; - const next: Record = { ...rest }; - const environment: Record = {}; - // A name of ours must not be one the run already has, or one the definition - // reads: it would hand Codex the wrong value under that name. Compared - // without letter case, as Windows compares variable names. - const taken = new Set( - [...Object.keys(runEnvironment), ...referencedVariables(definition)].map((name) => - name.toUpperCase(), - ), - ); - const allocate = (base: string): string => { - let name = base; - for (let suffix = 1; taken.has(name.toUpperCase()); suffix += 1) { - name = `${base}_${suffix}`; - } - taken.add(name.toUpperCase()); - return name; - }; - - const token = nonEmptyString(bearerToken); - if (token !== undefined && nonEmptyString(rest.env_key) === undefined) { - const variable = allocate(TOKEN_ENV); - next.env_key = variable; - environment[variable] = token; - } - - if (isRecord(plainHeaders)) { - const envHeaders: Record = isRecord(rest.env_http_headers) - ? { ...rest.env_http_headers } - : {}; - for (const [header, value] of Object.entries(plainHeaders)) { - if (typeof value !== "string") continue; - const existing = findHeader(envHeaders, header); - if (existing !== undefined) { - const variable = nonEmptyString(envHeaders[existing]); - if (variable !== undefined && nonEmptyString(runEnvironment[variable]) !== undefined) { - continue; - } - delete envHeaders[existing]; - } - const variable = allocate(HEADER_ENV); - envHeaders[header] = variable; - environment[variable] = value; - } - if (Object.keys(envHeaders).length > 0) { - next.env_http_headers = envHeaders; - } - } - - return { definition: next, environment }; -} - -/** - * The overrides for a `codex exec` run on provider `providerId`. - * `definition` is the provider's entry in `config/read`'s `model_providers`; - * leave it out for a provider Codex has built in, which needs only its id. - * `environment` is the one the run starts with, to tell which of the - * definition's variables are set. Undefined when the definition holds a - * secret that could only travel on the command line. + * What a `codex exec` run on provider `providerId` needs. `definition` is the + * provider's entry in `config/read`'s `model_providers`; leave it out for a + * provider Codex has built in, which needs only its id. Undefined when the + * provider needs the sign-in stored in the user's own Codex home. */ export function codexModelProviderLaunch(input: { readonly providerId: string; readonly model?: string | undefined; readonly definition?: unknown; - readonly environment: NodeJS.ProcessEnv; }): CodexModelProviderLaunch | undefined { + const selectProvider = `model_provider = ${tomlString(input.providerId)}\n`; const base = { providerId: input.providerId, ...(input.model ? { model: input.model } : {}), }; - const selectProvider = ["-c", `model_provider=${tomlString(input.providerId)}`]; if (!isRecord(input.definition)) { - return { ...base, args: selectProvider, environment: {} }; + return { ...base, config: selectProvider }; } - if (hasSecretOnlyArgumentsCanCarry(input.definition)) { + if (needsStoredSignIn(input.definition)) { return undefined; } - const { definition, environment } = withSecretsInEnvironment(input.definition, input.environment); - // The whole table, keyed by the quoted id: a dotted path would split an id - // that has a dot in it. - const table = `{${tomlString(input.providerId)} = ${tomlValue(definition) ?? "{}"}}`; - return { - ...base, - args: [...selectProvider, "-c", `model_providers=${table}`], - environment, - }; + // The whole table on one line, keyed by the quoted id: a `[model_providers.id]` + // header would split an id that has a dot in it. + const table = `{${tomlString(input.providerId)} = ${tomlValue(input.definition) ?? "{}"}}`; + return { ...base, config: `${selectProvider}model_providers = ${table}\n` }; } diff --git a/apps/server/src/textGeneration/CodexTextGeneration.test.ts b/apps/server/src/textGeneration/CodexTextGeneration.test.ts index 4e0ae9175..20319fee2 100644 --- a/apps/server/src/textGeneration/CodexTextGeneration.test.ts +++ b/apps/server/src/textGeneration/CodexTextGeneration.test.ts @@ -272,11 +272,16 @@ function makeFakeCodexBinary( "#!/bin/sh", ...(input.recordInvocationTo !== undefined ? [ - // The arguments one per line, then the provider's secret variables. + // The arguments one per line, the home the run was given, its + // config, and who may read them. // @effect-diagnostics-next-line preferSchemaOverJson:off `printf "%s\n" "$@" > ${JSON.stringify(`${input.recordInvocationTo}.args`)}`, // @effect-diagnostics-next-line preferSchemaOverJson:off - `env | grep "^THREADLINES_CODEX_PROVIDER_" > ${JSON.stringify(`${input.recordInvocationTo}.env`)}`, + `printf "%s" "$CODEX_HOME" > ${JSON.stringify(`${input.recordInvocationTo}.home`)}`, + // @effect-diagnostics-next-line preferSchemaOverJson:off + `cat "$CODEX_HOME/config.toml" > ${JSON.stringify(`${input.recordInvocationTo}.config`)}`, + // @effect-diagnostics-next-line preferSchemaOverJson:off + `ls -ld "$CODEX_HOME" "$CODEX_HOME/config.toml" | cut -c1-10 > ${JSON.stringify(`${input.recordInvocationTo}.modes`)}`, ] : []), 'output_path=""', @@ -482,7 +487,7 @@ function withFakeCodexEnv( failForModelExitCode?: number; stdinMustContain?: string; stdinMustNotContain?: string; - /** POSIX only: writes the run's arguments and provider variables to `.args` and `.env`. */ + /** POSIX only: writes the run's arguments, home, config and their modes to `.*`. */ recordInvocationTo?: string; }, effectFn: (textGeneration: TextGenerationShape) => Effect.Effect, @@ -709,7 +714,7 @@ it.layer(CodexTextGenerationTestLayer)("CodexTextGeneration", (it) => { }), { modelProviderLaunch: Effect.succeed( - codexModelProviderLaunch({ providerId: "acme", model: "acme-coder-1", environment: {} }), + codexModelProviderLaunch({ providerId: "acme", model: "acme-coder-1" }), ), }, ), @@ -742,7 +747,7 @@ it.layer(CodexTextGenerationTestLayer)("CodexTextGeneration", (it) => { }), { modelProviderLaunch: Effect.succeed( - codexModelProviderLaunch({ providerId: "acme", model: "acme-coder-1", environment: {} }), + codexModelProviderLaunch({ providerId: "acme", model: "acme-coder-1" }), ), }, ), @@ -845,10 +850,10 @@ it.layer(CodexTextGenerationTestLayer)("CodexTextGeneration", (it) => { ); // Titles and commit messages leave the user's Codex config out, and with it - // the gateway Codex runs on. The gateway alone is handed back, and a secret - // in its definition travels in the environment, never on the command line. + // the gateway Codex runs on. The run gets a home of its own with the gateway + // alone, in a file only this user can read: its secrets are never arguments. it.effect.skipIf(process.platform === "win32")( - "runs on the gateway Codex is configured for, with its secrets out of the arguments", + "runs on the gateway Codex is configured for, from a private home of its own", () => Effect.gen(function* () { const fs = yield* FileSystem.FileSystem; @@ -861,14 +866,12 @@ it.layer(CodexTextGenerationTestLayer)("CodexTextGeneration", (it) => { name: "Acme Gateway", base_url: "https://llm.acme.example/v1", experimental_bearer_token: "sk-secret-token", - env_key: null, + query_params: { code: "fn-key" }, }, - environment: {}, }); yield* withFakeCodexEnv( { output: JSON.stringify({ title: "Investigate reconnect regressions" }), - requireIgnoreUserConfig: true, recordInvocationTo: record, }, (textGeneration) => @@ -880,13 +883,21 @@ it.layer(CodexTextGenerationTestLayer)("CodexTextGeneration", (it) => { { modelProviderLaunch: Effect.succeed(launch) }, ); - const args = (yield* fs.readFileString(`${record}.args`)).split("\n"); - expect(args).toContain('model_provider="acme"'); - expect(args.some((arg) => arg.startsWith('model_providers={"acme" = {'))).toBe(true); - expect(args.join("\n")).not.toContain("sk-secret-token"); - expect(yield* fs.readFileString(`${record}.env`)).toContain( - "THREADLINES_CODEX_PROVIDER_TOKEN=sk-secret-token", - ); + const args = yield* fs.readFileString(`${record}.args`); + expect(args).not.toContain("sk-secret-token"); + expect(args).not.toContain("fn-key"); + expect(args).not.toContain("acme"); + // The home holds no user config to ignore. + expect(args.split("\n")).not.toContain("--ignore-user-config"); + expect(yield* fs.readFileString(`${record}.config`)).toBe(launch?.config); + expect((yield* fs.readFileString(`${record}.modes`)).split("\n").slice(0, 2)).toEqual([ + "drwx------", + "-rw-------", + ]); + // Gone with the run. + const home = yield* fs.readFileString(`${record}.home`); + expect(home).not.toBe(""); + expect(yield* fs.exists(home)).toBe(false); }).pipe(Effect.scoped), ); diff --git a/apps/server/src/textGeneration/CodexTextGeneration.ts b/apps/server/src/textGeneration/CodexTextGeneration.ts index 5a88ed326..37b4bdc13 100644 --- a/apps/server/src/textGeneration/CodexTextGeneration.ts +++ b/apps/server/src/textGeneration/CodexTextGeneration.ts @@ -92,6 +92,9 @@ function isCodexModelRefusal(error: TextGenerationError): boolean { return /^ERROR:.*\bmodel\b/iu.test(lastLine); } +/** A model id made of the characters model ids are made of, safe as an argument on any platform. */ +const PLAIN_MODEL_NAME = /^[A-Za-z0-9_\-+=.,:/@]+$/u; + function isUnsupportedIgnoreUserConfigFlagError(error: TextGenerationError): boolean { const detail = error.detail.toLowerCase(); return ( @@ -228,6 +231,36 @@ export const makeCodexTextGeneration = Effect.fn("makeCodexTextGeneration")(func return { imagePaths }; }); + /** + * A Codex home for one run on a gateway: a folder only this user can open, + * with the gateway's config in it, removed when the run's scope closes. It + * is named like a side-answer home so that the sweep of those also removes + * one a killed server left behind. + */ + const makeGatewayHome = ( + operation: CodexTextGenerationOperation, + launch: CodexModelProviderLaunch, + ): Effect.Effect => + Effect.gen(function* () { + const home = yield* fileSystem.makeTempDirectoryScoped({ + prefix: "threadlines-side-textgen-", + }); + yield* fileSystem.chmod(home, 0o700); + yield* fileSystem.writeFileString(path.join(home, "config.toml"), launch.config, { + mode: 0o600, + }); + return home; + }).pipe( + Effect.mapError( + (cause) => + new TextGenerationError({ + operation, + detail: "Failed to prepare a Codex home for the gateway", + cause, + }), + ), + ); + const runCodexJson = Effect.fn("runCodexJson")(function* ({ operation, cwd, @@ -262,47 +295,50 @@ export const makeCodexTextGeneration = Effect.fn("makeCodexTextGeneration")(func getModelSelectionStringOptionValue(modelSelection, "reasoningEffort") ?? DEFAULT_GIT_TEXT_GENERATION_REASONING_EFFORT; const serviceTier = resolveCodexCliServiceTier(modelSelection); - const baseEnv = { + // On a gateway the run gets a home of its own, holding the provider + // alone (see codexModelProviderLaunch.ts). That stands in for + // --ignore-user-config: there is no user config in it to ignore. + const gatewayHome = + options.ignoreUserConfig && gatewayLaunch !== undefined + ? yield* makeGatewayHome(operation, gatewayLaunch) + : undefined; + const env = { ...environment, - ...(codexConfig.homePath ? { CODEX_HOME: expandHomePath(codexConfig.homePath) } : {}), - }; - const planFor = (providerLaunch: CodexModelProviderLaunch | undefined) => { - const env = { ...baseEnv, ...providerLaunch?.environment }; - return { - env, - plan: planCliSpawn( - codexConfig.binaryPath || "codex", - [ - "exec", - "--ephemeral", - // One-shot text generation should not inherit agent-oriented user - // config (MCP servers, notify hooks, memories) from - // $CODEX_HOME/config.toml; auth still uses CODEX_HOME. - ...(options.ignoreUserConfig ? ["--ignore-user-config"] : []), - ...(providerLaunch?.args ?? []), - "--skip-git-repo-check", - ...(operation === "generateCommitMessage" ? ["--ignore-rules"] : []), - "-s", - "read-only", - "--model", - modelSelection.model, - "--config", - `model_reasoning_effort="${reasoningEffort}"`, - ...(serviceTier ? ["--config", `service_tier="${serviceTier}"`] : []), - "--output-schema", - schemaPath, - "--output-last-message", - outputPath, - ...imagePaths.flatMap((imagePath) => ["--image", imagePath]), - "-", - ], - env, - ), - }; + ...(gatewayHome !== undefined + ? { CODEX_HOME: gatewayHome } + : codexConfig.homePath + ? { CODEX_HOME: expandHomePath(codexConfig.homePath) } + : {}), }; - // The gateway goes along only beside --ignore-user-config: a run without - // the flag loads the user's own config, provider and all. - const { env, plan } = planFor(options.ignoreUserConfig ? gatewayLaunch : undefined); + const plan = planCliSpawn( + codexConfig.binaryPath || "codex", + [ + "exec", + "--ephemeral", + // One-shot text generation should not inherit agent-oriented user + // config (MCP servers, notify hooks, memories) from + // $CODEX_HOME/config.toml; auth still uses CODEX_HOME. + ...(options.ignoreUserConfig && gatewayHome === undefined + ? ["--ignore-user-config"] + : []), + "--skip-git-repo-check", + ...(operation === "generateCommitMessage" ? ["--ignore-rules"] : []), + "-s", + "read-only", + "--model", + modelSelection.model, + "--config", + `model_reasoning_effort="${reasoningEffort}"`, + ...(serviceTier ? ["--config", `service_tier="${serviceTier}"`] : []), + "--output-schema", + schemaPath, + "--output-last-message", + outputPath, + ...imagePaths.flatMap((imagePath) => ["--image", imagePath]), + "-", + ], + env, + ); const command = ChildProcess.make( plan.command, [...plan.args], @@ -409,26 +445,20 @@ export const makeCodexTextGeneration = Effect.fn("makeCodexTextGeneration")(func >( input: RunCodexJsonInput, ): Effect.fn.Return { - const knownLaunch = textGenerationOptions?.modelProviderLaunch + const providerLaunch = textGenerationOptions?.modelProviderLaunch ? yield* textGenerationOptions.modelProviderLaunch : undefined; - // A batch file that only cmd.exe can start takes its arguments as one - // command line, where the quotes in the gateway's settings would let an - // `&` or a `%NAME%` in them (or in its model's name) act as a command. - // Such a Codex runs without the gateway, as it did before. - const startsThroughShell = - knownLaunch !== undefined && - planCliSpawn(codexConfig.binaryPath || "codex", [], { - ...environment, - ...(codexConfig.homePath ? { CODEX_HOME: expandHomePath(codexConfig.homePath) } : {}), - }).options.shell === true; - const providerLaunch = startsThroughShell ? undefined : knownLaunch; const run = { ...input, providerLaunch }; // On a gateway, the model picked for titles and commit messages may be one // only OpenAI serves (the built-in pick is). The model Codex's config names // is known to work there, so a run refused for its model is tried once // more on that one. - const gatewayModel = providerLaunch?.model; + // The name comes from the user's Codex config and becomes an argument: + // one with anything but plain characters is left alone. + const gatewayModel = + providerLaunch?.model !== undefined && PLAIN_MODEL_NAME.test(providerLaunch.model) + ? providerLaunch.model + : undefined; return yield* runCodexJson(run).pipe( Effect.catchIf( (error) => diff --git a/docs/providers/codex.md b/docs/providers/codex.md index cf4e81d00..ee6679693 100644 --- a/docs/providers/codex.md +++ b/docs/providers/codex.md @@ -92,12 +92,12 @@ later. the model picked for them in `Settings`, Threadlines tries once more with the model from Codex's config. - Those short runs leave the rest of your Codex config out (MCP servers, notify hooks), as they - always have. A token or header written into the provider's entry in `config.toml` reaches them - through their environment, never their command line. -- Two setups keep those short runs on OpenAI, as before. One is a provider with a secret that - only the command line could carry: a password or key in its address or `query_params`, or a - secret among its `auth` command's arguments. The other is, on Windows, a Codex that starts - through a batch file other than npm's own launcher. + always have. Each gets a Codex folder of its own holding the provider's settings alone, + readable only by you and removed when the run ends. Nothing from those settings is passed on + the command line. +- One setup keeps those short runs on OpenAI, as before: a provider that signs in with what + Codex's own folder stores (a ChatGPT sign-in, or a gateway's OAuth). A folder made for one run + has no sign-in. - If Codex can't be asked which provider it is on for 15 minutes, Threadlines stops assuming the gateway until Codex names it again. - A Codex agent answering on the side in a room still runs on OpenAI with your ChatGPT sign-in, From 93177c142437368596736dd798bafce03b7a7abb Mon Sep 17 00:00:00 2001 From: Badcuban <108198679+badcuban@users.noreply.github.com> Date: Sat, 14 Nov 2026 14:46:49 -0500 Subject: [PATCH 3/3] fix(codex): built-in providers keep their sign-in, errors lose the gateway's secrets, and a slow start asks Codex Follow-ups to running one-shot Codex runs on a gateway from a home of their own. - A provider Codex has built in has no definition to hand over and may sign in with what Codex's own home stores: it runs there, named by its id beside --ignore-user-config. Amazon Bedrock and Google Cloud providers, whose credentials Codex keeps in its home too, get no routing, like the ones that need a ChatGPT sign-in or a gateway's OAuth. - Codex's error output can repeat the address of a failed request, and with it a key the address carries. The provider's token, headers, query values and address password are struck from the error before it is logged or shown. - A run that finds no recent answer about the provider no longer goes to OpenAI after waiting out the first status check: it waits five seconds for that check, then asks Codex itself (two reads of its own config, none of a status check's calls to OpenAI). - The sweep of leftover temp homes runs before the first gateway home is made, so one a killed server left does not wait for a room side answer to clear it. --- .../src/provider/Drivers/CodexDriver.ts | 46 +++++-- .../src/provider/Layers/CodexProvider.ts | 42 +++++- .../provider/codexModelProviderLaunch.test.ts | 50 ++++++-- .../src/provider/codexModelProviderLaunch.ts | 120 +++++++++++++++--- .../CodexTextGeneration.test.ts | 76 +++++++++++ .../src/textGeneration/CodexTextGeneration.ts | 34 +++-- docs/providers/codex.md | 9 +- 7 files changed, 329 insertions(+), 48 deletions(-) diff --git a/apps/server/src/provider/Drivers/CodexDriver.ts b/apps/server/src/provider/Drivers/CodexDriver.ts index 63ddddcd4..f6eb741c3 100644 --- a/apps/server/src/provider/Drivers/CodexDriver.ts +++ b/apps/server/src/provider/Drivers/CodexDriver.ts @@ -27,6 +27,7 @@ import * as DateTime from "effect/DateTime"; import * as Deferred from "effect/Deferred"; import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; +import * as Option from "effect/Option"; import * as FileSystem from "effect/FileSystem"; import * as Path from "effect/Path"; import * as Schema from "effect/Schema"; @@ -46,7 +47,10 @@ import { makeCodexAdapter } from "../Layers/CodexAdapter.ts"; import { freshOrCurrentSnapshot } from "../freshProviderSnapshot.ts"; import { checkCodexProviderStatus, + codexGatewayForCheck, currentCodexModelProvider, + isCodexModelProviderKnown, + probeCodexGateway, type CodexModelProviderMemory, type CodexSecurityNoticeMemory, type CodexWorkspaceMemory, @@ -88,10 +92,12 @@ const DRIVER_KIND = ProviderDriverKind.make("codex"); const SNAPSHOT_REFRESH_INTERVAL = Duration.minutes(5); /** * How long a title or commit message waits for the instance's first status - * check, which normally takes a second or two. Past it the run goes ahead - * without the check's answer rather than hold the user's commit. + * check, which normally takes a second or two. Past it the run asks Codex + * itself which provider it is on, rather than hold the user's commit. */ -const FIRST_CHECK_WAIT = Duration.seconds(20); +const FIRST_CHECK_WAIT = Duration.seconds(5); +/** How long that question may take: two reads of Codex's own config. */ +const GATEWAY_PROBE_TIMEOUT = Duration.seconds(10); const WINDOWS_NATIVE_INSTALL = makeWindowsNativeInstaller({ url: "https://chatgpt.com/codex/install.ps1", lockKey: "codex-native-win32", @@ -203,17 +209,35 @@ export const CodexDriver: ProviderDriver = { // spawner-availability failures surfaced from `checkCodexProviderStatus` // below. // Filled in by each status check; titles and commit messages read it - // at the moment they run. The first check starts with the instance, and - // a run asked for before it ends waits for it: without its answer the - // run would go to OpenAI, gateway or not. + // at the moment they run. Without an answer they would go to OpenAI, + // gateway or not, so a run that finds none waits a moment for the first + // check (it starts with the instance) and then asks Codex itself. const modelProviderMemory: CodexModelProviderMemory = {}; const firstCheckEnded = yield* Deferred.make(); + const modelProviderLaunch = Effect.gen(function* () { + yield* Deferred.await(firstCheckEnded).pipe(Effect.timeoutOption(FIRST_CHECK_WAIT)); + const nowMs = yield* Clock.currentTimeMillis; + if (!isCodexModelProviderKnown(modelProviderMemory, nowMs)) { + const reads = yield* probeCodexGateway({ + binaryPath: effectiveConfig.binaryPath, + ...(effectiveConfig.homePath ? { homePath: effectiveConfig.homePath } : {}), + cwd: process.cwd(), + environment: processEnv, + }).pipe( + Effect.scoped, + Effect.timeoutOption(GATEWAY_PROBE_TIMEOUT), + Effect.map(Option.getOrUndefined), + Effect.orElseSucceed(() => undefined), + Effect.provideService(ChildProcessSpawner.ChildProcessSpawner, spawner), + ); + if (reads) { + codexGatewayForCheck(reads, modelProviderMemory, nowMs); + } + } + return currentCodexModelProvider(modelProviderMemory, nowMs)?.launch; + }); const textGeneration = yield* makeCodexTextGeneration(effectiveConfig, processEnv, { - modelProviderLaunch: Deferred.await(firstCheckEnded).pipe( - Effect.timeoutOption(FIRST_CHECK_WAIT), - Effect.andThen(Clock.currentTimeMillis), - Effect.map((nowMs) => currentCodexModelProvider(modelProviderMemory, nowMs)?.launch), - ), + modelProviderLaunch, }); // Build a managed snapshot whose settings never change — mutations come diff --git a/apps/server/src/provider/Layers/CodexProvider.ts b/apps/server/src/provider/Layers/CodexProvider.ts index fb6d0d7a0..80a814747 100644 --- a/apps/server/src/provider/Layers/CodexProvider.ts +++ b/apps/server/src/provider/Layers/CodexProvider.ts @@ -996,6 +996,8 @@ export function withCodexConfigModel( * `currentCodexModelProvider`, which lets it lapse. */ export interface CodexModelProviderMemory { + /** When Codex last said which providers it has, whatever the answer. */ + answeredAtMs?: number | undefined; last?: | { readonly providerId: string; @@ -1028,6 +1030,43 @@ export function currentCodexModelProvider( return memory.last; } +/** + * Whether Codex said recently enough which providers it has for the memory to + * be gone by. Not so right after a start, or while its status checks fail. + */ +export function isCodexModelProviderKnown( + memory: CodexModelProviderMemory, + nowMs: number, +): boolean { + return ( + memory.answeredAtMs !== undefined && + nowMs - memory.answeredAtMs <= CODEX_MODEL_PROVIDER_MEMORY_MS + ); +} + +/** + * Asks Codex only which provider its chats run on: two reads of its own + * config, with none of a status check's calls to OpenAI. For a title or + * commit message that cannot wait out a slow check and must not guess. + * Undefined when Codex does not say. + */ +export const probeCodexGateway = Effect.fn("probeCodexGateway")(function* (input: { + readonly binaryPath: string; + readonly homePath?: string; + readonly cwd: string; + readonly environment?: NodeJS.ProcessEnv; +}) { + const client = yield* makeCodexAppServerClient(input); + yield* initializeCodexAppServerClient(client); + const modelProviders = yield* readCodexModelProviders(client); + if (!modelProviders) return undefined; + const gateway = yield* readCodexGatewayConfig(client, modelProviders); + return { modelProviders, ...(gateway ? { gateway } : {}) } satisfies Pick< + CodexAppServerProviderSnapshot, + "modelProviders" | "gateway" + >; +}); + /** * The gateway a check goes by, remembered for the next one. A check that * could not ask Codex which providers it has says nothing, so what was known @@ -1035,12 +1074,13 @@ export function currentCodexModelProvider( * what was known of that same provider. Codex's own answer that it is on * OpenAI forgets it at once. */ -function codexGatewayForCheck( +export function codexGatewayForCheck( snapshot: Pick, memory: CodexModelProviderMemory, nowMs: number, ): CodexModelProviderMemory["last"] { if (snapshot.modelProviders === undefined) return currentCodexModelProvider(memory, nowMs); + memory.answeredAtMs = nowMs; const gateway = snapshot.gateway; if (!gateway) { memory.last = undefined; diff --git a/apps/server/src/provider/codexModelProviderLaunch.test.ts b/apps/server/src/provider/codexModelProviderLaunch.test.ts index 5a81526c8..e74234475 100644 --- a/apps/server/src/provider/codexModelProviderLaunch.test.ts +++ b/apps/server/src/provider/codexModelProviderLaunch.test.ts @@ -1,6 +1,9 @@ import { describe, expect, it } from "vite-plus/test"; -import { codexModelProviderLaunch } from "./codexModelProviderLaunch.ts"; +import { + codexModelProviderLaunch, + redactCodexProviderSecrets, +} from "./codexModelProviderLaunch.ts"; describe("codexModelProviderLaunch", () => { it("writes a config with the provider alone, its definition whole", () => { @@ -15,11 +18,11 @@ describe("codexModelProviderLaunch", () => { model_catalog_url: null, experimental_bearer_token: "sk-secret-token", http_headers: { "X-Org": "org-secret" }, - query_params: { "api-version": "2026-01-01", code: "fn-key" }, + query_params: { "api-version": "2026-01-01", code: "fn-key-secret" }, request_max_retries: 2, requires_openai_auth: false, gateway_oauth: null, - auth: { command: "acme-token", args: ["--client-secret", "s3cret"] }, + auth: { command: "acme-token", args: ["--audience", "llm"] }, }, }); @@ -28,27 +31,58 @@ describe("codexModelProviderLaunch", () => { model: "acme-coder-1", config: [ 'model_provider = "acme.gw v2"', - 'model_providers = {"acme.gw v2" = {"name" = "Acme \\"GW\\"", "base_url" = "https://llm.acme.example/v1", "wire_api" = "responses", "experimental_bearer_token" = "sk-secret-token", "http_headers" = {"X-Org" = "org-secret"}, "query_params" = {"api-version" = "2026-01-01", "code" = "fn-key"}, "request_max_retries" = 2, "requires_openai_auth" = false, "auth" = {"command" = "acme-token", "args" = ["--client-secret", "s3cret"]}}}', + 'model_providers = {"acme.gw v2" = {"name" = "Acme \\"GW\\"", "base_url" = "https://llm.acme.example/v1", "wire_api" = "responses", "experimental_bearer_token" = "sk-secret-token", "http_headers" = {"X-Org" = "org-secret"}, "query_params" = {"api-version" = "2026-01-01", "code" = "fn-key-secret"}, "request_max_retries" = 2, "requires_openai_auth" = false, "auth" = {"command" = "acme-token", "args" = ["--audience", "llm"]}}}', "", ].join("\n"), + // Longest first, so one is never struck out from inside another. + secrets: ["sk-secret-token", "fn-key-secret", "org-secret", "2026-01-01"], }); }); - // A run's own home has no sign-in: these providers use the one stored in - // the user's Codex home, so their runs stay as they were. + // A run's own home has no sign-in: these providers use what the user's + // Codex home stores, so their runs stay as they were. it("gives no launch for a provider that needs Codex's stored sign-in", () => { for (const definition of [ { base_url: "https://proxy.acme.example/v1", requires_openai_auth: true }, { base_url: "https://llm.acme.example/v1", gateway_oauth: { issuer: "https://id.acme" } }, + { aws: { profile: "work", region: "us-east-1" } }, + { project_id: "acme-ml", location: "us-central1" }, ]) { expect(codexModelProviderLaunch({ providerId: "acme", definition })).toBeUndefined(); } }); - it("names a built-in provider by its id alone", () => { + // No definition to hand over: it runs in the user's own home, where any + // sign-in Codex keeps for it is, named beside --ignore-user-config. + it("names a built-in provider by its id alone, as an argument", () => { expect(codexModelProviderLaunch({ providerId: "ollama" })).toEqual({ providerId: "ollama", - config: 'model_provider = "ollama"\n', + args: ["-c", 'model_provider="ollama"'], + secrets: [], }); + expect(codexModelProviderLaunch({ providerId: 'odd" & id' })).toBeUndefined(); + }); + + // Codex's errors repeat the address of the request that failed. + it("strikes the provider's secrets from text, also where an address encodes them", () => { + const launch = codexModelProviderLaunch({ + providerId: "acme", + definition: { + base_url: "https://user:p%40ssw0rd@llm.acme.example/v1?sig=abc123def", + query_params: { code: "fn key/secret" }, + experimental_bearer_token: "sk-secret-token", + }, + }); + expect( + redactCodexProviderSecrets( + "ERROR: 401 for https://llm.acme.example/v1/responses?code=fn%20key%2Fsecret&sig=abc123def (Bearer sk-secret-token, p@ssw0rd)", + launch, + ), + ).toBe( + "ERROR: 401 for https://llm.acme.example/v1/responses?code=[redacted]&sig=[redacted] (Bearer [redacted], [redacted])", + ); + expect(redactCodexProviderSecrets("ERROR: nothing to strike", undefined)).toBe( + "ERROR: nothing to strike", + ); }); }); diff --git a/apps/server/src/provider/codexModelProviderLaunch.ts b/apps/server/src/provider/codexModelProviderLaunch.ts index a02afc51d..b3f43ea4a 100644 --- a/apps/server/src/provider/codexModelProviderLaunch.ts +++ b/apps/server/src/provider/codexModelProviderLaunch.ts @@ -15,23 +15,38 @@ * address), which is why it travels in a file only its owner can read and * never as `-c` arguments, which show in the process list. * - * A home of its own has no sign-in. A provider that signs in through Codex's - * stored sign-in (`requires_openai_auth`, or a gateway's OAuth) therefore - * gets no launch, and its runs go to OpenAI as they did before. + * A home of its own has no sign-in. A provider that signs in with what + * Codex's home stores (`requires_openai_auth`, a gateway's OAuth, Amazon + * Bedrock, Google Cloud) therefore gets no launch, and its runs go to OpenAI + * as they did before. A provider Codex has built in has no definition to + * hand over: it runs in the user's home, named by its id alone. * * @module codexModelProviderLaunch */ -/** What a `codex exec` run needs to use the provider. */ +/** What a `codex exec` run needs to use the provider: `config` or `args`, never both. */ export interface CodexModelProviderLaunch { readonly providerId: string; /** The model Codex's config names: the one known to work on this provider. */ readonly model?: string; /** - * The text of the run's own `config.toml`. It can hold the provider's - * secrets: keep it out of logs, arguments and anything sent to a client. + * The text of the `config.toml` for a home of the run's own. It can hold + * the provider's secrets: keep it out of logs, arguments and anything sent + * to a client. */ - readonly config: string; + readonly config?: string; + /** + * For a provider Codex has built in: arguments beside `--ignore-user-config` + * in the user's own home. Only the provider's id, never a secret. + */ + readonly args?: ReadonlyArray; + /** + * The definition's values that may be secret (its token, headers, query + * parameters, a password in its address). Codex's error output can repeat + * them, in a request's address say: strike them from anything shown or + * logged with `redactCodexProviderSecrets`. + */ + readonly secrets: ReadonlyArray; } type JsonRecord = { readonly [key: string]: unknown }; @@ -79,32 +94,101 @@ function tomlValue(value: unknown): string | undefined { /** * Whether the provider signs in with what the user's own Codex home stores: - * a ChatGPT or API-key sign-in, or a gateway's OAuth tokens. A home made for - * one run holds neither. + * a ChatGPT or API-key sign-in, a gateway's OAuth tokens, or the Amazon + * Bedrock or Google Cloud credentials Codex keeps there. A home made for one + * run holds none of them. */ function needsStoredSignIn(definition: JsonRecord): boolean { - const oauth = definition.gateway_oauth; - return definition.requires_openai_auth === true || (oauth !== undefined && oauth !== null); + const set = (value: unknown) => value !== undefined && value !== null; + return ( + definition.requires_openai_auth === true || + set(definition.gateway_oauth) || + set(definition.aws) || + set(definition.project_id) || + set(definition.location) || + set(definition.credential_file) + ); +} + +/** Shorter than this, a value is not a secret worth striking, and striking it would mangle text. */ +const MIN_SECRET_LENGTH = 6; + +function stringValues(table: unknown): ReadonlyArray { + return isRecord(table) + ? Object.values(table).filter((value): value is string => typeof value === "string") + : []; +} + +/** A password, user name or query values carried in an address. */ +function addressSecrets(value: unknown): ReadonlyArray { + if (typeof value !== "string") return []; + try { + const url = new URL(value); + return [ + decodeURIComponent(url.username), + decodeURIComponent(url.password), + ...url.searchParams.values(), + ]; + } catch { + return []; + } } +/** The definition's values that may be secret, longest first so one never hides inside another. */ +function definitionSecrets(definition: JsonRecord): ReadonlyArray { + const candidates = [ + ...(typeof definition.experimental_bearer_token === "string" + ? [definition.experimental_bearer_token] + : []), + ...stringValues(definition.http_headers), + ...stringValues(definition.query_params), + ...addressSecrets(definition.base_url), + ...addressSecrets(definition.model_catalog_url), + ]; + return [...new Set(candidates.filter((value) => value.length >= MIN_SECRET_LENGTH))].toSorted( + (left, right) => right.length - left.length, + ); +} + +/** + * Text with the provider's secrets struck out, also where they appear + * percent-encoded, as in the address of a failed request. + */ +export function redactCodexProviderSecrets( + text: string, + launch: Pick | undefined, +): string { + let redacted = text; + for (const secret of launch?.secrets ?? []) { + for (const form of new Set([secret, encodeURIComponent(secret)])) { + redacted = redacted.replaceAll(form, "[redacted]"); + } + } + return redacted; +} + +/** A built-in provider's id, made of plain characters: safe as an argument on any platform. */ +const PLAIN_PROVIDER_ID = /^[A-Za-z0-9_-]+$/u; + /** * What a `codex exec` run on provider `providerId` needs. `definition` is the * provider's entry in `config/read`'s `model_providers`; leave it out for a - * provider Codex has built in, which needs only its id. Undefined when the - * provider needs the sign-in stored in the user's own Codex home. + * provider Codex has built in. Undefined when the provider needs the sign-in + * stored in the user's own Codex home. */ export function codexModelProviderLaunch(input: { readonly providerId: string; readonly model?: string | undefined; readonly definition?: unknown; }): CodexModelProviderLaunch | undefined { - const selectProvider = `model_provider = ${tomlString(input.providerId)}\n`; const base = { providerId: input.providerId, ...(input.model ? { model: input.model } : {}), }; if (!isRecord(input.definition)) { - return { ...base, config: selectProvider }; + return PLAIN_PROVIDER_ID.test(input.providerId) + ? { ...base, args: ["-c", `model_provider=${tomlString(input.providerId)}`], secrets: [] } + : undefined; } if (needsStoredSignIn(input.definition)) { return undefined; @@ -112,5 +196,9 @@ export function codexModelProviderLaunch(input: { // The whole table on one line, keyed by the quoted id: a `[model_providers.id]` // header would split an id that has a dot in it. const table = `{${tomlString(input.providerId)} = ${tomlValue(input.definition) ?? "{}"}}`; - return { ...base, config: `${selectProvider}model_providers = ${table}\n` }; + return { + ...base, + config: `model_provider = ${tomlString(input.providerId)}\nmodel_providers = ${table}\n`, + secrets: definitionSecrets(input.definition), + }; } diff --git a/apps/server/src/textGeneration/CodexTextGeneration.test.ts b/apps/server/src/textGeneration/CodexTextGeneration.test.ts index 20319fee2..2f02ea444 100644 --- a/apps/server/src/textGeneration/CodexTextGeneration.test.ts +++ b/apps/server/src/textGeneration/CodexTextGeneration.test.ts @@ -901,6 +901,82 @@ it.layer(CodexTextGenerationTestLayer)("CodexTextGeneration", (it) => { }).pipe(Effect.scoped), ); + // Codex's error names the address of the request that failed, and this text + // is logged and shown to the user. + it.effect("strikes the gateway's secrets from a failed run's error", () => + withFakeCodexEnv( + { + output: JSON.stringify({ title: "unused" }), + exitCode: 1, + stderr: + "ERROR: unexpected status 503 for https://llm.acme.example/v1/responses?code=fn-key-secret", + }, + (textGeneration) => + Effect.gen(function* () { + const result = yield* textGeneration + .generateThreadTitle({ + cwd: process.cwd(), + message: "Please investigate websocket reconnect regressions.", + modelSelection: DEFAULT_TEST_MODEL_SELECTION, + }) + .pipe(Effect.result); + + expect(Result.isFailure(result)).toBe(true); + if (Result.isFailure(result)) { + expect(result.failure.detail).toContain("responses?code=[redacted]"); + expect(result.failure.detail).not.toContain("fn-key-secret"); + } + }), + { + modelProviderLaunch: Effect.succeed( + codexModelProviderLaunch({ + providerId: "acme", + definition: { + base_url: "https://llm.acme.example/v1", + query_params: { code: "fn-key-secret" }, + }, + }), + ), + }, + ), + ); + + // A provider Codex has built in has no definition to hand over, and may + // sign in with what Codex's own home stores: it runs there, named by id. + it.effect.skipIf(process.platform === "win32")( + "runs a built-in provider in the user's own home, named beside --ignore-user-config", + () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const recordDir = yield* fs.makeTempDirectoryScoped({ prefix: "threadlines-codex-args-" }); + const record = path.join(recordDir, "run"); + yield* withFakeCodexEnv( + { + output: JSON.stringify({ title: "Investigate reconnect regressions" }), + requireIgnoreUserConfig: true, + recordInvocationTo: record, + }, + (textGeneration) => + textGeneration.generateThreadTitle({ + cwd: process.cwd(), + message: "Please investigate websocket reconnect regressions.", + modelSelection: DEFAULT_TEST_MODEL_SELECTION, + }), + { + modelProviderLaunch: Effect.succeed(codexModelProviderLaunch({ providerId: "ollama" })), + }, + ); + + const args = (yield* fs.readFileString(`${record}.args`)).split("\n"); + expect(args).toContain('model_provider="ollama"'); + // No home of its own was made for it. + expect(yield* fs.readFileString(`${record}.home`)).not.toContain( + "threadlines-side-textgen-", + ); + }).pipe(Effect.scoped), + ); + it.effect("retries without --ignore-user-config when the codex CLI rejects the flag", () => withFakeCodexEnv( { diff --git a/apps/server/src/textGeneration/CodexTextGeneration.ts b/apps/server/src/textGeneration/CodexTextGeneration.ts index 37b4bdc13..aa8816332 100644 --- a/apps/server/src/textGeneration/CodexTextGeneration.ts +++ b/apps/server/src/textGeneration/CodexTextGeneration.ts @@ -20,7 +20,11 @@ import { sanitizeBranchFragment, sanitizeFeatureBranchName } from "@threadlines/ import { resolveAttachmentPath } from "../attachmentStore.ts"; import { ServerConfig } from "../config.ts"; import { expandHomePath } from "../pathExpansion.ts"; -import type { CodexModelProviderLaunch } from "../provider/codexModelProviderLaunch.ts"; +import { + redactCodexProviderSecrets, + type CodexModelProviderLaunch, +} from "../provider/codexModelProviderLaunch.ts"; +import { sweepStaleCodexSideAnswerHomes } from "../provider/codexSideAnswerHome.ts"; import { TextGenerationError } from "@threadlines/contracts"; import { type BranchNameGenerationInput, @@ -234,19 +238,24 @@ export const makeCodexTextGeneration = Effect.fn("makeCodexTextGeneration")(func /** * A Codex home for one run on a gateway: a folder only this user can open, * with the gateway's config in it, removed when the run's scope closes. It - * is named like a side-answer home so that the sweep of those also removes - * one a killed server left behind. + * is named like a side-answer home so that the sweep of those, run here + * before the first one is made, also removes any a killed server left. */ + let sweptGatewayHomes = false; const makeGatewayHome = ( operation: CodexTextGenerationOperation, - launch: CodexModelProviderLaunch, + config: string, ): Effect.Effect => Effect.gen(function* () { + if (!sweptGatewayHomes) { + sweptGatewayHomes = true; + yield* sweepStaleCodexSideAnswerHomes.pipe(Effect.forkDetach); + } const home = yield* fileSystem.makeTempDirectoryScoped({ prefix: "threadlines-side-textgen-", }); yield* fileSystem.chmod(home, 0o700); - yield* fileSystem.writeFileString(path.join(home, "config.toml"), launch.config, { + yield* fileSystem.writeFileString(path.join(home, "config.toml"), config, { mode: 0o600, }); return home; @@ -299,8 +308,8 @@ export const makeCodexTextGeneration = Effect.fn("makeCodexTextGeneration")(func // alone (see codexModelProviderLaunch.ts). That stands in for // --ignore-user-config: there is no user config in it to ignore. const gatewayHome = - options.ignoreUserConfig && gatewayLaunch !== undefined - ? yield* makeGatewayHome(operation, gatewayLaunch) + options.ignoreUserConfig && gatewayLaunch?.config !== undefined + ? yield* makeGatewayHome(operation, gatewayLaunch.config) : undefined; const env = { ...environment, @@ -318,8 +327,10 @@ export const makeCodexTextGeneration = Effect.fn("makeCodexTextGeneration")(func // One-shot text generation should not inherit agent-oriented user // config (MCP servers, notify hooks, memories) from // $CODEX_HOME/config.toml; auth still uses CODEX_HOME. + // A built-in provider is named beside the flag, in the user's own + // home: it has no definition to give a home of its own. ...(options.ignoreUserConfig && gatewayHome === undefined - ? ["--ignore-user-config"] + ? ["--ignore-user-config", ...(gatewayLaunch?.args ?? [])] : []), "--skip-git-repo-check", ...(operation === "generateCommitMessage" ? ["--ignore-rules"] : []), @@ -376,7 +387,12 @@ export const makeCodexTextGeneration = Effect.fn("makeCodexTextGeneration")(func if (exitCode !== 0) { const stderrDetail = stderr.trim(); const stdoutDetail = stdout.trim(); - const detail = stderrDetail.length > 0 ? stderrDetail : stdoutDetail; + // Codex's errors can repeat a gateway's address, and with it a key the + // address carries. This text is logged and shown to the user. + const detail = redactCodexProviderSecrets( + stderrDetail.length > 0 ? stderrDetail : stdoutDetail, + gatewayLaunch, + ); return yield* new TextGenerationError({ operation, detail: diff --git a/docs/providers/codex.md b/docs/providers/codex.md index ee6679693..ca9c10315 100644 --- a/docs/providers/codex.md +++ b/docs/providers/codex.md @@ -95,9 +95,12 @@ later. always have. Each gets a Codex folder of its own holding the provider's settings alone, readable only by you and removed when the run ends. Nothing from those settings is passed on the command line. -- One setup keeps those short runs on OpenAI, as before: a provider that signs in with what - Codex's own folder stores (a ChatGPT sign-in, or a gateway's OAuth). A folder made for one run - has no sign-in. +- One setup keeps those short runs on OpenAI, as before: a provider you add that signs in with + what Codex's own folder stores (a ChatGPT sign-in, a gateway's OAuth, Amazon Bedrock or Google + Cloud credentials). A folder made for one run has no sign-in. A provider built into Codex runs + in Codex's own folder, so its sign-in is there. +- If such a run fails, the error shown has the provider's token, headers and address keys struck + out. - If Codex can't be asked which provider it is on for 15 minutes, Threadlines stops assuming the gateway until Codex names it again. - A Codex agent answering on the side in a room still runs on OpenAI with your ChatGPT sign-in,