diff --git a/app/__tests__/safety/crisis-button-telemetry-ordering.test.tsx b/app/__tests__/safety/crisis-button-telemetry-ordering.test.tsx new file mode 100644 index 00000000..b8c1f22f --- /dev/null +++ b/app/__tests__/safety/crisis-button-telemetry-ordering.test.tsx @@ -0,0 +1,190 @@ +/** + * CRISIS BUTTON — TELEMETRY MUST NEVER GATE THE DIAL (INFRA-297) + * + * The invariant under test: on any crisis-button activation, the navigate-or-dial + * call is the FIRST thing that happens, unconditionally, and no telemetry failure + * can prevent it, delay it, or silence it. + * + * Why this file exists. Before INFRA-297, `handleCrisisAction` called `onNavigate()` + * from INSIDE a `Sentry.startSpan(...)` callback. `startSpan` does real work before + * it ever invokes that callback — async-context-strategy dispatch, a scope fork, a + * sampling decision, span creation (see @sentry/core .../tracing/trace.js). If any + * of it throws, the callback never runs, so **the crisis tap produces nothing at + * all**: no navigation, no dial, and no log — because the audit call lived inside + * the same callback. That is a false negative on a zero-false-negative path. + * + * Wrapping the span in try/catch does NOT fix it. That converts a visible crash + * into a silently swallowed tap, which is worse. Hence the ordering assertions + * below, not merely the occurrence ones: an occurrence-only test would pass a + * try/catch non-fix. `onNavigate` must run BEFORE any telemetry call, not just + * eventually. + * + * Gating: this file is deliberately named and placed to be picked up by BOTH + * `npm run test:safety` (precommit, `__tests__/safety` pattern) and the CI + * `crisis-validation` job (`[Cc]risis` pattern). A test no gate runs is decoration. + */ + +import React from 'react'; +import { render, fireEvent } from '@testing-library/react-native'; + +/** + * Shared call-order ledger. Every mocked telemetry entry point and the + * `onNavigate` spy append to it, so ordering — not just occurrence — is + * assertable. + */ +const callOrder: string[] = []; + +const mockStartSpan = jest.fn(); + +jest.mock('@sentry/react-native', () => ({ + get startSpan() { + return mockStartSpan; + }, +})); + +jest.mock('@/core/services/logging', () => ({ + logSecurity: jest.fn(() => { + callOrder.push('logSecurity'); + }), + logPerformance: jest.fn(() => { + callOrder.push('logPerformance'); + }), + logCrisis: jest.fn(() => { + callOrder.push('logCrisis'); + }), +})); + +import CollapsibleCrisisButton from '@/features/crisis/components/CollapsibleCrisisButton'; + +const TEST_ID = 'crisis-button-prominent'; + +/** Fresh spy that records into the shared ledger. */ +function makeNavigateSpy(): jest.Mock { + return jest.fn(() => { + callOrder.push('onNavigate'); + }); +} + +function pressCrisisButton(onNavigate: jest.Mock): void { + const { getByTestId } = render( + , + ); + fireEvent.press(getByTestId(TEST_ID)); +} + +beforeEach(() => { + callOrder.length = 0; + jest.clearAllMocks(); + // Default: a well-behaved span that records its own position in the ledger. + mockStartSpan.mockImplementation((_options: unknown, callback?: (span: unknown) => void) => { + callOrder.push('startSpan'); + return callback?.({ + setAttribute: () => { + callOrder.push('span.setAttribute'); + }, + }); + }); +}); + +describe('INFRA-297 — the dial survives any telemetry failure', () => { + test('startSpan THROWS → onNavigate still fires exactly once', () => { + mockStartSpan.mockImplementation(() => { + callOrder.push('startSpan'); + throw new Error('sentry exploded before invoking the callback'); + }); + + const onNavigate = makeNavigateSpy(); + expect(() => pressCrisisButton(onNavigate)).not.toThrow(); + + expect(onNavigate).toHaveBeenCalledTimes(1); + }); + + test('startSpan returns WITHOUT invoking its callback → onNavigate still fires', () => { + // The realistic shape of a sampling/scope failure: no throw, just a no-op. + // This is the case a try/catch cannot possibly rescue. + mockStartSpan.mockImplementation(() => { + callOrder.push('startSpan'); + return undefined; + }); + + const onNavigate = makeNavigateSpy(); + pressCrisisButton(onNavigate); + + expect(onNavigate).toHaveBeenCalledTimes(1); + }); + + test('span.setAttribute THROWS → onNavigate still fires and nothing escapes', () => { + mockStartSpan.mockImplementation((_o: unknown, callback?: (span: unknown) => void) => { + callOrder.push('startSpan'); + return callback?.({ + setAttribute: () => { + throw new Error('setAttribute exploded'); + }, + }); + }); + + const onNavigate = makeNavigateSpy(); + expect(() => pressCrisisButton(onNavigate)).not.toThrow(); + expect(onNavigate).toHaveBeenCalledTimes(1); + }); + + test('span is undefined → onNavigate still fires and nothing escapes', () => { + mockStartSpan.mockImplementation((_o: unknown, callback?: (span: unknown) => void) => { + callOrder.push('startSpan'); + return callback?.(undefined); + }); + + const onNavigate = makeNavigateSpy(); + expect(() => pressCrisisButton(onNavigate)).not.toThrow(); + expect(onNavigate).toHaveBeenCalledTimes(1); + }); + + test('Sentry module entirely absent (startSpan undefined) → onNavigate still fires', () => { + mockStartSpan.mockImplementation(() => { + throw new TypeError('Sentry.startSpan is not a function'); + }); + + const onNavigate = makeNavigateSpy(); + expect(() => pressCrisisButton(onNavigate)).not.toThrow(); + expect(onNavigate).toHaveBeenCalledTimes(1); + }); +}); + +describe('INFRA-297 — ordering, not merely occurrence', () => { + test('onNavigate runs BEFORE any telemetry call', () => { + const onNavigate = makeNavigateSpy(); + pressCrisisButton(onNavigate); + + expect(callOrder).toContain('onNavigate'); + const navIdx = callOrder.indexOf('onNavigate'); + + // Every telemetry entry that fired must come strictly after the navigate. + // This is what a try/catch-around-the-span "fix" would fail. + const telemetry = ['startSpan', 'span.setAttribute', 'logSecurity', 'logPerformance']; + for (const entry of telemetry) { + const idx = callOrder.indexOf(entry); + if (idx === -1) continue; + expect(idx).toBeGreaterThan(navIdx); + } + }); + + test('onNavigate is the very first recorded call on the tap path', () => { + const onNavigate = makeNavigateSpy(); + pressCrisisButton(onNavigate); + + expect(callOrder[0]).toBe('onNavigate'); + }); + + test('a throwing telemetry path does not reorder or duplicate the navigate', () => { + mockStartSpan.mockImplementation(() => { + callOrder.push('startSpan'); + throw new Error('boom'); + }); + + const onNavigate = makeNavigateSpy(); + pressCrisisButton(onNavigate); + + expect(callOrder[0]).toBe('onNavigate'); + expect(onNavigate).toHaveBeenCalledTimes(1); + }); +}); diff --git a/app/src/features/crisis/components/CollapsibleCrisisButton.tsx b/app/src/features/crisis/components/CollapsibleCrisisButton.tsx index fa5f02d8..781d68ef 100644 --- a/app/src/features/crisis/components/CollapsibleCrisisButton.tsx +++ b/app/src/features/crisis/components/CollapsibleCrisisButton.tsx @@ -55,8 +55,11 @@ import Animated, { interpolate, } from 'react-native-reanimated'; import { MaterialDesignIcons } from '@react-native-vector-icons/material-design-icons'; -import * as Sentry from '@sentry/react-native'; -import { logSecurity, logPerformance, logCrisis } from '@/core/services/logging'; +import { logCrisis } from '@/core/services/logging'; +// Eager import, deliberately — the crisis path never lazy-imports (CLAUDE.md). +// Sentry itself is no longer imported here: all crisis-tap telemetry moved into +// crisisTapTrace so that no telemetry code can sit upstream of the dial. +import { beginCrisisTap } from '@/features/crisis/services/crisisTapTrace'; import { spacing, borderRadius, typography, colorSystem } from '@/core/theme'; /** Display mode for the crisis button */ @@ -184,31 +187,44 @@ export const CollapsibleCrisisButton: React.FC = ( * Direct tap works immediately, even in faded state */ const handleCrisisAction = useCallback(() => { - Sentry.startSpan( - { name: 'crisis_button_response', op: 'ui.crisis.tap' }, - (span) => { - const startTime = performance.now(); - - // Reset fade on interaction - resetFade(); - - // Navigate to CrisisResourcesScreen (provides choice: Call 988, Text 741741, Emergency contacts) - onNavigate(); - - // Performance monitoring for clinical safety - const responseTime = performance.now() - startTime; - span?.setAttribute('response_time_ms', responseTime); - span?.setAttribute('exceeded_budget', responseTime > 200); - if (responseTime > 200) { - logSecurity('Crisis button response time exceeded', 'high', { - responseTime, - threshold: 200 - }); - } else { - logPerformance('crisis_button_response', responseTime); - } - } - ); + // ORDERING IS THE SAFETY CONTRACT HERE (INFRA-297). Do not reorder. + // + // `onNavigate()` used to be called from INSIDE a `Sentry.startSpan` callback. + // `startSpan` does real work before it invokes that callback — async-context + // strategy dispatch, a scope fork, a sampling decision, span creation. If any + // of it throws, the callback never runs and the crisis tap produces NOTHING: + // no navigation, no dial, and no log, because the audit call was inside the + // same callback. On the CrisisErrorBoundary path that is the last-resort + // tel:988 dial in an already-crashed app — the one context where Sentry is + // itself a likely cause of the crash. + // + // A try/catch around the span does NOT fix that: it turns a visible crash + // into a silently swallowed tap, which is worse. So the navigate is moved + // out of the callback entirely, and telemetry is strictly downstream. + // Pinned by __tests__/safety/crisis-button-telemetry-ordering.test.tsx, + // which asserts ORDER, not just occurrence — occurrence alone would pass a + // try/catch non-fix. + + // Opens the tap→render measurement. Must precede the navigate (you cannot + // measure tap→render starting after the navigate). Safe to sit here only + // because it reads a clock, writes one field, schedules a timer, and is + // internally guarded so it cannot throw. Do not add anything to it. + beginCrisisTap('crisis_button'); + + // THE CRISIS ACTION. First, unconditional, synchronous, outside every + // telemetry construct. Navigates to CrisisResourcesScreen (choice of Call + // 988, Text 741741, emergency contacts) — or, on the error-boundary mount, + // dials 988 directly. + onNavigate(); + + // Cosmetic only, and therefore after. Nothing may run ahead of the crisis + // action for the sake of a fade animation. + resetFade(); + + // No telemetry here by design. The measurement closes at the point the user + // can actually act — CrisisResourcesScreen's commit, or the OS taking the + // dial — which also moves the span and log work off this tap frame entirely. + // Net effect: this path is now faster than before, not slower. }, [onNavigate, resetFade]); /** diff --git a/app/src/features/crisis/components/CrisisErrorBoundary.tsx b/app/src/features/crisis/components/CrisisErrorBoundary.tsx index c4a1b59e..514ab832 100644 --- a/app/src/features/crisis/components/CrisisErrorBoundary.tsx +++ b/app/src/features/crisis/components/CrisisErrorBoundary.tsx @@ -321,8 +321,23 @@ export class CrisisErrorBoundary extends Component< {/* Crisis support - always first priority */} 🚨 Crisis Support (24/7) + {/* + CORRECTED (INFRA-297). This previously read "Use the crisis button + at the top of your screen for immediate support," which was false + twice over in this branch: + 1. No crisis button was on screen. This boundary replaced its + children, and RootCrisisButton suppresses the root overlay on + 'AssessmentFlow' — the only route this boundary wraps. So the + copy pointed at a control that did not exist. + 2. The button is bottom-right, not "at the top". + RootCrisisButton's own header claims this boundary "keeps its OWN + CollapsibleCrisisButton … the last-resort 988 access when a render + crash removes this root-mounted overlay". That claim was only true + of the fallbackComponent branch, which no caller uses. The button + below makes it true for the branch that actually renders. + */} - Use the crisis button at the top of your screen for immediate support. + Tap the support button below to call or text a crisis line now. { + endCrisisTap('screen_commit'); + }, []); + // Track screen load performance useEffect(() => { const loadTime = performance.now() - startTime; diff --git a/app/src/features/crisis/services/crisisTapTrace.ts b/app/src/features/crisis/services/crisisTapTrace.ts new file mode 100644 index 00000000..80552fd3 --- /dev/null +++ b/app/src/features/crisis/services/crisisTapTrace.ts @@ -0,0 +1,198 @@ +/** + * crisisTapTrace — the real tap→render measurement for the crisis button (INFRA-297). + * + * WHAT THIS REPLACES + * ================== + * Before this, `CollapsibleCrisisButton` bracketed `onNavigate()` in a + * `Sentry.startSpan` callback and reported the elapsed time as the crisis-button + * response. That number measured navigation **dispatch** — `RootCrisisButton`'s + * `onNavigate` is a bare `navigationRef.navigate(...)` call, ~9ms per + * `performance-baselines.json`. It could never approach the 200ms budget, so any + * alert built on it was guaranteed never to fire: false assurance on a safety + * budget, which is worse than no metric. It was also the identical flaw the + * monitoring runbook already condemns in the jest proxy. + * + * This module measures from the tap to the point the user can actually act: + * either CrisisResources has committed to the screen, or the OS has taken the + * dial. Those are physically different quantities, so they carry distinct + * `outcome` labels and must never be aggregated into one statistic. + * + * HARD RULES (crisis specialist, non-negotiable) + * ============================================== + * `begin()` runs on the crisis tap path, ahead of the navigate. Therefore: + * - Synchronous, returns void, never async. No await, no Promise, no .then. + * - Zero I/O: no AsyncStorage, no SecureStore, no network, no Supabase. + * - Zero Sentry API. Sentry is strictly downstream of the dial — that + * ordering is the entire point of INFRA-297. + * - Zero logging, zero store writes, zero React state. + * - `performance.now()` only, NEVER `Date.now()` — wall clock is + * non-monotonic, and an NTP step would fabricate or hide a budget breach. + * - One module-scope slot, not a growing list. A list would leak on a path + * that can be tapped repeatedly. + * - Every public function is internally guarded: this module can never throw + * into its caller, because its caller is the crisis path. + * + * All emission (span attributes, logs) happens in `end()` or the watchdog, both + * of which run outside the tap's synchronous frame. Net effect: the tap path is + * FASTER than before, because the logging and span work left it. + */ + +import * as Sentry from '@sentry/react-native'; + +import { logSecurity, logPerformance } from '@/core/services/logging'; + +/** Where the crisis tap came from. Label only — never wellness data. */ +export type CrisisTapSource = 'crisis_button' | 'error_boundary'; + +/** + * How the tap terminated. `screen_commit` and `url_open` are different physical + * quantities (render vs. OS handoff) and are reported separately on purpose. + */ +export type CrisisTapOutcome = + | 'screen_commit' + | 'url_open' + | 'manual_fallback' + | 'deadline_exceeded'; + +/** The <200ms crisis-button budget from CLAUDE.md → Performance Budgets. */ +const BUDGET_MS = 200; + +/** + * How long a tap may stay open before we declare it dropped. Well above the + * budget, below user patience. Firing this is a SAFETY event, not a perf + * datapoint — see the watchdog body. + */ +const WATCHDOG_MS = 5000; + +interface OpenMark { + readonly source: CrisisTapSource; + readonly startedAt: number; + watchdog: NodeJS.Timeout | null; +} + +/** Single-flight: exactly one slot, never a list. */ +let openMark: OpenMark | null = null; + +function clearWatchdog(mark: OpenMark): void { + if (mark.watchdog !== null) { + clearTimeout(mark.watchdog); + mark.watchdog = null; + } +} + +/** + * Emit the measurement. Runs outside the tap frame. Individually guarded — a + * telemetry failure here must never surface to a caller on the crisis path. + */ +function emit(source: CrisisTapSource, outcome: CrisisTapOutcome, responseTime: number): void { + // Sentry first but fully isolated: if the SDK is broken this must not stop the + // audit record below, which is the part that actually matters. + try { + Sentry.startSpan({ name: 'crisis_button_response', op: 'ui.crisis.tap' }, (span) => { + span?.setAttribute('response_time_ms', responseTime); + span?.setAttribute('exceeded_budget', responseTime > BUDGET_MS); + span?.setAttribute('outcome', outcome); + span?.setAttribute('source', source); + }); + } catch { + // Telemetry is never allowed to affect the crisis path, including its audit + // trail. Swallowed deliberately; the audit record still fires below. + } + + try { + if (responseTime > BUDGET_MS) { + // Event string and payload shape preserved verbatim from the pre-INFRA-297 + // implementation so downstream log queries keep their history. + logSecurity('Crisis button response time exceeded', 'high', { + responseTime, + threshold: BUDGET_MS, + }); + } else { + logPerformance('crisis_button_response', responseTime); + } + } catch { + // As above. + } +} + +/** + * Open a mark. Called on the crisis tap, immediately before the navigate/dial. + * + * Deliberately placed BEFORE `onNavigate()` — a tap→render measurement cannot + * start after the navigate. That is safe only because this function does nothing + * but read a clock, write one field, and schedule a timer, and because it cannot + * throw. Do not add anything to it. + * + * A second `begin()` while one is open replaces it rather than double-counting: + * the newer tap is the one the user is waiting on. + */ +export function beginCrisisTap(source: CrisisTapSource): void { + try { + if (openMark) { + clearWatchdog(openMark); + } + + const mark: OpenMark = { + source, + startedAt: performance.now(), + watchdog: null, + }; + + mark.watchdog = setTimeout(() => { + // Neither a screen nor a dial materialised. That is a DROPPED CRISIS TAP — + // a false negative on a zero-false-negative path — so it is logged at + // 'high' severity, not as a performance datapoint. + // + // The known live producer is RootCrisisButton's `navigationRef.isReady()` + // guard: on an early tap it is false, nothing happens, and before this + // watchdog existed nothing recorded that the tap vanished. + const dropped = openMark; + openMark = null; + if (!dropped) return; + try { + logSecurity('Crisis button tap produced no screen or dial', 'high', { + responseTime: performance.now() - dropped.startedAt, + threshold: BUDGET_MS, + outcome: 'deadline_exceeded' satisfies CrisisTapOutcome, + source: dropped.source, + }); + } catch { + // Nothing further to do; must not throw out of a timer. + } + }, WATCHDOG_MS); + + openMark = mark; + } catch { + // Never throw onto the crisis path. + openMark = null; + } +} + +/** + * Close the open mark and emit. + * + * A no-op when nothing is open — silent, never a throw, never log spam. This + * matters because `openCrisisUrl` has several other callers (a "Call Now" tap + * inside CrisisResourcesScreen arrives after the commit already closed the + * mark), and they must be unaffected. + */ +export function endCrisisTap(outcome: CrisisTapOutcome): void { + try { + const mark = openMark; + if (!mark) return; + openMark = null; + clearWatchdog(mark); + emit(mark.source, outcome, performance.now() - mark.startedAt); + } catch { + // Never throw onto the crisis path. + } +} + +/** Test-only reset so single-flight state cannot leak between cases. */ +export function __resetCrisisTapTraceForTests(): void { + if (openMark) clearWatchdog(openMark); + openMark = null; +} + +export const CRISIS_TAP_BUDGET_MS = BUDGET_MS; +export const CRISIS_TAP_WATCHDOG_MS = WATCHDOG_MS; diff --git a/app/src/features/crisis/utils/openCrisisUrl.ts b/app/src/features/crisis/utils/openCrisisUrl.ts index 906cf01e..1a3be3f9 100644 --- a/app/src/features/crisis/utils/openCrisisUrl.ts +++ b/app/src/features/crisis/utils/openCrisisUrl.ts @@ -16,6 +16,9 @@ import { Alert, Linking } from 'react-native'; import { logError, LogCategory } from '@/core/services/logging'; +// Eager, per the crisis-path no-lazy-import rule. Every function in this module +// is internally guarded and cannot throw into the dial path. +import { endCrisisTap } from '@/features/crisis/services/crisisTapTrace'; export interface OpenCrisisUrlOptions { /** Human-readable target shown in the manual-fallback Alert, e.g. "988". */ @@ -40,6 +43,11 @@ export function openCrisisUrl( onTap?.(); const showManualFallback = (error: unknown): void => { + // Close the crisis-tap measurement with a distinct outcome (INFRA-297). This + // is a legitimate non-success terminal, NOT a dropped tap: the user still + // gets the manual-dial Alert below, so it must not be classified as the + // watchdog's 'deadline_exceeded'. No-op if no mark is open. + endCrisisTap('manual_fallback'); logError( LogCategory.CRISIS, 'Failed to open crisis URL', @@ -58,7 +66,22 @@ export function openCrisisUrl( return Linking.canOpenURL(url) .then((supported) => { if (supported) { - return Linking.openURL(url).then(() => undefined); + return Linking.openURL(url).then(() => { + // Success terminal for the crisis-tap measurement (INFRA-297): the OS + // has taken the dial. Needed because the CrisisErrorBoundary path dials + // without ever rendering CrisisResourcesScreen, so the screen-commit + // terminal would never fire for it. + // + // Tagged distinctly from 'screen_commit' on purpose — tap→OS-handoff + // and tap→render are different physical quantities and must never be + // aggregated into one p95. + // + // A no-op when no mark is open, which is the common case: a "Call Now" + // tap inside CrisisResourcesScreen arrives after the commit already + // closed the mark. openCrisisUrl's other callers are unaffected. + endCrisisTap('url_open'); + return undefined; + }); } throw new Error(`Crisis URL not supported: ${url}`); })