diff --git a/src/cli/dispatch.ts b/src/cli/dispatch.ts index beb3170d992..a49dc0c16d9 100644 --- a/src/cli/dispatch.ts +++ b/src/cli/dispatch.ts @@ -516,21 +516,19 @@ const commandRunners: Record = { const cacheArgs = deps.args.slice(1); const restartScope = readRestartScope(cacheArgs, console); const { withCatalogWriteSerialization } = await import("../codex/catalog-write-serialization"); - const { invalidateCodexModelsCacheWithPermit } = await import("../codex/catalog/sync"); + const { syncCodexModelsCacheWithPermit } = await import("../codex/catalog/sync"); const { getCodexHome } = await import("../codex/paths"); - const { readCodexCatalogPathForHome } = await import("../codex/catalog/parsing"); - const { existsSync } = await import("node:fs"); const owningCodexHome = getCodexHome(); const cacheGateSnapshot = deps.loadConfig(); const desiredDisabled = !shouldSyncCodexOnStart(cacheGateSnapshot); const invalidated = withCatalogWriteSerialization(owningCodexHome, permit => - invalidateCodexModelsCacheWithPermit(permit, owningCodexHome, { allowWhenDesiredDisabled: true })); + syncCodexModelsCacheWithPermit(permit, owningCodexHome, { allowWhenDesiredDisabled: true })); const cacheJson = cacheArgs.includes("--json"); const jsonSafeLog = cacheJson ? { log: (...values: unknown[]) => console.error(...values), error: (...values: unknown[]) => console.error(...values) } : console; // Only warn/restart when models_cache was actually rewritten from a readable catalog. - if (invalidated.kind === "completed" && invalidated.value) { + if (invalidated.kind === "completed" && invalidated.value.status === "written") { await handleRestartScopeAfterWrite(restartScope, jsonSafeLog); } else if (desiredDisabled && !cacheJson) { // Worth saying in the human path, because it explains why nothing was written. @@ -541,8 +539,8 @@ const commandRunners: Record = { "No catalog or cache write resulted.", )); } - // `completed` with a falsy value means the cache was NOT rewritten. Previously every - // outcome exited 0, so a script could not tell a refreshed cache from a skipped one. + // A completed typed outcome distinguishes a rewrite, an already-current cache, + // a policy skip, and a failed refresh. // // Losing the catalog write lock to another process is a skip, not a failure: // serialization working as designed is the expected outcome under concurrency, and a @@ -557,36 +555,33 @@ const commandRunners: Record = { // means the user asked for it regardless of the toggle. Treating OFF as automatic success // would report exit 0 and `skipped: true` for a refresh that actually failed. // - // But `invalidateCodexModelsCacheWithPermit` returns a bare boolean for four different - // situations -- wrote it, no catalog file exists, the OFF gate fired, or it threw -- so - // `false` alone cannot be read as failure either. `!existsSync(catalogPath)` is a - // legitimate nothing-to-do: with no catalog there is no cache to derive, which is the - // normal state of a fully native home and the case - // `codex-composed-acceptance.test.ts` pins at exit 0. It is checked here rather than by - // widening that function's return type, because its boolean is consumed by a dozen - // management routes that have no use for the distinction. - const wrote = invalidated.kind === "completed" && Boolean(invalidated.value); + const completedStatus = invalidated.kind === "completed" ? invalidated.value.status : undefined; + const wrote = completedStatus === "written"; const contended = invalidated.kind === "unavailable" && invalidated.reason === "busy"; - const noCatalog = !wrote && !existsSync(readCodexCatalogPathForHome(owningCodexHome)); - const ok = wrote || contended || noCatalog; + const benignSkip = completedStatus === "unchanged" || completedStatus === "skipped"; + const ok = wrote || contended || benignSkip; if (cacheJson) { console.log(JSON.stringify({ schemaVersion: 1, ok, wrote, - skipped: contended || noCatalog, + skipped: contended || benignSkip, outcome: invalidated.kind, // `outcome` alone cannot separate a contended lock from a hard serialization // failure -- both are `unavailable`. Carry the reason so a caller can. reason: invalidated.kind === "unavailable" ? invalidated.reason : undefined, // Which of the two benign skips this was, so `skipped: true` is never opaque. - skippedReason: contended ? "contended" : noCatalog ? "no_catalog" : undefined, + skippedReason: contended ? "contended" : completedStatus === "unchanged" + ? "unchanged" : invalidated.kind === "completed" && invalidated.value.status === "skipped" + ? invalidated.value.reason : undefined, desiredDisabled, codexHome: owningCodexHome, }, null, 2)); } else if (contended) { console.log("Another process owns the catalog write; cache sync skipped."); - } else if (noCatalog) { + } else if (completedStatus === "unchanged") { + console.log("Codex model cache is already synchronized; nothing to write."); + } else if (invalidated.kind === "completed" && invalidated.value.status === "skipped" && invalidated.value.reason === "no_catalog") { console.log("No Codex catalog to derive a cache from; nothing to sync."); } else if (!ok) { console.error(`Cache refresh did not complete (${invalidated.kind}). The Codex model cache was not rewritten.`); diff --git a/src/codex/catalog/remote.ts b/src/codex/catalog/remote.ts index 9a6b534cdaa..4005315eeec 100644 --- a/src/codex/catalog/remote.ts +++ b/src/codex/catalog/remote.ts @@ -7,7 +7,7 @@ import { replaceActiveCodexCatalog } from "../internal/catalog-writer"; import { resetCodexAppServerCatalogStateCache } from "../app-server-processes"; import { getCodexHome } from "../paths"; import { readCodexCatalogPathForHome } from "./parsing"; -import { invalidateCodexModelsCacheWithPermit } from "./sync"; +import { syncCodexModelsCacheWithPermit } from "./sync"; const DEFAULT_TIMEOUT_MS = 15_000; const MAX_MODELS = 2_000; @@ -245,12 +245,12 @@ export async function pullRemoteCatalog(input: string, options: PullRemoteCatalo const lockedCurrent = existsSync(catalogPath) ? readFileSync(catalogPath) : null; if (lockedCurrent?.equals(candidate)) return { catalogWritten: false, cacheSynced: false }; replaceActiveCodexCatalog(permit, codexHome, { path: catalogPath, content: fetched.content }); - const cacheSynced = invalidateCodexModelsCacheWithPermit(permit, codexHome, { allowWhenDesiredDisabled: true }); - if (!cacheSynced) { + const cacheSync = syncCodexModelsCacheWithPermit(permit, codexHome, { allowWhenDesiredDisabled: true }); + if (cacheSync.status !== "written" && cacheSync.status !== "unchanged") { restorePreviousCatalog(permit, codexHome, catalogPath, lockedCurrent); throw new RemoteCatalogError("write_failed", "Remote catalog cache synchronization failed"); } - return { catalogWritten: true, cacheSynced: true }; + return { catalogWritten: true, cacheSynced: cacheSync.status === "written" }; }); if (outcome.kind !== "completed") return mapSerializationFailure(outcome); return { diff --git a/src/codex/catalog/retained-sync.ts b/src/codex/catalog/retained-sync.ts index efd3a63413e..b77ec278124 100644 --- a/src/codex/catalog/retained-sync.ts +++ b/src/codex/catalog/retained-sync.ts @@ -649,11 +649,17 @@ export async function syncCatalogModels( }; } -export function invalidateCodexModelsCacheWithPermit( +export type CodexModelsCacheSyncResult = + | { status: "written" } + | { status: "unchanged" } + | { status: "skipped"; reason: "desired_disabled" | "no_catalog" } + | { status: "failed" }; + +export function syncCodexModelsCacheWithPermit( permit: CatalogWritePermit, owningCodexHome: string, options?: CodexCatalogSyncOptions, -): boolean { +): CodexModelsCacheSyncResult { try { // This permit is a REACQUISITION: refreshCodexModelCatalog's commit released // K before this rewrite runs, so the commit-path desired-state check cannot @@ -661,9 +667,11 @@ export function invalidateCodexModelsCacheWithPermit( // routed cache write — re-read intent under this permit, same as the commit. // The catalog-only sync override applies here too so an explicit refresh // keeps the cache consistent with the catalog it just wrote. - if (!shouldSyncCodexOnStart(loadConfig()) && options?.allowWhenDesiredDisabled !== true) return false; + if (!shouldSyncCodexOnStart(loadConfig()) && options?.allowWhenDesiredDisabled !== true) { + return { status: "skipped", reason: "desired_disabled" }; + } const catalogPath = readCodexCatalogPathForHome(owningCodexHome); - if (!existsSync(catalogPath)) return false; + if (!existsSync(catalogPath)) return { status: "skipped", reason: "no_catalog" }; const catalog = JSON.parse(readFileSync(catalogPath, "utf8")); const models = catalog.models ?? catalog; const cachePath = join(owningCodexHome, "models_cache.json"); @@ -707,18 +715,26 @@ export function invalidateCodexModelsCacheWithPermit( // catalog reproduced byte-identically — the settled case — the warning still // claimed "Disk catalog/cache were updated" and told the operator their Codex // model list might be stale, when nothing on disk had changed and Codex held the - // same model set the file already described. Returning `false` here makes - // `cacheSynced` mean what its name and its consumers already assume, and what - // `pullRemoteCatalog` and the early returns in `refreshCodexModelCatalog` - // already assert: a write happened. - if (!preparedBytesDifferFromDisk(preparedCache)) return false; + // same model set the file already described. The compatibility boolean wrapper + // therefore returns false, keeping `cacheSynced` reserved for a real write. This + // typed result lets transactional callers distinguish that benign no-op from a + // failed synchronization. + if (!preparedBytesDifferFromDisk(preparedCache)) return { status: "unchanged" }; replaceCodexModelsCache(permit, owningCodexHome, preparedCache); - return true; + return { status: "written" }; } catch { - return false; + return { status: "failed" }; } } +export function invalidateCodexModelsCacheWithPermit( + permit: CatalogWritePermit, + owningCodexHome: string, + options?: CodexCatalogSyncOptions, +): boolean { + return syncCodexModelsCacheWithPermit(permit, owningCodexHome, options).status === "written"; +} + export function invalidateCodexModelsCache(options?: CodexCatalogSyncOptions): boolean { const owningCodexHome = getCodexHome(); const outcome = withCatalogWriteSerialization( diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts index 373bcb43779..2fd3d50d365 100644 --- a/src/codex/catalog/sync.ts +++ b/src/codex/catalog/sync.ts @@ -47,6 +47,7 @@ export { syncCatalogModels, invalidateCodexModelsCache, invalidateCodexModelsCacheWithPermit, + syncCodexModelsCacheWithPermit, } from "./retained-sync"; -export type { CodexCatalogSyncOptions } from "./retained-sync"; +export type { CodexCatalogSyncOptions, CodexModelsCacheSyncResult } from "./retained-sync"; export { restoreCodexCatalog, restoreCodexCatalogWithPermit } from "./restore"; diff --git a/structure/catalog.md b/structure/catalog.md index 7b035ee99fc..86c42c4118e 100644 --- a/structure/catalog.md +++ b/structure/catalog.md @@ -170,6 +170,9 @@ Entitlement-specific rosters (Qoder, Devin, Cursor) additionally bind their cach irreversible credential fingerprint: a credential switch observes neither the fresh nor the stale roster recorded under the previous credential, and a failed discovery's cooldown neither supplies the previous credential's stale roster nor suppresses the next credential's first discovery. +Models-cache synchronization reports writes, byte-identical no-ops, policy skips, and failures as +distinct outcomes internally. Transactional catalog installs accept an already-current cache as +synchronized without claiming it was rewritten, while preserving rollback for actual failures. A Devin live row spreads its measured `inputModalities` before `catalogHintsFromProviderConfig`, so exact `modelCapabilities` declarations, the legacy diff --git a/tests/adapters/openai/openai-chat-dangling-toolcalls.test.ts b/tests/adapters/openai/openai-chat-dangling-toolcalls.test.ts index 41a61c21023..075e3c33186 100644 --- a/tests/adapters/openai/openai-chat-dangling-toolcalls.test.ts +++ b/tests/adapters/openai/openai-chat-dangling-toolcalls.test.ts @@ -12,6 +12,10 @@ const provider: OcxProviderConfig = { baseUrl: "https://example.test/v1", apiKey: "sk-test", authMode: "key", + // The wire role folds to `system` unless a destination is recorded as accepting + // `developer`; this suite is about tool-result repair ordering, so it declares the + // destination rather than asserting the default. + foldDeveloperRoleToSystem: false, }; interface ChatMsg { diff --git a/tests/codex-integration/catalog-remote-pull.test.ts b/tests/codex-integration/catalog-remote-pull.test.ts index e92e0ccdf65..49db5f9f49f 100644 --- a/tests/codex-integration/catalog-remote-pull.test.ts +++ b/tests/codex-integration/catalog-remote-pull.test.ts @@ -332,6 +332,24 @@ describe("remote catalog coordinated installation", () => { expect([statSync(first.catalogPath).mtimeMs, statSync(cachePath).mtimeMs]).toEqual(before); }); + test("a top-level-only catalog update keeps an already-synchronized cache", async () => { + const codexHome = home(); + const first = await pullRemoteCatalog("https://hub.example/v1/catalog", { + codexHome, fetchImpl: async () => response(catalog), + }); + const cachePath = join(codexHome, "models_cache.json"); + const cacheBefore = readFileSync(cachePath); + const updated = { ...catalog, version: 2 }; + + const second = await pullRemoteCatalog("https://hub.example/v1/catalog", { + codexHome, fetchImpl: async () => response(updated), + }); + + expect(second).toMatchObject({ status: "updated", catalogWritten: true, cacheSynced: false }); + expect(JSON.parse(readFileSync(first.catalogPath, "utf8"))).toEqual(updated); + expect(readFileSync(cachePath)).toEqual(cacheBefore); + }); + test("lock contention is typed and preserves last-known-good files", async () => { const codexHome = home(); // Materialize K, then hold BEGIN IMMEDIATE from a separate connection while pull attempts it. diff --git a/tests/codex-integration/codex-app-server-processes.test.ts b/tests/codex-integration/codex-app-server-processes.test.ts index 343d193cea0..85fd4624980 100644 --- a/tests/codex-integration/codex-app-server-processes.test.ts +++ b/tests/codex-integration/codex-app-server-processes.test.ts @@ -772,8 +772,8 @@ describe("CLI /api sync wiring for stale app-servers (#476)", () => { // write actually landed, never on a refused/failed serialization attempt. expect(syncCacheCase).toContain("withCatalogWriteSerialization"); // #1931: explicit sync-cache refreshes even when injection is OFF (side profiles). - expect(syncCacheCase).toContain("invalidateCodexModelsCacheWithPermit(permit, owningCodexHome, { allowWhenDesiredDisabled: true })"); - const gate = 'if (invalidated.kind === "completed" && invalidated.value)'; + expect(syncCacheCase).toContain("syncCodexModelsCacheWithPermit(permit, owningCodexHome, { allowWhenDesiredDisabled: true })"); + const gate = 'if (invalidated.kind === "completed" && invalidated.value.status === "written")'; expect(syncCacheCase).toContain(gate); expect(syncCacheCase).toContain("handleRestartScopeAfterWrite"); expect(syncCacheCase.indexOf(gate)) diff --git a/tests/codex-integration/codex-catalog-sync-hardening.test.ts b/tests/codex-integration/codex-catalog-sync-hardening.test.ts index 5dc015d702f..20cef77247d 100644 --- a/tests/codex-integration/codex-catalog-sync-hardening.test.ts +++ b/tests/codex-integration/codex-catalog-sync-hardening.test.ts @@ -1365,6 +1365,28 @@ describe("Codex catalog sync hardening", () => { expect(out.bytesUnchanged).toBe(true); }, 20_000); + test("sync-cache reports an already-current cache as a successful skip", () => { + 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)] }, null, 2) + "\n"); + const env = { ...process.env, CODEX_HOME: codexHome, OPENCODEX_HOME: opencodexHome }; + const invoke = () => spawnSync(process.execPath, ["src/cli/index.ts", "sync-cache", "--json"], { + cwd: repoRoot, env, encoding: "utf8", + }); + + const first = invoke(); + const second = invoke(); + expect(first.status, first.stderr).toBe(0); + expect(second.status, second.stderr).toBe(0); + expect(JSON.parse(second.stdout)).toMatchObject({ + ok: true, + wrote: false, + skipped: true, + outcome: "completed", + skippedReason: "unchanged", + }); + }, 20_000); + test("the no-op guard compares bytes, so a malformed byte decoding to U+FFFD is still repaired", () => { // The guard above must not preserve corruption. `readFileSync(path, "utf8")` // substitutes U+FFFD for every invalid byte, so a catalog holding a bare 0x80 diff --git a/tests/codex-integration/codex-retained-root-serialization.test.ts b/tests/codex-integration/codex-retained-root-serialization.test.ts index c8f6a5bd2c2..a97c31db12b 100644 --- a/tests/codex-integration/codex-retained-root-serialization.test.ts +++ b/tests/codex-integration/codex-retained-root-serialization.test.ts @@ -307,7 +307,7 @@ test("startup and CLI sync-cache cannot write models_cache while another process const cliStart = cliSource.indexOf('"sync-cache": async'); const cliRoot = cliSource.slice(cliStart, cliSource.indexOf('gui: async', cliStart)); expect(cliRoot).toContain("withCatalogWriteSerialization(owningCodexHome"); - expect(cliRoot).toContain("invalidateCodexModelsCacheWithPermit"); + expect(cliRoot).toContain("syncCodexModelsCacheWithPermit"); const startup = readFileSync(join(repoRoot, "src/server/index.ts"), "utf8"); const startupStart = startup.indexOf("const startupCodexHome"); diff --git a/tests/responses/chat-inline-document-bytes.test.ts b/tests/responses/chat-inline-document-bytes.test.ts index dd88fa05788..a718c1c376a 100644 --- a/tests/responses/chat-inline-document-bytes.test.ts +++ b/tests/responses/chat-inline-document-bytes.test.ts @@ -28,6 +28,10 @@ const chatProvider: OcxProviderConfig = { adapter: "openai-chat", baseUrl: "https://gateway.example.internal/v1", apiKey: "k", + // The wire role folds to `system` unless a destination is recorded as accepting + // `developer`; the document test asserts the role a turn keeps, so it declares the + // destination rather than asserting the default. + foldDeveloperRoleToSystem: false, }; const anthropicProvider = { adapter: "anthropic",