address PR review polish: inject auth telemetry hooks + test wiring

- fetch-with-refresh: drop the direct Sentry import; take onRefreshOutcome /
  onLogout hooks instead (decouples the generic module from the Sentry
  singleton). client.ts wires them to addAuthBreadcrumb / captureLogout.
- Add web/test/fetch-with-refresh.test.ts asserting the logout wiring: one
  onLogout per terminal exit (refresh_rejected / retry_401), none on the happy
  or non-401 paths — locks the behavior the feature exists to deliver.
- Only register browserTracingIntegration when tracesSampleRate > 0, so an
  enabled-but-unsampled tenant doesn't instrument fetch / append trace headers.
- Soften the beforeBreadcrumb comment: opaque ids in URL *paths* are within the
  trust boundary (tagged anyway); only query strings are stripped.

Author: mgoldsborough

web/src/api/client.ts

@@ -9,7 +9,7 @@ import type {
   PlacementEntry,
   ToolCallResult,
 } from "../types";
-import { setSentryWorkspace } from "../sentry";
+import { addAuthBreadcrumb, captureLogout, setSentryWorkspace } from "../sentry";
 import { createFetchWithRefresh } from "./fetch-with-refresh";
 
 // ---------------------------------------------------------------------------
@@ -167,6 +167,10 @@ const refreshInterceptor = createFetchWithRefresh({
   fetch: ((input, init) => globalThis.fetch(input, init)) as typeof fetch,
   refreshUrl: `${API_BASE}/v1/auth/refresh`,
   onAuthError: () => onAuthError?.(),
+  // Telemetry: breadcrumb every refresh outcome; one event per involuntary
+  // logout (captureLogout dedupes the concurrent-401 calls into one incident).
+  onRefreshOutcome: (outcome) => addAuthBreadcrumb(`refresh:${outcome}`),
+  onLogout: captureLogout,
 });
 
 const fetchWithRefresh = refreshInterceptor;

web/src/api/fetch-with-refresh.ts

@@ -31,8 +31,6 @@
  * blips without surfacing an error at all.
  */
 
-import { addAuthBreadcrumb, captureLogout } from "../sentry";
-
 /** How many times to re-attempt a refresh that failed at the transport layer. */
 const REFRESH_NETWORK_RETRIES = 2;
 /** Delay between transport-failure retries. */
@@ -61,6 +59,18 @@ export interface FetchWithRefreshOptions {
    * refresh. NOT called on a transient transport failure.
    */
   onAuthError?: () => void;
+  /**
+   * Telemetry hook fired with each refresh outcome (`refreshed` / `rejected` /
+   * `unavailable`). Injected so this module stays decoupled from the Sentry
+   * singleton; the client wires it to a breadcrumb. Optional — defaults to no-op.
+   */
+  onRefreshOutcome?: (outcome: RefreshOutcome) => void;
+  /**
+   * Telemetry hook fired on the two terminal involuntary-logout exits, with a
+   * reason (`refresh_rejected` / `retry_401`). Injected for the same reason as
+   * `onRefreshOutcome`. Optional — defaults to no-op.
+   */
+  onLogout?: (reason: "refresh_rejected" | "retry_401") => void;
 }
 
 export interface FetchWithRefresh {
@@ -76,7 +86,7 @@ export interface FetchWithRefresh {
 }
 
 export function createFetchWithRefresh(options: FetchWithRefreshOptions): FetchWithRefresh {
-  const { fetch: fetchFn, refreshUrl, onAuthError } = options;
+  const { fetch: fetchFn, refreshUrl, onAuthError, onRefreshOutcome, onLogout } = options;
 
   let refreshInFlight: Promise<RefreshOutcome> | null = null;
 
@@ -137,14 +147,14 @@ export function createFetchWithRefresh(options: FetchWithRefreshOptions): FetchW
 
     // 401 — attempt silent refresh
     const outcome = await refresh();
-    // Breadcrumb every outcome (not an event) so the trail preceding a logout is
+    // Report every outcome (not an event) so the trail preceding a logout is
     // visible without quota burn — transient blips vs. a genuine rejection.
-    addAuthBreadcrumb(`refresh:${outcome}`);
+    onRefreshOutcome?.(outcome);
 
     if (outcome === "rejected") {
-      // The refresh token was refused — the session is genuinely over. Emit the
-      // single involuntary-logout event carrying the breadcrumb trail above.
-      captureLogout("refresh_rejected");
+      // The refresh token was refused — the session is genuinely over. Signal the
+      // involuntary logout (the client emits a single event for the incident).
+      onLogout?.("refresh_rejected");
       onAuthError?.();
       return res;
     }
@@ -160,7 +170,7 @@ export function createFetchWithRefresh(options: FetchWithRefreshOptions): FetchW
     const retry = await fetchFn(input, init);
     if (retry.status === 401) {
       // A fresh token still gets 401 → genuinely unauthenticated.
-      captureLogout("retry_401");
+      onLogout?.("retry_401");
       onAuthError?.();
     }
     return retry;

web/src/sentry.ts

@@ -52,10 +52,12 @@ export function initSentry(): void {
     environment: cfg.environment,
     release: cfg.release,
     sendDefaultPii: false,
-    integrations: [Sentry.browserTracingIntegration()],
-    // No Session Replay: replayIntegration records the DOM (prompts, tool
-    // results, file contents) — the exact content the trust rule forbids. Same
-    // intent as telemetry.ts disabling PostHog session recording.
+    // Performance tracing only when a tenant opts in (sample rate > 0) — avoids
+    // instrumenting fetch and appending sentry-trace/baggage headers for nothing.
+    // No Session Replay either: replayIntegration records the DOM (prompts, tool
+    // results, file contents) — the content the trust rule forbids (cf.
+    // telemetry.ts disabling PostHog session recording).
+    integrations: (s.tracesSampleRate ?? 0) > 0 ? [Sentry.browserTracingIntegration()] : [],
     tracesSampleRate: s.tracesSampleRate ?? 0,
     // tracePropagationTargets is left at the SDK default (same-origin), which is
     // exactly our case — all API calls are same-origin /v1/* proxied to the
@@ -97,7 +99,9 @@ export function beforeSend(event: Sentry.ErrorEvent): Sentry.ErrorEvent {
 export function beforeBreadcrumb(crumb: Sentry.Breadcrumb): Sentry.Breadcrumb | null {
   // App logs can carry prompts / PII — never breadcrumb them.
   if (crumb.category === "console") return null;
-  // A workspace or conversation id must not ride along in a fetch/xhr/nav URL.
+  // Strip query strings from fetch/xhr/nav URLs. (Opaque ids in the URL *path*
+  // — e.g. /w/<wsId> — are within the trust boundary: they're tagged anyway and
+  // aren't the content the rule forbids, so we don't rewrite path segments.)
   const url = crumb.data?.url;
   if (typeof url === "string") {
     const q = url.indexOf("?");

web/test/fetch-with-refresh.test.ts

@@ -0,0 +1,95 @@
+// ---------------------------------------------------------------------------
+// createFetchWithRefresh — telemetry wiring for the involuntary-logout signal.
+//
+// The whole point of this PR is to make "random logouts" observable. The auth
+// telemetry is injected as hooks (onRefreshOutcome / onLogout), so these tests
+// assert the exact wiring — one logout signal on each terminal exit, none on the
+// happy path — locking the behavior the feature exists to deliver.
+// ---------------------------------------------------------------------------
+
+import { describe, expect, mock, test } from "bun:test";
+import { createFetchWithRefresh } from "../src/api/fetch-with-refresh";
+
+const REFRESH = "/v1/auth/refresh";
+
+/** Fake fetch: refresh URL returns `refreshStatus`; the original request walks
+ *  `reqStatuses` (first call, then the post-refresh retry). */
+function makeFetch(reqStatuses: number[], refreshStatus: number) {
+  let i = 0;
+  const fn = mock((input: string) => {
+    if (input === REFRESH) return Promise.resolve(new Response(null, { status: refreshStatus }));
+    const status = reqStatuses[Math.min(i, reqStatuses.length - 1)] ?? 200;
+    i++;
+    return Promise.resolve(new Response(null, { status }));
+  });
+  return fn as unknown as typeof globalThis.fetch;
+}
+
+describe("createFetchWithRefresh telemetry", () => {
+  test("rejected refresh → onLogout('refresh_rejected') once + onAuthError", async () => {
+    const onLogout = mock(() => {});
+    const onAuthError = mock(() => {});
+    const onRefreshOutcome = mock(() => {});
+    const f = createFetchWithRefresh({
+      fetch: makeFetch([401], 401),
+      refreshUrl: REFRESH,
+      onAuthError,
+      onLogout,
+      onRefreshOutcome,
+    });
+    const res = await f("/v1/x");
+    expect(res.status).toBe(401);
+    expect(onRefreshOutcome).toHaveBeenCalledWith("rejected");
+    expect(onLogout).toHaveBeenCalledTimes(1);
+    expect(onLogout).toHaveBeenCalledWith("refresh_rejected");
+    expect(onAuthError).toHaveBeenCalledTimes(1);
+  });
+
+  test("refresh ok + retry ok → no logout", async () => {
+    const onLogout = mock(() => {});
+    const onAuthError = mock(() => {});
+    const onRefreshOutcome = mock(() => {});
+    const f = createFetchWithRefresh({
+      fetch: makeFetch([401, 200], 200),
+      refreshUrl: REFRESH,
+      onAuthError,
+      onLogout,
+      onRefreshOutcome,
+    });
+    const res = await f("/v1/x");
+    expect(res.status).toBe(200);
+    expect(onRefreshOutcome).toHaveBeenCalledWith("refreshed");
+    expect(onLogout).not.toHaveBeenCalled();
+    expect(onAuthError).not.toHaveBeenCalled();
+  });
+
+  test("refresh ok but retry still 401 → onLogout('retry_401')", async () => {
+    const onLogout = mock(() => {});
+    const onAuthError = mock(() => {});
+    const f = createFetchWithRefresh({
+      fetch: makeFetch([401, 401], 200),
+      refreshUrl: REFRESH,
+      onAuthError,
+      onLogout,
+    });
+    await f("/v1/x");
+    expect(onLogout).toHaveBeenCalledTimes(1);
+    expect(onLogout).toHaveBeenCalledWith("retry_401");
+    expect(onAuthError).toHaveBeenCalledTimes(1);
+  });
+
+  test("non-401 response: no refresh, no telemetry", async () => {
+    const onLogout = mock(() => {});
+    const onRefreshOutcome = mock(() => {});
+    const f = createFetchWithRefresh({
+      fetch: makeFetch([200], 200),
+      refreshUrl: REFRESH,
+      onLogout,
+      onRefreshOutcome,
+    });
+    const res = await f("/v1/x");
+    expect(res.status).toBe(200);
+    expect(onRefreshOutcome).not.toHaveBeenCalled();
+    expect(onLogout).not.toHaveBeenCalled();
+  });
+});