diff --git a/.github/workflows/ui-deploy.yml b/.github/workflows/ui-deploy.yml index 64258e69c4..fed6d18beb 100644 --- a/.github/workflows/ui-deploy.yml +++ b/.github/workflows/ui-deploy.yml @@ -37,4 +37,14 @@ jobs: - name: Validate frontend env: VITE_LOOPOVER_API_ORIGIN: https://api.loopover.ai + # PostHog web analytics (#8293). Inline rather than a repo secret, for the same reason + # VITE_LOOPOVER_API_ORIGIN above is: this is a write-only ingest token that is embedded in + # the client bundle and therefore public by design -- a secret would add ceremony while + # protecting nothing. + # + # It MUST be set here, at build time: Vite substitutes `import.meta.env.VITE_*` when the + # bundle is compiled, so setting this as a runtime Worker var/secret instead would leave + # the shipped bundle with no token and turn every capture into a silent no-op with zero + # errors anywhere. + VITE_POSTHOG_PROJECT_TOKEN: phc_DmhFq8vjWoXGzd27EFp3sCFL4ugqXftaXhdy4E9noUZU run: npm run ui:openapi:check && npm run ui:lint && npm run ui:typecheck && npm --workspace @loopover/ui run build diff --git a/.prettierignore b/.prettierignore new file mode 100644 index 0000000000..de6faaf421 --- /dev/null +++ b/.prettierignore @@ -0,0 +1,65 @@ +# Root prettier ignore -- DEFAULT DENY. +# +# Prettier governs only three workspaces in this repo (apps/loopover-ui, +# apps/loopover-miner-ui, packages/loopover-ui-kit). Each has its own .prettierrc +# and its own `format:check`, and `npm run ui:lint` -- which `test:ci` runs -- is +# what enforces them. Everything else (src/**, test/**, scripts/**, the non-UI +# packages, control-plane/**, review-enrichment/**) is hand-formatted at a much +# wider effective line length and is NOT prettier input. +# +# Why this file exists: prettier resolves config by walking UP from each file, so +# before this, a `prettier --write .` or a `prettier --write ` run +# from the repo root found NO config, silently fell back to prettier's own +# defaults (printWidth 80), and rewrapped thousands of lines of backend source +# that nothing in CI had ever asked to be formatted. That produced enormous, +# unreviewable diffs in otherwise small PRs -- repeatedly. Editors with +# "format on save" hit the same trap. +# +# Default-deny rather than an explicit list of the excluded trees: a list goes +# stale the moment a new top-level directory or package is added, and it fails +# OPEN (the new tree gets mangled). This fails CLOSED -- a new directory is +# ignored until someone deliberately un-ignores it below. +/* + +# The three prettier-governed workspaces, re-admitted explicitly. Un-ignoring a +# path requires un-ignoring each parent directory first, hence the paired lines. +!/apps/ +/apps/* +!/apps/loopover-ui/ +!/apps/loopover-miner-ui/ + +!/packages/ +/packages/* +!/packages/loopover-ui-kit/ + +# Never formatter input, even inside a governed workspace. +# +# These MUST be repeated here rather than inherited: prettier reads only ONE +# ignore file per run -- the one for the directory it was invoked from -- so the +# workspaces' own .prettierignore files do not apply to a root-level run. Without +# these, `prettier --write .` from the root reformats exactly the files each +# workspace deliberately exempts. Keep in sync with: +# apps/loopover-ui/.prettierignore +# apps/loopover-miner-ui/.prettierignore +# packages/loopover-ui-kit/.prettierignore +**/node_modules/ +**/dist/ +**/coverage/ +**/CHANGELOG.md +**/.output/ +**/.vinxi/ +**/package-lock.json +**/bun.lock +**/pnpm-lock.yaml + +# Generated -- reformatting these makes their own drift checks fail against a +# file nobody edited (`ui:openapi:check`, `mcp:tool-reference:check`). +**/routeTree.gen.ts +apps/loopover-ui/.source/ +apps/loopover-ui/public/openapi.json +apps/loopover-ui/src/lib/mcp-tool-reference.ts + +# Deliberately formatter-free: prettier's mdx pass rewrites the template-literal +# code inside attributes and destroys the embedded +# YAML/compose indentation (#8182 fallout, repaired once already). +apps/loopover-ui/content/docs/ diff --git a/apps/loopover-ui/package.json b/apps/loopover-ui/package.json index 098ce30ec3..a13a53c695 100644 --- a/apps/loopover-ui/package.json +++ b/apps/loopover-ui/package.json @@ -68,6 +68,7 @@ "input-otp": "^1.4.2", "lucide-react": "^0.577.0", "motion": "^12.42.2", + "posthog-js": "^1.409.3", "react": "^19.2.8", "react-day-picker": "^9.14.0", "react-dom": "^19.2.8", diff --git a/apps/loopover-ui/src/lib/analytics-proxy.test.ts b/apps/loopover-ui/src/lib/analytics-proxy.test.ts index 1b3fb24c2c..9151285988 100644 --- a/apps/loopover-ui/src/lib/analytics-proxy.test.ts +++ b/apps/loopover-ui/src/lib/analytics-proxy.test.ts @@ -1,11 +1,13 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { handleAnalyticsProxy } from "./analytics-proxy"; -// #8387: the analytics proxy is the cookieless-beacon relay to the Umami-compatible upstream. Its four -// security behaviors (strict allowlist, cookie strip, cf-connecting-ip-only x-forwarded-for, set-cookie -// strip) had zero coverage. These pin each one against a stubbed upstream fetch. +// The analytics proxy is the first-party relay to PostHog (#8293, replacing the Umami relay #8387 +// covered). Its security behaviors -- prefix allowlist, cookie strip, cf-connecting-ip-only +// x-forwarded-for, set-cookie strip, body-size caps -- plus the asset/capture host split and the +// best-effort edge cache are each pinned here against a stubbed upstream fetch. -const UPSTREAM = "https://tasty.aethereal.dev"; +const API = "https://us.i.posthog.com"; +const ASSETS = "https://us-assets.i.posthog.com"; type ForwardedCall = { url: string; method: string; headers: Headers }; @@ -26,55 +28,81 @@ function stubUpstream(response: Response) { return { fetchMock, calls }; } +/** A POST to the capture endpoint. `content-length` is set by default because the size gate below + * rejects a body-bearing request that omits it (411) -- tests that exercise that gate pass their own. */ function send(init: RequestInit & { path?: string; query?: string } = {}) { - const { path = "/stats/api/send", query = "", ...rest } = init; - return new Request(`https://loopover.ai${path}${query}`, { method: "POST", ...rest }); + const { path = "/stats/i/v0/e/", query = "", ...rest } = init; + const headers = new Headers(rest.headers); + if (!headers.has("content-length")) headers.set("content-length", "128"); + return new Request(`https://loopover.ai${path}${query}`, { method: "POST", ...rest, headers }); } afterEach(() => { vi.unstubAllGlobals(); }); -describe("handleAnalyticsProxy", () => { - it("forwards an allowed POST to the upstream collect endpoint, preserving the query and relaying the response", async () => { +describe("handleAnalyticsProxy — routing", () => { + it("forwards an allowed capture POST to the PostHog capture host, preserving path + query and relaying the response", async () => { const { fetchMock, calls } = stubUpstream( new Response("ok-body", { status: 202, statusText: "Accepted" }), ); const response = await handleAnalyticsProxy( - send({ query: "?v=2&cache=abc", body: "beacon-payload" }), + send({ query: "?v=2&ip=1", body: "batch-payload" }), ); expect(fetchMock).toHaveBeenCalledTimes(1); // /stats prefix stripped, path + query preserved onto the real upstream host. - expect(calls[0]!.url).toBe(`${UPSTREAM}/api/send?v=2&cache=abc`); + expect(calls[0]!.url).toBe(`${API}/i/v0/e/?v=2&ip=1`); expect(calls[0]!.method).toBe("POST"); - // Upstream status/statusText/body are relayed back untouched. - expect(response).toBeInstanceOf(Response); expect(response!.status).toBe(202); expect(response!.statusText).toBe("Accepted"); expect(await response!.text()).toBe("ok-body"); }); - it("rejects a disallowed method with 405 + an allow header, without ever calling fetch (method gate)", async () => { - const { fetchMock } = stubUpstream(new Response("should not be used")); - - const response = await handleAnalyticsProxy(send({ method: "GET" })); + it.each([ + ["/stats/e/", `${API}/e/`], + ["/stats/batch/", `${API}/batch/`], + ["/stats/decide/", `${API}/decide/`], + ["/stats/flags/", `${API}/flags/`], + ["/stats/engage/", `${API}/engage/`], + ["/stats/s/", `${API}/s/`], + ])("routes the allowlisted capture path %s to the capture host", async (path, expected) => { + const { calls } = stubUpstream(new Response(null, { status: 200 })); + await handleAnalyticsProxy(send({ path })); + expect(calls[0]!.url).toBe(expected); + }); - expect(response!.status).toBe(405); - expect(response!.headers.get("allow")).toBe("POST"); - expect(fetchMock).not.toHaveBeenCalled(); + it.each([ + ["/stats/static/array.js", `${ASSETS}/static/array.js`], + ["/stats/array/phc_x/config.js", `${ASSETS}/array/phc_x/config.js`], + ])("routes the asset path %s to the dedicated asset host", async (path, expected) => { + const { calls } = stubUpstream(new Response("sdk", { status: 200 })); + const request = new Request(`https://loopover.ai${path}`); + await handleAnalyticsProxy(request); + expect(calls[0]!.url).toBe(expected); }); it("returns undefined (falls through to SSR) and never fetches for a path outside the allowlist", async () => { const { fetchMock } = stubUpstream(new Response("should not be used")); - // The admin/auth API lives on the same upstream origin as the collect endpoint -- must NOT be proxied. + // The allowlist is load-bearing: nothing outside PostHog's documented ingest surface forwards. expect(await handleAnalyticsProxy(send({ path: "/stats/admin" }))).toBeUndefined(); - expect(await handleAnalyticsProxy(send({ path: "/stats/api/collect" }))).toBeUndefined(); + expect(await handleAnalyticsProxy(send({ path: "/stats/api/send" }))).toBeUndefined(); expect(fetchMock).not.toHaveBeenCalled(); }); + it("returns undefined for any path outside the /stats prefix entirely", async () => { + const { fetchMock } = stubUpstream(new Response("should not be used")); + + expect(await handleAnalyticsProxy(send({ path: "/docs" }))).toBeUndefined(); + // The bare prefix with no trailing segment is not an analytics path either. + expect(await handleAnalyticsProxy(send({ path: "/stats" }))).toBeUndefined(); + expect(fetchMock).not.toHaveBeenCalled(); + }); +}); + +describe("handleAnalyticsProxy — privacy and header hygiene", () => { it("strips the visitor's first-party cookie before forwarding (cookieless guarantee, #597)", async () => { const { calls } = stubUpstream(new Response(null, { status: 200 })); @@ -94,6 +122,7 @@ describe("handleAnalyticsProxy", () => { send({ headers: { "cf-connecting-ip": "203.0.113.7", "x-forwarded-for": "66.66.66.66" } }), ); + // This is what preserves PostHog's GeoIP enrichment -- the Umami-parity requirement in #8299. expect(calls[0]!.headers.get("x-forwarded-for")).toBe("203.0.113.7"); // The trusted-IP header itself is not leaked upstream. expect(calls[0]!.headers.get("cf-connecting-ip")).toBeNull(); @@ -111,7 +140,7 @@ describe("handleAnalyticsProxy", () => { stubUpstream( new Response("ok", { status: 200, - headers: { "set-cookie": "umami=1; Path=/", "x-app": "v1" }, + headers: { "set-cookie": "ph=1; Path=/", "x-app": "v1" }, }), ); @@ -120,8 +149,173 @@ describe("handleAnalyticsProxy", () => { expect(response!.headers.get("set-cookie")).toBeNull(); expect(response!.headers.get("x-app")).toBe("v1"); // unrelated response headers are still relayed }); +}); + +describe("handleAnalyticsProxy — body size gate", () => { + it("rejects a body-bearing request with no content-length with 411, before buffering or fetching", async () => { + const { fetchMock } = stubUpstream(new Response("should not be used")); + + const request = new Request("https://loopover.ai/stats/i/v0/e/", { + method: "POST", + body: "payload", + }); + request.headers.delete("content-length"); + const response = await handleAnalyticsProxy(request); + + expect(response!.status).toBe(411); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("rejects a non-numeric content-length with 411 rather than coercing it", async () => { + const { fetchMock } = stubUpstream(new Response("should not be used")); + + const response = await handleAnalyticsProxy(send({ headers: { "content-length": "abc" } })); + + expect(response!.status).toBe(411); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("rejects an oversized capture body with 413, before buffering or fetching", async () => { + const { fetchMock } = stubUpstream(new Response("should not be used")); + + const response = await handleAnalyticsProxy( + send({ headers: { "content-length": String(64 * 1024 + 1) } }), + ); + + expect(response!.status).toBe(413); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("allows a capture body exactly at the cap (boundary is inclusive)", async () => { + const { fetchMock } = stubUpstream(new Response(null, { status: 200 })); + + const response = await handleAnalyticsProxy( + send({ headers: { "content-length": String(64 * 1024) } }), + ); + + expect(response!.status).toBe(200); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("gives session-replay snapshots (/s/) their own larger ceiling, so enabling replay cannot silently 413", async () => { + const { calls } = stubUpstream(new Response(null, { status: 200 })); + + // Well past the 64 KiB capture cap, well inside the 2 MiB replay cap. + const response = await handleAnalyticsProxy( + send({ path: "/stats/s/", headers: { "content-length": String(512 * 1024) } }), + ); + + expect(response!.status).toBe(200); + expect(calls[0]!.url).toBe(`${API}/s/`); + }); + + it("still rejects a replay snapshot past the replay ceiling", async () => { + const { fetchMock } = stubUpstream(new Response("should not be used")); + + const response = await handleAnalyticsProxy( + send({ path: "/stats/s/", headers: { "content-length": String(2 * 1024 * 1024 + 1) } }), + ); + + expect(response!.status).toBe(413); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("does not apply the content-length gate to a bodyless GET", async () => { + const { fetchMock } = stubUpstream(new Response(null, { status: 200 })); + + const response = await handleAnalyticsProxy( + new Request("https://loopover.ai/stats/flags/?v=2"), + ); + + expect(response!.status).toBe(200); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); +}); + +describe("handleAnalyticsProxy — asset edge cache", () => { + const assetRequest = () => new Request("https://loopover.ai/stats/static/array.js"); + + it("serves a cache hit without touching the network", async () => { + const { fetchMock } = stubUpstream(new Response("should not be used")); + vi.stubGlobal("caches", { + default: { match: vi.fn(async () => new Response("cached-sdk")), put: vi.fn() }, + }); + + const response = await handleAnalyticsProxy(assetRequest(), { waitUntil: () => {} }); + + expect(await response!.text()).toBe("cached-sdk"); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("fetches and writes through to the cache on a miss", async () => { + stubUpstream(new Response("fresh-sdk", { status: 200 })); + const put = vi.fn(async () => {}); + vi.stubGlobal("caches", { default: { match: vi.fn(async () => undefined), put } }); + const deferred: Promise[] = []; + + const response = await handleAnalyticsProxy(assetRequest(), { + waitUntil: (p) => deferred.push(p), + }); + + expect(await response!.text()).toBe("fresh-sdk"); + await Promise.all(deferred); + expect(put).toHaveBeenCalledTimes(1); + }); + + it("degrades to an uncached fetch when ctx is undefined (the #7794 production incident)", async () => { + const { fetchMock } = stubUpstream(new Response("fresh-sdk", { status: 200 })); + const match = vi.fn(async () => undefined); + vi.stubGlobal("caches", { default: { match, put: vi.fn() } }); + + // server.ts casts an `unknown` ctx straight through, so the type-level contract carries no + // runtime guarantee -- an undefined ctx must never throw. + const response = await handleAnalyticsProxy(assetRequest(), undefined); + + expect(await response!.text()).toBe("fresh-sdk"); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(match).not.toHaveBeenCalled(); + }); + + it("falls through to a normal fetch when the cache read throws", async () => { + stubUpstream(new Response("fresh-sdk", { status: 200 })); + vi.stubGlobal("caches", { + default: { + match: vi.fn(async () => { + throw new Error("cache unavailable"); + }), + put: vi.fn(async () => {}), + }, + }); + + const response = await handleAnalyticsProxy(assetRequest(), { waitUntil: () => {} }); + + expect(await response!.text()).toBe("fresh-sdk"); + }); - it("fails quietly with 502 when the upstream fetch throws (analytics must never take the page down)", async () => { + it("does not fail the request when the background cache write rejects", async () => { + stubUpstream(new Response("fresh-sdk", { status: 200 })); + vi.stubGlobal("caches", { + default: { + match: vi.fn(async () => undefined), + put: vi.fn(async () => { + throw new Error("bad Vary header"); + }), + }, + }); + const deferred: Promise[] = []; + + const response = await handleAnalyticsProxy(assetRequest(), { + waitUntil: (p) => deferred.push(p), + }); + + expect(await response!.text()).toBe("fresh-sdk"); + // The whole point: an unhandled rejection here previously 500'd the current request. + await expect(Promise.all(deferred)).resolves.toBeDefined(); + }); +}); + +describe("handleAnalyticsProxy — failure isolation", () => { + it("fails quietly with 502 when the upstream capture fetch throws (analytics must never take the page down)", async () => { vi.stubGlobal( "fetch", vi.fn(async () => { @@ -129,7 +323,22 @@ describe("handleAnalyticsProxy", () => { }), ); - const response = await handleAnalyticsProxy(send({ body: "beacon" })); + const response = await handleAnalyticsProxy(send({ body: "batch" })); + + expect(response!.status).toBe(502); + }); + + it("fails quietly with 502 when an asset fetch throws", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => { + throw new Error("network down"); + }), + ); + + const response = await handleAnalyticsProxy( + new Request("https://loopover.ai/stats/static/a.js"), + ); expect(response!.status).toBe(502); }); diff --git a/apps/loopover-ui/src/lib/analytics-proxy.ts b/apps/loopover-ui/src/lib/analytics-proxy.ts index f6f1d34baf..ba0ce01702 100644 --- a/apps/loopover-ui/src/lib/analytics-proxy.ts +++ b/apps/loopover-ui/src/lib/analytics-proxy.ts @@ -1,24 +1,67 @@ -// First-party endpoint for forwarding cookieless analytics events. +// First-party endpoint for forwarding cookieless analytics events to PostHog +// (#8293, #8299 -- this module previously forwarded to self-hosted Umami). // // The browser only ever talks to our own origin (loopover.ai): -// POST /stats/api/send -> https://tasty.aethereal.dev/api/send +// POST /stats/i/v0/e/ -> https://us.i.posthog.com/i/v0/e/ (capture) +// GET /stats/static/* -> https://us-assets.i.posthog.com/... (SDK bundle) // -// Do not proxy the remote Umami script through this origin. Serving mutable -// third-party JavaScript as same-origin code lets a compromised analytics host -// execute with the app's privileges. The UI uses a tiny local beacon instead and -// this proxy only relays the resulting collect payload. +// The `/stats` prefix is retained from the Umami proxy this replaces. It is +// not cosmetic: PostHog's own Cloudflare-proxy guide warns that ad blockers +// pattern-match "analytics"/"tracking"/"posthog"/"ph" in URLs even on a +// first-party origin, so an unrelated-sounding prefix is the recommendation. +// Keeping the existing one also means no dangling references elsewhere. // -// The allowlist below is load-bearing: this must NOT become an open proxy onto -// the Umami host, whose admin/auth API lives on the same origin as the collect -// endpoint. +// WHY THE ALLOWLIST SURVIVED THE PORT (#8293 requires it stay load-bearing). +// Its original justification was Umami-specific: that host served its admin +// and auth API from the same origin as the collect endpoint, so an open proxy +// there would have exposed the analytics console itself. That specific threat +// does NOT carry over -- PostHog's ingest hosts (us.i.posthog.com, +// us-assets.i.posthog.com) serve ingest only, and the app/admin surface lives +// on the separate us.posthog.com. The allowlist is kept anyway, one level +// looser (path PREFIXES rather than exact paths), because it still bounds what +// this public unauthenticated route can be pointed at, while being tolerant of +// posthog-js choosing different sub-paths across SDK versions. An exact-path +// allowlist would silently break capture on a posthog-js upgrade; a prefix set +// covering PostHog's documented ingest surface will not. +// +// Note this DOES now serve PostHog's SDK bundle as same-origin JavaScript, +// which the Umami-era header comment here explicitly refused to do for the +// Umami tracker. That is a deliberate reversal, not an oversight: PostHog's +// documented proxy design requires it (posthog-js bootstraps itself from +// /static/array.js), the asset is served by a dedicated immutable asset host +// rather than the same host as an admin console, and it is the same posture +// JSONbored/metagraphed#7781 already runs in production. The alternative -- +// loading the SDK from a third-party origin -- is strictly worse for both +// privacy and ad-blocker resilience. -const UPSTREAM = "https://tasty.aethereal.dev"; export const ANALYTICS_PREFIX = "/stats"; -// First-party path -> methods we forward. Anything else under /stats 404s here. -const ROUTES: Record> = { - "/stats/api/send": new Set(["POST"]), -}; +const POSTHOG_API_HOST = "https://us.i.posthog.com"; +const POSTHOG_ASSET_HOST = "https://us-assets.i.posthog.com"; + +// Edge-cacheable, per-project rather than per-visitor: the SDK bundle and the +// remote-config document. Routed to the asset host, everything else to the +// capture host. Order matters only in that these are checked first. +const ASSET_PREFIXES = ["/static/", "/array/"] as const; + +// PostHog's documented ingest surface, as posthog-js actually calls it: +// /e/, /i/ capture (legacy and current paths) +// /batch/ batched capture +// /decide/ legacy remote config + flags +// /flags/ current flag evaluation +// /s/ session-replay snapshots (not enabled yet -- #8295 -- but +// allowed so enabling it is a one-line SDK change, not a +// silent 404 to debug) +// /engage/ person-property updates +const CAPTURE_PREFIXES = [ + "/e/", + "/i/", + "/batch/", + "/decide/", + "/flags/", + "/s/", + "/engage/", +] as const; // Request headers we never forward upstream (hop-by-hop or our-origin specific). const STRIP_REQUEST_HEADERS = new Set([ @@ -51,49 +94,119 @@ const STRIP_RESPONSE_HEADERS = new Set([ "set-cookie", // cookieless analytics: never relay cookies to the client ]); -/** - * Proxies the allowlisted Umami collect path through our own origin. - * Returns a `Response` for `/stats/api/send`, or - * `undefined` for any other request so the caller falls through to SSR. - */ -export async function handleAnalyticsProxy(request: Request): Promise { - const url = new URL(request.url); - const allowedMethods = ROUTES[url.pathname]; - if (!allowedMethods) return undefined; // not an analytics path — let SSR handle it - - if (!allowedMethods.has(request.method)) { - return new Response("Method Not Allowed", { - status: 405, - headers: { allow: [...allowedMethods].join(", ") }, - }); +// This route is public and unauthenticated -- reachable by anyone, not just +// posthog-js -- so it must never buffer an unbounded body into Worker memory. +// Sized well above the Umami proxy's old 16 KiB cap this replaces, because +// PostHog's capture endpoint accepts BATCHED events (posthog-js queues and +// flushes several in one POST) where Umami sent strictly one event per +// request. Our own defensive ceiling, not a PostHog-documented limit. +const MAX_INGEST_BODY_BYTES = 64 * 1024; + +// Session-replay snapshots are a different traffic class: rrweb DOM snapshots +// are orders of magnitude larger than an event batch, and a full-snapshot +// flush on a dense page blows past 64 KiB easily. Replay is not enabled yet +// (#8295), but the ceiling is sized correctly now so turning it on cannot +// silently 413 and drop recordings with nothing in the UI surfacing the loss -- +// which is exactly the production bug JSONbored/metagraphed#8263 hit. +const MAX_REPLAY_BODY_BYTES = 2 * 1024 * 1024; + +function isReplayPath(upstreamPath: string): boolean { + return upstreamPath.startsWith("/s/"); +} + +function startsWithAny(upstreamPath: string, prefixes: readonly string[]): boolean { + return prefixes.some((prefix) => upstreamPath.startsWith(prefix)); +} + +/** Cloudflare Workers runtime `caches` global. Absent under local `vite dev` + * (Node) and in this module's unit tests, which is why the asset path below + * degrades to an uncached fetch when it is undefined. */ +declare const caches: + | { + default: { + match(request: Request): Promise; + put(request: Request, response: Response): Promise; + }; + } + | undefined; + +export type PostHogAssetContext = { waitUntil(promise: Promise): void }; + +async function retrieveAnalyticsAsset( + request: Request, + upstreamUrl: string, + ctx: PostHogAssetContext | undefined, +): Promise { + // `ctx` is typed as present but is cast through from an `unknown` parameter + // in server.ts, so the type-level contract carries no runtime guarantee -- + // and an undefined ctx here was the root cause of a real production incident + // in the sibling implementation (JSONbored/metagraphed#7794): `ctx.waitUntil` + // threw, the rejection escaped, and every asset request 500'd. Edge caching + // is a best-effort optimization -- treat a missing or unusable ctx exactly + // like a missing `caches` global and degrade to a plain fetch, never throw. + const hasEdgeCache = typeof caches !== "undefined" && typeof ctx?.waitUntil === "function"; + + let cached: Response | undefined; + if (hasEdgeCache) { + try { + cached = await caches.default.match(request); + } catch { + // A cache read failure must fall through to a normal upstream fetch. + } } + if (cached) return cached; + + const upstream = await fetch(upstreamUrl); + if (hasEdgeCache) { + // A rejected promise handed to waitUntil() becomes an unhandled rejection + // at Worker global scope, which this app's own error-capture listeners + // (lib/error-capture.ts) then turn into a 500 for the CURRENT request -- + // even though the response below was already computed correctly. Swallow + // write failures; the response is already on its way to the browser. + ctx.waitUntil(caches.default.put(request, upstream.clone()).catch(() => undefined)); + } + return upstream; +} + +async function forwardToAnalyticsHost(request: Request, upstreamUrl: string): Promise { + const hasBody = request.method !== "GET" && request.method !== "HEAD"; - const upstreamUrl = UPSTREAM + url.pathname.slice(ANALYTICS_PREFIX.length) + url.search; + // Content-length-first gate: reject BEFORE buffering, never after, so an + // oversized or malformed request is never read into memory at all. + if (hasBody) { + const rawContentLength = request.headers.get("content-length"); + const contentLength = rawContentLength === null ? NaN : Number(rawContentLength); + if (!Number.isSafeInteger(contentLength) || contentLength < 0) { + return new Response("Length Required", { status: 411 }); + } + const maxBytes = isReplayPath(new URL(upstreamUrl).pathname) + ? MAX_REPLAY_BODY_BYTES + : MAX_INGEST_BODY_BYTES; + if (contentLength > maxBytes) { + return new Response("Payload Too Large", { status: 413 }); + } + } const headers = new Headers(); request.headers.forEach((value, key) => { if (!STRIP_REQUEST_HEADERS.has(key.toLowerCase())) headers.set(key, value); }); - // Preserve the real client IP so Umami geolocates the visitor, not the Worker. Set it to the - // trusted cf-connecting-ip only -- the client-supplied x-forwarded-for is stripped above so a - // visitor cannot spoof their geolocation. + // Preserve the real client IP so PostHog geolocates the visitor, not the + // Worker -- this is what keeps the country/city data the Umami beacon's own + // geo provided. Set from the trusted cf-connecting-ip only; the + // client-supplied x-forwarded-for is stripped above so a visitor cannot + // spoof their geolocation. const clientIp = request.headers.get("cf-connecting-ip"); if (clientIp) headers.set("x-forwarded-for", clientIp); - const hasBody = request.method !== "GET" && request.method !== "HEAD"; - - let upstream: Response; - try { - upstream = await fetch(upstreamUrl, { - method: request.method, - headers, - // Buffer the (tiny) collect payload so we don't need a streaming/duplex body. - body: hasBody ? await request.arrayBuffer() : null, - }); - } catch { - // Analytics must never take the page down — fail quietly. - return new Response(null, { status: 502 }); - } + const upstream = await fetch(upstreamUrl, { + method: request.method, + headers, + // Buffered, not streamed straight through: PostHog's own proxy guide flags + // passing request.body directly as an observed cause of corrupted event + // payloads on POST. + body: hasBody ? await request.arrayBuffer() : null, + }); const responseHeaders = new Headers(); upstream.headers.forEach((value, key) => { @@ -106,3 +219,35 @@ export async function handleAnalyticsProxy(request: Request): Promise { + const url = new URL(request.url); + if (!url.pathname.startsWith(`${ANALYTICS_PREFIX}/`)) return undefined; + + const upstreamPath = url.pathname.slice(ANALYTICS_PREFIX.length); + const isAsset = startsWithAny(upstreamPath, ASSET_PREFIXES); + const isCapture = startsWithAny(upstreamPath, CAPTURE_PREFIXES); + // Not an analytics path we know about. Returning undefined (rather than a + // 404) keeps the old behavior: anything under /stats that isn't allowlisted + // falls through to SSR, which renders the normal not-found page. + if (!isAsset && !isCapture) return undefined; + + const upstreamUrl = (isAsset ? POSTHOG_ASSET_HOST : POSTHOG_API_HOST) + upstreamPath + url.search; + + try { + return isAsset + ? await retrieveAnalyticsAsset(request, upstreamUrl, ctx) + : await forwardToAnalyticsHost(request, upstreamUrl); + } catch { + // Analytics must never take the page down -- fail quietly. + return new Response(null, { status: 502 }); + } +} diff --git a/apps/loopover-ui/src/lib/analytics.test.ts b/apps/loopover-ui/src/lib/analytics.test.ts new file mode 100644 index 0000000000..66d4250416 --- /dev/null +++ b/apps/loopover-ui/src/lib/analytics.test.ts @@ -0,0 +1,166 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +// Hoisted spies shared by the module mock below. posthog-js's default export IS the `posthog` +// singleton (same as `import posthog from "posthog-js"` in real code), so the mock mirrors that +// shape rather than wrapping it -- analytics.ts destructures `{ default: posthog }` off its own +// dynamic import(). +const mocks = vi.hoisted(() => ({ init: vi.fn(), capture: vi.fn() })); +vi.mock("posthog-js", () => ({ default: { init: mocks.init, capture: mocks.capture } })); + +import { + captureEvent, + capturePageview, + initAnalytics, + isAnalyticsConfigured, + resetAnalyticsForTest, +} from "./analytics"; + +const TOKEN = "phc_test_token"; + +beforeEach(() => { + vi.clearAllMocks(); + // The SDK promise is memoized for the module's lifetime, so without this each test after the + // first would reuse the previous one's already-resolved (or already-rejected) init. + resetAnalyticsForTest(); +}); + +afterEach(() => { + vi.unstubAllEnvs(); +}); + +describe("isAnalyticsConfigured", () => { + it("false when VITE_POSTHOG_PROJECT_TOKEN is unset or blank", () => { + vi.stubEnv("VITE_POSTHOG_PROJECT_TOKEN", ""); + expect(isAnalyticsConfigured()).toBe(false); + // A whitespace-only build variable is a real failure mode -- an empty CI value that still + // technically "exists" must read as unconfigured, not as a token made of spaces. + vi.stubEnv("VITE_POSTHOG_PROJECT_TOKEN", " "); + expect(isAnalyticsConfigured()).toBe(false); + }); + + it("true when VITE_POSTHOG_PROJECT_TOKEN is set", () => { + vi.stubEnv("VITE_POSTHOG_PROJECT_TOKEN", TOKEN); + expect(isAnalyticsConfigured()).toBe(true); + }); +}); + +describe("analytics — unconfigured (no token)", () => { + it("never loads or initializes the SDK from any entry point", async () => { + vi.stubEnv("VITE_POSTHOG_PROJECT_TOKEN", ""); + + initAnalytics(); + capturePageview("https://loopover.ai/docs"); + captureEvent("some_event", { a: 1 }); + await new Promise((resolve) => setTimeout(resolve, 0)); + + // The whole "zero cost when unconfigured" guarantee: no init, no capture, and because the + // import is dynamic, the library is never even fetched. + expect(mocks.init).not.toHaveBeenCalled(); + expect(mocks.capture).not.toHaveBeenCalled(); + }); +}); + +describe("initAnalytics", () => { + it("initializes exactly once across repeated calls (idempotent)", async () => { + vi.stubEnv("VITE_POSTHOG_PROJECT_TOKEN", TOKEN); + + initAnalytics(); + initAnalytics(); + initAnalytics(); + await vi.waitFor(() => expect(mocks.init).toHaveBeenCalledTimes(1)); + }); + + it("initializes with the SPA-correct, Umami-parity configuration", async () => { + vi.stubEnv("VITE_POSTHOG_PROJECT_TOKEN", TOKEN); + + initAnalytics(); + await vi.waitFor(() => expect(mocks.init).toHaveBeenCalledTimes(1)); + + const [token, config] = mocks.init.mock.calls[0]!; + expect(token).toBe(TOKEN); + // Proxied first-party through this origin, never posthog.com directly. + expect(config.api_host).toBe("/stats"); + expect(config.ui_host).toBe("https://us.posthog.com"); + // Pageviews are captured manually (the router subscription in __root.tsx) because an SPA's + // auto-pageview only ever covers the first load... + expect(config.capture_pageview).toBe(false); + // ...but pageleave must be re-enabled explicitly, since posthog-js's default for it piggybacks + // on capture_pageview and would otherwise silently go off too. + expect(config.capture_pageleave).toBe(true); + expect(config.capture_performance).toEqual({ web_vitals: true }); + // Parity with the Umami beacon, which checked navigator.doNotTrack itself. + expect(config.respect_dnt).toBe(true); + // Not "memory": that resets identity every reload and inflates unique visitors -- the exact + // regression JSONbored/metagraphed#8210 had to correct. localStorage sets no cookie. + expect(config.persistence).toBe("localStorage"); + }); + + it("honors a VITE_POSTHOG_HOST override for local testing against a real host", async () => { + vi.stubEnv("VITE_POSTHOG_PROJECT_TOKEN", TOKEN); + vi.stubEnv("VITE_POSTHOG_HOST", "https://us.i.posthog.com"); + + initAnalytics(); + await vi.waitFor(() => expect(mocks.init).toHaveBeenCalledTimes(1)); + + expect(mocks.init.mock.calls[0]![1].api_host).toBe("https://us.i.posthog.com"); + }); + + it("honors a VITE_POSTHOG_UI_HOST override", async () => { + vi.stubEnv("VITE_POSTHOG_PROJECT_TOKEN", TOKEN); + vi.stubEnv("VITE_POSTHOG_UI_HOST", "https://eu.posthog.com"); + + initAnalytics(); + await vi.waitFor(() => expect(mocks.init).toHaveBeenCalledTimes(1)); + + expect(mocks.init.mock.calls[0]![1].ui_host).toBe("https://eu.posthog.com"); + }); +}); + +describe("capturePageview", () => { + it("captures $pageview with the explicit url and the page title (Umami parity, #8299)", async () => { + vi.stubEnv("VITE_POSTHOG_PROJECT_TOKEN", TOKEN); + document.title = "LoopOver — Docs"; + + capturePageview("https://loopover.ai/docs"); + await vi.waitFor(() => expect(mocks.capture).toHaveBeenCalledTimes(1)); + + expect(mocks.capture).toHaveBeenCalledWith("$pageview", { + $current_url: "https://loopover.ai/docs", + // posthog-js has no built-in $title-equivalent, and the removed beacon did send `title`. + page_title: "LoopOver — Docs", + }); + }); + + it("omits $current_url when no url is passed, letting posthog-js read the location itself", async () => { + vi.stubEnv("VITE_POSTHOG_PROJECT_TOKEN", TOKEN); + document.title = "LoopOver"; + + capturePageview(); + await vi.waitFor(() => expect(mocks.capture).toHaveBeenCalledTimes(1)); + + const [event, properties] = mocks.capture.mock.calls[0]!; + expect(event).toBe("$pageview"); + expect("$current_url" in properties).toBe(false); + expect(properties.page_title).toBe("LoopOver"); + }); +}); + +describe("captureEvent", () => { + it("forwards the event name and properties verbatim", async () => { + vi.stubEnv("VITE_POSTHOG_PROJECT_TOKEN", TOKEN); + + captureEvent("docs_search", { query_length: 4 }); + await vi.waitFor(() => expect(mocks.capture).toHaveBeenCalledTimes(1)); + + expect(mocks.capture).toHaveBeenCalledWith("docs_search", { query_length: 4 }); + }); + + it("forwards an event with no properties", async () => { + vi.stubEnv("VITE_POSTHOG_PROJECT_TOKEN", TOKEN); + + captureEvent("cta_clicked"); + await vi.waitFor(() => expect(mocks.capture).toHaveBeenCalledTimes(1)); + + expect(mocks.capture).toHaveBeenCalledWith("cta_clicked", undefined); + }); +}); diff --git a/apps/loopover-ui/src/lib/analytics.ts b/apps/loopover-ui/src/lib/analytics.ts new file mode 100644 index 0000000000..3773f8cc24 --- /dev/null +++ b/apps/loopover-ui/src/lib/analytics.ts @@ -0,0 +1,203 @@ +// Centralized PostHog web-analytics seam for loopover-ui (#8293, #8299). +// +// Single chokepoint for client-side product analytics: the app calls +// `initAnalytics()` / `capturePageview()` / `captureEvent()` and never touches +// `posthog-js` directly anywhere else. This replaces the self-hosted Umami +// beacon that used to live in routes/__root.tsx (#8299's decommission). +// +// SCOPE. Web analytics only. Session replay (#8295), error tracking (#8291 -- +// browser-sentry.ts still owns that seam), and feature flags (#8297) are each +// their own issue and are deliberately NOT configured here; adding them means +// opening the corresponding `posthog.init` block under that issue, not +// widening this one. +// +// `posthog-js` is loaded via a DYNAMIC import, mirroring browser-sentry.ts's +// exact `void import("@sentry/react")` pattern: a build with no +// VITE_POSTHOG_PROJECT_TOKEN configured (self-hosters, local dev, PR CI) never +// downloads or parses the library at all, the same "zero cost when +// unconfigured" guarantee every other telemetry integration in this app +// provides. +// +// Proxied first-party through this origin (posthogApiHost() below, served by +// lib/analytics-proxy.ts's handleAnalyticsProxy) rather than posthog.com +// directly -- the same ad-blocker-resilience rationale the Umami proxy this +// replaces already documented, and PostHog's own Cloudflare-proxy guide's +// stated purpose. +// +// Umami-parity audit (#8299's decommission gate -- PostHog must capture what +// the old beacon did), checked against the removed beacon's actual payload +// (`website`, `hostname`, `screen`, `language`, `title`, `url`, `referrer`): +// - hostname/url/referrer/screen/language: posthog-js attaches these +// automatically on every event ($current_url, $referrer, $screen_width/ +// $screen_height, $browser_language) -- nothing to configure. +// - title: NOT a posthog-js default (it has no $title-equivalent the way it +// auto-attaches referrer/browser/os) -- added explicitly in +// `capturePageview` below, matching what the old beacon sent. +// - Geo: populated server-side from the real client IP, which +// analytics-proxy.ts forwards as `x-forwarded-for` -- the same mechanism +// that fed Umami's geo, unaffected by anything in this file. +// - DNT: the old beacon checked `navigator.doNotTrack === "1"` itself; +// `respect_dnt` below is posthog-js's own equivalent. +// +// One behavior deliberately CHANGES vs. the beacon: the beacon fired once per +// full page load and never on a client-side route change, so SPA navigations +// were invisible to Umami. This module captures every navigation (see +// `capture_pageview: false` plus the router subscription in routes/__root.tsx), +// so post-cutover pageview counts run legitimately higher than Umami's for the +// same traffic. That is a fix, not a regression -- but it is a real +// discontinuity at the cutover date when comparing the two series. + +import type { PostHog } from "posthog-js"; + +// Same VITE_*-prefixed build-time-injected convention as every other +// client-exposed config in this app (see lib/config.server.ts's own header on +// the split). A PostHog project token is a write-only ingest token and is safe +// to embed in client JS -- but it is injected at BUILD time by Vite, so it has +// to be set as a build variable on the UI's deploy workflow, NOT as a runtime +// Worker var/secret (setting it there silently leaves the bundle with no token +// and every call below a no-op with zero errors). +// Read at CALL time, not once at module scope, matching browser-sentry.ts's +// own convention for the same class of value. Beyond consistency this is what +// makes the gate testable: `vi.stubEnv` mutates `import.meta.env` in place, so +// a module-level `const` would capture whatever the value was at first import +// and no test could vary it afterwards without a module-registry reset dance. +function posthogToken(): string | undefined { + return (import.meta.env?.VITE_POSTHOG_PROJECT_TOKEN as string | undefined)?.trim() || undefined; +} + +// First-party proxy prefix (lib/analytics-proxy.ts), never PostHog's own +// domain directly. `/stats` is retained from the Umami proxy this replaces: +// PostHog's own proxy guide warns that ad blockers pattern-match +// "analytics"/"tracking"/"posthog"/"ph" in URLs even on a first-party origin, +// and `/stats` is exactly the kind of unrelated-sounding prefix it recommends. +function posthogApiHost(): string { + return (import.meta.env?.VITE_POSTHOG_HOST as string | undefined) || "/stats"; +} + +// Only used for the in-app toolbar's deep-link (an optional, admin-only +// feature) -- never a tracking endpoint, so pointing this at PostHog's real +// app domain rather than the proxy is correct and matches PostHog's own guide. +function posthogUiHost(): string { + return (import.meta.env?.VITE_POSTHOG_UI_HOST as string | undefined) || "https://us.posthog.com"; +} + +// Tracks PostHog's own "SDK defaults" versioning +// (posthog.com/docs/libraries/js#sdk-defaults) -- bump deliberately when +// adopting a newer default set, not on every posthog-js release. A typo here +// cannot silently degrade: posthog-js types this option as a closed +// string-literal union, and this `const` (no explicit annotation) infers that +// literal type, so a bad value fails `npm run typecheck` outright. +const SDK_DEFAULTS_DATE = "2026-05-30"; + +let posthogInit: Promise | null = null; + +function loadPostHog(): Promise { + if (posthogInit) return posthogInit; + posthogInit = import("posthog-js") + .then(({ default: posthog }) => { + posthog.init(posthogToken() as string, { + api_host: posthogApiHost(), + ui_host: posthogUiHost(), + defaults: SDK_DEFAULTS_DATE, + // This is a client-side-routed SPA, so the single pageview `defaults` + // would auto-fire on init only ever covers the first load. Every + // navigation (including the first) is captured explicitly instead, via + // the router subscription in routes/__root.tsx -- one predictable code + // path rather than automatic-for-the-first-load plus + // manual-for-the-rest. + capture_pageview: false, + // posthog-js's own default for this is 'if_capture_pageview' -- it + // piggybacks on capture_pageview and is therefore OFF whenever that is + // false. Disabling capture_pageview above would silently take + // pageleave down with it unless overridden here, and pageleave has no + // SPA caveat of its own (it is driven by page unload, which fires + // correctly regardless of client-side routing). Without it, bounce + // rate and session duration are both wrong. + capture_pageleave: true, + // Native Core Web Vitals capture (LCP/INP/CLS/FCP as $web_vitals). + // Explicit here rather than relying solely on the project's own "Web + // vitals autocapture" dashboard setting: that setting only reaches the + // client through the /array/*/config remote-config fetch, so a + // client-side default keeps this working even if that fetch is + // degraded or blocked. + capture_performance: { web_vitals: true }, + // Parity with the Umami beacon this replaces, which checked + // `navigator.doNotTrack === "1"` before sending anything. + respect_dnt: true, + // "localStorage", NOT posthog-js's own 'localStorage+cookie' default + // and NOT "memory". This is the one setting metagraphed got wrong + // first and had to correct (JSONbored/metagraphed#8210), so it is + // chosen here with that result already in hand: "memory" persists + // nothing, so every reload / new tab / return visit mints a fresh + // anonymous identity and is counted as a brand-new visitor -- which is + // what made PostHog's unique-visitor count run far hotter than Umami's + // for identical real traffic. "localStorage" persists a random + // anonymous id with NO cookie set, so the cookieless posture the Umami + // beacon had is preserved while a returning visitor in the same + // browser actually gets continuity. + // + // Deliberately NOT `cookieless_mode`, PostHog's dedicated + // cookieless-tracking feature -- rejected on its documented behavior, + // not a guess: it strips the request IP server-side before any GeoIP + // enrichment runs, unconditionally, so the country/city data the Umami + // beacon's own geo provided would never populate again. + persistence: "localStorage", + }); + return posthog; + }) + .catch((err) => { + // Never let telemetry wiring crash the host app. + if (import.meta.env?.DEV) console.error("[analytics] posthog load failed", err); + return null; + }); + return posthogInit; +} + +/** True when VITE_POSTHOG_PROJECT_TOKEN is configured -- the same gate every + * export below uses, exposed so callers can show whether web analytics is + * active without importing the SDK (mirrors browser-sentry.ts's + * `isBrowserSentryConfigured`). */ +export function isAnalyticsConfigured(): boolean { + return Boolean(posthogToken()); +} + +/** Drops the memoized SDK promise so a test can exercise a fresh init. Never + * called by app code -- the singleton is the point at runtime. */ +export function resetAnalyticsForTest(): void { + posthogInit = null; +} + +/** Starts loading PostHog. Idempotent (safe to call repeatedly); a no-op when + * unconfigured. Call once, early -- routes/__root.tsx's mount effect. */ +export function initAnalytics(): void { + if (!posthogToken()) return; + void loadPostHog(); +} + +/** Captures one `$pageview`. Pass `url` explicitly on an SPA route change so + * the event reflects the route just navigated to rather than posthog-js's own + * read of a possibly-stale location. + * + * `page_title` is set explicitly for Umami parity (#8299): posthog-js has no + * built-in $title-style property the way it auto-attaches referrer/browser/os, + * and the removed beacon did send `title`. `document.title` is read at call + * time rather than passed in, so it reflects the head the just-resolved route + * committed. */ +export function capturePageview(url?: string): void { + if (!posthogToken()) return; + void loadPostHog().then((posthog) => { + posthog?.capture("$pageview", { + ...(url ? { $current_url: url } : undefined), + ...(typeof document !== "undefined" ? { page_title: document.title } : undefined), + }); + }); +} + +/** Captures a custom event. Best-effort: a no-op when unconfigured, and + * dropped (never queued or retried) if PostHog has not finished loading -- + * matching this module's overall "telemetry must never affect the app" + * posture. */ +export function captureEvent(name: string, properties?: Record): void { + if (!posthogToken()) return; + void loadPostHog().then((posthog) => posthog?.capture(name, properties)); +} diff --git a/apps/loopover-ui/src/routes/__root.tsx b/apps/loopover-ui/src/routes/__root.tsx index 731ac178e1..c2950525d9 100644 --- a/apps/loopover-ui/src/routes/__root.tsx +++ b/apps/loopover-ui/src/routes/__root.tsx @@ -12,6 +12,7 @@ import { useEffect, useRef, type ReactNode } from "react"; import appCss from "../styles.css?url"; import { reportLovableError } from "../lib/lovable-error-reporting"; import { captureBrowserError } from "../lib/browser-sentry"; +import { initAnalytics, capturePageview } from "../lib/analytics"; import { SiteHeader } from "@/components/site/site-header"; import { SiteFooter } from "@/components/site/site-footer"; import { BackToTop } from "@/components/site/back-to-top"; @@ -82,38 +83,6 @@ function ErrorComponent({ error, reset }: { error: Error; reset: () => void }) { ); } -const ANALYTICS_BEACON_SCRIPT = ` -(function () { - var website = "2ec37da2-e519-4bd5-bc16-76e17b03a458"; - var endpoint = "/stats/api/send"; - function send() { - if (navigator.doNotTrack === "1") return; - var payload = JSON.stringify({ - type: "event", - payload: { - website: website, - hostname: location.hostname, - screen: screen.width + "x" + screen.height, - language: navigator.language, - title: document.title, - url: location.pathname + location.search, - referrer: document.referrer, - }, - }); - if (navigator.sendBeacon) { - navigator.sendBeacon(endpoint, new Blob([payload], { type: "application/json" })); - return; - } - fetch(endpoint, { method: "POST", headers: { "content-type": "application/json" }, body: payload, keepalive: true }); - } - if (document.readyState === "loading") { - document.addEventListener("DOMContentLoaded", send, { once: true }); - } else { - send(); - } -})(); -`; - export const Route = createRootRouteWithContext<{ queryClient: QueryClient }>()({ head: () => ({ meta: [ @@ -158,12 +127,6 @@ export const Route = createRootRouteWithContext<{ queryClient: QueryClient }>()( "Agents automate the entire development lifecycle — write, review, merge, learn — end to end, with a published safety record.", }), }, - // Local, cookieless analytics beacon. Do not load the mutable remote - // Umami tracker as first-party JavaScript; only the event payload is - // forwarded through /stats/api/send by the Worker proxy. - { - children: ANALYTICS_BEACON_SCRIPT, - }, ], }), shellComponent: RootShell, @@ -188,6 +151,23 @@ function RootShell({ children }: { children: ReactNode }) { function RootComponent() { const { queryClient } = Route.useRouteContext(); + const router = useRouter(); + + // PostHog web analytics (#8293). `capture_pageview` is disabled in + // lib/analytics.ts's own init call -- an SPA's auto-pageview only ever + // covers the very first load -- so every pageview, including this initial + // one, is captured explicitly here: once on mount, then once per navigation + // whose URL actually changed. `onResolved` also fires when a route + // re-resolves without navigating (e.g. after `router.invalidate()`), which + // is not a new pageview, hence the `hrefChanged` guard. + useEffect(() => { + initAnalytics(); + capturePageview(window.location.href); + return router.subscribe("onResolved", (event) => { + if (!event.hrefChanged) return; + capturePageview(window.location.href); + }); + }, [router]); return ( diff --git a/apps/loopover-ui/src/server.ts b/apps/loopover-ui/src/server.ts index 808fca14da..ac7cefe65c 100644 --- a/apps/loopover-ui/src/server.ts +++ b/apps/loopover-ui/src/server.ts @@ -2,7 +2,7 @@ import "./lib/error-capture"; import { consumeLastCapturedError } from "./lib/error-capture"; import { renderErrorPage } from "./lib/error-page"; -import { handleAnalyticsProxy } from "./lib/analytics-proxy"; +import { handleAnalyticsProxy, type PostHogAssetContext } from "./lib/analytics-proxy"; type ServerEntry = { fetch: (request: Request, env: unknown, ctx: unknown) => Promise | Response; @@ -40,10 +40,22 @@ async function normalizeCatastrophicSsrResponse(response: Response): Promise=8" } }, - "node_modules/@posthog/cli/node_modules/prettier": { - "version": "3.8.3", - "resolved": "https://registry.npmjs.org/prettier/-/prettier-3.8.3.tgz", - "integrity": "sha512-7igPTM53cGHMW8xWuVTydi2KO233VFiTNyF5hLJqpilHfmn8C8gPf+PS7dUT64YcXFbiMGZxS9pCSxL/Dxm/Jw==", - "extraneous": true, - "license": "MIT", - "bin": { - "prettier": "bin/prettier.cjs" - }, - "engines": { - "node": ">=14" - }, - "funding": { - "url": "https://github.com/prettier/prettier?sponsor=1" - } - }, "node_modules/@posthog/core": { - "version": "1.45.1", - "resolved": "https://registry.npmjs.org/@posthog/core/-/core-1.45.1.tgz", - "integrity": "sha512-tLtvzomavb2PPWdGYKsusyIzIeL2Px47v348Smibkay7sMy/83TyPk+Ptsp2NdeOgJsbuwSxWkR2+XA0aSCAaA==", + "version": "1.46.0", + "resolved": "https://registry.npmjs.org/@posthog/core/-/core-1.46.0.tgz", + "integrity": "sha512-9w/bzHkf8V26YDAk7bO+Y2M0O9LMJg53lw3TOpirWRY77CEq4EQ3FkrtxP6BM48qaeZ/ILmRcfz+u1jjeTLetg==", "license": "MIT", "dependencies": { - "@posthog/types": "^1.398.0" + "@posthog/types": "^1.399.0" } }, "node_modules/@posthog/types": { - "version": "1.398.0", - "resolved": "https://registry.npmjs.org/@posthog/types/-/types-1.398.0.tgz", - "integrity": "sha512-sJMkl4k+u8yS/0fjHsKqE9xTdsAh30a2WvgChiptellnVoE0e8QJKFgqOMD2sk8FaEArPdeFklAhXvmENAt3Sg==", + "version": "1.399.0", + "resolved": "https://registry.npmjs.org/@posthog/types/-/types-1.399.0.tgz", + "integrity": "sha512-/WDwBzqIPko8VJ1B+0rlso2XQEz9+2sqtsY9Tqy3p1GhgTqsFakcz/PmMpAnA321LTEZVRcO6x5hAwABV4yrDw==", "license": "MIT" }, "node_modules/@puppeteer/browsers": { @@ -9413,6 +9408,13 @@ "dev": true, "license": "MIT" }, + "node_modules/@types/trusted-types": { + "version": "2.0.7", + "resolved": "https://registry.npmjs.org/@types/trusted-types/-/trusted-types-2.0.7.tgz", + "integrity": "sha512-ScaPdn1dQczgbl0QFTeTOmVHFULt394XJgOQNoyVhZ6r2vLnMLJfBPd53SB52T/3G36VI1/g2MZaX0cwDuXsfw==", + "license": "MIT", + "optional": true + }, "node_modules/@types/unist": { "version": "3.0.3", "resolved": "https://registry.npmjs.org/@types/unist/-/unist-3.0.3.tgz", @@ -12181,6 +12183,17 @@ "node": ">=6.6.0" } }, + "node_modules/core-js": { + "version": "3.49.0", + "resolved": "https://registry.npmjs.org/core-js/-/core-js-3.49.0.tgz", + "integrity": "sha512-es1U2+YTtzpwkxVLwAFdSpaIMyQaq0PBgm3YD1W3Qpsn1NAmO3KSgZfu+oGSWVu6NvLHoHCV/aYcsE5wiB7ALg==", + "hasInstallScript": true, + "license": "MIT", + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/core-js" + } + }, "node_modules/core-js-pure": { "version": "3.49.0", "resolved": "https://registry.npmjs.org/core-js-pure/-/core-js-pure-3.49.0.tgz", @@ -12865,6 +12878,15 @@ "url": "https://github.com/fb55/domhandler?sponsor=1" } }, + "node_modules/dompurify": { + "version": "3.4.12", + "resolved": "https://registry.npmjs.org/dompurify/-/dompurify-3.4.12.tgz", + "integrity": "sha512-zQvGet8Z2sWbQhCmfFz/T5QWH2oBmjnqK3qvOjaqaNLrLEF912WamU+ohnTp0TCep/MFVHpdJuCZEdFOdTnEFg==", + "license": "(MPL-2.0 OR Apache-2.0)", + "optionalDependencies": { + "@types/trusted-types": "^2.0.7" + } + }, "node_modules/domutils": { "version": "3.2.2", "resolved": "https://registry.npmjs.org/domutils/-/domutils-3.2.2.tgz", @@ -14167,6 +14189,12 @@ "integrity": "sha512-YoZjBdafyLIop9lSxXVI33oLD5kN31q4Td+CasofLLYeLXRFeOsuOw0Uo+XNRi9PZlbfdlN2GmRtm4tCEQ9/KA==", "license": "MIT" }, + "node_modules/fflate": { + "version": "0.4.9", + "resolved": "https://registry.npmjs.org/fflate/-/fflate-0.4.9.tgz", + "integrity": "sha512-zdxgIEddhfsyCaWpJ2SdXEP8ZMrKJ6+5jl4OupODcywU0IhRk6gdXuVGcPICyfx2H97hVK7xmJtRLPjkxAX8Vw==", + "license": "MIT" + }, "node_modules/figures": { "version": "6.1.0", "resolved": "https://registry.npmjs.org/figures/-/figures-6.1.0.tgz", @@ -19499,6 +19527,23 @@ "node": ">=0.10.0" } }, + "node_modules/posthog-js": { + "version": "1.409.3", + "resolved": "https://registry.npmjs.org/posthog-js/-/posthog-js-1.409.3.tgz", + "integrity": "sha512-8ndnPs6AVeUcIexQ34aVSTrLpZS8J4y+0gXaug7SrDo8N51eeSXcXJgrsCI7t78NhxOx8OzIoGcF2biRDqgIaw==", + "license": "(Apache-2.0 AND MIT)", + "dependencies": { + "@posthog/browser-common": "^0.3.1", + "@posthog/core": "^1.46.0", + "@posthog/types": "^1.399.0", + "core-js": "^3.49.0", + "dompurify": "^3.3.2", + "fflate": "^0.4.8", + "preact": "^10.29.3", + "query-selector-shadow-dom": "^1.0.1", + "web-vitals": "^5.3.0" + } + }, "node_modules/posthog-node": { "version": "5.46.1", "resolved": "https://registry.npmjs.org/posthog-node/-/posthog-node-5.46.1.tgz", @@ -19519,6 +19564,24 @@ } } }, + "node_modules/preact": { + "version": "10.29.7", + "resolved": "https://registry.npmjs.org/preact/-/preact-10.29.7.tgz", + "integrity": "sha512-DCHYrK/B10yUD3ZjLfhZ3WIE/9Vf9VFUODcRE2dRomTYDpJk6z6L9wecSfhfE6M9ZTHUdyQkoC46arIDhEV84Q==", + "license": "MIT", + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/preact" + }, + "peerDependencies": { + "preact-render-to-string": ">=5" + }, + "peerDependenciesMeta": { + "preact-render-to-string": { + "optional": true + } + } + }, "node_modules/prelude-ls": { "version": "1.2.1", "resolved": "https://registry.npmjs.org/prelude-ls/-/prelude-ls-1.2.1.tgz", @@ -19728,6 +19791,12 @@ "url": "https://github.com/sponsors/ljharb" } }, + "node_modules/query-selector-shadow-dom": { + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/query-selector-shadow-dom/-/query-selector-shadow-dom-1.0.1.tgz", + "integrity": "sha512-lT5yCqEBgfoMYpf3F2xQRK7zEr1rhIIZuceDK6+xRkJQ4NMbHTwXqk4NkwDwQMNqXgG9r9fyHnzwNVs6zV5KRw==", + "license": "MIT" + }, "node_modules/queue-microtask": { "version": "1.2.3", "resolved": "https://registry.npmjs.org/queue-microtask/-/queue-microtask-1.2.3.tgz", @@ -23147,6 +23216,12 @@ "integrity": "sha512-weOVgZ3aAARgdnb220GqYuh7+rZU0Ka9k9yfKtGAzEYMa6GgiCzW9JjQRJyCJakvibQW+dfjJdihjInKuuCAUQ==", "license": "MIT" }, + "node_modules/web-vitals": { + "version": "5.3.0", + "resolved": "https://registry.npmjs.org/web-vitals/-/web-vitals-5.3.0.tgz", + "integrity": "sha512-q6LWsLatGYZp5VGBIOvbTj6JBV2nOmC8KvWztXBmwJcfFAzhwKwbOxhUH306XY3CcaZDUlSmSuNPBsCn0bFu+g==", + "license": "Apache-2.0" + }, "node_modules/web-worker": { "version": "1.5.0", "resolved": "https://registry.npmjs.org/web-worker/-/web-worker-1.5.0.tgz", diff --git a/packages/loopover-contract/src/telemetry.ts b/packages/loopover-contract/src/telemetry.ts index fade74d716..b2afdb12a7 100644 --- a/packages/loopover-contract/src/telemetry.ts +++ b/packages/loopover-contract/src/telemetry.ts @@ -15,6 +15,24 @@ import type { ToolCategory, ToolContract } from "./tool-definition.js"; /** PostHog's own MCP-Analytics event family (#7737 upstream). */ export const MCP_TOOL_CALL_EVENT = "$mcp_tool_call"; +/** + * PostHog's own MCP-Analytics handshake event, the sibling of MCP_TOOL_CALL_EVENT. + * + * Emitted once per `initialize` -- the first message of every MCP session -- and it is what lets + * PostHog's built-in MCP dashboards break usage down by CLIENT (Claude Code vs Cursor vs a raw SDK + * script) and by client version. `$mcp_tool_call` cannot answer that: it carries what was called, + * never who connected. + */ +export const MCP_INITIALIZE_EVENT = "$mcp_initialize"; + +/** + * PostHog's own tools/list event. + * + * Measures DISCOVERY, which no other event can: joined against `$mcp_tool_call` on `$session_id`, it + * separates a tool nobody wants from a tool nobody could find. + */ +export const MCP_TOOLS_LIST_EVENT = "$mcp_tools_list"; + /** LoopOver's own minimal usage event -- no arguments, no results, ever. */ export const MCP_USAGE_EVENT = "usage_event"; @@ -80,6 +98,150 @@ export const MCP_TELEMETRY_PROPERTY_KEYS = [ ] as const; export type McpTelemetryPropertyKey = (typeof MCP_TELEMETRY_PROPERTY_KEYS)[number]; +/** + * The allowlist for the canonical `$mcp_*` events, kept SEPARATE from the list above. + * + * These are `$`-prefixed because they are PostHog's OWN reserved MCP-Analytics property names, not + * LoopOver's. Their built-in MCP dashboards read `$mcp_tool_name` / `$mcp_duration_ms` / + * `$mcp_is_error` literally, so renaming them into this file's snake_case house style produces an + * event that ingests cleanly and populates nothing -- which is exactly what `$mcp_tool_call` did + * before #10175. Two lists rather than one union, so a native key can never quietly satisfy the + * canonical check (or the reverse). + */ +export const MCP_CANONICAL_PROPERTY_KEYS = [ + "$mcp_source", + "$session_id", + "$mcp_server_name", + "$mcp_server_version", + "$mcp_client_name", + "$mcp_client_version", + "$mcp_tool_name", + "$mcp_duration_ms", + "$mcp_is_error", + "$mcp_error_type", + "$mcp_parameters", + "$mcp_response", + "$mcp_listed_tool_names", +] as const; +export type McpCanonicalPropertyKey = (typeof MCP_CANONICAL_PROPERTY_KEYS)[number]; + +/** + * The literal `$mcp_source` value PostHog's own MCP-Analytics SDK stamps on every event it emits. + * + * Hardcoded to PostHog's constant rather than something LoopOver-specific: it is what their own + * dashboards filter on to separate MCP traffic from everything else in a mixed project, so a "more + * accurate" custom value here would simply make this server invisible to them. + */ +export const MCP_ANALYTICS_SOURCE = "posthog_mcp_analytics"; + +/** + * Session/server/client identity stamped onto every canonical `$mcp_*` event. + * + * `$session_id` is what ties a handshake, a tools/list, and the tool calls that followed into one + * analyzable session -- without it each event is an isolated row and no funnel across them works. + * + * `| undefined` is explicit on every field rather than just `?`: this package compiles under + * `exactOptionalPropertyTypes`, where an optional property may be ABSENT but not present-and- + * undefined. Callers build this by reading headers and request params that are routinely missing, + * so `{ sessionId: maybeUndefined }` is the normal shape and must typecheck without every call site + * spreading conditionally. + */ +export type McpAnalyticsContext = { + sessionId?: string | undefined; + serverName?: string | undefined; + serverVersion?: string | undefined; + clientName?: string | undefined; + clientVersion?: string | undefined; +}; + +/** Cap on the free-form, client-supplied strings that become dashboard dimensions, so a hostile or + * buggy client cannot push an unbounded (or unbounded-cardinality) value into a breakdown. */ +const MCP_LABEL_MAX_CHARS = 256; + +function mcpLabel(value: string | undefined): string | undefined { + const trimmed = value?.trim(); + if (!trimmed) return undefined; + return trimmed.length > MCP_LABEL_MAX_CHARS ? trimmed.slice(0, MCP_LABEL_MAX_CHARS) : trimmed; +} + +/** Absent fields are OMITTED rather than sent as null, matching buildUsageEventProperties' own + * reasoning: a breakdown should not grow a phantom bucket for what was never reported. */ +export function buildMcpContextProperties(context: McpAnalyticsContext): Record { + const sessionId = mcpLabel(context.sessionId); + const serverName = mcpLabel(context.serverName); + const serverVersion = mcpLabel(context.serverVersion); + const clientName = mcpLabel(context.clientName); + const clientVersion = mcpLabel(context.clientVersion); + return { + $mcp_source: MCP_ANALYTICS_SOURCE, + ...(sessionId === undefined ? {} : { $session_id: sessionId }), + ...(serverName === undefined ? {} : { $mcp_server_name: serverName }), + ...(serverVersion === undefined ? {} : { $mcp_server_version: serverVersion }), + ...(clientName === undefined ? {} : { $mcp_client_name: clientName }), + ...(clientVersion === undefined ? {} : { $mcp_client_version: clientVersion }), + }; +} + +/** + * LoopOver's closed error-code set, projected onto PostHog's own `$mcp_error_type` categories. + * + * Two vocabularies exist because both are fixed by someone else: MCP_TELEMETRY_ERROR_CODES is what + * this repo's tool envelopes actually return, and `$mcp_error_type` is the closed set PostHog's MCP + * dashboards group by. A projection is the only way to serve both without one going empty -- + * `usage_event` keeps the precise LoopOver code, this keeps the coarse PostHog category. + */ +const MCP_ERROR_TYPE_BY_CODE: Record = { + invalid_input: "validation", + unauthorized: "permission", + forbidden: "permission", + // A 404 here is a client asking for something absent -- PostHog's own bucket for an upstream 4xx, + // not an internal fault of ours. + not_found: "api_4xx", + // The OPERATOR has not supplied something the tool needs, which is what PostHog means by + // missing_context -- as opposed to a caller sending bad input, which is `validation`. + not_configured: "missing_context", + rate_limited: "rate_limited", + upstream_error: "api_5xx", + timeout: "timeout", + // The human declined an elicitation, so the call is missing something it needed to proceed. + elicitation_declined: "missing_context", + store_unavailable: "internal", + unknown_error: "internal", +}; + +/** `internal` for an absent code: `$mcp_is_error` is true by construction wherever this is called, + * and PostHog's set has no "unclassified" member, so the fault bucket is the safe default. */ +export function mcpErrorType(errorCode: McpTelemetryErrorCode | undefined): string { + return errorCode === undefined ? "internal" : MCP_ERROR_TYPE_BY_CODE[errorCode]; +} + +/** + * What a server observes about one `initialize` handshake. + * + * Both fields are optional because they are: `clientInfo` is optional in the MCP spec's own + * initialize params, and a client that omits it still completes a valid handshake. The event is + * still worth emitting then -- a session that connected is a session that connected, and an + * explicit "unknown client" bucket is a real signal about who is calling. + */ +export const McpInitializeTelemetry = z.object({ + clientName: z.string().min(1).optional(), + clientVersion: z.string().min(1).optional(), +}); +export type McpInitializeTelemetry = z.infer; + +/** The handshake event's properties. The handshake's own `clientInfo` wins over anything the caller + * inferred from headers: it is the MCP spec's own field, sent by the client about itself. */ +export function buildMcpInitializeProperties( + handshake: McpInitializeTelemetry, + context: McpAnalyticsContext = {}, +): Record { + return buildMcpContextProperties({ + ...context, + clientName: handshake.clientName ?? context.clientName, + clientVersion: handshake.clientVersion ?? context.clientVersion, + }); +} + /** What a dispatch chokepoint observes about one call. */ export const McpToolCallTelemetry = z.object({ tool: z.string().min(1), @@ -225,19 +387,46 @@ export function capturePayload(value: unknown, byteCap = MCP_TELEMETRY_PAYLOAD_B export function buildMcpToolCallProperties( call: McpToolCallTelemetry, payloads: { arguments?: unknown; result?: unknown; excluded: boolean }, + context: McpAnalyticsContext = {}, ): Record { - const base = buildUsageEventProperties(call); - if (payloads.excluded) return { ...base, payloads_excluded: true }; - const args = capturePayload(payloads.arguments); - const result = capturePayload(payloads.result); + const args = payloads.excluded ? undefined : capturePayload(payloads.arguments); + const result = payloads.excluded ? undefined : capturePayload(payloads.result); return { - ...base, - payloads_excluded: false, - ...(args === undefined ? {} : { arguments: args }), - ...(result === undefined ? {} : { result }), + ...buildMcpContextProperties(context), + $mcp_tool_name: call.tool, + $mcp_duration_ms: call.durationMs, + $mcp_is_error: !call.ok, + ...(call.ok ? {} : { $mcp_error_type: mcpErrorType(call.errorCode) }), + ...(args === undefined ? {} : { $mcp_parameters: args }), + ...(result === undefined ? {} : { $mcp_response: result }), + // LoopOver's own dimensions, carried ALONGSIDE the canonical keys rather than instead of them. + // PostHog's custom-server docs sanction this explicitly ("any extra props, spread verbatim, + // sitting alongside the $mcp_* keys"), and these three have no canonical equivalent: `surface` + // says which of the three servers answered, `transport` whether the stdio gateway proxied it, + // and `category` groups the ~125 tools. Without them on THIS event a breakdown of canonical + // tool calls by those dimensions is impossible -- `usage_event` carries them, but it is a + // different event, so the two cannot be combined in one breakdown. + surface: call.surface, + transport: call.transport ?? "local", + category: call.category, + payloads_excluded: payloads.excluded, }; } +/** + * `$mcp_tools_list` -- what the server advertised in a `tools/list` response. + * + * Names only, never descriptions or schemas. The array is COPIED because callers hand in their live + * registration array; a captured alias would let a later registration mutate an event already + * queued for flush. + */ +export function buildMcpToolsListProperties( + toolNames: readonly string[], + context: McpAnalyticsContext = {}, +): Record { + return { ...buildMcpContextProperties(context), $mcp_listed_tool_names: [...toolNames] }; +} + /** * OTel span attributes for one tool call. * diff --git a/packages/loopover-ui-kit/.prettierrc b/packages/loopover-ui-kit/.prettierrc new file mode 100644 index 0000000000..9c157ed5bf --- /dev/null +++ b/packages/loopover-ui-kit/.prettierrc @@ -0,0 +1,6 @@ +{ + "printWidth": 80, + "semi": true, + "singleQuote": false, + "trailingComma": "all" +} diff --git a/packages/loopover-ui-kit/src/components/chart.test.tsx b/packages/loopover-ui-kit/src/components/chart.test.tsx index 9cf80af706..801c0f1052 100644 --- a/packages/loopover-ui-kit/src/components/chart.test.tsx +++ b/packages/loopover-ui-kit/src/components/chart.test.tsx @@ -152,7 +152,8 @@ describe("ChartTooltipContent (recharts v3, #8610)", () => { const row = valueSpan!.closest(".flex.w-full"); expect(row).not.toBeNull(); const directZero = Array.from(row!.childNodes).some( - (node) => node.nodeType === Node.TEXT_NODE && node.textContent?.trim() === "0", + (node) => + node.nodeType === Node.TEXT_NODE && node.textContent?.trim() === "0", ); expect(directZero).toBe(false); expect(chart.getByText("Revenue")).toBeTruthy(); @@ -185,7 +186,11 @@ describe("ChartTooltipContent (recharts v3, #8610)", () => { }, ]; const nullish = renderInChart( - , + , ); expect(nullish.slot().textContent).toContain("Revenue"); expect(nullish.slot().querySelector("span.tabular-nums")).toBeNull(); diff --git a/packages/loopover-ui-kit/src/components/pagination.test.tsx b/packages/loopover-ui-kit/src/components/pagination.test.tsx index 4ee7899fb6..f6b10c818f 100644 --- a/packages/loopover-ui-kit/src/components/pagination.test.tsx +++ b/packages/loopover-ui-kit/src/components/pagination.test.tsx @@ -52,7 +52,7 @@ describe("PaginationLink aria-disabled styling (#8307)", () => { // #10052: aria-hidden on the outer wrapper removed the sr-only "More pages" label from the a11y tree. // Scope aria-hidden to the decorative icon only — same pattern as TypingIndicator. describe("PaginationEllipsis sr-only label not inside aria-hidden (#10052)", () => { - it("exposes \"More pages\" with no aria-hidden ancestor; icon is aria-hidden", () => { + it('exposes "More pages" with no aria-hidden ancestor; icon is aria-hidden', () => { const { container } = render(); const label = screen.getByText("More pages"); expect(label.closest("[aria-hidden='true'], [aria-hidden='']")).toBeNull(); diff --git a/src/mcp/dispatch-telemetry-sink.ts b/src/mcp/dispatch-telemetry-sink.ts index cff0c2bb7d..863ab8faf3 100644 --- a/src/mcp/dispatch-telemetry-sink.ts +++ b/src/mcp/dispatch-telemetry-sink.ts @@ -16,7 +16,16 @@ // argument for avoiding it here. (#9525's issue text assumed metagraphed's situation, where the // bundle cost was real.) import { capturePostHogWorkerError, isWorkerPostHogConfigured, type WorkerPostHogEnv } from "../api/worker-posthog"; -import { MCP_TOOL_CALL_EVENT, MCP_USAGE_EVENT } from "@loopover/contract"; +import { + buildMcpInitializeProperties, + buildMcpToolsListProperties, + MCP_INITIALIZE_EVENT, + MCP_TOOL_CALL_EVENT, + MCP_TOOLS_LIST_EVENT, + MCP_USAGE_EVENT, + type McpAnalyticsContext, + type McpInitializeTelemetry, +} from "@loopover/contract"; import type { DispatchTelemetrySink } from "./dispatch-telemetry"; import { getMcpDispatchSpanRunner } from "./dispatch-span-registry"; @@ -36,12 +45,15 @@ function trimmedOrUndefined(value: string | undefined): string | undefined { /** Deferred work the caller schedules via `waitUntil`, so a slow flush never delays the response. */ export type DeferWork = (work: Promise) => void; -/** Only ever reached from behind `recordToolCall`'s gate, so it takes the resolved key rather than - * re-deriving and re-checking it -- a second guard here would be unreachable by construction. */ +/** Only ever reached from behind a caller's own key gate, so it takes the resolved key rather than + * re-deriving and re-checking it -- a second guard here would be unreachable by construction. + * + * Takes a LIST of events rather than a fixed pair so the canonical handshake/tools-list events + * (#10175) share one client, one flush, and one best-effort boundary with the tool-call pair. */ async function captureEvents( env: DispatchTelemetryEnv, apiKey: string, - properties: { usage: Record; mcpToolCall: Record }, + events: readonly (readonly [string, Record])[], ): Promise { try { const { PostHog } = await import("posthog-node"); @@ -52,10 +64,7 @@ async function captureEvents( flushAt: 1, flushInterval: 0, }); - for (const [event, props] of [ - [MCP_USAGE_EVENT, properties.usage], - [MCP_TOOL_CALL_EVENT, properties.mcpToolCall], - ] as const) { + for (const [event, props] of events) { client.capture({ distinctId: MCP_TELEMETRY_DISTINCT_ID, event, properties: props, disableGeoip: true }); } await client.flush(); @@ -76,12 +85,14 @@ export function createDispatchTelemetrySink( env: DispatchTelemetryEnv, defer: DeferWork, withSpan?: (name: string, attributes: Record, fn: () => Promise) => Promise, + context: McpAnalyticsContext = {}, ): DispatchTelemetrySink { return { + context, recordToolCall: (_call, properties) => { const apiKey = trimmedOrUndefined(env.POSTHOG_API_KEY); if (!apiKey) return; - defer(captureEvents(env, apiKey, properties)); + defer(captureEvents(env, apiKey, [[MCP_USAGE_EVENT, properties.usage], [MCP_TOOL_CALL_EVENT, properties.mcpToolCall]])); }, captureException: (error, call) => { if (!isWorkerPostHogConfigured(env)) return; @@ -94,3 +105,37 @@ export function createDispatchTelemetrySink( withSpan: (name, attributes, fn) => (withSpan ?? getMcpDispatchSpanRunner() ?? ((_n, _a, run) => run()))(name, attributes, fn), }; } + +/** + * Record one `$mcp_initialize` handshake (#10175). + * + * Lives beside the tool-call sink because it shares its gate (POSTHOG_API_KEY), transport, anonymous + * distinct id, and best-effort contract -- only the event differs. It is deliberately NOT part of + * `DispatchTelemetrySink`: the handshake is observed at the HTTP layer, one level above the per-tool + * dispatch chokepoint that interface describes, so folding it in would make every sink implement a + * method no dispatch ever calls. + */ +export function recordMcpInitialize( + env: DispatchTelemetryEnv, + defer: DeferWork, + handshake: McpInitializeTelemetry, + context: McpAnalyticsContext = {}, +): void { + const apiKey = trimmedOrUndefined(env.POSTHOG_API_KEY); + if (!apiKey) return; + defer(captureEvents(env, apiKey, [[MCP_INITIALIZE_EVENT, buildMcpInitializeProperties(handshake, context)]])); +} + +/** Record one `$mcp_tools_list` (#10175). Same gate and contract as the handshake above; emitted from + * the HTTP layer for the same reason -- `tools/list` is a protocol request, not a tool dispatch, so + * no per-tool chokepoint ever sees it. */ +export function recordMcpToolsList( + env: DispatchTelemetryEnv, + defer: DeferWork, + toolNames: readonly string[], + context: McpAnalyticsContext = {}, +): void { + const apiKey = trimmedOrUndefined(env.POSTHOG_API_KEY); + if (!apiKey) return; + defer(captureEvents(env, apiKey, [[MCP_TOOLS_LIST_EVENT, buildMcpToolsListProperties(toolNames, context)]])); +} diff --git a/src/mcp/dispatch-telemetry.ts b/src/mcp/dispatch-telemetry.ts index 039aafedc9..70960bcce9 100644 --- a/src/mcp/dispatch-telemetry.ts +++ b/src/mcp/dispatch-telemetry.ts @@ -19,6 +19,7 @@ // does. Telemetry must never turn a working tool call into a failed one. import { buildMcpToolCallProperties, + type McpAnalyticsContext, buildMcpToolSpanAttributes, buildUsageEventProperties, getToolContract, @@ -49,6 +50,15 @@ export type DispatchTelemetrySink = { captureException: (error: unknown, call: McpToolCallTelemetry) => void; /** Wrap the call in a span when tracing is on; a no-op passthrough when it is not. */ withSpan: (name: string, attributes: Record, fn: () => Promise) => Promise; + /** + * Session/server/client identity for the canonical `$mcp_*` events (#10175). + * + * On the SINK rather than threaded through every handler because it is per-REQUEST, not per-call: + * the remote server already builds one sink per request (so deferred work rides that request's + * `waitUntil`), which is exactly the scope an MCP session id has. Optional so the stdio and miner + * sinks, which have no HTTP session, can omit it. + */ + context?: McpAnalyticsContext; }; /** A sink that does nothing, used when nothing is configured. Exported for tests. */ @@ -87,7 +97,7 @@ export function instrumentToolDispatch { // #9525: the dispatch chokepoint's sink is built per request so its deferred work rides this // request's waitUntil. No OTel span on the Worker path -- the collector is a self-host concern, and // importing src/selfhost/otel.ts here would pull it into the Worker bundle. - const server = new LoopoverMcp( - c.env, - identity, - createDispatchTelemetrySink(c.env, (work) => executionCtx.waitUntil(work)), - ).createServer(); + // #10175: session/server/client identity for PostHog's canonical $mcp_* events. `Mcp-Session-Id` is + // what ties one client's handshake, tools/list, and subsequent tool calls into a single analyzable + // session -- without it each event is an isolated row and no cross-event funnel works. + const analyticsContext = { + sessionId: trimmedHeader(c.req.raw.headers.get("mcp-session-id")), + serverName: MCP_SERVER_NAME, + serverVersion: LATEST_RECOMMENDED_MCP_VERSION, + clientName: telemetry.clientName, + clientVersion: telemetry.clientVersion, + }; + const defer = (work: Promise) => executionCtx.waitUntil(work); + const mcp = new LoopoverMcp(c.env, identity, createDispatchTelemetrySink(c.env, defer, undefined, analyticsContext)); + const server = mcp.createServer(); try { const response = await createMcpHandler(server, { route: "/mcp", enableJsonResponse: true })(c.req.raw, c.env, executionCtx); if (typeof usageMetadata.toolName === "string") { executionCtx.waitUntil(recordMcpToolTelemetry(c.env, usageMetadata.toolName, response.status < 400, Date.now() - startedAt)); } + // #10175: PostHog's canonical protocol-level events. Only on a request that actually succeeded, so + // a rejected handshake never inflates the session/client counts. + if (response.status < 400) { + if (usageMetadata.rpcMethod === "initialize") { + recordMcpInitialize(c.env, defer, await readInitializeHandshake(c.req.raw), analyticsContext); + } else if (usageMetadata.rpcMethod === "tools/list") { + // Names come from this server's own registration chokepoint, not the cross-server contract + // registry, so the event reports what was actually advertised to THIS client. + recordMcpToolsList(c.env, defer, mcp.registeredToolNames, analyticsContext); + } + } await recordProductUsageEvent(c.env, { surface: "mcp", role: "miner", @@ -730,6 +749,32 @@ async function recordMcpToolTelemetry(env: Env, tool: string, ok: boolean, durat } } +function trimmedHeader(value: string | null): string | undefined { + const trimmed = value?.trim(); + return trimmed ? trimmed : undefined; +} + +/** + * Read the `clientInfo` an `initialize` request introduced itself with (#10175). + * + * Deliberately reads the JSON-RPC params rather than reusing the `x-loopover-mcp-*` headers + * buildMcpClientTelemetry already parses: those are LoopOver's own convention, so only our published + * client sets them and every third-party MCP client (Claude Code, Cursor, a raw SDK script) would + * fall back to the "mcp" default and collapse into one indistinguishable bucket -- defeating the + * whole point of the event. `clientInfo` is the MCP spec's own field, sent by every conformant + * client. Returns an empty handshake for a malformed or absent body: the fields are optional by + * contract, and a session that connected is still worth counting. + */ +async function readInitializeHandshake(request: Request): Promise { + const body = await request.clone().json().catch(() => null); + if (!body || typeof body !== "object") return {}; + const clientInfo = (body as { params?: { clientInfo?: { name?: unknown; version?: unknown } } }).params?.clientInfo; + return { + clientName: typeof clientInfo?.name === "string" ? clientInfo.name : undefined, + clientVersion: typeof clientInfo?.version === "string" ? clientInfo.version : undefined, + }; +} + async function describeMcpUsageRequest(request: Request, telemetryMetadata: Record | undefined): Promise> { const body = await request.clone().json().catch(() => null); if (!body || typeof body !== "object") return { transport: "http", method: request.method, ...telemetryMetadata }; @@ -790,6 +835,11 @@ function isMcpAdminEnabled(env: Env): boolean { return /^(1|true|yes|on)$/i.test((env.LOOPOVER_MCP_ADMIN_ENABLED ?? "").trim()); } +/** The MCP `serverInfo.name` this server reports, and the `$mcp_server_name` its analytics carry. + * One constant so the handshake a client sees and the dashboards an operator reads can never + * disagree about what this server is called. */ +export const MCP_SERVER_NAME = "loopover"; + export class LoopoverMcp { private accessScopePromise: Promise | null = null; @@ -801,9 +851,14 @@ export class LoopoverMcp { private readonly telemetrySink?: DispatchTelemetrySink, ) {} + /** Tool names this server actually advertised, in registration order (#10175). Populated by the + * `register` wrapper below so `$mcp_tools_list` reports what THIS server exposes rather than the + * whole cross-server contract registry. */ + readonly registeredToolNames: string[] = []; + createServer(): McpServer { const server = new McpServer({ - name: "loopover", + name: MCP_SERVER_NAME, // #9526: derived, not a hand-bumped constant. LATEST_RECOMMENDED_MCP_VERSION already reads // @loopover/mcp's package.json, which the release automation owns -- so serverInfo, the compatibility // metadata, the server card, and server.json all report the one version that actually shipped. @@ -824,6 +879,7 @@ export class LoopoverMcp { // depending on which LoopOver surface you asked. Annotations were absent entirely, so a client that // gates confirmation on `destructiveHint` saw nothing for `loopover_delete_branch`. const register: McpServer["registerTool"] = (name, config, cb) => { + this.registeredToolNames.push(name); const advertised = getToolDefinition(name); /* v8 ignore next 2 -- unreachable while validate-mcp's "nothing registers without a contract entry" assertion holds; this throw is what keeps it unreachable. */ diff --git a/src/selfhost/posthog.ts b/src/selfhost/posthog.ts index 2ec5a760dc..6b1b08c543 100644 --- a/src/selfhost/posthog.ts +++ b/src/selfhost/posthog.ts @@ -417,9 +417,29 @@ export type PostHogAiGenerationEvent = { * parallel concern to error tracking, not a substitute gate for it. */ export function capturePostHogAiGeneration(event: PostHogAiGenerationEvent): void { if (!active || !client) return; + const operational = operationalProperties(event.context); + // #10185: the OTel trace this call already runs inside IS the trace PostHog should group by. + // operationalProperties has computed it (as plain `trace_id`/`span_id`) since #8296, but this + // function then minted a fresh randomUUID() for $ai_trace_id and threw it away -- so every + // generation became its own single-event trace and the review pipeline that produced them was + // never visible as one thing (13,428 events across 13,428 traces on the live project). + // + // withReviewPipelineSpan (./review-tracing.ts) wraps a whole review, so every provider attempt + // inside it -- both dual-review legs, every retry, every self-consistency run, the RAG embeddings + // -- shares that span's trace id and now nests under one PostHog trace. + // + // randomUUID() stays the fallback, NOT an error: AI_EMBED/AI_VISION/AI_ADVISORY and any call made + // outside a review span legitimately have no ambient trace, and an orphan trace is still a valid + // (if less useful) event. `$ai_span_id` is only set when there IS a real span to name. + const traceId = typeof operational.trace_id === "string" ? operational.trace_id : undefined; + const spanId = typeof operational.span_id === "string" ? operational.span_id : undefined; const properties: Record = { - ...operationalProperties(event.context), - $ai_trace_id: randomUUID(), + ...operational, + $ai_trace_id: traceId ?? randomUUID(), + ...(spanId === undefined ? {} : { $ai_span_id: spanId }), + // Names the node in the trace tree. Provider-and-kind rather than the tool/feature name, because + // that is what this function actually knows -- the feature lives in ai_usage_events, not here. + $ai_span_name: `ai.${event.requestKind}/${nonBlank(event.provider) ?? "unknown"}`, $ai_model: nonBlank(event.model) ?? "unknown", $ai_provider: nonBlank(event.provider) ?? "unknown", // PostHog's own $ai_generation schema reports latency in SECONDS, not ms. @@ -440,6 +460,13 @@ export function capturePostHogAiGeneration(event: PostHogAiGenerationEvent): voi distinctId: POSTHOG_DISTINCT_ID, event: event.requestKind === "embedding" ? "$ai_embedding" : "$ai_generation", properties, + // #10185: a real group, so "which repo costs the most in AI spend" is answerable in PostHog + // rather than only in the ai_usage_events SQL table. Deliberately reads the ALREADY-PROCESSED + // `repo` off operationalProperties rather than event.context: under the shared central key that + // value is HMAC-anonymized, and when the anon secret has not been injected the key is dropped + // entirely. Reusing it means the group inherits that fail-closed behavior for free -- a raw + // private repo name can never reach the group index by this path. + ...(typeof operational.repo === "string" ? { groups: { repo: operational.repo } } : {}), }); } diff --git a/test/unit/analytics-proxy.test.ts b/test/unit/analytics-proxy.test.ts index ecc0ba94cd..25f878d432 100644 --- a/test/unit/analytics-proxy.test.ts +++ b/test/unit/analytics-proxy.test.ts @@ -1,7 +1,21 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { handleAnalyticsProxy } from "../../apps/loopover-ui/src/lib/analytics-proxy"; -const UPSTREAM = "https://tasty.aethereal.dev"; +// The privacy invariants of the UI's first-party analytics relay, asserted from the ROOT suite -- +// deliberately a different vantage point from the module's own colocated tests +// (apps/loopover-ui/src/lib/analytics-proxy.test.ts), which cover the full routing/size/cache matrix. +// What is pinned here is only the small set of properties that must hold no matter how the proxy is +// refactored: no visitor cookie reaches the analytics host, no set-cookie reaches the browser, geo +// cannot be spoofed, and nothing outside the allowlist forwards at all. +// +// Repointed from Umami to PostHog in #8293/#8299. One assertion INVERTED in that move and is called +// out rather than quietly rewritten: the Umami-era version asserted that the remote tracker script +// was never proxied as first-party JavaScript. PostHog's documented proxy design requires exactly +// that (posthog-js bootstraps from /static/array.js), so the modern equivalent is that the SDK is +// served from PostHog's dedicated immutable ASSET host, never the capture host. + +const API = "https://us.i.posthog.com"; +const ASSETS = "https://us-assets.i.posthog.com"; function captureUpstream() { const calls: Array<{ url: string; init: RequestInit; headers: Headers }> = []; @@ -15,49 +29,39 @@ function captureUpstream() { }); return new Response("ok", { status: 200, - headers: { - "set-cookie": "umami=1", - "content-type": "application/json", - }, + headers: { "set-cookie": "ph=1", "content-type": "application/json" }, }); }, ); return calls; } +function capture(headers: Record) { + return new Request("https://loopover.ai/stats/i/v0/e/", { + method: "POST", + headers: { "content-length": "2", ...headers }, + body: "{}", + }); +} + describe("handleAnalyticsProxy", () => { afterEach(() => { vi.unstubAllGlobals(); }); - it("does not proxy the mutable remote analytics script as first-party JavaScript", async () => { - const calls = captureUpstream(); - - expect( - await handleAnalyticsProxy( - new Request("https://loopover.aethereal.dev/stats/script.js"), - ), - ).toBeUndefined(); - expect(calls).toHaveLength(0); - }); - it("does not forward the visitor's cookies to the analytics upstream", async () => { const calls = captureUpstream(); const response = await handleAnalyticsProxy( - new Request("https://loopover.aethereal.dev/stats/api/send", { - method: "POST", - headers: { - cookie: "loopover_session=secret; gh_oauth_state=abc", - "cf-connecting-ip": "203.0.113.7", - }, - body: "{}", + capture({ + cookie: "loopover_session=secret; gh_oauth_state=abc", + "cf-connecting-ip": "203.0.113.7", }), ); expect(response?.status).toBe(200); expect(calls).toHaveLength(1); - expect(calls[0]?.url).toBe(`${UPSTREAM}/api/send`); - // The first-party cookie must never reach the analytics host. + expect(calls[0]?.url).toBe(`${API}/i/v0/e/`); + // The first-party session cookie must never reach the analytics host. expect(calls[0]?.headers.has("cookie")).toBe(false); // The upstream set-cookie must never be relayed back to the browser. expect(response?.headers.has("set-cookie")).toBe(false); @@ -66,39 +70,46 @@ describe("handleAnalyticsProxy", () => { it("forwards only the trusted client IP, ignoring a spoofed x-forwarded-for", async () => { const calls = captureUpstream(); await handleAnalyticsProxy( - new Request("https://loopover.aethereal.dev/stats/api/send", { - method: "POST", - headers: { - "x-forwarded-for": "1.2.3.4", - "cf-connecting-ip": "203.0.113.7", - "content-type": "application/json", - }, - body: "{}", + capture({ + "x-forwarded-for": "1.2.3.4", + "cf-connecting-ip": "203.0.113.7", + "content-type": "application/json", }), ); expect(calls).toHaveLength(1); - expect(calls[0]?.url).toBe(`${UPSTREAM}/api/send`); expect(calls[0]?.headers.get("x-forwarded-for")).toBe("203.0.113.7"); }); - it("returns undefined for non-allowlisted paths and 405 for disallowed methods", async () => { - captureUpstream(); + it("serves the SDK bundle from PostHog's asset host, never the capture host", async () => { + const calls = captureUpstream(); + + await handleAnalyticsProxy( + new Request("https://loopover.ai/stats/static/array.js"), + ); + + expect(calls).toHaveLength(1); + expect(calls[0]?.url).toBe(`${ASSETS}/static/array.js`); + }); + + it("returns undefined for anything outside the allowlist, without touching the network", async () => { + const calls = captureUpstream(); + + // Not a PostHog ingest path -- including the retired Umami collect endpoint. expect( await handleAnalyticsProxy( - new Request("https://loopover.aethereal.dev/stats/api/admin"), + new Request("https://loopover.ai/stats/api/send"), ), ).toBeUndefined(); expect( await handleAnalyticsProxy( - new Request("https://loopover.aethereal.dev/about"), + new Request("https://loopover.ai/stats/admin"), ), ).toBeUndefined(); - const notAllowed = await handleAnalyticsProxy( - new Request("https://loopover.aethereal.dev/stats/api/send", { - method: "GET", - }), - ); - expect(notAllowed?.status).toBe(405); + // Not an analytics path at all. + expect( + await handleAnalyticsProxy(new Request("https://loopover.ai/about")), + ).toBeUndefined(); + expect(calls).toHaveLength(0); }); }); diff --git a/test/unit/mcp-dispatch-telemetry-sink.test.ts b/test/unit/mcp-dispatch-telemetry-sink.test.ts index 7f0c82ae84..4b6a1c30a6 100644 --- a/test/unit/mcp-dispatch-telemetry-sink.test.ts +++ b/test/unit/mcp-dispatch-telemetry-sink.test.ts @@ -4,7 +4,7 @@ // both sides of every gate, and the guarantee that a sink failure never reaches the tool caller. import { afterEach, describe, expect, it, vi } from "vitest"; import type { McpToolCallTelemetry } from "@loopover/contract"; -import { createDispatchTelemetrySink, type DispatchTelemetryEnv } from "../../src/mcp/dispatch-telemetry-sink"; +import { createDispatchTelemetrySink, recordMcpInitialize, recordMcpToolsList, type DispatchTelemetryEnv } from "../../src/mcp/dispatch-telemetry-sink"; import { getMcpDispatchSpanRunner, resetMcpDispatchSpanRunnerForTest, @@ -149,3 +149,67 @@ describe("LoopoverMcp telemetry-sink injection (#9525)", () => { expect(recorded[0]).toMatchObject({ tool: "loopover_get_repo_context", category: "maintainer", surface: "remote" }); }, 30_000); }); + +describe("PostHog canonical MCP event recorders (#10175)", () => { + const handshake = { clientName: "claude-code", clientVersion: "1.2.3" }; + const context = { sessionId: "ses_abc", serverName: "loopover", serverVersion: "3.18.4" }; + + it("records no handshake and defers nothing when POSTHOG_API_KEY is unset", () => { + const deferred: Promise[] = []; + recordMcpInitialize(env(), (work) => deferred.push(work), handshake, context); + expect(deferred).toEqual([]); + }); + + it("treats a blank POSTHOG_API_KEY as unset for the handshake", () => { + const deferred: Promise[] = []; + recordMcpInitialize(env({ POSTHOG_API_KEY: " " }), (work) => deferred.push(work), handshake, context); + expect(deferred).toEqual([]); + }); + + it("defers one handshake capture when the key is set, and never rejects", async () => { + const deferred: Promise[] = []; + recordMcpInitialize(env({ POSTHOG_API_KEY: "phc_test", POSTHOG_HOST: "http://127.0.0.1:1" }), (work) => deferred.push(work), handshake, context); + expect(deferred).toHaveLength(1); + await expect(deferred[0]).resolves.toBeUndefined(); + }); + + it("records a handshake with no context argument at all (the default)", async () => { + const deferred: Promise[] = []; + recordMcpInitialize(env({ POSTHOG_API_KEY: "phc_test", POSTHOG_HOST: "http://127.0.0.1:1" }), (work) => deferred.push(work), {}); + expect(deferred).toHaveLength(1); + await expect(deferred[0]).resolves.toBeUndefined(); + }); + + it("records no tools/list and defers nothing when POSTHOG_API_KEY is unset", () => { + const deferred: Promise[] = []; + recordMcpToolsList(env(), (work) => deferred.push(work), ["a_tool"], context); + expect(deferred).toEqual([]); + }); + + it("treats a blank POSTHOG_API_KEY as unset for tools/list", () => { + const deferred: Promise[] = []; + recordMcpToolsList(env({ POSTHOG_API_KEY: " " }), (work) => deferred.push(work), ["a_tool"], context); + expect(deferred).toEqual([]); + }); + + it("defers one tools/list capture when the key is set, and never rejects", async () => { + const deferred: Promise[] = []; + recordMcpToolsList(env({ POSTHOG_API_KEY: "phc_test", POSTHOG_HOST: "http://127.0.0.1:1" }), (work) => deferred.push(work), ["a_tool", "b_tool"], context); + expect(deferred).toHaveLength(1); + await expect(deferred[0]).resolves.toBeUndefined(); + }); + + it("records a tools/list with no context argument at all (the default)", async () => { + const deferred: Promise[] = []; + recordMcpToolsList(env({ POSTHOG_API_KEY: "phc_test", POSTHOG_HOST: "http://127.0.0.1:1" }), (work) => deferred.push(work), []); + expect(deferred).toHaveLength(1); + await expect(deferred[0]).resolves.toBeUndefined(); + }); + + it("carries the per-request analytics context on the sink, defaulting to an empty one", () => { + // The context lives on the sink because it is per-REQUEST: the remote server builds one sink per + // request, which is exactly the scope an MCP session id has. + expect(createDispatchTelemetrySink(env(), () => undefined).context).toEqual({}); + expect(createDispatchTelemetrySink(env(), () => undefined, undefined, context).context).toBe(context); + }); +}); diff --git a/test/unit/mcp-dispatch-telemetry.test.ts b/test/unit/mcp-dispatch-telemetry.test.ts index 52e5362b42..90563cf5d9 100644 --- a/test/unit/mcp-dispatch-telemetry.test.ts +++ b/test/unit/mcp-dispatch-telemetry.test.ts @@ -13,6 +13,11 @@ import { MCP_TELEMETRY_ERROR_CODES, MCP_TELEMETRY_PAYLOAD_BYTE_CAP, MCP_TELEMETRY_PROPERTY_KEYS, + MCP_CANONICAL_PROPERTY_KEYS, + MCP_ANALYTICS_SOURCE, + buildMcpInitializeProperties, + buildMcpToolsListProperties, + mcpErrorType, mcpToolSpanName, REDACTED, redactForTelemetry, @@ -47,24 +52,24 @@ describe("MCP telemetry event shapes (#9525)", () => { it("marks a payload-excluded tool explicitly rather than silently omitting", () => { const excluded = buildMcpToolCallProperties(call, { arguments: { a: 1 }, result: { b: 2 }, excluded: true }); expect(excluded.payloads_excluded).toBe(true); - expect(excluded.arguments).toBeUndefined(); - expect(excluded.result).toBeUndefined(); + expect(excluded.$mcp_parameters).toBeUndefined(); + expect(excluded.$mcp_response).toBeUndefined(); }); it("includes redacted payloads for a tool that permits them", () => { const included = buildMcpToolCallProperties(call, { arguments: { owner: "acme" }, result: undefined, excluded: false }); expect(included.payloads_excluded).toBe(false); - expect(included.arguments).toBe('{"owner":"acme"}'); - expect(included.result).toBeUndefined(); + expect(included.$mcp_parameters).toBe('{"owner":"acme"}'); + expect(included.$mcp_response).toBeUndefined(); }); it("includes a result with no arguments, and omits each independently", () => { const resultOnly = buildMcpToolCallProperties(call, { arguments: undefined, result: { n: 1 }, excluded: false }); - expect(resultOnly.arguments).toBeUndefined(); - expect(resultOnly.result).toBe('{"n":1}'); + expect(resultOnly.$mcp_parameters).toBeUndefined(); + expect(resultOnly.$mcp_response).toBe('{"n":1}'); const neither = buildMcpToolCallProperties(call, { excluded: false }); - expect("arguments" in neither).toBe(false); - expect("result" in neither).toBe(false); + expect("$mcp_parameters" in neither).toBe(false); + expect("$mcp_response" in neither).toBe(false); }); it("omits error_code from span attributes on a successful call", () => { @@ -157,17 +162,32 @@ describe("MCP telemetry error codes (#9525)", () => { describe("MCP telemetry allowlist (#9525)", () => { it("emits no property key outside the single-sourced allowlist", () => { - const allowed = new Set(MCP_TELEMETRY_PROPERTY_KEYS); + // TWO vocabularies, deliberately (#10175): LoopOver's snake_case keys on `usage_event` and the + // OTel span, PostHog's reserved `$mcp_*` keys on the canonical events. `$mcp_tool_call` draws + // from BOTH -- the canonical keys their dashboards read, plus the three LoopOver dimensions with + // no canonical equivalent -- so it is checked against the union. Nothing may emit a key outside. + const native = new Set(MCP_TELEMETRY_PROPERTY_KEYS); + const canonical = new Set(MCP_CANONICAL_PROPERTY_KEYS); + const union = new Set([...native, ...canonical]); const failing: McpToolCallTelemetry = { ...call, ok: false, errorCode: "timeout" }; - const payloads = [ - buildUsageEventProperties(call), - buildUsageEventProperties(failing), - buildMcpToolCallProperties(call, { arguments: { a: 1 }, result: { b: 2 }, excluded: false }), + const ctx = { sessionId: "ses_abc", serverName: "loopover", serverVersion: "3.18.4", clientName: "claude-code", clientVersion: "1.2.3" }; + + for (const payload of [buildUsageEventProperties(call), buildUsageEventProperties(failing), buildMcpToolSpanAttributes(failing)]) { + for (const key of Object.keys(payload)) expect(native, `${key} is not allowlisted`).toContain(key); + } + for (const payload of [ + buildMcpToolCallProperties(call, { arguments: { a: 1 }, result: { b: 2 }, excluded: false }, ctx), buildMcpToolCallProperties(failing, { arguments: { a: 1 }, excluded: true }), - buildMcpToolSpanAttributes(failing), - ]; - for (const payload of payloads) { - for (const key of Object.keys(payload)) expect(allowed, `${key} is not allowlisted`).toContain(key); + ]) { + for (const key of Object.keys(payload)) expect(union, `${key} is not allowlisted`).toContain(key); + } + for (const payload of [ + buildMcpInitializeProperties({ clientName: "claude-code", clientVersion: "1.2.3" }, ctx), + buildMcpInitializeProperties({}), + buildMcpToolsListProperties(["loopover_get_repo_context"], ctx), + buildMcpToolsListProperties([]), + ]) { + for (const key of Object.keys(payload)) expect(canonical, `${key} is not allowlisted`).toContain(key); } }); @@ -328,3 +348,108 @@ describe("MCP dispatch chokepoint (#9525)", () => { expect(calls[0]).toMatchObject({ category: "unknown" }); }); }); + +describe("PostHog canonical MCP analytics contract (#10175)", () => { + const ctx = { sessionId: "ses_abc123", serverName: "loopover", serverVersion: "3.18.4", clientName: "claude-code", clientVersion: "1.2.3" }; + + it("emits $mcp_tool_call under PostHog's own property names, not LoopOver's", () => { + // The whole point of this event: PostHog's built-in MCP dashboards read these exact `$mcp_*` + // keys literally. Emitting the event NAME with LoopOver-native keys ingests fine and leaves + // every breakdown empty -- which is precisely the bug #10175 fixes. + const properties = buildMcpToolCallProperties(call, { excluded: true }, ctx); + expect(properties.$mcp_source).toBe(MCP_ANALYTICS_SOURCE); + expect(properties.$mcp_tool_name).toBe("loopover_get_repo_context"); + expect(properties.$mcp_duration_ms).toBe(12); + expect(properties.$mcp_is_error).toBe(false); + expect(properties.$session_id).toBe("ses_abc123"); + expect(properties.$mcp_server_name).toBe("loopover"); + expect(properties.$mcp_server_version).toBe("3.18.4"); + expect(properties.$mcp_client_name).toBe("claude-code"); + expect(properties.$mcp_client_version).toBe("1.2.3"); + // LoopOver's own dimensions ride alongside rather than replacing the canonical ones. + expect(properties.surface).toBe("remote"); + expect(properties.transport).toBe("local"); + expect(properties.category).toBe("maintainer"); + }); + + it("omits $mcp_error_type on success and sets it on failure", () => { + expect("$mcp_error_type" in buildMcpToolCallProperties(call, { excluded: true })).toBe(false); + const failed = buildMcpToolCallProperties({ ...call, ok: false, errorCode: "rate_limited" }, { excluded: true }); + expect(failed.$mcp_is_error).toBe(true); + expect(failed.$mcp_error_type).toBe("rate_limited"); + }); + + it("projects every LoopOver error code onto a member of PostHog's closed $mcp_error_type set", () => { + // PostHog groups failures by this fixed set; a code mapping outside it would silently fall out + // of their error breakdown entirely. + const posthogTypes = new Set(["missing_context", "validation", "permission", "timeout", "rate_limited", "api_4xx", "api_5xx", "internal"]); + for (const code of MCP_TELEMETRY_ERROR_CODES) { + expect(posthogTypes, `${code} maps outside PostHog's set`).toContain(mcpErrorType(code)); + } + // No code at all is still a failure by construction at every call site, and PostHog's set has no + // "unclassified" member, so it buckets as the fault type. + expect(mcpErrorType(undefined)).toBe("internal"); + }); + + it("defaults the context to an empty one, still stamping $mcp_source", () => { + const properties = buildMcpToolCallProperties(call, { excluded: true }); + expect(properties.$mcp_source).toBe(MCP_ANALYTICS_SOURCE); + for (const key of ["$session_id", "$mcp_server_name", "$mcp_server_version", "$mcp_client_name", "$mcp_client_version"]) { + expect(key in properties, `${key} should be omitted, not null`).toBe(false); + } + }); + + it("omits blank and whitespace-only context values rather than emitting empty dimensions", () => { + const properties = buildMcpToolCallProperties(call, { excluded: true }, { sessionId: " ", serverName: "", clientName: "cursor" }); + expect("$session_id" in properties).toBe(false); + expect("$mcp_server_name" in properties).toBe(false); + expect(properties.$mcp_client_name).toBe("cursor"); + }); + + it("truncates an over-long client-supplied label so it cannot blow up a dashboard dimension", () => { + const properties = buildMcpToolCallProperties(call, { excluded: true }, { clientName: "x".repeat(500) }); + expect(properties.$mcp_client_name).toBe("x".repeat(256)); + }); + + it("prefers the handshake's own clientInfo over anything inferred from headers", () => { + // clientInfo is the MCP spec's own field, sent by the client about itself; the header-derived + // value is a LoopOver convention only our own published client sets. + const properties = buildMcpInitializeProperties({ clientName: "cursor", clientVersion: "9.9.9" }, { ...ctx }); + expect(properties.$mcp_client_name).toBe("cursor"); + expect(properties.$mcp_client_version).toBe("9.9.9"); + expect(properties.$mcp_server_name).toBe("loopover"); + }); + + it("falls back to the inferred client when the handshake omits clientInfo", () => { + const properties = buildMcpInitializeProperties({}, ctx); + expect(properties.$mcp_client_name).toBe("claude-code"); + expect(properties.$mcp_client_version).toBe("1.2.3"); + }); + + it("still emits a handshake with no client identity at all", () => { + // A client that omits clientInfo completes a valid handshake -- a session that connected is + // still worth counting, in an explicit "unknown client" bucket. + expect(buildMcpInitializeProperties({})).toEqual({ $mcp_source: MCP_ANALYTICS_SOURCE }); + }); + + it("lists advertised tool names for the discovery join", () => { + const properties = buildMcpToolsListProperties(["a_tool", "b_tool"], ctx); + expect(properties.$mcp_listed_tool_names).toEqual(["a_tool", "b_tool"]); + expect(properties.$session_id).toBe("ses_abc123"); + }); + + it("copies the tool-name array rather than aliasing the caller's own", () => { + // The server hands in its live registration array; a captured alias would let a later + // registration mutate an event already queued for flush. + const names = ["a_tool"]; + const properties = buildMcpToolsListProperties(names); + names.push("b_tool"); + expect(properties.$mcp_listed_tool_names).toEqual(["a_tool"]); + }); + + it("emits an empty advertised list rather than omitting the property", () => { + // A server advertising nothing is a real, diagnosable state; an absent key reads as "not + // measured" instead. + expect(buildMcpToolsListProperties([]).$mcp_listed_tool_names).toEqual([]); + }); +}); diff --git a/test/unit/mcp-local-telemetry-chokepoint.test.ts b/test/unit/mcp-local-telemetry-chokepoint.test.ts index ed0e2953c3..e2c3b40d60 100644 --- a/test/unit/mcp-local-telemetry-chokepoint.test.ts +++ b/test/unit/mcp-local-telemetry-chokepoint.test.ts @@ -91,6 +91,19 @@ afterEach(async () => { configDir = null; }); +// #10175: the three events per call now speak TWO deliberate vocabularies -- `usage_event` and the +// legacy `mcp_tool_call` keep LoopOver's snake_case keys, while `$mcp_tool_call` carries PostHog's +// reserved `$mcp_*` names because their built-in dashboards read those literally. These read +// whichever one an event uses, so the invariants below stay about BEHAVIOR (which tool, did it +// succeed) rather than about key spelling. +function toolOf(event: { event?: string; properties?: Record }): unknown { + return event.properties?.$mcp_tool_name ?? event.properties?.tool; +} +function okOf(event: { event?: string; properties?: Record }): unknown { + const isError = event.properties?.$mcp_is_error; + return isError === undefined ? event.properties?.ok : !isError; +} + 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-")); @@ -150,19 +163,27 @@ describe("loopover-mcp local telemetry chokepoint (#6238)", () => { // No `arguments`/`result`: payloads are excluded for every tool by default (#9525). This test // is what established that -- the first design included them, and this assertion found a real // commit message on the wire. - $mcp_tool_call: ["category", "duration_ms", "ok", "payloads_excluded", "surface", "tool", "transport"], + $mcp_tool_call: ["category", "payloads_excluded", "surface", "transport"], + }; + // #10175: the `$mcp_*` keys `$mcp_tool_call` is REQUIRED to carry, because PostHog's built-in MCP + // dashboards read these names literally. Asserted separately from the vendor's own `$` metadata + // so a canonical key can never be mistaken for library noise (or vice versa). + // `$session_id`/`$mcp_server_*`/`$mcp_client_*` are absent on purpose: the stdio server has no + // HTTP session, so it passes no analytics context. + const canonicalByEvent: Record = { + mcp_tool_call: [], + usage_event: [], + $mcp_tool_call: ["$mcp_duration_ms", "$mcp_is_error", "$mcp_source", "$mcp_tool_name"], }; + // The PostHog SDK's own library metadata -- vendor provenance, not anything about the user. + const vendorKeys = ["$geoip_disable", "$is_server", "$lib", "$lib_version"]; for (const event of received) { const properties = Object.keys(event.properties ?? {}); const ours = properties.filter((key) => !key.startsWith("$")).sort(); const allowed = allowedByEvent[event.event]!; expect(ours.filter((key) => !allowed.includes(key)), `${event.event} carries a field outside the allowlist`).toEqual([]); - expect(properties.filter((key) => key.startsWith("$") && key !== "$mcp_tool_call").sort()).toEqual([ - "$geoip_disable", - "$is_server", - "$lib", - "$lib_version", - ]); + expect(properties.filter((key) => key.startsWith("$") && !vendorKeys.includes(key)).sort()).toEqual(canonicalByEvent[event.event]!); + expect(properties.filter((key) => vendorKeys.includes(key)).sort()).toEqual(vendorKeys); 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"); @@ -188,7 +209,7 @@ describe("loopover-mcp local telemetry chokepoint (#6238)", () => { // process emitter would produce three here, not six. expect(received).toHaveLength(6); expect(received.filter((event) => event.event === "usage_event")).toHaveLength(2); - expect(received.every((event) => event.properties?.tool === "loopover_lint_pr_text")).toBe(true); + expect(received.every((event) => toolOf(event) === "loopover_lint_pr_text")).toBe(true); }, 45_000); it("`telemetry disable` returns the server to sending nothing", async () => { @@ -225,8 +246,8 @@ describe("loopover-mcp local telemetry chokepoint (#6238)", () => { // rather than answering (#9525). Every one of them reports ok=false, and the exception carries // the grouping properties an operator can actually act on -- the tool and the closed error code // -- and nothing about the call's content. - expect(received.every((event) => event.event === "$exception" || event.properties?.ok === false)).toBe(true); - expect(received.every((event) => event.event === "$exception" || event.properties?.tool === "loopover_get_repo_context")).toBe(true); + expect(received.every((event) => event.event === "$exception" || okOf(event) === false)).toBe(true); + expect(received.every((event) => event.event === "$exception" || toolOf(event) === "loopover_get_repo_context")).toBe(true); const usage = received.find((event) => event.event === "usage_event")!; expect(usage.properties?.error_code).toBeTypeOf("string"); }, 45_000); diff --git a/test/unit/selfhost-posthog.test.ts b/test/unit/selfhost-posthog.test.ts index e7ff7d87de..247c15ef6a 100644 --- a/test/unit/selfhost-posthog.test.ts +++ b/test/unit/selfhost-posthog.test.ts @@ -601,6 +601,54 @@ describe("withPostHogMonitor", () => { describe("capturePostHogAiGeneration (#8296)", () => { const BASE = { provider: "ollama", model: "llama3.1", requestKind: "review" as const, latencyMs: 1500, isError: false }; + // #10185: trace linking + repo grouping. These are what turn a pile of orphan generations into a + // reviewable pipeline and an answerable spend question, so both branches of each are pinned. + it("groups the generation under the AMBIENT OTel trace rather than minting a throwaway one", async () => { + otelMocks.currentOtelTraceIds.mockReturnValue({ trace_id: "review-trace-1", span_id: "provider-span-1" }); + await initPostHog({ POSTHOG_API_KEY: "phc_test_key" } as unknown as NodeJS.ProcessEnv); + capturePostHogAiGeneration(BASE); + const call = mocks.capture.mock.calls[0]?.[0]; + // The whole point: every provider attempt inside one withReviewPipelineSpan shares this id, so + // both dual-review legs, the retries, and the RAG embeddings nest under ONE PostHog trace. + expect(call.properties.$ai_trace_id).toBe("review-trace-1"); + expect(call.properties.$ai_span_id).toBe("provider-span-1"); + expect(call.properties.$ai_span_name).toBe("ai.review/ollama"); + }); + + it("falls back to a minted trace id, and omits $ai_span_id, when there is no ambient span", async () => { + // AI_EMBED / AI_VISION / AI_ADVISORY run outside any review span. An orphan trace is a valid + // event, not an error -- but it must not claim a span id it does not have. + otelMocks.currentOtelTraceIds.mockReturnValue(undefined); + await initPostHog({ POSTHOG_API_KEY: "phc_test_key" } as unknown as NodeJS.ProcessEnv); + capturePostHogAiGeneration(BASE); + const call = mocks.capture.mock.calls[0]?.[0]; + expect(call.properties.$ai_trace_id).toEqual(expect.any(String)); + expect(call.properties.$ai_trace_id).not.toBe(""); + expect("$ai_span_id" in call.properties).toBe(false); + }); + + it("names an embedding span by its own request kind", async () => { + otelMocks.currentOtelTraceIds.mockReturnValue({ trace_id: "t", span_id: "s" }); + await initPostHog({ POSTHOG_API_KEY: "phc_test_key" } as unknown as NodeJS.ProcessEnv); + capturePostHogAiGeneration({ ...BASE, requestKind: "embedding", provider: "" }); + expect(mocks.capture.mock.calls[0]?.[0].properties.$ai_span_name).toBe("ai.embedding/unknown"); + }); + + it("stamps a repo group so AI spend is attributable per repository", async () => { + await initPostHog({ POSTHOG_API_KEY: "phc_test_key" } as unknown as NodeJS.ProcessEnv); + capturePostHogAiGeneration({ ...BASE, context: { repo: "owner/repo", pullNumber: 7 } }); + const call = mocks.capture.mock.calls[0]?.[0]; + expect(call.groups).toEqual({ repo: "owner/repo" }); + }); + + it("sends no group at all when no repo survives the operational allowlist", async () => { + // Notably the fail-closed central-key path: when the repo is dropped rather than anonymized, + // the group must be dropped with it rather than defaulting to some placeholder bucket. + await initPostHog({ POSTHOG_API_KEY: "phc_test_key" } as unknown as NodeJS.ProcessEnv); + capturePostHogAiGeneration(BASE); + expect("groups" in mocks.capture.mock.calls[0]?.[0]).toBe(false); + }); + it("is a no-op when PostHog is unconfigured", () => { capturePostHogAiGeneration(BASE); expect(mocks.capture).not.toHaveBeenCalled();