fix(git): push + open PR on the task branch by name, not the current checkout

A developer's single clone is shared across all of their tasks, so by the
QA-submission / open_pr boundary the workspace is usually parked on a LATER
task's branch. push_task_branch / push_for_task asserted the current branch
and pushed it, and create_pull_request used get_current_branch as the PR head
— so for an earlier task the push was rejected (BRANCH_MISMATCH) and the
locally-committed work never reached origin, leaving the task branch empty and
open_pr failing with GitHub's "No commits between" 422. The work was committed
correctly on the local task branch, just never pushed.

Operate on the task's recorded branch by name, independent of the checkout:
- push() takes an explicit branch and pushes that named ref
- push_task_branch / push_for_task push the task's branch by name (drop the
  assert-on-current-branch gate that rejected the shared-clone case)
- create_pull_request uses the task's branch_name as the PR head via the new
  _pr_head_branch helper

Regression tests assert push and PR-head target the task branch from any
checkout. 45 git + 735 gateway + 59 git-integration tests green; ruff/mypy/
xenon clean.
This commit is contained in:
Renn F
2026-06-24 08:20:19 +02:00
parent 8cc8f15551
commit 7b0c8291bd
3 changed files with 105 additions and 14 deletions
+2
View File
@@ -14,6 +14,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
- **The orchestrator's own recovery actions now actually run.** Its background dispatcher made internal HTTP calls to its own API without an agent identity, so every self-`PATCH` to a task — auto-blocking a task with missing prerequisites, auto-resuming a PM's paused parent, auto-recovering a stale-blocked parent, annotating an SLA breach — was rejected with `401 Missing X-Agent-ID` and silently dropped. The visible effect was paused/blocked parent tasks staying wedged and their dependent work stranded (with the dispatcher logging a "respawning assignee" loop). Header propagation was inconsistent across the orchestrator's separate HTTP-client call-sites — only the main dispatch loop sent the identity. The system identity is now hoisted into one shared constant and applied to every API-facing dispatcher client (the external provider-recovery probe is intentionally excluded); the `system` role holds the permission required for the audited status-override path those routes use. - **The orchestrator's own recovery actions now actually run.** Its background dispatcher made internal HTTP calls to its own API without an agent identity, so every self-`PATCH` to a task — auto-blocking a task with missing prerequisites, auto-resuming a PM's paused parent, auto-recovering a stale-blocked parent, annotating an SLA breach — was rejected with `401 Missing X-Agent-ID` and silently dropped. The visible effect was paused/blocked parent tasks staying wedged and their dependent work stranded (with the dispatcher logging a "respawning assignee" loop). Header propagation was inconsistent across the orchestrator's separate HTTP-client call-sites — only the main dispatch loop sent the identity. The system identity is now hoisted into one shared constant and applied to every API-facing dispatcher client (the external provider-recovery probe is intentionally excluded); the `system` role holds the permission required for the audited status-override path those routes use.
- **A developer's completed work no longer silently fails to reach GitHub ("No commits between").** A developer's single git clone is shared across all of their tasks, so by the time a task's PR is opened the clone has usually moved on to a *later* task's branch. The push at the QA-submission / `open_pr` boundary, and the PR's head branch, were both taken from the clone's *current* checkout — so the push was rejected (the workspace was parked on another task's branch) and the locally-committed work never reached `origin`, leaving the task branch empty and `open_pr` failing with GitHub's "No commits between" 422. The work was on disk and correct, just never pushed. Both the push and the PR head now operate on the task's recorded branch **by name**, independent of the checkout (`push(branch=…)` targets the named ref; the PR head is the task's `branch_name`). Work committed on any of a shared clone's task branches now pushes and opens its PR correctly.
- **Hitting the Claude session limit now parks the workforce instead of crash-looping it.** When the org's Claude usage ("5-hour") limit is reached, each agent container exits with a 429 rejection; the orchestrator was treating that like any crash and immediately respawning the agent straight back into the limit, over and over, across the whole fleet. It already parks the provider on a persistent server *overload* (529/500/503) and revives the parked work once it recovers — but that detection only matched the overload signatures, not the session-limit 429. The same park-and-resume break now also recognizes the session limit: the provider is parked, dispatch goes quiet, and the background probe loop brings the agents back automatically when the window resets — no churn, no wasted respawns. - **Hitting the Claude session limit now parks the workforce instead of crash-looping it.** When the org's Claude usage ("5-hour") limit is reached, each agent container exits with a 429 rejection; the orchestrator was treating that like any crash and immediately respawning the agent straight back into the limit, over and over, across the whole fleet. It already parks the provider on a persistent server *overload* (529/500/503) and revives the parked work once it recovers — but that detection only matched the overload signatures, not the session-limit 429. The same park-and-resume break now also recognizes the session limit: the provider is parked, dispatch goes quiet, and the background probe loop brings the agents back automatically when the window resets — no churn, no wasted respawns.
- **A failed PR review no longer looks green.** On a task's detail page, the "PR Reviewer Notes" card was painted a fixed teal/green background regardless of the review verdict, so a `Failed` review — red badge and all — sat inside a green card and could read as passing at a glance. The card background now mirrors the verdict the way the QA Notes card already does: red on a failed review, green on approved/passed, amber on changes-requested, and neutral before a verdict is in. - **A failed PR review no longer looks green.** On a task's detail page, the "PR Reviewer Notes" card was painted a fixed teal/green background regardless of the review verdict, so a `Failed` review — red badge and all — sat inside a green card and could read as passing at a glance. The card background now mirrors the verdict the way the QA Notes card already does: red on a failed review, green on approved/passed, amber on changes-requested, and neutral before a verdict is in.
+42 -10
View File
@@ -1150,12 +1150,20 @@ class GitService(BaseService):
workspace = await self.get_workspace(data.project_slug, agent_id) workspace = await self.get_workspace(data.project_slug, agent_id)
await self.checkout(workspace, data.branch) await self.checkout(workspace, data.branch)
async def push(self, workspace: Path, force: bool = False) -> tuple[str, int]: async def push(
self, workspace: Path, force: bool = False, branch: str | None = None
) -> tuple[str, int]:
"""Push commits to remote. """Push commits to remote.
``branch`` pushes that named local branch by ref (``git push origin
<branch>``), independent of the workspace's current checkout — a dev's
single clone is shared across many tasks, so by push time it is often
parked on a LATER task's branch. Defaults to the current branch.
Returns: (branch, commits_pushed) Returns: (branch, commits_pushed)
""" """
branch = await self.get_current_branch(workspace) if branch is None:
branch = await self.get_current_branch(workspace)
token = await self._token_for_workspace(workspace) token = await self._token_for_workspace(workspace)
count_result = await self._run_git( count_result = await self._run_git(
@@ -1231,14 +1239,15 @@ class GitService(BaseService):
), ),
) )
force = getattr(data, "force", False)
if data.task_id is not None: if data.task_id is not None:
task = await self._assert_task_owned_with_branch(data.task_id, agent_id) task = await self._assert_task_owned_with_branch(data.task_id, agent_id)
workspace = await self.get_workspace(data.project_slug, agent_id) workspace = await self.get_workspace(data.project_slug, agent_id)
await self._assert_on_task_branch(workspace, task.branch_name) # Push the task's branch BY NAME, not the current checkout — the
else: # shared clone is often parked on a later task's branch by now.
workspace = await self.get_workspace(data.project_slug, agent_id) return await self.push(workspace, force, branch=str(task.branch_name))
workspace = await self.get_workspace(data.project_slug, agent_id)
return await self.push(workspace, getattr(data, "force", False)) return await self.push(workspace, force)
async def push_task_branch(self, agent_id: UUID, task_id: UUID) -> int: async def push_task_branch(self, agent_id: UUID, task_id: UUID) -> int:
"""Idempotently push a task's branch to origin; return commits pushed. """Idempotently push a task's branch to origin; return commits pushed.
@@ -1256,8 +1265,14 @@ class GitService(BaseService):
if project is None: if project is None:
return 0 return 0
workspace = await self.get_workspace(project.slug, agent_id) workspace = await self.get_workspace(project.slug, agent_id)
await self._assert_on_task_branch(workspace, task.branch_name) # Push the task's branch BY NAME, independent of the current checkout.
_branch, pushed = await self.push(workspace) # The dev's clone is shared across tasks, so by the QA-submission /
# open_pr boundary it is usually parked on a LATER task's branch; the
# old assert-on-current-branch then push-current rejected the push, and
# the locally-committed work never reached origin → open_pr then saw
# "No commits between" (origin branch empty/missing). The local task
# branch ref carries the commits; push it by name.
_branch, pushed = await self.push(workspace, branch=str(task.branch_name))
return pushed return pushed
# ========================================================================= # =========================================================================
@@ -1882,6 +1897,23 @@ class GitService(BaseService):
) )
return default_branch return default_branch
async def _pr_head_branch(
self, workspace: Path, request: GitCreatePRRequest
) -> str:
"""The PR head branch — the task's recorded branch by name.
A dev's single clone is shared across many tasks, so by open_pr time the
workspace is often parked on a LATER task's branch; ``get_current_branch``
would open the PR for the wrong head (or fail). Use the task's
``branch_name`` when ``task_id`` is set; fall back to the current branch.
"""
if request.task_id is not None:
task_service = get_task_service(self.session)
task = await task_service.get(request.task_id)
if task and task.branch_name:
return str(task.branch_name)
return await self.get_current_branch(workspace)
async def create_pull_request( async def create_pull_request(
self, workspace: Path, request: GitCreatePRRequest self, workspace: Path, request: GitCreatePRRequest
) -> tuple[int, str, str, str, str]: ) -> tuple[int, str, str, str, str]:
@@ -1894,7 +1926,7 @@ class GitService(BaseService):
Returns: (pr_number, pr_url, title, source_branch, target_branch) Returns: (pr_number, pr_url, title, source_branch, target_branch)
""" """
source_branch = await self.get_current_branch(workspace) source_branch = await self._pr_head_branch(workspace, request)
default_branch = await self._project_default_branch(request.project_slug) default_branch = await self._project_default_branch(request.project_slug)
git_token = await self._get_project_token_or_raise(request.project_slug) git_token = await self._get_project_token_or_raise(request.project_slug)
target_branch, pr_title, pr_body = await self._resolve_new_pr_context( target_branch, pr_title, pr_body = await self._resolve_new_pr_context(
+61 -4
View File
@@ -134,22 +134,79 @@ async def test_project_for_task_uses_project_id_when_present() -> None:
@pytest.mark.asyncio @pytest.mark.asyncio
async def test_push_task_branch_resolves_workspace_and_pushes() -> None: async def test_push_task_branch_pushes_task_branch_by_name() -> None:
"""Resolves the task's project + workspace, then pushes; returns the count.""" """Pushes the task's branch BY NAME (independent of the current checkout).
A dev's clone is shared across many tasks, so by the QA-submission /
open_pr boundary it is usually parked on a LATER task's branch. The old
assert-on-current-branch gate rejected the push and the locally-committed
work never reached origin; the push must target the named ref instead.
"""
task = MagicMock(branch_name="feature/backend/abc") task = MagicMock(branch_name="feature/backend/abc")
project = MagicMock(slug="roboco") project = MagicMock(slug="roboco")
svc = _service() svc = _service()
_bind(svc, "_assert_task_owned_with_branch", AsyncMock(return_value=task)) _bind(svc, "_assert_task_owned_with_branch", AsyncMock(return_value=task))
_bind(svc, "_project_for_task", AsyncMock(return_value=project)) _bind(svc, "_project_for_task", AsyncMock(return_value=project))
_bind(svc, "get_workspace", AsyncMock(return_value=Path("/tmp/ws"))) _bind(svc, "get_workspace", AsyncMock(return_value=Path("/tmp/ws")))
_bind(svc, "_assert_on_task_branch", AsyncMock()) assert_branch = AsyncMock()
_bind(svc, "_assert_on_task_branch", assert_branch)
push_mock = AsyncMock(return_value=("feature/backend/abc", _PUSHED_COMMIT_COUNT)) push_mock = AsyncMock(return_value=("feature/backend/abc", _PUSHED_COMMIT_COUNT))
_bind(svc, "push", push_mock) _bind(svc, "push", push_mock)
pushed = await svc.push_task_branch(uuid4(), uuid4()) pushed = await svc.push_task_branch(uuid4(), uuid4())
assert pushed == _PUSHED_COMMIT_COUNT assert pushed == _PUSHED_COMMIT_COUNT
push_mock.assert_awaited_once_with(Path("/tmp/ws")) push_mock.assert_awaited_once_with(Path("/tmp/ws"), branch="feature/backend/abc")
# The old current-branch gate is no longer consulted on the push path.
assert_branch.assert_not_awaited()
@pytest.mark.asyncio
async def test_push_targets_explicit_branch_not_current_checkout() -> None:
"""push(branch=X) pushes X by ref even when the workspace is on Y."""
svc = _service()
_bind(svc, "get_current_branch", AsyncMock(return_value="feature/frontend/OTHER"))
_bind(svc, "_token_for_workspace", AsyncMock(return_value=None))
calls: list[list[str]] = []
async def _run_git(_ws: object, args: list[str], **_kw: object) -> MagicMock:
calls.append(args)
res = MagicMock()
res.returncode = 0
res.stdout = "3" if args[:2] == ["rev-list", "--count"] else ""
return res
_bind(svc, "_run_git", AsyncMock(side_effect=_run_git))
branch, _pushed = await svc.push(Path("/tmp/ws"), branch="feature/frontend/TASK")
assert branch == "feature/frontend/TASK"
push_args = next(a for a in calls if a and a[0] == "push")
assert "feature/frontend/TASK" in push_args
assert "feature/frontend/OTHER" not in push_args
@pytest.mark.asyncio
async def test_pr_head_is_task_branch_not_current() -> None:
"""The PR head is the task's recorded branch, not the workspace checkout."""
task = MagicMock(branch_name="feature/frontend/TASK")
svc = _service()
_bind(svc, "get_current_branch", AsyncMock(return_value="feature/frontend/OTHER"))
req = MagicMock(task_id=uuid4())
with patch("roboco.services.git.get_task_service") as gts:
gts.return_value.get = AsyncMock(return_value=task)
head = await svc._pr_head_branch(Path("/tmp/ws"), req)
assert head == "feature/frontend/TASK"
@pytest.mark.asyncio
async def test_pr_head_falls_back_to_current_when_no_task() -> None:
"""No task_id → the PR head is the current checkout (unchanged behavior)."""
svc = _service()
_bind(svc, "get_current_branch", AsyncMock(return_value="feature/frontend/CUR"))
req = MagicMock(task_id=None)
head = await svc._pr_head_branch(Path("/tmp/ws"), req)
assert head == "feature/frontend/CUR"
@pytest.mark.asyncio @pytest.mark.asyncio