From 8977c7e9178f8f3430e901eadcd01f0b9a19da2e Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 10 May 2026 14:41:40 -0700 Subject: [PATCH] feat(telemetry): track agent_hook_install_failed per agent (#1668) * feat(telemetry): track agent_hook_install_failed per agent Replaces the closure-style installer loop in `src/main/index.ts` with a labelled `runManagedHookInstallers` so each catch can attribute the failure to its agent. Adds the `agent_hook_install_failed` event + `hookInstallAgentSchema` enum (claude/codex/gemini/cursor) and a unit test pinning fail-open semantics, label routing, and the 200-char error_message truncation. Co-authored-by: Orca * fix(telemetry): harden agent-hook installer fail-open - describeError always returns a string (JSON.stringify can return literal undefined for throw undefined / Symbol / function, which would crash the catch handler before track fires) - wrap track() in inner try/catch so a telemetry-side throw can't abort the installer loop - dedupe AGENT_HOOK_TARGETS into one tuple in agent-hook-types so the IPC AgentHookTarget type and hookInstallAgentSchema can't drift - regression tests for object/undefined throws and track-throws Co-authored-by: Orca --------- Co-authored-by: Orca --- .../agent-hooks/install-telemetry.test.ts | 155 ++++++++++++++++++ src/main/agent-hooks/install-telemetry.ts | 53 ++++++ src/main/index.ts | 25 +-- src/shared/agent-hook-types.ts | 3 +- src/shared/telemetry-events.ts | 41 +++-- 5 files changed, 250 insertions(+), 27 deletions(-) create mode 100644 src/main/agent-hooks/install-telemetry.test.ts create mode 100644 src/main/agent-hooks/install-telemetry.ts diff --git a/src/main/agent-hooks/install-telemetry.test.ts b/src/main/agent-hooks/install-telemetry.test.ts new file mode 100644 index 00000000000..9af8a321d4f --- /dev/null +++ b/src/main/agent-hooks/install-telemetry.test.ts @@ -0,0 +1,155 @@ +// Pins the contract between `runManagedHookInstallers` and the +// `agent_hook_install_failed` telemetry event: each catch must fire `track` +// with the correct agent label and a truncated error_message, and one +// installer's failure must not stop the others (fail-open semantics). + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { trackMock } = vi.hoisted(() => ({ trackMock: vi.fn() })) + +vi.mock('../telemetry/client', () => ({ track: trackMock })) + +import { runManagedHookInstallers } from './install-telemetry' + +describe('runManagedHookInstallers', () => { + beforeEach(() => { + trackMock.mockReset() + vi.spyOn(console, 'error').mockImplementation(() => {}) + }) + afterEach(() => { + vi.restoreAllMocks() + }) + + it('runs every installer when none throw and never calls track', () => { + const claude = vi.fn() + const codex = vi.fn() + runManagedHookInstallers([ + ['claude', claude], + ['codex', codex] + ]) + expect(claude).toHaveBeenCalledTimes(1) + expect(codex).toHaveBeenCalledTimes(1) + expect(trackMock).not.toHaveBeenCalled() + }) + + it('fires agent_hook_install_failed with the correct agent label when an installer throws', () => { + runManagedHookInstallers([ + [ + 'codex', + () => { + throw new Error('codex config malformed') + } + ] + ]) + + expect(trackMock).toHaveBeenCalledTimes(1) + expect(trackMock).toHaveBeenCalledWith('agent_hook_install_failed', { + agent: 'codex', + error_message: 'codex config malformed' + }) + }) + + it('continues running later installers after an earlier one throws (fail-open)', () => { + const codex = vi.fn() + const gemini = vi.fn() + runManagedHookInstallers([ + [ + 'claude', + () => { + throw new Error('claude failed') + } + ], + ['codex', codex], + ['gemini', gemini] + ]) + expect(codex).toHaveBeenCalledTimes(1) + expect(gemini).toHaveBeenCalledTimes(1) + expect(trackMock).toHaveBeenCalledTimes(1) + expect(trackMock).toHaveBeenCalledWith( + 'agent_hook_install_failed', + expect.objectContaining({ agent: 'claude' }) + ) + }) + + it('truncates error_message to 200 chars', () => { + const longMessage = 'x'.repeat(500) + runManagedHookInstallers([ + [ + 'gemini', + () => { + throw new Error(longMessage) + } + ] + ]) + expect(trackMock).toHaveBeenCalledTimes(1) + const [, props] = trackMock.mock.calls[0] as [string, { error_message: string }] + expect(props.error_message.length).toBe(200) + }) + + it('handles non-Error throws', () => { + runManagedHookInstallers([ + [ + 'cursor', + () => { + throw 'cursor string failure' + } + ] + ]) + expect(trackMock).toHaveBeenCalledWith('agent_hook_install_failed', { + agent: 'cursor', + error_message: 'cursor string failure' + }) + }) + + it('serializes thrown objects through JSON.stringify', () => { + runManagedHookInstallers([ + [ + 'cursor', + () => { + throw { code: 'EACCES', path: '/tmp' } + } + ] + ]) + expect(trackMock).toHaveBeenCalledTimes(1) + expect(trackMock).toHaveBeenCalledWith('agent_hook_install_failed', { + agent: 'cursor', + error_message: '{"code":"EACCES","path":"/tmp"}' + }) + }) + + it('does not throw when an installer throws undefined (regression for JSON.stringify undefined return)', () => { + expect(() => + runManagedHookInstallers([ + [ + 'cursor', + () => { + throw undefined + } + ] + ]) + ).not.toThrow() + expect(trackMock).toHaveBeenCalledTimes(1) + const [eventName, props] = trackMock.mock.calls[0] as [string, { error_message: string }] + expect(eventName).toBe('agent_hook_install_failed') + expect(typeof props.error_message).toBe('string') + }) + + it('continues running later installers when track itself throws (telemetry must not break fail-open)', () => { + const codex = vi.fn() + trackMock.mockImplementationOnce(() => { + throw new Error('telemetry blew up') + }) + expect(() => + runManagedHookInstallers([ + [ + 'claude', + () => { + throw new Error('claude failed') + } + ], + ['codex', codex] + ]) + ).not.toThrow() + expect(codex).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/main/agent-hooks/install-telemetry.ts b/src/main/agent-hooks/install-telemetry.ts new file mode 100644 index 00000000000..658872f7039 --- /dev/null +++ b/src/main/agent-hooks/install-telemetry.ts @@ -0,0 +1,53 @@ +// Per-agent managed-hook installer with fail-open semantics and PostHog +// attribution. Lifted out of `src/main/index.ts` so the loop is unit-testable +// without standing up the full Electron startup graph — the catch site needs +// the agent label to fire `agent_hook_install_failed`, and the previous +// closure-style loop lost it. + +import type { HookInstallAgent } from '../../shared/telemetry-events' +import { track } from '../telemetry/client' + +// Why: install errors are about config-file shape (malformed JSON, ACL +// denial), not user content — but messages can include paths or stack +// fragments. The 200-char cap matches `agentHookInstallFailedSchema.error_message` +// in `src/shared/telemetry-events.ts`; the validator drops overlength values, +// so truncation must happen here at the call site. +const ERROR_MESSAGE_MAX_LEN = 200 + +export type ManagedHookInstaller = readonly [HookInstallAgent, () => void] + +function describeError(error: unknown): string { + if (error instanceof Error) { + return error.message + } + if (typeof error === 'string') { + return error + } + try { + const json = JSON.stringify(error) + return typeof json === 'string' ? json : String(error) + } catch { + return String(error) + } +} + +export function runManagedHookInstallers(installers: readonly ManagedHookInstaller[]): void { + for (const [agent, install] of installers) { + try { + install() + } catch (error) { + console.error(`[agent-hooks] Failed to install ${agent} managed hooks:`, error) + // Why: telemetry must not break fail-open. A throw inside `track` (e.g. + // a corrupted settings store the resolveConsent path reads from) would + // otherwise abort the for-loop and skip later agents' installers. + try { + track('agent_hook_install_failed', { + agent, + error_message: describeError(error).slice(0, ERROR_MESSAGE_MAX_LEN) + }) + } catch (telemetryError) { + console.error('[agent-hooks] Failed to record install-failure telemetry:', telemetryError) + } + } + } +} diff --git a/src/main/index.ts b/src/main/index.ts index a2cad5f29f4..e9820c997e6 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -16,6 +16,7 @@ import { closeAllWatchers } from './ipc/filesystem-watcher' import { registerCoreHandlers } from './ipc/register-core-handlers' import { registerMobileHandlers } from './ipc/mobile' import { initTelemetry, shutdownTelemetry, trackAppOpenedOnce } from './telemetry/client' +import { runManagedHookInstallers } from './agent-hooks/install-telemetry' import { initCohortClassifier } from './telemetry/cohort-classifier' import { initOnboardingCohortClassifier } from './telemetry/onboarding-cohort-classifier' import { resolveConsent } from './telemetry/consent' @@ -488,22 +489,14 @@ app.whenReady().then(async () => { // Why: managed hook installation mutates user-global agent config. Each // installer runs inside its own try/catch so a malformed local config // (e.g. corrupted ~/.claude/settings.json) cannot brick Orca startup. - for (const installManagedHooks of [ - () => claudeHookService.install(), - () => codexHookService.install(), - () => geminiHookService.install() - ]) { - try { - installManagedHooks() - } catch (error) { - console.error('[agent-hooks] Failed to install managed hooks:', error) - } - } - try { - cursorHookService.install() - } catch (error) { - console.error('[agent-hooks] Failed to install Cursor managed hooks:', error) - } + // The agent label travels with each installer so the catch can attribute + // the failure in the `agent_hook_install_failed` telemetry event. + runManagedHookInstallers([ + ['claude', () => claudeHookService.install()], + ['codex', () => codexHookService.install()], + ['gemini', () => geminiHookService.install()], + ['cursor', () => cursorHookService.install()] + ]) registerAppMenu({ onCheckForUpdates: (options) => checkForUpdatesFromMenu(options), diff --git a/src/shared/agent-hook-types.ts b/src/shared/agent-hook-types.ts index 2b588d02a99..aa569b43002 100644 --- a/src/shared/agent-hook-types.ts +++ b/src/shared/agent-hook-types.ts @@ -4,7 +4,8 @@ // gemini/cursor/hook-service.ts). Lives in `shared/` to keep a single // source of truth for the version string and status contract. -export type AgentHookTarget = 'claude' | 'codex' | 'gemini' | 'cursor' +export const AGENT_HOOK_TARGETS = ['claude', 'codex', 'gemini', 'cursor'] as const +export type AgentHookTarget = (typeof AGENT_HOOK_TARGETS)[number] export type AgentHookInstallState = 'installed' | 'not_installed' | 'partial' | 'error' diff --git a/src/shared/telemetry-events.ts b/src/shared/telemetry-events.ts index 3de78556ecc..66e2369f8f5 100644 --- a/src/shared/telemetry-events.ts +++ b/src/shared/telemetry-events.ts @@ -15,6 +15,7 @@ import { z } from 'zod' +import { AGENT_HOOK_TARGETS } from './agent-hook-types' import { ONBOARDING_FINAL_STEP } from './constants' import type { DiscoveryStatusEmitted, GlobalSettings, OnboardingChecklistState } from './types' @@ -250,6 +251,27 @@ const workspaceCreateFailedSchema = z }) .strict() +// Managed-hook installer per-agent label. Distinct from `AGENT_KIND_VALUES`: +// hook installation only targets these four agents and the labels here match +// the `*HookService.install()` call sites in `src/main/index.ts`. `claude` +// (not `claude-code`) is intentional — the failure is about Claude Code's +// `~/.claude/settings.json`, not the broader product taxonomy. Sourced from +// `AGENT_HOOK_TARGETS` so the wire enum and the IPC `AgentHookTarget` type +// cannot drift if a fifth hook-install agent is added. +export const hookInstallAgentSchema = z.enum(AGENT_HOOK_TARGETS) +export type HookInstallAgent = z.infer + +// Why: install failures are config-file-shape errors (malformed JSON, missing +// keys, ACL denials on `~/.claude` etc.) — not user content. The 200-char +// cap is the truncation contract; callers must truncate before calling +// `track`, and the validator will drop overlength strings via `.max(200)`. +const agentHookInstallFailedSchema = z + .object({ + agent: hookInstallAgentSchema, + error_message: z.string().max(200) + }) + .strict() + // ── Onboarding ────────────────────────────────────────────────────────── // // Closed enums only — no raw paths, repo names, clone URLs, or error @@ -461,6 +483,7 @@ export const eventSchemas = { agent_started: agentStartedSchema, agent_error: agentErrorSchema, + agent_hook_install_failed: agentHookInstallFailedSchema, settings_changed: settingsChangedSchema, @@ -528,12 +551,11 @@ type _CohortExtendedRoster = type _DerivedCohortExtendedEvents = { [N in EventName]: 'nth_repo_added' extends keyof EventMap[N] ? N : never }[EventName] -type _CohortExtendedRosterSync = - _CohortExtendedRoster extends _DerivedCohortExtendedEvents - ? _DerivedCohortExtendedEvents extends _CohortExtendedRoster - ? true - : never +type _CohortExtendedRosterSync = _CohortExtendedRoster extends _DerivedCohortExtendedEvents + ? _DerivedCohortExtendedEvents extends _CohortExtendedRoster + ? true : never + : never const _cohortExtendedRosterSyncCheck: _CohortExtendedRosterSync = true void _cohortExtendedRosterSyncCheck @@ -577,12 +599,11 @@ type _OnboardingCohortRoster = type _DerivedOnboardingCohortEvents = { [N in EventName]: 'cohort' extends keyof EventMap[N] ? N : never }[EventName] -type _OnboardingCohortRosterSync = - _OnboardingCohortRoster extends _DerivedOnboardingCohortEvents - ? _DerivedOnboardingCohortEvents extends _OnboardingCohortRoster - ? true - : never +type _OnboardingCohortRosterSync = _OnboardingCohortRoster extends _DerivedOnboardingCohortEvents + ? _DerivedOnboardingCohortEvents extends _OnboardingCohortRoster + ? true : never + : never const _onboardingCohortRosterSyncCheck: _OnboardingCohortRosterSync = true void _onboardingCohortRosterSyncCheck