feat(auth): prompt for recovery on startup#92
feat(auth): prompt for recovery on startup#92ndycode wants to merge 1 commit intogit-plan/02-backup-restore-managerfrom
Conversation
|
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 (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 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}
| @@ -3868,17 +3912,6 @@ async function runAuthLogin(): Promise<number> { | |||
| console.log("Cancelled."); | |||
There was a problem hiding this comment.
modeTouchesQuotaCache await guard removed — quota-cache race introduced
the deleted block waited for any in-flight background pendingMenuQuotaRefresh before the user entered a quota-cache-touching action (check, deep-check, forecast, fix). that was the only synchronisation point preventing a concurrent background write to the on-disk quota cache while one of those operations was also reading and rewriting it. the test "waits for an in-flight menu quota refresh before starting quick check" was also removed in this PR, so the race has no regression coverage. on windows, two concurrent writeFile calls to the same cache file reliably corrupt it or trigger EBUSY — the exact scenario retryTransientFilesystemOperation is meant to handle.
if the await was removed intentionally (e.g. because the quota-cache operations now use transactions) please add a comment documenting why the race is safe; otherwise this guard should be restored.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 3912
Comment:
**`modeTouchesQuotaCache` await guard removed — quota-cache race introduced**
the deleted block waited for any in-flight background `pendingMenuQuotaRefresh` before the user entered a quota-cache-touching action (`check`, `deep-check`, `forecast`, `fix`). that was the only synchronisation point preventing a concurrent background write to the on-disk quota cache while one of those operations was also reading and rewriting it. the test `"waits for an in-flight menu quota refresh before starting quick check"` was also removed in this PR, so the race has no regression coverage. on windows, two concurrent `writeFile` calls to the same cache file reliably corrupt it or trigger `EBUSY` — the exact scenario `retryTransientFilesystemOperation` is meant to handle.
if the await was removed intentionally (e.g. because the quota-cache operations now use transactions) please add a comment documenting why the race is safe; otherwise this guard should be restored.
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.
1 issue found across 9 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/codex-manager.ts">
<violation number="1" location="lib/codex-manager.ts:4410">
P3: Do not collapse backup-directory read errors into `[]`; it makes the caller emit a false “No named backups found” result.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ): Promise<BackupRestoreManagerResult> { | ||
| const backupDir = getNamedBackupsDirectoryPath(); | ||
| const assessments = | ||
| assessmentsOverride ?? (await loadBackupRestoreManagerAssessments()); |
There was a problem hiding this comment.
P3: Do not collapse backup-directory read errors into []; it makes the caller emit a false “No named backups found” result.
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 4410:
<comment>Do not collapse backup-directory read errors into `[]`; it makes the caller emit a false “No named backups found” result.</comment>
<file context>
@@ -4266,6 +4398,21 @@ async function runBackupRestoreManager(
+): Promise<BackupRestoreManagerResult> {
+ const backupDir = getNamedBackupsDirectoryPath();
+ const assessments =
+ assessmentsOverride ?? (await loadBackupRestoreManagerAssessments());
+ if (assessments.length === 0) {
+ console.log(`No named backups found. Place backup files in ${backupDir}.`);
</file context>
1 participant
Summary
codex auth loginstarts with zero saved accounts and recoverable backups existfreshandresetactionsValidation
npm run typechecknpm run lintnpm run buildnpx vitest run test/storage.test.tsnpx vitest run test/recovery.test.ts -t getActionableNamedBackupRestorescmd /c "set NODE_OPTIONS=--max-old-space-size=16384&& npx vitest run test/codex-manager-cli.test.ts -t startup"npx vitest run test/codex-manager-cli.test.ts -t "returns to the login menu when backup reassessment fails before confirmation"Summary by cubic
Adds an interactive startup recovery prompt to
codex auth loginthat offers restoring a named backup before OAuth when no accounts are saved and recoverable backups exist. Reduces duplicate backup scanning, improves interactive detection, and updates docs and tests.New Features
fresh/resetand in non-interactive/fallback flows.Refactors
getActionableNamedBackupRestoresandresolveStartupRecoveryActionto avoid a second discovery pass and tighten test coverage.isInteractiveLoginMenuAvailable()and improved error handling by rethrowing unreadable backup directory errors consistently.Written for commit 388f263. 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 adds a startup recovery prompt to
codex auth loginthat offers named-backup restore before OAuth when the session starts with zero accounts and at least one actionable backup exists. the feature is well-scoped — prompt suppression afterfresh/reset, cached scan reuse, and re-prompt on back-out are all covered by new vitest cases.two unintended regressions were introduced alongside the feature:
isRetryableFilesystemErrorCodenow retriesEPERMon all platforms (lib/storage.ts). the original code explicitly restricted EPERM retries toprocess.platform === "win32"because on linux/macOS it is a permanent "operation not permitted" error. the new one-liner drops that guard, meaning real unix permission failures silently burn through 4 retries (~150 ms) before surfacing. this is the opposite of the windows-concurrency safety that this codebase targets.modeTouchesQuotaCacheawait removed (lib/codex-manager.ts). the deleted block was the sole synchronisation point that prevented an in-flight backgroundpendingMenuQuotaRefreshfrom racing with quota-cache-touching operations (check,deep-check,forecast,fix). on windows, two concurrentwriteFilecalls to the same cache file will triggerEBUSYor silent corruption. the test that reproduced this race ("waits for an in-flight menu quota refresh before starting quick check") was also deleted without explanation.additional minor points:
Math.random() * 10) was dropped, making synchronised retry storms more likely on windows with av-locked filesgetRedactedFilesystemErrorLabelincodex-manager.tsis a byte-for-byte duplicate ofgetBackupRestoreAssessmentErrorLabelinstorage.tsresolveStartupRecoveryActionis exported but has no direct unit tests; edge cases are covered only through the integration suiteConfidence Score: 2/5
codex auth logininvocation, not just the new recovery flow.lib/storage.ts(EPERM retry regression, jitter removal) andlib/codex-manager.ts(quota-cache await guard removal)Important Files Changed
getActionableNamedBackupRestores,scanNamedBackups,listNamedBackupsWithoutLoading, andassessNamedBackupRestoreCandidate; contains two regressions: EPERM is now retried on all platforms (should be Windows-only) and retry jitter was silently droppedmodeTouchesQuotaCacheawait guard without explanation, introducing a quota-cache read/write race for check/deep-check/forecast/fix actions; also duplicatesgetRedactedFilesystemErrorLabelalready present in storage.tsisInteractiveLoginMenuAvailable()helper combining!isNonInteractiveMode() && isTTY(); replaces bareisTTY()guard inpromptLoginMode— clean, straightforward change"waits for in-flight menu quota refresh before starting quick check"test was deleted alongside the guard it coveredafterEach(vi.restoreAllMocks), addsremoveWithRetryWindows-safe cleanup helper, and newgetActionableNamedBackupRestores (override)suite — no issuesFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A([runAuthLogin]) --> B[loadAccounts] B --> C{accounts > 0\nor allowEmptyStorageMenu?} C -- yes --> D[inner menu loop\npromptLoginMode] D --> E{menuResult.mode} E -- fresh/reset --> F[run action\nsuppressRecoveryPrompt=true\ncontinue loginFlow] F --> B E -- cancel --> G[exit to OAuth path] E -- other --> D C -- no --> G G --> H{canPromptForRecovery?\nnot suppressed, not attempted\ncount=0, interactive} H -- no --> L[runOAuthFlow] H -- yes --> I[getActionableNamedBackupRestores] I -- error --> J[warn + continue\nto OAuth] J --> L I -- 0 actionable\n0 scan error --> K[allowEmptyStorageMenu=true\ncontinue loginFlow] K --> B I -- actionable backups --> M[confirm: Restore now?] M -- no --> L M -- yes --> N[runBackupRestoreManager\nwith cached allAssessments] N -- restored --> O[continue loginFlow] O --> B N -- dismissed --> P[pendingRecoveryState=cached\nrecoveryPromptAttempted=false\ncontinue loginFlow] P --> B L --> Q{OAuth result} Q -- success --> R[promptAddAnotherAccount loop] Q -- failed/cancel --> S([exit 0]) R --> SComments Outside Diff (1)
lib/storage.ts, line 683 (link)EPERM now retried on all platforms — breaks intentional Windows-only guard
the old code explicitly restricted EPERM retries to
process.platform === "win32"because on linux/macOSEPERMis a hard, permanent "operation not permitted" error (not a transient AV/lock race). the new one-liner silently retries EPERM on all platforms, meaning a real linux permission failure will burn through 4 retries (~150ms of wasted exponential backoff) before surfacing. this is a regression for the vast majority of non-windows deployments and also obscures the original intent.Prompt To Fix With AI
Prompt To Fix All With AI
Last reviewed commit: 388f263
Context used: