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>

tests/unit/runtime/test_readopt_running_agents.py

@@ -0,0 +1,79 @@
+"""Startup re-adoption of still-running agent containers into ``_instances``.
+
+An orchestrator restart loses the in-memory ``_instances`` registry while the
+agent containers keep running. The reaper has a Docker-liveness fallback for
+that (``_assignee_container_running``), but the spawn gate's ``_is_agent_active``
+does not — so after a restart it sees a live agent as inactive and can
+double-spawn it onto work its forgotten-but-running container is already doing.
+``_readopt_running_agents`` probes each known agent slug's container and
+re-registers a minimal ACTIVE instance for any that is running, so both the
+reaper's live-skip and the spawn gate see the live agent immediately.
+"""
+
+from __future__ import annotations
+
+from unittest.mock import AsyncMock, MagicMock
+
+import pytest
+from roboco.runtime.orchestrator import AgentOrchestrator, AgentState
+
+_EXPECTED_READOPTED = 2
+
+
+def _orch() -> AgentOrchestrator:
+    orch = AgentOrchestrator.__new__(AgentOrchestrator)  # bypass __init__
+    orch._instances = {}
+    return orch
+
+
+@pytest.mark.asyncio
+async def test_readopts_running_containers_as_active() -> None:
+    orch = _orch()
+    running = {"be-dev-1", "fe-pm"}
+
+    async def inspect(name: str) -> tuple[bool, int | None]:
+        slug = name.removeprefix("roboco-agent-")
+        return (slug in running, 0)
+
+    orch._inspect_container_state = AsyncMock(side_effect=inspect)  # type: ignore[method-assign]
+
+    n = await orch._readopt_running_agents()
+
+    assert n == _EXPECTED_READOPTED
+    assert orch._instances["be-dev-1"].state == AgentState.ACTIVE
+    assert orch._instances["be-dev-1"].agent_id == "be-dev-1"
+    assert orch._instances["fe-pm"].state == AgentState.ACTIVE
+    assert "be-dev-2" not in orch._instances  # probed, not running → untracked
+
+
+@pytest.mark.asyncio
+async def test_readopt_leaves_already_tracked_instance_untouched() -> None:
+    orch = _orch()
+    sentinel = MagicMock()
+    orch._instances = {"be-dev-1": sentinel}
+    orch._inspect_container_state = AsyncMock(return_value=(True, 0))  # type: ignore[method-assign]
+
+    await orch._readopt_running_agents()
+
+    assert orch._instances["be-dev-1"] is sentinel  # not re-adopted over
+
+
+@pytest.mark.asyncio
+async def test_readopt_inert_when_nothing_running() -> None:
+    orch = _orch()
+    orch._inspect_container_state = AsyncMock(return_value=(False, None))  # type: ignore[method-assign]
+
+    n = await orch._readopt_running_agents()
+
+    assert n == 0
+    assert orch._instances == {}
+
+
+@pytest.mark.asyncio
+async def test_readopt_swallows_probe_errors() -> None:
+    orch = _orch()
+    orch._inspect_container_state = AsyncMock(side_effect=RuntimeError("no docker"))  # type: ignore[method-assign]
+
+    n = await orch._readopt_running_agents()
+
+    assert n == 0  # best-effort: a probe failure never raises into startup

tests/unit/services/test_git_branch_deletion_guard.py

