Files
roboco/tests/unit/services/test_git_branch_deletion_guard.py
T
2c403c77a2 Fix/run hardening prep (#263)
* fix(git): don't delete a branch that still has open dependent PRs

Root cause of the run-zombifying "integration branch gone from origin" wedge.
_delete_remote_branch_best_effort deleted a merged PR's head branch
unconditionally, so:
- merging a cell->root PR deleted the cell branch while a sibling leaf PR was
  still targeting it as base, and
- the CEO's root->master merge deleted the feature/main_pm/{root} integration
  branch.
The dependent PRs lost their base, every later git op against the vanished
branch failed, and the task zombified (a51c3d31 only made the post-merge sync
non-fatal; this removes the cause).

The remote-branch delete chokepoint (the single path all merge/close/cancel
deletions funnel through) now first checks _branch_has_open_dependents: any OPEN
PR targeting the branch as its base marks it an active integration target and
preserves it. Fails safe (any error => keep the branch; cleanup is best-effort,
stranding is not). True leaf branches with no open dependents are still cleaned
up. Adds 6 unit tests for the guard + the probe.

* fix(git): recover a drifted shared clone on resume instead of BRANCH_MISMATCH

A dev/documenter/QA clone is shared across that agent's tasks. On a
respawn/resume it can sit on a sibling task's branch, or a re-provisioned clone
can lack the task branch as a local ref (commits only on origin). The
fresh-claim path git-resets the clone clean, but resume deliberately
short-circuits before it (_dev_reentry), so the agent's next commit hit
_assert_on_task_branch's BRANCH_MISMATCH, failed, and the task wedged in a
blocked respawn loop (the documenter that could never land its doc commit).

_assert_on_task_branch now recovers instead of only rejecting: fetch + checkout
the task branch (recreating a missing local ref from origin via `git branch
<b> origin/<b>`), and raise only when the switch genuinely can't happen
(uncommitted changes block it). Never discards work — checkout, not reset — so
a resumed agent's unpushed commits are preserved. Updates the RAG troubleshooting
+ developer docs to describe the auto-recovery. Adds 5 unit tests.

* fix(runtime): re-adopt running agent containers on restart (no double-spawn)

An orchestrator restart loses the in-memory _instances registry while the agent
containers keep running. The reaper already had a Docker-liveness fallback
(_assignee_container_running), but the spawn gate (_is_agent_active) did not, so
right after a restart it saw a live agent as inactive and could launch a second
container onto work the forgotten-but-running one was already doing.

start() now calls _readopt_running_agents() after _reconcile_orphan_claims_on_startup
and before the dispatcher/reaper loops launch: it probes each known agent slug's
container (AGENT_IMAGES, reusing _inspect_container_state — the same docker
inspect the reaper uses) and registers a minimal AgentInstance(state=ACTIVE) for
any that is running and not already tracked. Inert when nothing runs (cold start
unchanged); best-effort (a probe error leaves that slot for the reaper's own
fallback). This is the gateway-health spec's Task 4 / the orchestrator-state
spec's Phase 3 (_instances reconcile). Adds 4 unit tests.

* fix(git): treat an already-merged PR as idempotent success on merge

A merge PUT against an already-merged PR returns the same 405 as a genuine
"not mergeable" conflict, so _merge_with_retry raised MergeConflictError and the
completion path tried to rebase / close-superseded / escalate a PR that had
already landed (a prior cycle, a sibling, or the CEO merged it) — the
cell_pm_complete block<->unblock respawn loop.

_merge_with_retry now disambiguates before raising: a new _pr_is_merged probe
(GET the PR, check merged==true) returns success on an already-merged PR so
completion proceeds idempotently; a genuinely-unmerged 405 still raises the
conflict. Best-effort probe (False on any error → falls through to the existing
conflict handling). Adds 4 unit tests.

---------

Co-authored-by: Renn F <rennf93@users.noreply.github.com>
2026-06-25 18:35:52 +02:00

122 lines
4.2 KiB
Python

"""GitService must not delete a branch that still has open dependent PRs.
Root cause of the run-zombifying "integration branch gone from origin" wedge:
`_delete_remote_branch_best_effort` deleted a merged PR's head branch
unconditionally. Merging a cell→root PR therefore deleted the cell branch out
from under in-flight leaf PRs still targeting it (and the CEO root→master merge
deleted the `feature/main_pm/{root}` integration branch). The fix guards the
deletion chokepoint: a branch that is still the BASE of any open PR is an active
integration target and is preserved. Fails safe — if the check can't run, the
branch is kept (cleanup is best-effort; stranding is not).
"""
from __future__ import annotations
from unittest.mock import AsyncMock, MagicMock, patch
import pytest
from roboco.services.git import GitService
def _service() -> GitService:
session = MagicMock()
session.execute = AsyncMock(return_value=None)
session.commit = AsyncMock()
return GitService(session)
def _bind(svc: GitService, name: str, value: object) -> None:
object.__setattr__(svc, name, value)
def _fake_client() -> MagicMock:
client = MagicMock()
client.__aenter__ = AsyncMock(return_value=client)
client.__aexit__ = AsyncMock(return_value=False)
client.delete = AsyncMock()
client.get = AsyncMock()
return client
# --- the deletion chokepoint guard ----------------------------------------
@pytest.mark.asyncio
async def test_delete_skips_branch_with_open_dependents() -> None:
svc = _service()
_bind(svc, "_branch_has_open_dependents", AsyncMock(return_value=True))
client = _fake_client()
with patch("roboco.services.git.httpx.AsyncClient", return_value=client):
await svc._delete_remote_branch_best_effort(
"acme", "repo", "feature/main_pm/abc123", "tok"
)
client.delete.assert_not_awaited()
@pytest.mark.asyncio
async def test_delete_removes_leaf_branch_with_no_dependents() -> None:
svc = _service()
_bind(svc, "_branch_has_open_dependents", AsyncMock(return_value=False))
client = _fake_client()
with patch("roboco.services.git.httpx.AsyncClient", return_value=client):
await svc._delete_remote_branch_best_effort(
"acme", "repo", "feature/backend/abc--cell--leaf", "tok"
)
client.delete.assert_awaited_once()
@pytest.mark.asyncio
async def test_delete_skips_default_branch_before_checking_dependents() -> None:
svc = _service()
dep = AsyncMock(return_value=False)
_bind(svc, "_branch_has_open_dependents", dep)
client = _fake_client()
with patch("roboco.services.git.httpx.AsyncClient", return_value=client):
await svc._delete_remote_branch_best_effort("acme", "repo", "master", "tok")
client.delete.assert_not_awaited()
dep.assert_not_awaited()
# --- the open-dependents probe --------------------------------------------
@pytest.mark.asyncio
async def test_has_open_dependents_true_when_open_pr_targets_base() -> None:
svc = _service()
resp = MagicMock(is_success=True)
resp.json.return_value = [{"number": 5}]
client = _fake_client()
client.get = AsyncMock(return_value=resp)
with patch("roboco.services.git.httpx.AsyncClient", return_value=client):
out = await svc._branch_has_open_dependents(
"acme", "repo", "feature/main_pm/abc123", "tok"
)
assert out is True
@pytest.mark.asyncio
async def test_has_open_dependents_false_when_none() -> None:
svc = _service()
resp = MagicMock(is_success=True)
resp.json.return_value = []
client = _fake_client()
client.get = AsyncMock(return_value=resp)
with patch("roboco.services.git.httpx.AsyncClient", return_value=client):
out = await svc._branch_has_open_dependents(
"acme", "repo", "feature/x--leaf", "tok"
)
assert out is False
@pytest.mark.asyncio
async def test_has_open_dependents_fails_safe_on_non_success() -> None:
svc = _service()
resp = MagicMock(is_success=False)
client = _fake_client()
client.get = AsyncMock(return_value=resp)
with patch("roboco.services.git.httpx.AsyncClient", return_value=client):
out = await svc._branch_has_open_dependents(
"acme", "repo", "feature/main_pm/abc123", "tok"
)
assert out is True