fix: address code review findings — critical, major, and minor issues

Fix 19 findings from comprehensive code review across SDK, API, storage,
and frontend:

- Fix cosine_similarity division by zero for near-zero vectors
- Fix unsafe datetime parsing with explicit error handling
- Fix SSE silent parse failures with user-visible notifications
- Fix N+1 queries in event_type filtering and entity extraction
- Fix timezone inconsistency in pattern_repo (now UTC)
- Add session count limit and logging for entity extraction
- Fix session store dead code in jumpToSearchResult
- Add max-size limit to request deduplication Map
- Change HindsightConfig.enabled default to False (opt-in)
- Add FileExporter path traversal validation
- Extract hardcoded limits to config constants
- Make SSE timeout configurable via env var
- Fix async dependency injection in analytics routes
- Add migration downgrade idempotency check
- Remove duplicate step-backward button in SessionReplay

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: acailic

agent_debugger_sdk/adapters/hindsight.py

@@ -34,7 +34,7 @@ class HindsightConfig:
     bank_id: str = "agent_debugger"
     api_key: str | None = None
     timeout_seconds: float = 30.0
-    enabled: bool = True
+    enabled: bool = False
 
     # Memory type mappings
     experience_memory_type: str = "failure_experience"

agent_debugger_sdk/core/exporters/file.py

@@ -142,12 +142,25 @@ def __init__(self, base_dir: str | Path | None = None):
         Args:
             base_dir: Base directory for storing exported data.
                      Defaults to ~/.peaky-peek/memory
+
+        Raises:
+            ValueError: If the provided base_dir path is outside the intended directory.
         """
         if base_dir is None:
             home = Path.home()
             base_dir = home / ".peaky-peek" / "memory"
 
-        self.base_dir = Path(base_dir)
+        # Resolve and validate the path
+        resolved_path = Path(base_dir).resolve()
+
+        # Reject paths with directory traversal components
+        if ".." in Path(base_dir).parts or "~" in Path(base_dir).parts:
+            raise ValueError(
+                f"Invalid base_dir: {base_dir}. "
+                "Path must not contain directory traversal components."
+            )
+
+        self.base_dir = resolved_path
         self.sessions_dir = self.base_dir / "sessions"
         self.patterns_dir = self.base_dir / "patterns"
         self.entities_dir = self.base_dir / "entities"

api/analytics_routes.py

@@ -5,10 +5,11 @@
 from datetime import datetime, timedelta, timezone
 from typing import Any, Literal
 
-from fastapi import APIRouter, Query
+from fastapi import APIRouter, Depends, Query
 from pydantic import BaseModel, ConfigDict, Field
 
 from api.analytics_db import get_aggregates, get_daily_breakdown, record_event
+from api.dependencies import get_repository
 
 router = APIRouter(tags=["analytics"])
 
@@ -248,17 +249,13 @@ async def get_patterns(
     status: str | None = Query(default="active", description="Filter by status"),
     hours: int | None = Query(default=None, description="Only return patterns from last N hours"),
     limit: int = Query(default=50, ge=1, le=200, description="Maximum results to return"),
+    repo=Depends(get_repository),
 ) -> PatternsListResponse:
     """Get detected patterns across sessions.
 
     Returns patterns matching the specified filters, ordered by detection time.
     Supports filtering by agent, pattern type, severity, status, and time range.
     """
-    from api.dependencies import get_repository
-    from storage import TraceRepository
-
-    repo: TraceRepository = get_repository()
-
     # Use the pattern repository through the session
     from storage.repositories.pattern_repo import PatternRepository
 
@@ -310,16 +307,14 @@ async def get_patterns(
 @router.get("/api/analytics/patterns/{pattern_id}", response_model=PatternDetailResponse)
 async def get_pattern_detail(
     pattern_id: str,
+    repo=Depends(get_repository),
 ) -> PatternDetailResponse:
     """Get detailed information about a specific pattern.
 
     Returns full pattern details including affected sessions list.
     """
-    from api.dependencies import get_repository
-    from storage import TraceRepository
     from storage.repositories.pattern_repo import PatternRepository
 
-    repo: TraceRepository = get_repository()
     pattern_repo = PatternRepository(repo.session, repo.tenant_id)
 
     pattern = await pattern_repo.get_pattern(pattern_id)
@@ -352,6 +347,7 @@ async def get_pattern_detail(
 async def get_health_report(
     agent_name: str | None = Query(default=None, description="Filter by agent name"),
     hours: int | None = Query(default=24, ge=1, le=168, description="Look at patterns from last N hours"),
+    repo=Depends(get_repository),
 ) -> HealthReportSchema:
     """Generate agent health report from detected patterns.
 
