Merge branch 'main' of github.com:NousResearch/hermes-agent into feat/ink-refactor

tests/tools/test_browser_orphan_reaper.py

@@ -0,0 +1,158 @@
+"""Tests for _reap_orphaned_browser_sessions() — kills orphaned agent-browser
+daemons whose Python parent exited without cleaning up."""
+
+import os
+import signal
+import textwrap
+from pathlib import Path
+from unittest.mock import patch, MagicMock
+
+import pytest
+
+
+@pytest.fixture
+def fake_tmpdir(tmp_path):
+    """Patch _socket_safe_tmpdir to return a temp dir we control."""
+    with patch("tools.browser_tool._socket_safe_tmpdir", return_value=str(tmp_path)):
+        yield tmp_path
+
+
+@pytest.fixture(autouse=True)
+def _isolate_sessions():
+    """Ensure _active_sessions is empty for each test."""
+    import tools.browser_tool as bt
+    orig = bt._active_sessions.copy()
+    bt._active_sessions.clear()
+    yield
+    bt._active_sessions.clear()
+    bt._active_sessions.update(orig)
+
+
+def _make_socket_dir(tmpdir, session_name, pid=None):
+    """Create a fake agent-browser socket directory with optional PID file."""
+    d = tmpdir / f"agent-browser-{session_name}"
+    d.mkdir()
+    if pid is not None:
+        (d / f"{session_name}.pid").write_text(str(pid))
+    return d
+
+
+class TestReapOrphanedBrowserSessions:
+    """Tests for the orphan reaper function."""
+
+    def test_no_socket_dirs_is_noop(self, fake_tmpdir):
+        """No socket dirs => nothing happens, no errors."""
+        from tools.browser_tool import _reap_orphaned_browser_sessions
+        _reap_orphaned_browser_sessions()  # should not raise
+
+    def test_stale_dir_without_pid_file_is_removed(self, fake_tmpdir):
+        """Socket dir with no PID file is cleaned up."""
+        from tools.browser_tool import _reap_orphaned_browser_sessions
+        d = _make_socket_dir(fake_tmpdir, "h_abc1234567")
+        assert d.exists()
+        _reap_orphaned_browser_sessions()
+        assert not d.exists()
+
+    def test_stale_dir_with_dead_pid_is_removed(self, fake_tmpdir):
+        """Socket dir whose daemon PID is dead gets cleaned up."""
+        from tools.browser_tool import _reap_orphaned_browser_sessions
+        d = _make_socket_dir(fake_tmpdir, "h_dead123456", pid=999999999)
+        assert d.exists()
+        _reap_orphaned_browser_sessions()
+        assert not d.exists()
+
+    def test_orphaned_alive_daemon_is_killed(self, fake_tmpdir):
+        """Alive daemon not tracked by _active_sessions gets SIGTERM."""
+        from tools.browser_tool import _reap_orphaned_browser_sessions
+
+        d = _make_socket_dir(fake_tmpdir, "h_orphan12345", pid=12345)
+
+        kill_calls = []
+        original_kill = os.kill
+
+        def mock_kill(pid, sig):
+            kill_calls.append((pid, sig))
+            if sig == 0:
+                return  # pretend process exists
+            # Don't actually kill anything
+
+        with patch("os.kill", side_effect=mock_kill):
+            _reap_orphaned_browser_sessions()
+
+        # Should have checked existence (sig 0) then killed (SIGTERM)
+        assert (12345, 0) in kill_calls
+        assert (12345, signal.SIGTERM) in kill_calls
+
+    def test_tracked_session_is_not_reaped(self, fake_tmpdir):
+        """Sessions tracked in _active_sessions are left alone."""
+        import tools.browser_tool as bt
+        from tools.browser_tool import _reap_orphaned_browser_sessions
+
+        session_name = "h_tracked1234"
+        d = _make_socket_dir(fake_tmpdir, session_name, pid=12345)
+
+        # Register the session as actively tracked
+        bt._active_sessions["some_task"] = {"session_name": session_name}
+
+        kill_calls = []
+
+        def mock_kill(pid, sig):
+            kill_calls.append((pid, sig))
+
+        with patch("os.kill", side_effect=mock_kill):
+            _reap_orphaned_browser_sessions()
+
+        # Should NOT have tried to kill anything
+        assert len(kill_calls) == 0
+        # Dir should still exist
+        assert d.exists()
+
+    def test_permission_error_on_kill_check_skips(self, fake_tmpdir):
+        """If we can't check the PID (PermissionError), skip it."""
+        from tools.browser_tool import _reap_orphaned_browser_sessions
+
+        d = _make_socket_dir(fake_tmpdir, "h_perm1234567", pid=12345)
+
+        def mock_kill(pid, sig):
+            if sig == 0:
+                raise PermissionError("not our process")
+
+        with patch("os.kill", side_effect=mock_kill):
+            _reap_orphaned_browser_sessions()
+
+        # Dir should still exist (we didn't touch someone else's process)
+        assert d.exists()
+
+    def test_cdp_sessions_are_also_reaped(self, fake_tmpdir):
+        """CDP sessions (cdp_ prefix) are also scanned."""
+        from tools.browser_tool import _reap_orphaned_browser_sessions
+
+        d = _make_socket_dir(fake_tmpdir, "cdp_abc1234567")
+        assert d.exists()
+        _reap_orphaned_browser_sessions()
+        # No PID file → cleaned up
+        assert not d.exists()
+
+    def test_non_hermes_dirs_are_ignored(self, fake_tmpdir):
+        """Socket dirs that don't match our naming pattern are left alone."""
+        from tools.browser_tool import _reap_orphaned_browser_sessions
+
+        # Create a dir that doesn't match h_* or cdp_* pattern
+        d = fake_tmpdir / "agent-browser-other_session"
+        d.mkdir()
+        (d / "other_session.pid").write_text("12345")
+
+        _reap_orphaned_browser_sessions()
+
+        # Should NOT be touched
+        assert d.exists()
+
+    def test_corrupt_pid_file_is_cleaned(self, fake_tmpdir):
+        """PID file with non-integer content is cleaned up."""
+        from tools.browser_tool import _reap_orphaned_browser_sessions
+
+        d = _make_socket_dir(fake_tmpdir, "h_corrupt1234")
+        (d / "h_corrupt1234.pid").write_text("not-a-number")
+
+        _reap_orphaned_browser_sessions()
+        assert not d.exists()

