fix(sync): keep Codex CLI state mirror-only

[ndycode]

Summary

• keep Codex CLI compatibility files mirror-only and stop them from seeding or overriding canonical multi-auth storage
• normalize active selection indexes defensively, including fractional values, and clean up the targeted sync regression coverage
• keep the branch scoped to sync behavior only

Why

This is the mirror-sync replacement for the mixed-scope sync slice in #51.

Supersedes #51.

Verification

• npm exec vitest run test/codex-cli-sync.test.ts
• npm run typecheck
• npm run lint
• npm test
• npm run build

Notes

• intentionally excludes the storage recovery flow and the auth/controller extraction from #51
• uses Windows-safe test cleanup in the touched sync suite

---

Summary by cubic

Keep Codex CLI state mirror-only. Sync stops reading Codex files or seeding canonical storage, and only normalizes local selection indexes (including NaN and fractional values) while avoiding spurious writes.

• Bug Fixes
  • syncAccountStorageFromCodexCli no longer reads Codex files; no-ops when canonical storage is missing, clones current storage, normalizes/clamps indexes, and skips writing missing family keys that resolve to the active index to prevent false “changed” signals.
  • normalizeIndexCandidate truncates both value and fallback; used by normalizeStoredFamilyIndexes and getActiveSelectionForFamily to handle NaN/fractional cases (including 3+ accounts).
  • Tests assert mirror-only behavior (no loadCodexCliState calls), add a NaN activeIndex clamp test, retain fractional cases, use Windows-safe cleanup via removeWithRetry with ETIMEDOUT retries, and keep concurrent writer serialization coverage.

^{Written for commit 310772e. Summary will update on new commits.}

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

this pr correctly narrows syncAccountStorageFromCodexCli to a pure mirror-only operation: it no longer reads codex cli files, seeds canonical storage, or imports account data — it only clamps/truncates local selection indexes and no-ops when canonical storage is missing. two previously flagged correctness bugs are both properly resolved: normalizeIndexCandidate now truncates the fallback path (preventing fractional indexes from escaping the Math.max/Math.min clamp in 3+ account scenarios), and the normalizeStoredFamilyIndexes skip logic prevents the false changed: true signal when activeIndexByFamily is empty but all families would resolve to the existing active index.

key changes:

• normalizeIndexCandidate(value, fallback) now calls Math.trunc(fallback) — not just Math.trunc(value) — closing the float-escape path on the NaN branch
• normalizeStoredFamilyIndexes uses !hasFamilyIndex && familyIndex === storage.activeIndex guard to skip redundant family key writes, eliminating spurious atomicWriteJson calls on windows (where extra opens carry EPERM/EBUSY risk during antivirus scans)
• syncAccountStorageFromCodexCli is now a pure synchronous computation wrapped in Promise.resolve — zero file i/o, zero token reads, no windows filesystem windows
• ETIMEDOUT added to RETRYABLE_REMOVE_CODES in test cleanup — correct for windows temp-dir teardown under heavy io
• new vitest cases: mirror-only assertion via loadCodexCliState spy, NaN activeIndex clamp, fractional 3-account path in getActiveSelectionForFamily, and the activeIndexByFamily: {} no-op guard

Confidence Score: 4/5

• safe to merge; all previously flagged correctness bugs are resolved and no new logic issues introduced
• both critical fixes (fallback truncation, spurious changed signal) are correct and well-tested; no file i/o means no windows concurrency surface in this path; one minor gap remains — the NaN/OOB test cases don't assert that activeIndexByFamily stays sparse after normalization, so a future regression that accidentally re-populates all family keys would not be caught by those tests
• test/codex-cli-sync.test.ts — NaN and OOB index test cases should assert activeIndexByFamily remains empty

Important Files Changed

Filename	Overview
lib/codex-cli/sync.ts	stripped to mirror-only semantics; normalizeIndexCandidate now truncates both value and fallback (fixing the float-escape regression); normalizeStoredFamilyIndexes skips writing missing family keys that resolve to activeIndex (fixing spurious changed signal); no file i/o, no token reads — clean from a windows concurrency and token-leakage perspective
test/codex-cli-sync.test.ts	covers mirror-only no-op, NaN activeIndex clamp, fractional 3-account via getActiveSelectionForFamily, empty-family no-change, and concurrent write serialization; ETIMEDOUT correctly added to retryable set; one gap: NaN / OOB tests don't assert activeIndexByFamily remains sparse after normalization

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[syncAccountStorageFromCodexCli] --> B{current is null?}
    B -- yes --> C[reconcileNoops\nreturn storage null, changed false]
    B -- no --> D[cloneStorage\nnext = copy of current]
    D --> E[snapshot previousActive\nand previousFamilies JSON]
    E --> F[normalizeStoredFamilyIndexes next]
    F --> F1[clamp activeIndex via\nnormalizeIndexCandidate + Math.max/min]
    F1 --> F2{for each MODEL_FAMILY}
    F2 --> F3{missing key AND\nresolves to activeIndex?}
    F3 -- skip write --> F2
    F3 -- write value --> F4[set activeIndexByFamily family = familyIndex]
    F4 --> F2
    F2 -- done --> G{previousActive changed\nOR families JSON changed?}
    G -- changed true --> H[reconcileChanges metric\nreturn storage next, changed true]
    G -- no change --> I[reconcileNoops metric\nreturn storage current, changed false]

Loading

_{Last reviewed commit: 310772e}

[coderabbitai]

📝 Walkthrough

Walkthrough

replaces a complex multi-file codex cli reconciliation system with a streamlined local-index normalization approach. the sync function is no longer declared async but still returns a promise. new internal normalizeIndexCandidate and normalizeStoredFamilyIndexes helpers treat codex cli data as non-authoritative, favoring existing local indexes instead.

Changes

Cohort / File(s)	Summary
Sync logic refactor lib/codex-cli/sync.ts	Removed extensive reconciliation code (319 lines) that loaded codex cli state and upserted snapshots. introduced normalizeIndexCandidate and normalizeStoredFamilyIndexes helpers. switched active-index handling to normalized local indexes. changed syncAccountStorageFromCodexCli from async to non-async declaration (still returns Promise). now treats missing canonical storage as no-op.
Test suite revision test/codex-cli-sync.test.ts	Replaced large test suite (868 lines removed, 345 added). introduced removeWithRetry utility with RETRYABLE_REMOVE_CODES for transient fs errors during teardown. added tests verifying canonical storage is not merged from codex cli mirrors, that normalization occurs without reading mirrors, and that active indices clamp/default by family. retained concurrency serialization and deterministic behavior tests.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

bug

---

review notes

flagging three explicit concerns:

1. regression coverage gap – test/codex-cli-sync.test.ts:1 removed 868 lines of tests. confirm the new test suite (345 lines) covers all critical paths the old reconciliation logic handled, especially around snapshot upserts and active-select overwrites. are there edge cases in concurrent writes that the retained serialization tests still catch?

2. windows edge case with retryable removal – test/codex-cli-sync.test.ts:1 introduces removeWithRetry with RETRYABLE_REMOVE_CODES. on windows, file locks during test teardown are common. verify the retry backoff and timeout logic handle slow lock releases, or tests may flake intermittently.

3. concurrency risk with normalized local indexes – lib/codex-cli/sync.ts:1 now relies entirely on normalizeStoredFamilyIndexes without reading codex cli state. if syncAccountStorageFromCodexCli is called concurrently from multiple processes, ensure the normalization is idempotent and doesn't create race conditions when the cloned storage is normalized. is canonical storage mutation guarded elsewhere?

🚥 Pre-merge checks | ✅ 2 | ❌ 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%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	title follows conventional commits format with type 'fix', valid scope 'sync', and descriptive lowercase summary under 72 characters. clearly matches the main changeset of making codex cli state mirror-only.
Description check	✅ Passed	PR description is comprehensive with clear summary, rationale, verification steps, and auto-generated cubic summary. However, the Docs and Governance Checklist section is missing entirely.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch clean/pr51-codex-cli-mirror

---

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 and usage tips.}

[cubic-dev-ai]

No issues found across 2 files

[graphite-app]

Critical bug: when raw is a non-finite number (NaN/Infinity) AND storage.activeIndex is also non-finite, the function returns NaN instead of a valid index.

What breaks:

// Example scenario:
storage = {
  activeIndex: NaN,
  activeIndexByFamily: { codex: NaN },
  accounts: [/* some accounts */]
};
getActiveSelectionForFamily(storage, 'codex');
// Returns NaN because:
// - raw = NaN (typeof NaN === 'number' is true)
// - normalizeIndexCandidate(NaN, NaN) returns NaN
// - Math.max(0, Math.min(NaN, count-1)) = NaN

Fix:
Ensure storage.activeIndex is normalized before using it as a fallback:

const normalizedActiveIndex = normalizeIndexCandidate(storage.activeIndex, 0);
const candidate =
  typeof raw === "number"
    ? normalizeIndexCandidate(raw, normalizedActiveIndex)
    : normalizedActiveIndex;

Suggested change

	const candidate =
	typeof raw === "number"
	? normalizeIndexCandidate(raw, storage.activeIndex)
	: normalizeIndexCandidate(storage.activeIndex, 0);
	return Math.max(0, Math.min(candidate, count - 1));
	const normalizedActiveIndex = normalizeIndexCandidate(storage.activeIndex, 0);
	const candidate =
	typeof raw === "number"
	? normalizeIndexCandidate(raw, normalizedActiveIndex)
	: normalizedActiveIndex;
	return Math.max(0, Math.min(candidate, count - 1));

Spotted by Graphite



Is this helpful? React 👍 or 👎 to let us know.

[ndycode]

@cubic
@greptile
@CodeRabbit

pls review

[cubic-dev-ai]

@cubic
@greptile
@CodeRabbit

pls review

@ndycode I have started the AI code review. It will take a few minutes to complete.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the sync implementation file
fd -e ts sync.ts --search-path lib | head -5

Length of output: 46

---

🏁 Script executed:

#!/bin/bash
# Read codex-cli sync file
fd sync.ts | xargs cat

Length of output: 10704

---

🏁 Script executed:

#!/bin/bash
# Read the test file
fd codex-cli-sync.test.ts | xargs cat

Length of output: 10442

---

🏁 Script executed:

#!/bin/bash
# Look for any other files that import syncAccountStorageFromCodexCli
rg -n "syncAccountStorageFromCodexCli" --type ts -C2

Length of output: 4993

---

🏁 Script executed:

#!/bin/bash
# Look for normalizeIndexCandidate and normalizeStoredFamilyIndexes usages
rg -n "normalizeIndexCandidate\|normalizeStoredFamilyIndexes\|getActiveSelectionForFamily" --type ts -C3

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Check AccountStorageV3 type definition
rg -n "AccountStorageV3\|activeIndexByFamily" --type ts -C3 | head -80

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Check AccountStorageV3 type and activeIndexByFamily
rg -n "AccountStorageV3" lib/ --type ts -C4 | head -100

Length of output: 4631

---

🏁 Script executed:

#!/bin/bash
# Check accounts.ts for call site context around syncAccountStorageFromCodexCli
cat lib/accounts.ts | head -160

Length of output: 4460

---

🏁 Script executed:

#!/bin/bash
# Check MODEL_FAMILIES definition
rg -n "MODEL_FAMILIES" lib/ --type ts -C4 | head -60

Length of output: 3388

---

🏁 Script executed:

#!/bin/bash
# Check incrementCodexCliMetric definition
rg -n "incrementCodexCliMetric\|reconcileAttempts\|reconcileNoops\|reconcileChanges" --type ts -C3 | head -60

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Check accounts-load-from-disk test to confirm mock shape aligns with new non-async signature
cat test/accounts-load-from-disk.test.ts

Length of output: 7680

---

@ndycode — reviewed lib/codex-cli/sync.ts and test/codex-cli-sync.test.ts. overall the mirror-only semantics are correct and the logic is clean. flagging a few items below:

---

lib/codex-cli/sync.ts

shallow clone of accounts entries (Line 10-11)

accounts: storage.accounts.map((account) => ({ ...account })),

each account is shallow-spread, so nested objects (e.g., rateLimitResetTimes, any future sub-objects in AccountMetadataV3) share references with the original. normalization currently only touches activeIndex and activeIndexByFamily so it's safe today, but if a future normalization pass mutates account-level fields it will silently mutate the current object the caller still holds. consider deep-cloning accounts or documenting the constraint.

