diff --git a/frappe_assistant_core/api/fac_endpoint.py b/frappe_assistant_core/api/fac_endpoint.py index 613cef2..f6b77e5 100644 --- a/frappe_assistant_core/api/fac_endpoint.py +++ b/frappe_assistant_core/api/fac_endpoint.py @@ -51,35 +51,112 @@ def _check_assistant_enabled(user: str) -> bool: return False -def _import_tools(): - """Import and register all enabled tools from plugin manager.""" +def _build_tool_registry(): + """ + Build a per-request tool registry for the current user. + + Returns a fresh ``OrderedDict`` (name -> tool_dict) built on the call stack + rather than mutating the module-level ``mcp`` instance. This keeps + concurrent MCP requests isolated from each other: one in-flight request can + no longer clear or overwrite the tool set another request is validating or + executing against (issue #197). The set is also genuinely per-user, since + ``get_available_tools`` filters by the requesting user's permissions. + + Each tool dict carries MCP annotation hints derived from its FAC tool + category, so MCP clients (e.g. Claude Desktop) can group tools into + Read-only vs Write/delete instead of an undifferentiated "Other tools" + bucket. The category is the same one shown/overridable on the FAC admin + page (FAC Tool Configuration.tool_category) — single source of truth. + + Returns: + OrderedDict mapping tool name to its MCP tool dict. + """ + from collections import OrderedDict + + registry_dict = OrderedDict() try: from frappe_assistant_core.core.tool_registry import get_tool_registry - from frappe_assistant_core.mcp.tool_adapter import register_base_tool - - # Clear existing tools to avoid duplicates on subsequent requests - # This is necessary because mcp is a module-level global instance - mcp._tool_registry.clear() + from frappe_assistant_core.mcp.tool_adapter import build_tool_dict + from frappe_assistant_core.utils.tool_category_detector import category_to_annotations # Get available tools (respects enabled/disabled state and permissions) registry = get_tool_registry() available_tools = registry.get_available_tools(user=frappe.session.user) - # Register each enabled tool with MCP server - tool_count = 0 + # Resolve each tool's category once (honors admin overrides stored on + # FAC Tool Configuration; falls back to auto-detection). + categories = _resolve_tool_categories( + [t.get("name") for t in available_tools if t.get("name")], registry + ) + for tool_metadata in available_tools: tool_name = tool_metadata.get("name") if tool_name: tool_instance = registry.get_tool(tool_name) if tool_instance: - register_base_tool(mcp, tool_instance) - tool_count += 1 + tool_dict = build_tool_dict(tool_instance) + annotations = category_to_annotations(categories.get(tool_name, "read_write")) + if annotations: + # Merge with any annotations the tool already declared. + tool_dict["annotations"] = {**(tool_dict.get("annotations") or {}), **annotations} + registry_dict[tool_name] = tool_dict - frappe.logger().info(f"Registered {tool_count} enabled tools for user {frappe.session.user}") + frappe.logger().info(f"Built {len(registry_dict)} enabled tools for user {frappe.session.user}") except Exception as e: frappe.log_error(title="Tool Import Error", message=f"Error importing tools: {str(e)}") + return registry_dict + + +def _resolve_tool_categories(tool_names: list, registry) -> dict: + """ + Resolve the FAC tool category for each tool name. + + Resolution order per tool: + 1. Stored ``FAC Tool Configuration.tool_category`` (honors admin override). + 2. Auto-detected category via ``detect_tool_category`` (no config row yet). + 3. ``"read_write"`` fallback (maps to no annotation hints — safe default). + + Stored categories are batch-fetched in one query to avoid a DB read per tool. + + Args: + tool_names: Tool names to resolve. + registry: The tool registry (used to fetch instances for auto-detection). + + Returns: + Dict mapping tool name -> category string. + """ + from frappe_assistant_core.utils.tool_category_detector import detect_tool_category + + categories = {} + + # 1. Batch-fetch stored categories. + try: + rows = frappe.get_all( + "FAC Tool Configuration", + filters={"tool_name": ["in", tool_names]} if tool_names else {}, + fields=["tool_name", "tool_category"], + ignore_permissions=True, + ) + for row in rows: + if row.get("tool_category"): + categories[row["tool_name"]] = row["tool_category"] + except Exception as e: + frappe.logger().warning(f"Could not batch-fetch tool categories: {e}") + + # 2 & 3. Fill gaps via auto-detection, defaulting to read_write. + for tool_name in tool_names: + if tool_name in categories: + continue + try: + tool_instance = registry.get_tool(tool_name) + categories[tool_name] = detect_tool_category(tool_instance) if tool_instance else "read_write" + except Exception: + categories[tool_name] = "read_write" + + return categories + def _authenticate_mcp_request(): """ @@ -93,9 +170,10 @@ def _authenticate_mcp_request(): str: Authenticated username None: Authentication failed (returns 401 response directly) """ - from frappe.oauth import get_server_url from werkzeug.wrappers import Response + from frappe_assistant_core.api.oauth_discovery import get_public_base_url + auth_header = frappe.request.headers.get("Authorization", "") # Try OAuth Bearer token authentication first @@ -130,7 +208,7 @@ def _authenticate_mcp_request(): except frappe.DoesNotExistError: frappe.logger().error("OAuth Bearer Token not found") # Token not found - return 401 - frappe_url = get_server_url() + frappe_url = get_public_base_url() metadata_url = f"{frappe_url}/.well-known/oauth-protected-resource" response = Response() @@ -151,7 +229,7 @@ def _authenticate_mcp_request(): frappe.log_error(title="OAuth Token Validation Error", message=f"{type(e).__name__}: {str(e)}") # Return 401 for invalid/expired tokens - frappe_url = get_server_url() + frappe_url = get_public_base_url() metadata_url = f"{frappe_url}/.well-known/oauth-protected-resource" response = Response() @@ -205,7 +283,7 @@ def _authenticate_mcp_request(): except frappe.AuthenticationError as e: # Return 401 for invalid API credentials - frappe_url = get_server_url() + frappe_url = get_public_base_url() metadata_url = f"{frappe_url}/.well-known/oauth-protected-resource" response = Response() @@ -222,7 +300,7 @@ def _authenticate_mcp_request(): frappe.log_error(title="API Key Authentication Error", message=f"{type(e).__name__}: {str(e)}") # Return 401 for other errors - frappe_url = get_server_url() + frappe_url = get_public_base_url() metadata_url = f"{frappe_url}/.well-known/oauth-protected-resource" response = Response() @@ -236,7 +314,7 @@ def _authenticate_mcp_request(): # No valid authentication method found frappe.logger().warning("No valid authentication method found in request") - frappe_url = get_server_url() + frappe_url = get_public_base_url() metadata_url = f"{frappe_url}/.well-known/oauth-protected-resource" response = Response() @@ -264,13 +342,14 @@ def handle_mcp(): 1. OAuth 2.0 Bearer tokens: "Authorization: Bearer " (for web clients) 2. API Key/Secret: "Authorization: token :" (for STDIO clients) """ - from frappe.oauth import get_server_url from werkzeug.wrappers import Response + from frappe_assistant_core.api.oauth_discovery import get_public_base_url + # Handle HEAD request for connectivity check (Claude Web uses this) if frappe.request.method == "HEAD": # Return 401 with WWW-Authenticate header to indicate auth is required - frappe_url = get_server_url() + frappe_url = get_public_base_url() metadata_url = f"{frappe_url}/.well-known/oauth-protected-resource" response = Response() @@ -296,8 +375,6 @@ def handle_mcp(): _("Assistant access is disabled for user {0}").format(authenticated_user), frappe.PermissionError ) - # Import tools (they auto-register via decorators) - _import_tools() - - # Return None to let the decorator continue with MCP handling - return None + # Build a per-request tool registry (isolated from concurrent requests) and + # hand it back to the MCP server wrapper, which passes it into handle(). + return _build_tool_registry() diff --git a/frappe_assistant_core/api/oauth_discovery.py b/frappe_assistant_core/api/oauth_discovery.py index 32553c9..9e83c28 100644 --- a/frappe_assistant_core/api/oauth_discovery.py +++ b/frappe_assistant_core/api/oauth_discovery.py @@ -27,6 +27,47 @@ from frappe.oauth import get_server_url +def _get_public_base_url() -> str: + """ + Resolve the canonical public base URL for OAuth/MCP discovery metadata. + + Resolution order: + 1. site_config host_name — explicit operator override; used verbatim + if it includes a scheme (https://example.com). + 2. frappe.oauth.get_server_url() — existing behavior (Social Login Key + "frappe" base_url, then frappe.request.url). + 3. If force_https is set in site_config and the result still starts + with http://, upgrade to https://. Gated so localhost dev still works. + + Trailing slash is stripped so callers can append /api/method/... safely. + """ + host_name = frappe.conf.get("host_name") + if host_name and "://" in host_name: + base = host_name + else: + base = get_server_url() + + if frappe.conf.get("force_https") and base.startswith("http://"): + base = "https://" + base[len("http://") :] + + return base.rstrip("/") + + +def get_public_base_url() -> str: + """ + Public accessor for the canonical public base URL. + + Other modules (e.g. the MCP endpoint's WWW-Authenticate header) must build + OAuth metadata URLs from the same host_name-aware base as the discovery + endpoints, so that a configured host_name including a non-standard port is + honored verbatim. Frappe's get_server_url() reconstructs host/port from the + request or the Social Login Key base_url and drops the configured port + (issue #196), which breaks the OAuth handshake behind port-restricted + networks. + """ + return _get_public_base_url() + + # nosemgrep: frappe-semgrep-rules.rules.security.guest-whitelisted-method — OpenID Connect Discovery 1.0 mandates unauthenticated access to this endpoint @frappe.whitelist(allow_guest=True, methods=["GET"]) def openid_configuration(): @@ -52,7 +93,16 @@ def openid_configuration(): metadata = frappe.local.response # Add MCP-required fields that are missing - frappe_url = get_server_url() + frappe_url = _get_public_base_url() + # Override Frappe-inherited URLs with canonical public base URL + metadata["issuer"] = frappe_url + metadata["authorization_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.authorize" + metadata["token_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.get_token" + metadata["revocation_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.revoke_token" + metadata["introspection_endpoint"] = ( + f"{frappe_url}/api/method/frappe.integrations.oauth2.introspect_token" + ) + metadata["userinfo_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.openid_profile" # Add jwks_uri (required by MCP Inspector) metadata["jwks_uri"] = f"{frappe_url}/api/method/frappe_assistant_core.api.oauth_discovery.jwks" @@ -107,7 +157,7 @@ def mcp_discovery(): """ from frappe_assistant_core import hooks - frappe_url = get_server_url() + frappe_url = _get_public_base_url() # Get MCP configuration from settings mcp_protocol_version = "2025-06-18" @@ -151,7 +201,7 @@ def _get_frappe_authorization_server_metadata(): return _get_authorization_server_metadata() except ImportError: # Fallback for Frappe V15 - build metadata manually - frappe_url = get_server_url() + frappe_url = _get_public_base_url() # Base metadata following RFC 8414 metadata = { @@ -201,8 +251,18 @@ def authorization_server_metadata(): # Get base metadata from Frappe (V16 built-in or V15 fallback) metadata = _get_frappe_authorization_server_metadata() + # Normalize all URLs using canonical public base URL (fixes http -> https issue #156) + frappe_url = _get_public_base_url() + metadata["issuer"] = frappe_url + metadata["authorization_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.authorize" + metadata["token_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.get_token" + metadata["revocation_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.revoke_token" + metadata["introspection_endpoint"] = ( + f"{frappe_url}/api/method/frappe.integrations.oauth2.introspect_token" + ) + metadata["userinfo_endpoint"] = f"{frappe_url}/api/method/frappe.integrations.oauth2.openid_profile" + # Add/override custom service documentation - frappe_url = get_server_url() metadata["service_documentation"] = "https://github.com/buildswithpaul/Frappe_Assistant_Core" # Add client_secret_post as an additional auth method (Frappe V16 only has client_secret_basic) @@ -261,7 +321,7 @@ def protected_resource_metadata(): raise NotFound("Protected resource metadata is not enabled") - frappe_url = get_server_url() + frappe_url = _get_public_base_url() # Build list of authorization servers authorization_servers = [frappe_url] diff --git a/frappe_assistant_core/change_log/v2/v2_5_0.md b/frappe_assistant_core/change_log/v2/v2_5_0.md new file mode 100644 index 0000000..4ef53ce --- /dev/null +++ b/frappe_assistant_core/change_log/v2/v2_5_0.md @@ -0,0 +1,21 @@ +## Version 2.5.0 + +### New Features + +- **MCP tool annotations from FAC tool category (#202)** — tools advertised over MCP now carry `readOnlyHint` / `destructiveHint` annotations derived from each tool's category in FAC Tool Configuration. Clients like Claude Desktop use these to group tools meaningfully instead of bucketing every FAC tool under "Other tools". + +### Security + +- **`list_documents` permission leak fixed (#189)** — `search_documents` now goes through permission-aware `get_list(ignore_permissions=False)` instead of `get_all`, so results respect the requesting user's document-level permissions. + +### Fixes + +- **`report_requirements` filter discovery is robust and diagnosable (#203)** — Script Report filters defined only in the `.js` file are now discovered reliably: the `Report.filters` child table is consulted first, the JS path is resolved via Frappe's `get_module_path`, and the parser tolerates JSON-style quoted keys and template-literal labels. Genuinely unparseable cases (e.g. filters built programmatically) now return a `discovery_diagnostics` payload instead of a silent empty result. +- **Concurrent MCP tool calls isolated (#197)** — each request builds its own tool registry, preventing cross-request state bleed under concurrency. +- **OAuth/MCP metadata URLs honor configured port (#196)** — the `WWW-Authenticate` metadata URL is built from `host_name` including its port, so discovery works behind non-standard ports and proxies. +- **`get_doctype_info` includes child table metadata (#192)** — child table fields are now surfaced in the doctype description. +- **beautifulsoup4 pin relaxed for Frappe v16 (#198)** — constraint widened to `>=4.12,<5` so it spans Frappe v15 (`~=4.12.2`) and v16 (`~=4.13.5`) without forcing a downgrade. + +### Improvements + +- **OAuth URL resolution centralized** — discovery URLs are normalized through a single `_get_public_base_url()` helper that honors `host_name` configuration. diff --git a/frappe_assistant_core/mcp/server.py b/frappe_assistant_core/mcp/server.py index 91a8ef2..b426b86 100644 --- a/frappe_assistant_core/mcp/server.py +++ b/frappe_assistant_core/mcp/server.py @@ -116,23 +116,30 @@ def decorator(fn): self._entry_fn = fn def wrapper() -> Response: - # Run user's function to import tools and perform auth checks + # Run user's function to perform auth checks and build the + # per-request tool registry. The registry is returned to keep it + # off any shared/global state, so concurrent requests stay + # isolated (see issue #197). result = fn() - # If fn() returned a response (e.g., 401 auth failure), use that - if result is not None: + # If fn() returned a Response (e.g., 401 auth failure), use that. + if isinstance(result, Response): return result + # Otherwise fn() returns the per-request tool registry (a dict), + # or None to fall back to the shared registry. + tool_registry = result if isinstance(result, dict) else None + # Handle MCP request request = frappe.request response = Response() - return self.handle(request, response) + return self.handle(request, response, tool_registry=tool_registry) return whitelister(wrapper) return decorator - def handle(self, request: Request, response: Response) -> Response: + def handle(self, request: Request, response: Response, tool_registry: Optional[Dict] = None) -> Response: """ Handle MCP request - main entry point. @@ -141,12 +148,24 @@ def handle(self, request: Request, response: Response) -> Response: Args: request: Werkzeug Request object response: Werkzeug Response object + tool_registry: Per-request tool registry (name -> tool_dict). When + provided, all tool routing for this request reads from it instead + of the shared ``self._tool_registry``. This is what keeps + concurrent requests isolated: each request builds its own + registry on the call stack rather than mutating a process-global + one. Falls back to ``self._tool_registry`` when not supplied + (e.g. tools registered directly via ``add_tool``). Returns: Populated Response object with MCP response """ import frappe + # Per-request registry isolates concurrent requests. Never mutate the + # shared singleton during request handling. + if tool_registry is None: + tool_registry = self._tool_registry + # Only POST allowed if request.method != "POST": response.status_code = 405 @@ -192,12 +211,12 @@ def handle(self, request: Request, response: Response) -> Response: if method == "initialize": result = self._handle_initialize(params) elif method == "tools/list": - result = self._handle_tools_list(params) + result = self._handle_tools_list(params, tool_registry) elif method == "tools/call": frappe.logger().info( f"MCP tools/call: tool={params.get('name')}, args={json.dumps(params.get('arguments', {}), default=str)[:200]}" ) - result = self._handle_tools_call(params) + result = self._handle_tools_call(params, tool_registry) elif method == "resources/list": result = self._handle_resources_list(params, request_id) elif method == "resources/read": @@ -294,10 +313,13 @@ def _handle_initialize(self, params: Dict) -> Dict: "serverInfo": {"name": self.name, "version": "2.0.0"}, } - def _handle_tools_list(self, params: Dict) -> Dict: + def _handle_tools_list(self, params: Dict, tool_registry: Optional[Dict] = None) -> Dict: """Handle tools/list request with optional token optimization.""" import frappe + if tool_registry is None: + tool_registry = self._tool_registry + tools_list = [] # Check skill_mode for token optimization @@ -311,7 +333,7 @@ def _handle_tools_list(self, params: Dict) -> Dict: except Exception: pass - for tool in self._tool_registry.values(): + for tool in tool_registry.values(): description = tool["description"] # In replace mode, minimize descriptions for tools with linked skills @@ -333,7 +355,7 @@ def _handle_tools_list(self, params: Dict) -> Dict: return {"tools": tools_list} - def _handle_tools_call(self, params: Dict) -> Dict: + def _handle_tools_call(self, params: Dict, tool_registry: Optional[Dict] = None) -> Dict: """ Handle tools/call request. @@ -342,21 +364,24 @@ def _handle_tools_call(self, params: Dict) -> Dict: """ import frappe + if tool_registry is None: + tool_registry = self._tool_registry + tool_name = params.get("name") arguments = params.get("arguments", {}) frappe.logger().debug(f"MCP _handle_tools_call: tool={tool_name}, args={arguments}") # Check tool exists - if tool_name not in self._tool_registry: - error_msg = f"Tool '{tool_name}' not found. Available tools: {list(self._tool_registry.keys())}" + if tool_name not in tool_registry: + error_msg = f"Tool '{tool_name}' not found. Available tools: {list(tool_registry.keys())}" frappe.logger().error(f"MCP Tool Not Found: {error_msg}") return { "content": [{"type": "text", "text": error_msg}], "isError": True, } - tool = self._tool_registry[tool_name] + tool = tool_registry[tool_name] fn = tool["fn"] try: diff --git a/frappe_assistant_core/mcp/tool_adapter.py b/frappe_assistant_core/mcp/tool_adapter.py index 401ac55..faada14 100644 --- a/frappe_assistant_core/mcp/tool_adapter.py +++ b/frappe_assistant_core/mcp/tool_adapter.py @@ -24,6 +24,35 @@ from typing import Any, Dict +def build_tool_dict(tool_instance) -> Dict[str, Any]: + """ + Build the MCP tool dict for a BaseTool instance. + + Returns the same dict shape that ``MCPServer.add_tool`` stores, but without + touching any server/global state. This lets request handlers register tools + into a per-request registry, keeping concurrent requests isolated (issue + #197). + + Args: + tool_instance: Instance of BaseTool or compatible class + + Returns: + Dict with keys: name, description, inputSchema, annotations, fn + """ + + def tool_wrapper(**arguments): + """Wrapper that calls BaseTool.execute()""" + return tool_instance._safe_execute(arguments) + + return { + "name": tool_instance.name, + "description": tool_instance.description, + "inputSchema": tool_instance.inputSchema, + "annotations": getattr(tool_instance, "annotations", None), + "fn": tool_wrapper, + } + + def register_base_tool(mcp_server, tool_instance): """ Register a BaseTool instance with the MCP server. @@ -45,21 +74,8 @@ def register_base_tool(mcp_server, tool_instance): ``` """ - # Create wrapper function that calls tool's execute method - def tool_wrapper(**arguments): - """Wrapper that calls BaseTool.execute()""" - return tool_instance._safe_execute(arguments) - # Register with MCP server - mcp_server.add_tool( - { - "name": tool_instance.name, - "description": tool_instance.description, - "inputSchema": tool_instance.inputSchema, - "annotations": getattr(tool_instance, "annotations", None), - "fn": tool_wrapper, - } - ) + mcp_server.add_tool(build_tool_dict(tool_instance)) def register_all_base_tools(mcp_server, tool_instances): diff --git a/frappe_assistant_core/plugins/core/tools/document_tools.py b/frappe_assistant_core/plugins/core/tools/document_tools.py deleted file mode 100644 index 764bdb9..0000000 --- a/frappe_assistant_core/plugins/core/tools/document_tools.py +++ /dev/null @@ -1,343 +0,0 @@ -# Frappe Assistant Core - AI Assistant integration for Frappe Framework -# Copyright (C) 2025 Paul Clinton -# -# This program is free software: you can redistribute it and/or modify -# it under the terms of the GNU Affero General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU Affero General Public License for more details. -# -# You should have received a copy of the GNU Affero General Public License -# along with this program. If not, see . - -import json -from typing import Any, Dict, List - -import frappe -from frappe import _ - - -class DocumentTools: - """assistant tools for Frappe document operations""" - - @staticmethod - def get_tools() -> List[Dict]: - """Return list of document-related assistant tools""" - return [ - { - "name": "document_create", - "description": "Create a new Frappe document (e.g., Customer, Sales Invoice, Item, etc.). Use this when users want to add new records to the system. Always check required fields for the doctype first.", - "inputSchema": { - "type": "object", - "properties": { - "doctype": { - "type": "string", - "description": "The Frappe DocType name (e.g., 'Customer', 'Sales Invoice', 'Item', 'User'). Must match exact DocType name in system.", - }, - "data": { - "type": "object", - "description": "Document field data as key-value pairs. Include all required fields for the doctype. Example: {'customer_name': 'ABC Corp', 'customer_type': 'Company'}", - }, - "submit": { - "type": "boolean", - "default": False, - "description": "Whether to submit the document after creation (for submittable doctypes like Sales Invoice). Use true only when explicitly requested.", - }, - }, - "required": ["doctype", "data"], - }, - }, - { - "name": "document_get", - "description": "Retrieve detailed information about a specific Frappe document. Use when users ask for details about a particular record they know the name/ID of.", - "inputSchema": { - "type": "object", - "properties": { - "doctype": { - "type": "string", - "description": "The Frappe DocType name (e.g., 'Customer', 'Sales Invoice', 'Item')", - }, - "name": { - "type": "string", - "description": "The document name/ID (e.g., 'CUST-00001', 'SINV-00001'). This is the unique identifier for the document.", - }, - }, - "required": ["doctype", "name"], - }, - }, - { - "name": "document_update", - "description": "Update/modify an existing Frappe document. Use when users want to change field values in an existing record. Always fetch the document first to understand current values.", - "inputSchema": { - "type": "object", - "properties": { - "doctype": { - "type": "string", - "description": "The Frappe DocType name (e.g., 'Customer', 'Sales Invoice', 'Item')", - }, - "name": { - "type": "string", - "description": "The document name/ID to update (e.g., 'CUST-00001', 'SINV-00001')", - }, - "data": { - "type": "object", - "description": "Field updates as key-value pairs. Only include fields that need to be changed. Example: {'customer_name': 'Updated Corp Name', 'phone': '+1234567890'}", - }, - }, - "required": ["doctype", "name", "data"], - }, - }, - { - "name": "document_list", - "description": "Search and list Frappe documents with optional filtering. Use this when users want to find records, get lists of documents, or search for data. This is the primary tool for data exploration and discovery.", - "inputSchema": { - "type": "object", - "properties": { - "doctype": { - "type": "string", - "description": "The Frappe DocType to search (e.g., 'Customer', 'Sales Invoice', 'Item', 'User'). Must match exact DocType name.", - }, - "filters": { - "type": "object", - "default": {}, - "description": "Search filters as key-value pairs. Examples: {'status': 'Active'}, {'customer_type': 'Company'}, {'creation': ['>', '2024-01-01']}. Use empty {} to get all records.", - }, - "fields": { - "type": "array", - "items": {"type": "string"}, - "description": "Specific fields to retrieve. Examples: ['name', 'customer_name', 'email'], ['name', 'item_name', 'item_code']. Leave empty to get standard fields.", - }, - "limit": { - "type": "integer", - "default": 20, - "description": "Maximum number of records to return. Use 50+ for comprehensive searches, 5-10 for quick previews.", - }, - "debug": { - "type": "boolean", - "default": False, - "description": "Enable debug mode to troubleshoot when no results are returned despite expecting data.", - }, - }, - "required": ["doctype"], - }, - }, - ] - - @staticmethod - def execute_tool(tool_name: str, arguments: Dict[str, Any]) -> Dict[str, Any]: - """Execute a document tool with given arguments""" - if tool_name == "document_create": - return DocumentTools.create_document(**arguments) - elif tool_name == "document_get": - return DocumentTools.get_document(**arguments) - elif tool_name == "document_update": - return DocumentTools.update_document(**arguments) - elif tool_name == "document_list": - return DocumentTools.list_documents(**arguments) - else: - raise Exception(f"Unknown document tool: {tool_name}") - - @staticmethod - def create_document(doctype: str, data: Dict[str, Any], submit: bool = False) -> Dict[str, Any]: - """Create a new Frappe document""" - try: - # Validate doctype exists - if not frappe.db.exists("DocType", doctype): - return {"success": False, "error": f"DocType '{doctype}' does not exist"} - - # Check permissions - if not frappe.has_permission(doctype, "create"): - return {"success": False, "error": f"No create permission for {doctype}"} - - # Ensure doctype is in the data - if "doctype" not in data: - data["doctype"] = doctype - - # Create document - doc = frappe.get_doc(data) - doc.insert() - - # Submit if requested and document supports it - if submit and hasattr(doc, "submit") and doc.docstatus == 0: - doc.submit() - - return { - "success": True, - "name": doc.name, - "doctype": doctype, - "status": "Submitted" if submit else "Draft", - } - - except Exception as e: - frappe.log_error(f"assistant Create Document Error: {str(e)}") - return {"success": False, "error": str(e)} - - @staticmethod - def get_document(doctype: str, name: str) -> Dict[str, Any]: - """Retrieve a specific document""" - try: - if not frappe.db.exists("DocType", doctype): - return {"success": False, "error": f"DocType '{doctype}' does not exist"} - - if not frappe.has_permission(doctype, "read", doc=name): - return {"success": False, "error": f"No read permission for {doctype} {name}"} - - doc = frappe.get_doc(doctype, name) - return {"success": True, "doctype": doctype, "name": name, "data": doc.as_dict()} - - except Exception as e: - frappe.log_error(f"assistant Get Document Error: {str(e)}") - return {"success": False, "error": str(e)} - - @staticmethod - def update_document(doctype: str, name: str, data: Dict[str, Any]) -> Dict[str, Any]: - """Update an existing document""" - try: - if not frappe.db.exists("DocType", doctype): - return {"success": False, "error": f"DocType '{doctype}' does not exist"} - - if not frappe.has_permission(doctype, "write", doc=name): - return {"success": False, "error": f"No write permission for {doctype} {name}"} - - doc = frappe.get_doc(doctype, name) - doc.update(data) - doc.save() - - return { - "success": True, - "doctype": doctype, - "name": name, - "message": "Document updated successfully", - } - - except Exception as e: - frappe.log_error(f"assistant Update Document Error: {str(e)}") - return {"success": False, "error": str(e)} - - @staticmethod - def list_documents( - doctype: str, - filters: Dict[str, Any] = None, - fields: List[str] = None, - limit: int = 20, - debug: bool = False, - ) -> Dict[str, Any]: - """List documents with filters""" - try: - if not frappe.db.exists("DocType", doctype): - return {"success": False, "error": f"DocType '{doctype}' does not exist"} - - # Check basic read permission - if not frappe.has_permission(doctype, "read"): - return {"success": False, "error": f"No read permission for {doctype}"} - - # Get total count first to debug - try: - total_count = frappe.db.count(doctype, filters or {}) - except Exception as e: - return {"success": False, "error": f"Error counting documents: {str(e)}"} - - # Use safer field selection - safe_fields = fields or ["name"] - if "name" not in safe_fields: - safe_fields.append("name") - - # Try to add common fields if they exist - try: - meta = frappe.get_meta(doctype) - for field_name in ["creation", "modified", "owner", "modified_by"]: - if ( - meta.has_field(field_name) and field_name not in safe_fields and len(safe_fields) < 10 - ): # Limit fields to avoid issues - safe_fields.append(field_name) - except Exception as e: - frappe.log_error(f"Error accessing meta for {doctype}: {str(e)}") - - # Try the query with progressively simpler approaches - documents = [] - error_details = [] - - # Attempt 1: Full query as intended - try: - documents = frappe.get_all( - doctype, - filters=filters or {}, - fields=safe_fields, - limit=limit, - order_by="modified desc" if "modified" in safe_fields else "name desc", - ) - except Exception as e: - error_details.append(f"Full query failed: {str(e)}") - - # Attempt 2: Try with just name field - try: - documents = frappe.get_all( - doctype, filters=filters or {}, fields=["name"], limit=limit, order_by="name desc" - ) - safe_fields = ["name"] # Update to reflect what worked - except Exception as e2: - error_details.append(f"Name-only query failed: {str(e2)}") - - # Attempt 3: Try with no filters - try: - documents = frappe.get_all( - doctype, fields=["name"], limit=limit, order_by="name desc" - ) - safe_fields = ["name"] - filters = {} # Clear filters since they caused issues - except Exception as e3: - error_details.append(f"No-filter query failed: {str(e3)}") - - # Build response with debug info - response = { - "success": True, - "doctype": doctype, - "documents": documents, - "results": documents, # Add results key for API compatibility - "count": len(documents), - "total_in_db": total_count, - "fields_returned": safe_fields, - "filters_applied": filters or {}, - } - - # Add debug information if requested - if debug or error_details or (len(documents) == 0 and total_count > 0): - response["debug_info"] = { - "permission_check_passed": True, - "errors_encountered": error_details if error_details else None, - "user": frappe.session.user, - "user_roles": frappe.get_roles(), - "doctype_permissions": { - # Reporting capabilities to the caller — not a security - # boundary. Enforcement happens via the read check - # earlier in list_documents; these booleans just tell - # the LLM what actions it could take next. - "read": frappe.has_permission(doctype, "read", throw=False), - "write": frappe.has_permission(doctype, "write", throw=False), - "create": frappe.has_permission(doctype, "create", throw=False), - "delete": frappe.has_permission(doctype, "delete", throw=False), - }, - "query_attempts": len(error_details) + 1, - "final_fields_used": safe_fields, - } - - # Add warning if no documents returned but some exist in DB - if len(documents) == 0 and total_count > 0: - response["warning"] = ( - f"No documents returned but {total_count} exist in database. Possible permission or filter issues." - ) - response["suggestion"] = "Try without filters or with simpler field selection." - - return response - - except Exception as e: - frappe.log_error(f"assistant List Documents Error: {str(e)}") - return { - "success": False, - "error": str(e), - "debug": {"doctype": doctype, "filters": filters, "fields": fields}, - } diff --git a/frappe_assistant_core/plugins/core/tools/list_documents.py b/frappe_assistant_core/plugins/core/tools/list_documents.py index 30c7ed6..bdb1f92 100644 --- a/frappe_assistant_core/plugins/core/tools/list_documents.py +++ b/frappe_assistant_core/plugins/core/tools/list_documents.py @@ -134,8 +134,8 @@ def execute(self, arguments: Dict[str, Any]) -> Dict[str, Any]: filtered_fields = ["name"] # Always allow name field fields = filtered_fields - # Get documents with proper permission checking - documents = frappe.get_all( + # Get documents with Frappe's permission-aware list API. + documents = frappe.get_list( doctype, filters=filters, fields=fields, @@ -150,8 +150,24 @@ def execute(self, arguments: Dict[str, Any]) -> Dict[str, Any]: filtered_doc = filter_sensitive_fields(doc, doctype, user_role) filtered_documents.append(filtered_doc) - # Get total count for pagination info - total_count = frappe.db.count(doctype, filters) + # Get permission-aware total count for pagination info. + try: + count_result = frappe.get_list( + doctype, + filters=filters, + fields=[{"COUNT": "name", "as": "count"}], + limit=1, + ignore_permissions=False, + ) + except AttributeError: + count_result = frappe.get_list( + doctype, + filters=filters, + fields=["count(name) as count"], + limit=1, + ignore_permissions=False, + ) + total_count = count_result[0].get("count") if count_result else 0 result = { "success": True, diff --git a/frappe_assistant_core/plugins/core/tools/metadata_tools.py b/frappe_assistant_core/plugins/core/tools/metadata_tools.py index 1873233..1226271 100644 --- a/frappe_assistant_core/plugins/core/tools/metadata_tools.py +++ b/frappe_assistant_core/plugins/core/tools/metadata_tools.py @@ -88,6 +88,21 @@ def execute_tool(tool_name: str, arguments: Dict[str, Any]) -> Dict[str, Any]: else: raise Exception(f"Unknown metadata tool: {tool_name}") + @staticmethod + def _serialize_field(field) -> Dict[str, Any]: + """Serialize a DocField row into the shape returned by get_doctype_metadata.""" + return { + "fieldname": field.fieldname, + "label": field.label, + "fieldtype": field.fieldtype, + "options": field.options, + "reqd": field.reqd, + "read_only": field.read_only, + "hidden": field.hidden, + "default": field.default, + "description": field.description, + } + @staticmethod def get_doctype_metadata(doctype: str) -> Dict[str, Any]: """Get DocType metadata and field information""" @@ -100,40 +115,44 @@ def get_doctype_metadata(doctype: str) -> Dict[str, Any]: meta = frappe.get_meta(doctype) - # Build field information - fields = [] - for field in meta.fields: - field_info = { - "fieldname": field.fieldname, - "label": field.label, - "fieldtype": field.fieldtype, - "options": field.options, - "reqd": field.reqd, - "read_only": field.read_only, - "hidden": field.hidden, - "default": field.default, - "description": field.description, + fields = [MetadataTools._serialize_field(field) for field in meta.fields] + + link_fields = [ + {"fieldname": field.fieldname, "label": field.label, "options": field.options} + for field in meta.get_link_fields() + ] + + # Child tables: include the child DocType's own field metadata so the + # caller can build nested row objects without a second tool call (#192). + child_tables = [] + for table_field in meta.get_table_fields(): + child_doctype = table_field.options + child_entry = { + "fieldname": table_field.fieldname, + "label": table_field.label, + "fieldtype": table_field.fieldtype, + "options": child_doctype, + "reqd": table_field.reqd, + "fields": [], } - fields.append(field_info) - - # Build link fields information - link_fields = [] - for field in meta.get_link_fields(): - link_fields.append( - {"fieldname": field.fieldname, "label": field.label, "options": field.options} - ) + if child_doctype and frappe.db.exists("DocType", child_doctype): + child_meta = frappe.get_meta(child_doctype) + child_entry["fields"] = [MetadataTools._serialize_field(f) for f in child_meta.fields] + child_tables.append(child_entry) return { "success": True, "doctype": doctype, "module": meta.module, - "is_submittable": meta.is_submittable, - "is_tree": meta.is_tree, - "is_single": meta.istable, + "is_submittable": bool(meta.is_submittable), + "is_tree": bool(meta.is_tree), + "is_single": bool(meta.issingle), + "is_child_table": bool(meta.istable), "naming_rule": meta.naming_rule, "title_field": meta.title_field, "fields": fields, "link_fields": link_fields, + "child_tables": child_tables, "permissions": [p.as_dict() for p in meta.permissions], } diff --git a/frappe_assistant_core/plugins/core/tools/report_requirements.py b/frappe_assistant_core/plugins/core/tools/report_requirements.py index 52a509d..849ee64 100644 --- a/frappe_assistant_core/plugins/core/tools/report_requirements.py +++ b/frappe_assistant_core/plugins/core/tools/report_requirements.py @@ -131,10 +131,13 @@ def execute(self, arguments: Dict[str, Any]) -> Dict[str, Any]: report_name, column_result.get("report_type") ) - # For Script Reports, try to parse filters from JS file and add to main response + # For Script Reports, discover filters from multiple sources and + # add to main response. if column_result.get("report_type") == "Script Report": - module_name = report_doc.module - parsed_filters = self._parse_script_report_filters(report_name, module_name) + parsed_filters, diagnostics = self._discover_script_report_filters( + report_name, report_doc + ) + result["discovery_diagnostics"] = diagnostics if parsed_filters and parsed_filters.get("filters"): result["filters_definition"] = parsed_filters["filters"] @@ -285,80 +288,196 @@ def _analyze_filter_requirements(self, report_name: str, report_type: str) -> Di return requirements + def _discover_script_report_filters(self, report_name: str, report_doc): + """ + Discover Script Report filters from multiple sources, first non-empty + wins, recording a diagnostic for each attempt so a silent empty result + is debuggable by agents and users (issue #203). + + Order: + 1. ``Report.filters`` child table (structured, no parsing). + 2. JS — on-disk .js file, then the ``Report.javascript`` DB field. + + Returns: + (parsed_filters_or_None, discovery_diagnostics dict) + """ + diagnostics = {} + + # --- Source 1: Report.filters child table --- + child_rows = report_doc.get("filters") or [] + diagnostics["filters_child_table"] = { + "row_count": len(child_rows), + "status": "success" if child_rows else "empty", + } + if child_rows: + parsed = self._parse_filters_child_table(child_rows) + if parsed.get("filters"): + diagnostics["filters_child_table"]["filters_found"] = len(parsed["filters"]) + return parsed, diagnostics + + # --- Source 2: JavaScript (disk file, then DB field) --- + self._last_discovery_diagnostics = {} + parsed = self._parse_script_report_filters(report_name, report_doc.module) + diagnostics["javascript"] = getattr(self, "_last_discovery_diagnostics", {}) + return parsed, diagnostics + + def _parse_filters_child_table(self, child_rows) -> Dict[str, Any]: + """Convert ``Report.filters`` child-table rows to the parsed-filter shape.""" + filters = [] + required_filters = [] + optional_filters = [] + for row in child_rows: + fieldname = row.get("fieldname") + if not fieldname: + continue + is_required = bool(row.get("mandatory") or row.get("reqd")) + filter_def = { + "fieldname": fieldname, + "label": row.get("label") or fieldname, + "fieldtype": row.get("fieldtype"), + "options": row.get("options"), + "default": row.get("default_value") or row.get("default"), + "required": is_required, + } + # Drop empty keys for a clean payload. + filter_def = {k: v for k, v in filter_def.items() if v not in (None, "")} + filter_def["required"] = is_required + filters.append(filter_def) + (required_filters if is_required else optional_filters).append(fieldname) + + return { + "filters": filters, + "required_filters": required_filters, + "optional_filters": optional_filters, + } + + def _resolve_report_js_path(self, report_name: str, module_name: str): + """ + Resolve the on-disk path of a Script Report's .js file. + + Uses Frappe's own resolution (``get_module_path`` + ``scrub``) rather + than reconstructing the path by looping over installed apps, so custom + apps with non-trivial package layouts resolve correctly. Returns None + for custom (DB-only) modules, which have no disk path (issue #203). + + Returns: + Absolute path string, or None if the module has no disk location. + """ + import os + + from frappe.modules import get_module_path, scrub + + # Custom modules exist only in the DB (no files on disk). + if frappe.get_cached_value("Module Def", module_name, "custom"): + return None + + module_path = get_module_path(module_name) + report_folder = scrub(report_name) + return os.path.join(module_path, "report", report_folder, f"{report_folder}.js") + + def _extract_filters_from_js(self, js_content: str): + """ + Extract the ``filters: [...]`` array from report JS and parse it. + + Returns: + (parsed_filters_or_None, diagnostic_note). diagnostic_note explains + why nothing was parsed, so callers can surface it. + """ + # Find the start of the filters array. Anchor on "filters:" then the + # next "[" — note this does not handle programmatically-built filters + # (e.g. ``filters: get_filters()``); that case is reported via the + # diagnostic note rather than failing silently. + filters_start = js_content.find("filters:") + if filters_start == -1: + filters_start = js_content.find('"filters"') + if filters_start == -1: + return None, "no 'filters:' key found in JS" + + bracket_start = js_content.find("[", filters_start) + if bracket_start == -1: + return None, "no '[' after 'filters:' (filters may be built programmatically)" + + # Guard against anchoring far past the key (e.g. filters: fn(); ... [ ). + between = js_content[filters_start:bracket_start] + if "(" in between or ";" in between: + return None, "'filters:' is not a literal array (built programmatically)" + + # Count brackets to find the matching closing bracket. + bracket_count = 0 + bracket_end = bracket_start + for i in range(bracket_start, len(js_content)): + if js_content[i] == "[": + bracket_count += 1 + elif js_content[i] == "]": + bracket_count -= 1 + if bracket_count == 0: + bracket_end = i + break + + if bracket_count != 0: + return None, "mismatched brackets in filters array" + + filters_text = js_content[bracket_start + 1 : bracket_end] + parsed = self._parse_js_filter_array(filters_text) + if not parsed or not parsed.get("filters"): + return None, "filters array found but no filter objects parsed (unexpected JS syntax)" + return parsed, None + def _parse_script_report_filters(self, report_name: str, module_name: str) -> Dict[str, Any]: """ - Parse JavaScript filter definitions from Script Report .js file. + Parse JavaScript filter definitions for a Script Report. - Args: - report_name: Name of the report - module_name: Module name (e.g., "Selling", "Stock") + Tries the on-disk .js file first (path resolved via Frappe), then falls + back to the ``Report.javascript`` DB field (covers custom DB-only + modules and reports whose JS lives in the doc). Stores a diagnostic of + what was attempted on ``frappe.local`` for the caller to surface. Returns: - Dictionary containing parsed filters, or None if parsing fails + Dictionary containing parsed filters, or None if parsing fails. """ import os - import re + diag = {"js_file": {}, "js_db_field": {}} try: - # Construct path to JS file - # Format: apps/{app_name}/{module}/report/{report_name}/{report_name}.js - report_folder = report_name.lower().replace(" ", "_").replace("-", "_") - module_folder = module_name.lower().replace(" ", "_") - - # Find the app that contains this module - for app in frappe.get_installed_apps(): - app_path = frappe.get_app_path(app) - js_path = os.path.join( - app_path, module_folder, "report", report_folder, f"{report_folder}.js" - ) - - if os.path.exists(js_path): - # nosemgrep: frappe-semgrep-rules.rules.security.frappe-security-file-traversal — path built from frappe.get_app_path + report metadata, not user input - with open(js_path, encoding="utf-8") as f: - js_content = f.read() - - # Extract filter array using regex - # Pattern: frappe.query_reports["Report Name"] = { filters: [...] } - # Need to handle nested objects with proper bracket counting - pattern = r'frappe\.query_reports\[["\'].*?["\']\]\s*=\s*\{.*?filters:\s*\[(.*?)\]' - - # Find the start of filters array - filters_start = js_content.find("filters:") - if filters_start == -1: - frappe.logger().debug(f"No 'filters:' found in JS file for {report_name}") - return None - - # Find the opening bracket - bracket_start = js_content.find("[", filters_start) - if bracket_start == -1: - frappe.logger().debug(f"No opening bracket after 'filters:' for {report_name}") - return None - - # Count brackets to find matching closing bracket - bracket_count = 0 - bracket_end = bracket_start - for i in range(bracket_start, len(js_content)): - if js_content[i] == "[": - bracket_count += 1 - elif js_content[i] == "]": - bracket_count -= 1 - if bracket_count == 0: - bracket_end = i - break - - if bracket_count != 0: - frappe.logger().debug(f"Mismatched brackets in filters array for {report_name}") - return None - - # Extract filter content (inside the brackets) - filters_text = js_content[bracket_start + 1 : bracket_end] - parsed_filters = self._parse_js_filter_array(filters_text) - return parsed_filters - - frappe.logger().debug(f"JS file not found for {report_name}") + # --- Source 1: on-disk .js file --- + js_path = self._resolve_report_js_path(report_name, module_name) + diag["js_file"]["path"] = js_path + if js_path and os.path.exists(js_path): + diag["js_file"]["file_exists"] = True + diag["js_file"]["file_readable"] = os.access(js_path, os.R_OK) + # nosemgrep: frappe-semgrep-rules.rules.security.frappe-security-file-traversal — path built from frappe.get_module_path + scrubbed report metadata, not user input + with open(js_path, encoding="utf-8") as f: + js_content = f.read() + parsed, note = self._extract_filters_from_js(js_content) + diag["js_file"]["status"] = "success" if parsed else "failed" + diag["js_file"]["filters_found"] = len(parsed["filters"]) if parsed else 0 + if note: + diag["js_file"]["note"] = note + if parsed: + self._last_discovery_diagnostics = diag + return parsed + else: + diag["js_file"]["file_exists"] = False + + # --- Source 2: Report.javascript DB field --- + js_db = frappe.db.get_value("Report", report_name, "javascript") + diag["js_db_field"]["present"] = bool(js_db) + if js_db: + parsed, note = self._extract_filters_from_js(js_db) + diag["js_db_field"]["status"] = "success" if parsed else "failed" + diag["js_db_field"]["filters_found"] = len(parsed["filters"]) if parsed else 0 + if note: + diag["js_db_field"]["note"] = note + if parsed: + self._last_discovery_diagnostics = diag + return parsed + + self._last_discovery_diagnostics = diag return None except Exception as e: + diag["error"] = f"{type(e).__name__}: {str(e)}" + self._last_discovery_diagnostics = diag frappe.log_error(f"Error parsing Script Report filters for {report_name}: {str(e)}") return None @@ -399,27 +518,36 @@ def _parse_js_filter_array(self, filters_text: str) -> Dict[str, Any]: for filter_obj in filter_objects: filter_def = {} + # Keys may be bare (fieldname:) or quoted (JSON-style "fieldname":), + # and may use template literals (`x`). Tolerate optional surrounding + # quotes on the key so JSON-style report JS isn't silently skipped + # (issue #203). # Extract fieldname - fieldname_match = re.search(r'fieldname:\s*["\']([^"\']+)["\']', filter_obj) + fieldname_match = re.search(r'["\']?fieldname["\']?\s*:\s*["\']([^"\']+)["\']', filter_obj) if fieldname_match: filter_def["fieldname"] = fieldname_match.group(1) else: continue # Skip if no fieldname - # Extract label + # Extract label — supports __("x"), "x", and `x` (template literal), + # with bare or quoted key. label_match = re.search( - r'label:\s*__\(["\']([^"\']+)["\']\)|label:\s*["\']([^"\']+)["\']', filter_obj + r'["\']?label["\']?\s*:\s*__\(\s*[`"\']([^`"\']+)[`"\']\s*\)' + r'|["\']?label["\']?\s*:\s*[`"\']([^`"\']+)[`"\']', + filter_obj, ) if label_match: filter_def["label"] = label_match.group(1) or label_match.group(2) # Extract fieldtype - fieldtype_match = re.search(r'fieldtype:\s*["\']([^"\']+)["\']', filter_obj) + fieldtype_match = re.search(r'["\']?fieldtype["\']?\s*:\s*["\']([^"\']+)["\']', filter_obj) if fieldtype_match: filter_def["fieldtype"] = fieldtype_match.group(1) # Extract options (can be array or string) - options_match = re.search(r'options:\s*(\[[\s\S]*?\]|["\'][^"\']+["\'])', filter_obj) + options_match = re.search( + r'["\']?options["\']?\s*:\s*(\[[\s\S]*?\]|["\'][^"\']+["\'])', filter_obj + ) if options_match: options_str = options_match.group(1) if options_str.startswith("["): @@ -431,12 +559,15 @@ def _parse_js_filter_array(self, filters_text: str) -> Dict[str, Any]: filter_def["options"] = options_str.strip("\"'") # Extract default value - default_match = re.search(r'default:\s*["\']([^"\']+)["\']|default:\s*(\d+)', filter_obj) + default_match = re.search( + r'["\']?default["\']?\s*:\s*["\']([^"\']+)["\']|["\']?default["\']?\s*:\s*(\d+)', + filter_obj, + ) if default_match: filter_def["default"] = default_match.group(1) or default_match.group(2) # Extract required flag - reqd_match = re.search(r"reqd:\s*(1|true)", filter_obj, re.IGNORECASE) + reqd_match = re.search(r'["\']?reqd["\']?\s*:\s*(1|true)', filter_obj, re.IGNORECASE) filter_def["required"] = bool(reqd_match) filters.append(filter_def) diff --git a/frappe_assistant_core/plugins/core/tools/search_tools.py b/frappe_assistant_core/plugins/core/tools/search_tools.py index fa3da38..3ddba2e 100644 --- a/frappe_assistant_core/plugins/core/tools/search_tools.py +++ b/frappe_assistant_core/plugins/core/tools/search_tools.py @@ -108,12 +108,16 @@ def global_search(query: str, limit: int = 20) -> Dict[str, Any]: if not frappe.has_permission(doctype, "read"): continue - # Simple search using name field - doctype_results = frappe.get_all( + # Simple search using name field. Use frappe.get_list (not + # get_all) so DocType-level AND user/row-level permissions are + # applied — get_all bypasses permissions and would leak + # records the user cannot read (issue #189). + doctype_results = frappe.get_list( doctype, filters={"name": ["like", f"%{query}%"]}, fields=["name"], limit=5, # Limit per doctype + ignore_permissions=False, ) # Add doctype info to results @@ -179,13 +183,16 @@ def search_doctype(doctype: str, query: str, limit: int = 20) -> Dict[str, Any]: for field in search_fields: filters.append([doctype, field, "like", f"%{query}%"]) - # Execute search - results = frappe.get_all( + # Execute search. Use frappe.get_list (not get_all) so the results + # are filtered by the user's read permissions — get_all bypasses + # permissions and would leak inaccessible records (issue #189). + results = frappe.get_list( doctype, or_filters=filters, fields=["name"] + search_fields, limit=limit, order_by="modified desc", + ignore_permissions=False, ) return { diff --git a/frappe_assistant_core/tests/test_document_tools.py b/frappe_assistant_core/tests/test_document_tools.py index a6286d1..bf98ae2 100644 --- a/frappe_assistant_core/tests/test_document_tools.py +++ b/frappe_assistant_core/tests/test_document_tools.py @@ -21,6 +21,7 @@ import json import unittest +from contextlib import ExitStack from unittest.mock import MagicMock, patch import frappe @@ -123,6 +124,128 @@ def test_list_documents_via_execute_tool(self): except Exception as e: self.fail(f"Tool execution raised exception: {str(e)}") + def test_list_documents_uses_permission_aware_queries_for_data_and_count(self): + """Regression guard for #189: list_documents must not use permission-bypassing APIs.""" + from frappe_assistant_core.plugins.core.tools.list_documents import DocumentList + + with ExitStack() as stack: + stack.enter_context( + patch( + "frappe_assistant_core.core.security_config.validate_document_access", + return_value={"success": True, "role": "Default"}, + ) + ) + stack.enter_context( + patch( + "frappe_assistant_core.core.security_config.filter_sensitive_fields", + side_effect=lambda doc, _doctype, _role: doc, + ) + ) + stack.enter_context( + patch( + "frappe_assistant_core.plugins.core.tools.list_documents.frappe.session", + MagicMock(user="restricted@example.com"), + ) + ) + get_all = stack.enter_context( + patch( + "frappe_assistant_core.plugins.core.tools.list_documents.frappe.get_all", + side_effect=AssertionError("frappe.get_all bypasses DocType permissions"), + ) + ) + db_count = stack.enter_context( + patch( + "frappe_assistant_core.plugins.core.tools.list_documents.frappe.db.count", + side_effect=AssertionError("frappe.db.count bypasses DocType permissions"), + ) + ) + get_list = stack.enter_context( + patch("frappe_assistant_core.plugins.core.tools.list_documents.frappe.get_list") + ) + get_list.side_effect = [ + [{"name": "EMP-0001", "employee_name": "Allowed Employee"}], + [{"count": 1}], + ] + + result = DocumentList().execute( + { + "doctype": "Employee", + "filters": {}, + "fields": ["name", "employee_name"], + "limit": 20, + } + ) + + self.assertTrue(result.get("success"), result) + self.assertEqual(result.get("count"), 1) + self.assertEqual(result.get("total_count"), 1) + get_all.assert_not_called() + db_count.assert_not_called() + self.assertEqual(get_list.call_count, 2) + + data_call = get_list.call_args_list[0] + self.assertEqual(data_call.args[0], "Employee") + self.assertEqual(data_call.kwargs["fields"], ["name", "employee_name"]) + self.assertEqual(data_call.kwargs["limit"], 20) + self.assertFalse(data_call.kwargs["ignore_permissions"]) + + count_call = get_list.call_args_list[1] + self.assertEqual(count_call.args[0], "Employee") + self.assertEqual(count_call.kwargs["fields"], [{"COUNT": "name", "as": "count"}]) + self.assertEqual(count_call.kwargs["limit"], 1) + self.assertFalse(count_call.kwargs["ignore_permissions"]) + + def test_list_documents_count_falls_back_to_legacy_aggregate_syntax(self): + """Frappe 15 does not support dict aggregate fields.""" + from frappe_assistant_core.plugins.core.tools.list_documents import DocumentList + + with ExitStack() as stack: + stack.enter_context( + patch( + "frappe_assistant_core.core.security_config.validate_document_access", + return_value={"success": True, "role": "Default"}, + ) + ) + stack.enter_context( + patch( + "frappe_assistant_core.core.security_config.filter_sensitive_fields", + side_effect=lambda doc, _doctype, _role: doc, + ) + ) + stack.enter_context( + patch( + "frappe_assistant_core.plugins.core.tools.list_documents.frappe.session", + MagicMock(user="restricted@example.com"), + ) + ) + get_list = stack.enter_context( + patch("frappe_assistant_core.plugins.core.tools.list_documents.frappe.get_list") + ) + get_list.side_effect = [ + [{"name": "EMP-0001", "employee_name": "Allowed Employee"}], + AttributeError("'dict' object has no attribute 'lower'"), + [{"count": 1}], + ] + + result = DocumentList().execute( + { + "doctype": "Employee", + "filters": {}, + "fields": ["name", "employee_name"], + "limit": 20, + } + ) + + self.assertTrue(result.get("success"), result) + self.assertEqual(result.get("total_count"), 1) + self.assertEqual(get_list.call_count, 3) + + fallback_count_call = get_list.call_args_list[2] + self.assertEqual(fallback_count_call.args[0], "Employee") + self.assertEqual(fallback_count_call.kwargs["fields"], ["count(name) as count"]) + self.assertEqual(fallback_count_call.kwargs["limit"], 1) + self.assertFalse(fallback_count_call.kwargs["ignore_permissions"]) + def test_update_document_basic(self): """Test basic document update""" if not self.registry.has_tool("update_document"): diff --git a/frappe_assistant_core/tests/test_mcp_concurrency.py b/frappe_assistant_core/tests/test_mcp_concurrency.py new file mode 100644 index 0000000..893d3b8 --- /dev/null +++ b/frappe_assistant_core/tests/test_mcp_concurrency.py @@ -0,0 +1,256 @@ +# Frappe Assistant Core - AI Assistant integration for Frappe Framework +# Copyright (C) 2025 Paul Clinton +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +""" +Concurrency regression tests for the MCP server tool registry. + +Regression tests for: https://github.com/buildswithpaul/Frappe_Assistant_Core/issues/197 + +Background +---------- +The MCP endpoint uses a single module-level ``MCPServer`` instance. The old +request flow cleared and repopulated that instance's shared ``_tool_registry`` +on every request: + + mcp._tool_registry.clear() + register_base_tool(mcp, tool_instance) # repeated per tool + +With concurrent ``tools/call`` requests handled in the same worker, one request +could ``clear()`` the registry while another was validating or executing a tool +against it. Symptoms reported in #197: + + * an in-flight call failing with "Tool '' not found. Available tools: + [...]" even though the tool exists, and + * only one of several concurrent executions making it into Assistant Audit + Log. + +The fix (Option A) builds a per-request tool registry on the call stack and +passes it into ``MCPServer.handle()``. No request mutates shared state, so +concurrent requests are isolated. + +These tests exercise ``MCPServer.handle()`` directly with mocked Werkzeug +requests, which is the layer where the race lived. The test tool overrides +``log_execution`` to a no-op so tool execution stays entirely in memory — the +threaded test never opens a DB connection and therefore never escapes the test +runner's transaction/rollback boundary. +""" + +import json +import threading +import time +from collections import OrderedDict +from typing import Any, Dict +from unittest.mock import MagicMock + +import frappe +from werkzeug.wrappers import Response + +from frappe_assistant_core.core.base_tool import BaseTool +from frappe_assistant_core.mcp.server import MCPServer +from frappe_assistant_core.mcp.tool_adapter import build_tool_dict +from frappe_assistant_core.tests.base_test import BaseAssistantTest + + +class _SlowEchoTool(BaseTool): + """A BaseTool that sleeps briefly (to force request overlap) then echoes. + + The sleep widens the window in which two requests are simultaneously inside + ``handle()`` — under the old shared-registry code this is exactly when one + request's ``clear()`` corrupted another's lookup. ``log_execution`` is a + no-op so executing the tool never touches the database, keeping the threaded + test free of cross-connection state. + """ + + def __init__(self, name: str, delay: float = 0.02): + super().__init__() + self.name = name + self.description = "Concurrency test tool" + self.inputSchema = { + "type": "object", + "properties": {"doc": {"type": "string"}}, + } + self.requires_permission = None # skip the DocType permission check + self.source_app = "frappe_assistant_core" + self._delay = delay + + def execute(self, arguments: Dict[str, Any]) -> Any: + if self._delay: + time.sleep(self._delay) + return {"echo": arguments.get("doc")} + + def log_execution(self, *args, **kwargs): + # No-op: keep execution in memory so threads need no DB connection. + return None + + +def _make_request(tool_name: str, doc: str, request_id: int) -> MagicMock: + """Build a mock Werkzeug request carrying a tools/call JSON-RPC payload.""" + payload = { + "jsonrpc": "2.0", + "id": request_id, + "method": "tools/call", + "params": {"name": tool_name, "arguments": {"doc": doc}}, + } + request = MagicMock() + request.method = "POST" + # Real dict so .get("mcp-protocol-version") works on the global proxy path. + request.headers = {} + request.get_json.return_value = payload + request.get_data.return_value = json.dumps(payload) + return request + + +def _handle(server: MCPServer, request: MagicMock, tool_registry) -> Response: + """Call server.handle with the mock bound to frappe.local.request. + + MCPServer reads the global ``frappe.request`` proxy (e.g. for the + mcp-protocol-version response header), which is unbound under tests unless we + bind a request onto ``frappe.local``. ``frappe.local`` is thread-local, so a + worker thread must bind its own request; this helper does that for whichever + thread calls it. + """ + frappe.local.request = request + return server.handle(request, Response(), tool_registry=tool_registry) + + +def _registry_with(*tools) -> "OrderedDict[str, dict]": + """Build a per-request registry dict from BaseTool instances.""" + reg = OrderedDict() + for tool in tools: + reg[tool.name] = build_tool_dict(tool) + return reg + + +def _result_of(response: Response) -> Dict[str, Any]: + """Parse a JSON-RPC Response body and return its ``result`` payload.""" + body = json.loads(response.get_data(as_text=True)) + return body.get("result", {}) + + +class TestMCPRegistryIsolation(BaseAssistantTest): + """A request must route against the registry passed into handle(), never a + shared/global one — this is the core of the #197 fix.""" + + def test_handle_uses_per_request_registry_not_shared(self): + """A tool present only in the per-request registry must be callable even + when the server's shared registry is empty. + + Under the old code, ``_handle_tools_call`` read ``self._tool_registry``, + so a tool absent from the shared singleton produced "tool not found". + """ + server = MCPServer("test") + self.assertEqual(len(server._tool_registry), 0, "shared registry should start empty") + + tool = _SlowEchoTool("only_in_request_registry", delay=0) + request = _make_request(tool.name, "DOC-1", request_id=1) + + response = _handle(server, request, _registry_with(tool)) + result = _result_of(response) + + self.assertFalse(result.get("isError"), f"unexpected error: {result}") + self.assertIn("DOC-1", result["content"][0]["text"]) + + def test_mutating_shared_registry_does_not_affect_in_flight_request(self): + """Clearing the shared singleton (the old per-request side effect) must + have zero impact on a request that carries its own registry.""" + server = MCPServer("test") + tool = _SlowEchoTool("isolated_tool", delay=0) + per_request = _registry_with(tool) + + # Simulate another request wiping the shared registry mid-flight. + server._tool_registry.clear() + + response = _handle(server, _make_request(tool.name, "DOC-2", 2), per_request) + result = _result_of(response) + + self.assertFalse(result.get("isError"), f"unexpected error: {result}") + self.assertIn("DOC-2", result["content"][0]["text"]) + + def test_distinct_registries_route_independently(self): + """Two requests carrying disjoint registries each resolve only their own + tool — a stand-in for two concurrent users with different tool sets. The + shared singleton is empty, so any leakage would surface as 'not found'. + """ + server = MCPServer("test") + tool_a = _SlowEchoTool("tool_a", delay=0) + tool_b = _SlowEchoTool("tool_b", delay=0) + + # Request for A sees only tool_a; request for B sees only tool_b. + resp_a = _handle(server, _make_request("tool_a", "A", 1), _registry_with(tool_a)) + resp_b = _handle(server, _make_request("tool_b", "B", 2), _registry_with(tool_b)) + self.assertFalse(_result_of(resp_a).get("isError")) + self.assertFalse(_result_of(resp_b).get("isError")) + + # A's registry must not expose B's tool, and vice versa. + cross = _handle(server, _make_request("tool_b", "X", 3), _registry_with(tool_a)) + self.assertTrue(_result_of(cross).get("isError")) + self.assertIn("not found", _result_of(cross)["content"][0]["text"]) + + +class TestMCPConcurrentToolsCall(BaseAssistantTest): + """End-to-end: N overlapping tools/call requests against one shared + MCPServer must all succeed, with no spurious 'tool not found'. + + Execution stays in memory (the tool's ``log_execution`` is a no-op), so the + worker threads need no DB connection and nothing escapes the test runner's + rollback. This isolates the property under test — registry isolation under + concurrency — from Frappe's per-thread DB-context mechanics. + """ + + def test_concurrent_calls_do_not_corrupt_registry(self): + server = MCPServer("test") + tool = _SlowEchoTool("concurrent_tool", delay=0.03) + + n = 8 + docs = [f"DOC-{i}" for i in range(n)] + results: Dict[int, Dict[str, Any]] = {} + errors: list = [] + start_barrier = threading.Barrier(n) + results_lock = threading.Lock() + + # Each thread mimics one concurrent request hitting the same worker: its + # own per-request tool registry (as the endpoint now builds) handed to a + # shared MCPServer instance — the production module-level singleton. + def run_call(index: int): + try: + start_barrier.wait() # release all threads together to maximise overlap + per_request = _registry_with(tool) + request = _make_request(tool.name, docs[index], request_id=index) + response = _handle(server, request, per_request) + with results_lock: + results[index] = _result_of(response) + except Exception as e: # pragma: no cover - surfaced via assertion below + with results_lock: + errors.append(f"thread {index}: {type(e).__name__}: {e}") + + threads = [threading.Thread(target=run_call, args=(i,)) for i in range(n)] + for t in threads: + t.start() + for t in threads: + t.join(timeout=30) + + self.assertEqual(errors, [], f"threads raised: {errors}") + self.assertEqual(len(results), n, "every request should have produced a result") + + # No request saw a corrupted registry ("tool not found"), and each got + # its own arguments back — proving the in-flight calls did not clobber + # one another's registry or routing. + for index, result in results.items(): + self.assertFalse( + result.get("isError"), + f"request {index} failed (registry corruption regression): {result}", + ) + self.assertIn(docs[index], result["content"][0]["text"]) diff --git a/frappe_assistant_core/tests/test_metadata_tools.py b/frappe_assistant_core/tests/test_metadata_tools.py index 2290378..65f11aa 100644 --- a/frappe_assistant_core/tests/test_metadata_tools.py +++ b/frappe_assistant_core/tests/test_metadata_tools.py @@ -110,6 +110,44 @@ def test_list_doctypes_custom_only(self): def test_list_doctypes_with_module_filter(self): self.skipTest("Module filter test placeholder") + def test_get_doctype_metadata_includes_child_tables(self): + """Regression guard for #192: child tables must be surfaced with their own field metadata.""" + from frappe_assistant_core.plugins.core.tools.metadata_tools import MetadataTools + + # User ships with at least one Table field ("roles" -> "Has Role") in every Frappe install. + result = MetadataTools.get_doctype_metadata("User") + + self.assertTrue(result.get("success"), result) + self.assertIn("child_tables", result) + child_tables = result["child_tables"] + self.assertIsInstance(child_tables, list) + + roles_entry = next((c for c in child_tables if c["fieldname"] == "roles"), None) + self.assertIsNotNone(roles_entry, f"Expected 'roles' in child_tables, got {child_tables}") + self.assertEqual(roles_entry["options"], "Has Role") + self.assertIn(roles_entry["fieldtype"], ("Table", "Table MultiSelect")) + + # Recursive child field metadata must be present so create_document has everything it needs. + self.assertTrue(roles_entry["fields"], "Child table 'roles' should expose its own fields") + child_field_names = {f["fieldname"] for f in roles_entry["fields"]} + self.assertIn("role", child_field_names) + + def test_get_doctype_metadata_distinguishes_single_from_child_table(self): + """Regression guard for #192: is_single must use meta.issingle, not meta.istable.""" + from frappe_assistant_core.plugins.core.tools.metadata_tools import MetadataTools + + # System Settings is a Single doctype (issingle=1, istable=0). + single_result = MetadataTools.get_doctype_metadata("System Settings") + self.assertTrue(single_result.get("success"), single_result) + self.assertTrue(single_result["is_single"]) + self.assertFalse(single_result["is_child_table"]) + + # Has Role is a child table (issingle=0, istable=1). + child_result = MetadataTools.get_doctype_metadata("Has Role") + self.assertTrue(child_result.get("success"), child_result) + self.assertFalse(child_result["is_single"]) + self.assertTrue(child_result["is_child_table"]) + class TestMetadataToolsIntegration(BaseAssistantTest): """Integration tests for metadata tools""" diff --git a/frappe_assistant_core/tests/test_oauth_metadata_port.py b/frappe_assistant_core/tests/test_oauth_metadata_port.py new file mode 100644 index 0000000..ccc61d3 --- /dev/null +++ b/frappe_assistant_core/tests/test_oauth_metadata_port.py @@ -0,0 +1,160 @@ +# Frappe Assistant Core - AI Assistant integration for Frappe Framework +# Copyright (C) 2025 Paul Clinton +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +""" +Regression tests for: https://github.com/buildswithpaul/Frappe_Assistant_Core/issues/196 + +OAuth metadata URLs must honor a configured host_name verbatim, including a +non-standard port. Frappe's frappe.oauth.get_server_url() reconstructs the +host/port from the request or the Social Login Key base_url and drops the +configured port, which broke the OAuth handshake for deployments behind a +port-restricted network (public URL on e.g. :8000). + +The MCP endpoint's 401 WWW-Authenticate header advertises the +resource_metadata URL; it previously used get_server_url() and dropped the +port. It now builds from get_public_base_url() (the same host_name-aware base +the discovery endpoints use), so the port is preserved. +""" + +from contextlib import ExitStack +from unittest.mock import MagicMock, patch + +import frappe +from werkzeug.wrappers import Response + +from frappe_assistant_core.tests.base_test import BaseAssistantTest + +_HOST_WITH_PORT = "https://mysite.example.net:8000" + + +def _patch_conf(stack: ExitStack, conf: dict): + """Patch frappe.conf.get to read from a test dict.""" + stack.enter_context(patch.object(frappe, "conf", conf)) + + +class TestPublicBaseUrlHonorsPort(BaseAssistantTest): + """get_public_base_url must return a host_name with a non-standard port + verbatim, without routing through Frappe's port-dropping get_server_url.""" + + def test_host_name_with_port_used_verbatim(self): + from frappe_assistant_core.api import oauth_discovery + + with ExitStack() as stack: + _patch_conf(stack, {"host_name": _HOST_WITH_PORT}) + # If host_name is honored, get_server_url must not even be consulted. + get_server_url = stack.enter_context( + patch( + "frappe_assistant_core.api.oauth_discovery.get_server_url", + side_effect=AssertionError("host_name with scheme must be used verbatim"), + ) + ) + + base = oauth_discovery.get_public_base_url() + + self.assertEqual(base, _HOST_WITH_PORT) + self.assertIn(":8000", base) + get_server_url.assert_not_called() + + def test_host_name_without_scheme_falls_back_to_server_url(self): + """Without a scheme in host_name, behavior is unchanged (uses + get_server_url) — this fix does not alter the fallback path.""" + from frappe_assistant_core.api import oauth_discovery + + with ExitStack() as stack: + _patch_conf(stack, {"host_name": "mysite.example.net:8000"}) # no scheme + stack.enter_context( + patch( + "frappe_assistant_core.api.oauth_discovery.get_server_url", + return_value="https://internal.local", + ) + ) + + base = oauth_discovery.get_public_base_url() + + self.assertEqual(base, "https://internal.local") + + +class TestWwwAuthenticatePreservesPort(BaseAssistantTest): + """The MCP 401 WWW-Authenticate resource_metadata URL must keep the port.""" + + def _make_request(self, auth_header: str = ""): + request = MagicMock() + request.method = "POST" + request.headers = {"Authorization": auth_header} if auth_header else {} + # A realistic *internal* request URL with no public port — this is what + # Frappe's get_server_url() would fall back to, so the pre-fix code + # produces a port-less metadata URL and these tests fail cleanly on it. + request.url = "https://internal.local/api/method/frappe_assistant_core.api.fac_endpoint.handle_mcp" + return request + + def test_unauthenticated_resource_metadata_keeps_port(self): + from frappe_assistant_core.api import fac_endpoint + + with ExitStack() as stack: + _patch_conf(stack, {"host_name": _HOST_WITH_PORT}) + # Guard: the endpoint must not fall back to Frappe's get_server_url. + stack.enter_context( + patch( + "frappe_assistant_core.api.oauth_discovery.get_server_url", + side_effect=AssertionError("must build metadata URL from host_name, not get_server_url"), + ) + ) + frappe.local.request = self._make_request(auth_header="") + + # No valid auth → returns a 401 Response with WWW-Authenticate. + result = fac_endpoint._authenticate_mcp_request() + + self.assertIsInstance(result, Response) + self.assertEqual(result.status_code, 401) + www_auth = result.headers.get("WWW-Authenticate", "") + self.assertIn("resource_metadata=", www_auth) + self.assertIn( + f'resource_metadata="{_HOST_WITH_PORT}/.well-known/oauth-protected-resource"', + www_auth, + ) + self.assertIn(":8000", www_auth) + + def test_invalid_bearer_token_resource_metadata_keeps_port(self): + """The Bearer-token-not-found 401 path must also keep the port.""" + from frappe_assistant_core.api import fac_endpoint + + with ExitStack() as stack: + _patch_conf(stack, {"host_name": _HOST_WITH_PORT}) + stack.enter_context( + patch( + "frappe_assistant_core.api.oauth_discovery.get_server_url", + side_effect=AssertionError("must build metadata URL from host_name, not get_server_url"), + ) + ) + # A bearer token that does not exist → DoesNotExistError → 401. + stack.enter_context( + patch.object( + fac_endpoint.frappe, + "get_doc", + side_effect=frappe.DoesNotExistError, + ) + ) + frappe.local.request = self._make_request(auth_header="Bearer nonexistent-token") + + result = fac_endpoint._authenticate_mcp_request() + + self.assertIsInstance(result, Response) + self.assertEqual(result.status_code, 401) + www_auth = result.headers.get("WWW-Authenticate", "") + self.assertIn( + f'resource_metadata="{_HOST_WITH_PORT}/.well-known/oauth-protected-resource"', + www_auth, + ) diff --git a/frappe_assistant_core/tests/test_report_requirements_filters.py b/frappe_assistant_core/tests/test_report_requirements_filters.py new file mode 100644 index 0000000..13be450 --- /dev/null +++ b/frappe_assistant_core/tests/test_report_requirements_filters.py @@ -0,0 +1,161 @@ +# Frappe Assistant Core - AI Assistant integration for Frappe Framework +# Copyright (C) 2025 Paul Clinton +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +""" +Regression tests for: https://github.com/buildswithpaul/Frappe_Assistant_Core/issues/203 + +report_requirements returned empty filter definitions for custom Script Reports +whose filters are defined in the .js file, and did so silently. Reproduced two +real triggers and verified the fixes: + + * JSON-style quoted keys ("fieldname": "x") — the regex only matched bare + keys (fieldname:), so every filter object was skipped. + * filters built programmatically (filters: get_filters()) — there is no + literal array to parse; this now surfaces a diagnostic instead of a silent + empty result. + +Also adds the Report.filters child table as a discovery source, and a +discovery_diagnostics payload so empty results are debuggable. +""" + +from unittest.mock import MagicMock + +from frappe_assistant_core.plugins.core.tools.report_requirements import ReportRequirements +from frappe_assistant_core.tests.base_test import BaseAssistantTest + +_BARE_KEYS = """ +frappe.query_reports["X"] = { filters: [ + { fieldname: "company", label: __("Company"), fieldtype: "Link", options: "Company", reqd: 1 }, + { fieldname: "bom", label: __("BOM"), fieldtype: "Link", options: "BOM" } +] }; +""" + +_QUOTED_KEYS = """ +frappe.query_reports["X"] = { "filters": [ + { "fieldname": "company", "label": __("Company"), "fieldtype": "Link", "options": "Company", "reqd": 1 }, + { "fieldname": "bom", "label": __("BOM"), "fieldtype": "Link", "options": "BOM" } +] }; +""" + +_PROGRAMMATIC = """ +function gf() { return [ { fieldname: "company", fieldtype: "Link", options: "Company", reqd: 1 } ]; } +frappe.query_reports["X"] = { filters: gf() }; +""" + +_TEMPLATE_LITERAL = """ +frappe.query_reports["X"] = { filters: [ + { fieldname: "company", label: `Company`, fieldtype: "Link", options: "Company", reqd: 1 } +] }; +""" + + +class TestJsFilterParsing(BaseAssistantTest): + """The JS parser must tolerate the legal syntax variants that previously + produced a silent empty result.""" + + def setUp(self): + super().setUp() + self.tool = ReportRequirements() + + def test_bare_keys_parse(self): + parsed, note = self.tool._extract_filters_from_js(_BARE_KEYS) + self.assertIsNone(note) + self.assertEqual([f["fieldname"] for f in parsed["filters"]], ["company", "bom"]) + self.assertEqual(parsed["required_filters"], ["company"]) + + def test_quoted_keys_parse(self): + """Regression: JSON-style quoted keys used to yield 0 filters.""" + parsed, note = self.tool._extract_filters_from_js(_QUOTED_KEYS) + self.assertIsNone(note) + self.assertEqual([f["fieldname"] for f in parsed["filters"]], ["company", "bom"]) + self.assertEqual(parsed["required_filters"], ["company"]) + + def test_template_literal_label_parses_fieldname(self): + parsed, note = self.tool._extract_filters_from_js(_TEMPLATE_LITERAL) + self.assertIsNone(note) + self.assertEqual(parsed["filters"][0]["fieldname"], "company") + self.assertEqual(parsed["filters"][0].get("label"), "Company") + + def test_programmatic_filters_report_a_diagnostic_not_silence(self): + """Genuinely unparseable (filters built by a function) must surface a + note explaining why, rather than returning a bare None.""" + parsed, note = self.tool._extract_filters_from_js(_PROGRAMMATIC) + self.assertIsNone(parsed) + self.assertTrue(note) + self.assertIn("programmatically", note) + + def test_missing_filters_key_reports_note(self): + parsed, note = self.tool._extract_filters_from_js("frappe.query_reports['X'] = {};") + self.assertIsNone(parsed) + self.assertIn("no 'filters:' key", note) + + +class TestFiltersChildTableSource(BaseAssistantTest): + """Report.filters child-table rows are a structured discovery source.""" + + def setUp(self): + super().setUp() + self.tool = ReportRequirements() + + def test_child_table_rows_convert_to_filters(self): + rows = [ + {"fieldname": "company", "label": "Company", "fieldtype": "Link", "mandatory": 1}, + {"fieldname": "bom", "label": "BOM", "fieldtype": "Link"}, + ] + parsed = self.tool._parse_filters_child_table(rows) + self.assertEqual([f["fieldname"] for f in parsed["filters"]], ["company", "bom"]) + self.assertEqual(parsed["required_filters"], ["company"]) + self.assertEqual(parsed["optional_filters"], ["bom"]) + + def test_rows_without_fieldname_skipped(self): + rows = [{"label": "No fieldname"}, {"fieldname": "ok"}] + parsed = self.tool._parse_filters_child_table(rows) + self.assertEqual([f["fieldname"] for f in parsed["filters"]], ["ok"]) + + +class TestDiscoveryOrchestration(BaseAssistantTest): + """_discover_script_report_filters prefers the child table and always + returns a diagnostics payload.""" + + def setUp(self): + super().setUp() + self.tool = ReportRequirements() + + def test_child_table_wins_and_short_circuits_js(self): + report_doc = MagicMock() + report_doc.module = "Selling" + report_doc.get.return_value = [ + {"fieldname": "company", "label": "Company", "fieldtype": "Link", "mandatory": 1} + ] + + parsed, diagnostics = self.tool._discover_script_report_filters("Some Report", report_doc) + + self.assertEqual([f["fieldname"] for f in parsed["filters"]], ["company"]) + self.assertEqual(diagnostics["filters_child_table"]["status"], "success") + # JS path not attempted when the child table satisfied discovery. + self.assertNotIn("javascript", diagnostics) + + def test_empty_child_table_falls_through_to_js_with_diagnostics(self): + report_doc = MagicMock() + report_doc.module = "Nonexistent Module XYZ" + report_doc.get.return_value = [] # empty child table + + parsed, diagnostics = self.tool._discover_script_report_filters("No Such Report", report_doc) + + self.assertIsNone(parsed) + self.assertEqual(diagnostics["filters_child_table"]["status"], "empty") + # JS discovery was attempted and recorded (even though it found nothing). + self.assertIn("javascript", diagnostics) diff --git a/frappe_assistant_core/tests/test_search_tools.py b/frappe_assistant_core/tests/test_search_tools.py index 96ab925..105f144 100644 --- a/frappe_assistant_core/tests/test_search_tools.py +++ b/frappe_assistant_core/tests/test_search_tools.py @@ -19,6 +19,8 @@ """ import unittest +from contextlib import ExitStack +from unittest.mock import MagicMock, patch import frappe @@ -78,11 +80,82 @@ def test_search_documents_basic(self): def test_search_documents_with_filters(self): self.skipTest("Search with filters test placeholder") - def test_search_documents_permissions(self): - self.skipTest("Search permissions test placeholder") - - def test_search_specific_doctype(self): - self.skipTest("DocType search test placeholder") + def test_global_search_uses_permission_aware_query(self): + """Regression guard for #189: global_search (behind the search_documents + tool) must use frappe.get_list, not the permission-bypassing get_all.""" + from frappe_assistant_core.plugins.core.tools import search_tools + + with ExitStack() as stack: + # Make exactly one doctype exist and be readable so a single query + # runs. global_search calls frappe.db.exists("DocType", ), + # so the doctype name is the second positional arg. + stack.enter_context( + patch.object( + search_tools.frappe.db, + "exists", + side_effect=lambda *a, **k: "Employee" in a, + ) + ) + stack.enter_context( + patch.object( + search_tools.frappe, + "has_permission", + side_effect=lambda doctype, *a, **k: doctype == "Employee", + ) + ) + get_all = stack.enter_context( + patch.object( + search_tools.frappe, + "get_all", + side_effect=AssertionError("frappe.get_all bypasses DocType permissions"), + ) + ) + get_list = stack.enter_context(patch.object(search_tools.frappe, "get_list")) + get_list.return_value = [{"name": "EMP-0001"}] + + result = search_tools.SearchTools.global_search(query="EMP", limit=20) + + self.assertTrue(result.get("success"), result) + get_all.assert_not_called() + self.assertTrue(get_list.called, "global_search must query via frappe.get_list") + for call in get_list.call_args_list: + self.assertFalse( + call.kwargs.get("ignore_permissions", True), + "global_search must pass ignore_permissions=False", + ) + + def test_search_doctype_uses_permission_aware_query(self): + """Regression guard for #189: search_doctype (behind the search_doctype + tool) must use frappe.get_list, not the permission-bypassing get_all.""" + from frappe_assistant_core.plugins.core.tools import search_tools + + with ExitStack() as stack: + stack.enter_context(patch.object(search_tools.frappe.db, "exists", return_value=True)) + stack.enter_context(patch.object(search_tools.frappe, "has_permission", return_value=True)) + # Minimal meta stub: one searchable Data field, no title field. + meta = MagicMock() + meta.title_field = None + field = MagicMock(fieldtype="Data", hidden=False, fieldname="employee_name") + meta.fields = [field] + stack.enter_context(patch.object(search_tools.frappe, "get_meta", return_value=meta)) + get_all = stack.enter_context( + patch.object( + search_tools.frappe, + "get_all", + side_effect=AssertionError("frappe.get_all bypasses DocType permissions"), + ) + ) + get_list = stack.enter_context(patch.object(search_tools.frappe, "get_list")) + get_list.return_value = [{"name": "EMP-0001", "employee_name": "Allowed"}] + + result = search_tools.SearchTools.search_doctype(doctype="Employee", query="All", limit=20) + + self.assertTrue(result.get("success"), result) + get_all.assert_not_called() + self.assertEqual(get_list.call_count, 1) + call = get_list.call_args_list[0] + self.assertEqual(call.args[0], "Employee") + self.assertFalse(call.kwargs.get("ignore_permissions", True)) def test_search_empty_query(self): self.skipTest("Empty query test placeholder") diff --git a/frappe_assistant_core/tests/test_tool_annotations.py b/frappe_assistant_core/tests/test_tool_annotations.py new file mode 100644 index 0000000..3137c0f --- /dev/null +++ b/frappe_assistant_core/tests/test_tool_annotations.py @@ -0,0 +1,185 @@ +# Frappe Assistant Core - AI Assistant integration for Frappe Framework +# Copyright (C) 2025 Paul Clinton +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +""" +Tests for MCP tool annotation hints derived from FAC tool categories. + +MCP clients (e.g. Claude Desktop) group tools and pick default approval +behavior from the annotation hints in the tools/list response. FAC previously +emitted no annotations, so every tool landed in a single "Other tools" bucket. +The FAC tool category (FAC Tool Configuration.tool_category, admin-overridable) +is now translated into readOnlyHint / destructiveHint, so the client grouping +matches the admin page — one source of truth. +""" + +from collections import OrderedDict +from contextlib import ExitStack +from unittest.mock import MagicMock, patch + +import frappe +from werkzeug.wrappers import Request, Response + +from frappe_assistant_core.tests.base_test import BaseAssistantTest +from frappe_assistant_core.utils.tool_category_detector import category_to_annotations + + +class TestCategoryToAnnotations(BaseAssistantTest): + """category_to_annotations maps the 4 FAC categories to MCP hints.""" + + def test_read_only(self): + self.assertEqual(category_to_annotations("read_only"), {"readOnlyHint": True}) + + def test_write(self): + self.assertEqual(category_to_annotations("write"), {"readOnlyHint": False}) + + def test_read_write(self): + self.assertEqual(category_to_annotations("read_write"), {"readOnlyHint": False}) + + def test_privileged_is_destructive(self): + self.assertEqual( + category_to_annotations("privileged"), + {"readOnlyHint": False, "destructiveHint": True}, + ) + + def test_dangerous_legacy_alias(self): + self.assertEqual( + category_to_annotations("dangerous"), + {"readOnlyHint": False, "destructiveHint": True}, + ) + + def test_unknown_category_yields_no_hints(self): + # Unknown -> empty dict (degrade to "no hint", never a wrong hint). + self.assertEqual(category_to_annotations("something_else"), {}) + + +class TestResolveToolCategories(BaseAssistantTest): + """_resolve_tool_categories prefers the stored (override-able) category.""" + + def test_stored_category_wins_over_autodetect(self): + from frappe_assistant_core.api import fac_endpoint + + registry = MagicMock() + # Auto-detect would say read_only, but the stored config says privileged + # (e.g. an admin override). The stored value must win. + with ExitStack() as stack: + stack.enter_context( + patch.object( + fac_endpoint.frappe, + "get_all", + return_value=[{"tool_name": "get_document", "tool_category": "privileged"}], + ) + ) + detect = stack.enter_context( + patch( + "frappe_assistant_core.utils.tool_category_detector.detect_tool_category", + return_value="read_only", + ) + ) + + result = fac_endpoint._resolve_tool_categories(["get_document"], registry) + + self.assertEqual(result["get_document"], "privileged") + detect.assert_not_called() # no need to auto-detect when stored value exists + + def test_falls_back_to_autodetect_when_no_config_row(self): + from frappe_assistant_core.api import fac_endpoint + + registry = MagicMock() + registry.get_tool.return_value = MagicMock(name="tool_instance") + with ExitStack() as stack: + stack.enter_context(patch.object(fac_endpoint.frappe, "get_all", return_value=[])) + stack.enter_context( + patch( + "frappe_assistant_core.utils.tool_category_detector.detect_tool_category", + return_value="write", + ) + ) + + result = fac_endpoint._resolve_tool_categories(["create_document"], registry) + + self.assertEqual(result["create_document"], "write") + + def test_defaults_to_read_write_when_instance_missing(self): + from frappe_assistant_core.api import fac_endpoint + + registry = MagicMock() + registry.get_tool.return_value = None # tool instance unavailable + with ExitStack() as stack: + stack.enter_context(patch.object(fac_endpoint.frappe, "get_all", return_value=[])) + + result = fac_endpoint._resolve_tool_categories(["mystery_tool"], registry) + + self.assertEqual(result["mystery_tool"], "read_write") + + +class TestToolsListEmitsAnnotations(BaseAssistantTest): + """The MCP tools/list response must carry the annotation hints so the + client can categorize tools.""" + + def test_tools_list_includes_annotations(self): + from frappe_assistant_core.mcp.server import MCPServer + + server = MCPServer("test") + tool_registry = OrderedDict() + tool_registry["get_document"] = { + "name": "get_document", + "description": "Read a document", + "inputSchema": {"type": "object", "properties": {}}, + "annotations": {"readOnlyHint": True}, + "fn": lambda **kw: {}, + } + tool_registry["delete_document"] = { + "name": "delete_document", + "description": "Delete a document", + "inputSchema": {"type": "object", "properties": {}}, + "annotations": {"readOnlyHint": False, "destructiveHint": True}, + "fn": lambda **kw: {}, + } + + result = server._handle_tools_list({}, tool_registry) + + by_name = {t["name"]: t for t in result["tools"]} + self.assertEqual(by_name["get_document"]["annotations"], {"readOnlyHint": True}) + self.assertEqual( + by_name["delete_document"]["annotations"], + {"readOnlyHint": False, "destructiveHint": True}, + ) + + +class TestBuildToolRegistryAttachesAnnotations(BaseAssistantTest): + """End-to-end: tools built for a request carry category-derived annotations + instead of landing unclassified.""" + + def test_every_built_tool_has_annotations(self): + from frappe_assistant_core.api.fac_endpoint import _build_tool_registry + + registry = _build_tool_registry() + self.assertTrue(registry, "expected at least one available tool") + + unclassified = [name for name, td in registry.items() if not td.get("annotations")] + self.assertEqual( + unclassified, + [], + f"these tools reached the client with no annotation hints: {unclassified}", + ) + + # Spot-check known classifications. + if "get_document" in registry: + self.assertEqual(registry["get_document"]["annotations"].get("readOnlyHint"), True) + if "delete_document" in registry: + ann = registry["delete_document"]["annotations"] + self.assertEqual(ann.get("readOnlyHint"), False) + self.assertEqual(ann.get("destructiveHint"), True) diff --git a/frappe_assistant_core/utils/tool_category_detector.py b/frappe_assistant_core/utils/tool_category_detector.py index 464d4b7..09922f5 100644 --- a/frappe_assistant_core/utils/tool_category_detector.py +++ b/frappe_assistant_core/utils/tool_category_detector.py @@ -234,6 +234,46 @@ def detect_tool_category(tool_instance) -> str: return get_detector().detect_category(tool_instance) +def category_to_annotations(category: str) -> dict: + """ + Translate a FAC tool category into MCP tool annotation hints. + + MCP clients (e.g. Claude Desktop) group tools and choose default approval + behavior from these behavioral hints in the tools/list response. Without + them every tool lands in a single "Other tools" bucket. The FAC category + (stored on FAC Tool Configuration, admin-overridable) is the single source + of truth, so the admin page and the client stay in sync. + + Mapping (per MCP spec; destructiveHint is only meaningful when + readOnlyHint is false): + read_only -> {"readOnlyHint": True} (Read-only group) + write -> {"readOnlyHint": False} (Write/delete group) + read_write -> {"readOnlyHint": False} (Write/delete group) + privileged -> {"readOnlyHint": False, "destructiveHint": True} + + Args: + category: One of read_only, write, read_write, privileged + ("dangerous" is accepted as a legacy alias for privileged). + + Returns: + Dict of MCP annotation hints. Empty dict for an unknown category, so an + unrecognized value degrades to "no hints" rather than a wrong hint. + """ + if category == "dangerous": # legacy alias + category = "privileged" + + if category == "read_only": + return {"readOnlyHint": True} + if category == "write": + return {"readOnlyHint": False} + if category == "read_write": + return {"readOnlyHint": False} + if category == "privileged": + return {"readOnlyHint": False, "destructiveHint": True} + + return {} + + def get_category_info(category: str) -> dict: """ Get display information for a category. diff --git a/pyproject.toml b/pyproject.toml index f4734c4..0a9e266 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -55,7 +55,7 @@ dependencies = [ "chardet>=5.1.0,<6.0.0", # Compatible with frappe ~=5.1.0 requirement "pymupdf>=1.24.0", # PDF page rendering for Ollama vision OCR "python-magic>=0.4.27", - "beautifulsoup4~=4.12.2", # Compatible with frappe ~=4.12.2 requirement + "beautifulsoup4>=4.12,<5", # Span Frappe v15 (~=4.12.2) and v16 (~=4.13.5) without forcing a downgrade ] [project.optional-dependencies]