fix: mcp improvements #472
Conversation
WalkthroughThis pull request involves modifications to the Changes
Sequence DiagramsequenceDiagram
participant Server
participant AsyncTools
participant Cognee
Server->>AsyncTools: call_tools(name, arguments)
AsyncTools->>Cognee: cognify(text)
Cognee-->>AsyncTools: Process knowledge graph
AsyncTools-->>Server: Return results
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (19)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cognee-mcp/src/server.py (1)
3-3: Add missing import of asyncio at the top for clarity.Even though
asynciois imported later around line 225, referencing it within the file (e.g., at line 114) is more readable if we import it consistently near the other top-level imports. This also helps ensure maintainers quickly see the module dependencies of the file.-import os +import os +import asyncio import cognee import logging import importlib.utilcognee-mcp/src/client.py (2)
36-37: Unused toolResult after listing tools.
toolResult = await session.list_tools()captures available tools but doesn’t use the result. Either remove the assignment or log/display the result if needed for debugging or user feedback. This avoids confusion and clarifies intent.toolResult = await session.list_tools() - toolResult + # For future usage or logging, e.g.: + print("Available tools:", toolResult)
38-38: Check for potential return value usage.The result of
call_tool("prune", arguments={})is also assigned totoolResultbut never referenced. If the prune operation returns relevant information, consider logging or storing it. Otherwise, removing the assignment avoids confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
cognee-mcp/uv.lockis excluded by!**/*.lockpoetry.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
cognee-mcp/pyproject.toml(1 hunks)cognee-mcp/src/__init__.py(1 hunks)cognee-mcp/src/client.py(2 hunks)cognee-mcp/src/server.py(4 hunks)cognee/infrastructure/engine/models/DataPoint.py(0 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (1)
- cognee/infrastructure/engine/models/DataPoint.py
🔇 Additional comments (7)
cognee-mcp/src/server.py (3)
70-102: Ensure consistent error handling.Within the
call_toolsfunction, the exception is logged and converted into a text response. While this is good for error visibility, consider whether returning a generic error message to the caller is sufficient or if more detailed diagnostic information (e.g., error type, stack trace) is needed for debugging. If exposing that detail is safe, you may provide it here or separately log it at a more detailed severity level.
135-151: Validate server capabilities at initialization.The
mainfunction configures server capabilities with a customNotificationOptions()and an empty dictionary forexperimental_capabilities. Verify that these capabilities reflect the intended configuration for your deployment. If some advanced settings are necessary, you might pass them here.
225-226: Good usage of asyncio.run.Switching to
asyncio.run(main())is aligned with modern asynchronous patterns. This change simplifies the runtime flow and follows the recommended approach for launching async functions in Python 3.7+.cognee-mcp/src/__init__.py (1)
1-7: Confirmed shift to asynchronous startup.Replacing a direct synchronous method call with
asyncio.run(server_main())aligns well with the new asynchronous server architecture. The code is succinct and meets common best practices for an entry point into an async program.cognee-mcp/src/client.py (1)
7-8: Reassess the need for "uv" command and new arguments.Previously, the
commandwas"mcp"with different arguments. Now it’s"uv"with["--directory", ".", "run", "cognee"]. Verify that these changes reflect the correct execution flow for your environment. If"uv"is a placeholder or an internal command, ensure it’s reliably available in all deployment environments.cognee-mcp/pyproject.toml (1)
10-10: Verify compatibility with downgraded mcp version.The downgrade from mcp 1.2.0 to 1.1.3 could have significant implications:
- Features or bug fixes from 1.2.0 might be lost
- This change appears related to architectural updates mentioned in the PR summary
Let's verify the changelog and any breaking changes:
Could you please:
- Document the reason for downgrading the mcp version
- Confirm that all required features are available in 1.1.3
- Verify that no critical bug fixes from 1.2.0 are needed
pyproject.toml (1)
25-25: Verify FastAPI version constraints and security implications.The change from a version range
>=0.109.2,<0.116.0to a specific version0.115.7:
- Reduces flexibility for future updates
- May affect compatibility with other dependencies
Let's verify the version compatibility and security:
Could you please:
- Document the reason for pinning to this specific version
- Consider using a version range to allow for security updates
- Verify compatibility with other dependencies in the project
✅ Verification successful
FastAPI version 0.115.7 is secure and compatible with dependencies
While pinning to a specific version helps with reproducibility, consider using
>=0.115.7,<0.116.0to allow for future security patches while maintaining API stability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check FastAPI versions and security advisories echo "Checking FastAPI versions..." curl -s https://pypi.org/pypi/fastapi/json | jq -r '.releases | keys[]' | sort -V | tail -n 5 echo "Checking for security advisories..." gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: PIP, package: "fastapi") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' echo "Checking dependency compatibility..." pip install fastapi==0.115.7 --dry-runLength of output: 2773
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
cognee-mcp/src/server.py (1)
Line range hint
100-114: Avoid nested task creation without proper handling.The function creates a nested task for
cognee.cognifywithout awaiting or storing it, which could lead to:
- Lost error information
- Race conditions
- Uncontrolled concurrency
async def cognify(text: str, graph_model_file: str = None, graph_model_name: str = None) -> str: if graph_model_file and graph_model_name: graph_model = load_class(graph_model_file, graph_model_name) else: graph_model = KnowledgeGraph await cognee.add(text) try: - asyncio.create_task(cognee.cognify(graph_model=graph_model)) + await cognee.cognify(graph_model=graph_model) except Exception as e: raise ValueError(f"Failed to cognify: {str(e)}")🧰 Tools
🪛 GitHub Actions: ruff format
[warning] File requires formatting using Ruff formatter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
cognee-mcp/uv.lockis excluded by!**/*.lockpoetry.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
cognee-mcp/src/server.py(8 hunks)
🧰 Additional context used
🪛 GitHub Actions: ruff format
cognee-mcp/src/server.py
[warning] File requires formatting using Ruff formatter
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: run_notebook_test / test
- GitHub Check: run_dynamic_steps_example_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_simple_example_test / test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: windows-latest
- GitHub Check: test
- GitHub Check: docker-compose-test
- GitHub Check: profiler
🔇 Additional comments (5)
cognee-mcp/src/server.py (5)
Line range hint
74-97: Improve logging and task management.
- Consider logging suppressed stdout/stderr instead of discarding them, as they might contain important debugging information.
- The cognify task is created but not awaited or stored, which could lead to unhandled errors or race conditions.
@mcp.call_tool() async def call_tools(name: str, arguments: dict) -> list[types.TextContent]: try: - with open(os.devnull, "w") as fnull: - with redirect_stdout(fnull), redirect_stderr(fnull): - if name == "cognify": - asyncio.create_task(cognify( - text=arguments["text"], - graph_model_file=arguments.get("graph_model_file", None), - graph_model_name=arguments.get("graph_model_name", None), - )) + if name == "cognify": + task = asyncio.create_task(cognify( + text=arguments["text"], + graph_model_file=arguments.get("graph_model_file", None), + graph_model_name=arguments.get("graph_model_name", None), + )) + try: + await task + except Exception as e: + logger.error(f"Cognify task failed: {e}") + return [types.TextContent(type="text", text=f"Error in cognify task: {str(e)}")]🧰 Tools
🪛 GitHub Actions: ruff format
[warning] File requires formatting using Ruff formatter
Line range hint
116-132: LGTM! Clean and well-structured implementation.The search and prune functions are well-implemented with proper async/await usage and clear error propagation.
🧰 Tools
🪛 GitHub Actions: ruff format
[warning] File requires formatting using Ruff formatter
Line range hint
173-198: LGTM! Well-implemented utility functions.The utility functions are well-structured with proper error handling and clear implementation.
🧰 Tools
🪛 GitHub Actions: ruff format
[warning] File requires formatting using Ruff formatter
Line range hint
224-227: LGTM! Proper async initialization.The main block correctly initializes and runs the async server.
🧰 Tools
🪛 GitHub Actions: ruff format
[warning] File requires formatting using Ruff formatter
Line range hint
23-70: Tool definitions look good but require formatting.The tool definitions are well-structured with clear schemas and descriptions. However, the file needs formatting according to Ruff.
Run the following to check and apply the required formatting:
✅ Verification successful
Formatting issues confirmed - needs cleanup
The file requires the following formatting fixes:
- Remove extra blank lines after Tool definitions
- Fix indentation in the
cognifytask creation block- Clean up unnecessary blank lines between functions
- Standardize spacing around function definitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check and apply Ruff formatting ruff format cognee-mcp/src/server.py --diffLength of output: 3616
🧰 Tools
🪛 GitHub Actions: ruff format
[warning] File requires formatting using Ruff formatter
cognee-mcp/src/server.py
Outdated
| except Exception as e: | ||
| logger.error(f"Server failed to start: {str(e)}", exc_info=True) | ||
| raise |
There was a problem hiding this comment.
Remove duplicate error handling block.
These lines duplicate the error handling that's already present in the main function above.
- except Exception as e:
- logger.error(f"Server failed to start: {str(e)}", exc_info=True)
- raise🧰 Tools
🪛 GitHub Actions: ruff format
[warning] File requires formatting using Ruff formatter
Description
DCO Affirmation
I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of Origin
Summary by CodeRabbit
Dependency Update
mcppackage version from 1.2.0 to 1.1.3Server Improvements