From 16a173a8d6912735c1c2dfb682fe93ab69f1ed6d Mon Sep 17 00:00:00 2001 From: ethernet Date: Thu, 13 Aug 2026 17:21:47 -0400 Subject: [PATCH] fix(tools): do not adopt a stale cwd after an interrupted command The command wrapper prints the cwd marker after the command returns. A killed or timed-out command emits no marker, so ``env.cwd`` still holds the directory of the last command to FINISH. One local environment serves every session, because ``_resolve_container_task_id`` collapses cwd-only overrides to ``"default"``. That leftover directory is therefore routinely another session's. The post-command dual-write copied ``env.cwd`` into the interrupted session's durable record. Every later command in that session then ran in the foreign directory, and the cwd echo told the model it had moved there. A desktop chat silently re-homed into a worktree that another chat had opened. Report the observation instead of inferring it. The marker parse now sets ``result["cwd_observed"]``, and both the record write and the echo read that flag. The local override clears the flag when it rolls back a path that does not exist, because the restored value is also unobserved. When a command reports no cwd, the session keeps the directory it already had. This needs no second session to be wrong: a lone session that interrupts a command re-adopts a stale value too. A second session only makes the wrong directory belong to somebody else. The same class of write exists in the file-tools rescue for a reaped environment (#26211). That rescue copied the cached snapshot of the shared ``env.cwd`` into the session record. The rescue is now fill-only: it writes the snapshot when the session has no record, and it never overwrites a record that the session wrote for itself. The tests drive ``terminal_tool`` itself through an interrupt, not a copy of its gate. Review found that a revert of either call site passed the first version of the tests. Each gate now has a test that fails when the gate is removed (verified by mutation). Two exact-dict assertions in the Vercel sandbox tests now assert the two fields they care about, so a new result key does not fail them. --- tests/tools/test_interrupted_command_cwd.py | 205 ++++++++++++++++++ tests/tools/test_session_cwd_store.py | 48 +++- .../tools/test_vercel_sandbox_environment.py | 6 +- tools/environments/base.py | 8 + tools/environments/local.py | 3 + tools/file_tools.py | 14 +- tools/terminal_tool.py | 16 +- 7 files changed, 292 insertions(+), 8 deletions(-) create mode 100644 tests/tools/test_interrupted_command_cwd.py diff --git a/tests/tools/test_interrupted_command_cwd.py b/tests/tools/test_interrupted_command_cwd.py new file mode 100644 index 0000000000..ecf14f176e --- /dev/null +++ b/tests/tools/test_interrupted_command_cwd.py @@ -0,0 +1,205 @@ +"""An interrupted command must not adopt the shared environment's cwd. + +The command wrapper prints the ``__HERMES_CWD_*`` marker AFTER the command +returns, so a killed / timed-out command emits none and ``env.cwd`` still holds +whatever the last command to FINISH left there. One local environment is shared +by every session (``_resolve_container_task_id`` collapses cwd-only overrides to +``"default"``), so that leftover is routinely ANOTHER session's directory. + +Without the ``cwd_observed`` gate, the post-command dual-write stamped that +foreign directory onto the interrupted session's durable record, and every later +command in that session ran there — a silent re-home into a directory the user +never opened. +""" + +import os +import tempfile + +import pytest + +import tools.terminal_tool as tt +from tools.environments.local import LocalEnvironment + + +@pytest.fixture(autouse=True) +def _clean_store(monkeypatch): + monkeypatch.setattr(tt, "_session_cwd", {}) + monkeypatch.setattr(tt, "_task_env_overrides", {}) + monkeypatch.delenv("TERMINAL_ENV", raising=False) + + +@pytest.fixture +def env(tmp_path): + environment = LocalEnvironment(cwd=str(tmp_path), timeout=30) + environment.init_session() + yield environment + environment.cleanup() + + +def _run(env, session_key, command, timeout=30): + """One turn of the terminal tool's per-session cwd handling. + + Mirrors terminal_tool.py: resolve this session's cwd, run, then dual-write + the record only when the command reported where it finished. + """ + command_cwd = tt._resolve_command_cwd( + workdir=None, default_cwd=env.cwd, session_key=session_key, env_type="local" + ) + result = env.execute(command, cwd=command_cwd, timeout=timeout) + if result.get("cwd_observed"): + tt.record_session_cwd(session_key, getattr(env, "cwd", None)) + return result, command_cwd + + +class TestCwdObservedFlag: + def test_completed_command_reports_its_cwd(self, env, tmp_path): + target = tmp_path / "done" + target.mkdir() + result, _ = _run(env, "sess", f"cd {target} && pwd") + assert result["cwd_observed"] is True + + def test_interrupted_command_reports_no_cwd(self, env, tmp_path): + target = tmp_path / "slow" + target.mkdir() + result, _ = _run(env, "sess", f"cd {target} && sleep 20", timeout=2) + # Killed before the wrapper could print the marker. + assert not result.get("cwd_observed") + + +class TestInterruptDoesNotStealAnotherSessionsCwd: + def test_record_survives_an_interrupt(self, env, tmp_path): + mine = tmp_path / "mine" + theirs = tmp_path / "theirs" + mine.mkdir() + theirs.mkdir() + + _run(env, "mine", f"cd {mine} && pwd") + assert tt.get_session_cwd("mine") == str(mine) + + # Another chat finishes a command; the shared env now points at it. + _run(env, "theirs", f"cd {theirs} && pwd") + assert os.path.realpath(env.cwd) == os.path.realpath(str(theirs)) + + # My command is interrupted. My record must not adopt their directory. + _run(env, "mine", f"cd {mine} && sleep 20", timeout=2) + assert tt.get_session_cwd("mine") == str(mine) + + def test_next_command_still_runs_in_my_directory(self, env, tmp_path): + mine = tmp_path / "mine" + theirs = tmp_path / "theirs" + mine.mkdir() + theirs.mkdir() + + _run(env, "mine", f"cd {mine} && pwd") + _run(env, "theirs", f"cd {theirs} && pwd") + _run(env, "mine", f"cd {mine} && sleep 20", timeout=2) + + result, _ = _run(env, "mine", "pwd") + assert os.path.realpath(result["output"].strip()) == os.path.realpath(str(mine)) + + def test_single_session_keeps_its_own_prior_directory(self, env, tmp_path): + """No second session needed: a lone session must not re-home either.""" + first = tmp_path / "first" + second = tmp_path / "second" + first.mkdir() + second.mkdir() + + _run(env, "solo", f"cd {first} && pwd") + # Move the shared env elsewhere the way any other consumer would. + env.execute(f"cd {second} && pwd", cwd=str(second)) + + _run(env, "solo", f"cd {first} && sleep 20", timeout=2) + assert tt.get_session_cwd("solo") == str(first) + + +class TestEchoIsGatedToo: + def test_interrupted_command_does_not_echo_a_foreign_cwd(self, env, tmp_path): + """The echo tells the model where it ended up; it must not lie.""" + mine = tmp_path / "mine" + theirs = tmp_path / "theirs" + mine.mkdir() + theirs.mkdir() + + _run(env, "mine", f"cd {mine} && pwd") + _run(env, "theirs", f"cd {theirs} && pwd") + + result, command_cwd = _run(env, "mine", f"cd {mine} && sleep 20", timeout=2) + + # The echo block in terminal_tool reads env.cwd only when observed. + post_cwd = getattr(env, "cwd", None) if result.get("cwd_observed") else None + echoed = ( + str(post_cwd) + if post_cwd + and command_cwd + and os.path.realpath(str(post_cwd)) != os.path.realpath(str(command_cwd)) + else None + ) + assert echoed is None + + +class TestTerminalToolReadsTheFlag: + """Drive ``tt.terminal_tool`` itself, not a reimplementation of its gate. + + The classes above prove that the ENVIRONMENT produces ``cwd_observed`` + correctly. These tests prove that the terminal tool CONSUMES it: the + record write and the cwd echo. Without them, reverting either call site + to the ungated read passes every other test in this file (verified by + mutation during review). + """ + + def _tool(self, monkeypatch, env, command, task_id, timeout=None): + import json + + # One shared env for every session, like the real local backend + # (_resolve_container_task_id collapses cwd-only sessions to "default"). + monkeypatch.setattr(tt, "_active_environments", {"default": env}) + monkeypatch.setattr(tt, "_last_activity", {}) + monkeypatch.setattr( + tt, "_get_env_config", + lambda: {"env_type": "local", "cwd": env.cwd, "timeout": 60, + "lifetime_seconds": 3600}, + ) + monkeypatch.setattr( + tt, "_check_all_guards", + lambda command, env_type, **kwargs: {"approved": True}, + ) + return json.loads( + tt.terminal_tool(command=command, task_id=task_id, timeout=timeout) + ) + + def test_interrupt_keeps_record_and_echoes_no_cwd(self, env, tmp_path, monkeypatch): + mine = tmp_path / "mine" + theirs = tmp_path / "theirs" + mine.mkdir() + theirs.mkdir() + + # My session establishes its directory through the real tool. + result = self._tool(monkeypatch, env, f"cd {mine} && pwd", "mine") + assert result["exit_code"] == 0 + assert tt.get_session_cwd("mine") == str(mine) + + # Another session finishes a command; the shared env moves to it. + result = self._tool(monkeypatch, env, f"cd {theirs} && pwd", "theirs") + assert result["exit_code"] == 0 + assert os.path.realpath(env.cwd) == os.path.realpath(str(theirs)) + + # My command is interrupted. The tool must not write the record + # (terminal_tool.py record write) ... + result = self._tool( + monkeypatch, env, f"cd {mine} && sleep 20", "mine", timeout=2 + ) + assert tt.get_session_cwd("mine") == str(mine) + # ... and must not echo the foreign directory to the model + # (terminal_tool.py cwd echo). Ungated, env.cwd (theirs) differs from + # my command_cwd (mine), so the echo would fire with THEIR directory. + assert "cwd" not in result or result.get("cwd") is None + + def test_completed_command_still_records_and_echoes(self, env, tmp_path, monkeypatch): + """The gate must not break the observed path: cd still round-trips.""" + target = tmp_path / "target" + target.mkdir() + + result = self._tool(monkeypatch, env, f"cd {target} && pwd", "sess") + assert result["exit_code"] == 0 + assert tt.get_session_cwd("sess") == str(target) + assert os.path.realpath(result["cwd"]) == os.path.realpath(str(target)) diff --git a/tests/tools/test_session_cwd_store.py b/tests/tools/test_session_cwd_store.py index c0e0d4dd2d..8a72c5c86e 100644 --- a/tests/tools/test_session_cwd_store.py +++ b/tests/tools/test_session_cwd_store.py @@ -73,9 +73,10 @@ class TestPostCommandDualWrite: env = {} cwd = "/start" def execute(self, command, **kwargs): - # Simulate the env's own post-command tracking (marker parse). + # Simulate the env's own post-command tracking (marker parse): + # the marker is what moves cwd AND what flags the observation. self.cwd = "/new/dir" - return {"output": "", "returncode": 0} + return {"output": "", "returncode": 0, "cwd_observed": True} result = self._run(monkeypatch, "sess-a", FakeEnv()) assert result["exit_code"] == 0 @@ -154,6 +155,47 @@ class TestDelegateSeedsChildRecord: assert tt.get_session_cwd("child-1") == "/child/scratch" +class TestReapedEnvFallbackIsFillOnly: + """file_tools' reaped-env rescue (#26211) must not overwrite the record. + + The cached file_ops' ``cwd`` is a snapshot of the SHARED env, so it can + belong to another session — the same class of error as the + interrupted-command bug (#85658). The rescue may only fill an ABSENT + record. + """ + + def _reap(self, monkeypatch, tmp_path, task_id, stale_cwd): + import tools.file_tools as ft + + class _StaleFileOps: + cwd = stale_cwd + + # Cached file_ops whose env was reaped: cache entry present, + # _active_environments empty. The cache is keyed by the COLLAPSED + # container id ("default" for plain sessions) — that collapse is + # exactly why the snapshot can belong to another session. + container_id = tt._resolve_container_task_id(task_id) + monkeypatch.setattr(ft, "_file_ops_cache", {container_id: _StaleFileOps()}) + monkeypatch.setattr(tt, "_active_environments", {}) + monkeypatch.setattr(tt, "_last_activity", {}) + monkeypatch.setattr( + tt, "_get_env_config", + lambda: {"env_type": "local", "cwd": str(tmp_path), "timeout": 60, + "lifetime_seconds": 3600}, + ) + ft._get_file_ops(task_id) + + def test_existing_record_survives_the_rescue(self, tmp_path, monkeypatch): + tt.record_session_cwd("sess-a", "/my/worktree") + self._reap(monkeypatch, tmp_path, "sess-a", "/other/sessions/dir") + assert tt.get_session_cwd("sess-a") == "/my/worktree" + + def test_absent_record_is_filled(self, tmp_path, monkeypatch): + """#26211 stays fixed: a recordless session still gets the rescue.""" + self._reap(monkeypatch, tmp_path, "sess-b", "/last/known/dir") + assert tt.get_session_cwd("sess-b") == "/last/known/dir" + + class TestCommandCwdReadsTheRecord: """_resolve_command_cwd: workdir > session record > default. Nothing else.""" @@ -187,6 +229,8 @@ class TestCommandCwdReadsTheRecord: self.last_cwd_arg = kwargs.get("cwd") if command.startswith("cd "): self.cwd = command[3:] + # A completed cd emits the cwd marker; the parse sets both. + return {"output": "", "returncode": 0, "cwd_observed": True} return {"output": "", "returncode": 0} fake = FakeEnv() diff --git a/tests/tools/test_vercel_sandbox_environment.py b/tests/tools/test_vercel_sandbox_environment.py index 65eb1ba2a8..dbcf2667e6 100644 --- a/tests/tools/test_vercel_sandbox_environment.py +++ b/tests/tools/test_vercel_sandbox_environment.py @@ -337,7 +337,8 @@ class TestFileSync: result = env.execute("echo hello") - assert result == {"output": "hello\n", "returncode": 0} + assert result["output"] == "hello\n" + assert result["returncode"] == 0 assert vercel_sdk.current.write_files_calls[-1] == [ { "path": "/home/vercel/.hermes/credentials/token.txt", @@ -480,7 +481,8 @@ class TestExecute: result = env.execute("echo hello") - assert result == {"output": "hello\n", "returncode": 0}, label + assert result["output"] == "hello\n", label + assert result["returncode"] == 0, label assert original.closed == 1 assert vercel_sdk.current is replacement diff --git a/tools/environments/base.py b/tools/environments/base.py index b3b04d0a5d..dbdb874240 100644 --- a/tools/environments/base.py +++ b/tools/environments/base.py @@ -1340,6 +1340,13 @@ class BaseEnvironment(ABC): Updates self.cwd and strips the marker from result["output"]. Used by remote backends (Docker, SSH, Modal, Daytona, Singularity). + + Sets ``result["cwd_observed"]`` when the marker yielded a directory for + THIS command. The wrapper prints the marker after the command returns, + so a killed / timed-out command never emits one and ``self.cwd`` keeps + whatever the previous command left there. That environment is shared by + every session, so callers must not attribute an unobserved cwd to the + session that ran this command (see terminal_tool's session-cwd record). """ output = result.get("output", "") marker = self._cwd_marker @@ -1356,6 +1363,7 @@ class BaseEnvironment(ABC): cwd_path = output[first + len(marker) : last].strip() if cwd_path: self.cwd = cwd_path + result["cwd_observed"] = True # Strip the marker line AND the \n we injected before it. # The wrapper emits: printf '\n__MARKER__%s__MARKER__\n' diff --git a/tools/environments/local.py b/tools/environments/local.py index 330d46c53d..de2a6e0346 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -1665,7 +1665,10 @@ class LocalEnvironment(BaseEnvironment): else: # Stale / non-existent path — keep previous cwd; _run_bash # will resolve a safe fallback on the next call if needed. + # The rollback restores a value this command did not observe, + # so it is not attributable to this command's session either. self.cwd = prev_cwd + result.pop("cwd_observed", None) def cleanup(self): """Clean up temp files.""" diff --git a/tools/file_tools.py b/tools/file_tools.py index 0c13e87f69..a47588d0ba 100644 --- a/tools/file_tools.py +++ b/tools/file_tools.py @@ -1437,11 +1437,21 @@ def _get_file_ops(task_id: str = "default") -> ShellFileOperations: # (fixes #26211: silent file-creation failures in long-running # conversations). Usually a no-op: every completed command # already recorded its cwd. + # + # Fill-only: ``cached.cwd`` is a snapshot of the SHARED env's + # cwd at cache-build time, so it is not attributable to this + # session (same class as the interrupted-command bug, #85658). + # Rescue a session that has no record, but never overwrite a + # record the session wrote for itself. old_cwd = getattr(cached, "cwd", None) if old_cwd: try: - from tools.terminal_tool import record_session_cwd - record_session_cwd(raw_task_id, old_cwd) + from tools.terminal_tool import ( + get_session_cwd, + record_session_cwd, + ) + if get_session_cwd(raw_task_id) is None: + record_session_cwd(raw_task_id, old_cwd) except Exception: pass with _file_ops_lock: diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index f94731ddd2..1e0eab2fe9 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -3311,7 +3311,14 @@ def terminal_tool( # (docstring: "Working directory for this command"). Recording it # would hijack the session's durable cwd for every later command # that doesn't pass ``workdir``. Skip the dual-write in that case. - if not workdir: + # + # AND only when the command actually reported its cwd. The marker + # is printed after the command returns, so an interrupted / killed + # / timed-out command emits none and env.cwd still holds whatever + # the last command to FINISH left there — on a shared env, that is + # another session's directory. Recording it silently re-homes this + # session into a directory the user never opened. + if not workdir and (result or {}).get("cwd_observed"): record_session_cwd(session_key, getattr(env, "cwd", None)) # Extract output @@ -3418,8 +3425,13 @@ def terminal_tool( # defensive 'cd X && ' prefix because the model can't see cwd # state; echoing it on change removes the guesswork (pattern # borrowed from crush's injection). + # + # Gated on the same observation flag as the record above: without + # it, an interrupted command echoes the shared env's leftover cwd + # and tells the model it moved to a directory another session + # opened. try: - post_cwd = getattr(env, "cwd", None) + post_cwd = getattr(env, "cwd", None) if (result or {}).get("cwd_observed") else None if post_cwd and command_cwd and os.path.realpath(str(post_cwd)) != os.path.realpath(str(command_cwd)): result_dict["cwd"] = str(post_cwd) except Exception: