diff --git a/roboco/services/git.py b/roboco/services/git.py index 21d69a41..48a5b9fa 100644 --- a/roboco/services/git.py +++ b/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)) diff --git a/tests/unit/services/test_git.py b/tests/unit/services/test_git.py index ca9b95e4..b637000e 100644 --- a/tests/unit/services/test_git.py +++ b/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()) # --------------------------------------------------------------------------- diff --git a/tests/unit/services/test_git_pr_target_scoping.py b/tests/unit/services/test_git_pr_target_scoping.py index 8a6e2517..3f9a10b0 100644 --- a/tests/unit/services/test_git_pr_target_scoping.py +++ b/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