@@ -363,12 +359,9 @@ async def get_health_report(
     - Top issues with recommendations
     - Actionable next steps
     """
-    from api.dependencies import get_repository
     from collector.patterns import generate_health_report
-    from storage import TraceRepository
     from storage.repositories.pattern_repo import PatternRepository
 
-    repo: TraceRepository = get_repository()
     pattern_repo = PatternRepository(repo.session, repo.tenant_id)
 
     # Get recent patterns

api/replay_routes.py

@@ -48,9 +48,13 @@ async def replay_session(
 
     # FastAPI resolves Query defaults in HTTP calls but not in direct/unit-test calls.
     # Extract the default value when the raw Query object is passed through.
-    if hasattr(collapse_threshold, "default"):
+    # Check for the Query class by examining if the value has a 'default' attribute
+    # and is not already a primitive type.
+    from fastapi import params
+
+    if isinstance(collapse_threshold, params.Query):
         collapse_threshold = float(collapse_threshold.default)
-    if hasattr(stop_at_breakpoint, "default"):
+    if isinstance(stop_at_breakpoint, params.Query):
         stop_at_breakpoint = bool(stop_at_breakpoint.default)
 
     # Record analytics event (fire-and-forget)

api/search_routes.py

@@ -164,21 +164,21 @@ async def search_sessions_nl(
     if request.min_errors is not None:
         filters_applied["min_errors"] = request.min_errors
 
-    # Parse datetime filters
+    # Parse datetime filters with explicit error handling
     started_after = None
     started_before = None
     if request.started_after:
         try:
             started_after = datetime.fromisoformat(request.started_after.replace("Z", "+00:00"))
             filters_applied["started_after"] = request.started_after
-        except ValueError:
-            pass  # Invalid datetime, ignore filter
+        except ValueError as e:
+            raise ValueError(f"Invalid started_after datetime '{request.started_after}': {e}")
     if request.started_before:
         try:
             started_before = datetime.fromisoformat(request.started_before.replace("Z", "+00:00"))
             filters_applied["started_before"] = request.started_before
-        except ValueError:
-            pass  # Invalid datetime, ignore filter
+        except ValueError as e:
+            raise ValueError(f"Invalid started_before datetime '{request.started_before}': {e}")
 
     # Perform search with all filters
     sessions = await repo.search_sessions(

api/services.py

@@ -5,6 +5,7 @@
 import asyncio
 import json
 import logging
+import os
 from typing import Any
 
 from sqlalchemy import String, cast, or_, select
@@ -24,6 +25,7 @@
 logger = logging.getLogger(__name__)
 SESSION_ANALYSIS_CAP = 100
 FAILURE_SIMILARITY_THRESHOLD = 0.5
+DEFAULT_SSE_TIMEOUT = int(os.getenv("AGENT_DEBUGGER_SSE_TIMEOUT", "300"))
 
 
 def normalize_session(
@@ -213,13 +215,16 @@ async def enrich_sessions_for_listing(
     if sort_by != "replay_value":
         return [normalize_session(session) for session in sessions]
 
-    capped_sessions = sessions[:SESSION_ANALYSIS_CAP]
     if len(sessions) > SESSION_ANALYSIS_CAP:
         logger.warning(
-            "Replay-value enrichment capped at %s sessions for one response page",
+            "Replay-value enrichment capped at %s sessions for one response page (has %s sessions). "
+            "Sessions beyond the cap will be returned without replay_value enrichment.",
             SESSION_ANALYSIS_CAP,
+            len(sessions),
         )
 
+    capped_sessions = sessions[:SESSION_ANALYSIS_CAP]
+
     # Parallelize session analysis for better performance
     # Note: We still analyze sessions to get enrichment data like representative_event_id,
     # but the replay_value itself may be cached from a previous analysis
@@ -333,15 +338,18 @@ async def event_generator(
     session_id: str,
     *,
     buffer: EventBuffer | None = None,
-    max_connection_time: int = 300,
+    max_connection_time: int | None = None,
 ):
     """Generate SSE events for a session.
 
     Args:
         session_id: Session ID to stream events for
         buffer: Optional event buffer (uses default if None)
-        max_connection_time: Maximum connection time in seconds (default 300)
+        max_connection_time: Maximum connection time in seconds (default from
+            AGENT_DEBUGGER_SSE_TIMEOUT env var, 300 if not set)
     """
+    if max_connection_time is None:
+        max_connection_time = DEFAULT_SSE_TIMEOUT
     import time
 
     buf = buffer or get_event_buffer()

api/session_routes.py

@@ -37,6 +37,7 @@
 from storage import TraceRepository
 
 router = APIRouter(tags=["sessions"])
+DEFAULT_EXPORT_CHECKPOINTS_LIMIT = 1000
 
 
 def _normalize_datetime(value: datetime | None) -> datetime | None:
@@ -212,7 +213,7 @@ async def export_session(
     session = await require_session(repo, session_id)
     events = await repo.list_events(session_id, limit=MAX_TRACES_PER_REQUEST)
     # Add limit to checkpoint loading to prevent unbounded queries
-    checkpoints = await repo.list_checkpoints(session_id, limit=1000)
+    checkpoints = await repo.list_checkpoints(session_id, limit=DEFAULT_EXPORT_CHECKPOINTS_LIMIT)
 
     export_data = {
         "export_version": "1.0",

docs/superpowers/reviews/2026-04-04-full-code-review.md

@@ -0,0 +1,175 @@
+# Code Review Report: Full Recent Changes (Last 20 Commits)
+
+**Date:** 2026-04-04
+**Reviewers:** 4 parallel code-reviewer agents (SDK, API, Storage, Frontend)
+**Scope:** ~14,800 lines across 149 files
+**Status:** COMPLETE
+
+---
+
+## Executive Summary
+
+| Domain | Files | Lines | Critical | Major | Minor | Nitpick | Grade |
+|--------|-------|-------|----------|-------|-------|---------|-------|
+| SDK + Exporters + Adapters | 21 | ~1,620 | 0 | 2 | 3 | 2 | **Conditional** |
+| API + Services + Schemas | 10 | ~945 | 1 | 3 | 4 | 4 | **B+** |
+| Storage + Search + Collector | 16 | ~2,171 | 1 | 3 | 1 | 0 | **7.5/10** |
+| Frontend + Integration | 38 | ~4,641 | 2 | 3 | 3 | 2 | **Pass** |
+| **TOTAL** | **85** | **~9,377** | **4** | **11** | **11** | **8** | |
+
+**Verdict:** The codebase is in good shape overall. Security posture is strong (tenant isolation, SQL injection protection, input validation). The 4 critical and 11 major findings should be addressed before the next release. No data loss or security vulnerability was found.
+
+---
+
+## Critical Findings (4)
+
+| # | Domain | File | Line(s) | Issue | Fix |
+|---|--------|------|---------|-------|-----|
+| C1 | API | `api/search_routes.py` | 171-179 | Unsafe datetime parsing with silent failure on invalid ISO strings | Use Pydantic datetime types or explicit error handling for `started_after`/`started_before` |
+| C2 | Frontend | `frontend/src/App.tsx` | 285-287 | SSE JSON parse failure silently swallows events without alerting user | Add user-visible notification after N consecutive parse failures |
+| C3 | Frontend | `frontend/src/components/SimilarFailuresPanel.tsx` | 66 | `ignore` flag doesn't account for rapid sessionId changes — stale state updates | Add sessionId to dependency array or use AbortController |
+| C4 | Storage | `storage/search.py` | 317-339 | Division by zero risk in cosine_similarity — near-zero vectors not protected | Add explicit check: `if magnitude < 1e-10: return 0.0` before division |
+
+---
+
+## Major Findings (11)
+
+| # | Domain | File | Line(s) | Issue | Fix |
+|---|--------|------|---------|-------|-----|
+| M1 | API | `api/entity_routes.py` | 72 | N+1 query: `extract_entities_from_all_sessions()` loads ALL events across ALL sessions | Implement caching or pre-computed entity tables |
+| M2 | API | `api/services.py` | 226 | Parallel session analysis cap at 100 may silently drop enrichment data | Log warning BEFORE truncation |
+| M3 | API | `api/services.py` | 477-485 | Similar failures query uses OR clause with multiple ILIKE without index | Add index on `(tenant_id, event_type)`, consider full-text search |
+| M4 | Frontend | `frontend/src/stores/sessionStore.ts` | 231-234 | `jumpToSearchResult` dead code — sets replayMode but doesn't call inspectEvent | Remove conditional or add missing call |
+| M5 | Frontend | `frontend/src/api/client.ts` | 19 | Request deduplication Map grows unbounded | Add TTL or max-size limit |
+| M6 | Frontend | `frontend/src/components/DecisionTree.tsx` | 296-619 | Heavy D3 rendering without debouncing | Add render debouncing for large trees |
+| M7 | Storage | `storage/repositories/pattern_repo.py` | 70 | Naive datetime without timezone — `datetime.now()` inconsistent with UTC elsewhere | Use `datetime.now(timezone.utc)` |
+| M8 | Storage | `storage/search.py` | 283-318 | N+1 query in event_type filtering — separate query per session | Move to single JOIN or EXISTS clause |
+| M9 | Storage | `storage/search.py` | 265-279 | Inefficient JSON tag filtering with LIKE — false positives possible | Use proper JSON operators |
+| M10 | SDK | `agent_debugger_sdk/core/exporters/*` | N/A | **No test coverage** for 1,100+ lines of new code (file.py, insights.py, pipeline.py, hindsight.py) | Add unit tests before merge |
+| M11 | SDK | `agent_debugger_sdk/core/context/session_manager.py` | 28-74 | No thread safety — SessionManager can be called from concurrent contexts | Add asyncio.Lock or document as single-threaded only |
+
+---
+
+## Minor Findings (11)
+
+| # | Domain | File | Issue |
+|---|--------|------|-------|
+| m1 | API | `api/analytics_routes.py:260` | `get_repository()` called without `await` on dependency |
+| m2 | API | `api/session_routes.py:215` | Hardcoded `limit=1000` without config constant |
+| m3 | API | `api/replay_routes.py:51-54` | Fragile workaround for FastAPI Query default extraction |
+| m4 | API | `api/services.py:332-384` | SSE 300s timeout not configurable |
+| m5 | Frontend | `frontend/src/components/WhyButton.tsx:68-87` | Doesn't differentiate timeout vs network error for retry |
+| m6 | Frontend | `frontend/src/App.tsx:602-631` | Global keyboard shortcuts may conflict with browser/input |
+| m7 | Frontend | `frontend/src/components/SessionReplay.tsx:183-193` | Duplicate step-backward button |
+| m8 | SDK | `agent_debugger_sdk/adapters/hindsight.py:68-71` | `HindsightConfig.enabled` defaults to `True` (should be opt-in) |
+| m9 | SDK | `agent_debugger_sdk/core/exporters/file.py:139-158` | File paths from `base_dir` not validated — path traversal risk |
+| m10 | SDK | `agent_debugger_sdk/cli.py:101-117` | `run_demo()` suppresses all process output (DEVNULL) |
+| m11 | Storage | `storage/migrations/versions/005_add_patterns.py:22-27` | Idempotency check only in upgrade, not downgrade |
+
+---
+
+## Security Assessment
+
+| Check | Status | Notes |
+|-------|--------|-------|
+| SQL Injection | PASS | All queries use SQLAlchemy ORM with parameterization |
+| Tenant Isolation | PASS | All new queries properly scoped with `tenant_id` |
+| Input Validation | PASS | Pydantic models with Field() constraints, regex validation |
+| CORS | PASS | Configurable via env var, defaults to `*` (local-first tool) |
+| Localhost Protection | PASS | New collector/server.py localhost check |
+| Path Traversal | WARN | File exporter `base_dir` not validated (m9) |
+| CLI Input | WARN | No input validation documented (m10) |
+
+---
+
+## Type Drift Analysis
+
+**No breaking type drift detected between frontend and backend.**
+
+| Schema | Status |
+|--------|--------|
+| SessionSchema / Session | Match |
+| TraceEventSchema / TraceEvent | Match (all new fields aligned) |
+| CheckpointSchema / Checkpoint | Match |
+| ReplayResponse | Match |
+| SimilarFailuresResponse | Match |
+| SearchResponse | Match |
+| AnalyticsResponse | Match |
+| EntityItem | Backend-only (frontend types not yet added — expected) |
+
+---
+
+## Performance Concerns
+
+| Priority | Area | Issue |
+|----------|------|-------|
+| HIGH | `api/entity_routes.py:72` | O(all_events) entity extraction on every request |
+| HIGH | `storage/search.py:283-318` | N+1 query in event_type filtering |
+| MEDIUM | `api/services.py:477-485` | ILIKE OR clause without index support |
+| MEDIUM | `frontend/src/api/client.ts:19` | Unbounded request deduplication Map |
+| MEDIUM | `frontend/src/components/DecisionTree.tsx` | D3 rendering without debouncing |
+
+**Recommended indexes:** `(tenant_id, event_type)`, `(tenant_id, started_at)`
+
+---
+
+## Test Coverage Assessment
+
+### Well-covered areas
+- Pattern detection (613 lines of tests)
+- NL search (421 lines of tests)
+- Entity extraction (339 lines of tests)
+- API contracts, collector regressions, session routes
+
+### Gaps
+- **SDK exporters**: Zero tests for 1,100+ lines of new code (file.py, insights.py, pipeline.py, hindsight.py)
+- **DecisionTree component**: Complex D3 logic untested
+- **SSE reconnection logic**: Not tested
+- **WhyButton error states**: Incomplete coverage
+
+---
+
+## Recommended Actions
+
+### Must Fix (Before Release)
+
+| # | Issue | Effort | Domain |
+|---|-------|--------|--------|
+| 1 | Add test coverage for SDK exporters (M10) | 2-3 hours | SDK |
+| 2 | Fix cosine_similarity division by zero (C4) | 5 min | Storage |
+| 3 | Fix unsafe datetime parsing in search (C1) | 15 min | API |
+| 4 | Fix SSE silent parse failure (C2) | 15 min | Frontend |
+| 5 | Fix SimilarFailuresPanel stale state (C3) | 15 min | Frontend |
+
+### Should Fix (Near-term)
+
+| # | Issue | Effort | Domain |
+|---|-------|--------|--------|
+| 6 | Fix N+1 entity extraction query (M1) | 30 min | API |
+| 7 | Fix N+1 event_type filtering (M8) | 30 min | Storage |
+| 8 | Add database indexes for search (M3) | 15 min | Storage |
+| 9 | Fix timezone inconsistency in pattern_repo (M7) | 5 min | Storage |
+| 10 | Add thread safety or document limitation (M11) | 30 min | SDK |
+| 11 | Add TTL to request deduplication Map (M5) | 15 min | Frontend |
+| 12 | Debounce DecisionTree D3 rendering (M6) | 30 min | Frontend |
+
+### Consider (Backlog)
+
+| # | Issue | Effort | Domain |
+|---|-------|--------|--------|
+| 13 | Opt-in default for HindsightConfig (m8) | 2 min | SDK |
+| 14 | Validate FileExporter base_dir (m9) | 15 min | SDK |
+| 15 | Extract hardcoded limits to config (m2, m4) | 15 min | API |
+| 16 | Use proper JSON operators for tag filtering (M9) | 30 min | Storage |
+| 17 | Fix sessionStore dead code (M4) | 10 min | Frontend |
+| 18 | Remove duplicate step-backward button (m7) | 2 min | Frontend |
+
+---
+
+## Cross-Domain Observations
+
+1. **Consistent architecture** — Repository pattern, tenant isolation, and separation of concerns are well-maintained across all domains.
+2. **Good error handling culture** — Most code paths handle errors gracefully, with proper wrapping and logging.
+3. **Test quality is high where it exists** — Tests are thorough with good edge case coverage.
+4. **Main risk is missing tests** — The SDK exporters represent 1,100+ lines of untested new code.
+5. **Performance will degrade at scale** — Several N+1 query patterns need attention before production workloads.