From 8f4bf155f6d71c1be73e3c5f6574f91987e3f5d0 Mon Sep 17 00:00:00 2001 From: Nelson Osacky Date: Tue, 29 Sep 2026 14:22:45 +0200 Subject: [PATCH] docs: Allow broad catches only at SDK boundaries Inside the SDK, catch the narrowest type. Broad catches are allowed only at boundaries where the thrown types are unknowable, must call rethrowIfFatal first, and must document what happens on failure. Align BUGBOT.md with AGENTS.md so Bugbot enforces the same policy. Co-Authored-By: Claude Opus 5.5 (1M context) --- .cursor/BUGBOT.md | 25 +++++++++++++++++------- AGENTS.md | 49 ++++++++++++++++++++++++----------------------- 2 files changed, 43 insertions(+), 31 deletions(-) diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md index 8882291ee46..b410220beb4 100644 --- a/.cursor/BUGBOT.md +++ b/.cursor/BUGBOT.md @@ -13,15 +13,26 @@ rule file in [`.cursor/rules/`](rules) for the area the diff touches (`api`, `op - While we don't want to crash or hang the host application, we also don't want to leave the host application in a bad or unrecoverable state. Therefore catch the narrowest type the guarded code can throw. -- Existing broad catches like `catch (Throwable)` are legacy, not precedent. Where a broad catch is - genuinely unavoidable (an entry point running user code or third-party callbacks), it must call - `ExceptionUtils.rethrowIfFatal(t)` first and a code comment must say why the broad catch is - needed. +- Flag any new broad catch (`Throwable`, `Error`, `Exception`, `RuntimeException`) outside a + boundary where we can't know what the other side throws, e.g. public API entry points like + `captureException`, or Android framework calls that cross into another process + (`ContentResolver`, system services). Existing broad catches are legacy, not precedent. See the + Exception Handling section in `AGENTS.md`. +- A broad catch at a boundary must call `ExceptionUtils.rethrowIfFatal(t)` first and requires a + code comment saying why the catch is broad and what happens on failure (event dropped, feature + disabled, ...). +- A broad catch that wraps user code requires documentation, both in code and in sentry-docs, + explaining how the user is expected to discover that the code they wrote isn't working. + sentry-docs is a separate repository, so ask the author to link the docs PR. - Code probing for an optional `compileOnly` dependency must catch the specific `LinkageError` - subclass (`NoClassDefFoundError`, `NoSuchMethodError`, ...) only. + subclass (`NoClassDefFoundError`, `NoSuchMethodError`, ...). Flag a probe guarded only by a broad + catch with `rethrowIfFatal`: it rethrows every `LinkageError`, crashing the host app when the + dependency is missing. +- Logging is not handling: `options.getLogger()` is silent unless `debug` is enabled. When a catch + only logs, ask the author what the customer sees when it fires. - The SDK must never `captureException`/`captureMessage` for its own failures or for exceptions - thrown inside user callbacks (`beforeSend`, `beforeBreadcrumb`, `tracesSampler`, ...). Log via - `options.getLogger()` instead — capturing here loops. See + thrown inside user callbacks (`beforeSend`, `beforeBreadcrumb`, `tracesSampler`, ...) — capturing + here loops. See [Never capture your own exceptions](https://develop.sentry.dev/sdk/getting-started/principles/#never-capture-your-own-exceptions). - Flag `System.out`/`System.err`, `printStackTrace()`, and `android.util.Log` in SDK source; use `options.getLogger().log(...)`. diff --git a/AGENTS.md b/AGENTS.md index 1027e513c4b..8bb5fa82644 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -130,30 +130,31 @@ The repository is organized into multiple modules: ### Exception Handling -**Never introduce a new `catch (Throwable)`.** Catch the narrowest type the guarded code can -actually throw. The repository still contains many pre-existing broad catches; they are legacy, -not a precedent to follow. - -A broad catch swallows `OutOfMemoryError`, `StackOverflowError`, `ThreadDeath` and `LinkageError` — -conditions the JVM/ART cannot recover from and that leave the process in an undefined state — and -it hides real bugs in our own code behind a log line. - -"The SDK must never crash the host application" is not a reason to catch `Throwable`. That goal is -served by `io.sentry.util.ExceptionUtils.rethrowIfFatal`, which lets the non-recoverable throwables -through while leaving everything else for the caller to log or ignore: - -```java -try { - doSomethingRisky(); -} catch (Throwable t) { - ExceptionUtils.rethrowIfFatal(t); - options.getLogger().log(SentryLevel.ERROR, "Failed to do something risky", t); -} -``` - -Apply that pattern only where a broad catch is genuinely unavoidable — an entry point that runs -arbitrary user code or third-party callbacks. Everywhere else, name the exception types. Say in the -PR description why the broad catch is necessary. +Inside the SDK, **never add a broad catch** (`Throwable`, `Error`, `Exception`, +`RuntimeException`). Catch the narrowest type the guarded code can actually throw. The many +existing broad catches are legacy, not precedent: they hide real bugs in our own code and make it +appear that Sentry is broken. + +A broad catch is allowed only at a boundary where we can't know what the other side throws. Two +examples: + +- public API entry points like `captureException` +- Android framework calls that cross into another process (`ContentResolver`, system services) + +At a boundary: + +- Call `ExceptionUtils.rethrowIfFatal(t)` first so `OutOfMemoryError`, `StackOverflowError`, etc. + are never swallowed. It also rethrows `LinkageError`, so code probing an optional `compileOnly` + dependency must catch the specific subclass (`NoClassDefFoundError`, ...) before it. +- A code comment is required saying why the catch is broad and what happens on failure (event + dropped, feature disabled, ...). +- If wrapping user code with a broad catch, documentation is **required** explaining how the user + is expected to discover that the code they wrote isn't working. Both in code and in sentry-docs. +- Logging is not handling: `options.getLogger()` is silent unless `debug` is enabled, so a failure + that is only logged is invisible in production. + +Instrumentation that wraps user code is not a boundary for the user's own exceptions: let them +propagate unchanged. ### Testing Requirements - Write comprehensive unit tests for new features