---

JSON.stringify for change detection (Lines 84–88)

const previousFamilies = JSON.stringify(next.activeIndexByFamily ?? {});
normalizeStoredFamilyIndexes(next);
const changed =
    previousActive !== next.activeIndex ||
    previousFamilies !== JSON.stringify(next.activeIndexByFamily ?? {});

key ordering is stable here because cloneStorage uses spread (insertion-order preserved) and normalizeStoredFamilyIndexes only adds/updates keys in MODEL_FAMILIES iteration order — so both serializations are deterministic within a single process. fine as-is, but a comment noting this assumption would help future maintainers.

---

incrementCodexCliMetric("reconcileAttempts") fires for null input (Line 69)

the metric fires unconditionally before the null-guard. a null current is a no-op, not a reconcile attempt — this can inflate the "reconcileAttempts" counter on dashboards. consider gating the metric after the null check, or renaming the counter to something like "reconcileInvocations" and adding a separate "reconcileSkippedNoCanonical" label for the null path.

---

test/codex-cli-sync.test.ts

missing test: syncAccountStorageFromCodexCli with fractional or non-finite activeIndex

normalizeIndexCandidate handles NaN, Infinity, -Infinity, and fractional values. the "clamps and defaults active selection indexes by model family" test covers getActiveSelectionForFamily with fractional values but there's no integration-level test feeding e.g. activeIndex: 1.9 or activeIndex: Infinity into syncAccountStorageFromCodexCli directly. given that normalizeStoredFamilyIndexes uses normalizeIndexCandidate for activeIndex, a regression here would be silent.

suggested addition to test/codex-cli-sync.test.ts:

it("clamps fractional and non-finite activeIndex through sync", async () => {
    const current: AccountStorageV3 = {
        version: 3,
        accounts: [
            { accountId: "acc_a", email: "a@example.com", refreshToken: "r-a", addedAt: 1, lastUsed: 1 },
            { accountId: "acc_b", email: "b@example.com", refreshToken: "r-b", addedAt: 1, lastUsed: 1 },
        ],
        activeIndex: 1.9,          // fractional — should truncate to 1
        activeIndexByFamily: {},
    };
    const result = await syncAccountStorageFromCodexCli(current);
    expect(result.changed).toBe(true);
    expect(result.storage?.activeIndex).toBe(1);

    const nonFinite: AccountStorageV3 = {
        version: 3,
        accounts: [
            { accountId: "acc_a", email: "a@example.com", refreshToken: "r-a", addedAt: 1, lastUsed: 1 },
        ],
        activeIndex: Number.POSITIVE_INFINITY,
        activeIndexByFamily: {},
    };
    const r2 = await syncAccountStorageFromCodexCli(nonFinite);
    expect(r2.changed).toBe(true);
    expect(r2.storage?.activeIndex).toBe(0); // clamped to count-1 = 0
});

---

Windows edge case — removeWithRetry max attempt delay

the retry back-off is 25 * 2 ** attempt ms, reaching 800ms on attempt 5 (total ~1.6s). on busy Windows CI agents EPERM can persist longer than that during antivirus scans. not a blocker but worth a note in the utility's comment.

---

lib/accounts.ts call site

lib/accounts.ts:113 — await syncAccountStorageFromCodexCli(stored) still works correctly since the function returns Promise.resolve(...). no change needed.

---

summary

severity	location	issue
medium	lib/codex-cli/sync.ts:10	shallow account clone — maintenance risk for future normalization passes
low	lib/codex-cli/sync.ts:69	reconcileAttempts metric fires on null no-op path
low	lib/codex-cli/sync.ts:84	JSON.stringify change detection assumes stable key order — add comment
low	test/codex-cli-sync.test.ts	missing syncAccountStorageFromCodexCli test for fractional/non-finite activeIndex
info	test/codex-cli-sync.test.ts:removeWithRetry	Windows retry window may be too short on slow CI

logic for mirror-only enforcement, index clamping, skip condition, and concurrency serialization all look correct. 👍

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cubic-dev-ai]

No issues found across 2 files