feat(cli): insights command with --native, --hook, --force modes (Issue #240)

[melagiri]

What

New code-insights insights <session_id> CLI command (Issue 3 of 6 in Phase 12, v4.8.0). Analyzes a session using either claude -p (native, zero-config) or the configured LLM provider, saving insights and prompt quality scores to SQLite.

Also adds V8 schema migration, syncSingleFile() for targeted syncing, and an insights check subcommand.

Why

Enables the zero-config analysis path: Claude Code users can run code-insights insights --hook --native -q from a SessionEnd hook without needing a separate API key. Closes Issue #240 (Issue 3 scope).

How

• cli/src/commands/insights.ts — new command with full flag support (--native, --hook, --force, -q, --source). Two-pass analysis: session analysis then prompt quality, each using the runner abstraction. DB writes are inline (no circular @code-insights/server import).
• cli/src/commands/sync.ts — exports syncSingleFile() for targeted single-file sync without provider scanning.
• cli/src/db/migrate.ts — applyV8() adds session_message_count INTEGER to analysis_usage for resume detection.
• cli/src/db/schema.ts — bumps CURRENT_SCHEMA_VERSION to 8.
• cli/src/index.ts — registers insights command and insights check subcommand.

Runner dispatch:

Invocation	Runner	Sync
<id> --native	ClaudeNativeRunner	none
<id>	ProviderRunner	none
--hook --native	ClaudeNativeRunner	syncSingleFile
--hook	ProviderRunner	syncSingleFile

Resume detection (hook mode only): compares sessions.message_count vs analysis_usage.session_message_count. Skips if unchanged, bypassed with --force.

Schema Impact

• SQLite schema changed: ALTER TABLE analysis_usage ADD COLUMN session_message_count INTEGER
• Types changed: no (uses existing AnalysisRunner interface from runner-types.ts)
• Server API changed: no
• Backward compatible: yes — new column is nullable, existing rows unaffected

Verification

• Build: PASS (pnpm build from repo root, zero errors)
• Tests: PASS (999 tests across 47 test files, zero failures)

Test plan

• V8 migration adds session_message_count column (3 migration tests)
• Provider mode calls ProviderRunner.fromConfig(), not ClaudeNativeRunner.validate()
• Native mode calls ClaudeNativeRunner.validate() and uses native runner
• Insights + analysis_usage rows saved after successful analysis
• session_message_count written to analysis_usage (V8 field)
• Session not found → throws with clear error
• --force bypasses resume detection even with matching message count
• Hook mode: skips when message count matches, proceeds when different or absent
• syncSingleFile() calls provider.parse() and inserts session + messages
• syncSingleFile() is a no-op when parse() returns null
• All 47 pre-existing test files still pass (no regressions)

[melagiri]

TA Synthesis (Phase 2): feat(cli): insights command — Round 1

Review of Specialist Comments

FIX NOW #1 — Migration function ordering (Unanimous: Node/CLI + SQL + TA Insider)

All three reviewers flagged this independently. Verified in the diff: applyV8() is defined at line ~1153 of the diff, BEFORE applyV7() at line ~1158. The call order in runMigrations() is correct (V7 before V8), so this is a file ordering convention violation, not a runtime bug. However, per project convention (MEMORY.md: "Migration functions must be in chronological order in migrate.ts"), this MUST be fixed.

Verdict: AGREE — FIX NOW. Move applyV8() definition below applyV7().

---

FIX NOW #2 — Server-side saveAnalysisUsage() clobbers session_message_count (TA Insider only)

Verified by reading server/src/llm/analysis-usage-db.ts:47-68. The server's saveAnalysisUsage() uses INSERT OR REPLACE with 11 columns but does NOT include session_message_count. SQLite's INSERT OR REPLACE deletes the entire row first, then re-inserts — so a dashboard re-analysis drops the CLI-written session_message_count to NULL, breaking resume detection.

However: this is a pre-existing issue in the server code, not introduced by this PR. The CLI path (this PR) correctly writes session_message_count. The server path has never written it because V8 didn't exist before this PR. After this PR merges, the server needs updating — but that's a companion fix, not a blocker for this PR.

Verdict: AGREE the bug is real, but reclassified from FIX NOW to ESCALATE. The server-side saveAnalysisUsage() needs to include session_message_count (or switch from INSERT OR REPLACE to an INSERT ... ON CONFLICT UPDATE that preserves unmentioned columns). This should be a tracked follow-up issue, not a blocker for this PR. The CLI path works correctly in isolation. The clobber only occurs if: (1) user analyzes via CLI hook, (2) then re-analyzes the same session from dashboard. Until the server is updated, resume detection degrades gracefully — it re-analyzes instead of skipping. No data loss, just redundant work.

---

SUGGESTION — estimated_cost_usd: 0 for provider mode (Node/CLI + TA Insider)

Two reviewers independently flagged that provider mode hardcodes estimated_cost_usd: 0. For native mode this is correct (no API cost). For provider mode, the runner returns inputTokens/outputTokens but no cost calculation is applied.

Verdict: AGREE as suggestion. The CLI doesn't have access to analysis-pricing.ts (server-only). Adding cost computation would require either duplicating the pricing logic or extracting it to a shared module. This is a valid enhancement but not a merge blocker — the token counts are recorded, cost can be computed retroactively. Non-blocking.

---

SUGGESTION — Missing [pattern-monitor] console.warn in inline saveFacetsToDb (TA Insider)

The CLI's inline saveFacetsToDb skips the [pattern-monitor] / [cot-monitor] console.warn guards that exist in the server version.

Verdict: PUSHBACK — NOT APPLICABLE. In hook/CLI mode, console.warn output would pollute the terminal during automated SessionEnd hooks. The server version runs in a long-lived process where monitoring makes sense. For CLI one-shot execution, the facets are saved and visible in the dashboard. Monitoring warnings add noise without actionable value in this context.

---

NOTE — Inline DB helpers duplicate server logic (Node/CLI + SQL)

Both specialists noted this. The PR comment documents it as a deliberate trade-off (no cross-package import from CLI to server).

Verdict: AGREE it's a noted trade-off, not an issue. The CLI package cannot import from the server package. Documented inline.

---

NOTE — deleteSessionInsights dynamic SQL (Node/CLI)

Safe pattern — parameterized placeholders with spread, not string interpolation. The IN (?, ?, ?) is built from array length, values go through .run(...params).

Verdict: NOT APPLICABLE. No injection risk.

---

NOTE — Test seed helper string interpolation (SQL)

seedSession() uses template literals in SQL. Test-only code with hardcoded values, not user input.

Verdict: NOT APPLICABLE. Test code with controlled inputs.

Second Pass Findings

No additional issues found beyond what the three reviews covered. The test coverage is solid — resume detection, force flag, native vs provider mode, V8 migration, and syncSingleFile are all tested.

Consolidated Review (For Dev Agent)

🔴 FIX NOW:

1. Migration function ordering — Move applyV8() function definition BELOW applyV7() in cli/src/db/migrate.ts. The function is currently defined before V7, violating the chronological convention. (Call site order in runMigrations() is already correct.)

⚠️ ESCALATE TO FOUNDER:

1. Server-side saveAnalysisUsage() will clobber session_message_count on dashboard re-analysis. server/src/llm/analysis-usage-db.ts uses INSERT OR REPLACE with 11 columns, omitting session_message_count. After V8, dashboard re-analysis will NULL out the CLI-written value. Recommended fix (companion PR): Either add session_message_count to the server's saveAnalysisUsage(), or switch to INSERT ... ON CONFLICT(session_id, analysis_type) DO UPDATE SET ... which preserves unmentioned columns. This does NOT block this PR — the degradation is graceful (re-analyzes instead of skipping).

❌ NOT APPLICABLE:

1. [pattern-monitor] console.warn in CLI inline saveFacetsToDb — CLI runs as one-shot hook; monitoring warnings would pollute automated output with no actionable value.
2. deleteSessionInsights dynamic SQL — parameterized, no injection risk.
3. Test seed string interpolation — test-only with hardcoded values.

🟡 SUGGESTIONS (non-blocking, dev's discretion):

1. estimated_cost_usd: 0 for provider mode — valid enhancement, but requires shared pricing module or duplication. Token counts are recorded; cost can be computed retroactively. Consider as follow-up.
2. ANALYSIS_VERSION hardcoded in CLI inline helpers — should stay in sync with server value. Currently both are '3.0.0'. Add a comment noting the dependency.

Final Verdict

CHANGES REQUIRED — Round 2 needed

One FIX NOW item (migration ordering — trivial fix, ~5 line move). One ESCALATE item for founder decision on companion PR scope.

[melagiri]

Review Addressal

FIX NOW items addressed:

1. Migration function ordering in cli/src/db/migrate.ts → Fixed: moved applyV8() to after applyV7(), restoring chronological order (V1...V8). No logic change — runMigrations() call order was always correct.

Pre-PR gate: pnpm build passing (zero errors), pnpm test 513/513 passing in cli (24 test files).

All review items addressed. Ready for re-review or merge.

[melagiri]

Review Addressal

FIX NOW items addressed:

1. Migration ordering (applyV8 defined before applyV7) → Fixed: moved applyV8 after applyV7 in cli/src/db/migrate.ts (commit fix(db): restore chronological order of migration functions)

2. INSERT OR REPLACE clobbers session_message_count → Fixed: switched both server/src/llm/analysis-usage-db.ts and cli/src/commands/insights.ts from INSERT OR REPLACE to INSERT ... ON CONFLICT DO UPDATE. Server's SET clause does not include session_message_count (preserving what CLI writes). CLI's SET clause includes session_message_count so re-analysis updates it. JSDoc updated to explain the pattern. (commit fix(db): use ON CONFLICT DO UPDATE in saveAnalysisUsage to preserve session_message_count)

Pre-PR gate: pnpm build passing (run from worktree root), 513 CLI tests + 486 server tests all passing.

All review items addressed. Ready for re-review or merge.