mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-20 15:33:54 +00:00
refactor(terminal): resolve command cwd from per-session records (step 3)
Third step of the cwd rearchitecture: _resolve_command_cwd now prefers the session's own cwd record over the shared env's live cwd. New resolution order: workdir > session record > legacy env.cwd (ownership-gated, transition-only) > config/override default. The record is written after every completed command for the session, so it IS the session's cd state — another session's cd lands in another record and cannot affect this session's commands. The legacy env.cwd branch only fires for a session with no record yet (no command has completed since this code loaded); it keeps the prev_owner ownership guard for that transition window and is deleted in step 4 along with env.cwd_owner stamping and file_tools' _last_known_cwd machinery. Adds command-path regression tests including the terminal sibling of the leak-A scenario (unowned shared env cwd vs session record) and an E2E cd round-trip through terminal_tool.
This commit is contained in:
parent
5461e0e098
commit
fd3d9c63d0
2 changed files with 106 additions and 14 deletions
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue