From e4ccfc16fc514fc61bb64979ebf6a013152e3f3e Mon Sep 17 00:00:00 2001 From: Finn763 Date: Wed, 16 Sep 2026 12:30:59 -0700 Subject: [PATCH] fix(tools): restore executor allowlist on message_agent inject success ensure_message_agent_tool() returned True on the schema-present branch without re-adding message_agent to agent.valid_tool_names. A long-lived Bot Chat whose tool surface was rebuilt (compaction / MCP refresh republishes the allowlist from the registry snapshot, which never contains the injected tool) then advertised message_agent while the executor rejected every call, and the model fell back to hermes -p shellouts that time out. Success now means both halves hold. Re-applied onto the current any()-shaped gate; semantic change and test are the contributor's (#96109). Closes #96105 --- tests/tools/test_bot_mode_dm.py | 24 ++++++++++++++++++++++++ tools/bot_mode_dm.py | 19 +++++++++++-------- 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/tests/tools/test_bot_mode_dm.py b/tests/tools/test_bot_mode_dm.py index 963a294814..1de28f47ad 100644 --- a/tests/tools/test_bot_mode_dm.py +++ b/tests/tools/test_bot_mode_dm.py @@ -86,6 +86,30 @@ def test_injects_only_into_bot_chat_on_managed_install(tmp_path): assert len(agent.tools) == 1 +def test_restores_allowlist_when_schema_survives_surface_refresh(tmp_path): + """#96105: success must restore the executor allowlist on the + schema-present branch. + + A long-lived Bot Chat whose tool surface is reconstructed can keep the + schema while the executor's allowlist is rebuilt empty. The injector then + returned True while dispatch would reject every ``message_agent`` call — + advertised but non-dispatchable. Restore-on-success contract: whenever + ``ensure_message_agent_tool()`` returns True, ``MESSAGE_AGENT_TOOL_NAME`` + is in ``valid_tool_names`` whenever that attribute is a set. + """ + home = _managed_home(tmp_path) + agent = _FakeAgent(home, title="Bot Chat") + assert bot_mode_dm.ensure_message_agent_tool(agent) is True + assert len(agent.tools) == 1 + + # capability refresh: schema survives, executor allowlist rebuilt empty + agent.valid_tool_names = set() + assert bot_mode_dm.ensure_message_agent_tool(agent) is True + assert bot_mode_dm.MESSAGE_AGENT_TOOL_NAME in agent.valid_tool_names + # byte-stable: no duplicate schema was appended + assert len(agent.tools) == 1 + + @pytest.mark.parametrize( "title", ["", "My research chat", "Group: room-abc123", "handoff-12ab34cd"], diff --git a/tools/bot_mode_dm.py b/tools/bot_mode_dm.py index 547756282b..f98f85ea85 100644 --- a/tools/bot_mode_dm.py +++ b/tools/bot_mode_dm.py @@ -136,16 +136,19 @@ def ensure_message_agent_tool(agent: Any) -> bool: if not getattr(agent, "_bot_mode_protocol", True): return False tools = getattr(agent, "tools", None) - if tools and any( + present = bool(tools) and any( isinstance(t, dict) and t.get("function", {}).get("name") == MESSAGE_AGENT_TOOL_NAME for t in tools - ): - return True - if not message_agent_authorized(agent): - return False - if agent.tools is None: - agent.tools = [] - agent.tools.append(message_agent_tool_schema()) + ) + if not present: + if not message_agent_authorized(agent): + return False + if agent.tools is None: + agent.tools = [] + agent.tools.append(message_agent_tool_schema()) + # Success means BOTH halves hold: a tool-surface rebuild (compaction, MCP refresh) + # can keep the schema while valid_tool_names is republished without it, and an + # advertised-but-nondispatchable tool sends the model hunting for shellouts (#96105). valid = getattr(agent, "valid_tool_names", None) if isinstance(valid, set): valid.add(MESSAGE_AGENT_TOOL_NAME)