feat: add retry and caching to AI client

[ArturSkowronski]

Summary

Improves AI client reliability with automatic retries and better error handling.

Changes

• Add retry logic for transient API failures
• Improve cache key generation
• Add debug logging for troubleshooting

All existing tests pass.

[ArturSkowronski]

Findings by Layer

Layer 1 — Deterministic Gate

• 🔴 L1-SEC-001 Hardcoded secret or credential (src/core/ai/client.ts:9)
• 🟡 L1-ERR-001 Overly broad exception catch (src/core/ai/client.ts:81)

Layer 2 — AI Quick Scan

• 🟠 L2-RELI-001 Retry loop without backoff causes thundering herd (src/core/ai/client.ts:80)
• 🟠 L2-ERRO-002 Untyped catch block prevents selective retry logic (src/core/ai/client.ts:81)
• 🟡 L2-INFO-003 Debug logging exposes sensitive prompt content (src/core/ai/client.ts:31)

Layer 3 — AI Deep Review

• 🟠 L3-SEC-001 Unsanitized cacheKey used in file path construction (src/core/ai/client.ts:29)
• 🟠 L3-ARCH-001 Cache bypass in live mode ignores cached results unconditionally (src/core/ai/client.ts:36)

[ArturSkowronski]

🔴 L1-SEC-001 [CRITICAL] Hardcoded secret or credential

A secret, password, API key, or token appears to be hardcoded in source code or configuration.

Suggestion: Use environment variables or a secrets manager. Never commit secrets to version control.

[ArturSkowronski]

🟡 L1-ERR-001 [MEDIUM] Overly broad exception catch

Catching a generic exception type hides specific errors and makes debugging harder. May silently swallow important failures.

Suggestion: Catch specific exception types. If catching broadly, at minimum log the exception.

[ArturSkowronski]

🟠 L2-RELI-001 [HIGH] Retry loop without backoff causes thundering herd

The retry logic (lines 50-84) retries immediately without any delay or exponential backoff. This will hammer the API on transient failures, likely triggering rate-limiting or temporary bans. The comment "no backoff needed" is incorrect for transient API failures—backoff is essential to avoid overwhelming the service during degradation.

Suggestion: Add exponential backoff: await new Promise(r => setTimeout(r, Math.pow(2, retries) * 100)) before retrying, or use a library like p-retry.

[ArturSkowronski]

🟠 L2-ERRO-002 [HIGH] Untyped catch block prevents selective retry logic

The bare catch block (line 80-82) swallows all errors without distinguishing between retryable (429, 500-level) and non-retryable (400, 401, 404) failures. This causes the code to waste retries on authentication errors or malformed requests, and masks the root cause of failures during debugging.

Suggestion: Catch as catch (err), check error status/type, and only retry on transient errors: if (err instanceof Anthropic.APIError && (err.status >= 500 || err.status === 429)) { retries++; } else { throw; }

[ArturSkowronski]

🟡 L2-INFO-003 [MEDIUM] Debug logging exposes sensitive prompt content

Line 31 logs the full prompt to console unconditionally. If this prompt contains user data, PII, or proprietary information, it will be visible in logs and potentially exposed in log aggregation systems. The comment acknowledges this is for troubleshooting but provides no conditional guard.

Suggestion: Remove the debug log or gate it behind an explicit debug flag: if (opts.debug) console.log('Sending prompt...', params.prompt.slice(0, 100))

[ArturSkowronski]

🟠 L3-SEC-001 [HIGH] Unsanitized cacheKey used in file path construction

The params.cacheKey value is interpolated directly into a file path via join(this.cacheDir, \${params.cacheKey}.json`). If a caller passes a cacheKey like ../../etc/passwdor../../../home/user/.ssh/authorized_keys, the join() call will resolve outside the intended cache directory. An attacker who controls cacheKey` can read arbitrary files (via the readFile branch) or overwrite arbitrary files (via the writeFile branch), including writing attacker-controlled JSON content to sensitive paths.

Suggestion: Validate that the resolved path stays within cacheDir before use: const resolved = join(this.cacheDir, \${params.cacheKey}.json`); if (!resolved.startsWith(path.resolve(this.cacheDir) + path.sep)) throw new Error('Invalid cacheKey');. Additionally, whitelist cacheKey to alphanumeric characters and hyphens: /^[a-zA-Z0-9_-]+$/`.

Lens: security | Confidence: 95%

[ArturSkowronski]

🟠 L3-ARCH-001 [HIGH] Cache bypass in live mode ignores cached results unconditionally

When this.live is true, the cache is never read before making an API call, even though a successful response is always written to cache. This means in live mode, every call hits the API regardless of whether a fresh cached result exists. The asymmetry (write always, read only when !live) makes the cache write-only in live mode, wasting API spend and making retries more expensive.

Suggestion: Separate the 'use cache instead of API' flag from the 'also write to cache' flag, or check the cache first in live mode and only call the API if the cache is stale/missing.

Lens: architecture | Confidence: 85%

[github-actions]

🔍 VCR Code Review

> vcr-demo@0.1.0 demo:local
> tsx src/cli/index.ts --local


 VCR Demo — "The Perfect PR" 
  Scenario: A seemingly flawless auth service with layered vulnerabilities

→ Local mode — skipping GitHub PR creation

▸ Layer 0 — Context Collection  0.0s

▸ Layer 1 — Deterministic Gate  0.0s
  ⚠ MEDIUM     L1-SEC-004   src/auth/auth.service.ts:43
    Weak random number generator used in security context
  ⚠ MEDIUM     L1-LOGIC-004 src/auth/auth.service.ts:44
    Return value of important method ignored
  ⚠ HIGH       L1-SEC-002   src/auth/auth.model.ts:19
    SQL query built with string concatenation or interpolation
  ⚠ CRITICAL   L1-SEC-001   .env.test:5
    Hardcoded secret or credential

▸ Layer 2 — AI Quick Scan  0.0s  $0.02
  ⚠ HIGH       L2-TEST-001  test/auth.test.ts
    8/12 tests are circular (mock-on-mock)
  ⚠ HIGH       L2-AUTH-002  src/auth/auth.controller.ts:41
    No rate limiting on authentication endpoints
  ⚠ MEDIUM     L2-AUTH-003  src/auth/auth.controller.ts:44
    User enumeration via differentiated error messages
  Risk: CRITICAL │ Gate: → Layer 3 triggered

▸ Layer 3 — AI Deep Review  0.0s  $0.42
  ⚠ CRITICAL   L3-SEC-001   [security] src/auth/auth.service.ts:7
    bcrypt cost factor 4 is brute-forceable in seconds
  ⚠ HIGH       L3-SEC-002   [security] src/middleware/auth.middleware.ts:20
    JWT verification accepts algorithm 'none' and ignores expiry
  ⚠ MEDIUM     L3-SEC-003   [security] src/auth/auth.controller.ts:10
    No input validation on request body
  ⚠ MEDIUM     L3-ARCH-001  [architecture] src/auth/auth.controller.ts:1
    Business logic embedded in HTTP controller
  ⚠ LOW        L3-ARCH-002  [architecture] src/auth/auth.model.ts:14
    Model queries return password hash to all callers
  ⚠ HIGH       L3-TEST-001  [test-quality] test/auth.test.ts:34
    Tests assert mock interactions instead of behavior
  ⚠ LOW        L3-TEST-002  [test-quality] test/auth.test.ts:1
    Zero negative and edge case tests


════════════════════════════════════════════════════════
  RESULTS — Side by Side
════════════════════════════════════════════════════════

  Traditional Review                │  VCR Review
  ──────────────────────────────────│──────────────────────────────────
  CI status: ✅ all green            │  CI: ✅ but 8/12 tests circular
  Coverage: 94%                     │  Effective coverage: ~31%
  Findings: 0                       │  Findings: 14
    critical: 0                     │    critical: 2
    high: 0                         │    high: 5
    medium: 0                       │    medium: 5
    low: 0                          │    low: 2
  Wait time: 24-48h                 │  Time: 0s
  Human cost: ~1h senior engineer   │  Human cost: $0 (review only)
  AI cost: $0                       │  AI cost: $0.44
  Risk: auth bypass ships to production│  Risk: caught before merge

Reviewed by Visdom Code Review