mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-07-14 12:48:03 +00:00
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>
This commit is contained in:
@@ -0,0 +1,122 @@
|
||||
"""PR #3681 — every email-tool registry derives from / covers BUILTIN_EMAIL_TOOLS.
|
||||
|
||||
Three review rounds on #3681 each found another hand-maintained copy of the
|
||||
email tool list that had drifted (NON_ADMIN_BLOCKED_TOOLS, the `disable_tool
|
||||
email` alias, tool_schemas' private copy). This test pins ALL of them to the
|
||||
single source of truth, including the email MCP server itself, so adding or
|
||||
removing an email tool fails loudly everywhere it matters instead of silently
|
||||
shrinking one surface.
|
||||
"""
|
||||
import re
|
||||
from pathlib import Path
|
||||
|
||||
import src.agent_tools # noqa: F401 — resolve the circular-import cluster first
|
||||
from src.tool_security import (
|
||||
BUILTIN_EMAIL_TOOLS,
|
||||
NON_ADMIN_BLOCKED_TOOLS,
|
||||
PLAN_MODE_READONLY_TOOLS,
|
||||
_PLAN_MODE_KNOWN_MUTATORS,
|
||||
)
|
||||
|
||||
_REPO_ROOT = Path(__file__).resolve().parent.parent
|
||||
|
||||
# Read-only email tools — the only ones plan mode may execute.
|
||||
_READONLY_EMAIL = {"list_email_accounts", "list_emails", "read_email", "search_emails"}
|
||||
|
||||
|
||||
def test_email_server_tools_match_builtin_set():
|
||||
"""BUILTIN_EMAIL_TOOLS must equal exactly what the email server exposes."""
|
||||
source = (_REPO_ROOT / "mcp_servers" / "email_server.py").read_text()
|
||||
served = set(re.findall(r'Tool\(\s*name="(\w+)"', source))
|
||||
assert served == set(BUILTIN_EMAIL_TOOLS), (
|
||||
f"email_server tools != BUILTIN_EMAIL_TOOLS; "
|
||||
f"server-only: {sorted(served - BUILTIN_EMAIL_TOOLS)}, "
|
||||
f"set-only: {sorted(BUILTIN_EMAIL_TOOLS - served)}"
|
||||
)
|
||||
|
||||
|
||||
def test_fence_tags_cover_email_tools():
|
||||
from src.agent_tools import TOOL_TAGS
|
||||
|
||||
assert BUILTIN_EMAIL_TOOLS <= set(TOOL_TAGS)
|
||||
|
||||
|
||||
def test_function_schemas_cover_email_tools():
|
||||
"""Every email tool must be advertised to native function-calling models."""
|
||||
from src.tool_schemas import FUNCTION_TOOL_SCHEMAS
|
||||
|
||||
names = {(t.get("function") or {}).get("name") for t in FUNCTION_TOOL_SCHEMAS}
|
||||
missing = set(BUILTIN_EMAIL_TOOLS) - names
|
||||
assert not missing, f"email tools without native schemas: {sorted(missing)}"
|
||||
|
||||
|
||||
def test_prompt_sections_cover_email_tools():
|
||||
"""Every email tool must be describable to fenced-block models."""
|
||||
from src.agent_loop import TOOL_SECTIONS
|
||||
|
||||
missing = set(BUILTIN_EMAIL_TOOLS) - set(TOOL_SECTIONS)
|
||||
assert not missing, f"email tools without prompt sections: {sorted(missing)}"
|
||||
|
||||
|
||||
def test_domain_rules_map_covers_email_tools():
|
||||
from src.agent_loop import _DOMAIN_TOOL_MAP
|
||||
|
||||
assert BUILTIN_EMAIL_TOOLS <= _DOMAIN_TOOL_MAP["email"]
|
||||
|
||||
|
||||
def test_rag_descriptions_and_keyword_hints_cover_email_tools():
|
||||
"""Every email tool must be retrievable (embedding index + keyword hints)."""
|
||||
from src.tool_index import (
|
||||
ASSISTANT_ALWAYS_AVAILABLE,
|
||||
BUILTIN_TOOL_DESCRIPTIONS,
|
||||
)
|
||||
|
||||
missing = set(BUILTIN_EMAIL_TOOLS) - set(BUILTIN_TOOL_DESCRIPTIONS)
|
||||
assert not missing, f"email tools not in the RAG description index: {sorted(missing)}"
|
||||
assert BUILTIN_EMAIL_TOOLS <= ASSISTANT_ALWAYS_AVAILABLE
|
||||
|
||||
|
||||
def test_known_tool_names_cover_email_tools():
|
||||
from src.tool_policy import _COMMON_TOOL_NAMES
|
||||
|
||||
assert BUILTIN_EMAIL_TOOLS <= _COMMON_TOOL_NAMES
|
||||
|
||||
|
||||
def test_non_admin_blocklist_covers_email_tools():
|
||||
assert BUILTIN_EMAIL_TOOLS <= NON_ADMIN_BLOCKED_TOOLS
|
||||
|
||||
|
||||
def test_plan_mode_partitions_email_tools():
|
||||
"""Plan mode must classify every email tool: the read-only four are
|
||||
allowed (both spellings, given the bare/qualified alias gate), everything
|
||||
else is in the fail-closed mutator backstop. An unclassified tool would be
|
||||
allowed bare and blocked qualified — the round-2 aliasing bug again."""
|
||||
readonly = set(BUILTIN_EMAIL_TOOLS) & PLAN_MODE_READONLY_TOOLS
|
||||
assert readonly == _READONLY_EMAIL, (
|
||||
f"plan-mode read-only email subset drifted: {sorted(readonly)}"
|
||||
)
|
||||
mutators = set(BUILTIN_EMAIL_TOOLS) - _READONLY_EMAIL
|
||||
missing = mutators - _PLAN_MODE_KNOWN_MUTATORS
|
||||
assert not missing, f"mutating email tools not in plan-mode backstop: {sorted(missing)}"
|
||||
|
||||
|
||||
def test_frontend_tool_selector_covers_email_tools():
|
||||
"""The assistant tool-selector UI must be able to grant/revoke every email
|
||||
tool — a name missing here can never be toggled from the UI."""
|
||||
source = (_REPO_ROOT / "static" / "js" / "assistant.js").read_text()
|
||||
m = re.search(r"'Email':\s*\[([^\]]*)\]", source)
|
||||
assert m, "Email group not found in assistant.js TOOL_GROUPS"
|
||||
listed = set(re.findall(r"'(\w+)'", m.group(1)))
|
||||
assert listed == set(BUILTIN_EMAIL_TOOLS), (
|
||||
f"assistant.js Email group != BUILTIN_EMAIL_TOOLS; "
|
||||
f"js-only: {sorted(listed - BUILTIN_EMAIL_TOOLS)}, "
|
||||
f"missing: {sorted(BUILTIN_EMAIL_TOOLS - listed)}"
|
||||
)
|
||||
|
||||
|
||||
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"
|
||||
)
|
||||
@@ -136,3 +136,31 @@ def test_markdown_info_string_fence_is_left_intact_in_display():
|
||||
# so not stripped from the displayed text.
|
||||
text = 'Example:\n```python title="example.py"\nprint("hi")\n```'
|
||||
assert strip_tool_blocks(text) == text
|
||||
|
||||
|
||||
def test_parse_strip_mirror_across_fence_shape_grid():
|
||||
# Invariant for ANY single fence: either it executes AND is stripped, or
|
||||
# it doesn't execute AND stays fully visible. The one allowed exception is
|
||||
# an empty tool-shaped fence (no header, no body): never executed, but
|
||||
# stripped as noise — pre-PR behavior, kept deliberately.
|
||||
from src.agent_tools import TOOL_TAGS
|
||||
|
||||
tags = ["bash", "python", "list_emails", "bulk_email", "manage_memory",
|
||||
"python3", "bash-session", "notatool"]
|
||||
headers = ["", " ", ' title="x"', ' {title="x"}', ' {"a": 1}', " [1, 2]",
|
||||
" {bad json", ' {"a": 1} extra']
|
||||
bodies = ["", "content line\n", '{"k": "v"}\n']
|
||||
|
||||
for tag in tags:
|
||||
for header in headers:
|
||||
for body in bodies:
|
||||
text = f"before\n```{tag}{header}\n{body}```\nafter"
|
||||
blocks = parse_tool_blocks(text)
|
||||
stripped = strip_tool_blocks(text)
|
||||
case = (tag, header, body)
|
||||
if blocks:
|
||||
assert stripped == "before\n\nafter", case
|
||||
elif stripped != text:
|
||||
assert (
|
||||
tag in TOOL_TAGS and not header.strip() and not body.strip()
|
||||
), f"non-executed fence was stripped: {case}"
|
||||
|
||||
Reference in New Issue
Block a user