Skip to content

feat: relax outputSchema to accept non-object JSON Schema types (SEP-2106)#895

Open
branben wants to merge 3 commits into
modelcontextprotocol:mainfrom
branben:feat/sep-2106-output-schema-non-object
Open

feat: relax outputSchema to accept non-object JSON Schema types (SEP-2106)#895
branben wants to merge 3 commits into
modelcontextprotocol:mainfrom
branben:feat/sep-2106-output-schema-non-object

Conversation

@branben

@branben branben commented Jun 6, 2026

Copy link
Copy Markdown

Summary

Relax schema_for_output to accept any JSON Schema 2020-12 root type (arrays, primitives, compositions) for outputSchema, while keeping schema_for_input enforcing type: "object". This implements SEP-2106, which was accepted May 18 2026.

PR #860 (merged Jun 2 2026) added title/desc stripping and input validation. This PR covers the remaining work: decoupling the type gate so output schemas are no longer rejected for non-object types.

Changes

  • crates/rmcp/src/handler/server/common.rs: Split validate_and_strip into:
    • validate_and_strip_input — keeps type: "object" check for inputSchema
    • validate_and_strip_output — strips title/desc, no type check (accepts any JSON Schema root type)
  • crates/rmcp/src/model/tool.rs: Updated with_output_schema doc comment to remove incorrect "root type object" panic reference
  • Tests: 17 new tests across 4 test files covering primitives, arrays, compositions (Option<T>), unit type, Json<T> macro path, ToolBase trait path, cache correctness, and negative tests for input rejection

Backward Compatibility

  • schema_for_output return type unchanged (Result<Arc<JsonObject>, String>)
  • All existing tests for object output types still pass
  • Change is strictly relaxing: types that were previously rejected are now accepted

Verification

  • cargo test -p rmcp --all-features: 424 passed, 0 failed
  • cargo clippy --all-targets --all-features -- -D warnings: No issues found

@branben branben requested a review from a team as a code owner June 6, 2026 18:39
@github-actions github-actions Bot added T-test Testing related changes T-core Core library changes T-handler Handler implementation changes T-model Model/data structure changes labels Jun 6, 2026
@michaelneale

Copy link
Copy Markdown
Contributor

thanks @branben if you can run a fmt over this to get CI happy - I think this is then good to merge, thanks!

@branben

branben commented Jun 10, 2026

Copy link
Copy Markdown
Author

thanks @branben if you can run a fmt over this to get CI happy - I think this is then good to merge, thanks!

just checked on this @michaelneale happy I could contribute!

@DaleSeo DaleSeo linked an issue Jun 23, 2026 that may be closed by this pull request
4 tasks

@DaleSeo DaleSeo left a comment

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.

Thanks for implementing the sep, @branben! This looks good overall. I have some minor comments.

Comment thread crates/rmcp/src/handler/server/common.rs Outdated
Comment thread crates/rmcp/src/model/tool.rs Outdated
Comment thread crates/rmcp/src/handler/server/common.rs Outdated
assert!(tool.output_schema.is_some());

let schema_str = serde_json::to_string(tool.output_schema.as_ref().unwrap()).unwrap();
assert!(schema_str.contains("\"type\":\"integer\""));

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.

To prevent false positives

Suggested change
assert!(schema_str.contains("\"type\":\"integer\""));
assert_eq!(schema.get("type"), Some(&serde_json::json!("integer")));

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.

Fixed in latest push. ✅

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.

@branben You marked this as resolved but it still doesn't seem to be fixed!

@branben

branben commented Jun 29, 2026

Copy link
Copy Markdown
Author

Thanks for the review @DaleSeo! All 4 comments addressed in the latest push:

  1. strip_output naming (line 78) — Added strip_output() that strips title/description without validation, per your suggestion.
  2. Dead panic path (line 321)schema_for_output now returns Arc<JsonObject> directly (no Result). The unwrap_or_else panic path is removed from with_output_schema, ToolBase::output_schema(), and the macros.
  3. Cache success only (line 125) — Cache changed to HashMap<TypeId, Arc<JsonObject>> — success values only, as suggested.
  4. Test false positives (line 72) — Tightened to assert_eq!(schema.get("type"), Some(&serde_json::json!("object"))) to prevent false positives.

The test_schema_for_output_rejects_primitive test was also updated to test_schema_for_output_accepts_primitive since non-object output schemas are now valid per SEP-2106.

Please let me know where else I can make meaningful contributions if there's a specific pain point that's bugging the team!

@DaleSeo

DaleSeo commented Jun 29, 2026

Copy link
Copy Markdown
Member

@branben You might've forgotten to push those fixes? I'm still seeing the original code. 😅
Could you double-check it? Happy to re-review as soon as it shows up.

@branben branben force-pushed the feat/sep-2106-output-schema-non-object branch from 47e5d23 to 6588978 Compare June 29, 2026 21:19
@github-actions github-actions Bot added the T-macros Macro changes label Jun 29, 2026
Brandon Bennett and others added 3 commits June 29, 2026 15:19
…urns Result

- Add strip_output() that strips title/description without validating type (Dale modelcontextprotocol#1)
- Change schema_for_output to return Arc<JsonObject> instead of Result (Dale modelcontextprotocol#2)
- Cache only Arc<JsonObject> success values, not Result (Dale modelcontextprotocol#3)
- Remove dead unwrap_or_else panic paths in with_output_schema, ToolBase, and macros
- Tighten test assertions from contains to assert_eq on type field (Dale modelcontextprotocol#4)
- Update test_schema_for_output_rejects_primitive to accept_primitive (SEP-2106)

Co-authored-by: Orca <help@stably.ai>
Add tests verifying schema_for_output accepts non-object types:
- test_tool_builder_methods: primitive (i32), array (Vec<String>), option
- test_structured_output: tool returning Json<Vec<T>> and Json<i32>
- test_json_schema_detection: Json<Vec<T>>, Result<Json<Vec<T>>,E>, Json<String>
- tool_traits: ToolBase::output_schema with Vec<AddOutput> output type
Add tests identified during code review:
- description stripping for primitive types
- composition types (Option<String> with anyOf/oneOf/null)
- cache correctness (Arc::ptr_eq for repeated calls)
- schema_for_input rejecting array types (not just primitives)
- schema_for_output accepting unit type ()
@branben branben force-pushed the feat/sep-2106-output-schema-non-object branch from 6588978 to 28c2bcc Compare June 29, 2026 22:34
@branben

branben commented Jun 30, 2026

Copy link
Copy Markdown
Author

@branben You might've forgotten to push those fixes? I'm still seeing the original code. 😅 Could you double-check it? Happy to re-review as soon as it shows up.

sorry about this @DaleSeo, could you check now and let me know if it's showing the fixes? Thank you again!

assert!(tool.output_schema.is_some());

let schema_str = serde_json::to_string(tool.output_schema.as_ref().unwrap()).unwrap();
assert!(schema_str.contains("\"type\":\"integer\""));

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.

@branben You marked this as resolved but it still doesn't seem to be fixed!

Comment on lines +417 to +418
let schema_str = serde_json::to_string(schema).unwrap();
assert!(schema_str.contains("integer"));

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.

Suggested change
let schema_str = serde_json::to_string(schema).unwrap();
assert!(schema_str.contains("integer"));
assert_eq!(schema.get("type"), Some(&serde_json::json!("integer")));

Comment on lines +376 to +380
let schema_value: serde_json::Value = serde_json::from_str(
&serde_json::to_string(&*schema).expect("failed to serialize schema"),
)
.expect("failed to parse schema JSON");
assert_eq!(schema_value["type"], "array");

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.

schema is already an Arc<JsonObject> so we can read the key directly.

Suggested change
let schema_value: serde_json::Value = serde_json::from_str(
&serde_json::to_string(&*schema).expect("failed to serialize schema"),
)
.expect("failed to parse schema JSON");
assert_eq!(schema_value["type"], "array");
assert_eq!(schema.get("type"), Some(&serde_json::json!("array")));

Comment on lines +354 to +369
impl SyncTool<TraitBasedToolServer> for ArrayTool {
fn invoke(
_service: &TraitBasedToolServer,
_param: Self::Parameter,
) -> Result<Self::Output, Self::Error> {
Ok(vec![])
}
}
impl AsyncTool<TraitBasedToolServer> for ArrayTool {
async fn invoke(
_service: &TraitBasedToolServer,
_param: Self::Parameter,
) -> Result<Self::Output, Self::Error> {
Ok(vec![])
}
}

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.

What are these for?


/// Generate and strip a JSON schema for outputSchema (top-level "title" and
/// "description" are removed; output schemas are not restricted to root type "object").
pub fn schema_for_output<T: JsonSchema + std::any::Any>() -> Arc<JsonObject> {

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.

Public API Check is red in CI because the schema_for_output return-type change is a breaking change:

-pub fn ...::schema_for_output<...>() -> Result<Arc<JsonObject>, String>
+pub fn ...::schema_for_output<...>() -> Arc<JsonObject>
Error: The API diff is not allowed as per --deny: Changed items not allowed

That's expected and fine, and the plan is to batch all 2026-07-28 spec breaking changes into v3.0. So this SEP-2106 implementation belongs in that major.

We need to mark the introducing commit breaking though replacing feat: with feat!: so the CI gate computes major and Public API Check passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2026-07-28 T-core Core library changes T-handler Handler implementation changes T-macros Macro 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.

Implement SEP-2106: Tool schemas conform to JSON Schema 2020-12

3 participants