feat(cli): insights check — count-based behavior + --analyze flag

[melagiri]

What

Enhances code-insights insights check with count-based behavior and a new --analyze flag. Closes #242.

Why

Users needed a way to discover and recover unanalyzed sessions without manually hunting for session IDs. The hook installs analysis on new sessions but doesn't backfill existing ones.

How

• insightsCheckCommand is now async (required for --analyze batch processing)
• Count-based dispatch:
  • 0: silent exit
  • 1–2: auto-analyze silently using ClaudeNativeRunner (zero-config)
  • 3–10: print count + suggest insights check --analyze
  • 11+: print count + time estimate (~22s/session) + suggest command
• --analyze: process all found sessions sequentially with [N/total] session-title ... done (Xs) progress; errors per-session are logged and processing continues
• --quiet: outputs just the count as a number (for scripting)
• --days: overrides lookback window (default 7)
• Added _runner param to runInsightsCommand for batch callers to reuse a single runner instance (avoids repeated validate() calls)
• Wired --analyze option in index.ts check subcommand

Schema Impact

• SQLite schema changed: no
• Types changed: InsightsCommandOptions — added optional _runner field (internal, not user-facing)
• Server API changed: no
• Backward compatible: yes

Testing

Build & Tests

• Build: PASS (pnpm build from repo root)
• Tests: PASS (27 test files, 0 failures)

New test coverage (TDD)

Added 13 new tests in cli/src/commands/__tests__/insights.test.ts:

• insightsCheckCommand — count-based behavior (6 tests): silent on 0, quiet count, 3-10 suggestion, 11+ time estimate, --days window
• insightsCheckCommand — auto-analyze (1-2 sessions) (2 tests): validate() called, runner used
• insightsCheckCommand — --analyze flag (3 tests): progress output, error resilience, silent on 0

Test plan

• 0 unanalyzed → silent exit
• 1-2 unanalyzed → auto-analyze silently
• 3-10 unanalyzed → print count + suggest --analyze
• 11+ unanalyzed → print count + time estimate + suggest --analyze
• --analyze processes all sessions with [N/total] progress
• --quiet outputs just the count
• --days overrides lookback window
• Error in one session doesn't stop batch processing

[melagiri]

TA Review (Phase 1 - Insider): insights check — count-based behavior + --analyze flag — Round 1

Data Contract Impact

• Types aligned across CLI, server, and dashboard — only InsightsCommandOptions touched (CLI-internal), no cross-layer impact
• SQLite schema unchanged — no migrations needed
• CLI binary remains code-insights
• No breaking changes to existing SQLite data

Architecture Alignment

SQL Query (LEFT JOIN anti-pattern): The fix from au.analysis_type IS NULL → au.session_id IS NULL is correct and idiomatic. Since analysis_usage has PRIMARY KEY (session_id, analysis_type), the LEFT JOIN with analysis_type = 'session' in the ON clause means au.session_id IS NULL correctly identifies sessions with no matching analysis_usage row. Good fix.

Count-based dispatch logic: Verified the dispatch tiers:

• 0 → silent exit ✅
• 1-2 → auto-analyze silently via ClaudeNativeRunner ✅
• 3-10 → print count + suggest --analyze ✅
• 11+ → print count + time estimate + suggest --analyze ✅

The ordering is correct: --quiet and count === 0 checks come first, then --analyze (processes all regardless of count), then auto-analyze (count <= 2), then 3-10, then 11+. No fall-through issues.

_runner reuse pattern: Clean approach. The underscore prefix correctly signals internal-only. ClaudeNativeRunner.validate() is called once before the batch loop, then the runner instance is passed via _runner to skip redundant validation. The AnalysisRunner interface is stateless (no mutable state between calls), so sharing is safe.

Error resilience: Each session in the --analyze loop is wrapped in its own try/catch. Failures log to stderr and processing continues. Success count is tracked separately for the summary line. The auto-analyze path (1-2 sessions) silently catches errors — appropriate for the "silent auto-analyze" behavior.

Async migration: insightsCheckCommand changed from sync to async. The index.ts action handler updated accordingly with async/await. No other callers exist in the codebase.

Test Coverage (TDD Domain Verification)

• cli/src/commands/__tests__/insights.test.ts — 13 new tests covering all dispatch tiers, auto-analyze, --analyze progress, error resilience, --quiet, --days, and 0-session edge cases
• Tests use in-memory SQLite with real migrations — good integration test pattern
• Mock structure correctly mocks ClaudeNativeRunner class with static validate and instance runAnalysis

Pattern Consistency

• Matches existing codebase patterns (chalk colors, [Code Insights] prefix, process.stdout.write for progress)
• SQLite queries use prepared statements
• Runner interface followed correctly

Issues Found

🔴 FIX NOW:

1. InsightsCommandOptions.sessionId is required but batch callers must provide it — The _runner field was added to InsightsCommandOptions, but sessionId is a required (non-optional) field on this interface. The batch call await runInsightsCommand({ sessionId: row.id, native: true, quiet: true, _runner: runner }) correctly provides it. No issue here actually — verified, this is fine.

(No FIX NOW items found.)

🟡 SUGGESTION:

1. Time estimate edge case at exactly 11 sessions: 11 * 22 = 242s → Math.round(242/60) = 4 min → displays ~4 min. The estimateMins < 2 guard means anything under 120s shows seconds. At 5 sessions (if this path were reachable), 5*22=110s → ~2 min. This is fine, but the ~ prefix in ~${estimateSecs}s only appears for the seconds case — consider consistency. Minor UX polish, not blocking.

2. SELECT columns in check query: The query now selects generated_title, custom_title, started_at, message_count — but only --analyze and auto-analyze paths use the title. The suggest/estimate paths (3-10, 11+) don't use per-row data. This is fine for correctness (SQLite is efficient here), but if the check query is called frequently (e.g., in hooks), selecting fewer columns for the count-only path could be marginally more efficient. Not blocking.

🔵 NOTE:

1. SECONDS_PER_SESSION = 22 — Hardcoded estimate. If native analysis performance changes significantly, this will drift. Consider making this a comment-documented constant with a note about the empirical basis. Already a module-level const, which is good.

2. No process.exit(1) in the new async paths — The original insightsCheckCommand had process.exit(1) in the catch block. The new code preserves this for the outer try/catch. Individual session failures in --analyze do NOT exit — correct behavior for batch processing.

Phase 1 Verdict

• Approved (from architecture perspective)

Clean implementation. No schema changes, no type contract breaks, no cross-layer impact. The SQL fix is correct, the dispatch logic is sound, error resilience is properly implemented, and test coverage is thorough. The _runner reuse pattern is a pragmatic optimization that avoids repeated validate() calls.

[melagiri]

Triple-Layer Code Review — Round 1

Reviewers

Role	Domain	Verdict
TA (Insider)	Architecture, SQL, async patterns	✅ APPROVED
Node/CLI Specialist	CLI UX, error resilience, async safety	✅ APPROVED

Pre-Review Gates

• New dependency audit: N/A
• Functional verification evidence: Build PASS, Tests PASS (27 files, 0 failures)
• Visual output attached: N/A

Issues Found & Resolution

🔴 FIX NOW

None.

🟡 SUGGESTIONS (non-blocking)

• SQL anti-join column change (au.session_id IS NULL vs au.analysis_type IS NULL) — neutral, both correct
• _runner field on public interface — mild code smell, acceptable for internal use
• Auto-analyze (1-2) silently swallows errors — appropriate for "opportunistic" UX
• Time estimate constant (22s) scoped to native runner — correct for current use

Round 1 verdict: PASS — Ready for merge