Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: luvs01/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const cacheMtimeMs = options.io?.modelsCacheMtimeMs | ||
| ? options.io.modelsCacheMtimeMs() | ||
| : options.io?.catalogMtimeMs ? null : defaultModelsCacheMtimeMs(); | ||
| const writeMtimeMs = catalogMtimeMs === null ? cacheMtimeMs | ||
| : cacheMtimeMs === null ? catalogMtimeMs : Math.max(catalogMtimeMs, cacheMtimeMs); |
There was a problem hiding this comment.
🟡 Startup cache-only writes still miss stale app-servers
Startup cache-only writes call warnIfStaleCodexAppServersAfterStartupWrite, which still compares only the catalog mtime. An app-server started after the catalog but before the cache rewrite receives no stale warning.
Learn more
The CLI startup path explicitly invokes the startup warning after either consumeStartupCacheInvalidationWrite() or startupSync.cacheSynced reports a cache write in handleStart. That warning delegates to the catalog-only collector, so the newly added models-cache boundary applies to interactive post-write handling but not startup cache writes.
Example: The catalog mtime is 1,000 ms, the app-server starts at 2,000 ms, and startup rewrites models_cache.json at 3,000 ms. The startup collector reports fresh from 2,000 > 1,000, although the process predates the cache write and retains the old model list.
Recommended fix: Share the maximum catalog/cache mtime calculation with warnIfStaleCodexAppServersAfterStartupWrite, while preserving its cache reset and no-signal behavior. Add a startup regression test with the catalog older than the process and the models cache newer than it.
Was this helpful? React with 👍 or 👎 to provide feedback.
| After a CLI catalog/cache write, advisory restart guidance compares each running Codex app-server's | ||
| start time with the written catalog mtime. It reports only processes proven stale; a fresh or | ||
| start time with the newest catalog or models-cache mtime. It reports only processes proven stale; a fresh or | ||
| unreadable observation does not claim that another restart is required. Explicit | ||
| `--restart-codex` and `--restart-app-server-only` retain their operator-consent semantics and act on |
There was a problem hiding this comment.
🔍 Public stale-state documentation remains catalog-only
The public sub-agent guide defines freshness only against the catalog. If cache writes belong to the global contract, update that page and its translations.
Was this helpful? React with 👍 or 👎 to provide feedback.
This branch has not been deployed
1 participant
Motivation
models_cache.jsoncould leave a running Codex app-server with an in-memory model list that is newer than the on-disk catalog, but the advisory logic previously compared only the catalog mtime and missed those cases.Description
models_cache.jsonmtime in the post-write freshness boundary by importingactiveCodexModelsCachePathand adding amodelsCacheMtimeMsseam toCodexAppServerProcessIoso tests can inject a cache clock.writeMtimeMsas the newest of the catalog mtime and models-cache mtime and use it for the stale-vs-fresh comparison inafterCatalogWriteHandleAppServers.defaultModelsCacheMtimeMs()that reads the active cache file mtime when an injected seam is not provided.tests/codex-integration/codex-app-server-processes.test.tsto cover the cache-only timing window and updatestructure/runtime.mdto document that advisory guidance compares against the newest catalog or models-cache mtime.Testing
bun test tests/codex-integration/codex-app-server-processes.test.tswith Bun v1.4.0 and the focused suite passed (59 passed, 1 skip).bun run typecheckandbun run structure:check, both succeeded.bun test tests/ci-workflows/file-size-ratchet.test.tsandbun test tests/ci-workflows/repo-hygiene.test.ts, both succeeded.bun run testfor full suite; unrelated shared-state failures surfaced outside the changed subsystem while the focused tests for this change passed. Thetest:changedwrapper could not run due to no resolvable comparison ref in the checkout environment.Codex Task