From c6a05c95a8eac8b8d14f6f937818364038ed1fd0 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 02:22:52 +0900 Subject: [PATCH 1/2] fix(codex): remove injected routing after the Codex app rewrites config.toml #1798. When the Codex app rewrites config.toml after injection, the journal's exact-bytes restore stops matching and the fallback strip is all that is left. That fallback recognized an injected `openai_base_url` only by the marker COMMENT on the preceding line -- and a reserializing app writer keeps every value while dropping every comment. So the proxy URL stopped being recognized as ours, `removeCodexConfig` reported "opencodex not present in Codex config", and plain Codex was left pointing at a dead 127.0.0.1:10100. The marker was formatting evidence for an ownership question. Record the value instead: `markJournalInjectedState` now stores the exact root `openai_base_url` the injection wrote, and the fallback strips a root URL whose value equals it. The match is deliberately exact rather than "any loopback-looking URL". Stripping by shape would delete a gateway the user configured themselves, which is why `hasInjectedOpenaiBaseUrl` was conservative in the first place; keying on what we recorded writing keeps that guarantee while surviving the rewrite. Verification: new `tests/codex-restore-app-rewrite.test.ts` injects, reserializes the config exactly as an app writer does (values kept, comments dropped), then restores. Driven red by pinning the journal accessor to null, which reproduces the issue's surviving `127.0.0.1:10100` line. Its second case covers the mirror-image risk: a user's own pre-injection `openai_base_url` still survives restore. `bun x tsc --noEmit` clean; 68 tests green across journal, inject, catalog-restore, and CLI restore suites. Not addressed here: the `models_cache.json` half of the issue. Catalog restore re-resolves its target from the post-rewrite TOML, so a config that dropped `model_catalog_json` never reaches the proxy-written cache. That needs the injected catalog path threaded through restore and is left for its own change. --- src/codex/inject.ts | 18 ++++- src/codex/injected-marker.ts | 28 +++++++ src/codex/journal.ts | 26 +++++- tests/codex-restore-app-rewrite.test.ts | 100 ++++++++++++++++++++++++ 4 files changed, 168 insertions(+), 4 deletions(-) create mode 100644 tests/codex-restore-app-rewrite.test.ts diff --git a/src/codex/inject.ts b/src/codex/inject.ts index 0a02e3dd10..5a64f7b29f 100644 --- a/src/codex/inject.ts +++ b/src/codex/inject.ts @@ -30,6 +30,7 @@ import { } from "./user-identity"; import { markJournalInjectedState, + journaledInjectedOpenaiBaseUrl, removeJournal, restoreJournalState, writeJournal, @@ -52,6 +53,7 @@ import { providerTableStart, providerTableString, rootTomlString, + stripJournaledOpenaiBaseUrl, tomlStringPattern, } from "./injected-marker"; import { @@ -1138,12 +1140,18 @@ interface StripOpencodexConfigResult { */ function stripOpencodexConfigResult( content: string, + journaledBaseUrl: string | null = null, ): StripOpencodexConfigResult { let out = content; const hadRootOcxProvider = readRootTomlString(out, "model_provider") === "opencodex"; - const hadInjectedBaseUrl = hasInjectedOpenaiBaseUrl(out); + // #1798: marker adjacency is FORMATTING evidence, and a Codex app rewrite keeps values + // while dropping comments. Fall back to VALUE evidence -- the exact URL we recorded + // writing -- so an app-rewritten config is still recognized as ours. + const hadInjectedBaseUrl = hasInjectedOpenaiBaseUrl(out) + || (journaledBaseUrl !== null && rootTomlString(out, "openai_base_url") === journaledBaseUrl); out = stripInjectedOpenaiBaseUrl(out); // before removeOcxSection — it keys on the marker line too + out = stripJournaledOpenaiBaseUrl(out, journaledBaseUrl); if (out.includes("[model_providers.opencodex]")) { out = removeOcxSection(out); } @@ -1195,8 +1203,12 @@ export function removeCodexConfig( // The unchanged fast path compares in LF space so an untouched file is never rewritten. const eol = dominantEol(rawContent); const content = applyEol(rawContent, "\n"); - const had = hasOpencodexRouting(content); - const stripped = stripOpencodexConfigResult(content); + // Read the recorded injection once: the strip below consumes it, and so does the + // ownership verdict, which must agree with what was actually removed. + const journaledBaseUrl = journaledInjectedOpenaiBaseUrl(); + const had = hasOpencodexRouting(content) + || (journaledBaseUrl !== null && rootTomlString(content, "openai_base_url") === journaledBaseUrl); + const stripped = stripOpencodexConfigResult(content, journaledBaseUrl); if (had || stripped.content !== content) { atomicWriteFile(CODEX_CONFIG_PATH, applyEol(stripped.content, eol)); } diff --git a/src/codex/injected-marker.ts b/src/codex/injected-marker.ts index 42ca16ddd0..f69d1343ae 100644 --- a/src/codex/injected-marker.ts +++ b/src/codex/injected-marker.ts @@ -50,6 +50,34 @@ export function providerTableString(content: string, provider: string, key: stri return null; } +/** + * Drop a root `openai_base_url` whose VALUE is the one a recorded injection wrote. + * + * #1798: the marker-adjacency rule below is formatting evidence, and the Codex app + * reserializes the file -- values kept, comments dropped. This rule is value evidence + * instead, so it still recognizes our URL after that rewrite. It is deliberately an + * EXACT value match against what we recorded writing: a user gateway we never wrote + * cannot match, so restore can never delete a URL that was not ours. + */ +export function stripJournaledOpenaiBaseUrl(content: string, injectedUrl: string | null): string { + if (!injectedUrl) return content; + const lines = content.split(String.fromCharCode(10)); + const firstTable = lines.findIndex(l => /^\s*\[/.test(l)); + const rootEnd = firstTable === -1 ? lines.length : firstTable; + const drop = new Set(); + for (let i = 0; i < rootEnd; i++) { + const line = lines[i]!; + if (!isRootOpenaiBaseUrlLine(line)) continue; + if (rootTomlString(line, "openai_base_url") !== injectedUrl) continue; + drop.add(i); + // Take an ownership marker directly above it too, so repeated cycles cannot + // accumulate orphaned comments. + if (i > 0 && lines[i - 1]!.includes(OCX_SECTION_MARKER)) drop.add(i - 1); + } + if (drop.size === 0) return content; + return lines.filter((_, i) => !drop.has(i)).join(String.fromCharCode(10)); +} + export function hasInjectedOpenaiBaseUrl(content: string): boolean { const lines = content.split("\n"); const firstTable = lines.findIndex(l => /^\s*\[/.test(l)); diff --git a/src/codex/journal.ts b/src/codex/journal.ts index 910861cd3d..e697e11f46 100644 --- a/src/codex/journal.ts +++ b/src/codex/journal.ts @@ -2,7 +2,7 @@ import { createHash } from "node:crypto"; import { existsSync, readFileSync, unlinkSync } from "node:fs"; import { join } from "node:path"; import { atomicWriteFile } from "../config"; -import { hasInjectedCodexRouting } from "./injected-marker"; +import { hasInjectedCodexRouting, rootTomlString } from "./injected-marker"; import { CODEX_HOME, CODEX_CONFIG_PATH, CODEX_PROFILE_PATH } from "./paths"; /** @@ -22,6 +22,15 @@ interface Journal { originalProfile: string | null; injectedConfigHash?: string; injectedProfileHash?: string | null; + /** + * The exact root `openai_base_url` this injection wrote, when it wrote one. + * + * #1798: ownership used to be inferred from a marker COMMENT on the preceding line, + * which a reserializing Codex app deletes while keeping the value. Recording the value + * we actually wrote makes ownership provable from evidence rather than from formatting, + * and it is what lets restore tell OUR loopback URL apart from a gateway the user set. + */ + injectedOpenaiBaseUrl?: string | null; pid: number; timestamp: string; } @@ -96,9 +105,24 @@ export function markJournalInjectedState(config: string, profile: string | null) if (journal.injectedConfigHash) return; journal.injectedConfigHash = sha256(config) ?? undefined; journal.injectedProfileHash = sha256(profile); + // Read from the bytes we are about to install, not from the file: another writer may + // already have rewritten it, and then the recorded value would describe their config. + journal.injectedOpenaiBaseUrl = rootTomlString(config, "openai_base_url"); atomicWriteFile(JOURNAL_PATH, JSON.stringify(journal)); } +/** + * The root `openai_base_url` the last injection wrote, or null when it wrote none. + * + * #1798: the fallback strip recognizes an injected URL by the marker COMMENT above it, + * and a Codex app rewrite keeps values while dropping comments. This is the evidence that + * survives such a rewrite, so restore can still prove the URL is ours -- and, just as + * importantly, prove that a DIFFERENT URL is not. + */ +export function journaledInjectedOpenaiBaseUrl(): string | null { + return readJournal()?.injectedOpenaiBaseUrl ?? null; +} + export function removeJournal(): void { try { unlinkSync(JOURNAL_PATH); } catch { /* ignore */ } } diff --git a/tests/codex-restore-app-rewrite.test.ts b/tests/codex-restore-app-rewrite.test.ts new file mode 100644 index 0000000000..c991a4df8e --- /dev/null +++ b/tests/codex-restore-app-rewrite.test.ts @@ -0,0 +1,100 @@ +import { describe, expect, test, beforeEach, afterEach } from "bun:test"; +import { mkdtempSync, rmSync, writeFileSync, readFileSync } from "node:fs"; +import { spawnSync } from "node:child_process"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; + +/** + * #1798: the Codex app rewrites config.toml AFTER injection, so the journal's + * exact-bytes restore no longer fires and the fallback strip is the only thing left. + * That fallback recognizes an injected `openai_base_url` ONLY by the marker comment + * on the line above it. An app rewrite reserializes the file and drops the comment, so + * the proxy URL stops being recognized as ours and survives `ocx stop` / `ocx restore` + * while the command reports success -- leaving plain Codex pointed at a dead port. + * + * These tests reproduce that state literally: inject, drop every comment the way a TOML + * reserializer would (values kept, comments gone), then restore. + */ + +const repoRoot = dirname(fileURLToPath(new URL("../package.json", import.meta.url))); + +/** Inject, simulate the app's comment-dropping rewrite, then restore. */ +const INJECT_REWRITE_RESTORE = [ + 'const fs = require("fs");', + 'const path = require("path");', + 'const { injectCodexConfig, restoreNativeCodex } = require("./src/codex/inject");', + "(async () => {", + " await injectCodexConfig(10100, {", + " port: 10100,", + " providers: {},", + ' defaultProvider: "openai",', + ' injectionModel: "gpt-5.6-sol",', + ' injectionEffort: "high",', + " }, { catalogPath: null });", + ' const configPath = path.join(process.env.CODEX_HOME, "config.toml");', + ' const injected = fs.readFileSync(configPath, "utf8");', + " // Exactly what a reserializing app writer produces: every VALUE survives,", + " // every COMMENT -- including our ownership marker -- is gone.", + " const rewritten = injected", + " .split(String.fromCharCode(10))", + ' .filter(line => !line.trim().startsWith("#"))', + " .join(String.fromCharCode(10));", + ' fs.writeFileSync(configPath, rewritten, "utf8");', + " const result = restoreNativeCodex();", + " console.log(JSON.stringify({ success: result.success, message: result.message }));", + "})();", +].join("\n"); + +function runScript(codexHome: string, script: string): { stdout: string; stderr: string; status: number } { + const result = spawnSync(process.execPath, ["--eval", script], { + cwd: repoRoot, + env: { ...process.env, CODEX_HOME: codexHome }, + encoding: "utf8", + }); + return { stdout: result.stdout?.trim() ?? "", stderr: result.stderr?.trim() ?? "", status: result.status ?? 1 }; +} + +describe("#1798 restore after the Codex app rewrites the config", () => { + let testDir: string; + + beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), "ocx-1798-")); + }); + + afterEach(() => { + rmSync(testDir, { recursive: true, force: true }); + }); + + test("an unmarked injected openai_base_url is still removed", () => { + writeFileSync(join(testDir, "config.toml"), '# original config\nmodel = "gpt-5.5"\n', "utf8"); + + const r = runScript(testDir, INJECT_REWRITE_RESTORE); + if (r.status !== 0) throw new Error(r.stderr || r.stdout); + + const restored = readFileSync(join(testDir, "config.toml"), "utf8"); + // The reported defect: the proxy URL survives, so plain Codex talks to a dead port. + expect(restored).not.toContain("openai_base_url"); + expect(restored).not.toContain("127.0.0.1:10100"); + // The user's own pre-injection content is still theirs. + expect(restored).toContain("gpt-5.5"); + }); + + test("a user's own openai_base_url written before injection is preserved", () => { + // The mirror-image risk of the fix: stripping ANY unmarked openai_base_url would + // delete a URL we never wrote. The journaled baseline is the arbiter, not the key name. + writeFileSync( + join(testDir, "config.toml"), + 'openai_base_url = "https://my-own-gateway.example/v1"\nmodel = "gpt-5.5"\n', + "utf8", + ); + + const r = runScript(testDir, INJECT_REWRITE_RESTORE); + if (r.status !== 0) throw new Error(r.stderr || r.stdout); + + const restored = readFileSync(join(testDir, "config.toml"), "utf8"); + expect(restored).toContain("https://my-own-gateway.example/v1"); + expect(restored).not.toContain("127.0.0.1:10100"); + }); +}); + From 6cd5b04b3d5b92cc62d53b1545d58819aaebfd68 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 17 Aug 2026 02:28:56 +0900 Subject: [PATCH 2/2] fix(codex): restore the catalog we actually wrote, not the one the config now names The second half of #1798. Catalog restore resolved its target with `readCodexCatalogPath()`, which re-reads `model_catalog_json` from the CURRENT config. After a Codex app rewrite drops that key, restore walks to the default catalog while the routed `models_cache.json` we really wrote keeps every `provider/slug` entry -- which is why the model picker still listed routed models after `ocx restore` reported success. Record the injected catalog path in the journal and pass it to `restoreCodexCatalogWithPermit` as an explicit target, falling back to today's resolution when nothing was recorded. The capture has to happen in the CALLER, before the config half runs: a successful journal restore deletes the journal and a config restore can remove `model_catalog_json`, so reading it inside the catalog step would be too late in both directions. Both restore entry points capture it up front. Verification: a third case in tests/codex-restore-app-rewrite.test.ts injects with an explicit cache path, drops `model_catalog_json` the way a rewrite does, and asserts the routed entries are gone from that exact file. Driven red by pinning the resolution back to `readCodexCatalogPath()`, which leaves `opencode-go/deepseek-v4-flash` in place. 69 tests green across the journal, inject, catalog-restore and CLI restore suites; `bun x tsc --noEmit` clean. --- src/codex/catalog/sync.ts | 10 +++++- src/codex/inject.ts | 27 ++++++++++++--- src/codex/journal.ts | 15 +++++++++ tests/codex-restore-app-rewrite.test.ts | 44 +++++++++++++++++++++++++ 4 files changed, 91 insertions(+), 5 deletions(-) diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts index 1275dbe2e6..4303f51cf2 100644 --- a/src/codex/catalog/sync.ts +++ b/src/codex/catalog/sync.ts @@ -1686,8 +1686,16 @@ export async function syncCatalogModels(config: OcxConfig): Promise revalidateDesiredState && shouldSyncCodexOnStart(loadConfig()) ? null - : restoreCodexCatalogWithPermit(permit, owningCodexHome)); + : restoreCodexCatalogWithPermit(permit, owningCodexHome, journaledCatalogPath)); return restored.kind === "completed" && restored.value !== null ? { state: "ok", changed: restored.value.removed > 0, ...restored.value, message: "Codex catalog restored." } : restored.kind === "completed" @@ -1447,6 +1458,10 @@ export async function restoreNativeCodexAsync( integrationRecord: () => readIntegrationRecord(), }); + // Captured before the config half: a successful journal restore DELETES the journal, and + // restoring the config can drop `model_catalog_json`. Either one would hide the routed + // catalog we actually wrote (#1798). + const journaledCatalogPath = journaledInjectedCatalogPath(); let config: CodexRestoreConfigResult; let transitionReceipt: { nativeGeneration: number; currentTxId: string } | undefined; @@ -1523,7 +1538,7 @@ export async function restoreNativeCodexAsync( config = restoreCodexConfigInline(); } - const catalog = restoreCodexCatalogArtifact(options.revalidateDesiredState === true); + const catalog = restoreCodexCatalogArtifact(options.revalidateDesiredState === true, journaledCatalogPath); const outcome = await runCodexHistoryJob({ ...resolveCodexHistoryJobTarget(), ...(options.revalidateDesiredState ? { expectedDesiredEnabled: false } : {}), @@ -1573,8 +1588,12 @@ export function restoreNativeCodex(options: { skipHistory?: boolean; revalidateD if (options.revalidateDesiredState && shouldSyncCodexOnStart(loadConfig())) { return desiredEnabledRestoreSkip(); } + // Captured before the config half: a successful journal restore DELETES the journal, and + // restoring the config can drop `model_catalog_json`. Either one would hide the routed + // catalog we actually wrote (#1798). + const journaledCatalogPath = journaledInjectedCatalogPath(); const config = restoreCodexConfigInline(); - const catalog = restoreCodexCatalogArtifact(options.revalidateDesiredState === true); + const catalog = restoreCodexCatalogArtifact(options.revalidateDesiredState === true, journaledCatalogPath); // Design B (loopback) steady state: threads are already tagged openai, so prove the // no-op with a readonly probe instead of write-opening a DB the Codex app may hold // (Windows: WAL writer lock -> seconds of stalling + a false warning on every stop). diff --git a/src/codex/journal.ts b/src/codex/journal.ts index e697e11f46..a1a34806c9 100644 --- a/src/codex/journal.ts +++ b/src/codex/journal.ts @@ -31,6 +31,15 @@ interface Journal { * and it is what lets restore tell OUR loopback URL apart from a gateway the user set. */ injectedOpenaiBaseUrl?: string | null; + /** + * The catalog path this injection actually wrote to. + * + * #1798: restore re-resolves the catalog from the CURRENT config, so a Codex app rewrite + * that dropped `model_catalog_json` sends restore to the default catalog while the + * proxy-written one is left routed. The injected path is the only durable record of which + * file we actually touched. + */ + injectedCatalogPath?: string | null; pid: number; timestamp: string; } @@ -108,6 +117,7 @@ export function markJournalInjectedState(config: string, profile: string | null) // Read from the bytes we are about to install, not from the file: another writer may // already have rewritten it, and then the recorded value would describe their config. journal.injectedOpenaiBaseUrl = rootTomlString(config, "openai_base_url"); + journal.injectedCatalogPath = rootTomlString(config, "model_catalog_json"); atomicWriteFile(JOURNAL_PATH, JSON.stringify(journal)); } @@ -123,6 +133,11 @@ export function journaledInjectedOpenaiBaseUrl(): string | null { return readJournal()?.injectedOpenaiBaseUrl ?? null; } +/** The catalog path the last injection wrote to, or null when none was recorded. */ +export function journaledInjectedCatalogPath(): string | null { + return readJournal()?.injectedCatalogPath ?? null; +} + export function removeJournal(): void { try { unlinkSync(JOURNAL_PATH); } catch { /* ignore */ } } diff --git a/tests/codex-restore-app-rewrite.test.ts b/tests/codex-restore-app-rewrite.test.ts index c991a4df8e..9792b8e146 100644 --- a/tests/codex-restore-app-rewrite.test.ts +++ b/tests/codex-restore-app-rewrite.test.ts @@ -46,6 +46,34 @@ const INJECT_REWRITE_RESTORE = [ "})();", ].join("\n"); +/** Inject with an explicit catalog path, drop `model_catalog_json` the way a rewrite does, then restore. */ +const CATALOG_REWRITE_RESTORE = [ + 'const fs = require("fs");', + 'const path = require("path");', + 'const { injectCodexConfig, restoreNativeCodex } = require("./src/codex/inject");', + "(async () => {", + ' const cachePath = path.join(process.env.CODEX_HOME, "models_cache.json");', + " // The catalog file itself is written by catalog sync, which needs network state this", + " // test has no business standing up. Seed it directly: what is under test is WHICH file", + " // restore targets, not how sync populates it.", + ' fs.writeFileSync(cachePath, JSON.stringify({ models: [{ slug: "gpt-5.5" }, { slug: "opencode-go/deepseek-v4-flash" }] }), "utf8");', + " await injectCodexConfig(10100, {", + " port: 10100,", + " providers: {},", + ' defaultProvider: "openai",', + ' injectionModel: "gpt-5.6-sol",', + ' injectionEffort: "high",', + " }, { catalogPath: cachePath });", + ' const configPath = path.join(process.env.CODEX_HOME, "config.toml");', + ' const rewritten = fs.readFileSync(configPath, "utf8")', + " .split(String.fromCharCode(10))", + ' .filter(line => !line.trim().startsWith("#") && !line.includes("model_catalog_json"))', + " .join(String.fromCharCode(10));", + ' fs.writeFileSync(configPath, rewritten, "utf8");', + " const result = restoreNativeCodex();", + " console.log(JSON.stringify({ success: result.success, catalog: result.artifacts.catalog.path }));", + "})();", +].join(String.fromCharCode(10)); function runScript(codexHome: string, script: string): { stdout: string; stderr: string; status: number } { const result = spawnSync(process.execPath, ["--eval", script], { cwd: repoRoot, @@ -96,5 +124,21 @@ describe("#1798 restore after the Codex app rewrites the config", () => { expect(restored).toContain("https://my-own-gateway.example/v1"); expect(restored).not.toContain("127.0.0.1:10100"); }); + + test("the routed catalog we wrote is restored even when the rewrite dropped model_catalog_json", () => { + // The catalog half of #1798. Restore used to re-resolve its target from the CURRENT + // config, so a rewrite that removed `model_catalog_json` sent it to the default catalog + // while the proxy-written models_cache.json kept every routed entry. + writeFileSync(join(testDir, "config.toml"), 'model = "gpt-5.5"' + String.fromCharCode(10), "utf8"); + + const r = runScript(testDir, CATALOG_REWRITE_RESTORE); + if (r.status !== 0) throw new Error(r.stderr || r.stdout); + + const cachePath = join(testDir, "models_cache.json"); + const cache = JSON.parse(readFileSync(cachePath, "utf8")); + const routed = (cache.models ?? []).filter((m: { slug?: string }) => typeof m.slug === "string" && m.slug.includes("/")); + expect(routed).toEqual([]); + expect(JSON.parse(r.stdout).catalog).toBe(cachePath); + }); });