From 9a2137b2cf1605f84742cdd2df30daec5c2dd6d3 Mon Sep 17 00:00:00 2001 From: mintaka Date: Wed, 9 Sep 2026 16:28:03 -0400 Subject: [PATCH] feat(ui): emit the PostHog session id on outbound requests (RIG-2874) --- apps/ui/src/analytics/analytics.test.ts | 61 ++++ apps/ui/src/analytics/analytics.ts | 16 +- apps/ui/src/index.tsx | 62 ++-- apps/ui/src/live/client.test.ts | 21 ++ apps/ui/src/live/client.ts | 12 +- packages/compass-client/src/index.test.ts | 357 +++++++++++++++++++++- packages/compass-client/src/index.ts | 135 +++++++- 7 files changed, 616 insertions(+), 48 deletions(-) diff --git a/apps/ui/src/analytics/analytics.test.ts b/apps/ui/src/analytics/analytics.test.ts index e2bb13fd5..24da649e6 100644 --- a/apps/ui/src/analytics/analytics.test.ts +++ b/apps/ui/src/analytics/analytics.test.ts @@ -14,6 +14,7 @@ interface FakePostHog { capture: (...args: unknown[]) => void; identify: (...args: unknown[]) => void; reset: (...args: unknown[]) => void; + get_session_id: (...args: unknown[]) => string; } function makeFake(): FakePostHog { @@ -29,6 +30,10 @@ function makeFake(): FakePostHog { capture: record("capture"), identify: record("identify"), reset: record("reset"), + get_session_id: () => { + calls.push({ method: "get_session_id", args: [] }); + return ""; + }, }; } @@ -46,6 +51,38 @@ describe("createAnalytics", () => { expect(fake.calls).toHaveLength(0); }); + test("disabled sessionId returns undefined with ZERO posthog calls", () => { + const fake = makeFake(); + const analytics = createAnalytics(undefined, { + posthog: fake as unknown as PostHog, + }); + + expect(analytics.sessionId()).toBeUndefined(); + expect(fake.calls).toHaveLength(0); + }); + + test("enabled sessionId delegates to posthog", () => { + const fake = makeFake(); + fake.get_session_id = () => "session-123"; + const analytics = createAnalytics( + { key: "phc_abc", host: "https://us.i.posthog.com" }, + { posthog: fake as unknown as PostHog }, + ); + + expect(analytics.sessionId()).toBe("session-123"); + }); + + test("enabled sessionId maps an empty posthog id to undefined", () => { + const fake = makeFake(); + fake.get_session_id = () => ""; + const analytics = createAnalytics( + { key: "phc_abc", host: "https://us.i.posthog.com" }, + { posthog: fake as unknown as PostHog }, + ); + + expect(analytics.sessionId()).toBeUndefined(); + }); + test("disabled with NO deps (the production shape) is a callable no-op", () => { // index.tsx calls createAnalytics(analyticsConfigFromEnv()) with no deps, // so the disabled production path is createAnalytics(undefined) — the real @@ -109,6 +146,30 @@ describe("createAnalytics", () => { expect(identifies).toHaveLength(1); expect(identifies[0]?.args).toEqual(["acct-1"]); }); + + // The session id ROTATES under the app (posthog mints a new one on idle and + // at max length), so the value must be re-read per call and never memoized. + // The interceptor side is pinned for this too, but a memo added HERE would + // defeat that: the interceptor would faithfully re-read a stale cache. + test("enabled sessionId re-reads posthog on every call, never memoizing", () => { + const fake = makeFake(); + const ids = ["sess-1", "sess-2"]; + let call = 0; + fake.get_session_id = () => { + fake.calls.push({ method: "get_session_id", args: [] }); + return ids[call++] ?? ""; + }; + const analytics = createAnalytics( + { key: "phc_abc", host: "https://us.i.posthog.com" }, + { posthog: fake as unknown as PostHog }, + ); + + expect(analytics.sessionId()).toBe("sess-1"); + expect(analytics.sessionId()).toBe("sess-2"); + expect( + fake.calls.filter((c) => c.method === "get_session_id"), + ).toHaveLength(2); + }); }); describe("$ai_trace_id stamping", () => { diff --git a/apps/ui/src/analytics/analytics.ts b/apps/ui/src/analytics/analytics.ts index 941a9f66c..379e84643 100644 --- a/apps/ui/src/analytics/analytics.ts +++ b/apps/ui/src/analytics/analytics.ts @@ -27,6 +27,8 @@ export interface Analytics { capture(event: string, props?: Record): void; /** Associate subsequent events with a stable distinct id (the caller). */ identify(distinctId: string): void; + /** Return the current PostHog session id, when one exists. */ + sessionId(): string | undefined; /** Tear down the identified session (logout / app teardown). */ shutdown(): void; } @@ -36,6 +38,9 @@ export interface Analytics { class NoopAnalytics implements Analytics { capture(): void {} identify(): void {} + sessionId(): string | undefined { + return undefined; + } shutdown(): void {} } @@ -45,8 +50,10 @@ class NoopAnalytics implements Analytics { class PostHogAnalytics implements Analytics { private readonly client: PostHog; /** The trace-id source, read at CAPTURE time rather than construction time: - * the transport that records trace ids is built before this client exists, - * so a value read once at construction would always be undefined. + * boot builds analytics BEFORE the transport, so this getter closes over a + * `clients` binding that is not yet initialized — reading it at construction + * would throw a ReferenceError, while reading it at capture time is long + * after boot bound it. * * A getter, not the sink object, on purpose — analytics reads one string and * has no business depending on compass-client's transport types, so the @@ -106,6 +113,11 @@ class PostHogAnalytics implements Analytics { this.client.identify(distinctId); } + sessionId(): string | undefined { + const sessionId = this.client.get_session_id(); + return sessionId === "" ? undefined : sessionId; + } + shutdown(): void { // PostHog's de-identify: reset the distinct id and start a fresh // anonymous session. The browser SDK batch-sends on its own; there is no diff --git a/apps/ui/src/index.tsx b/apps/ui/src/index.tsx index 6574779f5..65bfab74b 100644 --- a/apps/ui/src/index.tsx +++ b/apps/ui/src/index.tsx @@ -88,39 +88,53 @@ async function main( root: HTMLElement, connection: ResolvedConnection, ): Promise { - const clients = createLiveClients(connection); - - const callerId = await bootCaller(root, () => resolveCaller(clients.compass)); - // Undefined is bootCaller's stop signal — it already painted the WhoAmI - // failure screen, so the app must not come up (no caller to scope it). - if (!callerId) { - return; - } - // Product analytics, OFF by default: analyticsConfigFromEnv returns undefined // unless a PostHog project key is configured, and createAnalytics then hands // back a no-op that never touches posthog — an unconfigured deployment emits - // zero analytics. Identify the caller we just learned via WhoAmI so events - // attach to a stable distinct id. + // zero analytics. // - // The inbound half of correlation is wired here: `clients.traceId` is the slot - // the transport records each reply's server trace id into, and analytics reads - // it at capture time. Reading through a getter is what makes the ordering work - // — the clients exist before this line, but the first trace id only lands once - // a call has returned. + // Built FIRST, before the clients, because correlation now runs in both + // directions and the outbound half needs a real analytics object to read + // from. Both directions are lazy getters, and they point opposite ways: // - // Best-effort by construction, on two counts. The slot holds the LAST reply's - // trace id, so an event fired before any call has returned carries nothing, - // and one fired between calls carries the previous call's trace rather than - // its own. And the server sets `traceresponse` only on UNARY replies, and - // only when an OTel provider is installed — an unconfigured deployment - // (empty exporter endpoint ⇒ no span ⇒ no header) stamps nothing at all. + // inbound `clients.traceId` → analytics: the transport records each + // reply's server trace id into that slot, and analytics reads it + // at capture time. `clients` is a forward reference from inside + // this getter, which is safe because the getter only runs once + // an event is captured — long after the next statement binds it. + // outbound `analytics.sessionId()` → the transport: every request asks + // for the current PostHog session id and sends it as + // X-POSTHOG-SESSION-ID, so backend spans carry the same session + // the frontend recorded. // - // The OUTBOUND half (sending the PostHog session id to the server so its - // spans carry it) is deliberately not wired here. + // The inbound half is best-effort by construction, on two counts. The slot + // holds the LAST reply's trace id, so an event fired before any call has + // returned carries nothing, and one fired between calls carries the previous + // call's trace rather than its own. And the server sets `traceresponse` only + // on UNARY replies, and only when an OTel provider is installed — an + // unconfigured deployment (empty exporter endpoint ⇒ no span ⇒ no header) + // stamps nothing at all. + // + // The outbound half is best-effort too: the getter returns undefined until a + // PostHog session exists, and the interceptor then sends no header and + // self-heals on the next request. Only the TLS network door reads the header. const analytics = createAnalytics(analyticsConfigFromEnv(), { traceId: () => clients.traceId.current, }); + + const clients = createLiveClients(connection, { + sessionId: () => analytics.sessionId(), + }); + + const callerId = await bootCaller(root, () => resolveCaller(clients.compass)); + // Undefined is bootCaller's stop signal — it already painted the WhoAmI + // failure screen, so the app must not come up (no caller to scope it). + if (!callerId) { + return; + } + + // Identify the caller we just learned via WhoAmI so events attach to a stable + // distinct id. This stays AFTER bootCaller: the id is its output. analytics.identify(callerId); // One app-lifetime QueryClient — the server-state cache the query layer keys diff --git a/apps/ui/src/live/client.test.ts b/apps/ui/src/live/client.test.ts index 3c135f261..655ae8707 100644 --- a/apps/ui/src/live/client.test.ts +++ b/apps/ui/src/live/client.test.ts @@ -43,6 +43,27 @@ describe("createLiveClients (query record T1)", () => { expect(compassTransport).toBe(clients.transport); expect(commsTransport).toBe(compassTransport); }); + + test("passes the session id getter through to the transport factory", () => { + const transportSpy = spyOn(compassClient, "createCompassWebTransport"); + spies.push(transportSpy); + const sessionId = () => "session-id"; + + createLiveClients(conn, { sessionId }); + + const opts = transportSpy.mock.calls[0]?.[2]; + expect(opts?.sessionId).toBe(sessionId); + }); + + test("omits the session id option when deps are omitted", () => { + const transportSpy = spyOn(compassClient, "createCompassWebTransport"); + spies.push(transportSpy); + + createLiveClients(conn); + + const opts = transportSpy.mock.calls[0]?.[2]; + expect(opts).not.toHaveProperty("sessionId"); + }); }); describe("resolveCaller (WhoAmI boot probe)", () => { diff --git a/apps/ui/src/live/client.ts b/apps/ui/src/live/client.ts index bdbd651b6..5d8f5693a 100644 --- a/apps/ui/src/live/client.ts +++ b/apps/ui/src/live/client.ts @@ -41,9 +41,9 @@ export interface LiveClients { * * It rides on LiveClients because this is the one place that owns transport * construction — the sink is WRITTEN by the transport layer and READ above - * it (analytics stamps it on captured events), and boot builds the clients - * before analytics exists, so a shared mutable slot handed out here is what - * connects a writer and a reader that can never meet at construction. */ + * it (analytics stamps it on captured events). A shared mutable slot is what + * connects them because the trace id arrives on a REPLY: the writer has no + * value to hand over at construction time, whatever order boot runs in. */ readonly traceId: TraceIdSink; } @@ -54,11 +54,15 @@ export interface LiveClients { * cache-coherent with the clients' calls. `conn.fetchImpl` threads the resolved * transport fetch through: undefined (browser dev) uses the platform fetch; a * shell-provided fetch tunnels over IPC — the seam is invisible above here. */ -export function createLiveClients(conn: ResolvedConnection): LiveClients { +export function createLiveClients( + conn: ResolvedConnection, + deps?: { sessionId?: () => string | undefined }, +): LiveClients { const traceId: TraceIdSink = { current: undefined }; const transport = createCompassWebTransport(conn.baseUrl, conn.token, { fetch: conn.fetchImpl, traceSink: traceId, + ...(deps?.sessionId === undefined ? {} : { sessionId: deps.sessionId }), }); return { comms: createCommsClient(transport), diff --git a/packages/compass-client/src/index.test.ts b/packages/compass-client/src/index.test.ts index aa43d7bd1..37d3e77ae 100644 --- a/packages/compass-client/src/index.test.ts +++ b/packages/compass-client/src/index.test.ts @@ -17,7 +17,9 @@ import { createRouterTransport, GetServerInfoResponseSchema, parseTraceResponse, + posthogSessionHeader, SubscribeCommsResponseSchema, + sessionIdInterceptor, type TraceIdSink, type Transport, traceResponseInterceptor, @@ -78,7 +80,11 @@ type FetchLike = ( // transport has already invoked fetch and the capture is populated. async function captureRequest( run: (fetch: FetchLike) => Promise, -): Promise<{ url: string; authorization: string | null }> { +): Promise<{ + url: string; + authorization: string | null; + sessionId: string | null; +}> { let url = ""; let headers = new Headers(); const fetch: FetchLike = async (input, init) => { @@ -87,7 +93,14 @@ async function captureRequest( throw new Error("captureRequest: short-circuit before response"); }; await expect(run(fetch)).rejects.toThrow(); - return { url, authorization: headers.get("authorization") }; + return { + url, + authorization: headers.get("authorization"), + // Reported so the session-id cases can assert PRESENCE with a value and + // ABSENCE as `null` — an empty-string header would read back as "" and is + // a distinct (and forbidden) outcome from absent. + sessionId: headers.get(posthogSessionHeader), + }; } describe("bearerAuthInterceptor", () => { @@ -590,6 +603,278 @@ describe("traceResponseInterceptor round-trips through a real transport", () => }); }); +// A counting `next` returning a Symbol sentinel, plus a REAL `Headers` request +// — the direct-interceptor seam, following the bearerAuthInterceptor precedent +// above. The real `Headers` is what makes a `Headers.set` TypeError reachable, +// and the sentinel is what proves nothing threw. +// +// This seam exists because `captureRequest` CANNOT witness a would-throw case: +// its capturing fetch always throws, so it must gate on +// `rejects.toThrow()` — which a broken interceptor's own TypeError satisfies — +// and its header readback is a pre-initialized empty `Headers`, so absence also +// passes. Both assertions go green on exactly the defect. Never route a +// would-throw value onto the capture seam. +function directSeam() { + let calls = 0; + const sentinel = Symbol("next-response"); + const next = (_req: unknown) => { + calls++; + return Promise.resolve(sentinel); + }; + const req = { header: new Headers() }; + return { + req, + sentinel, + next, + get calls() { + return calls; + }, + }; +} + +describe("sessionIdInterceptor stamps only a sendable session id", () => { + const validId = "0199a1b2-3c4d-7e8f-9012-3456789abcde"; + + // The header name is a WIRE CONTRACT with the server's J1 interceptor + // (go/internal/otel/interceptor.go, PostHogSessionHeader) and is CORS-allowed + // by exactly that string in the network door. Every other case reads the + // header through the exported const, so all of them would stay green under a + // rename; only this pins the literal both ends must agree on. + test("the header name is exactly X-POSTHOG-SESSION-ID", () => { + expect(posthogSessionHeader).toBe("X-POSTHOG-SESSION-ID"); + }); + + test("a valid id is carried on the request as X-POSTHOG-SESSION-ID", async () => { + const { url, sessionId } = await captureRequest((fetch) => + createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: fetch as typeof globalThis.fetch, + sessionId: () => validId, + }), + ).getServerInfo({}), + ); + + expect(url).toBe( + "http://compass.localhost/compass.v1.CompassService/GetServerInfo", + ); + expect(sessionId).toBe(validId); + }); + + // Analytics off (NoopAnalytics.sessionId() ⇒ undefined) must send NO header + // at all, not an empty one: an empty header spends wire bytes asserting a + // correlation that does not exist, and the server trim-drops it anyway. + test("getter returns undefined ⇒ the header is absent, not empty", async () => { + const { sessionId } = await captureRequest((fetch) => + createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: fetch as typeof globalThis.fetch, + sessionId: () => undefined, + }), + ).getServerInfo({}), + ); + + expect(sessionId).toBeNull(); + }); + + // posthog-js's get_session_id() legitimately returns "" before it is fully + // initialized; the guard's `+` quantifier rejects it. + test('getter returns "" ⇒ the header is absent', async () => { + const { sessionId } = await captureRequest((fetch) => + createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: fetch as typeof globalThis.fetch, + sessionId: () => "", + }), + ).getServerInfo({}), + ); + + expect(sessionId).toBeNull(); + }); + + test("201 ASCII chars ⇒ over the cap, the header is absent", async () => { + const { sessionId } = await captureRequest((fetch) => + createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: fetch as typeof globalThis.fetch, + sessionId: () => "a".repeat(201), + }), + ).getServerInfo({}), + ); + + expect(sessionId).toBeNull(); + }); + + // The boundary the `<=` in `id.length <= MAX_SESSION_ID_LEN` owns: a `<` + // typo reddens HERE and nowhere else, because the 201 case stays green under + // both operators. 200 is legal on the server too — its check is + // `len(id) > maxSessionIDLen` — so refusing it would be needlessly stricter + // than the wire contract. + test("exactly 200 ASCII chars ⇒ at the cap, the header is PRESENT", async () => { + const atCap = "a".repeat(200); + const { sessionId } = await captureRequest((fetch) => + createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: fetch as typeof globalThis.fetch, + sessionId: () => atCap, + }), + ).getServerInfo({}), + ); + + expect(sessionId).toBe(atCap); + }); + + // A Latin-1 value does NOT throw — `Headers.set` accepts U+0080–U+00FF — so + // the capture seam is the right one here and the readback IS the whole + // assertion. What the ASCII guard buys: without it a browser emits this as + // a single raw high byte, which fails the server's utf8.ValidString check, + // so the id is silently DROPPED. Not asserting transmitted bytes here — + // that is a transport-encoding property, not this interceptor's contract. + test("a Latin-1 value (sess-é) ⇒ the header is absent", async () => { + const { sessionId } = await captureRequest((fetch) => + createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: fetch as typeof globalThis.fetch, + sessionId: () => "sess-é", + }), + ).getServerInfo({}), + ); + + expect(sessionId).toBeNull(); + }); + + // ONE transport, hence ONE interceptor instance, driven across two requests + // with a different capturing fetch each time. Building a fresh transport per + // request would let a construction-time memo re-read the getter and pass + // both multi-request cases below, which is exactly the defect they exist to + // catch — so the fetch is indirected through a mutable slot instead. + function oneClientAcrossRequests(sessionId: () => string | undefined) { + let currentFetch: FetchLike = () => + Promise.reject(new Error("oneClientAcrossRequests: no fetch installed")); + const client = createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: ((input: RequestInfo | URL, init?: RequestInit) => + currentFetch(input, init)) as typeof globalThis.fetch, + sessionId, + }), + ); + return (fetch: FetchLike) => { + currentFetch = fetch; + return client.getServerInfo({}); + }; + } + + test("the getter is called per request, so a fresh value is sent", async () => { + const ids = ["sess-first", "sess-second"]; + let call = 0; + const build = oneClientAcrossRequests(() => ids[call++]); + + const first = await captureRequest(build); + const second = await captureRequest(build); + + expect(first.sessionId).toBe("sess-first"); + expect(second.sessionId).toBe("sess-second"); + }); + + // Self-healing, the direction forward-propagation does NOT cover: a + // construction-time cache or a first-value memo still passes the laziness + // case above while failing this one, because it would pin the early empty + // value forever. + test('"" on the first call then a valid id on the second ⇒ only the second carries the header', async () => { + const ids: (string | undefined)[] = ["", validId]; + let call = 0; + const build = oneClientAcrossRequests(() => ids[call++]); + + const first = await captureRequest(build); + const second = await captureRequest(build); + + expect(first.sessionId).toBeNull(); + expect(second.sessionId).toBe(validId); + }); + + // The guard is deliberately NARROWER than `Headers.set`: set accepts a space, + // a tab, and DEL, and this rejects all three. Nothing else asserts that extra + // narrowing, so widening the class one codepoint (0x20 for 0x21) would put a + // space-bearing value on the wire unnoticed. Capture seam is correct here — + // none of these makes `Headers.set` throw, so the readback IS the assertion. + const narrowerThanHeadersSet: [string, string][] = [ + ["a space", "sess id"], + ["a tab", "sess\tid"], + ["a DEL byte", "sess\x7f"], + ]; + + for (const [label, value] of narrowerThanHeadersSet) { + test(`${label} ⇒ the header is absent, though Headers.set would take it`, async () => { + const { sessionId } = await captureRequest((fetch) => + createCompassClient( + createCompassWebTransport("http://compass.localhost", undefined, { + fetch: fetch as typeof globalThis.fetch, + sessionId: () => value, + }), + ).getServerInfo({}), + ); + + expect(sessionId).toBeNull(); + }); + } +}); + +// Three values that make `Headers.set` THROW a TypeError, so a missing guard +// would fail the whole RPC rather than merely lose a correlation key. Each case +// asserts the full triple: header absent, `next` ran exactly once, and the +// awaited result IS the sentinel — the third is load-bearing, since it cannot +// pass if the interceptor threw before reaching `next`. Values are kept SHORT +// on purpose: a long non-ASCII value is rejected by the length cap first and +// never reaches `header.set`, so it could not exercise the throw at all. +describe("sessionIdInterceptor rejects would-throw values without failing the request", () => { + const wouldThrow: [string, string][] = [ + ["a non-Latin-1 id (> U+00FF)", "sess-日本語"], + ["a value containing CRLF", "sess\r\nx"], + ["a lone surrogate", "\uD800"], + ]; + + for (const [label, value] of wouldThrow) { + test(`${label} ⇒ no header, next still ran, nothing threw`, async () => { + const seam = directSeam(); + + const result = await sessionIdInterceptor(() => value)( + seam.next as never, + )(seam.req as never); + + expect(seam.req.header.get(posthogSessionHeader)).toBeNull(); + expect(seam.calls).toBe(1); + expect(result as unknown).toBe(seam.sentinel); + }); + } +}); + +// `undefined` and `""` are absent-value cases rather than values `Headers.set` +// rejects — but the GUARD can throw on them, which puts them in the class above: +// a bare `SENDABLE.test(undefined)` coerces to the string "undefined" and +// PASSES, so evaluation reaches `id.length` on undefined and throws. So the +// capture seam cannot witness them either, and `undefined` is the case that +// matters most: it is the shipped default (`NoopAnalytics.sessionId()`), so a +// guard that throws on it fails EVERY request whenever analytics is off. +describe("sessionIdInterceptor survives an absent session id", () => { + const absent: [string, string | undefined][] = [ + ["undefined — analytics off, the shipped default", undefined], + ['"" — posthog before it has initialized', ""], + ]; + + for (const [label, value] of absent) { + test(`${label} ⇒ no header, next still ran, nothing threw`, async () => { + const seam = directSeam(); + + const result = await sessionIdInterceptor(() => value)( + seam.next as never, + )(seam.req as never); + + expect(seam.req.header.get(posthogSessionHeader)).toBeNull(); + expect(seam.calls).toBe(1); + expect(result as unknown).toBe(seam.sentinel); + }); + } +}); + describe("callInterceptors installs only what was asked for", () => { // The behavioral claim of the omitted-means-off rule: neither concern // requested ⇒ the transport is handed `undefined`, NOT an empty list, so an @@ -634,4 +919,72 @@ describe("callInterceptors installs only what was asked for", () => { expect(opts.interceptors).toHaveLength(2); }); + + // The direction that catches an append placed inside the old early return: + // `callInterceptors` used to be `const bearer = ...; if (!traceSink) return + // bearer;`, so a session interceptor appended after that guard is skipped + // entirely whenever no trace sink is configured — and every OTHER + // membership direction below still passes. This is the only one that reddens. + test("a sessionId getter alone ⇒ exactly one interceptor", () => { + const opts = transportOptionsFor(() => + createCompassWebTransport("http://compass.localhost", undefined, { + sessionId: () => "sess-1", + }), + ); + + expect(opts.interceptors).toHaveLength(1); + }); + + test("sessionId and sink ⇒ exactly two interceptors", () => { + const sink: TraceIdSink = { current: undefined }; + const opts = transportOptionsFor(() => + createCompassWebTransport("http://compass.localhost", undefined, { + traceSink: sink, + sessionId: () => "sess-1", + }), + ); + + expect(opts.interceptors).toHaveLength(2); + }); + + test("token, sink and sessionId ⇒ exactly three interceptors", () => { + const sink: TraceIdSink = { current: undefined }; + const opts = transportOptionsFor(() => + createCompassWebTransport("http://compass.localhost", "tok", { + traceSink: sink, + sessionId: () => "sess-1", + }), + ); + + expect(opts.interceptors).toHaveLength(3); + }); + + // Composition ORDER, not just membership. The restructure from an early + // return to an accumulating list made order a fresh degree of freedom, and + // every case above is order-blind (`toHaveLength` counts). Order is benign + // TODAY — the session interceptor writes a request header and the trace one + // reads a response header, so they commute — and this pins it so the suite + // notices if that stops being true. Each interceptor is identified by its + // observable effect: position 0 stamps authorization, position 2 stamps the + // session header, so trace is the middle by elimination. + test("token, sink and sessionId ⇒ bearer first, session last", async () => { + const sink: TraceIdSink = { current: undefined }; + const opts = transportOptionsFor(() => + createCompassWebTransport("http://compass.localhost", "tok", { + traceSink: sink, + sessionId: () => "sess-1", + }), + ); + const interceptors = opts.interceptors ?? []; + + const first = directSeam(); + await interceptors[0]?.(first.next as never)(first.req as never); + expect(first.req.header.get("authorization")).toBe("Bearer tok"); + expect(first.req.header.get(posthogSessionHeader)).toBeNull(); + + const last = directSeam(); + await interceptors[2]?.(last.next as never)(last.req as never); + expect(last.req.header.get(posthogSessionHeader)).toBe("sess-1"); + expect(last.req.header.get("authorization")).toBeNull(); + }); }); diff --git a/packages/compass-client/src/index.ts b/packages/compass-client/src/index.ts index b0e76ee1d..5c79abc01 100644 --- a/packages/compass-client/src/index.ts +++ b/packages/compass-client/src/index.ts @@ -46,12 +46,11 @@ const traceResponseHeader = "traceresponse"; /** * A one-slot mailbox holding the trace id of the most recent server reply. * - * Mutable on purpose, and the mutability is the whole point: the transport is - * constructed during boot BEFORE the analytics client exists, so the writer - * (this package's response interceptor) and the reader (the analytics wrapper, - * layers above) cannot be introduced to each other at construction time. A - * stable reference handed to both closes that gap without reordering boot and - * without the transport layer taking a dependency on analytics. + * Mutable on purpose, and the mutability is the whole point: the trace id + * arrives on a REPLY, so the writer (this package's response interceptor) has + * no value to hand the reader (the analytics wrapper, layers above) at + * construction time, whatever order boot runs in. A stable reference handed to + * both closes that gap without the transport layer depending on analytics. * * The write discipline — the transport interceptor writes, everything above it * only reads — is a CONVENTION, not a type guarantee: `current` is structurally @@ -161,21 +160,104 @@ export function traceResponseInterceptor(sink: TraceIdSink): Interceptor { }; } +/** The PostHog session-id REQUEST header the server's J1 interceptor reads + * (go/internal/otel/interceptor.go, PostHogSessionHeader). Already CORS-allowed + * by the network door, so a browser may send it cross-origin. */ +export const posthogSessionHeader = "X-POSTHOG-SESSION-ID"; + +// Printable ASCII only — no space (0x20), no control byte, nothing above 0x7E. +const SENDABLE = /^[\x21-\x7E]+$/; + +// Mirrors the server's maxSessionIDLen (go/internal/otel/interceptor.go), whose +// check is `len(id) > maxSessionIDLen`, so 200 is legal on both sides and the +// cap here is inclusive too. On input this guard accepts, `.length` IS the +// UTF-8 byte count, so no TextEncoder is needed to mean the same thing as Go's +// `len()`. +const MAX_SESSION_ID_LEN = 200; + +/** + * Whether `id` can be put on the wire as a session-id header value at all. + * + * Deliberately printable-ASCII rather than UTF-8-shaped, because `req.header` + * is a fetch `Headers` and `Headers.set` takes a WebIDL ByteString: + * + * - A perfectly well-formed id containing any code point above U+00FF makes + * `Headers.set` THROW a `TypeError`, which would fail the whole RPC. An + * analytics nicety that can kill every request is worse than any + * sender-side rejection, so such a value must be rejected BEFORE `set`. + * - U+0080–U+00FF does not throw: `set` accepts it and a browser emits it as a + * single raw high byte, which the server then rejects as invalid UTF-8 and + * silently DROPS — the same lost key, harder to notice. + * - A value containing CRLF also throws in `Headers.set`; `\x21-\x7E` excludes + * it, so it fails quietly here instead of failing the request. + * + * That makes this a strict SUBSET of what `Headers.set` accepts (`set` takes + * space and tab; this does not) and strictly stronger than the server's own + * `<=200 bytes` + valid-UTF-8 pair. Whitespace-only and empty values are + * rejected by the regex, which the server would trim-and-drop anyway. No + * `isWellFormed` either: a lone surrogate is non-ASCII, so it is already out. + */ +export function isSendableSessionId(id: string): boolean { + return SENDABLE.test(id) && id.length <= MAX_SESSION_ID_LEN; +} + +/** + * Sets `X-POSTHOG-SESSION-ID` on every request from a lazy session-id source. + * + * No usable value (undefined, empty, oversized, non-ASCII) ⇒ the header is not + * set at all — never an empty header, which would spend wire bytes asserting a + * correlation that does not exist and be trim-dropped by the server regardless. + * + * `sessionId` is called PER REQUEST and never cached: posthog-js's + * `get_session_id()` can legitimately return `""` before it is fully + * initialized, so an early request simply carries no header and the next one + * self-heals once a session exists. A construction-time read would pin that + * degraded state forever. + * + * Sent on ALL requests, unary and stream alike (`req.stream` is deliberately + * not inspected). The server reads the header only on unary — its + * `NewSessionIDInterceptor` is a `connect.UnaryInterceptorFunc` — so a stream + * request carries an unread header, which costs bytes, not correctness. Gating + * on `req.stream` would couple this client to a server-side interceptor kind it + * cannot observe, for a few bytes per stream. + */ +export function sessionIdInterceptor( + sessionId: () => string | undefined, +): Interceptor { + return (next) => (req) => { + const id = sessionId(); + if (id !== undefined && isSendableSessionId(id)) { + req.header.set(posthogSessionHeader, id); + } + return next(req); + }; +} + // The full interceptor list every client/transport factory installs, and the one -// place the two concerns compose. The bearer rule is unchanged (and still throws -// first on a misconfigured credential). The trace sink follows the same -// omitted-means-off discipline: no sink ⇒ no trace interceptor at all, so a -// caller that does not ask for correlation gets byte-identical behavior — -// including `undefined` rather than an empty list when neither is asked for. +// place the three concerns compose. The bearer rule is unchanged (and still +// throws first on a misconfigured credential). The trace sink and the session-id +// source follow the same omitted-means-off discipline: no sink ⇒ no trace +// interceptor at all and no getter ⇒ no session interceptor at all, so a caller +// that does not ask for correlation gets byte-identical behavior — including +// `undefined` rather than an empty list when nothing is asked for. +// +// Built as one list with no early return on purpose: an append placed after an +// `if (!traceSink) return bearer` guard would be skipped entirely whenever no +// trace sink is configured, so `sessionId` alone would silently install nothing. function callInterceptors( token?: string, traceSink?: TraceIdSink, + sessionId?: () => string | undefined, ): Interceptor[] | undefined { const bearer = bearerInterceptors(token); - if (!traceSink) { - return bearer; + const interceptors = [...(bearer ?? [])]; + if (traceSink) { + interceptors.push(traceResponseInterceptor(traceSink)); } - return [...(bearer ?? []), traceResponseInterceptor(traceSink)]; + if (sessionId) { + interceptors.push(sessionIdInterceptor(sessionId)); + } + return interceptors.length > 0 ? interceptors : undefined; } /** A typed client for the Compass server over a given transport. */ @@ -205,11 +287,21 @@ export function createCompassClient(transport: Transport): CompassClient { * `opts.traceSink` opts this transport into recording each reply's * `traceresponse` trace id; omitted, no trace interceptor is installed and the * transport behaves exactly as before. + * + * `opts.sessionId` opts this transport into stamping the PostHog session id on + * every outgoing request, read fresh from the getter per request; omitted, no + * session interceptor is installed and the transport behaves exactly as before. + * This is the shipped path for the header — see the note on the per-client + * factories below. */ export function createCompassWebTransport( baseUrl: string, token?: string, - opts?: { fetch?: typeof globalThis.fetch; traceSink?: TraceIdSink }, + opts?: { + fetch?: typeof globalThis.fetch; + traceSink?: TraceIdSink; + sessionId?: () => string | undefined; + }, ): Transport { return createGrpcWebTransport({ baseUrl, @@ -218,7 +310,7 @@ export function createCompassWebTransport( // truthiness guard keeps the browser dev path (no injected fetch) building // the same `{ baseUrl, interceptors }` config as before. ...(opts?.fetch ? { fetch: opts.fetch } : {}), - interceptors: callInterceptors(token, opts?.traceSink), + interceptors: callInterceptors(token, opts?.traceSink, opts?.sessionId), }); } @@ -233,6 +325,17 @@ export type { Transport } from "@connectrpc/connect"; // layer. Dev/test-only; the shipped app dials `createCompassWebTransport`. export { createRouterTransport } from "@connectrpc/connect"; +// The four per-client factories below — `createCompassWebClient`, +// `createCompassClientOverFetch`, `createCommsWebClient`, +// `createCommsClientOverFetch` — deliberately do NOT take a `sessionId` +// option, so a client built through any of them sends NO +// `X-POSTHOG-SESSION-ID` header. That is not an oversight: the shipped path +// for the session-id header is `createLiveClients` → `createCompassWebTransport` +// (the sole production transport construction; both native-shell modes route +// through it via `conn.fetchImpl`), and these four have no production caller. +// A future caller that needs the header must dial `createCompassWebTransport` +// with `opts.sessionId` rather than assume it rides along here. + /** * Create a compass.v1 client over gRPC-Web at `baseUrl` — the door the web UI * uses. Bundles the transport so UI code imports only `@compass/client`. When