fix: deterministic MCP tool ordering for prompt cache stability

[ahoblitz]

Summary

MCP tools are assembled from Go maps with undefined iteration order, causing the tool list to shuffle across requests. This breaks prefix-based prompt caching (~11x cost multiplier).

Changes

• core/mcp/utils.go — Sort client names and tool names before iteration in GetToolPerClient
• core/mcp/toolmanager.go — Sort client names in GetAvailableTools before flattening
• core/schemas/orderedmap.go — Sort keys in OrderedMapFromMap so tool schema properties are deterministic

Type of change

• Bug fix

Affected areas

• Core (Go)

Breaking changes

• No

Related issues

Closes #2347

Checklist

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

[coderabbitai]

📝 Walkthrough

Walkthrough

Fixes non-deterministic MCP tool ordering by sorting client names and tool names before iteration in GetAvailableTools and GetToolPerClient, and by adding lexicographic key sorting in OrderedMapFromMap. A changelog entry records the fix.

Changes

Deterministic MCP Tool Ordering

Layer / File(s)	Summary
Sorted client and tool iteration in GetAvailableTools and GetToolPerClient core/mcp/toolmanager.go, core/mcp/utils.go	Adds slices import to toolmanager.go and replaces direct availableToolsPerClient map iteration with a sorted-slice loop. In utils.go, GetToolPerClient builds sorted client name and tool name slices before iterating, replacing unordered range-over-map loops at both the client and tool levels.
Lexicographic key sorting in OrderedMapFromMap core/schemas/orderedmap.go, core/changelog.md	Adds sort.Strings(om.keys) after key collection in OrderedMapFromMap so tool property keys are ordered deterministically; updates the function comment accordingly. Changelog documents the fix.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

• akshaydeo
• TejasGhatte
• danpiths

Poem

🐇 Hop, hop, the maps used to roam,
Keys shuffling wildly, far from home.
Now sorted in order, neat as a row,
The cache finds its prefix — watch those hits grow!
No more random chaos, just deterministic cheer,
Your prompt cache is stable — the rabbit is here! 🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title clearly and concisely summarizes the main change: fixing non-deterministic MCP tool ordering to improve prompt cache stability.
Linked Issues check	✅ Passed	The code changes directly address all requirements from issue #2347: sorting client/tool names in GetToolPerClient [utils.go], sorting clients in GetAvailableTools [toolmanager.go], and sorting keys in OrderedMapFromMap [orderedmap.go].
Out of Scope Changes check	✅ Passed	All changes are tightly scoped to implementing deterministic sorting in the MCP tool assembly pipeline; the changelog entry and test instructions are appropriate supporting changes with no extraneous modifications.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check	✅ Passed	The PR description includes all required sections per template: Summary, Changes with bullet points, Type of change (Bug fix), Affected areas (Core/Go), Breaking changes (No), Related issues (#2347), and complete checklist. The description clearly explains the problem, solution, and affected files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[CLAassistant]

All committers have signed the CLA.

[greptile-apps]

Confidence Score: 3/5

Safe to merge for the common case (stable MCP connections), but the property-ordering gap means cache misses can still occur after any client reconnection even when tools haven't changed.

The client and tool list sorting in utils.go and toolmanager.go correctly eliminates the request-to-request ordering variance. However, convertMCPToolToBifrostSchema still builds property OrderedMaps by iterating a plain Go map without sorting, so tool schema property keys remain non-deterministic across reconnection events. The orderedmap.go fix does not reach this code path. Additionally, the clientsByName construction silently drops one client's tools whenever two clients share the same name, which is a behavior change from the original loop.

core/mcp/utils.go — the convertMCPToolToBifrostSchema property/defs building loops (lines ~524 and ~548) and the new clientsByName construction block (lines 75–81).

Important Files Changed

Filename	Overview
core/mcp/utils.go	Adds deterministic client and tool ordering in GetToolPerClient; however, convertMCPToolToBifrostSchema still builds property OrderedMaps from Go map iteration without sorting, which the PR's orderedmap.go fix does not cover.
core/mcp/toolmanager.go	Correctly sorts client names before flattening availableToolsPerClient into the final tool list; straightforward and correct fix.
core/schemas/orderedmap.go	Adds sort.Strings to OrderedMapFromMap; used correctly in SortKeys/SortedCopy/SortedCopyPreservingProperties paths, but not invoked from convertMCPToolToBifrostSchema's property-building loop.
core/changelog.md	Adds changelog entry for the MCP tool ordering fix.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Req as Incoming Request
    participant TM as ToolsManager
    participant MM as MCPManager
    participant CM as clientMap (map[ID]→State)

    Req->>TM: GetAvailableTools(ctx)
    TM->>MM: GetToolPerClient(ctx)
    MM->>CM: iterate clientMap
    Note over MM: collect names → clientsByName<br/>slices.Sort(clientNames) ✅
    loop sorted clientNames
        MM->>MM: iterate sorted toolNames ✅
        MM-->>MM: tools[clientName] append
    end
    MM-->>TM: map[clientName][]ChatTool
    Note over TM: slices.Sort(sortedClients) ✅
    loop sorted clients
        TM-->>TM: flatten into availableTools
    end
    TM-->>Req: []ChatTool (deterministic order ✅)

    Note over MM: At tool-discovery time (reconnect)
    MM->>MM: convertMCPToolToBifrostSchema
    Note over MM: for k,v := range Properties (Go map)<br/>→ NewOrderedMapWithCapacity+Set ❌<br/>key order non-deterministic

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Req as Incoming Request
    participant TM as ToolsManager
    participant MM as MCPManager
    participant CM as clientMap (map[ID]→State)

    Req->>TM: GetAvailableTools(ctx)
    TM->>MM: GetToolPerClient(ctx)
    MM->>CM: iterate clientMap
    Note over MM: collect names → clientsByName<br/>slices.Sort(clientNames) ✅
    loop sorted clientNames
        MM->>MM: iterate sorted toolNames ✅
        MM-->>MM: tools[clientName] append
    end
    MM-->>TM: map[clientName][]ChatTool
    Note over TM: slices.Sort(sortedClients) ✅
    loop sorted clients
        TM-->>TM: flatten into availableTools
    end
    TM-->>Req: []ChatTool (deterministic order ✅)

    Note over MM: At tool-discovery time (reconnect)
    MM->>MM: convertMCPToolToBifrostSchema
    Note over MM: for k,v := range Properties (Go map)<br/>→ NewOrderedMapWithCapacity+Set ❌<br/>key order non-deterministic

Loading

Comments Outside Diff (1)

1. core/mcp/utils.go, line 524-529 (link)

   Property key ordering remains non-deterministic after reconnection

   convertMCPToolToBifrostSchema builds the orderedProps map by iterating mcpTool.InputSchema.Properties, which is a plain map[string]interface{} with undefined iteration order. The OrderedMapFromMap fix in this PR does add sorting, but this code path uses NewOrderedMapWithCapacity + Set() instead of OrderedMapFromMap, so it is not covered by the fix.

   During normal operation this is stable (tools are discovered once per connection lifetime), but after any reconnection event—SSE drop, STDIO restart, network failure—tools are re-discovered and the property keys can land in a different order. Because ToolFunctionParameters.Normalized() calls preserveKeysWithPropertyAwareness(), which explicitly preserves whatever insertion order exists in the *OrderedMap, that reconnection-time non-determinism propagates directly into the serialized tool schema sent to the LLM, causing prompt cache misses for unchanged tools.

   The same issue applies to the orderedDefs block at line ~548. Replacing both NewOrderedMapWithCapacity + iteration loops with schemas.OrderedMapFromMap(mcpTool.InputSchema.Properties) (and ...Defs) would make discovery-time ordering deterministic and consistent across reconnects.

_{Reviews (1): Last reviewed commit: "fix: deterministic MCP tool ordering for..." | Re-trigger Greptile}

[ahoblitz]

Good catch on convertMCPToolToBifrostSchema — fixed. Both the Properties and Defs loops now use OrderedMapFromMap (which sorts keys) instead of manual NewOrderedMapWithCapacity + range iteration. This covers the reconnection-time property ordering gap.

Re: duplicate client names — GetClientByName already returns only the first match, so names are assumed unique. The original code's append-under-same-key behavior for duplicate names was already surprising; this doesn't regress it in practice.