feat(cli): update install-hook to add SessionEnd analysis hook

[melagiri]

What

Updates install-hook to install both a Stop (sync) hook and a SessionEnd (analysis) hook by default. Adds --sync-only and --analysis-only flags for granular control. Updates uninstall-hook to remove both hook types.

Closes #241.

Why

The insights command (Issue #240) can now analyze sessions via claude -p with no API key. To make this automatic, users need a SessionEnd hook that fires code-insights insights --hook --native -q when a session ends. Without this PR, users must manually run analysis.

How

• installHookCommand() now accepts InstallHookOptions ({ syncOnly?, analysisOnly? })
• Default install writes both Stop and SessionEnd entries to ~/.claude/settings.json
• SessionEnd hook has timeout: 120000 (2 minutes — analysis can take ~15-30s)
• uninstallHookCommand() filters both Stop and SessionEnd arrays
• Duplicate detection works per-hook-type (re-running doesn't add duplicates)
• Telemetry tracks hook_types, sync_installed, analysis_installed

Hook Event Verification

SessionEnd is a valid Claude Code hook event, confirmed via Context7 docs against the official anthropics/claude-code repo. Full event list: PreToolUse, PostToolUse, Stop, SubagentStop, SessionStart, SessionEnd, UserPromptSubmit, PreCompact, Notification. No fallback to Stop was needed.

Schema Impact

• SQLite schema changed: no
• Types changed: no (new InstallHookOptions interface is internal to install-hook.ts)
• Server API changed: no
• Backward compatible: yes — existing Stop hooks are preserved

Testing

• 15 new tests in cli/src/commands/__tests__/install-hook.test.ts
• Tests written first (RED), then implementation (GREEN)
• All 27 CLI test files pass (546 tests)
• pnpm build passes from repo root

Test plan

• Default install-hook installs both Stop and SessionEnd hooks
• --sync-only installs only Stop hook
• --analysis-only installs only SessionEnd hook
• Duplicate detection works for both hook types independently
• uninstall-hook removes both hook types
• uninstall-hook preserves non-code-insights hooks in both arrays
• uninstall-hook cleans up empty hooks object
• SessionEnd hook has timeout: 120000
• Existing settings.json content preserved on install

[melagiri]

TA Review (Phase 1 - Insider): feat(cli): update install-hook to add SessionEnd analysis hook — Round 1

Data Contract Impact

• Types aligned across CLI, server, and dashboard — N/A, this PR is CLI-internal only. No new types in cli/src/types.ts, no SQLite schema changes, no server/dashboard changes.
• SQLite schema changes have proper migrations — N/A, no schema changes.
• CLI binary name is code-insights — Confirmed, hook detection uses includes('code-insights').
• No breaking changes to existing SQLite data — Confirmed.

Test Coverage (TDD Domain Verification)

• cli/src/commands/install-hook.ts change → __tests__/install-hook.test.ts present — YES, 15 new tests covering: default install (both hooks), --sync-only, --analysis-only, duplicate detection (per-hook-type), uninstall (both types), preserve non-CI hooks, empty cleanup, missing settings.json.

Pattern Consistency

• Matches existing codebase patterns — Yes, follows the same fs.readFileSync → JSON.parse → mutate → JSON.stringify → fs.writeFileSync pattern.
• Hook detection uses existing getHookCommand() helper with includes('code-insights') — consistent.
• Telemetry tracking follows existing trackEvent pattern with appropriate properties.
• ClaudeSettings interface updated to include SessionEnd alongside existing Stop and PostToolUse.

Architecture Alignment

• SessionEnd hook command: node ${cliPath} insights --hook --native -q — Verified that --hook, --native, and -q are all registered flags on the insights command in index.ts:125-129. Correct.
• Timeout: 120000 ms (2 minutes) on the SessionEnd hook — appropriate given analysis takes ~15-30s.
• Settings.json structure: settings.hooks.SessionEnd[] matches Claude Code's hook configuration format (same structure as Stop).
• InstallHookOptions interface: Internal to install-hook.ts, exported but not in types.ts — correct, this is command-level plumbing, not a data contract type.
• Duplicate detection refactored: Extracted hookAlreadyInstalled() helper used for both Stop and SessionEnd — clean DRY improvement.
• Uninstall handles both arrays: Filters both Stop and SessionEnd, deletes empty arrays, deletes empty hooks object. Correct.

Issues Found

🔴 FIX NOW:

1. No guard against --sync-only and --analysis-only both being set. If a user runs code-insights install-hook --sync-only --analysis-only, the logic sets installSync = !analysisOnly = false and installAnalysis = !syncOnly = false, resulting in neither hook being installed — but the code still writes settings.json and prints "Hook installed successfully!" (if hooks object was previously empty and no prior CI hooks exist, it hits the "already installed" early return — but if there ARE pre-existing hooks, it writes and claims success with an empty installedTypes array). This is a confusing UX edge case. Fix: Add a mutual exclusivity check at the top of installHookCommand:

   if (syncOnly && analysisOnly) {
     console.log(chalk.red('Cannot use --sync-only and --analysis-only together. Use neither for both.'));
     return;
   }

🟡 SUGGESTION:

1. Tests operate on real ~/.claude/settings.json — The tests back up and restore the real file, but this is fragile. If the test process crashes mid-run, the user's real settings.json could be corrupted or deleted. A safer approach would be to mock HOOKS_FILE to point to a temp directory. However, this is the existing pattern from the codebase and the backup/restore logic is present, so this is not a blocker — just a note for future improvement.

2. Early return message when both hooks already exist could be clearer. When one hook is already installed but not the other, the code correctly installs only the missing one. But when BOTH are already installed on a default run, it prints "Stop hook already installed." then "SessionEnd hook already installed." then "Code Insights hooks already installed." — three messages saying basically the same thing. Consider consolidating to just the final summary message. Minor UX polish, not blocking.

🔵 NOTE:

1. Commander.js does not enforce mutual exclusivity of --sync-only and --analysis-only natively. The fix must be in application code (as suggested above). Commander has a .conflicts() API in newer versions, but given our locked Commander.js version, a simple if guard is sufficient.

2. installHookCommand signature changed from () => Promise<void> to (options?: InstallHookOptions) => Promise<void>. The default = {} ensures backward compatibility — existing code calling installHookCommand() with no args still works. The index.ts wiring passes the Commander options correctly via (opts) => installHookCommand({ syncOnly: opts.syncOnly, analysisOnly: opts.analysisOnly }).

Phase 1 Verdict

• Changes Required

One FIX NOW item: Add mutual exclusivity guard for --sync-only + --analysis-only. The rest of the implementation is clean, well-tested, and architecturally sound. No schema impact, no type contract changes, no cross-layer concerns.

[melagiri]

TA Synthesis (Phase 2): feat(cli): update install-hook to add SessionEnd analysis hook — Round 1

Consolidated Review (For Dev Agent)

FIX NOW:

1. --sync-only + --analysis-only mutual exclusion guard — Both reviewers independently flagged this. If a user passes both --sync-only and --analysis-only, neither hook gets installed but the command reports success. Add a check at the top of the handler: if both flags are set, print an error message (e.g., "Cannot use --sync-only and --analysis-only together") and return early with a non-zero exit. This is a correctness bug, not a style issue.

SUGGESTIONS (non-blocking, not required for this PR):

1. Consolidate "already installed" messages — When both hooks already exist, the output could be a single message instead of two separate lines. Minor UX polish, can be a follow-up.
2. process.argv[1] path stability — Pre-existing concern about resolving the CLI binary path. Not introduced by this PR.
3. Missing telemetry event on uninstall — Pre-existing gap. Not in scope for this PR.
4. Non-atomic write to settings.json — Pre-existing pattern. Not introduced by this PR.

NOT APPLICABLE:

• No items dismissed. Both reviewers were aligned; no conflicts to resolve.

Final Verdict

Changes Required — One FIX NOW item. Once the mutual exclusion guard is added, this PR is ready.

[melagiri]

Review Addressal

FIX NOW items addressed:

1. --sync-only + --analysis-only mutual exclusion → Fixed: added guard at the top of installHookCommand that returns early with a clear error message when both flags are passed simultaneously. Added test confirming no settings.json is written in this case (now 16 tests total, all passing).

Pre-PR gate: pnpm build passing (run from repo root), 27 CLI test files / 547 tests passing.

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

[melagiri]

Triple-Layer Code Review — Round 1

Reviewers

Role	Domain	Verdict
TA (Insider)	Architecture, settings schema	✅ APPROVED (after fix)
Node/CLI Specialist	CLI design, file system, flags	✅ APPROVED (after fix)

Pre-Review Gates

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

Issues Found & Resolution

🔴 FIX NOW (resolved)

1. --sync-only + --analysis-only mutual exclusion — both flags resulted in neither hook installed with misleading output. Fixed with guard check at handler top + test added.

🟡 SUGGESTIONS (non-blocking, pre-existing)

• process.argv[1] path stability across npx/npm link (pre-existing)
• Tests operate on real ~/.claude/settings.json (pre-existing pattern)
• Missing telemetry on uninstall (pre-existing)

Round 1 verdict: PASS — Ready for merge

[melagiri]

Review Addressal (Round 2)

Items addressed:

1. Console spy added to mutual exclusion test → Test now asserts expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('Cannot use')) before checking no file was written. Prevents silent regression if the error message is removed but the early return is kept.

2. Consolidated "already installed" messages → Removed per-hook inline prints (Stop hook already installed. / SessionEnd hook already installed.). The single consolidated check at the end now shows one message: Code Insights hooks already installed (sync + analysis). (label adapts based on which hooks were requested). Updated the duplicate detection test to assert exactly one "already installed" log call and that it contains sync + analysis.

Pre-PR gate: pnpm build passing (repo root), 27 CLI test files / 547 tests passing.

[melagiri]

Review Addressal (Round 3)

Items addressed:

3. process.argv[1] path stability → Replaced with import.meta.url-based resolution (Option A). CLI_ENTRY is computed once at module load as path.resolve(fileURLToPath(import.meta.url), '../../index.js') — stable across npm link, global install, and npx. Matches the pattern already used in dashboard.ts. Two new test assertions verify the generated commands match /^node .+index\.js (sync|insights) .../.

4. Tests no longer touch real ~/.claude/settings.json → Rewrote test setup using vi.mock('os', ...) with a mutable sentinel object (_mockOs.homeDir) updated in beforeEach. This avoids the ESM hoisting problem (let bindings can't be read before initialization inside a hoisted vi.mock factory, but object properties can). Each test gets an isolated mkdtemp dir; backup/restore logic removed entirely.

Pre-PR gate: pnpm build passing (repo root), 27 CLI test files / 547 tests passing.