mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-09 08:21:50 +00:00
fix(file-tools): reject sentinel TERMINAL_CWD; anchor worktree edits before live cwd exists (#41861)
Completes the worktree-misroute fix from #35399, which made misroutes visible (resolved_path) but did not prevent them: its divergence warning only fired once a terminal command had populated the live cwd registry. A fresh worktree session (registry still empty) with a stale TERMINAL_CWD='.' got neither a worktree anchor nor a warning, so a relative write_file/patch silently landed in the MAIN checkout. Two changes in tools/file_tools.py: - Treat sentinel TERMINAL_CWD values ('', '.', './', 'auto', 'cwd') and any relative value as UNSET rather than a literal anchor. Previously '.' was joined onto the process cwd, silently routing edits to wherever the process happened to be (the main repo, in a worktree session). The gateway already sanitizes the same set at import time; the file-tool layer now matches. - New _authoritative_workspace_root(): prefers the live terminal cwd, else a sentinel-free absolute TERMINAL_CWD (the worktree path cli.py/main.py set for -w). _resolve_base_dir() and _path_resolution_warning() both use it, so a worktree session resolves into — and warns about escaping — the worktree from the very first write, before any cd has run. Validation: 11 new/parametrized tests (sentinel handling, empty-registry anchoring, early divergence warning, live-cwd precedence). 32/32 pass under scripts/run_tests.sh. Live E2E: relative write in an empty-registry worktree session lands in the worktree, main untouched.
This commit is contained in:
Teknium
GitHub
parent
e02f4c03c3
commit
e45b745835
2 changed files with 172 additions and 22 deletions
|
|
@ -152,12 +152,109 @@ def test_no_warning_for_absolute_input(_isolated_cwd, monkeypatch):
|
|||
def test_no_warning_when_no_live_cwd(_isolated_cwd, monkeypatch):
|
||||
workspace, decoy = _isolated_cwd
|
||||
monkeypatch.setattr(ft, "_get_live_tracking_cwd", lambda task_id="default": None)
|
||||
monkeypatch.delenv("TERMINAL_CWD", raising=False)
|
||||
|
||||
warn = ft._path_resolution_warning("target.py", decoy / "target.py", task_id="default")
|
||||
|
||||
assert warn is None
|
||||
|
||||
|
||||
# ── Fix C: sentinel TERMINAL_CWD + empty-registry worktree anchoring ─────────
|
||||
# (May 2026 follow-up: PR #35399 made misroutes visible via resolved_path but
|
||||
# the divergence warning only fired when the live terminal cwd was known. A
|
||||
# worktree session whose terminal registry is still empty — no `cd` run yet —
|
||||
# got neither a worktree anchor nor a warning, so a relative edit silently
|
||||
# landed in main. These tests pin the sentinel handling + empty-registry
|
||||
# anchoring + early warning.)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("sentinel", ["", ".", "./", "auto", "cwd", "CWD", "Auto"])
|
||||
def test_sentinel_terminal_cwd_is_treated_as_unset(_isolated_cwd, monkeypatch, sentinel):
|
||||
"""Sentinel TERMINAL_CWD values are NOT used as a directory anchor.
|
||||
|
||||
They fall through to the (absolute) process cwd, exactly as if unset —
|
||||
never resolved as a literal relative directory.
|
||||
"""
|
||||
workspace, decoy = _isolated_cwd
|
||||
monkeypatch.setattr(ft, "_get_live_tracking_cwd", lambda task_id="default": None)
|
||||
monkeypatch.setenv("TERMINAL_CWD", sentinel)
|
||||
|
||||
assert ft._configured_terminal_cwd() is None
|
||||
resolved = ft._resolve_path_for_task("target.py", task_id="default")
|
||||
assert resolved.is_absolute()
|
||||
assert resolved == (decoy / "target.py").resolve()
|
||||
|
||||
|
||||
def test_relative_nonsentinel_terminal_cwd_rejected(_isolated_cwd, monkeypatch):
|
||||
"""A relative (but non-sentinel) TERMINAL_CWD is still rejected as an anchor.
|
||||
|
||||
A relative anchor is ambiguous (relative to which cwd?), which is the exact
|
||||
ambiguity that misroutes edits. It must fall through to the process cwd, not
|
||||
be joined onto it as a literal subdir.
|
||||
"""
|
||||
workspace, decoy = _isolated_cwd
|
||||
monkeypatch.setattr(ft, "_get_live_tracking_cwd", lambda task_id="default": None)
|
||||
monkeypatch.setenv("TERMINAL_CWD", "some/rel/path")
|
||||
|
||||
assert ft._configured_terminal_cwd() is None
|
||||
resolved = ft._resolve_path_for_task("target.py", task_id="default")
|
||||
assert resolved == (decoy / "target.py").resolve()
|
||||
|
||||
|
||||
def test_absolute_terminal_cwd_anchors_with_empty_registry(_isolated_cwd, monkeypatch):
|
||||
"""The incident-preventing case: worktree session, registry still empty.
|
||||
|
||||
With no live terminal cwd recorded yet but an absolute TERMINAL_CWD (the
|
||||
worktree path cli.py/main.py set for `-w`), a relative edit must land in the
|
||||
worktree — not the process cwd (main repo).
|
||||
"""
|
||||
workspace, decoy = _isolated_cwd
|
||||
monkeypatch.setattr(ft, "_get_live_tracking_cwd", lambda task_id="default": None)
|
||||
monkeypatch.setenv("TERMINAL_CWD", str(workspace))
|
||||
|
||||
resolved = ft._resolve_path_for_task("target.py", task_id="default")
|
||||
|
||||
assert resolved == (workspace / "target.py")
|
||||
assert not str(resolved).startswith(str(decoy))
|
||||
|
||||
|
||||
def test_warning_fires_from_terminal_cwd_when_registry_empty(_isolated_cwd, monkeypatch):
|
||||
"""Divergence warning must fire even before any terminal command runs.
|
||||
|
||||
PR #35399's warning required a live terminal cwd; a fresh worktree session
|
||||
(empty registry) silently misrouted with no warning. Now the warning falls
|
||||
back to the absolute TERMINAL_CWD anchor, so an edit aimed outside the
|
||||
worktree is flagged on the very first write.
|
||||
"""
|
||||
workspace, decoy = _isolated_cwd
|
||||
monkeypatch.setattr(ft, "_get_live_tracking_cwd", lambda task_id="default": None)
|
||||
monkeypatch.setenv("TERMINAL_CWD", str(workspace))
|
||||
|
||||
# Relative path that escapes the worktree into the decoy/main checkout.
|
||||
escaping = os.path.relpath(str(decoy / "target.py"), str(workspace))
|
||||
resolved = ft._resolve_path_for_task(escaping, task_id="default")
|
||||
|
||||
warn = ft._path_resolution_warning(escaping, resolved, task_id="default")
|
||||
|
||||
assert warn is not None
|
||||
assert "OUTSIDE the active workspace" in warn
|
||||
assert str(workspace) in warn
|
||||
|
||||
|
||||
def test_live_cwd_still_wins_over_absolute_terminal_cwd(_isolated_cwd, monkeypatch):
|
||||
"""When both are present, the live terminal cwd remains authoritative."""
|
||||
workspace, decoy = _isolated_cwd
|
||||
other = decoy.parent / "other"
|
||||
other.mkdir()
|
||||
# Live cwd = workspace; TERMINAL_CWD points elsewhere — live must win.
|
||||
monkeypatch.setattr(ft, "_get_live_tracking_cwd", lambda task_id="default": str(workspace))
|
||||
monkeypatch.setenv("TERMINAL_CWD", str(other))
|
||||
|
||||
resolved = ft._resolve_path_for_task("target.py", task_id="default")
|
||||
|
||||
assert resolved == (workspace / "target.py")
|
||||
|
||||
|
||||
# ── Fix A: write_file / patch report the resolved ABSOLUTE path ──────────────
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -85,6 +85,34 @@ def _resolve_path(filepath: str, task_id: str = "default") -> Path:
|
|||
return _resolve_path_for_task(filepath, task_id)
|
||||
|
||||
|
||||
# Sentinel ``TERMINAL_CWD`` values that mean "not configured", NOT a literal
|
||||
# directory to resolve against. A stale config / .env commonly leaves the
|
||||
# literal "." here; "auto"/"cwd" are setup-wizard placeholders. Treating any of
|
||||
# these as a real relative base silently anchors edits to the agent PROCESS cwd
|
||||
# (e.g. the main repo while a worktree session is active), routing writes to the
|
||||
# wrong checkout. The gateway sanitizes the same set at import time
|
||||
# (gateway/run.py); the file/terminal-tool layer must do likewise so CLI
|
||||
# sessions get the same protection. See references/worktree-cwd-discipline.md.
|
||||
_TERMINAL_CWD_SENTINELS = frozenset({"", ".", "./", "auto", "cwd"})
|
||||
|
||||
|
||||
def _configured_terminal_cwd() -> str | None:
|
||||
"""Return ``$TERMINAL_CWD`` only when it names a real directory anchor.
|
||||
|
||||
Sentinel values (see ``_TERMINAL_CWD_SENTINELS``) and relative paths are
|
||||
rejected — a relative anchor is meaningless without knowing which cwd it is
|
||||
relative to, which is exactly the ambiguity that misroutes worktree edits.
|
||||
Only an absolute, sentinel-free value is honored.
|
||||
"""
|
||||
raw = (os.environ.get("TERMINAL_CWD") or "").strip()
|
||||
if raw.lower() in _TERMINAL_CWD_SENTINELS:
|
||||
return None
|
||||
expanded = os.path.expanduser(raw)
|
||||
if not os.path.isabs(expanded):
|
||||
return None
|
||||
return expanded
|
||||
|
||||
|
||||
def _get_live_tracking_cwd(task_id: str = "default") -> str | None:
|
||||
"""Return the task's live terminal cwd for bookkeeping when available."""
|
||||
try:
|
||||
|
|
@ -116,33 +144,54 @@ def _get_live_tracking_cwd(task_id: str = "default") -> str | None:
|
|||
return None
|
||||
|
||||
|
||||
def _authoritative_workspace_root(task_id: str = "default") -> str | None:
|
||||
"""Best-effort absolute workspace root for divergence checks.
|
||||
|
||||
Prefers the live terminal cwd (the directory the agent is actually working
|
||||
in). When no terminal command has run yet — so the live registry is empty —
|
||||
falls back to a sentinel-free absolute ``$TERMINAL_CWD``. This is what lets
|
||||
a worktree session warn about (and resolve into) the worktree from the very
|
||||
first ``write_file``/``patch``, before any ``cd`` has populated the live cwd.
|
||||
|
||||
Returns ``None`` only when there is genuinely no reliable anchor, in which
|
||||
case callers fall back to the process cwd.
|
||||
"""
|
||||
live = _get_live_tracking_cwd(task_id)
|
||||
if live:
|
||||
return live
|
||||
return _configured_terminal_cwd()
|
||||
|
||||
|
||||
def _resolve_base_dir(task_id: str = "default") -> Path:
|
||||
"""Return the ABSOLUTE base directory for resolving relative paths.
|
||||
|
||||
Resolution order:
|
||||
1. The task's live terminal cwd (the directory the agent is actually
|
||||
working in — e.g. a git worktree). Authoritative when known.
|
||||
2. ``$TERMINAL_CWD`` from config/env.
|
||||
2. A sentinel-free, absolute ``$TERMINAL_CWD`` (the worktree path set by
|
||||
``cli.py``/``main.py`` for ``-w`` sessions). Used even before any
|
||||
terminal command has populated the live cwd registry.
|
||||
3. The process cwd.
|
||||
|
||||
The returned base is ALWAYS absolute. This is the core invariant that
|
||||
prevents the worktree-cwd divergence bug: a relative ``TERMINAL_CWD``
|
||||
(commonly the literal ``"."`` from a stale config) is meaningless as a
|
||||
resolution anchor — left to ``Path.resolve()`` it silently resolves
|
||||
against whatever the agent PROCESS cwd happens to be (e.g. the main repo
|
||||
while the terminal is in a worktree), routing edits to the wrong checkout.
|
||||
Anchoring a relative base against the process cwd here makes the resolution
|
||||
deterministic and inspectable rather than dependent on resolve()-time cwd.
|
||||
prevents the worktree-cwd divergence bug: a relative or sentinel
|
||||
``TERMINAL_CWD`` (commonly the literal ``"."`` from a stale config) is
|
||||
meaningless as a resolution anchor — left to ``Path.resolve()`` it silently
|
||||
resolves against whatever the agent PROCESS cwd happens to be (e.g. the main
|
||||
repo while the terminal is in a worktree), routing edits to the wrong
|
||||
checkout. We therefore reject sentinel/relative ``TERMINAL_CWD`` values
|
||||
outright (rather than anchoring them to the process cwd) and fall through to
|
||||
the process cwd only as a last resort, deterministically.
|
||||
"""
|
||||
live = _get_live_tracking_cwd(task_id)
|
||||
if live:
|
||||
base = Path(live).expanduser()
|
||||
root = _authoritative_workspace_root(task_id)
|
||||
if root:
|
||||
base = Path(root).expanduser()
|
||||
else:
|
||||
raw = os.environ.get("TERMINAL_CWD")
|
||||
base = Path(raw).expanduser() if raw else Path(os.getcwd())
|
||||
base = Path(os.getcwd())
|
||||
if not base.is_absolute():
|
||||
# A relative base (".", "./sub", "..") is anchored to the process cwd
|
||||
# once, here, so the result no longer depends on cwd at resolve() time.
|
||||
# Last-resort anchoring: a live cwd should already be absolute, but if a
|
||||
# terminal backend ever reports a relative cwd, anchor it to the process
|
||||
# cwd once, here, so the result no longer depends on cwd at resolve().
|
||||
base = Path(os.getcwd()) / base
|
||||
return base.resolve()
|
||||
|
||||
|
|
@ -164,18 +213,22 @@ def _path_resolution_warning(filepath: str, resolved: Path, task_id: str = "defa
|
|||
|
||||
Surfaces the worktree-cwd divergence the moment it would matter: if the
|
||||
agent passes a relative path but it resolves under a directory that is not
|
||||
the live terminal cwd (i.e. the edit is about to land in a different
|
||||
checkout than the one the agent is working in), return a message naming the
|
||||
absolute target. ``None`` when the path is absolute, the base is unknown,
|
||||
or the resolved path is correctly under the workspace root.
|
||||
the workspace root (i.e. the edit is about to land in a different checkout
|
||||
than the one the agent is working in), return a message naming the absolute
|
||||
target. ``None`` when the path is absolute, the base is unknown, or the
|
||||
resolved path is correctly under the workspace root.
|
||||
|
||||
The workspace root is the live terminal cwd when known, else a sentinel-free
|
||||
absolute ``$TERMINAL_CWD`` — so a worktree session whose terminal registry
|
||||
is still empty (no ``cd`` run yet) is warned on the very first write.
|
||||
"""
|
||||
try:
|
||||
if Path(filepath).expanduser().is_absolute():
|
||||
return None
|
||||
live = _get_live_tracking_cwd(task_id)
|
||||
if not live:
|
||||
workspace_root = _authoritative_workspace_root(task_id)
|
||||
if not workspace_root:
|
||||
return None # No authoritative workspace root to compare against.
|
||||
root = Path(live).expanduser().resolve()
|
||||
root = Path(workspace_root).expanduser().resolve()
|
||||
# Is `resolved` inside `root`?
|
||||
try:
|
||||
resolved.relative_to(root)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue