Skip to content

Commit 69b9bb0

Browse files
botinateclaude
andauthored
fix(agent): execute fenced tool calls with inline args and route bare email tool names (#3681)
* fix(agent): execute fenced tool calls with inline args and bare email tool names Two bugs made local (Ollama) models unable to use email tools, leaving raw fences like ```list_email_accounts {}``` in the chat: 1. _TOOL_BLOCK_RE required a newline right after the fence tag, so a tool call with args on the same line ("```list_email_accounts {}") never matched and was never executed. The fence now matches with optional spaces/newline after the tag. 2. Even when parsed, bare email tool names had no dispatch branch in tool_execution.py and fell through to "Unknown tool type". They now route to the email MCP server as mcp__email__<name>, matching how function_call_to_tool_block already maps them for native callers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): block all bare email tool names for non-admins; harden fence-tag regex Review follow-up on #3681 (thanks @vgalin): 1. Routing bare email names made 10 of the 14 email tools executable by non-admin owners — is_public_blocked_tool() runs on the bare name before dispatch, and NON_ADMIN_BLOCKED_TOOLS only listed 4. Define the full email tool set once (BUILTIN_EMAIL_TOOLS in tool_security.py) and derive the blocklist, the fence tags (TOOL_TAGS), the bare-name dispatch, and the native-call mapping from it so they can't drift. This also fixes 4 tools (search_emails, draft_email, draft_email_reply, ai_draft_email_reply) that were missing from the old tool_schemas copy and therefore unreachable even for native function-calling models. 2. The relaxed fence regex from the previous commit could prefix-match longer fence tags: ```python3 parsed as tool "python" with content "3\nprint(...)" and executed as code. Add a (?![\w-]) boundary after the tag. Tests: test_public_agent_policy_blocks_sensitive_tools now covers all 14 bare email names + the mcp__email__ form; new tests/test_fenced_inline_args.py pins inline-args parsing, the python3/hyphenated-tag non-matches, and strip/parse display mirroring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(security): gate bare and mcp-qualified email names together; stop executing Markdown info strings Review follow-up on #3681 (thanks @RaresKeY): 1. P1: execute_tool_block() checked disabled_tools / the turn ToolPolicy only against the incoming block name, then the bare-email branch qualified it to mcp__email__<name> and called the MCP manager. Plan mode and the MCP settings toggle write the QUALIFIED name into the denylist, so a bare fence like ```list_emails``` sailed past a mcp__email__list_emails entry. Both gates now match on both spellings (bare <-> mcp__email__-qualified), in either direction. 2. P2: the relaxed fence regex accepted arbitrary same-line text after a recognized tag, which made ordinary Markdown info strings executable: ```python title="example.py" ran as a python tool call. Same-line content now only counts as tool input when it starts with { or [ (JSON args); anything else leaves the fence as display text, and strip_tool_blocks mirrors that (the fence stays visible). Tests: disabled-tools alias regression (qualified entry blocks bare name and vice versa, never reaching the MCP manager), ToolPolicy alias regression, python/bash title="..." non-execution + display retention, and inline JSON-array args still parsing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(security): reject brace-style fence metadata; cover the full email set in the friendly toggle Review follow-up round 3 on #3681 (thanks @RaresKeY): 1. Brace-style fence metadata no longer executes. The previous narrowing still treated any same-line {/[ after a recognized tag as tool input, so ```bash {title="setup"} ran as a bash call. The fence header is now captured separately and judged by one predicate shared between parse_tool_blocks and strip_tool_blocks (_fenced_tool_call), so the execute and display decisions can't disagree: same-line content only counts as inline args when the tag is NOT a code tag (bash/python never take same-line args — that text is Markdown fence attributes) AND the inline text (plus any continuation lines) parses as standalone JSON. ```bash {title="setup"}, ```python {"title":"example.py"} and ```list_emails {title="x"} all stay visible and inert. 2. The friendly `disable_tool email` toggle covered 3 of the 14 email tools (mcp__email__{list_emails,read_email,send_email}); the other bare aliases this PR routes stayed executable after an operator disabled email. The alias now derives from BUILTIN_EMAIL_TOOLS in BOTH spellings — bare (function-schema hiding, bare-fence dispatch) and mcp__email__* (MCP schema hiding, qualified runtime blocks) — so the toggle and the runtime gate can't drift apart. Tests: brace/bracket metadata regressions for parse and strip symmetry (code tags, invalid-JSON inline on a JSON tool, multi-line inline JSON still parsing), and disable_tool/enable_tool email covering all 14 names in both spellings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(email): close remaining email-tool registry drift; classify every email tool for plan mode Deep self-review follow-up on #3681. Three review rounds each found another hand-maintained copy of the email tool list that had drifted; this commit hunts down ALL remaining copies and pins them to BUILTIN_EMAIL_TOOLS. The same 5 tools (search_emails, draft_email, draft_email_reply, ai_draft_email_reply, download_attachment) were missing from every advertising surface, so they were dispatchable but never offered: - FUNCTION_TOOL_SCHEMAS: native function-calling models never saw them (the round-1 fix covered dispatch only); schemas added, mirroring the email server's inputSchema definitions. - TOOL_SECTIONS: fenced-block models were never told about them; prompt sections added. - tool_index: absent from the RAG embedding registry (never retrievable), the email keyword hints, and the scheduled assistant's always-available set — the latter two now derive from BUILTIN_EMAIL_TOOLS. - agent_loop._DOMAIN_TOOL_MAP["email"], tool_policy._COMMON_TOOL_NAMES, the assistant tool-selector UI groups (assistant.js), and the default Assistant crew seed (task_scheduler) now derive from / cover the set. Plan mode now classifies every email tool explicitly: - list_email_accounts and search_emails join PLAN_MODE_READONLY_TOOLS. Without this, list_email_accounts sat in the plan-mode bare denylist (schema-derived) while its qualified form passed the MCP read-only filter — and the round-2 bare/qualified alias gate would have blocked the qualified call too, regressing read-only email discovery in plan mode. - draft_email, draft_email_reply, ai_draft_email_reply, and download_attachment join the fail-closed mutator backstop (drafts create documents; download_attachment writes to disk). Tests: tests/test_email_registry_sync.py pins every registry (including the email server source and assistant.js) to BUILTIN_EMAIL_TOOLS and asserts the plan-mode partition, so the next email tool can't drift; a parse/strip mirror grid covers 192 fence shapes (tag x header x body) asserting executed <=> stripped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * 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__<not-a-tool> 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 <noreply@anthropic.com> * revert: move the email registry consolidation to a follow-up PR Per review feedback on scope, this PR stays narrow: fenced inline-args parsing, bare email tool routing, and the directly required safety gates. This commit reverts the registry/advertising consolidation from db29046 and 016ce47 (native schemas, prompt sections, RAG description index + keyword hints, assistant always-available set, guide-only known-names union, frontend tool-selector groups, default assistant seed, and their sync tests) — all of that moves to a dedicated follow-up PR together with the _EMAIL_TOOL_HINTS finding. Kept here because the narrow scope needs them: - email_tool_policy_names() in tool_security + its use in the execute_tool_block gates and its unit test (refactor of this PR's own round-2 alias fix), - list_email_accounts in PLAN_MODE_READONLY_TOOLS (the alias gate works both ways, and the schema-derived plan-mode bare denylist would otherwise block the qualified read-only call too), - the parse/strip mirror grid test (parser scope), - the narrow registry sync tests (email server <-> BUILTIN_EMAIL_TOOLS match, fence-tag coverage, non-admin blocklist coverage). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(email): execute empty email fences with empty args; reject non-object JSON args Two gaps found by replaying captured local-model traffic against the narrowed branch: 1. ```list_email_accounts``` with NO body — a shape gemma really emits for no-arg tools — was silently dropped (parse skips empty content), so the model concluded email was broken: the original #337 symptom through a different door. Empty fences whose tag is a built-in email tool now dispatch with {} args and the tool's own validation answers (e.g. an empty send_email returns "to is required" instead of silence). Empty bash/python/other fences keep skipping, and strip stays mirrored (the fence was executed, so it is removed). 2. The fence parser accepts JSON arrays as inline args, but the email dispatch parsed only objects — an array silently became {} args. Non-object JSON now returns a correctable "arguments must be a JSON object" error before reaching the MCP server (same class as #3966). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(security): classify all email tools for plan mode statically; reject invalid email JSON bodies Review follow-up round 5 on #3681 (thanks @RaresKeY): 1. This PR makes every BUILTIN_EMAIL_TOOLS name fence-taggable, so each one must be explicitly classified for plan mode — the draft tools and download_attachment were in neither the read-only allowlist nor the static denylist, leaving their bare-alias plan-mode safety dependent on the MCP read-only inventory being present and current. search_emails joins PLAN_MODE_READONLY_TOOLS (explicit, not allowed-by-omission); draft_email, draft_email_reply, ai_draft_email_reply, and download_attachment join the fail-closed _PLAN_MODE_KNOWN_MUTATORS backstop. (Moved back from the #4053 split: the partition is directly required for this PR to merge independently.) 2. The classic tag/body fence form reaches execution unvalidated (only INLINE args are JSON-checked by the parser), so a body like {account: "work"} silently became {} args and read the DEFAULT mailbox instead of the intended one. JSON-looking bodies that fail to parse now return a correctable "not valid JSON" error before reaching the MCP server. Tests: a partition invariant (every email tool is explicitly read-only or plan-mode-denied), a mutating-alias probe that uses only the static denylist with a fake MCP manager (no inventory layer), and the body-form invalid-JSON regression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(tool-dispatch): decode inline JSON args for legacy MCP tools; reject all non-object email bodies Review follow-up round 6 on #3681 (thanks @RaresKeY) — both pre-existing on this branch, surfaced by the relaxed inline-args parser: 1. The relaxed parser accepts inline JSON for every non-code tag, but the legacy line-based arg builders (web_search/web_fetch/read_file/ write_file/generate_image/manage_memory) wrapped the whole JSON string as the query/url/path/prompt — so `web_search {"query": "x"}` executed as a search for the literal string `{"query": "x"}`. _build_mcp_args now uses a fenced JSON object directly when it carries the tool's primary arg key (query/url/path/prompt/action). Keyed off membership so it can't drift; an object without the primary key (e.g. a freeform JSON query, or bare object content for write_file) falls through to the line parser unchanged. Also fixes the same corruption for the classic newline-JSON form. 2. The bare-email dispatch only rejected bodies starting with { or [, so a non-empty non-JSON body like `account: work` still fell through to {} args and silently read the DEFAULT mailbox. Now ANY non-empty body must decode to a JSON object or it returns a correctable error; only a truly empty body keeps the no-arg path (```list_email_accounts```). Tests: inline-JSON arg decoding for the five legacy tools plus the freeform and missing-primary-key fallbacks; the email body rejection extended to cover the brace-looking and bare `key: value` shapes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(tool-dispatch): drop dead manage_memory JSON-decode entry; pin the live-path invariant Self-audit catch on the round-6 fix. manage_memory was added to _MCP_JSON_PRIMARY_KEYS, but _build_mcp_args is only reached via _call_mcp_tool, which only runs for _MCP_TOOL_MAP tools — and manage_memory isn't one (its tag routes through dispatch_ai_tool -> do_manage_memory, which line-parses). So the round-6 decode for manage_memory was dead code: the unit test exercising _build_mcp_args passed while a real `manage_memory {"action": ...}` fence still parsed the whole JSON blob as the action. Remove the dead entry and add test_mcp_json_primary_keys_are_all_live, which asserts every JSON-primary tool is in _MCP_TOOL_MAP so a dead decode can't be added again. The same inline-JSON corruption for manage_memory and the other tools that route through positional dispatchers (create_session, ui_control, send_to_session, search_chats, the document tools, etc.) is pre-existing (dev corrupts their newline JSON form too) and tracked separately; the proper fix there is to route fenced JSON through function_call_to_tool_block. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(tool-dispatch): decode inline JSON in WriteFileTool (its live path); round-6 fix was on the dead MCP path Self-audit: round 6 claimed to fix inline JSON args for write_file via _build_mcp_args, but there is no filesystem MCP server, so write_file always runs through _direct_fallback -> WriteFileTool, never through _build_mcp_args. WriteFileTool — unlike its siblings ReadFileTool / WebSearchTool / WebFetchTool, which all decode JSON — took lines[0] as the path, so `write_file {"path": "/tmp/x", "content": "y"}` wrote to a file literally named with the JSON blob. The round-6 _build_mcp_args entry decoded correctly but on a path that never executes (same class as the manage_memory dead entry), and the round-6 unit test passed on that dead path. WriteFileTool now decodes a JSON object carrying "path" (matching ReadFileTool directly above it), and the comment on _MCP_JSON_PRIMARY_KEYS records that only generate_image has a live MCP server today — the other entries are defense-in-depth for the MCP path; the live fix for each server-less tool is in its handler. Test: test_write_file_inline_json_args drives the LIVE path (execute_tool_block with no MCP) and asserts the intended path is used — verified to fail without the handler fix. web_search/web_fetch/read_file were already correct (their handlers decode); write_file was the gap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(strip-fence): derive the live-strip TOOL_TAGS from the real set Semantic conflict from the dev merge that textual auto-merge didn't flag: dev added test_live_strip_email_tool_fences.py whose _tool_tags() helper source-scrapes only the TOOL_TAGS literal `{...}`, which worked on dev because the email tool names were listed inline there. This branch makes TOOL_TAGS the single source — `{...} | BUILTIN_EMAIL_TOOLS` — so the email names are no longer in the literal and the scraper missed them, leaving the email-fence strip assertions failing even though TOOL_TAGS does contain them at runtime. Import the real TOOL_TAGS instead of scraping source, so the test mirrors exactly what GET /api/tools serves (sorted(TOOL_TAGS)) and the live EXEC_FENCE_RE derives from — robust to however the set is composed. The source-level frontend/route guards in the same file are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: botinate <285686135+botinate@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent df9c20e commit 69b9bb0

11 files changed

Lines changed: 861 additions & 32 deletions

src/agent_tools/__init__.py

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import logging
1515
from collections import namedtuple
1616

17+
from src.tool_security import BUILTIN_EMAIL_TOOLS
1718
from src.tool_utils import _truncate, get_mcp_manager, set_mcp_manager
1819

1920
logger = logging.getLogger(__name__)
@@ -86,9 +87,10 @@
8687
"manage_endpoints", "manage_mcp", "manage_webhooks",
8788
"manage_tokens", "manage_documents", "manage_settings",
8889
"manage_notes", "manage_calendar",
89-
"resolve_contact", "manage_contact", "list_email_accounts", "send_email", "list_emails",
90-
"read_email", "reply_to_email", "bulk_email", "archive_email",
91-
"delete_email", "mark_email_read",
90+
"resolve_contact", "manage_contact",
91+
# Email tool names come from BUILTIN_EMAIL_TOOLS (unioned below)
92+
# so the fence regex, dispatch, and non-admin blocklist all cover
93+
# the same set.
9294
# Cookbook tools (LLM serving + downloads). Without these
9395
# entries, native function calls to e.g. list_served_models
9496
# are rejected as "Unknown function call" before reaching
@@ -105,7 +107,7 @@
105107
# Generic loopback to any UI-button endpoint (cookbook,
106108
# gallery, email folders, etc.) — agent uses this when
107109
# there's no named tool wrapper for the action.
108-
"app_api"}
110+
"app_api"} | BUILTIN_EMAIL_TOOLS
109111

