feat: harden auth storage recovery and mirror sync#51
Closed
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (15)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+733
to
+786
| async function loadAccountsForRestoreAssessment(path: string): Promise<AccountStorageWithMetadata | null> { | ||
| const resetMarkerPath = getIntentionalResetMarkerPath(path); | ||
| const hasIntentionalResetMarker = existsSync(resetMarkerPath); | ||
|
|
||
| try { | ||
| const { normalized, schemaErrors } = await loadAccountsFromPath(path); | ||
| if (schemaErrors.length > 0) { | ||
| log.warn("Account storage schema validation warnings", { | ||
| errors: schemaErrors.slice(0, 5), | ||
| }); | ||
| } | ||
| if (!normalized) { | ||
| return null; | ||
| } | ||
|
|
||
| if (hasIntentionalResetMarker) { | ||
| await removeIntentionalResetMarker(path); | ||
| } | ||
|
|
||
| const annotated: AccountStorageWithMetadata = { ...normalized }; | ||
| if (annotated.accounts.length === 0) { | ||
| annotated.restoreEligible = hasIntentionalResetMarker ? false : true; | ||
| annotated.restoreReason = hasIntentionalResetMarker ? "intentional-reset" : "empty-storage"; | ||
| } else if (hasIntentionalResetMarker) { | ||
| annotated.restoreEligible = false; | ||
| annotated.restoreReason = "intentional-reset"; | ||
| } | ||
|
|
||
| return annotated; | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code === "ENOENT") { | ||
| if (hasIntentionalResetMarker) { | ||
| await removeIntentionalResetMarker(path); | ||
| return { | ||
| ...createEmptyAccountStorage(), | ||
| restoreEligible: false, | ||
| restoreReason: "intentional-reset", | ||
| }; | ||
| } | ||
| return { | ||
| ...createEmptyAccountStorage(), | ||
| restoreEligible: true, | ||
| restoreReason: "missing-storage", | ||
| }; | ||
| } | ||
|
|
||
| log.warn("Failed to load account storage for restore assessment", { | ||
| path, | ||
| error: String(error), | ||
| }); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
reset marker consumed too early — WAL recovery leaks tokens after intentional reset.
loadAccountsForRestoreAssessment removes the reset marker when it sees ENOENT + marker, then resolveLoginStorage calls loadAccounts() immediately after. at that point loadAccountsInternal no longer sees the marker, falls through to WAL recovery, and can return the pre-clear account pool (including old refresh tokens) as if no reset happened.
concrete path:
clearAccounts()→ writes marker, unlinks primarygetRestoreAssessment()→loadAccountsForRestoreAssessment→ removes marker, returnsrestoreEligible: falseresolveLoginStorage()→restoreEligible: false→ callsloadAccounts()loadAccountsInternal→ marker gone → WAL present → returns cleared accounts
on windows this is especially risky: stale tokens that should have been invalidated can resurface and be synced into codex cli mirror state.
fix: either also clear the WAL in clearAccounts(), or have loadAccountsInternal treat a valid WAL found after an ENOENT on primary as a restore candidate (not an automatic recovery) when restoreEligible is the expected state. the simplest defensive fix is to unlink the WAL in clearAccounts() the same way the old implementation did — the WAL is a journal artifact, not a user-created backup, and should be regenerated on the next save.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/storage.ts
Line: 733-786
Comment:
reset marker consumed too early — WAL recovery leaks tokens after intentional reset.
`loadAccountsForRestoreAssessment` removes the reset marker when it sees `ENOENT + marker`, then `resolveLoginStorage` calls `loadAccounts()` immediately after. at that point `loadAccountsInternal` no longer sees the marker, falls through to WAL recovery, and can return the pre-clear account pool (including old refresh tokens) as if no reset happened.
concrete path:
1. `clearAccounts()` → writes marker, unlinks primary
2. `getRestoreAssessment()` → `loadAccountsForRestoreAssessment` → removes marker, returns `restoreEligible: false`
3. `resolveLoginStorage()` → `restoreEligible: false` → calls `loadAccounts()`
4. `loadAccountsInternal` → marker gone → WAL present → returns cleared accounts
on windows this is especially risky: stale tokens that should have been invalidated can resurface and be synced into codex cli mirror state.
fix: either also clear the WAL in `clearAccounts()`, or have `loadAccountsInternal` treat a valid WAL found after an ENOENT on primary as a restore candidate (not an automatic recovery) when `restoreEligible` is the expected state. the simplest defensive fix is to unlink the WAL in `clearAccounts()` the same way the old implementation did — the WAL is a journal artifact, not a user-created backup, and should be regenerated on the next save.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+47
to
81
| export function syncAccountStorageFromCodexCli( | ||
| current: AccountStorageV3 | null, | ||
| ): Promise<{ storage: AccountStorageV3 | null; changed: boolean }> { | ||
| incrementCodexCliMetric("reconcileAttempts"); | ||
| try { | ||
| const state = await loadCodexCliState(); | ||
| if (!state) { | ||
| incrementCodexCliMetric("reconcileNoops"); | ||
| return { storage: current, changed: false }; | ||
| } | ||
|
|
||
| const next = current ? cloneStorage(current) : createEmptyStorage(); | ||
| let changed = false; | ||
|
|
||
| for (const snapshot of state.accounts) { | ||
| const updated = upsertFromSnapshot(next.accounts, snapshot); | ||
| if (updated) changed = true; | ||
| } | ||
|
|
||
| if (next.accounts.length === 0) { | ||
| incrementCodexCliMetric(changed ? "reconcileChanges" : "reconcileNoops"); | ||
| log.debug("Codex CLI reconcile completed", { | ||
| operation: "reconcile-storage", | ||
| outcome: changed ? "changed" : "noop", | ||
| accountCount: next.accounts.length, | ||
| }); | ||
| return { | ||
| storage: current ?? next, | ||
| changed, | ||
| }; | ||
| } | ||
|
|
||
| const activeFromSnapshots = readActiveFromSnapshots(state.accounts); | ||
| const applyActiveFromCodex = shouldApplyCodexCliSelection(state); | ||
| if (applyActiveFromCodex) { | ||
| const desiredIndex = resolveActiveIndex( | ||
| next.accounts, | ||
| state.activeAccountId ?? activeFromSnapshots.accountId, | ||
| state.activeEmail ?? activeFromSnapshots.email, | ||
| ); | ||
| incrementCodexCliMetric("reconcileAttempts"); | ||
|
|
||
| const previousActive = next.activeIndex; | ||
| const previousFamilies = JSON.stringify(next.activeIndexByFamily ?? {}); | ||
| writeFamilyIndexes(next, desiredIndex); | ||
| normalizeStoredFamilyIndexes(next); | ||
| if (previousActive !== next.activeIndex) { | ||
| changed = true; | ||
| } | ||
| if (previousFamilies !== JSON.stringify(next.activeIndexByFamily ?? {})) { | ||
| changed = true; | ||
| } | ||
| } else { | ||
| const previousActive = next.activeIndex; | ||
| const previousFamilies = JSON.stringify(next.activeIndexByFamily ?? {}); | ||
| normalizeStoredFamilyIndexes(next); | ||
| if (previousActive !== next.activeIndex) { | ||
| changed = true; | ||
| } | ||
| if (previousFamilies !== JSON.stringify(next.activeIndexByFamily ?? {})) { | ||
| changed = true; | ||
| } | ||
| log.debug("Skipped Codex CLI active selection overwrite due to newer local state", { | ||
| operation: "reconcile-storage", | ||
| outcome: "local-newer", | ||
| }); | ||
| } | ||
|
|
||
| incrementCodexCliMetric(changed ? "reconcileChanges" : "reconcileNoops"); | ||
| log.debug("Codex CLI reconcile completed", { | ||
| operation: "reconcile-storage", | ||
| outcome: changed ? "changed" : "noop", | ||
| accountCount: next.accounts.length, | ||
| activeAccountRef: makeAccountFingerprint({ | ||
| accountId: state.activeAccountId ?? activeFromSnapshots.accountId, | ||
| email: state.activeEmail ?? activeFromSnapshots.email, | ||
| }), | ||
| }); | ||
| return { | ||
| storage: next, | ||
| changed, | ||
| }; | ||
| } catch (error) { | ||
| incrementCodexCliMetric("reconcileFailures"); | ||
| log.warn("Codex CLI reconcile failed", { | ||
| if (!current) { | ||
| incrementCodexCliMetric("reconcileNoops"); | ||
| log.debug("Skipped Codex CLI reconcile because canonical storage is missing", { | ||
| operation: "reconcile-storage", | ||
| outcome: "error", | ||
| error: String(error), | ||
| outcome: "canonical-missing", | ||
| }); | ||
| return { storage: current, changed: false }; | ||
| return Promise.resolve({ storage: null, changed: false }); | ||
| } | ||
|
|
||
| const next = cloneStorage(current); | ||
| const previousActive = next.activeIndex; | ||
| const previousFamilies = JSON.stringify(next.activeIndexByFamily ?? {}); | ||
| normalizeStoredFamilyIndexes(next); | ||
|
|
||
| const changed = | ||
| previousActive !== next.activeIndex || | ||
| previousFamilies !== JSON.stringify(next.activeIndexByFamily ?? {}); | ||
|
|
||
| incrementCodexCliMetric(changed ? "reconcileChanges" : "reconcileNoops"); | ||
| log.debug("Skipped Codex CLI authority import; canonical storage remains authoritative", { | ||
| operation: "reconcile-storage", | ||
| outcome: changed ? "normalized-local-indexes" : "canonical-authoritative", | ||
| accountCount: next.accounts.length, | ||
| }); | ||
|
|
||
| return Promise.resolve({ | ||
| storage: changed ? next : current, | ||
| changed, | ||
| }); | ||
| } |
There was a problem hiding this comment.
mixed tabs and spaces indentation in function body.
the function signature and initial line use 4-space indent, but the function body uses tabs, and the return statement switches back to 4-space indent. this will fail the project's indent linter.
Suggested change
| export function syncAccountStorageFromCodexCli( | |
| current: AccountStorageV3 | null, | |
| ): Promise<{ storage: AccountStorageV3 | null; changed: boolean }> { | |
| incrementCodexCliMetric("reconcileAttempts"); | |
| try { | |
| const state = await loadCodexCliState(); | |
| if (!state) { | |
| incrementCodexCliMetric("reconcileNoops"); | |
| return { storage: current, changed: false }; | |
| } | |
| const next = current ? cloneStorage(current) : createEmptyStorage(); | |
| let changed = false; | |
| for (const snapshot of state.accounts) { | |
| const updated = upsertFromSnapshot(next.accounts, snapshot); | |
| if (updated) changed = true; | |
| } | |
| if (next.accounts.length === 0) { | |
| incrementCodexCliMetric(changed ? "reconcileChanges" : "reconcileNoops"); | |
| log.debug("Codex CLI reconcile completed", { | |
| operation: "reconcile-storage", | |
| outcome: changed ? "changed" : "noop", | |
| accountCount: next.accounts.length, | |
| }); | |
| return { | |
| storage: current ?? next, | |
| changed, | |
| }; | |
| } | |
| const activeFromSnapshots = readActiveFromSnapshots(state.accounts); | |
| const applyActiveFromCodex = shouldApplyCodexCliSelection(state); | |
| if (applyActiveFromCodex) { | |
| const desiredIndex = resolveActiveIndex( | |
| next.accounts, | |
| state.activeAccountId ?? activeFromSnapshots.accountId, | |
| state.activeEmail ?? activeFromSnapshots.email, | |
| ); | |
| incrementCodexCliMetric("reconcileAttempts"); | |
| const previousActive = next.activeIndex; | |
| const previousFamilies = JSON.stringify(next.activeIndexByFamily ?? {}); | |
| writeFamilyIndexes(next, desiredIndex); | |
| normalizeStoredFamilyIndexes(next); | |
| if (previousActive !== next.activeIndex) { | |
| changed = true; | |
| } | |
| if (previousFamilies !== JSON.stringify(next.activeIndexByFamily ?? {})) { | |
| changed = true; | |
| } | |
| } else { | |
| const previousActive = next.activeIndex; | |
| const previousFamilies = JSON.stringify(next.activeIndexByFamily ?? {}); | |
| normalizeStoredFamilyIndexes(next); | |
| if (previousActive !== next.activeIndex) { | |
| changed = true; | |
| } | |
| if (previousFamilies !== JSON.stringify(next.activeIndexByFamily ?? {})) { | |
| changed = true; | |
| } | |
| log.debug("Skipped Codex CLI active selection overwrite due to newer local state", { | |
| operation: "reconcile-storage", | |
| outcome: "local-newer", | |
| }); | |
| } | |
| incrementCodexCliMetric(changed ? "reconcileChanges" : "reconcileNoops"); | |
| log.debug("Codex CLI reconcile completed", { | |
| operation: "reconcile-storage", | |
| outcome: changed ? "changed" : "noop", | |
| accountCount: next.accounts.length, | |
| activeAccountRef: makeAccountFingerprint({ | |
| accountId: state.activeAccountId ?? activeFromSnapshots.accountId, | |
| email: state.activeEmail ?? activeFromSnapshots.email, | |
| }), | |
| }); | |
| return { | |
| storage: next, | |
| changed, | |
| }; | |
| } catch (error) { | |
| incrementCodexCliMetric("reconcileFailures"); | |
| log.warn("Codex CLI reconcile failed", { | |
| if (!current) { | |
| incrementCodexCliMetric("reconcileNoops"); | |
| log.debug("Skipped Codex CLI reconcile because canonical storage is missing", { | |
| operation: "reconcile-storage", | |
| outcome: "error", | |
| error: String(error), | |
| outcome: "canonical-missing", | |
| }); | |
| return { storage: current, changed: false }; | |
| return Promise.resolve({ storage: null, changed: false }); | |
| } | |
| const next = cloneStorage(current); | |
| const previousActive = next.activeIndex; | |
| const previousFamilies = JSON.stringify(next.activeIndexByFamily ?? {}); | |
| normalizeStoredFamilyIndexes(next); | |
| const changed = | |
| previousActive !== next.activeIndex || | |
| previousFamilies !== JSON.stringify(next.activeIndexByFamily ?? {}); | |
| incrementCodexCliMetric(changed ? "reconcileChanges" : "reconcileNoops"); | |
| log.debug("Skipped Codex CLI authority import; canonical storage remains authoritative", { | |
| operation: "reconcile-storage", | |
| outcome: changed ? "normalized-local-indexes" : "canonical-authoritative", | |
| accountCount: next.accounts.length, | |
| }); | |
| return Promise.resolve({ | |
| storage: changed ? next : current, | |
| changed, | |
| }); | |
| } | |
| incrementCodexCliMetric(changed ? "reconcileChanges" : "reconcileNoops"); | |
| log.debug("Skipped Codex CLI authority import; canonical storage remains authoritative", { | |
| operation: "reconcile-storage", | |
| outcome: changed ? "normalized-local-indexes" : "canonical-authoritative", | |
| accountCount: next.accounts.length, | |
| }); | |
| return Promise.resolve({ | |
| storage: changed ? next : current, | |
| changed, | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-cli/sync.ts
Line: 47-81
Comment:
mixed tabs and spaces indentation in function body.
the function signature and initial line use 4-space indent, but the function body uses tabs, and the return statement switches back to 4-space indent. this will fail the project's indent linter.
```suggestion
incrementCodexCliMetric(changed ? "reconcileChanges" : "reconcileNoops");
log.debug("Skipped Codex CLI authority import; canonical storage remains authoritative", {
operation: "reconcile-storage",
outcome: changed ? "normalized-local-indexes" : "canonical-authoritative",
accountCount: next.accounts.length,
});
return Promise.resolve({
storage: changed ? next : current,
changed,
});
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
1768
to
1793
| export async function clearAccounts(): Promise<void> { | ||
| return withStorageLock(async () => { | ||
| const path = getStoragePath(); | ||
| const walPath = getAccountsWalPath(path); | ||
| const backupPaths = getAccountsBackupRecoveryCandidates(path); | ||
| const clearPath = async (targetPath: string): Promise<void> => { | ||
| try { | ||
| await fs.unlink(targetPath); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| log.error("Failed to clear account storage artifact", { | ||
| path: targetPath, | ||
| error: String(error), | ||
| }); | ||
| } | ||
| } | ||
| }; | ||
| const markerPath = getIntentionalResetMarkerPath(path); | ||
|
|
||
| try { | ||
| await Promise.all([clearPath(path), clearPath(walPath), ...backupPaths.map(clearPath)]); | ||
| } catch { | ||
| // Individual path cleanup is already best-effort with per-artifact logging. | ||
| } | ||
| try { | ||
| await fs.mkdir(dirname(path), { recursive: true }); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "EEXIST" && code !== "ENOENT") { | ||
| log.warn("Failed to ensure storage directory before reset", { path, error: String(error) }); | ||
| } | ||
| } | ||
|
|
||
| await writeIntentionalResetMarker(path); | ||
|
|
||
| try { | ||
| await fs.unlink(path); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| log.error("Failed to clear account storage", { path, markerPath, error: String(error) }); | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
clearAccounts() no longer clears the WAL — breaks the old reset safety guarantee.
the old implementation deleted the WAL alongside the primary file to ensure all account data was gone after a user-initiated reset. the new implementation deliberately preserves backup files (.bak snapshots) for the recovery UI, which makes sense for user-visible backups, but the WAL is a low-level journal artifact, not a user-created backup.
preserving the WAL means a corrupted/partial write before the reset can surface as recovered accounts the next time loadAccountsInternal runs (see the WAL recovery bug noted on loadAccountsForRestoreAssessment). on windows, where file-locking can leave WAL writes partial, this is a real data-integrity risk.
consider adding the WAL to the items cleared here, or at minimum ensure loadAccountsInternal skips WAL recovery when the reset marker was recently present.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/storage.ts
Line: 1768-1793
Comment:
`clearAccounts()` no longer clears the WAL — breaks the old reset safety guarantee.
the old implementation deleted the WAL alongside the primary file to ensure all account data was gone after a user-initiated reset. the new implementation deliberately preserves backup files (`.bak` snapshots) for the recovery UI, which makes sense for user-visible backups, but the WAL is a low-level journal artifact, not a user-created backup.
preserving the WAL means a corrupted/partial write before the reset can surface as recovered accounts the next time `loadAccountsInternal` runs (see the WAL recovery bug noted on `loadAccountsForRestoreAssessment`). on windows, where file-locking can leave WAL writes partial, this is a real data-integrity risk.
consider adding the WAL to the items cleared here, or at minimum ensure `loadAccountsInternal` skips WAL recovery when the reset marker was recently present.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/runtime-paths.ts">
<violation number="1" location="lib/runtime-paths.ts:81">
P2: Normalize non-Windows paths before comparison; trailing slashes currently cause false `non-default` detection and bypass fallback recovery.</violation>
</file>
<file name="docs/reference/storage-paths.md">
<violation number="1" location="docs/reference/storage-paths.md:42">
P3: The flagged-account metadata does not include a WAL snapshot in `getBackupMetadata()`. This sentence implies the flagged state also has WAL snapshots, which is misleading—only the main account pool has a WAL entry.</violation>
</file>
<file name="test/codex-cli-sync.test.ts">
<violation number="1" location="test/codex-cli-sync.test.ts:64">
P3: Avoid bare fs.rm in test cleanup; use a removeWithRetry helper to handle Windows EBUSY/EPERM locks per repo test conventions.</violation>
</file>
<file name="lib/codex-manager/settings-persistence.ts">
<violation number="1" location="lib/codex-manager/settings-persistence.ts:124">
P2: Merging against `loadDashboardDisplaySettings()` can overwrite unrelated settings with defaults when the unified settings read transiently fails.</violation>
</file>
<file name="lib/codex-manager/auth-ui-controller.ts">
<violation number="1" location="lib/codex-manager/auth-ui-controller.ts:128">
P2: Normalize active index to an integer before clamping; fractional values can prevent current-account matching and break index-based lookups.</violation>
</file>
<file name="lib/codex-manager.ts">
<violation number="1" location="lib/codex-manager.ts:796">
P1: Intentional reset can be bypassed because this branch reloads accounts from recovery sources after reset assessment.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:749">
P1: `getRestoreAssessment` clears the intentional-reset marker, which can re-enable automatic account recovery right after a user reset.</violation>
<violation number="2" location="lib/storage.ts:1415">
P1: Intentional-reset suppression is checked too late; fallback/legacy migration can repopulate storage before the reset guard runs.</violation>
</file>
<file name="test/codex-manager-cli.test.ts">
<violation number="1" location="test/codex-manager-cli.test.ts:2267">
P3: Test title contradicts its assertions. The test verifies that transactional persistence is NOT used, but the title claims it does.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const latestSnapshot = assessment.restoreEligible ? findLatestRestorableSnapshot(assessment) : undefined; | ||
|
|
||
| if (!assessment.restoreEligible || !latestSnapshot) { | ||
| return { storage: await loadAccounts() }; |
There was a problem hiding this comment.
P1: Intentional reset can be bypassed because this branch reloads accounts from recovery sources after reset assessment.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/codex-manager.ts, line 796:
<comment>Intentional reset can be bypassed because this branch reloads accounts from recovery sources after reset assessment.</comment>
<file context>
@@ -931,6 +657,196 @@ async function promptOAuthSignInMode(): Promise<OAuthSignInMode> {
+ const latestSnapshot = assessment.restoreEligible ? findLatestRestorableSnapshot(assessment) : undefined;
+
+ if (!assessment.restoreEligible || !latestSnapshot) {
+ return { storage: await loadAccounts() };
+ }
+
</file context>
Suggested change
| return { storage: await loadAccounts() }; | |
| if (assessment.restoreReason === "intentional-reset") { | |
| await ensureEmptyAccountPoolExists(); | |
| return { storage: createEmptyAccountStorage() }; | |
| } | |
| return { storage: await loadAccounts() }; |
| if (normalized && storedVersion !== normalized.version) { | ||
| log.info("Migrating account storage to v3", { from: storedVersion, to: normalized.version }); | ||
| if (persistMigration) { | ||
| if (hasIntentionalResetMarker && !existsSync(path)) { |
There was a problem hiding this comment.
P1: Intentional-reset suppression is checked too late; fallback/legacy migration can repopulate storage before the reset guard runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/storage.ts, line 1415:
<comment>Intentional-reset suppression is checked too late; fallback/legacy migration can repopulate storage before the reset guard runs.</comment>
<file context>
@@ -899,22 +1399,40 @@ async function loadAccountsFromJournal(path: string): Promise<AccountStorageV3 |
- if (normalized && storedVersion !== normalized.version) {
- log.info("Migrating account storage to v3", { from: storedVersion, to: normalized.version });
- if (persistMigration) {
+ if (hasIntentionalResetMarker && !existsSync(path)) {
+ await removeIntentionalResetMarker(path);
+ const emptyStorageWithMetadata: AccountStorageWithMetadata = {
</file context>
| } | ||
|
|
||
| if (hasIntentionalResetMarker) { | ||
| await removeIntentionalResetMarker(path); |
There was a problem hiding this comment.
P1: getRestoreAssessment clears the intentional-reset marker, which can re-enable automatic account recovery right after a user reset.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/storage.ts, line 749:
<comment>`getRestoreAssessment` clears the intentional-reset marker, which can re-enable automatic account recovery right after a user reset.</comment>
<file context>
@@ -391,6 +448,364 @@ function computeSha256(value: string): string {
+ }
+
+ if (hasIntentionalResetMarker) {
+ await removeIntentionalResetMarker(path);
+ }
+
</file context>
| if (process.platform === "win32") { | ||
| return win32.normalize(trimmed).toLowerCase(); | ||
| } | ||
| return trimmed; |
There was a problem hiding this comment.
P2: Normalize non-Windows paths before comparison; trailing slashes currently cause false non-default detection and bypass fallback recovery.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/runtime-paths.ts, line 81:
<comment>Normalize non-Windows paths before comparison; trailing slashes currently cause false `non-default` detection and bypass fallback recovery.</comment>
<file context>
@@ -72,6 +72,17 @@ function deduplicatePaths(paths: string[]): string[] {
+ if (process.platform === "win32") {
+ return win32.normalize(trimmed).toLowerCase();
+ }
+ return trimmed;
+ };
+ return normalize(a) === normalize(b);
</file context>
Suggested change
| return trimmed; | |
| return trimmed === "/" ? "/" : trimmed.replace(/\/+$/, ""); |
| const fallback = helpers.cloneSettings(selected); | ||
| try { | ||
| return await withQueuedRetry(getDashboardSettingsPath(), async () => { | ||
| const latest = helpers.cloneSettings(await loadDashboardDisplaySettings()); |
There was a problem hiding this comment.
P2: Merging against loadDashboardDisplaySettings() can overwrite unrelated settings with defaults when the unified settings read transiently fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/codex-manager/settings-persistence.ts, line 124:
<comment>Merging against `loadDashboardDisplaySettings()` can overwrite unrelated settings with defaults when the unified settings read transiently fails.</comment>
<file context>
@@ -0,0 +1,153 @@
+ const fallback = helpers.cloneSettings(selected);
+ try {
+ return await withQueuedRetry(getDashboardSettingsPath(), async () => {
+ const latest = helpers.cloneSettings(await loadDashboardDisplaySettings());
+ const merged = helpers.mergeSettingsForKeys(latest, selected, keys);
+ await saveDashboardDisplaySettings(merged);
</file context>
| const total = storage.accounts.length; | ||
| if (total === 0) return 0; | ||
| const rawCandidate = storage.activeIndexByFamily?.[family] ?? storage.activeIndex; | ||
| const raw = Number.isFinite(rawCandidate) ? rawCandidate : 0; |
There was a problem hiding this comment.
P2: Normalize active index to an integer before clamping; fractional values can prevent current-account matching and break index-based lookups.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/codex-manager/auth-ui-controller.ts, line 128:
<comment>Normalize active index to an integer before clamping; fractional values can prevent current-account matching and break index-based lookups.</comment>
<file context>
@@ -0,0 +1,526 @@
+ const total = storage.accounts.length;
+ if (total === 0) return 0;
+ const rawCandidate = storage.activeIndexByFamily?.[family] ?? storage.activeIndex;
+ const raw = Number.isFinite(rawCandidate) ? rawCandidate : 0;
+ return Math.max(0, Math.min(raw, total - 1));
+}
</file context>
|
|
||
| Backup metadata: | ||
|
|
||
| - `getBackupMetadata()` reports deterministic snapshot lists for the canonical account pool and flagged-account state (primary, WAL, `.bak`, `.bak.1`, `.bak.2`, and discovered manual backups). Cache-like artifacts are excluded from recovery candidates. |
There was a problem hiding this comment.
P3: The flagged-account metadata does not include a WAL snapshot in getBackupMetadata(). This sentence implies the flagged state also has WAL snapshots, which is misleading—only the main account pool has a WAL entry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/reference/storage-paths.md, line 42:
<comment>The flagged-account metadata does not include a WAL snapshot in `getBackupMetadata()`. This sentence implies the flagged state also has WAL snapshots, which is misleading—only the main account pool has a WAL entry.</comment>
<file context>
@@ -36,6 +37,10 @@ Ownership note:
+Backup metadata:
+
+- `getBackupMetadata()` reports deterministic snapshot lists for the canonical account pool and flagged-account state (primary, WAL, `.bak`, `.bak.1`, `.bak.2`, and discovered manual backups). Cache-like artifacts are excluded from recovery candidates.
+
---
</file context>
Suggested change
| - `getBackupMetadata()` reports deterministic snapshot lists for the canonical account pool and flagged-account state (primary, WAL, `.bak`, `.bak.1`, `.bak.2`, and discovered manual backups). Cache-like artifacts are excluded from recovery candidates. | |
| - `getBackupMetadata()` reports deterministic snapshot lists for the canonical account pool (primary, WAL, `.bak`, `.bak.1`, `.bak.2`, and discovered manual backups) and flagged-account state (primary, `.bak`, `.bak.1`, `.bak.2`, and discovered manual backups). Cache-like artifacts are excluded from recovery candidates. |
| process.env.CODEX_MULTI_AUTH_ENFORCE_CLI_FILE_AUTH_STORE = | ||
| previousEnforceFileStore; | ||
| } | ||
| await rm(tempDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
P3: Avoid bare fs.rm in test cleanup; use a removeWithRetry helper to handle Windows EBUSY/EPERM locks per repo test conventions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/codex-cli-sync.test.ts, line 64:
<comment>Avoid bare fs.rm in test cleanup; use a removeWithRetry helper to handle Windows EBUSY/EPERM locks per repo test conventions.</comment>
<file context>
@@ -6,877 +6,316 @@ import type { AccountStorageV3 } from "../lib/storage.js";
+ process.env.CODEX_MULTI_AUTH_ENFORCE_CLI_FILE_AUTH_STORE =
+ previousEnforceFileStore;
+ }
+ await rm(tempDir, { recursive: true, force: true });
+ });
+
</file context>
| expect(saveAccountsMock.mock.calls[0]?.[0]?.accounts?.[0]?.enabled).toBe(false); | ||
| }); | ||
|
|
||
| it("resets all accounts through transactional persistence", async () => { |
There was a problem hiding this comment.
P3: Test title contradicts its assertions. The test verifies that transactional persistence is NOT used, but the title claims it does.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/codex-manager-cli.test.ts, line 2267:
<comment>Test title contradicts its assertions. The test verifies that transactional persistence is NOT used, but the title claims it does.</comment>
<file context>
@@ -1967,9 +2260,39 @@ describe("codex manager cli commands", () => {
expect(saveAccountsMock.mock.calls[0]?.[0]?.accounts?.[0]?.enabled).toBe(false);
});
+ it("resets all accounts through transactional persistence", async () => {
+ const now = Date.now();
+ loadAccountsMock.mockResolvedValue({
</file context>
Suggested change
| it("resets all accounts through transactional persistence", async () => { | |
| it("resets all accounts directly without transactional persistence", async () => { |
This was referenced Mar 10, 2026
Owner
Author
|
Superseded by the replacement cleanup PRs:
These branches rebuild the intended scope from |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Summary by cubic
Hardens multi-auth storage recovery and keeps Codex CLI state as a read-only mirror so the canonical multi-auth store stays authoritative. Adds backup metadata, restore assessment, and safer path selection for more reliable recoveries.
New Features
getBackupMetadata()for accounts and flagged accounts.getRestoreAssessment()andloadAccounts()now surfacesrestoreEligibleand reason.auth-ui-controller(renderer-agnostic dashboard models/commands) andsettings-persistencewith queued, retryable writes.Bug Fixes
CODEX_HOMEhandling; steady-state root selection; legacy-only installs still detected.Written for commit e4ce7a4. Summary will update on new commits.
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr hardens canonical multi-auth storage with backup metadata, restore assessment, reset suppression via a marker file, fallback migration, and codex cli demotion to mirror-only. it also extracts
auth-ui-controller.tsandsettings-persistence.tsas shared seams for the upcoming ink ui migration.key changes:
clearAccounts()now writes a.reset-intentmarker instead of deleting WAL/backups, preserving recovery artifactsloadAccounts()returnsAccountStorageWithMetadataannotated withrestoreEligibleandrestoreReasongetRestoreAssessment()+getBackupMetadata()provide a structured snapshot inventory for the restore-before-login UXsyncAccountStorageFromCodexCliis reduced to index normalization only — canonical storage is never seeded from codex cli fileswithAccountStorageTransactionis now used for all multi-step writes inpersistAccountPool,runSwitch, andautoSyncActiveAccountToCodexcritical issues:
loadAccountsForRestoreAssessment(insidegetRestoreAssessment) before the subsequentloadAccounts()call inresolveLoginStorage. this meansloadAccountsInternalno longer sees the marker and can recover pre-reset accounts from the WAL — leaking refresh tokens the user explicitly cleared. the oldclearAccountsdeleted the WAL; the new one does not.syncAccountStorageFromCodexCliinlib/codex-cli/sync.tsmixes tabs and 4-space indentation, which will fail the project linterclearAccounts → getRestoreAssessment → loadAccountssequence, which is the most safety-critical path added in this prConfidence Score: 2/5
loadAccounts()runs allows the WAL to resurface cleared tokens — exactly the class of bug this pr is trying to prevent. additionally the pr is missing the mandatory concurrency/token-safety comment, the indentation issue will break ci linting, and the criticalclearAccounts → getRestoreAssessment → loadAccountssequence has no test coverageSequence Diagram
sequenceDiagram participant CLI as codex-manager CLI participant RLS as resolveLoginStorage participant GRA as getRestoreAssessment participant LARA as loadAccountsForRestoreAssessment participant LA as loadAccounts / loadAccountsInternal participant FS as Filesystem CLI->>RLS: runAuthLogin() RLS->>GRA: getRestoreAssessment() GRA->>LARA: loadAccountsForRestoreAssessment(storagePath) LARA->>FS: existsSync(resetMarkerPath) FS-->>LARA: true (clearAccounts was called) LARA->>FS: loadAccountsFromPath(primary) → ENOENT LARA->>FS: removeIntentionalResetMarker() ⚠️ consumed here LARA-->>GRA: {restoreEligible: false, restoreReason: "intentional-reset"} GRA-->>RLS: assessment {restoreEligible: false} RLS->>LA: loadAccounts() [marker already gone] LA->>FS: existsSync(resetMarkerPath) → false LA->>FS: loadAccountsFromPath(primary) → ENOENT LA->>FS: loadAccountsFromJournal(WAL) → ⚠️ old accounts! LA-->>RLS: AccountStorageWithMetadata (pre-reset tokens) RLS-->>CLI: {storage: <stale accounts>} note over LARA,LA: Bug: marker consumed in step 1 allows WAL recovery in step 2Last reviewed commit: e4ce7a4
Context used: