feat(git): add update_pr_for_task + PRUpdateRequest schema

Smoke-5 surfaced that be-dev-1 had no gateway-native way to fix a
PR's title/body or request a reviewer after open_pr; `gh pr edit`
is bash-shimmed and the dev correctly escalated rather than bypass
the guard. This adds the GitService primitive: PATCH /pulls/{n}
for title/body and POST /pulls/{n}/requested_reviewers for the
reviewer list, with NotFound + 422 mapped to typed GitError. The
PRUpdateRequest schema enforces 'at least one field' via a
model_validator so the route returns 422 before reaching the verb.
This commit is contained in:
Renn F
2026-05-14 04:28:37 +02:00
parent 1bd6eb3372
commit b5d3d13346
3 changed files with 490 additions and 1 deletions
+32 -1
View File
@@ -1,8 +1,9 @@
"""Request schemas for /api/v2/do/* content tools."""
from typing import Self
from uuid import UUID
from pydantic import BaseModel, Field
from pydantic import BaseModel, Field, model_validator
class CommitRequest(BaseModel):
@@ -125,3 +126,33 @@ class NotifyAckRequest(BaseModel):
class ChannelsRequest(BaseModel):
"""No params — caller's identity comes from X-Agent-ID header."""
class PRUpdateRequest(BaseModel):
"""Update an open PR's title/body and/or request reviewers.
Smoke-5 surfaced the gap: agents who needed to fix the PR title or
request a reviewer after `open_pr` had no verb for it and got blocked
by the bash-guard on `gh pr edit`. This is the gateway-native fix.
At least one of `title`, `body`, or `reviewers` must be provided —
enforced via a `model_validator` so the route returns 422 before the
request ever reaches `ContentActions`.
`reviewers` is a list of agent slugs (e.g. `["be-dev-2", "be-qa"]`);
the gateway maps to GitHub usernames where the project records that
mapping, otherwise the slugs go through as-is.
"""
task_id: UUID
title: str | None = None
body: str | None = None
reviewers: list[str] | None = None
@model_validator(mode="after")
def _at_least_one_field(self) -> Self:
if self.title is None and self.body is None and self.reviewers is None:
raise ValueError(
"at least one of title, body, or reviewers must be provided"
)
return self
+149
View File
@@ -80,6 +80,10 @@ _REV_LIST_PARTS = 2
# GitHub REST API status codes
_GH_UNPROCESSABLE = 422
# 404 means the PR (or repo) does not exist; surfaced as a typed GitError
# by `update_pr_for_task` so the gateway can convert it into a specific
# invalid_state envelope rather than the generic refusal message.
_HTTP_NOT_FOUND = 404
# 409 means the PR can't be merged in its current state — typically because
# a concurrent sibling-subtask merge updated the target branch and our local
# refs are stale. `pr_merge` re-syncs and retries exactly once on this code.
@@ -1262,6 +1266,151 @@ class GitService(BaseService):
target_branch,
)
async def _patch_pr_title_body(
self,
owner: str,
repo: str,
pr_number: int,
git_token: str,
payload: dict[str, str],
) -> None:
"""PATCH /repos/{owner}/{repo}/pulls/{pr_number} with title/body.
Translates HTTP failures into GitError so the verb layer can map
them onto invalid_state envelopes. 404 → "PR not found"; any
other non-2xx surfaces the GitHub validation text inline.
"""
try:
async with httpx.AsyncClient(timeout=_GIT_TIMEOUT) as client:
resp = await client.patch(
f"https://api.github.com/repos/{owner}/{repo}/pulls/{pr_number}",
headers={
"Authorization": f"Bearer {git_token}",
"Accept": "application/vnd.github+json",
"X-GitHub-Api-Version": "2022-11-28",
},
json=payload,
)
except httpx.HTTPError as e:
raise GitError(
f"GitHub API error while updating PR #{pr_number}: {e}",
{"owner": owner, "repo": repo, "pr": pr_number},
) from e
if resp.status_code == _HTTP_NOT_FOUND:
raise GitError(
f"PR not found: #{pr_number} on {owner}/{repo}",
{"owner": owner, "repo": repo, "pr": pr_number},
)
if not resp.is_success:
raise GitError(
f"GitHub API refused PR update ({resp.status_code}): {resp.text[:200]}",
{"owner": owner, "repo": repo, "pr": pr_number},
)
async def _post_pr_reviewers(
self,
owner: str,
repo: str,
pr_number: int,
git_token: str,
reviewers: list[str],
) -> None:
"""POST /repos/{owner}/{repo}/pulls/{pr_number}/requested_reviewers.
Mirrors `_patch_pr_title_body` error handling. The reviewers list
is passed through verbatim — caller is responsible for mapping
agent slugs onto GitHub usernames where the project records that.
"""
try:
async with httpx.AsyncClient(timeout=_GIT_TIMEOUT) as client:
resp = await client.post(
f"https://api.github.com/repos/{owner}/{repo}/pulls/"
f"{pr_number}/requested_reviewers",
headers={
"Authorization": f"Bearer {git_token}",
"Accept": "application/vnd.github+json",
"X-GitHub-Api-Version": "2022-11-28",
},
json={"reviewers": reviewers},
)
except httpx.HTTPError as e:
raise GitError(
f"GitHub API error while adding reviewers to PR #{pr_number}: {e}",
{"owner": owner, "repo": repo, "pr": pr_number},
) from e
if resp.status_code == _HTTP_NOT_FOUND:
raise GitError(
f"PR not found: #{pr_number} on {owner}/{repo}",
{"owner": owner, "repo": repo, "pr": pr_number},
)
if not resp.is_success:
raise GitError(
f"GitHub API refused reviewer request ({resp.status_code}): "
f"{resp.text[:200]}",
{"owner": owner, "repo": repo, "pr": pr_number},
)
async def update_pr_for_task(
self,
task_id: UUID,
*,
title: str | None = None,
body: str | None = None,
reviewers: list[str] | None = None,
) -> dict[str, Any]:
"""Update an open PR's title/body and/or request reviewers.
Looks up the task, resolves the project's GitHub PAT, and routes
through `_patch_pr_title_body` (when title or body is set) and
`_post_pr_reviewers` (when reviewers is set). Either or both run;
the verb layer guarantees at least one is provided.
Returns a dict with `pr_number`, `pr_url`, and a `updated_fields`
list naming which of title/body/reviewers actually went out.
"""
task_service = get_task_service(self.session)
task = await task_service.get(task_id)
if task is None:
raise NotFoundError("Task", str(task_id))
if task.pr_number is None:
raise GitError(
f"Task {task_id} has no PR open; cannot update.",
{"task_id": str(task_id)},
)
project_service = get_project_service(self.session)
project = await project_service.get(UUID(str(task.project_id)))
if project is None:
raise NotFoundError("Project", str(task.project_id))
workspace_agent_id = self._resolve_workspace_agent_id(task, None)
workspace = await self.get_workspace(project.slug, agent_id=workspace_agent_id)
owner, repo = self._parse_github_remote(workspace)
git_token = await self._get_project_token_or_raise(project.slug)
pr_number = int(task.pr_number)
updated: list[str] = []
patch_payload: dict[str, str] = {}
if title is not None:
patch_payload["title"] = title
updated.append("title")
if body is not None:
patch_payload["body"] = body
updated.append("body")
if patch_payload:
await self._patch_pr_title_body(
owner, repo, pr_number, git_token, patch_payload
)
if reviewers is not None:
await self._post_pr_reviewers(owner, repo, pr_number, git_token, reviewers)
updated.append("reviewers")
return {
"pr_number": pr_number,
"pr_url": str(task.pr_url) if task.pr_url else "",
"updated_fields": updated,
}
_PR_OPEN_STATES: ClassVar[frozenset[str]] = frozenset(
{
TaskStatus.IN_PROGRESS.value,
+309
View File
@@ -0,0 +1,309 @@
"""Unit tests for GitService.update_pr_for_task.
Covers PATCH (title/body) + POST reviewers (requested_reviewers) round-trips
against GitHub's REST API. httpx is fully mocked — no real network.
"""
from __future__ import annotations
from pathlib import Path
from typing import TYPE_CHECKING, Any
from unittest.mock import AsyncMock, MagicMock, patch
from uuid import UUID, uuid4
import pytest
from roboco.exceptions import GitError
from roboco.services.base import NotFoundError
from roboco.services.git import GitService
if TYPE_CHECKING:
from contextlib import AbstractContextManager
_PR_NUMBER = 42
_HTTP_NOT_FOUND = 404
_HTTP_UNPROCESSABLE = 422
def _make_session() -> MagicMock:
session = MagicMock()
session.execute = AsyncMock()
session.commit = AsyncMock()
session.rollback = AsyncMock()
return session
def _service() -> GitService:
return GitService(_make_session())
def _patch_project_service(project: object | None) -> AbstractContextManager[object]:
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 _bind(svc: GitService, name: str, value: object) -> None:
object.__setattr__(svc, name, value)
def _task_with_pr(pr_number: int | None = _PR_NUMBER, pr_url: str = "") -> MagicMock:
"""Build a TaskTable-shaped MagicMock with a pr_number set."""
project_id = uuid4()
return MagicMock(
id=uuid4(),
project_id=project_id,
pr_number=pr_number,
pr_url=pr_url or f"https://github.com/acme/repo/pull/{pr_number}",
assigned_to=uuid4(),
created_by=uuid4(),
)
def _make_http_response(
*, status_code: int, json_payload: dict[str, Any] | None = None, text: str = ""
) -> MagicMock:
resp = MagicMock()
resp.status_code = status_code
resp.is_success = 200 <= status_code < 300 # noqa: PLR2004
resp.json.return_value = json_payload or {}
resp.text = text
return resp
def _make_async_client(
*,
patch_resp: MagicMock | None = None,
post_resp: MagicMock | None = None,
) -> MagicMock:
client = MagicMock()
client.__aenter__ = AsyncMock(return_value=client)
client.__aexit__ = AsyncMock(return_value=False)
client.patch = AsyncMock(return_value=patch_resp)
client.post = AsyncMock(return_value=post_resp)
return client
async def _stub_task_get(svc: GitService, task: object | None) -> None:
task_service = MagicMock()
task_service.get = AsyncMock(return_value=task)
_bind(svc, "_task_service_for_pr_update", task_service)
def _wire_service(svc: GitService, task: MagicMock) -> None:
"""Apply common bindings: workspace, remote parse, token resolution."""
_bind(svc, "get_workspace", AsyncMock(return_value=Path("/tmp/ws")))
_bind(svc, "_parse_github_remote", MagicMock(return_value=("acme", "repo")))
_bind(svc, "_get_project_token_or_raise", AsyncMock(return_value="tok"))
# update_pr_for_task fetches the task via get_task_service; we patch it
# at the module level.
fake_task_service = MagicMock()
fake_task_service.get = AsyncMock(return_value=task)
return fake_task_service # type: ignore[return-value]
@pytest.mark.asyncio
async def test_update_pr_title_only_calls_patch_with_title() -> None:
"""title-only call PATCHes /pulls/{n} with {title: ...} and no reviewers POST."""
svc = _service()
task = _task_with_pr()
fake_task_service = _wire_service(svc, task)
fake_project = MagicMock(slug="roboco")
patch_resp = _make_http_response(
status_code=200,
json_payload={
"number": _PR_NUMBER,
"html_url": task.pr_url,
"title": "new title",
},
)
fake_client = _make_async_client(patch_resp=patch_resp)
with (
patch("roboco.services.git.get_task_service", return_value=fake_task_service),
_patch_project_service(fake_project),
patch("roboco.services.git.httpx.AsyncClient", return_value=fake_client),
):
out = await svc.update_pr_for_task(
UUID(str(task.id)), title="new title", body=None, reviewers=None
)
fake_client.patch.assert_awaited_once()
fake_client.post.assert_not_awaited()
call = fake_client.patch.await_args
assert f"/pulls/{_PR_NUMBER}" in call.args[0]
assert call.kwargs["json"] == {"title": "new title"}
assert out["pr_number"] == _PR_NUMBER
assert out["updated_fields"] == ["title"]
@pytest.mark.asyncio
async def test_update_pr_title_and_body_calls_patch_with_both() -> None:
"""Both title and body in the same PATCH payload."""
svc = _service()
task = _task_with_pr()
fake_task_service = _wire_service(svc, task)
fake_project = MagicMock(slug="roboco")
patch_resp = _make_http_response(
status_code=200,
json_payload={"number": _PR_NUMBER, "html_url": task.pr_url},
)
fake_client = _make_async_client(patch_resp=patch_resp)
with (
patch("roboco.services.git.get_task_service", return_value=fake_task_service),
_patch_project_service(fake_project),
patch("roboco.services.git.httpx.AsyncClient", return_value=fake_client),
):
out = await svc.update_pr_for_task(
UUID(str(task.id)), title="t", body="b", reviewers=None
)
fake_client.patch.assert_awaited_once()
call = fake_client.patch.await_args
assert call.kwargs["json"] == {"title": "t", "body": "b"}
assert set(out["updated_fields"]) == {"title", "body"}
@pytest.mark.asyncio
async def test_update_pr_reviewers_only_calls_post_reviewers() -> None:
"""reviewers-only call POSTs to /pulls/{n}/requested_reviewers, skips PATCH."""
svc = _service()
task = _task_with_pr()
fake_task_service = _wire_service(svc, task)
fake_project = MagicMock(slug="roboco")
post_resp = _make_http_response(
status_code=201,
json_payload={"number": _PR_NUMBER, "html_url": task.pr_url},
)
fake_client = _make_async_client(post_resp=post_resp)
with (
patch("roboco.services.git.get_task_service", return_value=fake_task_service),
_patch_project_service(fake_project),
patch("roboco.services.git.httpx.AsyncClient", return_value=fake_client),
):
out = await svc.update_pr_for_task(
UUID(str(task.id)),
title=None,
body=None,
reviewers=["be-dev-2", "be-qa"],
)
fake_client.patch.assert_not_awaited()
fake_client.post.assert_awaited_once()
call = fake_client.post.await_args
assert f"/pulls/{_PR_NUMBER}/requested_reviewers" in call.args[0]
assert call.kwargs["json"] == {"reviewers": ["be-dev-2", "be-qa"]}
assert out["updated_fields"] == ["reviewers"]
@pytest.mark.asyncio
async def test_update_pr_all_three_fields_forwarded() -> None:
"""title + body + reviewers → one PATCH + one POST, both fields reported."""
svc = _service()
task = _task_with_pr()
fake_task_service = _wire_service(svc, task)
fake_project = MagicMock(slug="roboco")
patch_resp = _make_http_response(
status_code=200, json_payload={"number": _PR_NUMBER, "html_url": task.pr_url}
)
post_resp = _make_http_response(
status_code=201, json_payload={"number": _PR_NUMBER, "html_url": task.pr_url}
)
fake_client = _make_async_client(patch_resp=patch_resp, post_resp=post_resp)
with (
patch("roboco.services.git.get_task_service", return_value=fake_task_service),
_patch_project_service(fake_project),
patch("roboco.services.git.httpx.AsyncClient", return_value=fake_client),
):
out = await svc.update_pr_for_task(
UUID(str(task.id)), title="t", body="b", reviewers=["be-dev-2"]
)
fake_client.patch.assert_awaited_once()
fake_client.post.assert_awaited_once()
assert set(out["updated_fields"]) == {"title", "body", "reviewers"}
@pytest.mark.asyncio
async def test_update_pr_404_raises_pr_not_found() -> None:
"""HTTP 404 from PATCH → GitError mentioning PR not found."""
svc = _service()
task = _task_with_pr()
fake_task_service = _wire_service(svc, task)
fake_project = MagicMock(slug="roboco")
patch_resp = _make_http_response(status_code=_HTTP_NOT_FOUND, text="Not Found")
fake_client = _make_async_client(patch_resp=patch_resp)
with (
patch("roboco.services.git.get_task_service", return_value=fake_task_service),
_patch_project_service(fake_project),
patch("roboco.services.git.httpx.AsyncClient", return_value=fake_client),
pytest.raises(GitError, match="PR not found"),
):
await svc.update_pr_for_task(
UUID(str(task.id)), title="t", body=None, reviewers=None
)
@pytest.mark.asyncio
async def test_update_pr_422_raises_with_validation_message() -> None:
"""HTTP 422 from PATCH → GitError surfacing the validation text."""
svc = _service()
task = _task_with_pr()
fake_task_service = _wire_service(svc, task)
fake_project = MagicMock(slug="roboco")
patch_resp = _make_http_response(
status_code=_HTTP_UNPROCESSABLE, text="Validation Failed: body too long"
)
fake_client = _make_async_client(patch_resp=patch_resp)
with (
patch("roboco.services.git.get_task_service", return_value=fake_task_service),
_patch_project_service(fake_project),
patch("roboco.services.git.httpx.AsyncClient", return_value=fake_client),
pytest.raises(GitError, match="Validation Failed"),
):
await svc.update_pr_for_task(
UUID(str(task.id)), title=None, body="long body", reviewers=None
)
@pytest.mark.asyncio
async def test_update_pr_task_missing_raises_not_found() -> None:
"""Unknown task_id → NotFoundError (caller maps to invalid_state envelope)."""
svc = _service()
fake_task_service = MagicMock()
fake_task_service.get = AsyncMock(return_value=None)
with (
patch("roboco.services.git.get_task_service", return_value=fake_task_service),
pytest.raises(NotFoundError),
):
await svc.update_pr_for_task(uuid4(), title="t", body=None, reviewers=None)
@pytest.mark.asyncio
async def test_update_pr_no_pr_number_raises_git_error() -> None:
"""Task exists but has no pr_number → GitError ('no PR open')."""
svc = _service()
task = _task_with_pr(pr_number=None)
fake_task_service = MagicMock()
fake_task_service.get = AsyncMock(return_value=task)
with (
patch("roboco.services.git.get_task_service", return_value=fake_task_service),
pytest.raises(GitError, match="no PR"),
):
await svc.update_pr_for_task(
UUID(str(task.id)), title="t", body=None, reviewers=None
)