fix: address critical issues from code review

- fix(sdk): add 10s timeout to git operations to prevent hanging
- fix(sdk): add maxFiles limit (10000) to discoverProjectFiles to prevent memory issues
- fix(billing): add guard for empty paymentMethods array in getOrSetDefaultPaymentMethod
- fix(billing): fix card expiration check to use end of month (not start)

- fix(cli): prevent in-flight fetches from resurrecting deleted cache entries
  - Generation counter now persists after deletion
  - Added comprehensive tests for deletion protection

- test(cli): add unit tests for use-keyboard-navigation hook (40 tests)
- test(cli): add unit tests for use-keyboard-shortcuts hook (49 tests)
- test(cli): add tests for cache deletion and in-flight request protection (7 tests)

Author: brandonkachen

cli/src/hooks/__tests__/use-activity-query.test.ts

@@ -8,6 +8,13 @@ import {
   resetActivityQueryCache,
   isEntryStale,
 } from '../use-activity-query'
+import {
+  bumpGeneration,
+  getGeneration,
+  deleteCacheEntryCore,
+  setCacheEntry,
+  serializeQueryKey,
+} from '../../utils/query-cache'
 
 describe('use-activity-query utilities', () => {
   beforeEach(() => {
@@ -765,3 +772,193 @@ describe('cache edge cases and error handling', () => {
     expect(getActivityQueryData<string>(testKey)).toBe('second')
   })
 })
+
+/**
+ * Tests for the cache deletion and in-flight request protection.
+ * Verifies that in-flight fetches cannot "resurrect" deleted cache entries.
+ *
+ * The bug scenario was:
+ * 1. Fetch starts, captures myGen = 0 (generation not set defaults to 0)
+ * 2. Entry deleted: bumps generation to 1, then USED TO delete generation entry
+ * 3. Fetch completes, getGeneration(key) returned 0 again (entry was deleted!)
+ * 4. 0 === 0 passed, stale fetch wrote to cache, resurrecting the deleted entry
+ *
+ * The fix: Don't delete the generation entry after bumping it in deleteCacheEntryCore.
+ * The bumped generation persists so in-flight requests see a different generation.
+ */
+describe('cache deletion and in-flight request protection', () => {
+  beforeEach(() => {
+    resetActivityQueryCache()
+  })
+
+  test('deletion bumps generation from 0 to 1', () => {
+    const testKey = ['deletion-gen-test']
+    const serializedKey = serializeQueryKey(testKey)
+
+    // Set initial data
+    setActivityQueryData(testKey, 'initial-data')
+
+    // Before deletion, generation should be 0 (default)
+    expect(getGeneration(serializedKey)).toBe(0)
+
+    // Delete the entry
+    deleteCacheEntryCore(serializedKey)
+
+    // After deletion, generation should be bumped to 1
+    expect(getGeneration(serializedKey)).toBe(1)
+  })
+
+  test('generation persists after deletion (not cleared)', () => {
+    const testKey = ['gen-persist-test']
+    const serializedKey = serializeQueryKey(testKey)
+
+    // Set data and delete it
+    setActivityQueryData(testKey, 'data')
+    deleteCacheEntryCore(serializedKey)
+
+    // Generation should be 1 after first deletion
+    expect(getGeneration(serializedKey)).toBe(1)
+
+    // Data should be gone
+    expect(getActivityQueryData(testKey)).toBeUndefined()
+
+    // Set new data and delete again
+    setActivityQueryData(testKey, 'new-data')
+    deleteCacheEntryCore(serializedKey)
+
+    // Generation should be 2 after second deletion
+    expect(getGeneration(serializedKey)).toBe(2)
+
+    // This proves generation is NOT being deleted, but accumulated
+  })
+
+  test('simulated in-flight fetch cannot resurrect deleted entry', () => {
+    const testKey = ['in-flight-protection-test']
+    const serializedKey = serializeQueryKey(testKey)
+
+    // Step 1: Set initial data (simulating a previous successful fetch)
+    setActivityQueryData(testKey, 'original-data')
+    expect(getActivityQueryData<string>(testKey)).toBe('original-data')
+
+    // Step 2: Simulate a fetch starting - capture the generation
+    // In real code: const myGen = getGeneration(key)
+    const myGen = getGeneration(serializedKey)
+    expect(myGen).toBe(0) // Default generation
+
+    // Step 3: While the fetch is "in flight", the entry gets deleted
+    // (e.g., user navigates away, cache GC runs, or explicit removal)
+    deleteCacheEntryCore(serializedKey)
+
+    // Step 4: Entry is now deleted, but generation was bumped
+    expect(getActivityQueryData(testKey)).toBeUndefined()
+    expect(getGeneration(serializedKey)).toBe(1)
+
+    // Step 5: The in-flight fetch completes and tries to write
+    // In real code, this check happens before setCacheEntry:
+    // if (getGeneration(key) !== myGen) return
+    const currentGen = getGeneration(serializedKey)
+    const wouldSkipWrite = currentGen !== myGen
+
+    // The write SHOULD be skipped because generations don't match
+    expect(wouldSkipWrite).toBe(true)
+    expect(myGen).toBe(0)
+    expect(currentGen).toBe(1)
+
+    // Verify the cache stays empty (entry not resurrected)
+    expect(getActivityQueryData(testKey)).toBeUndefined()
+  })
+
+  test('generation check correctly allows writes when not deleted', () => {
+    const testKey = ['gen-allow-write-test']
+    const serializedKey = serializeQueryKey(testKey)
+
+    // Set initial data
+    setActivityQueryData(testKey, 'initial')
+
+    // Capture generation before fetch
+    const myGen = getGeneration(serializedKey)
+    expect(myGen).toBe(0)
+
+    // Simulate fetch completing WITHOUT any deletion happening
+    // The generation should still be 0
+    const currentGen = getGeneration(serializedKey)
+    const wouldSkipWrite = currentGen !== myGen
+
+    // Write should NOT be skipped - generations match
+    expect(wouldSkipWrite).toBe(false)
+
+    // Normal update should work
+    setActivityQueryData(testKey, 'updated')
+    expect(getActivityQueryData<string>(testKey)).toBe('updated')
+  })
+
+  test('multiple in-flight fetches all see their respective generations', () => {
+    const testKey = ['multi-flight-test']
+    const serializedKey = serializeQueryKey(testKey)
+
+    // Fetch 1 starts
+    const gen1 = getGeneration(serializedKey)
+    expect(gen1).toBe(0)
+
+    // Set some data and delete it
+    setActivityQueryData(testKey, 'data1')
+    deleteCacheEntryCore(serializedKey)
+
+    // Fetch 2 starts after deletion
+    const gen2 = getGeneration(serializedKey)
+    expect(gen2).toBe(1)
+
+    // Another deletion
+    setActivityQueryData(testKey, 'data2')
+    deleteCacheEntryCore(serializedKey)
+
+    // Fetch 3 starts after second deletion
+    const gen3 = getGeneration(serializedKey)
+    expect(gen3).toBe(2)
+
+    // Now check which fetches would be allowed to write
+    const currentGen = getGeneration(serializedKey)
+    expect(currentGen).toBe(2)
+
+    // Fetch 1: captured gen 0, current is 2 -> skip write
+    expect(currentGen !== gen1).toBe(true)
+
+    // Fetch 2: captured gen 1, current is 2 -> skip write
+    expect(currentGen !== gen2).toBe(true)
+
+    // Fetch 3: captured gen 2, current is 2 -> allow write
+    expect(currentGen !== gen3).toBe(false)
+  })
+
+  test('bumpGeneration without deletion also increments generation', () => {
+    const testKey = ['bump-only-test']
+    const serializedKey = serializeQueryKey(testKey)
+
+    expect(getGeneration(serializedKey)).toBe(0)
+
+    bumpGeneration(serializedKey)
+    expect(getGeneration(serializedKey)).toBe(1)
+
+    bumpGeneration(serializedKey)
+    expect(getGeneration(serializedKey)).toBe(2)
+
+    bumpGeneration(serializedKey)
+    expect(getGeneration(serializedKey)).toBe(3)
+  })
+
+  test('resetActivityQueryCache clears generations', () => {
+    const testKey = ['reset-gen-test']
+    const serializedKey = serializeQueryKey(testKey)
+
+    // Set data and delete it to bump generation
+    setActivityQueryData(testKey, 'data')
+    deleteCacheEntryCore(serializedKey)
+    expect(getGeneration(serializedKey)).toBe(1)
+
+    // Reset cache should clear everything including generations
+    resetActivityQueryCache()
+
+    // Generation should be back to 0 (default)
+    expect(getGeneration(serializedKey)).toBe(0)
+  })
+})

cli/src/utils/query-cache.ts

@@ -171,9 +171,11 @@ export function deleteCacheEntryCore(key: string): void {
   cache.refCounts.delete(key)
   snapshotMemo.delete(key)
   notifyKeyListeners(key)
-  // Clean up generation counter after deletion is complete.
-  // The bump above invalidates any in-flight requests; now we can free the memory.
-  generations.delete(key)
+  // NOTE: We intentionally do NOT delete the generation counter here.
+  // The bumped generation must persist so that in-flight requests see a different
+  // generation when they complete and will not "resurrect" the deleted entry.
+  // Memory impact is minimal (just a number per deleted key). Generations are
+  // cleaned up during resetCache() which is used for testing.
 }
 
 export function resetCache(): void {
@@ -190,7 +192,3 @@ export function resetCache(): void {
   snapshotMemo.clear()
   generations.clear()
 }
-
-export function clearGeneration(key: string): void {
-  generations.delete(key)
-}

packages/billing/src/__tests__/auto-topup-helpers.test.ts

@@ -483,16 +483,15 @@ describe('auto-topup-helpers', () => {
         expect(isValidPaymentMethod(card)).toBe(false)
       })
 
-      it('should return false for card expiring in current month', () => {
-        // The logic uses > not >= so cards expiring this month are invalid
-        // as the check creates a date at the START of the expiration month
+      it('should return true for card expiring in current month', () => {
+        // Cards are valid through the END of their expiration month
         const now = new Date()
         const card = createCardPaymentMethod(
           'pm_1',
           now.getFullYear(),
           now.getMonth() + 1,
         )
-        expect(isValidPaymentMethod(card)).toBe(false)
+        expect(isValidPaymentMethod(card)).toBe(true)
       })
 
       it('should return true for card expiring next month', () => {

packages/billing/src/auto-topup-helpers.ts

@@ -34,10 +34,13 @@ export async function fetchPaymentMethods(
  */
 export function isValidPaymentMethod(pm: Stripe.PaymentMethod): boolean {
   if (pm.type === 'card') {
+    // Cards are valid through the END of their expiration month.
+    // Compare against the first day of the month AFTER expiration.
+    // e.g., card expiring 01/2024 is valid until Feb 1, 2024
     return (
       pm.card?.exp_year !== undefined &&
       pm.card.exp_month !== undefined &&
-      new Date(pm.card.exp_year, pm.card.exp_month - 1) > new Date()
+      new Date() < new Date(pm.card.exp_year, pm.card.exp_month, 1)
     )
   }
   if (pm.type === 'link') {
@@ -123,6 +126,10 @@ export async function getOrSetDefaultPaymentMethod(params: {
 }): Promise<GetOrSetDefaultPaymentMethodResult> {
   const { stripeCustomerId, paymentMethods, logger, logContext } = params
 
+  if (paymentMethods.length === 0) {
+    throw new Error('No payment methods available for this customer')
+  }
+
   const customer = await stripeServer.customers.retrieve(stripeCustomerId)
 
   if (

sdk/src/impl/git-operations.ts

@@ -5,12 +5,22 @@
 import type { Logger } from '@codebuff/common/types/contracts/logger'
 import type { CodebuffSpawn } from '@codebuff/common/types/spawn'
 
+const DEFAULT_GIT_TIMEOUT_MS = 10000 // 10 seconds
+
 function childProcessToPromise(
   proc: ReturnType<CodebuffSpawn>,
+  timeoutMs: number = DEFAULT_GIT_TIMEOUT_MS,
 ): Promise<{ stdout: string; stderr: string }> {
   return new Promise((resolve, reject) => {
     let stdout = ''
     let stderr = ''
+    let timedOut = false
+
+    const timeoutId = setTimeout(() => {
+      timedOut = true
+      proc.kill()
+      reject(new Error(`Git command timed out after ${timeoutMs}ms`))
+    }, timeoutMs)
 
     proc.stdout?.on('data', (data: Buffer) => {
       stdout += data.toString()
@@ -21,14 +31,20 @@ function childProcessToPromise(
     })
 
     proc.on('close', (code: number | null) => {
+      clearTimeout(timeoutId)
+      if (timedOut) return
       if (code === 0) {
         resolve({ stdout, stderr })
       } else {
         reject(new Error(`Command exited with code ${code}`))
       }
     })
 
-    proc.on('error', reject)
+    proc.on('error', (err) => {
+      clearTimeout(timeoutId)
+      if (timedOut) return
+      reject(err)
+    })
   })
 }
 

sdk/src/impl/project-discovery.ts

@@ -9,15 +9,31 @@ import { getErrorObject } from '@codebuff/common/util/error'
 import type { Logger } from '@codebuff/common/types/contracts/logger'
 import type { CodebuffFileSystem } from '@codebuff/common/types/filesystem'
 
+const DEFAULT_MAX_FILES = 10000
+
 export async function discoverProjectFiles(params: {
   cwd: string
   fs: CodebuffFileSystem
   logger: Logger
+  maxFiles?: number
 }): Promise<Record<string, string>> {
-  const { cwd, fs, logger } = params
+  const { cwd, fs, logger, maxFiles = DEFAULT_MAX_FILES } = params
 
   const fileTree = await getProjectFileTree({ projectRoot: cwd, fs })
-  const filePaths = getAllFilePaths(fileTree)
+  const allFilePaths = getAllFilePaths(fileTree)
+
+  let filePaths = allFilePaths
+  if (allFilePaths.length > maxFiles) {
+    logger.warn(
+      {
+        totalFiles: allFilePaths.length,
+        maxFiles,
+        truncatedCount: allFilePaths.length - maxFiles,
+      },
+      `Project has ${allFilePaths.length} files, exceeding limit of ${maxFiles}. Processing first ${maxFiles} files only.`,
+    )
+    filePaths = allFilePaths.slice(0, maxFiles)
+  }
 
   const errors: Array<{ filePath: string; error: unknown }> = []
   const projectFiles: Record<string, string> = {}