|
| 1 | +# PR Review Guidelines for Cursor Bugbot |
| 2 | + |
| 3 | +You are reviewing a pull request for the Sentry Java/Android SDK. |
| 4 | + |
| 5 | +Read [`AGENTS.md`](../AGENTS.md) for build commands and contributing rules, and the matching |
| 6 | +rule file in [`.cursor/rules/`](rules) for the area the diff touches (`api`, `options`, `scopes`, |
| 7 | +`offline`, `opentelemetry`, ...). |
| 8 | + |
| 9 | +## Critical |
| 10 | + |
| 11 | +### Never crash or hang the host application |
| 12 | + |
| 13 | +- While we don't want to crash or hang the host application, we also don't want to leave the host |
| 14 | + application in a bad or unrecoverable state. Therefore catch the narrowest type the guarded code |
| 15 | + can throw. |
| 16 | +- Existing broad catches like `catch (Throwable)` are legacy, not precedent. Where a broad catch is |
| 17 | + genuinely unavoidable (an entry point running user code or third-party callbacks), it must call |
| 18 | + `ExceptionUtils.rethrowIfFatal(t)` first and a code comment must say why the broad catch is |
| 19 | + needed. |
| 20 | +- Code probing for an optional `compileOnly` dependency must catch the specific `LinkageError` |
| 21 | + subclass (`NoClassDefFoundError`, `NoSuchMethodError`, ...) only. |
| 22 | +- The SDK must never `captureException`/`captureMessage` for its own failures or for exceptions |
| 23 | + thrown inside user callbacks (`beforeSend`, `beforeBreadcrumb`, `tracesSampler`, ...). Log via |
| 24 | + `options.getLogger()` instead — capturing here loops. See |
| 25 | + [Never capture your own exceptions](https://develop.sentry.dev/sdk/getting-started/principles/#never-capture-your-own-exceptions). |
| 26 | +- Flag `System.out`/`System.err`, `printStackTrace()`, and `android.util.Log` in SDK source; use |
| 27 | + `options.getLogger().log(...)`. |
| 28 | +- Flag resources acquired but not released: streams, files, `ExecutorService`s, |
| 29 | + `BroadcastReceiver`s, lifecycle/activity callbacks, sensors, timers. Anything registered during |
| 30 | + init must be undone in the integration's `close()`. |
| 31 | +- Errors in instrumented user code should bubble up so the host app's handlers see them. Flag |
| 32 | + instrumentation that swallows an error without recording it, and instrumentation that captures an |
| 33 | + error that would also reach the global handlers (double reporting). |
| 34 | + |
| 35 | +### Security and privacy |
| 36 | + |
| 37 | +- Real secrets, tokens, or DSNs in code, logs, or configs. Obviously-fake DSNs in tests, samples, |
| 38 | + and docs are expected — do not flag those. |
| 39 | +- New code that collects user-identifiable data (headers, cookies, request/response bodies, URL |
| 40 | + query strings, IPs, usernames, file paths, device identifiers) must be gated behind |
| 41 | + `options.isSendDefaultPii()`, and must not be on by default otherwise. |
| 42 | +- Debug flags, verbose logging, or sampling overrides accidentally left enabled in production |
| 43 | + defaults. |
| 44 | + |
| 45 | +### Public API and compatibility |
| 46 | + |
| 47 | +- New public API must be intentional: new classes/methods not for public use need |
| 48 | + `@ApiStatus.Internal`, new unstable API needs `@ApiStatus.Experimental`. |
| 49 | +- Removing or changing the signature of public API, or silently changing a default, sampling rate, |
| 50 | + or feature toggle, without a deprecation and a `CHANGELOG.md`/`MIGRATION.md` note. |
| 51 | +- New features must be **opt-in by default** via `SentryOptions` (or a namespaced options class). |
| 52 | + If a feature is added without this, ask "are you sure" as a PR comment. |
| 53 | +- New fields on `io.sentry.protocol` classes need both serialization and deserialization, plus a |
| 54 | + round-trip test. |
| 55 | +- Raising `minSdk`, the Java level, or a supported framework version without an explicit callout. |
| 56 | +- Ensure dependency bumps are intentional. For example if a dependency is bumped in part of a |
| 57 | + matrix that isn't the newest version. |
| 58 | + |
| 59 | +## Java and Android specifics |
| 60 | + |
| 61 | +- The core `sentry` module is Java 8 and must not reference Android or JVM-only APIs. Reach optional |
| 62 | + platform code through `Platform`, `LoadClass`, or a separate module. |
| 63 | +- Android code calling an API newer than `minSdk` must be guarded by |
| 64 | + `BuildInfoProvider.getSdkInfoVersion()`. |
| 65 | +- `Sentry.init` can be called from any thread, and on Android it runs on the main thread during app |
| 66 | + startup. Flag disk I/O, network calls, reflection, class loading, regex compilation, or eager |
| 67 | + allocation newly added to an init path — and static mutable state that is not thread-safe. |
| 68 | +- Ensure any new reflection calls are mirrored in the proguard keep rules. |
| 69 | + |
| 70 | +## Instrumentation conventions |
| 71 | + |
| 72 | +- Every started span must be finished on all paths, including error paths. |
| 73 | +- Automatically instrumented spans set an origin (`SpanOptions.setOrigin`) and a standard |
| 74 | + [span op](https://develop.sentry.dev/sdk/telemetry/traces/span-operations/). Origins must match |
| 75 | + `[A-Za-z0-9_.]` — see the |
| 76 | + [trace origin spec](https://develop.sentry.dev/sdk/telemetry/traces/trace-origin/). |
| 77 | +- New integrations register themselves with `IntegrationUtils.addIntegrationToSdkVersion(...)`. |
| 78 | +- If we're adding a feature that requires bytecode manipulation from the |
| 79 | + sentry-android-gradle-plugin, make sure the code is properly commented as such to ensure it isn't |
| 80 | + accidentally changed in the future. |
| 81 | + |
| 82 | +## Concurrency |
| 83 | + |
| 84 | +- The SDK uses raw java concurrency primitives. Ensure we are using them correctly. |
| 85 | +- Ensure that atomic actions are atomic. |
| 86 | +- Watch for possible deadlocks in general but especially when two locks are held and another thread |
| 87 | + can grab them in the opposite order. |
| 88 | +- Prefer using existing executors over creating new threads. |
| 89 | +- Do not block the main thread on Android with locking, synchronization or I/O calls. |
| 90 | +- Watch for ordering issues when classes can be called from different threads. |
| 91 | +- Flag a lock held across a callback into user code, an I/O call, or an `ExecutorService` |
| 92 | + submission. |
| 93 | +- Mark a field `volatile` when it is written on one thread and read on another without a lock. A |
| 94 | + plain field read is a data race, not merely a stale value. |
| 95 | +- Read mutable shared state once per operation. Re-reading the same field for several decisions in |
| 96 | + one pass lets it change mid-pass, so the results disagree with each other. |
| 97 | +- Prefer the `synchronized` keyword. Existing code that uses `AutoClosableReentrantLock` is legacy. |
| 98 | +- New classes have a clear and defined threading and concurrency model as part of the javadoc if |
| 99 | + needed. |
| 100 | + |
| 101 | +## Clocks |
| 102 | + |
| 103 | +- Ensure we are using a monotonic clock to measure time intervals. |
| 104 | +- Ensure we are using a wall clock for dates and timestamps. |
| 105 | +- Ensure that time manipulations are not being misused e.g. adding or subtracting wall clocks to |
| 106 | + get a duration. |
| 107 | + |
| 108 | +## Tests |
| 109 | + |
| 110 | +- Public behavior (customer facing) changes need tests. A `fix` PR should include a regression test |
| 111 | + that fails without the fix; if the diff doesn't make that clear, ask the author to confirm. |
| 112 | +- Prefer tests against contracts. Avoid testing implementation details. |
| 113 | +- Flag hollow tests: assertions that only prove "did not throw", or that assert on a payload without |
| 114 | + checking the newly added data. |
| 115 | +- New assertions should use Google Truth (`com.google.common.truth.Truth.assertThat`); `kotlin.test` |
| 116 | + stays for structure (`@Test`, `assertFailsWith`). Don't flag existing `kotlin.test` assertions. |
| 117 | +- Flag likely flakes: `Thread.sleep`, wall-clock or ordering assumptions, real network or filesystem |
| 118 | + access, and shared static state left dirty between tests. |
| 119 | + |
| 120 | +## What NOT to flag |
| 121 | + |
| 122 | +- Formatting and import order — Spotless owns it. |
| 123 | +- Contents of generated `.api` files, beyond confirming `apiDump` was run. |
| 124 | +- Conventional commit / PR title format, and missing changelog entries — CI and Danger check both. |
| 125 | +- Speculative refactors or improvements unrelated to the diff. |
0 commit comments