Merge pull request #46552 from kshitijk4poor/salvage/file-tools-session-cwd

fix(tools): respect session cwd in file tools (salvage of #46460)

tests/tools/test_file_tools_container_config.py

@@ -27,7 +27,7 @@ def _make_env_config(**overrides):
 
 
 class TestFileToolsContainerConfig:
-    def _run(self, env_config, task_id):
+    def _run(self, env_config, task_id, task_env_overrides=None):
         captured = {}
         mock_env = MagicMock()
 
@@ -35,31 +35,51 @@ class TestFileToolsContainerConfig:
             captured.update(kwargs)
             return mock_env
 
-        with patch("tools.terminal_tool._get_env_config", return_value=env_config),              patch("tools.terminal_tool._task_env_overrides", {}),              patch("tools.terminal_tool._active_environments", {}),              patch("tools.terminal_tool._creation_locks", {}),              patch("tools.terminal_tool._creation_locks_lock", __import__("threading").Lock()),              patch("tools.terminal_tool._create_environment", side_effect=fake_create_env),              patch("tools.terminal_tool._start_cleanup_thread"),              patch("tools.terminal_tool._check_disk_usage_warning"),              patch("tools.file_tools._file_ops_cache", {}),              patch("tools.file_tools._file_ops_lock", __import__("threading").Lock()):
+        with patch("tools.terminal_tool._get_env_config", return_value=env_config), \
+             patch("tools.terminal_tool._task_env_overrides", task_env_overrides or {}), \
+             patch("tools.terminal_tool._active_environments", {}), \
+             patch("tools.terminal_tool._creation_locks", {}), \
+             patch("tools.terminal_tool._creation_locks_lock", __import__("threading").Lock()), \
+             patch("tools.terminal_tool._create_environment", side_effect=fake_create_env), \
+             patch("tools.terminal_tool._start_cleanup_thread"), \
+             patch("tools.terminal_tool._check_disk_usage_warning"), \
+             patch("tools.file_tools._file_ops_cache", {}), \
+             patch("tools.file_tools._file_ops_lock", __import__("threading").Lock()):
             file_tools._get_file_ops(task_id)
 
-        return captured.get("container_config", {})
+        return captured
 
     def test_docker_mount_cwd_to_workspace_passed(self):
         """docker_mount_cwd_to_workspace is forwarded to container_config."""
-        cc = self._run(_make_env_config(docker_mount_cwd_to_workspace=True), "t1")
+        cc = self._run(_make_env_config(docker_mount_cwd_to_workspace=True), "t1").get("container_config", {})
         assert cc.get("docker_mount_cwd_to_workspace") is True
 
     def test_docker_forward_env_passed(self):
         """docker_forward_env is forwarded to container_config."""
-        cc = self._run(_make_env_config(docker_forward_env=["MY_SECRET"]), "t2")
+        cc = self._run(_make_env_config(docker_forward_env=["MY_SECRET"]), "t2").get("container_config", {})
         assert cc.get("docker_forward_env") == ["MY_SECRET"]
 
     def test_docker_mount_cwd_defaults_to_false(self):
         """docker_mount_cwd_to_workspace defaults to False when absent from config."""
         cfg = _make_env_config()
         del cfg["docker_mount_cwd_to_workspace"]
-        cc = self._run(cfg, "t3")
+        cc = self._run(cfg, "t3").get("container_config", {})
         assert cc.get("docker_mount_cwd_to_workspace") is False
 
     def test_docker_forward_env_defaults_to_empty_list(self):
         """docker_forward_env defaults to [] when absent from config."""
         cfg = _make_env_config()
         del cfg["docker_forward_env"]
-        cc = self._run(cfg, "t4")
+        cc = self._run(cfg, "t4").get("container_config", {})
         assert cc.get("docker_forward_env") == []
+
+    def test_cwd_only_raw_task_override_reaches_file_environment(self):
+        """CWD-only task overrides collapse to default but must keep their cwd."""
+        captured = self._run(
+            _make_env_config(env_type="local", cwd="/config-cwd"),
+            "desktop-session-cwd",
+            task_env_overrides={"desktop-session-cwd": {"cwd": "/workspace/session"}},
+        )
+
+        assert captured["task_id"] == "default"
+        assert captured["cwd"] == "/workspace/session"

tests/tools/test_file_tools_cwd_resolution.py

@@ -21,6 +21,7 @@ from pathlib import Path
 import pytest
 
 import tools.file_tools as ft
+import tools.terminal_tool as terminal_tool
 
 
 @pytest.fixture
@@ -218,6 +219,28 @@ def test_absolute_terminal_cwd_anchors_with_empty_registry(_isolated_cwd, monkey
     assert not str(resolved).startswith(str(decoy))
 
 
+def test_registered_task_cwd_override_anchors_before_terminal_env_exists(_isolated_cwd, monkeypatch):
+    """TUI/Desktop sessions register cwd by raw session key before tools run.
+
+    CWD-only overrides collapse to the shared terminal environment key, but the
+    file resolver must still read the raw task/session override before falling
+    back to TERMINAL_CWD or the process cwd.
+    """
+    workspace, decoy = _isolated_cwd
+    task_id = "desktop-session-cwd"
+    monkeypatch.setattr(ft, "_get_live_tracking_cwd", lambda task_id="default": None)
+    monkeypatch.delenv("TERMINAL_CWD", raising=False)
+    monkeypatch.setattr(terminal_tool, "_task_env_overrides", {})
+
+    terminal_tool.register_task_env_overrides(task_id, {"cwd": str(workspace)})
+
+    resolved = ft._resolve_path_for_task("target.py", task_id=task_id)
+
+    assert terminal_tool._resolve_container_task_id(task_id) == "default"
+    assert resolved == (workspace / "target.py")
+    assert not str(resolved).startswith(str(decoy))
+
+
 def test_warning_fires_from_terminal_cwd_when_registry_empty(_isolated_cwd, monkeypatch):
     """Divergence warning must fire even before any terminal command runs.
 
@@ -291,4 +314,3 @@ def test_patch_reports_resolved_absolute_path(_isolated_cwd, monkeypatch):
     assert "WORKSPACE_PATCHED" in (workspace / "target.py").read_text()
     # And the decoy copy is untouched.
     assert (decoy / "target.py").read_text() == "DECOY_ORIGINAL\n"
-

tools/file_tools.py

@@ -96,6 +96,23 @@ def _resolve_path(filepath: str, task_id: str = "default") -> Path:
 _TERMINAL_CWD_SENTINELS = frozenset({"", ".", "./", "auto", "cwd"})
 
 
+def _sentinel_free_abs_cwd(raw: str | None) -> str | None:
+    """Normalize a cwd candidate to an absolute, sentinel-free anchor.
+
+    Returns the expanded path only when *raw* is non-empty, not a sentinel (see
+    ``_TERMINAL_CWD_SENTINELS``), and absolute. A relative anchor is meaningless
+    without knowing which cwd it is relative to — exactly the ambiguity that
+    misroutes worktree edits — so relative/sentinel/empty values yield ``None``.
+    """
+    raw = str(raw or "").strip()
+    if raw.lower() in _TERMINAL_CWD_SENTINELS:
+        return None
+    expanded = os.path.expanduser(raw)
+    if not os.path.isabs(expanded):
+        return None
+    return expanded
+
+
 def _configured_terminal_cwd() -> str | None:
     """Return ``$TERMINAL_CWD`` only when it names a real directory anchor.
 
@@ -104,13 +121,26 @@ def _configured_terminal_cwd() -> str | None:
     relative to, which is exactly the ambiguity that misroutes worktree edits.
     Only an absolute, sentinel-free value is honored.
     """
-    raw = (os.environ.get("TERMINAL_CWD") or "").strip()
-    if raw.lower() in _TERMINAL_CWD_SENTINELS:
+    return _sentinel_free_abs_cwd(os.environ.get("TERMINAL_CWD"))
+
+
+def _registered_task_cwd_override(task_id: str = "default") -> str | None:
+    """Return a registered cwd override for the raw task id, when available.
+
+    ``terminal_tool`` intentionally collapses CWD-only task overrides to the
+    shared ``"default"`` environment so TUI/dashboard/ACP sessions do not spin
+    up isolated sandboxes just because they have different workspaces. The cwd
+    value itself is still keyed by the raw session/task id, so file tools must
+    read that raw override before falling back to the collapsed container key.
+    """
+    try:
+        from tools.terminal_tool import resolve_task_overrides
+
+        overrides = resolve_task_overrides(task_id)
+    except Exception:
         return None
-    expanded = os.path.expanduser(raw)
-    if not os.path.isabs(expanded):
-        return None
-    return expanded
+
+    return _sentinel_free_abs_cwd(overrides.get("cwd"))
 
 
 def _get_live_tracking_cwd(task_id: str = "default") -> str | None:
@@ -149,8 +179,10 @@ def _authoritative_workspace_root(task_id: str = "default") -> str | None:
 
     Prefers the live terminal cwd (the directory the agent is actually working
     in). When no terminal command has run yet — so the live registry is empty —
-    falls back to a sentinel-free absolute ``$TERMINAL_CWD``. This is what lets
-    a worktree session warn about (and resolve into) the worktree from the very
+    falls back to a registered task/session cwd override (TUI/Desktop/ACP
+    sessions register a raw-keyed cwd before any tool runs), then to a
+    sentinel-free absolute ``$TERMINAL_CWD``. This is what lets a worktree or
+    Desktop session warn about (and resolve into) its workspace from the very
     first ``write_file``/``patch``, before any ``cd`` has populated the live cwd.
 
     Returns ``None`` only when there is genuinely no reliable anchor, in which
@@ -159,6 +191,9 @@ def _authoritative_workspace_root(task_id: str = "default") -> str | None:
     live = _get_live_tracking_cwd(task_id)
     if live:
         return live
+    registered = _registered_task_cwd_override(task_id)
+    if registered:
+        return registered
     return _configured_terminal_cwd()
 
 
@@ -168,10 +203,12 @@ def _resolve_base_dir(task_id: str = "default") -> Path:
     Resolution order:
       1. The task's live terminal cwd (the directory the agent is actually
          working in — e.g. a git worktree). Authoritative when known.
-      2. A sentinel-free, absolute ``$TERMINAL_CWD`` (the worktree path set by
+      2. A registered task/session cwd override (TUI/Desktop/ACP sessions
+         register a raw-keyed workspace cwd before any terminal command runs).
+      3. A sentinel-free, absolute ``$TERMINAL_CWD`` (the worktree path set by
          ``cli.py``/``main.py`` for ``-w`` sessions). Used even before any
          terminal command has populated the live cwd registry.
-      3. The process cwd.
+      4. The process cwd.
 
     The returned base is ALWAYS absolute. This is the core invariant that
     prevents the worktree-cwd divergence bug: a relative or sentinel
@@ -218,9 +255,10 @@ def _path_resolution_warning(filepath: str, resolved: Path, task_id: str = "defa
     target. ``None`` when the path is absolute, the base is unknown, or the
     resolved path is correctly under the workspace root.
 
-    The workspace root is the live terminal cwd when known, else a sentinel-free
-    absolute ``$TERMINAL_CWD`` — so a worktree session whose terminal registry
-    is still empty (no ``cd`` run yet) is warned on the very first write.
+    The workspace root is the live terminal cwd when known, else a registered
+    task/session cwd override, else a sentinel-free absolute ``$TERMINAL_CWD``
+    — so a worktree or Desktop session whose terminal registry is still empty
+    (no ``cd`` run yet) is warned on the very first write.
     """
     try:
         if Path(filepath).expanduser().is_absolute():
@@ -625,7 +663,8 @@ def _get_file_ops(task_id: str = "default") -> ShellFileOperations:
     )
     import time
 
-    task_id = _resolve_container_task_id(task_id)
+    raw_task_id = task_id or "default"
+    task_id = _resolve_container_task_id(raw_task_id)
 
     # Fast path: check cache -- but also verify the underlying environment
     # is still alive (it may have been killed by the cleanup thread).
@@ -658,11 +697,11 @@ def _get_file_ops(task_id: str = "default") -> ShellFileOperations:
                 terminal_env = None
 
         if terminal_env is None:
-            from tools.terminal_tool import _task_env_overrides
+            from tools.terminal_tool import resolve_task_overrides
 
             config = _get_env_config()
             env_type = config["env_type"]
-            overrides = _task_env_overrides.get(task_id, {})
+            overrides = resolve_task_overrides(raw_task_id)
 
             if env_type == "docker":
                 image = overrides.get("docker_image") or config["docker_image"]

tools/terminal_tool.py

@@ -1034,6 +1034,26 @@ def _resolve_container_task_id(task_id: Optional[str]) -> str:
     return "default"
 
 
+def resolve_task_overrides(task_id: Optional[str]) -> Dict[str, Any]:
+    """Return the env overrides for *task_id*, raw key first then collapsed.
+
+    ``register_task_env_overrides`` writes under the *raw* task/session id, but
+    a CWD-only override collapses (:func:`_resolve_container_task_id`) to the
+    shared ``"default"`` container so per-session surfaces (ACP/gateway/
+    dashboard) don't each spin up their own sandbox. Callers that need the
+    override (terminal command setup, file-tool cwd resolution) must therefore
+    read the raw id FIRST and only fall back to the collapsed container id, or
+    the originating session's override is silently dropped. This is the single
+    source of that lookup so the terminal and file layers can't drift apart.
+    """
+    raw = task_id or "default"
+    return (
+        _task_env_overrides.get(raw)
+        or _task_env_overrides.get(_resolve_container_task_id(raw))
+        or {}
+    )
+
+
 # Configuration from environment variables
 
 def _parse_env_var(name: str, default: str, converter: Any = int, type_label: str = "integer"):
@@ -1885,20 +1905,12 @@ def terminal_tool(
         effective_task_id = _resolve_container_task_id(task_id)
 
         # Check per-task overrides (set by environments like TerminalBench2Env)
-        # before falling back to global env var config.
-        #
-        # Overrides are keyed by the *raw* task_id (that's the key
-        # ``register_task_env_overrides`` writes under), NOT by the collapsed
-        # container id. A CWD-only override collapses ``effective_task_id`` to
-        # ``"default"`` for container sharing, but its cwd must still be read
-        # back here under the originating task_id, or the override is silently
-        # dropped. Fall back to the collapsed id so isolation-keyed RL/benchmark
-        # overrides (registered under an id that equals their container id) keep
-        # resolving as before.
-        overrides = (
-            (_task_env_overrides.get(task_id) if task_id else None)
-            or _task_env_overrides.get(effective_task_id, {})
-        )
+        # before falling back to global env var config. ``resolve_task_overrides``
+        # reads the raw task id first then the collapsed container id, so a
+        # CWD-only override (which collapses ``effective_task_id`` to
+        # ``"default"``) is still found under its originating session id while
+        # isolation-keyed RL/benchmark overrides keep resolving as before.
+        overrides = resolve_task_overrides(task_id)
         
         # Select image based on env type, with per-task override support
         if env_type == "docker":