feat: add xcaddy-style custom binary builder#141
Conversation
Adds pkg/builder and cmd/builder — a tool that generates and compiles
custom mcp-anything binaries containing only the registry-implementing
packages the user selects (tree-shaking).
Mirrors xcaddy's approach: creates a temp Go module with a replace
directive pointing to the local module, copies go.sum for offline
transitive-dep verification, renders a target-specific main.go from
a template, then runs go build.
Targets:
- proxy — standalone mcp-anything proxy with custom component set
- operator — Kubernetes operator (component list reserved for future
selective CRD generation)
- caddy — Caddy binary with mcp-anything module + extra backends
- kong — Kong Go plugin wrapping mcp-anything (downloads go-pdk)
Usage:
mcp-anything-builder \
--target=proxy \
--package=github.com/gaarutyunov/mcp-anything/pkg/cache/redis \
--package=github.com/gaarutyunov/mcp-anything/pkg/auth/inbound/jwt \
--output=bin/my-proxy
Closes #126
Co-authored-by: German Arutyunov <gaarutyunov@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds an xcaddy-style builder CLI and library that generate and compile custom binaries (proxy, operator, Caddy plugin, Kong plugin). Adds Makefile target and Dockerfiles to build via the builder, unit tests for the builder package, e2e tests exercising built binaries in Kubernetes, and CI steps to build/publish the Caddy builder image. Changes
Sequence DiagramsequenceDiagram
participant User as CLI User
participant CLI as cmd/builder
participant Builder as pkg/builder.Builder
participant TempDir as Temp Dir
participant GoTool as Go tool (mod/download & build)
User->>CLI: run mcp-anything-builder with flags
CLI->>Builder: New(Config) / Build(ctx)
Builder->>TempDir: create temp dir, write go.mod and main.go
Builder->>TempDir: copy go.sum (if present)
Builder->>GoTool: run `go mod download` (if needed)
Builder->>GoTool: run `go build -o <output>` (with LDFlags, env overrides)
GoTool-->>Builder: binary produced at output path
Builder->>TempDir: cleanup temp dir
Builder-->>CLI: return success / error
CLI-->>User: exit (0 or 1)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
57-57:⚠️ Potential issue | 🟡 Minor
checktarget doesn't includebuild-builder.The AI summary states that
check's dependency chain was updated to includebuild-builder, but line 57 still readscheck: lint vet test build build-operator treeshake. The new builder binary is therefore not validated bymake check, andbuild-builderis declared in.PHONYbut never exercised by the default verification flow.Proposed fix
-check: lint vet test build build-operator treeshake +check: lint vet test build build-operator build-builder treeshake🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 57, The Makefile's check target is missing the new build-builder prerequisite so the builder binary isn't validated by make check; update the check target definition (the line starting with "check:") to include build-builder among its prerequisites (e.g., add build-builder alongside lint vet test build build-operator treeshake) so the new builder is exercised by the default verification flow; verify .PHONY already lists build-builder and adjust if necessary.
🧹 Nitpick comments (4)
pkg/builder/builder.go (2)
213-223: PreferGOSUMDB=offoverGONOSUMDB=*, and usestrings.HasPrefix.
GONOSUMDBtakes a comma-separated glob list of module-path prefixes (inherited from / analogous toGOPRIVATE); passing*is undocumented and relies on glob semantics that may or may not match every module path depending on the Go toolchain version. The canonical way to fully disable checksum-database lookups isGOSUMDB=off. Since you also copygo.suminto the temp module for offline verification, this is the intended behavior.Separately, the manual length-8 slice compare is brittle; use
strings.HasPrefix(e, "GOFLAGS=").Proposed fix
- env := make([]string, 0, len(os.Environ())+2) - for _, e := range os.Environ() { - if len(e) >= 8 && e[:8] == "GOFLAGS=" { - continue - } - env = append(env, e) - } - env = append(env, "GONOSUMDB=*", "GOFLAGS=-mod=mod") + env := make([]string, 0, len(os.Environ())+2) + for _, e := range os.Environ() { + if strings.HasPrefix(e, "GOFLAGS=") { + continue + } + env = append(env, e) + } + env = append(env, "GOSUMDB=off", "GOFLAGS=-mod=mod")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/builder/builder.go` around lines 213 - 223, Replace the brittle manual prefix check and unset of GOFLAGS with strings.HasPrefix when filtering os.Environ() and change the sum DB disable from the undocumented "GONOSUMDB=*" to the canonical "GOSUMDB=off"; specifically update the loop that builds env (used to set cmd.Env) to use strings.HasPrefix(e, "GOFLAGS=") and append "GOSUMDB=off" and "GOFLAGS=-mod=mod" instead of "GONOSUMDB=*" so checksum lookups are reliably disabled.
148-148: Hardcodedgo 1.24in the generated go.mod may drift from the host module.If the parent repo's
go.modever declares a newer directive (e.g.go 1.25), the temp module will requirego 1.24while transitively pulling in packages that may use features/toolchain directives beyond that, leading to confusing build failures. Consider reading thegodirective from<ModuleDir>/go.mod(viagolang.org/x/mod/modfileor a small regex) and reusing it, or at minimum centralize this constant next topseudoVer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/builder/builder.go` at line 148, The generated go.mod in builder.go hardcodes "go 1.24"; update the code that writes the header (the fmt.Fprintf call producing "module mcp-anything-custom\n\ngo 1.24\n\nrequire (\n") to read the host module's "go" directive and reuse it (use golang.org/x/mod/modfile.Parse or a small regex to read the go directive from the parent module's go.mod), falling back to a single centralized constant (near pseudoVer) if parsing fails; ensure you reference this computed goVersion variable when formatting the go.mod instead of the literal "1.24".pkg/builder/targets.go (1)
334-334: Useerrors.Is(err, context.Canceled)instead of direct comparison.
startErr != context.Canceledmisses wrapped cancellation errors (e.g.fmt.Errorf("serving: %w", context.Canceled)), which is how the rest of the codebase surfaces errors per the repo's guideline to wrap with%w. Matches the proxy template at line 51 which already useserrors.Is.Proposed fix
- if startErr := proxy.Start(ctx); startErr != nil && startErr != context.Canceled { + if startErr := proxy.Start(ctx); startErr != nil && !errors.Is(startErr, context.Canceled) {(also add
"errors"to the template's import block)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/builder/targets.go` at line 334, Replace the direct comparison against context.Canceled in the proxy start error check with errors.Is to correctly handle wrapped cancellation errors: change the condition that checks startErr from "startErr != context.Canceled" to use errors.Is(startErr, context.Canceled) (i.e., treat the error as ignorable when errors.Is(startErr, context.Canceled) is true) and ensure the "errors" package is imported; this change applies to the proxy.Start(ctx) call and its startErr handling in targets.go.pkg/builder/builder_test.go (1)
69-116: Consider usingt.Helper()+errcheck-safeNewin helpers.Minor:
b, _ := New(...)inTestWriteGoMod_*,TestWriteMain_*, andTestCopyGoSum_*silently ignores errors; ifNewlater starts rejecting something (e.g.ModuleDirmust exist), these tests would segfault onb.writeGoModrather than fail cleanly. Usingif err != nil { t.Fatalf(...) }everywhere makes failures diagnosable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/builder/builder_test.go` around lines 69 - 116, Replace silent New calls in TestWriteGoMod_noExtraRequire and TestWriteGoMod_extraRequire with an err-checked helper: create a small helper (e.g. func mustNewBuilder(t *testing.T, cfg Config) *Builder) that calls t.Helper(), invokes New(cfg), fails the test with t.Fatalf if err != nil, and returns the builder; then use mustNewBuilder(t, Config{...}) instead of b, _ := New(...). Also update other tests noted (TestWriteMain_*, TestCopyGoSum_*) to use the same mustNewBuilder helper so New errors are surfaced instead of causing panics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/builder/targets.go`:
- Around line 299-339: startProxy has a TOCTOU on port reservation and
double-starts the server; fix by changing it to hand an open net.Listener into
the proxy (avoid reserve/close using net.Listen and pass that listener to
mcpanything.New or a new NewWithListener API) instead of setting cfg.Server.Addr
from ln.Addr(), remove the duplicate startup call so only one of
proxy.StartBackground(ctx) or proxy.Start(ctx) is invoked (pick the background
helper and delete the extra goroutine), and block until the server is actually
ready before returning (implement a short readiness poll using net.DialTimeout
to the listener address or use a StartBackground-ready signal if available) so
startProxy reliably returns a bound, accepting address.
- Around line 90-96: The init() currently uses utilruntime.Must which panics on
failure; instead implement an initScheme(s *runtime.Scheme) error that performs
the calls to clientgoscheme.AddToScheme(scheme), corev1.AddToScheme,
appsv1.AddToScheme, v1alpha1.AddToScheme and gatewayv1.Install and returns any
error encountered (do not call utilruntime.Must), then remove the panic-raising
init() body and call initScheme(scheme) from cmd/operator/main.go (or wherever
process startup occurs), logging the error and exiting with non-zero status on
failure; reference the symbols utilruntime.Must, init(),
initScheme(*runtime.Scheme), scheme, and main when making the change.
- Around line 277-295: In Access (func (c *Config) Access) replace the current
approach of setting "X-Forwarded-Host" on kong.ServiceRequest with a proper
upstream rewrite: parse proxyAddr using net.SplitHostPort to get host and port,
then call kong.Service.SetTarget(host, port) to direct Kong to the embedded
proxy (handle SplitHostPort and conversion errors and call
kong.Response.SetStatus on failure similar to the existing error handling); keep
existing proxy startup logic (startProxy, startOnce, proxyErr) and log errors
via slog as done now.
---
Outside diff comments:
In `@Makefile`:
- Line 57: The Makefile's check target is missing the new build-builder
prerequisite so the builder binary isn't validated by make check; update the
check target definition (the line starting with "check:") to include
build-builder among its prerequisites (e.g., add build-builder alongside lint
vet test build build-operator treeshake) so the new builder is exercised by the
default verification flow; verify .PHONY already lists build-builder and adjust
if necessary.
---
Nitpick comments:
In `@pkg/builder/builder_test.go`:
- Around line 69-116: Replace silent New calls in TestWriteGoMod_noExtraRequire
and TestWriteGoMod_extraRequire with an err-checked helper: create a small
helper (e.g. func mustNewBuilder(t *testing.T, cfg Config) *Builder) that calls
t.Helper(), invokes New(cfg), fails the test with t.Fatalf if err != nil, and
returns the builder; then use mustNewBuilder(t, Config{...}) instead of b, _ :=
New(...). Also update other tests noted (TestWriteMain_*, TestCopyGoSum_*) to
use the same mustNewBuilder helper so New errors are surfaced instead of causing
panics.
In `@pkg/builder/builder.go`:
- Around line 213-223: Replace the brittle manual prefix check and unset of
GOFLAGS with strings.HasPrefix when filtering os.Environ() and change the sum DB
disable from the undocumented "GONOSUMDB=*" to the canonical "GOSUMDB=off";
specifically update the loop that builds env (used to set cmd.Env) to use
strings.HasPrefix(e, "GOFLAGS=") and append "GOSUMDB=off" and "GOFLAGS=-mod=mod"
instead of "GONOSUMDB=*" so checksum lookups are reliably disabled.
- Line 148: The generated go.mod in builder.go hardcodes "go 1.24"; update the
code that writes the header (the fmt.Fprintf call producing "module
mcp-anything-custom\n\ngo 1.24\n\nrequire (\n") to read the host module's "go"
directive and reuse it (use golang.org/x/mod/modfile.Parse or a small regex to
read the go directive from the parent module's go.mod), falling back to a single
centralized constant (near pseudoVer) if parsing fails; ensure you reference
this computed goVersion variable when formatting the go.mod instead of the
literal "1.24".
In `@pkg/builder/targets.go`:
- Line 334: Replace the direct comparison against context.Canceled in the proxy
start error check with errors.Is to correctly handle wrapped cancellation
errors: change the condition that checks startErr from "startErr !=
context.Canceled" to use errors.Is(startErr, context.Canceled) (i.e., treat the
error as ignorable when errors.Is(startErr, context.Canceled) is true) and
ensure the "errors" package is imported; this change applies to the
proxy.Start(ctx) call and its startErr handling in targets.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30b5c0fb-2d7a-4a61-9744-57b57cafe633
📒 Files selected for processing (5)
Makefilecmd/builder/main.gopkg/builder/builder.gopkg/builder/builder_test.gopkg/builder/targets.go
| func init() { | ||
| utilruntime.Must(clientgoscheme.AddToScheme(scheme)) | ||
| utilruntime.Must(corev1.AddToScheme(scheme)) | ||
| utilruntime.Must(appsv1.AddToScheme(scheme)) | ||
| utilruntime.Must(v1alpha1.AddToScheme(scheme)) | ||
| utilruntime.Must(gatewayv1.Install(scheme)) | ||
| } |
There was a problem hiding this comment.
Generated operator uses utilruntime.Must, which panics on scheme-registration failure.
The repo guideline says: "Never use panic() anywhere in the codebase — not in library code, not in init(), not in factories, not in tests." utilruntime.Must internally panics via its handler chain, which violates this rule in generated code too. The existing cmd/operator/main.go presumably does the same, so this may be a pre-existing tradeoff for controller-runtime idioms — flagging for your awareness rather than as a blocker for this PR. If you prefer strict compliance, return an error from an initScheme(*runtime.Scheme) error helper and log-and-exit from main.
As per coding guidelines: "Never use panic() anywhere in the codebase — not in library code, not in init(), not in factories, not in tests. All error conditions must be surfaced by returning an error value."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/builder/targets.go` around lines 90 - 96, The init() currently uses
utilruntime.Must which panics on failure; instead implement an initScheme(s
*runtime.Scheme) error that performs the calls to
clientgoscheme.AddToScheme(scheme), corev1.AddToScheme, appsv1.AddToScheme,
v1alpha1.AddToScheme and gatewayv1.Install and returns any error encountered (do
not call utilruntime.Must), then remove the panic-raising init() body and call
initScheme(scheme) from cmd/operator/main.go (or wherever process startup
occurs), logging the error and exiting with non-zero status on failure;
reference the symbols utilruntime.Must, init(), initScheme(*runtime.Scheme),
scheme, and main when making the change.
| func startProxy(cfgPath string) (string, error) { | ||
| // Reserve a local port before starting the proxy so we know the address. | ||
| ln, err := net.Listen("tcp", "127.0.0.1:0") | ||
| if err != nil { | ||
| return "", fmt.Errorf("reserving local port: %w", err) | ||
| } | ||
| addr := ln.Addr().String() | ||
| if err := ln.Close(); err != nil { | ||
| return "", fmt.Errorf("releasing port: %w", err) | ||
| } | ||
|
|
||
| var ( | ||
| path string | ||
| cfg *pkgconfig.ProxyConfig | ||
| ) | ||
| if cfgPath != "" { | ||
| path = cfgPath | ||
| cfg, err = pkgconfig.Load(path) | ||
| } else { | ||
| path, cfg, err = mcpanything.LoadConfig() | ||
| } | ||
| if err != nil { | ||
| return "", fmt.Errorf("loading config: %w", err) | ||
| } | ||
|
|
||
| // Override the listen address with the reserved port. | ||
| cfg.Server.Addr = addr | ||
|
|
||
| ctx := context.Background() | ||
| proxy, err := mcpanything.New(ctx, cfg, mcpanything.WithConfigPath(path)) | ||
| if err != nil { | ||
| return "", fmt.Errorf("creating proxy: %w", err) | ||
| } | ||
| proxy.StartBackground(ctx) | ||
| go func() { | ||
| if startErr := proxy.Start(ctx); startErr != nil && startErr != context.Canceled { | ||
| slog.Error("mcp-anything kong plugin: proxy error", "error", startErr) | ||
| } | ||
| }() | ||
| return addr, nil | ||
| } |
There was a problem hiding this comment.
Kong plugin: port TOCTOU and double server start.
Two issues in startProxy:
-
Port reserve/close race. Lines 301-308 open a listener on
127.0.0.1:0, read the address, and close it before handing the port tomcpanything.New. Another process (or the Go runtime's own ephemeral allocator) may grab that port in the window beforeproxy.Startrebinds, producing a hard-to-debug startup failure on loaded machines. Consider passing the opennet.Listenerdirectly into the proxy instead of round-tripping through an address string, or useaddr := "127.0.0.1:0"and query the actual bound port from the proxy after startup. -
Double-start. Lines 332-337 call
proxy.StartBackground(ctx)and then immediately spawn a goroutine that also callsproxy.Start(ctx). Based on the naming, one of these already runs the server in the background; calling both will either panic on "address already in use" (the second bind) or double-register handlers. Pick exactly one. -
No readiness wait.
startProxyreturns as soon asStartis invoked, so the very first Kong request afterstartOnce.Domay race the listener and get502. A shortnet.DialTimeoutpoll (or usingStartBackground's ready signal if it exposes one) would make this deterministic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/builder/targets.go` around lines 299 - 339, startProxy has a TOCTOU on
port reservation and double-starts the server; fix by changing it to hand an
open net.Listener into the proxy (avoid reserve/close using net.Listen and pass
that listener to mcpanything.New or a new NewWithListener API) instead of
setting cfg.Server.Addr from ln.Addr(), remove the duplicate startup call so
only one of proxy.StartBackground(ctx) or proxy.Start(ctx) is invoked (pick the
background helper and delete the extra goroutine), and block until the server is
actually ready before returning (implement a short readiness poll using
net.DialTimeout to the listener address or use a StartBackground-ready signal if
available) so startProxy reliably returns a bound, accepting address.
|
@claude you must write e2e tests for this with examples similar to remote dev example and kraken. It’s important to use some real use case scenarios for users. For example for caddy and Kong users would have other normal APIs on the proxies. Also Kong has MCPs by itself, so we need to show our proxies capabilities that do not exist in Kong: JavaScript tools. Will need to setup CI too to build all that, because we don’t build during tests to avoid timeouts. |
E2E Tests for Builder
What was added
Deploys
Test verifies: direct HTTP calls through Caddy's reverse proxy work, and the MCP client can call
Deploys the standard
This demonstrates
CI workflow changes requiredSince I cannot modify 1. Add CADDY_BUILDER_IMAGE: ghcr.io/${{ github.repository }}-caddy-builder:sha-${{ github.sha }}2. Add a new build job (runs in parallel with other image builds): build-caddy-builder-image:
name: Build & Push Caddy Builder Image
runs-on: ubuntu-latest
permissions:
contents: read
packages: write
steps:
- uses: actions/checkout@v4
- name: Log in to GHCR
uses: docker/login-action@v3
with:
registry: ghcr.io
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
- name: Build and push
uses: docker/build-push-action@v6
with:
context: .
file: Dockerfile.caddy-builder
push: true
tags: ${{ env.CADDY_BUILDER_IMAGE }}3. Add e2e-tests:
needs: [build-image, build-operator-image, build-workspace-image, build-caddy-builder-image]4. Add the new tests to the include:
# ... existing entries ...
- name: Caddy Builder
test: TestCaddyBuilderE2E
- name: Kong JS Tools
test: TestKongJSToolsE2E5. Pass env:
PROXY_IMAGE: ${{ env.PROXY_IMAGE }}
OPERATOR_IMAGE: ${{ env.OPERATOR_IMAGE }}
CADDY_BUILDER_IMAGE: ${{ env.CADDY_BUILDER_IMAGE }}
E2E_TEST: ${{ matrix.test }}
WORKSPACE_IMAGE: ${{ env.WORKSPACE_IMAGE }}
|
TestCaddyBuilderE2E: deploys a custom Caddy binary built by mcp-anything-builder --target caddy in k3s. WireMock backs a product catalog REST API. Caddy routes /products/* as a normal reverse proxy and /mcp* via the mcpanything handler, verifying that the builder produces a binary that handles both use cases simultaneously. TestKongJSToolsE2E: deploys the standard proxy image configured with a script upstream. Two JavaScript tools (portfolio__calculate_value, portfolio__compare_pairs) use ctx.fetch + JavaScript math to compute crypto portfolio values and compare FX pairs — demonstrating JavaScript tool capabilities that Kong's native MCP plugins lack. Also adds Dockerfile.caddy-builder: a multi-stage Dockerfile that builds the mcp-anything-builder binary, runs it to compile a custom Caddy binary (--target caddy), and packages the result for CI use as CADDY_BUILDER_IMAGE. Co-authored-by: German Arutyunov <gaarutyunov@users.noreply.github.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tests/e2e/kong_js_tools_e2e_test.go (2)
86-100: Deadargs.amountfallback incalculate_value.js.The input schema declares only
coinandquantity(both required, lines 53–62), soargs.amountwill always beundefinedand the||fallback never fires. Either drop it for clarity, or addamountas an accepted alias in the schema if that was the intent.♻️ Proposed fix
- var coin = args.coin; - var quantity = args.amount || args.quantity; + var coin = args.coin; + var quantity = args.quantity;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/kong_js_tools_e2e_test.go` around lines 86 - 100, The script exported as kongJSCalculateValueScript (module.exports = function(args, ctx) { ... }) uses args.amount as a fallback for quantity, but the request schema only defines coin and quantity so args.amount is always undefined; fix this by removing the dead fallback and consistently using args.quantity for quantity, price calculation, and the summary, or alternatively add "amount" as an accepted alias in the input schema so args.amount can be populated—update the function references (quantity assignment, summary construction, and any tests) accordingly to keep naming consistent.
394-445: Consider using consistent constants for WireMock port references.The container, readiness probe, and service port all reference
wiremockPort(lines 397, 402, 422), but the port-forward on line 442 useswiremockAdminPort. Both constants are currently equal (8080), so there is no functional issue. However, using two different constant names interchangeably is a maintainability footgun—if one changes, the inconsistency becomes a silent bug. Either use the same constant throughout, or rename them to clarify the semantic distinction (e.g.,wiremockDefaultPortandwiremockAdminPortif they should ever differ).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/kong_js_tools_e2e_test.go` around lines 394 - 445, The code uses two interchangeable constants wiremockPort and wiremockAdminPort (used in the container, readiness probe, Service, and the portForwardToPod call), which is brittle; unify them by using a single constant for the WireMock port across all uses (e.g., replace wiremockAdminPort with wiremockPort in the portForwardToPod(...) call) or explicitly define clearly distinct constants (e.g., wiremockDefaultPort vs wiremockAdminPort) and update all references (Container spec, readiness probe, Service spec, and the portForwardToPod invocation) so the same semantic port symbol is used everywhere to avoid silent divergence.Dockerfile.caddy-builder (2)
28-32: Optional: drop privileges in the runtime stage.Trivy flags this image for running as root (DS-0002). For a CI-only artifact used by
TestCaddyBuilderE2Ethis is low risk, but since Caddy listens on:8080(not a privileged port), adding a non-rootUSERis trivial and keeps scanners quiet when this image is later reused outside CI.🛡️ Proposed diff
FROM alpine:3.21 RUN apk add --no-cache ca-certificates COPY --from=build /usr/local/bin/caddy /usr/local/bin/caddy +RUN addgroup -S caddy && adduser -S -G caddy caddy +USER caddy EXPOSE 8080 ENTRYPOINT ["caddy", "run", "--config", "/etc/caddy/Caddyfile"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.caddy-builder` around lines 28 - 32, The runtime stage runs Caddy as root which triggers container scanners; create a non-root user and switch to it before the ENTRYPOINT so Caddy still binds to :8080 but runs unprivileged. In the Dockerfile.caddy-builder runtime stage add a minimal user/group (e.g., caddy) and set ownership of any required paths (such as /etc/caddy and /usr/local/bin/caddy) to that user, then add USER caddy (or UID/GID) before ENTRYPOINT so the caddy binary and config are accessible and the process runs non-root.
22-25: Consider pinningCGO_ENABLED=0for the second-stage build.Stage 1 builds
mcp-builderwithCGO_ENABLED=0(line 9), but stage 2 invokesmcp-builder, which internally shells out togo build(seepkg/builder/builder.gocompile) and inherits the container environment. Ingolang:1.25-alpinethere is no C toolchain, so CGO is effectively disabled today, but this is implicit. If a future transitive dependency pulls in a CGO component (or someone addsgccto the image), the resultingcaddybinary could dynamically link against musl and break thealpine:3.21runtime copy-in. Pinning the env explicitly makes the final binary reproducibly static and matches stage 1.♻️ Proposed diff
-RUN /bin/mcp-builder \ +ENV CGO_ENABLED=0 +RUN /bin/mcp-builder \ --target caddy \ --output /usr/local/bin/caddy \ --module-dir /src🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.caddy-builder` around lines 22 - 25, The second-stage RUN invoking /bin/mcp-builder should explicitly set CGO_ENABLED=0 to ensure the final caddy binary is built statically like stage 1; modify the Dockerfile.caddy-builder RUN that calls /bin/mcp-builder (the caddy target/output/module-dir invocation) to export or prefix CGO_ENABLED=0 so the builder's internal go build inherits that environment and prevents accidental dynamic linking.tests/e2e/caddy_builder_e2e_test.go (1)
307-311: Minor: surfacesession.Close()errors on test teardown.
defer session.Close()silently drops the error. On MCP transport teardown this is usually benign, but logging it viat.Logf(or usingt.Cleanupwith an error check) makes it easier to diagnose cases where the server returned an unexpected state during shutdown, which is the kind of thing this e2e is specifically meant to catch. Same low-priority observation applies to the twoio.ReadAll(resp.Body)calls at Lines 280 and 292 whose errors are discarded with_.♻️ Proposed diff
- session, err := mcpClient.Connect(callCtx, mcpTransport, nil) + session, err := mcpClient.Connect(callCtx, mcpTransport, nil) if err != nil { t.Fatalf("connecting MCP client: %v", err) } - defer session.Close() + defer func() { + if err := session.Close(); err != nil { + t.Logf("closing MCP session: %v", err) + } + }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/caddy_builder_e2e_test.go` around lines 307 - 311, The defer session.Close() drops errors—wrap the close in a check and log via the test (e.g., replace defer session.Close() with defer func() { if err := session.Close(); err != nil { t.Logf("session.Close error: %v", err) } }() or use t.Cleanup to call session.Close and log any returned error) and likewise handle the errors returned by io.ReadAll(resp.Body) (check the error and call t.Fatalf/t.Errorf/t.Logf as appropriate after each io.ReadAll call) so teardown and response-read failures are surfaced; look for the session variable returned by mcpClient.Connect and the resp.Body/io.ReadAll calls near the test to apply these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Dockerfile.caddy-builder`:
- Around line 28-32: The runtime stage runs Caddy as root which triggers
container scanners; create a non-root user and switch to it before the
ENTRYPOINT so Caddy still binds to :8080 but runs unprivileged. In the
Dockerfile.caddy-builder runtime stage add a minimal user/group (e.g., caddy)
and set ownership of any required paths (such as /etc/caddy and
/usr/local/bin/caddy) to that user, then add USER caddy (or UID/GID) before
ENTRYPOINT so the caddy binary and config are accessible and the process runs
non-root.
- Around line 22-25: The second-stage RUN invoking /bin/mcp-builder should
explicitly set CGO_ENABLED=0 to ensure the final caddy binary is built
statically like stage 1; modify the Dockerfile.caddy-builder RUN that calls
/bin/mcp-builder (the caddy target/output/module-dir invocation) to export or
prefix CGO_ENABLED=0 so the builder's internal go build inherits that
environment and prevents accidental dynamic linking.
In `@tests/e2e/caddy_builder_e2e_test.go`:
- Around line 307-311: The defer session.Close() drops errors—wrap the close in
a check and log via the test (e.g., replace defer session.Close() with defer
func() { if err := session.Close(); err != nil { t.Logf("session.Close error:
%v", err) } }() or use t.Cleanup to call session.Close and log any returned
error) and likewise handle the errors returned by io.ReadAll(resp.Body) (check
the error and call t.Fatalf/t.Errorf/t.Logf as appropriate after each io.ReadAll
call) so teardown and response-read failures are surfaced; look for the session
variable returned by mcpClient.Connect and the resp.Body/io.ReadAll calls near
the test to apply these changes.
In `@tests/e2e/kong_js_tools_e2e_test.go`:
- Around line 86-100: The script exported as kongJSCalculateValueScript
(module.exports = function(args, ctx) { ... }) uses args.amount as a fallback
for quantity, but the request schema only defines coin and quantity so
args.amount is always undefined; fix this by removing the dead fallback and
consistently using args.quantity for quantity, price calculation, and the
summary, or alternatively add "amount" as an accepted alias in the input schema
so args.amount can be populated—update the function references (quantity
assignment, summary construction, and any tests) accordingly to keep naming
consistent.
- Around line 394-445: The code uses two interchangeable constants wiremockPort
and wiremockAdminPort (used in the container, readiness probe, Service, and the
portForwardToPod call), which is brittle; unify them by using a single constant
for the WireMock port across all uses (e.g., replace wiremockAdminPort with
wiremockPort in the portForwardToPod(...) call) or explicitly define clearly
distinct constants (e.g., wiremockDefaultPort vs wiremockAdminPort) and update
all references (Container spec, readiness probe, Service spec, and the
portForwardToPod invocation) so the same semantic port symbol is used everywhere
to avoid silent divergence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0cca380-5dd7-49ee-ab6d-fbfe5b3d7c3c
📒 Files selected for processing (3)
Dockerfile.caddy-buildertests/e2e/caddy_builder_e2e_test.gotests/e2e/kong_js_tools_e2e_test.go
|
@claude You must use the Kong plugin builder and build it with king for the JavaScript e2e test, not raw proxy. |
|
Claude finished @gaarutyunov's task in 15m 21s —— View job Refactoring Kong JS E2E Test to Use Builder
What changed
CI workflow changes requiredSince I cannot modify 1. Add KONG_BUILDER_IMAGE: ghcr.io/${{ github.repository }}-kong-builder:sha-${{ github.sha }}2. Add a new build job: build-kong-builder-image:
name: Build & Push Kong Builder Image
runs-on: ubuntu-latest
permissions:
contents: read
packages: write
steps:
- uses: actions/checkout@v4
- name: Log in to GHCR
uses: docker/login-action@v3
with:
registry: ghcr.io
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
- name: Build and push
uses: docker/build-push-action@v6
with:
context: .
file: Dockerfile.kong-builder
push: true
tags: ${{ env.KONG_BUILDER_IMAGE }}3. Add e2e-tests:
needs: [build-image, build-operator-image, build-workspace-image, build-caddy-builder-image, build-kong-builder-image]4. Update the include:
# ... existing entries ...
- name: Kong JS Tools
test: TestKongJSToolsE2E5. Pass env:
PROXY_IMAGE: ${{ env.PROXY_IMAGE }}
OPERATOR_IMAGE: ${{ env.OPERATOR_IMAGE }}
CADDY_BUILDER_IMAGE: ${{ env.CADDY_BUILDER_IMAGE }}
KONG_BUILDER_IMAGE: ${{ env.KONG_BUILDER_IMAGE }}
E2E_TEST: ${{ matrix.test }}
WORKSPACE_IMAGE: ${{ env.WORKSPACE_IMAGE }}| Branch |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 229: The CI workflow's timeout-minutes is too low (10) causing
TestCaddyBuilderE2E (and related Caddy Builder / Kong JS Tools matrix cases) to
be killed before their internal 15-minute context; update the workflow to raise
the per-job and per-attempt timeouts to exceed 15 minutes (recommend 20–30
minutes) and, if desired, apply the larger timeout only to the matrix entries
for "Caddy Builder" and "Kong JS Tools" so those jobs get a higher timeout while
others remain unchanged; ensure you update every timeout-minutes key affecting
the job and matrix (including the entries referenced in the comment) so the
action-level and job-level timeouts both allow the test's 15-minute deadline and
leave headroom for retries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| runs-on: ubuntu-latest | ||
| needs: [build-image, build-operator-image, build-workspace-image] | ||
| needs: [build-image, build-operator-image, build-caddy-builder-image, build-workspace-image] | ||
| timeout-minutes: 10 |
There was a problem hiding this comment.
Raise the e2e timeout for the new builder-backed tests.
TestCaddyBuilderE2E allows up to 15 minutes internally, but the workflow kills each attempt after 10 minutes and the whole job after 10 minutes. The new builder/Kong cases can be terminated before their own deadline, making CI flaky or consistently failing on slower runners.
Suggested timeout adjustment
- timeout-minutes: 10
+ timeout-minutes: 65
@@
- timeout_minutes: 10
+ timeout_minutes: 20If runtime is a concern, make this matrix-specific for only Caddy Builder and Kong JS Tools; the key is that the per-attempt timeout must exceed the test’s 15-minute context, and the job timeout must leave room for retries.
Also applies to: 242-245, 257-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml at line 229, The CI workflow's timeout-minutes is
too low (10) causing TestCaddyBuilderE2E (and related Caddy Builder / Kong JS
Tools matrix cases) to be killed before their internal 15-minute context; update
the workflow to raise the per-job and per-attempt timeouts to exceed 15 minutes
(recommend 20–30 minutes) and, if desired, apply the larger timeout only to the
matrix entries for "Caddy Builder" and "Kong JS Tools" so those jobs get a
higher timeout while others remain unchanged; ensure you update every
timeout-minutes key affecting the job and matrix (including the entries
referenced in the comment) so the action-level and job-level timeouts both allow
the test's 15-minute deadline and leave headroom for retries.
Replace raw proxy deployment with a proper Kong Gateway setup: - TestKongJSToolsE2E now requires KONG_BUILDER_IMAGE (Kong image with the mcp-anything Go plugin binary built by `builder --target kong`) - Add Dockerfile.kong-builder: builds the plugin, packages it in kong:3.9 - Fix kongTarget template: use kong.Service.SetTarget instead of X-Forwarded-Host, remove double-start in startProxy, add readiness poll - Fix builder.go: use strings.HasPrefix and GOSUMDB=off (was GONOSUMDB=*) - Add build-builder to Makefile check target The test scenario: Kong handles normal HTTP (GET /rates/* → WireMock) and the mcp-anything plugin adds JavaScript MCP tools (portfolio__calculate_value, portfolio__compare_pairs) that Kong's native MCP support cannot execute. Co-authored-by: German Arutyunov <gaarutyunov@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/e2e/kong_js_tools_e2e_test.go (2)
452-469: Minor:waitForHTTPNon5xxswallows transport errors, making CI timeouts hard to debug.When
c.Getreturns a non-nilerr(DNS failure, connection refused during Kong startup, etc.) the error is ignored andlastStatusstays at its previous value. On context expiration the returned message reportslast status: 0with no clue about the underlying network error, which is exactly the scenario where this warm-up helper is most likely to time out in CI. Consider tracking and surfacing the most recent error too:♻️ Proposed refactor
func waitForHTTPNon5xx(ctx context.Context, c *http.Client, targetURL string) error { - var lastStatus int + var ( + lastStatus int + lastErr error + ) for { resp, err := c.Get(targetURL) //nolint:noctx if err == nil { lastStatus = resp.StatusCode _ = resp.Body.Close() if resp.StatusCode < 500 { return nil } + } else { + lastErr = err } select { case <-ctx.Done(): - return fmt.Errorf("context expired waiting for non-5xx from %s (last status: %d): %w", targetURL, lastStatus, ctx.Err()) + return fmt.Errorf("context expired waiting for non-5xx from %s (last status: %d, last err: %v): %w", targetURL, lastStatus, lastErr, ctx.Err()) case <-time.After(3 * time.Second): } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/kong_js_tools_e2e_test.go` around lines 452 - 469, The helper waitForHTTPNon5xx currently ignores transport errors from c.Get, so on timeout you lose the underlying network error; modify waitForHTTPNon5xx to record the most recent non-nil error (e.g., lastErr) each loop iteration when c.Get returns err, and include that error in the returned fmt.Errorf alongside lastStatus when ctx expires; ensure you still close resp.Body only when resp != nil and preserve existing 3s retry behavior.
551-646:deployKongreturns before the pod is actually up — verify callers always gate on readiness.
deployKongcreates the Deployment and returns; it does not block on pod readiness. The caller at Line 280 doeswaitForDeploymentPod, so the current flow is correct, but a future caller might reasonably expectdeployKongto return a ready deployment (mirroringdeployKongJSWiremock, which does wait internally). Consider either making the helper symmetric by waiting internally, or renaming tocreateKongDeploymentto signal that the caller owns readiness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/kong_js_tools_e2e_test.go` around lines 551 - 646, deployKong currently creates the Kong Deployment but returns before pods are ready (unlike deployKongJSWiremock which waits); either make it symmetric by blocking until a pod is ready or rename it to indicate it only creates the resource. Fix by updating deployKong: after c.Create(...) succeed, call the existing waitForDeploymentPod (or the same readiness logic used by deployKongJSWiremock) to wait for the pod/readiness probe to pass before returning; alternatively, rename the function to createKongDeployment and update all callers (e.g. the caller that currently calls waitForDeploymentPod at line ~280) so the responsibility for readiness is clear.Dockerfile.kong-builder (1)
29-30: Consider pinning the Kong base image by digest and/or asserting a non-root user.
kong:3.9is a floating minor tag; a future rebuild can silently pull a different image. For reproducible e2e runs (especially in CI whereKONG_BUILDER_IMAGEis shared across tests) consider pinning to a specific patch tag (kong:3.9.x) or digest (kong@sha256:...). Thekong:3.9image runs as thekonguser by default, so the Trivy DS-0002 warning is effectively a false positive — but an explicitUSER kongat the bottom makes that intent explicit and guards against base-image regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.kong-builder` around lines 29 - 30, The Dockerfile currently uses the floating tag "kong:3.9" and doesn't assert the runtime user; change the FROM line to pin to a specific patch tag or digest (e.g., replace "kong:3.9" with "kong:3.9.x" or "kong@sha256:...") to ensure reproducible builds, and add an explicit runtime user declaration by appending "USER kong" (or the correct non-root username) at the end of the Dockerfile to make the non-root intent explicit and silence DS-0002.pkg/builder/builder.go (3)
207-230: Asymmetric stream config:Stderris user-configurable butStdoutis hardcoded.
cfg.Stderris overridable;cmd.Stdout = os.Stdoutis not. This makes it awkward for callers (CLI wrappers, tests, image build pipelines) to fully capture thego buildoutput — e.g. a library user wanting to tee both streams into a logger has to re-route globals. Consider adding aStdout io.Writerfield toConfigthat defaults toos.Stdout, mirroringStderr.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/builder/builder.go` around lines 207 - 230, The goCmd function currently writes stdout directly to os.Stdout while stderr honors the configurable b.cfg.Stderr, so add a Stdout io.Writer field to the Config struct (defaulting to os.Stdout) and use it in goCmd (replace cmd.Stdout = os.Stdout with cmd.Stdout = b.cfg.Stdout) to make both streams configurable; ensure Config initialization sets Stdout to os.Stdout when nil and update any constructors/tests to propagate the new field.
159-171: Minor: prefererrors.Is(err, fs.ErrNotExist)overos.IsNotExist.Also,
os.IsNotExistis called before the generic error check, butos.ReadFilecan return wrapped errors (*PathError).os.IsNotExisthandles that case, so behavior is correct today — buterrors.Is(err, fs.ErrNotExist)is the idiomatic replacement and composes better with wrapped errors.♻️ Proposed refactor
- src := filepath.Join(b.cfg.ModuleDir, "go.sum") - data, err := os.ReadFile(src) - if os.IsNotExist(err) { - return nil // module has no go.sum yet - } - if err != nil { - return fmt.Errorf("reading go.sum: %w", err) - } + src := filepath.Join(b.cfg.ModuleDir, "go.sum") + data, err := os.ReadFile(src) + if errors.Is(err, fs.ErrNotExist) { + return nil // module has no go.sum yet + } + if err != nil { + return fmt.Errorf("reading go.sum: %w", err) + }(requires adding
"errors"and"io/fs"to imports)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/builder/builder.go` around lines 159 - 171, In copyGoSum, replace the os.IsNotExist(err) check with the idiomatic errors.Is(err, fs.ErrNotExist) to better handle wrapped errors; keep that existence check before the generic if err != nil branch, and add the required imports ("errors" and "io/fs") to the file so the function (copyGoSum) compiles and preserves the current behavior of returning nil when the module has no go.sum.
147-157: Hardcodedgo 1.24does not match the project's actual Go directive.The root
go.modspecifiesgo 1.25.0, but the generatedgo.modis pinned togo 1.24. This mismatch can causego buildto fail or resolve modules differently than the main project if the parent module relies on language/toolchain features from the newer directive. Usegolang.org/x/mod/modfileto parse the rootgo.modand emit the matching version in the generated module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/builder/builder.go` around lines 147 - 157, The writeGoMod function currently hardcodes "go 1.24"; instead, read and parse the root go.mod to emit the matching "go" directive: use golang.org/x/mod/modfile to load and parse the root module's go.mod (e.g., from b.cfg.ModuleDir), extract the Go version from the parsed modfile.Go setting, and write that value into the generated buffer rather than the literal "1.24"; keep the existing use of localModule, pseudoVer, and t.extraRequire when building the file and return the same os.WriteFile result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/builder/builder.go`:
- Around line 213-224: The code currently unconditionally sets "GOSUMDB=off" in
the env slice (env, cmd.Env) which weakens checksum verification for targets
that fetch external deps; change the env construction so "GOSUMDB=off" is only
appended when the build target does not need downloads (i.e., when
!target.needsDownload or when the local replace is the only concern). Locate the
env/shell block that builds env and sets cmd.Env and conditionally append
"GOSUMDB=off" only when target.needsDownload is false (leave "GOFLAGS=-mod=mod"
handling the same), so external module downloads still consult the sum DB.
---
Nitpick comments:
In `@Dockerfile.kong-builder`:
- Around line 29-30: The Dockerfile currently uses the floating tag "kong:3.9"
and doesn't assert the runtime user; change the FROM line to pin to a specific
patch tag or digest (e.g., replace "kong:3.9" with "kong:3.9.x" or
"kong@sha256:...") to ensure reproducible builds, and add an explicit runtime
user declaration by appending "USER kong" (or the correct non-root username) at
the end of the Dockerfile to make the non-root intent explicit and silence
DS-0002.
In `@pkg/builder/builder.go`:
- Around line 207-230: The goCmd function currently writes stdout directly to
os.Stdout while stderr honors the configurable b.cfg.Stderr, so add a Stdout
io.Writer field to the Config struct (defaulting to os.Stdout) and use it in
goCmd (replace cmd.Stdout = os.Stdout with cmd.Stdout = b.cfg.Stdout) to make
both streams configurable; ensure Config initialization sets Stdout to os.Stdout
when nil and update any constructors/tests to propagate the new field.
- Around line 159-171: In copyGoSum, replace the os.IsNotExist(err) check with
the idiomatic errors.Is(err, fs.ErrNotExist) to better handle wrapped errors;
keep that existence check before the generic if err != nil branch, and add the
required imports ("errors" and "io/fs") to the file so the function (copyGoSum)
compiles and preserves the current behavior of returning nil when the module has
no go.sum.
- Around line 147-157: The writeGoMod function currently hardcodes "go 1.24";
instead, read and parse the root go.mod to emit the matching "go" directive: use
golang.org/x/mod/modfile to load and parse the root module's go.mod (e.g., from
b.cfg.ModuleDir), extract the Go version from the parsed modfile.Go setting, and
write that value into the generated buffer rather than the literal "1.24"; keep
the existing use of localModule, pseudoVer, and t.extraRequire when building the
file and return the same os.WriteFile result.
In `@tests/e2e/kong_js_tools_e2e_test.go`:
- Around line 452-469: The helper waitForHTTPNon5xx currently ignores transport
errors from c.Get, so on timeout you lose the underlying network error; modify
waitForHTTPNon5xx to record the most recent non-nil error (e.g., lastErr) each
loop iteration when c.Get returns err, and include that error in the returned
fmt.Errorf alongside lastStatus when ctx expires; ensure you still close
resp.Body only when resp != nil and preserve existing 3s retry behavior.
- Around line 551-646: deployKong currently creates the Kong Deployment but
returns before pods are ready (unlike deployKongJSWiremock which waits); either
make it symmetric by blocking until a pod is ready or rename it to indicate it
only creates the resource. Fix by updating deployKong: after c.Create(...)
succeed, call the existing waitForDeploymentPod (or the same readiness logic
used by deployKongJSWiremock) to wait for the pod/readiness probe to pass before
returning; alternatively, rename the function to createKongDeployment and update
all callers (e.g. the caller that currently calls waitForDeploymentPod at line
~280) so the responsibility for readiness is clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff71b507-ac9e-47ca-a683-eef49835a785
📒 Files selected for processing (5)
Dockerfile.kong-builderMakefilepkg/builder/builder.gopkg/builder/targets.gotests/e2e/kong_js_tools_e2e_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- pkg/builder/targets.go
|
|
||
| // Inherit env but clear GOFLAGS to avoid interference; disable sum DB so | ||
| // the local replacement module does not require a go.sum entry. | ||
| env := make([]string, 0, len(os.Environ())+2) | ||
| for _, e := range os.Environ() { | ||
| if strings.HasPrefix(e, "GOFLAGS=") { | ||
| continue | ||
| } | ||
| env = append(env, e) | ||
| } | ||
| env = append(env, "GOSUMDB=off", "GOFLAGS=-mod=mod") | ||
| cmd.Env = env |
There was a problem hiding this comment.
GOSUMDB=off silently weakens checksum verification for targets that fetch extra deps.
For kong (needsDownload: true with github.com/Kong/go-pdk), go mod download populates go.sum from downloaded content without consulting a sum DB. If the project's own go.sum doesn't already pin go-pdk (which is not a main-project dep), there is no external verification of the fetched module. The in-code comment justifies the setting only for the local replace directive — worth narrowing the rationale or keeping sum-DB verification on for external deps (e.g. only set GOSUMDB=off when !target.needsDownload, or add GOFLAGS=-insecure=false and rely on GOPROXY/sumdb for third-party modules).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/builder/builder.go` around lines 213 - 224, The code currently
unconditionally sets "GOSUMDB=off" in the env slice (env, cmd.Env) which weakens
checksum verification for targets that fetch external deps; change the env
construction so "GOSUMDB=off" is only appended when the build target does not
need downloads (i.e., when !target.needsDownload or when the local replace is
the only concern). Locate the env/shell block that builds env and sets cmd.Env
and conditionally append "GOSUMDB=off" only when target.needsDownload is false
(leave "GOFLAGS=-mod=mod" handling the same), so external module downloads still
consult the sum DB.
1 participant
Adds
pkg/builderandcmd/builder— a tool that generates and compiles custom mcp-anything binaries containing only the registry-implementing packages the user selects (tree-shaking).Mirrors xcaddy's approach: creates a temp Go module with a replace directive pointing to the local module, copies go.sum for offline transitive-dep verification, renders a target-specific main.go from a template, then runs go build.
Supports targets: proxy, operator, caddy, kong
Closes #126
Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores