mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
feat(git): protected-branches enforcement + panel editor (#649)
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 <rennf93@users.noreply.github.com>
This commit is contained in:
@@ -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"]);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<string[]>(
|
||||
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<HTMLInputElement>,
|
||||
) => {
|
||||
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({
|
||||
</p>
|
||||
</div>
|
||||
|
||||
{/* Protected Branches */}
|
||||
<div className="grid gap-2">
|
||||
<HelpTip label="Branches the fleet refuses to rebase onto, sync (force-push) as a task's own branch, or delete on the remote, in addition to the always-protected master/main defaults — matched exactly, case-sensitive. Environment-ladder rungs get separate protection, but only for task-branch cleanup, not a PR's source-branch cleanup after merge.">
|
||||
<Label htmlFor="protected_branch_input">Protected Branches</Label>
|
||||
</HelpTip>
|
||||
{protectedBranches.length > 0 && (
|
||||
<div className="flex flex-wrap gap-1.5">
|
||||
{protectedBranches.map((branch) => (
|
||||
<Badge key={branch} variant="secondary" className="gap-1 pr-1">
|
||||
{branch}
|
||||
<button
|
||||
type="button"
|
||||
onClick={() => removeProtectedBranch(branch)}
|
||||
aria-label={`Remove ${branch}`}
|
||||
className="rounded-full hover:bg-muted-foreground/20"
|
||||
>
|
||||
<X className="h-3 w-3" />
|
||||
</button>
|
||||
</Badge>
|
||||
))}
|
||||
</div>
|
||||
)}
|
||||
<div className="flex gap-2">
|
||||
<Input
|
||||
id="protected_branch_input"
|
||||
value={protectedBranchInput}
|
||||
onChange={(e) => setProtectedBranchInput(e.target.value)}
|
||||
onKeyDown={(e) => {
|
||||
if (e.key === "Enter" || e.key === ",") {
|
||||
e.preventDefault();
|
||||
addProtectedBranch();
|
||||
}
|
||||
}}
|
||||
onPaste={handleProtectedBranchPaste}
|
||||
placeholder="Type a branch name, press Enter"
|
||||
/>
|
||||
<Button
|
||||
type="button"
|
||||
variant="outline"
|
||||
size="sm"
|
||||
onClick={addProtectedBranch}
|
||||
>
|
||||
<Plus className="h-4 w-4" />
|
||||
</Button>
|
||||
</div>
|
||||
<p className="text-xs text-muted-foreground">
|
||||
Enter or comma adds a branch; click the × on a chip to remove it.
|
||||
</p>
|
||||
</div>
|
||||
|
||||
{/* Environment ladder */}
|
||||
<EnvironmentLadderEditor
|
||||
rungs={environments}
|
||||
|
||||
@@ -775,7 +775,7 @@ async def rebase_branch(
|
||||
try:
|
||||
workspace = await git_service.get_workspace(project_slug, agent.agent_id)
|
||||
conflict, conflicted_files = await git_service.rebase(
|
||||
workspace, data.target_branch
|
||||
workspace, data.target_branch, project_slug
|
||||
)
|
||||
except _TranslatableError as e:
|
||||
raise _translate_error(e) from e
|
||||
|
||||
+100
-23
@@ -1180,6 +1180,34 @@ class GitService(BaseService):
|
||||
return "master"
|
||||
return head_branch(project)
|
||||
|
||||
async def _protected_branches_for(self, project_slug: str | None) -> 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,
|
||||
|
||||
@@ -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."""
|
||||
|
||||
@@ -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 --------------------------------------------
|
||||
|
||||
|
||||
|
||||
@@ -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
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user