feat(router): add MCP Code Mode with search and execute tools#2575
Draft
feat(router): add MCP Code Mode with search and execute tools#2575
Conversation
Adds a new "Code Mode" MCP server that exposes two uber-tools (search + execute) accepting TypeScript code as input, executed in a secure JS sandbox. This enables AI agents to dynamically discover and execute GraphQL operations against the supergraph without pre-defined operation files. Key components: - Secure JS sandbox (router/pkg/sandbox/) with two runtimes: QuickJS via WASM (default) and goja (pure Go fallback), with resource limits and isolation - Code Mode MCP server (router/pkg/mcpserver/code_mode_server.go) with search tool (generateQuery via Yoko REST API) and execute tool (graphql via router) - Yoko client (router/pkg/yokoclient/) for prompt-to-query generation with static token and JWT auth support - Query hash deduplication, TOON encoding, mutation approval via MCP elicitation - OpenTelemetry metrics and tracing, CORS support, retryable HTTP client - Demo infrastructure: yoko-mock server, demo script, and config Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jensneuse
requested review from
a team,
Noroth,
StarpTech,
devsergiy and
endigma
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an MCP "Code Mode" subsystem: a sandboxed TypeScript→JS execution environment, Yoko query-generation client and mock, MCP search/execute tools, router wiring, configs, demo scripts, and extensive tests and docs. (≈45 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment Tip Migrating from UI to YAML configuration.Use the |
Router image scan passed✅ No security vulnerabilities found in image: |
The MCP Code Mode feature added new dependencies (mcp-go v0.43.2, esbuild, goja, qjs, toon-go) that need to be reflected in router-tests go.sum. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
router/pkg/yokoclient/mock_openapi.yaml-69-72 (1)
69-72:⚠️ Potential issue | 🟡 MinorBound the
queriesarray in the response schema.
Line 69defines an unbounded array. AddmaxItemsto keep the contract aligned with response-size controls.Proposed schema bound
queries: type: array + maxItems: 20 items: $ref: '#/components/schemas/QueryResult'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/yokoclient/mock_openapi.yaml` around lines 69 - 72, The schema for the response array "queries" (items: $ref: '#/components/schemas/QueryResult') is currently unbounded; update that array schema in router/pkg/yokoclient/mock_openapi.yaml to include a sensible maxItems value (e.g., maxItems: 100) so the contract enforces a response-size cap and aligns with response-size controls; keep the same items reference and, if desired, also add minItems or descriptive comment to document the chosen bound.rfc/mcp-agent-mode.md-40-67 (1)
40-67:⚠️ Potential issue | 🟡 MinorAdd language tags to fenced code blocks.
Line 40 and Line 124 use unlabeled fences, which triggers markdownlint
MD040.💡 Suggested doc fix
-``` +```http POST {endpoint}/v1/generate Authorization: Bearer {token} Content-Type: application/json @@ -``` +``` -``` +```text ┌─────────────────────────────────────────────────────────┐ │ AI Agent (Claude, etc.) │ @@ -``` +```Also applies to: 124-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rfc/mcp-agent-mode.md` around lines 40 - 67, The markdown contains unlabeled fenced code blocks (the HTTP POST request/response sample and the ASCII AI Agent box) which triggers MD040; update the fences to include language tags (e.g., change the POST request/response block to ```http and the ASCII agent/diagram block to a suitable tag like ```text or ```ascii) and apply the same labeling to the other unlabeled block later in the file so all unlabeled fenced code blocks are explicitly tagged.rfc/mcp-agent-mode.md-783-785 (1)
783-785:⚠️ Potential issue | 🟡 MinorImplementation summary references outdated filenames.
Line 783 mentions
router/pkg/mcpserver/agentic_server.go/agentic_descriptions.go, but this PR is centered oncode_mode_*naming. This can mislead readers navigating the code.💡 Suggested doc fix
-- Created `router/pkg/mcpserver/agentic_server.go` and `agentic_descriptions.go`. +- Created `router/pkg/mcpserver/code_mode_server.go` and `code_mode_descriptions.go`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rfc/mcp-agent-mode.md` around lines 783 - 785, The doc references outdated filenames and resource names (router/pkg/mcpserver/agentic_server.go, agentic_descriptions.go, and agentic://search-api.d.ts) which should be updated to the PR's current naming (e.g., code_mode_server.go, code_mode_descriptions.go and code_mode://search-api.d.ts); edit rfc/mcp-agent-mode.md to replace those legacy strings, ensure mentions of removed APIs (schema.* and validate()) and implemented functions (generateQuery()) remain accurate, and scan the surrounding text for any other "agentic_" identifiers to rename to the code_mode_* equivalents so readers are not misdirected.router/pkg/sandbox/runtime_goja.go-120-121 (1)
120-121:⚠️ Potential issue | 🟡 MinorHarden Math.random and Date.now to prevent runtime reassignment.
The current implementation uses simple reassignment, which sandbox code can override. Use
Object.definePropertywith immutable descriptors to prevent further modification:🔒 Proposed hardening
- _, _ = vm.RunString("Math.random = function() { return 0; }; Date.now = function() { return 0; };") + _, _ = vm.RunString(` +Object.defineProperty(Math, "random", { value: function() { return 0; }, writable: false, configurable: false }); +Object.defineProperty(Date, "now", { value: function() { return 0; }, writable: false, configurable: false }); +`)This pattern is already used in
runtime_qjs.gofor similar sandbox hardening and aligns with defense-in-depth security principles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/sandbox/runtime_goja.go` around lines 120 - 121, Replace the simple reassignment call to vm.RunString that sets Math.random and Date.now with a hardened definition using Object.defineProperty (called from the same vm.RunString invocation in runtime_goja.go) so both Math.random and Date.now are defined with non-writable and non-configurable descriptors that return 0; update the code around the vm.RunString usage to define the functions via Object.defineProperty on Math and Date (and ensure Math exists) so sandboxed scripts cannot later reassign or configure those properties.router/pkg/yokoclient/mock.go-165-176 (1)
165-176:⚠️ Potential issue | 🟡 MinorAccount for argument default values when determining if a field has required arguments.
Per the GraphQL specification, arguments with default values are optional regardless of their type. The current logic only checks for non-null types, causing fields with non-null arguments that have defaults to be unnecessarily skipped during mock query generation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/yokoclient/mock.go` around lines 165 - 176, hasRequiredArgs currently treats any NonNull argument as required; update it to consider argument default values so arguments with defaults are treated as optional. In the hasRequiredArgs function, for each argRef call m.schema.InputValueDefinitionDefaultValue(argRef) (or the project-specific accessor for the default) and only treat a NonNull type as required when that default is absent; i.e. return true when t.TypeKind == ast.TypeKindNonNull AND the argument has no default value. Also ensure the function returns false at the end if no required args are found. Use the existing symbols: hasRequiredArgs, m.schema.InputValueDefinitionType, m.schema.Types, and m.schema.InputValueDefinitionDefaultValue (or equivalent) to locate and implement the change.router/pkg/mcpserver/code_mode_server_test.go-431-433 (1)
431-433:⚠️ Potential issue | 🟡 MinorConsider handling decode errors in mock server.
The JSON decode error is silently ignored. While this is test code, unhandled errors could mask issues during test development.
Proposed fix
var body map[string]any - json.NewDecoder(r.Body).Decode(&body) + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Errorf("failed to decode request body: %v", err) + http.Error(w, "bad request", http.StatusBadRequest) + return + } query := body["query"].(string)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/code_mode_server_test.go` around lines 431 - 433, Capture and check the error returned by json.NewDecoder(r.Body).Decode(&body) instead of ignoring it; if decoding fails return early (e.g., call http.Error(w, "invalid JSON: "+err.Error(), http.StatusBadRequest)) or fail the test (t.Fatalf("...: %v", err)) so the mock handler does not continue to access body["query"] when Decode failed; update the lines around json.NewDecoder(r.Body).Decode(&body) and the subsequent use of query to perform this check.
🧹 Nitpick comments (9)
router/scripts/code-mode-demo.sh (1)
57-65: Use readiness probes instead of fixed sleeps.Line 57 and Line 65 depend on timing assumptions. This makes the demo flaky on slower machines/CI runners.
💡 Suggested refactor
+wait_for_http() { + local url="$1" + local name="$2" + local retries="${3:-60}" + local delay="${4:-1}" + for _ in $(seq 1 "$retries"); do + if curl -fsS "$url" >/dev/null 2>&1; then + echo " $name is ready" + return 0 + fi + sleep "$delay" + done + echo " $name did not become ready in time" + return 1 +} @@ go run cmd/all/main.go & SUBGRAPH_PID=$! echo " Subgraphs starting (PID: $SUBGRAPH_PID)" -sleep 3 # Give subgraphs time to start +wait_for_http "http://localhost:4001/graphql" "Subgraphs" @@ go run cmd/yoko-mock/main.go --addr localhost:5030 --schema "$SCHEMA_SDL" & YOKO_PID=$! echo " Yoko mock server starting (PID: $YOKO_PID)" -sleep 1 +wait_for_http "http://localhost:5030/health" "Yoko mock"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/scripts/code-mode-demo.sh` around lines 57 - 65, Replace the fixed sleep calls with readiness probes that poll the started services until they are actually ready: after starting subgraphs and before starting the Yoko mock, loop and check their listening ports or health endpoints (e.g., attempt TCP connect or curl the health URL) instead of sleep 3; after launching the Yoko mock via the go run command shown (go run cmd/yoko-mock/main.go --addr localhost:5030 --schema "$SCHEMA_SDL" & and YOKO_PID=$!), replace sleep 1 with a loop that probes localhost:5030 (TCP connect or HTTP health check) until it responds or a timeout is reached, and fail with a clear error if the probe times out; keep using ROUTER_DIR, SCHEMA_SDL and YOKO_PID names so the change is localized and easy to review.router/pkg/yokoclient/mock.go (1)
185-209: Consider caching scalar/type lookups in this mock client.
isScalarandgetTypeFieldsrepeatedly scanRootNodes; precomputed maps would reduce repeated O(n) traversals and simplify code paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/yokoclient/mock.go` around lines 185 - 209, Add cached lookup maps to MockClient (e.g., scalarSet map[string]bool and typeFields map[string][]int) and populate them once when the schema is loaded/assigned (populate by iterating m.schema.RootNodes and using m.schema.ScalarTypeDefinitionNameString and m.schema.ObjectTypeDefinitionNameString to fill the maps); then change isScalar to check scalarSet first (with an optional fallback to the scan if map is nil) and change getTypeFields to return typeFields[typeName] (with a nil/empty fallback), ensuring the cache keys use the exact strings returned by ScalarTypeDefinitionNameString/ObjectTypeDefinitionNameString to match existing behavior.router/pkg/mcpserver/code_mode_descriptions.go (1)
10-53: Reduce duplicated TS contracts to avoid drift.The same API/type contract is duplicated across inline tool descriptions and MCP resource typedefs. Consider generating both from shared constants/templates.
Also applies to: 59-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/code_mode_descriptions.go` around lines 10 - 53, The inline type/contracts in searchToolDescription and executeToolDescription are duplicated elsewhere; extract the shared TypeScript contracts (e.g., the QueryResult interface and GraphQLOptions/GraphQLResponse/GraphQLError definitions) into a single canonical constant or template (e.g., sharedTypeContracts or graphqlContracts) and reference or interpolate that constant into both searchToolDescription and executeToolDescription (and the MCP resource typedefs) so both use the same source-of-truth; update code that previously declared the interfaces inline to import/use the shared constant/template to prevent drift.router/pkg/sandbox/pool_test.go (1)
33-37: UseWaitGroup.Go()instead of manualAdd/Donepattern.Go 1.25 provides
sync.WaitGroup.Go(func())which automatically managesAddandDonecalls for launched goroutines. Replace the manual pattern throughout:
- Lines 33–37:
wg.Add(1); go func(idx int) { defer wg.Done(); ... }(i)→wg.Go(func() { ... })- Lines 76–80: Same pattern
- Lines 118–126:
started.Add(1); go func() { started.Done(); ... }()→started.Go(func() { ... })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/sandbox/pool_test.go` around lines 33 - 37, Replace manual goroutine Add/Done patterns with sync.WaitGroup.Go: for the loops using wg.Add(1); go func(idx int){ defer wg.Done(); ... }(i) switch to capturing the loop variable (e.g. idx := i) and call wg.Go(func(){ ... use idx ... }) and remove the defer wg.Done(); likewise replace started.Add/started.Done with started.Go(func(){ ... }) and remove the explicit Done call; ensure any loop-variable usage is copied to a local variable before calling Go to avoid closure issues.router/pkg/sandbox/transpiler.go (1)
69-74: Consider adding test coverage for string literal preservation when stripping async/await.The developer has already documented why this approach is safe for goja mode—input is strictly constrained to async arrow functions wrapped as IIFEs, which are minified by esbuild before stripping. However, no test validates that string literals containing
"async "or"await "are preserved. Adding a test case (e.g.,async () => { const msg = "async task"; return msg; }) would strengthen confidence in this approach and document the assumption that input constraints prevent corruption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/sandbox/transpiler.go` around lines 69 - 74, Add a unit test that validates stripAsyncAwait preserves string literals containing "async " and "await " by calling stripAsyncAwait with an IIFE async arrow function that includes const msg = "async task" and/or "await something" in a string and asserting the returned string still contains those string literals; reference the stripAsyncAwait function in transpiler.go and create a test (e.g., TestStripAsyncAwait_PreservesStringLiterals) that covers both raw and minified-like inputs to document and enforce the assumption that only the async/await keywords are removed, not content inside string literals.router/pkg/mcpserver/code_mode_server.go (4)
607-616: Usesync.WaitGroup.Go()instead of manualAdd/Donepattern.The manual
wg.Add(1)followed bygo func() { defer wg.Done(); ... }()pattern should be replaced withwg.Go(func())as per the repository's Go 1.25+ conventions.Proposed fix
var wg sync.WaitGroup for i, p := range prompts { - wg.Add(1) - go func(idx int, prompt string) { - defer wg.Done() + wg.Go(func() { results, err := s.yokoClient.Generate(ctx, prompt, schemaHash) ch <- indexedResult{index: idx, results: results, err: err} - }(i, p) + }) }Note: You'll need to capture loop variables properly for Go versions < 1.22, or ensure the loop uses the Go 1.22+ semantics where loop variables are per-iteration.
Based on learnings: "In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine, letting the WaitGroup manage Add/Done automatically."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/code_mode_server.go` around lines 607 - 616, Replace the manual wg.Add(1) / defer wg.Done() pattern with wg.Go(...) to spawn the goroutine for each prompt: in the loop over prompts use wg.Go(func() { ... }) to call s.yokoClient.Generate(ctx, prompt, schemaHash) and send indexedResult on ch; ensure loop variables are captured correctly by creating local copies (e.g., idx, p or prompt) before using them in the closure so s.yokoClient.Generate, indexedResult{index: idx, results: results, err: err}, and ch usage refer to the intended per-iteration values.
234-244: Formatting issue with extra parenthesis line.The
executetool registration has an unusual empty line 242 with just),. While syntactically valid, this appears to be a formatting inconsistency compared to thesearchtool registration above.Proposed fix for consistency
// Execute tool s.mcpServer.AddTool( mcp.NewTool(executeToolName, mcp.WithDescription(executeToolDescription), mcp.WithString("code", mcp.Required(), mcp.Description("An async arrow function. Example: async () => { const { data } = await graphql({ query: '{ users { id } }' }); return data; }"), ), - ), + ), s.handleExecute(), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/code_mode_server.go` around lines 234 - 244, The tool registration for execute (mcp.NewTool with executeToolName and mcp.WithString("code")) has an extra stand-alone closing tuple line containing just "),", causing a formatting inconsistency; edit the call that constructs the execute tool (the mcp.NewTool(...) invocation used with executeToolName and s.handleExecute()) to remove the stray closing line and collapse the parentheses so the mcp.WithString block and the NewTool call are closed consistently (matching the style used by the search tool registration).
667-676: Consider LRU eviction instead of clearing entire store.The current eviction strategy clears the entire map when reaching
maxQueryStoreSize. This could cause all in-flight operations to fail simultaneously if they rely on recently stored hashes.An LRU cache would provide more graceful eviction, keeping the most recently used queries available.
Alternative using an LRU cache
Consider using
github.com/hashicorp/golang-lru/v2or similar:+import lru "github.com/hashicorp/golang-lru/v2" + type CodeModeServer struct { // ... - queryStoreMu sync.RWMutex - queryStore map[string]string + queryStore *lru.Cache[string, string] } // In NewCodeModeServer: - queryStore: make(map[string]string), + // queryStore initialized with LRU cacheIf the current approach is intentional for simplicity, consider adding a comment explaining the tradeoff.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/code_mode_server.go` around lines 667 - 676, The storeQueryHash function currently clears the entire s.queryStore map when it reaches maxQueryStoreSize, which can evict all entries at once; replace the plain map with an LRU cache (e.g., github.com/hashicorp/golang-lru/v2) stored on the CodeModeServer struct (replace s.queryStore and s.queryStoreMu with an lru.Cache instance and its mutex if needed) and update storeQueryHash to Add the hash→query into the LRU (use cache.Add(hash, query)) so old entries are evicted incrementally; alternatively, if the all-clear behavior is intentional, add a clear comment in storeQueryHash documenting the tradeoff and why simple clear was chosen.
104-111: Consider logging metric registration failures.The errors from
meter.Int64Counterandmeter.Float64Histogramare silently discarded. While metric failures shouldn't break the server, logging them would help diagnose observability issues.Proposed fix
meter := otel.Meter("wundergraph.cosmo.router.mcp.code_mode") - execCounter, _ := meter.Int64Counter("mcp.code_mode.sandbox.executions", + execCounter, err := meter.Int64Counter("mcp.code_mode.sandbox.executions", otelmetric.WithDescription("Total number of Code Mode sandbox executions"), ) - execDuration, _ := meter.Float64Histogram("mcp.code_mode.sandbox.duration", + if err != nil { + cfg.Logger.Warn("failed to create executions counter", zap.Error(err)) + } + execDuration, err := meter.Float64Histogram("mcp.code_mode.sandbox.duration", otelmetric.WithDescription("Duration of Code Mode sandbox executions in milliseconds"), otelmetric.WithUnit("ms"), ) + if err != nil { + cfg.Logger.Warn("failed to create duration histogram", zap.Error(err)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/code_mode_server.go` around lines 104 - 111, The code currently discards errors returned from meter.Int64Counter and meter.Float64Histogram; update the initialization around meter, execCounter and execDuration to check the returned error values and log any non-nil errors (use the file's existing logger instance, e.g., logger or s.logger, or fall back to the standard log package) so metric registration failures are recorded without changing control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/cmd/yoko-mock/main.go`:
- Around line 83-86: The handler currently returns internal error text
(err.Error()) to API clients via writeError (seen around the writeError(...)
calls at the top block and the similar calls at lines ~144-146), which can leak
raw generator/Claude output; change these writeError calls to return a generic
client-facing message (e.g., "internal server error" or "query generation
failed") and avoid including err.Error() in the HTTP response. Continue to log
the full error and elapsed time with log.Printf (or processLogger) for
debugging, and optionally generate an internal error ID to return to the client
so logs can be correlated without exposing raw error contents. Ensure this
sanitization is applied to all places that call writeError with err.Error()
(including the parse error path).
- Around line 67-68: The handler decodes r.Body with
json.NewDecoder(r.Body).Decode(&req) without limiting size; wrap the request
body with http.MaxBytesReader (or io.LimitReader) before decoding to enforce a
safe max (e.g., 1MB), check the decode error for io.ErrUnexpectedEOF or
http.ErrBodyReadAfterClose/“request body too large” and return a 413 or 400 via
writeError if the body exceeds the limit or JSON is invalid; update the code
around json.NewDecoder/Decode (and r.Body usage) to use the limited reader and
handle the oversized-body error path.
In `@router/go.mod`:
- Line 81: The module bump to github.com/mark3labs/mcp-go v0.43.2 introduces
breaking changes; either pin back to the known-compatible v0.36.0 in
router/go.mod or migrate router code to the new API: update all usages of
Result.Meta to accept/handle *mcp.Meta (change types and any map access
accordingly), update any TokenStore implementations and calls (in server.go and
code_mode_server.go) to add context.Context as the first parameter to the
affected methods, and fix construction of mcp.Implementation values by switching
to keyed struct literals and populating the new Title field where applicable;
search for Result.Meta, TokenStore, and mcp.Implementation usages in server.go
and code_mode_server.go to make these targeted edits.
In `@router/pkg/config/config.schema.json`:
- Around line 2348-2380: The sandbox schema under "sandbox" must include the
missing "max_input_size_bytes" property and add validation bounds for numeric
limits: update properties "max_memory_mb", "max_fuel", and
"max_output_size_bytes" (and the new "max_input_size_bytes") to include
"minimum" (e.g., 0 or 1 as appropriate) and sensible "maximum" values consistent
with config/runtime, and adjust any "default" values if needed; ensure
"additionalProperties" remains false and document the new field's "description"
and "default".
- Around line 2397-2412: The auth schema currently only allows "type" and
"static_token", so add explicit properties for the Cosmo flow (e.g.,
"token_endpoint", "client_id", "client_secret") under the "auth" object and
enforce required fields conditionally when "type" == "cosmo" using JSON Schema
conditional validation (if/then or oneOf) so YAML configs can validate
non-static auth; keep additionalProperties: false and ensure the "auth" object
(and its "type" enum) is the target for these updates (reference the "auth"
schema, its "type" property, and add the new property names).
In `@router/pkg/mcpserver/server.go`:
- Around line 420-423: The current shutdown block swallows errors from
s.rawHTTPServer.Shutdown and Close; change it to surface failures to the caller
by capturing the error from s.rawHTTPServer.Shutdown(ctx) (and from
s.rawHTTPServer.Close() if Shutdown fails) and returning that error instead of
only logging it. Locate the code using s.rawHTTPServer.Shutdown and
s.rawHTTPServer.Close (in the server's shutdown/Stop/Close method) and: 1) if
Shutdown returns non-nil, attempt Close and if Close also returns non-nil, wrap
or aggregate those errors (or return the first meaningful one); 2) keep the
s.logger.Warn call but then return the error to the caller so upstream can
observe shutdown failure; 3) update the method signature to return error if it
doesn't already and adjust callers to handle the returned error.
In `@router/pkg/sandbox/pool_test.go`:
- Around line 80-99: The test injects a SyncFunc named trackConcurrency but the
JS snippet never calls it, so maxSeen and concurrent are never updated; update
the JS passed to pool.Execute (the snippet in the pool.Execute call) to actually
invoke the injected sync function (e.g., call trackConcurrency() inside the
heavy loop or call it repeatedly from the JS IIFE) so that trackConcurrency runs
and updates concurrent and maxSeen; ensure the SyncFunc name
("trackConcurrency") matches the JS call and keep using the existing
SyncFunc/variables (SyncFunc, trackConcurrency, maxSeen, concurrent) to assert
measured concurrency.
In `@router/pkg/sandbox/runtime_goja.go`:
- Around line 37-41: In Execute (method gojaRuntime.Execute) enforce
r.config.MaxInputBytes before parsing/executing: check len(jsCode) against
r.config.MaxInputBytes (when >0) and return a clear error (and no Result) if the
input exceeds the limit so oversized scripts are rejected early; do this check
immediately after determining timeout and before any further processing,
referencing Execute and r.config.MaxInputBytes to locate the change.
- Around line 127-129: The goja.InterruptedError handling in the exec goroutine
incorrectly always labels interrupts as timeouts; update the block that checks
err.(*goja.InterruptedError) (where execResult is sent) to inspect
interrupted.Value() and ctx.Err(): if interrupted.Value() == "context cancelled"
and ctx.Err() != nil, send an execResult with an error indicating execution
cancelled (include ctx.Err() or interrupted.Value()), otherwise keep the
existing timeout-style fmt.Errorf using timeout and interrupted.Value(); ensure
you reference goja.InterruptedError, interrupted.Value(), ctx.Err(), execResult
and timeout when making the change.
In `@router/pkg/sandbox/runtime_qjs.go`:
- Around line 163-165: The lockdown Eval call ignores errors and must fail
closed: capture the error returned by qctx.Eval("lockdown.js", ...) (the
hardening step after qctx.Global().DeleteProperty("eval")) and if non-nil,
propagate it (return the error from the surrounding initialization function) or
log and abort startup so the runtime does not start without the lockdown; ensure
the code around qctx.Eval checks the error and handles it instead of discarding
it.
- Around line 120-133: The async callback handler spawns an unbounded goroutine
per call (the anonymous goroutine that runs res, fnErr = fn.Fn(args) and sends
asyncResult into resultCh), allowing callers to exhaust resources; add a
concurrency cap (e.g., a buffered semaphore channel or a fixed worker pool) that
the goroutine must acquire before executing fn.Fn and release after sending the
result or on panic, so only N async callbacks can run concurrently; use the
existing symbols promise, resultCh, done, and asyncResult to keep behavior
identical while guarding goroutine fan-out (create a package-level or
runtime-level semaphore/worker queue and check/acquire before launching or
dispatching the task).
In `@router/pkg/sandbox/runtime.go`:
- Around line 69-75: NewRuntime currently silently falls back to goja for
unknown RuntimeType values; change it to fail loudly so typos don't disable QJS.
Update NewRuntime (and its switch over RuntimeType/RuntimeTypeQJS) to enumerate
supported cases (e.g., RuntimeTypeQJS and the explicit goja type if present) and
replace the current default branch that returns newGojaRuntime(config) with a
loud failure: either return a clear error (preferred if you can change the
signature) or call panic/log.Fatalf with a formatted message that includes the
invalid runtimeType value; ensure you reference NewRuntime, RuntimeType,
RuntimeTypeQJS, newQJSRuntime and newGojaRuntime when making the change so the
fallback no longer silently picks goja.
In `@router/pkg/yokoclient/client.go`:
- Around line 146-152: The token switch in the client token retrieval (where
c.auth.Type is inspected) rejects "cosmo" causing runtime errors; update the
switch in router/pkg/yokoclient/client.go to treat "cosmo" like "jwt" (e.g., add
a case "cosmo" that calls c.getJWTToken(ctx) or use fallthrough from "cosmo" to
"jwt") so c.getJWTToken(ctx) is invoked for cosmo auth instead of returning the
"unsupported auth type" error; ensure the error branch still covers truly
unsupported values.
- Around line 201-214: The code currently assigns tokenResp.AccessToken into
c.cachedToken and sets c.tokenExpiry without validating the payload; update the
token handling (around tokenResp, c.cachedToken and c.tokenExpiry) to check that
tokenResp.AccessToken is non-empty (and optionally that ExpiresIn is sensible)
before caching—if AccessToken is empty, return a clear error instead of caching
an empty token; only compute earlyExpiry and set c.tokenExpiry after validating
the token value.
In `@router/pkg/yokoclient/mock_openapi.yaml`:
- Around line 1-43: The OpenAPI spec lacks a top-level security requirement so
new paths could be unauthenticated; add a global security requirement (security:
- bearerAuth: []) and define the bearerAuth scheme under
components.securitySchemes (e.g., type: http, scheme: bearer, bearerFormat: JWT)
so the API (including operationId generateQueries) is protected by default while
keeping or overriding per-operation security as needed.
In `@router/pkg/yokoclient/mock.go`:
- Around line 211-227: findRootType currently hardcodes "Query"/"Mutation";
instead read the schema's explicit root operation types and resolve by
definition names: in MockClient.findRootType, inspect m.schema.SchemaDefinitions
(or m.schema.Document.SchemaDefinitions) and their RootOperationTypeDefinitions
to determine the type name (or directly the referenced node ref) for
ast.OperationTypeQuery and ast.OperationTypeMutation, then match that name/ref
against m.schema.RootNodes (or object definitions) rather than the literal
strings. Keep existing behavior as a fallback by using the standard
"Query"/"Mutation" when no schema block is present.
---
Minor comments:
In `@rfc/mcp-agent-mode.md`:
- Around line 40-67: The markdown contains unlabeled fenced code blocks (the
HTTP POST request/response sample and the ASCII AI Agent box) which triggers
MD040; update the fences to include language tags (e.g., change the POST
request/response block to ```http and the ASCII agent/diagram block to a
suitable tag like ```text or ```ascii) and apply the same labeling to the other
unlabeled block later in the file so all unlabeled fenced code blocks are
explicitly tagged.
- Around line 783-785: The doc references outdated filenames and resource names
(router/pkg/mcpserver/agentic_server.go, agentic_descriptions.go, and
agentic://search-api.d.ts) which should be updated to the PR's current naming
(e.g., code_mode_server.go, code_mode_descriptions.go and
code_mode://search-api.d.ts); edit rfc/mcp-agent-mode.md to replace those legacy
strings, ensure mentions of removed APIs (schema.* and validate()) and
implemented functions (generateQuery()) remain accurate, and scan the
surrounding text for any other "agentic_" identifiers to rename to the
code_mode_* equivalents so readers are not misdirected.
In `@router/pkg/mcpserver/code_mode_server_test.go`:
- Around line 431-433: Capture and check the error returned by
json.NewDecoder(r.Body).Decode(&body) instead of ignoring it; if decoding fails
return early (e.g., call http.Error(w, "invalid JSON: "+err.Error(),
http.StatusBadRequest)) or fail the test (t.Fatalf("...: %v", err)) so the mock
handler does not continue to access body["query"] when Decode failed; update the
lines around json.NewDecoder(r.Body).Decode(&body) and the subsequent use of
query to perform this check.
In `@router/pkg/sandbox/runtime_goja.go`:
- Around line 120-121: Replace the simple reassignment call to vm.RunString that
sets Math.random and Date.now with a hardened definition using
Object.defineProperty (called from the same vm.RunString invocation in
runtime_goja.go) so both Math.random and Date.now are defined with non-writable
and non-configurable descriptors that return 0; update the code around the
vm.RunString usage to define the functions via Object.defineProperty on Math and
Date (and ensure Math exists) so sandboxed scripts cannot later reassign or
configure those properties.
In `@router/pkg/yokoclient/mock_openapi.yaml`:
- Around line 69-72: The schema for the response array "queries" (items: $ref:
'#/components/schemas/QueryResult') is currently unbounded; update that array
schema in router/pkg/yokoclient/mock_openapi.yaml to include a sensible maxItems
value (e.g., maxItems: 100) so the contract enforces a response-size cap and
aligns with response-size controls; keep the same items reference and, if
desired, also add minItems or descriptive comment to document the chosen bound.
In `@router/pkg/yokoclient/mock.go`:
- Around line 165-176: hasRequiredArgs currently treats any NonNull argument as
required; update it to consider argument default values so arguments with
defaults are treated as optional. In the hasRequiredArgs function, for each
argRef call m.schema.InputValueDefinitionDefaultValue(argRef) (or the
project-specific accessor for the default) and only treat a NonNull type as
required when that default is absent; i.e. return true when t.TypeKind ==
ast.TypeKindNonNull AND the argument has no default value. Also ensure the
function returns false at the end if no required args are found. Use the
existing symbols: hasRequiredArgs, m.schema.InputValueDefinitionType,
m.schema.Types, and m.schema.InputValueDefinitionDefaultValue (or equivalent) to
locate and implement the change.
---
Nitpick comments:
In `@router/pkg/mcpserver/code_mode_descriptions.go`:
- Around line 10-53: The inline type/contracts in searchToolDescription and
executeToolDescription are duplicated elsewhere; extract the shared TypeScript
contracts (e.g., the QueryResult interface and
GraphQLOptions/GraphQLResponse/GraphQLError definitions) into a single canonical
constant or template (e.g., sharedTypeContracts or graphqlContracts) and
reference or interpolate that constant into both searchToolDescription and
executeToolDescription (and the MCP resource typedefs) so both use the same
source-of-truth; update code that previously declared the interfaces inline to
import/use the shared constant/template to prevent drift.
In `@router/pkg/mcpserver/code_mode_server.go`:
- Around line 607-616: Replace the manual wg.Add(1) / defer wg.Done() pattern
with wg.Go(...) to spawn the goroutine for each prompt: in the loop over prompts
use wg.Go(func() { ... }) to call s.yokoClient.Generate(ctx, prompt, schemaHash)
and send indexedResult on ch; ensure loop variables are captured correctly by
creating local copies (e.g., idx, p or prompt) before using them in the closure
so s.yokoClient.Generate, indexedResult{index: idx, results: results, err: err},
and ch usage refer to the intended per-iteration values.
- Around line 234-244: The tool registration for execute (mcp.NewTool with
executeToolName and mcp.WithString("code")) has an extra stand-alone closing
tuple line containing just "),", causing a formatting inconsistency; edit the
call that constructs the execute tool (the mcp.NewTool(...) invocation used with
executeToolName and s.handleExecute()) to remove the stray closing line and
collapse the parentheses so the mcp.WithString block and the NewTool call are
closed consistently (matching the style used by the search tool registration).
- Around line 667-676: The storeQueryHash function currently clears the entire
s.queryStore map when it reaches maxQueryStoreSize, which can evict all entries
at once; replace the plain map with an LRU cache (e.g.,
github.com/hashicorp/golang-lru/v2) stored on the CodeModeServer struct (replace
s.queryStore and s.queryStoreMu with an lru.Cache instance and its mutex if
needed) and update storeQueryHash to Add the hash→query into the LRU (use
cache.Add(hash, query)) so old entries are evicted incrementally; alternatively,
if the all-clear behavior is intentional, add a clear comment in storeQueryHash
documenting the tradeoff and why simple clear was chosen.
- Around line 104-111: The code currently discards errors returned from
meter.Int64Counter and meter.Float64Histogram; update the initialization around
meter, execCounter and execDuration to check the returned error values and log
any non-nil errors (use the file's existing logger instance, e.g., logger or
s.logger, or fall back to the standard log package) so metric registration
failures are recorded without changing control flow.
In `@router/pkg/sandbox/pool_test.go`:
- Around line 33-37: Replace manual goroutine Add/Done patterns with
sync.WaitGroup.Go: for the loops using wg.Add(1); go func(idx int){ defer
wg.Done(); ... }(i) switch to capturing the loop variable (e.g. idx := i) and
call wg.Go(func(){ ... use idx ... }) and remove the defer wg.Done(); likewise
replace started.Add/started.Done with started.Go(func(){ ... }) and remove the
explicit Done call; ensure any loop-variable usage is copied to a local variable
before calling Go to avoid closure issues.
In `@router/pkg/sandbox/transpiler.go`:
- Around line 69-74: Add a unit test that validates stripAsyncAwait preserves
string literals containing "async " and "await " by calling stripAsyncAwait with
an IIFE async arrow function that includes const msg = "async task" and/or
"await something" in a string and asserting the returned string still contains
those string literals; reference the stripAsyncAwait function in transpiler.go
and create a test (e.g., TestStripAsyncAwait_PreservesStringLiterals) that
covers both raw and minified-like inputs to document and enforce the assumption
that only the async/await keywords are removed, not content inside string
literals.
In `@router/pkg/yokoclient/mock.go`:
- Around line 185-209: Add cached lookup maps to MockClient (e.g., scalarSet
map[string]bool and typeFields map[string][]int) and populate them once when the
schema is loaded/assigned (populate by iterating m.schema.RootNodes and using
m.schema.ScalarTypeDefinitionNameString and
m.schema.ObjectTypeDefinitionNameString to fill the maps); then change isScalar
to check scalarSet first (with an optional fallback to the scan if map is nil)
and change getTypeFields to return typeFields[typeName] (with a nil/empty
fallback), ensuring the cache keys use the exact strings returned by
ScalarTypeDefinitionNameString/ObjectTypeDefinitionNameString to match existing
behavior.
In `@router/scripts/code-mode-demo.sh`:
- Around line 57-65: Replace the fixed sleep calls with readiness probes that
poll the started services until they are actually ready: after starting
subgraphs and before starting the Yoko mock, loop and check their listening
ports or health endpoints (e.g., attempt TCP connect or curl the health URL)
instead of sleep 3; after launching the Yoko mock via the go run command shown
(go run cmd/yoko-mock/main.go --addr localhost:5030 --schema "$SCHEMA_SDL" & and
YOKO_PID=$!), replace sleep 1 with a loop that probes localhost:5030 (TCP
connect or HTTP health check) until it responds or a timeout is reached, and
fail with a clear error if the probe times out; keep using ROUTER_DIR,
SCHEMA_SDL and YOKO_PID names so the change is localized and easy to review.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
router/go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
Makefiledemo/pkg/subgraphs/employees/subgraph/schema.resolvers.gorfc/mcp-agent-mode.mdrouter/.gitignorerouter/cmd/yoko-mock/main.gorouter/code-mode-demo.config.yamlrouter/core/router.gorouter/core/router_config.gorouter/go.modrouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.jsonrouter/pkg/mcpserver/code_mode_descriptions.gorouter/pkg/mcpserver/code_mode_server.gorouter/pkg/mcpserver/code_mode_server_test.gorouter/pkg/mcpserver/server.gorouter/pkg/mcpserver/util.gorouter/pkg/sandbox/pool.gorouter/pkg/sandbox/pool_test.gorouter/pkg/sandbox/runtime.gorouter/pkg/sandbox/runtime_goja.gorouter/pkg/sandbox/runtime_qjs.gorouter/pkg/sandbox/runtime_qjs_test.gorouter/pkg/sandbox/runtime_test.gorouter/pkg/sandbox/transpiler.gorouter/pkg/sandbox/transpiler_test.gorouter/pkg/yokoclient/client.gorouter/pkg/yokoclient/client_test.gorouter/pkg/yokoclient/mock.gorouter/pkg/yokoclient/mock_openapi.yamlrouter/pkg/yokoclient/mock_test.gorouter/scripts/code-mode-demo.shrouter/scripts/code-mode-graph.yaml
Comment on lines
+67
to
+68
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| writeError(w, http.StatusBadRequest, "Invalid JSON", err.Error()) |
Contributor
There was a problem hiding this comment.
Limit request body size before JSON decode.
Line 67 decodes an unbounded body; a large payload can exhaust memory.
Proposed guard
- var req generateRequest
- if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+ const maxGenerateBodyBytes = 1 << 20 // 1 MiB
+ r.Body = http.MaxBytesReader(w, r.Body, maxGenerateBodyBytes)
+
+ var req generateRequest
+ dec := json.NewDecoder(r.Body)
+ dec.DisallowUnknownFields()
+ if err := dec.Decode(&req); err != nil {
writeError(w, http.StatusBadRequest, "Invalid JSON", err.Error())
return
}📝 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.
Suggested change
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | |
| writeError(w, http.StatusBadRequest, "Invalid JSON", err.Error()) | |
| const maxGenerateBodyBytes = 1 << 20 // 1 MiB | |
| r.Body = http.MaxBytesReader(w, r.Body, maxGenerateBodyBytes) | |
| var req generateRequest | |
| dec := json.NewDecoder(r.Body) | |
| dec.DisallowUnknownFields() | |
| if err := dec.Decode(&req); err != nil { | |
| writeError(w, http.StatusBadRequest, "Invalid JSON", err.Error()) | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router/cmd/yoko-mock/main.go` around lines 67 - 68, The handler decodes
r.Body with json.NewDecoder(r.Body).Decode(&req) without limiting size; wrap the
request body with http.MaxBytesReader (or io.LimitReader) before decoding to
enforce a safe max (e.g., 1MB), check the decode error for io.ErrUnexpectedEOF
or http.ErrBodyReadAfterClose/“request body too large” and return a 413 or 400
via writeError if the body exceeds the limit or JSON is invalid; update the code
around json.NewDecoder/Decode (and r.Body usage) to use the limited reader and
handle the oversized-body error path.
Member
Author
There was a problem hiding this comment.
Won't fix — yoko-mock is a development/testing tool, not production infrastructure. Body size limits are unnecessary here.
Contributor
There was a problem hiding this comment.
@jensneuse, understood — I appreciate the clarification. Since yoko-mock is a dev/test utility, the additional hardening isn't necessary. Thanks for the context!
🧠 Learnings used
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Learnt from: asoorm
Repo: wundergraph/cosmo PR: 2379
File: router/pkg/connectrpc/operation_registry_test.go:381-399
Timestamp: 2026-01-06T12:37:21.521Z
Learning: In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine, letting the WaitGroup manage Add/Done automatically. Avoid manual wg.Add(1) followed by go func() { defer wg.Done(); ... }() patterns. Apply this guidance across all Go files in the wundergraph/cosmo repository where concurrency is used.
Comment on lines
+83
to
+86
| if err != nil { | ||
| log.Printf("[ERROR] %v (took %s)", err, elapsed) | ||
| writeError(w, http.StatusInternalServerError, "Query generation failed", err.Error()) | ||
| return |
Contributor
There was a problem hiding this comment.
Avoid returning internal generator details to API clients.
Line 85 exposes err.Error() to callers. With Line 145, parse errors can include raw Claude output, which leaks internal data.
Proposed redaction
- if err != nil {
- log.Printf("[ERROR] %v (took %s)", err, elapsed)
- writeError(w, http.StatusInternalServerError, "Query generation failed", err.Error())
+ if err != nil {
+ log.Printf("[ERROR] query generation failed: %v (took %s)", err, elapsed)
+ writeError(w, http.StatusInternalServerError, "Query generation failed", "")
return
}
@@
- if err := json.Unmarshal([]byte(text), &queries); err != nil {
- return nil, fmt.Errorf("failed to parse claude output as JSON: %w\nRaw output:\n%s", err, text)
+ if err := json.Unmarshal([]byte(text), &queries); err != nil {
+ return nil, fmt.Errorf("failed to parse claude output as JSON: %w", err)
}Also applies to: 144-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router/cmd/yoko-mock/main.go` around lines 83 - 86, The handler currently
returns internal error text (err.Error()) to API clients via writeError (seen
around the writeError(...) calls at the top block and the similar calls at lines
~144-146), which can leak raw generator/Claude output; change these writeError
calls to return a generic client-facing message (e.g., "internal server error"
or "query generation failed") and avoid including err.Error() in the HTTP
response. Continue to log the full error and elapsed time with log.Printf (or
processLogger) for debugging, and optionally generate an internal error ID to
return to the client so logs can be correlated without exposing raw error
contents. Ensure this sanitization is applied to all places that call writeError
with err.Error() (including the parse error path).
Member
Author
There was a problem hiding this comment.
Won't fix — yoko-mock is a development/testing tool. Exposing error details is useful for debugging during development.
Contributor
There was a problem hiding this comment.
@jensneuse, understood! Since yoko-mock is a development/testing tool, detailed error exposure makes sense for debugging purposes. Thanks for the clarification.
✏️ Learnings added
Learnt from: jensneuse
Repo: wundergraph/cosmo PR: 2575
File: router/cmd/yoko-mock/main.go:83-86
Timestamp: 2026-03-02T19:11:26.502Z
Learning: router/cmd/yoko-mock is a development/testing tool (not production code) where exposing detailed error messages including raw Claude output is acceptable and useful for debugging during development.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Learnt from: asoorm
Repo: wundergraph/cosmo PR: 2379
File: router/pkg/connectrpc/operation_registry_test.go:381-399
Timestamp: 2026-01-06T12:37:21.521Z
Learning: In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine, letting the WaitGroup manage Add/Done automatically. Avoid manual wg.Add(1) followed by go func() { defer wg.Done(); ... }() patterns. Apply this guidance across all Go files in the wundergraph/cosmo repository where concurrency is used.
| @@ -75,12 +78,13 @@ require ( | |||
| github.com/hashicorp/go-plugin v1.6.3 | |||
| github.com/iancoleman/strcase v0.3.0 | |||
| github.com/klauspost/compress v1.18.0 | |||
| github.com/mark3labs/mcp-go v0.36.0 | |||
| github.com/mark3labs/mcp-go v0.43.2 | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== mcp-go versions across all go.mod files ==="
fd -t f go.mod -x sh -c 'echo "--- $1 ---"; rg "github.com/mark3labs/mcp-go" "$1" || echo " (not found)"' sh {}
echo
echo "=== mcp-go imports in router code (if any) ==="
rg --type go "github.com/mark3labs/mcp-go|mcp\\.Server|mcp\\.Client" --max-count=20 2>/dev/null || echo " (no imports found)"Repository: wundergraph/cosmo
Length of output: 1389
🌐 Web query:
github.com/mark3labs/mcp-go v0.36.0 vs v0.43.2 breaking changes API differences
💡 Result:
Breaking changes between v0.36.0 → v0.43.2
Result.Metatype changed (v0.37.0)
- Before (≤ v0.36.0):
result.Metawasmap[string]any - After (≥ v0.37.0):
result.Metais*mcp.Meta(withAdditionalFields)
Migration example is shown in the v0.37.0 release notes. [1]
- OAuth
TokenStoreinterface method signatures changed (v0.40.0)
feat!: addscontext.ContexttoTokenStoremethods, so any customTokenStoreimplementation must update its method signatures accordingly. [2]
mcp.Implementationgained aTitlefield (v0.43.0)
- If your code uses unkeyed struct literals (positional composite literals) for
mcp.Implementation, adding a field can cause compile errors / misassignment. - Fix: switch to keyed fields in the literal. [3]
Other notable API / behavior differences (not necessarily “breaking”, but relevant)
- New “Roots” support in HTTP + stdio clients (v0.43.0) (new requests/flows you may want to adopt). [3]
- Request-based session management hooks via
SessionIdManagerResolver(v0.43.0). [3] v0.43.2itself is bugfix-only (notification/tool-call handling; schema unmarshal fix). [4]
Sources
[1] v0.37.0 release (Breaking change: Result.Meta → *Meta) (newreleases.io)
[2] v0.40.0 release (feat!: add context.Context to TokenStore methods) (newreleases.io)
[3] v0.43.0 release (adds Implementation.Title, Roots, SessionIdManagerResolver, etc.) (github.com)
[4] v0.43.2 release notes (newreleases.io)
Citations:
- 1: https://newreleases.io/project/github/mark3labs/mcp-go/release/v0.37.0?utm_source=openai
- 2: https://newreleases.io/project/github/mark3labs/mcp-go/release/v0.40.0?utm_source=openai
- 3: https://github.com/mark3labs/mcp-go/releases?utm_source=openai
- 4: https://newreleases.io/project/github/mark3labs/mcp-go/release/v0.43.2?utm_source=openai
🌐 Web query:
mark3labs mcp-go v0.43.2 release notes changelog
💡 Result:
mark3labs/mcp-go v0.43.2 — changelog / release notes (released 2025-12-04)
What’s changed
- fix: notification break the client tool call (fix: raw output not being a valid json #642)
- fix: handle either
$defsordefinitionswhen unmarshalingToolArgumentsSchema(Cosmo Router incorrectly checks the wrong "scopes" claim #645) [1]
Full changelog compare: v0.43.1...v0.43.2 [1]
Sources
- [1] (github.com)
Citations:
🌐 Web query:
mark3labs mcp-go v0.38.0 v0.39.0 v0.40.0 regression issues
💡 Result:
mark3labs/mcp-go regressions / breaking changes around v0.38.0–v0.40.0
-
v0.38.0 (released Aug 20, 2025): shipped fixes for stdio transport concurrency (race condition on concurrent tool calls) and CallToolResult JSON marshal/unmarshal correctness—so if you were seeing flaky stdio behavior or incorrect tool-result JSON before v0.38, upgrading to v0.38 addressed those. [1]
-
v0.39.0 (released Sep 2, 2025): changed stdio transport to use
bufio.Scanner“to avoid panic when … outputs a long line”. This is a common source of regressions with large JSON-RPC messages becausebufio.Scannerhas practical token-size limits and can fail/truncate on large payloads. [2]- This was later effectively confirmed/fixed when v0.41.1 replaced
bufio.Scannerwithbufio.Reader“to support large messages”. If you hit “large message” truncation/failures after moving to v0.39.x, upgrading to >= v0.41.1 (or backporting that change) is the main remediation. [4]
- This was later effectively confirmed/fixed when v0.41.1 replaced
-
v0.40.0 (released Sep 19, 2025): includes a breaking API change:
TokenStoremethods gained acontext.Contextparameter (feat!). Any custom TokenStore implementations will fail to compile until updated—this is often experienced as a “regression” during upgrade even though it’s an intentional breaking change. [3]
Sources: GitHub release notes for v0.38.0 [1], v0.39.0 [2], v0.40.0 [3], and the later large-message fix in v0.41.1 [4].
Urgent: router/go.mod mcp-go v0.43.2 introduces multiple breaking changes with no visible code migration.
router/go.mod line 81 jumps from the project's baseline v0.36.0 (used in router-tests/go.mod and demo/go.mod) to v0.43.2, which includes confirmed breaking API changes:
- v0.37.0:
Result.Metatype changed frommap[string]any→*mcp.Meta - v0.40.0:
TokenStoreinterface methods now requirecontext.Contextparameter - v0.43.0:
mcp.ImplementationgainedTitlefield (breaks unkeyed struct literals)
Router code actively imports and uses mcp-go/mcp and mcp-go/server (in server.go, code_mode_server.go). The diff provides no evidence that these breaking changes were handled. Either downgrade to v0.36.0 and migrate router-tests in a separate PR, or provide a detailed code migration diff showing all API call updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router/go.mod` at line 81, The module bump to github.com/mark3labs/mcp-go
v0.43.2 introduces breaking changes; either pin back to the known-compatible
v0.36.0 in router/go.mod or migrate router code to the new API: update all
usages of Result.Meta to accept/handle *mcp.Meta (change types and any map
access accordingly), update any TokenStore implementations and calls (in
server.go and code_mode_server.go) to add context.Context as the first parameter
to the affected methods, and fix construction of mcp.Implementation values by
switching to keyed struct literals and populating the new Title field where
applicable; search for Result.Meta, TokenStore, and mcp.Implementation usages in
server.go and code_mode_server.go to make these targeted edits.
Member
Author
There was a problem hiding this comment.
No action needed — we use the mcp-go version required by this project.
Contributor
There was a problem hiding this comment.
@jensneuse, understood! I acknowledge that the version bump to v0.43.2 is intentional for this project's requirements. Since the new MCP Code Mode functionality was developed with this version in mind, the API changes are already incorporated into the implementation.
🧠 Learnings used
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Learnt from: asoorm
Repo: wundergraph/cosmo PR: 2438
File: router-tests/go.mod:15-15
Timestamp: 2026-01-05T11:53:58.709Z
Learning: For mcp-go dependency in wundergraph/cosmo, patch releases may not have formal release notes published. Tag diffs should be reviewed to verify patch-level changes contain no API surface changes or breaking behavior before adopting new patch versions.
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Comment on lines
+1
to
+43
| openapi: "3.0.3" | ||
| info: | ||
| title: Yoko Query Generation API | ||
| description: REST API for generating GraphQL queries from natural language prompts. | ||
| version: "1.0.0" | ||
| servers: | ||
| - url: https://yoko.example.com | ||
| description: Production | ||
| paths: | ||
| /v1/generate: | ||
| post: | ||
| operationId: generateQueries | ||
| summary: Generate GraphQL queries from a natural language prompt | ||
| requestBody: | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/GenerateRequest' | ||
| responses: | ||
| '200': | ||
| description: Successfully generated queries | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/GenerateResponse' | ||
| '400': | ||
| description: Bad request or could not generate query | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/ErrorResponse' | ||
| '401': | ||
| description: Unauthorized | ||
| '500': | ||
| description: Internal server error | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/ErrorResponse' | ||
| security: | ||
| - bearerAuth: [] | ||
| components: |
Contributor
There was a problem hiding this comment.
Set a global security default for the API spec.
Line 41 secures the current operation, but without top-level security, future paths can be added unauthenticated by mistake.
Proposed OpenAPI hardening
servers:
- url: https://yoko.example.com
description: Production
+security:
+ - bearerAuth: []
paths:
/v1/generate:
post:
@@
- security:
- - bearerAuth: []📝 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.
Suggested change
| openapi: "3.0.3" | |
| info: | |
| title: Yoko Query Generation API | |
| description: REST API for generating GraphQL queries from natural language prompts. | |
| version: "1.0.0" | |
| servers: | |
| - url: https://yoko.example.com | |
| description: Production | |
| paths: | |
| /v1/generate: | |
| post: | |
| operationId: generateQueries | |
| summary: Generate GraphQL queries from a natural language prompt | |
| requestBody: | |
| required: true | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/GenerateRequest' | |
| responses: | |
| '200': | |
| description: Successfully generated queries | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/GenerateResponse' | |
| '400': | |
| description: Bad request or could not generate query | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/ErrorResponse' | |
| '401': | |
| description: Unauthorized | |
| '500': | |
| description: Internal server error | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/ErrorResponse' | |
| security: | |
| - bearerAuth: [] | |
| components: | |
| openapi: "3.0.3" | |
| info: | |
| title: Yoko Query Generation API | |
| description: REST API for generating GraphQL queries from natural language prompts. | |
| version: "1.0.0" | |
| servers: | |
| - url: https://yoko.example.com | |
| description: Production | |
| security: | |
| - bearerAuth: [] | |
| paths: | |
| /v1/generate: | |
| post: | |
| operationId: generateQueries | |
| summary: Generate GraphQL queries from a natural language prompt | |
| requestBody: | |
| required: true | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/GenerateRequest' | |
| responses: | |
| '200': | |
| description: Successfully generated queries | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/GenerateResponse' | |
| '400': | |
| description: Bad request or could not generate query | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/ErrorResponse' | |
| '401': | |
| description: Unauthorized | |
| '500': | |
| description: Internal server error | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/ErrorResponse' | |
| components: |
🧰 Tools
🪛 Checkov (3.2.334)
[high] 1-102: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router/pkg/yokoclient/mock_openapi.yaml` around lines 1 - 43, The OpenAPI
spec lacks a top-level security requirement so new paths could be
unauthenticated; add a global security requirement (security: - bearerAuth: [])
and define the bearerAuth scheme under components.securitySchemes (e.g., type:
http, scheme: bearer, bearerFormat: JWT) so the API (including operationId
generateQueries) is protected by default while keeping or overriding
per-operation security as needed.
Member
Author
There was a problem hiding this comment.
Won't fix — this is a mock OpenAPI spec for testing, not a deployed API.
Contributor
There was a problem hiding this comment.
@jensneuse, understood! Since this is a mock spec for local testing and development, the security hardening isn't necessary. Thanks for the clarification.
🧠 Learnings used
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Comment on lines
+211
to
+227
| func (m *MockClient) findRootType(opType ast.OperationType) (int, bool) { | ||
| for _, node := range m.schema.RootNodes { | ||
| if node.Kind == ast.NodeKindObjectTypeDefinition { | ||
| name := m.schema.ObjectTypeDefinitionNameString(node.Ref) | ||
| switch opType { | ||
| case ast.OperationTypeQuery: | ||
| if name == "Query" { | ||
| return node.Ref, true | ||
| } | ||
| case ast.OperationTypeMutation: | ||
| if name == "Mutation" { | ||
| return node.Ref, true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return -1, false |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and understand the context
cd router/pkg/yokoclient && ls -laRepository: wundergraph/cosmo
Length of output: 495
🏁 Script executed:
# Examine the mock.go file around the flagged lines
head -250 mock.go | tail -60Repository: wundergraph/cosmo
Length of output: 127
🏁 Script executed:
# Check if there's a schema definition file to understand the data structure
find router/pkg -name "*schema*" -type f | head -20Repository: wundergraph/cosmo
Length of output: 363
🏁 Script executed:
# Look for imports and dependencies in mock.go
head -30 mock.goRepository: wundergraph/cosmo
Length of output: 127
🏁 Script executed:
# Read the mock.go file from the repository root
head -30 router/pkg/yokoclient/mock.goRepository: wundergraph/cosmo
Length of output: 1038
🏁 Script executed:
# Read the specific lines mentioned in the review (211-227)
sed -n '211,227p' router/pkg/yokoclient/mock.goRepository: wundergraph/cosmo
Length of output: 526
🏁 Script executed:
# Get more context around the function
sed -n '200,240p' router/pkg/yokoclient/mock.goRepository: wundergraph/cosmo
Length of output: 1121
🏁 Script executed:
# Check the schema definition structure
find . -path ./dist -prune -o -name "*.go" -type f -exec grep -l "RootNodes\|RootOperationType" {} \; | head -10Repository: wundergraph/cosmo
Length of output: 712
🏁 Script executed:
# Search for RootOperationTypeDefinition or SchemaDefinition in the codebase
rg "RootOperationTypeDefinition|SchemaDefinition" --type go | head -30Repository: wundergraph/cosmo
Length of output: 1320
🏁 Script executed:
# Look for how the schema is typically structured and initialized
rg "schema\s*{" router/pkg/yokoclient/ --type go -A 3 -B 3Repository: wundergraph/cosmo
Length of output: 144
🏁 Script executed:
# Check test cases in mock_test.go to understand expected behavior
cat router/pkg/yokoclient/mock_test.goRepository: wundergraph/cosmo
Length of output: 5301
🏁 Script executed:
# Search for any existing root operation type handling in the client
rg "OperationType|schema.*query|schema.*mutation" router/pkg/yokoclient/ --type goRepository: wundergraph/cosmo
Length of output: 789
🏁 Script executed:
# Check if the ast package has SchemaDefinition or root operation types
rg "SchemaDefinition|RootOperation" --type go | grep -E "(graphql-go-tools|ast\\.)" | head -20Repository: wundergraph/cosmo
Length of output: 257
🏁 Script executed:
# Look for how other parts of the codebase handle schema root type definitions
rg "QueryRoot|MutationRoot|SubscriptionRoot" --type go | head -20Repository: wundergraph/cosmo
Length of output: 1554
🏁 Script executed:
# Check the graphql-go-tools library usage in the codebase
find . -path "*/graphql-go-tools*" -prune -o -name "*.go" -exec grep -l "ast\\.Document" {} \; | xargs grep -l "Query\|Mutation" | head -10Repository: wundergraph/cosmo
Length of output: 449
🏁 Script executed:
# Look at the actual AST Document structure being used
rg "ast\.Document" router/pkg/yokoclient/mock.go -B 5 -A 10 | head -40Repository: wundergraph/cosmo
Length of output: 774
🏁 Script executed:
# Check if there's documentation or examples showing schema definition handling
find . -name "*.go" -type f -exec grep -l "schema.*query.*:" {} \; | head -5Repository: wundergraph/cosmo
Length of output: 253
🏁 Script executed:
# Search for how schema definitions are accessed in the graphql-go-tools library
rg "SchemaDefinition|GetSchemaDefinition" --type go | head -20Repository: wundergraph/cosmo
Length of output: 1320
🏁 Script executed:
# Look for examples of how root operation types are resolved in the codebase
rg "Query.*type|Mutation.*type|OperationType" router/ --type go -B 2 -A 2 | head -50Repository: wundergraph/cosmo
Length of output: 2741
🏁 Script executed:
# Check the actual graphql-go-tools AST Document structure or documentation
find . -type f -name "*.go" -exec grep -l "ast.Document" {} \; | xargs grep "SchemaDefinition\|RootOperation" | head -10Repository: wundergraph/cosmo
Length of output: 984
🏁 Script executed:
# Look for any recent changes or TODOs related to schema handling
rg "TODO|FIXME|BUG" router/pkg/yokoclient/ --type goRepository: wundergraph/cosmo
Length of output: 43
🏁 Script executed:
# Check what fields are available on ast.Document by searching for usages
rg "m\.schema\." router/pkg/yokoclient/mock.go --type goRepository: wundergraph/cosmo
Length of output: 1146
🏁 Script executed:
# Search for SchemaDefinition usage in the graphql-go-tools or how root types should be resolved
rg "SchemaDefinition\|GetQuery\|GetMutation" --type go | head -20Repository: wundergraph/cosmo
Length of output: 43
🏁 Script executed:
# Check if there are any examples in the codebase showing custom root type names
rg "schema.*{" router/ --type go -A 5 | grep -E "(query|mutation|subscription)" | head -20Repository: wundergraph/cosmo
Length of output: 142
🏁 Script executed:
# Look at how operation_processor handles root types
grep -n "Query\|Mutation" router/core/operation_processor.go | head -20Repository: wundergraph/cosmo
Length of output: 1916
🏁 Script executed:
# Create a test schema with custom root type names to understand the problem
python3 << 'EOF'
# Check if the current implementation would fail with custom root type names
test_schema_custom = """
schema {
query: RootQuery
mutation: RootMutation
}
type RootQuery {
hello: String
}
type RootMutation {
createUser(name: String!): String
}
"""
test_schema_standard = """
schema {
query: Query
mutation: Mutation
}
type Query {
hello: String
}
type Mutation {
createUser(name: String!): String
}
"""
print("Custom root type names schema:")
print(test_schema_custom)
print("\n" + "="*50)
print("Standard root type names schema:")
print(test_schema_standard)
print("\nIssue: The current findRootType() only looks for 'Query' and 'Mutation' names")
print("It would FAIL for the custom schema since RootQuery and RootMutation are not found")
EOFRepository: wundergraph/cosmo
Length of output: 626
🏁 Script executed:
# Check if ast.Document has SchemaDefinition field
rg "ast\.Document\s*struct" . --type go -A 30 | head -50Repository: wundergraph/cosmo
Length of output: 43
🏁 Script executed:
# Look for how the schema root types should be accessed properly
rg "QueryRoot|MutationRoot|SchemaDefinition" . --type go | grep -v "vendor\|dist\|node_modules" | head -20Repository: wundergraph/cosmo
Length of output: 2179
🏁 Script executed:
# Look at what methods are available on ast.Document in the codebase
rg "m\.schema\.[A-Za-z]+" router/pkg/yokoclient/mock.go --type go -o | sort | uniqRepository: wundergraph/cosmo
Length of output: 387
🏁 Script executed:
# Check if there's a SchemaDefinitions or similar field we can access
rg "SchemaDefinitions\|QueryRootTypeName\|MutationRootTypeName" . --type go | head -20Repository: wundergraph/cosmo
Length of output: 43
🏁 Script executed:
# Look at the full mock.go to see all uses of the schema object
cat router/pkg/yokoclient/mock.goRepository: wundergraph/cosmo
Length of output: 6380
🏁 Script executed:
# Search for how the graphql-go-tools library handles SchemaDefinitions
find . -type f -name "*.go" -exec grep -l "SchemaDefinitions\|Document.*Schema" {} \; | head -10Repository: wundergraph/cosmo
Length of output: 386
🏁 Script executed:
# Check the go.mod to see the exact version of graphql-go-tools being used
cat go.mod | grep graphql-go-toolsRepository: wundergraph/cosmo
Length of output: 99
🏁 Script executed:
# Look for any usage patterns of schema definitions in the codebase
rg "SchemaDefinition|QueryRootTypeName" . --type go | head -30Repository: wundergraph/cosmo
Length of output: 1350
🏁 Script executed:
# Create a test to see what happens with custom root type names
cat > /tmp/test_root_types.go << 'EOF'
package main
import (
"fmt"
)
// The issue: findRootType() checks if name == "Query" or name == "Mutation"
// But GraphQL schemas can define custom root types via: schema { query: RootQuery }
// In that case, the root type object would be named "RootQuery", not "Query"
// Example schemas:
const standardSchema = `
schema {
query: Query
mutation: Mutation
}
type Query { hello: String }
type Mutation { update: String }
`
const customSchema = `
schema {
query: RootQuery
mutation: RootMutation
}
type RootQuery { hello: String }
type RootMutation { update: String }
`
const noSchemaDirective = `
type Query { hello: String }
type Mutation { update: String }
`
func main() {
fmt.Println("Standard Schema (works):")
fmt.Println(standardSchema)
fmt.Println("\nCustom Root Types Schema (would FAIL):")
fmt.Println(customSchema)
fmt.Println("\nNo Schema Directive (would FAIL):")
fmt.Println(noSchemaDirective)
fmt.Println("\nThe current findRootType() hardcodes checking for 'Query' and 'Mutation' names.")
fmt.Println("It should instead read from the schema directive to get the actual root type names.")
}
EOF
python3 << 'EOFPY'
# The real issue is that the code needs to:
# 1. Look for a SchemaDefinition in the ast.Document
# 2. Extract the query, mutation, subscription root type names from it
# 3. Use those names instead of hardcoded "Query"/"Mutation"
print("Issue confirmed:")
print("- findRootType() only matches hardcoded 'Query' and 'Mutation' names")
print("- Fails for schemas using: schema { query: RootQuery mutation: RootMutation }")
print("- Should read root type names from SchemaDefinition instead")
EOFPYRepository: wundergraph/cosmo
Length of output: 284
🏁 Script executed:
# Check if ast.Document has SchemaDefinitions field
rg "\.SchemaDefinitions\s*\[" . --type go | head -10Repository: wundergraph/cosmo
Length of output: 43
🏁 Script executed:
# Look for examples of accessing schema definitions in the codebase
rg "SchemaDefinition\|QueryRootTypeName\|GetQuery" router/ --type go -B 2 -A 2 | head -50Repository: wundergraph/cosmo
Length of output: 43
🏁 Script executed:
# Check if there are tests with custom root type names
rg "schema.*query.*:|schema.*mutation.*:" router/ --type go | head -20Repository: wundergraph/cosmo
Length of output: 6650
🏁 Script executed:
# Look at the plan_generator or operation_processor to see how they handle root types
grep -n "RootNodes\|RootType\|Query\|Mutation" router/core/operation_processor.go | head -30Repository: wundergraph/cosmo
Length of output: 2960
🏁 Script executed:
# Search for any references to how to properly get root operation types
rg "QueryRoot|SubscriptionRoot|operation.*root\|root.*operation" router/ --type go -i | head -20Repository: wundergraph/cosmo
Length of output: 572
🏁 Script executed:
# Check the ast package in graphql-go-tools by looking at what's imported
grep -A 50 "import" router/pkg/yokoclient/mock.go | head -60Repository: wundergraph/cosmo
Length of output: 1536
🏁 Script executed:
# Look for web documentation or comments about the ast.Document structure
web_search
graphql-go-tools ast.Document SchemaDefinition root typesRepository: wundergraph/cosmo
Length of output: 166
🌐 Web query:
graphql-go-tools ast.Document SchemaDefinition root operation types
💡 Result:
In graphql-go-tools (v2), the root operation types live in the AST as a two-step indirection:
-
ast.Document.SchemaDefinitions[]containsast.SchemaDefinitionnodes. EachSchemaDefinitionhas aRootOperationTypeDefinitionsfield of typeRootOperationTypeDefinitionList, which is basically a list of integer refs (Refs []int). Those refs point into… [1] -
ast.Document.RootOperationTypeDefinitions[]which stores the actualast.RootOperationTypeDefinitionentries (each one is an(OperationType, NamedType)pair, e.g.query: Query). TheDocumentstruct in v2 explicitly has both slices:SchemaDefinitions []SchemaDefinitionandRootOperationTypeDefinitions []RootOperationTypeDefinition. [2]
So, conceptually:
schema { query: Query mutation: Mutation subscription: Subscription }- becomes:
- a
SchemaDefinitionwhoseRootOperationTypeDefinitions.Refs = []int{...} - and each ref indexes
document.RootOperationTypeDefinitions[ref]
- a
Docs/example that show constructing it this way (create RootOperationTypeDefinition, append it to doc.RootOperationTypeDefinitions, then reference it from SchemaDefinition.RootOperationTypeDefinitions.Refs) are in the ast package docs. [1]
Sources: [1] [2]
Resolve root operation types from schema definitions, not hardcoded names.
findRootType() only matches object names "Query" and "Mutation". GraphQL allows custom root type names via schema { query: RootQuery mutation: RootMutation }. The method should read from ast.Document.SchemaDefinitions and RootOperationTypeDefinitions instead of hardcoding type names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router/pkg/yokoclient/mock.go` around lines 211 - 227, findRootType currently
hardcodes "Query"/"Mutation"; instead read the schema's explicit root operation
types and resolve by definition names: in MockClient.findRootType, inspect
m.schema.SchemaDefinitions (or m.schema.Document.SchemaDefinitions) and their
RootOperationTypeDefinitions to determine the type name (or directly the
referenced node ref) for ast.OperationTypeQuery and ast.OperationTypeMutation,
then match that name/ref against m.schema.RootNodes (or object definitions)
rather than the literal strings. Keep existing behavior as a fallback by using
the standard "Query"/"Mutation" when no schema block is present.
Member
Author
There was a problem hiding this comment.
Won't fix — mock implementation for testing purposes. The hardcoded root types are sufficient for the mock.
Contributor
There was a problem hiding this comment.
@jensneuse, understood! For a mock implementation used in testing, the simplified approach with hardcoded root types is perfectly reasonable. Thank you for clarifying the scope and intent.
🧠 Learnings used
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Learnt from: asoorm
Repo: wundergraph/cosmo PR: 2379
File: router/pkg/connectrpc/operation_registry_test.go:381-399
Timestamp: 2026-01-06T12:37:21.521Z
Learning: In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine, letting the WaitGroup manage Add/Done automatically. Avoid manual wg.Add(1) followed by go func() { defer wg.Done(); ... }() patterns. Apply this guidance across all Go files in the wundergraph/cosmo repository where concurrency is used.
Staticcheck S1039: fmt.Sprintf with no format directives is unnecessary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- runtime.go: explicitly handle RuntimeTypeGoja, default to qjs for unknown types instead of silently falling back to goja - runtime_qjs.go: check lockdown eval error instead of ignoring it - pool_test.go: actually call trackConcurrency() in concurrency test - yokoclient/client.go: accept "cosmo" auth type as alias for JWT, validate non-empty access_token before caching - config.schema.json: add missing max_input_size_bytes to sandbox schema, add token_endpoint/client_id/client_secret to auth schema Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
router/pkg/config/config.schema.json (2)
2491-2520:⚠️ Potential issue | 🟠 MajorEnforce auth credentials conditionally by
auth.type.Line 2491-2519 allows
type: "cosmo"withouttoken_endpoint/client_id/client_secret, andtype: "static"withoutstatic_token. This passes schema validation but can fail at runtime.Suggested schema patch
"auth": { "type": "object", "description": "Authentication configuration for the query generation service.", "additionalProperties": false, + "required": ["type"], "properties": { "type": { "type": "string", "enum": ["static", "cosmo"], "default": "static", "description": "Authentication type. 'static' uses a fixed token, 'cosmo' uses the Cosmo platform token." }, @@ "client_secret": { "type": "string", "description": "Client secret for client credentials auth flow." } - } + }, + "allOf": [ + { + "if": { + "properties": { "type": { "const": "static" } } + }, + "then": { + "required": ["static_token"] + } + }, + { + "if": { + "properties": { "type": { "const": "cosmo" } } + }, + "then": { + "required": ["token_endpoint", "client_id", "client_secret"] + } + } + ] },Based on learnings: Required-field validation for security/auth configuration is handled at the JSON schema level in this repository.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/config/config.schema.json` around lines 2491 - 2520, The auth schema currently allows mismatched credentials for auth.type; update the "auth" object to enforce conditional required fields: add JSON Schema conditional blocks using "if": {"properties": {"type": {"const": "static"}}} with "then": {"required": ["static_token"]} and another "if": {"properties": {"type": {"const": "cosmo"}}} with "then": {"required": ["token_endpoint","client_id","client_secret"]}; retain existing property definitions ("type","static_token","token_endpoint","client_id","client_secret") and keep "additionalProperties": false so validation fails when required fields for the selected auth.type are missing.
2454-2472:⚠️ Potential issue | 🟠 MajorAdd hard bounds for sandbox resource limits.
Line 2454, Line 2459, Line 2464, and Line 2469 define limit fields but allow unbounded/invalid integers. Please enforce at least minimum constraints (and ideally maximums aligned with runtime caps) at schema level.
Suggested schema patch
"max_memory_mb": { "type": "integer", "default": 16, + "minimum": 1, "description": "Maximum memory in MB available to the sandbox runtime." }, "max_fuel": { "type": "integer", "default": 10000000, + "minimum": 1, "description": "Maximum CPU fuel for sandbox execution." }, "max_input_size_bytes": { "type": "integer", "default": 65536, + "minimum": 1, "description": "Maximum size in bytes of the sandbox input code." }, "max_output_size_bytes": { "type": "integer", "default": 1048576, + "minimum": 1, "description": "Maximum size in bytes of the sandbox output." }Based on learnings: In the Cosmo router project, parameter validation is expected at the JSON schema level (with minimum constraints used to prevent invalid values).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/config/config.schema.json` around lines 2454 - 2472, The JSON schema properties max_memory_mb, max_fuel, max_input_size_bytes, and max_output_size_bytes currently lack bounds; update each property in the schema to include a "minimum" (to prevent zero/negative/invalid values) and add sensible "maximum" caps that align with runtime limits (for example, max_memory_mb: minimum 1 and a runtime-aligned maximum, max_fuel: minimum 1 and runtime cap, max_input_size_bytes: minimum 1 and an upper cap, max_output_size_bytes: minimum 1 and an upper cap). Modify the entries for "max_memory_mb", "max_fuel", "max_input_size_bytes", and "max_output_size_bytes" to include these "minimum" and "maximum" numeric constraints so validation is enforced at the schema level.
🧹 Nitpick comments (1)
router/pkg/yokoclient/client.go (1)
146-152: Normalizeauth.typebefore switching.Line [146] currently requires exact casing/spacing. Normalizing avoids config footguns (
" JWT "/"JWT").♻️ Suggested hardening
func (c *Client) getToken(ctx context.Context) (string, error) { - switch c.auth.Type { + authType := strings.ToLower(strings.TrimSpace(c.auth.Type)) + switch authType { case "static", "": return c.auth.StaticToken, nil case "jwt", "cosmo": return c.getJWTToken(ctx) default: - return "", fmt.Errorf("unsupported auth type: %s", c.auth.Type) + return "", fmt.Errorf("unsupported auth type: %s", c.auth.Type) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/yokoclient/client.go` around lines 146 - 152, The switch on c.auth.Type is case/whitespace-sensitive; normalize the value before switching by creating a local normalized variable (e.g., typ := strings.ToLower(strings.TrimSpace(c.auth.Type))) and switch on that instead of c.auth.Type; update the switch cases in the function that contains the switch (referenced by c.auth.Type and c.getJWTToken) to use the normalized variable and keep the same branches ("static","", "jwt","cosmo") and default error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/core/router.go`:
- Around line 1021-1034: The code starts a Code Mode server (codeModeSrv via
mcpserver.NewCodeModeServer and codeModeSrv.Start()) and assigns it to
r.codeModeServer, but if later bootstrap steps fail the server may remain
running; add bootstrap-level rollback cleanup that ensures any started auxiliary
servers are stopped on subsequent bootstrap errors by: tracking the started
server (codeModeSrv and r.codeModeServer), and in the outer bootstrap error path
ensure you call Stop(ctx) on codeModeSrv (or r.codeModeServer) and log warnings
on Stop errors (similar to the existing inner cleanup), so any failure after
assigning r.codeModeServer triggers a deterministic stop and prevents leaks.
- Around line 993-1000: The code is forcing wildcard origins by setting
cfg.AllowOrigins = []string{"*"} which overrides user policy; remove that
assignment and preserve r.corsOptions.AllowOrigins when copying cfg, only ensure
required MCP methods and headers are present (set cfg.AllowMethods to include
"GET","PUT","POST","DELETE","OPTIONS" and append the MCP headers to
cfg.AllowHeaders), then assign corsConfig = &cfg; also ensure you do not
introduce a wildcard origin when credentials are enabled (leave AllowOrigins
untouched instead of forcing "*").
In `@router/pkg/config/config.go`:
- Around line 1084-1089: The QueryGenerationAuth struct's fields (Type,
TokenEndpoint, ClientID, ClientSecret, StaticToken) lack env struct tags so
env.Parse cannot populate them; update the QueryGenerationAuth definition to add
appropriate `env:"..."` tags (and keep `envDefault` for Type) matching the field
names used elsewhere (e.g., `env:"QUERY_GENERATION_AUTH_TYPE"`,
`env:"QUERY_GENERATION_TOKEN_ENDPOINT"`, `env:"QUERY_GENERATION_CLIENT_ID"`,
`env:"QUERY_GENERATION_CLIENT_SECRET"`, and `env:"YOKO_TOKEN"` for StaticToken)
so that env.Parse called in the QueryGenerationConfiguration flow will inject
values into QueryGenerationAuth. Ensure tags follow the same naming convention
used by other nested configs like MCPSessionConfig and MCPStorageConfig.
---
Duplicate comments:
In `@router/pkg/config/config.schema.json`:
- Around line 2491-2520: The auth schema currently allows mismatched credentials
for auth.type; update the "auth" object to enforce conditional required fields:
add JSON Schema conditional blocks using "if": {"properties": {"type": {"const":
"static"}}} with "then": {"required": ["static_token"]} and another "if":
{"properties": {"type": {"const": "cosmo"}}} with "then": {"required":
["token_endpoint","client_id","client_secret"]}; retain existing property
definitions ("type","static_token","token_endpoint","client_id","client_secret")
and keep "additionalProperties": false so validation fails when required fields
for the selected auth.type are missing.
- Around line 2454-2472: The JSON schema properties max_memory_mb, max_fuel,
max_input_size_bytes, and max_output_size_bytes currently lack bounds; update
each property in the schema to include a "minimum" (to prevent
zero/negative/invalid values) and add sensible "maximum" caps that align with
runtime limits (for example, max_memory_mb: minimum 1 and a runtime-aligned
maximum, max_fuel: minimum 1 and runtime cap, max_input_size_bytes: minimum 1
and an upper cap, max_output_size_bytes: minimum 1 and an upper cap). Modify the
entries for "max_memory_mb", "max_fuel", "max_input_size_bytes", and
"max_output_size_bytes" to include these "minimum" and "maximum" numeric
constraints so validation is enforced at the schema level.
---
Nitpick comments:
In `@router/pkg/yokoclient/client.go`:
- Around line 146-152: The switch on c.auth.Type is case/whitespace-sensitive;
normalize the value before switching by creating a local normalized variable
(e.g., typ := strings.ToLower(strings.TrimSpace(c.auth.Type))) and switch on
that instead of c.auth.Type; update the switch cases in the function that
contains the switch (referenced by c.auth.Type and c.getJWTToken) to use the
normalized variable and keep the same branches ("static","", "jwt","cosmo") and
default error handling.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
router-tests/go.modrouter/core/router.gorouter/core/router_config.gorouter/go.modrouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.jsonrouter/pkg/mcpserver/code_mode_server_test.gorouter/pkg/sandbox/pool_test.gorouter/pkg/sandbox/runtime.gorouter/pkg/sandbox/runtime_qjs.gorouter/pkg/yokoclient/client.go
🚧 Files skipped from review as they are similar to previous changes (4)
- router/core/router_config.go
- router/pkg/sandbox/runtime_qjs.go
- router/pkg/sandbox/pool_test.go
- router/pkg/mcpserver/code_mode_server_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2575 +/- ##
===========================================
- Coverage 64.58% 32.93% -31.66%
===========================================
Files 301 236 -65
Lines 42900 26349 -16551
Branches 4601 0 -4601
===========================================
- Hits 27709 8678 -19031
- Misses 15170 16592 +1422
- Partials 21 1079 +1058
🚀 New features to boost your workflow:
|
TestQJS_AsyncFunctionWithAwait is typically the first QJS test to run, incurring the WASM module cold-start initialization cost. On GitHub Actions runners this can exceed the default 5s timeout. Increase to 30s. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/pkg/sandbox/runtime_qjs_test.go (1)
100-105: Consider using require for safer type assertions.The direct type assertions on lines 102-104 will panic with opaque messages if the structure differs from expectations. Using testify's type assertion helpers or two-value form would yield clearer failure diagnostics.
💡 Optional: Safer type assertions
var obj map[string]any require.NoError(t, json.Unmarshal(result.Value, &obj)) - user := obj["user"].(map[string]any) - assert.Equal(t, "Alice", user["name"]) - orders := obj["orders"].([]any) + user, ok := obj["user"].(map[string]any) + require.True(t, ok, "expected user to be map[string]any") + assert.Equal(t, "Alice", user["name"]) + orders, ok := obj["orders"].([]any) + require.True(t, ok, "expected orders to be []any") assert.Len(t, orders, 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/sandbox/runtime_qjs_test.go` around lines 100 - 105, The test currently uses direct type assertions on obj["user"] and obj["orders"] which can panic; change those to safe assertions using the two-value type assertion and testify require helpers: e.g., retrieve obj (map[string]any) as now, then for the user field use u, ok := obj["user"].(map[string]any) and call require.True(t, ok, "user must be a map"), then use require.Equal(t, "Alice", u["name"]); similarly for orders use o, ok := obj["orders"].([]any) and require.True(t, ok, "orders must be a slice") followed by require.Len(t, o, 1). This updates the unsafe casts around variables obj, user and orders to produce clearer test failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router/pkg/sandbox/runtime_qjs_test.go`:
- Around line 100-105: The test currently uses direct type assertions on
obj["user"] and obj["orders"] which can panic; change those to safe assertions
using the two-value type assertion and testify require helpers: e.g., retrieve
obj (map[string]any) as now, then for the user field use u, ok :=
obj["user"].(map[string]any) and call require.True(t, ok, "user must be a map"),
then use require.Equal(t, "Alice", u["name"]); similarly for orders use o, ok :=
obj["orders"].([]any) and require.True(t, ok, "orders must be a slice") followed
by require.Len(t, o, 1). This updates the unsafe casts around variables obj,
user and orders to produce clearer test failures.
Remove the dual-runtime abstraction from the sandbox package. The goja (pure-Go) JS runtime was a fallback that lacked native Promise support and WASM memory isolation. With QuickJS as the sole runtime, the API is simplified: NewRuntime/NewPool/NewTranspiler no longer take a runtime type parameter, and the stripAsyncAwait transpiler path is removed. Also adds missing env tags to QueryGenerationAuth config fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
router/core/router.go (2)
993-999:⚠️ Potential issue | 🟠 MajorPreserve configured CORS origins in Code Mode.
Line [996] forces
AllowOriginsto*, overriding user policy and potentially widening exposure unexpectedly.🛠️ Suggested fix
var corsConfig *cors.Config if r.corsOptions != nil && r.corsOptions.Enabled { cfg := *r.corsOptions - cfg.AllowOrigins = []string{"*"} - cfg.AllowMethods = []string{"GET", "PUT", "POST", "DELETE", "OPTIONS"} - cfg.AllowHeaders = append(cfg.AllowHeaders, "Content-Type", "Accept", "Authorization", "Last-Event-ID", "Mcp-Protocol-Version", "Mcp-Session-Id") + cfg.AllowMethods = stringsx.RemoveDuplicates(append(cfg.AllowMethods, "GET", "PUT", "POST", "DELETE", "OPTIONS")) + cfg.AllowHeaders = stringsx.RemoveDuplicates(append(cfg.AllowHeaders, + "Content-Type", "Accept", "Authorization", + "Last-Event-ID", "Mcp-Protocol-Version", "Mcp-Session-Id", + )) + if len(cfg.AllowOrigins) == 1 && cfg.AllowOrigins[0] == "*" { + cfg.AllowCredentials = false + } corsConfig = &cfg }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/router.go` around lines 993 - 999, The code currently forces cfg.AllowOrigins = []string{"*"} which overrides user-specified CORS origins; update the block that builds corsConfig (referencing r.corsOptions, cfg.AllowOrigins, and corsConfig) so it preserves any user-provided cfg.AllowOrigins and only sets a wildcard when no origins were configured (or based on an explicit allow-all flag such as r.codeMode if intended), instead of unconditionally assigning "*". Ensure other fields (AllowMethods, AllowHeaders) remain unchanged.
1020-1033:⚠️ Potential issue | 🟠 MajorAdd bootstrap rollback for started Code Mode server.
If any later bootstrap step fails after Line [1032] (for example in ConnectRPC initialization), bootstrap returns while the Code Mode listener may remain running. Please add bootstrap-level deferred cleanup for
r.codeModeServeron subsequent errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/router.go` around lines 1020 - 1033, After successfully creating and starting the Code Mode server (codeModeSrv.Start()) and assigning it to r.codeModeServer, register a bootstrap-level rollback that will Stop the server if any subsequent bootstrap step fails (e.g., during ConnectRPC). Implement this by adding a deferred cleanup or pushing a cleanup func into the existing bootstrap cleanup list that calls r.codeModeServer.Stop(ctx) (or codeModeSrv.Stop(ctx) if still in scope), checks/returns Stop error via logger (r.logger.Warn or similar), and ensures the server is stopped before returning an error from bootstrap; reference symbols: codeModeSrv, r.codeModeServer, Start(), Stop(ctx), ConnectRPC.router/pkg/sandbox/pool_test.go (1)
88-107:⚠️ Potential issue | 🟡 MinorConcurrency-limit assertion is currently too weak to validate pool enforcement.
trackConcurrency()returns immediately, so overlap is not guaranteed. The test can pass even if real in-flight concurrency isn’t exercised.🧪 Suggested strengthening
Fn: func(args []any) (any, error) { cur := concurrent.Add(1) defer concurrent.Add(-1) for { old := maxSeen.Load() if cur <= old || maxSeen.CompareAndSwap(old, cur) { break } } + time.Sleep(50 * time.Millisecond) // force overlap window return nil, nil }, }, }, nil, nil) @@ - require.True(t, maxSeen.Load() <= int32(2), "max concurrent %d exceeds pool size 2", maxSeen.Load()) + require.Equal(t, int32(2), maxSeen.Load(), "expected to saturate pool size 2, got %d", maxSeen.Load())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/sandbox/pool_test.go` around lines 88 - 107, The test's trackConcurrency() invocation returns immediately so tasks may not overlap; update the test (in pool_test.go) to force real overlap by turning the Fn closure used in trackConcurrency into a blocking worker: introduce a start barrier (e.g., startCh) that each Fn waits on until all goroutines are launched, then release the barrier so they run concurrently; keep the existing maxSeen, concurrent, and the Fn closure logic but ensure Fn blocks on startCh before updating maxSeen and defers decrement, then close or signal startCh after spawning all workers and before wg.Wait(), so the require.True assertion actually observes in-flight concurrency <= pool size.
🧹 Nitpick comments (1)
router/pkg/mcpserver/code_mode_server.go (1)
680-683: Update stale function comment informatToolResult.The comment mentions a
toonflag, but the function has no such parameter and always attempts TOON first, then falls back to JSON/raw.Doc-only fix
-// formatToolResult returns the sandbox result formatted for the MCP response. -// When toon is true, the result is encoded as TOON (Token-Oriented Object Notation). -// Falls back to JSON on encoding failure or when toon is false. +// formatToolResult returns the sandbox result formatted for the MCP response. +// It attempts TOON (Token-Oriented Object Notation) encoding first and falls +// back to the raw JSON payload string on decode/encode failure. func (s *CodeModeServer) formatToolResult(raw json.RawMessage) string {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/code_mode_server.go` around lines 680 - 683, The doc comment for formatToolResult is stale: remove the reference to a non-existent 'toon' flag and accurately describe the current behavior of CodeModeServer.formatToolResult — it attempts to encode the sandbox result as TOON first, falls back to JSON on encoding failure, and returns the original raw message if both encodings fail; update the comment text accordingly and keep it aligned with the function signature and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/pkg/mcpserver/code_mode_server.go`:
- Around line 399-402: The mutation approval currently inspects the whole
document via isMutation(queryStr) and blocks even when a non-mutation operation
is selected; fix by determining the selected operation using the provided
operationName (optsMap["operationName"]) and only require approval if that
operation is a mutation: update the check around
s.config.RequireMutationApproval to parse the GraphQL document and inspect the
operation with the given operationName (or, when no operationName is supplied
and there are multiple operations, resolve the intended operation per GraphQL
rules or conservatively skip gating), e.g., add or replace isMutation(queryStr)
with a function like isSelectedOperationMutation(queryStr, operationName) and
call s.requestMutationApproval(ctx, queryStr, optsMap["variables"]) only when
that returns true.
- Around line 601-610: Replace the unbounded per-prompt goroutine loop by adding
a concurrency cap (e.g., a semaphore channel or worker pool) around the
s.yokoClient.Generate calls so at most N prompts run concurrently, and switch
the WaitGroup Add/Done pattern to the Go 1.25 style by using wg.Go(func() { ...
}) for each started worker; specifically, keep using the existing ch and
indexedResult to send results, call s.yokoClient.Generate(ctx, prompt,
schemaHash) inside the bounded worker, and ensure you close(ch) after wg.Wait()
as before.
- Around line 305-313: The search preamble references the helper
"__generate_queries" but s.generateQueriesFunc is only registered into
asyncFuncs when s.yokoClient != nil, causing runtime failures; update the search
path in code_mode_server.go to fail fast when Yoko is not configured by checking
s.yokoClient before building searchPreamble/jsCode and returning a clear error
(e.g., a tool/validation error) instead of continuing, or alternatively remove
the "__generate_queries" reference when s.yokoClient == nil; change the logic
around s.yokoClient, asyncFuncs, generateQueriesFunc, searchPreamble and jsCode
so they are consistent (either register the function when referenced or abort
with a descriptive error) to prevent the runtime failure.
- Around line 19-20: The imports in code_mode_server.go pull
github.com/mark3labs/mcp-go at v0.43.2 while this repo expects the stable
v0.36.0; either revert the module requirement to v0.36.0 or perform a
compatibility audit and explicitly bump to v0.43.2. Fix by updating the module
reference for github.com/mark3labs/mcp-go back to v0.36.0 (or, if keeping
v0.43.2, review tag diffs between v0.36.0 and v0.43.2 to confirm no breaking API
changes for symbols used by code_mode_server.go such as the mcp and server
packages and their referenced types/methods), then run go mod tidy and run tests
to ensure nothing breaks.
In `@router/pkg/sandbox/pool_test.go`:
- Around line 33-37: Replace manual wg.Add(1) + go func patterns with the
sync.WaitGroup.Go method: in the loop that currently uses for i := range 10 {
wg.Add(1); go func(idx int) { ... }(i) } capture the loop variable before
launching the goroutine (idx := i) and call wg.Go(func() { ... use idx ... })
instead; do the same for the second occurrence (the block around lines 76–80) so
both goroutine launches use wg.Go and closed-over loop variables are explicitly
captured.
In `@router/pkg/sandbox/pool.go`:
- Around line 39-49: Execute currently only checks p.closed before waiting for
p.sem which allows a race where Close runs after that check but before or during
semaphore acquisition; to fix, after successfully acquiring the semaphore in
Execute (the select case that sends to p.sem) immediately re-check
p.closed.Load(), and if it is true release the semaphore (read from p.sem) and
return ErrPoolClosed; apply the same post-acquire re-check pattern for the other
similar block around lines 60-62 so no execution proceeds after Close. Ensure
you reference the same p.sem and p.closed symbols and the Execute/Close
functions when making the changes.
In `@router/pkg/sandbox/runtime.go`:
- Around line 45-47: The Result.MarshalJSON method returns r.Value directly
which yields invalid JSON when r or r.Value is nil/empty; modify
Result.MarshalJSON to check for nil receiver or nil/zero-length Result.Value and
return the JSON null literal (i.e. []byte("null"), nil) in those cases,
otherwise return r.Value as before, ensuring Result.MarshalJSON and references
to Result.Value are updated accordingly.
---
Duplicate comments:
In `@router/core/router.go`:
- Around line 993-999: The code currently forces cfg.AllowOrigins =
[]string{"*"} which overrides user-specified CORS origins; update the block that
builds corsConfig (referencing r.corsOptions, cfg.AllowOrigins, and corsConfig)
so it preserves any user-provided cfg.AllowOrigins and only sets a wildcard when
no origins were configured (or based on an explicit allow-all flag such as
r.codeMode if intended), instead of unconditionally assigning "*". Ensure other
fields (AllowMethods, AllowHeaders) remain unchanged.
- Around line 1020-1033: After successfully creating and starting the Code Mode
server (codeModeSrv.Start()) and assigning it to r.codeModeServer, register a
bootstrap-level rollback that will Stop the server if any subsequent bootstrap
step fails (e.g., during ConnectRPC). Implement this by adding a deferred
cleanup or pushing a cleanup func into the existing bootstrap cleanup list that
calls r.codeModeServer.Stop(ctx) (or codeModeSrv.Stop(ctx) if still in scope),
checks/returns Stop error via logger (r.logger.Warn or similar), and ensures the
server is stopped before returning an error from bootstrap; reference symbols:
codeModeSrv, r.codeModeServer, Start(), Stop(ctx), ConnectRPC.
In `@router/pkg/sandbox/pool_test.go`:
- Around line 88-107: The test's trackConcurrency() invocation returns
immediately so tasks may not overlap; update the test (in pool_test.go) to force
real overlap by turning the Fn closure used in trackConcurrency into a blocking
worker: introduce a start barrier (e.g., startCh) that each Fn waits on until
all goroutines are launched, then release the barrier so they run concurrently;
keep the existing maxSeen, concurrent, and the Fn closure logic but ensure Fn
blocks on startCh before updating maxSeen and defers decrement, then close or
signal startCh after spawning all workers and before wg.Wait(), so the
require.True assertion actually observes in-flight concurrency <= pool size.
---
Nitpick comments:
In `@router/pkg/mcpserver/code_mode_server.go`:
- Around line 680-683: The doc comment for formatToolResult is stale: remove the
reference to a non-existent 'toon' flag and accurately describe the current
behavior of CodeModeServer.formatToolResult — it attempts to encode the sandbox
result as TOON first, falls back to JSON on encoding failure, and returns the
original raw message if both encodings fail; update the comment text accordingly
and keep it aligned with the function signature and behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
router/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
router/core/router.gorouter/go.modrouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.jsonrouter/pkg/mcpserver/code_mode_server.gorouter/pkg/sandbox/pool.gorouter/pkg/sandbox/pool_test.gorouter/pkg/sandbox/runtime.gorouter/pkg/sandbox/runtime_qjs_test.gorouter/pkg/sandbox/runtime_test.gorouter/pkg/sandbox/transpiler.gorouter/pkg/sandbox/transpiler_test.go
Comment on lines
+19
to
+20
| "github.com/mark3labs/mcp-go/mcp" | ||
| "github.com/mark3labs/mcp-go/server" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Scanning go.mod files for mcp-go version pins..."
fd '^go\.mod$' -HI | sort | while read -r mod; do
echo "== ${mod} =="
rg -n 'github\.com/mark3labs/mcp-go' "$mod" || echo " (no mcp-go dependency)"
doneRepository: wundergraph/cosmo
Length of output: 634
Address mcp-go version mismatch: router/go.mod and router-tests/go.mod use v0.43.2, not the known-stable v0.36.0.
The modules importing github.com/mark3labs/mcp-go in this PR are pinned to v0.43.2 (router/go.mod:80, router-tests/go.mod:16), not v0.36.0. Since v0.36.0 is documented as the stable version for this repo and patch releases of mcp-go lack formal release notes, review tag diffs between v0.36.0 and v0.43.2 to verify API compatibility and confirm no breaking behavior changes before keeping this version upgrade.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router/pkg/mcpserver/code_mode_server.go` around lines 19 - 20, The imports
in code_mode_server.go pull github.com/mark3labs/mcp-go at v0.43.2 while this
repo expects the stable v0.36.0; either revert the module requirement to v0.36.0
or perform a compatibility audit and explicitly bump to v0.43.2. Fix by updating
the module reference for github.com/mark3labs/mcp-go back to v0.36.0 (or, if
keeping v0.43.2, review tag diffs between v0.36.0 and v0.43.2 to confirm no
breaking API changes for symbols used by code_mode_server.go such as the mcp and
server packages and their referenced types/methods), then run go mod tidy and
run tests to ensure nothing breaks.
Member
Author
There was a problem hiding this comment.
No action needed — mcp-go version is consistent across modules.
Contributor
There was a problem hiding this comment.
@jensneuse, understood—the v0.43.2 version is indeed consistent across both router/go.mod and router-tests/go.mod.
Just to clarify, the original concern was about moving from the documented stable v0.36.0 (per the repo's learnings) to v0.43.2, which skips past v0.38.0 that had known regressions. If you've already verified compatibility with v0.43.2 and tested the MCP functionality, then we're all set. If you'd like, I can add a note to the knowledge base that v0.43.2 has been validated for this PR to help with future reviews.
🧠 Learnings used
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Learnt from: asoorm
Repo: wundergraph/cosmo PR: 2438
File: router-tests/go.mod:15-15
Timestamp: 2026-01-05T11:53:58.709Z
Learning: For mcp-go dependency in wundergraph/cosmo, patch releases may not have formal release notes published. Tag diffs should be reviewed to verify patch-level changes contain no API surface changes or breaking behavior before adopting new patch versions.
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Learnt from: asoorm
Repo: wundergraph/cosmo PR: 2379
File: router/pkg/connectrpc/operation_registry_test.go:381-399
Timestamp: 2026-01-06T12:37:21.521Z
Learning: In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine, letting the WaitGroup manage Add/Done automatically. Avoid manual wg.Add(1) followed by go func() { defer wg.Done(); ... }() patterns. Apply this guidance across all Go files in the wundergraph/cosmo repository where concurrency is used.
Comment on lines
+33
to
+37
| for i := range 10 { | ||
| wg.Add(1) | ||
| go func(idx int) { | ||
| defer wg.Done() | ||
| cur := current.Add(1) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check Go version requirement
echo "=== Checking go.mod for Go version ==="
head -n 5 go.mod | grep '^go'
# Check the actual code at the specified lines
echo -e "\n=== Lines 33-37 in router/pkg/sandbox/pool_test.go ==="
sed -n '33,37p' router/pkg/sandbox/pool_test.go | cat -n
# Check lines 76-80 mentioned as "Also applies to"
echo -e "\n=== Lines 76-80 in router/pkg/sandbox/pool_test.go ==="
sed -n '76,80p' router/pkg/sandbox/pool_test.go | cat -n
# Broader context for both sections
echo -e "\n=== Broader context 30-45 ==="
sed -n '30,45p' router/pkg/sandbox/pool_test.go | cat -n
echo -e "\n=== Broader context 73-83 ==="
sed -n '73,83p' router/pkg/sandbox/pool_test.go | cat -nRepository: wundergraph/cosmo
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find go.mod files
echo "=== Finding go.mod files ==="
find . -name "go.mod" -type f | head -20
# Find the pool_test.go file
echo -e "\n=== Finding pool_test.go ==="
find . -name "pool_test.go" -type fRepository: wundergraph/cosmo
Length of output: 360
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check Go version in router/go.mod
echo "=== Go version in router/go.mod ==="
head -n 5 router/go.mod
# Check the actual code at the specified lines
echo -e "\n=== Lines 33-37 in router/pkg/sandbox/pool_test.go ==="
sed -n '33,37p' router/pkg/sandbox/pool_test.go | cat -n
# Check lines 76-80 mentioned as "Also applies to"
echo -e "\n=== Lines 76-80 in router/pkg/sandbox/pool_test.go ==="
sed -n '76,80p' router/pkg/sandbox/pool_test.go | cat -n
# Broader context for both sections
echo -e "\n=== Broader context 25-45 ==="
sed -n '25,45p' router/pkg/sandbox/pool_test.go | cat -n
echo -e "\n=== Broader context 70-85 ==="
sed -n '70,85p' router/pkg/sandbox/pool_test.go | cat -nRepository: wundergraph/cosmo
Length of output: 1661
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full function body for the first goroutine (around line 33)
echo "=== Full context for first goroutine (lines 33-50) ==="
sed -n '33,50p' router/pkg/sandbox/pool_test.go | cat -n
# Get the full function body for the second goroutine (around line 76)
echo -e "\n=== Full context for second goroutine (lines 76-90) ==="
sed -n '76,90p' router/pkg/sandbox/pool_test.go | cat -nRepository: wundergraph/cosmo
Length of output: 1201
Prefer sync.WaitGroup.Go for goroutine launches in tests.
Both locations (lines 33–37 and 76–80) use manual wg.Add(1) + go func patterns. Convert to wg.Go(func() { ... }) for consistency with Go 1.25+ conventions. At lines 33–37, explicitly capture the loop variable (idx := i) before the wg.Go() call to replace the parameter-passing pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router/pkg/sandbox/pool_test.go` around lines 33 - 37, Replace manual
wg.Add(1) + go func patterns with the sync.WaitGroup.Go method: in the loop that
currently uses for i := range 10 { wg.Add(1); go func(idx int) { ... }(i) }
capture the loop variable before launching the goroutine (idx := i) and call
wg.Go(func() { ... use idx ... }) instead; do the same for the second occurrence
(the block around lines 76–80) so both goroutine launches use wg.Go and
closed-over loop variables are explicitly captured.
Member
Author
There was a problem hiding this comment.
Won't fix — the tests already use modern Go patterns where applicable. The WaitGroup.Go pattern is used in new code.
Contributor
There was a problem hiding this comment.
@jensneuse, understood! I'll respect your decision to keep the current patterns in the test file. Thanks for the clarification.
🧠 Learnings used
Learnt from: asoorm
Repo: wundergraph/cosmo PR: 2379
File: router/pkg/connectrpc/operation_registry_test.go:381-399
Timestamp: 2026-01-06T12:37:21.521Z
Learning: In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine, letting the WaitGroup manage Add/Done automatically. Avoid manual wg.Add(1) followed by go func() { defer wg.Done(); ... }() patterns. Apply this guidance across all Go files in the wundergraph/cosmo repository where concurrency is used.
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Comment on lines
+39
to
+49
| if p.closed.Load() { | ||
| return nil, ErrPoolClosed | ||
| } | ||
|
|
||
| // Acquire semaphore slot | ||
| select { | ||
| case p.sem <- struct{}{}: | ||
| defer func() { <-p.sem }() | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| } |
Contributor
There was a problem hiding this comment.
Close() and Execute() have a race that can admit executions after closure.
Execute checks closed only before waiting on the semaphore. If Close runs during that window, execution can still proceed, violating the documented behavior.
🔒 Suggested fix
type Pool struct {
config ExecutionConfig
sem chan struct{} // semaphore for concurrency control
closed atomic.Bool
+ done chan struct{}
}
@@
return &Pool{
config: config,
sem: make(chan struct{}, size),
+ done: make(chan struct{}),
}
}
@@
// Acquire semaphore slot
select {
+ case <-p.done:
+ return nil, ErrPoolClosed
case p.sem <- struct{}{}:
defer func() { <-p.sem }()
case <-ctx.Done():
return nil, ctx.Err()
}
+ if p.closed.Load() {
+ return nil, ErrPoolClosed
+ }
@@
func (p *Pool) Close() {
- p.closed.Store(true)
+ if p.closed.CompareAndSwap(false, true) {
+ close(p.done)
+ }
}Also applies to: 60-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router/pkg/sandbox/pool.go` around lines 39 - 49, Execute currently only
checks p.closed before waiting for p.sem which allows a race where Close runs
after that check but before or during semaphore acquisition; to fix, after
successfully acquiring the semaphore in Execute (the select case that sends to
p.sem) immediately re-check p.closed.Load(), and if it is true release the
semaphore (read from p.sem) and return ErrPoolClosed; apply the same
post-acquire re-check pattern for the other similar block around lines 60-62 so
no execution proceeds after Close. Ensure you reference the same p.sem and
p.closed symbols and the Execute/Close functions when making the changes.
Member
Author
There was a problem hiding this comment.
Won't fix — the race is benign. At worst, one extra execution runs during shutdown. The pool is not a security boundary.
Contributor
There was a problem hiding this comment.
@jensneuse, understood — thank you for the clarification. I'll note this for future reference.
✏️ Learnings added
Learnt from: jensneuse
Repo: wundergraph/cosmo PR: 2575
File: router/pkg/sandbox/pool.go:39-49
Timestamp: 2026-03-02T19:11:47.309Z
Learning: In router/pkg/sandbox/pool.go, the race condition between Close() and Execute() where an execution can proceed after closure is considered benign and acceptable. At worst, one extra execution runs during shutdown, and the pool is not treated as a security boundary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Learnt from: asoorm
Repo: wundergraph/cosmo PR: 2379
File: router/pkg/connectrpc/operation_registry_test.go:381-399
Timestamp: 2026-01-06T12:37:21.521Z
Learning: In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine, letting the WaitGroup manage Add/Done automatically. Avoid manual wg.Add(1) followed by go func() { defer wg.Done(); ... }() patterns. Apply this guidance across all Go files in the wundergraph/cosmo repository where concurrency is used.
Comment on lines
+45
to
+47
| func (r *Result) MarshalJSON() ([]byte, error) { | ||
| return r.Value, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd router/pkg/sandbox && head -80 runtime.go | cat -nRepository: wundergraph/cosmo
Length of output: 1921
🏁 Script executed:
rg -A 10 "type Result struct" router/pkg/sandbox/Repository: wundergraph/cosmo
Length of output: 608
🏁 Script executed:
cd router/pkg/sandbox && sed -n '30,70p' runtime.go | cat -nRepository: wundergraph/cosmo
Length of output: 1046
🏁 Script executed:
cat > /tmp/test_marshal.go << 'EOF'
package main
import (
"encoding/json"
"fmt"
)
type Result struct {
Value []byte
}
func (r *Result) MarshalJSON() ([]byte, error) {
return r.Value, nil
}
func main() {
// Test 1: nil Value
r1 := &Result{Value: nil}
b1, _ := json.Marshal(r1)
fmt.Printf("nil Value marshals to: %q\n", string(b1))
// Test 2: empty Value
r2 := &Result{Value: []byte{}}
b2, _ := json.Marshal(r2)
fmt.Printf("empty Value marshals to: %q\n", string(b2))
// Test 3: normal Value
r3 := &Result{Value: []byte(`{"key":"value"}`)}
b3, _ := json.Marshal(r3)
fmt.Printf("normal Value marshals to: %q\n", string(b3))
// Test 4: Check if result is valid JSON
var dest interface{}
err1 := json.Unmarshal(b1, &dest)
fmt.Printf("nil Value result valid JSON: %v\n", err1 == nil)
var dest2 interface{}
err2 := json.Unmarshal(b2, &dest2)
fmt.Printf("empty Value result valid JSON: %v\n", err2 == nil)
}
EOF
python3 << 'PYTHON'
import subprocess
result = subprocess.run(['go', 'run', '/tmp/test_marshal.go'], capture_output=True, text=True, cwd='/tmp')
print(result.stdout)
if result.stderr:
print("STDERR:", result.stderr)
PYTHONRepository: wundergraph/cosmo
Length of output: 235
Guard MarshalJSON against nil/empty payloads to return valid JSON.
When Result.Value is nil or empty, the current implementation returns an empty string, which is invalid JSON. Return the JSON null literal instead.
🛠️ Suggested fix
func (r *Result) MarshalJSON() ([]byte, error) {
+ if r == nil || len(r.Value) == 0 {
+ return []byte("null"), nil
+ }
return r.Value, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router/pkg/sandbox/runtime.go` around lines 45 - 47, The Result.MarshalJSON
method returns r.Value directly which yields invalid JSON when r or r.Value is
nil/empty; modify Result.MarshalJSON to check for nil receiver or
nil/zero-length Result.Value and return the JSON null literal (i.e.
[]byte("null"), nil) in those cases, otherwise return r.Value as before,
ensuring Result.MarshalJSON and references to Result.Value are updated
accordingly.
Member
Author
There was a problem hiding this comment.
Won't fix — MarshalJSON handles the relevant cases correctly for current usage.
Contributor
There was a problem hiding this comment.
Rate Limit Exceeded
@jensneuse have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 17 seconds before sending another message.
- CORS: preserve user-configured origins instead of overriding to wildcard - Bootstrap: add deferred cleanup to stop Code Mode server on later failures - Search: only include generateQueries preamble when yokoClient is configured - Mutations: isMutation now respects operationName for multi-operation documents - Shutdown: return errors from Stop() instead of swallowing them Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
router/pkg/mcpserver/code_mode_server.go (3)
229-240: Missing tool annotation for the execute tool.The
searchtool includes aWithToolAnnotationspecifying hints likeReadOnlyHint, but theexecutetool omits this annotation. Since execute can perform mutations, it should include an annotation with appropriate hints (e.g.,DestructiveHint: true,IdempotentHint: false).Proposed fix
// Execute tool s.mcpServer.AddTool( mcp.NewTool(executeToolName, mcp.WithDescription(executeToolDescription), mcp.WithString("code", mcp.Required(), mcp.Description("An async arrow function. Example: async () => { const { data } = await graphql({ query: '{ users { id } }' }); return data; }"), ), + mcp.WithToolAnnotation(mcp.ToolAnnotation{ + Title: "Execute GraphQL Operations", + DestructiveHint: mcp.ToBoolPtr(true), + IdempotentHint: mcp.ToBoolPtr(false), + OpenWorldHint: mcp.ToBoolPtr(true), + }), ), s.handleExecute(), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/code_mode_server.go` around lines 229 - 240, The execute tool registration (mcp.NewTool with executeToolName and s.handleExecute) is missing a tool annotation indicating its destructive nature; update the tool builder to include mcp.WithToolAnnotation(...) for the execute tool and set the appropriate hints (e.g., DestructiveHint: true, IdempotentHint: false) so callers understand it may perform mutations—add this annotation alongside mcp.WithDescription and mcp.WithString in the mcp.NewTool call that constructs the execute tool.
665-678: Consider LRU eviction instead of clearing the entire store.When the query store reaches
maxQueryStoreSize, the entire store is cleared. This means valid hashes that clients are actively using could suddenly become invalid. An LRU eviction strategy would be more graceful, removing only the least recently used entries.For initial implementation, the clear-all strategy is acceptable, but consider tracking this as a future improvement if hash resolution failures become a problem in production. The current approach is simple and predictable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/code_mode_server.go` around lines 665 - 678, storeQueryHash currently clears the entire s.queryStore when len(s.queryStore) >= maxQueryStoreSize which invalidates all active hashes; replace the clear-all behavior with an LRU eviction: add an LRU structure (e.g., container/list + map or use an lru package) tracked on the CodeModeServer, protect it with s.queryStoreMu, and in storeQueryHash insert the new hash and evict only the least-recently-used entry when capacity is exceeded (update access order on lookups elsewhere when resolving hashes). Ensure you update all places that read s.queryStore to touch the LRU so recency is tracked, keeping names like storeQueryHash, s.queryStore, s.queryStoreMu and maxQueryStoreSize to locate the code.
609-618: Consider usingsync.WaitGroup.Go()pattern per repo conventions.Per repo conventions for Go 1.25+, prefer using
wg.Go(func() { ... })over the manualwg.Add(1)+go func() { defer wg.Done(); ... }()pattern.Proposed refactor
- for i, p := range prompts { - wg.Add(1) - go func(idx int, prompt string) { - defer wg.Done() - results, err := s.yokoClient.Generate(ctx, prompt, schemaHash) - ch <- indexedResult{index: idx, results: results, err: err} - }(i, p) - } + for i, p := range prompts { + idx, prompt := i, p + wg.Go(func() { + results, err := s.yokoClient.Generate(ctx, prompt, schemaHash) + ch <- indexedResult{index: idx, results: results, err: err} + }) + }Based on learnings: "In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine, letting the WaitGroup manage Add/Done automatically."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/mcpserver/code_mode_server.go` around lines 609 - 618, Replace the manual wg.Add(1)+go+defer wg.Done() pattern in the loop with the Go 1.25+ sync.WaitGroup.Go usage: create local copies of loop variables (e.g., idx, prompt := i, p) to avoid closure capture, then call wg.Go(func() { results, err := s.yokoClient.Generate(ctx, prompt, schemaHash); ch <- indexedResult{index: idx, results: results, err: err} }); keep the separate goroutine that waits and closes ch (go func() { wg.Wait(); close(ch) }()). This updates the loop using the wg variable and preserves behavior of s.yokoClient.Generate and sending indexedResult to ch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router/pkg/mcpserver/code_mode_server.go`:
- Around line 229-240: The execute tool registration (mcp.NewTool with
executeToolName and s.handleExecute) is missing a tool annotation indicating its
destructive nature; update the tool builder to include
mcp.WithToolAnnotation(...) for the execute tool and set the appropriate hints
(e.g., DestructiveHint: true, IdempotentHint: false) so callers understand it
may perform mutations—add this annotation alongside mcp.WithDescription and
mcp.WithString in the mcp.NewTool call that constructs the execute tool.
- Around line 665-678: storeQueryHash currently clears the entire s.queryStore
when len(s.queryStore) >= maxQueryStoreSize which invalidates all active hashes;
replace the clear-all behavior with an LRU eviction: add an LRU structure (e.g.,
container/list + map or use an lru package) tracked on the CodeModeServer,
protect it with s.queryStoreMu, and in storeQueryHash insert the new hash and
evict only the least-recently-used entry when capacity is exceeded (update
access order on lookups elsewhere when resolving hashes). Ensure you update all
places that read s.queryStore to touch the LRU so recency is tracked, keeping
names like storeQueryHash, s.queryStore, s.queryStoreMu and maxQueryStoreSize to
locate the code.
- Around line 609-618: Replace the manual wg.Add(1)+go+defer wg.Done() pattern
in the loop with the Go 1.25+ sync.WaitGroup.Go usage: create local copies of
loop variables (e.g., idx, prompt := i, p) to avoid closure capture, then call
wg.Go(func() { results, err := s.yokoClient.Generate(ctx, prompt, schemaHash);
ch <- indexedResult{index: idx, results: results, err: err} }); keep the
separate goroutine that waits and closes ch (go func() { wg.Wait(); close(ch)
}()). This updates the loop using the wg variable and preserves behavior of
s.yokoClient.Generate and sending indexedResult to ch.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
router/core/router.gorouter/pkg/mcpserver/code_mode_server.gorouter/pkg/mcpserver/code_mode_server_test.gorouter/pkg/mcpserver/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- router/core/router.go
… cold start The WASM module cold start on GitHub Actions runners can exceed 5s. Move the 30s timeout into defaultConfig() so all sandbox tests (including TestPool_BasicExecution) handle CI cold starts consistently. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The goja runtime was removed — only QuickJS remains. The config schema no longer allows runtime_type, causing the demo to fail on startup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
graphql("{ users { id } }") now works as shorthand for
graphql({ query: "{ users { id } }" }). This is the most intuitive API
for agents and avoids a common first-call error observed in real usage.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ode sandbox" This reverts commit d481dc6.
…mprove descriptions Replace JS sandbox-based search with direct prompt array input. Search now takes natural language prompts and returns operations with ready-to-use JS snippets. Improve execute_graphql descriptions based on observed model behavior: front-load single-call constraint, add anti-pattern guidance, and instruct compact code generation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove dead code (httpServer field, GraphName config, SetHTTPClient), use idiomatic Go patterns (errors.New, %v for errors, http.MethodPost, Go 1.22+ loop var scoping), add maxPrompts=20 limit, and log query store eviction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
TestExecute_SimpleReturn failed in CI with 9.78s exceeding the 5s timeout during WASM module initialization. Increase all mcpserver sandbox test timeouts to 30s, matching the convention in runtime_test.go. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Keep dlclark/regexp2 (qjs sandbox dep), take main's docker/cli v29.2.0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace query store map with ristretto LRU cache for proper eviction - Add minimum:1 constraints to sandbox config limits in JSON schema - Add conditional auth validation (static requires token, cosmo requires credentials) - Mark execute tool as destructive with IdempotentHint: false - Fix hasRequiredArgs to respect GraphQL arg default values - Handle JSON decode errors in test mock servers - Log OTel metric initialization failures - Use WaitGroup.Go() instead of manual Add/Done pattern - Update RFC: remove stale goja references, fix implementation filenames - Improve cache miss error message to guide model to re-generate queries Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add semaphore (cap 64) to limit concurrent async goroutines in qjs sandbox, preventing unbounded fan-out from user code calling async functions in loops - Add maxItems: 20 constraint to mock OpenAPI queries array - Add language tags to RFC markdown code fences (http, text) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dpoint-rfc # Conflicts: # demo/pkg/subgraphs/employees/subgraph/schema.resolvers.go
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
search+execute) accepting TypeScript code, executed in a secure JS sandbox.graphqloperation filesKey components
router/pkg/sandbox/): Runtime interface, QuickJS/WASM and goja implementations, esbuild transpiler, semaphore-based poolrouter/pkg/mcpserver/code_mode_server.go):searchtool (generateQuery via Yoko) andexecutetool (graphql via router endpoint)router/pkg/yokoclient/): HTTP client for prompt-to-query generation REST API with static token and JWT authrouter/cmd/yoko-mock/), demo script, and configSee
rfc/mcp-agent-mode.mdfor the full design document.Test plan
router/pkg/sandbox/unit tests (runtime, transpiler, pool)router/pkg/yokoclient/unit tests (client, mock)router/pkg/mcpserver/unit tests (code mode server)make code-mode-demo) and verify end-to-end with an MCP client🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests