mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(gateway): allow sync_branch onto a legitimately-master base
A standalone task (video/CI-watch/dep-update — no parent) and a child of a branchless coordination parent merge into the project default branch by design, but the protected-base guard refused every master/main base, hard-wedging their rebases into block/PM/respawn churn (hit live on the v0.19.0 video task). The rebase force-pushes only the task branch (with lease) and cannot write to the base, so the guard now refuses master/main only when it is mis-resolved: a branch-bearing parent exists or the parent row is missing/corrupt. The '-'-prefixed injection guard stays unconditional.
This commit is contained in:
@@ -4146,18 +4146,24 @@ class Choreographer:
|
||||
"",
|
||||
)
|
||||
base_branch = await resolve_parent_branch(t, self.task)
|
||||
# Defense-in-depth: agents never rebase into a protected/default branch
|
||||
# or a ``-``-prefixed (shell-injection) ref. A dev task's base is its
|
||||
# parent (cell-task) branch, so this should never fire — but never let a
|
||||
# rebase reach master through a branchless-parent fallback.
|
||||
if base_branch.startswith("-") or base_branch in ("master", "main"):
|
||||
# ``-``-prefixed refs are refused unconditionally (argument-injection
|
||||
# guard). master/main is refused only when the resolution looks WRONG
|
||||
# (a branch-bearing parent exists, or the parent row is unresolvable):
|
||||
# a standalone task or a child of a branchless coordination parent
|
||||
# legitimately merges into the project default branch, and the rebase
|
||||
# only ever force-pushes the task branch (with lease) — it cannot
|
||||
# write to the base.
|
||||
if base_branch.startswith("-") or (
|
||||
base_branch in ("master", "main") and await self._base_is_misresolved(t)
|
||||
):
|
||||
return (
|
||||
Envelope.invalid_state(
|
||||
message=f"resolved base branch '{base_branch}' is protected",
|
||||
remediate=(
|
||||
"the task's base resolved to master/main; sync_branch"
|
||||
" refuses to rebase into a protected branch — escalate"
|
||||
" via i_am_blocked(reason='...') if your base is wrong"
|
||||
"the task's base resolved to master/main even though it"
|
||||
" has a parent that should own the base branch — the"
|
||||
" hierarchy resolution looks wrong; escalate via"
|
||||
" i_am_blocked(reason='...') instead of rebasing"
|
||||
),
|
||||
context_briefing=briefing,
|
||||
).with_introspection(task=t, role=role_str),
|
||||
@@ -4165,6 +4171,27 @@ class Choreographer:
|
||||
)
|
||||
return None, base_branch
|
||||
|
||||
async def _base_is_misresolved(self, t: Any) -> bool:
|
||||
"""True when a master/main base cannot be the task's real merge target.
|
||||
|
||||
Mirrors ``resolve_parent_branch``: a parentless task and a child of a
|
||||
branchless coordination parent both legitimately merge into the
|
||||
project default branch, so master/main is their true base. A
|
||||
branch-bearing parent (the base should have been that branch) or an
|
||||
unresolvable parent row means the resolution went wrong — refuse.
|
||||
"""
|
||||
parent_id = getattr(t, "parent_task_id", None)
|
||||
if parent_id is None:
|
||||
return False
|
||||
try:
|
||||
pid = UUID(str(parent_id))
|
||||
except ValueError:
|
||||
return True
|
||||
parent = await self.task.get(pid)
|
||||
if parent is None:
|
||||
return True
|
||||
return bool(parent.branch_name)
|
||||
|
||||
async def i_am_idle(self, agent_id: UUID) -> Envelope:
|
||||
"""Report no more work. Soft-block if there are unread A2As or @mentions.
|
||||
|
||||
|
||||
@@ -4012,9 +4012,9 @@ class GitService(BaseService):
|
||||
``stash`` popped into a conflict (see below).
|
||||
|
||||
Never touches the base branch and only ever force-pushes
|
||||
``head_branch`` (with ``--force-with-lease``). The caller must ensure
|
||||
``base_branch`` is not a protected/default branch — agents never
|
||||
rebase-merge into master.
|
||||
``head_branch`` (with ``--force-with-lease``). A master/main base is
|
||||
legitimate when it is the head's true merge target; the choreographer
|
||||
refuses only a mis-resolved one.
|
||||
|
||||
Safety gate (mirrors :meth:`pull`): refuses on a dirty worktree so the
|
||||
``git reset --hard`` below can't discard uncommitted agent edits —
|
||||
@@ -4181,8 +4181,10 @@ class GitService(BaseService):
|
||||
``stash`` forwards to :meth:`rebase_onto_base` — auto-stash a dirty
|
||||
worktree instead of refusing DIRTY_WORKSPACE.
|
||||
|
||||
The caller MUST ensure ``base_branch`` is not a protected branch —
|
||||
agents never rebase into master/main; the choreographer guards this.
|
||||
A master/main base is legitimate when it is the task's true merge
|
||||
target (standalone task, branchless-parent child); the choreographer
|
||||
refuses only a mis-resolved one. The push only ever targets the task
|
||||
branch.
|
||||
"""
|
||||
if not task.branch_name:
|
||||
raise ValueError("sync_task_branch requires a task with a branch_name")
|
||||
|
||||
@@ -14,7 +14,11 @@ holds. These tests pin the handler:
|
||||
- not_authorized: only the current claimant can sync (ownership gate)
|
||||
- no branch: branchless / not-yet-claimed task → invalid_state, steer to
|
||||
i_will_work_on
|
||||
- protected base: resolved base == master/main → invalid_state (defense-in-depth)
|
||||
- protected base: master/main refused only when MIS-resolved — a branch-bearing
|
||||
parent exists (base should have been that branch) or the parent row is
|
||||
missing/corrupt. A standalone (parentless) task and a child of a branchless
|
||||
coordination parent legitimately rebase onto master (it IS their merge
|
||||
target; the push only ever hits the task branch). ``-``-refs always refused.
|
||||
- git failure: sync_task_branch raises → invalid_state, steer to i_am_blocked
|
||||
"""
|
||||
|
||||
@@ -181,18 +185,20 @@ async def test_sync_branch_no_branch_steers_to_i_will_work_on() -> None:
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_sync_branch_refuses_protected_base() -> None:
|
||||
async def test_sync_branch_refuses_master_base_with_branch_bearing_parent() -> None:
|
||||
aid = uuid4()
|
||||
tid = uuid4()
|
||||
t = _task(tid=tid, aid=aid)
|
||||
t.parent_task_id = uuid4()
|
||||
task_svc = AsyncMock()
|
||||
# Both the task fetch and the parent fetch resolve to a branch-bearing row:
|
||||
# the base should have been the parent's branch, so master is mis-resolved.
|
||||
task_svc.get.return_value = t
|
||||
task_svc.agent_for.return_value = MagicMock(role="developer", team="backend")
|
||||
git_svc = AsyncMock()
|
||||
deps = _make_deps(task=task_svc, git=git_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
# Defense-in-depth: a base that resolved to master must never be rebased into.
|
||||
with patch(
|
||||
"roboco.services.gateway.choreographer._impl.resolve_parent_branch",
|
||||
new=AsyncMock(return_value="master"),
|
||||
@@ -204,6 +210,109 @@ async def test_sync_branch_refuses_protected_base() -> None:
|
||||
git_svc.sync_task_branch.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_sync_branch_allows_master_base_for_standalone_task() -> None:
|
||||
aid = uuid4()
|
||||
tid = uuid4()
|
||||
t = _task(tid=tid, aid=aid)
|
||||
# Parentless standalone task (video / ci-watch / dep-update): master IS
|
||||
# the merge target, so the rebase must go through.
|
||||
t.parent_task_id = None
|
||||
task_svc = AsyncMock()
|
||||
task_svc.get.return_value = t
|
||||
task_svc.agent_for.return_value = MagicMock(role="developer", team="backend")
|
||||
git_svc = AsyncMock()
|
||||
git_svc.sync_task_branch.return_value = {"status": "rebased", "commits_rebased": 2}
|
||||
deps = _make_deps(task=task_svc, git=git_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
with patch(
|
||||
"roboco.services.gateway.choreographer._impl.resolve_parent_branch",
|
||||
new=AsyncMock(return_value="master"),
|
||||
):
|
||||
env = await c.sync_branch(aid, tid)
|
||||
|
||||
assert env.error is None
|
||||
git_svc.sync_task_branch.assert_awaited_once_with(
|
||||
t, base_branch="master", actor_agent_id=aid, stash=False
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_sync_branch_allows_master_base_for_branchless_parent_child() -> None:
|
||||
aid = uuid4()
|
||||
tid = uuid4()
|
||||
t = _task(tid=tid, aid=aid)
|
||||
t.parent_task_id = uuid4()
|
||||
branchless_parent = MagicMock(branch_name=None)
|
||||
task_svc = AsyncMock()
|
||||
# First get: the task itself; second get: the branchless coordination
|
||||
# parent — its child was cut from master, so master is the true base.
|
||||
task_svc.get.side_effect = [t, branchless_parent]
|
||||
task_svc.agent_for.return_value = MagicMock(role="developer", team="backend")
|
||||
git_svc = AsyncMock()
|
||||
git_svc.sync_task_branch.return_value = {"status": "rebased", "commits_rebased": 1}
|
||||
deps = _make_deps(task=task_svc, git=git_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
with patch(
|
||||
"roboco.services.gateway.choreographer._impl.resolve_parent_branch",
|
||||
new=AsyncMock(return_value="master"),
|
||||
):
|
||||
env = await c.sync_branch(aid, tid)
|
||||
|
||||
assert env.error is None
|
||||
git_svc.sync_task_branch.assert_awaited_once_with(
|
||||
t, base_branch="master", actor_agent_id=aid, stash=False
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_sync_branch_refuses_master_base_when_parent_row_missing() -> None:
|
||||
aid = uuid4()
|
||||
tid = uuid4()
|
||||
t = _task(tid=tid, aid=aid)
|
||||
t.parent_task_id = uuid4()
|
||||
task_svc = AsyncMock()
|
||||
task_svc.get.side_effect = [t, None]
|
||||
task_svc.agent_for.return_value = MagicMock(role="developer", team="backend")
|
||||
git_svc = AsyncMock()
|
||||
deps = _make_deps(task=task_svc, git=git_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
with patch(
|
||||
"roboco.services.gateway.choreographer._impl.resolve_parent_branch",
|
||||
new=AsyncMock(return_value="master"),
|
||||
):
|
||||
env = await c.sync_branch(aid, tid)
|
||||
|
||||
assert env.error == "invalid_state"
|
||||
git_svc.sync_task_branch.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_sync_branch_refuses_injection_ref_unconditionally() -> None:
|
||||
aid = uuid4()
|
||||
tid = uuid4()
|
||||
t = _task(tid=tid, aid=aid)
|
||||
t.parent_task_id = None
|
||||
task_svc = AsyncMock()
|
||||
task_svc.get.return_value = t
|
||||
task_svc.agent_for.return_value = MagicMock(role="developer", team="backend")
|
||||
git_svc = AsyncMock()
|
||||
deps = _make_deps(task=task_svc, git=git_svc)
|
||||
c = Choreographer(deps)
|
||||
|
||||
with patch(
|
||||
"roboco.services.gateway.choreographer._impl.resolve_parent_branch",
|
||||
new=AsyncMock(return_value="-evil-ref"),
|
||||
):
|
||||
env = await c.sync_branch(aid, tid)
|
||||
|
||||
assert env.error == "invalid_state"
|
||||
git_svc.sync_task_branch.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_sync_branch_git_failure_steers_to_i_am_blocked() -> None:
|
||||
aid = uuid4()
|
||||
|
||||
Reference in New Issue
Block a user