From 9e4025b82229b67195468407cbd09218ef630c66 Mon Sep 17 00:00:00 2001 From: Renzo F <45401804+rennf93@users.noreply.github.com> Date: Wed, 8 Jul 2026 16:38:32 +0200 Subject: [PATCH] fix(git): scope post-op ownership repair to what the op could change (#337) Co-authored-by: Renn F --- roboco/services/git.py | 113 +++++++- roboco/services/workspace.py | 45 +++ .../unit/services/test_git_ownership_scope.py | 271 ++++++++++++++++++ ...test_workspace_ensure_agent_owned_scope.py | 32 +++ 4 files changed, 447 insertions(+), 14 deletions(-) create mode 100644 tests/unit/services/test_git_ownership_scope.py diff --git a/roboco/services/git.py b/roboco/services/git.py index 339fa84a..4a7f0977 100644 --- a/roboco/services/git.py +++ b/roboco/services/git.py @@ -216,6 +216,74 @@ def _remove_stale_git_locks(workspace: Path) -> None: return +# Verbs that never write anything — `_run_git`'s post-op ownership repair is +# pure waste after these (live NAS: a "rev-parse --verify" cost 5165ms of +# chown for a command that touches nothing). Read-only forms of `branch` / +# `symbolic-ref` are handled separately below since they share a verb name +# with a mutating form. +_READ_ONLY_GIT_VERBS = frozenset( + { + "status", + "log", + "diff", + "rev-parse", + "rev-list", + "ls-remote", + "merge-base", + "show", + "cherry", + } +) + +# Verbs that write only inside `.git/` (refs, objects, index) — never the +# working tree. Ownership repair can be scoped to `.git/` alone instead of a +# full-workspace walk. +_GIT_SCOPED_VERBS = frozenset({"add", "commit", "fetch", "push"}) + + +def _branch_or_symbolic_ref_scope(verb: str, rest: list[str]) -> str: + """Query vs SET form of `branch` / `symbolic-ref` (same verb, different + scope): query reads only, SET writes a ref under `.git/`. + + Query: `branch --show-current` (0 positional) or `symbolic-ref [-q] + ` (1 positional — naming which ref to read). SET (`branch + `, `symbolic-ref `) always carries 2+ positional args. + """ + positional = [a for a in rest if not a.startswith("-")] + is_query = (verb == "branch" and not positional) or ( + verb == "symbolic-ref" and len(positional) <= 1 + ) + return "none" if is_query else "git" + + +def _git_ownership_scope(args: list[str]) -> str: + """Classify a git invocation's post-op ownership-repair scope. + + Root cause of the chown cost: the orchestrator's git subprocess runs as + root, so a MUTATING op leaves root-owned files under `.git/` (and, for + checkout/reset/rebase/pull, the working tree) that the agent container + (uid 1000) can't write. A read-only op never writes anything, so + repairing ownership after one is pure waste — on the NAS this cost + 5-10s PER git call, and a single `i_am_done` chains ~a dozen ops. + + Returns "none" (skip repair — zero syscalls), "git" (repair `.git/` + only, worktree-aware via `_resolve_clone_root`), or "full" (repair the + whole workspace — unchanged behavior, and the safe default for + checkout/reset/rebase/pull or any verb this classifier doesn't + recognize, so an unclassified op is never under-repaired). + """ + if not args: + return "full" + verb = args[0] + if verb in _READ_ONLY_GIT_VERBS: + return "none" + if verb in ("branch", "symbolic-ref"): + return _branch_or_symbolic_ref_scope(verb, args[1:]) + if verb in _GIT_SCOPED_VERBS: + return "git" + return "full" + + # `_get_gh_env` and the gh-CLI code paths were removed in favor of direct # GitHub REST API calls — no CLI dependency, and the PAT no longer touches # subprocess argv / environ. @@ -330,14 +398,15 @@ class GitService(BaseService): ``settings.git_commit_timeout_seconds``. After every orchestrator-side git op, hand ownership back to the - agent user. Git commands here run as root and create root-owned - files under .git/ (refs, logs/refs, packed-refs, index, objects). - If we don't re-chown, the agent container (uid 1000) can't append - to those files on its next commit and fails with - "unable to append to .git/logs/refs/heads/...". + agent user — SCOPED to what this op could actually have written + (see `_git_ownership_scope`). Git commands here run as root and + create root-owned files under .git/ (refs, logs/refs, packed-refs, + index, objects). If we don't re-chown, the agent container (uid + 1000) can't append to those files on its next commit and fails + with "unable to append to .git/logs/refs/heads/...". A read-only + op (status, log, diff, ...) never writes, so it skips the repair + entirely. """ - from roboco.services.workspace import _ensure_agent_owned - effective_timeout = timeout if timeout is not None else _default_git_timeout() prefix: list[str] = [] @@ -381,13 +450,7 @@ class GitService(BaseService): " ".join(args), e.stderr or e.stdout or "Unknown error" ) from e git_ms = (time.monotonic() - t0) * 1000.0 - - # Hand .git (and tracked files) back to the agent: this root-run op - # created root-owned files under .git/. Runs in the dedicated git pool - # so it doesn't compete with the event loop's default executor. - t1 = time.monotonic() - await loop.run_in_executor(_GIT_EXECUTOR, _ensure_agent_owned, workspace) - chown_ms = (time.monotonic() - t1) * 1000.0 + chown_ms = await self._reown_after_git_op(loop, workspace, args) # Surface slow git/chown ops (instrumentation): a single line that # pinpoints where an op's time went — the subprocess (e.g. a push to @@ -403,6 +466,28 @@ class GitService(BaseService): ) return result + @staticmethod + async def _reown_after_git_op( + loop: asyncio.AbstractEventLoop, workspace: Path, args: list[str] + ) -> float: + """Run the scope-appropriate post-op ownership repair; return its ms cost. + + Extracted out of `_run_git` so the classify-then-dispatch logic stays + out of that method's cyclomatic budget. Runs in the dedicated git + executor so it doesn't compete with the event loop's default pool. + A "none"-scope op costs zero syscalls and returns 0.0 so the slow-op + instrumentation still sees the true (near-zero) cost. + """ + from roboco.services.workspace import _ensure_agent_owned, _ensure_git_dir_owned + + scope = _git_ownership_scope(args) + if scope == "none": + return 0.0 + repair = _ensure_git_dir_owned if scope == "git" else _ensure_agent_owned + t1 = time.monotonic() + await loop.run_in_executor(_GIT_EXECUTOR, repair, workspace) + return (time.monotonic() - t1) * 1000.0 + async def _token_for_project(self, project_slug: str) -> str | None: """Decrypted project token for orchestrator-side remote git ops. diff --git a/roboco/services/workspace.py b/roboco/services/workspace.py index 7a3748f9..523f556a 100644 --- a/roboco/services/workspace.py +++ b/roboco/services/workspace.py @@ -195,6 +195,51 @@ def _resolve_clone_root(workspace: Path) -> Path: return workspace +def _iter_git_dir_entries(clone_root: Path) -> Iterator[str]: + """Yield ``clone_root/.git`` and every entry beneath it. + + No ``_PRUNE_DIRS`` filtering needed — ``.git`` never contains the heavy + gitignored/agent-regenerated trees (node_modules, .venv, ...) that make a + full-workspace walk expensive. + """ + git_dir = clone_root / ".git" + if not git_dir.is_dir(): + return + yield str(git_dir) + for root, dirs, files in os.walk(git_dir): + for name in (*dirs, *files): + yield str(Path(root) / name) + + +def _ensure_git_dir_owned(workspace: Path) -> None: + """Chown + group-write ONLY ``.git/`` — the fast path for git ops that can + only ever write refs/objects/index, never the working tree (add, commit, + fetch, push; the SET forms of branch/symbolic-ref). + + Worktree-aware: resolves through ``_resolve_clone_root`` so an op run + inside a ``.worktrees/`` checkout repairs the SHARED + ``clone_root/.git`` — where the worktree's own per-task admin dir + (``.git/worktrees//``) plus the shared refs/objects actually live — + not the worktree's own ``.git``, which is just a small gitlink FILE. + """ + clone_root = _resolve_clone_root(workspace) + if not clone_root.exists(): + return + + failed_chowns = sum( + _own_and_grant_rw(entry) for entry in _iter_git_dir_entries(clone_root) + ) + + if failed_chowns: + logger.warning( + "Some chowns failed during ensure_git_dir_owned — " + "agent .git writes may still fail. Check docker user-namespace " + "config or run agents as root on this host.", + workspace=str(clone_root), + failures=failed_chowns, + ) + + def _uv_subprocess_env(workspace: Path) -> dict[str, str]: """Env for a uv subprocess run by the orchestrator (root). diff --git a/tests/unit/services/test_git_ownership_scope.py b/tests/unit/services/test_git_ownership_scope.py new file mode 100644 index 00000000..db3ee082 --- /dev/null +++ b/tests/unit/services/test_git_ownership_scope.py @@ -0,0 +1,271 @@ +"""Scoped post-op ownership repair. + +Every git op used to pay a recursive full-workspace chown after `_run_git` +regardless of whether it wrote anything — live NAS logs showed a plain +`rev-parse --verify` costing 5165ms of pure chown, and `fetch origin` +10534ms; `i_am_done` alone chains ~a dozen ops. `_git_ownership_scope` +classifies each invocation so a read-only op skips the repair entirely and a +`.git`-only-writing op (add/commit/fetch/push, the SET forms of +branch/symbolic-ref) repairs only `.git/` instead of walking the whole +working tree. +""" + +from __future__ import annotations + +import asyncio +import subprocess +from typing import TYPE_CHECKING +from unittest.mock import MagicMock + +import pytest +from roboco.services.git import GitService, _git_ownership_scope + +if TYPE_CHECKING: + from pathlib import Path + + +def _svc() -> GitService: + return GitService(MagicMock()) + + +# --------------------------------------------------------------------------- +# Pure classification — every verb shape actually used across git.py's call +# sites (86 sites, hand-enumerated). +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "args", + [ + ["status", "--porcelain"], + ["log", "-1", "--format=%H|%s"], + ["diff", "--stat", "HEAD~1..HEAD"], + ["diff", "--name-only", "--diff-filter=U"], + ["rev-parse", "--verify", "--quiet", "refs/heads/x"], + ["rev-list", "--left-right", "--count", "a...b"], + ["rev-list", "--count", "a..b"], + ["ls-remote", "--heads", "origin", "x"], + ["merge-base", "--is-ancestor", "a", "b"], + ["show", "HEAD:path/to/file"], + ["cherry", "origin/main", "child-ref"], + ], +) +def test_read_only_verbs_classify_none(args: list[str]) -> None: + assert _git_ownership_scope(args) == "none" + + +@pytest.mark.parametrize( + "args", + [ + ["add", "file.py"], + ["add", "-A"], + ["commit", "-m", "msg", "--author", "a "], + ["fetch", "origin", "branch"], + ["fetch", "origin"], + ["push", "-u", "origin", "branch"], + ["push", "--force-with-lease", "origin", "HEAD:branch"], + ], +) +def test_git_scoped_verbs_classify_git(args: list[str]) -> None: + assert _git_ownership_scope(args) == "git" + + +@pytest.mark.parametrize( + "args", + [ + ["checkout", "branch"], + ["checkout", "-b", "branch", "origin/branch"], + ["checkout", "-B", "branch"], + ["checkout", "--detach"], + ["reset", "--hard", "origin/branch"], + ["pull"], + ["pull", "--ff-only"], + ["rebase", "target"], + ["rebase", "--abort"], + ], +) +def test_full_scope_verbs_classify_full(args: list[str]) -> None: + assert _git_ownership_scope(args) == "full" + + +def test_branch_query_form_classifies_none() -> None: + # `git branch --show-current` — the only branch-query shape used. + assert _git_ownership_scope(["branch", "--show-current"]) == "none" + + +def test_branch_set_form_classifies_git() -> None: + # `git branch ` creates a local ref — writes .git/. + assert ( + _git_ownership_scope(["branch", "task-branch", "origin/task-branch"]) == "git" + ) + + +def test_symbolic_ref_query_form_classifies_none() -> None: + # `git symbolic-ref [-q] ` reads what points to. + assert ( + _git_ownership_scope(["symbolic-ref", "--quiet", "refs/remotes/origin/HEAD"]) + == "none" + ) + + +def test_symbolic_ref_set_form_classifies_git() -> None: + # `git symbolic-ref ` writes — 2 positional args. + assert _git_ownership_scope(["symbolic-ref", "HEAD", "refs/heads/main"]) == "git" + + +def test_empty_args_classifies_full() -> None: + """Safe default: nothing to classify never under-repairs.""" + assert _git_ownership_scope([]) == "full" + + +def test_unrecognized_verb_classifies_full() -> None: + """Safe default: an unclassified verb never under-repairs.""" + assert _git_ownership_scope(["worktree", "add", "x", "-b", "y", "z"]) == "full" + + +# --------------------------------------------------------------------------- +# `_run_git` wiring — the classifier must actually gate which repair runs. +# --------------------------------------------------------------------------- + + +def _ok(args: list[str]) -> subprocess.CompletedProcess[str]: + return subprocess.CompletedProcess( + args=["git", *args], returncode=0, stdout="", stderr="" + ) + + +@pytest.mark.asyncio +async def test_read_only_op_skips_chown_entirely( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A read-only op must never call either repair function.""" + (tmp_path / ".git").mkdir() + monkeypatch.setattr( + "roboco.services.git.subprocess.run", lambda *_a, **_k: _ok(["status"]) + ) + full_repair = MagicMock() + git_repair = MagicMock() + monkeypatch.setattr("roboco.services.workspace._ensure_agent_owned", full_repair) + monkeypatch.setattr("roboco.services.workspace._ensure_git_dir_owned", git_repair) + + await _svc()._run_git(tmp_path, ["status", "--porcelain"]) + + full_repair.assert_not_called() + git_repair.assert_not_called() + + +@pytest.mark.asyncio +async def test_git_scoped_op_calls_git_repair_not_full_repair( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """add/commit/fetch/push call the .git-only repair, never the full walk.""" + (tmp_path / ".git").mkdir() + monkeypatch.setattr( + "roboco.services.git.subprocess.run", lambda *_a, **_k: _ok(["commit"]) + ) + full_repair = MagicMock() + git_repair = MagicMock() + monkeypatch.setattr("roboco.services.workspace._ensure_agent_owned", full_repair) + monkeypatch.setattr("roboco.services.workspace._ensure_git_dir_owned", git_repair) + + await _svc()._run_git(tmp_path, ["commit", "-m", "msg"]) + + git_repair.assert_called_once_with(tmp_path) + full_repair.assert_not_called() + + +@pytest.mark.asyncio +async def test_full_scope_op_calls_full_repair_not_git_repair( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """checkout/reset/rebase/pull keep the unchanged full-workspace repair.""" + (tmp_path / ".git").mkdir() + monkeypatch.setattr( + "roboco.services.git.subprocess.run", lambda *_a, **_k: _ok(["checkout"]) + ) + full_repair = MagicMock() + git_repair = MagicMock() + monkeypatch.setattr("roboco.services.workspace._ensure_agent_owned", full_repair) + monkeypatch.setattr("roboco.services.workspace._ensure_git_dir_owned", git_repair) + + await _svc()._run_git(tmp_path, ["checkout", "some-branch"]) + + full_repair.assert_called_once_with(tmp_path) + git_repair.assert_not_called() + + +@pytest.mark.asyncio +async def test_reown_after_git_op_returns_zero_ms_when_skipped() -> None: + """The instrumentation must see a true near-zero cost for a skipped repair, + not a stale/garbage value.""" + loop = asyncio.get_running_loop() + ms = await GitService._reown_after_git_op(loop, MagicMock(), ["status"]) + assert ms == 0.0 + + +# --------------------------------------------------------------------------- +# `.git`-scoped repair actually targets clone_root/.git — for a plain clone +# AND for a per-task worktree path (the worktree-awareness requirement). +# --------------------------------------------------------------------------- + + +@pytest.fixture +def _record_touched(monkeypatch: pytest.MonkeyPatch) -> list[str]: + """Record every path the real ownership-repair primitives touch.""" + touched: list[str] = [] + + def _record(entry: str) -> int: + touched.append(entry) + return 0 + + monkeypatch.setattr("roboco.services.workspace._own_and_grant_rw", _record) + return touched + + +@pytest.mark.asyncio +async def test_git_scoped_repair_targets_clone_git_for_plain_clone( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, _record_touched: list[str] +) -> None: + """A plain clone (not a worktree): `.git` scoped repair touches clone/.git.""" + clone = tmp_path / "clone" + (clone / ".git" / "objects").mkdir(parents=True) + (clone / ".git" / "config").write_text("[core]\n") + monkeypatch.setattr( + "roboco.services.git.subprocess.run", lambda *_a, **_k: _ok(["push"]) + ) + + await _svc()._run_git(clone, ["push", "-u", "origin", "branch"]) + + touched = set(_record_touched) + assert str(clone / ".git") in touched + assert str(clone / ".git" / "config") in touched + + +@pytest.mark.asyncio +async def test_git_scoped_repair_targets_shared_clone_git_for_worktree( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, _record_touched: list[str] +) -> None: + """An op run inside a `.worktrees/` checkout repairs the SHARED + clone_root/.git (the real object store) — NOT the worktree's own `.git`, + which is just a small gitlink file, not a directory to walk.""" + clone_root = tmp_path / "clone" + (clone_root / ".git" / "worktrees" / "abc123").mkdir(parents=True) + (clone_root / ".git" / "refs" / "heads").mkdir(parents=True) + + worktree = clone_root / ".worktrees" / "abc123" + worktree.mkdir(parents=True) + (worktree / ".git").write_text( + f"gitdir: {clone_root / '.git' / 'worktrees' / 'abc123'}\n" + ) + + monkeypatch.setattr( + "roboco.services.git.subprocess.run", lambda *_a, **_k: _ok(["commit"]) + ) + + await _svc()._run_git(worktree, ["commit", "-m", "msg"]) + + touched = set(_record_touched) + assert str(clone_root / ".git") in touched + assert str(clone_root / ".git" / "worktrees" / "abc123") in touched + # Never touches the worktree's own gitlink file as a directory walk root. + assert str(worktree / ".git") not in touched diff --git a/tests/unit/services/test_workspace_ensure_agent_owned_scope.py b/tests/unit/services/test_workspace_ensure_agent_owned_scope.py index 9710ab11..ae625286 100644 --- a/tests/unit/services/test_workspace_ensure_agent_owned_scope.py +++ b/tests/unit/services/test_workspace_ensure_agent_owned_scope.py @@ -91,3 +91,35 @@ def test_noop_when_workspace_absent(tmp_path: Path, _record_touched: list[str]) missing = tmp_path / "never_cloned" _ensure_agent_owned(missing) assert _record_touched == [] + + +def test_chown_failure_falls_back_to_chmod_and_warns( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Rootless/userns hosts reject chown. `_own_and_grant_rw` must still run + the chmod fallback (belt-and-suspenders for ACL-inheriting NAS volumes) + and `_ensure_agent_owned` must warn rather than swallow the failure + silently. Unchanged by the scoped-repair split — this exercises the real + (non-git-scoped) chown/chmod primitives via `_chown_entry`, forced to + fail regardless of the test process's actual uid/gid.""" + (tmp_path / "file.py").write_text("x = 1\n") + + chmod_calls: list[str] = [] + monkeypatch.setattr(workspace_module, "_chown_entry", lambda _entry: False) + monkeypatch.setattr( + workspace_module, "_make_owner_and_group_rw", chmod_calls.append + ) + warning_calls: list[tuple[str, dict[str, object]]] = [] + monkeypatch.setattr( + workspace_module.logger, + "warning", + lambda msg, **kw: warning_calls.append((msg, kw)), + ) + + _ensure_agent_owned(tmp_path) + + # chmod fallback still ran for every entry despite the chown failure. + assert str(tmp_path / "file.py") in chmod_calls + # The failure is surfaced, not swallowed. + assert warning_calls + assert warning_calls[0][1]["failures"]