From 8c1c5c2b2d8e31bd621177043ab439994c6fc1b9 Mon Sep 17 00:00:00 2001 From: Renn F Date: Sun, 28 Jun 2026 21:37:37 +0200 Subject: [PATCH] [F103] make project_id mandatory on close_pull_request (close cross-repo collision) --- roboco/services/git.py | 24 +++-- .../test_git_close_pull_request_scoping.py | 93 +++++++++++++++++++ .../unit/services/test_git_rebase_resolve.py | 10 +- 3 files changed, 115 insertions(+), 12 deletions(-) create mode 100644 tests/unit/services/test_git_close_pull_request_scoping.py diff --git a/roboco/services/git.py b/roboco/services/git.py index 48a5b9fa..2c465dda 100644 --- a/roboco/services/git.py +++ b/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)) diff --git a/tests/unit/services/test_git_close_pull_request_scoping.py b/tests/unit/services/test_git_close_pull_request_scoping.py new file mode 100644 index 00000000..81c49a52 --- /dev/null +++ b/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 == [] diff --git a/tests/unit/services/test_git_rebase_resolve.py b/tests/unit/services/test_git_rebase_resolve.py index 4519cc76..34f3210c 100644 --- a/tests/unit/services/test_git_rebase_resolve.py +++ b/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