fix(codex): report whether a sync actually wrote the catalog or cache - #526
Merged
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughCatalog synchronization now reports whether the catalog was written and whether cache invalidation succeeded. These statuses propagate through refresh and sync results, with tests covering missing catalogs, failed invalidation, successful writes, and external providers. ChangesCatalog write-state propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CodexSync
participant CatalogRefresh
participant CatalogSync
participant CacheInvalidation
CodexSync->>CatalogRefresh: refreshCodexModelCatalog()
CatalogRefresh->>CatalogSync: syncCatalogModels()
CatalogSync-->>CatalogRefresh: catalogWritten and comboOmissions
CatalogRefresh->>CacheInvalidation: invalidateCodexModelsCache()
CacheInvalidation-->>CatalogRefresh: cacheSynced boolean
CatalogRefresh-->>CodexSync: refresh result
CodexSync-->>CodexSync: return catalogWritten
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
4 tasks
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/codex/catalog/sync.ts`:
- Around line 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/.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b9d8ad29-3432-4cda-8339-ff88c77929ef
📒 Files selected for processing (6)
src/codex/catalog/sync.tssrc/codex/refresh.tssrc/codex/sync.tstests/codex-refresh.test.tstests/codex-sync-api.test.tstests/injection-model-api.test.ts
Comment on lines
457
to
+465
| 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: [] }; |
There was a problem hiding this comment.
📐 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
lidge-jun
force-pushed
the
codex/catalog-written-signal
branch
from
July 27, 2026 10:59
1ba588e to
43d0eff
Compare
lidge-jun
force-pushed
the
codex/catalog-written-signal
branch
from
July 27, 2026 11:15
43d0eff to
6462471
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@tests/codex-refresh.test.ts`:
- Around line 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.
In `@tests/injection-model-api.test.ts`:
- Around line 250-254: Update the mock return object in syncCatalogModels to
include the required comboOmissions property as an empty array, while preserving
the existing added, path, and catalogWritten values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bded7192-c70e-40f1-921f-1a5236a89017
📒 Files selected for processing (6)
src/codex/catalog/sync.tssrc/codex/refresh.tssrc/codex/sync.tstests/codex-refresh.test.tstests/codex-sync-api.test.tstests/injection-model-api.test.ts
Comment on lines
+170
to
+216
| 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(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 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
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.
lidge-jun
force-pushed
the
codex/catalog-written-signal
branch
from
July 27, 2026 11:55
6462471 to
ce716cc
Compare
lidge-jun
added a commit
that referenced
this pull request
Jul 27, 2026
Inventory all 23 open issues and 14 open PRs on the axis of decisions only the maintainer can make, clustered into six bundles for later PABCD cycles. Live findings: - anthropics/claude-code#1124, cited as upstream evidence on #543, is CLOSED - #462/#401/#241 carry upstream-tracking with no upstream ticket at all - #92's encrypted_content body has no filed upstream issue either - #527 targets #526's head branch, so enforce-target fails - #529 is already merged; the earlier triage matrix still lists it in flight
lidge-jun
added a commit
that referenced
this pull request
Jul 27, 2026
Two read-only audit lenses returned 30 contradictions; the verified ones are applied here and recorded in 006_corrections.md. Factual corrections: - claude-code#1124 closed 2025-05-16, not 2025-08-10 - 13 open enhancement issues, not 11 - #529 is merged, so issue #42 phase 2 is done - #418 has no PR behind it; it was miscounted as in-flight - #528 does not contain #424's current head, so it needs a rebase - #527's retarget is manual and independent of #526 Framing corrections: - stale-needs-info.yml is absent from the default branch, so bundle C had no real deadline - the debug switch #543's reporter asked for already exists - whether #545 is our defect is answerable from code, not owner judgment - #491 is an OAuth credential change, so it is a security boundary rather than a warm-up; #533 and #447 keep their security classification too Structure: bundle G added for roadmap honesty, bundle F rescoped to the security boundary, #498 split out of bundle B, cycle order rebuilt, and the loop archetype corrected to decision-elicitation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split 1 of 2 from #518 (see #518 for the original combined change).
What
syncCatalogModels()andinvalidateCodexModelsCache()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.syncCatalogModels()returnscatalogWritten.invalidateCodexModelsCache()returns whether it rewrotemodels_cache.refreshCodexModelCatalog()carries both outward.Nothing acts on the signal yet. The consumer — warning about, and optionally restarting, stale Codex app-server processes (#476) — is split 2 and lands on top of this branch.
Why split
The other half sends SIGTERM to matching processes. Process matching is where a mistake is expensive, and it drew four rounds of review feedback on #518. Separating the signal from the action lets that half be reviewed on its own without a 1200-line diff around it.
Test plan
bun run typecheck— exit 0 on this branch alone.bun test tests/codex-refresh.test.ts tests/codex-sync-api.test.ts tests/injection-model-api.test.ts— 22 pass / 0 fail.Derived from #518 at
d93b469; the union of both splits matches that commit's 21 files exactly.Summary by CodeRabbit