[F103] make project_id mandatory on close_pull_request (close cross-repo collision)

roboco/services/git.py

@@ -3878,10 +3878,10 @@ class GitService(BaseService):
         self,
         pr_number: int,
         *,
+        project_id: UUID,
         comment: str | None = None,
         delete_branch: bool = True,
         actor_agent_id: UUID | None = None,
-        project_id: UUID | None = None,
     ) -> None:
         """Close PR ``pr_number`` on GitHub, optionally with an explanatory comment.
 
@@ -3890,20 +3890,24 @@ class GitService(BaseService):
         action agents had no verb for. Best-effort branch cleanup on close.
 
         ``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. Idempotent: a PR that is already closed is a no-op (no
-        duplicate comment), so a retried close-on-land never re-comments.
+        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 (and closed) by accident. Mirrors :meth:`pr_merge`.
+        Idempotent: a PR that is already closed is a no-op (no duplicate
+        comment), so a retried close-on-land never re-comments.
         """
         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_close_pull_request_scoping.py

@@ -0,0 +1,93 @@
+"""close_pull_request 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 #159 and a frontend repo's PR #159 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`` resolves the wrong
+project and the GitHub close hits (and closes!) the wrong repo's PR.
+
+``project_id`` is therefore MANDATORY: the caller can never close 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`` / ``pr_target`` already
+established.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+from unittest.mock import AsyncMock, MagicMock, patch
+from uuid import uuid4
+
+import pytest
+from roboco.services.base import NotFoundError
+from roboco.services.git import GitService
+
+_PR_NUMBER = 159
+
+
+def _make_session(recorder: list[object]) -> MagicMock:
+    session = MagicMock()
+
+    async def _execute(stmt: object) -> MagicMock:
+        recorder.append(stmt)
+        result = MagicMock()
+        result.scalar_one_or_none.return_value = None
+        return result
+
+    session.execute = AsyncMock(side_effect=_execute)
+    return session
+
+
+def _service(recorder: list[object]) -> GitService:
+    return GitService(_make_session(recorder))
+
+
+def _patch_project_service(project: object | None) -> Any:
+    fake_service = MagicMock()
+    fake_service.get = AsyncMock(return_value=project)
+    fake_service.get_by_slug = AsyncMock(return_value=project)
+    return patch("roboco.services.git.get_project_service", return_value=fake_service)
+
+
+def _compiled_sql(stmt: Any) -> str:
+    """Render a SQLAlchemy stmt to literal-bound SQL for assertion."""
+    return str(stmt.compile(compile_kwargs={"literal_binds": True}))
+
+
+@pytest.mark.asyncio
+async def test_close_pull_request_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()
+
+    # Task lookup returns None → NotFoundError, but we only care about the SQL
+    # the lookup was issued with.
+    with _patch_project_service(MagicMock(slug="roboco")), pytest.raises(NotFoundError):
+        await svc.close_pull_request(_PR_NUMBER, project_id=project_id)
+
+    assert len(recorder) == 1
+    sql = _compiled_sql(recorder[0])
+    assert "tasks.pr_number =" in sql
+    # The WHERE clause filters on project_id (the select list renders the
+    # column as ``tasks.project_id,`` with a trailing comma; the WHERE
+    # comparison renders as ``tasks.project_id =``).
+    assert "tasks.project_id =" in sql
+
+
+@pytest.mark.asyncio
+async def test_close_pull_request_requires_project_id() -> None:
+    """``project_id`` is mandatory — a caller can NEVER close 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 close another
+    project's same-numbered PR (the cross-repo collision)."""
+    recorder: list[object] = []
+    svc = _service(recorder)
+
+    with pytest.raises(TypeError):
+        await svc.close_pull_request(_PR_NUMBER, comment="superseded")  # no project_id
+
+    # The unscoped lookup was never issued — no SQL reached the session.
+    assert recorder == []

tests/unit/services/test_git_rebase_resolve.py

@@ -12,6 +12,7 @@ from __future__ import annotations
 from pathlib import Path
 from typing import Any
 from unittest.mock import AsyncMock, MagicMock, patch
+from uuid import uuid4
 
 import pytest
 from roboco.services.git import GitService
@@ -184,7 +185,9 @@ async def test_close_pull_request_patches_state_closed(
             return _Resp()
 
     with patch("roboco.services.git.httpx.AsyncClient", return_value=_Client()):
-        await svc.close_pull_request(159, comment="superseded by #158")
+        await svc.close_pull_request(
+            159, project_id=uuid4(), comment="superseded by #158"
+        )
 
     assert (
         "POST",
@@ -259,7 +262,10 @@ async def test_close_pull_request_idempotent_when_already_closed(
 
     with patch("roboco.services.git.httpx.AsyncClient", return_value=_Client()):
         await svc.close_pull_request(
-            159, comment="superseded by #158", delete_branch=False
+            159,
+            project_id=uuid4(),
+            comment="superseded by #158",
+            delete_branch=False,
         )
 
     assert [c[0] for c in calls] == ["GET"]  # no POST comment, no PATCH