diff --git a/flicknote-cli/src/main_tests/mcp.rs b/flicknote-cli/src/main_tests/mcp.rs index 4bd19da..02cca88 100644 --- a/flicknote-cli/src/main_tests/mcp.rs +++ b/flicknote-cli/src/main_tests/mcp.rs @@ -346,6 +346,229 @@ async fn mcp_server_exposes_stable_tool_contract() { assert!(project_get["inputSchema"]["properties"].get("id").is_none()); } +/// Fail if any schema position holds a bare boolean schema (e.g. +/// `"metadata": true`). +/// +/// JSON Schema itself allows booleans as schema shorthand — `true` accepts +/// anything, `additionalProperties: false` forbids extra keys. This test is a +/// proxy for strict MCP clients that reject boolean schema terms, so every +/// schema position (including `additionalProperties` and +/// `unevaluatedProperties`) must stay an object-form term. The walk covers the +/// subschema keywords of JSON Schema 2020-12, skipping non-schema keywords +/// such as `required`, `enum`, and `type`. +fn assert_no_boolean_schema_terms(schema: &serde_json::Value, tool: &str) { + match schema { + serde_json::Value::Bool(_) => { + panic!("tool {tool}: outputSchema contains a boolean schema term") + } + serde_json::Value::Object(map) => { + for (key, value) in map { + let is_named_schemas = matches!( + key.as_str(), + "properties" + | "patternProperties" + | "$defs" + | "definitions" + | "dependentSchemas" + ); + let is_schema_list = + matches!(key.as_str(), "allOf" | "anyOf" | "oneOf" | "prefixItems"); + let is_single_schema = matches!( + key.as_str(), + "items" + | "additionalProperties" + | "unevaluatedProperties" + | "propertyNames" + | "contains" + | "not" + | "if" + | "then" + | "else" + | "unevaluatedItems" + | "contentSchema" + ); + if is_named_schemas { + for subschema in value.as_object().unwrap().values() { + assert_no_boolean_schema_terms(subschema, tool); + } + } else if is_schema_list { + for subschema in value.as_array().unwrap() { + assert_no_boolean_schema_terms(subschema, tool); + } + } else if is_single_schema { + assert_no_boolean_schema_terms(value, tool); + } + } + } + _ => {} + } +} + +#[tokio::test] +async fn mcp_tool_output_schemas_are_strict_client_compatible() { + let mut harness = McpHarness::start().await; + let tools = harness.tools().await; + for tool in &tools { + let name = tool["name"].as_str().unwrap(); + let output = &tool["outputSchema"]; + assert_eq!( + output["type"], + serde_json::json!("object"), + "tool {name}: outputSchema root type must be object" + ); + assert_no_boolean_schema_terms(output, name); + } + + let list = tools + .iter() + .find(|tool| tool["name"] == "note_list") + .unwrap(); + assert!( + list["outputSchema"]["properties"]["notes"]["items"].is_object(), + "note_list outputSchema must advertise the notes array item schema" + ); + let projects = tools + .iter() + .find(|tool| tool["name"] == "project_list") + .unwrap(); + assert!( + projects["outputSchema"]["properties"]["projects"]["items"].is_object(), + "project_list outputSchema must advertise the projects array item schema" + ); +} + +#[tokio::test] +async fn mcp_note_get_output_schema_advertises_detail_structure() { + let mut harness = McpHarness::start().await; + let tools = harness.tools().await; + let note_get = tools + .iter() + .find(|tool| tool["name"] == "note_get") + .unwrap(); + let output = ¬e_get["outputSchema"]; + let properties = output["properties"].as_object().unwrap(); + for required_property in ["content", "metadata", "extractions", "sections"] { + assert!( + properties.contains_key(required_property), + "note_get outputSchema must advertise {required_property}" + ); + } + let required = output["required"].as_array().unwrap(); + for required_field in ["content", "extractions", "sections"] { + assert!( + required.iter().any(|field| field == required_field), + "note_get outputSchema must require {required_field}" + ); + } + let metadata = &properties["metadata"]; + assert!( + metadata.is_object(), + "note_get metadata schema must be an object-form term, got {metadata}" + ); + let metadata_types = metadata["type"].as_array().unwrap(); + for json_type in ["null", "object", "array", "string", "number", "boolean"] { + assert!( + metadata_types.iter().any(|allowed| allowed == json_type), + "note_get metadata schema must represent arbitrary JSON including {json_type}" + ); + } +} + +#[tokio::test] +async fn mcp_note_source_output_schema_advertises_all_views() { + let mut harness = McpHarness::start().await; + let tools = harness.tools().await; + let note_source = tools + .iter() + .find(|tool| tool["name"] == "note_source") + .unwrap(); + let output = ¬e_source["outputSchema"]; + assert_eq!( + output["type"], + serde_json::json!("object"), + "note_source outputSchema root type must be object" + ); + let variants = output["oneOf"].as_array().unwrap(); + assert_eq!( + variants.len(), + 3, + "note_source must advertise rendered, raw, and info variants" + ); + let mut views = BTreeSet::new(); + let mut rendered_fields = BTreeSet::new(); + let mut raw_value_types = Vec::new(); + let mut info_fields = BTreeSet::new(); + for variant in variants { + let view_schema = &variant["properties"]["view"]; + let view = view_schema["const"] + .as_str() + .or_else(|| { + view_schema["enum"] + .as_array() + .and_then(|values| values.first()) + .and_then(|value| value.as_str()) + }) + .unwrap_or_else(|| { + panic!("variant must declare its view discriminator via const or enum") + }); + views.insert(view.to_string()); + assert!( + variant["required"] + .as_array() + .unwrap() + .iter() + .any(|field| field == "view"), + "{view} variant must retain the view discriminator in required" + ); + let fields: BTreeSet = variant["properties"] + .as_object() + .unwrap() + .keys() + .cloned() + .collect(); + match view { + "rendered" => rendered_fields = fields, + "raw" => { + raw_value_types = variant["properties"]["value"]["type"] + .as_array() + .unwrap() + .clone() + } + "info" => info_fields = fields, + other => panic!("unexpected source view {other}"), + } + } + assert_eq!( + views, + BTreeSet::from(["rendered", "raw", "info"].map(String::from)) + ); + for field in [ + "view", + "source_type", + "range_unit", + "total_count", + "selected_start", + "selected_end", + "content", + ] { + assert!( + rendered_fields.contains(field), + "rendered variant must advertise {field}" + ); + } + assert!( + raw_value_types.contains(&serde_json::json!("null")) + && raw_value_types.contains(&serde_json::json!("object")), + "raw variant value must represent arbitrary JSON including null" + ); + for field in ["view", "source_type", "range_unit", "count"] { + assert!( + info_fields.contains(field), + "info variant must advertise {field}" + ); + } +} + #[tokio::test] async fn mcp_note_queries_use_short_ids_and_hide_uuid() { let mut harness = McpHarness::start().await; diff --git a/flicknote-cli/src/mcp/dto.rs b/flicknote-cli/src/mcp/dto.rs index 9bee3b2..ea2b81c 100644 --- a/flicknote-cli/src/mcp/dto.rs +++ b/flicknote-cli/src/mcp/dto.rs @@ -1,8 +1,13 @@ +use std::sync::Arc; + use flicknote_core::services::dto::{ ExtractionDto, NoteArchiveResult, NoteDetail, NoteMutationResult, NoteSummary, ProjectDto, SectionDto, }; -use rmcp::schemars::JsonSchema; +use flicknote_core::services::source::SourceResult; +use rmcp::handler::server::tool::schema_for_output; +use rmcp::model::JsonObject; +use rmcp::schemars::{JsonSchema, Schema, SchemaGenerator}; use serde::Serialize; #[derive(Debug, Serialize, JsonSchema)] @@ -54,6 +59,7 @@ pub(super) struct McpNoteDetail { #[serde(flatten)] pub note: McpNoteSummary, pub content: String, + #[schemars(schema_with = "arbitrary_json_schema")] pub metadata: Option, pub extractions: Vec, pub sections: Vec, @@ -71,6 +77,99 @@ impl From for McpNoteDetail { } } +/// Every JSON value type plus `null` (for `Option` fields). Used wherever +/// `serde_json::Value` appears so the schema stays an object-form term. +const ARBITRARY_JSON_TYPES: [&str; 7] = [ + "array", "boolean", "integer", "null", "number", "object", "string", +]; + +/// Object-form JSON Schema term for arbitrary JSON values. +/// +/// `serde_json::Value` derives a bare boolean schema term (`true`) that strict +/// MCP clients reject. This term keeps arbitrary values unconstrained while +/// remaining a parseable JSON Schema object. +fn arbitrary_json_schema(_generator: &mut SchemaGenerator) -> Schema { + serde_json::from_value(serde_json::json!({ "type": ARBITRARY_JSON_TYPES })) + .expect("arbitrary-json schema is valid JSON Schema") +} + +/// MCP boundary result for source queries. +/// +/// Mirrors `SourceResult`'s `view`-tagged variants so that the advertised +/// output schema and the serialized `structuredContent` evolve together. The +/// raw `value` keeps an object-form arbitrary-JSON term (its only schema +/// customization); the explicit object root is applied by +/// [`source_output_schema`]. +#[derive(Debug, Serialize, JsonSchema)] +#[serde(tag = "view", rename_all = "snake_case")] +pub(super) enum McpSourceResult { + Rendered { + source_type: String, + range_unit: String, + total_count: usize, + selected_start: usize, + selected_end: usize, + content: String, + }, + Raw { + source_type: String, + #[schemars(schema_with = "arbitrary_json_schema")] + value: serde_json::Value, + }, + Info { + source_type: String, + range_unit: String, + count: usize, + }, +} + +impl From for McpSourceResult { + fn from(result: SourceResult) -> Self { + match result { + SourceResult::Rendered { + source_type, + range_unit, + total_count, + selected_start, + selected_end, + content, + } => Self::Rendered { + source_type, + range_unit, + total_count, + selected_start, + selected_end, + content, + }, + SourceResult::Raw { source_type, value } => Self::Raw { source_type, value }, + SourceResult::Info { + source_type, + range_unit, + count, + } => Self::Info { + source_type, + range_unit, + count, + }, + } + } +} + +/// Object-rooted output schema for `McpSourceResult`. +/// +/// Derives the union from the boundary DTO (so schema and serialization stay +/// in sync) and applies the single local compatibility fix: internally tagged +/// enum unions derive without an explicit root `type`, which strict MCP +/// clients reject. +pub(super) fn source_output_schema() -> Arc { + let mut schema = (*schema_for_output::()).clone(); + schema.insert( + "type".to_string(), + serde_json::Value::String("object".to_string()), + ); + Arc::new(schema) +} + #[derive(Debug, Serialize, JsonSchema)] pub(super) struct McpNoteMutationResult { pub note: McpNoteSummary, diff --git a/flicknote-cli/src/mcp/server.rs b/flicknote-cli/src/mcp/server.rs index 67907d8..bc7e107 100644 --- a/flicknote-cli/src/mcp/server.rs +++ b/flicknote-cli/src/mcp/server.rs @@ -13,14 +13,14 @@ use flicknote_core::services::source::SourceResult; use flicknote_sync::ipc::{AppRequest, AppResult, DaemonClient}; use rmcp::handler::server::router::tool::ToolRouter; use rmcp::handler::server::wrapper::Parameters; -use rmcp::model::{CallToolResult, Implementation, JsonObject, ServerCapabilities, ServerInfo}; +use rmcp::model::{CallToolResult, Implementation, ServerCapabilities, ServerInfo}; use rmcp::schemars::JsonSchema; use rmcp::{Json, ServerHandler, ServiceExt, tool, tool_handler, tool_router}; use serde::Serialize; use super::dto::{ McpNoteArchiveResult, McpNoteDetail, McpNoteListResult, McpNoteMutationResult, McpNoteSummary, - McpProjectDto, McpProjectListResult, + McpProjectDto, McpProjectListResult, McpSourceResult, source_output_schema, }; use super::error::tool_error; use super::note_tools::*; @@ -100,22 +100,6 @@ fn structured(result: Result) -> Result, CallToolRes result.map(Json).map_err(|error| tool_error(&error)) } -/// Minimal, spec-compliant output schema (`type: "object"`). -/// -/// MCP 2025-era clients validate `outputSchema.type` strictly and reject root -/// types other than `"object"` (e.g. the `"array"` schemas derived from list -/// return types, or the `oneOf` union derived from `SourceResult`). Each tool's -/// description documents the actual result shape, so this declaration stays -/// valid for strict clients without inventing structure. -fn object_output_schema() -> Arc { - let mut schema = JsonObject::new(); - schema.insert( - "type".to_string(), - serde_json::Value::String("object".to_string()), - ); - Arc::new(schema) -} - #[tool_router(router = tool_router)] impl FlickNoteMcp { #[tool( @@ -226,21 +210,22 @@ impl FlickNoteMcp { #[tool( name = "note_source", description = "Read stored source data as rendered content, raw JSON/text, or compact info. Normal notes often have no source data; use note_get for editable content. Use info then a 1-based range for large text or meeting sources.", - output_schema = object_output_schema(), + output_schema = source_output_schema(), annotations(read_only_hint = true) )] async fn note_source( &self, Parameters(params): Parameters, - ) -> Result, CallToolResult> { + ) -> Result, CallToolResult> { structured( - self.call(AppRequest::NoteSource { + self.call::(AppRequest::NoteSource { id: params.id.to_string(), archived: params.archived, view: params.view, range: params.range, }) - .await, + .await + .map(Into::into), ) }