Skip to content

Conversation

@cristipufu
Copy link
Member

No description provided.

@cristipufu cristipufu self-assigned this May 4, 2025
@cristipufu cristipufu force-pushed the feat/slack_agent_smaple branch from d0bd33f to eda671f Compare May 4, 2025 13:54
@cristipufu
Copy link
Member Author

/help review

Copy link
Member Author

@cristipufu cristipufu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/ai generated [2849233972]

Code Review for PR #89: "samples: dynamic mcp with input json schema"

Changed Files

  • samples/mcp-dynamic-server/pyproject.toml
  • samples/mcp-dynamic-server/server.py

1. pyproject.toml

  • Change: Version bump from 0.0.7 to 0.0.16.
  • Assessment: This is a standard version update, likely to reflect the significant changes in the server's capabilities.

2. server.py

Major Refactor and Improvements

  • Built-in Tools Refactored:
    • Now uses a list of MCPTool objects instead of a dict of dicts.
    • Each tool has a proper JSON schema (inputSchema) for its parameters, improving documentation and validation.
  • Tool Registry:
    • The registry now stores inputSchema instead of a generic param_descriptions.
    • All metadata is more structured and consistent with JSON schema standards.
  • get_tools:
    • Now returns a list of MCPTool objects, including both built-in and dynamic tools.
    • Uses the new schema-based metadata for all tools.
  • add_tool:
    • Accepts an inputSchema parameter (JSON schema) for new tools, instead of a loose param description dict.
    • Usage examples and error messages updated to reflect the new schema-based approach.
    • Registers tools with their schema for better introspection and validation.
  • call_tool:
    • Now expects arguments to conform to the tool's JSON schema.
    • Improved error handling: returns the expected schema and a usage example on argument mismatch.
    • Prevents calling built-in tools via call_tool, with clear error messages and schema info.
    • Usage examples are dynamically generated based on the schema.

Good Practices

  • Type Hints: Consistent use of type hints throughout, improving readability and maintainability.
  • Error Handling: Much more robust, with clear, actionable error messages and usage examples.
  • Schema-Driven Design: Moving to JSON schema for tool parameters is a significant improvement for interoperability, validation, and documentation.
  • Code Organization: The separation of built-in and dynamic tools is now clearer and more maintainable.
  • Backward Compatibility: The changes are mostly additive and improve the API, but some breaking changes are possible for clients expecting the old parameter format.

Potential Issues / Suggestions

  1. Backward Compatibility:
    • The change from param_descriptions to inputSchema is breaking for any clients/scripts expecting the old format. Consider documenting this in the changelog or providing a migration note.
  2. Input Schema Validation:
    • While the schema is now accepted and returned, there is no explicit validation of input arguments against the schema before calling the tool. Consider using a library like jsonschema to validate args before invoking the tool, to catch errors earlier and provide more precise feedback.
  3. Security:
    • The server still allows arbitrary Python code execution for dynamic tools. This is a known risk, but should be clearly documented and, if possible, mitigated (e.g., via sandboxing, or at least a big warning in the README).
  4. Docstrings and Comments:
    • Some docstrings are slightly out of sync with the new parameter names (e.g., references to param_descriptions instead of inputSchema). Review and update all docstrings for accuracy.
  5. Async Consistency:
    • get_tools is now async, but the rest of the code is sync. Ensure the server framework supports this mix, or consider making all handlers async for consistency.
  6. Type Checking:
    • The code assumes that inputSchema is always a dict. Consider adding type checks or validation to avoid runtime errors if a user provides an invalid schema.

Minor Suggestions

  • Logging: Consider adding logging for tool registration and invocation for easier debugging and auditing.
  • Tests: Ensure that there are tests covering the new schema-driven tool registration and invocation, including error cases.

Summary

This PR is a significant improvement to the dynamic MCP server, moving to a schema-driven approach for tool registration and invocation. The code is cleaner, more robust, and better documented. There are some breaking changes and a few areas for further improvement, especially around input validation and security, but overall this is a strong step forward.

Recommendation: Approve after addressing docstring accuracy and (optionally) adding input validation against the provided schema. Consider documenting the breaking changes and security implications.

@cristipufu cristipufu merged commit 80c1764 into main May 4, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant