Feat/add registry support #95
Conversation
Signed-off-by: Tommy Nguyen <[email protected]>
Signed-off-by: Tommy Nguyen <[email protected]>
WalkthroughThe PR replaces DXT packaging with MCPB packaging, removes the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions
participant GR as GoReleaser
participant MK as Make (pack-mcpb-from-dist)
participant FS as Filesystem
participant GHRel as GitHub Release
Dev->>GH: Push tag vX.Y.Z
GH->>GR: Run GoReleaser (build binaries to dist/)
GR-->>GH: Dist artifacts ready
GH->>MK: make pack-mcpb-from-dist (GORELEASER_VERSION=vX.Y.Z)
MK->>FS: Copy platform binaries from dist/ to mcpb-bundle/server
MK->>FS: Validate manifest.json and binaries
MK->>FS: Zip to mcp-trino.mcpb
MK-->>GH: mcp-trino.mcpb created
GH->>GHRel: gh release upload mcp-trino.mcpb --clobber
GHRel-->>GH: Upload result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
79-82: Enable race detector in tests.Guidelines call for
-race.-test: - go test ./... +test: + go test -race ./...
🧹 Nitpick comments (8)
server.json (1)
20-49: Mirror all supported env vars for consistency.Manifest and code support more envs (scheme/SSL/timeout/write queries; MCP transport/port). Expose them here for better UX:
- TRINO_SCHEME, TRINO_SSL, TRINO_SSL_INSECURE
- TRINO_ALLOW_WRITE_QUERIES, TRINO_QUERY_TIMEOUT
- MCP_TRANSPORT, MCP_PORT
I can provide a patch list if desired.
manifest.json (1)
5-5: Safer version injection.Your Makefile uses sed that may touch any
"version": "..."line. Use jq to set only the top-level field:- sed 's/"version": ".*"/"version": "$(VERSION)"/' manifest.json > mcpb-bundle/manifest.json + jq --arg v "$(VERSION)" '.version=$v' manifest.json > mcpb-bundle/manifest.jsonCLAUDE.md (1)
35-39: Docs match the new MCPB flow; add note on removed DXT targets.Since DXT targets were removed, add a short “Replaced targets” note or provide backward-compatible aliases to avoid breaking external scripts.
.github/workflows/release.yml (1)
65-75: Attach checksum for supply-chain hygiene.Generate and upload a SHA256 for
mcp-trino.mcpbto help clients verify downloads.Example:
run: | if [ -f "mcp-trino.mcpb" ]; then + shasum -a 256 mcp-trino.mcpb > mcp-trino.mcpb.sha256 gh release upload ${{ steps.tag.outputs.new_tag }} mcp-trino.mcpb --clobber + gh release upload ${{ steps.tag.outputs.new_tag }} mcp-trino.mcpb.sha256 --clobberMakefile (4)
1-1: Add ‘all’ to .PHONY.Static analysis flags missing phony for the existing
alltarget.-.PHONY: build build-platform-binaries build-mcpb pack-mcpb-from-dist test clean run-dev release-snapshot run-docker run docker-compose-up docker-compose-down lint docker-test +.PHONY: all build build-platform-binaries build-mcpb pack-mcpb-from-dist test clean run-dev release-snapshot run-docker run docker-compose-up docker-compose-down lint docker-test
33-36: Prefer jq over sed for version update.Avoid accidental edits and handle JSON safely.
- # Update version in manifest.json to match build version - sed 's/"version": ".*"/"version": "$(VERSION)"/' manifest.json > mcpb-bundle/manifest.json + # Update version in manifest.json to match build version + jq --arg v "$(VERSION)" '.version=$v' manifest.json > mcpb-bundle/manifest.json
109-116: Optional: prefer ‘docker compose’.Newer Docker recommends
docker compose. Keep as-is if CI/env relies on legacy binary.
1-1: Backward-compatibility aliases for removed DXT targets (optional).If any scripts still call old targets, add no-op shims:
+.PHONY: build-dxt pack-dxt +build-dxt: + @echo "build-dxt is deprecated; use 'make build-mcpb'"; $(MAKE) build-mcpb +pack-dxt: + @echo "pack-dxt is deprecated; use 'make pack-mcpb-from-dist'"; $(MAKE) pack-mcpb-from-dist
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.dxtignore(0 hunks).github/workflows/release.yml(3 hunks)CLAUDE.md(1 hunks)Makefile(4 hunks)manifest.json(3 hunks)server.json(1 hunks)
💤 Files with no reviewable changes (1)
- .dxtignore
🧰 Additional context used
📓 Path-based instructions (1)
Makefile
📄 CodeRabbit inference engine (CLAUDE.md)
Makefile: Inject version via ldflags (-X main.Version=$(VERSION)) in build/release targets
Provide development targets with described behavior: build (to ./bin/mcp-trino), test (with -race), run-dev (go run ./cmd), run, clean, lint (golangci-lint + go mod tidy), docker-compose-up/down, run-docker, release-snapshot, build-dxt, pack-dxt
Files:
Makefile
🧠 Learnings (2)
📚 Learning: 2025-09-01T14:40:22.612Z
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-01T14:40:22.612Z
Learning: Applies to Makefile : Provide development targets with described behavior: build (to ./bin/mcp-trino), test (with -race), run-dev (go run ./cmd), run, clean, lint (golangci-lint + go mod tidy), docker-compose-up/down, run-docker, release-snapshot, build-dxt, pack-dxt
Applied to files:
CLAUDE.mdMakefile
📚 Learning: 2025-09-01T14:40:22.612Z
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-01T14:40:22.612Z
Learning: Applies to .github/workflows/build.yml : Build/Release must use GoReleaser for multi-platform builds, publish Docker images to GHCR, auto-release on main pushes, and generate SLSA provenance
Applied to files:
.github/workflows/release.ymlMakefile
🧬 Code graph analysis (1)
server.json (2)
internal/mcp/server.go (1)
mcpServer(23-28)cmd/main.go (1)
main(21-74)
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
⏰ 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). (1)
- GitHub Check: Build Verification
🔇 Additional comments (3)
server.json (1)
10-16: Keep version synchronized automatically.Makefile updates manifest.json but not server.json. Add the same version injection here (e.g., with jq) during build to keep 2.2.x in lockstep.
manifest.json (1)
131-135: Tool registration looks good.New
explain_querytool is clearly described and consistent with docs.Makefile (1)
13-22: Nice: ldflags version injection across platforms.Cross-compile matrix and chmod are correct for local packaging.
| - name: Create MCPB bundle | ||
| if: steps.tag.outputs.new_tag != '' | ||
| run: | | ||
| GORELEASER_VERSION=${{ steps.tag.outputs.new_tag }} make pack-mcpb-from-dist | ||
| ls -la mcp-trino.mcpb || (echo "MCPB bundle not found" && exit 1) | ||
There was a problem hiding this comment.
Strip leading ‘v’ before writing manifest version.
steps.tag.outputs.new_tag includes the ‘v’ (e.g., v2.2.3). Your Makefile writes it verbatim, producing "version": "v2.2.3". Strip it here:
- GORELEASER_VERSION=${{ steps.tag.outputs.new_tag }} make pack-mcpb-from-dist
+ CLEAN_TAG=${{ steps.tag.outputs.new_tag }}; CLEAN_TAG="${CLEAN_TAG#v}"; \
+ GORELEASER_VERSION="$CLEAN_TAG" make pack-mcpb-from-dist📝 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.
| - name: Create MCPB bundle | |
| if: steps.tag.outputs.new_tag != '' | |
| run: | | |
| GORELEASER_VERSION=${{ steps.tag.outputs.new_tag }} make pack-mcpb-from-dist | |
| ls -la mcp-trino.mcpb || (echo "MCPB bundle not found" && exit 1) | |
| - name: Create MCPB bundle | |
| if: steps.tag.outputs.new_tag != '' | |
| run: | | |
| CLEAN_TAG=${{ steps.tag.outputs.new_tag }}; CLEAN_TAG="${CLEAN_TAG#v}"; \ | |
| GORELEASER_VERSION="$CLEAN_TAG" make pack-mcpb-from-dist | |
| ls -la mcp-trino.mcpb || (echo "MCPB bundle not found" && exit 1) |
🤖 Prompt for AI Agents
In .github/workflows/release.yml around lines 59-64, the workflow passes
steps.tag.outputs.new_tag (e.g., "v2.2.3") directly into GORELEASER_VERSION so
the manifest gets "version": "v2.2.3"; strip any leading "v" before writing the
manifest by normalizing the tag value (e.g., remove a leading "v" via bash
parameter expansion or a GitHub expression) and pass the stripped value into
GORELEASER_VERSION when invoking make pack-mcpb-from-dist so the manifest
contains "version": "2.2.3".
| for platform in "linux_amd64" "darwin_amd64" "darwin_arm64" "windows_amd64"; do \ | ||
| found=$$(find dist -path "*_$${platform}_*" -name "$(BINARY_NAME)*" -type f | head -1); \ | ||
| if [ -n "$$found" ]; then \ | ||
| case "$$platform" in \ | ||
| "linux_amd64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-linux-amd64" ;; \ | ||
| "darwin_amd64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-darwin-amd64" ;; \ | ||
| "darwin_arm64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-darwin-arm64" ;; \ | ||
| "windows_amd64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-windows-amd64.exe" ;; \ | ||
| esac; \ | ||
| echo "Copied $$found -> $(BINARY_NAME)-$$platform"; \ | ||
| found_count=$$((found_count + 1)); \ | ||
| else \ | ||
| echo "Warning: No binary found for $$platform"; \ | ||
| fi; \ |
There was a problem hiding this comment.
Robust artifact selection; avoid copying archives.
Current find may grab .tar.gz/.zip instead of binaries. Filter out archives and ensure executability after copy.
- found=$$(find dist -path "*_$${platform}_*" -name "$(BINARY_NAME)*" -type f | head -1); \
+ found=$$(find dist -path "*_$${platform}_*" -type f \
+ \( -name "$(BINARY_NAME)" -o -name "$(BINARY_NAME).exe" -o -name "$(BINARY_NAME)-*" \) \
+ ! -name "*.tar.gz" ! -name "*.tgz" ! -name "*.zip" | head -1); \
@@
- "linux_amd64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-linux-amd64" ;; \
+ "linux_amd64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-linux-amd64" ;; \
@@
esac; \
+ chmod +x mcpb-bundle/server/$(BINARY_NAME)-* 2>/dev/null || true; \📝 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.
| for platform in "linux_amd64" "darwin_amd64" "darwin_arm64" "windows_amd64"; do \ | |
| found=$$(find dist -path "*_$${platform}_*" -name "$(BINARY_NAME)*" -type f | head -1); \ | |
| if [ -n "$$found" ]; then \ | |
| case "$$platform" in \ | |
| "linux_amd64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-linux-amd64" ;; \ | |
| "darwin_amd64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-darwin-amd64" ;; \ | |
| "darwin_arm64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-darwin-arm64" ;; \ | |
| "windows_amd64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-windows-amd64.exe" ;; \ | |
| esac; \ | |
| echo "Copied $$found -> $(BINARY_NAME)-$$platform"; \ | |
| found_count=$$((found_count + 1)); \ | |
| else \ | |
| echo "Warning: No binary found for $$platform"; \ | |
| fi; \ | |
| for platform in "linux_amd64" "darwin_amd64" "darwin_arm64" "windows_amd64"; do \ | |
| found=$$(find dist -path "*_$${platform}_*" -type f \ | |
| \( -name "$(BINARY_NAME)" -o -name "$(BINARY_NAME).exe" -o -name "$(BINARY_NAME)-*" \) \ | |
| ! -name "*.tar.gz" ! -name "*.tgz" ! -name "*.zip" | head -1); \ | |
| if [ -n "$$found" ]; then \ | |
| case "$$platform" in \ | |
| "linux_amd64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-linux-amd64" ;; \ | |
| "darwin_amd64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-darwin-amd64" ;; \ | |
| "darwin_arm64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-darwin-arm64" ;; \ | |
| "windows_amd64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-windows-amd64.exe" ;; \ | |
| esac; \ | |
| chmod +x mcpb-bundle/server/$(BINARY_NAME)-* 2>/dev/null || true; \ | |
| echo "Copied $$found -> $(BINARY_NAME)-$$platform"; \ | |
| found_count=$$((found_count + 1)); \ | |
| else \ | |
| echo "Warning: No binary found for $$platform"; \ | |
| fi; \ |
🤖 Prompt for AI Agents
In Makefile around lines 45 to 58, the find invocation can match archive files
(e.g. .tar.gz, .zip) instead of actual binaries; update the find filter to
exclude common archive extensions and prefer executable files (use find
predicates like -not -name '*.tar.gz' -not -name '*.zip' and/or -executable or
-perm to ensure executables are selected), keep the same platform case mapping
for destination names, and after copying set executable permission (chmod +x)
for non-Windows targets so copied artifacts are runnable; preserve the warning
when none found and increment found_count as before.
Makefile
Outdated
| sed 's/"version": ".*"/"version": "$(GORELEASER_VERSION)"/' manifest.json > mcpb-bundle/manifest.json; \ | ||
| else \ | ||
| sed 's/"version": ".*"/"version": "$(VERSION)"/' manifest.json > mcpb-bundle/manifest.json; \ | ||
| fi | ||
| cd mcpb-bundle && zip -r ../$(BINARY_NAME).mcpb . && cd .. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize tag before injecting version.
Prevent "version": "vX.Y.Z":
- @if [ -n "$(GORELEASER_VERSION)" ]; then \
- sed 's/"version": ".*"/"version": "$(GORELEASER_VERSION)"/' manifest.json > mcpb-bundle/manifest.json; \
+ @if [ -n "$(GORELEASER_VERSION)" ]; then \
+ ver_no_v=$$(printf '%s' "$(GORELEASER_VERSION)" | sed 's/^v//'); \
+ jq --arg v "$$ver_no_v" '.version=$$v' manifest.json > mcpb-bundle/manifest.json; \
else \
- sed 's/"version": ".*"/"version": "$(VERSION)"/' manifest.json > mcpb-bundle/manifest.json; \
+ jq --arg v "$(VERSION)" '.version=$$v' manifest.json > mcpb-bundle/manifest.json; \
fi🤖 Prompt for AI Agents
In Makefile around lines 70 to 74, the recipe injects GORELEASER_VERSION or
VERSION into manifest.json but can produce a value with a leading "v" (e.g.
"v1.2.3"); normalize the tag first by stripping any leading "v" before
substituting into manifest.json—compute a sanitized variable (prefer shell
parameter expansion or a small sed/tr command) that removes a leading 'v' from
GORELEASER_VERSION or VERSION and use that sanitized value in the sed
substitution so the resulting manifest.json contains "version": "1.2.3".
| "entry_point": { | ||
| "darwin-arm64": "server/mcp-trino-darwin-arm64", | ||
| "darwin-amd64": "server/mcp-trino-darwin-amd64", | ||
| "linux": "server/mcp-trino-linux-amd64", | ||
| "win32": "server/mcp-trino-windows-amd64.exe" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize platform keys to match produced binaries.
You ship linux-amd64 and windows-amd64. Prefer explicit keys for symmetry and future arm64:
- "darwin-arm64": "server/mcp-trino-darwin-arm64",
- "darwin-amd64": "server/mcp-trino-darwin-amd64",
- "linux": "server/mcp-trino-linux-amd64",
- "win32": "server/mcp-trino-windows-amd64.exe"
+ "darwin-arm64": "server/mcp-trino-darwin-arm64",
+ "darwin-amd64": "server/mcp-trino-darwin-amd64",
+ "linux-amd64": "server/mcp-trino-linux-amd64",
+ "win32-amd64": "server/mcp-trino-windows-amd64.exe"Align any client-side resolver accordingly.
📝 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.
| "entry_point": { | |
| "darwin-arm64": "server/mcp-trino-darwin-arm64", | |
| "darwin-amd64": "server/mcp-trino-darwin-amd64", | |
| "linux": "server/mcp-trino-linux-amd64", | |
| "win32": "server/mcp-trino-windows-amd64.exe" | |
| }, | |
| "entry_point": { | |
| "darwin-arm64": "server/mcp-trino-darwin-arm64", | |
| "darwin-amd64": "server/mcp-trino-darwin-amd64", | |
| "linux-amd64": "server/mcp-trino-linux-amd64", | |
| "win32-amd64": "server/mcp-trino-windows-amd64.exe" | |
| }, |
🤖 Prompt for AI Agents
In manifest.json around lines 25 to 30, the platform keys are inconsistent with
produced binary names; change "linux" to "linux-amd64" and "win32" to
"windows-amd64" so keys explicitly match shipped binaries (keep darwin-arm64 and
darwin-amd64 as-is), and update any client-side resolver/lookup logic to use
these normalized keys (linux-amd64, windows-amd64, darwin-amd64, darwin-arm64)
when selecting the entry_point.
| "registry_type": "mcpb", | ||
| "identifier": "https://github.com/tuannvm/mcp-trino/releases/download/v2.2.1/mcp-trino_2.2.1_darwin_arm64.tar.gz", | ||
| "version": "2.2.1", | ||
| "file_sha256": "1a18882ab43243e17420f3562118a4c3e7fff12bc6b145066ae64b90b2dc0159", | ||
| "transport": { | ||
| "type": "stdio" | ||
| }, |
There was a problem hiding this comment.
Use the MCPB bundle, not a single-platform tarball.
Pointing identifier at a darwin-arm64 tar.gz makes installs non-portable and bypasses the MCPB you build in CI. Publish and reference the cross-platform .mcpb bundle instead, and keep file_sha256 in sync.
Example:
- "identifier": "https://github.com/tuannvm/mcp-trino/releases/download/v2.2.1/mcp-trino_2.2.1_darwin_arm64.tar.gz",
- "version": "2.2.1",
- "file_sha256": "1a18882ab43243e17420f3562118a4c3e7fff12bc6b145066ae64b90b2dc0159",
+ "identifier": "https://github.com/tuannvm/mcp-trino/releases/download/v2.2.1/mcp-trino.mcpb",
+ "version": "2.2.1",
+ "file_sha256": "<sha256-of-mcpb>",Also consider updating this file from Makefile during packaging to avoid drift.
📝 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.
| "registry_type": "mcpb", | |
| "identifier": "https://github.com/tuannvm/mcp-trino/releases/download/v2.2.1/mcp-trino_2.2.1_darwin_arm64.tar.gz", | |
| "version": "2.2.1", | |
| "file_sha256": "1a18882ab43243e17420f3562118a4c3e7fff12bc6b145066ae64b90b2dc0159", | |
| "transport": { | |
| "type": "stdio" | |
| }, | |
| "registry_type": "mcpb", | |
| - "identifier": "https://github.com/tuannvm/mcp-trino/releases/download/v2.2.1/mcp-trino_2.2.1_darwin_arm64.tar.gz", | |
| - "version": "2.2.1", | |
| "identifier": "https://github.com/tuannvm/mcp-trino/releases/download/v2.2.1/mcp-trino.mcpb", | |
| "version": "2.2.1", | |
| "file_sha256": "<sha256-of-mcpb>", | |
| "transport": { | |
| "type": "stdio" | |
| }, |
🤖 Prompt for AI Agents
In server.json around lines 13 to 19, the identifier currently points at a
darwin_arm64 tar.gz which makes installs platform-specific; change the
identifier to reference the cross-platform .mcpb bundle you publish from CI,
update the file_sha256 to the SHA256 of that .mcpb file, and keep registry_type
as "mcpb"; also modify the packaging/Makefile to write this server.json entry
automatically during release so the identifier and file_sha256 never drift.
Signed-off-by: Tommy Nguyen <[email protected]>
Signed-off-by: Tommy Nguyen <[email protected]>
Signed-off-by: Tommy Nguyen <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
80-82: Run tests with race detector.Add -race to catch data races early.
-test: - go test ./... +test: + go test -race ./...
♻️ Duplicate comments (3)
manifest.json (1)
25-30: Entry-point mapping looks good; aligns with shipped binaries.The linux-amd64 and win32-amd64 keys resolve the earlier inconsistency.
Makefile (2)
39-58: Find may copy archives; filter and ensure executability.Exclude .tar.gz/.zip and prefer actual binaries; set +x after copy.
- for platform in "linux_amd64" "darwin_amd64" "darwin_arm64" "windows_amd64"; do \ - found=$$(find dist -path "*_$${platform}_*" -name "$(BINARY_NAME)*" -type f | head -1); \ + for platform in "linux_amd64" "darwin_amd64" "darwin_arm64" "windows_amd64"; do \ + found=$$(find dist -path "*_$${platform}_*" -type f \ + \( -name "$(BINARY_NAME)" -o -name "$(BINARY_NAME).exe" -o -name "$(BINARY_NAME)-*" \) \ + ! -name "*.tar.gz" ! -name "*.tgz" ! -name "*.zip" | head -1); \ if [ -n "$$found" ]; then \ case "$$platform" in \ "linux_amd64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-linux-amd64" ;; \ "darwin_amd64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-darwin-amd64" ;; \ "darwin_arm64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-darwin-arm64" ;; \ "windows_amd64") cp "$$found" "mcpb-bundle/server/$(BINARY_NAME)-windows-amd64.exe" ;; \ esac; \ + chmod +x mcpb-bundle/server/$(BINARY_NAME)-* 2>/dev/null || true; \
70-74: Normalize tag before injecting version; prefer jq.Prevent "version": "vX.Y.Z".
- @if [ -n "$(GORELEASER_VERSION)" ]; then \ - sed -e 's/"version": ".*"/"version": "$(GORELEASER_VERSION)"/' -e 's/"name": "$(BINARY_NAME)"/"name": "$(BINARY_NAME)"/' manifest.json > mcpb-bundle/manifest.json; \ - else \ - sed -e 's/"version": ".*"/"version": "$(VERSION)"/' -e 's/"name": "$(BINARY_NAME)"/"name": "$(BINARY_NAME)"/' manifest.json > mcpb-bundle/manifest.json; \ - fi + ver_src=$${GORELEASER_VERSION:-$(VERSION)}; \ + ver_no_v=$$(printf '%s' "$$ver_src" | sed 's/^v//'); \ + jq --arg v "$$ver_no_v" --arg n "$(BINARY_NAME)" '.version=$$v | .name=$$n' \ + manifest.json > mcpb-bundle/manifest.json
🧹 Nitpick comments (4)
manifest.json (2)
20-21: Align compatibility.platforms with entry_point keys (or confirm schema).Platforms list uses generic ["darwin","win32","linux"] while entry_point keys are arch-specific. Either:
- keep generic list but confirm the registry/client doesn’t derive keys from it, or
- switch to ["darwin-amd64","darwin-arm64","linux-amd64","win32-amd64"] for symmetry.
131-135: Ensure tool is implemented and documented.explain_query is declared—confirm the server exposes it (handler, validation, errors) and add a short usage snippet to README.
Makefile (2)
1-1: Add 'all' to .PHONY (quiet checkmake) and keep legacy aliases.
- all target exists but isn’t phony.
- Consider keeping build-dxt/pack-dxt as aliases to avoid breaking scripts.
-.PHONY: build build-platform-binaries build-mcpb pack-mcpb-from-dist test clean run-dev release-snapshot run-docker run docker-compose-up docker-compose-down lint docker-test +.PHONY: all build build-platform-binaries build-mcpb pack-mcpb-from-dist test clean run-dev release-snapshot run-docker run docker-compose-up docker-compose-down lint docker-test build-dxt pack-dxt + +# Back-compat aliases for removed DXT flow +build-dxt: ## Alias -> build-mcpb + @$(MAKE) build-mcpb +pack-dxt: ## Alias -> pack-mcpb-from-dist + @$(MAKE) pack-mcpb-from-dist
4-4: Harden BINARY_NAME resolution with default fallback.git remote may be unavailable in some CI contexts; default to mcp-trino.
-BINARY_NAME ?= $(shell basename $(shell git remote get-url origin) .git | sed 's/.*\///') +BINARY_NAME ?= $(shell basename $$(git remote get-url origin 2>/dev/null || echo mcp-trino) .git | sed 's/.*\///')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/release.yml(3 hunks)Makefile(4 hunks)manifest.json(3 hunks)server.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server.json
- .github/workflows/release.yml
🧰 Additional context used
📓 Path-based instructions (1)
Makefile
📄 CodeRabbit inference engine (CLAUDE.md)
Makefile: Inject version via ldflags (-X main.Version=$(VERSION)) in build/release targets
Provide development targets with described behavior: build (to ./bin/mcp-trino), test (with -race), run-dev (go run ./cmd), run, clean, lint (golangci-lint + go mod tidy), docker-compose-up/down, run-docker, release-snapshot, build-dxt, pack-dxt
Files:
Makefile
🧠 Learnings (3)
📚 Learning: 2025-09-01T14:40:22.612Z
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-01T14:40:22.612Z
Learning: Applies to Makefile : Provide development targets with described behavior: build (to ./bin/mcp-trino), test (with -race), run-dev (go run ./cmd), run, clean, lint (golangci-lint + go mod tidy), docker-compose-up/down, run-docker, release-snapshot, build-dxt, pack-dxt
Applied to files:
Makefile
📚 Learning: 2025-09-01T14:40:22.612Z
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-01T14:40:22.612Z
Learning: Applies to Makefile : Inject version via ldflags (-X main.Version=$(VERSION)) in build/release targets
Applied to files:
Makefile
📚 Learning: 2025-09-01T14:40:22.612Z
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-01T14:40:22.612Z
Learning: Applies to .github/workflows/build.yml : Build/Release must use GoReleaser for multi-platform builds, publish Docker images to GHCR, auto-release on main pushes, and generate SLSA provenance
Applied to files:
Makefile
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
🔇 Additional comments (4)
manifest.json (1)
2-2: Confirm manifest schema version."manifest_version": "0.1"—verify this matches the registry’s expected schema for MCPB manifests.
Makefile (3)
13-21: Cross-compile target: solid use of ldflags and outputs.Produces the 4 platforms with version injection; chmod is helpful for non-Windows.
95-99: Nice integration: pack MCPB after snapshot releases.Keeps artifacts coherent with registry packaging.
101-103: run depends on build: good DX.Ensures a fresh binary before running.
| # Build MCPB bundle | ||
| build-mcpb: build-platform-binaries | ||
| @echo "Creating MCPB bundle..." | ||
| @if [ ! -f "manifest.json" ]; then \ | ||
| echo "Error: manifest.json not found"; \ | ||
| exit 1; \ | ||
| fi | ||
| mkdir -p mcpb-bundle/server | ||
| cp server/* mcpb-bundle/server/ | ||
| # Update version and name in manifest.json to match build version and binary name | ||
| sed -e 's/"version": ".*"/"version": "$(VERSION)"/' -e 's/"name": "$(BINARY_NAME)"/"name": "$(BINARY_NAME)"/' manifest.json > mcpb-bundle/manifest.json | ||
| cd mcpb-bundle && zip -r ../$(BINARY_NAME).mcpb . && cd .. | ||
| rm -rf mcpb-bundle | ||
| @echo "MCPB bundle created: $(BINARY_NAME).mcpb" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sed-based JSON patching is brittle; name replacement is a no-op.
The sed for "name" won’t match (it looks for literal "$(BINARY_NAME)"), and generic s/".*"/ risks touching unintended fields. Use jq and strip leading v from tags.
- # Update version and name in manifest.json to match build version and binary name
- sed -e 's/"version": ".*"/"version": "$(VERSION)"/' -e 's/"name": "$(BINARY_NAME)"/"name": "$(BINARY_NAME)"/' manifest.json > mcpb-bundle/manifest.json
+ # Update version and name via jq; strip leading 'v' if present
+ ver_no_v=$$(printf '%s' "$(VERSION)" | sed 's/^v//'); \
+ jq --arg v "$$ver_no_v" --arg n "$(BINARY_NAME)" \
+ '.version=$$v | .name=$$n' manifest.json > mcpb-bundle/manifest.json📝 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.
| # Build MCPB bundle | |
| build-mcpb: build-platform-binaries | |
| @echo "Creating MCPB bundle..." | |
| @if [ ! -f "manifest.json" ]; then \ | |
| echo "Error: manifest.json not found"; \ | |
| exit 1; \ | |
| fi | |
| mkdir -p mcpb-bundle/server | |
| cp server/* mcpb-bundle/server/ | |
| # Update version and name in manifest.json to match build version and binary name | |
| sed -e 's/"version": ".*"/"version": "$(VERSION)"/' -e 's/"name": "$(BINARY_NAME)"/"name": "$(BINARY_NAME)"/' manifest.json > mcpb-bundle/manifest.json | |
| cd mcpb-bundle && zip -r ../$(BINARY_NAME).mcpb . && cd .. | |
| rm -rf mcpb-bundle | |
| @echo "MCPB bundle created: $(BINARY_NAME).mcpb" | |
| # Build MCPB bundle | |
| build-mcpb: build-platform-binaries | |
| @echo "Creating MCPB bundle..." | |
| @if [ ! -f "manifest.json" ]; then \ | |
| echo "Error: manifest.json not found"; \ | |
| exit 1; \ | |
| fi | |
| mkdir -p mcpb-bundle/server | |
| cp server/* mcpb-bundle/server/ | |
| # Update version and name via jq; strip leading 'v' if present | |
| ver_no_v=$$(printf '%s' "$(VERSION)" | sed 's/^v//'); \ | |
| jq --arg v "$$ver_no_v" --arg n "$(BINARY_NAME)" \ | |
| '.version=$$v | .name=$$n' manifest.json > mcpb-bundle/manifest.json | |
| cd mcpb-bundle && zip -r ../$(BINARY_NAME).mcpb . && cd .. | |
| rm -rf mcpb-bundle | |
| @echo "MCPB bundle created: $(BINARY_NAME).mcpb" |
🤖 Prompt for AI Agents
In Makefile around lines 24 to 38, the sed-based JSON edits are brittle and the
name replacement is a no-op because it looks for the literal "$(BINARY_NAME)";
replace the sed step with a jq-based rewrite that safely updates the
manifest.json "name" and "version" fields: set .name to the shell-expanded
$(BINARY_NAME) and set .version to $(VERSION) after stripping a leading "v" if
present (do the strip in shell before passing to jq or use jq sub to remove a
leading "v"), write the modified JSON into mcpb-bundle/manifest.json and
continue bundling as before.
Summary by CodeRabbit
New Features
Changes
Chores