From dedcde4ca0e0d005b770e2c5f88a088273599f7a Mon Sep 17 00:00:00 2001 From: luciferlive112116 <291889058+luciferlive112116@users.noreply.github.com> Date: Thu, 16 Jul 2026 15:19:22 +0800 Subject: [PATCH] feat(mcp): record local stdio tool calls at the dispatch chokepoint The local telemetry wrapper landed with the opt-in flag and the enable/disable/status commands, but nothing ever called it: a user could opt in and still have zero usage recorded. Wire it into the stdio tool-dispatch path. registerStdioTool is already the single point every one of the registered tools passes through, so no new chokepoint was needed -- it now wraps each handler to time the call and record exactly once, on both the return and the throw path. Fields match the remote side: tool name, callerType "local", ok, and coarse duration. Nothing else, and no behaviour change to any tool's response. `ok` mirrors the remote's caller-visible outcome (`response.status < 400`) rather than just "did it throw": a handler returning an error result is not a success. No handler does that today -- they signal failure by throwing, and the SDK maps it -- so this only matters if one ever starts. The opt-in read lives at module scope, not inside registerStdioTool: that function's second parameter is the TOOL's config and shadows the CLI's own `config`, so reading the flag inside it would have silently seen the wrong object and never fired. Telemetry failures stay invisible to callers: the chokepoint keeps a defensive try/catch over recordMcpToolCall's own never-throw guarantee, mirroring recordMcpToolTelemetry on the remote. Tests run the real stdio server as a subprocess against a local stand-in PostHog endpoint rather than mocking the SDK, because the opt-in guarantee is worth verifying on the wire and an in-process mock cannot reach a subprocess anyway. They cover: nothing sent by default even with an API key present, exactly one allowlisted event per call once opted in, one event per invocation rather than per session, disable returning to silence, and a failing tool recorded as ok=false while still failing identically for the caller. Closes #6238 --- packages/loopover-mcp/bin/loopover-mcp.js | 34 +++- .../mcp-local-telemetry-chokepoint.test.ts | 189 ++++++++++++++++++ 2 files changed, 222 insertions(+), 1 deletion(-) create mode 100644 test/unit/mcp-local-telemetry-chokepoint.test.ts diff --git a/packages/loopover-mcp/bin/loopover-mcp.js b/packages/loopover-mcp/bin/loopover-mcp.js index c541545153..0f335b15ce 100755 --- a/packages/loopover-mcp/bin/loopover-mcp.js +++ b/packages/loopover-mcp/bin/loopover-mcp.js @@ -24,6 +24,9 @@ import { buildBranchAnalysisPayload, collectLocalDiff, collectLocalBranchMetadat import { formatTable } from "../lib/format-table.js"; import { argsWantJson, describeCliError, reportCliFailure } from "../lib/cli-error.js"; import { redactKnownLocalPaths, redactLocalPath } from "../lib/redact-local-path.js"; +// Aliased: this file's own recordStdioToolTelemetry is the chokepoint that calls it, and the two names sitting +// side by side unaliased would read as the same function (#6238). +import { recordMcpToolCall as recordLocalMcpToolCall } from "../lib/telemetry.js"; // Read name/version from this package's own package.json (always present in any install -- // global, npx, or local -- npm ships it regardless of the "files" allowlist) instead of hand-synced @@ -901,8 +904,37 @@ const server = new McpServer({ // #4777: register a stdio tool under its loopover_ name. Thin wrapper kept so all 37 call sites // stay uniform with the rest of this file's registration style. +// Single chokepoint for the #6228 PostHog tool-call telemetry (#6238): every registerStdioTool-registered tool +// routes through here exactly once per invocation, whether it returns or throws. Pure observability -- a +// telemetry failure must never reach the tool caller, so this keeps a defensive try/catch on top of +// recordMcpToolCall's own never-throw guarantee (#6236), mirroring recordMcpToolTelemetry on the remote side +// (#6237). +// +// Reads the opt-in flag HERE, at module scope, on purpose: registerStdioTool's second parameter is the TOOL's +// config and shadows the module-level `config` this resolves from, so a read inside that function would silently +// see the wrong object and never fire. +function recordStdioToolTelemetry(tool, ok, durationMs) { + try { + recordLocalMcpToolCall({ telemetryEnabled: telemetryState().enabled }, { tool, callerType: "local", ok, durationMs }); + } catch { + // Telemetry must never affect the tool response (#6238). + } +} + function registerStdioTool(name, config, handler) { - server.registerTool(name, config, handler); + server.registerTool(name, config, async (...args) => { + const startedAt = Date.now(); + try { + const result = await handler(...args); + // Mirror the remote's caller-visible outcome (`response.status < 400`): a handler that reports failure by + // returning an error result is not a success, even though it never threw. + recordStdioToolTelemetry(name, result?.isError !== true, Date.now() - startedAt); + return result; + } catch (error) { + recordStdioToolTelemetry(name, false, Date.now() - startedAt); + throw error; + } + }); } registerStdioTool( diff --git a/test/unit/mcp-local-telemetry-chokepoint.test.ts b/test/unit/mcp-local-telemetry-chokepoint.test.ts new file mode 100644 index 0000000000..8739412b57 --- /dev/null +++ b/test/unit/mcp-local-telemetry-chokepoint.test.ts @@ -0,0 +1,189 @@ +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; +import { execFileSync } from "node:child_process"; +import { createServer, type IncomingMessage, type Server } from "node:http"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { gunzipSync } from "node:zlib"; +import { afterEach, describe, expect, it } from "vitest"; + +// #6238: wires the #6236 local telemetry wrapper into the stdio tool-dispatch chokepoint. The opt-in guarantee +// is the whole point of this surface, so these tests do not mock the PostHog SDK -- the stdio server runs as a +// real subprocess, where an in-process vi.mock could not reach it anyway. Instead they point the SDK at a local +// recorder via LOOPOVER_MCP_POSTHOG_HOST and assert what actually leaves the process. Default-off is therefore +// verified, not documented. +const bin = join(process.cwd(), "packages/loopover-mcp/bin/loopover-mcp.js"); + +type PostHogEvent = { event: string; distinct_id?: string; properties?: Record }; + +let client: Client | null = null; +let configDir: string | null = null; +let recorder: Server | null = null; +let received: PostHogEvent[] = []; + +/** A stand-in PostHog ingestion endpoint: accepts anything, records every event body it is sent. + * posthog-node POSTs gzipped JSON to `/batch/`, so the body is inflated before parsing -- reading it as plain + * UTF-8 yields garbage that silently parses to nothing, which would make every "sent nothing" assertion below + * pass no matter what the CLI did. */ +async function startRecorder(): Promise { + received = []; + recorder = createServer((request: IncomingMessage, response) => { + const chunks: Buffer[] = []; + request.on("data", (chunk: Buffer) => chunks.push(chunk)); + request.on("end", () => { + const raw = Buffer.concat(chunks); + const text = request.headers["content-encoding"] === "gzip" ? gunzipSync(raw).toString("utf8") : raw.toString("utf8"); + const body = JSON.parse(text) as { batch?: PostHogEvent[] } & PostHogEvent; + // posthog-node posts either a single event or a `batch` array depending on the flush path. + if (Array.isArray(body.batch)) received.push(...body.batch); + else if (body.event) received.push(body); + response.statusCode = 200; + response.end(JSON.stringify({ status: 1 })); + }); + }); + await new Promise((resolve) => recorder!.listen(0, "127.0.0.1", resolve)); + const address = recorder!.address(); + if (typeof address === "string" || !address) throw new Error("recorder failed to bind"); + return `http://127.0.0.1:${address.port}`; +} + +/** Wait until `predicate` holds or the window elapses -- the SDK flushes on its own turn, not ours. */ +async function waitFor(predicate: () => boolean, ms = 3000): Promise { + const deadline = Date.now() + ms; + while (Date.now() < deadline) { + if (predicate()) return; + await new Promise((resolve) => setTimeout(resolve, 50)); + } +} + +function cliEnv(host: string, extra: Record = {}) { + return { + ...process.env, + LOOPOVER_CONFIG_DIR: configDir!, + LOOPOVER_MCP_POSTHOG_API_KEY: "phc-test-key", + LOOPOVER_MCP_POSTHOG_HOST: host, + LOOPOVER_API_TIMEOUT_MS: "1000", + ...extra, + } as NodeJS.ProcessEnv; +} + +async function connect(host: string) { + const transport = new StdioClientTransport({ command: "node", args: [bin, "--stdio"], env: cliEnv(host) as Record }); + client = new Client({ name: "telemetry-chokepoint-test", version: "0.0.1" }); + await client.connect(transport); +} + +/** A tool with no API round-trip, so the only thing on the wire is telemetry. */ +async function callLintPrText() { + return client!.callTool({ + name: "loopover_lint_pr_text", + arguments: { commitMessages: ["feat(mcp): add telemetry chokepoint"], prBody: "Wires telemetry. Validated with npm test.", linkedIssue: 6238 }, + }); +} + +afterEach(async () => { + await client?.close().catch(() => undefined); + client = null; + if (recorder) await new Promise((resolve) => recorder!.close(() => resolve())); + recorder = null; + if (configDir) rmSync(configDir, { recursive: true, force: true }); + configDir = null; +}); + +describe("loopover-mcp local telemetry chokepoint (#6238)", () => { + it("sends NOTHING by default, even with an API key configured, and the tool still works", async () => { + configDir = mkdtempSync(join(tmpdir(), "loopover-telemetry-off-")); + const host = await startRecorder(); + await connect(host); + + const result = await callLintPrText(); + expect(result.isError).toBeFalsy(); + + // Give a would-be event every chance to arrive before declaring silence. + await waitFor(() => received.length > 0); + expect(received).toEqual([]); + }, 45_000); + + it("records exactly one allowlisted event per call once the user opts in", async () => { + configDir = mkdtempSync(join(tmpdir(), "loopover-telemetry-on-")); + const host = await startRecorder(); + // Opt in the way a user does -- through the real command, not by hand-writing the config file. + execFileSync("node", [bin, "telemetry", "enable"], { env: cliEnv(host), stdio: "ignore" }); + await connect(host); + + await callLintPrText(); + await waitFor(() => received.length > 0); + + expect(received).toHaveLength(1); + const event = received[0]!; + expect(event.event).toBe("mcp_tool_call"); + expect(event.properties?.tool).toBe("loopover_lint_pr_text"); + expect(event.properties?.caller_type).toBe("local"); + expect(event.properties?.ok).toBe(true); + expect(typeof event.properties?.duration_ms).toBe("number"); + + // The allowlist is exhaustive for everything LoopOver puts on the event. What remains on the wire is the + // PostHog SDK's own `$`-prefixed library metadata ($lib, $lib_version, $is_server, $geoip_disable) -- vendor + // provenance, not anything about the user or their call. Asserted as two separate sets rather than one flat + // list, so a future field of OURS can never hide among the vendor's. + const properties = Object.keys(event.properties ?? {}); + expect(properties.filter((key) => !key.startsWith("$")).sort()).toEqual(["caller_type", "duration_ms", "ok", "tool"]); + expect(properties.filter((key) => key.startsWith("$")).sort()).toEqual(["$geoip_disable", "$is_server", "$lib", "$lib_version"]); + expect(event.properties?.$geoip_disable).toBe(true); + // Anonymous by construction: one shared handle, never a per-user id. + expect(event.distinct_id).toBe("loopover-mcp"); + // The call's actual content never leaves: not the PR body, not the commit message. + expect(JSON.stringify(event)).not.toContain("Wires telemetry"); + expect(JSON.stringify(event)).not.toContain("feat(mcp): add telemetry chokepoint"); + }, 45_000); + + it("records one event per invocation, not one per session", async () => { + configDir = mkdtempSync(join(tmpdir(), "loopover-telemetry-count-")); + const host = await startRecorder(); + execFileSync("node", [bin, "telemetry", "enable"], { env: cliEnv(host), stdio: "ignore" }); + await connect(host); + + await callLintPrText(); + await callLintPrText(); + await waitFor(() => received.length >= 2); + + expect(received).toHaveLength(2); + expect(received.every((event) => event.properties?.tool === "loopover_lint_pr_text")).toBe(true); + }, 45_000); + + it("`telemetry disable` returns the server to sending nothing", async () => { + configDir = mkdtempSync(join(tmpdir(), "loopover-telemetry-toggle-")); + const host = await startRecorder(); + execFileSync("node", [bin, "telemetry", "enable"], { env: cliEnv(host), stdio: "ignore" }); + execFileSync("node", [bin, "telemetry", "disable"], { env: cliEnv(host), stdio: "ignore" }); + await connect(host); + + const result = await callLintPrText(); + expect(result.isError).toBeFalsy(); + await waitFor(() => received.length > 0); + expect(received).toEqual([]); + }, 45_000); + + it("a failing tool is recorded as ok=false, and still fails the same way for the caller", async () => { + configDir = mkdtempSync(join(tmpdir(), "loopover-telemetry-fail-")); + const host = await startRecorder(); + execFileSync("node", [bin, "telemetry", "enable"], { env: cliEnv(host), stdio: "ignore" }); + // No API server on this port, so an API-backed tool's fetch fails -- the handler throws. + const transport = new StdioClientTransport({ + command: "node", + args: [bin, "--stdio"], + env: cliEnv(host, { LOOPOVER_API_URL: "http://127.0.0.1:1", LOOPOVER_TOKEN: "session-token" }) as Record, + }); + client = new Client({ name: "telemetry-fail-test", version: "0.0.1" }); + await client.connect(transport); + + const result = await client.callTool({ name: "loopover_get_repo_context", arguments: { owner: "owner", repo: "repo" } }); + expect(result.isError).toBe(true); + + await waitFor(() => received.length > 0); + expect(received).toHaveLength(1); + expect(received[0]!.properties?.tool).toBe("loopover_get_repo_context"); + expect(received[0]!.properties?.ok).toBe(false); + }, 45_000); +});