From 016ce473f4f9df5fbcfaf63757971972491cc9e9 Mon Sep 17 00:00:00 2001 From: botinate <285686135+botinate@users.noreply.github.com> Date: Thu, 11 Jun 2026 22:25:30 +0200 Subject: [PATCH] refactor: move the email alias rule into tool_security; extract the assistant seed constant Code-quality pass over the PR's own changes: - The bare<->qualified email aliasing rule lived inline in the generic dispatcher (_execute_tool_block_impl). It is policy knowledge, so it moves next to BUILTIN_EMAIL_TOOLS as email_tool_policy_names(); the dispatcher just consumes it, and the rule gets its own unit test (including the mcp__email__ and mcp__other__ non-alias cases). - The default Assistant's enabled_tools list was an inline literal inside the CrewMember seed, and its registry-sync test asserted a source-code substring. Extracted to DEFAULT_ASSISTANT_ENABLED_TOOLS so the test imports and checks the actual value. - _fenced_tool_call return type tightened to Optional[Tuple[str, str]]. No behavior change; suite green (3295 passed). Co-Authored-By: Claude Fable 5 --- src/task_scheduler.py | 29 ++++++++++++++++------------- src/tool_execution.py | 24 +++++++++++------------- src/tool_parsing.py | 4 ++-- src/tool_security.py | 22 ++++++++++++++++++++++ tests/test_email_registry_sync.py | 24 ++++++++++++++++++++---- 5 files changed, 71 insertions(+), 32 deletions(-) diff --git a/src/task_scheduler.py b/src/task_scheduler.py index 98d118149..82a3ceb7a 100644 --- a/src/task_scheduler.py +++ b/src/task_scheduler.py @@ -13,6 +13,21 @@ from src.tool_security import BUILTIN_EMAIL_TOOLS logger = logging.getLogger(__name__) +# Tools granted to the default Assistant crew member created on first run. +# Email names derive from BUILTIN_EMAIL_TOOLS so a tool added to the email +# server is granted to new default assistants automatically; users can still +# untick tools in the assistant settings UI. +DEFAULT_ASSISTANT_ENABLED_TOOLS = ( + "manage_calendar", "manage_notes", "manage_tasks", "manage_memory", + *sorted(BUILTIN_EMAIL_TOOLS), + "resolve_contact", + "search_chats", "web_search", "web_fetch", "read_file", + "create_document", "update_document", "edit_document", + "generate_image", "trigger_research", + "download_model", "serve_model", "list_served_models", "stop_served_model", + "edit_image", +) + def _utcnow() -> datetime: """Return naive UTC for task DB fields without using deprecated APIs.""" @@ -2293,19 +2308,7 @@ class TaskScheduler: model=model, endpoint_url=endpoint_url, greeting=None, - enabled_tools=json.dumps([ - "manage_calendar", "manage_notes", "manage_tasks", "manage_memory", - # Full built-in email tool set (single source of truth in - # tool_security) so a tool added to the email server is - # granted to new default assistants without touching this. - *sorted(BUILTIN_EMAIL_TOOLS), - "resolve_contact", - "search_chats", "web_search", "web_fetch", "read_file", - "create_document", "update_document", "edit_document", - "generate_image", "trigger_research", - "download_model", "serve_model", "list_served_models", "stop_served_model", - "edit_image", - ]), + enabled_tools=json.dumps(list(DEFAULT_ASSISTANT_ENABLED_TOOLS)), session_id=session_id, is_active=True, sort_order=0, diff --git a/src/tool_execution.py b/src/tool_execution.py index b7adef0a0..2e97b84cc 100644 --- a/src/tool_execution.py +++ b/src/tool_execution.py @@ -21,7 +21,12 @@ from typing import Any, Awaitable, Callable, Dict, Optional, Tuple -from src.tool_security import BUILTIN_EMAIL_TOOLS, is_public_blocked_tool, owner_is_admin_or_single_user +from src.tool_security import ( + BUILTIN_EMAIL_TOOLS, + email_tool_policy_names, + is_public_blocked_tool, + owner_is_admin_or_single_user, +) from src.tool_policy import ToolPolicy from src.constants import MAX_OUTPUT_CHARS, MAX_READ_CHARS, MAX_DIFF_LINES, DATA_DIR from src.tool_utils import _truncate, get_mcp_manager @@ -559,18 +564,11 @@ 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__"):]) + # The block/disable gates below must match every policy-equivalent + # spelling of the tool name (bare email names alias their mcp__email__ + # form — see email_tool_policy_names), not just the spelling the model + # happened to emit. + policy_names = email_tool_policy_names(tool) # Misformatted tool call detection: model put JSON inside ```python``` (or # similar) without naming the tool. Common with MiniMax-style outputs. diff --git a/src/tool_parsing.py b/src/tool_parsing.py index 67c949a7e..28245bf3a 100644 --- a/src/tool_parsing.py +++ b/src/tool_parsing.py @@ -9,7 +9,7 @@ import ast import json import logging import re -from typing import List, Optional +from typing import List, Optional, Tuple from src.agent_tools import ToolBlock, TOOL_TAGS @@ -41,7 +41,7 @@ _TOOL_BLOCK_RE = re.compile( _CODE_FENCE_TAGS = frozenset({"bash", "python"}) -def _fenced_tool_call(m) -> Optional[tuple]: +def _fenced_tool_call(m) -> Optional[Tuple[str, str]]: """Classify a Pattern-1 fence match: (tag, content) when it is an executable tool call, None when the fence must stay display text. diff --git a/src/tool_security.py b/src/tool_security.py index 5b9f075f8..d099581c5 100644 --- a/src/tool_security.py +++ b/src/tool_security.py @@ -184,6 +184,28 @@ def plan_mode_disabled_tools() -> Set[str]: return (all_names | _PLAN_MODE_KNOWN_MUTATORS) - PLAN_MODE_READONLY_TOOLS +def email_tool_policy_names(tool_name: str) -> frozenset: + """All policy-equivalent spellings of a tool name. + + A bare built-in email tool name and its MCP-qualified mcp__email__ + form dispatch to the same email server tool, but policy sources spell + them either way — plan mode and the MCP settings toggle write qualified + names into denylists, chat-level toggles write bare ones. Every gate must + match against the full alias set, or a call in one spelling slips past a + denylist entry written in the other. Non-email names alias only to + themselves. + """ + if not isinstance(tool_name, str): + return frozenset((tool_name,)) + if tool_name in BUILTIN_EMAIL_TOOLS: + return frozenset((tool_name, f"mcp__email__{tool_name}")) + if tool_name.startswith("mcp__email__"): + bare = tool_name[len("mcp__email__"):] + if bare in BUILTIN_EMAIL_TOOLS: + return frozenset((tool_name, bare)) + return frozenset((tool_name,)) + + def is_public_blocked_tool(tool_name: Optional[str]) -> bool: """Return True when a non-admin/public user must not execute this tool. diff --git a/tests/test_email_registry_sync.py b/tests/test_email_registry_sync.py index ca3a4e2cb..83d65db6f 100644 --- a/tests/test_email_registry_sync.py +++ b/tests/test_email_registry_sync.py @@ -116,7 +116,23 @@ def test_frontend_tool_selector_covers_email_tools(): def test_default_assistant_seed_covers_email_tools(): """The default Assistant crew seed grants the full email set.""" - source = (_REPO_ROOT / "src" / "task_scheduler.py").read_text() - assert "*sorted(BUILTIN_EMAIL_TOOLS)" in source, ( - "default assistant enabled_tools no longer derives from BUILTIN_EMAIL_TOOLS" - ) + from src.task_scheduler import DEFAULT_ASSISTANT_ENABLED_TOOLS + + assert BUILTIN_EMAIL_TOOLS <= set(DEFAULT_ASSISTANT_ENABLED_TOOLS) + + +def test_email_policy_name_aliases(): + """The alias rule every execution gate relies on.""" + from src.tool_security import email_tool_policy_names + + assert email_tool_policy_names("list_emails") == { + "list_emails", "mcp__email__list_emails", + } + assert email_tool_policy_names("mcp__email__delete_email") == { + "delete_email", "mcp__email__delete_email", + } + # Non-email names alias only to themselves — including mcp__email__ + # spellings of tools the email server doesn't expose. + assert email_tool_policy_names("bash") == {"bash"} + assert email_tool_policy_names("mcp__email__not_a_tool") == {"mcp__email__not_a_tool"} + assert email_tool_policy_names("mcp__other__list_emails") == {"mcp__other__list_emails"}