Skip to content

feat(scopes): add new scopes#2113

Open
Litarnus wants to merge 1 commit into
replace-hubs-with-scopesfrom
new-scopes-light
Open

feat(scopes): add new scopes#2113
Litarnus wants to merge 1 commit into
replace-hubs-with-scopesfrom
new-scopes-light

Conversation

@Litarnus

Copy link
Copy Markdown
Contributor

No description provided.

Base automatically changed from merge-master to 5.x June 15, 2026 12:25
@Litarnus Litarnus marked this pull request as ready for review June 22, 2026 14:38

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 15faea1. Configure here.

Comment thread src/State/Scope.php
$mergedScope->user = (clone $globalScope->user)->merge($isolationScope->user);
} elseif ($globalScope->user !== null) {
$mergedScope->user = clone $globalScope->user;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global user revives cleared isolation

Medium Severity

When the isolation scope has no user (including after removeUser()), mergeScopes still copies the global scope user onto the merged scope. Applying that scope can attach global user data to events that the isolation scope intentionally left anonymous.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 15faea1. Configure here.

Comment thread src/State/Scope.php
*/
public static function mergeScopes(self $globalScope, self $isolationScope): self
{
$mergedScope = clone $isolationScope;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The mergeScopes function performs a shallow clone of the scope, causing the span object to be shared between the original and merged scopes, which can lead to side effects.
Severity: LOW

Suggested Fix

Update the Scope::__clone() method to perform a deep clone of the $span property. This will ensure that merged scopes have their own independent span instance, preventing unintended side effects if the span is mutated after the merge operation. Additionally, add a test case to verify the behavior when the isolation scope has a span.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/State/Scope.php#L124

Potential issue: The `mergeScopes()` function creates a new scope by cloning an existing
`isolationScope`. However, the `Scope::__clone()` method performs a shallow copy and
does not clone the `span` object. As a result, the new `mergedScope` and the original
`isolationScope` share the same `span` instance. If the span is mutated in one scope,
the change will unexpectedly affect the other. While the current `applyToEvent()` usage
is read-only, any future code that mutates the span after merging could lead to
incorrect trace data. This potential issue is not covered by tests, as the relevant test
case only handles scenarios where the isolation scope has no span.

Did we get this right? 👍 / 👎 to inform future reviews.

@Litarnus Litarnus changed the base branch from 5.x to replace-hubs-with-scopes June 22, 2026 15:58
@Litarnus Litarnus force-pushed the new-scopes-light branch 2 times, most recently from 9477d70 to 1ca81df Compare June 22, 2026 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant