fix(catalog): stop respawning the codex --version probe on every catalog read

[mihneaptu]

Fixes the root cause behind #606.

Problem

loadBundledCodexCatalog() spawns codex --version on every call, including warm
60s cache hits. On Windows that pushes /api/claude-code and /v1/models to 5-8s,
past the 3s budget cmdClaude allows each, so ocx claude aborts both and prints:

⚠ 모델 컨텍스트 정보를 불러오지 못했습니다 — 1M 자동 표시는 이번 실행에서 생략됩니다.
⚠ Gateway model cache could not be refreshed; the model picker may be stale.

The consequences are quiet: [1m] marking is skipped for the run, and
~/.claude/cache/gateway-models.json is never rewritten. Since Claude Code only
refreshes that cache itself when it holds a credential — and the
subscription-preserving launch deliberately injects none — the picker keeps serving
whatever is on disk. Mine was 6 days stale (80 entries vs 98 current).

Root causes

1. The bundled-catalog cache can never be hit. candidates is computed in an
IIFE that runs before the bundledCatalogCache lookup, because cacheKey is
assigned inside it — so the runtime resolve happens on every call. That would still
be cheap if it were memoized, but the call forwarded its own already-defaulted
execFileSync:

const execFile = deps.execFileSync ?? (execFileSync as unknown as ExecFile);
// ...
resolveAndPersistCodexRuntime({ execFileSync: execFile, /* always truthy */ });

and resolveCacheKey() deliberately refuses to memoize injected-dependency
resolves. The guard is right in intent (injected deps imply a test double), but this
caller defeated it for the real-runtime path. Now only a genuinely injected
execFileSync is forwarded.

2. The persist write cleared its own memo. resolveAndPersistCodexRuntime()
wrote on any non-fallback selection. persistCodexRuntime() ends with
resolveCache = null, and persistedRuntimeCacheStamp() folds the on-disk
updatedAt into the cache key — so each write both cleared the memo and changed the
key it would have been stored under, even when the resolved runtime was
byte-identical to what was already persisted. Now it only writes when the selection
actually changed.

3. Cold reads probed every codex on PATH. The catalog path passed
discoverAlternatives: deps.discoverAlternatives (i.e. undefined), which
resolveCodexRuntimeUncached() treats as "discover". It then probed every candidate
to populate newerAvailable, each probe spawning codex --version with a fresh
mkdtempSync sandbox:

resolveCodexRuntime({})                              1224 ms   failures: 102
resolveCodexRuntime({ discoverAlternatives: false })    60 ms

Both select the identical runtime, and catalog loading never reads newerAvailable
or failures. Defaulted to false here; callers that want discovery diagnostics
still opt in explicitly.

Measurements

/api/claude-code over HTTP, Windows 11:

	before	after
cold (fresh proxy)	5000-8300 ms	165 ms
warm	6300 ms	~25 ms
after 65s idle (cache expired)	6325 ms	1440 ms

loadBundledCodexCatalog() three times in a row, nothing changing on disk:

before:  1214 ms / 1136 ms / 1186 ms
after:   1483 ms /    1 ms /    0 ms

After the fix ocx claude runs clean and gateway-models.json refreshed from 80
stale entries to 98 current ones, 25 carrying [1m].

Tests

Three regression tests in tests/codex-runtime.test.ts, all failing on dev and
passing here:

• repeated catalog reads reuse the runtime probe instead of respawning it —
  uses a real launcher script that appends to a log on each --version, so it counts
  actual subprocess spawns rather than mocking them. Asserts the count stops growing
  across repeated reads. Note it deliberately allows the second read to probe once
  more: the first read has no persisted selection yet, so it legitimately writes one
  and that write clears the memo. Observed progression is [1,2,2,2,2,2] — the
  invariant is convergence, not a single probe. The POSIX launcher uses shell built-ins
  only, so the test remains isolated with PATH="" on Linux and macOS.
• repeat resolveAndPersistCodexRuntime keeps an unchanged selection's persisted stamp — injects a monotonic clock and asserts an identical selection does not
  rewrite updatedAt.
• treats missing persisted and resolved versions as the same selection —
  covers JavaScript callers that surface undefined despite the TypeScript null
  contract, preventing the same rewrite/memo invalidation loop for unversioned runtimes.

Verification

• bun run typecheck — clean
• bun run privacy:scan — passed
• bun test tests/codex-runtime.test.ts tests/codex-catalog*.test.ts — 145 pass,
  0 fail across 5 files (655 assertions)
• Confirmed all three regression tests fail against the relevant prior implementation
• POSIX launcher smoke-tested through Git Bash with PATH=""

One gap I should be upfront about: the full bun run test sweep did not complete on
my machine. Bun 1.3.14 on Windows panics with Internal assertion failure at
~2.9GB peak RSS partway through the 415-file suite — the same runtime memory issue
ocx doctor warns about on this platform. It is not triggered by this change, so
please treat CI's cross-platform run as authoritative for the full suite.

Suggestions beyond this fix (not implemented here)

1. Check bundledCatalogCache before resolving the runtime. Deriving the cache
   key from the very probe the cache exists to avoid means a warm hit can only ever
   save the codex debug models call (~87ms), never the probe (~1.2s) that
   dominates. A cheaper key would let warm hits skip both. I left the ordering alone
   to keep this change minimal.
2. Attribute the timeout in the warning text. Both messages read like auth or
   network faults; something like timed out after 3000ms would point straight at
   the cause. I went looking at OAuth state and provider connectivity first.
3. Consider making the 3s budget configurable. Even fully patched, a cold read is
   ~1.4s here, so a slower machine or longer PATH could still trip it silently.

Tradeoff worth reviewing

Defaulting discoverAlternatives to false on the catalog path means catalog-path
callers no longer receive newerAvailable diagnostics. Nothing in that path consumes
them today, but if you would rather keep discovery observable, moving it to a slow
background interval instead of every catalog read would preserve the diagnostics
while keeping the request path fast. Happy to rework it that way.

Summary by CodeRabbit

• Bug Fixes
  • Improved Codex runtime caching so repeated bundled catalog reads reuse the existing version probe.
  • Avoids rewriting the persisted Codex runtime when the resolved selection is unchanged, preserving timestamps.
  • Improved bundled runtime discovery behavior when optional injected execution settings are present, including safer defaults for alternative discovery.
• Tests
  • Added/expanded coverage for cache warm-start behavior and persisted-selection stability.

[coderabbitai]

📝 Walkthrough

Walkthrough

Codex catalog loading now preserves injected probing behavior and defaults alternative discovery to disabled. Runtime persistence skips rewrites when the resolved selection is unchanged, with tests covering warm-read probe reuse and timestamp preservation.

Changes

Codex runtime caching

Layer / File(s)	Summary
Runtime resolution and persistence src/codex/catalog/bundled.ts, src/codex/runtime.ts	Bundled catalog loading conditionally forwards an injected executable probe and defaults discoverAlternatives to false; runtime state is rewritten only when the non-fallback selection changes.
Caching regression coverage tests/codex-runtime.test.ts	Tests verify repeated catalog reads reuse runtime probes, unchanged selections preserve updatedAt, and missing versions do not trigger rewrites.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

• ocx claude: 3s budget exceeded by uncached codex --version probe on /api/claude-code and /v1/models #606 — Covers the Codex catalog caching, dependency injection, default discovery, conditional persistence, and regression-test changes implemented here.

Possibly related PRs

• lidge-jun/opencodex#359 — Modifies the same Codex runtime resolution and persistence path.

Suggested reviewers: wibias, lidge-jun, ingwannu

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: reducing repeated codex --version probing during bundled catalog reads.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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 `@src/codex/runtime.ts`:
- Around line 509-514: Update the selection comparison in the persisted-runtime
flow around persistedRuntime and selectionUnchanged to normalize
result.runtime.version the same way as persistedRuntime.selectedVersion,
treating an absent resolved version as null. Preserve the existing command,
source, and rewrite conditions so unversioned runtimes remain unchanged across
reloads.

In `@tests/codex-runtime.test.ts`:
- Around line 385-405: Update the test around resolveAndPersistCodexRuntime to
inject a monotonic now function through deps, returning different timestamps on
successive calls. Ensure the repeated identical selection exercises a potential
second persistence attempt and assert the persisted updatedAt remains the first
timestamp, making the no-rewrite check deterministic.

🪄 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: c812c313-8e60-48a1-a694-fb4c518d6666

📥 Commits

Reviewing files that changed from the base of the PR and between 7710185 and 1310dd2.

📒 Files selected for processing (3)

• src/codex/catalog/bundled.ts
• src/codex/runtime.ts
• tests/codex-runtime.test.ts

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1310dd20b9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lidge-jun]

The P1 on the POSIX launcher does not reproduce — measured

Reviewing this from the maintainer side. You already addressed the bot's P1 in 056aa2d6e with the builtin launcher (d=${0%/*} plus a while IFS= read -r line || [ -n "$line" ] loop), and that form is correct and slightly better than what I would have written — the || [ -n "$line" ] guard keeps a final line with no trailing newline.

Worth recording, though: the failure the P1 predicts does not actually occur, so you were not blocked by a real defect. I measured it three ways against dev.

1. The catalog does load. I temporarily instrumented loadBundledCodexCatalog() inside the persisted runtime stamp busts resolve memo test on dev (the version still using cat "$(dirname "$0")"):

