diff --git a/00-review-context.md b/00-review-context.md new file mode 100644 index 00000000000..d37e848ba54 --- /dev/null +++ b/00-review-context.md @@ -0,0 +1,75 @@ +# Review Context + +## Branch Info + +- Base: origin/main (merge-base 148f33c47062562d0678ec8972ed20edf08b1ec9) +- Current: brennanb2025/orca-cli-telemetry + +## Changed Files Summary + +- M config/tsconfig.cli.json +- M src/cli/dispatch.ts +- A src/cli/telemetry.test.ts +- A src/cli/telemetry.ts +- M src/main/runtime/rpc/methods/index.ts +- A src/main/runtime/rpc/methods/telemetry.test.ts +- A src/main/runtime/rpc/methods/telemetry.ts +- M src/shared/telemetry-events.test.ts +- M src/shared/telemetry-events.ts + +## Changed Line Ranges (PR Scope) + +| File | Changed Lines | +| --------------------------------------------- | ------------------------------ | +| config/tsconfig.cli.json | 3-8 | +| src/cli/dispatch.ts | 3, 62-74 | +| src/cli/telemetry.test.ts | 1-181 (new file) | +| src/cli/telemetry.ts | 1-344 (new file) | +| src/main/runtime/rpc/methods/index.ts | 11, 25-26 | +| src/main/runtime/rpc/methods/telemetry.test.ts| 1-131 (new file) | +| src/main/runtime/rpc/methods/telemetry.ts | 1-82 (new file) | +| src/shared/telemetry-events.test.ts | 10-11, 15, 194-272 | +| src/shared/telemetry-events.ts | 113-133, 211-220, 246-260 | + +## Review Standards Reference + +- Follow /review-code standards +- Focus on: correctness, security, performance, maintainability +- Priority levels: Critical > High > Medium > Low + +## File Categories + +### Electron/Main +- src/main/runtime/rpc/methods/index.ts +- src/main/runtime/rpc/methods/telemetry.ts +- src/main/runtime/rpc/methods/telemetry.test.ts + +### Config/Build +- config/tsconfig.cli.json + +### Utility/Common +- src/cli/dispatch.ts +- src/cli/telemetry.ts +- src/cli/telemetry.test.ts +- src/shared/telemetry-events.ts +- src/shared/telemetry-events.test.ts + +## Skipped Issues (Do Not Re-validate) + +- [src/main/runtime/rpc/methods/telemetry.ts:44] | Low | Cosmetic — per-event `.strict()` schemas catch non-object props at the same boundary | params.props z.unknown() doesn't enforce object shape at params layer +- [src/main/runtime/rpc/methods/telemetry.ts:42] | Low | Stylistic — sibling RPC files use the same transform-then-pipe pattern | "Missing event name" error message is misleading for non-string name values +- [config/tsconfig.cli.json:7] | Low | No-harm pre-emptive include; potentially needed for future telemetry shared-code work | `../src/types/**/*` include not currently referenced +- [src/cli/telemetry.ts:283-344] | Low | Significant test infra (tmp socket server) for code with extensive load-bearing comments | defaultSender socket-lifecycle has no integration test +- [src/cli/telemetry.ts:9-10] | Low | Documented design choice — "best-effort, fire-and-forget" per design doc | unref + likely `process.exit` means short commands drop events +- [src/cli/telemetry.ts:220-227] | Low | Documented design choice — "first-write-wins" per design doc | first-failure-wins biases dashboard +- [src/main/runtime/rpc/methods/telemetry.ts:18-19] | Low | Architectural refactor — out of scope for this PR | direct import of `client` instead of routing through `OrcaRuntimeService` +- [src/main/runtime/rpc/methods/telemetry.ts:58-60] | Low | Threat-model says no wire leak; main-side debug log is judgment call | no observability hook for rejected CLI event names +- [src/cli/telemetry.ts:186-204] | Low | Stylistic — `_*ForTests` exports are an existing project pattern | test seams in production module +- [src/cli/telemetry.ts:294-302] | Low | Belt-and-braces is documented; structure works correctly | createConnection sync-throw try/catch awkwardness +- [src/cli/dispatch.ts:3] | Low | Stylistic only | `./telemetry` import not grouped with handler imports + +## Iteration State + +Current iteration: 1 +Last completed phase: Validation +Files fixed this iteration: [] diff --git a/config/tsconfig.cli.json b/config/tsconfig.cli.json index dbcdaa729a4..9b7051ce20a 100644 --- a/config/tsconfig.cli.json +++ b/config/tsconfig.cli.json @@ -1,6 +1,11 @@ { "extends": "@electron-toolkit/tsconfig/tsconfig.node.json", - "include": ["../src/cli/**/*", "../src/shared/**/*", "../src/main/runtime/runtime-metadata.ts"], + "include": [ + "../src/cli/**/*", + "../src/shared/**/*", + "../src/main/runtime/runtime-metadata.ts", + "../src/types/**/*" + ], "compilerOptions": { "composite": true, "module": "CommonJS", diff --git a/src/cli/dispatch.ts b/src/cli/dispatch.ts index 6dd41e3a554..19351b3cf70 100644 --- a/src/cli/dispatch.ts +++ b/src/cli/dispatch.ts @@ -1,5 +1,6 @@ import type { RuntimeClient } from './runtime-client' import { RuntimeClientError } from './runtime-client' +import { recordCliFeatureUsed } from './telemetry' import { CORE_HANDLERS } from './handlers/core' import { REPO_HANDLERS } from './handlers/repo' import { WORKTREE_HANDLERS } from './handlers/worktree' @@ -58,5 +59,17 @@ export async function dispatch(commandPath: string[], ctx: HandlerContext): Prom if (!handler) { throw new RuntimeClientError('invalid_argument', `Unknown command: ${commandPath.join(' ')}`) } - await handler(ctx) + // Why: telemetry is fire-at-action — emit on the first call in a feature + // group with that call's exit_status, then never again for this process. + // The recording call is non-blocking and silent on failure, so it cannot + // observably affect the command. We record `failure` if the handler + // throws and `success` otherwise, then re-throw to preserve the original + // error path. No buffering, no process-exit flush. + try { + await handler(ctx) + } catch (error) { + recordCliFeatureUsed(commandPath, 'failure') + throw error + } + recordCliFeatureUsed(commandPath, 'success') } diff --git a/src/cli/telemetry.test.ts b/src/cli/telemetry.test.ts new file mode 100644 index 00000000000..c426cc9e8f5 --- /dev/null +++ b/src/cli/telemetry.test.ts @@ -0,0 +1,181 @@ +// Dispatch-side dedupe and feature-group-mapping coverage. The wire shape +// (what props flow to the runtime RPC method) is the contract we're +// pinning here — `recordCliFeatureUsed` is the boundary the rest of the +// CLI talks to, and a regression in the dedupe logic would either flood +// PostHog (no dedupe) or starve adoption signal (over-dedupe). +// +// Tests intentionally substitute a fake sender via `_setTelemetrySenderForTests` +// rather than letting a socket open against a runtime that may or may not +// be running — the fire-and-forget transport's design (unref, swallow +// errors) makes it impossible to observe failures otherwise. + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + _getEmittedGroupsForTests, + _resetEmittedGroupsForTests, + _setTelemetrySenderForTests, + recordCliFeatureUsed, + resolveFeatureGroup +} from './telemetry' +import type { RuntimeMetadata } from '../shared/runtime-bootstrap' +import type * as MetadataModule from './runtime/metadata' + +// Stand in for a metadata file on disk. The sender shim never reads from +// disk in tests, but `tryReadMetadata` does — see the `vi.mock` below. +const FAKE_METADATA: RuntimeMetadata = { + runtimeId: 'test-runtime', + pid: 0, + authToken: 'test-token', + startedAt: 0, + transports: [{ kind: 'unix', endpoint: '/tmp/orca-test.sock' }] +} + +vi.mock('./runtime/metadata', async (importOriginal) => { + const actual = (await importOriginal()) as typeof MetadataModule + return { + ...actual, + tryReadMetadata: () => FAKE_METADATA, + getDefaultUserDataPath: () => '/tmp/orca-test-userdata' + } +}) + +type SenderCall = { + endpoint: string + body: { method: string; params: unknown } +} + +describe('resolveFeatureGroup', () => { + it('maps representative commands to their declared groups', () => { + // One per group — protects the design-doc grouping table from drift. + expect(resolveFeatureGroup(['snapshot'])).toBe('browser_observation') + expect(resolveFeatureGroup(['click'])).toBe('browser_interaction') + expect(resolveFeatureGroup(['goto'])).toBe('browser_navigation') + expect(resolveFeatureGroup(['cookie', 'set'])).toBe('browser_config') + expect(resolveFeatureGroup(['terminal', 'create'])).toBe('terminal_orchestration') + expect(resolveFeatureGroup(['terminal', 'send'])).toBe('terminal_io') + expect(resolveFeatureGroup(['worktree', 'create'])).toBe('worktree_orchestration') + expect(resolveFeatureGroup(['orchestration', 'run'])).toBe('orchestration_coordinator') + expect(resolveFeatureGroup(['orchestration', 'check'])).toBe('orchestration_messaging') + expect(resolveFeatureGroup(['status'])).toBe('discovery') + }) + + it('returns null for unmapped commands (soft no-op)', () => { + expect(resolveFeatureGroup(['definitely-not-a-real-command'])).toBeNull() + expect(resolveFeatureGroup([])).toBeNull() + }) +}) + +describe('recordCliFeatureUsed', () => { + let senderCalls: SenderCall[] + + beforeEach(() => { + _resetEmittedGroupsForTests() + senderCalls = [] + _setTelemetrySenderForTests(async (_metadata, endpoint, body) => { + senderCalls.push({ endpoint, body }) + }) + }) + + afterEach(() => { + _setTelemetrySenderForTests(null) + _resetEmittedGroupsForTests() + }) + + // Drains microtasks so the fire-and-forget `void send(...).catch(...)` + // resolves before assertions. `recordCliFeatureUsed` deliberately does + // not return a Promise so dispatch never blocks on telemetry. + async function flush(): Promise { + await new Promise((resolve) => setImmediate(resolve)) + } + + it('emits exactly one event for the first call in a group', async () => { + recordCliFeatureUsed(['snapshot'], 'success') + await flush() + expect(senderCalls).toHaveLength(1) + expect(senderCalls[0].body.method).toBe('telemetry.captureCliEvent') + expect(senderCalls[0].body.params).toEqual({ + name: 'cli_feature_used', + props: { feature_group: 'browser_observation', exit_status: 'success' } + }) + }) + + // The cap-saving design: a single agent loop that calls `snapshot` 200 + // times must produce exactly one event. If this regresses, the PostHog + // bill regresses with it. + it('dedupes subsequent calls in the same group within one process', async () => { + for (let i = 0; i < 50; i++) { + recordCliFeatureUsed(['snapshot'], 'success') + } + await flush() + expect(senderCalls).toHaveLength(1) + }) + + // First-write-wins on exit_status: a later failure in the same group + // does not retransmit. Adopting "any failure flips the bit" would mean + // buffering and a process-exit flush, which the design explicitly + // rejected. + it('first-write-wins on exit_status (later failure does not re-emit)', async () => { + recordCliFeatureUsed(['snapshot'], 'success') + recordCliFeatureUsed(['screenshot'], 'failure') // same group: browser_observation + await flush() + expect(senderCalls).toHaveLength(1) + expect(senderCalls[0].body.params).toEqual({ + name: 'cli_feature_used', + props: { feature_group: 'browser_observation', exit_status: 'success' } + }) + }) + + it('emits separate events for distinct groups', async () => { + recordCliFeatureUsed(['snapshot'], 'success') + recordCliFeatureUsed(['click'], 'success') + recordCliFeatureUsed(['goto'], 'failure') + await flush() + expect(senderCalls).toHaveLength(3) + const groups = senderCalls.map((c) => { + const params = c.body.params as { props: { feature_group: string } } + return params.props.feature_group + }) + expect(groups.sort()).toEqual(['browser_interaction', 'browser_navigation', 'browser_observation']) + }) + + it('records exit_status: failure when the first call fails', async () => { + recordCliFeatureUsed(['snapshot'], 'failure') + await flush() + expect(senderCalls).toHaveLength(1) + expect(senderCalls[0].body.params).toEqual({ + name: 'cli_feature_used', + props: { feature_group: 'browser_observation', exit_status: 'failure' } + }) + }) + + it('is a no-op for unmapped commands and does not consume the dedupe slot', async () => { + recordCliFeatureUsed(['unknown-command'], 'success') + await flush() + expect(senderCalls).toHaveLength(0) + expect(_getEmittedGroupsForTests().size).toBe(0) + }) + + // Belt-and-braces: the dedupe set must be consulted BEFORE the send is + // dispatched. If a sender failure could un-mark the group, retries on a + // broken socket would amplify into one connect attempt per command — + // exactly the "telemetry never affects user-visible behavior" violation + // the design rules out. + it('does not retry on sender failure', async () => { + const failingSender = vi.fn().mockRejectedValue(new Error('connection refused')) + _setTelemetrySenderForTests(failingSender) + + recordCliFeatureUsed(['snapshot'], 'success') + recordCliFeatureUsed(['snapshot'], 'success') + recordCliFeatureUsed(['screenshot'], 'success') + await flush() + expect(failingSender).toHaveBeenCalledTimes(1) + }) + + // The function returns synchronously (`void`) — the design forbids + // dispatch from awaiting telemetry so a stuck socket can never delay + // the user-visible command. + it('returns synchronously without awaiting the send', () => { + const result = recordCliFeatureUsed(['snapshot'], 'success') + expect(result).toBeUndefined() + }) +}) diff --git a/src/cli/telemetry.ts b/src/cli/telemetry.ts new file mode 100644 index 00000000000..cc5613087e1 --- /dev/null +++ b/src/cli/telemetry.ts @@ -0,0 +1,344 @@ +// Dispatch-side CLI telemetry. One job: emit `cli_feature_used` at most once +// per (feature_group, CLI process) and never observably affect command +// behavior. See docs/cli-telemetry-design.md for the full design — the +// constraints below are load-bearing, not stylistic preferences. +// +// Constraints (each maps to a comment further down): +// - never write to stdout/stderr (preserves --json purity, no +// UnhandledPromiseRejectionWarning leaks) +// - never block process exit (unref the socket + any pending timeout, no +// `await` from the call site) +// - never throw out of the dispatch path on telemetry failure +// - dedupe per process via an in-memory Set +// +// Communication path: CLI → runtime RPC `telemetry.captureCliEvent` → main +// `track()`. If the app isn't running, metadata is missing, or the auth +// token is stale, we silently swallow — the CLI is not the consent +// authority and not the place to log telemetry health. + +import { createConnection } from 'net' +import { randomUUID } from 'crypto' +import { findTransport, type RuntimeMetadata } from '../shared/runtime-bootstrap' +import { getDefaultUserDataPath, tryReadMetadata } from './runtime/metadata' + +import type { CliFeatureGroup } from '../shared/telemetry-events' + +// Map from `commandPath` (the same `commandPath.join(' ')` key dispatch uses) +// to a feature group. Kept exhaustive for the commands shipped today; +// unmapped commands return `null` and emit no event. Adding a new command +// without a mapping is a soft no-op — telemetry will silently miss it +// rather than mis-bucket — which is the right tradeoff for a coarse +// adoption signal. +// +// Groupings mirror docs/cli-telemetry-design.md §Events. If you change the +// grouping for a command, change it there too — dashboards built on the +// original grouping will silently mix old + new rows. +const COMMAND_TO_FEATURE_GROUP: Record = { + // worktree_orchestration: lifecycle of worktrees and base-ref repo state + 'worktree create': 'worktree_orchestration', + 'worktree rm': 'worktree_orchestration', + 'worktree set': 'worktree_orchestration', + 'repo add': 'worktree_orchestration', + 'repo set-base-ref': 'worktree_orchestration', + + // terminal_orchestration: terminal lifecycle + focus + 'terminal create': 'terminal_orchestration', + 'terminal split': 'terminal_orchestration', + 'terminal close': 'terminal_orchestration', + 'terminal stop': 'terminal_orchestration', + 'terminal rename': 'terminal_orchestration', + 'terminal switch': 'terminal_orchestration', + 'terminal focus': 'terminal_orchestration', + + // terminal_io: data flow into/out of an existing terminal + 'terminal read': 'terminal_io', + 'terminal send': 'terminal_io', + 'terminal wait': 'terminal_io', + + // browser_navigation: things that change "where" the page is + goto: 'browser_navigation', + back: 'browser_navigation', + forward: 'browser_navigation', + reload: 'browser_navigation', + 'tab create': 'browser_navigation', + 'tab close': 'browser_navigation', + 'tab switch': 'browser_navigation', + + // browser_observation: read-only inspection + snapshot: 'browser_observation', + screenshot: 'browser_observation', + 'full-screenshot': 'browser_observation', + eval: 'browser_observation', + wait: 'browser_observation', + pdf: 'browser_observation', + get: 'browser_observation', + is: 'browser_observation', + find: 'browser_observation', + + // browser_interaction: mutate the page + click: 'browser_interaction', + dblclick: 'browser_interaction', + fill: 'browser_interaction', + type: 'browser_interaction', + select: 'browser_interaction', + check: 'browser_interaction', + uncheck: 'browser_interaction', + focus: 'browser_interaction', + clear: 'browser_interaction', + 'select-all': 'browser_interaction', + keypress: 'browser_interaction', + hover: 'browser_interaction', + drag: 'browser_interaction', + upload: 'browser_interaction', + scroll: 'browser_interaction', + scrollintoview: 'browser_interaction', + inserttext: 'browser_interaction', + download: 'browser_interaction', + highlight: 'browser_interaction', + 'mouse move': 'browser_interaction', + 'mouse down': 'browser_interaction', + 'mouse up': 'browser_interaction', + 'mouse wheel': 'browser_interaction', + + // browser_config: viewport/device/cookies/storage/intercept/capture + 'set device': 'browser_config', + 'set offline': 'browser_config', + 'set headers': 'browser_config', + 'set credentials': 'browser_config', + 'set media': 'browser_config', + 'clipboard read': 'browser_config', + 'clipboard write': 'browser_config', + 'dialog accept': 'browser_config', + 'dialog dismiss': 'browser_config', + 'cookie get': 'browser_config', + 'cookie set': 'browser_config', + 'cookie delete': 'browser_config', + 'storage local get': 'browser_config', + 'storage local set': 'browser_config', + 'storage local clear': 'browser_config', + 'storage session get': 'browser_config', + 'storage session set': 'browser_config', + 'storage session clear': 'browser_config', + 'intercept enable': 'browser_config', + 'intercept disable': 'browser_config', + 'intercept list': 'browser_config', + 'capture start': 'browser_config', + 'capture stop': 'browser_config', + 'tab profile list': 'browser_config', + 'tab profile create': 'browser_config', + 'tab profile delete': 'browser_config', + 'tab profile set': 'browser_config', + 'tab profile show': 'browser_config', + 'tab profile use-default': 'browser_config', + 'tab profile clone': 'browser_config', + + // orchestration_coordinator: agents driving other agents + 'orchestration run': 'orchestration_coordinator', + 'orchestration run-stop': 'orchestration_coordinator', + 'orchestration task-create': 'orchestration_coordinator', + 'orchestration dispatch': 'orchestration_coordinator', + 'orchestration ask': 'orchestration_coordinator', + 'orchestration gate-create': 'orchestration_coordinator', + 'orchestration gate-resolve': 'orchestration_coordinator', + 'orchestration reset': 'orchestration_coordinator', + + // orchestration_messaging: inbox and message-mutation surfaces + 'orchestration check': 'orchestration_messaging', + 'orchestration inbox': 'orchestration_messaging', + 'orchestration task-list': 'orchestration_messaging', + 'orchestration task-update': 'orchestration_messaging', + 'orchestration send': 'orchestration_messaging', + 'orchestration reply': 'orchestration_messaging', + 'orchestration dispatch-show': 'orchestration_messaging', + 'orchestration gate-list': 'orchestration_messaging', + + // discovery: status / list / show / current / search + status: 'discovery', + open: 'discovery', + 'worktree list': 'discovery', + 'worktree ps': 'discovery', + 'worktree show': 'discovery', + 'worktree current': 'discovery', + 'terminal list': 'discovery', + 'terminal show': 'discovery', + 'tab list': 'discovery', + 'tab show': 'discovery', + 'tab current': 'discovery', + 'repo list': 'discovery', + 'repo show': 'discovery', + 'repo search-refs': 'discovery' +} + +export function resolveFeatureGroup(commandPath: readonly string[]): CliFeatureGroup | null { + return COMMAND_TO_FEATURE_GROUP[commandPath.join(' ')] ?? null +} + +// Per-process dedupe set. The "first-write-wins on exit_status" rule lives +// here: once a group is in this Set, any later call in the same process — +// success or failure — never re-emits. See docs/cli-telemetry-design.md +// §Events for why. +const emittedGroups = new Set() + +// Test seam for the network layer. Production uses `defaultSender` below. +// Tests substitute a fake to assert what would be sent without opening a +// socket. Kept separate from `_resetEmittedGroupsForTests` so a test can +// reset dedupe state without rewiring the sender. +type Sender = ( + metadata: RuntimeMetadata, + endpoint: string, + body: { method: string; params: unknown } +) => Promise + +let senderOverride: Sender | null = null + +export function _setTelemetrySenderForTests(sender: Sender | null): void { + senderOverride = sender +} + +export function _resetEmittedGroupsForTests(): void { + emittedGroups.clear() +} + +export function _getEmittedGroupsForTests(): ReadonlySet { + return emittedGroups +} + +/** + * Record that `commandPath` ran with `exitStatus`, and emit a + * `cli_feature_used` event if this is the first call in the group for this + * process. Fire-and-forget — this function never throws, never blocks, and + * never logs. + */ +export function recordCliFeatureUsed( + commandPath: readonly string[], + exitStatus: 'success' | 'failure' +): void { + const group = resolveFeatureGroup(commandPath) + if (group === null) { + return + } + if (emittedGroups.has(group)) { + return + } + // Mark BEFORE attempting the send. If the send fails, we still treat the + // group as "reported for this process" — retrying on every subsequent call + // would amplify a broken-RPC scenario into one failed connect per command, + // which is the opposite of "telemetry never affects user-visible behavior." + emittedGroups.add(group) + + // `void` + `.catch` together cover the two ways a stray rejection could + // become user-visible: an `await`-less promise without a `.catch` would + // surface as `UnhandledPromiseRejectionWarning` to stderr, breaking + // `--json` purity. Belt-and-braces because the implementation is supposed + // to swallow internally, but a future refactor mustn't be able to leak. + void sendCliFeatureUsed(group, exitStatus).catch(() => { + // Swallow. This is the entire failure-handling story. + }) +} + +async function sendCliFeatureUsed( + group: CliFeatureGroup, + exitStatus: 'success' | 'failure' +): Promise { + // Resolve userDataPath at send time so `ORCA_USER_DATA_PATH` is honored + // identically to a regular RPC call. + let userDataPath: string + try { + userDataPath = getDefaultUserDataPath() + } catch { + return + } + const metadata = tryReadMetadata(userDataPath) + if (!metadata) { + return + } + const transport = findTransport(metadata, 'unix', 'named-pipe') + if (!transport) { + return + } + const send = senderOverride ?? defaultSender + await send(metadata, transport.endpoint, { + method: 'telemetry.captureCliEvent', + params: { + name: 'cli_feature_used', + props: { feature_group: group, exit_status: exitStatus } + } + }) +} + +// A purpose-built minimal RPC sender for telemetry. It diverges from +// `runtime/transport.ts` in two important ways: +// 1. The socket is `unref()`ed so a still-in-flight send never holds the +// Node process open after the user-visible command has finished. A +// short-lived `orca worktree list` exits as soon as `printResult` +// returns, not when telemetry round-trips. +// 2. The timeout is `unref()`ed for the same reason. Without this, a +// pending timer would keep the event loop alive even after the socket +// is unref'd. +// We do not parse / validate the response — the CLI doesn't care whether +// main accepted, dropped at the consent gate, or rejected at the validator. +// The contract is: post the event and walk away. +const TELEMETRY_RPC_TIMEOUT_MS = 2_000 + +const defaultSender: Sender = async (metadata, endpoint, body) => { + await new Promise((resolve) => { + let settled = false + const settle = (): void => { + if (settled) { + return + } + settled = true + resolve() + } + + let socket: ReturnType + try { + socket = createConnection(endpoint) + } catch { + // `createConnection` typically does not throw synchronously, but + // belt-and-braces — a malformed endpoint string would surface here. + settle() + return + } + + socket.unref() + + // Discard whatever the server sends back. We only care about the + // connect → write → end lifecycle for unrefing purposes; reading the + // response is incidental and any read errors are themselves a "swallow" + // case. + socket.on('error', settle) + socket.on('close', settle) + socket.on('end', settle) + // Drain incoming data so the kernel buffer doesn't sit around. + socket.resume() + + const timeout = setTimeout(() => { + socket.destroy() + settle() + }, TELEMETRY_RPC_TIMEOUT_MS) + timeout.unref() + + socket.on('connect', () => { + try { + socket.write( + `${JSON.stringify({ + id: randomUUID(), + authToken: metadata.authToken, + method: body.method, + params: body.params + })}\n`, + () => { + // Half-close: signals "no more requests on this socket" so main + // can release its handler slot promptly. The server's reply (if + // any) is what triggers `end`/`close` above and resolves us. + socket.end() + } + ) + } catch { + socket.destroy() + settle() + } + }) + }) +} diff --git a/src/main/runtime/rpc/methods/index.ts b/src/main/runtime/rpc/methods/index.ts index cf3fbf34f86..e01e3726bbe 100644 --- a/src/main/runtime/rpc/methods/index.ts +++ b/src/main/runtime/rpc/methods/index.ts @@ -8,6 +8,7 @@ import { BROWSER_EXTRA_METHODS } from './browser-extras' import { ORCHESTRATION_METHODS } from './orchestration' import { NOTIFICATION_METHODS } from './notifications' import { STATS_METHODS } from './stats' +import { TELEMETRY_METHODS } from './telemetry' // Why: a flat manifest keeps registration order explicit and provides one // grep-point for "what methods does the RPC server expose?" — useful when @@ -21,5 +22,6 @@ export const ALL_RPC_METHODS: readonly RpcAnyMethod[] = [ ...BROWSER_EXTRA_METHODS, ...ORCHESTRATION_METHODS, ...NOTIFICATION_METHODS, - ...STATS_METHODS + ...STATS_METHODS, + ...TELEMETRY_METHODS ] diff --git a/src/main/runtime/rpc/methods/telemetry.test.ts b/src/main/runtime/rpc/methods/telemetry.test.ts new file mode 100644 index 00000000000..6c847be3e67 --- /dev/null +++ b/src/main/runtime/rpc/methods/telemetry.test.ts @@ -0,0 +1,131 @@ +// Coverage for the CLI-facing telemetry RPC method. Two invariants the +// system depends on: +// 1. A CLI-originated payload that names a non-CLI event is dropped +// silently — the CLI must not be a privilege-escalation surface for +// main-owned events (`agent_started`, `telemetry_opted_in`, etc.). +// 2. A schema-invalid payload is dropped without throwing — the CLI +// gets back `{}` and never learns the request was rejected, so +// telemetry health cannot leak through the wire shape. + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { TELEMETRY_METHODS } from './telemetry' +import { buildRegistry, type RpcContext } from '../core' +import { OrcaRuntimeService } from '../../orca-runtime' + +const trackMock = vi.fn() +vi.mock('../../../telemetry/client', () => ({ + track: (name: string, props: unknown) => trackMock(name, props) +})) + +describe('telemetry RPC methods', () => { + let ctx: RpcContext + + beforeEach(() => { + trackMock.mockReset() + ctx = { runtime: new OrcaRuntimeService() } + }) + + afterEach(() => { + trackMock.mockReset() + }) + + function findMethod(name: string) { + const method = TELEMETRY_METHODS.find((m) => m.name === name) + if (!method) { + throw new Error(`Method not found: ${name}`) + } + return method + } + + async function call(name: string, params: Record) { + const method = findMethod(name) + const parsed = method.params ? method.params.parse(params) : undefined + return method.handler(parsed, ctx) + } + + it('registers telemetry.captureCliEvent', () => { + const registry = buildRegistry([...TELEMETRY_METHODS]) + expect(registry.has('telemetry.captureCliEvent')).toBe(true) + }) + + it('forwards a valid cli_feature_used payload to track()', async () => { + await call('telemetry.captureCliEvent', { + name: 'cli_feature_used', + props: { feature_group: 'browser_observation', exit_status: 'success' } + }) + expect(trackMock).toHaveBeenCalledTimes(1) + expect(trackMock).toHaveBeenCalledWith('cli_feature_used', { + feature_group: 'browser_observation', + exit_status: 'success' + }) + }) + + // The narrow at the boundary: a compromised or buggy CLI must not be + // able to emit main-owned events. Without this drop, the CLI's auth + // token would let it ship `agent_started` / `telemetry_opted_in` / + // anything else in the registry. + it('drops non-CLI event names without calling track()', async () => { + await call('telemetry.captureCliEvent', { + name: 'agent_started', + props: { agent_kind: 'claude-code', launch_source: 'sidebar', request_kind: 'new' } + }) + await call('telemetry.captureCliEvent', { + name: 'telemetry_opted_in', + props: { via: 'first_launch_banner' } + }) + await call('telemetry.captureCliEvent', { + name: 'app_opened', + props: {} + }) + expect(trackMock).not.toHaveBeenCalled() + }) + + it('drops schema-invalid props without calling track()', async () => { + // Unknown feature_group + await call('telemetry.captureCliEvent', { + name: 'cli_feature_used', + props: { feature_group: 'made_up_group', exit_status: 'success' } + }) + // Missing exit_status + await call('telemetry.captureCliEvent', { + name: 'cli_feature_used', + props: { feature_group: 'discovery' } + }) + // Extra key (would leak free-form content if forwarded) + await call('telemetry.captureCliEvent', { + name: 'cli_feature_used', + props: { + feature_group: 'browser_navigation', + exit_status: 'failure', + error_message: 'ENOENT' + } + }) + expect(trackMock).not.toHaveBeenCalled() + }) + + it('rejects an empty event name at the params boundary', () => { + const method = findMethod('telemetry.captureCliEvent') + expect(() => + method.params!.parse({ + name: '', + props: { feature_group: 'discovery', exit_status: 'success' } + }) + ).toThrow(/Missing event name/) + }) + + it('returns an empty object on success (not raw track() output)', async () => { + const result = await call('telemetry.captureCliEvent', { + name: 'cli_feature_used', + props: { feature_group: 'discovery', exit_status: 'success' } + }) + expect(result).toEqual({}) + }) + + it('returns an empty object on a rejected (non-CLI) event', async () => { + const result = await call('telemetry.captureCliEvent', { + name: 'agent_started', + props: { agent_kind: 'claude-code', launch_source: 'sidebar', request_kind: 'new' } + }) + expect(result).toEqual({}) + }) +}) diff --git a/src/main/runtime/rpc/methods/telemetry.ts b/src/main/runtime/rpc/methods/telemetry.ts new file mode 100644 index 00000000000..a8762294ede --- /dev/null +++ b/src/main/runtime/rpc/methods/telemetry.ts @@ -0,0 +1,82 @@ +// CLI-side telemetry funnels through this RPC method into main's `track()`. +// The CLI process never talks to PostHog directly — there is one write key, +// one consent gate, one validator, one wire identity (install_id, app_version, +// session_id, …) attached on the main side. This file is the narrow boundary +// where a CLI-originated event becomes a typed `track()` call. +// +// Threat model: the CLI is a separate process and the RPC socket is +// auth-token gated, but we still narrow the incoming `name` to a CLI subset +// (`isCliEventName`). A buggy or compromised CLI must not be able to spoof +// `agent_started` / `telemetry_opted_in` / etc. by posting them through this +// channel — those are main-owned events. +// +// Not in `src/main/ipc/telemetry.ts`: that file is renderer-only (Electron +// `ipcMain.handle()`). The CLI talks to main over the UNIX-socket / named- +// pipe runtime RPC dispatcher, not Electron IPC. + +import { z } from 'zod' +import { defineMethod, type RpcMethod } from '../core' +import { track } from '../../../telemetry/client' +import { + cliFeatureGroupSchema, + isCliEventName, + type CliEventName, + type EventProps +} from '../../../../shared/telemetry-events' + +// Per-CLI-event property schemas. Kept here (not in `telemetry-events.ts`) +// because they describe what the RPC method accepts on the wire — the +// authoritative event-shape validator inside `track()` re-checks these +// against `eventSchemas[name]`. This is the boundary type-narrow; that is +// the wire-contract enforcement. +const CliFeatureUsedProps = z + .object({ + feature_group: cliFeatureGroupSchema, + exit_status: z.enum(['success', 'failure']) + }) + .strict() + +const CaptureCliEventParams = z.object({ + name: z + .unknown() + .transform((value) => (typeof value === 'string' ? value : '')) + .pipe(z.string().min(1, 'Missing event name')), + props: z.unknown() +}) + +export const TELEMETRY_METHODS: RpcMethod[] = [ + defineMethod({ + name: 'telemetry.captureCliEvent', + params: CaptureCliEventParams, + handler: (params) => { + // Narrow `name` to the CLI subset before any further work. A non-CLI + // event name is silently dropped — the CLI must never learn whether + // a name was rejected (that would be a side-channel for probing the + // server's allow-list), and the validator inside `track()` would + // drop it anyway. Returning `{}` keeps the wire shape consistent + // with other `defineMethod` handlers. + if (!isCliEventName(params.name)) { + return {} + } + const name: CliEventName = params.name + // Per-event prop narrowing. Only `cli_feature_used` exists today. + // Adding a new CLI event means adding a branch here AND a schema in + // `eventSchemas`; the type system makes the omission a `tsc` error + // because `name` is the discriminated `CliEventName` union. + switch (name) { + case 'cli_feature_used': { + const parsed = CliFeatureUsedProps.safeParse(params.props) + if (!parsed.success) { + return {} + } + // The cast is the boundary handoff: `track()` re-validates + // against `eventSchemas.cli_feature_used` (the single source of + // truth) so this assignment is "I have already checked the + // shape, hand it to the central validator." + track(name, parsed.data satisfies EventProps<'cli_feature_used'>) + return {} + } + } + } + }) +] diff --git a/src/shared/telemetry-events.test.ts b/src/shared/telemetry-events.test.ts index 1a6f76a864a..4af6b001529 100644 --- a/src/shared/telemetry-events.test.ts +++ b/src/shared/telemetry-events.test.ts @@ -7,9 +7,12 @@ import { describe, expect, it } from 'vitest' import { AGENT_KIND_VALUES, agentKindSchema, + CLI_EVENT_NAMES, + CLI_FEATURE_GROUP_VALUES, commonPropsSchema, errorClassSchema, eventSchemas, + isCliEventName, SETTINGS_CHANGED_WHITELIST, settingsChangedKeySchema } from './telemetry-events' @@ -188,6 +191,85 @@ describe('commonPropsSchema', () => { }) }) +describe('cli_feature_used schema', () => { + it('round-trips a minimal valid payload', () => { + const parsed = eventSchemas.cli_feature_used.safeParse({ + feature_group: 'browser_observation', + exit_status: 'success' + }) + expect(parsed.success).toBe(true) + }) + + it('accepts every declared feature_group value', () => { + for (const group of CLI_FEATURE_GROUP_VALUES) { + const parsed = eventSchemas.cli_feature_used.safeParse({ + feature_group: group, + exit_status: 'success' + }) + expect(parsed.success).toBe(true) + } + }) + + it('rejects unknown feature_group values', () => { + const parsed = eventSchemas.cli_feature_used.safeParse({ + feature_group: 'made_up_group', + exit_status: 'success' + }) + expect(parsed.success).toBe(false) + }) + + it('rejects unknown exit_status values', () => { + const parsed = eventSchemas.cli_feature_used.safeParse({ + feature_group: 'discovery', + exit_status: 'partial' + }) + expect(parsed.success).toBe(false) + }) + + // The whole point of the wire-shape constraint: the CLI never transmits a + // raw command name, URL, error message, or path. `.strict()` enforces this + // at the schema level — any extra key drops the event. + it('rejects extra keys via .strict() (no command name leak, no error_message)', () => { + const withCommand = eventSchemas.cli_feature_used.safeParse({ + feature_group: 'browser_navigation', + exit_status: 'success', + command: 'goto' + }) + expect(withCommand.success).toBe(false) + + const withErrorMessage = eventSchemas.cli_feature_used.safeParse({ + feature_group: 'browser_navigation', + exit_status: 'failure', + error_message: 'ENOENT: no such file' + }) + expect(withErrorMessage.success).toBe(false) + }) +}) + +describe('isCliEventName', () => { + it('accepts every CLI_EVENT_NAMES entry', () => { + for (const name of CLI_EVENT_NAMES) { + expect(isCliEventName(name)).toBe(true) + } + }) + + // The narrow exists so a CLI process cannot spoof main-owned events + // (`agent_started`, `telemetry_opted_in`, etc.) via the runtime RPC + // method. If these ever pass, `methods/telemetry.ts` would be a + // privilege-escalation surface. + it('rejects main-owned event names', () => { + expect(isCliEventName('app_opened')).toBe(false) + expect(isCliEventName('agent_started')).toBe(false) + expect(isCliEventName('telemetry_opted_in')).toBe(false) + expect(isCliEventName('settings_changed')).toBe(false) + }) + + it('rejects unknown event names', () => { + expect(isCliEventName('made_up_event')).toBe(false) + expect(isCliEventName('')).toBe(false) + }) +}) + describe('exported enum schemas', () => { it('agentKindSchema accepts the known product IDs', () => { for (const kind of AGENT_KIND_VALUES) { diff --git a/src/shared/telemetry-events.ts b/src/shared/telemetry-events.ts index a12d8ec0f74..e8a74d5b229 100644 --- a/src/shared/telemetry-events.ts +++ b/src/shared/telemetry-events.ts @@ -110,6 +110,27 @@ export type RequestKind = z.infer export const optInViaSchema = z.enum(['first_launch_banner', 'settings']) export type OptInVia = z.infer +// CLI feature groups for `cli_feature_used`. Coarse buckets — finer +// per-command granularity is intentionally deferred (see +// docs/cli-telemetry-design.md §"What we explicitly do not ship"). The CLI +// process dedupes per `(feature_group, process)`, so a single agent loop +// that snapshots the page hundreds of times produces exactly one +// `browser_observation` event for that whole process. +export const CLI_FEATURE_GROUP_VALUES = [ + 'worktree_orchestration', + 'terminal_orchestration', + 'terminal_io', + 'browser_navigation', + 'browser_observation', + 'browser_interaction', + 'browser_config', + 'orchestration_coordinator', + 'orchestration_messaging', + 'discovery' +] as const +export const cliFeatureGroupSchema = z.enum(CLI_FEATURE_GROUP_VALUES) +export type CliFeatureGroup = z.infer + // Whitelist of settings whose `setting_key` may be emitted on // `settings_changed`. If a setting isn't in this list, we do not emit. // @@ -187,6 +208,16 @@ const settingsChangedSchema = z const telemetryOptedInSchema = z.object({ via: optInViaSchema }).strict() const telemetryOptedOutSchema = z.object({ via: optInViaSchema }).strict() +// First-write-wins per `(feature_group, CLI process)`: emitted on the first +// call to a group, carrying that call's `exit_status`; subsequent calls in +// the same process never re-emit. See docs/cli-telemetry-design.md §Events. +const cliFeatureUsedSchema = z + .object({ + feature_group: cliFeatureGroupSchema, + exit_status: z.enum(['success', 'failure']) + }) + .strict() + // ── Event registry: the one record the validator consumes ─────────────── // // The validator does `eventSchemas[name].safeParse(props)`. `EventMap` is @@ -212,9 +243,21 @@ export const eventSchemas = { settings_changed: settingsChangedSchema, telemetry_opted_in: telemetryOptedInSchema, - telemetry_opted_out: telemetryOptedOutSchema + telemetry_opted_out: telemetryOptedOutSchema, + + cli_feature_used: cliFeatureUsedSchema } as const +// CLI-eligible event subset: the runtime RPC method narrows the incoming +// `name` to this set so the CLI process can never emit non-CLI events +// (e.g. `agent_started`) by spoofing the wire. Keep aligned with the +// `cli_*` prefix convention. +export const CLI_EVENT_NAMES = ['cli_feature_used'] as const +export type CliEventName = (typeof CLI_EVENT_NAMES)[number] +export function isCliEventName(name: string): name is CliEventName { + return (CLI_EVENT_NAMES as readonly string[]).includes(name) +} + export type EventMap = { [N in keyof typeof eventSchemas]: z.infer<(typeof eventSchemas)[N]> } export type EventName = keyof EventMap export type EventProps = EventMap[N]