diff --git a/.changeset/telemetry-skill-id-first-success.md b/.changeset/telemetry-skill-id-first-success.md new file mode 100644 index 000000000..852fbae37 --- /dev/null +++ b/.changeset/telemetry-skill-id-first-success.md @@ -0,0 +1,7 @@ +--- +"browse": patch +--- + +Emit a `skill_id` property on `cli.command_completed` telemetry. + +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/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..fe970e535 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 = @@ -105,7 +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: skill.id }); const npxPath = await findExecutable("npx"); if (!npxPath) { fail( @@ -115,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`, ); @@ -132,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" }, ); @@ -146,11 +151,12 @@ export async function installSkill(rawSkillId: string): Promise { "add", "browserbase/browse.sh", "--skill", - skillId.id, + skill.id, ]); } 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..00d91b97d 100644 --- a/packages/cli/src/lib/telemetry.ts +++ b/packages/cli/src/lib/telemetry.ts @@ -255,6 +255,7 @@ function commandCompletedProperties( failureTelemetry?.requestHadHttpResponse ?? runTelemetry.requestHadHttpResponse ?? null, + skill_id: runTelemetry.skillId ?? null, }; } diff --git a/packages/cli/tests/cli-telemetry.test.ts b/packages/cli/tests/cli-telemetry.test.ts index 5ed951d17..1287fd1d5 100644 --- a/packages/cli/tests/cli-telemetry.test.ts +++ b/packages/cli/tests/cli-telemetry.test.ts @@ -64,6 +64,7 @@ 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); } finally { await telemetryServer.close(); } @@ -220,6 +221,72 @@ 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).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"); + } 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("does not emit telemetry for help and topic-help paths", async () => { const telemetryServer = await startTelemetryServer();