fix(python/robomp): corrected issue classification updates on rename-fail

- Moved issue classification updates to run only after branch rename succeeds, preventing partial labeling or DB writes when rename fails.
- Adjusted workspace ownership normalization to chown workspaces to the active slot or, when slotless, to the current euid/egid, then apply shared permissions.
- Added tests for classify_issue rename-failure rollback and chown_workspace normalization in non-slot mode.

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",

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/<key>/`, 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/<owner>__<repo>/`): 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)
 
 

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(

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: