Boilerplate: Add software tests and CI configuration #9
Conversation
|
@coderabbitai: full review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1455 🏁 Script executed: Length of output: 4138 🏁 Script executed: Length of output: 5457 @amotl Here's my full review of your PR introducing tests and CI configuration: Overall AssessmentThis is an excellent addition to the project! You've established a robust foundation for testing and CI/CD that will greatly improve code quality and development workflow. Strengths
Suggestions for Improvement
The PR sets up an excellent foundation for maintaining and improving code quality as the project evolves. Nice work! |
|
Warning Rate limit exceeded@amotl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis update introduces comprehensive project scaffolding and automation for a Python package, including CI/CD workflows, dependency management, development and release documentation, and initial test coverage. It refactors configuration handling, expands project metadata and tool integrations, and adds or updates documentation, code style, and testing infrastructure. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub
participant CI_Workflow
participant PyPI
participant TestPyPI
Developer->>GitHub: Push code / PR / Tag / Manual trigger
GitHub->>CI_Workflow: Trigger tests.yml (on push/PR)
CI_Workflow->>CI_Workflow: Setup Python, install deps, run lint/tests
CI_Workflow->>Codecov: Upload coverage
GitHub->>CI_Workflow: Trigger release-pypi.yml (on tag/PR/schedule)
CI_Workflow->>CI_Workflow: Build package
CI_Workflow->>TestPyPI: Publish to TestPyPI (always)
alt On tag push
CI_Workflow->>PyPI: Publish to PyPI (release)
end
Poem
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 (14)
cratedb_mcp/knowledge.py (1)
1-1: Review the scope of thenoqadirective.Disabling E501 for the entire file suppresses line-length checks globally, including critical SQL query strings. Consider scoping the
noqato only the overly long lines or refactoring the SQL (e.g., usingtextwrap.dedent, splitting into smaller literals, or externalizing it) to preserve lint coverage elsewhere.pyproject.toml (7)
28-34: Optimize development dependencies.Dev extras include key tools; consider adding
pre-commitfor consistent linting/formatting and ensuring hook alignment across contributors.
44-45: Reevaluate Ruff line-length setting.A limit of 110 chars deviates from common defaults (e.g., 88 or 100). Align with your code style guide or document this choice for contributors.
82-93: Assess Pytest configuration verbosity.Logging at DEBUG (
log_level,log_cli_level) may produce excessive output in CI. Consider lowering to INFO or making it configurable.
103-107: Consider enabling branch coverage.Currently
branch = false; enabling (branch = true) would yield more complete coverage metrics.
116-123: Tighten MyPy settings.With
ignore_missing_imports = true, missing stubs won’t be flagged. Consider addingwarn_unused_ignores = trueor providing stub packages for dependencies.
142-148: Uncomment formatting commands.Auto-formatting is disabled; consider enabling Ruff’s
formatfixer:-format = [ - #{ cmd = "ruff format ." }, +format = [ + { cmd = "ruff format ." }, { cmd = "pyproject-fmt --keep-full-version pyproject.toml" }, ]to catch issues pre-commit.
150-154: Extendlinttask with format checks.Consider adding a
poe format --checkstep beforeruff checkto enforce formatting.cratedb_mcp/settings.py (1)
1-3: Centralize HTTP URL and add validation.Defaulting to
http://localhost:4200is fine. Consider annotatingHTTP_URL: strand adding a basic URL validation (e.g., viaurllib.parse) to catch misconfigurations early.tests/test_knowledge.py (1)
10-11: Consider expanding the queries test for more comprehensive coverage.While the current test verifies that the TABLES_METADATA query contains a specific string, consider adding more assertions to validate other critical parts of the query structure or syntax.
def test_queries(): assert "information_schema.tables" in Queries.TABLES_METADATA + # Verify other critical parts of the query + assert "sys.health" in Queries.TABLES_METADATA + assert "WITH partitions_health" in Queries.TABLES_METADATA + assert "LEFT JOIN" in Queries.TABLES_METADATAREADME.md (2)
58-59: Fix markdown linting issues with bare URLs.The markdown linter detected bare URLs that should be properly formatted as links.
-For example, when connecting to CrateDB Cloud, use a value like -https://admin:dZ...6LqB@testdrive.eks1.eu-west-1.aws.cratedb.net:4200/. -When connecting to CrateDB on localhost, use http://localhost:4200/. +For example, when connecting to CrateDB Cloud, use a value like +`https://admin:dZ...6LqB@testdrive.eks1.eu-west-1.aws.cratedb.net:4200/`. +When connecting to CrateDB on localhost, use `http://localhost:4200/`.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
58-58: Bare URL used
null(MD034, no-bare-urls)
59-59: Bare URL used
null(MD034, no-bare-urls)
117-119: Remove unused badge definitions.The static analysis identified unused badge definitions that should be removed to keep the README clean.
[badge-bluesky]: https://img.shields.io/badge/Bluesky-0285FF?logo=bluesky&logoColor=fff&label=Follow%20%40CrateDB -[badge-issues]: https://img.shields.io/github/issues/crate/cratedb-mcp [badge-license]: https://img.shields.io/github/license/crate/cratedb-mcp -[badge-release-notes]: https://img.shields.io/badge/Release%20Notes-v0.0.0-blue [badge-status]: https://img.shields.io/badge/status--alpha-orange🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
117-117: Link and image reference definitions should be needed
Unused link or image reference definition: "badge-issues"(MD053, link-image-reference-definitions)
119-119: Link and image reference definitions should be needed
Unused link or image reference definition: "badge-release-notes"(MD053, link-image-reference-definitions)
DEVELOP.md (2)
15-32: Well-documented test workflow with practical examplesClear explanation of how to run tests both through the task runner and directly with pytest, including helpful examples for specific test cases. This will be valuable for contributors.
For improved conciseness, consider changing "In order to invoke" to "To invoke" on line 24.
🧰 Tools
🪛 LanguageTool
[style] ~23-~23: Consider a more concise word here.
Context: ...used on CI/GHA.shell poe checkIn order to invoke individual software tests for wo...(IN_ORDER_TO_PREMIUM)
33-44: Clear release process documentationThe release documentation clearly explains the versioning approach using
versioningitand provides the exact commands needed for creating a new release.The comma before "because" on line 36 could be removed as the clause is essential to the meaning: "...any version bumping within files because the version number will be derived from the Git tag."
🧰 Tools
🪛 LanguageTool
[formatting] ~36-~36: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...d to do any version bumping within files, because the version number will be derived from...(COMMA_BEFORE_BECAUSE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/dependabot.yml(1 hunks).github/workflows/release-pypi.yml(1 hunks).github/workflows/tests.yml(1 hunks)CHANGES.md(1 hunks)DEVELOP.md(1 hunks)README.md(4 hunks)cratedb_mcp/__main__.py(2 hunks)cratedb_mcp/cli.py(1 hunks)cratedb_mcp/knowledge.py(1 hunks)cratedb_mcp/settings.py(1 hunks)pyproject.toml(1 hunks)tests/test_knowledge.py(1 hunks)tests/test_mcp.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_knowledge.py (1)
cratedb_mcp/knowledge.py (1)
Queries(3-83)
cratedb_mcp/__main__.py (1)
cratedb_mcp/knowledge.py (1)
Queries(3-83)
🪛 LanguageTool
README.md
[style] ~76-~76: As a shorter alternative for ‘able to’, consider using “can not”.
Context: ...NSPORT=sse cratedb-mcp ``` Note: If you are not able to use uv tool install, please use `uv r...
(BE_ABLE_TO)
DEVELOP.md
[style] ~23-~23: Consider a more concise word here.
Context: ...used on CI/GHA. shell poe check In order to invoke individual software tests for wo...
(IN_ORDER_TO_PREMIUM)
[formatting] ~36-~36: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...d to do any version bumping within files, because the version number will be derived from...
(COMMA_BEFORE_BECAUSE)
🪛 markdownlint-cli2 (0.17.2)
README.md
58-58: Bare URL used
null
(MD034, no-bare-urls)
59-59: Bare URL used
null
(MD034, no-bare-urls)
117-117: Link and image reference definitions should be needed
Unused link or image reference definition: "badge-issues"
(MD053, link-image-reference-definitions)
119-119: Link and image reference definitions should be needed
Unused link or image reference definition: "badge-release-notes"
(MD053, link-image-reference-definitions)
🔇 Additional comments (43)
pyproject.toml (14)
1-6: Verify build-system requirements versions.Including
setuptools>=42andversioningitunder[build-system]is correct, but pinversioningitto a minimum version to avoid unexpected build failures.
12-18: Confirm Python classifiers accuracy.Classifiers for 3.10–3.13 and
Programming Language :: Python :: 3 :: Onlylook good; ensure official support for 3.13 is released and update as new interpreters stabilize.
20-21: Validate dynamic versioning setup.Using
dynamic = ["version"]withversioningitrequires proper tag-based version generation. Confirm that CI workflows produce the expected version string.
23-26: Review core dependency declaration.
mcp[cli]>=1.5.0is specified—verify this range covers needed features without pulling in breaking changes.
36-40: Inspect test dependencies.
pytest<9,pytest-cov<6,pytest-mock<4are reasonable bounds. Double-check compatibility with CI runners or drop upper bounds if newer versions remain compatible.
50-76: Review selected Ruff lint rules.The enabled codes cover a broad spectrum. Remove or adjust rules that conflict with project patterns to reduce false positives.
78-80: Permitassertin tests.Ignoring
S101intests/*is correct to allow pytest-style assertions.
98-101: Confirm coverage paths.Pointing
source = ["src/"]matches your layout—ensure no other source folders need inclusion.
109-114: Validate coverage report exclusions.Excluding
# pragma: no coverandraise NotImplementedis standard; check if other patterns (e.g., debug-only code) should be excluded.
126-129: Review Versioningit VCS configuration.
method = "git-archive"works for source distributions; verify tags (default-tag,describe-subst) produce correct versions in both sdist and wheel.
135-135: Check task section formatting.The separator comments under
[tool.poe.tasks]are decorative; ensure they don’t confuse parsers or documentation tools.
137-140: Confirmchecktask composition.Combining
lintandtestis clear—verify CI workflows callpoe checkas intended.
156-159: Ensure release tasks are explicit.Using
twine upload --skip-existingis safe; if targeting multiple indices or private repos, add flags (e.g.,--repository).
160-160: Harmonizetesttask with CI.
test = { cmd = "pytest" }bypasses coverage flags. Ensure CI invokespoe check(or similar) so coverage reports are generated..github/dependabot.yml (1)
1-17: Approve Dependabot configuration.Daily
pipupdates and monthlygithub-actionsupdates are reasonable. Optionally refine schedules (e.g., weekly) or set versioning policies (major/minor/patch) based on project stability.CHANGES.md (1)
1-14: Well-structured changelog entry for the initial release.The changelog provides a clear and organized record of the initial project setup and features. It effectively documents the key changes across project structure, features, packaging, and documentation.
tests/test_knowledge.py (1)
4-8: Good test for the documentation index constant.This test effectively verifies the structure and content of the DOCUMENTATION_INDEX constant, checking both its length and specific entries.
README.md (6)
6-7: Great addition of CI and coverage badges.The CI and coverage badges enhance the README by providing immediate visibility into the project's build status and test coverage, which is particularly valuable for an open-source project.
48-52: Clear installation instructions with helpful context.The updated installation command and explanatory note provide clear guidance for users on the recommended installation method.
56-65: Improved configuration section with explicit examples.The configuration section now provides clear instructions with practical examples for both CrateDB Cloud and localhost scenarios.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
58-58: Bare URL used
null(MD034, no-bare-urls)
59-59: Bare URL used
null(MD034, no-bare-urls)
76-77: Clear fallback instructions for users without uv tool install.The note provides helpful guidance for users who can't use the recommended installation method.
🧰 Tools
🪛 LanguageTool
[style] ~76-~76: As a shorter alternative for ‘able to’, consider using “can not”.
Context: ...NSPORT=sse cratedb-mcp ``` Note: If you are not able to useuv tool install, please use `uv r...(BE_ABLE_TO)
98-100: Great addition of a Development section with link to detailed documentation.The new Development section with a link to DEVELOP.md provides a clear path for contributors to find detailed setup instructions.
114-122: Well-configured badge definitions with appropriate links.The badge definitions are well-structured and link to the appropriate resources.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
117-117: Link and image reference definitions should be needed
Unused link or image reference definition: "badge-issues"(MD053, link-image-reference-definitions)
119-119: Link and image reference definitions should be needed
Unused link or image reference definition: "badge-release-notes"(MD053, link-image-reference-definitions)
.github/workflows/tests.yml (4)
9-11: Good use of concurrency controls to prevent redundant workflow runs.The concurrency configuration ensures that only one workflow runs at a time for a given branch and cancels in-progress runs when new commits are pushed, which improves CI efficiency.
37-45: Well-configured CrateDB service container for integration testing.The workflow effectively sets up a CrateDB service container with appropriate port mappings and configuration, which enables proper integration testing against a real database instance.
56-63: Efficient dependency caching with uv.The workflow uses the astral-sh/setup-uv action with dependency caching based on pyproject.toml, which will significantly speed up CI runs by reusing cached dependencies when possible.
72-79: Comprehensive coverage reporting setup with failure on upload errors.The workflow uploads coverage results to Codecov with appropriate environment variables and is configured to fail if the upload encounters errors, ensuring consistent coverage reporting.
cratedb_mcp/__main__.py (4)
4-5: Improved imports and configuration managementThe refactoring to import constants from centralized modules (
.knowledgeand.settings) instead of directly accessing environment variables improves maintainability and testability.
11-11: Clean implementation of the HTTP URL configurationUsing the
HTTP_URLconstant from settings rather than accessing environment variables directly in the function is a good practice that supports better testing and configuration management.
17-17: Improved conditional statement readabilityChanged from
if not 'select' in query.lower()toif 'select' not in query.lower(), which is more idiomatic Python and improves readability.
30-30: Consistent conditional style improvementChanged from
if not 'https://raw.githubusercontent.com/crate/crate/' in linktoif 'https://raw.githubusercontent.com/crate/crate/' not in link, matching the style used elsewhere and improving readability.DEVELOP.md (1)
1-14: Excellent development setup documentationThe setup instructions are clear, concise, and use modern tools like
uvfor virtual environment and package management. The editable install with development and test dependencies is a good practice for contributors.tests/test_mcp.py (7)
12-14: Good basic test for documentation indexThe test verifies that the documentation index contains at least three entries, establishing a baseline expectation for this functionality.
16-20: Thorough security boundary test for documentation fetchingThe test properly verifies that attempts to fetch documentation from non-GitHub URLs are rejected with the appropriate error message.
22-25: Positive test case for documentation fetchingThe test confirms that permitted GitHub raw URLs can be successfully fetched and that the response contains expected content.
27-31: SQL injection prevention testGood test to verify that non-SELECT statements are properly rejected, which is an important security boundary.
33-35: Positive test case for SQL queriesThe test confirms that valid SELECT queries return expected results.
42-44: Good functional test for table metadataThe test verifies that the table metadata function returns data containing expected key components.
46-48: Good functional test for health informationThe test confirms that the health status function returns data with expected key components.
.github/workflows/release-pypi.yml (4)
1-20: Well-configured workflow triggersThe release workflow is properly set up to trigger on tags (for releases), pull requests (for validation), scheduled runs (for regular validation), and manual triggers (for flexibility). This comprehensive approach ensures the package can be validated in multiple scenarios.
21-40: Proper security configuration for Trusted PublishingThe workflow correctly implements GitHub's Trusted Publishing with appropriate environment and permission settings, which is a best practice for secure package publishing.
41-57: Efficient build setup with cachingThe workflow uses the modern
uvtool with appropriate caching configuration to improve build efficiency.
58-65: Smart publishing strategyThe workflow publishes to TestPyPI unconditionally (for validation) but only publishes to the main PyPI when a tag is pushed, which is a good practice for release management.
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: ['ubuntu-latest'] | ||
| python-version: [ | ||
| '3.10', | ||
| '3.13', | ||
| ] | ||
| cratedb-version: [ | ||
| 'nightly', | ||
| ] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider expanding the Python version matrix for comprehensive compatibility testing.
The PR summary mentions support for Python 3.10 through 3.13, but the test matrix only includes 3.10 and 3.13. Adding 3.11 and 3.12 would provide more comprehensive coverage and ensure compatibility across all supported versions.
python-version: [
'3.10',
+ '3.11',
+ '3.12',
'3.13',
]🌐 Web query:
What are the active maintenance status of Python 3.10, 3.11, 3.12, and 3.13 versions?
💡 Result:
As of May 10, 2025, the maintenance statuses for Python versions 3.10, 3.11, 3.12, and 3.13 are as follows:
-
Python 3.10: Released on October 4, 2021, Python 3.10 has completed its active support phase and is now in the security support phase. This means it receives only security updates and no new features or general bug fixes. Security updates are provided irregularly in source-only form until October 2026. (peps.python.org)
-
Python 3.11: Released on October 24, 2022, Python 3.11 has also completed its active support phase and is now in the security support phase. Similar to Python 3.10, it receives only security updates in source-only form until October 2027. (endoflife.date)
-
Python 3.12: Released on October 2, 2023, Python 3.12 is currently in its active support phase, which includes regular bug fixes and security updates. Active support is scheduled to end on April 2, 2025, after which it will enter the security support phase, receiving only security updates until October 2, 2028. (peps.python.org)
-
Python 3.13: Released on October 7, 2024, Python 3.13 is the latest stable release and is currently in its active support phase, receiving regular updates and new features. Active support is scheduled to end on October 1, 2026, after which it will enter the security support phase, receiving only security updates until October 10, 2029. (endoflife.date)
For the most current information on Python release schedules and support phases, you can refer to the official Python website or the Python Enhancement Proposals (PEPs) related to release schedules.
Citations:
- 1: https://peps.python.org/pep-0619//?utm_source=openai
- 2: https://endoflife.date/python?utm_source=openai
- 3: https://peps.python.org/pep-0693/?utm_source=openai
- 4: https://endoflife.date/python?utm_source=openai
Include Python 3.11 and 3.12 in the CI test matrix
All supported versions (3.10–3.13) are still receiving maintenance—3.10 and 3.11 in security-only support, 3.12 and 3.13 in active support—so we should validate compatibility across the full range.
- File:
.github/workflows/tests.yml(lines 20–30) - Update the
python-versionmatrix:
python-version: [
'3.10',
+ '3.11',
+ '3.12',
'3.13',
]📝 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.
| strategy: | |
| fail-fast: false | |
| matrix: | |
| os: ['ubuntu-latest'] | |
| python-version: [ | |
| '3.10', | |
| '3.13', | |
| ] | |
| cratedb-version: [ | |
| 'nightly', | |
| ] | |
| python-version: [ | |
| '3.10', | |
| + '3.11', | |
| + '3.12', | |
| '3.13', | |
| ] |
🤖 Prompt for AI Agents (early access)
In .github/workflows/tests.yml around lines 20 to 30, the python-version matrix
currently includes only 3.10 and 3.13, missing 3.11 and 3.12. To ensure
comprehensive compatibility testing across all supported Python versions, add
'3.11' and '3.12' to the python-version list in the matrix section.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pyproject.toml (1)
38-38: Align entry point with PEP 621
Thescripts.cratedb-mcpkey should be moved under an explicit[project.scripts]table for PEP 621 compliance.-[project] -scripts.cratedb-mcp = "cratedb_mcp.cli:main" +[project.scripts] +cratedb-mcp = "cratedb_mcp.cli:main"
🧹 Nitpick comments (6)
README.md (3)
58-59: Wrap bare URLs in angle brackets
Markdown linting (MD034) flags bare URLs. Consider updating to:- For example, use https://example.com + For example, use <https://example.com>🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
58-58: Bare URL used
null(MD034, no-bare-urls)
59-59: Bare URL used
null(MD034, no-bare-urls)
76-76: Refine wording for clarity
LanguageTool suggests replacing “not able to” with “unable to”:- Note: If you are not able to use `uv tool install` + Note: If you are unable to use `uv tool install`🧰 Tools
🪛 LanguageTool
[style] ~76-~76: As a shorter alternative for ‘able to’, consider using “can not”.
Context: ...NSPORT=sse cratedb-mcp ``` Note: If you are not able to useuv tool install, please use `uv r...(BE_ABLE_TO)
117-119: Remove unused link references
Static analysis (MD053) reports unused definitions forbadge-issuesandbadge-release-notes. They can be removed:-[badge-issues]: https://img.shields.io/github/issues/crate/cratedb-mcp -[badge-release-notes]: https://img.shields.io/badge/Release%20Notes-v0.0.0-blue🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
117-117: Link and image reference definitions should be needed
Unused link or image reference definition: "badge-issues"(MD053, link-image-reference-definitions)
119-119: Link and image reference definitions should be needed
Unused link or image reference definition: "badge-release-notes"(MD053, link-image-reference-definitions)
DEVELOP.md (2)
23-25: Simplify phrasing
LanguageTool suggests using “To” instead of “In order to”:- In order to invoke individual software tests for working on the spot + To invoke individual software tests on the spot🧰 Tools
🪛 LanguageTool
[style] ~23-~23: Consider a more concise word here.
Context: ...used on CI/GHA.shell poe checkIn order to invoke individual software tests for wo...(IN_ORDER_TO_PREMIUM)
35-44: Refine punctuation before “because”
Remove the comma before “because” to comply with style guidelines:- so you don't need to do any version bumping within files, because + so you don't need to do any version bumping within files because🧰 Tools
🪛 LanguageTool
[formatting] ~36-~36: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...d to do any version bumping within files, because the version number will be derived from...(COMMA_BEFORE_BECAUSE)
pyproject.toml (1)
140-143: Clean up commented task snippets
Commented-out commands (e.g.,ruff format --fix) might confuse users. Consider removing or moving them to documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/release-pypi.yml(1 hunks)CHANGES.md(1 hunks)DEVELOP.md(1 hunks)README.md(4 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGES.md
- .github/workflows/release-pypi.yml
🧰 Additional context used
🪛 LanguageTool
DEVELOP.md
[style] ~23-~23: Consider a more concise word here.
Context: ...used on CI/GHA. shell poe check In order to invoke individual software tests for wo...
(IN_ORDER_TO_PREMIUM)
[formatting] ~36-~36: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...d to do any version bumping within files, because the version number will be derived from...
(COMMA_BEFORE_BECAUSE)
README.md
[style] ~76-~76: As a shorter alternative for ‘able to’, consider using “can not”.
Context: ...NSPORT=sse cratedb-mcp ``` Note: If you are not able to use uv tool install, please use `uv r...
(BE_ABLE_TO)
🪛 markdownlint-cli2 (0.17.2)
README.md
58-58: Bare URL used
null
(MD034, no-bare-urls)
59-59: Bare URL used
null
(MD034, no-bare-urls)
117-117: Link and image reference definitions should be needed
Unused link or image reference definition: "badge-issues"
(MD053, link-image-reference-definitions)
119-119: Link and image reference definitions should be needed
Unused link or image reference definition: "badge-release-notes"
(MD053, link-image-reference-definitions)
🔇 Additional comments (18)
README.md (3)
6-7: Great addition: CI & Coverage badges
Including badges for GitHub Actions and Codecov provides immediate visibility into the project’s health.
48-50: Installation instructions updated
Recommendinguv tool install …for user-wide installation is clear and aligns with the project's preferred tooling.
98-100: Excellent: Added Development section
Pointing contributors toDEVELOP.mdstrengthens onboarding for local development and CI workflows.DEVELOP.md (3)
3-13: Clear sandbox setup instructions
The steps to clone, create a venv, and install in editable mode usinguv venvanduv pipare straightforward and align with the project’s tooling.
17-22: poethepoet and pytest usage documented
Showing both thepoe checkumbrella command and directpytestexamples covers common developer workflows.
27-31: Test invocation examples
Providing both file-specific and marker-based pytest commands is helpful for targeted testing.pyproject.toml (12)
1-6: Build system configuration is correct
The[build-system]section properly specifiessetuptools.build_metawith required versions andversioningit.
12-18: Python classifiers accurately reflect supported versions
Theclassifierslist covers Python 3.10 through 3.13, and theProgramming Language :: Python :: 3 :: Onlyclassifier clarifies the package’s scope.
19-21: Dynamic versioning is enabled
Addingdynamic = ["version"]offloads version management toversioningit, eliminating manual bumps.
23-24: Core dependency specification looks good
Lockingmcp[cli]>=1.5.0ensures we use the CLI extras correctly.
25-31: Development dependencies are comprehensive
Includingmypy,poethepoet,pyproject-fmt,ruff, andvalidate-pyprojectprovides a robust linting and formatting workflow.
32-36: Test dependencies are well-defined
Pinningpytest,pytest-cov, andpytest-mockversions supports consistent test execution across environments.
40-46: Ruff configuration is clear
Setting aline-lengthand specifying exclusions and selections aligns with project style requirements.
78-86: pytest configuration supports CI and debugging
Theaddopts,minversion, and logging settings enforce strict marker checks and verbose output, aiding both local development and CI runs.
94-104: Coverage reporting is properly scoped
Definingsourcepaths and omitting tests ensures that coverage metrics focus on production code.
112-121: Type checking settings are reasonable
Enablingcheck_untyped_defsandignore_missing_importsbalances strictness with practicality given external dependencies.
122-126: Versioningit integration is configured correctly
Usinggit-archiveanddescribe-substprovides a predictable tagging workflow for releases.
131-139: poethepoet tasks streamline workflows
Groupingcheck,format,lint,test, andreleasetasks centralizes CI and developer commands.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (1)
78-78: Fix grammar in usage note
Change “If you unable to use” to “If you are unable to use” for clarity.- Note: If you unable to use `uv tool install`, please use + Note: If you are unable to use `uv tool install`, please use🧰 Tools
🪛 LanguageTool
[grammar] ~78-~78: Did you mean “you are” or “you were”?
Context: ..._TRANSPORT=sse cratedb-mcp ``` Note: If you unable to useuv tool install, please...(PRP_ABLE_TO)
DEVELOP.md (1)
35-36: Remove unnecessary comma before ‘because’
Since the “because” clause is essential, omit the comma for proper grammar.- The project uses [versioningit] so you don't need to do any version bumping, - within files, because the version number will be derived from the Git tag. + The project uses [versioningit] so you don't need to do any version bumping + within files because the version number will be derived from the Git tag.🧰 Tools
🪛 LanguageTool
[formatting] ~36-~36: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...d to do any version bumping within files, because the version number will be derived from...(COMMA_BEFORE_BECAUSE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/release-pypi.yml(1 hunks)DEVELOP.md(1 hunks)README.md(4 hunks)cratedb_mcp/cli.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cratedb_mcp/cli.py
- .github/workflows/release-pypi.yml
🧰 Additional context used
🪛 LanguageTool
DEVELOP.md
[formatting] ~36-~36: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...d to do any version bumping within files, because the version number will be derived from...
(COMMA_BEFORE_BECAUSE)
README.md
[grammar] ~78-~78: Did you mean “you are” or “you were”?
Context: ..._TRANSPORT=sse cratedb-mcp ``` Note: If you unable to use uv tool install, please...
(PRP_ABLE_TO)
🔇 Additional comments (10)
README.md (6)
4-4: Add new project status badges
The Release Notes, CI, and Coverage badges provide quick links to essential project metrics and visibility.Also applies to: 8-9
50-54: Update installation instructions to useuv tool install
Recommendinguv tool install --upgrade git+...ensures a consistent user-wide installation method and aligns with the tooling approach introduced elsewhere in the docs.
58-67: Clarify environment variable configuration examples
Providing both CrateDB Cloud and localhost examples helps users quickly configureCRATEDB_MCP_HTTP_URLfor different deployment scenarios.
72-72: Document direct usage ofcratedb-mcp
Showing how to start the server with the defaultstdiotransport improves discoverability for end users.
100-102: Add Development section with link to detailed guide
Linking toDEVELOP.mddirects contributors to full development sandbox, testing, and release instructions.
116-124: Define new badge reference URLs
All new badge image and link definitions match the badges added above and will render correctly.DEVELOP.md (4)
1-3: Document title and sandbox section header
The “Development documentation” title and “Sandbox” header clearly introduce the setup instructions.
8-12: Provide sandbox setup commands
Cloning the repo, creating a Python 3.13 venv, and installing in editable mode ensures contributors have a reproducible environment.
21-21: Highlightpoe checkfor unified linting and testing
Includingpoe checkreinforces the CI alignment and simplifies local development checks.
27-31: Show targeted pytest invocations
Examples for running specific tests without coverage collection help speed up development iterations.
| " modify data, columns or are otherwise deemed un-safe are rejected.") | ||
| def query_sql(query: str): | ||
| if not 'select' in query.lower(): | ||
| if 'select' not in query.lower(): |
There was a problem hiding this comment.
Note: for future reference, this is very low effort way to avoid un-safe queries and very easy to bypass. We mostly depend on the LLM to understand the description, checking if 'select' is present is weak.
There was a problem hiding this comment.
Yeah, I think you've seen GH-11 already, which aims to make it a bit stronger.
About
When receiving more changes going forward, I hope you appreciate to accompany this project with a few software tests and a CI configuration?
/cc @WalBeh