feat(ecosystem): Implement cross-system issue synchronization - #4
feat(ecosystem): Implement cross-system issue synchronization#4CodingKylo wants to merge 1 commit into
Conversation
![re-entry-ai[bot]](https://avatars.githubusercontent.com/in/3466347?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🔴 Risk Score: 74/100 · HIGH
| Dimension | Level |
|---|---|
| Likelihood | 🔴 Critical |
| Impact | 🔴 Critical |
| Detectability | 🟠 High |
Intent
Implement cross-system issue synchronization for assignees while preventing sync cycles by propagating an assignment source through the sync pipeline.
Summary
The PR introduces an AssignmentSource dataclass and threads it through inbound/outbound assignee sync, including a new sync_source parameter on IssueSyncIntegration.should_sync() to prevent sync cycles. Outbound task payloads now accept assignment_source_dict and pass a parsed AssignmentSource into should_sync() and sync_assignee_outbound(). The highest risk is a contract mismatch in AssignmentSource serialization/deserialization: queued is a datetime default but from_dict() blindly constructs the dataclass, which will fail if queued arrives as a string (common in task payloads), silently disabling cycle prevention. Reviewers must verify the end-to-end payload contract and that all should_sync() call sites supply the new sync_source/assignment_source consistently.
🎯 Review Focus
Validate the end-to-end AssignmentSource payload contract (especially queued serialization) and confirm every should_sync() call site supplies the new sync_source/assignment_source so sync-cycle prevention cannot be bypassed by parsing failures.
Key Findings
- 🚨 [src/sentry/integrations/services/assignment_source.py:L24-L35] CRITICAL:
to_dict()includesqueued: datetime = timezone.now()butfrom_dict()blindly doescls(**input_dict)and returnsNoneonValueError/TypeError— ifqueuedis serialized as an ISO string in the task payload, deserialization will fail and cycle prevention is silently bypassed. — Fix: makefrom_dict()parsequeued(acceptstr|datetime) and never silently returnNonefor partial/valid payloads; e.g.:
from datetime import datetime
from django.utils.dateparse import parse_datetime
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
queued = input_dict.get("queued")
if isinstance(queued, str):
queued_dt = parse_datetime(queued)
if queued_dt is None:
return None
input_dict = {**input_dict, "queued": queued_dt}
return cls(**input_dict)
except (ValueError, TypeError):
return NoneAlso consider removing queued from to_dict() entirely if it’s not required for sync-cycle logic.
⚠️ [src/sentry/integrations/tasks/sync_assignee_outbound.py:L42-L58] WARNING: Cycle prevention depends onparsed_assignment_sourcebeing non-None, butAssignmentSource.from_dict()can returnNoneon deserialization issues, and the code then callsinstallation.should_sync("outbound_assignee", parsed_assignment_source)withNone. Ifshould_sync()only blocks whensync_sourceis truthy, then any payload contract failure disables the protection. — Fix: treat parse failure as a hard failure or at least log loudly and default to a safe behavior. For example:
parsed = AssignmentSource.from_dict(assignment_source_dict) if assignment_source_dict else None
if assignment_source_dict and parsed is None:
logger.error("invalid_assignment_source_payload", extra={"assignment_source_dict": assignment_source_dict})
return # or raise to avoid bypassing cycle preventionAlternatively, change should_sync() to block when assignment_source_dict indicates the same integration even if parsing fails (requires passing raw integration_id).
✅ Action Checklist
- [ ] CRITICAL [src/sentry/integrations/services/assignment_source.py:L24-L35] —
to_dict()includesqueued: datetime = timezone.now()butfrom_dict()blindly doescls(**input_dict)and returnsNoneonValueError/TypeError— ifqueuedis serialized as an ISO string in the task payload, deserialization will fail and cycle prevention is silently bypassed. — Fix: makefrom_dict()parsequeued(acceptstr|datetime) and never silently returnNonefor partial/valid payloads; e.g.:
from datetime import datetime
from django.utils.dateparse import parse_datetime
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
queued = input_dict.get("queued")
if isinstance(queued, str):
queued_dt = parse_datetime(queued)
if queued_dt is None:
return None
input_dict = {**input_dict, "queued": queued_dt}
return cls(**input_dict)
except (ValueError, TypeError):
return NoneAlso consider removing queued from to_dict() entirely if it’s not required for sync-cycle logic.
- [ ] WARNING [src/sentry/integrations/tasks/sync_assignee_outbound.py:L42-L58] — Cycle prevention depends on
parsed_assignment_sourcebeing non-None, butAssignmentSource.from_dict()can returnNoneon deserialization issues, and the code then callsinstallation.should_sync("outbound_assignee", parsed_assignment_source)withNone. Ifshould_sync()only blocks whensync_sourceis truthy, then any payload contract failure disables the protection. — Fix: treat parse failure as a hard failure or at least log loudly and default to a safe behavior. For example:
parsed = AssignmentSource.from_dict(assignment_source_dict) if assignment_source_dict else None
if assignment_source_dict and parsed is None:
logger.error("invalid_assignment_source_payload", extra={"assignment_source_dict": assignment_source_dict})
return # or raise to avoid bypassing cycle preventionAlternatively, change should_sync() to block when assignment_source_dict indicates the same integration even if parsing fails (requires passing raw integration_id).
- [ ] SUGGESTION — [src/sentry/integrations/mixins/issues.py:L379-L392] WARNING:
should_sync()now takessync_source: AssignmentSource | None = Noneand blocks only whensync_source.integration_id == self.org_integration.integration_id. Verify all call sites ofshould_sync()across the integration sync surface are updated to pass the newsync_source; otherwise cycles can still occur for other assignee-related flows. Concrete action: grep forshould_sync(and update every invocation to passsync_source=AssignmentSource.from_integration(integration)(or equivalent) where outbound/inbound sync is triggered. - [ ] SUGGESTION — [src/sentry/integrations/utils/sync.py:L109-L134] INFO:
sync_group_assignee_outbound()now acceptsassignment_sourcebut the diff snippet truncates theapply_asynckwargs. Ensure the task is actually called withassignment_source_dict(not justassignment_source) and that the dict matches whatAssignmentSource.from_dict()expects. Concrete action: insync_group_assignee_outbound, setassignment_source_dict=assignment_source.to_dict()(or a contract-stable subset like{source_name, integration_id}) and add a unit test asserting the queued payload round-trips.
Suggestions
- [src/sentry/integrations/mixins/issues.py:L379-L392] WARNING:
should_sync()now takessync_source: AssignmentSource | None = Noneand blocks only whensync_source.integration_id == self.org_integration.integration_id. Verify all call sites ofshould_sync()across the integration sync surface are updated to pass the newsync_source; otherwise cycles can still occur for other assignee-related flows. Concrete action: grep forshould_sync(and update every invocation to passsync_source=AssignmentSource.from_integration(integration)(or equivalent) where outbound/inbound sync is triggered. - [src/sentry/integrations/utils/sync.py:L109-L134] INFO:
sync_group_assignee_outbound()now acceptsassignment_sourcebut the diff snippet truncates theapply_asynckwargs. Ensure the task is actually called withassignment_source_dict(not justassignment_source) and that the dict matches whatAssignmentSource.from_dict()expects. Concrete action: insync_group_assignee_outbound, setassignment_source_dict=assignment_source.to_dict()(or a contract-stable subset like{source_name, integration_id}) and add a unit test asserting the queued payload round-trips.
Posted by re-entry.ai · Risk governance for autonomous engineering teams
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🔴 Risk Score: 75/100 · HIGH
| Dimension | Level |
|---|---|
| Likelihood | 🔴 Critical |
| Impact | 🔴 Critical |
| Detectability | 🟠 High |
Intent
Add cross-system issue/assignee synchronization with metadata-based cycle prevention so outbound syncs don’t re-trigger inbound syncs across integrations.
Summary
The PR introduces an AssignmentSource payload that is serialized into outbound assignee sync tasks and threaded through should_sync to prevent sync cycles when the source integration matches the current integration. It also extends GroupAssignee.assign/deassign and sync utilities to accept and propagate assignment_source. The top risks are (1) a serialization contract bug: AssignmentSource.queued is a datetime default that is not safely serialized/deserialized across task boundaries, and (2) cycle-prevention correctness: should_sync now depends on sync_source being passed consistently, but malformed/None payloads silently disable the guard. Verify that the task payload serialization round-trips correctly and that all call sites pass the new argument consistently so cycle prevention is reliable.
🎯 Review Focus
The serialization/deserialization contract for AssignmentSource across the async task boundary (to_dict()/from_dict() and queued handling) and whether cycle-prevention remains reliable when payloads are missing or malformed.
Key Findings
- 🚨 [src/sentry/integrations/services/assignment_source.py:L14-L35] CRITICAL:
queuedis adatetimedefault butto_dict()returns rawdatetimeobjects andfrom_dict()blindlycls(**input_dict); this will break task serialization and/or produce inconsistent values. —queued: datetime = timezone.now()is evaluated at import time (dataclass default), andasdict(self)will include adatetimeobject. If the task queue expects JSON-serializable payloads, this will either fail at enqueue time or deserialize incorrectly, and becausefrom_dict()catchesTypeError/ValueErrorand returnsNone, the cycle-prevention guard can be silently disabled. — Fix: makequeuedserialization explicit and ensure the default is per-instance. Example:
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
queued: datetime = field(default_factory=timezone.now)
def to_dict(self) -> dict[str, Any]:
return {
"source_name": self.source_name,
"integration_id": self.integration_id,
"queued": self.queued.isoformat(),
}
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
queued = input_dict.get("queued")
if isinstance(queued, str):
queued = datetime.fromisoformat(queued)
return cls(
source_name=input_dict["source_name"],
integration_id=int(input_dict["integration_id"]),
queued=queued or timezone.now(),
)
except (KeyError, ValueError, TypeError):
return NoneAlso add a test that enqueues sync_assignee_outbound with assignment_source_dict and asserts it round-trips.
- 🚨 [src/sentry/integrations/mixins/issues.py:L378-L408] WARNING: Cycle prevention is conditional on
sync_sourcebeing passed correctly; malformed/Noneassignment_source_dictdisables the guard and can re-propagate changes. —should_sync(..., sync_source)returnsFalseonly whensync_source.integration_id == self.org_integration.integration_id. ButAssignmentSource.from_dict()returnsNoneon any parsing issue, andsync_assignee_outboundpassesparsed_assignment_sourcedirectly. That means any serialization mismatch (see critical finding) or missing field will silently fall back toinstallation.should_sync(..., None)and allow outbound propagation, defeating the cycle-prevention intent. — Fix: treat parse failure as “do not sync” (fail closed) or at least log loudly. Example insync_assignee_outbound:
parsed = AssignmentSource.from_dict(assignment_source_dict) if assignment_source_dict else None
if assignment_source_dict and parsed is None:
logger.warning("invalid-assignment-source", extra={"assignment_source_dict": assignment_source_dict})
return # fail closed to preserve cycle preventionAlternatively, change from_dict to raise and handle explicitly at the task boundary so you don’t silently disable the guard.
✅ Action Checklist
- [ ] CRITICAL [src/sentry/integrations/services/assignment_source.py:L14-L35] —
queuedis adatetimedefault butto_dict()returns rawdatetimeobjects andfrom_dict()blindlycls(**input_dict); this will break task serialization and/or produce inconsistent values. —queued: datetime = timezone.now()is evaluated at import time (dataclass default), andasdict(self)will include adatetimeobject. If the task queue expects JSON-serializable payloads, this will either fail at enqueue time or deserialize incorrectly, and becausefrom_dict()catchesTypeError/ValueErrorand returnsNone, the cycle-prevention guard can be silently disabled. — Fix: makequeuedserialization explicit and ensure the default is per-instance. Example:
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
queued: datetime = field(default_factory=timezone.now)
def to_dict(self) -> dict[str, Any]:
return {
"source_name": self.source_name,
"integration_id": self.integration_id,
"queued": self.queued.isoformat(),
}
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
queued = input_dict.get("queued")
if isinstance(queued, str):
queued = datetime.fromisoformat(queued)
return cls(
source_name=input_dict["source_name"],
integration_id=int(input_dict["integration_id"]),
queued=queued or timezone.now(),
)
except (KeyError, ValueError, TypeError):
return NoneAlso add a test that enqueues sync_assignee_outbound with assignment_source_dict and asserts it round-trips.
- [ ] WARNING [src/sentry/integrations/mixins/issues.py:L378-L408] — Cycle prevention is conditional on
sync_sourcebeing passed correctly; malformed/Noneassignment_source_dictdisables the guard and can re-propagate changes. —should_sync(..., sync_source)returnsFalseonly whensync_source.integration_id == self.org_integration.integration_id. ButAssignmentSource.from_dict()returnsNoneon any parsing issue, andsync_assignee_outboundpassesparsed_assignment_sourcedirectly. That means any serialization mismatch (see critical finding) or missing field will silently fall back toinstallation.should_sync(..., None)and allow outbound propagation, defeating the cycle-prevention intent. — Fix: treat parse failure as “do not sync” (fail closed) or at least log loudly. Example insync_assignee_outbound:
parsed = AssignmentSource.from_dict(assignment_source_dict) if assignment_source_dict else None
if assignment_source_dict and parsed is None:
logger.warning("invalid-assignment-source", extra={"assignment_source_dict": assignment_source_dict})
return # fail closed to preserve cycle preventionAlternatively, change from_dict to raise and handle explicitly at the task boundary so you don’t silently disable the guard.
- [ ] SUGGESTION — [src/sentry/integrations/tasks/sync_assignee_outbound.py:L24-L60] WARNING:
assignment_source_dictis typed asdict[str, Any]but there’s no validation that it matches the expected schema;from_dict()returnsNoneand the code proceeds. — Add explicit schema checks and/or fail-closed behavior (see key finding) and add a unit test for malformed payloads (missingintegration_id, wrong types,queuedas string) to ensure cycle prevention remains correct. - [ ] SUGGESTION — [src/sentry/integrations/utils/sync.py:L104-L134] INFO: Ensure all outbound call sites pass
assignment_sourceconsistently. —sync_group_assignee_outbound(..., assignment_source: AssignmentSource | None = None)defaults toNone, and the outbound task’s cycle prevention depends on it. Search for all invocations ofsync_group_assignee_outboundand ensure they passAssignmentSource.from_integration(integration)when an integration context exists; otherwise cycles may still occur. Add a test that simulates an outbound sync withassignment_source=Noneand asserts the expected behavior (either allowed or blocked) is intentional.
Suggestions
- [src/sentry/integrations/tasks/sync_assignee_outbound.py:L24-L60] WARNING:
assignment_source_dictis typed asdict[str, Any]but there’s no validation that it matches the expected schema;from_dict()returnsNoneand the code proceeds. — Add explicit schema checks and/or fail-closed behavior (see key finding) and add a unit test for malformed payloads (missingintegration_id, wrong types,queuedas string) to ensure cycle prevention remains correct. - [src/sentry/integrations/utils/sync.py:L104-L134] INFO: Ensure all outbound call sites pass
assignment_sourceconsistently. —sync_group_assignee_outbound(..., assignment_source: AssignmentSource | None = None)defaults toNone, and the outbound task’s cycle prevention depends on it. Search for all invocations ofsync_group_assignee_outboundand ensure they passAssignmentSource.from_integration(integration)when an integration context exists; otherwise cycles may still occur. Add a test that simulates an outbound sync withassignment_source=Noneand asserts the expected behavior (either allowed or blocked) is intentional.
Posted by re-entry.ai · Risk governance for autonomous engineering teams
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🔴 Risk Score: 75/100 · HIGH
| Dimension | Level |
|---|---|
| Likelihood | 🔴 Critical |
| Impact | 🔴 Critical |
| Detectability | 🟠 High |
Intent
Add cross-system issue/assignee synchronization with metadata-based cycle prevention so outbound syncs don’t re-trigger inbound syncs across integrations.
Summary
The PR introduces an AssignmentSource payload that is serialized into outbound assignee sync tasks and threaded through should_sync to prevent sync cycles when the source integration matches the current integration. It also extends GroupAssignee.assign/deassign and sync utilities to accept and propagate assignment_source. The top risks are (1) a serialization contract bug: AssignmentSource.queued is a datetime default that is not safely serialized/deserialized across task boundaries, and (2) cycle-prevention correctness: should_sync now depends on sync_source being passed consistently, but malformed/None payloads silently disable the guard. Verify that the task payload serialization round-trips correctly and that all call sites pass the new argument consistently so cycle prevention is reliable.
🎯 Review Focus
The serialization/deserialization contract for AssignmentSource across the async task boundary (to_dict()/from_dict() and queued handling) and whether cycle-prevention remains reliable when payloads are missing or malformed.
Key Findings
- 🚨 [src/sentry/integrations/services/assignment_source.py:L14-L35] CRITICAL:
queuedis adatetimedefault butto_dict()returns rawdatetimeobjects andfrom_dict()blindlycls(**input_dict); this will break task serialization and/or produce inconsistent values. —queued: datetime = timezone.now()is evaluated at import time (dataclass default), andasdict(self)will include adatetimeobject. If the task queue expects JSON-serializable payloads, this will either fail at enqueue time or deserialize incorrectly, and becausefrom_dict()catchesTypeError/ValueErrorand returnsNone, the cycle-prevention guard can be silently disabled. — Fix: makequeuedserialization explicit and ensure the default is per-instance. Example:
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
queued: datetime = field(default_factory=timezone.now)
def to_dict(self) -> dict[str, Any]:
return {
"source_name": self.source_name,
"integration_id": self.integration_id,
"queued": self.queued.isoformat(),
}
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
queued = input_dict.get("queued")
if isinstance(queued, str):
queued = datetime.fromisoformat(queued)
return cls(
source_name=input_dict["source_name"],
integration_id=int(input_dict["integration_id"]),
queued=queued or timezone.now(),
)
except (KeyError, ValueError, TypeError):
return NoneAlso add a test that enqueues sync_assignee_outbound with assignment_source_dict and asserts it round-trips.
- 🚨 [src/sentry/integrations/mixins/issues.py:L378-L408] WARNING: Cycle prevention is conditional on
sync_sourcebeing passed correctly; malformed/Noneassignment_source_dictdisables the guard and can re-propagate changes. —should_sync(..., sync_source)returnsFalseonly whensync_source.integration_id == self.org_integration.integration_id. ButAssignmentSource.from_dict()returnsNoneon any parsing issue, andsync_assignee_outboundpassesparsed_assignment_sourcedirectly. That means any serialization mismatch (see critical finding) or missing field will silently fall back toinstallation.should_sync(..., None)and allow outbound propagation, defeating the cycle-prevention intent. — Fix: treat parse failure as “do not sync” (fail closed) or at least log loudly. Example insync_assignee_outbound:
parsed = AssignmentSource.from_dict(assignment_source_dict) if assignment_source_dict else None
if assignment_source_dict and parsed is None:
logger.warning("invalid-assignment-source", extra={"assignment_source_dict": assignment_source_dict})
return # fail closed to preserve cycle preventionAlternatively, change from_dict to raise and handle explicitly at the task boundary so you don’t silently disable the guard.
✅ Action Checklist
- [ ] CRITICAL [src/sentry/integrations/services/assignment_source.py:L14-L35] —
queuedis adatetimedefault butto_dict()returns rawdatetimeobjects andfrom_dict()blindlycls(**input_dict); this will break task serialization and/or produce inconsistent values. —queued: datetime = timezone.now()is evaluated at import time (dataclass default), andasdict(self)will include adatetimeobject. If the task queue expects JSON-serializable payloads, this will either fail at enqueue time or deserialize incorrectly, and becausefrom_dict()catchesTypeError/ValueErrorand returnsNone, the cycle-prevention guard can be silently disabled. — Fix: makequeuedserialization explicit and ensure the default is per-instance. Example:
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
queued: datetime = field(default_factory=timezone.now)
def to_dict(self) -> dict[str, Any]:
return {
"source_name": self.source_name,
"integration_id": self.integration_id,
"queued": self.queued.isoformat(),
}
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource | None:
try:
queued = input_dict.get("queued")
if isinstance(queued, str):
queued = datetime.fromisoformat(queued)
return cls(
source_name=input_dict["source_name"],
integration_id=int(input_dict["integration_id"]),
queued=queued or timezone.now(),
)
except (KeyError, ValueError, TypeError):
return NoneAlso add a test that enqueues sync_assignee_outbound with assignment_source_dict and asserts it round-trips.
- [ ] WARNING [src/sentry/integrations/mixins/issues.py:L378-L408] — Cycle prevention is conditional on
sync_sourcebeing passed correctly; malformed/Noneassignment_source_dictdisables the guard and can re-propagate changes. —should_sync(..., sync_source)returnsFalseonly whensync_source.integration_id == self.org_integration.integration_id. ButAssignmentSource.from_dict()returnsNoneon any parsing issue, andsync_assignee_outboundpassesparsed_assignment_sourcedirectly. That means any serialization mismatch (see critical finding) or missing field will silently fall back toinstallation.should_sync(..., None)and allow outbound propagation, defeating the cycle-prevention intent. — Fix: treat parse failure as “do not sync” (fail closed) or at least log loudly. Example insync_assignee_outbound:
parsed = AssignmentSource.from_dict(assignment_source_dict) if assignment_source_dict else None
if assignment_source_dict and parsed is None:
logger.warning("invalid-assignment-source", extra={"assignment_source_dict": assignment_source_dict})
return # fail closed to preserve cycle preventionAlternatively, change from_dict to raise and handle explicitly at the task boundary so you don’t silently disable the guard.
- [ ] SUGGESTION — [src/sentry/integrations/tasks/sync_assignee_outbound.py:L24-L60] WARNING:
assignment_source_dictis typed asdict[str, Any]but there’s no validation that it matches the expected schema;from_dict()returnsNoneand the code proceeds. — Add explicit schema checks and/or fail-closed behavior (see key finding) and add a unit test for malformed payloads (missingintegration_id, wrong types,queuedas string) to ensure cycle prevention remains correct. - [ ] SUGGESTION — [src/sentry/integrations/utils/sync.py:L104-L134] INFO: Ensure all outbound call sites pass
assignment_sourceconsistently. —sync_group_assignee_outbound(..., assignment_source: AssignmentSource | None = None)defaults toNone, and the outbound task’s cycle prevention depends on it. Search for all invocations ofsync_group_assignee_outboundand ensure they passAssignmentSource.from_integration(integration)when an integration context exists; otherwise cycles may still occur. Add a test that simulates an outbound sync withassignment_source=Noneand asserts the expected behavior (either allowed or blocked) is intentional.
Suggestions
- [src/sentry/integrations/tasks/sync_assignee_outbound.py:L24-L60] WARNING:
assignment_source_dictis typed asdict[str, Any]but there’s no validation that it matches the expected schema;from_dict()returnsNoneand the code proceeds. — Add explicit schema checks and/or fail-closed behavior (see key finding) and add a unit test for malformed payloads (missingintegration_id, wrong types,queuedas string) to ensure cycle prevention remains correct. - [src/sentry/integrations/utils/sync.py:L104-L134] INFO: Ensure all outbound call sites pass
assignment_sourceconsistently. —sync_group_assignee_outbound(..., assignment_source: AssignmentSource | None = None)defaults toNone, and the outbound task’s cycle prevention depends on it. Search for all invocations ofsync_group_assignee_outboundand ensure they passAssignmentSource.from_integration(integration)when an integration context exists; otherwise cycles may still occur. Add a test that simulates an outbound sync withassignment_source=Noneand asserts the expected behavior (either allowed or blocked) is intentional.
Posted by re-entry.ai · Risk governance for autonomous engineering teams
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
🔴 Risk Score: 62/100 · HIGH
| Dimension | Level |
|---|---|
| Likelihood | 🟠 High |
| Impact | 🟠 High |
| Detectability | 🟡 Medium |
Intent
Implement cross-system issue synchronization for assignees while preventing sync cycles between integrations.
Summary
This PR threads a new AssignmentSource payload through inbound and outbound assignee sync paths and uses it in IssueSyncIntegration.should_sync to prevent sync cycles. The biggest correctness/security risk is that AssignmentSource.queued is defined with timezone.now() at import time, so every instance created without an explicit queued value shares the same timestamp. There is also an API-breaking change to IssueSyncIntegration.should_sync (and likely sync_status_outbound) that can break existing integration subclasses if they weren’t updated in lockstep. Finally, outbound async task parsing uses best-effort deserialization that can silently disable cycle prevention when payloads are malformed; you need to verify the failure mode is safe and observable.
🎯 Review Focus
Verify the cycle-prevention logic is correct and safe under malformed/missing async payloads—specifically, ensure AssignmentSource.queued is per-instance (not import-time) and that sync_assignee_outbound does not silently disable cycle prevention when parsing fails.
Key Findings
- 🚨 [src/sentry/integrations/mixins/issues.py:L408] CRITICAL:
def should_sync(self, attribute: str, sync_source: AssignmentSource | None = None) -> bool:andif sync_source and sync_source.integration_id == self.org_integration.integration_id: return False— This cycle-prevention relies entirely on sync_source.integration_id being correct; if the queued payload is missing/malformed (see task parsing) or integration_id is wrong, you can suppress legitimate outbound syncs or fail to prevent cycles. Because this is used to gate outbound propagation, it can cause persistent sync divergence between systems — Fix: make the cycle check more robust by including both source_name and integration_id and validating them against the installation/integration context. Concretely: (1) extend AssignmentSource to includesource_nameand validate both fields, (2) in sync_assignee_outbound, if from_dict returns None, do NOT silently disable cycle prevention—either reject the task (raise) or log+fallback to a safe default that still prevents cycles (e.g., treat unknown payload as “do not sync outbound” for the same integration).
✅ Action Checklist
- [ ] CRITICAL [src/sentry/integrations/mixins/issues.py:L408] —
def should_sync(self, attribute: str, sync_source: AssignmentSource | None = None) -> bool:andif sync_source and sync_source.integration_id == self.org_integration.integration_id: return False— This cycle-prevention relies entirely on sync_source.integration_id being correct; if the queued payload is missing/malformed (see task parsing) or integration_id is wrong, you can suppress legitimate outbound syncs or fail to prevent cycles. Because this is used to gate outbound propagation, it can cause persistent sync divergence between systems — Fix: make the cycle check more robust by including both source_name and integration_id and validating them against the installation/integration context. Concretely: (1) extend AssignmentSource to includesource_nameand validate both fields, (2) in sync_assignee_outbound, if from_dict returns None, do NOT silently disable cycle prevention—either reject the task (raise) or log+fallback to a safe default that still prevents cycles (e.g., treat unknown payload as “do not sync outbound” for the same integration). - [ ] SUGGESTION — Fix queued default to be per-instance: [src/sentry/integrations/services/assignment_source.py:L18] change to
from dataclasses import asdict, dataclass, fieldandqueued: datetime = field(default_factory=timezone.now). - [ ] SUGGESTION — Make task boundary parsing strict and observable: [src/sentry/integrations/tasks/sync_assignee_outbound.py:L27] replace best-effort parsing with validation + logging. Example:
parsed = AssignmentSource.from_dict(assignment_source_dict or {}) ; if parsed is None: logger.error("invalid_assignment_source", extra={"assignment_source_dict": assignment_source_dict}); return(or raise to fail the task depending on desired safety). - [ ] SUGGESTION — Update/verify all should_sync call sites and subclasses for the new signature: [src/sentry/integrations/mixins/issues.py:L408] search for
should_sync(overrides/calls and ensure every override acceptssync_source(or provides a compatible default). If you have third-party/older subclasses, add an adapter in the base class (e.g., accept *args/**kwargs and normalize) to avoid runtime TypeError. - [ ] SUGGESTION — Add edge-case tests for AssignmentSource.from_dict beyond empty/invalid keys: [tests/sentry/integrations/services/test_assignment_source.py] include cases like missing integration_id, wrong types (string for integration_id), and queued present but invalid type, and assert the task behavior (fail/skip) matches the intended safety model.
Suggestions
- Fix queued default to be per-instance: [src/sentry/integrations/services/assignment_source.py:L18] change to
from dataclasses import asdict, dataclass, fieldandqueued: datetime = field(default_factory=timezone.now). - Make task boundary parsing strict and observable: [src/sentry/integrations/tasks/sync_assignee_outbound.py:L27] replace best-effort parsing with validation + logging. Example:
parsed = AssignmentSource.from_dict(assignment_source_dict or {}) ; if parsed is None: logger.error("invalid_assignment_source", extra={"assignment_source_dict": assignment_source_dict}); return(or raise to fail the task depending on desired safety). - Update/verify all should_sync call sites and subclasses for the new signature: [src/sentry/integrations/mixins/issues.py:L408] search for
should_sync(overrides/calls and ensure every override acceptssync_source(or provides a compatible default). If you have third-party/older subclasses, add an adapter in the base class (e.g., accept *args/**kwargs and normalize) to avoid runtime TypeError. - Add edge-case tests for AssignmentSource.from_dict beyond empty/invalid keys: [tests/sentry/integrations/services/test_assignment_source.py] include cases like missing integration_id, wrong types (string for integration_id), and queued present but invalid type, and assert the task behavior (fail/skip) matches the intended safety model.
📝 This review includes 3 inline comments (2 warnings, 1 note)
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
This is a real bug: timezone.now() is evaluated once at import time, so every AssignmentSource created without an explicit queued will reuse the same timestamp. The root cause is using a mutable/default-value pattern for a runtime field; this should be a default_factory or equivalent per-instance initialization.
| @@ -400,7 +408,14 @@ def sync_assignee_outbound( | |||
| raise NotImplementedError | |||
There was a problem hiding this comment.
The abstract method signature changed to accept assignment_source, but the diff does not show any compatibility layer for existing subclasses. Any concrete integration that overrides sync_status_outbound with the old positional signature will now fail when called with the new keyword argument, so this is a real API-breaking change unless all implementations were updated in lockstep.
| @@ -24,7 +27,12 @@ | |||
| Organization.DoesNotExist, | |||
There was a problem hiding this comment.
💡 NOTE
Using dict[str, Any] | None here is not itself a correctness bug, but the task boundary is now accepting untyped serialized data from the queue. The real risk is that AssignmentSource.from_dict silently returns None on malformed payloads, which can disable cycle-prevention and make debugging payload/schema drift harder. Consider validating the schema explicitly instead of relying on Any plus best-effort parsing.
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🔴 Risk Score: 61/100 · HIGH
| Dimension | Level |
|---|---|
| Likelihood | 🟠 High |
| Impact | 🟠 High |
| Detectability | 🟡 Medium |
Intent
Propagate a cross-system “assignment source” through inbound/outbound assignee sync to prevent sync cycles and improve provenance.
Summary
The PR introduces a new AssignmentSource dataclass and threads it through group assignee inbound/outbound sync and the outbound async task, plus it updates integration should_sync() to suppress outbound propagation when the sync_source originates from the same integration. The highest risks are (1) a CRITICAL dataclass default timestamp bug that will cause all AssignmentSource instances to share the same queued time, and (2) CRITICAL runtime contract fragility where should_sync() is called with a new parameter that may not be implemented consistently across all integration installations. Reviewers must verify that all should_sync overrides accept the new signature and that AssignmentSource deserialization failures and None-handling cannot silently bypass the cycle-prevention logic.
🎯 Review Focus
Confirm runtime compatibility of the updated should_sync() signature across all integration installations/providers, and fix the AssignmentSource queued timestamp default_factory bug before relying on queued/provenance semantics.
✅ Action Checklist
- [ ] SUGGESTION — Verify and enforce the should_sync contract across the codebase: search for all
def should_sync(overrides and ensure they accept(attribute, sync_source: AssignmentSource | None = None)(or compatible*args/**kwargs). The immediate call site is [src/sentry/integrations/tasks/sync_assignee_outbound.py:L34]. - [ ] SUGGESTION — Add an end-to-end test that exercises the async task payload path: enqueue
sync_assignee_outbound.apply_asyncwithassignment_source_dictand assert that whenassignment_source_dict.integration_id == org_integration.id, outbound sync is suppressed. This specifically covers the integration between [src/sentry/integrations/utils/sync.py] and [src/sentry/integrations/tasks/sync_assignee_outbound.py]. - [ ] SUGGESTION — Harden payload validation: in [src/sentry/integrations/services/assignment_source.py:L30], include enough context in logs/metrics (e.g., keys present, integration_id/source_name types) and add a test for malformed
queued(string/epoch/None) to ensure behavior is deterministic and observable.
Suggestions
- Verify and enforce the should_sync contract across the codebase: search for all
def should_sync(overrides and ensure they accept(attribute, sync_source: AssignmentSource | None = None)(or compatible*args/**kwargs). The immediate call site is [src/sentry/integrations/tasks/sync_assignee_outbound.py:L34]. - Add an end-to-end test that exercises the async task payload path: enqueue
sync_assignee_outbound.apply_asyncwithassignment_source_dictand assert that whenassignment_source_dict.integration_id == org_integration.id, outbound sync is suppressed. This specifically covers the integration between [src/sentry/integrations/utils/sync.py] and [src/sentry/integrations/tasks/sync_assignee_outbound.py]. - Harden payload validation: in [src/sentry/integrations/services/assignment_source.py:L30], include enough context in logs/metrics (e.g., keys present, integration_id/source_name types) and add a test for malformed
queued(string/epoch/None) to ensure behavior is deterministic and observable.
📝 This review includes 3 inline comments (2 critical, 1 warning)
Posted by re-entry.ai · Risk governance for autonomous engineering teams
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class AssignmentSource: |
There was a problem hiding this comment.
🚨 CRITICAL
Quote: queued: datetime = timezone.now()
Issue: In a dataclass, default values are evaluated at class definition/import time, not per-instance. This means every AssignmentSource created without an explicit queued will share the same timestamp, breaking ordering/TTL logic and causing nondeterministic behavior across process restarts.
Fix: Use a default_factory so the timestamp is computed per instance:
from dataclasses import dataclass, field
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
queued: datetime = field(default_factory=timezone.now)| external_issue_id: int, | ||
| user_id: int | None, | ||
| assign: bool, | ||
| assignment_source_dict: dict[str, Any] | None = None, |
There was a problem hiding this comment.
🚨 CRITICAL
Quote: if installation.should_sync("outbound_assignee", parsed_assignment_source):
Issue: The call signature of installation.should_sync is changed to pass parsed_assignment_source. If should_sync implementations across different integration installations are not updated to accept the new second argument, this will raise TypeError at runtime for some providers.
Fix: Ensure backward compatibility at the call site or enforce a consistent interface. For example, call with keyword args if supported, or gate by introspection/adapter:
if installation.should_sync("outbound_assignee", parsed_assignment_source):
...But only after updating all implementations. Alternatively, provide a default wrapper in the integration base class so all installations accept the new parameter. (see also L34)
| def to_dict(self) -> dict[str, Any]: | ||
| return asdict(self) | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
Quote: except (ValueError, TypeError): return None
Issue: Deserialization failures are silently swallowed and converted into None. If callers don’t strictly check for None, this can lead to dropped sync events or partially missing assignment sources without any visibility.
Fix: Either (a) log/trace the failure with enough context, or (b) raise a typed exception so the caller can handle it explicitly. For example:
import logging
logger = logging.getLogger(__name__)
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource:
try:
return cls(**input_dict)
except (ValueError, TypeError) as e:
logger.warning("Invalid AssignmentSource payload: %s", input_dict, exc_info=e)
raise
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🟡 Risk Score: 50/100 · MEDIUM
| Dimension | Level |
|---|---|
| Likelihood | 🟠 High |
| Impact | 🟡 Medium |
| Detectability | 🟡 Medium |
Intent
Implement cross-system issue/assignee synchronization while preventing sync cycles by propagating an “assignment source” through the outbound sync pipeline.
Summary
The PR introduces an AssignmentSource dataclass, serializes it into outbound assignee sync task payloads, and threads it into IssueSyncIntegration.should_sync() to suppress outbound propagation when the source integration matches the current integration. The highest risks are (1) a CRITICAL dataclass default timestamp bug (queued is evaluated at import time), and (2) silent failure in AssignmentSource.from_dict() that can disable the cycle-prevention signal without any observability. You should verify that cycle prevention cannot be bypassed by malformed payloads, and that the async task signature changes are consistently handled across all call sites/workers.
🎯 Review Focus
Verify that sync-cycle prevention cannot be silently bypassed: specifically, ensure AssignmentSource.from_dict() failures are handled in a fail-closed/observable way and that every should_sync call site passes the new sync_source parameter consistently across inbound/outbound sync paths.
Key Findings
⚠️ [src/sentry/integrations/mixins/issues.py:L379] WARNING:IssueBasicIntegration.should_syncis changed to acceptsync_sourcebut the baseIssueBasicIntegration.should_sync(self, attribute, sync_source: AssignmentSource | None = None)returnsFalseunconditionally, andIssueSyncIntegration.should_syncadds cycle suppression based onsync_source.integration_id; fix by ensuring all concrete integrations overrideshould_synccorrectly and add repo-wide tests asserting cycle prevention works for every attribute that usesshould_sync.
✅ Action Checklist
- WARNING [src/sentry/integrations/mixins/issues.py:L379] —
IssueBasicIntegration.should_syncis changed to acceptsync_sourcebut the baseIssueBasicIntegration.should_sync(self, attribute, sync_source: AssignmentSource | None = None)returnsFalseunconditionally, andIssueSyncIntegration.should_syncadds cycle suppression based onsync_source.integration_id; fix by ensuring all concrete integrations overrideshould_synccorrectly and add repo-wide tests asserting cycle prevention works for every attribute that usesshould_sync. - SUGGESTION — Fix the dataclass default timestamp bug: in [src/sentry/integrations/services/assignment_source.py:L16], change
queued: datetime = timezone.now()toqueued: datetime = field(default_factory=timezone.now)(and importfield). - SUGGESTION — Make invalid payload handling observable and non-bypassable: in [src/sentry/integrations/services/assignment_source.py:L30], replace
return Nonewith either (a) logging + raising a domain exception, or (b) return a sentinel and have the task treat “invalid but provided” as a reason to skip sync (fail closed). Example:logger.warning(...); raise ValueError("Invalid AssignmentSource")and in [src/sentry/integrations/tasks/sync_assignee_outbound.py:L50] catch andreturnbefore callingshould_sync/sync_assignee_outbound. - SUGGESTION — Harden the async task contract: in [src/sentry/integrations/tasks/sync_assignee_outbound.py:L24-L60], ensure all callers pass
assignment_source_dictwith the exact schema expected byAssignmentSource.from_dict(and that workers running older code don’t crash). Concretely, add a defensive guard: ifassignment_source_dictis notNonebutfrom_dictreturnsNone, log andreturn(or skip cycle suppression) rather than silently proceeding. - SUGGESTION — Add edge-case tests for serialization: extend [tests/sentry/integrations/services/test_assignment_source.py:L1-L38] to assert
queuedis instance-specific (not constant across instances) and that timezone-aware datetimes round-trip as expected (or explicitly document thatqueuedis not serialized/used). Also fix the typo in the test nametest_from_dict_inalid_datatoinvalid.
Suggestions
- Fix the dataclass default timestamp bug: in [src/sentry/integrations/services/assignment_source.py:L16], change
queued: datetime = timezone.now()toqueued: datetime = field(default_factory=timezone.now)(and importfield). - Make invalid payload handling observable and non-bypassable: in [src/sentry/integrations/services/assignment_source.py:L30], replace
return Nonewith either (a) logging + raising a domain exception, or (b) return a sentinel and have the task treat “invalid but provided” as a reason to skip sync (fail closed). Example:logger.warning(...); raise ValueError("Invalid AssignmentSource")and in [src/sentry/integrations/tasks/sync_assignee_outbound.py:L50] catch andreturnbefore callingshould_sync/sync_assignee_outbound. - Harden the async task contract: in [src/sentry/integrations/tasks/sync_assignee_outbound.py:L24-L60], ensure all callers pass
assignment_source_dictwith the exact schema expected byAssignmentSource.from_dict(and that workers running older code don’t crash). Concretely, add a defensive guard: ifassignment_source_dictis notNonebutfrom_dictreturnsNone, log andreturn(or skip cycle suppression) rather than silently proceeding. - Add edge-case tests for serialization: extend [tests/sentry/integrations/services/test_assignment_source.py:L1-L38] to assert
queuedis instance-specific (not constant across instances) and that timezone-aware datetimes round-trip as expected (or explicitly document thatqueuedis not serialized/used). Also fix the typo in the test nametest_from_dict_inalid_datatoinvalid.
📝 This review includes 2 inline comments (1 critical, 1 warning)
Posted by re-entry.ai · Risk governance for autonomous engineering teams
|
|
||
| @dataclass(frozen=True) | ||
| class AssignmentSource: | ||
| source_name: str |
There was a problem hiding this comment.
🚨 CRITICAL
Issue: queued uses timezone.now() as a default value in the dataclass field: queued: datetime = timezone.now(). In Python, default values are evaluated at import/class definition time, so every instance created without an explicit queued will share the same timestamp.
Fix: Use default_factory so the timestamp is computed per instance.
from dataclasses import dataclass, field
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
queued: datetime = field(default_factory=timezone.now)| def to_dict(self) -> dict[str, Any]: | ||
| return asdict(self) | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
Issue: Deserialization swallows all ValueError/TypeError and returns None:
except (ValueError, TypeError): return None
This can silently drop/ignore bad or tampered payloads, making debugging and data integrity harder and potentially causing downstream logic to treat missing sources as legitimate.
Fix: Either (a) log/record the failure with enough context, or (b) raise a domain-specific exception so callers can handle it explicitly.
Example:
import logging
logger = logging.getLogger(__name__)
@classmethod
def from_dict(cls, input_dict: dict[str, Any]) -> AssignmentSource:
try:
return cls(**input_dict)
except (ValueError, TypeError) as e:
logger.warning("Invalid AssignmentSource payload: %s", input_dict, exc_info=e)
raise![re-entry-local[bot]](https://avatars.githubusercontent.com/in/3584620?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🔴 Risk Score: 60/100 · HIGH
| Dimension | Level |
|---|---|
| Likelihood | 🟠 High |
| Impact | 🟠 High |
| Detectability | 🟡 Medium |
Intent
Add an assignment_source concept to cross-system issue assignee synchronization to prevent sync cycles and propagate attribution across inbound/outbound flows.
Summary
This PR threads a new assignment_source payload through inbound/outbound group assignee sync and into integration should_sync to prevent sync cycles. The main behavioral risk is that cycle-prevention depends on correct parsing/propagation of assignment_source_dict; if parsing fails or timestamps are wrong, legitimate syncs may be suppressed or activity attribution may be inconsistent. There are also correctness and observability gaps: AssignmentSource.queued is currently defined with a default evaluated at import time, and AssignmentSource.from_dict silently returns None on invalid input, which can disable cycle prevention without any logging/metrics. Verify that both assign and deassign paths update activity consistently with assignment_source, and that invalid/malformed task payloads do not silently change sync behavior.
🎯 Review Focus
The correctness and observability of AssignmentSource parsing/propagation: confirm that invalid assignment_source_dict cannot silently disable sync-cycle prevention, and that both assign and deassign paths consistently apply assignment_source to activity/state updates.
Key Findings
⚠️ [src/sentry/integrations/mixins/issues.py:L379] WARNING:should_syncnow takessync_sourceand blocks whensync_source.integration_id == self.org_integration.integration_id, but the baseIssueBasicIntegration.should_syncsignature changed to acceptsync_sourcewhile still returningFalseby default; ensure all concrete integrations override the updated signature and that callers always pass the correctAssignmentSource(otherwise Python will raise at runtime or silently skip cycle prevention).
✅ Action Checklist
- WARNING [src/sentry/integrations/mixins/issues.py:L379] —
should_syncnow takessync_sourceand blocks whensync_source.integration_id == self.org_integration.integration_id, but the baseIssueBasicIntegration.should_syncsignature changed to acceptsync_sourcewhile still returningFalseby default; ensure all concrete integrations override the updated signature and that callers always pass the correctAssignmentSource(otherwise Python will raise at runtime or silently skip cycle prevention). - SUGGESTION — Fix the dataclass default timestamp: in
src/sentry/integrations/services/assignment_source.py, changequeued: datetime = timezone.now()tofrom dataclasses import fieldandqueued: datetime = field(default_factory=timezone.now). - SUGGESTION — Add observability for malformed payloads: in
src/sentry/integrations/services/assignment_source.py, updatefrom_dictto log/metric on invalidinput_dict(e.g.,logger.warning('assignment_source_invalid', extra={'keys': list(input_dict.keys())})) before returningNone. If you have a metrics helper, increment a counter likeintegration.assignment_source.invalid. - SUGGESTION — Harden task parsing and behavior: in
src/sentry/integrations/tasks/sync_assignee_outbound.py, whenAssignmentSource.from_dictreturnsNone, explicitly log that cycle-prevention is being skipped for this task (includeexternal_issue_id,integration_idif available, and whetherassignment_source_dictwas present). - SUGGESTION — Verify deassign/assign activity consistency: in
src/sentry/integrations/utils/sync.py, inbound deassign now callsGroupAssignee.objects.deassign(group, assignment_source=AssignmentSource.from_integration(integration))and assign does the same; add/extend tests to assert both assign and deassign paths record activity/history with the expectedassignment_sourcefields (not just assign). - SUGGESTION — Update/extend tests to cover failure modes: in
tests/sentry/integrations/services/test_assignment_source.py, add cases for missing required keys (integration_idabsent), wrong types (integration_idas string), and ensurefrom_dictreturnsNonewhile also asserting the new logging/metrics behavior (once added).
Suggestions
- Fix the dataclass default timestamp: in
src/sentry/integrations/services/assignment_source.py, changequeued: datetime = timezone.now()tofrom dataclasses import fieldandqueued: datetime = field(default_factory=timezone.now). - Add observability for malformed payloads: in
src/sentry/integrations/services/assignment_source.py, updatefrom_dictto log/metric on invalidinput_dict(e.g.,logger.warning('assignment_source_invalid', extra={'keys': list(input_dict.keys())})) before returningNone. If you have a metrics helper, increment a counter likeintegration.assignment_source.invalid. - Harden task parsing and behavior: in
src/sentry/integrations/tasks/sync_assignee_outbound.py, whenAssignmentSource.from_dictreturnsNone, explicitly log that cycle-prevention is being skipped for this task (includeexternal_issue_id,integration_idif available, and whetherassignment_source_dictwas present). - Verify deassign/assign activity consistency: in
src/sentry/integrations/utils/sync.py, inbound deassign now callsGroupAssignee.objects.deassign(group, assignment_source=AssignmentSource.from_integration(integration))and assign does the same; add/extend tests to assert both assign and deassign paths record activity/history with the expectedassignment_sourcefields (not just assign). - Update/extend tests to cover failure modes: in
tests/sentry/integrations/services/test_assignment_source.py, add cases for missing required keys (integration_idabsent), wrong types (integration_idas string), and ensurefrom_dictreturnsNonewhile also asserting the new logging/metrics behavior (once added).
📝 This review includes 3 inline comments (3 warnings)
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
Potential correctness issue: AssignmentSource.queued uses timezone.now() as a default value in the dataclass field, which is evaluated at import time rather than per-instance. This can cause all instances to share the same queued timestamp; fix by using field(default_factory=timezone.now).
| @@ -1,6 +1,9 @@ | |||
| from typing import Any | |||
There was a problem hiding this comment.
The outbound sync task has no explicit job-level success/failure logging or metrics around the main sync path (after parsing assignment_source_dict and calling installation.sync_assignee_outbound). Add structured logs/metrics (e.g., increment a counter and record latency) keyed by integration_id, external_issue_id, assign, and whether parsed_assignment_source was present; also log when installation.should_sync(...) returns false to avoid silent no-ops.
| @@ -0,0 +1,35 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
AssignmentSource.from_dict silently swallows invalid payloads by returning None on ValueError/TypeError, which can lead to sync-cycle prevention being skipped without any observability. Add a warning/info log (with safe context like integration_id if available) and/or a metric counter for invalid assignment_source_dict inputs.
Also affects 1 other file:
• src/sentry/integrations/services/assignment_source.py:27
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🔴 Risk Score: 60/100 · HIGH
| Dimension | Level |
|---|---|
| Likelihood | 🟠 High |
| Impact | 🟠 High |
| Detectability | 🟡 Medium |
Intent
Add an assignment_source concept to cross-system issue assignee synchronization to prevent sync cycles and propagate attribution across inbound/outbound flows.
Summary
This PR threads a new assignment_source payload through inbound/outbound group assignee sync and into integration should_sync to prevent sync cycles. The main behavioral risk is that cycle-prevention depends on correct parsing/propagation of assignment_source_dict; if parsing fails or timestamps are wrong, legitimate syncs may be suppressed or activity attribution may be inconsistent. There are also correctness and observability gaps: AssignmentSource.queued is currently defined with a default evaluated at import time, and AssignmentSource.from_dict silently returns None on invalid input, which can disable cycle prevention without any logging/metrics. Verify that both assign and deassign paths update activity consistently with assignment_source, and that invalid/malformed task payloads do not silently change sync behavior.
🎯 Review Focus
The correctness and observability of AssignmentSource parsing/propagation: confirm that invalid assignment_source_dict cannot silently disable sync-cycle prevention, and that both assign and deassign paths consistently apply assignment_source to activity/state updates.
Key Findings
⚠️ [src/sentry/integrations/mixins/issues.py:L379] WARNING:should_syncnow takessync_sourceand blocks whensync_source.integration_id == self.org_integration.integration_id, but the baseIssueBasicIntegration.should_syncsignature changed to acceptsync_sourcewhile still returningFalseby default; ensure all concrete integrations override the updated signature and that callers always pass the correctAssignmentSource(otherwise Python will raise at runtime or silently skip cycle prevention).
✅ Action Checklist
- WARNING [src/sentry/integrations/mixins/issues.py:L379] —
should_syncnow takessync_sourceand blocks whensync_source.integration_id == self.org_integration.integration_id, but the baseIssueBasicIntegration.should_syncsignature changed to acceptsync_sourcewhile still returningFalseby default; ensure all concrete integrations override the updated signature and that callers always pass the correctAssignmentSource(otherwise Python will raise at runtime or silently skip cycle prevention). - SUGGESTION — Fix the dataclass default timestamp: in
src/sentry/integrations/services/assignment_source.py, changequeued: datetime = timezone.now()tofrom dataclasses import fieldandqueued: datetime = field(default_factory=timezone.now). - SUGGESTION — Add observability for malformed payloads: in
src/sentry/integrations/services/assignment_source.py, updatefrom_dictto log/metric on invalidinput_dict(e.g.,logger.warning('assignment_source_invalid', extra={'keys': list(input_dict.keys())})) before returningNone. If you have a metrics helper, increment a counter likeintegration.assignment_source.invalid. - SUGGESTION — Harden task parsing and behavior: in
src/sentry/integrations/tasks/sync_assignee_outbound.py, whenAssignmentSource.from_dictreturnsNone, explicitly log that cycle-prevention is being skipped for this task (includeexternal_issue_id,integration_idif available, and whetherassignment_source_dictwas present). - SUGGESTION — Verify deassign/assign activity consistency: in
src/sentry/integrations/utils/sync.py, inbound deassign now callsGroupAssignee.objects.deassign(group, assignment_source=AssignmentSource.from_integration(integration))and assign does the same; add/extend tests to assert both assign and deassign paths record activity/history with the expectedassignment_sourcefields (not just assign). - SUGGESTION — Update/extend tests to cover failure modes: in
tests/sentry/integrations/services/test_assignment_source.py, add cases for missing required keys (integration_idabsent), wrong types (integration_idas string), and ensurefrom_dictreturnsNonewhile also asserting the new logging/metrics behavior (once added).
Suggestions
- Fix the dataclass default timestamp: in
src/sentry/integrations/services/assignment_source.py, changequeued: datetime = timezone.now()tofrom dataclasses import fieldandqueued: datetime = field(default_factory=timezone.now). - Add observability for malformed payloads: in
src/sentry/integrations/services/assignment_source.py, updatefrom_dictto log/metric on invalidinput_dict(e.g.,logger.warning('assignment_source_invalid', extra={'keys': list(input_dict.keys())})) before returningNone. If you have a metrics helper, increment a counter likeintegration.assignment_source.invalid. - Harden task parsing and behavior: in
src/sentry/integrations/tasks/sync_assignee_outbound.py, whenAssignmentSource.from_dictreturnsNone, explicitly log that cycle-prevention is being skipped for this task (includeexternal_issue_id,integration_idif available, and whetherassignment_source_dictwas present). - Verify deassign/assign activity consistency: in
src/sentry/integrations/utils/sync.py, inbound deassign now callsGroupAssignee.objects.deassign(group, assignment_source=AssignmentSource.from_integration(integration))and assign does the same; add/extend tests to assert both assign and deassign paths record activity/history with the expectedassignment_sourcefields (not just assign). - Update/extend tests to cover failure modes: in
tests/sentry/integrations/services/test_assignment_source.py, add cases for missing required keys (integration_idabsent), wrong types (integration_idas string), and ensurefrom_dictreturnsNonewhile also asserting the new logging/metrics behavior (once added).
📝 This review includes 3 inline comments (3 warnings)
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
Potential correctness issue: AssignmentSource.queued uses timezone.now() as a default value in the dataclass field, which is evaluated at import time rather than per-instance. This can cause all instances to share the same queued timestamp; fix by using field(default_factory=timezone.now).
| @@ -1,6 +1,9 @@ | |||
| from typing import Any | |||
There was a problem hiding this comment.
The outbound sync task has no explicit job-level success/failure logging or metrics around the main sync path (after parsing assignment_source_dict and calling installation.sync_assignee_outbound). Add structured logs/metrics (e.g., increment a counter and record latency) keyed by integration_id, external_issue_id, assign, and whether parsed_assignment_source was present; also log when installation.should_sync(...) returns false to avoid silent no-ops.
| @@ -0,0 +1,35 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
AssignmentSource.from_dict silently swallows invalid payloads by returning None on ValueError/TypeError, which can lead to sync-cycle prevention being skipped without any observability. Add a warning/info log (with safe context like integration_id if available) and/or a metric counter for invalid assignment_source_dict inputs.
Also affects 1 other file:
• src/sentry/integrations/services/assignment_source.py:27
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/sentry/integrations/services/assignment_source.py">
<violation number="1" location="src/sentry/integrations/services/assignment_source.py:18">
P1: `timezone.now()` as a default value for a dataclass field is evaluated once at class definition time, not per-instance. This causes all `AssignmentSource` instances created without an explicit `queued` to share the same (stale) timestamp, defeating the purpose of tracking when each assignment was queued.</violation>
</file>
<file name="src/sentry/integrations/mixins/issues.py">
<violation number="1" location="src/sentry/integrations/mixins/issues.py:411">
P2: Outbound status sync does not pass source context, so the new sync-cycle prevention check is bypassed for status updates.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| class AssignmentSource: | ||
| source_name: str | ||
| integration_id: int | ||
| queued: datetime = timezone.now() |
There was a problem hiding this comment.
P1: timezone.now() as a default value for a dataclass field is evaluated once at class definition time, not per-instance. This causes all AssignmentSource instances created without an explicit queued to share the same (stale) timestamp, defeating the purpose of tracking when each assignment was queued.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/sentry/integrations/services/assignment_source.py, line 18:
<comment>`timezone.now()` as a default value for a dataclass field is evaluated once at class definition time, not per-instance. This causes all `AssignmentSource` instances created without an explicit `queued` to share the same (stale) timestamp, defeating the purpose of tracking when each assignment was queued.</comment>
<file context>
@@ -0,0 +1,35 @@
+class AssignmentSource:
+ source_name: str
+ integration_id: int
+ queued: datetime = timezone.now()
+
+ @classmethod
</file context>
|
|
||
| @abstractmethod | ||
| def sync_status_outbound(self, external_issue, is_resolved, project_id, **kwargs): | ||
| def sync_status_outbound( |
There was a problem hiding this comment.
P2: Outbound status sync does not pass source context, so the new sync-cycle prevention check is bypassed for status updates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/sentry/integrations/mixins/issues.py, line 411:
<comment>Outbound status sync does not pass source context, so the new sync-cycle prevention check is bypassed for status updates.</comment>
<file context>
@@ -400,7 +408,14 @@ def sync_assignee_outbound(
@abstractmethod
- def sync_status_outbound(self, external_issue, is_resolved, project_id, **kwargs):
+ def sync_status_outbound(
+ self,
+ external_issue,
</file context>
2 participants
Martian Code Review Benchmark PR (mirrored from source #7)