[STG-2280] feat(cli): emit skill_id telemetry property#2251
Conversation
skill_id: attach the validated, catalog-public skill id to cli.command_completed for browse skills add/install (success and all downstream failure paths). Never the raw argument — only the parsed, regex-validated id. first_success: emitted exactly once per anonymous install, the first time a browser-driver command completes successfully. Tracked via an 'activated' marker file alongside the existing anonymous install id. Best-effort: telemetry never affects command behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 2c526da The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
1 issue found across 5 files
Confidence score: 4/5
- In
packages/cli/src/lib/telemetry.ts, the non-atomicfirst_successmarker write can race across concurrent CLI runs, causing duplicatefirst_successtelemetry and skewed analytics after merge; switch to an atomic create (flag: "wx") and handleEEXISTas "not first" before merging.
Architecture diagram
sequenceDiagram
participant CLI as CLI Command
participant Install as installSkill()
participant Parse as parseSkillId()
participant Telemetry as Telemetry Module
participant Marker as Activated Marker
participant PostHog as PostHog API
Note over CLI,PostHog: SKILL_ID FLOW (skills add / install commands)
CLI->>Install: rawSkillId (e.g. "google.com/search-flights-ts4g1f")
Install->>Parse: parseSkillId(rawSkillId)
Parse-->>Install: validated skill id or throws
alt Validated successfully
Install->>Telemetry: setRunTelemetryCompletion({ skillId: id.id })
else Invalid (e.g. "../secret-local-path/oops")
Install-->>CLI: fail()
CLI->>Telemetry: captureCommandCompleted()
Telemetry->>Telemetry: skill_id = null, result_code = "invalid_skill_id"
Note over Telemetry,PostHog: Raw argument NEVER attached to payload
end
Install->>Install: npx skills add <skillId>
alt Failure (e.g. skill_not_found)
Install-->>CLI: fail()
CLI->>Telemetry: captureCommandCompleted()
Telemetry->>Telemetry: skill_id still set from earlier call
Telemetry->>PostHog: { command_path: "skills.add", skill_id: "...", result_code: "skill_not_found" }
else Success
Install-->>CLI: exit 0
CLI->>Telemetry: captureCommandCompleted()
Telemetry->>PostHog: { command_path: "skills.add", skill_id: "...", result_code: "ok" }
end
Note over CLI,PostHog: FIRST_SUCCESS FLOW (browser-driver commands)
CLI->>CLI: Command completes (success or failure)
CLI->>Telemetry: captureCommandCompleted(command, success)
Telemetry->>Telemetry: detectFirstSuccess(command, success)
alt Non-driver command (cloud, skills, daemon, status, ...)
Telemetry->>Telemetry: return false — no first_success property
else Driver command + failure
Telemetry->>Telemetry: return false — no first_success property
else Driver command + success
Telemetry->>Telemetry: Check isTelemetryDisabled()
opt Telemetry disabled (DO_NOT_TRACK, CI, etc.)
Telemetry->>Telemetry: return false — no marker check, no property
end
Telemetry->>Marker: access(<install-id-dir>/activated)
alt Marker exists
Marker-->>Telemetry: File found
Telemetry->>Telemetry: return false — already activated
else No marker yet
Marker-->>Telemetry: File not found
Telemetry->>Marker: mkdir + writeFile(<install-id-dir>/activated, ISO timestamp)
Telemetry->>Telemetry: return true — emit first_success
end
end
Telemetry->>PostHog: Package properties (includes first_success only when true)
PostHog-->>Telemetry: (async, non-blocking)
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Concurrent CLI runs could both pass the access() check and double-report first_success. Use an exclusive create so exactly one run can win; treat EEXIST (or any persistence failure) as not-first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Re: the What it is: a boolean stamped on exactly one Can the backend derive it? Yes — verified live just now. PostHog's native What backend derivation costs: How important is it? The metric is the north star — #2247 is literally parked on it. The client-side property is a convenience/robustness layer, not the only way to get the metric. Recommendation, given the live proof: consistent with the de-scope philosophy on #2200 — drop |
… first_time_for_user PostHog's first_time_for_user math on (cli.command_completed, success=true, driver commands) reproduces the activation signal at query time (verified live: 118->456 weekly first-activations over the last 4 weeks), so the client-side marker file is unnecessary. skill_id stays — that data exists in no event today. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Amended in 2c526dac5 per the decision above — |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Design note for the record — why Verified against the code:
Cost asymmetry vs the dropped Complementary uses of the API logs: authoritative skill popularity (including non-CLI catalog clients) and cross-checking CLI-reported totals. If true server-side joinability is ever wanted, the path is adding |
Summary
Adds two properties to the existing
cli.command_completedtelemetry event:skill_id— the validated, catalog-public skill id (e.g.google.com/search-flights-ts4g1f, orbundled/browseforbrowse skills install), attached forskills add/installon success and every downstream failure path (skill_not_found,skill_install_failed,npx_missing, ...).nullon all other commands.Impact if merged
Implementation notes
skill_idis set only afterparseSkillIdvalidation succeeds (ids are catalog-public and regex-constrained). The raw, unparsed argument — which can contain arbitrary user input or local paths — is never attached. A test asserts the raw argument does not appear anywhere in any payload.skill_idflows through the existingRunTelemetryState(same channel asresult_code/http_status), so it is captured by the onecli.command_completedemit point regardless of how the command ends.E2E Test Matrix
Run against the locally built
browse(node <local build>/bin/run.js) with a local HTTP capture server (BROWSERBASE_TELEMETRY_HOST=http://127.0.0.1:<port>) logging full POST bodies, and a freshBROWSERBASE_TELEMETRY_INSTALL_ID_FILEdir. All payloads below are real capturedcli.command_completedevents.browse skills add google.com/search-flights-ts4g1f(real catalog + realnpx skills add, exit 0){"command_path":"skills.add", "success":true, "result_code":"ok", "skill_id":"google.com/search-flights-ts4g1f"}browse skills find flights(real catalog)google.com/search-flights-ts4g1f; captured event hasskill_id:nullskill_idstays null for non-install skills commands.pnpm --filter browse lint(prettier + eslint + tsc)Changeset
🤖 Generated with Claude Code
Summary by cubic
New Features
skill_id: attach the validated, catalog-public id forskills add/installon success and all failure paths;nullfor other commands. Never includes the raw argument.Bug Fixes
Written for commit c87ee42. Summary will update on new commits.
Why no first_success
The original revision also stamped a
first_successproperty via a per-install marker file. Dropped in2c526dac5per maintainer decision: PostHog'sfirst_time_for_usermath on (cli.command_completed,success=true, driver commands) derives the same activation signal at query time — verified live (118→456 weekly first-activations over the last 4 weeks; ~2%→37% normalized per new install). Details: #2251 (comment).skill_idremains — that data exists in no event today.