diff --git a/src/tool_execution.py b/src/tool_execution.py index 902070a27..b7adef0a0 100644 --- a/src/tool_execution.py +++ b/src/tool_execution.py @@ -559,6 +559,19 @@ async def _execute_tool_block_impl( tool = block.tool_type content = block.content + # A bare email tool name is an alias for its MCP-qualified form (the + # dispatch below routes it to mcp__email__). Policy sources spell + # email tools either way — plan mode and the MCP settings toggle write the + # qualified name into the denylist, chat-level toggles the bare one — so + # the block/disable gates below must match on BOTH spellings. Gating only + # the name the model happened to emit lets a bare fence slip past a + # qualified denylist entry (and vice versa). + policy_names = {tool} + if tool in BUILTIN_EMAIL_TOOLS: + policy_names.add(f"mcp__email__{tool}") + elif tool.startswith("mcp__email__") and tool[len("mcp__email__"):] in BUILTIN_EMAIL_TOOLS: + policy_names.add(tool[len("mcp__email__"):]) + # Misformatted tool call detection: model put JSON inside ```python``` (or # similar) without naming the tool. Common with MiniMax-style outputs. # Return a helpful error so the model retries with the correct format. @@ -586,13 +599,13 @@ async def _execute_tool_block_impl( pass # Reject tools that the user has disabled for this request - if disabled_tools and tool in disabled_tools: + if disabled_tools and not policy_names.isdisjoint(disabled_tools): desc = f"{tool}: BLOCKED" result = {"error": f"Tool '{tool}' is disabled by user.", "exit_code": 1} logger.info(f"Tool blocked by user: {tool}") return desc, result - if tool_policy and tool_policy.blocks(tool): + if tool_policy and any(tool_policy.blocks(name) for name in policy_names): desc = f"{tool}: BLOCKED" result = { "error": f"Execution of tool '{tool}' is forbade by the active guide-only policy.", diff --git a/src/tool_parsing.py b/src/tool_parsing.py index 836a1e933..cc1078082 100644 --- a/src/tool_parsing.py +++ b/src/tool_parsing.py @@ -19,13 +19,17 @@ logger = logging.getLogger(__name__) # Regex patterns # --------------------------------------------------------------------------- -# Pattern 1: ```bash ... ``` fenced code blocks. The tag may be followed by -# inline args on the same line (```list_email_accounts {}) or a newline. +# Pattern 1: ```bash ... ``` fenced code blocks. The tag may be followed by a +# newline (classic form) or by inline JSON args on the same line +# (```list_email_accounts {}). Same-line content is accepted ONLY when it +# starts with { or [ — anything else after the tag is a Markdown info string +# (```python title="example.py"), which must stay display text rather than +# become executable tool input. # (?![\w-]) keeps the alternation from prefix-matching longer fence tags: # without it, ```python3 would match as tool "python" with content "3\n..." # and execute as code. _TOOL_BLOCK_RE = re.compile( - r"```(" + "|".join(TOOL_TAGS) + r")(?![\w-])[ \t]*\n?([\s\S]*?)```", + r"```(" + "|".join(TOOL_TAGS) + r")(?![\w-])[ \t]*(?=\r?\n|[{\[])\r?\n?([\s\S]*?)```", re.IGNORECASE, ) diff --git a/tests/test_fenced_inline_args.py b/tests/test_fenced_inline_args.py index ba835a6c7..4604f9a0f 100644 --- a/tests/test_fenced_inline_args.py +++ b/tests/test_fenced_inline_args.py @@ -58,6 +58,27 @@ def test_hyphenated_tag_is_not_a_tool_call(): assert blocks == [], blocks +def test_markdown_info_string_is_not_executable_python(): + # ```python title="example.py" is Markdown fence metadata, not tool args. + # Same-line content other than JSON args ({...}/[...]) must not execute — + # otherwise a fence the model meant to display runs as code. + blocks = parse_tool_blocks('```python title="example.py"\nprint("hi")\n```') + assert blocks == [], blocks + + +def test_markdown_info_string_is_not_executable_bash(): + blocks = parse_tool_blocks('```bash title="setup"\necho hi\n```') + assert blocks == [], blocks + + +def test_inline_json_array_args_still_parse(): + # The narrowed same-line rule must keep accepting JSON args: { or [. + blocks = parse_tool_blocks('```bulk_email {"action": "archive", "uids": [1, 2]}\n```') + assert [(b.tool_type, b.content) for b in blocks] == [ + ("bulk_email", '{"action": "archive", "uids": [1, 2]}') + ] + + def test_inline_args_fence_is_stripped_from_display(): # strip must mirror parse: an executed inline-args fence must not leak # into the displayed text. @@ -69,3 +90,10 @@ def test_python3_fence_is_left_intact_in_display(): # ...and a fence that did NOT parse as a tool call must stay visible. text = 'Example:\n```python3\nprint("hi")\n```' assert strip_tool_blocks(text) == text + + +def test_markdown_info_string_fence_is_left_intact_in_display(): + # strip must mirror parse for info-string fences too: not executed, + # so not stripped from the displayed text. + text = 'Example:\n```python title="example.py"\nprint("hi")\n```' + assert strip_tool_blocks(text) == text diff --git a/tests/test_review_regressions.py b/tests/test_review_regressions.py index daa3b5489..35a4fdf2f 100644 --- a/tests/test_review_regressions.py +++ b/tests/test_review_regressions.py @@ -635,6 +635,59 @@ async def test_public_agent_policy_blocks_sensitive_tools(monkeypatch): assert "restricted to admin users" in result["error"] +@pytest.mark.asyncio +async def test_disabled_qualified_email_tool_blocks_bare_alias(monkeypatch): + """A bare email fence is an alias for its mcp__email__ form. Plan mode and + the MCP settings toggle write the QUALIFIED name into disabled_tools, so + the gate must block the bare spelling too — and never reach the MCP + manager (PR #3681 review follow-up).""" + import src.tool_execution as tool_execution + from src.tool_execution import execute_tool_block + + def fail_get_mcp_manager(): + raise AssertionError("blocked email tool must not reach the MCP manager") + + monkeypatch.setattr(tool_execution, "get_mcp_manager", fail_get_mcp_manager) + + for bare, disabled in ( + # qualified denylist entry blocks the bare alias… + ("list_emails", {"mcp__email__list_emails"}), + ("download_attachment", {"mcp__email__download_attachment"}), + # …and a bare denylist entry blocks the qualified spelling. + ("mcp__email__delete_email", {"delete_email"}), + ): + desc, result = await execute_tool_block( + SimpleNamespace(tool_type=bare, content="{}"), + owner="admin-user", + disabled_tools=disabled, + ) + assert desc == f"{bare}: BLOCKED" + assert result["exit_code"] == 1 + assert "disabled by user" in result["error"] + + +@pytest.mark.asyncio +async def test_tool_policy_qualified_email_block_covers_bare_alias(monkeypatch): + """Same aliasing rule for the turn ToolPolicy denylist.""" + import src.tool_execution as tool_execution + from src.tool_execution import execute_tool_block + from src.tool_policy import ToolPolicy + + def fail_get_mcp_manager(): + raise AssertionError("blocked email tool must not reach the MCP manager") + + monkeypatch.setattr(tool_execution, "get_mcp_manager", fail_get_mcp_manager) + + policy = ToolPolicy(disabled_tools=frozenset({"mcp__email__send_email"})) + desc, result = await execute_tool_block( + SimpleNamespace(tool_type="send_email", content="{}"), + owner="admin-user", + tool_policy=policy, + ) + assert desc == "send_email: BLOCKED" + assert result["exit_code"] == 1 + + def test_public_agent_policy_hides_sensitive_tools(monkeypatch): auth_mod = _install_core_auth_stub(monkeypatch) from src.tool_security import blocked_tools_for_owner