test: add boundary regression tests for storage, oauth poll abort, and prompt refresh dedupe#35
test: add boundary regression tests for storage, oauth poll abort, and prompt refresh dedupe#35
Conversation
- add storage identity fallback test for malformed .git metadata - add OAuth poll abort test for deterministic close behavior - add prompt refresh dedupe test for concurrent stale cache reads Co-authored-by: Codex <noreply@openai.com>
📝 WalkthroughThis is a minor, low-risk, test-only PR that increases coverage for three boundary behaviors (storage root detection, OAuth polling abort, and prompt-cache refresh dedupe). No production code changes, no security or data-loss risks introduced, and regression tests were added and validated locally. Changes
Validation
Risk & Rollback
Reviewer focus / architectural notes
walkthroughthree new unit tests added validating concurrent cache refresh deduplication, .git edge-case path resolution, and oauth server cleanup when closing during active polling. changes
sequence diagram(s)(silently skipped) estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes notes
🚥 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)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/codex-prompts.test.ts`:
- Around line 258-265: The test currently only checks mockFetch hit count at a
single instant which allows a late second refresh to slip through; update the
test around resolvePromptText and mockedWriteFile to assert stability by first
waiting for mockFetch to reach 2 (using mockFetch and resolvePromptText), then
ensure no further calls occur by advancing or waiting a deterministic short
window (use vi.useFakeTimers/vi.advanceTimersByTime or an awaited stable wait)
and re-assert mockFetch remains at 2 and mockedWriteFile was called; reference
the test symbols mockFetch, resolvePromptText, mockedWriteFile and tighten
verification to cover the dedupe logic in lib/prompts/codex.ts (the
refresh/dedupe block around lines 236-246) so the test deterministically fails
on late double-refreshes.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Run ID: 28b5a36a-5be6-4f50-8964-03360e78244e
📒 Files selected for processing (3)
test/codex-prompts.test.tstest/paths.test.tstest/server.unit.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 (1)
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/server.unit.test.tstest/paths.test.tstest/codex-prompts.test.ts
🧬 Code graph analysis (2)
test/paths.test.ts (1)
lib/storage/paths.ts (1)
resolveProjectStorageIdentityRoot(256-298)
test/codex-prompts.test.ts (1)
lib/prompts/codex.ts (1)
getCodexInstructions(204-293)
🔇 Additional comments (2)
test/paths.test.ts (1)
657-671: good regression for the non-file.gitedge case.this is deterministic and directly validates the early-return guard in
lib/storage/paths.ts:263-267, including the no-read safety check attest/paths.test.ts:670. windows filesystem coverage remains intact viatest/paths.test.ts:293-579.test/server.unit.test.ts (1)
303-326: good regression for poll-loop abort concurrency.
test/server.unit.test.ts:303-326cleanly reproduces close-during-poll and verifieswaitForCoderesolvesnullwithout timeout-noise behavior.
- replace instantaneous fetch-count check with stable post-refresh assertion window - verify fetch count remains fixed after refresh persistence completes Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/codex-prompts.test.ts`:
- Around line 258-271: Replace the Promise.resolve() spin-loops in the test (the
loops that wait for background ops around mockFetch, mockedWriteFile and
resolvePromptText) with vitest's vi.waitFor so the test doesn't rely on
microtask scheduling; specifically, change the blocks that assert mockFetch has
been called twice, that mockedWriteFile has been called after
resolvePromptText("fresh deduped content"), and the final microtask drain into
vi.waitFor callbacks that poll the respective expectations
(expect(mockFetch).toHaveBeenCalledTimes(2),
expect(mockedWriteFile).toHaveBeenCalled(), and any final settles) using
vi.waitFor's default timeout/interval (or an explicit small timeout if needed)
to make the test deterministic instead of the current spin-loop approach.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Run ID: 31a84a7e-6565-408e-8a23-de30aa67a25f
📒 Files selected for processing (1)
test/codex-prompts.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 (1)
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/codex-prompts.test.ts
🧬 Code graph analysis (1)
test/codex-prompts.test.ts (1)
lib/prompts/codex.ts (1)
getCodexInstructions(204-293)
- replace microtask spin-loops with vi.waitFor polling - keep final no-extra-fetch stability assertion after refresh Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/codex-prompts.test.ts (1)
271-273:⚠️ Potential issue | 🟠 Majortighten the final no-extra-fetch check to hold over a stable window.
Line 271 in test/codex-prompts.test.ts validates fetch count at one instant; a delayed extra refresh from lib/prompts/codex.ts:236-246 can still slip past. keep the assertion true across a short deterministic window.
proposed fix
- await vi.waitFor(() => { - expect(mockFetch).toHaveBeenCalledTimes(settledFetchCalls); - }); + const stableWindowMs = 150; + const stableWindowStart = Date.now(); + await vi.waitFor( + () => { + expect(mockFetch).toHaveBeenCalledTimes(settledFetchCalls); + expect(Date.now() - stableWindowStart).toBeGreaterThanOrEqual( + stableWindowMs, + ); + }, + { interval: 20, timeout: 1000 }, + );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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-prompts.test.ts` around lines 271 - 273, The final check that mockFetch was called exactly settledFetchCalls needs to be held stable across a short deterministic window to catch delayed refreshes from the codex token-refresh path; replace the single instant assertion with a stability check that repeatedly verifies mockFetch.callCount === settledFetchCalls for a short fixed duration (e.g., 50–200ms) using vitest utilities (vi.waitFor/vi.advanceTimers or a small loop with await new Promise) so the assertion must remain true across the window; target the existing mockFetch and settledFetchCalls variables and ensure the test waits for stability rather than a one-time snapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/codex-prompts.test.ts`:
- Around line 271-273: The final check that mockFetch was called exactly
settledFetchCalls needs to be held stable across a short deterministic window to
catch delayed refreshes from the codex token-refresh path; replace the single
instant assertion with a stability check that repeatedly verifies
mockFetch.callCount === settledFetchCalls for a short fixed duration (e.g.,
50–200ms) using vitest utilities (vi.waitFor/vi.advanceTimers or a small loop
with await new Promise) so the assertion must remain true across the window;
target the existing mockFetch and settledFetchCalls variables and ensure the
test waits for stability rather than a one-time snapshot.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Run ID: fc9ec878-97fc-4202-bb0b-e62f0bf9d1d0
📒 Files selected for processing (1)
test/codex-prompts.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 (1)
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/codex-prompts.test.ts
🧬 Code graph analysis (1)
test/codex-prompts.test.ts (1)
lib/prompts/codex.ts (1)
getCodexInstructions(204-293)
🔇 Additional comments (1)
test/codex-prompts.test.ts (1)
217-270: strong concurrency regression coverage for stale calls.test/codex-prompts.test.ts:217-270 now deterministically exercises the stale-while-revalidate concurrency path against lib/prompts/codex.ts:236-246 with vitest polling. this explicitly covers the concurrency risk. windows filesystem behavior and token refresh races are out of scope for this segment.
1 participant
Summary
resolveProjectStorageIdentityRootfallback when.gitentry exists but is neither file nor directorywaitForCodeabort path whenclose()is called during active pollingWhat Changed
test/paths.test.tstest/server.unit.test.tstest/codex-prompts.test.tsValidation
npx vitest run test/paths.test.ts -t "falls back when .git entry exists but is neither file nor directory"npx vitest run test/server.unit.test.ts -t "waitForCode returns null when close aborts active poll loop"npx vitest run test/codex-prompts.test.ts -t "deduplicates background refresh for concurrent stale calls"npx vitest run test/paths.test.ts test/server.unit.test.ts test/codex-prompts.test.tsnpm run typecheckRisk and Rollback
77d0e0309c30a6cf9635b56c29f34859ae63531fnote: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
added three regression tests covering edge cases and race conditions: storage path fallback for non-file/non-directory
.gitentries (sockets/pipes on unix), oauth poll abort during activewaitForCodeloop, and concurrent stale cache refresh deduplication.test/paths.test.ts: validatesresolveProjectStorageIdentityRootfallback when.gitexists butisFile()andisDirectory()both return falsetest/server.unit.test.ts: validateswaitForCodereturns null (not timeout) whenclose()aborts active poll, uses fake timerstest/codex-prompts.test.ts: validates concurrent calls togetCodexInstructionswith stale cache trigger exactly one background refresh, useswaitForfor async stabilityall tests properly mock filesystem and network calls, use appropriate timing controls, and verify expected behavior without side effects.
Confidence Score: 5/5
Important Files Changed
.gitentry that is neither file nor directory (socket/pipe edge case)waitForCodewhenclose()is called during active pollingwaitForfor stability, validates single background fetch for multiple stale callsLast reviewed commit: e06e5e5