From fbddef403be8c20685f940be37d248e49de9f790 Mon Sep 17 00:00:00 2001 From: Renn F Date: Sun, 28 Jun 2026 04:46:09 +0200 Subject: [PATCH] [feature] i_am_done behind-base submit gate (Phase B2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../services/gateway/choreographer/_impl.py | 52 +++++ roboco/services/git.py | 51 +++++ tests/unit/gateway/test_behind_base_gate.py | 180 ++++++++++++++++++ .../unit/services/test_git_is_behind_base.py | 122 ++++++++++++ 4 files changed, 405 insertions(+) create mode 100644 tests/unit/gateway/test_behind_base_gate.py create mode 100644 tests/unit/services/test_git_is_behind_base.py diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index 1ededd40..7e7fafcf 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -1814,6 +1814,7 @@ class Choreographer: ctx.agent_id, ctx.task_id, ctx.task ), lambda: self._ensure_branch_pushed(ctx), + lambda: self._behind_base_gate(ctx), lambda: self._check_quality_gate(ctx), lambda: self._toolchain_broken_guard(ctx.agent_id, ctx.task), lambda: self._conventions_gate(ctx), @@ -2021,6 +2022,57 @@ class Choreographer: ) 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 def _extract_first_commit_sha(t: Any) -> str | None: """Read the first commit hash off the task, dict or model alike.""" diff --git a/roboco/services/git.py b/roboco/services/git.py index 728ae1bc..95a84259 100644 --- a/roboco/services/git.py +++ b/roboco/services/git.py @@ -3732,6 +3732,57 @@ class GitService(BaseService): 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 = 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( self, pr_number: int, diff --git a/tests/unit/gateway/test_behind_base_gate.py b/tests/unit/gateway/test_behind_base_gate.py new file mode 100644 index 00000000..56f3663b --- /dev/null +++ b/tests/unit/gateway/test_behind_base_gate.py @@ -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 diff --git a/tests/unit/services/test_git_is_behind_base.py b/tests/unit/services/test_git_is_behind_base.py new file mode 100644 index 00000000..630eaf85 --- /dev/null +++ b/tests/unit/services/test_git_is_behind_base.py @@ -0,0 +1,122 @@ +"""Unit tests for ``GitService.is_behind_base`` (multi-level sequencing Phase B2). + +Pins the rev-list ``--left-right --count`` parsing: ``" "`` 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)