fix(catalog): skip byte-identical catalog writes so a resync cannot mark Codex stale - #1460
fix(catalog): skip byte-identical catalog writes so a resync cannot mark Codex stale#1460tlsdnwn55 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCatalog synchronization now compares generated catalog bytes with the existing file before writing. Unchanged syncs preserve the file modification time and report ChangesCatalog synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
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)
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 |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The reported issue is real, and this is the right minimal direction: avoiding a true no-op write keeps the catalog mtime meaningful without weakening the existing permit, revalidation, or atomic-write path. I reproduced the focused suite on exact PR head 7bf8b84a after installing the locked dependencies: 100 pass / 0 fail; typecheck and privacy scan also pass.
I am requesting one correctness change before this becomes review-ready:
currentCatalogFileContent() in src/codex/catalog/sync.ts claims to compare exact bytes, but readFileSync(path, "utf8") === content compares decoded JavaScript strings. Invalid UTF-8 bytes are replaced with U+FFFD, so different on-disk bytes can compare equal to a legitimately encoded replacement character. I reproduced the boundary directly: a buffer containing raw byte 0x80 and a buffer containing UTF-8 EF BF BD have equal decoded strings but unequal bytes. In a catalog string field, this path can therefore preserve malformed bytes, skip the atomic repair write, and return catalogWritten: false even though the prepared bytes are not identical to the file.
Please read the file as bytes and compare it with the UTF-8 bytes of content (Buffer.equals or an equivalent exact Uint8Array comparison), while preserving the current unreadable/absent fallback to a real write. Add a regression where an invalid on-disk byte decodes to the same string as a real U+FFFD in the prepared content and assert that the sync still rewrites it. The existing mtime no-op regression should continue to prove that genuinely identical bytes are skipped.
The branch is currently four commits behind dev, still inside the repository's accepted freshness window. Once the exact-byte boundary is fixed, the focused checks are rerun, and the four readiness boxes are completed against the resulting head, this remains a strong merge candidate for #1459.
7bf8b84 to
0b0fee6
Compare
|
@Ingwannu thanks — the finding is correct and is fixed on new head
Regression added: Re-verified on The branch was force-pushed to rewrite the commit author, so the SHA you reviewed is superseded. Sorry for the extra round. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…ark Codex stale `writeRetainedCatalogSync` wrote the catalog unconditionally, with no content comparison, so every sync moved the file's mtime even when the produced bytes were identical to what was already on disk. `collectCodexAppServerCatalogState()` compares that mtime against each running Codex's start time, so an ordinary `ocx start` — or any dashboard action that re-syncs an unchanged model set — classified every already-running Codex as holding an outdated in-memory catalog. Since lidge-jun#1407 that verdict withholds all opencodex-authored v2 model guidance, so a configured `injectionModel` and roster silently stop reaching sessions for the rest of that Codex's lifetime, even though the advertised model set never changed. A long-lived Codex App app-server outlives every proxy restart, which makes the state effectively latched: restarting the CLI does not clear it. An identical write is now skipped and reported as `catalogWritten: false`. `added` still reports the routed rows the catalog carries, because they are on disk either way, and `cacheSynced` is unaffected (`refreshCodexModelCatalog` invalidates the models cache whenever the catalog exists, independently of whether it was rewritten). The comparison is byte-exact, not a decoded-string comparison. Reading the file as UTF-8 replaces every invalid sequence with U+FFFD, so a malformed byte on disk would compare equal to a legitimately encoded replacement character in the prepared content: the skip would then preserve the corruption and report `catalogWritten: false` for a file that is not what we prepared. An unreadable or absent file still falls back to a real write. The convergence/retained-sync byte-agreement assertions asserted `catalogWritten: true` for exactly the no-op case while also asserting the bytes did not change; they now assert the no-op they describe (no policy skip, no write, bytes unchanged). Assertions covering syncs that really change content are untouched.
0b0fee6 to
642805c
Compare
|
Rebased onto the current Verified locally on
Scope note on the full-suite claim, so the attestation is precise: the last full |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR head and changed files ==='
git rev-parse HEAD
git diff --name-status HEAD^1 HEAD
echo '=== Diff ==='
git diff --find-renames --unified=80 HEAD^1 HEAD -- \
src/codex/catalog/sync.ts \
tests/codex-catalog-sync-hardening.test.ts \
tests/codex-convergence-account-selectors.test.ts \
docs-site/src/content/docs/guides/sub-agent-surface.md
echo '=== sync.ts outline ==='
ast-grep outline src/codex/catalog/sync.ts --items all --view expanded
echo '=== Catalog write/read references ==='
rg -n -C 4 -P '\b(currentCatalogFileBytes|currentCatalogFileContent|catalogWritten|writeFile|readFile|withPermit|permit)\b' \
src/codex/catalog/sync.ts tests/codex-catalog-sync-hardening.test.ts tests/codex-convergence-account-selectors.test.tsLength of output: 78987 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== syncCatalogModels commit path ==='
sed -n '1450,1520p' src/codex/catalog/sync.ts
echo '=== Catalog replacement primitive definitions and callers ==='
rg -n -C 8 -P '\b(function|const)\s+(replaceActiveCodexCatalog|withCatalogWriteSerialization)\b|\breplaceActiveCodexCatalog\s*\(|\bwithCatalogWriteSerialization\s*\(' src
echo '=== catalogWritten consumers ==='
rg -n -C 8 -P '\bcatalogWritten\b' --glob '!tests/**' .
echo '=== Refresh and cache synchronization entry points ==='
rg -n -C 6 -P '\b(syncCatalogModels|cacheSynced|invalidateCodexModelsCache|refreshCodexModelCatalog)\s*\(' srcLength of output: 80627
I found no blocking issues. The update in The tests cover unchanged catalog bytes, real catalog changes, malformed UTF-8 with replacement-decoding equivalence, and convergence output. You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Landed on Your fix is on You independently reached the same Regression on Thank you — this was a good catch and a clean fix. |
3 participants
Summary
writeRetainedCatalogSyncwrote the Codex catalog unconditionally —replaceActiveCodexCatalog→atomicWriteFile, with no comparison against the bytes already on disk — so every sync moved thecatalog file's mtime even when the produced content was identical and the advertised model set did
not change.
collectCodexAppServerCatalogState()compares that mtime against each running Codex's start time.An ordinary
ocx start, or any dashboard/CLI action that re-syncs an unchanged model set, thereforeclassified every already-running Codex as holding an outdated in-memory catalog. Since #1407 that
verdict withholds all opencodex-authored v2 model guidance, so a configured
injectionModelandsubagentModelsroster silently stop reaching the session for the rest of that Codex's lifetime,even though nothing about the catalog changed. A long-lived Codex App app-server outlives every
proxy restart, which makes the state effectively latched: restarting the Codex CLI does not clear it.
This PR skips the write when the prepared bytes equal what is on disk, and reports
catalogWritten: falsefor that case.readFileSync(path, "utf8")replaces every invalid sequence with
U+FFFD, so a malformed byte on disk would compare equal toa legitimately encoded replacement character in the prepared content; the skip would then preserve
the corruption and report
catalogWritten: falsefor a file that is not what we prepared. Anunreadable or absent file still falls back to a real write.
addedstill reports the routed rows the catalog carries, because they are on disk either way.cacheSyncedis unaffected:refreshCodexModelCataloginvalidates the models cache whenever thecatalog exists, independently of whether it was rewritten. The stale-app-server hint and the
startup cache-invalidation branch both key on
cacheSyncedas well, so their behaviour isunchanged.
#1046covered warning about stale app-servers after a catalog rewrite; it did not coveravoiding the rewrite when there is nothing to write.
Closes #1459
Verification
bun x tsc --noEmit— clean.bun test tests/codex-catalog-sync-hardening.test.ts tests/codex-convergence-account-selectors.test.ts tests/codex-refresh.test.ts tests/codex-sync-api.test.ts tests/codex-catalog-restore.test.ts tests/codex-retained-root-serialization.test.ts— 72 pass, 0 fail.
bun run test(full suite) — 10871 pass, 7 skip, 1 fail. The single failure,server combo failover 030 activation matrix > connect cancellation wins with 499, no backup, warning, or cooldown(30s hook timeout), reproduces identically with this branch's changesstashed on unmodified
dev, and touches neither catalog sync nor guidance. That run predates therebase onto current
devand the byte-exactness change.bun run privacy:scan— passed.secondWritten(Expected: false, Received: true), andthe same test mutates the catalog on disk and asserts the next sync does rewrite it, so the
guard is not vacuous;
a malformed on-disk byte that decodes to the same string is still repairedonwritten(
Expected: true, Received: false). That test corrupts one byte inside a preserved JSON stringvalue to
0x80, then asserts the repaired file differs in bytes while decoding to the samestring as the corrupted file — exactly the pair a string comparison equates — and that the
result is valid UTF-8 containing
EF BF BD.@bitkyc08/opencodex2.12.0, Codex CLI 0.147.0(
originator: codex-tui), Codex App running:collectCodexAppServerCatalogState()reportedstalesolely because of the Codex App app-server (started 02:43:11Z) against a catalog whosemtime had been moved to 02:52:27Z by a resync, while both live CLI-owned processes were newer.
Checklist
section now states that a byte-identical resync leaves the file untouched.
credential, workflow, or release-automation surface is touched. The write path keeps its
existing permit check, write serialization, and atomic-write behaviour; the only change is an
equal-content short-circuit evaluated inside the same serialized region.
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Bug Fixes
Documentation