From ffccfe96319dc77587a74341170ec2503352dde8 Mon Sep 17 00:00:00 2001 From: Nelson Osacky Date: Wed, 2 Sep 2026 16:13:50 +0200 Subject: [PATCH 1/3] chore: Add Cursor Bugbot PR review guidelines Bugbot reviews PRs against generic Java advice unless the repo tells it what this SDK actually cares about. This adds .cursor/BUGBOT.md, the same location sentry-javascript, sentry-dart, and sentry-react-native use. The rules are drawn from this repo's own invariants: the narrow-catch rule and ExceptionUtils.rethrowIfFatal, binary compatibility and the IScope/IScopes implementation fan-out, opt-in-by-default options, cost added to the Android main-thread init path, span origin and op conventions, and monotonic-vs-wall clock selection. Formatting, PR title format, and changelog entries are listed as out of scope, since Spotless, validate-pr, and Danger already enforce them. Co-Authored-By: Claude Opus 5 --- .cursor/BUGBOT.md | 108 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 .cursor/BUGBOT.md diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md new file mode 100644 index 0000000000..ce59c4e579 --- /dev/null +++ b/.cursor/BUGBOT.md @@ -0,0 +1,108 @@ +# PR Review Guidelines for Cursor Bugbot + +You are reviewing a pull request for the Sentry Java/Android SDK. + +Read [`AGENTS.md`](../AGENTS.md) for build commands and contributing rules, and the matching +rule file in [`.cursor/rules/`](rules) for the area the diff touches (`api`, `options`, `scopes`, +`offline`, `opentelemetry`, ...). + +## Critical + +### Never crash or hang the host application + +- 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 the PR description must say why the broad catch is needed. +- Code probing for an optional `compileOnly` dependency must catch the specific `LinkageError` + subclass (`NoClassDefFoundError`, `NoSuchMethodError`, ...) only. +- 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 + [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(...)`. +- Flag resources acquired but not released: streams, files, `ExecutorService`s, `BroadcastReceiver`s, + lifecycle/activity callbacks, sensors, timers. Anything registered during init must be undone in + the integration's `close()`. + +### Security and privacy + +- Real secrets, tokens, or DSNs in code, logs, or configs. Obviously-fake DSNs in tests, samples, + and docs are expected — do not flag those. +- New code that collects user-identifiable data (headers, cookies, request/response bodies, URL + query strings, IPs, usernames, file paths, device identifiers) must be gated behind + `options.isSendDefaultPii()`, and must not be on by default otherwise. +- Debug flags, verbose logging, or sampling overrides accidentally left enabled in production + defaults. + +### Public API and compatibility + +- `.api` files are generated. Flag hand edits; the fix is `./gradlew apiDump`. +- New public API must be intentional: new internal classes/methods need `@ApiStatus.Internal`, new + unstable API needs `@ApiStatus.Experimental`. +- Removing or changing the signature of public API, or silently changing a default, sampling rate, + or feature toggle, without a deprecation and a `CHANGELOG.md`/`MIGRATION.md` note. +- New features must be **opt-in by default** via `SentryOptions` (or a namespaced options class). +- Adding a method to `IScope`/`IScopes` requires updating every implementation and stub — + `Scope`, `Scopes`, `CombinedScopeView`, `NoOpScope`, `NoOpScopes`, `ScopesAdapter`, `HubAdapter`. + Flag partial updates. +- New fields on `io.sentry.protocol` classes need both serialization and deserialization, plus a + round-trip test. +- Raising `minSdk`, the Java level, or a supported framework version without an explicit callout. + +## Java and Android specifics + +- The core `sentry` module is Java 8 and must not reference Android or JVM-only APIs. Reach optional + platform code through `Platform`, `LoadClass`, or a separate module. +- Android code calling an API newer than `minSdk` must be guarded by + `BuildInfoProvider.getSdkInfoVersion()`. +- `Sentry.init` can be called from any thread, and on Android it runs on the main thread during app + startup. Flag disk I/O, network calls, reflection, class loading, regex compilation, or eager + allocation newly added to an init path — and static mutable state that is not thread-safe. + +## Instrumentation conventions + +- Every started span must be finished on all paths, including error paths. +- Automatically instrumented spans set an origin (`SpanOptions.setOrigin`) and a standard + [span op](https://develop.sentry.dev/sdk/telemetry/traces/span-operations/). Origins must match + `[A-Za-z0-9_.]` — see the + [trace origin spec](https://develop.sentry.dev/sdk/telemetry/traces/trace-origin/). +- New integrations register themselves with `IntegrationUtils.addIntegrationToSdkVersion(...)`. +- Errors in instrumented user code should bubble up so the host app's handlers see them. Flag + instrumentation that swallows an error without recording it, and instrumentation that captures an + error that would also reach the global handlers (double reporting). + +## Concurrency +- The SDK uses raw java concurrency primitives. Ensure we are using them correctly. +- Ensure that atomic actions are atomic. +- Watch for possible deadlocks in general but especially when two locks are held and another thread can grab them in the opposite order. +- Prefer using existing executors over creating new threads. +- Do not block the main thread on Android with locking, synchronization or I/O calls. +- Watch for ordering issues when classes can be called from different threads. +- Flag a lock held across a callback into user code, an I/O call, or an `ExecutorService` submission. +- Mark a field `volatile` when it is written on one thread and read on another without a lock. A plain field read is a data race, not merely a stale value. +- Read mutable shared state once per operation. Re-reading the same field for several decisions in one pass lets it change mid-pass, so the results disagree with each other. +- Prefer the `synchronized` keyword. Existing code that uses `AutoClosableReentrantLock` is legacy. + +## Clocks +- Ensure we are using a monotonic clock to measure time intervals. +- Ensure we are using a wall clock for dates and timestamps. + +## Tests + +- Behavior changes need tests. A `fix` PR should include a regression test that fails without the + fix; if the diff doesn't make that clear, ask the author to confirm. +- Flag hollow tests: assertions that only prove "did not throw", or that assert on a payload without + checking the newly added data. +- New assertions should use Google Truth (`com.google.common.truth.Truth.assertThat`); `kotlin.test` + stays for structure (`@Test`, `assertFailsWith`). Don't flag existing `kotlin.test` assertions. +- Flag likely flakes: `Thread.sleep`, wall-clock or ordering assumptions, real network or filesystem + access, and shared static state left dirty between tests. + +## What NOT to flag + +- Formatting and import order — Spotless owns it. +- Contents of generated `.api` files, beyond confirming `apiDump` was run. +- Conventional commit / PR title format, and missing changelog entries — CI and Danger check both. +- Speculative refactors or improvements unrelated to the diff. From 4d32a671e341428d86e720840c986d995b824b5e Mon Sep 17 00:00:00 2001 From: Nelson Osacky Date: Wed, 2 Sep 2026 16:17:56 +0200 Subject: [PATCH 2/3] style: Wrap BUGBOT.md lines at 100 characters Re-wrap the bullets that ran long and add the missing blank line after the Concurrency and Clocks headings, so the file matches the wrapping used throughout. No wording changes. Co-Authored-By: Claude Opus 5 --- .cursor/BUGBOT.md | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md index ce59c4e579..4158151910 100644 --- a/.cursor/BUGBOT.md +++ b/.cursor/BUGBOT.md @@ -10,10 +10,13 @@ rule file in [`.cursor/rules/`](rules) for the area the diff touches (`api`, `op ### Never crash or hang the host application -- 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 the PR description must say why the broad catch is needed. +- 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 the PR description must say why the broad catch is + needed. - Code probing for an optional `compileOnly` dependency must catch the specific `LinkageError` subclass (`NoClassDefFoundError`, `NoSuchMethodError`, ...) only. - The SDK must never `captureException`/`captureMessage` for its own failures or for exceptions @@ -22,9 +25,9 @@ rule file in [`.cursor/rules/`](rules) for the area the diff touches (`api`, `op [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(...)`. -- Flag resources acquired but not released: streams, files, `ExecutorService`s, `BroadcastReceiver`s, - lifecycle/activity callbacks, sensors, timers. Anything registered during init must be undone in - the integration's `close()`. +- Flag resources acquired but not released: streams, files, `ExecutorService`s, + `BroadcastReceiver`s, lifecycle/activity callbacks, sensors, timers. Anything registered during + init must be undone in the integration's `close()`. ### Security and privacy @@ -74,18 +77,24 @@ rule file in [`.cursor/rules/`](rules) for the area the diff touches (`api`, `op error that would also reach the global handlers (double reporting). ## Concurrency + - The SDK uses raw java concurrency primitives. Ensure we are using them correctly. - Ensure that atomic actions are atomic. -- Watch for possible deadlocks in general but especially when two locks are held and another thread can grab them in the opposite order. +- Watch for possible deadlocks in general but especially when two locks are held and another thread + can grab them in the opposite order. - Prefer using existing executors over creating new threads. - Do not block the main thread on Android with locking, synchronization or I/O calls. - Watch for ordering issues when classes can be called from different threads. -- Flag a lock held across a callback into user code, an I/O call, or an `ExecutorService` submission. -- Mark a field `volatile` when it is written on one thread and read on another without a lock. A plain field read is a data race, not merely a stale value. -- Read mutable shared state once per operation. Re-reading the same field for several decisions in one pass lets it change mid-pass, so the results disagree with each other. +- Flag a lock held across a callback into user code, an I/O call, or an `ExecutorService` + submission. +- Mark a field `volatile` when it is written on one thread and read on another without a lock. A + plain field read is a data race, not merely a stale value. +- Read mutable shared state once per operation. Re-reading the same field for several decisions in + one pass lets it change mid-pass, so the results disagree with each other. - Prefer the `synchronized` keyword. Existing code that uses `AutoClosableReentrantLock` is legacy. ## Clocks + - Ensure we are using a monotonic clock to measure time intervals. - Ensure we are using a wall clock for dates and timestamps. From a624361d000d4232728b941a5a3e5acc88a96c7f Mon Sep 17 00:00:00 2001 From: Nelson Osacky Date: Thu, 3 Sep 2026 16:21:22 +0200 Subject: [PATCH 3/3] chore: Expand Bugbot PR review guidelines Add checks for reflection keep rules, SAGP bytecode manipulation, threading models on new classes, clock arithmetic misuse, dependency bump intent, and contract-focused tests. Require broad catches to justify themselves in a code comment rather than the PR description, so the reasoning stays with the code. Narrow the test requirement to customer-facing behavior, and drop the `.api` and IScope/IScopes rules now covered elsewhere. Co-Authored-By: Claude Opus 5 (1M context) --- .cursor/BUGBOT.md | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md index 4158151910..8882291ee4 100644 --- a/.cursor/BUGBOT.md +++ b/.cursor/BUGBOT.md @@ -15,7 +15,7 @@ rule file in [`.cursor/rules/`](rules) for the area the diff touches (`api`, `op 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 the PR description must say why the broad catch is + `ExceptionUtils.rethrowIfFatal(t)` first and a code comment must say why the broad catch is needed. - Code probing for an optional `compileOnly` dependency must catch the specific `LinkageError` subclass (`NoClassDefFoundError`, `NoSuchMethodError`, ...) only. @@ -28,6 +28,9 @@ rule file in [`.cursor/rules/`](rules) for the area the diff touches (`api`, `op - Flag resources acquired but not released: streams, files, `ExecutorService`s, `BroadcastReceiver`s, lifecycle/activity callbacks, sensors, timers. Anything registered during init must be undone in the integration's `close()`. +- Errors in instrumented user code should bubble up so the host app's handlers see them. Flag + instrumentation that swallows an error without recording it, and instrumentation that captures an + error that would also reach the global handlers (double reporting). ### Security and privacy @@ -41,18 +44,17 @@ rule file in [`.cursor/rules/`](rules) for the area the diff touches (`api`, `op ### Public API and compatibility -- `.api` files are generated. Flag hand edits; the fix is `./gradlew apiDump`. -- New public API must be intentional: new internal classes/methods need `@ApiStatus.Internal`, new - unstable API needs `@ApiStatus.Experimental`. +- New public API must be intentional: new classes/methods not for public use need + `@ApiStatus.Internal`, new unstable API needs `@ApiStatus.Experimental`. - Removing or changing the signature of public API, or silently changing a default, sampling rate, or feature toggle, without a deprecation and a `CHANGELOG.md`/`MIGRATION.md` note. - New features must be **opt-in by default** via `SentryOptions` (or a namespaced options class). -- Adding a method to `IScope`/`IScopes` requires updating every implementation and stub — - `Scope`, `Scopes`, `CombinedScopeView`, `NoOpScope`, `NoOpScopes`, `ScopesAdapter`, `HubAdapter`. - Flag partial updates. + If a feature is added without this, ask "are you sure" as a PR comment. - New fields on `io.sentry.protocol` classes need both serialization and deserialization, plus a round-trip test. - Raising `minSdk`, the Java level, or a supported framework version without an explicit callout. +- Ensure dependency bumps are intentional. For example if a dependency is bumped in part of a + matrix that isn't the newest version. ## Java and Android specifics @@ -63,6 +65,7 @@ rule file in [`.cursor/rules/`](rules) for the area the diff touches (`api`, `op - `Sentry.init` can be called from any thread, and on Android it runs on the main thread during app startup. Flag disk I/O, network calls, reflection, class loading, regex compilation, or eager allocation newly added to an init path — and static mutable state that is not thread-safe. +- Ensure any new reflection calls are mirrored in the proguard keep rules. ## Instrumentation conventions @@ -72,9 +75,9 @@ rule file in [`.cursor/rules/`](rules) for the area the diff touches (`api`, `op `[A-Za-z0-9_.]` — see the [trace origin spec](https://develop.sentry.dev/sdk/telemetry/traces/trace-origin/). - New integrations register themselves with `IntegrationUtils.addIntegrationToSdkVersion(...)`. -- Errors in instrumented user code should bubble up so the host app's handlers see them. Flag - instrumentation that swallows an error without recording it, and instrumentation that captures an - error that would also reach the global handlers (double reporting). +- If we're adding a feature that requires bytecode manipulation from the + sentry-android-gradle-plugin, make sure the code is properly commented as such to ensure it isn't + accidentally changed in the future. ## Concurrency @@ -92,16 +95,21 @@ rule file in [`.cursor/rules/`](rules) for the area the diff touches (`api`, `op - Read mutable shared state once per operation. Re-reading the same field for several decisions in one pass lets it change mid-pass, so the results disagree with each other. - Prefer the `synchronized` keyword. Existing code that uses `AutoClosableReentrantLock` is legacy. +- New classes have a clear and defined threading and concurrency model as part of the javadoc if + needed. ## Clocks - Ensure we are using a monotonic clock to measure time intervals. - Ensure we are using a wall clock for dates and timestamps. +- Ensure that time manipulations are not being misused e.g. adding or subtracting wall clocks to + get a duration. ## Tests -- Behavior changes need tests. A `fix` PR should include a regression test that fails without the - fix; if the diff doesn't make that clear, ask the author to confirm. +- Public behavior (customer facing) changes need tests. A `fix` PR should include a regression test + that fails without the fix; if the diff doesn't make that clear, ask the author to confirm. +- Prefer tests against contracts. Avoid testing implementation details. - Flag hollow tests: assertions that only prove "did not throw", or that assert on a payload without checking the newly added data. - New assertions should use Google Truth (`com.google.common.truth.Truth.assertThat`); `kotlin.test`