From 9c9ca092a19b8e21c8d4b8aebaa91c10f1b4a3d2 Mon Sep 17 00:00:00 2001 From: Ingwannu Date: Tue, 11 Aug 2026 06:48:36 +0000 Subject: [PATCH] fix(codex): preflight sync injection before catalog writes --- .../content/docs/reference/cli/lifecycle.md | 5 ++ src/codex/inject.ts | 15 +++- src/codex/sync.ts | 29 +++++++- structure/02_config-and-codex-home.md | 14 ++++ tests/cli-restore-back.test.ts | 10 ++- tests/codex-sync-api.test.ts | 73 +++++++++++++++++++ 6 files changed, 142 insertions(+), 4 deletions(-) diff --git a/docs-site/src/content/docs/reference/cli/lifecycle.md b/docs-site/src/content/docs/reference/cli/lifecycle.md index ccf2bc18e..ca7c772ec 100644 --- a/docs-site/src/content/docs/reference/cli/lifecycle.md +++ b/docs-site/src/content/docs/reference/cli/lifecycle.md @@ -180,6 +180,11 @@ not fabricate official-client metadata. Doctor never mutates credentials or appl Fetch the live model list from every configured provider and re-inject the merged catalog into Codex. Run it after adding a provider or to refresh available models. +Before provider discovery or catalog/cache replacement, `ocx sync` validates that the managed +Codex configuration can be injected. If that validation refuses the config, the command exits +nonzero, prints the concrete reason on stderr, and leaves the existing catalog and cache unchanged. +`ocx restore back` uses the same no-write preflight before it re-enables routing. + If long-lived Codex `app-server` processes are still running, `ocx sync` warns that they may keep serving the previous in-memory model list even though `opencodex-catalog.json` / `models_cache.json` were updated. Pass `--restart-codex` to send `SIGTERM` only to matching `codex … app-server` and diff --git a/src/codex/inject.ts b/src/codex/inject.ts index 6ab07183e..7d5b516ab 100644 --- a/src/codex/inject.ts +++ b/src/codex/inject.ts @@ -129,6 +129,12 @@ export interface InjectCodexOptions { * caller that is willing to wait can raise it. */ lockTimeoutMs?: number; + /** + * Validate the same config transformations and write-coordination eligibility without + * changing the journal, config, profile, catalog, cache, or history. Sync uses this before + * provider discovery so a deterministic config refusal cannot degrade an existing catalog. + */ + validateOnly?: boolean; } function configuredManagedSubagentDefaults( @@ -660,7 +666,7 @@ export async function injectCodexConfig( if (activeProvider) { // A launcher may have journaled before the provider manager took ownership. Never let shutdown // replay that stale snapshot over externally managed config. - removeJournal(); + if (!options.validateOnly) removeJournal(); const nativeSubagentDefaultsWarning = configuredManagedSubagentDefaults( config, ) @@ -867,6 +873,13 @@ export async function injectCodexConfig( }; } + if (options.validateOnly) { + return { + success: true, + message: "Codex config injection preflight passed; no files were changed.", + }; + } + const applyNativeArtifacts = (): void => { writeJournal({ currentStateIsNative: !hasInjectedCodexRouting(rawContent), diff --git a/src/codex/sync.ts b/src/codex/sync.ts index 955db3804..43b1ce898 100644 --- a/src/codex/sync.ts +++ b/src/codex/sync.ts @@ -101,7 +101,8 @@ export async function syncModelsToCodex( const externalProvider = (deps.currentExternalCodexModelProvider ?? currentExternalCodexModelProvider)(); if (externalProvider) { const result = await deps.injectCodexConfig(p, config, {}); - log?.log(result.message); + if (result.success) log?.log(result.message); + else log?.error(result.message); reportCodexHomeTarget(log, deps.collectCodexHomeDiagnostic ?? collectOrcaCodexHomeDiagnostic); return { status: "applied", @@ -116,6 +117,29 @@ export async function syncModelsToCodex( }; } + // Injection has deterministic refusal paths (for example an ambiguous marker-owned TOML + // table) that do not depend on provider discovery. Exercise the SAME transformation and + // coordination eligibility before catalog gathering: a known-bad config must not turn a + // working catalog/cache into the partial result of an otherwise unnecessary refresh. + const preflight = await deps.injectCodexConfig(p, config, { validateOnly: true }); + if (!preflight.success) { + log?.error(preflight.message); + reportCodexHomeTarget(log, deps.collectCodexHomeDiagnostic ?? collectOrcaCodexHomeDiagnostic); + return { + status: "applied", + ok: false, + added: 0, + catalogPath: null, + catalogExists: false, + catalogWritten: false, + cacheSynced: false, + message: preflight.message, + ...(preflight.nativeSubagentDefaultsWarning + ? { nativeSubagentDefaultsWarning: preflight.nativeSubagentDefaultsWarning } + : {}), + }; + } + applyProxyEnv(config); // `ocx ensure`/`ocx sync` fetch provider models outside the server process let added = 0; let catalogPath: string | null = null; @@ -168,7 +192,8 @@ export async function syncModelsToCodex( message: result.message, }; } - log?.log(result.message); + if (result.success) log?.log(result.message); + else log?.error(result.message); reportCodexHomeTarget(log, deps.collectCodexHomeDiagnostic ?? collectOrcaCodexHomeDiagnostic); const projectConfigWarnings = printProjectCodexConfigWarnings(log, { cwd: process.cwd() }); return { diff --git a/structure/02_config-and-codex-home.md b/structure/02_config-and-codex-home.md index 9d3c8440c..62d30f012 100644 --- a/structure/02_config-and-codex-home.md +++ b/structure/02_config-and-codex-home.md @@ -186,6 +186,20 @@ provider managers own that routing configuration, and replacing their provider i otherwise intact Codex sessions. This ownership check must run before catalog/cache refresh, journal creation, and the background history migration guardian. +`ocx sync` and `ocx restore back` run the injector's non-writing preflight before provider +discovery or catalog/cache replacement. Deterministic config and ownership refusals therefore +leave the existing catalog and cache untouched, and their concrete messages are emitted on stderr. +The real injection still revalidates under its normal write boundary after catalog convergence; +the preflight is an early no-write guard, not an authorization token for a later write. + +[Decision Log] +- 목적과 의도: Prevent a refused Codex config injection from degrading a previously usable model catalog and make the refusal actionable from the CLI. +- 기존 구현 및 제약 조건: Catalog discovery and replacement ran before injection, while the injector alone owned the authoritative TOML transforms and write-coordination eligibility checks. +- 검토한 주요 대안: Roll back catalog and cache bytes after a later refusal, duplicate a partial TOML validator in the CLI, or run the injector's existing planning path without committing before discovery. +- 선택한 방식: Add a non-writing mode to the injector and call it before catalog work; keep the normal injector call as the final under-lock authority check. +- 다른 대안 대신 이 방식을 선택한 이유: Post-hoc rollback can overwrite a concurrent catalog writer, and a second validator would drift from the real refusal rules. Reusing the injector keeps one policy path and avoids compensating writes. +- 장점, 단점 및 영향: Deterministic refusals preserve catalog/cache bytes and print their reason on stderr. A concurrent state change can still make the final injection refuse, but catalog and injection retain their existing independent revalidation and serialization boundaries. + `supports_websockets = true` is appended to the provider table only when `websocketsEnabled(config)` returns true. diff --git a/tests/cli-restore-back.test.ts b/tests/cli-restore-back.test.ts index 93d958388..8acaa301f 100644 --- a/tests/cli-restore-back.test.ts +++ b/tests/cli-restore-back.test.ts @@ -132,6 +132,12 @@ describe("ocx restore back", () => { defaultProvider: "fixture", checkForUpdates: false, }), "utf8"); + const catalogPath = join(codexHome, "opencodex-catalog.json"); + const cachePath = join(codexHome, "models_cache.json"); + const catalogBefore = '{"models":[{"slug":"fixture/keep-me"}]}\n'; + const cacheBefore = '{"models":[{"slug":"fixture/cached-keep-me"}],"fetched_at":1}\n'; + writeFileSync(catalogPath, catalogBefore, "utf8"); + writeFileSync(cachePath, cacheBefore, "utf8"); const result = runCli(["sync"], { ...ownedEnvironment(codexHome, ocxHome), @@ -141,8 +147,10 @@ describe("ocx restore back", () => { }); expect(result.status).toBe(1); - expect(`${result.stdout}\n${result.stderr}`).toContain("Codex config injection refused"); + expect(result.stderr).toContain("Codex config injection refused"); expect(result.stderr).toContain("Codex sync did not complete"); + expect(readFileSync(catalogPath, "utf8")).toBe(catalogBefore); + expect(readFileSync(cachePath, "utf8")).toBe(cacheBefore); } finally { rmSync(codexHome, { recursive: true, force: true }); rmSync(ocxHome, { recursive: true, force: true }); diff --git a/tests/codex-sync-api.test.ts b/tests/codex-sync-api.test.ts index 1ec9f790a..8ec58fbc7 100644 --- a/tests/codex-sync-api.test.ts +++ b/tests/codex-sync-api.test.ts @@ -123,6 +123,76 @@ describe("GUI/CLI Codex sync backend", () => { expect(errors).toEqual([]); }); + test("refuses during injection preflight before catalog or cache mutation", async () => { + let refreshCalls = 0; + let injectCalls = 0; + const logs: string[] = []; + const errors: string[] = []; + const refusal = "Codex config injection refused: ambiguous managed defaults; inspect config.toml."; + + const result = await syncModelsToCodex(12345, config, { + log: line => logs.push(String(line)), + error: line => errors.push(String(line)), + }, { + admitCodexWrite: admittedSync, + refreshCodexModelCatalog: async () => { + refreshCalls++; + throw new Error("catalog refresh must not run after a deterministic refusal"); + }, + injectCodexConfig: async (_port, _config, options) => { + injectCalls++; + expect(options.validateOnly).toBe(true); + return { success: false, message: refusal }; + }, + currentExternalCodexModelProvider: () => null, + collectCodexHomeDiagnostic: () => homeDiagnostic(), + }); + + expect(injectCalls).toBe(1); + expect(refreshCalls).toBe(0); + expect(result).toEqual({ + status: "applied", + ok: false, + added: 0, + catalogPath: null, + catalogExists: false, + catalogWritten: false, + cacheSynced: false, + message: refusal, + }); + expect(logs).toEqual([" Target Codex home: C:\\Users\\[USER]\\.codex"]); + expect(errors).toEqual([refusal]); + }); + + test("the real successful injection preflight writes no Codex artifacts", () => { + const configPath = join(TEST_CODEX_HOME, "config.toml"); + const profilePath = join(TEST_CODEX_HOME, "opencodex.config.toml"); + const journalPath = join(TEST_CODEX_HOME, "opencodex-journal.json"); + const before = readFileSync(configPath, "utf8"); + + const child = spawnSync(process.execPath, ["-e", ` + const { injectCodexConfig } = await import("./src/codex/inject.ts"); + const result = await injectCodexConfig(10100, ${JSON.stringify(config)}, { validateOnly: true }); + console.log(JSON.stringify(result)); + `], { + cwd: repoRoot, + env: { + ...process.env, + HOME: TEST_HOME, + USERPROFILE: TEST_HOME, + CODEX_HOME: TEST_CODEX_HOME, + OPENCODEX_HOME: TEST_OCX_HOME, + }, + encoding: "utf8", + }); + + expect(child.status).toBe(0); + expect(JSON.parse(child.stdout.trim())).toMatchObject({ success: true }); + expect(readFileSync(configPath, "utf8")).toBe(before); + expect(existsSync(profilePath)).toBe(false); + expect(existsSync(journalPath)).toBe(false); + }); + test("returns a policy skip without touching the catalog or config", async () => { let refreshed = false; let injected = false; @@ -280,6 +350,7 @@ describe("GUI/CLI Codex sync backend", () => { test("keeps injection fallback behavior when catalog refresh throws", async () => { let injectedCatalogPath: string | null | undefined = "unset"; + let injectionCalls = 0; const result = await syncModelsToCodex(undefined, config, null, { admitCodexWrite: admittedSync, @@ -287,12 +358,14 @@ describe("GUI/CLI Codex sync backend", () => { throw new Error("catalog boom"); }, injectCodexConfig: async (_port, _config, options) => { + injectionCalls++; injectedCatalogPath = options.catalogPath; return { success: true, message: "injected fallback" }; }, currentExternalCodexModelProvider: () => null, }); + expect(injectionCalls).toBe(2); expect(injectedCatalogPath).toBeUndefined(); expect(result.ok).toBe(true); expect(result.catalogPath).toBeNull();