fix: address critical issues from phase 2 review

- Fix memory leak in query-cache.ts: clean up generations Map when entries are deleted
- Fix potential data leak in analytics/log-helpers.ts: use explicit allowlist instead of spread operator
- Add tests for PII filtering in analytics

Author: brandonkachen

cli/src/utils/query-cache.ts

@@ -171,6 +171,9 @@ 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)
 }
 
 export function resetCache(): void {

common/src/analytics/log-helpers.ts

@@ -38,6 +38,92 @@ export function getAnalyticsEventId(data: unknown): AnalyticsEvent | null {
     : null
 }
 
+// Allowlist of properties safe to send to analytics.
+// Be conservative - only include properties that are clearly non-PII.
+const SAFE_ANALYTICS_PROPERTIES = new Set([
+  // Event metadata
+  'eventId',
+  'level',
+  'msg',
+  // Timing/metrics
+  'duration',
+  'durationMs',
+  'latency',
+  'latencyMs',
+  'timestamp',
+  // Counts/sizes
+  'count',
+  'size',
+  'length',
+  'total',
+  // Status/type identifiers
+  'status',
+  'type',
+  'action',
+  'source',
+  'target',
+  'category',
+  // Agent/model info
+  'agentId',
+  'agentType',
+  'modelId',
+  'modelName',
+  // Feature flags/versions
+  'version',
+  'feature',
+  'variant',
+  // Error info (without stack traces or sensitive details)
+  'errorCode',
+  'errorType',
+  // Boolean flags
+  'success',
+  'enabled',
+  'cached',
+  // Run/step identifiers
+  'runId',
+  'stepNumber',
+  'stepId',
+])
+
+// Properties that should never be sent to analytics (PII/sensitive)
+const BLOCKED_ANALYTICS_PROPERTIES = new Set([
+  'userId',
+  'user_id',
+  'user',
+  'email',
+  'name',
+  'password',
+  'token',
+  'apiKey',
+  'secret',
+  'authorization',
+  'cookie',
+  'session',
+  'ip',
+  'ipAddress',
+  'fingerprint',
+  'deviceId',
+])
+
+function extractSafeProperties(
+  record: AnalyticsLogData,
+): Record<string, unknown> {
+  const safeProps: Record<string, unknown> = {}
+
+  for (const [key, value] of Object.entries(record)) {
+    // Skip blocked properties
+    if (BLOCKED_ANALYTICS_PROPERTIES.has(key)) continue
+    // Skip complex objects that might contain PII
+    if (value !== null && typeof value === 'object') continue
+    // Only include properties in the allowlist
+    if (SAFE_ANALYTICS_PROPERTIES.has(key)) {
+      safeProps[key] = value
+    }
+  }
+
+  return safeProps
+}
+
 export function toTrackableAnalyticsPayload({
   data,
   level,
@@ -69,7 +155,7 @@ export function toTrackableAnalyticsPayload({
     event: eventId,
     userId,
     properties: {
-      ...record,
+      ...extractSafeProperties(record),
       level,
       msg,
     },

common/src/util/__tests__/analytics-dispatcher.test.ts

@@ -37,11 +37,13 @@ describe('analytics dispatcher', () => {
       event: AnalyticsEvent.APP_LAUNCHED,
       userId: 'u',
       properties: expect.objectContaining({
-        userId: 'u',
+        eventId: AnalyticsEvent.APP_LAUNCHED,
         level,
         msg,
       }),
     })
+    // PII fields should NOT be in properties (security fix)
+    expect(out[0].properties).not.toHaveProperty('userId')
   })
 
   it('buffers when no user and flushes once user appears', () => {

common/src/util/__tests__/analytics-log.test.ts

@@ -51,19 +51,49 @@ describe('analytics-log helpers', () => {
 
   it('builds payload when event and userId exist', () => {
     const payload = toTrackableAnalyticsPayload({
-      data: { eventId: AnalyticsEvent.APP_LAUNCHED, userId: 'u1', extra: 123 },
+      data: { eventId: AnalyticsEvent.APP_LAUNCHED, userId: 'u1', duration: 123 },
       level: baseLevel,
       msg: baseMsg,
     })!
 
     expect(payload.event).toBe(AnalyticsEvent.APP_LAUNCHED)
     expect(payload.userId).toBe('u1')
+    // Only allowlisted properties are included (userId is extracted separately, not spread)
     expect(payload.properties).toMatchObject({
-      userId: 'u1',
-      extra: 123,
+      eventId: AnalyticsEvent.APP_LAUNCHED,
+      duration: 123,
       level: baseLevel,
       msg: baseMsg,
     })
+    // PII fields should NOT be in properties
+    expect(payload.properties).not.toHaveProperty('userId')
+  })
+
+  it('filters out PII and unknown properties', () => {
+    const payload = toTrackableAnalyticsPayload({
+      data: {
+        eventId: AnalyticsEvent.APP_LAUNCHED,
+        userId: 'u1',
+        email: 'test@example.com',
+        password: 'secret',
+        unknownField: 'value',
+        duration: 500,
+        success: true,
+      },
+      level: baseLevel,
+      msg: baseMsg,
+    })!
+
+    // Safe properties are included
+    expect(payload.properties.duration).toBe(500)
+    expect(payload.properties.success).toBe(true)
+    expect(payload.properties.eventId).toBe(AnalyticsEvent.APP_LAUNCHED)
+    // PII is excluded
+    expect(payload.properties).not.toHaveProperty('userId')
+    expect(payload.properties).not.toHaveProperty('email')
+    expect(payload.properties).not.toHaveProperty('password')
+    // Unknown properties are excluded
+    expect(payload.properties).not.toHaveProperty('unknownField')
   })
 
   it('falls back to nested and underscored user ids', () => {