mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(task): push a pre-set branch_name when the ref is missing on origin
A branch_name set on a task was treated as proof the ref existed on origin, so _finalize_claim skipped _ensure_branch_for_task and create_branch/push never ran. A manual field write (or a prior failed create_branch whose rollback didn't restore branch_name) left the field set while the branch was never pushed; descendants then ls-remote'd the name, found it empty, and cut from master via create_branch's silent fallback — breaking the cell->root branch hierarchy (MegaTask f7d0a61a root-branch 404). Defect A: - _ensure_branch_for_task trust-but-verifies a pre-set branch_name: probe origin, and when the ref is confirmed missing run the full create to push it. An inconclusive probe (network error) fails soft so a transient glitch can't fail a normal resume claim. Gated on project_id so branchless coordination/umbrella tasks are untouched. - _finalize_claim always runs _ensure_branch_for_task (the single chokepoint that ensures the branch exists) and snapshots+restores branch_name on rollback, so a failed first attempt can't leave the field half-set and short-circuit a retry. - GitService.branch_exists_on_remote: ls-remote probe returning True (present) / False (absent) / None (probe errored, fail soft).
This commit is contained in:
@@ -591,6 +591,41 @@ class GitService(BaseService):
|
|||||||
|
|
||||||
return workspace
|
return workspace
|
||||||
|
|
||||||
|
async def branch_exists_on_remote(
|
||||||
|
self,
|
||||||
|
project_slug: str,
|
||||||
|
branch_name: str,
|
||||||
|
agent_id: UUID | None = None,
|
||||||
|
) -> bool | None:
|
||||||
|
"""Probe whether ``branch_name`` exists on the project's ``origin``.
|
||||||
|
|
||||||
|
Returns True when the ref is present, False when confirmed absent, and
|
||||||
|
None when the probe itself errored (network blip, missing workspace or
|
||||||
|
token) — callers fail-soft on None so a transient glitch can't fail a
|
||||||
|
normal claim. Mirrors the ``ls-remote --heads origin <branch>`` idiom
|
||||||
|
in :meth:`create_branch`'s parent-branch check, reusing the same
|
||||||
|
workspace + decrypted-token resolution so the probe is authoritative.
|
||||||
|
"""
|
||||||
|
try:
|
||||||
|
workspace = await self.get_workspace(project_slug, agent_id)
|
||||||
|
token = await self._token_for_project(project_slug)
|
||||||
|
result = await self._run_git(
|
||||||
|
workspace,
|
||||||
|
["ls-remote", "--heads", "origin", branch_name],
|
||||||
|
check=False,
|
||||||
|
token=token,
|
||||||
|
timeout=_network_git_timeout(),
|
||||||
|
)
|
||||||
|
except Exception as exc:
|
||||||
|
self.log.warning(
|
||||||
|
"branch_exists_on_remote probe failed; failing soft",
|
||||||
|
project_slug=project_slug,
|
||||||
|
branch_name=branch_name,
|
||||||
|
error=str(exc),
|
||||||
|
)
|
||||||
|
return None
|
||||||
|
return bool(result.stdout.strip())
|
||||||
|
|
||||||
# =========================================================================
|
# =========================================================================
|
||||||
# STATUS / INFO METHODS
|
# STATUS / INFO METHODS
|
||||||
# =========================================================================
|
# =========================================================================
|
||||||
|
|||||||
+51
-1
@@ -2132,6 +2132,20 @@ class TaskService(BaseService):
|
|||||||
ValueError: If branch cannot be created
|
ValueError: If branch cannot be created
|
||||||
"""
|
"""
|
||||||
if task.branch_name:
|
if task.branch_name:
|
||||||
|
# A set branch_name is not proof the ref is on origin: a manual
|
||||||
|
# field write, or a prior failed create_branch whose rollback
|
||||||
|
# didn't restore branch_name, can leave the field set while the
|
||||||
|
# branch was never pushed. Descendants then ls-remote this name,
|
||||||
|
# find it empty, and silently cut from master via create_branch's
|
||||||
|
# fallback — breaking the hierarchy. For a task that owns a repo,
|
||||||
|
# trust-but-verify: if the ref is confirmed missing, run the full
|
||||||
|
# create to push it. An inconclusive probe (network error) fails
|
||||||
|
# soft and returns the name as before, so a transient glitch can't
|
||||||
|
# fail a normal resume claim.
|
||||||
|
if task.project_id and await self._named_branch_missing_on_remote(
|
||||||
|
task, agent_id
|
||||||
|
):
|
||||||
|
return await self._auto_create_branch(task, agent_id)
|
||||||
return str(task.branch_name)
|
return str(task.branch_name)
|
||||||
|
|
||||||
if not task.project_id:
|
if not task.project_id:
|
||||||
@@ -2160,6 +2174,31 @@ class TaskService(BaseService):
|
|||||||
|
|
||||||
return await self._auto_create_branch(task, agent_id)
|
return await self._auto_create_branch(task, agent_id)
|
||||||
|
|
||||||
|
async def _named_branch_missing_on_remote(
|
||||||
|
self, task: TaskTable, agent_id: UUID
|
||||||
|
) -> bool:
|
||||||
|
"""True only when the task's named branch is confirmed absent from origin.
|
||||||
|
|
||||||
|
False (don't re-create) when the ref is present OR the probe was
|
||||||
|
inconclusive (network error / project unresolvable) — an inconclusive
|
||||||
|
probe must fail soft so a transient glitch can't trigger a redundant
|
||||||
|
full create or fail the claim. Only a confirmed-absent result flips
|
||||||
|
this True, driving :meth:`_ensure_branch_for_task` to push the branch
|
||||||
|
a pre-set ``branch_name`` field promised but never delivered.
|
||||||
|
"""
|
||||||
|
from roboco.services.git import get_git_service
|
||||||
|
from roboco.services.project import get_project_service
|
||||||
|
|
||||||
|
project = await get_project_service(self.session).get(
|
||||||
|
UUID(str(task.project_id))
|
||||||
|
)
|
||||||
|
if project is None:
|
||||||
|
return False
|
||||||
|
probe = await get_git_service(self.session).branch_exists_on_remote(
|
||||||
|
project.slug, str(task.branch_name), agent_id
|
||||||
|
)
|
||||||
|
return probe is False
|
||||||
|
|
||||||
async def _find_ancestor_branch(self, task: TaskTable) -> str | None:
|
async def _find_ancestor_branch(self, task: TaskTable) -> str | None:
|
||||||
"""Walk up task hierarchy to find nearest ancestor with a branch.
|
"""Walk up task hierarchy to find nearest ancestor with a branch.
|
||||||
|
|
||||||
@@ -3011,6 +3050,11 @@ class TaskService(BaseService):
|
|||||||
original_claimed_at = task.claimed_at
|
original_claimed_at = task.claimed_at
|
||||||
original_heartbeat = task.last_heartbeat_at
|
original_heartbeat = task.last_heartbeat_at
|
||||||
original_claimant_id = task.active_claimant_id
|
original_claimant_id = task.active_claimant_id
|
||||||
|
# branch_name too: _ensure_branch_for_task may set it (create_branch's
|
||||||
|
# task_service.update + the in-memory assignment) before a later step
|
||||||
|
# throws — without restoring it, a retried claim sees the field set and
|
||||||
|
# the trust-but-verify short-circuits, never re-pushing the branch.
|
||||||
|
original_branch_name = task.branch_name
|
||||||
|
|
||||||
now = datetime.now(UTC)
|
now = datetime.now(UTC)
|
||||||
task.assigned_to = cast("Any", agent_id)
|
task.assigned_to = cast("Any", agent_id)
|
||||||
@@ -3035,7 +3079,12 @@ class TaskService(BaseService):
|
|||||||
|
|
||||||
await self.session.flush()
|
await self.session.flush()
|
||||||
|
|
||||||
if not task.branch_name:
|
# Always run _ensure_branch_for_task — it is the single chokepoint that
|
||||||
|
# ensures the task's branch exists on origin, whether by creating it
|
||||||
|
# (branch_name unset) or by trust-but-verify of a pre-set name
|
||||||
|
# (branch_name set: a manual field write or a prior failed create can
|
||||||
|
# leave the field set while the ref was never pushed). Branchless
|
||||||
|
# coordination/umbrella tasks return "" without touching the network.
|
||||||
try:
|
try:
|
||||||
await self._ensure_branch_for_task(task, agent_id)
|
await self._ensure_branch_for_task(task, agent_id)
|
||||||
except Exception:
|
except Exception:
|
||||||
@@ -3046,6 +3095,7 @@ class TaskService(BaseService):
|
|||||||
task.claimed_at = original_claimed_at
|
task.claimed_at = original_claimed_at
|
||||||
task.last_heartbeat_at = original_heartbeat
|
task.last_heartbeat_at = original_heartbeat
|
||||||
task.active_claimant_id = original_claimant_id
|
task.active_claimant_id = original_claimant_id
|
||||||
|
task.branch_name = original_branch_name
|
||||||
await self.session.flush()
|
await self.session.flush()
|
||||||
# emit the reversal audit row so the journey doesn't diverge
|
# emit the reversal audit row so the journey doesn't diverge
|
||||||
# from real state. The forward ``task.claimed`` audit row was
|
# from real state. The forward ``task.claimed`` audit row was
|
||||||
|
|||||||
@@ -0,0 +1,269 @@
|
|||||||
|
"""_ensure_branch_for_task trust-but-verifies a pre-set branch_name (Defect A).
|
||||||
|
|
||||||
|
A ``branch_name`` set on the task is not proof the ref is on origin — a manual
|
||||||
|
field write, or a prior failed ``create_branch`` whose rollback didn't restore
|
||||||
|
``branch_name``, can leave the field set while the branch was never pushed.
|
||||||
|
Descendants then ``ls-remote`` this name, find it empty, and silently cut from
|
||||||
|
master via ``create_branch``'s fallback, breaking the hierarchy. The
|
||||||
|
short-circuit now probes origin and pushes the branch when confirmed missing;
|
||||||
|
an inconclusive probe fails soft. The claim rollback also restores
|
||||||
|
``branch_name`` so a failed first attempt can't leave the field half-set.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from unittest.mock import AsyncMock, MagicMock, patch
|
||||||
|
from uuid import uuid4
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
from roboco.models.base import TaskStatus
|
||||||
|
from roboco.services.git import GitService
|
||||||
|
from roboco.services.task import TaskService
|
||||||
|
|
||||||
|
|
||||||
|
def _service() -> TaskService:
|
||||||
|
svc = TaskService.__new__(TaskService)
|
||||||
|
svc.log = MagicMock()
|
||||||
|
svc.session = MagicMock()
|
||||||
|
return svc
|
||||||
|
|
||||||
|
|
||||||
|
def _git_service() -> GitService:
|
||||||
|
g = GitService.__new__(GitService)
|
||||||
|
g.log = MagicMock()
|
||||||
|
g.session = MagicMock()
|
||||||
|
return g
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# _ensure_branch_for_task: trust-but-verify a pre-set branch_name
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_preset_branch_missing_on_remote_recreates() -> None:
|
||||||
|
"""branch_name set + ref confirmed absent → run _auto_create_branch (push)."""
|
||||||
|
svc = _service()
|
||||||
|
task = MagicMock(id=uuid4(), project_id=uuid4(), branch_name="feature/main_pm/x--y")
|
||||||
|
object.__setattr__(
|
||||||
|
svc, "_named_branch_missing_on_remote", AsyncMock(return_value=True)
|
||||||
|
)
|
||||||
|
auto = AsyncMock(return_value="feature/main_pm/x--y")
|
||||||
|
object.__setattr__(svc, "_auto_create_branch", auto)
|
||||||
|
|
||||||
|
out = await svc._ensure_branch_for_task(task, uuid4())
|
||||||
|
|
||||||
|
assert out == "feature/main_pm/x--y"
|
||||||
|
auto.assert_awaited_once()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_preset_branch_present_on_remote_skips_create() -> None:
|
||||||
|
"""branch_name set + ref present → return name, do not recreate."""
|
||||||
|
svc = _service()
|
||||||
|
task = MagicMock(id=uuid4(), project_id=uuid4(), branch_name="feature/main_pm/x--y")
|
||||||
|
object.__setattr__(
|
||||||
|
svc, "_named_branch_missing_on_remote", AsyncMock(return_value=False)
|
||||||
|
)
|
||||||
|
auto = AsyncMock(return_value="should-not-be-called")
|
||||||
|
object.__setattr__(svc, "_auto_create_branch", auto)
|
||||||
|
|
||||||
|
out = await svc._ensure_branch_for_task(task, uuid4())
|
||||||
|
|
||||||
|
assert out == "feature/main_pm/x--y"
|
||||||
|
auto.assert_not_awaited()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_preset_branch_inconclusive_probe_fails_soft() -> None:
|
||||||
|
"""branch_name set + probe inconclusive → return name, do not recreate.
|
||||||
|
|
||||||
|
``_named_branch_missing_on_remote`` returns False for an inconclusive
|
||||||
|
probe (None), so the short-circuit returns the name without triggering a
|
||||||
|
redundant full create — a transient network glitch can't fail the claim.
|
||||||
|
"""
|
||||||
|
svc = _service()
|
||||||
|
task = MagicMock(id=uuid4(), project_id=uuid4(), branch_name="feature/main_pm/x--y")
|
||||||
|
object.__setattr__(
|
||||||
|
svc, "_named_branch_missing_on_remote", AsyncMock(return_value=False)
|
||||||
|
)
|
||||||
|
auto = AsyncMock(return_value="should-not-be-called")
|
||||||
|
object.__setattr__(svc, "_auto_create_branch", auto)
|
||||||
|
|
||||||
|
out = await svc._ensure_branch_for_task(task, uuid4())
|
||||||
|
|
||||||
|
assert out == "feature/main_pm/x--y"
|
||||||
|
auto.assert_not_awaited()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_preset_branch_no_project_skips_verify() -> None:
|
||||||
|
"""branch_name set + no project_id (coordination/umbrella) → return name.
|
||||||
|
|
||||||
|
A branchless coordination task carries no repo, so there is nothing to
|
||||||
|
probe or push — the verify is gated on task.project_id.
|
||||||
|
"""
|
||||||
|
svc = _service()
|
||||||
|
task = MagicMock(id=uuid4(), project_id=None, branch_name="feature/main_pm/x")
|
||||||
|
probe = AsyncMock(return_value=True)
|
||||||
|
object.__setattr__(svc, "_named_branch_missing_on_remote", probe)
|
||||||
|
auto = AsyncMock(return_value="should-not-be-called")
|
||||||
|
object.__setattr__(svc, "_auto_create_branch", auto)
|
||||||
|
|
||||||
|
out = await svc._ensure_branch_for_task(task, uuid4())
|
||||||
|
|
||||||
|
assert out == "feature/main_pm/x"
|
||||||
|
probe.assert_not_awaited()
|
||||||
|
auto.assert_not_awaited()
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# _named_branch_missing_on_remote: True only on confirmed-absent
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_named_branch_missing_true_only_when_confirmed_absent() -> None:
|
||||||
|
svc = _service()
|
||||||
|
task = MagicMock(id=uuid4(), project_id=uuid4(), branch_name="feature/main_pm/x--y")
|
||||||
|
project = MagicMock(slug="roboco-api")
|
||||||
|
proj_svc = MagicMock()
|
||||||
|
proj_svc.get = AsyncMock(return_value=project)
|
||||||
|
git_svc = MagicMock()
|
||||||
|
# absent → True; present → False; inconclusive (None) → False
|
||||||
|
git_svc.branch_exists_on_remote = AsyncMock(side_effect=[False, True, None])
|
||||||
|
with (
|
||||||
|
patch(
|
||||||
|
"roboco.services.project.get_project_service",
|
||||||
|
MagicMock(return_value=proj_svc),
|
||||||
|
),
|
||||||
|
patch("roboco.services.git.get_git_service", MagicMock(return_value=git_svc)),
|
||||||
|
):
|
||||||
|
assert await svc._named_branch_missing_on_remote(task, uuid4()) is True
|
||||||
|
assert await svc._named_branch_missing_on_remote(task, uuid4()) is False
|
||||||
|
assert await svc._named_branch_missing_on_remote(task, uuid4()) is False
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_named_branch_missing_false_when_project_unresolved() -> None:
|
||||||
|
svc = _service()
|
||||||
|
task = MagicMock(id=uuid4(), project_id=uuid4(), branch_name="feature/main_pm/x--y")
|
||||||
|
proj_svc = MagicMock()
|
||||||
|
proj_svc.get = AsyncMock(return_value=None)
|
||||||
|
with patch(
|
||||||
|
"roboco.services.project.get_project_service",
|
||||||
|
MagicMock(return_value=proj_svc),
|
||||||
|
):
|
||||||
|
assert await svc._named_branch_missing_on_remote(task, uuid4()) is False
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# GitService.branch_exists_on_remote: True / False / None
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
def _run_git_result(stdout: str) -> MagicMock:
|
||||||
|
res = MagicMock()
|
||||||
|
res.stdout = stdout
|
||||||
|
return res
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_branch_exists_on_remote_present() -> None:
|
||||||
|
g = _git_service()
|
||||||
|
clone = MagicMock()
|
||||||
|
object.__setattr__(g, "get_workspace", AsyncMock(return_value=clone))
|
||||||
|
object.__setattr__(g, "_token_for_project", AsyncMock(return_value="tok"))
|
||||||
|
object.__setattr__(
|
||||||
|
g,
|
||||||
|
"_run_git",
|
||||||
|
AsyncMock(
|
||||||
|
return_value=_run_git_result("abc123\trefs/heads/feature/main_pm/x--y\n")
|
||||||
|
),
|
||||||
|
)
|
||||||
|
assert (
|
||||||
|
await g.branch_exists_on_remote("roboco-api", "feature/main_pm/x--y", uuid4())
|
||||||
|
is True
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_branch_exists_on_remote_absent() -> None:
|
||||||
|
g = _git_service()
|
||||||
|
clone = MagicMock()
|
||||||
|
object.__setattr__(g, "get_workspace", AsyncMock(return_value=clone))
|
||||||
|
object.__setattr__(g, "_token_for_project", AsyncMock(return_value="tok"))
|
||||||
|
object.__setattr__(g, "_run_git", AsyncMock(return_value=_run_git_result("")))
|
||||||
|
assert (
|
||||||
|
await g.branch_exists_on_remote("roboco-api", "feature/main_pm/x--y", uuid4())
|
||||||
|
is False
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_branch_exists_on_remote_probe_error_fails_soft() -> None:
|
||||||
|
g = _git_service()
|
||||||
|
object.__setattr__(
|
||||||
|
g, "get_workspace", AsyncMock(side_effect=RuntimeError("no workspace"))
|
||||||
|
)
|
||||||
|
object.__setattr__(g, "_token_for_project", AsyncMock(return_value="tok"))
|
||||||
|
assert (
|
||||||
|
await g.branch_exists_on_remote("roboco-api", "feature/main_pm/x--y", uuid4())
|
||||||
|
is None
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# _finalize_claim rollback restores branch_name
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_finalize_claim_rollback_restores_branch_name() -> None:
|
||||||
|
"""A failed _ensure_branch_for_task that set branch_name restores it.
|
||||||
|
|
||||||
|
Simulates create_branch setting task.branch_name (the in-memory assignment
|
||||||
|
in _create_branch_in_project) before a later step throws — the rollback
|
||||||
|
must restore branch_name so a retried claim re-runs the create instead of
|
||||||
|
short-circuiting on a half-set field.
|
||||||
|
"""
|
||||||
|
svc = _service()
|
||||||
|
agent_id = uuid4()
|
||||||
|
task = MagicMock(
|
||||||
|
id=uuid4(),
|
||||||
|
project_id=uuid4(),
|
||||||
|
branch_name=None,
|
||||||
|
status=TaskStatus.PENDING,
|
||||||
|
assigned_to=None,
|
||||||
|
claimed_by=None,
|
||||||
|
claimed_at=None,
|
||||||
|
last_heartbeat_at=None,
|
||||||
|
active_claimant_id=None,
|
||||||
|
)
|
||||||
|
agent = MagicMock()
|
||||||
|
agent.role.value = "main_pm"
|
||||||
|
|
||||||
|
object.__setattr__(svc, "_set_original_developer_context", MagicMock())
|
||||||
|
|
||||||
|
async def _set_branch_then_raise(t: MagicMock, _aid: object) -> None:
|
||||||
|
t.branch_name = "feature/main_pm/x--y"
|
||||||
|
raise RuntimeError("push failed")
|
||||||
|
|
||||||
|
object.__setattr__(
|
||||||
|
svc, "_ensure_branch_for_task", AsyncMock(side_effect=_set_branch_then_raise)
|
||||||
|
)
|
||||||
|
object.__setattr__(svc, "_validate_and_set_status", MagicMock())
|
||||||
|
object.__setattr__(svc, "_emit_status_transition_audit", MagicMock())
|
||||||
|
object.__setattr__(svc, "_create_work_session_if_needed", AsyncMock())
|
||||||
|
object.__setattr__(svc, "_inject_proactive_context", AsyncMock())
|
||||||
|
object.__setattr__(svc, "_CLAIMABLE_STATUSES", {TaskStatus.PENDING})
|
||||||
|
|
||||||
|
session = MagicMock()
|
||||||
|
session.flush = AsyncMock()
|
||||||
|
session.refresh = AsyncMock()
|
||||||
|
svc.session = session
|
||||||
|
|
||||||
|
with pytest.raises(RuntimeError, match="push failed"):
|
||||||
|
await svc._finalize_claim(task, agent, agent_id)
|
||||||
|
|
||||||
|
assert task.branch_name is None, "rollback must restore branch_name to None"
|
||||||
Reference in New Issue
Block a user