diff --git a/agent/tool_executor.py b/agent/tool_executor.py index 5c12edcc7d..4b55c1b88d 100644 --- a/agent/tool_executor.py +++ b/agent/tool_executor.py @@ -74,23 +74,17 @@ def _record_persisted_path_for_stub(agent, tool_call_id: str, function_result) - logger.debug("persisted-path record for result stub failed: %s", exc) -def _checkpoint_container_backend(effective_task_id: str) -> Optional[str]: - """Return the task's backend name when its file paths belong to a container.""" - from tools.file_tools_paths import container_backend_for_task - - return container_backend_for_task(effective_task_id or "default") - - def _ensure_file_checkpoint(agent, function_name: str, function_args: dict, effective_task_id: str) -> None: """Checkpoint the same workspace path that the file tool will mutate, resolved the way file tools do (against the task's live cwd, which differs from the process cwd in Docker).""" file_path = function_args.get("path", "") if not file_path: return - if _checkpoint_container_backend(effective_task_id) is not None: - return # container paths: nothing to checkpoint on the host from agent.file_safety import is_nt_namespace_path - from tools.file_tools_paths import _resolve_path_for_task + from tools.file_tools_paths import _resolve_path_for_task, container_backend_for_task + + if container_backend_for_task(effective_task_id or "default") is not None: + return # container paths: nothing to checkpoint on the host # Resolving an NT-namespace path is itself the NTLM-leak trigger; leave the # tool's raw-string guard to refuse it without a checkpoint stat. @@ -989,7 +983,8 @@ def _begin_tool_execution(agent, ref: _ToolCallRef, display_index: int | None) - elif function_name == "terminal": command = function_args.get("command", "") if _is_destructive_command(command): - if _checkpoint_container_backend(effective_task_id) is None: + from tools.file_tools_paths import container_backend_for_task + if container_backend_for_task(effective_task_id or "default") is None: from agent.runtime_cwd import scope_terminal_cwd cwd = function_args.get("workdir") or scope_terminal_cwd() or os.getcwd() agent._checkpoint_mgr.ensure_checkpoint(cwd, f"before terminal: {command[:60]}") diff --git a/agent/turn_explainers.py b/agent/turn_explainers.py index b517690fb2..7acd4de8a0 100644 --- a/agent/turn_explainers.py +++ b/agent/turn_explainers.py @@ -255,8 +255,8 @@ class TurnExplainersMixin: if mgr is not None and getattr(mgr, "enabled", False): backend = None with suppress(Exception): - from agent.tool_executor import _checkpoint_container_backend - backend = _checkpoint_container_backend(task_id or "default") + from tools.file_tools_paths import container_backend_for_task + backend = container_backend_for_task(task_id or "default") if backend is None: # container paths carry no host ledger entry for _p in landed_paths: with suppress(Exception): diff --git a/gateway/slash_commands.py b/gateway/slash_commands.py index 9a7955b821..37c0acca3e 100644 --- a/gateway/slash_commands.py +++ b/gateway/slash_commands.py @@ -696,9 +696,15 @@ class GatewaySlashCommandsMixin( tokens = event.get_command_args().strip().split() restore_all = any(tok.lower() in ("--all", "--force") for tok in tokens) arg = " ".join(tok for tok in tokens if tok.lower() not in ("--all", "--force")) + # Container-backed session: host checkpoints belong to another tree, so a restore is + # refused; the bare listing stays visible, prefixed with the reason (same as the CLI). + reason = mgr.unsupported_backend_reason() + if reason and arg: + return reason checkpoints = mgr.list_checkpoints(cwd) if not arg: - return format_checkpoint_list(checkpoints, cwd) + listing = format_checkpoint_list(checkpoints, cwd) + return f"{reason}\n{listing}" if reason else listing if not checkpoints: return t("gateway.rollback.none_found", cwd=cwd) @@ -736,6 +742,8 @@ class GatewaySlashCommandsMixin( mgr = self._checkpoint_manager() if mgr is None: return t("gateway.diff.not_enabled") + if reason := mgr.unsupported_backend_reason(): # host baseline is not this session's tree + return reason result = await asyncio.to_thread(mgr.session_diff, cwd) else: from tools.working_diff import collect_working_diff diff --git a/hermes_cli/cli_commands_mixin.py b/hermes_cli/cli_commands_mixin.py index 263555085c..12dff10176 100644 --- a/hermes_cli/cli_commands_mixin.py +++ b/hermes_cli/cli_commands_mixin.py @@ -764,6 +764,8 @@ class CLICommandsMixin: " (Plain /diff still works — it uses git directly.)")) if mgr is None: return + if reason := mgr.unsupported_backend_reason(): # host baseline is not this session's tree + return print(f" {reason}") result = mgr.session_diff(cwd) if not result.get("success"): return print(f" {result.get('error', 'Could not generate diff')}") diff --git a/tests/agent/test_tool_executor_checkpoint_paths.py b/tests/agent/test_tool_executor_checkpoint_paths.py index 8fd91c9df6..0e64210de0 100644 --- a/tests/agent/test_tool_executor_checkpoint_paths.py +++ b/tests/agent/test_tool_executor_checkpoint_paths.py @@ -2,6 +2,7 @@ import json import os +import threading from types import SimpleNamespace import pytest @@ -14,15 +15,13 @@ from tools.terminal_tool import _active_environments, _env_lock @pytest.fixture def manager(tmp_path, monkeypatch): - monkeypatch.setattr( - "tools.checkpoint_manager.CHECKPOINT_BASE", tmp_path / "checkpoints" - ) + monkeypatch.setattr("tools.checkpoint_manager.CHECKPOINT_BASE", tmp_path / "checkpoints") return CheckpointManager(enabled=True) @pytest.fixture def container_task_id(monkeypatch): - class FakeDockerEnvironment: + class FakeDockerEnvironment: # class-name hint is how file_tools_paths classifies live envs pass task_id = "container-checkpoint-task" @@ -67,159 +66,79 @@ def test_relative_file_checkpoint_uses_task_workspace(tmp_path, monkeypatch): assert manager.list_checkpoints(str(process_cwd)) == [] -def _assert_container_skipped(manager, task_id): - assert manager.list_all_checkpoints() == [] - assert "docker" in manager.unsupported_backend_reason(task_id) - - -def test_container_backend_file_checkpoint_is_not_taken_on_missing_host_path( - manager, container_task_id, -): - agent = SimpleNamespace(_checkpoint_mgr=manager) - _ensure_file_checkpoint( - agent, "write_file", {"path": "/workspace/project/a.txt"}, container_task_id - ) - _assert_container_skipped(manager, container_task_id) - - -@pytest.mark.skipif(os.name == "nt", reason="POSIX absolute container path") -def test_container_backend_file_checkpoint_does_not_snapshot_a_colliding_host_tree( - tmp_path, manager, container_task_id, -): - host_dir = tmp_path / "workspace" / "project" - host_dir.mkdir(parents=True) - (host_dir / "pyproject.toml").write_text("[project]\n", encoding="utf-8") - (host_dir / "a.txt").write_text("host content\n", encoding="utf-8") - _ensure_file_checkpoint( - SimpleNamespace(_checkpoint_mgr=manager), - "write_file", - {"path": str(host_dir / "a.txt")}, - container_task_id, - ) - assert manager.list_checkpoints(str(host_dir)) == [] - _assert_container_skipped(manager, container_task_id) - - -def test_container_backend_destructive_terminal_checkpoint_is_not_taken( - tmp_path, monkeypatch, manager, container_task_id, -): - # Pin the host cwd the branch would snapshot on main to a small tree, not the test process cwd. - host_cwd = tmp_path / "host-cwd" - host_cwd.mkdir() - (host_cwd / "keep.txt").write_text("host", encoding="utf-8") - monkeypatch.setenv("TERMINAL_CWD", str(host_cwd)) - agent = SimpleNamespace( - quiet_mode=True, - tool_progress_callback=None, - tool_start_callback=None, - _checkpoint_mgr=manager, - _touch_activity=lambda *_: None, - ) - ref = _ToolCallRef( - "terminal", - {"command": "rm -f /workspace/project/a.txt"}, - container_task_id, - "call-1", - [], - ) - _begin_tool_execution(agent, ref, None) - _assert_container_skipped(manager, container_task_id) - - -def test_container_backend_post_write_ledger_is_not_recorded( - tmp_path, manager, container_task_id, -): - # A real host file at the container path's spelling: on main the ledger loop hashes it - # on the host and persists an entry, which is exactly the false attribution to prevent. +def test_container_backend_task_leaves_host_store_untouched(tmp_path, manager, container_task_id): + """A container-backed task's paths are container paths: no host snapshot from the file or + destructive-terminal hooks and no host ledger entry, even when an unrelated host tree shares + the spelling. A local task on the same manager still gets its checkpoint (control).""" host_file = tmp_path / "workspace" / "project" / "a.txt" host_file.parent.mkdir(parents=True) - host_file.write_text("after\n", encoding="utf-8") + host_file.write_text("host content\n", encoding="utf-8") path = str(host_file) agent = SimpleNamespace( - _turn_failed_file_mutations={}, - _turn_file_mutation_paths=set(), - _checkpoint_mgr=manager, + _checkpoint_mgr=manager, _turn_failed_file_mutations={}, _turn_file_mutation_paths=set(), + quiet_mode=True, tool_progress_callback=None, tool_start_callback=None, _touch_activity=lambda *_: None, ) + + _ensure_file_checkpoint(agent, "write_file", {"path": path}, container_task_id) + ref = _ToolCallRef("terminal", {"command": f"rm -rf {host_file.parent}"}, container_task_id, "call-1", []) + _begin_tool_execution(agent, ref, None) TurnExplainersMixin._record_file_mutation_result( - agent, - "write_file", - {"path": path, "content": "after\n"}, - json.dumps({"bytes_written": 6, "resolved_path": path}), - False, - task_id=container_task_id, + agent, "write_file", {"path": path, "content": "x"}, + json.dumps({"bytes_written": 1, "resolved_path": path}), False, task_id=container_task_id, ) - assert not (tmp_path / "checkpoints" / "store" / "ledgers").exists() - _assert_container_skipped(manager, container_task_id) + assert manager.list_checkpoints(str(host_file.parent)) == [] + assert not (tmp_path / "checkpoints").exists() + + _ensure_file_checkpoint(agent, "write_file", {"path": path}, "local-task") + assert len(manager.list_checkpoints(str(host_file.parent))) == 1 -def test_backend_reason_follows_the_session_backend(monkeypatch, manager): - """Nothing is remembered: the same manager answers for the backend configured right now, - even after a container-backed mutation went through the checkpoint hook.""" - monkeypatch.setenv("TERMINAL_ENV", "docker") - _ensure_file_checkpoint( - SimpleNamespace(_checkpoint_mgr=manager), "write_file", {"path": "/workspace/project/a.txt"}, "default" - ) - assert "docker" in manager.unsupported_backend_reason() - monkeypatch.setenv("TERMINAL_ENV", "local") - assert manager.unsupported_backend_reason() is None - monkeypatch.setenv("TERMINAL_ENV", "docker") - assert "docker" in manager.unsupported_backend_reason() - - -def test_local_backend_behaviour_unchanged(tmp_path, monkeypatch, manager): - workspace = tmp_path / "local-project" - workspace.mkdir() - (workspace / "pyproject.toml").write_text("[project]\n", encoding="utf-8") - (workspace / "existing.txt").write_text("before\n", encoding="utf-8") - monkeypatch.setenv("TERMINAL_ENV", "local") - monkeypatch.setenv("TERMINAL_CWD", str(workspace)) - _ensure_file_checkpoint( - SimpleNamespace(_checkpoint_mgr=manager), - "write_file", - {"path": "new.txt"}, - "local-checkpoint-task", - ) - assert manager.list_checkpoints(str(workspace)) - - -def test_container_session_rollback_restore_is_refused(tmp_path, monkeypatch, manager, container_task_id, capsys): - """A host checkpoint that predates the container session must not be restored from it.""" +@pytest.mark.asyncio +async def test_container_session_refuses_host_rollback_on_every_surface(tmp_path, monkeypatch, manager, capsys): + """A host checkpoint left by an earlier local session is never restored or diffed from a + container-backed session: CLI /rollback + /diff session, gateway /rollback + /diff session, + and the rollback.restore RPC all answer with the backend reason instead.""" + from gateway import run as gateway_run + from gateway.config import Platform + from gateway.platforms.event import MessageEvent + from gateway.session import SessionSource from hermes_cli.cli_commands_mixin import CLICommandsMixin - - host_dir = tmp_path / "workspace" / "project" - host_dir.mkdir(parents=True) - (host_dir / "a.txt").write_text("before\n", encoding="utf-8") - monkeypatch.setenv("TERMINAL_ENV", "docker") # the configured backend, as the product bridges it - monkeypatch.setenv("TERMINAL_CWD", str(host_dir)) - manager.ensure_checkpoint(str(host_dir), "earlier local session") - assert manager.list_checkpoints(str(host_dir)) - # Fresh session: no mutation has been observed, so only the backend classification can refuse. - - def refuse(*_args, **_kwargs): - raise AssertionError("host restore reached from a container-backed session") - - cli = SimpleNamespace( - _checkpoint_manager=lambda _lines: manager, - _resolve_checkpoint_ref=lambda ref, cps: cps[int(ref) - 1]["hash"], - _rollback_restore=refuse, - _rollback_diff=refuse, - ) - for command in ("/rollback 1 --all", "/rollback diff 1"): - CLICommandsMixin._handle_rollback_command(cli, command) - assert "docker" in capsys.readouterr().out - assert len(manager.list_checkpoints(str(host_dir))) == 1 # no pre-rollback snapshot either - - -def test_container_session_rpc_restore_is_refused(tmp_path, monkeypatch, manager, container_task_id): - import threading - from tui_gateway import server host_dir = tmp_path / "workspace" / "project" host_dir.mkdir(parents=True) (host_dir / "a.txt").write_text("before\n", encoding="utf-8") - monkeypatch.setenv("TERMINAL_ENV", "docker") manager.ensure_checkpoint(str(host_dir), "earlier local session") + monkeypatch.setenv("TERMINAL_ENV", "docker") # the configured backend, as the product bridges it + monkeypatch.setenv("TERMINAL_CWD", str(host_dir)) + + def refuse(*_args, **_kwargs): + raise AssertionError("host checkpoint operation reached from a container-backed session") + + monkeypatch.setattr(manager, "restore", refuse) + monkeypatch.setattr(manager, "diff", refuse) + monkeypatch.setattr(manager, "session_diff", refuse) + + cli = SimpleNamespace( + _checkpoint_manager=lambda _lines: manager, + _resolve_checkpoint_ref=lambda ref, cps: cps[int(ref) - 1]["hash"], + _rollback_restore=refuse, _rollback_diff=refuse, + ) + for command in ("/rollback 1 --all", "/rollback diff 1"): + CLICommandsMixin._handle_rollback_command(cli, command) + assert "docker" in "".join(capsys.readouterr()) + CLICommandsMixin._print_session_diff(cli, str(host_dir), False) + assert "docker" in "".join(capsys.readouterr()) + + runner = object.__new__(gateway_run.GatewayRunner) + runner._checkpoint_manager = lambda: manager + source = SessionSource(platform=Platform.TELEGRAM, user_id="u", chat_id="c", user_name="t", chat_type="dm") + for text in ("/rollback 1", "/rollback 1 --all"): + assert "docker" in await runner._handle_rollback_command(MessageEvent(text=text, source=source)) + assert "docker" in await runner._handle_diff_command(MessageEvent(text="/diff session", source=source)) + listing = await runner._handle_rollback_command(MessageEvent(text="/rollback", source=source)) + assert "docker" in listing and "earlier local session" in listing # listing stays visible + session = { "agent": SimpleNamespace(_checkpoint_mgr=manager), "cwd": str(host_dir), "running": False, "session_key": "container-key", "history": [], "history_lock": threading.Lock(), "history_version": 0, @@ -228,52 +147,13 @@ def test_container_session_rpc_restore_is_refused(tmp_path, monkeypatch, manager resp = server.handle_request( {"id": "1", "method": "rollback.restore", "params": {"session_id": "container-sid", "hash": "1"}} ) - assert resp["result"]["success"] is False - assert "docker" in resp["result"]["error"] - assert len(manager.list_checkpoints(str(host_dir))) == 1 + assert resp["result"]["success"] is False and "docker" in resp["result"]["error"] - -def test_local_session_rollback_restore_still_dispatches(tmp_path, monkeypatch, manager, capsys): - """The other direction: a local session with a host checkpoint restores as before.""" - import threading - - from hermes_cli.cli_commands_mixin import CLICommandsMixin - from tui_gateway import server - - host_dir = tmp_path / "local-project" - host_dir.mkdir() - (host_dir / "a.txt").write_text("before", encoding="utf-8") - monkeypatch.setenv("TERMINAL_ENV", "local") - monkeypatch.setenv("TERMINAL_CWD", str(host_dir)) - manager.ensure_checkpoint(str(host_dir), "earlier local session") - - calls = [] - cli = SimpleNamespace( - _checkpoint_manager=lambda _lines: manager, - _resolve_checkpoint_ref=lambda ref, cps: cps[int(ref) - 1]["hash"], - _rollback_restore=lambda *args: calls.append(args), - ) - CLICommandsMixin._handle_rollback_command(cli, "/rollback 1 --all") - assert len(calls) == 1 and "Checkpoints are not taken" not in capsys.readouterr().out - - session = { - "agent": SimpleNamespace(_checkpoint_mgr=manager), "cwd": str(host_dir), "running": False, - "session_key": "local-key", "history": [], "history_lock": threading.Lock(), "history_version": 0, - } - monkeypatch.setitem(server._sessions, "local-sid", session) + monkeypatch.setenv("TERMINAL_ENV", "local") # control: a local session still dispatches + monkeypatch.setattr(manager, "restore", lambda *a, **k: {"success": True, "restored_to": "x", "reason": "r"}) resp = server.handle_request( - {"id": "1", "method": "rollback.restore", "params": {"session_id": "local-sid", "hash": "1"}} - ) - assert resp["result"]["success"] is True - - # A persistent docker container of the launch profile already occupies the "default" registry - # slot; this local session must still be classified by its own key, not by that cached env. - class FakeDockerEnvironment: - pass - - with _env_lock: - monkeypatch.setitem(_active_environments, "default", FakeDockerEnvironment()) - resp = server.handle_request( - {"id": "2", "method": "rollback.restore", "params": {"session_id": "local-sid", "hash": "1"}} + {"id": "2", "method": "rollback.restore", "params": {"session_id": "container-sid", "hash": "1"}} ) assert resp["result"]["success"] is True + assert (host_dir / "a.txt").read_text(encoding="utf-8") == "before\n" + assert os.environ["TERMINAL_ENV"] == "local" diff --git a/tests/hermes_cli/test_diff_command.py b/tests/hermes_cli/test_diff_command.py index 0652f0f77b..a742999176 100644 --- a/tests/hermes_cli/test_diff_command.py +++ b/tests/hermes_cli/test_diff_command.py @@ -36,6 +36,9 @@ class _Mgr: self._result = result self.calls = [] + def unsupported_backend_reason(self, task_id="default"): + return None + def session_diff(self, cwd): self.calls.append(cwd) return self._result diff --git a/website/docs/user-guide/checkpoints-and-rollback.md b/website/docs/user-guide/checkpoints-and-rollback.md index 299f324623..0809207a86 100644 --- a/website/docs/user-guide/checkpoints-and-rollback.md +++ b/website/docs/user-guide/checkpoints-and-rollback.md @@ -236,7 +236,7 @@ Restore just one file from a checkpoint without affecting the rest of the direct ### Container Backends -With a container terminal backend (`docker`, `singularity`, `modal`, `daytona`, `vercel_sandbox`, or a container plugin), file paths belong to the sandbox rather than the host. Hermes therefore does not take checkpoints or record the agent-write ledger for those paths, and `/rollback` explains the limitation: it still lists existing host checkpoints but refuses diff and restore. Over the gateway RPC, `rollback.list` and `rollback.diff` remain available for inspecting host checkpoints while `rollback.restore` is refused. Local and SSH backends are unaffected. +With a container terminal backend (`docker`, `singularity`, `modal`, `daytona`, `vercel_sandbox`, or a container plugin), file paths belong to the sandbox rather than the host. Hermes therefore does not take checkpoints or record the agent-write ledger for those paths, and `/rollback` explains the limitation: it still lists existing host checkpoints but refuses diff and restore, on the CLI and in messaging-gateway chats alike; `/diff session` answers with the same reason. Over the TUI/Desktop RPC, `rollback.list` and `rollback.diff` remain available for inspecting host checkpoints while `rollback.restore` is refused. Local and SSH backends are unaffected. To point `terminal.cwd` at the container-side view of a mounted directory see [`terminal.docker_mount_cwd_to_workspace`](./configuration.md). - **Git availability** — if `git` is not found on `PATH`, checkpoints are transparently disabled. - **Directory scope** — Hermes skips overly broad directories (root `/`, home `$HOME`).