From 7a315bd702a8df0fe01d235a1d3ddc477fa8a8ea Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 31 May 2026 23:50:40 +0530 Subject: [PATCH] fix(tools): preserve live session cwd in terminal_tool, and keep ACP update_cwd authoritative terminal_tool re-sent the init-time/config cwd on every command, clobbering session-local `cd` state: the environment tracked the new directory in `env.cwd`, but foreground/background calls forced the old cwd back. A small `_resolve_command_cwd` resolver now applies the precedence `workdir > live env.cwd > config/override cwd` to: - foreground `env.execute(...)` - background `process_registry.spawn_local(...)` - background `process_registry.spawn_via_env(...)` Additionally, syncing the cwd onto the live cached env when a `cwd` override is (re-)registered. Preferring live `env.cwd` would otherwise demote the ACP `update_cwd` override (registered via `register_task_env_overrides` on `session/load` / `session/resume`) below an already-set `env.cwd`, silently ignoring an editor's mid-session project-root change once any command had run. `register_task_env_overrides` now pushes a new cwd onto the cached env so an explicit ACP cwd change wins, while ordinary in-session `cd` tracking is preserved. Regression coverage: - foreground/background commands follow live `env.cwd` - explicit `workdir` still overrides everything - registering a cwd override updates the live env cwd (ACP authority) - no-op when no live env exists; non-cwd overrides leave env.cwd untouched Based on #35510 by @Dusk1e. Co-authored-by: Dusk1e --- tests/tools/test_terminal_task_cwd.py | 146 ++++++++++++++++++++++++++ tools/terminal_tool.py | 53 +++++++++- 2 files changed, 197 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_terminal_task_cwd.py b/tests/tools/test_terminal_task_cwd.py index 8c8ff867c..1947836fb 100644 --- a/tests/tools/test_terminal_task_cwd.py +++ b/tests/tools/test_terminal_task_cwd.py @@ -1,6 +1,7 @@ """Regression tests for task/session cwd propagation in terminal_tool.""" import json +from types import SimpleNamespace import tools.terminal_tool as terminal_tool @@ -10,6 +11,7 @@ def _minimal_terminal_config(cwd="/default"): "env_type": "local", "cwd": cwd, "timeout": 60, + "lifetime_seconds": 3600, } @@ -72,3 +74,147 @@ def test_explicit_workdir_still_wins_over_registered_task_cwd(monkeypatch): assert result["exit_code"] == 0 assert calls == [{"timeout": 60, "cwd": "/explicit/workdir"}] + + +def test_foreground_command_prefers_live_env_cwd_over_init_time_cwd(monkeypatch): + """A prior `cd` updates env.cwd; terminal_tool must honor that live cwd.""" + calls = [] + + class FakeEnv: + env = {} + cwd = "/workspace/live" + + def execute(self, command, **kwargs): + calls.append((command, kwargs)) + return {"output": "ok", "returncode": 0} + + task_id = "session-live-cwd" + monkeypatch.setattr(terminal_tool, "_active_environments", {task_id: FakeEnv()}) + monkeypatch.setattr(terminal_tool, "_last_activity", {}) + monkeypatch.setattr(terminal_tool, "_task_env_overrides", {task_id: {"cwd": "/workspace/init"}}) + monkeypatch.setattr(terminal_tool, "_get_env_config", lambda: _minimal_terminal_config(cwd="/workspace/init")) + monkeypatch.setattr(terminal_tool, "_start_cleanup_thread", lambda: None) + monkeypatch.setattr(terminal_tool, "_resolve_container_task_id", lambda value: value or "default") + monkeypatch.setattr( + terminal_tool, + "_check_all_guards", + lambda command, env_type: {"approved": True}, + ) + + result = json.loads(terminal_tool.terminal_tool(command="pwd", task_id=task_id)) + + assert result["exit_code"] == 0 + assert calls == [("pwd", {"timeout": 60, "cwd": "/workspace/live"})] + + +def test_background_command_prefers_live_env_cwd_over_init_time_cwd(monkeypatch): + """Background process launches must also use the live session cwd.""" + + class FakeEnv: + env = {} + cwd = "/workspace/live" + + class FakeRegistry: + def __init__(self): + self.calls = [] + self.pending_watchers = [] + + def spawn_local(self, **kwargs): + self.calls.append(kwargs) + return SimpleNamespace(id="proc_test", pid=1234) + + import tools.process_registry as process_registry_mod + + registry = FakeRegistry() + task_id = "session-live-cwd-bg" + monkeypatch.setattr(terminal_tool, "_active_environments", {task_id: FakeEnv()}) + monkeypatch.setattr(terminal_tool, "_last_activity", {}) + monkeypatch.setattr(terminal_tool, "_task_env_overrides", {task_id: {"cwd": "/workspace/init"}}) + monkeypatch.setattr(terminal_tool, "_get_env_config", lambda: _minimal_terminal_config(cwd="/workspace/init")) + monkeypatch.setattr(terminal_tool, "_start_cleanup_thread", lambda: None) + monkeypatch.setattr(terminal_tool, "_resolve_container_task_id", lambda value: value or "default") + monkeypatch.setattr( + terminal_tool, + "_check_all_guards", + lambda command, env_type: {"approved": True}, + ) + monkeypatch.setattr(process_registry_mod, "process_registry", registry) + + result = json.loads( + terminal_tool.terminal_tool( + command="sleep 1", + task_id=task_id, + background=True, + ) + ) + + assert result["exit_code"] == 0 + assert registry.calls == [{ + "command": "sleep 1", + "cwd": "/workspace/live", + "task_id": task_id, + "session_key": "", + "env_vars": {}, + "use_pty": False, + }] + + +def test_registering_cwd_override_updates_live_env_cwd(monkeypatch): + """An ACP ``update_cwd`` (re-)registered mid-session must win over a + previously ``cd``-ed live ``env.cwd``. + + Preferring live ``env.cwd`` (so session-local ``cd`` survives) means a + freshly registered ``cwd`` override would otherwise sit *below* the + already-set ``env.cwd`` and be silently ignored. ``register_task_env_overrides`` + syncs the new cwd onto the live cached env so an explicit ACP project-root + change takes effect, as the editor client expects. + """ + + class FakeEnv: + env = {} + cwd = "/workspace/old" + + task_id = "acp-session-update" + fake_env = FakeEnv() + monkeypatch.setattr(terminal_tool, "_active_environments", {task_id: fake_env}) + monkeypatch.setattr(terminal_tool, "_task_env_overrides", {}) + + terminal_tool.register_task_env_overrides(task_id, {"cwd": "/workspace/new"}) + + # The live env now reflects the editor's new project root. + assert fake_env.cwd == "/workspace/new" + + # A subsequent command resolves to the new cwd (env.cwd precedence). + assert terminal_tool._resolve_command_cwd( + workdir=None, env=fake_env, default_cwd="/workspace/config" + ) == "/workspace/new" + + +def test_registering_cwd_override_noop_when_no_live_env(monkeypatch): + """Registering an override before the env exists must not crash; the cwd + is applied at env creation time instead.""" + monkeypatch.setattr(terminal_tool, "_active_environments", {}) + monkeypatch.setattr(terminal_tool, "_task_env_overrides", {}) + + # Should not raise even though no env is cached yet. + terminal_tool.register_task_env_overrides("acp-session-pending", {"cwd": "/workspace/new"}) + + assert terminal_tool._task_env_overrides["acp-session-pending"] == {"cwd": "/workspace/new"} + + +def test_registering_non_cwd_override_leaves_live_env_cwd_untouched(monkeypatch): + """A non-cwd override (e.g. a per-task Modal image) must not disturb the + live env's cwd.""" + + class FakeEnv: + env = {} + cwd = "/workspace/keep" + + task_id = "rl-rollout-1" + fake_env = FakeEnv() + monkeypatch.setattr(terminal_tool, "_active_environments", {task_id: fake_env}) + monkeypatch.setattr(terminal_tool, "_task_env_overrides", {}) + + terminal_tool.register_task_env_overrides(task_id, {"modal_image": "custom:latest"}) + + assert fake_env.cwd == "/workspace/keep" diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index 8351d61eb..1a7c32170 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -962,6 +962,23 @@ def register_task_env_overrides(task_id: str, overrides: Dict[str, Any]): """ _task_env_overrides[task_id] = overrides + # If a live environment already exists for this task, a freshly registered + # ``cwd`` override (e.g. the ACP client switching the editor's project root + # mid-session via ``session/load`` / ``session/resume``) must take effect on + # the cached env too. ``terminal_tool`` resolves the per-command cwd as + # ``workdir > env.cwd > config/override cwd`` so that ordinary in-session + # ``cd`` state is preserved; without syncing here the override would sit + # below the (already-set) ``env.cwd`` and be silently ignored once any + # command has run. Pushing it onto the live env keeps ``cd`` tracking intact + # while letting an explicit ACP cwd change win, as the client expects. + new_cwd = overrides.get("cwd") + if isinstance(new_cwd, str) and new_cwd.strip(): + container_id = _resolve_container_task_id(task_id) + with _env_lock: + env = _active_environments.get(container_id) + if env is not None and getattr(env, "cwd", None) is not None: + env.cwd = new_cwd + def clear_task_env_overrides(task_id: str): """ @@ -1718,6 +1735,30 @@ def _resolve_notification_flag_conflict( return watch_patterns, "" +def _resolve_command_cwd( + *, + workdir: Optional[str], + env: Any, + default_cwd: str, +) -> str: + """Return the cwd for a command, preferring the live session cwd. + + ``terminal_tool`` historically re-sent the init-time/config cwd on every + call. That broke session-local ``cd`` state: the environment tracked the + new directory in ``env.cwd``, but foreground/background calls kept forcing + the old cwd back through ``env.execute(..., cwd=...)``. Explicit + ``workdir=`` must still override everything. + """ + if workdir: + return workdir + + live_cwd = getattr(env, "cwd", None) + if isinstance(live_cwd, str) and live_cwd.strip(): + return live_cwd + + return default_cwd + + def terminal_tool( command: str, background: bool = False, @@ -1990,7 +2031,11 @@ def terminal_tool( from tools.process_registry import process_registry session_key = get_current_session_key(default="") - effective_cwd = workdir or cwd + effective_cwd = _resolve_command_cwd( + workdir=workdir, + env=env, + default_cwd=cwd, + ) try: if env_type == "local": proc_session = process_registry.spawn_local( @@ -2207,7 +2252,11 @@ def terminal_tool( try: execute_kwargs = { "timeout": effective_timeout, - "cwd": workdir or cwd, + "cwd": _resolve_command_cwd( + workdir=workdir, + env=env, + default_cwd=cwd, + ), } result = env.execute(command, **execute_kwargs) except Exception as e: