mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[feature] i_am_done behind-base submit gate (Phase B2)
A sibling's PR merging into the parent branch while a dev worked leaves the
dev's branch behind its base — the assembled PR then can't merge cleanly and
the sibling's changes go missing (the 2026-06-27 out-of-order dev-task break).
The behind-base gate refuses i_am_done in that state and steers the dev to
sync_branch (the Phase B1 gate-level rebase verb).
- GitService.is_behind_base: rev-list --left-right --count across
origin/{base}...origin/{head} → (behind, ahead); fetch-first so origin
reflects the pushed head. Raises on git failure (consistent with
rebase_onto_base); malformed stdout degrades to (0,0).
- Choreographer._behind_base_gate: wired into _i_am_done_gate after
_ensure_branch_pushed. behind>0 → invalid_state remediate→sync_branch.
Fail-open on git/base-resolution error (flaky fetch can't strand a task at
the submit gate — the merge layer has its own behind checks). Skipped for
branchless roots and protected bases (master/main/-prefixed).
Tests: gate (6: refuse+steer/up-to-date/branchless/protected/fail-open-base/
fail-open-git), is_behind_base (6: parse/up-to-date/malformed/argv-form/
requires-branch/missing-project). ruff + mypy roboco/ tests/ clean; unit green.
This commit is contained in:
@@ -1814,6 +1814,7 @@ class Choreographer:
|
|||||||
ctx.agent_id, ctx.task_id, ctx.task
|
ctx.agent_id, ctx.task_id, ctx.task
|
||||||
),
|
),
|
||||||
lambda: self._ensure_branch_pushed(ctx),
|
lambda: self._ensure_branch_pushed(ctx),
|
||||||
|
lambda: self._behind_base_gate(ctx),
|
||||||
lambda: self._check_quality_gate(ctx),
|
lambda: self._check_quality_gate(ctx),
|
||||||
lambda: self._toolchain_broken_guard(ctx.agent_id, ctx.task),
|
lambda: self._toolchain_broken_guard(ctx.agent_id, ctx.task),
|
||||||
lambda: self._conventions_gate(ctx),
|
lambda: self._conventions_gate(ctx),
|
||||||
@@ -2021,6 +2022,57 @@ class Choreographer:
|
|||||||
)
|
)
|
||||||
return None
|
return None
|
||||||
|
|
||||||
|
async def _behind_base_gate(self, ctx: _IAmDoneContext) -> Envelope | None:
|
||||||
|
"""Refuse i_am_done when the task branch has fallen behind its base.
|
||||||
|
|
||||||
|
A sibling's PR merged into the parent branch while this dev worked →
|
||||||
|
the head lacks that merged work, so the assembled PR can't merge
|
||||||
|
cleanly and a sibling's changes go missing from this branch (the
|
||||||
|
2026-06-27 out-of-order dev-task break). The dev must ``sync_branch``
|
||||||
|
(the gate-level rebase verb) before submitting. Fail-open on git /
|
||||||
|
base-resolution error so a flaky fetch can't strand a task at the
|
||||||
|
submit gate — the merge layer has its own behind checks. Skipped for
|
||||||
|
branchless coordination roots (no ``branch_name``) and when the base
|
||||||
|
resolves to a protected branch (master/main) or a ``-``-prefixed ref.
|
||||||
|
Runs after ``_ensure_branch_pushed`` so origin reflects the pushed head.
|
||||||
|
"""
|
||||||
|
t = ctx.task
|
||||||
|
if not getattr(t, "branch_name", None):
|
||||||
|
return None
|
||||||
|
try:
|
||||||
|
base_branch = await resolve_parent_branch(t, self.task)
|
||||||
|
except Exception as exc:
|
||||||
|
logger.warning("behind_base_skip", task_id=str(ctx.task_id), error=str(exc))
|
||||||
|
return None
|
||||||
|
if (
|
||||||
|
not base_branch
|
||||||
|
or base_branch.startswith("-")
|
||||||
|
or base_branch in ("master", "main")
|
||||||
|
):
|
||||||
|
return None
|
||||||
|
try:
|
||||||
|
behind, _ahead = await self.git.is_behind_base(
|
||||||
|
t, base_branch=base_branch, actor_agent_id=ctx.agent_id
|
||||||
|
)
|
||||||
|
except Exception as exc:
|
||||||
|
logger.warning("behind_base_skip", task_id=str(ctx.task_id), error=str(exc))
|
||||||
|
return None
|
||||||
|
if behind > 0:
|
||||||
|
return Envelope.invalid_state(
|
||||||
|
message=(
|
||||||
|
f"your branch is {behind} commit(s) behind its base "
|
||||||
|
f"'{base_branch}' — a sibling's PR merged into the parent "
|
||||||
|
f"branch while you worked; your branch is missing that work"
|
||||||
|
),
|
||||||
|
remediate=(
|
||||||
|
"call sync_branch(task_id) to rebase your branch onto its base "
|
||||||
|
"through the gate, then i_am_done again. do NOT submit a branch "
|
||||||
|
"that is behind its base — the PR cannot merge cleanly"
|
||||||
|
),
|
||||||
|
context_briefing=ctx.briefing,
|
||||||
|
)
|
||||||
|
return None
|
||||||
|
|
||||||
@staticmethod
|
@staticmethod
|
||||||
def _extract_first_commit_sha(t: Any) -> str | None:
|
def _extract_first_commit_sha(t: Any) -> str | None:
|
||||||
"""Read the first commit hash off the task, dict or model alike."""
|
"""Read the first commit hash off the task, dict or model alike."""
|
||||||
|
|||||||
@@ -3732,6 +3732,57 @@ class GitService(BaseService):
|
|||||||
git_token=git_token,
|
git_token=git_token,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
async def is_behind_base(
|
||||||
|
self,
|
||||||
|
task: Any,
|
||||||
|
*,
|
||||||
|
base_branch: str,
|
||||||
|
actor_agent_id: UUID | None = None,
|
||||||
|
) -> tuple[int, int]:
|
||||||
|
"""Return ``(behind, ahead)`` commit counts: head vs base branch.
|
||||||
|
|
||||||
|
``behind`` = commits on ``origin/{base_branch}`` NOT on
|
||||||
|
``origin/{head_branch}`` — the work the head has fallen behind by (e.g.
|
||||||
|
a sibling's PR merged into the parent branch while this dev worked).
|
||||||
|
``ahead`` = commits on the head NOT on the base — the head's own work.
|
||||||
|
Both are read from origin after a fetch, so they reflect the pushed
|
||||||
|
state a merge / PR base would actually see.
|
||||||
|
|
||||||
|
The submit-time behind-base gate (``i_am_done``) uses this: a non-zero
|
||||||
|
``behind`` means the branch fell behind its base and the dev must
|
||||||
|
``sync_branch`` before submitting, else the PR can't merge cleanly /
|
||||||
|
a sibling's merged work is missing from this branch. Mirrors
|
||||||
|
:meth:`sync_task_branch`'s workspace/token resolution. Raises on git
|
||||||
|
failure (consistent with :meth:`rebase_onto_base`); the gate handler
|
||||||
|
fail-opens on a raised error so a flaky fetch can't strand a task at
|
||||||
|
the submit gate — the merge layer has its own behind checks.
|
||||||
|
"""
|
||||||
|
if not task.branch_name:
|
||||||
|
raise ValueError("is_behind_base requires a task with a branch_name")
|
||||||
|
project = await self._project_for_task(task)
|
||||||
|
if project is None:
|
||||||
|
raise NotFoundError("Project for task", str(task.id))
|
||||||
|
workspace_agent_id = self._resolve_workspace_agent_id(task, actor_agent_id)
|
||||||
|
workspace = await self.get_workspace(project.slug, agent_id=workspace_agent_id)
|
||||||
|
git_token = await self._get_project_token_or_raise(project.slug)
|
||||||
|
await self._run_git(workspace, ["fetch", "origin"], token=git_token)
|
||||||
|
# --left-right --count A...B → "<left> <right>": left = commits only in
|
||||||
|
# A (base, what the head is BEHIND by); right = commits only in B (head,
|
||||||
|
# the head's own ahead work). Triple-dot = symmetric difference.
|
||||||
|
count = await self._run_git(
|
||||||
|
workspace,
|
||||||
|
[
|
||||||
|
"rev-list",
|
||||||
|
"--left-right",
|
||||||
|
"--count",
|
||||||
|
f"origin/{base_branch}...origin/{task.branch_name}",
|
||||||
|
],
|
||||||
|
)
|
||||||
|
left, _, right = count.stdout.strip().partition(" ")
|
||||||
|
behind = int(left) if left.strip().isdigit() else 0
|
||||||
|
ahead = int(right) if right.strip().isdigit() else 0
|
||||||
|
return behind, ahead
|
||||||
|
|
||||||
async def close_pull_request(
|
async def close_pull_request(
|
||||||
self,
|
self,
|
||||||
pr_number: int,
|
pr_number: int,
|
||||||
|
|||||||
@@ -0,0 +1,180 @@
|
|||||||
|
"""i_am_done behind-base submit gate (multi-level sequencing Phase B2).
|
||||||
|
|
||||||
|
A sibling's PR merging into the parent branch while a dev worked leaves the
|
||||||
|
dev's branch behind its base — the assembled PR then can't merge cleanly and
|
||||||
|
the sibling's changes go missing (the 2026-06-27 out-of-order dev-task break).
|
||||||
|
The behind-base gate refuses ``i_am_done`` in that state and steers the dev to
|
||||||
|
``sync_branch`` (the gate-level rebase). Fail-open on git / base-resolution
|
||||||
|
error so a flaky fetch can't strand a task at the submit gate; the merge layer
|
||||||
|
has its own behind checks. Skipped for branchless roots and protected bases.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from unittest.mock import AsyncMock, patch
|
||||||
|
from uuid import UUID, uuid4
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps
|
||||||
|
from roboco.services.gateway.choreographer._impl import _IAmDoneContext
|
||||||
|
|
||||||
|
|
||||||
|
def _make_deps(**overrides: object) -> ChoreographerDeps:
|
||||||
|
base: dict[str, object] = {
|
||||||
|
"task": AsyncMock(),
|
||||||
|
"work_session": AsyncMock(),
|
||||||
|
"git": AsyncMock(),
|
||||||
|
"a2a": AsyncMock(),
|
||||||
|
"journal": AsyncMock(),
|
||||||
|
"audit": AsyncMock(),
|
||||||
|
"evidence_repo": AsyncMock(),
|
||||||
|
}
|
||||||
|
base.update(overrides)
|
||||||
|
repo = base["evidence_repo"]
|
||||||
|
assert isinstance(repo, AsyncMock)
|
||||||
|
for method in (
|
||||||
|
"list_unread_a2a",
|
||||||
|
"list_unread_mentions",
|
||||||
|
"list_pending_notifications",
|
||||||
|
"task_metadata_gaps",
|
||||||
|
"recent_team_activity",
|
||||||
|
"blockers_in_lane",
|
||||||
|
):
|
||||||
|
getattr(repo, method).return_value = []
|
||||||
|
return ChoreographerDeps(**base)
|
||||||
|
|
||||||
|
|
||||||
|
def _ctx(
|
||||||
|
task: object, *, agent_id: UUID | None = None, task_id: UUID | None = None
|
||||||
|
) -> _IAmDoneContext:
|
||||||
|
return _IAmDoneContext(
|
||||||
|
agent_id=agent_id or uuid4(),
|
||||||
|
task_id=task_id or uuid4(),
|
||||||
|
task=task,
|
||||||
|
role_str="developer",
|
||||||
|
briefing={},
|
||||||
|
notes="",
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
_BRANCH = "feature/backend/abc12345"
|
||||||
|
_BASE = "feature/backend/parent12345"
|
||||||
|
|
||||||
|
|
||||||
|
class _Task:
|
||||||
|
def __init__(self, branch_name: str | None = _BRANCH) -> None:
|
||||||
|
self.branch_name = branch_name
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_behind_base_gate_refuses_and_steers_to_sync_branch() -> None:
|
||||||
|
t = _Task()
|
||||||
|
task_svc = AsyncMock()
|
||||||
|
git_svc = AsyncMock()
|
||||||
|
git_svc.is_behind_base.return_value = (3, 2) # 3 behind, 2 ahead
|
||||||
|
deps = _make_deps(task=task_svc, git=git_svc)
|
||||||
|
c = Choreographer(deps)
|
||||||
|
ctx = _ctx(t)
|
||||||
|
|
||||||
|
with patch(
|
||||||
|
"roboco.services.gateway.choreographer._impl.resolve_parent_branch",
|
||||||
|
new=AsyncMock(return_value=_BASE),
|
||||||
|
):
|
||||||
|
env = await c._behind_base_gate(ctx)
|
||||||
|
|
||||||
|
git_svc.is_behind_base.assert_awaited_once()
|
||||||
|
assert env is not None
|
||||||
|
assert env.error == "invalid_state"
|
||||||
|
assert "sync_branch" in (env.remediate or "")
|
||||||
|
assert "behind" in (env.message or "")
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_behind_base_gate_passes_when_up_to_date() -> None:
|
||||||
|
t = _Task()
|
||||||
|
task_svc = AsyncMock()
|
||||||
|
git_svc = AsyncMock()
|
||||||
|
git_svc.is_behind_base.return_value = (0, 5) # not behind
|
||||||
|
deps = _make_deps(task=task_svc, git=git_svc)
|
||||||
|
c = Choreographer(deps)
|
||||||
|
ctx = _ctx(t)
|
||||||
|
|
||||||
|
with patch(
|
||||||
|
"roboco.services.gateway.choreographer._impl.resolve_parent_branch",
|
||||||
|
new=AsyncMock(return_value=_BASE),
|
||||||
|
):
|
||||||
|
env = await c._behind_base_gate(ctx)
|
||||||
|
|
||||||
|
assert env is None
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_behind_base_gate_skips_branchless_task() -> None:
|
||||||
|
# No branch_name → branchless coordination root; nothing to sync.
|
||||||
|
t = _Task(branch_name=None)
|
||||||
|
git_svc = AsyncMock()
|
||||||
|
deps = _make_deps(git=git_svc)
|
||||||
|
c = Choreographer(deps)
|
||||||
|
ctx = _ctx(t)
|
||||||
|
|
||||||
|
env = await c._behind_base_gate(ctx)
|
||||||
|
|
||||||
|
assert env is None
|
||||||
|
git_svc.is_behind_base.assert_not_awaited()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_behind_base_gate_skips_protected_base() -> None:
|
||||||
|
# A base that resolved to master must never be rebased into — skip (the
|
||||||
|
# merge layer guards master); never block the submit on it.
|
||||||
|
t = _Task()
|
||||||
|
git_svc = AsyncMock()
|
||||||
|
deps = _make_deps(git=git_svc)
|
||||||
|
c = Choreographer(deps)
|
||||||
|
ctx = _ctx(t)
|
||||||
|
|
||||||
|
with patch(
|
||||||
|
"roboco.services.gateway.choreographer._impl.resolve_parent_branch",
|
||||||
|
new=AsyncMock(return_value="master"),
|
||||||
|
):
|
||||||
|
env = await c._behind_base_gate(ctx)
|
||||||
|
|
||||||
|
assert env is None
|
||||||
|
git_svc.is_behind_base.assert_not_awaited()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_behind_base_gate_fail_opens_on_base_resolution_error() -> None:
|
||||||
|
t = _Task()
|
||||||
|
git_svc = AsyncMock()
|
||||||
|
deps = _make_deps(git=git_svc)
|
||||||
|
c = Choreographer(deps)
|
||||||
|
ctx = _ctx(t)
|
||||||
|
|
||||||
|
with patch(
|
||||||
|
"roboco.services.gateway.choreographer._impl.resolve_parent_branch",
|
||||||
|
new=AsyncMock(side_effect=RuntimeError("db unavailable")),
|
||||||
|
):
|
||||||
|
env = await c._behind_base_gate(ctx)
|
||||||
|
|
||||||
|
assert env is None
|
||||||
|
git_svc.is_behind_base.assert_not_awaited()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_behind_base_gate_fail_opens_on_git_error() -> None:
|
||||||
|
t = _Task()
|
||||||
|
git_svc = AsyncMock()
|
||||||
|
git_svc.is_behind_base.side_effect = RuntimeError("fetch timeout")
|
||||||
|
deps = _make_deps(git=git_svc)
|
||||||
|
c = Choreographer(deps)
|
||||||
|
ctx = _ctx(t)
|
||||||
|
|
||||||
|
with patch(
|
||||||
|
"roboco.services.gateway.choreographer._impl.resolve_parent_branch",
|
||||||
|
new=AsyncMock(return_value=_BASE),
|
||||||
|
):
|
||||||
|
env = await c._behind_base_gate(ctx)
|
||||||
|
|
||||||
|
# A flaky fetch must not strand the task at the submit gate — fail-open.
|
||||||
|
assert env is None
|
||||||
@@ -0,0 +1,122 @@
|
|||||||
|
"""Unit tests for ``GitService.is_behind_base`` (multi-level sequencing Phase B2).
|
||||||
|
|
||||||
|
Pins the rev-list ``--left-right --count`` parsing: ``"<left> <right>"`` where
|
||||||
|
left = commits only in the base (what the head is BEHIND by) and right = the
|
||||||
|
head's own ahead work. The behind-base submit gate keys off the ``behind``
|
||||||
|
count. Guards: requires ``branch_name``; raises when the project lookup misses.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from pathlib import Path
|
||||||
|
from typing import Any
|
||||||
|
from unittest.mock import AsyncMock, MagicMock
|
||||||
|
from uuid import uuid4
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
from roboco.services.base import NotFoundError
|
||||||
|
from roboco.services.git import GitService
|
||||||
|
|
||||||
|
_WORKSPACE = Path("/tmp/fake-ws")
|
||||||
|
_TOKEN = "ghp_fake"
|
||||||
|
_BASE = "feature/backend/parent12345"
|
||||||
|
_HEAD = "feature/backend/abc12345"
|
||||||
|
|
||||||
|
|
||||||
|
def _git_service() -> GitService:
|
||||||
|
svc = GitService.__new__(GitService)
|
||||||
|
svc.log = MagicMock()
|
||||||
|
return svc
|
||||||
|
|
||||||
|
|
||||||
|
def _result(stdout: str = "", returncode: int = 0) -> Any:
|
||||||
|
r = MagicMock()
|
||||||
|
r.stdout = stdout
|
||||||
|
r.returncode = returncode
|
||||||
|
r.stderr = ""
|
||||||
|
return r
|
||||||
|
|
||||||
|
|
||||||
|
def _task(branch: str | None = _HEAD) -> Any:
|
||||||
|
return MagicMock(id=uuid4(), branch_name=branch)
|
||||||
|
|
||||||
|
|
||||||
|
def _project() -> Any:
|
||||||
|
return MagicMock(slug="roboco")
|
||||||
|
|
||||||
|
|
||||||
|
async def _wire(svc: GitService, *, rev_list_stdout: str) -> AsyncMock:
|
||||||
|
"""Stub the workspace/token resolution + _run_git; return the run mock."""
|
||||||
|
svc._project_for_task = AsyncMock(return_value=_project()) # type: ignore[method-assign]
|
||||||
|
svc._resolve_workspace_agent_id = MagicMock(return_value=uuid4()) # type: ignore[method-assign]
|
||||||
|
svc.get_workspace = AsyncMock(return_value=_WORKSPACE) # type: ignore[method-assign]
|
||||||
|
svc._get_project_token_or_raise = AsyncMock(return_value=_TOKEN) # type: ignore[method-assign]
|
||||||
|
run = AsyncMock(side_effect=[_result(), _result(stdout=rev_list_stdout)])
|
||||||
|
svc._run_git = run # type: ignore[method-assign]
|
||||||
|
return run
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_is_behind_base_parses_left_right_counts() -> None:
|
||||||
|
"""'3 2' → behind=3, ahead=2 (3 commits on base not on head)."""
|
||||||
|
svc = _git_service()
|
||||||
|
await _wire(svc, rev_list_stdout="3 2")
|
||||||
|
|
||||||
|
behind, ahead = await svc.is_behind_base(_task(), base_branch=_BASE)
|
||||||
|
|
||||||
|
assert (behind, ahead) == (3, 2)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_is_behind_base_up_to_date_returns_zeros() -> None:
|
||||||
|
"""'0 5' → not behind (0), 5 commits ahead."""
|
||||||
|
svc = _git_service()
|
||||||
|
await _wire(svc, rev_list_stdout="0 5")
|
||||||
|
|
||||||
|
behind, ahead = await svc.is_behind_base(_task(), base_branch=_BASE)
|
||||||
|
|
||||||
|
assert (behind, ahead) == (0, 5)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_is_behind_base_malformed_stdout_fails_open_to_zero() -> None:
|
||||||
|
"""A non-numeric stdout (git error text) must not crash — degrade to (0, 0)."""
|
||||||
|
svc = _git_service()
|
||||||
|
await _wire(svc, rev_list_stdout="fatal: bad rev")
|
||||||
|
|
||||||
|
behind, ahead = await svc.is_behind_base(_task(), base_branch=_BASE)
|
||||||
|
|
||||||
|
assert (behind, ahead) == (0, 0)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_is_behind_base_uses_left_right_triple_dot_form() -> None:
|
||||||
|
"""The rev-list call must use --left-right --count with triple-dot (symmetric
|
||||||
|
difference) across origin/{base}...origin/{head}."""
|
||||||
|
svc = _git_service()
|
||||||
|
run = await _wire(svc, rev_list_stdout="1 4")
|
||||||
|
|
||||||
|
await svc.is_behind_base(_task(), base_branch=_BASE)
|
||||||
|
|
||||||
|
# second _run_git call is the rev-list; assert its argv shape.
|
||||||
|
rev_list_call = run.call_args_list[1]
|
||||||
|
argv = rev_list_call.args[1]
|
||||||
|
assert argv[:3] == ["rev-list", "--left-right", "--count"]
|
||||||
|
assert argv[3] == f"origin/{_BASE}...origin/{_HEAD}"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_is_behind_base_requires_branch_name() -> None:
|
||||||
|
"""A branchless task has nothing to compare — ValueError, not a silent (0,0)."""
|
||||||
|
svc = _git_service()
|
||||||
|
with pytest.raises(ValueError, match="branch_name"):
|
||||||
|
await svc.is_behind_base(_task(branch=None), base_branch=_BASE)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_is_behind_base_raises_when_project_missing() -> None:
|
||||||
|
svc = _git_service()
|
||||||
|
svc._project_for_task = AsyncMock(return_value=None) # type: ignore[method-assign]
|
||||||
|
|
||||||
|
with pytest.raises(NotFoundError):
|
||||||
|
await svc.is_behind_base(_task(), base_branch=_BASE)
|
||||||
Reference in New Issue
Block a user