From da4d9b333d1dd51f3054f16d5d136de65a00c819 Mon Sep 17 00:00:00 2001 From: Renzo F <45401804+rennf93@users.noreply.github.com> Date: Wed, 22 Jul 2026 20:30:05 +0200 Subject: [PATCH] feat(git): protected-branches enforcement + panel editor (#649) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit projects.protected_branches existed end-to-end but nothing consulted it — the panel had no editor and the git safety checks used hardcoded sets. Now: GitService._protected_branches_for(slug) (frozenset, stripped, fail-open to the hardcoded floor with a warning log) is unioned — never replacing, only tightening — into rebase()'s refusal set, the shared _delete_remote_branch_best_effort skip set (threaded through every caller: task cleanup, PR merge/close cleanup), and sync_task_branch, which now refuses to force-push a protected-named head (the dev-facing sync_branch verb path the HTTP-only fix would have missed). Matching is exact and case-sensitive; an empty list degrades to exactly the old hardcoded behavior, pinned by union-floor regression tests (master/main stay refused regardless of the project list). Panel: chips editor for the field in the edit-project dialog (add via Enter/comma, paste-splitting on comma-separated lists, dedup, clear-to- empty persists []) with an honest tooltip scoped to what is actually enforced. Tests cover both the incumbent GitHub-App dialog suite and the new Protected Branches suite in one harness. Co-authored-by: Renn F --- .../__tests__/edit-project-dialog.test.tsx | 118 +++++++++ .../projects/edit-project-dialog.tsx | 91 ++++++- roboco/api/routes/git.py | 2 +- roboco/services/git.py | 123 +++++++-- tests/unit/gateway/test_sync_branch.py | 32 +++ .../test_git_branch_deletion_guard.py | 188 +++++++++++++ tests/unit/services/test_git_rebase.py | 246 ++++++++++++++++++ 7 files changed, 775 insertions(+), 25 deletions(-) diff --git a/panel/src/components/projects/__tests__/edit-project-dialog.test.tsx b/panel/src/components/projects/__tests__/edit-project-dialog.test.tsx index 04e5c739..2bc66c0e 100644 --- a/panel/src/components/projects/__tests__/edit-project-dialog.test.tsx +++ b/panel/src/components/projects/__tests__/edit-project-dialog.test.tsx @@ -267,3 +267,121 @@ describe("EditProjectDialog — GitHub App binding", () => { expect(call.updates.github_installation_id).toBeNull(); }); }); + +describe("EditProjectDialog — Protected Branches", () => { + beforeEach(() => { + vi.clearAllMocks(); + getCredentialsStatus.mockResolvedValue({ has_credentials: true }); + mutateAsync.mockResolvedValue(makeProject()); + useUpdateProject.mockReturnValue({ mutateAsync, isPending: false }); + }); + + it("renders the project's existing protected branches as chips", async () => { + renderDialog(makeProject({ protected_branches: ["master", "slave"] })); + + await screen.findByText("master"); + expect(screen.getByText("slave")).toBeInTheDocument(); + }); + + it("adds a branch via Enter and removes another via its chip, saving both changes", async () => { + renderDialog(makeProject({ protected_branches: ["master", "slave"] })); + await screen.findByText("master"); + + // Remove "slave". + fireEvent.click(screen.getByLabelText("Remove slave")); + expect(screen.queryByText("slave")).not.toBeInTheDocument(); + + // Add "release" by typing + Enter. + const input = screen.getByPlaceholderText( + "Type a branch name, press Enter", + ); + fireEvent.change(input, { target: { value: "release" } }); + fireEvent.keyDown(input, { key: "Enter" }); + expect(screen.getByText("release")).toBeInTheDocument(); + // The input clears after a successful add. + expect(input).toHaveValue(""); + + fireEvent.click(screen.getByRole("button", { name: /Save Changes/i })); + + await waitFor(() => expect(mutateAsync).toHaveBeenCalled()); + const call = mutateAsync.mock.calls[0][0] as { + updates: { protected_branches?: string[] }; + }; + expect(call.updates.protected_branches).toEqual(["master", "release"]); + }); + + it("adding via a trailing comma also commits the chip", async () => { + renderDialog(makeProject({ protected_branches: ["master", "slave"] })); + await screen.findByText("master"); + + const input = screen.getByPlaceholderText( + "Type a branch name, press Enter", + ); + fireEvent.change(input, { target: { value: "hotfix" } }); + fireEvent.keyDown(input, { key: "," }); + expect(screen.getByText("hotfix")).toBeInTheDocument(); + }); + + it("pasting a comma-separated list splits it into individual chips instead of one malformed chip", async () => { + renderDialog(makeProject({ protected_branches: ["master", "slave"] })); + await screen.findByText("master"); + + const input = screen.getByPlaceholderText( + "Type a branch name, press Enter", + ); + fireEvent.paste(input, { + clipboardData: { getData: () => "release,hotfix,staging" }, + }); + + expect(screen.getByText("release")).toBeInTheDocument(); + expect(screen.getByText("hotfix")).toBeInTheDocument(); + expect(screen.getByText("staging")).toBeInTheDocument(); + expect( + screen.queryByText("release,hotfix,staging"), + ).not.toBeInTheDocument(); + expect(input).toHaveValue(""); + }); + + it("does not add a duplicate chip for a branch already in the list", async () => { + renderDialog(makeProject({ protected_branches: ["master", "slave"] })); + await screen.findByText("master"); + + const input = screen.getByPlaceholderText( + "Type a branch name, press Enter", + ); + fireEvent.change(input, { target: { value: "master" } }); + fireEvent.keyDown(input, { key: "Enter" }); + expect(screen.getAllByText("master")).toHaveLength(1); + }); + + it("clearing every branch sends an explicit empty array, not an omitted field", async () => { + renderDialog(makeProject({ protected_branches: ["master", "slave"] })); + await screen.findByText("master"); + + fireEvent.click(screen.getByLabelText("Remove master")); + fireEvent.click(screen.getByLabelText("Remove slave")); + expect(screen.queryByText("master")).not.toBeInTheDocument(); + expect(screen.queryByText("slave")).not.toBeInTheDocument(); + + fireEvent.click(screen.getByRole("button", { name: /Save Changes/i })); + + await waitFor(() => expect(mutateAsync).toHaveBeenCalled()); + const call = mutateAsync.mock.calls[0][0] as { + updates: { protected_branches?: string[] }; + }; + expect(call.updates.protected_branches).toEqual([]); + }); + + it("leaving the list untouched still round-trips the same branches on save", async () => { + renderDialog(makeProject({ protected_branches: ["master", "slave"] })); + await screen.findByText("master"); + + fireEvent.click(screen.getByRole("button", { name: /Save Changes/i })); + + await waitFor(() => expect(mutateAsync).toHaveBeenCalled()); + const call = mutateAsync.mock.calls[0][0] as { + updates: { protected_branches?: string[] }; + }; + expect(call.updates.protected_branches).toEqual(["master", "slave"]); + }); +}); diff --git a/panel/src/components/projects/edit-project-dialog.tsx b/panel/src/components/projects/edit-project-dialog.tsx index 7b095db4..f23f89b4 100644 --- a/panel/src/components/projects/edit-project-dialog.tsx +++ b/panel/src/components/projects/edit-project-dialog.tsx @@ -23,9 +23,10 @@ import { } from "@/components/ui/select"; import { Switch } from "@/components/ui/switch"; import { Skeleton } from "@/components/ui/skeleton"; +import { Badge } from "@/components/ui/badge"; import { Tabs, TabsContent, TabsList, TabsTrigger } from "@/components/ui/tabs"; import { ConventionsTab } from "@/components/conventions/conventions-tab"; -import { Key, KeyRound, AlertTriangle } from "lucide-react"; +import { Key, KeyRound, AlertTriangle, Plus, X } from "lucide-react"; import { toast } from "sonner"; import { Team, type ProjectUpdate, type Project } from "@/types"; import { githubAppApi } from "@/lib/api"; @@ -165,6 +166,43 @@ function EditProjectForm({ >(project.github_installation_id); const [assignedCell, setAssignedCell] = useState(project.assigned_cell); const [defaultBranch, setDefaultBranch] = useState(project.default_branch); + const [protectedBranches, setProtectedBranches] = useState( + project.protected_branches ?? [], + ); + const [protectedBranchInput, setProtectedBranchInput] = useState(""); + // Shared by the single Enter/comma-key add and the multi-value paste + // handler below — trims, drops empties, and dedups against both the + // existing list and duplicates within the same batch. + const addProtectedBranches = (names: string[]) => { + const cleaned = names.map((n) => n.trim()).filter(Boolean); + if (cleaned.length === 0) return; + setProtectedBranches((prev) => { + const next = [...prev]; + for (const name of cleaned) { + if (!next.includes(name)) next.push(name); + } + return next; + }); + }; + const addProtectedBranch = () => { + addProtectedBranches([protectedBranchInput]); + setProtectedBranchInput(""); + }; + const handleProtectedBranchPaste = ( + e: React.ClipboardEvent, + ) => { + const pasted = e.clipboardData.getData("text"); + // A single name (no comma) falls through to normal paste-into-input + // behavior; only a multi-value paste is split into chips directly — + // otherwise "release,hotfix,staging" lands as one malformed chip. + if (!pasted.includes(",")) return; + e.preventDefault(); + addProtectedBranches(pasted.split(",")); + setProtectedBranchInput(""); + }; + const removeProtectedBranch = (branch: string) => { + setProtectedBranches((prev) => prev.filter((b) => b !== branch)); + }; const [environments, setEnvironments] = useState( project.environments ?? null, ); @@ -284,6 +322,7 @@ function EditProjectForm({ github_installation_id: githubInstallationId, assigned_cell: assignedCell, default_branch: defaultBranch || "main", + protected_branches: protectedBranches, environments, is_active: isActive, test_command: testCommand || undefined, @@ -571,6 +610,56 @@ function EditProjectForm({

+ {/* Protected Branches */} +
+ + + + {protectedBranches.length > 0 && ( +
+ {protectedBranches.map((branch) => ( + + {branch} + + + ))} +
+ )} +
+ setProtectedBranchInput(e.target.value)} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === ",") { + e.preventDefault(); + addProtectedBranch(); + } + }} + onPaste={handleProtectedBranchPaste} + placeholder="Type a branch name, press Enter" + /> + +
+

