fix(codex): fully sync provider switch state for Codex app sessions#346
fix(codex): fully sync provider switch state for Codex app sessions#346luuuuyang wants to merge 1 commit into
Conversation
Code Review by ClaudeThanks for the careful fix, @luuuuyang! 🙏 Routing direct switches through That said, I want to flag two P1 side effects before this lands, plus a few smaller cleanups. 🔴 P1 #1 — Every provider switch full-copies
|
| Location | Before | After |
|---|---|---|
src/commands/config-switch.ts:161 |
switchCodexProvider |
switchToProvider ✅ |
src/commands/init.ts:1287 |
switchCodexProvider |
switchToProvider ✅ |
But the function is still exported, and tests/unit/utils/code-tools/codex-provider-switch.test.ts still pins its old behavior ("only updates model_provider, doesn't touch auth.json"). Two risks:
- If zcf is consumed as a library, downstream callers still using
switchCodexProviderhit the exact half-sync bug from bug(codex): switching provider can hide existing Codex app sessions and direct switch does not fully sync auth #345. - Old tests passing ≠ old behavior is safe — they actively hide the risk.
Suggest one of: add @deprecated JSDoc + a runtime console.warn, or just remove it and migrate the tests onto switchToProvider.
🟢 P3 — Test coverage gaps
The new session-sync test in tests/unit/utils/code-tools/codex.test.ts covers single file + happy path only. Missing:
archived_sessionswith actual files (current mock returns[])- A single file with mixed records:
session_meta+response_item - A session whose
model_provideralready matches → should skip and not callwriteFile(idempotency) - Malformed JSON line → preserved unchanged, no throw
- CRLF line endings (the code handles them, but no regression guard)
writeFilethrows →switchToProvidershould still returntrue
tests/unit/commands/config-switch.test.ts:277 uses mockRejectedValue, but the real switchToProvider has internal try/catch and always returns a boolean — it never throws. Suggest adding a mockResolvedValue(false) case to verify what configSwitchCommand shows the user when a switch fails.
🟢 P3 — Smaller items
JSON.stringify(record)loses the original line's formatting (key order, whitespace). Semantically equivalent, but if any downstream tool does line-level diff or checksums it'll flag the changed lines as "not written by Codex". Worth a one-line comment in the function.- All session I/O is sync (
readDir/readFile/writeFile/isFile). Fine for ~hundreds of files, but a user with thousands of sessions in a deep year/month/day tree will see a noticeable pause. Consider printingSyncing N session files...when N > 100. CODEX_SESSION_DIRECTORIES = ['sessions', 'archived_sessions']is hardcoded — if Codex ever adds a new session location it'll be silently missed. A short comment noting where to update it would help future maintainers.
Summary
| Priority | Issue | Action |
|---|---|---|
| 🔴 P1 | Backup balloons disk | Filter out sessions/archived_sessions |
| 🔴 P1 | Silent rewrite of session ownership | Notify user + preserve original_model_provider |
| 🟡 P2 | Errors swallowed | Track failure count and warn |
| 🟡 P2 | switchCodexProvider half-dead |
@deprecated or remove |
| 🟢 P3 | Test gaps | Add ~5 edge cases |
| 🟢 P3 | Sync I/O | Print progress on large session sets |
The core fix is sound ✅. I'd just recommend handling the two P1 side effects before flipping out of draft, otherwise we'd resolve #345 while introducing two new ones (disk bloat + session-attribution loss).
Thanks again for digging into this — the JSONL-without-SQLite approach is exactly the right call for the Node 18 compat goal. 👏
Summary
Fixes #345.
This keeps Codex provider switching aligned across config, auth, and session visibility:
config-switchCodex provider changes throughswitchToProvider()instead of the partialswitchCodexProvider()pathsession_meta.payload.model_providerinside rollout JSONL files under~/.codex/sessionsand~/.codex/archived_sessionsWhy
switchCodexProvider()only updatesmodel_provider, so direct switches could leaveauth.jsonpointing at the previous provider key. Separately, Codex Desktop/App session visibility can drift when existing rollout metadata still points at the old provider.This implementation stays compatible with Node 18 by syncing JSONL session metadata directly and avoiding any SQLite dependency.
Testing
corepack pnpm vitest "tests/unit/commands/config-switch.test.ts" "tests/unit/commands/config-switch-claude-code.test.ts" "tests/unit/commands/handleMultiConfigurations.test.ts" "tests/unit/commands/init-multi-config.test.ts" "tests/unit/utils/code-tools"corepack pnpm typecheck