refactor: move analysis DB helpers from server to CLI (Option A deduplication)

[melagiri]

What

Move analysis-db.ts and analysis-usage-db.ts from server/src/llm/ to cli/src/analysis/. Server files become thin re-export wrappers. Eliminates ~170 lines of inline duplicates from insights.ts.

Closes #248.

Why

The CLI had ~170 lines of inline DB helpers duplicating the server's analysis-db.ts and analysis-usage-db.ts. Any change to the DB persistence logic required updating both copies. Moving to CLI with server re-exports follows the same pattern established by PR #238 (prompt module migration).

How

• New cli/src/analysis/analysis-db.ts: all insight/facet DB helpers with converged divergences:
  • deleteSessionInsights gains includeOnlyTypes option (from server version)
  • saveFacetsToDb takes analysisVersion as optional parameter with default ANALYSIS_VERSION
  • Defensive ?? [] guards on decisions/learnings (from CLI inline version)
  • Preserves [pattern-monitor] and [pq-monitor] console.warn monitors
• New cli/src/analysis/analysis-usage-db.ts: usage tracking with session_message_count as optional field on SaveAnalysisUsageData
• Server re-exports: same public API, adds convertPromptQualityToInsightRow alias for backward compat
• cli/src/commands/insights.ts: replaces ~170 inline lines with imports; adds sessionData adapter to bridge null (SQLite row) → undefined (SessionData interface)
• cli/package.json: adds ./analysis/analysis-db and ./analysis/analysis-usage-db exports

Schema Impact

• SQLite schema changed: no
• Types changed: no (new types in CLI, server re-exports them)
• Server API changed: no
• Backward compatible: yes — server re-exports preserve all existing import paths

Testing

• pnpm build: PASS (zero errors)
• pnpm test: PASS (1016 tests across CLI + server)
• TDD: analysis-db.test.ts (15 tests) and analysis-usage-db.test.ts (8 tests) written first, RED verified, then implementation made them GREEN

Verification

• Build: PASS
• CLI Tests: PASS (530 tests)
• Server Tests: PASS (486 tests)

[melagiri]

TA Review (Phase 1 - Insider): refactor: move analysis DB helpers from server to CLI — Round 1

TA Plan Compliance (6 adjustments)

#	Adjustment	Status	Evidence
1	Both new files in cli/src/analysis/ (NOT cli/src/db/)	PASS	cli/src/analysis/analysis-db.ts and cli/src/analysis/analysis-usage-db.ts
2	ANALYSIS_VERSION as parameter with default	PASS	saveFacetsToDb(sessionId, facets, analysisVersion: string = ANALYSIS_VERSION)
3	getSessionAnalysisUsage() moved too (table cohesion)	PASS	Present in cli/src/analysis/analysis-usage-db.ts, re-exported from server
4	All console.warn monitors preserved	PASS	[pattern-monitor] preserved in saveFacetsToDb. [pq-monitor] was already removed in a prior PR (confirmed via CHANGELOG) — not a gap
5	Divergences converged: includeOnlyTypes, session_message_count optional, defensive ?? []	PASS	See details below
6	SessionData and SessionRow kept as separate types	PASS	SessionData in analysis-db.ts, SessionRow remains local to insights.ts

Divergence convergence details (adjustment 5):

• deleteSessionInsights: gains includeOnlyTypes option from server version — PASS
• session_message_count: optional on SaveAnalysisUsageData, included in INSERT SQL — PASS
• Defensive ?? [] on response.decisions and response.learnings in convertToInsightRows — PASS (line 577: response.decisions ?? [], line 617: response.learnings ?? [])
• SaveAnalysisUsageData.chunk_count: now optional with ?? 1 default — PASS (converged from server's required field + CLI's hardcoded 1)

Architecture Alignment

Data Contract Impact

• Types aligned across CLI, server, and dashboard — InsightRow, SessionData, DeleteOptions re-exported from server
• SQLite schema NOT changed — no migrations needed
• CLI binary name is code-insights — unchanged
• No breaking changes to existing SQLite data
• Server re-exports preserve ALL existing public exports (verified: convertPromptQualityToInsightRow alias included)