tests/tools/test_checkpoint_manager.py

@@ -1,9 +1,6 @@
 """Tests for tools/checkpoint_manager.py — CheckpointManager."""
 
 import logging
-import os
-import json
-import shutil
 import subprocess
 import pytest
 from pathlib import Path
@@ -42,6 +39,19 @@ def checkpoint_base(tmp_path):
     return tmp_path / "checkpoints"
 
 
+@pytest.fixture()
+def fake_home(tmp_path, monkeypatch):
+    """Set a deterministic fake home for expanduser/path-home behavior."""
+    home = tmp_path / "home"
+    home.mkdir()
+    monkeypatch.setenv("HOME", str(home))
+    monkeypatch.setenv("USERPROFILE", str(home))
+    monkeypatch.delenv("HOMEDRIVE", raising=False)
+    monkeypatch.delenv("HOMEPATH", raising=False)
+    monkeypatch.setattr(Path, "home", classmethod(lambda cls: home))
+    return home
+
+
 @pytest.fixture()
 def mgr(work_dir, checkpoint_base, monkeypatch):
     """CheckpointManager with redirected checkpoint base."""
@@ -78,6 +88,16 @@ class TestShadowRepoPath:
         p = _shadow_repo_path(str(work_dir))
         assert str(p).startswith(str(checkpoint_base))
 
