starred/orca
1
0
Fork 0
mirror of https://github.com/stablyai/orca.git synced 2026-10-12 08:02:37 +00:00
Code Issues Packages Projects Releases Wiki Activity

WIP: Changes before auto-review fixes

Browse Source 
Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
brennanb2025
2026-05-05 23:06:29 -07:00
co-authored by Orca
parent 9b489c2bcb
commit 2c2c135430
6 changed files with 117 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files
00-review-context.md
+37 -34
View File
@@ -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 |
<!-- In scope: issues on these lines OR caused by these changes. Out of scope: unrelated pre-existing issues -->
| 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
<!-- Issues validated but deemed not worth fixing. Do not re-validate these in future iterations. -->
<!-- Format: [file:line-range] | [severity] | [reason skipped] | [issue summary] -->
<!-- NOTE: Skips should be RARE - only purely cosmetic issues with no functional impact -->
(none yet)
## Iteration State
<!-- Updated after each phase to enable crash recovery -->
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)
src/cli/telemetry.test.ts
+51 -1
View File
@@ -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([])
})
})
src/cli/telemetry.ts
+5
View File
@@ -69,6 +69,9 @@ const COMMAND_TO_FEATURE_GROUP: Record<string, CliFeatureGroup> = {
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<string, CliFeatureGroup> = {
'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',
src/main/runtime/rpc/methods/telemetry.ts
+12 -14
View File
@@ -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
}
}
}
})
src/shared/telemetry-events.test.ts
+11
View File
@@ -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', () => {
src/shared/telemetry-events.ts
+1 -1
View File
@@ -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'])