Skip to content

docs(core): backfill shared Bugbot review rules #6639

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
sentry-junior wants to merge 1 commit into
base: main
Choose a base branch
from
+39 −6
Draft

docs(core): backfill shared Bugbot review rules #6639

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 39 additions & 6 deletions .cursor/BUGBOT.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@
- High-level review guidance for the entire Sentry React Native SDK monorepo.
- Optimize for **signal over noise**: only comment when there's material correctness, security/privacy, performance, or API-quality impact.
- If you find anything to flag, mention that you flagged this in the review because it was mentioned in this rules file.
- Do not flag the issues below if they appear only in tests.
- Do not flag the issues below if they appear only in tests (unless covered by Testing conventions below).
- These rules operationalize the Sentry SDK [philosophy](https://develop.sentry.dev/sdk/getting-started/philosophy/) and [principles](https://develop.sentry.dev/sdk/getting-started/principles/): protect customer apps and data, prefer safe defaults, keep the base SDK lean, stay compatible, and never let SDK or callback failures become Sentry traffic or host crashes.

**Reviewer style**

- Be concise. Quote exact lines/spans and propose a minimal fix (tiny diff/code block).
- If something is subjective, ask a brief question rather than asserting.
- Prefer principles over nitpicks; avoid noisy style-only comments that don't impact behavior.
- Limit to high-confidence findings; no drive-by refactors unrelated to the diff.

---

Expand All @@ -32,6 +34,7 @@
- **Breaking changes**: Signature/behavior changes, renamed/removed symbols, altered nullability/defaults, or event/telemetry shape changes **without** deprecation/migration notes.
- **Behavioral compatibility**: Silent changes to defaults, sampling, or feature toggles that affect existing apps.
- **Native bridge compatibility**: Changes to native module method signatures (iOS `RCT_EXPORT_METHOD` / Android `@ReactMethod`) must be backward-compatible or versioned, as they affect all consumers including Expo and bare React Native apps.
- **Support floor drops**: Raising min RN/iOS/Android/JS engine versions, or dropping a supported platform, without explicit docs/changelog/migration callout.

### C. Dependency Updates

Expand All @@ -41,6 +44,7 @@
- Flag breaking API changes, deprecated features being removed, new requirements, or behavioral changes that could affect existing integrations.
- Check if version bumps require corresponding changes in the native bridge code (Objective-C/Swift on iOS, Java/Kotlin on Android).
- **JavaScript dependency updates**: For PRs updating JS/TS dependencies, check for breaking API changes that affect the SDK's public surface or internal usage.
- **New baseline dependencies**: Flag new runtime dependencies on the base SDK path. Integration-only optional deps are fine when justified; baseline deps increase license, maintenance, and supply-chain surface.

---

Expand All @@ -54,9 +58,10 @@

**Correctness & safety**

- Add/update tests with behavioral changes and bug fixes.
- Handle error paths explicitly; never let a Sentry instrumentation error crash the host app.
- Add/update tests with behavioral changes and bug fixes. Tests must prove user-visible or SDK behavior (payloads, contracts), not merely coverage or "did not throw".
- Handle error paths explicitly; never let a Sentry instrumentation error crash the host app. Prefer graceful degrade / no-op when a path cannot safely run.
- Avoid global mutable state; prefer immutability and clear ownership.
- Do not capture exceptions thrown inside the SDK itself or inside user callbacks (`beforeSend`, `tracesSampler`, and similar). Swallow gracefully and emit an error-level SDK log — capturing here can loop. See [Never capture your own exceptions](https://develop.sentry.dev/sdk/getting-started/principles/#never-capture-your-own-exceptions).

**DRY & cohesion**

Expand Down Expand Up @@ -125,8 +130,11 @@

- Any span started must be **closed** (including on error paths).
- For _automated_ instrumented spans, always set:
- `sentry.origin`
- `sentry.op` using a standard operation where applicable (see [Sentry's list of standard ops](https://develop.sentry.dev/sdk/telemetry/traces/span-operations/)).
- `sentry.origin` — only `[a-zA-Z0-9_.]`; flag non-conforming values ([trace origin](https://develop.sentry.dev/sdk/telemetry/traces/trace-origin/))
- `sentry.op` — lowercase snake_case segments joined by `.` where applicable ([span ops](https://develop.sentry.dev/sdk/telemetry/traces/span-operations/))
- If attribute values are known at span start, set them via start-span `attributes` rather than immediate follow-up `setAttribute` calls, so samplers / ignore rules see full context.
- When instrumentation catches errors: prefer letting user errors propagate. Flag swallowing without capture, and capture that would double-report an error still bubbling to the app.
- When calling `captureException` (or equivalent) for user/app errors, set mechanism `handled` and a stable `type` identifying the integration/site.

**Structured logs**

Expand All @@ -142,6 +150,29 @@
- Any new UI instrumentation must respect the masking/unmasking API.
- Default to masking sensitive views; opt-in to unmasking.

**Defaults & OOTB**

- Prefer auto-enabling integrations when safe; flag new required config that could reasonably default.
- Avoid heavy in-SDK transforms of wire-format data when rawer collection + server-side processing would do.

---

## 5) Testing conventions

- `feat` PRs: prefer at least one integration or E2E test covering the new behavior.
- `fix` PRs: prefer a regression test that fails without the fix and passes with it; if unclear from the diff, ask the author.
- Flag likely flakes: sleep/timeouts instead of signals; start-wait-after-act instead of register-wait-then-act; multi-request waits that assume a hard order.

---

## 6) What NOT to flag

- Pure style/formatting owned by linters (`yarn lint` / platform formatters)
- Speculative refactors or cleanup with no clear user benefit or linked motivation
- Idiomatic native bridges / low-level hooks solely for being "non-pure" — only flag when unsafe or host-harmful
- Test-only issues outside Testing conventions
- Conventional commit format when CI already validates it

---

## Quick reviewer checklist
Expand All @@ -153,5 +184,7 @@
- [ ] TurboModule/New Architecture spec updated if native module interface changed.
- [ ] Spans started are always closed; automated spans/logs include `sentry.origin` (+ valid `sentry.op` for spans).
- [ ] Dangerous init paths guarded; app remains usable on failure.
- [ ] No SDK-own / callback exception capture (no self-capture loops).
- [ ] `captureException` sets mechanism; instrumentation does not swallow or double-report.
- [ ] `NativeModules.RNSentry` existence checked before use; async bridge calls wrapped in try/catch.
- [ ] Tests/docs/CHANGELOG updated for behavior changes.
- [ ] Tests prove behavior (feat/fix bar); docs/CHANGELOG updated for behavior changes.
Loading