+    def test_tilde_and_expanded_home_share_shadow_repo(self, fake_home, checkpoint_base, monkeypatch):
+        monkeypatch.setattr("tools.checkpoint_manager.CHECKPOINT_BASE", checkpoint_base)
+        project = fake_home / "project"
+        project.mkdir()
+
+        tilde_path = f"~/{project.name}"
+        expanded_path = str(project)
+
+        assert _shadow_repo_path(tilde_path) == _shadow_repo_path(expanded_path)
+
 
 # =========================================================================
 # Shadow repo init
@@ -221,6 +241,20 @@ class TestListCheckpoints:
         assert result[0]["reason"] == "third"
         assert result[2]["reason"] == "first"
 
+    def test_tilde_path_lists_same_checkpoints_as_expanded_path(self, checkpoint_base, fake_home, monkeypatch):
+        monkeypatch.setattr("tools.checkpoint_manager.CHECKPOINT_BASE", checkpoint_base)
+        mgr = CheckpointManager(enabled=True, max_snapshots=50)
+        project = fake_home / "project"
+        project.mkdir()
+        (project / "main.py").write_text("v1\n")
+
+        tilde_path = f"~/{project.name}"
+        assert mgr.ensure_checkpoint(tilde_path, "initial") is True
+
+        listed = mgr.list_checkpoints(str(project))
+        assert len(listed) == 1
+        assert listed[0]["reason"] == "initial"
+
 
 # =========================================================================
 # CheckpointManager — restoring
@@ -271,6 +305,28 @@ class TestRestore:
         assert len(all_cps) >= 2
         assert "pre-rollback" in all_cps[0]["reason"]
 
+    def test_tilde_path_supports_diff_and_restore_flow(self, checkpoint_base, fake_home, monkeypatch):
+        monkeypatch.setattr("tools.checkpoint_manager.CHECKPOINT_BASE", checkpoint_base)
+        mgr = CheckpointManager(enabled=True, max_snapshots=50)
+        project = fake_home / "project"
+        project.mkdir()
+        file_path = project / "main.py"
+        file_path.write_text("original\n")
+
+        tilde_path = f"~/{project.name}"
+        assert mgr.ensure_checkpoint(tilde_path, "initial") is True
+        mgr.new_turn()
+
+        file_path.write_text("changed\n")
+        checkpoints = mgr.list_checkpoints(str(project))
+        diff_result = mgr.diff(tilde_path, checkpoints[0]["hash"])
+        assert diff_result["success"] is True
+        assert "main.py" in diff_result["diff"]
+
+        restore_result = mgr.restore(tilde_path, checkpoints[0]["hash"])
+        assert restore_result["success"] is True
+        assert file_path.read_text() == "original\n"
+
 
 # =========================================================================
 # CheckpointManager — working dir resolution
@@ -310,6 +366,19 @@ class TestWorkingDirResolution:
         result = mgr.get_working_dir_for_path(str(filepath))
         assert result == str(filepath.parent)
 
+    def test_resolves_tilde_path_to_project_root(self, fake_home):
+        mgr = CheckpointManager(enabled=True)
+        project = fake_home / "myproject"
+        project.mkdir()
+        (project / "pyproject.toml").write_text("[project]\n")
+        subdir = project / "src"
+        subdir.mkdir()
+        filepath = subdir / "main.py"
+        filepath.write_text("x\n")
+
+        result = mgr.get_working_dir_for_path(f"~/{project.name}/src/main.py")
+        assert result == str(project)
+
 
 # =========================================================================
 # Git env isolation
@@ -333,6 +402,14 @@ class TestGitEnvIsolation:
         env = _git_env(shadow, str(tmp_path))
         assert "GIT_INDEX_FILE" not in env
 
+    def test_expands_tilde_in_work_tree(self, fake_home, tmp_path):
+        shadow = tmp_path / "shadow"
+        work = fake_home / "work"
+        work.mkdir()
+
+        env = _git_env(shadow, f"~/{work.name}")
+        assert env["GIT_WORK_TREE"] == str(work.resolve())
+
 
 # =========================================================================
 # format_checkpoint_list
@@ -384,6 +461,8 @@ class TestErrorResilience:
         assert result is False
 
     def test_run_git_allows_expected_nonzero_without_error_log(self, tmp_path, caplog):
+        work = tmp_path / "work"
+        work.mkdir()
         completed = subprocess.CompletedProcess(
             args=["git", "diff", "--cached", "--quiet"],
             returncode=1,
@@ -395,7 +474,7 @@ class TestErrorResilience:
                 ok, stdout, stderr = _run_git(
                     ["diff", "--cached", "--quiet"],
                     tmp_path / "shadow",
-                    str(tmp_path / "work"),
+                    str(work),
                     allowed_returncodes={1},
                 )
         assert ok is False
@@ -403,6 +482,38 @@ class TestErrorResilience:
         assert stderr == ""
         assert not caplog.records
 
+    def test_run_git_invalid_working_dir_reports_path_error(self, tmp_path, caplog):
+        missing = tmp_path / "missing"
+        with caplog.at_level(logging.ERROR, logger="tools.checkpoint_manager"):
+            ok, stdout, stderr = _run_git(
+                ["status"],
+                tmp_path / "shadow",
+                str(missing),
+            )
+        assert ok is False
+        assert stdout == ""
+        assert "working directory not found" in stderr
+        assert not any("Git executable not found" in r.getMessage() for r in caplog.records)
+
+    def test_run_git_missing_git_reports_git_not_found(self, tmp_path, monkeypatch, caplog):
+        work = tmp_path / "work"
+        work.mkdir()
+
+        def raise_missing_git(*args, **kwargs):
+            raise FileNotFoundError(2, "No such file or directory", "git")
+
+        monkeypatch.setattr("tools.checkpoint_manager.subprocess.run", raise_missing_git)
+        with caplog.at_level(logging.ERROR, logger="tools.checkpoint_manager"):
+            ok, stdout, stderr = _run_git(
+                ["status"],
+                tmp_path / "shadow",
+                str(work),
+            )
+        assert ok is False
+        assert stdout == ""
+        assert stderr == "git not found"
+        assert any("Git executable not found" in r.getMessage() for r in caplog.records)
+
     def test_checkpoint_failure_does_not_raise(self, mgr, work_dir, monkeypatch):
         """Checkpoint failures should never raise — they're silently logged."""
         def broken_run_git(*args, **kwargs):
@@ -411,3 +522,68 @@ class TestErrorResilience:
         # Should not raise
         result = mgr.ensure_checkpoint(str(work_dir), "test")
         assert result is False
+
+
+# =========================================================================
+# Security / Input validation
+# =========================================================================
+
+class TestSecurity:
+    def test_restore_rejects_argument_injection(self, mgr, work_dir):
+        mgr.ensure_checkpoint(str(work_dir), "initial")
+        # Try to pass a git flag as a commit hash
+        result = mgr.restore(str(work_dir), "--patch")
+        assert result["success"] is False
+        assert "Invalid commit hash" in result["error"]
+        assert "must not start with '-'" in result["error"]
+        
+        result = mgr.restore(str(work_dir), "-p")
+        assert result["success"] is False
+        assert "Invalid commit hash" in result["error"]
+        
+    def test_restore_rejects_invalid_hex_chars(self, mgr, work_dir):
+        mgr.ensure_checkpoint(str(work_dir), "initial")
+        # Git hashes should not contain characters like ;, &, |
+        result = mgr.restore(str(work_dir), "abc; rm -rf /")
+        assert result["success"] is False
+        assert "expected 4-64 hex characters" in result["error"]
+        
+        result = mgr.diff(str(work_dir), "abc&def")
+        assert result["success"] is False
+        assert "expected 4-64 hex characters" in result["error"]
+
+    def test_restore_rejects_path_traversal(self, mgr, work_dir):
+        mgr.ensure_checkpoint(str(work_dir), "initial")
+        # Real commit hash but malicious path
+        checkpoints = mgr.list_checkpoints(str(work_dir))
+        target_hash = checkpoints[0]["hash"]
+        
+        # Absolute path outside
+        result = mgr.restore(str(work_dir), target_hash, file_path="/etc/passwd")
+        assert result["success"] is False
+        assert "got absolute path" in result["error"]
+        
+        # Relative traversal outside path
+        result = mgr.restore(str(work_dir), target_hash, file_path="../outside_file.txt")
+        assert result["success"] is False
+        assert "escapes the working directory" in result["error"]
+
+    def test_restore_accepts_valid_file_path(self, mgr, work_dir):
+        mgr.ensure_checkpoint(str(work_dir), "initial")
+        checkpoints = mgr.list_checkpoints(str(work_dir))
+        target_hash = checkpoints[0]["hash"]
+        
+        # Valid path inside directory
+        result = mgr.restore(str(work_dir), target_hash, file_path="main.py")
+        assert result["success"] is True
+        
+        # Another valid path with subdirectories
+        (work_dir / "subdir").mkdir()
+        (work_dir / "subdir" / "test.txt").write_text("hello")
+        mgr.new_turn()
+        mgr.ensure_checkpoint(str(work_dir), "second")
+        checkpoints = mgr.list_checkpoints(str(work_dir))
+        target_hash = checkpoints[0]["hash"]
+        
+        result = mgr.restore(str(work_dir), target_hash, file_path="subdir/test.txt")
+        assert result["success"] is True

tests/tools/test_code_execution.py

@@ -780,14 +780,18 @@ class TestLoadConfig(unittest.TestCase):
 @unittest.skipIf(sys.platform == "win32", "UDS not available on Windows")
 class TestInterruptHandling(unittest.TestCase):
     def test_interrupt_event_stops_execution(self):
-        """When _interrupt_event is set, execute_code should stop the script."""
+        """When interrupt is set for the execution thread, execute_code should stop."""
         code = "import time; time.sleep(60); print('should not reach')"
+        from tools.interrupt import set_interrupt
+
+        # Capture the main thread ID so we can target the interrupt correctly.
+        # execute_code runs in the current thread; set_interrupt needs its ID.
+        main_tid = threading.current_thread().ident
 
         def set_interrupt_after_delay():
             import time as _t
             _t.sleep(1)
-            from tools.terminal_tool import _interrupt_event
-            _interrupt_event.set()
+            set_interrupt(True, main_tid)
 
         t = threading.Thread(target=set_interrupt_after_delay, daemon=True)
         t.start()
@@ -804,8 +808,7 @@ class TestInterruptHandling(unittest.TestCase):
             self.assertEqual(result["status"], "interrupted")
             self.assertIn("interrupted", result["output"])
         finally:
-            from tools.terminal_tool import _interrupt_event
-            _interrupt_event.clear()
+            set_interrupt(False, main_tid)
             t.join(timeout=3)
 
 

tests/tools/test_notify_on_complete.py

@@ -227,6 +227,8 @@ class TestCheckpointNotify:
             "session_key": "sk1",
             "watcher_platform": "telegram",
             "watcher_chat_id": "123",
+            "watcher_user_id": "u123",
+            "watcher_user_name": "alice",
             "watcher_thread_id": "42",
             "watcher_interval": 5,
             "notify_on_complete": True,
@@ -236,6 +238,8 @@ class TestCheckpointNotify:
             assert recovered == 1
             assert len(registry.pending_watchers) == 1
             assert registry.pending_watchers[0]["notify_on_complete"] is True
+            assert registry.pending_watchers[0]["user_id"] == "u123"
+            assert registry.pending_watchers[0]["user_name"] == "alice"
 
     def test_recover_defaults_false(self, registry, tmp_path):
         """Old checkpoint entries without the field default to False."""

tests/tools/test_process_registry.py

@@ -438,6 +438,8 @@ class TestCheckpoint:
             s = _make_session()
             s.watcher_platform = "telegram"
             s.watcher_chat_id = "999"
+            s.watcher_user_id = "u123"
+            s.watcher_user_name = "alice"
             s.watcher_thread_id = "42"
             s.watcher_interval = 60
             registry._running[s.id] = s
@@ -447,6 +449,8 @@ class TestCheckpoint:
             assert len(data) == 1
             assert data[0]["watcher_platform"] == "telegram"
             assert data[0]["watcher_chat_id"] == "999"
+            assert data[0]["watcher_user_id"] == "u123"
+            assert data[0]["watcher_user_name"] == "alice"
             assert data[0]["watcher_thread_id"] == "42"
             assert data[0]["watcher_interval"] == 60
 
@@ -460,6 +464,8 @@ class TestCheckpoint:
             "session_key": "sk1",
             "watcher_platform": "telegram",
             "watcher_chat_id": "123",
+            "watcher_user_id": "u123",
+            "watcher_user_name": "alice",
             "watcher_thread_id": "42",
             "watcher_interval": 60,
         }]))
