feat: TASK-03 MVP Proxy — Happy Path End-to-End#20
Conversation
- Add config structs and koanf-based YAML loader with defaults - Add OpenAPI 3.0 spec loader using kin-openapi with gorillamux router - Add tool generator that walks OpenAPI operations and produces MCP tools with path/query param schemas and HTTP verb-based name slugification - Add MCP server that registers generated tools and dispatches real HTTP requests to the upstream, returning JSON body as TextContent - Add HTTP server with chi router, /healthz, /readyz, and /mcp mount - Replace placeholder main.go with real entrypoint using signal handling - Add integration test with WireMock container verifying full request cycle Co-authored-by: German Arutyunov <gaarutyunov@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughImplements an MVP MCP proxy: loads YAML config, loads and validates an upstream OpenAPI spec, generates MCP tools, registers handlers that translate tool calls to upstream HTTP requests, and exposes a Chi-based HTTP server with /mcp, /healthz, and /readyz plus integration tests against WireMock. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HTTPSrv
participant MCPProxy as Proxy (router/handler)
participant MCPSrv as MCP Server
participant Upstream
Note over HTTPSrv,MCPProxy: Startup: Load config → Load OpenAPI → Generate tools → Register handlers
Client->>HTTPSrv: CONNECT /mcp (streamable HTTP)
HTTPSrv->>MCPProxy: Deliver request
MCPProxy->>MCPSrv: tools/list or tools/call
alt tools/list
MCPSrv-->>MCPProxy: Tool list
MCPProxy-->>Client: Return list
else tools/call
MCPProxy->>MCPSrv: Invoke tool handler (with parsed args)
MCPSrv->>Upstream: HTTP request (method, path with substituted params, query, headers)
Upstream-->>MCPSrv: HTTP response
MCPSrv-->>MCPProxy: Call result (TextContent or IsError with status/body)
MCPProxy-->>Client: Return tool call result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 unit tests (beta)
Comment |
The Claude CI workflow was missing setup-go and golangci-lint installation steps, so golangci-lint was unavailable during Claude's runs. Also fixes a contextcheck lint error in server shutdown. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
internal/server/server.go (1)
27-33:/readyzalways returns 200, even if startup failed.Per the objectives,
/readyzshould return 503 if config load failed. Currently it unconditionally returns 200. If readiness checks need to reflect actual startup state, consider adding a ready flag.For MVP happy-path this may be acceptable, but for proper readiness semantics:
♻️ Sketch of readiness flag approach
type Server struct { cfg *config.ProxyConfig httpServer *http.Server ready atomic.Bool } // In New: r.Get("/readyz", func(w http.ResponseWriter, _ *http.Request) { if !s.ready.Load() { w.WriteHeader(http.StatusServiceUnavailable) return } w.WriteHeader(http.StatusOK) }) // After successful startup: s.ready.Store(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/server.go` around lines 27 - 33, The /readyz handler always returns 200 even if startup failed; add a readiness flag to Server (e.g., ready atomic.Bool) and change the /readyz handler in New (or where routes are registered) to check s.ready.Load() and return 503 when false and 200 when true; after successful startup/config load set s.ready.Store(true) (and ensure the Server type has cfg and httpServer fields as shown so you can access s.ready from the route closure).internal/openapi/generator.go (2)
46-51: Extension value type handling is correct but could handle more cases.The
x-mcp-enabledcheck safely handles the bool type assertion. However, YAML/JSON may parse"false"(string) differently. Consider handling string"false"as well if configs might use quoted values.♻️ Optional: handle string "false"
if val, ok := op.Extensions["x-mcp-enabled"]; ok { if enabled, ok := val.(bool); ok && !enabled { continue } + if s, ok := val.(string); ok && strings.EqualFold(s, "false") { + continue + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/openapi/generator.go` around lines 46 - 51, The current x-mcp-enabled check uses a bool assertion on op.Extensions["x-mcp-enabled"] and skips when false; update the handling in the same block (op.Extensions lookup) to also accept string values by checking the value's type (e.g., type switch or string assertion) and treating case-insensitive "false" (and possibly "true") accordingly so quoted JSON/YAML values like "false" are interpreted as disabled and trigger the same continue; preserve the existing bool path and only continue when the parsed result is false.
40-41: Tool generation order is non-deterministic.
doc.Paths.Map()returns a Go map, and iteration order is not guaranteed. This means the returned[]*GeneratedToolslice may have different ordering across runs, which could affect tests or logging consistency.♻️ Proposed fix: sort paths before iteration
+import "sort" + func GenerateTools(doc *openapi3.T, upstream *config.UpstreamConfig, sep string) ([]*GeneratedTool, error) { var tools []*GeneratedTool - for path, pathItem := range doc.Paths.Map() { + paths := doc.Paths.Map() + pathKeys := make([]string, 0, len(paths)) + for k := range paths { + pathKeys = append(pathKeys, k) + } + sort.Strings(pathKeys) + + for _, path := range pathKeys { + pathItem := paths[path]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/openapi/generator.go` around lines 40 - 41, The iteration over doc.Paths.Map() is non-deterministic because Go map iteration is unordered, causing the returned []*GeneratedTool slice to vary; fix by extracting the keys from doc.Paths.Map() into a slice, sort that slice (e.g., sort.Strings), then iterate over the sorted keys to access pathItem and operations and build the []*GeneratedTool in deterministic order; reference doc.Paths.Map(), the path iteration loop, and the slice of *GeneratedTool so you update the loop that constructs and returns the [] *GeneratedTool to use the sorted keys.internal/config/loader.go (1)
46-52: Consider handlingk.Seterrors for robustness.The
Setcalls ignore errors with_. While unlikely to fail after successfulLoad, handling or at least logging could improve diagnostics.♻️ Optional: log Set failures
func applyDefaults(k *koanf.Koanf) { if !k.Exists("server.port") { - _ = k.Set("server.port", 8080) + if err := k.Set("server.port", 8080); err != nil { + slog.Warn("setting default server.port", "error", err) + } } if !k.Exists("naming.separator") { - _ = k.Set("naming.separator", "__") + if err := k.Set("naming.separator", "__"); err != nil { + slog.Warn("setting default naming.separator", "error", err) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/loader.go` around lines 46 - 52, The applyDefaults function currently ignores errors returned by k.Set on the koanf.Koanf instance; update applyDefaults to check the error result of each k.Set call (for keys "server.port" and "naming.separator") and handle failures by either logging the error with context (e.g., "failed to set server.port" / "failed to set naming.separator") or returning the error up to the caller so callers can react; locate and modify the applyDefaults function and the k.Set calls to perform error checking and appropriate logging/propagation.internal/mcp/mvp_integration_test.go (1)
103-111: Consider checkingClose()errors on temp files.
specFile.Close()on line 111 (andcfgFile.Close()on line 139) ignore errors. While unlikely to cause issues in tests, unchecked close errors can mask underlying I/O problems.♻️ Optional: check close errors
- specFile.Close() + if err := specFile.Close(); err != nil { + t.Fatalf("close spec file: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/mvp_integration_test.go` around lines 103 - 111, The temp file Close() calls for specFile (and cfgFile) are unchecked; update the test to capture and handle any error returned by specFile.Close() (and cfgFile.Close()) after writing testOpenAPISpec (and after writing cfg contents) and fail the test if Close() returns an error (e.g., use t.Fatalf or t.Fatalf-like assertion). Locate the specFile.WriteString/testOpenAPISpec block and the corresponding cfgFile write block and add error checks for specFile.Close() and cfgFile.Close() to surface I/O issues.internal/mcp/server.go (1)
67-70: Consider limiting response body size.
io.ReadAllreads the entire response into memory without bounds. A misbehaving upstream could return extremely large responses, potentially exhausting memory.♻️ Proposed fix using io.LimitReader
+const maxResponseBodySize = 10 * 1024 * 1024 // 10MB + - body, err := io.ReadAll(resp.Body) + body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBodySize)) if err != nil { return nil, fmt.Errorf("reading response body: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/server.go` around lines 67 - 70, The code uses io.ReadAll(resp.Body) which can OOM on very large responses; change it to read via io.LimitReader with a defined limit (e.g., add const MaxResponseBodySize = 10 << 20) and replace io.ReadAll(resp.Body) with io.ReadAll(io.LimitReader(resp.Body, MaxResponseBodySize)); keep the same error wrapping but detect/return a clear error when the body exceeds the limit (e.g., "response body too large") and ensure resp.Body is still closed as before; the relevant symbols to update are the io.ReadAll call and resp.Body usage in the function handling the HTTP response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/proxy/main.go`:
- Around line 36-42: Before using cfg.Upstreams[0] add a bounds check on
cfg.Upstreams (e.g., if len(cfg.Upstreams) == 0) and log a clear error and exit;
specifically, in cmd/proxy/main.go where you set upstream := cfg.Upstreams[0]
and call openapi.Load, verify the slice is non-empty, emit a descriptive error
via slog.Error (mention "no upstreams configured") and os.Exit(1) before
attempting to access Upstreams[0], so the subsequent openapi.Load call always
receives a valid upstream.
In `@go.mod`:
- Line 24: The go.mod currently marks packages that are directly imported as
indirect; remove the "// indirect" markers (or move those entries into the
top-level direct require block) for github.com/getkin/kin-openapi,
github.com/knadh/koanf/parsers/yaml, github.com/knadh/koanf/providers/file,
github.com/knadh/koanf/v2, and github.com/modelcontextprotocol/go-sdk (these are
imported in internal/openapi/loader.go, internal/openapi/generator.go,
internal/config/loader.go, and internal/mcp/server.go); the easiest fix is to
run `go mod tidy` to automatically correct the require entries and then commit
the updated go.mod.
- Line 54: The go.mod currently pins github.com/modelcontextprotocol/go-sdk at
v0.3.0 which has critical vulnerabilities; update the module requirement to
github.com/modelcontextprotocol/go-sdk v1.4.1, run go mod tidy (or go get
github.com/modelcontextprotocol/go-sdk@v1.4.1) to update the lockfile, and
ensure CI/tooling use Go 1.25+ (since v1.4.1 requires Go 1.25); verify builds
and tests pass after the change.
- Line 25: The go.mod entry for github.com/go-chi/chi/v5 is incorrectly marked
as indirect even though it is directly imported (see internal/server/server.go);
remove the `// indirect` marker by updating modules: run `go mod tidy` (or
manually remove the comment and run `go mod tidy`) to refresh go.mod and go.sum
so chi/v5 is recorded as a direct dependency.
In `@internal/mcp/server.go`:
- Around line 124-130: The loop replacing path parameters in server.go currently
injects raw values (using fmt.Sprintf("%v", val)) which can break URLs; update
the replacement to URL-encode each parameter using
url.PathEscape(fmt.Sprintf("%v", val)) (or net/url equivalent) before calling
strings.ReplaceAll so special characters like /, ?, # are escaped; ensure the
code imports the net/url package and apply this change where pathParams, args,
and strings.ReplaceAll are used.
In `@internal/server/server.go`:
- Around line 67-77: Replace the fresh background context used for shutdown with
a context that preserves values/tracing from the incoming ctx but is not
cancelable: use context.WithoutCancel(ctx) as the parent when creating the
shutdown timeout context used for s.httpServer.Shutdown so that tracing/values
are retained (i.e., create shutdownCtx via
context.WithTimeout(context.WithoutCancel(ctx), s.cfg.Server.ShutdownTimeout)
and keep the existing cancel/defer and Shutdown call). Ensure this change
compiles against Go 1.21+ or, if supporting older Go versions, add a concise
lint-suppressing comment explaining why context.Background() was previously
used.
---
Nitpick comments:
In `@internal/config/loader.go`:
- Around line 46-52: The applyDefaults function currently ignores errors
returned by k.Set on the koanf.Koanf instance; update applyDefaults to check the
error result of each k.Set call (for keys "server.port" and "naming.separator")
and handle failures by either logging the error with context (e.g., "failed to
set server.port" / "failed to set naming.separator") or returning the error up
to the caller so callers can react; locate and modify the applyDefaults function
and the k.Set calls to perform error checking and appropriate
logging/propagation.
In `@internal/mcp/mvp_integration_test.go`:
- Around line 103-111: The temp file Close() calls for specFile (and cfgFile)
are unchecked; update the test to capture and handle any error returned by
specFile.Close() (and cfgFile.Close()) after writing testOpenAPISpec (and after
writing cfg contents) and fail the test if Close() returns an error (e.g., use
t.Fatalf or t.Fatalf-like assertion). Locate the
specFile.WriteString/testOpenAPISpec block and the corresponding cfgFile write
block and add error checks for specFile.Close() and cfgFile.Close() to surface
I/O issues.
In `@internal/mcp/server.go`:
- Around line 67-70: The code uses io.ReadAll(resp.Body) which can OOM on very
large responses; change it to read via io.LimitReader with a defined limit
(e.g., add const MaxResponseBodySize = 10 << 20) and replace
io.ReadAll(resp.Body) with io.ReadAll(io.LimitReader(resp.Body,
MaxResponseBodySize)); keep the same error wrapping but detect/return a clear
error when the body exceeds the limit (e.g., "response body too large") and
ensure resp.Body is still closed as before; the relevant symbols to update are
the io.ReadAll call and resp.Body usage in the function handling the HTTP
response.
In `@internal/openapi/generator.go`:
- Around line 46-51: The current x-mcp-enabled check uses a bool assertion on
op.Extensions["x-mcp-enabled"] and skips when false; update the handling in the
same block (op.Extensions lookup) to also accept string values by checking the
value's type (e.g., type switch or string assertion) and treating
case-insensitive "false" (and possibly "true") accordingly so quoted JSON/YAML
values like "false" are interpreted as disabled and trigger the same continue;
preserve the existing bool path and only continue when the parsed result is
false.
- Around line 40-41: The iteration over doc.Paths.Map() is non-deterministic
because Go map iteration is unordered, causing the returned []*GeneratedTool
slice to vary; fix by extracting the keys from doc.Paths.Map() into a slice,
sort that slice (e.g., sort.Strings), then iterate over the sorted keys to
access pathItem and operations and build the []*GeneratedTool in deterministic
order; reference doc.Paths.Map(), the path iteration loop, and the slice of
*GeneratedTool so you update the loop that constructs and returns the []
*GeneratedTool to use the sorted keys.
In `@internal/server/server.go`:
- Around line 27-33: The /readyz handler always returns 200 even if startup
failed; add a readiness flag to Server (e.g., ready atomic.Bool) and change the
/readyz handler in New (or where routes are registered) to check s.ready.Load()
and return 503 when false and 200 when true; after successful startup/config
load set s.ready.Store(true) (and ensure the Server type has cfg and httpServer
fields as shown so you can access s.ready from the route closure).
🪄 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: 8c4cfe0e-933f-40e1-95ef-74e8779c8939
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
cmd/proxy/main.gogo.modinternal/config/config.gointernal/config/loader.gointernal/mcp/mvp_integration_test.gointernal/mcp/server.gointernal/openapi/generator.gointernal/openapi/loader.gointernal/server/server.go
| // For this task: single upstream only. | ||
| upstream := cfg.Upstreams[0] | ||
| doc, router, err := openapi.Load(ctx, upstream.OpenAPI) | ||
| if err != nil { | ||
| slog.Error("load openapi spec", "upstream", upstream.Name, "error", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
Add bounds check before accessing Upstreams[0].
If the config file has an empty upstreams array, this will panic with an unhelpful "index out of range" error. A clearer error message improves operator experience.
🛡️ Proposed fix
// For this task: single upstream only.
+ if len(cfg.Upstreams) == 0 {
+ slog.Error("no upstreams configured")
+ os.Exit(1)
+ }
upstream := cfg.Upstreams[0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // For this task: single upstream only. | |
| upstream := cfg.Upstreams[0] | |
| doc, router, err := openapi.Load(ctx, upstream.OpenAPI) | |
| if err != nil { | |
| slog.Error("load openapi spec", "upstream", upstream.Name, "error", err) | |
| os.Exit(1) | |
| } | |
| // For this task: single upstream only. | |
| if len(cfg.Upstreams) == 0 { | |
| slog.Error("no upstreams configured") | |
| os.Exit(1) | |
| } | |
| upstream := cfg.Upstreams[0] | |
| doc, router, err := openapi.Load(ctx, upstream.OpenAPI) | |
| if err != nil { | |
| slog.Error("load openapi spec", "upstream", upstream.Name, "error", err) | |
| os.Exit(1) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/proxy/main.go` around lines 36 - 42, Before using cfg.Upstreams[0] add a
bounds check on cfg.Upstreams (e.g., if len(cfg.Upstreams) == 0) and log a clear
error and exit; specifically, in cmd/proxy/main.go where you set upstream :=
cfg.Upstreams[0] and call openapi.Load, verify the slice is non-empty, emit a
descriptive error via slog.Error (mention "no upstreams configured") and
os.Exit(1) before attempting to access Upstreams[0], so the subsequent
openapi.Load call always receives a valid upstream.
| github.com/ebitengine/purego v0.8.2 // indirect | ||
| github.com/felixge/httpsnoop v1.0.4 // indirect | ||
| github.com/fsnotify/fsnotify v1.7.0 // indirect | ||
| github.com/getkin/kin-openapi v0.134.0 // indirect |
There was a problem hiding this comment.
Fix incorrect indirect dependency markers.
The following packages are directly imported in your codebase but are marked as // indirect:
- Line 24:
github.com/getkin/kin-openapi(imported ininternal/openapi/loader.goandinternal/openapi/generator.go) - Lines 40-42:
github.com/knadh/koanf/parsers/yaml,github.com/knadh/koanf/providers/file, andgithub.qkg1.top/knadh/koanf/v2(imported ininternal/config/loader.go) - Line 54:
github.com/modelcontextprotocol/go-sdk(imported ininternal/openapi/generator.goandinternal/mcp/server.go)
Dependencies must be marked as direct (without // indirect) when they are explicitly imported. Running go mod tidy will fix this automatically.
🔧 Proposed fix
These dependencies should be moved to a direct require block at the top of the file or have the // indirect comment removed. Run:
go mod tidyThis will reorganize the dependencies correctly.
Also applies to: 40-42, 54-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 24, The go.mod currently marks packages that are directly
imported as indirect; remove the "// indirect" markers (or move those entries
into the top-level direct require block) for github.com/getkin/kin-openapi,
github.com/knadh/koanf/parsers/yaml, github.com/knadh/koanf/providers/file,
github.com/knadh/koanf/v2, and github.com/modelcontextprotocol/go-sdk (these are
imported in internal/openapi/loader.go, internal/openapi/generator.go,
internal/config/loader.go, and internal/mcp/server.go); the easiest fix is to
run `go mod tidy` to automatically correct the require entries and then commit
the updated go.mod.
| github.com/felixge/httpsnoop v1.0.4 // indirect | ||
| github.com/fsnotify/fsnotify v1.7.0 // indirect | ||
| github.com/getkin/kin-openapi v0.134.0 // indirect | ||
| github.com/go-chi/chi/v5 v5.2.1 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if go-chi/chi/v5 is directly imported in the codebase.
# Search for direct imports of chi/v5
rg -n --type=go 'github\.com/go-chi/chi/v5' -g '!vendor/**' -g '!go.mod' -g '!go.sum'Repository: gaarutyunov/mcp-anything
Length of output: 1790
🏁 Script executed:
#!/bin/bash
# Check if go-chi/chi/v5 is directly imported in the codebase
rg -n --type=go 'github\.com/go-chi/chi/v5' -g '!vendor/**' -g '!go.mod' -g '!go.sum'Repository: gaarutyunov/mcp-anything
Length of output: 124
Remove the // indirect marker from chi/v5 dependency.
github.com/go-chi/chi/v5 is directly imported in internal/server/server.go, so it should not be marked as indirect in go.mod. Run go mod tidy to correct this.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 25, The go.mod entry for github.com/go-chi/chi/v5 is
incorrectly marked as indirect even though it is directly imported (see
internal/server/server.go); remove the `// indirect` marker by updating modules:
run `go mod tidy` (or manually remove the comment and run `go mod tidy`) to
refresh go.mod and go.sum so chi/v5 is recorded as a direct dependency.
| github.com/moby/sys/user v0.1.0 // indirect | ||
| github.com/moby/sys/userns v0.1.0 // indirect | ||
| github.com/moby/term v0.5.0 // indirect | ||
| github.com/modelcontextprotocol/go-sdk v0.3.0 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest version of github.com/modelcontextprotocol/go-sdk and does it fix the vulnerabilities GHSA-89xv-2j6f-qhc8, GHSA-q382-vc8q-7jhj, and GHSA-wvj2-96wp-fq3f?
💡 Result:
The latest version of github.com/modelcontextprotocol/go-sdk is v1.4.1, released on 2026-03-13.
Citations:
- 1: https://github.com/modelcontextprotocol/go-sdk
- 2: https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk
- 3: https://newreleases.io/project/github/modelcontextprotocol/go-sdk/release/v1.4.1
🌐 Web query:
GHSA-89xv-2j6f-qhc8 modelcontextprotocol go-sdk fixed version
💡 Result:
The fixed version for GHSA-89xv-2j6f-qhc8 in the modelcontextprotocol/go-sdk is v1.4.1. This vulnerability affects versions <= 1.4.0 and was patched in v1.4.1, which adds Content-Type header validation for POST requests and configurable origin verification. Note that v1.4.1 requires Go 1.25 or later. The issue is a high-severity (7.1 CVSS) Cross-Site Tool Execution (CSRF, CWE-352) in the Streamable HTTP transport for deployments without authorization. CVE-2026-33252, GO-2026-4773.
Citations:
🌐 Web query:
GHSA-q382-vc8q-7jhj go-sdk vulnerability fix
💡 Result:
GHSA-q382-vc8q-7jhj is a vulnerability in the modelcontextprotocol/go-sdk Go package. It involves improper handling of duplicate keys in JSON parsing during MCP message processing, where "last key wins" behavior could override intended messages when combined with duplicate keys. This was listed in Chainguard security advisories (CGA-hv38-3wq2-mc2v) as affecting the package in jaeger-2-query component, with fixed version 2.16.0-r3 in their images. The upstream go-sdk fixed related case-insensitivity issues (CVE-2026-27896, GHSA-wvj2-96wp-fq3f) in v1.3.1 and cross-site protections in v1.4.1. Update to modelcontextprotocol/go-sdk >= v1.4.1 (latest as of 2026-03-13) or Chainguard jaeger-2-query >= 2.16.0-r3.
Citations:
- 1: https://test.osv.dev/vulnerability/GHSA-q382-vc8q-7jhj
- 2: https://images.chainguard.dev/security/CGA-hv38-3wq2-mc2v
- 3: https://github.com/modelcontextprotocol/go-sdk/releases/tag/v1.4.0
- 4: https://newreleases.io/project/github/modelcontextprotocol/go-sdk/release/v1.3.0
- 5: https://github.com/modelcontextprotocol/go-sdk
- 6: GHSA-89xv-2j6f-qhc8
- 7: GHSA-wvj2-96wp-fq3f
- 8: https://newreleases.io/project/github/modelcontextprotocol/go-sdk/release/v1.4.1
🌐 Web query:
GHSA-wvj2-96wp-fq3f go-sdk vulnerability fix
💡 Result:
The GHSA-wvj2-96wp-fq3f vulnerability affects the MCP (Model Context Protocol) Go SDK (github.com/modelcontextprotocol/go-sdk) due to improper handling of case sensitivity in JSON unmarshaling. Go's standard encoding/json.Unmarshal performs case-insensitive matching of JSON keys to struct field tags, violating JSON-RPC 2.0 specification requirements for exact field names. This could allow a malicious MCP peer to send messages with non-standard casing, potentially causing cross-implementation inconsistencies or security-boundary confusion. Affected versions: prior to v1.3.1. Fix: Update to v1.3.1 or later. The fix replaces Go's standard JSON unmarshaling with a case-sensitive decoder from github.com/segmentio/encoding in commit 7b8d81c264074404abdf5aa16e2cf0c2d9c64cc0. Official advisory: GHSA-wvj2-96wp-fq3f CVE: CVE-2026-27896 Severity: CVSS 4.0 score 7.0 (High)
Citations:
- 1: https://db.gcve.eu/vuln/ghsa-wvj2-96wp-fq3f
- 2: GHSA-wvj2-96wp-fq3f
- 3: https://mondoo.com/vulnerability-intelligence/vulnerability/GHSA-wvj2-96wp-fq3f
- 4: https://osv.dev/vulnerability/GHSA-wvj2-96wp-fq3f
- 5: https://github.com/modelcontextprotocol/go-sdk
Update github.com/modelcontextprotocol/go-sdk to v1.4.1 to fix critical vulnerabilities.
Version 0.3.0 contains three HIGH severity vulnerabilities:
- GHSA-89xv-2j6f-qhc8 (CVSS 7.1): Cross-Site Tool Execution (CSRF) in the Streamable HTTP transport allows unauthenticated attackers to trigger tool execution.
- GHSA-wvj2-96wp-fq3f (CVSS 7.0): Improper case-sensitive JSON unmarshaling violates JSON-RPC 2.0 spec and could enable cross-implementation attacks.
- GHSA-q382-vc8q-7jhj: Improper handling of duplicate keys in JSON parsing during MCP message processing.
All three are fixed in v1.4.1 (latest, released 2026-03-13). Note: v1.4.1 requires Go 1.25 or later.
🧰 Tools
🪛 OSV Scanner (2.3.5)
[HIGH] 54-54: github.com/modelcontextprotocol/go-sdk 0.3.0: MCP Go SDK Vulnerable to Improper Handling of Case Sensitivity in github.com/modelcontextprotocol/go-sdk
(GO-2026-4569)
[HIGH] 54-54: github.com/modelcontextprotocol/go-sdk 0.3.0: Improper handling of null Unicode character when parsing JSON in github.com/modelcontextprotocol/go-sdk
(GO-2026-4770)
[HIGH] 54-54: github.com/modelcontextprotocol/go-sdk 0.3.0: Cross-Site Tool Execution for HTTP Servers without Authorizatrion in github.com/modelcontextprotocol/go-sdk
(GO-2026-4773)
[HIGH] 54-54: github.com/modelcontextprotocol/go-sdk 0.3.0: Cross-Site Tool Execution for HTTP Servers without Authorizatrion in github.com/modelcontextprotocol/go-sdk
[HIGH] 54-54: github.com/modelcontextprotocol/go-sdk 0.3.0: Improper handling of null Unicode character when parsing JSON in github.com/modelcontextprotocol/go-sdk
[HIGH] 54-54: github.com/modelcontextprotocol/go-sdk 0.3.0: MCP Go SDK Vulnerable to Improper Handling of Case Sensitivity
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 54, The go.mod currently pins
github.com/modelcontextprotocol/go-sdk at v0.3.0 which has critical
vulnerabilities; update the module requirement to
github.com/modelcontextprotocol/go-sdk v1.4.1, run go mod tidy (or go get
github.com/modelcontextprotocol/go-sdk@v1.4.1) to update the lockfile, and
ensure CI/tooling use Go 1.25+ (since v1.4.1 requires Go 1.25); verify builds
and tests pass after the change.
| for _, name := range pathParams { | ||
| val, ok := args[name] | ||
| if !ok { | ||
| return "", fmt.Errorf("missing required path parameter %q", name) | ||
| } | ||
| path = strings.ReplaceAll(path, "{"+name+"}", fmt.Sprintf("%v", val)) | ||
| } |
There was a problem hiding this comment.
Path parameter values should be URL-encoded.
Using fmt.Sprintf("%v", val) embeds raw values into the URL path. Special characters in parameter values (e.g., /, ?, #) could break URL parsing or cause unexpected routing.
🛡️ Proposed fix
for _, name := range pathParams {
val, ok := args[name]
if !ok {
return "", fmt.Errorf("missing required path parameter %q", name)
}
- path = strings.ReplaceAll(path, "{"+name+"}", fmt.Sprintf("%v", val))
+ path = strings.ReplaceAll(path, "{"+name+"}", url.PathEscape(fmt.Sprintf("%v", val)))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, name := range pathParams { | |
| val, ok := args[name] | |
| if !ok { | |
| return "", fmt.Errorf("missing required path parameter %q", name) | |
| } | |
| path = strings.ReplaceAll(path, "{"+name+"}", fmt.Sprintf("%v", val)) | |
| } | |
| for _, name := range pathParams { | |
| val, ok := args[name] | |
| if !ok { | |
| return "", fmt.Errorf("missing required path parameter %q", name) | |
| } | |
| path = strings.ReplaceAll(path, "{"+name+"}", url.PathEscape(fmt.Sprintf("%v", val))) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/mcp/server.go` around lines 124 - 130, The loop replacing path
parameters in server.go currently injects raw values (using fmt.Sprintf("%v",
val)) which can break URLs; update the replacement to URL-encode each parameter
using url.PathEscape(fmt.Sprintf("%v", val)) (or net/url equivalent) before
calling strings.ReplaceAll so special characters like /, ?, # are escaped;
ensure the code imports the net/url package and apply this change where
pathParams, args, and strings.ReplaceAll are used.
| select { | ||
| case <-ctx.Done(): | ||
| shutdownCtx, cancel := context.WithTimeout(context.Background(), s.cfg.Server.ShutdownTimeout) | ||
| defer cancel() | ||
| if err := s.httpServer.Shutdown(shutdownCtx); err != nil { | ||
| return fmt.Errorf("graceful shutdown: %w", err) | ||
| } | ||
| return ctx.Err() | ||
| case err := <-errCh: | ||
| return err | ||
| } |
There was a problem hiding this comment.
Fix linter error: use context.WithoutCancel to satisfy contextcheck.
The pipeline fails because context.Background() on line 71 doesn't inherit from ctx. While the intention is correct (shutdown needs a fresh context), using context.WithoutCancel(ctx) preserves tracing/values while avoiding immediate cancellation.
🔧 Proposed fix
case <-ctx.Done():
- shutdownCtx, cancel := context.WithTimeout(context.Background(), s.cfg.Server.ShutdownTimeout)
+ shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), s.cfg.Server.ShutdownTimeout)
defer cancel()Note: context.WithoutCancel was added in Go 1.21. If targeting an earlier version, you may need to suppress the lint rule with a comment explaining the rationale.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| select { | |
| case <-ctx.Done(): | |
| shutdownCtx, cancel := context.WithTimeout(context.Background(), s.cfg.Server.ShutdownTimeout) | |
| defer cancel() | |
| if err := s.httpServer.Shutdown(shutdownCtx); err != nil { | |
| return fmt.Errorf("graceful shutdown: %w", err) | |
| } | |
| return ctx.Err() | |
| case err := <-errCh: | |
| return err | |
| } | |
| select { | |
| case <-ctx.Done(): | |
| shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), s.cfg.Server.ShutdownTimeout) | |
| defer cancel() | |
| if err := s.httpServer.Shutdown(shutdownCtx); err != nil { | |
| return fmt.Errorf("graceful shutdown: %w", err) | |
| } | |
| return ctx.Err() | |
| case err := <-errCh: | |
| return err | |
| } |
🧰 Tools
🪛 GitHub Actions: CI
[error] 71-71: golangci-lint: Non-inherited new context; use function like context.WithXXX instead (contextcheck)
🪛 GitHub Check: Lint, Vet, Unit Tests, Build
[failure] 71-71:
Non-inherited new context, use function like context.WithXXX instead (contextcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/server.go` around lines 67 - 77, Replace the fresh background
context used for shutdown with a context that preserves values/tracing from the
incoming ctx but is not cancelable: use context.WithoutCancel(ctx) as the parent
when creating the shutdown timeout context used for s.httpServer.Shutdown so
that tracing/values are retained (i.e., create shutdownCtx via
context.WithTimeout(context.WithoutCancel(ctx), s.cfg.Server.ShutdownTimeout)
and keep the existing cancel/defer and Shutdown call). Ensure this change
compiles against Go 1.21+ or, if supporting older Go versions, add a concise
lint-suppressing comment explaining why context.Background() was previously
used.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/claude.yml:
- Around line 38-42: Remove the unsupported install-only input from the
golangci/golangci-lint-action@v7 step and replace that installation behavior
with an explicit go install command run in a separate run step; specifically,
remove the install-only: true line from the action invocation and add a new
workflow run step that executes the appropriate go install ...@latest (or pinned
version) to install golangci-lint, ensuring the workflow uses
golangci/golangci-lint-action@v7 only for linting invocation and not for
installation.
In `@internal/server/server.go`:
- Around line 73-77: The returns that currently return raw errors from the
select (the return ctx.Err() and return err from the errCh case) must be wrapped
with operation context using fmt.Errorf; locate the select handling around errCh
and ctx and replace those bare returns with wrapped errors (e.g.,
fmt.Errorf("graceful shutdown: %w", ctx.Err()) or fmt.Errorf("server run error:
%w", err)) so that both the ctx.Err() and the err received from errCh are
wrapped consistently with descriptive context using fmt.Errorf("<context>: %w",
...).
🪄 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: 2abec72b-c8c9-4d9f-8882-0daad243f260
📒 Files selected for processing (2)
.github/workflows/claude.ymlinternal/server/server.go
| return fmt.Errorf("graceful shutdown: %w", err) | ||
| } | ||
| return ctx.Err() | ||
| case err := <-errCh: | ||
| return err |
There was a problem hiding this comment.
Wrap returned errors with operation context.
A few return paths still return raw errors, which violates the Go error-wrapping rule in this repo.
🔧 Suggested fix
case <-ctx.Done():
shutdownCtx, cancel := context.WithTimeout(context.Background(), s.cfg.Server.ShutdownTimeout)
defer cancel()
//nolint:contextcheck // parent ctx is cancelled; shutdown needs a fresh context with its own timeout
if err := s.httpServer.Shutdown(shutdownCtx); err != nil {
return fmt.Errorf("graceful shutdown: %w", err)
}
- return ctx.Err()
+ return fmt.Errorf("start context cancelled: %w", ctx.Err())
case err := <-errCh:
- return err
+ if err != nil {
+ return fmt.Errorf("listen and serve: %w", err)
+ }
+ return nil
}
}
// Shutdown gracefully stops the server.
func (s *Server) Shutdown(ctx context.Context) error {
- return s.httpServer.Shutdown(ctx)
+ if err := s.httpServer.Shutdown(ctx); err != nil {
+ return fmt.Errorf("shutdown: %w", err)
+ }
+ return nil
}As per coding guidelines, **/*.go: All errors must be wrapped with context using fmt.Errorf("<context>: %w", err) format.
Also applies to: 82-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/server.go` around lines 73 - 77, The returns that currently
return raw errors from the select (the return ctx.Err() and return err from the
errCh case) must be wrapped with operation context using fmt.Errorf; locate the
select handling around errCh and ctx and replace those bare returns with wrapped
errors (e.g., fmt.Errorf("graceful shutdown: %w", ctx.Err()) or
fmt.Errorf("server run error: %w", err)) so that both the ctx.Err() and the err
received from errCh are wrapped consistently with descriptive context using
fmt.Errorf("<context>: %w", ...).
… image Integration tests now run against a pre-built Docker image pulled from GHCR instead of importing internal packages directly. CI builds and pushes the image in a separate job before running integration tests. - Add Dockerfile (multi-stage, golang:1.25-alpine) - Add build-image CI job that pushes to ghcr.io with SHA-based tag - Test reads image from PROXY_IMAGE env var (default: dev tag) - Add weekly GHCR cleanup workflow (keeps 5 latest, preserves semver) - Remove old internal integration tests and testutil package Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove deprecated `exclude-use-default` from .golangci.yml (removed in v2) - Pin golangci-lint v2.7.2 in CI action and Claude workflow - Use `go install` with pinned version in claude.yml Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move linters-settings to linters.settings / formatters.settings - Change local-prefixes from string to array Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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:
- Around line 29-35: The workflow uses the golangci/golangci-lint-action@v7 step
named "Install golangci-lint" with the unsupported input install-only and then
runs a separate shell step "golangci-lint"; replace the two-step pattern by
updating the action invocation (golangci/golangci-lint-action@v7) to use the
supported input install-mode (e.g., install-mode: install) or remove that key
entirely and pass args: ./... to the single action step, then delete the
separate run: golangci-lint shell step so linting runs directly from the action.
In `@Dockerfile`:
- Around line 9-12: The container runs the proxy as root; create a non-root
user/group (e.g., proxy) in the Dockerfile, chown the installed binary and any
required runtime dirs to that user (referencing COPY --from=builder /proxy
/usr/local/bin/proxy and the ENTRYPOINT ["proxy"]), and switch to that user with
USER before the ENTRYPOINT; ensure any files or sockets the proxy needs are
owned/writable by that user and adjust WORKDIR if needed.
In `@tests/integration/mvp_test.go`:
- Around line 192-213: The test only checks tool names but must also assert each
tool's InputSchema to avoid regressions; update the block that inspects
toolsResult.Tools (and uses toolListPets / toolGetPet) to find the tool by Name
and then validate its InputSchema: for test__list_pets assert the schema
includes a "limit" query parameter (correct type/format and whether it's
required or optional per spec), and for test__get_pets_petid assert the path
parameter "petId" is present and marked required in the InputSchema; use the
existing toolsResult.Tools iteration and toolNames helper to locate each tool
and add concise assertions on the InputSchema/Parameters rather than only
checking names.
- Around line 24-30: The test currently falls back to a hard-coded shared image
in defaultProxyImage when PROXY_IMAGE is unset; change proxyImage to not
silently use that dev image—either remove defaultProxyImage and make proxyImage
fail fast (log.Fatalf or panic) with a clear message instructing to set
PROXY_IMAGE, or derive a local image tag built from the current checkout (e.g.,
use a BUILD_IMAGE env or git commit tag) and return that; update references to
proxyImage() accordingly so tests always run against an explicitly provided or
locally-built image rather than ghcr.io/...:dev.
- Around line 309-320: The test currently only asserts len(result.Requests) >= 2
which misses wrong routing; update the assertion to verify the actual URLs
WireMock received by inspecting result.Requests[].Request.URL (the anonymous
struct in the test) and assert that the expected endpoint(s) were called (e.g.,
contains "/pets" and the other expected path such as "/pets/1" or the specific
URL used by test__list_pets). Locate the unmarshalling and result variable in
the test (the struct with Requests -> Request -> URL) and add checks that the
slice of URLs includes the exact expected values (or at least one request
matching the test__list_pets expected URL) and fail the test with a helpful
message if not.
- Around line 1-3: Rename the test file from mvp_test.go to follow the repo
convention by using the *_integration_test.go pattern (e.g.,
mvp_integration_test.go), and ensure the top of the file still contains the
integration build tag line "//go:build integration" and the package declaration
"package integration_test" so the test remains an integration-only suite
discoverable by the repo rules.
- Around line 273-300: The three helpers registerStub, assertHTTPStatus, and
verifyWireMockRequests do network I/O without a context; add a context.Context
as the first parameter to each function and use context-aware HTTP calls (e.g.,
http.PostWithContext for registerStub and create an http.Request with
http.NewRequestWithContext and call http.DefaultClient.Do for assertHTTPStatus
and verifyWireMockRequests) so each request respects deadlines/cancellation from
the test context; update error handling and keep defer resp.Body.Close() as
before and remove the //nolint:noctx comments.
🪄 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: f2e763a9-0c94-41b5-9c86-8c61f8640ef7
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/cleanup-ghcr.ymlDockerfileMakefileinternal/testutil/containers.gointernal/testutil/smoke_integration_test.gotests/integration/mvp_test.go
💤 Files with no reviewable changes (2)
- internal/testutil/containers.go
- internal/testutil/smoke_integration_test.go
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/cleanup-ghcr.yml
| FROM alpine:3.21 | ||
| RUN apk add --no-cache ca-certificates | ||
| COPY --from=builder /proxy /usr/local/bin/proxy | ||
| ENTRYPOINT ["proxy"] |
There was a problem hiding this comment.
Run the proxy as a non-root user.
The runtime image never switches away from root, so the exposed proxy process keeps full root privileges inside the container. That is an avoidable security gap for an upstream-facing service.
🔐 Suggested hardening
FROM alpine:3.21
-RUN apk add --no-cache ca-certificates
+RUN apk add --no-cache ca-certificates \
+ && addgroup -S proxy \
+ && adduser -S -G proxy proxy
COPY --from=builder /proxy /usr/local/bin/proxy
+USER proxy
ENTRYPOINT ["proxy"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 9 - 12, The container runs the proxy as root; create
a non-root user/group (e.g., proxy) in the Dockerfile, chown the installed
binary and any required runtime dirs to that user (referencing COPY
--from=builder /proxy /usr/local/bin/proxy and the ENTRYPOINT ["proxy"]), and
switch to that user with USER before the ENTRYPOINT; ensure any files or sockets
the proxy needs are owned/writable by that user and adjust WORKDIR if needed.
| //go:build integration | ||
|
|
||
| package integration_test |
There was a problem hiding this comment.
Use the repo’s *_integration_test.go filename pattern.
mvp_test.go is build-tagged as integration-only, but the repo convention expects these suites to live in *_integration_test.go files.
Based on learnings: Applies to **/*_integration_test.go : Integration tests must live in files named *_integration_test.go with build tag //go:build integration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/mvp_test.go` around lines 1 - 3, Rename the test file from
mvp_test.go to follow the repo convention by using the *_integration_test.go
pattern (e.g., mvp_integration_test.go), and ensure the top of the file still
contains the integration build tag line "//go:build integration" and the package
declaration "package integration_test" so the test remains an integration-only
suite discoverable by the repo rules.
| const defaultProxyImage = "ghcr.io/gaarutyunov/mcp-anything:dev" | ||
|
|
||
| func proxyImage() string { | ||
| if v := os.Getenv("PROXY_IMAGE"); v != "" { | ||
| return v | ||
| } | ||
| return defaultProxyImage |
There was a problem hiding this comment.
Don’t silently fall back to a shared :dev image.
CI injects PROXY_IMAGE, but local make integration does not. When that env var is missing, this suite validates ghcr.io/gaarutyunov/mcp-anything:dev instead of the image built from the current checkout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/mvp_test.go` around lines 24 - 30, The test currently falls
back to a hard-coded shared image in defaultProxyImage when PROXY_IMAGE is
unset; change proxyImage to not silently use that dev image—either remove
defaultProxyImage and make proxyImage fail fast (log.Fatalf or panic) with a
clear message instructing to set PROXY_IMAGE, or derive a local image tag built
from the current checkout (e.g., use a BUILD_IMAGE env or git commit tag) and
return that; update references to proxyImage() accordingly so tests always run
against an explicitly provided or locally-built image rather than
ghcr.io/...:dev.
| // 7. Assert tools/list returns exactly 2 tools. | ||
| toolsResult, err := session.ListTools(callCtx, nil) | ||
| if err != nil { | ||
| t.Fatalf("list tools: %v", err) | ||
| } | ||
| if len(toolsResult.Tools) != 2 { | ||
| t.Fatalf("expected 2 tools, got %d: %v", len(toolsResult.Tools), toolNames(toolsResult.Tools)) | ||
| } | ||
|
|
||
| const toolListPets = "test__list_pets" | ||
| const toolGetPet = "test__get_pets_petid" | ||
|
|
||
| nameSet := make(map[string]bool) | ||
| for _, tool := range toolsResult.Tools { | ||
| nameSet[tool.Name] = true | ||
| } | ||
| if !nameSet[toolListPets] { | ||
| t.Errorf("expected tool %s, got %v", toolListPets, toolNames(toolsResult.Tools)) | ||
| } | ||
| if !nameSet[toolGetPet] { | ||
| t.Errorf("expected tool %s, got %v", toolGetPet, toolNames(toolsResult.Tools)) | ||
| } |
There was a problem hiding this comment.
Assert the generated InputSchema too.
The linked acceptance criteria call out input-schema coverage, but this block only checks tool names. A regression that drops the limit query param or stops marking petId as required would still pass here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/mvp_test.go` around lines 192 - 213, The test only checks
tool names but must also assert each tool's InputSchema to avoid regressions;
update the block that inspects toolsResult.Tools (and uses toolListPets /
toolGetPet) to find the tool by Name and then validate its InputSchema: for
test__list_pets assert the schema includes a "limit" query parameter (correct
type/format and whether it's required or optional per spec), and for
test__get_pets_petid assert the path parameter "petId" is present and marked
required in the InputSchema; use the existing toolsResult.Tools iteration and
toolNames helper to locate each tool and add concise assertions on the
InputSchema/Parameters rather than only checking names.
| func registerStub(t *testing.T, base, body string) { | ||
| t.Helper() | ||
| resp, err := http.Post(base+"/__admin/mappings", "application/json", bytes.NewBufferString(body)) //nolint:noctx // test helper | ||
| if err != nil { | ||
| t.Fatalf("register wiremock stub: %v", err) | ||
| } | ||
| defer resp.Body.Close() | ||
| if resp.StatusCode != http.StatusCreated { | ||
| b, _ := io.ReadAll(resp.Body) | ||
| t.Fatalf("register wiremock stub: got %d: %s", resp.StatusCode, b) | ||
| } | ||
| } | ||
|
|
||
| func assertHTTPStatus(t *testing.T, url string, wantStatus int) { | ||
| t.Helper() | ||
| resp, err := http.Get(url) //nolint:noctx // test helper | ||
| if err != nil { | ||
| t.Fatalf("GET %s: %v", url, err) | ||
| } | ||
| defer resp.Body.Close() | ||
| if resp.StatusCode != wantStatus { | ||
| t.Errorf("GET %s: expected %d, got %d", url, wantStatus, resp.StatusCode) | ||
| } | ||
| } | ||
|
|
||
| func verifyWireMockRequests(t *testing.T, base string) { | ||
| t.Helper() | ||
| resp, err := http.Get(base + "/__admin/requests") //nolint:noctx // test helper |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the specific file and lines mentioned
cd /tmp && find . -name "mvp_test.go" -type f 2>/dev/null | head -5Repository: gaarutyunov/mcp-anything
Length of output: 50
🏁 Script executed:
# Search for the test file in the repository
find . -name "mvp_test.go" -type fRepository: gaarutyunov/mcp-anything
Length of output: 99
🏁 Script executed:
# Once we find it, let's examine the functions
cat -n tests/integration/mvp_test.go | sed -n '270,310p'Repository: gaarutyunov/mcp-anything
Length of output: 1527
🏁 Script executed:
# Search for all three functions to see their full implementations
rg -A 15 "func (registerStub|assertHTTPStatus|verifyWireMockRequests)" tests/integration/mvp_test.goRepository: gaarutyunov/mcp-anything
Length of output: 1289
Pass context.Context into the HTTP helpers.
registerStub, assertHTTPStatus, and verifyWireMockRequests perform network I/O via http.Post and http.Get without request deadlines. The //nolint:noctx comments suppress the linter check, but network stalls will block until the outer test timeout instead of failing fast. Add context.Context as the first parameter to each function and use context-aware variants (http.PostWithContext, request context) to enforce request deadlines per the coding guidelines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/mvp_test.go` around lines 273 - 300, The three helpers
registerStub, assertHTTPStatus, and verifyWireMockRequests do network I/O
without a context; add a context.Context as the first parameter to each function
and use context-aware HTTP calls (e.g., http.PostWithContext for registerStub
and create an http.Request with http.NewRequestWithContext and call
http.DefaultClient.Do for assertHTTPStatus and verifyWireMockRequests) so each
request respects deadlines/cancellation from the test context; update error
handling and keep defer resp.Body.Close() as before and remove the
//nolint:noctx comments.
| var result struct { | ||
| Requests []struct { | ||
| Request struct { | ||
| URL string `json:"url"` | ||
| } `json:"request"` | ||
| } `json:"requests"` | ||
| } | ||
| if err := json.Unmarshal(b, &result); err != nil { | ||
| t.Fatalf("parse wiremock requests: %v", err) | ||
| } | ||
| if len(result.Requests) < 2 { | ||
| t.Errorf("expected at least 2 wiremock requests, got %d", len(result.Requests)) |
There was a problem hiding this comment.
Check which URLs WireMock saw, not only how many.
The current assertion only enforces len(result.Requests) >= 2. If test__list_pets is accidentally routed to /pets/1, the content checks still pass because both stubbed responses contain Fido.
✅ Stronger assertion
- if len(result.Requests) < 2 {
- t.Errorf("expected at least 2 wiremock requests, got %d", len(result.Requests))
- }
+ seen := map[string]int{}
+ for _, req := range result.Requests {
+ seen[req.Request.URL]++
+ }
+ if seen["/pets"] == 0 || seen["/pets/1"] == 0 {
+ t.Errorf("expected requests to /pets and /pets/1, got %#v", seen)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var result struct { | |
| Requests []struct { | |
| Request struct { | |
| URL string `json:"url"` | |
| } `json:"request"` | |
| } `json:"requests"` | |
| } | |
| if err := json.Unmarshal(b, &result); err != nil { | |
| t.Fatalf("parse wiremock requests: %v", err) | |
| } | |
| if len(result.Requests) < 2 { | |
| t.Errorf("expected at least 2 wiremock requests, got %d", len(result.Requests)) | |
| var result struct { | |
| Requests []struct { | |
| Request struct { | |
| URL string `json:"url"` | |
| } `json:"request"` | |
| } `json:"requests"` | |
| } | |
| if err := json.Unmarshal(b, &result); err != nil { | |
| t.Fatalf("parse wiremock requests: %v", err) | |
| } | |
| seen := map[string]int{} | |
| for _, req := range result.Requests { | |
| seen[req.Request.URL]++ | |
| } | |
| if seen["/pets"] == 0 || seen["/pets/1"] == 0 { | |
| t.Errorf("expected requests to /pets and /pets/1, got %#v", seen) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/mvp_test.go` around lines 309 - 320, The test currently
only asserts len(result.Requests) >= 2 which misses wrong routing; update the
assertion to verify the actual URLs WireMock received by inspecting
result.Requests[].Request.URL (the anonymous struct in the test) and assert that
the expected endpoint(s) were called (e.g., contains "/pets" and the other
expected path such as "/pets/1" or the specific URL used by test__list_pets).
Locate the unmarshalling and result variable in the test (the struct with
Requests -> Request -> URL) and add checks that the slice of URLs includes the
exact expected values (or at least one request matching the test__list_pets
expected URL) and fail the test with a helpful message if not.
1 participant
Implements TASK-03: MVP Proxy — Happy Path End-to-End
Closes #6
Generated with Claude Code
Summary by CodeRabbit
New Features
/healthz) and readiness (/readyz) endpointsTests
Chores