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.
This commit is contained in:
@@ -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))
|
||||
@@ -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()
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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'
|
||||
|
||||
@@ -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."""
|
||||
|
||||
+12
-2
@@ -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:
|
||||
|
||||
+14
-2
@@ -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 <cwd> 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:
|
||||
|
||||
Reference in New Issue
Block a user