From e65d4f1943307a229c3b6656b607dfb0cfbbda4a Mon Sep 17 00:00:00 2001 From: Shrey Pandya Date: Fri, 12 Jun 2026 12:38:29 -0700 Subject: [PATCH 1/4] feat(cli): emit skill_id and first_success telemetry properties MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../telemetry-skill-id-first-success.md | 8 + packages/cli/src/lib/run-telemetry.ts | 1 + packages/cli/src/lib/skills/install.ts | 6 + packages/cli/src/lib/telemetry.ts | 69 ++++++- packages/cli/tests/cli-telemetry.test.ts | 175 +++++++++++++++++- 5 files changed, 256 insertions(+), 3 deletions(-) create mode 100644 .changeset/telemetry-skill-id-first-success.md diff --git a/.changeset/telemetry-skill-id-first-success.md b/.changeset/telemetry-skill-id-first-success.md new file mode 100644 index 000000000..e68254602 --- /dev/null +++ b/.changeset/telemetry-skill-id-first-success.md @@ -0,0 +1,8 @@ +--- +"browse": patch +--- + +Emit `skill_id` and `first_success` properties on `cli.command_completed` telemetry. + +- `skill_id`: the validated, catalog-public skill id (e.g. `yelp.com/extract-reviews`, or `bundled/browse` for `skills install`) is attached to the completion event for `browse skills add`/`install`, covering both successful installs and every downstream failure path (`skill_not_found`, `skill_install_failed`, ...). Only the parsed, regex-validated id is ever attached — never the raw argument. +- `first_success: true` is emitted exactly once per anonymous install: the first time a browser-driver command completes successfully, tracked via a marker file stored alongside the existing anonymous install id. Best-effort like all CLI telemetry — it never affects command behavior. diff --git a/packages/cli/src/lib/run-telemetry.ts b/packages/cli/src/lib/run-telemetry.ts index 30d6cd01d..9b4d705ac 100644 --- a/packages/cli/src/lib/run-telemetry.ts +++ b/packages/cli/src/lib/run-telemetry.ts @@ -2,6 +2,7 @@ export interface RunTelemetryState { resultCode?: string; httpStatus?: number; requestHadHttpResponse?: boolean; + skillId?: string; } let currentRunTelemetry: RunTelemetryState = {}; diff --git a/packages/cli/src/lib/skills/install.ts b/packages/cli/src/lib/skills/install.ts index 2eee173bd..82c129ba9 100644 --- a/packages/cli/src/lib/skills/install.ts +++ b/packages/cli/src/lib/skills/install.ts @@ -13,6 +13,7 @@ import { delimiter, dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { fail } from "../errors.js"; +import { setRunTelemetryCompletion } from "../run-telemetry.js"; import { defaultSkillsApiBaseUrl, isRecord, responseDetail } from "./shared.js"; const defaultBlobBaseUrl = @@ -106,6 +107,10 @@ export function isBlobSkillId(skillId: ParsedSkillId): boolean { export async function installSkill(rawSkillId: string): Promise { const skillId = parseSkillId(rawSkillId); + // Attach only the validated, catalog-public id to telemetry — never the raw + // argument, which can contain arbitrary user input. Setting it right after + // parsing covers success and every downstream failure path. + setRunTelemetryCompletion({ skillId: skillId.id }); const npxPath = await findExecutable("npx"); if (!npxPath) { fail( @@ -151,6 +156,7 @@ export async function installSkill(rawSkillId: string): Promise { } export async function installBundledCliSkill(): Promise { + setRunTelemetryCompletion({ skillId: "bundled/browse" }); const npxPath = await findExecutable("npx"); if (!npxPath) { fail( diff --git a/packages/cli/src/lib/telemetry.ts b/packages/cli/src/lib/telemetry.ts index 8b0f04024..b27bc0521 100644 --- a/packages/cli/src/lib/telemetry.ts +++ b/packages/cli/src/lib/telemetry.ts @@ -1,5 +1,5 @@ import { randomUUID } from "node:crypto"; -import { mkdir, readFile, writeFile } from "node:fs/promises"; +import { access, mkdir, readFile, writeFile } from "node:fs/promises"; import { homedir } from "node:os"; import { dirname, join } from "node:path"; @@ -113,12 +113,14 @@ export async function captureCommandCompleted( const exitCode = resolveExitCode(error); const success = exitCode === 0; + const firstSuccess = await detectFirstSuccess(command, success); const completionCapture = getCliTelemetry(cliVersion) .capture( "cli.command_completed", commandCompletedProperties(command, { error, exitCode, + firstSuccess, recordedError: state.recordedError, success, }), @@ -221,6 +223,7 @@ function commandCompletedProperties( completion: { error: Error | undefined; exitCode: number; + firstSuccess?: boolean; recordedError?: RecordedCommandError; success: boolean; }, @@ -236,7 +239,7 @@ function commandCompletedProperties( runTelemetry.resultCode ?? fallbackResultCode(completion.success, errorType); - return { + const properties: TelemetryProperties = { command_path: invocation.commandPath, top_level_command: invocation.topLevelCommand, leaf_command: invocation.leafCommand, @@ -255,7 +258,69 @@ function commandCompletedProperties( failureTelemetry?.requestHadHttpResponse ?? runTelemetry.requestHadHttpResponse ?? null, + skill_id: runTelemetry.skillId ?? null, }; + + if (completion.firstSuccess) { + properties.first_success = true; + } + + return properties; +} + +// Top-level command topics that are not browser-driver commands. Activation +// (first_success) counts only successful driver commands, so new driver +// commands are covered by default. +const nonDriverTopLevelCommands = new Set([ + "cloud", + "daemon", + "doctor", + "functions", + "skills", + "status", + "stop", + "templates", +]); + +const activatedMarkerFileName = "activated"; + +// Returns true exactly once per install: the first time a browser-driver +// command completes successfully. Tracked via a marker file alongside the +// anonymous install id. Best-effort — never throws, never blocks the command. +async function detectFirstSuccess( + command: CommandInvocation, + success: boolean, + env: NodeJS.ProcessEnv = process.env, +): Promise { + if ( + !success || + nonDriverTopLevelCommands.has(command.topLevelCommand) || + isTelemetryDisabled(env) + ) { + return false; + } + + const markerPath = join( + dirname(resolveInstallIdPath(env)), + activatedMarkerFileName, + ); + + try { + await access(markerPath); + return false; + } catch { + // No marker yet: this is the first successful driver command. + } + + try { + await mkdir(dirname(markerPath), { recursive: true }); + await writeFile(markerPath, `${new Date().toISOString()}\n`, "utf8"); + } catch { + // If persistence fails, a later success may report first_success again + // rather than affecting CLI behavior. + } + + return true; } function fallbackResultCode( diff --git a/packages/cli/tests/cli-telemetry.test.ts b/packages/cli/tests/cli-telemetry.test.ts index 5ed951d17..30785c012 100644 --- a/packages/cli/tests/cli-telemetry.test.ts +++ b/packages/cli/tests/cli-telemetry.test.ts @@ -1,9 +1,11 @@ import { access, mkdtemp, rm } from "node:fs/promises"; +import net from "node:net"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; +import { getSocketPath } from "../src/lib/driver/daemon/paths.js"; import { jsonResponse, startFakeBrowserbaseServer, @@ -64,6 +66,11 @@ describe("CLI telemetry", () => { expect(completedPayload.properties.result_code).toBe("ok"); expect(completedPayload.properties.http_status).toBe(null); expect(completedPayload.properties.request_had_http_response).toBe(null); + expect(completedPayload.properties.skill_id).toBe(null); + expect(completedPayload.properties).not.toHaveProperty("first_success"); + await expect( + access(join(dirname(installIdFile), "activated")), + ).rejects.toThrow(); } finally { await telemetryServer.close(); } @@ -220,6 +227,172 @@ describe("CLI telemetry", () => { } }); + it("attaches the validated skill id to completion telemetry for skills add", async () => { + const telemetryServer = await startTelemetryServer(); + const skillsApiServer = await startFakeBrowserbaseServer( + (_request, response) => { + jsonResponse(response, 404, { error: "not found" }); + }, + ); + + try { + const installIdFile = await tempInstallIdFile( + "browse-telemetry-skill-id-", + ); + const result = await runCli( + ["skills", "add", "example.com/extract-reviews"], + { + env: { + ...telemetryEnv(telemetryServer, installIdFile), + BROWSE_SKILLS_API_BASE_URL: skillsApiServer.baseUrl, + }, + }, + ); + + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain("not found in the catalog"); + + const payloads = telemetryPayloads(telemetryServer); + const completedPayload = findPayload(payloads, "cli.command_completed"); + expect(completedPayload.properties.command_path).toBe("skills.add"); + expect(completedPayload.properties.skill_id).toBe( + "example.com/extract-reviews", + ); + expect(completedPayload.properties.result_code).toBe("skill_not_found"); + expect(completedPayload.properties).not.toHaveProperty("first_success"); + + const invokedPayload = findPayload(payloads, "cli.command_invoked"); + expect(invokedPayload.properties).not.toHaveProperty("skill_id"); + } finally { + await skillsApiServer.close(); + await telemetryServer.close(); + } + }); + + it("never attaches unvalidated skill arguments to telemetry", async () => { + const telemetryServer = await startTelemetryServer(); + + try { + const installIdFile = await tempInstallIdFile( + "browse-telemetry-skill-raw-", + ); + const result = await runCli( + ["skills", "add", "../secret-local-path/oops"], + { env: telemetryEnv(telemetryServer, installIdFile) }, + ); + + expect(result.exitCode).toBe(1); + + const payloads = telemetryPayloads(telemetryServer); + const completedPayload = findPayload(payloads, "cli.command_completed"); + expect(completedPayload.properties.skill_id).toBe(null); + expect(completedPayload.properties.result_code).toBe("invalid_skill_id"); + expect(JSON.stringify(payloads)).not.toContain("secret-local-path"); + } finally { + await telemetryServer.close(); + } + }); + + it("emits first_success exactly once for successful driver commands", async () => { + const telemetryServer = await startTelemetryServer(); + const daemonDir = await mkdtemp(join(tmpdir(), "browse-telemetry-daemon-")); + cleanupPaths.push(daemonDir); + const previousDaemonDir = process.env.BROWSE_DAEMON_DIR; + process.env.BROWSE_DAEMON_DIR = daemonDir; + const session = "telemetry-first-success"; + const server = net.createServer((socket) => { + let buffer = ""; + socket.on("data", (chunk) => { + buffer += chunk.toString(); + const newline = buffer.indexOf("\n"); + if (newline === -1) return; + + const request = JSON.parse(buffer.slice(0, newline)) as { + id: string; + type: string; + }; + + if (request.type === "status") { + socket.end( + JSON.stringify({ + data: { + browserConnected: true, + initialized: true, + mode: "managed-local", + pages: [], + pid: process.pid, + session, + target: { headless: true, kind: "managed-local" }, + }, + id: request.id, + type: "success", + }) + "\n", + ); + return; + } + + socket.end( + JSON.stringify({ + data: { ok: true }, + id: request.id, + type: "success", + }) + "\n", + ); + }); + }); + + try { + await new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(getSocketPath(session), resolve); + }); + + const installIdFile = await tempInstallIdFile( + "browse-telemetry-first-success-", + ); + const env = { + ...telemetryEnv(telemetryServer, installIdFile), + BROWSE_DAEMON_DIR: daemonDir, + }; + + const first = await runCli( + ["viewport", "1024", "768", "--session", session], + { env }, + ); + expect(first.exitCode).toBe(0); + + const second = await runCli( + ["viewport", "800", "600", "--session", session], + { env }, + ); + expect(second.exitCode).toBe(0); + + const completedPayloads = telemetryPayloads(telemetryServer).filter( + (payload) => payload.event === "cli.command_completed", + ); + expect(completedPayloads).toHaveLength(2); + expect(completedPayloads[0]?.properties.first_success).toBe(true); + expect(completedPayloads[1]?.properties).not.toHaveProperty( + "first_success", + ); + + // The activation marker persists alongside the anonymous install id. + await expect( + access(join(dirname(installIdFile), "activated")), + ).resolves.toBeUndefined(); + } finally { + if (previousDaemonDir === undefined) { + delete process.env.BROWSE_DAEMON_DIR; + } else { + process.env.BROWSE_DAEMON_DIR = previousDaemonDir; + } + await new Promise((resolve, reject) => { + server.close((error) => (error ? reject(error) : resolve())); + }); + await telemetryServer.close(); + } + }); + it("does not emit telemetry for help and topic-help paths", async () => { const telemetryServer = await startTelemetryServer(); From c87ee42a0ab920696d80bef3d281ef91865261e8 Mon Sep 17 00:00:00 2001 From: Shrey Pandya Date: Fri, 12 Jun 2026 12:45:22 -0700 Subject: [PATCH 2/4] fix(cli): create first_success marker atomically (wx) to prevent race 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 --- packages/cli/src/lib/telemetry.ts | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/cli/src/lib/telemetry.ts b/packages/cli/src/lib/telemetry.ts index b27bc0521..aa710caef 100644 --- a/packages/cli/src/lib/telemetry.ts +++ b/packages/cli/src/lib/telemetry.ts @@ -1,5 +1,5 @@ import { randomUUID } from "node:crypto"; -import { access, mkdir, readFile, writeFile } from "node:fs/promises"; +import { mkdir, readFile, writeFile } from "node:fs/promises"; import { homedir } from "node:os"; import { dirname, join } from "node:path"; @@ -305,19 +305,19 @@ async function detectFirstSuccess( activatedMarkerFileName, ); - try { - await access(markerPath); - return false; - } catch { - // No marker yet: this is the first successful driver command. - } - try { await mkdir(dirname(markerPath), { recursive: true }); - await writeFile(markerPath, `${new Date().toISOString()}\n`, "utf8"); + // Atomic create: exactly one run can win the race, so concurrent + // commands cannot both report first_success. + await writeFile(markerPath, `${new Date().toISOString()}\n`, { + encoding: "utf8", + flag: "wx", + }); } catch { - // If persistence fails, a later success may report first_success again - // rather than affecting CLI behavior. + // Marker already exists (this install already activated, or a concurrent + // run won the race) or persistence failed. Either way, do not report — + // and never affect CLI behavior. + return false; } return true; From 2c526dac53c63971a6031babb0eadc181a773112 Mon Sep 17 00:00:00 2001 From: Shrey Pandya Date: Fri, 12 Jun 2026 15:38:30 -0700 Subject: [PATCH 3/4] =?UTF-8?q?refactor(cli):=20drop=20first=5Fsuccess=20p?= =?UTF-8?q?roperty=20=E2=80=94=20derived=20backend-side=20via=20first=5Fti?= =?UTF-8?q?me=5Ffor=5Fuser?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../telemetry-skill-id-first-success.md | 5 +- packages/cli/src/lib/telemetry.ts | 66 +---------- packages/cli/tests/cli-telemetry.test.ts | 109 +----------------- 3 files changed, 4 insertions(+), 176 deletions(-) diff --git a/.changeset/telemetry-skill-id-first-success.md b/.changeset/telemetry-skill-id-first-success.md index e68254602..852fbae37 100644 --- a/.changeset/telemetry-skill-id-first-success.md +++ b/.changeset/telemetry-skill-id-first-success.md @@ -2,7 +2,6 @@ "browse": patch --- -Emit `skill_id` and `first_success` properties on `cli.command_completed` telemetry. +Emit a `skill_id` property on `cli.command_completed` telemetry. -- `skill_id`: the validated, catalog-public skill id (e.g. `yelp.com/extract-reviews`, or `bundled/browse` for `skills install`) is attached to the completion event for `browse skills add`/`install`, covering both successful installs and every downstream failure path (`skill_not_found`, `skill_install_failed`, ...). Only the parsed, regex-validated id is ever attached — never the raw argument. -- `first_success: true` is emitted exactly once per anonymous install: the first time a browser-driver command completes successfully, tracked via a marker file stored alongside the existing anonymous install id. Best-effort like all CLI telemetry — it never affects command behavior. +The validated, catalog-public skill id (e.g. `yelp.com/extract-reviews`, or `bundled/browse` for `skills install`) is attached to the completion event for `browse skills add`/`install`, covering both successful installs and every downstream failure path (`skill_not_found`, `skill_install_failed`, ...). Only the parsed, regex-validated id is ever attached — never the raw argument. diff --git a/packages/cli/src/lib/telemetry.ts b/packages/cli/src/lib/telemetry.ts index aa710caef..00d91b97d 100644 --- a/packages/cli/src/lib/telemetry.ts +++ b/packages/cli/src/lib/telemetry.ts @@ -113,14 +113,12 @@ export async function captureCommandCompleted( const exitCode = resolveExitCode(error); const success = exitCode === 0; - const firstSuccess = await detectFirstSuccess(command, success); const completionCapture = getCliTelemetry(cliVersion) .capture( "cli.command_completed", commandCompletedProperties(command, { error, exitCode, - firstSuccess, recordedError: state.recordedError, success, }), @@ -223,7 +221,6 @@ function commandCompletedProperties( completion: { error: Error | undefined; exitCode: number; - firstSuccess?: boolean; recordedError?: RecordedCommandError; success: boolean; }, @@ -239,7 +236,7 @@ function commandCompletedProperties( runTelemetry.resultCode ?? fallbackResultCode(completion.success, errorType); - const properties: TelemetryProperties = { + return { command_path: invocation.commandPath, top_level_command: invocation.topLevelCommand, leaf_command: invocation.leafCommand, @@ -260,67 +257,6 @@ function commandCompletedProperties( null, skill_id: runTelemetry.skillId ?? null, }; - - if (completion.firstSuccess) { - properties.first_success = true; - } - - return properties; -} - -// Top-level command topics that are not browser-driver commands. Activation -// (first_success) counts only successful driver commands, so new driver -// commands are covered by default. -const nonDriverTopLevelCommands = new Set([ - "cloud", - "daemon", - "doctor", - "functions", - "skills", - "status", - "stop", - "templates", -]); - -const activatedMarkerFileName = "activated"; - -// Returns true exactly once per install: the first time a browser-driver -// command completes successfully. Tracked via a marker file alongside the -// anonymous install id. Best-effort — never throws, never blocks the command. -async function detectFirstSuccess( - command: CommandInvocation, - success: boolean, - env: NodeJS.ProcessEnv = process.env, -): Promise { - if ( - !success || - nonDriverTopLevelCommands.has(command.topLevelCommand) || - isTelemetryDisabled(env) - ) { - return false; - } - - const markerPath = join( - dirname(resolveInstallIdPath(env)), - activatedMarkerFileName, - ); - - try { - await mkdir(dirname(markerPath), { recursive: true }); - // Atomic create: exactly one run can win the race, so concurrent - // commands cannot both report first_success. - await writeFile(markerPath, `${new Date().toISOString()}\n`, { - encoding: "utf8", - flag: "wx", - }); - } catch { - // Marker already exists (this install already activated, or a concurrent - // run won the race) or persistence failed. Either way, do not report — - // and never affect CLI behavior. - return false; - } - - return true; } function fallbackResultCode( diff --git a/packages/cli/tests/cli-telemetry.test.ts b/packages/cli/tests/cli-telemetry.test.ts index 30785c012..8d8f14df9 100644 --- a/packages/cli/tests/cli-telemetry.test.ts +++ b/packages/cli/tests/cli-telemetry.test.ts @@ -1,11 +1,9 @@ import { access, mkdtemp, rm } from "node:fs/promises"; -import net from "node:net"; import { tmpdir } from "node:os"; -import { dirname, join } from "node:path"; +import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; -import { getSocketPath } from "../src/lib/driver/daemon/paths.js"; import { jsonResponse, startFakeBrowserbaseServer, @@ -67,10 +65,6 @@ describe("CLI telemetry", () => { expect(completedPayload.properties.http_status).toBe(null); expect(completedPayload.properties.request_had_http_response).toBe(null); expect(completedPayload.properties.skill_id).toBe(null); - expect(completedPayload.properties).not.toHaveProperty("first_success"); - await expect( - access(join(dirname(installIdFile), "activated")), - ).rejects.toThrow(); } finally { await telemetryServer.close(); } @@ -259,7 +253,6 @@ describe("CLI telemetry", () => { "example.com/extract-reviews", ); expect(completedPayload.properties.result_code).toBe("skill_not_found"); - expect(completedPayload.properties).not.toHaveProperty("first_success"); const invokedPayload = findPayload(payloads, "cli.command_invoked"); expect(invokedPayload.properties).not.toHaveProperty("skill_id"); @@ -293,106 +286,6 @@ describe("CLI telemetry", () => { } }); - it("emits first_success exactly once for successful driver commands", async () => { - const telemetryServer = await startTelemetryServer(); - const daemonDir = await mkdtemp(join(tmpdir(), "browse-telemetry-daemon-")); - cleanupPaths.push(daemonDir); - const previousDaemonDir = process.env.BROWSE_DAEMON_DIR; - process.env.BROWSE_DAEMON_DIR = daemonDir; - const session = "telemetry-first-success"; - const server = net.createServer((socket) => { - let buffer = ""; - socket.on("data", (chunk) => { - buffer += chunk.toString(); - const newline = buffer.indexOf("\n"); - if (newline === -1) return; - - const request = JSON.parse(buffer.slice(0, newline)) as { - id: string; - type: string; - }; - - if (request.type === "status") { - socket.end( - JSON.stringify({ - data: { - browserConnected: true, - initialized: true, - mode: "managed-local", - pages: [], - pid: process.pid, - session, - target: { headless: true, kind: "managed-local" }, - }, - id: request.id, - type: "success", - }) + "\n", - ); - return; - } - - socket.end( - JSON.stringify({ - data: { ok: true }, - id: request.id, - type: "success", - }) + "\n", - ); - }); - }); - - try { - await new Promise((resolve, reject) => { - server.once("error", reject); - server.listen(getSocketPath(session), resolve); - }); - - const installIdFile = await tempInstallIdFile( - "browse-telemetry-first-success-", - ); - const env = { - ...telemetryEnv(telemetryServer, installIdFile), - BROWSE_DAEMON_DIR: daemonDir, - }; - - const first = await runCli( - ["viewport", "1024", "768", "--session", session], - { env }, - ); - expect(first.exitCode).toBe(0); - - const second = await runCli( - ["viewport", "800", "600", "--session", session], - { env }, - ); - expect(second.exitCode).toBe(0); - - const completedPayloads = telemetryPayloads(telemetryServer).filter( - (payload) => payload.event === "cli.command_completed", - ); - expect(completedPayloads).toHaveLength(2); - expect(completedPayloads[0]?.properties.first_success).toBe(true); - expect(completedPayloads[1]?.properties).not.toHaveProperty( - "first_success", - ); - - // The activation marker persists alongside the anonymous install id. - await expect( - access(join(dirname(installIdFile), "activated")), - ).resolves.toBeUndefined(); - } finally { - if (previousDaemonDir === undefined) { - delete process.env.BROWSE_DAEMON_DIR; - } else { - process.env.BROWSE_DAEMON_DIR = previousDaemonDir; - } - await new Promise((resolve, reject) => { - server.close((error) => (error ? reject(error) : resolve())); - }); - await telemetryServer.close(); - } - }); - it("does not emit telemetry for help and topic-help paths", async () => { const telemetryServer = await startTelemetryServer(); From 41a8c56a37f1cbe3246560dbcf07b553c9a05876 Mon Sep 17 00:00:00 2001 From: Shrey Pandya Date: Tue, 16 Jun 2026 14:02:32 -0700 Subject: [PATCH 4/4] test(cli): address review on skill_id telemetry - rename local skillId -> skill (skill.id reads cleaner) - assert the full completed-event shape with toMatchObject Co-Authored-By: Claude Fable 5 --- packages/cli/src/lib/skills/install.ts | 12 ++++++------ packages/cli/tests/cli-telemetry.test.ts | 11 ++++++----- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/packages/cli/src/lib/skills/install.ts b/packages/cli/src/lib/skills/install.ts index 82c129ba9..fe970e535 100644 --- a/packages/cli/src/lib/skills/install.ts +++ b/packages/cli/src/lib/skills/install.ts @@ -106,11 +106,11 @@ export function isBlobSkillId(skillId: ParsedSkillId): boolean { } export async function installSkill(rawSkillId: string): Promise { - const skillId = parseSkillId(rawSkillId); + const skill = parseSkillId(rawSkillId); // Attach only the validated, catalog-public id to telemetry — never the raw // argument, which can contain arbitrary user input. Setting it right after // parsing covers success and every downstream failure path. - setRunTelemetryCompletion({ skillId: skillId.id }); + setRunTelemetryCompletion({ skillId: skill.id }); const npxPath = await findExecutable("npx"); if (!npxPath) { fail( @@ -120,9 +120,9 @@ export async function installSkill(rawSkillId: string): Promise { ); } - const files = await fetchSkillFiles(skillId); + const files = await fetchSkillFiles(skill); if (files.status === "found") { - const result = await downloadBlobSkill(skillId, files.files); + const result = await downloadBlobSkill(skill, files.files); process.stdout.write( `Downloaded ${result.fileCount} skill file${result.fileCount === 1 ? "" : "s"} to ${result.installPath}\n`, ); @@ -137,7 +137,7 @@ export async function installSkill(rawSkillId: string): Promise { if (files.status === "not_found") { fail( - `Skill "${skillId.id}" not found in the catalog. Run \`browse skills find ${skillId.domain}\` to discover available skills, or \`browse skills list\` to browse them.`, + `Skill "${skill.id}" not found in the catalog. Run \`browse skills find ${skill.domain}\` to discover available skills, or \`browse skills list\` to browse them.`, 1, { resultCode: "skill_not_found" }, ); @@ -151,7 +151,7 @@ export async function installSkill(rawSkillId: string): Promise { "add", "browserbase/browse.sh", "--skill", - skillId.id, + skill.id, ]); } diff --git a/packages/cli/tests/cli-telemetry.test.ts b/packages/cli/tests/cli-telemetry.test.ts index 8d8f14df9..1287fd1d5 100644 --- a/packages/cli/tests/cli-telemetry.test.ts +++ b/packages/cli/tests/cli-telemetry.test.ts @@ -248,11 +248,12 @@ describe("CLI telemetry", () => { const payloads = telemetryPayloads(telemetryServer); const completedPayload = findPayload(payloads, "cli.command_completed"); - expect(completedPayload.properties.command_path).toBe("skills.add"); - expect(completedPayload.properties.skill_id).toBe( - "example.com/extract-reviews", - ); - expect(completedPayload.properties.result_code).toBe("skill_not_found"); + expect(completedPayload.properties).toMatchObject({ + command_path: "skills.add", + skill_id: "example.com/extract-reviews", + result_code: "skill_not_found", + success: false, + }); const invokedPayload = findPayload(payloads, "cli.command_invoked"); expect(invokedPayload.properties).not.toHaveProperty("skill_id");