diff --git a/roboco/api/routes/v2/do.py b/roboco/api/routes/v2/do.py index e6c43cf6..a1b3b984 100644 --- a/roboco/api/routes/v2/do.py +++ b/roboco/api/routes/v2/do.py @@ -20,6 +20,7 @@ from roboco.api.schemas.v2.do import ( NotifyRequest, OpenSessionRequest, ProgressRequest, + PRUpdateRequest, SayRequest, ) from roboco.services.gateway.content_actions import ContentActions @@ -243,3 +244,20 @@ async def do_channels( ) -> dict: env = await actions.channels(agent_id=x_agent_id) return envelope_to_response(env, request) + + +@router.post("/pr_update") +async def do_pr_update( + request: Request, + body: PRUpdateRequest, + x_agent_id: _AgentIdHeader, + actions: _ContentActionsDep, +) -> dict: + env = await actions.pr_update( + agent_id=x_agent_id, + task_id=body.task_id, + title=body.title, + body=body.body, + reviewers=body.reviewers, + ) + return envelope_to_response(env, request) diff --git a/roboco/mcp/do_server.py b/roboco/mcp/do_server.py index 1cb192f9..2369d099 100644 --- a/roboco/mcp/do_server.py +++ b/roboco/mcp/do_server.py @@ -332,6 +332,43 @@ def channels() -> dict[str, Any]: return _post("/api/v2/do/channels", {}) +def pr_update( + task_id: str, + title: str | None = None, + body: str | None = None, + reviewers: list[str] | None = None, +) -> dict[str, Any]: + """Update an existing PR's title, body, and/or requested reviewers. + + Use after ``open_pr`` when you need to correct the title/body or + request a reviewer. At least one of ``title``, ``body``, or + ``reviewers`` must be provided — passing all three None is rejected + with ``invalid_state`` before any GitHub call. + + Args: + task_id: UUID of the task whose PR you're editing. + title: Replacement PR title (omit to leave unchanged). + body: Replacement PR body markdown (omit to leave unchanged). + reviewers: List of agent slugs to request as reviewers (e.g. + ``["be-dev-2", "be-qa"]``). The gateway maps slugs to GitHub + usernames where the project records that mapping, otherwise + the slugs are forwarded as-is. + + Authorization: caller must be the task's assignee OR a PM on the + task's team (cell_pm same-team, or main_pm cross-team). Anyone else + receives ``not_authorized``. + """ + return _post( + "/api/v2/do/pr_update", + { + "task_id": task_id, + "title": title, + "body": body, + "reviewers": reviewers, + }, + ) + + # ---------- Tool registry ---------- # # Maps the tool name an agent calls (matches manifest entries and the @@ -351,6 +388,7 @@ _TOOLS: dict[str, Any] = { "notify_get": notify_get, "notify_ack": notify_ack, "channels": channels, + "pr_update": pr_update, } diff --git a/roboco/services/gateway/content_actions.py b/roboco/services/gateway/content_actions.py index ea3c3c0c..8f665021 100644 --- a/roboco/services/gateway/content_actions.py +++ b/roboco/services/gateway/content_actions.py @@ -15,6 +15,7 @@ import re from dataclasses import dataclass from typing import TYPE_CHECKING, Any, ClassVar +from roboco.exceptions import GitError from roboco.foundation.policy import communications as _comms from roboco.foundation.policy.journaling import Scope as _Scope from roboco.services.gateway.commit_validator import validate_commit_message @@ -927,6 +928,107 @@ class ContentActions: context_briefing={}, ) + _PM_ROLES_FOR_PR_UPDATE: ClassVar[frozenset[str]] = frozenset( + {"cell_pm", "main_pm"} + ) + + async def pr_update( + self, + *, + agent_id: UUID, + task_id: UUID, + title: str | None = None, + body: str | None = None, + reviewers: list[str] | None = None, + ) -> Envelope: + """Update an existing PR's title, body, and/or requested reviewers. + + Smoke-5 surfaced this gap: agents who needed to edit a PR's + title/body or assign a reviewer after ``open_pr`` had no verb + for it and got bash-shimmed by the ``gh pr edit`` guard. This + verb is the gateway-native replacement. + + Authorization: caller must be the task's ``assigned_to`` OR a + PM on the task's team (cell_pm.team == task.team, or main_pm + which is cross-team). + + Preconditions: + - task must exist (else not_found) + - task.pr_number must be set (else invalid_state, remediate + 'call open_pr') + - at least one of title/body/reviewers must be non-None (else + invalid_state — schema-level check is the first line of + defense; this guard catches direct gateway calls) + """ + if title is None and body is None and reviewers is None: + return Envelope.invalid_state( + message="no fields to update", + remediate=( + "provide at least one of title, body, or reviewers; " + "passing all None has no effect" + ), + context_briefing={}, + ) + t = await self.task.get(task_id) + if t is None: + return Envelope.not_found(message=f"task {task_id} not found") + if t.pr_number is None: + return Envelope.invalid_state( + message=f"task {task_id} has no PR open", + remediate=( + "call open_pr(task_id) first; pr_update only edits an " + "already-open PR" + ), + context_briefing={}, + ) + agent = await self.task.agent_for(agent_id) + role_str = str(agent.role) if agent is not None else "" + is_assignee = t.assigned_to == agent_id + is_main_pm = role_str == "main_pm" + is_cell_pm_on_team = ( + role_str == "cell_pm" + and agent is not None + and agent.team is not None + and t.team is not None + and str(agent.team) == str(t.team) + ) + if not (is_assignee or is_main_pm or is_cell_pm_on_team): + return Envelope.not_authorized( + message=( + f"role {role_str!r} is neither the assignee nor a PM on " + f"this task's team; cannot update PR" + ), + remediate=( + "only the task's assignee or a PM on the task's team " + "may edit the PR; ask the assignee or your PM to call " + "pr_update instead" + ), + context_briefing={}, + ) + try: + result = await self.git.update_pr_for_task( + task_id, + title=title, + body=body, + reviewers=reviewers, + ) + except GitError as exc: + return Envelope.invalid_state( + message=str(exc), + remediate=( + "check the PR number on the task and retry; if the PR " + "was closed externally, the task should be reset" + ), + context_briefing={}, + ) + return Envelope.ok( + status=str(t.status), + task_id=str(task_id), + next="continue working, or i_am_done when ready", + evidence=result, + context_briefing={}, + ) + async def notify_ack( self, *, diff --git a/tests/unit/api/routes/v2/test_do_pr_update.py b/tests/unit/api/routes/v2/test_do_pr_update.py new file mode 100644 index 00000000..e5dbb7b8 --- /dev/null +++ b/tests/unit/api/routes/v2/test_do_pr_update.py @@ -0,0 +1,111 @@ +"""Unit tests for POST /api/v2/do/pr_update — route + schema. + +Pydantic's model_validator must reject an all-None payload with 422 +before ContentActions ever runs; a valid payload must forward title / +body / reviewers verbatim. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock +from uuid import uuid4 + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient +from roboco.api.deps import get_content_actions +from roboco.api.routes.v2.do import router +from roboco.services.gateway.content_actions import ContentActions + +_HTTP_200 = 200 +_HTTP_422 = 422 + +_AGENT_ID = str(uuid4()) +_TASK_ID = str(uuid4()) +_HEADERS = {"X-Agent-ID": _AGENT_ID} + + +def _make_envelope(payload: dict | None = None) -> MagicMock: + env = MagicMock() + base = {"status": "in_progress", "task_id": _TASK_ID, "next": "continue"} + if payload: + base.update(payload) + env.as_dict.return_value = base + return env + + +def _build_app(mock_actions: MagicMock) -> FastAPI: + app = FastAPI() + app.include_router(router) + app.dependency_overrides[get_content_actions] = lambda: mock_actions + return app + + +@pytest.mark.asyncio +async def test_pr_update_all_none_returns_422() -> None: + """Body with task_id only (no title/body/reviewers) → 422 from validator.""" + mock_actions = MagicMock(spec=ContentActions) + mock_actions.pr_update = AsyncMock(return_value=_make_envelope()) + client = TestClient(_build_app(mock_actions)) + + resp = client.post( + "/api/v2/do/pr_update", + json={"task_id": _TASK_ID}, + headers=_HEADERS, + ) + + assert resp.status_code == _HTTP_422 + mock_actions.pr_update.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_pr_update_title_only_forwards_to_content_actions() -> None: + """Body with title only → 200, forwarded to ContentActions.pr_update.""" + mock_actions = MagicMock(spec=ContentActions) + mock_actions.pr_update = AsyncMock( + return_value=_make_envelope({"evidence": {"updated_fields": ["title"]}}) + ) + client = TestClient(_build_app(mock_actions)) + + resp = client.post( + "/api/v2/do/pr_update", + json={"task_id": _TASK_ID, "title": "new title"}, + headers=_HEADERS, + ) + + assert resp.status_code == _HTTP_200 + mock_actions.pr_update.assert_awaited_once() + call_kwargs = mock_actions.pr_update.call_args.kwargs + assert str(call_kwargs["task_id"]) == _TASK_ID + assert call_kwargs["title"] == "new title" + assert call_kwargs["body"] is None + assert call_kwargs["reviewers"] is None + + +@pytest.mark.asyncio +async def test_pr_update_all_fields_forwarded() -> None: + """Body with title + body + reviewers → all three forwarded verbatim.""" + mock_actions = MagicMock(spec=ContentActions) + mock_actions.pr_update = AsyncMock( + return_value=_make_envelope( + {"evidence": {"updated_fields": ["title", "body", "reviewers"]}} + ) + ) + client = TestClient(_build_app(mock_actions)) + + resp = client.post( + "/api/v2/do/pr_update", + json={ + "task_id": _TASK_ID, + "title": "t", + "body": "b", + "reviewers": ["be-dev-2", "be-qa"], + }, + headers=_HEADERS, + ) + + assert resp.status_code == _HTTP_200 + call_kwargs = mock_actions.pr_update.call_args.kwargs + assert call_kwargs["title"] == "t" + assert call_kwargs["body"] == "b" + assert call_kwargs["reviewers"] == ["be-dev-2", "be-qa"] diff --git a/tests/unit/gateway/test_pr_update.py b/tests/unit/gateway/test_pr_update.py new file mode 100644 index 00000000..34f6411f --- /dev/null +++ b/tests/unit/gateway/test_pr_update.py @@ -0,0 +1,347 @@ +"""Tests for ContentActions.pr_update — gateway verb behavior matrix. + +Covers: missing pr_number, all-None fields, non-assignee non-PM rejection, +per-field forwarding, and GitError surfacing. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock +from uuid import UUID, uuid4 + +import pytest +from roboco.exceptions import GitError +from roboco.services.gateway.content_actions import ContentActions, ContentActionsDeps + + +def _make_deps(**overrides: AsyncMock) -> ContentActionsDeps: + """Wire ContentActionsDeps with mocks; honour caller overrides.""" + task = overrides.get("task", AsyncMock()) + git = overrides.get("git", AsyncMock()) + return ContentActionsDeps( + task=task, + git=git, + messaging=overrides.get("messaging", AsyncMock()), + a2a=overrides.get("a2a", AsyncMock()), + journal=overrides.get("journal", AsyncMock()), + workspace=overrides.get("workspace", AsyncMock()), + notifications=overrides.get("notifications", AsyncMock()), + notification_delivery=overrides.get("notification_delivery", AsyncMock()), + ) + + +def _task( + *, + pr_number: int | None = 7, + assigned_to: UUID, + team: str = "backend", +) -> MagicMock: + return MagicMock( + id=uuid4(), + status="in_progress", + pr_number=pr_number, + pr_url=( + f"https://github.com/acme/repo/pull/{pr_number}" + if pr_number is not None + else None + ), + assigned_to=assigned_to, + team=team, + ) + + +def _agent(role: str, team: str | None = None) -> MagicMock: + return MagicMock(id=uuid4(), role=role, team=team) + + +@pytest.mark.asyncio +async def test_pr_update_missing_pr_number_returns_invalid_state() -> None: + """task.pr_number is None → invalid_state with remediate 'call open_pr'.""" + agent_id = uuid4() + task = _task(pr_number=None, assigned_to=agent_id) + task_svc = AsyncMock() + task_svc.get.return_value = task + task_svc.agent_for.return_value = _agent("developer", "backend") + + deps = _make_deps(task=task_svc) + ca = ContentActions(deps) + + env = await ca.pr_update( + agent_id=agent_id, task_id=task.id, title="new", body=None, reviewers=None + ) + body = env.as_dict() + + assert body["error"] == "invalid_state" + assert "open_pr" in body["remediate"] + deps.git.update_pr_for_task.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_pr_update_all_fields_none_returns_invalid_state() -> None: + """All of title/body/reviewers None → invalid_state.""" + agent_id = uuid4() + task = _task(assigned_to=agent_id) + task_svc = AsyncMock() + task_svc.get.return_value = task + task_svc.agent_for.return_value = _agent("developer", "backend") + + deps = _make_deps(task=task_svc) + ca = ContentActions(deps) + + env = await ca.pr_update( + agent_id=agent_id, task_id=task.id, title=None, body=None, reviewers=None + ) + body = env.as_dict() + + assert body["error"] == "invalid_state" + assert "at least one" in body["remediate"] + deps.git.update_pr_for_task.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_pr_update_non_assignee_non_pm_returns_not_authorized() -> None: + """A developer who is neither the assignee nor a PM → not_authorized.""" + assignee_id = uuid4() + caller_id = uuid4() + task = _task(assigned_to=assignee_id) + task_svc = AsyncMock() + task_svc.get.return_value = task + task_svc.agent_for.return_value = _agent("developer", "backend") + + deps = _make_deps(task=task_svc) + ca = ContentActions(deps) + + env = await ca.pr_update( + agent_id=caller_id, + task_id=task.id, + title="new title", + body=None, + reviewers=None, + ) + body = env.as_dict() + + assert body["error"] == "not_authorized" + deps.git.update_pr_for_task.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_pr_update_assignee_title_only_forwarded() -> None: + """Assignee + title only forwards (title=X, body=None, reviewers=None).""" + agent_id = uuid4() + task = _task(assigned_to=agent_id) + task_svc = AsyncMock() + task_svc.get.return_value = task + task_svc.agent_for.return_value = _agent("developer", "backend") + + git_svc = AsyncMock() + git_svc.update_pr_for_task.return_value = { + "pr_number": 7, + "pr_url": task.pr_url, + "updated_fields": ["title"], + } + deps = _make_deps(task=task_svc, git=git_svc) + ca = ContentActions(deps) + + env = await ca.pr_update( + agent_id=agent_id, + task_id=task.id, + title="new title", + body=None, + reviewers=None, + ) + body = env.as_dict() + + assert body["error"] is None + assert body["evidence"]["updated_fields"] == ["title"] + git_svc.update_pr_for_task.assert_awaited_once() + call_kwargs = git_svc.update_pr_for_task.call_args.kwargs + assert call_kwargs["title"] == "new title" + assert call_kwargs["body"] is None + assert call_kwargs["reviewers"] is None + + +@pytest.mark.asyncio +async def test_pr_update_reviewers_only_forwarded() -> None: + """Assignee + reviewers only → forwarded as the only non-None arg.""" + agent_id = uuid4() + task = _task(assigned_to=agent_id) + task_svc = AsyncMock() + task_svc.get.return_value = task + task_svc.agent_for.return_value = _agent("developer", "backend") + + git_svc = AsyncMock() + git_svc.update_pr_for_task.return_value = { + "pr_number": 7, + "pr_url": task.pr_url, + "updated_fields": ["reviewers"], + } + deps = _make_deps(task=task_svc, git=git_svc) + ca = ContentActions(deps) + + env = await ca.pr_update( + agent_id=agent_id, + task_id=task.id, + title=None, + body=None, + reviewers=["be-dev-2"], + ) + body = env.as_dict() + + assert body["error"] is None + git_svc.update_pr_for_task.assert_awaited_once() + call_kwargs = git_svc.update_pr_for_task.call_args.kwargs + assert call_kwargs["title"] is None + assert call_kwargs["body"] is None + assert call_kwargs["reviewers"] == ["be-dev-2"] + + +@pytest.mark.asyncio +async def test_pr_update_all_three_forwarded() -> None: + """All three fields → all forwarded; updated_fields reflects all three.""" + agent_id = uuid4() + task = _task(assigned_to=agent_id) + task_svc = AsyncMock() + task_svc.get.return_value = task + task_svc.agent_for.return_value = _agent("developer", "backend") + + git_svc = AsyncMock() + git_svc.update_pr_for_task.return_value = { + "pr_number": 7, + "pr_url": task.pr_url, + "updated_fields": ["title", "body", "reviewers"], + } + deps = _make_deps(task=task_svc, git=git_svc) + ca = ContentActions(deps) + + env = await ca.pr_update( + agent_id=agent_id, + task_id=task.id, + title="t", + body="b", + reviewers=["be-dev-2"], + ) + body = env.as_dict() + + assert body["error"] is None + call_kwargs = git_svc.update_pr_for_task.call_args.kwargs + assert call_kwargs["title"] == "t" + assert call_kwargs["body"] == "b" + assert call_kwargs["reviewers"] == ["be-dev-2"] + assert set(body["evidence"]["updated_fields"]) == {"title", "body", "reviewers"} + + +@pytest.mark.asyncio +async def test_pr_update_cell_pm_on_same_team_allowed() -> None: + """Cell PM whose team == task.team can update the PR (PM authority).""" + pm_id = uuid4() + assignee_id = uuid4() + task = _task(assigned_to=assignee_id, team="backend") + task_svc = AsyncMock() + task_svc.get.return_value = task + task_svc.agent_for.return_value = _agent("cell_pm", "backend") + + git_svc = AsyncMock() + git_svc.update_pr_for_task.return_value = { + "pr_number": 7, + "pr_url": task.pr_url, + "updated_fields": ["title"], + } + deps = _make_deps(task=task_svc, git=git_svc) + ca = ContentActions(deps) + + env = await ca.pr_update( + agent_id=pm_id, task_id=task.id, title="t", body=None, reviewers=None + ) + body = env.as_dict() + + assert body["error"] is None + git_svc.update_pr_for_task.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_pr_update_cell_pm_on_other_team_rejected() -> None: + """Cell PM on a different team than the task → not_authorized.""" + pm_id = uuid4() + assignee_id = uuid4() + task = _task(assigned_to=assignee_id, team="backend") + task_svc = AsyncMock() + task_svc.get.return_value = task + task_svc.agent_for.return_value = _agent("cell_pm", "frontend") + + deps = _make_deps(task=task_svc) + ca = ContentActions(deps) + + env = await ca.pr_update( + agent_id=pm_id, task_id=task.id, title="t", body=None, reviewers=None + ) + + assert env.as_dict()["error"] == "not_authorized" + deps.git.update_pr_for_task.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_pr_update_main_pm_any_team_allowed() -> None: + """Main PM is cross-team and may update any task's PR.""" + pm_id = uuid4() + assignee_id = uuid4() + task = _task(assigned_to=assignee_id, team="backend") + task_svc = AsyncMock() + task_svc.get.return_value = task + task_svc.agent_for.return_value = _agent("main_pm", team=None) + + git_svc = AsyncMock() + git_svc.update_pr_for_task.return_value = { + "pr_number": 7, + "pr_url": task.pr_url, + "updated_fields": ["title"], + } + deps = _make_deps(task=task_svc, git=git_svc) + ca = ContentActions(deps) + + env = await ca.pr_update( + agent_id=pm_id, task_id=task.id, title="t", body=None, reviewers=None + ) + + assert env.as_dict()["error"] is None + git_svc.update_pr_for_task.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_pr_update_task_not_found_returns_not_found() -> None: + """Unknown task_id → not_found envelope; git layer never invoked.""" + agent_id = uuid4() + task_svc = AsyncMock() + task_svc.get.return_value = None + deps = _make_deps(task=task_svc) + ca = ContentActions(deps) + + env = await ca.pr_update( + agent_id=agent_id, task_id=uuid4(), title="t", body=None, reviewers=None + ) + body = env.as_dict() + + assert body["error"] == "not_found" + deps.git.update_pr_for_task.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_pr_update_git_error_returned_as_invalid_state() -> None: + """GitService raises GitError → mapped to invalid_state envelope with detail.""" + agent_id = uuid4() + task = _task(assigned_to=agent_id) + task_svc = AsyncMock() + task_svc.get.return_value = task + task_svc.agent_for.return_value = _agent("developer", "backend") + + git_svc = AsyncMock() + git_svc.update_pr_for_task.side_effect = GitError("PR not found: #7 on acme/repo") + deps = _make_deps(task=task_svc, git=git_svc) + ca = ContentActions(deps) + + env = await ca.pr_update( + agent_id=agent_id, task_id=task.id, title="t", body=None, reviewers=None + ) + body = env.as_dict() + + assert body["error"] == "invalid_state" + assert "PR not found" in body["message"]