docs: update CLAUDE.md for testing and build improvements #55
Conversation
WalkthroughThe CLAUDE.md documentation was extensively expanded and restructured to provide more detailed and organized information about development commands, architecture, SQL security, supported transports, configuration, CI/CD pipelines, manual testing, and build/release processes. No changes to code or exported entities were made; only documentation content was updated. Changes
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 (
|
|
@claude review PR |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
CLAUDE.md (1)
11-15: Update minimum Go version to latest stable
The docs currently pin Go to 1.24.2, but Go 1.24.4 is the latest patch release as of July 2025. To keep the requirements up to date (and avoid future doc churn on patch releases), update the minimum Go version to either the exact latest (1.24.4) or more flexibly as “Go ≥ 1.24”.• File: CLAUDE.md
Lines 11–15
Replace:- **Language:** Go 1.24.2 + **Language:** Go ≥ 1.24 # (or: Go 1.24.4)
🧹 Nitpick comments (2)
CLAUDE.md (2)
48-56: Hard-coding source line numbers is brittleRefs such as
cmd/main.go:26-134will drift after the next refactor and mislead readers.
Prefer hyperlinking to the file in GitHub or describing the responsibility without exact ranges.
78-82: Document the write-query override caveatYou note
TRINO_ALLOW_WRITE_QUERIES=true“logs warning”, but not the security impact. Add a caution block clarifying that enabling it bypasses read-only enforcement and should only be used in isolated environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CLAUDE.md(1 hunks)
🔇 Additional comments (1)
CLAUDE.md (1)
106-131: CI job names vs Make targets divergeDocs say the
verifyjob runs “go mod tidy verification”, butmake lintcomment already claims to do that. Double-check that the workflow and Makefile stay in sync to avoid stale instructions.
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
e71e2cb to
91a4874
Compare
|
@claude review PR |
|
Claude finished @tuannvm's task —— View job PR Review: docs: update CLAUDE.md for testing and build improvementsReview Tasks:
Overall AssessmentThis is an excellent documentation update that significantly improves the CLAUDE.md file. The expanded content provides comprehensive guidance for developers working with the mcp-trino codebase. The documentation is well-organized, technically accurate, and follows good practices for developer documentation. Strengths✅ Accuracy
✅ Comprehensive Coverage
✅ Organization & Structure
✅ Developer Experience
Issues Found🔧 Minor Issues
Addressing CodeRabbit FeedbackThe CodeRabbit review raised valid points that align with my findings:
Technical VerificationI verified the documentation against the actual codebase:
Recommendations for Future
ConclusionThis PR represents a significant improvement to the project documentation. The expanded CLAUDE.md will greatly benefit developers working with this codebase by providing comprehensive, accurate guidance. The minor issues identified are easily addressable and don't detract from the overall quality of this contribution. Recommendation: ✅ APPROVE - This is a valuable addition that should be merged, with the minor issues addressed in follow-up PRs if desired. --- | Branch: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
CLAUDE.md (5)
22-27: Clarify test & lint targets for better developer feedback
make testandmake lintare excellent shortcuts, but the docs don’t indicate whether coverage is enforced (-cover), nor whethergo vetis part of the lint target. Consider adding a quick note so contributors know what to expect from each command.
38-42: Collapse redundant per-package test commandsListing three nearly identical
go test ./internal/...commands is verbose and easy to drift. You could simplify to a wildcard (go test ./internal/...) unless there’s a specific reason to run them individually.
48-68: Remove hard-coded line numbers from file referencesEmbedding ranges like
cmd/main.go:26-134will rot quickly after any refactor and create confusion. Prefer section headers / function names or omit the range entirely.-1. **Main Entry Point** (`cmd/main.go:26-134`): +1. **Main Entry Point** (`cmd/main.go`):
71-75: Add payload/response examples for new HTTP endpointsThe transport section lists endpoints but omits example requests or expected SSE/event payloads. Even a minimal cURL snippet would accelerate adoption.
101-105: Connection-pool defaults: consider surfacing tunablesThe hard-coded
10/5/5mpool limits are reasonable, yet some deployments will need tweaking. A short note that these are configurable (or intentionally fixed) would pre-empt support questions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CLAUDE.md(1 hunks)
🔇 Additional comments (3)
CLAUDE.md (3)
31-31: Document port / env overrides formake run-docker
make run-dockeris handy, but the documentation doesn’t mention how the container’s ports or environment variables are exposed/overridden. A one-liner (e.g., “exposes :9097 by default; override withPORT=...”) would remove guesswork.
33-37: Verify that DXT targets exist in the MakefileThe
make build-dxtandmake pack-dxttargets are new in the docs, but a quick grep shows no Makefile context here. Please confirm these targets are implemented; if not, the docs will mislead contributors.
78-81: Edge-case write operations can still slip through CTEs
WITHclauses are allowed, but a crafty query like
WITH t AS (INSERT INTO …) SELECT * FROM t RETURNING …
bypasses a naïve “starts-with” check. Make sure the implementation inspects the full statement tree, not just the first verb; update the doc to reflect those safeguards.
Summary by CodeRabbit