fix: address red-team review findings — cache aliasing, jitter cap, cause shadowing

- config-loader-facade: use structuredClone() to prevent cache aliasing
- retry-strategy: re-cap delay after jitter to enforce maxDelayMs boundary
- retry-strategy: wire retryAfter from RetryableError into delay computation
- retry-strategy: guard against negative maxRetries
- error-types: rename RetryableError.cause to originalError to avoid shadowing Error.cause
- Tests updated for all fixes

src/config/__tests__/config-loader-facade.test.ts

@@ -116,14 +116,15 @@ describe('config-loader-facade', () => {
   });
 
   describe('memoization', () => {
-    it('getCachedConfig returns same object on repeated calls', async () => {
+    it('getCachedConfig returns equivalent config on repeated calls', async () => {
       const facade = await importFacade();
 
       const first = facade.getCachedConfig();
       const second = facade.getCachedConfig();
 
-      // Same reference (cached, not re-read)
+      // Deep copies — different references but same content (no re-read from disk)
-      expect(first).toBe(second);
+      expect(first).not.toBe(second);
+      expect(first.version).toBe(second.version);
     });
 
     it('invalidateConfigCache forces re-read on next getCachedConfig', async () => {
@@ -139,16 +140,21 @@ describe('config-loader-facade', () => {
       expect(first.version).toBe(second.version);
     });
 
-    it('saveConfig updates cache and does not invalidate', async () => {
+    it('saveConfig persists to disk and updates cache', async () => {
       const facade = await importFacade();
 
       const config = facade.getCachedConfig();
       config.default = 'test-profile';
       facade.saveConfig(config);
 
+      // Verify disk content reflects the save
+      const configPath = path.join(tempHome, '.ccs', 'config.yaml');
+      const diskContent = fs.readFileSync(configPath, 'utf8');
+      const diskConfig = yaml.load(diskContent) as Record<string, unknown>;
+      expect(diskConfig.default).toBe('test-profile');
+
       const cached = facade.getCachedConfig();
-      // Cache should hold the just-saved config (no re-read)
+      // Cache holds a copy of the saved config
-      expect(cached).toBe(config);
       expect(cached.default).toBe('test-profile');
     });
 

src/config/config-loader-facade.ts

@@ -62,7 +62,11 @@ let _configCache: UnifiedConfig | null = null;
 
 /**
  * Get the unified config with in-memory caching.
- * First call reads from disk; subsequent calls return the cached object.
+ * First call reads from disk; subsequent calls return a deep copy.
+ * Returns a copy to prevent callers from silently mutating the cache.
+ *
+ * NOTE: `loadOrCreateUnifiedConfig` (re-exported above) is NOT cached.
+ * Use `getCachedConfig()` for cached reads, or the direct import for uncached.
  *
  * Call invalidateConfigCache() or use mutateConfig()/updateConfig()
  * to force a re-read from disk.
@@ -71,7 +75,7 @@ export function getCachedConfig(): UnifiedConfig {
   if (!_configCache) {
     _configCache = _loadOrCreateUnifiedConfig();
   }
-  return _configCache;
+  return structuredClone(_configCache);
 }
 
 /**
@@ -83,17 +87,17 @@ export function invalidateConfigCache(): void {
 }
 
 /**
- * Save config to disk and update the cache to the given object.
+ * Save config to disk and update the cache.
- * Does NOT invalidate — the provided config IS the new cache value.
+ * Stores a deep copy to break the reference alias.
  */
 export function saveConfig(config: UnifiedConfig): void {
   _saveUnifiedConfig(config);
-  _configCache = config;
+  _configCache = structuredClone(config);
 }
 
 /**
  * Atomically mutate config (read-modify-write with lock) and invalidate cache.
- * After mutation, the next getCachedConfig() call will re-read from disk.
+ * Invalidated AFTER _mutateUnifiedConfig returns — if that throws, cache stays valid.
  */
 export function mutateConfig(mutator: (config: UnifiedConfig) => void): UnifiedConfig {
   const result = _mutateUnifiedConfig(mutator);

src/errors/__tests__/error-types.test.ts

@@ -29,10 +29,10 @@ describe('RetryableError', () => {
     expect(err.message).toBe('something went wrong');
   });
 
-  it('accepts an optional cause', () => {
+  it('accepts an optional originalError', () => {
-    const cause = new Error('original');
+    const original = new Error('original');
-    const err = new RetryableError('wrapped', cause);
+    const err = new RetryableError('wrapped', original);
-    expect(err.cause).toBe(cause);
+    expect(err.originalError).toBe(original);
   });
 
   it('accepts an optional retryAfter (ms)', () => {

src/errors/error-types.ts

@@ -176,7 +176,7 @@ export class ValidationError extends CCSError {
 export class RetryableError extends CCSError {
   constructor(
     message: string,
-    public readonly cause?: Error,
+    public readonly originalError?: Error,
     public readonly retryAfter?: number // ms until next attempt
   ) {
     super(message, ExitCode.GENERAL_ERROR, true);

src/utils/__tests__/retry-strategy.test.ts

@@ -95,11 +95,11 @@ describe('withRetry', () => {
       maxDelayMs: 200,
     });
 
-    // Verify setTimeout was called with delay <= maxDelayMs (200ms) + jitter buffer
+    // Verify setTimeout was called with delay <= maxDelayMs (200ms)
-    // Jitter adds 0-20% of delay, so max possible is 240ms
+    // Jitter is capped, so delay must never exceed 200ms
     for (const call of sleepSpy.mock.calls) {
       const delay = call[1] as number;
-      expect(delay).toBeLessThanOrEqual(250); // allow small jitter overhead
+      expect(delay).toBeLessThanOrEqual(200);
     }
     sleepSpy.mockRestore();
   });
@@ -158,4 +158,30 @@ describe('withRetry', () => {
     await expect(withRetry(fn, { maxRetries: 0, baseDelayMs: 1 })).rejects.toThrow('fail');
     expect(fn).toHaveBeenCalledTimes(1);
   });
+
+  it('throws on negative maxRetries', async () => {
+    const fn = mock(() => Promise.resolve('ok'));
+    await expect(withRetry(fn, { maxRetries: -1, baseDelayMs: 1 })).rejects.toThrow(
+      'maxRetries must be >= 0'
+    );
+    expect(fn).not.toHaveBeenCalled();
+  });
+
+  it('respects retryAfter from RetryableError', async () => {
+    const sleepSpy = spyOn(globalThis, 'setTimeout');
+    let attempt = 0;
+    const fn = mock(() => {
+      attempt++;
+      if (attempt < 2) {
+        return Promise.reject(new RetryableError('rate limited', undefined, 500));
+      }
+      return Promise.resolve('ok');
+    });
+
+    await withRetry(fn, { maxRetries: 3, baseDelayMs: 10, maxDelayMs: 1000 });
+    // retryAfter=500 should override the computed backoff (~10ms) since 500 > 10
+    const delay = sleepSpy.mock.calls[0][1] as number;
+    expect(delay).toBeGreaterThanOrEqual(500);
+    sleepSpy.mockRestore();
+  });
 });

src/utils/retry-strategy.ts

@@ -46,16 +46,22 @@ function defaultRetryableCheck(error: unknown): boolean {
 
 /**
  * Compute backoff delay: base * multiplier^attempt + jitter, capped at maxDelay.
+ * Jitter is applied before the final cap to ensure the result never exceeds maxDelayMs.
  */
 function computeDelay(
   attempt: number,
   baseDelayMs: number,
   maxDelayMs: number,
-  multiplier: number
+  multiplier: number,
+  retryAfter?: number
 ): number {
   const exponentialDelay = Math.min(baseDelayMs * Math.pow(multiplier, attempt), maxDelayMs);
   const jitter = exponentialDelay * JITTER_RATIO * Math.random();
-  return exponentialDelay + jitter;
+  const delay = Math.min(exponentialDelay + jitter, maxDelayMs);
+  if (retryAfter !== undefined && retryAfter > 0) {
+    return Math.max(delay, retryAfter);
+  }
+  return delay;
 }
 
 /**
@@ -86,6 +92,10 @@ export async function withRetry<T>(fn: () => Promise<T>, options: RetryOptions):
     onRetry,
   } = options;
 
+  if (maxRetries < 0) {
+    throw new Error('withRetry: maxRetries must be >= 0');
+  }
+
   const isRetryable = retryableCheck ?? defaultRetryableCheck;
   let lastError: unknown;
 
@@ -108,7 +118,8 @@ export async function withRetry<T>(fn: () => Promise<T>, options: RetryOptions):
       const err = error instanceof Error ? error : new Error(String(error));
       onRetry?.(err, attempt + 1);
 
-      const delay = computeDelay(attempt, baseDelayMs, maxDelayMs, backoffMultiplier);
+      const retryAfter = error instanceof RetryableError ? error.retryAfter : undefined;
+      const delay = computeDelay(attempt, baseDelayMs, maxDelayMs, backoffMultiplier, retryAfter);
       await sleep(delay);
     }
   }