268 lines
10 KiB
Python
268 lines
10 KiB
Python
"""Tests that the background review agent restricts tools at runtime, not at schema time.
|
|
|
|
Regression coverage for issue #15204 (the background skill-review agent must
|
|
not perform non-skill side effects like terminal, send_message, delegate_task)
|
|
combined with issue #25322 / PR #17276 (the review fork must hit the parent's
|
|
Anthropic/OpenRouter prefix cache).
|
|
|
|
Reconciling the two: the fork now inherits the parent's full ``tools`` schema
|
|
so the cache-key matches, and enforces the memory+skills restriction at
|
|
runtime via a thread-local whitelist on the existing
|
|
``get_pre_tool_call_block_message`` gate. Safety is preserved mechanically
|
|
(any non-whitelisted dispatch is blocked) without the schema-level narrowing
|
|
that caused the prefix-cache miss.
|
|
"""
|
|
|
|
from unittest.mock import patch
|
|
|
|
|
|
def _make_agent_stub(agent_cls):
|
|
"""Create a minimal AIAgent-like object with just enough state for _spawn_background_review."""
|
|
agent = object.__new__(agent_cls)
|
|
agent.model = "test-model"
|
|
agent.platform = "test"
|
|
agent.provider = "openai"
|
|
agent.session_id = "sess-123"
|
|
agent.quiet_mode = True
|
|
agent._memory_store = None
|
|
agent._memory_enabled = True
|
|
agent._user_profile_enabled = False
|
|
agent._memory_nudge_interval = 5
|
|
agent._skill_nudge_interval = 5
|
|
agent.background_review_callback = None
|
|
agent.status_callback = None
|
|
agent._cached_system_prompt = None
|
|
import datetime as _dt
|
|
agent.session_start = _dt.datetime(2026, 1, 1, 12, 0, 0)
|
|
agent._MEMORY_REVIEW_PROMPT = "review memory"
|
|
agent._SKILL_REVIEW_PROMPT = "review skills"
|
|
agent._COMBINED_REVIEW_PROMPT = "review both"
|
|
# Non-None so the test catches a missing-kwarg regression.
|
|
agent.enabled_toolsets = ["memory", "skills", "terminal"]
|
|
agent.disabled_toolsets = ["spotify", "feishu_doc"]
|
|
return agent
|
|
|
|
|
|
class _SyncThread:
|
|
"""Drop-in replacement for threading.Thread that runs the target inline."""
|
|
|
|
def __init__(self, *, target=None, daemon=None, name=None):
|
|
self._target = target
|
|
|
|
def start(self):
|
|
if self._target:
|
|
self._target()
|
|
|
|
|
|
def test_background_review_matches_parent_toolset_config():
|
|
"""Fork must receive parent's toolset config so ``tools[]`` cache key matches."""
|
|
import run_agent
|
|
|
|
agent = _make_agent_stub(run_agent.AIAgent)
|
|
captured = {}
|
|
|
|
def _capture_init(self, *args, **kwargs):
|
|
captured["enabled_toolsets"] = kwargs.get("enabled_toolsets", "UNSET")
|
|
captured["disabled_toolsets"] = kwargs.get("disabled_toolsets", "UNSET")
|
|
raise RuntimeError("stop after capturing init args")
|
|
|
|
with patch.object(run_agent.AIAgent, "__init__", _capture_init), \
|
|
patch("threading.Thread", _SyncThread):
|
|
agent._spawn_background_review(
|
|
messages_snapshot=[],
|
|
review_memory=True,
|
|
review_skills=False,
|
|
)
|
|
|
|
assert "enabled_toolsets" in captured, "AIAgent.__init__ was not called"
|
|
assert captured["enabled_toolsets"] == agent.enabled_toolsets, (
|
|
f"enabled_toolsets mismatch: {captured['enabled_toolsets']!r} "
|
|
f"vs expected {agent.enabled_toolsets!r}"
|
|
)
|
|
assert captured["disabled_toolsets"] == agent.disabled_toolsets, (
|
|
f"disabled_toolsets mismatch: {captured['disabled_toolsets']!r} "
|
|
f"vs expected {agent.disabled_toolsets!r}"
|
|
)
|
|
|
|
|
|
def test_background_review_installs_thread_local_whitelist():
|
|
"""The review fork must install a memory/skills-only thread-local whitelist.
|
|
|
|
The schema-level toolset narrowing was lifted (for prefix-cache parity),
|
|
so #15204's safety contract now relies on the runtime whitelist gate to
|
|
deny terminal/send_message/delegate_task at dispatch time. Verify the
|
|
whitelist is set with exactly the memory+skills tool names.
|
|
"""
|
|
import run_agent
|
|
from hermes_cli import plugins as _plugins
|
|
|
|
captured = {}
|
|
|
|
def _capture_whitelist(whitelist, deny_msg_fmt=None):
|
|
captured["whitelist"] = set(whitelist)
|
|
captured["deny_msg_fmt"] = deny_msg_fmt
|
|
# Stop here — we just want to see what gets installed.
|
|
raise RuntimeError("stop after capturing whitelist")
|
|
|
|
agent = _make_agent_stub(run_agent.AIAgent)
|
|
|
|
def _no_init(self, *args, **kwargs):
|
|
# Don't crash AIAgent.__init__; let execution flow reach
|
|
# set_thread_tool_whitelist.
|
|
return None
|
|
|
|
with patch.object(run_agent.AIAgent, "__init__", _no_init), \
|
|
patch.object(_plugins, "set_thread_tool_whitelist", _capture_whitelist), \
|
|
patch("threading.Thread", _SyncThread):
|
|
agent._spawn_background_review(
|
|
messages_snapshot=[],
|
|
review_memory=True,
|
|
review_skills=False,
|
|
)
|
|
|
|
assert "whitelist" in captured, "set_thread_tool_whitelist was not called"
|
|
whitelist = captured["whitelist"]
|
|
# memory + skills tools must be allowed
|
|
assert "memory" in whitelist
|
|
assert "skill_manage" in whitelist
|
|
assert "skill_view" in whitelist
|
|
assert "skills_list" in whitelist
|
|
# read-only file tools are allowed too (#61521): the model reaches for
|
|
# read_file to inspect a skill before patching; denying it caused a
|
|
# per-review denial storm that starved the self-improvement loop.
|
|
assert "read_file" in whitelist
|
|
assert "search_files" in whitelist
|
|
# write/dangerous tools must NOT be in the whitelist
|
|
assert "write_file" not in whitelist
|
|
assert "patch" not in whitelist
|
|
assert "terminal" not in whitelist
|
|
assert "send_message" not in whitelist
|
|
assert "delegate_task" not in whitelist
|
|
assert "web_search" not in whitelist
|
|
assert "execute_code" not in whitelist
|
|
# The deny message must name the correct substitutes so a single denial
|
|
# redirects the model instead of a 142-denial storm (#61521).
|
|
deny = captured.get("deny_msg_fmt") or ""
|
|
assert "skill_manage" in deny
|
|
assert "skill_view" in deny
|
|
|
|
|
|
def test_read_file_registers_background_review_read_mark(tmp_path):
|
|
"""read_file inside a review fork must satisfy the read-before-write guard.
|
|
|
|
The whitelist now allows read_file; without this mark, the model would
|
|
read SKILL.md via read_file and still get "content has not been loaded
|
|
in this review turn" on the follow-up skill_manage patch (#61521).
|
|
"""
|
|
from tools.file_tools import read_file_tool
|
|
from tools.skill_manager_tool import (
|
|
_background_review_has_read,
|
|
_reset_background_review_read_marks,
|
|
)
|
|
from tools.skill_provenance import (
|
|
BACKGROUND_REVIEW,
|
|
reset_current_write_origin,
|
|
set_current_write_origin,
|
|
)
|
|
|
|
target = tmp_path / "SKILL.md"
|
|
target.write_text("---\nname: t\n---\nbody\n")
|
|
|
|
token = set_current_write_origin(BACKGROUND_REVIEW)
|
|
try:
|
|
_reset_background_review_read_marks()
|
|
assert not _background_review_has_read(target)
|
|
out = read_file_tool(str(target), task_id="bg-review-test")
|
|
assert "body" in out
|
|
assert _background_review_has_read(target), (
|
|
"full read_file inside a review fork must register with the "
|
|
"read-before-write guard"
|
|
)
|
|
|
|
# A partial read must NOT satisfy the guard.
|
|
_reset_background_review_read_marks()
|
|
read_file_tool(str(target), offset=2, task_id="bg-review-test2")
|
|
assert not _background_review_has_read(target)
|
|
finally:
|
|
reset_current_write_origin(token)
|
|
|
|
|
|
def test_read_file_outside_review_does_not_mark(tmp_path):
|
|
"""Foreground reads must not populate the review-fork read set."""
|
|
from tools.file_tools import read_file_tool
|
|
from tools.skill_manager_tool import (
|
|
_background_review_has_read,
|
|
_reset_background_review_read_marks,
|
|
)
|
|
|
|
target = tmp_path / "SKILL.md"
|
|
target.write_text("content\n")
|
|
_reset_background_review_read_marks()
|
|
read_file_tool(str(target), task_id="fg-test")
|
|
assert not _background_review_has_read(target)
|
|
|
|
|
|
def test_background_review_whitelist_includes_configured_extra_tools(
|
|
tmp_path, monkeypatch
|
|
):
|
|
"""A profile may opt a specific proposal tool into background review.
|
|
|
|
The review fork inherits the parent's full tool schema for cache parity,
|
|
but runtime dispatch remains denied unless the tool is also present in the
|
|
thread-local whitelist. This config hook lets profiles grant a narrowly
|
|
scoped, human-gated proposal tool without enabling unrelated side effects.
|
|
"""
|
|
hermes_home = tmp_path / ".hermes"
|
|
hermes_home.mkdir()
|
|
(hermes_home / "config.yaml").write_text(
|
|
"auxiliary:\n"
|
|
" background_review:\n"
|
|
" extra_tools:\n"
|
|
" - propose_shared_memory\n",
|
|
encoding="utf-8",
|
|
)
|
|
monkeypatch.setenv("HERMES_HOME", str(hermes_home))
|
|
|
|
import run_agent
|
|
from hermes_cli import config as config_module
|
|
from hermes_cli import plugins as _plugins
|
|
|
|
config_module._LOAD_CONFIG_CACHE.clear()
|
|
config_module._RAW_CONFIG_CACHE.clear()
|
|
|
|
captured = {}
|
|
|
|
def _capture_whitelist(whitelist, deny_msg_fmt=None):
|
|
captured["whitelist"] = set(whitelist)
|
|
|
|
def _capture_run_conversation(self, *, user_message, **kwargs):
|
|
captured["review_prompt"] = user_message
|
|
return {"final_response": "Nothing to save."}
|
|
|
|
agent = _make_agent_stub(run_agent.AIAgent)
|
|
|
|
def _no_init(self, *args, **kwargs):
|
|
return None
|
|
|
|
with patch.object(run_agent.AIAgent, "__init__", _no_init), \
|
|
patch.object(
|
|
run_agent.AIAgent,
|
|
"run_conversation",
|
|
_capture_run_conversation,
|
|
), \
|
|
patch.object(run_agent.AIAgent, "shutdown_memory_provider", lambda self: None), \
|
|
patch.object(run_agent.AIAgent, "close", lambda self: None), \
|
|
patch.object(_plugins, "set_thread_tool_whitelist", _capture_whitelist), \
|
|
patch("threading.Thread", _SyncThread):
|
|
agent._spawn_background_review(
|
|
messages_snapshot=[],
|
|
review_memory=True,
|
|
review_skills=False,
|
|
)
|
|
|
|
assert "propose_shared_memory" in captured["whitelist"]
|
|
assert "terminal" not in captured["whitelist"]
|
|
assert "propose_shared_memory" in captured["review_prompt"]
|
|
|
|
|