mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(git): cancel closes the task's open PR; bulk cleanup spares live dependents (#593)
Task cancellation left the task's PR open on the forge forever: cancel() now best-effort-closes the recorded PR for the task and its cascaded descendants (close_task_pr_best_effort resolves owner/repo off git_url — no clone needed; never raises into the cancel). The bulk stale-branch sweep gains a dependents guard: a branch still recorded by a non-terminal task, or serving as a live child's resolve_parent_branch base, is excluded from the candidate window — mirroring the existing env-ladder-rung skip. Scoped to the sweep, not delete_task_branch, so the BFS cascade-cancel can't falsely block a parent's branch on its own about-to-cancel child. Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
Renzo F
@@ -165,6 +165,61 @@ async def test_env_ladder_branch_is_excluded(cleanup_setup: dict[str, Any]) -> N
|
||||
assert call.args[1] == "feature/backend/real-task"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_live_child_branch_dependent_is_excluded(
|
||||
cleanup_setup: dict[str, Any],
|
||||
) -> None:
|
||||
"""GAP B: a completed root's branch is still the merge base a live cell
|
||||
task's PR would target (``resolve_parent_branch`` reads the parent's own
|
||||
``branch_name``) — deleting it out from under an in-progress child that
|
||||
hasn't opened a PR yet (so ``_branch_has_open_dependents`` can't see it)
|
||||
would strand the child. The sweep must skip it."""
|
||||
root = _task(
|
||||
cleanup_setup, branch="feature/main_pm/root", status=TaskStatus.COMPLETED
|
||||
)
|
||||
await cleanup_setup["db"].flush()
|
||||
child = _task(
|
||||
cleanup_setup,
|
||||
branch="feature/backend/root--cell",
|
||||
status=TaskStatus.IN_PROGRESS,
|
||||
)
|
||||
child.parent_task_id = root.id
|
||||
_task(
|
||||
cleanup_setup, branch="feature/backend/unrelated", status=TaskStatus.COMPLETED
|
||||
)
|
||||
await cleanup_setup["db"].flush()
|
||||
|
||||
result = await cleanup_setup["svc"].cleanup_stale_branches(
|
||||
cleanup_setup["project"].slug
|
||||
)
|
||||
|
||||
# Only the unrelated completed task's branch is a candidate — the root's
|
||||
# branch is still load-bearing for its live child.
|
||||
assert result == (1, 1, 0, 0, False, None)
|
||||
call = cleanup_setup["ws_svc"].delete_local_branch.await_args
|
||||
assert call is not None
|
||||
assert call.args[1] == "feature/backend/unrelated"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_branch_still_claimed_by_a_live_task_is_excluded(
|
||||
cleanup_setup: dict[str, Any],
|
||||
) -> None:
|
||||
"""Defensive case: a NON-terminal task still recording this exact branch
|
||||
as its own must never be swept out from under it, even though the
|
||||
candidate is a *different*, terminal task row."""
|
||||
_task(cleanup_setup, branch="feature/backend/reused", status=TaskStatus.COMPLETED)
|
||||
_task(cleanup_setup, branch="feature/backend/reused", status=TaskStatus.IN_PROGRESS)
|
||||
await cleanup_setup["db"].flush()
|
||||
|
||||
result = await cleanup_setup["svc"].cleanup_stale_branches(
|
||||
cleanup_setup["project"].slug
|
||||
)
|
||||
|
||||
assert result == (0, 0, 0, 0, False, None)
|
||||
cleanup_setup["ws_svc"].delete_local_branch.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cancelled_task_force_deletes_local_branch(
|
||||
cleanup_setup: dict[str, Any],
|
||||
|
||||
@@ -729,6 +729,7 @@ async def test_cancel_with_branch_and_work_session(
|
||||
)
|
||||
fake_git = MagicMock()
|
||||
fake_git.delete_task_branch = AsyncMock()
|
||||
fake_git.close_task_pr_best_effort = AsyncMock()
|
||||
monkeypatch.setattr("roboco.services.git.get_git_service", lambda _s: fake_git)
|
||||
out = await svc.cancel(task.id, agent_role="cell_pm")
|
||||
assert out is not None
|
||||
@@ -736,6 +737,57 @@ async def test_cancel_with_branch_and_work_session(
|
||||
fake_git.delete_task_branch.assert_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cancel_closes_open_pr(
|
||||
task_setup: dict,
|
||||
db_session: AsyncSession,
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""GAP A: cancelling a task with an open PR closes it on the forge —
|
||||
previously ``cancel`` force-deleted the branch/worktree but left
|
||||
``pr_number`` PRs open forever."""
|
||||
svc = task_setup["svc"]
|
||||
task = await svc.create(_req(task_setup))
|
||||
task.branch_name = "feature/backend/x"
|
||||
task.pr_number = 42
|
||||
task.pr_url = "https://example.com/r/pull/42"
|
||||
await db_session.flush()
|
||||
|
||||
fake_git = MagicMock()
|
||||
fake_git.delete_task_branch = AsyncMock()
|
||||
fake_git.close_task_pr_best_effort = AsyncMock()
|
||||
monkeypatch.setattr("roboco.services.git.get_git_service", lambda _s: fake_git)
|
||||
|
||||
out = await svc.cancel(task.id, agent_role="cell_pm")
|
||||
|
||||
assert out is not None
|
||||
fake_git.close_task_pr_best_effort.assert_awaited_once_with(
|
||||
task_setup["project_slug"], 42
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cancel_skips_pr_close_without_pr_number(
|
||||
task_setup: dict,
|
||||
db_session: AsyncSession,
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
svc = task_setup["svc"]
|
||||
task = await svc.create(_req(task_setup))
|
||||
task.branch_name = "feature/backend/x"
|
||||
await db_session.flush()
|
||||
|
||||
fake_git = MagicMock()
|
||||
fake_git.delete_task_branch = AsyncMock()
|
||||
fake_git.close_task_pr_best_effort = AsyncMock()
|
||||
monkeypatch.setattr("roboco.services.git.get_git_service", lambda _s: fake_git)
|
||||
|
||||
out = await svc.cancel(task.id, agent_role="cell_pm")
|
||||
|
||||
assert out is not None
|
||||
fake_git.close_task_pr_best_effort.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cancel_descendants_cascades_for_authorized_pm(
|
||||
task_setup: dict, db_session: AsyncSession
|
||||
|
||||
@@ -0,0 +1,172 @@
|
||||
"""GitService.close_task_pr_best_effort (GAP A) — cancellation never closed
|
||||
a task's own PR. Mirrors ``delete_task_branch``: resolves owner/repo
|
||||
straight off the project's ``git_url``, no workspace/clone needed, so it's
|
||||
safe to call from the cancel chokepoint for any task — assigned or not.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from types import SimpleNamespace
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import httpx
|
||||
import pytest
|
||||
import roboco.services.git as git_module
|
||||
from roboco.services.git import GitService
|
||||
|
||||
_PR_NUMBER = 42
|
||||
|
||||
|
||||
def _bind(svc: GitService, name: str, value: object) -> None:
|
||||
setattr(svc, name, value)
|
||||
|
||||
|
||||
def _service() -> GitService:
|
||||
svc = GitService.__new__(GitService)
|
||||
_bind(svc, "log", MagicMock())
|
||||
_bind(svc, "session", MagicMock())
|
||||
return svc
|
||||
|
||||
|
||||
def _wire_project(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
*,
|
||||
git_url: str | None = "https://github.com/acme/repo.git",
|
||||
) -> None:
|
||||
project = SimpleNamespace(git_url=git_url) if git_url else None
|
||||
project_svc = MagicMock(get_by_slug=AsyncMock(return_value=project))
|
||||
monkeypatch.setattr(git_module, "get_project_service", lambda _s: project_svc)
|
||||
|
||||
|
||||
def _resp(*, status_code: int = 200, json_payload: dict[str, Any] | None = None) -> Any:
|
||||
resp = MagicMock()
|
||||
resp.status_code = status_code
|
||||
resp.is_success = 200 <= status_code < 300 # noqa: PLR2004
|
||||
resp.json.return_value = json_payload or {}
|
||||
return resp
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_closes_an_open_pr(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
svc = _service()
|
||||
_bind(svc, "_token_for_project", AsyncMock(return_value="tok"))
|
||||
_wire_project(monkeypatch)
|
||||
|
||||
forge = MagicMock(
|
||||
get_pr=AsyncMock(return_value=_resp(json_payload={"state": "open"})),
|
||||
update_pr=AsyncMock(return_value=_resp()),
|
||||
)
|
||||
monkeypatch.setattr(GitService, "_forge", property(lambda _self: forge))
|
||||
|
||||
out = await svc.close_task_pr_best_effort("roboco", _PR_NUMBER)
|
||||
|
||||
assert out is True
|
||||
forge.update_pr.assert_awaited_once()
|
||||
call = forge.update_pr.await_args
|
||||
assert call.args[2] == _PR_NUMBER
|
||||
assert call.kwargs["payload"] == {"state": "closed"}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_already_closed_pr_is_a_noop(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
svc = _service()
|
||||
_bind(svc, "_token_for_project", AsyncMock(return_value="tok"))
|
||||
_wire_project(monkeypatch)
|
||||
|
||||
forge = MagicMock(
|
||||
get_pr=AsyncMock(return_value=_resp(json_payload={"state": "closed"})),
|
||||
update_pr=AsyncMock(),
|
||||
)
|
||||
monkeypatch.setattr(GitService, "_forge", property(lambda _self: forge))
|
||||
|
||||
out = await svc.close_task_pr_best_effort("roboco", _PR_NUMBER)
|
||||
|
||||
assert out is False
|
||||
forge.update_pr.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_merged_pr_is_a_noop(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
# A merged PR also reports state="closed" on GitHub — never re-close it.
|
||||
svc = _service()
|
||||
_bind(svc, "_token_for_project", AsyncMock(return_value="tok"))
|
||||
_wire_project(monkeypatch)
|
||||
|
||||
forge = MagicMock(
|
||||
get_pr=AsyncMock(
|
||||
return_value=_resp(json_payload={"state": "closed", "merged": True})
|
||||
),
|
||||
update_pr=AsyncMock(),
|
||||
)
|
||||
monkeypatch.setattr(GitService, "_forge", property(lambda _self: forge))
|
||||
|
||||
out = await svc.close_task_pr_best_effort("roboco", _PR_NUMBER)
|
||||
|
||||
assert out is False
|
||||
forge.update_pr.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_token_is_a_noop(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
svc = _service()
|
||||
_bind(svc, "_token_for_project", AsyncMock(return_value=None))
|
||||
forge = MagicMock(get_pr=AsyncMock(), update_pr=AsyncMock())
|
||||
monkeypatch.setattr(GitService, "_forge", property(lambda _self: forge))
|
||||
|
||||
out = await svc.close_task_pr_best_effort("roboco", _PR_NUMBER)
|
||||
|
||||
assert out is False
|
||||
forge.get_pr.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_project_git_url_is_a_noop(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
svc = _service()
|
||||
_bind(svc, "_token_for_project", AsyncMock(return_value="tok"))
|
||||
_wire_project(monkeypatch, git_url=None)
|
||||
forge = MagicMock(get_pr=AsyncMock(), update_pr=AsyncMock())
|
||||
monkeypatch.setattr(GitService, "_forge", property(lambda _self: forge))
|
||||
|
||||
out = await svc.close_task_pr_best_effort("roboco", _PR_NUMBER)
|
||||
|
||||
assert out is False
|
||||
forge.get_pr.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_lookup_transport_error_is_swallowed(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
svc = _service()
|
||||
_bind(svc, "_token_for_project", AsyncMock(return_value="tok"))
|
||||
_wire_project(monkeypatch)
|
||||
|
||||
forge = MagicMock(
|
||||
get_pr=AsyncMock(side_effect=httpx.HTTPError("boom")),
|
||||
update_pr=AsyncMock(),
|
||||
)
|
||||
monkeypatch.setattr(GitService, "_forge", property(lambda _self: forge))
|
||||
|
||||
out = await svc.close_task_pr_best_effort("roboco", _PR_NUMBER)
|
||||
|
||||
assert out is False
|
||||
forge.update_pr.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_pr_non_success_is_a_noop(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
svc = _service()
|
||||
_bind(svc, "_token_for_project", AsyncMock(return_value="tok"))
|
||||
_wire_project(monkeypatch)
|
||||
|
||||
forge = MagicMock(
|
||||
get_pr=AsyncMock(return_value=_resp(status_code=404)),
|
||||
update_pr=AsyncMock(),
|
||||
)
|
||||
monkeypatch.setattr(GitService, "_forge", property(lambda _self: forge))
|
||||
|
||||
out = await svc.close_task_pr_best_effort("roboco", _PR_NUMBER)
|
||||
|
||||
assert out is False
|
||||
forge.update_pr.assert_not_awaited()
|
||||
@@ -0,0 +1,114 @@
|
||||
"""Cancel closes the task's own open PR (GAP A).
|
||||
|
||||
``_delete_task_branch_best_effort`` already force-deletes the task's remote
|
||||
branch + worktree on cancel, but an open PR (``task.pr_number`` set) was left
|
||||
open on the forge forever — nothing ever closed it. This is the isolated
|
||||
unit test for the new ``_close_task_pr_best_effort`` best-effort chokepoint;
|
||||
``tests/integration/test_task_service_lifecycle_misc.py`` covers the full
|
||||
``cancel()`` wiring against a real DB.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.services.task import TaskService
|
||||
|
||||
|
||||
def _service() -> TaskService:
|
||||
svc = TaskService.__new__(TaskService)
|
||||
svc.log = MagicMock()
|
||||
svc.session = MagicMock()
|
||||
return svc
|
||||
|
||||
|
||||
def _session(slug: str | None) -> MagicMock:
|
||||
session = MagicMock()
|
||||
result = MagicMock()
|
||||
result.scalar_one_or_none.return_value = slug
|
||||
session.execute = AsyncMock(return_value=result)
|
||||
return session
|
||||
|
||||
|
||||
def _task(*, pr_number: int | None) -> MagicMock:
|
||||
return MagicMock(id=uuid4(), project_id=uuid4(), pr_number=pr_number)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_closes_open_pr_via_git_service() -> None:
|
||||
svc = _service()
|
||||
task = _task(pr_number=42)
|
||||
svc.session = _session("roboco-api")
|
||||
|
||||
git_service = MagicMock()
|
||||
git_service.close_task_pr_best_effort = AsyncMock()
|
||||
|
||||
with patch(
|
||||
"roboco.services.git.get_git_service",
|
||||
MagicMock(return_value=git_service),
|
||||
):
|
||||
await svc._close_task_pr_best_effort(task)
|
||||
|
||||
git_service.close_task_pr_best_effort.assert_awaited_once_with("roboco-api", 42)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_skips_when_no_pr_number() -> None:
|
||||
# Task never opened a PR (or was cancelled before claim) — nothing to
|
||||
# close, and no project lookup should even fire.
|
||||
svc = _service()
|
||||
task = _task(pr_number=None)
|
||||
svc.session = _session("roboco-api")
|
||||
|
||||
git_service = MagicMock()
|
||||
git_service.close_task_pr_best_effort = AsyncMock()
|
||||
|
||||
with patch(
|
||||
"roboco.services.git.get_git_service",
|
||||
MagicMock(return_value=git_service),
|
||||
):
|
||||
await svc._close_task_pr_best_effort(task)
|
||||
|
||||
git_service.close_task_pr_best_effort.assert_not_awaited()
|
||||
svc.session.execute.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_skips_when_project_slug_unresolvable() -> None:
|
||||
svc = _service()
|
||||
task = _task(pr_number=42)
|
||||
svc.session = _session(None)
|
||||
|
||||
git_service = MagicMock()
|
||||
git_service.close_task_pr_best_effort = AsyncMock()
|
||||
|
||||
with patch(
|
||||
"roboco.services.git.get_git_service",
|
||||
MagicMock(return_value=git_service),
|
||||
):
|
||||
await svc._close_task_pr_best_effort(task)
|
||||
|
||||
git_service.close_task_pr_best_effort.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pr_close_failure_does_not_raise() -> None:
|
||||
# Best-effort: a forge failure logs and never blocks the cancellation.
|
||||
svc = _service()
|
||||
task = _task(pr_number=42)
|
||||
svc.session = _session("roboco-api")
|
||||
|
||||
git_service = MagicMock()
|
||||
git_service.close_task_pr_best_effort = AsyncMock(
|
||||
side_effect=RuntimeError("forge unreachable")
|
||||
)
|
||||
|
||||
with patch(
|
||||
"roboco.services.git.get_git_service",
|
||||
MagicMock(return_value=git_service),
|
||||
):
|
||||
await svc._close_task_pr_best_effort(task) # must not raise
|
||||
|
||||
svc.log.warning.assert_called_once()
|
||||
Reference in New Issue
Block a user