+ Enter or comma adds a branch; click the × on a chip to remove it. +

+
+ {/* Environment ladder */} frozenset[str]: + """The project's own ``protected_branches``, normalized. + + Consulted by every hardcoded rebase/delete safety gate as a UNION + with its own literal set — this can only ADD branches to what's + refused, never remove one, so a missing/unresolvable project or an + emptied field degrades to exactly the prior hardcoded-only behavior. + Branch names are matched case-sensitively (git refs are); entries are + stripped of surrounding whitespace defensively. + """ + if not project_slug: + return frozenset() + try: + project = await get_project_service(self.session).get_by_slug(project_slug) + except Exception as e: + # Fail-open by design (additive-only protection degrading to the + # hardcoded floor is the right posture) — but log it, so a DB + # blip silently narrowing protection is at least observable. + self.log.warning( + "protected_branches lookup failed; degrading to hardcoded floor only", + project_slug=project_slug, + error=str(e), + ) + return frozenset() + if project is None or not project.protected_branches: + return frozenset() + return frozenset(b.strip() for b in project.protected_branches if b.strip()) + async def _checkout_base_with_fallback( self, workspace: Path, @@ -1893,13 +1921,17 @@ class GitService(BaseService): return await self.get_status(workspace) async def rebase( - self, workspace: Path, target_branch: str + self, workspace: Path, target_branch: str, project_slug: str | None = None ) -> tuple[bool, list[str]]: """Rebase the current branch onto target_branch. Safety gate: raises :class:`ValidationError` if the HEAD branch or - ``target_branch`` is ``master`` or ``main`` — rebasing a protected - integration branch is never safe in automation. + ``target_branch`` is ``master``/``main``, OR one of the project's own + declared ``protected_branches`` (when ``project_slug`` is given) — + rebasing a protected integration branch is never safe in automation. + ``master``/``main`` are refused unconditionally regardless of the + project's list (see :meth:`_protected_branches_for`): the union can + only tighten what's refused, never loosen it. On conflict (non-zero exit): captures unmerged files via ``git diff --name-only --diff-filter=U``, aborts the rebase to @@ -1907,17 +1939,21 @@ class GitService(BaseService): On success: returns ``(False, [])``. """ - _PROTECTED = frozenset({"master", "main"}) + _PROTECTED = frozenset({"master", "main"}) | await self._protected_branches_for( + project_slug + ) if target_branch in _PROTECTED: raise ValidationError( f"REBASE_FORBIDDEN: Cannot rebase onto '{target_branch}'. " - "Rebasing onto 'master' or 'main' is not allowed in automation." + "Rebasing onto 'master', 'main', or a project-declared " + "protected branch is not allowed in automation." ) head_branch = await self.get_current_branch(workspace) if head_branch in _PROTECTED: raise ValidationError( f"REBASE_FORBIDDEN: Cannot rebase '{head_branch}'. " - "Rebasing 'master' or 'main' is not allowed in automation." + "Rebasing 'master', 'main', or a project-declared protected " + "branch is not allowed in automation." ) result = await self._run_git(workspace, ["rebase", target_branch], check=False) if result.returncode != 0: @@ -3607,19 +3643,30 @@ class GitService(BaseService): return True async def _delete_remote_branch_best_effort( - self, repo_ref: RepoRef, branch: str, git_token: str + self, + repo_ref: RepoRef, + branch: str, + git_token: str, + project_slug: str | None = None, ) -> bool: """Best-effort: delete a remote branch by name. Silently swallows errors — cleanup is not critical. Skips branches that - look like project defaults (main / master / develop) and any branch that - still has open dependent PRs (an active integration target — deleting it - would strand in-flight child work). Returns True if the delete request - was issued with no transport error, False on any skip/failure — callers - that only fire-and-forget can ignore it; the branch-cleanup sweep uses - it to report counts. + look like project defaults (main / master / develop), any branch in + the project's own declared ``protected_branches`` (when + ``project_slug`` is given — a UNION with the hardcoded set, so a + missing/emptied field only ever loses the extra protection, never the + main/master/develop floor), and any branch that still has open + dependent PRs (an active integration target — deleting it would + strand in-flight child work). Returns True if the delete request was + issued with no transport error, False on any skip/failure — callers + that only fire-and-forget can ignore it; the branch-cleanup sweep + uses it to report counts. """ - if branch in ("main", "master", "develop", ""): + protected = frozenset( + ("main", "master", "develop", "") + ) | await self._protected_branches_for(project_slug) + if branch in protected: return False if await self._branch_has_open_dependents(repo_ref, branch, git_token): self.log.info( @@ -3638,11 +3685,18 @@ class GitService(BaseService): return False async def _delete_pr_branch_best_effort( - self, repo_ref: RepoRef, pr_number: int, git_token: str + self, + repo_ref: RepoRef, + pr_number: int, + git_token: str, + project_slug: str | None = None, ) -> None: """Best-effort: delete the PR's source branch on the remote after merge. Silently swallows errors — branch cleanup is not critical. + ``project_slug``, when given, is forwarded to + :meth:`_delete_remote_branch_best_effort` so its own protected-branch + union covers this path too. """ try: pr_resp = await self._forge.get_pr( @@ -3653,7 +3707,9 @@ class GitService(BaseService): branch = (pr_resp.json().get("head") or {}).get("ref") if not branch: return - await self._delete_remote_branch_best_effort(repo_ref, branch, git_token) + await self._delete_remote_branch_best_effort( + repo_ref, branch, git_token, project_slug + ) except httpx.HTTPError: return @@ -3690,7 +3746,7 @@ class GitService(BaseService): except Exception: return False return await self._delete_remote_branch_best_effort( - repo_ref, branch_name, git_token + repo_ref, branch_name, git_token, project_slug ) async def close_task_pr_best_effort( @@ -3998,7 +4054,9 @@ class GitService(BaseService): status_code=resp.status_code, ) - await self._delete_pr_branch_best_effort(repo_ref, pr_number, git_token) + await self._delete_pr_branch_best_effort( + repo_ref, pr_number, git_token, project_slug + ) target_branch = await self._project_default_branch(project_slug) # Default branch always exists on origin, so the plain sync is correct @@ -4978,7 +5036,9 @@ class GitService(BaseService): target=target, ) ) - await self._delete_pr_branch_best_effort(repo_ref, pr_number, git_token) + await self._delete_pr_branch_best_effort( + repo_ref, pr_number, git_token, project.slug + ) merge_commit = await self._sync_target_branch_best_effort( workspace, target, git_token ) @@ -5207,15 +5267,30 @@ class GitService(BaseService): worktree instead of refusing DIRTY_WORKSPACE. 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. + target (standalone task, branchless-parent child); the choreographer's + ``_sync_base_refused`` guards THAT (untouched by this refusal). This + method separately guards what it actually force-pushes: the HEAD/task + branch. If ``task.branch_name`` itself is master/main or one of the + project's declared ``protected_branches``, ``branch_name`` was + mis-set and force-pushing (with lease) over it is exactly what the + field exists to prevent — refuse before touching any workspace. """ if not task.branch_name: raise ValueError("sync_task_branch 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)) + protected_heads = frozenset( + {"master", "main"} + ) | await self._protected_branches_for(project.slug) + if str(task.branch_name) in protected_heads: + raise ValidationError( + f"REBASE_FORBIDDEN: task branch_name '{task.branch_name}' is a " + "protected branch (master/main or a project-declared " + "protected_branches entry) — branch_name was mis-set; " + "force-pushing over it is exactly what protected_branches " + "exists to prevent. Escalate via i_am_blocked(reason='...')." + ) workspace_agent_id = self._resolve_workspace_agent_id(task, actor_agent_id) clone_root = await self.get_workspace(project.slug, agent_id=workspace_agent_id) git_token = await self._get_project_token_or_raise(project.slug) @@ -5447,7 +5522,9 @@ class GitService(BaseService): {"owner": repo_ref.owner, "repo": repo_ref.repo, "pr": pr_number}, ) if delete_branch: - await self._delete_pr_branch_best_effort(repo_ref, pr_number, git_token) + await self._delete_pr_branch_best_effort( + repo_ref, pr_number, git_token, project.slug + ) async def pr_target( self, diff --git a/tests/unit/gateway/test_sync_branch.py b/tests/unit/gateway/test_sync_branch.py index 5df1ece8..7b8d0fdb 100644 --- a/tests/unit/gateway/test_sync_branch.py +++ b/tests/unit/gateway/test_sync_branch.py @@ -28,6 +28,7 @@ from unittest.mock import AsyncMock, MagicMock, patch from uuid import uuid4 import pytest +from roboco.services.base import ValidationError from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps @@ -336,6 +337,37 @@ async def test_sync_branch_git_failure_steers_to_i_am_blocked() -> None: assert "i_am_blocked" in (env.remediate or "") +@pytest.mark.asyncio +async def test_sync_branch_protected_head_refusal_steers_to_i_am_blocked() -> None: + """GitService.sync_task_branch's protected-HEAD-branch guard (2026-07-22 + follow-up — a mis-set branch_name matching master/main or a project's + declared protected_branches) surfaces through the same generic + invalid_state/i_am_blocked path as any other git failure — the + choreographer doesn't need to special-case it, only propagate it.""" + aid = uuid4() + tid = uuid4() + t = _task(tid=tid, aid=aid) + 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.side_effect = ValidationError( + f"REBASE_FORBIDDEN: task branch_name '{_BRANCH}' is a protected branch" + ) + 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=_BASE), + ): + env = await c.sync_branch(aid, tid) + + assert env.error == "invalid_state" + assert "REBASE_FORBIDDEN" in (env.message or "") + assert "i_am_blocked" in (env.remediate or "") + + @pytest.mark.asyncio async def test_sync_branch_passes_stash_flag_through() -> None: """stash=True on the verb forwards to GitService.sync_task_branch.""" diff --git a/tests/unit/services/test_git_branch_deletion_guard.py b/tests/unit/services/test_git_branch_deletion_guard.py index 64b7db28..41ac1842 100644 --- a/tests/unit/services/test_git_branch_deletion_guard.py +++ b/tests/unit/services/test_git_branch_deletion_guard.py @@ -80,6 +80,194 @@ async def test_delete_skips_default_branch_before_checking_dependents() -> None: dep.assert_not_awaited() +# --- projects.protected_branches UNION (2026-07-22 follow-up) ------------- +# `_delete_remote_branch_best_effort` unions its hardcoded skip tuple with +# the project's own declared `protected_branches` when a project_slug is +# given. The union can only ADD protection: a missing project_slug, an +# unresolvable project, or an emptied field must reproduce the exact +# hardcoded-only behavior above. + + +def _project_service_returning(project: MagicMock) -> MagicMock: + svc = MagicMock() + svc.get_by_slug = AsyncMock(return_value=project) + return svc + + +@pytest.mark.asyncio +async def test_delete_skips_project_declared_protected_branch() -> None: + """A custom protected branch (not in the hardcoded set) is refused when + the project declares it — the open-dependents probe is never reached, + mirroring the hardcoded-name short-circuit above.""" + svc = _service() + dep = AsyncMock(return_value=False) + _bind(svc, "_branch_has_open_dependents", dep) + client = _fake_client() + project = MagicMock(protected_branches=["release"]) + with ( + patch("roboco.services.git.httpx.AsyncClient", return_value=client), + patch( + "roboco.services.git.get_project_service", + return_value=_project_service_returning(project), + ), + ): + await svc._delete_remote_branch_best_effort( + RepoRef("acme", "repo"), "release", "tok", "acme-repo" + ) + client.delete.assert_not_awaited() + dep.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_delete_allows_branch_not_in_projects_protected_list() -> None: + """A branch that isn't hardcoded AND isn't in the project's declared + list is deleted normally — the union only blocks what's actually + listed, it doesn't become deny-by-default.""" + svc = _service() + _bind(svc, "_branch_has_open_dependents", AsyncMock(return_value=False)) + client = _fake_client() + project = MagicMock(protected_branches=["release"]) + with ( + patch("roboco.services.git.httpx.AsyncClient", return_value=client), + patch( + "roboco.services.git.get_project_service", + return_value=_project_service_returning(project), + ), + ): + await svc._delete_remote_branch_best_effort( + RepoRef("acme", "repo"), + "feature/backend/abc--cell--leaf", + "tok", + "acme-repo", + ) + client.delete.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_delete_empty_protected_branches_matches_hardcoded_only_behavior() -> ( + None +): + """An empty (or null) protected_branches field degrades to exactly the + prior hardcoded-only behavior — clearing the list never loosens + anything, but it also never invents new protection.""" + svc = _service() + _bind(svc, "_branch_has_open_dependents", AsyncMock(return_value=False)) + client = _fake_client() + project = MagicMock(protected_branches=[]) + with ( + patch("roboco.services.git.httpx.AsyncClient", return_value=client), + patch( + "roboco.services.git.get_project_service", + return_value=_project_service_returning(project), + ), + ): + await svc._delete_remote_branch_best_effort( + RepoRef("acme", "repo"), + "feature/backend/abc--cell--leaf", + "tok", + "acme-repo", + ) + client.delete.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_delete_no_project_slug_matches_hardcoded_only_behavior() -> None: + """Omitting project_slug entirely (legacy call shape) never touches the + project service and behaves byte-for-byte like before this change.""" + svc = _service() + _bind(svc, "_branch_has_open_dependents", AsyncMock(return_value=False)) + client = _fake_client() + with ( + patch("roboco.services.git.httpx.AsyncClient", return_value=client), + patch("roboco.services.git.get_project_service") as get_project_service, + ): + await svc._delete_remote_branch_best_effort( + RepoRef("acme", "repo"), "feature/backend/abc--cell--leaf", "tok" + ) + client.delete.assert_awaited_once() + get_project_service.assert_not_called() + + +@pytest.mark.asyncio +async def test_delete_matches_stripped_branch_case_sensitively() -> None: + """Stored entries are stripped of whitespace defensively, but matching + stays case-sensitive (git branch names are case-sensitive): a + differently-cased request is NOT protected by a stored ' Release '.""" + svc = _service() + dep = AsyncMock(return_value=False) + _bind(svc, "_branch_has_open_dependents", dep) + client = _fake_client() + project = MagicMock(protected_branches=[" Release "]) + with ( + patch("roboco.services.git.httpx.AsyncClient", return_value=client), + patch( + "roboco.services.git.get_project_service", + return_value=_project_service_returning(project), + ), + ): + # Exact match after stripping -> refused. + await svc._delete_remote_branch_best_effort( + RepoRef("acme", "repo"), "Release", "tok", "acme-repo" + ) + client.delete.assert_not_awaited() + dep.assert_not_awaited() + + client2 = _fake_client() + with ( + patch("roboco.services.git.httpx.AsyncClient", return_value=client2), + patch( + "roboco.services.git.get_project_service", + return_value=_project_service_returning(project), + ), + ): + # Different case -> not the same git ref -> allowed. + await svc._delete_remote_branch_best_effort( + RepoRef("acme", "repo"), "release", "tok", "acme-repo" + ) + client2.delete.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_delete_union_never_collapses_hardcoded_floor_to_project_list_only() -> ( + None +): + """A project declaring its OWN protected_branches (e.g. ["release"]) must + NOT replace the hardcoded main/master/develop skip — the union is + additive, never a substitution. master and main stay refused regardless + of what the project's list contains.""" + project = MagicMock(protected_branches=["release"]) + + svc = _service() + dep = AsyncMock(return_value=False) + _bind(svc, "_branch_has_open_dependents", dep) + client = _fake_client() + with ( + patch("roboco.services.git.httpx.AsyncClient", return_value=client), + patch( + "roboco.services.git.get_project_service", + return_value=_project_service_returning(project), + ), + ): + await svc._delete_remote_branch_best_effort( + RepoRef("acme", "repo"), "master", "tok", "acme-repo" + ) + client.delete.assert_not_awaited() + dep.assert_not_awaited() + + client2 = _fake_client() + with ( + patch("roboco.services.git.httpx.AsyncClient", return_value=client2), + patch( + "roboco.services.git.get_project_service", + return_value=_project_service_returning(project), + ), + ): + await svc._delete_remote_branch_best_effort( + RepoRef("acme", "repo"), "main", "tok", "acme-repo" + ) + client2.delete.assert_not_awaited() + + # --- the open-dependents probe -------------------------------------------- diff --git a/tests/unit/services/test_git_rebase.py b/tests/unit/services/test_git_rebase.py index c7b421c0..b79f4bc3 100644 --- a/tests/unit/services/test_git_rebase.py +++ b/tests/unit/services/test_git_rebase.py @@ -64,6 +64,9 @@ def _git_service() -> GitService: """Instantiate GitService without a real DB session.""" svc = GitService.__new__(GitService) svc.log = MagicMock() # silence warning/info calls + # A placeholder — only touched (as an opaque arg to a patched + # get_project_service) by the protected_branches union tests below. + svc.session = MagicMock() return svc @@ -303,6 +306,249 @@ async def test_rebase_raises_validation_error_when_head_branch_is_main( await svc.rebase(_WORKSPACE, "feature/backend/some-task") +# --------------------------------------------------------------------------- +# rebase() — projects.protected_branches UNION (2026-07-22 follow-up) +# --------------------------------------------------------------------------- +# rebase()'s hardcoded {"master", "main"} refusal is unioned with the +# project's own declared protected_branches when project_slug is given. The +# union can only ADD refusals: no project_slug, an unresolvable project, or +# an emptied field must reproduce the exact hardcoded-only behavior above. + + +def _project_service_returning(project: MagicMock) -> MagicMock: + svc = MagicMock() + svc.get_by_slug = AsyncMock(return_value=project) + return svc + + +@pytest.mark.asyncio +async def test_rebase_raises_for_project_declared_protected_target_branch() -> None: + """A custom protected branch (not master/main) is refused as a rebase + target when the project declares it, before any git command runs.""" + svc = _git_service() + project = MagicMock(protected_branches=["release"]) + with ( + patch( + "roboco.services.git.get_project_service", + return_value=_project_service_returning(project), + ), + pytest.raises(ValidationError, match="REBASE_FORBIDDEN"), + ): + await svc.rebase(_WORKSPACE, "release", "acme-repo") + + +@pytest.mark.asyncio +async def test_rebase_allows_target_not_in_projects_protected_list( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A target that isn't master/main AND isn't in the project's declared + list rebases normally — the union doesn't become deny-by-default.""" + run = AsyncMock(return_value=_result()) + monkeypatch.setattr(GitService, "_run_git", run) + monkeypatch.setattr( + GitService, "get_current_branch", AsyncMock(return_value="feature/x") + ) + svc = _git_service() + project = MagicMock(protected_branches=["release"]) + with patch( + "roboco.services.git.get_project_service", + return_value=_project_service_returning(project), + ): + conflict, files = await svc.rebase( + _WORKSPACE, "feature/backend/some-task", "acme-repo" + ) + assert (conflict, files) == (False, []) + + +@pytest.mark.asyncio +async def test_rebase_empty_protected_branches_matches_hardcoded_only_behavior( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """An empty protected_branches field degrades to exactly the prior + master/main-only behavior — no extra refusal is invented.""" + run = AsyncMock(return_value=_result()) + monkeypatch.setattr(GitService, "_run_git", run) + monkeypatch.setattr( + GitService, "get_current_branch", AsyncMock(return_value="feature/x") + ) + svc = _git_service() + project = MagicMock(protected_branches=[]) + with patch( + "roboco.services.git.get_project_service", + return_value=_project_service_returning(project), + ): + conflict, files = await svc.rebase( + _WORKSPACE, "feature/backend/some-task", "acme-repo" + ) + assert (conflict, files) == (False, []) + + +@pytest.mark.asyncio +async def test_rebase_no_project_slug_matches_hardcoded_only_behavior( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Omitting project_slug entirely (legacy call shape) never touches the + project service and behaves byte-for-byte like before this change.""" + run = AsyncMock(return_value=_result()) + monkeypatch.setattr(GitService, "_run_git", run) + monkeypatch.setattr( + GitService, "get_current_branch", AsyncMock(return_value="feature/x") + ) + svc = _git_service() + with patch("roboco.services.git.get_project_service") as get_project_service: + conflict, files = await svc.rebase(_WORKSPACE, "feature/backend/some-task") + assert (conflict, files) == (False, []) + get_project_service.assert_not_called() + + +@pytest.mark.asyncio +async def test_rebase_matches_stripped_target_case_sensitively() -> None: + """Stored entries are stripped defensively, but matching stays + case-sensitive: a differently-cased target is NOT refused by a stored + ' Release '.""" + project = MagicMock(protected_branches=[" Release "]) + + svc = _git_service() + with ( + patch( + "roboco.services.git.get_project_service", + return_value=_project_service_returning(project), + ), + pytest.raises(ValidationError, match="REBASE_FORBIDDEN"), + ): + await svc.rebase(_WORKSPACE, "Release", "acme-repo") + + +@pytest.mark.asyncio +async def test_rebase_union_never_collapses_hardcoded_floor_to_project_list_only() -> ( + None +): + """A project declaring its OWN protected_branches (e.g. ["release"]) must + NOT replace the hardcoded master/main refusal — the union is additive, + never a substitution. Both master and main stay refused regardless of + what the project's list contains.""" + project = MagicMock(protected_branches=["release"]) + + svc = _git_service() + with ( + patch( + "roboco.services.git.get_project_service", + return_value=_project_service_returning(project), + ), + pytest.raises(ValidationError, match="REBASE_FORBIDDEN"), + ): + await svc.rebase(_WORKSPACE, "master", "acme-repo") + + svc2 = _git_service() + with ( + patch( + "roboco.services.git.get_project_service", + return_value=_project_service_returning(project), + ), + pytest.raises(ValidationError, match="REBASE_FORBIDDEN"), + ): + await svc2.rebase(_WORKSPACE, "main", "acme-repo") + + +# --------------------------------------------------------------------------- +# sync_task_branch() — protected HEAD branch guard (2026-07-22 follow-up) +# --------------------------------------------------------------------------- +# sync_branch force-pushes (with lease) the task's OWN branch_name, not the +# base — so the guard that matters here is on the HEAD, not the base (the +# choreographer's _sync_base_refused already guards the base and is +# untouched). A branch_name that IS master/main or one of the project's +# declared protected_branches means branch_name was mis-set; refuse before +# any workspace/rebase work. + + +def _sync_task(branch_name: str) -> MagicMock: + # assigned_to=None so _resolve_workspace_agent_id (no actor_agent_id + # passed) falls through to its None default instead of trying to parse + # an auto-generated MagicMock attribute as a UUID. + return MagicMock(id=uuid4(), branch_name=branch_name, assigned_to=None) + + +def _patch_sync_plumbing( + monkeypatch: pytest.MonkeyPatch, project: MagicMock +) -> AsyncMock: + """Stub every collaborator sync_task_branch calls AFTER the HEAD guard, + so a test that reaches them proves the guard let a normal branch through + without actually touching a filesystem or running git. Returns the + rebase_onto_base mock so callers can assert on it.""" + monkeypatch.setattr( + GitService, "_project_for_task", AsyncMock(return_value=project) + ) + # _protected_branches_for (called from the new HEAD guard) resolves the + # project independently via get_project_service, not _project_for_task. + monkeypatch.setattr( + "roboco.services.git.get_project_service", + lambda _session: _project_service_returning(project), + ) + monkeypatch.setattr( + GitService, "get_workspace", AsyncMock(return_value=Path("/tmp/clone")) + ) + monkeypatch.setattr( + GitService, "_get_project_token_or_raise", AsyncMock(return_value="tok") + ) + monkeypatch.setattr(GitService, "_ensure_worktree_for_commit", AsyncMock()) + rebase_onto_base = AsyncMock( + return_value={"status": "rebased", "unique_commits": 1} + ) + monkeypatch.setattr(GitService, "rebase_onto_base", rebase_onto_base) + return rebase_onto_base + + +@pytest.mark.asyncio +async def test_sync_task_branch_refuses_project_declared_protected_head( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A task whose branch_name IS a project-declared protected branch is + refused before any workspace/rebase work runs.""" + project = MagicMock(slug="acme-repo", protected_branches=["release"]) + rebase_onto_base = _patch_sync_plumbing(monkeypatch, project) + svc = _git_service() + task = _sync_task("release") + + with pytest.raises(ValidationError, match="REBASE_FORBIDDEN"): + await svc.sync_task_branch(task, base_branch="feature/backend/parent") + + rebase_onto_base.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_sync_task_branch_allows_normal_head_unaffected( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A normal task branch_name (not in the project's protected list) syncs + through exactly as before — the guard doesn't become deny-by-default.""" + project = MagicMock(slug="acme-repo", protected_branches=["release"]) + rebase_onto_base = _patch_sync_plumbing(monkeypatch, project) + svc = _git_service() + task = _sync_task("feature/backend/abc12345") + + result = await svc.sync_task_branch(task, base_branch="feature/backend/parent") + + assert result == {"status": "rebased", "unique_commits": 1} + rebase_onto_base.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_sync_task_branch_empty_protected_branches_matches_current_behavior( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """An empty (or null) protected_branches field degrades to exactly the + prior master/main-only behavior — a normal branch still syncs fine.""" + project = MagicMock(slug="acme-repo", protected_branches=[]) + rebase_onto_base = _patch_sync_plumbing(monkeypatch, project) + svc = _git_service() + task = _sync_task("feature/backend/abc12345") + + result = await svc.sync_task_branch(task, base_branch="feature/backend/parent") + + assert result == {"status": "rebased", "unique_commits": 1} + rebase_onto_base.assert_awaited_once() + + # --------------------------------------------------------------------------- # pull() safety-gate tests # ---------------------------------------------------------------------------