Make tool schema & mcp_servers serialization deterministic (HashMap -> IndexMap)#1931
Open
stephentoub wants to merge 3 commits into
Open
Make tool schema & mcp_servers serialization deterministic (HashMap -> IndexMap)#1931stephentoub wants to merge 3 commits into
stephentoub wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses nondeterministic JSON serialization in the Rust SDK that was caused by std::collections::HashMap iteration order, which in turn could invalidate provider-side prompt caching when tool schemas / MCP server maps are part of the model-visible prefix.
Changes:
- Replaces
HashMapwithindexmap::IndexMapfor model-visible maps (Tool.parameters, andmcp_serversacross session/agent config and wire serialization), and re-exportsIndexMapfrom the crate root. - Updates E2E tests and docs to use the new
IndexMap-based API surface. - Adds targeted regression tests to ensure deterministic tool-schema serialization and MCP server insertion-order serialization.
Show a summary per file
| File | Description |
|---|---|
| rust/tests/e2e/rpc_mcp_lifecycle.rs | Updates test helper map type to IndexMap for MCP servers. |
| rust/tests/e2e/rpc_mcp_and_skills.rs | Updates MCP servers test helper map type to IndexMap. |
| rust/tests/e2e/pre_mcp_tool_call_hook.rs | Switches MCP servers map to IndexMap for hook-related E2E coverage. |
| rust/tests/e2e/mcp_oauth.rs | Updates MCP server config construction to IndexMap in OAuth lifecycle tests. |
| rust/src/wire.rs | Changes SessionCreateWire / SessionResumeWire mcp_servers to Option<IndexMap<...>> to keep serialization deterministic. |
| rust/src/types.rs | Updates public types/builders (Tool.parameters, SessionConfig / ResumeSessionConfig / CustomAgentConfig MCP server maps) and adds an insertion-order regression test. |
| rust/src/tool.rs | Updates tool_parameters / try_tool_parameters return types to IndexMap and adds deterministic serialization regression test. |
| rust/src/lib.rs | Re-exports IndexMap at github_copilot_sdk::IndexMap for consumer ergonomics. |
| rust/README.md | Documents the IndexMap choice and rationale (deterministic prompt-prefix bytes). |
| rust/Cargo.toml | Adds explicit indexmap dependency with serde feature. |
| rust/Cargo.lock | Records the new direct dependency inclusion. |
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 1
- Review effort level: Low
…ashMap -> IndexMap) Tool.parameters and the mcp_servers config maps used std::collections::HashMap, whose per-process random iteration seed made serde_json emit each tool schema's top-level JSON keys in a different order every session, busting the model provider's prompt cache on the system+tools prefix. Switch exactly the model-visible maps to indexmap::IndexMap for deterministic serialization: - Tool.parameters (+ skip_serializing_if = IndexMap::is_empty) - mcp_servers on SessionConfig/ResumeSessionConfig/CustomAgentConfig and the wire.rs SessionCreateWire/SessionResumeWire structs, plus the three with_mcp_servers setters - tool_parameters/try_tool_parameters return types Add indexmap with the serde feature (already a transitive dep) and re-export it as github_copilot_sdk::IndexMap. Non-model-visible maps (MCP env/headers, system-message sections, telemetry, internal handler maps) stay as HashMap. Adds regression tests for byte-stable Tool serialization with pinned key order and mcp_servers insertion order through the wire struct. BREAKING CHANGE: public field/param/return types change from HashMap to IndexMap. Migration is mechanical - IndexMap mirrors HashMap's API and is re-exported as github_copilot_sdk::IndexMap. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
435a78f to
35d8a75
Compare
Expand the regression test from 4 to 16 MCP server keys so a HashMap regression cannot coincidentally serialize in insertion order (1/N! instead of 1/24), removing the spurious-pass risk flagged in review. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Move the IndexMap re-export to the import position required by nightly rustfmt. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Collaborator
Author
|
@tclem , any concerns? It is a breaking change, but in the past you've suggested that's acceptable. |
Collaborator
Author
|
@SteveSandersonMS any concerns? |
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.
Problem / root cause
Tool.parameters(and themcp_serversconfig maps) usedstd::collections::HashMap. Rust'sHashMapseeds its iteration order with a per-process random seed (SipHashRandomState), andserde_jsonserializes maps in iteration order — so each tool schema's top-level JSON keys came out byte-different every session (e.g.…"required":["url"],"type":"object"}one session vs…"type":"object",…,"required":["url"]}the next). That busts the model provider's prompt cache on the system+tools prefix, driving up cost and latency. Nested keys were already stable (they'reserde_json::Value, whose default map isBTreeMap-sorted), matching the observed fingerprint: only top-level schema keys flipped. Rust was the only one of the six SDKs with this bug (Go/Java/.NET/Python/Node all use insertion- or sort-ordered maps).Fix
Swap
HashMap→indexmap::IndexMapfor exactly the model-visible maps:Tool.parameters(+skip_serializing_if = "IndexMap::is_empty")mcp_serversonSessionConfig/ResumeSessionConfig/CustomAgentConfig, thewire.rsSessionCreateWire/SessionResumeWireserialization structs, and the threewith_mcp_serverssetterstool_parameters/try_tool_parametersreturn typesindexmapis added with theserdefeature (already a 2.14 transitive dep) and re-exported asgithub_copilot_sdk::IndexMap. Non-model-visible maps (MCPenv/headers, system-messagesections, telemetry, internal handler maps) are intentionally left asHashMap— they never reach the prompt prefix.Result
Deterministic serialization. Top-level tool-schema keys are now stable (alphabetically sorted, since parameters round-trip through a
BTreeMap-backedserde_json::Value);mcp_serversserialize in insertion order. We deliberately did not enableserde_json'spreserve_orderfeature — it would add nested-key declared-order fidelity too, but at the cost of a global blast radius on everyserde_json::Valuemap. The top-level container was the cache-buster, and it's fixed.Tests
Two regression tests:
tool_parameters_serialize_in_deterministic_order— builds aTool64× and asserts byte-identical serialization, plus pins the exact top-level key order.mcp_servers_serialize_in_insertion_order— inserts servers in non-sorted order, runsinto_wire, and asserts the wire-JSON key positions follow insertion order.Validation
From
rust/:cargo build --all-features— okcargo test --lib --all-features— 181 passedcargo test --doc --all-features— 23 passedcargo clippy --all-features --all-targets -- -D warnings— cleancargo +nightly-2026-04-14 fmt --check— cleanPublic field/param/return types change from
HashMaptoIndexMap(Tool.parameters, the threemcp_serversfields + setters, andtool_parameters/try_tool_parameters). Migration is mechanical —IndexMapmirrorsHashMap's API and is re-exported asgithub_copilot_sdk::IndexMap, so most consumers just swap the type name.Generated by Copilot