110112
ToolBlock = namedtuple("ToolBlock", ["tool_type", "content"])
111113

src/agent_tools/admin_tools.py

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
from typing import Optional, Dict
1515

1616
from src.tool_utils import get_mcp_manager, _parse_tool_args
17+
from src.tool_security import BUILTIN_EMAIL_TOOLS
1718

1819
logger = logging.getLogger(__name__)
1920

@@ -706,7 +707,14 @@ def _mask(k, v):
706707
"tasks": ["manage_tasks"],
707708
"notes": ["manage_notes"],
708709
"calendar": ["manage_calendar"],
709-
"email": ["mcp__email__list_emails", "mcp__email__read_email", "mcp__email__send_email"],
710+
# The full built-in email tool set, in BOTH spellings: the
711+
# qualified mcp__email__* names drive MCP schema hiding, the
712+
# bare names drive function-schema hiding, and the runtime
713+
# gate accepts either — deriving from BUILTIN_EMAIL_TOOLS
714+
# keeps the toggle covering every tool the email server
715+
# exposes instead of a hand-picked subset.
716+
"email": sorted(BUILTIN_EMAIL_TOOLS)
717+
+ [f"mcp__email__{t}" for t in sorted(BUILTIN_EMAIL_TOOLS)],
710718
"research": ["web_search", "web_fetch"], # research is a per-request flag, not a tool (closest analog)
711719
}
712720

