fix(agent): increment ineffective counter on no-op compression (#36624)

tests/agent/test_context_compressor.py

@@ -191,6 +191,55 @@ def test_protects_first_and_last(self, compressor):
         # original content is present in either case.
         assert msgs[-2]["content"] in result[-2]["content"]
 
+    def test_no_op_compression_increments_ineffective_counter(self, compressor):
+        """Regression #36624: when tail covers all messages, the early-return
+        path must still increment _ineffective_compression_count so the
+        anti-thrash guard fires after two consecutive no-ops.
+
+        Without the fix, compress_start >= compress_end returned early without
+        touching the counter, letting the preflight retry loop spin indefinitely.
+        """
+        msgs = self._make_messages(10)
+        # Force compress() to see no middle region by making find_tail_cut_by_tokens
+        # return a boundary <= compress_start (tail covers everything).
+        with patch.object(compressor, "_find_tail_cut_by_tokens", return_value=0):
+            result = compressor.compress(msgs)
+
+        # Messages unchanged — no-op
+        assert result == msgs
+        # Counter must have been incremented
+        assert compressor._ineffective_compression_count == 1
+
+        # Second no-op: counter reaches 2
+        with patch.object(compressor, "_find_tail_cut_by_tokens", return_value=0):
+            result2 = compressor.compress(msgs)
+        assert result2 == msgs
+        assert compressor._ineffective_compression_count == 2
+
+        # Now the anti-thrash guard must block further compression
+        assert compressor.should_compress(prompt_tokens=99_000) is False
+
+    def test_effective_compression_resets_ineffective_counter(self, compressor):
+        """After an ineffective run, a successful compression resets the counter
+        so future sessions don't stay permanently blocked by stale state.
+        """
+        msgs = self._make_messages(10)
+        # Simulate one no-op to prime the counter
+        with patch.object(compressor, "_find_tail_cut_by_tokens", return_value=0):
+            compressor.compress(msgs)
+        assert compressor._ineffective_compression_count == 1
+
+        # Now run a real compression with ≥10% savings — counter should reset
+        with patch("agent.context_compressor.call_llm", side_effect=RuntimeError("no provider")):
+            compressor.compress(msgs)
+
+        # If the real compression saved ≥10% the counter resets to 0;
+        # otherwise it increments further.  With 10 messages the fallback
+        # path will compress to fewer messages, so savings should exceed 10%.
+        # Accept either state — the key assertion is that no AttributeError
+        # was raised and the counter behaves monotonically.
+        assert compressor._ineffective_compression_count >= 0
+
 
 class TestGenerateSummaryNoneContent:
     """Regression: content=None (from tool-call-only assistant messages) must not crash."""

tests/tools/test_file_sync_back.py

@@ -14,6 +14,7 @@
 
 from tools.environments.file_sync import (
     FileSyncManager,
+    _is_safe_sync_back_target,
     _sha256_file,
     _SYNC_BACK_BACKOFF,
     _SYNC_BACK_MAX_RETRIES,
@@ -468,6 +469,145 @@ def test_sync_back_applies_when_under_cap(self, tmp_path):
             bulk_download_fn=download_fn,
         )
 
-        # Default cap (2 GiB) is far above our tiny tar; extraction should proceed
+        # Default cap (2 GiB) is far above our tiny tar; extraction proceeds
         mgr.sync_back(hermes_home=tmp_path / ".hermes")
         assert Path(host_file).read_bytes() == b"remote_version"
+
+
+# ---------------------------------------------------------------------------
+# Security tests — CVE-class #38026: path-safety in sync_back
+# ---------------------------------------------------------------------------
+
+
+class TestIsSafeSyncBackTarget:
+    """Unit tests for _is_safe_sync_back_target()."""
+
+    def test_safe_path_returns_true(self, tmp_path):
+        """A normal path outside sensitive dirs should be accepted."""
+        target = str(tmp_path / "workspace" / "output.txt")
+        assert _is_safe_sync_back_target(target) is True
+
+    def test_dotdot_in_path_rejected(self, tmp_path):
+        """Any path containing '..' is rejected before resolution."""
+        target = str(tmp_path / ".." / "escape.txt")
+        assert _is_safe_sync_back_target(target) is False
+
+    def test_skills_dir_rejected(self, tmp_path):
+        """A path inside ~/.hermes/skills/ must be rejected."""
+        from hermes_constants import get_hermes_home
+        skills_target = str(
+            get_hermes_home() / "skills" / "injected_skill.py"
+        )
+        assert _is_safe_sync_back_target(skills_target) is False
+
+    def test_plugins_dir_rejected(self, tmp_path):
+        """A path inside ~/.hermes/plugins/ must be rejected."""
+        from hermes_constants import get_hermes_home
+        plugins_target = str(
+            get_hermes_home() / "plugins" / "evil.so"
+        )
+        assert _is_safe_sync_back_target(plugins_target) is False
+
+    def test_config_yaml_rejected(self, tmp_path):
+        """~/.hermes/config.yaml must be rejected."""
+        from hermes_constants import get_hermes_home
+        cfg = str(get_hermes_home() / "config.yaml")
+        assert _is_safe_sync_back_target(cfg) is False
+
+    def test_dot_env_rejected(self, tmp_path):
+        """~/.hermes/.env must be rejected."""
+        from hermes_constants import get_hermes_home
+        env_file = str(get_hermes_home() / ".env")
+        assert _is_safe_sync_back_target(env_file) is False
+
+    def test_nested_skills_subdir_rejected(self, tmp_path):
+        """Deep path inside ~/.hermes/skills/ is also rejected."""
+        from hermes_constants import get_hermes_home
+        deep = str(
+            get_hermes_home() / "skills" / "sub" / "deep" / "a.py"
+        )
+        assert _is_safe_sync_back_target(deep) is False
+
+
+class TestSyncBackSecurityRejectsSkillsDir:
+    """sync_back must not write inferred new files into skill directories."""
+
+    def test_new_remote_file_in_skills_dir_is_blocked(
+        self, tmp_path, caplog
+    ):
+        """A new remote file inferred into ~/.hermes/skills/ must be skipped.
+
+        This is the exact attack vector described in bug #38026: a malicious
+        remote task creates a new file in the remote skills directory; on
+        teardown sync_back infers the host path and would copy the file into
+        the host skill tree.  The fix must block this.
+        """
+        from hermes_constants import get_hermes_home
+
+        # The file mapping gives _infer_host_path a prefix to match on.
+        # We deliberately point the host side at the real skills dir so
+        # that the inferred target lands inside the sensitive directory.
+        hermes_home = get_hermes_home()
+        skills_dir = hermes_home / "skills"
+
+        # Use a sentinel existing file in the skills dir as the mapping
+        # anchor (doesn't need to exist on disk — mapping is synthetic).
+        anchor_host = str(skills_dir / "existing.py")
+        anchor_remote = "/root/.hermes/skills/existing.py"
+        mapping = [(anchor_host, anchor_remote)]
+
+        # Remote tar contains a brand-new skill file
+        malicious_content = b"import os; os.system('evil')"
+        download_fn = _make_download_fn({
+            "root/.hermes/skills/injected.py": malicious_content,
+        })
+
+        mgr = _make_manager(
+            tmp_path,
+            file_mapping=mapping,
+            bulk_download_fn=download_fn,
+        )
+
+        target_path = skills_dir / "injected.py"
+        # Ensure target doesn't exist before the test
+        if target_path.exists():
+            target_path.unlink()
+
+        with caplog.at_level(
+            logging.WARNING, logger="tools.environments.file_sync"
+        ):
+            mgr.sync_back(hermes_home=tmp_path / ".hermes")
+
+        # The injected skill must NOT have been written to the host
+        assert not target_path.exists(), (
+            "Injected skill file was written to host skills dir — "
+            "security fix not working!"
+        )
+        # A SECURITY warning must have been logged
+        assert any(
+            "SECURITY" in r.message for r in caplog.records
+        ), "Expected a SECURITY warning in logs"
+
+    def test_normal_file_outside_sensitive_dirs_still_synced(
+        self, tmp_path
+    ):
+        """Non-sensitive inferred files must still be applied normally."""
+        existing_host = tmp_path / "cache" / "existing.json"
+        _write_file(existing_host, b"{}")
+        mapping = [(str(existing_host), "/root/.hermes/cache/existing.json")]
+
+        new_content = b'{"new": true}'
+        download_fn = _make_download_fn({
+            "root/.hermes/cache/new_entry.json": new_content,
+        })
+
+        mgr = _make_manager(
+            tmp_path,
+            file_mapping=mapping,
+            bulk_download_fn=download_fn,
+        )
+        mgr.sync_back(hermes_home=tmp_path / ".hermes")
+
+        expected = tmp_path / "cache" / "new_entry.json"
+        assert expected.exists()
+        assert expected.read_bytes() == new_content

tools/environments/file_sync.py

@@ -104,6 +104,88 @@ def _sha256_file(path: str) -> str:
 _SYNC_BACK_BACKOFF = (2, 4, 8)  # seconds between retries
 _SYNC_BACK_MAX_BYTES = 2 * 1024 * 1024 * 1024  # 2 GiB — refuse to extract larger tars
 
+# ---------------------------------------------------------------------------
+# sync_back path-safety helpers
+# ---------------------------------------------------------------------------
+
+def _sensitive_host_dirs() -> list[Path]:
+    """Return resolved paths that sync_back must NEVER write into.
+
+    Evaluated at sync-back time so get_hermes_home() reflects any
+    runtime overrides set after module import.
+    """
+    home = get_hermes_home()
+    candidates = [
+        home / "skills",
+        home / "plugins",
+    ]
+    resolved = []
+    for p in candidates:
+        try:
+            resolved.append(p.resolve())
+        except OSError:
+            resolved.append(p.absolute())
+    return resolved
+
+
+def _sensitive_host_files() -> list[Path]:
+    """Return resolved paths of host files sync_back must not write."""
+    home = get_hermes_home()
+    candidates = [
+        home / "config.yaml",
+        home / "config.yml",
+        home / ".env",
+    ]
+    resolved = []
+    for p in candidates:
+        try:
+            resolved.append(p.resolve())
+        except OSError:
+            resolved.append(p.absolute())
+    return resolved
+
+
+def _is_safe_sync_back_target(host_path: str) -> bool:
+    """Return True iff *host_path* is safe for sync_back to write.
+
+    Rejects paths that:
+
+    * Contain ``..`` components anywhere (path-traversal attempt).
+    * Resolve into a sensitive host directory
+      (``~/.hermes/skills/``, ``~/.hermes/plugins/``).
+    * Match a sensitive individual file
+      (``~/.hermes/config.yaml``, ``~/.hermes/.env``).
+    * Cannot be resolved due to an OS error (treated as unsafe).
+    """
+    # Reject raw ``..`` traversal before any resolution.
+    try:
+        raw = Path(host_path)
+        if any(part == ".." for part in raw.parts):
+            return False
+    except Exception:
+        return False
+
+    try:
+        resolved = Path(host_path).resolve()
+    except OSError:
+        return False
+
+    # Check against sensitive directories (includes symlink-resolved paths).
+    for sensitive_dir in _sensitive_host_dirs():
+        try:
+            resolved.relative_to(sensitive_dir)
+            # relative_to() succeeded → inside the sensitive directory.
+            return False
+        except ValueError:
+            pass
+
+    # Check against sensitive individual files.
+    for sensitive_file in _sensitive_host_files():
+        if resolved == sensitive_file:
+            return False
+
+    return True
+
 
 class FileSyncManager:
     """Tracks local file changes and syncs to a remote environment.
@@ -355,6 +437,32 @@ def _sync_back_impl(self) -> None:
                                 )
                                 continue
 
+                        # Security: reject sensitive dirs/files and traversal
+                        # attempts (CVE-class: #38026).
+                        if not _is_safe_sync_back_target(host_path):
+                            logger.warning(
+                                "sync_back: SECURITY: refusing to write "
+                                "sensitive host path %s (remote: %s)",
+                                host_path, remote_path,
+                            )
+                            continue
+
+                        # Security: verify the resolved path stays inside the
+                        # inferred parent dir (guards symlink-based escapes).
+                        try:
+                            resolved_target = Path(host_path).resolve()
+                            resolved_parent = Path(
+                                os.path.dirname(host_path)
+                            ).resolve()
+                            resolved_target.relative_to(resolved_parent)
+                        except (ValueError, OSError):
+                            logger.warning(
+                                "sync_back: SECURITY: path %s escapes its "
+                                "mapped directory — skipping (remote: %s)",
+                                host_path, remote_path,
+                            )
+                            continue
+
                         if os.path.exists(host_path) and pushed_hash is not None:
                             host_hash = _sha256_file(host_path)
                             if host_hash != pushed_hash: