refactor: document strict vs. lenient content-item semantics in convertMapToCallToolResult#7571
Merged
Merged
Conversation
Adds a Note comment explaining why convertMapToCallToolResult uses strict error-on-invalid-type semantics (returning an error with index context) rather than calling mcpresult.NormalizeContentItems, which silently skips non-map items for lenient callers (rate-limit detection, path labelling). This prevents future contributors from inadvertently collapsing the two code paths and changing the strict-vs-lenient contract. The import-graph audit confirms consolidating mcpresult into mcp is NOT safe: mcp already imports difc, so moving mcpresult there would create the cycle mcp → difc → mcp. Closes #7543
Copilot
AI
changed the title
[WIP] Refactor semantic function clustering analysis code
refactor: document strict vs. lenient content-item semantics in convertMapToCallToolResult
Jun 15, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR clarifies (in-code) the intentional semantic difference between strict and lenient MCP content-item normalization in convertMapToCallToolResult, preventing an incorrect future refactor to mcpresult.NormalizeContentItems that would change error-handling behavior.
Changes:
- Added an explicit
// Note:comment documenting whyconvertMapToCallToolResultmust remain strict (error on non-map items) rather than using the lenientNormalizeContentItemshelper (skips non-map items).
Show a summary per file
| File | Description |
|---|---|
| internal/mcp/tool_result.go | Documents strict vs. lenient handling of malformed content items to preserve SDK-valid CallToolResult behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 0
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
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.
From a semantic function clustering analysis, two refactoring items were identified for MCP content normalization. This PR addresses them.
Changes
Documentation (
mcp/tool_result.go) — Added a// Note:comment block inconvertMapToCallToolResultexplaining why the inline[]interface{}switch must not be replaced withmcpresult.NormalizeContentItems:Structural refactor (not done — blocked) — Import graph audit confirmed
mcpalready importsdifc, anddifcis a caller ofmcpresult. Mergingmcpresultintomcpwould introduce the cyclemcp → difc → mcp. Consolidation is deferred until the dependency is broken.