Prompt: Enable user-defined instructions #57
Conversation
WalkthroughThis update refactors the MCP server initialization by encapsulating instruction prompt loading and tool registration inside a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant CrateDbMcp
participant InstructionsPrompt
participant FastMCP
User->>CLI: Run serve command (with optional --instructions, --conventions)
CLI->>CrateDbMcp: Instantiate (instructions, conventions)
CrateDbMcp->>InstructionsPrompt: Initialize with instructions, conventions
InstructionsPrompt->>InstructionsPrompt: Load and render fragments
CrateDbMcp->>FastMCP: Initialize with rendered instructions
CrateDbMcp->>FastMCP: Register CrateDB tools
CLI->>FastMCP: Run server (with transport options)
Estimated code review effort3 (~45 minutes) Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
cratedb_mcp/prompt.py (1)
23-23: Address the TODO comment about fragment ordering.The TODO comment suggests uncertainty about the order of instruction fragments. Consider documenting the intended order or making it configurable.
Would you like me to help clarify the intended order of instruction fragments or suggest a configuration approach for this?
cratedb_mcp/cli.py (2)
62-62: Fix typo in help text.There's an extra closing parenthesis in the help text for the
--conventionsoption.Apply this diff to fix the typo:
- help="If you want to add custom conventions to the prompt, use this option)", + help="If you want to add custom conventions to the prompt, use this option",
65-67: Update type annotations for optional parameters.The
instructionsandconventionsparameters should be annotated as optional since they haverequired=Falsein their CLI option definitions.Apply this diff to fix the type annotations:
-def serve( - ctx: click.Context, transport: str, host: str, port: int, instructions: str, conventions: str -) -> None: +def serve( + ctx: click.Context, transport: str, host: str, port: int, instructions: t.Optional[str], conventions: t.Optional[str] +) -> None:cratedb_mcp/core.py (1)
31-81: Well-organized tool registration with minor enhancement opportunities.The tool registration is clearly organized with appropriate categorization and descriptions. Consider these optional improvements:
- Method chaining: Return
selfto enable fluent interface- Error handling: Add try-catch for tool registration failures
Optional enhancement:
def add_tools(self): + """Register all CrateDB MCP tools with the FastMCP instance.""" + try: # Text-to-SQL tools self.mcp.add_tool(...) # ... other tool registrations + return self + except Exception as e: + raise RuntimeError(f"Failed to register MCP tools: {e}") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGES.md(1 hunks)cratedb_mcp/__main__.py(1 hunks)cratedb_mcp/cli.py(2 hunks)cratedb_mcp/core.py(1 hunks)cratedb_mcp/mcp-instructions.md(0 hunks)cratedb_mcp/prompt.py(1 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (1)
- cratedb_mcp/mcp-instructions.md
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the `backlog.md` document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
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.
Learnt from: amotl
PR: crate/cratedb-mcp#24
File: pyproject.toml:76-76
Timestamp: 2025-05-17T16:02:09.342Z
Learning: For the cratedb-mcp project, FastMCP pre-2.0 releases provide the same API surface needed by the application, so a lower bound on the FastMCP dependency is not necessary.
Learnt from: amotl
PR: crate/cratedb-mcp#54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.649Z
Learning: In the cratedb-mcp project, tests should avoid asserting against `mcp.settings` internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
CHANGES.md (4)
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.
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the `backlog.md` document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
Learnt from: amotl
PR: crate/cratedb-mcp#24
File: pyproject.toml:76-76
Timestamp: 2025-05-17T16:02:09.342Z
Learning: For the cratedb-mcp project, FastMCP pre-2.0 releases provide the same API surface needed by the application, so a lower bound on the FastMCP dependency is not necessary.
Learnt from: amotl
PR: crate/cratedb-mcp#54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.649Z
Learning: In the cratedb-mcp project, tests should avoid asserting against `mcp.settings` internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
pyproject.toml (4)
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.
Learnt from: amotl
PR: crate/cratedb-mcp#54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.649Z
Learning: In the cratedb-mcp project, tests should avoid asserting against `mcp.settings` internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the `backlog.md` document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
Learnt from: amotl
PR: crate/cratedb-mcp#24
File: pyproject.toml:76-76
Timestamp: 2025-05-17T16:02:09.342Z
Learning: For the cratedb-mcp project, FastMCP pre-2.0 releases provide the same API surface needed by the application, so a lower bound on the FastMCP dependency is not necessary.
cratedb_mcp/prompt.py (1)
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the `backlog.md` document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
cratedb_mcp/core.py (4)
Learnt from: amotl
PR: crate/cratedb-mcp#54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.649Z
Learning: In the cratedb-mcp project, tests should avoid asserting against `mcp.settings` internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
Learnt from: amotl
PR: crate/cratedb-mcp#24
File: pyproject.toml:76-76
Timestamp: 2025-05-17T16:02:09.342Z
Learning: For the cratedb-mcp project, FastMCP pre-2.0 releases provide the same API surface needed by the application, so a lower bound on the FastMCP dependency is not necessary.
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the `backlog.md` document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
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.
cratedb_mcp/cli.py (2)
Learnt from: amotl
PR: crate/cratedb-mcp#54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.649Z
Learning: In the cratedb-mcp project, tests should avoid asserting against `mcp.settings` internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
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.
cratedb_mcp/__main__.py (4)
Learnt from: amotl
PR: crate/cratedb-mcp#54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.649Z
Learning: In the cratedb-mcp project, tests should avoid asserting against `mcp.settings` internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
Learnt from: amotl
PR: crate/cratedb-mcp#24
File: pyproject.toml:76-76
Timestamp: 2025-05-17T16:02:09.342Z
Learning: For the cratedb-mcp project, FastMCP pre-2.0 releases provide the same API surface needed by the application, so a lower bound on the FastMCP dependency is not necessary.
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.
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the `backlog.md` document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
🧬 Code Graph Analysis (2)
cratedb_mcp/core.py (2)
cratedb_mcp/prompt.py (2)
InstructionsPrompt(10-53)render(34-35)cratedb_mcp/tool.py (5)
fetch_cratedb_docs(48-52)get_cluster_health(58-60)get_cratedb_documentation_index(43-45)get_table_metadata(26-33)query_sql(20-23)
cratedb_mcp/cli.py (1)
cratedb_mcp/core.py (1)
CratedbMCP(17-81)
🔇 Additional comments (4)
CHANGES.md (1)
7-8: LGTM! Clear documentation of new CLI options.The changelog entry accurately documents the new
--instructionsand--conventionsCLI options, highlighting their support for both file paths and URLs.pyproject.toml (1)
98-100: LGTM! Package data configuration supports subdirectory markdown files.The change from
"*.md"to"**/*.md"correctly enables inclusion of markdown files from subdirectories, which aligns with the new instruction and prompt files being added to the package.cratedb_mcp/cli.py (1)
72-79: LGTM! Clean integration of the new CLI options.The implementation properly:
- Conditionally sets transport kwargs based on transport type
- Passes the custom instructions and conventions to
CratedbMCP- Maintains backward compatibility with existing transport options
cratedb_mcp/core.py (1)
1-15: Clean import structure and organization.The imports are well-organized with proper grouping of external and internal dependencies. The use of relative imports for local modules is appropriate.
cratedb_mcp/__main__.py
Outdated
| # Is that a standard entrypoint that should be kept alive? | ||
| mcp = CratedbMCP().mcp |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify the purpose and design of this entry point.
The comment suggests uncertainty about whether this entry point should be maintained. Consider:
- If this is intended as a default/fallback entry point, document its purpose clearly
- This creates a
CratedbMCPinstance with default parameters, which may not align with user customizations available through the CLI - Consider whether this entry point is still needed given the CLI interface in
cratedb_mcp.cli
Let me check how this entry point is used in the codebase:
🏁 Script executed:
#!/bin/bash
# Check for references to this entry point
rg -A 3 -B 3 "cratedb_mcp\.__main__"Length of output: 483
Document and retain the default mcp entry point used by tests
This entry point is actively imported in your test suite, so it cannot be removed or renamed without breaking tests:
- tests/test_instructions.py and tests/test_cli.py both do
from cratedb_mcp.__main__ import mcp mcp = CratedbMCP().mcpprovides the default instance for both CLI fallback and test harnesses
Recommended actions:
- In
cratedb_mcp/__main__.py, add a module-level docstring explaining that this variable is the canonical defaultCratedbMCPinstance used by downstream consumers (CLI, tests, etc.). - If you intend to support custom configurations at runtime, either extend this entry point to accept parameters (e.g. via
if __name__ == "__main__":and CLI flags) or expose a factory function in a separate module and update tests to import from there.
🤖 Prompt for AI Agents
In cratedb_mcp/__main__.py around lines 3 to 4, add a module-level docstring at
the top explaining that the variable `mcp = CratedbMCP().mcp` is the canonical
default instance used by downstream consumers such as CLI fallback and test
suites. Retain this variable as is to avoid breaking imports in tests. If you
want to support custom configurations, implement a factory function in another
module and update tests to import from there, or add CLI parameter handling
under an `if __name__ == "__main__":` block.
There was a problem hiding this comment.
@surister: Maybe you know anything about it? Otherwise, let's remove __main__.py altogether?
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
18bd3f2 to
288f9d6
Compare
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
288f9d6 to
104aad3
Compare
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
104aad3 to
79b6913
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cratedb_mcp/__main__.py (1)
3-4: Document the purpose of this entry point.Based on the retrieved learnings, this entry point is actively imported in your test suite (
tests/test_instructions.pyandtests/test_cli.py), so it serves as the canonical defaultCrateDbMcpinstance for downstream consumers.Add a module-level docstring explaining that
mcp = CrateDbMcp().mcpis the canonical default instance used by downstream consumers such as CLI fallback and test suites.+""" +Default MCP instance entry point. + +This module provides the canonical default CrateDbMcp instance used by +downstream consumers such as CLI fallback and test suites. +""" from cratedb_mcp.core import CrateDbMcp -# Is that a standard entrypoint that should be kept alive? mcp = CrateDbMcp().mcp
🧹 Nitpick comments (4)
cratedb_mcp/instructions.py (2)
23-23: Address the TODO comment about instruction ordering.The TODO comment indicates uncertainty about the order of MCP and general instructions. Consider documenting the intended behavior or provide a configuration option for users to control the order.
Would you like me to suggest an approach for making the instruction ordering configurable or help clarify the intended default behavior?
44-53: Add error handling for edge cases.Consider adding error handling for common failure scenarios to provide better user experience.
@staticmethod def load_fragment(fragment: str) -> str: """ - From `llm`. + Load instruction fragment from various sources. + + Supports loading from: + - HTTP(S) URLs + - Local file paths + - Standard input (when fragment is "-") + - Direct string content - -- https://github.com/simonw/llm + Adapted from: https://github.com/simonw/llm """ + try: if fragment.startswith("http://") or fragment.startswith("https://"): with httpx.Client(follow_redirects=True, max_redirects=3, timeout=5.0) as client: response = client.get(fragment) response.raise_for_status() return response.text if fragment == "-": return sys.stdin.read() if Path(fragment).exists(): return Path(fragment).read_text() return fragment + except (httpx.RequestError, httpx.HTTPStatusError) as e: + raise ValueError(f"Failed to load fragment from URL '{fragment}': {e}") + except (OSError, IOError) as e: + raise ValueError(f"Failed to load fragment from file '{fragment}': {e}")tests/test_instruction.py (2)
24-24: Fix typos in test docstrings.There are typos in the docstrings: "Varify" should be "Verify".
- Varify custom instructions replace the built-in ones. + Verify custom instructions replace the built-in ones.- Varify custom conventions are added to the built-in ones. + Verify custom conventions are added to the built-in ones.- Varify fragments are loaded from local filesystem. + Verify fragments are loaded from local filesystem.Also applies to: 32-32, 42-42
52-52: Fix typo in test docstring.There are more typos in the docstrings: "Varify" should be "Verify".
- Varify fragments are loaded from STDIN. + Verify fragments are loaded from STDIN.- Varify fragments are loaded from HTTP URLs. + Verify fragments are loaded from HTTP URLs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGES.md(1 hunks)cratedb_mcp/__main__.py(1 hunks)cratedb_mcp/cli.py(2 hunks)cratedb_mcp/core.py(1 hunks)cratedb_mcp/instructions.md(0 hunks)cratedb_mcp/instructions.py(1 hunks)pyproject.toml(1 hunks)tests/test_cli.py(4 hunks)tests/test_instruction.py(1 hunks)tests/test_instructions.py(0 hunks)
💤 Files with no reviewable changes (2)
- cratedb_mcp/instructions.md
- tests/test_instructions.py
🚧 Files skipped from review as they are similar to previous changes (4)
- CHANGES.md
- pyproject.toml
- cratedb_mcp/core.py
- cratedb_mcp/cli.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the `backlog.md` document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
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.
Learnt from: amotl
PR: crate/cratedb-mcp#54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.649Z
Learning: In the cratedb-mcp project, tests should avoid asserting against `mcp.settings` internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
Learnt from: amotl
PR: crate/cratedb-mcp#24
File: pyproject.toml:76-76
Timestamp: 2025-05-17T16:02:09.342Z
Learning: For the cratedb-mcp project, FastMCP pre-2.0 releases provide the same API surface needed by the application, so a lower bound on the FastMCP dependency is not necessary.
tests/test_cli.py (2)
Learnt from: amotl
PR: crate/cratedb-mcp#54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.649Z
Learning: In the cratedb-mcp project, tests should avoid asserting against `mcp.settings` internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
Learnt from: amotl
PR: crate/cratedb-mcp#24
File: pyproject.toml:76-76
Timestamp: 2025-05-17T16:02:09.342Z
Learning: For the cratedb-mcp project, FastMCP pre-2.0 releases provide the same API surface needed by the application, so a lower bound on the FastMCP dependency is not necessary.
cratedb_mcp/__main__.py (4)
Learnt from: amotl
PR: crate/cratedb-mcp#54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.649Z
Learning: In the cratedb-mcp project, tests should avoid asserting against `mcp.settings` internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
Learnt from: amotl
PR: crate/cratedb-mcp#24
File: pyproject.toml:76-76
Timestamp: 2025-05-17T16:02:09.342Z
Learning: For the cratedb-mcp project, FastMCP pre-2.0 releases provide the same API surface needed by the application, so a lower bound on the FastMCP dependency is not necessary.
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.
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the `backlog.md` document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
cratedb_mcp/instructions.py (2)
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the `backlog.md` document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
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.
tests/test_instruction.py (1)
Learnt from: amotl
PR: crate/cratedb-mcp#54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.649Z
Learning: In the cratedb-mcp project, tests should avoid asserting against `mcp.settings` internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
🔇 Additional comments (5)
tests/test_cli.py (4)
3-3: LGTM: Import updated to reflect new architecture.The import change from
cratedb_mcp.__main__tofastmcp.server.servercorrectly aligns with the architectural shift to using theCrateDbMcpclass instead of a globalmcpinstance.
69-69: LGTM: Mock target correctly updated.The mock target has been properly updated from the global
mcpinstance toFastMCP.run_async, which aligns with the newCrateDbMcpclass-based approach.
90-90: LGTM: Consistent mock target across tests.Good consistency in updating the mock target to
FastMCP.run_asyncacross all test functions.
102-102: LGTM: Explicit parameter verification aligns with best practices.The updated call arguments check with explicit
hostandportparameters is more robust and follows the learning about focusing on function call parameters rather than internal state.tests/test_instruction.py (1)
7-66: Excellent test coverage for the new InstructionsPrompt class.The test suite comprehensively covers all major functionality:
- Default instruction loading and rendering
- Custom instruction replacement
- Convention addition
- Loading from multiple sources (files, stdin, URLs)
The use of proper mocking for stdin and temporary files follows testing best practices.
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
79b6913 to
3db7b64
Compare
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
3db7b64 to
486fd55
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cratedb_mcp/prompt.py (2)
18-31: Solid constructor design with good fallback logic.The constructor handles both custom and default instruction scenarios well. The use of
importlib.resourcesfor accessing package files is appropriate.Consider adding validation to ensure the markdown file exists:
else: instructions_general = GeneralInstructions().render() + mcp_instructions_file = importlib.resources.files("cratedb_mcp") / "cratedb-mcp-instructions.md" + if not mcp_instructions_file.is_file(): + raise FileNotFoundError(f"MCP instructions file not found: {mcp_instructions_file}") - instructions_mcp = ( - importlib.resources.files("cratedb_mcp") / "cratedb-mcp-instructions.md" - ).read_text() + instructions_mcp = mcp_instructions_file.read_text()
36-65: Excellent implementation addressing previous review feedback.The method now properly handles:
- HTTP requests with context manager, timeout, and redirects
- Comprehensive error handling for network, file, and encoding errors
- UTF-8 encoding specification for file reading
- Descriptive error messages
The implementation effectively addresses all the concerns raised in previous reviews.
For enhanced security, consider adding basic path validation to prevent directory traversal:
if fragment == "-": return sys.stdin.read() path = Path(fragment) + # Basic security check to prevent directory traversal + if path.is_absolute() and not str(path).startswith(str(Path.cwd())): + raise ValueError(f"Absolute paths outside current directory not allowed: {path}") if path.exists(): return path.read_text(encoding="utf-8")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGES.md(1 hunks)cratedb_mcp/__main__.py(1 hunks)cratedb_mcp/cli.py(3 hunks)cratedb_mcp/core.py(1 hunks)cratedb_mcp/prompt.py(1 hunks)pyproject.toml(1 hunks)tests/test_cli.py(3 hunks)tests/test_instruction.py(1 hunks)tests/test_instructions.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/test_instructions.py
🚧 Files skipped from review as they are similar to previous changes (7)
- CHANGES.md
- pyproject.toml
- tests/test_cli.py
- cratedb_mcp/main.py
- cratedb_mcp/core.py
- cratedb_mcp/cli.py
- tests/test_instruction.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the `backlog.md` document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
Learnt from: amotl
PR: crate/cratedb-mcp#54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against `mcp.settings` internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
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.
Learnt from: amotl
PR: crate/cratedb-mcp#24
File: pyproject.toml:76-76
Timestamp: 2025-05-17T16:02:09.342Z
Learning: For the cratedb-mcp project, FastMCP pre-2.0 releases provide the same API surface needed by the application, so a lower bound on the FastMCP dependency is not necessary.
cratedb_mcp/prompt.py (2)
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the backlog.md document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
Learnt from: amotl
PR: #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.
🔇 Additional comments (3)
cratedb_mcp/prompt.py (3)
1-7: LGTM! Clean import structure.The imports are well-organized, following the standard convention of standard library imports first, then third-party libraries. All imports are used within the code.
10-16: Well-documented class with clear purpose.The class name and docstring clearly communicate the intent to bundle MCP tools instructions with general CrateDB instructions. The external references provide helpful context.
33-34: Clean and efficient render implementation.The method effectively concatenates fragments with proper spacing while handling whitespace cleanup. Simple and effective.
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
486fd55 to
c6a1b2a
Compare
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
c6a1b2a to
c928fe1
Compare
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
c928fe1 to
ba58751
Compare
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
ba58751 to
bf306f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
312-318: Streamline the opening sentence for clarity and concisionThe current wording is a bit verbose and contains an unnecessary comma after “prompt.” A tighter phrasing improves readability and removes the “provide options to give” pattern flagged by LanguageTool.
-The CrateDB MCP server provides options to give users the flexibility to adjust -the system prompt, by redefining the baseline instructions, or by extending them -using custom conventions. Those additional conventions could be about user-specific -bits of information that are required to work with certain ER data models, or other -conventions users may elaborate as they go. +The CrateDB MCP server lets you adjust the system prompt by either redefining the +baseline instructions or extending them with custom conventions. Additional +conventions can capture domain-specific details—such as information required for +particular ER data models—or any other guidelines you develop over time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGES.md(1 hunks)README.md(1 hunks)cratedb_mcp/__main__.py(1 hunks)cratedb_mcp/cli.py(3 hunks)cratedb_mcp/core.py(1 hunks)cratedb_mcp/prompt/__init__.py(1 hunks)pyproject.toml(2 hunks)tests/test_cli.py(5 hunks)tests/test_instructions.py(0 hunks)tests/test_prompt.py(1 hunks)
🧠 Learnings (2)
📓 Common learnings
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the `backlog.md` document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
Learnt from: amotl
PR: crate/cratedb-mcp#54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against `mcp.settings` internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
Learnt from: amotl
PR: crate/cratedb-mcp#24
File: pyproject.toml:76-76
Timestamp: 2025-05-17T16:02:09.342Z
Learning: For the cratedb-mcp project, FastMCP pre-2.0 releases provide the same API surface needed by the application, so a lower bound on the FastMCP dependency is not necessary.
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.
README.md (3)
Learnt from: amotl
PR: #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.
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the backlog.md document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
Learnt from: amotl
PR: #54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against mcp.settings internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
🪛 LanguageTool
README.md
[style] ~314-~314: Consider replacing ‘give’ with a different word to let your writing stand out.
Context: ... CrateDB MCP server provides options to give users the flexibility to adjust the system pr...
(GIVE_TIME_STYLE)
💤 Files with no reviewable changes (1)
- tests/test_instructions.py
🚧 Files skipped from review as they are similar to previous changes (8)
- CHANGES.md
- cratedb_mcp/core.py
- tests/test_prompt.py
- cratedb_mcp/main.py
- pyproject.toml
- tests/test_cli.py
- cratedb_mcp/cli.py
- cratedb_mcp/prompt/init.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the `backlog.md` document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
Learnt from: amotl
PR: crate/cratedb-mcp#54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against `mcp.settings` internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
Learnt from: amotl
PR: crate/cratedb-mcp#24
File: pyproject.toml:76-76
Timestamp: 2025-05-17T16:02:09.342Z
Learning: For the cratedb-mcp project, FastMCP pre-2.0 releases provide the same API surface needed by the application, so a lower bound on the FastMCP dependency is not necessary.
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.
README.md (3)
Learnt from: amotl
PR: #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.
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the backlog.md document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
Learnt from: amotl
PR: #54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against mcp.settings internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
🪛 LanguageTool
README.md
[style] ~314-~314: Consider replacing ‘give’ with a different word to let your writing stand out.
Context: ... CrateDB MCP server provides options to give users the flexibility to adjust the system pr...
(GIVE_TIME_STYLE)
🔇 Additional comments (1)
README.md (1)
332-338: Double-check thatshow-promptis the exact CLI subcommandThe README introduces a
show-promptsubcommand, but the actual implementation was not provided in this diff. If the code registers the command under a different namespace (e.g.prompt showorcratedb-mcp prompt --show), this example will confuse users.Would you confirm the CLI entry point name in
cratedb_mcp/cli.pyand adjust the snippet if necessary?
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
bf306f9 to
38bf85c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
314-318: Tighten wording & drop redundant commasThe sentence is slightly clunky (“provides options to give users the flexibility…”) and has an extra comma after prompt. A crisper version improves readability.
-The CrateDB MCP server provides options to give users the flexibility to adjust -the system prompt, by redefining the baseline instructions, or by extending them -using custom conventions. Those additional conventions could be about user-specific +The CrateDB MCP server allows users to adjust the system prompt by either +redefining the baseline instructions or extending them with custom conventions. +These additional conventions can capture user-specific bits of information that are required to work with certain ER data models, or other conventions users may elaborate as they go.
339-347: Minor style nits (hyphen & list heading)
- “Instruction- and convention-fragments” reads awkwardly; drop the hyphen.
- Consider adding a short lead-in line so the bullet list isn’t floating.
-Instruction- and convention-fragments can be loaded from various sources. +Instruction and convention fragments can be loaded from the following sources:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGES.md(1 hunks)README.md(1 hunks)cratedb_mcp/__main__.py(1 hunks)cratedb_mcp/cli.py(3 hunks)cratedb_mcp/core.py(1 hunks)cratedb_mcp/prompt/__init__.py(1 hunks)pyproject.toml(2 hunks)tests/test_cli.py(5 hunks)tests/test_instructions.py(0 hunks)tests/test_prompt.py(1 hunks)
🧠 Learnings (1)
README.md (3)
Learnt from: amotl
PR: #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.
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the backlog.md document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
Learnt from: amotl
PR: #54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against mcp.settings internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
🪛 LanguageTool
README.md
[style] ~314-~314: Consider replacing ‘give’ with a different word to let your writing stand out.
Context: ... CrateDB MCP server provides options to give users the flexibility to adjust the system pr...
(GIVE_TIME_STYLE)
💤 Files with no reviewable changes (1)
- tests/test_instructions.py
🚧 Files skipped from review as they are similar to previous changes (8)
- CHANGES.md
- cratedb_mcp/main.py
- tests/test_cli.py
- pyproject.toml
- cratedb_mcp/cli.py
- cratedb_mcp/core.py
- cratedb_mcp/prompt/init.py
- tests/test_prompt.py
🧰 Additional context used
🧠 Learnings (1)
README.md (3)
Learnt from: amotl
PR: #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.
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the backlog.md document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
Learnt from: amotl
PR: #54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against mcp.settings internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
🪛 LanguageTool
README.md
[style] ~314-~314: Consider replacing ‘give’ with a different word to let your writing stand out.
Context: ... CrateDB MCP server provides options to give users the flexibility to adjust the system pr...
(GIVE_TIME_STYLE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: oci
- GitHub Check: oci
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
38bf85c to
001ec0d
Compare
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
001ec0d to
e03139d
Compare
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
e03139d to
a57e298
Compare
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
a57e298 to
6c67011
Compare
amotl
changed the title
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
6c67011 to
23a78dc
Compare
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
23a78dc to
6122d4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cratedb_mcp/__main__.py (1)
1-4: Retain and document this entry point - it's actively used by tests.Based on the past review comments, this entry point is imported by tests (
from cratedb_mcp.__main__ import mcp), so it cannot be removed. The architectural change to delegate toCrateDbMcpis excellent - it centralizes the MCP setup logic.Consider adding a module docstring to clarify this serves as the canonical default MCP instance for downstream consumers:
+""" +Default MCP instance for cratedb-mcp. + +This module provides the canonical default CrateDbMcp instance used by +downstream consumers such as tests and CLI fallback scenarios. +""" from cratedb_mcp.core import CrateDbMcp
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGES.md(1 hunks)README.md(1 hunks)cratedb_mcp/__main__.py(1 hunks)cratedb_mcp/cli.py(3 hunks)cratedb_mcp/core.py(1 hunks)cratedb_mcp/prompt/__init__.py(1 hunks)pyproject.toml(2 hunks)tests/test_cli.py(5 hunks)tests/test_instructions.py(0 hunks)tests/test_prompt.py(1 hunks)
🧠 Learnings (3)
📓 Common learnings
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the `backlog.md` document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
Learnt from: amotl
PR: crate/cratedb-mcp#54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against `mcp.settings` internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
Learnt from: amotl
PR: crate/cratedb-mcp#24
File: pyproject.toml:76-76
Timestamp: 2025-05-17T16:02:09.342Z
Learning: For the cratedb-mcp project, FastMCP pre-2.0 releases provide the same API surface needed by the application, so a lower bound on the FastMCP dependency is not necessary.
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.
tests/test_prompt.py (1)
Learnt from: amotl
PR: #54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against mcp.settings internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
cratedb_mcp/__main__.py (4)
Learnt from: amotl
PR: #54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against mcp.settings internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
Learnt from: amotl
PR: #24
File: pyproject.toml:76-76
Timestamp: 2025-05-17T16:02:09.342Z
Learning: For the cratedb-mcp project, FastMCP pre-2.0 releases provide the same API surface needed by the application, so a lower bound on the FastMCP dependency is not necessary.
Learnt from: amotl
PR: #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.
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the backlog.md document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
🧬 Code Graph Analysis (2)
tests/test_prompt.py (1)
cratedb_mcp/prompt/__init__.py (2)
InstructionsPrompt(10-68)render(36-37)
cratedb_mcp/__main__.py (1)
cratedb_mcp/core.py (1)
CrateDbMcp(18-90)
🪛 GitHub Actions: Tests
tests/test_prompt.py
[error] 76-76: Test failure: AssertionError due to regex mismatch. Expected error message for HTTP 404 but got HTTP 503 'Service Temporarily Unavailable' instead.
💤 Files with no reviewable changes (1)
- tests/test_instructions.py
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/test_cli.py
- cratedb_mcp/cli.py
- CHANGES.md
- pyproject.toml
- cratedb_mcp/core.py
- cratedb_mcp/prompt/init.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the `backlog.md` document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
Learnt from: amotl
PR: crate/cratedb-mcp#54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against `mcp.settings` internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
Learnt from: amotl
PR: crate/cratedb-mcp#24
File: pyproject.toml:76-76
Timestamp: 2025-05-17T16:02:09.342Z
Learning: For the cratedb-mcp project, FastMCP pre-2.0 releases provide the same API surface needed by the application, so a lower bound on the FastMCP dependency is not necessary.
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.
tests/test_prompt.py (1)
Learnt from: amotl
PR: #54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against mcp.settings internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
cratedb_mcp/__main__.py (4)
Learnt from: amotl
PR: #54
File: tests/test_cli.py:0-0
Timestamp: 2025-07-20T17:17:10.660Z
Learning: In the cratedb-mcp project, tests should avoid asserting against mcp.settings internal state as these assertions will break with later versions of FastMCP. Tests should focus on verifying that functions are called with correct parameters rather than checking internal implementation details.
Learnt from: amotl
PR: #24
File: pyproject.toml:76-76
Timestamp: 2025-05-17T16:02:09.342Z
Learning: For the cratedb-mcp project, FastMCP pre-2.0 releases provide the same API surface needed by the application, so a lower bound on the FastMCP dependency is not necessary.
Learnt from: amotl
PR: #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.
Learnt from: amotl
PR: crate/cratedb-mcp#0
File: :0-0
Timestamp: 2025-05-17T14:55:23.400Z
Learning: In the cratedb-mcp repository, the backlog.md document is intended as a functional planning document that doesn't need to adhere to strict formatting standards. Avoid pointing out formatting or layout shortcomings in this file during reviews.
🧬 Code Graph Analysis (2)
tests/test_prompt.py (1)
cratedb_mcp/prompt/__init__.py (2)
InstructionsPrompt(10-68)render(36-37)
cratedb_mcp/__main__.py (1)
cratedb_mcp/core.py (1)
CrateDbMcp(18-90)
🪛 GitHub Actions: Tests
tests/test_prompt.py
[error] 76-76: Test failure: AssertionError due to regex mismatch. Expected error message for HTTP 404 but got HTTP 503 'Service Temporarily Unavailable' instead.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: oci
- GitHub Check: oci
🔇 Additional comments (6)
tests/test_prompt.py (6)
9-22: Well-structured default instructions test.The test appropriately verifies that the default MCP instance contains both MCP-specific and general CrateDB instructions. The assertions check for expected content without relying on FastMCP internal state, which aligns with the project's testing guidelines.
24-30: Good test for custom instructions replacement.This test correctly verifies that custom instructions completely replace the built-in ones, which is the expected behavior when users provide their own instructions.
32-40: Effective test for custom conventions augmentation.This test properly validates that custom conventions are added to the existing built-in instructions rather than replacing them, which matches the intended functionality.
42-50: Good test coverage for local file loading.The test properly uses pytest's
tmp_pathfixture to create a temporary file and verify that fragments can be loaded from the local filesystem.
52-60: Proper stdin mocking for fragment loading.The test correctly mocks
sys.stdinto verify that fragments can be loaded from standard input when using the "-" parameter.
62-68: Good test for successful HTTP fragment loading.This test verifies that fragments can be loaded from HTTP URLs successfully by using a reliable public endpoint.
- Both options `--instructions` and `--conventions` accept file paths, URLs, or stdin. - Add subcommand `cratedb-mcp show-prompt` to display the system prompt. With both features, you can easily customize the built-in system prompt and iterate on it.
amotl
force-pushed
the
prompt-user-defined-instructions
branch
from
6122d4e to
21f6628
Compare
About
Give users the flexibility to adjust the system prompt by means of redefining the baseline instructions, or by extending them using custom conventions. Those additional conventions could be about user-specific bits of information that are required to work with certain ER data models, or other conventions users may elaborate as they go.
Details
--instructionsand--conventionsaccept file paths, URLs, or stdin.cratedb-mcp show-promptdisplays the system prompt.With both features, you can easily customize the built-in system prompt and iterate on it.
Backlog