Pattern Consistency

• Matches PR refactor: migrate prompt modules from server to CLI (native analysis foundation) #238 precedent (prompt module migration: CLI owns, server re-exports)
• SQLite queries use prepared statements
• getDb() import from ../db/client.js — correct relative path within CLI
• Normalizers imported from sibling modules in cli/src/analysis/
• No circular dependencies — analysis-db.ts imports from db/client, pattern-normalize, prompt-quality-normalize (all leaf modules)

Test Coverage (TDD Domain Verification)

• cli/src/analysis/__tests__/analysis-db.test.ts — 15 tests covering ANALYSIS_VERSION, convertToInsightRows, convertPQToInsightRow, deleteSessionInsights SQL logic, saveFacetsToDb version parameter
• cli/src/analysis/__tests__/analysis-usage-db.test.ts — 8 tests covering SaveAnalysisUsageData interface shape, saveAnalysisUsage SQL logic (insert + upsert), getSessionAnalysisUsage SQL logic
• Tests use in-memory SQLite — correct pattern, no file system side effects

Server Re-Export Completeness

server/src/llm/analysis-db.ts re-exports:

Export	Type	Status
saveInsightsToDb	value	PASS
deleteSessionInsights	value	PASS
saveFacetsToDb	value	PASS
convertToInsightRows	value	PASS
convertPQToInsightRow	value	PASS
convertPromptQualityToInsightRow (alias)	value	PASS — backward compat for prompt-quality-analysis.ts
ANALYSIS_VERSION	value	PASS
InsightRow	type	PASS
SessionData	type	PASS
DeleteOptions	type	PASS

server/src/llm/analysis-usage-db.ts re-exports:

Export	Type	Status
saveAnalysisUsage	value	PASS
getSessionAnalysisUsage	value	PASS
SaveAnalysisUsageData	type	PASS
AnalysisUsageRow	type	PASS

CLI package.json Exports

• "./analysis/analysis-db": "./dist/analysis/analysis-db.js" — correct
• "./analysis/analysis-usage-db": "./dist/analysis/analysis-usage-db.js" — correct

insights.ts Adapter Pattern

• sessionData adapter correctly bridges SessionRow (null) → SessionData (undefined) via ?? undefined
• Inline DB helpers fully removed (~170 lines: saveInsightsToDb, deleteSessionInsights, saveFacetsToDb, saveAnalysisUsage, convertToInsightRows, convertPQToInsightRow, InsightRow interface, ANALYSIS_VERSION constant)
• randomUUID import removed (no longer needed in insights.ts)
• normalizePatternCategory and normalizePromptQualityCategory imports removed (now internal to analysis-db.ts)

Issues Found

No FIX NOW issues.

No SUGGESTIONS.

NOTES:

1. NOTE: The altBullets filter in convertToInsightRows was tightened from a && typeof a === 'object' && a.option (server) to a TypeScript type guard (a): a is { option: string; rejected_because: string } => Boolean(a?.option). Functionally equivalent, better typed. No concern.

2. NOTE: The old CLI inline saveFacetsToDb had facets.course_correction_reason ?? null while the new version (matching server) uses facets.course_correction_reason without the guard. This is safe — undefined is coerced to null by better-sqlite3.

3. NOTE: The new SaveAnalysisUsageData now includes session_message_count which the old server version lacked. This is the correct convergence — the CLI needs this field for resume detection, and the server re-export inherits the superset interface. Server callers that don't pass session_message_count get undefined ?? null which is correct.

Phase 1 Verdict

[x] Approved (from architecture perspective)

All 6 TA plan adjustments implemented correctly. Server re-exports are complete with backward-compatible aliases. Inline duplicates fully eliminated. No data contract breaks. TDD tests cover the key behaviors. Clean execution of Option A deduplication.

[melagiri]

TA Synthesis (Phase 2): refactor: move analysis DB helpers from server to CLI — Round 1

Review of Specialist Comments

