feat(core): Add refs field to Scope for non-serialized references - #23236
Open
mydea wants to merge 2 commits into
Open
feat(core): Add refs field to Scope for non-serialized references#23236mydea wants to merge 2 commits into
refs field to Scope for non-serialized references#23236mydea wants to merge 2 commits into
Conversation
Store the active span (core) and OpenTelemetry context (opentelemetry) in a shared, non-enumerable `refs` bag on the scope, and clone it in `clone()`. This makes the span/context references survive `scope.clone()` consistently across both the core and OpenTelemetry implementations. Previously the OTel context lived in a separate `_scopeContext` field that `clone()` did not copy, so `getActiveSpan(clonedScope)` returned `undefined` under OTel. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 58d67a2. Configure here.
|
|
||
| const clonedScope = scope.clone(); | ||
| expect(getContextFromScope(clonedScope)).toBe(context); | ||
| }); |
There was a problem hiding this comment.
Feat PR lacks integration test
Medium Severity
Flagging this because it was mentioned in the PR review rules. This is a feat PR, but the diff only adds a unit test for clone preserving context. There is no integration or E2E coverage of the OTel scope-clone path that originally dropped the active span/context.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 58d67a2. Configure here.
| setContextOnScope(scope, context); | ||
|
|
||
| // The _scopeContext property should not appear in Object.keys | ||
| expect(Object.keys(scope)).not.toContain('_scopeContext'); |
There was a problem hiding this comment.
Non-enumerable refs untested
Low Severity
Flagging this because it was mentioned in the PR review rules. The old test asserted that context storage was non-enumerable; it was replaced with a check that only re-reads the context. Nothing now verifies that refs stays out of Object.keys / structural comparisons, despite that being a stated design goal.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 58d67a2. Configure here.
Contributor
size-limit report 📦
|
mydea
marked this pull request as ready for review
August 10, 2026 13:52
mydea
requested review from
JPeer264,
Lms24 and
isaacs
and removed request for
a team
August 10, 2026 13:52
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.
Adds a
refsfield toScope— a shared bag for object references that are associated with a scope but must not be serialized (the active span in core, the OpenTelemetryContextin the opentelemetry package).refsis cloned as-is (shallow) inScope.clone().Root cause
The active span and the OTel context were stored in two separate, ad-hoc non-enumerable fields (
_sentrySpanand_scopeContext).Scope.clone()only knew about the core span field and explicitly re-set it on the clone; it had no knowledge of the opentelemetry-owned_scopeContext. As a result, cloning a scope under OTel dropped the context reference, sogetActiveSpan(clonedScope)— which reads the span out of the context attached to the scope — resolved toundefined. The two implementations disagreed on whether a clone preserved the active span.Consolidating both references into a single
refsobject thatclone()copies wholesale removes that asymmetry: a cloned scope now retains its span (core) and its context (OTel) uniformly.This also allows us to remove some span/scope specifics from the scope class to keep this more generic. the scope class will simply clone/reset refs, whatever they may be.
Notable decisions:
refsis stored as a non-enumerable property (viaaddNonEnumerableProperty), matching the previous behavior of_sentrySpan/_scopeContext. This keeps it out oftoEqual,Object.keys,toJSON, and event serialization — an enumerable field would otherwise leak the active span into structural comparisons and serialized payloads.clone()does a shallow spread ({ ...this.refs }), consistent with how the other reference-holding scope fields are copied.clear()resetsrefsto{}, replacing the previous per-field span reset.The existing
MaybeWeakRefwrapping is preserved for both the span and the context, so the GC/circular-reference protections are unchanged.