feat(ecosystem): Implement cross-system issue synchronization#79
feat(ecosystem): Implement cross-system issue synchronization#79camcalaquian wants to merge 1 commit into
Conversation
|
Tenki Code Review - Complete Files Reviewed: 7 By Severity:
This PR adds assignment source tracking to prevent sync cycles, but contains two bugs: the Files Reviewed (7 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
The PR introduces AssignmentSource, a new dataclass to track which integration initiated an assignment, enabling cycle-prevention logic in the should_sync() method. The implementation spans sync utilities, task handlers, and model APIs.
Findings
🔴 High Severity: Dataclass Default Evaluated Once at Module Import
The queued field in AssignmentSource uses timezone.now() as a direct default, which Python evaluates exactly once when the module loads. Every instance created without an explicit queued= argument shares the same timestamp—the process startup time—rather than capturing the time each assignment was actually queued. This breaks the semantic meaning of the field and any time-based diagnostics.
Impact: Loss of temporal information; all assignments appear to have been queued at process startup.
🟡 Medium Severity: Silent Loss of Cycle-Prevention Context
The from_dict() classmethod catches ValueError and TypeError and returns None without logging. If deserialization fails (e.g., schema drift from a future version, extra keys), the cycle-prevention context is silently dropped. The calling code then proceeds with parsed_assignment_source = None, allowing the same integration to trigger an outbound sync and potentially create a cycle.
Impact: Deserialization failures are unobservable; sync cycles may occur without any production log evidence.
Files Reviewed
src/sentry/integrations/mixins/issues.py— Updatedshould_sync()signaturessrc/sentry/integrations/services/assignment_source.py—⚠️ New dataclass with two bugssrc/sentry/integrations/tasks/sync_assignee_outbound.py— Task handler integrating source trackingsrc/sentry/integrations/utils/sync.py— Sync pipeline updating to pass assignment sourcesrc/sentry/models/groupassignee.py— Model API extendingassign()/deassign()with source paramtests/sentry/integrations/services/test_assignment_source.py— Unit tests for AssignmentSourcetests/sentry/models/test_groupassignee.py— Integration tests including cycle-prevention scenario
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🔴 Dataclass default timezone.now() is evaluated once at module import, not per instance (bug)
In assignment_source.py line 18, the queued field is defined as:
queued: datetime = timezone.now()Because Python evaluates default expressions at class-body execution time (i.e. when the module is first imported), timezone.now() is called exactly once and the resulting datetime object is shared as the default for every AssignmentSource instance created without an explicit queued= argument. All such instances will report the same queued timestamp — the module import time — regardless of when they were actually constructed. This defeats the purpose of the field as a "when was this assignment queued" timestamp.
💡 Suggestion: Use dataclasses.field(default_factory=timezone.now) so that timezone.now() is called fresh for each new instance. This requires adding field to the import from dataclasses.
| queued: datetime = timezone.now() | |
| queued: datetime = field(default_factory=timezone.now) |
📋 Prompt for AI Agents
In src/sentry/integrations/services/assignment_source.py at line 18, change the queued field default from a direct call to a default_factory. Replace queued: datetime = timezone.now() with queued: datetime = field(default_factory=timezone.now). Also add field to the from dataclasses import asdict, dataclass import at line 3 so it becomes from dataclasses import asdict, dataclass, field. This ensures each instance gets a fresh timezone.now() value at construction time rather than all sharing the single timestamp captured at module import.
| def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None: | ||
| try: | ||
| return cls(**input_dict) | ||
| except (ValueError, TypeError): | ||
| return None |
There was a problem hiding this comment.
🟠 AssignmentSource.from_dict silently returns None on any malformed dict, losing cycle-prevention context (bug)
In assignment_source.py lines 31-34, from_dict catches all ValueError and TypeError exceptions and returns None. While this prevents crashes, it means that if the serialized dict is malformed (e.g. extra keys from a future version, or a type mismatch), the assignment_source is silently dropped. The calling code in sync_assignee_outbound.py line 53-55 then proceeds with parsed_assignment_source = None, which disables the sync-cycle prevention: installation.should_sync("outbound_assignee", None) returns True even for the originating integration, potentially triggering a sync loop. There is no logging in the error path to diagnose deserialization failures.
💡 Suggestion: Add a logger.warning (or logger.exception) call in the except block before returning None, so that deserialization failures are observable. This avoids silent loss of cycle-prevention context.
📋 Prompt for AI Agents
In src/sentry/integrations/services/assignment_source.py, in the from_dict classmethod (lines 31-35), add logging when the dict cannot be deserialized. Import a logger at the module level (e.g. import logging; logger = logging.getLogger(__name__)), then in the except (ValueError, TypeError): block add logger.warning("AssignmentSource.from_dict failed to deserialize", extra={"input_dict": input_dict}, exc_info=True) before the return None. This makes deserialization failures visible in logs so silent loss of cycle-prevention context can be detected and debugged.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
🔴 Dataclass default timezone.now() is evaluated once at import time, not per-instance
The queued field default timezone.now() in the AssignmentSource dataclass is evaluated once at module import time, not each time a new instance is created. This is a well-known Python gotcha: dataclass field defaults are evaluated at class definition time, just like function default arguments. Every AssignmentSource instance created without an explicit queued argument (including all instances from from_integration() at assignment_source.py:22) will share the same stale timestamp from when the module was first imported. The fix is to use field(default_factory=timezone.now).
| queued: datetime = timezone.now() | |
| queued: datetime = field(default_factory=timezone.now) |
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Uh oh!
There was an error while loading. Please reload this page.