fix(catalog): skip byte-identical models-cache writes so a start cannot claim disk state changed

[neerajdad123-byte]

Summary

Codex's models cache was rewritten unconditionally on every sync, with no comparison against what was already on disk, and the write was reported as cacheSynced: true regardless of whether any byte changed.

The call chain. ocx start ends in this condition (src/cli/index.ts:622):

if (consumeStartupCacheInvalidationWrite() || startupSync.catalogWritten || startupSync.cacheSynced) {
  warnIfStaleCodexAppServersAfterStartupWrite({ log: console });
}

refreshCodexModelCatalog (src/codex/refresh.ts:61) sets cacheSynced from invalidateCodexModelsCache, which reaches invalidateCodexModelsCacheWithPermit and its unconditional replaceCodexModelsCache.

The bug. Two files are written during a start: the catalog and the cache. The catalog writer was fixed in #1459 / #1460 — it compares bytes, skips an identical write, and reports catalogWritten: false. The cache writer never got that treatment, so cacheSynced was a constant rather than a signal: whenever the catalog file existed, refresh.ts:61 set it to true. That made the condition above permanently true, so a start that changed nothing still reported:

Disk catalog/cache were updated, but Codex may keep showing the old model list until those processes restart.

and pointed the operator at ocx sync --restart-codex, which ends live conversations. Nothing was stale, so the advice was not merely noisy — it was unactionable, because restarting cannot clear a staleness that does not exist. The identical rewrite also moved models_cache.json's mtime for no reason.

Scenario	catalogWritten	cacheSynced before	cacheSynced after	Warning
First run, or the catalog genuinely changed	true	true	true	fires (correct)
Settled start, nothing changed	false	true (bug)	false	silent (correct)
No catalog yet	false	false	false	silent

The fix. One guard, applied before the cache write, moved into src/codex/internal/catalog-writer.ts so both writers share it and cannot drift:

export function preparedBytesDifferFromDisk(prepared: PreparedCatalogFileWrite): boolean {
  let onDisk: Buffer;
  try {
    onDisk = readFileSync(prepared.path);
  } catch {
    return true;
  }
  return !onDisk.equals(Buffer.from(prepared.content, "utf8"));
}

Identical bytes skip the write and report false; different bytes write and report true. An unreadable or absent file reports "differs", so the caller performs the real write and a missing file still converges.

The comparison stays a Buffer rather than a decoded string for the reason #1460 established: readFileSync(path, "utf8") substitutes U+FFFD for every invalid byte, so a malformed cache would compare equal to prepared content holding a real U+FFFD and the guard would preserve the corruption while skipping the atomic repair.

writeRetainedCatalogSync now uses the same shared helper in place of #1460's private copy — no behaviour change there, it is a deduplication.

Why this is a bug and not working as intended. The intended semantics are asserted by this subsystem's own code:

• src/codex/catalog/remote.ts already guards this same pair with a byte comparison and returns cacheSynced: false when the bytes are unchanged.
• src/codex/refresh.ts:53-60 returns cacheSynced: false on both of its no-write paths, commented "Invalidate nothing: rewriting the models cache here would be exactly the routed-cache write the skip refused."
• tests/codex-integration/codex-models-cache-invalidate.test.ts:239 — "Mirrors ocx sync --restart-codex: only handle app-servers after a real write."

The eager invalidation itself was deliberate — refreshCodexModelCatalog's docstring says the cache is forced stale. What went wrong is that only one of two writers received the guard, and once #1459 made the catalog honest, refresh.ts:61 became the line that cancelled that fix out: cacheSynced alone keeps the OR true, so #1459's carefully honest catalogWritten never reaches the condition.

That commit recorded the gap explicitly:

cacheSynced is unaffected (refreshCodexModelCatalog invalidates the models cache whenever the catalog exists, independently of whether it was rewritten).

That is accurate about the invalidation being attempted; the defect is that "attempted" was reported as "wrote".

One honest limit on the claim. The startup warning has a third input, consumeStartupCacheInvalidationWrite(), fed by a separate cache write in src/server/index.ts:275. When that one fires the message is arguably true. So this change does not make the warning always correct — it removes a permanently-true input so the warning reflects actual disk changes instead of firing on every start where the catalog exists.

Adjacent effect. pullRemoteCatalog treats !cacheSynced as a cache-sync failure and rolls the catalog back (restorePreviousCatalog + a write_failed throw). That rollback cannot misfire from this change: the function returns early when the catalog bytes are unchanged, so reaching the cache call implies the catalog changed and the cache changes with it. The change only makes that branch mean what it says.

Review readiness checklist

• All CI tests are green on my local testing. (Named commands and results under Verification above.)
• I pushed my PR to the latest dev commit. (Rebased onto 04761a188; branch is 0 commits behind lidge-jun:dev.)
• I resolved all correct Codex and CodeRabbit findings. (CodeRabbit reported "No actionable comments were generated in the recent review." No Codex findings were posted. CodeRabbit's only non-passing pre-merge item is its Docstring Coverage advisory at 16.67% against an 80% threshold, which is scoped to functions touched by the diff; the one function this diff adds, preparedBytesDifferFromDisk, carries a full JSDoc block. retained-sync.ts is a long-standing file whose functions are documented by inline reasoning comments rather than JSDoc, so bringing it to 80% is unrelated cleanup this change deliberately avoids.)
• My PR is ready for review.

Verification

• bun test tests/codex-integration/codex-catalog-sync-hardening.test.ts tests/codex-integration/codex-catalog-writer.test.ts tests/codex-integration/codex-convergence-account-selectors.test.ts tests/codex-integration/codex-convergence-contract.test.ts → 86 pass / 0 fail
• bun test tests/codex-integration/codex-models-cache-invalidate.test.ts tests/codex-integration/codex-refresh.test.ts → 17 pass / 0 fail (covers invalidateCodexModelsCache reporting real write success and failure, and the #476 / #518 write gate)
• bun test tests/codex-integration/codex-catalog-sync-hardening.test.ts tests/codex-integration/codex-refresh.test.ts tests/codex-integration/codex-catalog-writer.test.ts on the rebased head → 45 tests, 44 pass / 1 fail, the single failure being "Gap B: drops legacy and unentitled account-gated natives…" hitting its 5000 ms per-test timeout while a full-suite run was still releasing resources in parallel. It passes in the 86-pass run above, where it completed in 2248 ms; the timeout was contention, not a regression.
• tsc --noEmit (strict) clean; privacy:scan passed

New regression, "an identical cache resync leaves models_cache untouched, so the startup stale warning stays quiet": asserts the first pass writes (a bare catalog becomes Codex's cache wrapper), then that a second pass is skipped with the mtime unchanged and the bytes identical, and that invalidateCodexModelsCache returns false. The first-pass assertion is deliberate — it fails if the guard is replaced by one that simply refuses every write, so the test cannot pass vacuously.

One existing assertion was updated rather than loosened. "retired Spark cannot return through %s across two sync and cache passes" asserted invalidated: true on both passes while also asserting the second produced identical catalog bytes — a no-op it described as a write. It now asserts the no-op it describes (passes[0].invalidated === true, passes[1].invalidated === false), with the catalog and cache row checks underneath unchanged and still running over both passes.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed. (No user-facing config or CLI surface changed. docs-site/ describes catalog sync behaviour, not this internal return value; the observable change is that a spurious warning stops appearing.)
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. (No credential, auth, or path-resolution logic touched. The guard can only remove writes, never add one.)

Refs #1459

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.

[github-actions]

✅ Deterministic PR hygiene checks passed.

[github-actions]

✅ READY

• all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

• ✅ 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.

✅ 4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers: @lidge-jun @Ingwannu

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: f0754e5a-ff0d-4e3d-bf97-b6d385cbe341

📥 Commits

Reviewing files that changed from the base of the PR and between 04761a1 and 89b587f.

📒 Files selected for processing (3)

• src/codex/catalog/retained-sync.ts
• src/codex/internal/catalog-writer.ts
• tests/codex-integration/codex-catalog-sync-hardening.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

The change centralizes raw-byte comparison for prepared catalog writes. Active catalog and models cache synchronization now skip replacements when bytes are unchanged. Integration tests verify write results, file bytes, and modification times.

Changes

Catalog write hardening

Layer / File(s)	Summary
Prepared byte comparison src/codex/internal/catalog-writer.ts	Adds preparedBytesDifferFromDisk, which compares prepared UTF-8 bytes with raw file bytes and treats missing or unreadable files as different.
Conditional catalog writes src/codex/catalog/retained-sync.ts, tests/codex-integration/codex-catalog-sync-hardening.test.ts	Active catalog and models cache writes now use prepared writes and skip replacement when bytes match. Tests verify first-write invalidation, unchanged second syncs, preserved cache bytes, and unchanged modification times.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
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 and concisely describes the main change: skipping byte-identical models-cache writes to prevent incorrect disk-state reporting during startup.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[lidge-jun]

리뷰 · 우선순위 62 / 80

ocx start는 catalog와 models cache를 쓰는데, cache 쪽은 바이트가 같아도 매번 rewrite하고 cacheSynced: true를 돌려 stale-app-server 경고를 울렸습니다. catalog는 이미 #1459에서 no-op을 했는데 cache만 빠져 있었습니다.

이(draft) PR은 preparedBytesDifferFromDisk를 catalog-writer.ts로 올려 두 writer가 같은 Buffer 비교를 쓰게 하고, invalidateCodexModelsCacheWithPermit이 동일 바이트면 false를 반환합니다. utf8 디코드 비교를 피한 이유도(U+FFFD로 손상 파일을 “동일”로 오인) 주석·기존 catalog 테스트와 맞습니다. hardening 테스트에 identical resync·mtime 유지 케이스를 추가했습니다.

다만 draft이고 enforce-target이 fail입니다. head 메타가 dev로 보이며 외부 기여자(neerajdad123-byte) PR입니다. 의도·코드는 타당해 보이지만, 타깃/브랜치·권한 게이트를 먼저 고쳐야 merge 후보가 됩니다. types/config 스플릿과 직접 충돌하진 않습니다.

경로 preparedBytesDifferFromDisk - catalog·cache 공통 no-op. drift 방지 위치가 맞다
경로 invalidateCodexModelsCacheWithPermit - cacheSynced 의미를 “실제 write”로 되돌린다
테스트 identical resync - first write true / second false / mtime 유지가 경고 오탐을 직접 막는다
상태 draft + enforce-target fail - 리뷰는 하되 merge 불가
작성자/헤드 - fork·브랜치 설정이 로컬 컨벤션과 맞는지 확인 필요

메인테이너의 판단이 필요한 지점

• 동일 수정을 내부 브랜치로 다시 열어 가져갈지, 이 draft를 ready + enforce-target 통과시킬지
• 외부 PR 기여 수용 정책

너의 추천
의도는 merge 가치 있음. draft를 ready로 올리고 enforce-target을 고친 뒤 CI 그린이면 받는다. 게이트가 계속 막히면 같은 변경을 내부 PR로 옮기고 이 PR은 닫는 편이 낫다. types/config 스플릿으로 무효화될 종류는 아니다.

이 댓글은 grok-bot이 작성했습니다