Conversation
Use async telemetry log rotation, serialize flagged-account secret rotation, and prefer the freshest credential set during conflict recovery. Co-authored-by: Codex <noreply@openai.com>
📝 WalkthroughWalkthroughchanges to account credential merging add a new selectCredentialMergeSource helper that compares expiration times to decide whether to preserve current or prefer incoming credentials. storage.ts refactors flagged account load/save operations to separate unlocked variants and coordinate them under locking. telemetry.ts converts log rotation from sync to async file operations. tests verify conflict resolution during merges and concurrent updates during rotation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
review notesconcurrency & locking risks:
missing regression test coverage:
windows edge cases:
async/await hazards:
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@lib/storage.ts`:
- Around line 2156-2162: The current save block uses a single fs.rename which
can fail on Windows with transient EPERM/EBUSY; replace the direct await
fs.rename(tempPath, path) in the flagged-save block that uses
normalizeFlaggedStorage and cloneFlaggedStorageForPersist with the project’s
existing retry helper (the retry function defined near lib/storage.ts:454) to
attempt the rename multiple times with backoff and only surface the error after
retries exhaust; ensure the retry only targets transient filesystem errors
(EPERM/EBUSY) and preserves the same tempPath/path semantics, add/adjust tests
(vitest) to cover Windows-like EBUSY/429 scenarios for rotation/concurrent
updates, and avoid logging any sensitive data (tokens/emails) during retries or
errors.
In `@lib/telemetry.ts`:
- Around line 161-176: The unlink/rename calls in the telemetry rotation code
(around the block using fs.unlink(target) and fs.rename(source, target)) must
retry on transient Windows filesystem errors (EBUSY/EPERM/EAGAIN) instead of
failing immediately; implement a small retry-with-exponential-backoff-and-jitter
helper (e.g., retryFsOp or useTransientRetry) and use it to wrap the existing
unlink and rename calls, consult and reuse isMissingFsError for
non-transient-not-found handling, and ensure non-transient errors are still
rethrown; update recordTelemetryEvent-related flows (where failures were
previously swallowed) and add/adjust Vitest unit tests to cover
EBUSY/EPERM/EAGAIN transient failures and successful retry, making sure no logs
leak tokens/emails.
In `@test/accounts-edge.test.ts`:
- Around line 611-617: The test currently checks credential fields for the
merged account but omits an explicit identity assertion; update the test so
after finding mergedAccount from retriedPayload.accounts (the variable
`mergedAccount` in accounts-edge.test.ts) you assert mergedAccount.accountId ===
"account-identity-1" to ensure identity stability and catch merge regressions
(this ties to the merge behavior in lib/accounts.ts around the merge logic at
~810); keep the assertion deterministic using the vitest expect API already in
the file and do not mock secrets or remove existing credential assertions.
In `@test/storage-flagged.test.ts`:
- Around line 245-260: Move the process.env mutations for
CODEX_AUTH_ENCRYPTION_KEY and CODEX_AUTH_PREVIOUS_ENCRYPTION_KEY into the
try/finally scope so the original values saved in previousEncryptionKey and
previousPreviousKey are always restored; specifically, in the test surrounding
saveFlaggedAccounts(...) wrap the env setup (setting
process.env.CODEX_AUTH_ENCRYPTION_KEY and deleting
process.env.CODEX_AUTH_PREVIOUS_ENCRYPTION_KEY) right after entering try and
perform the restoration of previousEncryptionKey/previousPreviousKey in the
finally block to guarantee cleanup even if setup or saveFlaggedAccounts throws.
In `@test/telemetry.test.ts`:
- Around line 179-203: Add a deterministic Vitest regression test (in
test/telemetry.test.ts) that mirrors the existing "preserves concurrent events
across async log rotation" case but specifically simulates Windows busy-file
rename/unlink behavior: use vitest.spyOn(fs, "rename") and vitest.spyOn(fs,
"unlink") (or only the one used by the rotation code) to throw an error with
code "EBUSY" exactly once and then delegate to the original implementation on
subsequent calls, call recordTelemetryEvent repeatedly (same pattern as the
existing test), await queryTelemetryEvents and assert all events are preserved,
and finally restore spies; target the rotation logic exercised in
lib/telemetry.ts (around the rename/unlink logic referenced at line ~161) and
ensure the test uses Vitest APIs (no nondeterministic timing or real secret
mocking) so it reliably reproduces the Windows busy-file retry path and verifies
no event loss.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 466cac5d-f709-462c-9db3-c83d104baf21
📒 Files selected for processing (6)
lib/accounts.tslib/storage.tslib/telemetry.tstest/accounts-edge.test.tstest/storage-flagged.test.tstest/telemetry.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/accounts.tslib/telemetry.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/telemetry.test.tstest/accounts-edge.test.tstest/storage-flagged.test.ts
🧬 Code graph analysis (3)
test/telemetry.test.ts (1)
lib/telemetry.ts (4)
configureTelemetry(240-242)recordTelemetryEvent(252-277)queryTelemetryEvents(279-299)getTelemetryLogPath(248-250)
test/accounts-edge.test.ts (1)
lib/accounts.ts (1)
AccountManager(105-1029)
test/storage-flagged.test.ts (1)
lib/storage.ts (4)
saveFlaggedAccounts(2177-2179)getFlaggedAccountsPath(961-963)rotateStoredSecretEncryption(2284-2314)loadFlaggedAccounts(2173-2175)
🔇 Additional comments (3)
lib/accounts.ts (1)
857-926: credential-source selection looks correct and is covered in both directions.the merge gate in
lib/accounts.ts:857plusselectCredentialMergeSourceinlib/accounts.ts:907matches the regression coverage attest/accounts-edge.test.ts:512andtest/accounts-edge.test.ts:567. nice fix for stale-disk overwrite behavior.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.test/storage-flagged.test.ts (1)
262-307: good race reproduction for flagged rotation serialization.injecting a concurrent
saveFlaggedAccountsfromtest/storage-flagged.test.ts:273while rotation is reading is a solid regression for the lock-serialization fix inlib/storage.ts:2301.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.lib/storage.ts (1)
2301-2308: single-lock flagged rotation path closes the in-process read/write toctou window.wrapping
loadFlaggedAccountsUnlocked+saveFlaggedAccountsUnlockedinside one lock atlib/storage.ts:2301aligns with the regression intest/storage-flagged.test.ts:244and removes the prior unlocked read/write gap.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.
| try { | ||
| await fs.mkdir(dirname(path), { recursive: true }); | ||
| const normalized = normalizeFlaggedStorage(storage); | ||
| const content = JSON.stringify(cloneFlaggedStorageForPersist(normalized), null, 2); | ||
| await fs.writeFile(tempPath, content, { encoding: "utf-8", mode: 0o600 }); | ||
| await fs.rename(tempPath, path); | ||
| } catch (error) { |
There was a problem hiding this comment.
use retrying rename for flagged saves on windows locks.
lib/storage.ts:2161 uses a single fs.rename. transient EPERM/EBUSY can fail flagged writes during rotation and concurrent updates. reuse the existing retry helper at lib/storage.ts:454.
proposed patch
- await fs.rename(tempPath, path);
+ await renameFileWithRetry(tempPath, 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.
📝 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.
| try { | |
| await fs.mkdir(dirname(path), { recursive: true }); | |
| const normalized = normalizeFlaggedStorage(storage); | |
| const content = JSON.stringify(cloneFlaggedStorageForPersist(normalized), null, 2); | |
| await fs.writeFile(tempPath, content, { encoding: "utf-8", mode: 0o600 }); | |
| await fs.rename(tempPath, path); | |
| } catch (error) { | |
| try { | |
| await fs.mkdir(dirname(path), { recursive: true }); | |
| const normalized = normalizeFlaggedStorage(storage); | |
| const content = JSON.stringify(cloneFlaggedStorageForPersist(normalized), null, 2); | |
| await fs.writeFile(tempPath, content, { encoding: "utf-8", mode: 0o600 }); | |
| await renameFileWithRetry(tempPath, path); | |
| } catch (error) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/storage.ts` around lines 2156 - 2162, The current save block uses a
single fs.rename which can fail on Windows with transient EPERM/EBUSY; replace
the direct await fs.rename(tempPath, path) in the flagged-save block that uses
normalizeFlaggedStorage and cloneFlaggedStorageForPersist with the project’s
existing retry helper (the retry function defined near lib/storage.ts:454) to
attempt the rename multiple times with backoff and only surface the error after
retries exhaust; ensure the retry only targets transient filesystem errors
(EPERM/EBUSY) and preserves the same tempPath/path semantics, add/adjust tests
(vitest) to cover Windows-like EBUSY/429 scenarios for rotation/concurrent
updates, and avoid logging any sensitive data (tokens/emails) during retries or
errors.
| if (i === telemetryConfig.maxFiles - 1) { | ||
| try { | ||
| await fs.unlink(target); | ||
| } catch (error) { | ||
| if (!isMissingFsError(error)) { | ||
| throw error; | ||
| } | ||
| } | ||
| } | ||
| if (existsSync(source)) { | ||
| renameSync(source, target); | ||
| try { | ||
| await fs.rename(source, target); | ||
| } catch (error) { | ||
| if (!isMissingFsError(error)) { | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
add retry/backoff for windows busy-file rotation failures.
fs.unlink and fs.rename in lib/telemetry.ts:163 and lib/telemetry.ts:171 fail hard on transient EBUSY/EPERM/EAGAIN. that bubbles into recordTelemetryEvent and gets swallowed at lib/telemetry.ts:274, dropping events.
proposed patch
+function isRetryableFsBusyError(error: unknown): boolean {
+ const code = (error as NodeJS.ErrnoException | undefined)?.code;
+ return code === "EBUSY" || code === "EPERM" || code === "EAGAIN";
+}
+
+async function withFsRetry(task: () => Promise<void>): Promise<void> {
+ for (let attempt = 0; attempt < 5; attempt += 1) {
+ try {
+ await task();
+ return;
+ } catch (error) {
+ if (!isRetryableFsBusyError(error) || attempt === 4) {
+ throw error;
+ }
+ await new Promise((resolve) => setTimeout(resolve, 20 * 2 ** attempt));
+ }
+ }
+}
+
async function rotateLogsIfNeeded(): Promise<void> {
@@
- try {
- await fs.unlink(target);
+ try {
+ await withFsRetry(() => fs.unlink(target));
} catch (error) {
@@
- try {
- await fs.rename(source, target);
+ try {
+ await withFsRetry(() => fs.rename(source, target));
} catch (error) {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/telemetry.ts` around lines 161 - 176, The unlink/rename calls in the
telemetry rotation code (around the block using fs.unlink(target) and
fs.rename(source, target)) must retry on transient Windows filesystem errors
(EBUSY/EPERM/EAGAIN) instead of failing immediately; implement a small
retry-with-exponential-backoff-and-jitter helper (e.g., retryFsOp or
useTransientRetry) and use it to wrap the existing unlink and rename calls,
consult and reuse isMissingFsError for non-transient-not-found handling, and
ensure non-transient errors are still rethrown; update
recordTelemetryEvent-related flows (where failures were previously swallowed)
and add/adjust Vitest unit tests to cover EBUSY/EPERM/EAGAIN transient failures
and successful retry, making sure no logs leak tokens/emails.
| const mergedAccount = retriedPayload.accounts.find( | ||
| (account) => account.accountId === "account-identity-1", | ||
| ); | ||
| expect(mergedAccount?.refreshToken).toBe("refresh-fresh"); | ||
| expect(mergedAccount?.accessToken).toBe("access-fresh"); | ||
| expect(mergedAccount?.expiresAt).toBe(freshExpiresAt); | ||
| }); |
There was a problem hiding this comment.
assert account id stability in this regression.
the new case verifies credential fields but not identity stability. add an explicit accountId assertion so identity merge regressions fail fast. see test/accounts-edge.test.ts:611 and lib/accounts.ts:810.
proposed patch
const mergedAccount = retriedPayload.accounts.find(
(account) => account.accountId === "account-identity-1",
);
+ expect(mergedAccount?.accountId).toBe("account-identity-1");
expect(mergedAccount?.refreshToken).toBe("refresh-fresh");
expect(mergedAccount?.accessToken).toBe("access-fresh");
expect(mergedAccount?.expiresAt).toBe(freshExpiresAt);As per coding guidelines test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mergedAccount = retriedPayload.accounts.find( | |
| (account) => account.accountId === "account-identity-1", | |
| ); | |
| expect(mergedAccount?.refreshToken).toBe("refresh-fresh"); | |
| expect(mergedAccount?.accessToken).toBe("access-fresh"); | |
| expect(mergedAccount?.expiresAt).toBe(freshExpiresAt); | |
| }); | |
| const mergedAccount = retriedPayload.accounts.find( | |
| (account) => account.accountId === "account-identity-1", | |
| ); | |
| expect(mergedAccount?.accountId).toBe("account-identity-1"); | |
| expect(mergedAccount?.refreshToken).toBe("refresh-fresh"); | |
| expect(mergedAccount?.accessToken).toBe("access-fresh"); | |
| expect(mergedAccount?.expiresAt).toBe(freshExpiresAt); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/accounts-edge.test.ts` around lines 611 - 617, The test currently checks
credential fields for the merged account but omits an explicit identity
assertion; update the test so after finding mergedAccount from
retriedPayload.accounts (the variable `mergedAccount` in accounts-edge.test.ts)
you assert mergedAccount.accountId === "account-identity-1" to ensure identity
stability and catch merge regressions (this ties to the merge behavior in
lib/accounts.ts around the merge logic at ~810); keep the assertion
deterministic using the vitest expect API already in the file and do not mock
secrets or remove existing credential assertions.
| const previousEncryptionKey = process.env.CODEX_AUTH_ENCRYPTION_KEY; | ||
| const previousPreviousKey = process.env.CODEX_AUTH_PREVIOUS_ENCRYPTION_KEY; | ||
| process.env.CODEX_AUTH_ENCRYPTION_KEY = "0123456789abcdef0123456789abcdef"; | ||
| delete process.env.CODEX_AUTH_PREVIOUS_ENCRYPTION_KEY; | ||
|
|
||
| await saveFlaggedAccounts({ | ||
| version: 1, | ||
| accounts: [ | ||
| { | ||
| refreshToken: "flagged-alpha", | ||
| flaggedAt: 1, | ||
| addedAt: 1, | ||
| lastUsed: 1, | ||
| }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
move env mutation inside the try/finally restoration scope.
process.env is changed before the try starts in test/storage-flagged.test.ts:247-260. if setup throws there, keys are not restored and later tests can flake.
proposed patch
- process.env.CODEX_AUTH_ENCRYPTION_KEY = "0123456789abcdef0123456789abcdef";
- delete process.env.CODEX_AUTH_PREVIOUS_ENCRYPTION_KEY;
-
- await saveFlaggedAccounts({
- version: 1,
- accounts: [
- {
- refreshToken: "flagged-alpha",
- flaggedAt: 1,
- addedAt: 1,
- lastUsed: 1,
- },
- ],
- });
+ try {
+ process.env.CODEX_AUTH_ENCRYPTION_KEY = "0123456789abcdef0123456789abcdef";
+ delete process.env.CODEX_AUTH_PREVIOUS_ENCRYPTION_KEY;
+
+ await saveFlaggedAccounts({
+ version: 1,
+ accounts: [
+ {
+ refreshToken: "flagged-alpha",
+ flaggedAt: 1,
+ addedAt: 1,
+ lastUsed: 1,
+ },
+ ],
+ });
- let concurrentSavePromise: Promise<void> | null = null;
+ let concurrentSavePromise: Promise<void> | null = null;
// ... existing test body ...
- try {
- const result = await rotateStoredSecretEncryption();
+ const result = await rotateStoredSecretEncryption();
// ...
- } finally {
+ } finally {
readFileSpy.mockRestore();
// env restore stays 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-flagged.test.ts` around lines 245 - 260, Move the process.env
mutations for CODEX_AUTH_ENCRYPTION_KEY and CODEX_AUTH_PREVIOUS_ENCRYPTION_KEY
into the try/finally scope so the original values saved in previousEncryptionKey
and previousPreviousKey are always restored; specifically, in the test
surrounding saveFlaggedAccounts(...) wrap the env setup (setting
process.env.CODEX_AUTH_ENCRYPTION_KEY and deleting
process.env.CODEX_AUTH_PREVIOUS_ENCRYPTION_KEY) right after entering try and
perform the restoration of previousEncryptionKey/previousPreviousKey in the
finally block to guarantee cleanup even if setup or saveFlaggedAccounts throws.
| it("preserves concurrent events across async log rotation", async () => { | ||
| configureTelemetry({ maxFileSizeBytes: 220, maxFiles: 16 }); | ||
|
|
||
| await Promise.all( | ||
| Array.from({ length: 12 }, (_, index) => | ||
| recordTelemetryEvent({ | ||
| source: index % 2 === 0 ? "plugin" : "cli", | ||
| event: `rotation.concurrent.${index}`, | ||
| outcome: index % 3 === 0 ? "failure" : "success", | ||
| details: { | ||
| index, | ||
| message: "x".repeat(96), | ||
| }, | ||
| }), | ||
| ), | ||
| ); | ||
|
|
||
| const events = await queryTelemetryEvents({ limit: 50 }); | ||
|
|
||
| expect(events).toHaveLength(12); | ||
| expect(events.map((event) => event.event).sort()).toEqual( | ||
| Array.from({ length: 12 }, (_, index) => `rotation.concurrent.${index}`).sort(), | ||
| ); | ||
| expect(existsSync(`${getTelemetryLogPath()}.1`)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
add a windows busy-file rotation regression case.
this test covers concurrent ordering, but it does not exercise windows lock behavior. add a vitest case that spies fs.rename/fs.unlink to throw EBUSY once, then succeed, and assert no event loss. see test/telemetry.test.ts:179 and lib/telemetry.ts:161.
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/telemetry.test.ts` around lines 179 - 203, Add a deterministic Vitest
regression test (in test/telemetry.test.ts) that mirrors the existing "preserves
concurrent events across async log rotation" case but specifically simulates
Windows busy-file rename/unlink behavior: use vitest.spyOn(fs, "rename") and
vitest.spyOn(fs, "unlink") (or only the one used by the rotation code) to throw
an error with code "EBUSY" exactly once and then delegate to the original
implementation on subsequent calls, call recordTelemetryEvent repeatedly (same
pattern as the existing test), await queryTelemetryEvents and assert all events
are preserved, and finally restore spies; target the rotation logic exercised in
lib/telemetry.ts (around the rename/unlink logic referenced at line ~161) and
ensure the test uses Vitest APIs (no nondeterministic timing or real secret
mocking) so it reliably reproduces the Windows busy-file retry path and verifies
no event loss.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export async function loadFlaggedAccounts(): Promise<FlaggedAccountStorageV1> { | ||
| return loadFlaggedAccountsUnlocked(); | ||
| } |
There was a problem hiding this comment.
loadFlaggedAccounts exports the unlocked variant without guarding — any future caller doing a read-modify-write cycle risks a TOCTOU race.
The rotateStoredSecretEncryption fix is solid (both read and write under withStorageLock), but loadFlaggedAccounts() is now just a passthrough to the unguarded loadFlaggedAccountsUnlocked(). if external code does:
const accounts = await loadFlaggedAccounts(); // no lock held
// concurrent saveFlaggedAccounts can fire here
await saveFlaggedAccounts(mergeOrModify(accounts)); // writes stale snapshot...the concurrent write is silently lost. on windows this is especially risky because antivirus i/o can extend the TOCTOU window substantially.
consider adding explicit jsdoc to loadFlaggedAccounts documenting that callers needing a read-modify-write cycle must hold withStorageLock externally, or expose a locked variant as the public API to prevent future misuse.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/storage.ts
Line: 2173-2175
Comment:
`loadFlaggedAccounts` exports the unlocked variant without guarding — any future caller doing a read-modify-write cycle risks a TOCTOU race.
The `rotateStoredSecretEncryption` fix is solid (both read and write under `withStorageLock`), but `loadFlaggedAccounts()` is now just a passthrough to the unguarded `loadFlaggedAccountsUnlocked()`. if external code does:
```ts
const accounts = await loadFlaggedAccounts(); // no lock held
// concurrent saveFlaggedAccounts can fire here
await saveFlaggedAccounts(mergeOrModify(accounts)); // writes stale snapshot
```
...the concurrent write is silently lost. on windows this is especially risky because antivirus i/o can extend the TOCTOU window substantially.
consider adding explicit jsdoc to `loadFlaggedAccounts` documenting that callers needing a read-modify-write cycle must hold `withStorageLock` externally, or expose a locked variant as the public API to prevent future misuse.
How can I resolve this? If you propose a fix, please make it concise.
| if (incomingExpiresAt === undefined && currentExpiresAt === undefined) { | ||
| return "incoming"; | ||
| } |
There was a problem hiding this comment.
when both credentials lack expiresAt, the code returns "incoming" (disk token), which contradicts the PR goal to "prefer the freshest credential set."
the PR description says fresh tokens are preferred during conflict recovery, but when both tokens lack expiry timestamps, preferring incoming (the on-disk snapshot) can silently regress non-expiring api-key style credentials to stale disk tokens regardless of recency.
current is the in-memory merged value (fresher), while incoming is the on-disk snapshot (potentially stale). when timestamps are absent on both, preferring current is safer and aligns with the stated goal.
suggest:
| if (incomingExpiresAt === undefined && currentExpiresAt === undefined) { | |
| return "incoming"; | |
| } | |
| if (incomingExpiresAt === undefined && currentExpiresAt === undefined) { | |
| return "current"; | |
| } |
also missing: no vitest coverage exists for this tie-breaking path, which changed behavior and carries token safety risk.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/accounts.ts
Line: 916-918
Comment:
when both credentials lack `expiresAt`, the code returns `"incoming"` (disk token), which contradicts the PR goal to "prefer the freshest credential set."
the PR description says fresh tokens are preferred during conflict recovery, but when both tokens lack expiry timestamps, preferring `incoming` (the on-disk snapshot) can silently regress non-expiring api-key style credentials to stale disk tokens regardless of recency.
`current` is the in-memory merged value (fresher), while `incoming` is the on-disk snapshot (potentially stale). when timestamps are absent on both, preferring `current` is safer and aligns with the stated goal.
suggest:
```suggestion
if (incomingExpiresAt === undefined && currentExpiresAt === undefined) {
return "current";
}
```
also missing: no vitest coverage exists for this tie-breaking path, which changed behavior and carries token safety risk.
How can I resolve this? If you propose a fix, please make it concise.| try { | ||
| await fs.rename(source, target); | ||
| } catch (error) { | ||
| if (!isMissingFsError(error)) { | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
isMissingFsError only catches ENOENT when renaming intermediate archive slots. on windows/ntfs, if a target file is held open by antivirus or another process, fs.rename throws EPERM instead, which propagates out uncaught.
since rotateLogsIfNeeded() is called from within recordTelemetryEvent's outer catch { } (line 274-276), the error gets swallowed silently — the telemetry event for that call is lost permanently because the appendFile line never executes.
the oldest archive (line 161-168) is explicitly unlinked first, mitigating this for that slot, but intermediate renames (i < maxFiles - 1) are vulnerable. on busy windows systems with aggressive antivirus, this can cause systematic telemetry loss under log rotation.
consider extending isMissingFsError to include EPERM and EACCES:
function isMissingFsError(error: unknown): boolean {
const code = (error as NodeJS.ErrnoException | undefined)?.code;
return code === "ENOENT" || code === "EPERM" || code === "EACCES";
}also consider adding a regression test that mocks intermediate rename to reject with EPERM to verify graceful handling.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/telemetry.ts
Line: 170-176
Comment:
`isMissingFsError` only catches `ENOENT` when renaming intermediate archive slots. on windows/ntfs, if a target file is held open by antivirus or another process, `fs.rename` throws `EPERM` instead, which propagates out uncaught.
since `rotateLogsIfNeeded()` is called from within `recordTelemetryEvent`'s outer `catch { }` (line 274-276), the error gets swallowed silently — the telemetry event for that call is lost permanently because the `appendFile` line never executes.
the oldest archive (line 161-168) is explicitly unlinked first, mitigating this for that slot, but intermediate renames (i < maxFiles - 1) are vulnerable. on busy windows systems with aggressive antivirus, this can cause systematic telemetry loss under log rotation.
consider extending `isMissingFsError` to include `EPERM` and `EACCES`:
```ts
function isMissingFsError(error: unknown): boolean {
const code = (error as NodeJS.ErrnoException | undefined)?.code;
return code === "ENOENT" || code === "EPERM" || code === "EACCES";
}
```
also consider adding a regression test that mocks intermediate rename to reject with `EPERM` to verify graceful handling.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
Context
Follow-up to merged integration PR #46 and review #46 (review).
Validation
npm testnpm run typechecknpm run buildnpm run lint:tsnpm run clean:repo:checkNotes
npm run lintstill shells out to a barebiomeexecutable in this environment; the TypeScript lint gate passed locally.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 addresses three correctness issues flagged in the pr #46 review: async telemetry log rotation, flagged-account secret rotation TOCTOU, and stale-disk token preference during conflict merge.
lib/storage.ts:rotateStoredSecretEncryptioncorrectly wraps bothloadFlaggedAccountsUnlockedandsaveFlaggedAccountsUnlockedunder onewithStorageLockcall, closing the TOCTOU window for that operation. however, the publicloadFlaggedAccountsexport is now an unguarded alias without documentation about locking contracts — any future caller doing a read-modify-write cycle without holdingwithStorageLockexternally will silently lose concurrent writes.lib/accounts.ts:selectCredentialMergeSourceadds expiry-based credential preference during conflict merge. the tie-breaking case when both credentials lackexpiresAtnow returns"incoming"— preferring the disk token — which contradicts the stated PR goal to prefer fresher credentials. no vitest coverage exists for this tie-breaking path, which changes behavior and carries token safety risk.lib/telemetry.ts: the async rotation correctly usesawait fs.stat/unlink/renameand handlesENOENTgracefully. however, intermediate archive renames (log.1 → log.2, etc.) only catchENOENT; on windows, antivirus or file-lock holds can causeEPERMwhich propagates uncaught and is swallowed by the outer error handler, causing that event'sappendFileto be skipped silently.tests: regression tests for concurrent rotation, concurrent flagged-account saves, and fresh-credential merge are solid additions. missing coverage: the no-expiry tie-breaking case in credential merge, and windows
EPERMmid-rotation for intermediate archive slots.Confidence Score: 2/5
lib/accounts.ts(credential merge tie-breaking behavior change) andlib/telemetry.ts(windows EPERM in intermediate archive rename) require attention. consider either fixing the issues or adding explicit test coverage for both scenarios before merge.Important Files Changed
rotateStoredSecretEncryptionis correct and closes the TOCTOU window for secret rotation. however, the publicloadFlaggedAccountsexport now lacks documentation about external locking requirements — callers attempting a read-modify-write cycle without external lock acquisition will silently lose concurrent writes, especially risky on windows with antivirus i/o delays.expiresAtnow prefersincoming(disk), contradicting the PR goal to prefer fresher tokens. this silently regresses non-expiring api-key style credentials to potentially stale disk tokens. no test coverage for this behavior change creates token safety risk; consider returning"current"(in-memory) instead to align with stated goals.awaiton fs operations andENOENThandling. intermediate archive renames do not handleEPERM/EACCES, so windows antivirus locks can cause uncaught errors that propagate into the outer catch handler, silently losing telemetry events. extend error handling or add regression test for windows lock scenario to prevent event loss on desktop deployments.saveFlaggedAccountsqueued during rotation does not lose its writes after the lock is released.expiresAt" tie-breaking case that now returns"incoming", which is a behavior change not yet validated.EPERMscenario for intermediate archive slots that could cause event loss under antivirus contention.Sequence Diagram
sequenceDiagram participant Caller participant rotateStoredSecretEncryption participant withStorageLock participant loadFlaggedAccountsUnlocked participant saveFlaggedAccountsUnlocked participant ConcurrentWriter Caller->>rotateStoredSecretEncryption: rotateStoredSecretEncryption() rotateStoredSecretEncryption->>withStorageLock: acquire lock withStorageLock-->>rotateStoredSecretEncryption: lock held rotateStoredSecretEncryption->>loadFlaggedAccountsUnlocked: read flagged accounts (no sub-lock) loadFlaggedAccountsUnlocked-->>rotateStoredSecretEncryption: { accounts: [alpha] } Note over ConcurrentWriter: concurrent save queued via saveFlaggedAccounts ConcurrentWriter->>withStorageLock: withStorageLock (BLOCKS — lock held) rotateStoredSecretEncryption->>saveFlaggedAccountsUnlocked: write re-encrypted [alpha] saveFlaggedAccountsUnlocked-->>rotateStoredSecretEncryption: done rotateStoredSecretEncryption->>withStorageLock: release lock withStorageLock-->>ConcurrentWriter: lock acquired ConcurrentWriter->>saveFlaggedAccountsUnlocked: write [alpha, beta] saveFlaggedAccountsUnlocked-->>ConcurrentWriter: done ConcurrentWriter->>withStorageLock: release lock Note over Caller: final state: [alpha, beta] — no writes lostLast reviewed commit: 6d8c362
Context used:
dashboard- What: Every code change must explain how it defends against Windows filesystem concurrency bugs and ... (source)