Skip to content

fix(model): preserve elicitation property order - #1150

Open
nightcityblade wants to merge 2 commits into
modelcontextprotocol:mainfrom
nightcityblade:fix/issue-1109
Open

fix(model): preserve elicitation property order#1150
nightcityblade wants to merge 2 commits into
modelcontextprotocol:mainfrom
nightcityblade:fix/issue-1109

Conversation

@nightcityblade

Copy link
Copy Markdown

What Problem This Solves

ElicitationSchema.properties used a BTreeMap, so deserializing a server-declared form reordered its fields alphabetically. Clients could not recover the presentation order from the typed model.

Why This Change Was Made

The schema and builder now use IndexMap, whose Serde implementation preserves encounter and insertion order. The schemars dependency enables its indexmap2 integration so all-feature builds continue to derive JSON Schema successfully.

User Impact

Elicitation forms can render fields in the order declared by the server, and serializing the typed schema retains that order.

Evidence

  • cargo test -p rmcp --all-features model::elicitation_schema::tests — 28 passed
  • cargo clippy -p rmcp --all-targets --all-features -- -D warnings
  • cargo +nightly fmt --all -- --check

Fixes #1109

@nightcityblade
nightcityblade requested a review from a team as a code owner August 7, 2026 15:31
@github-actions github-actions Bot added T-dependencies Dependencies related changes T-config Configuration file changes T-core Core library changes T-model Model/data structure changes labels Aug 7, 2026
impl ElicitationSchema {
/// Create a new elicitation schema with the given properties
pub fn new(properties: BTreeMap<String, PrimitiveSchemaDefinition>) -> Self {
pub fn new(properties: IndexMap<String, PrimitiveSchemaDefinition>) -> Self {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The no-local integration test still calls ElicitationSchema::new(BTreeMap::new()), so this parameter change prevents that target from compiling.

@nightcityblade nightcityblade Aug 8, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. I updated the no-local integration test to construct an IndexMap and ran the exact no-local CI test command; it passes locally. The current head is f85271d.

@github-actions github-actions Bot added the T-test Testing related changes label Aug 8, 2026
nightcityblade added 2 commits August 8, 2026 11:14
BREAKING CHANGE: ElicitationSchema properties and ElicitationSchemaBuilder properties now use IndexMap instead of BTreeMap.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-config Configuration file changes T-core Core library changes T-dependencies Dependencies related changes T-model Model/data structure changes T-test Testing related changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Elicitation schema properties uses BTreeMap, discarding the server's declared field order

2 participants