[F051] open_conventions_pr: refuse dirty tree + verify checkout-base landed

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 <base>`` no-ops or is refused, ``checkout -B
+        # <scaffold>`` 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 <base>`` with ``check=False`` failed — a missing base ref,
+        or a dirty tree that refused the switch). In that case ``checkout -B
+        <scaffold>`` 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

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 <base>`` with
+``check=False`` and then ``checkout -B <scaffold>``. On a dirty tree the
+``checkout <base>`` either no-ops (already on base) or is refused and silently
+swallowed; ``checkout -B <scaffold>`` 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 <base>`` fails (base ref missing locally) the code must
+    not fall through to ``checkout -B <scaffold>`` 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)