Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/codex/catalog/sync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: [] };
Comment on lines 457 to +465

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add direct tests for the filesystem write-state branches.

The changed tests exercise mocked contracts, but do not execute these new loadCatalogForSync, JSON parsing, or atomicWriteFile branches. Add focused Bun tests covering: no loadable catalog → catalogWritten: false; successful catalog/cache writes → true; and missing, malformed, unreadable, or failed-write cases → false. This prevents regressions in the actual boolean-producing code from being hidden by propagation-only tests.

As per path instructions, behavior changes in src/ should have focused regression tests under tests/.

Also applies to: 503-504, 535-551

🧰 Tools
🪛 ast-grep (0.44.1)

[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(detect-child-process-typescript)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/codex/catalog/sync.ts` around lines 457 - 465, Expand the Bun tests for
syncCatalogModels to execute the real loadCatalogForSync, JSON parsing, and
atomicWriteFile filesystem branches rather than only mocked propagation. Cover
no loadable catalog, successful catalog and cache writes, and missing,
malformed, unreadable, or failed-write cases, asserting catalogWritten is false
or true as appropriate. Place focused regression tests under tests/.

Source: Path instructions


const template = findNativeTemplate(catalog);

Expand Down Expand Up @@ -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 } {
Expand Down Expand Up @@ -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 = {
Expand All @@ -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;
}
}
10 changes: 7 additions & 3 deletions src/codex/refresh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export interface CodexCatalogRefreshResult {
added: number;
path: string;
catalogExists: boolean;
catalogWritten: boolean;
cacheSynced: boolean;
comboOmissions: ComboCatalogOmission[];
}
Expand Down Expand Up @@ -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 };
}
5 changes: 5 additions & 0 deletions src/codex/sync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export interface CodexSyncResult {
added: number;
catalogPath: string | null;
catalogExists: boolean;
catalogWritten: boolean;
cacheSynced: boolean;
message: string;
warning?: string;
Expand Down Expand Up @@ -61,6 +62,7 @@ export async function syncModelsToCodex(
added: 0,
catalogPath: null,
catalogExists: false,
catalogWritten: false,
cacheSynced: false,
message: result.message,
};
Expand All @@ -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[] = [];
Expand All @@ -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;
Expand Down Expand Up @@ -110,6 +114,7 @@ export async function syncModelsToCodex(
added,
catalogPath,
catalogExists,
catalogWritten,
cacheSynced,
message: result.message,
...(warning ? { warning } : {}),
Expand Down
184 changes: 179 additions & 5 deletions tests/codex-refresh.test.ts
Original file line number Diff line number Diff line change
@@ -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";

Expand All @@ -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: [],
});
Expand All @@ -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();
}
});
Comment on lines +170 to +216

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Split the four invalidateCodexModelsCache scenarios into separate tests.

All four cases (success, missing catalog, malformed catalog, unwritable cache) run sequentially inside one test(). If an earlier expect() throws, the later scenarios never execute, silently hiding whether those branches still pass. Splitting into four test() blocks (reusing installTempHomes/nativeCatalogFixture) gives independent pass/fail signal per branch at negligible cost.

♻️ Proposed refactor sketch
-  test("invalidateCodexModelsCache reports real cache write success and failure cases", () => {
-    const success = installTempHomes();
-    try {
-      ...
-    } finally {
-      success.restore();
-    }
-
-    const missingCatalog = installTempHomes();
-    ...
-  });
+  test("invalidateCodexModelsCache writes real cache on success", () => {
+    const home = installTempHomes();
+    try { /* success scenario */ } finally { home.restore(); }
+  });
+
+  test("invalidateCodexModelsCache returns false when catalog is missing", () => {
+    const home = installTempHomes();
+    try { /* missing-catalog scenario */ } finally { home.restore(); }
+  });
+
+  test("invalidateCodexModelsCache returns false on malformed catalog JSON", () => { /* ... */ });
+  test("invalidateCodexModelsCache returns false on unwritable cache path", () => { /* ... */ });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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();
}
});
test("invalidateCodexModelsCache writes real cache on success", () => {
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();
}
});
test("invalidateCodexModelsCache returns false when catalog is missing", () => {
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();
}
});
test("invalidateCodexModelsCache returns false on malformed catalog JSON", () => {
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();
}
});
test("invalidateCodexModelsCache returns false on unwritable cache path", () => {
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();
}
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/codex-refresh.test.ts` around lines 170 - 216, Split the four scenarios
in invalidateCodexModelsCache reports real cache write success and failure cases
into separate test() blocks: successful cache writing, missing catalog,
malformed catalog, and unwritable cache. Keep each scenario’s setup, assertions,
and try/finally cleanup unchanged, reusing installTempHomes and
nativeCatalogFixture so each branch reports an independent result.

Source: Path instructions

});
5 changes: 5 additions & 0 deletions tests/codex-sync-api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ describe("GUI/CLI Codex sync backend", () => {
added: 3,
path: "/tmp/opencodex-catalog.json",
catalogExists: true,
catalogWritten: true,
cacheSynced: true,
comboOmissions: [],
}),
Expand All @@ -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",
});
Expand All @@ -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],
}),
Expand Down Expand Up @@ -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],
}),
Expand Down Expand Up @@ -192,6 +196,7 @@ describe("GUI/CLI Codex sync backend", () => {
added: 0,
catalogPath: null,
catalogExists: false,
catalogWritten: false,
cacheSynced: false,
message: "external provider preserved",
});
Expand Down
4 changes: 2 additions & 2 deletions tests/injection-model-api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
existsSync: () => false,
});
expect(flagSeenBySync).toBe(false);
Expand Down
Loading