From fb850e82352bd50aab5e5cac4e7b19b6a89b4382 Mon Sep 17 00:00:00 2001 From: Renn F Date: Mon, 29 Jun 2026 00:49:36 +0200 Subject: [PATCH] [F139] scope active_task_owns_branch to the polled project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit active_task_owns_branch did an unscoped WHERE branch_name = ? — a cross-project branch_name collision (UUID-derived 8-char prefixes, theoretical) made the internal-PR reviewer skip the WRONG project's PR (project A's leftover PR skipped because project B happened to have an active task with the same branch). Pass project_id (in scope at the orchestrator call site) and add TaskTable.project_id == project_id to the WHERE. Correct for single-project tasks and MegaTask multi-repo batches alike: each root-subtask carries its own project_id matching its own repo, so a branch on project A's repo is owned only by a task whose project_id == A. --- roboco/runtime/orchestrator.py | 3 +- roboco/services/task.py | 11 +- .../test_active_task_owns_branch_scoping.py | 123 ++++++++++++++++++ .../unit/services/test_external_pr_ingest.py | 7 +- 4 files changed, 138 insertions(+), 6 deletions(-) create mode 100644 tests/integration/services/test_active_task_owns_branch_scoping.py diff --git a/roboco/runtime/orchestrator.py b/roboco/runtime/orchestrator.py index 6595c2b5..0401744a 100644 --- a/roboco/runtime/orchestrator.py +++ b/roboco/runtime/orchestrator.py @@ -6604,7 +6604,8 @@ Start by: if not settings.internal_pr_enabled: return False if await task_service.active_task_owns_branch( - str(pr.get("head_ref") or "") + str(pr.get("head_ref") or ""), + cast("UUID", project.id), ): return False source = "internal_pr" diff --git a/roboco/services/task.py b/roboco/services/task.py index 685dc3a0..7b717383 100644 --- a/roboco/services/task.py +++ b/roboco/services/task.py @@ -1144,19 +1144,26 @@ class TaskService(BaseService): await self.session.flush() return task - async def active_task_owns_branch(self, branch_name: str) -> bool: - """True if a non-terminal task already owns this git branch. + async def active_task_owns_branch(self, branch_name: str, project_id: UUID) -> bool: + """True if a non-terminal task on ``project_id`` already owns this branch. Lets the internal-PR reviewer skip the org's own in-flight integration PRs — those whose head branch a live task created via the agent task-flow (and which therefore already pass QA + PM review) — and review only org-repo PRs opened outside that flow. + + Scoped to the polled project: a branch in project A's repo can only be + owned by a task whose ``project_id == A`` (each task branches in its + own project's repo, including each root-subtask of a multi-repo + MegaTask). An unscoped lookup would match the wrong project's task on a + cross-project branch_name collision and false-skip project A's PR. """ if not branch_name: return False result = await self.session.execute( select(TaskTable.id).where( TaskTable.branch_name == branch_name, + TaskTable.project_id == project_id, TaskTable.status.notin_([TaskStatus.COMPLETED, TaskStatus.CANCELLED]), ) ) diff --git a/tests/integration/services/test_active_task_owns_branch_scoping.py b/tests/integration/services/test_active_task_owns_branch_scoping.py new file mode 100644 index 00000000..6b95b451 --- /dev/null +++ b/tests/integration/services/test_active_task_owns_branch_scoping.py @@ -0,0 +1,123 @@ +"""active_task_owns_branch must be scoped to the polled project. + +The internal-PR reviewer (orchestrator) calls this to skip the org's own +in-flight integration PRs — a PR on project A's repo is "ours" only if a +non-terminal task ON PROJECT A owns its head branch. The query was unscoped +(``WHERE branch_name = ?``), so a cross-project branch_name collision (two +tasks sharing an 8-char-UUID-prefix branch on different projects) made it +match the WRONG project's task — project A's leftover PR was skipped because +project B happened to have an active task with the same branch_name. + +Scoping by ``project_id`` is correct for both single-project tasks and +MegaTask multi-repo batches: each root-subtask carries its own ``project_id`` +matching its own repo, so a branch on project A's repo is owned only by a +task whose ``project_id == A``. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, cast +from uuid import UUID, uuid4 + +import pytest +from roboco.db.tables import AgentTable, ProjectTable, TaskTable +from roboco.foundation import identity as _foundation +from roboco.models.base import ( + AgentRole, + AgentStatus, + Complexity, + TaskNature, + TaskStatus, + TaskType, + Team, +) +from roboco.services.task import get_task_service + +if TYPE_CHECKING: + from sqlalchemy.ext.asyncio import AsyncSession + +SYSTEM_UUID = _foundation.AGENTS["system"].uuid +_BRANCH = "feature/backend/collide001" + + +async def _seed_project(db: AsyncSession, slug: str) -> ProjectTable: + if await db.get(AgentTable, SYSTEM_UUID) is None: + db.add( + AgentTable( + id=SYSTEM_UUID, + name="System", + slug=f"system-{uuid4().hex[:8]}", + role=AgentRole.SYSTEM, + team=None, + status=AgentStatus.ACTIVE, + model_config={}, + system_prompt="x", + capabilities=[], + permissions={}, + metrics={}, + ) + ) + await db.flush() + project = ProjectTable( + id=uuid4(), + name=slug, + slug=slug, + git_url=f"https://github.com/rennf93/{slug}", + assigned_cell=Team.BACKEND, + created_by=SYSTEM_UUID, + ) + db.add(project) + await db.flush() + return project + + +def _task(project_id, *, branch: str, status: TaskStatus) -> TaskTable: + return TaskTable( + id=uuid4(), + title=f"task {branch}", + description="x", + acceptance_criteria=["criterion"], + status=status, + priority=2, + task_type=TaskType.CODE, + nature=TaskNature.TECHNICAL, + project_id=project_id, + branch_name=branch, + created_by=SYSTEM_UUID, + team=Team.BACKEND, + dependency_ids=[], + blocker_ids=[], + sequence=0, + estimated_complexity=Complexity.MEDIUM, + ) + + +@pytest.mark.asyncio +async def test_branch_owned_only_by_its_own_project(db_session: AsyncSession) -> None: + """Project A has an ACTIVE task on ``_BRANCH``; project B has a COMPLETED + task with the SAME ``_BRANCH``. Asking for project B must NOT match + project A's active task (the unscoped query did). Asking for project A + matches its own active task.""" + proj_a = await _seed_project(db_session, "gca-collide-a") + proj_b = await _seed_project(db_session, "gca-collide-b") + db_session.add_all( + [ + _task(proj_a.id, branch=_BRANCH, status=TaskStatus.IN_PROGRESS), + _task(proj_b.id, branch=_BRANCH, status=TaskStatus.COMPLETED), + ] + ) + await db_session.flush() + svc = get_task_service(db_session) + + # The active task on project A owns it for project A. + assert await svc.active_task_owns_branch(_BRANCH, cast("UUID", proj_a.id)) is True + # Project B's task is terminal AND the only active owner is on project A — + # the unscoped query would wrongly return True here (matching A's task). + assert await svc.active_task_owns_branch(_BRANCH, cast("UUID", proj_b.id)) is False + + +@pytest.mark.asyncio +async def test_empty_branch_never_owned(db_session: AsyncSession) -> None: + proj = await _seed_project(db_session, "gca-collide-empty") + svc = get_task_service(db_session) + assert await svc.active_task_owns_branch("", cast("UUID", proj.id)) is False diff --git a/tests/unit/services/test_external_pr_ingest.py b/tests/unit/services/test_external_pr_ingest.py index c8dc238a..7240e251 100644 --- a/tests/unit/services/test_external_pr_ingest.py +++ b/tests/unit/services/test_external_pr_ingest.py @@ -146,17 +146,18 @@ def _branch_service(*, found: bool) -> TaskService: @pytest.mark.asyncio async def test_active_task_owns_branch_true_when_live_task_holds_it() -> None: assert ( - await _branch_service(found=True).active_task_owns_branch("feature/x") is True + await _branch_service(found=True).active_task_owns_branch("feature/x", uuid4()) + is True ) @pytest.mark.asyncio async def test_active_task_owns_branch_false_when_no_live_task() -> None: svc = _branch_service(found=False) - assert await svc.active_task_owns_branch("feature/x") is False + assert await svc.active_task_owns_branch("feature/x", uuid4()) is False @pytest.mark.asyncio async def test_active_task_owns_branch_false_for_empty_branch() -> None: # No branch → cannot be owned; never hits the DB. - assert await TaskService(MagicMock()).active_task_owns_branch("") is False + assert await TaskService(MagicMock()).active_task_owns_branch("", uuid4()) is False