[passionworkeer]

Fixes #1454

Why

EnumSchemaBuilder<MultiSelect> rejects inconsistent elicitation schemas, but
whether it does so depends on the order of the setter calls. with_default()
checks the number of default values against min_items / max_items;
min_items() and max_items() never check a default that was already set.

So .max_items(1)?.with_default(["a", "b"])? is rejected, while
.with_default(["a", "b"])?.max_items(1)? succeeds and build() emits
{"maxItems":1,"default":["a","b"],...}. A client pre-filling the form from
default immediately exceeds maxItems; minItems has the mirror case with a
short default. The asymmetry dates back to e9029cc (#539, SEP-1330), which
introduced both with_default's checks and the two bounds setters.

What changed

min_items() and max_items() now validate the already-set default against the
new bound, reusing the same error strings as with_default(). build() is
unchanged: it returns EnumSchema, not Result, so validating there would be a
breaking API change. For single-pass chains, the only ones whose behavior
changes are those that previously produced a self-contradictory schema.

An empty default means "no default". build() already omits it, and now none
of the three setters checks it against the bounds, so both orders are accepted
and emit the same schema. A non-empty default is checked in every setter order.

The setters consume the builder, so an Err ends the chain: a later call cannot
recover a value an intermediate setter rejected. Configure bounds before a
default that violates them. Public API and generated schemas are unchanged.

Scope notes

• TitledMultiSelectEnumSchema's infallible with_min_items / with_max_items
  / with_default setters can still build the same contradiction. They are
  plain struct setters on the schema type, not the validating builder; adding
  a check there needs a breaking -> Result change and is out of scope.
• The new tests cover both bounds directions in both orders on the untitled
  branch, pin the error strings, and cover an empty default below min_items
  in both orders. They do not exercise the titled branch.

Tests

Two unit tests in model::elicitation_schema::tests:

• test_enum_schema_multi_select_default_bounds_are_order_independent covers
  above-max and below-min defaults in both setter orders. It fails on unmodified
  main (1 failed / 1 passed) and passes here (2 passed).
• test_enum_schema_multi_select_default_within_bounds_is_accepted_in_any_order
  is a guard that an in-bounds default stays accepted in either order, so the
  fix cannot over-reject.

The red/green comparison ran the identical test code in a detached worktree at
main with only the tests added; the two worktrees differ only in the
production hunk.

Checked locally on the final commit: cargo test -p rmcp --lib (286 passed),
cargo test -p rmcp --doc (38 passed, 7 ignored),
cargo test -p rmcp --lib --no-default-features (196 passed),
cargo test -p rmcp --all-features --lib (844 passed),
cargo +nightly fmt --all -- --check, typos,
cargo clippy --all-targets --all-features -- -D warnings,
RUSTDOCFLAGS=-Dwarnings cargo doc -p rmcp --all-features, and the declared MSRV
(1.88.0) cargo check --all-features plus the regression test: all clean.

Upstream CI, cargo-semver-checks / cargo-public-api, and the full-workspace
cargo test --all-features have not been run locally.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[chatgpt-codex-connector]

Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-14T14:54:00.554196Z	18bdebe	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[github-actions]

@jokemanfire is on triage rotation this week and is the reviewer for this one.

Labels come from the PR labeler. Please review the change.

_{Automated triage. Labels are a first pass and the assigned maintainer has the final say.}

[jokemanfire]

Verified at head 9c3af5c (base c644b75). The fix is the right shape and the new test fails without it, so this is a comment on completeness, not on correctness.

What I ran

• cargo test -p rmcp --lib --features client -- enum_schema_multi_select -> 3 passed (..._default_bounds_are_order_independent, ..._within_bounds_is_accepted_in_any_order, existing ..._with_default).
• Red before green: removed only the two new guards (keeping your tests), rebuilt -> test_enum_schema_multi_select_default_bounds_are_order_independent fails at the default-first assertion ("a default with more entries than max_items must be rejected regardless of the order..."), while the within-bounds case still passes. So the test pins the bug rather than the implementation.
• Coverage of the state space is now complete for the pairwise checks: with_default validates against existing bounds, both bounds validate against an existing default, and min-vs-max is checked in both directions.
• CI at this SHA: 24 success / 1 neutral / 3 skipped / 0 failure.

I agree with leaving TitledMultiSelectEnumSchema's infallible setters alone; the issue calls that a separate question.

One remaining order dependence (same class as the bug)

The empty default is treated as "absent" in the two new guards but not in with_default:

// Ok, builds {"type":"array","minItems":1,...} with no default
EnumSchema::builder(v).multiselect().with_default(vec![]).and_then(|b| b.min_items(1));
// Err("Number of provided default values is less than min_items")
EnumSchema::builder(v).multiselect().min_items(1).and_then(|b| b.with_default(vec![]));

Both end at the same schema because build() drops an empty default, but only one order is accepted. Either reject with_default(vec![]) when min_items is set (0 < min is checked there already once a min exists) and document that an empty default means "no default", or treat it as absent in with_default too — but pick one and say which in the docs. A test for the accepted pair would keep a later refactor from silently deciding the other way.

Nits

1. The new tests assert is_err() only, so swapping which guard fires — or rewording to a different message — still passes. assert_eq!(err.as_deref(), Some("Number of provided default values is greater than max_items")) pins the contract the issue quotes.
2. min_items/max_items consume the builder on Err, so the "transient inconsistent state" chain the issue calls out (.with_default(v).max_items(1).max_items(3)) is terminal now, not transient. One line on those setters ("configure bounds before a default that violates them; the error ends the chain") makes that limitation discoverable instead of surprising.

Not verified

I did not run the full --all-features suite; this is a single-file, client-feature change, and the CI matrix already covers the rest.

[passionworkeer]

Addressed in 18bdebe.

An empty default means "no default": build() already omits it, and with_default, min_items, and max_items now all skip it, so both orders are accepted and emit the same schema. A non-empty default is still checked in every order, and the bounds tests now pin the error strings. The setter docs note that these methods consume the builder, so an Err ends the chain.