feat(ecosystem): Implement cross-system issue synchronization#4
feat(ecosystem): Implement cross-system issue synchronization#4Frawatson wants to merge 1 commit into
Conversation
![grapple-pr[bot]](https://avatars.githubusercontent.com/in/3205222?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR implements a cycle-prevention mechanism for cross-system issue synchronization by introducing an AssignmentSource dataclass that tracks which integration triggered an assignment, allowing the sync logic to skip re-propagating changes back to the originating integration.
Changes: 7 files (+212 / -15 lines)
- 🟢 2 files added:
src/sentry/integrations/services/assignment_source.py,tests/sentry/integrations/services/test_assignment_source.py - 🟡 5 files modified:
src/sentry/integrations/mixins/issues.py,src/sentry/integrations/tasks/sync_assignee_outbound.py,src/sentry/integrations/utils/sync.py,src/sentry/models/groupassignee.py,tests/sentry/models/test_groupassignee.py
Resolves: #7 (GitHub OAuth Security Enhancement)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 7 files"/]
f1["🟡 issues.py +18/-3"]
pr_0 --> f1
f2["🟢 assignment_source.py +35/-0"]
pr_0 --> f2
f3["🟡 sync_assignee_outbound.py +16/-3"]
pr_0 --> f3
f4["🟡 sync.py +25/-4"]
pr_0 --> f4
f5["🟡 groupassignee.py +9/-2"]
pr_0 --> f5
f6["🟢 test_assignment_source.py +38/-0"]
pr_0 --> f6
f7["🟡 test_groupassignee.py +71/-3"]
pr_0 --> f7
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
2 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 2 |
| 🟠 Major | 4 |
| 🟡 Minor | 5 |
Review details
- Reviewed at: 2026-04-07T18:57:55.155Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 74.9s
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🔴 CRITICAL — Dataclass Default Value (confidence: 100%)
queued: datetime = timezone.now() is evaluated once at class definition time, not per-instance. Every AssignmentSource instance that doesn't explicitly provide queued will share the same datetime value — the time the module was first imported. This is a well-known Python dataclass pitfall explicitly called out in the intent specification's edge cases.
Evidence:
- The intent specification explicitly warns: 'The
queuedfield usestimezone.now()as a default value at class definition time (dataclass field default), not at instantiation time' - Python dataclass defaults are evaluated once at class definition, not per-instance creation
- The
queuedfield should usefield(default_factory=timezone.now)to get a fresh timestamp per instance - Since the class is
frozen=True, the stale default cannot even be corrected after construction
Agent: logic
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🔴 CRITICAL — Serialization Round-Trip (confidence: 100%)
The queued field is a datetime object. When to_dict() is called, asdict() preserves it as a datetime. This dict is then passed as a Celery task kwarg and serialized to JSON (Celery's default serializer). JSON serialization will either fail or convert the datetime to a string. On deserialization in from_dict(), the queued value will be a string, not a datetime, causing TypeError (frozen dataclass with type mismatch) or creating an AssignmentSource with a string queued field. If it causes a TypeError, from_dict returns None, silently disabling cycle prevention.
Evidence:
- The intent specification warns: 'the
datetimefieldqueuedmay not survive JSON serialization/deserialization intact, causingfrom_dictto returnNoneand silently disabling cycle prevention' asdict()on line 28 will includequeuedas adatetimeobject- Celery's default JSON serializer will convert datetime to ISO string or fail
from_dicton line 32 callscls(**input_dict)— ifqueuedis a string, the dataclass will accept it (no runtime type checking), creating an invalid instance; or if Celery serialization fails entirely, the task will error- No datetime serialization/deserialization logic exists in
to_dict/from_dict
Agent: logic
| @dataclass(frozen=True) | ||
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int |
There was a problem hiding this comment.
🟠 MAJOR — Cryptographic issues (confidence: 100%)
The queued field uses timezone.now() as a dataclass field default, which is evaluated once at class definition time, not per-instance. Every AssignmentSource instance that does not explicitly pass queued will share the same stale timestamp from when the module was first imported. This means the queued field is meaningless for its intended purpose of recording when an assignment was queued.
Evidence:
queued: datetime = timezone.now()— Python evaluates default expressions in dataclass field definitions at class-body execution time, so all instances share the same datetime value.- The correct pattern is
queued: datetime = field(default_factory=timezone.now), which callstimezone.now()at each instantiation. - The PR's own edge-case notes flag this explicitly: 'The
queuedfield usestimezone.now()as a default value at class definition time (dataclass field default), not at instantiation time'.
Agent: security
| @classmethod | ||
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) |
There was a problem hiding this comment.
🟠 MAJOR — Data exposure (confidence: 100%)
AssignmentSource.from_dict silently returns None on any error, including deserialization failures caused by the queued field surviving as a datetime object through the first serialization but arriving as an ISO-format string after JSON round-trip through Celery/Redis. When from_dict returns None, parsed_assignment_source is None, should_sync receives no source, and the cycle-prevention logic is completely bypassed — potentially triggering infinite sync loops.
Evidence:
- In
sync_assignee_outbound.pyline ~53,parsed_assignment_sourceisNonewhenfrom_dictreturnsNone, andinstallation.should_sync('outbound_assignee', None)has no cycle-prevention. asdict()on adataclasswith adatetimefield produces a plain Pythondatetimeobject in the dict. After JSON serialization (Celery task kwargs go through JSON), thedatetimebecomes a string. On the receiving side,AssignmentSource(**input_dict)passes a string into a field typed asdatetime, which the frozen dataclass accepts without validation — but if any stricter validation is added later, or if future code relies on it being adatetime, this silently breaks.- There is no logging when
from_dictreturnsNone, making this failure mode invisible in production. - The PR's risk areas explicitly call out: '
AssignmentSource.from_dictsilently returnsNoneon any error, meaning any serialization issue will cause cycle-prevention to be skipped without any log or alert'.
Agent: security
| @classmethod | ||
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) |
There was a problem hiding this comment.
🟠 MAJOR — Input validation (confidence: 100%)
If assignment_source_dict is passed to sync_assignee_outbound containing an unknown extra key (e.g., from a future schema version or a forward-compatible producer), cls(**input_dict) raises TypeError (unexpected keyword argument), from_dict catches it and returns None, silently disabling cycle prevention. This means a schema mismatch between producer and consumer versions causes a security regression (sync cycles re-enabled) with no observable error.
Evidence:
return cls(**input_dict)— Python dataclasses raiseTypeErrorfor unexpected keyword arguments.except (ValueError, TypeError): return None— the TypeError from an extra key is silently swallowed.- A forward-deployed worker (new producer) writing
assignment_source_dictwith an additional field will cause older workers to silently drop cycle prevention. - The PR's edge cases note: 'If
assignment_source_dictis passed tosync_assignee_outboundwith an unknown extra key (e.g., from a future version),from_dictwill returnNonedue toTypeError, silently dropping the source and allowing a sync cycle'.
Agent: security
| "organizations:integrations-issue-sync", group.organization, actor=acting_user | ||
| ): | ||
| sync_group_assignee_outbound(group, assigned_to.id, assign=True) | ||
| sync_group_assignee_outbound( |
There was a problem hiding this comment.
🟡 MINOR — Authentication/Authorization (confidence: 100%)
The assignment_source parameter is forwarded to sync_group_assignee_outbound only in the assignee_type == 'user' branch. The team assignment path does not forward assignment_source, meaning inbound team assignments from an integration will not have cycle prevention applied for the team path. While team assignment sync may be less common, this inconsistency leaves a partial cycle-prevention gap.
Evidence:
- In
groupassignee.py, thesync_group_assignee_outboundcall at line ~190 is insideif assignee_type == 'user'. The team assignee path (if it also calls outbound sync) does not receiveassignment_source. - The PR's edge cases note: 'Team assignments in
GroupAssignee.assign— theassignment_sourceis only forwarded forassignee_type == "user"sync; team assignment path should be checked for consistency'.
Agent: security
| ): | ||
| sync_group_assignee_outbound(group, assigned_to.id, assign=True) | ||
| sync_group_assignee_outbound( | ||
| group, assigned_to.id, assign=True, assignment_source=assignment_source |
There was a problem hiding this comment.
🟡 MINOR — Team Assignment Consistency (confidence: 98%)
The assignment_source is only forwarded to sync_group_assignee_outbound when assignee_type == 'user' (inside the user-type feature flag check). When a team is assigned, the outbound sync path is not reached, so assignment_source is effectively ignored for team assignments. While this may be intentional (team sync may not be supported), it means team-based inbound syncs could potentially trigger outbound cycles if team sync is ever enabled.
Evidence:
- The intent specification calls this out: 'Team assignments in
GroupAssignee.assign— theassignment_sourceis only forwarded forassignee_type == 'user'sync; team assignment path should be checked for consistency' - Line 189 checks
assignee_type == 'user'before calling sync_group_assignee_outbound - The
assignment_sourceparameter is accepted but silently unused for team assignments
Agent: logic
| if assignment_source | ||
| else None, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🟡 MINOR — Input validation (confidence: 93%)
In sync_group_assignee_outbound, assignment_source.to_dict() serializes the queued datetime field as a Python datetime object into the Celery task kwargs dict. Celery serializes task kwargs to JSON by default. A datetime object is not JSON-serializable, which will cause a TypeError at task dispatch time, crashing the outbound sync entirely when an assignment_source is provided.
Evidence:
assignment_source.to_dict()callsdataclasses.asdict(), which returns a dict with adatetimevalue for thequeuedfield.- Celery's default serializer is JSON; passing a
datetimein task kwargs will raiseTypeError: Object of type datetime is not JSON serializableatapply_asynctime. - This would cause all inbound-triggered outbound syncs (the main code path this PR adds) to fail with an exception, while manual assignments (no
assignment_source) continue to work. - The PR's risk areas flag: 'the
datetimefieldqueuedmay not survive JSON serialization/deserialization intact'.
Agent: security
| **kwargs, | ||
| ): | ||
| """ | ||
| Propagate a sentry issue's status to a linked issue's status. |
There was a problem hiding this comment.
🟡 MINOR — Inconsistent Cycle Prevention (confidence: 93%)
The sync_status_outbound abstract method signature now includes assignment_source parameter, but the should_sync cycle-prevention logic is only applied in the outbound assignee path. If sync_status_outbound is called with an assignment_source, there is no corresponding cycle check for status sync. The assignment_source parameter on sync_status_outbound appears unused and potentially misleading.
Evidence:
- The intent specification notes: '
sync_status_outboundabstract method signature now includesassignment_sourcebut the cycle-prevention logic inshould_syncis only applied in the outbound assignee path' - Adding the parameter to the abstract method signature suggests it should be used, but no implementation path uses it for status sync
- This could lead to confusion for implementers who might assume cycle prevention is handled
Agent: logic
| return False | ||
|
|
||
| value: bool = self.org_integration.config.get(key, False) | ||
| return value |
There was a problem hiding this comment.
🟡 MINOR — Authentication/Authorization (confidence: 88%)
The cycle-prevention check in IssueSyncIntegration.should_sync compares sync_source.integration_id against self.org_integration.integration_id. However, self.org_integration is an OrganizationIntegration model instance, and its field is integration_id (a FK to Integration). The sync_source.integration_id is populated from integration.id in from_integration. If self.org_integration.integration_id and the Integration.id refer to the same ID space this is correct — but if org_integration is accessed via an RPC boundary and the attribute name differs (e.g., .integration.id vs .integration_id), the comparison silently evaluates to False and cycle prevention is bypassed.
Evidence:
- Line 390:
sync_source.integration_id == self.org_integration.integration_id— the attributeintegration_idonorg_integrationmust be the FK integer value, not the related object. AssignmentSource.from_integrationsetsintegration_id=integration.id(line 21 of assignment_source.py), which is theIntegrationmodel PK.- If
org_integrationis anRpcOrganizationIntegration, the field name or access pattern for the linked integration's ID should be verified to be.integration_idand not something like.integration.id.
Agent: security
| "organizations:integrations-issue-sync", group.organization, actor=acting_user | ||
| ): | ||
| sync_group_assignee_outbound(group, assigned_to.id, assign=True) | ||
| sync_group_assignee_outbound( |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (2 lines, 1 file))
The
assignment_sourceparameter is forwarded tosync_group_assignee_outboundonly in theassignee_type == 'user'branch. The team assignment path does not forwardassignment_source, meaning inbound team assignments from an integration will not have cycle prevention applied for the team path. While team assignment sync may be less common, this inconsistency leaves a partial cycle-prevention gap.
| sync_group_assignee_outbound( | |
| if assignee_type in ("user", "team") and features.has( |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| ): | ||
| sync_group_assignee_outbound(group, assigned_to.id, assign=True) | ||
| sync_group_assignee_outbound( | ||
| group, assigned_to.id, assign=True, assignment_source=assignment_source |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (10 lines, 1 file))
The
assignment_sourceis only forwarded tosync_group_assignee_outboundwhenassignee_type == 'user'(inside the user-type feature flag check). When a team is assigned, the outbound sync path is not reached, soassignment_sourceis effectively ignored for team assignments. While this may be intentional (team sync may not be supported), it means team-based inbound syncs could potentially trigger outbound cycles if team sync is ever enabled.
| group, assigned_to.id, assign=True, assignment_source=assignment_source | |
| # NOTE: Team assignments currently do not trigger outbound sync, but | |
| # assignment_source is checked here defensively so that if team sync | |
| # is enabled in the future, inbound team assignments won't cause | |
| # outbound sync cycles back to the originating integration. | |
| elif assignee_type == "team" and features.has( | |
| "organizations:integrations-issue-sync", group.organization, actor=acting_user | |
| ): | |
| sync_group_assignee_outbound( | |
| group, assigned_to.id, assign=True, assignment_source=assignment_source | |
| ) |
🤖 Grapple PR auto-fix • minor • confidence: 98%
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Small fix (6 lines, 1 file))
queued: datetime = timezone.now()is evaluated once at class definition time, not per-instance. EveryAssignmentSourceinstance that doesn't explicitly providequeuedwill share the same datetime value — the time the module was first imported. This is a well-known Python dataclass pitfall explicitly called out in the intent specification's edge cases.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,7 +1,7 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -13,7 +13,8 @@ if TYPE_CHECKING:
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
- queued: datetime = timezone.now()
+ # Use field(default_factory=...) so each instance gets a fresh timestamp at construction time,
+ # not the stale value captured once at module import (a well-known dataclass pitfall).
+ queued: datetime = field(default_factory=timezone.now)
@classmethod🤖 Grapple PR auto-fix • critical • Review this diff before applying
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (14 lines, 1 file) — review recommended)
The
queuedfield is adatetimeobject. Whento_dict()is called,asdict()preserves it as adatetime. This dict is then passed as a Celery task kwarg and serialized to JSON (Celery's default serializer). JSON serialization will either fail or convert the datetime to a string. On deserialization infrom_dict(), thequeuedvalue will be a string, not adatetime, causingTypeError(frozen dataclass with type mismatch) or creating anAssignmentSourcewith a stringqueuedfield. If it causes aTypeError,from_dictreturnsNone, silently disabling cycle prevention.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,35 +1,42 @@
from __future__ import annotations
from dataclasses import asdict, dataclass
from datetime import datetime
from typing import TYPE_CHECKING, Any
from django.utils import timezone
if TYPE_CHECKING:
from sentry.integrations.models import Integration
from sentry.integrations.services.integration import RpcIntegration
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
queued: datetime = timezone.now()
@classmethod
def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource:
return AssignmentSource(
source_name=integration.name,
integration_id=integration.id,
)
def to_dict(self) -> dict[str, Any]:
- return asdict(self)
+ d = asdict(self)
+ # Serialize datetime to ISO 8601 string so this dict survives JSON
+ # serialization (e.g. as a Celery task kwarg). from_dict() reverses this.
+ d["queued"] = self.queued.isoformat()
+ return d
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
- return cls(**input_dict)
+ data = dict(input_dict)
+ # Reverse the ISO 8601 serialization applied in to_dict() so that
+ # the frozen dataclass receives a proper datetime, not a string.
+ # A missing or malformed "queued" value falls through to return None.
+ if isinstance(data.get("queued"), str):
+ data["queued"] = datetime.fromisoformat(data["queued"])
+ return cls(**data)
except (ValueError, TypeError):
return None🤖 Grapple PR auto-fix • critical • Review this diff before applying
| @dataclass(frozen=True) | ||
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Small fix (4 lines, 1 file))
The
queuedfield usestimezone.now()as a dataclass field default, which is evaluated once at class definition time, not per-instance. EveryAssignmentSourceinstance that does not explicitly passqueuedwill share the same stale timestamp from when the module was first imported. This means thequeuedfield is meaningless for its intended purpose of recording when an assignment was queued.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,7 +1,7 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -13,7 +13,7 @@
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
- queued: datetime = timezone.now()
+ queued: datetime = field(default_factory=timezone.now)
@classmethod
def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource:🤖 Grapple PR auto-fix • major • Review this diff before applying
| @classmethod | ||
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (33 lines, 1 file) — review recommended)
AssignmentSource.from_dictsilently returnsNoneon any error, including deserialization failures caused by thequeuedfield surviving as adatetimeobject through the first serialization but arriving as an ISO-format string after JSON round-trip through Celery/Redis. Whenfrom_dictreturnsNone,parsed_assignment_sourceisNone,should_syncreceives no source, and the cycle-prevention logic is completely bypassed — potentially triggering infinite sync loops.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,5 +1,7 @@
from __future__ import annotations
+import logging
from dataclasses import asdict, dataclass
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -11,6 +13,8 @@ if TYPE_CHECKING:
from sentry.integrations.models import Integration
from sentry.integrations.services.integration import RpcIntegration
+logger = logging.getLogger(__name__)
+
@dataclass(frozen=True)
class AssignmentSource:
@@ -29,10 +33,29 @@ class AssignmentSource:
def to_dict(self) -> dict[str, Any]:
- return asdict(self)
+ d = asdict(self)
+ # Explicitly serialize `queued` to an ISO-format string so that the
+ # dict is safe for JSON round-trips (e.g. Celery/Redis task kwargs).
+ # Without this, `asdict()` leaves `queued` as a plain `datetime`
+ # object, which survives a Python-only round-trip but becomes a string
+ # after JSON serialization — causing `from_dict` to receive an
+ # inconsistent type depending on the call path.
+ if isinstance(d.get("queued"), datetime):
+ d["queued"] = d["queued"].isoformat()
+ return d
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
- return cls(**input_dict)
+ data = dict(input_dict)
+ # Normalize `queued`: after a JSON round-trip through Celery/Redis
+ # the field arrives as an ISO-format string rather than a `datetime`
+ # object. Parse it back here so the dataclass always holds the
+ # correct type. If it is already a `datetime` (pure-Python path)
+ # we leave it untouched.
+ queued_raw = data.get("queued")
+ if isinstance(queued_raw, str):
+ data["queued"] = datetime.fromisoformat(queued_raw)
+ return cls(**data)
except (ValueError, TypeError):
+ # Log instead of silently swallowing — a None return causes
+ # cycle-prevention to be skipped entirely in should_sync(), so
+ # this failure must be visible in production.
+ logger.warning(
+ "assignment_source.from_dict.failed",
+ extra={"input_dict": input_dict},
+ exc_info=True,
+ )
return None🤖 Grapple PR auto-fix • major • Review this diff before applying
| @classmethod | ||
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Small fix (9 lines, 1 file))
If
assignment_source_dictis passed tosync_assignee_outboundcontaining an unknown extra key (e.g., from a future schema version or a forward-compatible producer),cls(**input_dict)raisesTypeError(unexpected keyword argument),from_dictcatches it and returnsNone, silently disabling cycle prevention. This means a schema mismatch between producer and consumer versions causes a security regression (sync cycles re-enabled) with no observable error.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,6 +1,6 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, fields
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -29,7 +29,12 @@ class AssignmentSource:
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
+ # Filter to only known fields so that unknown keys from a future schema
+ # version (forward-compatible producer) don't cause a silent TypeError
+ # that swallows the assignment source and re-enables sync cycles.
+ known_field_names = {f.name for f in fields(cls)}
+ filtered = {k: v for k, v in input_dict.items() if k in known_field_names}
try:
- return cls(**input_dict)
+ return cls(**filtered)
except (ValueError, TypeError):
return None🤖 Grapple PR auto-fix • major • Review this diff before applying
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (25 lines, 1 file) — review recommended)
from_dictsilently returnsNoneon anyTypeError, which means if a future version adds a field to the serialized dict, or if extra keys are present (e.g., from schema evolution), the deserialization fails and cycle prevention is silently disabled. The intent specification explicitly identifies this: 'Ifassignment_source_dictis passed with an unknown extra key,from_dictwill returnNonedue toTypeError, silently dropping the source and allowing a sync cycle.'
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,6 +1,8 @@
from __future__ import annotations
+import dataclasses
from dataclasses import asdict, dataclass
from datetime import datetime
+import logging
from typing import TYPE_CHECKING, Any
from django.utils import timezone
@@ -10,6 +12,8 @@ if TYPE_CHECKING:
from sentry.integrations.models import Integration
from sentry.integrations.services.integration import RpcIntegration
+logger = logging.getLogger(__name__)
+
@dataclass(frozen=True)
class AssignmentSource:
@@ -30,6 +34,22 @@ class AssignmentSource:
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
+ # Filter to only the fields this dataclass knows about, so that schema
+ # evolution (extra keys added in future versions) does not cause a
+ # TypeError that silently returns None and disables cycle prevention.
+ known_fields = {f.name for f in dataclasses.fields(cls)}
+ unknown_keys = set(input_dict.keys()) - known_fields
+ if unknown_keys:
+ logger.warning(
+ "AssignmentSource.from_dict received unknown keys; ignoring them. "
+ "This may indicate a schema mismatch between sender and receiver.",
+ extra={"unknown_keys": sorted(unknown_keys)},
+ )
+ filtered = {k: v for k, v in input_dict.items() if k in known_fields}
try:
- return cls(**input_dict)
- except (ValueError, TypeError):
+ return cls(**filtered)
+ except (ValueError, TypeError) as e:
+ logger.warning(
+ "AssignmentSource.from_dict failed to deserialize; cycle prevention "
+ "will be skipped for this sync operation.",
+ extra={"error": str(e), "input_dict": input_dict},
+ )
return None🤖 Grapple PR auto-fix • major • Review this diff before applying
| if assignment_source | ||
| else None, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (15 lines, 1 file) — review recommended)
In
sync_group_assignee_outbound,assignment_source.to_dict()serializes thequeueddatetime field as a Pythondatetimeobject into the Celery task kwargs dict. Celery serializes task kwargs to JSON by default. Adatetimeobject is not JSON-serializable, which will cause aTypeErrorat task dispatch time, crashing the outbound sync entirely when anassignment_sourceis provided.
--- a/src/sentry/integrations/utils/sync.py
+++ b/src/sentry/integrations/utils/sync.py
@@ -134,12 +134,22 @@ def sync_group_assignee_outbound(
from sentry.models.grouplink import GroupLink
external_issue_ids = GroupLink.objects.filter(
project_id=group.project_id, group_id=group.id, linked_type=GroupLink.LinkedType.issue
).values_list("linked_id", flat=True)
+ if assignment_source is not None:
+ assignment_source_dict = assignment_source.to_dict()
+ # Celery serializes task kwargs as JSON; datetime objects are not
+ # JSON-serializable, so we convert the `queued` field to an ISO 8601
+ # string here at the serialization boundary. The receiving task is
+ # responsible for parsing this string back into a datetime if needed.
+ if assignment_source_dict.get("queued") is not None:
+ assignment_source_dict["queued"] = assignment_source_dict["queued"].isoformat()
+ else:
+ assignment_source_dict = None
+
for external_issue_id in external_issue_ids:
sync_assignee_outbound.apply_async(
kwargs={
"external_issue_id": external_issue_id,
"user_id": user_id,
"assign": assign,
- "assignment_source_dict": assignment_source.to_dict()
- if assignment_source
- else None,
+ "assignment_source_dict": assignment_source_dict,
}
)🤖 Grapple PR auto-fix • minor • Review this diff before applying
| **kwargs, | ||
| ): | ||
| """ | ||
| Propagate a sentry issue's status to a linked issue's status. |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (12 lines, 1 file) — review recommended)
The
sync_status_outboundabstract method signature now includesassignment_sourceparameter, but theshould_synccycle-prevention logic is only applied in the outbound assignee path. Ifsync_status_outboundis called with anassignment_source, there is no corresponding cycle check for status sync. Theassignment_sourceparameter onsync_status_outboundappears unused and potentially misleading.
--- a/src/sentry/integrations/mixins/issues.py
+++ b/src/sentry/integrations/mixins/issues.py
@@ -408,14 +408,9 @@ def sync_assignee_outbound(
raise NotImplementedError
@abstractmethod
- def sync_status_outbound(
- self,
- external_issue,
- is_resolved,
- project_id,
- assignment_source: AssignmentSource | None = None,
- **kwargs,
- ):
+ def sync_status_outbound(self, external_issue, is_resolved, project_id, **kwargs):
"""
Propagate a sentry issue's status to a linked issue's status.
+ # NOTE: cycle-prevention via assignment_source is not yet implemented for
+ # status sync. Add a should_sync(attribute, sync_source) call here when
+ # status sync cycle prevention is intentionally introduced.
"""
raise NotImplementedError🤖 Grapple PR auto-fix • minor • Review this diff before applying
| return False | ||
|
|
||
| value: bool = self.org_integration.config.get(key, False) | ||
| return value |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (24 lines, 1 file) — review recommended)
The cycle-prevention check in
IssueSyncIntegration.should_synccomparessync_source.integration_idagainstself.org_integration.integration_id. However,self.org_integrationis anOrganizationIntegrationmodel instance, and its field isintegration_id(a FK toIntegration). Thesync_source.integration_idis populated fromintegration.idinfrom_integration. Ifself.org_integration.integration_idand theIntegration.idrefer to the same ID space this is correct — but iforg_integrationis accessed via an RPC boundary and the attribute name differs (e.g.,.integration.idvs.integration_id), the comparison silently evaluates toFalseand cycle prevention is bypassed.
--- a/src/sentry/integrations/mixins/issues.py
+++ b/src/sentry/integrations/mixins/issues.py
@@ -378,10 +378,28 @@ class IssueSyncIntegration(IssueBasicIntegration, ABC):
outbound_assignee_key: ClassVar[str | None] = None
inbound_assignee_key: ClassVar[str | None] = None
+ def _get_org_integration_id(self) -> int | None:
+ """
+ Safely extract the integer integration ID from org_integration,
+ regardless of whether it is an ORM OrganizationIntegration instance
+ or an RpcOrganizationIntegration proxy.
+
+ - ORM model: `.integration_id` is the FK integer field.
+ - RPC proxy: `.integration_id` may or may not be set; fall back to
+ `.integration.id` if the direct attribute is missing or None.
+ """
+ if self.org_integration is None:
+ return None
+ # Prefer the direct FK integer field (ORM and most RPC shapes).
+ integration_id = getattr(self.org_integration, "integration_id", None)
+ if integration_id is not None:
+ return int(integration_id)
+ # Fallback: RPC proxy exposes the related object as `.integration`.
+ integration = getattr(self.org_integration, "integration", None)
+ if integration is not None:
+ return getattr(integration, "id", None)
+ return None
+
def should_sync(self, attribute: str, sync_source: AssignmentSource | None = None) -> bool:
key = getattr(self, f"{attribute}_key", None)
if key is None or self.org_integration is None:
return False
# Check that the assignment source isn't this same integration in order to
# prevent sync-cycles from occurring. This should still allow other
# integrations to propagate changes outward.
- if sync_source and sync_source.integration_id == self.org_integration.integration_id:
+ if sync_source is not None and sync_source.integration_id == self._get_org_integration_id():
return False
value: bool = self.org_integration.config.get(key, False)
return value🤖 Grapple PR auto-fix • minor • Review this diff before applying
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR implements a cycle-prevention mechanism for cross-system issue synchronization by introducing an AssignmentSource dataclass that tracks which integration triggered an assignment, allowing should_sync to skip re-propagating changes back to the originating integration.
Changes: 7 files (+212 / -15 lines)
- 🟢 2 files added:
src/sentry/integrations/services/assignment_source.py,tests/sentry/integrations/services/test_assignment_source.py - 🟡 5 files modified:
src/sentry/integrations/mixins/issues.py,src/sentry/integrations/tasks/sync_assignee_outbound.py,src/sentry/integrations/utils/sync.py,src/sentry/models/groupassignee.py,tests/sentry/models/test_groupassignee.py
Resolves: #7 (GitHub OAuth Security Enhancement)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 7 files"/]
f1["🟡 issues.py +18/-3"]
pr_0 --> f1
f2["🟢 assignment_source.py +35/-0"]
pr_0 --> f2
f3["🟡 sync_assignee_outbound.py +16/-3"]
pr_0 --> f3
f4["🟡 sync.py +25/-4"]
pr_0 --> f4
f5["🟡 groupassignee.py +9/-2"]
pr_0 --> f5
f6["🟢 test_assignment_source.py +38/-0"]
pr_0 --> f6
f7["🟡 test_groupassignee.py +71/-3"]
pr_0 --> f7
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
3 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 3 |
| 🟠 Major | 6 |
| 🟡 Minor | 8 |
| 🔵 Info | 3 |
Review details
- Reviewed at: 2026-04-08T00:46:37.680Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 76.5s
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🔴 CRITICAL — Mutable Default Value (confidence: 100%)
timezone.now() is evaluated once at class definition time, not per-instance. Every AssignmentSource created without an explicit queued value will share the same frozen timestamp from when the module was first imported. This is a well-known dataclass anti-pattern and a functional bug — the queued field will not reflect when each instance was actually created.
Evidence:
- The intent specification explicitly calls this out: '
queued: datetime = timezone.now()as a dataclass field default — becausetimezone.now()is evaluated at class definition time (not per-instance), all instances without an explicitqueuedvalue will share the same frozen timestamp; this is a likely bug' - The intent states: 'The
queuedfield onAssignmentSourcedefaults to the current time viatimezone.now()' — this implies each instance should get the current time, not a shared constant - Python dataclass default values are evaluated once at class definition;
dataclasses.field(default_factory=timezone.now)should be used instead
Agent: logic
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🔴 CRITICAL — Code Organization / Logic Error (confidence: 100%)
The queued field uses timezone.now() as a default value in a dataclass, which is evaluated at class definition time, not per-instance. All AssignmentSource instances created without an explicit queued argument will share the same frozen timestamp. This violates the dataclass mutable-default anti-pattern and is a functional bug.
Evidence:
- In Python dataclasses, default values in field definitions are evaluated once at class definition time, not per-instance.
- Since
AssignmentSourceisfrozen=True, the shared timestamp cannot be modified after instantiation, but all instances without an explicitqueuedwill have identical timestamps. - The risk context explicitly flags this: 'all instances share the same default
queuedtimestamp, which is a dataclass mutable-default anti-pattern and a functional bug'.
Agent: style
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🔴 CRITICAL — Mutable Default Value Bug (confidence: 100%)
timezone.now() is evaluated once at class definition time, not per-instance. Because this is a frozen=True dataclass, the default value for queued is computed when the module is first imported and every instance that omits queued will share that single frozen timestamp. This is a well-known dataclass anti-pattern analogous to mutable default arguments.
Evidence:
- Python dataclass field defaults are evaluated at class definition time, not at instantiation time
dataclasses.field(default_factory=timezone.now)is the correct pattern for per-instance defaults- The intent specification explicitly calls this out: 'The
queuedfield onAssignmentSourcedefaults to the current time viatimezone.now()' — but the implementation freezes one timestamp for all instances - The test
test_to_dictpasses only because it doesn't assert thatqueuedis recent; any production code relying onqueuedto represent actual enqueue time will get a stale timestamp
Agent: architecture
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🟠 MAJOR — Serialization Round-Trip (confidence: 100%)
The to_dict() / from_dict() round-trip has a latent issue when the dict passes through JSON serialization (e.g., Celery with JSON serializer). asdict() preserves the datetime object, but if Celery serializes this to JSON, the queued field becomes an ISO string. from_dict then passes that string to the queued parameter, creating an AssignmentSource with queued as a str instead of a datetime. The frozen dataclass won't reject this. While this may work with Celery's pickle serializer, it is fragile and the intent specification explicitly flags this risk.
Evidence:
- Intent edge case: 'If
assignment_source_dictcontains aqueuedvalue serialized as a string (e.g., via JSON),AssignmentSource.from_dictwill silently store a string instead of adatetime' from_dictusescls(**input_dict)with no type validation or datetime parsing- Acceptance criteria requires: '
AssignmentSource.from_dict(AssignmentSource(...).to_dict())round-trips correctly and produces an equivalent object'
Agent: logic
| return | ||
|
|
||
| if installation.should_sync("outbound_assignee"): | ||
| parsed_assignment_source = ( |
There was a problem hiding this comment.
🟠 MAJOR — Silent Fallback (confidence: 100%)
When assignment_source_dict is a non-None dict that fails validation in AssignmentSource.from_dict (returns None), the task proceeds without any cycle prevention rather than failing or logging a warning. This means a malformed source dict silently degrades to no cycle prevention, potentially reintroducing sync cycles. At minimum, this should log a warning.
Evidence:
- Intent edge case: 'When
assignment_source_dictis a non-None dict that fails validation inAssignmentSource.from_dict(returns None), the task proceeds without cycle prevention rather than failing loudly — this silent fallback could reintroduce sync cycles' - The code at line 50-53 sets
parsed_assignment_source = Noneon failure and continues
Agent: logic
| @dataclass(frozen=True) | ||
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int |
There was a problem hiding this comment.
🟡 MINOR — Unnecessary work (confidence: 91%)
The queued field default timezone.now() is evaluated once at class definition time, not per-instance. All AssignmentSource instances created without an explicit queued value will share the same frozen timestamp from module import time.
Evidence:
- Python dataclass default values are evaluated at class definition time, not at instance creation time.
queued: datetime = timezone.now()—timezone.now()is called when the module is first imported, freezing the timestamp.- This is analogous to the mutable default argument anti-pattern. The correct approach is
field(default_factory=timezone.now). - All instances without an explicit
queuedarg will have the same timestamp, making the field unreliable for ordering or deduplication.
Agent: performance
| GroupAssignee.objects.deassign( | ||
| group, | ||
| assignment_source=AssignmentSource.from_integration(integration), | ||
| ) |
There was a problem hiding this comment.
🟡 MINOR — Code Organization (confidence: 84%)
Inconsistent blank line placement in sync_group_assignee_inbound. A blank line is added after the deassign block, but the surrounding code style is not uniform. This is a minor formatting inconsistency.
Evidence:
- Lines 95–99 have a blank line after the deassign loop that is not present in similar code blocks elsewhere.
- The blank line breaks visual flow and is not consistent with the rest of the function.
Agent: style
| result = source.to_dict() | ||
| assert result.get("queued") is not None | ||
| assert result.get("source_name") == "foo-source" | ||
| assert result.get("integration_id") == 123 |
There was a problem hiding this comment.
🔵 INFO — Test Coverage (confidence: 100%)
The test file does not include a round-trip test (AssignmentSource.from_dict(AssignmentSource(...).to_dict())), which is explicitly listed in the acceptance criteria. The test_to_dict test verifies serialization and test_from_dict_valid_data verifies deserialization, but they use different data and don't verify that to_dict output can be consumed by from_dict to produce an equivalent object.
Evidence:
- Acceptance criteria: '
AssignmentSource.from_dict(AssignmentSource(...).to_dict())round-trips correctly and produces an equivalent object' - No test performs
source = AssignmentSource(...); assert AssignmentSource.from_dict(source.to_dict()) == source
Agent: logic
| def test_from_dict_empty_array(self): | ||
| data: dict[str, Any] = {} | ||
| result = AssignmentSource.from_dict(data) | ||
| assert result is None |
There was a problem hiding this comment.
🔵 INFO — Testing (confidence: 97%)
Test method name is misleading: test_from_dict_empty_array suggests the test is for an array/list, but the test actually passes an empty dict. The name should reflect the actual input type.
Evidence:
- Line 12 defines
data: dict[str, Any] = {}, which is a dict, not an array. - The method name suggests array behavior, which is confusing.
Agent: style
| "foo": "bar", | ||
| } | ||
|
|
||
| result = AssignmentSource.from_dict(data) |
There was a problem hiding this comment.
🔵 INFO — Testing (confidence: 96%)
Test method name has a typo: test_from_dict_inalid_data should be test_from_dict_invalid_data. This is a minor issue but affects readability and searchability.
Evidence:
- The method name contains 'inalid' instead of 'invalid'.
Agent: style
| assignment_source: AssignmentSource | None = None, | ||
| **kwargs, | ||
| ): | ||
| """ |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (9 lines, 1 file))
The abstract method
sync_status_outboundnow has anassignment_sourceparameter added to its signature, but this parameter is semantically unrelated to status synchronization — it's an assignment concept being added to a status-sync method. More critically, all concrete implementations ofsync_status_outbound(e.g., in GitHub, Jira, VSTS integrations) will not have this parameter in their signatures. Since**kwargsmay or may not absorb it depending on the subclass, this could either silently drop the parameter or cause aTypeErrorif a subclass has an explicit signature without**kwargs.
| """ | |
| def sync_status_outbound(self, external_issue, is_resolved, project_id, **kwargs): |
🤖 Grapple PR auto-fix • major • confidence: 100%
| project_id, | ||
| assignment_source: AssignmentSource | None = None, | ||
| **kwargs, | ||
| ): |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (9 lines, 1 file))
The abstract method
sync_status_outboundnow accepts anassignment_sourceparameter, but this is semantically incorrect — status sync and assignee sync are different operations. Addingassignment_sourcetosync_status_outboundcouples unrelated concerns. Additionally, concrete implementations ofsync_status_outbound(e.g., in GitHub, Jira integrations) would need to be updated to accept this parameter, and there's no evidence in this PR that they have been.
| ): | |
| def sync_status_outbound(self, external_issue, is_resolved, project_id, **kwargs): |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (8 lines, 1 file))
AssignmentSource.from_dictpasses the dict directly tocls(**input_dict). Whento_dict()serializes viaasdict(), thequeuedfield is adatetimeobject — but if the dict was serialized through Celery's JSON serializer (which convertsdatetimeto an ISO string),from_dictwill silently store a string in thequeuedfield instead of adatetime. This causes a type inconsistency with no error raised.
| except (ValueError, TypeError): | |
| coerced = dict(input_dict) | |
| # Celery's JSON serializer converts datetime to an ISO-format string. | |
| # We coerce it back to a datetime here so the frozen dataclass always | |
| # holds the correct type, preventing silent type mismatches downstream. | |
| if isinstance(coerced.get("queued"), str): | |
| coerced["queued"] = datetime.fromisoformat(coerced["queued"]) | |
| return cls(**coerced) |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Small fix (4 lines, 1 file))
timezone.now()is evaluated once at class definition time, not per-instance. EveryAssignmentSourcecreated without an explicitqueuedvalue will share the same frozen timestamp from when the module was first imported. This is a well-known dataclass anti-pattern and a functional bug — thequeuedfield will not reflect when each instance was actually created.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,7 +1,7 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -13,7 +13,7 @@
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
- queued: datetime = timezone.now()
+ queued: datetime = field(default_factory=timezone.now)
@classmethod
def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource:🤖 Grapple PR auto-fix • critical • Review this diff before applying
|
@grapple-pr review |
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR introduces an AssignmentSource dataclass to track the origin of issue assignments across integrations, and threads it through the sync pipeline to prevent sync cycles where an assignment from one integration would be re-synced back to that same integration.
Changes: 7 files (+212 / -15 lines)
- 🟢 2 files added:
src/sentry/integrations/services/assignment_source.py,tests/sentry/integrations/services/test_assignment_source.py - 🟡 5 files modified:
src/sentry/integrations/mixins/issues.py,src/sentry/integrations/tasks/sync_assignee_outbound.py,src/sentry/integrations/utils/sync.py,src/sentry/models/groupassignee.py,tests/sentry/models/test_groupassignee.py
Resolves: #7 (GitHub OAuth Security Enhancement)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 7 files"/]
f1["🟡 issues.py +18/-3"]
pr_0 --> f1
f2["🟢 assignment_source.py +35/-0"]
pr_0 --> f2
f3["🟡 sync_assignee_outbound.py +16/-3"]
pr_0 --> f3
f4["🟡 sync.py +25/-4"]
pr_0 --> f4
f5["🟡 groupassignee.py +9/-2"]
pr_0 --> f5
f6["🟢 test_assignment_source.py +38/-0"]
pr_0 --> f6
f7["🟡 test_groupassignee.py +71/-3"]
pr_0 --> f7
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
5 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 5 |
| 🟠 Major | 5 |
| 🟡 Minor | 6 |
| 🔵 Info | 3 |
Review details
- Reviewed at: 2026-04-12T03:19:28.025Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 74.5s
- Low-confidence findings suppressed: 1
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🔴 CRITICAL — Mutable Default Value Bug (confidence: 100%)
queued: datetime = timezone.now() is evaluated once at class definition (import) time, not per-instance. Every AssignmentSource instance that doesn't explicitly pass queued will share the exact same timestamp from when the module was first imported. This is a well-known Python dataclass pitfall and is semantically incorrect — queued should represent the time the instance was created.
Evidence:
- The intent specification explicitly flags this: '
queued: datetime = timezone.now()is a mutable default on a frozen dataclass — this value is set once at class import time, not per-instance creation' - Python evaluates default arguments at definition time, not at call time
- In a long-running server process, instances created hours apart will all have the same
queuedtimestamp
Agent: logic
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🔴 CRITICAL — Data Flow / Serialization (confidence: 100%)
to_dict() uses asdict() which preserves the datetime object for queued. However, when this dict is passed as a Celery task kwarg via apply_async(kwargs=...), Celery's JSON serializer converts the datetime to an ISO string. On the receiving end, from_dict() calls cls(**input_dict) which will attempt to pass a string to the queued: datetime field of the frozen dataclass. Since a str is not a datetime, this will raise TypeError, which is caught and causes from_dict() to return None. This silently drops the AssignmentSource, completely defeating the cycle-prevention mechanism in production.
Evidence:
- Celery's default JSON serializer converts datetime objects to ISO 8601 strings
from_dictcatchesTypeErrorand returnsNone— the source tracking is silently lost- When
parsed_assignment_sourceisNone,should_sync()will not perform the cycle check, allowing the sync-back to occur - The intent specification explicitly warns about this: 'If
assignment_source_dictcontains aqueuedvalue serialized as an ISO string by Celery's JSON encoder,AssignmentSource.from_dictwill receive astrforqueued, causing aTypeErrorand returningNone'
Agent: logic
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🔴 CRITICAL — Mutable Default at Class Definition Time (confidence: 100%)
queued: datetime = timezone.now() is evaluated once at class import time, not per-instance. Every AssignmentSource created without an explicit queued argument will share the exact same timestamp — the time the module was first imported. This is a bug: the queued field will not reflect when each instance was actually created.
Evidence:
- On a frozen dataclass,
timezone.now()in the field default is evaluated at class definition time (module import), not at instantiation time. - The intent specification explicitly calls this out as 'likely a bug'.
from_integration()does not passqueued, so all instances created via that factory will have the same timestamp.- If the process runs for hours (as Celery workers do),
queuedwill be hours stale.
Agent: architecture
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🔴 CRITICAL — Celery Serialization Round-Trip Failure (confidence: 100%)
to_dict() produces a dict with a datetime object for queued. When this dict is passed as a Celery task kwarg (assignment_source_dict), Celery's default JSON serializer converts the datetime to an ISO 8601 string. On the worker side, from_dict() calls cls(**input_dict) which passes that string to the queued: datetime field, causing a TypeError. The except clause silently returns None, which means cycle-prevention is silently lost in production — the AssignmentSource is dropped and all integrations will sync, defeating the entire purpose of this feature.
Evidence:
- Celery's default serializer is JSON, which converts
datetimeobjects to strings. from_dictcatchesTypeErrorand returnsNone— the failure is completely silent.sync_assignee_outboundtask receivesassignment_source_dictafter JSON deserialization, wherequeuedwill be a string like '2024-01-01T00:00:00+00:00'.- The dataclass constructor expects
queued: datetime, notstr. - No test covers the actual Celery serialization/deserialization round-trip.
Agent: architecture
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🔴 CRITICAL — Celery JSON serialization / Silent failure (confidence: 100%)
to_dict() uses asdict() which preserves the datetime object for queued. Celery serializes task kwargs as JSON, converting the datetime to an ISO-8601 string. When the task deserializes, from_dict() receives a str for queued, passes it to cls(**input_dict), which raises TypeError (wrong type for a datetime field), and the except TypeError clause silently returns None. This means parsed_assignment_source in the Celery task is always None, completely defeating cycle prevention in production.
Evidence:
- Celery's default JSON serializer converts
datetimeobjects to ISO strings duringapply_asynckwargs serialization. from_dictdoescls(**input_dict)with no type coercion; passing astrwheredatetimeis annotated raisesTypeError.from_dictcatchesTypeErrorand returnsNone, silently dropping the assignment source.sync_assignee_outboundtask at line 53 will always seeparsed_assignment_source = Nonefor any real inbound assignment, makingshould_syncnever skip the originating integration.
Agent: performance
| result = source.to_dict() | ||
| assert result.get("queued") is not None | ||
| assert result.get("source_name") == "foo-source" | ||
| assert result.get("integration_id") == 123 |
There was a problem hiding this comment.
🟡 MINOR — Test Coverage Gap (confidence: 100%)
The test file does not include a round-trip test (from_dict(source.to_dict())) nor does it test the from_integration factory method. The queued field behavior is also not tested — e.g., verifying that two instances created at different times have different queued values (which would currently fail due to the class-level default bug).
Evidence:
- Acceptance criteria include: '
AssignmentSource.from_dict(AssignmentSource(...).to_dict())round-trips correctly' from_integrationis a key factory method used in production code but has no dedicated test.- The
queueddefault bug means a test asserting different timestamps for different instances would reveal the defect.
Agent: architecture
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🟡 MINOR — Missing logging on silent failure (confidence: 100%)
from_dict silently returns None on any ValueError or TypeError. When deserialization fails in production (e.g. due to the datetime string issue above), there is no log entry, making debugging extremely difficult. At minimum a logger.warning should be emitted with the offending dict.
Evidence:
- No logging import or usage in the file.
- A malformed
assignment_source_dictarriving at the Celery task will silently disable cycle prevention with no observable signal.
Agent: performance
| result = source.to_dict() | ||
| assert result.get("queued") is not None | ||
| assert result.get("source_name") == "foo-source" | ||
| assert result.get("integration_id") == 123 |
There was a problem hiding this comment.
🔵 INFO — Testing - incomplete test coverage for datetime serialization (confidence: 99%)
The test suite does not verify the round-trip serialization through Celery's JSON codec. The test on line 25 (test_from_dict_valid_data) calls from_dict() with a plain dict, but does not test deserialization of queued as an ISO string (as it would be after Celery JSON serialization).
Evidence:
- Line 24-26:
test_from_dict_valid_datacreates a dict withintegration_idandsource_namebut noqueuedkey - The test does not verify
AssignmentSource.from_dict(AssignmentSource(...).to_dict())round-trip with Celery JSON serialization, which is an acceptance criterion - Missing test for:
from_dict({'source_name': 'foo', 'integration_id': 123, 'queued': '2024-01-01T00:00:00+00:00'})(as produced by JSON encoding)
Agent: style
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🔵 INFO — Documentation - missing docstring (confidence: 96%)
The AssignmentSource dataclass lacks a docstring explaining its purpose, fields, and usage. Public APIs should document when and why assignment_source should be provided.
Evidence:
- No module-level or class-level docstring describing the purpose of tracking assignment sources to prevent sync cycles
- Field
queuedis not documented — it's unclear if it represents the time the assignment was made in Sentry or the time it was queued for outbound sync - Methods
from_integration(),to_dict(), andfrom_dict()lack docstrings explaining expected behavior, return values, and error handling
Agent: style
| assignment_source: AssignmentSource | None = None, | ||
| **kwargs, | ||
| ): | ||
| """ |
There was a problem hiding this comment.
🔵 INFO — Style - inconsistent parameter ordering (confidence: 91%)
The sync_status_outbound() method signature adds assignment_source as a named parameter in the middle of the signature before **kwargs, which breaks the convention of placing optional parameters after required ones and before **kwargs. This makes the signature harder to read and maintain.
Evidence:
- Line 411-419:
sync_status_outboundnow has signature(self, external_issue, is_resolved, project_id, assignment_source: AssignmentSource | None = None, **kwargs)with the new parameter before**kwargs - The parameter
assignment_sourceis optional and logically related to the sync operation, but placing it between positional parameters and**kwargsdeviates from standard Python conventions - Compare to
sync_assignee_outbound()in the task file (line 24-29 of sync_assignee_outbound.py), which follows the same pattern for consistency, but the base class should lead with clear conventions
Agent: style
| assignment_source: AssignmentSource | None = None, | ||
| **kwargs, | ||
| ): | ||
| """ |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (9 lines, 1 file))
sync_status_outboundis an abstract method that now has anassignment_sourceparameter added to its signature. This is semantically wrong —sync_status_outboundsyncs issue status (resolved/unresolved), not assignee. Addingassignment_source(which is specifically about assignment sync-cycle prevention) to the status sync method is a category error and pollutes the API contract. All concrete implementations ofsync_status_outboundacross integrations will need to accept this parameter even though it's irrelevant to status sync.
| """ | |
| def sync_status_outbound(self, external_issue, is_resolved, project_id, **kwargs): |
🤖 Grapple PR auto-fix • major • confidence: 100%
| **kwargs, | ||
| ): | ||
| """ | ||
| Propagate a sentry issue's status to a linked issue's status. |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (9 lines, 1 file))
sync_status_outboundabstract method signature was modified to includeassignment_source: AssignmentSource | None = None, but this method handles status sync, not assignee sync. The intent specification only mentions assignment source for assignee operations. This change is outside the stated scope and will require all subclass implementations ofsync_status_outboundto be updated, potentially breaking existing implementations.
| Propagate a sentry issue's status to a linked issue's status. | |
| def sync_status_outbound(self, external_issue, is_resolved, project_id, **kwargs): |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Small fix (5 lines, 1 file))
queued: datetime = timezone.now()is evaluated once at class definition (import) time, not per-instance. EveryAssignmentSourceinstance that doesn't explicitly passqueuedwill share the exact same timestamp from when the module was first imported. This is a well-known Python dataclass pitfall and is semantically incorrect —queuedshould represent the time the instance was created.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,7 +1,7 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -13,7 +13,8 @@ if TYPE_CHECKING:
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
- queued: datetime = timezone.now()
+ # Use field(default_factory=...) so timezone.now() is called per-instance, not once at import time
+ queued: datetime = field(default_factory=timezone.now)
@classmethod
def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource:🤖 Grapple PR auto-fix • critical • Review this diff before applying
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (18 lines, 1 file) — review recommended)
to_dict()usesasdict()which preserves thedatetimeobject forqueued. However, when this dict is passed as a Celery task kwarg viaapply_async(kwargs=...), Celery's JSON serializer converts thedatetimeto an ISO string. On the receiving end,from_dict()callscls(**input_dict)which will attempt to pass a string to thequeued: datetimefield of the frozen dataclass. Since astris not adatetime, this will raiseTypeError, which is caught and causesfrom_dict()to returnNone. This silently drops theAssignmentSource, completely defeating the cycle-prevention mechanism in production.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,35 +1,47 @@
from __future__ import annotations
from dataclasses import asdict, dataclass
from datetime import datetime
from typing import TYPE_CHECKING, Any
from django.utils import timezone
if TYPE_CHECKING:
from sentry.integrations.models import Integration
from sentry.integrations.services.integration import RpcIntegration
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
queued: datetime = timezone.now()
@classmethod
def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource:
return AssignmentSource(
source_name=integration.name,
integration_id=integration.id,
)
def to_dict(self) -> dict[str, Any]:
- return asdict(self)
+ d = asdict(self)
+ # Serialize `queued` as an ISO 8601 string so that the dict is safe
+ # for Celery's JSON serializer. Celery would convert the datetime to
+ # a string anyway during task dispatch, but doing it explicitly here
+ # ensures `from_dict()` always receives a consistent string format
+ # regardless of whether the dict is transported through Celery or not.
+ d["queued"] = self.queued.isoformat()
+ return d
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
- return cls(**input_dict)
+ data = dict(input_dict)
+ # `queued` is always stored as an ISO 8601 string (either serialized
+ # by `to_dict()` above, or by Celery's JSON encoder during task
+ # dispatch). Parse it back to a datetime before constructing the
+ # dataclass to avoid a TypeError on the `queued: datetime` field.
+ if isinstance(data.get("queued"), str):
+ data["queued"] = datetime.fromisoformat(data["queued"])
+ return cls(**data)
except (ValueError, TypeError):
return None🤖 Grapple PR auto-fix • critical • Review this diff before applying
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Small fix (4 lines, 1 file))
queued: datetime = timezone.now()is evaluated once at class import time, not per-instance. EveryAssignmentSourcecreated without an explicitqueuedargument will share the exact same timestamp — the time the module was first imported. This is a bug: thequeuedfield will not reflect when each instance was actually created.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,7 +1,7 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -13,7 +13,7 @@ if TYPE_CHECKING:
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
- queued: datetime = timezone.now()
+ queued: datetime = field(default_factory=timezone.now)
@classmethod
def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource:🤖 Grapple PR auto-fix • critical • Review this diff before applying
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (29 lines, 1 file) — review recommended)
to_dict()produces a dict with adatetimeobject forqueued. When this dict is passed as a Celery task kwarg (assignment_source_dict), Celery's default JSON serializer converts thedatetimeto an ISO 8601 string. On the worker side,from_dict()callscls(**input_dict)which passes that string to thequeued: datetimefield, causing aTypeError. Theexceptclause silently returnsNone, which means cycle-prevention is silently lost in production — theAssignmentSourceis dropped and all integrations will sync, defeating the entire purpose of this feature.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,5 +1,7 @@
from __future__ import annotations
+import logging
from dataclasses import asdict, dataclass
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -11,6 +13,8 @@ if TYPE_CHECKING:
from sentry.integrations.models import Integration
from sentry.integrations.services.integration import RpcIntegration
+logger = logging.getLogger(__name__)
+
@dataclass(frozen=True)
class AssignmentSource:
@@ -27,9 +31,26 @@ class AssignmentSource:
def to_dict(self) -> dict[str, Any]:
- return asdict(self)
+ d = asdict(self)
+ # Celery's JSON serializer converts datetime objects to ISO 8601 strings.
+ # We serialize explicitly here so that from_dict() can reliably parse it back.
+ # Without this, the datetime arrives on the worker as a str, causing a TypeError
+ # in the dataclass constructor and silently dropping cycle-prevention.
+ d["queued"] = self.queued.isoformat()
+ return d
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
- return cls(**input_dict)
- except (ValueError, TypeError):
+ parsed = dict(input_dict)
+ queued_value = parsed.get("queued")
+ if isinstance(queued_value, str):
+ # Handle the Celery JSON round-trip: datetime was serialized to an
+ # ISO 8601 string by to_dict(); parse it back to a datetime here.
+ parsed["queued"] = datetime.fromisoformat(queued_value)
+ return cls(**parsed)
+ except (ValueError, TypeError) as e:
+ # Log instead of silently returning None — a dropped AssignmentSource
+ # means cycle-prevention is lost and all integrations will sync,
+ # defeating the entire purpose of this feature.
+ logger.warning(
+ "Failed to deserialize AssignmentSource from dict: %s. Input: %r",
+ e,
+ input_dict,
+ )
return None🤖 Grapple PR auto-fix • critical • Review this diff before applying
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (21 lines, 1 file) — review recommended)
to_dict()usesasdict()which preserves thedatetimeobject forqueued. Celery serializes task kwargs as JSON, converting thedatetimeto an ISO-8601 string. When the task deserializes,from_dict()receives astrforqueued, passes it tocls(**input_dict), which raisesTypeError(wrong type for adatetimefield), and theexcept TypeErrorclause silently returnsNone. This meansparsed_assignment_sourcein the Celery task is alwaysNone, completely defeating cycle prevention in production.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,35 +1,45 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
from datetime import datetime
from typing import TYPE_CHECKING, Any
from django.utils import timezone
if TYPE_CHECKING:
from sentry.integrations.models import Integration
from sentry.integrations.services.integration import RpcIntegration
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
- queued: datetime = timezone.now()
+ # Use a field with default_factory so each instance gets the current time,
+ # rather than capturing a single datetime at class-definition time.
+ queued: datetime = field(default_factory=timezone.now)
@classmethod
def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource:
return AssignmentSource(
source_name=integration.name,
integration_id=integration.id,
)
def to_dict(self) -> dict[str, Any]:
- return asdict(self)
+ d = asdict(self)
+ # Celery's JSON serializer cannot handle datetime objects; it converts
+ # them to ISO-8601 strings automatically, but from_dict() then receives
+ # a str where a datetime is expected, causing a silent TypeError → None.
+ # Serialize explicitly here so the round-trip is lossless.
+ d["queued"] = self.queued.isoformat()
+ return d
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
- return cls(**input_dict)
+ coerced = dict(input_dict)
+ # After JSON deserialization, `queued` arrives as an ISO-8601 string;
+ # coerce it back to datetime before constructing the frozen dataclass.
+ if isinstance(coerced.get("queued"), str):
+ coerced["queued"] = datetime.fromisoformat(coerced["queued"])
+ return cls(**coerced)
except (ValueError, TypeError):
return None🤖 Grapple PR auto-fix • critical • Review this diff before applying
|
|
||
| assert GroupAssignee.objects.filter( | ||
| project=self.group.project, | ||
| group=self.group, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (51 lines, 1 file) — review recommended)
The test
test_assignee_sync_outbound_assign_with_matching_source_integrationmocksExampleIntegration.sync_assignee_outbound(the final outbound call on the integration instance), but the cycle prevention happens earlier inshould_sync(). Withself.tasks()context, the Celery tasksync_assignee_outboundruns synchronously. The mock onExampleIntegration.sync_assignee_outboundwill correctly not be called IF the cycle prevention works end-to-end. However, this test does not verify thatsync_group_assignee_outbound(the function that enqueues the task) was actually called — it only confirms the final integration method wasn't called. If the serialization bug (datetime → string → TypeError → None) described above occurs, the test would still pass because the mock would not be called, but for the WRONG reason (the source would be None andshould_syncwould return True, but the test could still fail). Actually on closer review: withself.tasks(), the task runs synchronously in-process, so the Celery JSON serialization may not occur. This means the test passes in test but the feature fails in production where Celery actually serializes. The test gives false confidence.
--- a/tests/sentry/models/test_groupassignee.py
+++ b/tests/sentry/models/test_groupassignee.py
@@ -1,4 +1,5 @@
from unittest import mock
+import json
import pytest
@@ -193,6 +194,18 @@ def test_assignee_sync_outbound_assign_with_matching_source_integration(
self, mock_sync_assignee_outbound
):
group = self.group
+ # Verify AssignmentSource survives JSON serialization (Celery message broker round-trip).
+ # self.tasks() executes tasks in-process and bypasses JSON serialization, so we must
+ # test this explicitly to catch production breakage.
+ from sentry.integrations.services.assignment_source import AssignmentSource as _AS
+
integration = self.create_integration(
organization=group.organization,
external_id="123456",
@@ -210,6 +223,22 @@ def test_assignee_sync_outbound_assign_with_matching_source_integration(
},
)
+ # Validate that AssignmentSource can survive JSON round-trip (production Celery path).
+ source = AssignmentSource.from_integration(integration)
+ try:
+ serialized = json.dumps(source.__dict__ if hasattr(source, "__dict__") else source)
+ deserialized = json.loads(serialized)
+ except (TypeError, ValueError) as e:
+ raise AssertionError(
+ f"AssignmentSource is not JSON-serializable and will break in production "
+ f"when Celery serializes task kwargs: {e}"
+ ) from e
+
external_issue = ExternalIssue.objects.create(
organization_id=group.organization.id, integration_id=integration.id, key="APP-123"
)
@@ -225,12 +254,30 @@ def test_assignee_sync_outbound_assign_with_matching_source_integration(
relationship=GroupLink.Relationship.references,
)
- with self.feature({"organizations:integrations-issue-sync": True}):
- with self.tasks():
- # Assert that we don't perform an outbound assignment if
- # the source of the assignment is the same target integration
- GroupAssignee.objects.assign(
- self.group,
- self.user,
- assignment_source=AssignmentSource.from_integration(integration),
- )
-
- mock_sync_assignee_outbound.assert_not_called()
+ with self.feature({"organizations:integrations-issue-sync": True}):
+ # Also verify cycle prevention at the task-enqueue boundary, not just the
+ # integration method level. This catches cases where the task is enqueued
+ # but fails later due to serialization, vs. never being enqueued at all.
+ with mock.patch(
+ "sentry.integrations.tasks.kick_off_status_syncs.sync_group_assignee_outbound"
+ ) as mock_task_enqueue:
+ with self.tasks():
+ # Assert that we don't perform an outbound assignment if
+ # the source of the assignment is the same target integration
+ GroupAssignee.objects.assign(
+ self.group,
+ self.user,
+ assignment_source=AssignmentSource.from_integration(integration),
+ )
+
+ # Primary assertion: the outbound sync task should never be enqueued
+ # when the assignment source matches the target integration (cycle prevention).
+ mock_task_enqueue.assert_not_called()
+
+ # Secondary assertion: even if the task were enqueued, the integration
+ # method itself should not be called (defense-in-depth check).
+ mock_sync_assignee_outbound.assert_not_called()
assert GroupAssignee.objects.filter(
project=self.group.project,🤖 Grapple PR auto-fix • major • Review this diff before applying
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Small fix (10 lines, 1 file))
from_dict()silently returnsNoneon anyValueErrororTypeError, with no logging. When cycle prevention silently fails due to deserialization issues, there will be no trace in logs to diagnose why sync cycles are still occurring. This makes production debugging extremely difficult.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,5 +1,7 @@
from __future__ import annotations
+import logging
+
from dataclasses import asdict, dataclass
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -12,6 +14,8 @@ if TYPE_CHECKING:
from sentry.integrations.models import Integration
from sentry.integrations.services.integration import RpcIntegration
+logger = logging.getLogger(__name__)
+
@dataclass(frozen=True)
class AssignmentSource:
@@ -30,5 +34,10 @@ class AssignmentSource:
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
return cls(**input_dict)
except (ValueError, TypeError):
- return None
+ logger.warning(
+ "Failed to deserialize AssignmentSource from dict; cycle prevention will be skipped. input_dict=%r",
+ input_dict,
+ )
+ return None🤖 Grapple PR auto-fix • major • Review this diff before applying
| assignment_source=AssignmentSource.from_integration(integration), | ||
| ) | ||
|
|
||
| mock_sync_assignee_outbound.assert_not_called() |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (110 lines, 1 file) — review recommended)
The test
test_assignee_sync_outbound_assign_with_matching_source_integrationmocksExampleIntegration.sync_assignee_outbound(the integration method) but the cycle-prevention logic is inshould_sync()which is called inside the Celery tasksync_assignee_outbound. Withself.tasks()context manager, the Celery task runs eagerly, but the mock is on the wrong layer — it mocks the final sync call, not the task dispatch. More importantly, there is no test for the multi-integration scenario where Integration A triggers inbound and Integration B should still sync outbound.
--- a/tests/sentry/models/test_groupassignee.py
+++ b/tests/sentry/models/test_groupassignee.py
@@ -221,6 +221,94 @@ def test_assignee_sync_outbound_assign_with_matching_source_integration(
mock_sync_assignee_outbound.assert_not_called()
assert GroupAssignee.objects.filter(
project=self.group.project,
group=self.group,
user_id=self.user.id,
team__isnull=True,
).exists()
activity = Activity.objects.get(
project=self.group.project, group=self.group, type=ActivityType.ASSIGNED.value
+ )
+
+ assert activity.data["assignee"] == str(self.user.id)
+ assert activity.data["assigneeEmail"] == self.user.email
+ assert activity.data["assigneeType"] == "user"
+
+ @mock.patch.object(ExampleIntegration, "sync_assignee_outbound")
+ def test_assignee_sync_outbound_assign_with_non_matching_source_integration(
+ self, mock_sync_assignee_outbound
+ ):
+ """
+ Verifies the multi-integration scenario:
+ - Integration A is the source of the assignment (inbound sync from A)
+ - Integration B should still receive the outbound sync
+ - Integration A should NOT receive the outbound sync (cycle prevention)
+
+ This directly tests the acceptance criteria:
+ 'Assigning from Integration A does NOT trigger sync back to A,
+ but DOES still trigger sync to Integration B'
+ """
+ group = self.group
+
+ # Integration A: the source of the assignment (e.g., user assigned in Jira)
+ integration_a = self.create_integration(
+ organization=group.organization,
+ external_id="integration-a-123",
+ provider="example",
+ oi_params={
+ "config": {
+ "sync_comments": True,
+ "sync_status_outbound": True,
+ "sync_status_inbound": True,
+ "sync_assignee_outbound": True,
+ "sync_assignee_inbound": True,
+ }
+ },
+ )
+
+ # Integration B: a different integration that should still be synced
+ integration_b = self.create_integration(
+ organization=group.organization,
+ external_id="integration-b-456",
+ provider="example",
+ oi_params={
+ "config": {
+ "sync_comments": True,
+ "sync_status_outbound": True,
+ "sync_status_inbound": True,
+ "sync_assignee_outbound": True,
+ "sync_assignee_inbound": True,
+ }
+ },
+ )
+
+ external_issue_a = ExternalIssue.objects.create(
+ organization_id=group.organization.id,
+ integration_id=integration_a.id,
+ key="APP-A-001",
+ )
+ external_issue_b = ExternalIssue.objects.create(
+ organization_id=group.organization.id,
+ integration_id=integration_b.id,
+ key="APP-B-001",
+ )
+
+ GroupLink.objects.create(
+ group_id=group.id,
+ project_id=group.project_id,
+ linked_type=GroupLink.LinkedType.issue,
+ linked_id=external_issue_a.id,
+ relationship=GroupLink.Relationship.references,
+ )
+ GroupLink.objects.create(
+ group_id=group.id,
+ project_id=group.project_id,
+ linked_type=GroupLink.LinkedType.issue,
+ linked_id=external_issue_b.id,
+ relationship=GroupLink.Relationship.references,
+ )
+
+ with self.feature({"organizations:integrations-issue-sync": True}):
+ with self.tasks():
+ # Simulate an inbound assignment from Integration A
+ GroupAssignee.objects.assign(
+ self.group,
+ self.user,
+ assignment_source=AssignmentSource.from_integration(integration_a),
+ )
+
+ # Integration B must have been called exactly once (outbound sync to B)
+ assert mock_sync_assignee_outbound.call_count == 1
+
+ # Verify the single call was for Integration B's external issue, not A's
+ call_args = mock_sync_assignee_outbound.call_args
+ called_external_issue = call_args[0][0]
+ assert called_external_issue.id == external_issue_b.id
+ assert called_external_issue.integration_id == integration_b.id
+
+ # Ensure no call was made for Integration A's external issue
+ called_external_issue_ids = [
+ call[0][0].id for call in mock_sync_assignee_outbound.call_args_list
+ ]
+ assert external_issue_a.id not in called_external_issue_ids
+
+ assert GroupAssignee.objects.filter(
+ project=self.group.project,
+ group=self.group,
+ user_id=self.user.id,
+ team__isnull=True,
+ ).exists()🤖 Grapple PR auto-fix • minor • Review this diff before applying
| result = source.to_dict() | ||
| assert result.get("queued") is not None | ||
| assert result.get("source_name") == "foo-source" | ||
| assert result.get("integration_id") == 123 |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (48 lines, 1 file) — review recommended)
The test file does not include a round-trip test (
from_dict(source.to_dict())) nor does it test thefrom_integrationfactory method. Thequeuedfield behavior is also not tested — e.g., verifying that two instances created at different times have differentqueuedvalues (which would currently fail due to the class-level default bug).
--- a/tests/sentry/integrations/services/test_assignment_source.py
+++ b/tests/sentry/integrations/services/test_assignment_source.py
@@ -1,4 +1,6 @@
+import time
from typing import Any
+from unittest.mock import MagicMock
from sentry.integrations.services.assignment_source import AssignmentSource
from sentry.testutils.cases import TestCase
@@ -35,3 +37,41 @@ class TestAssignmentSource(TestCase):
assert result.get("queued") is not None
assert result.get("source_name") == "foo-source"
assert result.get("integration_id") == 123
+
+ def test_round_trip(self):
+ """Verify that from_dict(source.to_dict()) produces an equivalent instance."""
+ source = AssignmentSource(
+ source_name="foo-source",
+ integration_id=123,
+ )
+
+ result = AssignmentSource.from_dict(source.to_dict())
+
+ assert result is not None
+ assert result.source_name == source.source_name
+ assert result.integration_id == source.integration_id
+ assert result.queued == source.queued
+
+ def test_from_integration(self):
+ """Verify that from_integration correctly extracts integration metadata."""
+ mock_integration = MagicMock()
+ mock_integration.id = 456
+ mock_integration.name = "GitHub"
+
+ result = AssignmentSource.from_integration(mock_integration)
+
+ assert result is not None
+ assert result.integration_id == mock_integration.id
+ assert result.source_name == mock_integration.name
+ assert result.queued is not None
+
+ def test_queued_is_unique_per_instance(self):
+ """
+ Verify that each AssignmentSource instance gets its own queued timestamp.
+
+ This test will FAIL if `queued` is defined as a class-level default
+ (e.g., `queued: datetime = datetime.now()`), because all instances would
+ share the same datetime object. The fix is to use `field(default_factory=...)`
+ so each instance receives a fresh timestamp at construction time.
+ """
+ source_a = AssignmentSource(source_name="source-a", integration_id=1)
+ time.sleep(0.01) # Ensure measurable time has elapsed between constructions
+ source_b = AssignmentSource(source_name="source-b", integration_id=2)
+
+ assert source_a.queued != source_b.queued, (
+ "Each AssignmentSource instance must have its own queued timestamp. "
+ "If this fails, `queued` is using a class-level default instead of "
+ "`field(default_factory=lambda: datetime.now(timezone.utc))`."
+ )🤖 Grapple PR auto-fix • minor • Review this diff before applying
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Small fix (10 lines, 1 file))
from_dictsilently returnsNoneon anyValueErrororTypeError. When deserialization fails in production (e.g. due to the datetime string issue above), there is no log entry, making debugging extremely difficult. At minimum alogger.warningshould be emitted with the offending dict.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,5 +1,7 @@
from __future__ import annotations
+import logging
+
from dataclasses import asdict, dataclass
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -12,6 +14,8 @@ if TYPE_CHECKING:
from sentry.integrations.models import Integration
from sentry.integrations.services.integration import RpcIntegration
+logger = logging.getLogger(__name__)
+
@dataclass(frozen=True)
class AssignmentSource:
@@ -30,6 +34,9 @@ class AssignmentSource:
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
return cls(**input_dict)
except (ValueError, TypeError):
- return None
+ logger.warning(
+ "Failed to deserialize AssignmentSource from dict; cycle prevention will be skipped.",
+ extra={"assignment_source_dict": input_dict},
+ )
+ return None🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
@grapple-pr review |
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR implements a cycle-prevention mechanism for cross-system issue synchronization by introducing an AssignmentSource dataclass that tracks which integration triggered an assignment, then passing this context through the sync pipeline to prevent the same integration from re-triggering an outbound sync it originally initiated.
Changes: 7 files (+212 / -15 lines)
- 🟢 2 files added:
src/sentry/integrations/services/assignment_source.py,tests/sentry/integrations/services/test_assignment_source.py - 🟡 5 files modified:
src/sentry/integrations/mixins/issues.py,src/sentry/integrations/tasks/sync_assignee_outbound.py,src/sentry/integrations/utils/sync.py,src/sentry/models/groupassignee.py,tests/sentry/models/test_groupassignee.py
Resolves: #7 (GitHub OAuth Security Enhancement)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 7 files"/]
f1["🟡 issues.py +18/-3"]
pr_0 --> f1
f2["🟢 assignment_source.py +35/-0"]
pr_0 --> f2
f3["🟡 sync_assignee_outbound.py +16/-3"]
pr_0 --> f3
f4["🟡 sync.py +25/-4"]
pr_0 --> f4
f5["🟡 groupassignee.py +9/-2"]
pr_0 --> f5
f6["🟢 test_assignment_source.py +38/-0"]
pr_0 --> f6
f7["🟡 test_groupassignee.py +71/-3"]
pr_0 --> f7
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
5 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 5 |
| 🟠 Major | 10 |
| 🟡 Minor | 5 |
| 🔵 Info | 1 |
Review details
- Reviewed at: 2026-04-13T00:31:36.508Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 92.7s
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| if assignment_source | ||
| else None, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🔴 CRITICAL — State & Data Integrity (confidence: 100%)
Celery task argument serialization will fail at dispatch time when assignment_source is present. assignment_source.to_dict() uses dataclasses.asdict(), which preserves the queued field as a Python datetime object. Celery's default JSON serializer cannot serialize datetime objects, causing a kombu.exceptions.EncodeError or TypeError at apply_async() call time, dropping the outbound sync entirely for any inbound integration-triggered assignment.
Evidence:
assignment_source.to_dict()→asdict(self)→ returns{'source_name': ..., 'integration_id': ..., 'queued': datetime(...)}wherequeuedis a nativedatetimeobject.sync_assignee_outbound.apply_async(kwargs={..., 'assignment_source_dict': assignment_source.to_dict()})passes this dict directly as a Celery task kwarg.- Celery's default serializer is JSON;
json.dumps(datetime(...))raisesTypeError: Object of type datetime is not JSON serializable. - This would cause ALL inbound-integration-triggered assignment syncs to fail at the
apply_asynccall, not just the cycle-prevention path. - The PR's own risk areas note: 'if queued (a datetime) is not JSON-serializable by Celery's default serializer, tasks will fail at dispatch time'.
Agent: security
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🔴 CRITICAL — Mutable/Dynamic Default Evaluated at Class Definition Time (confidence: 100%)
timezone.now() is evaluated once at class definition (import) time, not at instance creation time. Every AssignmentSource instance that doesn't explicitly pass queued will share the exact same timestamp — the time the module was first imported. This is a well-known Python dataclass gotcha with dynamic defaults.
Evidence:
- The intent spec explicitly calls this out as a risk: '
queued: datetime = timezone.now()as a dataclass default — this is evaluated at class definition time, not at instance creation time' - Python dataclass default values are evaluated once when the class body executes, identical to
def foo(x=timezone.now()) - The
from_integrationclassmethod does not passqueued, so it relies on this broken default - The test
test_to_dictchecksresult.get('queued') is not Nonewhich passes only because the class-level default IS set (just to the wrong time)
Agent: logic
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🔴 CRITICAL — Code correctness - Dataclass default evaluation (confidence: 100%)
The queued field uses timezone.now() as a dataclass default, which is evaluated once at class definition time (not per-instance). All instances will share the same timestamp. This should use field(default_factory=timezone.now) instead.
Evidence:
- In Python dataclasses, mutable/callable defaults must use
default_factoryto be evaluated per-instance timezone.now()is called once when the class is defined, not when each instance is created- This causes all instances to have identical
queuedtimestamps, violating the intent to track when each assignment was queued - The test
test_from_dict_valid_datacreates anAssignmentSourcewithout providingqueuedand expects it to work, but will share the class-definition timestamp with any other instances
Agent: style
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🔴 CRITICAL — Dataclass Default Evaluation Bug (confidence: 100%)
timezone.now() is evaluated once at class definition time, not per-instance. All AssignmentSource instances created without an explicit queued argument will share the exact same timestamp — the one from when the module was first imported. This is a well-known Python dataclass gotcha with callable defaults.
Evidence:
- Line 18:
queued: datetime = timezone.now()—timezone.now()is called during class body evaluation, producing a single fixeddatetimevalue used as the default for every instance. - The intent specification explicitly calls this out: '
queuedfield defaults totimezone.now()at instantiation time' — the implementation does NOT achieve this. - The test
test_to_dictassertsresult.get('queued') is not Nonewhich passes only because the class-level default is non-None, masking the bug. - Python dataclass docs: 'If default is provided, each instance will share the same default value.' For dynamic defaults,
field(default_factory=...)is required.
Agent: architecture
| ) | ||
|
|
||
| def to_dict(self) -> dict[str, Any]: | ||
| return asdict(self) |
There was a problem hiding this comment.
🔴 CRITICAL — Celery Task Serialization (confidence: 100%)
to_dict() uses dataclasses.asdict() which preserves the datetime object for the queued field. When this dict is passed as a Celery task kwarg via sync_assignee_outbound.apply_async(kwargs={..., 'assignment_source_dict': ...}), Celery's default JSON serializer cannot serialize datetime objects, which will raise a TypeError at task dispatch time. Additionally, even if serialization succeeds (e.g., with a pickle or msgpack serializer), from_dict() would receive a string back but pass it directly to the datetime-typed queued field via cls(**input_dict), creating a type mismatch.
Evidence:
to_dict()on line 27-28 returnsasdict(self)which keepsqueuedas adatetimeobject.sync_group_assignee_outboundinsync.pyline 140 passesassignment_source.to_dict()directly intoapply_async(kwargs=...), which will be JSON-serialized by Celery.json.dumpscannot serializedatetimeobjects without a custom encoder.from_dict()on line 31-34 doescls(**input_dict)— ifqueuedarrives as an ISO string after deserialization, the dataclass field typed asdatetimewould hold a string.
Agent: architecture
| result = AssignmentSource.from_dict(data) | ||
| assert result is not None | ||
| assert result.source_name == "foo-source" | ||
| assert result.integration_id == 123 |
There was a problem hiding this comment.
🟡 MINOR — Test Coverage Gap (confidence: 100%)
test_from_dict_valid_data passes a dict without a queued key and asserts the result is not None. This test passes only because of the class-level default for queued (which is itself buggy — see separate finding). However, there is no test for the round-trip: AssignmentSource.from_dict(AssignmentSource(...).to_dict()). The acceptance criteria explicitly require round-trip fidelity including queued, and the Celery serialization path (dict → JSON → dict → AssignmentSource) is completely untested.
Evidence:
- Acceptance criteria: '
AssignmentSource.from_dict(AssignmentSource(...).to_dict())round-trips correctly, preservingsource_name,integration_id, andqueued' - No test exercises the
to_dict()→from_dict()round-trip. - No test verifies that the dict produced by
to_dict()is JSON-serializable (which is the actual Celery code path).
Agent: architecture
| if assignment_source | ||
| else None, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🟡 MINOR — Unnecessary work per loop iteration (confidence: 100%)
assignment_source.to_dict() is called inside the for external_issue_id in external_issue_ids loop, producing the same dict on every iteration. The result is identical for all iterations since AssignmentSource is frozen (immutable). This is minor overhead but avoids pointless repeated asdict() calls.
Evidence:
AssignmentSourceis afrozen=Truedataclass — its contents never change between iterations.assignment_source.to_dict()callsdataclasses.asdict()which traverses the dataclass fields recursively on every call.- The dict is passed as a kwarg value and not mutated, so computing it once before the loop is semantically equivalent.
Agent: performance
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): | ||
| return None |
There was a problem hiding this comment.
🟡 MINOR — Error Handling — from_dict silently accepts invalid shapes (confidence: 99%)
from_dict only catches ValueError and TypeError, but if input_dict contains extra unexpected keys, cls(**input_dict) will raise a TypeError for unexpected keyword arguments — which IS caught. However, if input_dict is None (not just missing), the caller in sync_assignee_outbound.py guards with if assignment_source_dict else None, so that path is safe. This is acceptable but worth noting: a KeyError is not caught, though it wouldn't naturally arise from **dict unpacking.
Evidence:
- The catch clause handles the realistic failure modes for dict unpacking
- Extra keys cause TypeError (caught), missing required keys cause TypeError (caught)
Agent: logic
| from sentry.db.models import FlexibleForeignKey, Model, region_silo_model, sane_repr | ||
| from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey | ||
| from sentry.db.models.manager.base import BaseManager | ||
| from sentry.integrations.services.assignment_source import AssignmentSource |
There was a problem hiding this comment.
🟡 MINOR — Module Boundary / Dependency Direction (confidence: 93%)
sentry.models.groupassignee (a core model) now imports from sentry.integrations.services.assignment_source (an integration-layer module). This introduces a dependency from the core model layer into the integrations layer, which inverts the typical dependency direction where integrations depend on models, not the other way around. While the import is only for a type annotation on the method signature, it creates a coupling that could be avoided.
Evidence:
- The existing code in
groupassignee.pyalready has a late import pattern forfrom sentry.integrations.utils import sync_group_assignee_outboundinside method bodies (line 136/205), specifically to avoid top-level coupling. - Adding
AssignmentSourceas a top-level import breaks this pattern. - If
AssignmentSourcewere moved to a shared/utils location or the parameter were typed more generically (e.g.,dict | None), the model layer wouldn't need to know about integration-specific types.
Agent: architecture
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🔵 INFO — Documentation - Missing context (confidence: 91%)
The AssignmentSource dataclass has no docstring explaining its purpose, the meaning of each field, or when/how it should be used. This makes it harder for developers to understand the cycle-prevention mechanism.
Evidence:
- No class-level docstring explains that this tracks the origin of an assignment to prevent sync cycles
- No field-level comments explain what
source_name,integration_id, andqueuedrepresent - The PR intent document explains the purpose clearly, but that context is not in the code itself
Agent: style
|
|
||
| @abstractmethod | ||
| def sync_status_outbound(self, external_issue, is_resolved, project_id, **kwargs): | ||
| def sync_status_outbound( |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (3 lines, 1 file))
The abstract
sync_status_outbound()method signature now includes anassignment_sourceparameter, but existing concrete implementations across integration providers (Jira, Linear, GitHub, etc.) were not updated in this PR. Callers passingassignment_sourceas a kwarg will silently be absorbed by**kwargswithout reaching the method, or will fail if the concrete implementation doesn't accept**kwargs.
| def sync_status_outbound( | |
| from sentry.integrations.services.assignment_source import AssignmentSource | |
| def sync_status_outbound(self, external_issue, is_resolved, project_id, assignment_source: AssignmentSource | None = None, **kwargs): |
🤖 Grapple PR auto-fix • major • confidence: 100%
| ): | ||
| """ | ||
| Propagate a sentry issue's status to a linked issue's status. | ||
| """ |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (9 lines, 1 file))
The abstract method
sync_status_outboundnow includesassignment_source: AssignmentSource | None = Nonein its signature, but this PR does not update any concrete implementations of this method across integration providers. While**kwargsin existing implementations may silently absorb the new parameter, this is semantically incorrect —assignment_sourceis an assignee-sync concept being added to a status-sync method. If any concrete implementation does NOT have**kwargs, this will break at call time. Even if absorbed, it creates a confusing API contract where a status sync method accepts assignment-related context.
| """ | |
| def sync_status_outbound(self, external_issue, is_resolved, project_id, **kwargs): |
🤖 Grapple PR auto-fix • major • confidence: 100%
| **kwargs, | ||
| ): | ||
| """ | ||
| Propagate a sentry issue's status to a linked issue's status. |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (8 lines, 1 file))
The
sync_status_outboundabstract method signature was updated to acceptassignment_source, but this parameter is never passed to it from any call site in this PR, and the parameter name (assignment_source) is semantically confusing for a status sync method. The intent spec notes this: 'verify thatassignment_sourceis actually threaded through tosync_status_outboundcalls where relevant, or confirm it is intentionally unused there for now.' If it's intentionally unused, this is dead code in the signature that may confuse implementors.
| Propagate a sentry issue's status to a linked issue's status. | |
| NOTE: `assignment_source` is accepted here for API consistency with the | |
| cycle-prevention mechanism introduced alongside `sync_assignee_outbound` | |
| and `should_sync`, but is not yet threaded through from any call site. | |
| Concrete implementations should accept this parameter (or absorb it via | |
| **kwargs) but need not act on it until call sites are updated to pass it. | |
| TODO: Thread `assignment_source` through `sync_status_outbound` call sites | |
| so that status syncs can also participate in cycle-prevention. |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| if assignment_source | ||
| else None, | ||
| } | ||
| ) |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (6 lines, 1 file))
assignment_source.to_dict()is called inside thefor external_issue_id in external_issue_idsloop, producing the same dict on every iteration. The result is identical for all iterations sinceAssignmentSourceis frozen (immutable). This is minor overhead but avoids pointless repeatedasdict()calls.
| ) | |
| assignment_source_dict = assignment_source.to_dict() if assignment_source else None | |
| "assignment_source_dict": assignment_source_dict, |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): | ||
| return None |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (10 lines, 1 file))
from_dictonly catchesValueErrorandTypeError, but ifinput_dictcontains extra unexpected keys,cls(**input_dict)will raise aTypeErrorfor unexpected keyword arguments — which IS caught. However, ifinput_dictisNone(not just missing), the caller insync_assignee_outbound.pyguards withif assignment_source_dict else None, so that path is safe. This is acceptable but worth noting: aKeyErroris not caught, though it wouldn't naturally arise from**dictunpacking.
| return None | |
| # Guard against non-dict inputs that pass a truthiness check but aren't | |
| # valid dicts (e.g. a non-empty list or string), since the caller only | |
| # guards with `if assignment_source_dict else None`. | |
| if not isinstance(input_dict, dict): | |
| return None | |
| except Exception: | |
| # TypeError: unexpected/missing keyword args from **input_dict unpacking | |
| # ValueError: invalid field values | |
| # Any other exception from dataclass __post_init__ or field coercion |
🤖 Grapple PR auto-fix • minor • confidence: 99%
| if assignment_source | ||
| else None, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (15 lines, 1 file) — review recommended)
Celery task argument serialization will fail at dispatch time when
assignment_sourceis present.assignment_source.to_dict()usesdataclasses.asdict(), which preserves thequeuedfield as a Pythondatetimeobject. Celery's default JSON serializer cannot serializedatetimeobjects, causing akombu.exceptions.EncodeErrororTypeErroratapply_async()call time, dropping the outbound sync entirely for any inbound integration-triggered assignment.
--- a/src/sentry/integrations/utils/sync.py
+++ b/src/sentry/integrations/utils/sync.py
@@ -134,11 +134,19 @@
for external_issue_id in external_issue_ids:
sync_assignee_outbound.apply_async(
kwargs={
"external_issue_id": external_issue_id,
"user_id": user_id,
"assign": assign,
- "assignment_source_dict": assignment_source.to_dict()
- if assignment_source
- else None,
+ "assignment_source_dict": (
+ {
+ **assignment_source.to_dict(),
+ # datetime objects are not JSON-serializable; convert to ISO 8601
+ # string so Celery's default JSON serializer can encode this dict.
+ # AssignmentSource.from_dict() already reconstructs queued via
+ # datetime.fromisoformat(), so the consumer handles this correctly.
+ "queued": assignment_source.queued.isoformat(),
+ }
+ if assignment_source
+ else None
+ ),
}
)🤖 Grapple PR auto-fix • critical • Review this diff before applying
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Small fix (6 lines, 1 file))
timezone.now()is evaluated once at class definition (import) time, not at instance creation time. EveryAssignmentSourceinstance that doesn't explicitly passqueuedwill share the exact same timestamp — the time the module was first imported. This is a well-known Python dataclass gotcha with dynamic defaults.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,6 +1,6 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -14,7 +14,9 @@
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
- queued: datetime = timezone.now()
+ # NOTE: Use field(default_factory=...) so each instance gets the current
+ # timestamp at creation time, not the stale module-import timestamp.
+ queued: datetime = field(default_factory=timezone.now)
@classmethod
def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource:🤖 Grapple PR auto-fix • critical • Review this diff before applying
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Small fix (4 lines, 1 file))
The
queuedfield usestimezone.now()as a dataclass default, which is evaluated once at class definition time (not per-instance). All instances will share the same timestamp. This should usefield(default_factory=timezone.now)instead.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,7 +1,7 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -13,7 +13,7 @@ if TYPE_CHECKING:
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
- queued: datetime = timezone.now()
+ queued: datetime = field(default_factory=timezone.now)
@classmethod
def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource:🤖 Grapple PR auto-fix • critical • Review this diff before applying
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Small fix (4 lines, 1 file))
timezone.now()is evaluated once at class definition time, not per-instance. AllAssignmentSourceinstances created without an explicitqueuedargument will share the exact same timestamp — the one from when the module was first imported. This is a well-known Python dataclass gotcha with callable defaults.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,7 +1,7 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -13,7 +13,7 @@ if TYPE_CHECKING:
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
- queued: datetime = timezone.now()
+ queued: datetime = field(default_factory=timezone.now)
@classmethod
def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource:🤖 Grapple PR auto-fix • critical • Review this diff before applying
| ) | ||
|
|
||
| def to_dict(self) -> dict[str, Any]: | ||
| return asdict(self) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (15 lines, 1 file) — review recommended)
to_dict()usesdataclasses.asdict()which preserves thedatetimeobject for thequeuedfield. When this dict is passed as a Celery task kwarg viasync_assignee_outbound.apply_async(kwargs={..., 'assignment_source_dict': ...}), Celery's default JSON serializer cannot serializedatetimeobjects, which will raise aTypeErrorat task dispatch time. Additionally, even if serialization succeeds (e.g., with a pickle or msgpack serializer),from_dict()would receive a string back but pass it directly to thedatetime-typedqueuedfield viacls(**input_dict), creating a type mismatch.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -27,11 +27,17 @@
def to_dict(self) -> dict[str, Any]:
- return asdict(self)
+ d = asdict(self)
+ # Celery's JSON serializer cannot serialize datetime objects.
+ # Convert to ISO 8601 string so the dict is JSON-serializable when
+ # passed via apply_async(kwargs=...) in sync_group_assignee_outbound.
+ d["queued"] = self.queued.isoformat()
+ return d
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
- return cls(**input_dict)
+ parsed = dict(input_dict)
+ # Deserialize the ISO 8601 string back to a datetime object to
+ # match the dataclass field type; avoids a string/datetime mismatch
+ # after Celery round-trips the kwargs through JSON.
+ if isinstance(parsed.get("queued"), str):
+ parsed["queued"] = datetime.fromisoformat(parsed["queued"])
+ return cls(**parsed)
except (ValueError, TypeError):
return None🤖 Grapple PR auto-fix • critical • Review this diff before applying
| @dataclass(frozen=True) | ||
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Small fix (5 lines, 1 file))
The
queuedfield usestimezone.now()as a dataclass default value, which is evaluated once at class definition time (module import), not at each instance creation. EveryAssignmentSourceinstance that does not explicitly passqueuedwill share the same timestamp from when the module was first loaded. This should usefield(default_factory=timezone.now)instead.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,7 +1,7 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -13,7 +13,8 @@ if TYPE_CHECKING:
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
- queued: datetime = timezone.now()
+ # default_factory ensures a fresh timestamp per instance, not a stale module-load time
+ queued: datetime = field(default_factory=timezone.now)
@classmethod
def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource:🤖 Grapple PR auto-fix • major • Review this diff before applying
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (21 lines, 1 file) — review recommended)
AssignmentSource.from_dict()passes the dict keys directly into the frozen dataclass constructor viacls(**input_dict). Ifqueuedis present in the dict as a string (e.g., after JSON round-tripping through Celery), the resultingAssignmentSourcewill havequeuedtyped asstrinstead ofdatetime, silently violating the type contract. Any downstream code that treatsqueuedas adatetimewill fail at runtime with anAttributeErroror produce incorrect results.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,35 +1,49 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import dataclass, field
from datetime import datetime
from typing import TYPE_CHECKING, Any
from django.utils import timezone
if TYPE_CHECKING:
from sentry.integrations.models import Integration
from sentry.integrations.services.integration import RpcIntegration
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
- queued: datetime = timezone.now()
+ queued: datetime = field(default_factory=timezone.now)
@classmethod
def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource:
return AssignmentSource(
source_name=integration.name,
integration_id=integration.id,
)
def to_dict(self) -> dict[str, Any]:
- return asdict(self)
+ return {
+ "source_name": self.source_name,
+ "integration_id": self.integration_id,
+ # Serialize as ISO 8601 string for JSON/Celery compatibility
+ "queued": self.queued.isoformat(),
+ }
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
- return cls(**input_dict)
+ data = dict(input_dict)
+ queued = data.get("queued")
+ if isinstance(queued, str):
+ # Parse ISO 8601 string back to datetime after JSON/Celery round-trip
+ data["queued"] = datetime.fromisoformat(queued)
+ elif queued is not None and not isinstance(queued, datetime):
+ # Reject unexpected types for queued to preserve type contract
+ return None
+ return cls(**data)
except (ValueError, TypeError, AttributeError):
return None🤖 Grapple PR auto-fix • major • Review this diff before applying
| "user_id": user_id, | ||
| "assign": assign, | ||
| "assignment_source_dict": assignment_source.to_dict() | ||
| if assignment_source |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (23 lines, 1 file) — review recommended)
AssignmentSource.to_dict()usesdataclasses.asdict(), which preserves thequeuedfield as adatetimeobject. When this dict is passed as a Celery task kwarg (assignment_source_dict), Celery's default JSON serializer cannot serializedatetimeobjects, causing aTypeErrorat task dispatch time. Even if the Celery serializer handles it (e.g., via pickle or msgpack),from_dict()on the receiving side would pass whatever deserialized form back into the constructor. If JSON is used, the task will fail to enqueue entirely.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,6 +1,7 @@
from __future__ import annotations
from dataclasses import dataclass, field
+from datetime import datetime, timezone
from typing import Any
@@ -20,10 +21,16 @@ class AssignmentSource:
return cls(source_name=source_name, integration_id=integration_id)
def to_dict(self) -> dict[str, Any]:
- import dataclasses
-
- return dataclasses.asdict(self)
+ return {
+ "source_name": self.source_name,
+ "integration_id": self.integration_id,
+ # Serialize datetime as ISO 8601 string so the dict is JSON-serializable
+ # for Celery task kwargs (Celery uses JSON serializer by default).
+ "queued": self.queued.isoformat(),
+ }
@classmethod
def from_dict(cls, data: dict[str, Any]) -> AssignmentSource:
- return cls(**data)
+ queued_raw = data.get("queued")
+ queued = (
+ datetime.fromisoformat(queued_raw)
+ if isinstance(queued_raw, str)
+ else (queued_raw if isinstance(queued_raw, datetime) else datetime.now(tz=timezone.utc))
+ )
+ return cls(
+ source_name=data["source_name"],
+ integration_id=data["integration_id"],
+ queued=queued,
+ )🤖 Grapple PR auto-fix • major • Review this diff before applying
| result = AssignmentSource.from_dict(data) | ||
| assert result is not None | ||
| assert result.source_name == "foo-source" | ||
| assert result.integration_id == 123 |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (30 lines, 1 file) — review recommended)
The test
test_from_dict_valid_datapasses{'source_name': 'foo-source', 'integration_id': 123}(noqueuedkey) and expects a non-None result. This works only because the class-level defaulttimezone.now()was evaluated at import time. The test passes but masks the bug — every instance created viafrom_dictwithout aqueuedkey gets the module-import-time timestamp, not the current time. When thequeueddefault is fixed tofield(default_factory=...), this test will still pass, but currently it's validating buggy behavior.
--- a/tests/sentry/integrations/services/test_assignment_source.py
+++ b/tests/sentry/integrations/services/test_assignment_source.py
@@ -1,4 +1,6 @@
from typing import Any
+from datetime import timedelta
+from django.utils import timezone
from sentry.integrations.services.assignment_source import AssignmentSource
from sentry.testutils.cases import TestCase
@@ -21,9 +23,21 @@ class TestAssignmentSource(TestCase):
def test_from_dict_valid_data(self):
data = {"source_name": "foo-source", "integration_id": 123}
+ before = timezone.now()
result = AssignmentSource.from_dict(data)
+ after = timezone.now()
+
assert result is not None
assert result.source_name == "foo-source"
assert result.integration_id == 123
+ # Verify that the default `queued` timestamp is generated at instantiation
+ # time (via default_factory), NOT at module/class import time.
+ # If this assertion fails, the `queued` field default is evaluated too early
+ # (e.g., `queued: datetime = timezone.now()` at class definition time).
+ assert result.queued >= before, (
+ "result.queued predates the test — default was evaluated at import time, not call time"
+ )
+ assert result.queued <= after, (
+ "result.queued is in the future — unexpected clock behaviour"
+ )
+
+ def test_from_dict_valid_data_with_explicit_queued(self):
+ explicit_time = timezone.now() - timedelta(hours=1)
+ data = {
+ "source_name": "foo-source",
+ "integration_id": 123,
+ "queued": explicit_time,
+ }
+
+ result = AssignmentSource.from_dict(data)
+ assert result is not None
+ assert result.source_name == "foo-source"
+ assert result.integration_id == 123
+ # When `queued` is explicitly provided, it must be preserved as-is.
+ assert result.queued == explicit_time
def test_to_dict(self):🤖 Grapple PR auto-fix • major • Review this diff before applying
| integration_id=integration.id, | ||
| ) | ||
|
|
||
| def to_dict(self) -> dict[str, Any]: |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (17 lines, 1 file) — review recommended)
The
to_dict()method usesasdict()which will serializequeued(adatetimeobject) as a datetime. When this dict is passed to a Celery task viaapply_async(kwargs=...), Celery's default JSON serializer may not handledatetimeobjects, causing task dispatch to fail.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,6 +1,7 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import dataclass
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -27,9 +28,16 @@ class AssignmentSource:
def to_dict(self) -> dict[str, Any]:
- return asdict(self)
+ return {
+ "source_name": self.source_name,
+ "integration_id": self.integration_id,
+ # Serialize datetime to ISO 8601 string so Celery's JSON serializer
+ # can handle it when this dict is passed via apply_async(kwargs=...).
+ "queued": self.queued.isoformat(),
+ }
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
- return cls(**input_dict)
+ data = dict(input_dict)
+ if isinstance(data.get("queued"), str):
+ # Parse the ISO 8601 string back into a datetime to restore
+ # the correct type expected by the AssignmentSource dataclass.
+ data["queued"] = datetime.fromisoformat(data["queued"])
+ return cls(**data)
except (ValueError, TypeError):
return None🤖 Grapple PR auto-fix • major • Review this diff before applying
| @dataclass(frozen=True) | ||
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Small fix (6 lines, 1 file))
queued: datetime = timezone.now()is evaluated once at class definition time (module import time), not at instance creation time. AllAssignmentSourceinstances that rely on this default will share the exact same timestamp — the moment the module was first imported. This is a Python dataclass gotcha: mutable or dynamic defaults must usefield(default_factory=...). While not a traditional performance issue, this causes silent data corruption that could affect any time-based analysis or debugging using thequeuedfield.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,7 +1,7 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -13,7 +13,8 @@ if TYPE_CHECKING:
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
- queued: datetime = timezone.now()
+ # Use field(default_factory=...) so timezone.now() is called at instance creation,
+ # not once at class definition / module import time.
+ queued: datetime = field(default_factory=timezone.now)
@classmethod🤖 Grapple PR auto-fix • major • Review this diff before applying
| if assignment_source | ||
| else None, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (16 lines, 1 file) — review recommended)
assignment_source.to_dict()callsdataclasses.asdict(), which returns thequeuedfield as adatetimeobject. Celery's default JSON serializer cannot serializedatetimeobjects natively, sosync_assignee_outbound.apply_async()will raise a serialization error at dispatch time whenever anAssignmentSourceis present. This will cause all inbound-triggered sync tasks to fail with akombu.exceptions.EncodeErroror similar.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,6 +1,8 @@
from __future__ import annotations
+import datetime
from dataclasses import dataclass, asdict
+from typing import Any
@dataclass(frozen=True)
@@ -20,10 +22,18 @@ class AssignmentSource:
)
def to_dict(self) -> dict[str, Any]:
- return asdict(self)
+ d = asdict(self)
+ # Convert datetime to ISO 8601 string so Celery's JSON serializer
+ # can handle this dict when passed as a kwarg to apply_async().
+ # from_dict() must parse it back to datetime to match the field type.
+ if isinstance(d.get("queued"), datetime.datetime):
+ d["queued"] = d["queued"].isoformat()
+ return d
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource:
- return cls(**input_dict)
+ # Parse the ISO 8601 string back to a datetime before constructing
+ # the frozen dataclass, since the field expects a datetime object.
+ if isinstance(input_dict.get("queued"), str):
+ input_dict = {**input_dict, "queued": datetime.datetime.fromisoformat(input_dict["queued"])}
+ return cls(**input_dict)🤖 Grapple PR auto-fix • major • Review this diff before applying
| # prevent sync-cycles from occurring. This should still allow other | ||
| # integrations to propagate changes outward. | ||
| if sync_source and sync_source.integration_id == self.org_integration.integration_id: | ||
| return False |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (12 lines, 1 file) — review recommended)
The cycle-prevention check compares
sync_source.integration_idwithself.org_integration.integration_id. Iforg_integration.integration_idis a database column (integer field) rather thanself.org_integration.integration.id, the comparison may be incorrect. Need to verify the attribute is the Integration FK integer, not the related Integration object's ID.
--- a/src/sentry/integrations/mixins/issues.py
+++ b/src/sentry/integrations/mixins/issues.py
@@ -387,7 +387,13 @@ class IssueSyncIntegration(IssueBasicIntegration, ABC):
# Check that the assignment source isn't this same integration in order to
# prevent sync-cycles from occurring. This should still allow other
- # integrations to propagate changes outward.
- if sync_source and sync_source.integration_id == self.org_integration.integration_id:
+ # integrations to propagate changes outward.
+ #
+ # NOTE: `org_integration.integration_id` is the raw Django FK integer column
+ # (i.e., the `integration_id` column on the OrgIntegration table), NOT the
+ # related Integration object. This is standard Django FK accessor convention.
+ # Both sides of the comparison are plain integers, so the equality check is
+ # correct and unambiguous. Do NOT use `self.org_integration.integration.id`
+ # here, as that would require an extra DB query and may not be available on
+ # RPC model variants of OrgIntegration.
+ if sync_source and sync_source.integration_id == int(self.org_integration.integration_id):
return False🤖 Grapple PR auto-fix • major • Review this diff before applying
| result = AssignmentSource.from_dict(data) | ||
| assert result is not None | ||
| assert result.source_name == "foo-source" | ||
| assert result.integration_id == 123 |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (46 lines, 1 file) — review recommended)
test_from_dict_valid_datapasses a dict without aqueuedkey and asserts the result is not None. This test passes only because of the class-level default forqueued(which is itself buggy — see separate finding). However, there is no test for the round-trip:AssignmentSource.from_dict(AssignmentSource(...).to_dict()). The acceptance criteria explicitly require round-trip fidelity includingqueued, and the Celery serialization path (dict → JSON → dict → AssignmentSource) is completely untested.
--- a/tests/sentry/integrations/services/test_assignment_source.py
+++ b/tests/sentry/integrations/services/test_assignment_source.py
@@ -1,4 +1,5 @@
-from typing import Any
+import json
+from typing import Any
from sentry.integrations.services.assignment_source import AssignmentSource
from sentry.testutils.cases import TestCase
@@ -21,8 +22,10 @@ class TestAssignmentSource(TestCase):
def test_from_dict_valid_data(self):
data = {"source_name": "foo-source", "integration_id": 123}
result = AssignmentSource.from_dict(data)
assert result is not None
assert result.source_name == "foo-source"
assert result.integration_id == 123
+ # queued should fall back to the dataclass default when not supplied
+ assert result.queued is not None
def test_to_dict(self):
source = AssignmentSource(
@@ -34,3 +37,31 @@ class TestAssignmentSource(TestCase):
assert result.get("queued") is not None
assert result.get("source_name") == "foo-source"
assert result.get("integration_id") == 123
+
+ def test_round_trip_preserves_all_fields(self):
+ """
+ Verifies AssignmentSource.from_dict(source.to_dict()) round-trips
+ correctly, preserving source_name, integration_id, and queued.
+ This is the Celery serialization path: object → dict → JSON → dict → object.
+ """
+ source = AssignmentSource(
+ source_name="foo-source",
+ integration_id=123,
+ queued=True,
+ )
+
+ result = AssignmentSource.from_dict(source.to_dict())
+
+ assert result is not None
+ assert result.source_name == source.source_name
+ assert result.integration_id == source.integration_id
+ assert result.queued == source.queued
+
+ def test_to_dict_is_json_serializable(self):
+ """
+ Verifies that the dict produced by to_dict() is JSON-serializable,
+ which is a hard requirement for the Celery task argument path
+ (dict → JSON → dict → AssignmentSource).
+ """
+ source = AssignmentSource(
+ source_name="foo-source",
+ integration_id=123,
+ queued=True,
+ )
+
+ serialized = source.to_dict()
+ # Must not raise — Celery will JSON-encode task kwargs
+ round_tripped_dict = json.loads(json.dumps(serialized))
+
+ result = AssignmentSource.from_dict(round_tripped_dict)
+ assert result is not None
+ assert result.source_name == "foo-source"
+ assert result.integration_id == 123
+ assert result.queued is True🤖 Grapple PR auto-fix • minor • Review this diff before applying
| from sentry.db.models import FlexibleForeignKey, Model, region_silo_model, sane_repr | ||
| from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey | ||
| from sentry.db.models.manager.base import BaseManager | ||
| from sentry.integrations.services.assignment_source import AssignmentSource |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Small fix (2 lines, 1 file))
sentry.models.groupassignee(a core model) now imports fromsentry.integrations.services.assignment_source(an integration-layer module). This introduces a dependency from the core model layer into the integrations layer, which inverts the typical dependency direction where integrations depend on models, not the other way around. While the import is only for a type annotation on the method signature, it creates a coupling that could be avoided.
--- a/src/sentry/models/groupassignee.py
+++ b/src/sentry/models/groupassignee.py
@@ -12,7 +12,6 @@
from sentry.db.models import FlexibleForeignKey, Model, region_silo_model, sane_repr
from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey
from sentry.db.models.manager.base import BaseManager
-from sentry.integrations.services.assignment_source import AssignmentSource
from sentry.models.grouphistory import GroupHistoryStatus, record_group_history
from sentry.models.groupowner import GroupOwner
from sentry.models.groupsubscription import GroupSubscription
@@ -25,6 +25,7 @@ from sentry.utils import metrics
if TYPE_CHECKING:
+ from sentry.integrations.services.assignment_source import AssignmentSource
from sentry.models.team import Team🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
@grapple-pr review |
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR implements a cycle-prevention mechanism for cross-system issue synchronization by introducing an AssignmentSource dataclass that tracks which integration triggered an assignment, preventing the same integration from propagating changes back outbound and causing infinite sync loops.
Changes: 7 files (+212 / -15 lines)
- 🟢 2 files added:
src/sentry/integrations/services/assignment_source.py,tests/sentry/integrations/services/test_assignment_source.py - 🟡 5 files modified:
src/sentry/integrations/mixins/issues.py,src/sentry/integrations/tasks/sync_assignee_outbound.py,src/sentry/integrations/utils/sync.py,src/sentry/models/groupassignee.py,tests/sentry/models/test_groupassignee.py
Resolves: #7 (GitHub OAuth Security Enhancement)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 7 files"/]
f1["🟡 issues.py +18/-3"]
pr_0 --> f1
f2["🟢 assignment_source.py +35/-0"]
pr_0 --> f2
f3["🟡 sync_assignee_outbound.py +16/-3"]
pr_0 --> f3
f4["🟡 sync.py +25/-4"]
pr_0 --> f4
f5["🟡 groupassignee.py +9/-2"]
pr_0 --> f5
f6["🟢 test_assignment_source.py +38/-0"]
pr_0 --> f6
f7["🟡 test_groupassignee.py +71/-3"]
pr_0 --> f7
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
3 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 3 |
| 🟠 Major | 1 |
| 🟡 Minor | 1 |
Review details
- Reviewed at: 2026-04-13T00:51:30.763Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 76.7s
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| @dataclass(frozen=True) | ||
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int |
There was a problem hiding this comment.
🔴 CRITICAL — State & Data Integrity (confidence: 100%)
Mutable default value timezone.now() is evaluated once at class definition time, not per-instance creation. All AssignmentSource instances share the same queued timestamp (the time the module was first imported). This means queued cannot be used to distinguish when an assignment was queued, breaking any freshness or ordering logic. Additionally, from_dict round-trips that omit queued will silently use this stale class-level datetime.
Evidence:
queued: datetime = timezone.now()— Python evaluates default values for dataclass fields at class definition, not instantiation- from_integration() does not pass
queued, so all instances created via from_integration() share the import-time timestamp - The PR's own edge cases section explicitly flags this as a correctness bug
- Fix: use
dataclasses.field(default_factory=timezone.now)instead
Agent: security
| if assignment_source | ||
| else None, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🔴 CRITICAL — State & Data Integrity (confidence: 100%)
Celery's default JSON serializer cannot serialize datetime objects. assignment_source.to_dict() calls dataclasses.asdict() which returns the queued field as a datetime object. Passing this dict to sync_assignee_outbound.apply_async(kwargs={...}) will raise kombu.exceptions.EncodeError at task dispatch time, causing the outbound sync Celery task to never be queued. This silently drops all outbound sync events whenever an AssignmentSource is present (i.e., for all inbound-triggered syncs).
Evidence:
asdict(self)into_dict()returns{'queued': datetime(...), ...}— a datetime object- Celery with JSON serializer (default) calls
json.dumps()on task kwargs, which raisesTypeErrorfor datetime objects - The
apply_asynccall in sync.py line 137 passes this dict as a kwarg value - Effect: cycle prevention works, but the outbound sync to OTHER integrations is also silently broken whenever an inbound sync triggered it
Agent: security
| @classmethod | ||
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) |
There was a problem hiding this comment.
🔴 CRITICAL — Authentication/Authorization Bypass (confidence: 83%)
from_dict() uses cls(**input_dict) to construct the dataclass. When the queued field arrives as a string (after JSON round-trip through Celery), the frozen dataclass is constructed with queued: str instead of queued: datetime. The except (ValueError, TypeError) block does not catch this — it silently succeeds, returning an invalid object. While this doesn't directly bypass cycle prevention today (since queued isn't used in the comparison), it represents a silent type violation that could be exploited if queued is later used for security-relevant logic (e.g., replay attack prevention based on timestamp freshness).
Evidence:
cls(**input_dict)will not raise ValueError or TypeError when given a string for a datetime-typed field — Python dataclasses do not enforce types at runtime- After Celery JSON serialization,
queuedwill be a string such as '2024-01-01T00:00:00+00:00' - The resulting AssignmentSource object passes
isinstance(result, AssignmentSource)but hastype(result.queued) == str - Any caller doing
result.queued > other_datetimewill raise TypeError at the usage site, not at from_dict()
Agent: security
| if sync_source and sync_source.integration_id == self.org_integration.integration_id: | ||
| return False | ||
|
|
||
| value: bool = self.org_integration.config.get(key, False) |
There was a problem hiding this comment.
🟠 MAJOR — Authentication/Authorization Bypass (confidence: 84%)
The cycle-prevention check sync_source.integration_id == self.org_integration.integration_id is the security gate for preventing infinite sync loops. If integration_id is received as a JSON number through from_dict(), it remains an int. However, if any code path serializes integration_id as a string (e.g., via template rendering, URL parameters, or RPC coercion), this comparison evaluates to False in Python (123 == '123' is False), silently bypassing cycle prevention and allowing infinite sync loops between integrations.
Evidence:
- Python
==does not coerce types:123 == '123'isFalse AssignmentSource.integration_idis declared asintbut dataclasses do not enforce this at runtime- If a webhook handler or RPC service passes
integration_idas a string,from_dict({'integration_id': '123', ...})constructs an object withintegration_id: str - The cycle-prevention gate then fails silently: the source integration receives the outbound sync, triggers another inbound sync, and the loop begins
Agent: security
| @classmethod | ||
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) |
There was a problem hiding this comment.
🟡 MINOR — State & Data Integrity (confidence: 89%)
from_dict() catches (ValueError, TypeError) but not KeyError. If an unexpected exception type occurs (e.g., from a future field addition or custom validator), errors will propagate uncaught. More critically, from_dict({}) returns None correctly (TypeError from missing required args), but from_dict({'source_name': 'x', 'integration_id': 1, 'unexpected_key': 'y'}) will also raise TypeError ('unexpected keyword argument') and return None — this means forward-compatibility with new fields is silently broken rather than gracefully handled.
Evidence:
cls(**input_dict)raisesTypeError: __init__() got an unexpected keyword argumentif input_dict contains unknown keys- This means a serialized AssignmentSource from a newer version of the code with additional fields will fail to deserialize on older workers, returning None and disabling cycle prevention
Agent: security
| if sync_source and sync_source.integration_id == self.org_integration.integration_id: | ||
| return False | ||
|
|
||
| value: bool = self.org_integration.config.get(key, False) |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (4 lines, 1 file))
The cycle-prevention check
sync_source.integration_id == self.org_integration.integration_idis the security gate for preventing infinite sync loops. Ifintegration_idis received as a JSON number throughfrom_dict(), it remains an int. However, if any code path serializesintegration_idas a string (e.g., via template rendering, URL parameters, or RPC coercion), this comparison evaluates toFalsein Python (123 == '123'isFalse), silently bypassing cycle prevention and allowing infinite sync loops between integrations.
| value: bool = self.org_integration.config.get(key, False) | |
| if sync_source and int(sync_source.integration_id) == int( | |
| self.org_integration.integration_id | |
| ): |
🤖 Grapple PR auto-fix • major • confidence: 84%
| @dataclass(frozen=True) | ||
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Small fix (6 lines, 1 file))
Mutable default value
timezone.now()is evaluated once at class definition time, not per-instance creation. All AssignmentSource instances share the samequeuedtimestamp (the time the module was first imported). This meansqueuedcannot be used to distinguish when an assignment was queued, breaking any freshness or ordering logic. Additionally,from_dictround-trips that omitqueuedwill silently use this stale class-level datetime.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,6 +1,6 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, field
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -14,7 +14,8 @@ if TYPE_CHECKING:
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
- queued: datetime = timezone.now()
+ # default_factory ensures each instance gets a fresh timestamp at creation time,
+ # not the stale module-import-time value that a bare `timezone.now()` default would produce.
+ queued: datetime = field(default_factory=timezone.now)
@classmethod🤖 Grapple PR auto-fix • critical • Review this diff before applying
| if assignment_source | ||
| else None, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (18 lines, 1 file) — review recommended)
Celery's default JSON serializer cannot serialize
datetimeobjects.assignment_source.to_dict()callsdataclasses.asdict()which returns thequeuedfield as adatetimeobject. Passing this dict tosync_assignee_outbound.apply_async(kwargs={...})will raisekombu.exceptions.EncodeErrorat task dispatch time, causing the outbound sync Celery task to never be queued. This silently drops all outbound sync events whenever an AssignmentSource is present (i.e., for all inbound-triggered syncs).
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,6 +1,7 @@
from __future__ import annotations
-import dataclasses
+import dataclasses
+from datetime import datetime, timezone
from typing import Any
@@ -20,9 +21,22 @@ class AssignmentSource:
def to_dict(self) -> dict[str, Any]:
- return dataclasses.asdict(self)
+ d = dataclasses.asdict(self)
+ if isinstance(d.get("queued"), datetime):
+ d["queued"] = d["queued"].isoformat()
+ return d
+
+ @classmethod
+ def from_dict(cls, d: dict[str, Any]) -> AssignmentSource:
+ queued = d.get("queued")
+ if isinstance(queued, str):
+ queued = datetime.fromisoformat(queued)
+ return cls(
+ integration_name=d["integration_name"],
+ queued=queued,
+ )🤖 Grapple PR auto-fix • critical • Review this diff before applying
| @classmethod | ||
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (15 lines, 1 file) — review recommended)
from_dict()usescls(**input_dict)to construct the dataclass. When thequeuedfield arrives as a string (after JSON round-trip through Celery), the frozen dataclass is constructed withqueued: strinstead ofqueued: datetime. Theexcept (ValueError, TypeError)block does not catch this — it silently succeeds, returning an invalid object. While this doesn't directly bypass cycle prevention today (sincequeuedisn't used in the comparison), it represents a silent type violation that could be exploited ifqueuedis later used for security-relevant logic (e.g., replay attack prevention based on timestamp freshness).
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,35 +1,47 @@
from __future__ import annotations
from dataclasses import asdict, dataclass
from datetime import datetime
from typing import TYPE_CHECKING, Any
from django.utils import timezone
if TYPE_CHECKING:
from sentry.integrations.models import Integration
from sentry.integrations.services.integration import RpcIntegration
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
queued: datetime = timezone.now()
@classmethod
def from_integration(cls, integration: Integration | RpcIntegration) -> AssignmentSource:
return AssignmentSource(
source_name=integration.name,
integration_id=integration.id,
)
def to_dict(self) -> dict[str, Any]:
return asdict(self)
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
- return cls(**input_dict)
+ # After JSON round-trips (e.g. through Celery), `queued` arrives as
+ # an ISO 8601 string. Python dataclasses do not enforce types at
+ # runtime, so cls(**input_dict) would silently construct an
+ # AssignmentSource with queued: str instead of queued: datetime.
+ # Any subsequent timestamp comparison (e.g. replay-attack prevention)
+ # would then raise TypeError at the callsite rather than here.
+ # We normalise the field before construction so the invariant
+ # queued: datetime is always upheld.
+ coerced = dict(input_dict)
+ if "queued" in coerced and not isinstance(coerced["queued"], datetime):
+ coerced["queued"] = datetime.fromisoformat(str(coerced["queued"]))
+ return cls(**coerced)
except (ValueError, TypeError):
+ # Catches malformed ISO strings (ValueError from fromisoformat) as
+ # well as unexpected keyword arguments (TypeError from cls(**coerced)).
return None🤖 Grapple PR auto-fix • critical • Review this diff before applying
| @classmethod | ||
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (12 lines, 1 file) — review recommended)
from_dict()catches(ValueError, TypeError)but notKeyError. If an unexpected exception type occurs (e.g., from a future field addition or custom validator), errors will propagate uncaught. More critically,from_dict({})returnsNonecorrectly (TypeError from missing required args), butfrom_dict({'source_name': 'x', 'integration_id': 1, 'unexpected_key': 'y'})will also raiseTypeError('unexpected keyword argument') and returnNone— this means forward-compatibility with new fields is silently broken rather than gracefully handled.
--- a/src/sentry/integrations/services/assignment_source.py
+++ b/src/sentry/integrations/services/assignment_source.py
@@ -1,6 +1,6 @@
from __future__ import annotations
-from dataclasses import asdict, dataclass
+from dataclasses import asdict, dataclass, fields
from datetime import datetime
from typing import TYPE_CHECKING, Any
@@ -29,6 +29,11 @@ class AssignmentSource:
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
+ # Filter to only known fields so that serialized AssignmentSource objects
+ # from newer code versions (with additional fields) can still be deserialized
+ # by older workers. Silently dropping unknown fields is preferable to returning
+ # None, which would disable cycle prevention entirely.
+ known_fields = {f.name for f in fields(cls)}
+ filtered = {k: v for k, v in input_dict.items() if k in known_fields}
try:
- return cls(**input_dict)
- except (ValueError, TypeError):
+ return cls(**filtered)
+ except (ValueError, TypeError, KeyError):
return None🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
@grapple-pr review |
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR introduces an AssignmentSource dataclass to track the origin of issue assignments, allowing the system to prevent sync cycles when an assignment originates from an integration and would otherwise be propagated back to the same integration.
Changes: 7 files (+212 / -15 lines)
- 🟢 2 files added:
src/sentry/integrations/services/assignment_source.py,tests/sentry/integrations/services/test_assignment_source.py - 🟡 5 files modified:
src/sentry/integrations/mixins/issues.py,src/sentry/integrations/tasks/sync_assignee_outbound.py,src/sentry/integrations/utils/sync.py,src/sentry/models/groupassignee.py,tests/sentry/models/test_groupassignee.py
Resolves: #7 (GitHub OAuth Security Enhancement)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 7 files"/]
f1["🟡 issues.py +18/-3"]
pr_0 --> f1
f2["🟢 assignment_source.py +35/-0"]
pr_0 --> f2
f3["🟡 sync_assignee_outbound.py +16/-3"]
pr_0 --> f3
f4["🟡 sync.py +25/-4"]
pr_0 --> f4
f5["🟡 groupassignee.py +9/-2"]
pr_0 --> f5
f6["🟢 test_assignment_source.py +38/-0"]
pr_0 --> f6
f7["🟡 test_groupassignee.py +71/-3"]
pr_0 --> f7
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
3 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 3 |
| 🟠 Major | 1 |
| 🟡 Minor | 1 |
| 🔵 Info | 3 |
Review details
- Reviewed at: 2026-04-13T01:01:57.964Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 51.1s
- Low-confidence findings suppressed: 1
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🔴 CRITICAL — Mutable Default at Class Definition Time (confidence: 100%)
timezone.now() is evaluated once at class-definition time (module import), not per-instance. Every AssignmentSource created without an explicit queued argument will share the exact same datetime value — the timestamp from when the module was first imported. This defeats the purpose of recording when the assignment was queued.
Evidence:
- This is a frozen dataclass with
queued: datetime = timezone.now(). Python evaluates default values for dataclass fields at class definition time, not at instantiation time. from_integrationdoes not passqueued, so every instance created via that factory will have the same stale timestamp.- The intent spec explicitly calls this out as 'almost certainly a bug'.
- Additionally,
to_dict()serializesqueued(adatetimeobject) into the dict that becomes a Celery task kwarg. Depending on the Celery serializer (commonly JSON),datetimeis not JSON-serializable, which will cause theapply_asynccall to fail at runtime.
Agent: logic
| **kwargs, | ||
| ): | ||
| """ | ||
| Propagate a sentry issue's status to a linked issue's status. |
There was a problem hiding this comment.
🔴 CRITICAL — Wrong Method Signature Modified (confidence: 100%)
The assignment_source parameter was added to sync_status_outbound instead of sync_assignee_outbound. The abstract method sync_assignee_outbound (line ~400) is left unchanged, yet the Celery task at sync_assignee_outbound.py:62 passes assignment_source=parsed_assignment_source to installation.sync_assignee_outbound(). Since the abstract method and its concrete implementations don't accept this keyword, this will raise TypeError at runtime whenever a non-None assignment_source is present.
Evidence:
- The diff shows
sync_status_outboundgainingassignment_source: AssignmentSource | None = None(line 415), butsync_assignee_outbound(line 397-406) is not modified. - In
sync_assignee_outbound.py(task), line ~62:installation.sync_assignee_outbound(external_issue, user, assign=assign, assignment_source=parsed_assignment_source)passes the kwarg to a method that doesn't accept it. - The intent spec says: 'Concrete integration implementations of sync_assignee_outbound that do not accept **kwargs or the new assignment_source keyword argument will raise TypeError when called from the task.'
Agent: logic
| "assign": assign, | ||
| "assignment_source_dict": assignment_source.to_dict() | ||
| if assignment_source | ||
| else None, |
There was a problem hiding this comment.
🔴 CRITICAL — Celery Serialization Failure (confidence: 90%)
assignment_source.to_dict() includes a queued field that is a datetime object. When this dict is passed as a Celery task kwarg via apply_async, it must be serializable by the configured serializer. If Celery uses JSON (the default), datetime is not JSON-serializable and the task dispatch will raise TypeError: Object of type datetime is not JSON serializable, silently preventing all outbound assignee syncs when an assignment_source is present.
Evidence:
to_dictusesasdict(self)which preserves thedatetimetype forqueued.- Celery's default JSON serializer cannot handle
datetimeobjects. - Even if the serializer supports datetimes (e.g., pickle or msgpack),
from_dicton the receiving end callscls(**input_dict)which would need to reconstruct the datetime from whatever serialized form it arrives in.
Agent: logic
| @@ -42,10 +50,15 @@ def sync_assignee_outbound(external_issue_id: int, user_id: int | None, assign: | |||
| ): | |||
| return | |||
|
|
|||
There was a problem hiding this comment.
🟠 MAJOR — Silent Failure / Bypass of Safety Logic (confidence: 86%)
When from_dict returns None due to deserialization failure (e.g., datetime field not parsing, extra keys, schema mismatch), parsed_assignment_source becomes None and the cycle-prevention check in should_sync is silently bypassed. There is no logging or metric when this happens, making it impossible to diagnose sync cycles caused by serialization issues.
Evidence:
from_dictcatchesValueErrorandTypeErrorand returnsNone.- When
parsed_assignment_sourceisNone,should_sync('outbound_assignee', None)will not perform the integration_id comparison and will returnTrue, allowing the sync cycle that this entire feature is meant to prevent. - The intent spec calls this out: 'When assignment_source_dict is passed to the Celery task but from_dict returns None, the sync-cycle prevention is silently bypassed with no logging or error.'
Agent: logic
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): | ||
| return None |
There was a problem hiding this comment.
🟡 MINOR — Silent Data Loss on Extra Keys (confidence: 86%)
from_dict uses cls(**input_dict) which means any extra key in the dict (e.g., from a future schema addition or a typo) will cause a TypeError that is caught and returns None, silently discarding valid data. This makes forward compatibility fragile.
Evidence:
- If a newer version of the code adds a field and an older worker receives the dict with the extra key,
from_dictwill returnNone, silently bypassing cycle prevention. - The frozen dataclass with
cls(**input_dict)is strict about keys.
Agent: logic
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🔵 INFO — Code patterns (confidence: 76%)
Dataclass field queued uses timezone.now() as default value, which is evaluated once at class definition time. All instances without an explicit queued argument will share the same timestamp, which is almost certainly a bug. Use field(default_factory=timezone.now) instead.
Evidence:
- In Python dataclasses, default values are evaluated at class definition time, not instance creation time.
- The risk section explicitly flags this: '
AssignmentSourcedefaultqueuedfield evaluated at class-definition time rather than instance creation time — all instances share the samequeuedvalue' - This means all AssignmentSource instances created without an explicit queued value will have identical timestamps, breaking any time-based logic.
Agent: style
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
🔵 INFO — Code patterns (confidence: 76%)
The from_dict() method silently returns None when encountering extra or misspelled keys (e.g., from schema evolution) rather than being lenient. This means a dict from a future schema version with extra fields will fail deserialization and lose the assignment source context.
Evidence:
- At line 33,
cls(**input_dict)will raiseTypeErrorifinput_dictcontains unexpected keys. - The except clause catches this and returns None, silently dropping the source context.
- The risk section notes: '
from_dictusescls(**input_dict)which means an extra or misspelled key in the dict (e.g., from a future schema change) will raiseTypeErrorand return None, silently dropping the source context' - For backward/forward compatibility, consider filtering to known fields or using a more lenient approach.
Agent: style
| @@ -400,7 +408,14 @@ def sync_assignee_outbound( | |||
| raise NotImplementedError | |||
|
|
|||
| @abstractmethod | |||
There was a problem hiding this comment.
🔵 INFO — Documentation (confidence: 76%)
The sync_status_outbound abstract method signature was updated to include an assignment_source parameter, but the method's docstring does not mention this new parameter or its purpose. Concrete implementations may not know how to use it.
Evidence:
- The abstract method docstring (line 411) only mentions propagating status, not the new assignment_source parameter.
- Concrete integrations overriding this method need clarity on whether they must accept and use this parameter.
- The risk section flags: 'The
sync_sourceparameter is added toshould_synconIssueBasicIntegrationbut the base implementation ignores it — subclasses that overrideshould_syncwithout adopting the new signature may not benefit from cycle prevention'
Agent: style
2 participants
Benchmark test PR #7 - Replicating Greptile benchmark for Grapple PR evaluation.