Skip to content

feat(ecosystem): Implement cross-system issue synchronization#11

Open
zaibkhan wants to merge 1 commit into
ecosystem-sync-integration-beforefrom
ecosystem-sync-integration-after
Open

feat(ecosystem): Implement cross-system issue synchronization#11
zaibkhan wants to merge 1 commit into
ecosystem-sync-integration-beforefrom
ecosystem-sync-integration-after

Conversation

@zaibkhan

@zaibkhan zaibkhan commented Sep 9, 2025

Copy link
Copy Markdown

Mirrors ai-code-review-evaluation#7 for like-for-like benchmarking.

  • Base: ecosystem-sync-integration-before
  • Head: ecosystem-sync-integration-after

Original PR excerpt:

Test 7

@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 9, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Fix assignment source timestamp, guard API compatibility
What’s good: Prevents assignee sync cycles by passing assignment_source and filtering in should_sync, and adds targeted tests for both call and no-call cases.
Review Status: ❌ Requires changes
Overall Priority: High

Issues (Critical & High only)

Severity Issue Why it matters
High Correctness — Incorrect default timestamp; use default_factory …/services/assignment_source.py
timezone.now() is evaluated at import time, so all AssignmentSource instances created without an explicit queued will share the same timestamp. Use a default_factory to generate the value at instantiation.
High Compatibility — Backward-incompatible should_sync call may break integrat... …/tasks/sync_assignee_outbound.py
Passing a second argument to should_sync may raise TypeError for integrations that override should_sync with the legacy single-argument signature. Add a backward-compatible fallback.

Showing top 2 issues. Critical: 0, High: 2. See inline suggestions for more.

Key Feedback (click to expand)
  • Needs improvement: Use a default_factory for queued to avoid shared timestamps, and make the task's should_sync call backward-compatible with existing integrations.
  • Testing: Consider adding a test that exercises sync_assignee_outbound with a real assignment_source (non-None) to ensure the task runs successfully end-to-end, including serialization through apply_async. Also include a regression test for installations with legacy should_sync signatures by simulating a stub that only accepts one positional argument.
  • Documentation: Document the new AssignmentSource contract (required fields and optional queued) and the updated should_sync signature in the integration mixin to guide implementors.
  • Compatibility: Calling installation.should_sync with a second argument may break integrations that override should_sync with the legacy single-argument signature. Add a safe fallback to preserve backward compatibility.
  • Open questions: Do all existing integration installations override should_sync with the new signature? If Celery uses JSON, is queued's datetime in assignment_source_dict serializable in your deployment?

Confidence: 3/5 — Needs work before merge (2 high · status: Requires changes)

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Utils
    participant Task
    participant Installation
    Caller->>Utils: sync_group_assignee_outbound(group, user_id, assign, assignment_source)
    Utils->>Task: sync_assignee_outbound.apply_async(kwargs)
    Task->>Installation: should_sync("outbound_assignee", assignment_source?)
    alt should_sync returns true
        Task->>Installation: sync_assignee_outbound(external_issue, user, assign, assignment_source)
    else should_sync returns false
        Task-->>Caller: no-op
    end
Loading

React with 👍 or 👎 if you found this review useful.

class AssignmentSource:
source_name: str
integration_id: int
queued: datetime = timezone.now()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ High: timezone.now() is evaluated at import time, so all AssignmentSource instances created without an explicit queued will share the same timestamp. Use a default_factory to generate the value at instantiation.

Suggested change
queued: datetime = timezone.now()
from dataclasses import asdict, dataclass, field
@dataclass(frozen=True)
class AssignmentSource:
source_name: str
integration_id: int
queued: datetime = field(default_factory=timezone.now)


if installation.should_sync("outbound_assignee"):
parsed_assignment_source = (
AssignmentSource.from_dict(assignment_source_dict) if assignment_source_dict else None

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ High: Passing a second argument to should_sync may raise TypeError for integrations that override should_sync with the legacy single-argument signature. Add a backward-compatible fallback.

Suggested change
AssignmentSource.from_dict(assignment_source_dict) if assignment_source_dict else None
try:
_should_sync = installation.should_sync("outbound_assignee", parsed_assignment_source)
except TypeError:
_should_sync = installation.should_sync("outbound_assignee")
if _should_sync:

sync_assignee_outbound.apply_async(
kwargs={"external_issue_id": external_issue_id, "user_id": user_id, "assign": assign}
kwargs={
"external_issue_id": external_issue_id,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔷 Medium: assignment_source.to_dict() includes a datetime (queued) which may not be JSON-serializable in some Celery configurations. Since only integration_id/source_name are used downstream, pass a minimal dict to avoid serialization issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants