Skip to content

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

Open
nightcityblade wants to merge 1 commit into
modelcontextprotocol:mainfrom
nightcityblade:fix/issue-1109
Open

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

Conversation

@nightcityblade

@nightcityblade nightcityblade commented Aug 7, 2026

Copy link
Copy Markdown

What Problem This Solves

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

Why This Change Was Made

The public properties field, constructor, and builder remain BTreeMap-based to avoid a breaking API change. Deserialization now uses an internal IndexMap wire representation to capture encountered property names in a new serde-skipped property_order: Option<Vec<String>> field. IndexMap is therefore not exposed in the public API.

User Impact

Clients can render deserialized elicitation fields in server-declared order by consulting property_order, while existing code using properties remains source-compatible within 3.x.

Evidence

  • cargo test -p rmcp --all-features --lib test_legacy_enum_schema_roundtrip_preserves_enum_names — passed
  • cargo +nightly fmt --all -- --check — passed

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
Comment thread crates/rmcp/src/model/elicitation_schema.rs Outdated
@github-actions github-actions Bot added the T-test Testing related changes label Aug 8, 2026
@DaleSeo

DaleSeo commented Aug 8, 2026

Copy link
Copy Markdown
Member

@nightcityblade Thanks for addressing my comment. The no-local test fix looks good.

Sorry to bring this up late, but I noticed the ! in the commit message and realized this is a breaking change. We just cut v3, so merging this would make release-plz publish v4. It would also make indexmap a public dependency.

Could we fix the ordering within 3.x instead? I think we could keep properties: BTreeMap and store the wire order as explicit metadata, like this:

pub struct ElicitationSchema {
    // ...
    pub property_order: Option<Vec<String>>,
}

Let me know if this would work as a fix.

@github-actions github-actions Bot removed the T-test Testing related changes label Aug 9, 2026
@nightcityblade

Copy link
Copy Markdown
Author

Thanks for the suggestion — the metadata approach works without changing the public properties type.

I updated the PR so that:

  • ElicitationSchema::properties, its constructor, and the builder remain BTreeMap-based.
  • Deserialization captures the encountered keys in a new serde-skipped property_order field.
  • IndexMap is used only inside the private wire representation, so it is not exposed in the public API.
  • The branch is now a single non-breaking commit, removing the commit-lint failure caused by the earlier ! marker.

Validation passed with cargo test -p rmcp --all-features --lib test_legacy_enum_schema_roundtrip_preserves_enum_names and rustfmt checking. Could you take another look?

@nightcityblade
nightcityblade force-pushed the fix/issue-1109 branch 2 times, most recently from a3663f4 to c240e6f Compare August 9, 2026 15:27
@nightcityblade

Copy link
Copy Markdown
Author

Follow-up on the CI failures: the current head (c240e6f) keeps derived equality intact and initializes property_order consistently in the constructor and builder, so existing serialization round trips remain equal. The handwritten Deserialize signature also now matches the previous derived public signature.

Local validation passed:

  • cargo test -p rmcp --all-features --test test_elicitation — 56 passed
  • cargo test -p rmcp --all-features --lib model::elicitation_schema::tests — 27 passed
  • nightly rustfmt check
  • cargo public-api against the current base with default features and with all non-local features — no removed or changed items; only the new property_order field is added

The refreshed fork workflows are currently awaiting GitHub Actions approval.

/// .build();
/// ```
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
#[derive(Debug, Clone, PartialEq, Serialize)]

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.

Serialize is still derived for ElicitationSchema, so properties: BTreeMap<...> gets serialized alphabetically. That means the round-trip and proxy/relay parts of the issue are still unresolved. When property_order is available, serialization should follow it and then append any properties that are not listed.

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, you're right — deserialization captured the order, but derived serialization still sorted the BTreeMap.

The current head (297deb9) now converts through the private wire representation on serialization, rebuilding its IndexMap from property_order (and appending any newly added properties). The regression test asserts that a deserialize/serialize round trip preserves the exact property order.

Local validation passed:

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

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

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