feat(sync): add sync center and status surface#107
feat(sync): add sync center and status surface#107ndycode wants to merge 20 commits intofresh/02-backup-restore-managerfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughthe pr adds an interactive-only startup recovery prompt and backup restore manager, a preview-first sync-center with apply/rollback, a full codex-cli sync subsystem with pending-run tracking, and threads sync commit/commit-failure into account persistence paths. it also adds UI text and many tests. Changes
Sequence Diagram(s)sequenceDiagram
participant user as client
participant cli as codex auth login
participant recovery as startup recovery
participant storage as account storage
participant codexcli as codex cli state
user->>cli: start login (zero accounts)
cli->>recovery: resolveStartupRecoveryAction()
recovery->>storage: getActionableNamedBackupRestores()
storage-->>recovery: backup assessments
cli->>cli: isInteractiveLoginMenuAvailable()
alt interactive + backups available
cli-->>user: prompt restore manager
user->>cli: choose restore or skip
alt restore
cli->>recovery: runBackupRestoreManager()
recovery->>storage: perform restore
storage-->>recovery: restored accounts
recovery->>cli: continue to oauth/login
else skip
cli->>codexcli: proceed fresh login
end
else non-interactive / none
cli->>codexcli: proceed fresh login (prompt suppressed)
end
sequenceDiagram
participant user as settings user
participant settings as settings hub
participant sync as sync center
participant codexcli as codex cli state
participant storage as account storage
user->>settings: open settings
user->>settings: select sync-center
settings->>sync: promptSyncCenter()
sync->>codexcli: loadCodexCliState()
codexcli-->>sync: state/snapshots
sync->>storage: loadAccountsReadOnly()
storage-->>sync: current storage
sync->>sync: previewCodexCliSync()
sync-->>user: display preview
alt user applies
user->>sync: apply
sync->>storage: applyCodexCliSyncToStorage()
storage-->>sync: {storage, changed, pendingRun}
sync->>codexcli: commitPendingCodexCliSyncRun(pendingRun)
sync-->>user: success
else user refreshes
user->>sync: refresh
sync->>codexcli: loadCodexCliState(forceRefresh: true)
codexcli-->>sync: new state
sync->>sync: regenerate preview
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes notes and reviewer flags:
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds a new Sync Center surface for preview/apply Codex CLI sync flows and introduces startup recovery prompting for named backups when starting from an empty account store.
Changes:
- Adds Sync Center UI copy, settings-hub menu entry, overview formatting, and interactive prompt flow (refresh/apply).
- Adds session-scoped “last sync run” tracking + preview/apply APIs for Codex CLI sync, plus richer live watcher status publishing.
- Refactors named-backup scan/assessment/restore flows and updates related tests/docs.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| test/storage.test.ts | Updates storage tests to match new backup/import/export semantics and reduced transient/containment behaviors. |
| test/storage-recovery-paths.test.ts | Adjusts recovery-path metadata test to manipulate checkpoint mtime. |
| test/settings-hub-utils.test.ts | Adds coverage for Sync Center overview formatting and retry behavior in the prompt flow. |
| test/live-account-sync.test.ts | Extends tests for published watcher snapshots and reload queuing/path switching behaviors. |
| test/codex-cli-sync.test.ts | Adds extensive coverage for preview/apply sync behavior, selection precedence, retries, and published run tracking. |
| test/accounts.test.ts | Adds tests ensuring pending sync runs are committed only after storage persistence (and failures recorded on persist errors). |
| test/accounts-load-from-disk.test.ts | Updates loadFromDisk tests to validate pending-run commit/failure behavior. |
| test/accounts-edge.test.ts | Expands mocking to include sync-run commit/failure hooks. |
| lib/ui/copy.ts | Adds Sync Center UI strings and removes restore-backup success/confirm copy helpers. |
| lib/storage.ts | Refactors named-backup listing/scanning, adjusts restore/import/export contracts, adds saveAccounts options, and introduces actionable restore discovery. |
| lib/live-account-sync.ts | Adds global “last watcher snapshot” publishing across instances, generation tracking, and reload queuing. |
| lib/codex-manager/settings-hub.ts | Implements Sync Center menu, overview builder, retry-wrapped preview/apply flow, and ties into backup policy + published sync run state. |
| lib/codex-manager.ts | Adds startup recovery prompt flow and refactors backup restore manager; tweaks relative-date formatting. |
| lib/codex-cli/sync.ts | Adds preview/apply APIs, run tracking with pending commits, selection precedence logic, and retryable persisted-timestamp reads. |
| lib/cli.ts | Extracts reusable “interactive login menu available” predicate. |
| lib/accounts.ts | Commits or records failures for pending Codex CLI sync runs based on persistence outcome. |
| docs/upgrade.md | Documents startup recovery prompt behavior. |
| docs/troubleshooting.md | Notes recovery prompt behavior during login. |
| docs/reference/settings.md | Documents Sync Center behavior and what it displays. |
| docs/reference/public-api.md | Removes prior compatibility note related to importAccounts() return shape. |
| docs/getting-started.md | Notes recovery prompt behavior in the login flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export async function restoreNamedBackup( | ||
| name: string, | ||
| ): Promise<ImportAccountsResult> { | ||
| const assessment = await assessNamedBackupRestore(name); | ||
| return restoreAssessedNamedBackup(assessment); | ||
| } | ||
|
|
||
| export async function restoreAssessedNamedBackup( | ||
| assessment: Pick<BackupRestoreAssessment, "backup" | "eligibleForRestore" | "error">, | ||
| ): Promise<ImportAccountsResult> { | ||
| if (!assessment.eligibleForRestore) { | ||
| throw new Error( | ||
| assessment.error ?? "Backup is not eligible for restore.", | ||
| ); | ||
| } | ||
| const resolvedPath = await resolveNamedBackupRestorePath( | ||
| assessment.backup.name, | ||
| ); | ||
| return importAccounts(resolvedPath); | ||
| ): Promise<{ imported: number; total: number; skipped: number }> { | ||
| const backupPath = await resolveNamedBackupRestorePath(name); | ||
| return importAccounts(backupPath); | ||
| } |
lib/storage.ts
Outdated
| export async function importAccounts( | ||
| filePath: string, | ||
| ): Promise<ImportAccountsResult> { | ||
| ): Promise<{ imported: number; total: number; skipped: number }> { |
lib/storage.ts
Outdated
| const merged = [...existingAccounts, ...normalized.accounts]; | ||
|
|
||
| if (!changed) { | ||
| return { | ||
| imported, | ||
| total: deduplicatedAccounts.length, | ||
| skipped, | ||
| changed, | ||
| }; | ||
| if (merged.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) { | ||
| const deduped = deduplicateAccounts(merged); | ||
| if (deduped.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) { | ||
| throw new Error( | ||
| `Import would exceed maximum of ${ACCOUNT_LIMITS.MAX_ACCOUNTS} accounts (would have ${deduped.length})`, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| const deduplicatedAccounts = deduplicateAccounts(merged); |
lib/storage.ts
Outdated
| await persist(newStorage); | ||
| return { | ||
| imported, | ||
| total: deduplicatedAccounts.length, | ||
| skipped, | ||
| changed, | ||
| }; | ||
|
|
||
| const imported = deduplicatedAccounts.length - existingAccounts.length; | ||
| const skipped = normalized.accounts.length - imported; | ||
| return { imported, total: deduplicatedAccounts.length, skipped }; |
| const transactionState = transactionSnapshotContext.getStore(); | ||
| const currentStoragePath = normalizeStoragePathForComparison(getStoragePath()); | ||
| const currentStoragePath = getStoragePath(); | ||
| const storage = transactionState?.active | ||
| ? normalizeStoragePathForComparison(transactionState.storagePath) === | ||
| currentStoragePath | ||
| ? transactionState.storagePath === currentStoragePath | ||
| ? transactionState.snapshot | ||
| : (() => { |
lib/storage.ts
Outdated
| } | ||
| throw error; | ||
| } | ||
| const content = await fs.readFile(resolvedPath, "utf-8"); |
lib/storage.ts
Outdated
| const mergedAccounts = deduplicateAccounts([ | ||
| ...currentDeduplicatedAccounts, | ||
| ...incomingDeduplicatedAccounts, | ||
| ...currentAccounts, | ||
| ...candidate.normalized.accounts, | ||
| ]); | ||
| const wouldExceedLimit = mergedAccounts.length > ACCOUNT_LIMITS.MAX_ACCOUNTS; | ||
| const imported = wouldExceedLimit | ||
| ? null | ||
| : mergedAccounts.length - currentDeduplicatedAccounts.length; | ||
| : mergedAccounts.length - currentAccounts.length; | ||
| const skipped = wouldExceedLimit | ||
| ? null | ||
| : Math.max(0, incomingDeduplicatedAccounts.length - (imported ?? 0)); | ||
| const changed = !haveEquivalentAccountRows( | ||
| mergedAccounts, | ||
| currentDeduplicatedAccounts, | ||
| ); | ||
| : Math.max(0, candidate.normalized.accounts.length - (imported ?? 0)); |
lib/storage.ts
Outdated
| wouldExceedLimit, | ||
| eligibleForRestore: !wouldExceedLimit && changed, | ||
| eligibleForRestore: !wouldExceedLimit, |
lib/codex-manager.ts
Outdated
| if (timestamp === null || timestamp === undefined || timestamp === 0) | ||
| return null; | ||
| if (!Number.isFinite(timestamp)) return null; | ||
| if (timestamp === null || timestamp === undefined) return null; |
|
|
||
| const RETRYABLE_REMOVE_CODES = new Set([ | ||
| "EBUSY", | ||
| "EPERM", | ||
| "ENOTEMPTY", | ||
| "EACCES", | ||
| "ETIMEDOUT", | ||
| ]); | ||
|
|
||
| async function removeWithRetry( | ||
| targetPath: string, | ||
| options: { recursive?: boolean; force?: boolean }, | ||
| ): Promise<void> { | ||
| for (let attempt = 0; attempt < 6; attempt += 1) { | ||
| try { | ||
| await fs.rm(targetPath, options); | ||
| return; | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code === "ENOENT") { | ||
| return; | ||
| } | ||
| if (!code || !RETRYABLE_REMOVE_CODES.has(code) || attempt === 5) { | ||
| throw error; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 25 * 2 ** attempt)); | ||
| } | ||
| } | ||
| } | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/storage.test.ts (1)
1418-1442:⚠️ Potential issue | 🟠 Majoradd the missing win32 retry regression for backup directory scans.
test/storage.test.ts:1418proves the error surfaces, andtest/storage.test.ts:1431proves the linux fast-fail path, but there is still no companionwin32case showing thatlistNamedBackups()retries transientepermdirectory errors before giving up. that leaves the windows-only branch unpinned by this suite.As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 1418 - 1442, Add a Windows-specific regression that pins the win32 retry behavior of listNamedBackups(): mock process.platform (getter) to return "win32", spy on fs.readdir (readdirSpy) and have it fail with an EPERM NodeJS.ErrnoException for the first N attempts then succeed, call listNamedBackups() and assert readdirSpy was called more than once and the function eventually returns the expected result; also add a companion case where readdir keeps rejecting EPERM and assert listNamedBackups rejects after the configured retry limit and that readdirSpy was called the expected number of times; remember to restore the spies/mocks after each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/upgrade.md`:
- Around line 65-69: The upgrade guide currently documents only the "Startup
Recovery Prompt" but misses the new sync-center workflow referenced from the
settings doc; update the upgrade notes to either add a "Sync Center" section
describing the new preview/apply/status commands and their validation behavior
(including how validation errors are surfaced and any new npm scripts
introduced) or remove the cross-reference from the settings doc so it no longer
points here; search for the "Startup Recovery Prompt" heading and the
"sync-center" term to find where to insert or remove the content and ensure the
docs/upgrade notes match the actual CLI flags and workflows.
In `@lib/codex-cli/sync.ts`:
- Around line 26-27: Add "EAGAIN" to the RETRYABLE_SELECTION_TIMESTAMP_CODES set
so getPersistedLocalSelectionTimestamp() will retry on transient EAGAIN errors
(in addition to "EBUSY" and "EPERM"), and update or add a vitest that simulates
fs.stat() returning EAGAIN then succeeding to assert the function retries and
that shouldApplyCodexCliSelection() receives the correct timestamp; ensure
SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS logic remains honored and the test covers
the retry path analogous to the storage layer handling already in
lib/storage.ts.
In `@lib/codex-manager.ts`:
- Around line 4357-4419: The code currently swallows real IO/listing errors in
loadBackupRestoreManagerAssessments (when calling listNamedBackups) by logging
the raw error and returning an empty array which causes runBackupRestoreManager
to treat a failure as "no named backups"; instead, preserve failure semantics
and redact sensitive details: change loadBackupRestoreManagerAssessments to
throw or return a distinct error result when listNamedBackups fails (do not
convert to []), and update runBackupRestoreManager to handle that error path
(e.g., surface a failure result rather than "dismissed"); also replace raw
error.message logging in loadBackupRestoreManagerAssessments and the
Promise.allSettled failure branch (assessNamedBackupRestore) with a consistent
redaction helper (e.g., collapseWhitespaceRedacted) that strips tokens/emails
before logging the brief "failed to read backup directory" / "skipped backup
assessment" messages, and reference functions listNamedBackups,
assessNamedBackupRestore, loadBackupRestoreManagerAssessments, and
getNamedBackupsDirectoryPath when making the changes.
In `@lib/codex-manager/settings-hub.ts`:
- Around line 2681-2701: buildPreview() currently calls loadAccounts(), which
can persist migration/recovery paths and thus mutates storage during preview;
replace that call with a non-persisting account loader (e.g., a new or existing
loadAccountsNonPersisting / loadAccounts({ persist: false }) variant) when
constructing preview so no writes occur, and keep the rest of the flow
(sourceState via loadCodexCliState, previewCodexCliSync,
getStorageBackupEnabled, resolveSyncCenterContext) unchanged; add a Vitest
regression test that exercises the migration/recovery fixtures and asserts no
storage writes occur during buildPreview()/previewCodexCliSync previewing to
prevent future regressions.
- Around line 2846-2861: The current flow using withQueuedRetry plus
loadAccounts -> applyCodexCliSyncToStorage -> saveAccounts is a racy
read-modify-write because reconcile and persist happen outside the storage
transaction; fix it by moving the reconcile (applyCodexCliSyncToStorage), the
persist (saveAccounts with backupEnabled from getStorageBackupEnabled), and the
pending-run commit into a single storage transaction using the storage module’s
transaction API referenced in lib/storage (wrap the logic currently in the
withQueuedRetry callback so loadAccounts, the reconcile result assignment to
syncedStorage, saveAccounts, and the pending-run commit all execute inside that
one transaction), keep using preview.targetPath for queueing as needed, and add
a vitest that interleaves a competing save to assert the transaction prevents
the overwrite race.
In `@lib/live-account-sync.ts`:
- Around line 39-58: The current global snapshot selection in
refreshLastLiveAccountSyncSnapshot (and the similar logic around lines 233-250)
incorrectly picks the highest instanceId instead of the latest published
snapshot; change both places to rank by a monotonic publishRevision or
publishedAt timestamp stored on LiveAccountSyncSnapshot (use publishedAt if
present or attach a numeric publishRevision when publishing), pick the snapshot
with the greatest revision/timestamp, and fall back to
lastStoppedLiveAccountSyncSnapshot similarly by its published timestamp; add a
vitest that creates multiple LiveAccountSync instances and stops them in reverse
creation order to assert the global snapshot follows publish order, and update
any affected tests to reference the new behavior; ensure any new queue or retry
logic you add for publishing accounts handles EBUSY and 429 by retrying/backoff
so concurrent filesystem/auth-rotation scenarios remain robust.
- Around line 165-179: The fsWatch handler in live-account-sync fires reloads
for transient sibling files (e.g., rotate-temp and .cache) causing spurious
reloads; update the callback in fsWatch (the closure using
normalizeFsWatchFilename, targetName, scheduleReload, and this.watcher) to
early-ignore filenames that match the same storage-noise rules used elsewhere
(extract/reuse the existing ignore predicate used by the storage layer—e.g., the
isKnownStorageNoise/isIgnoredStorageArtifact logic—or implement the same checks:
skip names like rotate-temp patterns and ".cache" and other artifacts) before
calling scheduleReload; add a vitest in storage-recovery-paths.test.ts that
simulates the rotate-temp path to assert no reload is queued for those transient
files, and ensure the reload queue logic still properly handles EBUSY/429
scenarios after this change.
In `@lib/storage.ts`:
- Around line 1992-1997: The import path bypasses the transient FS retry: modify
importAccounts to wrap its direct file read (the read performed inside
importAccounts that restoreNamedBackup calls) with
retryTransientFilesystemOperation so EPERM/EBUSY on Windows are retried; update
the restore/import flow (restoreNamedBackup -> importAccounts) to use that retry
wrapper and add a Vitest that simulates a one-shot locked-file read (file locked
once then released) to assert importAccounts succeeds after a single retry;
reference functions importAccounts, restoreNamedBackup, and
retryTransientFilesystemOperation and ensure the new test lives with other
vitest FS concurrency tests and documents handling of EBUSY/EPERM/429 scenarios.
In `@test/recovery.test.ts`:
- Around line 49-65: The retry helper removeWithRetry should widen the set of
transient Windows fs.rm errors to match other tests: update the error check in
removeWithRetry to treat "EACCES" and "ETIMEDOUT" (case-insensitive variants
like "eacces"/"etimedout") as retriable alongside "EBUSY", "EPERM", and
"ENOTEMPTY"; keep the same max attempts and backoff logic (the exponential delay
and final throw on attempt === 4) so teardowns won't flake on Windows when fs.rm
surfaces these additional error codes.
- Around line 647-656: The fs.stat mock in the test uses the raw path for
equality (path === lockedBackup?.path) while also comparing a normalizedPath to
"/locked-backup.json", causing the mock to miss Windows-style paths and drop the
ebusy injection; update the condition in the mockImplementation (the statSpy) to
compare normalized forms consistently: compute normalizedLocked = typeof
lockedBackup?.path === "string" ? lockedBackup.path.replaceAll("\\", "/") :
String(lockedBackup?.path) and then use either normalizedPath ===
normalizedLocked OR normalizedPath.endsWith("/locked-backup.json") (i.e.,
perform both comparisons against normalizedPath) so the ebusy branch is reliably
hit on Windows and POSIX paths.
---
Outside diff comments:
In `@test/storage.test.ts`:
- Around line 1418-1442: Add a Windows-specific regression that pins the win32
retry behavior of listNamedBackups(): mock process.platform (getter) to return
"win32", spy on fs.readdir (readdirSpy) and have it fail with an EPERM
NodeJS.ErrnoException for the first N attempts then succeed, call
listNamedBackups() and assert readdirSpy was called more than once and the
function eventually returns the expected result; also add a companion case where
readdir keeps rejecting EPERM and assert listNamedBackups rejects after the
configured retry limit and that readdirSpy was called the expected number of
times; remember to restore the spies/mocks after each test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cbf9d6a6-6fad-4f2e-865f-d9f246f0925d
📒 Files selected for processing (23)
docs/getting-started.mddocs/reference/public-api.mddocs/reference/settings.mddocs/troubleshooting.mddocs/upgrade.mdlib/accounts.tslib/cli.tslib/codex-cli/sync.tslib/codex-manager.tslib/codex-manager/settings-hub.tslib/live-account-sync.tslib/storage.tslib/ui/copy.tstest/accounts-edge.test.tstest/accounts-load-from-disk.test.tstest/accounts.test.tstest/codex-cli-sync.test.tstest/codex-manager-cli.test.tstest/live-account-sync.test.tstest/recovery.test.tstest/settings-hub-utils.test.tstest/storage-recovery-paths.test.tstest/storage.test.ts
💤 Files with no reviewable changes (1)
- docs/reference/public-api.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/getting-started.mddocs/reference/settings.mddocs/upgrade.mddocs/troubleshooting.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/accounts.test.tstest/accounts-load-from-disk.test.tstest/live-account-sync.test.tstest/recovery.test.tstest/storage-recovery-paths.test.tstest/settings-hub-utils.test.tstest/codex-cli-sync.test.tstest/storage.test.tstest/accounts-edge.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/cli.tslib/accounts.tslib/live-account-sync.tslib/codex-manager.tslib/codex-manager/settings-hub.tslib/codex-cli/sync.tslib/storage.tslib/ui/copy.ts
🔇 Additional comments (5)
test/storage-recovery-paths.test.ts (1)
738-743: good test hardening.
test/storage-recovery-paths.test.ts:738makes the ordering assertion independent of incidental write timing, which is a better fit for coarse windows mtime resolution.test/settings-hub-utils.test.ts (1)
375-416: strong regression coverage for windows labels and retryable sync-center loads.good coverage in
test/settings-hub-utils.test.ts:375-416andtest/settings-hub-utils.test.ts:498-618. you added deterministic vitest checks for windows-style source matching plus ebusy/429 retry behavior during preview loading.Also applies to: 498-618
lib/ui/copy.ts (1)
71-71: sync center copy additions look clean and complete.
lib/ui/copy.ts:71andlib/ui/copy.ts:115-123provide a full, consistent copy surface for the new sync-center settings flow.Also applies to: 115-123
test/codex-cli-sync.test.ts (1)
869-965: excellent deterministic coverage for windows fs retries and sync-run concurrency ordering.
test/codex-cli-sync.test.ts:869-965,test/codex-cli-sync.test.ts:1053-1267, andtest/codex-cli-sync.test.ts:1607-1841add strong vitest regression cases for ebusy/eperm/429 handling and concurrent pending-run publish behavior.Also applies to: 1053-1267, 1607-1841
lib/codex-manager.ts (1)
3827-3842: regression tests for startup recovery already exist—no action needed.startup recovery routing tests are in place at test/recovery.test.ts:783-815 (unit tests for
resolveStartupRecoveryAction), and integration tests cover the full flow including ebusy scenarios at test/codex-manager-cli.test.ts:2483 and :2517. exit code mapping for restore-backup is tested at test/codex-manager-cli.test.ts line 926. tests verify state transitions (restored/dismissed/failed), error redaction withgetRedactedFilesystemErrorLabel, and windows fs error fallbacks (ebusy). coverage aligns with coding guidelines requiring vitest verification of lib/** changes.> Likely an incorrect or invalid review comment.
| const originalStat = fs.stat.bind(fs); | ||
| const statSpy = vi.spyOn(fs, "stat").mockImplementation( | ||
| (async (...args: Parameters<typeof fs.stat>) => { | ||
| const [path] = args; | ||
| const normalizedPath = | ||
| typeof path === "string" ? path.replaceAll("\\", "/") : String(path); | ||
| if ( | ||
| path === lockedBackup?.path || | ||
| normalizedPath.endsWith("/locked-backup.json") | ||
| ) { |
There was a problem hiding this comment.
fix the windows path matcher in this ebusy regression.
test/recovery.test.ts:642 creates first-backup and second-backup, but test/recovery.test.ts:655 still checks for /locked-backup.json. if fs.stat() reports a normalized path instead of the original string, this mock stops injecting the ebusy and the test no longer exercises the branch it claims to cover.
suggested fix
if (
path === lockedBackup?.path ||
- normalizedPath.endsWith("/locked-backup.json")
+ normalizedPath.endsWith("/first-backup.json")
) {As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const originalStat = fs.stat.bind(fs); | |
| const statSpy = vi.spyOn(fs, "stat").mockImplementation( | |
| (async (...args: Parameters<typeof fs.stat>) => { | |
| const [path] = args; | |
| const normalizedPath = | |
| typeof path === "string" ? path.replaceAll("\\", "/") : String(path); | |
| if ( | |
| path === lockedBackup?.path || | |
| normalizedPath.endsWith("/locked-backup.json") | |
| ) { | |
| const originalStat = fs.stat.bind(fs); | |
| const statSpy = vi.spyOn(fs, "stat").mockImplementation( | |
| (async (...args: Parameters<typeof fs.stat>) => { | |
| const [path] = args; | |
| const normalizedPath = | |
| typeof path === "string" ? path.replaceAll("\\", "/") : String(path); | |
| if ( | |
| path === lockedBackup?.path || | |
| normalizedPath.endsWith("/first-backup.json") | |
| ) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/recovery.test.ts` around lines 647 - 656, The fs.stat mock in the test
uses the raw path for equality (path === lockedBackup?.path) while also
comparing a normalizedPath to "/locked-backup.json", causing the mock to miss
Windows-style paths and drop the ebusy injection; update the condition in the
mockImplementation (the statSpy) to compare normalized forms consistently:
compute normalizedLocked = typeof lockedBackup?.path === "string" ?
lockedBackup.path.replaceAll("\\", "/") : String(lockedBackup?.path) and then
use either normalizedPath === normalizedLocked OR
normalizedPath.endsWith("/locked-backup.json") (i.e., perform both comparisons
against normalizedPath) so the ebusy branch is reliably hit on Windows and POSIX
paths.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/storage.ts (1)
3050-3061: 🧹 Nitpick | 🔵 Trivialwindows path comparison fix is correct but the nested ternary is hard to follow.
lib/storage.ts:3050-3061correctly handles case-insensitive path comparison on windows. the nested ternary with IIFE makes review harder than necessary.consider extracting to a helper for readability:
optional refactor
+function isMatchingStoragePath(left: string | undefined, right: string | undefined): boolean { + if (!left || !right) return false; + return process.platform === "win32" + ? left.toLowerCase() === right.toLowerCase() + : left === right; +} + const storage = transactionState?.active - ? (process.platform === "win32" - ? transactionState.storagePath?.toLowerCase() === - currentStoragePath?.toLowerCase() - : transactionState.storagePath === currentStoragePath) + ? isMatchingStoragePath(transactionState.storagePath, currentStoragePath) ? transactionState.snapshot : (() => { throw new Error( "exportAccounts called inside an active transaction for a different storage path", ); })() : await withAccountStorageTransaction((current) => Promise.resolve(current));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 3050 - 3061, The nested ternary in the storage selection logic (variables: currentStoragePath, transactionState, storage, transactionState.snapshot) is hard to read; refactor by extracting the path-equality logic into a helper (e.g., isSameStoragePath(a,b) that does case-insensitive comparison on Windows and case-sensitive elsewhere) and then replace the ternary with a clear if/else: if transactionState?.active then if isSameStoragePath(transactionState.storagePath, currentStoragePath) return transactionState.snapshot else throw the existing Error, otherwise use the non-transaction path; keep the existing Error text and behavior.
♻️ Duplicate comments (4)
test/recovery.test.ts (2)
49-63:⚠️ Potential issue | 🟠 Majorexpand windows retry codes in the teardown helper.
at Line [57] in
test/recovery.test.ts:57, retries only cover EBUSY/EPERM/ENOTEMPTY. windows cleanup can also return EACCES/ETIMEDOUT, so teardown can still flake and break determinism.As per coding guidelines, `test/**`: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.proposed fix
async function removeWithRetry(targetPath: string): Promise<void> { for (let attempt = 0; attempt < 5; attempt += 1) { try { await fs.rm(targetPath, { recursive: true, force: true }); return; } catch (error) { - const code = (error as NodeJS.ErrnoException).code; + const code = ((error as NodeJS.ErrnoException).code ?? "").toUpperCase(); if ( - (code !== "EBUSY" && code !== "EPERM" && code !== "ENOTEMPTY") || + (code !== "EBUSY" && + code !== "EPERM" && + code !== "ENOTEMPTY" && + code !== "EACCES" && + code !== "ETIMEDOUT") || attempt === 4 ) { throw error; } await new Promise((resolve) => setTimeout(resolve, 25 * 2 ** attempt)); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/recovery.test.ts` around lines 49 - 63, The retry helper removeWithRetry currently treats only EBUSY, EPERM, and ENOTEMPTY as transient; expand the transient-error check in removeWithRetry to also include Windows-specific codes like EACCES and ETIMEDOUT (i.e., treat "EACCES" and "ETIMEDOUT" as retryable), preserving the existing max attempts/backoff logic and throwing for other codes or after the final attempt so tests remain deterministic; locate the check that reads (error as NodeJS.ErrnoException).code in removeWithRetry and add those string codes to the allowed-retry condition.
647-656:⚠️ Potential issue | 🟠 Majorfix the windows path matcher in the metadata-stat EBUSY regression.
at Line [655] in
test/recovery.test.ts:655, the suffix still targets/locked-backup.jsoneven though this case createsfirst-backup/second-backup. at Line [654] intest/recovery.test.ts:654, raw-vs-normalized comparison can also miss windows-style paths.As per coding guidelines, `test/**`: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.proposed fix
const statSpy = vi.spyOn(fs, "stat").mockImplementation( (async (...args: Parameters<typeof fs.stat>) => { const [path] = args; const normalizedPath = typeof path === "string" ? path.replaceAll("\\", "/") : String(path); + const normalizedLockedPath = + typeof lockedBackup?.path === "string" + ? lockedBackup.path.replaceAll("\\", "/") + : String(lockedBackup?.path); if ( - path === lockedBackup?.path || - normalizedPath.endsWith("/locked-backup.json") + normalizedPath === normalizedLockedPath || + normalizedPath.endsWith("/first-backup.json") ) { const error = new Error("resource busy") as NodeJS.ErrnoException; error.code = "EBUSY"; throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/recovery.test.ts` around lines 647 - 656, The stat mock in the test (around originalStat, statSpy mocking fs.stat) incorrectly compares raw Windows paths and checks for a hardcoded "/locked-backup.json" suffix; normalize both the incoming path and lockedBackup?.path by replacing backslashes with forward slashes before comparing, and change the suffix check to normalizedPath.endsWith("locked-backup.json") (no leading slash) or compare full normalized paths, so the mock correctly matches both "first-backup" / "second-backup" cases and Windows-style paths.lib/codex-cli/sync.ts (1)
26-27:⚠️ Potential issue | 🟠 Majorretry
eagainwhen reading the local selection timestamp.
getPersistedLocalSelectionTimestamp()atlib/codex-cli/sync.ts:365-395still treatsEAGAINas terminal becauseRETRYABLE_SELECTION_TIMESTAMP_CODESatlib/codex-cli/sync.ts:26-27only includesEBUSYandEPERM. on windows file-lock contention,fs.stat()can transiently returnEAGAIN, which feedsnullintoshouldApplyCodexCliSelection()atlib/codex-cli/sync.ts:474-505and can preserve a stale local selection. please addEAGAINhere and cover it with a vitest intest/codex-cli-sync.test.ts. As per coding guidelines,lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.suggested fix
-const RETRYABLE_SELECTION_TIMESTAMP_CODES = new Set(["EBUSY", "EPERM"]); +const RETRYABLE_SELECTION_TIMESTAMP_CODES = new Set([ + "EBUSY", + "EPERM", + "EAGAIN", +]);Also applies to: 365-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-cli/sync.ts` around lines 26 - 27, Add "EAGAIN" to the RETRYABLE_SELECTION_TIMESTAMP_CODES set and add a vitest in test/codex-cli-sync.test.ts that stubs fs.stat to transiently throw EAGAIN for a few attempts then succeed to verify getPersistedLocalSelectionTimestamp (which uses SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS) retries and returns the correct timestamp, and that shouldApplyCodexCliSelection behaves correctly when transient nulls occur; ensure the test covers Windows-like EAGAIN behavior, references the RETRYABLE_SELECTION_TIMESTAMP_CODES symbol, and that no logs in the path leak tokens or emails.lib/codex-manager.ts (1)
4375-4385:⚠️ Potential issue | 🟠 Majordo not turn backup directory failures into an empty restore list.
the catch at
lib/codex-manager.ts:4375-4385still convertslistNamedBackups()failures into[], sorunBackupRestoreManager()falls into the benign "no named backups found" path atlib/codex-manager.ts:4435-4440. the warning path atlib/codex-manager.ts:4408-4416also still logs raw error text instead ofgetRedactedFilesystemErrorLabel(). this hides real windows/filesystem failures from the user and weakens the redaction work added elsewhere in this file. please propagate a failed result here and add a vitest that makeslistNamedBackups()reject so the restore manager returns"failed"with redacted logging. As per coding guidelines,lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.suggested fix
try { backups = await listNamedBackups(); } catch (error) { - const message = error instanceof Error ? error.message : String(error); - console.error( - `Could not read backup directory: ${ - collapseWhitespace(message) || "unknown error" - }`, - ); - return []; + const errorLabel = getRedactedFilesystemErrorLabel(error); + console.error(`Could not read backup directory (${errorLabel}).`); + throw error; } @@ - const reason = - result.reason instanceof Error - ? result.reason.message - : String(result.reason); + const errorLabel = getRedactedFilesystemErrorLabel(result.reason); console.warn( - `Skipped backup assessment for "${backupName}": ${ - collapseWhitespace(reason) || "unknown error" - }`, + `Skipped backup assessment for "${backupName}" (${errorLabel}).`, ); } } @@ - const assessments = - assessmentsOverride ?? (await loadBackupRestoreManagerAssessments()); + let assessments: BackupRestoreAssessment[]; + try { + assessments = + assessmentsOverride ?? (await loadBackupRestoreManagerAssessments()); + } catch { + return "failed"; + }Also applies to: 4408-4416, 4435-4440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4375 - 4385, The catch block around listNamedBackups() should not swallow filesystem errors into an empty restore list—change the error handling in lib/codex-manager.ts so listNamedBackups() rejections propagate as a failure for runBackupRestoreManager() (return or throw a sentinel like "failed" from the restore manager rather than returning []), and replace raw console.error messages at the 4408-4416 logging site with getRedactedFilesystemErrorLabel(error) (or equivalent redaction helper) so the log uses the redacted filesystem error label; update runBackupRestoreManager() paths (currently handling no backups at 4435-4440) to detect and return "failed" when upstream listNamedBackups() failed. Add a vitest that mocks listNamedBackups() to reject and asserts runBackupRestoreManager() returns "failed" and that logs include the redacted label; ensure tests cover EBUSY/429 queue behavior and avoid leaking tokens/emails in logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-cli/sync.ts`:
- Around line 150-169: The publish path currently uses a newly allocated
revision when committing (in commitPendingCodexCliSyncRun and
commitCodexCliSyncRunFailure) which can reorder in-flight runs; instead, use the
original pending revision stored on the pendingRun (publish via
publishCodexCliSyncRun(run, pendingRun.revision)) and remove that revision from
activePendingCodexCliSyncRunRevisions when completing; update
commitPendingCodexCliSyncRun and commitCodexCliSyncRunFailure to stop calling
allocatePendingCodexCliSyncRunRevision and instead pass pendingRun.revision into
publishCodexCliSyncRun, and ensure publishCodexCliSyncRun compares and updates
lastCodexCliSyncRunRevision/lastCodexCliSyncRun as before; add a vitest in
test/codex-cli-sync.test.ts that enqueues two pending runs and commits them out
of order to assert the later revision wins, and ensure tests note EBUSY/429
queue handling and avoid leaking tokens/emails in logs.
In `@lib/codex-manager/settings-hub.ts`:
- Around line 13-21: Import commitCodexCliSyncRunFailure and ensure persist() is
wrapped in its own try/catch inside the withAccountStorageTransaction callback
so that next.pendingRun remains in scope; on persist failure call
commitCodexCliSyncRunFailure(next.pendingRun, error) to record the failed run
instead of exiting the loop. Also explicitly type the synced variable (e.g., let
synced: SyncResult | null) to satisfy noImplicitAny and replace any uses with a
null-safe check (synced?.changed) where appropriate. Finally add a regression
test that mocks withAccountStorageTransaction to make persist() throw a
retryable error and assert commitCodexCliSyncRunFailure was invoked and the run
recorded as failed.
In `@test/codex-cli-sync.test.ts`:
- Around line 1318-1365: The test currently asserts a hardcoded retry count
(expect(targetStatCalls).toBe(7)); change this to use the shared constant
SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS so the test stays in sync with retry
logic: replace the hardcoded 7 with SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS in the
assertion referencing targetStatCalls (and ensure
SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS is imported into the test file), keeping
the rest of the previewCodexCliSync and targetStatCalls setup unchanged.
---
Outside diff comments:
In `@lib/storage.ts`:
- Around line 3050-3061: The nested ternary in the storage selection logic
(variables: currentStoragePath, transactionState, storage,
transactionState.snapshot) is hard to read; refactor by extracting the
path-equality logic into a helper (e.g., isSameStoragePath(a,b) that does
case-insensitive comparison on Windows and case-sensitive elsewhere) and then
replace the ternary with a clear if/else: if transactionState?.active then if
isSameStoragePath(transactionState.storagePath, currentStoragePath) return
transactionState.snapshot else throw the existing Error, otherwise use the
non-transaction path; keep the existing Error text and behavior.
---
Duplicate comments:
In `@lib/codex-cli/sync.ts`:
- Around line 26-27: Add "EAGAIN" to the RETRYABLE_SELECTION_TIMESTAMP_CODES set
and add a vitest in test/codex-cli-sync.test.ts that stubs fs.stat to
transiently throw EAGAIN for a few attempts then succeed to verify
getPersistedLocalSelectionTimestamp (which uses
SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS) retries and returns the correct
timestamp, and that shouldApplyCodexCliSelection behaves correctly when
transient nulls occur; ensure the test covers Windows-like EAGAIN behavior,
references the RETRYABLE_SELECTION_TIMESTAMP_CODES symbol, and that no logs in
the path leak tokens or emails.
In `@lib/codex-manager.ts`:
- Around line 4375-4385: The catch block around listNamedBackups() should not
swallow filesystem errors into an empty restore list—change the error handling
in lib/codex-manager.ts so listNamedBackups() rejections propagate as a failure
for runBackupRestoreManager() (return or throw a sentinel like "failed" from the
restore manager rather than returning []), and replace raw console.error
messages at the 4408-4416 logging site with
getRedactedFilesystemErrorLabel(error) (or equivalent redaction helper) so the
log uses the redacted filesystem error label; update runBackupRestoreManager()
paths (currently handling no backups at 4435-4440) to detect and return "failed"
when upstream listNamedBackups() failed. Add a vitest that mocks
listNamedBackups() to reject and asserts runBackupRestoreManager() returns
"failed" and that logs include the redacted label; ensure tests cover EBUSY/429
queue behavior and avoid leaking tokens/emails in logs.
In `@test/recovery.test.ts`:
- Around line 49-63: The retry helper removeWithRetry currently treats only
EBUSY, EPERM, and ENOTEMPTY as transient; expand the transient-error check in
removeWithRetry to also include Windows-specific codes like EACCES and ETIMEDOUT
(i.e., treat "EACCES" and "ETIMEDOUT" as retryable), preserving the existing max
attempts/backoff logic and throwing for other codes or after the final attempt
so tests remain deterministic; locate the check that reads (error as
NodeJS.ErrnoException).code in removeWithRetry and add those string codes to the
allowed-retry condition.
- Around line 647-656: The stat mock in the test (around originalStat, statSpy
mocking fs.stat) incorrectly compares raw Windows paths and checks for a
hardcoded "/locked-backup.json" suffix; normalize both the incoming path and
lockedBackup?.path by replacing backslashes with forward slashes before
comparing, and change the suffix check to
normalizedPath.endsWith("locked-backup.json") (no leading slash) or compare full
normalized paths, so the mock correctly matches both "first-backup" /
"second-backup" cases and Windows-style paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b55ce4e-8917-42bd-8ebd-19f42e0491ba
📒 Files selected for processing (10)
docs/reference/settings.mdlib/codex-cli/sync.tslib/codex-manager.tslib/codex-manager/settings-hub.tslib/storage.tstest/codex-cli-sync.test.tstest/codex-manager-cli.test.tstest/recovery.test.tstest/settings-hub-utils.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Greptile Review
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/reference/settings.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/settings-hub-utils.test.tstest/recovery.test.tstest/codex-cli-sync.test.tstest/storage.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager/settings-hub.tslib/codex-manager.tslib/codex-cli/sync.tslib/storage.ts
🪛 Biome (2.4.7)
lib/codex-manager/settings-hub.ts
[error] 2848-2848: This variable implicitly has the any type.
(lint/suspicious/noImplicitAnyLet)
🔇 Additional comments (7)
docs/reference/settings.md (1)
67-80: sync center docs are aligned with the preview-first workflow.
docs/reference/settings.md:67todocs/reference/settings.md:80clearly describe preview-before-apply, destination preservation, and rollback artifacts, anddocs/reference/settings.md:70links upgrade notes.test/storage.test.ts (1)
1676-1701: good regression coverage for execution-time restore path validation.
test/storage.test.ts:1676now asserts thatrestoreAssessedNamedBackupre-validates the backup path at execution time, which closes a real toctou gap in restore flows.test/settings-hub-utils.test.ts (1)
498-622: strong retry-path coverage in sync-center preview loading.
test/settings-hub-utils.test.ts:498totest/settings-hub-utils.test.ts:622adds concrete regressions for retryable lock errors and 429 handling, which improves reliability of the sync-center flow.lib/storage.ts (4)
2088-2096:loadAccountsFromPathlacks retry wrapper for transient filesystem errors.
lib/storage.ts:2093performs a directfs.readFilewithoutretryTransientFilesystemOperation. this function is called fromdescribeAccountSnapshot(line 707),loadAccountsInternal(line 2266), and backup recovery loops (lines 2303, 2375).while
loadBackupCandidatewraps calls in the retry helper andloadAccountsInternalhas WAL recovery as a fallback, the inconsistency means transientEBUSY/EPERMon windows could cause spurious failures during metadata inspection or primary storage loads before fallback kicks in.consider wrapping the read for consistency with the rest of the module:
suggested fix
async function loadAccountsFromPath(path: string): Promise<{ normalized: AccountStorageV3 | null; storedVersion: unknown; schemaErrors: string[]; }> { - const content = await fs.readFile(path, "utf-8"); + const content = await retryTransientFilesystemOperation(() => + fs.readFile(path, "utf-8"), + ); const data = JSON.parse(content) as unknown; return parseAndNormalizeStorage(data); }as per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.[suggest_recommended_refactor, request_verification]
#!/bin/bash # verify which callers of loadAccountsFromPath are not already protected by retry wrappers or fallback logic ast-grep --pattern 'loadAccountsFromPath($_)'
1790-1793:listNamedBackupssignature simplification looks good.
lib/storage.ts:1790-1793removes the options parameter, and the context snippet atlib/codex-manager.ts:4388-4498confirms production callers invoke it with no arguments. the function now consistently returns full metadata by scanning and loading candidates.
3096-3110: past review addressed:importAccountsnow uses retry wrapper.
lib/storage.ts:3103-3105wrapsfs.readFileinretryTransientFilesystemOperation, addressing the previously flagged gap where restore/import could fail on transient windows filesystem errors.
1833-1931: tests already cover all three scenarios - no action needed.
test/recovery.test.tsincludes comprehensive coverage forgetActionableNamedBackupRestores:
- fast path (no options):
test/recovery.test.ts:371("scans named backups by default and returns actionable restores")- custom assess injection:
test/recovery.test.ts:194("accepts injected backups and assessor")- error handling:
test/recovery.test.ts:315("keeps actionable backups when another assessment throws") andtest/recovery.test.ts:438(ebusy on fast path)the function's two code paths and error recovery are fully tested.
lib/codex-manager.ts
Outdated
| try { | ||
| const result = await restoreAssessedNamedBackup(latestAssessment); | ||
| if (!result.changed) { | ||
| const result = await restoreNamedBackup(latestAssessment.backup.name); |
There was a problem hiding this comment.
latestAssessment was already loaded and validated on line 4488. calling restoreNamedBackup(latestAssessment.backup.name) discards that work and calls assessNamedBackupRestore a second time internally, re-reading the backup file from disk. on windows with antivirus, this opens a new EPERM/EBUSY window between the eligibility gate and the actual importAccounts call — the same concurrency concern the original restoreAssessedNamedBackup design was solving by accepting a pre-loaded assessment.
restoreAssessedNamedBackup still exists and accepts the pre-loaded assessment. use it directly:
| const result = await restoreNamedBackup(latestAssessment.backup.name); | |
| const result = await restoreAssessedNamedBackup(latestAssessment); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 4513
Comment:
**Redundant re-assessment adds an extra Windows file lock window**
`latestAssessment` was already loaded and validated on line 4488. calling `restoreNamedBackup(latestAssessment.backup.name)` discards that work and calls `assessNamedBackupRestore` a second time internally, re-reading the backup file from disk. on windows with antivirus, this opens a new `EPERM`/`EBUSY` window between the eligibility gate and the actual `importAccounts` call — the same concurrency concern the original `restoreAssessedNamedBackup` design was solving by accepting a pre-loaded assessment.
`restoreAssessedNamedBackup` still exists and accepts the pre-loaded assessment. use it directly:
```suggestion
const result = await restoreAssessedNamedBackup(latestAssessment);
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
lib/codex-cli/sync.ts (1)
26-27:⚠️ Potential issue | 🟠 Majoradd
eagainto retryable timestamp stat errors.
lib/codex-cli/sync.ts:26-27still omitsEAGAIN, so transient windows lock contention can bypass retry and degrade selection reconciliation behavior.proposed fix
-const RETRYABLE_SELECTION_TIMESTAMP_CODES = new Set(["EBUSY", "EPERM"]); +const RETRYABLE_SELECTION_TIMESTAMP_CODES = new Set([ + "EBUSY", + "EPERM", + "EAGAIN", +]);#!/bin/bash # verify retryable errno coverage + test coverage for timestamp-read retries rg -n -C2 'RETRYABLE_SELECTION_TIMESTAMP_CODES|getPersistedLocalSelectionTimestamp|EAGAIN|EIO|SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS'As per coding guidelines
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-cli/sync.ts` around lines 26 - 27, RETRYABLE_SELECTION_TIMESTAMP_CODES is missing the EAGAIN errno, causing transient Windows/posix lock contention to skip retries; update the constant RETRYABLE_SELECTION_TIMESTAMP_CODES to include "EAGAIN" (alongside "EBUSY" and "EPERM"), then adjust/add vitest tests for getPersistedLocalSelectionTimestamp to assert that reads retry up to SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS on EAGAIN and still succeed/fail appropriately; ensure any retry logic that references SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS remains unchanged and update test names/comments to note EAGAIN/EIO coverage and concurrency/Windows IO scenarios.lib/live-account-sync.ts (1)
180-182:⚠️ Potential issue | 🟠 Majorfilter storage-noise siblings before queueing reloads.
lib/live-account-sync.ts:180-182still queues reloads for anyopenai-codex-accounts.json.*sibling. this can include rotate-temp/cache artifacts and create pointless reload churn on windows during file rotation.proposed fix
+function isIgnoredSyncSiblingArtifact(targetName: string, name: string): boolean { + if (!name.startsWith(`${targetName}.`)) return false; + return name.endsWith(".cache") || name.includes(".rotate-temp."); +} @@ - if (name === targetName || name.startsWith(`${targetName}.`)) { + if (name === targetName) { + this.scheduleReload("watch"); + return; + } + if (isIgnoredSyncSiblingArtifact(targetName, name)) { + return; + } + if (name.startsWith(`${targetName}.`)) { this.scheduleReload("watch"); }add a vitest regression in
test/live-account-sync.test.tsthat emits rotate-temp/cache sibling events and asserts no reload is queued.As per coding guidelines
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/live-account-sync.ts` around lines 180 - 182, The watcher currently treats any sibling like "openai-codex-accounts.json.*" as a trigger because the condition in the event handler uses name === targetName || name.startsWith(`${targetName}.`); tighten this to ignore rotation/cache/temp sibling artifacts by changing the check in the handler that calls scheduleReload to only match the exact filename (name === targetName) or extensions/rotation suffixes you explicitly allow (e.g., numeric rotate suffixes or specific extensions), and do not trigger on transient suffixes like ".tmp", ".rotate-temp", or cache patterns; update the code referencing targetName and scheduleReload accordingly and add a vitest regression in test/live-account-sync.test.ts that emits sibling events (rotate-temp/cache patterns) and asserts that no reload is queued, and ensure new queue logic still handles EBUSY/429 edge cases per existing concurrency handling.lib/codex-manager/settings-hub.ts (1)
2849-2849:⚠️ Potential issue | 🟠 Majortype
syncedexplicitly to remove implicit-any in apply flow.in
lib/codex-manager/settings-hub.ts: Line 2849,let synced;is still implicit-any and trips the lint rule noted by static analysis. this also weakens type checks in the transaction/apply path.proposed patch
- let synced; + let synced: Awaited< + ReturnType<typeof applyCodexCliSyncToStorage> + > | null = null;#!/bin/bash sed -n '2846,2883p' lib/codex-manager/settings-hub.ts | cat -n🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager/settings-hub.ts` at line 2849, The local variable "synced" is declared without a type (let synced;) which triggers implicit-any; change it to an explicit type based on how it's used in the transaction/apply flow (for example let synced: boolean | undefined; or let synced: SyncResult | undefined;), i.e., inspect usages of synced within the apply/transaction code paths and replace the declaration with let synced: <correctType>; so TypeScript enforces proper typing and removes the implicit-any lint error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-cli/sync.ts`:
- Around line 381-386: The code currently returns null for non-retryable stat
errors (when code is not in RETRYABLE_SELECTION_TIMESTAMP_CODES), which masks IO
faults like EIO; update the error handling in the selection-timestamp stat/read
path (the block that checks typeof code and RETRYABLE_SELECTION_TIMESTAMP_CODES)
to throw or propagate a specific SyncError (or similar explicit error type) for
non-retryable IO errors instead of returning null, and adjust callers such as
shouldApplyCodexCliSelection and the reconciliation logic that assumes null
means "missing file" to handle that SyncError path (i.e., treat it as a hard
sync failure rather than falling back to in-process timestamps); also add tests
in test/codex-cli-sync.test.ts that simulate EIO and other non-retryable errors
to assert the new explicit error behavior.
In `@lib/codex-manager.ts`:
- Around line 4127-4130: The code currently treats any non-"restored"
restoreResult the same and resets recoveryPromptAttempted, which re-arms the
startup prompt even when the user dismissed it; change the logic in the
restore-handling block (look for restoreResult, pendingRecoveryState,
recoveryPromptAttempted in lib/codex-manager.ts) so that recoveryPromptAttempted
is only reset when restoreResult === "failed" (not when restoreResult ===
"dismissed"), and add a regression test in test/recovery.test.ts (or the
login-flow test file) that simulates a restore returning "dismissed" and asserts
that the prompt is not immediately re-armed (e.g., recoveryPromptAttempted
remains set/does not flip to false or prompt count does not increase), ensuring
one dismissal does not trigger an immediate re-prompt loop.
In `@lib/codex-manager/settings-hub.ts`:
- Around line 1283-1326: Add focused vitest cases in
test/settings-hub-utils.test.ts that exercise formatSyncRunTime,
formatSyncTimestamp, formatSyncMtime (and formatSyncRunOutcome for completeness)
against the missing/edge inputs called out in the review: assert
formatSyncRunTime(null) returns "No sync applied in this session." and that a
valid run.runAt produces an ISO string with the "T" replaced by a space; assert
formatSyncTimestamp returns "none" for undefined, null, NaN, Infinity, zero, and
negative values and returns a properly formatted ISO string for a positive
finite number; assert formatSyncMtime returns "unknown" for null, NaN, Infinity,
zero, and negative values and that a positive mtime is rounded and formatted
(check the "T"→" " replacement); and assert formatSyncRunOutcome handles null,
"changed","noop","disabled","unavailable" and error-message cases. Use vitest
describe/it blocks and explicit expectations for each edge input to prevent
silent regressions.
- Around line 1357-1367: normalizePathForComparison currently collapses repeated
slashes which turns UNC paths like //server/share into /server/share and
misclassifies Windows sources; update normalizePathForComparison to preserve
leading double-slash UNC prefixes while still normalizing backslashes,
collapsing internal duplicate separators, trimming trailing slashes (but not
removing the UNC leading //), and lowercasing only when a drive-letter path is
detected; add vitest regression tests in test/settings-hub-utils.test.ts
covering drive-letter (C:\path), UNC (//server/share and \\server\share), mixed
separators, and trailing-slash variants to assert identity-preserving behavior.
In `@test/codex-cli-sync.test.ts`:
- Around line 869-1051: Add a new deterministic vitest case for the
non-retryable "EIO" stat failure mirroring the existing tests that exercise
previewCodexCliSync and applyCodexCliSyncToStorage: create two additional tests
(one for previewCodexCliSync and one for applyCodexCliSyncToStorage) that mock
nodeFs.promises.stat to throw an ErrnoException with code "EIO" when args[0] ===
targetStoragePath, then assert the local selection remains unchanged
(preview.status "noop" / result.changed false and
activeIndex/activeIndexByFamily unchanged) and that the stat call count is 1
(i.e. not retried) instead of SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS; reuse the
same setup for current/sourceState, spy on storageModule.getStoragePath,
writerModule.getLastCodexCliSelectionWriteTimestamp,
storageModule.getLastAccountsSaveTimestamp, and restore the stat spy in finally.
---
Duplicate comments:
In `@lib/codex-cli/sync.ts`:
- Around line 26-27: RETRYABLE_SELECTION_TIMESTAMP_CODES is missing the EAGAIN
errno, causing transient Windows/posix lock contention to skip retries; update
the constant RETRYABLE_SELECTION_TIMESTAMP_CODES to include "EAGAIN" (alongside
"EBUSY" and "EPERM"), then adjust/add vitest tests for
getPersistedLocalSelectionTimestamp to assert that reads retry up to
SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS on EAGAIN and still succeed/fail
appropriately; ensure any retry logic that references
SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS remains unchanged and update test
names/comments to note EAGAIN/EIO coverage and concurrency/Windows IO scenarios.
In `@lib/codex-manager/settings-hub.ts`:
- Line 2849: The local variable "synced" is declared without a type (let
synced;) which triggers implicit-any; change it to an explicit type based on how
it's used in the transaction/apply flow (for example let synced: boolean |
undefined; or let synced: SyncResult | undefined;), i.e., inspect usages of
synced within the apply/transaction code paths and replace the declaration with
let synced: <correctType>; so TypeScript enforces proper typing and removes the
implicit-any lint error.
In `@lib/live-account-sync.ts`:
- Around line 180-182: The watcher currently treats any sibling like
"openai-codex-accounts.json.*" as a trigger because the condition in the event
handler uses name === targetName || name.startsWith(`${targetName}.`); tighten
this to ignore rotation/cache/temp sibling artifacts by changing the check in
the handler that calls scheduleReload to only match the exact filename (name ===
targetName) or extensions/rotation suffixes you explicitly allow (e.g., numeric
rotate suffixes or specific extensions), and do not trigger on transient
suffixes like ".tmp", ".rotate-temp", or cache patterns; update the code
referencing targetName and scheduleReload accordingly and add a vitest
regression in test/live-account-sync.test.ts that emits sibling events
(rotate-temp/cache patterns) and asserts that no reload is queued, and ensure
new queue logic still handles EBUSY/429 edge cases per existing concurrency
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17825736-41c9-4b74-8189-1ee74e856f25
📒 Files selected for processing (9)
docs/upgrade.mdlib/codex-cli/sync.tslib/codex-manager.tslib/codex-manager/settings-hub.tslib/live-account-sync.tstest/codex-cli-sync.test.tstest/codex-manager-cli.test.tstest/live-account-sync.test.tstest/recovery.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/upgrade.md
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager/settings-hub.tslib/live-account-sync.tslib/codex-cli/sync.tslib/codex-manager.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/recovery.test.tstest/codex-cli-sync.test.tstest/live-account-sync.test.ts
🪛 Biome (2.4.7)
lib/codex-manager/settings-hub.ts
[error] 2849-2849: This variable implicitly has the any type.
(lint/suspicious/noImplicitAnyLet)
🔇 Additional comments (5)
docs/upgrade.md (1)
65-77: upgrade note coverage looks good for this change set.
docs/upgrade.md:65-77now documents both startup recovery prompt behavior and sync-center preview/apply semantics in the upgrade guide. this closes the workflow gap for this pr.test/recovery.test.ts (1)
49-71: good deterministic windows-fs regression coverage.
test/recovery.test.ts:49-71andtest/recovery.test.ts:336-787tighten retry + ebusy path coverage and keep the suite deterministic under windows-like io contention.Also applies to: 336-787
lib/codex-manager/settings-hub.ts (1)
2689-2697: good fix: preview is now truly read-only.in
lib/codex-manager/settings-hub.ts: Line 2689, switching preview loading toloadAccountsReadOnly()removes storage write side effects during sync-center preview and aligns with the read-only contract.test/codex-cli-sync.test.ts (1)
1324-1365: nice: retry assertions are now tied to the shared constant.in
test/codex-cli-sync.test.ts: Line 1324-1365, usingSELECTION_TIMESTAMP_READ_MAX_ATTEMPTSinstead of a hardcoded count keeps the test aligned with sync retry behavior changes.test/live-account-sync.test.ts (1)
336-639: strong concurrency regression coverage here.
test/live-account-sync.test.ts: Line 336-639now directly exercises in-flight reload queuing, stale completion dropping after path switches, and stop/switch races with deterministic deferred controls. this is the right shape of vitest coverage for the new watcher-generation behavior inlib/live-account-sync.ts.
lib/codex-manager.ts
Outdated
| if (restoreResult !== "restored") { | ||
| pendingRecoveryState = recoveryState; | ||
| recoveryPromptAttempted = false; | ||
| } |
There was a problem hiding this comment.
dismissed restore re-arms startup prompt and can loop.
in lib/codex-manager.ts:4127-4130, restoreResult !== "restored" resets recoveryPromptAttempted for both "failed" and "dismissed". the dismissed path should not re-arm the same prompt immediately; this can loop prompting in the same session.
proposed fix
- if (restoreResult !== "restored") {
+ if (restoreResult === "failed") {
pendingRecoveryState = recoveryState;
recoveryPromptAttempted = false;
}please add a regression around this in test/recovery.test.ts (or the login-flow test file) that proves one dismissal does not trigger an immediate re-prompt loop.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (restoreResult !== "restored") { | |
| pendingRecoveryState = recoveryState; | |
| recoveryPromptAttempted = false; | |
| } | |
| if (restoreResult === "failed") { | |
| pendingRecoveryState = recoveryState; | |
| recoveryPromptAttempted = false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 4127 - 4130, The code currently treats any
non-"restored" restoreResult the same and resets recoveryPromptAttempted, which
re-arms the startup prompt even when the user dismissed it; change the logic in
the restore-handling block (look for restoreResult, pendingRecoveryState,
recoveryPromptAttempted in lib/codex-manager.ts) so that recoveryPromptAttempted
is only reset when restoreResult === "failed" (not when restoreResult ===
"dismissed"), and add a regression test in test/recovery.test.ts (or the
login-flow test file) that simulates a restore returning "dismissed" and asserts
that the prompt is not immediately re-armed (e.g., recoveryPromptAttempted
remains set/does not flip to false or prompt count does not increase), ensuring
one dismissal does not trigger an immediate re-prompt loop.
| function formatSyncRunTime(run: CodexCliSyncRun | null): string { | ||
| if (!run) return "No sync applied in this session."; | ||
| return new Date(run.runAt).toISOString().replace("T", " "); | ||
| } | ||
|
|
||
| function formatSyncRunOutcome(run: CodexCliSyncRun | null): string { | ||
| if (!run) return "none"; | ||
| if (run.outcome === "changed") return "applied changes"; | ||
| if (run.outcome === "noop") return "already aligned"; | ||
| if (run.outcome === "disabled") return "disabled"; | ||
| if (run.outcome === "unavailable") return "source missing"; | ||
| return run.message ? `error: ${run.message}` : "error"; | ||
| } | ||
|
|
||
| function formatSyncSummary(summary: CodexCliSyncSummary): string { | ||
| return [ | ||
| `add ${summary.addedAccountCount}`, | ||
| `update ${summary.updatedAccountCount}`, | ||
| `preserve ${summary.destinationOnlyPreservedCount}`, | ||
| `after ${summary.targetAccountCountAfter}`, | ||
| ].join(" | "); | ||
| } | ||
|
|
||
| function formatSyncTimestamp(timestamp: number | null | undefined): string { | ||
| if ( | ||
| typeof timestamp !== "number" || | ||
| !Number.isFinite(timestamp) || | ||
| timestamp <= 0 | ||
| ) { | ||
| return "none"; | ||
| } | ||
| return new Date(timestamp).toISOString().replace("T", " "); | ||
| } | ||
|
|
||
| function formatSyncMtime(mtimeMs: number | null): string { | ||
| if ( | ||
| typeof mtimeMs !== "number" || | ||
| !Number.isFinite(mtimeMs) || | ||
| mtimeMs <= 0 | ||
| ) { | ||
| return "unknown"; | ||
| } | ||
| return new Date(Math.round(mtimeMs)).toISOString().replace("T", " "); | ||
| } |
There was a problem hiding this comment.
add direct vitest coverage for new sync formatter edge inputs.
lib/codex-manager/settings-hub.ts: Line 1283-1326 adds formatSyncRunTime, formatSyncTimestamp, and formatSyncMtime, but this diff does not show direct assertions for null/undefined, non-finite, and non-positive timestamp paths. add focused vitest cases in test/settings-hub-utils.test.ts so ui status text doesn’t regress silently.
As per coding guidelines, lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager/settings-hub.ts` around lines 1283 - 1326, Add focused
vitest cases in test/settings-hub-utils.test.ts that exercise formatSyncRunTime,
formatSyncTimestamp, formatSyncMtime (and formatSyncRunOutcome for completeness)
against the missing/edge inputs called out in the review: assert
formatSyncRunTime(null) returns "No sync applied in this session." and that a
valid run.runAt produces an ISO string with the "T" replaced by a space; assert
formatSyncTimestamp returns "none" for undefined, null, NaN, Infinity, zero, and
negative values and returns a properly formatted ISO string for a positive
finite number; assert formatSyncMtime returns "unknown" for null, NaN, Infinity,
zero, and negative values and that a positive mtime is rounded and formatted
(check the "T"→" " replacement); and assert formatSyncRunOutcome handles null,
"changed","noop","disabled","unavailable" and error-message cases. Use vitest
describe/it blocks and explicit expectations for each edge input to prevent
silent regressions.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
lib/codex-manager.ts (1)
4127-4130:⚠️ Potential issue | 🟠 Majordismissed restore re-arms startup recovery and can loop prompts.
lib/codex-manager.ts:4127-4130resetsrecoveryPromptAttemptedwhen restore result is"dismissed", then immediatelycontinue loginFlow. withpendingRecoveryStateretained, this can re-open the same startup recovery prompt in a loop.proposed fix
- if (restoreResult === "dismissed") { + if (restoreResult === "failed") { pendingRecoveryState = recoveryState; recoveryPromptAttempted = false; }also add a regression in
test/recovery.test.tsproving one dismissal does not trigger an immediate re-prompt in the same session.As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4127 - 4130, The current logic in the loginFlow loop resets recoveryPromptAttempted to false when restoreResult === "dismissed", which combined with retaining pendingRecoveryState can re-open the same startup recovery prompt immediately; change the handling so that when restoreResult is "dismissed" you keep pendingRecoveryState but do NOT reset recoveryPromptAttempted (or explicitly set a flag like recoveryDismissedThisSession) to prevent re-arming the prompt in the same session, updating the branch in the block that checks restoreResult to only clear pendingRecoveryState for "recovered" or terminal outcomes and to preserve recoveryPromptAttempted for "dismissed"; update tests by adding a vitest in test/recovery.test.ts that simulates a dismissed restore and asserts no immediate re-prompt occurs in the same session, and mention the change in the test name so CI reviewers can spot the regression coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-cli/sync.ts`:
- Around line 26-30: The retry matrix is missing Windows-specific error codes
and conflicts with the EIO handling that returns null; update
RETRYABLE_SELECTION_TIMESTAMP_CODES to include "ENXIO" (and consider adding
"EIO" if tests expect retry behavior) and adjust the function that currently
treats EIO as non-retryable (the block that returns null for EIO) to instead
treat these codes as retryable by using RETRYABLE_SELECTION_TIMESTAMP_CODES;
ensure retry logic paths (both preview/apply) consult
RETRYABLE_SELECTION_TIMESTAMP_CODES so EBUSY/EPERM/EAGAIN/ENXIO (and EIO if
added) are retried, and update associated vitest expectations mentioned in the
review to cover ENXIO alongside existing matrices.
In `@lib/storage.ts`:
- Around line 1891-1894: getActionableNamedBackupRestores() currently calls
loadAccounts() when computing currentStorage (via the currentStorage variable),
which can perform writes/migrations and acquire locks; change that call to
loadAccountsReadOnly() so preview/assessment flows use the read-only loader and
avoid persistence side effects and extra lock contention. Locate the assignment
to currentStorage in getActionableNamedBackupRestores() and replace the
loadAccounts() invocation with loadAccountsReadOnly(), preserving the existing
undefined-check logic and behavior for options.currentStorage.
- Around line 1572-1574: The loadAccountsReadOnly function currently calls
loadAccountsInternal(null) but that path still triggers
cleanupStaleRotatingBackupArtifacts() and mutates the filesystem; change
loadAccountsInternal to accept an explicit readOnly boolean (or an options
object) and short-circuit or skip any destructive calls (specifically
cleanupStaleRotatingBackupArtifacts()) when readOnly is true, then update
loadAccountsReadOnly to call loadAccountsInternal(true) so callers get a truly
non-mutating read-only load; keep the cleanup logic unchanged elsewhere but
ensure other call sites pass false or the default so rotation cleanup still runs
where intended.
- Around line 1697-1703: scanNamedBackups and listNamedBackupsWithoutLoading
currently rely on readdir filtering to skip symlinks but then call
loadBackupCandidate later, leaving a TOCTOU window; fix by performing an
immediate lstat/readlink check (the same check used by
findExistingNamedBackupPath, e.g., !isSymbolicLink()) on the resolved path right
before calling loadBackupCandidate (and before returning any path in
listNamedBackupsWithoutLoading), and skip/ignore entries that are symlinks or
where lstat fails; update tests by adding a regression test in
test/storage.test.ts that simulates swapping a valid file for a symlink between
readdir and read (the "swap" scenario) to assert the code rejects the entry.
---
Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 4127-4130: The current logic in the loginFlow loop resets
recoveryPromptAttempted to false when restoreResult === "dismissed", which
combined with retaining pendingRecoveryState can re-open the same startup
recovery prompt immediately; change the handling so that when restoreResult is
"dismissed" you keep pendingRecoveryState but do NOT reset
recoveryPromptAttempted (or explicitly set a flag like
recoveryDismissedThisSession) to prevent re-arming the prompt in the same
session, updating the branch in the block that checks restoreResult to only
clear pendingRecoveryState for "recovered" or terminal outcomes and to preserve
recoveryPromptAttempted for "dismissed"; update tests by adding a vitest in
test/recovery.test.ts that simulates a dismissed restore and asserts no
immediate re-prompt occurs in the same session, and mention the change in the
test name so CI reviewers can spot the regression coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66174509-26e8-47b9-9c6c-bd27cc5c786b
📒 Files selected for processing (6)
lib/codex-cli/sync.tslib/codex-manager.tslib/storage.tstest/codex-cli-sync.test.tstest/codex-manager-cli.test.tstest/recovery.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/codex-cli/sync.tslib/storage.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-cli-sync.test.ts
🔇 Additional comments (2)
lib/storage.ts (2)
3134-3136: good windows fs hardening on import path.
lib/storage.ts:3134-3136now wrapsfs.readFilewithretryTransientFilesystemOperation(). this cleanly covers transient lock failures during restore/import on windows.As per coding guidelines,
lib/**: focus on auth rotation, windows filesystem IO, and concurrency.
1572-1574: add regression tests for win32 platform-specific lock handling and read-only semantics.test coverage exists for EBUSY in recovery.test.ts (lines 386, 513, 660, 755) and all-fail scan behavior (lines 845-875), but lacks:
- explicit
process.platform === "win32"stub to verifyEPERMretry behavior inretryTransientFilesystemOperation(lib/storage.ts:1821-1828) triggers only on windows.- integration test confirming
loadAccountsReadOnly(lib/storage.ts:1572-1574) enforces read-only semantics—no disk writes during scan/assessment.- scan-wide failure test combining windows lock errors across
scanNamedBackups→getActionableNamedBackupRestores→ assessment flow.the retry logic itself is sound (exponential backoff + platform check), but the test suite doesn't validate that win32-exclusive retry conditions actually fire. add vitest cases with
vi.stubGlobalmockingprocess.platformto "win32" and inject EPERM errors alongside existing EBUSY tests.also applies to: 1859-1957
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/codex-cli/sync.ts (1)
26-31:⚠️ Potential issue | 🟡 Minor
ENXIOstill missing from retry matrix.past review flagged that
ENXIOshould be added alongside the other retryable codes for windows unreadable-stat edge cases. EIO was added but ENXIO remains uncovered atlib/codex-cli/sync.ts:26-31. add it and cover the retry path with a vitest intest/codex-cli-sync.test.ts.proposed fix
const RETRYABLE_SELECTION_TIMESTAMP_CODES = new Set([ "EBUSY", "EPERM", "EAGAIN", "EIO", + "ENXIO", ]);,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-cli/sync.ts` around lines 26 - 31, The retryable error code set RETRYABLE_SELECTION_TIMESTAMP_CODES in lib/codex-cli/sync.ts is missing "ENXIO"; update that Set to include "ENXIO" alongside "EBUSY","EPERM","EAGAIN","EIO", and ensure the retry logic that checks RETRYABLE_SELECTION_TIMESTAMP_CODES still functions. Add a Vitest unit test in test/codex-cli-sync.test.ts that simulates a filesystem/stat failure returning ENXIO and asserts the code path retries (mock the function that reads selection timestamp or the fs/stat call and verify the retry branch is exercised). Keep test focused on the retry behavior and the Set membership used by the existing retry logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-cli/sync.ts`:
- Around line 504-506: The branch checking persistedLocalTimestamp is dead
because persistedLocalTimestamp is already guarded earlier; in the block where
localVersion <= 0 replace the redundant "return persistedLocalTimestamp !==
null;" with "return true;" (or remove the conditional and add a comment
explaining why we always return true) to simplify logic; update the code paths
around the localVersion check and the persistedLocalTimestamp guard to ensure no
other references depend on that null-check.
---
Duplicate comments:
In `@lib/codex-cli/sync.ts`:
- Around line 26-31: The retryable error code set
RETRYABLE_SELECTION_TIMESTAMP_CODES in lib/codex-cli/sync.ts is missing "ENXIO";
update that Set to include "ENXIO" alongside "EBUSY","EPERM","EAGAIN","EIO", and
ensure the retry logic that checks RETRYABLE_SELECTION_TIMESTAMP_CODES still
functions. Add a Vitest unit test in test/codex-cli-sync.test.ts that simulates
a filesystem/stat failure returning ENXIO and asserts the code path retries
(mock the function that reads selection timestamp or the fs/stat call and verify
the retry branch is exercised). Keep test focused on the retry behavior and the
Set membership used by the existing retry logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1bdfbcb-6985-4a6f-88fa-d6abaa968436
📒 Files selected for processing (1)
lib/codex-cli/sync.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (1)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-cli/sync.ts
🔇 Additional comments (6)
lib/codex-cli/sync.ts (6)
219-241: revision tracking now correctly uses the original pending revision.
commitPendingCodexCliSyncRun()andcommitCodexCliSyncRunFailure()atlib/codex-cli/sync.ts:219-241now publish withpendingRun.revisioninstead of allocating a fresh one. this addresses the out-of-order commit race flagged in past reviews.
370-402: retry logic for stat looks solid.
getPersistedLocalSelectionTimestamp()atlib/codex-cli/sync.ts:370-402implements exponential backoff with a max-attempts guard. the debug log at line 393-396 safely logs only error messages, no tokens or emails. the ENXIO gap is noted separately.
789-797: logging uses fingerprint correctly—no token or email leakage.the debug log at
lib/codex-cli/sync.ts:789-797callsmakeAccountFingerprint()foractiveAccountRef, avoiding exposure of raw emails or account ids. this aligns with the coding guidelines forlib/**.
833-875: sync fallback path handles canonical-missing and normalization correctly.
syncAccountStorageFromCodexCli()atlib/codex-cli/sync.ts:833-875properly increments metrics, logs only non-sensitive fields, and returnspendingRun: nullsince no external sync is applied here. the index normalization preserves local authority.
512-605: reconcileCodexCliState merges accounts and selection correctly.the reconciliation at
lib/codex-cli/sync.ts:512-605clones storage, upserts from snapshots, and applies codex selection when appropriate. debug logs at lines 575-591 contain only operation/outcome metadata—no sensitive data. the selection-ambiguous and local-newer branches are properly guarded.
288-330: upsert logic safely handles tokens without exposing them.
upsertFromSnapshot()atlib/codex-cli/sync.ts:288-330readsrefreshTokenfor matching and merge purposes but never logs it. theJSON.stringifycomparison at line 322 produces only a boolean result. conflict detection preserves data integrity.
| context.sourceAccountCount !== null | ||
| ? `Visible source accounts: ${context.sourceAccountCount}.` | ||
| : "No readable Codex CLI source is visible right now.", | ||
| ].join("\n"); | ||
| const selectionHint = preview.summary.selectionChanged | ||
| ? "When the Codex CLI source is newer, target selection follows activeAccountId first, then activeEmail or the active snapshot email. If local storage or a local Codex selection write is newer, the target keeps the local selection." | ||
| : "Selection precedence stays accountId first, then email, with newer local target state preserving its own active selection instead of being overwritten."; | ||
| return [ | ||
| { | ||
| label: `Status: ${preview.status}`, | ||
| hint: `${preview.statusDetail}\nLast sync: ${formatSyncRunOutcome(lastSync)} at ${formatSyncRunTime(lastSync)}`, | ||
| }, | ||
| { | ||
| label: `Target path: ${preview.targetPath}`, | ||
| hint: preview.sourcePath | ||
| ? `Source path: ${preview.sourcePath}` | ||
| : "Source path: not available", | ||
| }, | ||
| { | ||
| label: `Codex CLI source visibility: ${activeSourceLabel}`, | ||
| hint: sourceStateHint, | ||
| }, | ||
| { | ||
| label: `Live watcher: ${liveSyncLabel}`, | ||
| hint: liveSyncHint, | ||
| }, | ||
| { | ||
| label: "Preview mode: read-only until apply", | ||
| hint: "Refresh only re-reads the Codex CLI source and recomputes the one-way result. Apply writes the latest preview snapshot into the target path; refresh before apply if the Codex CLI files may have changed. It does not create a bidirectional merge.", | ||
| }, | ||
| { | ||
| label: `Preview summary: ${formatSyncSummary(preview.summary)}`, | ||
| hint: preview.summary.selectionChanged | ||
| ? "Active selection also updates to match the current Codex CLI source when that source is newer." |
There was a problem hiding this comment.
normalizePathForComparison only lowercases a path when isWindowsPath is true. the detection relies on path.includes("\\") or a drive-letter prefix. a UNC path provided with forward slashes and mixed case — e.g., //Server/Share/.codex/accounts.json from an env variable — will NOT be lowercased because neither condition fires.
if getCodexCliAccountsPath() returns //server/share/.codex/accounts.json (lowercase) but the Codex CLI state resolves //Server/Share/.codex/accounts.json (mixed case), the comparison fails and the sync center shows "custom source path active" instead of "accounts.json active". this is a UI-only mislead, not a data issue, but it erodes trust in the status surface on windows UNC shares.
add a UNC forward-slash detection branch:
const isWindowsPath =
path.includes("\\") ||
/^[a-z]:\//i.test(trimmed) ||
normalized.startsWith("//"); // treat all UNC as case-insensitive
return isWindowsPath ? trimmed.toLowerCase() : trimmed;Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager/settings-hub.ts
Line: 1397-1430
Comment:
**UNC forward-slash paths not lowercased — sync source label can mismatch**
`normalizePathForComparison` only lowercases a path when `isWindowsPath` is `true`. the detection relies on `path.includes("\\")` or a drive-letter prefix. a UNC path provided with forward slashes and mixed case — e.g., `//Server/Share/.codex/accounts.json` from an env variable — will NOT be lowercased because neither condition fires.
if `getCodexCliAccountsPath()` returns `//server/share/.codex/accounts.json` (lowercase) but the Codex CLI state resolves `//Server/Share/.codex/accounts.json` (mixed case), the comparison fails and the sync center shows "custom source path active" instead of "accounts.json active". this is a UI-only mislead, not a data issue, but it erodes trust in the status surface on windows UNC shares.
add a UNC forward-slash detection branch:
```typescript
const isWindowsPath =
path.includes("\\") ||
/^[a-z]:\//i.test(trimmed) ||
normalized.startsWith("//"); // treat all UNC as case-insensitive
return isWindowsPath ? trimmed.toLowerCase() : trimmed;
```
How can I resolve this? If you propose a fix, please make it concise.
Summary
#78fresh/02-backup-restore-managerWhat Changed
Validation
npm run lintnpm run typechecknpm testnpm test -- test/documentation.test.tsnpm run buildDocs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed)docs/features.mdupdated (if capability surface changed)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (if migration behavior changed)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
eeecf6eandccb06f6Additional Notes
fresh/02-backup-restore-manager; the startup-recovery cleanup it needs is carried here as an equivalent preserved commit, not by direct ancestry tofresh/03.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 reopens the archived sync-center slice from pr #78 and stacks it on
fresh/02-backup-restore-manager. it adds a preview-first sync center to the settings hub, a live-account-sync snapshot registry, a startup recovery prompt (restore-before-oauth), and the fullpreviewCodexCliSync/applyCodexCliSyncToStoragepipeline with pending-run revision tracking for safe async commit/failure reporting.lib/codex-manager/settings-hub.ts):promptSyncCenterloads aCodexCliSyncPreview, shows a read-only overview, and gates the apply button onstatus === "ready". apply uses the capturedsourceStatefrom the last refresh, passes it throughwithAccountStorageTransaction, and commits or fails the pending run correctly.formatSyncCenterStatusErrorconsistently usesgetRedactedFilesystemErrorLabel— token-path leakage addressed.lib/codex-cli/sync.ts):shouldApplyCodexCliSelectionnow returnsfalseonpersistedLocalTimestamp === null(unreadable local file defers to local, not epoch-0).RETRYABLE_SELECTION_TIMESTAMP_CODESnow includesEIOandENXIO. revision-orderedpublishCodexCliSyncRunprevents out-of-order updates from concurrent applies.lib/live-account-sync.ts): generation counter prevents stalesyncToPathcompletions from overwriting newer state.reloadQueueddo-while correctly drains queued reloads without holding the event loop. module-level snapshot registry correctly tracks the most recently published instance.lib/codex-manager.ts):resolveStartupRecoveryActioncorrectly gates oauth, empty-storage menu, and recovery prompt. confirm text now shows both visible and recoverable backup counts, addressing the previous count-mismatch concern.normalizePathForComparisontreats forward-slash UNC paths (//Server/Share/...) as case-sensitive (not lowercased) becausepath.includes("\\")is false and the drive-letter regex doesn't match. if both sides use forward-slash UNC with mixed case, the "accounts.json active" label won't fire and the user sees "custom source path active" instead.promptSyncCenteris unreachable becausebuildPreviewSafelyswallows all its own errors internally.resolveStartupRecoveryActionis exported but has no direct unit test; the partial-success case (assessments.length > 0 && recoveryScanFailed === true) is untested.lib/storage.ts, windows AV retry window cut from ~1.26 s to ~150 ms, skipped count over-reported when backup has internal duplicates, codex cli not synced after backup restore, and double backup file reads on the list→assess pipeline.Confidence Score: 2/5
lib/storage.ts(symlink protection removed, AV retry window halved, skipped count over-reported) andlib/codex-manager.ts(codex CLI not synced after restore, double backup reads, setStoragePath(null) missing) represent real windows-desktop regressions that affect the backup restore path this branch depends on. those are not minor style concerns — they are the exact class of filesystem concurrency and token-safety bugs this project requires to be explicitly addressed before merge.Important Files Changed
shouldApplyCodexCliSelectiontimestamp logic; several concerns flagged in previous threads remain (runAt stamped twice for changed runs, RETRYABLE_SELECTION_TIMESTAMP_CODES now includes EIO/ENXIO which is an improvement, dead-code branch at localVersion<=0 replaced with explicit return true)promptSyncCenterandbuildSyncCenterOverviewfor the new sync center UI;formatSyncCenterStatusErrorcorrectly usesgetRedactedFilesystemErrorLabel; outer try-catch in apply path is dead code;normalizePathForComparisonmisses forward-slash UNC paths for case-insensitive matchingactiveLiveAccountSyncSnapshots,lastStoppedLiveAccountSyncSnapshot) and generation-based concurrency guards;shouldIgnoreWatchedStorageSiblingintentionally passes.bak/.walthrough (confirmed by tests);reloadQueueddo-while loop correctly handles queued reloads without data racessuppressRecoveryPrompt,allowEmptyStorageMenu, andresolveStartupRecoveryAction; confirm text now shows both visible and recoverable backup counts addressing previous discrepancy;resolveStartupRecoveryActionlacks direct unit tests; several pre-existing concerns (double backup reads, no CLI sync after restore, setStoragePath(null) removed from restore-backup) remain unaddressedcommitPendingCodexCliSyncRun/commitCodexCliSyncRunFailureinto the background sync path; the legacysyncAccountStorageFromCodexClialways returnspendingRun: nullso the calls are currently no-ops but correctly set up for future promotion to the new sync pathgetRedactedFilesystemErrorLabel,loadAccountsReadOnly,scanNamedBackups, andActionableNamedBackupRecoveries; several serious concerns remain from previous threads: backup root symlink protection removed, retry window cut from ~1.26s to ~150ms, skipped count over-reported with internal duplicates, eligibleForRestore semantics silently changedbuildSyncCenterOverviewfor all outcome types, Windows drive-letter and UNC path matching, and retry behavior on EBUSY lock; does not test the forward-slash mixed-case UNC mismatch edge case flagged in the reviewshouldIgnoreWatchedStorageSibling, multi-instance snapshot publishing, and generation-based concurrency; explicitly documents that.bak/.walreturn false (trigger reloads)Sequence Diagram
sequenceDiagram participant User participant promptSyncCenter participant buildPreviewSafely participant previewCodexCliSync participant loadCodexCliState participant applyCodexCliSyncToStorage participant withAccountStorageTransaction participant persist participant commitPendingRun User->>promptSyncCenter: open sync center promptSyncCenter->>buildPreviewSafely: initial load (forceRefresh=true) buildPreviewSafely->>loadCodexCliState: load source state loadCodexCliState-->>buildPreviewSafely: sourceState buildPreviewSafely->>previewCodexCliSync: preview(current, sourceState) previewCodexCliSync-->>buildPreviewSafely: CodexCliSyncPreview buildPreviewSafely-->>promptSyncCenter: {preview, context, sourceState} loop menu loop User->>promptSyncCenter: select action alt refresh promptSyncCenter->>buildPreviewSafely: rebuild (forceRefresh=true) buildPreviewSafely-->>promptSyncCenter: updated {preview, context, sourceState} else apply (status=ready only) promptSyncCenter->>withAccountStorageTransaction: begin transaction withAccountStorageTransaction->>applyCodexCliSyncToStorage: apply(current, sourceState) applyCodexCliSyncToStorage->>loadCodexCliState: skipped — sourceState override used applyCodexCliSyncToStorage-->>withAccountStorageTransaction: {storage, changed, pendingRun} alt changed withAccountStorageTransaction->>persist: persist(storage, backupEnabled) alt persist fails persist-->>withAccountStorageTransaction: throw withAccountStorageTransaction->>commitPendingRun: commitCodexCliSyncRunFailure(pendingRun, error) withAccountStorageTransaction-->>promptSyncCenter: throw → status=error else persist succeeds persist-->>withAccountStorageTransaction: ok withAccountStorageTransaction-->>promptSyncCenter: synced promptSyncCenter->>commitPendingRun: commitPendingCodexCliSyncRun(pendingRun) promptSyncCenter->>buildPreviewSafely: post-apply refresh buildPreviewSafely-->>promptSyncCenter: updated preview (status=noop) end end else back promptSyncCenter-->>User: exit end endComments Outside Diff (4)
lib/codex-manager.ts, line 1057-1058 (link)epoch-date display bug for
lastUsed: 0the
=== 0and!Number.isFiniteguards were removed.toStorageAccountinlib/codex-cli/sync.tsexplicitly setslastUsed: 0for every account imported from codex cli. without the zero guard,formatRelativeDateShort(0)now computesdays = Math.floor((Date.now() - 0) / 86_400_000)(~20 000 days) and falls through tonew Date(0).toLocaleDateString()→ "1/1/1970", instead of returning null and hiding the field.Prompt To Fix With AI
lib/storage.ts, line 2826-2828 (link)EBUSYnow retried on all platforms, not just windowsold:
EBUSYwas only retried onprocess.platform === "win32". new:EBUSYis retried everywhere. this is a correctness improvement for linux/mac (docker mounts, nfs, overlayfs) but it's a silent behavioral change worth noting: callers on linux that previously got a fastEBUSYfailure will now wait up to ~310 ms (10+20+40+80+160 ms backoff) before propagating the error.also the old code had a separate
TRANSIENT_FILESYSTEM_MAX_ATTEMPTS = 7giving ~1.26 s total; new code hard-codesattempt < 5andattempt === 4for the rethrow. the max retry count dropped from 7 to 5 — make sure any tuning discussion in comments/docs still reflects the new ceiling.Prompt To Fix With AI
lib/codex-manager.ts, line 141-149 (link)timestamp === 0guard silently removed — epoch date shown for synced accountsthe old function returned
nullfortimestamp === 0. the new version drops that guard, soformatRelativeDateShup(0)computesdays ≈ 20 000, falls through tonew Date(0).toLocaleDateString(), and renders "1/1/1970" (or locale equivalent) in account menus.toStorageAccountinlib/codex-cli/sync.tsexplicitly setslastUsed: 0for every account written by the new Codex CLI reconcile path. every account synced through the new sync center will show a 1970 last-used date until it is actually used.Prompt To Fix With AI
lib/codex-manager.ts, line 4146-4156 (link)Recovery re-prompt loops on failed restore
when
runBackupRestoreManagerreturns"dismissed"or"failed",recoveryPromptAttemptedis reset tofalseandcontinue loginFlowfires. on the next iteration,existingCountis still 0,!recoveryPromptAttemptedistrue, and the startup recovery confirm prompt appears again immediately.on windows with a filesystem error (e.g.,
EPERM/EBUSYon the backup file),restoreResultwill be"failed"every iteration and the user is stuck re-prompting until they explicitly say "no" to the initial confirm. there is no consecutive-failure counter or cooldown. suggest only resettingrecoveryPromptAttemptedwhen the result is"dismissed"(user backed out intentionally), not when it is"failed":Prompt To Fix With AI
Prompt To Fix All With AI
Last reviewed commit: 2a8d6f2
Context used: