[F102] make project_id mandatory on pr_target (close cross-repo pr_number collision)

roboco/services/git.py

@@ -3955,8 +3955,8 @@ class GitService(BaseService):
         self,
         pr_number: int,
         *,
+        project_id: UUID,
         actor_agent_id: UUID | None = None,
-        project_id: UUID | None = None,
     ) -> str:
         """Return the current target (base) branch of an open PR.
 
@@ -3965,19 +3965,22 @@ class GitService(BaseService):
         ``submit_qa`` has cleared ``assigned_to`` without ValidationError.
 
         ``pr_number`` alone is ambiguous across projects (GitHub numbers PRs
-        per-repo), so when the caller knows which project the PR belongs to it
-        MUST pass ``project_id`` — the task lookup is then scoped to it so a
-        same-numbered PR in another project's repo is never resolved by
-        accident (mirrors ``close_pull_request``).
+        per-repo, but ``tasks.pr_number`` stores the bare integer with no repo
+        scoping, so two tasks on different repos can share a number). The
+        caller MUST pass the ``project_id`` the PR belongs to — the task lookup
+        is scoped to it so a same-numbered PR in another project's repo is
+        never resolved by accident. Mirrors :meth:`pr_merge`.
         """
         from sqlalchemy import select
 
         from roboco.db.tables import TaskTable as _TaskTable
 
-        stmt = select(_TaskTable).where(_TaskTable.pr_number == pr_number)
-        if project_id is not None:
-            stmt = stmt.where(_TaskTable.project_id == project_id)
-        result = await self.session.execute(stmt.limit(1))
+        result = await self.session.execute(
+            select(_TaskTable)
+            .where(_TaskTable.pr_number == pr_number)
+            .where(_TaskTable.project_id == project_id)
+            .limit(1)
+        )
         task = result.scalar_one_or_none()
         if task is None:
             raise NotFoundError("PR", str(pr_number))

tests/unit/services/test_git.py

@@ -377,7 +377,7 @@ async def test_pr_target_returns_base_ref() -> None:
         _patch_project_service(fake_project),
         patch("roboco.services.git.httpx.AsyncClient", return_value=fake_client),
     ):
-        out = await svc.pr_target(42)
+        out = await svc.pr_target(42, project_id=project_id)
     assert out == "feature/parent"
 
 
@@ -387,7 +387,7 @@ async def test_pr_target_raises_when_pr_not_found() -> None:
     result.scalar_one_or_none.return_value = None
     svc = _service(execute_returns=result)
     with pytest.raises(NotFoundError):
-        await svc.pr_target(99)
+        await svc.pr_target(99, project_id=uuid4())
 
 
 # ---------------------------------------------------------------------------

tests/unit/services/test_git_pr_target_scoping.py

@@ -1,15 +1,15 @@
-"""F052: pr_target must scope its task lookup by project_id when the caller
-knows it, mirroring close_pull_request.
+"""pr_target must scope its task lookup by project_id — always.
 
 GitHub numbers PRs per-repo, so ``pr_number`` is ambiguous across projects: a
 backend repo's PR #132 and a frontend repo's PR #132 are different PRs. The
 bare ``WHERE pr_number == N LIMIT 1`` query returns whichever task row comes
 first — the wrong repo's task, whose ``_project_for_task`` then resolves the
-wrong project and the GitHub fetch hits the wrong repo. When the caller knows
-the project (the Main PM coordinating a known root), it must pass
-``project_id`` so the lookup is scoped and a same-numbered PR in another
-project's repo is never resolved by accident — the pattern
-``close_pull_request`` already established.
+wrong project and the GitHub fetch hits the wrong repo.
+
+``project_id`` is therefore MANDATORY: the caller can never resolve a PR
+without scoping it to a project, so a same-numbered PR in another project's
+repo is unreachable by accident — the pattern ``pr_merge`` already
+established (and ``close_pull_request`` follows).
 """
 
 from __future__ import annotations
@@ -59,10 +59,10 @@ def _compiled_sql(stmt: Any) -> str:
 
 
 @pytest.mark.asyncio
-async def test_pr_target_scopes_task_lookup_by_project_id_when_provided() -> None:
-    """With ``project_id`` the task lookup WHERE clause filters on BOTH
-    pr_number and project_id — a same-numbered PR in another project's repo
-    can't be resolved by accident."""
+async def test_pr_target_scopes_task_lookup_by_project_id() -> None:
+    """The task lookup WHERE clause filters on BOTH pr_number and project_id
+    — a same-numbered PR in another project's repo can't be resolved by
+    accident."""
     recorder: list[object] = []
     svc = _service(recorder)
     project_id = uuid4()
@@ -82,21 +82,19 @@ async def test_pr_target_scopes_task_lookup_by_project_id_when_provided() -> Non
 
 
 @pytest.mark.asyncio
-async def test_pr_target_without_project_id_does_not_scope() -> None:
-    """Without ``project_id`` the lookup stays unscoped (backward-compatible
-    with callers that don't know the project) — only pr_number is filtered."""
+async def test_pr_target_requires_project_id() -> None:
+    """``project_id`` is mandatory — a caller can NEVER resolve a PR without
+    scoping it to a project. Omitting it is a programming error (TypeError at
+    call time), not a silent unscoped lookup that could hit another project's
+    same-numbered PR (the cross-repo #132 collision)."""
     recorder: list[object] = []
     svc = _service(recorder)
 
-    with _patch_project_service(MagicMock(slug="roboco")), pytest.raises(NotFoundError):
-        await svc.pr_target(_PR_NUMBER)
+    with pytest.raises(TypeError):
+        await svc.pr_target(_PR_NUMBER)  # missing required project_id
 
-    assert len(recorder) == 1
-    sql = _compiled_sql(recorder[0])
-    assert "tasks.pr_number =" in sql
-    # No project_id WHERE filter (the column still appears in the select list
-    # with a trailing comma, but the ``=`` comparison is absent).
-    assert "tasks.project_id =" not in sql
+    # The unscoped lookup was never issued — no SQL reached the session.
+    assert recorder == []
 
 
 @pytest.mark.asyncio