@@ -0,0 +1,121 @@
+"""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

tests/unit/services/test_git_merge_idempotent.py

@@ -0,0 +1,105 @@
+"""``_merge_with_retry`` treats an already-merged PR as idempotent success.
+
+A merge PUT on an already-merged PR returns the same 405 as a genuine
+"not mergeable" conflict. Treating it as a conflict made `cell_pm_complete`
+try to rebase/escalate a PR that had already landed — the block<->unblock
+respawn loop. The merge path now disambiguates: already-merged → success
+(no-op), otherwise a real `MergeConflictError`.
+"""
+
+from __future__ import annotations
+
+from pathlib import Path
+from typing import Any
+from unittest.mock import AsyncMock, MagicMock, patch
+
+import pytest
+from roboco.exceptions import MergeConflictError
+from roboco.services.git import GitService
+
+
+def _git_service() -> GitService:
+    svc = GitService.__new__(GitService)
+    svc.log = MagicMock()
+    return svc
+
+
+def _resp(status_code: int, *, is_success: bool) -> Any:
+    return type(
+        "R",
+        (),
+        {
+            "status_code": status_code,
+            "is_success": is_success,
+            "text": "",
+            "json": lambda _self=None: {},
+        },
+    )()
+
+
+def _ctx() -> Any:
+    return GitService._MergeContext(
+        owner="acme",
+        repo="repo",
+        pr_number=42,
+        git_token="tok",
+        workspace=Path("/ws"),
+        target="feature/main_pm/abc",
+    )
+
+
+@pytest.mark.asyncio
+async def test_merge_idempotent_when_pr_already_merged(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    svc = _git_service()
+    monkeypatch.setattr(
+        svc, "_call_merge_api", AsyncMock(return_value=_resp(405, is_success=False))
+    )
+    already = AsyncMock(return_value=True)
+    monkeypatch.setattr(svc, "_pr_is_merged", already)
+
+    # Must NOT raise — an already-merged PR is a no-op success.
+    await svc._merge_with_retry(_ctx())
+
+    already.assert_awaited_once()
+
+
+@pytest.mark.asyncio
+async def test_merge_raises_conflict_when_not_already_merged(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    svc = _git_service()
+    monkeypatch.setattr(
+        svc, "_call_merge_api", AsyncMock(return_value=_resp(405, is_success=False))
+    )
+    monkeypatch.setattr(svc, "_pr_is_merged", AsyncMock(return_value=False))
+
+    with pytest.raises(MergeConflictError):
+        await svc._merge_with_retry(_ctx())
+
+
+@pytest.mark.asyncio
+async def test_pr_is_merged_true_when_github_reports_merged() -> None:
+    svc = _git_service()
+    resp = type(
+        "R", (), {"is_success": True, "json": lambda _self=None: {"merged": True}}
+    )()
+    client = MagicMock()
+    client.__aenter__ = AsyncMock(return_value=client)
+    client.__aexit__ = AsyncMock(return_value=False)
+    client.get = AsyncMock(return_value=resp)
+    with patch("roboco.services.git.httpx.AsyncClient", return_value=client):
+        assert await svc._pr_is_merged("acme", "repo", 42, "tok") is True
+
+
+@pytest.mark.asyncio
+async def test_pr_is_merged_false_on_non_success() -> None:
+    svc = _git_service()
+    resp = type("R", (), {"is_success": False, "json": lambda _self=None: {}})()
+    client = MagicMock()
+    client.__aenter__ = AsyncMock(return_value=client)
+    client.__aexit__ = AsyncMock(return_value=False)
+    client.get = AsyncMock(return_value=resp)
+    with patch("roboco.services.git.httpx.AsyncClient", return_value=client):
+        assert await svc._pr_is_merged("acme", "repo", 42, "tok") is False

tests/unit/services/test_git_resume_resync.py

@@ -0,0 +1,114 @@
+"""On resume, the branch-mismatch chokepoint recovers the clone instead of
+hard-failing.
+
+A dev/documenter/QA clone is shared across 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). `_assert_on_task_branch` used to raise
+BRANCH_MISMATCH in that state, so the agent's next commit failed and the task
+wedged in a blocked respawn loop (the documented resume deadlock — e.g. the
+documenter PR #102 case). It now fetches + checks out the task branch (recreating
+a missing local ref from origin) and only raises if it genuinely cannot switch
+(uncommitted changes block it). It NEVER discards local commits — checkout, not
+reset.
+"""
+
+from __future__ import annotations
+
+from pathlib import Path
+from unittest.mock import AsyncMock, MagicMock
+
+import pytest
+from roboco.services.base import ValidationError
+from roboco.services.git import GitService
+
+
+def _service() -> GitService:
+    session = MagicMock()
+    session.execute = AsyncMock(return_value=None)
+    return GitService(session)
+
+
+def _bind(svc: GitService, name: str, value: object) -> None:
+    object.__setattr__(svc, name, value)
+
+
+def _run_git_mock(*, local_ref_rc: int = 0, checkout_rc: int = 0) -> AsyncMock:
+    async def _run(_workspace: Path, args: list[str], **_kw: object) -> MagicMock:
+        if args[:2] == ["rev-parse", "--verify"]:
+            return MagicMock(returncode=local_ref_rc, stdout="")
+        if args[0] == "checkout":
+            return MagicMock(returncode=checkout_rc, stdout="")
+        return MagicMock(returncode=0, stdout="")
+
+    return AsyncMock(side_effect=_run)
+
+
+@pytest.mark.asyncio
+async def test_noop_when_already_on_task_branch() -> None:
+    svc = _service()
+    _bind(svc, "get_current_branch", AsyncMock(return_value="feature/main_pm/abc"))
+    run = _run_git_mock()
+    _bind(svc, "_run_git", run)
+    await svc._assert_on_task_branch(Path("/ws"), "feature/main_pm/abc")
+    run.assert_not_awaited()  # already on it → no git work, no raise
+
+
+@pytest.mark.asyncio
+async def test_noop_when_task_branch_none() -> None:
+    svc = _service()
+    gcb = AsyncMock(return_value="whatever")
+    _bind(svc, "get_current_branch", gcb)
+    await svc._assert_on_task_branch(Path("/ws"), None)
+    gcb.assert_not_awaited()
+
+
+@pytest.mark.asyncio
+async def test_recovers_by_checkout_when_local_ref_present() -> None:
+    svc = _service()
+    _bind(
+        svc,
+        "get_current_branch",
+        AsyncMock(side_effect=["feature/other--leaf", "feature/main_pm/abc"]),
+    )
+    _bind(svc, "_token_for_workspace", AsyncMock(return_value="tok"))
+    run = _run_git_mock(local_ref_rc=0, checkout_rc=0)
+    _bind(svc, "_run_git", run)
+
+    await svc._assert_on_task_branch(Path("/ws"), "feature/main_pm/abc")
+
+    cmds = [c.args[1] for c in run.await_args_list]
+    assert ["checkout", "feature/main_pm/abc"] in cmds
+    # local ref present → no recovery fetch/branch-create
+    assert not any(c[0] == "fetch" for c in cmds)
+
+
+@pytest.mark.asyncio
+async def test_recovers_missing_local_ref_from_origin() -> None:
+    svc = _service()
+    _bind(
+        svc,
+        "get_current_branch",
+        AsyncMock(side_effect=["feature/other--leaf", "feature/main_pm/abc"]),
+    )
+    _bind(svc, "_token_for_workspace", AsyncMock(return_value="tok"))
+    run = _run_git_mock(local_ref_rc=1, checkout_rc=0)  # local ref missing
+    _bind(svc, "_run_git", run)
+
+    await svc._assert_on_task_branch(Path("/ws"), "feature/main_pm/abc")
+
+    cmds = [c.args[1] for c in run.await_args_list]
+    assert ["fetch", "origin", "feature/main_pm/abc"] in cmds
+    assert ["branch", "feature/main_pm/abc", "origin/feature/main_pm/abc"] in cmds
+    assert ["checkout", "feature/main_pm/abc"] in cmds
+
+
+@pytest.mark.asyncio
+async def test_raises_when_cannot_switch() -> None:
+    svc = _service()
+    _bind(svc, "get_current_branch", AsyncMock(return_value="feature/other--leaf"))
+    _bind(svc, "_token_for_workspace", AsyncMock(return_value="tok"))
+    run = _run_git_mock(local_ref_rc=0, checkout_rc=1)  # checkout fails (dirty tree)
+    _bind(svc, "_run_git", run)
+
+    with pytest.raises(ValidationError, match="BRANCH_MISMATCH"):
+        await svc._assert_on_task_branch(Path("/ws"), "feature/main_pm/abc")