From 2c2c1354301a4f82e004f61dffd5e1f7ef6f36eb Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Tue, 5 May 2026 23:06:29 -0700 Subject: [PATCH] WIP: Changes before auto-review fixes Co-authored-by: Orca --- 00-review-context.md | 71 ++++++++++++----------- src/cli/telemetry.test.ts | 52 ++++++++++++++++- src/cli/telemetry.ts | 5 ++ src/main/runtime/rpc/methods/telemetry.ts | 26 ++++----- src/shared/telemetry-events.test.ts | 11 ++++ src/shared/telemetry-events.ts | 2 +- 6 files changed, 117 insertions(+), 50 deletions(-) diff --git a/00-review-context.md b/00-review-context.md index d37e848ba54..63ce8dd0eb6 100644 --- a/00-review-context.md +++ b/00-review-context.md @@ -7,29 +7,31 @@ ## 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 +- 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 | + + +| File | Changed Lines | +| ----------------------------------------------- | ---------------------------------------------------- | +| config/tsconfig.cli.json | 3-8 | +| src/cli/dispatch.ts | 3, 62-74 | +| src/cli/telemetry.test.ts | 1-227 (new file, all lines) | +| src/cli/telemetry.ts | 1-349 (new file, all lines) | +| src/main/runtime/rpc/methods/index.ts | 11, 25-26 | +| src/main/runtime/rpc/methods/telemetry.test.ts | 1-131 (new file, all lines) | +| src/main/runtime/rpc/methods/telemetry.ts | 1-80 (new file, all lines) | +| src/shared/telemetry-events.test.ts | 10-11, 15, 194-283 | +| src/shared/telemetry-events.ts | 113-133, 211-220, 246-248, 251-260 | ## Review Standards Reference @@ -39,15 +41,15 @@ ## File Categories -### Electron/Main +### Electron/Main (priority 1) - 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/Build (priority 4) - config/tsconfig.cli.json -### Utility/Common +### Utility/Common (priority 5) - src/cli/dispatch.ts - src/cli/telemetry.ts - src/cli/telemetry.test.ts @@ -56,20 +58,21 @@ ## 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 + + + + +(none yet) ## Iteration State + + Current iteration: 1 Last completed phase: Validation Files fixed this iteration: [] +Validated issues: + - Fix: src/main/runtime/rpc/methods/telemetry.ts (wrap track() in try/catch) + - Fix: src/cli/telemetry.ts (early return if metadata.authToken is null) + - Fix: src/cli/telemetry.test.ts (add failure→success symmetric dedupe test) +Skipped/non-actionable: ~10 reviewer notes explicitly "no change required" (cosmetic comment wording, mutability style nit, follow-up architectural refactors, observation-only notes) diff --git a/src/cli/telemetry.test.ts b/src/cli/telemetry.test.ts index c426cc9e8f5..b4051fdd1a6 100644 --- a/src/cli/telemetry.test.ts +++ b/src/cli/telemetry.test.ts @@ -17,6 +17,19 @@ import { recordCliFeatureUsed, resolveFeatureGroup } from './telemetry' +import { CORE_HANDLERS } from './handlers/core' +import { REPO_HANDLERS } from './handlers/repo' +import { WORKTREE_HANDLERS } from './handlers/worktree' +import { TERMINAL_HANDLERS } from './handlers/terminal' +import { BROWSER_NAV_HANDLERS } from './handlers/browser-nav' +import { BROWSER_INTERACT_HANDLERS } from './handlers/browser-interact' +import { BROWSER_TAB_HANDLERS } from './handlers/browser-tab' +import { BROWSER_PROFILE_HANDLERS } from './handlers/browser-profile' +import { BROWSER_COOKIE_HANDLERS } from './handlers/browser-cookie' +import { BROWSER_CAPTURE_HANDLERS } from './handlers/browser-capture' +import { BROWSER_ENV_HANDLERS } from './handlers/browser-env' +import { BROWSER_STORAGE_HANDLERS } from './handlers/browser-storage' +import { ORCHESTRATION_HANDLERS } from './handlers/orchestration' import type { RuntimeMetadata } from '../shared/runtime-bootstrap' import type * as MetadataModule from './runtime/metadata' @@ -135,7 +148,11 @@ describe('recordCliFeatureUsed', () => { 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']) + expect(groups.sort()).toEqual([ + 'browser_interaction', + 'browser_navigation', + 'browser_observation' + ]) }) it('records exit_status: failure when the first call fails', async () => { @@ -179,3 +196,36 @@ describe('recordCliFeatureUsed', () => { expect(result).toBeUndefined() }) }) + +// Guards the hand-maintained COMMAND_TO_FEATURE_GROUP map against drift +// from dispatch.ts's handler registration — a new CLI command added to +// any handler group without a corresponding entry would silently emit no +// telemetry, starving the adoption signal. +describe('COMMAND_TO_FEATURE_GROUP completeness', () => { + it('every dispatch handler key resolves to a feature group', () => { + const groups = [ + CORE_HANDLERS, + REPO_HANDLERS, + WORKTREE_HANDLERS, + TERMINAL_HANDLERS, + BROWSER_NAV_HANDLERS, + BROWSER_INTERACT_HANDLERS, + BROWSER_TAB_HANDLERS, + BROWSER_PROFILE_HANDLERS, + BROWSER_COOKIE_HANDLERS, + BROWSER_CAPTURE_HANDLERS, + BROWSER_ENV_HANDLERS, + BROWSER_STORAGE_HANDLERS, + ORCHESTRATION_HANDLERS + ] + const unmapped: string[] = [] + for (const group of groups) { + for (const key of Object.keys(group)) { + if (resolveFeatureGroup(key.split(' ')) === null) { + unmapped.push(key) + } + } + } + expect(unmapped).toEqual([]) + }) +}) diff --git a/src/cli/telemetry.ts b/src/cli/telemetry.ts index cc5613087e1..98d607d7c1d 100644 --- a/src/cli/telemetry.ts +++ b/src/cli/telemetry.ts @@ -69,6 +69,9 @@ const COMMAND_TO_FEATURE_GROUP: Record = { screenshot: 'browser_observation', 'full-screenshot': 'browser_observation', eval: 'browser_observation', + exec: 'browser_observation', + console: 'browser_observation', + network: 'browser_observation', wait: 'browser_observation', pdf: 'browser_observation', get: 'browser_observation', @@ -101,6 +104,8 @@ const COMMAND_TO_FEATURE_GROUP: Record = { 'mouse wheel': 'browser_interaction', // browser_config: viewport/device/cookies/storage/intercept/capture + viewport: 'browser_config', + geolocation: 'browser_config', 'set device': 'browser_config', 'set offline': 'browser_config', 'set headers': 'browser_config', diff --git a/src/main/runtime/rpc/methods/telemetry.ts b/src/main/runtime/rpc/methods/telemetry.ts index a8762294ede..8ba4ce22e17 100644 --- a/src/main/runtime/rpc/methods/telemetry.ts +++ b/src/main/runtime/rpc/methods/telemetry.ts @@ -18,24 +18,16 @@ import { z } from 'zod' import { defineMethod, type RpcMethod } from '../core' import { track } from '../../../telemetry/client' import { - cliFeatureGroupSchema, + cliFeatureUsedSchema, 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() - +// Per-CLI-event property narrowing happens via the shared `eventSchemas` +// entries (re-exported here as `cliFeatureUsedSchema`). One shape +// definition; no drift between this boundary check and the central +// validator inside `track()`. const CaptureCliEventParams = z.object({ name: z .unknown() @@ -65,7 +57,7 @@ export const TELEMETRY_METHODS: RpcMethod[] = [ // because `name` is the discriminated `CliEventName` union. switch (name) { case 'cli_feature_used': { - const parsed = CliFeatureUsedProps.safeParse(params.props) + const parsed = cliFeatureUsedSchema.safeParse(params.props) if (!parsed.success) { return {} } @@ -76,6 +68,12 @@ export const TELEMETRY_METHODS: RpcMethod[] = [ track(name, parsed.data satisfies EventProps<'cli_feature_used'>) return {} } + default: { + // Force a `tsc` error if a new `CliEventName` lands without a + // matching `case`. Without this, the switch would silently fall + // through and drop the new event. + return name satisfies never + } } } }) diff --git a/src/shared/telemetry-events.test.ts b/src/shared/telemetry-events.test.ts index 4af6b001529..8dbc4513d1f 100644 --- a/src/shared/telemetry-events.test.ts +++ b/src/shared/telemetry-events.test.ts @@ -268,6 +268,17 @@ describe('isCliEventName', () => { expect(isCliEventName('made_up_event')).toBe(false) expect(isCliEventName('')).toBe(false) }) + + // Pin the hand-maintained CLI_EVENT_NAMES list to the `cli_*` prefix + // convention. A future `cli_*` event added to `eventSchemas` but missing + // here would silently fail the runtime RPC narrow. + it('CLI_EVENT_NAMES matches cli_* prefixed keys in eventSchemas', () => { + const expected = Object.keys(eventSchemas) + .filter((name) => name.startsWith('cli_')) + .sort() + const actual = [...CLI_EVENT_NAMES].sort() + expect(actual).toEqual(expected) + }) }) describe('exported enum schemas', () => { diff --git a/src/shared/telemetry-events.ts b/src/shared/telemetry-events.ts index e8a74d5b229..899a7adc3c7 100644 --- a/src/shared/telemetry-events.ts +++ b/src/shared/telemetry-events.ts @@ -211,7 +211,7 @@ 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 +export const cliFeatureUsedSchema = z .object({ feature_group: cliFeatureGroupSchema, exit_status: z.enum(['success', 'failure'])