fix(storage): harden recovery and intentional reset flows#71
Conversation
Co-authored-by: Codex <noreply@openai.com>
📝 Walkthroughwalkthroughadds backup/restore metadata, reset-marker, and backup rotation flows for accounts and flagged accounts; adds path normalization and explicit CODEX_HOME handling; exposes new storage apis and extensive recovery tests. (≤50 words) changes
sequence diagram(s)sequenceDiagram
participant cli as "cli"
participant storage as "lib/storage.ts"
participant fs as "filesystem"
participant backup as "backup rotation (.bak)"
cli->>storage: saveAccountsUnlocked(payload)
storage->>fs: write primary storage file
fs-->>storage: write success
storage->>backup: rotate existing .bak files (rename -> .bak.N)
backup-->>storage: rotation success
storage->>fs: removeIntentionalResetMarker(path)
fs-->>storage: marker removed
storage-->>cli: success
estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes suggested labels
key concerns during review
🚥 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: Codex <noreply@openai.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/runtime-paths.ts">
<violation number="1" location="lib/runtime-paths.ts:79">
P2: Windows path equality treats trailing separators as different, so explicit default `CODEX_HOME` values like `...\.codex\` are misclassified as non-default and bypass fallback recovery logic.</violation>
</file>
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:664">
P3: Test name no longer matches the asserted behavior (it now checks restore metadata, not a null return). Rename the test to reflect the new expected result.</violation>
<violation number="2" location="test/storage.test.ts:1892">
P3: Updated assertions preserve `.bak*` files, but the test label still says backups are removed. Rename the test description to avoid misleading behavior docs.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:751">
P1: Do not clear the intentional-reset marker before the new canonical file is successfully written. A failed save after this line can re-enable WAL/backup token revival.</violation>
<violation number="2" location="lib/storage.ts:882">
P0: Do not swallow reset-marker write failures; this reset path should abort when marker creation fails so primary/WAL deletion cannot proceed without suppression state.</violation>
<violation number="3" location="lib/storage.ts:994">
P1: Fallback migration is not scoped to global storage paths. In project-scoped mode it can migrate (and delete) global account files into a single project storage file.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: Codex <noreply@openai.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Co-authored-by: Codex <noreply@openai.com>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| export async function clearFlaggedAccounts(): Promise<void> { | ||
| return withStorageLock(async () => { | ||
| try { | ||
| await fs.unlink(getFlaggedAccountsPath()); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| log.error("Failed to clear flagged account storage", { error: String(error) }); | ||
| const path = getFlaggedAccountsPath(); | ||
| const backupPaths = getAccountsBackupRecoveryCandidates(path); | ||
| const clearPath = async (targetPath: string): Promise<void> => { | ||
| try { | ||
| await fs.unlink(targetPath); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| log.error("Failed to clear flagged account storage artifact", { | ||
| path: targetPath, | ||
| error: String(error), | ||
| }); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| try { | ||
| await Promise.all([clearPath(path), ...backupPaths.map(clearPath)]); | ||
| } catch { | ||
| // Individual cleanup is already best effort with per-artifact logging. | ||
| } | ||
| }); |
There was a problem hiding this comment.
Critical inconsistency: clearFlaggedAccounts() deletes all backup artifacts including .bak files, but clearAccounts() (lines 1802-1815) only deletes the primary file and WAL while preserving backups. This violates the PR's stated intent to "preserve .bak* recovery artifacts" and creates inconsistent behavior between the two clear functions.
Impact: Users lose flagged account recovery snapshots after reset, but retain regular account recovery snapshots. This could cause data loss if flagged accounts need to be recovered after an intentional reset.
Fix: Make clearFlaggedAccounts() consistent with clearAccounts() by only deleting the primary flagged accounts file:
export async function clearFlaggedAccounts(): Promise<void> {
return withStorageLock(async () => {
const path = getFlaggedAccountsPath();
try {
await fs.unlink(path);
} catch (error) {
const code = (error as NodeJS.ErrnoException).code;
if (code !== "ENOENT") {
log.error("Failed to clear flagged account storage", {
path,
error: String(error),
});
}
}
});
}| export async function clearFlaggedAccounts(): Promise<void> { | |
| return withStorageLock(async () => { | |
| try { | |
| await fs.unlink(getFlaggedAccountsPath()); | |
| } catch (error) { | |
| const code = (error as NodeJS.ErrnoException).code; | |
| if (code !== "ENOENT") { | |
| log.error("Failed to clear flagged account storage", { error: String(error) }); | |
| const path = getFlaggedAccountsPath(); | |
| const backupPaths = getAccountsBackupRecoveryCandidates(path); | |
| const clearPath = async (targetPath: string): Promise<void> => { | |
| try { | |
| await fs.unlink(targetPath); | |
| } catch (error) { | |
| const code = (error as NodeJS.ErrnoException).code; | |
| if (code !== "ENOENT") { | |
| log.error("Failed to clear flagged account storage artifact", { | |
| path: targetPath, | |
| error: String(error), | |
| }); | |
| } | |
| } | |
| }; | |
| try { | |
| await Promise.all([clearPath(path), ...backupPaths.map(clearPath)]); | |
| } catch { | |
| // Individual cleanup is already best effort with per-artifact logging. | |
| } | |
| }); | |
| export async function clearFlaggedAccounts(): Promise<void> { | |
| return withStorageLock(async () => { | |
| const path = getFlaggedAccountsPath(); | |
| try { | |
| await fs.unlink(path); | |
| } catch (error) { | |
| const code = (error as NodeJS.ErrnoException).code; | |
| if (code !== "ENOENT") { | |
| log.error("Failed to clear flagged account storage", { | |
| path, | |
| error: String(error), | |
| }); | |
| } | |
| } | |
| }); | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/reference/storage-paths.md`:
- Around line 40-42: Update the recovery-candidate documentation for
getBackupMetadata() to explicitly exclude the reset marker `.reset-intent`
(i.e., list it alongside "cache-like artifacts" / `.bak` variants) so operators
don't treat it as a restore candidate; also add a matching entry in the upgrade
notes and update docs/README/SECURITY sections to mention the new reset contract
and any new npm scripts introduced for reset/upgrade workflows. Ensure the text
names `.reset-intent`, references getBackupMetadata() as the source of truth for
discovery rules, and mirrors the exclusion in the upgrade notes and
documentation updates.
In `@lib/codex-manager.ts`:
- Around line 3809-3811: The helper clearAccountsAndReset() currently calls
saveAccounts() which writes an empty storage file and bypasses the new
reset-marker/WAL suppression flow; replace that call with clearAccounts() so the
interactive "Reset Accounts" path (invoked via runActionPanel("Reset Accounts",
...)) follows the same safe host reset behavior, update any logging there to
avoid overstating safety, and add a vitest regression test that simulates the
interactive reset flow to assert the reset-marker/WAL suppression is used
(covering codex auth login regression as well).
In `@lib/storage.ts`:
- Around line 890-909: writeIntentionalResetMarker and
removeIntentionalResetMarker can fail on Windows due to transient locks
(EBUSY/EPERM); update both functions to use the same retry-on-EBUSY/EPERM
pattern used by copyFileWithRetry/renameFileWithRetry: for
writeIntentionalResetMarker wrap the fs.writeFile call in the retry loop so
transient errors are retried and any final failure still bubbles up (so
clearAccounts remains fatal), and for removeIntentionalResetMarker wrap
fs.unlink in the same retry loop (still ignore ENOENT but only log/warn after
all retries fail); prefer reusing the existing retry helper if present,
otherwise implement the same backoff/retry behavior and reference
markerPath/getIntentionalResetMarkerPath in the implementation.
- Around line 1670-1674: Add a test that simulates removeIntentionalResetMarker
failing during saveAccounts: mock the underlying unlink used by
removeIntentionalResetMarker to throw on the first save, call saveAccounts and
assert it still resolves (save succeeds), then call loadAccounts and assert the
returned state shows the stale marker (restoreEligible is false), then make the
unlink mock succeed (or restore behavior) and call saveAccounts again and
finally call loadAccounts to assert the marker was cleared (restoreEligible
true); reference removeIntentionalResetMarker, saveAccounts, and loadAccounts
when locating code to mock and verify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1262e321-12a4-4ea8-bfcb-52590b736806
📒 Files selected for processing (10)
docs/reference/storage-paths.mdindex.tslib/codex-manager.tslib/runtime-paths.tslib/storage.tstest/helpers/remove-with-retry.tstest/runtime-paths.test.tstest/storage-flagged.test.tstest/storage-recovery-paths.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
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/runtime-paths.tslib/storage.tslib/codex-manager.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage-flagged.test.tstest/runtime-paths.test.tstest/helpers/remove-with-retry.tstest/storage.test.tstest/storage-recovery-paths.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/reference/storage-paths.md
🔇 Additional comments (29)
index.ts (1)
3183-3185: good copy update.this message now matches the host-side reset flow that clears active storage before starting fresh. index.ts:3180-3185.
test/helpers/remove-with-retry.ts (1)
3-22: good windows cleanup retry helper.bounded retries on
ebusy/eperm/enotempty/eacces, while treatingenoentas success, is the right shape for deterministic vitest cleanup on windows without hiding real failures. test/helpers/remove-with-retry.ts:3-22.lib/storage.ts (10)
119-124: type definitions look good.the
RestoreMetadataunion andAccountStorageWithMetadataintersection cleanly extend storage with eligibility metadata without breaking existing callers.
209-212: cache artifact filtering is reasonable.filtering
.cacheentries from backup discovery prevents spurious recovery candidates. covered bytest/storage-recovery-paths.test.ts:620-690.
280-319: fallback path inference looks reasonable.the
DevTools/config/codexhierarchy detection atlib/storage.ts:287-293handles windows-specific paths. the deduplication logic properly filters the current path from candidates.
476-520: backup metadata types are well-structured.the
BackupSnapshotMetadataandRestoreAssessmenttypes provide comprehensive introspection for recovery flows. good separation of concerns.
998-1052: fallback migration logic is safe.
migrateFallbackAccountStorageIfNeededatlib/storage.ts:998-1052correctly returns the fallback storage even when persist fails (lib/storage.ts:1045), preventing data loss. the fallback file persists until a successful migration, which is the right tradeoff.
1362-1364: return type change is backward-compatible.
loadAccounts()now returnsAccountStorageWithMetadata | null. callers inlib/accounts.ts:111-122andindex.ts:989-1001use object spread or direct property access, preserving the new metadata fields. the change is additive.
1419-1434: reset marker suppresses migrations and wal recovery correctly.
lib/storage.ts:1419-1428returns early when marker exists without primary file.lib/storage.ts:1429-1434skips legacy and fallback migrations when marker is present. the wal recovery path atlib/storage.ts:1523-1541is only reached after checking for marker atlib/storage.ts:1507-1512.the pr description notes "a narrow race where migrations can run while a reset marker exists (project-scoped mode)" - but project-scoped mode returns early at
lib/storage.ts:1002-1004, so fallback migration is scoped correctly.
1772-1816: clearaccounts correctly writes marker before deleting artifacts.the ordering at
lib/storage.ts:1791-1800(write marker first, abort on failure) ensures the reset marker is authoritative. if marker write fails, primary and wal remain intact. test coverage attest/storage.test.ts:760-781validates this abort behavior.logging at
lib/storage.ts:1794-1798includes only paths, no tokens or emails. safe per coding guidelines.
1954-1963: flagged accounts now get backup rotation.
saveFlaggedAccountsatlib/storage.ts:1954-1963mirrors the backup rotation pattern fromsaveAccountsUnlocked. test coverage attest/storage-flagged.test.ts:164-184validates backup creation.
1727-1753: utility functions are well-implemented.
cloneAccountStorageatlib/storage.ts:1727-1739correctly shallow-clones nested structures.createEmptyAccountStorageatlib/storage.ts:1741-1753initializes all model families to index 0.lib/runtime-paths.ts (2)
75-88: pathsequalnormalized handles windows edge cases well.
lib/runtime-paths.ts:75-88correctly handles:
- trailing separators via
win32.parse(normalized).rootcomparison- drive roots like
C:\preserved without stripping- case-insensitive comparison on windows
this implementation is more thorough than the one in
lib/storage.ts:276-278. consider consolidating.
187-201: explicit codex_home override takes precedence correctly.
lib/runtime-paths.ts:189-191detects whenCODEX_HOMEis explicitly set to a non-default path. the early return atlib/runtime-paths.ts:199-201prevents fallback probing, ensuring the canonical root is stable even when fallback directories contain accounts.test coverage at
test/runtime-paths.test.ts:89-103validates this behavior.test/runtime-paths.test.ts (2)
89-103: test validates explicit codex_home precedence.
test/runtime-paths.test.ts:89-103ensures that whenCODEX_HOMEis set to a non-default path like.codex-canonical, the primary directory is returned even when fallback contains accounts. good regression coverage.
138-155: test covers windows trailing separator edge case.
test/runtime-paths.test.ts:138-155validates thatC:\Users\Neil\.codex\(with trailing separator) is treated as equal to the default home, allowing fallback probing. good coverage for thepathsEqualNormalizednormalization logic.test/storage-flagged.test.ts (2)
164-184: test validates backup preservation after clear.
test/storage-flagged.test.ts:164-184confirms that.bakfile persists afterclearFlaggedAccounts(). the double-save pattern correctly triggers backup creation before the clear operation.
186-226: test validates flagged account backup metadata.
test/storage-flagged.test.ts:186-226exercisesgetBackupMetadata()for flagged accounts, verifyingsnapshotCount,latestValidPath, and individual snapshotflaggedCountvalues. comprehensive coverage for the new metadata infrastructure.test/storage.test.ts (5)
642-647: test correctly validates new restore metadata.
test/storage.test.ts:642-647verifies thatloadAccounts()returnsrestoreEligible: trueandrestoreReason: "missing-storage"when storage file doesn't exist. aligns with theAccountStorageWithMetadatatype.
760-781: excellent regression test for marker write failure.
test/storage.test.ts:760-781validates thatclearAccounts()aborts when the reset marker can't be written, leaving primary and wal intact. this is critical for preventing accidental token revival. good coverage for the safety mechanism atlib/storage.ts:1791-1800.
861-910: test validates fallback migration scope isolation.
test/storage.test.ts:881-909confirms that global fallback storage at~/.codexis not migrated into project-scoped storage. this is the correct behavior perlib/storage.ts:1002-1004which returns early whencurrentProjectRootis set.
1140-1170: test validates fallback migration with cleanup.
test/storage.test.ts:1141-1170exercises themigrateFallbackAccountStorageIfNeededflow, verifying that the fallback file is deleted only after canonical write succeeds. covers the happy path atlib/storage.ts:1021-1033.
1919-1948: test validates clearaccounts preserves recovery artifacts.
test/storage.test.ts:1920-1948confirms thatclearAccounts()removes primary and wal but preserves.bak,.bak.1,.bak.2. this aligns with the pr objective to preserve recovery snapshots during intentional reset.test/storage-recovery-paths.test.ts (6)
17-26: test helper is well-typed.
getRestoreEligibilityattest/storage-recovery-paths.test.ts:17-26provides type-safe extraction of metadata fields from the opaqueloadAccounts()return value. clean implementation.
344-366: tests validate restore eligibility for missing and empty storage.
test/storage-recovery-paths.test.ts:344-352(missing) and354-366(empty) cover the two primary restore-eligible states. good baseline coverage.
368-391: test validates marker lifecycle.
test/storage-recovery-paths.test.ts:368-391confirms thatclearAccounts()suppresses restore eligibility andsaveAccounts()clears the marker, re-enabling eligibility. critical regression test for the reset flow.
510-543: critical test: wal not revived after reset.
test/storage-recovery-paths.test.ts:510-543is the key security regression test. it ensures that manually writing a wal file afterclearAccounts()does not revive tokens - the reset marker is authoritative. this validates the safety mechanism atlib/storage.ts:1507-1512.
545-567: test validates reset marker exclusion from discovery.
test/storage-recovery-paths.test.ts:545-567confirms that.reset-intentfiles are excluded fromgetBackupMetadata()snapshots, per the filter atlib/storage.ts:227.
619-690: comprehensive backup metadata test.
test/storage-recovery-paths.test.ts:620-690exercisesgetBackupMetadata()with multiple artifact types:
.cachefiles excluded (validateslib/storage.ts:209-212)- wal detected as
latestValidPathwhen primary is invalid- discovered backups properly categorized
this test provides strong coverage for the backup metadata infrastructure.
| Backup metadata: | ||
|
|
||
| - `getBackupMetadata()` reports deterministic snapshot lists for the canonical account pool (primary, WAL, `.bak`, `.bak.1`, `.bak.2`, and discovered manual backups) and flagged-account state (primary, `.bak`, `.bak.1`, `.bak.2`, and discovered manual backups). Cache-like artifacts are excluded from recovery candidates. |
There was a problem hiding this comment.
document the reset marker explicitly here.
this section now defines the recovery candidate set, but it only says "cache-like artifacts" are excluded. the new reset contract depends on .reset-intent being excluded from backup discovery, so please name that file here and mirror it in the upgrade notes so operators do not mistake it for a restore candidate. docs/reference/storage-paths.md:40-42.
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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/reference/storage-paths.md` around lines 40 - 42, Update the
recovery-candidate documentation for getBackupMetadata() to explicitly exclude
the reset marker `.reset-intent` (i.e., list it alongside "cache-like artifacts"
/ `.bak` variants) so operators don't treat it as a restore candidate; also add
a matching entry in the upgrade notes and update docs/README/SECURITY sections
to mention the new reset contract and any new npm scripts introduced for
reset/upgrade workflows. Ensure the text names `.reset-intent`, references
getBackupMetadata() as the source of truth for discovery rules, and mirrors the
exclusion in the upgrade notes and documentation updates.
| function normalizePathForDedup(pathValue: string): string { | ||
| const normalized = normalize(pathValue.trim()); | ||
| return process.platform === "win32" ? normalized.toLowerCase() : normalized; | ||
| } | ||
|
|
||
| function deduplicatePathList(paths: string[]): string[] { | ||
| const seen = new Set<string>(); | ||
| const result: string[] = []; | ||
|
|
||
| for (const candidate of paths) { | ||
| const trimmed = candidate.trim(); | ||
| if (!trimmed) continue; | ||
| const key = normalizePathForDedup(trimmed); | ||
| if (seen.has(key)) continue; | ||
| seen.add(key); | ||
| result.push(trimmed); | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| function pathsEqualNormalized(a: string, b: string): boolean { | ||
| return normalizePathForDedup(a) === normalizePathForDedup(b); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# verify both pathsEqualNormalized implementations
echo "=== storage.ts implementation ==="
rg -n "pathsEqualNormalized" lib/storage.ts -A5
echo "=== runtime-paths.ts implementation ==="
rg -n "pathsEqualNormalized" lib/runtime-paths.ts -A15Repository: ndycode/codex-multi-auth
Length of output: 1891
🏁 Script executed:
#!/bin/bash
# check for tests covering these functions
echo "=== test files referencing pathsEqualNormalized ==="
rg -l "pathsEqualNormalized" test/
echo "=== test files for storage.ts ==="
fd "storage" test/ -t f
echo "=== all usages of pathsEqualNormalized ==="
rg -n "pathsEqualNormalized" --type ts -B2 -A2
echo "=== deduplicatePaths usage in runtime-paths.ts ==="
rg -n "deduplicatePath" lib/runtime-paths.ts -B2 -A2Repository: ndycode/codex-multi-auth
Length of output: 2281
two pathsEqualNormalized implementations with divergent trailing-slash handling — no test coverage.
lib/storage.ts:276-278 and lib/runtime-paths.ts:75-88 both normalize paths for comparison but handle trailing separators differently. storage.ts uses normalize() without stripping trailing slashes, while runtime-paths.ts explicitly removes them with regex before comparison. on windows, C:\Users\Neil\.codex\ and C:\Users\Neil\.codex will fail to match in storage.ts but will match in runtime-paths.ts.
both functions handle critical home directory and storage fallback selection logic. runtime-paths.ts:190 checks explicit vs default home detection; storage.ts:318 filters storage candidates. during fallback migration, inconsistent comparisons could cause the system to skip valid candidates or switch homes unexpectedly.
consolidate into a shared utility in lib/ and add regression tests for windows trailing-slash edge cases with vitest.
| async function writeIntentionalResetMarker(storagePath: string): Promise<void> { | ||
| const markerPath = getIntentionalResetMarkerPath(storagePath); | ||
| await fs.writeFile( | ||
| markerPath, | ||
| JSON.stringify({ version: 1, createdAt: Date.now() }), | ||
| "utf-8", | ||
| ); | ||
| } | ||
|
|
||
| async function removeIntentionalResetMarker(storagePath: string): Promise<void> { | ||
| const markerPath = getIntentionalResetMarkerPath(storagePath); | ||
| try { | ||
| await fs.unlink(markerPath); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| log.warn("Failed to remove reset marker", { path: markerPath, error: String(error) }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
consider retry logic for marker operations on windows.
writeIntentionalResetMarker at lib/storage.ts:890-897 and removeIntentionalResetMarker at lib/storage.ts:899-909 don't include the EBUSY/EPERM retry pattern used elsewhere (e.g., copyFileWithRetry, renameFileWithRetry). on windows, antivirus or file indexers can transiently lock files.
the marker write failure is treated as fatal in clearAccounts, which is correct for safety. however, if a transient lock causes removeIntentionalResetMarker to fail silently after save, the marker persists and suppresses restore eligibility until the next successful save.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 890 - 909, writeIntentionalResetMarker and
removeIntentionalResetMarker can fail on Windows due to transient locks
(EBUSY/EPERM); update both functions to use the same retry-on-EBUSY/EPERM
pattern used by copyFileWithRetry/renameFileWithRetry: for
writeIntentionalResetMarker wrap the fs.writeFile call in the retry loop so
transient errors are retried and any final failure still bubbles up (so
clearAccounts remains fatal), and for removeIntentionalResetMarker wrap
fs.unlink in the same retry loop (still ignore ENOENT but only log/warn after
all retries fail); prefer reusing the existing retry helper if present,
otherwise implement the same backoff/retry behavior and reference
markerPath/getIntentionalResetMarkerPath in the implementation.
| try { | ||
| await removeIntentionalResetMarker(path); | ||
| } catch { | ||
| // Best effort cleanup. Saved storage remains authoritative even if marker removal is delayed. | ||
| } |
There was a problem hiding this comment.
missing test coverage for marker removal failure.
the pr description notes "a silent marker-removal-failure path after saveAccounts that lacks test coverage." if removeIntentionalResetMarker fails at lib/storage.ts:1671, the marker persists and subsequent loads will incorrectly report restoreEligible: false until the next successful save.
consider adding a test that mocks marker unlink to fail and verifies:
- save still succeeds
- subsequent load sees the stale marker
- next save clears the marker
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 1670 - 1674, Add a test that simulates
removeIntentionalResetMarker failing during saveAccounts: mock the underlying
unlink used by removeIntentionalResetMarker to throw on the first save, call
saveAccounts and assert it still resolves (save succeeds), then call
loadAccounts and assert the returned state shows the stale marker
(restoreEligible is false), then make the unlink mock succeed (or restore
behavior) and call saveAccounts again and finally call loadAccounts to assert
the marker was cleared (restoreEligible true); reference
removeIntentionalResetMarker, saveAccounts, and loadAccounts when locating code
to mock and verify.
Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
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/storage.ts">
<violation number="1" location="lib/storage.ts:1422">
P1: The reset-marker guard is still stale across awaited migration steps, so a concurrent clear can be bypassed and fallback accounts can be re-persisted.</violation>
<violation number="2" location="lib/storage.ts:1983">
P2: `clearFlaggedAccounts` only clears known backup slots; discovered backup artifacts can still be auto-recovered and revive flagged accounts after clear.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/storage.ts">
<violation number="1" location="lib/storage.ts:1449">
P1: Fallback migration can still race an in-flight intentional reset and repopulate accounts after reset suppression should apply.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
lib/codex-manager.ts (1)
3805-3808:⚠️ Potential issue | 🟡 Minoradd a vitest for the interactive reset branch.
this wiring is now correct, but i still do not see a vitest that drives
runAuthLogin()throughrunActionPanel("Reset Accounts", ...)and proves this menu path keeps callingclearAccounts()rather than drifting back tosaveAccounts(). storage-level recovery tests will not catch that caller regression inlib/codex-manager.ts:3653-3655.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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 3805 - 3808, Add a vitest that exercises the interactive "Reset Accounts" menu path by invoking runActionPanel("Reset Accounts", ...) and driving the flow into runAuthLogin to ensure clearAccounts() is called (and saveAccounts() is not); mock/stub runAuthLogin, clearAccounts, and saveAccounts (and any storage IO) so the test simulates the user confirmation branch, assert clearAccounts was invoked once and saveAccounts was not invoked, and include assertions that any recovery snapshot behavior remains unchanged and no tokens/emails are logged; reference runActionPanel, runAuthLogin, clearAccounts, and saveAccounts when locating the code to hook into for the test.lib/storage.ts (1)
898-917:⚠️ Potential issue | 🟠 Majorretry reset-marker writes and unlinks on windows.
these marker operations still use bare
fs.writeFileandfs.unlink, unlike the retrying file paths inlib/storage.ts:329-377. a transientEPERMorEBUSYwill either abortclearAccounts()unnecessarily or leave a stale marker suppressing recovery until some later save happens to clean it up. the new windows helper intest/storage-flagged.test.ts:17-36does not cover this marker path.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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 898 - 917, The writeIntentionalResetMarker and removeIntentionalResetMarker functions currently use bare fs.writeFile/fs.unlink and must be changed to use the same retry-on-Windows-IO logic used earlier in this file (the retrying helper used around lib/storage.ts:329-377) so transient EPERM/EBUSY/429 errors are retried instead of failing or leaving a stale marker; update writeIntentionalResetMarker to call the retry wrapper when writing the JSON marker and update removeIntentionalResetMarker to retry unlink and only suppress ENOENT, logging failures with safe context (path and error string) but without leaking tokens/emails; also add/adjust the vitest in test/storage-flagged.test.ts to assert retries occur on simulated Windows EPERM/EBUSY scenarios.
🤖 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/storage.ts`:
- Around line 766-814: The current loadAccountsForRestoreAssessment captures
hasIntentionalResetMarker before awaiting loadAccountsFromPath, creating a race;
update loadAccountsForRestoreAssessment to re-check the reset marker after the
await (mirror the closure-based re-check used in loadAccountsInternal: compute
resetMarkerPath, call existsSync(resetMarkerPath) again after
loadAccountsFromPath resolves and base restoreEligible/restoreReason on the
post-await value), ensure annotated/returned values reflect the final marker
state, and add a vitest race regression that concurrently calls
clearAccounts()/saveAccounts() while loadAccountsForRestoreAssessment runs to
assert restoreEligible/restoreReason match the final marker; reference function
names loadAccountsForRestoreAssessment, loadAccountsFromPath,
loadAccountsInternal, clearAccounts/saveAccounts in the test.
- Around line 693-696: selectLatestValidSnapshotPath is returning the first
valid snapshot in scan order rather than the newest one; update
selectLatestValidSnapshotPath to choose the most recent valid snapshot (not the
first) by either iterating the snapshots array in reverse (since discovered
backups are appended in lexicographic filename order) or by sorting/filtering by
the snapshot timestamp field on BackupSnapshotMetadata and returning the max;
ensure callers like getBackupMetadata and getRestoreAssessment.latestSnapshot
then use this corrected function so latestSnapshot points to the newest valid
backup.
- Around line 1802-1846: The reset currently swallows non-ENOENT unlink errors
for the primary storage file (path) which can leave token-bearing state on disk;
modify clearAccounts() so that after writeIntentionalResetMarker(path) any
fs.unlink failure for the primary path (targetPath === path) that is not ENOENT
should log an error and rethrow (or return a rejected promise) to abort the
reset, while preserving the existing non-fatal behavior for the WAL path
(walPath); update the unlink loop or split primary vs WAL handling in
clearAccounts() and ensure tests cover a windows/EBUSY/EPERM regression (see
test/storage-recovery-paths.test.ts and lib/codex-manager.ts usage that prints
the success message) so a failed primary delete prevents the success log path.
- Around line 311-326: getFallbackAccountStoragePaths is building wrong
candidate homes because dirname(getLegacyCodexDir()) yields
"<home>/DevTools/config" instead of the real home; fix by deriving the actual
legacy home with dirname(dirname(legacyRoot)) (call this legacyHome) and ensure
candidateHomes always includes legacyHome even when
inferHomeFromStoragePath(currentPath) returns null; update the candidateHomes
construction (referencing getFallbackAccountStoragePaths, getLegacyCodexDir,
inferHomeFromStoragePath, deduplicatePathList, ACCOUNTS_FILE_NAME) to use
[inferredHome, legacyHome] (or just [legacyHome] when inferredHome is null) so
the generated candidates check the real legacy/default locations, then continue
to deduplicate and filter via pathsEqualNormalized as before.
- Around line 1929-1941: The catch in loadFlaggedAccounts() erroneously falls
through to recoverFlaggedAccountsFromBackups() for any error from
loadFlaggedAccountsFromPath(path); change the logic so recovery is only
attempted when the primary is missing (i.e., when caught error.code ===
"ENOENT"), otherwise rethrow or return null/undefined so backups are not used
for malformed or permission-denied primaries; update the code paths around
loadFlaggedAccountsFromPath, loadFlaggedAccounts(), storageBackupEnabled, and
recoverFlaggedAccountsFromBackups to reflect this gating. Add a vitest
regression that simulates a present but malformed/permission-denied primary file
and asserts that recoverFlaggedAccountsFromBackups is NOT called (mock the
function), and verify logging does not leak sensitive data and that the new
change handles filesystem EBUSY/429 retry scenarios per lib/* guidelines. Ensure
errors other than ENOENT are logged (without tokens/emails) and rethrown or
handled consistently.
---
Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 3805-3808: Add a vitest that exercises the interactive "Reset
Accounts" menu path by invoking runActionPanel("Reset Accounts", ...) and
driving the flow into runAuthLogin to ensure clearAccounts() is called (and
saveAccounts() is not); mock/stub runAuthLogin, clearAccounts, and saveAccounts
(and any storage IO) so the test simulates the user confirmation branch, assert
clearAccounts was invoked once and saveAccounts was not invoked, and include
assertions that any recovery snapshot behavior remains unchanged and no
tokens/emails are logged; reference runActionPanel, runAuthLogin, clearAccounts,
and saveAccounts when locating the code to hook into for the test.
In `@lib/storage.ts`:
- Around line 898-917: The writeIntentionalResetMarker and
removeIntentionalResetMarker functions currently use bare fs.writeFile/fs.unlink
and must be changed to use the same retry-on-Windows-IO logic used earlier in
this file (the retrying helper used around lib/storage.ts:329-377) so transient
EPERM/EBUSY/429 errors are retried instead of failing or leaving a stale marker;
update writeIntentionalResetMarker to call the retry wrapper when writing the
JSON marker and update removeIntentionalResetMarker to retry unlink and only
suppress ENOENT, logging failures with safe context (path and error string) but
without leaking tokens/emails; also add/adjust the vitest in
test/storage-flagged.test.ts to assert retries occur on simulated Windows
EPERM/EBUSY scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1e9eacd-b120-49fc-b10d-86a1d974c18b
📒 Files selected for processing (4)
docs/reference/storage-paths.mdlib/codex-manager.tslib/storage.tstest/storage-flagged.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/reference/storage-paths.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage-flagged.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/storage.ts
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
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="test/storage-recovery-paths.test.ts">
<violation number="1" location="test/storage-recovery-paths.test.ts:595">
P3: Restore the `fs.readFile` spy in a `finally` block so a failing assertion doesn't leak global mocking state into later tests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const path = getFlaggedAccountsPath(); | ||
| const backupPaths = await getAccountsBackupRecoveryCandidatesWithDiscovery(path); | ||
| for (const candidate of [path, ...backupPaths]) { | ||
| try { | ||
| await fs.unlink(candidate); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| log.error("Failed to clear flagged account storage", { | ||
| path: candidate, | ||
| error: String(error), | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Critical bug: clearFlaggedAccounts() can revive flagged accounts from backups after an incomplete deletion.
Unlike clearAccounts() which uses a .reset-intent marker to prevent WAL/backup revival, clearFlaggedAccounts() has no such mechanism. If the deletion loop fails partway through (e.g., EACCES on one backup file), some backup files remain. When loadFlaggedAccounts() runs later, it calls recoverFlaggedAccountsFromBackups() which will restore from those surviving backups, defeating the clear operation.
Fix:
export async function clearFlaggedAccounts(): Promise<void> {
return withStorageLock(async () => {
const path = getFlaggedAccountsPath();
const markerPath = getIntentionalResetMarkerPath(path);
// Write marker first to suppress revival
try {
await fs.mkdir(dirname(path), { recursive: true });
await writeIntentionalResetMarker(path);
} catch (error) {
log.error("Failed to write flagged reset marker", { path, error: String(error) });
throw error;
}
const backupPaths = await getAccountsBackupRecoveryCandidatesWithDiscovery(path);
for (const candidate of [path, ...backupPaths]) {
// ... existing deletion logic
}
});
}Also update loadFlaggedAccounts() to check for the reset marker before attempting backup recovery.
| const path = getFlaggedAccountsPath(); | |
| const backupPaths = await getAccountsBackupRecoveryCandidatesWithDiscovery(path); | |
| for (const candidate of [path, ...backupPaths]) { | |
| try { | |
| await fs.unlink(candidate); | |
| } catch (error) { | |
| const code = (error as NodeJS.ErrnoException).code; | |
| if (code !== "ENOENT") { | |
| log.error("Failed to clear flagged account storage", { | |
| path: candidate, | |
| error: String(error), | |
| }); | |
| } | |
| } | |
| } | |
| }); | |
| const path = getFlaggedAccountsPath(); | |
| const markerPath = getIntentionalResetMarkerPath(path); | |
| // Write marker first to suppress revival | |
| try { | |
| await fs.mkdir(dirname(path), { recursive: true }); | |
| await writeIntentionalResetMarker(path); | |
| } catch (error) { | |
| log.error("Failed to write flagged reset marker", { path, error: String(error) }); | |
| throw error; | |
| } | |
| const backupPaths = await getAccountsBackupRecoveryCandidatesWithDiscovery(path); | |
| for (const candidate of [path, ...backupPaths]) { | |
| try { | |
| await fs.unlink(candidate); | |
| } catch (error) { | |
| const code = (error as NodeJS.ErrnoException).code; | |
| if (code !== "ENOENT") { | |
| log.error("Failed to clear flagged account storage", { | |
| path: candidate, | |
| error: String(error), | |
| }); | |
| } | |
| } | |
| } | |
| }); |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
| const backupPaths = await getAccountsBackupRecoveryCandidatesWithDiscovery(path); | ||
| for (const candidate of [path, ...backupPaths]) { | ||
| try { | ||
| await fs.unlink(candidate); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| log.error("Failed to clear flagged account storage", { | ||
| path: candidate, | ||
| error: String(error), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical inconsistency: clearFlaggedAccounts() does not throw an error when the primary flagged accounts file deletion fails, while clearAccounts() correctly throws at line 1887-1889 when the primary accounts file deletion fails.
This creates a dangerous silent failure scenario:
- Reset marker is written successfully (line 2066)
- Primary flagged accounts file deletion fails
- Function returns without error
- Caller believes reset succeeded
- System is in inconsistent state: marker exists but file also exists with sensitive flagged account data
Fix by adding a throw condition for the primary file:
const backupPaths = await getAccountsBackupRecoveryCandidatesWithDiscovery(path);
for (const candidate of [path, ...backupPaths]) {
try {
await fs.unlink(candidate);
} catch (error) {
const code = (error as NodeJS.ErrnoException).code;
if (code !== "ENOENT") {
log.error("Failed to clear flagged account storage", {
path: candidate,
error: String(error),
});
if (candidate === path) {
throw error;
}
}
}
}This matches the behavior in clearAccounts() and ensures callers are aware when the primary file deletion fails.
| const backupPaths = await getAccountsBackupRecoveryCandidatesWithDiscovery(path); | |
| for (const candidate of [path, ...backupPaths]) { | |
| try { | |
| await fs.unlink(candidate); | |
| } catch (error) { | |
| const code = (error as NodeJS.ErrnoException).code; | |
| if (code !== "ENOENT") { | |
| log.error("Failed to clear flagged account storage", { | |
| path: candidate, | |
| error: String(error), | |
| }); | |
| } | |
| } | |
| const backupPaths = await getAccountsBackupRecoveryCandidatesWithDiscovery(path); | |
| for (const candidate of [path, ...backupPaths]) { | |
| try { | |
| await fs.unlink(candidate); | |
| } catch (error) { | |
| const code = (error as NodeJS.ErrnoException).code; | |
| if (code !== "ENOENT") { | |
| log.error("Failed to clear flagged account storage", { | |
| path: candidate, | |
| error: String(error), | |
| }); | |
| if (candidate === path) { | |
| throw error; | |
| } | |
| } | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/storage.ts">
<violation number="1" location="lib/storage.ts:1987">
P1: Re-check the reset marker after backup recovery await; a concurrent clear can set the marker after the first check, allowing backup data to be returned and persisted.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| } | ||
|
|
||
| if (existsSync(resetMarkerPath) && !existsSync(path)) { |
There was a problem hiding this comment.
P1: Re-check the reset marker after backup recovery await; a concurrent clear can set the marker after the first check, allowing backup data to be returned and persisted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/storage.ts, line 1987:
<comment>Re-check the reset marker after backup recovery await; a concurrent clear can set the marker after the first check, allowing backup data to be returned and persisted.</comment>
<file context>
@@ -1983,6 +1984,10 @@ export async function loadFlaggedAccounts(): Promise<FlaggedAccountStorageV1> {
}
}
+ if (existsSync(resetMarkerPath) && !existsSync(path)) {
+ return empty;
+ }
</file context>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/storage.test.ts (1)
1991-2029:⚠️ Potential issue | 🔴 Criticalgive this describe an isolated storage path.
test/storage.test.ts:1991-2029callssaveAccounts()andclearAccounts()without asetStoragePathDirect(...)orHOMEtest harness. That meansgetStoragePath()falls back tolib/storage.ts:904-909, so these cases can create or delete the runner's real~/.codex/...storage, wal, and reset marker. Mirror the temp-dir setup used by the other storage suites before touching the filesystem here.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 1991 - 2029, This test manipulates the real user storage because it calls saveAccounts() and clearAccounts() without isolating getStoragePath(); fix by setting an isolated temp storage path at the start of the test (use the same temp-dir harness as other storage suites) — call the helper used elsewhere (e.g., setStoragePathDirect(...) or set process.env.HOME to a temp dir) before calling saveAccounts()/clearAccounts(), and ensure teardown restores HOME/setStoragePathDirect and removes the temp dir; reference getStoragePath, saveAccounts, clearAccounts, and setStoragePathDirect to locate where to add the setup/teardown.lib/storage.ts (1)
1615-1645:⚠️ Potential issue | 🔴 Criticalrecheck the reset marker inside the backup recovery loop.
lib/storage.ts:1615-1645only checks.reset-intentbefore the wal path. IfclearAccounts()writes the marker after that but beforeloadAccountsFromPath(backupPath)orpersistMigration, this branch can repopulate token-bearing storage, andlib/storage.ts:1744-1748will then clear the marker on the successful save.test/storage-recovery-paths.test.ts:495-600exercises the reset suppression and wal race, but there is no matching vitest regression for the backup loop.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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 1615 - 1645, When iterating backupCandidates in the recovery loop (functions/methods: getAccountsBackupRecoveryCandidatesWithDiscovery, loadAccountsFromPath, persistMigration), re-check the intentional reset marker (hasIntentionalResetMarker) immediately after loadAccountsFromPath and before applying/persisting the recovered backup; if the marker is now present, skip this backup and continue to the next candidate to avoid re-populating token-bearing storage. Also ensure any logging in this path does not include tokens/emails, and add/adjust a vitest regression that mirrors test/storage-recovery-paths.test.ts (covering the WAL race and the backup loop) and note handling of EBUSY/429 filesystem concurrency in the new test setup or retry queue for persistMigration to avoid races.
🤖 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/storage.ts`:
- Around line 2059-2073: The unlink loop deletes the primary flagged file before
backups which allows a concurrent loadFlaggedAccounts() to recover from a backup
and re-save it; change the deletion order in the block that calls
getFlaggedAccountsPath() and getAccountsBackupRecoveryCandidatesWithDiscovery()
so you delete all backup candidates first (from
getAccountsBackupRecoveryCandidatesWithDiscovery) and only then unlink the
primary path returned by getFlaggedAccountsPath(), and add robust retry/backoff
for filesystem EBUSY/EMFILE on Windows and 429-like transient errors;
update/extend test/storage-flagged.test.ts to include a vitest race regression
that simulates concurrent loadFlaggedAccounts()/saveFlaggedAccounts() to assert
backups are removed before primary is deleted, and ensure logging in the failure
path does not leak tokens or emails.
In `@test/runtime-paths.test.ts`:
- Around line 138-151: The test mocks process.platform = "win32" but uses
path.join to construct the expected candidate and fallback string, which yields
POSIX separators on non-Windows hosts; update the test
(test/runtime-paths.test.ts) to build the expected path using path.win32.join
(or path.win32.normalize) wherever path.join is used in the setup and for the
fallback string so the existsSync mock and the fallback variable match Windows
separators and the getCodexMultiAuthDir() assertion remains deterministic across
runners.
In `@test/storage-recovery-paths.test.ts`:
- Around line 557-599: Add the explicit vi import used by the test harness:
import { vi } from "vitest"; Place this at the top of the test file alongside
other imports so usages like vi.spyOn(...) (in the test that creates readSpy and
calls readSpy.mockRestore()) are backed by the explicit import, matching the
codebase pattern used by other tests.
---
Outside diff comments:
In `@lib/storage.ts`:
- Around line 1615-1645: When iterating backupCandidates in the recovery loop
(functions/methods: getAccountsBackupRecoveryCandidatesWithDiscovery,
loadAccountsFromPath, persistMigration), re-check the intentional reset marker
(hasIntentionalResetMarker) immediately after loadAccountsFromPath and before
applying/persisting the recovered backup; if the marker is now present, skip
this backup and continue to the next candidate to avoid re-populating
token-bearing storage. Also ensure any logging in this path does not include
tokens/emails, and add/adjust a vitest regression that mirrors
test/storage-recovery-paths.test.ts (covering the WAL race and the backup loop)
and note handling of EBUSY/429 filesystem concurrency in the new test setup or
retry queue for persistMigration to avoid races.
In `@test/storage.test.ts`:
- Around line 1991-2029: This test manipulates the real user storage because it
calls saveAccounts() and clearAccounts() without isolating getStoragePath(); fix
by setting an isolated temp storage path at the start of the test (use the same
temp-dir harness as other storage suites) — call the helper used elsewhere
(e.g., setStoragePathDirect(...) or set process.env.HOME to a temp dir) before
calling saveAccounts()/clearAccounts(), and ensure teardown restores
HOME/setStoragePathDirect and removes the temp dir; reference getStoragePath,
saveAccounts, clearAccounts, and setStoragePathDirect to locate where to add the
setup/teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fef834bb-60fb-470c-a298-e41472d97adf
📒 Files selected for processing (6)
lib/runtime-paths.tslib/storage.tstest/runtime-paths.test.tstest/storage-flagged.test.tstest/storage-recovery-paths.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/runtime-paths.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/runtime-paths.test.tstest/storage.test.tstest/storage-flagged.test.tstest/storage-recovery-paths.test.ts
| const path = getFlaggedAccountsPath(); | ||
| const backupPaths = await getAccountsBackupRecoveryCandidatesWithDiscovery(path); | ||
| for (const candidate of [path, ...backupPaths]) { | ||
| try { | ||
| await fs.unlink(candidate); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| log.error("Failed to clear flagged account storage", { | ||
| path: candidate, | ||
| error: String(error), | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
clear flagged backups before the primary, or a concurrent load can revive them.
lib/storage.ts:2059-2073 unlinks the flagged primary first. loadFlaggedAccounts() at lib/storage.ts:1972-1989 treats “primary missing” as permission to recover from backups, so a concurrent load between those two steps can read a backup and saveFlaggedAccounts() it back after the lock releases. test/storage-flagged.test.ts:186-221 and test/storage-flagged.test.ts:267-288 cover the sequential case only; this still needs a vitest race regression.
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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 2059 - 2073, The unlink loop deletes the primary
flagged file before backups which allows a concurrent loadFlaggedAccounts() to
recover from a backup and re-save it; change the deletion order in the block
that calls getFlaggedAccountsPath() and
getAccountsBackupRecoveryCandidatesWithDiscovery() so you delete all backup
candidates first (from getAccountsBackupRecoveryCandidatesWithDiscovery) and
only then unlink the primary path returned by getFlaggedAccountsPath(), and add
robust retry/backoff for filesystem EBUSY/EMFILE on Windows and 429-like
transient errors; update/extend test/storage-flagged.test.ts to include a vitest
race regression that simulates concurrent
loadFlaggedAccounts()/saveFlaggedAccounts() to assert backups are removed before
primary is deleted, and ensure logging in the failure path does not leak tokens
or emails.
| it("treats default Windows CODEX_HOME with a trailing separator as the default root", async () => { | ||
| const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32"); | ||
| try { | ||
| homedir.mockReturnValue("C:\\Users\\Neil"); | ||
| process.env.CODEX_HOME = "C:\\Users\\Neil\\.codex\\"; | ||
| const fallback = "C:\\Users\\Neil\\DevTools\\config\\codex\\multi-auth"; | ||
|
|
||
| existsSync.mockImplementation((candidate: unknown) => { | ||
| if (typeof candidate !== "string") return false; | ||
| return candidate === path.join(fallback, "openai-codex-accounts.json"); | ||
| }); | ||
|
|
||
| const mod = await import("../lib/runtime-paths.js"); | ||
| expect(mod.getCodexMultiAuthDir()).toBe(fallback); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node - <<'NODE'
const path = require("node:path");
console.log("host platform:", process.platform);
console.log("path.join:", path.join("C:\\Users\\Neil\\.codex\\", "multi-auth"));
console.log("path.win32.join:", path.win32.join("C:\\Users\\Neil\\.codex\\", "multi-auth"));
NODERepository: ndycode/codex-multi-auth
Length of output: 181
🏁 Script executed:
cat -n test/runtime-paths.test.ts | sed -n '138,155p'Repository: ndycode/codex-multi-auth
Length of output: 927
🏁 Script executed:
cat -n lib/runtime-paths.ts | sed -n '188,202p'Repository: ndycode/codex-multi-auth
Length of output: 768
🏁 Script executed:
# Also check the full test setup around line 138
cat -n test/runtime-paths.test.ts | sed -n '130,170p'Repository: ndycode/codex-multi-auth
Length of output: 1868
🏁 Script executed:
head -n 30 lib/runtime-paths.tsRepository: ndycode/codex-multi-auth
Length of output: 893
🏁 Script executed:
# Check the import of join
rg "^import.*join" lib/runtime-paths.tsRepository: ndycode/codex-multi-auth
Length of output: 119
🏁 Script executed:
# Also check what getResolvedUserHomeDir does and its path handling
sed -n '1,100p' lib/runtime-paths.ts | grep -A 20 "getResolvedUserHomeDir"Repository: ndycode/codex-multi-auth
Length of output: 1518
🏁 Script executed:
# Check getCodexHomeDir to understand what's returned
sed -n '170,180p' lib/runtime-paths.tsRepository: ndycode/codex-multi-auth
Length of output: 710
🏁 Script executed:
# Verify the full getCodexHomeDir function
rg -A 15 "function getCodexHomeDir" lib/runtime-paths.tsRepository: ndycode/codex-multi-auth
Length of output: 788
🏁 Script executed:
# Check where exactly the mocked existsSync is called in the implementation
rg -B 5 -A 5 "existsSync" lib/runtime-paths.ts | head -40Repository: ndycode/codex-multi-auth
Length of output: 904
🏁 Script executed:
# Look at the full getCodexMultiAuthDir implementation
sed -n '188,220p' lib/runtime-paths.tsRepository: ndycode/codex-multi-auth
Length of output: 1143
🏁 Script executed:
# Check if there are other uses of join in getCodexMultiAuthDir
sed -n '188,230p' lib/runtime-paths.tsRepository: ndycode/codex-multi-auth
Length of output: 1463
🏁 Script executed:
# Verify how hasAccountsStorage builds its paths
rg -A 10 "function hasAccountsStorage" lib/runtime-paths.tsRepository: ndycode/codex-multi-auth
Length of output: 371
🏁 Script executed:
# Check if the implementation file imports join and how it's used
head -5 lib/runtime-paths.tsRepository: ndycode/codex-multi-auth
Length of output: 281
🏁 Script executed:
# Verify: does the implementation check process.platform before calling join?
rg -B 3 "join\(" lib/runtime-paths.ts | head -30Repository: ndycode/codex-multi-auth
Length of output: 885
🏁 Script executed:
# Check the imports in the test file
head -20 test/runtime-paths.test.tsRepository: ndycode/codex-multi-auth
Length of output: 537
🏁 Script executed:
cat > /tmp/test-paths.js << 'EOF'
const path = require("node:path");
// Simulate Linux host
console.log("=== Linux host, mock platform win32 ===");
const fallback = "C:\\Users\\Neil\\DevTools\\config\\codex\\multi-auth";
const codexHome = "C:\\Users\\Neil\\.codex\\";
// What the test builds as the mock key
const mockKey = path.join(fallback, "openai-codex-accounts.json");
console.log("Test mock key (path.join):", mockKey);
// What the implementation would build
const implPrimary = path.join(codexHome, "multi-auth");
console.log("Impl primary (path.join):", implPrimary);
const implCandidate = path.join(implPrimary, "openai-codex-accounts.json");
console.log("Impl candidate (path.join):", implCandidate);
// With path.win32
const mockKeyWin32 = path.win32.join(fallback, "openai-codex-accounts.json");
console.log("Test mock key (path.win32.join):", mockKeyWin32);
const implPrimaryWin32 = path.win32.join(codexHome, "multi-auth");
console.log("Impl primary (path.win32.join):", implPrimaryWin32);
EOF
node /tmp/test-paths.jsRepository: ndycode/codex-multi-auth
Length of output: 526
🏁 Script executed:
# Verify getFallbackCodexHomeDirs includes the DevTools path
rg -A 5 "function getFallbackCodexHomeDirs" lib/runtime-paths.tsRepository: ndycode/codex-multi-auth
Length of output: 284
🏁 Script executed:
# Check the pathsEqualNormalized function to understand path comparison
rg -A 15 "function pathsEqualNormalized" lib/runtime-paths.tsRepository: ndycode/codex-multi-auth
Length of output: 655
use path.win32.join in this windows-only case.
test/runtime-paths.test.ts:138-151 mocks process.platform to "win32" but calls path.join() at line 147 to build the mock key. on a linux or posix host, path.join() produces mixed separators: C:\Users\Neil\DevTools\config\codex\multi-auth/openai-codex-accounts.json. the test hardcodes fallback as all backslashes. when the implementation builds candidates via lib/runtime-paths.ts:193-195 using the host-platform join(), it produces mismatched separators, breaking the string equality check in the mock. replace path.join() with path.win32.join() or path.win32.normalize() in the test's mock setup and fallback reference so windows filesystem behavior stays deterministic across all runners.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/runtime-paths.test.ts` around lines 138 - 151, The test mocks
process.platform = "win32" but uses path.join to construct the expected
candidate and fallback string, which yields POSIX separators on non-Windows
hosts; update the test (test/runtime-paths.test.ts) to build the expected path
using path.win32.join (or path.win32.normalize) wherever path.join is used in
the setup and for the fallback string so the existsSync mock and the fallback
variable match Windows separators and the getCodexMultiAuthDir() assertion
remains deterministic across runners.
| it("suppresses WAL recovery when a reset marker appears while the WAL is being read", async () => { | ||
| const walPayload = { | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [ | ||
| { | ||
| refreshToken: "racing-refresh", | ||
| accountId: "racing-account", | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }; | ||
| const walContent = JSON.stringify(walPayload); | ||
| const walEntry = { | ||
| version: 1, | ||
| createdAt: Date.now(), | ||
| path: storagePath, | ||
| checksum: sha256(walContent), | ||
| content: walContent, | ||
| }; | ||
|
|
||
| await fs.writeFile(`${storagePath}.wal`, JSON.stringify(walEntry), "utf-8"); | ||
|
|
||
| const originalReadFile = fs.readFile.bind(fs); | ||
| const originalWriteFile = fs.writeFile.bind(fs); | ||
| const readSpy = vi.spyOn(fs, "readFile").mockImplementation(async (...args) => { | ||
| const [targetPath] = args; | ||
| if (targetPath === `${storagePath}.wal`) { | ||
| await originalWriteFile( | ||
| `${storagePath}.reset-intent`, | ||
| JSON.stringify({ version: 1, createdAt: Date.now() }), | ||
| "utf-8", | ||
| ); | ||
| } | ||
| return originalReadFile(...args); | ||
| }); | ||
|
|
||
| const reloaded = await loadAccounts(); | ||
| expect(reloaded?.accounts).toHaveLength(0); | ||
| expect(getRestoreEligibility(reloaded).restoreReason).toBe("intentional-reset"); | ||
|
|
||
| readSpy.mockRestore(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
fd 'vitest.config.*' . -t f -x sed -n '1,200p' {}
fd 'vite.config.*' . -t f -x sed -n '1,200p' {}
rg -n '\bglobals\s*:\s*(true|false)\b' .
rg -n '^import \{[^}]*\bvi\b[^}]*\} from "vitest";$' test/Repository: ndycode/codex-multi-auth
Length of output: 7179
add explicit vi import to match codebase pattern
test/storage-recovery-paths.test.ts uses vi.spyOn but doesn't import vi from vitest. globals: true is enabled in vitest.config.ts:17, so this works at runtime, but all 57 other test files explicitly import vi. add the import for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage-recovery-paths.test.ts` around lines 557 - 599, Add the explicit
vi import used by the test harness: import { vi } from "vitest"; Place this at
the top of the test file alongside other imports so usages like vi.spyOn(...)
(in the test that creates readSpy and calls readSpy.mockRestore()) are backed by
the explicit import, matching the codebase pattern used by other tests.
| for (const candidate of [path, ...backupPaths]) { | ||
| try { | ||
| await fs.unlink(candidate); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| log.error("Failed to clear flagged account storage", { | ||
| path: candidate, | ||
| error: String(error), | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
clearFlaggedAccounts silently swallows primary deletion failure, leaving stale tokens accessible
clearAccounts throws when fs.unlink(path) fails with a non-ENOENT code (e.g. Windows EPERM/EBUSY), which alerts the caller and prevents a half-cleared state from being silently trusted. clearFlaggedAccounts does the opposite — every error is caught and logged, including failure to delete the primary file.
the combined effect: antivirus or a file lock causes EPERM on fs.unlink(path); the reset marker is written, but the primary still exists. loadFlaggedAccounts only checks the marker inside the ENOENT branch (existsSync(resetMarkerPath) && !existsSync(path)). when the primary file is present and readable, the marker is never consulted — so the stale flagged accounts are returned to callers with no suppression and no error.
loadAccountsInternal handles the equivalent case by annotating the result with restoreEligible: false even when the primary loads OK and the marker is present. loadFlaggedAccounts has no such fallback because FlaggedAccountStorageV1 carries no restoreEligible field.
the immediate fix is to throw on primary deletion failure, matching clearAccounts:
for (const candidate of [path, ...backupPaths]) {
try {
await fs.unlink(candidate);
} catch (error) {
const code = (error as NodeJS.ErrnoException).code;
if (code !== "ENOENT") {
log.error("Failed to clear flagged account storage", {
path: candidate,
error: String(error),
});
if (candidate === path) {
throw error;
}
}
}
}no vitest test currently covers the path where clearFlaggedAccounts writes the marker but fs.unlink(path) fails with EPERM — a companion test verifying clearFlaggedAccounts() rejects and that loadFlaggedAccounts() still returns empty is needed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/storage.ts
Line: 2079-2091
Comment:
**`clearFlaggedAccounts` silently swallows primary deletion failure, leaving stale tokens accessible**
`clearAccounts` throws when `fs.unlink(path)` fails with a non-`ENOENT` code (e.g. Windows `EPERM`/`EBUSY`), which alerts the caller and prevents a half-cleared state from being silently trusted. `clearFlaggedAccounts` does the opposite — every error is caught and logged, including failure to delete the primary file.
the combined effect: antivirus or a file lock causes `EPERM` on `fs.unlink(path)`; the reset marker is written, but the primary still exists. `loadFlaggedAccounts` only checks the marker inside the `ENOENT` branch (`existsSync(resetMarkerPath) && !existsSync(path)`). when the primary file is present and readable, the marker is never consulted — so the stale flagged accounts are returned to callers with no suppression and no error.
`loadAccountsInternal` handles the equivalent case by annotating the result with `restoreEligible: false` even when the primary loads OK and the marker is present. `loadFlaggedAccounts` has no such fallback because `FlaggedAccountStorageV1` carries no `restoreEligible` field.
the immediate fix is to throw on primary deletion failure, matching `clearAccounts`:
```typescript
for (const candidate of [path, ...backupPaths]) {
try {
await fs.unlink(candidate);
} catch (error) {
const code = (error as NodeJS.ErrnoException).code;
if (code !== "ENOENT") {
log.error("Failed to clear flagged account storage", {
path: candidate,
error: String(error),
});
if (candidate === path) {
throw error;
}
}
}
}
```
no vitest test currently covers the path where `clearFlaggedAccounts` writes the marker but `fs.unlink(path)` fails with `EPERM` — a companion test verifying `clearFlaggedAccounts()` rejects and that `loadFlaggedAccounts()` still returns empty is needed.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/storage.ts (1)
1615-1645:⚠️ Potential issue | 🔴 Criticalre-check the reset marker inside backup recovery.
lib/storage.ts:1570-1576andlib/storage.ts:1588-1594guard theenoentand wal paths, butlib/storage.ts:1615-1645never re-checks the marker after awaitingloadAccountsFromPath(backupPath). ifclearAccounts()lands in that window, this branch can still persist a.bakor discovered snapshot back into the canonical file after an intentional reset. please mirror the wal guard here and add a vitest race regression besidetest/storage-recovery-paths.test.ts:522-602for both.bakand discovered backups.suggested guard
if (storageBackupEnabled) { const backupCandidates = await getAccountsBackupRecoveryCandidatesWithDiscovery(path); for (const backupPath of backupCandidates) { + if (hasIntentionalResetMarker()) { + return { + ...createEmptyAccountStorage(), + restoreEligible: false, + restoreReason: "intentional-reset", + }; + } try { const backup = await loadAccountsFromPath(backupPath); + if (hasIntentionalResetMarker()) { + return { + ...createEmptyAccountStorage(), + restoreEligible: false, + restoreReason: "intentional-reset", + }; + } if (backup.schemaErrors.length > 0) {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.
♻️ Duplicate comments (2)
lib/storage.ts (2)
915-933:⚠️ Potential issue | 🟠 Majoradd the windows retry path to reset-marker io.
lib/storage.ts:915-933is still the only reset-path io here that skips theEPERM/EBUSYbackoff already used for backup copies and renames. on windows, a transient lock can makeclearAccounts()orclearFlaggedAccounts()fail spuriously on marker creation, or leave a stale.reset-intentmarker behind after a successful save. please reuse the same retry/backoff pattern here and cover marker write/unlink failures with vitest.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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 915 - 933, The writeIntentionalResetMarker and removeIntentionalResetMarker functions currently do not apply the Windows-safe retry/backoff used elsewhere; modify both functions to reuse the existing retry/backoff utility (the same pattern used for backup copies/renames) to retry on transient errors like EPERM and EBUSY with exponential backoff and a max attempts limit, wrapping the fs.writeFile and fs.unlink calls so they reattempt on EPERM/EBUSY (and still treat ENOENT in removeIntentionalResetMarker as non-fatal), add vitest unit tests that simulate transient EPERM/EBUSY failures then success (and a final failure) for both write and unlink to assert correct retry behavior and that no sensitive data is logged, and ensure any log.warn calls do not leak tokens/emails.
311-327:⚠️ Potential issue | 🟠 Majorre-verify the legacy home derivation here.
lib/storage.ts:312-317still deriveslegacyHomewith a singledirname(getLegacyCodexDir()). iflib/runtime-paths.tsstill returns<home>/DevTools/config/codex, this builds fallback candidates under<home>/DevTools/config/DevTools/...and skips fallback migration for explicit canonical roots. please confirm the helper contract and add a vitest that exercises a non-default canonical path against the default legacy roots.#!/bin/bash set -e echo "=== getLegacyCodexDir implementation ===" rg -n "getLegacyCodexDir" lib/runtime-paths.ts -A12 -B4 echo echo "=== fallback path builder ===" sed -n '311,327p' lib/storage.ts echo echo "=== related recovery tests ===" rg -n "CODEX_HOME|getFallbackAccountStoragePaths|fallback account storage|legacy" test/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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 311 - 327, The fallback builder getFallbackAccountStoragePaths currently computes legacyHome as dirname(getLegacyCodexDir()) which can double-up path segments if getLegacyCodexDir() already returns a root like "<home>/DevTools/config/codex"; change the derivation to detect whether getLegacyCodexDir() already points to the canonical codex directory and only use dirname when you actually need the user home (e.g., compare basename segments or use a helper like inferHomeFromStoragePath on getLegacyCodexDir()); update getFallbackAccountStoragePaths to use that corrected legacyHome when creating candidates and ensure pathsEqualNormalized(currentPath, candidate) logic remains. Add a vitest that calls getFallbackAccountStoragePaths with a non-default canonical path (simulate getLegacyCodexDir returning "<home>/DevTools/config/codex") and asserts the expected fallback candidates include "<home>/.codex/..." and not malformed "<home>/DevTools/config/DevTools/..." paths; reference getFallbackAccountStoragePaths, getLegacyCodexDir, and inferHomeFromStoragePath in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/storage.ts`:
- Around line 626-692: The current loadFlaggedAccountsFromPath uses
normalizeFlaggedStorage which silently accepts malformed JSON as empty storage;
change to a strict validator instead: implement/invoke a strict parse/validate
function (e.g., parseFlaggedStorageStrict or validateFlaggedStorageSchema)
inside loadFlaggedAccountsFromPath and throw on schema mismatch (do not fall
back to normalizeFlaggedStorage), update describeFlaggedSnapshot to rely on that
strict parse so malformed .bak files produce valid: false, and update
recoverFlaggedAccountsFromBackups to continue iterating candidates on
schema-validation failures (only stop on ENOENT or a real load success), and add
a vitest in test/storage-flagged.test.ts that places a malformed .bak followed
by a valid .bak.1 to assert recovery skips the malformed file and restores from
the valid backup; reference functions loadFlaggedAccountsFromPath,
describeFlaggedSnapshot, recoverFlaggedAccountsFromBackups,
normalizeFlaggedStorage, and the new parse/validate function in your changes.
In `@test/storage-recovery-paths.test.ts`:
- Around line 751-777: The test uses a time-based sleep to order snapshots;
replace the setTimeout-based delay with explicit file mtime updates using
fs.utimes() so the newerManualPath has a later modification time than
olderManualPath before calling getBackupMetadata(); specifically, after writing
both files (or immediately after writing olderManualPath), call fs.utimes on
olderManualPath and newerManualPath to set increasing mtime values (e.g., use
Date.now()/1000 or fixed offsets) so the
expect(metadata.accounts.latestValidPath) assertion deterministically picks
newerManualPath without relying on wall-clock sleeps.
---
Duplicate comments:
In `@lib/storage.ts`:
- Around line 915-933: The writeIntentionalResetMarker and
removeIntentionalResetMarker functions currently do not apply the Windows-safe
retry/backoff used elsewhere; modify both functions to reuse the existing
retry/backoff utility (the same pattern used for backup copies/renames) to retry
on transient errors like EPERM and EBUSY with exponential backoff and a max
attempts limit, wrapping the fs.writeFile and fs.unlink calls so they reattempt
on EPERM/EBUSY (and still treat ENOENT in removeIntentionalResetMarker as
non-fatal), add vitest unit tests that simulate transient EPERM/EBUSY failures
then success (and a final failure) for both write and unlink to assert correct
retry behavior and that no sensitive data is logged, and ensure any log.warn
calls do not leak tokens/emails.
- Around line 311-327: The fallback builder getFallbackAccountStoragePaths
currently computes legacyHome as dirname(getLegacyCodexDir()) which can
double-up path segments if getLegacyCodexDir() already returns a root like
"<home>/DevTools/config/codex"; change the derivation to detect whether
getLegacyCodexDir() already points to the canonical codex directory and only use
dirname when you actually need the user home (e.g., compare basename segments or
use a helper like inferHomeFromStoragePath on getLegacyCodexDir()); update
getFallbackAccountStoragePaths to use that corrected legacyHome when creating
candidates and ensure pathsEqualNormalized(currentPath, candidate) logic
remains. Add a vitest that calls getFallbackAccountStoragePaths with a
non-default canonical path (simulate getLegacyCodexDir returning
"<home>/DevTools/config/codex") and asserts the expected fallback candidates
include "<home>/.codex/..." and not malformed
"<home>/DevTools/config/DevTools/..." paths; reference
getFallbackAccountStoragePaths, getLegacyCodexDir, and inferHomeFromStoragePath
in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c69f645-f630-49a4-b095-73ed13587b01
📒 Files selected for processing (3)
lib/storage.tstest/storage-flagged.test.tstest/storage-recovery-paths.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
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-flagged.test.tstest/storage-recovery-paths.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/storage.ts
🔇 Additional comments (1)
test/storage-flagged.test.ts (1)
223-265: good regression on the unreadable-primary path.
test/storage-flagged.test.ts:223-265exercises theEPERMbranch inlib/storage.ts:1977-1994and proves backups stay suppressed while the primary still exists. that is the right shape of vitest coverage for this recovery rule.
| async function loadFlaggedAccountsFromPath(path: string): Promise<FlaggedAccountStorageV1> { | ||
| const content = await fs.readFile(path, "utf-8"); | ||
| const data = JSON.parse(content) as unknown; | ||
| return normalizeFlaggedStorage(data); | ||
| } | ||
|
|
||
| async function describeFlaggedSnapshot( | ||
| path: string, | ||
| kind: BackupSnapshotKind, | ||
| index?: number, | ||
| ): Promise<BackupSnapshotMetadata> { | ||
| const base = await describePathStats(path, kind, index); | ||
| if (!base.exists) { | ||
| return { ...base, valid: false }; | ||
| } | ||
|
|
||
| try { | ||
| const storage = await loadFlaggedAccountsFromPath(path); | ||
| return { | ||
| ...base, | ||
| valid: true, | ||
| version: storage.version, | ||
| flaggedCount: storage.accounts.length, | ||
| }; | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| log.warn("Failed to inspect flagged account snapshot", { | ||
| path, | ||
| error: String(error), | ||
| }); | ||
| } | ||
| return { ...base, valid: false }; | ||
| } | ||
| } | ||
|
|
||
| async function recoverFlaggedAccountsFromBackups(path: string): Promise<FlaggedAccountStorageV1 | null> { | ||
| const candidates = await getAccountsBackupRecoveryCandidatesWithDiscovery(path); | ||
| for (const candidate of candidates) { | ||
| try { | ||
| const recovered = await loadFlaggedAccountsFromPath(candidate); | ||
| log.warn("Recovered flagged account storage from backup file", { | ||
| path, | ||
| backupPath: candidate, | ||
| accounts: recovered.accounts.length, | ||
| }); | ||
| try { | ||
| await saveFlaggedAccounts(recovered); | ||
| } catch (persistError) { | ||
| log.warn("Failed to persist recovered flagged account storage", { | ||
| path, | ||
| error: String(persistError), | ||
| }); | ||
| } | ||
| return recovered; | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| log.warn("Failed to load flagged account backup", { | ||
| path: candidate, | ||
| error: String(error), | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
treat malformed flagged snapshots as invalid, not as empty storage.
lib/storage.ts:626-630 uses normalizeFlaggedStorage(), which returns { version: 1, accounts: [] } for any json object that misses the flagged schema. that makes lib/storage.ts:643-649 report malformed backup files as valid: true, and lib/storage.ts:665-680 stops recovery on the first bad candidate instead of continuing to an older real backup. please use a strict parse path for flagged backup inspection/recovery and add a vitest with a malformed .bak followed by a valid .bak.1 in test/storage-flagged.test.ts:223-376.
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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 626 - 692, The current
loadFlaggedAccountsFromPath uses normalizeFlaggedStorage which silently accepts
malformed JSON as empty storage; change to a strict validator instead:
implement/invoke a strict parse/validate function (e.g.,
parseFlaggedStorageStrict or validateFlaggedStorageSchema) inside
loadFlaggedAccountsFromPath and throw on schema mismatch (do not fall back to
normalizeFlaggedStorage), update describeFlaggedSnapshot to rely on that strict
parse so malformed .bak files produce valid: false, and update
recoverFlaggedAccountsFromBackups to continue iterating candidates on
schema-validation failures (only stop on ENOENT or a real load success), and add
a vitest in test/storage-flagged.test.ts that places a malformed .bak followed
by a valid .bak.1 to assert recovery skips the malformed file and restores from
the valid backup; reference functions loadFlaggedAccountsFromPath,
describeFlaggedSnapshot, recoverFlaggedAccountsFromBackups,
normalizeFlaggedStorage, and the new parse/validate function in your changes.
| it("prefers the newest valid discovered snapshot in backup metadata", async () => { | ||
| const olderManualPath = `${storagePath}.manual-older`; | ||
| const newerManualPath = `${storagePath}.manual-newer`; | ||
|
|
||
| await fs.writeFile( | ||
| olderManualPath, | ||
| JSON.stringify({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [{ refreshToken: "older-refresh", accountId: "older", addedAt: 1, lastUsed: 1 }], | ||
| }), | ||
| "utf-8", | ||
| ); | ||
| await new Promise((resolve) => setTimeout(resolve, 20)); | ||
| await fs.writeFile( | ||
| newerManualPath, | ||
| JSON.stringify({ | ||
| version: 3, | ||
| activeIndex: 0, | ||
| accounts: [{ refreshToken: "newer-refresh", accountId: "newer", addedAt: 2, lastUsed: 2 }], | ||
| }), | ||
| "utf-8", | ||
| ); | ||
|
|
||
| const metadata = await getBackupMetadata(); | ||
| expect(metadata.accounts.latestValidPath).toBe(newerManualPath); | ||
| }); |
There was a problem hiding this comment.
avoid wall-clock sleeps for snapshot ordering.
test/storage-recovery-paths.test.ts:764 relies on a 20ms delay to change mtimeMs. that is flaky on coarse-grained or contended filesystems, especially on windows ci. set the mtimes explicitly with fs.utimes() so this regression stays deterministic.
one deterministic pattern
await fs.writeFile(
olderManualPath,
JSON.stringify({
version: 3,
activeIndex: 0,
accounts: [{ refreshToken: "older-refresh", accountId: "older", addedAt: 1, lastUsed: 1 }],
}),
"utf-8",
);
- await new Promise((resolve) => setTimeout(resolve, 20));
await fs.writeFile(
newerManualPath,
JSON.stringify({
version: 3,
activeIndex: 0,
accounts: [{ refreshToken: "newer-refresh", accountId: "newer", addedAt: 2, lastUsed: 2 }],
}),
"utf-8",
);
+
+ const olderTime = new Date("2026-03-01T00:00:00.000Z");
+ const newerTime = new Date("2026-03-01T00:00:01.000Z");
+ await fs.utimes(olderManualPath, olderTime, olderTime);
+ await fs.utimes(newerManualPath, newerTime, newerTime);As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("prefers the newest valid discovered snapshot in backup metadata", async () => { | |
| const olderManualPath = `${storagePath}.manual-older`; | |
| const newerManualPath = `${storagePath}.manual-newer`; | |
| await fs.writeFile( | |
| olderManualPath, | |
| JSON.stringify({ | |
| version: 3, | |
| activeIndex: 0, | |
| accounts: [{ refreshToken: "older-refresh", accountId: "older", addedAt: 1, lastUsed: 1 }], | |
| }), | |
| "utf-8", | |
| ); | |
| await new Promise((resolve) => setTimeout(resolve, 20)); | |
| await fs.writeFile( | |
| newerManualPath, | |
| JSON.stringify({ | |
| version: 3, | |
| activeIndex: 0, | |
| accounts: [{ refreshToken: "newer-refresh", accountId: "newer", addedAt: 2, lastUsed: 2 }], | |
| }), | |
| "utf-8", | |
| ); | |
| const metadata = await getBackupMetadata(); | |
| expect(metadata.accounts.latestValidPath).toBe(newerManualPath); | |
| }); | |
| it("prefers the newest valid discovered snapshot in backup metadata", async () => { | |
| const olderManualPath = `${storagePath}.manual-older`; | |
| const newerManualPath = `${storagePath}.manual-newer`; | |
| await fs.writeFile( | |
| olderManualPath, | |
| JSON.stringify({ | |
| version: 3, | |
| activeIndex: 0, | |
| accounts: [{ refreshToken: "older-refresh", accountId: "older", addedAt: 1, lastUsed: 1 }], | |
| }), | |
| "utf-8", | |
| ); | |
| await fs.writeFile( | |
| newerManualPath, | |
| JSON.stringify({ | |
| version: 3, | |
| activeIndex: 0, | |
| accounts: [{ refreshToken: "newer-refresh", accountId: "newer", addedAt: 2, lastUsed: 2 }], | |
| }), | |
| "utf-8", | |
| ); | |
| const olderTime = new Date("2026-03-01T00:00:00.000Z"); | |
| const newerTime = new Date("2026-03-01T00:00:01.000Z"); | |
| await fs.utimes(olderManualPath, olderTime, olderTime); | |
| await fs.utimes(newerManualPath, newerTime, newerTime); | |
| const metadata = await getBackupMetadata(); | |
| expect(metadata.accounts.latestValidPath).toBe(newerManualPath); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/storage-recovery-paths.test.ts` around lines 751 - 777, The test uses a
time-based sleep to order snapshots; replace the setTimeout-based delay with
explicit file mtime updates using fs.utimes() so the newerManualPath has a later
modification time than olderManualPath before calling getBackupMetadata();
specifically, after writing both files (or immediately after writing
olderManualPath), call fs.utimes on olderManualPath and newerManualPath to set
increasing mtime values (e.g., use Date.now()/1000 or fixed offsets) so the
expect(metadata.accounts.latestValidPath) assertion deterministically picks
newerManualPath without relying on wall-clock sleeps.
1 participant
Summary
#51CODEX_HOMEcanonical, fix restore assessment/reset ordering, and prevent WAL-backed token revival after resetWhy
This is the storage-focused replacement for the mixed-scope portion of
#51.Supersedes #51.
Verification
npm exec vitest run test/storage-recovery-paths.test.ts test/storage.test.ts test/runtime-paths.test.ts test/storage-flagged.test.tsnpm run typechecknpm run lintnpm testnpm run buildNotes
.bak*recovery artifacts while deleting the WAL on intentional resetauth-ui-controller/settings-persistence/ Ink dashboard extraction from#51Summary by cubic
Strengthens account and flagged-account recovery and intentional reset flows. Adds restore assessment and backup metadata, closes races that could revive tokens after a reset, and keeps the canonical root steady with normalized
CODEX_HOMEhandling.New Features
getRestoreAssessment()andgetBackupMetadata()with deterministic snapshots for accounts and flagged accounts; validates WAL entries; ignores cache-like artifacts and.reset-intent; selects the latest valid snapshot.loadAccounts()now returnsrestoreEligibleandrestoreReasonfor missing, empty, and intentional reset states; migrates fallback storage from legacy and DevTools roots into the canonical root only after a successful persist (never into project-scoped storage); pins the canonical multi-auth root whenCODEX_HOMEis explicitly non-default with normalized path comparisons across platforms.Bug Fixes
loadAccountsInternalbefore migrations and again before WAL and backup recovery to close revival races (including WindowsEPERM); CLI reset now callsclearAccounts()and clarifies that recovery snapshots remain after a clear.clearFlaggedAccounts(), deleting.bak*and discovered backups, creating rotating backups on save, and recovering only when the primary is missing; emits snapshot metadata for flagged-account state.Written for commit 0b5fbcd. 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 closes nine previously-flagged windows filesystem concurrency and token-revival races in the multi-auth storage layer — marker write is now authoritative before any deletion,
hasIntentionalResetMarkeris a liveexistsSyncclosure rather than a snapshot, WAL/backup recovery rechecks the marker after everyawaityield point,.reset-intentis excluded from discovered-backup candidates,loadFlaggedAccountsreturnsemptyon non-ENOENTerrors, andpathsEqualNormalizedinruntime-paths.tsnow callsnormalize()on posix paths. one inconsistency remains:clearFlaggedAccountsswallows non-ENOENTfailures when deleting the primary file and doesn't throw, whileloadFlaggedAccountsonly consults the reset marker in theENOENTpath — so a windowsEPERM/EBUSYon the flagged-accounts primary leaves stale tokens accessible after a supposedly authoritative clear.clearAccountsis hardened: marker write is atomic, primary deletion failure throws, WAL deletion failure is logged-only (suppressed on next load via marker)loadAccountsInternalre-evaluateshasIntentionalResetMarker()as a live function after everyawait, closing the toctou window from the previously captured boolean snapshotmigrateFallbackAccountStorageIfNeededaborts inside the storage lock if a reset marker appears between the pre-migration check and the persist, preventing migrated tokens from outliving a concurrentclearAccountsgetRestoreAssessmentsequencesloadAccountsForRestoreAssessmentbeforegetBackupMetadata(no longerPromise.all), removing the directory-scan vs. marker-removal raceclearFlaggedAccountsdoesn't throw when primary deletion fails — the reset marker is written but the primary survives;loadFlaggedAccountsdoesn't check the marker when the primary file loads successfully, so stale flagged accounts can be read back; no vitest test covers this path (see inline comment)removeWithRetrytest helper reduces flakyafterEachteardown failures on windowsgetBackupMetadata()/getRestoreAssessment()apis andBackupSnapshotMetadatatypes are well-structured; thelatestValidPathin backup metadata is not reset-marker-aware, so callers must gate onrestoreEligiblebefore acting on snapshot dataConfidence Score: 3/5
clearFlaggedAccountsprimary-deletion-failure gap needs a follow-up before the flagged-accounts reset path can be considered fully hardened on windowsclearFlaggedAccounts(line 2079) and the marker-check logic inloadFlaggedAccounts(line 1987)Important Files Changed
clearFlaggedAccountsstill silently swallows primary deletion failure whileloadFlaggedAccountsdoesn't consult the reset marker when the primary exists, leaving a Windows EPERM/EBUSY revival path open for flagged accountspathsEqualNormalizedwithnormalize()on both platforms and theisExplicitNonDefaultHomeearly-return ingetCodexMultiAuthDir; the POSIX branch now correctly callsnormalize()before stripping the trailing slash, closing the double-slash/dot-segment mismatch flagged in #51fs.rmonEBUSY/EPERM/ENOTEMPTY/EACCESwith exponential back-off; used inafterEachteardown to avoid flaky test failures on Windows where directory removal can race against OS file locksclearFlaggedAccountsprimary deletion failure leaving the marker written while the primary survivesSequence Diagram
sequenceDiagram participant Caller participant clearAccounts participant clearFlaggedAccounts participant loadAccountsInternal participant loadFlaggedAccounts participant Disk Note over clearAccounts: Hardened reset flow Caller->>clearAccounts: clearAccounts() clearAccounts->>Disk: writeIntentionalResetMarker() [throws on failure] clearAccounts->>Disk: unlink(primary) [throws on non-ENOENT] clearAccounts->>Disk: unlink(WAL) [logs only] clearAccounts-->>Caller: resolved Note over loadAccountsInternal: Live marker re-check after every await Caller->>loadAccountsInternal: loadAccounts() loadAccountsInternal->>Disk: existsSync(marker) [live closure] alt marker + no primary loadAccountsInternal-->>Caller: {restoreEligible:false, reason:"intentional-reset"} else primary exists loadAccountsInternal->>Disk: loadAccountsFromPath() alt hasIntentionalResetMarker() after read loadAccountsInternal-->>Caller: annotated {restoreEligible:false} else normal loadAccountsInternal-->>Caller: accounts end end Note over clearFlaggedAccounts: Gap — no throw on primary delete failure Caller->>clearFlaggedAccounts: clearFlaggedAccounts() clearFlaggedAccounts->>Disk: writeIntentionalResetMarker() [throws on failure ✓] clearFlaggedAccounts->>Disk: unlink(primary) [logs only — no throw ⚠️] Note over Disk: primary survives on Windows EPERM Note over loadFlaggedAccounts: Marker not checked when primary loads OK Caller->>loadFlaggedAccounts: loadFlaggedAccounts() loadFlaggedAccounts->>Disk: readFile(primary) Disk-->>loadFlaggedAccounts: stale flagged accounts loadFlaggedAccounts-->>Caller: stale accounts returned ⚠️ (no restoreEligible)Comments Outside Diff (1)
lib/storage.ts, line 1979-1994 (link)clearFlaggedAccountshas no reset marker — backup auto-revives flagged accountsclearFlaggedAccountsdeletes only the primary file.saveFlaggedAccountsnow callscreateRotatingAccountsBackupbefore every write, so a.bakfile is always left on disk.loadFlaggedAccountsnow callsrecoverFlaggedAccountsFromBackupswhenever the primary is missing. The combined effect: everyclearFlaggedAccounts()→loadFlaggedAccounts()round-trip silently revives the flagged account list from the backup, defeating the clear operation.There is no
flagged.reset-intentmarker analog to prevent this revival, and no vitest coverage that exercises theclearFlaggedAccounts → loadFlaggedAccountspath to catch this.The same reset-marker guard that protects accounts should be applied here. At minimum,
clearFlaggedAccountsshould either write a flagged-specific marker before deletion, or explicitly delete the backup files it intends to supersede:windows risk: on windows
EPERM/EBUSYfrom antivirus during backup deletion leaves the backup in place; a subsequent load still revives it. needs explicit test coverage for this path.Prompt To Fix With AI
Last reviewed commit: 0b5fbcd
Context used: