Draft
Conversation
Adds a second MCP server next to the existing per-operation one, exposing two generic tools instead of one-tool-per-operation: - code_mode_search_tools: takes prompts, generates GraphQL ops, registers them in the session catalog, returns their TS signatures. - code_mode_run_js: runs an async arrow against the session catalog inside a JS sandbox (V8 isolate), with tools.<name>(vars) bound to the registered ops. Includes: - router/internal/codemode: harness, sandbox, server, storage, tsgen, yoko (query-gen client), calltrace, observability. - proto/wg/cosmo/code_mode/yoko/v1: Yoko query-gen Connect API. - router/pkg/config: code_mode config block (auth, sandbox limits, query-generation endpoint, named-ops, mutation approval). - demo/code-mode: local federation + Yoko mock + start scripts + MCP client config snippets (Claude Code, Claude Desktop, Codex). - demo/code-mode-connect: alternate demo against an external yoko Connect supergraph (set YOKO_DIR). - router-tests: end-to-end named-ops integration test.
Contributor
WalkthroughThis PR introduces Code Mode, a comprehensive MCP server feature for the Cosmo router that enables LLM-powered GraphQL code generation and execution. It includes a QuickJS-based JavaScript sandbox for executing generated code, named operations storage with Redis support, TypeScript type generation, mutation approval workflows, observability instrumentation, and extensive demo applications with configuration. ChangesCode Mode MCP Implementation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes This PR implements a large, intricate code execution and code generation subsystem with high logic density across multiple tightly-coupled layers. The sandbox execution (QuickJS integration, promise handling, source map rewriting), dual storage backends (in-memory + Redis with transactions), TypeScript generation (abstract type lowering), and request-response pipelines require separate reasoning for each major component. The non-repetitive, heterogeneous nature of changes across protocols, runtimes, backends, and servers demands thorough understanding of interactions and correctness guarantees. Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
|
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2825 +/- ##
===========================================
+ Coverage 46.24% 50.28% +4.04%
===========================================
Files 1084 271 -813
Lines 145014 28879 -116135
Branches 9247 0 -9247
===========================================
- Hits 67055 14522 -52533
+ Misses 76179 12867 -63312
+ Partials 1780 1490 -290
🚀 New features to boost your workflow:
|
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router-tests/go.mod (1)
35-37:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRemove the
replacedirective downgradinggo.opentelemetry.io/otel/sdktov1.28.0, or upgrade tov1.40.0+This module requires
go.opentelemetry.io/otel/sdk v1.39.0but areplacedirective at line 213 downgrades it tov1.28.0. Version 1.28.0 is vulnerable to CVE-2026-24051, a PATH hijacking vulnerability in the resource detection code that allows arbitrary code execution on macOS/Darwin systems. The vulnerability is fixed in v1.40.0.Also applies to: lines 44-45, 213-214
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router-tests/go.mod` around lines 35 - 37, The go.mod contains a replace that forces go.opentelemetry.io/otel/sdk down to v1.28.0 which is vulnerable; remove that replace or change it to at least v1.40.0 so the required module go.opentelemetry.io/otel/sdk v1.39.0 (and other otel modules) are not downgraded — update or delete the replace directive(s) referencing go.opentelemetry.io/otel/sdk (and any matching replace entries on lines that reference the same module) to point to v1.40.0+ or remove them entirely.
🧹 Nitpick comments (19)
router/internal/codemode/sandbox/sandbox_preamble.js (1)
45-55: ⚡ Quick winUse explicit null/undefined checks in these helpers.
This block uses
== null/!= null, which goes against the repository's JS rules and makes the coercion behavior implicit in a security-sensitive preamble.As per coding guidelines, "Always use strict equality (
===and!==) instead of loose equality (==and!=)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/codemode/sandbox/sandbox_preamble.js` around lines 45 - 55, Replace loose-equality null checks in the sandbox helpers with explicit null/undefined checks: update globalThis.notNull to throw when v === null || v === undefined (instead of v == null), and in globalThis.compact use Array.isArray(v) branch to map and filter with x !== null && x !== undefined, use the object branch only when v !== null && v !== undefined && typeof v === "object", and test c !== null && c !== undefined before assigning to out; keep the same function names (notNull, compact) so the behavior is identical but uses strict null/undefined comparisons.router/pkg/config/config.schema.json (1)
2459-2499: ⚡ Quick winMirror the query-generation runtime requirements in the JSON schema.
This block currently accepts
query_generation.enabled: truewithout anendpoint, andauth.typecan be any string with no mode-specific required fields. That means schema-based validation and editor completion will accept configs that can only fail later at startup. Anif/thenonenabled, an enum forauth.type, and per-moderequiredfields would catch those earlier.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/pkg/config/config.schema.json` around lines 2459 - 2499, Update the query_generation schema so runtime constraints are enforced: add an if/then that when query_generation.enabled is true requires query_generation.endpoint; change auth.type to an enum (e.g. "static" and "oauth") instead of free string; and add conditional sub-schemas for auth using if auth.type == "static" then require "static_token", and if auth.type == "oauth" then require "token_endpoint", "client_id", and "client_secret" (keep additionalProperties: false). Reference the existing query_generation object and its properties enabled, endpoint, and auth (auth.type, static_token, token_endpoint, client_id, client_secret) when adding these if/then/required clauses.router/internal/codemode/sandbox/host.go (1)
187-199: 💤 Low valuePotential redundant message prefix.
When
reasonis empty, line 189 sets it to"Mutation declined by operator", then line 194 prepends"Mutation declined by operator: ", resulting in"Mutation declined by operator: Mutation declined by operator". Consider using the reason directly without the prefix, or adjusting the default to avoid duplication.Suggested fix
func mutationDeclinedResponse(reason string) json.RawMessage { if reason == "" { reason = "Mutation declined by operator" } body, _ := json.Marshal(map[string]any{ "data": nil, "errors": []map[string]string{{ - "message": "Mutation declined by operator: " + reason, + "message": reason, }}, "declined": map[string]string{"reason": reason}, }) return body }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/codemode/sandbox/host.go` around lines 187 - 199, The function mutationDeclinedResponse duplicates the phrase "Mutation declined by operator" when reason is empty because reason is set to that exact string and then the errors.message prepends the same prefix; update mutationDeclinedResponse to avoid duplication by either setting the default reason to an empty string and letting the errors.message build the full text, or by using the reason directly in errors.message without adding the hardcoded prefix when reason equals the default; modify the body construction in mutationDeclinedResponse so the "message" and the "declined.reason" are consistent and non-redundant.router/internal/codemode/storage/memory_backend_test.go (1)
218-230: 💤 Low valueConsider using
sync.WaitGroup.Go()for cleaner goroutine management.Based on learnings for this repository (Go 1.25+), prefer
wg.Go(func() {...})over the manualwg.Add(1)+go func() { defer wg.Done(); ... }()pattern.Suggested refactor
for i := range goroutines { - wg.Add(1) - go func(i int) { - defer wg.Done() + wg.Go(func() { _, err := backend.Append(ctx, "shared", []SessionOp{{ Name: "shared-op", Body: fmt.Sprintf("query Shared%d { shared%d }", i, i), Kind: OperationKindQuery, Description: fmt.Sprintf("Shared %d", i), }}) errs <- err - }(i) + }) }Note: This refactor removes the closure parameter
isincewg.Godoesn't support parameters. The loop variableiis captured correctly in Go 1.22+ due to the loop variable semantic change.Based on learnings: "In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/codemode/storage/memory_backend_test.go` around lines 218 - 230, Replace the manual goroutine management inside the loop (where goroutines is iterated and you call wg.Add(1); go func(i int){ defer wg.Done(); ... }(i)) with sync.WaitGroup.Go by calling wg.Go(func() { ... }) so you no longer call wg.Add or defer wg.Done; remove the closure parameter since Go 1.22+ loop variable capture is safe and let the body call backend.Append(ctx, "shared", []SessionOp{...}) and send its error to errs as before; update references to wg, backend.Append, errs and the loop variable usage accordingly.router/core/router_config.go (1)
117-117: ⚡ Quick winConsider adding Code Mode-specific keys in
Config.Usage().The new feature is wired into config state, but usage telemetry still only reports generic MCP toggles. Adding Code Mode keys would keep adoption visibility complete.
Possible follow-up in
Usage()usage["mcp"] = c.mcp.Enabled + usage["mcp_code_mode_enabled"] = c.mcp.CodeMode.Enabled + usage["mcp_code_mode_named_ops_enabled"] = c.mcp.CodeMode.NamedOps.Enabled + usage["mcp_code_mode_query_generation_enabled"] = c.mcp.CodeMode.QueryGeneration.Enabled usage["mcp_enable_arbitrary_operations"] = c.mcp.EnableArbitraryOperations usage["mcp_exclude_mutations"] = c.mcp.ExcludeMutations usage["mcp_expose_schema"] = c.mcp.ExposeSchema🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/core/router_config.go` at line 117, Config.Usage() currently emits only generic MCP toggle keys and misses Code Mode telemetry; update the Usage method on the Config type to add Code Mode-specific usage keys (e.g., "code_mode.enabled", "code_mode.server_present", or similar) derived from the fields controlling the feature (reference the Config struct and the codeModeServer field / any Code Mode-related flags) so adoption and enablement are reported; ensure keys follow the existing naming/format used by other MCP toggles and are conditionally emitted based on the same presence/enable checks used elsewhere in the code.router/internal/codemode/server/search_handler_test.go (1)
300-346: ⚡ Quick winUse
sync.WaitGroup.Go()instead of manualAdd(1)anddefer Done().This simplifies the code and removes boilerplate. Applies to lines 300-346, 358-377, and 410-435.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/codemode/server/search_handler_test.go` around lines 300 - 346, Replace manual wg.Add(1) / go func() { defer wg.Done() ... } blocks with the WaitGroup's Go method to reduce boilerplate: locate the goroutine launches around handleSearch/searchToolRequest in search_handler_test.go (the anonymous goroutines that call srv.handleSearch and populate results) and change each pattern from "wg.Add(1); go func(...) { defer wg.Done(); ... }(...)" to "wg.Go(func() { ... })" (remove the Add and defer Done). Apply the same replacement for the other occurrences noted (the blocks iterating prompts and other goroutines mentioned in the review).router/internal/codemode/server/search_handler.go (2)
224-248: ⚖️ Poor tradeoff
extractGraphQLVariablesBlockmay misparse parentheses inside strings.The parser counts
(and)without considering GraphQL string literals. A variable definition likequery Foo($name: String = "(test)")would incorrectly parse. This edge case is likely rare in generated operations, but worth noting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/codemode/server/search_handler.go` around lines 224 - 248, The function extractGraphQLVariablesBlock miscounts parentheses inside string literals; update its loop to track string state (single-quote, double-quote, and GraphQL block string triple-quotes) and escape sequences so '(' and ')' are ignored when inside any string literal. Specifically, in the for-loop in extractGraphQLVariablesBlock maintain flags like inSingle, inDouble, inBlockString and an escape flag to handle backslashes and detect triple-quote entry/exit, and only increment/decrement depth when none of those in-* flags are true; keep the same return behavior (trimmed substring pointer) and function signature.
21-24: 💤 Low valueHardcoded enum value may drift from proto definition.
yokoOperationKindSubscription = 3assumes the proto enum value. If the proto definition changes, this constant will silently become incorrect. Consider adding a comment referencing the proto definition or using a build-time check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/codemode/server/search_handler.go` around lines 21 - 24, The file defines yokoOperationKindSubscription as a hardcoded numeric constant (yokoOperationKindSubscription yokov1.OperationKind = 3) which can drift if the proto enum changes; replace the magic number by referencing the generated proto enum symbol (use the enum value from the yokov1 package instead of 3) or add a compile-time assertion/readme: add a comment pointing to the exact proto enum name and location and/or add a build-time check (e.g., a const compile-time comparison between yokoOperationKindSubscription and the generated enum value) so the code fails to compile if the proto value changes; update references to yokoOperationKindSubscription accordingly.demo/code-mode/yoko-mock/main_test.go (1)
153-153: 💤 Low valueOrphan interface compliance check serves no purpose.
This line verifies that
*http.ServeMuximplementshttp.Handler, which is always true and tested by the standard library. Consider removing it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@demo/code-mode/yoko-mock/main_test.go` at line 153, Remove the redundant compile-time interface assertion `var _ http.Handler = (*http.ServeMux)(nil)` from the test — it serves no purpose because `*http.ServeMux` already implements `http.Handler`; delete that line (the orphan assertion referencing http.ServeMux and http.Handler) to clean up the test.router/internal/codemode/server/server.go (1)
289-309: 💤 Low valueConsider documenting the cross-origin bypass intent.
The
AddInsecureBypassPattern("/{path...}")disables cross-origin protection for all paths under the MCP endpoint. While this appears intentional for MCP streaming compatibility, adding a brief comment explaining why this bypass is safe/necessary would help future maintainers understand the security decision.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/codemode/server/server.go` around lines 289 - 309, The cross-origin bypass call AddInsecureBypassPattern("/{path...}") in handler() (where cop is a CrossOriginProtection used in the mcp.NewStreamableHTTPHandler options) needs an inline comment documenting why bypassing CORS for MCP streaming is necessary and why it is safe; add a concise comment above the cop.AddInsecureBypassPattern call referencing CrossOriginProtection/AddInsecureBypassPattern and mcp streaming behavior, explaining the compatibility requirement (e.g., long-lived stream/handshake constraints), any mitigations (endpoint is internal/ authenticated via session ID), and that this is an intentional, reviewed decision for maintainers.demo/code-mode/yoko-mock/main.go (1)
295-303: 💤 Low value
filterNonNilmutates the input slice in place.The function reuses the input slice's backing array (
out := ops[:0]), which mutates the originalresultsslice. While this works correctly here sinceresultsisn't used after the call, this pattern can cause subtle bugs if the caller expects the original slice to remain unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@demo/code-mode/yoko-mock/main.go` around lines 295 - 303, The function filterNonNil currently reuses and mutates the input slice backing array (out := ops[:0]); change it to allocate a new slice to avoid mutating the caller's slice by creating a fresh slice (e.g., make([]*yokov1.GeneratedOperation, 0, len(ops))) and append non-nil ops into it, returning that new slice so callers' original results/ops remain unchanged; update references inside filterNonNil to use the new local slice variable instead of ops[:0].router/internal/codemode/storage/redis_backend_test.go (1)
182-196: 💤 Low valueConsider using
sync.WaitGroup.Gofor cleaner concurrency.The manual
wg.Add(1)followed bygo func() { defer wg.Done() }pattern can be simplified usingwg.Go(func())which manages Add/Done automatically.♻️ Simplified with WaitGroup.Go
var wg sync.WaitGroup errs := make(chan error, goroutines) for i := 0; i < goroutines; i++ { - wg.Add(1) - go func(worker int) { - defer wg.Done() + wg.Go(func() { + worker := i ops := make([]SessionOp, 0, opsPerGoroutine) for j := 0; j < opsPerGoroutine; j++ { ops = append(ops, SessionOp{Name: fmt.Sprintf("op_%02d_%02d", worker, j), Body: "query { ok }", Kind: OperationKindQuery}) } _, err := backend.Append(ctx, "session-1", ops) errs <- err - }(i) + }) }Based on learnings: In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/codemode/storage/redis_backend_test.go` around lines 182 - 196, Replace the manual Add/Done pattern with WaitGroup.Go to simplify the concurrent worker launch: use wg.Go to start each worker instead of calling wg.Add(1) then launching a goroutine that defers wg.Done(); keep the same body that builds ops (SessionOp creation loop) and calls backend.Append(ctx, "session-1", ops), and still send the result into errs channel; ensure wg variable remains the sync.WaitGroup used and that errs and opsPerGoroutine/goroutines are unchanged.router/internal/codemode/sandbox/execute.go (1)
192-202: 💤 Low valueRedundant condition in memory error detection.
Line 198:
strings.Contains(lower, "out of memory")is redundant since any string containing "out of memory" will also contain "memory", which is already checked by the first condition.♻️ Simplified condition
func classifyRuntimeError(err error, ctx context.Context) *ErrorEnvelope { if ctx.Err() != nil { return &ErrorEnvelope{Name: "Timeout", Message: ctx.Err().Error(), Stack: ""} } msg := err.Error() lower := strings.ToLower(msg) - if strings.Contains(lower, "memory") || strings.Contains(lower, "out of memory") { + if strings.Contains(lower, "memory") { return &ErrorEnvelope{Name: "MemoryLimit", Message: msg, Stack: ""} } return &ErrorEnvelope{Name: "Error", Message: msg, Stack: ""} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/codemode/sandbox/execute.go` around lines 192 - 202, In classifyRuntimeError, remove the redundant strings.Contains(lower, "out of memory") check and simplify the memory detection to a single condition (e.g., if strings.Contains(lower, "memory") { ... }) so the function only checks for "memory" once; update the if in classifyRuntimeError to use the single contains-check and keep the returned ErrorEnvelope.Name as "MemoryLimit".router/internal/codemode/server/execute_handler_test.go (1)
328-332: 💤 Low valueUnused method
lastSpanContext.This method is defined but never called in any test. Consider removing it or adding a test that validates span context propagation if that's the intent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/codemode/server/execute_handler_test.go` around lines 328 - 332, The method recordingPipeline.lastSpanContext is defined but unused; either remove this dead code or add a test that asserts span context propagation by invoking recordingPipeline.lastSpanContext after running a pipeline operation that creates a span. Locate the recordingPipeline type and the lastSpan field alongside the lastSpanContext() method, then either delete the lastSpanContext method and its mutex use, or add a new test that runs the pipeline (e.g., triggers Execute/Handle) and calls recordingPipeline.lastSpanContext() to verify the returned trace.SpanContext matches the expected span.router/internal/codemode/yoko/client_test.go (2)
321-336: 💤 Low valuePrefer
sync.WaitGroup.Gofor goroutine management.The manual
wg.Add(2)followed bygo func() { defer wg.Done(); ... }()pattern should be replaced with the idiomaticwg.Go(func())pattern available in Go 1.25+.♻️ Suggested refactor
var wg sync.WaitGroup - wg.Add(2) results := make([]*yokov1.SearchResponse, 2) errs := make([]error, 2) - go func() { - defer wg.Done() + wg.Go(func() { results[0], errs[0] = client.Search(context.Background(), "session-1", []string{"first"}) - }() + }) <-indexStarted - go func() { - defer wg.Done() + wg.Go(func() { results[1], errs[1] = client.Search(context.Background(), "session-2", []string{"second"}) - }() + })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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/codemode/yoko/client_test.go` around lines 321 - 336, Replace the manual wg.Add(2) + two go func() { defer wg.Done(); ... }() blocks with the Go 1.25+ idiom using wg.Go(func() { ... }) for each goroutine: keep the existing variables (wg, results, errs), preserve the call sites client.Search(context.Background(), "session-1", ... ) and client.Search(... "session-2", ... ), and retain the synchronization sequence around indexStarted, time.Sleep(25*time.Millisecond), close(releaseIndex) and wg.Wait(); this removes the explicit wg.Add and uses wg.Go to manage Add/Done automatically while preserving behavior.
364-379: 💤 Low valueSame WaitGroup pattern issue as above.
Apply the same
wg.Go(func())refactor here.♻️ Suggested refactor
var wg sync.WaitGroup - wg.Add(2) results := make([]*yokov1.SearchResponse, 2) errs := make([]error, 2) - go func() { - defer wg.Done() + wg.Go(func() { results[0], errs[0] = client.Search(context.Background(), "session-1", []string{"first"}) - }() + }) <-indexStarted - go func() { - defer wg.Done() + wg.Go(func() { results[1], errs[1] = client.Search(context.Background(), "session-2", []string{"second"}) - }() + })Based on learnings: "In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/codemode/yoko/client_test.go` around lines 364 - 379, The test uses manual goroutines with wg.Add and defer wg.Done; replace those with sync.WaitGroup.Go calls to avoid the pattern issue: remove explicit wg.Add(2) and the two go func() blocks and instead call wg.Go(func() { results[0], errs[0] = client.Search(context.Background(), "session-1", []string{"first"}) }) and similarly wg.Go for the "session-2" call (keeping the <-indexStarted, time.Sleep and close(releaseIndex) logic intact); ensure you reference the same results, errs slices and client.Search invocations and keep wg.Wait() at the end.router/internal/codemode/storage/redis_backend.go (2)
345-349: ⚡ Quick winNon-atomic SET + EXPIRE could leave keys without TTL.
If
Setsucceeds butExpirefails, the key will persist indefinitely. Consider usingSetwith the TTL option directly.♻️ Suggested fix
func (b *RedisBackend) setWithTTL(ctx context.Context, key string, value []byte) error { - if err := b.client.Set(ctx, key, value, 0).Err(); err != nil { - return err - } - return b.client.Expire(ctx, key, b.sessionTTL).Err() + return b.client.Set(ctx, key, value, b.sessionTTL).Err() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/codemode/storage/redis_backend.go` around lines 345 - 349, setWithTTL performs a non-atomic Set followed by Expire which can leave keys without TTL if Expire fails; change RedisBackend.setWithTTL to call the client's Set with the expiration argument directly (use b.client.Set(ctx, key, value, b.sessionTTL).Err()) and remove the separate b.client.Expire call so the value is written with TTL atomically (refer to RedisBackend.setWithTTL, b.client.Set, b.client.Expire, and b.sessionTTL).
91-147: ⚖️ Poor tradeoffUnbounded retry loop in Append may cause indefinite blocking.
The retry loop has no maximum retry count. While context cancellation provides an escape hatch, persistent Redis failures (e.g., connection issues, cluster failover) could cause requests to block indefinitely until the context times out. Consider adding a max retry limit.
♻️ Suggested approach
backoff := 5 * time.Millisecond + const maxRetries = 10 var appended []SessionOp - for { + for attempt := 0; attempt < maxRetries; attempt++ { if err := ctx.Err(); err != nil { return nil, err } // ... existing logic ... } + return nil, fmt.Errorf("code mode redis append: max retries (%d) exceeded", maxRetries)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/codemode/storage/redis_backend.go` around lines 91 - 147, The Append retry loop currently retries indefinitely; add a max retry limit (e.g., maxRetries constant) and an attempts counter inside the loop (or use a for attempts := 0; attempts < maxRetries; attempts++ pattern) so that after maxRetries failed Watch attempts you stop retrying, log and return a clear error (e.g., ErrMaxAppendRetries or a wrapped error) instead of looping forever; ensure you still check ctx.Err() each iteration, keep the existing backoff logic with sleepWithContext(ctx, backoff), and reference the same symbols (opsKey/bundleKey computation, b.readOps, tx.TxPipelined, backoff, sleepWithContext) when implementing the early exit and error return.router/internal/codemode/sandbox/sandbox_test.go (1)
580-592: 💤 Low valuePrefer
sync.WaitGroup.Gofor goroutine management.The loop with
wg.Add(1)followed bygo func()should use the idiomaticwg.Go(func())pattern.♻️ Suggested refactor
var wg sync.WaitGroup for range 5 { - wg.Add(1) - go func() { - defer wg.Done() + wg.Go(func() { _, err := s.Execute(context.Background(), ExecuteRequest{ SessionID: "s1", ToolNames: []string{"ping"}, WrappedJS: `async () => await tools.ping()`, }) assert.NoError(t, err) - }() + }) }Based on learnings: "In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/internal/codemode/sandbox/sandbox_test.go` around lines 580 - 592, Replace the manual wg.Add(1)/go func(){ defer wg.Done(); ... } pattern with the Go 1.25+ idiom wg.Go(func() { ... }) for goroutine management: remove the explicit wg.Add and defer wg.Done lines and call wg.Go(func() { _, err := s.Execute(context.Background(), ExecuteRequest{ SessionID: "s1", ToolNames: []string{"ping"}, WrappedJS: `async () => await tools.ping()`, }); assert.NoError(t, err) }) so the WaitGroup handles starting the goroutine; keep references to wg, s.Execute and ExecuteRequest unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@demo/code-mode/mcp-configs/codex.toml`:
- Line 2: Update the MCP URL in codex.toml to use an IPv4 loopback address:
change the url value (the "url" key in demo/code-mode/mcp-configs/codex.toml)
from "http://localhost:5027/mcp" to use "127.0.0.1" so the demo connects over
IPv4 rather than resolving to ::1.
In `@demo/code-mode/yoko-mock/main.go`:
- Around line 153-190: The Index method has a race where s.schemas[id] is
checked, unlocked, and later written, allowing duplicate work and temp-dir
leaks; fix by applying a double-check: after obtaining sessionID from
s.runCodexIndex and before assigning s.schemas[id], re-acquire s.mu, check if an
entry for id now exists (use existing := s.schemas[id] / schemaEntry), and if it
does, discard (remove) the newly created dir and use the existing
entry/sessionID for the response; otherwise insert the new schemaEntry;
alternatively replace with a singleflight.Group keyed by schema id to dedupe
concurrent runs while keeping parallelism for different schemas.
In `@router/core/graph_server.go`:
- Around line 1403-1410: The SDL print and codeModeServer.Reload failures are
currently only logged (via s.logger.Error) so the router starts with a broken
Code Mode; change this to hard-fail the mux build just like the MCP block: when
astprinter.PrintString(executor.ClientSchema) returns printErr or when
s.codeModeServer.Reload(executor.ClientSchema, sdl) returns mErr,
propagate/return the error (or call the same fatal/exit path used by the MCP
reload) instead of only logging it so startup aborts; update the block that
checks opts.IsBaseGraph() && s.codeModeServer != nil to return the error
(wrapping printErr/mErr with context mentioning Code Mode SDL/Reload and
executor.ClientSchema) so the build fails deterministically.
In `@router/internal/codemode/sandbox/errors.go`:
- Around line 144-160: The lookup function in type sourceMap seeds best with
line[0], causing columns before the first mapped segment to return a
false-positive mapping; change the logic in lookup (function name: lookup,
receiver: *sourceMap) to first check whether the earliest segment's
generatedColumn is greater than column0 and return (mapping{}, false) if so,
otherwise iterate and update best only when candidate.generatedColumn <= column0
(do not pre-seed best with line[0]); this ensures columns before the first
segment correctly return no mapping.
In `@router/internal/codemode/sandbox/headers.go`:
- Around line 31-43: The copyAllowedHeaders function currently only checks
static hopByHopHeaders but ignores dynamic hop-by-hop names declared in the
Connection header; update copyAllowedHeaders to parse src.Get("Connection") (or
src.Values("Connection")), split on commas, canonicalize each token (use
http.CanonicalHeaderKey then strings.ToLower) and treat those names as
additional hop-by-hop headers (merge into a temporary set or check before
copying). Keep existing checks for hopByHopHeaders and allow map; ensure any
header named in Connection is skipped just like entries in hopByHopHeaders
before dst.Add is called.
In `@router/internal/codemode/sandbox/sandbox_preamble.js`:
- Around line 62-66: The sandbox deletion of globalThis.eval and
globalThis.Function is insufficient because constructors like
AsyncFunction/GeneratorFunction remain reachable via constructors on real
functions; update the hardening in sandbox_preamble.js to also remove or
neutralize constructor access on function prototypes (e.g., remove/overwrite
Function.prototype.constructor and related prototype constructor properties)
and/or freeze Function.prototype so that expressions like (async
()=>{}).constructor, (function*(){}).constructor and (async
function*(){}).constructor cannot be used to obtain executable constructors;
target globalThis.eval, globalThis.Function, Function.prototype.constructor and
any prototype constructor lookups and ensure they are deleted or replaced with
non-callable values or made non-configurable to block constructor-based code
evaluation.
In `@router/internal/codemode/server/lifecycle.go`:
- Around line 122-140: When storage.NewRedisBackend(storage.RedisConfig{...})
fails after creating the Redis client via
factory(&rediscloser.RedisCloserOptions{...}), close the client to avoid leaking
connections: after err != nil from NewRedisBackend, call the client's
close/shutdown method (e.g., client.Close() or client.Shutdown() depending on
the client type) and handle/log any close error, then return the formatted
error; update the error path in the function that calls factory and
storage.NewRedisBackend to ensure client is always closed on backend creation
failure.
In `@router/internal/codemode/server/search_handler_test.go`:
- Around line 303-320: The test spawns goroutines that call srv.handleSearch via
searchToolRequest and currently uses require.* inside those worker goroutines;
replace in-goroutine assertions with collecting the returned error/results into
shared slices or an error channel (e.g., errs []error or errCh) alongside
results[], and let each goroutine set results[i] and errs[i] before returning;
keep require.Eventually(parent) checks like require.Eventually(...
searcher.callCount()) in the main goroutine, call wg.Wait(), then in the parent
test goroutine iterate over errs/results and perform require.NoError/assertions
there (referencing wg, results, errs/errCh, srv.handleSearch, searchToolRequest,
and searcher.callCount to locate spots).
In `@router/internal/codemode/storage/memory_backend.go`:
- Around line 159-191: In Bundle, when the session is missing you call
renderCapped before verifying b.renderer, which can panic; modify Bundle
(function Bundle on MemoryBackend) to check for b.renderer == nil and return the
configured error ("code mode storage renderer is not configured") before any
call to b.renderCapped (including the early path that handles a missing
session), ensuring all paths validate the renderer prior to invoking
renderCapped and leaving other logic (session locking, bundle caching on
memSession, setting lastUsed) unchanged.
In `@router/internal/codemode/storage/naming.go`:
- Around line 144-151: The lowerFirst function currently only lowercases the
first rune which produces mixed-case artifacts for inputs like "GETValue";
update lowerFirst to lowercase the entire initial token (the contiguous run of
identifier characters) rather than a single rune: in lowerFirst, iterate runes
from the start to find the token end (use unicode.IsLetter/unicode.IsDigit or
unicode.IsMark as appropriate), lowercase the substring comprising that token
(e.g., via strings.ToLower on that slice), then append the remainder and return;
keep the function name lowerFirst and preserve behavior for empty strings.
In `@router/internal/codemode/tsgen/graphql.go`:
- Around line 17-30: The renderOperation function currently only checks parse
errors; after parsing opDoc and before calling singleOperationRef you must
validate the parsed document against r.schema and return a clear error if
validation fails. Add a schema validation step (e.g., call the project’s GraphQL
document validator with r.schema and opDoc — place it after
astparser.ParseGraphqlDocumentString and before singleOperationRef in
renderOperation) and on validation errors return fmt.Errorf("render op %q:
validate GraphQL operation: %s", op.Name, <validation error details>); ensure
you import/use the repository's validator API and include the validation error
details in the log/returned error.
- Around line 150-169: The inputObjectType function currently always inlines
input objects and will infinitely recurse on recursive input types (e.g.,
InputObjectTypeDefinitions with self-references). Add a visited/alias strategy:
maintain a visited map from inputObjectRef (int) to a generated type alias name
(string) on the operationRenderer (or pass a context map into inputObjectType);
when first visiting an inputObjectRef generate and record a unique alias (e.g.,
UserFilterInput), emit/collect a top-level alias declaration instead of fully
inlining nested structure, and for subsequent visits return the alias name
(respecting writeNullable) instead of recursing; update any call sites that rely
on inputObjectType to ensure collected aliases are emitted (use symbols:
inputObjectType, writeInlineObject, writeNullable, tsProperty,
InputObjectTypeDefinitions, InputFieldsDefinition).
- Around line 61-87: variablesType currently treats any NonNull variable as
required; instead detect whether a variable has a default and treat defaulted
variables as optional. Inside variablesType (and where you construct tsProperty
for each varRef) call the document helper that exposes a variable default (e.g.
the same area that used VariableDefinitionNameString/VariableDefinitionType —
use VariableDefinitionDefaultValue or HasVariableDefinitionDefaultValue
equivalent) and then: 1) compute requiredOnly := (type is NonNull AND no default
present); 2) set tsProperty.optional = !requiredOnly; and 3) only flip
varsOptional to false when requiredOnly is true. Update the code paths in
variablesType and the tsProperty construction to use this default-aware logic.
In `@router/pkg/config/code_mode_validation.go`:
- Around line 3-22: The validator ValidateMCPCodeMode currently documents but
does not reject the unsupported combination of cfg.NamedOps.Enabled with
sessionStateless=true; change it to return a clear error when cfg.Enabled &&
cfg.NamedOps.Enabled && sessionStateless are true (e.g., return
fmt.Errorf("mcp.code_mode.named_ops requires stateful MCP sessions but sessions
are configured stateless")), and add the fmt import if missing so the config
load fails fast instead of deferring to runtime; reference ValidateMCPCodeMode,
cfg.NamedOps.Enabled and sessionStateless to locate the change.
---
Outside diff comments:
In `@router-tests/go.mod`:
- Around line 35-37: The go.mod contains a replace that forces
go.opentelemetry.io/otel/sdk down to v1.28.0 which is vulnerable; remove that
replace or change it to at least v1.40.0 so the required module
go.opentelemetry.io/otel/sdk v1.39.0 (and other otel modules) are not downgraded
— update or delete the replace directive(s) referencing
go.opentelemetry.io/otel/sdk (and any matching replace entries on lines that
reference the same module) to point to v1.40.0+ or remove them entirely.
---
Nitpick comments:
In `@demo/code-mode/yoko-mock/main_test.go`:
- Line 153: Remove the redundant compile-time interface assertion `var _
http.Handler = (*http.ServeMux)(nil)` from the test — it serves no purpose
because `*http.ServeMux` already implements `http.Handler`; delete that line
(the orphan assertion referencing http.ServeMux and http.Handler) to clean up
the test.
In `@demo/code-mode/yoko-mock/main.go`:
- Around line 295-303: The function filterNonNil currently reuses and mutates
the input slice backing array (out := ops[:0]); change it to allocate a new
slice to avoid mutating the caller's slice by creating a fresh slice (e.g.,
make([]*yokov1.GeneratedOperation, 0, len(ops))) and append non-nil ops into it,
returning that new slice so callers' original results/ops remain unchanged;
update references inside filterNonNil to use the new local slice variable
instead of ops[:0].
In `@router/core/router_config.go`:
- Line 117: Config.Usage() currently emits only generic MCP toggle keys and
misses Code Mode telemetry; update the Usage method on the Config type to add
Code Mode-specific usage keys (e.g., "code_mode.enabled",
"code_mode.server_present", or similar) derived from the fields controlling the
feature (reference the Config struct and the codeModeServer field / any Code
Mode-related flags) so adoption and enablement are reported; ensure keys follow
the existing naming/format used by other MCP toggles and are conditionally
emitted based on the same presence/enable checks used elsewhere in the code.
In `@router/internal/codemode/sandbox/execute.go`:
- Around line 192-202: In classifyRuntimeError, remove the redundant
strings.Contains(lower, "out of memory") check and simplify the memory detection
to a single condition (e.g., if strings.Contains(lower, "memory") { ... }) so
the function only checks for "memory" once; update the if in
classifyRuntimeError to use the single contains-check and keep the returned
ErrorEnvelope.Name as "MemoryLimit".
In `@router/internal/codemode/sandbox/host.go`:
- Around line 187-199: The function mutationDeclinedResponse duplicates the
phrase "Mutation declined by operator" when reason is empty because reason is
set to that exact string and then the errors.message prepends the same prefix;
update mutationDeclinedResponse to avoid duplication by either setting the
default reason to an empty string and letting the errors.message build the full
text, or by using the reason directly in errors.message without adding the
hardcoded prefix when reason equals the default; modify the body construction in
mutationDeclinedResponse so the "message" and the "declined.reason" are
consistent and non-redundant.
In `@router/internal/codemode/sandbox/sandbox_preamble.js`:
- Around line 45-55: Replace loose-equality null checks in the sandbox helpers
with explicit null/undefined checks: update globalThis.notNull to throw when v
=== null || v === undefined (instead of v == null), and in globalThis.compact
use Array.isArray(v) branch to map and filter with x !== null && x !==
undefined, use the object branch only when v !== null && v !== undefined &&
typeof v === "object", and test c !== null && c !== undefined before assigning
to out; keep the same function names (notNull, compact) so the behavior is
identical but uses strict null/undefined comparisons.
In `@router/internal/codemode/sandbox/sandbox_test.go`:
- Around line 580-592: Replace the manual wg.Add(1)/go func(){ defer wg.Done();
... } pattern with the Go 1.25+ idiom wg.Go(func() { ... }) for goroutine
management: remove the explicit wg.Add and defer wg.Done lines and call
wg.Go(func() { _, err := s.Execute(context.Background(), ExecuteRequest{
SessionID: "s1", ToolNames: []string{"ping"}, WrappedJS: `async () => await
tools.ping()`, }); assert.NoError(t, err) }) so the WaitGroup handles starting
the goroutine; keep references to wg, s.Execute and ExecuteRequest unchanged.
In `@router/internal/codemode/server/execute_handler_test.go`:
- Around line 328-332: The method recordingPipeline.lastSpanContext is defined
but unused; either remove this dead code or add a test that asserts span context
propagation by invoking recordingPipeline.lastSpanContext after running a
pipeline operation that creates a span. Locate the recordingPipeline type and
the lastSpan field alongside the lastSpanContext() method, then either delete
the lastSpanContext method and its mutex use, or add a new test that runs the
pipeline (e.g., triggers Execute/Handle) and calls
recordingPipeline.lastSpanContext() to verify the returned trace.SpanContext
matches the expected span.
In `@router/internal/codemode/server/search_handler_test.go`:
- Around line 300-346: Replace manual wg.Add(1) / go func() { defer wg.Done()
... } blocks with the WaitGroup's Go method to reduce boilerplate: locate the
goroutine launches around handleSearch/searchToolRequest in
search_handler_test.go (the anonymous goroutines that call srv.handleSearch and
populate results) and change each pattern from "wg.Add(1); go func(...) { defer
wg.Done(); ... }(...)" to "wg.Go(func() { ... })" (remove the Add and defer
Done). Apply the same replacement for the other occurrences noted (the blocks
iterating prompts and other goroutines mentioned in the review).
In `@router/internal/codemode/server/search_handler.go`:
- Around line 224-248: The function extractGraphQLVariablesBlock miscounts
parentheses inside string literals; update its loop to track string state
(single-quote, double-quote, and GraphQL block string triple-quotes) and escape
sequences so '(' and ')' are ignored when inside any string literal.
Specifically, in the for-loop in extractGraphQLVariablesBlock maintain flags
like inSingle, inDouble, inBlockString and an escape flag to handle backslashes
and detect triple-quote entry/exit, and only increment/decrement depth when none
of those in-* flags are true; keep the same return behavior (trimmed substring
pointer) and function signature.
- Around line 21-24: The file defines yokoOperationKindSubscription as a
hardcoded numeric constant (yokoOperationKindSubscription yokov1.OperationKind =
3) which can drift if the proto enum changes; replace the magic number by
referencing the generated proto enum symbol (use the enum value from the yokov1
package instead of 3) or add a compile-time assertion/readme: add a comment
pointing to the exact proto enum name and location and/or add a build-time check
(e.g., a const compile-time comparison between yokoOperationKindSubscription and
the generated enum value) so the code fails to compile if the proto value
changes; update references to yokoOperationKindSubscription accordingly.
In `@router/internal/codemode/server/server.go`:
- Around line 289-309: The cross-origin bypass call
AddInsecureBypassPattern("/{path...}") in handler() (where cop is a
CrossOriginProtection used in the mcp.NewStreamableHTTPHandler options) needs an
inline comment documenting why bypassing CORS for MCP streaming is necessary and
why it is safe; add a concise comment above the cop.AddInsecureBypassPattern
call referencing CrossOriginProtection/AddInsecureBypassPattern and mcp
streaming behavior, explaining the compatibility requirement (e.g., long-lived
stream/handshake constraints), any mitigations (endpoint is internal/
authenticated via session ID), and that this is an intentional, reviewed
decision for maintainers.
In `@router/internal/codemode/storage/memory_backend_test.go`:
- Around line 218-230: Replace the manual goroutine management inside the loop
(where goroutines is iterated and you call wg.Add(1); go func(i int){ defer
wg.Done(); ... }(i)) with sync.WaitGroup.Go by calling wg.Go(func() { ... }) so
you no longer call wg.Add or defer wg.Done; remove the closure parameter since
Go 1.22+ loop variable capture is safe and let the body call backend.Append(ctx,
"shared", []SessionOp{...}) and send its error to errs as before; update
references to wg, backend.Append, errs and the loop variable usage accordingly.
In `@router/internal/codemode/storage/redis_backend_test.go`:
- Around line 182-196: Replace the manual Add/Done pattern with WaitGroup.Go to
simplify the concurrent worker launch: use wg.Go to start each worker instead of
calling wg.Add(1) then launching a goroutine that defers wg.Done(); keep the
same body that builds ops (SessionOp creation loop) and calls
backend.Append(ctx, "session-1", ops), and still send the result into errs
channel; ensure wg variable remains the sync.WaitGroup used and that errs and
opsPerGoroutine/goroutines are unchanged.
In `@router/internal/codemode/storage/redis_backend.go`:
- Around line 345-349: setWithTTL performs a non-atomic Set followed by Expire
which can leave keys without TTL if Expire fails; change RedisBackend.setWithTTL
to call the client's Set with the expiration argument directly (use
b.client.Set(ctx, key, value, b.sessionTTL).Err()) and remove the separate
b.client.Expire call so the value is written with TTL atomically (refer to
RedisBackend.setWithTTL, b.client.Set, b.client.Expire, and b.sessionTTL).
- Around line 91-147: The Append retry loop currently retries indefinitely; add
a max retry limit (e.g., maxRetries constant) and an attempts counter inside the
loop (or use a for attempts := 0; attempts < maxRetries; attempts++ pattern) so
that after maxRetries failed Watch attempts you stop retrying, log and return a
clear error (e.g., ErrMaxAppendRetries or a wrapped error) instead of looping
forever; ensure you still check ctx.Err() each iteration, keep the existing
backoff logic with sleepWithContext(ctx, backoff), and reference the same
symbols (opsKey/bundleKey computation, b.readOps, tx.TxPipelined, backoff,
sleepWithContext) when implementing the early exit and error return.
In `@router/internal/codemode/yoko/client_test.go`:
- Around line 321-336: Replace the manual wg.Add(2) + two go func() { defer
wg.Done(); ... }() blocks with the Go 1.25+ idiom using wg.Go(func() { ... })
for each goroutine: keep the existing variables (wg, results, errs), preserve
the call sites client.Search(context.Background(), "session-1", ... ) and
client.Search(... "session-2", ... ), and retain the synchronization sequence
around indexStarted, time.Sleep(25*time.Millisecond), close(releaseIndex) and
wg.Wait(); this removes the explicit wg.Add and uses wg.Go to manage Add/Done
automatically while preserving behavior.
- Around line 364-379: The test uses manual goroutines with wg.Add and defer
wg.Done; replace those with sync.WaitGroup.Go calls to avoid the pattern issue:
remove explicit wg.Add(2) and the two go func() blocks and instead call
wg.Go(func() { results[0], errs[0] = client.Search(context.Background(),
"session-1", []string{"first"}) }) and similarly wg.Go for the "session-2" call
(keeping the <-indexStarted, time.Sleep and close(releaseIndex) logic intact);
ensure you reference the same results, errs slices and client.Search invocations
and keep wg.Wait() at the end.
In `@router/pkg/config/config.schema.json`:
- Around line 2459-2499: Update the query_generation schema so runtime
constraints are enforced: add an if/then that when query_generation.enabled is
true requires query_generation.endpoint; change auth.type to an enum (e.g.
"static" and "oauth") instead of free string; and add conditional sub-schemas
for auth using if auth.type == "static" then require "static_token", and if
auth.type == "oauth" then require "token_endpoint", "client_id", and
"client_secret" (keep additionalProperties: false). Reference the existing
query_generation object and its properties enabled, endpoint, and auth
(auth.type, static_token, token_endpoint, client_id, client_secret) when adding
these if/then/required clauses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75bd35af-298a-4502-af56-1e827ad4e903
⛔ Files ignored due to path filters (6)
demo/code-mode/mcp-stdio-proxy/go.sumis excluded by!**/*.sumdemo/code-mode/yoko-mock/go.sumis excluded by!**/*.sumrouter-tests/go.sumis excluded by!**/*.sumrouter/gen/proto/wg/cosmo/code_mode/yoko/v1/yoko.pb.gois excluded by!**/*.pb.go,!**/gen/**router/gen/proto/wg/cosmo/code_mode/yoko/v1/yokov1connect/yoko.connect.gois excluded by!**/gen/**router/go.sumis excluded by!**/*.sum
📒 Files selected for processing (101)
.gitignoreMakefiledemo/code-mode-connect/README.mddemo/code-mode-connect/router-config.yamldemo/code-mode-connect/start.shdemo/code-mode/.gitignoredemo/code-mode/Makefiledemo/code-mode/README.mddemo/code-mode/graph.yamldemo/code-mode/mcp-configs/README.mddemo/code-mode/mcp-configs/claude.desktop.jsondemo/code-mode/mcp-configs/claude.mcp.jsondemo/code-mode/mcp-configs/codex.tomldemo/code-mode/mcp-configs/sample-prompts/01-search-employees.txtdemo/code-mode/mcp-configs/sample-prompts/02-execute-fetch.txtdemo/code-mode/mcp-configs/sample-prompts/03-multi-tool.txtdemo/code-mode/mcp-configs/sample-prompts/04-mutation-not-approved.txtdemo/code-mode/mcp-stdio-proxy/go.moddemo/code-mode/mcp-stdio-proxy/main.godemo/code-mode/mcp-stdio-proxy/main_test.godemo/code-mode/router-config.yamldemo/code-mode/run_subgraphs_subset.shdemo/code-mode/start.shdemo/code-mode/yoko-mock/.gitignoredemo/code-mode/yoko-mock/README.mddemo/code-mode/yoko-mock/go.moddemo/code-mode/yoko-mock/main.godemo/code-mode/yoko-mock/main_test.godemo/code-mode/yoko-mock/schema.graphqldemo/pkg/subgraphs/employees/subgraph/schema.resolvers.goproto/wg/cosmo/code_mode/yoko/v1/yoko.protorouter-tests/code_mode_named_ops_test.gorouter-tests/go.modrouter-tests/testenv/testenv.gorouter/core/graph_server.gorouter/core/router.gorouter/core/router_config.gorouter/go.modrouter/internal/codemode/calltrace/calltrace.gorouter/internal/codemode/calltrace/calltrace_test.gorouter/internal/codemode/deps.gorouter/internal/codemode/harness/envelope.gorouter/internal/codemode/harness/envelope_test.gorouter/internal/codemode/harness/pipeline.gorouter/internal/codemode/harness/pipeline_test.gorouter/internal/codemode/harness/shape.gorouter/internal/codemode/harness/shape_test.gorouter/internal/codemode/harness/transpile.gorouter/internal/codemode/harness/transpile_test.gorouter/internal/codemode/observability/logging.gorouter/internal/codemode/observability/logging_test.gorouter/internal/codemode/observability/metrics.gorouter/internal/codemode/observability/metrics_test.gorouter/internal/codemode/observability/tracing.gorouter/internal/codemode/observability/tracing_test.gorouter/internal/codemode/sandbox/errors.gorouter/internal/codemode/sandbox/execute.gorouter/internal/codemode/sandbox/headers.gorouter/internal/codemode/sandbox/host.gorouter/internal/codemode/sandbox/preamble.gorouter/internal/codemode/sandbox/preamble_test.gorouter/internal/codemode/sandbox/sandbox.gorouter/internal/codemode/sandbox/sandbox_preamble.jsrouter/internal/codemode/sandbox/sandbox_test.gorouter/internal/codemode/sandbox/semaphore.gorouter/internal/codemode/sandbox/validation.gorouter/internal/codemode/server/approval.gorouter/internal/codemode/server/approval_test.gorouter/internal/codemode/server/execute_handler.gorouter/internal/codemode/server/execute_handler_test.gorouter/internal/codemode/server/lifecycle.gorouter/internal/codemode/server/lifecycle_test.gorouter/internal/codemode/server/observability_handler_test.gorouter/internal/codemode/server/search_handler.gorouter/internal/codemode/server/search_handler_test.gorouter/internal/codemode/server/server.gorouter/internal/codemode/server/server_test.gorouter/internal/codemode/server/session.gorouter/internal/codemode/storage/memory_backend.gorouter/internal/codemode/storage/memory_backend_test.gorouter/internal/codemode/storage/naming.gorouter/internal/codemode/storage/naming_test.gorouter/internal/codemode/storage/redis_backend.gorouter/internal/codemode/storage/redis_backend_test.gorouter/internal/codemode/storage/storage.gorouter/internal/codemode/storage/types.gorouter/internal/codemode/tsgen/bundle_test.gorouter/internal/codemode/tsgen/graphql.gorouter/internal/codemode/tsgen/tsgen.gorouter/internal/codemode/tsgen/tsgen_test.gorouter/internal/codemode/tsgen/typescript.gorouter/internal/codemode/yoko/client.gorouter/internal/codemode/yoko/client_test.gorouter/internal/codemode/yoko/searcher.gorouter/pkg/config/code_mode_config_test.gorouter/pkg/config/code_mode_validation.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/fixtures/full.yamlrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.json
| @@ -0,0 +1,2 @@ | |||
| [mcp_servers."yoko"] | |||
| url = "http://localhost:5027/mcp" | |||
Contributor
There was a problem hiding this comment.
Use 127.0.0.1 instead of localhost for the demo MCP URL.
Using localhost can resolve to ::1 first on some systems; with an IPv4-only listener this causes confusing connection failures.
Suggested patch
[mcp_servers."yoko"]
-url = "http://localhost:5027/mcp"
+url = "http://127.0.0.1:5027/mcp"📝 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
| url = "http://localhost:5027/mcp" | |
| [mcp_servers."yoko"] | |
| url = "http://127.0.0.1:5027/mcp" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@demo/code-mode/mcp-configs/codex.toml` at line 2, Update the MCP URL in
codex.toml to use an IPv4 loopback address: change the url value (the "url" key
in demo/code-mode/mcp-configs/codex.toml) from "http://localhost:5027/mcp" to
use "127.0.0.1" so the demo connects over IPv4 rather than resolving to ::1.
Comment on lines
+153
to
+190
| func (s *yokoService) Index(ctx context.Context, req *connect.Request[yokov1.IndexRequest]) (*connect.Response[yokov1.IndexResponse], error) { | ||
| schemaSDL := req.Msg.GetSchemaSdl() | ||
| id := schemaID(schemaSDL) | ||
|
|
||
| s.mu.Lock() | ||
| if existing, ok := s.schemas[id]; ok { | ||
| s.mu.Unlock() | ||
| existing.mu.RLock() | ||
| existingSession := existing.sessionID | ||
| existing.mu.RUnlock() | ||
| log.Printf("Index schema_id=%s reused dir=%s session_id=%s", id, existing.dir, existingSession) | ||
| return connect.NewResponse(&yokov1.IndexResponse{SchemaId: id}), nil | ||
| } | ||
| s.mu.Unlock() | ||
|
|
||
| dir, err := os.MkdirTemp("", "yoko-schema-"+id+"-") | ||
| if err != nil { | ||
| return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("create schema temp dir: %w", err)) | ||
| } | ||
| if err := os.WriteFile(filepath.Join(dir, "schema.graphql"), []byte(schemaSDL), 0o600); err != nil { | ||
| _ = os.RemoveAll(dir) | ||
| return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("write schema.graphql: %w", err)) | ||
| } | ||
|
|
||
| sessionID, err := s.runCodexIndex(ctx, dir) | ||
| if err != nil { | ||
| _ = os.RemoveAll(dir) | ||
| return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("codex pre-warm: %w", err)) | ||
| } | ||
|
|
||
| entry := &schemaEntry{dir: dir, sessionID: sessionID} | ||
| s.mu.Lock() | ||
| s.schemas[id] = entry | ||
| s.mu.Unlock() | ||
|
|
||
| log.Printf("Index schema_id=%s schema_sdl_size=%d schema_dir=%s session_id=%s rotate_after=%d", id, len(schemaSDL), dir, sessionID, s.rotateAfter) | ||
| return connect.NewResponse(&yokov1.IndexResponse{SchemaId: id}), nil | ||
| } |
Contributor
There was a problem hiding this comment.
Race condition in Index allows duplicate schema entries and temp dir leaks.
Between lines 157-166, the lock is released after checking s.schemas[id] but before re-acquiring it at line 184 to insert. Two concurrent Index calls with the same schema SDL could both pass the initial existence check, both create temp directories, and both run codex pre-warm. The second insert would overwrite the first entry, leaking its temp directory.
🔒 Proposed fix using double-check pattern
func (s *yokoService) Index(ctx context.Context, req *connect.Request[yokov1.IndexRequest]) (*connect.Response[yokov1.IndexResponse], error) {
schemaSDL := req.Msg.GetSchemaSdl()
id := schemaID(schemaSDL)
s.mu.Lock()
if existing, ok := s.schemas[id]; ok {
s.mu.Unlock()
existing.mu.RLock()
existingSession := existing.sessionID
existing.mu.RUnlock()
log.Printf("Index schema_id=%s reused dir=%s session_id=%s", id, existing.dir, existingSession)
return connect.NewResponse(&yokov1.IndexResponse{SchemaId: id}), nil
}
- s.mu.Unlock()
+ // Keep lock held to prevent concurrent Index for same schema
+ defer s.mu.Unlock()
dir, err := os.MkdirTemp("", "yoko-schema-"+id+"-")
if err != nil {
return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("create schema temp dir: %w", err))
}
if err := os.WriteFile(filepath.Join(dir, "schema.graphql"), []byte(schemaSDL), 0o600); err != nil {
_ = os.RemoveAll(dir)
return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("write schema.graphql: %w", err))
}
sessionID, err := s.runCodexIndex(ctx, dir)
if err != nil {
_ = os.RemoveAll(dir)
return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("codex pre-warm: %w", err))
}
entry := &schemaEntry{dir: dir, sessionID: sessionID}
- s.mu.Lock()
s.schemas[id] = entry
- s.mu.Unlock()
log.Printf("Index schema_id=%s schema_sdl_size=%d schema_dir=%s session_id=%s rotate_after=%d", id, len(schemaSDL), dir, sessionID, s.rotateAfter)
return connect.NewResponse(&yokov1.IndexResponse{SchemaId: id}), nil
}Note: Holding the lock during runCodexIndex will serialize concurrent Index calls for different schemas. If that's unacceptable, consider using singleflight.Group keyed by schema ID to deduplicate concurrent calls while allowing parallelism for different schemas.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@demo/code-mode/yoko-mock/main.go` around lines 153 - 190, The Index method
has a race where s.schemas[id] is checked, unlocked, and later written, allowing
duplicate work and temp-dir leaks; fix by applying a double-check: after
obtaining sessionID from s.runCodexIndex and before assigning s.schemas[id],
re-acquire s.mu, check if an entry for id now exists (use existing :=
s.schemas[id] / schemaEntry), and if it does, discard (remove) the newly created
dir and use the existing entry/sessionID for the response; otherwise insert the
new schemaEntry; alternatively replace with a singleflight.Group keyed by schema
id to dedupe concurrent runs while keeping parallelism for different schemas.
Comment on lines
+1403
to
+1410
| if opts.IsBaseGraph() && s.codeModeServer != nil { | ||
| sdl, printErr := astprinter.PrintString(executor.ClientSchema) | ||
| if printErr != nil { | ||
| s.logger.Error("failed to reload MCP server", zap.Error(fmt.Errorf("failed to print Code Mode schema SDL: %w", printErr))) | ||
| } else if mErr := s.codeModeServer.Reload(executor.ClientSchema, sdl); mErr != nil { | ||
| s.logger.Error("failed to reload MCP server", zap.Error(mErr)) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Make Code Mode schema reload fail the mux build.
If SDL printing or codeModeServer.Reload fails here, the router still comes up even though Code Mode will be serving against stale or missing schema. That turns a deterministic startup failure into a runtime feature break. This should mirror the hard-fail behavior of the MCP reload block just above.
Proposed fix
if opts.IsBaseGraph() && s.codeModeServer != nil {
sdl, printErr := astprinter.PrintString(executor.ClientSchema)
if printErr != nil {
- s.logger.Error("failed to reload MCP server", zap.Error(fmt.Errorf("failed to print Code Mode schema SDL: %w", printErr)))
- } else if mErr := s.codeModeServer.Reload(executor.ClientSchema, sdl); mErr != nil {
- s.logger.Error("failed to reload MCP server", zap.Error(mErr))
+ return nil, fmt.Errorf("failed to print Code Mode schema SDL: %w", printErr)
+ }
+ if mErr := s.codeModeServer.Reload(executor.ClientSchema, sdl); mErr != nil {
+ return nil, fmt.Errorf("failed to reload Code Mode server: %w", mErr)
}
}📝 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 opts.IsBaseGraph() && s.codeModeServer != nil { | |
| sdl, printErr := astprinter.PrintString(executor.ClientSchema) | |
| if printErr != nil { | |
| s.logger.Error("failed to reload MCP server", zap.Error(fmt.Errorf("failed to print Code Mode schema SDL: %w", printErr))) | |
| } else if mErr := s.codeModeServer.Reload(executor.ClientSchema, sdl); mErr != nil { | |
| s.logger.Error("failed to reload MCP server", zap.Error(mErr)) | |
| } | |
| } | |
| if opts.IsBaseGraph() && s.codeModeServer != nil { | |
| sdl, printErr := astprinter.PrintString(executor.ClientSchema) | |
| if printErr != nil { | |
| return nil, fmt.Errorf("failed to print Code Mode schema SDL: %w", printErr) | |
| } | |
| if mErr := s.codeModeServer.Reload(executor.ClientSchema, sdl); mErr != nil { | |
| return nil, fmt.Errorf("failed to reload Code Mode server: %w", mErr) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/core/graph_server.go` around lines 1403 - 1410, The SDL print and
codeModeServer.Reload failures are currently only logged (via s.logger.Error) so
the router starts with a broken Code Mode; change this to hard-fail the mux
build just like the MCP block: when
astprinter.PrintString(executor.ClientSchema) returns printErr or when
s.codeModeServer.Reload(executor.ClientSchema, sdl) returns mErr,
propagate/return the error (or call the same fatal/exit path used by the MCP
reload) instead of only logging it so startup aborts; update the block that
checks opts.IsBaseGraph() && s.codeModeServer != nil to return the error
(wrapping printErr/mErr with context mentioning Code Mode SDL/Reload and
executor.ClientSchema) so the build fails deterministically.
Comment on lines
+144
to
+160
| func (sm *sourceMap) lookup(generatedLine, generatedColumn int) (mapping, bool) { | ||
| if generatedLine < 1 || generatedLine > len(sm.lines) { | ||
| return mapping{}, false | ||
| } | ||
| line := sm.lines[generatedLine-1] | ||
| if len(line) == 0 { | ||
| return mapping{}, false | ||
| } | ||
| column0 := generatedColumn - 1 | ||
| best := line[0] | ||
| for _, candidate := range line { | ||
| if candidate.generatedColumn > column0 { | ||
| break | ||
| } | ||
| best = candidate | ||
| } | ||
| return best, true |
Contributor
There was a problem hiding this comment.
Avoid false-positive source map hits before first mapped column.
Line 153 seeds best with line[0], so a lookup for a column earlier than the first segment still returns a mapping. That can rewrite stack traces to the wrong source location.
Suggested fix
func (sm *sourceMap) lookup(generatedLine, generatedColumn int) (mapping, bool) {
if generatedLine < 1 || generatedLine > len(sm.lines) {
return mapping{}, false
}
line := sm.lines[generatedLine-1]
if len(line) == 0 {
return mapping{}, false
}
column0 := generatedColumn - 1
+ if column0 < line[0].generatedColumn {
+ return mapping{}, false
+ }
best := line[0]
for _, candidate := range line {
if candidate.generatedColumn > column0 {
break
}
best = candidate
}
return best, true
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (sm *sourceMap) lookup(generatedLine, generatedColumn int) (mapping, bool) { | |
| if generatedLine < 1 || generatedLine > len(sm.lines) { | |
| return mapping{}, false | |
| } | |
| line := sm.lines[generatedLine-1] | |
| if len(line) == 0 { | |
| return mapping{}, false | |
| } | |
| column0 := generatedColumn - 1 | |
| best := line[0] | |
| for _, candidate := range line { | |
| if candidate.generatedColumn > column0 { | |
| break | |
| } | |
| best = candidate | |
| } | |
| return best, true | |
| func (sm *sourceMap) lookup(generatedLine, generatedColumn int) (mapping, bool) { | |
| if generatedLine < 1 || generatedLine > len(sm.lines) { | |
| return mapping{}, false | |
| } | |
| line := sm.lines[generatedLine-1] | |
| if len(line) == 0 { | |
| return mapping{}, false | |
| } | |
| column0 := generatedColumn - 1 | |
| if column0 < line[0].generatedColumn { | |
| return mapping{}, false | |
| } | |
| best := line[0] | |
| for _, candidate := range line { | |
| if candidate.generatedColumn > column0 { | |
| break | |
| } | |
| best = candidate | |
| } | |
| return best, true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/internal/codemode/sandbox/errors.go` around lines 144 - 160, The
lookup function in type sourceMap seeds best with line[0], causing columns
before the first mapped segment to return a false-positive mapping; change the
logic in lookup (function name: lookup, receiver: *sourceMap) to first check
whether the earliest segment's generatedColumn is greater than column0 and
return (mapping{}, false) if so, otherwise iterate and update best only when
candidate.generatedColumn <= column0 (do not pre-seed best with line[0]); this
ensures columns before the first segment correctly return no mapping.
Comment on lines
+31
to
+43
| func copyAllowedHeaders(dst, src http.Header, allow map[string]struct{}) { | ||
| for name, values := range src { | ||
| canonical := strings.ToLower(http.CanonicalHeaderKey(name)) | ||
| if _, hop := hopByHopHeaders[canonical]; hop { | ||
| continue | ||
| } | ||
| if _, ok := allow[canonical]; !ok { | ||
| continue | ||
| } | ||
| for _, value := range values { | ||
| dst.Add(name, value) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Filter Connection-declared hop-by-hop headers too
Current logic blocks the standard hop-by-hop names, but not headers dynamically listed in Connection (e.g. Connection: X-Foo). Those should also be dropped before forwarding.
💡 Suggested patch
func copyAllowedHeaders(dst, src http.Header, allow map[string]struct{}) {
+ connectionScoped := map[string]struct{}{}
+ for _, value := range src.Values("Connection") {
+ for _, token := range strings.Split(value, ",") {
+ name := strings.ToLower(http.CanonicalHeaderKey(strings.TrimSpace(token)))
+ if name != "" {
+ connectionScoped[name] = struct{}{}
+ }
+ }
+ }
+
for name, values := range src {
canonical := strings.ToLower(http.CanonicalHeaderKey(name))
if _, hop := hopByHopHeaders[canonical]; hop {
continue
}
+ if _, hop := connectionScoped[canonical]; hop {
+ continue
+ }
if _, ok := allow[canonical]; !ok {
continue
}
for _, value := range values {
dst.Add(name, value)
}
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/internal/codemode/sandbox/headers.go` around lines 31 - 43, The
copyAllowedHeaders function currently only checks static hopByHopHeaders but
ignores dynamic hop-by-hop names declared in the Connection header; update
copyAllowedHeaders to parse src.Get("Connection") (or src.Values("Connection")),
split on commas, canonicalize each token (use http.CanonicalHeaderKey then
strings.ToLower) and treat those names as additional hop-by-hop headers (merge
into a temporary set or check before copying). Keep existing checks for
hopByHopHeaders and allow map; ensure any header named in Connection is skipped
just like entries in hopByHopHeaders before dst.Add is called.
Comment on lines
+144
to
+151
| func lowerFirst(value string) string { | ||
| if value == "" { | ||
| return value | ||
| } | ||
| runes := []rune(value) | ||
| runes[0] = unicode.ToLower(runes[0]) | ||
| return string(runes) | ||
| } |
Contributor
There was a problem hiding this comment.
Normalize the full first token, not only its first rune.
Current behavior can yield unexpected names (for example, all-caps input produces mixed-case artifacts like gET...). Lowercasing the full token first makes normalization more predictable.
Suggested patch
func lowerFirst(value string) string {
if value == "" {
return value
}
- runes := []rune(value)
+ runes := []rune(strings.ToLower(value))
runes[0] = unicode.ToLower(runes[0])
return string(runes)
}📝 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
| func lowerFirst(value string) string { | |
| if value == "" { | |
| return value | |
| } | |
| runes := []rune(value) | |
| runes[0] = unicode.ToLower(runes[0]) | |
| return string(runes) | |
| } | |
| func lowerFirst(value string) string { | |
| if value == "" { | |
| return value | |
| } | |
| runes := []rune(strings.ToLower(value)) | |
| runes[0] = unicode.ToLower(runes[0]) | |
| return string(runes) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/internal/codemode/storage/naming.go` around lines 144 - 151, The
lowerFirst function currently only lowercases the first rune which produces
mixed-case artifacts for inputs like "GETValue"; update lowerFirst to lowercase
the entire initial token (the contiguous run of identifier characters) rather
than a single rune: in lowerFirst, iterate runes from the start to find the
token end (use unicode.IsLetter/unicode.IsDigit or unicode.IsMark as
appropriate), lowercase the substring comprising that token (e.g., via
strings.ToLower on that slice), then append the remainder and return; keep the
function name lowerFirst and preserve behavior for empty strings.
Comment on lines
+17
to
+30
| func (r operationRenderer) renderOperation(op storage.SessionOp) (string, error) { | ||
| if r.schema == nil { | ||
| return "", fmt.Errorf("render op %q: schema is nil", op.Name) | ||
| } | ||
|
|
||
| opDoc, report := astparser.ParseGraphqlDocumentString(op.Body) | ||
| if report.HasErrors() { | ||
| return "", fmt.Errorf("render op %q: parse GraphQL operation: %s", op.Name, report.Error()) | ||
| } | ||
|
|
||
| opRef, err := singleOperationRef(&opDoc) | ||
| if err != nil { | ||
| return "", fmt.Errorf("render op %q: %w", op.Name, err) | ||
| } |
Contributor
There was a problem hiding this comment.
Validate the operation against the schema before generating a signature.
Right now this only rejects parse errors. A document like query Q { health { id } } or an impossible fragment such as viewer { ... on Cat { name } } will still produce a TypeScript contract even though the operation can never execute successfully. Failing fast here avoids registering unusable tools and mismatched signatures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/internal/codemode/tsgen/graphql.go` around lines 17 - 30, The
renderOperation function currently only checks parse errors; after parsing opDoc
and before calling singleOperationRef you must validate the parsed document
against r.schema and return a clear error if validation fails. Add a schema
validation step (e.g., call the project’s GraphQL document validator with
r.schema and opDoc — place it after astparser.ParseGraphqlDocumentString and
before singleOperationRef in renderOperation) and on validation errors return
fmt.Errorf("render op %q: validate GraphQL operation: %s", op.Name, <validation
error details>); ensure you import/use the repository's validator API and
include the validation error details in the log/returned error.
Comment on lines
+61
to
+87
| func (r operationRenderer) variablesType(opDoc *ast.Document, opRef int) (string, bool, error) { | ||
| op := opDoc.OperationDefinitions[opRef] | ||
| if !op.HasVariableDefinitions || len(op.VariableDefinitions.Refs) == 0 { | ||
| return "{}", true, nil | ||
| } | ||
|
|
||
| fields := make([]tsProperty, 0, len(op.VariableDefinitions.Refs)) | ||
| varsOptional := true | ||
| for _, varRef := range op.VariableDefinitions.Refs { | ||
| name := opDoc.VariableDefinitionNameString(varRef) | ||
| typeRef := opDoc.VariableDefinitionType(varRef) | ||
| required := opDoc.Types[typeRef].TypeKind == ast.TypeKindNonNull | ||
|
|
||
| typ, nullable, err := r.inputType(opDoc, typeRef) | ||
| if err != nil { | ||
| return "", false, err | ||
| } | ||
| if nullable { | ||
| typ = writeNullable(typ) | ||
| } else { | ||
| varsOptional = false | ||
| } | ||
|
|
||
| fields = append(fields, tsProperty{name: name, typ: typ, optional: !required}) | ||
| } | ||
|
|
||
| return writeInlineObject(fields), varsOptional, nil |
Contributor
There was a problem hiding this comment.
Default-valued variables should remain optional in the generated vars shape.
Requiredness is derived only from TypeKindNonNull, but GraphQL callers may omit variables that have defaults even when the declared type is non-null. For example, query Q($limit: Int! = 10) should not become vars: { limit: number }. Please factor the variable definition's default value into both the property optionality and varsOptional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/internal/codemode/tsgen/graphql.go` around lines 61 - 87,
variablesType currently treats any NonNull variable as required; instead detect
whether a variable has a default and treat defaulted variables as optional.
Inside variablesType (and where you construct tsProperty for each varRef) call
the document helper that exposes a variable default (e.g. the same area that
used VariableDefinitionNameString/VariableDefinitionType — use
VariableDefinitionDefaultValue or HasVariableDefinitionDefaultValue equivalent)
and then: 1) compute requiredOnly := (type is NonNull AND no default present);
2) set tsProperty.optional = !requiredOnly; and 3) only flip varsOptional to
false when requiredOnly is true. Update the code paths in variablesType and the
tsProperty construction to use this default-aware logic.
Comment on lines
+150
to
+169
| func (r operationRenderer) inputObjectType(inputObjectRef int) (string, error) { | ||
| def := r.schema.InputObjectTypeDefinitions[inputObjectRef] | ||
| fields := make([]tsProperty, 0, len(def.InputFieldsDefinition.Refs)) | ||
| for _, fieldRef := range def.InputFieldsDefinition.Refs { | ||
| name := r.schema.InputValueDefinitionNameString(fieldRef) | ||
| typeRef := r.schema.InputValueDefinitionType(fieldRef) | ||
| required := r.schema.Types[typeRef].TypeKind == ast.TypeKindNonNull | ||
|
|
||
| typ, nullable, err := r.inputType(r.schema, typeRef) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| if nullable { | ||
| typ = writeNullable(typ) | ||
| } | ||
|
|
||
| fields = append(fields, tsProperty{name: name, typ: typ, optional: !required}) | ||
| } | ||
|
|
||
| return writeInlineObject(fields), nil |
Contributor
There was a problem hiding this comment.
Recursive input objects will blow up this renderer.
This code always expands input objects inline, so schemas with common recursive filter shapes like input UserFilter { AND: [UserFilter!] } will recurse until stack overflow during bundle generation. Code Mode is supposed to work against arbitrary graphs, so this needs a visited-set/alias strategy before merge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/internal/codemode/tsgen/graphql.go` around lines 150 - 169, The
inputObjectType function currently always inlines input objects and will
infinitely recurse on recursive input types (e.g., InputObjectTypeDefinitions
with self-references). Add a visited/alias strategy: maintain a visited map from
inputObjectRef (int) to a generated type alias name (string) on the
operationRenderer (or pass a context map into inputObjectType); when first
visiting an inputObjectRef generate and record a unique alias (e.g.,
UserFilterInput), emit/collect a top-level alias declaration instead of fully
inlining nested structure, and for subsequent visits return the alias name
(respecting writeNullable) instead of recursing; update any call sites that rely
on inputObjectType to ensure collected aliases are emitted (use symbols:
inputObjectType, writeInlineObject, writeNullable, tsProperty,
InputObjectTypeDefinitions, InputFieldsDefinition).
Comment on lines
+3
to
+22
| func ValidateMCPCodeMode(cfg *MCPCodeModeConfiguration, sessionStateless bool) error { | ||
| if !cfg.Enabled { | ||
| return nil | ||
| } | ||
|
|
||
| if !cfg.NamedOps.Enabled { | ||
| return nil | ||
| } | ||
|
|
||
| // Storage backend selection: when ProviderID is set, the router resolves it | ||
| // against the central storage_providers registry (Redis backend). Otherwise | ||
| // the in-memory backend is used. The provider lookup error (unknown id) is | ||
| // emitted by the router at startup, not here. | ||
|
|
||
| // Named ops require stateful MCP sessions to work, but this intentionally | ||
| // does not fail boot. The Code Mode runtime emits the warn log on first | ||
| // reload so deployments can enable Code Mode before flipping session mode. | ||
| _ = sessionStateless | ||
|
|
||
| return nil |
Contributor
There was a problem hiding this comment.
Reject named_ops when MCP sessions are stateless.
This validator already documents that mcp.code_mode.named_ops cannot work with stateless sessions, but it still returns nil and defers the failure to runtime. That unsupported combination should be rejected during config load.
Proposed fix
package config
+import "errors"
+
func ValidateMCPCodeMode(cfg *MCPCodeModeConfiguration, sessionStateless bool) error {
if !cfg.Enabled {
return nil
}
if !cfg.NamedOps.Enabled {
return nil
}
@@
- // Named ops require stateful MCP sessions to work, but this intentionally
- // does not fail boot. The Code Mode runtime emits the warn log on first
- // reload so deployments can enable Code Mode before flipping session mode.
- _ = sessionStateless
+ if sessionStateless {
+ return errors.New("mcp.code_mode.named_ops requires mcp.session.stateless=false")
+ }
return nil
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/pkg/config/code_mode_validation.go` around lines 3 - 22, The validator
ValidateMCPCodeMode currently documents but does not reject the unsupported
combination of cfg.NamedOps.Enabled with sessionStateless=true; change it to
return a clear error when cfg.Enabled && cfg.NamedOps.Enabled &&
sessionStateless are true (e.g., return fmt.Errorf("mcp.code_mode.named_ops
requires stateful MCP sessions but sessions are configured stateless")), and add
the fmt import if missing so the config load fails fast instead of deferring to
runtime; reference ValidateMCPCodeMode, cfg.NamedOps.Enabled and
sessionStateless to locate the change.
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.
Adds a second MCP server next to the existing per-operation one, exposing two generic tools instead of one-tool-per-operation:
Includes:
Summary by CodeRabbit
Release Notes
New Features
Chores
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.