diff --git a/src/api/worker-posthog.ts b/src/api/worker-posthog.ts index edcb2be49..04ba6eb41 100644 --- a/src/api/worker-posthog.ts +++ b/src/api/worker-posthog.ts @@ -99,16 +99,29 @@ export interface WorkerErrorRequestContext { /** Capture one exception from the hosted Worker path. Never throws -- a PostHog init/capture/flush failure * degrades to recording nothing, matching every other capture* function in this codebase's identical * best-effort guarantee. Resolves once the event has actually been flushed (or given up on), so the caller - * should schedule this via ctx.waitUntil rather than await it inline on the request's hot path. */ -export async function capturePostHogWorkerError(env: WorkerPostHogEnv, error: unknown, context: WorkerErrorRequestContext): Promise { + * should schedule this via ctx.waitUntil rather than await it inline on the request's hot path. + * + * `extraProperties` lets a non-HTTP caller (the MCP dispatch sink, #10037) attach its own grouping + * properties (e.g. `mcp_tool`/`error_code`) alongside the fixed `environment`/`request_path`/ + * `request_method` shape -- scrubbed the same way those are, then merged in. Omitting it leaves + * `createWorkerPostHogErrorMiddleware`'s HTTP callers unaffected. */ +export async function capturePostHogWorkerError( + env: WorkerPostHogEnv, + error: unknown, + context: WorkerErrorRequestContext, + extraProperties?: Record, +): Promise { try { const client = await buildClient(env); if (!client) return; const err = error instanceof Error ? error : new Error(String(error)); + const scrubbedExtra = extraProperties ? { ...extraProperties } : undefined; + if (scrubbedExtra) scrubRecord(scrubbedExtra, 0); client.captureException(err, WORKER_ERROR_DISTINCT_ID, { environment: trimmedOrUndefined(env.WORKER_POSTHOG_ENVIRONMENT) ?? "production", request_path: scrubString(context.path), request_method: context.method, + ...scrubbedExtra, }); await client.flush(); } catch { diff --git a/src/mcp/dispatch-telemetry-sink.ts b/src/mcp/dispatch-telemetry-sink.ts index f096edae2..44149f845 100644 --- a/src/mcp/dispatch-telemetry-sink.ts +++ b/src/mcp/dispatch-telemetry-sink.ts @@ -102,7 +102,10 @@ export function createDispatchTelemetrySink( if (!isWorkerPostHogConfigured(env)) return; // `mcp_tool` + `error_code` are the grouping properties: an exception dashboard broken down by // tool and cause is the thing an operator can act on, unlike a stack-only view. - defer(capturePostHogWorkerError(env, error, { path: `mcp.tool/${call.tool}`, method: call.errorCode ?? "unknown_error" })); + const errorCode = call.errorCode ?? "unknown_error"; + defer( + capturePostHogWorkerError(env, error, { path: `mcp.tool/${call.tool}`, method: errorCode }, { mcp_tool: call.tool, error_code: errorCode }), + ); }, // The registry is consulted per call rather than captured at construction so a self-host boot // that fills the slot after the first request still traces. diff --git a/test/unit/mcp-dispatch-telemetry-sink.test.ts b/test/unit/mcp-dispatch-telemetry-sink.test.ts index 50cff48df..b849bc8da 100644 --- a/test/unit/mcp-dispatch-telemetry-sink.test.ts +++ b/test/unit/mcp-dispatch-telemetry-sink.test.ts @@ -138,6 +138,60 @@ describe("MCP dispatch telemetry sink (#9525)", () => { }); }); +// The exception properties tests below mock posthog-node so they can read the properties the sink +// actually hands to captureException, instead of only observing whether the deferred promise resolves. +describe("MCP dispatch telemetry sink exception properties (#10037)", () => { + afterEach(() => { + vi.doUnmock("posthog-node"); + vi.resetModules(); + }); + + it("attaches mcp_tool and error_code to the captured exception, matching the stdio/miner sinks", async () => { + vi.resetModules(); + const captureException = vi.fn(); + const flush = vi.fn().mockResolvedValue(undefined); + vi.doMock("posthog-node", () => ({ + PostHog: vi.fn(function (this: { captureException: typeof captureException; flush: typeof flush }) { + this.captureException = captureException; + this.flush = flush; + }), + })); + const { createDispatchTelemetrySink: freshCreateDispatchTelemetrySink } = await import("../../src/mcp/dispatch-telemetry-sink"); + const deferred: Promise[] = []; + const sink = freshCreateDispatchTelemetrySink(env({ WORKER_POSTHOG_API_KEY: "phc_worker" }), (work) => deferred.push(work)); + const forbiddenCall: McpToolCallTelemetry = { ...call, ok: false, errorCode: "forbidden" }; + + sink.captureException(new Error("boom"), forbiddenCall); + expect(deferred).toHaveLength(1); + await deferred[0]; + + const properties = captureException.mock.calls.at(-1)?.[2] as Record; + expect(properties).toMatchObject({ mcp_tool: forbiddenCall.tool, error_code: "forbidden" }); + }); + + it("defaults error_code to unknown_error when the call carries none", async () => { + vi.resetModules(); + const captureException = vi.fn(); + const flush = vi.fn().mockResolvedValue(undefined); + vi.doMock("posthog-node", () => ({ + PostHog: vi.fn(function (this: { captureException: typeof captureException; flush: typeof flush }) { + this.captureException = captureException; + this.flush = flush; + }), + })); + const { createDispatchTelemetrySink: freshCreateDispatchTelemetrySink } = await import("../../src/mcp/dispatch-telemetry-sink"); + const deferred: Promise[] = []; + const sink = freshCreateDispatchTelemetrySink(env({ WORKER_POSTHOG_API_KEY: "phc_worker" }), (work) => deferred.push(work)); + const noCodeCall: McpToolCallTelemetry = { ...call, ok: false }; + + sink.captureException(new Error("boom"), noCodeCall); + await deferred[0]; + + const properties = captureException.mock.calls.at(-1)?.[2] as Record; + expect(properties).toMatchObject({ mcp_tool: noCodeCall.tool, error_code: "unknown_error" }); + }); +}); + 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"); diff --git a/test/unit/worker-posthog.test.ts b/test/unit/worker-posthog.test.ts index 04c55d6a6..cc71b943d 100644 --- a/test/unit/worker-posthog.test.ts +++ b/test/unit/worker-posthog.test.ts @@ -149,6 +149,30 @@ describe("capturePostHogWorkerError", () => { expect(flushed).toBe(true); }); + it("merges extraProperties into the captured exception alongside the fixed request shape (#10037)", async () => { + await capturePostHogWorkerError({ WORKER_POSTHOG_API_KEY: "phc_test" } as WorkerPostHogEnv, new Error("boom"), { path: "mcp.tool/loopover_x", method: "forbidden" }, { + mcp_tool: "loopover_x", + error_code: "forbidden", + }); + const properties = mocks.captureException.mock.calls.at(-1)?.[2] as Record; + expect(properties).toMatchObject({ + environment: "production", + request_path: "mcp.tool/loopover_x", + request_method: "forbidden", + mcp_tool: "loopover_x", + error_code: "forbidden", + }); + }); + + it("scrubs a secret-shaped extraProperties value the same way request_path is scrubbed (#10037)", async () => { + await capturePostHogWorkerError({ WORKER_POSTHOG_API_KEY: "phc_test" } as WorkerPostHogEnv, new Error("boom"), { path: "/x", method: "GET" }, { + leaked_token: `${"github" + "_pat_"}${"a".repeat(24)}`, + }); + const properties = mocks.captureException.mock.calls.at(-1)?.[2] as Record; + expect(properties.leaked_token).not.toContain("github_pat_"); + expect(properties.leaked_token).toContain("[redacted]"); + }); + it("never throws when the PostHog client construction fails", async () => { mocks.PostHog.mockImplementationOnce(() => { throw new Error("client construction failed"); @@ -223,6 +247,16 @@ describe("createWorkerPostHogErrorMiddleware", () => { expect((properties as Record).request_method).toBe("GET"); }); + it("still carries only request_path/request_method, never mcp_tool, when no extraProperties argument is passed (#10037)", async () => { + const { app, executionCtx, getWaited } = buildTestApp(); + const res = await app.fetch(new Request("https://loopover.test/boom"), { WORKER_POSTHOG_API_KEY: "phc_test" } as WorkerPostHogEnv, executionCtx); + expect(res.status).toBe(500); + await getWaited(); + const properties = mocks.captureException.mock.calls.at(-1)?.[2] as Record; + expect(properties).toMatchObject({ request_path: "/boom", request_method: "GET" }); + expect(properties).not.toHaveProperty("mcp_tool"); + }); + it("falls back to a no-op executionCtx when c.executionCtx throws (self-host calling the same Worker fetch handler outside a real isolate)", async () => { const app = new Hono<{ Bindings: WorkerPostHogEnv }>(); app.use(createWorkerPostHogErrorMiddleware());