src/agent_tools/filesystem_tools.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,21 @@ async def execute(self, content: str, ctx: dict) -> dict:
186186
lines = content.split("\n", 1)
187187
raw_path = lines[0].strip()
188188
body = lines[1] if len(lines) > 1 else ""
189+
# Decode JSON-object args (the fenced inline-args shape
190+
# ```write_file {"path": "...", "content": "..."}```), matching
191+
# ReadFileTool above. Without this the whole JSON string becomes the
192+
# path and the file is written under a garbage name. This is the live
193+
# path: there is no filesystem MCP server, so write_file always runs
194+
# here via _direct_fallback, not through _build_mcp_args.
195+
_stripped = content.strip()
196+
if _stripped.startswith("{"):
197+
try:
198+
_a = json.loads(_stripped)
199+
if isinstance(_a, dict) and "path" in _a:
200+
raw_path = str(_a.get("path", "")).strip()
201+
body = str(_a.get("content", ""))
202+
except (json.JSONDecodeError, TypeError, ValueError):
203+
pass
189204
try:
190205
path = _resolve_tool_path(raw_path)
191206
except ValueError as e:

src/tool_execution.py

Lines changed: 90 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,12 @@
2121

2222

2323

24-
from src.tool_security import is_public_blocked_tool, owner_is_admin_or_single_user
24+
from src.tool_security import (
25+
BUILTIN_EMAIL_TOOLS,
26+
email_tool_policy_names,
27+
is_public_blocked_tool,
28+
owner_is_admin_or_single_user,
29+
)
2530
from src.tool_policy import ToolPolicy
2631
from src.constants import MAX_OUTPUT_CHARS, MAX_READ_CHARS, MAX_DIFF_LINES, DATA_DIR
2732
from src.tool_utils import _truncate, get_mcp_manager
@@ -390,8 +395,42 @@ def _parse_write_file(content: str) -> Dict:
390395
}
391396

392397

398+
# Primary argument key(s) for the legacy line-parsed tools. When a fenced
399+
# block's content is a JSON object carrying one of these keys, it's structured
400+
# inline args (the relaxed parser's ```web_search {"query": "..."}``` shape) —
401+
# use the object directly instead of letting the line-based parsers wrap the
402+
# whole JSON string as the query/url/path/prompt. Keyed off membership only
403+
# (the primary key never changes), so this can't drift; an unrecognized object
404+
# safely falls through to the line-based parser, i.e. the previous behavior.
405+
#
406+
# IMPORTANT — this only covers the MCP path. _build_mcp_args is reached via
407+
# _call_mcp_tool only for _MCP_TOOL_MAP tools (so an entry outside that map is
408+
# dead, as manage_memory was). And of these, only generate_image has a live MCP
409+
# server today; web_search/web_fetch/read_file/write_file have none, so they run
410+
# via _direct_fallback -> TOOL_HANDLERS, whose handlers decode JSON themselves
411+
# (see ReadFileTool/WriteFileTool/WebSearchTool/WebFetchTool). The entries here
412+
# are kept as defense-in-depth for if/when those servers are added. The live
413+
# fix for each server-less tool lives in its handler. test_write_file_inline_
414+
# json_args and test_mcp_json_primary_keys_are_all_live pin both halves.
415+
_MCP_JSON_PRIMARY_KEYS: Dict[str, tuple] = {
416+
"web_search": ("query", "queries"),
417+
"web_fetch": ("url",),
418+
"read_file": ("path",),
419+
"write_file": ("path",),
420+
"generate_image": ("prompt",),
421+
}
422+
423+
393424
def _build_mcp_args(tool: str, content: str) -> Dict:
394425
"""Convert fenced-block text content to structured MCP arguments."""
426+
primaries = _MCP_JSON_PRIMARY_KEYS.get(tool)
427+
if primaries and content.strip().startswith("{"):
428+
try:
429+
decoded = json.loads(content.strip())
430+
except (json.JSONDecodeError, TypeError):
431+
decoded = None
432+
if isinstance(decoded, dict) and any(k in decoded for k in primaries):
433+
return decoded
395434
parser = _MCP_ARG_PARSERS.get(tool)
396435
return parser(content) if parser else {}
397436

@@ -596,6 +635,12 @@ async def _execute_tool_block_impl(
596635
tool = block.tool_type
597636
content = block.content
598637

638+
# The block/disable gates below must match every policy-equivalent
639+
# spelling of the tool name (bare email names alias their mcp__email__
640+
# form — see email_tool_policy_names), not just the spelling the model
641+
# happened to emit.
642+
policy_names = email_tool_policy_names(tool)
643+
599644
# Misformatted tool call detection: model put JSON inside ```python``` (or
600645
# similar) without naming the tool. Common with MiniMax-style outputs.
601646
# Return a helpful error so the model retries with the correct format.
@@ -623,13 +668,13 @@ async def _execute_tool_block_impl(
623668
pass
624669

625670
# Reject tools that the user has disabled for this request
626-
if disabled_tools and tool in disabled_tools:
671+
if disabled_tools and not policy_names.isdisjoint(disabled_tools):
627672
desc = f"{tool}: BLOCKED"
628673
result = {"error": f"Tool '{tool}' is disabled by user.", "exit_code": 1}
629674
logger.info(f"Tool blocked by user: {tool}")
630675
return desc, result
631676

632-
if tool_policy and tool_policy.blocks(tool):
677+
if tool_policy and any(tool_policy.blocks(name) for name in policy_names):
633678
desc = f"{tool}: BLOCKED"
634679
result = {
635680
"error": f"Execution of tool '{tool}' is forbade by the active guide-only policy.",
@@ -823,6 +868,48 @@ async def _execute_tool_block_impl(
823868
elif tool == "vault_unlock":
824869
desc = "vault_unlock"
825870
result = await do_vault_unlock(content, owner=owner)
871+
elif tool in BUILTIN_EMAIL_TOOLS:
872+
# Bare email tool name from fenced-block models (e.g. Ollama) — route to MCP email server.
873+
# Non-admin owners never reach here: BUILTIN_EMAIL_TOOLS ⊆ NON_ADMIN_BLOCKED_TOOLS,
874+
# so is_public_blocked_tool() above already rejected them.
875+
mcp = get_mcp_manager()
876+
qualified = f"mcp__email__{tool}"
877+
desc = f"email: {tool}"
878+
if mcp:
879+
_raw = content.strip()
880+
args = {}
881+
_args_error = None
882+
if _raw:
883+
# A non-empty body is always meant to be the call's arguments,
884+
# and every email tool takes a JSON object. Anything that
885+
# isn't one is a correctable error — NOT a silent empty-args
886+
# call, which would read the DEFAULT mailbox/folder instead of
887+
# the one the model meant (#3966 class). Only an EMPTY body
888+
# keeps the no-arg path (e.g. ```list_email_accounts```).
889+
try:
890+
parsed = json.loads(_raw)
891+
except (json.JSONDecodeError, TypeError) as _je:
892+
# Covers both `{account: "work"}` (looks like JSON, bad)
893+
# and `account: work` (not JSON at all).
894+
_args_error = (
895+
f"'{tool}' arguments are not valid JSON ({_je}). "
896+
'Send a JSON object, e.g. {"account": "work"} — '
897+
"keys and string values need double quotes."
898+
)
899+
else:
900+
if isinstance(parsed, dict):
901+
args = parsed
902+
else:
903+
_args_error = (
904+
f"'{tool}' arguments must be a JSON object, "
905+
'e.g. {"uid": "..."} — got a JSON array/value instead.'
906+
)
907+
if _args_error is not None:
908+
result = {"error": _args_error, "exit_code": 1}
909+
else:
910+
result = await mcp.call_tool(qualified, args)
911+
else:
912+
result = {"error": "MCP manager not available", "exit_code": 1}
826913
elif tool.startswith("mcp__"):
827914
# MCP tool dispatch
828915
mcp = get_mcp_manager()

src/tool_parsing.py

Lines changed: 72 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,22 +10,74 @@
1010
import json
1111
import logging
1212
import re
13-
from typing import List, Optional
13+
from typing import List, Optional, Tuple
1414

1515
from src.agent_tools import ToolBlock, TOOL_TAGS
16+
from src.tool_security import BUILTIN_EMAIL_TOOLS
1617

1718
logger = logging.getLogger(__name__)
1819

1920
# ---------------------------------------------------------------------------
2021
# Regex patterns
2122
# ---------------------------------------------------------------------------
2223

23-
# Pattern 1: ```bash ... ``` fenced code blocks
24+
# Pattern 1: ```bash ... ``` fenced code blocks. The tag may be followed by a
25+
# newline (classic form) or by inline JSON args on the same line
26+
# (```list_email_accounts {}). The same-line part is captured separately
27+
# (group 2) and judged by _fenced_tool_call below — the regex alone only
28+
# requires it to start with { or [; anything else after the tag is a Markdown
29+
# info string (```python title="example.py") and the fence never matches.
30+
# (?![\w-]) keeps the alternation from prefix-matching longer fence tags:
31+
# without it, ```python3 would match as tool "python" with content "3\n..."
32+
# and execute as code.
2433
_TOOL_BLOCK_RE = re.compile(
25-
r"```(" + "|".join(TOOL_TAGS) + r")\s*\n([\s\S]*?)```",
34+
r"```(" + "|".join(TOOL_TAGS) + r")(?![\w-])"
35+
r"[ \t]*([{\[][^\n]*?)?[ \t]*(?=\r?\n|```)\r?\n?([\s\S]*?)```",
2636
re.IGNORECASE,
2737
)
2838

39+
# Tags whose fenced content is raw code, not JSON args. Same-line text after
40+
# these tags is Markdown fence metadata on a real language (```bash {title=
41+
# "setup"}), never inline tool args — only the classic tag-then-newline form
42+
# executes for them.
43+
_CODE_FENCE_TAGS = frozenset({"bash", "python"})
44+
45+
46+
def _fenced_tool_call(m) -> Optional[Tuple[str, str]]:
47+
"""Classify a Pattern-1 fence match: (tag, content) when it is an
48+
executable tool call, None when the fence must stay display text.
49+
50+
Shared by parse_tool_blocks and strip_tool_blocks so the execute and
51+
display decisions can never disagree: a fence that doesn't execute is
52+
never stripped, and vice versa.
53+
54+
Same-line text after the tag only counts as inline tool args when the
55+
tag's tool takes JSON args (not a code tag) AND the text is valid
56+
standalone JSON. ```bash {title="setup"} and ```python {"x": 1} are
57+
fence attributes on real languages, and {title="x"} on any tag is
58+
metadata, not arguments — all of those stay visible and inert.
59+
"""
60+
tag = m.group(1).lower()
61+
inline = (m.group(2) or "").strip()
62+
body = (m.group(3) or "").strip()
63+
if not inline:
64+
return tag, body
65+
if tag in _CODE_FENCE_TAGS:
66+
return None
67+
# Inline args may continue onto following lines (a JSON object opened on
68+
# the tag line); the combined text must parse as JSON or nothing runs.
69+
content = f"{inline}\n{body}" if body else inline
70+
try:
71+
json.loads(content)
72+
except (ValueError, TypeError):
73+
return None
74+
return tag, content
75+
76+
77+
def _strip_executed_fence(m) -> str:
78+
"""re.sub callback: remove only fences that parse as tool calls."""
79+
return "" if _fenced_tool_call(m) is not None else m.group(0)
80+
2981
# Pattern 2: [TOOL_CALL] ... [/TOOL_CALL] blocks (some models use this format)
3082
# Matches: {tool => "shell", args => {--command "ls -la"}} etc.
3183
_TOOL_CALL_RE = re.compile(
@@ -923,9 +975,20 @@ def parse_tool_blocks(text: str, skip_fenced: bool = False) -> List[ToolBlock]:
923975
# Pattern 1: fenced code blocks (skipped when `skip_fenced` — see docstring).
924976
if not skip_fenced:
925977
for m in _TOOL_BLOCK_RE.finditer(text):
926-
tag = m.group(1).lower()
927-
content = m.group(2).strip()
978+
call = _fenced_tool_call(m)
979+
if call is None:
980+
continue
981+
tag, content = call
928982
if not content:
983+
# An empty fence is still an unambiguous call for the email
984+
# tools — ```list_email_accounts``` with no body is a shape
985+
# local models really emit for no-arg tools. Dispatch with
986+
# empty args and let the tool's own validation answer;
987+
# silently dropping the call left models concluding email was
988+
# broken. Other tags (bash, python, ...) keep skipping: empty
989+
# content is nothing to run.
990+
if tag in BUILTIN_EMAIL_TOOLS:
991+
blocks.append(ToolBlock(tag, ""))
929992
continue
930993
# If a code block's content is an <invoke> XML call (some models wrap
931994
# tool calls in ```python or ```xml fences), parse the invoke instead.
@@ -1037,7 +1100,10 @@ def strip_tool_blocks(text: str, skip_fenced: bool = False) -> str:
10371100
# Normalize DSML first so its markup gets stripped by the <invoke>
10381101
# / <tool_call> removers below instead of leaking to the user.
10391102
text = _normalize_dsml(text)
1040-
cleaned = text if skip_fenced else _TOOL_BLOCK_RE.sub('', text)
1103+
# Keep the executed-vs-illustrative fence distinction (only strip fences
1104+
# that actually dispatched; leave example fences from native models inert
1105+
# but visible), then remove [TOOL_CALL]{...}[/TOOL_CALL] markup.
1106+
cleaned = text if skip_fenced else _TOOL_BLOCK_RE.sub(_strip_executed_fence, text)
10411107
# Forward-only removal mirrors parse_tool_blocks: _strip_delimited pairs each
10421108
# opener with a later closer and stops when none is reachable, so untrusted
10431109
# output can't drive the O(n^2) lazy-rescan (ReDoS); see _iter_delimited.

src/tool_schemas.py

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
from src.agent_tools import ToolBlock, TOOL_TAGS
1616
from src.tool_parsing import _TOOL_NAME_MAP
17+
from src.tool_security import BUILTIN_EMAIL_TOOLS
1718

1819
logger = logging.getLogger(__name__)
1920

@@ -1222,15 +1223,15 @@ def function_call_to_tool_block(name: str, arguments: str) -> Optional[ToolBlock
12221223
return None
12231224

12241225
tool_type = _TOOL_NAME_MAP.get(name, name)
1225-
_BUILTIN_EMAIL_TOOLS = {"list_email_accounts", "send_email", "list_emails", "read_email", "reply_to_email",
1226-
"archive_email", "delete_email", "mark_email_read", "bulk_email", "download_attachment"}
12271226

12281227
# Some models emit valid JSON that isn't an object (e.g. a bare array
12291228
# ["ls -la"], string, or number) as function arguments. Most local tools keep
12301229
# the legacy empty-object coercion for stream robustness, but email MCP tools
12311230
# must fail closed so a malformed call cannot read the default mailbox.
1231+
# Uses the shared BUILTIN_EMAIL_TOOLS (single source of truth) so the
1232+
# fail-closed set can't drift from the dispatch/blocklist sets.
12321233
if not isinstance(args, dict):
1233-
if tool_type.startswith("mcp__email__") or name in _BUILTIN_EMAIL_TOOLS:
1234+
if tool_type.startswith("mcp__email__") or name in BUILTIN_EMAIL_TOOLS:
12341235
logger.warning(f"Non-object email function call arguments for {name}: {args!r}; rejecting")
12351236
return None
12361237
logger.warning(f"Non-object function call arguments for {name}: {args!r}; treating as empty")
@@ -1241,7 +1242,7 @@ def function_call_to_tool_block(name: str, arguments: str) -> Optional[ToolBlock
12411242
content = json.dumps(args) if args else "{}"
12421243
return ToolBlock(tool_type, content)
12431244
# Email tools are implemented as MCP — route them to email
1244-
if name in _BUILTIN_EMAIL_TOOLS:
1245+
if name in BUILTIN_EMAIL_TOOLS:
12451246
return ToolBlock(f"mcp__email__{name}", json.dumps(args) if args else "{}")
12461247
if tool_type not in TOOL_TAGS:
12471248
logger.warning(f"Unknown function call: {name}")

0 commit comments

Comments
 (0)