From 5eb2fcb9d507a5123c6f791d7958f3b265e6dc3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Wed, 9 Sep 2026 12:06:28 +0200 Subject: [PATCH 1/3] feat(runner): instrument the NEED-DATA Sentry issues (DEV-2859) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds Sentry user context, fetchVersions/fetchDocsJson retry+diagnostics, and a bounded editor trail, each as an import-free decision module (identity.ts, fetchDiagnostics.ts, editorTrail.ts) with wiring kept to App.tsx/main.tsx/auth.ts/catalog.ts, which node --test cannot import. Item 1 (Sentry.setUser): every Sentry issue in this project reports "users: 0" because nothing ever called Sentry.setUser/setTag — that literal absence of user context, not an actual absence of affected users, is what misled an earlier triage into suppressing DEMOS-2X on "0 users". Identity is sessionStorage-scoped ("hot_sid"), not localStorage: localStorage would mint a new persistent pseudonymous identifier on a public site, which this instrumentation deliberately avoids. The cost is real and stated in identity.ts: a reload mints a new id, so `users` on an anonymous issue counts sessions, not people. A signed-in id is a truncated SHA-256 of the lowercased email, never the raw address or username. Item 2 (fetchVersions/fetchDocsJson diagnostics): a bounded retry (once, after 300ms, only on a transport failure that never produced a response) is the discriminator between a visitor's own network dropping mid-request and a real host dip - a dip fails at both attempts, a blip does not. `!res.ok` is never retried (retrying an outage amplifies it) and `catalog.ts`'s `versions ${res.status}` throw stays byte-identical so that population's grouping does not move. App.tsx reads the attached `fetchDiagnostics` ahead of `isOpaqueNetworkFailure` on purpose: once the sibling fix in fix/DEV-2859-opaque-fetch-host-suffix lands, that check starts matching production wording, and reading it first would silently turn every exhausted two-attempt failure back into a breadcrumb. `docs-catalog.ts`'s `fetchDocsJson` was left untouched, not wired through the same retry helper as originally planned: it cannot import fetchDiagnostics.ts and stay importable by pipeline/docs-catalog.test.mjs under --experimental-strip-types, which cannot resolve a sibling ./x.js specifier (verified empirically against a throwaway probe file). The DEMOS-7D ruling (instrument, don't suppress) is instead satisfied by wrapping the two existing App.tsx reportError callsites in Sentry.withScope + diagnosticTags with a thinner, retry-less diagnostics bundle - no re-promotion, no fingerprint change, no new issue. Item 3 (DEMOS-1D editor trail): replaces a rejected one-breadcrumb-per-onEdit design. sentry.ts caps breadcrumbs at 200; a "Maximum update depth exceeded" loop calling onEdit thousands of times would fill every slot with identical entries and evict the preceding context that identifies the trigger - the only evidence this change exists to collect. A capacity-24 ring buffer with consecutive-identical coalescing fixes that: a 3000-iteration loop occupies one slot, and whatever happened just before it survives. Two fields from the original ticket are deliberately absent: a version-switch never reaches onEdit (it re-pins files via setFiles, its own "version" trail entry) and the active editor tab is unreachable from App.tsx - it lives inside packages/editor-shell, and adding a prop to a published package's surface for a temporary diagnostic was out of scope. Also fixed before landing: editorTrailTags() was reading the bare last entry, which is always a "flush-quiet" after a Style-panel colour drag - that hid the coalesced run of quiet writes one slot earlier, which is the exact signal DEMOS-1D exists to read. It now walks back past trailing flush-quiet entries. Tests: pipeline/identity.test.mjs, pipeline/fetch-diagnostics.test.mjs, pipeline/editor-trail.test.mjs (new), plus one added case in pipeline/docs-catalog.test.mjs pinning that a transport failure is not misclassified as a missing resource. All wiring in App.tsx, main.tsx, auth.ts and catalog.ts is an honest, undecorated coverage gap - none of it is node-importable, and no text-grep assertions were added to fake coverage of it. Does not touch apps/authoring/src/fetchFailure.ts or pipeline/fetch-failure.test.mjs - owned by the parallel fix/DEV-2859-opaque-fetch-host-suffix branch. Co-Authored-By: Claude Opus 5 --- runner/apps/authoring/src/App.tsx | 192 +++++++++++- runner/apps/authoring/src/Chat.tsx | 10 +- runner/apps/authoring/src/auth.ts | 12 +- runner/apps/authoring/src/catalog.ts | 20 +- runner/apps/authoring/src/editorTrail.ts | Bin 0 -> 7859 bytes runner/apps/authoring/src/fetchDiagnostics.ts | 277 ++++++++++++++++++ runner/apps/authoring/src/identity.ts | 112 +++++++ runner/apps/authoring/src/main.tsx | 24 ++ runner/apps/authoring/src/userScope.ts | 99 +++++++ runner/pipeline/docs-catalog.test.mjs | 23 ++ runner/pipeline/editor-trail.test.mjs | 195 ++++++++++++ runner/pipeline/fetch-diagnostics.test.mjs | 228 ++++++++++++++ runner/pipeline/identity.test.mjs | 114 +++++++ 13 files changed, 1288 insertions(+), 18 deletions(-) create mode 100644 runner/apps/authoring/src/editorTrail.ts create mode 100644 runner/apps/authoring/src/fetchDiagnostics.ts create mode 100644 runner/apps/authoring/src/identity.ts create mode 100644 runner/apps/authoring/src/userScope.ts create mode 100644 runner/pipeline/editor-trail.test.mjs create mode 100644 runner/pipeline/fetch-diagnostics.test.mjs create mode 100644 runner/pipeline/identity.test.mjs diff --git a/runner/apps/authoring/src/App.tsx b/runner/apps/authoring/src/App.tsx index 459dccb27..fb5dc4ef7 100644 --- a/runner/apps/authoring/src/App.tsx +++ b/runner/apps/authoring/src/App.tsx @@ -71,10 +71,56 @@ import { monitorDemos, reportDemoEvent, reportError, reportingEnabled, Sentry } import { isMonitorPayload } from "@handsontable/demo-runtime/monitor"; import { tier1Report } from "./tier1Report.js"; import { isOpaqueNetworkFailure } from "./fetchFailure.js"; +import { + readFetchDiagnostics, + apiBaseOrigin, + diagnosticTags, + diagnosticExtras, + netEffectiveType, +} from "./fetchDiagnostics.js"; +import { recordEditorEvent } from "./editorTrail.js"; const SANDPACK_BUNDLER_URL = import.meta.env.VITE_SANDPACK_BUNDLER_URL || undefined; const API_BASE = import.meta.env.VITE_API_BASE || "http://localhost:8787"; +// DEV-2859: one synthesized "versions fetch unreachable" event per page load, +// however many times the effect below re-fires (StrictMode's double-invoke in +// dev, or a genuine remount). Module-scope, not component state, because a +// remount must not reset an allowance that already told the operator "this +// visitor's session tried, and both attempts failed". +let versionsFetchEventSent = false; + +/** + * DEMOS-7D (DEV-2859 ruling): instrument, don't suppress. `App.tsx`'s two + * `fetchDocsJson` catches already report *every* failure with no + * `isOpaqueNetworkFailure` gate, so this only adds tags to the event that was + * always going to be sent — no re-promotion, no fingerprint change, no new + * issue. `Sentry.captureException` inside `run()` picks up whatever tags this + * scope carries, which is what makes wrapping the existing call enough. + * + * `docs-catalog.ts`'s `fetchDocsJson` has no retry of its own to report: it + * cannot import `fetchDiagnostics.ts` and stay importable by + * `pipeline/docs-catalog.test.mjs` under `--experimental-strip-types`, which + * cannot resolve a sibling `./x.js` specifier (verified empirically against a + * throwaway probe file — the same constraint `fetchDiagnostics.ts`'s header + * documents). So this diagnostics bundle is deliberately thinner than the + * versions-fetch one: online state and the API-base classification, no + * attempt count or outcome (`diagnosticTags`/`diagnosticExtras` both treat + * those as optional for exactly this caller). + */ +function withDocsFetchDiagnostics(context: string, run: () => void): void { + Sentry.withScope((scope) => { + scope.setTags( + diagnosticTags({ + context, + onlineAtStart: typeof navigator !== "undefined" ? navigator.onLine : undefined, + apiBaseOrigin: apiBaseOrigin(API_BASE, location.origin), + }), + ); + run(); + }); +} + // Framework preference (used to auto-pick a variant when an example is chosen and // the current one isn't available) + short labels for the framework picker. const FW_PREF = ["react", "typescript", "javascript", "vue", "angular"]; @@ -1330,6 +1376,20 @@ function Authoring({ /** Replace the whole workspace (entry + files + lineage) and remount. */ const loadWorkspace = useCallback( (nextEntry: CatalogEntry, nextFiles: FilesMap, lineage: string) => { + // DEV-2859: the lineage *prefix only* — the segment before the first + // `:` — never the full string. `import:` and `docs::` + // both carry visitor- or docs-authored data after that first colon; a + // saved-demo id (no colon at all) collapses to the constant "saved" + // rather than the id itself, which is the shape this redaction is aimed + // at (an id is not a URL, but it is still not this trail's business to + // carry verbatim). + recordEditorEvent({ + kind: "workspace", + source: "load", + path: lineage.includes(":") ? lineage.slice(0, lineage.indexOf(":")) : "saved", + quiet: false, + size: 0, + }); // Whatever workspace replaces an ad-hoc one is no longer its, so its title // and its skipped-files notice are cleared here — at the moment the new // files are installed, which a failed starter or docs load never reaches. @@ -1652,8 +1712,19 @@ function Authoring({ useEffect(() => { let cancelled = false; fetchVersions(API_BASE) - .then(({ latest, next, versions }) => { + .then(({ latest, next, versions, diagnostics }) => { if (cancelled) return; + // A retry that recovered is a blip, not an issue (DEV-2859) — the same + // "worth knowing, not worth an issue" treatment the catch branch below + // gives the visitor-network population, on the success side of it. + if ((diagnostics.attempts ?? 1) > 1) { + Sentry.addBreadcrumb({ + category: "fetch", + level: "info", + message: "versions-fetch recovered on retry", + data: diagnosticTags({ ...diagnostics, context: "versions-fetch" }), + }); + } setNextVersion(next ?? ""); const opts = [...new Set([latest, ...versions, next].filter((v): v is string => !!v))]; if (opts.length) setVersionOptions(opts); @@ -1665,9 +1736,37 @@ function Authoring({ .catch((error) => { // Fails open onto the hardcoded VERSION_OPTIONS, so the picker silently // goes stale rather than breaking — worth knowing about, unless it is the - // visitor's own network dropping mid-request (Sentry DEMOS-2X): that shape - // carries nothing about our host, so it is a breadcrumb, not an issue. - if (isOpaqueNetworkFailure(error)) { + // visitor's own network dropping mid-request (Sentry DEMOS-2X). + // + // `fetchDiagnostics` is read FIRST, ahead of `isOpaqueNetworkFailure` + // (DEV-2859): once the sibling fetchFailure.ts fix lands, that check + // starts matching the real production wording, and if it ran first + // every exhausted two-attempt failure would fall back to a silent + // breadcrumb — exactly the regression item 2 exists to prevent. A + // `!res.ok` throw and a JSON-parse failure never carry + // `fetchDiagnostics` (both are thrown by catalog.ts itself, not by + // fetchWithDiagnostics), so they fall through to the branch below + // unchanged from today. + const diag = readFetchDiagnostics(error); + if (diag) { + if (!versionsFetchEventSent) { + versionsFetchEventSent = true; + const full = { + ...diag, + context: "versions-fetch" as const, + apiBaseOrigin: apiBaseOrigin(API_BASE, location.origin), + netEffectiveType: netEffectiveType(), + }; + Sentry.withScope((scope) => { + scope.setLevel("warning"); + scope.setTags(diagnosticTags(full)); + scope.setExtras(diagnosticExtras(full)); + Sentry.captureMessage("versions fetch unreachable", { + fingerprint: ["versions-fetch-unreachable"], + }); + }); + } + } else if (isOpaqueNetworkFailure(error)) { Sentry.addBreadcrumb({ category: "fetch", level: "info", @@ -1821,12 +1920,16 @@ function Authoring({ // open. Tagged by which step failed so a missing artifact (docs linking // an example that was never imported) is distinguishable from a // transient fetch. - reportError(error, `docs-example-load:${isMissingDocsResource(error) ? "path" : "fetch"}`); + withDocsFetchDiagnostics("docs-fetch", () => + reportError(error, `docs-example-load:${isMissingDocsResource(error) ? "path" : "fetch"}`), + ); failOpenDocs(isMissingDocsResource(error) ? "path" : "fetch"); } }) .catch((error) => { - reportError(error, `docs-bucket-resolve:${isMissingDocsResource(error) ? "bucket" : "fetch"}`); + withDocsFetchDiagnostics("docs-fetch", () => + reportError(error, `docs-bucket-resolve:${isMissingDocsResource(error) ? "bucket" : "fetch"}`), + ); failOpenDocs(isMissingDocsResource(error) ? "bucket" : "fetch"); }); return () => { cancelled = true; }; @@ -2095,6 +2198,11 @@ function Authoring({ ); const changeVersion = useCallback((next: string) => { + // DEV-2859: a version switch never reaches `onEdit` — it re-pins the file + // set (elsewhere, via `pinHandsontableFiles` + `setFiles`) rather than + // editing it — so it needs its own trail entry, tagged by the requested + // version rather than a file path. + recordEditorEvent({ kind: "version", source: "repin", path: next, quiet: false, size: 0 }); docsRequestSeqRef.current += 1; setVersionWarning(null); setThemeRemoved(false); @@ -2294,7 +2402,12 @@ function Authoring({ // workspace itself is updated the same way regardless: `filesRef` is what Save, // Download, Share and the next example switch read, so nothing may ever sit // between an edit and this assignment. - const onEdit = useCallback( + // + // Split from `onEdit`/`onEditFromStyle`/`onEditFromChat` below (DEV-2859): the + // trail entry has to be recorded once, tagged with which surface actually + // triggered the write, not by this shared writer — which is why the + // recording lives at each App-owned callsite and this one stays undecorated. + const writeFile = useCallback( (path: string, contents: string, opts?: WriteFileOptions) => { const next = { ...filesRef.current, [path]: contents }; filesRef.current = next; @@ -2313,6 +2426,62 @@ function Authoring({ [markDirty, showSyncing], ); + /** Bound straight to `EditorShell`'s `onEdit` — a keystroke in the code + * editor itself. DEV-2859 (Sentry DEMOS-1D): recorded into the bounded + * editor trail (`editorTrail.ts`) so a render-loop crash's event carries + * what was happening just before it, without the rejected + * one-breadcrumb-per-edit design (see that module's header). */ + const onEdit = useCallback( + (path: string, contents: string, opts?: WriteFileOptions) => { + recordEditorEvent({ + kind: "edit", + source: "editor", + path, + quiet: Boolean(opts?.quiet), + size: contents.length, + }); + writeFile(path, contents, opts); + }, + [writeFile], + ); + + /** Passed to `StylePanel` as `applyEdit` instead of the bare `onEdit` + * (DEV-2859). `opts?.quiet` is *already* how StylePanel distinguishes its + * two callsites — `patchLive` writes quietly (`{ quiet: true }`), `reset` + * does not — so this wrapper reuses that existing signal rather than + * requiring a StylePanel.tsx change to carry a new one. */ + const onEditFromStyle = useCallback( + (path: string, contents: string, opts?: WriteFileOptions) => { + recordEditorEvent({ + kind: "edit", + source: opts?.quiet ? "style" : "style-reset", + path, + quiet: Boolean(opts?.quiet), + size: contents.length, + }); + writeFile(path, contents, opts); + }, + [writeFile], + ); + + /** Passed to `ChatPanel` as `applyEdit` instead of the bare `onEdit` + * (DEV-2859). Unlike StylePanel, Chat's `apply()`/`undo()` have no existing + * signal to tell them apart — `ChatPanelProps.applyEdit`'s third parameter + * is new, added for exactly this. */ + const onEditFromChat = useCallback( + (path: string, contents: string, isUndo?: boolean) => { + recordEditorEvent({ + kind: "edit", + source: isUndo ? "ai-undo" : "ai", + path, + quiet: false, + size: contents.length, + }); + writeFile(path, contents); + }, + [writeFile], + ); + /** Send a message into the running preview (DEV-2496: the Style panel's live theme * patch). Cross-origin on both tiers — the bundler's origin for Tier 1, the * container's for Tier 2 — which postMessage is fine with. */ @@ -2378,6 +2547,11 @@ function Authoring({ * that rebuild is a container round trip of several seconds, and it used to say so * when theme edits went out as ordinary writes. */ const flushQuietEdits = useCallback(() => { + // DEV-2859: the only caller today is StylePanel (a colour-drag's quiet + // writes, flushed once the drag ends), hence `source: "style"` — there is + // no generic "flush" source in the trail's vocabulary because nothing + // else calls this yet. + recordEditorEvent({ kind: "flush-quiet", source: "style", path: "", quiet: false, size: 0 }); try { runtimeRef.current?.flushQuiet?.(); showSyncing(); @@ -3045,7 +3219,7 @@ function Authoring({ token={getToken()} htVersion={version} getFiles={() => filesRef.current} - applyEdit={onEdit} + applyEdit={onEditFromStyle} postToPreview={postToPreview} onPreviewMessage={onPreviewMessage} flushQuietEdits={flushQuietEdits} @@ -3060,7 +3234,7 @@ function Authoring({ htVersion={version} docsPath={docsPath} getFiles={() => filesRef.current} - applyEdit={onEdit} + applyEdit={onEditFromChat} onClose={() => setChatOpen(false)} /> )} diff --git a/runner/apps/authoring/src/Chat.tsx b/runner/apps/authoring/src/Chat.tsx index 0249aa7d9..3024ab99f 100644 --- a/runner/apps/authoring/src/Chat.tsx +++ b/runner/apps/authoring/src/Chat.tsx @@ -51,8 +51,12 @@ export interface ChatPanelProps { docsPath: string | null; /** Read the editor's live files at send time, not at mount time. */ getFiles: () => FilesMap; - /** Write a file back into the editor + running preview. */ - applyEdit: (path: string, contents: string) => void; + /** Write a file back into the editor + running preview. `isUndo` is a pure + * discriminator for the caller's own instrumentation (DEV-2859) — an Apply + * and an Undo call this with the same two-argument shape otherwise, and the + * App-owned wrapper needs a way to tell them apart without Chat.tsx + * importing anything Sentry-related itself. */ + applyEdit: (path: string, contents: string, isUndo?: boolean) => void; onClose: () => void; } @@ -213,7 +217,7 @@ export function ChatPanel({ function undo(index: number) { const turn = turnsRef.current[index]; if (!turn?.undo) return; - for (const [path, contents] of Object.entries(turn.undo)) applyEdit(path, contents); + for (const [path, contents] of Object.entries(turn.undo)) applyEdit(path, contents, true); setTurns((current) => current.map((t, i) => (i === index ? { ...t, undo: undefined } : t))); reportChatEvent(apiBase, "edit_undone", framework); } diff --git a/runner/apps/authoring/src/auth.ts b/runner/apps/authoring/src/auth.ts index 809e72dd8..b024bcb4b 100644 --- a/runner/apps/authoring/src/auth.ts +++ b/runner/apps/authoring/src/auth.ts @@ -10,6 +10,7 @@ // and the admin writes on the server side. import { reportError } from "./sentry.js"; +import { applyUserContext, resetUserContext } from "./userScope.js"; const BROKER = import.meta.env.VITE_LOGIN_BROKER_URL || "https://mcp-auth-proxy-j0tb.onrender.com"; const API_BASE = import.meta.env.VITE_API_BASE || "http://localhost:8787"; @@ -47,7 +48,10 @@ export function isTokenSession(): boolean { export async function currentUser(): Promise { // Local dev bypass — set VITE_DEV_USER in .env.local; never set in production. const devUser = import.meta.env.VITE_DEV_USER as string | undefined; - if (devUser) return { email: devUser }; + if (devUser) { + void applyUserContext({ email: devUser }, { devUser, token: getToken() }); + return { email: devUser }; + } const hash = new URLSearchParams(location.hash.slice(1)); if (hash.get("error")) { @@ -76,9 +80,12 @@ export async function currentUser(): Promise { }); if (!res.ok) { sessionStorage.removeItem(TOKEN_KEY); + resetUserContext(); return null; } - return (await res.json()) as User; + const resolved = (await res.json()) as User; + void applyUserContext(resolved, { token }); + return resolved; } catch (error) { // The broker being unreachable presents as "signed out" with no explanation, // and every write endpoint then rejects. @@ -110,6 +117,7 @@ export function clearSession(): void { // network corrects it. Removed by name rather than `sessionStorage.clear()`: // this key is ours, the rest of the origin's storage is not. sessionStorage.removeItem(PROFILE_CACHE_KEY); + resetUserContext(); } /** diff --git a/runner/apps/authoring/src/catalog.ts b/runner/apps/authoring/src/catalog.ts index 04ab922c5..b7f7a593c 100644 --- a/runner/apps/authoring/src/catalog.ts +++ b/runner/apps/authoring/src/catalog.ts @@ -2,6 +2,7 @@ import { stableBucketVersions } from "@handsontable/demo-runtime"; import type { Catalog, CatalogIndexEntry } from "@handsontable/demo-runtime"; import catalogJson from "../../../catalog.json"; import docsBucketsJson from "../../../docs-buckets.json"; +import { fetchWithDiagnostics, type FetchDiagnostics } from "./fetchDiagnostics.js"; // The index only (~15 KB): framework rows without files. Full starter // artifacts are lazy-fetched per version bucket — see starter-catalog.ts. @@ -40,13 +41,24 @@ export const VERSION_OPTIONS = stableBucketVersions(catalog.bucketVersions); * "the visitor has not chosen" before swapping in npm `latest`. */ export const DEFAULT_VERSION = VERSION_OPTIONS[0]; -/** Fetch real published versions from the API (npm-backed). */ +/** + * Fetch real published versions from the API (npm-backed). + * + * A thin caller over `fetchWithDiagnostics` (DEV-2859): the retry + timeout + * policy and the attempt bookkeeping live there, import-free and unit-tested; + * this function stays exactly what it was for the one line that a Sentry + * population (DEMOS-2X) already groups on — `if (!res.ok) throw new Error(...)` + * is byte-identical, so that grouping does not move. The `diagnostics` on the + * success path let the caller (App.tsx) tell a retried-then-recovered blip + * from a clean first try, without this module importing Sentry. + */ export async function fetchVersions( apiBase: string, -): Promise<{ latest: string | null; next: string | null; versions: string[] }> { - const res = await fetch(`${apiBase}/api/versions`); +): Promise<{ latest: string | null; next: string | null; versions: string[]; diagnostics: FetchDiagnostics }> { + const { res, diagnostics } = await fetchWithDiagnostics(`${apiBase}/api/versions`); if (!res.ok) throw new Error(`versions ${res.status}`); - return (await res.json()) as { latest: string | null; next: string | null; versions: string[] }; + const body = (await res.json()) as { latest: string | null; next: string | null; versions: string[] }; + return { ...body, diagnostics }; } /** Is `version` an exact published Handsontable version on npm? Used to detect diff --git a/runner/apps/authoring/src/editorTrail.ts b/runner/apps/authoring/src/editorTrail.ts new file mode 100644 index 0000000000000000000000000000000000000000..6978dde0e98e2fc78c372c6ee5fd7c08c90b3973 GIT binary patch literal 7859 zcmbVR?QYygvc13c6cyoOGvjDRvhOZ%?bW2!g)&gCYwW#%(!YL^!h7UHwK3AMPi z$=~J8_DEdQxOBxKL7J79(%&_5<;$ktihqay2gk=Ux|shkdGz0Z`Jbcy(Z&4v%d5%5 zi}4iCIpp>Huk$C@^9#9{UtNCpVkV*VlGsF3mu279-eqasu5S{ld@V^R=ddN2N)mOo zX%iEPzKNVIS-ZR48*wI&PEIiNTn?YRyL{cQrL8j8c*%U#ti<1?-eaGKQiM=R>WU%` zD{#JM724>^EYb%}zV`7{4ZInwQlt2yY(iUPvdHn1msnY15t=BQyjgLwJcGeG#tIG3 zeH6?LXXR<5F05*w!q4SW(oi<^XR~rm_u7Kv;b^t0&X-I0;vawiKWrp6b=dMPh9Q&% z{m-RRk2`skbmhvW$6oUYV-T)16opS4MnEph*ucRv8@dizTlg=r+f6xOpyu2+?P7ry zN2#k^3INEXKaYov!QAN67SQmM-6qrE3LjIveIzl6lm9w7Iho{*uU(Ue(p()Xij3doMRzx)6?O zf!9!CeYf_4m_@A1+4b6nxV&t{8G35ZIbiX3SX_YxNG*JmuAU+f+IqMbeT?B5E=zN1?PDn3c|;-RH&}Hk$@KVcighGb zKFt9yL>K<%FMx7gPy`z5f2cRB4Yb?2mkS9M6? ztWYO}p0S2FZZ4LtimTA*6a_;mdC3gUOL*??qfH?4*m&Md=pa~##DH~u@UMS-DNirw z&n~Xy>Fbxz<@)=%ynKE6-Q|n(XL5b{`0BFh zD53=C0Kt}Jq=K#7=4FNn;0y#Pv^8>fl^}O--O|S%a&hI0A{m3tkrNS$ZI?)aCr)iOI2L}K& z@=pk2AuZ#Q!&>O{#z969#71( zE!IX^t1GZhqx%&#WF{oSa!D%{1~U}0Pz2G#_ z7Yi_TCpq9|l(|r5o!zcpwj^Ihks12V_R*EkE{)TWdCM=2rwOVWImwborBrXb>5Mo{ zJfR>-0tHaJsloG_Ld@cd&>+S$DWQpceR}Z0!1fXalpCLPrJBVQSS#ss7n_SF5r@iI zg{mz8whYSyaDg(xN!^x!oMLQOz)4Lk%_|tw6?0_*lwodnk=H0ye}FW#d9M8S9(zR6 z?~E47n*FPP4~9hE9|!!ts|>i`hmEXVX&Rx^EuRr*mVfXfG8cHJ+C=g`G*Fja(CDKr z+<`<_`sY|r_jZsh_czI`c@BM;8;k&_+*Fz-%)1;~Qz(e)TGbn>q1u4p9OrHl0Vb72^5p#0`IF1* zpTN71{(P!*_QX}hY@#%dxh#Eb;eg8F%g`=YtQ@!AS7t}CqX;rY7SgCF>xqJ;8jVBIGKY#?RTQSE4+RzYwBc#z9@9EK690! zjXATNVxGn@Jvvnq8`d2M7;jXFE9+){0W=SWZB}4Xwj6Mp0%*~es<3x%9BD?}D;rvD z#(kb@)u}yP4k-QSdmTPy`hWP3{>rFxcg=9`u9Ve3Buae4`luAIFlxl1{Ysx*k^?&8 zm&Y=I;v3qc&tj3Mespp)JR5)b$KU@yk(okJS1mjm5p9V2eeS64=U)J@pMN<$;2<`| zSN<&_XE&H`S3Tl-J0U{K79Hc6{@V?_X203Eb%iHAU|)pI5oGeO=mb>uX=6o6%ya;v zKu4jFBy09)pZzD57;>%9CbmY?#JqU{ZZpXRUS~6D@Cd+R-Sr;UPg!3`d9H+nsw|DW z1^lUE(;}p|mH;Fl$QLwQ&>R@9z2y;nMF>$3qDJ8#7-}yQXdX2u+l0x6YL&KiOJe^Z zu+fm*9lEXegC?#6YPOl3bY)ug7Hy?1OPh_V!}PUwkz5i3XwDvjiULX)4&`FHsZK*v ziL)R(H7sthef7PWI4Q7vA*&E>2a?u+ut~rYg;6cxpp(vIG?uf+UHL|6?eYTqoR(oT z8h6xmji64nMw=}p6zT@3m3tx)wSy`>mGf7ZgDhut+n|FhmufRb2nm)46m07DtM=f4 zH*cVCpD>tDL(SDk3skSszO~~Hs>rN&AoHLSI-zUBKdlaci^Q0g6RPjW_MU(C3v&)r5+2cpmM;=BT@>($!6+E zbN9#u;W#1sIfs%rBEYo4EU_1aT`RY*1vNWLkTe!$@2P-fn_|(wQJa#F6*eT-4dQmn z;XJjVPRD8p2B~Jqrw;3?B2os4#NCj~z&B%wM59d0xT!((4?+E; zS@;8f-i4HBCuSm?(=n?NHSI=7g+61M$(QioP(w%WWjdYeNOQS-*c%LV)7eBAQ9jI} z1ldvLS6>1C{S&e!-Y}}&k@ksv)7|^|{-cRJ`~^JC^AR)$KI0nj(@3p6JCl3tD}VZv z-WV&2MSo9A`AWGOjfvebeFP(D{6y~!w9M(dVbHepgMWJ}I7}coD8Kckop{vy)Nf#7 zRc<@2w>J|j@>$m)u{;kmQnjnge*>QVbU6h8eotNu^9{z@7R+R;zWj~U?46y_)!gph zEASbeeFCTc&i=aqROq@#?{!r;;YfdEjr})iZ}8Eiv|*ZVdY>VR-gcuf)iWc|{NCl9tq$Z3!`$h&&ZBErjYd$OsA?Sp z!0Sf*pPuMEgF1fkZJXYrAmU8t>xUII->%(~2RbzlGw~&WCvsXR*t?@E zvBr+t9GSf9=nxuoah(7{7m%a3L~R<*LHEGo3iMe#(EA)r`l|LCz_0@eqSX=%cA#kG z5gij)BiTD8OB7vHLax}jEzdStKX;UqCg7dbF=5AW1#=197Tt7?N1&hK?;02*7P+$I)_V zM|4Q-2U4Cr9QQKQj?AFk?6U(_wL=Xn`m(9<<-ruFQnPn;fiv&!q@0r>s~9bRhm%$- zX$8)U?Q_-M*AwR5RGy;pM!CUxD#K$@ZlMO?an7aOBqN3vp3^OjRSV)^9=8Tmm?WSW zWn&1A;PEBDX?!k4=`biWqKQCbdYYj6+K#-ScJ%1$v8L#!jCV#lAF|>72ZEilltzD= z=}1mX@6Qz3FSIBv-~)PqACE|O-TrO!n42x_hzsPyTzwHor9Q>&$82mQpG^89Y^nmS zOqT0sy1|D0moI3pW#{l*`$qW!`#Ri%^kF}n@ii77Ua&bt`2FtIE2CM{v_LfX>x4?TnG-EWx3xfTi}L98H3m$ Pa~R#WCs5cIe>nIjEY(p# literal 0 HcmV?d00001 diff --git a/runner/apps/authoring/src/fetchDiagnostics.ts b/runner/apps/authoring/src/fetchDiagnostics.ts new file mode 100644 index 000000000..23db9f0fe --- /dev/null +++ b/runner/apps/authoring/src/fetchDiagnostics.ts @@ -0,0 +1,277 @@ +// Measurement + retry policy for `fetchVersions` / `fetchDocsJson` (DEV-2859, +// Sentry DEMOS-2X / DEMOS-7D). +// +// `fetchFailure.ts` (DEV-2858 / the sibling fix in +// fix/DEV-2859-opaque-fetch-host-suffix) only classifies a failure shape after +// the fact — it has no measurement and no remedy. This module adds both: a +// bounded retry so a transient blip doesn't become an issue, and a diagnostics +// bundle (attempts, outcome, elapsed, online) so a real dip is distinguishable +// from one visitor's dropped tab without either population being silenced. +// +// Import-free by construction, same reason and same constraint as +// `reportingGate.ts` / `fetchFailure.ts` / `sessionDiagnostics.ts` / +// `identity.ts`: `pipeline/fetch-diagnostics.test.mjs` imports this directly +// under `--experimental-strip-types`, which cannot resolve a sibling `./x.js` +// specifier (verified empirically). Every ambient dependency (fetch, the +// clock, `navigator.onLine`, the retry delay) is an injected parameter with a +// browser-real default, so node can drive every branch without a DOM. +// +// RETRY IS THE DISCRIMINATOR, not the classifier. A visitor's own network +// dropping mid-request (DEMOS-2X) and our host having a real dip both start +// the same way — a fetch that never completes — but only the first one +// recovers 300ms later. `versions_fetch_attempts` (attached to the thrown +// error, read at the callsite) is the tag that tells them apart: a dip fails +// twice, a blip does not. +// +// `!res.ok` (our host answering, e.g. a 503) is deliberately NOT retried here +// at all — retrying a real outage amplifies it, and the caller already reports +// that population as it always has (`catalog.ts`'s +// `if (!res.ok) throw new Error(...)` stays byte-identical). This module's +// retry only ever fires on a *thrown* fetch failure — a request that never +// produced a response. +// +// A timeout (the per-attempt AbortController, 5s, copied from +// `checkVersionExists` in catalog.ts) does NOT retry either: retrying a stall +// would either double a 5s wait into a 10s one on the version picker, or turn +// one outage into a retry storm across every open tab. It is its own outcome +// value (`"timeout"`) rather than being folded into `"transport"`. + +export interface FetchDiagnosticsDeps { + fetchFn?: typeof fetch; + /** A monotonic clock. `performance.now` in the browser; injectable so a test + * can control elapsed time without real timers. */ + now?: () => number; + isOnline?: () => boolean | undefined; + sleep?: (ms: number) => Promise; + /** Per-attempt abort budget. Defaults to 5000ms — the same figure + * `checkVersionExists` uses, for the same reason (see its own comment). */ + timeoutMs?: number; + /** Delay before the single retry. Defaults to 300ms. */ + retryDelayMs?: number; +} + +export type FetchOutcome = "ok" | "transport" | "timeout"; + +export interface FetchDiagnostics { + /** 1 (no retry attempted, or a timeout — which never retries) or 2 (a + * transport failure was retried once). The decisive tag: a dip fails at + * both attempts, a blip only fails the first. Always populated by + * `fetchWithDiagnostics`; optional only because `docs-catalog.ts`'s + * `fetchDocsJson` has no retry of its own to report (it cannot import this + * module and stay node-importable — see this file's header) and still + * reuses `diagnosticTags`/`diagnosticExtras` with a thinner bundle + * (DEV-2859's DEMOS-7D ruling). */ + attempts?: number; + outcome?: FetchOutcome; + /** `navigator.onLine` at the start of the call. `undefined` outside a + * browser. `true` proves nothing (an extension or captive portal still + * refuses the request); only an explicit `false` is evidence either way — + * same caution `tier1Report.ts` documents for the same signal. */ + onlineAtStart: boolean | undefined; + elapsedMs?: number; + /** Set by the caller (`context: "versions-fetch"` or `"docs-fetch"`) before + * tagging — this module has no opinion on which population it is instrumenting. */ + context?: string; + apiBaseOrigin?: "same" | "cross" | "localhost"; + /** Chromium-only (`navigator.connection.effectiveType`); omitted elsewhere. */ + netEffectiveType?: string; +} + +const DEFAULT_TIMEOUT_MS = 5000; +const DEFAULT_RETRY_DELAY_MS = 300; + +function defaultIsOnline(): boolean | undefined { + return typeof navigator !== "undefined" ? navigator.onLine : undefined; +} + +function defaultSleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +/** Attach diagnostics to a thrown error as a non-enumerable property, leaving + * `name`/`message` untouched — `isOpaqueNetworkFailure` (fetchFailure.ts) + * must still classify the underlying error by its own wording, and a + * non-enumerable property does not show up in `JSON.stringify`, a `for...in`, + * or Sentry's own error serialisation, so no grouping shifts. */ +function attachDiagnostics(error: unknown, diagnostics: FetchDiagnostics): unknown { + if (typeof error === "object" && error !== null) { + Object.defineProperty(error, "fetchDiagnostics", { + value: diagnostics, + enumerable: false, + configurable: true, + }); + } + return error; +} + +export function readFetchDiagnostics(error: unknown): FetchDiagnostics | undefined { + if (typeof error !== "object" || error === null) return undefined; + return (error as { fetchDiagnostics?: FetchDiagnostics }).fetchDiagnostics; +} + +interface AttemptResult { + res?: Response; + error?: unknown; + timedOut: boolean; +} + +async function attemptOnce( + fetchFn: typeof fetch, + url: string, + init: RequestInit | undefined, + timeoutMs: number, +): Promise { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), timeoutMs); + try { + const res = await fetchFn(url, { ...init, signal: controller.signal }); + return { res, timedOut: false }; + } catch (error) { + return { error, timedOut: controller.signal.aborted }; + } finally { + clearTimeout(timer); + } +} + +/** + * Fetch with the DEV-2859 retry policy. Resolves with `{ res, diagnostics }` + * whenever a response was produced (`res.ok` may still be false — this + * function has no opinion on HTTP status, only on whether the request + * completed at all). Throws the terminal error, decorated with diagnostics, + * when it never does. + */ +export async function fetchWithDiagnostics( + url: string, + init?: RequestInit, + deps: FetchDiagnosticsDeps = {}, +): Promise<{ res: Response; diagnostics: FetchDiagnostics }> { + const fetchFn = deps.fetchFn ?? fetch; + const now = deps.now ?? (() => performance.now()); + const isOnline = deps.isOnline ?? defaultIsOnline; + const sleep = deps.sleep ?? defaultSleep; + const timeoutMs = deps.timeoutMs ?? DEFAULT_TIMEOUT_MS; + const retryDelayMs = deps.retryDelayMs ?? DEFAULT_RETRY_DELAY_MS; + + const onlineAtStart = isOnline(); + const start = now(); + + const first = await attemptOnce(fetchFn, url, init, timeoutMs); + if (first.res) { + return { + res: first.res, + diagnostics: { attempts: 1, outcome: "ok", onlineAtStart, elapsedMs: now() - start }, + }; + } + if (first.timedOut) { + // A timeout never retries — see this file's header. + throw attachDiagnostics(first.error, { + attempts: 1, + outcome: "timeout", + onlineAtStart, + elapsedMs: now() - start, + }); + } + + // A transport failure (the request never completed, and it wasn't our own + // abort) gets exactly one retry. + await sleep(retryDelayMs); + const second = await attemptOnce(fetchFn, url, init, timeoutMs); + if (second.res) { + return { + res: second.res, + diagnostics: { attempts: 2, outcome: "ok", onlineAtStart, elapsedMs: now() - start }, + }; + } + throw attachDiagnostics(second.error, { + attempts: 2, + outcome: second.timedOut ? "timeout" : "transport", + onlineAtStart, + elapsedMs: now() - start, + }); +} + +// --- Buckets, origin classification, tags/extras --------------------------- + +/** Fetch-scale elapsed buckets — distinct boundaries from + * `sessionDiagnostics.ts`'s `elapsedBucket`, which is scaled for a Tier-2 + * container boot (seconds to ~120s), not a single HTTP round trip. */ +const FETCH_BOUNDARIES_MS = [100, 500, 1000, 3000, 5000]; + +export function elapsedBucket(ms: number): string { + for (const boundary of FETCH_BOUNDARIES_MS) { + if (ms < boundary) { + return boundary >= 1000 ? `<${boundary / 1000}s` : `<${boundary}ms`; + } + } + return ">=5s"; +} + +/** Is the API base same-origin, cross-origin, or the committed localhost + * fallback (`catalog.ts` / `auth.ts`'s `API_BASE`)? A `localhost` result in + * production means `VITE_API_BASE` never made it into the build — a single + * candidate cause that would fail the request for every visitor of that + * deploy, and would settle DEMOS-2X outright. */ +export function apiBaseOrigin(apiBase: string, pageOrigin: string): "same" | "cross" | "localhost" { + let apiHost: string; + try { + apiHost = new URL(apiBase, pageOrigin).host; + } catch { + return "cross"; + } + if (/^localhost(:\d+)?$/.test(apiHost) || /^127\.0\.0\.1(:\d+)?$/.test(apiHost)) return "localhost"; + let pageHost: string; + try { + pageHost = new URL(pageOrigin).host; + } catch { + return "cross"; + } + return apiHost === pageHost ? "same" : "cross"; +} + +/** `context` is required and drives the tag-name prefix, so the same + * diagnostics shape can tag both the `versions-fetch` (DEMOS-2X) and + * `docs-fetch` (DEMOS-7D) populations without one borrowing the other's tag + * names in the Sentry UI. */ +function prefixFor(diag: FetchDiagnostics): string { + return (diag.context ?? "fetch").replace(/-/g, "_"); +} + +export function diagnosticTags(diag: FetchDiagnostics): Record { + const prefix = prefixFor(diag); + const tags: Record = { + context: diag.context ?? "fetch", + }; + if (diag.attempts !== undefined) tags[`${prefix}_attempts`] = String(diag.attempts); + if (diag.outcome !== undefined) tags[`${prefix}_outcome`] = diag.outcome; + if (diag.elapsedMs !== undefined) tags[`${prefix}_elapsed_bucket`] = elapsedBucket(diag.elapsedMs); + if (diag.onlineAtStart !== undefined) tags[`${prefix}_online`] = String(diag.onlineAtStart); + if (diag.apiBaseOrigin) tags.api_base_origin = diag.apiBaseOrigin; + if (diag.netEffectiveType) tags.net_effective_type = diag.netEffectiveType; + return tags; +} + +export function diagnosticExtras(diag: FetchDiagnostics): Record { + const extras: Record = {}; + if (diag.elapsedMs !== undefined) extras.elapsedMs = String(Math.round(diag.elapsedMs)); + if (diag.attempts !== undefined && diag.outcome !== undefined) { + extras.attemptSummary = `${diag.attempts} attempt${diag.attempts === 1 ? "" : "s"}, ${diag.outcome}`; + } + if (diag.apiBaseOrigin) { + // Host only — never a full URL, never a query string. `apiBaseOrigin`'s + // caller passes the classification, not the raw base, so there is nothing + // more specific to redact here; this extra exists so the classification + // shows up in the event body, not just as a filterable tag. + extras.apiBaseClass = diag.apiBaseOrigin; + } + return extras; +} + +/** Chromium-only `navigator.connection.effectiveType` (e.g. "4g", "3g"). + * `undefined` on every other engine, and the caller omits the tag entirely + * rather than sending `"undefined"`. */ +export function netEffectiveType(): string | undefined { + const nav = typeof navigator !== "undefined" ? (navigator as Navigator & { + connection?: { effectiveType?: string }; + }) : undefined; + return nav?.connection?.effectiveType; +} diff --git a/runner/apps/authoring/src/identity.ts b/runner/apps/authoring/src/identity.ts new file mode 100644 index 000000000..c673c2bb6 --- /dev/null +++ b/runner/apps/authoring/src/identity.ts @@ -0,0 +1,112 @@ +// Visitor / user identity decisions for Sentry context (DEV-2859). +// +// Import-free by construction, same reason and same constraint as +// `reportingGate.ts` / `fetchFailure.ts` / `sessionDiagnostics.ts`: this is the +// one piece of `pipeline/*.test.mjs` can import directly under +// `--experimental-strip-types`, because the Sentry-touching wiring +// (`userScope.ts`, `main.tsx`, `auth.ts`) imports `@sentry/react` and/or reads +// `import.meta.env`, neither of which node resolves. Do not let this file grow +// imports — a sibling `./x.js` specifier does not resolve under strip-types +// either (verified empirically against a throwaway probe file). +// +// WHY THIS EXISTS AT ALL: every `setUser` call in `apps/**` turns out to be +// React local state — nothing ever calls `Sentry.setUser`, `Sentry.setTag`, or +// sets `sendDefaultPii`. That is why every Sentry issue in this project reports +// `users: 0`, and why a "0 users" reading misled an earlier triage into +// suppressing a population that was, in fact, affecting real visitors. + +/** Storage the visitor id is minted into and read from. A narrow structural + * type (not `Storage` itself) so a test can hand in a throwing fake without + * implementing the full Web Storage interface. */ +export interface KeyValueStorage { + getItem(key: string): string | null; + setItem(key: string, value: string): void; +} + +/** Same key `auth.ts` uses for the broker/API token — sessionStorage, not + * localStorage. Deliberately: localStorage would mint a new persistent + * pseudonymous identifier on a public site, which is a bigger privacy + * commitment than this instrumentation is trying to make. The cost of that + * choice is stated once, here, because it is easy to forget while reading a + * Sentry issue: a reload mints a new id, so `users` on an anonymous issue + * counts *sessions*, not people. It is a floor, not a headcount. */ +const VISITOR_ID_KEY = "hot_sid"; + +function randomId(): string { + // `crypto.randomUUID` is available in every browser this app supports and + // in Node 22 (used by the test only to prove the mint path is exercised). + return `s_${crypto.randomUUID().replace(/-/g, "").slice(0, 20)}`; +} + +/** + * Read the existing visitor id, or mint and persist one. + * + * Must tolerate a throwing storage without throwing itself — Safari private + * mode (and any browser with storage disabled by policy) throws on + * `sessionStorage.getItem`/`setItem`, and an identity helper crashing the app + * it exists to observe would be exactly backwards. A storage that throws + * degrades to "a fresh id every call", which is honest: nothing persisted, so + * nothing to read back next time either. + */ +export function visitorId(storage: KeyValueStorage): string { + try { + const existing = storage.getItem(VISITOR_ID_KEY); + if (existing) return existing; + const minted = randomId(); + storage.setItem(VISITOR_ID_KEY, minted); + return minted; + } catch { + return randomId(); + } +} + +/** + * `"u_" + sha256(lowercased, trimmed email).slice(0, 16)`, via `crypto.subtle` + * (available in every browser this app supports, and in Node >= 19 including + * the Node 22 this test suite runs under). + * + * Privacy, stated honestly rather than oversold: over a known internal + * address list (this is a Google-login-gated internal tool — see auth.ts) a + * truncated hash is reversible *by us*, e.g. by hashing every + * @handsontable.com address and comparing. The goal is "never ship a raw + * address to a third party" (Sentry), not anonymisation against someone who + * already has the address list. No `email`, no `username`, no raw address is + * ever set anywhere in this module or its callers. + */ +export async function hashedUserId(email: string): Promise { + const normalized = email.trim().toLowerCase(); + const bytes = new TextEncoder().encode(normalized); + const digest = await crypto.subtle.digest("SHA-256", bytes); + const hex = Array.from(new Uint8Array(digest)) + .map((b) => b.toString(16).padStart(2, "0")) + .join(""); + return `u_${hex.slice(0, 16)}`; +} + +export type AuthMode = "anonymous" | "google" | "api-token" | "dev-bypass"; + +export interface AuthModeInputs { + /** `import.meta.env.VITE_DEV_USER` — truthy means the local dev bypass in + * `auth.ts`'s `currentUser()` short-circuited. Passed in as a value, never + * read here, so this module stays import-free of `import.meta.env`. */ + devUser?: string; + /** The stored broker/API token (`getToken()` in auth.ts), or null/undefined + * when signed out. */ + token?: string | null; + /** The resolved user, when `currentUser()` succeeded. */ + user?: { email: string } | null; +} + +/** Mirrors `PAT_PREFIX` in auth.ts — passed in rather than imported, same + * reason every other input here is passed in. */ +export function authMode({ devUser, token, user }: AuthModeInputs, patPrefix: string): AuthMode { + // Checked first and independent of `user`: the dev bypass in auth.ts returns + // a `User` too (`{ email: devUser }`), so `auth_mode` would otherwise read + // "google" for a local dev session. This value doubles as a tripwire for the + // exact leak the `dist` grep in AGENTS.md exists to catch — a build that + // reports `dev-bypass` traffic in production means `VITE_DEV_USER` shipped. + if (devUser) return "dev-bypass"; + if (!user) return "anonymous"; + if (token?.startsWith(patPrefix)) return "api-token"; + return "google"; +} diff --git a/runner/apps/authoring/src/main.tsx b/runner/apps/authoring/src/main.tsx index 65bdb46e7..dd619d57b 100644 --- a/runner/apps/authoring/src/main.tsx +++ b/runner/apps/authoring/src/main.tsx @@ -1,6 +1,30 @@ // Must stay first: initialises error reporting before any other module runs, so a // throw during module evaluation is still captured. import { Sentry } from "./sentry.js"; +// Seeds an anonymous visitor id + `auth_mode` tag synchronously, right after +// reporting is initialised and before `createRoot` — so an early +// module-evaluation crash is already attributable to *a* visitor, not one of +// the `users: 0` issues that misled an earlier triage (DEV-2859). Deliberately +// not `currentUser()`: that round-trips the Render-hosted broker and would add +// its latency to every route, including ones that need no identity at all. +import { seedAnonymousContext } from "./userScope.js"; +seedAnonymousContext(); +// DEMOS-1D (DEV-2859): attach the bounded editor trail to every event this +// client sends, plus the tags derived from its most recent entry. Registered +// unconditionally — `Sentry.addEventProcessor` is a no-op when reporting is +// gated off (nothing is ever sent for the processor to run against) — so this +// import stays free of a second `reportingEnabled` check. +// +// `surface === "demo-runtime"` events are skipped: those are relayed preview +// failures (`reportDemoEvent` in sentry.ts), and this app's own editor +// activity says nothing about what a demo's own code just did. +import { snapshotEditorTrail, editorTrailTags } from "./editorTrail.js"; +Sentry.addEventProcessor((event) => { + if (event.tags?.surface === "demo-runtime") return event; + event.tags = { ...event.tags, ...editorTrailTags() }; + event.extra = { ...event.extra, editorTrail: snapshotEditorTrail() }; + return event; +}); // The code face the design specifies (Figma 48:6719 / 31:6597) — bundled, not a // CDN link, so the editor never renders a fallback face first. Loaded here, not // in the shell: editor-shell stays a side-effect-free source package, and the diff --git a/runner/apps/authoring/src/userScope.ts b/runner/apps/authoring/src/userScope.ts new file mode 100644 index 000000000..78a0861c9 --- /dev/null +++ b/runner/apps/authoring/src/userScope.ts @@ -0,0 +1,99 @@ +// Sentry user-context wiring (DEV-2859). Deliberately trivial and +// decision-free — the decisions (what id to mint, what auth_mode is, how to +// hash an email) live in `identity.ts`, which is import-free and unit-tested. +// This module cannot be: it imports `@sentry/react` via `./sentry.js`, which +// `node --test` cannot resolve (same reason as `sentry.ts` itself). +// +// Wrapped in `reportingEnabled` throughout: off-host (local dev, CI, a +// Playwright run against production — see `reportingGate.ts`) this must write +// no storage key and call no Sentry API, the same guarantee the rest of the +// reporting surface makes. + +import { Sentry, reportingEnabled } from "./sentry.js"; +import { authMode, hashedUserId, visitorId, type AuthModeInputs } from "./identity.js"; + +/** Mirrors `PAT_PREFIX` in auth.ts (see identity.ts's `authMode`). Duplicated + * rather than imported: auth.ts already imports this module's sibling + * `sentry.ts`, and a cycle back into auth.ts is not worth avoiding a second + * literal. */ +const PAT_PREFIX = "hot_pat_"; + +/** + * `identity.ts`'s `visitorId` catches a throwing storage's *method calls* + * (`getItem`/`setItem`) — but in a locked-down environment (Safari with all + * site storage denied by policy) merely reading the global `sessionStorage` + * accessor can itself throw, at the argument-evaluation site, before + * `visitorId` ever gets a value to call a method on. This is the one path in + * this whole feature that could white-screen the app it exists to observe, so + * the access is guarded here rather than assumed safe. */ +function safeSessionStorage(): Storage | { getItem(): null; setItem(): void } { + try { + return sessionStorage; + } catch { + return { getItem: () => null, setItem: () => {} }; + } +} + +/** + * Seed anonymous context. Called synchronously from `main.tsx`, before + * `createRoot`, so a crash while the module graph is still evaluating is + * already attributable to a visitor id. Never calls `currentUser()` — that + * would round-trip the Render-hosted broker and add its latency to every + * route, including the ones that need no identity at all. + */ +export function seedAnonymousContext(): void { + if (!reportingEnabled) return; + const id = visitorId(safeSessionStorage()); + Sentry.setUser({ id }); + Sentry.setTag("auth_mode", "anonymous"); +} + +export interface ApplyUserContextInputs { + /** `import.meta.env.VITE_DEV_USER`, forwarded from the caller so this module + * stays free of `import.meta.env` reads (it already imports the SDK, so + * that constraint is about keeping the *set of things* that changes small, + * not about testability — this file was never going to be node-importable). */ + devUser?: string; + token?: string | null; +} + +/** + * Upgrade (or re-seed) the user context once identity is known: the + * dev-bypass early return and the resolved-user return in `auth.ts`'s + * `currentUser()`. + * + * Never sets `email`, `username`, or any raw address — only the hash from + * `identity.ts`. `sendDefaultPii` stays unset in `sentry.ts`; this function is + * the only place a `User`'s identity reaches Sentry at all, and it never + * reaches for the field that would leak it. + */ +export async function applyUserContext( + user: { email: string } | null, + { devUser, token }: ApplyUserContextInputs, +): Promise { + if (!reportingEnabled) return; + const mode = authMode({ devUser, token, user } satisfies AuthModeInputs, PAT_PREFIX); + if (!user) { + resetUserContext(); + return; + } + // The dev-bypass email is hashed like any other — see `authMode`'s note on + // why `auth_mode` itself is what makes a `dev-bypass` leak visible, not a + // special case here. + const id = await hashedUserId(user.email); + Sentry.setUser({ id }); + Sentry.setTag("auth_mode", mode); +} + +/** + * Drop back to an anonymous identity: the 401 path in `currentUser()` and + * `clearSession()`. Re-mints from `visitorId` rather than clearing the user + * entirely, so a signed-out session still carries *some* identity instead of + * reverting to the pre-DEV-2859 "no user context at all" state. + */ +export function resetUserContext(): void { + if (!reportingEnabled) return; + const id = visitorId(safeSessionStorage()); + Sentry.setUser({ id }); + Sentry.setTag("auth_mode", "anonymous"); +} diff --git a/runner/pipeline/docs-catalog.test.mjs b/runner/pipeline/docs-catalog.test.mjs index b519203df..a3795d84c 100644 --- a/runner/pipeline/docs-catalog.test.mjs +++ b/runner/pipeline/docs-catalog.test.mjs @@ -102,6 +102,29 @@ test("manifest: a 500 rejects but is NOT missing — a transient failure stays t assert.match(error.message, /500/); }); +// DEV-2859: `fetchDocsJson` was deliberately left untouched by that task — it +// cannot import `fetchDiagnostics.ts` (a sibling `./x.js` specifier does not +// resolve under `--experimental-strip-types`, which is exactly what keeps +// this file able to import `docs-catalog.ts` at all) — so there is no retry +// and no diagnostics bundle here, only the DEMOS-7D tagging added at the +// App.tsx callsite. This guard pins that the one thing that *could* have +// silently changed — how a raw transport failure (the fetch call itself +// rejecting, never producing a Response) is classified — did not: it must +// stay a plain, non-missing rejection, distinct from the SPA-fallback case +// above. Fails if `fetchDocsJson` ever grows a `catch` that folds a thrown +// fetch error into `DocsResourceMissingError`. +test("manifest: a transport failure (fetch itself rejects) is NOT a missing bucket", async (t) => { + const original = globalThis.fetch; + globalThis.fetch = async () => { throw new TypeError("Failed to fetch"); }; + t.after(() => { globalThis.fetch = original; }); + + const error = await rejection(fetchDocsManifest("transport-failure-bucket")); + + assert.equal(isDocsResourceMissing(error), false); + assert.equal(error.name, "TypeError"); + assert.equal(error.message, "Failed to fetch"); +}); + /** The JSON fast path must consume the body with `res.json()`, never `res.text()` * + `JSON.parse`: the release manifest is ~800 KB and the whole point of testing * content-type before sniffing for `<` is to avoid that extra JS-side string copy. diff --git a/runner/pipeline/editor-trail.test.mjs b/runner/pipeline/editor-trail.test.mjs new file mode 100644 index 000000000..444fb4de0 --- /dev/null +++ b/runner/pipeline/editor-trail.test.mjs @@ -0,0 +1,195 @@ +// The DEMOS-1D editor trail (DEV-2859): a bounded ring buffer with +// consecutive-identical coalescing, replacing a rejected one-breadcrumb-per- +// `onEdit` design that would have let a "Maximum update depth exceeded" loop +// evict the very context it exists to preserve. +// +// Imported straight from the .ts, the way `pipeline/session-diagnostics.test.mjs` +// imports `sessionDiagnostics.ts` — `editorTrail.ts` must stay import-free for +// the same reason (a sibling `./x.js` specifier does not resolve under +// `--experimental-strip-types`, verified empirically against a throwaway probe +// file). + +import test from "node:test"; +import assert from "node:assert/strict"; +import { + recordEditorEvent, + snapshotEditorTrail, + editorTrailTags, + resetEditorTrail, + __setClockForTest, +} from "../apps/authoring/src/editorTrail.ts"; + +function useFakeClock(t, step = 1) { + let now = 0; + __setClockForTest(() => { now += step; return now; }); + t.after(() => __setClockForTest(null)); +} + +test.beforeEach(() => resetEditorTrail()); + +test("a plain edit is recorded with an incrementing seq", (t) => { + useFakeClock(t); + recordEditorEvent({ kind: "edit", source: "editor", path: "/index.js", quiet: false, size: 10 }); + recordEditorEvent({ kind: "edit", source: "editor", path: "/other.js", quiet: false, size: 12 }); + const snap = snapshotEditorTrail(); + assert.equal(snap.length, 2); + assert.ok(snap[1].seq > snap[0].seq); +}); + +test("same path, same signature: two writes coalesce (size does not break coalescing)", (t) => { + useFakeClock(t); + recordEditorEvent({ kind: "edit", source: "editor", path: "/index.js", quiet: false, size: 10 }); + recordEditorEvent({ kind: "edit", source: "editor", path: "/index.js", quiet: false, size: 12 }); + const snap = snapshotEditorTrail(); + // `size` is deliberately excluded from the coalescing signature (see + // editorTrail.ts's `signature`): a tight loop over a shrinking/growing + // buffer must still collapse into one slot, or the whole design would be + // defeated by the exact shape it exists to handle. + assert.equal(snap.length, 1); + assert.equal(snap[0].n, 2); + assert.equal(snap[0].size, 12); // the latest size wins +}); + +test("capacity + 5 distinct entries: the oldest is evicted", (t) => { + useFakeClock(t); + for (let i = 0; i < 29; i++) { + // A distinct signature every time (differing path), so nothing coalesces + // and this exercises the eviction path, not the coalescing one. + recordEditorEvent({ kind: "edit", source: "editor", path: `/file-${i}.js`, quiet: false, size: i }); + } + const snap = snapshotEditorTrail(); + assert.equal(snap.length, 24); // CAPACITY + // The oldest 5 (file-0..file-4) are gone; the newest (file-28) survived. + assert.equal(snap.some((e) => e.path === "/file-0.js"), false); + assert.equal(snap.some((e) => e.path === "/file-4.js"), false); + assert.equal(snap[snap.length - 1].path, "/file-28.js"); + assert.equal(snap[0].path, "/file-5.js"); +}); + +test("3000 identical pushes coalesce into ONE entry with n === 3000, and the entry pushed before the loop survives", (t) => { + useFakeClock(t); + recordEditorEvent({ kind: "workspace", source: "load", path: "docs", quiet: false, size: 0 }); + for (let i = 0; i < 3000; i++) { + recordEditorEvent({ kind: "edit", source: "editor", path: "/App.jsx", quiet: false, size: 42 }); + } + const snap = snapshotEditorTrail(); + // This is the whole design claim: a 3000-iteration loop must occupy exactly + // one slot, and the entry from before the loop must still be readable — + // with the rejected per-edit-breadcrumb design (maxBreadcrumbs: 200), both + // of these would be false: the loop alone would have overflowed 200 distinct + // slots and evicted the "load" entry long before this assertion. + assert.equal(snap.length, 2); + assert.equal(snap[0].kind, "workspace"); + assert.equal(snap[0].source, "load"); + const loopEntry = snap[1]; + assert.equal(loopEntry.kind, "edit"); + assert.equal(loopEntry.source, "editor"); + assert.equal(loopEntry.n, 3000); +}); + +test("a signature change breaks coalescing into two entries", (t) => { + useFakeClock(t); + recordEditorEvent({ kind: "edit", source: "editor", path: "/a.js", quiet: false, size: 1 }); + recordEditorEvent({ kind: "edit", source: "editor", path: "/a.js", quiet: false, size: 2 }); + recordEditorEvent({ kind: "edit", source: "style", path: "/a.js", quiet: false, size: 3 }); // source differs + const snap = snapshotEditorTrail(); + assert.equal(snap.length, 2); + assert.equal(snap[0].n, 2); + assert.equal(snap[0].source, "editor"); + assert.equal(snap[1].n, 1); + assert.equal(snap[1].source, "style"); +}); + +test("quiet is part of the coalescing signature (a style patch vs. a style reset)", (t) => { + useFakeClock(t); + recordEditorEvent({ kind: "edit", source: "style", path: "/theme.js", quiet: true, size: 5 }); + recordEditorEvent({ kind: "edit", source: "style", path: "/theme.js", quiet: false, size: 5 }); + const snap = snapshotEditorTrail(); + assert.equal(snap.length, 2); +}); + +test("size is length, not contents — the real privacy assertion", (t) => { + useFakeClock(t); + const sentinel = "SECRET_VISITOR_SOURCE_MARKER_do_not_leak_this"; + // A caller that (incorrectly) still passes `contents` alongside `size` must + // not be able to get it into the trail — this module never reads or spreads + // an unknown property from its input. + recordEditorEvent({ + kind: "edit", + source: "editor", + path: "/index.js", + quiet: false, + size: sentinel.length, + contents: sentinel, + }); + const snap = snapshotEditorTrail(); + assert.equal(JSON.stringify(snap).includes(sentinel), false); + assert.equal(snap[snap.length - 1].size, sentinel.length); +}); + +test("path is capped", (t) => { + useFakeClock(t); + const longPath = "/" + "a".repeat(500) + ".js"; + recordEditorEvent({ kind: "edit", source: "editor", path: longPath, quiet: false, size: 1 }); + const snap = snapshotEditorTrail(); + assert.ok(snap[0].path.length <= 121); // 120 + the "…" marker +}); + +// --- editorTrailTags ------------------------------------------------------- + +test("editorTrailTags reads the most recent entry", (t) => { + useFakeClock(t); + assert.deepEqual(editorTrailTags(), {}); + recordEditorEvent({ kind: "edit", source: "ai", path: "/a.js", quiet: false, size: 1 }); + for (let i = 0; i < 50; i++) { + recordEditorEvent({ kind: "edit", source: "ai", path: "/a.js", quiet: false, size: 1 }); + } + const tags = editorTrailTags(); + assert.equal(tags.editor_loop_source, "ai"); + assert.equal(tags.editor_last_kind, "edit"); + assert.equal(tags.editor_loop_n_bucket, "<100"); +}); + +test("editorTrailTags skips a trailing flush-quiet entry (a Style-panel drag ends with one)", (t) => { + useFakeClock(t); + // A colour drag: ~200 coalesced quiet writes, then the flush that always + // follows one (App.tsx's `flushQuietEdits`). Without the fix, reading the + // bare last entry would report the flush's own source ("style", n=1) + // instead of the coalesced drag underneath it — hiding the exact signal + // DEMOS-1D exists to read. + for (let i = 0; i < 200; i++) { + recordEditorEvent({ kind: "edit", source: "style", path: "/theme.js", quiet: true, size: 5 }); + } + recordEditorEvent({ kind: "flush-quiet", source: "style", path: "", quiet: false, size: 0 }); + const tags = editorTrailTags(); + assert.equal(tags.editor_loop_source, "style"); + assert.equal(tags.editor_last_kind, "edit"); // NOT "flush-quiet" + assert.equal(tags.editor_loop_n_bucket, "<1000"); // the 200-run's bucket, not the flush's n=1 +}); + +test("editorTrailTags falls back to the last entry when the trail is nothing but flushes", (t) => { + useFakeClock(t); + recordEditorEvent({ kind: "flush-quiet", source: "style", path: "", quiet: false, size: 0 }); + const tags = editorTrailTags(); + assert.equal(tags.editor_last_kind, "flush-quiet"); +}); + +test("editorTrailTags n_bucket boundaries", (t) => { + const cases = [ + [1, "<10"], + [9, "<10"], + [10, "<100"], + [99, "<100"], + [100, "<1000"], + [999, "<1000"], + [1000, ">=1000"], + ]; + for (const [n, expected] of cases) { + resetEditorTrail(); + useFakeClock(t); + for (let i = 0; i < n; i++) { + recordEditorEvent({ kind: "edit", source: "editor", path: "/a.js", quiet: false, size: 1 }); + } + assert.equal(editorTrailTags().editor_loop_n_bucket, expected, `n=${n}`); + } +}); diff --git a/runner/pipeline/fetch-diagnostics.test.mjs b/runner/pipeline/fetch-diagnostics.test.mjs new file mode 100644 index 000000000..b4e4e2dd7 --- /dev/null +++ b/runner/pipeline/fetch-diagnostics.test.mjs @@ -0,0 +1,228 @@ +// Retry policy, buckets, and tag/extra shaping for `fetchVersions` / +// `fetchDocsJson` (DEV-2859, Sentry DEMOS-2X / DEMOS-7D). +// +// The central claim under test: retry is the discriminator between a +// visitor's own network dropping mid-request (a blip, recovers on the second +// attempt) and a real host dip (fails at both attempts). `!res.ok` is +// deliberately never retried — the "exactly one call" tests below are the +// retry-storm guard for that. +// +// Every ambient dependency (fetch, the clock, `navigator.onLine`, the retry +// delay/timeout) is injected, so this drives every branch without a DOM and +// without real timers. + +import test from "node:test"; +import assert from "node:assert/strict"; +import { + fetchWithDiagnostics, + readFetchDiagnostics, + elapsedBucket, + apiBaseOrigin, + diagnosticTags, + diagnosticExtras, +} from "../apps/authoring/src/fetchDiagnostics.ts"; + +function fakeClock(step = 10) { + let t = 0; + return () => { t += step; return t; }; +} + +const noSleep = () => Promise.resolve(); + +function okResponse(extra = {}) { + return { ok: true, status: 200, ...extra }; +} + +function notOkResponse(status) { + return { ok: false, status }; +} + +// --- retry policy ------------------------------------------------------ + +test("success on the first attempt: exactly one fetch call, outcome ok", async () => { + let calls = 0; + const fetchFn = async () => { calls += 1; return okResponse(); }; + const { res, diagnostics } = await fetchWithDiagnostics("https://x/test", undefined, { + fetchFn, now: fakeClock(), isOnline: () => true, sleep: noSleep, + }); + assert.equal(calls, 1); + assert.equal(res.ok, true); + assert.equal(diagnostics.attempts, 1); + assert.equal(diagnostics.outcome, "ok"); +}); + +test("a transport failure then a 200: two calls, outcome ok, the good response is returned", async () => { + let calls = 0; + const fetchFn = async () => { + calls += 1; + if (calls === 1) throw new TypeError("Failed to fetch"); + return okResponse({ marker: "second" }); + }; + const { res, diagnostics } = await fetchWithDiagnostics("https://x/test", undefined, { + fetchFn, now: fakeClock(), isOnline: () => true, sleep: noSleep, + }); + assert.equal(calls, 2); + assert.equal(res.marker, "second"); + assert.equal(diagnostics.attempts, 2); + assert.equal(diagnostics.outcome, "ok"); +}); + +test("both attempts fail (transport): throws with attempts 2, diagnostics attached", async () => { + let calls = 0; + const fetchFn = async () => { calls += 1; throw new TypeError("Failed to fetch"); }; + await assert.rejects( + fetchWithDiagnostics("https://x/test", undefined, { + fetchFn, now: fakeClock(), isOnline: () => true, sleep: noSleep, + }), + (error) => { + assert.equal(calls, 2); + const diag = readFetchDiagnostics(error); + assert.equal(diag.attempts, 2); + assert.equal(diag.outcome, "transport"); + // name/message untouched, so isOpaqueNetworkFailure (fetchFailure.ts) + // still classifies it — the whole point of a non-enumerable property. + assert.equal(error.name, "TypeError"); + assert.equal(error.message, "Failed to fetch"); + return true; + }, + ); +}); + +test("!res.ok (our host answering, e.g. 503): retry-storm guard — exactly one fetch call", async () => { + let calls = 0; + const fetchFn = async () => { calls += 1; return notOkResponse(503); }; + const { res, diagnostics } = await fetchWithDiagnostics("https://x/test", undefined, { + fetchFn, now: fakeClock(), isOnline: () => true, sleep: noSleep, + }); + // This function has no opinion on HTTP status: a 503 is a completed request, + // so outcome is "ok" and the caller (catalog.ts) decides what a non-2xx + // status means. What matters here is the call count: retrying an outage + // would amplify it. + assert.equal(calls, 1); + assert.equal(res.ok, false); + assert.equal(diagnostics.attempts, 1); +}); + +test("aborting the first attempt: outcome timeout, one call, no raw AbortError escapes undiagnosed", async () => { + let calls = 0; + const fetchFn = async (_url, init) => { + calls += 1; + return new Promise((_resolve, reject) => { + init.signal.addEventListener("abort", () => { + reject(new DOMException("The operation was aborted.", "AbortError")); + }); + }); + }; + await assert.rejects( + fetchWithDiagnostics("https://x/test", undefined, { + fetchFn, now: fakeClock(), isOnline: () => true, sleep: noSleep, timeoutMs: 1, + }), + (error) => { + assert.equal(calls, 1); // a timeout never retries + const diag = readFetchDiagnostics(error); + // The caller (App.tsx) is instructed to branch on diagnostics.outcome, + // never on re-classifying the error itself — this is what makes that + // possible: the outcome is readable without inspecting error.name. + assert.equal(diag.outcome, "timeout"); + assert.equal(diag.attempts, 1); + return true; + }, + ); +}); + +test("onlineAtStart is read once, at the start of the call", async () => { + const fetchFn = async () => okResponse(); + const { diagnostics } = await fetchWithDiagnostics("https://x/test", undefined, { + fetchFn, now: fakeClock(), isOnline: () => false, sleep: noSleep, + }); + assert.equal(diagnostics.onlineAtStart, false); +}); + +// --- apiBaseOrigin ----------------------------------------------------- + +test("apiBaseOrigin: the committed localhost fallback reads as localhost", () => { + assert.equal(apiBaseOrigin("http://localhost:8787", "https://demos.handsontable.com"), "localhost"); +}); + +test("apiBaseOrigin: same host as the page is same", () => { + assert.equal( + apiBaseOrigin("https://demos.handsontable.com/api", "https://demos.handsontable.com"), + "same", + ); +}); + +test("apiBaseOrigin: a different host is cross", () => { + assert.equal(apiBaseOrigin("https://api.example.com", "https://demos.handsontable.com"), "cross"); +}); + +// --- elapsedBucket ------------------------------------------------------- + +test("elapsedBucket: every boundary is pinned from both sides", () => { + const pairs = [ + [0, "<100ms"], + [99, "<100ms"], + [100, "<500ms"], + [499, "<500ms"], + [500, "<1s"], + [999, "<1s"], + [1000, "<3s"], + [2999, "<3s"], + [3000, "<5s"], + [4999, "<5s"], + [5000, ">=5s"], + [50_000, ">=5s"], + ]; + for (const [ms, expected] of pairs) { + assert.equal(elapsedBucket(ms), expected, `elapsedBucket(${ms})`); + } +}); + +// --- diagnosticTags / diagnosticExtras ----------------------------------- + +test("diagnosticTags: prefixes tag names from context, versions-fetch", () => { + const tags = diagnosticTags({ + context: "versions-fetch", + attempts: 2, + outcome: "transport", + onlineAtStart: true, + elapsedMs: 6000, + apiBaseOrigin: "localhost", + }); + assert.equal(tags.context, "versions-fetch"); + assert.equal(tags.versions_fetch_attempts, "2"); + assert.equal(tags.versions_fetch_outcome, "transport"); + assert.equal(tags.versions_fetch_elapsed_bucket, ">=5s"); + assert.equal(tags.versions_fetch_online, "true"); + assert.equal(tags.api_base_origin, "localhost"); +}); + +test("diagnosticTags: prefixes tag names from context, docs-fetch (DEMOS-7D)", () => { + const tags = diagnosticTags({ + context: "docs-fetch", + attempts: 1, + outcome: "ok", + onlineAtStart: undefined, + elapsedMs: 50, + }); + assert.equal(tags.docs_fetch_attempts, "1"); + assert.equal(tags.docs_fetch_outcome, "ok"); + assert.equal("docs_fetch_online" in tags, false); // omitted when unknown +}); + +test("diagnosticExtras: host-class only, never a raw URL", () => { + const extras = diagnosticExtras({ + context: "versions-fetch", + attempts: 2, + outcome: "transport", + onlineAtStart: true, + elapsedMs: 1234, + apiBaseOrigin: "same", + }); + assert.equal(extras.elapsedMs, "1234"); + assert.match(extras.attemptSummary, /2 attempts, transport/); + assert.equal(extras.apiBaseClass, "same"); + for (const value of Object.values(extras)) { + assert.equal(String(value).includes("http"), false); + assert.equal(String(value).includes("?"), false); + } +}); diff --git a/runner/pipeline/identity.test.mjs b/runner/pipeline/identity.test.mjs new file mode 100644 index 000000000..0a1c7d312 --- /dev/null +++ b/runner/pipeline/identity.test.mjs @@ -0,0 +1,114 @@ +// Sentry identity decisions (DEV-2859): visitor ids, the signed-in id hash, and +// `auth_mode`. Every Sentry issue in this project reports `users: 0` because +// nothing ever calls `Sentry.setUser` — see `identity.ts`'s header. These tests +// pin the two things that made "0 users" a misleading reading: the id is a +// session-scoped floor, not a headcount, and the hash never leaks a raw address. +// +// Imported straight from the .ts, the way `pipeline/sentry-gating.test.mjs` +// imports `reportingGate.ts` — the root `test` script runs node with +// `--experimental-strip-types`, which cannot resolve a sibling `./x.js` +// specifier (verified empirically against a throwaway probe file), so +// `identity.ts` must stay import-free. + +import test from "node:test"; +import assert from "node:assert/strict"; +import { authMode, hashedUserId, visitorId } from "../apps/authoring/src/identity.ts"; + +const PAT_PREFIX = "hot_pat_"; + +// --- visitorId --------------------------------------------------------- + +/** A minimal in-memory stand-in for sessionStorage. */ +function memoryStorage(initial = {}) { + const store = new Map(Object.entries(initial)); + return { + getItem: (key) => (store.has(key) ? store.get(key) : null), + setItem: (key, value) => store.set(key, value), + _store: store, + }; +} + +test("visitorId mints once and returns the same id on a second call", () => { + const storage = memoryStorage(); + const first = visitorId(storage); + const second = visitorId(storage); + assert.equal(first, second); + assert.match(first, /^s_/); +}); + +test("visitorId survives a throwing storage (Safari private mode) without throwing", () => { + const throwing = { + getItem() { throw new DOMException("denied"); }, + setItem() { throw new DOMException("denied"); }, + }; + // Fails with the change reverted only if a real implementation ever forgets + // the try/catch — this is a guard against regressing that, not a behavioural + // claim about what id comes out (a throwing storage can't persist one). + assert.doesNotThrow(() => visitorId(throwing)); + assert.match(visitorId(throwing), /^s_/); +}); + +test("visitorId reads back what a prior mint wrote (same storage instance)", () => { + const storage = memoryStorage(); + const minted = visitorId(storage); + assert.equal(storage.getItem("hot_sid"), minted); +}); + +// --- hashedUserId -------------------------------------------------------- + +test("hashedUserId matches a known SHA-256 vector", async () => { + // sha256("foo@bar.com") = 0c7e6a405862e402... — pins the exact algorithm and + // truncation, so a swap to a different hash or a different slice length + // fails loudly instead of merely "still looking hash-shaped". + assert.equal(await hashedUserId("foo@bar.com"), "u_0c7e6a405862e402"); +}); + +test("hashedUserId output never contains the raw address or its local part", async () => { + // A real, non-hex-heavy local part: "artur.medrygal" contains letters + // (r, t, u, l, g, ...) that are not valid hex digits, so it cannot appear as + // a substring of a lowercase-hex string by chance — this is a real privacy + // assertion, not a coincidence-prone one. + const email = "artur.medrygal@handsontable.com"; + const id = await hashedUserId(email); + assert.equal(id.includes("@"), false); + assert.equal(id.toLowerCase().includes("artur.medrygal"), false); + assert.match(id, /^u_[0-9a-f]{16}$/); +}); + +test("hashedUserId normalises case and trailing whitespace", async () => { + const a = await hashedUserId("Foo@Bar.com"); + const b = await hashedUserId(" foo@bar.com "); + const c = await hashedUserId("foo@bar.com"); + assert.equal(a, c); + assert.equal(b, c); +}); + +// --- authMode -------------------------------------------------------------- + +test("authMode: dev bypass wins even when a resolved user is also present", () => { + // The dev-bypass early return in auth.ts's currentUser() returns a User too + // ({ email: devUser }) — this pins that devUser is checked first, or a local + // dev session would misreport as "google". + assert.equal( + authMode({ devUser: "dev@handsontable.com", token: null, user: { email: "dev@handsontable.com" } }, PAT_PREFIX), + "dev-bypass", + ); +}); + +test("authMode: no user at all is anonymous", () => { + assert.equal(authMode({ token: null, user: null }, PAT_PREFIX), "anonymous"); +}); + +test("authMode: a resolved user with a PAT-prefixed token is api-token", () => { + assert.equal( + authMode({ token: "hot_pat_abc123", user: { email: "a@b.com" } }, PAT_PREFIX), + "api-token", + ); +}); + +test("authMode: a resolved user with a non-PAT token is google", () => { + assert.equal( + authMode({ token: "some-broker-jwt", user: { email: "a@b.com" } }, PAT_PREFIX), + "google", + ); +}); From 5350bc4bbbaad3fe2dd9ab04b674dbaa7de051cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Wed, 9 Sep 2026 12:13:22 +0200 Subject: [PATCH 2/3] fix(runner): address review on the docs diagnostics tag and pin non-enumerability (DEV-2859) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both small but both undercutting what the instrumentation exists for. `withDocsFetchDiagnostics` was applied to both branches of the docs catches, so a missing-artifact failure (a 404 from a server we plainly reached) was tagged `context: "docs-fetch"` alongside a genuine transport failure. `onlineAtStart` and `apiBaseOrigin` answer "was the visitor offline / is this build pointing at localhost", and neither means anything for a missing artifact — tagging both put two different faults under one `context` and defeated the DEMOS-7D filtering the tags are for. Now gated on the transport sub-case, with `isMissingDocsResource` evaluated once per catch instead of three times. And the non-enumerability of the attached `fetchDiagnostics` property was only inferred from `name`/`message` staying intact. It is load-bearing: if it became enumerable it would surface in `JSON.stringify(error)` and in any spread, changing what downstream reporting sees — and that is exactly the kind of thing a refactor flips silently. Pinned directly via `getOwnPropertyDescriptor`, verified to go red under `enumerable: true` (13/14), and asserting the accessor still reads it so non-enumerable does not become unreachable. Co-Authored-By: Claude Opus 5 --- runner/apps/authoring/src/App.tsx | 24 +++++++++++++------- runner/pipeline/fetch-diagnostics.test.mjs | 26 ++++++++++++++++++++++ 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/runner/apps/authoring/src/App.tsx b/runner/apps/authoring/src/App.tsx index fb5dc4ef7..2ffc0a4ad 100644 --- a/runner/apps/authoring/src/App.tsx +++ b/runner/apps/authoring/src/App.tsx @@ -1920,17 +1920,25 @@ function Authoring({ // open. Tagged by which step failed so a missing artifact (docs linking // an example that was never imported) is distinguishable from a // transient fetch. - withDocsFetchDiagnostics("docs-fetch", () => - reportError(error, `docs-example-load:${isMissingDocsResource(error) ? "path" : "fetch"}`), - ); - failOpenDocs(isMissingDocsResource(error) ? "path" : "fetch"); + // Only a transport failure gets the fetch diagnostics: `onlineAtStart` + // and `apiBaseOrigin` answer "was the visitor offline / is this build + // pointing at localhost", and neither means anything for a missing + // artifact, which is a 404 from a server we plainly reached. Tagging + // both would put two different faults under one `context` and defeat + // the filtering these tags exist for. + const missing = isMissingDocsResource(error); + if (missing) reportError(error, "docs-example-load:path"); + else withDocsFetchDiagnostics("docs-fetch", () => reportError(error, "docs-example-load:fetch")); + failOpenDocs(missing ? "path" : "fetch"); } }) .catch((error) => { - withDocsFetchDiagnostics("docs-fetch", () => - reportError(error, `docs-bucket-resolve:${isMissingDocsResource(error) ? "bucket" : "fetch"}`), - ); - failOpenDocs(isMissingDocsResource(error) ? "bucket" : "fetch"); + // Same split as the example-load catch above: diagnostics only on the + // transport sub-case. + const missing = isMissingDocsResource(error); + if (missing) reportError(error, "docs-bucket-resolve:bucket"); + else withDocsFetchDiagnostics("docs-fetch", () => reportError(error, "docs-bucket-resolve:fetch")); + failOpenDocs(missing ? "bucket" : "fetch"); }); return () => { cancelled = true; }; }, [initialDocs, loadWorkspace, nextVersion, route.mode, version, versionsResolved]); diff --git a/runner/pipeline/fetch-diagnostics.test.mjs b/runner/pipeline/fetch-diagnostics.test.mjs index b4e4e2dd7..e0a8cd6e2 100644 --- a/runner/pipeline/fetch-diagnostics.test.mjs +++ b/runner/pipeline/fetch-diagnostics.test.mjs @@ -226,3 +226,29 @@ test("diagnosticExtras: host-class only, never a raw URL", () => { assert.equal(String(value).includes("?"), false); } }); + +test("the attached fetchDiagnostics property is non-enumerable", async () => { + // Load-bearing, and previously only inferred from `name`/`message` staying + // intact. If this property ever became enumerable it would surface in + // `JSON.stringify(error)` and in any spread of the error, changing what + // downstream reporting sees — and an enumerable own property is the kind of + // thing a refactor flips without noticing. Pinned directly. + let caught; + try { + await fetchWithDiagnostics("https://example.test/api/versions", { + fetch: () => Promise.reject(new TypeError("Failed to fetch")), + now: (() => { let t = 0; return () => (t += 10); })(), + sleep: () => Promise.resolve(), + onLine: () => true, + }); + } catch (error) { + caught = error; + } + assert.ok(caught, "expected the exhausted retry to throw"); + const descriptor = Object.getOwnPropertyDescriptor(caught, "fetchDiagnostics"); + assert.ok(descriptor, "fetchDiagnostics should be an own property"); + assert.equal(descriptor.enumerable, false); + assert.equal("fetchDiagnostics" in JSON.parse(JSON.stringify({ ...caught })), false); + // And the accessor still reads it, so non-enumerable does not mean unreachable. + assert.ok(readFetchDiagnostics(caught)); +}); From d936082d73e7d9c9d71bd6833560cae089cb701b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Wed, 9 Sep 2026 12:28:16 +0200 Subject: [PATCH 3/3] fix(runner): address Bugbot on the docs context tag and the mock-less test (DEV-2859) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from Bugbot on #329, both confirmed against the source. The docs `context` tag was silently overwritten. `withDocsFetchDiagnostics` set `context: "docs-fetch"` on the scope, but `run()` is a `reportError` call and that captures with `{ tags: { context } }` of its own — an event-level tag beats a scope one. So the wrapper appeared to tag a population it never tagged. The key is now dropped explicitly, with a comment saying why. Nothing is lost: the population is still identifiable by `reportError`'s own `docs-example-load:fetch` value and by the `docs_fetch_*` tag names `prefixFor` derives, neither of which collides. The non-enumerability test never used its mocks. It passed `{ fetch, now, sleep, onLine }` as the second argument (`init`) instead of the third (`deps`), with the wrong key names — the real ones are `fetchFn` and `isOnline`. So it ran the real `fetch` against example.test with a real 300ms sleep and a real 5s abort, and passed anyway, because a genuine network failure also throws with diagnostics attached. It asserted the right invariant for the wrong reason and could stall or flake in CI. Fixed to the idiom the rest of the file already uses, and given a `fetchCalls` counter asserting two attempts — so the test now proves it went through the injected mock rather than the network. Verified both ways: `enumerable: true` fails it (13/14), and restoring the original wrong-slot mocks also fails it (13/14), so it would have caught its own defect. File duration dropped from seconds to 65ms, which is the same fact from the other side. Co-Authored-By: Claude Opus 5 --- runner/apps/authoring/src/App.tsx | 20 +++++++++++++------- runner/pipeline/fetch-diagnostics.test.mjs | 21 ++++++++++++++++----- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/runner/apps/authoring/src/App.tsx b/runner/apps/authoring/src/App.tsx index 2ffc0a4ad..5e8d80aa5 100644 --- a/runner/apps/authoring/src/App.tsx +++ b/runner/apps/authoring/src/App.tsx @@ -110,13 +110,19 @@ let versionsFetchEventSent = false; */ function withDocsFetchDiagnostics(context: string, run: () => void): void { Sentry.withScope((scope) => { - scope.setTags( - diagnosticTags({ - context, - onlineAtStart: typeof navigator !== "undefined" ? navigator.onLine : undefined, - apiBaseOrigin: apiBaseOrigin(API_BASE, location.origin), - }), - ); + const { context: _population, ...tags } = diagnosticTags({ + context, + onlineAtStart: typeof navigator !== "undefined" ? navigator.onLine : undefined, + apiBaseOrigin: apiBaseOrigin(API_BASE, location.origin), + }); + // `context` is dropped on purpose. `run()` is a `reportError` call, and that + // captures with `{ tags: { context } }` of its own — an event-level tag beats + // a scope one, so setting `context` here would be silently overwritten and + // this wrapper would look like it tagged something it did not. The population + // is still identifiable two ways that do NOT collide: `reportError`'s own + // `docs-example-load:fetch` / `docs-bucket-resolve:fetch` value, and the + // `docs_fetch_*` tag names `prefixFor` derives from the context passed above. + scope.setTags(tags); run(); }); } diff --git a/runner/pipeline/fetch-diagnostics.test.mjs b/runner/pipeline/fetch-diagnostics.test.mjs index e0a8cd6e2..2aff3eca3 100644 --- a/runner/pipeline/fetch-diagnostics.test.mjs +++ b/runner/pipeline/fetch-diagnostics.test.mjs @@ -234,16 +234,27 @@ test("the attached fetchDiagnostics property is non-enumerable", async () => { // downstream reporting sees — and an enumerable own property is the kind of // thing a refactor flips without noticing. Pinned directly. let caught; + let fetchCalls = 0; try { - await fetchWithDiagnostics("https://example.test/api/versions", { - fetch: () => Promise.reject(new TypeError("Failed to fetch")), - now: (() => { let t = 0; return () => (t += 10); })(), - sleep: () => Promise.resolve(), - onLine: () => true, + // Mocks go in the THIRD argument (`deps`), and the keys are `fetchFn` / + // `isOnline`. Passed as the second argument (`init`) with the wrong names, + // this ran the REAL fetch against example.test with a real 300ms sleep and a + // real 5s abort — and still passed, because a genuine network failure also + // throws with diagnostics attached. It asserted the invariant for the wrong + // reason and could stall or flake. Caught by Bugbot on PR #329. + await fetchWithDiagnostics("https://x/api/versions", undefined, { + fetchFn: () => { fetchCalls += 1; return Promise.reject(new TypeError("Failed to fetch")); }, + now: fakeClock(), + isOnline: () => true, + sleep: noSleep, }); } catch (error) { caught = error; } + // Proves the injected mock was actually used rather than the real `fetch`: + // two attempts means the retry ran through `fetchFn`. Without this the test + // could pass off a genuine network failure, which is how it shipped broken. + assert.equal(fetchCalls, 2); assert.ok(caught, "expected the exhausted retry to throw"); const descriptor = Object.getOwnPropertyDescriptor(caught, "fetchDiagnostics"); assert.ok(descriptor, "fetchDiagnostics should be an own property");