From 824c44416127882689b568bb16c3f1944894a5f2 Mon Sep 17 00:00:00 2001 From: Renn F Date: Sun, 28 Jun 2026 15:45:54 +0200 Subject: [PATCH] [F051] open_conventions_pr: refuse dirty tree + verify checkout-base landed --- roboco/services/git.py | 36 +++- .../services/test_git_conventions_pr_dirty.py | 184 ++++++++++++++++++ 2 files changed, 218 insertions(+), 2 deletions(-) create mode 100644 tests/unit/services/test_git_conventions_pr_dirty.py diff --git a/roboco/services/git.py b/roboco/services/git.py index cc52efbf..543f563a 100644 --- a/roboco/services/git.py +++ b/roboco/services/git.py @@ -4364,24 +4364,56 @@ class GitService(BaseService): body=body, ) original = await self.get_current_branch(ws) + # A dirty tree is an agent's active workspace. Cutting the scaffold + # branch here would sweep their uncommitted work into the conventions + # commit — ``checkout `` no-ops or is refused, ``checkout -B + # `` carries the dirty change, and ``commit`` captures it, + # so the agent's in-progress edit rides a project-level PR they never + # intended and vanishes from their working tree. Refuse outright before + # any checkout touches the tree. + if not await self._working_tree_is_clean(ws): + return None try: - await self._commit_conventions_file(ws, base, spec) + if not await self._commit_conventions_file(ws, base, spec): + return None return await self._push_and_open_conventions_pr( project_slug, ws, base, spec ) finally: await self._run_git(ws, ["checkout", original], check=False) + async def _working_tree_is_clean(self, workspace: Path) -> bool: + """True iff ``git status --porcelain`` is empty (no staged/unstaged + changes, no untracked entries). Used to gate workspace-mutating + project-level ops (the conventions scaffold) away from an agent's + active dirty tree.""" + result = await self._run_git(workspace, ["status", "--porcelain"]) + return not result.stdout.strip() + async def _commit_conventions_file( self, workspace: Path, base: str, spec: _ConventionsPr - ) -> None: + ) -> bool: + """Commit the conventions file on the scaffold branch cut from ``base``. + + Returns False when the scaffold cannot be safely cut from ``base`` (the + ``checkout `` with ``check=False`` failed — a missing base ref, + or a dirty tree that refused the switch). In that case ``checkout -B + `` would cut the scaffold from the *current* branch (the + agent's task branch), basing a project-level PR on the agent's work; + the caller refuses rather than commit on the wrong base. + """ await self._run_git(workspace, ["checkout", base], check=False) + # check=False swallows the failed checkout; verify we actually landed + # on base before cutting the scaffold from here. + if await self.get_current_branch(workspace) != base: + return False await self._run_git(workspace, ["checkout", "-B", spec.branch]) target = workspace / ".roboco" / "conventions.yml" target.parent.mkdir(parents=True, exist_ok=True) target.write_text(spec.content) await self._run_git(workspace, ["add", ".roboco/conventions.yml"]) await self._run_git(workspace, ["commit", "-m", spec.title]) + return True async def _push_and_open_conventions_pr( self, project_slug: str, workspace: Path, base: str, spec: _ConventionsPr diff --git a/tests/unit/services/test_git_conventions_pr_dirty.py b/tests/unit/services/test_git_conventions_pr_dirty.py new file mode 100644 index 00000000..46be612f --- /dev/null +++ b/tests/unit/services/test_git_conventions_pr_dirty.py @@ -0,0 +1,184 @@ +"""F051: open_conventions_pr must not operate on a dirty working tree. + +``open_conventions_pr`` cuts its scaffold branch in an agent's clone (or the +project's shared ``workspace_path``). It does ``checkout `` with +``check=False`` and then ``checkout -B ``. On a dirty tree the +``checkout `` either no-ops (already on base) or is refused and silently +swallowed; ``checkout -B `` then carries the agent's uncommitted +work onto the scaffold branch, and the ``commit`` sweeps it into the +project-level conventions commit — the agent's in-progress change is gone +from their working tree and rides a PR they never intended. Refuse a dirty +tree up front (return None, no checkout) so an active workspace is never +touched. +""" + +from __future__ import annotations + +import subprocess +from typing import TYPE_CHECKING +from unittest.mock import AsyncMock, MagicMock + +import pytest +from roboco.services import git as git_module +from roboco.services.git import GitService + +if TYPE_CHECKING: + from pathlib import Path + +_SCAFFOLD_BRANCH = "chore/roboco-conventions-scaffold" + + +def _git(repo: Path, *args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["git", *args], cwd=repo, check=True, capture_output=True, text=True + ) + + +def _init_repo(repo: Path) -> None: + repo.mkdir(parents=True, exist_ok=True) + _git(repo, "init", "-b", "master") + _git(repo, "config", "user.email", "t@example.com") + _git(repo, "config", "user.name", "T") + _git(repo, "config", "commit.gpgsign", "false") + (repo / "README.md").write_text("# r\n") + _git(repo, "add", "README.md") + _git(repo, "commit", "-m", "init") + + +def _svc(monkeypatch: pytest.MonkeyPatch, repo: Path) -> GitService: + svc = GitService.__new__(GitService) + svc.session = AsyncMock() + + project = MagicMock() + project.workspace_path = str(repo) + project.default_branch = "master" + project_service = MagicMock() + project_service.get_by_slug = AsyncMock(return_value=project) + monkeypatch.setattr(git_module, "get_project_service", lambda _s: project_service) + # No remote token → push/PR skipped; we only exercise the local commit path. + monkeypatch.setattr(svc, "_token_for_project", AsyncMock(return_value=None)) + return svc + + +def _branch_exists(repo: Path, branch: str) -> bool: + res = subprocess.run( + ["git", "rev-parse", "--verify", branch], + cwd=repo, + capture_output=True, + text=True, + check=False, + ) + return res.returncode == 0 + + +@pytest.mark.asyncio +async def test_dirty_tree_refused_before_any_checkout( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A dirty working tree is the agent's active workspace — refuse, return + None, and leave the tree exactly as it was (no scaffold branch, dirty + change still uncommitted in the working tree).""" + repo = tmp_path / "repo" + _init_repo(repo) + # Dirty the tree: modify a tracked file (the agent's in-progress work). + (repo / "README.md").write_text("# dirty work in progress\n") + + svc = _svc(monkeypatch, repo) + + result = await svc.open_conventions_pr( + "g-proj", content="version: 1\n", title="scaffold", body="b" + ) + + # Refused. + assert result is None + # The scaffold branch was never created (no commit landed anywhere). + assert not _branch_exists(repo, _SCAFFOLD_BRANCH) + # The agent's dirty change is still in the working tree, uncommitted. + status = subprocess.run( + ["git", "status", "--porcelain"], + cwd=repo, + capture_output=True, + text=True, + check=True, + ).stdout + assert "README.md" in status + # master's history is untouched (still just the init commit). + log = subprocess.run( + ["git", "log", "--oneline", "master"], + cwd=repo, + capture_output=True, + text=True, + check=True, + ).stdout.strip() + assert log.count("\n") == 0 # exactly one commit on master + + +@pytest.mark.asyncio +async def test_clean_tree_proceeds_and_commits_on_scaffold( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A clean tree proceeds: the conventions file is committed on the + scaffold branch cut from master, and master itself is untouched + (regression guard for the fix not over-rejecting the clean case).""" + repo = tmp_path / "repo" + _init_repo(repo) + + svc = _svc(monkeypatch, repo) + + result = await svc.open_conventions_pr( + "g-proj", content="version: 1\n", title="scaffold", body="b" + ) + + assert result is not None + assert result["branch"] == _SCAFFOLD_BRANCH + assert result["pr_number"] is None # no token → no remote PR + assert _branch_exists(repo, _SCAFFOLD_BRANCH) + show = subprocess.run( + ["git", "show", f"{_SCAFFOLD_BRANCH}:.roboco/conventions.yml"], + cwd=repo, + capture_output=True, + text=True, + check=False, + ) + assert show.returncode == 0 + assert show.stdout == "version: 1\n" + # The working tree ends back on master, clean. + assert ( + subprocess.run( + ["git", "branch", "--show-current"], + cwd=repo, + capture_output=True, + text=True, + check=True, + ).stdout.strip() + == "master" + ) + + +@pytest.mark.asyncio +async def test_missing_base_branch_refused( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """If ``checkout `` fails (base ref missing locally) the code must + not fall through to ``checkout -B `` from the current branch — + that would base the scaffold on the agent's task branch. Refuse when the + checkout doesn't actually land on base.""" + repo = tmp_path / "repo" + _init_repo(repo) + + svc = _svc(monkeypatch, repo) + # Override the project default to a branch that doesn't exist locally. + project = MagicMock() + project.workspace_path = str(repo) + project.default_branch = "nonexistent-base" + project_service = MagicMock() + project_service.get_by_slug = AsyncMock(return_value=project) + monkeypatch.setattr(git_module, "get_project_service", lambda _s: project_service) + + result = await svc.open_conventions_pr( + "g-proj", content="version: 1\n", title="scaffold", body="b" + ) + + # Refused — no scaffold branch fabricated on top of master. + assert result is None + assert not _branch_exists(repo, _SCAFFOLD_BRANCH)