Sentry / PR #7

feat(ecosystem): Cross-system issue synchronization

getsentry/sentry#77754
Golden recall: 100% Extra findings: 8

Golden Comments (4/4 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Shared mutable default in dataclass timestamp MEDIUM ✓ Found FriendlyReviewer pinpoints the shared mutability: queued: datetime = timezone.now() evaluated at import time, suggests field(default_factory=timezone.now).
#2 Typo: test_from_dict_inalid_data should be invalid_data LOW ✓ Found FriendlyReviewer caught the typo explicitly.
#3 test_from_dict_empty_array but tests empty dict LOW ✓ Found FriendlyReviewer flags: "MISLEADING NAME: tests an empty dict, not an array".
#4 to_dict() returns datetime for queued — JSON serialization failure in Celery MEDIUM ✓ Found FriendlyReviewer confirms: Celery will crash with TypeError on serialization, proposes converting to ISO string. Also flags the round-trip: if to_dict() is fixed, from_dict() must accept both datetimes and strings.

Supplementary Findings (9 findings)

Finding File Severity Legitimate?
from_dict() silently returns None → disables anti-cycle protection assignment_source.py:33 HIGH ✓ Real bug — no logging, silent feature disable
from_dict() rejects unknown keys → breaks forward compatibility assignment_source.py:33 MEDIUM ✓ Real risk
Round-trip broken: if to_dict() is fixed to output ISO strings, from_dict() expects datetime and will fail assignment_source.py:31 HIGH ✓ Serialization consistency must be maintained
Inconsistency: sync_assignee_outbound uses **kwargs for assignment_source, sync_status_outbound has explicit parameter issues.py:397 MEDIUM ✓ Maintainability issue
where_should_sync calls should_sync(key) without sync_source — limited cycle prevention sync.py:43 LOW ✓ Scope limitation
No test for from_integration() — the main creation path test_assignment_source.py:1 MEDIUM ✓ Missing test coverage
No round-trip test to_dict() → from_dict() — currently broken test_assignment_source.py:1 MEDIUM ✓ Golden bug #4 would be detected
No test for from_dict with extra keys or queued as string test_assignment_source.py:1 MEDIUM ✓ Missing edge-case coverage
No cycle prevention test on deassign path test_groupassignee.py:218 MEDIUM ✓ Missing test coverage

Honest Analysis

FriendlyReviewer detected all 4 golden bugs in this PR — a clean sweep. The shared mutable default (#1) and the datetime serialization crash in Celery (#4) were both upgraded from Medium to CRITICAL, which is the right call: these are runtime failures, not architectural concerns. The typo (#2) and misleading test name (#3) were caught as trivial findings, correctly classified as LOW.

The strongest finding is from_dict() silently returning None — a critical bug that the golden data set didn't include. On any deserialization error, cycle prevention is silently disabled with no logging, which can lead to infinite sync loops in production. FriendlyReviewer flagged this as CRITICAL in the "These might need a close look" section, and it's arguably the most impactful finding on the entire PR.

The round-trip inconsistency between to_dict() and from_dict() is well spotted: fixing one direction without the other leaves the code broken. The missing edge-case test coverage (from_dict with extra keys or string input) is a logical gap that follows from the serialization bugs, though aggregating three separate "missing test" entries on the same file is a bit repetitive.

The where_should_sync finding (calling the old signature without sync_source) is borderline — it's correct technically but the scope limitation is already visible from the code structure. It's included as acceptable noise from a reviewer that casts a wide net.

One pattern worth noting: FriendlyReviewer split the datetime serialization bug into two separate CRITICAL findings (the to_dict output issue and the from_dict input issue) when they're really two sides of the same round-trip bug. In the golden table, they're merged into a single entry (#4). This is classic FriendlyReviewer behavior: it enumerates every call site independently rather than deduplicating at the bug level, which inflates finding counts but doesn't miss edge cases.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
4 high, 6 medium, 3 low

The merge request introduces an AssignmentSource dataclass and a sync cycle prevention mechanism for issue assignee sync. The core design is sound, but several critical issues exist, primarily in the AssignmentSource serialization and deserialization logic, which will cause runtime failures and silent data loss. Additionally, the abstract method signatures are inconsistent, and test coverage is incomplete. The following technical findings address these issues, with priority on fixing the serialization bugs and adding proper error logging.

These might need a close look

  • 🔴 src/sentry/integrations/services/assignment_source.py (L17)
    CRITICAL: queued: datetime = timezone.now() evaluates the timestamp once at module import time. All instances without an explicit queued will share the same timestamp. Use field(default_factory=timezone.now).
  • 🔴 src/sentry/integrations/services/assignment_source.py (L28)
    CRITICAL: to_dict() returns a dict with raw datetime objects via asdict(), which is not JSON-serializable. Celery will crash when serializing this dict. Either convert queued to an ISO string in to_dict() or store it as a string internally.
  • 🔴 src/sentry/integrations/services/assignment_source.py (L31)
    CRITICAL: from_dict() passes the queued value directly to the constructor, expecting a datetime. If to_dict() is fixed to output an ISO string, this will break. The round-trip must be consistent (e.g., both use ISO strings).
  • 🔴 src/sentry/integrations/services/assignment_source.py (L33)
    CRITICAL: from_dict silently returns None on any deserialization failure, including corrupted queued datetime. This disables cycle prevention and can lead to infinite sync loops. Add logging before returning None.

Worth checking

  • 🟡 src/sentry/integrations/services/assignment_source.py (L33)
    RESILIENCE: from_dict rejects extra keys, breaking forward compatibility. Filter input_dict to only known fields before constructing the dataclass.
  • 🟡 src/sentry/integrations/mixins/issues.py (L397)
    Inconsistency: sync_assignee_outbound accepts assignment_source via **kwargs: Any, while sync_status_outbound has it as an explicit parameter. Align to the same pattern for type safety and discoverability.
  • 🟡 tests/sentry/integrations/services/test_assignment_source.py (L1)
    MISSING COVERAGE: No test for from_integration() factory method, which is the primary creation path.
  • 🟡 tests/sentry/integrations/services/test_assignment_source.py (L1)
    MISSING COVERAGE: No round‑trip test (to_dict() → from_dict()), which currently fails due to the datetime serialization bug.
  • 🟡 tests/sentry/integrations/services/test_assignment_source.py (L1)
    MISSING COVERAGE: No test for from_dict with extra keys or with queued as a string.
  • 🟡 tests/sentry/models/test_groupassignee.py (L218)
    MISSING COVERAGE: No test for cycle prevention on the deassign path.

Small things (take or leave)

  • 🔵 src/sentry/integrations/utils/sync.py (L43)
    where_should_sync calls installation.should_sync(key) without passing the new sync_source argument, limiting cycle‑prevention reach.
  • 🔵 tests/sentry/integrations/services/test_assignment_source.py (L12)
    TYPO: test_from_dict_inalid_data should be test_from_dict_invalid_data.
  • 🔵 tests/sentry/integrations/services/test_assignment_source.py (L8)
    MISLEADING NAME: test_from_dict_empty_array tests an empty dict, not an array.