From 67d53b50422d86b54d2730317bb1203b83cf5d67 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 28 Sep 2026 07:46:58 -0700 Subject: [PATCH 1/3] feat(telemetry): add auth_success, mcp_connect, and mcp_oauth events --- src/mcp/client.ts | 58 ++++- src/telemetry/index.ts | 89 +++++++- src/tui/chrome-state.ts | 29 +++ src/tui/provider/connect.ts | 13 ++ tests/unit/telemetry-product-events.test.ts | 227 +++++++++++++++++++- 5 files changed, 402 insertions(+), 14 deletions(-) diff --git a/src/mcp/client.ts b/src/mcp/client.ts index 21337f238..5cef865ea 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -15,6 +15,14 @@ import type { McpToolAnnotations } from "./tool-permissions.js"; import { buildStdioMcpProcessEnv } from "./stdio-env.js"; import { isHttpServer } from "./is-http-server.js"; import { MCP_CLIENT_NAME } from "../branding.js"; +import { + captureMcpConnect, + captureMcpOauth, + classifyMcpConnectOutcome, + classifyMcpOauthOutcome, + NOOP_TELEMETRY, + type Telemetry, +} from "../telemetry/index.js"; export interface MCPTool { name: string; @@ -94,6 +102,9 @@ export interface MCPConnectOptions { // learns the transport died under a live client. Never fired for // intentional teardown — close() disarms it before closing the transport. onDisconnect?: () => void; + // Optional telemetry handle for enum-only connect/oauth outcome events. No + // globals: callers that care pass their instance; otherwise nothing emits. + telemetry?: Telemetry; } export function unwrapToolContent(content: unknown): string { @@ -137,6 +148,7 @@ interface HTTPAuthContext { interactive: boolean; serverName: string; onAuthorized?: (serverName: string) => void; + telemetry: Telemetry; } interface BrowserAuthFlow { @@ -441,16 +453,28 @@ async function driveRecovery( } const browserFlow = coordinator.browserFlow; - await browserFlow.promptEmitted; - const code = await waitForBrowserAuthCode(context); - await new StreamableHTTPClientTransport( - context.url, - streamableHTTPTransportOptions( - context.authProvider, - coordinator.lifecycle.signal, - ), - ).finishAuth(code); - await coordinator.probe(); + // One enum-only outcome per browser attempt: prompt, wait, token exchange, + // and probe either complete together or map the failure to exactly one of + // cancelled (explicit abort) / timeout (expiry, cap, denial, teardown). + try { + await browserFlow.promptEmitted; + const code = await waitForBrowserAuthCode(context); + await new StreamableHTTPClientTransport( + context.url, + streamableHTTPTransportOptions( + context.authProvider, + coordinator.lifecycle.signal, + ), + ).finishAuth(code); + await coordinator.probe(); + } catch (error) { + captureMcpOauth( + context.telemetry, + classifyMcpOauthOutcome(error, coordinator.lifecycle.signal.aborted), + ); + throw error; + } + captureMcpOauth(context.telemetry, "completed"); } function completeVerifiedRecovery( @@ -763,6 +787,7 @@ async function connectHttp( ...(options.onAuthorized !== undefined ? { onAuthorized: options.onAuthorized } : {}), + telemetry: options.telemetry ?? NOOP_TELEMETRY, }; gateRedirectToAuthorization(authContext); } @@ -835,16 +860,27 @@ export async function connectMCPServers( onWarning: (message: string) => void, options: MCPConnectOptions = {}, ): Promise { + const telemetry = options.telemetry ?? NOOP_TELEMETRY; const results = await Promise.all( configs.map((c) => connectMCPServer(c, options)), ); const clients: MCPClient[] = []; - for (const result of results) { + for (let index = 0; index < results.length; index += 1) { + const result = results[index]; + if (result === undefined) continue; if (result.ok) clients.push(result.client); else onWarning( `[mcp] Warning: failed to connect to MCP server "${result.serverName}": ${result.error}`, ); + // One enum-only event per server: the transport that was dialed and the + // outcome enum. Server name, URL, and error text never leave the process. + const config = configs[index]; + captureMcpConnect(telemetry, { + transport: + config !== undefined && isHttpServer(config) ? "http" : "stdio", + result: classifyMcpConnectOutcome(result), + }); } return clients; } diff --git a/src/telemetry/index.ts b/src/telemetry/index.ts index 8e6688a1d..b6d5d4501 100644 --- a/src/telemetry/index.ts +++ b/src/telemetry/index.ts @@ -63,6 +63,9 @@ export type TelemetryEvent = | "summarizer_failure" | "crash" | "auth_failure" + | "auth_success" + | "mcp_connect" + | "mcp_oauth" // PostHog Surveys event name (space included). Intentional operator feedback // from /feedback — can ship when ambient product telemetry is off; still // blocked by env kill switches. See captureIntentional. @@ -199,6 +202,15 @@ const EVENT_PROPERTY_ALLOWLIST: Record = { // Which provider rejected the credentials, not why — the rejection detail is // provider-authored text and error_class means a JS constructor name. auth_failure: ["auth_provider"], + // Which provider completed sign-in. Same enum as auth_failure, no message: + // a success has no rejection detail to report. + auth_success: ["auth_provider"], + // Per-server connect outcome. transport is the stdio/http connect path and + // result is the outcome enum — never the server name, URL, or error text. + mcp_connect: ["transport", "result"], + // Per-attempt browser-OAuth outcome. result only — never the + // authorization URL, state, server name, or error text. + mcp_oauth: ["result"], // Intentional /feedback survey response (PostHog custom survey capture shape). // Free text is only sent because the operator typed it for that purpose. // turn_trace_id links to the last $ai_generation in this session when known. @@ -226,7 +238,8 @@ export function aiSpansEnabled(env: NodeJS.ProcessEnv = process.env): boolean { /** * Sample rate for successful `$ai_generation` events (0–1). Default 1.0 (no - * drop). Errors (`$ai_is_error: true`), `crash`, and `auth_failure` always ship. + * drop). Errors (`$ai_is_error: true`), `crash`, `auth_failure`, + * `auth_success`, `mcp_connect`, and `mcp_oauth` always ship. */ export function generationSampleRate( env: NodeJS.ProcessEnv = process.env, @@ -529,3 +542,77 @@ export function createTelemetry(options: CreateTelemetryOptions): Telemetry { discard, }; } + +// --------------------------------------------------------------------------- +// Enum-only product events (CL-9017). These helpers live beside the schema +// rather than the call sites so the values they can send stay visible next +// to the allowlists that bound them: fixed enums in, nothing else out. +// Sampling gates only `$ai_generation`, so every event here always ships. +// --------------------------------------------------------------------------- + +export type McpTransport = "stdio" | "http"; + +export type McpConnectOutcome = "ok" | "auth" | "timeout" | "fail"; + +export interface McpConnectReport { + readonly transport: McpTransport; + readonly result: McpConnectOutcome; +} + +// Structural subset of the connect result: the report carries the outcome +// enum, never the server name or the error text it was classified from. +export interface McpConnectClassificationInput { + readonly ok: boolean; + readonly serverName?: string; + readonly error?: string; + readonly authPending?: boolean; +} + +const MCP_CONNECT_TIMEOUT_MESSAGE = /abort|timeout|timed out|expir/i; + +export function classifyMcpConnectOutcome( + result: McpConnectClassificationInput, +): McpConnectOutcome { + if (result.ok) return "ok"; + if (result.authPending === true) return "auth"; + // Substring match, not word match: AbortError surfaces as "The operation + // was aborted", and only the timeout-vs-fail bucket rides on this — the + // error text itself never ships. + if ( + result.error !== undefined && + MCP_CONNECT_TIMEOUT_MESSAGE.test(result.error) + ) + return "timeout"; + return "fail"; +} + +export function captureMcpConnect( + telemetry: Telemetry, + report: McpConnectReport, +): void { + telemetry.capture("mcp_connect", { + transport: report.transport, + result: report.result, + }); +} + +export type McpOauthOutcome = "completed" | "cancelled" | "timeout"; + +// Only an explicit abort (aborted signal, AbortError) is "cancelled". Every +// other browser-wait end without a code — wait expiry, attempt cap, denial, +// teardown — is "timeout", so each attempt maps to exactly one outcome. +export function classifyMcpOauthOutcome( + error: unknown, + aborted: boolean, +): McpOauthOutcome { + if (aborted) return "cancelled"; + if (error instanceof Error && error.name === "AbortError") return "cancelled"; + return "timeout"; +} + +export function captureMcpOauth( + telemetry: Telemetry, + result: McpOauthOutcome, +): void { + telemetry.capture("mcp_oauth", { result }); +} diff --git a/src/tui/chrome-state.ts b/src/tui/chrome-state.ts index 7ea0d8ca8..68a592f01 100644 --- a/src/tui/chrome-state.ts +++ b/src/tui/chrome-state.ts @@ -854,6 +854,18 @@ export type SendFailureKind = "abort" | "auth" | "error"; /** First-party auth_provider values only — never free-text provider labels. */ export type AuthProviderId = "codex" | "xai" | "anthropic" | "other"; +/** + * Map a setup/connect provider id to its telemetry enum. Exact first-party + * matches only — every other catalog id (openai, ollama, custom, …) is + * "other", and free-text display names must never reach this function. + */ +export function authProviderFromConnectId(providerId: string): AuthProviderId { + if (providerId === "codex") return "codex"; + if (providerId === "xai") return "xai"; + if (providerId === "anthropic") return "anthropic"; + return "other"; +} + export interface ClassifiedSendFailure { readonly kind: SendFailureKind; readonly authProvider: AuthProviderId | null; @@ -908,6 +920,23 @@ export function captureAuthFailure( telemetry.capture("auth_failure", { auth_provider: failure.authProvider }); } +// Mirrors ClassifiedSendFailure without the rejection detail: a success has +// no message to classify, only the provider that completed sign-in. Stays +// silent unless the sign-in actually completed, so failed attempts are never +// counted as successes. +export interface AuthSuccessOutcome { + readonly connected: boolean; + readonly authProvider: AuthProviderId | null; +} + +export function captureAuthSuccess( + telemetry: Telemetry, + outcome: AuthSuccessOutcome, +): void { + if (!outcome.connected || outcome.authProvider === null) return; + telemetry.capture("auth_success", { auth_provider: outcome.authProvider }); +} + /** Same classification as `classifyAgentSendFailure`, from the message alone. */ export function classifySendFailureMessage( message: string, diff --git a/src/tui/provider/connect.ts b/src/tui/provider/connect.ts index c047b00d9..992cc47dc 100644 --- a/src/tui/provider/connect.ts +++ b/src/tui/provider/connect.ts @@ -6,6 +6,11 @@ */ import type { Settings } from "../../config/settings.js"; +import { getTelemetry } from "../../telemetry/singleton.js"; +import { + authProviderFromConnectId, + captureAuthSuccess, +} from "../chrome-state.js"; import { runProviderSetup } from "./setup.js"; import type { ProviderSetupConfig } from "./types.js"; import { @@ -84,5 +89,13 @@ export async function connectProviderInline( }); if (!submitted) return { connected: false }; + // Login-completion site: exactly one enum-only event per completed /connect + // sign-in. Cancelled flows return above and failed submits never set + // connected, so neither can emit. The setup choice id maps to the enum with + // other-fallback — free-text names never leave the process. + captureAuthSuccess(getTelemetry(), { + connected: result.connected, + authProvider: authProviderFromConnectId(input.providerId), + }); return result; } diff --git a/tests/unit/telemetry-product-events.test.ts b/tests/unit/telemetry-product-events.test.ts index 6bd18ed5d..9929483a5 100644 --- a/tests/unit/telemetry-product-events.test.ts +++ b/tests/unit/telemetry-product-events.test.ts @@ -48,15 +48,24 @@ import { resetFeedbackStateForTests, } from "../../src/telemetry/feedback.js"; import { + authProviderFromConnectId, captureAuthFailure, + captureAuthSuccess, classifyAgentSendFailure, } from "../../src/tui/chrome-state.js"; +import { + captureMcpConnect, + captureMcpOauth, + classifyMcpConnectOutcome, + classifyMcpOauthOutcome, + type McpConnectClassificationInput, +} from "../../src/telemetry/index.js"; interface BatchBody { batch: { event: string; properties: Record }[]; } -function harness(): { +function harness(env: NodeJS.ProcessEnv = {}): { telemetry: Telemetry; wire: () => Promise; events: () => Promise; @@ -72,7 +81,7 @@ function harness(): { }; const telemetry = createTelemetry({ settings, - env: {}, + env, fetchFn, apiKey: "test-key", }); @@ -755,3 +764,217 @@ test("keys outside an event's allowlist are stripped from the payload", async () expect(event?.properties.command).toBeUndefined(); expect(await wire()).not.toContain("acmecorp"); }); + +// --------------------------------------------------------------------------- +// auth_success — mirrors auth_failure: the provider enum ships, nothing else +// --------------------------------------------------------------------------- + +test("auth_success names the provider and ships even when generations are unsampled", async () => { + // Generation sampling gates only $ai_generation; product events always ship. + const { telemetry, wire, events } = harness({ + CORBITS_TELEMETRY_GENERATION_SAMPLE_RATE: "0", + }); + + for (const authProvider of ["codex", "xai", "anthropic", "other"] as const) { + captureAuthSuccess(telemetry, { connected: true, authProvider }); + } + + const captured = await events(); + expect(captured.map((e) => e.properties.auth_provider)).toEqual([ + "codex", + "xai", + "anthropic", + "other", + ]); + expect(captured.every((e) => e.event === "auth_success")).toBe(true); + expect(await wire()).not.toContain("acmecorp"); +}); + +test("auth_success stays silent unless the sign-in actually completed", async () => { + const { telemetry, events } = harness(); + + captureAuthSuccess(telemetry, { connected: false, authProvider: "codex" }); + captureAuthSuccess(telemetry, { connected: true, authProvider: null }); + + expect(await events()).toEqual([]); +}); + +test("auth_success strips everything outside its allowlist", async () => { + const { telemetry, wire, events } = harness(); + telemetry.capture("auth_success", { + auth_provider: "codex", + serverName: "acme", + message: "connected acmecorp-eng profile", + }); + + const [event] = await events(); + expect(event?.properties.auth_provider).toBe("codex"); + expect(event?.properties.serverName).toBeUndefined(); + expect(event?.properties.message).toBeUndefined(); + expect(await wire()).not.toContain("acmecorp"); +}); + +test("connect ids map to the provider enum exactly, everything else is other", () => { + expect(authProviderFromConnectId("codex")).toBe("codex"); + expect(authProviderFromConnectId("xai")).toBe("xai"); + expect(authProviderFromConnectId("anthropic")).toBe("anthropic"); + expect(authProviderFromConnectId("openai")).toBe("other"); + expect(authProviderFromConnectId("my-codex")).toBe("other"); + expect(authProviderFromConnectId("ChatGPT")).toBe("other"); +}); + +// --------------------------------------------------------------------------- +// mcp_connect — one enum-only event per server +// --------------------------------------------------------------------------- + +test("mcp_connect reports transport and outcome and nothing else", async () => { + const { telemetry, wire, events } = harness(); + + captureMcpConnect(telemetry, { transport: "http", result: "ok" }); + captureMcpConnect(telemetry, { transport: "stdio", result: "auth" }); + captureMcpConnect(telemetry, { transport: "http", result: "timeout" }); + captureMcpConnect(telemetry, { transport: "stdio", result: "fail" }); + + const captured = await events(); + expect(captured.map((e) => e.event)).toEqual([ + "mcp_connect", + "mcp_connect", + "mcp_connect", + "mcp_connect", + ]); + expect(captured.map((e) => e.properties.transport)).toEqual([ + "http", + "stdio", + "http", + "stdio", + ]); + expect(captured.map((e) => e.properties.result)).toEqual([ + "ok", + "auth", + "timeout", + "fail", + ]); + expect(await wire()).not.toContain("acmecorp"); +}); + +test("mcp_connect classifies connect results without leaking server identity", () => { + const ok = { + ok: true, + client: { serverName: "acme-prod" }, + } as unknown as McpConnectClassificationInput; + expect(classifyMcpConnectOutcome(ok)).toBe("ok"); + + const pending = { + ok: false, + serverName: "acme-prod", + error: "Authorization timed out waiting for the browser.", + authPending: true, + } satisfies McpConnectClassificationInput; + expect(classifyMcpConnectOutcome(pending)).toBe("auth"); + + const aborted = { + ok: false, + serverName: "acme-prod", + error: "This operation was aborted", + } satisfies McpConnectClassificationInput; + expect(classifyMcpConnectOutcome(aborted)).toBe("timeout"); + + const refused = { + ok: false, + serverName: "acme-prod", + error: "spawn /opt/acme/bin/server ENOENT", + } satisfies McpConnectClassificationInput; + expect(classifyMcpConnectOutcome(refused)).toBe("fail"); +}); + +test("mcp_connect strips serverName, url, and error text", async () => { + const { telemetry, wire, events } = harness(); + telemetry.capture("mcp_connect", { + transport: "http", + result: "fail", + serverName: "acme-prod", + url: "https://mcp.acmecorp.internal", + error: "acmecorp token rejected", + }); + + const [event] = await events(); + expect(event?.properties.transport).toBe("http"); + expect(event?.properties.result).toBe("fail"); + expect(event?.properties.serverName).toBeUndefined(); + expect(event?.properties.url).toBeUndefined(); + expect(event?.properties.error).toBeUndefined(); + expect(await wire()).not.toContain("acmecorp"); +}); + +// --------------------------------------------------------------------------- +// mcp_oauth — exactly one enum outcome per browser attempt +// --------------------------------------------------------------------------- + +test("mcp_oauth reports completed, cancelled, and timeout and nothing else", async () => { + const { telemetry, wire, events } = harness(); + + captureMcpOauth(telemetry, "completed"); + captureMcpOauth(telemetry, "cancelled"); + captureMcpOauth(telemetry, "timeout"); + + const captured = await events(); + expect(captured.map((e) => e.properties.result)).toEqual([ + "completed", + "cancelled", + "timeout", + ]); + expect(captured.every((e) => e.event === "mcp_oauth")).toBe(true); + expect(await wire()).not.toContain("acmecorp"); +}); + +test("mcp_oauth treats explicit aborts as cancelled and everything else as timeout", () => { + const abortError = new Error("The operation was aborted"); + abortError.name = "AbortError"; + expect(classifyMcpOauthOutcome(abortError, false)).toBe("cancelled"); + expect(classifyMcpOauthOutcome(new Error("denied"), true)).toBe("cancelled"); + + const waitError = new Error( + "Authorization timed out waiting for the browser.", + ); + expect(classifyMcpOauthOutcome(waitError, false)).toBe("timeout"); + expect( + classifyMcpOauthOutcome( + new Error("Authorization failed: access_denied"), + false, + ), + ).toBe("timeout"); +}); + +test("mcp_oauth strips server, url, state, and error text", async () => { + const { telemetry, wire, events } = harness(); + telemetry.capture("mcp_oauth", { + result: "timeout", + serverName: "acme-prod", + authorizationUrl: "https://auth.acmecorp.internal/authorize", + state: "acme-state", + error: "acmecorp denied the request", + }); + + const [event] = await events(); + expect(event?.properties.result).toBe("timeout"); + expect(event?.properties.serverName).toBeUndefined(); + expect(event?.properties.authorizationUrl).toBeUndefined(); + expect(event?.properties.state).toBeUndefined(); + expect(event?.properties.error).toBeUndefined(); + expect(await wire()).not.toContain("acmecorp"); +}); + +// --------------------------------------------------------------------------- +// kill switch — the new product events respect disabled telemetry +// --------------------------------------------------------------------------- + +test("the new product events respect the env kill switch", async () => { + const { telemetry, wire } = harness({ CORBITS_TELEMETRY: "0" }); + expect(telemetry.enabled).toBe(false); + + captureAuthSuccess(telemetry, { connected: true, authProvider: "codex" }); + captureMcpConnect(telemetry, { transport: "http", result: "ok" }); + captureMcpOauth(telemetry, "completed"); + + expect(await wire()).toBe("[]"); +}); From e3d414c7fce0b1dbf494f3f4a85ab3413938cd72 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 28 Sep 2026 08:16:20 -0700 Subject: [PATCH 2/3] feat(telemetry): wire prod MCP callers and instrument the auth cap denial Prod MCP connects never received a telemetry handle, so mcp_oauth stayed silent outside tests; the browser-auth attempt cap also threw before a flow existed, skipping the instrumented recovery path. The toolset now forwards its telemetry to each connect and web-search uses the ambient singleton, and the cap throw emits a terminal timeout like every other non-abort browser-wait end. --- src/agent/tools.ts | 3 + src/mcp/client-auth-reauth-cap.test.ts | 44 +++++++++++- src/mcp/client-telemetry.test.ts | 94 ++++++++++++++++++++++++++ src/mcp/client.ts | 7 ++ src/tools/web-search.ts | 5 +- 5 files changed, 151 insertions(+), 2 deletions(-) create mode 100644 src/mcp/client-telemetry.test.ts diff --git a/src/agent/tools.ts b/src/agent/tools.ts index f76267638..0fa2f0913 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -1264,6 +1264,9 @@ export async function createAgentToolset( try { result = await connectMCPClient(config, { stderr: "ignore", + ...(args.telemetry !== undefined + ? { telemetry: args.telemetry } + : {}), ...(callbacks.interactiveAuth ? { onAuthURL: (name: string, url: string) => { diff --git a/src/mcp/client-auth-reauth-cap.test.ts b/src/mcp/client-auth-reauth-cap.test.ts index 984cac008..e26ed356d 100644 --- a/src/mcp/client-auth-reauth-cap.test.ts +++ b/src/mcp/client-auth-reauth-cap.test.ts @@ -7,6 +7,7 @@ import { test, } from "bun:test"; import { UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js"; +import type { Telemetry } from "../telemetry/index.js"; import { withMockedModule } from "../../tests/helpers/mock-module.js"; let finishAuthCalls = 0; @@ -263,7 +264,7 @@ const config = { url: "https://mcp.linear.app/mcp", }; -async function connectWithAuthPrompt(): Promise<{ +async function connectWithAuthPrompt(telemetry?: Telemetry): Promise<{ ok: boolean; error?: string; authPending?: boolean; @@ -273,6 +274,7 @@ async function connectWithAuthPrompt(): Promise<{ authURLCount += 1; authEvents.push("authURL"); }, + ...(telemetry !== undefined ? { telemetry } : {}), }); return result.ok ? { ok: true } @@ -864,6 +866,46 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(providerCreates).toBeGreaterThan(MAX_BROWSER_AUTH_ATTEMPTS); }); + test("a capped browser-auth denial emits one mcp_oauth timeout", async () => { + const oauthResults: unknown[] = []; + const telemetry: Telemetry = { + enabled: true, + installationId: "test", + capture: (event, properties) => { + if (event === "mcp_oauth") + oauthResults.push( + (properties as Record | undefined)?.result, + ); + }, + captureIntentional: () => false, + flush: async () => undefined, + discard: () => undefined, + }; + const connectWithTelemetry = () => connectWithAuthPrompt(telemetry); + connectFailuresLeft = Number.POSITIVE_INFINITY; + + for (let episode = 0; episode < MAX_BROWSER_AUTH_ATTEMPTS; episode += 1) { + const result = await connectWithTelemetry(); + expect(result.ok).toBe(false); + expect(result.error).toContain("finishAuth exploded"); + } + expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS); + // Each failed episode emits exactly one timeout through recovery. + expect(oauthResults).toEqual( + Array.from({ length: MAX_BROWSER_AUTH_ATTEMPTS }, () => "timeout"), + ); + + const capped = await connectWithTelemetry(); + expect(capped.ok).toBe(false); + expect(capped.error).toContain("retrying paused"); + expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS); + // The cap denial exits before a browser flow exists, but it still maps + // to exactly one terminal timeout rather than staying silent. + expect(oauthResults).toEqual( + Array.from({ length: MAX_BROWSER_AUTH_ATTEMPTS + 1 }, () => "timeout"), + ); + }); + test("successful interactive auth clears the cap so a later failure can prompt", async () => { finishAuthError = undefined; connectFailuresLeft = 1; diff --git a/src/mcp/client-telemetry.test.ts b/src/mcp/client-telemetry.test.ts new file mode 100644 index 000000000..00f6fb8a4 --- /dev/null +++ b/src/mcp/client-telemetry.test.ts @@ -0,0 +1,94 @@ +import { beforeEach, describe, expect, test } from "bun:test"; +import { NOOP_TELEMETRY, type Telemetry } from "../telemetry/index.js"; +import { withMockedModule } from "../../tests/helpers/mock-module.js"; + +let connectCalls = 0; +let failOnCall = -1; +const connectError = new Error("spawn ENOENT"); + +await withMockedModule( + import.meta.resolve("@modelcontextprotocol/sdk/client/index.js"), + (real: typeof import("@modelcontextprotocol/sdk/client/index.js")) => ({ + ...real, + Client: class { + async connect(): Promise { + connectCalls += 1; + if (connectCalls === failOnCall) throw connectError; + } + async listTools(): Promise<{ tools: [] }> { + return { tools: [] }; + } + async close(): Promise { + return undefined; + } + }, + }), +); + +const { connectMCPServers } = await import("./client.js"); + +function recordingTelemetry(events: Record[]): Telemetry { + return { + enabled: true, + installationId: "test", + capture: (event, properties) => { + if (event === "mcp_connect") events.push({ ...(properties ?? {}) }); + }, + captureIntentional: () => false, + flush: async () => undefined, + discard: () => undefined, + }; +} + +describe("connectMCPServers telemetry", () => { + beforeEach(() => { + connectCalls = 0; + failOnCall = -1; + }); + + test("a mixed multi-config run emits exactly one mcp_connect per config", async () => { + failOnCall = 2; + const events: Record[] = []; + const warnings: string[] = []; + const clients = await connectMCPServers( + [ + { name: "a", type: "stdio", command: "test-server-a" }, + { name: "b", type: "stdio", command: "test-server-b" }, + ], + (message) => warnings.push(message), + { telemetry: recordingTelemetry(events) }, + ); + try { + expect(clients).toHaveLength(1); + expect(warnings).toHaveLength(1); + expect(events).toHaveLength(2); + expect(events.map((event) => event.transport)).toEqual([ + "stdio", + "stdio", + ]); + expect(events.map((event) => event.result)).toEqual(["ok", "fail"]); + } finally { + for (const client of clients) await client.close(); + } + }); + + test("omitted telemetry stays silent on the dropping default", async () => { + // The omitted path routes capture calls into NOOP_TELEMETRY, whose + // capture drops everything — the gate lives in the Telemetry handle, + // not at the call site. This keeper pins that contract: the default + // handle is disabled, and connecting without telemetry resolves + // normally with nothing to flush or send. + expect(NOOP_TELEMETRY.enabled).toBe(false); + const warnings: string[] = []; + const clients = await connectMCPServers( + [{ name: "a", type: "stdio", command: "test-server-a" }], + (message) => warnings.push(message), + ); + try { + expect(clients).toHaveLength(1); + expect(warnings).toHaveLength(0); + } finally { + for (const client of clients) await client.close(); + } + }); +}); diff --git a/src/mcp/client.ts b/src/mcp/client.ts index 5cef865ea..456d3d46b 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -365,6 +365,13 @@ function gateRedirectToAuthorization(context: HTTPAuthContext): void { return promptEmitted; } catch (err) { unfreezePkce(coordinator); + // The cap throws before a browser flow exists, so this denial never + // reaches the instrumented recovery path below — emit here so the + // terminal denial maps to timeout like every other non-abort end. + captureMcpOauth( + context.telemetry, + classifyMcpOauthOutcome(err, coordinator.lifecycle.signal.aborted), + ); throw err; } }; diff --git a/src/tools/web-search.ts b/src/tools/web-search.ts index 73d2949e6..b6baa9c02 100644 --- a/src/tools/web-search.ts +++ b/src/tools/web-search.ts @@ -5,6 +5,7 @@ import type { ToolDefinition } from "@intx/types/runtime"; import { connectMCPServer, type MCPClient } from "../mcp/client.js"; import type { MCPServerConfig } from "../config/settings.js"; +import { getTelemetry } from "../telemetry/singleton.js"; import { EXA_MCP_URL } from "../mcp/exa.js"; export { EXA_MCP_URL } from "../mcp/exa.js"; @@ -93,7 +94,9 @@ async function getClient(provider: WebSearchProviderId): Promise { type: "http", url: endpointFor(provider), }; - const connecting = connectMCPServer(config).then((result) => { + const connecting = connectMCPServer(config, { + telemetry: getTelemetry(), + }).then((result) => { if (!result.ok) { clientCache.delete(provider); throw new Error(result.error); From f76498660b3520c828c2500ee370185de47d3e05 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 28 Sep 2026 08:22:32 -0700 Subject: [PATCH 3/3] feat(telemetry): emit mcp_connect on the singular MCP connect path --- src/mcp/client-telemetry.test.ts | 40 +++++++++++++++++++++++++++++--- src/mcp/client.ts | 31 ++++++++++++++----------- 2 files changed, 54 insertions(+), 17 deletions(-) diff --git a/src/mcp/client-telemetry.test.ts b/src/mcp/client-telemetry.test.ts index 00f6fb8a4..00569cb7a 100644 --- a/src/mcp/client-telemetry.test.ts +++ b/src/mcp/client-telemetry.test.ts @@ -25,7 +25,7 @@ await withMockedModule( }), ); -const { connectMCPServers } = await import("./client.js"); +const { connectMCPServers, connectMCPServer } = await import("./client.js"); function recordingTelemetry(events: Record[]): Telemetry { return { @@ -62,11 +62,17 @@ describe("connectMCPServers telemetry", () => { expect(clients).toHaveLength(1); expect(warnings).toHaveLength(1); expect(events).toHaveLength(2); - expect(events.map((event) => event.transport)).toEqual([ + // Order-insensitive: the batch fans out over Promise.all, and each + // singular connect captures its own event on completion, so arrival + // order follows settle order, not config order. + expect(events.map((event) => event.transport).sort()).toEqual([ "stdio", "stdio", ]); - expect(events.map((event) => event.result)).toEqual(["ok", "fail"]); + expect(events.map((event) => event.result).sort()).toEqual([ + "fail", + "ok", + ]); } finally { for (const client of clients) await client.close(); } @@ -92,3 +98,31 @@ describe("connectMCPServers telemetry", () => { } }); }); + +describe("connectMCPServer telemetry", () => { + beforeEach(() => { + connectCalls = 0; + failOnCall = -1; + }); + + test("singular ok + fail emits one mcp_connect each, exactly once per attempt", async () => { + const events: Record[] = []; + const telemetry = recordingTelemetry(events); + const ok = await connectMCPServer( + { name: "a", type: "stdio", command: "test-server-a" }, + { telemetry }, + ); + expect(ok.ok).toBe(true); + if (ok.ok) await ok.client.close(); + failOnCall = 2; + const failed = await connectMCPServer( + { name: "b", type: "stdio", command: "test-server-b" }, + { telemetry }, + ); + expect(failed.ok).toBe(false); + expect(connectCalls).toBe(2); + expect(events).toHaveLength(connectCalls); + expect(events.map((event) => event.transport)).toEqual(["stdio", "stdio"]); + expect(events.map((event) => event.result)).toEqual(["ok", "fail"]); + }); +}); diff --git a/src/mcp/client.ts b/src/mcp/client.ts index 456d3d46b..f8eb62a57 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -857,9 +857,19 @@ export async function connectMCPServer( config: ResolvedMCPServerConfig, options: MCPConnectOptions = {}, ): Promise { - return isHttpServer(config) + const telemetry = options.telemetry ?? NOOP_TELEMETRY; + const result = await (isHttpServer(config) ? connectHttp(config, options) - : connectStdio(config, options); + : connectStdio(config, options)); + // One enum-only event per outward connection outcome. Internal HTTP + // auth-recovery re-dials stay inside connectHttp, so this wrapper fires + // exactly once per connectMCPServer call. Server name, URL, and error + // text never leave the process. + captureMcpConnect(telemetry, { + transport: isHttpServer(config) ? "http" : "stdio", + result: classifyMcpConnectOutcome(result), + }); + return result; } export async function connectMCPServers( @@ -867,27 +877,20 @@ export async function connectMCPServers( onWarning: (message: string) => void, options: MCPConnectOptions = {}, ): Promise { - const telemetry = options.telemetry ?? NOOP_TELEMETRY; + // Telemetry flows through options into connectMCPServer, which captures + // one mcp_connect per config. const results = await Promise.all( configs.map((c) => connectMCPServer(c, options)), ); const clients: MCPClient[] = []; - for (let index = 0; index < results.length; index += 1) { - const result = results[index]; - if (result === undefined) continue; + for (const result of results) { if (result.ok) clients.push(result.client); else onWarning( `[mcp] Warning: failed to connect to MCP server "${result.serverName}": ${result.error}`, ); - // One enum-only event per server: the transport that was dialed and the - // outcome enum. Server name, URL, and error text never leave the process. - const config = configs[index]; - captureMcpConnect(telemetry, { - transport: - config !== undefined && isHttpServer(config) ? "http" : "stdio", - result: classifyMcpConnectOutcome(result), - }); + // No emit here: connectMCPServer already captured one enum-only + // mcp_connect per config. Emitting again would double-count. } return clients; }