Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update refactors the CLI for cratedb-mcp to use the Click library, introducing a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (Click)
participant FastMCP
User->>CLI (Click): cratedb-mcp serve --transport=... --port=...
CLI (Click)->>CLI (Click): Parse options (transport, port)
CLI (Click)->>FastMCP: Run server with selected transport and port
FastMCP-->>CLI (Click): Server starts
CLI (Click)-->>User: Outputs logs and status
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
CHANGES.md (1)
6-7: Minor: Remove redundancy in "CLI interface""CLI" already stands for "Command Line Interface," so "CLI interface" is redundant. Consider rewording to simply "CLI" or "Command Line Interface."
- CLI: Added solid CLI interface using Click, `cratedb-mcp serve`, with + CLI: Added solid CLI using Click, `cratedb-mcp serve`, with🧰 Tools
🪛 LanguageTool
[style] ~6-~6: This phrase is redundant (‘I’ stands for ‘interface’). Use simply “CLI”.
Context: ...n naming conventions - CLI: Added solid CLI interface using Click,cratedb-mcp serve, with ...(ACRONYM_TAUTOLOGY)
cratedb_mcp/cli.py (1)
24-25: Type handling for transport choices could be improved.While using
Literalfor typing is good, accessing__args__with a type ignore comment is a bit hacky.Consider defining the choices directly and using them to build the Literal type:
-transport_types = t.Literal["stdio", "sse", "streamable-http"] -transport_choices = transport_types.__args__ # type: ignore +transport_choices = ("stdio", "sse", "streamable-http") +transport_types = t.Literal[transport_choices[0], transport_choices[1], transport_choices[2]]Or even more maintainable:
transport_choices = ("stdio", "sse", "streamable-http") TransportType = t.Literal["stdio", "sse", "streamable-http"]tests/test_cli.py (1)
1-63: Consider adding more test cases for complete coverage.While the current tests cover the basic functionality, consider adding tests for:
- Using the
--portoption with different values- Explicitly setting the
--transportoption (rather than via environment variable)- Testing all valid transport types including the new "streamable-http" option
This would provide more comprehensive test coverage for the CLI interface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGES.md(1 hunks)README.md(3 hunks)cratedb_mcp/__main__.py(2 hunks)cratedb_mcp/cli.py(1 hunks)pyproject.toml(2 hunks)tests/test_cli.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
CHANGES.md (1)
Learnt from: amotl
PR: crate/cratedb-mcp#17
File: CHANGES.md:5-5
Timestamp: 2025-05-16T13:35:04.487Z
Learning: For the cratedb-mcp repository, version numbers and release dates in CHANGES.md are updated by a dedicated "Release vX.Y.Z" commit as part of the release process, not during documentation preparation PRs.
🧬 Code Graph Analysis (1)
tests/test_cli.py (1)
cratedb_mcp/cli.py (1)
cli(15-21)
🪛 LanguageTool
CHANGES.md
[style] ~6-~6: This phrase is redundant (‘I’ stands for ‘interface’). Use simply “CLI”.
Context: ...n naming conventions - CLI: Added solid CLI interface using Click, cratedb-mcp serve, with ...
(ACRONYM_TAUTOLOGY)
🔇 Additional comments (15)
cratedb_mcp/__main__.py (1)
4-4: LGTM: Good refactoring to use a centralized application nameImporting
__appname__from the module rather than hardcoding the application name is a good practice that follows the DRY principle.Also applies to: 13-13
pyproject.toml (2)
75-75: LGTM: Dependencies align with new CLI implementationThe addition of Click (with version constraint) and Pueblo dependencies properly supports the new CLI implementation.
Also applies to: 79-79
95-95: LGTM: Entry point correctly updated for new CLI structureThe CLI entry point has been correctly updated to use the new Click-based command group
cliinstead of the previousmainfunction.README.md (4)
115-115: LGTM: Updated usage examples for the new CLI structureThe command examples have been properly updated to use the new
servesubcommand with the--transportoption. The additional example for the newstreamable-httptransport type is helpful for users.Also applies to: 119-119, 121-124
125-126: LGTM: Documentation on alternative environment variable usageGood addition of information about the alternative approach using the environment variable instead of the command-line option.
137-137: LGTM: Updated Claude configuration exampleThe Anthropic Claude configuration example has been correctly updated to include the new
servesubcommand.
160-160: LGTM: Updated mcptools examplesAll mcptools command examples have been correctly updated to include the new
servesubcommand.Also applies to: 163-163, 166-166, 171-171, 176-176
cratedb_mcp/cli.py (4)
4-6: Well-chosen dependency imports for the CLI refactoring.The addition of
clickfor CLI functionality andpueblo.util.cli.boot_clickfor initialization demonstrates a good approach to structuring the command-line interface.
12-22: Good implementation of the main CLI command group.The implementation follows Click's best practices with:
- Clear group structure with
@click.group()- Version option support with
@click.version_option()- Well-formatted docstring with usage information
- Proper initialization with
boot_clickThis creates a solid foundation for extending with subcommands.
28-42: Well-structured command options with appropriate defaults and environment variable support.The implementation properly:
- Defines transport and port options with clear help text
- Supports configuration via environment variables
- Provides sensible defaults
- Includes help text that explains what each option does
This follows the principle "Explicit is better than implicit" as mentioned in the PR objectives.
43-50: Good implementation of the serve command.The command correctly:
- Logs the transport choice for visibility
- Sets the MCP server port
- Executes the server with the specified transport
The type casting ensures type safety while allowing Click to handle string inputs from the command line.
tests/test_cli.py (4)
1-5: Good transition to Click testing utilities.Replacing manual
sys.argvmanipulation with theCliRunnerand updating imports to use the new CLI entry point is the correct approach when testing Click-based command-line interfaces.
8-23: Well-structured version command test.The test correctly:
- Uses
CliRunnerto invoke the CLI with the version option- Verifies the exit code is successful
- Checks that the version number is present in the output
This is a good pattern for testing CLI commands.
26-43: Good test for the default serve command.The test properly:
- Mocks
FastMCP.runto prevent the server from actually running- Uses
CliRunnerto invoke the CLI with the serve command- Verifies that the run method was called
This ensures that the CLI correctly passes control to the actual server implementation.
46-62: Proper test for transport validation.The test effectively:
- Sets an invalid transport value through environment variables
- Verifies that the CLI exits with an error code
- Checks that an appropriate error message is displayed
This validates that the CLI correctly enforces the allowed transport types.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cratedb_mcp/cli.py (1)
24-25: Type definition looks correct but could be improvedThe type definition for transport types is good, but accessing
__args__directly with a type ignore comment isn't ideal.Consider using
get_argsfrom typing_extensions for more explicit type handling:-transport_types = t.Literal["stdio", "sse", "streamable-http"] -transport_choices = transport_types.__args__ # type: ignore +from typing_extensions import get_args + +transport_types = t.Literal["stdio", "sse", "streamable-http"] +transport_choices = get_args(transport_types)This avoids the type ignore comment and will be more future-proof.
tests/test_cli.py (1)
1-145: Consider adding tests for edge casesWhile the test coverage is good, consider adding tests for additional edge cases:
- Test with a valid but unusual port number (e.g., 65535)
- Test command help output (
cratedb-mcp serve --help)- Test with no arguments (
cratedb-mcpwithout subcommands)These additional tests would enhance robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGES.md(1 hunks)README.md(3 hunks)cratedb_mcp/__main__.py(2 hunks)cratedb_mcp/cli.py(1 hunks)pyproject.toml(2 hunks)tests/test_cli.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- CHANGES.md
- cratedb_mcp/main.py
- pyproject.toml
- README.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_cli.py (1)
cratedb_mcp/cli.py (1)
cli(15-21)
🔇 Additional comments (12)
cratedb_mcp/cli.py (4)
4-5: LGTM: Necessary imports for Click frameworkThe imports for Click and the boot_click utility are appropriate for setting up the CLI framework.
12-22: LGTM: Well-structured CLI entry pointThe main CLI group is properly defined with version option and documentation. The use of
boot_clickfor initialization is appropriate.
28-42: LGTM: Well-structured options for the serve commandThe transport and port options are well-defined with appropriate defaults, environment variable mappings, and help messages.
43-51: Serve command implementation looks goodThe serve command implementation correctly logs the transport type, sets the port, and runs the MCP server with the specified transport.
One minor observation: The type cast in line 50 is necessary because you're passing a string to a function that expects the Literal type. This is a common pattern when dealing with string inputs that need to be validated against Literal types.
tests/test_cli.py (8)
1-8: LGTM: Appropriate imports for testingThe necessary imports for the tests are in place, including mock, anyio, CliRunner, and the cli to be tested.
11-26: LGTM: Version test is well implementedThe test for the version option correctly invokes the CLI and verifies the output contains the version.
29-49: LGTM: Default serve command test is comprehensiveThe test for the default serve command correctly mocks anyio.run, invokes the CLI, and verifies that the right function is called with the default port.
51-71: LGTM: Custom transport and port test is thoroughThe test for custom transport and port options correctly verifies that the streamable-http transport is used and the port is set to 1234.
73-89: LGTM: Validation test for invalid transport optionThe test correctly verifies that providing an invalid transport option results in an error message and non-zero exit code.
91-108: LGTM: Validation test for invalid transport environment variableThe test correctly verifies that setting an invalid transport via environment variable results in an error.
110-126: LGTM: Validation test for invalid port optionThe test correctly verifies that providing an invalid port option results in an error message.
128-145: LGTM: Validation test for invalid port environment variableThe test correctly verifies that setting an invalid port via environment variable results in an error.
cratedb-mcp servecratedb-mcp serve using Click
... with dedicated options `--transport` and `--port`.
About
Explicit is better than implicit: Following common sense, and excellent blueprints of others, this patch adds command-line parsing using a standard interface, to improve discoverability of subcommands and options for both humans and machines.
Details
cratedb-mcp servenow provides dedicated options--transportand--port.streamable-httptransport.