MCP: Fix get_cratedb_documentation_index tool
#60
Conversation
It was blocking calls, at least as observed with `mcptools` program. -- https://github.com/f/mcptools
Otherwise, validating the example gives a skewed impression of the outcome, because validation happens against the _previous_ release instead of the current working tree.
WalkthroughThe changes update the return type annotation of the Changes
Estimated code review effort1 (~5 minutes) Possibly related PRs
Suggested reviewers
Poem
✨ 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGES.md(1 hunks)cratedb_mcp/tool.py(1 hunks)examples/mcptools.sh(1 hunks)
🧠 Learnings (3)
📓 Common learnings
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#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.
CHANGES.md (4)
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.
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.
examples/mcptools.sh (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: #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: #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.
🧬 Code Graph Analysis (1)
examples/mcptools.sh (2)
cratedb_mcp/cli.py (1)
serve(58-72)cratedb_mcp/tool.py (5)
query_sql(20-23)get_table_metadata(26-33)get_cluster_health(58-60)get_cratedb_documentation_index(43-45)fetch_cratedb_docs(48-52)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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#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.
CHANGES.md (4)
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.
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.
examples/mcptools.sh (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: #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: #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.
🧬 Code Graph Analysis (1)
examples/mcptools.sh (2)
cratedb_mcp/cli.py (1)
serve(58-72)cratedb_mcp/tool.py (5)
query_sql(20-23)get_table_metadata(26-33)get_cluster_health(58-60)get_cratedb_documentation_index(43-45)fetch_cratedb_docs(48-52)
⏰ 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 (4)
examples/mcptools.sh (3)
27-27: Consistent removal ofuvxargument across all commands.The removal of
uvxfrom themcpt toolscommand is consistent with the pattern applied throughout the script.
30-32: LGTM! Consistent command format changes.The removal of
uvxfrom these Text-to-SQL tool invocations maintains consistency with the updated command format.
35-38: Manual Verification Required: EnsuremcptoolsIs Installed and AccessibleThe test output shows that the
mcptoolsbinary isn’t present in the environment, so we can’t confirm whether the new command format works withoutuvx. Please verify the following in your local setup wheremcptoolsis installed:• Install or build
mcptools(e.g., viapip install mcptoolsor from source).
• Run the updated commands and ensure they complete without errors:mcptools call get_cratedb_documentation_index cratedb-mcp serve mcptools call \ fetch_cratedb_docs --params '{"link":"https://cratedb.com/docs/cloud/en/latest/_sources/cluster/integrations/mongo-cdc.md.txt"}' \ cratedb-mcp serve• Confirm that both calls succeed and return the expected documentation output.
CHANGES.md (1)
4-4: LGTM! Clear and accurate changelog entry.The changelog entry clearly documents the fix for the
get_cratedb_documentation_indextool, which aligns with the changes made in the codebase and the PR objectives.
|
|
||
|
|
||
| def get_cratedb_documentation_index() -> dict: | ||
| def get_cratedb_documentation_index() -> list: |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Type annotation fix aligns with actual return type.
The change from -> dict to -> list appears to correctly reflect what documentation_index.items() actually returns. This type mismatch likely caused the tool dysfunction mentioned in the PR objectives.
To confirm the fix is correct, let's verify what documentation_index.items() actually returns:
🏁 Script executed:
#!/bin/bash
# Verify the return type of documentation_index.items()
ast-grep --pattern $'class DocumentationIndex {
$$$
def items($_) {
$$$
}
$$$
}'Length of output: 93
Let’s locate and inspect the actual DocumentationIndex implementation and its items() method:
🏁 Script executed:
#!/bin/bash
set -e
# Find the file(s) defining DocumentationIndex
echo "Finding DocumentationIndex definition…"
DOC_FILE=$(rg -l "class DocumentationIndex")
# Dump the start of that file to see its class signature and context
echo "----- $DOC_FILE (head) -----"
sed -n '1,200p' "$DOC_FILE"
# Show exactly how items() is implemented
echo "----- items() implementation -----"
grep -R "def items" -n "$DOC_FILE"Length of output: 7191
Inspecting get_cratedb_documentation_index implementation:
🏁 Script executed:
#!/bin/bash
# Show the function definition and surrounding context in tool.py
rg -n "get_cratedb_documentation_index" -A5 -B5 cratedb_mcp/tool.pyLength of output: 409
Fix return type annotation for get_cratedb_documentation_index
The call to documentation_index.items() returns the result of .to_dict(), i.e. a Python dict, not a list. Please update the signature accordingly:
• File: cratedb_mcp/tool.py
• Line 43: change
-def get_cratedb_documentation_index() -> list:
+def get_cratedb_documentation_index() -> dict:(or an equivalent Dict[str, Any] annotation for clarity)
This will realign the annotation with the actual return value and prevent downstream type-checker or client issues.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_cratedb_documentation_index() -> list: | |
| -def get_cratedb_documentation_index() -> list: | |
| +def get_cratedb_documentation_index() -> dict: |
🤖 Prompt for AI Agents
In cratedb_mcp/tool.py at line 43, the return type annotation of
get_cratedb_documentation_index is incorrectly set to list, but the function
actually returns a dictionary from documentation_index.items(). Update the
return type annotation to Dict[str, Any] or an equivalent dictionary type to
accurately reflect the returned value and avoid type-checking errors.
There was a problem hiding this comment.
@coderabbitai: Thanks for finding this. We found the to_dict() method uses a semantically wrong name, so this conveyed a wrong impression about its return type. We've created a ticket to fix it on the other project later. Currently, it doesn't do any other harm.
There was a problem hiding this comment.
@amotl Thank you for the clarification! That makes perfect sense - having a method named to_dict() that actually returns a list would definitely create confusion about return types. It's great that you've created a ticket to address the semantic naming issue in the other project. This kind of misleading method naming can easily lead to type annotation errors like we saw here.
✏️ Learnings added
Learnt from: amotl
PR: crate/cratedb-mcp#60
File: cratedb_mcp/tool.py:43-43
Timestamp: 2025-07-22T12:46:59.324Z
Learning: In the cratedb-mcp repository, the DocumentationIndex class has a method named `to_dict()` that actually returns a list, not a dictionary. This misleading method name can cause confusion about return types in functions that call it, such as `get_cratedb_documentation_index()`. A ticket was created to fix this semantic naming issue: https://github.com/crate/about/issues/55
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
About
With v0.0.4, the tool
get_cratedb_documentation_indexwent south, because of a wrong type annotation.References
get_cratedb_documentation_indexblocks with v0.0.4 #58