-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove MCP crates #48
Conversation
I'm getting to work. See my steps and track my cost here.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Review: Remove MCP crates
I've reviewed the changes to remove MCP (Model Context Protocol) related dependencies from the project. Here are my observations:
Changes Overview
- Removed optional HTTP server dependencies: axum, tower, tower-http
- Removed other dependencies likely used for the MCP server: async-trait, thiserror, futures
- Removed the
http
feature that referenced these dependencies - Reorganized some remaining dependencies
Questions and Considerations
- Is this removal temporary (as suggested by the branch name "remove-mcp-for-now") or permanent?
- Will users who relied on the MCP server functionality (
ummon serve
command) be affected? - Have other related code changes been made to:
- Remove the
serve
command from the CLI - Update any code that referenced the removed dependencies
- Update documentation that mentions MCP functionality
- Remove the
Documentation Impact
The README currently contains significant sections about MCP server functionality, including:
- Description of MCP as a core feature
- Instructions for using
ummon serve
- Example MCP usage with MCP Inspector
- Testing the MCP server
If this is a permanent removal, these sections would need to be updated or removed to avoid confusion for users.
Suggestion
Consider adding a note to the PR description explaining the rationale for removing the MCP crates and whether this is temporary or permanent, which would help reviewers understand the context of this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Observations
After examining the CLI code (src/cli.rs
) and commands module (src/commands/mod.rs
), I've noticed some inconsistencies between the codebase and documentation:
- The CLI doesn't currently include a
serve
command for the MCP server functionality that's described in the README. - There's no module for MCP server implementation in the commands directory (no
serve.rs
).
This suggests one of two possibilities:
- The MCP server command was previously removed from the code but remained in the dependencies and documentation
- The MCP server functionality was planned and documented but not fully implemented in the CLI
Either way, this PR appears to be removing the dependencies for a feature that isn't visible in the current CLI implementation. If this is intentional, it might be worth considering a follow-up PR to update the README to remove or modify the MCP server sections to avoid confusion for users.
Given the branch name "remove-mcp-for-now", it seems this might be a temporary removal. If MCP functionality is planned to return in the future, adding a note about this to the README would be helpful for users and contributors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final Review Summary
After further inspection, I've found:
-
The CI build passed successfully ✅, suggesting the removed dependencies aren't used in the actual implementation.
-
There's a reference to "MCP Integration" documentation in
docs/query_system.md
that links to a non-existent file:docs/mcp_integration.md
. -
The README extensively documents MCP functionality and the
ummon serve
command, but:- There's no
serve
command in the CLI implementation - No MCP server module exists in the commands directory
- There's no
This suggests the MCP functionality was documented and dependencies were added in preparation, but the implementation might not have been fully completed before this PR to remove the MCP-related dependencies.
Recommendations:
-
Update Documentation: The README should be updated to either:
- Remove references to MCP functionality and the
serve
command - Add a note that MCP functionality is planned for a future release (if that's the case)
- Remove references to MCP functionality and the
-
Fix Documentation References: Update or remove the reference to
mcp_integration.md
in the query system documentation. -
Add PR Context: Consider adding a brief explanation to the PR description about why MCP is being removed "for now" and any plans for its future.
The code changes themselves look good - removing unused dependencies will simplify the project and reduce build times. The main issue is ensuring documentation accurately reflects the codebase's actual capabilities.
@MentatBot can you add the changes in your Recommendations? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes Made
I've updated the documentation to remove references to the MCP (Model Context Protocol) functionality as requested:
In README.md:
- Removed the "AI Agent Integration with MCP" from the Core Features section
- Removed the
ummon serve
command example from the Usage section - Removed the entire MCP Server section and its subsections (Available Tools, Example MCP Usage)
- Removed "MCP server for AI agent integration" from the Architecture description
- Removed the "Testing the MCP Server" section
- Updated the description of the JavaScript test files, removing specific references to MCP clients
In docs/query_system.md:
- Removed the reference to the non-existent "MCP Integration" documentation file
These changes align the documentation with the code changes in this PR, which removes the MCP-related dependencies. The documentation now accurately reflects the current functionality without referring to features that aren't currently implemented.
No description provided.