From b069e1bce4ba1131f88754fe1ac89ba9bf002202 Mon Sep 17 00:00:00 2001 From: Renn F Date: Wed, 17 Jun 2026 02:02:14 +0200 Subject: [PATCH] feat(external-pr): re-review on change, skip unchanged (head-SHA dedup) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reviewer was one-shot: external_review_task_exists deduped on (project, pr_number) only, so an external PR was reviewed exactly once ever — a contributor pushing a fix never triggered a re-review (the review went stale). Drive re-review off the PR's head commit instead: - list_open_prs now returns head_sha (the change signal). - ingest records the reviewed SHA as an external_pr_head= marker in the review task's quick_context. - external_review_task_exists is head-SHA aware: same SHA -> skip (unchanged); new SHA -> open a fresh review (changed); no task yet -> first review; legacy/markerless task or unknown SHA -> skip (never re-review on a guess, so existing reviews don't re-fire after deploy). No migration — reuses quick_context, like the supersede markers. --- roboco/services/git.py | 4 +- roboco/services/task.py | 42 ++++++++++--- .../unit/services/test_external_pr_ingest.py | 63 +++++++++++++++++++ 3 files changed, 99 insertions(+), 10 deletions(-) create mode 100644 tests/unit/services/test_external_pr_ingest.py diff --git a/roboco/services/git.py b/roboco/services/git.py index 0f7247af..883076e0 100644 --- a/roboco/services/git.py +++ b/roboco/services/git.py @@ -1414,7 +1414,8 @@ class GitService(BaseService): The inbound counterpart to the org's outbound PR calls: lists ALL open PRs (no ``head=`` filter), so it sees external/fork contributions the org did not create. Each record carries ``number``, ``url``, ``title``, - ``head_ref``, ``is_fork`` (head repo differs from base repo), + ``head_ref``, ``head_sha`` (the head commit — the change signal for + re-review), ``is_fork`` (head repo differs from base repo), ``user_login`` and ``author_association`` so the caller can classify trust. Returns ``[]`` on a missing token, unparseable remote, or any GitHub error — it never raises into the poll loop. @@ -1467,6 +1468,7 @@ class GitService(BaseService): "url": pr.get("html_url") or "", "title": pr.get("title") or "", "head_ref": head.get("ref"), + "head_sha": head.get("sha"), "is_fork": bool(head_full and head_full != base_full), "user_login": (pr.get("user") or {}).get("login"), "author_association": pr.get("author_association"), diff --git a/roboco/services/task.py b/roboco/services/task.py index c1e9521b..58145ed1 100644 --- a/roboco/services/task.py +++ b/roboco/services/task.py @@ -628,22 +628,39 @@ class TaskService(BaseService): return task async def external_review_task_exists( - self, project_id: UUID, pr_number: int + self, project_id: UUID, pr_number: int, head_sha: str | None = None ) -> bool: - """True if a review task already exists for this (project, external PR). + """True if this (project, external PR) at this head commit is already reviewed. - The de-dupe key for inbound external-PR ingestion: one review task per - ``(project_id, source='external_pr', pr_number)`` so re-polling an open - PR never creates a duplicate. + De-dupe key for inbound external-PR ingestion. Re-review is driven by the + PR's head commit: a review task records the SHA it covered as an + ``external_pr_head=`` marker in ``quick_context``. So: + + - no review task for this PR yet -> False (ingest the first review); + - a task already covers THIS ``head_sha`` -> True (skip — nothing changed); + - a legacy/markerless task exists, or ``head_sha`` is unknown -> True + (can't prove it changed, so don't re-review / don't spam); + - tasks exist but all cover OTHER SHAs -> False (the PR got new commits — + open a fresh review for the change). """ result = await self.session.execute( - select(TaskTable.id).where( + select(TaskTable.quick_context).where( TaskTable.project_id == project_id, TaskTable.source == "external_pr", TaskTable.pr_number == pr_number, ) ) - return result.first() is not None + contexts = result.scalars().all() + if not contexts: + return False + if not head_sha: + return True + marker = f"external_pr_head={head_sha}" + for qc in contexts: + text = qc or "" + if marker in text or "external_pr_head=" not in text: + return True + return False async def ingest_external_pr( self, @@ -656,7 +673,9 @@ class TaskService(BaseService): """Create one review task for a newly-seen external PR; ``None`` if it exists. ``pr`` is a normalized record from ``GitService.list_open_prs`` (number, - url, title). De-duped on ``(project_id, source='external_pr', pr_number)``. + url, title, head_sha). De-duped per ``(project_id, pr_number, head_sha)`` + — re-polling an unchanged PR is skipped, but new commits (a new head SHA) + open a fresh review (see ``external_review_task_exists``). The task is CODE-typed with ``source='external_pr'`` and ``confirmed_by_human=False`` — a deliberate gate: no agent fetches, checks out, or runs the contributor's code until a human confirms the PR. Caller @@ -665,7 +684,8 @@ class TaskService(BaseService): pr_number = int(pr["number"]) pr_url = str(pr.get("url") or "") pr_title = str(pr.get("title") or "") - if await self.external_review_task_exists(project_id, pr_number): + head_sha = str(pr.get("head_sha") or "") + if await self.external_review_task_exists(project_id, pr_number, head_sha): return None title = f"Review external PR #{pr_number}: {pr_title}".strip() req = TaskCreateRequest( @@ -693,6 +713,10 @@ class TaskService(BaseService): task = await self.create(req) task.pr_number = pr_number task.pr_url = pr_url + # Record the reviewed head commit so a later push (new SHA) re-reviews, + # while an unchanged PR is skipped (see external_review_task_exists). + if head_sha: + task.quick_context = f"external_pr_head={head_sha}" await self.session.flush() return task diff --git a/tests/unit/services/test_external_pr_ingest.py b/tests/unit/services/test_external_pr_ingest.py new file mode 100644 index 00000000..6aa6c6a5 --- /dev/null +++ b/tests/unit/services/test_external_pr_ingest.py @@ -0,0 +1,63 @@ +"""External-PR review dedup — review once per (project, PR, head commit). + +``external_review_task_exists`` drives re-review off the PR's head SHA: an +unchanged PR (same head) is skipped, new commits (a new head SHA) open a fresh +review, and legacy/unknown-SHA tasks are never re-reviewed (no spam). +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock +from uuid import uuid4 + +import pytest +from roboco.services.task import TaskService + + +def _service(quick_contexts: list[str | None]) -> TaskService: + """A TaskService whose review-task query returns these quick_context values.""" + res = MagicMock() + res.scalars.return_value.all.return_value = quick_contexts + session = MagicMock() + session.execute = AsyncMock(return_value=res) + return TaskService(session) + + +@pytest.mark.asyncio +async def test_no_task_yet_ingests() -> None: + svc = _service([]) + assert await svc.external_review_task_exists(uuid4(), 170, "abc") is False + + +@pytest.mark.asyncio +async def test_same_head_sha_skips() -> None: + svc = _service(["external_pr_head=abc"]) + assert await svc.external_review_task_exists(uuid4(), 170, "abc") is True + + +@pytest.mark.asyncio +async def test_new_head_sha_rereviews() -> None: + # PR got new commits since the last review → open a fresh review. + svc = _service(["external_pr_head=abc"]) + assert await svc.external_review_task_exists(uuid4(), 170, "def") is False + + +@pytest.mark.asyncio +async def test_legacy_markerless_task_not_rereviewed() -> None: + # A task ingested before head-SHA tracking existed → don't re-review it. + svc = _service([None]) + assert await svc.external_review_task_exists(uuid4(), 170, "def") is True + + +@pytest.mark.asyncio +async def test_unknown_head_sha_does_not_spam() -> None: + # Can't detect change (no SHA from GitHub) → treat as reviewed. + svc = _service(["external_pr_head=abc"]) + assert await svc.external_review_task_exists(uuid4(), 170, None) is True + + +@pytest.mark.asyncio +async def test_multiple_old_shas_still_rereviews_new() -> None: + svc = _service(["external_pr_head=abc", "external_pr_head=def"]) + assert await svc.external_review_task_exists(uuid4(), 170, "ghi") is False + assert await svc.external_review_task_exists(uuid4(), 170, "def") is True