Node/CLI Specialist — APPROVED
All findings reviewed. Module resolution, ESM imports, server re-exports, circular dep direction — all verified correct. No actionable items. Suggestions noted but not required for merge.

SQL/DB Specialist — CHANGES REQUIRED
One FIX NOW item identified. I verified against the diff:

• Old server code (removed, lines 1680-1689): The ON CONFLICT UPDATE SET clause deliberately excluded session_message_count — server callers never write this field, so it was never touched on conflict.
• New unified CLI code (line 891): The ON CONFLICT UPDATE SET clause includes session_message_count = excluded.session_message_count. When the server calls saveAnalysisUsage without session_message_count, the field binds to NULL (line 904: data.session_message_count ?? null), which then overwrites any CLI-written value with NULL.

This is a regression: the deduplication merged two callers with different write patterns into one function without protecting the optional field. Same class of bug as PR #247.

AGREE with SQL specialist. Finding is valid and verified against the diff.

---

Consolidated Review (For Dev Agent)

FIX NOW:

1. session_message_count NULL clobber on server re-analysis — In cli/src/analysis/analysis-usage-db.ts, the saveAnalysisUsage ON CONFLICT clause must use COALESCE to preserve existing values when NULL is passed:

   -- Current (broken):
   session_message_count = excluded.session_message_count
   
   -- Fix:
   session_message_count = COALESCE(excluded.session_message_count, analysis_usage.session_message_count)

   The test in analysis-usage-db.test.ts should also add a case: insert with session_message_count = 42, then upsert without session_message_count (undefined/null), verify the value is still 42.

NOT APPLICABLE:
(none)

SUGGESTIONS (non-blocking):

1. Node/CLI specialist noted sessionData adapter does partial conversion — correct as-is, no change needed.
2. Node/CLI specialist noted analysis_type union could be broadened — improvement for a future PR, not this one.

---

Final Verdict

Changes Required — one FIX NOW item before merge.

Dev agent: fix the COALESCE issue, add a regression test for the NULL-clobber case, then report back.

[melagiri]

Review Addressal

FIX NOW items addressed:

1. session_message_count NULL clobber in saveAnalysisUsage → Fixed: changed session_message_count = excluded.session_message_count to session_message_count = COALESCE(excluded.session_message_count, analysis_usage.session_message_count) in cli/src/analysis/analysis-usage-db.ts. Regression test added: CLI writes 42, server upserts without session_message_count (null), 42 is preserved.

Pre-PR gate: pnpm build passing, pnpm test 1017/1017 passing (run from repo root).

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

[melagiri]

Triple-Layer Code Review — Round 1

Reviewers

Role	Domain	Verdict
TA (Insider)	Architecture, TA plan compliance	✅ APPROVED
Node/CLI Specialist	ESM, re-exports, imports	✅ APPROVED
SQL/DB Specialist	Query correctness, SQL safety	✅ APPROVED (after fix)

Pre-Review Gates

• New dependency audit: N/A
• Functional verification evidence: Build PASS, CLI tests PASS (530), Server tests PASS (486)
• Visual output attached: N/A

TA Plan Compliance (6 adjustments)

All 6 TA-approved adjustments verified implemented:

1. ✅ Files in cli/src/analysis/
2. ✅ ANALYSIS_VERSION as parameter with default
3. ✅ getSessionAnalysisUsage() moved (table cohesion)
4. ✅ [pattern-monitor] console.warn preserved
5. ✅ Divergences converged (includeOnlyTypes, session_message_count, ?? [] guards)
6. ✅ SessionData and SessionRow kept as separate types

Issues Found & Resolution

🔴 FIX NOW (resolved)

1. session_message_count NULL clobber — ON CONFLICT SET overwrote CLI-written values with NULL when server callers didn't provide the field. Fixed with COALESCE(excluded.session_message_count, analysis_usage.session_message_count) + regression test added.

🟡 SUGGESTIONS (non-blocking)

• convertPQToInsightRow naming alias — backward compat handled correctly
• sessionData adapter partial conversion — correct as-is
• analysis_type union broadened to include 'facet' — improvement

Round 1 verdict: PASS — Ready for merge