From bd2d6ae212812faf024d0947eb59fcb9b520d2e8 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 23 Sep 2026 05:07:20 -0400 Subject: [PATCH] Record what already reports before a catch removes it An uncaught throw in the extension already reaches Sentry, because `Sentry.init` never sets `defaultIntegrations: false` and the browser SDK merges `globalHandlersIntegration` in. So a catch that only logs deletes the sole report rather than softening a crash. `tsc-blindspots` keeps its own passage and gains a pointer. Its description is trimmed to 1024 characters, which the repo lints and which #158's sweep missed because #167 landed three days earlier. --- .../knowledge/error-reporting-paths.md | 46 +++++++++++++++++++ .../repos/metamask-extension.md | 8 ++++ .../typescript/skills/tsc-blindspots/skill.md | 34 ++++++-------- 3 files changed, 69 insertions(+), 19 deletions(-) create mode 100644 domains/observability/knowledge/error-reporting-paths.md diff --git a/domains/observability/knowledge/error-reporting-paths.md b/domains/observability/knowledge/error-reporting-paths.md new file mode 100644 index 00000000..232cd7c2 --- /dev/null +++ b/domains/observability/knowledge/error-reporting-paths.md @@ -0,0 +1,46 @@ +--- +domain: observability +title: Error reporting paths +--- + +# Error Reporting Paths + +What reaches Sentry when something throws, and what a `catch` does to that. + +Read at `metamask-extension` [`a5fc11920`](https://github.com/MetaMask/metamask-extension/commit/a5fc11920). + +## An uncaught throw already reports + +`Sentry.init` in [`app/scripts/lib/setupSentry.ts`](https://github.com/MetaMask/metamask-extension/blob/a5fc11920/app/scripts/lib/setupSentry.ts#L199) passes an `integrations` array and never sets `defaultIntegrations: false`. A grep for `defaultIntegrations` across `app/` and `shared/` returns nothing, so the browser SDK merges its own defaults alongside the listed ones. + +That default list includes `globalHandlersIntegration()`, which hooks `onerror` and `onunhandledrejection`. So an uncaught throw and an unhandled rejection both produce a Sentry event with no code at the throw site doing anything. + +## Therefore the two states are reported and silent + +Adding `.catch(console.debug)` to a path that previously threw does not degrade a feature gracefully. It deletes the only report. The pair is not crash versus degrade, it is reported versus silent, and the second state has no signal anywhere. + +The shape that keeps both is a catch that reports: + +```ts +import { captureException } from '../../../shared/lib/sentry'; + +somethingAsync().catch((error) => { + captureException(error); +}); +``` + +`captureException` is exported at [`shared/lib/sentry.ts:18`](https://github.com/MetaMask/metamask-extension/blob/a5fc11920/shared/lib/sentry.ts#L18) and is already the house value-import at `background.js`, `metamask-controller.js`, `offscreen.ts` and the migrations. + +## A reported-but-swallowed path is still quiet + +Reporting is not the same as being noticed. A change inside `try { … } catch { captureException(e); return; }` degrades a feature without crashing: nothing goes red, no test fails, and the only signal is an error-tracker entry nobody is watching. The same edit in a hot path is caught in minutes. + +Weight findings by observability rather than by likelihood alone. **An unlikely failure in a swallowed path can outrank a likely one in a loud path.** + +## Not established + +Whether the MV3 service worker's global scope hooks these handlers the way the window scope does. `globalHandlersIntegration` is written against `window` events, and the worker has no `window`. Settling it needs a runtime check in the worker context rather than a reading of the SDK source, and until then the reported-versus-silent claim above is verified for page contexts only. + +## Related + +- `typescript` domain `skills/tsc-blindspots/skill.md`, "Silent failure modes deserve their own pass" — the same weighting applied to reviewing a diff. diff --git a/domains/observability/skills/instrumentation/repos/metamask-extension.md b/domains/observability/skills/instrumentation/repos/metamask-extension.md index 33d06bc3..8e544b69 100644 --- a/domains/observability/skills/instrumentation/repos/metamask-extension.md +++ b/domains/observability/skills/instrumentation/repos/metamask-extension.md @@ -66,3 +66,11 @@ grep -rn "excludeMetaMetricsId: true" app/ ui/ shared/ --include="*.ts" --includ - Slack: `#metamask-metametrics` - Team: `@consensys/data-council` + +## Swallowing an Error Deletes Its Report + +`Sentry.init` here never sets `defaultIntegrations: false`, so the browser SDK merges `globalHandlersIntegration()` in and an uncaught throw or unhandled rejection already reports on its own. + +A `.catch(console.debug)` added to such a path is not graceful degradation. The two states are reported and silent, and the shape that keeps both is a catch calling `captureException` from `shared/lib/sentry`. + +Mechanism, sources and the one unestablished case: `knowledge/error-reporting-paths.md`. diff --git a/domains/typescript/skills/tsc-blindspots/skill.md b/domains/typescript/skills/tsc-blindspots/skill.md index 75395b8a..619d9ca3 100644 --- a/domains/typescript/skills/tsc-blindspots/skill.md +++ b/domains/typescript/skills/tsc-blindspots/skill.md @@ -1,25 +1,19 @@ --- name: tsc-blindspots description: >- - Find the type defects `tsc` is structurally unable to report — a green build is - not evidence the types are correct. Covers the two classes: (1) hand-written - types that restate an authoritative source and disagree with it, caught by - substituting the derived type at a fixed commit and diffing `tsc` output; and - (2) the standing blind spots in the language and config — unchecked array/record - indexing, bivariant method parameters, covariant arrays, `any` absorption at - untyped boundaries, precise signatures fed `any` at every call site, ambient - `declare module` assertions that launder an `any` into a confident type, - excess-property checks that only fire on fresh literals, and external data - asserted rather than validated. Also audits typing edits that quietly change - runtime behavior: - stripped `| undefined`, deleted default parameters, literals swapped for runtime - enum lookups, calls made optional so a throw becomes a silent no-op. Use when - reviewing a JS→TS migration, a PR that hand-writes types for values that already - have them, a "rename-only" refactor, or any PR claiming a change is mechanical. - Triggers on /mms-tsc-blindspots, or on phrases like "validate this TypeScript - migration", "is this type right", "does this type match the real shape", - "why didn't CI catch this type", "derive vs define", and "what can tsc not - check". + Find the type defects `tsc` is structurally unable to report: a green build is + not evidence the types are correct. Two classes. Hand-written types that restate + an authoritative source and disagree with it, caught by substituting the derived + type at a fixed commit and diffing `tsc` output. And the standing blind spots in + the language and config: unchecked indexing, bivariant method parameters, + covariant arrays, `any` absorption at untyped boundaries, ambient `declare + module` assertions that launder an `any`, excess-property checks that fire only + on fresh literals, and external data asserted rather than validated. Also audits + typing edits that quietly change runtime behavior: stripped `| undefined`, + deleted defaults, a call made optional so a throw becomes a silent no-op. Use + when reviewing a JS to TS migration, or any PR claiming a change is mechanical. + Triggers on /mms-tsc-blindspots, "is this type right", "derive vs define", + "what can tsc not check". maturity: experimental --- @@ -267,6 +261,8 @@ entry nobody is watching. The same edit in a hot path would be caught in minutes Weight findings by observability, not just by likelihood: **an unlikely failure in a swallowed path can outrank a likely one in a loud path.** +On `metamask-extension` the prior question is whether the path reported at all: an uncaught throw already does, so a `catch` that only logs removes the report rather than softening a crash. See observability domain `knowledge/error-reporting-paths.md`, delivered beside `instrumentation`. + ## Why the build stays green regardless - The hand-written type **compiles by construction** — that is why it was written.