From 7b0c8291bd606d9878bfeb60b35b36e07b996bf8 Mon Sep 17 00:00:00 2001 From: Renn F Date: Wed, 24 Jun 2026 08:20:19 +0200 Subject: [PATCH] fix(git): push + open PR on the task branch by name, not the current checkout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 2 + roboco/services/git.py | 52 +++++++++++++++++++++----- tests/unit/services/test_git.py | 65 +++++++++++++++++++++++++++++++-- 3 files changed, 105 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 302bf25e..a6f8e7e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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. +- **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. - **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. diff --git a/roboco/services/git.py b/roboco/services/git.py index 680852f6..d464eebb 100644 --- a/roboco/services/git.py +++ b/roboco/services/git.py @@ -1150,12 +1150,20 @@ class GitService(BaseService): workspace = await self.get_workspace(data.project_slug, agent_id) 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. + ``branch`` pushes that named local branch by ref (``git push origin + ``), 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) """ - 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) 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: task = await self._assert_task_owned_with_branch(data.task_id, agent_id) workspace = await self.get_workspace(data.project_slug, agent_id) - await self._assert_on_task_branch(workspace, task.branch_name) - else: - workspace = await self.get_workspace(data.project_slug, agent_id) - - return await self.push(workspace, getattr(data, "force", False)) + # Push the task's branch BY NAME, not the current checkout — the + # shared clone is often parked on a later task's branch by now. + 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, force) async def push_task_branch(self, agent_id: UUID, task_id: UUID) -> int: """Idempotently push a task's branch to origin; return commits pushed. @@ -1256,8 +1265,14 @@ class GitService(BaseService): if project is None: return 0 workspace = await self.get_workspace(project.slug, agent_id) - await self._assert_on_task_branch(workspace, task.branch_name) - _branch, pushed = await self.push(workspace) + # Push the task's branch BY NAME, independent of the current checkout. + # 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 # ========================================================================= @@ -1882,6 +1897,23 @@ class GitService(BaseService): ) 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( self, workspace: Path, request: GitCreatePRRequest ) -> tuple[int, str, str, str, str]: @@ -1894,7 +1926,7 @@ class GitService(BaseService): 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) git_token = await self._get_project_token_or_raise(request.project_slug) target_branch, pr_title, pr_body = await self._resolve_new_pr_context( diff --git a/tests/unit/services/test_git.py b/tests/unit/services/test_git.py index ef8aac6e..f9f7173e 100644 --- a/tests/unit/services/test_git.py +++ b/tests/unit/services/test_git.py @@ -134,22 +134,79 @@ async def test_project_for_task_uses_project_id_when_present() -> None: @pytest.mark.asyncio -async def test_push_task_branch_resolves_workspace_and_pushes() -> None: - """Resolves the task's project + workspace, then pushes; returns the count.""" +async def test_push_task_branch_pushes_task_branch_by_name() -> None: + """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") project = MagicMock(slug="roboco") svc = _service() _bind(svc, "_assert_task_owned_with_branch", AsyncMock(return_value=task)) _bind(svc, "_project_for_task", AsyncMock(return_value=project)) _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)) _bind(svc, "push", push_mock) pushed = await svc.push_task_branch(uuid4(), uuid4()) 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