@@ -471,6 +477,8 @@ class TestCheckpoint:
             assert w["session_id"] == "proc_live"
             assert w["platform"] == "telegram"
             assert w["chat_id"] == "123"
+            assert w["user_id"] == "u123"
+            assert w["user_name"] == "alice"
             assert w["thread_id"] == "42"
             assert w["check_interval"] == 60
 

tests/tools/test_skill_manager_tool.py

@@ -348,7 +348,7 @@ word word
             result = _patch_skill("my-skill", "old text", "new text", file_path="references/evil.md")
 
         assert result["success"] is False
-        assert "boundary" in result["error"].lower()
+        assert "escapes" in result["error"].lower()
         assert outside_file.read_text() == "old text here"
 
 
@@ -412,7 +412,7 @@ class TestWriteFile:
             result = _write_file("my-skill", "references/escape/owned.md", "malicious")
 
         assert result["success"] is False
-        assert "boundary" in result["error"].lower()
+        assert "escapes" in result["error"].lower()
         assert not (outside_dir / "owned.md").exists()
 
 
@@ -449,7 +449,7 @@ class TestRemoveFile:
             result = _remove_file("my-skill", "references/escape/keep.txt")
 
         assert result["success"] is False
-        assert "boundary" in result["error"].lower()
+        assert "escapes" in result["error"].lower()
         assert outside_file.exists()
 
 

