fix: deterministic MCP tool and property ordering for prompt caching - #2348
ivanetchart wants to merge 1 commit into
Conversation
MCP tools backed by Go maps have non-deterministic iteration order, causing the tool list and property keys to shuffle across requests. This breaks prefix-based prompt caching (Anthropic, OpenAI) since the tool definitions form the first level of the cache prefix. - Sort GetAvailableTools() result by function name - Sort MCP tool property keys before inserting into OrderedMap - Sort keys in OrderedMapFromMap for deterministic output
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes implement deterministic ordering of MCP tools and their properties to support prefix-based prompt caching. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Confidence Score: 5/5Safe to merge — the fix is correct, backward-compatible, and well-tested; all remaining findings are minor style nits. No P0 or P1 issues found. The core logic (sorting in No files require special attention. Important Files Changed
|
| sort.Slice(availableTools, func(i, j int) bool { | ||
| return chatToolName(availableTools[i]) < chatToolName(availableTools[j]) | ||
| }) |
There was a problem hiding this comment.
Tools with nil
Function sort to the front together
chatToolName returns "" when Function is nil, so any such tools will cluster at the front of the sorted slice in an undefined relative order. In practice the inner loop already guards against nil / empty-name tools before appending, so availableTools should never contain them by the time the sort runs. This is safe today, but a brief comment (or an assert in tests) noting that invariant would make the code easier to audit in future.
This is a documentation/hardening nit — no change to logic needed.
|
Would be nice to get this merged. We really need it :) |
|
This is already fixed so closing the PR |
convertMCPToolToBifrostSchema built the OrderedMap for inputSchema properties and $defs by ranging over the Go maps mcp-go decodes them into. Map iteration order is random, and the conversion runs again on every connection check tick, so the stored tool could change property order at every sync. The tool JSON sent to providers changed with it, which invalidated provider prompt caches. computeToolsHash changed too, so the callback set with SetToolsChangeCallback fired for tools that had not changed. Copy both maps with OrderedMapFromMap, which sorts keys. mcp-go does not record the server's key order, so sorted order is the stable order available here. maximhq#2348 proposed this change and was closed after maximhq#4588 merged, but maximhq#4588 sorted tool and client names and did not change this function. Related: maximhq#3362, maximhq#6591. Fixes maximhq#7169 Affected packages: - core/mcp/utils.go - core/mcp/toolschemaorder_test.go - core/changelog.md
Summary
MCP tools backed by Go maps have non-deterministic iteration order, causing the tool list and property keys to shuffle across requests. This breaks prefix-based prompt caching (Anthropic, OpenAI) since tool definitions form the first level of the cache prefix.
Changes
GetAvailableTools()result by function name (core/mcp/toolmanager.go)OrderedMapFromMapdefault sorting (core/mcp/utils.go)OrderedMapFromMapsort keys lexicographically by default, with an optionalsortFnoverride for custom ordering (core/schemas/orderedmap.go)Type of change
Affected areas
How to test
Key tests:
TestOrderedMapFromMap_DefaultSortsKeys— verifies default lexicographic sortingTestOrderedMapFromMap_CustomSortFn— verifies custom sort overrideTestOrderedMap*tests verify no regressions in insertion-order preservationScreenshots/Recordings
N/A
Breaking changes
OrderedMapFromMapnow sorts keys by default. Existing callers that relied on non-deterministic (random) ordering will now get sorted output, which is strictly more deterministic. The variadicsortFnparameter is backward-compatible — no call sites need changes.Related issues
Closes #2347
Related upstream context:
MergeExtraParamsproduces non-deterministic JSON key ordering, breaking Anthropic prompt caching #1742 —MergeExtraParamsproduces non-deterministic JSON key orderingSecurity considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelines