fix(codex): report catalog and cache write signals

lidge-jun · web-flow · commit 9dd3c42dae2e · 2026-07-27T21:05:44.000+09:00
* fix(codex): report whether a sync actually wrote the catalog or cache syncCatalogModels() and invalidateCodexModelsCache() both succeeded silently whether or not they wrote anything, so a caller could not tell a real catalog update from a no-op on a missing or unreadable catalog. Return that fact: syncCatalogModels() gains catalogWritten, and invalidateCodexModelsCache() returns whether it rewrote models_cache. refreshCodexModelCatalog() carries both outward. This is the signal half of #518, split out so it can be reviewed on its own. Nothing acts on it yet — the consumer (warning about, and optionally restarting, stale app-server processes) is the other half, and it touches process termination, which deserves its own review. * test(codex): cover real catalog sync write signals
diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts
@@ -457,11 +457,12 @@ export function mergeCatalogEntriesForSync(
 export async function syncCatalogModels(config: OcxConfig): Promise<{
   added: number;
   path: string;
+  catalogWritten: boolean;
   comboOmissions: ComboCatalogOmission[];
 }> {
   const catalogPath = readCodexCatalogPath();
   const catalog = loadCatalogForSync(catalogPath);
-  if (!catalog) return { added: 0, path: catalogPath, comboOmissions: [] };
+  if (!catalog) return { added: 0, path: catalogPath, catalogWritten: false, comboOmissions: [] };
 
   const template = findNativeTemplate(catalog);
 
@@ -500,7 +501,7 @@ export async function syncCatalogModels(config: OcxConfig): Promise<{
   clampCatalogModelsToCodexSupport(catalog.models);
 
   atomicWriteFile(catalogPath, JSON.stringify(catalog, null, 2) + "\n");
-  return { added: goEntries.length, path: catalogPath, comboOmissions };
+  return { added: goEntries.length, path: catalogPath, catalogWritten: true, comboOmissions };
 }
 
 export function restoreCodexCatalog(): { removed: number; kept: number; path: string } {
@@ -531,10 +532,11 @@ export function restoreCodexCatalog(): { removed: number; kept: number; path: st
   return { removed, kept: native.length, path: catalogPath };
 }
 
-export function invalidateCodexModelsCache(): void {
+/** Force Codex's models_cache stale from the on-disk catalog. Returns whether a cache write occurred. */
+export function invalidateCodexModelsCache(): boolean {
   try {
     const catalogPath = readCodexCatalogPath();
-    if (!existsSync(catalogPath)) return;
+    if (!existsSync(catalogPath)) return false;
     const catalog = JSON.parse(readFileSync(catalogPath, "utf8"));
     const models = catalog.models ?? catalog;
     const wrapper = {
@@ -543,5 +545,8 @@ export function invalidateCodexModelsCache(): void {
       models,
     };
     atomicWriteFile(activeCodexModelsCachePath(), JSON.stringify(wrapper, null, 2) + "\n");
-  } catch { /* best-effort */ }
+    return true;
+  } catch {
+    return false;
+  }
 }
diff --git a/src/codex/refresh.ts b/src/codex/refresh.ts
@@ -9,6 +9,7 @@ export interface CodexCatalogRefreshResult {
   added: number;
   path: string;
   catalogExists: boolean;
+  catalogWritten: boolean;
   cacheSynced: boolean;
   comboOmissions: ComboCatalogOmission[];
 }
@@ -42,8 +43,11 @@ export async function refreshCodexModelCatalog(
 ): Promise<CodexCatalogRefreshResult> {
   const result = await deps.syncCatalogModels(config);
   const catalogExists = deps.existsSync(result.path);
+  const catalogWritten = result.catalogWritten === true;
   const comboOmissions = result.comboOmissions ?? [];
-  if (!catalogExists) return { ...result, catalogExists, cacheSynced: false, comboOmissions };
-  deps.invalidateCodexModelsCache();
-  return { ...result, catalogExists, cacheSynced: true, comboOmissions };
+  if (!catalogExists) {
+    return { ...result, catalogExists, catalogWritten: false, cacheSynced: false, comboOmissions };
+  }
+  const cacheSynced = deps.invalidateCodexModelsCache();
+  return { ...result, catalogExists, catalogWritten, cacheSynced, comboOmissions };
 }
diff --git a/src/codex/sync.ts b/src/codex/sync.ts
@@ -11,6 +11,7 @@ export interface CodexSyncResult {
   added: number;
   catalogPath: string | null;
   catalogExists: boolean;
+  catalogWritten: boolean;
   cacheSynced: boolean;
   message: string;
   warning?: string;
@@ -61,6 +62,7 @@ export async function syncModelsToCodex(
       added: 0,
       catalogPath: null,
       catalogExists: false,
+      catalogWritten: false,
       cacheSynced: false,
       message: result.message,
     };
@@ -71,6 +73,7 @@ export async function syncModelsToCodex(
   let catalogPath: string | null = null;
   let catalogPathForInjection: string | null | undefined;
   let catalogExists = false;
+  let catalogWritten = false;
   let cacheSynced = false;
   let warning: string | undefined;
   let comboOmissions: ComboCatalogOmission[] = [];
@@ -79,6 +82,7 @@ export async function syncModelsToCodex(
     const cat = await deps.refreshCodexModelCatalog(config);
     added = cat.added;
     catalogExists = cat.catalogExists;
+    catalogWritten = cat.catalogWritten;
     cacheSynced = cat.cacheSynced;
     catalogPathForInjection = cat.catalogExists ? cat.path : null;
     catalogPath = catalogPathForInjection;
@@ -110,6 +114,7 @@ export async function syncModelsToCodex(
     added,
     catalogPath,
     catalogExists,
+    catalogWritten,
     cacheSynced,
     message: result.message,
     ...(warning ? { warning } : {}),
diff --git a/tests/codex-refresh.test.ts b/tests/codex-refresh.test.ts
@@ -1,4 +1,8 @@
-import { describe, expect, test } from "bun:test";
+import { afterEach, describe, expect, test } from "bun:test";
+import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { invalidateCodexModelsCache, syncCatalogModels } from "../src/codex/catalog";
 import { refreshCodexModelCatalog } from "../src/codex/refresh";
 import type { OcxConfig } from "../src/types";
 
@@ -8,19 +12,73 @@ const config = {
   providers: {},
 } as OcxConfig;
 
+const tempHomes: string[] = [];
+
+function installTempHomes(): { codexHome: string; opencodexHome: string; restore(): void } {
+  const previousCodexHome = process.env.CODEX_HOME;
+  const previousOpenCodexHome = process.env.OPENCODEX_HOME;
+  const codexHome = mkdtempSync(join(tmpdir(), "ocx-refresh-codex-"));
+  const opencodexHome = mkdtempSync(join(tmpdir(), "ocx-refresh-ocx-"));
+  tempHomes.push(codexHome, opencodexHome);
+  process.env.CODEX_HOME = codexHome;
+  process.env.OPENCODEX_HOME = opencodexHome;
+
+  return {
+    codexHome,
+    opencodexHome,
+    restore() {
+      if (previousCodexHome === undefined) delete process.env.CODEX_HOME;
+      else process.env.CODEX_HOME = previousCodexHome;
+      if (previousOpenCodexHome === undefined) delete process.env.OPENCODEX_HOME;
+      else process.env.OPENCODEX_HOME = previousOpenCodexHome;
+      rmSync(codexHome, { recursive: true, force: true });
+      rmSync(opencodexHome, { recursive: true, force: true });
+    },
+  };
+}
+
+function nativeCatalogFixture(slug = "gpt-5.5"): string {
+  return JSON.stringify({
+    models: [{
+      slug,
+      display_name: slug,
+      description: "native",
+      priority: 9,
+      visibility: "list",
+      base_instructions: "You are Codex, a coding agent based on GPT-5.",
+      supported_reasoning_levels: [{ effort: "medium", description: "m" }],
+    }],
+  }, null, 2) + "\n";
+}
+
+afterEach(() => {
+  for (const path of tempHomes.splice(0)) {
+    rmSync(path, { recursive: true, force: true });
+  }
+});
+
 describe("Codex catalog refresh", () => {
   test("writes an expired Codex models cache whenever the materialized catalog exists", async () => {
     let invalidated = 0;
     const result = await refreshCodexModelCatalog(config, {
-      syncCatalogModels: async () => ({ added: 0, path: "/tmp/opencodex-catalog.json", comboOmissions: [] }),
-      invalidateCodexModelsCache: () => { invalidated += 1; },
+      syncCatalogModels: async () => ({
+        added: 0,
+        path: "/tmp/opencodex-catalog.json",
+        catalogWritten: true,
+        comboOmissions: [],
+      }),
+      invalidateCodexModelsCache: () => {
+        invalidated += 1;
+        return true;
+      },
       existsSync: () => true,
     });
 
     expect(result).toEqual({
       added: 0,
       path: "/tmp/opencodex-catalog.json",
       catalogExists: true,
+      catalogWritten: true,
       cacheSynced: true,
       comboOmissions: [],
     });
@@ -30,14 +88,130 @@ describe("Codex catalog refresh", () => {
   test("does not touch the cache when no Codex catalog can be materialized", async () => {
     let invalidated = 0;
     const result = await refreshCodexModelCatalog(config, {
-      syncCatalogModels: async () => ({ added: 0, path: "/tmp/missing-catalog.json", comboOmissions: [] }),
-      invalidateCodexModelsCache: () => { invalidated += 1; },
+      syncCatalogModels: async () => ({
+        added: 0,
+        path: "/tmp/missing-catalog.json",
+        catalogWritten: false,
+        comboOmissions: [],
+      }),
+      invalidateCodexModelsCache: () => {
+        invalidated += 1;
+        return true;
+      },
       existsSync: () => false,
     });
 
     expect(result.catalogExists).toBe(false);
+    expect(result.catalogWritten).toBe(false);
     expect(result.cacheSynced).toBe(false);
     expect(result.comboOmissions).toEqual([]);
     expect(invalidated).toBe(0);
   });
+
+  test("reports cacheSynced false when invalidate cannot write", async () => {
+    const result = await refreshCodexModelCatalog(config, {
+      syncCatalogModels: async () => ({
+        added: 0,
+        path: "/tmp/opencodex-catalog.json",
+        catalogWritten: true,
+        comboOmissions: [],
+      }),
+      invalidateCodexModelsCache: () => false,
+      existsSync: () => true,
+    });
+
+    expect(result.catalogExists).toBe(true);
+    expect(result.catalogWritten).toBe(true);
+    expect(result.cacheSynced).toBe(false);
+    expect(result.comboOmissions).toEqual([]);
+  });
+
+  test("preserves catalogWritten false when the catalog path exists but sync did not write", async () => {
+    const result = await refreshCodexModelCatalog(config, {
+      syncCatalogModels: async () => ({
+        added: 0,
+        path: "/tmp/broken-catalog.json",
+        catalogWritten: false,
+        comboOmissions: [],
+      }),
+      invalidateCodexModelsCache: () => false,
+      existsSync: () => true,
+    });
+
+    expect(result.catalogExists).toBe(true);
+    expect(result.catalogWritten).toBe(false);
+    expect(result.cacheSynced).toBe(false);
+  });
+
+  test("reports catalogWritten true after syncCatalogModels rewrites a real catalog file", async () => {
+    const home = installTempHomes();
+    try {
+      const catalogPath = join(home.codexHome, "nested", "catalog.json");
+      mkdirSync(join(home.codexHome, "nested"), { recursive: true });
+      writeFileSync(join(home.codexHome, "config.toml"), 'model_catalog_json = "nested/catalog.json"\n', "utf8");
+      writeFileSync(catalogPath, nativeCatalogFixture("gpt-5.6-sol"), "utf8");
+      const before = readFileSync(catalogPath, "utf8");
+
+      const result = await syncCatalogModels(config);
+      const after = readFileSync(catalogPath, "utf8");
+      const rewritten = JSON.parse(after);
+
+      expect(result.path).toBe(join(realpathSync.native(home.codexHome), "nested", "catalog.json"));
+      expect(result.catalogWritten).toBe(true);
+      expect(after).not.toBe(before);
+      expect(rewritten.models[0].slug).toBe("gpt-5.6-sol");
+      expect(rewritten.models[0].display_name).toBe("GPT-5.6-Sol");
+      expect(rewritten.models[0].context_window).toBeGreaterThan(0);
+    } finally {
+      home.restore();
+    }
+  });
+
+  test("invalidateCodexModelsCache reports real cache write success and failure cases", () => {
+    const success = installTempHomes();
+    try {
+      writeFileSync(join(success.codexHome, "config.toml"), 'model_catalog_json = "opencodex-catalog.json"\n', "utf8");
+      writeFileSync(join(success.codexHome, "opencodex-catalog.json"), nativeCatalogFixture("gpt-5.6-sol"), "utf8");
+
+      expect(invalidateCodexModelsCache()).toBe(true);
+      const cache = JSON.parse(readFileSync(join(success.codexHome, "models_cache.json"), "utf8"));
+      expect(cache.fetched_at).toBe("2000-01-01T00:00:00Z");
+      expect(cache.client_version).toBe("0.0.0");
+      expect(cache.models[0].slug).toBe("gpt-5.6-sol");
+    } finally {
+      success.restore();
+    }
+
+    const missingCatalog = installTempHomes();
+    try {
+      writeFileSync(join(missingCatalog.codexHome, "config.toml"), 'model_catalog_json = "missing-catalog.json"\n', "utf8");
+
+      expect(invalidateCodexModelsCache()).toBe(false);
+      expect(existsSync(join(missingCatalog.codexHome, "models_cache.json"))).toBe(false);
+    } finally {
+      missingCatalog.restore();
+    }
+
+    const malformedCatalog = installTempHomes();
+    try {
+      writeFileSync(join(malformedCatalog.codexHome, "config.toml"), 'model_catalog_json = "opencodex-catalog.json"\n', "utf8");
+      writeFileSync(join(malformedCatalog.codexHome, "opencodex-catalog.json"), "{not-json", "utf8");
+
+      expect(invalidateCodexModelsCache()).toBe(false);
+      expect(existsSync(join(malformedCatalog.codexHome, "models_cache.json"))).toBe(false);
+    } finally {
+      malformedCatalog.restore();
+    }
+
+    const unwritableCache = installTempHomes();
+    try {
+      writeFileSync(join(unwritableCache.codexHome, "config.toml"), 'model_catalog_json = "opencodex-catalog.json"\n', "utf8");
+      writeFileSync(join(unwritableCache.codexHome, "opencodex-catalog.json"), nativeCatalogFixture(), "utf8");
+      mkdirSync(join(unwritableCache.codexHome, "models_cache.json"));
+
+      expect(invalidateCodexModelsCache()).toBe(false);
+    } finally {
+      unwritableCache.restore();
+    }
+  });
 });
diff --git a/tests/codex-sync-api.test.ts b/tests/codex-sync-api.test.ts
@@ -53,6 +53,7 @@ describe("GUI/CLI Codex sync backend", () => {
         added: 3,
         path: "/tmp/opencodex-catalog.json",
         catalogExists: true,
+        catalogWritten: true,
         cacheSynced: true,
         comboOmissions: [],
       }),
@@ -72,6 +73,7 @@ describe("GUI/CLI Codex sync backend", () => {
       added: 3,
       catalogPath: "/tmp/opencodex-catalog.json",
       catalogExists: true,
+      catalogWritten: true,
       cacheSynced: true,
       message: "injected",
     });
@@ -93,6 +95,7 @@ describe("GUI/CLI Codex sync backend", () => {
         added: 1,
         path: "/tmp/opencodex-catalog.json",
         catalogExists: true,
+        catalogWritten: true,
         cacheSynced: true,
         comboOmissions: [omission],
       }),
@@ -121,6 +124,7 @@ describe("GUI/CLI Codex sync backend", () => {
         added: 0,
         path: "/tmp/opencodex-catalog.json",
         catalogExists: true,
+        catalogWritten: true,
         cacheSynced: true,
         comboOmissions: [omission],
       }),
@@ -192,6 +196,7 @@ describe("GUI/CLI Codex sync backend", () => {
       added: 0,
       catalogPath: null,
       catalogExists: false,
+      catalogWritten: false,
       cacheSynced: false,
       message: "external provider preserved",
     });
diff --git a/tests/injection-model-api.test.ts b/tests/injection-model-api.test.ts
@@ -249,9 +249,9 @@ describe("/api/injection-model guidance kill switch + partial update", () => {
     await refreshCodexModelCatalog(config, {
       syncCatalogModels: async syncedConfig => {
         flagSeenBySync = syncedConfig.multiAgentGuidanceEnabled;
-        return { added: 0, path: join(tempHome!, "missing-catalog.json") };
+        return { added: 0, path: join(tempHome!, "missing-catalog.json"), catalogWritten: false, comboOmissions: [] };
       },
-      invalidateCodexModelsCache: () => {},
+      invalidateCodexModelsCache: () => false,
       existsSync: () => false,
     });
     expect(flagSeenBySync).toBe(false);
