feat(router): typed custom scalars in MCP tool schemas with scalar_mappings overrides - #3147
Draft
asoorm wants to merge 25 commits into
Draft
Conversation
…orting in schemaloader
…tool schemas Bumps graphql-go-tools to the eng-9903 branch pseudo-version; must be replaced with the tagged v2.15.0 release before this PR leaves draft.
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:
WalkthroughChangesThe PR adds JSON Schema generation for GraphQL variables, including custom scalar defaults and configurable mappings. MCP configuration, server wiring, validation, tests, fixtures, and documentation now support scalar-to-schema type mappings. MCP scalar schema mappings
Estimated code review effort: 5 (Critical) | ~90 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![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: 1
🧹 Nitpick comments (2)
router/pkg/mcpserver/scalar_mappings.go (1)
9-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the scalar mapping allowlist consistent across validation paths.
The same six values are maintained independently in the runtime mapper and configuration schema. A future edit can make YAML validation and environment validation disagree.
router/pkg/mcpserver/scalar_mappings.go#L9-L18: add a parity test or centralize the allowed values.router/pkg/config/config.schema.json#L2736-L2743: derive or update the enum from the same source.🤖 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/mcpserver/scalar_mappings.go` around lines 9 - 18, Keep the scalar mapping allowlist synchronized across both validation paths: in router/pkg/mcpserver/scalar_mappings.go lines 9-18, centralize or test the six allowed values; in router/pkg/config/config.schema.json lines 2736-2743, derive or update the enum from that same source. Ensure YAML and environment validation accept exactly the same values.router/pkg/config/config.go (1)
1352-1357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the environment map format explicit.
ScalarMappingsreads one environment variable, but the environment encoding is not stated in the field comment. Thecaarlos0/envv11 documentation defines comma-separated entries and colon-separated key/value pairs. (pkg.go.dev) Add explicit separator tags and a test or documentation example such asJSON:object,BigInt:integer. Verify the tag behavior against the pinnedenv/v11version.Proposed configuration contract
// number, boolean, object, array. +// Environment format: scalar:type,scalar:type. -ScalarMappings map[string]string `yaml:"scalar_mappings,omitempty" env:"MCP_SCALAR_MAPPINGS"` +ScalarMappings map[string]string `yaml:"scalar_mappings,omitempty" env:"MCP_SCALAR_MAPPINGS" envSeparator:"," envKeyValSeparator:":"`🤖 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.go` around lines 1352 - 1357, Make the ScalarMappings environment encoding explicit by adding the caarlos0/env v11 separator tags for comma-separated entries and colon-separated key/value pairs to ScalarMappings. Update its comment or add a focused test using a value such as JSON:object,BigInt:integer, and verify parsing against the pinned env/v11 behavior.
🤖 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 `@router/pkg/schemaloader/schema_builder.go`:
- Around line 62-63: Update the schema construction around
NewVariablesSchemaBuilder to preserve nullability when scalarSchemas maps a
nullable scalar to object, emitting both object and null types so MCP validation
accepts null. Add a regression test in the schema builder tests covering a
nullable object-mapped scalar and its generated schema.
---
Nitpick comments:
In `@router/pkg/config/config.go`:
- Around line 1352-1357: Make the ScalarMappings environment encoding explicit
by adding the caarlos0/env v11 separator tags for comma-separated entries and
colon-separated key/value pairs to ScalarMappings. Update its comment or add a
focused test using a value such as JSON:object,BigInt:integer, and verify
parsing against the pinned env/v11 behavior.
In `@router/pkg/mcpserver/scalar_mappings.go`:
- Around line 9-18: Keep the scalar mapping allowlist synchronized across both
validation paths: in router/pkg/mcpserver/scalar_mappings.go lines 9-18,
centralize or test the six allowed values; in
router/pkg/config/config.schema.json lines 2736-2743, derive or update the enum
from that same source. Ensure YAML and environment validation accept exactly the
same values.
🪄 Autofix
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: 67bd3f19-4d93-4d9b-bff1-2d0c1f93cf6e
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
router-tests/go.modrouter-tests/protocol/mcp_test.gorouter-tests/protocol/testdata/mcp_operations_custom_scalar/UploadFile.graphqlrouter/core/router.gorouter/go.modrouter/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.jsonrouter/pkg/mcpserver/operation_manager.gorouter/pkg/mcpserver/scalar_mappings.gorouter/pkg/mcpserver/scalar_mappings_test.gorouter/pkg/mcpserver/server.gorouter/pkg/schemaloader/schema_builder.gorouter/pkg/schemaloader/schema_builder_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs-website/router/mcp/configuration.mdx`:
- Line 42: Update the scalar_mappings documentation in
docs-website/router/mcp/configuration.mdx lines 42-42 and 71-71, and the tool
schema reference in docs-website/router/mcp/operations.mdx lines 105-106, to
state that unmapped custom scalars default to string for non-nullable fields and
["string", "null"] for nullable fields.
🪄 Autofix
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: d500dee7-c9fb-4935-acd8-3d1f67e69904
📒 Files selected for processing (4)
docs-website/router/mcp/configuration.mdxdocs-website/router/mcp/operations.mdxrouter/pkg/mcpserver/operation_manager.gorouter/pkg/mcpserver/scalar_mappings.go
🚧 Files skipped from review as they are similar to previous changes (2)
- router/pkg/mcpserver/scalar_mappings.go
- router/pkg/mcpserver/operation_manager.go
…nal package The MCP tool schema generator moves from graphql-go-tools engine/jsonschema into router/internal/jsonschema. The router no longer depends on graphql-go-tools for JSON schema generation. The library package is unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
router/internal/jsonschema/recursive_input_test.go (1)
84-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a negative assertion for the recursive
$ref.The doc comment at Lines 17-21 motivates the test with
additionalProperties: falserejecting valid nested payloads. The test only asserts that a valid depth-2 payload passes. A schema that dropped the recursive fields but also droppedadditionalProperties: falsewould still pass.Assert that an unknown property inside a nested node is rejected. The test then proves that the
$refresolves at depth and that the strict object constraint is still in place.🤖 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/jsonschema/recursive_input_test.go` around lines 84 - 88, Add a negative validation case alongside the existing compiled.Validate assertion in the recursive input test: construct a depth-2 payload containing an unknown property inside a nested node, then assert validation returns an error. Keep the valid nested payload assertion unchanged so the test verifies both recursive fields and additionalProperties: false.router/internal/jsonschema/variables_schema.go (2)
515-582: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the two value converters.
convertOperationValueToNativeandconvertDefinitionValueToNativeare identical except for the document they read from. Two copies of the sameast.Valueswitch will drift when a newast.ValueKindis handled.Take the document as a parameter and keep one implementation.
♻️ Proposed refactor
-// convertOperationValueToNative converts a GraphQL AST value from the operation document to a native Go value func (v *VariablesSchemaBuilder) convertOperationValueToNative(value ast.Value) any { - switch value.Kind { - case ast.ValueKindString: - return v.operationDocument.StringValueContentString(value.Ref) - ... - } - return nil + return convertValueToNative(v.operationDocument, value) } -// convertDefinitionValueToNative converts a GraphQL AST value from the definition document to a native Go value func (v *VariablesSchemaBuilder) convertDefinitionValueToNative(value ast.Value) any { - switch value.Kind { - ... - } - return nil + return convertValueToNative(v.definitionDocument, value) } + +// convertValueToNative converts a GraphQL AST value from the given document to a native Go value. +func convertValueToNative(doc *ast.Document, value ast.Value) any { + switch value.Kind { + case ast.ValueKindString: + return doc.StringValueContentString(value.Ref) + case ast.ValueKindInteger: + return doc.IntValueAsInt(value.Ref) + case ast.ValueKindFloat: + return doc.FloatValueAsFloat32(value.Ref) + case ast.ValueKindBoolean: + return doc.BooleanValue(value.Ref) + case ast.ValueKindNull: + return nil + case ast.ValueKindEnum: + return doc.EnumValueNameString(value.Ref) + case ast.ValueKindList: + list := make([]any, 0) + for _, itemRef := range doc.ListValues[value.Ref].Refs { + list = append(list, convertValueToNative(doc, doc.Value(itemRef))) + } + return list + case ast.ValueKindObject: + obj := make(map[string]any) + for _, fieldRef := range doc.ObjectValues[value.Ref].Refs { + obj[doc.ObjectFieldNameString(fieldRef)] = convertValueToNative(doc, doc.ObjectFieldValue(fieldRef)) + } + return obj + } + 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/internal/jsonschema/variables_schema.go` around lines 515 - 582, Deduplicate convertOperationValueToNative and convertDefinitionValueToNative by introducing one converter that accepts the relevant AST document as a parameter. Route both operationDocument and definitionDocument callers through this shared implementation, including recursive list and object conversions, and remove the duplicate switch logic.
337-362: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRecompute the recursive-type set once per definition document.
EnterDocumentcallscomputeRecursiveInputTypeson every build (Line 78), andschema_builder.gocreates oneVariablesSchemaBuilderper operation. The result depends only ondefinitionDocument, which is the same for every operation in a build. The graph scan and the per-nodereachableFromSelftraversal therefore repeat once per operation, at roughly O(N*E) each.This runs at startup and at reload, not per request, so it does not affect request latency. On a large federated schema with many input types and many operations it still adds avoidable startup time.
Consider computing the set once and passing it in through a
VariablesSchemaOption, so the per-operation builders reuse 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 `@router/internal/jsonschema/variables_schema.go` around lines 337 - 362, Compute the recursive input-type set once per definition document in the build flow, then pass it to each VariablesSchemaBuilder through a VariablesSchemaOption. Update EnterDocument and the schema_builder.go construction path to reuse this shared set, and make computeRecursiveInputTypes run only at the document-level rather than once per operation.router/internal/jsonschema/variables_schema_test.go (1)
1805-1865: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for a nullable object-mapped scalar.
Every object-mapped assertion here uses
JSON!, which is non-null. The nullable case is the one that exposes the forced non-nullability inEnterVariableDefinition(variables_schema.goLines 172-177): an optional variable of an object-mapped scalar emits"type": "object"and rejectsnull.Add
$meta: JSONto this subtest and assert the emitted type. The test then pins the current behavior and fails when the limitation is fixed.🤖 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/jsonschema/variables_schema_test.go` around lines 1805 - 1865, Extend the test case around the overridden JSON scalar to include an optional `$meta: JSON` variable and its corresponding operation usage. Update the expected schema to assert that the nullable object-mapped scalar currently emits the object type without nullability, covering the behavior in EnterVariableDefinition while preserving the existing required filter and cursor assertions.router/internal/jsonschema/schema.go (1)
271-307: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument or handle the
Defaultfield inClone.
Clonedeep-copies every reference-typed field exceptDefault.Defaultis typedanyand can hold amap[string]anyor[]any, becauseconvertOperationValueToNativeandconvertDefinitionValueToNativeproduce those values for object and list defaults. The clone then aliases that value.No current caller triggers this: the builder assigns a new value to
Defaultinstead of mutating it in place, and scalar overrides carry no default. The struct doc at Line 21 states the invariant for reference-typed fields, so record the intentional exception to keep the invariant reviewable.♻️ Proposed comment
if s.Maximum != nil { val := *s.Maximum clone.Maximum = &val } + // Default is shared, not deep-copied: callers replace it rather than mutate + // it in place, so a shared map/slice default cannot be observed as aliasing. return &clone }🤖 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/jsonschema/schema.go` around lines 271 - 307, Document the intentional shallow-copy exception for the any-typed Default field in JsonSchema.Clone, noting that it may contain reference values but current callers replace rather than mutate them. Keep the existing Clone behavior unchanged and place the explanation alongside the Default handling or relevant struct documentation so the invariant remains reviewable.router/internal/jsonschema/schema_test.go (1)
621-655: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtend the
Clonetest to coverDefs.The test asserts isolation for
Properties,Required,AdditionalProperties,Items,Enum, andMinimum. It does not coverDefsorMaximum.Defsis the field that carries recursive input type definitions, so a regression there would affect generated$defsoutput.♻️ Proposed test addition
additionalProps := false minimum := 1.0 + maximum := 10.0 original := &JsonSchema{ Type: TypeObject, Properties: map[string]*JsonSchema{"name": NewStringSchema()}, Required: []string{"name"}, AdditionalProperties: &additionalProps, Description: "original", Nullable: true, Items: NewStringSchema(), Enum: []string{"a", "b"}, Minimum: &minimum, + Maximum: &maximum, + Defs: map[string]*JsonSchema{"Node": NewStringSchema()}, } clone := original.Clone() @@ *clone.Minimum = 99 + *clone.Maximum = 99 + clone.Defs["Node"].Type = TypeBoolean @@ assert.Equal(t, 1.0, *original.Minimum) + assert.Equal(t, 10.0, *original.Maximum) + assert.Equal(t, TypeString, original.Defs["Node"].Type)🤖 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/jsonschema/schema_test.go` around lines 621 - 655, Extend the “mutating the clone does not affect the original” test around JsonSchema.Clone to initialize a Defs entry, mutate the corresponding cloned definition, and assert the original definition remains unchanged. Focus on Defs isolation; do not add coverage for Maximum unless separately requested.router/go.mod (1)
83-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
github.com/santhosh-tekuri/jsonschema/v6in both tests.
v6supports the required$refandnullvalidation. ReplaceCompileStringwithUnmarshalJSON,NewCompiler,AddResource, andCompile, then remove thev5requirement.🤖 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/go.mod` around lines 83 - 84, Update both test implementations to use github.com/santhosh-tekuri/jsonschema/v6: replace CompileString with the NewCompiler, AddResource, UnmarshalJSON, and Compile workflow, and remove the v5 dependency from go.mod while preserving the existing schema validation behavior.
🤖 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 `@router/internal/jsonschema/variables_schema.go`:
- Around line 170-177: Update the top-level nullability handling around
processInputObjectType so Nullable is forced false only for schemas originating
from GraphQL input objects, not every schema with TypeObject. Carry an
input-object provenance flag through the builder and use it at the Nullable
assignment, preserving GraphQL nullability for custom scalars mapped to object
while retaining empty-object defaults for actual input objects.
- Around line 306-315: Update the scalar override lookup in the
ast.NodeKindScalarTypeDefinition branch to treat a nil value from
v.scalarSchemas[typeName] as unmapped. Only clone and apply the override
description when the schema is non-nil; otherwise continue to the existing
string-default path.
---
Nitpick comments:
In `@router/go.mod`:
- Around line 83-84: Update both test implementations to use
github.com/santhosh-tekuri/jsonschema/v6: replace CompileString with the
NewCompiler, AddResource, UnmarshalJSON, and Compile workflow, and remove the v5
dependency from go.mod while preserving the existing schema validation behavior.
In `@router/internal/jsonschema/recursive_input_test.go`:
- Around line 84-88: Add a negative validation case alongside the existing
compiled.Validate assertion in the recursive input test: construct a depth-2
payload containing an unknown property inside a nested node, then assert
validation returns an error. Keep the valid nested payload assertion unchanged
so the test verifies both recursive fields and additionalProperties: false.
In `@router/internal/jsonschema/schema_test.go`:
- Around line 621-655: Extend the “mutating the clone does not affect the
original” test around JsonSchema.Clone to initialize a Defs entry, mutate the
corresponding cloned definition, and assert the original definition remains
unchanged. Focus on Defs isolation; do not add coverage for Maximum unless
separately requested.
In `@router/internal/jsonschema/schema.go`:
- Around line 271-307: Document the intentional shallow-copy exception for the
any-typed Default field in JsonSchema.Clone, noting that it may contain
reference values but current callers replace rather than mutate them. Keep the
existing Clone behavior unchanged and place the explanation alongside the
Default handling or relevant struct documentation so the invariant remains
reviewable.
In `@router/internal/jsonschema/variables_schema_test.go`:
- Around line 1805-1865: Extend the test case around the overridden JSON scalar
to include an optional `$meta: JSON` variable and its corresponding operation
usage. Update the expected schema to assert that the nullable object-mapped
scalar currently emits the object type without nullability, covering the
behavior in EnterVariableDefinition while preserving the existing required
filter and cursor assertions.
In `@router/internal/jsonschema/variables_schema.go`:
- Around line 515-582: Deduplicate convertOperationValueToNative and
convertDefinitionValueToNative by introducing one converter that accepts the
relevant AST document as a parameter. Route both operationDocument and
definitionDocument callers through this shared implementation, including
recursive list and object conversions, and remove the duplicate switch logic.
- Around line 337-362: Compute the recursive input-type set once per definition
document in the build flow, then pass it to each VariablesSchemaBuilder through
a VariablesSchemaOption. Update EnterDocument and the schema_builder.go
construction path to reuse this shared set, and make computeRecursiveInputTypes
run only at the document-level rather than once per operation.
🪄 Autofix
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: 2fb7e157-a529-48ef-ab03-faadb3c7fc3a
📒 Files selected for processing (13)
router/go.modrouter/internal/jsonschema/nullable_2020_12_test.gorouter/internal/jsonschema/recursive_input_test.gorouter/internal/jsonschema/schema.gorouter/internal/jsonschema/schema_test.gorouter/internal/jsonschema/variables_schema.gorouter/internal/jsonschema/variables_schema_test.gorouter/pkg/mcpserver/operation_manager.gorouter/pkg/mcpserver/scalar_mappings.gorouter/pkg/mcpserver/scalar_mappings_test.gorouter/pkg/mcpserver/server.gorouter/pkg/schemaloader/schema_builder.gorouter/pkg/schemaloader/schema_builder_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- router/pkg/mcpserver/scalar_mappings.go
- router/pkg/schemaloader/schema_builder.go
- router/pkg/mcpserver/scalar_mappings_test.go
- router/pkg/mcpserver/server.go
- router/pkg/mcpserver/operation_manager.go
- router/pkg/schemaloader/schema_builder_test.go
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3147 +/- ##
==========================================
+ Coverage 62.37% 62.59% +0.22%
==========================================
Files 262 264 +2
Lines 31003 31355 +352
==========================================
+ Hits 19337 19628 +291
- Misses 10158 10209 +51
- Partials 1508 1518 +10
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs-website/router/mcp/tools.mdx (1)
161-162: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winExclude defaulted non-null variables from
required. The generator adds every non-null variable torequiredbefore processing defaults. List only non-null variables without defaults asrequired.🤖 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 `@docs-website/router/mcp/tools.mdx` around lines 161 - 162, Update the generator logic described around the non-nullable variable and default-value handling so a variable is added to required only when it is non-nullable and has no default. Ensure defaulted non-null variables are represented solely through their default value and excluded from required.
🤖 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 `@docs-website/router/mcp/tools.mdx`:
- Line 157: Update the custom scalar variables documentation to distinguish
nullable and non-nullable unmapped schemas: document `"type": "string"` for
non-nullable variables and `"type": ["string", "null"]` for nullable variables.
Keep the existing SearchInput example unchanged.
---
Outside diff comments:
In `@docs-website/router/mcp/tools.mdx`:
- Around line 161-162: Update the generator logic described around the
non-nullable variable and default-value handling so a variable is added to
required only when it is non-nullable and has no default. Ensure defaulted
non-null variables are represented solely through their default value and
excluded from required.
🪄 Autofix
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: 1723ce5a-945b-401f-bfd1-73f6a774bf33
⛔ Files ignored due to path filters (1)
router/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
docs-website/router/mcp/configuration.mdxdocs-website/router/mcp/tools.mdxrouter-tests/protocol/mcp_test.gorouter/core/router.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.jsonrouter/pkg/mcpserver/server.go
🚧 Files skipped from review as they are similar to previous changes (9)
- router/pkg/config/config.schema.json
- router/pkg/config/testdata/config_defaults.json
- router/core/router.go
- router/pkg/config/testdata/config_full.json
- router/pkg/config/config.go
- docs-website/router/mcp/configuration.mdx
- router-tests/protocol/mcp_test.go
- router/pkg/mcpserver/server.go
- router/pkg/config/fixtures/full.yaml
…d openai Skipped unless SCHEMA_VENDOR_LIVE_TEST=1 and the vendor API key are set. Each probe uses max_tokens=1. The vendors validate tool schemas before inference, so a probe costs a fraction of a cent.
…e api All three Anthropic subtests verified live 2026-08-08: the generated schema is accepted, strict mode accepts type-array nullability, and a schema without a root type is rejected with 400.
Deletes the hand-written JsonSchema model (struct, custom MarshalJSON, deep Clone). Nullability is decided at node construction, so schema nodes are immutable after creation and sharing needs no copies. Output is unchanged: the existing golden tests pass without assertion edits.
asoorm
force-pushed
the
ahmet/eng-9903-mcp-custom-scalars-produce-untyped-json-schema-in-mcp-tool
branch
from
August 8, 2026 11:31
807957b to
88de1e4
Compare
… overrides Top-level non-null forcing for object-typed variables was applied to any schema whose emitted JSON type was "object", including a custom scalar mapped to object via WithScalarSchemas. That collapsed a nullable mapped scalar (e.g. $filter: JSON) to a bare non-nullable object type, rejecting an explicit null that GraphQL allows. Gate the forcing on provenance instead of shape: a topLevel flag threaded through typeRefSchema/namedTypeSchema (topLevelTypeRefSchema is the sole entry point with topLevel=true, only for a variable's own direct, non-list type) forces non-null only when the resolved node is a GraphQL NodeKindInputObjectTypeDefinition. Nullability is still decided before construction and no stored node is mutated. Also treat a nil WithScalarSchemas map value as unmapped instead of dereferencing it, falling back to the string default and counting it in DefaultedScalars. Adds three tests: a nullable object-mapped scalar keeps its null union, a non-null one emits a bare object type, and a nil override value falls back to string.
Document both nullability forms for MCP tool input schemas: a non-nullable variable emits the type alone, a nullable one adds "null" (e.g. ["string", "null"], ["object", "null"] for mappings). Rewrite the top-level-object note in configuration.mdx to match the provenance-gated forcing: only GraphQL input object variables are forced non-nullable at the top level; a scalar mapped to object keeps its own declared nullability.
… schema values to the sdk The generated schema value now flows from the schema builder to the MCP SDK and to input validation, with no re-parsing of the canonical bytes. santhosh-tekuri/jsonschema leaves the mcp server path; google/jsonschema-go resolves each schema once at registration and validates tool arguments. The validator swap changes the suffix wording of input validation errors behind the unchanged "Input validation error: " prefix.
The upstream google validator renders a JSON null instance as the Go artifact "<invalid reflect.Value>"; validateInput now substitutes the exact substring with "null" before wrapping, a no-op when absent. The tools/list schema assignment gains a guard comment: key order is not guaranteed and must not be restored via the removed bytes round trip.
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.
What this PR does
The Cosmo Router can expose GraphQL operations as MCP tools. Each tool publishes a tool input schema (JSON Schema). AI clients read this schema to build tool arguments. The router also validates incoming tool arguments against the same schema.
Without this PR, a custom scalar variable (for example
$after: Cursor) produces a schema property with notypefield. Strict MCP clients reject tools that have untyped properties. Examples of strict clients: Anthropic's marketplace submission checks, Claude Code (2.0.21 and later), OpenAI strict mode, and GitHub Copilot CLI. Operators have no workaround: if they change the variable toStringin the operation, the operation fails GraphQL validation and the router drops the tool.With this PR, every schema property carries a type. Custom scalars default to
string. A new config option, the scalar mapping, sets a different JSON type for scalars whose values are not strings (for example objects or numbers).Changes
"type": "string"in tool input schemas. Nullable variables have"type": ["string", "null"]. Built-in scalars do not change.mcp.scalar_mappingsmaps a scalar name to a JSON Schema type. Allowed values:string,integer,number,boolean,object,array. The environment-variable form isMCP_SCALAR_MAPPINGS=Foo:object,BigInt:integer. Mapping does not change nullability: a nullable variable of a scalar mapped toobjecthas"type": ["object", "null"]. Mappings are per scalar name: a scalar has one wire format across the graph, so there is nothing to configure per operation.mcp.scalar_mappings.router/internal/jsonschema. Before, the router imported it from the graphql-go-tools library asv2/pkg/engine/jsonschema. The router no longer uses graphql-go-tools for JSON Schema generation.router/go.modandrouter-tests/go.modstay on the released library version (v2.14.1). The library is unchanged.github.com/google/jsonschema-go. That is the schema model the MCP Go SDK uses. This PR deletes the hand-written schema struct and serializer. The generator builds each schema once and never modifies it afterwards. Because no schema changes after it is built, two variables can point at the same mapping entry and one can never corrupt the other's schema.google/jsonschema-goinstead ofsanthosh-tekuri/jsonschema. One consequence: the wording of validation error messages changes, because the validator changed. TheInput validation error:prefix stays. The built-inget_operation_infotool, which shows an operation's details as text, still prints byte-for-byte the same schema JSON as before.Why google/jsonschema-go
I picked
github.com/google/jsonschema-gofor four reasons:["string", "null"],$defsand$reffor recursive types,anyOf, and enums that include null. I checked each of these against the package source and with test runs, including that it marshals a false-schema as literaladditionalProperties: false.I also considered keeping our own schema model. I rejected that: it needed a custom serializer and a deep-copy method to stay correct, and the MCP server converted its output to bytes and back for the SDK anyway.
Behavior change (intentional; add to release notes)
All custom scalar properties in tool input schemas change from
{}to a concrete type. Input validation then rejects non-string values for unmapped custom scalars. The tool call still succeeds at the network level; the error text sits inside the tool response. A client recovers by fetching the tool list again and correcting its arguments. Onescalar_mappingsentry per scalar restores acceptance. The example maps two custom scalars:Foocarries objects,BigIntcarries integers:Two smaller behavior corrections ride along:
objectnow generates the correct schema:"type": ["object", "null"]. Before, the schema said only"type": "object", so the router rejected anullargument that GraphQL allows. GraphQLinputobject variables are not affected: they stay non-nullable on purpose, so a client always sends an object (possibly empty), nevernull.inputobject variables now get the same schema as non-recursive ones: non-nullable at the top level. The old check missed the recursive case.How to review 3,900 lines
Most of this diff is one package:
router/internal/jsonschema. Most of that package is tests.History: I first built this fix into graphql-go-tools - and decided to close it without merge (wundergraph/graphql-go-tools#1625). I thought that the router must own this code - for two reasons. It only exists for the MCP server, and the rules it implements are MCP-client rules, not engine concerns. And code that lives in the router ships immediately - a library change needs a graphql-go-tools release plus a dependency bump here. So I moved the package into the router, rewrote the generator to build
google/jsonschema-goschemas, and closed the library PR.Where to spend review time, in order:
router/internal/jsonschema/variables_schema.go- the generator. This file is a rewrite. The test files predate the rewrite and assert exact JSON output, and their assertions are unchanged. Unchanged assertions are strong evidence that the rewrite produces the same output.pkg/config,pkg/schemaloader,pkg/mcpserver, andcore/router.go.The test files come from the closed library PR (commit
979913e3on branchahmet/eng-9903-mcp-custom-scalars-produce-untyped-json-schema-in-mcp-tool). They differ in three ways:santhosh-tekuri/jsonschemav5 to v6.vendor_compat_test.gois new.Out of scope (tracked in ENG-9929)
required- the rules should move into one profile function per vendor, each rule with a test and a link to the vendor document that requires it. That restructuring changes no output today, so it waits until a second shape exists.Test plan
Custom scalar variables carry a JSON Schema type in tool input schemasandScalar mapping overrides the string default for a mapped custom scalar. Both pass. The first fails without the generator change.router/internal/jsonschema/vendor_compat_test.gosends generated schemas to the real Anthropic and OpenAI APIs withmax_tokens: 1. The tests start with at.Skipcall so no automated run touches the network; a comment explains how to run them locally. Every assertion matched the live API responses when we ran them on 2026-08-08; the test file's comments record this.grep -rn "engine/jsonschema" --include="*.go" router/ router-tests/returns no hits (the old library import path is gone).grep -rn santhosh router/pkg/mcpserver/returns no hits. Input validation runs ongoogle/jsonschema-go. The santhosh-tekuri validator remains only inside the generator tests, as a second, independent check that generated schemas are valid.grep -n graphql-go-tools router/go.mod router-tests/go.modshowsv2.14.1in both, with noreplacelines pointing at a local copy of the library.Fixes ENG-9903.