fix(mcp): preserve schema tags on nested fields - #920
Conversation
|
Connected to Huly®: MCP_G-484 |
|
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:
WalkthroughChangesNested schema tag handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
mcp/tools.go (2)
862-868: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog
schemaToRawerrors for consistency withschemaForerror handling.When
schemaFor[T]()fails, the error is printed to stderr. However, whenschemaToRaw(schema)fails (line 868), the error is silently swallowed at the unchanged lines 869–871. SinceschemaToRawis a new function that may have different failure modes than the previousjson.Marshal, adding error logging would improve debugability.♻️ Suggested fix: log schemaToRaw error before returning
mcpSchema, err := schemaToRaw(schema) if err != nil { + fmt.Fprintln(os.Stderr, err) return }🤖 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 `@mcp/tools.go` around lines 862 - 868, The error handling in the schema conversion path is inconsistent: `schemaFor[T]()` logs failures to stderr, but `schemaToRaw(schema)` errors are currently dropped. Update the `schemaToRaw` call site in the same helper to print the returned error to stderr before returning, matching the existing `schemaFor[T]()` handling and making failures from `schemaToRaw` visible for debugging.
913-919: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog
schemaToRawandjson.Unmarshalerrors for consistency.Same pattern as
WithInputSchema: theschemaForerror is logged to stderr, but both theschemaToRawerror (line 919, handled at unchanged lines 920–922) and thejson.Unmarshalerror (unchanged lines 924–925) are silently swallowed. Adding error logging would make schema conversion failures debuggable.♻️ Suggested fix: log errors before returning
mcpSchema, err := schemaToRaw(schema) if err != nil { + fmt.Fprintln(os.Stderr, err) return } if err := json.Unmarshal(mcpSchema, &t.OutputSchema); err != nil { + fmt.Fprintln(os.Stderr, err) return }🤖 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 `@mcp/tools.go` around lines 913 - 919, The error handling in the schema conversion flow is inconsistent: `schemaFor` logs to stderr, but `schemaToRaw` and the later `json.Unmarshal` failure paths in the same helper silently return. Update the schema-building logic around `schemaFor`, `schemaToRaw`, and `json.Unmarshal` to emit the error to stderr before returning, matching the existing `WithInputSchema` pattern so all schema conversion failures are debuggable.
🤖 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 `@mcp/schema_tags.go`:
- Around line 84-151: The recursion tracking in sanitizedSchemaTypeFor and
sanitizedStructSchemaType is using seen as both an in-progress guard and a
cache, which causes reused struct types to bypass sanitization on later
occurrences. Update the logic so seen only marks structs currently being
traversed, and cache or return the sanitized clone separately; make sure
repeated uses of the same struct still flow through sanitizedStructTag and
sanitizedStructSchemaType rather than returning the original legacy-tagged type.
---
Nitpick comments:
In `@mcp/tools.go`:
- Around line 862-868: The error handling in the schema conversion path is
inconsistent: `schemaFor[T]()` logs failures to stderr, but
`schemaToRaw(schema)` errors are currently dropped. Update the `schemaToRaw`
call site in the same helper to print the returned error to stderr before
returning, matching the existing `schemaFor[T]()` handling and making failures
from `schemaToRaw` visible for debugging.
- Around line 913-919: The error handling in the schema conversion flow is
inconsistent: `schemaFor` logs to stderr, but `schemaToRaw` and the later
`json.Unmarshal` failure paths in the same helper silently return. Update the
schema-building logic around `schemaFor`, `schemaToRaw`, and `json.Unmarshal` to
emit the error to stderr before returning, matching the existing
`WithInputSchema` pattern so all schema conversion failures are debuggable.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3467cc41-3c14-4005-8292-9973743546c3
📒 Files selected for processing (5)
mcp/schema_cache.gomcp/schema_cache_test.gomcp/schema_tags.gomcp/tools.gomcp/tools_test.go
|
@ychampion there is a merge conflict, can you have a quick look please? |
Constraint: Build on the struct-tag support merged in mark3labs#931 while retaining useful coverage from this PR Rejected: Replaying the original schema implementation | duplicates current main and leaves the conflict unresolved Confidence: high Scope-risk: narrow Directive: Keep nested struct tags working through pointers, slices, arrays, and string-keyed maps Tested: go test ./... -race -count=1; go test ./... -count=1 in otel; golangci-lint v2.8 in root and otel; go generate ./...; git diff --check
5aafbd6 to
7afa1c5
Compare
ychampion
changed the title
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 (2)
mcp/struct_schema.go (2)
21-39: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not swallow unsupported
jsonschemaoptions.
isJSONSchemaTagOptionErrorfalls back for everyWORD=option. Consequently,jsonschema:"description=foo"no longer returns the library error;parseJSONSchemaTagtreats it as plain description text and emits a schema. Only fall back when every option causing the error is supportedenum=...; otherwise return the original error. Add a regression test fordescription=....Also applies to: 232-251
🤖 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 `@mcp/struct_schema.go` around lines 21 - 39, Restrict the fallback in isJSONSchemaTagOptionError to errors caused exclusively by supported enum=... options; unsupported options such as description=foo must preserve and return the original jsonschema error. Update the schema-generation flow around jsonschema.For and schemaForStructFields accordingly, and add a regression test confirming description=... returns an error rather than producing a schema.
60-70: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRespect JSON names on anonymous fields.
Anonymous structs with an explicit JSON field name like
Embedded \json:"embedded"`should be treated as a named property, not flattened. Flatten only truly unnamed anonymous structs, and parse thejson` tag before schema/annotation walk paths to avoid writing schema annotations against the wrong property.🤖 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 `@mcp/struct_schema.go` around lines 60 - 70, Update the anonymous-field handling in walk to parse the JSON tag before deciding whether to flatten the field. Only recurse into truly unnamed anonymous structs; when an explicit JSON name is present, retain the field as a named property and ensure schema/annotation processing uses that property name.
🧹 Nitpick comments (1)
mcp/struct_schema_test.go (1)
11-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTable-drive the enum-whitespace variants.
The two tests differ only by the tag string. Combine them into cases so additional tag-parsing variants do not duplicate tool setup and assertions.
As per coding guidelines,
**/*_test.gorequires table-driven tests.🤖 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 `@mcp/struct_schema_test.go` around lines 11 - 49, Combine TestSchemaFor_JSONSchemaDescriptionTag and TestSchemaFor_JSONSchemaEnumTagWithoutLeadingSpace into one table-driven test covering both jsonschema tag variants. Move the differing tag strings into test cases, then reuse the shared tool creation, schema unmarshalling, and description/enum assertions for each case.Source: Coding guidelines
🤖 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 `@mcp/struct_schema.go`:
- Around line 111-130: Update schemaForFieldType and the schemaForStructFields
traversal to track struct types currently being expanded, including pointer
unwrapping, and detect re-entry for recursive fields such as Node.Next. On
re-entry, emit a schema reference/definition using the existing jsonschema
mechanisms or return a controlled error, instead of calling
schemaForStructFields indefinitely; preserve normal handling for non-recursive
pointers, arrays, slices, maps, and structs.
---
Outside diff comments:
In `@mcp/struct_schema.go`:
- Around line 21-39: Restrict the fallback in isJSONSchemaTagOptionError to
errors caused exclusively by supported enum=... options; unsupported options
such as description=foo must preserve and return the original jsonschema error.
Update the schema-generation flow around jsonschema.For and
schemaForStructFields accordingly, and add a regression test confirming
description=... returns an error rather than producing a schema.
- Around line 60-70: Update the anonymous-field handling in walk to parse the
JSON tag before deciding whether to flatten the field. Only recurse into truly
unnamed anonymous structs; when an explicit JSON name is present, retain the
field as a named property and ensure schema/annotation processing uses that
property name.
---
Nitpick comments:
In `@mcp/struct_schema_test.go`:
- Around line 11-49: Combine TestSchemaFor_JSONSchemaDescriptionTag and
TestSchemaFor_JSONSchemaEnumTagWithoutLeadingSpace into one table-driven test
covering both jsonschema tag variants. Move the differing tag strings into test
cases, then reuse the shared tool creation, schema unmarshalling, and
description/enum assertions for each case.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 51112e3d-50d9-4def-a09d-6e586e515f1e
📒 Files selected for processing (2)
mcp/struct_schema.gomcp/struct_schema_test.go
Constraint: Address all review findings without broadening support beyond documented enum tags Rejected: Silently skipping recursive fallback fields | hides schema loss and risks unbounded expansion Confidence: high Scope-risk: moderate Directive: Preserve library errors for unsupported schema options and keep named anonymous fields nested Tested: go test ./... -race -count=1; go test ./... -count=1 in otel; golangci-lint v2.8 in root and otel; go generate ./...; git diff --check
|
Addressed the review-body findings in a1e85c8:
Root/otel lint, generation, and |
Description
Builds on #931 by preserving
jsonschema_descriptionandjsonschema:"enum=..."tags when a tagged struct is nested inside another tool schema.Previously, the fallback schema builder skipped those nested fields. The recursive fallback now covers repeated structs, pointers, slices, arrays, and string-keyed maps.
Type of Change
Checklist
Validation
go test ./... -race -count=1(cd otel && go test ./... -count=1)golangci-lint runin the root andotelmodules using v2.8go generate ./...Summary by CodeRabbit
jsonschemastruct tag options can’t be applied, including better handling of nested structs, pointers, arrays/slices, and string-keyed maps.jsonschema_descriptionandjsonschemaenum annotations are applied recursively through nested and embedded schemas.