mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(workspace): scope _ensure_agent_owned walk to .git subtree only
Walking the entire workspace (incl node_modules) to chown+chmod every entry cost 2.7-15.5s per git op. The agent only needs write ownership on .git/ during git ops; working-tree files don't need chowning. Restrict the walk to .git, and no-op when .git is absent.
This commit is contained in:
@@ -86,31 +86,35 @@ def _make_owner_and_group_rw(entry: str) -> None:
|
||||
|
||||
|
||||
def _ensure_agent_owned(workspace: Path) -> None:
|
||||
"""Recursively chown + group-write a workspace for the agent user.
|
||||
"""Chown + group-write the .git subtree for the agent user.
|
||||
|
||||
Orchestrator runs as root so anything it clones or writes is root-owned.
|
||||
Agent containers run as uid 1000 and must be able to create
|
||||
.git/index.lock, refs, packed-refs, and new source files — otherwise
|
||||
every git operation (and even plain file writes) fails with
|
||||
"Permission denied". Called after clone and on every ensure_workspace
|
||||
so legacy (pre-fix) workspaces get repaired.
|
||||
.git/index.lock, refs, packed-refs, and objects — otherwise every git
|
||||
operation fails with "Permission denied". Called after clone and on every
|
||||
ensure_workspace so legacy (pre-fix) workspaces get repaired.
|
||||
|
||||
Two defenses (both cheap, both idempotent):
|
||||
1. chown every entry to (AGENT_UID, AGENT_GID). On setups where user
|
||||
namespaces silently remap or reject the chown (some NAS / rootless
|
||||
docker configs), we log the failure instead of swallowing it — so
|
||||
when writes still fail from the agent, we can actually see why.
|
||||
2. chmod g+w on every file/dir. If chown doesn't take effect, having
|
||||
the group writable (and with AGENT_GID) is enough for uid 1000 to
|
||||
write, provided agent is in that group. Belt + suspenders.
|
||||
The walk is scoped to ``.git`` only. Working-tree files (and especially a
|
||||
multi-thousand-entry ``node_modules/``) don't need chowning for git to
|
||||
work, and walking them cost 2.7-15.5s per git op. If ``.git`` is absent
|
||||
(never cloned), there is nothing to own and we no-op.
|
||||
|
||||
The previous fast-path (skip walk if top-level stat already matches)
|
||||
was unsafe: a root-owned file deep under an agent-owned top dir would
|
||||
not get repaired, which is exactly how README.md ends up root:root
|
||||
even after ensure_workspace runs.
|
||||
Two defenses (both cheap, both idempotent), applied to every entry under
|
||||
``.git``:
|
||||
1. chown to (AGENT_UID, AGENT_GID). On setups where user namespaces
|
||||
silently remap or reject the chown (some NAS / rootless docker
|
||||
configs), we log the failure instead of swallowing it — so when writes
|
||||
still fail from the agent, we can actually see why.
|
||||
2. chmod g+w. If chown doesn't take effect, having the group writable
|
||||
(and with AGENT_GID) is enough for uid 1000 to write, provided agent
|
||||
is in that group. Belt + suspenders.
|
||||
"""
|
||||
git_dir = workspace / ".git"
|
||||
if not git_dir.exists():
|
||||
return
|
||||
|
||||
failed_chowns = 0
|
||||
for root, dirs, files in os.walk(workspace):
|
||||
for root, dirs, files in os.walk(git_dir):
|
||||
entries = (
|
||||
root,
|
||||
*[str(Path(root) / d) for d in dirs],
|
||||
|
||||
@@ -0,0 +1,95 @@
|
||||
"""Tests that _ensure_agent_owned only touches the .git subtree.
|
||||
|
||||
The agent only needs write ownership on .git/ (index.lock, refs, packed-refs,
|
||||
objects) during git ops. Walking the entire working tree — including a large
|
||||
node_modules/ — chown+chmod'ing every entry made every git op take seconds.
|
||||
The walk must be scoped to .git only.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
from roboco.services import workspace as workspace_module
|
||||
from roboco.services.workspace import _ensure_agent_owned
|
||||
|
||||
|
||||
def _build_workspace(root: Path) -> None:
|
||||
"""Create a workspace with a .git dir and a large node_modules tree."""
|
||||
git_dir = root / ".git"
|
||||
(git_dir / "refs" / "heads").mkdir(parents=True)
|
||||
(git_dir / "objects").mkdir(parents=True)
|
||||
(git_dir / "config").write_text("[core]\n")
|
||||
(git_dir / "HEAD").write_text("ref: refs/heads/main\n")
|
||||
(git_dir / "refs" / "heads" / "main").write_text("abc123\n")
|
||||
(git_dir / "packed-refs").write_text("# pack-refs\n")
|
||||
|
||||
# A large working tree with a deep node_modules/ that must NOT be walked.
|
||||
src = root / "src"
|
||||
src.mkdir(parents=True)
|
||||
(src / "main.py").write_text("print('hi')\n")
|
||||
node_modules = root / "node_modules"
|
||||
for pkg in range(20):
|
||||
pkg_dir = node_modules / f"pkg-{pkg}" / "dist"
|
||||
pkg_dir.mkdir(parents=True)
|
||||
(pkg_dir / "index.js").write_text("module.exports = {}\n")
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def _record_touched(monkeypatch: pytest.MonkeyPatch) -> list[str]:
|
||||
"""Record every path _ensure_agent_owned tries to chown/chmod."""
|
||||
touched: list[str] = []
|
||||
|
||||
def fake_chown_entry(entry: str) -> bool:
|
||||
touched.append(entry)
|
||||
return True
|
||||
|
||||
def fake_make_rw(entry: str) -> None:
|
||||
touched.append(entry)
|
||||
|
||||
monkeypatch.setattr(workspace_module, "_chown_entry", fake_chown_entry)
|
||||
monkeypatch.setattr(workspace_module, "_make_owner_and_group_rw", fake_make_rw)
|
||||
return touched
|
||||
|
||||
|
||||
def test_ensure_agent_owned_scopes_to_git_only(
|
||||
tmp_path: Path, _record_touched: list[str]
|
||||
) -> None:
|
||||
_build_workspace(tmp_path)
|
||||
|
||||
_ensure_agent_owned(tmp_path)
|
||||
|
||||
git_dir = tmp_path / ".git"
|
||||
assert _record_touched, "expected .git entries to be touched"
|
||||
|
||||
# Every touched path must live inside .git/.
|
||||
for entry in _record_touched:
|
||||
resolved = Path(entry).resolve()
|
||||
assert git_dir.resolve() in (resolved, *resolved.parents), (
|
||||
f"{entry} is outside the .git subtree"
|
||||
)
|
||||
|
||||
# No node_modules path may be touched.
|
||||
assert not any("node_modules" in entry for entry in _record_touched)
|
||||
|
||||
# The git internals that need agent ownership were in fact visited.
|
||||
expected = {
|
||||
str(git_dir / "config"),
|
||||
str(git_dir / "HEAD"),
|
||||
str(git_dir / "packed-refs"),
|
||||
str(git_dir / "refs" / "heads" / "main"),
|
||||
}
|
||||
assert expected.issubset(set(_record_touched))
|
||||
|
||||
|
||||
def test_ensure_agent_owned_noop_when_git_absent(
|
||||
tmp_path: Path, _record_touched: list[str]
|
||||
) -> None:
|
||||
# Working tree with no .git/ — nothing to own.
|
||||
(tmp_path / "node_modules" / "pkg").mkdir(parents=True)
|
||||
(tmp_path / "node_modules" / "pkg" / "index.js").write_text("x\n")
|
||||
|
||||
_ensure_agent_owned(tmp_path)
|
||||
|
||||
assert _record_touched == []
|
||||
Reference in New Issue
Block a user