tests/tools/test_tool_result_storage.py

@@ -124,6 +124,34 @@ class TestWriteToSandbox:
         cmd = env.execute.call_args[0][0]
         assert "mkdir -p /data/data/com.termux/files/usr/tmp/hermes-results" in cmd
 
+    def test_path_with_spaces_is_quoted(self):
+        env = MagicMock()
+        env.execute.return_value = {"output": "", "returncode": 0}
+        remote_path = "/tmp/hermes results/abc file.txt"
+        _write_to_sandbox("content", remote_path, env)
+        cmd = env.execute.call_args[0][0]
+        assert "'/tmp/hermes results'" in cmd
+        assert "'/tmp/hermes results/abc file.txt'" in cmd
+
+    def test_shell_metacharacters_neutralized(self):
+        """Paths with shell metacharacters must be quoted to prevent injection."""
+        env = MagicMock()
+        env.execute.return_value = {"output": "", "returncode": 0}
+        malicious_path = "/tmp/hermes-results/$(whoami).txt"
+        _write_to_sandbox("content", malicious_path, env)
+        cmd = env.execute.call_args[0][0]
+        # The $() must not appear unquoted — shlex.quote wraps it
+        assert "'/tmp/hermes-results/$(whoami).txt'" in cmd
+
+    def test_semicolon_injection_neutralized(self):
+        env = MagicMock()
+        env.execute.return_value = {"output": "", "returncode": 0}
+        malicious_path = "/tmp/x; rm -rf /; echo .txt"
+        _write_to_sandbox("content", malicious_path, env)
+        cmd = env.execute.call_args[0][0]
+        # The semicolons must be inside quotes, not acting as command separators
+        assert "'/tmp/x; rm -rf /; echo .txt'" in cmd
+
 
 class TestResolveStorageDir:
     def test_defaults_to_storage_dir_without_env(self):