diff --git a/src/codex/inject.ts b/src/codex/inject.ts index e137d1567b..e7e123c6e3 100644 --- a/src/codex/inject.ts +++ b/src/codex/inject.ts @@ -894,7 +894,15 @@ export async function injectCodexConfig( }); atomicWriteFile(CODEX_CONFIG_PATH, content); atomicWriteFile(CODEX_PROFILE_PATH, profileContent); - markJournalInjectedState(content, profileContent); + markJournalInjectedState(content, profileContent, { + // A root override is ours only in loopback Design B when no user-owned value won. + injectedOpenaiBaseUrl: legacyMode || keptUserBaseUrl + ? null + : rootTomlString(content, "openai_base_url"), + // This is the catalog artifact selected for this injection, even when config.toml + // already points at that path and therefore needs no textual rewrite. + injectedCatalogPath: catalogPath, + }); }; /* diff --git a/src/codex/journal.ts b/src/codex/journal.ts index a1a34806c9..b8923a6daa 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, rootTomlString } from "./injected-marker"; +import { hasInjectedCodexRouting } from "./injected-marker"; import { CODEX_HOME, CODEX_CONFIG_PATH, CODEX_PROFILE_PATH } from "./paths"; /** @@ -108,16 +108,30 @@ export function writeJournal(options: WriteJournalOptions = {}): void { atomicWriteFile(JOURNAL_PATH, JSON.stringify(journal)); } -export function markJournalInjectedState(config: string, profile: string | null): void { +export interface InjectedJournalOwnership { + injectedOpenaiBaseUrl: string | null; + injectedCatalogPath: string | null; +} + +export function markJournalInjectedState( + config: string, + profile: string | null, + ownership: InjectedJournalOwnership, +): void { const journal = readJournal(); if (!journal) return; - if (journal.injectedConfigHash) return; - journal.injectedConfigHash = sha256(config) ?? undefined; + // The first exact injected config is the only safe whole-snapshot restore boundary for + // the first native snapshot. A later reinjection may preserve user edits made while routed; + // hashing those newer bytes and restoring the first snapshot would delete those edits. + // Keep the first hash so changed/reinjected configs take the owned-field fallback path. + journal.injectedConfigHash ??= sha256(config) ?? undefined; + // The profile file is wholly generated by OpenCodex, so its latest exact hash remains safe + // to refresh and lets restore remove the latest generated profile after a port change. 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"); - journal.injectedCatalogPath = rootTomlString(config, "model_catalog_json"); + // Only the caller knows which values it actually owns. Deriving these from the final TOML + // would mistake a preserved user override for injected routing. + journal.injectedOpenaiBaseUrl = ownership.injectedOpenaiBaseUrl; + journal.injectedCatalogPath = ownership.injectedCatalogPath; atomicWriteFile(JOURNAL_PATH, JSON.stringify(journal)); } diff --git a/tests/codex-journal.test.ts b/tests/codex-journal.test.ts index 827ddd29d4..b1e5e1fbce 100644 --- a/tests/codex-journal.test.ts +++ b/tests/codex-journal.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test, beforeEach, afterEach, setDefaultTimeout } from "bun:test"; +import { createHash } from "node:crypto"; import { mkdtempSync, rmSync, writeFileSync, readFileSync, existsSync } from "node:fs"; import { spawnSync } from "node:child_process"; import { tmpdir } from "node:os"; @@ -553,7 +554,10 @@ describe("codex-journal", () => { const r = runScript(testDir, ` const { markJournalInjectedState } = require("./src/codex/journal"); - markJournalInjectedState("# injected\\n", null); + markJournalInjectedState("# injected\\n", null, { + injectedOpenaiBaseUrl: null, + injectedCatalogPath: null, + }); console.log(String(process.pid)); `); expect(r.status).toBe(0); @@ -564,6 +568,38 @@ describe("codex-journal", () => { expect(typeof second.injectedConfigHash).toBe("string"); // marked by the second }); + test("reinjection keeps the first config hash while refreshing owned route and catalog", () => { + const r = runScript(testDir, ` + const fs = require("fs"); + const path = require("path"); + const { writeJournal, markJournalInjectedState } = require("./src/codex/journal"); + const journalPath = path.join(process.env.CODEX_HOME, "opencodex-journal.json"); + writeJournal(); + markJournalInjectedState("# first injection\\n", null, { + injectedOpenaiBaseUrl: "http://127.0.0.1:10100/v1", + injectedCatalogPath: "first-catalog.json", + }); + const first = JSON.parse(fs.readFileSync(journalPath, "utf8")); + markJournalInjectedState("# second injection\\n", "# second profile\\n", { + injectedOpenaiBaseUrl: "http://127.0.0.1:10200/v1", + injectedCatalogPath: "second-catalog.json", + }); + const second = JSON.parse(fs.readFileSync(journalPath, "utf8")); + console.log(JSON.stringify({ firstHash: first.injectedConfigHash, secondHash: second.injectedConfigHash })); + `); + expect(r.status).toBe(0); + const hashes = JSON.parse(r.stdout) as { firstHash: string; secondHash: string }; + expect(typeof hashes.firstHash).toBe("string"); + expect(hashes.firstHash).toBe(createHash("sha256").update("# first injection\n").digest("hex")); + expect(hashes.secondHash).toBe(hashes.firstHash); + + const journal = JSON.parse(readFileSync(join(testDir, "opencodex-journal.json"), "utf8")); + expect(Buffer.from(journal.originalConfig, "base64").toString("utf8")).toContain("# original config"); + expect(journal.injectedOpenaiBaseUrl).toBe("http://127.0.0.1:10200/v1"); + expect(journal.injectedCatalogPath).toBe("second-catalog.json"); + expect(typeof journal.injectedProfileHash).toBe("string"); + }); + test("writeJournal() with no options still snapshots a native config", () => { const r = runScript(testDir, `require("./src/codex/journal").writeJournal(); console.log("written");`); expect(r.status).toBe(0); diff --git a/tests/codex-restore-app-rewrite.test.ts b/tests/codex-restore-app-rewrite.test.ts index 21cea8dce4..2e37309e18 100644 --- a/tests/codex-restore-app-rewrite.test.ts +++ b/tests/codex-restore-app-rewrite.test.ts @@ -39,7 +39,7 @@ const INJECT_REWRITE_RESTORE = [ " const rewritten = injected", " .split(String.fromCharCode(10))", ' .filter(line => !line.trim().startsWith("#"))', - " .join(String.fromCharCode(10));", + " .join(String.fromCharCode(10)) + String.fromCharCode(10) + '# app rewrite';", ' fs.writeFileSync(configPath, rewritten, "utf8");', " const result = restoreNativeCodex();", " console.log(JSON.stringify({ success: result.success, message: result.message }));", @@ -74,6 +74,60 @@ const CATALOG_REWRITE_RESTORE = [ " console.log(JSON.stringify({ success: result.success, catalog: result.artifacts.catalog.path }));", "})();", ].join(String.fromCharCode(10)); + +/** Reinject with a new route and catalog, then expose the durable ownership record. */ +const REINJECT_AND_READ_JOURNAL = [ + 'const fs = require("fs");', + 'const path = require("path");', + 'const { injectCodexConfig } = require("./src/codex/inject");', + "(async () => {", + ' const firstCatalog = path.join(process.env.CODEX_HOME, "first-catalog.json");', + ' const secondCatalog = path.join(process.env.CODEX_HOME, "second-catalog.json");', + " const config = {", + " port: 10100,", + " providers: {},", + ' defaultProvider: "openai",', + ' injectionModel: "gpt-5.6-sol",', + ' injectionEffort: "high",', + " };", + " await injectCodexConfig(10100, config, { catalogPath: firstCatalog });", + " await injectCodexConfig(10200, { ...config, port: 10200 }, { catalogPath: secondCatalog });", + ' const journal = JSON.parse(fs.readFileSync(path.join(process.env.CODEX_HOME, "opencodex-journal.json"), "utf8"));', + " console.log(JSON.stringify({ url: journal.injectedOpenaiBaseUrl, catalog: journal.injectedCatalogPath }));", + "})();", +].join(String.fromCharCode(10)); + +/** Preserve a user edit made after the first injection across reinjection and restore. */ +const REINJECT_AFTER_USER_EDIT_RESTORE = [ + 'const fs = require("fs");', + 'const path = require("path");', + 'const { injectCodexConfig, restoreNativeCodex } = require("./src/codex/inject");', + "(async () => {", + " const config = {", + " port: 10100,", + " providers: {},", + ' defaultProvider: "openai",', + ' injectionModel: "gpt-5.6-sol",', + ' injectionEffort: "high",', + " };", + " await injectCodexConfig(10100, config, { catalogPath: null });", + ' const configPath = path.join(process.env.CODEX_HOME, "config.toml");', + ' fs.appendFileSync(configPath, String.fromCharCode(10) + \'approval_policy = "never"\' + String.fromCharCode(10), "utf8");', + " await injectCodexConfig(10200, { ...config, port: 10200 }, { catalogPath: null });", + ' const beforeRestore = fs.readFileSync(configPath, "utf8");', + " const result = restoreNativeCodex({ skipHistory: true });", + ' const afterRestore = fs.readFileSync(configPath, "utf8");', + ' const profileExistsAfterRestore = fs.existsSync(path.join(process.env.CODEX_HOME, "opencodex.config.toml"));', + " console.log(JSON.stringify({", + " success: result.success,", + " action: result.artifacts.config.action,", + " beforeRestore,", + " afterRestore,", + " profileExistsAfterRestore,", + " }));", + "})();", +].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, @@ -109,8 +163,9 @@ describe("#1798 restore after the Codex app rewrites the config", () => { }, 15_000); test("a user's own openai_base_url written before injection is preserved", () => { + // Force a byte mismatch so exact journal restore cannot hide a fallback ownership bug. // 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. + // delete a URL we never wrote. The journaled ownership evidence is the arbiter. writeFileSync( join(testDir, "config.toml"), 'openai_base_url = "https://my-own-gateway.example/v1"\nmodel = "gpt-5.5"\n', @@ -125,6 +180,41 @@ describe("#1798 restore after the Codex app rewrites the config", () => { expect(restored).not.toContain("127.0.0.1:10100"); }, 15_000); + test("reinjection refreshes the owned route and catalog recorded for restore", () => { + writeFileSync(join(testDir, "config.toml"), 'model = "gpt-5.5"\n', "utf8"); + + const r = runScript(testDir, REINJECT_AND_READ_JOURNAL); + if (r.status !== 0) throw new Error(r.stderr || r.stdout); + + const recorded = JSON.parse(r.stdout) as { url: string; catalog: string }; + expect(recorded.url).toBe("http://127.0.0.1:10200/v1"); + expect(recorded.catalog).toBe(join(testDir, "second-catalog.json")); + }, 20_000); + + test("a user setting added after first injection survives reinjection and restore", () => { + writeFileSync(join(testDir, "config.toml"), 'model = "gpt-5.5"\n', "utf8"); + + const r = runScript(testDir, REINJECT_AFTER_USER_EDIT_RESTORE); + if (r.status !== 0) throw new Error(r.stderr || r.stdout); + + const result = JSON.parse(r.stdout) as { + success: boolean; + action: string; + beforeRestore: string; + afterRestore: string; + profileExistsAfterRestore: boolean; + }; + expect(result.success).toBe(true); + expect(result.action).toBe("owned-fields-stripped"); + expect(result.beforeRestore).toContain('approval_policy = "never"'); + expect(result.beforeRestore).toContain("127.0.0.1:10200"); + expect(result.afterRestore).toContain('approval_policy = "never"'); + expect(result.afterRestore).toContain('model = "gpt-5.5"'); + expect(result.afterRestore).not.toContain("openai_base_url"); + expect(result.afterRestore).not.toContain("127.0.0.1:10200"); + expect(result.profileExistsAfterRestore).toBe(false); + }, 20_000); + 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