Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/telemetry-skill-id-first-success.md
Original file line number Diff line number Diff line change
@@ -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.
1 change: 1 addition & 0 deletions packages/cli/src/lib/run-telemetry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ export interface RunTelemetryState {
resultCode?: string;
httpStatus?: number;
requestHadHttpResponse?: boolean;
skillId?: string;
}

let currentRunTelemetry: RunTelemetryState = {};
Expand Down
6 changes: 6 additions & 0 deletions packages/cli/src/lib/skills/install.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -106,6 +107,10 @@ export function isBlobSkillId(skillId: ParsedSkillId): boolean {

export async function installSkill(rawSkillId: string): Promise<void> {
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(
Expand Down Expand Up @@ -151,6 +156,7 @@ export async function installSkill(rawSkillId: string): Promise<void> {
}

export async function installBundledCliSkill(): Promise<void> {
setRunTelemetryCompletion({ skillId: "bundled/browse" });
const npxPath = await findExecutable("npx");
if (!npxPath) {
fail(
Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/lib/telemetry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ function commandCompletedProperties(
failureTelemetry?.requestHadHttpResponse ??
Comment thread
shrey150 marked this conversation as resolved.
runTelemetry.requestHadHttpResponse ??
null,
skill_id: runTelemetry.skillId ?? null,
};
}

Expand Down
66 changes: 66 additions & 0 deletions packages/cli/tests/cli-telemetry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -220,6 +221,71 @@ 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");

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();

Expand Down
Loading