Make background review toolsets configurable - #45014
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for making the runtime whitelist configurable. The current patch needs two corrections before it can preserve main's safety and Tool Search contracts.
Problems
agent/background_review.py:510replaces the current profile gate atagent/background_review.py:802-808with a default containingmemory. Commit437dcacbbadded that gate because a memory-disabled profile could otherwise receive the built-in memory tool; please retain it when composing configured toolsets.- The MCP/plugin claim is incomplete for a review-only configured toolset. The fork still inherits the parent scope (
agent/background_review.py:711-712), andagent/tool_executor.py:239-256,402-415rejects an unwrappedtool_calloutside that scope. The whitelist alone does not change it. - The added test only mocks schema collection and captures the whitelist (
tests/run_agent/test_background_review_cache_parity.py:304-326); it does not exercise actual bridge dispatch or the memory-disabled profile case.
Suggested changes
- Compose configured toolsets with the existing memory-enabled/user-profile-enabled gate.
- Add real Tool Search dispatch coverage for an in-scope configured deferred tool, plus a memory-disabled regression test.
Automated hermes-sweeper review.
| clear_thread_tool_whitelist, | ||
| ) | ||
|
|
||
| review_toolsets = _resolve_background_review_toolsets() |
There was a problem hiding this comment.
Blocking: this replaces main's current memory-disable gate (agent/background_review.py:802-808, commit 437dcacbb). Because the new default contains memory, a profile with both _memory_enabled and _user_profile_enabled false will again whitelist the built-in memory tool. Compose the configured list with that existing gate instead of replacing it.
| captured["allowed"] = set(allowed) | ||
| captured["deny_msg_fmt"] = deny_msg_fmt | ||
|
|
||
| with patch.object(run_agent, "AIAgent", _Recorder), \ |
There was a problem hiding this comment.
Blocking coverage gap: this test mocks schema resolution and stops before any tool dispatch. It cannot verify the claimed deferred MCP path, whose real executor scope comes from the inherited agent.enabled_toolsets (agent/tool_executor.py:239-256,402-415), nor catch the memory-disabled-profile regression.
Summary
memory.review_toolsetswith the existing safe default of["memory", "skills"].tool_search/tool_call.Tests
python -m pytest tests/run_agent/test_background_review.py tests/run_agent/test_background_review_toolset_restriction.py tests/run_agent/test_background_review_summary.py tests/run_agent/test_background_review_cache_parity.py tests/hermes_cli/test_plugins.py::TestThreadToolWhitelist -q -o 'addopts='python -m pytest tests/hermes_cli/test_config.py tests/hermes_cli/test_config_validation.py -q -o 'addopts='python -m ruff check agent/background_review.py hermes_cli/config.py tests/run_agent/test_background_review_cache_parity.py