From 73fdfe93da1b8f0a7bf6c31ee4479cf5d7ec3381 Mon Sep 17 00:00:00 2001 From: Tapan Chugh Date: Sat, 12 Jul 2025 12:00:10 -0700 Subject: [PATCH 1/4] Fix MCP actions: offer a single invoke action tool instead of individual tools --- src/praga_core/action_executor.py | 18 +- .../integrations/mcp/descriptions.py | 272 +++++++++++++----- src/praga_core/integrations/mcp/server.py | 183 +----------- src/pragweb/google_api/client.py | 9 + src/pragweb/google_api/gmail/service.py | 122 +++++--- .../integration/test_mcp_gmail_integration.py | 186 ++++-------- tests/integration/test_mcp_real_actions.py | 271 +++++++++-------- tests/services/test_gmail_actions.py | 248 ++++++++++++++-- tests/services/test_gmail_service.py | 192 +++++++------ 9 files changed, 877 insertions(+), 624 deletions(-) diff --git a/src/praga_core/action_executor.py b/src/praga_core/action_executor.py index 5b8a7cb..0e13871 100644 --- a/src/praga_core/action_executor.py +++ b/src/praga_core/action_executor.py @@ -184,11 +184,23 @@ def _convert_page_type_to_uri_type(self, param_type: Any) -> Any: origin = get_origin(param_type) args = get_args(param_type) - if origin in (list, List) and args and self._is_page_type(args[0]): + # Handle Sequence[Page], List[Page], etc. + if origin in (list, List, Sequence) and args and self._is_page_type(args[0]): return List[PageURI] - if self._is_optional_page_type(param_type): - return Union[PageURI, None] + # Handle Optional types + if origin is Union and len(args) == 2 and type(None) in args: + non_none_type = args[0] if args[1] is type(None) else args[1] + # Optional[Page] -> Optional[PageURI] + if self._is_page_type(non_none_type): + return Union[PageURI, None] + # Optional[Sequence[Page]] -> Optional[List[PageURI]] + elif ( + get_origin(non_none_type) in (list, List, Sequence) + and get_args(non_none_type) + and self._is_page_type(get_args(non_none_type)[0]) + ): + return Union[List[PageURI], None] # For non-Page types, return unchanged return param_type diff --git a/src/praga_core/integrations/mcp/descriptions.py b/src/praga_core/integrations/mcp/descriptions.py index cfa6a48..04219fc 100644 --- a/src/praga_core/integrations/mcp/descriptions.py +++ b/src/praga_core/integrations/mcp/descriptions.py @@ -1,150 +1,286 @@ """Description templates for MCP tools and resources.""" import inspect -from typing import Any, List, Union, get_args, get_origin, get_type_hints +from typing import Any, Dict, List, Union, get_args, get_origin, get_type_hints from praga_core.action_executor import ActionFunction -from praga_core.types import Page +from praga_core.types import PageURI def get_search_tool_description(type_names: List[str]) -> str: """Generate description for the search_pages tool.""" return f"""Search for pages using natural language instructions. -Returns JSON with search results and resolved page content. +Returns JSON with search results and optionally resolved page content. Available page types: {', '.join(type_names)} +Parameters: +- instruction: Natural language search instruction (required) +- resolve_references: Whether to resolve page content in results (default: true) + Examples: - "Find emails from Alice about project X" - "Get calendar events for next week" - "Show me all documents mentioning quarterly report" +- "Search for person named John Smith" + +Response format: +- results: Array of search results with page references +- If resolve_references=true: Each result includes full page content +- If resolve_references=false: Each result includes only page URI and metadata """ def get_pages_tool_description(type_names: List[str]) -> str: """Generate description for the get_pages tool. Optionally accepts allow_stale to return invalid pages.""" - return f"""Get specific pages/documents by their type and ID(s). + return f"""Get specific pages/documents by their URIs. Returns JSON with complete page content and metadata. Supports both single page and bulk operations. Available page types: {', '.join(type_names)} -Single page examples: -- Get email: page_type="EmailPage", page_ids=["msg_12345"] -- Get calendar event: page_type="CalendarEventPage", page_ids=["event_67890"] -- Use aliases: page_type="Email", page_ids=["msg_12345"] +Parameters: +- page_uris: List of page URIs in format "PageType:id" +- allow_stale: Optional boolean to allow stale data (default: false) -Bulk examples: -- Get multiple emails: page_type="EmailPage", page_ids=["msg_1", "msg_2", "msg_3"] -- Get multiple events: page_type="CalendarEvent", page_ids=["event_1", "event_2"] +Examples: +- Single page: page_uris=["EmailPage:msg_12345"] +- Multiple pages: page_uris=["EmailPage:msg_1", "CalendarEventPage:event_2"] +- Full URI format: page_uris=["root/EmailPage:msg@1", "root/CalendarEventPage:event@2"] + +Response format: +- requested_count: Number of pages requested +- successful_count: Number of pages successfully retrieved +- error_count: Number of pages that failed +- pages: Array of successfully retrieved pages with uri, content, and status +- errors: Array of failed pages with uri, status, and error message (if any failures) """ -def get_action_tool_description(action_name: str, action_func: ActionFunction) -> str: - """Generate description for an action tool.""" - # Get function signature and docstring - sig = inspect.signature(action_func) - doc = inspect.getdoc(action_func) or "Perform an action on a page." +def get_invoke_action_tool_description(actions: Dict[str, ActionFunction]) -> str: + """Generate comprehensive description for the single invoke_action tool.""" + if not actions: + return """Execute any registered action on pages. + +This tool provides a unified interface for invoking actions on pages. Actions are operations that can modify page state or perform operations on pages. + +Available actions: No actions available + +Parameters: +- action_name: str - Name of the action to execute (required) +- action_input: Dict[str, Any] - Dictionary containing action parameters (required) + +Returns JSON with success status and error information: +- Success: {"success": true} +- Failure: {"success": false, "error": ""} +""" + + action_names = list(actions.keys()) + actions_text = ", ".join(action_names) + + # Generate detailed descriptions for each action + action_details = [] + for action_name, action_func in actions.items(): + action_detail = _generate_action_detail(action_name, action_func) + action_details.append(action_detail) + + action_details_text = "\n\n".join(action_details) + + return f"""Execute any registered action on pages. - # Get type hints for proper parameter descriptions +This tool provides a unified interface for invoking actions on pages. Actions are operations that can modify page state or perform operations on pages. + +Available actions: {actions_text} + +Parameters: +- action_name: str - Name of the action to execute (required) +- action_input: Dict[str, Any] - Dictionary containing action parameters (required) + +Action input format: +- Page parameters should be provided as string URIs +- Simple format: {{"email": "EmailPage:msg_12345"}} +- Full format: {{"email": "root/EmailPage:msg_12345@1"}} +- For actions with multiple parameters: {{"email": "EmailPage:msg_1", "mark_read": true}} + +Returns JSON with success status and error information: +- Success: {{"success": true}} +- Failure: {{"success": false, "error": ""}} + +## Available Actions + +{action_details_text} + +## Example Usage + +{_generate_example_usage(actions)} + +Note: This tool can execute any registered action. The specific parameters required depend on the action being invoked. +""" + + +def _generate_action_detail(action_name: str, action_func: ActionFunction) -> str: + """Generate detailed description for a single action.""" + # Get docstring + doc = inspect.getdoc(action_func) or "No description available." + + # Get function signature and type hints + sig = inspect.signature(action_func) try: type_hints = get_type_hints(action_func) except Exception: type_hints = {} + # Generate parameter descriptions param_descriptions = [] for param_name, param in sig.parameters.items(): - # Get the type annotation - transform Page types to PageURI param_type = type_hints.get(param_name, param.annotation) if param_type == inspect.Parameter.empty: param_type = "Any" - # Transform Page types to PageURI types for description + # Transform Page types to URI types for description transformed_type = _convert_page_type_to_uri_type_for_description(param_type) + # Check if parameter has default value + has_default = param.default != inspect.Parameter.empty default_text = ( - f" (default: {param.default})" - if param.default != inspect.Parameter.empty - else "" + f" (optional, default: {param.default})" if has_default else " (required)" ) - param_descriptions.append(f"- {param_name}: {transformed_type}{default_text}") + + param_descriptions.append(f" - {param_name}: {transformed_type}{default_text}") param_text = ( - "\n".join(param_descriptions) - if param_descriptions - else "No parameters required." + "\n".join(param_descriptions) if param_descriptions else " No parameters" ) - return f"""Action: {action_name} + return f"""### {action_name} {doc} Parameters: -{param_text} - -Returns JSON with success status (true/false) and any error message if the action fails. - -Example usage: -- Provide the page URI and any additional parameters required by the action -- The action will be executed on the specified page -- Returns {{"success": true}} on success or {{"success": false, "error": "..."}} on failure -""" +{param_text}""" def _convert_page_type_to_uri_type_for_description(param_type: Any) -> str: - """Convert Page-related type annotations to PageURI equivalents for descriptions.""" - # Handle direct Page type - if _is_page_type(param_type): - return "PageURI" + """Convert Page-related type annotations to string equivalents for descriptions.""" + # Handle direct PageURI type + if param_type is PageURI: + return "str (page URI)" - # Handle generic types like List[Page], Optional[Page], etc. + # Handle generic types like List[PageURI], Optional[PageURI], etc. origin = get_origin(param_type) args = get_args(param_type) - if origin in (list, List): - if args and _is_page_type(args[0]): - return "List[PageURI]" - elif args: - # Handle List[SomePageType] - inner_type = _convert_page_type_to_uri_type_for_description(args[0]) - return f"List[{inner_type}]" - else: - return "List" - - if _is_optional_page_type(param_type): - return "Optional[PageURI]" + if origin in (list, List) and args and args[0] is PageURI: + return "List[str] (page URIs)" - # Handle Optional[List[Page]] and similar complex types - if origin is Union: - non_none_types = [arg for arg in args if arg is not type(None)] - if len(non_none_types) == 1: - inner_desc = _convert_page_type_to_uri_type_for_description( - non_none_types[0] - ) - return f"Optional[{inner_desc}]" - - # For non-Page types, return string representation + if origin is Union and len(args) == 2 and type(None) in args: + non_none_type = args[0] if args[1] is type(None) else args[1] + if non_none_type is PageURI: + return "Optional[str] (page URI)" + # Handle Optional[List[PageURI]] + elif ( + get_origin(non_none_type) is list + and get_args(non_none_type) + and get_args(non_none_type)[0] is PageURI + ): + return "Optional[List[str]] (page URIs)" + + # For non-PageURI types, return string representation if hasattr(param_type, "__name__"): return str(param_type.__name__) else: return str(param_type) +def _generate_example_usage(actions: Dict[str, ActionFunction]) -> str: + """Generate example usage for actions.""" + if not actions: + return "No actions available for examples." + + examples = [] + for action_name, action_func in actions.items(): + example = _generate_action_example(action_name, action_func) + if example: + examples.append(example) + + return "\n".join(examples[:3]) # Show up to 3 examples to keep it manageable + + +def _generate_action_example(action_name: str, action_func: ActionFunction) -> str: + """Generate a usage example for a specific action.""" + sig = inspect.signature(action_func) + try: + type_hints = get_type_hints(action_func) + except Exception: + type_hints = {} + + # Build example action_input + example_params: Dict[str, Any] = {} + for param_name, param in sig.parameters.items(): + param_type = type_hints.get(param_name, param.annotation) + + # Generate example values based on parameter type + if param_type is PageURI or _is_page_type(param_type): + # Use the parameter name to infer page type + if "email" in param_name.lower(): + example_params[param_name] = "EmailPage:msg_12345" + elif "person" in param_name.lower(): + example_params[param_name] = "PersonPage:person_123" + elif "thread" in param_name.lower(): + example_params[param_name] = "EmailThreadPage:thread_456" + else: + # Generic page URI + example_params[param_name] = "SomePage:page_123" + elif _is_list_page_type(param_type): + # List of pages + if "email" in param_name.lower(): + example_params[param_name] = ["EmailPage:msg_1", "EmailPage:msg_2"] + elif "person" in param_name.lower() or "recipient" in param_name.lower(): + example_params[param_name] = [ + "PersonPage:person_1", + "PersonPage:person_2", + ] + else: + example_params[param_name] = ["SomePage:page_1", "SomePage:page_2"] + elif param_type == str or param_type is str: + # String parameters + if "message" in param_name.lower(): + example_params[param_name] = "Your message here" + elif "subject" in param_name.lower(): + example_params[param_name] = "Email subject" + else: + example_params[param_name] = f"example_{param_name}" + elif param_type == bool or param_type is bool: + example_params[param_name] = True + elif param.default != inspect.Parameter.empty: + # Skip optional parameters with defaults + continue + else: + # For other types, use a generic example + example_params[param_name] = f"<{param_name}_value>" + + if not example_params: + return f'- action_name="{action_name}", action_input={{}}' + + return f'- action_name="{action_name}", action_input={example_params}' + + def _is_page_type(param_type: Any) -> bool: """Check if a type is Page or a subclass of Page.""" + from praga_core.types import Page + return param_type is Page or ( isinstance(param_type, type) and issubclass(param_type, Page) ) -def _is_optional_page_type(param_type: Any) -> bool: - """Check if a type is Optional[Page] or similar union with None.""" +def _is_list_page_type(param_type: Any) -> bool: + """Check if a type is List[Page] or similar.""" origin = get_origin(param_type) args = get_args(param_type) - if origin is Union and len(args) == 2 and type(None) in args: - non_none_type = args[0] if args[1] is type(None) else args[1] - return _is_page_type(non_none_type) + if origin in (list, List) and args: + return _is_page_type(args[0]) return False diff --git a/src/praga_core/integrations/mcp/server.py b/src/praga_core/integrations/mcp/server.py index 5f40187..b02fa24 100644 --- a/src/praga_core/integrations/mcp/server.py +++ b/src/praga_core/integrations/mcp/server.py @@ -1,28 +1,17 @@ """Main MCP server implementation for Praga Core.""" -import inspect import json import logging -from typing import ( - Any, - List, - Optional, - Union, - get_args, - get_origin, - get_type_hints, -) +from typing import Any, Dict, List, Optional from fastmcp import Context, FastMCP -from praga_core.action_executor import ActionFunction from praga_core.context import ServerContext from praga_core.integrations.mcp.descriptions import ( - get_action_tool_description, + get_invoke_action_tool_description, get_pages_tool_description, get_search_tool_description, ) -from praga_core.types import Page, PageURI logging.basicConfig(level=logging.INFO) logger = logging.getLogger(__name__) @@ -181,77 +170,38 @@ async def get_pages( await ctx.error(error_msg) raise RuntimeError(error_msg) - # Setup action tools dynamically - setup_action_tools(mcp, server_context) + # Setup a single invoke_action tool + setup_invoke_action_tool(mcp, server_context) -def setup_action_tools( +def setup_invoke_action_tool( mcp: FastMCP, # type: ignore[type-arg] server_context: ServerContext, ) -> None: - """Setup MCP tools for registered actions. + """Setup a single MCP tool for invoking any registered action. Args: mcp: FastMCP server instance server_context: ServerContext with registered actions """ - # Get all registered actions + # Get all registered actions for the description actions = server_context.actions - for action_name, action_func in actions.items(): - # Create a unique tool for each action - _create_individual_action_tool(mcp, server_context, action_name, action_func) - - -def _create_individual_action_tool( - mcp: FastMCP, # type: ignore[type-arg] - server_context: ServerContext, - action_name: str, - action_func: ActionFunction, -) -> None: - """Create an individual MCP tool for a specific action. - - Args: - mcp: FastMCP server instance - server_context: ServerContext with registered actions - action_name: Name of the action - action_func: Action function - """ - # Generate description for this specific action - description = get_action_tool_description(action_name, action_func) - - # Get the transformed signature (Page -> PageURI) from the action function - sig = inspect.signature(action_func) - - # Create parameter list for the dynamic function - param_names = list(sig.parameters.keys()) - - # Create a dynamic function with proper parameter signature - async def dynamic_action_tool( - *args: Any, ctx: Optional[Context] = None, **kwargs: Any + @mcp.tool(description=get_invoke_action_tool_description(actions)) + async def invoke_action( + action_name: str, action_input: Dict[str, Any], ctx: Optional[Context] = None ) -> str: - """Execute an action on a page. + """Execute an action on pages. Args: - *args: Positional arguments based on action signature + action_name: Name of the action to execute + action_input: Dictionary containing the action parameters ctx: MCP context for logging - **kwargs: Keyword arguments based on action signature Returns: JSON string containing action result with success status """ try: - # Build action_input dict from args and kwargs - action_input = {} - - # Map positional args to parameter names - for i, arg in enumerate(args): - if i < len(param_names): - action_input[param_names[i]] = arg - - # Add keyword arguments - action_input.update(kwargs) - if ctx: await ctx.info(f"Executing action: {action_name}") await ctx.info(f"Action input: {action_input}") @@ -270,110 +220,3 @@ async def dynamic_action_tool( await ctx.error(error_msg) return json.dumps({"success": False, "error": str(e)}, indent=2) - - # Set the proper signature on the dynamic function - _set_dynamic_function_signature(dynamic_action_tool, action_func, action_name) - - # Register the tool with the MCP server - mcp.tool(description=description)(dynamic_action_tool) - - -def _set_dynamic_function_signature( - dynamic_func: Any, - original_func: ActionFunction, - action_name: str, -) -> None: - """Set the proper signature on a dynamic function based on the original action function. - - This transforms Page types to PageURI types and creates explicit parameters. - """ - try: - # Get original signature and type hints - original_sig = inspect.signature(original_func) - original_type_hints = get_type_hints(original_func) - - # Create new parameters with transformed types - new_params = [] - for param_name, param in original_sig.parameters.items(): - # Get the type annotation - param_type = original_type_hints.get(param_name, param.annotation) - - # Transform Page types to PageURI types - transformed_type = _convert_page_type_to_uri_type(param_type) - - # Create new parameter with transformed type - new_param = param.replace(annotation=transformed_type) - new_params.append(new_param) - - # Add the ctx parameter for MCP context - ctx_param = inspect.Parameter( - "ctx", - inspect.Parameter.KEYWORD_ONLY, - default=None, - annotation=Optional[Context], - ) - new_params.append(ctx_param) - - # Create new signature - new_sig = inspect.Signature( - parameters=new_params, return_annotation=str # MCP tools return strings - ) - - # Set the signature and name on the dynamic function - dynamic_func.__signature__ = new_sig - dynamic_func.__name__ = f"{action_name}_tool" - dynamic_func.__qualname__ = f"{action_name}_tool" - - # Set type annotations for better introspection - dynamic_func.__annotations__ = { - param.name: param.annotation for param in new_params - } - dynamic_func.__annotations__["return"] = str - - except Exception as e: - logger.warning(f"Failed to set signature for action {action_name}: {e}") - # Fallback to basic naming - dynamic_func.__name__ = f"{action_name}_tool" - dynamic_func.__qualname__ = f"{action_name}_tool" - - -def _convert_page_type_to_uri_type(param_type: Any) -> Any: - """Convert Page-related type annotations to PageURI equivalents. - - This is similar to the logic in ActionExecutorMixin but adapted for MCP. - """ - # Direct Page type -> PageURI - if _is_page_type(param_type): - return PageURI - - # Handle generic types like List[Page], Optional[Page], etc. - origin = get_origin(param_type) - args = get_args(param_type) - - if origin in (list, List) and args and _is_page_type(args[0]): - return List[PageURI] - - if _is_optional_page_type(param_type): - return Union[PageURI, None] - - # For non-Page types, return unchanged - return param_type - - -def _is_page_type(param_type: Any) -> bool: - """Check if a type is Page or a subclass of Page.""" - return param_type is Page or ( - isinstance(param_type, type) and issubclass(param_type, Page) - ) - - -def _is_optional_page_type(param_type: Any) -> bool: - """Check if a type is Optional[Page] or similar union with None.""" - origin = get_origin(param_type) - args = get_args(param_type) - - if origin is Union and len(args) == 2 and type(None) in args: - non_none_type = args[0] if args[1] is type(None) else args[1] - return _is_page_type(non_none_type) - - return False diff --git a/src/pragweb/google_api/client.py b/src/pragweb/google_api/client.py index 08c36b6..bcfca11 100644 --- a/src/pragweb/google_api/client.py +++ b/src/pragweb/google_api/client.py @@ -71,6 +71,15 @@ async def search_messages( return messages, next_token + async def get_user_profile(self) -> Dict[str, Any]: + """Get the current user's Gmail profile.""" + loop = asyncio.get_event_loop() + result = await loop.run_in_executor( + self._executor, + lambda: self._gmail.users().getProfile(userId="me").execute(), + ) + return result # type: ignore + async def send_message( self, to: List[str], diff --git a/src/pragweb/google_api/gmail/service.py b/src/pragweb/google_api/gmail/service.py index f51ee64..c748ad3 100644 --- a/src/pragweb/google_api/gmail/service.py +++ b/src/pragweb/google_api/gmail/service.py @@ -2,7 +2,7 @@ import logging from datetime import datetime -from email.utils import parsedate_to_datetime +from email.utils import parseaddr, parsedate_to_datetime from typing import Any, List, Optional, Tuple from praga_core.agents import PaginatedResponse, tool @@ -11,7 +11,6 @@ from ..client import GoogleAPIClient from ..people.page import PersonPage -from ..people.service import PeopleService from ..utils import resolve_person_identifier from .page import EmailPage, EmailSummary, EmailThreadPage from .utils import GmailParser @@ -25,11 +24,24 @@ class GmailService(ToolkitService): def __init__(self, api_client: GoogleAPIClient) -> None: super().__init__(api_client) self.parser = GmailParser() + self._current_user_email: Optional[str] = None # Register handlers using decorators self._register_handlers() logger.info("Gmail service initialized and handlers registered") + async def _get_current_user_email(self) -> Optional[str]: + """Get current user email, fetching it if not already cached.""" + if self._current_user_email is None: + try: + profile = await self.api_client.get_user_profile() + self._current_user_email = profile.get("emailAddress", "").lower() + logger.debug(f"Current user email: {self._current_user_email}") + except Exception as e: + logger.warning(f"Could not get current user email: {e}") + self._current_user_email = "" + return self._current_user_email if self._current_user_email else None + def _register_handlers(self) -> None: """Register handlers with context using decorators.""" ctx = self.context @@ -103,13 +115,29 @@ def _parse_message_content(self, message: dict[str, Any]) -> dict[str, Any]: # Parse basic fields subject = headers.get("Subject", "") - sender = headers.get("From", "") - recipients = headers.get("To", "").split(",") if headers.get("To") else [] - cc_list = headers.get("Cc", "").split(",") if headers.get("Cc") else [] - # Clean up recipient lists - recipients = [r.strip() for r in recipients if r.strip()] - cc_list = [cc.strip() for cc in cc_list if cc.strip()] + # Extract sender email address + sender_header = headers.get("From", "") + _, sender_email = parseaddr(sender_header) + sender = sender_email if sender_email and "@" in sender_email else sender_header + + # Extract recipient email addresses + recipients_header = headers.get("To", "") + recipients = [] + if recipients_header: + for addr in recipients_header.split(","): + _, email = parseaddr(addr.strip()) + if email and "@" in email: + recipients.append(email) + + # Extract CC email addresses + cc_header = headers.get("Cc", "") + cc_list = [] + if cc_header: + for addr in cc_header.split(","): + _, email = parseaddr(addr.strip()) + if email and "@" in email: + cc_list.append(email) # Extract body using parser body = self.parser.extract_body(message.get("payload", {})) @@ -367,36 +395,50 @@ async def _reply_to_thread_internal( # Fetch the full email page page = await self.context.get_page(latest_email_uri) if not isinstance(page, EmailPage): - logger.error(f"Failed to get email page for {latest_email_uri}") - return False + error_msg = f"Failed to get email page for {latest_email_uri}" + logger.error(error_msg) + raise ValueError(error_msg) email = page if email is None: - logger.error("No email to reply to in thread") - return False - - # Determine recipients if not provided - if recipients is None: - # Default to replying to the sender of the email being replied to - sender_email = email.sender - # Try to find person page for sender - try: - people_service = self.context.get_service("people") - if isinstance(people_service, PeopleService): - sender_people = await people_service.search_existing_records( - sender_email - ) - else: - logger.warning("People service is not a PeopleService instance") - sender_people = [] - except Exception as e: - logger.warning(f"Could not find people service or sender: {e}") - sender_people = [] - recipients = sender_people[:1] if sender_people else [] - - # Convert PersonPage objects to email addresses - to_emails = [person.email for person in (recipients or [])] - cc_emails = [person.email for person in (cc_list or [])] + error_msg = "No email to reply to in thread" + logger.error(error_msg) + raise ValueError(error_msg) + + # Determine recipients and CC if not provided - Reply All behavior + if recipients is None and cc_list is None: + # Default Reply All behavior: include sender + original recipients as recipients, + # and original CC list as CC (excluding current user) + + # Get current user email to exclude from reply + current_user_email = await self._get_current_user_email() + + # Build recipient list: sender + original recipients (excluding current user) + to_emails = [] + + # Add sender as recipient + if email.sender and email.sender.lower() != current_user_email: + to_emails.append(email.sender) + + # Add original recipients (excluding current user) + for recipient in email.recipients: + if ( + recipient.lower() != current_user_email + and recipient not in to_emails + ): + to_emails.append(recipient) + + # Add original CC as CC (excluding current user) + cc_emails = [] + for cc in email.cc_list: + if cc.lower() != current_user_email: + cc_emails.append(cc) + + logger.info(f"Reply All - to: {to_emails}, cc: {cc_emails}") + else: + # Use provided recipients and CC - convert PersonPage objects to email addresses + to_emails = [person.email for person in (recipients or [])] + cc_emails = [person.email for person in (cc_list or [])] # Prepare the reply subject = email.subject @@ -418,8 +460,9 @@ async def _reply_to_thread_internal( return True except Exception as e: - logger.error(f"Failed to reply to thread: {e}") - return False + error_msg = f"Failed to reply to thread: {e}" + logger.error(error_msg) + raise RuntimeError(error_msg) from e async def _send_email_internal( self, @@ -450,8 +493,9 @@ async def _send_email_internal( return True except Exception as e: - logger.error(f"Failed to send email: {e}") - return False + error_msg = f"Failed to send email: {e}" + logger.error(error_msg) + raise RuntimeError(error_msg) from e @property def name(self) -> str: diff --git a/tests/integration/test_mcp_gmail_integration.py b/tests/integration/test_mcp_gmail_integration.py index d4b1bc7..36fb794 100644 --- a/tests/integration/test_mcp_gmail_integration.py +++ b/tests/integration/test_mcp_gmail_integration.py @@ -55,72 +55,53 @@ def mcp_server_with_gmail(self, context_with_gmail): context, gmail_service, mock_client = context_with_gmail return create_mcp_server(context), context, gmail_service, mock_client - async def test_gmail_actions_registered_as_separate_tools( + async def test_gmail_actions_available_via_invoke_action( self, mcp_server_with_gmail ): - """Test that Gmail actions are registered as separate MCP tools.""" + """Test that Gmail actions are available via the single invoke_action tool.""" mcp_server, context, gmail_service, mock_client = mcp_server_with_gmail tools = await mcp_server.get_tools() tool_names = [tool for tool in tools] - # Check that Gmail action tools are registered with correct names - assert "reply_to_email_thread_tool" in tool_names - assert "send_email_tool" in tool_names + # Check that invoke_action tool is registered + assert "invoke_action" in tool_names - # Verify no generic action_tool exists - assert "action_tool" not in tool_names + # Verify no individual action tools exist + assert "reply_to_email_thread_tool" not in tool_names + assert "send_email_tool" not in tool_names - async def test_reply_to_email_thread_tool_exists(self, mcp_server_with_gmail): - """Test that reply_to_email_thread_tool exists and has correct description.""" - mcp_server, context, gmail_service, mock_client = mcp_server_with_gmail - - # Get the reply tool - reply_tool = await mcp_server.get_tool("reply_to_email_thread_tool") - assert reply_tool is not None - - # Check that the description contains expected information - description = reply_tool.description + # Get the invoke_action tool and check it lists Gmail actions + invoke_tool = await mcp_server.get_tool("invoke_action") + description = invoke_tool.description assert "reply_to_email_thread" in description - assert "Reply to an email thread" in description - assert "thread:" in description # Should show parameters - - async def test_send_email_tool_exists(self, mcp_server_with_gmail): - """Test that send_email_tool exists and has correct description.""" - mcp_server, context, gmail_service, mock_client = mcp_server_with_gmail - - # Get the send tool - send_tool = await mcp_server.get_tool("send_email_tool") - assert send_tool is not None - - # Check that the description contains expected information - description = send_tool.description assert "send_email" in description - assert "Send a new email" in description - assert "person:" in description # Should show parameters - async def test_reply_to_email_thread_tool_execution(self, mcp_server_with_gmail): - """Test executing the reply_to_email_thread_tool.""" + async def test_reply_to_email_thread_action_execution(self, mcp_server_with_gmail): + """Test executing reply_to_email_thread action via invoke_action tool.""" mcp_server, context, gmail_service, mock_client = mcp_server_with_gmail # Mock the invoke_action method context.invoke_action = AsyncMock(return_value={"success": True}) - # Get the reply tool - reply_tool = await mcp_server.get_tool("reply_to_email_thread_tool") - - # Execute the tool with explicit parameters - result = await reply_tool.fn( - thread="EmailThreadPage:thread123", - email="EmailPage:email456", - recipients=["PersonPage:person1"], - cc_list=["PersonPage:person2"], - message="This is a test reply", + # Get the invoke_action tool + invoke_tool = await mcp_server.get_tool("invoke_action") + + # Execute the tool with action_name and action_input + result = await invoke_tool.fn( + action_name="reply_to_email_thread", + action_input={ + "thread": "EmailThreadPage:thread123", + "email": "EmailPage:email456", + "recipients": ["PersonPage:person1"], + "cc_list": ["PersonPage:person2"], + "message": "This is a test reply", + }, ) # Verify the result result_data = json.loads(result) - assert result_data == {"success": True} + assert result_data["success"] is True # Verify the action was called correctly expected_action_input = { @@ -134,28 +115,31 @@ async def test_reply_to_email_thread_tool_execution(self, mcp_server_with_gmail) "reply_to_email_thread", expected_action_input ) - async def test_send_email_tool_execution(self, mcp_server_with_gmail): - """Test executing the send_email_tool.""" + async def test_send_email_action_execution(self, mcp_server_with_gmail): + """Test executing send_email action via invoke_action tool.""" mcp_server, context, gmail_service, mock_client = mcp_server_with_gmail # Mock the invoke_action method context.invoke_action = AsyncMock(return_value={"success": True}) - # Get the send tool - send_tool = await mcp_server.get_tool("send_email_tool") - - # Execute the tool with explicit parameters - result = await send_tool.fn( - person="PersonPage:person1", - additional_recipients=["PersonPage:person2"], - cc_list=["PersonPage:person3"], - subject="Test Email Subject", - message="This is a test email message", + # Get the invoke_action tool + invoke_tool = await mcp_server.get_tool("invoke_action") + + # Execute the tool with action_name and action_input + result = await invoke_tool.fn( + action_name="send_email", + action_input={ + "person": "PersonPage:person1", + "additional_recipients": ["PersonPage:person2"], + "cc_list": ["PersonPage:person3"], + "subject": "Test Email Subject", + "message": "This is a test email message", + }, ) # Verify the result result_data = json.loads(result) - assert result_data == {"success": True} + assert result_data["success"] is True # Verify the action was called correctly expected_action_input = { @@ -169,8 +153,8 @@ async def test_send_email_tool_execution(self, mcp_server_with_gmail): "send_email", expected_action_input ) - async def test_action_tool_error_handling(self, mcp_server_with_gmail): - """Test error handling in action tools.""" + async def test_invoke_action_error_handling(self, mcp_server_with_gmail): + """Test error handling in invoke_action tool.""" mcp_server, context, gmail_service, mock_client = mcp_server_with_gmail # Mock the invoke_action method to raise an exception @@ -178,14 +162,17 @@ async def test_action_tool_error_handling(self, mcp_server_with_gmail): side_effect=ValueError("Email sending failed") ) - # Get the send tool - send_tool = await mcp_server.get_tool("send_email_tool") - - # Execute the tool with explicit parameters - result = await send_tool.fn( - person="PersonPage:person1", - subject="Test Subject", - message="Test message", + # Get the invoke_action tool + invoke_tool = await mcp_server.get_tool("invoke_action") + + # Execute the tool with parameters that will cause an error + result = await invoke_tool.fn( + action_name="send_email", + action_input={ + "person": "PersonPage:person1", + "subject": "Test Subject", + "message": "Test message", + }, ) # Verify the error result @@ -193,31 +180,6 @@ async def test_action_tool_error_handling(self, mcp_server_with_gmail): assert result_data["success"] is False assert "Email sending failed" in result_data["error"] - async def test_tools_have_unique_names_and_descriptions( - self, mcp_server_with_gmail - ): - """Test that each Gmail action tool has a unique name and description.""" - mcp_server, context, gmail_service, mock_client = mcp_server_with_gmail - - # Get the Gmail action tools - reply_tool = await mcp_server.get_tool("reply_to_email_thread_tool") - send_tool = await mcp_server.get_tool("send_email_tool") - - assert reply_tool is not None - assert send_tool is not None - - # Verify names are different - assert reply_tool != send_tool - - # Verify descriptions are different - assert reply_tool.description != send_tool.description - - # Verify each description is specific to its action - assert "reply_to_email_thread" in reply_tool.description - assert "reply_to_email_thread" not in send_tool.description - assert "send_email" in send_tool.description - assert "send_email" not in reply_tool.description - async def test_mcp_server_with_all_services(self): """Test MCP server creation with all Google services.""" # Clear any existing global context @@ -267,48 +229,22 @@ async def test_mcp_server_with_all_services(self): # Get all tools tools = await mcp_server.get_tools() - # Should have core tools + Gmail action tools + # Should have core tools + single invoke_action tool # Core tools: search_pages, get_pages - # Gmail action tools: reply_to_email_thread_tool, send_email_tool + # Action tool: invoke_action expected_tools = { "search_pages", "get_pages", - "reply_to_email_thread_tool", - "send_email_tool", + "invoke_action", } actual_tools = set(tools) assert expected_tools.issubset(actual_tools) - # Verify no generic action_tool exists - assert "action_tool" not in actual_tools + # Verify no individual action tools exist + assert "reply_to_email_thread_tool" not in actual_tools + assert "send_email_tool" not in actual_tools finally: # Clean up global context after test clear_global_context() - - async def test_tool_parameter_extraction(self, mcp_server_with_gmail): - """Test that tool descriptions correctly extract parameters.""" - mcp_server, context, gmail_service, mock_client = mcp_server_with_gmail - - # Get the reply tool - reply_tool = await mcp_server.get_tool("reply_to_email_thread_tool") - description = reply_tool.description - - # Check that all expected parameters are documented - expected_params = [ - "thread:", - "email:", - "recipients:", - "cc_list:", - "message:", - ] - - for param in expected_params: - assert param in description, f"Parameter {param} not found in description" - - # Check parameter types are shown (transformed to PageURI types) - assert "PageURI" in description - assert "Optional[PageURI]" in description - assert "Optional[List[PageURI]]" in description - assert "str" in description diff --git a/tests/integration/test_mcp_real_actions.py b/tests/integration/test_mcp_real_actions.py index edf181a..89ef820 100644 --- a/tests/integration/test_mcp_real_actions.py +++ b/tests/integration/test_mcp_real_actions.py @@ -87,52 +87,55 @@ def mcp_server_with_actions(self, context_with_actions): """Create MCP server with realistic actions.""" return create_mcp_server(context_with_actions) - async def test_realistic_action_tools_registered(self, mcp_server_with_actions): - """Test that realistic action tools are registered as separate tools.""" + async def test_single_invoke_action_tool_registered(self, mcp_server_with_actions): + """Test that a single invoke_action tool is registered.""" tools = await mcp_server_with_actions.get_tools() tool_names = [tool for tool in tools] - # Check that action tools are registered with correct names - assert "send_email_tool" in tool_names - assert "reply_to_email_tool" in tool_names - assert "mark_email_read_tool" in tool_names + # Check that invoke_action tool is registered + assert "invoke_action" in tool_names # Check core tools are also present assert "search_pages" in tool_names assert "get_pages" in tool_names - # Verify no generic action_tool exists - assert "action_tool" not in tool_names + # Verify no individual action tools exist + assert "send_email_tool" not in tool_names + assert "reply_to_email_tool" not in tool_names + assert "mark_email_read_tool" not in tool_names - # Should have 5 tools total (3 actions + 2 core) - assert len(tool_names) == 5 + # Should have 3 tools total (1 invoke_action + 2 core) + assert len(tool_names) == 3 - async def test_send_email_tool_execution( + async def test_send_email_action_execution( self, mcp_server_with_actions, context_with_actions ): - """Test executing the send_email_tool with realistic parameters.""" + """Test executing send_email action via invoke_action tool.""" # Mock the invoke_action method context_with_actions.invoke_action = AsyncMock(return_value={"success": True}) - # Get the send email tool - send_tool = await mcp_server_with_actions.get_tool("send_email_tool") - assert send_tool is not None - - # Execute the tool with explicit parameters - result = await send_tool.fn( - person="SamplePersonPage:recipient1", - additional_recipients=[ - "SamplePersonPage:recipient2", - "SamplePersonPage:recipient3", - ], - cc_list=["SamplePersonPage:cc1"], - subject="Test Email Subject", - message="This is a test email message", + # Get the invoke_action tool + invoke_tool = await mcp_server_with_actions.get_tool("invoke_action") + assert invoke_tool is not None + + # Execute the tool with action_name and action_input + result = await invoke_tool.fn( + action_name="send_email", + action_input={ + "person": "SamplePersonPage:recipient1", + "additional_recipients": [ + "SamplePersonPage:recipient2", + "SamplePersonPage:recipient3", + ], + "cc_list": ["SamplePersonPage:cc1"], + "subject": "Test Email Subject", + "message": "This is a test email message", + }, ) # Verify the result result_data = json.loads(result) - assert result_data == {"success": True} + assert result_data["success"] is True # Verify the action was called correctly expected_action_input = { @@ -149,28 +152,31 @@ async def test_send_email_tool_execution( "send_email", expected_action_input ) - async def test_reply_to_email_tool_execution( + async def test_reply_to_email_action_execution( self, mcp_server_with_actions, context_with_actions ): - """Test executing the reply_to_email_tool.""" + """Test executing reply_to_email action via invoke_action tool.""" # Mock the invoke_action method context_with_actions.invoke_action = AsyncMock(return_value={"success": True}) - # Get the reply tool - reply_tool = await mcp_server_with_actions.get_tool("reply_to_email_tool") - assert reply_tool is not None - - # Execute the tool with explicit parameters - result = await reply_tool.fn( - email="SampleEmailPage:email123", - recipients=["SamplePersonPage:person1", "SamplePersonPage:person2"], - cc_list=["SamplePersonPage:cc1"], - message="This is a reply message", + # Get the invoke_action tool + invoke_tool = await mcp_server_with_actions.get_tool("invoke_action") + assert invoke_tool is not None + + # Execute the tool with action_name and action_input + result = await invoke_tool.fn( + action_name="reply_to_email", + action_input={ + "email": "SampleEmailPage:email123", + "recipients": ["SamplePersonPage:person1", "SamplePersonPage:person2"], + "cc_list": ["SamplePersonPage:cc1"], + "message": "This is a reply message", + }, ) # Verify the result result_data = json.loads(result) - assert result_data == {"success": True} + assert result_data["success"] is True # Verify the action was called correctly expected_action_input = { @@ -183,25 +189,28 @@ async def test_reply_to_email_tool_execution( "reply_to_email", expected_action_input ) - async def test_mark_email_read_tool_execution( + async def test_mark_email_read_action_execution( self, mcp_server_with_actions, context_with_actions ): - """Test executing the mark_email_read_tool.""" + """Test executing mark_email_read action via invoke_action tool.""" # Mock the invoke_action method context_with_actions.invoke_action = AsyncMock(return_value={"success": True}) - # Get the mark read tool - mark_read_tool = await mcp_server_with_actions.get_tool("mark_email_read_tool") - assert mark_read_tool is not None + # Get the invoke_action tool + invoke_tool = await mcp_server_with_actions.get_tool("invoke_action") + assert invoke_tool is not None - # Execute the tool with explicit parameters - result = await mark_read_tool.fn( - email="SampleEmailPage:email456", + # Execute the tool with action_name and action_input + result = await invoke_tool.fn( + action_name="mark_email_read", + action_input={ + "email": "SampleEmailPage:email456", + }, ) # Verify the result result_data = json.loads(result) - assert result_data == {"success": True} + assert result_data["success"] is True # Verify the action was called correctly expected_action_input = { @@ -211,98 +220,122 @@ async def test_mark_email_read_tool_execution( "mark_email_read", expected_action_input ) - async def test_action_tool_descriptions_contain_parameters( + async def test_invoke_action_tool_description_contains_available_actions( self, mcp_server_with_actions ): - """Test that action tool descriptions contain parameter information.""" - # Get the send email tool - send_tool = await mcp_server_with_actions.get_tool("send_email_tool") - description = send_tool.description + """Test that invoke_action tool description contains available actions.""" + # Get the invoke_action tool + invoke_tool = await mcp_server_with_actions.get_tool("invoke_action") + description = invoke_tool.description # Check that the description contains expected information + assert "Execute any registered action" in description assert "send_email" in description - assert "Send a new email" in description + assert "reply_to_email" in description + assert "mark_email_read" in description - # Check that parameters are documented (note: first Page parameter is excluded from description) - assert "additional_recipients:" in description - assert "cc_list:" in description - assert "subject:" in description - assert "message:" in description + # Check that parameters are documented + assert "action_name" in description + assert "action_input" in description - # Check parameter types are shown (transformed to PageURI types) - assert "PageURI" in description - assert "List[PageURI]" in description - assert "str" in description + # Check usage examples + assert "action_name=" in description + assert "action_input=" in description - async def test_all_action_tools_have_unique_descriptions( - self, mcp_server_with_actions - ): - """Test that all action tools have unique, specific descriptions.""" - # Get all action tools - send_tool = await mcp_server_with_actions.get_tool("send_email_tool") - reply_tool = await mcp_server_with_actions.get_tool("reply_to_email_tool") - mark_read_tool = await mcp_server_with_actions.get_tool("mark_email_read_tool") - - # Verify all tools exist - assert send_tool is not None - assert reply_tool is not None - assert mark_read_tool is not None - - # Verify descriptions are unique - assert send_tool.description != reply_tool.description - assert send_tool.description != mark_read_tool.description - assert reply_tool.description != mark_read_tool.description - - # Verify each description contains the correct action name - assert "send_email" in send_tool.description - assert "reply_to_email" in reply_tool.description - assert "mark_email_read" in mark_read_tool.description - - async def test_action_tool_error_handling_realistic( + async def test_invoke_action_error_handling( self, mcp_server_with_actions, context_with_actions ): - """Test error handling in action tools with realistic error scenarios.""" + """Test error handling in invoke_action tool.""" # Mock the invoke_action method to raise an exception context_with_actions.invoke_action = AsyncMock( - side_effect=ValueError("Email sending failed: invalid recipient") + side_effect=ValueError("Action execution failed: invalid parameters") ) - # Get the send email tool - send_tool = await mcp_server_with_actions.get_tool("send_email_tool") - - # Execute the tool with explicit parameters - result = await send_tool.fn( - person="SamplePersonPage:invalid_person", - subject="Test Subject", - message="Test message", + # Get the invoke_action tool + invoke_tool = await mcp_server_with_actions.get_tool("invoke_action") + + # Execute the tool with parameters that will cause an error + result = await invoke_tool.fn( + action_name="send_email", + action_input={ + "person": "SamplePersonPage:invalid_person", + "subject": "Test Subject", + "message": "Test message", + }, ) # Verify the error result result_data = json.loads(result) assert result_data["success"] is False - assert "Email sending failed: invalid recipient" in result_data["error"] + assert "Action execution failed: invalid parameters" in result_data["error"] - async def test_no_generic_action_tool_in_realistic_setup( - self, mcp_server_with_actions + async def test_invoke_action_with_invalid_action_name( + self, mcp_server_with_actions, context_with_actions ): - """Test that no generic 'action_tool' exists in realistic setup.""" - tools = await mcp_server_with_actions.get_tools() + """Test invoke_action with invalid action name.""" + # Mock the invoke_action method to raise an exception for invalid action + context_with_actions.invoke_action = AsyncMock( + side_effect=ValueError("Action 'invalid_action' not found") + ) - # Should not have any generic action tool - assert "action_tool" not in tools + # Get the invoke_action tool + invoke_tool = await mcp_server_with_actions.get_tool("invoke_action") - # All action tools should be specifically named - action_tools = [tool for tool in tools if tool.endswith("_tool")] - expected_action_tools = { - "send_email_tool", - "reply_to_email_tool", - "mark_email_read_tool", - } + # Execute the tool with invalid action name + result = await invoke_tool.fn( + action_name="invalid_action", action_input={"some_param": "some_value"} + ) + + # Verify the error result + result_data = json.loads(result) + assert result_data["success"] is False + assert "Action 'invalid_action' not found" in result_data["error"] - actual_action_tools = set(action_tools) - assert expected_action_tools == actual_action_tools + async def test_invoke_action_with_complex_input( + self, mcp_server_with_actions, context_with_actions + ): + """Test invoke_action with complex action input containing lists and nested data.""" + # Mock invoke_action to capture the arguments + captured_args = {} + + async def mock_invoke_action(action_name, args): + captured_args[action_name] = args + return {"success": True} + + context_with_actions.invoke_action = mock_invoke_action + + # Get the invoke_action tool + invoke_tool = await mcp_server_with_actions.get_tool("invoke_action") + + # Execute the tool with complex action input + result = await invoke_tool.fn( + action_name="send_email", + action_input={ + "person": "SamplePersonPage:person1", + "additional_recipients": [ + "SamplePersonPage:person2", + "SamplePersonPage:person3", + ], + "cc_list": ["SamplePersonPage:cc1"], + "subject": "Test Email Subject", + "message": "This is a test email message", + }, + ) - # Verify no generic tool exists - for tool_name in action_tools: - assert tool_name != "action_tool" - assert "_tool" in tool_name + # Verify the result + result_data = json.loads(result) + assert result_data["success"] is True + + # Verify that the complex input was passed through correctly + assert "send_email" in captured_args + args = captured_args["send_email"] + + # All parameters should be passed through as-is + assert args["person"] == "SamplePersonPage:person1" + assert args["additional_recipients"] == [ + "SamplePersonPage:person2", + "SamplePersonPage:person3", + ] + assert args["cc_list"] == ["SamplePersonPage:cc1"] + assert args["subject"] == "Test Email Subject" + assert args["message"] == "This is a test email message" diff --git a/tests/services/test_gmail_actions.py b/tests/services/test_gmail_actions.py index f46f8b3..48f08c0 100644 --- a/tests/services/test_gmail_actions.py +++ b/tests/services/test_gmail_actions.py @@ -225,15 +225,10 @@ async def test_reply_to_email_thread_action_without_specific_email(self): # Mock get_page to return the latest email self.mock_context.get_page.return_value = latest_email - # Mock people service to find sender - sender_person = PersonPage( - uri=PageURI(root="test-root", type="person", id="sender_person", version=1), - first_name="Sender", - last_name="Two", - email="sender2@example.com", - source="emails", + # Mock _get_current_user_email to return current user email for Reply All + self.service._get_current_user_email = AsyncMock( + return_value="current-user@example.com" ) - self.mock_people_service.search_existing_records.return_value = [sender_person] # Mock send_message to succeed self.mock_api_client.send_message.return_value = {"id": "sent_msg_id"} @@ -242,8 +237,8 @@ async def test_reply_to_email_thread_action_without_specific_email(self): result = await self.service._reply_to_thread_internal( thread=thread, email=None, - recipients=None, # Should default to sender of latest email - cc_list=None, + recipients=None, # Should trigger Reply All behavior + cc_list=None, # Should trigger Reply All behavior message="Reply to the thread", ) @@ -253,15 +248,17 @@ async def test_reply_to_email_thread_action_without_specific_email(self): # Verify get_page was called for latest email self.mock_context.get_page.assert_called_once_with(email_uri2) - # Verify people service was called to find sender - self.mock_people_service.search_existing_records.assert_called_once_with( - "sender2@example.com" - ) + # Verify Reply All behavior - people service should NOT be called + self.mock_people_service.search_existing_records.assert_not_called() - # Verify send_message was called correctly + # Verify _get_current_user_email was called for Reply All + self.service._get_current_user_email.assert_called_once() + + # Verify send_message was called with Reply All behavior + # Should include sender + original recipients (excluding current user) self.mock_api_client.send_message.assert_called_once_with( - to=["sender2@example.com"], - cc=[], + to=["sender2@example.com", "recipient@example.com"], # sender + recipients + cc=[], # no CC in original email subject="Re: Test Subject", body="Reply to the thread", thread_id="thread123", @@ -479,6 +476,223 @@ async def test_send_email_action_failure(self): # Verify it returns False on failure assert result is False + @pytest.mark.asyncio + async def test_reply_to_email_thread_reply_all_behavior(self): + """Test default Reply All behavior when no recipients/CC specified.""" + # Create test data with multiple recipients and CC + thread_uri = PageURI( + root="test-root", type="email_thread", id="thread123", version=1 + ) + email_uri = PageURI(root="test-root", type="email", id="msg1", version=1) + + thread = EmailThreadPage( + uri=thread_uri, + thread_id="thread123", + subject="Test Thread", + emails=[], + permalink="https://mail.google.com/mail/u/0/#inbox/thread123", + ) + + email = EmailPage( + uri=email_uri, + message_id="msg1", + thread_id="thread123", + subject="Test Subject", + sender="sender@example.com", + recipients=[ + "current-user@example.com", + "other1@example.com", + "other2@example.com", + ], + cc_list=["cc1@example.com", "current-user@example.com", "cc2@example.com"], + body="Original email", + time=datetime.now(), + permalink="https://mail.google.com/mail/u/0/#inbox/thread123", + ) + + # Mock _get_current_user_email to return current user email + self.service._get_current_user_email = AsyncMock( + return_value="current-user@example.com" + ) + + # Mock send_message to succeed + self.mock_api_client.send_message.return_value = {"id": "sent_msg_id"} + + # Call the internal action method with no recipients/CC (Reply All) + result = await self.service._reply_to_thread_internal( + thread=thread, + email=email, + recipients=None, # Should trigger Reply All behavior + cc_list=None, # Should trigger Reply All behavior + message="Reply All message", + ) + + # Verify the result + assert result is True + + # Verify _get_current_user_email was called + self.service._get_current_user_email.assert_called_once() + + # Verify send_message was called with Reply All behavior + self.mock_api_client.send_message.assert_called_once() + call_args = self.mock_api_client.send_message.call_args + + # Should include sender + other recipients (excluding current user) + expected_to = ["sender@example.com", "other1@example.com", "other2@example.com"] + assert sorted(call_args[1]["to"]) == sorted(expected_to) + + # Should include original CC (excluding current user) + expected_cc = ["cc1@example.com", "cc2@example.com"] + assert sorted(call_args[1]["cc"]) == sorted(expected_cc) + + assert call_args[1]["subject"] == "Re: Test Subject" + assert call_args[1]["body"] == "Reply All message" + assert call_args[1]["thread_id"] == "thread123" + + # Verify people service was NOT called for Reply All + self.mock_people_service.search_existing_records.assert_not_called() + + @pytest.mark.asyncio + async def test_reply_to_email_thread_with_specific_recipients_and_cc(self): + """Test reply with specific recipients and CC list specified.""" + # Create test data + thread_uri = PageURI( + root="test-root", type="email_thread", id="thread123", version=1 + ) + email_uri = PageURI(root="test-root", type="email", id="msg1", version=1) + + thread = EmailThreadPage( + uri=thread_uri, + thread_id="thread123", + subject="Test Thread", + emails=[], + permalink="https://mail.google.com/mail/u/0/#inbox/thread123", + ) + + email = EmailPage( + uri=email_uri, + message_id="msg1", + thread_id="thread123", + subject="Test Subject", + sender="sender@example.com", + recipients=["original@example.com"], + body="Original email", + time=datetime.now(), + permalink="https://mail.google.com/mail/u/0/#inbox/thread123", + ) + + # Create mock PersonPages for custom recipients and CC + recipient1 = PersonPage( + uri=PageURI(root="test-root", type="person", id="recipient1", version=1), + first_name="Recipient", + last_name="One", + email="custom1@example.com", + source="emails", + ) + recipient2 = PersonPage( + uri=PageURI(root="test-root", type="person", id="recipient2", version=1), + first_name="Recipient", + last_name="Two", + email="custom2@example.com", + source="emails", + ) + cc_person = PersonPage( + uri=PageURI(root="test-root", type="person", id="cc_person", version=1), + first_name="CC", + last_name="Person", + email="cc@example.com", + source="emails", + ) + + # Mock send_message to succeed + self.mock_api_client.send_message.return_value = {"id": "sent_msg_id"} + + # Call the internal action method with specific recipients and CC + result = await self.service._reply_to_thread_internal( + thread=thread, + email=email, + recipients=[recipient1, recipient2], + cc_list=[cc_person], + message="Custom reply", + ) + + # Verify the result + assert result is True + + # Verify send_message was called with custom recipients and CC + self.mock_api_client.send_message.assert_called_once() + call_args = self.mock_api_client.send_message.call_args + assert call_args[1]["to"] == ["custom1@example.com", "custom2@example.com"] + assert call_args[1]["cc"] == ["cc@example.com"] + assert call_args[1]["subject"] == "Re: Test Subject" + assert call_args[1]["body"] == "Custom reply" + assert call_args[1]["thread_id"] == "thread123" + + # Verify people service was NOT called since recipients were provided + self.mock_people_service.search_existing_records.assert_not_called() + + @pytest.mark.asyncio + async def test_reply_to_email_thread_reply_all_current_user_is_sender(self): + """Test Reply All when current user is the sender.""" + # Create test data where current user is the sender + thread_uri = PageURI( + root="test-root", type="email_thread", id="thread123", version=1 + ) + email_uri = PageURI(root="test-root", type="email", id="msg1", version=1) + + thread = EmailThreadPage( + uri=thread_uri, + thread_id="thread123", + subject="Test Thread", + emails=[], + permalink="https://mail.google.com/mail/u/0/#inbox/thread123", + ) + + email = EmailPage( + uri=email_uri, + message_id="msg1", + thread_id="thread123", + subject="Test Subject", + sender="current-user@example.com", # Current user is sender + recipients=["recipient1@example.com", "recipient2@example.com"], + cc_list=["cc1@example.com"], + body="Original email", + time=datetime.now(), + permalink="https://mail.google.com/mail/u/0/#inbox/thread123", + ) + + # Mock _get_current_user_email to return current user email + self.service._get_current_user_email = AsyncMock( + return_value="current-user@example.com" + ) + + # Mock send_message to succeed + self.mock_api_client.send_message.return_value = {"id": "sent_msg_id"} + + # Call the internal action method with no recipients/CC (Reply All) + result = await self.service._reply_to_thread_internal( + thread=thread, + email=email, + recipients=None, # Should trigger Reply All behavior + cc_list=None, # Should trigger Reply All behavior + message="Reply All to own email", + ) + + # Verify the result + assert result is True + + # Verify send_message was called with Reply All behavior + self.mock_api_client.send_message.assert_called_once() + call_args = self.mock_api_client.send_message.call_args + + # Should include only other recipients (sender excluded since they're current user) + expected_to = ["recipient1@example.com", "recipient2@example.com"] + assert sorted(call_args[1]["to"]) == sorted(expected_to) + + # Should include original CC + expected_cc = ["cc1@example.com"] + assert call_args[1]["cc"] == expected_cc + @pytest.mark.asyncio async def test_action_registration(self): """Test that actions are properly registered with the context.""" diff --git a/tests/services/test_gmail_service.py b/tests/services/test_gmail_service.py index 48ec7c5..536ad88 100644 --- a/tests/services/test_gmail_service.py +++ b/tests/services/test_gmail_service.py @@ -878,6 +878,115 @@ async def test_thread_page_contains_email_summaries(self): assert email_summary.body == "Email body content" assert isinstance(email_summary.time, datetime) + @pytest.mark.asyncio + async def test_parse_email_headers_with_display_names(self): + """Test that email addresses are correctly extracted from headers with display names.""" + # Setup mock message with display names in headers + mock_message = { + "id": "msg123", + "threadId": "thread456", + "payload": { + "headers": [ + {"name": "Subject", "value": "Test Subject"}, + {"name": "From", "value": "Sam from Cursor "}, + { + "name": "To", + "value": "Tapan C , John Doe ", + }, + { + "name": "Cc", + "value": "Jane Smith , admin@example.org", + }, + {"name": "Date", "value": "Thu, 15 Jun 2023 10:30:00 +0000"}, + ], + "body": {"data": "VGVzdCBlbWFpbCBib2R5"}, # "Test email body" in base64 + }, + } + + self.mock_api_client.get_message.return_value = mock_message + + # Create service and fetch page + service = GmailService(self.mock_api_client) + + page_uri = PageURI(root="test-root", type="email", id="msg123", version=1) + result = await service.create_email_page(page_uri) + + # Verify email addresses were extracted correctly + assert result.sender == "hi@cursor.com" # Should extract just the email + assert result.recipients == ["tapanc@cs.washington.edu", "john@example.com"] + assert result.cc_list == ["jane@example.com", "admin@example.org"] + assert result.subject == "Test Subject" + + @pytest.mark.asyncio + async def test_parse_email_headers_without_display_names(self): + """Test that plain email addresses are handled correctly.""" + # Setup mock message with plain email addresses + mock_message = { + "id": "msg456", + "threadId": "thread789", + "payload": { + "headers": [ + {"name": "Subject", "value": "Plain Email Test"}, + {"name": "From", "value": "plain@example.com"}, + {"name": "To", "value": "recipient@example.com"}, + {"name": "Date", "value": "Thu, 15 Jun 2023 10:30:00 +0000"}, + ], + "body": {"data": "VGVzdCBlbWFpbCBib2R5"}, # "Test email body" in base64 + }, + } + + self.mock_api_client.get_message.return_value = mock_message + + # Create service and fetch page + service = GmailService(self.mock_api_client) + + page_uri = PageURI(root="test-root", type="email", id="msg456", version=1) + result = await service.create_email_page(page_uri) + + # Verify plain email addresses are preserved + assert result.sender == "plain@example.com" + assert result.recipients == ["recipient@example.com"] + assert result.cc_list == [] + + @pytest.mark.asyncio + async def test_parse_email_headers_edge_cases(self): + """Test edge cases in email header parsing.""" + # Setup mock message with edge cases + mock_message = { + "id": "msg789", + "threadId": "thread123", + "payload": { + "headers": [ + {"name": "Subject", "value": "Edge Case Test"}, + { + "name": "From", + "value": "", + }, # Only email in brackets + { + "name": "To", + "value": "Name Only, , email3@example.com", + }, # Mixed formats + {"name": "Cc", "value": ""}, # Empty CC + {"name": "Date", "value": "Thu, 15 Jun 2023 10:30:00 +0000"}, + ], + "body": {"data": "VGVzdCBlbWFpbCBib2R5"}, # "Test email body" in base64 + }, + } + + self.mock_api_client.get_message.return_value = mock_message + + # Create service and fetch page + service = GmailService(self.mock_api_client) + + page_uri = PageURI(root="test-root", type="email", id="msg789", version=1) + result = await service.create_email_page(page_uri) + + # Verify edge cases are handled correctly + assert result.sender == "only-email@example.com" + # Should skip "Name Only" since it has no "@", but include the others + assert result.recipients == ["email2@example.com", "email3@example.com"] + assert result.cc_list == [] # Empty CC should result in empty list + class TestGmailToolkit: """Test suite for GmailService toolkit methods (now integrated into GmailService).""" @@ -952,86 +1061,3 @@ async def test_search_emails_from_person_with_keywords(self): assert query == 'from:"test@example.com" urgent project' assert len(result) == 1 assert isinstance(result[0], EmailPage) - - @pytest.mark.asyncio - async def test_search_emails_to_person_basic(self): - mock_messages = [{"id": "msg1"}] - self.mock_api_client.search_messages.return_value = (mock_messages, None) - mock_pages = [AsyncMock(spec=EmailPage)] - self.mock_context.get_page.side_effect = mock_pages - self.mock_context.get_pages.return_value = mock_pages - with patch( - "pragweb.google_api.utils.resolve_person_identifier", - return_value="recipient@example.com", - ): - result = await self.toolkit.search_emails_to_person("recipient@example.com") - args, kwargs = self.mock_api_client.search_messages.call_args - query = args[0] - assert query == 'to:"recipient@example.com" OR cc:"recipient@example.com"' - assert len(result) == 1 - assert isinstance(result[0], EmailPage) - - @pytest.mark.asyncio - async def test_search_emails_to_person_with_keywords(self): - mock_messages = [{"id": "msg1"}] - self.mock_api_client.search_messages.return_value = (mock_messages, None) - mock_pages = [AsyncMock(spec=EmailPage)] - self.mock_context.get_page.side_effect = mock_pages - self.mock_context.get_pages.return_value = mock_pages - with patch( - "pragweb.google_api.utils.resolve_person_identifier", - return_value="recipient@example.com", - ): - result = await self.toolkit.search_emails_to_person( - "recipient@example.com", content="meeting notes" - ) - args, kwargs = self.mock_api_client.search_messages.call_args - query = args[0] - assert ( - query - == 'to:"recipient@example.com" OR cc:"recipient@example.com" meeting notes' - ) - assert len(result) == 1 - assert isinstance(result[0], EmailPage) - - @pytest.mark.asyncio - async def test_search_emails_by_content(self): - mock_messages = [{"id": "msg1"}] - self.mock_api_client.search_messages.return_value = (mock_messages, None) - mock_pages = [AsyncMock(spec=EmailPage)] - self.mock_context.get_page.side_effect = mock_pages - self.mock_context.get_pages.return_value = mock_pages - result = await self.toolkit.search_emails_by_content("important announcement") - args, kwargs = self.mock_api_client.search_messages.call_args - query = args[0] - assert query == "important announcement" - assert len(result) == 1 - assert isinstance(result[0], EmailPage) - - @pytest.mark.asyncio - async def test_get_recent_emails_basic(self): - mock_messages = [{"id": "msg1"}] - self.mock_api_client.search_messages.return_value = (mock_messages, None) - mock_pages = [AsyncMock(spec=EmailPage)] - self.mock_context.get_page.side_effect = mock_pages - self.mock_context.get_pages.return_value = mock_pages - result = await self.toolkit.get_recent_emails(days=7) - args, kwargs = self.mock_api_client.search_messages.call_args - query = args[0] - assert query == "newer_than:7d" - assert len(result) == 1 - assert isinstance(result[0], EmailPage) - - @pytest.mark.asyncio - async def test_get_recent_emails_with_keywords(self): - mock_messages = [{"id": "msg1"}] - self.mock_api_client.search_messages.return_value = (mock_messages, None) - mock_pages = [AsyncMock(spec=EmailPage)] - self.mock_context.get_page.side_effect = mock_pages - self.mock_context.get_pages.return_value = mock_pages - result = await self.toolkit.get_recent_emails(days=3) - args, kwargs = self.mock_api_client.search_messages.call_args - query = args[0] - assert query == "newer_than:3d" - assert len(result) == 1 - assert isinstance(result[0], EmailPage) From a5bd368cbe1f9dd862ce34495ca791304a38e7a9 Mon Sep 17 00:00:00 2001 From: Tapan Chugh Date: Sat, 12 Jul 2025 12:25:29 -0700 Subject: [PATCH 2/4] Minor update --- src/praga_core/integrations/mcp/descriptions.py | 7 ++----- src/praga_core/integrations/mcp/server.py | 14 ++++---------- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/src/praga_core/integrations/mcp/descriptions.py b/src/praga_core/integrations/mcp/descriptions.py index 04219fc..12149ef 100644 --- a/src/praga_core/integrations/mcp/descriptions.py +++ b/src/praga_core/integrations/mcp/descriptions.py @@ -11,13 +11,12 @@ def get_search_tool_description(type_names: List[str]) -> str: """Generate description for the search_pages tool.""" return f"""Search for pages using natural language instructions. -Returns JSON with search results and optionally resolved page content. +Returns JSON with search results and resolved page content. Available page types: {', '.join(type_names)} Parameters: - instruction: Natural language search instruction (required) -- resolve_references: Whether to resolve page content in results (default: true) Examples: - "Find emails from Alice about project X" @@ -26,9 +25,7 @@ def get_search_tool_description(type_names: List[str]) -> str: - "Search for person named John Smith" Response format: -- results: Array of search results with page references -- If resolve_references=true: Each result includes full page content -- If resolve_references=false: Each result includes only page URI and metadata +- results: Array of search results with page references and resolved page content """ diff --git a/src/praga_core/integrations/mcp/server.py b/src/praga_core/integrations/mcp/server.py index b02fa24..d8d0489 100644 --- a/src/praga_core/integrations/mcp/server.py +++ b/src/praga_core/integrations/mcp/server.py @@ -55,28 +55,22 @@ def setup_mcp_tools( type_names = list(server_context._handlers.keys()) @mcp.tool(description=get_search_tool_description(type_names)) - async def search_pages( - instruction: str, resolve_references: bool = True, ctx: Optional[Context] = None - ) -> str: + async def search_pages(instruction: str, ctx: Optional[Context] = None) -> str: """Search for pages using natural language instructions. Args: instruction: Natural language search instruction - resolve_references: Whether to resolve page content in results ctx: MCP context for logging Returns: - JSON string containing search results with page references and resolved content + JSON string containing search results with resolved page content """ try: if ctx: await ctx.info(f"Searching for: {instruction}") - await ctx.info(f"Resolve references: {resolve_references}") - # Perform the search - search_response = await server_context.search( - instruction, resolve_references=resolve_references - ) + # Perform the search (page resolution is now handled internally by the agent) + search_response = await server_context.search(instruction) if ctx: await ctx.info(f"Found {len(search_response.results)} results") From ef47153b0041a67571d6b3d3af5f37857a2c1803 Mon Sep 17 00:00:00 2001 From: Tapan Chugh Date: Sat, 12 Jul 2025 12:35:02 -0700 Subject: [PATCH 3/4] Fix tests and CI issues --- .github/workflows/main.yml | 5 +++- pyproject.toml | 2 +- tests/services/test_gmail_actions.py | 42 +++++++++++++--------------- 3 files changed, 25 insertions(+), 24 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index c28cb8d..326477e 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -24,5 +24,8 @@ jobs: source .venv/bin/activate uv pip install -e . - - name: Run tests + - name: Run unit tests run: uv run pytest + + - name: Run integration tests + run: uv run pytest tests/integration/ diff --git a/pyproject.toml b/pyproject.toml index ceff7e8..45e909e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -45,7 +45,7 @@ build-backend = "hatchling.build" [tool.pytest.ini_options] testpaths = ["tests"] python_files = ["test_*.py"] -addopts = "--asyncio-mode=auto" +addopts = "--asyncio-mode=auto --ignore=tests/integration/" [tool.black] line-length = 88 diff --git a/tests/services/test_gmail_actions.py b/tests/services/test_gmail_actions.py index 48f08c0..aa55ef1 100644 --- a/tests/services/test_gmail_actions.py +++ b/tests/services/test_gmail_actions.py @@ -343,17 +343,17 @@ async def test_reply_to_email_thread_action_failure(self): # Verify the registered action exists assert "reply_to_email_thread" in self.registered_actions - # Call the action - result = await self.service._reply_to_thread_internal( - thread=thread, - email=email, - recipients=[], - cc_list=None, - message="Reply message", - ) - - # Verify it returns False on failure - assert result is False + # Call the action and verify it raises RuntimeError + with pytest.raises( + RuntimeError, match="Failed to reply to thread: Send failed" + ): + await self.service._reply_to_thread_internal( + thread=thread, + email=email, + recipients=[], + cc_list=None, + message="Reply message", + ) @pytest.mark.asyncio async def test_send_email_action_basic(self): @@ -464,17 +464,15 @@ async def test_send_email_action_failure(self): # Mock send_message to fail self.mock_api_client.send_message.side_effect = Exception("Send failed") - # Call the internal action method directly - result = await self.service._send_email_internal( - person=recipient, - additional_recipients=None, - cc_list=None, - subject="Test Email", - message="Test body", - ) - - # Verify it returns False on failure - assert result is False + # Call the internal action method directly and verify it raises RuntimeError + with pytest.raises(RuntimeError, match="Failed to send email: Send failed"): + await self.service._send_email_internal( + person=recipient, + additional_recipients=None, + cc_list=None, + subject="Test Email", + message="Test body", + ) @pytest.mark.asyncio async def test_reply_to_email_thread_reply_all_behavior(self): From 2102611762e24887c86bcfe1233433f611b4cfd4 Mon Sep 17 00:00:00 2001 From: Tapan Chugh Date: Sat, 12 Jul 2025 12:51:56 -0700 Subject: [PATCH 4/4] Fix tests and CI issues --- .../test_mcp_gmail_integration.py | 0 .../test_mcp_integration.py | 117 ++++++++---------- .../test_mcp_real_actions.py | 0 3 files changed, 53 insertions(+), 64 deletions(-) rename tests/{integration => mcp}/test_mcp_gmail_integration.py (100%) rename tests/{integration => mcp}/test_mcp_integration.py (75%) rename tests/{integration => mcp}/test_mcp_real_actions.py (100%) diff --git a/tests/integration/test_mcp_gmail_integration.py b/tests/mcp/test_mcp_gmail_integration.py similarity index 100% rename from tests/integration/test_mcp_gmail_integration.py rename to tests/mcp/test_mcp_gmail_integration.py diff --git a/tests/integration/test_mcp_integration.py b/tests/mcp/test_mcp_integration.py similarity index 75% rename from tests/integration/test_mcp_integration.py rename to tests/mcp/test_mcp_integration.py index bf22ac7..7f7e9f3 100644 --- a/tests/integration/test_mcp_integration.py +++ b/tests/mcp/test_mcp_integration.py @@ -95,27 +95,21 @@ async def test_tool_registration(self, mcp_server): # Check that core tools are registered assert "search_pages" in tool_names assert "get_pages" in tool_names - - # Check that action tools are registered with correct names - assert "test_action_basic_tool" in tool_names - assert "test_action_with_person_tool" in tool_names - assert "test_action_failure_tool" in tool_names + assert "invoke_action" in tool_names # Verify we have the expected number of tools - assert len(tool_names) == 5 # 2 core + 3 action tools + assert len(tool_names) == 3 # search_pages, get_pages, invoke_action - async def test_individual_action_tools_registered(self, mcp_server): - """Test that each action gets its own tool.""" + async def test_invoke_action_tool_registered(self, mcp_server): + """Test that the unified invoke_action tool is registered.""" tools = await mcp_server.get_tools() - action_tools = [tool for tool in tools if tool.endswith("_tool")] - # Each action should have its own tool - assert "test_action_basic_tool" in action_tools - assert "test_action_with_person_tool" in action_tools - assert "test_action_failure_tool" in action_tools + # Should have invoke_action tool + assert "invoke_action" in tools - # No generic 'action_tool' should exist - assert "action_tool" not in tools + # Should not have individual action tools + action_tools = [tool for tool in tools if tool.endswith("_tool")] + assert len(action_tools) == 0 async def test_search_pages_tool(self, mcp_server, context): """Test search_pages tool functionality.""" @@ -130,15 +124,11 @@ async def test_search_pages_tool(self, mcp_server, context): context.search.return_value = mock_response # Test calling the tool - result = await search_tool.fn( - instruction="find test pages", resolve_references=True - ) + result = await search_tool.fn(instruction="find test pages") # Verify the result assert result == '{"results": []}' - context.search.assert_called_once_with( - "find test pages", resolve_references=True - ) + context.search.assert_called_once_with("find test pages") async def test_get_pages_tool(self, mcp_server, context): """Test get_pages tool functionality.""" @@ -168,15 +158,18 @@ async def test_get_pages_tool(self, mcp_server, context): async def test_action_tool_success(self, mcp_server, context): """Test successful action tool execution.""" - # Get the action tool - action_tool = await mcp_server.get_tool("test_action_basic_tool") + # Get the invoke_action tool + action_tool = await mcp_server.get_tool("invoke_action") assert action_tool is not None # Mock the context invoke_action method context.invoke_action = AsyncMock(return_value={"success": True}) # Test calling the tool with explicit parameters - result = await action_tool.fn(page="MCPTestPage:123", message="test") + result = await action_tool.fn( + action_name="test_action_basic", + action_input={"page": "MCPTestPage:123", "message": "test"}, + ) # Verify the result result_data = json.loads(result) @@ -189,15 +182,17 @@ async def test_action_tool_success(self, mcp_server, context): async def test_action_tool_failure(self, mcp_server, context): """Test action tool execution with failure.""" - # Get the action tool - action_tool = await mcp_server.get_tool("test_action_failure_tool") + # Get the invoke_action tool + action_tool = await mcp_server.get_tool("invoke_action") assert action_tool is not None # Mock the context invoke_action method to raise exception context.invoke_action = AsyncMock(side_effect=ValueError("Test error")) # Test calling the tool with explicit parameters - result = await action_tool.fn(page="MCPTestPage:123") + result = await action_tool.fn( + action_name="test_action_failure", action_input={"page": "MCPTestPage:123"} + ) # Verify the result result_data = json.loads(result) @@ -206,8 +201,8 @@ async def test_action_tool_failure(self, mcp_server, context): async def test_action_tool_with_multiple_pages(self, mcp_server, context): """Test action tool with multiple page parameters.""" - # Get the action tool - action_tool = await mcp_server.get_tool("test_action_with_person_tool") + # Get the invoke_action tool + action_tool = await mcp_server.get_tool("invoke_action") assert action_tool is not None # Mock the context invoke_action method @@ -215,9 +210,12 @@ async def test_action_tool_with_multiple_pages(self, mcp_server, context): # Test calling the tool with explicit parameters result = await action_tool.fn( - page="MCPTestPage:123", - person="MCPTestPersonPage:456", - note="test note", + action_name="test_action_with_person", + action_input={ + "page": "MCPTestPage:123", + "person": "MCPTestPersonPage:456", + "note": "test note", + }, ) # Verify the result @@ -255,9 +253,7 @@ async def test_search_tool_with_context_logging(self, mcp_server, context): mock_ctx.error = AsyncMock() # Test calling the tool with context - result = await search_tool.fn( - instruction="find test pages", resolve_references=True, ctx=mock_ctx - ) + result = await search_tool.fn(instruction="find test pages", ctx=mock_ctx) # Verify the result assert '"uri": "test:123"' in result @@ -270,8 +266,8 @@ async def test_search_tool_with_context_logging(self, mcp_server, context): async def test_action_tool_with_context_logging(self, mcp_server, context): """Test action tool with MCP context logging.""" - # Get the action tool - action_tool = await mcp_server.get_tool("test_action_basic_tool") + # Get the invoke_action tool + action_tool = await mcp_server.get_tool("invoke_action") assert action_tool is not None # Mock the context invoke_action method @@ -284,7 +280,9 @@ async def test_action_tool_with_context_logging(self, mcp_server, context): # Test calling the tool with explicit parameters and context result = await action_tool.fn( - page="MCPTestPage:123", message="test", ctx=mock_ctx + action_name="test_action_basic", + action_input={"page": "MCPTestPage:123", "message": "test"}, + ctx=mock_ctx, ) # Verify the result @@ -326,33 +324,24 @@ async def test_get_pages_tool_with_errors(self, mcp_server, context): assert result_data["errors"][0]["uri"] == "MCPTestPage:456" assert "Page not found" in result_data["errors"][0]["error"] - async def test_tool_descriptions_are_specific(self, mcp_server): - """Test that each tool has a specific description.""" - # Get tool descriptions - basic_tool = await mcp_server.get_tool("test_action_basic_tool") - person_tool = await mcp_server.get_tool("test_action_with_person_tool") - failure_tool = await mcp_server.get_tool("test_action_failure_tool") - - # Check that descriptions are specific to each action - assert basic_tool.description != person_tool.description - assert basic_tool.description != failure_tool.description - assert person_tool.description != failure_tool.description - - # Check that action names are in descriptions - assert "test_action_basic" in basic_tool.description - assert "test_action_with_person" in person_tool.description - assert "test_action_failure" in failure_tool.description - - async def test_no_generic_action_tool_exists(self, mcp_server): - """Test that no generic 'action_tool' exists.""" + async def test_invoke_action_tool_description(self, mcp_server): + """Test that invoke_action tool has a proper description.""" + # Get the invoke_action tool + invoke_tool = await mcp_server.get_tool("invoke_action") + assert invoke_tool is not None + + # Check that the description mentions action execution + description = invoke_tool.description + assert "action" in description.lower() + assert "invoke" in description.lower() or "execute" in description.lower() + + async def test_unified_action_architecture(self, mcp_server): + """Test that only the unified invoke_action tool exists for actions.""" tools = await mcp_server.get_tools() - # Should not have any generic action tool - assert "action_tool" not in tools + # Should have invoke_action tool + assert "invoke_action" in tools - # All action tools should be specifically named + # Should not have any individual action tools action_tools = [tool for tool in tools if tool.endswith("_tool")] - for tool_name in action_tools: - assert tool_name != "action_tool" - assert "_tool" in tool_name - assert tool_name.startswith("test_action_") + assert len(action_tools) == 0 diff --git a/tests/integration/test_mcp_real_actions.py b/tests/mcp/test_mcp_real_actions.py similarity index 100% rename from tests/integration/test_mcp_real_actions.py rename to tests/mcp/test_mcp_real_actions.py