test(mcp): close the remaining telemetry coverage gaps in the three package sinks

codecov/patch's second pass flagged what the first round missed: the stdio
dispatch emitter's no-contract and no-error branches, the miner's thin
capture send, and the sink-injected path through LoopoverMcp's constructor
-- every existing test builds that server without a sink, so only the
NOOP fallback side of that '??' was ever taken.

Also makes withMinerToolErrorHandling's toolName REQUIRED. It was optional so
an un-threaded caller would still compile; every registration passes it, and
leaving it optional made 'instrumented' a property of each call site rather
than of the wrapper -- plus an unreachable branch on both arms.

Author: JSONbored

packages/loopover-miner/bin/loopover-miner-mcp.ts

@@ -114,19 +114,19 @@ function isTextOverride<T extends object>(value: MinerToolRun<T>): value is { st
 async function withMinerToolErrorHandling<T extends object>(
   run: () => Promise<MinerToolRun<T>> | MinerToolRun<T>,
   // #9525: the tool name, so this same wrapper doubles as the miner's dispatch-telemetry
-  // chokepoint. Optional so an existing caller that has not been threaded through yet still
-  // compiles; every registration below passes it.
-  toolName?: string,
+  // chokepoint. REQUIRED -- every registration passes it, and leaving it optional would have made
+  // "instrumented" a property of each call site rather than of the wrapper.
+  toolName: string,
 ): Promise<{ content: [{ type: "text"; text: string }]; structuredContent: Record<string, unknown>; isError?: true }> {
   const startedAt = Date.now();
   try {
     const result = await run();
     const payload = isTextOverride(result) ? minerToolResult(result.structured, result.text) : minerToolResult(result);
-    if (toolName) recordMinerDispatchTelemetry({ tool: toolName, ok: true, durationMs: Date.now() - startedAt, result: payload.structuredContent });
+    recordMinerDispatchTelemetry({ tool: toolName, ok: true, durationMs: Date.now() - startedAt, result: payload.structuredContent });
     return payload;
   } catch (error) {
     const data = { error: { code: toolErrorCode(error), message: error instanceof Error ? error.message : String(error) } };
-    if (toolName) recordMinerDispatchTelemetry({ tool: toolName, ok: false, durationMs: Date.now() - startedAt, error });
+    recordMinerDispatchTelemetry({ tool: toolName, ok: false, durationMs: Date.now() - startedAt, error });
     return { ...minerToolResult(data), isError: true };
   }
 }

test/unit/mcp-dispatch-telemetry-sink.test.ts

@@ -119,3 +119,33 @@ describe("MCP dispatch telemetry sink (#9525)", () => {
     expect(injectedCalls).toBe(1);
   });
 });
+
+describe("LoopoverMcp telemetry-sink injection (#9525)", () => {
+  it("routes a real tool call through the injected sink", async () => {
+    const { Client } = await import("@modelcontextprotocol/sdk/client/index.js");
+    const { InMemoryTransport } = await import("@modelcontextprotocol/sdk/inMemory.js");
+    const { LoopoverMcp } = await import("../../src/mcp/server");
+    const { createTestEnv } = await import("../helpers/d1");
+
+    const recorded: McpToolCallTelemetry[] = [];
+    const sink = {
+      recordToolCall: (entry: McpToolCallTelemetry) => recorded.push(entry),
+      captureException: () => undefined,
+      withSpan: async <T>(_name: string, _attributes: Record<string, unknown>, fn: () => Promise<T>) => fn(),
+    };
+
+    const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();
+    const client = new Client({ name: "sink-injection-test", version: "0.0.0" });
+    await Promise.all([new LoopoverMcp(createTestEnv(), undefined, sink).createServer().connect(serverTransport), client.connect(clientTransport)]);
+    try {
+      await client.callTool({ name: "loopover_get_repo_context", arguments: { owner: "acme", repo: "widgets" } });
+    } finally {
+      await client.close().catch(() => undefined);
+    }
+
+    // The chokepoint is the register wrapper, so this proves the injection reaches every tool
+    // rather than just the one under test -- there is only one wrapper.
+    expect(recorded).toHaveLength(1);
+    expect(recorded[0]).toMatchObject({ tool: "loopover_get_repo_context", category: "maintainer", surface: "remote" });
+  }, 30_000);
+});

test/unit/mcp-local-telemetry.test.ts

@@ -6,6 +6,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 const h = vi.hoisted(() => ({
   constructSpy: vi.fn(),
   captureSpy: vi.fn(),
+  captureExceptionSpy: vi.fn(),
   flushSpy: vi.fn(),
   state: {
     throwOnConstruct: false,
@@ -26,6 +27,9 @@ vi.mock("posthog-node", () => ({
       h.captureSpy(message);
       if (h.state.throwOnCapture) throw new Error("posthog capture failed");
     }
+    captureException(error: unknown, distinctId: unknown, properties: unknown): void {
+      h.captureExceptionSpy(error, distinctId, properties);
+    }
     async flush(): Promise<void> {
       h.flushSpy();
       if (h.state.throwOnFlush) throw new Error("posthog flush failed");
@@ -34,7 +38,7 @@ vi.mock("posthog-node", () => ({
   },
 }));
 
-const { recordMcpToolCall, recordStdioToolTelemetry, wrapStdioToolHandler } = await import(
+const { recordMcpToolCall, recordStdioDispatchTelemetry, recordStdioToolTelemetry, wrapStdioToolHandler } = await import(
   "../../packages/loopover-mcp/lib/telemetry"
 );
 
@@ -47,6 +51,7 @@ describe("recordMcpToolCall (local MCP wrapper, #6236)", () => {
   beforeEach(() => {
     h.constructSpy.mockClear();
     h.captureSpy.mockClear();
+    h.captureExceptionSpy.mockClear();
     h.flushSpy.mockClear();
     h.state.throwOnConstruct = false;
     h.state.throwOnCapture = false;
@@ -229,6 +234,7 @@ describe("recordStdioToolTelemetry / wrapStdioToolHandler (#8690)", () => {
   beforeEach(() => {
     h.constructSpy.mockClear();
     h.captureSpy.mockClear();
+    h.captureExceptionSpy.mockClear();
     h.flushSpy.mockClear();
     h.state.throwOnConstruct = false;
     h.state.throwOnCapture = false;
@@ -315,9 +321,44 @@ describe("recordStdioToolTelemetry / wrapStdioToolHandler (#8690)", () => {
       throw new Error("handler boom");
     });
     await expect(wrapped()).rejects.toThrow("handler boom");
-    expect(h.flushSpy).toHaveBeenCalledTimes(1);
+    // Two flushes since #9525, same as the success path: the legacy event, then the shared pair.
+    expect(h.flushSpy).toHaveBeenCalledTimes(2);
     const message = h.captureSpy.mock.calls[0]![0] as CapturedMessage;
     expect(message.properties).toMatchObject({ tool: "loopover_demo", ok: false });
+    // The throw is additionally captured as an exception, grouped by tool and closed error code.
+    expect(h.captureExceptionSpy).toHaveBeenCalledOnce();
+    expect(h.captureExceptionSpy.mock.calls[0]![2]).toMatchObject({ mcp_tool: "loopover_demo" });
+  });
+
+  // #9525: the shared dispatch pair, driven directly so the branches the wrapper cannot reach are
+  // covered -- a tool with no contract entry, and a failed call that carries no error value.
+  it("recordStdioDispatchTelemetry sends nothing unless BOTH gates are open", async () => {
+    vi.stubEnv("LOOPOVER_MCP_POSTHOG_API_KEY", "phc_test");
+    await recordStdioDispatchTelemetry(false, { tool: "loopover_lint_pr_text", ok: true, durationMs: 1 });
+    expect(h.captureSpy).not.toHaveBeenCalled();
+
+    vi.stubEnv("LOOPOVER_MCP_POSTHOG_API_KEY", "");
+    await recordStdioDispatchTelemetry(true, { tool: "loopover_lint_pr_text", ok: true, durationMs: 1 });
+    expect(h.captureSpy).not.toHaveBeenCalled();
+  });
+
+  it("recordStdioDispatchTelemetry falls back to the unknown category for a tool with no contract entry", async () => {
+    vi.stubEnv("LOOPOVER_MCP_POSTHOG_API_KEY", "phc_test");
+    await recordStdioDispatchTelemetry(true, { tool: "loopover_not_in_the_registry", ok: true, durationMs: 2, args: { a: 1 } });
+    const usage = h.captureSpy.mock.calls.map((entry) => entry[0] as CapturedMessage).find((message) => message.event === "usage_event")!;
+    expect(usage.properties).toMatchObject({ category: "unknown", surface: "stdio" });
+    // No contract means no way to know the payload is safe, so it is withheld.
+    const toolCall = h.captureSpy.mock.calls.map((entry) => entry[0] as CapturedMessage).find((message) => message.event === "$mcp_tool_call")!;
+    expect(toolCall.properties).toMatchObject({ payloads_excluded: true });
+  });
+
+  it("recordStdioDispatchTelemetry captures an exception only when the failure carried one", async () => {
+    vi.stubEnv("LOOPOVER_MCP_POSTHOG_API_KEY", "phc_test");
+    await recordStdioDispatchTelemetry(true, { tool: "loopover_lint_pr_text", ok: false, durationMs: 2 });
+    expect(h.captureExceptionSpy).not.toHaveBeenCalled();
+
+    await recordStdioDispatchTelemetry(true, { tool: "loopover_lint_pr_text", ok: false, durationMs: 2, error: "a bare string" });
+    expect(h.captureExceptionSpy).toHaveBeenCalledOnce();
   });
 
   it("wrapStdioToolHandler is a no-op for PostHog when telemetry is disabled", async () => {

test/unit/miner-posthog.test.ts

@@ -18,6 +18,7 @@ vi.mock("posthog-node", () => ({ PostHog: posthogMock.PostHog }));
 
 import {
   captureMinerPostHogAiGeneration,
+  captureMinerPostHogEvent,
   captureMinerPostHogError,
   captureMinerPostHogErrorAndFlush,
   flushMinerPostHog,
@@ -246,3 +247,39 @@ describe("loopover-miner opt-in PostHog (#8292, epic #8286)", () => {
     });
   });
 });
+
+// #9525: the thin send the MCP dispatch chokepoint composes its events for. The properties come
+// from @loopover/contract so all three servers emit one shape; what is asserted here is this
+// function's own contract -- gated, scrubbed, and never throwing.
+describe("captureMinerPostHogEvent (#9525)", () => {
+  it("sends nothing when PostHog was never initialized", () => {
+    captureMinerPostHogEvent("usage_event", { tool: "loopover_miner_ping" });
+    expect(posthogMock.capture).not.toHaveBeenCalled();
+  });
+
+  it("sends the event anonymously, with geoip disabled, once initialized", async () => {
+    await initMinerPostHog({ LOOPOVER_MINER_POSTHOG_API_KEY: "phc_test_key" });
+    captureMinerPostHogEvent("usage_event", { tool: "loopover_miner_ping", ok: true });
+    expect(posthogMock.capture).toHaveBeenCalledOnce();
+    expect(posthogMock.capture.mock.calls[0]![0]).toMatchObject({
+      distinctId: "loopover-miner",
+      event: "usage_event",
+      properties: { tool: "loopover_miner_ping", ok: true },
+      disableGeoip: true,
+    });
+  });
+
+  it("scrubs a secret-shaped property key on the way out", async () => {
+    await initMinerPostHog({ LOOPOVER_MINER_POSTHOG_API_KEY: "phc_test_key" });
+    captureMinerPostHogEvent("usage_event", { tool: "t", githubToken: "ghp_realtokenvaluehere" });
+    expect(posthogMock.capture.mock.calls[0]![0].properties.githubToken).toBe("[redacted]");
+  });
+
+  it("never throws when the SDK's capture does", async () => {
+    await initMinerPostHog({ LOOPOVER_MINER_POSTHOG_API_KEY: "phc_test_key" });
+    posthogMock.capture.mockImplementationOnce(() => {
+      throw new Error("capture failed");
+    });
+    expect(() => captureMinerPostHogEvent("usage_event", { tool: "t" })).not.toThrow();
+  });
+});