[DO NOT MERGE] feat(core): add opt-in assertion instrumentation - #6592
Draft
antonis wants to merge 11 commits into
Draft
[DO NOT MERGE] feat(core): add opt-in assertion instrumentation#6592antonis wants to merge 11 commits into
antonis wants to merge 11 commits into
Conversation
Add an opt-in Metro/Babel transform that rewrites assertion call sites (invariant, assert, warning, console.assert) so a violated assertion reports a non-fatal Sentry event instead of being stripped from release bundles or crashing with a minified message. Adds the captureInvariantViolation runtime API and fixes a related gap in the RN global error handler so an already-captured, re-thrown error is not reported a second time. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📲 Install BuildsAndroid
|
Contributor
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ecf47a2+dirty | 3835.20 ms | 1222.43 ms | -2612.77 ms |
| 3817909+dirty | 1183.90 ms | 1187.50 ms | 3.60 ms |
| 5ee78d6+dirty | 3847.52 ms | 1224.27 ms | -2623.25 ms |
| 890d145+dirty | 1223.59 ms | 1231.37 ms | 7.78 ms |
| e763471+dirty | 3841.16 ms | 1218.06 ms | -2623.09 ms |
| 3d31fcf+dirty | 3838.09 ms | 1223.46 ms | -2614.63 ms |
| 9210ae6+dirty | 3815.93 ms | 1214.14 ms | -2601.79 ms |
| 26843eb+dirty | 3861.60 ms | 1238.77 ms | -2622.82 ms |
| 7a89652+dirty | 3861.46 ms | 1229.61 ms | -2631.85 ms |
| 9ad4522+dirty | 3850.40 ms | 1222.40 ms | -2628.00 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ecf47a2+dirty | 4.98 MiB | 6.46 MiB | 1.49 MiB |
| 3817909+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| 5ee78d6+dirty | 5.15 MiB | 6.69 MiB | 1.53 MiB |
| 890d145+dirty | 3.38 MiB | 4.77 MiB | 1.38 MiB |
| e763471+dirty | 4.98 MiB | 6.51 MiB | 1.53 MiB |
| 3d31fcf+dirty | 4.98 MiB | 6.56 MiB | 1.58 MiB |
| 9210ae6+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 26843eb+dirty | 4.98 MiB | 6.53 MiB | 1.55 MiB |
| 7a89652+dirty | 5.15 MiB | 6.70 MiB | 1.55 MiB |
| 9ad4522+dirty | 4.98 MiB | 6.56 MiB | 1.58 MiB |
Contributor
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ad66da3+dirty | 3855.02 ms | 1213.43 ms | -2641.59 ms |
| 7d8c8bd+dirty | 3847.98 ms | 1230.77 ms | -2617.21 ms |
| 4acd4e9+dirty | 3806.73 ms | 1208.48 ms | -2598.25 ms |
| c2e182c+dirty | 3841.76 ms | 1220.76 ms | -2621.01 ms |
| 3d377b5+dirty | 1201.55 ms | 1201.80 ms | 0.25 ms |
| c004dae+dirty | 3857.82 ms | 1224.87 ms | -2632.95 ms |
| a858ac3+dirty | 3850.72 ms | 1226.35 ms | -2624.37 ms |
| 4e0b819+dirty | 3828.96 ms | 1205.64 ms | -2623.32 ms |
| 5c1e987+dirty | 1208.43 ms | 1220.72 ms | 12.29 ms |
| 882f8ae+dirty | 3842.51 ms | 1230.40 ms | -2612.11 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ad66da3+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| 7d8c8bd+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 4acd4e9+dirty | 4.98 MiB | 6.53 MiB | 1.55 MiB |
| c2e182c+dirty | 4.98 MiB | 6.51 MiB | 1.53 MiB |
| 3d377b5+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| c004dae+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| a858ac3+dirty | 4.98 MiB | 6.51 MiB | 1.53 MiB |
| 4e0b819+dirty | 4.98 MiB | 6.46 MiB | 1.49 MiB |
| 5c1e987+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| 882f8ae+dirty | 5.15 MiB | 6.70 MiB | 1.54 MiB |
Rename the public API from invariant/loudInvariants to an assertions vocabulary: `loudInvariants` config → `captureAssertions`, `captureInvariantViolation` → `captureAssertionViolation`, `InvariantViolationOptions` → `AssertionViolationOptions`, `DEFAULT_INVARIANT_MECHANISM` → `DEFAULT_ASSERTION_MECHANISM`. Report a uniform `mechanism.type = 'assertion'` for every violation and move the specific pragma to `mechanism.data.pragma`. Add a plugin↔runtime coupling test asserting the injected `CAPTURE_FN` matches the exported reporter name so a one-sided rename fails CI instead of breaking silently. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
antonis
changed the title
Add a CHANGELOG caveat that hard preconditions (invariant/assert) still throw after reporting — the transform yields a readable event, not crash suppression. Update the RN global handler comment to the assertions vocabulary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
Address review findings on the assertion instrumentation:
- assertion.ts: stringify values defensively so a Symbol (String(symbol)
throws) or a value with a throwing toString/Symbol.toPrimitive can't crash
the reporting path; fall back to [unstringifiable <type>].
- sentryAssertionBabelPlugin.ts: capture only identifiers whose binding is
visible from the call-site scope, so a nested arrow/callback param no longer
emits an out-of-scope reference (ReferenceError on the report path).
- sentryAssertionBabelPlugin.ts: normalize path separators before matching so
the SDK self-exclusion and the includeNodeModules allowlist work on Windows.
- Document that aliased/destructured pragmas (e.g. const { assert } = console)
are intentionally not matched.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Guard the reporter's `values` handling against `null` and throwing getters so the no-throw reporting path holds for hand-written calls. - Support `util.format`-style message interpolation via a new `messageArgs` option; the Babel plugin forwards variadic pragma args (e.g. RN's `invariant(dims, 'No dimension set for key %s', key)`). - Construct the call-site `Error` through a hoisted global-`Error` alias so a lexical `Error` shadow can't break the injected `new Error()`. - Normalize path separators in the transformer's node_modules gate so the `includeNodeModules` allowlist matches on Windows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Guard the `%d`/`%i`/`%f` coercions in the reporter's message formatter so a non-coercible arg (e.g. a Symbol) renders as `NaN` instead of throwing on the no-throw reporting path. - Drop `let`/`const` identifiers declared textually after the call site from the captured `values`, so a short-circuited condition can't read them in their temporal dead zone and throw a ReferenceError on the report path. - Only rewrite standalone assertion statements; a pragma used as a subexpression (e.g. `warning(cond) && next()`) is left untouched to preserve its semantics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…o__ keys, and @sentry-internal Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment on lines
+225
to
+237
| for (const key of Object.keys(values)) { | ||
| try { | ||
| const value = values[key]; | ||
| data[`values.${key}`] = typeof value === 'boolean' ? value : truncate(stringifyValue(value), MAX_VALUE_LENGTH); | ||
| } catch (_e) { | ||
| // A throwing getter must not break the no-throw reporting path. | ||
| data[`values.${key}`] = '[unreadable]'; | ||
| } | ||
| } | ||
| try { | ||
| data.values = truncate(JSON.stringify(values) ?? 'undefined', MAX_SNAPSHOT_LENGTH); | ||
| } catch (_e) { | ||
| // Circular or non-serializable values — the flattened entries above still apply. |
Contributor
There was a problem hiding this comment.
Unbounded values iteration in flattenValues allows DoS via large objects
flattenValues iterates over every key in values without a count cap and calls JSON.stringify(values) on the full object before truncation, allowing a large input object to exhaust CPU and memory.
Evidence
flattenValuescallsObject.keys(values)and loops over every key, creating onedataentry per key with no limit on the total number of entries.- It then calls
JSON.stringify(values)on the entire object before passing the result totruncate, so a large object is fully serialized first. - Individual values and snapshots are length-capped (
MAX_VALUE_LENGTHandMAX_SNAPSHOT_LENGTH), but the key count is unbounded, contradicting the function's JSDoc claim that a large object "can't bloat the event payload". - Because
captureAssertionViolationis a public API, a caller can pass an arbitrarily largevaluesobject (e.g. a config map or array with millions of indices), causing unbounded CPU and memory consumption on the no-throw reporting path.
Identified by Warden · find-bugs, code-review · CTS-DF4
Contributor
Author
There was a problem hiding this comment.
Fixed. flattenValues now caps the entry count at MAX_VALUE_ENTRIES (50) and builds the JSON snapshot from only that capped subset rather than JSON.stringify-ing the full input, so a large object handed to the public API no longer produces unbounded entries or forces a full serialization on the no-throw path. Over-cap keys are summarized in a values.__truncated__ marker, and the JSDoc is updated to describe all three bounds (count, per-entry length, snapshot length). Added a test with a 1000-key object (162eefb → 4ed8f2d).
…ptions Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| f9c1ed4+dirty | 431.00 ms | 466.22 ms | 35.22 ms |
| c2e182c+dirty | 471.64 ms | 553.59 ms | 81.95 ms |
| 41d6254+dirty | 424.45 ms | 474.34 ms | 49.89 ms |
| af33f3b+dirty | 434.90 ms | 506.14 ms | 71.24 ms |
| 882f8ae+dirty | 399.98 ms | 427.06 ms | 27.08 ms |
| 0b5120f+dirty | 503.22 ms | 538.60 ms | 35.38 ms |
| 774257e+dirty | 407.09 ms | 447.46 ms | 40.37 ms |
| bf168a4+dirty | 418.21 ms | 489.74 ms | 71.53 ms |
| 5569641+dirty | 406.43 ms | 428.51 ms | 22.08 ms |
| c151573+dirty | 530.34 ms | 559.43 ms | 29.09 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| f9c1ed4+dirty | 49.74 MiB | 54.86 MiB | 5.12 MiB |
| c2e182c+dirty | 49.74 MiB | 54.85 MiB | 5.11 MiB |
| 41d6254+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| af33f3b+dirty | 49.74 MiB | 55.09 MiB | 5.35 MiB |
| 882f8ae+dirty | 48.30 MiB | 53.60 MiB | 5.29 MiB |
| 0b5120f+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| 774257e+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| bf168a4+dirty | 49.74 MiB | 55.09 MiB | 5.35 MiB |
| 5569641+dirty | 48.30 MiB | 53.48 MiB | 5.18 MiB |
| c151573+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
Contributor
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7436d0f+dirty | 429.58 ms | 452.52 ms | 22.94 ms |
| 1122a96+dirty | 510.16 ms | 542.00 ms | 31.84 ms |
| 26843eb+dirty | 442.02 ms | 506.29 ms | 64.27 ms |
| 88735e9+dirty | 427.04 ms | 487.37 ms | 60.33 ms |
| 882f8ae+dirty | 408.19 ms | 435.86 ms | 27.67 ms |
| 1a2e7e0+dirty | 451.98 ms | 501.50 ms | 49.52 ms |
| 68672fc+dirty | 407.55 ms | 442.96 ms | 35.41 ms |
| 4966363+dirty | 415.67 ms | 448.60 ms | 32.93 ms |
| 038a6d7+dirty | 499.02 ms | 527.68 ms | 28.66 ms |
| 6176a94+dirty | 403.58 ms | 446.73 ms | 43.15 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7436d0f+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| 1122a96+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
| 26843eb+dirty | 49.74 MiB | 55.26 MiB | 5.52 MiB |
| 88735e9+dirty | 49.74 MiB | 54.82 MiB | 5.07 MiB |
| 882f8ae+dirty | 48.30 MiB | 53.60 MiB | 5.29 MiB |
| 1a2e7e0+dirty | 49.74 MiB | 54.82 MiB | 5.07 MiB |
| 68672fc+dirty | 48.30 MiB | 53.61 MiB | 5.31 MiB |
| 4966363+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
| 038a6d7+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| 6176a94+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
- Guard the error.message backfill so a frozen/read-only/non-object error can't throw on the no-throw reporting path. - Match node_modules as a real path segment (not a raw substring) via a shared isInNodeModules() helper, so first-party files like node_modules_helper.ts are still instrumented. - Build siteId from a root-relative path with a parentDir/basename fallback, so files sharing a basename no longer collide in the runtime dedup set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
PII note — Unchecking the "No new PII / gated by This is a deliberate call for the experimental/hackweek landing — capturing the failing value is the core point of the feature, and gating it off-by-default would neuter it. If this graduates to an actual feature, revisit before release: likely "primitives always, full object/array snapshots behind |
antonis
changed the title
Comment on lines
+169
to
+172
| if (normalizedFilename.includes('node_modules')) { | ||
| if (!inc) { | ||
| return undefined; | ||
| } |
Contributor
There was a problem hiding this comment.
First-party files with node_modules in path incorrectly skipped from assertion instrumentation
The includes('node_modules') substring check over-matches and skips first-party files like src/utils/node_modules_helper.ts, while the plugin itself correctly instruments them. Use the same path-segment regex the plugin uses.
Evidence
sentryAssertionBabelPlugin.ts:396uses/(?:^|\/)node_modules\//inisInNodeModules, explicitly documented to allow first-party files likesrc/utils/node_modules_helper.ts.addSentryCaptureAssertionsPluginusesnormalizedFilename.includes('node_modules')which matches any substring, not just a path segment.- A first-party file named
node_modules_helper.tsis skipped by the transformer guard but would be instrumented by the plugin, so the early return removes valid instrumentation. - The plugin's own test (
sentryAssertionBabelPlugin.test.ts:252-254) asserts thatnode_modules_helper.tsshould be instrumented.
Also found at 1 additional location
packages/core/src/js/tools/sentryAssertionBabelPlugin.ts:231-237
Identified by Warden · code-review · EPW-EC8
Comment on lines
+209
to
+215
| * honors the flag — React Native's ErrorUtils global handler skip it instead of | ||
| * reporting the re-thrown error a second time as an unhandled crash. Set on | ||
| * *every* rethrow path (including the dedup-suppressed branch, which never | ||
| * reaches the tail) so the guard can never be bypassed. | ||
| */ | ||
| function rethrowCaptured(error: Error): never { | ||
| addNonEnumerableProperty(error as unknown as Record<string, unknown>, '__sentry_captured__', true); |
Contributor
There was a problem hiding this comment.
rethrowCaptured can throw wrong error for frozen errors, bypassing dedup guard
rethrowCaptured lacks a try-catch around addNonEnumerableProperty, so a frozen or sealed error causes a TypeError instead of the original error and skips the __sentry_captured__ tag. If the dedup path also reaches rethrowCaptured for a frozen error, the same conclusion applies.
Evidence
rethrowCaptureddoesaddNonEnumerableProperty(error, '__sentry_captured__', true)thenthrow errorwith no surrounding try-catch.addNonEnumerablePropertyusesObject.defineProperty, which throwsTypeErrorwhen adding a new property to a frozen object.- The comment above
rethrowCapturedexplicitly states the tag is set on every rethrow path so the guard can never be bypassed, yet it can be. setupErrorUtilsGlobalHandlerinreactnativeerrorhandlers.ts(also changed in this PR) checkserror?.__sentry_captured__to avoid reporting the re-thrown error a second time; if the tag was never set, the violation is reported again as an unhandled crash.
Identified by Warden · find-bugs · T8D-QC8
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📢 Type of change
📜 Description
Adds an opt-in Metro/Babel transform that rewrites assertion call sites (
invariant,assert,warning,console.assert) so a violated assertion reports a non-fatal Sentry event instead of being stripped from release bundles or crashing with a minified message.captureAssertionsin the Sentry Metro config; off by default. First-party code is instrumented by default; dependencies are opt-in through anincludeNodeModulesallowlist.invariant/assert) still throw after reporting;warning/console.assertare report-only.captureAssertionViolation()(also usable by hand).mechanism.type = 'assertion'; the specific pragma is recorded undermechanism.data.pragma. Grouping keys by pragma + call site, so flavors stay separable without fragmenting the mechanism type.💡 Motivation and Context
Assertions are usually stripped from release builds or, when they do fire, surface as unreadable minified crashes. This surfaces them as grouped, non-fatal Sentry events with the condition, runtime values, and a stack anchored at the assertion site — without changing app source.
The public surface uses an "assertions" vocabulary (
captureAssertions,captureAssertionViolation,AssertionViolationOptions) rather than "invariant" so the naming isn't tied to a single pragma.💚 How did you test it?
Unit tests for the reporter, the Babel plugin, the transformer wiring, and the RN error handler — including a plugin↔runtime coupling test that fails CI if the injected helper name and the exported reporter name drift apart. Manually verified on the RN sample (iOS):
invariantfrom a dependency, first-partyconsole.assert, and a hand-written call each produced the expected handled events.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
If this graduates from a hackweek experiment to a real feature, revisit before release:
valuesPII (primitives always; object/array snapshots behindsendDefaultPII).