fix(auth): align OpenCode import replay with current contracts#102
fix(auth): align OpenCode import replay with current contracts#102
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. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughadds backup discovery/restore UI, opencode account-pool detection/import, sync preview/rollback, and snapshot/restore APIs; login menu gains new actions (restore-backup, import-opencode, reset) and a health-summary row passed into the auth menu. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant user as user
participant auth as auth menu (ui)
participant cli as cli prompt
participant mgr as manager (`lib/codex-manager.ts`)
participant storage as storage (`lib/storage.ts`)
participant settings as settings hub
user->>auth: select "import opencode"
auth->>cli: emit action import-opencode
cli->>mgr: runAuthLogin(mode: "import-opencode")
mgr->>storage: detectOpencodeAccountPoolPath()
storage-->>mgr: path | null
alt path found
mgr->>storage: assessOpencodeAccountPool(path)
storage-->>mgr: BackupRestoreAssessment
mgr->>cli: prompt import confirmation
cli->>user: request confirmation
user-->>cli: confirm
alt confirmed
mgr->>storage: importAccounts(path)
storage-->>mgr: import result
mgr->>settings: apply dashboard settings (optional)
mgr-->>user: report imported accounts
else cancelled
mgr-->>user: cancelled
end
else no path
mgr-->>cli: show no-import available
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes flags to call out:
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Reintroduces and aligns the OpenCode account-pool import flow with the current restore/import contracts, updating detection, CLI recovery menu wiring, tests, and onboarding docs.
Changes:
- Add OpenCode account pool path detection + restore-style assessment helpers in storage.
- Add a Recovery menu action to preview/confirm importing OpenCode accounts via CLI.
- Extend storage/CLI tests and update getting-started guidance for the recovery import path.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/storage.test.ts | Adds detection/assessment coverage for OpenCode pool sources and malformed inputs. |
| test/codex-manager-cli.test.ts | Adds CLI flow coverage for the “import-opencode” recovery path. |
| lib/ui/copy.ts | Adds UI copy label for the new recovery menu item. |
| lib/ui/auth-menu.ts | Adds a new AuthMenuAction and menu item for importing OpenCode accounts. |
| lib/storage.ts | Implements pool-path detection and assessment built on existing restore-candidate logic. |
| lib/codex-manager.ts | Wires the new recovery import action into the login dashboard flow. |
| lib/cli.ts | Extends LoginMode + prompt mapping to include “import-opencode”. |
| docs/getting-started.md | Documents the recovery import option for companion/OpenCode pools. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/storage.test.ts
Outdated
| delete process.env.CODEX_OPENCODE_POOL_PATH; | ||
| else process.env.CODEX_OPENCODE_POOL_PATH = originalPoolPath; | ||
| setStoragePathDirect(null); | ||
| await fs.rm(tempRoot, { recursive: true, force: true }); |
lib/codex-manager.ts
Outdated
| await runActionPanel( | ||
| "Import OpenCode Accounts", | ||
| `Importing from ${assessment.backup.path}`, | ||
| async () => { | ||
| const imported = await importAccounts(assessment.backup.path); | ||
| console.log( | ||
| `Imported ${imported.imported} account${imported.imported === 1 ? "" : "s"}. Skipped ${imported.skipped}. Total accounts: ${imported.total}.`, | ||
| ); | ||
| }, | ||
| displaySettings, | ||
| ); |
lib/codex-manager.ts
Outdated
| `Import OpenCode accounts from ${assessment.backup.path}?`, | ||
| ); | ||
| if (!confirmed) { | ||
| continue; | ||
| } | ||
| await runActionPanel( | ||
| "Import OpenCode Accounts", | ||
| `Importing from ${assessment.backup.path}`, |
docs/getting-started.md
Outdated
|
|
||
| The restore manager shows each backup name, account count, freshness, and whether the restore would exceed the account limit before it lets you apply anything. | ||
|
|
||
| If you already have a companion app pool on the same machine, use the recovery import option from the login dashboard to preview and import those saved accounts before creating new OAuth sessions. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai resolve |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/getting-started.md`:
- Around line 85-86: Update the sentence in docs/getting-started.md to
explicitly name the dashboard item "Import OpenCode Accounts" (so users can find
it in the login dashboard), and add companion documentation changes:
create/modify upgrade notes documenting this behavior change and list any new or
changed npm scripts (if any were introduced) so README, SECURITY, and docs/**
remain consistent with the CLI flags and workflows; reference the dashboard item
"Import OpenCode Accounts" and the upgrade notes and npm scripts sections when
making the edits.
In `@lib/cli.ts`:
- Around line 65-66: The fallback flow is missing a branch to parse the new
"import-opencode" interactive option, so update promptLoginModeFallback in
lib/cli.ts to accept and map both "i" and "import-opencode" (and
full-word/short-letter variants like "I"/"import-opencode") to the same login
mode value used by the interactive menu; then add a vitest regression that calls
promptLoginModeFallback with inputs "i" and "import-opencode" (and optionally
"I") asserting the returned mode equals the expected import-opencode mode.
Ensure any other parsing helper used by promptLoginModeFallback is updated
consistently (e.g., the same enum/constant used by the interactive menu) so
fallback and interactive paths produce identical results.
In `@lib/codex-manager.ts`:
- Around line 4375-4383: After calling importAccounts inside the runActionPanel
callback (the block using runActionPanel and importAccounts with
assessment.backup.path), ensure the Codex auth/active account selection is
refreshed: after the import completes and before finishing the action, call the
module/method that updates Codex auth state (e.g., a function like
syncCodexActiveSelection, refreshCodexAuthState, or whatever existing Codex
manager method you have) so the active Codex account reflects newly imported
accounts and the UI/auth state is refreshed; if no such helper exists, add one
that reads current account storage and sets the active selection accordingly and
invoke it right after the imported result is logged.
- Around line 4374-4389: The import flow calls importAccounts from
lib/codex-manager.ts inside runActionPanel without handling transient filesystem
locks (EBUSY/EPERM) that can be raised by fs.readFile in lib/storage.ts; add
retry logic around the read in importAccounts (or wrap the importAccounts call
here) to retry on EBUSY/EPERM with a small backoff and limited attempts, and add
a vitest regression in test/codex-manager-cli.test.ts that simulates fs.readFile
throwing EBUSY/EPERM once (then succeeding) to assert the import still
completes; reference the importAccounts function and the fs.readFile behavior in
lib/storage.ts when implementing the retry and update tests accordingly.
In `@lib/storage.ts`:
- Around line 2072-2099: The detectOpencodeAccountPoolPath function adds
candidate paths from appDataBases and homedir without normalizing them, causing
cwd-dependent behavior for relative APPDATA/LOCALAPPDATA values; update
detectOpencodeAccountPoolPath to resolve each candidate before adding/probing
(use resolvePath on values passed to join for entries added from appDataBases
and the homedir entry that uses ACCOUNTS_FILE_NAME), keep the existing explicit
env resolution logic, and add a Vitest regression that sets relative
APPDATA/LOCALAPPDATA (and a relative homedir) to verify the function returns the
resolved absolute path when the target file exists; reference
detectOpencodeAccountPoolPath, ACCOUNTS_FILE_NAME, resolvePath, join, and
existsSync when updating code and tests.
In `@test/codex-manager-cli.test.ts`:
- Around line 1018-1146: Add two deterministic regression tests to
test/codex-manager-cli.test.ts targeting runCodexMultiAuthCli: one that mocks
importAccountsMock to throw filesystem errors with error.code set to "EBUSY" and
"EPERM" (use mockRejectedValueOnce(new Error("...")) then set the code property)
and assert the CLI handles them like other login paths (exit code 0,
importAccountsMock called, and an error message containing "Import failed" or
similar via console.error spy); and a second that reproduces the in-flight
concurrency race by making importAccountsMock return a promise that first
rejects with an "in-flight" error (or a rejection) and then resolves on retry
(e.g., mockRejectedValueOnce then mockResolvedValueOnce) and assert
runCodexMultiAuthCli returns to the dashboard (exit 0), promptLoginModeMock
called the expected number of times, and importAccountsMock was
retried—reference the existing mocks assessOpencodeAccountPoolMock,
importAccountsMock, promptLoginModeMock, confirmMock, and the
runCodexMultiAuthCli invocation to place these new tests consistent with the
surrounding tests.
- Around line 1169-1229: The parameterized test currently invokes "auth
restore-backup" instead of exercising the fresh/reset actions and then checking
that a subsequent "auth login" does not show the startup restore prompt; update
the test to call runCodexMultiAuthCli with the deliberate action under test (use
the existing action mocks deleteSavedAccountsMock or resetLocalStateMock via
runCodexMultiAuthCli(["auth", "<appropriate-action>"]) or call the helper that
triggers those code paths), then invoke runCodexMultiAuthCli(["auth", "login"])
in the same session and assert promptLoginModeMock was not called and
confirmMock was called once (instead of running
restore-backup/restoreNamedBackupMock). Reference runCodexMultiAuthCli,
deleteSavedAccountsMock, resetLocalStateMock, promptLoginModeMock, confirmMock
to locate and change the two-step flow in the test so it matches the test title.
In `@test/storage.test.ts`:
- Around line 4794-4840: Add a deterministic race-regression test that simulates
the file changing between detection and assessment: after writing a valid pool
JSON to poolPath and calling detectOpencodeAccountPoolPath() (to verify
detection still returns poolPath), immediately mutate the file (e.g., overwrite
poolPath with "not valid json" or unlink it) before calling
assessOpencodeAccountPool(), then assert assessOpencodeAccountPool() reports
backup.valid === false, eligibleForRestore === false, imported === null and that
loadAccounts() remains unchanged; use the same helpers
detectOpencodeAccountPoolPath, assessOpencodeAccountPool, poolPath and
loadAccounts, await fs promises to keep the test deterministic under vitest and
perform the atomic overwrite/unlink between detect and assess to reproduce the
race.
- Around line 4769-4818: Add deterministic Windows-specific regression tests:
add one test where process.env.LOCALAPPDATA is deleted (or set to undefined),
process.env.APPDATA is set to tempRoot and platform pinned to Windows if your
test harness supports it, then write the pool file and assert
detectOpencodeAccountPoolPath() returns the APPDATA-derived pool path and
assessOpencodeAccountPool() reports valid/eligible/imported; add another test
that sets process.env.CODEX_OPENCODE_POOL_PATH to an explicit custom path
(different from APPDATA/LOCALAPPDATA), write the pool file there, and assert
detectOpencodeAccountPoolPath() prefers that path and
assessOpencodeAccountPool() uses it. Reference symbols:
detectOpencodeAccountPoolPath, assessOpencodeAccountPool,
CODEX_OPENCODE_POOL_PATH, poolPath, tempRoot.
🪄 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: d19acb00-4c4d-427e-932a-354039727a90
📒 Files selected for processing (8)
docs/getting-started.mdlib/cli.tslib/codex-manager.tslib/storage.tslib/ui/auth-menu.tslib/ui/copy.tstest/codex-manager-cli.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). (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.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/cli.tslib/ui/auth-menu.tslib/ui/copy.tslib/codex-manager.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/storage.test.tstest/codex-manager-cli.test.ts
🔇 Additional comments (7)
lib/ui/copy.ts (1)
19-19: good menu copy addition
lib/ui/copy.ts:19(line 19) cleanly adds the recovery label and matches the new action id used elsewhere.lib/ui/auth-menu.ts (1)
73-73: action wiring is consistent
lib/ui/auth-menu.ts:73,lib/ui/auth-menu.ts:528, andlib/ui/auth-menu.ts:694(line 73, line 528, line 694) correctly wire the new action through type union, focus identity, and recovery menu rendering.Also applies to: 528-528, 694-698
lib/storage.ts (1)
2469-2496: assessment flow reuse looks solid
lib/storage.ts:2469(line 2469) reusesloadBackupCandidateandassessNamedBackupRestoreCandidate, so dedupe/limit/preview behavior stays aligned with existing restore contracts. no new concurrency risk is introduced in this path.test/storage.test.ts (1)
16-23: imports are aligned with the new storage api surface.good update in
test/storage.test.ts:16-23; the new tests consume both additions cleanly.test/codex-manager-cli.test.ts (1)
17-19: nice wiring for the new storage api mocks.the new
assessOpencodeAccountPoolandimportAccountsmocks are exported and reset consistently, which keeps this suite isolated and deterministic. test/codex-manager-cli.test.ts:17-19, test/codex-manager-cli.test.ts:146-148, test/codex-manager-cli.test.ts:643-684Also applies to: 146-148, 643-684
lib/codex-manager.ts (2)
65-91: import and type wiring looks consistent with current contracts.
lib/codex-manager.ts:65-91andlib/codex-manager.ts:103-107are clean: new storage/settings symbols are correctly imported and used.Also applies to: 103-107
4759-4761: type alias migration is clean and keeps the local api stable.
lib/codex-manager.ts:4759-4761correctly maps to storage metadata/assessment types without changing downstream usage.
ndycode
force-pushed
the
fresh/18-import-adapter-opencode
branch
from
704af0a to
e6e235a
Compare
ndycode
force-pushed
the
fresh/18-import-adapter-opencode
branch
from
5bd459c to
3b1ba04
Compare
| if (assessment.wouldExceedLimit) { | ||
| console.log( | ||
| `Import would exceed the account limit (${assessment.currentAccountCount ?? "?"} current, ${assessment.mergedAccountCount ?? "?"} after import). Remove accounts first.`, | ||
| ); | ||
| continue; | ||
| } | ||
| if (menuResult.mode === "verify-flagged") { | ||
| await runActionPanel("Problem Account Check", "Checking problem accounts", async () => { | ||
| await runVerifyFlagged([]); | ||
| }, displaySettings); | ||
| const backupLabel = basename(assessment.backup.path); |
There was a problem hiding this comment.
wouldExceedLimit check is unreachable dead code
assessNamedBackupRestoreCandidate sets eligibleForRestore = hasRestorableChanges, where hasRestorableChanges = !wouldExceedLimit && (...). this means whenever wouldExceedLimit is true, eligibleForRestore is always false. the guard at line 4451 (!assessment.eligibleForRestore) therefore catches every limit-exceeded case and continues before this block is ever tested.
the more informative message with current/merged counts here is dead code — users instead see assessment.error = "Restore would exceed maximum of N accounts" from assessNamedBackupRestoreCandidate, which omits the currentAccountCount and mergedAccountCount context.
to fix this and surface the counts, swap the check order or inline the limit message into the eligibility guard:
if (!assessment.backup.valid) {
const assessmentErrorLabel =
assessment.error || "OpenCode account pool is not importable.";
console.log(assessmentErrorLabel);
continue;
}
if (assessment.wouldExceedLimit) {
console.log(
`Import would exceed the account limit (${assessment.currentAccountCount ?? "?"} current, ${assessment.mergedAccountCount ?? "?"} after import). Remove accounts first.`,
);
continue;
}
if (!assessment.eligibleForRestore) {
const assessmentErrorLabel =
assessment.error || "OpenCode account pool is not importable.";
console.log(assessmentErrorLabel);
continue;
}Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 4457-4463
Comment:
**`wouldExceedLimit` check is unreachable dead code**
`assessNamedBackupRestoreCandidate` sets `eligibleForRestore = hasRestorableChanges`, where `hasRestorableChanges = !wouldExceedLimit && (...)`. this means whenever `wouldExceedLimit` is `true`, `eligibleForRestore` is always `false`. the guard at line 4451 (`!assessment.eligibleForRestore`) therefore catches every limit-exceeded case and `continue`s before this block is ever tested.
the more informative message with current/merged counts here is dead code — users instead see `assessment.error = "Restore would exceed maximum of N accounts"` from `assessNamedBackupRestoreCandidate`, which omits the `currentAccountCount` and `mergedAccountCount` context.
to fix this and surface the counts, swap the check order or inline the limit message into the eligibility guard:
```typescript
if (!assessment.backup.valid) {
const assessmentErrorLabel =
assessment.error || "OpenCode account pool is not importable.";
console.log(assessmentErrorLabel);
continue;
}
if (assessment.wouldExceedLimit) {
console.log(
`Import would exceed the account limit (${assessment.currentAccountCount ?? "?"} current, ${assessment.mergedAccountCount ?? "?"} after import). Remove accounts first.`,
);
continue;
}
if (!assessment.eligibleForRestore) {
const assessmentErrorLabel =
assessment.error || "OpenCode account pool is not importable.";
console.log(assessmentErrorLabel);
continue;
}
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/codex-manager.ts (1)
3386-3497:⚠️ Potential issue | 🟠 Majornew doctor checks for wal, rotating backups, and snapshots lack test coverage
the extended
runDoctordiagnostics atlib/codex-manager.ts:3386-3497add four new checks:storage-journal,rotating-backups,snapshot-backups, andrecovery-chain. whiletest/codex-manager-cli.test.ts:4153+tests the doctor command generally, there are no test assertions for these four new check keys. per coding guidelines,lib/**changes must cite affected tests. add vitest cases that exercise each new check path, including the valid/invalid backup scenarios and the recovery chain logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 3386 - 3497, The new doctor checks added in runDoctor (keys: "storage-journal", "rotating-backups", "snapshot-backups", "recovery-chain") lack unit tests; add vitest cases that mock the environment and the helper functions listRotatingBackups and listAccountSnapshots to exercise all branches (wal present vs missing; rotating backups: none / valid / invalid; snapshots: none / valid / invalid) and verify the returned checks array contains entries with those keys and the expected severity/message combinations. Specifically, in the existing doctor CLI tests add stubs for existsSync/fs.stat and for listRotatingBackups/listAccountSnapshots to produce controlled valid/invalid results, call the function that runs the diagnostics (runDoctor or the CLI entry used in tests), and assert the presence and severity strings of "storage-journal", "rotating-backups", "snapshot-backups", and "recovery-chain" for each scenario. Ensure you cover: (1) no artifacts => warn severities, (2) only WAL/storage present => storage-journal ok/recovery-chain ok, (3) valid rotating backups => rotating-backups ok, (4) invalid rotating backups => rotating-backups error, (5) valid snapshots => snapshot-backups ok, and (6) combinations that exercise recovery-chain ok vs warn.lib/storage.ts (1)
3742-3791:⚠️ Potential issue | 🟠 Major
clearFlaggedAccounts()leaves the legacy blocked file behind.
lib/storage.ts:3759-3791only deletes the current flagged storage path and its backups.loadFlaggedAccounts()still migratesgetLegacyFlaggedAccountsPath()back into place atlib/storage.ts:3657-3680, solib/destructive-actions.ts:189-200can reportflaggedCleared: trueand the next startup resurrects the old flagged pool for upgraded installs. please include the legacy blocked file in the deletion set and add a vitest regression intest/storage.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."proposed fix
const backupPaths = await getAccountsBackupRecoveryCandidatesWithDiscovery(path); + const legacyPath = getLegacyFlaggedAccountsPath(); let hadError = false; - for (const candidate of [path, ...backupPaths]) { + for (const candidate of [path, legacyPath, ...backupPaths]) { try { await unlinkWithRetry(candidate);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 3742 - 3791, clearFlaggedAccounts() currently deletes getFlaggedAccountsPath() and backups but omits getLegacyFlaggedAccountsPath(), so loadFlaggedAccounts() can re-migrate the legacy file and resurrect flagged accounts; update clearFlaggedAccounts() to include getLegacyFlaggedAccountsPath() in the deletion candidate list (alongside path and backupPaths) and use unlinkWithRetry/error handling consistent with the existing loop, ensure marker removal logic remains the same, and add a vitest regression in test/storage.test.ts that verifies the legacy blocked file is removed and that lib/destructive-actions.ts reporting (flaggedCleared) behaves correctly after deletion.
♻️ Duplicate comments (2)
test/storage.test.ts (2)
4919-4952:⚠️ Potential issue | 🟠 Majorthe win32
appdatafallback is still uncovered.
test/storage.test.ts:line 4919covers the homedir path, but there is still no deterministic vitest case forprocess.platform === "win32"withlocalappdataunset andappdataset. that is the windows-specific fallback most likely to regress indetectOpencodeAccountPoolPath(), and this suite will not catch it. please add a case that pins win32, deleteslocalappdata, writes the pool underappdata, and asserts both detection andassessOpencodeAccountPool()use that path. 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 4919 - 4952, Add a deterministic vitest case exercising the Windows-appdata fallback: create a test that temporarily sets process.platform to "win32" (using Object.defineProperty) deletes process.env.LOCALAPPDATA, sets process.env.APPDATA to a fake temp folder, writes the openai-codex-accounts.json pool file at join(process.env.APPDATA, "OpenCode" or the expected path used by detectOpencodeAccountPoolPath()), then assert detectOpencodeAccountPoolPath() returns that APPDATA path and assessOpencodeAccountPool() reports assessment.backup.path equal to it and eligibleForRestore/imported as expected; ensure you clean up/reset process.platform and environment vars after the test to keep tests deterministic.
4999-5009:⚠️ Potential issue | 🟠 Majorthe detect-to-assess race is still missing coverage.
test/storage.test.ts:line 5002andtest/storage.test.ts:line 5005validate malformed input at rest, but they do not cover the more important race where detection succeeds and the file is deleted or corrupted before assessment. that leaves the opencode import path unprotected against the same windows lock/move race this pr is trying to tolerate. add a vitest regression that mutates or unlinkspoolPathafterdetectOpencodeAccountPoolPath()and beforeassessOpencodeAccountPool(), then asserts current storage stays unchanged. 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 4999 - 5009, Add a deterministic vitest regression to the "refuses malformed opencode source before any mutation" test that simulates the detect-to-assess race by calling detectOpencodeAccountPoolPath(), then asynchronously mutating/unlinking poolPath (e.g., await fs.unlink(poolPath) or overwrite it) before calling assessOpencodeAccountPool(), and finally asserting that assessOpencodeAccountPool() returns the expected failure (assessment?.backup.valid false, eligibleForRestore false, imported null) and that current storage/state did not change; use the existing symbols detectOpencodeAccountPoolPath, assessOpencodeAccountPool, and poolPath, ensure the mutation occurs between the two calls (use a microtask or explicit await to order operations), and clean up/reset poolPath after the test to keep it deterministic.
🤖 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 359-364: captureRollbackSnapshot currently returns null when
snapshotAccountStorage yields no accounts, causing first manual sync to have no
rollback; change captureRollbackSnapshot (and where it sets rollbackSnapshot
around line 1040) to persist an explicit empty checkpoint or a sentinel rollback
action (e.g., "clearLocalStorage" marker) when snapshotAccountStorage() returns
null so getLatestCodexCliSyncRollbackPlan can restore an empty state; update
tests in test/codex-cli-sync.test.ts to cover the first-sync-empty-pool case and
adjust test/storage.test.ts expectations accordingly, and ensure any new
sentinel logging follows guidelines (no leaking tokens/emails), and that new
persistence logic handles Windows IO and concurrency error cases (EBUSY/429) and
integrates with existing auth-rotation paths.
In `@lib/codex-manager.ts`:
- Around line 124-126: The boolean guard destructiveActionInFlight has a TOCTOU
race; replace it with an async mutex instead of reading/then-writing the flag.
Import and instantiate a mutex (e.g., Mutex from the async-mutex package) at the
same scope where destructiveActionInFlight is declared, then wrap the critical
destructive sections (the code paths that currently check/set
destructiveActionInFlight around the checks referenced by
destructiveActionInFlight) in mutex.runExclusive()/acquire()/release() so only
one async invocation can enter the destructive block at a time; remove direct
reads/writes of destructiveActionInFlight and use the mutex to control exclusive
access.
- Around line 4288-4298: Add a unit test that exercises
resolveStartupRecoveryAction when the recoveryState object contains a non-empty
assessments array: construct a mock Awaited<ReturnType<typeof
getActionableNamedBackupRestores>> with assessments.length > 0 (other fields can
be defaults), call resolveStartupRecoveryAction(recoveryState,
recoveryScanFailed) with recoveryScanFailed true and/or false, and assert the
return value is "show-recovery-prompt"; place this new test alongside the
existing recovery tests so the branch for resolveStartupRecoveryAction that
returns "show-recovery-prompt" is covered.
In `@lib/storage.ts`:
- Around line 3381-3386: The snapshot captures storagePath via getStoragePath()
but persist/rollback still re-read global mutable storage state (e.g.,
setStoragePath / setStoragePathDirect), so thread the pinned storagePath through
the save helpers and transaction commit/rollback paths instead of relying on
async-local state: update persist(), the save helpers invoked by persist (e.g.,
functions that call loadAccountsInternal, exportAccounts, and any saveAccounts*
helpers) to accept and use an explicit storagePath parameter, ensure rollback
uses that pinned path, and stop re-reading getStoragePath()/global state inside
those helpers; add a vitest that starts a transaction, flips the global storage
target between start and persist, then asserts the original pinned path was used
for persist/rollback, and reference the modified symbols (persist,
exportAccounts, loadAccountsInternal, setStoragePath/setStoragePathDirect) in
the test.
- Around line 2851-2855: equalsResolvedStoragePath currently compares raw
strings (only lowercasing on Windows), which misses cases when one side is
relative; update equalsResolvedStoragePath to resolve/normalize both operands
(use path.resolve or fs.realpathSync to get absolute canonical paths) before
comparing and still perform platform-specific case normalization on the resolved
values; ensure callers assessOpencodeAccountPool and importAccounts that pass
absolute paths will match targets set via setStoragePathDirect when given
relative values, and add a vitest that calls setStoragePathDirect with a
relative path then runs importAccounts/assessOpencodeAccountPool to assert the
self-import block triggers.
- Around line 3318-3329: After committing the new storage file with
renameFileWithRetry, do not swallow transient unlink failures for the reset
marker: replace the best-effort fs.unlink(resetMarkerPath) (and likewise the
walPath unlink) with unlinkWithRetry(resetMarkerPath) so transient EBUSY/EPERM
retries are attempted and a real failure is surfaced; update the block around
renameFileWithRetry/lastAccountsSaveTimestamp to call unlinkWithRetry for
resetMarkerPath (and walPath) and preserve existing logging/context, and add a
vitest in test/storage.test.ts that simulates an unlink failure after a
successful save/import to assert unlinkWithRetry is used and the stale reset
marker is removed (or the error is retried/propagated) to prevent the marker
from hiding the just-written accounts.
In `@test/storage.test.ts`:
- Around line 2385-2394: The test "should migrate from old filename to new
filename" currently contains a placeholder assertion (expect(true).toBe(true))
and doesn't exercise the migration; replace it with a deterministic end-to-end
assertion that verifies the actual migration behavior: create a test fixture
where a file named "openai-codex-accounts.json" (oldName) exists and
"codex-accounts.json" (newName) does not, call the code path that triggers
migration (e.g., invoke loadAccounts() or the function that uses
getStoragePath()), then assert that the migration occurred by checking that
"codex-accounts.json" now exists (or that loadAccounts() returns the expected
accounts) and that "openai-codex-accounts.json" was removed or ignored; if
migration behavior isn't implemented yet, remove the placeholder test instead of
leaving expect(true).toBe(true) so the suite remains meaningful.
---
Outside diff comments:
In `@lib/codex-manager.ts`:
- Around line 3386-3497: The new doctor checks added in runDoctor (keys:
"storage-journal", "rotating-backups", "snapshot-backups", "recovery-chain")
lack unit tests; add vitest cases that mock the environment and the helper
functions listRotatingBackups and listAccountSnapshots to exercise all branches
(wal present vs missing; rotating backups: none / valid / invalid; snapshots:
none / valid / invalid) and verify the returned checks array contains entries
with those keys and the expected severity/message combinations. Specifically, in
the existing doctor CLI tests add stubs for existsSync/fs.stat and for
listRotatingBackups/listAccountSnapshots to produce controlled valid/invalid
results, call the function that runs the diagnostics (runDoctor or the CLI entry
used in tests), and assert the presence and severity strings of
"storage-journal", "rotating-backups", "snapshot-backups", and "recovery-chain"
for each scenario. Ensure you cover: (1) no artifacts => warn severities, (2)
only WAL/storage present => storage-journal ok/recovery-chain ok, (3) valid
rotating backups => rotating-backups ok, (4) invalid rotating backups =>
rotating-backups error, (5) valid snapshots => snapshot-backups ok, and (6)
combinations that exercise recovery-chain ok vs warn.
In `@lib/storage.ts`:
- Around line 3742-3791: clearFlaggedAccounts() currently deletes
getFlaggedAccountsPath() and backups but omits getLegacyFlaggedAccountsPath(),
so loadFlaggedAccounts() can re-migrate the legacy file and resurrect flagged
accounts; update clearFlaggedAccounts() to include
getLegacyFlaggedAccountsPath() in the deletion candidate list (alongside path
and backupPaths) and use unlinkWithRetry/error handling consistent with the
existing loop, ensure marker removal logic remains the same, and add a vitest
regression in test/storage.test.ts that verifies the legacy blocked file is
removed and that lib/destructive-actions.ts reporting (flaggedCleared) behaves
correctly after deletion.
---
Duplicate comments:
In `@test/storage.test.ts`:
- Around line 4919-4952: Add a deterministic vitest case exercising the
Windows-appdata fallback: create a test that temporarily sets process.platform
to "win32" (using Object.defineProperty) deletes process.env.LOCALAPPDATA, sets
process.env.APPDATA to a fake temp folder, writes the openai-codex-accounts.json
pool file at join(process.env.APPDATA, "OpenCode" or the expected path used by
detectOpencodeAccountPoolPath()), then assert detectOpencodeAccountPoolPath()
returns that APPDATA path and assessOpencodeAccountPool() reports
assessment.backup.path equal to it and eligibleForRestore/imported as expected;
ensure you clean up/reset process.platform and environment vars after the test
to keep tests deterministic.
- Around line 4999-5009: Add a deterministic vitest regression to the "refuses
malformed opencode source before any mutation" test that simulates the
detect-to-assess race by calling detectOpencodeAccountPoolPath(), then
asynchronously mutating/unlinking poolPath (e.g., await fs.unlink(poolPath) or
overwrite it) before calling assessOpencodeAccountPool(), and finally asserting
that assessOpencodeAccountPool() returns the expected failure
(assessment?.backup.valid false, eligibleForRestore false, imported null) and
that current storage/state did not change; use the existing symbols
detectOpencodeAccountPoolPath, assessOpencodeAccountPool, and poolPath, ensure
the mutation occurs between the two calls (use a microtask or explicit await to
order operations), and clean up/reset poolPath after the test to keep it
deterministic.
🪄 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: 95975d5e-055b-401a-b6ad-b5ccf87855f2
📒 Files selected for processing (12)
docs/getting-started.mdlib/cli.tslib/codex-cli/sync.tslib/codex-manager.tslib/storage.tslib/ui/auth-menu.tslib/ui/copy.tstest/cli.test.tstest/codex-cli-sync.test.tstest/codex-manager-cli.test.tstest/documentation.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). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
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/documentation.test.tstest/cli.test.tstest/codex-cli-sync.test.tstest/storage.test.ts
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.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/ui/auth-menu.tslib/cli.tslib/codex-cli/sync.tslib/codex-manager.tslib/ui/copy.tslib/storage.ts
🪛 LanguageTool
docs/getting-started.md
[style] ~77-~77: To make your writing flow more naturally, try moving the adverb ‘already’ closer to the verb ‘named’.
Context: ...rt Fresh Use the restore path when you already have named backup files and want to recover accoun...
(PERF_TENS_ADV_PLACEMENT)
🔇 Additional comments (36)
docs/getting-started.md (4)
85-86: docs should name the exact dashboard itemline 85 says "recovery import option from the login dashboard" but doesn't name the menu item
Import OpenCode Accounts. users scanning the dashboard won't find "recovery import option" anywhere. also, per coding guidelines, behavior changes require companion upgrade notes — verifydocs/upgrade.mddocuments this new import flow.as per coding guidelines, "docs/**: keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts."
#!/bin/bash # check if upgrade.md mentions import-opencode or opencode pool rg -i 'import.?opencode|opencode.?pool|CODEX_OPENCODE_POOL_PATH' docs/upgrade.md
52-53: lgtm — recovery prompt suppression documentedthe conditional restore prompt behavior and suppression rules are clearly documented for interactive vs non-interactive flows.
75-84: restore path documentation looks goodthe backup location, automatic vs manual paths, and restore manager behavior are documented consistently with the referenced coding guidelines for
docs/reference/storage-paths.md.
89-98: sync and settings taxonomy documented correctlythe split between "Everyday Settings" and "Advanced & Operator" aligns with the ui copy changes in
lib/ui/copy.tsand the settings reference structure.test/documentation.test.ts (3)
169-172: opencode allowlist expansion looks correctadding
docs/getting-started.mdto the allowlist aligns with the new import-opencode documentation in that file.
291-324: settings copy assertions lock in the new ui taxonomythese assertions provide regression coverage for the renamed labels (
List Appearance,Details Line,Results & Refresh,Colors) and their hint strings. good coverage for preventing accidental copy drift.
345-365: section alignment assertions updated correctlythe switch from
##to###heading assertions matches the new menu hierarchy documented in the pr summary.test/cli.test.ts (3)
176-217: reset mode and cancellation tests look solidthese tests cover the typed
RESETconfirmation and the cancellation message"\nReset local state cancelled.\n". good regression coverage for the new destructive action flow.
219-266: tty reset-all path test correctly mocks auth-menuthe test properly mocks
isTTYandshowAuthMenuto simulate the interactive menu returning{ type: "reset-all" }. the typedRESETconfirmation is asserted.one note:
FORCE_INTERACTIVE_MODE=1is set inbeforeEach(line 16), soisNonInteractiveMode()already returns false. thestdin/stdout.isTTYoverrides are redundant here but don't break the test — they become relevant ifFORCE_INTERACTIVE_MODEwere cleared.
707-743: fallback aliases for restore-backup and import-opencode have regression coveragethese tests validate that
u,restore,backup,restore-backupmap torestore-backupmode, andi,import-opencodemap toimport-opencodemode. this addresses the past review concern about missing fallback parsing for import-opencode.lib/ui/auth-menu.ts (4)
52-55: new read-only row type for health summary
AuthMenuReadOnlyRowprovides a clean interface for the disabled health summary heading in the menu.
92-93: precompiled regex patterns improve sanitization performancemoving
ANSI_ESCAPE_PATTERNandCONTROL_CHAR_PATTERNto module scope avoids recompiling on everysanitizeTerminalTextcall.
616-633: health summary row insertion gated correctlythe health summary heading and disabled row are only inserted when
healthSummaryLabelis truthy. the separator placement looks correct.
688-719: recovery and danger-zone menu items wired correctly
import-opencodeandrestore-backupare yellow (recovery),delete-allandreset-allare red (danger). the action types align with the extendedAuthMenuActionunion.lib/cli.ts (4)
30-32: interactive menu availability check consolidates tty logic
isInteractiveLoginMenuAvailable()correctly combines!isNonInteractiveMode()andisTTY()checks. this aligns with context snippet 1 showing the full gate.
169-179: promptResetTypedConfirm follows the same pattern as delete-alluses
DESTRUCTIVE_ACTION_COPY.resetLocalState.typedConfirmand expects exactRESETinput. readline is properly closed in finally block.
244-254: fallback parsing covers restore-backup and import-opencode aliases
u,backup,restore,restore-backup→restore-backupmode.i,import-opencode→import-opencodemode. this addresses the past review concern.
298-313: interactive reset-all and import flows handled correctly
reset-alltriggers typedRESETconfirmation before returning{ mode: "reset" }.import-opencodeandrestore-backupreturn immediately since they trigger their own flows downstream.test/codex-cli-sync.test.ts (7)
80-91: test setup correctly isolates sync history and storage path
configureSyncHistoryForTestsand thegetStoragePathmock ensure each test uses isolated state.__resetLastCodexCliSyncRunForTests()clears in-memory run state.
270-350: preview test validates no canonical storage mutationthe
JSON.parse(JSON.stringify(current))snapshot before preview andexpect(current).toEqual(snapshot)after confirms preview doesn't mutate. the secret-free serialization assertions at 344-348 are a good privacy check.
883-979: ebusy/eperm retry exhaustion tests provide windows regression coveragethe
it.each(["EBUSY", "EPERM"])tests mockfs.promises.statto throw the error code on every call totargetStoragePath, then asserttargetStatCalls === SELECTION_TIMESTAMP_READ_MAX_ATTEMPTS. this validates the retry loop atlib/codex-cli/sync.ts:630-660exhausts all attempts before returning null.
1067-1178: exhausted retries log test validates observabilitythe test re-mocks
createLoggerto capture debug calls, then verifies the"Exhausted retries reading persisted local selection timestamp"message is logged with the error. good coverage for debugging windows filesystem issues.
1578-1686: rollback serialization test validates concurrent write orderingthe
withAccountStorageTransactionmock injects a concurrentsaveAccountscall during the transaction, then asserts the final state reflects the concurrent write. this validates that rollback doesn't clobber concurrent modifications.
2130-2189: concurrent apply isolation test validates distinct pending runs
Promise.all([applyCodexCliSyncToStorage(current), applyCodexCliSyncToStorage(current)])returns two results with distinctpendingRun.revisionvalues. this prevents double-commit bugs when callers race.
2242-2309: revision ordering test validates last-to-finish winsthe test commits a failure for the second run, then commits success for the first run, and asserts the first run's
changedoutcome is the finalgetLastCodexCliSyncRun()result. this validates that later completions overwrite earlier failures.lib/codex-manager.ts (11)
4470-4486: opencode import still missing retry and codex sync (prior feedback).as flagged in prior reviews:
importAccountsatlib/codex-manager.ts:4475can hitEBUSY/EPERMon windows if the opencode pool file is locked. no retry logic wraps this call.- after a successful import, the codex active selection is not synced. users may need to manually switch or log in again.
per coding guidelines: "verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios."
3283-3289: good pattern for token fingerprinting without leakage.using a truncated sha256 hash to detect duplicates keeps the raw refresh token out of memory comparisons and logs. looks good.
4163-4197: defensive error handling for health summary components looks good.each health summary component (restores, rollback, sync, doctor) fails independently with redacted error labels via
logLoginMenuHealthSummaryWarning. this keeps the login menu resilient when secondary data sources fail.
4576-4604: startup recovery scan error handling is defensive and appropriate.if
getActionableNamedBackupRestoresfails (e.g.,EPERMon windows), the code logs a redacted warning and continues with oauth. this prevents a filesystem glitch from blocking login entirely.
5252-5292: chunked concurrent assessment with per-entry error handling looks solid.the loop at
lib/codex-manager.ts:5254-5292usesNAMED_BACKUP_LIST_CONCURRENCYto bound concurrentassessNamedBackupRestorecalls. each failure is caught, logged withnormalizeBackupAssessmentError(no sensitive data), and returns a fallback entry. this keeps the backup browser resilient against individual file failures.
5161-5225: re-assessment before restore prevents stale preview data.
runBackupRestorePreviewre-callsassessNamedBackupRestoreatlib/codex-manager.ts:5168before showing the restore confirmation. this ensures the preview reflects current storage state rather than a potentially stale cached assessment.
4493-4510: try-finally cleanup fordestructiveActionInFlightis correct.the guard is properly cleared in
finallyblocks atlib/codex-manager.ts:4508-4510andlib/codex-manager.ts:4540-4542, ensuring it resets even if the action throws. the toctou issue is noted separately.Also applies to: 4519-4542
4100-4106: error redaction before logging prevents filesystem path leakage.
logLoginMenuHealthSummaryWarningatlib/codex-manager.ts:4100-4106usesgetRedactedFilesystemErrorLabelto strip potentially sensitive filesystem details before logging.
4438-4446: import assessment error handling is graceful with redacted output.if
assessOpencodeAccountPoolfails atlib/codex-manager.ts:4439, the error is formatted viaformatRedactedFilesystemErrorbefore logging, preventing sensitive path details from leaking to the console.
5494-5498:restore-backupcli command wiring is straightforward.the new command at
lib/codex-manager.ts:5494-5498delegates torunBackupBrowserManagerand returns exit code 1 only on explicit "failed" result. clean wiring.
4302-4303: add regression test for recovery prompt suppression after destructive actions.
suppressRecoveryPromptguard at lib/codex-manager.ts:4511 (delete-all) and 4543 (reset) prevents recovery prompt when storage is cleared. tests at test/codex-manager-cli.test.ts:7153 and 7185 execute destructive actions then immediately cancel, never validating the suppression works. add test that runs delete-all or reset, then loops back to login menu with no accounts and actionable backups available, asserting recovery prompt is NOT offered.
| function publishCodexCliSyncRun( | ||
| run: CodexCliSyncRun, | ||
| revision: number, | ||
| ): boolean { | ||
| if (revision <= lastCodexCliSyncRunRevision) { | ||
| return false; | ||
| } | ||
| lastCodexCliSyncRunRevision = revision; | ||
| lastCodexCliSyncRun = cloneCodexCliSyncRun(run); | ||
| void appendSyncHistoryEntry({ | ||
| kind: "codex-cli-sync", | ||
| recordedAt: run.runAt, | ||
| run: cloneCodexCliSyncRun(run), | ||
| }).catch((error) => { | ||
| log.debug("Failed to record codex-cli sync history", { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| }); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
manual apply can lose rollback availability behind the async history write.
lib/codex-cli/sync.ts:line 228 appends sync history fire-and-forget, but lib/codex-cli/sync.ts:line 378 looks for rollback candidates in persisted history only. a user can commit a manual apply and immediately ask for rollback, and getLatestCodexCliSyncRollbackPlan() can still say nothing is available until that background write finishes. please either consult getLastCodexCliSyncRun() before hitting disk or make the commit path wait for history persistence. add a vitest regression in test/codex-cli-sync.test.ts for immediate apply-to-rollback in the same process. 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
async function findLatestManualRollbackRun(): Promise<
CodexCliSyncRun | null
> {
+ const inMemoryRun = getLastCodexCliSyncRun();
+ if (isManualChangedSyncRun(inMemoryRun)) {
+ return inMemoryRun;
+ }
+
const history = await readSyncHistory({ kind: "codex-cli-sync" });
for (let index = history.length - 1; index >= 0; index -= 1) {
const entry = history[index];
if (!entry || entry.kind !== "codex-cli-sync") continue;
const run = normalizeCodexCliSyncRun(entry.run);Also applies to: 375-388
| async function captureRollbackSnapshot(): Promise<CodexCliSyncRollbackSnapshot | null> { | ||
| const snapshot: NamedBackupMetadata | null = await snapshotAccountStorage({ | ||
| reason: "codex-cli-sync", | ||
| failurePolicy: "warn", | ||
| }); | ||
| if (!snapshot) return null; |
There was a problem hiding this comment.
the first manual sync into an empty pool has no rollback path.
lib/codex-cli/sync.ts:line 360 relies on snapshotAccountStorage(), and test/storage.test.ts:line 2108 locks in that helper returning null when no accounts exist. that means lib/codex-cli/sync.ts:line 1040 records rollbackSnapshot: null for a manual apply into an empty target, so getLatestCodexCliSyncRollbackPlan() has nothing to restore in the common first-sync case. persist an explicit empty checkpoint or store a sentinel rollback action that clears local storage, and cover it in test/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."
Also applies to: 1040-1048
🤖 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 359 - 364, captureRollbackSnapshot
currently returns null when snapshotAccountStorage yields no accounts, causing
first manual sync to have no rollback; change captureRollbackSnapshot (and where
it sets rollbackSnapshot around line 1040) to persist an explicit empty
checkpoint or a sentinel rollback action (e.g., "clearLocalStorage" marker) when
snapshotAccountStorage() returns null so getLatestCodexCliSyncRollbackPlan can
restore an empty state; update tests in test/codex-cli-sync.test.ts to cover the
first-sync-empty-pool case and adjust test/storage.test.ts expectations
accordingly, and ensure any new sentinel logging follows guidelines (no leaking
tokens/emails), and that new persistence logic handles Windows IO and
concurrency error cases (EBUSY/429) and integrates with existing auth-rotation
paths.
| const log = createLogger("codex-manager"); | ||
|
|
||
| let destructiveActionInFlight = false; |
There was a problem hiding this comment.
destructiveActionInFlight guard has a toctou race window.
the check at lib/codex-manager.ts:4489 and lib/codex-manager.ts:4515 reads then writes destructiveActionInFlight without atomicity. if two async paths reach the guard before either sets true, both pass. in practice this is unlikely for a single-user cli, but the pattern is fragile.
consider using a Promise-based lock or an async mutex from a well-known library if concurrent invocations become possible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 124 - 126, The boolean guard
destructiveActionInFlight has a TOCTOU race; replace it with an async mutex
instead of reading/then-writing the flag. Import and instantiate a mutex (e.g.,
Mutex from the async-mutex package) at the same scope where
destructiveActionInFlight is declared, then wrap the critical destructive
sections (the code paths that currently check/set destructiveActionInFlight
around the checks referenced by destructiveActionInFlight) in
mutex.runExclusive()/acquire()/release() so only one async invocation can enter
the destructive block at a time; remove direct reads/writes of
destructiveActionInFlight and use the mutex to control exclusive access.
| export function resolveStartupRecoveryAction( | ||
| recoveryState: Awaited<ReturnType<typeof getActionableNamedBackupRestores>>, | ||
| recoveryScanFailed: boolean, | ||
| ): StartupRecoveryAction { | ||
| if (recoveryState.assessments.length > 0) { | ||
| return "show-recovery-prompt"; | ||
| } | ||
| return recoveryScanFailed | ||
| ? "continue-with-oauth" | ||
| : "open-empty-storage-menu"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# check for resolveStartupRecoveryAction test coverage
rg -n --type=ts 'resolveStartupRecoveryAction' test/Repository: ndycode/codex-multi-auth
Length of output: 485
🏁 Script executed:
sed -n '783,820p' test/recovery.test.tsRepository: ndycode/codex-multi-auth
Length of output: 1118
add test for the show-recovery-prompt branch in resolveStartupRecoveryAction.
test/recovery.test.ts has coverage for the empty assessments path but misses the critical case where assessments.length > 0. when recovery state contains assessments, the function should return "show-recovery-prompt". add a test case at test/recovery.test.ts:783 covering this branch so all three outcomes are verified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 4288 - 4298, Add a unit test that
exercises resolveStartupRecoveryAction when the recoveryState object contains a
non-empty assessments array: construct a mock Awaited<ReturnType<typeof
getActionableNamedBackupRestores>> with assessments.length > 0 (other fields can
be defaults), call resolveStartupRecoveryAction(recoveryState,
recoveryScanFailed) with recoveryScanFailed true and/or false, and assert the
return value is "show-recovery-prompt"; place this new test alongside the
existing recovery tests so the branch for resolveStartupRecoveryAction that
returns "show-recovery-prompt" is covered.
| function equalsResolvedStoragePath(left: string, right: string): boolean { | ||
| return process.platform === "win32" | ||
| ? left.toLowerCase() === right.toLowerCase() | ||
| : left === right; | ||
| } |
There was a problem hiding this comment.
normalize both sides of the self-import comparison.
assessOpencodeAccountPool() and importAccounts() pass an absolute source path into equalsResolvedStoragePath() at lib/storage.ts:2492-2494 and lib/storage.ts:3860-3867, but the active target can still be relative via setStoragePathDirect() at lib/storage.ts:1132-1137. because lib/storage.ts:2851-2855 only lowercases and compares raw strings, the same file can slip past the self-import block when one side is relative. please resolve both operands inside the comparator and add a vitest for a relative direct-storage override.
proposed fix
function equalsResolvedStoragePath(left: string, right: string): boolean {
+ const resolvedLeft = resolvePath(left);
+ const resolvedRight = resolvePath(right);
return process.platform === "win32"
- ? left.toLowerCase() === right.toLowerCase()
- : left === right;
+ ? resolvedLeft.toLowerCase() === resolvedRight.toLowerCase()
+ : resolvedLeft === resolvedRight;
}📝 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.
| function equalsResolvedStoragePath(left: string, right: string): boolean { | |
| return process.platform === "win32" | |
| ? left.toLowerCase() === right.toLowerCase() | |
| : left === right; | |
| } | |
| function equalsResolvedStoragePath(left: string, right: string): boolean { | |
| const resolvedLeft = resolvePath(left); | |
| const resolvedRight = resolvePath(right); | |
| return process.platform === "win32" | |
| ? resolvedLeft.toLowerCase() === resolvedRight.toLowerCase() | |
| : resolvedLeft === resolvedRight; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 2851 - 2855, equalsResolvedStoragePath currently
compares raw strings (only lowercasing on Windows), which misses cases when one
side is relative; update equalsResolvedStoragePath to resolve/normalize both
operands (use path.resolve or fs.realpathSync to get absolute canonical paths)
before comparing and still perform platform-specific case normalization on the
resolved values; ensure callers assessOpencodeAccountPool and importAccounts
that pass absolute paths will match targets set via setStoragePathDirect when
given relative values, and add a vitest that calls setStoragePathDirect with a
relative path then runs importAccounts/assessOpencodeAccountPool to assert the
self-import block triggers.
| await renameFileWithRetry(tempPath, path); | ||
| try { | ||
| await fs.unlink(resetMarkerPath); | ||
| } catch { | ||
| // Best effort cleanup. | ||
| } | ||
| if (lastError) throw lastError; | ||
| lastAccountsSaveTimestamp = Date.now(); | ||
| try { | ||
| await fs.unlink(walPath); | ||
| } catch { | ||
| // Best effort cleanup. | ||
| } |
There was a problem hiding this comment.
don't swallow reset-marker cleanup failures after a committed save.
after lib/storage.ts:3318 the new storage file is already live. if lib/storage.ts:3319-3323 loses marker cleanup to a transient windows eperm/ebusy, this still returns success, but lib/storage.ts:3019-3021 will treat the stale marker as an intentional reset and hide the just-written accounts on the next load. please run this through unlinkWithRetry() and add a vitest in test/storage.test.ts that forces marker cleanup failure after a successful save/import.
proposed fix
await renameFileWithRetry(tempPath, path);
- try {
- await fs.unlink(resetMarkerPath);
- } catch {
- // Best effort cleanup.
- }
+ await unlinkWithRetry(resetMarkerPath);
lastAccountsSaveTimestamp = Date.now();
try {
await fs.unlink(walPath);📝 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.
| await renameFileWithRetry(tempPath, path); | |
| try { | |
| await fs.unlink(resetMarkerPath); | |
| } catch { | |
| // Best effort cleanup. | |
| } | |
| if (lastError) throw lastError; | |
| lastAccountsSaveTimestamp = Date.now(); | |
| try { | |
| await fs.unlink(walPath); | |
| } catch { | |
| // Best effort cleanup. | |
| } | |
| await renameFileWithRetry(tempPath, path); | |
| await unlinkWithRetry(resetMarkerPath); | |
| lastAccountsSaveTimestamp = Date.now(); | |
| try { | |
| await fs.unlink(walPath); | |
| } catch { | |
| // Best effort cleanup. | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 3318 - 3329, After committing the new storage
file with renameFileWithRetry, do not swallow transient unlink failures for the
reset marker: replace the best-effort fs.unlink(resetMarkerPath) (and likewise
the walPath unlink) with unlinkWithRetry(resetMarkerPath) so transient
EBUSY/EPERM retries are attempted and a real failure is surfaced; update the
block around renameFileWithRetry/lastAccountsSaveTimestamp to call
unlinkWithRetry for resetMarkerPath (and walPath) and preserve existing
logging/context, and add a vitest in test/storage.test.ts that simulates an
unlink failure after a successful save/import to assert unlinkWithRetry is used
and the stale reset marker is removed (or the error is retried/propagated) to
prevent the marker from hiding the just-written accounts.
| const storagePath = getStoragePath(); | ||
| const state = { | ||
| snapshot: await loadAccountsInternal(saveAccountsUnlocked), | ||
| active: true, | ||
| storagePath, | ||
| }; |
There was a problem hiding this comment.
pin the transaction's storage paths through persist, not just through async-local state.
the new storagePath captured at lib/storage.ts:3381-3386 and lib/storage.ts:3408-3413 is only used later by exportAccounts() at lib/storage.ts:3812-3822. the actual persist and rollback paths still go through helpers that re-read mutable global state at lib/storage.ts:3246, lib/storage.ts:3426-3428, and lib/storage.ts:3693, so an async setStoragePath() / setStoragePathDirect() in the middle of the transaction can snapshot project a and commit project b. please thread pinned account/flagged paths into the save helpers and add a vitest that flips the storage target between transaction start and persist().
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."
Also applies to: 3408-3413
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 3381 - 3386, The snapshot captures storagePath
via getStoragePath() but persist/rollback still re-read global mutable storage
state (e.g., setStoragePath / setStoragePathDirect), so thread the pinned
storagePath through the save helpers and transaction commit/rollback paths
instead of relying on async-local state: update persist(), the save helpers
invoked by persist (e.g., functions that call loadAccountsInternal,
exportAccounts, and any saveAccounts* helpers) to accept and use an explicit
storagePath parameter, ensure rollback uses that pinned path, and stop
re-reading getStoragePath()/global state inside those helpers; add a vitest that
starts a transaction, flips the global storage target between start and persist,
then asserts the original pinned path was used for persist/rollback, and
reference the modified symbols (persist, exportAccounts, loadAccountsInternal,
setStoragePath/setStoragePathDirect) in the test.
| it("should migrate from old filename to new filename", async () => { | ||
| // This test is tricky because it depends on the internal state of getStoragePath() | ||
| // which we are about to change. | ||
|
|
||
| const oldName = "openai-codex-accounts.json"; | ||
| const newName = "codex-accounts.json"; | ||
|
|
||
| // We'll need to mock/verify that loadAccounts checks for oldName if newName is missing | ||
| // Since we haven't implemented it yet, this is just a placeholder for the logic | ||
| expect(true).toBe(true); |
There was a problem hiding this comment.
placeholder migration test still passes without exercising migration.
test/storage.test.ts:line 2394 is just expect(true).toBe(true), so this suite stays green even if the old-to-new filename migration never works. replace it with a real end-to-end assertion or remove the placeholder until the behavior exists. 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 2385 - 2394, The test "should migrate from
old filename to new filename" currently contains a placeholder assertion
(expect(true).toBe(true)) and doesn't exercise the migration; replace it with a
deterministic end-to-end assertion that verifies the actual migration behavior:
create a test fixture where a file named "openai-codex-accounts.json" (oldName)
exists and "codex-accounts.json" (newName) does not, call the code path that
triggers migration (e.g., invoke loadAccounts() or the function that uses
getStoragePath()), then assert that the migration occurred by checking that
"codex-accounts.json" now exists (or that loadAccounts() returns the expected
accounts) and that "openai-codex-accounts.json" was removed or ignored; if
migration behavior isn't implemented yet, remove the placeholder test instead of
leaving expect(true).toBe(true) so the suite remains meaningful.
| const targetPath = getStoragePath(); | ||
| const syncEnabled = isCodexCliSyncEnabled(); | ||
| const backup = { | ||
| enabled: options.storageBackupEnabled ?? true, | ||
| targetPath, | ||
| rollbackPaths: formatRollbackPaths(targetPath), | ||
| }; | ||
| const lastSync = getLastCodexCliSyncRun(); |
There was a problem hiding this comment.
previewCodexCliSync defaults backup.enabled to true when omitted
storageBackupEnabled defaults to true when not provided (options.storageBackupEnabled ?? true). any caller that omits the field (tests, future callers, or a direct previewCodexCliSync(current) call) will see backup.enabled: true in the rendered preview and the hint "pre-sync backup enabled" — even when the user has disabled backups in settings. the settings hub passes it correctly at call site, but the misleading default creates a footgun for future callers and misrepresents the user's actual backup config in all bare calls.
consider flipping the default to false and making callers opt in, or replacing the boolean with "inherit" / "override" to make intent explicit:
const backup = {
enabled: options.storageBackupEnabled ?? false,
targetPath,
rollbackPaths: formatRollbackPaths(targetPath),
};or document the intent explicitly with a comment so future callers understand undefined ≠ disabled.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-cli/sync.ts
Line: 874-881
Comment:
**`previewCodexCliSync` defaults `backup.enabled` to `true` when omitted**
`storageBackupEnabled` defaults to `true` when not provided (`options.storageBackupEnabled ?? true`). any caller that omits the field (tests, future callers, or a direct `previewCodexCliSync(current)` call) will see `backup.enabled: true` in the rendered preview and the hint "pre-sync backup enabled" — even when the user has disabled backups in settings. the settings hub passes it correctly at call site, but the misleading default creates a footgun for future callers and misrepresents the user's actual backup config in all bare calls.
consider flipping the default to `false` and making callers opt in, or replacing the boolean with `"inherit"` / `"override"` to make intent explicit:
```typescript
const backup = {
enabled: options.storageBackupEnabled ?? false,
targetPath,
rollbackPaths: formatRollbackPaths(targetPath),
};
```
or document the intent explicitly with a comment so future callers understand `undefined` ≠ disabled.
How can I resolve this? If you propose a fix, please make it concise.| export function commitPendingCodexCliSyncRun( | ||
| pendingRun: PendingCodexCliSyncRun | null | undefined, | ||
| ): void { | ||
| if (!pendingRun) return; | ||
| if (!markPendingCodexCliSyncRunCompleted(pendingRun.revision)) { | ||
| return; | ||
| } | ||
| publishCodexCliSyncRun( | ||
| { | ||
| ...pendingRun.run, | ||
| runAt: Date.now(), | ||
| rollbackSnapshot: normalizeRollbackSnapshot( | ||
| pendingRun.run.rollbackSnapshot, | ||
| ), | ||
| }, | ||
| allocateCodexCliSyncRunRevision(), | ||
| ); | ||
| } | ||
|
|
||
| export function commitCodexCliSyncRunFailure( | ||
| pendingRun: PendingCodexCliSyncRun | null | undefined, | ||
| error: unknown, | ||
| ): void { | ||
| if (!pendingRun) return; | ||
| if (!markPendingCodexCliSyncRunCompleted(pendingRun.revision)) { | ||
| return; | ||
| } | ||
| publishCodexCliSyncRun( | ||
| buildSyncRunError( | ||
| { | ||
| ...pendingRun.run, | ||
| runAt: Date.now(), | ||
| }, | ||
| error, | ||
| ), | ||
| allocateCodexCliSyncRunRevision(), | ||
| ); | ||
| } | ||
|
|
||
| export function __resetLastCodexCliSyncRunForTests(): void { |
There was a problem hiding this comment.
commitPendingCodexCliSyncRun calls allocateCodexCliSyncRunRevision() at commit time — not at allocation time. in a scenario where two applyCodexCliSyncToStorage calls are in flight concurrently (possible from the settings hub if the user triggers apply while a background refresh is still pending), the later-committing run gets a higher revision and overwrites lastCodexCliSyncRun, regardless of which run was initiated first:
- call A allocated pending rev 3, call B allocated pending rev 4
- B commits first → allocates publish rev 5
- A commits second → allocates publish rev 6 → overwrites B as "latest"
lastCodexCliSyncRun ends up reflecting the older run's data. for the sync center display this means the "last sync" status could show stale counts/outcome after an apply.
the fix is to publish with the pending revision directly (not re-allocate), or record startedAt alongside runAt and order by startedAt instead. windows filesystem races between quota-cache refresh and manual apply make this a realistic path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-cli/sync.ts
Line: 312-351
Comment:
**out-of-order commit overwrites newer sync run**
`commitPendingCodexCliSyncRun` calls `allocateCodexCliSyncRunRevision()` at commit time — not at allocation time. in a scenario where two `applyCodexCliSyncToStorage` calls are in flight concurrently (possible from the settings hub if the user triggers apply while a background refresh is still pending), the later-committing run gets a higher revision and overwrites `lastCodexCliSyncRun`, regardless of which run was initiated first:
- call A allocated pending rev 3, call B allocated pending rev 4
- B commits first → allocates publish rev 5
- A commits second → allocates publish rev 6 → **overwrites B as "latest"**
`lastCodexCliSyncRun` ends up reflecting the older run's data. for the sync center display this means the "last sync" status could show stale counts/outcome after an apply.
the fix is to publish with the pending revision directly (not re-allocate), or record `startedAt` alongside `runAt` and order by `startedAt` instead. windows filesystem races between quota-cache refresh and manual apply make this a realistic path.
How can I resolve this? If you propose a fix, please make it concise.
|
Superseded by clean replacement PR #128 ( |
Summary
fresh/17-health-summary-dashboard#82What Changed
CODEX_OPENCODE_POOL_PATH.Validation
cmd /c npm test -- test/storage.test.ts -t "opencode account pool detection"cmd /c npm test -- test/codex-manager-cli.test.ts -t "OpenCode"cmd /c npm test -- test/documentation.test.tsnpm run lintnpm run typechecknpm run buildcmd /c npm testDocs 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
fresh/18-import-adapter-opencodeback tofresh/17-health-summary-dashboardAdditional Notes
fresh/17-health-summary-dashboard, notmain.EPERMimport regression intest/codex-manager-cli.test.ts.e6e235a.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 replays the opencode import adapter on top of the health-summary dashboard base, aligning the login flow, import eligibility checks, and storage behavior with current restore contracts. the changes span three areas: (1) the
import-opencodehandler inrunAuthLoginwith assessment, confirmation, and redacted error paths; (2) a large new sync infrastructure inlib/codex-cli/sync.tscovering preview, apply, rollback snapshots, and pending-run tracking; and (3) thedestructive-actionsmodule extracting delete/reset operations with snapshot-before-action recovery context.improvements from previous review threads:
assessOpencodeAccountPoolcall is now wrapped in try/catch — assessment throws no longer crash the login loopassessment.backup.pathis reduced tobasename(...)before display — full windows filesystem path no longer shown in confirm prompt or action panel subtitleCODEX_OPENCODE_POOL_PATHoverride now short-circuits on missing file (returnsnull) instead of silently falling back to auto-detectionprocess.env.HOME = fakeHomealongsideUSERPROFILE, fixing the posix CI failureremaining concerns from previous threads (not addressed in this pr):
assessment.wouldExceedLimitcheck at line 4457 is still unreachable dead code —eligibleForRestoreis alwaysfalsewhenwouldExceedLimitistrue, so the counts-aware "Import would exceed the account limit (X current, Y after import)" message is never shown; users see "Restore would exceed maximum of N accounts" fromassessment.errorinsteadassessment.erroron the!eligibleForRestorebranch is still not passed throughformatRedactedFilesystemError—backup.loadErrorfrombuildBackupFileMetadatacan embed raw windows filesystem pathsredactFilesystemDetailsregex still requires two or more path segments to match, leaving single-segment windows paths unredactednew issues found in this pr:
previewCodexCliSyncdefaultsstorageBackupEnabledtotruewhen omitted — callers that don't pass the flag see backup shown as enabled regardless of actual user settingscommitPendingCodexCliSyncRunallocates a new revision at commit time rather than reusing the pending revision; if twoapplyCodexCliSyncToStoragecalls complete out of order the later-committing (but earlier-initiated) run overwriteslastCodexCliSyncRunwith stale data## Backend Categoriesheading indocs/reference/settings.mdis at H2 level instead of H3, breaking the document hierarchyConfidence Score: 2/5
wouldExceedLimitdead code means the more-informative counts message is never reached in production. theassessment.errorredaction gap is a windows token-safety risk. theredactFilesystemDetailsregex gap is documented in the previous thread but still unpatched. additionally, the newpreviewCodexCliSyncdefault and thecommitPendingCodexCliSyncRunout-of-order overwrite are new issues introduced in this pr. the sync history and destructive-actions modules are clean; the risk is concentrated in codex-manager.ts, storage.ts, and the new sync.ts.Important Files Changed
import-opencodehandler andbuildLoginMenuHealthSummary. the previous-thread issues around try/catch (assessment throws now caught), basename path display, and assessment error redaction are partially addressed. thewouldExceedLimitdead code at line 4457 remains —eligibleForRestoreis always false whenwouldExceedLimitis true, so the counts-aware message is never shown in production.destructiveActionInFlightguard is absent forimportAccounts, thoughwithAccountStorageTransactionin the lower layer provides some protection.previewCodexCliSyncdefaultsstorageBackupEnabledtotruewhich misleads callers that omit the flag; (2)commitPendingCodexCliSyncRunallocates a new revision at commit time rather than publish time, allowing out-of-order concurrent commits to overwrite a newer run with older data.detectOpencodeAccountPoolPath,assessOpencodeAccountPool,formatRedactedFilesystemError, and redaction helpers. the previous-thread issues aroundCODEX_OPENCODE_POOL_PATHsilent fallback and theredactFilesystemDetailssingle-segment regex gap are not fully resolved (the regex still requires two path segments to match).assessment.errorset frombackup.loadErrorcan still surface raw filesystem paths on the ineligible-restore branch in codex-manager.readLatestSyncHistorySyncusesreadFileSync(one-time cost on first menu render).pruneSyncHistoryEntriescorrectly preserves the latest entry of each kind. no significant issues found.isInteractiveLoginMenuAvailable(),promptResetTypedConfirm(),import-opencodeandrestore-backupmodes to the login menu fallback path. the!isTTY()guard is correctly upgraded to!isInteractiveLoginMenuAvailable(), blocking TUI launch in non-interactive envs. fallback path now requires typed confirm before fresh/reset, consistent with the TUI path.HOMEenv var is now saved inoriginalHomeand restored inafterEach, and also set in the homedir fallback test body — addressing the prior POSIX CI failure. the explicit-path no-fallback test correctly validatesdetectOpencodeAccountPoolPathreturnsnullwhen the override file is absent.actionwas called once and assertsgetActionableNamedBackupRestoresMockwas invoked. theit.eachlimit fixture at line 1295 setseligibleForRestore: true, wouldExceedLimit: true, error: ""— a combination that is impossible fromassessNamedBackupRestoreCandidatein production — meaning the counts-aware branch it exercises is dead code in practice.## Backend Categoriesheading is at H2 level but should be H3 — it is logically a sub-section of### Advanced Backend Controls, and the current level breaks the document's heading hierarchy for TOC and nav generators.Sequence Diagram
sequenceDiagram participant User participant runAuthLogin participant detectOpencode as detectOpencodeAccountPoolPath participant assessPool as assessOpencodeAccountPool participant storage as storage.ts (withAccountStorageTransaction) participant syncHistory as sync-history.ts User->>runAuthLogin: codex auth login runAuthLogin->>runAuthLogin: loadAccounts() — existing accounts? alt import-opencode mode selected runAuthLogin->>assessPool: assessOpencodeAccountPool(currentStorage) assessPool->>detectOpencode: detectOpencodeAccountPoolPath() note over detectOpencode: checks CODEX_OPENCODE_POOL_PATH first<br/>returns null if explicit path missing (no fallback) detectOpencode-->>assessPool: resolvedPath | null assessPool->>assessPool: equalsResolvedStoragePath guard (throws if self-import) assessPool->>assessPool: loadBackupCandidate + assessNamedBackupRestoreCandidate assessPool-->>runAuthLogin: BackupRestoreAssessment | null runAuthLogin->>runAuthLogin: check valid + eligibleForRestore runAuthLogin->>User: confirm(basename(path)) User-->>runAuthLogin: confirmed runAuthLogin->>storage: importAccounts(path) storage-->>runAuthLogin: {imported, skipped, total} end alt restore-backup mode selected runAuthLogin->>runAuthLogin: runBackupBrowserManager(displaySettings) end alt fresh/reset mode selected runAuthLogin->>storage: deleteSavedAccounts() or resetLocalState() runAuthLogin->>runAuthLogin: suppressRecoveryPrompt = true end alt applyCodexCliSync (manual, settings hub) runAuthLogin->>syncHistory: appendSyncHistoryEntry (fire-and-forget) syncHistory->>syncHistory: withHistoryLock → appendFile → trimHistory → rewriteLatest endPrompt To Fix All With AI
Last reviewed commit: "fix(test): remove st..."