diff --git a/tests/tools/test_session_cwd_store.py b/tests/tools/test_session_cwd_store.py index 239ca5c07b7e..6db7f10b1367 100644 --- a/tests/tools/test_session_cwd_store.py +++ b/tests/tools/test_session_cwd_store.py @@ -174,3 +174,87 @@ class TestDelegateSeedsChildRecord: tt.record_session_cwd("child-1", "/child/scratch") assert tt.get_session_cwd("parent-task") == "/parent/worktree" assert tt.get_session_cwd("child-1") == "/child/scratch" + + +class TestCommandCwdReadsTheRecord: + """Step 3: _resolve_command_cwd prefers the session's own record.""" + + def test_record_beats_foreign_env_cwd_for_commands(self): + class _Env: + cwd = "/other/sessions/worktree" + cwd_owner = "" # unowned shared env — the leak-A shape + + tt.record_session_cwd("sess-a", "/my/worktree") + resolved = tt._resolve_command_cwd( + workdir=None, + env=_Env(), + default_cwd="/config/default", + prev_owner="", + session_key="sess-a", + ) + assert resolved == "/my/worktree" + + def test_workdir_still_beats_the_record(self): + tt.record_session_cwd("sess-a", "/my/worktree") + resolved = tt._resolve_command_cwd( + workdir="/explicit/place", + env=None, + default_cwd="/config/default", + session_key="sess-a", + ) + assert resolved == "/explicit/place" + + def test_no_record_falls_back_to_legacy_env_cwd(self): + """Transition path: session with no record keeps prior behavior.""" + class _Env: + cwd = "/live/dir" + cwd_owner = "sess-a" + + resolved = tt._resolve_command_cwd( + workdir=None, + env=_Env(), + default_cwd="/config/default", + prev_owner="sess-a", + session_key="sess-a", + ) + assert resolved == "/live/dir" + + def test_no_record_no_env_falls_back_to_default(self): + resolved = tt._resolve_command_cwd( + workdir=None, + env=None, + default_cwd="/config/default", + session_key="sess-a", + ) + assert resolved == "/config/default" + + def test_cd_then_next_command_runs_in_the_new_dir(self, monkeypatch): + """E2E through terminal_tool: the record round-trips cd state.""" + import json + + class FakeEnv: + env = {} + cwd = "/start" + def execute(self, command, **kwargs): + self.last_cwd_arg = kwargs.get("cwd") + if command.startswith("cd "): + self.cwd = command[3:] + return {"output": "", "returncode": 0} + + fake = FakeEnv() + monkeypatch.setattr(tt, "_active_environments", {"sess-a": fake}) + monkeypatch.setattr(tt, "_last_activity", {}) + monkeypatch.setattr( + tt, "_get_env_config", + lambda: {"env_type": "local", "cwd": "/default", "timeout": 60, + "lifetime_seconds": 3600}, + ) + monkeypatch.setattr( + tt, "_check_all_guards", + lambda command, env_type, **kwargs: {"approved": True}, + ) + + json.loads(tt.terminal_tool(command="cd /project", task_id="sess-a")) + assert tt.get_session_cwd("sess-a") == "/project" + json.loads(tt.terminal_tool(command="pwd", task_id="sess-a")) + assert fake.last_cwd_arg == "/project" diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index c3fc8774d332..a988c2dbe24c 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -2045,25 +2045,31 @@ def _resolve_command_cwd( env: Any, default_cwd: str, prev_owner: Optional[str] = None, + session_key: Optional[str] = None, ) -> str: - """Return the cwd for a command, preferring the live session cwd. + """Return the cwd for a command. Explicit ``workdir=`` overrides everything. - ``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. + Resolution (cwd rearch, step 3): - When ``prev_owner`` is provided and differs from the current session, - ``env.cwd`` was mutated by a *different* session's ``cd`` and must NOT be - trusted — fall through to ``default_cwd`` (the config/override cwd) so - the command runs in this session's own workspace, not the previous - session's leftover checkout. This mirrors the ``_live_cwd_if_owned`` - guard file_tools uses for the same shared-env problem. + 1. ``workdir`` — the caller said exactly where to run. + 2. The session's own cwd RECORD (``get_session_cwd(session_key)``) — + written after every completed command for this session, so it IS the + session's ``cd`` state, with no shared-env ambiguity: another + session's ``cd`` lands in another record and can't affect us. + 3. Transition-only: the shared env's live cwd, gated by the legacy + ownership guard. Only reachable for a session with no record yet + (no command completed since this code loaded). When ``prev_owner`` + differs from the current session, ``env.cwd`` is a different + session's leftover ``cd`` and falls through to ``default_cwd``. + 4. ``default_cwd`` (config/override cwd). """ if workdir: return workdir + recorded = get_session_cwd(session_key) + if recorded: + return recorded + live_cwd = getattr(env, "cwd", None) if isinstance(live_cwd, str) and live_cwd.strip(): # The env is shared (collapsed to "default"); its cwd tracks the LAST @@ -2071,10 +2077,10 @@ def _resolve_command_cwd( # before this call claimed it, env.cwd is that session's leftover `cd` # — not ours. Don't use it. if prev_owner is not None: - session_key = getattr(env, "cwd_owner", "") + owner_key = getattr(env, "cwd_owner", "") # cwd_owner was already overwritten to the current session at the # call site, so compare against the captured previous owner. - if prev_owner and prev_owner != "default" and session_key != prev_owner: + if prev_owner and prev_owner != "default" and owner_key != prev_owner: return default_cwd return live_cwd @@ -2459,6 +2465,7 @@ def terminal_tool( env=env, default_cwd=cwd, prev_owner=prev_cwd_owner, + session_key=session_key, ) try: if env_type == "local": @@ -2720,6 +2727,7 @@ def terminal_tool( env=env, default_cwd=cwd, prev_owner=prev_cwd_owner, + session_key=session_key, ) execute_kwargs = { "timeout": effective_timeout,