diff --git a/python/robomp/src/host_tools.py b/python/robomp/src/host_tools.py index 70abbda22..aeee25c76 100644 --- a/python/robomp/src/host_tools.py +++ b/python/robomp/src/host_tools.py @@ -1093,20 +1093,6 @@ def _build_classify_issue(bindings: ToolBindings) -> HostTool[Any, Any]: labels.append(platform) labels.append("triaged") - try: - applied = _run_coro( - bindings.loop, - bindings.github.add_issue_labels( - bindings.repo.full_name, - bindings.issue.number, - labels, - ), - ) - except GitHubError as exc: - _audit(bindings, "classify_issue", args, error=str(exc)) - _raise_command(f"GitHub rejected labels: {exc.status} {exc.message}") - - bindings.db.set_issue_classification(bindings.issue_key, primary) renamed_to: str | None = None if branch_slug: try: @@ -1128,6 +1114,21 @@ def _build_classify_issue(bindings: ToolBindings) -> HostTool[Any, Any]: # refactor of that helper still surfaces the mismatch. _raise_command("classify_issue internal: branch rename inconsistent.") bindings.db.set_issue_branch(bindings.issue_key, renamed_to) + + try: + applied = _run_coro( + bindings.loop, + bindings.github.add_issue_labels( + bindings.repo.full_name, + bindings.issue.number, + labels, + ), + ) + except GitHubError as exc: + _audit(bindings, "classify_issue", args, error=str(exc)) + _raise_command(f"GitHub rejected labels: {exc.status} {exc.message}") + + bindings.db.set_issue_classification(bindings.issue_key, primary) _audit( bindings, "classify_issue", diff --git a/python/robomp/src/sandbox.py b/python/robomp/src/sandbox.py index 904ea0ad9..15a671b0e 100644 --- a/python/robomp/src/sandbox.py +++ b/python/robomp/src/sandbox.py @@ -14,15 +14,14 @@ Permission model There are four ownership zones on disk; do not let them blur: 1. **Workspace tree** (`/data/workspaces//`, including `repo/`, - `.omp-session/`, `context/`, `artifacts/`, `.omp-tmp/`, `.omp-xdg/`): - single-owner. Owned by the active slot UID/GID (`omp-N`) with mode - `u=rwX,g=rwX,o=` (effectively `0770` dirs / `0660` files; the group is - the slot's own private gid so group bits are functionally identical to - owner-only). The orchestrator (root) reads/writes via uid-0 bypass when - it must, and drops to the slot for any subprocess that touches paths the - agent will revisit. `ensure_workspace` + `_chown_workspace` are the - single point of truth for this zone — no other helper sets ownership - inside `ws_root`. + `.omp-session/`, `context/`, `artifacts/`, `.omp-tmp/`, `.omp-xdg`): + single-owner. Owned by the active slot UID/GID (`omp-N`) when slot + isolation is enabled, otherwise by the orchestrator's own UID/GID. Modes + stay `u=rwX,g=rwX,o=` (effectively `0770` dirs / `0660` files). The + orchestrator (root) reads/writes via uid-0 bypass when it must, and drops + to the slot for any subprocess that touches paths the agent will revisit. + `ensure_workspace` + `_chown_workspace` are the single point of truth for + this zone — no other helper sets ownership inside `ws_root`. 2. **Clone pool** (`/data/workspaces/_pool/__/`): genuinely multi-slot. Owned by `root:omp` (gid 2000) with setgid `02770`; cross-slot writes are bridged by `_share_git_metadata_with_slots`. @@ -194,6 +193,7 @@ def rename_workspace_branch( proc = _safe_run( ["git", "branch", "-m", workspace.branch, new_branch], cwd=workspace.repo_dir, + env=_git_env_for_repo(workspace.repo_dir), **_slot_subprocess_kwargs(slot_uid), ) if proc.returncode != 0: @@ -572,30 +572,31 @@ def _share_git_metadata_with_slots(repo_dir: Path, slot_uid: int | None) -> None def _chown_workspace(ws_root: Path, slot_uid: int | None) -> None: - """Hand the entire workspace tree to the active slot UID/GID. + """Hand the workspace tree to the identity that will run repo-local git. + + With slot isolation enabled, that identity is ``slot_uid:slot_uid``. + Without slots, the agent and host-side repo commands run as the + orchestrator user itself. Existing workspaces may still be owned by an + old slot UID from a prior deploy; normalizing them back to the current + euid/egid keeps Git's ownership check satisfied without persistent + ``safe.directory`` config. Single-ownership invariant: every file under ``ws_root`` ends up owned by - ``slot_uid:slot_uid`` with mode ``u=rwX,g=rwX,o=`` (``0770`` dirs / ``0660`` - files). The slot's GID is its own private gid (created by entrypoint.sh), - so the group bits are functionally identical to owner-only — they exist - for parity with the existing pattern and to make accidental future - ``setgid`` use safe. + the active runner with mode ``u=rwX,g=rwX,o=`` (``0770`` dirs / ``0660`` + files). The orchestrator (root) keeps read/write access via uid-0 bypass; any - subprocess that touches paths the agent will revisit MUST drop to the slot - via ``_slot_subprocess_kwargs`` so tools like bun/biome/cargo (which - chmod/utime their own cache state) never encounter a non-owner file. - - Self-healing on re-entry: an existing workspace left over from the old - ``root:slot`` model gets re-chown'd on the next ``ensure_workspace`` call. + subprocess that touches paths the agent will revisit MUST either run as + the same owner or call this helper before invoking Git/tools that enforce + owner-sensitive state. """ - if slot_uid is None: - return if platform.system() != "Linux": return if os.geteuid() != 0: return - subprocess.run(["chown", "-R", f"{slot_uid}:{slot_uid}", str(ws_root)], check=True) + uid = slot_uid if slot_uid is not None else os.geteuid() + gid = slot_uid if slot_uid is not None else os.getegid() + subprocess.run(["chown", "-R", f"{uid}:{gid}", str(ws_root)], check=True) subprocess.run(["chmod", "-R", "u=rwX,g=rwX,o=", str(ws_root)], check=True) diff --git a/python/robomp/tests/test_host_tools.py b/python/robomp/tests/test_host_tools.py index 433ee37b9..37fa4225b 100644 --- a/python/robomp/tests/test_host_tools.py +++ b/python/robomp/tests/test_host_tools.py @@ -849,6 +849,43 @@ def test_classify_issue_rejects_invalid_branch_slug(db: Database, tmp_path: Path assert bindings.workspace.branch == "farm/abc12345/some-issue" +def test_classify_issue_rename_failure_does_not_apply_labels_or_classification( + db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + requests: list[str] = [] + + def handler(request: httpx.Request) -> httpx.Response: + requests.append(str(request.url)) + return httpx.Response(200, json=[]) + + def fail_rename(*_args: object, **_kwargs: object) -> str: + raise host_tools.GitCommandError(["git", "branch", "-m"], 128, "", "fatal: detected dubious ownership") + + bindings, loop, t = _bindings(db, tmp_path, httpx.MockTransport(handler)) + monkeypatch.setattr(host_tools, "rename_workspace_branch", fail_rename) + try: + tool = next(x for x in build(bindings) if x.name == "classify_issue") + with pytest.raises(RpcCommandError): + tool.execute( + { + "primary": "bug", + "priority": "prio:p1", + "rationale": "x", + "branch_slug": "fix-orphan-tool-output", + }, + _ctx(), + ) + finally: + _stop_loop(loop, t) + + assert requests == [] + row = db.get_issue(bindings.issue_key) + assert row is not None + assert row.classification is None + assert row.branch == "farm/abc12345/some-issue" + assert bindings.workspace.branch == "farm/abc12345/some-issue" + + def test_classify_issue_omitting_branch_slug_is_a_noop(db: Database, tmp_path: Path) -> None: """Existing callers that don't pass branch_slug must keep the original branch.""" bindings, loop, t = _bindings( diff --git a/python/robomp/tests/test_sandbox.py b/python/robomp/tests/test_sandbox.py index 1d5641618..d80f6beea 100644 --- a/python/robomp/tests/test_sandbox.py +++ b/python/robomp/tests/test_sandbox.py @@ -616,6 +616,28 @@ def test_slot_subprocess_kwargs_run_as_slot_on_linux_root(monkeypatch: pytest.Mo } +def test_chown_workspace_normalizes_to_root_when_slots_disabled( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + calls: list[tuple[list[str], bool | None]] = [] + + def fake_run(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[str]: + calls.append((cmd, kwargs.get("check") if isinstance(kwargs.get("check"), bool) else None)) + return subprocess.CompletedProcess(cmd, 0, "", "") + + monkeypatch.setattr("robomp.sandbox.platform.system", lambda: "Linux") + monkeypatch.setattr("robomp.sandbox.os.geteuid", lambda: 0) + monkeypatch.setattr("robomp.sandbox.os.getegid", lambda: 0) + monkeypatch.setattr("robomp.sandbox.subprocess.run", fake_run) + + _chown_workspace(tmp_path, None) + + assert calls == [ + (["chown", "-R", "0:0", str(tmp_path)], True), + (["chmod", "-R", "u=rwX,g=rwX,o=", str(tmp_path)], True), + ] + + def test_prepare_slot_runtime_env_returns_workspace_private_paths_without_chown( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: