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