From 1ed1317a3561fbbaaa345099fce1b7b7728d2195 Mon Sep 17 00:00:00 2001 From: Renn F Date: Sun, 3 May 2026 09:06:40 +0200 Subject: [PATCH] fix(workspace): re-apply agent ownership after refresh fetch I1: _fetch_origin_best_effort runs as root and writes new pack files + ref updates that land root-owned, undoing _ensure_agent_owned that ran before. Subsequent spawns hit Permission denied. Mirror the fetch_branch_for_inspection pattern: re-run _ensure_agent_owned AFTER the fetch. I2: separate workspace_refresh_fetch_timeout_seconds (default 60s) from workspace_clone_timeout (300s). Refresh transfers small deltas; 300s of blocking on every spawn against a hung remote is operationally bad. 60s is enough for any sane refresh. --- roboco/config.py | 11 +++ roboco/services/workspace.py | 14 +++- tests/unit/services/test_workspace_refresh.py | 75 ++++++++++++++++++- 3 files changed, 95 insertions(+), 5 deletions(-) diff --git a/roboco/config.py b/roboco/config.py index fc587fc4..aac77532 100644 --- a/roboco/config.py +++ b/roboco/config.py @@ -238,6 +238,17 @@ class Settings(BaseSettings): ge=30, description="Timeout in seconds for git clone operations", ) + workspace_refresh_fetch_timeout_seconds: int = Field( + default=60, + ge=5, + description=( + "Timeout in seconds for the best-effort `git fetch origin` " + "that runs on every healthy-clone re-entry into " + "ensure_workspace. Refresh fetches transfer small deltas only " + "— blocking 300s (the full-clone timeout) on every spawn " + "against a hung remote is operationally bad." + ), + ) # ========================================================================== # Agent Guardrails (per-session budgets, loop detection, SLAs) diff --git a/roboco/services/workspace.py b/roboco/services/workspace.py index 1e67c94b..7425cc0c 100644 --- a/roboco/services/workspace.py +++ b/roboco/services/workspace.py @@ -321,6 +321,11 @@ class WorkspaceService: stripped remote URL. Public repos and refresh-only fetches succeed without auth; auth-protected refreshes will surface their stderr in the warning log without aborting workspace setup. + + Timeout uses `workspace_refresh_fetch_timeout_seconds` (default + 60s), NOT `workspace_clone_timeout` (300s) — a refresh transfers + small deltas, so 300s of blocking on every spawn against a hung + remote is operationally bad. """ def _do_fetch() -> subprocess.CompletedProcess[str]: @@ -329,7 +334,7 @@ class WorkspaceService: cwd=str(workspace), capture_output=True, text=True, - timeout=settings.workspace_clone_timeout, + timeout=settings.workspace_refresh_fetch_timeout_seconds, check=False, ) @@ -432,6 +437,13 @@ class WorkspaceService: # network blips and offline mode must not break workspace # setup; checkout is unchanged. await self._fetch_origin_best_effort(workspace, project_slug) + # Re-chown so the agent user can still write into .git + # after our root-side fetch updated refs/objects. Mirrors + # the pattern in `fetch_branch_for_inspection` — without + # this, new pack files under .git/objects/pack/ and ref + # updates under .git/refs/remotes/origin/ land root-owned + # and undo the chown we just ran above. + await asyncio.to_thread(_ensure_agent_owned, workspace) logger.debug( "Workspace already exists", workspace=str(workspace), diff --git a/tests/unit/services/test_workspace_refresh.py b/tests/unit/services/test_workspace_refresh.py index 828ae048..6da0c493 100644 --- a/tests/unit/services/test_workspace_refresh.py +++ b/tests/unit/services/test_workspace_refresh.py @@ -25,6 +25,11 @@ if TYPE_CHECKING: # `origin`). Named to satisfy ruff PLR2004 — magic-value comparison. _MIN_GIT_FETCH_ARGC = 3 +# Healthy short-circuit chowns BEFORE fetch (repair pre-existing root +# ownership) AND AFTER fetch (repair root-owned pack/refs the fetch +# just wrote). Named to satisfy ruff PLR2004. +_MIN_CHOWN_CALLS_AROUND_FETCH = 2 + def _service() -> WorkspaceService: """Build a WorkspaceService over a MagicMock session.""" @@ -101,10 +106,72 @@ async def test_ensure_workspace_fetches_origin_on_healthy_short_circuit( f"Expected `git fetch origin` on healthy short-circuit, " f"got subprocess calls: {captured}" ) - # Specifically: `git fetch origin` (no extra positional refspec — fetch - # all branches' refs so PM/Doc sees every dev branch). - assert any(a[-2:] == ["fetch", "origin"] for a in fetch_calls), ( - f"Expected exact `git fetch origin`, got: {fetch_calls}" + # Specifically: `git fetch origin` with NO `-c` flag and no extra + # positional refspec. The `-c` check protects the docstring's + # no-token-injection invariant — a future refactor that added + # `git -c http.extraheader=...` would still satisfy a loose + # `a[-2:] == ["fetch", "origin"]` assertion, silently regressing + # the no-PAT-injection guarantee. + assert any( + a[0] == "git" and "-c" not in a and a[-2:] == ["fetch", "origin"] + for a in fetch_calls + ), f"Expected exact `git fetch origin` (no `-c`), got: {fetch_calls}" + + +@pytest.mark.asyncio +async def test_ensure_workspace_rechowns_after_refresh_fetch( + healthy_workspace: Path, +) -> None: + """Healthy-clone re-entry MUST chown again AFTER `git fetch origin`. + + The orchestrator runs as root, so `git fetch` writes new pack files + under `.git/objects/pack/` and updates refs under + `.git/refs/remotes/origin/` — those land root-owned, undoing the + pre-fetch chown. Without a post-fetch chown, the next agent-side + write (.git/index.lock, packed-refs, etc.) hits Permission denied. + + This mirrors the pattern in `fetch_branch_for_inspection`. + """ + svc = _service() + agent = _fake_agent() + _bind(svc, "_lookup_agent_or_raise", AsyncMock(return_value=agent)) + _bind(svc, "get_workspace_path", MagicMock(return_value=healthy_workspace)) + + call_log: list[str] = [] + + def _fake_run( + args: list[str], **_kwargs: object + ) -> subprocess.CompletedProcess[str]: + if "fetch" in args: + call_log.append("fetch") + return subprocess.CompletedProcess( + args=args, returncode=0, stdout="", stderr="" + ) + + def _fake_chown(_workspace: object) -> None: + call_log.append("chown") + + with ( + patch("roboco.services.workspace.subprocess.run", side_effect=_fake_run), + patch( + "roboco.services.workspace._ensure_agent_owned", + side_effect=_fake_chown, + ), + ): + await svc.ensure_workspace( + project_slug="roboco", + agent_id=agent.id, + ) + + # Expect at least: chown (pre-fetch) -> fetch -> chown (post-fetch). + # The post-fetch chown is the load-bearing one — it repairs ownership + # of objects/refs the root-side fetch just wrote. + assert call_log.count("chown") >= _MIN_CHOWN_CALLS_AROUND_FETCH, ( + f"Expected at least two chown calls (pre + post fetch), got: {call_log}" + ) + fetch_idx = call_log.index("fetch") + assert "chown" in call_log[fetch_idx + 1 :], ( + f"Expected a chown AFTER the fetch, got: {call_log}" )