mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
Fix the PR-divergence respawn loop: loop gate, CEO god-mode, PR conflict resolver, sequence-ordered merge (#164)
* fix(orchestrator,panel): bound the respawn loop gate and give the CEO a status override
The PM respawn loop gate could never fire on a recurring tracing_gap: every
same-status respawn that emitted a tracing_gap reset the strike counter, so a
task whose unblock can never satisfy its decision gate respawned forever. Cap
the number of tracing_gap resets (pm_respawn_max_tracing_resets) so strikes
accrue once a gap is clearly recurring rather than progressing, and route the
pm-review and blocker dispatch respawn paths through the gate so it actually
applies to those loops.
Panel: the task status dropdown was driven solely by the lifecycle graph, so a
task wedged in a terminal/blocked state offered no actionable transitions. Add
an audited admin status override (PATCH status -> admin_set_status) for every
non-in-band target, letting the human operator force any state.
* feat(git): add rebase_onto_base and close_pull_request PR-divergence primitives
Agents had no way to resolve a PR that could not merge because a sibling merged
overlapping work first: their only moves were complete (which 405s) or block
(which loops). Add the two missing operations:
- rebase_onto_base rebases a head branch onto the latest base and classifies
the outcome: superseded (no unique commits -> safe to close), rebased (unique
work -> force-pushed, ready to merge), or conflicts (aborted, needs a human).
- close_pull_request retires a superseded PR with an explanatory comment.
These back both the sequence-ordered merge and the conflict resolver.
* feat(gateway): auto-resolve a leaf PR that can't merge instead of looping
When a sibling lands overlapping work first, the cell PM's complete() merge
hits a GitHub 405 and the task re-blocks, respawning the PM forever (the
production wedge: one task burned 6000+ tool calls over 3 hours). The merge
now raises MergeConflictError, and cell_pm_complete resolves it:
- rebase the branch onto the current base;
- superseded (no unique commits) -> close the dead PR + complete the task
without a redundant merge (the manual action operators kept requesting);
- rebased (unique work) -> retry the merge, then complete;
- genuine conflicts -> admin-override the task to awaiting_ceo_approval and
alert the CEO, so it leaves agent dispatch instead of looping.
MergeConflictError subclasses GitError, so existing handlers are unaffected.
* test(git): silence unused-arg lint in close_pull_request stub
* feat(orchestrator): sequence-ordered merge for leaf siblings
Leaf siblings share one cell branch, but within-cell siblings were all left at
the default sequence 0, so two leaf PRs raced into the same branch and the
second wedged. Now:
- decomposition assigns each new sibling the next ordinal within its parent, so
the merge order is well-defined;
- the pm-review dispatcher holds a higher-sequence leaf until its earlier
same-team siblings are terminal, so they merge into the shared branch in order
instead of racing.
Loop-free by construction: a gated task is simply not dispatched this tick (no
reject, no respawn). Terminal siblings never block, so a cancelled sibling can't
deadlock the rest; any sibling lookup failure degrades to dispatch.
* test: use monkeypatch.setattr instead of type:ignore in new tests
CI type-checks tests/ (the type-gated suite) which my local 'mypy roboco/' skipped.
The method-mock assignments tripped mypy method-assign/assignment; replace the
silencing comments with monkeypatch.setattr and local mock refs for assertions,
matching the project's no-type:ignore rule.
* fix(git): stop get_status misreporting an unstaged deletion as staged
git_status used stdout.strip().split() before parsing porcelain. strip() eats
the leading space on the first line, so an unstaged deletion (' D file') became
'D file' and parsed as a STAGED deletion — the false 'staged' that caused 6
wasted QA cycles when a dev deleted a file without staging it. Use splitlines(),
which preserves the index/worktree status columns.
* feat(panel): mobile sidebar hamburger + Sheet drawer (AC1)
The umbrella's AC1 was never built: on mobile the sidebar had no entry point.
Extract the nav/footer into shared SidebarNav/SidebarFooter, hide the static
sidebar below md, and add a hamburger in the header that opens the same nav in a
left Sheet drawer (closing on navigation). Desktop is unchanged.
---------
Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
Renzo F
@@ -0,0 +1,187 @@
|
||||
"""GitService PR-divergence primitives: rebase_onto_base + close_pull_request.
|
||||
|
||||
These back both the sequence-ordered merge (rebase a later sibling onto the
|
||||
prior one's merged result) and the conflict resolver (rebase a wedged PR,
|
||||
then close-if-superseded / re-merge / escalate). The classification a rebase
|
||||
yields — superseded vs rebased vs conflicts — drives the whole resolution, so
|
||||
each branch is pinned here against a mocked git.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
from roboco.services.git import GitService
|
||||
|
||||
|
||||
def _git_service() -> GitService:
|
||||
return GitService.__new__(GitService)
|
||||
|
||||
|
||||
def _result(returncode: int = 0, stdout: str = "") -> Any:
|
||||
return type("R", (), {"returncode": returncode, "stdout": stdout})()
|
||||
|
||||
|
||||
_HEAD = "feature/frontend/root--cell--leaf"
|
||||
_BASE = "feature/frontend/root--cell"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rebase_superseded_when_no_unique_commits() -> None:
|
||||
"""Clean rebase + zero commits ahead of base => superseded (safe to close)."""
|
||||
svc = _git_service()
|
||||
pushed: list[list[str]] = []
|
||||
|
||||
async def fake_run(_ws: Any, args: list[str], **_kw: Any) -> Any:
|
||||
if args[0] == "push":
|
||||
pushed.append(args)
|
||||
if args[:2] == ["rev-list", "--count"]:
|
||||
return _result(stdout="0\n")
|
||||
return _result()
|
||||
|
||||
with patch.object(svc, "_run_git", new=fake_run):
|
||||
out = await svc.rebase_onto_base(
|
||||
Path("/tmp/ws"), head_branch=_HEAD, base_branch=_BASE, git_token="tok"
|
||||
)
|
||||
assert out == {"status": "superseded"}
|
||||
# A superseded branch must NOT be force-pushed — nothing changed.
|
||||
assert pushed == []
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rebase_rebased_force_pushes_when_unique_commits() -> None:
|
||||
"""Clean rebase + commits ahead of base => rebased + force-push the head."""
|
||||
svc = _git_service()
|
||||
pushed: list[list[str]] = []
|
||||
|
||||
async def fake_run(_ws: Any, args: list[str], **_kw: Any) -> Any:
|
||||
if args[0] == "push":
|
||||
pushed.append(args)
|
||||
return _result()
|
||||
if args[:2] == ["rev-list", "--count"]:
|
||||
return _result(stdout="3\n")
|
||||
return _result()
|
||||
|
||||
with patch.object(svc, "_run_git", new=fake_run):
|
||||
out = await svc.rebase_onto_base(
|
||||
Path("/tmp/ws"), head_branch=_HEAD, base_branch=_BASE, git_token="tok"
|
||||
)
|
||||
assert out == {"status": "rebased", "unique_commits": 3}
|
||||
# Only the head branch is force-pushed, with lease, never the base.
|
||||
assert pushed == [["push", "--force-with-lease", "origin", f"HEAD:{_HEAD}"]]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rebase_conflicts_aborts_and_reports_files() -> None:
|
||||
"""A failed rebase is aborted and the conflicting files reported."""
|
||||
svc = _git_service()
|
||||
aborted = False
|
||||
|
||||
async def fake_run(_ws: Any, args: list[str], **_kw: Any) -> Any:
|
||||
nonlocal aborted
|
||||
if args == ["rebase", f"origin/{_BASE}"]:
|
||||
return _result(returncode=1)
|
||||
if args[:2] == ["diff", "--name-only"]:
|
||||
return _result(stdout="src/a.tsx\nsrc/b.tsx\n")
|
||||
if args == ["rebase", "--abort"]:
|
||||
aborted = True
|
||||
return _result()
|
||||
return _result()
|
||||
|
||||
with patch.object(svc, "_run_git", new=fake_run):
|
||||
out = await svc.rebase_onto_base(
|
||||
Path("/tmp/ws"), head_branch=_HEAD, base_branch=_BASE, git_token="tok"
|
||||
)
|
||||
assert out == {"status": "conflicts", "files": ["src/a.tsx", "src/b.tsx"]}
|
||||
assert aborted is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rebase_never_force_pushes_on_conflict() -> None:
|
||||
"""Guard: the destructive force-push must not fire when a rebase conflicts."""
|
||||
svc = _git_service()
|
||||
pushed: list[list[str]] = []
|
||||
|
||||
async def fake_run(_ws: Any, args: list[str], **_kw: Any) -> Any:
|
||||
if args[0] == "push":
|
||||
pushed.append(args)
|
||||
if args == ["rebase", f"origin/{_BASE}"]:
|
||||
return _result(returncode=1)
|
||||
if args[:2] == ["diff", "--name-only"]:
|
||||
return _result(stdout="")
|
||||
return _result()
|
||||
|
||||
with patch.object(svc, "_run_git", new=fake_run):
|
||||
await svc.rebase_onto_base(
|
||||
Path("/tmp/ws"), head_branch=_HEAD, base_branch=_BASE, git_token="tok"
|
||||
)
|
||||
assert pushed == []
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_close_pull_request_patches_state_closed(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""close_pull_request issues a PATCH state=closed (and an optional comment)."""
|
||||
svc = _git_service()
|
||||
# Stub the task/project/workspace/token/remote resolution chain via
|
||||
# monkeypatch.setattr (not direct assignment) so mypy's method-assign check
|
||||
# stays satisfied without silencing it.
|
||||
task = type("T", (), {"id": "t", "assigned_to": None, "created_by": None})()
|
||||
session = AsyncMock()
|
||||
session.execute = AsyncMock(
|
||||
return_value=type("Res", (), {"scalar_one_or_none": lambda _self: task})()
|
||||
)
|
||||
delete_branch = AsyncMock()
|
||||
monkeypatch.setattr(svc, "session", session, raising=False)
|
||||
monkeypatch.setattr(
|
||||
svc,
|
||||
"_project_for_task",
|
||||
AsyncMock(return_value=type("P", (), {"slug": "proj"})()),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
svc, "_resolve_workspace_agent_id", MagicMock(return_value=None)
|
||||
)
|
||||
monkeypatch.setattr(svc, "get_workspace", AsyncMock(return_value=Path("/tmp/ws")))
|
||||
monkeypatch.setattr(
|
||||
svc, "_get_project_token_or_raise", AsyncMock(return_value="tok")
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
svc, "_parse_github_remote", MagicMock(return_value=("owner", "repo"))
|
||||
)
|
||||
monkeypatch.setattr(svc, "_delete_pr_branch_best_effort", delete_branch)
|
||||
|
||||
calls: list[tuple[str, str]] = []
|
||||
|
||||
class _Resp:
|
||||
is_success = True
|
||||
status_code = 200
|
||||
text = ""
|
||||
|
||||
class _Client:
|
||||
async def __aenter__(self) -> _Client:
|
||||
return self
|
||||
|
||||
async def __aexit__(self, *_a: Any) -> None:
|
||||
return None
|
||||
|
||||
async def post(self, url: str, **_kw: Any) -> _Resp:
|
||||
calls.append(("POST", url))
|
||||
return _Resp()
|
||||
|
||||
async def patch(self, url: str, **_kw: Any) -> _Resp:
|
||||
calls.append(("PATCH", url))
|
||||
return _Resp()
|
||||
|
||||
with patch("roboco.services.git.httpx.AsyncClient", return_value=_Client()):
|
||||
await svc.close_pull_request(159, comment="superseded by #158")
|
||||
|
||||
assert (
|
||||
"POST",
|
||||
"https://api.github.com/repos/owner/repo/issues/159/comments",
|
||||
) in calls
|
||||
assert ("PATCH", "https://api.github.com/repos/owner/repo/pulls/159") in calls
|
||||
delete_branch.assert_awaited_once()
|
||||
@@ -0,0 +1,91 @@
|
||||
"""get_status must not misreport an unstaged deletion as staged.
|
||||
|
||||
Porcelain encodes the index (staged) state in column 0 and the worktree state
|
||||
in column 1. A worktree-only change has a SPACE in column 0 (e.g. " D file" =
|
||||
unstaged deletion). The old code ran stdout.strip() before splitting, which ate
|
||||
the leading space on the first line, turning " D file" into "D file" — parsed as
|
||||
a STAGED deletion. That false "staged" caused 6 wasted QA cycles when a dev
|
||||
deleted a file but had not staged it. Regression test: the deletion must land in
|
||||
unstaged, never staged.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
import pytest
|
||||
from roboco.services.git import GitService
|
||||
|
||||
|
||||
def _git_service() -> GitService:
|
||||
return GitService.__new__(GitService)
|
||||
|
||||
|
||||
def _result(returncode: int = 0, stdout: str = "") -> Any:
|
||||
return type("R", (), {"returncode": returncode, "stdout": stdout})()
|
||||
|
||||
|
||||
def _fake_run_with_status(porcelain: str) -> Any:
|
||||
async def fake_run(_ws: Any, args: list[str], **_kw: Any) -> Any:
|
||||
if args[:2] == ["branch", "--show-current"]:
|
||||
return _result(stdout="feature/x\n")
|
||||
if args[:2] == ["status", "--porcelain"]:
|
||||
return _result(stdout=porcelain)
|
||||
return _result(returncode=1) # ahead/behind rev-list -> treated as 0,0
|
||||
|
||||
return fake_run
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_unstaged_deletion_first_line_not_reported_as_staged(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
svc = _git_service()
|
||||
monkeypatch.setattr(
|
||||
svc, "_run_git", _fake_run_with_status(" D piragi_patches.py\n")
|
||||
)
|
||||
monkeypatch.setattr(svc, "_ahead_behind", AsyncMock(return_value=(0, 0)))
|
||||
|
||||
_branch, has_changes, staged, unstaged, _untracked, _a, _b = await svc.get_status(
|
||||
Path("/tmp/ws")
|
||||
)
|
||||
|
||||
assert "piragi_patches.py" in unstaged
|
||||
assert "piragi_patches.py" not in staged
|
||||
assert has_changes is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_staged_deletion_still_reported_as_staged(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""A genuine staged deletion ('D file') must still read as staged."""
|
||||
svc = _git_service()
|
||||
monkeypatch.setattr(svc, "_run_git", _fake_run_with_status("D gone.py\n"))
|
||||
monkeypatch.setattr(svc, "_ahead_behind", AsyncMock(return_value=(0, 0)))
|
||||
|
||||
_branch, _has, staged, unstaged, _untracked, _a, _b = await svc.get_status(
|
||||
Path("/tmp/ws")
|
||||
)
|
||||
|
||||
assert "gone.py" in staged
|
||||
assert "gone.py" not in unstaged
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_first_line_unstaged_modify_not_misread(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""The strip() bug hit any worktree-only first line, not just deletions."""
|
||||
svc = _git_service()
|
||||
monkeypatch.setattr(svc, "_run_git", _fake_run_with_status(" M app.py\n"))
|
||||
monkeypatch.setattr(svc, "_ahead_behind", AsyncMock(return_value=(0, 0)))
|
||||
|
||||
_branch, _has, staged, unstaged, _untracked, _a, _b = await svc.get_status(
|
||||
Path("/tmp/ws")
|
||||
)
|
||||
|
||||
assert "app.py" in unstaged
|
||||
assert "app.py" not in staged
|
||||
Reference in New Issue
Block a user