Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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);
Comment thread
shrey150 marked this conversation as resolved.
Outdated
// 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");
Comment thread
shrey150 marked this conversation as resolved.
Outdated
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