feat: add binary download and caching system with sigstore verification #217
Conversation
WalkthroughReplaces embedded server binaries with a download-and-cache system: build resolves a server version (flag or auto-resolve "latest" for dev), downloads GitHub release ZIPs, verifies binaries with Sigstore bundles, caches per-version/platform binaries with SHA256, supports offline fallback, and integrates with the ImageBuilder constructor. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as genmcp CLI
participant Resolver as VersionResolver
participant Downloader as BinaryDownloader
participant Cache as BinaryCache
participant GitHub as GitHubReleases
participant Sigstore as SigstoreVerifier
participant Builder as ImageBuilder
User->>CLI: Run `genmcp build` [--server-version X]
CLI->>Resolver: Resolve server version (flag | CLI version | "latest" for dev)
Resolver-->>CLI: server_version
CLI->>Downloader: GetBinary(server_version, os, arch)
Downloader->>Cache: Get(server_version, platform)
alt Cache hit
Cache-->>Downloader: cached_path
else Cache miss
Downloader->>GitHub: Download release ZIP + sigstore bundle
GitHub-->>Downloader: zip_path, bundle_path
Downloader->>Downloader: extract platform binary from ZIP
Downloader->>Sigstore: VerifyBlob(binary_path, bundle_path)
alt Verified
Sigstore-->>Downloader: ok
Downloader->>Cache: Add(version, platform, binary_path)
Cache-->>Downloader: cached_path
else Verification failed
Sigstore-->>Downloader: verification_error
Downloader->>Cache: GetLatestCached(platform)
alt Fallback found
Cache-->>Downloader: fallback_path
else
Downloader-->>CLI: error
end
end
end
Downloader-->>CLI: binary_path
CLI->>Builder: New(push, server_version, verbose)
Builder->>Downloader: ExtractServerBinary(platform)
Builder->>Builder: Build image using binary
Builder-->>CLI: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
nader-ziada
force-pushed
the
download-server
branch
from
4b6f52b to
61897ff
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
pkg/builder/download_binary_provider_test.go (1)
35-53: Consider expanding test coverage.The test creates a mock binary file but doesn't actually exercise
ExtractServerBinary. While avoiding network calls in unit tests is appropriate, consider adding tests with a mockBinaryDownloaderto verify the provider's logic independently of network operations.pkg/cli/utils/binary_cache_test.go (1)
14-14: Explicitly handle the cleanup error.The deferred
os.RemoveAllerror should be explicitly ignored or checked. In test cleanup contexts, it's acceptable to ignore the error, but make it explicit.Apply this diff:
- defer os.RemoveAll(tmpDir) + defer func() { _ = os.RemoveAll(tmpDir) }()pkg/cli/utils/binary_downloader.go (2)
76-89: Explicitly handle cleanup error.The deferred
os.RemoveAll(tempDir)should explicitly ignore or check the error return value.Apply this diff:
- defer os.RemoveAll(tempDir) // Clean up after caching + defer func() { _ = os.RemoveAll(tempDir) }() // Clean up after caching
154-192: Handle deferred close errors.The deferred
Close()calls on lines 159 and 175 should explicitly handle or ignore their error returns.Apply this diff:
r, err := zip.OpenReader(zipPath) if err != nil { return "", err } - defer r.Close() + defer func() { _ = r.Close() }() expectedName := fmt.Sprintf("genmcp-server-%s-%s", goos, goarch) if goos == "windows" { @@ -172,7 +172,7 @@ rc, err := f.Open() if err != nil { return "", err } - defer rc.Close() + defer func() { _ = rc.Close() }()Otherwise, the extraction logic is solid: it correctly handles platform-specific binary names (including
.exefor Windows) and preserves file permissions.pkg/cli/utils/binary_cache.go (2)
155-182: Handle deferred close error.The checksum calculation logic is correct, but the deferred
f.Close()should explicitly handle or ignore the error return.Apply this diff:
f, err := os.Open(path) if err != nil { return "", err } - defer f.Close() + defer func() { _ = f.Close() }()Otherwise, the SHA-256 checksum logic is sound and the verification properly detects mismatches.
185-208: Handle deferred close errors.The file copy logic correctly preserves permissions, but the deferred
Close()calls should explicitly handle or ignore error returns.Apply this diff:
source, err := os.Open(src) if err != nil { return err } - defer source.Close() + defer func() { _ = source.Close() }() destination, err := os.Create(dst) if err != nil { return err } - defer destination.Close() + defer func() { _ = destination.Close() }()Otherwise, the copy implementation is solid with proper permission preservation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumhack/jsonschemagen/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
CHANGELOG.md(1 hunks)README.md(2 hunks)docs/commands.md(2 hunks)go.mod(5 hunks)hack/jsonschemagen/go.mod(1 hunks)pkg/builder/builder.go(1 hunks)pkg/builder/download_binary_provider.go(1 hunks)pkg/builder/download_binary_provider_test.go(1 hunks)pkg/cli/build.go(4 hunks)pkg/cli/root.go(1 hunks)pkg/cli/utils/binary_cache.go(1 hunks)pkg/cli/utils/binary_cache_test.go(1 hunks)pkg/cli/utils/binary_downloader.go(1 hunks)pkg/cli/utils/sigstore_verifier.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use the server-only logger for any logs containing sensitive information; choose between the MCPServer-embedded logger (via logging.BaseFromContext(ctx)) and the shared server/client logger appropriately
Files:
pkg/cli/root.gopkg/builder/download_binary_provider_test.gopkg/builder/download_binary_provider.gopkg/builder/builder.gopkg/cli/utils/sigstore_verifier.gopkg/cli/utils/binary_cache_test.gopkg/cli/build.gopkg/cli/utils/binary_cache.gopkg/cli/utils/binary_downloader.go
⚙️ CodeRabbit configuration file
**/*.go: Review Go code for:
- Proper error handling
- Resource cleanup (defer statements)
- Potential race conditions
- Performance implications
Files:
pkg/cli/root.gopkg/builder/download_binary_provider_test.gopkg/builder/download_binary_provider.gopkg/builder/builder.gopkg/cli/utils/sigstore_verifier.gopkg/cli/utils/binary_cache_test.gopkg/cli/build.gopkg/cli/utils/binary_cache.gopkg/cli/utils/binary_downloader.go
pkg/**/*.go
⚙️ CodeRabbit configuration file
pkg/**/*.go: This is library code that may affect users.CHANGELOG requirement:
- If this PR changes public APIs, server behavior, or user-visible functionality, verify that CHANGELOG.md has been updated
- Breaking API changes must be marked with BREAKING prefix in CHANGELOG.md
- Internal implementation changes with no user impact do not require CHANGELOG updates
- If unclear whether the change is user-visible, ask the PR author to clarify
- If CHANGELOG.md is not modified for a user-visible change, REQUEST that it be updated
Referenced Documentation Contents
CHANGELOG.md:
Documented added binary download/caching system, Sigstore verification, version-platform cache, --server-version flag, latest/dev-version resolution, offline fallback, and cache cleanup (keep last 3 versions). Also notes removal of embedded binaries and related flag.
Files:
pkg/cli/root.gopkg/builder/download_binary_provider_test.gopkg/builder/download_binary_provider.gopkg/builder/builder.gopkg/cli/utils/sigstore_verifier.gopkg/cli/utils/binary_cache_test.gopkg/cli/build.gopkg/cli/utils/binary_cache.gopkg/cli/utils/binary_downloader.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When writing unit tests, follow the table-driven test convention used throughout the codebase
Files:
pkg/builder/download_binary_provider_test.gopkg/cli/utils/binary_cache_test.go
docs/**
⚙️ CodeRabbit configuration file
docs/**: Documentation changes generally do not require CHANGELOG updates unless:
- They document a new feature (in which case the feature itself should be in CHANGELOG)
- They correct significant user-facing documentation errors
Files:
docs/commands.md
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Before opening a PR with user-visible changes, update CHANGELOG.md with a short summary of the changes
Files:
CHANGELOG.md
**/CHANGELOG.md
⚙️ CodeRabbit configuration file
**/CHANGELOG.md: Verify that changelog entries:
- Follow Keep a Changelog format (Added, Changed, Fixed, Removed, etc.)
- Are user-focused (describe impact, not implementation details)
- Include PR references (#XXX)
- Breaking changes are marked with BREAKING prefix
- Are in the correct section under the unreleased/current version
Referenced Documentation Contents
CHANGELOG.md:
Documented added binary download/caching system, Sigstore verification, version-platform cache, --server-version flag, latest/dev-version resolution, offline fallback, and cache cleanup (keep last 3 versions). Also notes removal of embedded binaries and related flag.
Files:
CHANGELOG.md
🧠 Learnings (6)
📚 Learning: 2025-11-05T00:59:13.052Z
Learnt from: Cali0707
Repo: genmcp/gen-mcp PR: 199
File: pkg/invocation/http/config.go:30-41
Timestamp: 2025-11-05T00:59:13.052Z
Learning: For the genmcp/gen-mcp repository, breaking changes are defined by changes to the YAML/JSON configuration file schema, not internal Go struct field names. If the JSON tags and serialization format remain unchanged, it is considered internal refactoring and does not require a CHANGELOG entry for breaking changes.
Applied to files:
docs/commands.mdCHANGELOG.mdhack/jsonschemagen/go.mod
📚 Learning: 2025-09-30T13:36:23.973Z
Learnt from: matzew
Repo: genmcp/gen-mcp PR: 130
File: hack/lib/common.sh:16-16
Timestamp: 2025-09-30T13:36:23.973Z
Learning: In the genmcp/gen-mcp repository, for the CONTAINER_ENGINE variable in hack/lib/common.sh, the maintainer prefers a simple approach without preflight validation checks for the container engine binary, favoring simplicity over early error detection.
Applied to files:
docs/commands.mdREADME.md
📚 Learning: 2025-10-07T17:29:19.445Z
Learnt from: Cali0707
Repo: genmcp/gen-mcp PR: 155
File: hack/jsonschemagen/go.mod:3-8
Timestamp: 2025-10-07T17:29:19.445Z
Learning: The gen-mcp project targets Go 1.24+ and does not require backward compatibility with Go 1.21.
Applied to files:
docs/commands.mdCHANGELOG.mdREADME.mdhack/jsonschemagen/go.mod
📚 Learning: 2025-10-20T06:48:47.678Z
Learnt from: CR
Repo: genmcp/gen-mcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-20T06:48:47.678Z
Learning: Applies to CHANGELOG.md : Before opening a PR with user-visible changes, update CHANGELOG.md with a short summary of the changes
Applied to files:
CHANGELOG.mdREADME.md
📚 Learning: 2025-11-06T22:50:14.022Z
Learnt from: Cali0707
Repo: genmcp/gen-mcp PR: 203
File: pkg/mcpfile/parser.go:79-0
Timestamp: 2025-11-06T22:50:14.022Z
Learning: In the genmcp/gen-mcp repository, MCP file parsing (via ParseMCPFile in pkg/mcpfile/parser.go) happens only once at startup, so concerns about stale global state from multiple sequential parses do not apply.
Applied to files:
README.mdhack/jsonschemagen/go.mod
📚 Learning: 2025-10-08T21:13:36.046Z
Learnt from: aliok
Repo: genmcp/gen-mcp PR: 155
File: pkg/cli/utils/schema.go:9-16
Timestamp: 2025-10-08T21:13:36.046Z
Learning: In pkg/cli/utils/schema.go, the schema URL intentionally uses refs/heads/main instead of versioned tags, so that all mcpfiles validate against the latest schema on the main branch.
Applied to files:
hack/jsonschemagen/go.mod
🧬 Code graph analysis (7)
pkg/builder/download_binary_provider_test.go (1)
pkg/builder/download_binary_provider.go (1)
NewDownloadBinaryProvider(19-29)
pkg/builder/download_binary_provider.go (1)
pkg/cli/utils/binary_downloader.go (2)
BinaryDownloader(21-25)NewBinaryDownloader(28-44)
pkg/builder/builder.go (1)
pkg/builder/download_binary_provider.go (1)
NewDownloadBinaryProvider(19-29)
pkg/cli/utils/binary_cache_test.go (1)
pkg/cli/utils/binary_cache.go (1)
NewBinaryCacheWithDir(40-59)
pkg/cli/build.go (2)
pkg/cli/root.go (1)
GetVersion(64-66)pkg/builder/builder.go (1)
New(214-233)
pkg/cli/utils/binary_cache.go (1)
pkg/cli/utils/cache_file.go (1)
GetCacheDir(9-23)
pkg/cli/utils/binary_downloader.go (2)
pkg/cli/utils/binary_cache.go (2)
BinaryCache(24-28)NewBinaryCache(31-37)pkg/cli/utils/sigstore_verifier.go (2)
SigstoreVerifier(20-23)NewSigstoreVerifier(26-49)
🪛 GitHub Actions: golangci-lint
pkg/builder/download_binary_provider_test.go
[error] 16-16: golangci-lint: Error return value of os.RemoveAll is not checked (errcheck).
🪛 GitHub Check: lint
pkg/builder/download_binary_provider_test.go
[failure] 16-16:
Error return value of os.RemoveAll is not checked (errcheck)
pkg/cli/utils/binary_cache_test.go
[failure] 14-14:
Error return value of os.RemoveAll is not checked (errcheck)
pkg/cli/utils/binary_cache.go
[failure] 196-196:
Error return value of destination.Close is not checked (errcheck)
[failure] 190-190:
Error return value of source.Close is not checked (errcheck)
[failure] 160-160:
Error return value of f.Close is not checked (errcheck)
pkg/cli/utils/binary_downloader.go
[failure] 175-175:
Error return value of rc.Close is not checked (errcheck)
[failure] 159-159:
Error return value of r.Close is not checked (errcheck)
[failure] 144-144:
Error return value of out.Close is not checked (errcheck)
[failure] 134-134:
Error return value of resp.Body.Close is not checked (errcheck)
[failure] 79-79:
Error return value of os.RemoveAll is not checked (errcheck)
🪛 markdownlint-cli2 (0.18.1)
README.md
163-163: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🔇 Additional comments (29)
README.md (1)
153-226: LGTM!The documentation comprehensively covers the new binary download and caching system, including:
- Sigstore verification details
- Version matching behavior for release/dev builds
- Offline support and cache fallback
- OS-specific cache locations
- Multi-architecture build behavior
The content is clear, user-focused, and well-organized.
docs/commands.md (1)
305-344: LGTM!The command documentation accurately describes:
- The new
--server-versionflag with clear default behavior- Binary download and caching mechanism
- Sigstore verification (built-in)
- OS-specific cache locations
- Version matching rules
The documentation is consistent with README.md and provides clear examples.
CHANGELOG.md (1)
17-34: LGTM!The CHANGELOG entries are well-structured and user-focused:
- Follows Keep a Changelog format correctly
- Describes user-visible impact clearly
- Includes PR references
- Covers all major changes (binary downloads, caching, verification, flags)
The removal of
--use-embedded-binariesis documented appropriately. Since the new behavior is automatic and doesn't require user action, the lack of a BREAKING prefix is acceptable.Based on coding guidelines
pkg/builder/builder.go (1)
213-233: All callers correctly handle the new error return and version parameter.The single caller at
pkg/cli/build.go:63properly:
- Passes the version parameter to
builder.New(push, version)- Handles the error return with
if err != niland exits gracefully- Provides meaningful error context
The CHANGELOG.md is updated with the feature documentation under PR #151, including binary download system, caching, and version specification. The API signature change to support this feature has been properly integrated throughout the codebase.
go.mod (1)
17-17: sigstore-go v1.1.3 is the latest stable version with no active vulnerabilities.Verification confirms v1.1.3 (released Sep 26, 2025) is the latest stable release. A LOW severity advisory exists for sigstore-go but affects only versions ≤ 0.6.0, with the fix available in v0.6.1. Since v1.1.3 is far beyond the patched version, it is not vulnerable.
pkg/cli/root.go (1)
63-66: Initialization order is correct; this review comment is inaccurate.The execution flow guarantees that
GetVersion()is only called aftercliVersionis initialized:
cmd/genmcp/main.gocallscli.Execute(version)Execute()setscliVersion(lines 20–22) before callingrootCmd.Execute()rootCmd.Execute()processes subcommands and only invokes their handlers after the function has set upGetVersion()is called exclusively from Cobra command handlers (executeBuildCmdat line 49, version command handler), which run afterrootCmd.Execute()is invokedThere is no code path where
GetVersion()can be invoked beforeExecute()completes initialization.Likely an incorrect or invalid review comment.
hack/jsonschemagen/go.mod (1)
7-7: I need to gather more information about the remaining dependency. Let me search for details on the google/jsonschema-go version and any potential breaking changes in easyjson.Let me search more specifically for the easyjson v0.9.0 release notes and google/jsonschema-go v0.3.0 stability:
Run tests to verify easyjson and google/jsonschema-go compatibility after the update.
Based on verification:
google/jsonschema-go implements JSON Schema specification with support for creating schemas, validating JSON values, and inferring schemas from Go structs, but specific v0.3.0 release stability information was unavailable in public sources.
easyjson v0.9.0 included "up go version and bugfixes", and later patch releases (v0.9.1 included fixes for unmarshal null handling to existing values and non-pointer fields), indicating behavioral changes. Since the project is still at 0.x, breaking changes between v0.7.7 and v0.9.0 are possible per Go semver conventions.
go.uber.org/multierr v1.10.0 → v1.11.0 is safe; the package is stable under the v1.x line.
Action: Run the project's test suite after upgrading, particularly focusing on JSON unmarshaling edge cases to ensure compatibility.
pkg/cli/build.go (4)
20-20: LGTM! Verify CHANGELOG entry.The new
--server-versionflag is well-documented and provides valuable flexibility for users.Per the coding guidelines, verify that CHANGELOG.md documents this user-visible CLI flag addition. Based on the coding guidelines summary, it appears the CHANGELOG has been updated, but please confirm it mentions the new flag explicitly.
46-60: LGTM! Version resolution logic is clear and well-structured.The version selection logic properly handles three scenarios:
- User-specified version via flag
- Development CLI → latest server binaries
- Release CLI → matching server version
The diagnostic print statements provide good visibility into which server version is being used.
63-67: LGTM! Proper error handling for builder initialization.The error handling correctly checks the builder constructor's error return and provides a clear error message before exiting.
169-172: LGTM! Simple and effective version detection.The
isDevelopmentVersionhelper correctly identifies development builds using a prefix match. This aligns well with the version resolution logic inexecuteBuildCmd.pkg/builder/download_binary_provider.go (2)
19-29: LGTM! Clean constructor with proper error handling.The constructor correctly initializes the binary downloader and wraps any errors appropriately.
32-49: LGTM! Proper error handling and context.The method correctly:
- Delegates binary retrieval to the downloader
- Reads the binary data from the cached path
- Collects file metadata
- Wraps all errors with platform-specific context for better debugging
pkg/cli/utils/sigstore_verifier.go (4)
14-17: Certificate identity configuration looks correct.The OIDC issuer and SAN regex pattern correctly target GitHub Actions-signed artifacts for this repository. The regex pattern allows any path under the repository, which is appropriate for release artifacts.
52-91: LGTM! Comprehensive blob verification implementation.The verification flow correctly:
- Loads the sigstore bundle
- Reads and hashes the artifact (SHA-256)
- Constructs certificate identity with OIDC issuer and SAN regex
- Creates verification policy with artifact digest and certificate requirements
- Executes verification and returns detailed errors
94-98: LGTM! Correct SHA-256 digest implementation.The helper function correctly computes and returns a hex-encoded SHA-256 digest.
26-49: Pass context with timeout toFetchTrustedRoot()to prevent indefinite hangs.The current code calls
root.FetchTrustedRoot()without a context parameter. Based on sigstore-go's API, this call should pass a context with timeout configured (e.g.,context.WithTimeout()). Consider updatingNewSigstoreVerifierto accept a context parameter and pass it toFetchTrustedRoot(ctx)with an appropriate deadline.Verify the exact function signature in your sigstore-go version and ensure the fix is applied at line 29 in
pkg/cli/utils/sigstore_verifier.go.pkg/cli/utils/binary_downloader.go (6)
14-25: LGTM! Well-defined constants and structure.The GitHub release URLs and cache retention count (3 versions) align with the PR objectives.
48-74: LGTM! Smart cache fallback logic.The method properly:
- Resolves "latest" to an actual version tag
- Checks cache before downloading
- Falls back to any cached version if download fails (good offline support)
- Provides clear warning messages to users
This aligns well with the PR objective of supporting offline builds.
95-124: LGTM! Proper download and verification flow.The method correctly:
- Creates temporary directory for downloads (caller handles cleanup as documented)
- Downloads both the binary archive and signature bundle
- Performs Sigstore verification before extraction
- Returns extracted binary path with proper error wrapping
195-197: LGTM! Convenient wrapper for current platform.Simple delegation to
GetBinarywith the current runtime's OS and architecture.
200-224: LGTM! Proper version resolution from GitHub API.The method correctly:
- Calls the GitHub releases API
- Checks HTTP status
- Parses JSON response
- Validates tag_name presence
- Returns the version string
Note: The HTTP timeout issue mentioned in the earlier comment for
downloadFileapplies here too, but the fix location is inNewBinaryDownloader.
227-229: LGTM! Clean delegation to cache cleanup.Properly delegates cache maintenance to the BinaryCache implementation.
pkg/cli/utils/binary_cache.go (6)
14-28: LGTM! Well-designed cache structures.The cache entry includes all necessary metadata:
- Version and platform for identification
- Path to cached binary
- SHA-256 checksum for integrity verification
- Timestamp for cleanup/retention logic
The in-memory map with composite key ("version-platform") provides efficient lookups.
31-59: LGTM! Proper cache initialization.The constructors correctly:
- Create the cache directory structure
- Define the index path
- Load existing index or gracefully handle missing index
- Wrap errors appropriately
62-82: LGTM! Robust cache retrieval with integrity checks.The
Getmethod properly:
- Checks if the file exists on disk (handles external deletion)
- Verifies the SHA-256 checksum (detects corruption)
- Automatically removes invalid entries from the index
- Returns not found for missing or corrupted entries
This defensive approach ensures cache reliability.
85-117: LGTM! Complete cache addition workflow.The
Addmethod correctly:
- Computes SHA-256 before caching
- Generates descriptive filenames with version and platform
- Preserves file extensions (important for Windows
.exe)- Creates a complete cache entry with timestamp
- Persists the index to disk
- Returns the cached path
120-152: LGTM! Straightforward index persistence.The index serialization correctly:
- Uses JSON for human-readable storage
- Converts between array (JSON) and map (in-memory) representations
- Provides indentation for easier debugging
- Sets appropriate file permissions (0644)
211-227: LGTM! Simple and effective latest version lookup.The method correctly finds the most recently downloaded binary for a platform by:
- Filtering entries by platform
- Comparing RFC3339 timestamps (lexicographically sortable)
- Returning the version and path, or empty strings if not found
For the expected cache size (3 versions per platform), the linear search is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
pkg/cli/utils/binary_cache.go (1)
230-256: Confirmed: Sorting issue has been resolved.The previous review comment about sorting before removal has been addressed. The Clean method now correctly sorts entries by timestamp (lines 241-244) before removing old versions.
pkg/cli/utils/binary_downloader.go (2)
28-47: Confirmed: HTTP timeout has been added.The previous review comment about adding an HTTP timeout has been addressed. The client now has a 5-minute timeout (line 43), which is appropriate for binary downloads.
127-152: Confirmed: Defer error handling has been fixed.The previous review comment about handling close errors has been addressed. Both
resp.Body.Close()(line 135) andout.Close()(line 145) now properly handle errors in deferred functions.pkg/builder/download_binary_provider_test.go (1)
11-16: Confirmed: Defer error handling has been fixed.The previous review comment about unchecked error in defer statement has been addressed. Line 16 now properly handles the cleanup error with
defer func() { _ = os.RemoveAll(tmpDir) }().
🧹 Nitpick comments (3)
pkg/cli/utils/binary_downloader.go (2)
49-92: Consider using structured logging for user messages.The fallback warnings (lines 70-71) are printed directly to stdout. While this provides user feedback, consider using a structured logger for consistency, especially since the coding guidelines mention logger usage for Go code.
However, for CLI user-facing messages, stdout may be appropriate. If this is intentional, this comment can be disregarded.
200-225: Consider validating tag_name format.The fetchLatestVersion method correctly fetches and parses the GitHub API response. However, it doesn't validate the format of
tag_name(e.g., ensuring it matches a version pattern likevX.Y.Z).While the current implementation will fail gracefully at download time with a 404 if the tag is malformed, adding format validation here could provide earlier, more descriptive error messages.
pkg/builder/download_binary_provider_test.go (1)
35-53: Test doesn't exercise ExtractServerBinary method.The test name suggests it tests
ExtractServerBinary, but it only validates platform initialization without actually calling the method. While the comment at line 52 acknowledges this limitation, consider either:
- Renaming the test to reflect what it actually validates (e.g., "Platform initialization"), or
- Adding a mock-based test that actually exercises the ExtractServerBinary interface.
This would improve test coverage and clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/builder/download_binary_provider_test.go(1 hunks)pkg/cli/utils/binary_cache.go(1 hunks)pkg/cli/utils/binary_cache_test.go(1 hunks)pkg/cli/utils/binary_downloader.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/cli/utils/binary_cache_test.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use the server-only logger for any logs containing sensitive information; choose between the MCPServer-embedded logger (via logging.BaseFromContext(ctx)) and the shared server/client logger appropriately
Files:
pkg/builder/download_binary_provider_test.gopkg/cli/utils/binary_cache.gopkg/cli/utils/binary_downloader.go
⚙️ CodeRabbit configuration file
**/*.go: Review Go code for:
- Proper error handling
- Resource cleanup (defer statements)
- Potential race conditions
- Performance implications
Files:
pkg/builder/download_binary_provider_test.gopkg/cli/utils/binary_cache.gopkg/cli/utils/binary_downloader.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When writing unit tests, follow the table-driven test convention used throughout the codebase
Files:
pkg/builder/download_binary_provider_test.go
pkg/**/*.go
⚙️ CodeRabbit configuration file
pkg/**/*.go: This is library code that may affect users.CHANGELOG requirement:
- If this PR changes public APIs, server behavior, or user-visible functionality, verify that CHANGELOG.md has been updated
- Breaking API changes must be marked with BREAKING prefix in CHANGELOG.md
- Internal implementation changes with no user impact do not require CHANGELOG updates
- If unclear whether the change is user-visible, ask the PR author to clarify
- If CHANGELOG.md is not modified for a user-visible change, REQUEST that it be updated
Files:
pkg/builder/download_binary_provider_test.gopkg/cli/utils/binary_cache.gopkg/cli/utils/binary_downloader.go
🧬 Code graph analysis (3)
pkg/builder/download_binary_provider_test.go (1)
pkg/builder/download_binary_provider.go (1)
NewDownloadBinaryProvider(19-29)
pkg/cli/utils/binary_cache.go (1)
pkg/cli/utils/cache_file.go (1)
GetCacheDir(9-23)
pkg/cli/utils/binary_downloader.go (2)
pkg/cli/utils/binary_cache.go (2)
BinaryCache(25-29)NewBinaryCache(32-38)pkg/cli/utils/sigstore_verifier.go (2)
SigstoreVerifier(20-23)NewSigstoreVerifier(26-49)
⏰ 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: lint
- GitHub Check: Unit Tests
🔇 Additional comments (15)
pkg/cli/utils/binary_cache.go (8)
15-29: LGTM!The type definitions are well-structured with appropriate fields for caching and tracking binary metadata.
31-60: LGTM!The constructors properly initialize the cache directory and handle the case where the index file doesn't exist yet.
62-83: LGTM!The Get method properly validates both file existence and checksum, removing invalid entries as needed. Ignoring saveIndex errors during cleanup is acceptable for this read-heavy operation.
85-118: LGTM!The Add method properly calculates checksums, preserves file permissions, and maintains the cache index.
120-153: LGTM!The index persistence logic correctly serializes and deserializes the cache entries.
155-183: LGTM!The checksum calculation and verification logic is correct with proper resource handling.
185-209: LGTM!The copyFile method correctly copies file content and preserves permissions with proper resource cleanup.
211-228: LGTM!The GetLatestCached method correctly identifies the most recently downloaded binary for a platform using RFC3339 timestamp comparison.
pkg/cli/utils/binary_downloader.go (6)
15-19: LGTM!The constants are appropriately defined with reasonable defaults.
21-26: LGTM!The BinaryDownloader type properly encapsulates its dependencies.
94-125: LGTM!The download and verification flow is correct. Sigstore verification occurs before extraction, which is the proper security practice. The temp directory cleanup is appropriately delegated to the caller (as noted in the comment at line 95).
154-193: LGTM!The extractBinary method correctly extracts the platform-specific binary from the ZIP archive. The defer statements inside the loop (lines 176, 183) are safe because the loop continues on name mismatch (line 169) and returns immediately on match (line 189), ensuring only one file's resources are managed.
195-198: LGTM!The GetCurrentBinary convenience method correctly uses the current runtime platform.
227-230: LGTM!The CleanOldBinaries method correctly delegates to the cache's Clean method.
pkg/builder/download_binary_provider_test.go (1)
18-33: LGTM!The test appropriately handles potential initialization failures (which may occur in unit test environments without network access) and validates the provider's version field.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/builder/download_binary_provider_test.go (1)
7-44: Optional: collapse duplicate subtest logic into a table‑driven testThe two subtests share almost identical setup and assertions, differing only by the version string and expected value. To follow the project’s table‑driven convention and reduce duplication, you could refactor to a single table‑driven test:
-func TestNewDownloadBinaryProvider(t *testing.T) { - t.Run("creates provider with specified version", func(t *testing.T) { - provider, err := NewDownloadBinaryProvider("v0.1.0") - ... - }) - - t.Run("creates provider with latest version", func(t *testing.T) { - provider, err := NewDownloadBinaryProvider("latest") - ... - }) -} +func TestNewDownloadBinaryProvider(t *testing.T) { + tests := []struct { + name string + version string + }{ + {name: "creates provider with specified version", version: "v0.1.0"}, + {name: "creates provider with latest version", version: "latest"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + provider, err := NewDownloadBinaryProvider(tt.version) + if err != nil { + t.Fatalf("unexpected error creating download provider for %q: %v", tt.version, err) + } + + if provider == nil { + t.Fatal("expected provider to be created") + } + if provider.version != tt.version { + t.Errorf("expected version %s, got %s", tt.version, provider.version) + } + if provider.downloader == nil { + t.Error("expected downloader to be initialized") + } + }) + } +}This keeps the behavior the same while aligning with the table‑driven test style.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/builder/download_binary_provider_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use the server-only logger for any logs containing sensitive information; choose between the MCPServer-embedded logger (via logging.BaseFromContext(ctx)) and the shared server/client logger appropriately
Files:
pkg/builder/download_binary_provider_test.go
⚙️ CodeRabbit configuration file
**/*.go: Review Go code for:
- Proper error handling
- Resource cleanup (defer statements)
- Potential race conditions
- Performance implications
Files:
pkg/builder/download_binary_provider_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When writing unit tests, follow the table-driven test convention used throughout the codebase
Files:
pkg/builder/download_binary_provider_test.go
pkg/**/*.go
⚙️ CodeRabbit configuration file
pkg/**/*.go: This is library code that may affect users.CHANGELOG requirement:
- If this PR changes public APIs, server behavior, or user-visible functionality, verify that CHANGELOG.md has been updated
- Breaking API changes must be marked with BREAKING prefix in CHANGELOG.md
- Internal implementation changes with no user impact do not require CHANGELOG updates
- If unclear whether the change is user-visible, ask the PR author to clarify
- If CHANGELOG.md is not modified for a user-visible change, REQUEST that it be updated
Referenced Documentation Contents
CHANGELOG.md:
Notes Unreleased entries for PR 151: add binary download/caching with Sigstore verification, version-platform cache, --server-version flag, offline fallback, and 3-version retention. Highlights removal of embedded binaries and --use-embedded-binaries flag. Emphasizes no public API changes beyond changelog statements.README.md:
Documentation sections describing binary downloads with Sigstore verification, version matching (Release/Development/Custom server version), offline support, and cache management per OS. Includes usage examples for --server-version and multi-arch handling; reiterates cache location and retention behavior.docs/commands.md:
New Build command guidance: --server-version flag, binary management section detailing source (GitHub releases), cache, Sigstore verification, per-OS cache paths, version matching rules, and network requirements. Examples show building with --server-version and derivations for development/releases.
Files:
pkg/builder/download_binary_provider_test.go
🧬 Code graph analysis (1)
pkg/builder/download_binary_provider_test.go (1)
pkg/builder/download_binary_provider.go (1)
NewDownloadBinaryProvider(19-29)
⏰ 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: Unit Tests
- GitHub Check: lint
nader-ziada
force-pushed
the
download-server
branch
from
2b94a66 to
de8f61b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/builder/download_binary_provider_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use the server-only logger for any logs containing sensitive information; choose between the MCPServer-embedded logger (via logging.BaseFromContext(ctx)) and the shared server/client logger appropriately
Files:
pkg/builder/download_binary_provider_test.go
⚙️ CodeRabbit configuration file
**/*.go: Review Go code for:
- Proper error handling
- Resource cleanup (defer statements)
- Potential race conditions
- Performance implications
Files:
pkg/builder/download_binary_provider_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When writing unit tests, follow the table-driven test convention used throughout the codebase
Files:
pkg/builder/download_binary_provider_test.go
pkg/**/*.go
⚙️ CodeRabbit configuration file
pkg/**/*.go: This is library code that may affect users.CHANGELOG requirement:
- If this PR changes public APIs, server behavior, or user-visible functionality, verify that CHANGELOG.md has been updated
- Breaking API changes must be marked with BREAKING prefix in CHANGELOG.md
- Internal implementation changes with no user impact do not require CHANGELOG updates
- If unclear whether the change is user-visible, ask the PR author to clarify
- If CHANGELOG.md is not modified for a user-visible change, REQUEST that it be updated
Referenced Documentation Contents
CHANGELOG.md:
Notes Unreleased changes for PR #151: binary download/caching system, Sigstore verification, version-platform cache with last 3 retained, --server-version flag, offline fallback, and cache cleanup. Also notes removal of embedded binaries and --use-embedded-binaries flag.
Files:
pkg/builder/download_binary_provider_test.go
🧬 Code graph analysis (1)
pkg/builder/download_binary_provider_test.go (1)
pkg/builder/download_binary_provider.go (1)
NewDownloadBinaryProvider(19-29)
⏰ 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: lint
- GitHub Check: Unit Tests
nader-ziada
force-pushed
the
download-server
branch
from
de8f61b to
84539eb
Compare
Replace embedded server binaries with on-demand downloads from GitHub releases, reducing CLI size from ~100MB to ~78MB. Downloaded binaries are cryptographically verified using Sigstore and cached locally for reuse. - Add binary download system with automatic version resolution - Implement version-platform cache with automatic cleanup (keeps last 3 versions) - Add --server-version flag to specify server binary version - Support offline builds with cached binary fallback - Remove embedded binaries and build-server-binaries make target - Dev builds automatically use latest stable server binaries - Release builds download matching server version by default Signed-off-by: Nader Ziada <[email protected]>
Signed-off-by: Nader Ziada <[email protected]>
Signed-off-by: Nader Ziada <[email protected]>
nader-ziada
force-pushed
the
download-server
branch
from
84539eb to
c58ab54
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
go.mod (1)
36-208: Large expansion of indirect dependencies warrants documentation.The indirect dependency tree has expanded significantly (~70+ transitive dependencies added/updated), primarily from sigstore-go's ecosystem integration (Google Cloud, OpenTelemetry, Sigstore modules, Kubernetes, security libraries). While this is expected given the Sigstore verification requirement, it increases supply chain complexity and build artifact footprint.
Consider documenting in PR notes or architecture decisions:
- Rationale for accepting the transitive dependency expansion
- Any steps taken to audit high-risk transitive dependencies (e.g., cryptographic libs)
- Whether the net binary size reduction (100MB → 78MB) justifies the dependency scope increase
Do you want me to generate a shell script to identify the most high-risk transitive dependencies (cryptography, networking, serialization) for targeted audit, or check for any duplicate/conflicting versions across the tree?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumhack/jsonschemagen/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
CHANGELOG.md(1 hunks)README.md(2 hunks)docs/commands.md(2 hunks)go.mod(5 hunks)hack/jsonschemagen/go.mod(1 hunks)pkg/builder/builder.go(1 hunks)pkg/builder/download_binary_provider.go(1 hunks)pkg/builder/download_binary_provider_test.go(1 hunks)pkg/cli/build.go(4 hunks)pkg/cli/root.go(1 hunks)pkg/cli/utils/binary_cache.go(1 hunks)pkg/cli/utils/binary_cache_test.go(1 hunks)pkg/cli/utils/binary_downloader.go(1 hunks)pkg/cli/utils/sigstore_verifier.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- hack/jsonschemagen/go.mod
- pkg/builder/download_binary_provider.go
- pkg/cli/utils/binary_cache_test.go
- docs/commands.md
- pkg/builder/download_binary_provider_test.go
- pkg/cli/root.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Check whether you are using the server-only logger (embedded in the
MCPServertype, and accessed from invocation contexts withlogging.BaseFromContext(ctx)) or the server and client logger. Ensure that any logs with sensitive info use the server-only logger
Files:
pkg/builder/builder.gopkg/cli/utils/sigstore_verifier.gopkg/cli/build.gopkg/cli/utils/binary_downloader.gopkg/cli/utils/binary_cache.go
⚙️ CodeRabbit configuration file
**/*.go: Review Go code for:
- Proper error handling
- Resource cleanup (defer statements)
- Potential race conditions
- Performance implications
Files:
pkg/builder/builder.gopkg/cli/utils/sigstore_verifier.gopkg/cli/build.gopkg/cli/utils/binary_downloader.gopkg/cli/utils/binary_cache.go
pkg/**/*.go
⚙️ CodeRabbit configuration file
pkg/**/*.go: This is library code that may affect users.CHANGELOG requirement:
- If this PR changes public APIs, server behavior, or user-visible functionality, verify that CHANGELOG.md has been updated
- Breaking API changes must be marked with BREAKING prefix in CHANGELOG.md
- Internal implementation changes with no user impact do not require CHANGELOG updates
- If unclear whether the change is user-visible, ask the PR author to clarify
- If CHANGELOG.md is not modified for a user-visible change, REQUEST that it be updated
Files:
pkg/builder/builder.gopkg/cli/utils/sigstore_verifier.gopkg/cli/build.gopkg/cli/utils/binary_downloader.gopkg/cli/utils/binary_cache.go
**/CHANGELOG.md
⚙️ CodeRabbit configuration file
**/CHANGELOG.md: Verify that changelog entries:
- Follow Keep a Changelog format (Added, Changed, Fixed, Removed, etc.)
- Are user-focused (describe impact, not implementation details)
- Include PR references (#XXX)
- Breaking changes are marked with BREAKING prefix
- Are in the correct section under the unreleased/current version
Files:
CHANGELOG.md
🧠 Learnings (5)
📚 Learning: 2025-09-30T13:36:23.973Z
Learnt from: matzew
Repo: genmcp/gen-mcp PR: 130
File: hack/lib/common.sh:16-16
Timestamp: 2025-09-30T13:36:23.973Z
Learning: In the genmcp/gen-mcp repository, for the CONTAINER_ENGINE variable in hack/lib/common.sh, the maintainer prefers a simple approach without preflight validation checks for the container engine binary, favoring simplicity over early error detection.
Applied to files:
README.md
📚 Learning: 2025-10-07T17:29:19.445Z
Learnt from: Cali0707
Repo: genmcp/gen-mcp PR: 155
File: hack/jsonschemagen/go.mod:3-8
Timestamp: 2025-10-07T17:29:19.445Z
Learning: The gen-mcp project targets Go 1.24+ and does not require backward compatibility with Go 1.21.
Applied to files:
README.mdCHANGELOG.md
📚 Learning: 2025-11-06T22:50:14.022Z
Learnt from: Cali0707
Repo: genmcp/gen-mcp PR: 203
File: pkg/mcpfile/parser.go:79-0
Timestamp: 2025-11-06T22:50:14.022Z
Learning: In the genmcp/gen-mcp repository, MCP file parsing (via ParseMCPFile in pkg/mcpfile/parser.go) happens only once at startup, so concerns about stale global state from multiple sequential parses do not apply.
Applied to files:
README.md
📚 Learning: 2025-11-05T00:59:13.052Z
Learnt from: Cali0707
Repo: genmcp/gen-mcp PR: 199
File: pkg/invocation/http/config.go:30-41
Timestamp: 2025-11-05T00:59:13.052Z
Learning: For the genmcp/gen-mcp repository, breaking changes are defined by changes to the YAML/JSON configuration file schema, not internal Go struct field names. If the JSON tags and serialization format remain unchanged, it is considered internal refactoring and does not require a CHANGELOG entry for breaking changes.
Applied to files:
CHANGELOG.md
📚 Learning: 2025-11-29T19:44:58.854Z
Learnt from: CR
Repo: genmcp/gen-mcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-29T19:44:58.854Z
Learning: Update the `./CHANGELOG.md` file with relevant changes before opening a PR. Every PR with user-visible changes should update the `./CHANGELOG.md` with a short summary of the changes
Applied to files:
CHANGELOG.md
🧬 Code graph analysis (2)
pkg/builder/builder.go (1)
pkg/builder/download_binary_provider.go (1)
NewDownloadBinaryProvider(19-29)
pkg/cli/utils/binary_cache.go (1)
pkg/cli/utils/cache_file.go (1)
GetCacheDir(9-23)
⏰ 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: lint
- GitHub Check: Unit Tests
🔇 Additional comments (13)
go.mod (2)
27-27: Minor version bump for easyjson is safe.The update from
v0.7.7tov0.9.0is a minor version bump and should be backward-compatible. No issues detected.
17-17: No action needed. sigstore-go v1.1.3 is a stable, officially released version (Sep 26, 2025, marked Latest) with no known CVEs affecting it. The dependency is appropriate for production use.pkg/builder/builder.go (1)
213-233: Verify whether this public API change requires a BREAKING tag in CHANGELOG.The
Newfunction signature has changed fromNew(saveToRegistry bool) *ImageBuildertoNew(saveToRegistry bool, version string) (*ImageBuilder, error), which is a breaking change for any Go code that importspkg/builder.Since this is in
pkg/builder, and the coding guidelines indicate thatpkg/**/*.gois library code that may affect users, please clarify:
- Is
pkg/builderintended as a public API for external consumers, or is it internal-only?- If it's public, should this change be marked with BREAKING in the CHANGELOG?
Based on learnings, if
pkg/builderis not intended for external use and only the CLI is the public interface, then the current CHANGELOG entry is appropriate. Otherwise, consider marking this as a breaking change.pkg/cli/utils/binary_cache.go (1)
231-256: LGTM! Past sorting issue has been addressed.The
Cleanmethod now correctly sorts entries by download timestamp (newest first) before removing old versions. This addresses the critical issue flagged in the previous review where entries were being removed without sorting.The implementation properly:
- Groups entries by platform
- Sorts each platform's entries by timestamp (descending)
- Keeps the first
keepVersionsentries (newest)- Removes older entries and cleans up their files
README.md (1)
155-228: Excellent documentation coverage.The documentation comprehensively covers all aspects of the new binary download and caching system:
- Binary downloads with Sigstore verification
- Version matching strategies (release builds, development builds, custom versions)
- Network requirements and offline support
- Cache management and cleanup procedures
- Platform-specific cache locations
The examples clearly demonstrate the new
--server-versionflag usage and align well with the implementation.pkg/cli/build.go (2)
62-83: Well-implemented version resolution logic.The version determination logic correctly implements the documented behavior:
- Explicit
--server-versiontakes precedence- Development CLI builds automatically use latest server binaries
- Release CLI builds match server binary versions
- Clear diagnostic messages inform users of the chosen version
The error handling for builder initialization is appropriate, providing clear feedback if setup fails.
255-258: Simple and effective development version detection.The
isDevelopmentVersionhelper cleanly identifies development builds using the "development@" prefix convention.pkg/cli/utils/sigstore_verifier.go (2)
25-49: Proper Sigstore verifier initialization.The verifier correctly:
- Fetches the trusted root from TUF (The Update Framework)
- Requires SCT (Signed Certificate Timestamp)
- Requires Rekor transparency log entry
- Requires observer timestamps
This follows Sigstore best practices for secure verification.
51-91: Robust blob verification implementation.The
VerifyBlobmethod properly:
- Loads the sigstore bundle
- Computes the artifact's SHA256 digest
- Constructs certificate identity with OIDC issuer and SAN regex matching GitHub Actions
- Builds and applies verification policy
- Returns detailed error messages on verification failure
The certificate identity regex
https://github.com/genmcp/gen-mcp/.*appropriately matches GitHub Actions workflow runs from this repository.pkg/cli/utils/binary_downloader.go (4)
40-46: Past review issue addressed: HTTP timeout configured.The HTTP client now includes a 5-minute timeout, which prevents indefinite hangs on slow or stalled connections. This addresses the past review comment.
127-152: Past review issue addressed: Close errors handled properly.All
defer Close()calls now explicitly handle errors using_ =to acknowledge and ignore them where appropriate:
- Line 135:
resp.Body.Close()- Line 145:
out.Close()This addresses the errcheck linting issues flagged in the previous review.
49-92: Well-designed binary retrieval with offline fallback.The
GetBinarymethod implements a robust retrieval strategy:
- Resolves "latest" to an actual version tag
- Checks cache before downloading
- Falls back to latest cached version on download failure
- Provides clear warnings when using cached fallback
- Automatically cleans old binaries to prevent cache bloat
- Proper cleanup of temporary directories
This design ensures both reliability and good UX for offline scenarios.
154-193: Clean ZIP extraction with platform-specific handling.The
extractBinarymethod correctly:
- Handles platform-specific binary naming (adds
.exefor Windows)- Searches for the expected binary in the ZIP archive
- Preserves file permissions from the archive
- Returns a clear error if the binary is not found
nader-ziada
force-pushed
the
download-server
branch
from
c58ab54 to
208a5e6
Compare
Cali0707
left a comment
Cali0707
left a comment
There was a problem hiding this comment.
@nader-ziada this looks good! Left one comment about where we are verifying the binaries
Also, would it be possible to output when running genmcp build ... which files are being downloaded? Possibly behind a -v flag?
I realize this may involve changing the CLI logging architecture a bit as we may need to propagate something through to the builder package...
|
thanks for the review @Cali0707, will work on these changes |
- also add verbose output to build command Signed-off-by: Nader Ziada <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
pkg/cli/utils/binary_downloader_test.go (1)
11-79: Consider refactoring to table-driven tests.Both test functions share nearly identical structure (pipe setup, downloader creation, printf call, pipe teardown, output verification). This could be consolidated into a single table-driven test with verbose/silent as parameters, reducing duplication. However, the current approach is acceptable and readable.
pkg/builder/download_binary_provider.go (1)
31-49: Implementation is correct; consider nil-check for robustness.The
ExtractServerBinarymethod correctly orchestrates binary retrieval. One defensive improvement: ifplatformis nil, accessingplatform.OSandplatform.Architecturewill panic.func (dp *DownloadBinaryProvider) ExtractServerBinary(platform *v1.Platform) ([]byte, fs.FileInfo, error) { + if platform == nil { + return nil, nil, fmt.Errorf("platform cannot be nil") + } + binaryPath, err := dp.downloader.GetBinary(dp.version, platform.OS, platform.Architecture)pkg/cli/utils/binary_downloader.go (1)
230-241: Defers accumulate in loop iterations.The
deferstatements forrc.Close()anddest.Close()on lines 234 and 241 are inside a loop. While this works correctly here because the function returns immediately after finding a match, it's generally better practice to use explicit close or wrap in a helper function for clarity.Since the function returns on the first match (line 247), the current behavior is correct. However, if the loop logic changes in the future, this could cause resource leaks. Consider extracting the file extraction logic into a helper function for clearer resource management.
pkg/cli/utils/binary_cache.go (2)
62-83: Consider handlingsaveIndexerrors when pruning invalid entries inGetWhen cache entries are invalid (missing file or checksum mismatch),
Getdeletes them frombc.entriesbut ignores any error frombc.saveIndex(). If the index write fails, the on-disk index can still contain the bad entry, which may get reloaded in a new process and cause repeated verification failures.If feasible, consider at least surfacing/logging the
saveIndexerror, or having a best-effort mechanism so repeated bad entries don’t persist across runs. This is not a blocker but would make cache corruption more self-healing.
1-256: Confirm CHANGELOG entry for this user-visible caching behaviorThis cache is part of a larger change that alters how
genmcp buildlocates and reuses server binaries (download-and-cache vs embedded). That’s clearly user-visible behavior for the CLI.Per the repo guidelines, please ensure there is a corresponding entry in
CHANGELOG.mddescribing the new binary download/caching behavior (and marking it as BREAKING if any existing workflows might be impacted). If you’ve already added it elsewhere in the PR, feel free to ignore this.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pkg/builder/builder.go(1 hunks)pkg/builder/download_binary_provider.go(1 hunks)pkg/builder/download_binary_provider_test.go(1 hunks)pkg/cli/build.go(4 hunks)pkg/cli/utils/binary_cache.go(1 hunks)pkg/cli/utils/binary_downloader.go(1 hunks)pkg/cli/utils/binary_downloader_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/builder/download_binary_provider_test.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Check whether you are using the server-only logger (embedded in the
MCPServertype, and accessed from invocation contexts withlogging.BaseFromContext(ctx)) or the server and client logger. Ensure that any logs with sensitive info use the server-only logger
Files:
pkg/cli/utils/binary_downloader_test.gopkg/builder/builder.gopkg/cli/utils/binary_downloader.gopkg/cli/utils/binary_cache.gopkg/cli/build.gopkg/builder/download_binary_provider.go
⚙️ CodeRabbit configuration file
**/*.go: Review Go code for:
- Proper error handling
- Resource cleanup (defer statements)
- Potential race conditions
- Performance implications
Files:
pkg/cli/utils/binary_downloader_test.gopkg/builder/builder.gopkg/cli/utils/binary_downloader.gopkg/cli/utils/binary_cache.gopkg/cli/build.gopkg/builder/download_binary_provider.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Follow the table test convention used throughout the codebase when writing unit tests
Files:
pkg/cli/utils/binary_downloader_test.go
pkg/**/*.go
⚙️ CodeRabbit configuration file
pkg/**/*.go: This is library code that may affect users.CHANGELOG requirement:
- If this PR changes public APIs, server behavior, or user-visible functionality, verify that CHANGELOG.md has been updated
- Breaking API changes must be marked with BREAKING prefix in CHANGELOG.md
- Internal implementation changes with no user impact do not require CHANGELOG updates
- If unclear whether the change is user-visible, ask the PR author to clarify
- If CHANGELOG.md is not modified for a user-visible change, REQUEST that it be updated
Files:
pkg/cli/utils/binary_downloader_test.gopkg/builder/builder.gopkg/cli/utils/binary_downloader.gopkg/cli/utils/binary_cache.gopkg/cli/build.gopkg/builder/download_binary_provider.go
🧬 Code graph analysis (4)
pkg/cli/utils/binary_downloader_test.go (1)
pkg/cli/utils/binary_downloader.go (1)
NewBinaryDownloader(30-49)
pkg/cli/utils/binary_cache.go (1)
pkg/cli/utils/cache_file.go (1)
GetCacheDir(9-23)
pkg/cli/build.go (2)
pkg/cli/root.go (1)
GetVersion(64-66)pkg/builder/builder.go (1)
New(214-233)
pkg/builder/download_binary_provider.go (1)
pkg/cli/utils/binary_downloader.go (2)
BinaryDownloader(22-27)NewBinaryDownloader(30-49)
⏰ 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: Unit Tests
- GitHub Check: lint
🔇 Additional comments (7)
pkg/cli/build.go (3)
25-26: New CLI flags look good.The
--server-versionand--verboseflags are well-documented with sensible defaults. The verbose flag defaulting totrueprovides good UX during binary downloads.
64-85: Version resolution logic is sound.The logic correctly:
- Uses "latest" for development builds
- Matches CLI version for release builds
- Allows explicit override via
--server-version- Handles builder initialization errors appropriately
257-260: Verify CHANGELOG.md is updated for user-visible changes.This PR introduces significant user-visible changes:
- New
--server-versionand--verboseCLI flags- Changed build behavior (downloads binaries instead of using embedded ones)
- Public API change:
builder.Newsignature now includes version and verbose parameters and returns an errorAs per coding guidelines for
pkg/**/*.go, ensure CHANGELOG.md has been updated to document these changes. If CHANGELOG.md was modified, mark breaking API changes with a BREAKING prefix.pkg/builder/builder.go (1)
213-233: Constructor changes are well-structured.The updated
Newfunction:
- Properly initializes the
DownloadBinaryProviderwith version and verbose settings- Correctly returns an error when provider creation fails
- Maintains clean separation of concerns with dependency injection
The breaking API change (now returns error) is necessary to handle download/verification failures gracefully.
pkg/cli/utils/binary_downloader.go (2)
29-49: HTTP timeout and error handling look good.The implementation correctly addresses previous review feedback:
- HTTP client has a 5-minute timeout for binary downloads
- All
Close()calls usedefer func() { _ = ... }()pattern to handle errors
74-82: Cached binary verification is properly implemented.This addresses the previous review comment from Cali0707 requesting verification of cached binaries. The code now re-verifies cached binaries with
verifyAndExtractZipand falls back to re-downloading if verification fails.pkg/cli/utils/binary_cache.go (1)
211-228: Bug inGetLatestCached: pointer to range variable causes incorrect logicThe code takes the address of the range variable
entry, which is reused across loop iterations in Go. This causes two problems:
- The comparison
entry.DownloadedAt > latestEntry.DownloadedAtdoesn't work as intended since both may reference the same memory location being updated.- After the loop,
latestEntrypoints to whatever value was assigned on the last iteration, not the entry with the latestDownloadedAtfor the requested platform.Fix by tracking the entry by value instead of pointer:
-func (bc *BinaryCache) GetLatestCached(platform string) (string, string) { - var latestEntry *BinaryCacheEntry - - for _, entry := range bc.entries { - if entry.Platform == platform { - if latestEntry == nil || entry.DownloadedAt > latestEntry.DownloadedAt { - latestEntry = &entry - } - } - } - - if latestEntry != nil { - return latestEntry.Version, latestEntry.ZipPath - } - - return "", "" -} +func (bc *BinaryCache) GetLatestCached(platform string) (string, string) { + var latestEntry BinaryCacheEntry + found := false + + for _, entry := range bc.entries { + if entry.Platform != platform { + continue + } + if !found || entry.DownloadedAt > latestEntry.DownloadedAt { + latestEntry = entry + found = true + } + } + + if found { + return latestEntry.Version, latestEntry.ZipPath + } + return "", "" +}
|
@Cali0707 add validation on getting file from cache and a verbose option for |
Cali0707
left a comment
Cali0707
left a comment
There was a problem hiding this comment.
LGTM, thanks @nader-ziada !
This is a great improvement to the build capabilities
2 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Replace embedded server binaries with on-demand downloads from GitHub releases, reducing CLI size from ~100MB to ~78MB. Downloaded binaries are cryptographically verified using Sigstore and cached locally for reuse.
Which issue(s) this PR fixes:
Fixes #174
Proposed Changes
Release note:
Summary by CodeRabbit
New Features
Changed
Removed
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.