Conversation
- Added LLMInteractionLogger for logging interactions with the LLM, configurable via app settings. - Introduced max_steps and logging options in app settings. - Updated AIQueryHandler to support multi-step query processing and logging of translation and response generation. - Enhanced prompts to include request context for better query resolution. - Improved error handling and logging throughout the AI query service. - Added request context retrieval in SchemaAnalyzer for better context awareness in queries.
…andling - Added `retry_on_empty` setting to app configuration to enable retrying queries that return no results. - Introduced `translate_retry_on_empty` method in `AIQueryHandler` to generate alternative queries or confirm no results. - Enhanced `LLMInteractionLogger` to log retry attempts and responses. - Updated `build_retry_on_empty_prompt` method in `PromptBuilder` to create prompts for suggesting alternative queries. - Modified `AIQueryService` to utilize the new retry mechanism when no results are found.
rboixaderg
requested review from
bloodbare,
masipcat and
vangheem
as code owners
There was a problem hiding this comment.
Pull request overview
This pull request adds MCP (Model Context Protocol) support to Guillotina as a new contrib module. MCP enables AI assistants and LLMs to interact with Guillotina content through a standardized protocol. The implementation provides both in-process and HTTP-based backends for querying and retrieving content.
Changes:
- Added a new
guillotina.contrib.mcpmodule with services, backends, tools, and command support for MCP protocol integration - Implemented in-process and HTTP backends for search, count, get_content, and list_children operations
- Added authentication and token management services for MCP clients
- Included basic test coverage for the MCP functionality
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| guillotina/tests/test_mcp.py | Adds basic tests for MCP service registration, backend context requirements, authentication, and token generation |
| guillotina/contrib/mcp/tools.py | Defines MCP tool functions (search, count, get_content, list_children) and registers them with the MCP server |
| guillotina/contrib/mcp/services.py | Implements the @mcp endpoint service and @mcp-token service for JWT token generation |
| guillotina/contrib/mcp/server.py | Provides MCP server initialization and ASGI app creation with global state management |
| guillotina/contrib/mcp/permissions.py | Defines two new permissions for MCP usage and token issuance, granted to authenticated users |
| guillotina/contrib/mcp/interfaces.py | Defines IMCPDescriptionExtras interface for extending tool descriptions |
| guillotina/contrib/mcp/command.py | Implements CLI command for running standalone MCP server with HTTP backend |
| guillotina/contrib/mcp/backend.py | Implements InProcessBackend and HttpBackend classes for content operations with context management |
| guillotina/contrib/mcp/init.py | Module initialization with default settings and configuration scanning |
| Makefile | Adds format and tests targets for code formatting and test execution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Added new MCP documentation to describe the Model Context Protocol integration. - Implemented the `@mcp` and `@chat` endpoints for LLM interactions. - Refactored tool definitions to support additional chat functionalities. - Removed deprecated chat tools and updated related service logic for better performance.
…txt for compatibility
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_mcp_app_and_server(): | ||
| global _mcp_server, _mcp_app | ||
| if _mcp_app is not None: | ||
| return _mcp_app, _mcp_server | ||
| from mcp.server.fastmcp import FastMCP | ||
|
|
||
| backend = InProcessBackend() | ||
| mcp = FastMCP( | ||
| "Guillotina MCP", | ||
| json_response=True, | ||
| stateless_http=True, | ||
| ) | ||
| if hasattr(mcp, "settings") and hasattr(mcp.settings, "streamable_http_path"): | ||
| mcp.settings.streamable_http_path = "/" | ||
| register_tools(mcp, backend) | ||
| extra_module = app_settings.get("mcp", {}).get("extra_tools_module") | ||
| if extra_module: | ||
| mod = __import__(str(extra_module), fromlist=["register_extra_tools"]) | ||
| getattr(mod, "register_extra_tools")(mcp, backend) | ||
| _mcp_server = mcp | ||
| _mcp_app = mcp.streamable_http_app() | ||
| return _mcp_app, _mcp_server |
There was a problem hiding this comment.
The InProcessBackend is created once when the MCP server is first initialized and then reused for all subsequent requests. However, the backend relies on the MCP context which changes per request via a ContextVar. While ContextVars are thread-safe and async-safe, this design should be verified to work correctly when multiple requests are handled concurrently. Consider adding a comment explaining this design decision or restructuring to create the backend per-request if needed.
| extra_module = app_settings.get("mcp", {}).get("extra_tools_module") | ||
| if extra_module: | ||
| mod = __import__(str(extra_module), fromlist=["register_extra_tools"]) | ||
| getattr(mod, "register_extra_tools")(mcp, backend) |
There was a problem hiding this comment.
When extra_tools_module is configured, the code assumes the register_extra_tools function exists but doesn't handle AttributeError if it's missing. This will cause the server initialization to fail with an unclear error. Add error handling to validate that the function exists and provide a helpful error message if it's missing.
| getattr(mod, "register_extra_tools")(mcp, backend) | |
| extra_tools_registrar = getattr(mod, "register_extra_tools", None) | |
| if not callable(extra_tools_registrar): | |
| raise RuntimeError( | |
| f"Configured extra_tools_module '{extra_module}' does not define a callable " | |
| "'register_extra_tools(mcp, backend)' function." | |
| ) | |
| extra_tools_registrar(mcp, backend) |
| original_task_group = session_manager._task_group # Workaround: mcp lib does not expose task group. | ||
| async with anyio.create_task_group() as tg: | ||
| session_manager._task_group = tg | ||
| try: | ||
| await app(scope, request.receive, request.send) | ||
| finally: | ||
| session_manager._task_group = original_task_group |
There was a problem hiding this comment.
The code accesses a private attribute _task_group on the session_manager. While there's a comment explaining this is a workaround, accessing private attributes is fragile and could break if the mcp library changes its internal implementation. Consider documenting which version of the mcp library this workaround is for and adding error handling in case the attribute doesn't exist in future versions.
| original_task_group = session_manager._task_group # Workaround: mcp lib does not expose task group. | |
| async with anyio.create_task_group() as tg: | |
| session_manager._task_group = tg | |
| try: | |
| await app(scope, request.receive, request.send) | |
| finally: | |
| session_manager._task_group = original_task_group | |
| # Workaround: current mcp library version does not expose the session task group via | |
| # a public API, so we temporarily override the private `_task_group` attribute. | |
| # This is intentionally defensive: if a future mcp version removes or renames | |
| # `_task_group`, we fall back to calling the app without overriding it. | |
| original_task_group = getattr(session_manager, "_task_group", None) | |
| if original_task_group is not None: | |
| async with anyio.create_task_group() as tg: | |
| session_manager._task_group = tg | |
| try: | |
| await app(scope, request.receive, request.send) | |
| finally: | |
| # Restore original task group to avoid leaving session_manager mutated. | |
| session_manager._task_group = original_task_group | |
| else: | |
| logger.warning( | |
| "MCP session_manager has no '_task_group' attribute; " | |
| "running MCP app without overriding task group. " | |
| "This may indicate an incompatible mcp library version." | |
| ) | |
| await app(scope, request.receive, request.send) |
| for tc in tool_calls: | ||
| tc_id = getattr(tc, "id", None) or (tc.get("id") if isinstance(tc, dict) else "") | ||
| fn = getattr(tc, "function", None) or (tc.get("function") if isinstance(tc, dict) else {}) | ||
| name = getattr(fn, "name", None) if fn else None | ||
| if name is None and isinstance(fn, dict): | ||
| name = fn.get("name", "") | ||
| raw_args = getattr(fn, "arguments", None) if fn else None | ||
| if raw_args is None and isinstance(fn, dict): | ||
| raw_args = fn.get("arguments", "{}") | ||
| tool_calls_list.append( | ||
| { | ||
| "id": tc_id, | ||
| "type": "function", | ||
| "function": {"name": name or "", "arguments": raw_args or "{}"}, | ||
| } | ||
| ) | ||
| assistant_msg["tool_calls"] = tool_calls_list | ||
| messages.append(assistant_msg) | ||
| for tc in tool_calls: | ||
| tc_id = getattr(tc, "id", None) or (tc.get("id") if isinstance(tc, dict) else "") | ||
| fn = getattr(tc, "function", None) or (tc.get("function") if isinstance(tc, dict) else {}) | ||
| name = getattr(fn, "name", None) if fn else None | ||
| if name is None and isinstance(fn, dict): | ||
| name = fn.get("name", "") | ||
| raw_args = getattr(fn, "arguments", None) if fn else None | ||
| if raw_args is None and isinstance(fn, dict): | ||
| raw_args = fn.get("arguments", "{}") | ||
| name = name or "" | ||
| raw_args = raw_args or "{}" | ||
| try: | ||
| arguments = json.loads(raw_args) if isinstance(raw_args, str) else (raw_args or {}) | ||
| except json.JSONDecodeError: | ||
| arguments = {} | ||
| try: | ||
| result = await _execute_tool(backend, name, arguments) | ||
| except Exception as e: | ||
| logger.exception("MCP chat tool %s failed", name) | ||
| result = {"error": str(e)} | ||
| messages.append({"role": "tool", "tool_call_id": tc_id, "content": json.dumps(result)}) |
There was a problem hiding this comment.
There is significant code duplication in parsing tool calls. Lines 141-156 and 159-169 contain nearly identical logic for extracting tc_id, function name, and arguments. This makes the code harder to maintain. Consider extracting this logic into a helper function to reduce duplication and improve maintainability.
| litellm = __import__("litellm", fromlist=["acompletion"]) | ||
| acompletion = getattr(litellm, "acompletion") |
There was a problem hiding this comment.
The chat service dynamically imports litellm without verifying it's installed first. While pytest.importorskip is used in tests, the actual service code should handle ImportError gracefully and return a clear error message if litellm is not available. This would provide better user experience when litellm is not installed but chat is enabled.
| litellm = __import__("litellm", fromlist=["acompletion"]) | |
| acompletion = getattr(litellm, "acompletion") | |
| try: | |
| litellm = __import__("litellm") | |
| except ImportError: | |
| raise HTTPPreconditionFailed( | |
| content={ | |
| "reason": "litellm is not installed", | |
| "hint": "Install the 'litellm' package to use the @chat endpoint.", | |
| } | |
| ) | |
| acompletion = getattr(litellm, "acompletion", None) | |
| if acompletion is None or not callable(acompletion): | |
| raise HTTPPreconditionFailed( | |
| content={ | |
| "reason": "litellm.acompletion is unavailable", | |
| "hint": "Ensure a compatible version of 'litellm' providing 'acompletion' is installed.", | |
| } | |
| ) |
guillotina/contrib/mcp/tools.py
Outdated
| def _context_for_path(container_path: typing.Optional[str]): | ||
| ctx = get_mcp_context() | ||
| if ctx is None: | ||
| return None | ||
| if container_path: | ||
| from guillotina.utils import navigate_to | ||
|
|
||
| try: | ||
| return navigate_to(ctx, "/" + container_path.strip("/")) | ||
| except KeyError: | ||
| return None | ||
| return ctx |
There was a problem hiding this comment.
The navigate_to function is async but is being called without await. This will return a coroutine object instead of the actual result, causing the function to return None or raise an error when the coroutine is used. This function should be made async and the navigate_to call should be awaited.
…ved compatibility and performance
1 participant
No description provided.