From 642805c11e1abbb019c5d3b03b5bc101117df205 Mon Sep 17 00:00:00 2001 From: tlsdnwn55 <48666403+tlsdnwn55@users.noreply.github.com> Date: Tue, 11 Aug 2026 17:48:48 +0900 Subject: [PATCH] fix(catalog): skip byte-identical catalog writes so a resync cannot mark Codex stale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `writeRetainedCatalogSync` wrote the catalog unconditionally, with no content comparison, so every sync moved the file's mtime even when the produced bytes were identical to what was already on disk. `collectCodexAppServerCatalogState()` compares that mtime against each running Codex's start time, so an ordinary `ocx start` — or any dashboard action that re-syncs an unchanged model set — classified every already-running Codex as holding an outdated in-memory catalog. Since #1407 that verdict withholds all opencodex-authored v2 model guidance, so a configured `injectionModel` and roster silently stop reaching sessions for the rest of that Codex's lifetime, even though the advertised model set never changed. A long-lived Codex App app-server outlives every proxy restart, which makes the state effectively latched: restarting the CLI does not clear it. An identical write is now skipped and reported as `catalogWritten: false`. `added` still reports the routed rows the catalog carries, because they are on disk either way, and `cacheSynced` is unaffected (`refreshCodexModelCatalog` invalidates the models cache whenever the catalog exists, independently of whether it was rewritten). The comparison is byte-exact, not a decoded-string comparison. Reading the file as UTF-8 replaces every invalid sequence with U+FFFD, so a malformed byte on disk would compare equal to a legitimately encoded replacement character in the prepared content: the skip would then preserve the corruption and report `catalogWritten: false` for a file that is not what we prepared. An unreadable or absent file still falls back to a real write. The convergence/retained-sync byte-agreement assertions asserted `catalogWritten: true` for exactly the no-op case while also asserting the bytes did not change; they now assert the no-op they describe (no policy skip, no write, bytes unchanged). Assertions covering syncs that really change content are untouched. --- .../content/docs/guides/sub-agent-surface.md | 4 + src/codex/catalog/sync.ts | 39 +++++- tests/codex-catalog-sync-hardening.test.ts | 129 ++++++++++++++++++ ...odex-convergence-account-selectors.test.ts | 15 +- 4 files changed, 182 insertions(+), 5 deletions(-) diff --git a/docs-site/src/content/docs/guides/sub-agent-surface.md b/docs-site/src/content/docs/guides/sub-agent-surface.md index 233975fad..00afb8d21 100644 --- a/docs-site/src/content/docs/guides/sub-agent-surface.md +++ b/docs-site/src/content/docs/guides/sub-agent-surface.md @@ -231,6 +231,10 @@ timestamp, an unreadable process start time, or a failed process enumeration — separately by `ocx doctor`. `stale` clears only after every detected Codex app-server starts after the final catalog write; it does not necessarily clear `unknown`. +Only a real change counts. A sync whose result is byte-identical to the catalog already on disk +leaves the file untouched, so restarting the proxy or re-syncing an unchanged model set does not +make a running Codex look stale. + ### Reasoning effort `injectionEffort` affects only delegated-worker guidance and, when explicitly enabled, native Codex diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts index 12866bcb1..30208ccc9 100644 --- a/src/codex/catalog/sync.ts +++ b/src/codex/catalog/sync.ts @@ -1186,6 +1186,23 @@ function revalidateRetainedCatalogSync( }; } +/** + * Exact bytes currently on disk at `path`, or null when unreadable/absent. + * + * Bytes, not a decoded string: `readFileSync(path, "utf8")` replaces every + * invalid sequence with `U+FFFD`, so a malformed byte on disk would compare + * equal to a legitimately encoded replacement character in the prepared + * content. The equal-content skip below would then preserve the corruption and + * report `catalogWritten: false` for a file that is not what we prepared. + */ +function currentCatalogFileBytes(path: string): Buffer | null { + try { + return readFileSync(path); + } catch { + return null; + } +} + function pristineCatalogBytes(read: RetainedCatalogSyncRead): string | null { if (read.onDiskCatalog && !catalogHasRoutedEntries(read.onDiskCatalog)) { try { @@ -1355,12 +1372,30 @@ function writeRetainedCatalogSync({ }); clampCatalogModelsToCodexSupport(catalog.models); + const added = goEntries.length + accountBoundEntries.length; + const content = `${JSON.stringify(catalog, null, 2)}\n`; + // A byte-identical rewrite is not a catalog change, but every mtime-keyed reader + // has to treat it as one. The app-server staleness classifier (#857) is the one + // that matters: it compares this file's mtime against each running Codex's start + // time, so an ordinary `ocx start` — or any dashboard action that re-syncs an + // unchanged model set — marked every already-running Codex as holding an outdated + // in-memory catalog. Since #1407 that verdict silences opencodex's own model + // guidance entirely (no preferred model, no roster) for the rest of that Codex's + // lifetime, so a configured injectionModel stops reaching the session even though + // nothing about the catalog changed. Skipping the no-op write keeps both the mtime + // and `catalogWritten` honest; `added` still reports the routed rows the catalog + // carries, because they are on disk either way. + const onDiskBytes = currentCatalogFileBytes(catalogPath); + if (onDiskBytes !== null && onDiskBytes.equals(Buffer.from(content, "utf8"))) { + return { added, path: catalogPath, catalogWritten: false, comboOmissions }; + } + replaceActiveCodexCatalog(permit, owningCodexHome, { path: catalogPath, - content: `${JSON.stringify(catalog, null, 2)}\n`, + content, }); return { - added: goEntries.length + accountBoundEntries.length, + added, path: catalogPath, catalogWritten: true, comboOmissions, diff --git a/tests/codex-catalog-sync-hardening.test.ts b/tests/codex-catalog-sync-hardening.test.ts index 580a3858d..09d63715c 100644 --- a/tests/codex-catalog-sync-hardening.test.ts +++ b/tests/codex-catalog-sync-hardening.test.ts @@ -968,6 +968,135 @@ describe("Codex catalog sync hardening", () => { expect(slugs).not.toContain("cursor/stale-model"); }); + test("an identical resync leaves the catalog file untouched, a real change still writes", () => { + // The app-server staleness classifier (#857) compares this file's mtime against + // each running Codex's start time, so a no-op rewrite would report every + // already-running Codex as holding an outdated catalog — and since #1407 that + // verdict withholds opencodex's model guidance for the rest of that Codex's + // lifetime, even though the advertised model set never changed. + const catalogPath = join(codexHome, "catalog.json"); + writeFileSync(join(codexHome, "config.toml"), 'model_catalog_json = "catalog.json"\n', "utf8"); + writeFileSync(catalogPath, JSON.stringify({ + models: [ + nativeEntry("gpt-5.5", 0), + nativeEntry("gpt-5.2", 104), // legacy -> dropped by the first sync + ], + }, null, 2) + "\n"); + + const r = runScript(codexHome, opencodexHome, ` + const { statSync, writeFileSync, readFileSync } = require("node:fs"); + const { syncCatalogModels } = require("./src/codex/catalog"); + const path = ${JSON.stringify(catalogPath)}; + const sleep = ms => new Promise(resolve => setTimeout(resolve, ms)); + (async () => { + const first = await syncCatalogModels({ providers: {} }); + const afterFirst = statSync(path).mtimeMs; + await sleep(1100); + const second = await syncCatalogModels({ providers: {} }); + const afterSecond = statSync(path).mtimeMs; + // Not vacuous: a catalog that really differs must still be rewritten. + const catalog = JSON.parse(readFileSync(path, "utf8")); + catalog.models = catalog.models.filter(model => model.slug !== "gpt-5.5"); + writeFileSync(path, JSON.stringify(catalog, null, 2) + "\\n"); + const changedAt = statSync(path).mtimeMs; + await sleep(1100); + const third = await syncCatalogModels({ providers: {} }); + console.log(JSON.stringify({ + firstWritten: first.catalogWritten, + secondWritten: second.catalogWritten, + secondAdded: second.added, + identicalResyncKeptMtime: afterFirst === afterSecond, + thirdWritten: third.catalogWritten, + realChangeBumpedMtime: statSync(path).mtimeMs > changedAt, + })); + })(); + `); + expect(r.status).toBe(0); + + const out = JSON.parse(r.stdout) as { + firstWritten: boolean; + secondWritten: boolean; + secondAdded: number; + identicalResyncKeptMtime: boolean; + thirdWritten: boolean; + realChangeBumpedMtime: boolean; + }; + expect(out.firstWritten).toBe(true); + expect(out.secondWritten).toBe(false); + expect(out.identicalResyncKeptMtime).toBe(true); + expect(out.thirdWritten).toBe(true); + expect(out.realChangeBumpedMtime).toBe(true); + }); + + test("a malformed on-disk byte that decodes to the same string is still repaired", () => { + // The equal-content skip must compare bytes. `readFileSync(path, "utf8")` + // replaces an invalid sequence with U+FFFD, so a raw 0x80 on disk decodes to + // exactly what the prepared content re-encodes as EF BF BD: a decoded-string + // comparison would call that "identical", keep the corruption, and report + // `catalogWritten: false` for a file that is not what we prepared. + const catalogPath = join(codexHome, "catalog.json"); + writeFileSync(join(codexHome, "config.toml"), 'model_catalog_json = "catalog.json"\n', "utf8"); + writeFileSync(catalogPath, JSON.stringify({ + models: [nativeEntry("gpt-5.5", 0), nativeEntry("user-native", 10)], + }, null, 2) + "\n"); + + const r = runScript(codexHome, opencodexHome, ` + const { readFileSync, writeFileSync } = require("node:fs"); + const { syncCatalogModels } = require("./src/codex/catalog"); + const path = ${JSON.stringify(catalogPath)}; + (async () => { + // Settle the catalog first, so the only later difference is the encoding. + await syncCatalogModels({ providers: {} }); + const settled = readFileSync(path); + // Corrupt one byte inside a preserved JSON string value. + const at = settled.lastIndexOf(Buffer.from("user-native", "utf8")); + const corrupted = Buffer.from(settled); + corrupted[at] = 0x80; + writeFileSync(path, corrupted); + const result = await syncCatalogModels({ providers: {} }); + const after = readFileSync(path); + const decodes = bytes => { + try { + new TextDecoder("utf-8", { fatal: true }).decode(bytes); + return true; + } catch { + return false; + } + }; + console.log(JSON.stringify({ + corruptedFound: at > 0, + corruptedFileWasInvalidUtf8: !decodes(corrupted), + written: result.catalogWritten, + // The repaired bytes differ from the corrupted file while decoding to the + // same string: exactly the pair a decoded-string comparison equates, and + // therefore the case that would have skipped the repair. + bytesDiffer: !corrupted.equals(after), + decodedStringsEqual: corrupted.toString("utf8") === after.toString("utf8"), + repairedFileIsValidUtf8: decodes(after), + replacementEncoded: after.includes(Buffer.from([0xef, 0xbf, 0xbd])), + })); + })(); + `); + expect(r.status).toBe(0); + + const out = JSON.parse(r.stdout) as { + corruptedFound: boolean; + corruptedFileWasInvalidUtf8: boolean; + written: boolean; + bytesDiffer: boolean; + decodedStringsEqual: boolean; + repairedFileIsValidUtf8: boolean; + replacementEncoded: boolean; + }; + expect(out.corruptedFound).toBe(true); + expect(out.corruptedFileWasInvalidUtf8).toBe(true); + expect(out.written).toBe(true); + expect(out.bytesDiffer).toBe(true); + expect(out.decodedStringsEqual).toBe(true); + expect(out.repairedFileIsValidUtf8).toBe(true); + expect(out.replacementEncoded).toBe(true); + }); + test("readCodexCatalogPath honors CODEX_HOME at call time", () => { const alternateHome = join(codexHome, "alternate-codex-home"); mkdirSync(alternateHome, { recursive: true }); diff --git a/tests/codex-convergence-account-selectors.test.ts b/tests/codex-convergence-account-selectors.test.ts index e58f7270d..2e51dc96e 100644 --- a/tests/codex-convergence-account-selectors.test.ts +++ b/tests/codex-convergence-account-selectors.test.ts @@ -571,7 +571,12 @@ test("routed-only custom catalogs remain authoritative across convergence and re expect(catalog.models?.some(entry => entry.slug === "static/newer")).toBe(true); const convergenceBytes = readFileSync(catalogPath, "utf8"); - expect((await syncCatalogModels(nextConfig)).catalogWritten).toBe(true); + // Agreement, not a rewrite: the retained sync runs to completion (no policy skip) + // and reports no write because convergence already produced these exact bytes. + // Rewriting them would move the catalog mtime and mark every running Codex stale (#857). + const resync = await syncCatalogModels(nextConfig); + expect(resync.skippedReason).toBeUndefined(); + expect(resync.catalogWritten).toBe(false); expect(readFileSync(catalogPath, "utf8")).toBe(convergenceBytes); }); @@ -627,7 +632,9 @@ test("disabled-provider selections cannot delete a foreign row in either writer" expect((JSON.parse(convergenceBytes) as RawCatalog).models) .toContainEqual(expect.objectContaining({ slug: "disabled/foreign-model" })); - expect((await syncCatalogModels(nextConfig)).catalogWritten).toBe(true); + const resync = await syncCatalogModels(nextConfig); + expect(resync.skippedReason).toBeUndefined(); + expect(resync.catalogWritten).toBe(false); expect(readFileSync(catalogPath, "utf8")).toBe(convergenceBytes); }); @@ -774,7 +781,9 @@ test("retained sync and convergence produce identical canonical bytes in either await convergeCatalog(nextConfig); const convergenceFirst = readBytes(); expectCanonicalContent(convergenceFirst, pickerEnabled); - expect((await syncCatalogModels(nextConfig)).catalogWritten).toBe(true); + const resync = await syncCatalogModels(nextConfig); + expect(resync.skippedReason).toBeUndefined(); + expect(resync.catalogWritten).toBe(false); expect(readBytes()).toBe(convergenceFirst); seed();