[PROBE] oldCatalog = loaded, models=1
[PROBE] newCatalog = loaded, models=1
(pass) resolveCodexRuntime > persisted runtime stamp busts resolve memo [140.50ms]

Not null — the launcher produced its catalog.

2. A null return would have failed the test, not passed it. The natural worry is that optional chaining makes a null catalog satisfy .toBe(false). It does not:

expect(undefined).toBe(false)
  Expected: false
  Received: undefined
(fail)

So the assertion pair is a real oracle for catalog loading; it is not silently green.

3. PATH="" never reaches the child. runCodexDebugModels() at src/codex/catalog/bundled.ts:137-143 calls execFile without an env option, so the child inherits the environment captured at process start — a later process.env.PATH = "" does not propagate. Same launcher, three conditions:

A: process.env.PATH='' , no env opt      OK    {"models":[{"slug":"x"}]}
B: explicit env {...process.env}         THREW 127
C: explicit env {PATH:''}                THREW 127

A is the path this code actually takes. B and C are what you get when env is passed explicitly, which is presumably how the reviewer modelled it.

One real observation this did surface, unrelated to your PR: because PATH="" does not propagate, the isolation that line intends is not in effect. The test is still safe today — it pins the launcher through CODEX_CLI_PATH and persistCodexRuntime() with absolute paths — but the guard is decorative rather than load-bearing. Changing how runCodexDebugModels passes env would touch probe behavior and overlaps your change, so I am leaving it for a maintainer call rather than patching around you.

Nothing needed from you on this. Your builtin launcher is a fine improvement regardless.

[lidge-jun]

Correction to my previous comment — two things I got wrong

A follow-up review caught two errors in what I wrote above, and one of them was bad advice about this codebase. Correcting both, with measurements.

1. The env behavior is Bun-specific, not general. I wrote that "the child inherits the environment captured at process start" as though that were ordinary execFile behavior. It is not. Same script, both runtimes:

process.env.PATH = "";  execFileSync(bin, [], { encoding: "utf8" })

BUN   -> CHILD_PATH=[/Users/.../bin:/opt/homebrew/bin:...]   ← original PATH
NODE  -> CHILD_PATH=[]                                        ← mutation propagates

Bun snapshots the environment at startup for child spawns and does not observe later process.env mutations; Node does. So the P1's reasoning is correct under Node semantics — it simply does not fire on our Bun runtime. The bot was not wrong about the shell; it was reasoning about a runtime we do not use.

That also means the test's green state rests on a Bun implementation detail. If Bun ever aligns with Node here, tests/codex-runtime.test.ts:360 starts failing with no code change. Worth knowing.

There is a related asymmetry I should have spotted: src/codex/runtime.ts:261 does pass env explicitly ({ ...(deps.env ?? process.env), CODEX_HOME: probeHome }), so within one test run the --version probe sees PATH="" while the catalog probe sees the original PATH. --version survives only because echo is a shell builtin needing no lookup. That asymmetry is the actual reason the suite is green.

2. PATH="" is load-bearing — I was wrong to call it decorative. This is the part worth retracting properly, since it was advice about your test's surroundings.

PATH is not only consumed by child processes. pathCandidates() at src/codex/runtime.ts:308-312 reads deps.env ?? process.env and splits PATH to build the candidate binary list in-process, where the mutation is visible immediately. Measured with a fake codex planted on PATH:

PATH=<fakedir> -> source: path      version: 9.9.9
PATH=''        -> source: fallback  version: null

So that line does exactly what it was written to do: it keeps a machine-installed codex out of the candidate list. The CODEX_CLI_PATH pinning is a second layer, not a substitute. Anyone acting on my earlier "decorative" framing and deleting it would have made the test machine-dependent.

My NOOP conclusion stands — nothing here needs fixing, and nothing is needed from you. But the reasoning I published was wrong in both places, so it seemed worth correcting rather than leaving on the record.

[mihneaptu]

Ping on CI approval — the two pull_request workflows (Cross-platform CI, React Doctor) are sitting in action_required, so the full suite has not run on this branch yet.

Since I flagged in the description that my local bun run test sweep could not complete (Bun 1.3.14 panics with Internal assertion failure at ~2.9GB RSS on Windows partway through the 415-file suite), CI is the authoritative signal here, and I would rather not push for review before it is green.

Current state, for convenience:

• All three review threads are resolved — the bot P1 on the POSIX launcher was addressed in 056aa2d6e with a builtins-only launcher (d=${0%/*} plus a while IFS= read -r line || [ -n "$line" ] loop).
• Still mergeable against dev. The two commits it is behind (c48a6eee3, ca7b1049f) touch unrelated files, so there is no overlap with the three files here.
• Head is 056aa2d6e; I have not pushed since, so an approved run would cover the reviewed commit.

Whenever one of you has a moment to approve the run, thanks.